From 82ef3b3578038330399a6dce4e8360f40dcb99c6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 02:41:02 +0000 Subject: [PATCH 1/2] fix(spec): refuse a declared def rename that would collapse two baseline keys onto one `checkRenameTable` validates `RENAMED_DEFS` against the defs a build EMITS, which is all its call site can see. A single rename `A -> B` where the recorded BASELINE already holds a property name under both `A` and `B` is invisible there: it is not two sources onto one target, the source is unemitted and the target is emitted, so all four existing rules pass. Every carry in `build-schemas.ts` is a plain `Map.set` keyed by the carried key, so the two entries collapse and the later write wins. The loss is the recorded RETIRED state and the recorded DEFAULT of the losing side, and it happens inside the carry, before any comparison runs -- so each ratchet downstream adjudicates against input this script already corrupted, in both directions: a genuine default change on the merged key can read as no change at all, and a key whose default never moved can read as `changed`. `checkRenameBaselineCollisions` refuses exactly the intersection: the property names the baseline records under BOTH defs. A merge whose two defs share no property name writes every key exactly once and is deliberately left green -- refusing a populated target as such would redden 24 of the 39 committed entries and forbid the in-tree `cloud/Sha256Digest -> system/Sha256Digest` shape. It runs once per baseline this script carries, because the in-tree snapshot and the upstream anchor are different documents and a collision can exist in either. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01MkQhmuuJAVDjmeWNixwDDH --- packages/spec/scripts/build-schemas.ts | 41 +++++++- packages/spec/scripts/lib/renamed-defs.ts | 81 ++++++++++++++++ packages/spec/scripts/renamed-defs.test.ts | 107 +++++++++++++++++++++ 3 files changed, 228 insertions(+), 1 deletion(-) diff --git a/packages/spec/scripts/build-schemas.ts b/packages/spec/scripts/build-schemas.ts index 3cb8119931..90466d3694 100644 --- a/packages/spec/scripts/build-schemas.ts +++ b/packages/spec/scripts/build-schemas.ts @@ -17,7 +17,12 @@ import { formatDefKeyCollisions, type EmittedDef, } from './lib/def-key-collisions'; -import { RENAMED_DEFS, carryAuthorableKey, checkRenameTable } from './lib/renamed-defs'; +import { + RENAMED_DEFS, + carryAuthorableKey, + checkRenameBaselineCollisions, + checkRenameTable, +} from './lib/renamed-defs'; // The Zod-graph walkers the authorable-surface reachability BFS runs on. Extracted // at #5317 so the pipe-direction rule (#4488) is assertable without running the // whole generator — see scripts/zod-graph.test.ts. @@ -871,11 +876,41 @@ try { process.exit(1); } +/** + * Refuse a declared rename that would COLLAPSE two of a baseline's own keys + * (#17383). Every carry below is a plain `Map.set` keyed by the carried key, so + * a rename whose source and target both hold the same property name in this + * baseline loses one of the two recorded facts — its retired state and its + * default — before any comparison runs. `checkRenameTable` cannot see this: it + * validates the table against the defs this build EMITS, and the damage lives + * in the baseline. Called once per baseline this script carries, because the + * in-tree snapshot and the upstream anchor are different documents and a + * collision can exist in either alone. + */ +function assertNoRenameBaselineCollisions(label: string, baselineKeys: Iterable): void { + const problems = checkRenameBaselineCollisions(baselineKeys); + if (problems.length === 0) return; + console.error( + `\n❌ ${problems.length} declared def rename(s) would COLLAPSE keys of the ${label}:`, + ); + for (const p of problems) console.error(` - ${p}`); + console.error( + `\n A rename may MOVE keys; it may never merge two of them onto one name. The carry\n` + + ` runs before every ratchet below, so a collapsed key makes the diff they report a\n` + + ` diff against input this script already corrupted — in both directions: a real\n` + + ` default change on the merged key can read as no change, and a key whose default\n` + + ` never moved can read as changed. See scripts/lib/renamed-defs.ts (#4684, #17383).`, + ); + process.exit(1); +} + if (surfaceDoc) { const snapshot = new Map( surfaceDoc.keys.map((e) => [e.replace(RETIRED_MARK, ''), e.endsWith(RETIRED_MARK)]), ); + assertNoRenameBaselineCollisions(`committed ${SURFACE_FILE_NAME}`, snapshot.keys()); + // Carry the snapshot through any declared def rename FIRST, so every check // below compares like with like. A rename moves keys between defs; it must // never be able to drop one, and it must never launder a retirement past @@ -2137,6 +2172,10 @@ let gitResolvedAnchor: { rev: string; keys: string[] } | null = null; if (base) { // Carry base keys through declared def renames first — same discipline as // the snapshot carry above — so a rename is never misread as a deletion. + assertNoRenameBaselineCollisions( + `upstream baseline ${base.rev.slice(0, 12)}`, + (base.doc.keys ?? []).map((entry) => entry.replace(RETIRED_MARK, '')), + ); const baseSnapshot = new Map(); for (const entry of base.doc.keys ?? []) { const key = entry.replace(RETIRED_MARK, ''); diff --git a/packages/spec/scripts/lib/renamed-defs.ts b/packages/spec/scripts/lib/renamed-defs.ts index 11231db63c..f32305e766 100644 --- a/packages/spec/scripts/lib/renamed-defs.ts +++ b/packages/spec/scripts/lib/renamed-defs.ts @@ -245,6 +245,87 @@ export function carryAuthorableKey( * The last two rules are about entries *interacting*, and only bind once the * table holds more than one entry — which #4703 is the first change to do. */ +/** + * Property names a recorded baseline holds under BOTH a rename's source def and + * its target def — the keys the carry would silently collapse (#17383). + * + * ## Why this is a separate rule from the four in {@link checkRenameTable} + * + * That function validates the table against the defs a build EMITS, which is + * all it can see at its call site. The damage here is not visible there at all: + * it lives in the BASELINE, and it is reached by a single, perfectly well-formed + * rename. `A → B` where the baseline already records `B:mode` is not two sources + * onto one target, so the merge rule never sees it; the source is gone and the + * target is emitted, so the decay rules pass; and every carry in + * `build-schemas.ts` is a plain `Map.set` keyed by the CARRIED key, so + * `A:mode` and `B:mode` land on the same entry and the later write wins. + * + * The damage is the same one the two-sources rule already refuses, reached by + * one rename instead of two: the surviving entry keeps only one of the two + * recorded RETIRED states and only one of the two recorded DEFAULTS, and the + * loss happens INSIDE the carry, before any comparison runs. So every gate + * downstream — check (b)'s live → retired transition, the deletion gate, the + * authorable-defaults differ — adjudicates against already-clobbered input, in + * both directions: a genuine default change on the merged key can read as no + * change at all, and a key whose default never moved can read as `changed`. + * + * ## What it deliberately does NOT refuse + * + * A rename onto a def that already exists is a legitimate, in-tree shape — + * `cloud/Sha256Digest → system/Sha256Digest` is one, and 24 of the committed + * entries have a target that already holds baseline keys once the surface + * snapshot has been regenerated under the new name. None of that loses + * anything: `Map.set` can only collapse two entries that are the SAME key, so a + * merge whose source and target share no property NAME writes every key exactly + * once. Refusing a populated target as such would redden the committed table; + * the collision — the intersection — is the whole of the damage and the whole + * of what is refused. + * + * Returns one problem line per colliding entry; empty means this baseline + * survives the carry intact. + */ +export function checkRenameBaselineCollisions( + baselineKeys: Iterable, + renames: Readonly> = RENAMED_DEFS, +): string[] { + const propsByDef = new Map>(); + for (const key of baselineKeys) { + const sep = key.indexOf(':'); + // A bare def key names no property, so it cannot collide with one. + if (sep < 0) continue; + const def = key.slice(0, sep); + let props = propsByDef.get(def); + if (props === undefined) propsByDef.set(def, (props = new Set())); + props.add(key.slice(sep + 1)); + } + const problems: string[] = []; + for (const [from, to] of Object.entries(renames)) { + // A self-rename collides with itself on every key; rule 1 of + // `checkRenameTable` already names it, and a second line would bury it. + if (from === to) continue; + const source = propsByDef.get(from); + const target = propsByDef.get(to); + if (source === undefined || target === undefined) continue; + const collisions = [...source].filter((prop) => target.has(prop)).sort(); + if (collisions.length === 0) continue; + problems.push( + `${from} → ${to}: the baseline already records ${collisions.length} of this ` + + `rename's property name(s) under the TARGET def — ${collisions.join(', ')}. ` + + `Carrying the rename collapses each pair onto one key (last write wins), and ` + + `takes the losing side's recorded retired state and recorded default with it. ` + + `That is the same damage the two-sources-onto-one-target rule refuses, reached ` + + `by one rename instead of two, and it happens INSIDE the carry — before any ` + + `comparison runs — so the diff this build reports is computed against clobbered ` + + `input. Converging two defs on a shared property name is a real change: keep the ` + + `rename, and retire the losing side explicitly with \`retiredKey()\` plus its ` + + `registered ADR-0087 conversion, exactly as a retirement without a rename would ` + + `require. A merge whose two defs share no property name is lossless and is not ` + + `refused here.`, + ); + } + return problems; +} + export function checkRenameTable( emittedDefs: ReadonlySet, renames: Readonly> = RENAMED_DEFS, diff --git a/packages/spec/scripts/renamed-defs.test.ts b/packages/spec/scripts/renamed-defs.test.ts index 47d746ed94..7ee21acbd4 100644 --- a/packages/spec/scripts/renamed-defs.test.ts +++ b/packages/spec/scripts/renamed-defs.test.ts @@ -18,6 +18,7 @@ import { describe, it, expect } from 'vitest'; import { RENAMED_DEFS, carryAuthorableKey, + checkRenameBaselineCollisions, checkRenameTable, } from './lib/renamed-defs'; @@ -137,6 +138,112 @@ describe('checkRenameTable', () => { }); }); +describe('checkRenameBaselineCollisions — a rename may MOVE keys, never MERGE them (#17383)', () => { + const renames = { 'integration/Old': 'integration/New' } as const; + + // The shape the four rules of `checkRenameTable` cannot see. It is ONE rename, + // so the two-sources rule never fires; the source is unemitted and the target + // is emitted, so both decay rules pass. The damage is in the BASELINE, and + // every carry in build-schemas.ts is a plain `Map.set` on the carried key. + it('refuses a rename whose target already holds the same property name, and names it', () => { + const problems = checkRenameBaselineCollisions( + ['integration/New:mode', 'integration/Old:mode'], + renames, + ); + expect(problems).toHaveLength(1); + expect(problems[0]).toContain('integration/Old → integration/New'); + expect(problems[0]).toContain('under the TARGET def — mode'); + // The remedy must be the one the two-sources rule already prescribes. + expect(problems[0]).toContain('retiredKey()'); + }); + + it('is silent where checkRenameTable is loud, and loud where it is silent', () => { + // The discriminator, stated as one assertion: the SAME baseline damage is + // invisible to the emitted-def rules, which is why this rule exists. + const baseline = ['integration/New:mode', 'integration/Old:mode']; + expect(checkRenameTable(new Set(['integration/New']), renames)).toEqual([]); + expect(checkRenameBaselineCollisions(baseline, renames)).toHaveLength(1); + }); + + it('reports EVERY colliding property, sorted, not just the first', () => { + const problems = checkRenameBaselineCollisions( + [ + 'integration/New:alpha', 'integration/New:beta', 'integration/New:gamma', + 'integration/Old:beta', 'integration/Old:alpha', 'integration/Old:delta', + ], + renames, + ); + expect(problems).toHaveLength(1); + expect(problems[0]).toContain('2 of this'); + expect(problems[0]).toContain('alpha, beta'); + }); + + // ─── What it must NOT refuse — the cost this rule can impose ───────────── + // A `Map.set` can only collapse two entries that are the SAME key, so a merge + // whose two defs share no property NAME writes every key exactly once and + // loses nothing. Refusing a populated target as such would redden 24 of the + // committed entries, whose targets all hold keys once the surface snapshot has + // been regenerated under the new name — and it would forbid the in-tree + // `cloud/Sha256Digest → system/Sha256Digest` shape outright. + + it('ACCEPTS a merge into a populated target whose property names are disjoint', () => { + expect( + checkRenameBaselineCollisions( + ['integration/New:kept', 'integration/Old:moved'], + renames, + ), + ).toEqual([]); + }); + + it('ACCEPTS the landing shape: the baseline holds the keys under the SOURCE only', () => { + // The real `authorable-surface.base.json` shape while a rename lands — the + // upstream anchor predates it, so the target has no keys there at all. + expect( + checkRenameBaselineCollisions(['integration/Old:mode', 'integration/Old:other'], renames), + ).toEqual([]); + }); + + it('ACCEPTS the settled shape: the baseline holds the keys under the TARGET only', () => { + // The committed `authorable-surface/` shape after regeneration — the entry is + // inert against the snapshot but still enforces the hygiene invariants. + expect( + checkRenameBaselineCollisions(['integration/New:mode', 'integration/New:other'], renames), + ).toEqual([]); + }); + + it('matches the def exactly — a def that merely shares a prefix is not the target', () => { + expect( + checkRenameBaselineCollisions(['integration/NewThing:mode', 'integration/Old:mode'], renames), + ).toEqual([]); + }); + + it('ignores bare def keys, which name no property and so can collide with none', () => { + expect(checkRenameBaselineCollisions(['integration/New', 'integration/Old'], renames)).toEqual([]); + }); + + it('splits on the FIRST separator, so a property containing ":" still collides', () => { + const problems = checkRenameBaselineCollisions( + ['integration/New:a:b', 'integration/Old:a:b'], + renames, + ); + expect(problems).toHaveLength(1); + expect(problems[0]).toContain('a:b'); + }); + + it('leaves a self-rename to checkRenameTable rather than reporting it twice', () => { + // Every key of a self-rename collides with itself; rule 1 already names the + // entry, and a second line would bury the real diagnosis. + expect( + checkRenameBaselineCollisions(['integration/Old:mode'], { + 'integration/Old': 'integration/Old', + }), + ).toEqual([]); + expect( + checkRenameTable(new Set(['integration/Old']), { 'integration/Old': 'integration/Old' }), + ).toHaveLength(1); + }); +}); + describe('the committed RENAMED_DEFS table', () => { it('no longer carries the #4684 connector rate-limit rename — #4911 absorbed it', () => { // The #4684 rename (`integration/RateLimitConfig` → From 00307017fa455b69c9c3893965ee382498b389bb Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 02:47:16 +0000 Subject: [PATCH 2/2] test(spec): pin the baseline-collision refusal, and correct a fixture that modelled an impossible baseline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three pins, two of them about the cost this guard can impose. The existing carry fixture INJECTED the old key into the base while leaving the carried one in place, so the anchor recorded `source` under both defs at once — a shape no real landing produces (an anchor from before the rename holds the property under the OLD def only) and the one the new guard refuses outright. Measured, it now exits 1 before the check it was written for is reached, so the fixture removes the carried key as well and models the real pre-rename anchor. The new pins: the colliding baseline is refused, names both defs and the property, and writes nothing; and a rename into a target that is still POPULATED in the base stays green when no property name is shared, because `Map.set` can only collapse entries that are the same key. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01MkQhmuuJAVDjmeWNixwDDH --- .../scripts/build-schemas-check-mode.test.ts | 74 ++++++++++++++++++- 1 file changed, 73 insertions(+), 1 deletion(-) diff --git a/packages/spec/scripts/build-schemas-check-mode.test.ts b/packages/spec/scripts/build-schemas-check-mode.test.ts index 8c98fec6a3..8a199dad1c 100644 --- a/packages/spec/scripts/build-schemas-check-mode.test.ts +++ b/packages/spec/scripts/build-schemas-check-mode.test.ts @@ -975,6 +975,11 @@ const DELETED_AGED = `data/Object:${DELETED_AGED_LEAF} [RETIRED]`; * fixture vacuous. */ const DELETED_BY_RENAME_SOURCE_DEF = 'integration/FieldMapping'; const DELETED_BY_RENAME = `${DELETED_BY_RENAME_SOURCE_DEF}:source`; +/** The SAME property under the rename's TARGET def. The committed surface is the + * post-rename snapshot, so it records this one and not `DELETED_BY_RENAME`; an + * upstream anchor from before the rename is the mirror image, and holding both + * at once is the #17383 collision. */ +const CARRIED_BY_RENAME = `${RENAMED_DEFS[DELETED_BY_RENAME_SOURCE_DEF]}:source`; describe('build-schemas.ts — deleted baseline lines must prove themselves (#4650)', () => { beforeAll(() => { @@ -1266,7 +1271,14 @@ describe('build-schemas.ts — deleted baseline lines must prove themselves (#46 expect(pristineSurface).toContain( `${RENAMED_DEFS[DELETED_BY_RENAME_SOURCE_DEF]}:source`, ); - seedBase((s) => [...s, DELETED_BY_RENAME].sort()); + // ⚠️ The old key is INJECTED and the carried one REMOVED, which is what an + // upstream anchor from before the rename really looks like: the property + // is recorded under the OLD def and not yet under the new one. Injecting + // alone left the base recording `source` under BOTH defs — a shape no + // real landing produces, and one #17383's collision guard now refuses + // outright (measured: the run exits 1 before this check is reached), so + // the fixture would have been asserting about a build that never got here. + seedBase((s) => [...s.filter((k) => k !== CARRIED_BY_RENAME), DELETED_BY_RENAME].sort()); seedSurface((s) => s); const { status, output } = run(['--check']); @@ -1276,6 +1288,66 @@ describe('build-schemas.ts — deleted baseline lines must prove themselves (#46 expect(status).toBe(0); }, ); + + // ─── #17383 — a rename may MOVE keys, it may never MERGE two onto one ───── + // `checkRenameTable` validates the table against the defs the build EMITS, so + // this shape is invisible to it: one well-formed rename, source unemitted, + // target emitted. The damage is in the BASELINE, where the carry's plain + // `Map.set` collapses the two entries and drops one side's recorded retired + // state and default — before any ratchet below runs. + + it( + 'refuses a rename whose baseline records the same property under BOTH defs, and writes nothing', + { timeout: SPAWN_TIMEOUT_MS }, + () => { + // The target keeps `CARRIED_BY_RENAME`, so the base records `source` under + // the source def AND the target def. That is the collision. + seedBase((s) => [...s, DELETED_BY_RENAME].sort()); + const surfaceBytes = seedSurface((s) => s); + + const { status, output } = run(['--check']); + + expect(status).toBe(1); + expect(output).toContain('would COLLAPSE keys of the upstream baseline'); + expect(output).toContain( + `${DELETED_BY_RENAME_SOURCE_DEF} → ${RENAMED_DEFS[DELETED_BY_RENAME_SOURCE_DEF]}`, + ); + expect(output).toContain('under the TARGET def — source'); + // The remedy is the one the two-sources rule already prescribes. + expect(output).toContain('retiredKey()'); + // A check reports; it does not write (#4711). + expect(readSurface()).toBe(surfaceBytes); + }, + ); + + it( + 'does NOT refuse a rename into a populated target when no property name is shared', + { timeout: SPAWN_TIMEOUT_MS }, + () => { + // The cost this guard can impose, pinned: the target def is populated in + // the base (six other keys survive the filter below), and the carried + // property name is not one of them. `Map.set` collapses only entries that + // are the SAME key, so this merge writes every key exactly once and loses + // nothing. Refusing a populated target as such would redden most of the + // committed table. + const baseKeys = [ + ...pristineSurface.filter((k) => k !== CARRIED_BY_RENAME), + DELETED_BY_RENAME, + ].sort(); + const targetDef = RENAMED_DEFS[DELETED_BY_RENAME_SOURCE_DEF]; + expect( + baseKeys.filter((k) => k.startsWith(`${targetDef}:`)).length, + 'the target def must still hold keys in the base, or this pins nothing', + ).toBeGreaterThan(0); + seedBase(() => baseKeys); + seedSurface((s) => s); + + const { status, output } = run(['--check']); + + expect(output).not.toContain('would COLLAPSE keys'); + expect(status).toBe(0); + }, + ); }); // ─────────────────────────────────────────────────────────────────────────────