fix(runtime): a nested sandboxed hook refusal is a rejection, not a sandbox fault

.changeset/17265-nested-hook-refusal-is-a-rejection.md

@@ -0,0 +1,42 @@
+---
+'@objectstack/runtime': patch
+---
+
+A sandboxed hook's business refusal reached through a script action answers 4xx, not `500 INTERNAL_ERROR`
+
+`POST /api/v1/actions/:object/:action` answered **`500 INTERNAL_ERROR`** when a
+`beforeUpdate` hook refused a state transition for a business reason and the
+refusal travelled out through the action body's `ctx.api` write. The same refusal
+has answered **`400`**, with the hook's sentence verbatim, on `/data` since
+objectstack#11588. A 500 tells every client "the platform broke", so a
+well-behaved one retries, alerts or pages for a guard that will never say yes.
+
+**Where the producer was.** Not in the action route's classifier — that read the
+shape it was handed correctly, and both sides of the line it pins (`a deliberate
+REJECTION is a 400` / `an unexpected FAULT is a 500`) are unchanged. The refusal
+arrived already stripped of every mark that says "a body reported this on
+purpose", one VM hop earlier: `hostErrorToVm` marked **every** `SandboxError`
+crossing into the action body's VM as the sandbox's OWN fault (objectstack#4431)
+on an `instanceof` test — and a nested sandboxed hook's refusal *is* a
+`SandboxError`, wrapped by the same runner one level down. The pump branch that
+reads that marker then discarded `innerMessage`, `code`, `status` and `fields`,
+and the classifier read the missing business message as a crash.
+
+**What changed.** The marker now asks the question the `/data` door asks —
+`sandboxBusinessMessage`, objectstack#11588 — instead of testing the error's
+class. Both of that predicate's conditions travel, because both are load-bearing:
+a capability denial carries no business message and stays a fault, and a nested
+body that **crashed** carries `TypeError: …` and stays a fault too.
+
+**No status was picked for this route.** It matches what `/data` already answers
+for the same producer: the status the body declared, or `400` when it declared
+none. A refusal that declares `{ status: 409, code: 'RECORD_LOCKED' }` now
+reaches the caller as `409 RECORD_LOCKED` instead of losing both.
+
+**The sentence a caller receives is byte-identical to what the 500 carried** —
+this moves the status, not the prose. The flattened `SandboxError: ` name prefix
+is stripped on the rejection path by the same helper the fault path already used.
+
+No authorable key, accept set or export surface moves; no consumer needs a
+change. Clients branching on 5xx to decide whether to retry will stop retrying
+these refusals.

packages/runtime/src/sandbox/nested-hook-refusal-is-a-rejection.test.ts

@@ -0,0 +1,228 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * [#17265] A sandboxed hook's BUSINESS REFUSAL reached through a script
+ * action's `ctx.api` write is a rejection, not the sandbox faulting.
+ *
+ * ## What was measured broken
+ *
+ * The card reports `POST /api/v1/actions/clm_contract/submit_contract`
+ * answering `500 INTERNAL_ERROR` for a `beforeUpdate` hook that refused a state
+ * transition with a written, user-facing sentence — the same refusal `/data`
+ * has answered `400` with the sentence verbatim since #11588.
+ *
+ * `domains/actions.ts`'s classifier is NOT the producer: it reads the shape it
+ * is handed correctly (`actions-fault-vs-rejection.test.ts` pins both sides of
+ * that line and neither moves). The refusal arrives at it already stripped of
+ * every mark that says "a body reported this on purpose", one VM hop earlier:
+ *
+ *  1. the sandboxed `beforeUpdate` hook refuses — its own runner wraps that as
+ *     `SandboxError("hook 'g' threw: <biz>", "<biz>")`, `innerMessage` SET;
+ *  2. it travels out of `engine.update()` into the ACTION body's host call, so
+ *     `hostErrorToVm` marshals it INTO the action's VM — and marked every
+ *     `SandboxError` reaching it as {@link SANDBOX_FAULT_PROP}, the sandbox's
+ *     OWN fault (#4431), on an `instanceof` test;
+ *  3. escaping the action body uncaught, the pump loop reads that marker and
+ *     throws a bare `SandboxError` — no `innerMessage`, no `code`, no
+ *     `status`, no `fields`;
+ *  4. which is, by the #3951 contract, exactly a CRASH ⇒ `errorFromThrown(err,
+ *     500)` ⇒ `500 INTERNAL_ERROR`.
+ *
+ * The marker's own sibling pin already named this risk — "a marker applied too
+ * broadly would turn every failed write into a 500"
+ * (`capability-denial-is-a-fault.test.ts`) — and measured it with a plain
+ * `ValidationError`, which is not a `SandboxError` and so never tripped the
+ * `instanceof`. A NESTED sandboxed refusal is.
+ *
+ * ## The line this file pins
+ *
+ * The discriminator is the one `/data` asks (`sandboxBusinessMessage`, #11588):
+ * does the error carry a caller-addressed business sentence that is not a
+ * script fault? A refusal does and stays a rejection; a capability denial and a
+ * nested CRASH do not and stay faults. So `/data`'s answer and the action
+ * route's answer are the same answer, per route and per status.
+ */
+
+import { describe, it, expect, vi } from 'vitest';
+
+import { HttpDispatcher } from '../http-dispatcher.js';
+import { QuickJSScriptRunner, SandboxError } from './quickjs-runner.js';
+import type { ScriptContext, ScriptRunOptions } from './script-runner.js';
+
+/** The sentence the consuming app's guard addressed to its end user. */
+const REFUSAL = 'A contract cannot be submitted without a version file';
+
+const runner = new QuickJSScriptRunner({ hookTimeoutMs: 10_000, actionTimeoutMs: 10_000 });
+const actionOpts: ScriptRunOptions = { origin: { kind: 'action', name: 'submit_contract' } };
+
+/**
+ * The shape a SANDBOXED `beforeUpdate` hook's deliberate refusal really has
+ * when `engine.update()` hands it back to the action body's host call — built
+ * by `quickjs-runner`'s own pump loop one level down, so `innerMessage` is set.
+ */
+function nestedHookRefusal(): SandboxError {
+    return new SandboxError(`hook 'guard_contract_submit' threw: ${REFUSAL}`, REFUSAL);
+}
+
+/** A `ctx.api.object(x).update(...)` host seam whose write the hook refuses. */
+function refusingApi(thrown: () => unknown) {
+    return {
+        object: (_n: string) => ({
+            update: async () => { throw thrown(); },
+        }),
+    };
+}
+
+function ctx(over: Partial<ScriptContext> = {}): ScriptContext {
+    return { input: {}, ...over };
+}
+
+/** Run a script action whose `ctx.api` write throws `thrown`, and return the escape. */
+async function escapeOf(thrown: () => unknown): Promise<any> {
+    const err = await runner.runScript(
+        {
+            language: 'js',
+            source: "return await ctx.api.object('clm_contract').update('c_1', { status: 'submitted' });",
+            capabilities: ['api.write'],
+        },
+        ctx({ api: refusingApi(thrown) }),
+        actionOpts,
+    ).then(() => null, (e) => e);
+    expect(err, 'expected the action to reject, but the script resolved').toBeInstanceOf(SandboxError);
+    return err;
+}
+
+// ── the wire half ────────────────────────────────────────────────────────────
+
+const scriptAction = {
+    name: 'submit_contract',
+    objectName: 'clm_contract',
+    type: 'script',
+    body: { language: 'js', source: 'return 1;', capabilities: ['api.write'] },
+};
+
+/** The same dispatcher harness `actions-fault-vs-rejection.test.ts` uses. */
+function makeDispatcher(thrown: unknown) {
+    const objectDef = { name: 'clm_contract', actions: [scriptAction] };
+    const ql: any = {
+        executeAction: vi.fn(async () => { throw thrown; }),
+        getSchema: (n: string) => (n === objectDef.name ? objectDef : undefined),
+        registry: { getObject: (n: string) => (n === objectDef.name ? objectDef : undefined), getItem: () => undefined },
+        find: vi.fn(async () => [{ id: 'c_1', status: 'draft' }]),
+        insert: vi.fn(), update: vi.fn(), delete: vi.fn(),
+    };
+    const metadata: any = {
+        load: vi.fn(async () => null),
+        listObjects: vi.fn(async () => [objectDef]),
+        getObject: vi.fn(async () => objectDef),
+    };
+    const kernel: any = {
+        context: {
+            getService: (n: string) =>
+                n === 'objectql' || n === 'data' ? ql : n === 'metadata' ? metadata : null,
+        },
+    };
+    return new HttpDispatcher(kernel);
+}
+
+async function wireAnswer(thrown: unknown) {
+    const res: any = await makeDispatcher(thrown).handleActions(
+        '/clm_contract/submit_contract/c_1',
+        'POST',
+        {},
+        { request: {}, environmentId: 'platform', executionContext: { userId: 'u1', systemPermissions: [] } } as any,
+    );
+    return res.response;
+}
+
+describe('[#17265] a nested sandboxed hook refusal keeps its business message', () => {
+    it("a beforeUpdate refusal reached through ctx.api.update is NOT the sandbox's own fault", async () => {
+        const err = await escapeOf(nestedHookRefusal);
+
+        // THE defect: the marker was applied on `instanceof SandboxError`, so
+        // the nested refusal came back as a bare fault — no business message at
+        // all, which is exactly what the #3951 contract reads as a CRASH.
+        expect(err.innerMessage, 'the hook refusal must survive as a business message').toBeDefined();
+        expect(err.innerMessage).toContain(REFUSAL);
+        // …and the action's own debug wrapper still identifies who threw, in
+        // the log-only `.message`, which keeps the WHOLE chain.
+        expect(err.message).toContain("action 'submit_contract' threw:");
+        expect(err.message).toContain("hook 'guard_contract_submit' threw:");
+
+        // Message-NEUTRAL: this repair moves the status, not the sentence. The
+        // client-facing text is byte-identical to what the 500 already carried
+        // — the VM's `SandboxError: ` name prefix is a debug artefact and has
+        // never reached the wire.
+        expect(err.innerMessage).toBe(`hook 'guard_contract_submit' threw: ${REFUSAL}`);
+        expect(err.innerMessage).not.toContain('SandboxError:');
+    });
+
+    it('the wire answer matches /data: 400 VALIDATION_ERROR with the sentence', async () => {
+        // /data answers this refusal `400` with `error.innerMessage` verbatim
+        // (`error-response.ts`'s sandbox unwrap door, `declared ?? 400`). The
+        // action route must not answer a second thing for one refusal.
+        const response = await wireAnswer(await escapeOf(nestedHookRefusal));
+
+        expect(response.status).toBe(400);
+        expect(response.body.error.code).toBe('VALIDATION_ERROR');
+        expect(response.body.error.message).toContain(REFUSAL);
+    });
+
+    it("a nested refusal's DECLARED status and code survive the hop", async () => {
+        // The fault branch dropped the whole `__errorInfo` payload, not just
+        // `innerMessage`, so a hook declaring `{ status: 409, code:
+        // 'RECORD_LOCKED' }` lost both and was flattened to 500. `/data`
+        // answers `declared ?? 400` for this producer (#9967); the action door
+        // honours a declared status at its own first arm (#7867), so once the
+        // classification is right the two agree without a second rule.
+        const locked = () => {
+            const e: any = new SandboxError(
+                `hook 'guard_contract_submit' threw: ${REFUSAL}`,
+                REFUSAL,
+                { code: 'RECORD_LOCKED', status: 409 },
+            );
+            return e;
+        };
+        const response = await wireAnswer(await escapeOf(locked));
+
+        expect(response.status).toBe(409);
+        expect(response.body.error.code).toBe('RECORD_LOCKED');
+        expect(response.body.error.message).toContain(REFUSAL);
+    });
+});
+
+describe('[#17265] the FAULT side of the #4431 contract is untouched', () => {
+    it("a nested CRASH is still a fault — sandboxBusinessMessage declines it", async () => {
+        // A hook that blew up arrives in the same shape with a native error
+        // name inside `innerMessage`. `/data` answers the sanitised 500 for it
+        // (#7543), so the action route must too: the business-message read is
+        // what separates them, never the error's class.
+        const crash = () =>
+            new SandboxError(
+                "hook 'guard_contract_submit' threw: TypeError: cannot read properties of undefined",
+                'TypeError: cannot read properties of undefined',
+            );
+        const response = await wireAnswer(await escapeOf(crash));
+
+        expect(response.status).toBe(500);
+        expect(response.body.error.code).toBe('INTERNAL_ERROR');
+    });
+
+    it('a capability denial inside the action body is still a fault', async () => {
+        // The #4431 case itself: the sandbox refused before user code ran, so
+        // there is no business message to carry and the 500 must stand.
+        const err = await runner.runScript(
+            {
+                language: 'js',
+                source: "return ctx.api.object('clm_contract').count({});",
+                capabilities: [],
+            },
+            ctx({ api: { object: (_n: string) => ({ count: (_f: unknown) => 1 }) } }),
+            actionOpts,
+        ).then(() => null, (e: any) => e);
+
+        expect(err).toBeInstanceOf(SandboxError);
+        expect(err.innerMessage).toBeUndefined();
+        expect((await wireAnswer(err)).status).toBe(500);
+    });
+});

