Skip to content

fix(metadata): emit the declared ISO string at the five adapter boundaries that cast a driver Date - #14939

Merged
os-musk merged 3 commits into
mainfrom
claude/issue-14037-adapter-boundary-date-to-iso
Sep 3, 2026
Merged

fix(metadata): emit the declared ISO string at the five adapter boundaries that cast a driver Date#14939
os-musk merged 3 commits into
mainfrom
claude/issue-14037-adapter-boundary-date-to-iso

Conversation

@os-musk

@os-musk os-musk commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Fixes #14037

Five metadata adapter boundaries asserted as string over a driver timestamp column that the live dialects hand out as a JS Date. They now emit the ISO-8601 string their declared type promises, at the producer.

The sharp edge, first

Three of the five land in fields declared z.string().datetime(), not z.string(). That is not "the type is imprecise" — it is a refinement that rejects outright. The only reason nothing is red today is that no production path parses these values: rowToRecord's output is consumed as an already-typed MetadataRecord throughout database-loader.ts with nothing revalidating it. The day anyone adds a .parse() on one of these paths, the production default driver fails it. Measured during this card: no such parse exists yet, so the p2 grade stands and no severity re-grade is owed.

The five sites, re-verified on origin/main 431979e67

The card's line numbers had drifted; the sites themselves are exactly the five it names.

site (before) expression declared as
metadata-protocol/src/sys-metadata-repository.ts:1157 (rowToEvent) ts: (row.recorded_at as string) ?? new Date(0).toISOString() MetadataEvent.tsz.string(), metadata-core/src/types.ts:147
metadata/src/loaders/database-loader.ts:746 (rowToRecord) createdAt: row.created_at as string | undefined MetadataRecord.createdAtz.string().datetime(), spec/src/system/metadata-persistence.zod.ts:139
metadata/src/loaders/database-loader.ts:748 (rowToRecord) updatedAt: row.updated_at as string | undefined MetadataRecord.updatedAt — same file, :141
metadata/src/loaders/database-loader.ts:1080 (getHistoryRecord) recordedAt: row.recorded_at as string MetadataHistoryRecord.recordedAt — same file, :452
metadata/src/loaders/database-loader.ts:1161 (queryHistory) recordedAt: row.recorded_at as string same as above

All five are unchecked casts, which is why tsc reported nothing: the string is an assertion about a driver row, never a measurement of one.

Why both column classes are affected

SqlDriver#formatOutput repairs the BUILTIN audit columns (repairNaiveUtcAuditTimestamp) and folds declared Field.datetime columns (normalizeSqliteDatetimeOutput) only inside its if (this.isSqlite) arm — verified at sql-driver.ts:16048, where both loops sit. withPostgresCalendarDayAsText leaves timestamptz / timestamp alone on purpose, because those are instants. So a column being declared Field.datetime does not protect it, and on Postgres and MySQL both classes come out of the record read door as a Date. That dialect asymmetry is pinned live in packages/drivers/driver-sql/src/sql-driver-13567-audit-stamp-materialisation.test.ts.

The route, and the two prohibitions kept

Producer-side canonicalisation at the adapter boundary that asserts the declared type — the route #13997 established. Not a tolerant ?? fallback in a consumer (#13973's standing prohibition), and not route B at the driver's read door, which would reverse a deliberate driver decision and belongs to the whole census rather than to this card.

⚠️ On the contested shared normaliser — read this part

The obvious spelling was to call the canonicalIsoInstant helper that #13997 already left in both of these files, which would have made four of five sites a one-word change. This PR deliberately does not do that, and the reason is a live adjudication rather than taste.

canonicalIsoInstant (and its canonicalIsoStamp sibling in rest-server.ts, and auditMetaItem in protocol.ts) reaches value.toISOString() for any Date, which raises RangeError: Invalid time value on an Invalid Date. #14078 measured that input to be reachable on both live dialects — a MySQL zero datetime returns mysql2's INVALID_DATE constant by name, and any Postgres year in 275760..294276 materialises through postgres-date as new Date(NaN). Whether the shared spelling should throw there (option A) or fall back to a rendering (option B) is a maintainer call across four packages, and #13973 is pm:blocked on it.

So adopting it here would have imported option A's consequence into five new call sites while the question is open, turning a silently-wrong field into an uncaught 500 on a read path. Instead each file gets a narrow local helper that converts only a valid Date and returns every other shape untouched:

function isoFromValidDate(value: unknown): unknown {
  if (value instanceof Date && !Number.isNaN(value.getTime())) return value.toISOString();
  return value;
}

An Invalid Date therefore reaches the consumer exactly as it does today — no new throw, no invented rendering, nothing for #14078 to un-decide. The Number.isNaN(value.getTime()) guard is the spelling already in use at rest/src/export-format.ts:291 and rest/src/import-prepare.ts:115, not a new one. When #14078 rules, both helpers collapse into the shared spelling; each docblock says so.

What adopting canonicalIsoInstant would have changed, site by site, for the Invalid-Date input only (every other input is identical either way):

  • database-loader.ts rowToRecord createdAt / updatedAt — would throw inside rowToRecord, taking load() and stat() with it.
  • database-loader.ts getHistoryRecord recordedAt — would throw on the history read.
  • database-loader.ts queryHistory recordedAt — would throw on the history page.
  • sys-metadata-repository.ts rowToEvent ts — would throw inside the history() generator and inside watch's replay.

Two new test cases pin the neutral behaviour, so this stays a measured decision rather than a claim.

⛔ Two things this PR does not do, both deliberate: it does not decide #14078, and it does not promote the helper to a shared export in @objectstack/metadata-core. The card floated that consolidation and triage allowed it in scope, but it would widen a package's public surface for a spelling #14078 is about to consolidate anyway — the smaller, reversible move is two locals with docblocks pointing at each other.

Behaviour preserved for every non-Date shape

  • An already-canonical SQLite string passes through byte-identically (pinned).
  • An absent column still yields undefined, so each caller's existing ?? default chain keeps exactly its current meaning — including rowToEvent's epoch fallback (pinned).
  • stat()'s mtime, the one in-repo consumer of rowToRecord's timestamps, reads through canonicalIsoInstant and now receives a string it returns unchanged.

Measurements the card asked for

  • Count. Five, and they are the five the card names. The dispatch note's separate file list (metadata-core/src/**, metadata-fs/src/repository.ts, metadata/src/metadata-manager.ts) is falsified: zero hits in all three, and metadata-fs/src/repository.ts and metadata-manager.ts contain no as string cast at all, while the same grep shape returned hits in the two real files.
  • sqlite / memory. They round-trip canonical ISO text, so this is a Postgres/MySQL divergence — stated in the [finding] No live-Postgres coverage of the record-data OCC seam — the driver shape that broke it is only ever a hand-made fixture #13567 pin's own header. Consequence taken up: the regression pins need no live cell. DatabaseLoader's seam is IDataDriver / IDataEngine and SysMetadataRepository's is an engine, so both suites drive a hand-made Date in process, which is also what keeps these packages free of a driver dependency.
  • Consumers depending on a Date. None — measured, not assumed. MetadataEvent.ts has exactly one in-repo reader, MetadataManager.applyRepoEvent at metadata-manager.ts:2933, which forwards it to MetadataWatchEvent.timestamp, itself declared z.string().datetime() — so converting repairs that consumer rather than breaking it. recordedAt's only readers (rollback, diff) touch .metadata and never the timestamp. rowToRecord's timestamps never leave the loader: load() reads only record.checksum and stat() passes them through canonicalIsoInstant. No stop condition triggered.

Out of scope, filed rather than fixed

