From cf83620392e7dc8daf2d899e8a3a0c515b4a1f85 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 00:07:10 +0000 Subject: [PATCH] =?UTF-8?q?fix(actions)!:=20failures=20speak=20HTTP=20?= =?UTF-8?q?=E2=80=94=20rejections=20are=20400,=20success=20is=20a=20single?= =?UTF-8?q?=20wrap=20(#3962)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 200-with-inner-envelope wire was never a designed contract: no ADR or doc specified it, it originated as the catch block reusing deps.success(), and /actions was the only route of 12 that double-wrapped. #3962 (platform decision: third-party-facing API, normalized results) classifies it as a bug. Contract now, identical to /data: 200 single wrap (data = handler return value); deliberate rejection (body throw / flow rejection / ValidationError) 400 via errorFromThrown with code/fields in details; dispatch failures 404/403/400/503 and crashes 500 unchanged (#3930/#3951). A rejected flow regains code FLOW_FAILED at 400. The #3951 name discriminator now selects 400 vs 500. actions-validation-envelope.test.ts is flipped citing #3962 — the previous revision pinned 200 explicitly so that flipping it would be "a conscious, documented decision, not a side effect"; #3962 is that decision. client.actions.invoke/invokeGlobal still never throw: they fold every failure status into {success:false, error}, read the single wrap on success, and keep a NARROW legacy heuristic (boolean success + no foreign keys) so a current SDK still folds pre-#3962 double-wrapped 200s. Migration for raw-HTTP callers: branch on the status; on 200, data is the handler's return value directly. SDK callers need no change. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_011AvZj6cLX7APd7roh2eK4F --- .changeset/actions-failures-speak-http.md | 41 +++++++++ content/docs/api/client-sdk.mdx | 6 +- content/docs/api/error-catalog.mdx | 30 +++---- content/docs/ui/actions.mdx | 25 +++--- packages/client/src/actions-surface.test.ts | 49 +++++++--- packages/client/src/index.ts | 45 +++++----- packages/runtime/src/action-execution.ts | 13 +-- .../actions-fault-vs-rejection.test.ts | 41 +++++---- .../actions-validation-envelope.test.ts | 44 ++++----- packages/runtime/src/domains/actions.ts | 89 +++++++++---------- ...http-dispatcher.actions-global-key.test.ts | 52 +++++------ ...p-dispatcher.actions-type-dispatch.test.ts | 13 +-- 12 files changed, 253 insertions(+), 195 deletions(-) create mode 100644 .changeset/actions-failures-speak-http.md diff --git a/.changeset/actions-failures-speak-http.md b/.changeset/actions-failures-speak-http.md new file mode 100644 index 0000000000..d3fd68dfea --- /dev/null +++ b/.changeset/actions-failures-speak-http.md @@ -0,0 +1,41 @@ +--- +"@objectstack/runtime": major +"@objectstack/client": minor +--- + +fix(actions)!: failures speak HTTP — business rejections are 400, success is a single wrap (#3962) + +**BREAKING (raw-HTTP callers of `POST /api/v1/actions/...` only).** The +200-with-inner-envelope wire was never a designed contract: no ADR or doc ever +specified it, it originated as the route's catch block reusing +`deps.success()`, and `/actions` was the only route of 12 that double-wrapped. +#3962 classifies it as a bug. Five defects traced back to that one extra layer +(the console's green toast on failed actions, `redirectUrl` never firing, a +marketplace install reported as installed when it failed, the client-envelope +divergence #3927 papered over, and crashes invisible to monitoring). + +The contract now, identical to `/data`: + +| Outcome | HTTP | Body | +|:---|:---:|:---| +| Ran, returned | **200** | `{success: true, data: }` — single wrap | +| Ran, rejected (business rule / validation) | **400** | `{success: false, error: {message, code, details: {code?, fields?}}}` | +| Never dispatched (unknown / denied / wrong type / unavailable) | 404 / 403 / 400 / 503 | unchanged (#3930/#3951) | +| Crashed (`TypeError`, driver class, sandbox timeout) | **500** | unchanged (#3951) | + +A validation rejection carries `details.code: 'VALIDATION_FAILED'` and +`details.fields[]` — the exact payload #3937 fought for, now on the same wire +shape `/data` has always used, which `@objectstack/client` normalizes to +`err.code` / `err.fields` (#3927). A rejected flow is a 400 with +`details.code: 'FLOW_FAILED'`. The crash-vs-rejection discriminator (#3951, +error `name`) now selects 400 vs 500. + +`client.actions.invoke` / `invokeGlobal` still never throw: they fold every +failure status into `{success: false, error}`, read the single wrap on +success, and keep a NARROW legacy heuristic so a current SDK talking to a +pre-#3962 server still folds the old double-wrapped 200s correctly. + +**Migration for raw-HTTP third parties:** branch on the HTTP status — a +non-2xx is the failure, `error.message` / `error.details` carry the detail; on +a 200, `data` is the handler's return value directly (one level less than +before). Callers using `@objectstack/client` need no change. diff --git a/content/docs/api/client-sdk.mdx b/content/docs/api/client-sdk.mdx index dad5eabba5..950d52bd45 100644 --- a/content/docs/api/client-sdk.mdx +++ b/content/docs/api/client-sdk.mdx @@ -394,9 +394,9 @@ await client.automation.getScreen('convert_lead', runId); // (engine.registerAction). The result is `{ success, data | error }` — a // failure comes back as `success: false` + `error` instead of a thrown // exception, so it can be surfaced as a toast. This surface is the one place -// a non-2xx does NOT throw: a handler that rejects answers 200 with the inner -// envelope, a request that never dispatched answers 404 / 403 / 400 / 503, -// and the SDK folds both into the same result. +// a non-2xx does NOT throw: failures speak HTTP (400 rejection, 404 / 403 / +// 503 dispatch failures, 500 crash) and the SDK folds them into the result; +// on success, `data` is the handler's return value directly. const res = await client.actions.invoke('crm_lead', 'convert', { recordId, params: { create_opportunity: true }, diff --git a/content/docs/api/error-catalog.mdx b/content/docs/api/error-catalog.mdx index 78116dd9e9..e9366c8738 100644 --- a/content/docs/api/error-catalog.mdx +++ b/content/docs/api/error-catalog.mdx @@ -351,17 +351,14 @@ 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. - +Since #3962 `/actions` failures speak HTTP like every other route — the status +code is the failure signal, and on success `data` is the handler's return +value directly (single wrap). | 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? } }` | +| Action ran, returned | **200** | `{ success: true, data: }` | +| Action ran, **rejected** (business rule, validation) | **400** | `{ success: false, error: { message, code, details: { 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 } }` | @@ -370,21 +367,20 @@ carry a status. Branch on both. 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. +`ValidationError` is a rejection (400); a `TypeError` / `ReferenceError` / a +driver's own error class is a crash (500). An error carrying its own `status` +is served with it. A validation rejection carries `details.code: +'VALIDATION_FAILED'` and `details.fields[]`, identical to `/data`. ```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 +if (!res.ok) showToast(json.error?.message); // the status IS the failure signal +else use(json.data); // the handler's return value ``` -Using `@objectstack/client` removes the footgun: `client.actions.invoke()` -folds both shapes into one result and never throws. +`client.actions.invoke()` folds this (and the pre-#3962 legacy 200 envelope) +into one `{ success, data?, error? }` result and never throws. ```typescript const res = await client.actions.invoke(object, action, { recordId, params }); diff --git a/content/docs/ui/actions.mdx b/content/docs/ui/actions.mdx index da777216a8..3df45afb28 100644 --- a/content/docs/ui/actions.mdx +++ b/content/docs/ui/actions.mdx @@ -229,22 +229,19 @@ curl -b cookies.txt -X POST \ https://your-app.example.com/api/v1/actions/todo_task/complete_task \ -H "Content-Type: application/json" \ -d '{ "recordId": "rec_123", "params": {} }' -# → 200 { "success": true, "data": { "success": true, "data": ... } } +# → 200 { "success": true, "data": } ``` -Failures split three ways: - -- **It ran and rejected** — HTTP **200**, `data: { "success": false, "error", - "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. +Failures speak HTTP — the status code is the signal (#3962): + +- **400** — the action ran and **rejected** (a business rule said no, or + validation failed — then with `error.details.fields[]` to anchor the input). +- **404 / 403 / 503** — it never dispatched: no such action, denied, service + unavailable. A `url`/`modal`/`form`/`api` type with no server dispatch is + also a 400. +- **500** — it **crashed**: a `TypeError` in your handler, a driver error, a + sandbox timeout. A deliberate `throw new Error('…')` is a 400 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 diff --git a/packages/client/src/actions-surface.test.ts b/packages/client/src/actions-surface.test.ts index 730117f4f2..5763b233b9 100644 --- a/packages/client/src/actions-surface.test.ts +++ b/packages/client/src/actions-surface.test.ts @@ -28,7 +28,7 @@ describe('client.actions', () => { it('invoke POSTs to /api/v1/actions/:object/:action with recordId + params in the body', async () => { const { client, fetchMock } = createMockClient({ success: true, - data: { success: true, data: { converted: true } }, + data: { converted: true }, // #3962: single wrap — data IS the handler value }); const result = await client.actions.invoke('crm_lead', 'convert', { @@ -71,11 +71,10 @@ describe('client.actions', () => { expect(JSON.parse(fetchMock.mock.calls[0][1].body)).toEqual({ params: { dryRun: true } }); }); - it('surfaces the handler business failure without throwing (inner success:false)', async () => { - // A handler that RAN and rejected is a business outcome, reported in - // the payload at HTTP 200 — unchanged by #3913, which only moved the - // DISPATCH failures to a status. The inner envelope is authoritative - // on a 2xx. + it('still folds a PRE-#3962 server\'s 200-wrapped failure (legacy inner envelope)', async () => { + // Servers older than #3962 reported a rejection as HTTP 200 with the + // inner `{success:false, error}`. A current SDK still talks to them, + // so the narrowly-detected legacy shape stays authoritative on a 2xx. const { client } = createMockClient({ success: true, data: { success: false, error: 'Lead is already converted' }, @@ -86,12 +85,9 @@ describe('client.actions', () => { }); it('reports a dispatch failure (404) as the same non-throwing envelope (#3913)', async () => { - // An unregistered action never dispatched, so the server answers a real - // status with `{success:false, error:{message,code}}`. `client.fetch` - // throws on any non-2xx, but this surface's contract is to REPORT, not - // throw — callers toast `result.error` rather than wrapping every call - // in a try/catch, and that must not change just because these routes - // gained a status. + // `client.fetch` throws on any non-2xx, but this surface's contract is + // to REPORT, not throw — callers toast `result.error` rather than + // wrapping every call in a try/catch. const { client } = createMockClient( { success: false, error: { message: "Action 'log_call' on object 'global' not found", code: 404 } }, 404, @@ -103,6 +99,35 @@ describe('client.actions', () => { expect(result.error).toBe("Action 'log_call' on object 'global' not found"); }); + it('reports a business rejection (400) with the message, non-throwing (#3962)', async () => { + const { client } = createMockClient( + { + success: false, + error: { + message: 'ValidationError: issued_on is required', + code: 400, + details: { code: 'VALIDATION_FAILED', fields: [{ field: 'issued_on' }] }, + }, + }, + 400, + ); + const result = await client.actions.invoke('crm_invoice', 'submit_signoff', { recordId: 'inv_1' }); + expect(result.success).toBe(false); + expect(result.error).toBe('ValidationError: issued_on is required'); + }); + + it('passes a handler value that merely CONTAINS success-ish keys through untouched (#3962)', async () => { + // Single wrap means data is handler-owned. Only the exact legacy + // envelope shape (boolean `success`, no foreign keys) is unwrapped. + const { client } = createMockClient({ + success: true, + data: { success: true, rows: 3, data: { id: 'x' }, extra: 'mine' }, + }); + const result = await client.actions.invoke('crm_lead', 'sync'); + expect(result.success).toBe(true); + expect(result.data).toEqual({ success: true, rows: 3, data: { id: 'x' }, extra: 'mine' }); + }); + it('reports a 403 denial the same way', async () => { const { client } = createMockClient( { success: false, error: { message: 'Missing capability can_convert', code: 403 } }, diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index b520ac68a5..2191439965 100644 --- a/packages/client/src/index.ts +++ b/packages/client/src/index.ts @@ -323,20 +323,28 @@ function actionErrorMessage(e: any): string { * Fold a 2xx `POST /api/v1/actions/...` body into the `{ success, data?, * error? }` shape `client.actions.invoke` has always returned. * - * `payload` is what `unwrapResponse` produced — the INNER envelope. Two shapes - * arrive on a 2xx and a caller must not have to tell them apart: + * `payload` is what `unwrapResponse` produced. On a #3962 server a 2xx means + * the action ran and returned, and `payload` IS the handler's return value — + * every failure (rejection 400, dispatch 404/403/503, crash 500) arrives as a + * non-2xx, `fetch` throws on it, and `invoke` catches. This surface does not + * throw either way. * - * - `{ success: true, data }` — the action ran and returned. - * - `{ success: false, error, code?, fields? }` — the handler RAN and - * rejected. That is a business outcome, so it rides the payload at HTTP - * 200 (#3937) and the inner envelope is authoritative. - * - * The non-2xx case never reaches here: those are DISPATCH failures (404 / 403 - * / 400 / 503), `fetch` throws on them, and `invoke` catches — this surface - * does not throw either way. + * Servers older than #3962 answered a 2xx with the legacy INNER envelope — + * `{success: true, data}` on success, `{success: false, error, code?, + * fields?}` when the handler rejected. A current SDK still has to talk to + * them, so that shape is detected NARROWLY (a `boolean` `success` and no keys + * beyond the envelope's own) and unwrapped; anything else passes through as + * the handler's data. The residual ambiguity — a handler whose own return + * value is exactly envelope-shaped — is unavoidable while both servers exist + * and resolves once the fleet is on #3962. */ function normalizeActionResult(payload: any): { success: boolean; data?: T; error?: string } { - if (payload && typeof payload === 'object' && 'success' in payload) { + const ENVELOPE_KEYS = new Set(['success', 'data', 'error', 'code', 'fields']); + const legacy = + payload && typeof payload === 'object' && !Array.isArray(payload) + && typeof payload.success === 'boolean' + && Object.keys(payload).every((k) => ENVELOPE_KEYS.has(k)); + if (legacy) { return payload.success === false ? { success: false, error: actionErrorMessage(payload.error) } : { success: true, data: payload.data as T }; @@ -2916,15 +2924,12 @@ export class ObjectStackClient { * Result shape is `{ success, data | error }` and this surface does NOT * throw — a failed business action is a toast, not a crash. * - * Two server answers fold into that one result: - * - a handler that RAN and rejected → HTTP 200 with the inner - * `{success: false, error}` (unchanged — a business outcome is reported - * in the payload, not as a transport error); - * - a request that never DISPATCHED → a real status (404 unregistered, 403 - * denied, 400 wrong action type, 503 unavailable) with - * `{success: false, error: {message, code}}`. `fetch` throws on those, so - * `invoke` catches and reports instead of propagating — #3913, where an - * unregistered action came back as a 200 and read as a success. + * Since #3962 every failure speaks HTTP: 400 rejection (with `code` / + * `fields` in `error.details`), 404 unregistered, 403 denied, 503 + * unavailable, 500 crash. `fetch` throws on any non-2xx, so `invoke` + * catches and folds it into this result instead of propagating. On success, + * `data` is the handler's return value directly (single wrap); the legacy + * double-wrapped 200s of pre-#3962 servers are still recognised and folded. */ actions = { /** diff --git a/packages/runtime/src/action-execution.ts b/packages/runtime/src/action-execution.ts index 4e51e7838e..0a4c7b2a22 100644 --- a/packages/runtime/src/action-execution.ts +++ b/packages/runtime/src/action-execution.ts @@ -429,11 +429,14 @@ export async function dispatchFlowAction(deps: ActionExecutionDeps, params: seedFlowActionParams(deps, action, { objectName, record, params, recordId }), }); if (result && typeof result === 'object' && 'success' in result && result.success === false) { - // The flow RAN and rejected — a business outcome, so the REST route - // reports it in the payload (`{success: false, error}`, HTTP 200), the - // same as a script body that throws. Only a route that never dispatched - // gets a status (#3913). - throw new Error(`Flow '${action.target}' failed: ${result.error ?? 'unknown error'}`); + // The flow RAN and rejected — a deliberate business rejection, served + // as a 400 (#3962). Tagging the status/code here (rather than relying + // on the route's name heuristic) keeps the semantic `FLOW_FAILED` on + // the wire for callers that branch on `err.code`. + const err: any = new Error(`Flow '${action.target}' failed: ${result.error ?? 'unknown error'}`); + err.status = 400; + err.code = 'FLOW_FAILED'; + throw err; } return result ?? null; } diff --git a/packages/runtime/src/domains/actions-fault-vs-rejection.test.ts b/packages/runtime/src/domains/actions-fault-vs-rejection.test.ts index a34d29c141..0c973fab9e 100644 --- a/packages/runtime/src/domains/actions-fault-vs-rejection.test.ts +++ b/packages/runtime/src/domains/actions-fault-vs-rejection.test.ts @@ -16,8 +16,11 @@ * 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. + * rule"). Since #3962 both sides are HTTP statuses — a rejection is a 400 + * carrying `code`/`fields` in `details`, a crash is a 500 — and this file pins + * the LINE between them, because the risk is misclassification in either + * direction: a business rejection served as a 500 gets its message eaten by + * the leak sanitiser; a crash served as a 400 hides from alerting. */ import { describe, it, expect, vi } from 'vitest'; @@ -66,15 +69,15 @@ async function invoke(thrown: unknown) { return res.response; } -describe('a deliberate REJECTION keeps the payload at HTTP 200', () => { +describe('a deliberate REJECTION is a 400 (#3962)', () => { 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' }); + expect(response.status).toBe(400); + expect(response.body.error.message).toBe('Lead is already converted'); }); it('a sandboxed body that threw on purpose (SandboxError.innerMessage)', async () => { @@ -82,8 +85,9 @@ describe('a deliberate REJECTION keeps the payload at HTTP 200', () => { 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' }); + expect(response.status).toBe(400); + // The sandbox debug wrapper stays in the server log, not on the wire. + expect(response.body.error.message).toBe('Contact has no phone'); }); it('a record validation failure, code + fields intact (#3937)', async () => { @@ -96,8 +100,8 @@ describe('a deliberate REJECTION keeps the payload at HTTP 200', () => { ), ); - expect(response.status).toBe(200); - expect(response.body.data).toMatchObject({ success: false, code: 'VALIDATION_FAILED', fields }); + expect(response.status).toBe(400); + expect(response.body.error.details).toMatchObject({ code: 'VALIDATION_FAILED', fields }); }); it('a bare ValidationError whose fields were stripped in transit', async () => { @@ -108,8 +112,8 @@ describe('a deliberate REJECTION keeps the payload at HTTP 200', () => { err.name = 'ValidationError'; const response = await invoke(err); - expect(response.status).toBe(200); - expect(response.body.data.success).toBe(false); + expect(response.status).toBe(400); + expect(response.body.error.details?.code).toBe('VALIDATION_FAILED'); }); it('a flow that ran and rejected', async () => { @@ -117,18 +121,17 @@ describe('a deliberate REJECTION keeps the payload at HTTP 200', () => { // 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/); + expect(response.status).toBe(400); + expect(response.body.error.message).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. + it('an unrecognisable throw — no name at all — reads as a rejection, not a crash', async () => { + // A thrown string or a bare object is not confidently a server fault, + // so it takes the rejection exit (400), not the 500. const response = await invoke('something odd'); - expect(response.status).toBe(200); - expect(response.body.data.success).toBe(false); + expect(response.status).toBe(400); + expect(response.body.success).toBe(false); }); }); diff --git a/packages/runtime/src/domains/actions-validation-envelope.test.ts b/packages/runtime/src/domains/actions-validation-envelope.test.ts index aca5c4f8e2..df4fef4f0f 100644 --- a/packages/runtime/src/domains/actions-validation-envelope.test.ts +++ b/packages/runtime/src/domains/actions-validation-envelope.test.ts @@ -1,7 +1,7 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * #3918 follow-up — the LAST hop: what `/actions` puts on the wire. + * #3918 follow-up + #3962 — the LAST hop: what `/actions` puts on the wire. * * The sandbox now carries `code` / `fields` back out of the VM on a * `SandboxError` (see `sandbox/error-passthrough.test.ts`). This pins the other @@ -11,12 +11,12 @@ * action could raise a toast but never highlight the offending input — the * symptom in the original report. * - * **The status stays 200.** `/actions` has always reported business failure in - * the payload (`{ success: false }`), not as a transport error, and every - * caller branches on `data.success`. Making this a 4xx would be a wire-contract - * break in exchange for a strictly additive fix, so the fix is additive: the - * same envelope, now carrying what the client needs. That choice is asserted - * below so it cannot drift silently. + * **The status is 400 since #3962** — the conscious, documented decision the + * previous revision of this file asked for. The 200-with-inner-envelope wire + * was never designed: no ADR or doc specified it, and /actions was the only + * route of 12 that double-wrapped. `code` and `fields` now ride in + * `error.details`, the exact shape `@objectstack/client` normalizes to + * `err.code` / `err.fields` (#3927), identical to a ValidationError on /data. */ import { describe, it, expect, vi } from 'vitest'; @@ -81,8 +81,8 @@ describe('#3918 follow-up — /actions surfaces code + fields on a validation fa it('carries `fields[]` so a form can anchor the error to the input', async () => { const response = await invoke(validationSandboxError()); - expect(response.body.data).toMatchObject({ - success: false, + expect(response.status).toBe(400); + expect(response.body.error.details).toMatchObject({ code: 'VALIDATION_FAILED', fields: FIELDS, }); @@ -91,17 +91,17 @@ describe('#3918 follow-up — /actions surfaces code + fields on a validation fa it('keeps the human message as the toast text', async () => { const response = await invoke(validationSandboxError()); - expect(response.body.data.error).toBe('ValidationError: issued_on is required'); + expect(response.body.error.message).toBe('ValidationError: issued_on is required'); }); - it('KEEPS HTTP 200 and `success: false` — the wire contract is unchanged', async () => { - // Deliberate: see the file header. If this flips to 4xx it must be a - // conscious, documented break, not a side effect. + it('is an HTTP 400 — failures speak HTTP (#3962)', async () => { + // The previous revision of this test pinned 200 so that flipping it + // would be "a conscious, documented break, not a side effect". + // #3962 is that documented decision. const response = await invoke(validationSandboxError()); - expect(response.status).toBe(200); - expect(response.body.success).toBe(true); // dispatcher envelope - expect(response.body.data.success).toBe(false); // business outcome + expect(response.status).toBe(400); + expect(response.body.success).toBe(false); }); it('omits `code` / `fields` entirely for an ordinary failure', async () => { @@ -111,9 +111,9 @@ describe('#3918 follow-up — /actions surfaces code + fields on a validation fa new SandboxError("action 'x' threw: boom", 'boom'), ); - expect(response.body.data).toEqual({ success: false, error: 'boom' }); - expect('code' in response.body.data).toBe(false); - expect('fields' in response.body.data).toBe(false); + expect(response.status).toBe(400); + expect(response.body.error.message).toBe('boom'); + expect(response.body.error.details).toBeUndefined(); }); it('carries a code without fields when that is all the error had', async () => { @@ -121,8 +121,8 @@ describe('#3918 follow-up — /actions surfaces code + fields on a validation fa new SandboxError("action 'x' threw: pick another", 'pick another', { code: 'DUPLICATE' }), ); - expect(response.body.data).toEqual({ - success: false, error: 'pick another', code: 'DUPLICATE', - }); + expect(response.status).toBe(400); + expect(response.body.error.message).toBe('pick another'); + expect(response.body.error.details).toMatchObject({ code: 'DUPLICATE' }); }); }); diff --git a/packages/runtime/src/domains/actions.ts b/packages/runtime/src/domains/actions.ts index 1282828ccf..b329b65870 100644 --- a/packages/runtime/src/domains/actions.ts +++ b/packages/runtime/src/domains/actions.ts @@ -32,20 +32,20 @@ * caller that did not hand-inspect the INNER envelope read it as a success. * Nothing DISPATCHED there, so it is a 404 now — joining the pre-dispatch * 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. + * 503 unavailable). * - * 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. + * #3962 finished the unification: FAILURES SPEAK HTTP, exactly as /data always + * has. The old 200-with-inner-`{success:false}` wire was never a designed + * contract — no ADR or doc specified it; it was the catch block reusing + * `deps.success()`, and /actions was the only route of 12 that double-wrapped. + * Success is now a SINGLE wrap (`data` = the handler's return value), a + * deliberate rejection (body `throw`, flow rejection, ValidationError) is a + * 400 carrying `code`/`fields` in `details`, and a crash (`TypeError`, driver + * class, sandbox timeout — told apart by the error's NAME, #3951) is a 500. * - * 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 + * did a handler run? no → 404 / 403 / 400 / 503 + * did it reject or crash? reject → 400; crash → 500 + * did it return? 200, `data` = handler return value */ import * as actionExec from '../action-execution.js'; @@ -266,7 +266,11 @@ export async function handleActionsRequest(deps: DomainHandlerDeps, path: string ec, envId: _context?.environmentId, }); - return { handled: true, response: deps.success({ success: true, data: result }) }; + // [#3962] Single wrap: `data` is the handler's return value, exactly as + // every other domain serializes. The former inner `{success, data}` + // envelope existed only to carry a failure signal at HTTP 200; failures + // carry a status now, so the extra layer lost its job. + return { handled: true, response: deps.success(result) }; } // [#2849] Same trusted-mode elevation as the MCP path — keep it audible. @@ -304,7 +308,11 @@ export async function handleActionsRequest(deps: DomainHandlerDeps, path: string response: deps.error(`Action '${actionName}' on object '${objectName}' not found`, 404), }; } - return { handled: true, response: deps.success({ success: true, data: result }) }; + // [#3962] Single wrap: `data` is the handler's return value, exactly as + // every other domain serializes. The former inner `{success, data}` + // envelope existed only to carry a failure signal at HTTP 200; failures + // carry a status now, so the extra layer lost its job. + return { handled: true, response: deps.success(result) }; } catch (err: any) { const full = err?.message ?? String(err); // The sandbox wraps a user throw as ` '' threw: ` for @@ -313,27 +321,12 @@ export async function handleActionsRequest(deps: DomainHandlerDeps, path: string // leaking the debug prefix. Keep the full wrapper in the log for debugging. const inner: unknown = err?.innerMessage; const clientMsg = (typeof inner === 'string' && inner) ? inner : full; - if (clientMsg !== full) console.error(`[action ${objectName}/${actionName}] ${full}`); - // [#3918 follow-up] Carry the structured payload when the failure was a - // record validation error. Until the sandbox learned to pass `code` / - // `fields` back OUT of the VM, this envelope was the message string and - // nothing else — so a form action could only ever raise a toast, never - // highlight the field the user actually got wrong, which is the symptom - // the original report was about. - // - // The HTTP status stays 200 and `success: false` remains the failure - // signal. `/actions` has always reported business failure in the payload - // (an action that "fails" is a normal outcome, not a transport error) and - // every existing caller branches on `data.success`; turning this into a - // 4xx would be a wire-contract break in exchange for a strictly additive - // fix. - // - // [#3913] That reasoning covers a handler that RAN and rejected — it - // does NOT cover a route that never dispatched at all. An unregistered - // action has no business outcome to report, so it exits above as a 404, - // alongside the pre-dispatch gates this route already answers with a - // status (403 denied, 400 wrong type, 503 unavailable). The line is - // "did a handler run": below it, the payload; above it, the status. + if (clientMsg !== full) { + console.error(`[action ${objectName}/${actionName}] ${full}`); + // Every exit below reads `.message`; hand it the client-safe text so + // the debug wrapper stays in the log and off the wire. + try { err.message = clientMsg; } catch { /* frozen error */ } + } const code: unknown = err?.code; const fields: unknown = err?.fields; @@ -373,13 +366,12 @@ export async function handleActionsRequest(deps: DomainHandlerDeps, path: string // 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. + // name absent an unrecognisable throw; treated as a + // rejection (the caller-fault reading), not + // a server fault. // 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. @@ -396,14 +388,15 @@ export async function handleActionsRequest(deps: DomainHandlerDeps, path: string return { handled: true, response: deps.errorFromThrown(err, 500) }; } - return { - handled: true, - response: deps.success({ - success: false, - error: clientMsg, - ...(typeof code === 'string' && code ? { code } : {}), - ...(Array.isArray(fields) ? { fields } : {}), - }), - }; + // [#3962] A deliberate REJECTION is a 400. The 200-with-inner-envelope + // wire this used to emit was never a designed contract — no ADR or doc + // specified it, it was the catch block reusing `deps.success()`, and + // /actions was the only route of 12 that double-wrapped. The platform + // decision in #3962 classifies it as a bug: failures speak HTTP, same + // as /data has always done. `errorFromThrown` carries the structured + // payload #3937 fought for — `VALIDATION_FAILED` + `fields[]` land in + // `details`, the exact shape @objectstack/client normalizes to + // `err.code` / `err.fields` (#3927). + return { handled: true, response: deps.errorFromThrown(err, 400) }; } } diff --git a/packages/runtime/src/http-dispatcher.actions-global-key.test.ts b/packages/runtime/src/http-dispatcher.actions-global-key.test.ts index 7254b3b053..1e78087c5e 100644 --- a/packages/runtime/src/http-dispatcher.actions-global-key.test.ts +++ b/packages/runtime/src/http-dispatcher.actions-global-key.test.ts @@ -23,13 +23,10 @@ * did not hand-unwrap the inner envelope (the shipped console among them, * which showed a green toast) swallowed it. * - * Only the DISPATCH FAILURE moved to a status. A handler that RAN and - * rejected still reports `{success: false, error, code?, fields?}` at HTTP - * 200 — that is a business outcome, not a transport error, and #3937 pins - * it (`actions-validation-envelope.test.ts`). The line this file asserts is - * "did a handler run": below it the payload, above it the status, alongside - * the pre-dispatch answers the route already gives one (403 denied, 400 - * wrong type, 503 unavailable). + * #3913 moved the dispatch failure to a 404; #3962 finished the job — + * every failure speaks HTTP (rejection 400, crash 500), success is a + * SINGLE wrap (`data` = the handler's return value), and the inner + * envelope is gone. * * The engine double here is deliberately faithful on the one point that * matters: a real `Map` keyed `:` that throws @@ -90,7 +87,7 @@ describe('REST /actions — object-less ("global") action key (#3913)', () => { const res = await dispatcher.handleActions('/global/log_call', 'POST', {}, ctx()); expect(res.response.status).toBe(200); - expect(res.response.body.data).toEqual({ success: true, data: { logged: true } }); + expect(res.response.body.data).toEqual({ logged: true }); }); it('reaches the same handler from POST /actions//:action — the empty-object shape', async () => { @@ -104,7 +101,7 @@ describe('REST /actions — object-less ("global") action key (#3913)', () => { const res = await dispatcher.handleActions('//log_call', 'POST', {}, ctx()); expect(res.response.status).toBe(200); - expect(res.response.body.data).toEqual({ success: true, data: { logged: true } }); + expect(res.response.body.data).toEqual({ logged: true }); expect(executeAction).toHaveBeenCalledWith('global', 'log_call', expect.anything()); }); @@ -119,7 +116,7 @@ describe('REST /actions — object-less ("global") action key (#3913)', () => { const res = await dispatcher.handleActions('/crm_contact/log_call', 'POST', {}, ctx()); expect(res.response.status).toBe(200); - expect(res.response.body.data).toEqual({ success: true, data: { logged: true } }); + expect(res.response.body.data).toEqual({ logged: true }); expect(executeAction).toHaveBeenNthCalledWith(1, 'crm_contact', 'log_call', expect.anything()); expect(executeAction).toHaveBeenNthCalledWith(2, 'global', 'log_call', expect.anything()); }); @@ -133,7 +130,7 @@ describe('REST /actions — object-less ("global") action key (#3913)', () => { const res = await dispatcher.handleActions('/crm_contact/log_call', 'POST', {}, ctx()); expect(res.response.status).toBe(200); - expect(res.response.body.data).toEqual({ success: true, data: { logged: 'wildcard' } }); + expect(res.response.body.data).toEqual({ logged: 'wildcard' }); }); it('probes `global` exactly once when the route already names it', async () => { @@ -156,7 +153,7 @@ describe('REST /actions — object-less ("global") action key (#3913)', () => { const res = await dispatcher.handleActions('/crm_contact/log_call', 'POST', {}, ctx()); - expect(res.response.body.data).toEqual({ success: true, data: { scope: 'object' } }); + expect(res.response.body.data).toEqual({ scope: 'object' }); }); it('does not try to load a record for an object-less action', async () => { @@ -167,7 +164,7 @@ describe('REST /actions — object-less ("global") action key (#3913)', () => { const res = await dispatcher.handleActions('/global/log_call/rec_1', 'POST', {}, ctx()); // No `get` against an object named 'global' — only the id echo. - expect(res.response.body.data.data.record).toEqual({ id: 'rec_1' }); + expect(res.response.body.data.record).toEqual({ id: 'rec_1' }); }); it('404s an unregistered action, naming the ROUTED object rather than the last probe', async () => { @@ -210,12 +207,9 @@ describe('REST /actions — object-less ("global") action key (#3913)', () => { }); describe('REST /actions — dispatch failure vs business failure (#3913)', () => { - it('a handler that RAN and rejected still reports in the payload at HTTP 200', async () => { - // The line #3913 draws is "did a handler run". This one did, so the - // failure is a business outcome and stays in the envelope — the - // contract #3937 pins. Asserted here too so the not-found 404 above - // can never be widened into "every action failure is a 4xx" by - // someone reading only this file. + it('a handler that RAN and rejected is a 400 with the business message (#3962)', async () => { + // Distinct from the 404 above: the 404 says "no such action", the 400 + // says "the action said no". Both speak HTTP since #3962. const { dispatcher } = makeDispatcher({ registered: { 'global:log_call': () => { @@ -228,13 +222,12 @@ describe('REST /actions — dispatch failure vs business failure (#3913)', () => const res = await dispatcher.handleActions('/global/log_call', 'POST', {}, ctx()); - expect(res.response.status).toBe(200); - expect(res.response.body.data.success).toBe(false); + expect(res.response.status).toBe(400); // The debug wrapper stays in the server log, not on the wire. - expect(res.response.body.data.error).toBe('Contact has no phone number'); + expect(res.response.body.error.message).toBe('Contact has no phone number'); }); - it('carries a validation failure\'s code/fields in the payload, unchanged (#3937)', async () => { + it('carries a validation failure\'s code/fields in error.details (#3937 → #3962)', async () => { const { dispatcher } = makeDispatcher({ registered: { 'global:log_call': () => { @@ -248,17 +241,16 @@ describe('REST /actions — dispatch failure vs business failure (#3913)', () => const res = await dispatcher.handleActions('/global/log_call', 'POST', {}, ctx()); - expect(res.response.status).toBe(200); - expect(res.response.body.data).toMatchObject({ - success: false, + expect(res.response.status).toBe(400); + expect(res.response.body.error.details).toMatchObject({ code: 'VALIDATION_FAILED', fields: [{ field: 'phone', message: 'required' }], }); }); - it('leaves the success envelope untouched', async () => { - // Only the not-found exit moved; a successful action keeps the - // `{success: true, data: {success: true, data}}` shape the SDK reads. + it('serves success as a SINGLE wrap — data is the handler return value (#3962)', async () => { + // The former inner {success, data} envelope existed only to carry a + // failure signal at HTTP 200; failures carry a status now. const { dispatcher } = makeDispatcher({ registered: { 'global:log_call': () => ({ id: 'call_1' }) }, }); @@ -268,7 +260,7 @@ describe('REST /actions — dispatch failure vs business failure (#3913)', () => expect(res.response.status).toBe(200); expect(res.response.body).toMatchObject({ success: true, - data: { success: true, data: { id: 'call_1' } }, + data: { id: 'call_1' }, }); }); }); diff --git a/packages/runtime/src/http-dispatcher.actions-type-dispatch.test.ts b/packages/runtime/src/http-dispatcher.actions-type-dispatch.test.ts index 08405e3d03..6f39fbec86 100644 --- a/packages/runtime/src/http-dispatcher.actions-type-dispatch.test.ts +++ b/packages/runtime/src/http-dispatcher.actions-type-dispatch.test.ts @@ -106,7 +106,8 @@ describe('REST /actions — flow dispatch (#3915)', () => { }), ); expect(res.response.status).toBe(200); - expect(res.response.body.data).toEqual({ success: true, data: { success: true, output: { converted: true } } }); + // Single wrap (#3962): `data` is the automation engine's own result. + expect(res.response.body.data).toEqual({ success: true, output: { converted: true } }); }); // ── params seeding ── the half a mocked automation service could not @@ -247,8 +248,10 @@ describe('REST /actions — flow dispatch (#3915)', () => { const res = await dispatcher.handleActions('/crm_lead/convert_lead', 'POST', {}, ctxFor()); - expect(res.response.body.data.success).toBe(false); - expect(res.response.body.data.error).toMatch(/crm_convert_lead_wizard.*lead already converted/i); + // A rejected flow is a 400 with the semantic code (#3962). + expect(res.response.status).toBe(400); + expect(res.response.body.error.message).toMatch(/crm_convert_lead_wizard.*lead already converted/i); + expect(res.response.body.error.details?.code).toBe('FLOW_FAILED'); }); it('reports a missing automation service as 503, not as a business failure', async () => { @@ -337,7 +340,7 @@ describe('REST /actions — script dispatch is unchanged (#3915 regression guard const res = await dispatcher.handleActions('/crm_lead/mark_done', 'POST', {}, ctxFor()); expect(executeAction).toHaveBeenCalledTimes(1); - expect(res.response.body.data).toEqual({ success: true, data: { ran: 'script' } }); + expect(res.response.body.data).toEqual({ ran: 'script' }); }); it('still runs an UNDECLARED action through the handler registry (handler-only actions)', async () => { @@ -348,7 +351,7 @@ describe('REST /actions — script dispatch is unchanged (#3915 regression guard const res = await dispatcher.handleActions('/crm_lead/handler_only', 'POST', {}, ctxFor()); expect(executeAction).toHaveBeenCalledTimes(1); - expect(res.response.body.data.success).toBe(true); + expect(res.response.status).toBe(200); }); });