diff --git a/.changeset/tidy-pugs-shave.md b/.changeset/tidy-pugs-shave.md new file mode 100644 index 0000000000..06a36a21db --- /dev/null +++ b/.changeset/tidy-pugs-shave.md @@ -0,0 +1,11 @@ +--- +'@objectstack/metadata-protocol': patch +--- + +Remove the four dead `'objects'` spelling tolerances in the metadata protocol's object registry and storage seams. + +`applyObjectRegistryMutation`, `applyRegistryWriteThrough`, `ensureObjectStorage` and `dropObjectStorage` each admitted a plural `'objects'` type key, and the first of them *registered under it* — the spelling-tolerant-lookup shape `canonicalMetaType`'s header rejects, and the one that previously let a plural registry entry shadow an entire code-authored listing. + +All four are unreachable: every producer folds the type through `PLURAL_TO_SINGULAR` / `canonicalMetaType` before these seams see it. No behaviour changes for any caller that folds — which is all of them. What changes is the failure mode of a future caller that does *not* fold: it no longer silently registers an object under a plural key, so `assertObjectRegistered` fails closed with a loud, recoverable error instead. + +Folding at the producer remains the rule; these guards were never a second line of defence. diff --git a/packages/metadata-protocol/src/protocol.object-registry-write-through-spelling.test.ts b/packages/metadata-protocol/src/protocol.object-registry-write-through-spelling.test.ts index 646a2ca60a..dd7b7f0d0a 100644 --- a/packages/metadata-protocol/src/protocol.object-registry-write-through-spelling.test.ts +++ b/packages/metadata-protocol/src/protocol.object-registry-write-through-spelling.test.ts @@ -1,18 +1,25 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. // // [#8862] Every `applyRegistryWriteThrough` route registers an object under the -// CANONICAL SINGULAR key — the `'objects'` limb below it is dormant. +// CANONICAL SINGULAR key — and the `'objects'` limb below it is now REMOVED. +// +// This file was written in two halves, and reads as one only if you know that. +// The MEASUREMENT half proved the limb dormant (below). The REMOVAL half then +// deleted it and added section 4, which keeps it deleted. Everything the +// measurement half says about the limb is therefore in the PAST TENSE — the +// code it quotes no longer exists, and is quoted because it is what the +// verdict was about. // // --------------------------------------------------------------------------- // The question this file answers, and why it had to be measured // --------------------------------------------------------------------------- -// `applyObjectRegistryMutation` does not merely ADMIT a plural type key, it -// consumes one: +// `applyObjectRegistryMutation` did not merely ADMIT a plural type key, it +// consumed one — this is the code as it stood before the removal: // // if (request.type !== 'object' && request.type !== 'objects') return; // this.engine.registry.registerItem(request.type, request.item, 'name'); // -// The spelling that came in is the spelling the registry entry is minted +// The spelling that came in was the spelling the registry entry got minted // under. `canonicalMetaType`'s header names that exact shape as having already // cost the repo a real bug: one plural-spelled read minted a plural registry // entry, `listItems('actions')` stopped being empty, and the singular fallback @@ -20,11 +27,12 @@ // overlay row shadowed an entire code-authored listing and survived the DELETE // meant to lift it. // -// So the limb is either a live registry-shadowing defect or dead tolerance, -// and which one it is depends entirely on whether any caller can deliver +// So the limb was either a live registry-shadowing defect or dead tolerance, +// and which one it was depended entirely on whether any caller could deliver // `'objects'`. #8862 was filed WITHOUT that measurement, deliberately: it came // out of #8820, the card where an unmeasured reachability claim was the whole -// defect. This file is that measurement. +// defect. This file is that measurement — and, in section 4, the guard that +// the removal it licensed stays done. // // --------------------------------------------------------------------------- // The trace, measured on `origin/main` — all four routes fold at the producer @@ -54,36 +62,59 @@ // call graph. // // --------------------------------------------------------------------------- -// What this file is FOR, given the limb is dead +// What this file is FOR, now that the limb is gone // --------------------------------------------------------------------------- -// Dormancy proven once is dormancy unenforced. Nothing in the type system says -// a fifth caller must fold — the parameter is a bare `type: string`, exactly -// the shape that let #8820's hazard sit unnoticed. Two guards close that: +// Removal is not self-enforcing either. Nothing in the type system says a +// fifth caller must fold — the parameter is a bare `type: string`, exactly +// the shape that let #8820's hazard sit unnoticed. Three guards close that +// (the third added by the removal half): // // • the per-route cases pin the SPELLING that reaches the registry, so a // route that stops folding mints `'objects'` here and fails loudly rather // than silently shadowing a code-authored listing; // • `applyRegistryWriteThrough` call-site COUNT is pinned, so a fifth caller -// cannot be added without a human re-reading the trace above. +// cannot be added without a human re-reading the trace above; +// • [removal half] section 4 pins that no tolerance limb comes BACK — the +// guard the count pin cannot give, since a fifth caller and a re-widened +// predicate are different regressions. // // The fold maps are pinned too: they are an external dependency of this -// verdict, living in `@objectstack/spec`, and the dormancy dies the day either -// stops resolving `objects`. +// verdict, living in `@objectstack/spec`, and the removal's safety dies the +// day either stops resolving `objects`. +// +// ⚠️ One thing removal did NOT buy, recorded because the obvious reading +// overstates it: a hypothetical fifth, unfolded caller no longer registers an +// OBJECT (so `assertObjectRegistered` fails CLOSED — a loud error replacing a +// silent one), but on an unscoped kernel the value still falls through to +// `hydrateOverlayIntoRegistry`, which registers under the raw type like every +// other overlay kind. Removal took the plural OUT of the object-specific +// shadowing path; it did not add a second line of defence, and folding at the +// producer remains the only thing that actually prevents a plural key. // // --------------------------------------------------------------------------- -// Ablation directions, predicted BEFORE running (results in the PR body) +// Ablation directions, predicted BEFORE running (results in the PR bodies) // --------------------------------------------------------------------------- +// Measurement half (#9008), against the tree that still HAD the limb: // 1. Ship state -> GREEN // 2. `'objects'` limb deleted from BOTH // `applyRegistryWriteThrough` and // `applyObjectRegistryMutation` -> GREEN // (the limb is dead once the producers fold — this is the -// evidence that licenses its removal as dead tolerance) +// evidence that licensed its removal as dead tolerance) // 3. `applyObjectRegistryMutation`'s guard inverted to accept // ONLY `'objects'` -> RED // (non-vacuity control: proves the cases really observe // this seam registering, rather than passing for want of // an assertion) +// +// Removal half (#8862), against the tree with the limb GONE. ⚠️ Direction 2 +// above is why the per-route cases alone could NOT have caught a bad removal — +// they are green either way. Section 4 is what carries this half: +// 4. Ship state (limb removed) -> GREEN +// 5. Any one tolerance limb restored -> RED in §4 +// (the regression this half exists to prevent) +// 6. Comment stripper neutered to return '' -> RED in §4 +// (non-vacuity: the expected count is ONE, not zero) import { describe, expect, it } from 'vitest'; import { readFileSync } from 'node:fs'; @@ -458,3 +489,102 @@ describe('[#8862] the `applyRegistryWriteThrough` caller set stays closed', () = expect(callSites).toHaveLength(1); }); }); + +// ═══════════════════════════════════════════════════════════════════════════ +// 4. [#8862 removal half] The tolerance stays removed +// ═══════════════════════════════════════════════════════════════════════════ +// +// The four `'objects'` tolerance limbs were deleted once the dormancy above +// was proven. This block keeps them deleted. +// +// It scans EXECUTABLE text — comments stripped — for a reason that is not +// cosmetic: the removal commit deliberately kept the deleted limbs QUOTED in +// the surrounding comments (a reader needs to see what was removed and why), +// so a scan of the raw file would match its own documentation and could never +// go red. Stripping is what makes this pin capable of failing. +// +// ⭐ NON-VACUITY IS STRUCTURAL, not a separate control case. The expected count +// is ONE, not zero — the surviving occurrence is `listCollection('object', +// 'objects')`, a genuine singular/plural COLLECTION PAIR and a different +// construct entirely (it names two registry collections; it does not tolerate +// a spelling). So a stripper that silently blanked the file would report zero +// and fail, and a returning tolerance limb reports two and fails. Measured +// both directions on the removal commit: `origin/main` before it scored FIVE +// (the pair + the four limbs), the tree after it scores ONE. +// +// ⛔ Do not "fix" a failure here by relaxing the count. If a plural spelling +// genuinely has to reach one of these seams again, the answer is the same one +// #8820 and #8862 both landed on: fold at the PRODUCER. A tolerant predicate +// one layer below a folding boundary is the shape `canonicalMetaType`'s header +// rejects, and the shape that already cost this repo one registry-shadowing +// bug. + +/** + * Remove comments while preserving string literals, so a scan can tell code + * from prose ABOUT that code. Handles `//`, block comments, and the three + * quote forms with escapes. + */ +function executableTextOf(src: string): string { + let out = ''; + let i = 0; + const n = src.length; + while (i < n) { + const c = src[i]; + const d = src[i + 1]; + if (c === '/' && d === '/') { + while (i < n && src[i] !== '\n') i++; + continue; + } + if (c === '/' && d === '*') { + i += 2; + while (i < n && !(src[i] === '*' && src[i + 1] === '/')) i++; + i += 2; + continue; + } + if (c === "'" || c === '"' || c === '`') { + const quote = c; + out += c; + i++; + while (i < n) { + if (src[i] === '\\') { + out += src[i] + (src[i + 1] ?? ''); + i += 2; + continue; + } + out += src[i]; + if (src[i] === quote) { i++; break; } + i++; + } + continue; + } + out += c; + i++; + } + return out; +} + +describe('[#8862] the `objects` tolerance stays removed', () => { + const code = executableTextOf(readFileSync( + fileURLToPath(new URL('./protocol.ts', import.meta.url)), + 'utf8', + )); + + it('has exactly one executable `objects` literal — the collection pair, not a tolerance', () => { + const occurrences = code.match(/'objects'/g) ?? []; + // ONE, not zero: see the non-vacuity note above. Zero means the + // stripper broke; two or more means a tolerance limb came back. + expect(occurrences).toHaveLength(1); + const lines = code.split('\n').filter((l) => l.includes("'objects'")); + expect(lines).toHaveLength(1); + expect(lines[0]).toContain("listCollection('object', 'objects')"); + }); + + it('none of the four seams admits a plural any more', () => { + // The narrowed forms, pinned positively so a reformat that defeats the + // count above still has to keep these honest. + expect(code.match(/request\.type !== 'object'\) return;/g) ?? []).toHaveLength(1); + expect(code.match(/request\.type === 'object'\) \{/g) ?? []).toHaveLength(1); + // `ensureObjectStorage` + `dropObjectStorage`. + expect(code.match(/\n\s*if \(type !== 'object'\) return;/g) ?? []).toHaveLength(2); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 75041f633f..fc4690c77d 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -11158,7 +11158,23 @@ export class ObjectStackProtocolImplementation implements * re-hydration already writes for the same rows. */ private applyObjectRegistryMutation(request: { type: string; name: string; item?: any; packageId?: string | null }): void { - if (request.type !== 'object' && request.type !== 'objects') return; + // [#8862] The `&& request.type !== 'objects'` limb that stood here is + // GONE. It did not merely admit a plural, it registered under one — + // `registerItem` below mints the entry under the spelling that arrived + // — which is the shape {@link canonicalMetaType}'s header blames for a + // real prior bug (one plural read minted a plural entry and shadowed an + // entire code-authored listing). It was measured DORMANT in #8862: the + // sole caller {@link applyRegistryWriteThrough} has four, and every one + // folds at its producer. Removing it makes a hypothetical fifth, + // unfolded caller fail CLOSED — it does not register, and + // `assertObjectRegistered` raises loudly — instead of silently minting + // `registerItem('objects', …)`. What keeps a fifth caller from being + // added unexamined is the call-site count pin in + // `protocol.object-registry-write-through-spelling.test.ts`, not this + // guard; the two are a pair. + if (request.type !== 'object') return; + // `request.type` is provably `'object'` on the line above, so this + // mints the canonical key rather than whatever spelling arrived. this.engine.registry.registerItem(request.type, request.item, 'name'); try { const layer = this.classifyObjectContribution(request.name, request.packageId); @@ -11473,7 +11489,28 @@ export class ObjectStackProtocolImplementation implements /** The row's org scope — `null` for an env-wide row. [#6602] */ organizationId: string | null; }): void { - if (request.type === 'object' || request.type === 'objects') { + // [#8862] The `|| request.type === 'objects'` limb is GONE — this + // branch admitted the plural that {@link applyObjectRegistryMutation} + // then consumed. All four call sites of this method fold at their + // producer (traced in + // `protocol.object-registry-write-through-spelling.test.ts`), so the + // limb was dead. + // + // ⚠️ What removal does and does NOT buy, stated precisely because the + // obvious reading overstates it. A hypothetical fifth, UNFOLDED caller + // passing `'objects'` no longer takes this branch — so it no longer + // reaches `registerObject`, and the object is simply not registered: + // `assertObjectRegistered` fails CLOSED, a loud recoverable error in + // place of a silent one. That is the win. It does NOT mean nothing is + // registered at all: on an unscoped kernel the value falls through to + // {@link hydrateOverlayIntoRegistry}, which registers under the RAW + // type like every other overlay kind. So the plural is no longer an + // object-specific hazard minting a shadow OBJECT — it is merely the + // same general "producers must fold" contract every other metadata + // type already lives under. Folding at the producer stays the rule; + // this guard is not a second line of defence and must not be written + // as one. + if (request.type === 'object') { // NOT org-gated, deliberately: an `object` is `allowOrgOverride: // false` (ADR-0005) and its physical TABLE is env-wide, so the // registry entry backing it is env-wide too — `assertObjectRegistered` @@ -11775,7 +11812,16 @@ export class ObjectStackProtocolImplementation implements * simply no-op, and a sync failure must not abort the publish. */ private async ensureObjectStorage(type: string, name: string): Promise { - if (type !== 'object' && type !== 'objects') return; + // [#8862] The `&& type !== 'objects'` limb is GONE. Both call sites + // hand over a folded value — `saveMetaItem` after + // `canonicalizeMetaRequestType`, and `runPublishSideEffects` as + // `args.singularType`, which `promoteDraftForPublish` folds through + // `PLURAL_TO_SINGULAR` for BOTH its callers (including + // `publishPackageDrafts`, the one #8820 named as the structural + // hazard because `listDrafts` returns the draft row's stored type + // verbatim). Guard-only either way: `type` never reaches + // `syncObjectSchema`, which takes `name` alone. + if (type !== 'object') return; try { await this.engine.syncObjectSchema(name); } catch (err: any) { @@ -11830,7 +11876,14 @@ export class ObjectStackProtocolImplementation implements * reclaimed by the next sync/drop rather than blocking the delete. */ private async dropObjectStorage(type: string, name: string): Promise { - if (type !== 'object' && type !== 'objects') return; + // [#8862] The `&& type !== 'objects'` limb is GONE. Both call sites + // are in `deleteMetaItem` and fold at the producer: one passes + // `singularTypeForRepo` (`PLURAL_TO_SINGULAR[request.type] ?? + // request.type`), the other re-folds the same expression inline. + // Guard-only: `type` never reaches `dropObjectSchema`, which takes + // `name` alone. {@link shouldDropStorage} already folded before + // deciding, so this guard was answering a question settled upstream. + if (type !== 'object') return; try { await this.engine.dropObjectSchema(name); } catch (err: any) { @@ -13397,11 +13450,17 @@ export class ObjectStackProtocolImplementation implements // `type = 'view'` missed a publish addressed `/meta/views/…`. // // ⚠️ `ensureObjectStorage` is NOT in that list, because it answered both - // spellings identically before this fold and after it: it opens + // spellings identically before this fold and after it: it used to open // `if (type !== 'object' && type !== 'objects')`, a spelling-tolerant // lookup one layer down — the shape {@link canonicalMetaType}'s header // rejects. // + // [#8862] That tolerant limb is now GONE — the guard reads + // `if (type !== 'object')`. The paragraph is kept in the past tense + // rather than deleted because the reasoning below is what LICENSED the + // removal, and a reader arriving at the fold still needs to know why + // the helper was never a second line of defence. + // // [#8820] This paragraph used to justify that with "both of its call // sites stand behind a fold". That reason was FALSE. Counting the // helper's own two call sites is not the reachability question: the