packages/runtime/src/sandbox/quickjs-runner.ts

@@ -395,11 +395,19 @@ export class QuickJSScriptRunner implements ScriptRunner {
           // the 400 a denial used to get — and the client sees the capability
           // text without the `SandboxError: ` debug prefix.
           if (info?.sandboxFault) {
-            throw new SandboxError(sandboxFaultMessage(String(errStr)));
+            throw new SandboxError(withoutSandboxErrorPrefix(String(errStr)));
           }
+          // [#17265] The `.message` wrapper keeps the WHOLE flattened chain for
+          // the log — `action 'x' threw: SandboxError: hook 'g' threw: <msg>`
+          // names every frame that refused. The client-facing sentence drops the
+          // inner `SandboxError: ` token by the same rule the fault branch above
+          // applies: the VM's name prefix is a debug artefact and never reached
+          // the wire before a nested refusal stopped being classified as a
+          // fault, so this repair moves the STATUS and leaves the sentence a
+          // caller receives byte-identical to what it was.
           throw new SandboxError(
             `${args.origin.kind} '${args.origin.name}' threw: ${errStr}`,
-            userFacingMessage(String(errStr)),
+            userFacingMessage(withoutSandboxErrorPrefix(String(errStr))),
             info,
           );
         }
@@ -1291,7 +1299,19 @@ function hostErrorToVm(vm: QuickJSContext, err: unknown): QuickJSHandle {
     }
     // [#4431] Mark the sandbox's OWN faults so the pump loop can tell them
     // apart from a user throw after the VM has flattened both to a string.
