fix(console): a flow or action that failed under HTTP 200 stops reporting success (#2958) (#2995)

Business failures come back HTTP 200 with the failure on the INNER envelope
(objectstack#3913), and a failed flow launch carries neither `status` nor
`screen`. Three call sites read that response by hand and two read it
incompletely, so a failed run was indistinguishable from a completed one: no
dialog, a green "completed successfully" toast, a refresh, and the real error
swallowed.

- `utils/flowResponse.ts` (new) — the flow trigger/resume rule, once:
  transport failure / flow failure / screen pause / terminal success, with
  `error` guaranteed to be a STRING. Failure is classified before `paused`,
  which is safe because the engine always stamps `success: true` alongside
  `status: 'paused'` (service-automation engine.ts).
- Both flow-launch handlers (`useConsoleActionRuntime`, `RecordDetailView`)
  now go through it, so a launch that failed returns `success: false` and no
  longer refreshes. RecordDetailView additionally stops passing `json.error`
  through raw — the nested `{code, message}` shape reaches `toast.error()` as
  a React child and crashes the page (React #31).
- `FlowRunner`'s resume shares the same interpreter, keeping its retryable
  (transport) vs terminal (flow) close behaviour and gaining the coercion.
- `apiHandler` classifies an HTTP-200 `success: false` body as a failure
  before refreshing on it.

The `script` handler was already fixed via `interpretActionResponse`; this is
the same lesson on the automation route, with a matching ratchet test so the
rule cannot be hand-rolled back into three copies.

Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

Author: os-zhuang

packages/app-shell/src/flow-envelope.ratchet.test.ts

@@ -0,0 +1,92 @@
+/**
+ * ObjectUI
+ * Copyright (c) 2024-present ObjectStack Inc.
+ *
+ * #2958 ratchet — every flow `trigger` / `resume` caller goes through
+ * `interpretFlowResponse`.
+ *
+ * The sibling of `actions-envelope.ratchet.test.ts`, for the same reason. The
+ * rule for reading an `AutomationResult` lived in THREE hand-rolled copies —
+ * the flow handlers in `useConsoleActionRuntime` and `RecordDetailView`, and
+ * `FlowRunner`'s resume — and only the resume copy was complete. The two launch
+ * copies validated the transport envelope and then treated everything else as
+ * terminal success, so a run that failed on its first node was reported as a
+ * completed one: no dialog, a green "completed successfully" toast, a refresh,
+ * and the real error swallowed. One copy also passed the error value through
+ * raw, where a nested `{code, message}` reaches `toast.error()` as a React child
+ * and crashes the page (React #31).
+ *
+ * If this fails: don't hand-roll the check. Import `interpretFlowResponse` from
+ * `utils/flowResponse` — it classifies transport failure / flow failure /
+ * screen pause / terminal success, and guarantees a string error.
+ */
+
+import { describe, it, expect } from 'vitest';
+import { readdirSync, readFileSync, statSync } from 'node:fs';
+import path from 'node:path';
+import { fileURLToPath } from 'node:url';
+
+const here = path.dirname(fileURLToPath(import.meta.url));
+const appShellSrc = here;
+
+/** The helper that owns the rule — exempt from its own guard. */
+const OWNER = path.join(appShellSrc, 'utils', 'flowResponse.ts');
+
+function walk(dir: string, out: string[] = []): string[] {
+    for (const entry of readdirSync(dir)) {
+        if (entry === 'node_modules' || entry === 'dist' || entry === '__tests__') continue;
+        const full = path.join(dir, entry);
+        if (statSync(full).isDirectory()) walk(full, out);
+        else if (/\.(ts|tsx)$/.test(entry) && !/\.test\.tsx?$/.test(entry)) out.push(full);
+    }
+    return out;
+}
+
+/**
+ * Comments name routes freely while explaining them. Only CODE counts.
+ */
+function stripComments(src: string): string {
+    return src
+        .replace(/\/\*[\s\S]*?\*\//g, '')
+        .replace(/(^|[^:])\/\/.*$/gm, '$1');
+}
+
+/**
+ * A file that LAUNCHES or RESUMES a flow run — i.e. one that receives an
+ * `AutomationResult` to interpret.
+ *
+ * Deliberately narrower than "names the automation route": most automation
+ * callers READ the registry (`/connectors`, `/actions`, `/_status`, `/runs` for
+ * the observability panel) and get back a list, not a run result. Those have no
+ * envelope rule to get wrong.
+ */
+function runsAFlow(src: string): boolean {
+    const code = stripComments(src);
+    if (!/\/api\/v1\/automation\//.test(code)) return false;
+    return /\/trigger[`'"]/.test(code) || /\/resume[`'"]/.test(code);
+}
+
+describe('#2958 ratchet — flow trigger/resume callers use interpretFlowResponse', () => {
+    const callers = walk(appShellSrc)
+        .filter((f) => f !== OWNER && runsAFlow(readFileSync(f, 'utf8')));
+
+    const offenders = callers
+        .filter((f) => !readFileSync(f, 'utf8').includes('interpretFlowResponse'))
+        .map((f) => path.relative(appShellSrc, f));
+
+    it('no app-shell file interprets a flow run result by hand', () => {
+        expect(offenders).toEqual([]);
+    });
+
+    it('still guards something — the three known callers are present', () => {
+        // A ratchet that matches nothing passes vacuously forever. Pin that the
+        // copies it exists for are actually being scanned.
+        expect(callers.map((f) => path.relative(appShellSrc, f))).toEqual(
+            expect.arrayContaining([
+                path.join('hooks', 'useConsoleActionRuntime.tsx'),
+                path.join('views', 'RecordDetailView.tsx'),
+                path.join('views', 'FlowRunner.tsx'),
+            ]),
+        );
+    });
+});

packages/app-shell/src/hooks/__tests__/useConsoleActionRuntime.test.tsx

@@ -662,6 +662,159 @@ describe('flowHandler — list_toolbar selection fallback', () => {
   });
 });
 
+// #2958 — a business failure comes back HTTP 200 with the failure on the INNER
+// envelope (`data.success === false`), and a failed flow launch carries neither
+// `status` nor `screen`. Both used to land in the terminal-success return: the
+// user saw a green "completed successfully" toast, the view refreshed, and the
+// real error was swallowed. These pin the failure paths AND that the success
+// paths they sit next to still work.
+describe('#2958 — a failure reported under HTTP 200 is a failure, not success', () => {
+  it('flowHandler reports a failed launch (no status, no screen) instead of terminal success', async () => {
+    // The reported repro: a screen flow whose first CRUD node fails. Outer
+    // envelope says success; only `data.success` shows the truth.
+    authFetchSpy.mockResolvedValue({
+      ok: true,
+      json: async () => ({
+        success: true,
+        data: { success: false, error: "Node 'apply' failed: Update requires an ID" },
+      }),
+    });
+    const refreshSpy = vi.fn();
+    const { result } = renderHook(() =>
+      useConsoleActionRuntime({ dataSource: {}, objects: [], objectName: 'inv', onRefresh: refreshSpy }),
+    );
+
+    let res: any;
+    await act(async () => {
+      res = await result.current.flowHandler({ type: 'flow', name: 'convert_lead', target: 'convert_lead' } as any);
+    });
+
+    expect(res.success).toBe(false);
+    expect(res.error).toBe("Node 'apply' failed: Update requires an ID");
+    // A failed run changed nothing — refreshing implies it did.
+    expect(refreshSpy).not.toHaveBeenCalled();
+    // The ActionRunner's post-execution hook owns the error toast; the handler
+    // must not fire a second one.
+    expect((toast as any).error).not.toHaveBeenCalled();
+  });
+
+  it('flowHandler prefers the flow-declared errorMessage over the raw engine error', async () => {
+    authFetchSpy.mockResolvedValue({
+      ok: true,
+      json: async () => ({
+        success: true,
+        data: {
+          success: false,
+          status: 'failed',
+          error: "Node 'apply' failed: constraint violation on lead.status",
+          errorMessage: 'This lead has already been converted.',
+        },
+      }),
+    });
+    const { result } = renderHook(() => useConsoleActionRuntime({ dataSource: {}, objects: [] }));
+
+    let res: any;
+    await act(async () => {
+      res = await result.current.flowHandler({ type: 'flow', name: 'convert_lead', target: 'convert_lead' } as any);
+    });
+
+    expect(res).toMatchObject({ success: false, error: 'This lead has already been converted.' });
+  });
+
+  it('flowHandler still OPENS the wizard on a screen pause (success path intact)', async () => {
+    // The engine stamps `success: true` alongside `status: 'paused'`, which is
+    // why classifying failure first cannot swallow a wizard. `silent: true` is
+    // the marker that the action only opened the runner.
+    authFetchSpy.mockResolvedValue({
+      ok: true,
+      json: async () => ({
+        success: true,
+        data: {
+          success: true,
+          status: 'paused',
+          runId: 'run-7',
+          screen: { nodeId: 'collect', title: 'New Assignee', fields: [] },
+        },
+      }),
+    });
+    const refreshSpy = vi.fn();
+    const { result } = renderHook(() =>
+      useConsoleActionRuntime({ dataSource: {}, objects: [], onRefresh: refreshSpy }),
+    );
+
+    let res: any;
+    await act(async () => {
+      res = await result.current.flowHandler({ type: 'flow', name: 'f', target: 'f' } as any);
+    });
+
+    expect(res).toMatchObject({ success: true, silent: true });
+    // The run hasn't completed — the runner refreshes on completion, not now.
+    expect(refreshSpy).not.toHaveBeenCalled();
+  });
+
+  it('flowHandler coerces a nested {code, message} error to a string (React #31)', async () => {
+    // Handing the object through as `error` reaches `toast.error()` as a React
+    // child and crashes the page.
+    authFetchSpy.mockResolvedValue({
+      ok: false,
+      status: 403,
+      json: async () => ({
+        success: false,
+        error: { message: 'Run is parked on a service-owned node', code: 'PERMISSION_DENIED' },
+      }),
+    });
+    const { result } = renderHook(() => useConsoleActionRuntime({ dataSource: {}, objects: [] }));
+
+    let res: any;
+    await act(async () => {
+      res = await result.current.flowHandler({ type: 'flow', name: 'f', target: 'f' } as any);
+    });
+
+    expect(typeof res.error).toBe('string');
+    expect(res.error).toBe('Run is parked on a service-owned node');
+  });
+
+  it('apiHandler reports an HTTP-200 success:false body instead of refreshing on it', async () => {
+    // The `log_call`-style repro on the api transport: 200, `success: false`.
+    authFetchSpy.mockResolvedValue({
+      ok: true,
+      json: async () => ({ success: false, error: "Action 'log_call' on object '*' not found" }),
+    });
+    const refreshSpy = vi.fn();
+    const { result } = renderHook(() =>
+      useConsoleActionRuntime({ dataSource: {}, objects: [], onRefresh: refreshSpy }),
+    );
+
+    let res: any;
+    await act(async () => {
+      res = await result.current.apiHandler({ type: 'api', name: 'log_call', target: '/api/v1/x' } as any);
+    });
+
+    expect(res.success).toBe(false);
+    expect(res.error).toBe("Action 'log_call' on object '*' not found");
+    expect(refreshSpy).not.toHaveBeenCalled();
+  });
+
+  it('apiHandler leaves a payload that merely CONTAINS a success key alone', async () => {
+    // Only the envelope's own `success: false` is a failure. A handler value
+    // that happens to carry `success` is data, and a `success: true` envelope
+    // is of course fine — neither may be misread as a rejection.
+    authFetchSpy.mockResolvedValue({
+      ok: true,
+      json: async () => ({ success: true, data: { success: false, rows: 0, note: 'partial' } }),
+    });
+    const { result } = renderHook(() => useConsoleActionRuntime({ dataSource: {}, objects: [] }));
+
+    let res: any;
+    await act(async () => {
+      res = await result.current.apiHandler({ type: 'api', name: 'x', target: '/api/v1/x' } as any);
+    });
+
+    expect(res.success).toBe(true);
+    expect(res.data).toEqual({ success: false, rows: 0, note: 'partial' });
+  });
+});
+
 describe('serverActionHandler — list_toolbar selection fallback', () => {
   it('uses the single selected row from the runner context as recordId', async () => {
     authFetchSpy.mockResolvedValue({ ok: true, json: async () => ({ success: true, data: {} }) });

packages/app-shell/src/hooks/useConsoleActionRuntime.tsx

@@ -43,13 +43,14 @@ import { useActionModal } from './useActionModal';
 import { ActionConfirmDialog, type ConfirmDialogState } from '../views/ActionConfirmDialog';
 import { ActionParamDialog, type ParamDialogState } from '../views/ActionParamDialog';
 import { ActionResultDialog, type ResultDialogState } from '../views/ActionResultDialog';
-import { FlowRunner, type ScreenFlowState } from '../views/FlowRunner';
+import { FlowRunner, type ScreenFlowState, type ScreenSpec } from '../views/FlowRunner';
 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';
 import { interpretActionResponse } from '../utils/actionResponse';
+import { interpretFlowResponse } from '../utils/flowResponse';
 
 const FALLBACK_USER = { id: 'current-user', name: 'Demo User', isPlatformAdmin: false };
 
@@ -350,6 +351,16 @@ export function useConsoleActionRuntime(opts: ConsoleActionRuntimeOptions): Cons
           return { success: false, error: errorDetail(body, `HTTP ${res.status}`) };
         }
         const json = await res.json().catch(() => ({}));
+        // A business rejection can arrive as HTTP 200 with `success: false`
+        // (objectstack#3913). `res.ok` alone misses it, so the call reported
+        // success, toasted green and refreshed while the error was swallowed
+        // (#2958). Classified BEFORE the refresh below — a rejected call
+        // changed nothing to re-fetch.
+        if (json && typeof json === 'object' && (json as { success?: unknown }).success === false) {
+          // `name` is not guaranteed on an api action (it can be target-only),
+          // so fall back to the endpoint rather than interpolating `undefined`.
+          return { success: false, error: errorDetail(json, `Action "${action.name || targetStr}" failed`) };
+        }
         if (action.refreshAfter !== false) refresh();
         // Unwrap the ObjectStack `{ success, data }` envelope so `result.data`
         // is the inner payload — the contract every `result.data` consumer
@@ -470,21 +481,28 @@ export function useConsoleActionRuntime(opts: ConsoleActionRuntimeOptions): Cons
         },
       );
       const json = await res.json().catch(() => null);
-      if (!res.ok || (json && json.success === false)) {
-        return { success: false, error: errorDetail(json, `Flow "${flowName}" failed (HTTP ${res.status})`) };
+      // Single source for the flow-response rule — shared with
+      // RecordDetailView's copy of this handler and with FlowRunner's resume.
+      // A launch that FAILED (HTTP 200, `data.success === false`, no `status`
+      // and no `screen`) used to be indistinguishable from a completed run and
+      // fell into the terminal-success return below: no dialog, a green toast,
+      // and a refresh (#2958). See utils/flowResponse.
+      const outcome = interpretFlowResponse<ScreenSpec>(res, json, `Flow "${flowName}"`);
+      if (outcome.kind === 'failed') {
+        // The ActionRunner's post-execution hook surfaces `error` as a toast.
+        return { success: false, error: outcome.error };
       }
       // Screen-flow runtime: paused at a `screen` node awaiting input — open
       // the FlowRunner to render the form + resume. Refresh happens on complete.
-      const data = json?.data ?? {};
-      if (data.status === 'paused' && data.screen) {
-        setScreenFlow({ flowName, runId: data.runId, screen: data.screen });
+      if (outcome.kind === 'paused') {
+        setScreenFlow({ flowName, runId: outcome.runId ?? '', screen: outcome.screen });
         // The action only OPENED the wizard — it hasn't completed. Suppress the
         // action-level success toast; the flow-runner owns completion messaging.
         return { success: true, silent: true };
       }
       const shouldRefresh = action.refreshAfter !== false;
       if (shouldRefresh) refresh();
-      return { success: true, data: json?.data, reload: shouldRefresh };
+      return { success: true, data: outcome.data, reload: shouldRefresh };
     } catch (error) {
       return { success: false, error: (error as Error).message };
     }