From 52dcd646a859455a6b8c95679db555c70539601d Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Mon, 22 Jun 2026 12:14:41 +0800 Subject: [PATCH] feat(spec): reject script actions with no executable binding + showcase execution tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prevents the class of bug fixed in #2169, where `showcase_mark_done` declared `type: 'script'` but carried neither a `body` nor a `target`. AppPlugin only registers an engine handler for actions with a runnable binding, so the action fell through to the `'*'` wildcard lookup and failed at invocation with `Action '' on object '*' not found` — a soft failure that build- and shape-level tests never caught. Two complementary layers: 1. Author/compile-time guard (root cause): `ActionSchema` now requires `body || target` when `type === 'script'` (a `superRefine`, mirroring the existing "non-script types require `target`" rule). `os build` / `defineAction` now reject the broken shape immediately, for every bundle. Verified against the full monorepo build — every shipped bundle still compiles, so this only rejects configurations that were already non-functional at runtime. Existing spec fixtures that relied on the looser rule were updated. 2. Execution-path test (catch-net): examples/app-showcase/test/actions.test.ts drives the real `actionBodyRunnerFactory` + QuickJS sandbox against the shipped actions — asserting Mark Done produces a handler and writes `{ done: true, progress: 100 }`. The prior coverage test only checked that each ActionType *appeared* in the bundle, which is what let #2169 ship. Co-Authored-By: Claude Opus 4.8 --- .changeset/action-script-executable-guard.md | 19 +++++ examples/app-showcase/test/actions.test.ts | 86 ++++++++++++++++++++ packages/spec/src/stack.test.ts | 27 +++--- packages/spec/src/ui/action.test.ts | 60 ++++++++++++-- packages/spec/src/ui/action.zod.ts | 15 ++++ 5 files changed, 188 insertions(+), 19 deletions(-) create mode 100644 .changeset/action-script-executable-guard.md create mode 100644 examples/app-showcase/test/actions.test.ts diff --git a/.changeset/action-script-executable-guard.md b/.changeset/action-script-executable-guard.md new file mode 100644 index 0000000000..e61f538109 --- /dev/null +++ b/.changeset/action-script-executable-guard.md @@ -0,0 +1,19 @@ +--- +"@objectstack/spec": minor +--- + +spec(action): a `script` action must declare an executable binding — reject at +author/compile time when it has neither an inline `body` nor a `target`. + +A `type: 'script'` action with no `body` and no `target` registers no runtime +handler: `AppPlugin` skips it, and invoking it falls through to the wildcard +lookup and fails with `Action '' on object '*' not found` (the #2169 +"Mark Done" bug). The shape was schema-valid and passed coverage tests, so the +break only surfaced when a user clicked the button. + +`ActionSchema` now enforces the invariant via `superRefine`: `script` requires +`body || target` (mirroring the existing "non-script types require `target`" +rule). `body`-bound actions are auto-registered by the runtime; `target`-bound +actions name a function wired imperatively (e.g. via `onEnable`). This only +rejects configurations that were already non-functional at runtime — verified +against the full monorepo build (every shipped bundle still compiles). diff --git a/examples/app-showcase/test/actions.test.ts b/examples/app-showcase/test/actions.test.ts new file mode 100644 index 0000000000..265d1babac --- /dev/null +++ b/examples/app-showcase/test/actions.test.ts @@ -0,0 +1,86 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { actionBodyRunnerFactory, QuickJSScriptRunner } from '@objectstack/runtime'; + +import { allActions, MarkDoneAction } from '../src/actions/index.js'; + +/** + * Execution-path coverage for declared actions. + * + * The `coverage.test.ts` check only asserts that every `ActionType` *appears* + * in the bundle — a `type: 'script'` action with no executable handler passes + * it. That blind spot shipped the #2169 bug: `showcase_mark_done` declared + * `type: 'script'` but carried neither a `body` nor a `target`, so AppPlugin + * registered no engine handler and clicking "Mark Done" failed at runtime with + * `Action 'showcase_mark_done' on object '*' not found`. + * + * These tests drive the **real** runtime path — `actionBodyRunnerFactory` + + * the QuickJS sandbox, the exact bridge AppPlugin uses — against the actions as + * shipped. A body that fails to parse, references the wrong field, or is missing + * entirely fails here, not in production. + */ +describe('showcase actions — executability', () => { + const runner = new QuickJSScriptRunner(); + + it('every declared `script` action is executable (has a body or a target)', () => { + // Mirrors the platform invariant enforced by ActionSchema: a script action + // must be bound to *something* runnable. `target` actions are wired + // imperatively (e.g. via onEnable); `body` actions are auto-registered. + const scriptActions = allActions.filter((a) => a.type === 'script'); + expect(scriptActions.length).toBeGreaterThan(0); + for (const a of scriptActions) { + expect( + Boolean((a as { body?: unknown }).body) || Boolean((a as { target?: unknown }).target), + `script action '${a.name}' has neither body nor target — it cannot be invoked`, + ).toBe(true); + } + }); + + it('the runtime produces a handler for Mark Done (regression: #2169)', () => { + const factory = actionBodyRunnerFactory(runner, { ql: {}, appId: 'showcase' }); + const handler = factory(MarkDoneAction as never); + expect(typeof handler).toBe('function'); + }); + + it('Mark Done flips `done` + `progress` via the sandboxed body', async () => { + // Capture what the action writes through the proxied ObjectQL engine. + let written: { object: string; data: Record } | undefined; + const ql = { + object: (object: string) => ({ + update: async (data: Record) => { + written = { object, data }; + return { id: data.id }; + }, + }), + }; + + const factory = actionBodyRunnerFactory(runner, { ql, appId: 'showcase' }); + const handler = factory(MarkDoneAction as never); + expect(typeof handler).toBe('function'); + + const result = await handler!({ + recordId: 'task_1', + record: { id: 'task_1', status: 'in_progress', progress: 40, done: false }, + params: {}, + user: { id: 'u1' }, + }); + + // It updates the right object with the completion fields — and deliberately + // does NOT touch `status` (the state-machine only permits in_review -> done). + expect(written?.object).toBe('showcase_task'); + expect(written?.data).toMatchObject({ id: 'task_1', done: true, progress: 100 }); + expect(written?.data).not.toHaveProperty('status'); + expect(result).toEqual({ ok: true, id: 'task_1' }); + }); + + it('a body-less `script` action yields no handler (the #2169 failure mode)', () => { + // Documents exactly what used to ship: with neither body nor target the + // runtime has nothing to register, so the HTTP action route falls into the + // wildcard fallback. ActionSchema now rejects this at author time; this + // asserts the runtime half of the contract. + const factory = actionBodyRunnerFactory(runner, { ql: {}, appId: 'showcase' }); + const handler = factory({ name: 'broken', object: 'showcase_task' } as never); + expect(handler).toBeUndefined(); + }); +}); diff --git a/packages/spec/src/stack.test.ts b/packages/spec/src/stack.test.ts index 5b61b3e4d8..18e4aed419 100644 --- a/packages/spec/src/stack.test.ts +++ b/packages/spec/src/stack.test.ts @@ -728,6 +728,7 @@ describe('defineStack - Map Format Support', () => { approve_deal: { label: 'Approve Deal', type: 'script', + target: 'noop', }, }, }; @@ -1062,7 +1063,7 @@ describe('defineStack - Action Auto-Merge into Objects', () => { { name: 'task', fields: { title: { type: 'text' as const } } }, ], actions: [ - { name: 'approve_task', label: 'Approve', objectName: 'task' }, + { name: 'approve_task', label: 'Approve', objectName: 'task', target: 'noop' }, ], }; @@ -1078,8 +1079,8 @@ describe('defineStack - Action Auto-Merge into Objects', () => { { name: 'deal', fields: { amount: { type: 'number' as const } } }, ], actions: [ - { name: 'close_deal', label: 'Close Deal', objectName: 'deal' }, - { name: 'reopen_deal', label: 'Reopen Deal', objectName: 'deal' }, + { name: 'close_deal', label: 'Close Deal', objectName: 'deal', target: 'noop' }, + { name: 'reopen_deal', label: 'Reopen Deal', objectName: 'deal', target: 'noop' }, ], }; @@ -1096,8 +1097,8 @@ describe('defineStack - Action Auto-Merge into Objects', () => { { name: 'project', fields: { name: { type: 'text' as const } } }, ], actions: [ - { name: 'complete_task', label: 'Complete', objectName: 'task' }, - { name: 'archive_project', label: 'Archive', objectName: 'project' }, + { name: 'complete_task', label: 'Complete', objectName: 'task', target: 'noop' }, + { name: 'archive_project', label: 'Archive', objectName: 'project', target: 'noop' }, ], }; @@ -1115,7 +1116,7 @@ describe('defineStack - Action Auto-Merge into Objects', () => { { name: 'task', fields: { title: { type: 'text' as const } } }, ], actions: [ - { name: 'global_action', label: 'Global' }, + { name: 'global_action', label: 'Global', target: 'noop' }, ], }; @@ -1130,8 +1131,8 @@ describe('defineStack - Action Auto-Merge into Objects', () => { { name: 'task', fields: { title: { type: 'text' as const } } }, ], actions: [ - { name: 'approve_task', label: 'Approve', objectName: 'task' }, - { name: 'global_search', label: 'Search' }, + { name: 'approve_task', label: 'Approve', objectName: 'task', target: 'noop' }, + { name: 'global_search', label: 'Search', target: 'noop' }, ], }; @@ -1149,11 +1150,11 @@ describe('defineStack - Action Auto-Merge into Objects', () => { { name: 'task', fields: { title: { type: 'text' as const } }, - actions: [{ name: 'inline_action', label: 'Inline' }], + actions: [{ name: 'inline_action', label: 'Inline', target: 'noop' }], }, ], actions: [ - { name: 'merged_action', label: 'Merged', objectName: 'task' }, + { name: 'merged_action', label: 'Merged', objectName: 'task', target: 'noop' }, ], }; @@ -1170,7 +1171,7 @@ describe('defineStack - Action Auto-Merge into Objects', () => { { name: 'task', fields: { title: { type: 'text' as const } } }, ], actions: [ - { name: 'approve_task', label: 'Approve', objectName: 'task' }, + { name: 'approve_task', label: 'Approve', objectName: 'task', target: 'noop' }, ], }; @@ -1186,7 +1187,7 @@ describe('defineStack - Action Auto-Merge into Objects', () => { { name: 'task', fields: { title: { type: 'text' as const } } }, ], actions: [ - { name: 'approve_deal', label: 'Approve', objectName: 'nonexistent_object' }, + { name: 'approve_deal', label: 'Approve', objectName: 'nonexistent_object', target: 'noop' }, ], }; @@ -1198,7 +1199,7 @@ describe('defineStack - Action Auto-Merge into Objects', () => { const config = { manifest: baseManifest, actions: [ - { name: 'approve_deal', label: 'Approve', objectName: 'deal' }, + { name: 'approve_deal', label: 'Approve', objectName: 'deal', target: 'noop' }, ], }; diff --git a/packages/spec/src/ui/action.test.ts b/packages/spec/src/ui/action.test.ts index 3cb6547d88..d304467d06 100644 --- a/packages/spec/src/ui/action.test.ts +++ b/packages/spec/src/ui/action.test.ts @@ -43,9 +43,12 @@ describe('ActionParamSchema', () => { describe('ActionSchema', () => { describe('Basic Action Properties', () => { it('should accept minimal action', () => { + // A `script` action (the default type) must be bound to something + // runnable — here a `target` naming a registered handler. const action: ActionType = { name: 'approve', label: 'Approve', + target: 'approve_handler', }; const result = ActionSchema.parse(action); @@ -56,12 +59,12 @@ describe('ActionSchema', () => { it('should enforce snake_case for action name', () => { const validNames = ['approve_record', 'send_email', 'close_case']; validNames.forEach(name => { - expect(() => ActionSchema.parse({ name, label: 'Test' })).not.toThrow(); + expect(() => ActionSchema.parse({ name, label: 'Test', target: 'h' })).not.toThrow(); }); const invalidNames = ['approveRecord', 'Approve-Record', '123action', '_internal']; invalidNames.forEach(name => { - expect(() => ActionSchema.parse({ name, label: 'Test' })).toThrow(); + expect(() => ActionSchema.parse({ name, label: 'Test', target: 'h' })).toThrow(); }); }); @@ -70,6 +73,7 @@ describe('ActionSchema', () => { name: 'delete_record', label: 'Delete', icon: 'trash-2', + target: 'delete_handler', }; expect(() => ActionSchema.parse(action)).not.toThrow(); @@ -91,14 +95,25 @@ describe('ActionSchema', () => { }); }); - it('should accept script type without target', () => { + it('should accept a script action bound by inline body (no target)', () => { expect(() => ActionSchema.parse({ name: 'test_action', label: 'Test', type: 'script', + body: { language: 'expression', source: 'true' }, })).not.toThrow(); }); + it('should reject a script action with neither body nor target', () => { + // Regression guard for #2169: a body-less, target-less script action + // registers no runtime handler and fails on invocation. + expect(() => ActionSchema.parse({ + name: 'test_action', + label: 'Test', + type: 'script', + })).toThrow(/body|target/); + }); + it('should reject url/flow/modal/api types without target', () => { const targetRequiredTypes = ['url', 'flow', 'modal', 'api'] as const; targetRequiredTypes.forEach(type => { @@ -114,6 +129,7 @@ describe('ActionSchema', () => { const action = { name: 'custom_action', label: 'Custom', + target: 'custom_handler', }; const result = ActionSchema.parse(action); @@ -135,6 +151,7 @@ describe('ActionSchema', () => { const action: ActionType = { name: 'multi_location', label: 'Multi Location', + target: 'noop', locations, }; @@ -145,6 +162,7 @@ describe('ActionSchema', () => { const action: ActionType = { name: 'toolbar_action', label: 'Toolbar Action', + target: 'noop', locations: ['list_toolbar'], }; @@ -157,6 +175,7 @@ describe('ActionSchema', () => { const action = { name: 'approve_task', label: 'Approve Task', + target: 'noop', objectName: 'task', }; @@ -168,6 +187,7 @@ describe('ActionSchema', () => { const action = { name: 'global_search', label: 'Global Search', + target: 'noop', }; const result = ActionSchema.parse(action); @@ -185,6 +205,7 @@ describe('ActionSchema', () => { name: 'test_action', label: 'Test', objectName: 'my_object', + target: 'noop', })).not.toThrow(); }); }); @@ -229,6 +250,7 @@ describe('ActionSchema', () => { const action: ActionType = { name: 'transfer_ownership', label: 'Transfer Ownership', + target: 'noop', type: 'script', params: [ { @@ -253,6 +275,7 @@ describe('ActionSchema', () => { const action: ActionType = { name: 'change_status', label: 'Change Status', + target: 'noop', params: [ { name: 'status', @@ -276,6 +299,7 @@ describe('ActionSchema', () => { const action: ActionType = { name: 'delete_all', label: 'Delete All', + target: 'noop', confirmText: 'Are you sure you want to delete all records? This cannot be undone.', }; @@ -286,6 +310,7 @@ describe('ActionSchema', () => { const action: ActionType = { name: 'send_notification', label: 'Send Notification', + target: 'noop', successMessage: 'Notification sent successfully!', }; @@ -296,6 +321,7 @@ describe('ActionSchema', () => { const action: ActionType = { name: 'update_status', label: 'Update Status', + target: 'noop', refreshAfter: true, }; @@ -306,6 +332,7 @@ describe('ActionSchema', () => { const action: ActionType = { name: 'complete_task', label: 'Complete Task', + target: 'noop', confirmText: 'Mark this task as complete?', successMessage: 'Task completed successfully!', refreshAfter: true, @@ -320,6 +347,7 @@ describe('ActionSchema', () => { const action: ActionType = { name: 'approve', label: 'Approve', + target: 'approve_handler', visible: 'status == "pending" && user.can_approve', }; @@ -487,6 +515,7 @@ describe('Action Factory', () => { const action = Action.create({ name: 'test_action', label: 'Test Action', + target: 'noop', }); expect(action.name).toBe('test_action'); @@ -510,6 +539,7 @@ describe('Action Factory', () => { const action = Action.create({ name: 'update_record', label: 'Update', + target: 'noop', refreshAfter: true, }); @@ -525,6 +555,7 @@ describe('Action Factory', () => { expect(() => Action.create({ name: 'valid_name', label: 'Valid', + target: 'noop', })).not.toThrow(); }); }); @@ -576,13 +607,14 @@ describe('ActionSchema - ai block (ADR-0011)', () => { const result = ActionSchema.parse({ name: 'maybe_expose', label: 'Maybe', + target: 'noop', ai: {}, }); expect(result.ai?.exposed).toBe(false); }); it('accepts an action with no ai block (not exposed)', () => { - const result = ActionSchema.parse({ name: 'plain', label: 'Plain' }); + const result = ActionSchema.parse({ name: 'plain', label: 'Plain', target: 'noop' }); expect(result.ai).toBeUndefined(); }); @@ -610,6 +642,7 @@ describe('ActionSchema - ai block (ADR-0011)', () => { const result = ActionSchema.parse({ name: 'triage_case', label: 'Triage Case', + target: 'noop', objectName: 'crm_case', params: [{ name: 'priority', type: 'text' }], ai: { @@ -652,6 +685,7 @@ describe('ActionSchema - ai block (ADR-0011)', () => { ActionSchema.parse({ name: 'hint_record_id', label: 'Hint', + target: 'noop', objectName: 'task', locations: ['record_header'], ai: { exposed: true, description: longDescription, paramHints: { recordId: { description: 'The task id.' } } }, @@ -661,7 +695,7 @@ describe('ActionSchema - ai block (ADR-0011)', () => { it('does not require a description when exposed is false', () => { expect(() => - ActionSchema.parse({ name: 'opted_out', label: 'Out', ai: { exposed: false } }), + ActionSchema.parse({ name: 'opted_out', label: 'Out', target: 'noop', ai: { exposed: false } }), ).not.toThrow(); }); }); @@ -671,6 +705,7 @@ describe('Action ARIA Integration', () => { expect(() => ActionSchema.parse({ name: 'accessible_action', label: 'Delete', + target: 'noop', aria: { ariaLabel: 'Delete this record permanently', role: 'button' }, })).not.toThrow(); }); @@ -687,6 +722,7 @@ describe('ActionSchema - variant', () => { const result = ActionSchema.parse({ name: 'test_action', label: 'Test', + target: 'noop', variant, }); expect(result.variant).toBe(variant); @@ -697,6 +733,7 @@ describe('ActionSchema - variant', () => { const result = ActionSchema.parse({ name: 'no_variant', label: 'Action', + target: 'noop', }); expect(result.variant).toBeUndefined(); }); @@ -713,6 +750,7 @@ describe('ActionSchema - variant', () => { const result = ActionSchema.parse({ name: 'delete_record', label: 'Delete', + target: 'delete_handler', variant: 'danger', confirmText: 'Are you sure?', icon: 'trash', @@ -748,11 +786,21 @@ describe('ActionSchema - execute → target migration', () => { expect(result.target).toBe('preferredHandler'); }); - it('should allow script type without target or execute', () => { + it('should reject a script with neither target/execute nor body', () => { + // #2169: a script action with no handler binding registers nothing. expect(() => ActionSchema.parse({ name: 'inline_script', label: 'Inline', type: 'script', + })).toThrow(/body|target/); + }); + + it('should allow a script bound by inline body (no target/execute)', () => { + expect(() => ActionSchema.parse({ + name: 'inline_body_script', + label: 'Inline', + type: 'script', + body: { language: 'expression', source: 'true' }, })).not.toThrow(); }); }); diff --git a/packages/spec/src/ui/action.zod.ts b/packages/spec/src/ui/action.zod.ts index d1eb4a4c4e..88d6f826d2 100644 --- a/packages/spec/src/ui/action.zod.ts +++ b/packages/spec/src/ui/action.zod.ts @@ -467,6 +467,21 @@ export const ActionSchema = lazySchema(() => z.object({ }, { message: "Action 'target' is required when type is 'url', 'flow', 'modal', 'api', or 'form'.", path: ['target'], +}).refine((data) => { + // A `script` action must be *executable*: it needs either an inline `body` + // (the runtime invokes it in the sandbox) or a `target` naming a registered + // bundle function. With neither, AppPlugin registers no engine handler and + // the action fails at runtime with `Action '' on object '*' not found` + // (the #2169 Mark Done bug) — a soft failure invisible to build & shape + // tests. Reject it at author/compile time instead. + if (data.type === 'script' && !data.body && !data.target) { + return false; + } + return true; +}, { + message: + "A 'script' action requires either an inline `body` (sandboxed L1/L2 handler) or a `target` (a registered bundle function name).", + path: ['body'], }).refine((data) => { // ADR-0011: an exposed action must carry an LLM-facing description. if (data.ai?.exposed === true && !data.ai.description) {