Skip to content

fix(metadata-protocol): listCommits emits the ISO-8601 string createdAt declares - #15008

Open
os-musk wants to merge 7 commits into
mainfrom
claude/issue-14038-listcommits-createdat-iso
Open

fix(metadata-protocol): listCommits emits the ISO-8601 string createdAt declares#15008
os-musk wants to merge 7 commits into
mainfrom
claude/issue-14038-listcommits-createdat-iso

Conversation

@os-musk

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

Copy link
Copy Markdown
Collaborator

Fixes #14038

The defect

listCommits's declared return type says createdAt?: string
(packages/metadata-protocol/src/protocol.ts:19049), but the mapping
assigned the raw driver value straight through
(protocol.ts:19107 before this change:
...(r.created_at ? { createdAt: r.created_at } : {})). created_at on
sys_metadata_commit is an engine-injected audit column — not in
datetimeFields — and SqlDriver#formatOutput repairs it only inside its
if (this.isSqlite) arm (packages/drivers/driver-sql/src/sql-driver.ts:16056,
audit-column repair at :16123, the datetimeFields fold at :16137
both inside the same arm). So on Postgres and MySQL, listCommits handed
every in-process consumer a Date in a field the type promised was a
string. The REST door (GET /packages/:id/commits) was never affected —
JSON.stringify already renders a Date as canonical ISO-Z text.

The fix

A narrow per-site conversion at the producer
(protocol.ts:1726, isoFromValidDate), following #14037's
precedent (landed as #14939) rather than the shared canonicalIsoInstant
spelling: a valid Date is canonicalised to ISO-8601, and every other
shape — an already-canonical SQLite string, an absent column, and
an Invalid Date — passes through unchanged. #14037 deliberately did
not adopt canonicalIsoInstant here because #14078 measured an Invalid
Date reachable on both live dialects (a MySQL zero datetime; a Postgres
year in 275760..294276) where that spelling raises RangeError, and #13973
is blocked on that ruling. This PR imports neither answer to #14078 — it
just doesn't regress the field it touches. The Invalid-Date neutrality is
pinned in §D of the new test file so it reds if the contested spelling is
ever swapped in here.

Evidence (Zone 2, file:line)

  • A — declared packages/metadata-protocol/src/protocol.ts:19049;
    emitted (pre-fix) protocol.ts:19107 (r.created_at, unconverted).
  • B — route: narrow per-site conversion, not canonicalIsoInstant; see
    the doc comment on isoFromValidDate (protocol.ts:1726) and §D of the
    new test.
  • C — Postgres/MySQL-only divergence confirmed: sql-driver.ts's
    formatOutput gates both the builtin-audit-column repair and the
    datetimeFields fold inside one if (this.isSqlite) arm
    (:16056..:16137). No live cell needed — the new test drives a
    hand-made Date, matching this package's existing convention (no driver
    dependency).
  • D — one real in-process consumer: rollbackToPackageCommit
    (protocol.ts:19742) reads listCommits' createdAt through
    compareAuditInstants, which already normalises both a Date and an
    ISO string to epoch ms via canonicalVersionInstant — so canonicalising
    the emitted value to a string does not change its behaviour (no stop
    condition; not a regression).

Scope

