fix(actions)!: failures speak HTTP — rejections are 400, success is a single wrap (#3962)

.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: <handler return value>}` — 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.

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 },

content/docs/api/error-catalog.mdx

@@ -372,17 +372,14 @@ an environment scope (no `X-Environment-Id` header and no hostname mapping).
 
 ## 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>
+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: <handler return value> }` |
+| 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 } }` |
@@ -391,21 +388,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 });

content/docs/ui/actions.mdx

@@ -229,7 +229,7 @@ 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": <your handler's return value> }
 ```
 
 The URL names the action by its **`name`**, never by `target`. `target` binds
@@ -239,19 +239,16 @@ the server resolves your declaration by name and derives the handler key from
 it. Rename the underlying function freely; as long as the declaration's
 `target` follows, the public URL is unchanged.
 
-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

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 } },

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<T>(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 = {
       /**

packages/runtime/src/action-execution.ts

@@ -430,11 +430,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;
 }