fix(spec): refuse a declared def rename that would collapse two baseline keys onto one

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);
+    },
+  );
 });
 
 // ─────────────────────────────────────────────────────────────────────────────

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<string>): 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<string, boolean>(
     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<string, boolean>();
     for (const entry of base.doc.keys ?? []) {
       const key = entry.replace(RETIRED_MARK, '');

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<string>,
+  renames: Readonly<Record<string, string>> = RENAMED_DEFS,
+): string[] {
+  const propsByDef = new Map<string, Set<string>>();
+  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<string>()));
+    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<string>,
   renames: Readonly<Record<string, string>> = RENAMED_DEFS,

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` →