Out of scope, per the dispatch: #14938 (listDrafts, a sixth site with
an inline return type) and the family's standing question of a shared
@objectstack/metadata-core export (ruled: after #14078, not before).

Tests

New: packages/metadata-protocol/src/protocol-14038-list-commits-created-at-iso.test.ts
(§A Postgres/MySQL Date → ISO; §B SQLite text passthrough; §C absent
column; §D #14078 neutrality pin).

Union run at final head 8c9c060f2 (after merging origin/main twice while
this branch was in flight, most recently to pick up #14939 itself):
pnpm --filter @objectstack/metadata-protocol exec vitest run over the new
file plus the sibling #13995 and #14037 pins in this package — 3 files,
13 tests, all passed; pnpm --filter @objectstack/metadata-protocol typecheck
clean; pnpm check:engine-double-contract, check:system-context-census,
check:doc-authoring, check:nul-bytes, check:type-check-coverage, and
the rest of the ~70 commands node scripts/pm/dispatch-gates.mjs --commands
derives for this diff — all green. check:dual-build-cjs-loads,
check:test-completeness and check:type-check-debt read NOT MEASURED
locally (each needs a full-tree build or a CI test log this environment
doesn't produce) — not evidence of a red, left for CI.

Mechanical fallout from merging origin/main twice: two
check:system-context-census re-anchors (doc-table line numbers shifted by
unrelated commits) and one check:engine-double-contract pinned-ledger
entry for the new test's read-only fake engine. No behaviour change in
either.


Generated by Claude Code

…At declares

`listCommits`'s declared return type says `createdAt?: string`, but the
mapping assigned the raw driver value straight through. `created_at` is an
engine-injected audit column, and `SqlDriver#formatOutput` repairs it only
inside its `if (this.isSqlite)` arm, so Postgres and MySQL hand it out of the
record read door as a JS `Date` — a value every in-process consumer received
in a field the type promised was a `string`.

Follows #14037's precedent: a narrow per-site `isoFromValidDate` helper
converts the one measured shape (a valid `Date`) and returns every other
shape, including an Invalid `Date`, unchanged — deliberately not the shared
`canonicalIsoInstant` spelling, which raises RangeError on an Invalid `Date`
reachable on both live dialects (#14078, on which #13973 is blocked).

Fixes #14038

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68
check:system-context-census pins doc-table anchors by line number; the
listCommits fix above inserted ~40 lines earlier in protocol.ts and shifted
one anchored elevation-read site. Mechanical re-anchor via
`node scripts/check-system-context-census.mjs --fix` — no behaviour change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68
check:engine-double-contract requires every findOne/update/delete fake
engine double in a test file to be registered in the pinned ledger.
Registers the read-only findOne double the new #14038 pin test uses.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68
Merging origin/main shifted lines in identity-write-guard.ts, auth-plugin.ts
and share-link-service.ts (unrelated incoming commits); mechanical re-anchor
via `node scripts/check-system-context-census.mjs --fix` — no behaviour
change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68
@github-actions github-actions Bot added the size/m label Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

3 anchor(s) derived from 1 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 — 9 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 23c72be3cfe7a282da3a07784ad9922f1b860cc8packageMentionDocs.

Which tree this was computed on

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

node scripts/docs-audit/affected-docs.mjs --json 23c72be3cfe7a282da3a07784ad9922f1b860cc8

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

os-musk commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review — PASS; marked ready and armed for the merge queue

An independent reviewer (not the author) re-derived this PR from the card, the ruling and the code rather than from the PR body. Verdict PASS, no blocking defects. What was re-proved rather than accepted:

The helper decision is not an implementer's invention — it executes a ruling that predates #14037. Triage's ruling on #14038 (comment 5487875301) rules option 1 (canonicalise at the mapping) and names this route explicitly: 「与 #13997 走的、以及 #14037 将走的是同一条路」. So the local-helper pattern was pre-authorised before #14037 existed, and #14939 (which closed #14037) then landed the byte-identical isoFromValidDate — same name, same body, same !Number.isNaN(value.getTime()) guard — into two files with the same docblock reason. This PR is the third faithful instance, not a competing idiom.

The three premises behind refusing canonicalIsoInstant were each verified rather than taken on trust: it does throw on an Invalid Date (sys-metadata-repository.ts:118-123value.toISOString() with no NaN guard), #14078 is genuinely open and genuinely about that (bug, priority:p1, pm:retriage), and #13973 is genuinely pm:blocked on it.

The reviewer found stronger evidence for the choice than the PR argues for itself. Tracing what would happen if the RangeError were allowed to propagate: listCommits' own try/catch (protocol.ts:19122-19127) routes it through rethrowUnlessMetadataStoreUnprovisioned, which converts it into a generic "metadata store could not be read". Adopting the contested spelling here would not merely risk a loud 500 — it would mislabel a code defect as a store outage. Confirmed by the ablation stack trace below.

Ruling compliance on the one instruction that constrains the prose. The ruling also says ⛔ 「PR说明里不要把这次修复写成「行为变更」」 — the fix honours a declaration rather than changing behaviour. Verified: no "behaviour/behavior change" framing appears in this PR's own text (the two hits in protocol.ts are pre-existing lines elsewhere).

Changeset patch, against the direct precedent. #14939's changeset, quoted: '@objectstack/metadata': patch / '@objectstack/metadata-protocol': patch — the identical defect class, matched exactly. The declared type does not move (createdAt?: string before and after); only the runtime value an in-process Postgres/MySQL caller receives does. That is not published type narrowing (no minor + **BREAKING** + adr-0087: owed) and adds no export or payload key.

Clause-② = no. isoFromValidDate is declared without export at protocol.ts:1726 and is re-exported nowhere; createdAt already existed on listCommits' return type (:19049) and on ListPackageCommitsResponseSchema (packages/spec/src/api/package-lifecycle.zod.ts:65, untouched).

Symbol fence, resolved on this head rather than from quoted numbers. listCommits opens 19036 and closes 19128; revertCommit opens 19160 — 32 lines clear of #14982's territory, and far from #13609's receipt path (~:5062-5199). The :19107 figure that appeared in an earlier reading as revertCommit's start came from a different base; on this head it is inside listCommits.

Two ablations, both directions, restore proven by state. Restoring the raw assignment reddens exactly §A (1 failed | 3 passed). Dropping the !Number.isNaN guard — i.e. importing the contested spelling's consequence — reddens exactly §D, which proves the neutrality pin is not vacuous. Both restores verified by blob-hash equality (c0730b3…), empty git diff HEAD and empty git status --porcelain, never by an exit code.

Consumers searched independently. rollbackToPackageCommit normalises both shapes to epoch ms via compareAuditInstantsauditInstantMscanonicalVersionInstant (.getTime() for a Date, Date.parse for a string — no throw), confirmed by reading the chain. The only other in-process caller is the REST handler (packages/runtime/src/domains/packages.ts:803-809), which never reads createdAt itself. Eleven test files mentioning listCommits carry no createdAt assertions outside the new and sibling pins. No missed consumer.

Merge fallout resolved the right way. The two check:system-context-census re-anchor commits are tool-generated (--fix) and touch only line-number columns; the generator was re-run on the final head (OK — 106 elevation read sites, all anchored). The engine-double-contract ledger diff is purely additive and carries both #14037's four rows and this card's one simultaneously.

Gates: 72 commands derived off the merge base with no path arguments — 68 exit 0, 4 genuine PREREQUISITE NOT MET, and 4 that first failed on an unbuilt prerequisite and passed clean once built. check-governed-merges --test: NOT governed. All 37 check runs on head 8c9c060f2 are success or skipped, zero red — ready, and auto-merge armed.

One non-blocking fast-follow, recorded not filed

protocol.ts already imports from ./sys-metadata-repository.js (:31), and that same-package file already declares a byte-identical, unexported isoFromValidDate (:165). Exporting it within the package and importing it — a same-package import, not a new package export, so clause-② and the public surface are untouched — would remove the duplication. Deliberately not filed as its own card: whatever resolves #14078 will replace all three local copies at once, so this cleanup belongs to that card's sweep rather than to a card of its own that would be closed by it.


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/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

metadata-protocol: listCommits declares createdAt as a string but emits the raw driver value, so it hands consumers a Date on Postgres/MySQL

2 participants