diff --git a/.changeset/compose-stacks-action-echo.md b/.changeset/compose-stacks-action-echo.md new file mode 100644 index 0000000000..97e4d2db5a --- /dev/null +++ b/.changeset/compose-stacks-action-echo.md @@ -0,0 +1,44 @@ +--- +"@objectstack/spec": patch +--- + +fix(spec): `composeStacks` carries each bound standalone action once in the composed object — the second merge no longer re-appends the copy each input's build already made (#14847) + +`defineStack` ends with `mergeActionsIntoObjects`: every standalone action +carrying `objectName` is copied into that object's `actions` on the way out, +and the standalone stays in `stack.actions`. `composeStacks` concatenates its +inputs' `actions` and ended with the same merge — so each bound action was +appended to its object a SECOND time, beside the copy the input's own build had +put there: two copies for one declaration, three with three stacks under +`objectConflict: 'override'` / `'merge'`, `manifest: 'preserve'` inheriting it. +Downstream (the runtime note recorded on #14686): `collectActionDeclarations` +pushes every embedded entry, so MCP `listActions` listed a composed app's bound +actions twice and bare-name `resolveActionByName` refused the ambiguity; `os +build` shipped the doubled entries to `dist/objectstack.json` unremarked, +because `compile.ts` validates the lowered stack with `safeParse`, not +`defineStack`. + +`mergeActionsIntoObjects` is now idempotent over its own output: a bound action +the object already carries BY IDENTITY is not appended again. Identity against +the standalone list, deliberately not equality — the only way an entry of +`stack.actions` is the very same object as an entry of `object.actions` is that +a previous merge put it there. A hand-written twin (one action authored in both +positions) is two objects after the strict parse and is still refused by +#14686's same-key rule, which runs before the merge and is untouched; a marker +cannot do this job (`ActionSchema` is strict — an unknown key is refused). +`collectComposedActionKeyCollisions` (#14854) is untouched too: it runs before +the merge and counts distinct stacks per key, so its refusals and their wording +do not move. + +Shape change, and only this: a composed object's `actions` is the previous +output minus the duplicate entries — nothing is reordered, and `order` sorts the +once-merged set as before. The accept set does not move: nothing that composed +before is refused now, and nothing refused is accepted. Round trip: a composed +artifact is still refused by `defineStack` when an input binds an action — a +built artifact carries each bound action in both positions by #14686's landed +design (author the source shape, not the artifact) — but with the same +"declared twice" line a single built input gets, no longer "3 times"; with no +bound action it parses cleanly, as before. One measured consequence outside +composition: under `strict: false`, ONE action object placed in both positions +is now carried once (before: twice) — a single declaration, in the mode that +opts out of #14686's walk by choice. diff --git a/packages/spec/src/compose-stacks-action-echo.test.ts b/packages/spec/src/compose-stacks-action-echo.test.ts new file mode 100644 index 0000000000..0dfba800a2 --- /dev/null +++ b/packages/spec/src/compose-stacks-action-echo.test.ts @@ -0,0 +1,214 @@ +/** + * `composeStacks` carries each bound standalone action ONCE in the composed + * object — the second `mergeActionsIntoObjects` no longer re-appends the copy + * each input's own build already put there (#14847). + * + * `defineStack` ends with `mergeActionsIntoObjects`: every standalone action + * carrying `objectName` is copied into that object's `actions` on the way out, + * and the standalone stays in `stack.actions`. `composeStacks` concatenates its + * inputs' `actions` and ends with the same merge, so before this change every + * bound action was appended to its object a SECOND time beside the copy the + * input's build had already made. Measured on `main` @ `6392b9c2`, `defineStack` + * outputs as inputs (the shape `examples/app-multi-package` composes): + * + * ``` + * two stacks, default : a_item.actions=["dup_x/BOUND","dup_x/BOUND"] two copies, one declaration + * … with manifest: 'preserve' : same on `objects`; packages[].manifest.objects carry ONE each + * three stacks binding b1/b2/b3 to one object, override / merge + * : shared.actions=["emb3/EMB","b3/BOUND","b1/BOUND","b2/BOUND","b3/BOUND"] + * defineStack(composeStacks([a, b])) : REFUSED 'a_item:dup_x' is declared 3 times + * defineStack(a) (a lone built input) : REFUSED 'a_item:dup_x' is declared twice ← #14686's landed pin + * ``` + * + * The discriminator is IDENTITY against the standalone list, not equality + * (the triage ruling on the card): the only way an entry of `stack.actions` is + * the very same object as an entry of `object.actions` is that a previous merge + * put it there. A strict parse produces fresh objects, so a hand-written twin — + * the same action authored in both positions — never shares identity, and + * #14686's same-key refusal, which runs before the merge, still refuses it. A + * marker key cannot do this job: `ActionSchema` is a strict object, so the key + * is refused before it could travel (measured below). + * + * What does NOT change: `defineStack`'s refusal of a built stack fed back in + * (`stack-duplicate-action-key.test.ts` pins it — a built artifact carries each + * bound action in both positions by design; author the source shape, not the + * artifact). So the composed output round-trips through `defineStack` exactly + * as far as any single built input does: cleanly when no input binds an action, + * otherwise refused with the SAME "declared twice" line a lone build gets — no + * longer "3 times". `collectComposedActionKeyCollisions` (#14854) and its + * message are untouched: it runs before the merge and counts distinct stacks + * per key. + */ +import { describe, it, expect } from 'vitest'; +import { composeStacks, defineStack, type ObjectStackDefinition } from './stack.zod'; + +const mf = (id: string) => ({ id, name: id.split('.').pop()!, version: '1.0.0', type: 'app' as const }); + +const act = (name: string, extra: Record = {}) => + ({ name, label: name, type: 'script' as const, target: 'noop', ...extra }); + +// `as const` on the field type is load-bearing (see stack.test.ts): hoisted +// without it the literal widens to `string`, which the input type refuses. +const obj = (name: string, actions?: ReturnType[]) => ({ + name, + label: name, + fields: { title: { type: 'text' as const } }, + ...(actions ? { actions } : {}), +}); + +/** The runtime keys a stack would register, per position. */ +const keysOf = (s: ObjectStackDefinition) => ({ + top: (s.actions ?? []).map((a) => `${a.objectName ?? 'global'}:${a.name}`), + embedded: Object.fromEntries( + (s.objects ?? []).map((o) => [o.name, (o.actions ?? []).map((a) => `${a.name}/${a.objectName ? 'BOUND' : 'EMB'}`)]), + ), +}); + +/** The thrown message, or `null` when the call is accepted. */ +function refusal(fn: () => unknown): string | null { + try { + fn(); + return null; + } catch (e) { + return (e as Error).message; + } +} + +const boundA = () => + defineStack({ manifest: mf('com.example.a'), objects: [obj('a_item')], actions: [act('dup_x', { objectName: 'a_item' })] }); +const boundB = () => + defineStack({ manifest: mf('com.example.b'), objects: [obj('b_item')], actions: [act('dup_y', { objectName: 'b_item' })] }); + +describe('composeStacks - a bound standalone action appears once in the composed object (#14847)', () => { + it("carries each input's own bound action once under the default strategy, and the standalone once at the top", () => { + const a = boundA(); + const b = boundB(); + // Each input's build already carries the echo — one copy, the very same object. + expect(keysOf(a).embedded).toEqual({ a_item: ['dup_x/BOUND'] }); + expect(a.objects![0].actions![0]).toBe(a.actions![0]); + + const out = composeStacks([a, b]); + expect(keysOf(out)).toEqual({ + top: ['a_item:dup_x', 'b_item:dup_y'], + embedded: { a_item: ['dup_x/BOUND'], b_item: ['dup_y/BOUND'] }, + }); + // The carried copy IS the input's declaration, not a re-appended clone. + const aItem = out.objects!.find((o) => o.name === 'a_item')!; + expect(aItem.actions![0]).toBe(a.actions![0]); + }); + + it("carries it once under manifest: 'preserve' too, and the packages[] halves agree with the composed objects", () => { + const out = composeStacks([boundA(), boundB()], { manifest: 'preserve' }); + expect(keysOf(out).embedded).toEqual({ a_item: ['dup_x/BOUND'], b_item: ['dup_y/BOUND'] }); + const perPackage = (out.packages ?? []).map((p) => + ((p.manifest as { objects?: ObjectStackDefinition['objects'] }).objects ?? []).map((o) => [o.name, (o.actions ?? []).length]), + ); + expect(perPackage).toEqual([[['a_item', 1]], [['b_item', 1]]]); + }); + + // Three stacks each declaring `shared`, each embedding one action on it and + // binding one standalone to it — distinct names, so no key collides and the + // object strategy alone decides what survives. + const s = (n: 1 | 2 | 3) => + defineStack({ + manifest: mf(`com.example.s${n}`), + objects: [obj('shared', [act(`emb${n}`)])], + actions: [act(`b${n}`, { objectName: 'shared' })], + }); + + it.each(['override', 'merge'] as const)( + "with three stacks under objectConflict: '%s', the surviving object carries the surviving stack's declared actions plus each concatenated standalone once", + (objectConflict) => { + const out = composeStacks([s(1), s(2), s(3)], { objectConflict }); + expect(keysOf(out).top).toEqual(['shared:b1', 'shared:b2', 'shared:b3']); + // s3's object survives with its built array as-is (embedded `emb3`, then + // the echo of its own `b3`); s1's and s2's bound actions join it once + // each, in concatenation order. The lost `emb1` / `emb2` are the object + // strategy's own semantics, not this merge's. Before: `b3/BOUND` twice. + expect(keysOf(out).embedded).toEqual({ shared: ['emb3/EMB', 'b3/BOUND', 'b1/BOUND', 'b2/BOUND'] }); + }, + ); + + it("still binds another stack's standalone to an object it does not own — the add-on shape — once", () => { + const core = defineStack({ manifest: mf('com.example.core'), objects: [obj('core_item', [act('archive')])] }); + const addon = defineStack({ manifest: mf('com.example.addon'), actions: [act('approve', { objectName: 'core_item' })] }, { strict: false }); + const out = composeStacks([core, addon]); + expect(keysOf(out).embedded).toEqual({ core_item: ['archive/EMB', 'approve/BOUND'] }); + }); + + it('still honours `order` across the once-merged set', () => { + const base = defineStack({ + manifest: mf('com.example.base'), + objects: [obj('deal', [act('inline', { order: 0 })])], + actions: [act('own_bound', { objectName: 'deal' })], + }); + const approvals = defineStack({ manifest: mf('com.example.approvals'), actions: [act('approve', { objectName: 'deal', order: -100 })] }, { strict: false }); + const out = composeStacks([base, approvals]); + // Before: `own_bound/BOUND` twice, the stable sort keeping both. + expect(keysOf(out).embedded).toEqual({ deal: ['approve/BOUND', 'inline/EMB', 'own_bound/BOUND'] }); + }); +}); + +describe('composeStacks - what the identity skip does NOT fold', () => { + it("refuses a hand-written twin — the same action authored standalone AND on its object — through #14686's rule, unchanged", () => { + const msg = refusal(() => + defineStack({ + manifest: mf('com.example.t'), + objects: [obj('t_item', [act('tw', { objectName: 't_item' })])], + actions: [act('tw', { objectName: 't_item' })], + }), + ); + expect(msg).toContain('defineStack cross-reference validation failed (1 issue):'); + expect(msg).toContain( + " ✗ Action key 't_item:tw' is declared twice: stack.actions[0] (objectName 't_item') and " + + "objects['t_item'].actions[0] (embedded on the object). ", + ); + }); + + it('cannot be done by marking: a marker key on the standalone is refused by the strict parse before it could travel', () => { + const msg = refusal(() => + defineStack({ manifest: mf('com.example.m'), objects: [obj('m_item')], actions: [act('mk', { objectName: 'm_item', __echo: true })] }), + ); + expect(msg).toContain('actions.0: Unrecognized key(s) on this action: `__echo`.'); + }); + + it('under strict: false, ONE action object placed in both positions is one declaration — carried once (before: twice)', () => { + // No parse clones it, so it shares identity exactly as a build's echo does. + // #14686's walk does not run in this mode by the author's choice, and the + // runtime dedupes a standalone against an embedded entry by key anyway. + const shared = act('sh', { objectName: 'n_item' }); + const out = defineStack({ manifest: mf('com.example.n'), objects: [obj('n_item', [shared])], actions: [shared] }, { strict: false }); + expect(keysOf(out)).toEqual({ top: ['n_item:sh'], embedded: { n_item: ['sh/BOUND'] } }); + }); + + it('is idempotent: re-merging a built stack (strict: false) leaves the object untouched, by reference', () => { + const built = boundA(); + const again = defineStack(built, { strict: false }); + expect(keysOf(again).embedded).toEqual({ a_item: ['dup_x/BOUND'] }); + expect(again.objects![0]).toBe(built.objects![0]); + }); +}); + +describe('composeStacks - round trip through defineStack', () => { + it('parses cleanly when no input binds a standalone action to an object — no echo exists to double', () => { + const g1 = defineStack({ manifest: mf('com.example.g1'), objects: [obj('g_item', [act('e1')])], actions: [act('glob1')] }); + const g2 = defineStack({ manifest: mf('com.example.g2'), objects: [obj('h_item', [act('e2')])], actions: [act('glob2')] }); + const out = composeStacks([g1, g2]); + expect(refusal(() => defineStack(out))).toBeNull(); + expect(keysOf(defineStack(out))).toEqual(keysOf(out)); + }); + + it("with bound actions, is refused exactly as far as a lone built input is — 'declared twice' by #14686's landed pin, never '3 times'", () => { + const a = boundA(); + const LINE = + " ✗ Action key 'a_item:dup_x' is declared twice: stack.actions[0] (objectName 'a_item') and " + + "objects['a_item'].actions[0] (embedded on the object). "; + expect(refusal(() => defineStack(a))).toContain(LINE); + + const composed = refusal(() => defineStack(composeStacks([a, boundB()]))); + expect(composed).toContain('defineStack cross-reference validation failed (2 issues):'); + expect(composed).toContain(LINE); + expect(composed).toContain(" ✗ Action key 'b_item:dup_y' is declared twice: "); + expect(composed).not.toContain('is declared 3 times'); + }); +}); diff --git a/packages/spec/src/compose-stacks-action-key-collision.test.ts b/packages/spec/src/compose-stacks-action-key-collision.test.ts index 04ba926d1f..e39d66d85b 100644 --- a/packages/spec/src/compose-stacks-action-key-collision.test.ts +++ b/packages/spec/src/compose-stacks-action-key-collision.test.ts @@ -308,10 +308,8 @@ describe('composeStacks - what stays accepted', () => { const out = composeStacks([orders, core], { manifest: 'preserve' }); expect(out.packages).toHaveLength(2); expect(keysOf(out).top).toEqual(['crm_order:ship_order']); - // #14847: the bound action appears twice on `crm_order` today — the measured - // shape, not the contract — so the pin asks only that it is carried, and - // that the other package's embedded action is carried exactly once. - expect(keysOf(out).embedded.crm_order).toContain('ship_order/BOUND'); - expect(keysOf(out).embedded.crm_account).toEqual(['archive_account/EMB']); + // Each package's action is carried exactly once — the bound one no longer + // doubled by the composed merge (#14847), the embedded one as before. + expect(keysOf(out).embedded).toEqual({ crm_order: ['ship_order/BOUND'], crm_account: ['archive_account/EMB'] }); }); }); diff --git a/packages/spec/src/stack.zod.ts b/packages/spec/src/stack.zod.ts index 311458ea92..8c27145e45 100644 --- a/packages/spec/src/stack.zod.ts +++ b/packages/spec/src/stack.zod.ts @@ -2064,6 +2064,17 @@ function sortActionsByOrder(actions: T[]): T[] { * top-level `actions` array is preserved for global access (e.g., platform * overview, search). * + * Idempotent over its own output (#14847): a bound action the object already + * carries BY IDENTITY is not appended again. That is the shape `composeStacks` + * feeds it — inputs built by `defineStack`, each object already carrying the + * copy its own build appended while the standalone still sits in `actions` — + * and before this the second merge doubled every bound action in the composed + * object (three copies for two declarations under `objectConflict: 'override'` + * / `'merge'`). Identity, deliberately not equality: an author writing one + * action in both positions produces two distinct objects, which #14686's + * same-key refusal (run before this merge) rejects and which this merge must + * not quietly fold. + * * After merging, every action group (each object's `actions` and the top-level * `actions`) is stable-sorted by `order` via {@link sortActionsByOrder}. Because * that sort is a no-op unless an author sets `order`, this is fully backward @@ -2102,12 +2113,23 @@ function mergeActionsIntoObjects(config: ObjectStackDefinition): ObjectStackDefi // references, consistent with mergeObjects() and Zod output). let objectsChanged = false; const newObjects = config.objects.map((obj) => { - const objActions = actionsByObject.get(obj.name); const base = obj.actions ?? []; - const merged = objActions ? [...base, ...objActions] : base; + // Idempotent (#14847): append only the bound actions `base` does not carry + // ALREADY — judged by identity, never by equality. The one way an entry of + // `config.actions` can be the very same object as an entry of `obj.actions` + // is a previous run of this merge having put it there: `defineStack` ends + // here, so a BUILT stack carries each bound action in both positions, and + // `composeStacks` — which concatenates its inputs' `actions` and hands the + // surviving objects through as-is — ran this merge a second time over that + // echo and doubled every bound action. A hand-written twin (one action + // authored in both positions) is two objects after the strict parse, and + // #14686's same-key refusal has already run ahead of this merge to refuse + // it; an equality skip here would have swallowed it instead. + const fresh = (actionsByObject.get(obj.name) ?? []).filter((action) => !base.includes(action)); + const merged = fresh.length > 0 ? [...base, ...fresh] : base; const sorted = sortActionsByOrder(merged); - // Untouched: no top-level actions merged in AND the sort was a no-op. - if (!objActions && sorted === base) return obj; + // Untouched: nothing new merged in AND the sort was a no-op. + if (sorted === base) return obj; objectsChanged = true; return { ...obj, actions: sorted }; }); @@ -3148,5 +3170,10 @@ export function composeStacks( throw new Error(formatComposedActionKeyCollisions(actionCollisions)); } + // 7. Bind every standalone action to its object — ONCE. Each input built by + // `defineStack` already carries its own bound actions on its objects (the + // echo step 6 steps around), and the surviving objects reach here as-is, + // so the merge appends only what an object does not already carry by + // identity (#14847): the other inputs' actions bound to it. return mergeActionsIntoObjects(composed as ObjectStackDefinition); }