Skip to content

finding: revertCommit has no STORED_TYPE_NOT_CANONICAL pre-flight, so a non-canonical stored item degrades to a console.warn where the sibling doors give a wire-visible code #9174

Closed

Description

@os-zhuang

Surfaced while implementing #9111 (PR #9173) — the reachability sweep over every producer of hydrateOverlayIntoRegistry. Filed unassigned, as a recording. Observation-class: nothing here is silent after #9173, and reachability is the same pre-#7894 at-rest residue that family already tracks.

What was measured

#8908 introduced isNonCanonicalStoredType (packages/metadata-protocol/src/protocol.ts:3482) — the derived six-member class of stored spellings whose type the manifest-collection map omits (fields, seeds, external_catalogs, externalCatalogs, translations, email_templates). It guards two doors that consume an at-rest type:

revertCommit is the third consumer of an at-rest type and has no such pre-flight. Its per-item loop folds with the manifest map at :16062 and :16243 and then hands the item straight to applyRegistryWriteThrough.

Why it is only an observation

Before PR #9173 that combination minted a registry entry under the raw stored spelling. After it, the mint door asserts and the throw lands in applyRegistryWriteThrough's best-effort catch, which is correct there — the row is already persisted and #4521's contract is that a registry hiccup must never fail a committed write. So the outcome is now loud, just less informative than its two siblings: an operator gets a console.warn on the server rather than a named failed[].code in the response, for exactly the class the other two doors name explicitly.

The question for triage

Whether the revert door should carry the same pre-flight the publish door carries — refusing such an item up front with STORED_TYPE_NOT_CANONICAL — or whether a revert is deliberately more permissive than a publish because it restores a row that already existed rather than minting a new active namespace. That is a contract question about revertCommit's receipt, not a defect report, which is why this is filed as a finding rather than graded.

⛔ Note for whoever takes it: revertCommit's items feed a commit record (the append-only revert commit) and the reverted[] / failed[] receipts, and #9161 deliberately kept the caller's spelling on audit and ledger keys so #8908's AUDIT_TYPE_NOT_CANONICAL assert fires loudly. Any fold here would touch a ledger key. PR #9173 changed no producer spelling for exactly that reason.

Backlinks: #9111 / PR #9173 (where this was measured) · #8908 (the predicate and the two guarded doors) · #8957 (the migrate door's skipped verdict) · #7894 (the URL door that closed the namespace).

