fix(actions): a failed server action no longer reports as success (green toast) (#2963)

os-zhuang · web-flow · commit 4db7eb351903 · 2026-07-29T20:05:56.000+08:00
objectstack#3913. useConsoleActionRuntime.serverActionHandler — the console's main action path (list toolbars, row actions, page actions) — decided success from res.ok and the OUTER envelope only. A server that reports a handler failure as HTTP 200 with the failure nested one level down passes both guards, so the ActionRunner fired its green "completed" toast, the list refreshed, and the real error was swallowed. RecordDetailView's copy of the handler already inspected the inner envelope; the shared runtime now does too, as does marketplaceApi.installPackage, which had the identical hole and could report a package as installed when it was not. Also: the failure body can be {success:false, error:{message, code}}. RecordDetailView read `json?.error` raw and would have handed that OBJECT to toast.error() as a React child, crashing the page (React #31) — the exact failure the runtime's errorDetail helper existed to prevent. That helper is now a shared util (utils/actionErrorDetail) and both call sites use it.
diff --git a/.changeset/action-failure-inner-envelope.md b/.changeset/action-failure-inner-envelope.md
@@ -0,0 +1,39 @@
+---
+"@object-ui/app-shell": patch
+---
+
+fix(actions): a failed server action no longer reports as success (green toast) — objectstack#3913
+
+`useConsoleActionRuntime.serverActionHandler` — the console's **main** action
+path (list toolbars, row actions, page actions) — decided success from
+`res.ok` and the OUTER envelope only:
+
+```ts
+if (!res.ok || (json && json.success === false)) { /* failure */ }
+```
+
+A server older than objectstack#3913 reports a handler failure as HTTP **200**
+with the failure nested one level down:
+
+```json
+{"success":true,"data":{"success":false,"error":"Action 'log_call' on object '*' not found"}}
+```
+
+Both guards pass, so the action was reported as completed: the ActionRunner
+fired its green "completed" toast, the list refreshed, and the real error was
+swallowed. `RecordDetailView`'s copy of the same handler already inspected the
+inner envelope; the shared runtime now does too, and the marketplace install
+call (`marketplaceApi.installPackage`), which had the identical hole and could
+report a package as installed when it was not.
+
+Current servers answer a failed action with a real HTTP status, which `!res.ok`
+catches first — the inner-envelope check is what keeps the console honest
+against a runtime that has not been upgraded yet.
+
+**Also fixed:** with objectstack#3913 the failure body is
+`{success: false, error: {message, code}}`. `RecordDetailView` read `json?.error`
+raw and would have handed that **object** to `toast.error()` as a React child,
+crashing the page (React #31) — the exact failure the console runtime's
+`errorDetail` helper existed to prevent. That helper is now a shared util
+(`utils/actionErrorDetail`) and both call sites go through it, so a nested
+`{message}` always resolves to a string.
diff --git a/packages/app-shell/src/console/marketplace/marketplaceApi.ts b/packages/app-shell/src/console/marketplace/marketplaceApi.ts
@@ -248,9 +248,20 @@ export async function installPackage(input: {
   });
   let payload: any = null;
   try { payload = await res.json(); } catch { /* empty */ }
-  if (!res.ok) {
+  // A server older than objectstack#3913 reports a failed install action as
+  // HTTP 200 `{success: true, data: {success: false, error}}` — checking only
+  // `res.ok` reported a package as installed when it was not. Current servers
+  // answer with a real status, which `!res.ok` catches first.
+  const inner = payload?.data;
+  const innerFailed = !!inner && typeof inner === 'object' && inner.success === false;
+  if (!res.ok || innerFailed) {
     const code = payload?.code ?? payload?.error?.code ?? `HTTP_${res.status}`;
-    const message = payload?.error ?? payload?.message ?? res.statusText;
+    // `error` is a nested `{code, message}` object on the current wire and a
+    // plain string on the older one — read the message out of both before
+    // falling back, or a real explanation degrades into a bare status code.
+    const message = innerFailed
+      ? (inner.error ?? res.statusText)
+      : (payload?.error?.message ?? payload?.error ?? payload?.message ?? res.statusText);
     const err = new Error(typeof message === 'string' ? message : `${code}`);
     (err as any).code = code;
     (err as any).status = res.status;
diff --git a/packages/app-shell/src/hooks/__tests__/useConsoleActionRuntime.test.tsx b/packages/app-shell/src/hooks/__tests__/useConsoleActionRuntime.test.tsx
@@ -305,6 +305,79 @@ describe('useConsoleActionRuntime — authenticated handlers', () => {
     expect((toast as any).error).not.toHaveBeenCalled();
   });
 
+  it('serverActionHandler treats an INNER success:false as a failure (objectstack#3913 — no green toast on a failed action)', async () => {
+    // A server older than objectstack#3913 wraps a handler failure as HTTP 200
+    // `{success: true, data: {success: false, error}}`. Reading only `res.ok`
+    // and the OUTER `success` reported that as a completed action and fired the
+    // green "completed" toast while swallowing the real error — the reported
+    // bug. The console must inspect the inner envelope for those servers.
+    authFetchSpy.mockResolvedValue({
+      ok: true,
+      status: 200,
+      json: async () => ({
+        success: true,
+        data: { success: false, error: "Action 'log_call' on object '*' not found" },
+      }),
+    });
+    const { result } = renderHook(() =>
+      useConsoleActionRuntime({ dataSource: {}, objects: [], objectName: 'crm_call' }),
+    );
+
+    let res: any;
+    await act(async () => {
+      res = await result.current.serverActionHandler({ type: 'script', name: 'log_call' } as any);
+    });
+
+    expect(res).toEqual({ success: false, error: "Action 'log_call' on object '*' not found" });
+    expect((toast as any).error).not.toHaveBeenCalled();
+  });
+
+  it('serverActionHandler resolves a nested {error:{message}} to a STRING (objectstack#3913 wire)', async () => {
+    // Current servers answer a failed action with a real status and the nested
+    // envelope. Passing that object through as `ActionResult.error` reaches
+    // `toast.error()` as a React child and crashes the page (React #31).
+    authFetchSpy.mockResolvedValue({
+      ok: false,
+      status: 404,
+      json: async () => ({
+        success: false,
+        error: { message: "Action 'log_call' on object 'global' not found", code: 404 },
+      }),
+    });
+    const { result } = renderHook(() =>
+      useConsoleActionRuntime({ dataSource: {}, objects: [] }),
+    );
+
+    let res: any;
+    await act(async () => {
+      res = await result.current.serverActionHandler({ type: 'script', name: 'log_call' } as any);
+    });
+
+    expect(res.success).toBe(false);
+    expect(typeof res.error).toBe('string');
+    expect(res.error).toBe("Action 'log_call' on object 'global' not found");
+  });
+
+  it('serverActionHandler still reports success when the inner envelope says so', async () => {
+    // The success wire is unchanged by objectstack#3913 — guard against the
+    // inner-envelope check turning a good action into a failure.
+    authFetchSpy.mockResolvedValue({
+      ok: true,
+      status: 200,
+      json: async () => ({ success: true, data: { success: true, data: { id: 'call_1' } } }),
+    });
+    const { result } = renderHook(() =>
+      useConsoleActionRuntime({ dataSource: {}, objects: [], objectName: 'crm_call' }),
+    );
+
+    let res: any;
+    await act(async () => {
+      res = await result.current.serverActionHandler({ type: 'script', name: 'log_call' } as any);
+    });
+
+    expect(res).toMatchObject({ success: true });
+  });
+
   it('exposes ActionProvider props with the api/flow/script/modal handlers wired', () => {
     const { result } = renderHook(() =>
       useConsoleActionRuntime({ dataSource: {}, objects: [], objectName: 'inv' }),
diff --git a/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx b/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx
@@ -48,6 +48,7 @@ import { resolveActionParams } from '../utils/resolveActionParams';
 import { EnvironmentEntitlementDialog, type EntitlementDialogState } from '../environment/EnvironmentEntitlementDialog';
 import { entitlementDialogFromError, type EntitlementDialogSpec } from '../environment/entitlements';
 import { resolvePageVarTokens } from '../utils/resolvePageVarTokens';
+import { actionErrorDetail } from '../utils/actionErrorDetail';
 
 const FALLBACK_USER = { id: 'current-user', name: 'Demo User', isPlatformAdmin: false };
 
@@ -68,20 +69,11 @@ function isRecordScoped(action: ActionDef): boolean {
 }
 
 /**
- * Extract a human-readable message from an error response body. The
- * ObjectStack envelope nests it as `{ error: { code, message } }` — passing
- * that object through as `ActionResult.error` reaches `toast.error()` as a
- * React child and crashes the page (React #31). Always resolve to a string.
+ * Extract a human-readable message from an error response body — shared with
+ * `RecordDetailView`, which runs the same `/actions` request and needs the same
+ * React-#31 guard. See `../utils/actionErrorDetail`.
  */
-function errorDetail(body: unknown, fallback: string): string {
-  const b = body as { error?: unknown; message?: unknown } | null;
-  const err = b?.error;
-  if (typeof err === 'string' && err.length > 0) return err;
-  const nested = (err as { message?: unknown } | null)?.message;
-  if (typeof nested === 'string' && nested.length > 0) return nested;
-  if (typeof b?.message === 'string' && b.message.length > 0) return b.message;
-  return fallback;
-}
+const errorDetail = actionErrorDetail;
 
 export interface ConsoleActionRuntimeOptions {
   /** Adapter for generic CRUD / execute calls. */
@@ -595,8 +587,21 @@ export function useConsoleActionRuntime(opts: ConsoleActionRuntimeOptions): Cons
         },
       );
       const json = await res.json().catch(() => null);
-      if (!res.ok || (json && json.success === false)) {
-        const errMsg = errorDetail(json, `Action "${targetName}" failed (HTTP ${res.status})`);
+      // A server older than objectstack#3913 reports a script action that THREW
+      // as HTTP 200 `{success: true, data: {success: false, error}}` — transport
+      // success wrapping a business failure. Reading only `res.ok` and the OUTER
+      // `success` mistook that for a completed action and fired the green
+      // "completed" toast while swallowing the real error. Inspect the INNER
+      // envelope too (RecordDetailView's copy of this handler already did).
+      // Current servers answer with a real status, so `!res.ok` catches it
+      // first; this branch is what keeps the console honest against a server
+      // that has not been upgraded yet.
+      const inner = json?.data;
+      const innerFailed = !!inner && typeof inner === 'object' && (inner as any).success === false;
+      if (!res.ok || (json && json.success === false) || innerFailed) {
+        const errMsg = innerFailed
+          ? errorDetail(inner, `Action "${targetName}" failed`)
+          : errorDetail(json, `Action "${targetName}" failed (HTTP ${res.status})`);
         if (preOpenedTab) { try { preOpenedTab.close(); } catch { /* ignore */ } }
         // Don't toast here — the ActionRunner's post-execution hook surfaces
         // `error` as a toast (see apiHandler/flowHandler, which likewise only
diff --git a/packages/app-shell/src/utils/actionErrorDetail.ts b/packages/app-shell/src/utils/actionErrorDetail.ts
@@ -0,0 +1,23 @@
+/**
+ * Resolve a human-readable message out of an ObjectStack error payload.
+ *
+ * Always returns a STRING. The envelope nests the message as
+ * `{ error: { code, message } }`, and passing that object through as
+ * `ActionResult.error` reaches `toast.error()` as a React child and crashes the
+ * page (React #31). That shape used to be rare on the `/actions` route —
+ * failures came back as HTTP 200 with a plain-string `error` — but since
+ * objectstack#3913 a failed action answers with a real status and the nested
+ * object, so every `/actions` caller has to go through this.
+ *
+ * Handles, in order: `{error: 'msg'}`, `{error: {message: 'msg'}}`,
+ * `{message: 'msg'}`, else the caller's fallback.
+ */
+export function actionErrorDetail(body: unknown, fallback: string): string {
+  const b = body as { error?: unknown; message?: unknown } | null;
+  const err = b?.error;
+  if (typeof err === 'string' && err.length > 0) return err;
+  const nested = (err as { message?: unknown } | null)?.message;
+  if (typeof nested === 'string' && nested.length > 0) return nested;
+  if (typeof b?.message === 'string' && b.message.length > 0) return b.message;
+  return fallback;
+}
diff --git a/packages/app-shell/src/views/RecordDetailView.tsx b/packages/app-shell/src/views/RecordDetailView.tsx
@@ -33,6 +33,7 @@ import { RelatedRecordActionsBridge } from './RelatedRecordActionsBridge';
 import { withPageTabsUrlSync } from '../utils/pageTabsUrlSync';
 import { RECORD_DETAIL_TAB_PARAM, RECORD_TRAIL_PARAM, decodeRecordTrail, buildRecordTrailHref } from '../urlParams';
 import { resolveActionParams } from '../utils/resolveActionParams';
+import { actionErrorDetail } from '../utils/actionErrorDetail';
 import { useRecordBreadcrumbTitle } from '../context/NavigationContext';
 import type { FeedItem } from '@object-ui/types';
 import type { ActionDef, ActionParamDef } from '@object-ui/core';
@@ -759,15 +760,22 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri
       );
       const json = await res.json().catch(() => null);
       // The action route wraps the handler's return value in a {success, data}
-      // envelope. A script action that THROWS is reported as
-      // `data: { success: false, error }` while the OUTER success stays true,
-      // so we must inspect the inner envelope too — otherwise a failed action
-      // is mistaken for success and fires the green "completed" toast while the
-      // real error is swallowed.
+      // envelope. A server older than objectstack#3913 reports a script action
+      // that THROWS as `data: { success: false, error }` while the OUTER success
+      // stays true, so we must inspect the inner envelope too — otherwise a
+      // failed action is mistaken for success and fires the green "completed"
+      // toast while the real error is swallowed. Current servers answer with a
+      // real status, which `!res.ok` catches first.
       const inner = json?.data;
       const innerFailed = inner && typeof inner === 'object' && inner.success === false;
       if (!res.ok || (json && json.success === false) || innerFailed) {
-        const errMsg = (innerFailed && inner.error) || json?.error || `Action "${targetName}" failed (HTTP ${res.status})`;
+        // Always resolve to a STRING. Since objectstack#3913 a failed action
+        // answers with the nested `{error: {code, message}}` envelope — handing
+        // that object to `toast.error()` as a React child crashes the page
+        // (React #31), which the raw `json?.error` read here used to do.
+        const errMsg = innerFailed
+          ? actionErrorDetail(inner, `Action "${targetName}" failed`)
+          : actionErrorDetail(json, `Action "${targetName}" failed (HTTP ${res.status})`);
         if (preOpenedTab) { try { preOpenedTab.close(); } catch { /* ignore */ } }
         // Don't toast here. This handler always runs through the ActionRunner
         // (registered as the `script` handler on the ActionProvider below, which
