From e4db9931859626984dd1a0d0a02aa22ea12d15c0 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 12:21:51 +0000 Subject: [PATCH] fix(actions): an action that CRASHED is a 500, not a 200 reporting success:false (#3913 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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 puts a driver dump behind the internal-error-leak sanitiser (#3867) instead of letting it reach 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: a plain Error, a SandboxError carrying innerMessage, anything with code/fields, and a ValidationError by name are rejections; TypeError / ReferenceError / a driver's own class, and a SandboxError with no innerMessage (timeout, capability denial) are crashes. A throw with no name at all is not confidently a fault and keeps its 200 — deliberately the narrow direction. Also in the same exit: an error carrying its own status/statusCode (a plugin's FORBIDDEN with status 403) is served with it rather than buried in a 200 payload. Record ValidationErrors 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. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011AvZj6cLX7APd7roh2eK4F --- .changeset/action-crash-vs-rejection.md | 50 +++++ content/docs/api/error-catalog.mdx | 44 ++++ content/docs/ui/actions.mdx | 15 +- .../actions-fault-vs-rejection.test.ts | 207 ++++++++++++++++++ packages/runtime/src/domains/actions.ts | 73 ++++++ 5 files changed, 386 insertions(+), 3 deletions(-) create mode 100644 .changeset/action-crash-vs-rejection.md create mode 100644 packages/runtime/src/domains/actions-fault-vs-rejection.test.ts diff --git a/.changeset/action-crash-vs-rejection.md b/.changeset/action-crash-vs-rejection.md new file mode 100644 index 0000000000..0c894f4790 --- /dev/null +++ b/.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`. diff --git a/content/docs/api/error-catalog.mdx b/content/docs/api/error-catalog.mdx index 69d9946661..78116dd9e9 100644 --- a/content/docs/api/error-catalog.mdx +++ b/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`) + + +**`/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. + + +| 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: diff --git a/content/docs/ui/actions.mdx b/content/docs/ui/actions.mdx index 6a12b4a3fe..da777216a8 100644 --- a/content/docs/ui/actions.mdx +++ b/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, diff --git a/packages/runtime/src/domains/actions-fault-vs-rejection.test.ts b/packages/runtime/src/domains/actions-fault-vs-rejection.test.ts new file mode 100644 index 0000000000..a34d29c141 --- /dev/null +++ b/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'); + }); +}); diff --git a/packages/runtime/src/domains/actions.ts b/packages/runtime/src/domains/actions.ts index f483b7b04e..1282828ccf 100644 --- a/packages/runtime/src/domains/actions.ts +++ b/packages/runtime/src/domains/actions.ts @@ -34,9 +34,22 @@ * answers this route already gives a status (403 denied, 400 wrong type, * 503 unavailable). A handler that RAN and rejected still reports in the * payload, unchanged: that is a business outcome, not a transport error. + * + * Its follow-up separated a third case out of that payload: an UNEXPECTED + * FAULT. A `TypeError` in a handler or a driver blowing up is not an outcome + * the action chose to report, and serving it as 200 hid every handler crash + * from gateway error rates, retry policy, APM and alerting. Those are 500 now, + * told apart by the error's NAME — a plain `Error` (or a sandbox/validation + * marker) is a deliberate rejection and keeps the payload; a `TypeError` / + * `SqliteError` / driver class is a fault. + * + * So the route answers on two axes: + * did a handler run? no → status (404 / 403 / 400 / 503) + * did it reject or crash? reject → 200 + payload; crash → 500 */ import * as actionExec from '../action-execution.js'; +import { validationFailureDetails } from '../validation-failure.js'; import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js'; import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry.js'; @@ -323,6 +336,66 @@ export async function handleActionsRequest(deps: DomainHandlerDeps, path: string // "did a handler run": below it, the payload; above it, the status. const code: unknown = err?.code; const fields: unknown = err?.fields; + + // An error that NAMES its own HTTP status is asking to be served with + // it — a plugin's `FORBIDDEN` (status 403), a domain error from the + // protocol layer. Honour that first: burying an explicit 403 in a 200 + // payload discards the one thing the thrower was unambiguous about. + // Safe against #3937's cases by construction — a record + // `ValidationError` deliberately carries no `.status` (see + // `validation-failure.ts`), so it never reaches this branch. + if (typeof err?.status === 'number' || typeof err?.statusCode === 'number') { + return { handled: true, response: deps.errorFromThrown(err, 500) }; + } + + // [#3913 follow-up] …and it does not cover an UNEXPECTED FAULT either. + // "A failed action is a normal outcome" is a statement about the action + // REJECTING — a business rule saying no. A `TypeError` in a handler, a + // driver that blew up: those are not outcomes the action chose to + // report, they are the server failing to produce one. Reporting them as + // 200 makes them invisible to every layer that exists to catch server + // faults — gateway error rates, retry/circuit-breaker policy, APM + // auto-capture, alerting — so "customer action bodies are throwing" has + // no signal short of body-parsing at every hop. + // + // Told apart by the error's NAME, the same 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"): + // + // name === 'Error' a deliberate `throw new Error(msg)` — the + // shape a registered handler uses to reject. + // innerMessage present the sandbox's mark for "user code threw + // this deliberately" (SandboxError). + // code / fields present a structured domain failure — the + // ValidationError shape #3937 carries out. + // a ValidationError matched by `validationFailureDetails`, the + // same predicate the dispatcher's error exits + // use, so one whose `fields` were stripped in + // transit is still recognised by NAME alone. + // name absent an unrecognisable throw; NOT confidently a + // fault, so it keeps the status quo. + // any other name TypeError / ReferenceError / SqliteError / + // a driver's own class ⇒ a fault. + // + // Deliberately the narrow direction: this only moves what it is sure + // about, and everything it is unsure about keeps the 200 it has today. + // `errorFromThrown` is the same exit every other domain catch has used + // since #3925, and an error carrying its own `.status` still wins there + // so a hand-thrown 4xx keeps it. + const name: unknown = err?.name; + const unexpectedFault = + typeof name === 'string' + && name !== 'Error' + && !(typeof inner === 'string' && inner) + && !(typeof code === 'string' && code) + && !Array.isArray(fields) + && !validationFailureDetails(err); + if (unexpectedFault) { + console.error(`[action ${objectName}/${actionName}] unexpected fault (${name}): ${full}`); + return { handled: true, response: deps.errorFromThrown(err, 500) }; + } + return { handled: true, response: deps.success({