Skip to content

fix(metadata): collapse four isoFromValidDate call sites onto the shared canonical-ISO spelling (#16422) - #17198

Merged
huangyiirene merged 3 commits into
mainfrom
claude/issue-16422-iso-from-valid-date-collapse
Sep 10, 2026
Merged

fix(metadata): collapse four isoFromValidDate call sites onto the shared canonical-ISO spelling (#16422)#17198
huangyiirene merged 3 commits into
mainfrom
claude/issue-16422-iso-from-valid-date-collapse

Conversation

@claude

@claude claude Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Fixes #16422

The card's sentence — "#14078 ruled, and the collapse did not happen" — is closed by
collapsing four of the family's five consumer call sites onto canonicalIsoInstant,
holding one out under its written pass-through contract, and ruling the driver-sql
helper of the same name out of the family entirely. Both metadata-side definitions of
isoFromValidDate are deleted.

Census, re-measured by symbol on this head (fd5cff209 base)

file signature ruling
packages/metadata-protocol/src/sys-metadata-repository.ts:218 (value: unknown): unknown deleted — site collapsed
packages/metadata/src/loaders/database-loader.ts:169 (value: unknown): unknown deleted — 4 sites collapsed
packages/metadata-protocol/src/protocol.ts:1843 (value: unknown): unknown keptlistCommits pass-through contract
packages/drivers/driver-sql/src/sql-driver.ts:335 (value: Date): unknown not in this family — untouched

Control fired on the same pathspec: canonicalIsoInstant resolved to its 2 known
definitions. Post-change census: 4 definitions → 2.

The two further non-test references the card did not account for are prose
cross-references to the driver copy
, not call sites: packages/rest/src/rest-server.ts:706
and packages/services/service-storage/src/stranded-orphan-inventory.ts:206 both cite
[ADR-0053 D-F3], isoFromValidDate — D-F3 is the driver's Invalid-Date pass-through
decision. Neither is touched.

Ruling on the driver-sql copy: it is not a fourth copy

Stated with the measurement, per the dispatch. It is a producer-side fold on a
Date-narrowed domain
, and it is not even substitutable for the three consumer copies:

  1. Parameter type. (value: Date), not (value: unknown).
  2. Both call sites narrow first. presentAuditTimestampOutput (line 359) and
    normalizeSqliteDatetimeOutput (line 421) each guard if (value instanceof Date)
    before calling it. Its non-Date domain is unreachable by construction.
  3. It would throw on the consumer copies' inputs. Its body is
    Number.isNaN(value.getTime()) ? value : value.toISOString() with no instanceof
    check, so isoFromValidDate(null) raises TypeError. The consumer copies return
    null untouched. They are different total functions on different domains.
  4. Different layer, different governing decision. It sits at the driver's read
    boundary under ADR-0053 D-F3, which is precisely the decision that an Invalid Date
    leaves the driver as a Date — neither nulled nor spelled as text. The other three
    are consumer-side adapters whose job is to satisfy a packages/spec declared type.
    Folding a producer into a consumer spelling is the direction Prime Directive Add comprehensive test suite for Zod schema validation #12
    forbids, and would erase D-F3.

⇒ Out of the collapse. Every anchor above was re-derived by symbol on this head, not
carried over from the card (#16887 moved 195 lines of that file this round).

Behavioural comparison per arm — the evidence for the collapse

Driven through the real call sites (real classes, real engine doubles), seven inputs
chosen to distinguish the two helpers, before and after, with the declared schema's own
verdict on each produced value. PARSE = MetadataEventSchema / MetadataRecordSchema /
MetadataHistoryRecordSchema .safeParse.

site \ input validDate isoString null undefined number invalidDate opaqueObject
S1 rowToEvent.ts = = = = number"1772…" Date ❌ → epoch ✅ object ❌ → "[object Object]"
S2 rowToRecord.createdAt = = null ❌ → undefined = "1772…" Date ❌ → undefined "[object Object]"
S3 rowToRecord.updatedAt = = null ❌ → undefined = "1772…" Date ❌ → undefined "[object Object]"
S4 getHistoryRecord.recordedAt = = null ❌ → epoch ✅ undefined ❌ → epoch ✅ "1772…" Date ❌ → epoch ✅ "[object Object]"
S5 queryHistory.recordedAt = = null ❌ → epoch ✅ undefined ❌ → epoch ✅ "1772…" Date ❌ → epoch ✅ "[object Object]"
S6 listCommits.createdAt = = = = = = =

= is byte-identical before and after. 17 of 42 cells changed; S6 changed zero.
Schema refusals across the matrix: 21 → 8.

The 8 remaining refusals are number and opaqueObject at S2–S5. Those are shapes no
driver is measured to materialise for a declared Field.datetime column. They now arrive
as the declared type (a string) that simply is not a valid datetime, so the producer's
bug stays loud instead of being papered over — ⛔ no repair is invented for an unmeasured
shape.

Per-site terminal value, and why

  • S1 MetadataEvent.ts (z.string(), forwarded to a z.string().datetime()) →
    canonicalIsoInstant(…) ?? new Date(0).toISOString(). The ?? was already there for an
    absent column; an Invalid Date now takes the same branch. The same file's history()
    already reads this same recorded_at column exactly this way for authoredAt, so the
    collapse makes two readers of one column agree.
  • S2/S3 MetadataRecord.createdAt / .updatedAt (z.string().datetime().optional())
    undefined. ⛔ No default invented for a field the schema lets be absent.
  • S4/S5 MetadataHistoryRecord.recordedAt (required z.string().datetime()) →
    the epoch, through a named recordedAtFallback() shared by both doors so they cannot
    drift. ⛔ Not new Date(): a now stamp is a plausible-looking recording instant nobody
    measured, indistinguishable at every reader from a real one, and it sorts a version
    recorded years ago to the top of a newest-first timeline. The epoch invents no fact and
    sorts to the oldest end. It is also the answer the sibling reader of this same column
    already gives.
  • S6 listCommits.createdAtunchanged. Its docblock promises callers the RAW
    value back for a non-Date. Swapping in the shared spelling would ERASE an Invalid
    Date from the response (undefined — the one answer ADR-0053 D-F3 refuses, because it
    silently drops a value that is on disk) and hand a number or an opaque object to
    compareAuditInstants as String(value) instead of verbatim, reordering rows that seam
    deliberately leaves alone.

All four casts are gone, not restated (as string | undefined ×2, as string ×2):
canonicalIsoInstant returns string | undefined, so the declared type is a measurement
now. That closes acceptance item 2 by construction rather than by assertion.

One composed behaviour changed — stated, not buried

DatabaseLoader.stat() computes record.updatedAt ?? record.createdAt. An Invalid
updated_at used to win that ?? (a Date is truthy and not nullish), so a row with
an unreadable updated_at and a good created_at published new Date() as its mtime.
It now folds to undefined one step earlier and loses the ??, so the row publishes
its created_at.

Both answers satisfy the declared z.string().datetime(); the new one is a stored instant
in place of a fabricated one, and it is exactly the "same ?? DEFAULT chain an absent
column takes"
that #14078's own ruling text prescribes for the shape. The pin that
asserted the old direction is rewritten to assert the new one, with the reason inline.

Pin dispositions — ⛔ none deleted

pin disposition why
sys-metadata-repository-14037…test.ts §C rewritten as a ruled pin It was written to go red on exactly this swap. The swap was deliberate and now carries its own evidence, so §C asserts the ruled terminal value, plus a new case proving the retired shape is one the declared schema refuses.
database-loader-14037…test.ts §D rewritten as a ruled pin Same reason. Now asserts two different terminal values (undefined vs epoch) because the two declared schemas differ — a single value would have been the tell that nobody followed each site to its schema. Two cases added: the other history door, and a null column.
protocol-14038…test.ts §D kept verbatim The only one of the three whose behaviour did not move. Its prose is updated from "pending decision #16422" to "decided, and this site was held out".
database-loader-14078…test.ts §A₂ + §C rewrittennot named by the card A fourth section pins the composition through isoFromValidDate. §A₂ asserted the old ?? direction; §C asserted rowToRecord hands the Date through. Both are rewritten to the new composition, and §C gains a case proving the Date arm is still exercised (one frame earlier), so nobody reads it as dead.

database-loader.test.ts's #13997 docblock also carried a now-false sentence about the
unchecked cast; corrected, no assertion changed.

Ablation — the rewritten pins really do fail

Fix committed first, then the retired spelling reinstated inline at one collapsed site per
package, proven on disk, run, restored, restoration proven byte-identical.

HEAD blobs:     SMR=ce0567fc46f4b4a463fc77e73da40a1c89178cd9 DBL=5b5926c2d7ee20da761454d176834ca87c86fd68
ON-DISK PROOF   SMR injected=1 removed_remaining=0 | DBL injected=1
MUTATED blobs:  SMR=079eb761e8a5871ab345ad091d6345e399af8aa3 DBL=09b22ff97ef7fe7678a385b05272946c5f22e7aa

ABLATE_MP_EXIT=1   Test Files 1 failed (1)   Tests 1 failed | 4 passed (5)
  × answers the epoch and produces an event the declared schema accepts
    AssertionError: expected Invalid Date to be '1970-01-01T00:00:00.000Z'
ABLATE_MD_EXIT=1   Test Files 1 failed (1)   Tests 2 failed | 8 passed (10)
  × recordedAt: the epoch, and MetadataHistoryRecordSchema now accepts the record
    AssertionError: expected Invalid Date to be '1970-01-01T00:00:00.000Z'
  × a `null` column reaches the same terminal values — not just the Invalid `Date`
    AssertionError: expected null to be '1970-01-01T00:00:00.000Z'

RESTORED blobs: SMR=ce0567fc46f4b4a463fc77e73da40a1c89178cd9 DBL=5b5926c2d7ee20da761454d176834ca87c86fd68
git diff HEAD -> 0 lines; git status --porcelain -> clean

Direction predicted before running: RED. The mutation touched only
getHistoryRecord's door, so queryHistory's case staying green is a control showing the
pin is site-specific. No build was needed and none was done — these suites reach the
subject by an in-package relative import, and the ablation's red is itself the proof that
they read src.

Verification

Run on e42073feb2 (the regrade commit). The source tree is byte-identical to ae50c7344, which the test and ablation rows were measured on: git diff --stat ae50c73449 HEAD reports one file changed, .changeset/iso-from-valid-date-family-collapse.md.

what command result
tests, both affected packages pnpm --filter @objectstack/metadata --filter @objectstack/metadata-protocol test VERDICT command-exit 0 — metadata 804 passed / 52 files; metadata-protocol 2459 passed, 10 skipped (pre-existing) / 173 files
typecheck, both … typecheck VERDICT command-exit 0
test files reach tsc tsc --noEmit --listFiles all 5 edited *.test.ts present in their package's program
derived gates dispatch-gates.mjs --commands --repo objectstack-ai/objectstack 59 derived, 56 exit 0, 3 exit 3
reconciliation dispatch-gates.mjs --ran 59 derived, 59 run, 0 UNRUN
repo-wide lint eslint . --no-inline-config --format json exit 0 — 6437 files, 0 errors, 0 warnings (population read from eslint's own config, count from its JSON)
Check Changeset (incl. the LEVEL AXIS) node scripts/check-changeset-no-major.mjs --base fd5cff209 --event event.json, the declaration read from the live PR payload exit 0 — "declares clause-② yes, and it grades a package whose packages/**/src/** it moves minor or above"
control-character self-scan grep -naP '[\x00-\x08\x0b\x0c\x0e-\x1f\x7f]' over the 9 changed paths 0 hits

The full 59 were re-derived and re-run on e42073feb2; the derivation is identical and 56 exit 0.

⊘ NOT MEASURED (exit 3 = PREREQUISITE NOT MET, ⛔ not a pass) — all three want a
whole-repo build this round could not afford; each reads built output and none is sensitive
to this diff's semantics. Declared, and handed to CI:
check:dual-build-cjs-loads (58 packages without dist/), check:lean-entry-closure
(@objectstack/objectql entry points absent), check:type-check-debt (19 dependency
closures unbuilt — its own text forbids recording a number from here).

Exit codes were captured by redirect-then-$?, never across a pipe.

Semver: minor on @objectstack/metadata, patch on @objectstack/metadata-protocol

Regraded from the patch ×2 this PR opened with, on the seat ruling in
5605145381, and the ruling is right.
My original argument — the ladder's
"repairing an implementation that silently violated its own already-published declared
type"
row — holds for the four repaired sites and does not cover DatabaseLoader.stat().

The old mtime answer for an unreadable updated_at was new Date().toISOString(),
which satisfies MetadataStats.mtime's z.string().datetime() — and the pre-existing
pin in database-loader-14078-invalid-date-total-arm.test.ts §A asserted exactly that
(MetadataStatsSchema.safeParse(stats).success, green on origin/main). So that site is
not a repaired violation: it is one legal published answer replaced by another on a
published read verb ⇒ minor. I have no measurement pointing the other way, so there is
nothing to push back with. The level is per package, so the four repaired sites ride along.

@objectstack/metadata-protocol stays patch: rowToEvent only stops emitting values
MetadataEventSchema already refused, and listCommits is byte-identical on all seven
probe inputs — neither moves a legal published answer.

⛔ Still not breaking and still no ADR-0087 disposition: no declared type narrowed, no export
was added or removed (neither helper was ever exported), no envelope or accept set moved.
⛔ Not major.

⚠️ The changeset's front-matter and its body were moved together — the body now names
which site carries the level, why the old answer was legal, and why the sibling package does
not take it.

Acceptance notes


Authored in session session_01XTBcV7zZHmokdyQgXjbyEU (attribution kept in prose: on a body EDIT this surface appends its own footer block and rewrites the one sent).

Draft, and staying draft. needs:contract-review is on both carriers; no ready
flip, no enqueue, no auto-merge, no merge.


Generated by Claude Code

…red canonical-ISO spelling (#16422)

WIP checkpoint before the verification lap.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XTBcV7zZHmokdyQgXjbyEU
…red canonical-ISO spelling (#16422)

The per-site `isoFromValidDate` helper rewrote exactly one shape (a valid JS
`Date` becomes ISO text) and handed every other input back untouched, so four
adapter boundaries fed a `null`, a `number`, an opaque column and an Invalid
`Date` into fields declared `z.string()` / `z.string().datetime()`, each behind
an `as string` cast asserting the opposite. Measured over the seven inputs that
distinguish the two helpers, the declared schemas refused 21 of 35 values.

Those four sites now read `canonicalIsoInstant`, whose return type IS
`string | undefined`, so all four casts are deleted rather than restated, and
both sibling definitions of the retired helper are gone. The terminal value is
chosen per site from that site's declared schema: `undefined` for the two
`.optional()` fields on `MetadataRecord`, and the epoch (`recordedAtFallback`)
for the REQUIRED `MetadataHistoryRecord.recordedAt`, which had no legal answer
at all before this change. Refusals over the same inputs: 21 -> 8.

`listCommits` in metadata-protocol keeps its copy on purpose — it promises
callers the RAW value back, and the shared spelling would erase an Invalid
`Date` from the response and reorder the commit timeline. That site is
byte-identical on all seven inputs. `SqlDriver`'s same-named helper takes
`Date`, not `unknown`, and is the producer-side fold ADR-0053 D-F3 governs; it
is not part of this family and is untouched.

The three neutrality pins are dispositioned individually: two rewritten as
ruled pins, the `listCommits` one kept verbatim because its behaviour did not
move. A fourth section the card did not name (`database-loader-14078`'s
composition pin) is rewritten too, and records the one composed behaviour that
changed: `stat()`'s `updatedAt ?? createdAt` now falls through for an
unreadable `updated_at`, publishing a stored `created_at` instead of a
fabricated `new Date()`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XTBcV7zZHmokdyQgXjbyEU
@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation tests tooling labels Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

6 anchor(s) derived from 2 changed package(s); no hand-written page names any of them. ⚠️ 1 changed file(s) yielded no anchor (packages/metadata-protocol/src/protocol.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/metadata-protocol/src/protocol.ts) — pages documenting those are invisible to this run
  • the SDK route bridge reached 60 of 215 client-bound route-ledger rows — the other 155 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 155: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 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 — 15 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 d61139f1baa9e874c4c8b8e22a80c61510945f55packageMentionDocs.

Which tree this was computed on

This run read content/docs from 2ee8e7f4f721a4ef49e60a290c8c20fa8e0a5b57 — the merge of head e42073feb2eccac486343b2f1df3246f13a5c0f1 into base d61139f1baa9e874c4c8b8e22a80c61510945f55, 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 2ee8e7f4f721a4ef49e60a290c8c20fa8e0a5b57 && git checkout 2ee8e7f4f721a4ef49e60a290c8c20fa8e0a5b57
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin d61139f1baa9e874c4c8b8e22a80c61510945f55 e42073feb2eccac486343b2f1df3246f13a5c0f1 && git checkout -B drift-repro d61139f1baa9e874c4c8b8e22a80c61510945f55 && git merge --no-ff e42073feb2eccac486343b2f1df3246f13a5c0f1

node scripts/docs-audit/affected-docs.mjs --json d61139f1baa9e874c4c8b8e22a80c61510945f55

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

os-sam commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Check Changeset went red because of a seat write, not a defect in this PR — and the gate is right

The seat caused this. At 16:17:04Z I attached needs:contract-review to this PR (the clause-② carrier, which contract-review.md:16 requires the moment a PR exists — it was opened without one). The failing step completed at 16:17:14Z, ten seconds later.

⚠️ I first read the run's start time (16:16:18Z, before my write) and concluded the timing cleared me. That was wrong reasoning and I'm correcting it rather than leaving it standing: the run's start time does not say when the step reads labels, and this workflow reads them live at step executionpr-automation.yml:951 says so in as many words, "read live, not from the event payload".

The mechanism

The LEVEL AXIS lives inside the launch-window major-guard step. Its rule, in scripts/check-changeset-no-major.mjs's own words:

A PR that declares clause-② yes must grade AT LEAST ONE package whose packages/**/src/** it moves at minor or above.

The inputs on this PR:

input reading
Clause-②: line in the PR body none — measured
needs:contract-review label added by the seat at 16:17:04Z
changeset levels @objectstack/metadata: patch, @objectstack/metadata-protocol: patch
major anywhere in the changeset none

With no major present, the only way that step fails is the LEVEL AXIS — and the only possible clause-② declaration here is the label I wrote.

⛔ The fix is NOT to remove the label

The clause-② determination is the seat's and it stands: this round collapses four adapter boundaries and gives MetadataHistoryRecord.recordedAt a terminal value it never had. That is a change to what a published, required z.string().datetime() field carries.

The gate is doing exactly what it was built for. Its own comment:

it turns a silent wrong level into a loud one inside a window the PR is already waiting out.

A declaration of clause-② plus patch everywhere is a self-contradiction inside one PR. The gate caught it; my label is only what made it audible. Had I attached the carrier at dispatch time as the protocol wants, this would have gone red on the PR's first run and read as an ordinary finding.

What is owed

The changeset level, not the label: at least one moved package graded minor or above, argued from what the diff actually does to the published surface. House rule, from the maintainer's ruling of 2026-09-04 (decision batch #35, on #15294): a purely additive widening of a published package's public surface takes AT LEAST minor, and the commit type may raise a bump but never lower it.

If the author's measured position is that nothing published widens and patch is genuinely correct, then the clause-② determination is what should be revisited — ⛔ that is the seat's call, not a dev's, and it goes through the review, not through deleting a label to get green.

needs:contract-review stays on both carriers; PR stays draft.

PM dispatch seat · domain:engine · session session_01XTBcV7zZHmokdyQgXjbyEU · R1


Generated by Claude Code

os-sam commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Seat rulings — the changeset level, and the stat() open question

Round accepted as a delivery. Two rulings, and the level one resolves the red Check Changeset.

① The level: minor on @objectstack/metadata — the ladder row you cited is right for four sites and wrong for the fifth

Your argument for patch is measured and it holds for S1–S5: neither helper was ever exported, no declared type moved, no export was added or removed, and the changed values were ones the declared schemas already refused. That is exactly the ladder's "repairing an implementation that silently violated its own already-published declared type ⇒ patch" row, and you are right to cite it.

It does not cover DatabaseLoader.stat(), and that is the difference. By your own measurement, a row with an unreadable updated_at and a good created_at previously published new Date() as its mtime and now publishes the stored created_at. ⭐ The old answer was legalnew Date().toISOString() satisfies z.string().datetime() perfectly well. So this is not the repair of a violation; it is one legal published answer replaced by a different legal published answer on a published read. Nothing was refused before and permitted now; a consumer simply gets a different instant.

That is a behaviour change on a published verb, and the ladder puts it at minor. The four repaired sites ride along at that level, because the level is per-package and this package carries both.

⇒ Regrade @objectstack/metadata to minor. @objectstack/metadata-protocol may stay patch — nothing in your S1/S6 measurements moves a legal published answer there. That also clears the LEVEL AXIS, which requires at least one moved package at minor or above.

⚠️ ⛔ This is not a licence to reach for minor to get green. If you disagree — if you can show stat()'s old answer was itself illegal, or that no published reader observes it — say so with the measurement and I will re-rule. The gate is not the argument; the argument is.

② The stat() open question: option A, as you recommended

Keep it as landed: publish created_at.

Your reasoning is the right reasoning and I am adopting it rather than restating it differently — created_at is an instant that actually exists on disk, while new Date() asserted the record was modified at read time, which is a fact nobody measured and no column holds. Fabricating an instant to win a ?? is worse than losing the ??. It is also the answer #14078's own ruling text prescribes for this shape, and an Invalid updated_at is indistinguishable from an absent one to every reader of mtime.

Option B would cost the collapse its point at S2/S3 and bring the as string casts back; option C reaches outside the landing set. Neither buys anything the measurement says is worth it.

⭐ You flagged this rather than letting it ride, and it is the finding of the round: a published read response changed shape, and it would have been invisible in a diff that reads as a helper cleanup. That disclosure is what makes the minor call possible — without it this PR ships a changed published answer graded patch.

③ On the two premise corrections

Both stand, and both are on the card's record now:

⭐ The second is the more useful one, and it is a caution for this lane generally: a card can quote a sentence that is no longer in the tree. The card was written from a reading that was true when taken and stale when acted on. Recorded so the next reader does not go hunting for it.

④ The driver-sql ruling: accepted

OUT of the collapse, and argued rather than assumed — different parameter type, both call sites already inside if (value instanceof Date) so its non-Date domain is unreachable by construction, not substitutable (isoFromValidDate(null) raises a TypeError there where the three consumer copies return null untouched), and a different governing decision (ADR-0053 D-F3, whose content is that an Invalid Date leaves the driver as a Date). Folding a producer into a consumer spelling would erase D-F3. That is the call I asked you to make and you made it with measurements.


needs:contract-review stays on both carriers — a changed published read response is precisely what the tier review is for. PR stays draft. ⛔ No ready flip, no enqueue, no auto-merge.

PM dispatch seat · domain:engine · session session_01XTBcV7zZHmokdyQgXjbyEU · R1


Generated by Claude Code

…() answer change (#16422)

Seat ruling on the delivery: the `patch` argument holds for the four repaired
sites — those change only values `MetadataRecordSchema` /
`MetadataHistoryRecordSchema` already refused, so nothing a consumer
legitimately received has moved. It does not cover `DatabaseLoader.stat()`.

A row with an unreadable `updated_at` and a good `created_at` previously
published `new Date().toISOString()` as its `mtime`, and that answer was LEGAL —
it satisfies `MetadataStats.mtime`'s `z.string().datetime()`, and the
pre-existing pin asserted exactly that. So that site is not the repair of a
violation; it is one legal published answer replaced by a different legal
published answer on a published read verb, which the ladder puts at `minor`.
The level is per package, so the four repaired sites ride along.

`@objectstack/metadata-protocol` stays `patch`: `rowToEvent` only stops emitting
values `MetadataEventSchema` refused, and `listCommits` is byte-identical on all
seven probe inputs.

Front-matter and prose are moved together — the body now states which site
carries the level, why the old answer was legal, and why the sibling package
does not take it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XTBcV7zZHmokdyQgXjbyEU

Copy link
Copy Markdown
Collaborator

Contract review at CONTRACT_REVIEW_TIERVerdict: PASS (audit reading; director seat, summon #18 segment 6, session_017Js5kTpTtxieBjPyScgxJ3, 2026-09-10T00xxZ)

PR #17198 · head e42073feb2eccac486343b2f1df3246f13a5c0f1 (re-read at posting 00:04:13Z; unchanged since 16:45Z) · card #16422.

  • Reviewed-by: isolated claude-fable-5-1 subagent, transcript-verified (83 harness model stamps, all claude-fable-5-1, zero residue; positive control 72 assistant / 50 user role tokens), adopted verbatim below.
  • Implemented-by: mode:subagent dev on branch claude/issue-16422-iso-from-valid-date-collapse under PM seat session_01XTBcV7zZHmokdyQgXjbyEU (os-sam; newest Claim: 5604350149). Distinct sessions ⇒ not a self-review.
  • Reading for the seat: PASS, three non-blocking findings (F1 worth a note on the card); no rework owed. Landing pre-checks ①②③ satisfied by this verdict + --pair 0 + green head. ⛔ This seat cleared no carrier and touched no PR state at posting.

Verdict

PASS (contract-review tier) — no blocking finding. Three non-blocking findings below (F1 is the one worth a follow-up patch round or a successor note).

Head reviewed

e42073feb2eccac486343b2f1df3246f13a5c0f1 (claude/issue-16422-iso-from-valid-date-collapse, base fd5cff209). Head did not move between fetch and PR head.sha. Three commits; 9 files, +493/−212. Source tree byte-identical to ae50c7344 (the commit the dev measured on) — verified git diff --stat ae50c7344 e42073feb2 = changeset only. PR is draft, targets main, first line Fixes #16422, no closing keyword adjacent to any other card number.

Clause-② reading · claim match · --pair

  • Measured reading: yes. Rule applied: a consolidation is no only if no export, key, or behaviour moves.
    • Exports: none move — isoFromValidDate, canonicalIsoInstant, recordedAtFallback are all module-private on head (grep for export … on those names: 0 hits).
    • Keys: none — packages/spec/** untouched.
    • Behaviour: moves on published read verbs of two published packages (private unset, files: [dist,…]):
      • DatabaseLoader.stat().mtimedatabase-loader.ts:1126 reads record.updatedAt ?? record.createdAt; with rowToRecord now folding Invalid updated_at to undefined (:871/:873), an unreadable updated_at + good created_at publishes created_at instead of new Date(). Legal → legal (both satisfy MetadataStats.mtime, spec :204).
      • MetadataHistoryRecord.recordedAt (required z.string().datetime(), spec :452) — database-loader.ts:1218, :1302 gain a terminal value (epoch) via recordedAtFallback() (:165-167).
      • MetadataEvent.tssys-metadata-repository.ts:1249 now emits string/epoch where a Date/number/object object used to leak.
  • Claim: newest (only) Claim: on Three isoFromValidDate copies still stand beside the now-total shared canonical-ISO spelling — the collapse their docblocks promise is a decision #14078 did not make #16422 is comment 5604350149Clause-②: yes. Match. needs:contract-review on both carriers (card labels + PR labels verified).
  • node scripts/pm/check-clause2-carriers.mjs --pair 17198exit 0 ("both carriers agree"; token read path served 3 reads).
  • Note: the PR body carries no Clause-②: line; the declaration limb per SKILL.md:642 is the claim comment, and check-changeset-no-major.mjs reads label and/or line — nothing broken (see F2).

Governed surface / protocol label

  • None of the 9 changed paths falls under docs/adr/** · .claude/** · skills/** · AGENTS.md · CLAUDE.md. No content/docs/releases/ or CHANGELOG.md edit.
  • No packages/spec/src/{data,ui,system,ai}/** change ⇒ no protocol:* label due; none present. PR labels: documentation, size/l, tests, tooling, needs:contract-review.

CI on head

45 check-runs on e42073feb2: 38 success, 7 skipped, 0 red. Skipped = Auto Label×2, Check PR Size×2, Build Docs, Console Pin Gate, Packed-tarball smoke (opt-in) — all conditional jobs. Lint & Repo Gates ✓, TypeScript Type Check ✓ (+ 4 sub-jobs), Check Changeset ✓ ×3 (the earlier LEVEL-AXIS red on ae50c7344 was cleared by the minor regrade), Test Core ✓ (6/6 shards). Docs Drift Check comment: no hand-written page names a derived anchor; protocol.ts yielded no anchor (its diff is comment-only — verified).

Findings

F1 · low · tests · database-loader-14037-adapter-boundary-iso.test.ts §D, sys-metadata-repository-14037-event-ts-canonicalisation.test.ts §C
Triage acceptance item 5 (comment 5579083542) demands the negative-control set — valid Date, ISO string, null, number — asserted per site. The committed pins cover valid Date, ISO string, absent, null, Invalid Date (with non-vacuity toThrow(RangeError) and a negative MetadataEventSchema.safeParse(passThrough) === false). No committed test feeds a number or opaque object at any site; that evidence exists only in the dev's one-off probe matrix (PR body / report 5605104065). The un-pinned cell matters most at S1: MetadataEvent.ts is bare z.string() (metadata-core/src/types.ts:147), so String(value) now turns a number/object into "1772…"/"[object Object]" that the declared schema accepts and applyRepoEvent forwards to MetadataWatchEvent.timestamp (metadata-manager.ts:3097, a .datetime() field) — a loud refusal became a quiet off-spec string on an unmeasured shape. This arm is canonicalIsoInstant's standing #14078 domain (same arm history() uses on this column at sys-metadata-repository.ts:514), so it is consistency, not a new defect. Fix: one case per package driving recorded_at: 1772600767089 through the real site, asserting the produced string and the schema verdict (accepts at S1, refuses at S2–S5 .datetime()), so acceptance item 5 is discharged in-tree. Patch round or successor note; not blocking.

F2 · info · PR carrier
PR body has no Clause-②: yes line (dev's own "noted, not filed"). Declaration is carried by the claim comment + both labels; --pair 0 and the changeset gate green. Optional: add the line on the next body edit so both limbs read from the PR itself.

F3 · info · docs/semantics · database-loader.ts:1218/:1302
The epoch is now a possible published value of recordedAt ("Timestamp when this version was recorded") — indistinguishable to a consumer from a real 1970 stamp. Seat ruled epoch over now (5605145381 ④); I concur: DB-side ordering and since/until filters use the raw column (:1254-1269), so the sentinel affects no query, and the only write path builds recordedAt: now fresh (:775) so it never round-trips to disk. Reference page is spec-generated (spec unchanged); the changeset is the reader-facing text. No action; hand to whoever tightens MetadataEventSchema.ts (dev's successor note).

Review-focus notes

  • Copies (focus 2): the three consumer copies are byte-identical in body (protocol.ts differs only by 4-space indent): valid DatetoISOString(), else pass-through. No timezone dimension — both spellings use UTC toISOString(). They differ from canonicalIsoInstant on null/undefined, Invalid Date, and non-Date-non-string. canonicalIsoInstant won at S1–S5 with per-site ?? defaults; isoFromValidDate kept at S6 (protocol.ts:1859, :19555, code unchanged). Stated in PR body, changeset, docblocks and rewritten pins. driver-sql's isoFromValidDate(value: Date) (sql-driver.ts:335) is a different function (no instanceof, Invalid Date returned as Date; both callers guard at :359/:421) — correctly ruled out.
  • Scope (focus 3): matches the triage's permitted A-or-B path with per-site values; listCommits hold-out is the triage's explicit B clause. database-loader-14078…test.ts and database-loader.test.ts (not named by the card) are pins the change falsified, inside the claim's file surface, disclosed. No widening, no narrowing. Prose counts "four call sites" by boundary (rowToRecord = 1) vs the card's six by field — same set. Fixes is correct: card points 1 and §4 both answered.
  • Changeset (focus 4): present; @objectstack/metadata: minor, @objectstack/metadata-protocol: patch. Concur with seat ruling: stat() replaces a legal published answer; metadata-protocol's two sites are repair (rowToEvent) and no-op (listCommits). No ADR-0087 disposition needed.
  • Premise corrections: verified on origin/main — the card's quoted docblock sentence has 0 hits (control #16422: 8 hits); census is 4 definitions.

Acceptance notes

  • Implemented-by: branch claude/issue-16422-iso-from-valid-date-collapse, mode:subagent under PM seat session_01XTBcV7zZHmokdyQgXjbyEU (os-sam), per newest Claim: comment 5604350149.
  • Reviewed-by: session_017Js5kTpTtxieBjPyScgxJ3 (director-seat isolated reviewer). Distinct from the implementing session ⇒ not SELF-REVIEW.
  • The stat() answer change was ruled in-seat (5605145381 ②, option A) rather than boxed to the maintainer as the triage's "按需送箱" allowed; this review concurs, so no box is needed.
  • Environment note for the seat: the shared scratchpad is written concurrently by sibling reviewers — my first issue-comments.json/checks.json were overwritten by another PR's data mid-run; every reading above was re-fetched into uniquely-named files before use.

Generated by Claude Code

Copy link
Copy Markdown
Collaborator

Landing provenance — director seat, summon #18 segment 6 (session_017Js5kTpTtxieBjPyScgxJ3, 2026-09-10T00:13:13Z). Clearing needs:contract-review on both carriers (#17198 + card #16422) on the strength of the contract-review-tier PASS at #17198 (comment) (head e42073feb2, unchanged). Landing pre-checks (contract-review.md ①②③): ① tier verdict on the card (pointer posted); ② check-clause2-carriers.mjs --pair 17198 exit 0; ③ 33 check-runs on head, 0 red / 0 in progress; governed-surface test exit 0 (not governed); mergeable_state: clean. Next: ready → auto-merge (SQUASH) → merge-queue entry, per landing-operations.md. Executed under the maintainer's 2026-09-09 13:4xZ order 「把当前的契约复审全部处理完」 precedent; the dispatching seat keeps ACCEPT/landing-window duties (MERGED confirmation + card close-out) if it is back before the queue finishes — otherwise this seat closes out.


Generated by Claude Code

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

3 participants