fix(actions): an action that CRASHED is a 500, not a 200 reporting success:false (#3913 follow-up)

.changeset/action-crash-vs-rejection.md

@@ -0,0 +1,50 @@
+---
+"@objectstack/runtime": minor
+---
+
+fix(actions): an action that CRASHED is a 500, not a 200 reporting success:false (#3913 follow-up)
+
+#3937 settled that a failed action reports in the payload at HTTP 200 — "an
+action that fails is a normal outcome, not a transport error". That is a
+statement about the action **rejecting**: a business rule saying no. The same
+exit was also covering a third case it never argued for.
+
+A `TypeError` in a handler, a driver blowing up, a sandbox timeout — those are
+not outcomes the action chose to report, they are the server failing to produce
+one. Serving them as 200 hid **every handler crash** from the layers that exist
+to catch server faults: gateway error rates, retry and circuit-breaker policy,
+APM auto-capture, alerting, `fetch().ok`. For a platform whose main extension
+surface is customer-authored script bodies, "customer action bodies are
+throwing" had no signal short of body-parsing at every hop.
+
+Those are **500** now, through the same `errorFromThrown` exit every other
+domain catch has used since #3925 — which also means a driver dump finally goes
+through the internal-error-leak sanitiser (#3867) instead of reaching the client
+verbatim in a 200 body.
+
+**Nothing #3937 put in the payload moves.** A rejection and a crash are told
+apart by the error's NAME, the signal `@objectstack/rest` already uses on this
+exact distinction ("non-default names (`TypeError: …`) […] signal a genuine
+script bug rather than a deliberately thrown business rule"):
+
+| Thrown | Verdict | Wire |
+|:---|:---|:---|
+| `new Error(msg)` — a registered handler rejecting | rejection | 200 + payload |
+| `SandboxError` with `innerMessage` — a body's deliberate throw | rejection | 200 + payload |
+| Anything carrying `code` / `fields`, or a `ValidationError` by name | rejection | 200 + payload |
+| A throw with no `name` at all | *not confidently a fault* | 200 + payload |
+| `TypeError` / `ReferenceError` / `SqliteError` / a driver's class | crash | **500** |
+| `SandboxError` with no `innerMessage` — timeout, capability denial | crash | **500** |
+
+Deliberately the narrow direction: only what is *certainly* a fault moves, and
+everything uncertain keeps the 200 it has today.
+
+One related fix in the same exit: an error carrying its own `status` /
+`statusCode` (a plugin's `FORBIDDEN` with `status: 403`) is now served with it
+rather than buried in a 200 payload — that status was the one thing the thrower
+was unambiguous about. Record `ValidationError`s deliberately carry no
+`.status`, so #3937's cases never reach that branch.
+
+Documented in `api/error-catalog.mdx` (new **Action Errors** section with the
+full status table and the two-check pattern a raw `fetch` caller needs) and
+`ui/actions.mdx`.

content/docs/api/error-catalog.mdx

@@ -349,6 +349,50 @@ ObjectStack uses a structured error system with **9 error categories** and **51
 
 ---
 
+## Action Errors (`/api/v1/actions`)
+
+<Callout type="warn">
+**`/actions` is the one route where a failure is not always a non-2xx.** An
+action that runs and *rejects* is a business outcome, so it is reported inside
+the payload at HTTP 200. Only failures where no handler produced that outcome
+carry a status. Branch on both.
+</Callout>
+
+| What happened | HTTP | Body |
+|:---|:---:|:---|
+| Action ran, returned | **200** | `{ success: true, data: { success: true, data } }` |
+| Action ran, **rejected** (business rule, validation) | **200** | `{ success: true, data: { success: false, error, code?, fields? } }` |
+| No such action registered | **404** | `{ success: false, error: { message, code } }` |
+| Caller lacks `requiredPermissions` | **403** | `{ success: false, error: { message, code } }` |
+| Type has no server dispatch (`url`/`modal`/`form`/`api`), or a param-contract violation | **400** | `{ success: false, error: { message, code } }` |
+| Data engine or automation service unavailable | **503** | `{ success: false, error: { message, code } }` |
+| Handler **crashed** (`TypeError`, driver error, sandbox timeout) | **500** | `{ success: false, error: { message, code } }` |
+
+A rejection and a crash are told apart by the thrown error: a plain
+`throw new Error(msg)`, a sandboxed body's deliberate throw, or a
+`ValidationError` is a rejection; a `TypeError` / `ReferenceError` / a driver's
+own error class is a crash. An error carrying its own `status` is served with
+it.
+
+```typescript
+const res = await fetch(`/api/v1/actions/${object}/${action}`, { /* … */ });
+const json = await res.json();
+
+// BOTH checks are required — neither one alone is sufficient.
+if (!res.ok) throw new Error(json.error?.message);             // never dispatched, or crashed
+if (json.data?.success === false) showToast(json.data.error);  // ran and rejected
+```
+
+Using `@objectstack/client` removes the footgun: `client.actions.invoke()`
+folds both shapes into one result and never throws.
+
+```typescript
+const res = await client.actions.invoke(object, action, { recordId, params });
+if (!res.success) showToast(res.error);
+```
+
+---
+
 ## Error Response Structure
 
 Every error response follows the `EnhancedApiError` schema:

content/docs/ui/actions.mdx

@@ -232,14 +232,23 @@ curl -b cookies.txt -X POST \
 # → 200 { "success": true, "data": { "success": true, "data": ... } }
 ```
 
-Failures split by **whether a handler ran**:
+Failures split three ways:
 
 - **It ran and rejected** — HTTP **200**, `data: { "success": false, "error",
-  "code"?, "fields"? }`. An action that fails is a normal business outcome, not
-  a transport error, so it rides the payload. Always branch on `data.success`.
+  "code"?, "fields"? }`. An action that says no is a normal business outcome,
+  not a transport error, so it rides the payload. Always branch on
+  `data.success`.
 - **It never dispatched** — a real status: **404** no such action, **403**
   denied, **400** wrong action type or a param-contract violation, **503**
   unavailable — with `{ "success": false, "error": { "message", "code" } }`.
+- **It crashed** — **500**. A `TypeError` in your handler, a driver error or a
+  sandbox timeout is not an outcome your action chose to report, so it is a
+  server fault and shows up as one in logs, alerting and gateway error rates.
+  A deliberate `throw new Error('…')` is a rejection, not a crash.
+
+Full table in the [error catalog](/docs/api/error-catalog). The
+[client SDK](/docs/api/client-sdk) folds all of it into one
+`{ success, data?, error? }` result.
 
 Global (object-less) actions post to `/api/v1/actions/global/:action`, or to
 `/api/v1/actions//:action` with the object segment left empty. For credentials,

packages/runtime/src/domains/actions-fault-vs-rejection.test.ts

@@ -0,0 +1,207 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * #3913 follow-up — an action that CRASHED is not an action that REJECTED.
+ *
+ * #3937 settled that a failed action reports in the payload at HTTP 200: "an
+ * action that fails is a normal outcome, not a transport error". That is a
+ * statement about the action **rejecting** — a business rule saying no. It was
+ * also covering a third case it never argued for: a `TypeError` in a handler,
+ * a driver blowing up, a sandbox timeout. Those are not outcomes the action
+ * chose to report; they are the server failing to produce one, and serving
+ * them as 200 hid every handler crash from gateway error rates, retry policy,
+ * APM auto-capture and alerting — the platform had no signal for "customer
+ * action bodies are throwing" short of body-parsing at every hop.
+ *
+ * The discriminator is the error's NAME, the same signal `@objectstack/rest`
+ * already uses on this distinction ("non-default names (`TypeError: …`) […]
+ * signal a genuine script bug rather than a deliberately thrown business
+ * rule"). This file pins BOTH sides, because the whole risk of the change is
+ * over-reach: everything #3937 put in the payload must stay there.
+ */
+
+import { describe, it, expect, vi } from 'vitest';
+
+import { HttpDispatcher } from '../http-dispatcher.js';
+import { SandboxError } from '../sandbox/quickjs-runner.js';
+
+const scriptAction = {
+    name: 'submit_signoff',
+    objectName: 'crm_invoice',
+    type: 'script',
+    body: { language: 'js', source: 'return 1;', capabilities: ['api.write'] },
+};
+
+/** A dispatcher whose action handler rejects with `thrown`. */
+function makeDispatcher(thrown: unknown) {
+    const objectDef = { name: 'crm_invoice', 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: 'inv_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 invoke(thrown: unknown) {
+    const res: any = await makeDispatcher(thrown).handleActions(
+        '/crm_invoice/submit_signoff/inv_1',
+        'POST',
+        {},
+        { request: {}, environmentId: 'platform', executionContext: { userId: 'u1', systemPermissions: [] } } as any,
+    );
+    return res.response;
+}
+
+describe('a deliberate REJECTION keeps the payload at HTTP 200', () => {
+    it('a plain `throw new Error(msg)` from a registered handler', async () => {
+        // The shape user code registered via `engine.registerAction` uses to
+        // reject. `name === 'Error'` is what marks it deliberate — this is the
+        // case a naive "no sandbox marker ⇒ fault" rule would have broken.
+        const response = await invoke(new Error('Lead is already converted'));
+
+        expect(response.status).toBe(200);
+        expect(response.body.data).toEqual({ success: false, error: 'Lead is already converted' });
+    });
+
+    it('a sandboxed body that threw on purpose (SandboxError.innerMessage)', async () => {
+        const response = await invoke(
+            new SandboxError("action 'submit_signoff' threw: Contact has no phone", 'Contact has no phone'),
+        );
+
+        expect(response.status).toBe(200);
+        expect(response.body.data).toEqual({ success: false, error: 'Contact has no phone' });
+    });
+
+    it('a record validation failure, code + fields intact (#3937)', async () => {
+        const fields = [{ field: 'issued_on', code: 'required', message: 'issued_on is required' }];
+        const response = await invoke(
+            new SandboxError(
+                "action 'submit_signoff' threw: ValidationError: issued_on is required",
+                'ValidationError: issued_on is required',
+                { code: 'VALIDATION_FAILED', fields },
+            ),
+        );
+
+        expect(response.status).toBe(200);
+        expect(response.body.data).toMatchObject({ success: false, code: 'VALIDATION_FAILED', fields });
+    });
+
+    it('a bare ValidationError whose fields were stripped in transit', async () => {
+        // Recognised by NAME via `validationFailureDetails` — the same predicate
+        // the dispatcher's error exits use — so losing `fields` downgrades the
+        // detail, never the classification.
+        const err: any = new Error('issued_on is required');
+        err.name = 'ValidationError';
+        const response = await invoke(err);
+
+        expect(response.status).toBe(200);
+        expect(response.body.data.success).toBe(false);
+    });
+
+    it('a flow that ran and rejected', async () => {
+        // `dispatchFlowAction` throws a plain Error, so it lands on the
+        // deliberate side with no special-casing.
+        const response = await invoke(new Error("Flow 'convert_wizard' failed: lead already converted"));
+
+        expect(response.status).toBe(200);
+        expect(response.body.data.success).toBe(false);
+        expect(response.body.data.error).toMatch(/lead already converted/);
+    });
+
+    it('an unrecognisable throw — no name at all — keeps the status quo', async () => {
+        // The change only moves what it is SURE about. A thrown string or a
+        // bare object is not confidently a fault, so it keeps its 200.
+        const response = await invoke('something odd');
+
+        expect(response.status).toBe(200);
+        expect(response.body.data.success).toBe(false);
+    });
+});
+
+describe('an unexpected FAULT is a 500', () => {
+    it('a TypeError from a buggy handler', async () => {
+        const response = await invoke(new TypeError("Cannot read properties of undefined (reading 'id')"));
+
+        expect(response.status).toBe(500);
+        expect(response.body.success).toBe(false);
+        // No `{success:true, data:{...}}` wrapper — this is the dispatcher's
+        // error exit, so monitoring sees a 5xx.
+        expect(response.body.data).toBeUndefined();
+    });
+
+    it('a ReferenceError from a buggy handler', async () => {
+        const response = await invoke(new ReferenceError('x is not defined'));
+
+        expect(response.status).toBe(500);
+    });
+
+    it("a driver's own error class", async () => {
+        const err: any = new Error('no such table: crm_invoice');
+        err.name = 'SqliteError';
+        const response = await invoke(err);
+
+        expect(response.status).toBe(500);
+    });
+
+    it('a driver dump the leak heuristic recognises is sanitised', async () => {
+        // Reaching the 5xx exit also puts these messages behind
+        // `looksLikeInternalErrorLeak` (#3867) — which the 200 payload never
+        // consulted, so a driver dump used to reach the client verbatim.
+        const err: any = new Error('UNIQUE constraint failed: crm_invoice.number');
+        err.name = 'SqliteError';
+        const response = await invoke(err);
+
+        expect(response.status).toBe(500);
+        expect(response.body.error.message).toBe('Internal server error');
+    });
+
+    it("the sandbox's OWN internal errors — a timeout, a capability denial", async () => {
+        // A `SandboxError` with no `innerMessage` is the sandbox failing, not
+        // user code rejecting: a timeout, a denied capability, a marshalling
+        // failure. Precisely the class an operator wants to alert on, and
+        // precisely what a 200 made invisible.
+        const response = await invoke(new SandboxError('action timed out after 5000ms'));
+
+        expect(response.status).toBe(500);
+    });
+
+    it('still honours an error that carries its own status', async () => {
+        // `errorFromThrown` reads `.status` first, so a hand-thrown 4xx is not
+        // flattened into the 500 fallback.
+        const err: any = new TypeError('Not allowed');
+        err.status = 403;
+        err.code = 'FORBIDDEN';
+        const response = await invoke(err);
+
+        expect(response.status).toBe(403);
+    });
+
+    it('honours an explicit status even on an otherwise-deliberate error', async () => {
+        // A plugin's `FORBIDDEN` is a plain Error carrying `status: 403` — the
+        // "deliberate" side by name, but burying an explicit 403 in a 200
+        // payload would discard the one thing the thrower was unambiguous
+        // about, so `.status` is checked first.
+        const err: any = new Error('Record is outside your sharing scope');
+        err.status = 403;
+        err.code = 'FORBIDDEN';
+        const response = await invoke(err);
+
+        expect(response.status).toBe(403);
+        expect(response.body.error.message).toBe('Record is outside your sharing scope');
+        expect(response.body.error.details?.code).toBe('FORBIDDEN');
+    });
+});