diff --git a/packages/spec/scripts/build-schemas-check-mode.test.ts b/packages/spec/scripts/build-schemas-check-mode.test.ts index 8c98fec6a31..8a199dad1cf 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); + }, + ); }); // ───────────────────────────────────────────────────────────────────────────── diff --git a/packages/spec/scripts/build-schemas.ts b/packages/spec/scripts/build-schemas.ts index 3cb8119931c..90466d3694c 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 11231db63c5..f32305e7663 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 47d746ed941..7ee21acbd47 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` →