fix(runtime,spec,lint): bind action.body only for type 'script' (#4352)

.changeset/action-body-type-gate.md

@@ -0,0 +1,80 @@
+---
+"@objectstack/runtime": minor
+"@objectstack/lint": minor
+"@objectstack/spec": patch
+---
+
+fix(runtime,lint): `action.body` binds a handler only for `type: 'script'` (#4352)
+
+`ActionSchema.body` has always described itself as "Only used when type is
+`script`", and its JSDoc went further — "Only meaningful when
+`type === 'script'`. When set, the runtime invokes the body inside the sandbox
+… and ignores `target`." The runtime read none of it:
+`actionBodyRunnerFactory` bound a handler the moment `body` parsed, and
+`collectBundleActions` collected any named action. A `type: 'url'` action
+carrying a leftover `body` was therefore registered in the action registry and
+executed in the sandbox — reachable through
+`POST /api/v1/actions/:object/:action` and through
+`ql.object(o).execute(name)`, and counted by the governance inventory as a live
+handler.
+
+Declared ≠ enforced, in the shape that is hardest to debug: an author flips
+`type` from `script` to `url`, reasonably concludes the body is now dead code,
+and it keeps running with nothing anywhere saying so.
+
+**Behaviour change.** `body` now runs only under `type: 'script'`:
+
+| Action | Before | After |
+|:--|:--|:--|
+| `type: 'script'` + `body` | body runs | unchanged — body runs |
+| `type` omitted + `body` | body runs | unchanged — body runs (`ActionType.default('script')`) |
+| `type: 'url' \| 'modal' \| 'flow' \| 'api' \| 'form'` + `body` | body ran | **no handler is bound**; the refusal is logged |
+
+Only an action that **explicitly** declares a non-`script` type *and* carries a
+`body` changes behaviour. An omitted `type` still means `script`, because the
+collectors walk raw bundle objects — a `strict: false` `defineStack` or a legacy
+`manifest.actions[]` never passes through `ActionSchema`, so the schema's own
+default has to be applied at the gate rather than assumed to have been applied
+already.
+
+**FROM → TO.** If you have an action whose body you want to keep running, set
+`type: 'script'` and move the navigation/dispatch target elsewhere; if you want
+the target behaviour, delete the now-inert `body`:
+
+```diff
+  {
+    name: 'open_portal',
+-   type: 'url',
++   type: 'script',
+    target: '/portal',
+    body: { language: 'js', source: "await ctx.api.object('lead').update(…)", capabilities: ['api.write'] },
+  }
+```
+
+The refusal is **not** silent — silence would only relocate the invisibility the
+issue is about. `actionBodyRunnerFactory` logs a warning naming the action, its
+declared `type`, and both fixes.
+
+Authoring-time rejection of the same contradiction already shipped in #4438
+(`ActionSchema` rejects `body` alongside a non-`script` `type`), so what remains
+reachable here is data at rest published before that gate existed, plus bundles
+that never parsed. This release closes that half. New tests also pin that the
+**publish gate resolves to the rejecting schema** — through
+`getMetadataTypeSchema('action')` and `ObjectSchema.actions` — so a re-point of
+either registration cannot silently reopen the hole while the schema's own unit
+tests stay green.
+
+`@objectstack/lint`'s `validate-action-body-writes` filters by `type` again.
+#4344 deliberately made that rule type-blind on the grounds that "the runtime
+binds a handler from `action.body` alone … checking what executes beats checking
+what the schema says should" — true then, and the comment predicted its own
+revision. Execution and declaration are the same set again, so a non-`script`
+body no longer produces write-set advice about writes that provably never
+happen; the publish gate names that metadata's real defect (`type`) with its own
+prescription.
+
+`collectBundleActions` stays deliberately type-blind: it feeds governance
+surfaces that must enumerate every declared action, bound or not, and the other
+bind path (`engine.setDefaultActionRunner`, for Studio-authored actions) never
+walks it. The gate lives at the single point where a `body` becomes an
+executable handler, so there is no second copy of the rule to drift.

packages/lint/src/validate-action-body-writes.test.ts

@@ -390,10 +390,32 @@ describe('validateActionBodyWrites — where action bodies live', () => {
     expect(findings[0].where).toBe('action "close_deal" › body');
   });
 
-  it('checks a body on a non-script action — the runtime binds one regardless of `type`', () => {
+  // [#4352] The inversion of #4344's original assertion. That test pinned
+  // "checks a body on a non-script action — the runtime binds one regardless of
+  // `type`", which was true of the runtime at the time and was recorded as
+  // provisional. The ruling closed the gap at the producer instead: the body
+  // no longer binds (`actionBodyRunnerFactory`) and the pair no longer
+  // publishes (`ActionSchema`). Nothing runs, so there is no write set to
+  // advise about — and a finding here would point at the write when the defect
+  // is the `type`.
+  it('skips a body on a non-script action — nothing binds it, so there is no write to check', () => {
     const findings = validateActionBodyWrites(
       stackWith("await ctx.api.object('crm_deal').update({ stag: 'won' });", { type: 'url', target: '/x' }),
     );
+    expect(findings).toEqual([]);
+  });
+
+  it('still checks a body on an action that omits `type` — the spec default is `script`', () => {
+    const findings = validateActionBodyWrites(
+      stackWith("await ctx.api.object('crm_deal').update({ stag: 'won' });"),
+    );
+    expect(findings).toHaveLength(1);
+  });
+
+  it('still checks a body on an explicit `type: "script"` action', () => {
+    const findings = validateActionBodyWrites(
+      stackWith("await ctx.api.object('crm_deal').update({ stag: 'won' });", { type: 'script' }),
+    );
     expect(findings).toHaveLength(1);
   });
 });

packages/lint/src/validate-action-body-writes.ts

@@ -218,17 +218,32 @@ function actionObjectBinding(action: AnyRec, parentObject?: string): string | un
  * The top-level entry is walked first, so a merged action reports at
  * `actions[i]` — the authored location, not the derived copy.
  *
- * `type` is deliberately not consulted: the runtime binds a handler from
- * `action.body` alone (`actionBodyRunnerFactory` never reads `type`), so a body
- * on a non-`script` action still runs and still fails silently. Checking what
- * executes beats checking what the schema says should.
+ * Only `type: 'script'` bodies are walked (`type` omitted counts, since
+ * `ActionType.default('script')` makes that the same declaration).
+ *
+ * This rule USED to be deliberately type-blind, on the grounds that the
+ * runtime bound a handler from `action.body` alone and so a body on a
+ * non-`script` action still ran and still failed silently — checking what
+ * executes beat checking what the schema said should. That comment predicted
+ * its own revision ("定了之后 lint 那边要跟着调"), and #4352 is the ruling:
+ * `actionBodyRunnerFactory` now refuses to bind a handler unless the type is
+ * `script`, and `ActionSchema` rejects the contradictory pair at publish. So
+ * what executes and what the schema says are the same set again, and walking
+ * a non-`script` body here would produce advice about writes that provably
+ * never happen — noise pointing at metadata whose real defect is the `type`,
+ * which the publish gate already names with its own prescription.
  */
 function collectActionBodies(stack: AnyRec): ActionBodySite[] {
   const sites: ActionBodySite[] = [];
   const seen = new Set<string>();
 
   const collect = (actions: unknown, pathPrefix: string, parentObject?: string): void => {
     asArray(actions).forEach((action, index) => {
+      // Same default the spec declares, and the same one the runtime gate
+      // applies — a stack may reach lint unparsed, so an omitted `type` is
+      // `'script'`, not "unknown".
+      const type = typeof action.type === 'string' ? action.type : 'script';
+      if (type !== 'script') return;
       const body = action.body;
       if (!isRec(body) || body.language !== 'js') return;
       const source = body.source;

packages/runtime/src/action-body-type-gate.test.ts

@@ -0,0 +1,122 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * [#4352] `action.body` binds a handler ONLY for `type: 'script'`.
+ *
+ * `ActionSchema.body` has always said "Only used when type is `script`", and
+ * its JSDoc is more explicit still ("Only meaningful when `type === 'script'`").
+ * The runtime read neither: `collectBundleActions` collected any named action
+ * and `actionBodyRunnerFactory` bound a handler the moment `body` parsed. So a
+ * `type: 'url'` action carrying a leftover body was registered in the action
+ * registry and executed in the sandbox — the declared ≠ enforced shape of
+ * Prime Directive #10, in its nastiest form: an author flips `type` away from
+ * `script`, reasonably concludes the body is now dead, and it is not.
+ *
+ * The sibling tests in `sandbox/body-runner.test.ts` pin the factory in
+ * isolation. THIS file pins the composition AppPlugin actually performs —
+ * `collectBundleActions(bundle)` → `actionBodyRunnerFactory(...)` → skip when
+ * no handler → `ql.registerAction(...)` — because that loop is where the
+ * registration decision is really made, and a factory that returns `undefined`
+ * only matters if the loop honours it (it does: `if (!handler) continue`).
+ *
+ * The bind loop is replicated rather than driven through a booted AppPlugin on
+ * purpose: booting one needs a kernel, an ObjectQL engine and a QuickJS
+ * sandbox, none of which participate in the decision under test. The
+ * replication is kept honest by asserting the collector's own output too, so a
+ * change to how actions are collected still surfaces here.
+ */
+
+import { describe, it, expect } from 'vitest';
+import { collectBundleActions } from './app-plugin.js';
+import { actionBodyRunnerFactory } from './sandbox/body-runner.js';
+import { QuickJSScriptRunner } from './sandbox/quickjs-runner.js';
+
+const jsBody = { language: 'js', source: 'return { ran: true };', capabilities: [] } as const;
+
+/** The exact registration loop from `AppPlugin.bindDeclarativeActions`. */
+function bindActions(bundle: unknown, logger?: { warn: (msg: string) => void }) {
+  const registered: Array<{ object: string; name: string }> = [];
+  const actions = collectBundleActions(bundle);
+  const runner = actionBodyRunnerFactory(new QuickJSScriptRunner(), {
+    ql: {},
+    appId: 'crm',
+    logger,
+  });
+  for (const action of actions) {
+    const handler = runner(action);
+    if (!handler) continue;
+    registered.push({ object: action.object ?? 'global', name: action.name });
+  }
+  return { collected: actions, registered };
+}
+
+describe('#4352 — a non-script action with a body binds no handler', () => {
+  it("registers the script action and skips the `type: 'url'` one", () => {
+    const warnings: string[] = [];
+    const { collected, registered } = bindActions(
+      {
+        actions: [
+          // The regression population: an explicit non-script type + a body.
+          { name: 'open_docs', label: 'Docs', type: 'url', target: 'https://x', body: jsBody },
+          // The overwhelmingly common case — unchanged.
+          { name: 'close_deal', label: 'Close', type: 'script', object: 'crm_deal', body: jsBody },
+        ],
+      },
+      { warn: (msg: string) => warnings.push(msg) },
+    );
+
+    // The collector stays type-blind by design — it feeds governance surfaces
+    // that must see every declared action, bound or not.
+    expect(collected.map((a) => a.name)).toEqual(['open_docs', 'close_deal']);
+
+    // ...but only the script action becomes an executable handler.
+    expect(registered).toEqual([{ object: 'crm_deal', name: 'close_deal' }]);
+
+    // And the refusal is audible: silence here would just move the invisibility.
+    expect(warnings).toHaveLength(1);
+    expect(warnings[0]).toContain('open_docs');
+    expect(warnings[0]).toContain("type: 'url'");
+  });
+
+  it('skips a non-script body declared under an object', () => {
+    const { registered } = bindActions({
+      objects: [
+        {
+          name: 'crm_lead',
+          actions: [
+            { name: 'open_portal', label: 'Portal', type: 'url', target: '/p', body: jsBody },
+            { name: 'score_lead', label: 'Score', type: 'script', body: jsBody },
+          ],
+        },
+      ],
+    });
+    expect(registered).toEqual([{ object: 'crm_lead', name: 'score_lead' }]);
+  });
+
+  it('binds an action that omits `type` — `ActionType.default(\'script\')`', () => {
+    // Bundles reach the collector RAW. A `strict: false` `defineStack` and a
+    // legacy `manifest.actions[]` never pass through `ActionSchema`, so the
+    // schema's default has to be applied here or the common shape breaks.
+    const { registered } = bindActions({
+      manifest: { actions: [{ name: 'legacy_untyped', label: 'Legacy', body: jsBody }] },
+    });
+    expect(registered).toEqual([{ object: 'global', name: 'legacy_untyped' }]);
+  });
+
+  it('leaves bodyless non-script actions exactly as they were', () => {
+    const warnings: string[] = [];
+    const { collected, registered } = bindActions(
+      {
+        actions: [
+          { name: 'open_docs', label: 'Docs', type: 'url', target: 'https://x' },
+          { name: 'convert', label: 'Convert', type: 'flow', target: 'crm_convert' },
+        ],
+      },
+      { warn: (msg: string) => warnings.push(msg) },
+    );
+    expect(collected).toHaveLength(2);
+    // They never bound a handler before this change either — nothing to warn about.
+    expect(registered).toEqual([]);
+    expect(warnings).toEqual([]);
+  });
+});

packages/runtime/src/app-plugin.ts

@@ -1441,6 +1441,15 @@ export function collectBundleHooks(bundle: any): any[] {
  *
  * Each returned record is a shallow copy with `object` set when the action
  * originated under an object (and not already present on the action itself).
+ *
+ * Deliberately type-BLIND, and it must stay that way: this collects every
+ * declared action, most of which (`url`, `modal`, `flow`, `api`, `form`)
+ * legitimately have no body and bind nothing. The `type: 'script'` gate that
+ * decides whether a `body` becomes an executable handler lives at the single
+ * bind point — `actionBodyRunnerFactory` (#4352) — because the other binder
+ * (`engine.setDefaultActionRunner`, for Studio-authored actions) never walks
+ * this collector at all. Re-filtering here would duplicate half the rule and
+ * leave the other binder ungated.
  */
 export function collectBundleActions(
     bundle: any,

packages/runtime/src/sandbox/body-runner.test.ts

@@ -166,6 +166,64 @@ describe('actionBodyRunnerFactory', () => {
     expect(factory({ name: 'noop' })).toBeUndefined();
   });
 
+  // ─── [#4352] the `type` gate ──────────────────────────────────────────────
+  // `ActionSchema.body` always said "Only used when type is `script`"; the
+  // runtime never read `type`, so a `type: 'url'` action carrying a leftover
+  // body still bound a handler and still executed. These pin the enforcement.
+  describe('binds a body only for `type: "script"` (#4352)', () => {
+    const body = { language: 'js', source: 'return { ran: true };', capabilities: [] } as const;
+
+    for (const type of ['url', 'modal', 'flow', 'api', 'form'] as const) {
+      it(`binds no handler for type: '${type}' and says why`, () => {
+        const warnings: string[] = [];
+        const factory = actionBodyRunnerFactory(runner, {
+          ql: {},
+          appId: 'crm',
+          logger: { warn: (msg: string) => warnings.push(msg) },
+        });
+        expect(factory({ name: 'leftover', object: 'lead', type, body })).toBeUndefined();
+        // Refusing silently would only relocate the invisibility the issue is about.
+        expect(warnings).toHaveLength(1);
+        expect(warnings[0]).toContain("type: '" + type + "'");
+        expect(warnings[0]).toContain('#4352');
+      });
+    }
+
+    it("binds for an explicit type: 'script'", async () => {
+      const factory = actionBodyRunnerFactory(runner, { ql: {}, appId: 'crm' });
+      const fn = factory({ name: 'ok', object: 'lead', type: 'script', body });
+      expect(typeof fn).toBe('function');
+      await expect(fn!({ params: {} })).resolves.toEqual({ ran: true });
+    });
+
+    it('binds when `type` is omitted — the spec default is `script`', async () => {
+      // The collectors walk RAW bundle objects; a `strict: false` defineStack or
+      // a legacy `manifest.actions[]` never passed through `ActionType.default`,
+      // so an omitted type must still mean `script` here.
+      const warnings: string[] = [];
+      const factory = actionBodyRunnerFactory(runner, {
+        ql: {},
+        appId: 'crm',
+        logger: { warn: (msg: string) => warnings.push(msg) },
+      });
+      const fn = factory({ name: 'ok', object: 'lead', body });
+      expect(typeof fn).toBe('function');
+      await expect(fn!({ params: {} })).resolves.toEqual({ ran: true });
+      expect(warnings).toEqual([]);
+    });
+
+    it('stays silent for a non-script action with no body — nothing is contradictory', () => {
+      const warnings: string[] = [];
+      const factory = actionBodyRunnerFactory(runner, {
+        ql: {},
+        appId: 'crm',
+        logger: { warn: (msg: string) => warnings.push(msg) },
+      });
+      expect(factory({ name: 'open_docs', type: 'url' })).toBeUndefined();
+      expect(warnings).toEqual([]);
+    });
+  });
+
   it('runs an L2 action body and returns its value', async () => {
     const factory = actionBodyRunnerFactory(runner, { ql: {}, appId: 'crm' });
     const fn = factory({