Skip to content

fix(metadata-protocol): report a non-canonical stored type as skipped instead of canonical (#8957) - #9059

Merged
os-zhuang merged 3 commits into
mainfrom
claude/issue-8957-migrate-stored-noncanonical-report
Aug 16, 2026
Merged

fix(metadata-protocol): report a non-canonical stored type as skipped instead of canonical (#8957)#9059
os-zhuang merged 3 commits into
mainfrom
claude/issue-8957-migrate-stored-noncanonical-report

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#8957

Shape 1 only, as queued: give the scan the canonical fold it lacks, and report a row whose stored spelling is non-canonical as skipped with a reason, so it lands in report.rows instead of vanishing into the unreported canonical count. The method's contract — body canonicalization — is unchanged, and it still writes nothing for this class.

The defect

migrateStoredMetadata opened every row with PLURAL_TO_SINGULAR[rawType] ?? rawType — the manifest-collection map, which legitimately omits the metadata types that are not stack collections. For a row stored under one of their plural spellings the fold was a no-op: the pass looked up ADR-0087 body conversions registered for a type named fields, found none, saw changed === false, and recorded the row canonical. canonical is counted and never itemised — by design, because on a healthy deployment that is every row — so the row disappeared from report.rows entirely.

canonical means "nothing to do", and there was something to do: the row sits in a second namespace that no registry read and no compliance query on the canonical type can reach.

Since #8908 landed (PR #8986, 1a7f9073e), publishPackageDraftsrefuses exactly these rows at its pre-flight with STORED_TYPE_NOT_CANONICAL. The stored migration is the door an operator naturally reaches for after that refusal, and it answered that the row was already fine. That contradiction was live on main; this closes it.

Premise verification, and the two fold sites

The card's body describes one fold site; there are two occurrences of [rawType] ?? rawType in protocol.ts, and only one of them is this card:

site (at origin/main192213f66)enclosing methoddisposition
:12990migrateStoredMetadata (opens :12931)this card — fixed
:15171duplicatePackage (opens :14994)not this card — deliberately untouched, see below

:13971 is prose describing the first fold, not a third site. It is rewritten here because this change makes it false.

duplicatePackage is a different method with a different report, and is not the same defect. Its singular feeds exactly one decision — singular === 'flow', which canonicalizer branch to take — and the non-flow branch then converts with rawType, not singular. The copy is written with saveMetaItem({ type: row.type }), and saveMetaItem opens with canonicalizeMetaRequestType, which folds through canonicalMetaType. So the stored spelling cannot propagate into a new row through it, and the outcome is never a silent "already correct" — the copy either lands canonical or arrives in failed[] with saveMetaItem's own authoring refusal. That is a different, loud, non-silent shape, outside this card's declared surface. Recorded here rather than fixed, so the class is not left half-read.

The change

1. The scan folds canonically.canonicalMetaType(rawType) replaces the manifest lookup. The swap cannot change the answer for any spelling the old fold resolved: META_URL_TO_SINGULAR embeds every manifest spelling verbatim (limb 1 of buildMetaUrlMap) under a module-load agreement assertion that throws if the two ever disagree.

Re-measured on this tree rather than re-typed from #8986:

class (isNonCanonicalStoredType): ["email_templates","externalCatalogs","external_catalogs","fields","seeds","translations"]
canonical types whose own spelling folds elsewhere (must be empty): []
spellings where the two folds disagree (must be empty): []
spellings the fold swap NEWLY folds: ["email_templates","externalCatalogs","external_catalogs","fields","seeds","translations"]
identical to class? true

Six, not the four the card names — externalCatalogs and email_templates are the two a hand-written list misses. The last two lines are this change's own safety proof, and are stronger than a list: the only spellings the new fold treats differently are exactly the class the guard below reports.

2. The --type filter folds the same way, and had to move with it. An operator arrives holding the spelling the publish refusal just quoted (--type fields); another holds --type field. Both are asking about this row. Folding only the row would have moved the residue out of reach of the very spelling the operator was handed.

3. The row is reported skipped, before its body is read. The verdict is about where the row lives, which is decided without its body.

// before — the row was invisible
{ "scanned": 1, "canonical": 1, "skipped": 0, "rows": [] }
// after
{ "scanned": 1, "canonical": 0, "skipped": 1,
"rows": [{ "type": "field", "name": "showcase_task.title", "outcome": "skipped",
"reason": "the row is stored under the non-canonical metadata type 'fields' ('fields/showcase_task.title'), and its canonical type is 'field'. …" }] }

The reason names the stored spelling in the same type/name form the publish refusal quotes — so the two doors' output can be text-matched — plus the canonical type, why this pass will not fix it, the other door's error code, and the re-author path.

⛔ The continue is load-bearing, not tidiness. With the canonical fold in place, a row falling through would hand convertStoredItemDetailed the CANONICAL type for a row stored under a different one, and an apply run would re-save it through saveMetaItem({ type: singular }) — performing the identity move (a new (org, type, name, package_id) key, history and audit continuity, a collision question when the canonical row already exists) that #8908's ruling parked as a follow-up needing its own appetite. Shape 2 is not attempted here. The "an apply run writes NOTHING for it" case is the pin that keeps this pass out of it.

storedMigrationClean is deliberately unchanged

skipped rows still do not flip it, and this is a decision, not an oversight. This pass has no lever for the condition, so failing the verdict over it would give os migrate meta --stored a non-zero exit that no run of that command could ever clear — a gate failing on something its own tool cannot fix. The row is reported loudly per-row instead, and the publish door is what refuses it. The reasoning is on the function's doc, and pinned.

Two statements this change falsifies, and repairs

⚠️Surface note — both are inside publishPackageDrafts, which the dispatch declared off-limits. They are repaired because this PR is what makes them false; neither is a behavioural change to that method's gate, and #8986's pin on the message (toContain('_migrate-stored')) still holds. Flagged for the PM rather than done silently.

  • The prose comment (formerly :13971) stated that the stored migration "converts BODIES under the plural key, finds nothing to change, and reports the row canonical". Rewritten to record that as history and state what it does now.
  • The operator-facing refusal message ended …and reports rows of this class as already canonical. That clause is now false, and it is the sentence pointing the operator at the other door — the exact lie this card is about, relocated. It now reads …and reports rows of this class as 'skipped' with that same reason (#8957).

Also in stored-migration.ts (the report shape): the skipped-section header asserted that skipped rows "keep reading through the chain", which held for the two carve-outs that existed then and is false for this class — a row in a second namespace is not read on its canonical type at all. The header now states only what is true of every skip class; each row's reason carries the specific truth.

Verification — union run at 521d475af, the final commit (post-merge)

packages/metadata-protocol/src/protocol.migrate-stored-noncanonical-type.test.ts, 10 cases. Every expectation is a literal: the source derives the class from the two maps precisely so it cannot drift, and a test re-deriving it from the same maps would agree by construction and prove nothing. Each refusal is paired with a control — a genuinely canonical row still canonical and still not itemised, and a manifest-PRESENT plural (objects) still converting exactly as before.

Reverse verification. Direction predicted before running: RED, with the boundary control surviving. Committed first, then protocol.ts restored from origin/main, the pin re-run, and the file restored and proved byte-identical (git hash-object = git rev-parse HEAD:… = 9531705c8), not retyped.

Measured: 9 failed / 1 passed. The survivor is the boundary control, as predicted — objects folds identically under both maps, so this change did not touch it. The defect came back verbatim in the positive-control case: report.canonical was 2 where the fix makes it 1, the residue row counted as already correct alongside the healthy one, with report.rows empty.

Suites, at 521d475af: metadata-protocol 109 files / 1542 tests. Then the DOWNSTREAM/consumer direction — the packages that actually consume migrateStoredMetadata / storedMigrationClean / formatStoredMigrationReport / _migrate-stored: cli 122 / 1358, rest 120 / 2001, client 23 / 301. All green.

Gates, all green at 521d475af: nul-bytes, cross-package-test-inputs (both the pnpm and the ci.yml script form), durability-log-level, filter-alias-parity, meta-type-normalized, stack-collection-maps, engine-double-contract, where-matcher, query-options-erasure, type-check-coverage, changeset-gate-self-tests, objectui-changeset, error-code-casing, check-adr-0087-registration, check-changeset-no-major, check-empty-changeset, check-error-status-conformance, and check:type-check-debt --re-measure (33 ledger entries re-measured in 236.9s on a fully built closure, 1926 raw errors, none above its recorded number).

Beyond the dispatch's named set: meta-type-normalized and stack-collection-maps judge exactly the maps this change reads; engine-double-contract and where-matcher are convention-triggered by the new test file and were re-derived from the actual diff, not recalled.

⚠️The ratchet caught a real one. Its first run measured the new pin file adding 11 raw tsc errors to metadata-protocol's TEST_DEBT entry (63 → 74), all one class: Partial< Row > & { metadata: unknown } collapses to metadata: string, because the row's stored metadata IS the string, so every fixture body written as an object literal was refused. Fixed at the type — a SeedRow alias that Omits metadata before re-adding it as unknown, which is what the seeding harness actually accepts — not with casts and ⛔ not by raising the ledger. The entry stays at 63.

origin/main was merged at 521d475af before this PR opened. protocol.ts took no commits in that range; #8862's PR (#9008) added a test-only file to this package, which is why the suite number above is 109/1542 rather than the pre-merge 108/1534. The sibling concurrent editor on protocol.ts, #8896, has not landed — its declared region (searchAll, findReferencesToMeta, publishPackageDrafts's commit-item catch, seed-loader.ts) is disjoint from both regions touched here.

Generated by Claude Code


Generated by Claude Code

… instead of canonical (#8957)
migrateStoredMetadata folded each row's `type` through PLURAL_TO_SINGULAR,
the manifest-collection map, which legitimately omits the types that are not
stack collections. For a row stored under one of their plural spellings the
fold was a no-op: the pass looked up ADR-0087 body conversions registered for
a type named `fields`, found none, and recorded the row `canonical` — a
verdict that means "nothing to do" about a row it had never looked at, and
one that is never itemised, so the row vanished from `report.rows`.
Since #8908 landed, publishPackageDrafts refuses exactly these rows at its
pre-flight, so the door an operator reaches for next was the one that told
them the row was fine.
The scan now folds with canonicalMetaType (the URL/registry map, which embeds
every manifest spelling verbatim under a module-load agreement assertion) and
reports a row whose stored spelling is non-canonical as `skipped` with the
reason, before its body is read. The method's contract is unchanged: it still
canonicalizes bodies, and it still writes nothing for this class — rewriting
a stored type is an identity move, explicitly unruled.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj
…s zero tsc debt (#8957)
`check:type-check-debt --re-measure` measured the new pin file adding 11 raw
errors to metadata-protocol's TEST_DEBT entry (63 -> 74), all one class:
`Partial<Row> & { metadata: unknown }` collapses to `metadata: string`,
because the row's stored `metadata` IS the string, so every body written as
an object literal was refused.
Fixed at the type, not with casts and not by raising the ledger: `SeedRow`
omits `metadata` from the `Partial<Row>` half before re-adding it as
`unknown`, which is what the seeding harness actually accepts. The package's
entry stays at 63.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/metadata-protocol.

3 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/concepts/metadata-lifecycle.mdx(via @objectstack/metadata-protocol)
  • content/docs/kernel/services-checklist.mdx(via @objectstack/metadata-protocol)
  • content/docs/protocol/kernel/http-protocol.mdx(via @objectstack/metadata-protocol)

1 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/v9.mdx(via @objectstack/metadata-protocol)

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.

Advisory only. 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.

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