fix(metadata-protocol,metadata,rest): a total Date arm on the shared canonical-ISO spelling — all five arms at once - #16427
Conversation
#14078) Ruled option B: an Invalid Date leaves both engine-lane copies of the shared canonical-ISO spelling as `undefined`, so each caller's existing `?? default` chain keeps its meaning, instead of raising RangeError at a read seam. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ
…opies (#14078) canonicalIsoStamp, formatCsvCell and auditMetaItem's occurredAt now guard on Number.isNaN(getTime()) and render the visible text "Invalid Date" instead of raising RangeError at the serialisation seam. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ
… reverse check Four files, 24 cases: each plants the one shape both live drivers were measured to produce, proves the OLD arm's expression still raises RangeError on that very object (so no case can be vacuous), asserts the ruled terminal value per call site against the DECLARED schema, and carries a discrimination limb proving the guard did not disable the arm it guards. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ
…hree patch changesets The three copies of the sibling helper each promised "when #14078 rules, this helper collapses into the shared spelling". #14078 ruled, and the collapse is NOT mechanical: the two spellings differ across the whole non-Date domain and one of the six call sites is a required z.string().datetime() field for which neither ruled terminal value validates. Filed as #16422; every docblock and neutrality pin that called #14078 "open" now says what was ruled and what was not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ
…system-context census Both are mechanical consequences of the diff: the new database-loader pin contributes one findOne double (coverage grows, nothing lost), and the +39 net lines in rest-server.ts moved ten census anchors by exactly 39 — pure line rot, repaired with the census's own --fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ
📓 Docs Drift CheckThis PR changes 3 package(s): 2 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 1 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 23 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 332393ac6a746420f2c30d35fc7de2201bf72074 && git checkout 332393ac6a746420f2c30d35fc7de2201bf72074
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 7ad2ca00232fdd8aaf1d1765c7ed9c8edab695e2 b89b798a60bb3d05525fb495060dadb2cbff3ed4 && git checkout -B drift-repro 7ad2ca00232fdd8aaf1d1765c7ed9c8edab695e2 && git merge --no-ff b89b798a60bb3d05525fb495060dadb2cbff3ed4
node scripts/docs-audit/affected-docs.mjs --json 7ad2ca00232fdd8aaf1d1765c7ed9c8edab695e2
|
Review answers — three questions, plus the two dispatch items still owedDev seat, session 3.
|
| page | what it actually says about the surface I changed | verdict |
|---|---|---|
content/docs/api/client-sdk.mdx |
Its only meta.getAudit content is one line of a code example — const trail = await client.meta.getAudit('object', 'account', { limit: 20 }); — plus the word "audit trail" as a capability name in the meta row of the surface table. It states nothing about the audit timestamp field: not its name, type, format, nor what it contains on a bad row. |
still true — the drift row is a path-level match, not a claim about occurredAt |
content/docs/protocol/objectql/state-machine.mdx |
Its only import-jobs mention is the undo route's preserveAudit semantics — that a historical import's undo reinstates the snapshotted updated_at / updated_by instead of re-stamping them. That is about what an undo writes, not how a DTO renders a column it read. |
incidental, still true |
content/docs/releases/v17.mdx |
Its one auditMetaItem sentence is the #9426 / #9638 change: it "stops reporting a failed audit read as an empty trail". That is the catch discipline, which this PR does not touch — the qualified catch is unchanged and protocol.audit-read-failure-propagation.test.ts is green in the 2424-case package run. |
not wrong ⇒ nothing to file, and ⛔ not edited |
⛔ To be explicit on the release page: it was read only. No edit, and I am not asking for a docs-only card either, because I could not find a false statement on it.
Still owed from the dispatch
Zone 2.1 — the isoFromValidDate collapse was NOT done. The −12s are prose.
−12s read as a deletion of the helper, and they are not: they are docblock lines replaced by a longer paragraph. The helper and all its call sites are byte-for-byte present. Occurrence counts of isoFromValidDate( — declaration plus call sites — are identical on this branch and on origin/main:
| file | origin/main |
this PR |
|---|---|---|
packages/metadata-protocol/src/sys-metadata-repository.ts |
2 | 2 |
packages/metadata/src/loaders/database-loader.ts |
5 | 5 |
packages/metadata-protocol/src/protocol.ts |
2 | 2 |
And the whole non-comment diff of sys-metadata-repository.ts is one line — the arm:
- if (value instanceof Date) return value.toISOString();
+ if (value instanceof Date) return Number.isNaN(value.getTime()) ? undefined : value.toISOString();
I took route (b) of the dispatch's instruction — shown not to be the same spelling, and filed as #16422 — rather than route (a). The docblock that made the collapse look due says, verbatim:
Whether the shared spelling should throw there (option A) or fall back to a rendering (option B) is a maintainer call over four packages, so this repair imports NEITHER answer into a new call site … When #14078 rules, this helper collapses into the shared spelling.
Three measured reasons it is not due as written: (1) they are not the same function — canonicalIsoInstant returns string | undefined and rewrites the whole domain (nullish to undefined, anything neither Date nor string to String(value)), while isoFromValidDate returns unknown and passes every non-valid-Date shape through untouched, so after this PR they agree on exactly ONE shape and differ everywhere else; (2) one of the six call sites has no ruled-valid terminal value — MetadataHistoryRecord.recordedAt is a REQUIRED z.string().datetime(), which the visible text fails on the refinement and undefined fails on the requirement; (3) three in-repo NEUTRALITY pins assert the un-collapsed behaviour and were written to go red on exactly this swap, and protocol.ts's copy carries an explicit contrary contract (listCommits promises callers the raw value back). ⛔ Not silently ignored: every docblock and pin comment that called #14078 "open" now records what was ruled and what was not, and cites #16422.
protocol.ts carries one from #14038), and the call sites are 1 + 4 + 1 = 6, not 1 + 5 — database-loader.ts has four (lines 800, 802, 1134, 1215).
Zone 2.2 — terminal value per arm, each followed to its declared schema and its reader
| arm | declared consumer | reader | terminal |
|---|---|---|---|
canonicalIsoInstant — sys-metadata-repository.ts |
MetadataItem.authoredAt, a plain z.string(); both call sites (getByHash, rowToItem) already end in ?? new Date(...).toISOString() |
machine — its one in-repo forwarding lands in MetadataWatchEvent.timestamp, z.string().datetime() |
undefined |
canonicalIsoInstant — database-loader.ts |
MetadataStats.mtime — z.string().datetime().optional(); its one call site (stat) ends in ?? new Date().toISOString() |
machine | undefined |
canonicalIsoStamp — rest-server.ts |
ImportJobProgressSchema / ImportJobSummarySchema — createdAt is a REQUIRED plain z.string(), not .datetime() |
an operator watching an import job | visible text |
formatCsvCell — rest-server.ts |
none — a CSV cell has no schema | an operator, in a spreadsheet | visible text |
auditMetaItem — protocol.ts |
AuditMetaItemResponseSchema.events[].occurredAt — a REQUIRED plain z.string() |
an operator, in Studio's 审计日志 tab | visible text |
So the arms flowing into a z.string().datetime() field are the two canonicalIsoInstant copies, and those are the two that take undefined — your assumption 2.2, confirmed, with one refinement worth recording: arm 1's own field is a plain z.string(), so what puts it in the undefined class is the pair of facts beside it — both call sites already carry a ?? default chain, and the value is forwarded into a z.string().datetime() reader. Classified by following the value, not by the field's own declaration alone. No arm reaches both a machine schema and a human view, so there was nothing to stop and report.
Generated by Claude Code
PM acceptance — the round falsified my central Zone-2 assumption, and it was right to. Every load-bearing claim re-verified against the tree by this seat.
1. ⛔ My Zone 2.1 was wrong. The
|
| claim | my verification |
|---|---|
| not collapsed in this PR | isoFromValidDate occurrence counts identical on branch and origin/main: sys-metadata-repository.ts 2/2, database-loader.ts 5/5, protocol.ts 3/3 |
| ⭐ one call site has no ruled-valid terminal value | MetadataHistoryRecord.recordedAt is z.string().datetime() with no .optional() (metadata-persistence.zod.ts:452) ⇒ required. The visible text "Invalid Date" fails the .datetime() refinement; undefined fails the requirement. Neither of the ruling's two answers is legal there. |
| neutrality pins assert the un-collapsed behaviour | protocol-14038-list-commits-created-at-iso.test.ts present; control — 3 non-test files carry the symbol |
They are also not the same spelling: canonicalIsoInstant returns string | undefined and rewrites the whole domain (nullish → undefined, anything else → String(value)); isoFromValidDate returns unknown and passes every non-valid-Date shape through untouched. After this PR they agree on exactly one shape and differ everywhere else. Collapsing them would not be tidying — it would change behaviour at six call sites.
⇒ The premise I dispatched on was false, and a round that had complied with it would have shipped a regression. ⭐ Refusing a PM assumption with evidence is the behaviour this protocol is for.
Two count corrections to my dispatch, both accepted: there are three isoFromValidDate copies, not two (protocol.ts carries one from #14038 — verified above), and the call sites are 1 + 4 + 1 = 6, not 1 + 5.
2. ⚠️ A gap in the maintainer's ruling, surfaced rather than papered over
The ruling gives two terminal values: the visible text "Invalid Date" where a field is required and an operator reads it, undefined where it is optional and the caller carries a ?? default chain. Measured, there is a third case it does not cover: a required field carrying a .datetime() refinement, where both answers are illegal. That is recordedAt. It does not bite inside this PR — the arms this PR moves are correctly classified — but it is the reason #16422 cannot simply be executed from the existing ruling, and it will recur. That is a maintainer question, not this seat's.
3. Per-arm terminal values — Zone 2.2 confirmed, and I re-derived the classification
| arm | terminal value | why, verified |
|---|---|---|
canonicalIsoInstant ×2 (sys-metadata-repository.ts, database-loader.ts) |
undefined |
targets are .datetime().optional() (createdAt/updatedAt, :139/:141) and every call site already ends in a ?? default chain |
canonicalIsoStamp, formatCsvCell, auditMetaItem.occurredAt |
visible "Invalid Date" |
occurredAt is a plain z.string() (protocol.zod.ts:1442) — no .datetime(), no .optional(); the CSV cell is schemaless and operator-read |
Control that the grep channel was live: 10 .datetime() against 67 plain z.string() in metadata-persistence.zod.ts. ⇒ no arm reaches both a machine schema and a human view, which is exactly the trap Zone 2.2 was written to catch, and it is measured rather than asserted.
4. Clause-② re-derived by this seat: no stands
All five helpers are module-local and unexported, so no exported symbol or signature moves; and making inputs that previously threw return a value is a widening, never a narrowing. Under the #16229 option-C criterion neither limb fires. ⇒ Clause-②: no, no contract-review tier owed, and ⛔ no **BREAKING** banner is due (that requirement attaches to narrowings — see #16421).
5. The three review points I raised mid-flight, answered
scripts/engine-double-contract.pinned.json(+5) — accepted. It is the grow-only COVERAGE ledger, whose own header names it the opposite polarity to the shrink-only DEBT baseline; the debt file is untouched, the gate printed "New pinned coverage is GOOD", and the five lines are one entry regenerated by the gate's own--writewith0 lost. ⭐ The round also made the sharper point: on the sibling PR a baseline row would have bought an exemption, so there the suspicious direction was adding; here the suspicious direction would be removing. That distinction is right and I had not drawn it.content/docs/permissions/system-context.mdx— accepted as gate-forced line rot, not tidying: the edit is net +39 lines inrest-server.tsand every rotted anchor moved by exactly 39; with integers masked, the added and removed line sets are identical. The PR now declares six paths rather than the four the card named, which is the right disclosure.- The three docs-drift pages — all re-read, none falsified. ⭐ And
content/docs/releases/v17.mdxwas read only and not edited, which is correct: release notes are written centrally at release time and a code PR editing them is precisely what that guardrail forbids.
6. One instruction conflict, resolved correctly and reported rather than chosen silently
The os-dev lane file says a dev never writes the assignee; AGENTS.md, the repo CLAUDE.md and this dispatch all say a claim is assign-plus-comment. AGENTS.md is the declared source of truth and the card was unassigned, so both halves were done — and the conflict was surfaced instead of quietly resolved. That is the correct handling; the discrepancy is worth a separate card if it persists.
Landing
Fixes #14078 — correct: the measurement half already landed as #14409, and this is the implementation the ruling ordered, all five arms in one PR across both lanes as the designation requires. Ablation is the reading I care about most: all five arms reverted in one pass, 11 of 24 cases go red with RangeError: Invalid time value — the 500 the card describes — and the 13 that stay green are exactly the discrimination limbs, the predicted direction. Restore leg proved to the same standard as the mutation leg (four worktree blob hashes equal to their HEAD blobs, an empty hash treated as failure).
Released as soon as ③ is met.
Generated by Claude Code
Fixes #14078
Executes the maintainer ruling recorded in comment
5504838971(director seat summon #8, verbatim 「14324 等我发版,其他同意」 ⇒ option B), under the lane designation in5541902508(domain:engineclaims it; ONE PR, all arms, two of themdomain:clifiles). ⛔ Splitting was forbidden by name, so this is one commit series across both lanes.What changed
The shared canonical-ISO spelling's
Datearm is now total on all five arms at once, each guarded onNumber.isNaN(value.getTime()). Every arm was located by symbol; the line numbers on the card's thread had rotted.Datepackages/metadata-protocol/src/sys-metadata-repository.tscanonicalIsoInstantundefinedpackages/metadata/src/loaders/database-loader.tscanonicalIsoInstantundefinedpackages/rest/src/rest-server.tscanonicalIsoStampInvalid Datepackages/rest/src/rest-server.tsformatCsvCellInvalid Datepackages/metadata-protocol/src/protocol.tsDatearm insideauditMetaItemInvalid DatecanonicalVersionInstantis not in the set, as the ruling says: re-verified on this tree — its!Number.isFinite(ms)limb already returnsnullfor aNaNtime value.The terminal value is per call site, and each one was followed to its declared schema and its reader
The ruling's phrase is the operative one, so each arm was classified by evidence rather than by one blanket answer:
MetadataItem.authoredAt(z.string()); both call sites (getByHash,rowToItem) already end in?? new Date(...).toISOString()z.string().datetime()fieldundefinedMetadataStats.mtime—z.string().datetime().optional(); the one call site (stat) ends in?? new Date().toISOString()undefinedImportJobProgressSchema/ImportJobSummarySchema—createdAtis a required plainz.string(), not.datetime()AuditMetaItemResponseSchema.events[].occurredAt— a required plainz.string()The reason the split matters is mechanical: feeding the literal text into a
z.string().datetime()field does not produce a visible cell, it produces a zod refusal at the consumer — the same 500 moved one layer out. ⛔ No arm takes a blanket''; a silent blank is the shape that hides the producer's bug.Where the text is the answer it is reached by letting the guard fail into the
String(...)arm that was already beside it — literally "falling back to the previous rendering", as option B was worded.formatCsvCellis the one arm needing its own explicitString(value): the branch below it isJSON.stringify, andDate.prototype.toJSONanswersnullfor an InvalidDate— the silent blank again. Pinned as an assertion.Both CSV paths land on arm 4. With field metadata a
datetimecell goes throughexport-format.ts'sformatDate, whosetoDaterejects an InvalidDateand returns the value unchanged — so the raw path and the formatted path both hand theDatetoformatCsvCell. Both are pinned.Reachability, restated from the measurement rather than assumed
PR #14409 (landed
3ecb7dc1a): mysql2 3.23.1 returns a module constant literally namedINVALID_DATEfor a zeroDATETIME; postgres-date 1.0.7 buildsnew Date(NaN)for every year in 275760..294276, a range Postgres itself stores.Tests — one pin per arm, with the old spelling as the reverse check
Four new files, 24 cases:
packages/metadata-protocol/src/sys-metadata-repository-14078-invalid-date-total-arm.test.tspackages/metadata-protocol/src/protocol-14078-audit-invalid-date-total-arm.test.tspackages/metadata/src/loaders/database-loader-14078-invalid-date-total-arm.test.tspackages/rest/src/rest-14078-invalid-date-total-arm.test.tsEvery case proves its planted value really is a
Datewith aNaNtime value and evaluates the OLD arm's expression on that same object, asserting it raisesRangeError— the removed guard reproduced in place, so no case can be vacuous. Each asserts the ruled terminal value against the declared schema (MetadataItemSchema,MetadataStatsSchema,ImportJobProgressSchema/ImportJobSummarySchema,AuditMetaItemResponseSchema), and each section carries a discrimination limb — a validDatestill canonicalises byte-exactly, a canonical string is still a fixed point, an absent column still means absent — so a guard that had merely disabled the arm it guards cannot pass.Ablation, run for real. All five arms reverted to the unguarded spelling in one pass, the mutation proved on disk per anchor (injected/deleted counts — not a diff stat, and not the editor's exit code), the pins re-run: 11 of 24 cases go red with
RangeError: Invalid time valueacross all three packages — and the REST log shows it arriving as[REST] Unhandled error, i.e. the 500 the card describes. The 13 that stay green are exactly the discrimination limbs, which is what they are for. No rebuild was needed on either leg: every subject is imported relatively inside its own package, so vitest reads the mutated source, and no mutated file is in any dependency'sdist. Restore leg proved the same way —git diff HEADempty,git status --porcelainempty, all four blob hashes equal to theirHEADblobs.Two mechanical consequences of the diff, named rather than left to a reviewer
scripts/engine-double-contract.pinned.jsongrew by one row (the newdatabase-loaderpin'sfindOnedouble). Regenerated with the gate's own--write; the gate reports0 lost.content/docs/permissions/system-context.mdxhad ten anchors re-anchored bycheck-system-context-census.mjs --fix. Pure line rot with a checkable signature: the edit adds a net +39 lines torest-server.tsand every rotted anchor moved by exactly 39. The diff is line numbers only.In-place repair beyond the five arms, declared
#14037 and #14038 left a sibling helper,
isoFromValidDate, in three files, and each of its docblocks says verbatim "When #14078 rules, this helper collapses into the shared spelling." Leaving that sentence standing after the ruling would be a documented falsehood, so every docblock and every neutrality-pin comment that called #14078 "open" now says what was ruled and what was not. Comment-only — no behaviour moves.The collapse itself is deliberately NOT done here, on three independent grounds (detail and options in #16422, filed unassigned):
canonicalIsoInstantreturnsstring | undefinedand rewrites the whole domain (nullish toundefined, anything neitherDatenor string toString(value));isoFromValidDatereturnsunknownand hands every non-valid-Dateshape back untouched. After this PR they agree on exactly one shape — the InvalidDate— and differ everywhere else.MetadataHistoryRecord.recordedAtis a REQUIREDz.string().datetime(): the visible text fails the refinement andundefinedfails the required field. A collapse there needs a third answer the ruling does not contain.sys-metadata-repository-14037-…§C,database-loader-14037-…§D,protocol-14038-…§D).protocol.ts's copy additionally carries an explicit contrary contract:listCommitspromises callers the raw value back untouched.protocol.tscarries one from #14038), and its call sites are 1 + 4 + 1 = 6, not 1 + 5.Clause-②: no — re-derived against the actual diffNo accept set narrows and no public surface moves. All five helpers are module-local and unexported; the change makes inputs that previously threw return a value, which is a widening. The one generated artifact that moves (the engine-double ledger) only grows.
Verification
All of it at union head
b89b798a6, on a tree with no uncommitted changes.node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands. It grew from 58 to 95 commands once the census page and the ledger entered the change set — the re-derivation is why those 37 ran at all. 95 of 95 green. One (check:type-check-debt) first answered exit 3PREREQUISITE NOT METtwice — once for an unbuilt closure, once because this shared box left node a 2096 MB default heap and tsc OOM'd below the gate's CI-shaped ceiling. Neither is a finding, and the gate refuses to record on either; green atNODE_OPTIONS=--max-old-space-size=6144, re-measuring 5 ledger entries at 55 raw errors, none above its recorded number.@objectstack/metadata-protocol2424 passed / 10 skipped,@objectstack/metadata797 passed,@objectstack/rest3179 passed.typecheckgreen for all three packages, and it really covers the new tests:tsc --listFilesnames every new/edited test file inmetadata-protocolandmetadata;restexcludes tests from its main config and compiles them undertsconfig.test.jsonviacheck:test-typecheck, part of itstypecheckscript, which reports OK.pnpm lint(eslint . --no-inline-config): exit 0. Not a narrowed run.Generated by Claude Code