Skip to content

fix(metadata-protocol): revertCommit refuses a non-canonical stored type on its restore limb, with the wire code its sibling doors give (#9174) - #9360

Merged
os-zhuang merged 3 commits into
mainfrom
claude/issue-9174-revertcommit-stored-type-preflight
Aug 17, 2026
Merged

fix(metadata-protocol): revertCommit refuses a non-canonical stored type on its restore limb, with the wire code its sibling doors give (#9174)#9360
os-zhuang merged 3 commits into
mainfrom
claude/issue-9174-revertcommit-stored-type-preflight

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#9174

revertCommit was the third consumer of an at-rest type and the only one without a STORED_TYPE_NOT_CANONICAL pre-flight. Its restore limb handed the raw stored spelling to the registry write-through, where #9111's mint door refuses it and the throw lands in the best-effort catch — so an operator got a server-side console.warn where both sibling doors give a named, wire-visible verdict.

Premise re-verified at HEAD (4906c900f), not inherited

Still open. protocol.ts at HEAD carries #9111's own producer trace, which names this route:

 5. `revertCommit` → write-through — `PLURAL_TO_SINGULAR[it.type]`
over a STORED commit-item type. **Unguarded.**

What was measured, per limb, before any shape was chosen

Driven end to end over the real SysMetadataRepository on an unscoped kernel, with a fields/showcase_task.title row at rest (the harness is now protocol.revert-stored-type-canonical.test.ts):

RESTORE limb (existedBefore: true, prevVersion: 1)

{ success: true, revertedCount: 1, failedCount: 0,
reverted: [ { type: 'fields', name: 'showcase_task.title', action: 'restored' } ],
failed: [] }
registerItem calls: 0
console.warn: [Protocol] registry write-through failed for fields/showcase_task.title:
[registry_type_not_canonical] Refusing to register a SchemaRegistry
overlay entry under the non-canonical metadata type 'fields' …
stored head: rolled back to V1

The receipt claims the pre-commit body is what the platform now serves. For this class it cannot be: the mint door refuses the entry and boot refuses it too, so the restored body reaches no reader at all.

SOFT-REMOVE limb (existedBefore: false)

{ success: true, reverted: [ { type: 'fields', name: 'legacy_only', action: 'removed' } ],
failed: [] }
rows left in sys_metadata: none
console.warn calls: 0 registry keys touched: none

Nothing about that outcome is wrong.

The shape: saveMetaItem's refusal, not migrateStoredMetadata's decline

Matched sibling: saveMetaItem — a wire-visible coded refusal, carried on this door's existing per-item failed[] channel, the same one VERSION_NOT_FOUND, ITEM_LOCKED and NOT_OVERRIDABLE already ride. No new receipt surface and no new error code.

The test that separates the two siblings is whether the door can do what it promises for this row:

doorcan it deliver its promise here?verdict
migrateStoredMetadatano — rewriting a stored type spelling is an identity move, out of reach entirelydecline, outcome: 'skipped' (must not poison storedMigrationClean, which runs forever)
saveMetaItem / publishPackageDraftsyes, and doing so mints the dead namespacerefuse, coded
revertCommit restore limbyes at the row — and it still delivers none of what restored promisesrefuse, coded

So the item is refused and success goes false: the commit the operator asked to undo was not undone. A one-shot operator action has no forever to poison, and answering success: true over a deliberately untouched item is the class of lie #5980 and #6621 already paid for in this function.

Also measured: what the caller can DO with each answer. With the refusal the caller gets the stored spelling, the canonical spelling, the PUT /meta/field/… re-author path, and the note that POST /meta/_migrate-stored will not fix it either — the operator does not bounce between two doors. With today's console.warn the caller gets a receipt saying the revert worked.

The soft-remove limb is deliberately outside the gate

It performs its promise exactly and completely, and the removal is the one action that makes this residue smaller. Refusing it would answer success: false for a revert that fully succeeded, and would hand back an instruction — "drop the fields row" — naming the very operation it had just declined to perform. #8908's own predicate is scoped the same way and for the same reason: it excludes the manifest-PRESENT plurals because widening "would change a wire-visible failed[].code for rows that are NOT this defect".

This is a scope decision, not a softened gate, and ablation 3 is what keeps it honest.

No ledger key is touched

The refusal writes no audit row and no commit record, and carries it.type into failed[] verbatim. #9161's ruling — the caller's spelling reaches the ledger keys unfolded so AUDIT_TYPE_NOT_CANONICAL fires loudly — is untouched in both directions. A refused item is simply absent from reverted[], so the append-only revert commit built from it never claims an undo that did not happen (pinned in §4).

The publish pre-flight audits its refusals; this one does not, on purpose. revertCommit writes no audit row for any of its per-item failures, so a row minted only for this class would be a lone entry in a ledger the function otherwise never writes — and it would need the type folded to be readable, which is the one move the card rules out.

Predicate: the narrow at-rest one

isNonCanonicalStoredType, not the complete canonicalMetaType(t) !== t. objects/views fold in the manifest map, so the restore limb already hands the write-through a canonical key and those rows revert and register correctly today. Ablation 4 pins that.

Tier fence

No new error code. STORED_TYPE_NOT_CANONICAL already exists and is already listed under @objectstack/metadata-protocol in packages/spec/src/api/error-code-ledger.zod.ts:381. packages/spec is untouched — no clause ②.

Reviewer note, deliberately not acted on: that ledger line's trailing comment now names one of two producers ("refused at the publish pre-flight, batch-atomic"). Amending it means editing packages/spec, which pulls in a different gate family than this diff has, so it is left for whoever next touches the ledger.

Changed line ranges (git diff --unified=0 vs origin/main) — protocol.ts is the hot file

@@ -11450 +11450,3 hydrateOverlayIntoRegistry header — route 5 is no longer "unguarded"
@@ -11487 +11489,4 …and the loudness-per-route paragraph beside it
@@ -16154,0 +16160,10 revertCommit TSDoc — the new pre-flight, stated
@@ -16233,0 +16249,122 the pre-flight itself, at the top of the per-item loop
@@ -16353 +16490 the restore branch condition -> `restoreToVersion !== null`
@@ -16398 +16535 `repo.restoreVersion(ref, restoreToVersion, …)`

Only two of the six are executable-line changes to existing code; the two large hunks are the gate plus its reasoning, and the first two are comment corrections made necessary by the change.

Other files: protocol.hydrate-overlay-canonical-type.test.ts (7 lines, route-5 header correction only — no assertion changed, its call-site count pin is untouched at 3), the new test file, and the changeset.

Ablation legs, predicted before running

legpredictedobserved
1. ship stateGREENGREEN, 10/10
2. pre-flight block deletedRED in §1RED, 6 failed — broader than predicted. §4 goes red with §1: a gate that refuses nothing lets the item back into reverted[] and therefore into the revert commit. §2 and §3 stayed GREEN, which is the half that matters. Recorded as observed, not trimmed to the prediction. This leg is the pre-fix measurement quoted above.
3. restore-limb guard dropped (both limbs gated)RED in §2RED, exactly 1: "removes a commit-created non-canonical row and reports success"
4. predicate widened to canonicalMetaType(t) !== tRED in §3RED, exactly 1: "a manifest-PRESENT plural is NOT refused"

Leg 2 first ran red-but-unreadable on eight tests for a reason that had nothing to do with the ablation: vi.spyOn(console, 'warn') was restored at the end of each test body, so the first failing assertion skipped the restore and the spy accumulated into every later test. The spy now restores from afterEach. A leg that cannot be read is a leg that was not run, so this is recorded rather than quietly fixed.

Verification — all at c40b70024, the final commit

pnpm --filter '@objectstack/metadata-protocol^...' build build closure, green
pnpm --filter @objectstack/metadata-protocol build DTS/typecheck green
pnpm --filter @objectstack/metadata-protocol test 120 files, 1651 tests passed
pnpm exec turbo run build --filter='./packages/*' … 70/70 tasks
pnpm check:type-check-debt --re-measure OK, 33 entries, none above its number

Gate union re-derived from the actual changed paths with node scripts/pm/dispatch-gates.mjs and run at c40b70024: check:nul-bytes, check:error-code-casing, check:engine-double-contract, check:where-matcher, check:query-options-erasure, check:cross-package-test-inputs, check:durability-log-level, check:filter-alias-parity, check:changeset-gate-self-tests, check:objectui-changeset, check:type-check-coverage, check-adr-0087-registration, check-changeset-no-major, check-empty-changeset, docs-audit/check-affected-docs — all PASS.

Two defects in my own work were caught by running the pipeline rather than reading it, and both are recorded because each was invisible to one half of it:

  1. TS2345 at restoreVersion. Hoisting the limb condition into a boolean broke TypeScript's narrowing on it.prevVersion, a mutable property whose narrowing does not survive an aliased boolean. All 1651 tests stayed green — vitest transpiles without type-checking — and only the DTS build saw it. Fixed by binding restoreToVersion: number | null, which is still one source of truth for both the pre-flight and the branch, and narrows.
  2. The shrink-only DEBT ratchet moved 63 → 69 on six TS6133 unused warn bindings left behind when the spy restore moved into afterEach. Back to 63; the ratchet re-measures clean. No TEST_DEBT entry was raised and no ledger number was touched.

Generated by Claude Code

…ype on its restore limb (#9174)
The third at-rest door joins the two that already name this class on the wire.
Measured per limb at HEAD: the restore limb answered success with
reverted[].action 'restored' while registerItem was never called and the only
trace was a server-side console.warn from the write-through's best-effort catch;
the soft-remove limb performs its promise exactly and is deliberately left
outside the gate.
Matches saveMetaItem's refusal shape on this door's existing per-item failed[]
channel: code STORED_TYPE_NOT_CANONICAL, which already exists and is already in
the error-code ledger for this package. No fold on any ledger key, no audit row,
no new receipt surface.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj
…onst (#9174)
The hoisted boolean broke TypeScript's narrowing on `it.prevVersion` — a
MUTABLE property, so a narrowing does not survive an aliased boolean — and
`repo.restoreVersion(ref, it.prevVersion, …)` stopped compiling with
`TS2345: 'number | null' is not assignable to 'number'`. Caught by the DTS
build while all 1651 package tests stayed green, because vitest transpiles
without type-checking. `restoreToVersion` is the same single source of truth
for both the pre-flight and the branch, and narrows.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj
…restore left behind (#9174)
Moving the spy restore into `afterEach` (so a failing assertion cannot leak the
spy into the next test) left six `const warn = spyWarn()` bindings unread. tsc
counted them (TS6133) and the shrink-only DEBT ratchet for this package went
63 -> 69. Back to 63.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj
@github-actionsgithub-actionsBot added size/l documentation Improvements or additions to documentation tests tooling labels Aug 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/metadata-protocol, touching 3 documentable anchor(s).

1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/concepts/metadata-lifecycle.mdx(via ObjectStackProtocolImplementation (symbol))

2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via ObjectStackProtocolImplementation (symbol))
  • content/docs/releases/v17.mdx(via ObjectStackProtocolImplementation (symbol))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see

Coarse fallback — 5 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 origin/mainpackageMentionDocs.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@os-zhuangClaude

Copy link
Copy Markdown
ContributorAuthor

PM review — the refuse/decline choice was the work, and it was made by measurement

⛔ Not flipping yet; checking CI job-by-job. ⚠️ Note the queue now runs the full suite as a required check (#9283 Option A), so landing is slower than earlier today.

⭐ The discriminator is the contribution

I dispatched this saying the real work was choosing between refuse and decline-and-report, and that measuring what the caller can do with each answer had to come first. You produced a discriminator sharper than the one I gave you:

whether the door can do what it promises for this row

and then applied it three ways rather than asserting it. migrateStoredMetadata cannot rewrite a stored spelling at all → decline. saveMetaItem can, and doing so mints a dead namespace → refuse. revertCommit's restore limb can act at the row and still delivers none of what restored promises → refuse. That third line is the one a weaker analysis collapses, and the receipt proves it:

success: true, revertedCount: 1, action: 'restored'
registerItem calls: 0
console.warn: [registry_type_not_canonical] Refusing to register …

A receipt asserting the pre-commit body is what the platform now serves, for a body that reaches no reader at all.

⭐ The soft-remove exclusion is the part I would have got wrong

Left un-gated because it performs its promise exactly, and because refusing it would answer success: false for a revert that fully succeeded and hand back an instruction naming the very operation it had just declined to perform. That second clause is the kind of thing you only see by imagining the operator reading the message. The #8908 precedent for the same scoping, plus ablation 3 pinning it, turns a judgement call into a defended one.

The discipline that made the rest trustworthy

  • Ablation leg 2 came back broader than predicted (6 red, not §1 alone) and you "recorded as observed, not trimmed to the prediction." A prediction that survives by being edited afterwards measures nothing.
  • The unreadable leg was reported, not quietly repaired.vi.spyOn(console,'warn') restoring at the end of the test body meant a failing assertion skipped the restore and the spy bled into every later test. "A leg that cannot be read is a leg that was not run." That sentence belongs in the playbook.
  • Both self-caught defects were invisible to one half of the pipeline, which is why running it beat reading it: TS2345 passed 1651 green tests because vitest transpiles without type-checking and only the DTS build saw it; and the shrink-only ratchet moved 63 → 69 on six TS6133 bindings left by the spy move. Back to 63, no TEST_DEBT raised, no ledger number touched — under real pressure, on your own mess, "ledgers may only shrink" held.
  • Changed line ranges declared as asked, with the honest gloss that only two of six hunks are executable changes to existing code.

Tier fence — held, including the tempting part

No new error code; STORED_TYPE_NOT_CANONICAL already exists at error-code-ledger.zod.ts:381; packages/spec untouched. ⭐ And you noticed that line's trailing comment now names one of two producers and deliberately did not fix it, because amending it means editing packages/spec and pulling in a different gate family. Correct restraint — that is the fence working rather than being worked around.

⚠️But "left for whoever next touches the ledger" is a hope, not an owner — and this shift has spent a lot of effort on exactly that shape (#9196's JSDoc, #9271's two decline lists, #9331's stale anchor: prose that carries a count or an enumeration and silently goes wrong). So I am filing it rather than letting it ride. Nothing owed from you.

Route

Fixes #9174 is right — this closes the card, and with it the exclusion I recorded on the objects tolerance family's closing measurement (#63675315344490), which named this card as "same class, different carrier". When this lands that carrier is closed and the narrow claim widens by one. I will restate it on #6367 with an artifact rather than by editing the old comment.

Holding for CI.


Generated by Claude Code

@os-zhuang
os-zhuang marked this pull request as ready for review August 17, 2026 17:12
@os-zhuang
os-zhuang added this pull request to the merge queueAug 17, 2026
Merged via the queue into main with commit 2416dd5Aug 17, 2026
26 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-9174-revertcommit-stored-type-preflight branch August 17, 2026 17:34
os-steve pushed a commit that referenced this pull request Aug 17, 2026
…cers and their atomicity
The ledger entry at error-code-ledger.zod.ts:381 named only the publish
pre-flight as STORED_TYPE_NOT_CANONICAL's producer ("refused at the publish
pre-flight, batch-atomic"). PR #9360 (#9174) added a second producer -
revertCommit's restore limb, which refuses per-item on its existing
failed[] channel and is explicitly NOT batch-atomic - leaving the comment
naming one of two producers and the wrong atomicity for the one it omitted.
Names both now: the publish pre-flight (batch-atomic, #8908) and
revertCommit's restore limb (per-item on failed[], NOT batch-atomic,
#9174), verified against the merged #9360 diff at protocol.ts:14432 and
protocol.ts:16399.
Text-only change - accept/reject behavior, the error code, and its
envelope are all unchanged.
Fixes#9361
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fs18A2DdXLVN2h8PaaFBcP
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@os-zhuang@claude