-    if (err instanceof SandboxError) {
+    //
+    // [#17265] …asked as a QUESTION about the error, never as a bare
+    // `instanceof`. A NESTED sandboxed body's refusal is a `SandboxError` too:
+    // the `beforeUpdate` hook that `engine.update()` dispatched underneath this
+    // body's `ctx.api` write was wrapped by this very runner one level down. So
+    // the type test marked a deliberate business refusal as the sandbox
+    // faulting, the pump branch that reads the marker then dropped its
+    // `innerMessage` (and its `code`/`status`/`fields`), and
+    // `domains/actions.ts` read that absence as a CRASH — answering
+    // `500 INTERNAL_ERROR` for a refusal `/data` has answered `400` with the
+    // sentence verbatim since #11588. {@link sandboxRefusalMessage} is that
+    // door's own question, so the two doors answer one refusal once.
+    if (err instanceof SandboxError && sandboxRefusalMessage(err) === undefined) {
       const h = vm.true;
       vm.setProp(errH, SANDBOX_FAULT_PROP, h);
     }
@@ -1345,6 +1365,59 @@ function hostErrorToVm(vm: QuickJSContext, err: unknown): QuickJSHandle {
  */
 const SANDBOX_FAULT_PROP = '__objectstackSandboxFault';
 
+/**
+ * [#17265] The ECMA-262 native error constructors, plus SpiderMonkey's
+ * `InternalError` which QuickJS also raises — the third copy of one pattern,
+ * and deliberately a copy.
+ *
+ * `packages/rest`'s `isScriptFaultMessage` (`error-response.ts`) is the
+ * original and `packages/objectql`'s `isScriptCrash`
+ * (`hook-withheld-readonly-fault.ts`) already keeps its own, for the reason
+ * stated there: the importing package must not take a dependency on
+ * `@objectstack/rest` for a regex. This package's reason is one step narrower —
+ * `@objectstack/runtime` DOES depend on `@objectstack/rest`, but that package
+ * declares exactly one export subpath (`"."`) and re-exports nothing from
+ * `error-response`, so importing the predicate would mean WIDENING rest's
+ * published surface for an internal read.
+ *
+ * ⛔ Same deliberate omission of a bare `Error:` as both siblings: a body's
+ * plain `Error` is the documented way to AUTHOR a refusal, so it is never a
+ * crash.
+ */
+const NATIVE_ERROR_NAME_RE =
+  /^(?:Type|Reference|Range|Syntax|URI|Eval|Internal|Aggregate)Error(?::|$)/;
+
+/**
+ * [#17265] The caller-addressed BUSINESS sentence a sandboxed body threw, or
+ * `undefined` when this error is not a body's deliberate refusal.
+ *
+ * This is `packages/rest`'s `sandboxBusinessMessage` (#11588) — the read the
+ * `/data` door and, since #11684, the `/analytics/dataset/query` door both make
+ * instead of open-coding a local opinion. Both of its conditions travel, in the
+ * same order, because both are load-bearing HERE:
+ *
+ *  - a non-empty string `.innerMessage` — the sandbox's own mark for "user code
+ *    threw this deliberately", and by {@link SandboxError}'s contract the thing
+ *    a capability denial, a timeout and a marshalling failure all lack. Its
+ *    absence is what keeps every #4431 case marked as a fault;
+ *  - NOT a native error name (#7543). A nested body that CRASHED arrives in the
+ *    identical shape carrying `TypeError: …`, which is an internal fault and
+ *    not a sentence addressed to anyone. Dropping this half would turn a nested
+ *    crash into a 400 and move the `an unexpected FAULT is a 500` line that
+ *    `domains/actions-fault-vs-rejection.test.ts` pins.
+ *
+ * ⛔ A READ of the field the runner populated, never a pattern-strip of the
+ * `<kind> '<name>' threw:` wrapper off `.message` — the sibling's rule, for the
+ * sibling's reason: a plain error whose own prose contains `threw:` must not be
+ * rewritten.
+ */
+function sandboxRefusalMessage(error: unknown): string | undefined {
+  const inner = (error as { innerMessage?: unknown } | null | undefined)?.innerMessage;
+  if (typeof inner !== 'string' || !inner) return undefined;
+  if (NATIVE_ERROR_NAME_RE.test(inner.trim())) return undefined;
+  return inner;
+}
+
 /**
  * [#4431] Throw a sandbox-internal fault OUT OF a host function so it reaches
  * the VM carrying {@link SANDBOX_FAULT_PROP}.
@@ -1371,8 +1444,15 @@ function throwSandboxFault(vm: QuickJSContext, message: string): never {
  * for a sandbox fault there is no business message at all, so what reaches the
  * client is this text — the capability, the origin and the call that tripped
  * the gate — with the debug prefix removed.
+ *
+ * [#17265] Named for the OPERATION rather than for one of its callers, because
+ * it now has two: the same flattened prefix appears on the REJECTION path once
+ * a nested body's refusal stops being marked a fault, and the prefix belongs in
+ * the log there for exactly the same reason. One strip, two readers — the
+ * alternative was a second helper doing the same thing, which is the
+ * local-opinion shape this card exists to remove.
  */
-function sandboxFaultMessage(raw: string): string {
+function withoutSandboxErrorPrefix(raw: string): string {
   return raw.startsWith('SandboxError: ') ? raw.slice('SandboxError: '.length) : raw;
 }