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.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 e68fe4ce7e..03dd68e1cd 100644 --- a/packages/spec/src/automation/io-node-config.zod.ts +++ b/packages/spec/src/automation/io-node-config.zod.ts @@ -74,34 +74,45 @@ 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 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.', }; // ─── 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..764b050b54 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'; @@ -10,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)', () => { @@ -136,7 +138,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 +155,156 @@ 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('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. + 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 +597,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 +849,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..4b74a19f83 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,71 @@ 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 { + // 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; - 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 ` +