From fe725231af2507f471d703ba08aebbf89aad0b90 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 02:21:54 +0000 Subject: [PATCH 1/2] fix(spec): resolve a shadowed ADR-0087 alias by VALUE, not by always keeping it (#4923) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `renameKey` / `renameConfigKey` did nothing whenever the canonical key was already present, so every D2 rename left the retired spelling sitting in the converted metadata — the conversion had not finished converting. Invisible while the flow-node config contracts were `.strip`; an execute-time guard refusal once #4001 batch 9 made them strict. Per the maintainer ruling recorded on the issue, the case now splits on whether the two values disagree: - equal (structural equality) -> the alias carries nothing the canonical key does not, so it is deleted and the usual notice fires. Lossless hygiene, inside the D2 contract, and idempotent in both shape and notices. - different -> both keys survive and nothing is emitted. Two spellings with two values is genuine author ambiguity; picking one would edit a config the customer never agreed to. The surviving pair is what lets the strict gates refuse with a prescription naming BOTH keys. `renameConfigKey` now delegates to `renameKey` so the rule has one definition, and the structural-equality predicate is the one the composer already used for "same value composes fine" (#5005), extracted to `shared/deep-equal.ts` so the two surfaces cannot disagree about what "the same" means. `liftNotifySourceShape` follows the same rule: a nested part repeating its flat counterpart is redundant (and `source` is dropped), a part that disagrees leaves the node entirely untouched so `source` reaches the strict contract. The `wait` node's loose-key lift is deliberately NOT covered and says so — it moves keys between locations rather than resolving two spellings of one slot. Batch-9 guidance reworded: a surviving twin is no longer "dead", it holds a value the canonical key does not, so each prescription names both keys and asks for a decision. Also adopts `mapPageComponents` (#5509) for protocol-15 `page-component-visibility-to-visibleWhen` per #5511 — its existing fixture is unchanged and stays green, which is the equivalence acceptance. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018fxLGQdatPbBUvCgiVxg6D --- .../conversion-shadowed-alias-by-value.md | 51 +++++ .../automation/builtin-node-config.test.ts | 20 +- .../src/automation/builtin-node-config.zod.ts | 38 +++- .../spec/src/automation/io-node-config.zod.ts | 38 ++-- .../automation/schemaless-node-config.zod.ts | 22 +- .../spec/src/conversions/conversions.test.ts | 158 ++++++++++++- packages/spec/src/conversions/registry.ts | 209 +++++++++++------- packages/spec/src/conversions/stored.test.ts | 13 +- packages/spec/src/conversions/walk.ts | 60 ++++- packages/spec/src/shared/deep-equal.ts | 73 ++++++ packages/spec/src/stack.zod.ts | 31 +-- 11 files changed, 547 insertions(+), 166 deletions(-) create mode 100644 .changeset/conversion-shadowed-alias-by-value.md create mode 100644 packages/spec/src/shared/deep-equal.ts diff --git a/.changeset/conversion-shadowed-alias-by-value.md b/.changeset/conversion-shadowed-alias-by-value.md new file mode 100644 index 0000000000..c5820fe9d4 --- /dev/null +++ b/.changeset/conversion-shadowed-alias-by-value.md @@ -0,0 +1,51 @@ +--- +"@objectstack/spec": patch +--- + +fix(spec): an ADR-0087 rename resolves a shadowed alias by VALUE, instead of always leaving it behind (#4923) + +Every ADR-0087 D2 conversion that renames a key (`renameKey` / +`renameConfigKey`, so `object` → `objectName`, `filters` → `filter`, +`flow` → `flowName`, the four `notify` aliases, `description` → `subtitle`, +the datasource driver-config aliases, …) used to do **nothing at all** when the +canonical key was already present. The retired spelling then stayed in the +converted metadata forever — the conversion had not finished converting. + +That was invisible while flow-node config contracts were `.strip`: the dead key +was silently dropped at the execute-time parse and the node ran. Once #4001 批 9 +made those contracts strict it stopped being invisible — a stored `subflow` +carrying `{ flowName, flow }` loads today and would be refused at execute time +as a guard failure (not routable through a `fault` edge). + +**What changes.** A rename that meets both spellings now splits on whether the +two values actually disagree: + +- **Same value** (structural equality, so two separately-authored + `{ status: 'stale' }` filters count as one declaration) → the alias carries + nothing the canonical key does not, so it is **deleted** and the conversion + emits its usual notice. Lossless hygiene, and it makes the transform + idempotent in both shape and notices. +- **Different values** → **both keys are kept** and no notice is emitted. Two + spellings holding two different values is genuine author ambiguity, and an + upgrade tool that silently picked the canonical one would be editing a + configuration the customer never agreed to. The surviving pair is what lets + the strict node-config gates refuse with a prescription that **names both + keys** and asks for a decision. + +`notify`'s nested `source: { object, id }` lift follows the same rule: a part +that repeats its flat counterpart is redundant and `source` is dropped, while a +part that disagrees leaves the node **entirely** untouched so `source` reaches +the strict contract intact. (The `wait` node's loose-key lift is deliberately +NOT covered — it moves keys between two locations rather than resolving two +spellings of one slot.) + +**What an author sees.** Metadata that named one slot twice with the same value +loses the retired spelling at load and gains one deprecation notice per removal +— the same notice a plain rename already emitted, so `objectstack validate` +output is unchanged in kind. Metadata that named one slot twice with *different* +values is unchanged by the conversion and is now refused by the strict +node-config contracts with a message naming both keys; the fix is to decide +which value is right, put it on the canonical key, and delete the alias. The +batch-9 prescriptions were reworded accordingly: they no longer say the +surviving twin is "dead" (true only under the old rule), because a key that now +reaches that parse holds a value the canonical key does not. diff --git a/packages/spec/src/automation/builtin-node-config.test.ts b/packages/spec/src/automation/builtin-node-config.test.ts index dbe9eeff4b..f7ad4ca4a8 100644 --- a/packages/spec/src/automation/builtin-node-config.test.ts +++ b/packages/spec/src/automation/builtin-node-config.test.ts @@ -244,12 +244,18 @@ describe('MapConfigSchema — strict as of #4001 批 9', () => { expect(message).toMatch(/delete/i); }); - it('rejects the SHADOWED alias the ADR-0087 conversion deliberately leaves behind', () => { - // `renameConfigKey` does nothing when the canonical key is already - // present, so `{ flowName, flow }` survives the load-path conversion - // intact — and under `.strip` the dead twin was then deleted in silence at - // this parse. That silence is the whole point of #4001: the author wrote - // two names for one thing and the platform picked one without saying so. - expect(MapConfigSchema.safeParse({ collection: '{r}', flowName: 'per_row', flow: 'ignored' }).success).toBe(false); + it('rejects the AMBIGUOUS pair the ADR-0087 conversion deliberately leaves behind', () => { + // Since #4923 `renameConfigKey` resolves `{ flowName, flow }` itself when + // the two agree, so the pair that survives the load-path conversion to + // reach this parse is the one naming two DIFFERENT flows. Under `.strip` + // the loser was then deleted in silence — the whole point of #4001: the + // author wrote two names for one thing and the platform picked one without + // saying so. This is the case where picking would be a guess, so the + // refusal names both keys instead. + expect(MapConfigSchema.safeParse({ collection: '{r}', flowName: 'per_row', flow: 'per_row_v2' }).success) + .toBe(false); + const message = unknownKeyMessage(MapConfigSchema, { collection: '{r}', flowName: 'per_row', flow: 'per_row_v2' })!; + expect(message).toContain('`flow`'); + expect(message).toContain('`flowName`'); }); }); diff --git a/packages/spec/src/automation/builtin-node-config.zod.ts b/packages/spec/src/automation/builtin-node-config.zod.ts index b3bd7c284d..93b9fff467 100644 --- a/packages/spec/src/automation/builtin-node-config.zod.ts +++ b/packages/spec/src/automation/builtin-node-config.zod.ts @@ -84,11 +84,23 @@ const BUILTIN_NODE_CONFIG_HISTORY = * The two ADR-0087 D2 aliases every CRUD node shares. * * Both are retired SPELLINGS rather than typos, and both are rewritten at load - * (`flow-node-crud-object-alias`, `flow-node-crud-filter-alias`), so a config - * still carrying one at parse time carries the canonical key too — - * `renameConfigKey` leaves a shadowed alias in place instead of clobbering the - * winner. Hence each prescription answers both readings: the rename, and - * "delete the dead twin". + * (`flow-node-crud-object-alias`, `flow-node-crud-filter-alias`). So each + * prescription has to answer two different readers, and since #4923 the split + * between them is exact: + * + * - whoever **parses this contract directly** (never went through the load + * path) wrote the retired spelling on its own → the fix is the rename; + * - whoever **came through the load path** still has the alias only because + * the conversion refused to resolve it, and it refuses in exactly one + * situation: the canonical key is ALSO present and carries a DIFFERENT + * value. An identical twin is deleted by the conversion now, so it can no + * longer reach this parse. + * + * That second reader is why the prescription names BOTH keys and asks for a + * decision rather than a deletion. Telling them "the canonical one already won, + * delete yours" — true while a shadowed alias was left in place — would now be + * advice to discard the one value the platform deliberately declined to discard + * on their behalf. * * `object` also earns its entry on distance alone: `object` → `objectName` is * four edits against a threshold of two, so the suggester would say nothing at @@ -97,13 +109,17 @@ const BUILTIN_NODE_CONFIG_HISTORY = const CRUD_ALIAS_GUIDANCE = { object: 'The object slot is `objectName`. `object` was the last tenant of the `readAliasedConfig` executor shim; it ' - + 'graduated into the ADR-0087 D2 conversion `flow-node-crud-object-alias` (#3796), which rewrites it at load — ' - + 'so a surviving `object` means `objectName` already won and this key is dead. Delete it.', + + 'graduated into the ADR-0087 D2 conversion `flow-node-crud-object-alias` (#3796), which rewrites it at load. ' + + 'If `objectName` is also present, this node names two different objects and the conversion left both keys ' + + 'alone rather than picking for you (#4923) — decide which object this node acts on, put it on `objectName`, ' + + 'and delete `object`.', filters: 'The match map is `filter` (singular). `filters` was a consumer-side executor fallback that graduated into the ' + 'ADR-0087 D2 conversion `flow-node-crud-filter-alias`, which rewrites it at load; delete it once `filter` ' - + 'carries the pairs. Beware the half-migrated shape: an empty `filter` next to a populated `filters` is what ' - + 'made this alias dangerous enough to declare (#3810 — a match-everything write).', + + 'carries the pairs. If `filter` is also present, the two carry DIFFERENT match maps and the conversion kept ' + + 'both rather than choosing (#4923) — reconcile them onto `filter`. Beware the half-migrated shape: an empty ' + + '`filter` next to a populated `filters` is what made this alias dangerous enough to declare (#3810 — a ' + + 'match-everything write).', } as const; /** @@ -457,7 +473,9 @@ export const MapConfigSchema = lazySchema(() => strictObject({ flow: 'The per-item subflow is named by `flowName`. `flow` was an undeclared executor fallback no schema or form ' + 'described; it graduated into the ADR-0087 D2 conversion `flow-node-map-flow-alias` (#4045), which rewrites ' - + 'it at load — so a surviving `flow` means `flowName` already won and this key is dead. Delete it.', + + 'it at load. If `flowName` is also present, the two name DIFFERENT subflows and the conversion kept both ' + + 'rather than picking one to run per item (#4923) — decide which flow this is, put it on `flowName`, and ' + + 'delete `flow`.', }, }, { /** The collection — a `{token}` template / bare variable name, or an inline array. */ diff --git a/packages/spec/src/automation/io-node-config.zod.ts b/packages/spec/src/automation/io-node-config.zod.ts index e68fe4ce7e..5ea11e9d1c 100644 --- a/packages/spec/src/automation/io-node-config.zod.ts +++ b/packages/spec/src/automation/io-node-config.zod.ts @@ -74,34 +74,44 @@ const IO_NODE_CONFIG_HISTORY = * * Each is a RETIRED SPELLING, not a typo, so a bare "did you mean" would * under-serve it: `flow-node-notify-config-aliases` rewrites all five at load - * (including the `registerFlow` rehydration seam), which means a config that - * still carries one when it reaches this parse carries the canonical key too — - * `renameConfigKey` leaves a SHADOWED alias in place rather than clobbering the - * winner. So each prescription answers both readings: the rename, for whoever - * parses this contract directly, and "delete the dead twin", for whoever came - * through the load path. + * (including the `registerFlow` rehydration seam). Since #4923 that rewrite + * also DELETES a retired spelling that merely repeats the canonical key's + * value, which sharpens what a surviving one means: it survived because the + * canonical key is also present and says something DIFFERENT, and the + * conversion will not pick between two values the author wrote. + * + * So each prescription answers both readings — the rename, for whoever parses + * this contract directly, and a reconciliation naming BOTH keys, for whoever + * came through the load path. */ const NOTIFY_KEY_GUIDANCE: Readonly> = { to: 'The recipient slot is `recipients`. `to` is the pre-17 spelling, rewritten at load by the ADR-0087 D2 ' - + 'conversion `flow-node-notify-config-aliases` — so if `recipients` is already present, the conversion left ' - + '`to` behind as a dead twin (a shadowed alias is not clobbered) and it should be deleted.', + + 'conversion `flow-node-notify-config-aliases` — so if `recipients` is also present, the two name DIFFERENT ' + + 'recipients and the conversion kept both rather than choosing who gets notified (#4923). Decide the ' + + 'recipients, put them on `recipients`, and delete `to`.', subject: 'The heading slot is `title`. `subject` is the pre-17 spelling rewritten at load by ' - + '`flow-node-notify-config-aliases`; delete it once `title` carries the text.', + + '`flow-node-notify-config-aliases`; delete it once `title` carries the text. If `title` is also present with ' + + 'DIFFERENT text, the conversion kept both rather than choosing (#4923) — reconcile them onto `title`.', body: 'The body slot is `message`. `body` is the pre-17 spelling rewritten at load by ' - + '`flow-node-notify-config-aliases`; delete it once `message` carries the text. (`body` IS canonical on an ' - + '`http` node — the key is wrong only here.)', + + '`flow-node-notify-config-aliases`; delete it once `message` carries the text. If `message` is also present ' + + 'with DIFFERENT text, the conversion kept both rather than choosing (#4923) — reconcile them onto `message`. ' + + '(`body` IS canonical on an `http` node — the key is wrong only here.)', url: 'The click-through slot is `actionUrl`. It was renamed at 17 because `url` elsewhere on the platform means ' + '"HTTP endpoint to call" (`http` node, webhooks), a different concept from an in-app click target. ' - + '`flow-node-notify-config-aliases` rewrites it at load; delete it once `actionUrl` carries the link.', + + '`flow-node-notify-config-aliases` rewrites it at load; delete it once `actionUrl` carries the link. If ' + + '`actionUrl` is also present with a DIFFERENT link, the conversion kept both rather than choosing where the ' + + 'notification points (#4923) — reconcile them onto `actionUrl`.', source: 'The click-through target is the flat PAIR `sourceObject` + `sourceId`, never a nested `source: { object, id }`. ' + '`flow-node-notify-config-aliases` lifts the nested shape at load and drops it once every part is accounted ' - + 'for, so a surviving `source` means both flat keys were already set — delete it. Note the pair only takes ' - + 'effect together: a half-specified target is dropped so the inbox never renders a dead link.', + + 'for, so a surviving `source` means a part of it DISAGREES with the flat key already holding that slot, and ' + + 'the conversion declined to pick (#4923) — reconcile onto `sourceObject` / `sourceId` and delete `source`. ' + + 'Note the pair only takes effect together: a half-specified target is dropped so the inbox never renders a ' + + 'dead link.', }; // ─── notify ────────────────────────────────────────────────────────── diff --git a/packages/spec/src/automation/schemaless-node-config.zod.ts b/packages/spec/src/automation/schemaless-node-config.zod.ts index 62f0b966eb..c64530e912 100644 --- a/packages/spec/src/automation/schemaless-node-config.zod.ts +++ b/packages/spec/src/automation/schemaless-node-config.zod.ts @@ -119,10 +119,11 @@ const SCHEMALESS_NODE_CONFIG_HISTORY = * `functionName` and `input` are retired SPELLINGS that * `flow-node-script-config-aliases` rewrites at load, so — like the notify * family — a config still carrying one at parse time carries the canonical key - * too (`renameConfigKey` leaves a shadowed alias alone). `input` earns its - * entry twice over: edit distance would suggest `inputs` without ever saying - * that `input` is *canonical* on `connector_action`'s `connectorConfig`, which - * is where the spelling leaked in from and where it must NOT be changed. + * too, and since #4923 it carries a canonical key holding a DIFFERENT value + * (an identical twin is deleted by the conversion). `input` earns its entry + * twice over: edit distance would suggest `inputs` without ever saying that + * `input` is *canonical* on `connector_action`'s `connectorConfig`, which is + * where the spelling leaked in from and where it must NOT be changed. * * The five `actionType`-branch keys need no entry here: `retiredKey()` puts the * prescription in the shape itself, which is strictly stronger (it also types @@ -132,12 +133,15 @@ const SCHEMALESS_NODE_CONFIG_HISTORY = const SCRIPT_KEY_GUIDANCE: Readonly> = { functionName: 'The callable reference is `function` (#1870). `functionName` was the AI/template-emitted alias, rewritten at ' - + 'load by the ADR-0087 D2 conversion `flow-node-script-config-aliases`; if `function` is already present the ' - + 'conversion left `functionName` behind as a dead twin — delete it.', + + 'load by the ADR-0087 D2 conversion `flow-node-script-config-aliases`. If `function` is also present, the two ' + + 'name DIFFERENT callables and the conversion kept both rather than picking which one runs (#4923) — decide ' + + 'which it is, put it on `function`, and delete `functionName`.', input: 'The input map on a `script` node is `inputs` (plural). The singular `input` leaked in from ' + "`connector_action`, where `connectorConfig.input` is a DIFFERENT and canonical surface — do not \"fix\" that " - + 'one. `flow-node-script-config-aliases` rewrites this key at load; delete it once `inputs` carries the values.', + + 'one. `flow-node-script-config-aliases` rewrites this key at load; delete it once `inputs` carries the values. ' + + 'If `inputs` is also present with DIFFERENT values, the conversion kept both rather than choosing (#4923) — ' + + 'reconcile them onto `inputs`.', }; /** `subflow` prescriptions — one retired spelling, one wrong layer. */ @@ -145,7 +149,9 @@ const SUBFLOW_KEY_GUIDANCE: Readonly> = { flow: 'The invoked flow is named by `flowName`. `flow` was an undeclared executor fallback that no schema or form ' + 'ever described; it graduated into the ADR-0087 D2 conversion `flow-node-subflow-flow-alias` (#4278), which ' - + 'rewrites it at load — so a surviving `flow` means `flowName` already won and this key is dead. Delete it.', + + 'rewrites it at load. If `flowName` is also present, the two name DIFFERENT flows and the conversion kept ' + + 'both rather than picking which one this step invokes (#4923) — decide which it is, put it on `flowName`, ' + + 'and delete `flow`.', timeoutMs: "A subflow step's timeout is the engine's per-node guard, so it belongs on the NODE, not in its config: " + '`{ id, type: "subflow", timeoutMs: 30000, config: { … } }`. `FlowNodeSchema.timeoutMs` is the declared key.', diff --git a/packages/spec/src/conversions/conversions.test.ts b/packages/spec/src/conversions/conversions.test.ts index 0771cb62c0..e0a9aadedb 100644 --- a/packages/spec/src/conversions/conversions.test.ts +++ b/packages/spec/src/conversions/conversions.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from 'vitest'; +import { CreateRecordConfigSchema } from '../automation/builtin-node-config.zod.js'; import { FlowSchema } from '../automation/flow.zod.js'; import { ScriptConfigSchema } from '../automation/schemaless-node-config.zod.js'; import { normalizeStackInput } from '../shared/metadata-collection.zod.js'; @@ -136,7 +137,7 @@ describe('conversion layer (ADR-0087 D2)', () => { expect(notices).toHaveLength(1); }); - it('does not clobber an existing canonical filter', () => { + it('does not clobber an existing canonical filter that says something DIFFERENT', () => { const { stack, notices } = collectConversionNotices({ flows: [ { @@ -153,7 +154,146 @@ describe('conversion layer (ADR-0087 D2)', () => { }); const node = (stack.flows as any[])[0].nodes[0]; expect(node.config.filter).toEqual({ keep: true }); - expect(notices).toHaveLength(0); // canonical present → no conversion + // Two different match maps under two names is the ambiguous half of the + // #4923 ruling: BOTH survive, so the strict gate can refuse naming both. + expect(node.config.filters).toEqual({ drop: true }); + expect(notices).toHaveLength(0); + }); + }); + + /** + * The shadowed-alias rule, split by value (#4923). + * + * `renameKey` used to do nothing whenever the canonical key was already + * present, which left the retired spelling sitting in converted metadata + * forever — invisible while the node contracts were `.strip`, an execute-time + * refusal once #4001 批 9 made them strict. The maintainer ruling splits that + * case by whether the two values actually disagree; these tests pin both + * halves, because only one of them is a change. + */ + describe('shadowed alias, split by value (#4923)', () => { + const crudFlow = (config: Record) => ({ + flows: [{ name: 'f', nodes: [{ id: 'a', type: 'create_record', config }] }], + }); + const configOf = (stack: Record) => + (stack.flows as Array<{ nodes: Array<{ config: Record }> }>)[0]!.nodes[0]!.config; + + // ── Direction 1: equal values → the twin is deleted, loudly ────────── + // + // Predicted BEFORE the run, against unmodified code: red. The old + // `renameKey` returned `null` the moment `objectName` was present, so the + // dead `object` survived into `after` and no notice was emitted at all. + + it('deletes an alias whose value EQUALS the canonical key, and emits a notice', () => { + const { stack, notices } = collectConversionNotices( + crudFlow({ objectName: 'task', object: 'task' }), + ); + expect(configOf(stack)).toEqual({ objectName: 'task' }); + expect(configOf(stack)).not.toHaveProperty('object'); + expect(notices).toHaveLength(1); + expect(notices[0]).toMatchObject({ + conversionId: 'flow-node-crud-object-alias', + from: 'object', + to: 'objectName', + }); + }); + + it('compares STRUCTURALLY — two separately-authored identical filters are one declaration', () => { + // `===` would call these different and keep both; they are the same + // authored match map, so the retired spelling carries nothing. + const { stack, notices } = collectConversionNotices({ + flows: [ + { + name: 'f', + nodes: [ + { + id: 'a', + type: 'delete_record', + config: { filter: { status: 'stale' }, filters: { status: 'stale' } }, + }, + ], + }, + ], + }); + expect(configOf(stack)).toEqual({ filter: { status: 'stale' } }); + expect(notices).toHaveLength(1); + }); + + it('is idempotent in shape AND notices — the deduped result replays to itself', () => { + const once = collectConversionNotices(crudFlow({ objectName: 'task', object: 'task' })); + expect(once.notices).toHaveLength(1); + const twice = collectConversionNotices(once.stack); + expect(twice.stack).toEqual(once.stack); + expect(twice.notices).toHaveLength(0); + }); + + // ── Direction 2: different values → BOTH survive ───────────────────── + // + // Predicted BEFORE the run: GREEN against unmodified code too. Keeping a + // differing pair is what the old code already did for every pair, so this + // half is a REGRESSION PIN, not a proof of change. What the ruling + // actually changed here is the *reason* — and the prescription that reads + // it, pinned in the strict-gate test below. + + it('keeps BOTH spellings when the values differ — the upgrade tool does not choose', () => { + const before = crudFlow({ objectName: 'task', object: 'ticket' }); + const { stack, notices } = collectConversionNotices(structuredClone(before)); + expect(stack).toEqual(before); + expect(configOf(stack)).toEqual({ objectName: 'task', object: 'ticket' }); + expect(notices).toHaveLength(0); + }); + + it('judges each pair on its own value — one twin dedupes while its neighbour survives', () => { + const { stack, notices } = collectConversionNotices( + crudFlow({ objectName: 'task', object: 'task', filter: { a: 1 }, filters: { a: 2 } }), + ); + expect(configOf(stack)).toEqual({ objectName: 'task', filter: { a: 1 }, filters: { a: 2 } }); + expect(notices).toHaveLength(1); + expect(notices[0]!.from).toBe('object'); + }); + + it('applies the same rule one level up, on a plain dict rename (page header)', () => { + // `renameConfigKey` delegates to `renameKey`, so a non-`config` surface + // must not develop its own dialect of the rule. + const header = (properties: Record) => ({ + pages: [{ name: 'p', regions: [{ name: 'header', components: [{ type: 'page:header', properties }] }] }], + }); + const propsOf = (stack: Record) => + (stack.pages as Array<{ regions: Array<{ components: Array<{ properties: unknown }> }> }>)[0]! + .regions[0]!.components[0]!.properties; + + const equal = collectConversionNotices(header({ title: 'T', subtitle: 'One', description: 'One' })); + expect(propsOf(equal.stack)).toEqual({ title: 'T', subtitle: 'One' }); + expect(equal.notices).toHaveLength(1); + + const differs = header({ title: 'T', subtitle: 'One', description: 'Another' }); + const kept = collectConversionNotices(structuredClone(differs)); + expect(kept.stack).toEqual(differs); + expect(kept.notices).toHaveLength(0); + }); + + // ── The half the strict gate owns: the prescription names BOTH keys ─── + // + // Predicted BEFORE the run: red against unmodified guidance, which told + // the author the surviving twin was "dead" — true under the old rule and + // false under the new one, where a survivor means the two values disagree. + + it('the surviving pair is refused by the strict gate with BOTH keys named', () => { + const parsed = CreateRecordConfigSchema.safeParse({ + objectName: 'task', + object: 'ticket', + fields: { title: 'x' }, + }); + expect(parsed.success).toBe(false); + const message = parsed.success ? '' : parsed.error.issues.map((i) => i.message).join('\n'); + // Both spellings, so the author can see the disagreement they authored… + expect(message).toContain('`object`'); + expect(message).toContain('`objectName`'); + // …and the refusal must NOT claim the survivor is a dead duplicate: the + // conversion now deletes those, so a key that reached this parse carries + // a value that differs from the canonical one. + expect(message).not.toMatch(/already won and this key is dead/i); + expect(message).toMatch(/differ|different/i); }); }); @@ -446,7 +586,10 @@ describe('conversion layer (ADR-0087 D2)', () => { timerDuration: 'PT5M', timeoutMs: 60_000, }); - // The shadowed alias is not deleted — same treatment `renameConfigKey` gives one. + // The loose counterpart is not deleted. NOTE this is the wait-node LIFT + // (a loose key into a declared block), not a `renameKey` alias pair, so + // #4923's by-value split does not reach it — see the comment on + // WAIT_EVENT_CONFIG_LIFTS. expect(waitNodeOf(stack).config).toEqual({ duration: 'PT9M' }); expect(notices).toHaveLength(1); }); @@ -695,12 +838,13 @@ describe('conversion layer (ADR-0087 D2)', () => { expect(componentsOf(stack)[0]!.type).toBe('page-header'); }); - it('leaves a shadowed `description` alone when `subtitle` is already there (canonical wins)', () => { - // The house precedence `renameKey` encodes, same as - // flow-node-crud-object-alias: no rewrite, no notice, no deletion. + it('keeps a `description` that DISAGREES with an existing `subtitle` (both survive)', () => { + // The house precedence `renameKey` encodes since #4923, same as + // flow-node-crud-object-alias: two different second lines are the + // author's to reconcile, so no rewrite, no notice, no deletion. const before = pageWith({ type: 'page:header', - properties: { title: 'Both', subtitle: 'wins', description: 'ignored' }, + properties: { title: 'Both', subtitle: 'wins', description: 'other' }, }); const { stack, notices } = collectConversionNotices(structuredClone(before)); expect(stack).toEqual(before); diff --git a/packages/spec/src/conversions/registry.ts b/packages/spec/src/conversions/registry.ts index a07bcbb19e..b9f349916a 100644 --- a/packages/spec/src/conversions/registry.ts +++ b/packages/spec/src/conversions/registry.ts @@ -26,6 +26,7 @@ import { renameKey, } from './walk.js'; import { resolveDriverId, type BuiltinDriverId } from '../data/driver/config-registry.zod.js'; +import { deepEqualAuthored } from '../shared/deep-equal.js'; /** * Flow callout node type rename (protocol 11.0). @@ -550,34 +551,13 @@ const pageComponentVisibilityToVisibleWhen: MetadataConversion = { surface: 'page.component.visibility', summary: "page component key 'visibility' → 'visibleWhen' (ADR-0089)", apply(stack, emit) { - return mapPages(stack, (page, path) => { - const regions = page.regions; - if (!Array.isArray(regions)) return page; - let regionsChanged = false; - const nextRegions = regions.map((region, ri) => { - if (!region || typeof region !== 'object' || Array.isArray(region)) return region; - const dict = region as Record; - const components = dict.components; - if (!Array.isArray(components)) return region; - let componentsChanged = false; - const nextComponents = components.map((component, ci) => { - if (!component || typeof component !== 'object' || Array.isArray(component)) return component; - const mapped = renameVisibilityAlias( - component as Record, - 'visibility', - `${path}.regions[${ri}].components[${ci}]`, - emit, - ); - if (mapped !== component) componentsChanged = true; - return mapped; - }); - if (!componentsChanged) return region; - regionsChanged = true; - return { ...dict, components: nextComponents }; - }); - if (!regionsChanged) return page; - return { ...page, regions: nextRegions }; - }); + // The region→component descent this used to open-code is the shared + // `mapPageComponents` walker (#5509, adopted per #5511): identical + // copy-on-write contract, identical `pages[i].regions[j].components[k]` + // paths, and one fewer hand-rolled traversal to keep in step with + // `PageComponentSchema`'s reach. + return mapPageComponents(stack, (component, path) => + renameVisibilityAlias(component, 'visibility', path, emit)); }, fixture: { before: { @@ -923,8 +903,12 @@ const flowNodeCrudObjectAlias: MetadataConversion = { nodes: [ { id: 'n1', type: 'start' }, { id: 'n2', type: 'get_record', config: { object: 'lead', recordId: '{leadId}' } }, - // canonical already present → the shadowed alias is left alone (no notice) - { id: 'n3', type: 'create_record', config: { objectName: 'task', object: 'ignored' } }, + // Both spellings, DIFFERENT objects (#4923): real author ambiguity, + // so both keys survive and the strict gate refuses naming both. + { id: 'n3', type: 'create_record', config: { objectName: 'task', object: 'ticket' } }, + // Both spellings, SAME object: the alias carries nothing the + // canonical key does not, so it is deleted (with a notice). + { id: 'n4', type: 'update_record', config: { objectName: 'lead', object: 'lead' } }, ], }, ], @@ -936,12 +920,15 @@ const flowNodeCrudObjectAlias: MetadataConversion = { nodes: [ { id: 'n1', type: 'start' }, { id: 'n2', type: 'get_record', config: { objectName: 'lead', recordId: '{leadId}' } }, - { id: 'n3', type: 'create_record', config: { objectName: 'task', object: 'ignored' } }, + { id: 'n3', type: 'create_record', config: { objectName: 'task', object: 'ticket' } }, + { id: 'n4', type: 'update_record', config: { objectName: 'lead' } }, ], }, ], }, - expectedNotices: 1, + // n2's rename + n4's redundant-twin deletion. n3 is the ambiguous pair: no + // rewrite, no notice — the refusal is the strict gate's to make. + expectedNotices: 2, }, }; @@ -951,14 +938,26 @@ const flowNodeCrudObjectAlias: MetadataConversion = { * * The fifth notify alias, and the only one that is not a 1:1 rename — it is a * 1→2 destructuring, so {@link renameFlowConfigAliases}' pair mechanism cannot - * express it. Semantics mirror the `??` precedence the executor used to carry: - * a canonical key already present WINS and its nested counterpart is left - * shadowed, exactly as {@link renameConfigKey} treats a shadowed alias. - * - * `source` is dropped once at least one part was lifted — every part is by then - * either lifted or shadowed by a canonical key, so nothing observable is lost - * (the executor only ever read `.object` / `.id`). A `source` that is not a dict, - * or carries neither key, is left untouched rather than silently deleted. + * express it. It nevertheless follows {@link renameConfigKey}'s rule for a + * shadowed alias, which #4923 settled **by value**: + * + * - a nested part whose flat counterpart is ABSENT is lifted (a notice each); + * - a nested part that merely REPEATS the value already on its flat key is + * redundant — nothing to lift, but the part is accounted for, and the notice + * still fires so the removal is loud; + * - a nested part that DISAGREES with its flat key is genuine author + * ambiguity. The node is then left **entirely** untouched — no partial lift, + * no notice — so `source` survives to the strict `notify` contract, which + * refuses naming both the nested key and the flat pair. Choosing here would + * mean rewriting a click-through target the author never agreed to. + * + * `source` is dropped only when every part it carries was lifted or found + * redundant, so nothing observable is lost. Leaving the ambiguous node whole + * rather than half-converted also keeps the pass idempotent: a replay of the + * unresolved shape re-derives the same verdict and emits nothing. + * + * A `source` that is not a dict, or carries neither key, is left untouched + * rather than silently deleted. */ function liftNotifySourceShape(stack: Dict, emit: Emit): Dict { return mapFlowNodes(stack, (node, path) => { @@ -969,15 +968,22 @@ function liftNotifySourceShape(stack: Dict, emit: Emit): Dict { if (!isDict(source)) return node; const nextConfig: Dict = { ...config }; - let lifted = false; + const pending: ConversionApplication[] = []; + let ambiguous = false; for (const [from, to] of [['object', 'sourceObject'], ['id', 'sourceId']] as const) { if (source[from] == null) continue; - if (nextConfig[to] != null) continue; // canonical already wins - nextConfig[to] = source[from]; - emit({ from: `source.${from}`, to, path: `${path}.config.${to}` }); - lifted = true; + if (nextConfig[to] == null) { + nextConfig[to] = source[from]; + } else if (!deepEqualAuthored(source[from], nextConfig[to])) { + ambiguous = true; + break; + } + pending.push({ from: `source.${from}`, to, path: `${path}.config.${to}` }); } - if (!lifted) return node; + // The author named one slot twice, differently — hand the whole shape to + // the strict gate rather than resolving part of it (#4923). + if (ambiguous || pending.length === 0) return node; + for (const application of pending) emit(application); delete nextConfig.source; return { ...node, config: nextConfig }; }); @@ -1043,15 +1049,29 @@ const flowNodeNotifyConfigAliases: MetadataConversion = { source: { object: 'showcase_task', id: '{record.id}' }, }, }, - // A canonical `sourceObject` WINS: only the unshadowed `id` is - // lifted, and `source` is dropped since every part is accounted for. + // `source.object` DISAGREES with the flat `sourceObject` (#4923). + // The node is left whole — not even the unambiguous `id` is + // lifted — so `source` reaches the strict `notify` contract and is + // refused there, naming both the nested key and the flat pair. { id: 'n3', type: 'notify', config: { recipients: ['{record.owner}'], sourceObject: 'showcase_project', - source: { object: 'ignored', id: '{record.project}' }, + source: { object: 'crm_account', id: '{record.project}' }, + }, + }, + // `source.object` merely REPEATS the flat `sourceObject`: redundant, + // so it counts as accounted for, `id` lifts, and `source` is + // dropped — two notices, nothing observable lost. + { + id: 'n4', + type: 'notify', + config: { + recipients: ['{record.reviewer}'], + sourceObject: 'showcase_task', + source: { object: 'showcase_task', id: '{record.task}' }, }, }, ], @@ -1083,16 +1103,26 @@ const flowNodeNotifyConfigAliases: MetadataConversion = { config: { recipients: ['{record.owner}'], sourceObject: 'showcase_project', - sourceId: '{record.project}', + source: { object: 'crm_account', id: '{record.project}' }, + }, + }, + { + id: 'n4', + type: 'notify', + config: { + recipients: ['{record.reviewer}'], + sourceObject: 'showcase_task', + sourceId: '{record.task}', }, }, ], }, ], }, - // 4 renames on n2 + `source.object`/`source.id` lifted on n2 + the single - // unshadowed `source.id` on n3 (its `source.object` is shadowed → no notice). - expectedNotices: 7, + // 4 renames on n2 + `source.object`/`source.id` lifted on n2 = 6; n3 is the + // ambiguous pair and converts nothing; n4 adds 2 (the redundant `object` + // and the lifted `id`). + expectedNotices: 8, }, }; @@ -1135,8 +1165,15 @@ const WAIT_EVENT_CONFIG_LIFTS: ReadonlyArray { const eh = flow.errorHandling; if (!eh || typeof eh !== 'object' || Array.isArray(eh)) return flow; @@ -4187,8 +4237,9 @@ const pageHeaderSubtitleAlias: MetadataConversion = { // The canonical type authored with the legacy key: converted too, // because today this second line is dropped on the floor. { type: 'page:header', properties: { title: 'Lead', description: 'One lead' } }, - // Canonical already present → the shadowed alias is left alone (no notice). - { type: 'page:header', properties: { title: 'Both', subtitle: 'wins', description: 'ignored' } }, + // Both spellings, DIFFERENT text (#4923): kept, so the author + // reconciles the two second lines rather than the loader picking. + { type: 'page:header', properties: { title: 'Both', subtitle: 'wins', description: 'other' } }, // `description` is this component's OWN declared prop (helper text) — untouched. { type: 'element:text_input', properties: { label: 'Note', description: 'Helper text' } }, ], @@ -4207,7 +4258,7 @@ const pageHeaderSubtitleAlias: MetadataConversion = { components: [ { type: 'page-header', properties: { title: 'Leads', subtitle: 'All open leads' } }, { type: 'page:header', properties: { title: 'Lead', subtitle: 'One lead' } }, - { type: 'page:header', properties: { title: 'Both', subtitle: 'wins', description: 'ignored' } }, + { type: 'page:header', properties: { title: 'Both', subtitle: 'wins', description: 'other' } }, { type: 'element:text_input', properties: { label: 'Note', description: 'Helper text' } }, ], }, diff --git a/packages/spec/src/conversions/stored.test.ts b/packages/spec/src/conversions/stored.test.ts index 3f6c088822..5c0a330705 100644 --- a/packages/spec/src/conversions/stored.test.ts +++ b/packages/spec/src/conversions/stored.test.ts @@ -116,12 +116,21 @@ describe('applyConversionsToStoredItem (stored sys_metadata rows, #3903)', () => expect(applyConversionsToStoredItem('datasource', row)).toBe(row); }); - it('lets a canonical key win over a shadowed legacy alias', () => { + it('keeps BOTH spellings on a stored row when they disagree', () => { const row = { name: 'ds', driver: 'sqlite', config: { filename: './real.db', file: './stale.db' } }; const out = applyConversionsToStoredItem('datasource', row) as { config: Record }; - // renameKey convention: canonical present → alias left shadowed, untouched. + // `renameKey` convention since #4923: two DIFFERENT files under two names + // is the author's ambiguity to resolve, so the replay picks neither. expect(out.config).toEqual({ filename: './real.db', file: './stale.db' }); }); + + it('drops a redundant alias on a stored row when it repeats the canonical value', () => { + const row = { name: 'ds', driver: 'sqlite', config: { filename: './real.db', file: './real.db' } }; + const out = applyConversionsToStoredItem('datasource', row) as { config: Record }; + // Data at rest gets the same hygiene as a fresh load (#3903 replays the + // full chain), so a rehydrated row is canonical rather than half-migrated. + expect(out.config).toEqual({ filename: './real.db' }); + }); }); it('threads the conflict guard context through (flow callers that own a registry)', () => { diff --git a/packages/spec/src/conversions/walk.ts b/packages/spec/src/conversions/walk.ts index 8cff62785c..4b2d58295a 100644 --- a/packages/spec/src/conversions/walk.ts +++ b/packages/spec/src/conversions/walk.ts @@ -14,6 +14,7 @@ */ import { FLOW_REGION_SLOTS_BY_TYPE } from '../automation/region-slots.js'; +import { deepEqualAuthored } from '../shared/deep-equal.js'; type Dict = Record; @@ -266,27 +267,66 @@ export function mapCollection( } /** - * Rename `dict[from]` → `dict[to]`, immutably, only when `from` is present - * (non-null) and the canonical `to` is absent. Returns `null` when there is - * nothing to do — the caller keeps the original reference. + * Rename `dict[from]` → `dict[to]`, immutably. Returns `null` when there is + * nothing to do — the caller keeps the original reference and emits no notice. + * + * Three cases, and the third is the one #4923 ruled on: + * + * 1. **`from` absent (or null)** → `null`. Nothing to convert. + * 2. **`from` present, `to` absent** → the plain rename. The value moves to the + * canonical key and the old spelling is deleted. + * 3. **BOTH present** — the author wrote two names for one slot — splits *by + * value*, because the two halves are genuinely different facts: + * - **values structurally equal** → the alias carries nothing the canonical + * key does not already carry, so deleting it is **lossless hygiene** and + * squarely inside the D2 contract ("the runtime only ever sees the + * canonical shape"). The alias is dropped and the caller emits its notice, + * so the rewrite stays loud. + * - **values differ** → `null`, and BOTH spellings survive. This is real + * author ambiguity, and an upgrade tool that picked the canonical one + * would be editing a configuration the customer never agreed to. The + * surviving pair is what lets the strict node-config gates refuse with a + * prescription that NAMES BOTH KEYS (`builtin-node-config.zod.ts` and + * friends) instead of the platform choosing silently. + * + * Before #4923 case 3 was uniformly "leave the alias shadowed", which is why + * every `renameFlowConfigAliases` entry's fixture used to pin a retired + * spelling surviving in its `after` half. Case 3's equal branch also makes the + * transform **idempotent in both shape and notices**: once the twin is gone, + * a replay hits case 1. + * + * Structural equality (not `===`) is the right test because these values are + * authored data, not identities: two separately-written `{ status: 'stale' }` + * filters are the same declaration. It is the same predicate the composer uses + * for "same value composes fine" (#5005) — deliberately one definition, so the + * two surfaces cannot disagree about whether a pair of values is "the same". */ export function renameKey(dict: Dict, from: string, to: string): Dict | null { if (!(from in dict) || dict[from] == null) return null; - if (dict[to] != null) return null; // canonical already wins — nothing to do const next: Dict = { ...dict }; + if (dict[to] != null) { + // Both spellings present (#4923). Only a redundant twin may be removed. + if (!deepEqualAuthored(dict[from], dict[to])) return null; + delete next[from]; + return next; + } next[to] = next[from]; delete next[from]; return next; } -/** Rename `config[from]` → `config[to]` on a node dict, immutably, only if `to` is absent. */ +/** + * Rename `config[from]` → `config[to]` on a node dict, immutably. + * + * A thin positional wrapper over {@link renameKey} — deliberately delegating + * rather than re-implementing, so the shadowed-alias rule #4923 settled has + * exactly one definition and a flow node's `config` cannot drift from every + * other dict the conversion layer rewrites. + */ export function renameConfigKey(node: Dict, from: string, to: string): Dict | null { const config = node.config; if (!isDict(config)) return null; - if (!(from in config) || config[from] == null) return null; - if (config[to] != null) return null; // canonical already wins — nothing to do - const nextConfig: Dict = { ...config }; - nextConfig[to] = nextConfig[from]; - delete nextConfig[from]; + const nextConfig = renameKey(config, from, to); + if (!nextConfig) return null; return { ...node, config: nextConfig }; } diff --git a/packages/spec/src/shared/deep-equal.ts b/packages/spec/src/shared/deep-equal.ts new file mode 100644 index 0000000000..a00b292fdd --- /dev/null +++ b/packages/spec/src/shared/deep-equal.ts @@ -0,0 +1,73 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Structural equality for **authored declarations** — the one predicate that + * answers "did the author write the same thing twice?". + * + * It exists because two independent surfaces need that exact question and must + * not answer it differently: + * + * - **compose** (#5005, `stack.zod.ts`): two stacks declaring the same key + * compose fine when the declarations are the SAME, and are a reported + * conflict when they differ; + * - **the ADR-0087 conversion layer** (#4923, `conversions/walk.ts`): an alias + * sitting next to its canonical key is redundant when the two values are the + * SAME (the conversion deletes it), and is genuine author ambiguity when + * they differ (both survive, and the strict gate refuses naming both). + * + * Both are the same judgement — *same declaration or not* — and neither is + * allowed to guess how to reconcile two different ones. Keeping one definition + * is what stops the conversion layer and the composer from disagreeing about + * whether a given pair of values is "the same", which would be visible to + * authors as two different verdicts on one stack. + * + * Deliberately strict and deliberately small: + * + * - keys explicitly set to `undefined` are treated as absent, matching how the + * composer reads a declaration in the first place; + * - callables compare by reference (`Object.is`) — two distinct closures are + * two distinct declarations, which is the honest answer for a config value; + * - anything past {@link MAX_COMPARE_DEPTH} compares as NOT equal. + * + * @internal — not re-exported from `shared/index`; import by path. + */ + +/** + * Depth ceiling for the recursion. + * + * A stack handed to `defineStack` is hand-built objects rather than parsed + * JSON, so a self-referencing value is reachable and would otherwise be an + * unbounded recursion on the load path — the same exposure, for the same + * reason, that `conversions/walk.ts` guards with `MAX_REGION_DEPTH`. + * + * Bailing out as **not equal** is the safe direction at both call sites: the + * composer reports a conflict naming the two stacks, and the conversion layer + * keeps both spellings so the strict gate names both keys. Neither degrades + * toward silently dropping an author's value. + */ +const MAX_COMPARE_DEPTH = 32; + +/** + * Deep structural equality between two authored values. + * + * @param a - left value + * @param b - right value + * @param depth - current recursion depth (internal) + */ +export function deepEqualAuthored(a: unknown, b: unknown, depth = 0): boolean { + if (Object.is(a, b)) return true; + if (depth >= MAX_COMPARE_DEPTH) return false; + if (typeof a !== 'object' || typeof b !== 'object' || a === null || b === null) return false; + if (Array.isArray(a) !== Array.isArray(b)) return false; + + if (Array.isArray(a) && Array.isArray(b)) { + return a.length === b.length && a.every((item, i) => deepEqualAuthored(item, b[i], depth + 1)); + } + + const left = a as Record; + const right = b as Record; + const leftKeys = Object.keys(left).filter((k) => left[k] !== undefined); + const rightKeys = Object.keys(right).filter((k) => right[k] !== undefined); + if (leftKeys.length !== rightKeys.length) return false; + return leftKeys.every((k) => deepEqualAuthored(left[k], right[k], depth + 1)); +} diff --git a/packages/spec/src/stack.zod.ts b/packages/spec/src/stack.zod.ts index aad58042bb..61b16fbc03 100644 --- a/packages/spec/src/stack.zod.ts +++ b/packages/spec/src/stack.zod.ts @@ -10,6 +10,7 @@ import { TranslationBundleSchema, TranslationConfigSchema } from './system/trans import { StackServerConfigSchema } from './system/stack-server.zod'; import { hasPlatformObjectPrefix } from './system/constants/platform-object-names'; import { objectStackErrorMap, formatZodError } from './shared/error-map.zod'; +import { deepEqualAuthored } from './shared/deep-equal'; import { normalizeStackInput, type MetadataCollectionInput, type MapSupportedField } from './shared/metadata-collection.zod'; import type { ConversionNotice } from './conversions/types.js'; import { formatUnknownAuthoringKey } from './data/authoring-key-lint'; @@ -1445,34 +1446,6 @@ const COMPOSE_KEY_DISPOSITIONS: Record COMPOSE_KEY_DISPOSITIONS[key] === 'concat'); -/** - * Structural deep equality for the "same value composes fine" rule (#5005). - * - * Deliberately strict and deliberately small: it decides only whether two - * authored declarations are the SAME, never how to reconcile two different - * ones. Keys explicitly set to `undefined` are treated as absent, matching how - * the composer reads a declaration in the first place. Callables compare by - * reference (`Object.is`) — two distinct closures are two distinct - * declarations, which is the honest answer for a config value. - * @internal - */ -function deepEqualDeclarations(a: unknown, b: unknown): boolean { - if (Object.is(a, b)) return true; - if (typeof a !== 'object' || typeof b !== 'object' || a === null || b === null) return false; - if (Array.isArray(a) !== Array.isArray(b)) return false; - - if (Array.isArray(a) && Array.isArray(b)) { - return a.length === b.length && a.every((item, i) => deepEqualDeclarations(item, b[i])); - } - - const left = a as Record; - const right = b as Record; - const leftKeys = Object.keys(left).filter((k) => left[k] !== undefined); - const rightKeys = Object.keys(right).filter((k) => right[k] !== undefined); - if (leftKeys.length !== rightKeys.length) return false; - return leftKeys.every((k) => deepEqualDeclarations(left[k], right[k])); -} - /** * Name a stack the way its author would recognise it (#5005). * @@ -1512,7 +1485,7 @@ function composeSingleValue( continue; } const held = (stacks[holder] as Record)[key]; - if (deepEqualDeclarations(held, value)) continue; + if (deepEqualAuthored(held, value)) continue; throw new Error( `composeStacks conflict: top-level key '${key}' is declared with different values by ` + From e56af879b5b0eb71d303dbbf68db5804835306cb Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 02:36:35 +0000 Subject: [PATCH 2/2] fix(spec): guard the new equal-value branch against a self-rename, and finish the guidance sweep (#4923) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up on the same ruling. `renameKey(dict, k, k)` would have reached the new equal-value branch, compared the value against itself, called it a redundant twin and deleted the only copy. No registry pair is written that way today (all 40 literal pairs scanned), so this is a guard rather than a fix: one added later now converts nothing instead of erasing data. Pinned by a test on both `renameKey` and `renameConfigKey`. Also completes the batch-9 wording sweep the ruling asked for: the notify guidance test still asserted the old "dead twin" reading, and the `source` prescription said DISAGREES where the rest of the family says DIFFERENT. Both now speak one vocabulary, and the test additionally requires each prescription to say the two values differ — not just that one of them should be deleted. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018fxLGQdatPbBUvCgiVxg6D --- .../spec/src/automation/io-node-config.test.ts | 15 +++++++++------ .../spec/src/automation/io-node-config.zod.ts | 5 +++-- packages/spec/src/conversions/conversions.test.ts | 11 +++++++++++ packages/spec/src/conversions/walk.ts | 5 +++++ 4 files changed, 28 insertions(+), 8 deletions(-) diff --git a/packages/spec/src/automation/io-node-config.test.ts b/packages/spec/src/automation/io-node-config.test.ts index 0c3d81daf5..83b346653a 100644 --- a/packages/spec/src/automation/io-node-config.test.ts +++ b/packages/spec/src/automation/io-node-config.test.ts @@ -64,7 +64,7 @@ describe('NotifyConfigSchema — strict as of #4001 批 9', () => { ['url', '/task/1', '`actionUrl`'], ['source', { object: 'showcase_task', id: '1' }, '`sourceObject` + `sourceId`'], ] as ReadonlyArray<[string, unknown, string]>)( - 'names the canonical key AND the dead-twin case for the retired `%s` alias', + 'names the canonical key AND the disagreeing-pair case for the retired `%s` alias', (key, value, canonical) => { const message = unknownKeyMessage(NotifyConfigSchema, { recipients: ['u1'], title: 'hi', [key]: value, @@ -72,12 +72,15 @@ describe('NotifyConfigSchema — strict as of #4001 批 9', () => { expect(message).toContain(canonical); // Both readings must be served: the ADR-0087 conversion rewrites this // key at load, so a config that still carries it at PARSE time also - // carries the canonical key — `renameConfigKey` leaves a shadowed alias - // in place rather than clobbering the winner. Without this half the - // prescription ("rename it") is wrong for the population that actually - // reaches this error. + // carries the canonical key — and since #4923 it carries one holding a + // DIFFERENT value, because an identical twin is deleted by the + // conversion. Without this half the prescription ("rename it") is wrong + // for the population that actually reaches this error. expect(message).toContain('flow-node-notify-config-aliases'); - expect(message).toMatch(/delete/i); + expect(message).toMatch(/delete|reconcile/i); + // The reconciliation reading has to name the OTHER key too, or the + // author cannot see which two spellings disagree. + expect(message).toMatch(/DIFFERENT|differ/i); }, ); diff --git a/packages/spec/src/automation/io-node-config.zod.ts b/packages/spec/src/automation/io-node-config.zod.ts index 5ea11e9d1c..03dd68e1cd 100644 --- a/packages/spec/src/automation/io-node-config.zod.ts +++ b/packages/spec/src/automation/io-node-config.zod.ts @@ -108,8 +108,9 @@ const NOTIFY_KEY_GUIDANCE: Readonly> = { source: 'The click-through target is the flat PAIR `sourceObject` + `sourceId`, never a nested `source: { object, id }`. ' + '`flow-node-notify-config-aliases` lifts the nested shape at load and drops it once every part is accounted ' - + 'for, so a surviving `source` means a part of it DISAGREES with the flat key already holding that slot, and ' - + 'the conversion declined to pick (#4923) — reconcile onto `sourceObject` / `sourceId` and delete `source`. ' + + 'for, so a surviving `source` means a part of it holds a DIFFERENT value from the flat `sourceObject` / ' + + '`sourceId` already in that slot, and the conversion declined to pick (#4923) — reconcile onto the flat pair ' + + 'and delete `source`. ' + 'Note the pair only takes effect together: a half-specified target is dropped so the inbox never renders a ' + 'dead link.', }; diff --git a/packages/spec/src/conversions/conversions.test.ts b/packages/spec/src/conversions/conversions.test.ts index e0a9aadedb..764b050b54 100644 --- a/packages/spec/src/conversions/conversions.test.ts +++ b/packages/spec/src/conversions/conversions.test.ts @@ -11,6 +11,7 @@ import { PageSchema } from '../ui/page.zod.js'; import { applyConversions, collectConversionNotices } from './apply.js'; import { ALL_CONVERSIONS, CONVERSIONS_BY_MAJOR } from './registry.js'; import { applyConversionsToStoredItem } from './stored.js'; +import { renameConfigKey, renameKey } from './walk.js'; import { CONVERSION_NOTICE_CODE, type ConversionNotice } from './types.js'; describe('conversion layer (ADR-0087 D2)', () => { @@ -252,6 +253,16 @@ describe('conversion layer (ADR-0087 D2)', () => { expect(notices[0]!.from).toBe('object'); }); + it('never deletes the only copy when a pair renames a key to itself', () => { + // Guard on the new equal-value branch: `from === to` would compare the + // value against itself, call it a redundant twin, and delete it. No + // registry pair is written that way; this pins that one added later + // converts nothing instead of erasing the key. + const dict = { objectName: 'task' }; + expect(renameKey(dict, 'objectName', 'objectName')).toBeNull(); + expect(renameConfigKey({ config: { ...dict } }, 'objectName', 'objectName')).toBeNull(); + }); + it('applies the same rule one level up, on a plain dict rename (page header)', () => { // `renameConfigKey` delegates to `renameKey`, so a non-`config` surface // must not develop its own dialect of the rule. diff --git a/packages/spec/src/conversions/walk.ts b/packages/spec/src/conversions/walk.ts index 4b2d58295a..4b74a19f83 100644 --- a/packages/spec/src/conversions/walk.ts +++ b/packages/spec/src/conversions/walk.ts @@ -302,6 +302,11 @@ export function mapCollection( * two surfaces cannot disagree about whether a pair of values is "the same". */ export function renameKey(dict: Dict, from: string, to: string): Dict | null { + // A pair that renames a key to itself has no work to do, and the equal-value + // branch below would otherwise read it as "a twin identical to the canonical + // key" and delete the only copy. No registry pair is written that way today; + // this is here so one added later fails to convert instead of erasing data. + if (from === to) return null; if (!(from in dict) || dict[from] == null) return null; const next: Dict = { ...dict }; if (dict[to] != null) {