The card's lower-confidence neighbour is a real sixth site of the same class, and the declaration it could not locate is an inline TypeScript return type rather than a Zod schema: listDrafts declares updatedAt: string | null at sys-metadata-repository.ts:1108 and emits row.updated_at ?? row.created_at ?? null at :1140, with rows cast as any[] one line above. Triage ruled that site explicitly out of this card with "file a separate card if it is real", so it is filed as #14938 and untouched here. #14938 is not addressed by this PR.

Verification

Gate union re-run at the final commit 2a96397bc, derived by node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands with no path arguments, on the final six-path change set: 47 commands, 43 exit 0, 0 findings. Four exited 3 (PREREQUISITE NOT MET, recorded as NOT MEASURED, each by its own printed verdict): check-test-completeness and pm/check-half-states have no local input, check:dual-build-cjs-loads and check:type-check-debt need a whole-repo build. check:engine-double-contract reported the two new test files as new pinned coverage the ledger did not record; --write added 4 rows, 0 lost.

  • pnpm --filter @objectstack/metadata --filter @objectstack/metadata-protocol test — 691/691 and 2339 passed, 10 skipped.
  • pnpm --filter @objectstack/metadata-protocol typecheck — clean; tsc --listFiles confirms the new test file is in the program (1 hit, against a control hit for the MetadataItem.authoredAt is declared z.string() but receives a JS Date on Postgres/MySQL — a silent declared-contract violation, because the schema is parsed only in its own test #13997 sibling).
  • packages/metadata declares no typecheck script and carries a DEBT ledger entry of 89; a direct tsc --noEmit there reports exactly 89, none naming either changed file. That is a corroborating reading, not the ratchet's re-measure, which needs the whole closure built.
  • pnpm lint — whole repo, eslint . --no-inline-config, clean in 95s. No narrowing claimed.
  • node scripts/pm/check-governed-merges.mjs --test on the final file list — 0 of 6 paths governed.

Ablation. Both helper bodies were mutated to a bare return value; — the pre-fix behaviour at all five sites in one edit per file — with the mutation confirmed on disk in both directions (guard-line count 0 in each file; whole-tree git diff --stat showing the two deletions) and both packages rebuilt; scripts/ablation-dist-preflight.mjs --absent confirmed the guard had left both dist/ trees. Predicted direction was stated before the run and matched: 3 of 7 red in metadata, 1 of 4 red in metadata-protocol, with the SQLite-passthrough, absent-column and #14078-neutrality cases correctly staying green because the mutation does not change them. Restore leg: git checkout HEAD -- on absolute paths, proven by an empty git diff HEAD, a whole-tree git status --porcelain with no output, and a git hash-object match against each path's HEAD blob; both packages rebuilt and the preflight confirmed the guard back in dist/; the restored run is 7/7 and 4/4 green.

🤖 Generated with Claude Code

https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68


Generated by Claude Code

Claude and others added 3 commits September 3, 2026 10:30
@github-actions github-actions Bot added the size/l label Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

5 anchor(s) derived from 2 changed package(s); no hand-written page names any of them, so this run has nothing to listnot a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 12 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 7317cf266e9682edc830ccac6fe78e66b3ddfc32packageMentionDocs.

Which tree this was computed on

This run read content/docs from 2aa6f79e2e858aabd1fea891c409cb3872e38fdf — the merge of head 2a96397bcb692f4e77687255447a6457a779017a into base 7317cf266e9682edc830ccac6fe78e66b3ddfc32, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 2aa6f79e2e858aabd1fea891c409cb3872e38fdf && git checkout 2aa6f79e2e858aabd1fea891c409cb3872e38fdf
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 7317cf266e9682edc830ccac6fe78e66b3ddfc32 2a96397bcb692f4e77687255447a6457a779017a && git checkout -B drift-repro 7317cf266e9682edc830ccac6fe78e66b3ddfc32 && git merge --no-ff 2a96397bcb692f4e77687255447a6457a779017a

node scripts/docs-audit/affected-docs.mjs --json 7317cf266e9682edc830ccac6fe78e66b3ddfc32

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

1 participant