feat(data-access)!: drop startedAt/result/error from Preflight schema (SITES-47254)

[GeezerPelletier]

Summary

Schema-layer half of SITES-47254. Drops the startedAt, result, and error attributes from the Preflight schema — they mirror columns on the preflights table that no longer exist after the mysticat-data-service migration.

⚠️ DO NOT MERGE before spacecat-api-service#2713 is deployed. Today's spacecat-api-service PreflightDto.toDetailJSON still calls preflight.getStartedAt() / getResult() / getError(). If this PR publishes a new major before #2713 deploys and any service picks up the bump, every GET /sites/:siteId/preflights/:preflightId request will throw TypeError: preflight.getStartedAt is not a function and return 500. The api-service migration to await preflight.getAsyncJob() already merged on the branch in #2713 — that PR just needs to deploy first.

After this change, lifecycle internals live only on the joined AsyncJob:

const preflight = await dataAccess.Preflight.findById(preflightId);
const asyncJob = await preflight.getAsyncJob();
asyncJob.getStartedAt();
asyncJob.getResult();
asyncJob.getError();

Preflight.status and Preflight.endedAt remain on the schema as a small denormalized cache for hot list-view reads — the projector (mysticat-projector-service#216) keeps them in sync.

Why the bump is a major (feat!)

Removing schema attributes removes the auto-generated getStartedAt() / getResult() / getError() and corresponding setters from the Preflight model. Any downstream consumer that calls those on Preflight will throw TypeError: preflight.getResult is not a function after upgrading.

Today the only known consumer is spacecat-api-service, and PR #2713 has already migrated getPreflightById to source those fields from the joined AsyncJob. No other usages found via grep across the workspace.

Changes

• src/models/preflight/preflight.schema.js — drop startedAt, result, error attributes. Updated header comment explaining the AsyncJob ownership.
• src/models/preflight/index.d.ts — drop the getStartedAt / getResult / getError and corresponding setter declarations; getEndedAt return is now string | undefined (not | null) to match the runtime — normalizeModelValue maps DB NULL → JS undefined.
• src/models/async-job/index.d.ts — add getStartedAt() / setStartedAt() / getEndedAt() / setEndedAt() declarations (runtime getters exist via the schema; the TS surface was missing them — this PR makes them load-bearing since consumers are now redirected to asyncJob.getStartedAt()). Return types use string | undefined for the same reason; header comment documents the convention.
• test/fixtures/preflights.fixture.js — strip the dropped fields from the three fixture rows; one-line comment explains where the lifecycle data lives now.
• test/unit/models/preflight/preflight.model.test.js — drop the three startedAt / result / error describe blocks and the corresponding fields from mockRecord.
• test/unit/models/preflight/preflight.collection.test.js — drop stale startedAt field from mockRecord (no test was failing because the schema discards unknown fields on read, but the residue misled future readers).
• test/unit/models/preflight/preflight.schema.test.js — drop the now-irrelevant error attribute validate-suite, and add a regression guard for SITES-47254 block asserting the three attributes are NOT declared (re-introduction fails CI here rather than only at downstream new electrodb.Service(...) time).

Verification

• Touched-file lint clean.
• npx mocha packages/spacecat-shared-data-access/test/unit/models/preflight/*.test.js packages/spacecat-shared-data-access/test/unit/models/async-job/*.test.js — 55 passing, 0 failing, including the new regression guards.
• npx mocha --timeout 30000 packages/.../electrodb-service-construction.test.js — passes; the cross-entity Service construction guard validates the full registry still constructs cleanly after the schema change.
• CI Test job — currently failing on an ECR image manifest issue (mysticat-data-service:v5.24.1 not found) that's unrelated to this PR. Tracking separately if it doesn't auto-resolve on the next CI run.

Deploy ordering

Step	What	Why
1	spacecat-api-service#2713 deploys	Stops calling preflight.getResult() / getError() / getStartedAt() — migrates to asyncJob.getXxx().
2	This PR merges + publishes	Major bump removes the now-unused getters from Preflight.
3	Consumers npm i @adobe/spacecat-shared-data-access@<new-major> when convenient	No functional change required in api-service (already on the new path post-#2713).

The data-service column drop (mysticat-data-service#746) is order-independent relative to this PR — it can land before or after; either ordering keeps the model and DB in sync (both stop carrying the columns).

Related

• SITES-47254 — ticket
• spacecat-api-service#2713 — read-path migrated off Preflight.getResult() / getError() / getStartedAt() onto the AsyncJob join
• mysticat-data-service#746 — column drop on the preflights table
• mysticat-projector-service#216 (SITES-47253) — projector PATCHes only {status, ended_at} onto preflights now
• SITES-45776 — original completion-path that introduced the duplication

🤖 Generated with Claude Code

[clotton]

This is a clean, well-scoped removal with good regression guards and thorough test cleanup. The schema change, the three absence assertions, and the comment trail are all correct. Two issues need attention before merge, and two smaller items are worth addressing.

Findings

1. BLOCKER — AsyncJob/index.d.ts is missing getStartedAt() / setStartedAt() — this PR makes them load-bearing

The updated schema comment and the PR description both redirect consumers to await preflight.getAsyncJob() then call asyncJob.getStartedAt(). The runtime getter exists (async-job.schema.js declares startedAt), but src/models/async-job/index.d.ts has no getStartedAt() or setStartedAt() declaration. Any TypeScript consumer following the migration guidance gets a compile error on asyncJob.getStartedAt(). This gap pre-existed the PR but becomes blocking the moment it is the only advertised path.

Fix: Add to src/models/async-job/index.d.ts:

getStartedAt(): string | undefined;
setStartedAt(startedAt: string): void;

2. MEDIUM — getEndedAt() return type should be string | undefined, not string | null

postgrest.utils.js::normalizeModelValue() converts DB null → undefined and skips setting the key (if (normalized !== undefined) { acc[...] = ... }). The auto-generated getter returns this.record['endedAt'], which is undefined (key absent) when the column is NULL in Postgres — never null. The declaration getEndedAt(): string | null is therefore wrong for the PostgREST v3 implementation. Callers doing getEndedAt() === null will silently miss the branch on the common unfinished-job case. (The deleted declarations getStartedAt(): string | null etc. had the same bug — those are gone, but getEndedAt() survives with it.)

Fix: Change line 24 of index.d.ts to getEndedAt(): string | undefined;

3. MEDIUM — Deploy-ordering dependency is real and the companion PR is still open

spacecat-api-service/src/dto/preflight.js lines 29, 31-32 still call preflight.getStartedAt(), preflight.getResult(), preflight.getError(). The PR description correctly identifies this and says "safe to land any time after spacecat-api-service#2713 deploys." However, #2713 is still open. If this schema bump is published to npm and picked up by the API service before #2713 merges, every GET /preflights/:id request that reaches toDetailJSON will throw TypeError: preflight.getStartedAt is not a function and return 500. The description could be hardened: make the dependency explicit in the PR body (e.g., "DO NOT merge before #2713 is deployed") or add a peerDependencies / package-level note.

4. MINOR — preflight.collection.test.js mockRecord still carries startedAt (missed cleanup)

The PR correctly cleaned preflights.fixture.js and preflight.model.test.js, but preflight.collection.test.js line 40 still has startedAt: '2025-06-01T10:00:01.000Z' in its local mockRecord. The schema silently discards unknown fields on read so no test fails, but the fixture misleads future readers into thinking startedAt is still a Preflight attribute.

Fix: Delete startedAt: '2025-06-01T10:00:01.000Z' from the collection test's mockRecord.

[clotton]

Return type should be string | undefined, not string | null.

postgrest.utils.js::normalizeModelValue() maps DB null → undefined and skips writing the key to this.record entirely. The auto-generated getter returns this.record['endedAt'], which is undefined (absent key) when ended_at is NULL — never null. Callers checking getEndedAt() === null will silently miss the "not yet ended" branch.

Change to:

getEndedAt(): string | undefined;

[clotton]

The PR description and this comment both direct consumers to asyncJob.getStartedAt(), but AsyncJob/index.d.ts has no getStartedAt() declaration.

The async-job.schema.js declares startedAt as an attribute (the runtime JS getter exists), but the TypeScript surface omits it. Any typed consumer following this migration guidance will get a compile error.

Please add to src/models/async-job/index.d.ts:

getStartedAt(): string | undefined;
setStartedAt(startedAt: string): void;

[MysticatBot]

Hey @GeezerPelletier,

⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.

Verdict: Approve - clean, well-scoped schema removal with proper regression guards and clear migration path documented.
Complexity: MEDIUM - medium diff size (schema attribute removal + corresponding test updates).
Changes: Drops startedAt/result/error attributes from the Preflight schema, centralizing lifecycle data on the joined AsyncJob entity (5 files).
Note: CI checks are currently failing - the integration test job cannot pull mysticat-data-service:v5.24.1 from ECR (image not found). This is an infrastructure issue, not a defect introduced by this PR. Unit tests and lint pass. Resolve the image availability before merge.

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 2m 6s | Cost: $4.63 | Commit: 8c0d42afd56bd51b62132390da07f5082dea82ad
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[github-actions]

This PR will trigger a patch release when merged.

[GeezerPelletier]

Thanks @clotton — all four findings addressed in a6deaea9 + a PR-description update.

Blocker #1 — Missing getStartedAt() / setStartedAt() on AsyncJob/index.d.ts

Real gap that this PR made load-bearing. The runtime getters exist (async-job.schema.js declares the attribute), but the TS surface omitted them — typed consumers following the migration guidance (await preflight.getAsyncJob() → asyncJob.getStartedAt()) would have hit a compile error. Added both:

getStartedAt(): string | undefined;
getEndedAt(): string | undefined;
setStartedAt(startedAt: string): void;
setEndedAt(endedAt: string): void;

Return types use string | undefined per your other finding — see below. Also added getEndedAt/setEndedAt for symmetry since those were silently missing from the TS surface too.

Medium #2 — Preflight.getEndedAt() return type string | null → string | undefined

Same root cause as the wire-contract bug you flagged on PR #2713 — normalizeModelValue maps DB NULL → JS undefined, never null. The remaining declaration was wrong. Fixed:

getEndedAt(): string | undefined;

Added a header comment on async-job/index.d.ts documenting the convention so future hands writing nullable-column declarations get it right by default; the Preflight type points back at it. (Existing pre-PR ... | null declarations on getResult / getError / getMetadata survive untouched — they're load-bearing legacy for downstream consumers, and tightening them would be a separate breaking change.)

Medium #3 — Deploy-ordering hardening

Updated the PR description with a prominent ⚠️ block at the top:

DO NOT MERGE before spacecat-api-service#2713 is deployed.

…and a deploy-ordering table at the bottom that explicitly walks the three steps (api-service deploys → this PR merges/publishes → consumers bump). The risk is real — a downstream service picking up the new major before #2713 deploys would 500 every preflight detail request.

Minor #4 — Stale startedAt in preflight.collection.test.js mockRecord

Removed. The schema's silent-discard-of-unknown-fields meant no test was failing, but you're right that it misled future readers.

CI Test failure (unrelated)

The failing job is the integration-test pull of mysticat-data-service:v5.24.1 from ECR returning manifest unknown: Requested image not found. That tag predates my changes (this PR doesn't touch the IT image reference), so it's environmental — likely an ECR retention prune. Mentioned it in the verification block of the updated PR description; if it doesn't auto-resolve on the next CI run I'll file a separate ticket to update the image reference.

State

• 55 unit tests passing across preflight + async-job suites.
• Lint clean on touched files.
• Re-requesting review.

[clotton]

All findings from the prior review resolved:

• AsyncJob/index.d.ts missing lifecycle declarations — getStartedAt(), getEndedAt(), setStartedAt(), setEndedAt() all added with correct string | undefined return types (not | null), matching the normalizeModelValue DB-null → undefined runtime contract. The header comment explaining why | undefined is correct is a useful guard against future === null mistakes.
• Preflight/index.d.ts wrong return type — getEndedAt() corrected to string | undefined; getStartedAt(), getResult(), getError() removed cleanly with a redirect comment to getAsyncJob().
• Stale startedAt in collection test — removed from the mockRecord.
• Schema regression guard — the new 'attribute surface' describe block pins that startedAt/result/error are absent from the Preflight schema, preventing silent re-introduction.

LGTM.