diff --git a/.changeset/action-execute-target-precedence.md b/.changeset/action-execute-target-precedence.md new file mode 100644 index 0000000000..eecab5a77b --- /dev/null +++ b/.changeset/action-execute-target-precedence.md @@ -0,0 +1,49 @@ +--- +"@objectstack/spec": minor +"@objectstack/cli": patch +--- + +fix(action): one precedence for `target` vs the deprecated `execute` — lower the alias, then drop it (#3713) + +`execute` is the deprecated alias of `target`, and three readers resolved "the +author declared both" in **two opposite directions**: + +| Reader | Preferred | +|---|---| +| `ActionSchema` transform (spec) | `target` | +| objectui `ActionRunner.executeScript` | `execute` | +| CLI compile step (`lowerCallables`) | `execute` | + +So `defineAction({ type: 'script', target: 'preferredHandler', execute: 'legacyHandler' })` +ran `preferredHandler` server-side and `legacyHandler` client-side — two +different scripts for one button, silently, with no error anywhere. Low +frequency (it needs an author to set both, which happens mid-migration or by +copy-paste), but the failure mode is "the wrong code ran". + +**`target` now wins everywhere, and the alias is removed from the parsed +output** — the same "canonical wins, alias disappears" shape as +`agent.knowledge.topics` → `sources`. The conflict is now *unrepresentable* +rather than merely agreed-upon: no renderer can see a second slot to disagree +about. Worth noting the server runtime never read `execute` at all +(`isHeadlessInvokableAction` gates on `target || body`; dispatch probes +`target`/`name`), so authoring `execute` worked *solely* because it was lowered +at parse time — dropping it costs the server nothing. + +The CLI's inline-handler lowering had the same bug in compile-time form: with a +function in both slots it bundled the `execute` one and then overwrote +`action.target` with that ref, silently discarding the function the author +declared on `target`. It now probes `target` first and drops the alias. + +**Authoring is unchanged** — `execute` is still accepted on input (`ActionInput`), +still lowered to `target`, and still listed in the reference docs. Nothing to +migrate in your app metadata. + +**Consumers of the parsed metadata**, however, must read the canonical slot: + +- FROM: `parsedAction.execute` → TO: `parsedAction.target` +- One-line fix: delete the alias fallback, e.g. `action.execute || action.target` + becomes `action.target`. + +`z.infer` no longer carries `execute`, so any such reader +fails to compile rather than silently reading `undefined`. The objectui +`ActionRunner` counterpart ships separately. diff --git a/content/docs/protocol/objectui/actions.mdx b/content/docs/protocol/objectui/actions.mdx index b250440efa..4409d5eca0 100644 --- a/content/docs/protocol/objectui/actions.mdx +++ b/content/docs/protocol/objectui/actions.mdx @@ -51,7 +51,7 @@ The `type` field selects how an action is dispatched. The complete enum is **`sc | `api` | Call an API endpoint (`method` defaults to `POST`). | Yes | | `form` | Open a FormView by name, routed to `/console/forms/:name`. | Yes | -`target` is the canonical binding for every non-`script` type. The deprecated `execute` field is auto-migrated to `target` during parsing. +`target` is the canonical binding for every non-`script` type. The deprecated `execute` field is lowered into `target` during parsing and then **removed from the parsed metadata**, so every consumer — server dispatch and renderer alike — reads exactly one slot. If an action declares both, `target` wins and `execute` is discarded. ### Script Actions diff --git a/content/docs/references/ui/action.mdx b/content/docs/references/ui/action.mdx index d0e58c94ba..b6cc865511 100644 --- a/content/docs/references/ui/action.mdx +++ b/content/docs/references/ui/action.mdx @@ -87,7 +87,7 @@ const result = Action.parse(data); | **target** | `string` | optional | URL, Script Name, Flow ID, or API Endpoint. Supports $`{param.X}` and $`{ctx.X}` interpolation. | | **openIn** | `Enum<'self' \| 'new-tab'>` | optional | For type:'url' — where to open `target`. 'new-tab' opens a new browser tab; 'self' navigates in place. When omitted, external/absolute URLs open in a new tab and relative URLs navigate in place. Static execution option — keep it OUT of `params` (which is user-input-collection only). | | **body** | `{ language: 'expression'; source: string } \| { language: 'js'; source: string; capabilities?: Enum<'api.read' \| 'api.write' \| 'api.transaction' \| 'crypto.uuid' \| 'crypto.hash' \| 'log'>[]; timeoutMs?: integer; … }` | optional | Action body — expression (L1) or sandboxed JS (L2). Only used when type is `script`. | -| **execute** | `string` | optional | @deprecated — Use target instead. Auto-migrated to target during parsing. | +| **execute** | `string` | optional | @deprecated — Use target instead. Lowered into target during parsing and dropped from the parsed output; when both are set, target wins. | | **params** | `{ name?: string; field?: string; objectOverride?: string; label?: string; … }[]` | optional | Input parameters required from user | | **variant** | `Enum<'primary' \| 'secondary' \| 'danger' \| 'ghost' \| 'link'>` | optional | Button visual variant for styling (primary = highlighted, danger = destructive, ghost = transparent) | | **order** | `number` | optional | Sort order within a location group (lower = higher). Promotes/demotes an action toward the record_header primary button; stable, so actions without `order` keep their registration order. | diff --git a/packages/cli/src/utils/lower-callables.test.ts b/packages/cli/src/utils/lower-callables.test.ts new file mode 100644 index 0000000000..76577f6b67 --- /dev/null +++ b/packages/cli/src/utils/lower-callables.test.ts @@ -0,0 +1,115 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { lowerCallables } from './lower-callables.js'; + +// ── #3713: `target` vs the deprecated `execute` alias — one precedence, one slot ── +// +// `execute` is the deprecated alias of `target`. Before this was aligned, three +// readers resolved "the author declared both" in two directions: the spec +// transform kept `target`, objectui's ActionRunner did `execute || target`, and +// THIS compile step preferred `execute`. The compile-step half was the nastiest: +// it bundled the `execute` function and then overwrote `action.target` with the +// resulting ref, so the function the author actually declared on `target` was +// silently dropped from the build. +describe('lowerCallables — action handler slot precedence (#3713)', () => { + const actionsOf = (result: { lowered: Record }) => + (result.lowered as { actions: Array> }).actions; + + it('binds the canonical `target` function when both slots carry a callable', () => { + const result = lowerCallables({ + actions: [{ + name: 'convert', + label: 'Convert', + type: 'script', + target: function preferredHandler() { return 'preferred'; }, + execute: function legacyHandler() { return 'legacy'; }, + }], + }); + + const [action] = actionsOf(result); + const ref = action.target as string; + expect(typeof ref).toBe('string'); + // The bundled function must be the one from `target`, not from `execute`. + expect(result.functions[ref]()).toBe('preferred'); + expect(Object.values(result.functions).map((fn) => fn())).not.toContain('legacy'); + }); + + it('drops the `execute` alias once a ref is bound to `target`', () => { + const result = lowerCallables({ + actions: [{ + name: 'convert', + label: 'Convert', + type: 'script', + target: function preferredHandler() { return 'preferred'; }, + execute: function legacyHandler() { return 'legacy'; }, + }], + }); + + const [action] = actionsOf(result); + // A leftover alias is stale by construction — and a function-valued one + // would fail `ActionSchema` (it expects a string) further down the pipeline. + expect('execute' in action).toBe(false); + // The lowered stack must be JSON-safe: no function values survive. + expect(() => JSON.stringify(result.lowered)).not.toThrow(); + expect(JSON.stringify(result.lowered)).not.toContain('legacy'); + }); + + it('still lowers an `execute`-only callable (back-compat) onto `target`', () => { + const result = lowerCallables({ + actions: [{ + name: 'legacy_only', + label: 'Legacy', + type: 'script', + execute: function legacyHandler() { return 'legacy'; }, + }], + }); + + const [action] = actionsOf(result); + const ref = action.target as string; + expect(typeof ref).toBe('string'); + expect(result.functions[ref]()).toBe('legacy'); + expect('execute' in action).toBe(false); + }); + + it('leaves a string-valued handler pair untouched (the spec transform owns it)', () => { + // No callable to lower here, so the compile step must not editorialise: the + // spec's `ActionSchema` transform is the single place that resolves a + // string/string pair (keeping `target`, dropping `execute`). + const result = lowerCallables({ + actions: [{ + name: 'strings', + label: 'Strings', + type: 'script', + target: 'preferredHandler', + execute: 'legacyHandler', + }], + }); + + const [action] = actionsOf(result); + expect(action.target).toBe('preferredHandler'); + expect(action.execute).toBe('legacyHandler'); + expect(result.count).toBe(0); + }); + + it('applies the same precedence to actions nested under an object', () => { + const result = lowerCallables({ + objects: [{ + name: 'crm_deal', + actions: [{ + name: 'convert', + label: 'Convert', + type: 'script', + target: function preferredHandler() { return 'preferred'; }, + execute: function legacyHandler() { return 'legacy'; }, + }], + }], + }); + + const objects = (result.lowered as { objects: Array<{ actions: Array> }> }).objects; + const [action] = objects[0].actions; + const ref = action.target as string; + expect(result.functions[ref]()).toBe('preferred'); + expect('execute' in action).toBe(false); + }); +}); diff --git a/packages/cli/src/utils/lower-callables.ts b/packages/cli/src/utils/lower-callables.ts index 59f91250fb..791ee391ee 100644 --- a/packages/cli/src/utils/lower-callables.ts +++ b/packages/cli/src/utils/lower-callables.ts @@ -105,7 +105,8 @@ export function lowerCallables(input: Record): LoweringResult { } // 1b. Lower inline action handlers found inside `objects[*].actions[*]` - // and `actions[*]`. We accept either `execute: fn` or `target: fn`. + // and `actions[*]`. We accept either `target: fn` or the deprecated + // `execute: fn`; `target` wins when both are present (#3713). if (Array.isArray(lowered.objects)) { lowered.objects = (lowered.objects as unknown[]).map((rawObj) => { if (!isPlainObject(rawObj)) return rawObj; @@ -173,9 +174,10 @@ export function lowerCallables(input: Record): LoweringResult { } /** - * Lower a single action definition: detect callable on `execute` or - * `target`, register it, optionally extract a metadata body. Mutates a - * shallow clone, never the input. + * Lower a single action definition: detect a callable on `target` or on the + * deprecated `execute` alias (canonical `target` first — #3713), register it, + * optionally extract a metadata body, and drop the alias. Mutates a shallow + * clone, never the input. */ function lowerActionCallable( raw: unknown, @@ -189,11 +191,17 @@ function lowerActionCallable( const baseName = typeof action.name === 'string' && action.name.length > 0 ? `${ownerLabel}_${action.name}` : `${ownerLabel}_anon_action`; + // #3713: `target` is the canonical slot and `execute` its deprecated alias, so + // probe `target` FIRST. This used to prefer `execute`, which made the compile + // step the third reader of this pair — and it disagreed with the spec transform + // (which keeps `target` when both are set). An author who inlined a function in + // both slots got the `execute` one bundled while `action.target = ref` below + // silently overwrote the `target` function they had actually declared. const handlerSlot: 'execute' | 'target' | null = - typeof action.execute === 'function' - ? 'execute' - : typeof action.target === 'function' - ? 'target' + typeof action.target === 'function' + ? 'target' + : typeof action.execute === 'function' + ? 'execute' : null; if (!handlerSlot) return action; const fn = action[handlerSlot] as AnyFn; @@ -206,6 +214,10 @@ function lowerActionCallable( } // Keep a string-named target so the legacy executor can still resolve it. action.target = ref; - if (handlerSlot === 'execute') delete action.execute; + // Drop the alias unconditionally, matching the spec transform's "canonical wins, + // alias disappears" rule (#3713). Once `target` carries the bundled ref, any + // leftover `execute` is stale by construction — and a function-valued one would + // fail `ActionSchema` (it expects a string) further down the pipeline. + if ('execute' in action) delete action.execute; return action; } diff --git a/packages/spec/liveness/README.md b/packages/spec/liveness/README.md index f74680b7b4..b5ed6b90f2 100644 --- a/packages/spec/liveness/README.md +++ b/packages/spec/liveness/README.md @@ -66,7 +66,7 @@ as `live`, 10 were wrong** — a 77% error rate for the preview-renderer standar | Verdict | Properties | |---|---| -| `live`, evidence corrected to the real reader | `action.execute` (ActionRunner + the spec transform), `action.disabled` (six render surfaces), `flow.status` (engine gates binding + execution since `497bda853`) | +| `live`, evidence corrected to the real reader | `action.execute` (the spec transform's parse-time lowering — the second reader, objectui's ActionRunner, resolved the `target`/`execute` pair in the *opposite* direction; aligned and the alias dropped from the parsed output in #3713), `action.disabled` (six render surfaces), `flow.status` (engine gates binding + execution since `497bda853`) | | corrected to `dead` + `authorWarn` | `action.shortcut`, `action.bulkEnabled`, `flow.active`, `skill.triggerPhrases`, `tool.category`, `tool.requiresConfirmation`, `tool.active`, `tool.builtIn`, `skill.permissions`*, `agent.knowledge` | \* `skill.permissions` was subsequently pruned outright — it was never enforced. diff --git a/packages/spec/liveness/action.json b/packages/spec/liveness/action.json index 5d8b1508de..bcb86abbe5 100644 --- a/packages/spec/liveness/action.json +++ b/packages/spec/liveness/action.json @@ -44,8 +44,8 @@ }, "execute": { "status": "live", - "evidence": "objectui packages/core/src/actions/ActionRunner.ts:704 (executeScript reads action.execute) + framework packages/spec/src/ui/action.zod.ts:577-580 (.transform lowers execute -> target)", - "note": "RE-VERIFIED 2026-07 (#3686 preview-claim sweep): the prior `live` verdict cited only a metadata-admin PREVIEW panel, which echoes what the author typed. Verdict stands but for a different reason: TWO independent runtime readers exist. NOTE a live divergence — the spec transform keeps `target` when both are set (action.test.ts:1032), while ActionRunner does `execute || target`, so an action declaring both runs different code client- vs server-side." + "evidence": "packages/spec/src/ui/action.zod.ts:581 — the .transform lowers execute -> target and DROPS the alias, so authoring it changes what the runtime dispatches; `target` is the one slot every consumer reads", + "note": "RE-VERIFIED 2026-07 (#3686 preview-claim sweep): the prior `live` verdict cited only a metadata-admin PREVIEW panel, which echoes what the author typed. Verdict stands, but the real reader is the parse-time lowering, not a second field reader. DIVERGENCE RESOLVED in #3713: the alias is now consumed at parse time and removed from the output, so the 'both declared' conflict is unrepresentable rather than merely agreed-upon (mirrors agent.knowledge.topics -> sources, #1891). Before that fix three readers disagreed in two directions — this transform kept `target`, objectui ActionRunner did `execute || target`, and the CLI compile step (packages/cli/src/utils/lower-callables.ts) preferred a function on `execute`; all now prefer `target`. Note the server runtime never read `execute` at all (runtime/src/action-execution.ts gates on `target || body` and dispatches on `target`/`name`), which is why lowering is the whole of this property's liveness." }, "params": { "status": "live", diff --git a/packages/spec/src/ui/action.test.ts b/packages/spec/src/ui/action.test.ts index 329ac3d502..935de63ee3 100644 --- a/packages/spec/src/ui/action.test.ts +++ b/packages/spec/src/ui/action.test.ts @@ -1027,6 +1027,8 @@ describe('ActionSchema - execute → target migration', () => { execute: 'legacyHandler', }); expect(result.target).toBe('legacyHandler'); + // #3713: the alias is consumed, not carried alongside the canonical slot. + expect('execute' in result).toBe(false); }); it('should preserve target over execute when both are set', () => { @@ -1040,6 +1042,35 @@ describe('ActionSchema - execute → target migration', () => { expect(result.target).toBe('preferredHandler'); }); + it('should DROP execute from the parsed output so no consumer can disagree (#3713)', () => { + // The divergence this pins: the spec kept `target` when both were set, while + // objectui's ActionRunner did `execute || target` — so one button ran + // `preferredHandler` server-side and `legacyHandler` client-side, silently. + // Lowering the alias and removing it makes the conflict *unrepresentable* + // rather than merely agreed-upon — same shape as `agent.knowledge.topics` + // → `sources` (#1891), which asserts `'topics' in parsed === false`. + const both = ActionSchema.parse({ + name: 'both_fields', + label: 'Both', + type: 'script', + target: 'preferredHandler', + execute: 'legacyHandler', + }); + expect('execute' in both).toBe(false); + expect(Object.keys(both)).not.toContain('execute'); + expect(JSON.parse(JSON.stringify(both)).execute).toBeUndefined(); + + // Authors may still WRITE `execute` — only the parsed output is canonical. + const aliasOnly = ActionSchema.parse({ + name: 'alias_only', + label: 'Alias', + type: 'url', + execute: 'https://example.com/report', + }); + expect(aliasOnly.target).toBe('https://example.com/report'); + expect('execute' in aliasOnly).toBe(false); + }); + it('should reject a script with neither target/execute nor body', () => { // #2169: a script action with no handler binding registers nothing. expect(() => ActionSchema.parse({ diff --git a/packages/spec/src/ui/action.zod.ts b/packages/spec/src/ui/action.zod.ts index 6b2b298398..0f7c91e9a8 100644 --- a/packages/spec/src/ui/action.zod.ts +++ b/packages/spec/src/ui/action.zod.ts @@ -169,7 +169,9 @@ const TARGET_REQUIRED_TYPES: ReadonlySet = new Set( * - `type: 'form'` — `target` is **required** (the FormView name to open, routed to `/console/forms/:name`). * * The `execute` field is **deprecated** and will be removed in a future version. - * If `execute` is provided without `target`, it is automatically migrated to `target`. + * If `execute` is provided without `target`, it is lowered into `target` at parse + * time. Either way `execute` is **dropped from the parsed output** (#3713), so + * every consumer reads one canonical slot; when both are declared, `target` wins. * * @example Good action names * - 'on_close_deal' @@ -383,10 +385,13 @@ export const ActionSchema = lazySchema(() => z.object({ */ body: HookBodySchema.optional().describe('Action body — expression (L1) or sandboxed JS (L2). Only used when type is `script`.'), - /** - * @deprecated Use `target` instead. This field is auto-migrated to `target` during parsing. + /** + * @deprecated Use `target` instead. Accepted on input, lowered into `target` + * during parsing, then **removed from the parsed output** (#3713) — so a + * renderer only ever sees the canonical `target`. When both are set, `target` + * wins and this value is discarded. */ - execute: z.string().optional().describe('@deprecated — Use target instead. Auto-migrated to target during parsing.'), + execute: z.string().optional().describe('@deprecated — Use target instead. Lowered into target during parsing and dropped from the parsed output; when both are set, target wins.'), /** User Input Requirements */ params: z.array(ActionParamSchema).optional().describe('Input parameters required from user'), @@ -574,11 +579,28 @@ export const ActionSchema = lazySchema(() => z.object({ /** ARIA accessibility attributes */ aria: AriaPropsSchema.optional().describe('ARIA accessibility attributes'), }).transform((data) => { - // Auto-migrate deprecated `execute` → `target` for backward compatibility - if (data.execute && !data.target) { - return { ...data, target: data.execute }; + // #3713: lower the deprecated `execute` alias into the canonical `target` AND + // drop it from the output — the same "canonical wins, alias disappears" shape + // as `agent.knowledge.topics` → `sources` (#1891). + // + // Keeping both slots on the parsed output made the conflict *representable*, + // and the readers resolved it in OPPOSITE directions: this transform preferred + // `target`, while objectui's ActionRunner did `execute || target`. An action + // declaring both therefore ran `target` server-side and `execute` client-side + // — two different scripts for one button, with no error anywhere. + // + // `target` wins, and the alias no longer reaches a renderer. The server runtime + // never reads `execute` at all (`isHeadlessInvokableAction` gates on + // `target || body`, and the dispatch candidate chain probes `target`/`name` — + // see runtime/src/action-execution.ts), so authoring `execute` works *solely* + // because it is lowered here: dropping it costs the server nothing and takes + // the ambiguity off the wire. Authors may still write `execute` — it stays on + // the input type (`ActionInput`), only the parsed output is canonical. + const { execute, ...rest } = data; + if (execute && !rest.target) { + return { ...rest, target: execute }; } - return data; + return rest; }).refine((data) => { // Require `target` for types that reference an external resource if (TARGET_REQUIRED_TYPES.has(data.type) && !data.target) {