Activity

  1. added
    findingRecorded observation — held, not dispatchable until the findings triage round grades it
    on Aug 16, 2026
  2. os-zhuang commented on Aug 17, 2026

    @os-zhuang
    ContributorAuthor

    First-touch grading (triage seat, 2026-08-17): finding → pm:queue, type Task.

    Adjudicated by the sibling-branch meta-rule: #8908 ruled the class (a stored spelling outside the canonical map is refused loudly, by name, at a door consuming an at-rest type) and #8957 extended it to the migrate door with a per-row verdict. The revert door is the third branch of the same family, and the card's own candidate reason for divergence ("a revert restores a row that already existed") buys nothing after PR #9173: the registry mint refuses the item anyway, so today's behaviour is the same refusal minus the wire-visible receipt — strictly less informative than both siblings. The mother ruling's rationale transfers intact ⇒ align: add the STORED_TYPE_NOT_CANONICAL pre-flight to revertCommit.

    PM mechanism assumptions (verify, don't inherit): suggest mirroring the migrate door's per-row posture (failed[] entry per item) rather than publish's batch-atomic refusal, since revertCommit is already per-item — but verify against revertCommit's existing receipt semantics before committing to it. ⛔ Respect the card's ledger-key caution: the pre-flight refuses up front and must NOT fold any spelling on audit/ledger keys (#9161's AUDIT_TYPE_NOT_CANONICAL assert relies on that). Size M / judgement tier (suggest opus). File surface: packages/metadata-protocol/src/protocol.ts + tests; packages/spec untouched ⇒ no clause-② tier.


    Generated by Claude Code

  3. added
    pm:queueReady for the PM dispatch loop
    and removed
    findingRecorded observation — held, not dispatchable until the findings triage round grades it
    on Aug 17, 2026
  4. added theissue type on Aug 17, 2026
  5. added
    pm:dispatchedDispatched to a dev agent by /pm-dispatch
    and removed
    pm:queueReady for the PM dispatch loop
    on Aug 17, 2026
  6. self-assigned this
    on Aug 17, 2026
  7. os-zhuang commented on Aug 17, 2026

    @os-zhuang
    ContributorAuthor

    PM claim · seat pm-dispatch · session 17d48cbe-d3e0-562b-95a3-e2653a9bd9fb · branch claude/issue-9174-revertcommit-stored-type-preflight

    裁决 — not re-litigable

    1. This is the carrier I explicitly excluded when I closed the objects tolerance family. The closing measurement on seat post [PM seat] domain:server · seat 1 — 🟢 os-elon-musk · 由 os-elon-musk 账户的项目负责 #6367 (5315344490) states the family is at zero under the narrow definition and names this card as "same class, different carrier", deliberately out of that claim. So this is not a re-opening and not a regression — it is the piece that was named and left. Close it and the exclusion goes away.

    2. The sibling doors are the specification. saveMetaItem refuses with a wire-visible code; hydrateOverlayIntoRegistry asserts REGISTRY_TYPE_NOT_CANONICAL (hydrateOverlayIntoRegistry mints registry entries under the RAW type with no fold and no assertion — the last unfolded seam of the objects tolerance family #9111, landed as 29d055b68); migrateStoredMetadata declines with outcome: 'skipped' and says why. revertCommit degrading to a console.warn is the odd one out. ⛔ Do not invent a fourth shape — match the nearest sibling and say which one you matched and why.

    3. ⚠️ Choosing between "refuse" and "decline-and-report" is the actual work, and the two are not interchangeable. revertCommit is a write door, which argues for refusal like saveMetaItem; but a revert operating over stored rows it did not author may be closer to migrateStoredMetadata's reported-skip. Measure what the caller can do with each answer before choosing, and state the reasoning in the PR. ⛔ A console.warn is not a candidate — that is the defect.

    PM 机制假设 — measure, don't inherit

    Route

    Worktree-first. Changeset per repo rules. Draft PR. ⛔ Never merge your own PR. ⛔ Never git stash. ⛔ Never raise a TEST_DEBT entry.

    ⚠️ The queue now runs the FULL suite as a required check (maintainer ruled #9283 Option A at ~11:59Z), so expect landing to be slower than earlier today. That is the intended cost, not a stall.


    Generated by Claude Code

  8. os-zhuang commented on Aug 17, 2026

    @os-zhuang
    ContributorAuthor
    {
      "issue": 9174,
      "status": "done",
      "branch": "claude/issue-9174-revertcommit-stored-type-preflight",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/9360",
      "premise_still_valid": true,
      "summary": "Premise verified at HEAD (4906c900f): protocol.ts still carries #9111's own producer trace naming route 5, `revertCommit` -> write-through, as **Unguarded**, and the console.warn is still the only signal. Measured the door per limb over the real SysMetadataRepository on an unscoped kernel before choosing a shape. RESTORE limb: {success:true, revertedCount:1, failed:[]} with reverted[0].action 'restored', registerItem called ZERO times, and one line of server stderr ('[Protocol] registry write-through failed for fields/showcase_task.title: [registry_type_not_canonical] ...') as the only trace — the receipt promises a body the platform cannot serve for this class. SOFT-REMOVE limb: {success:true, action:'removed'}, row gone from sys_metadata, no warning, no registry key touched — nothing wrong with it. Matched sibling: saveMetaItem's REFUSAL, carried on revertCommit's existing per-item failed[] channel with code STORED_TYPE_NOT_CANONICAL, and success goes false. The separating test between refuse and decline is whether the door can deliver what it promises for the row: migrateStoredMetadata declines because rewriting a stored spelling is an identity move and out of its reach entirely (so 'skipped' must not poison storedMigrationClean, which runs forever); here the write is squarely in reach and still delivers none of what 'restored' promises, which is saveMetaItem's case, and a one-shot operator action has no forever to poison. The soft-remove limb is deliberately outside the gate — it performs its promise completely and the removal is the one action that shrinks this residue; refusing it would answer success:false for a revert that fully succeeded and would hand back an instruction ('drop the fields row') naming the operation it had just declined. No fold, no audit row, no commit-record entry for a refused item, so #9161's ledger-key ruling is untouched in both directions. No new error code and packages/spec untouched — STORED_TYPE_NOT_CANONICAL is already listed under @objectstack/metadata-protocol at error-code-ledger.zod.ts:381 — so no clause ②.",
      "tests": "All at c40b70024, the final commit. NEW: packages/metadata-protocol/src/protocol.revert-stored-type-canonical.test.ts, 10 tests, 4 sections. Full package suite `pnpm --filter @objectstack/metadata-protocol test` -> 'Test Files 120 passed (120) / Tests 1651 passed (1651)'. Build closure `pnpm --filter '@objectstack/metadata-protocol^...' build` green; package DTS build green. Full workspace closure `pnpm exec turbo run build --filter='./packages/*' --filter='./packages/*/*'` -> 70/70. Ratchet `pnpm check:type-check-debt --re-measure` -> 'OK — 33 ledger entr(ies) re-measured in 220.9s, 1926 raw tsc error(s) total, none above its recorded number'. Gate union re-derived with `node scripts/pm/dispatch-gates.mjs` over the actual changed paths 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. ABLATIONS, direction predicted before running (no dogfood/dist ablation here — this is a unit harness importing ./protocol.js from source, so no rebuild leg applies): (1) ship state GREEN 10/10. (2) pre-flight deleted, predicted RED §1 -> RED 6 failed, BROADER than predicted: §4 goes red with §1 because a gate that refuses nothing lets the item back into reverted[] and therefore into the append-only revert commit; §2/§3 stayed GREEN, which is the half that matters. Recorded as observed rather than trimmed to the prediction. This leg IS the pre-fix measurement. (3) restore-limb guard dropped so both limbs are gated, predicted RED §2 -> RED, exactly 1 ('removes a commit-created non-canonical row and reports success'). (4) predicate widened to the complete canonicalMetaType(t) !== t, predicted RED §3 -> RED, exactly 1 ('a manifest-PRESENT plural is NOT refused'). CI on PR #9360: in_progress at report time (queue now runs the full suite per #9283 Option A). TWO DEFECTS IN MY OWN WORK, both found by running the pipeline rather than reading it, each invisible to one half of it: (a) TS2345 at repo.restoreVersion — hoisting the limb condition into a boolean broke TS narrowing on it.prevVersion (a mutable property; narrowing does not survive an aliased boolean). All 1651 tests stayed green because vitest transpiles without type-checking; only the DTS build saw it. Fixed by binding `restoreToVersion: number | null`, still one source of truth for both the pre-flight and the branch. (b) The shrink-only DEBT ratchet for this package moved 63 -> 69 on six TS6133 unused `warn` bindings left behind when the console.warn spy restore moved into afterEach. Back to 63, re-measured clean. No TEST_DEBT entry raised, no ledger number touched.",
      "open_questions": [
        {
          "question": "The error-code ledger's comment on STORED_TYPE_NOT_CANONICAL (packages/spec/src/api/error-code-ledger.zod.ts:381) reads 'refused at the publish pre-flight, batch-atomic' and now names one of two producers. Amend it, or leave it?",
          "options": [
            "A: leave it — the fix would put packages/spec in this diff, which pulls in a gate family (authorable-surface, gen:schema, gen:openapi) this change otherwise does not touch; named in the PR body as a reviewer note instead",
            "B: amend the one comment line in this PR and run the spec gate family",
            "C: fold it into whoever next touches the ledger for a real reason"
          ],
          "recommendation": "A, taken as the shipped choice, with C as the follow-up. The bounded in-place exemption's fourth condition (same gate families, no new verification surface) fails outright for packages/spec, and the dispatch brief scoped the file surface to protocol.ts plus tests. The stale half is a comment, not a contract: the code's membership under @objectstack/metadata-protocol is what the gate reads and that is already correct."
        }
      ],
      "out_of_scope_findings": []
    }

    Generated by Claude Code

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

Metadata

Metadata

Assignees

Labels

pm:dispatchedDispatched to a dev agent by /pm-dispatch

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions