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({