fix(console): marketplace read cloud errors seven different ways — two break on the conversion, two are broken today (cloud#944) (#3086)

`service-cloud` answers failures in two shapes and is mid-conversion between
them: `{ success: false, error: '<message>' }` via `fail()`, and the declared
`{ success: false, error: { code, message } }`.

`marketplaceApi.ts` hand-rolled that read at seven call sites in four different
ways. Two read `payload?.error` bare, so a converted producer would hand them
an object where the type declares `error?: string` — and an object reaching
`toast.error()` as a React child crashes the page (objectstack#3913's symptom).
Two others read only the nested form and are therefore broken TODAY, degrading
a real explanation into a bare status code.

One `readApiError(payload, res)` replaces all seven: nested first, then the
bare string, then a top-level `message`, then `statusText` — and it guarantees
a STRING back, so an unexpected shape degrades to the code rather than
travelling onward as an object. `installPackage` keeps its own `innerFailed`
branch, which reads a different object.

Consumer half of cloud#944's conversion order, same shape as objectui#2992
teaching `useAgents` four shapes before the /ai/agents producers moved.

8 new tests; app-shell tsc clean.

Author: os-zhuang

packages/app-shell/src/console/marketplace/__tests__/readApiError.test.ts

@@ -0,0 +1,84 @@
+/**
+ * ObjectUI
+ * Copyright (c) 2024-present ObjectStack Inc.
+ *
+ * `readApiError` — every failure dialect `service-cloud` answers in (cloud#944).
+ *
+ * These routes are mid-conversion. `fail()` / `failWithCode()` emit
+ * `{ success: false, error: '<message>' }` today; the declared envelope nests it
+ * as `{ code, message }`. Both must read the same here, so the server side can
+ * convert on its own schedule instead of landing in lockstep with a console
+ * release — the same reason `useAgents` learned four shapes before the /ai/agents
+ * producers moved (objectui#2992, objectstack#4053).
+ *
+ * Why a test rather than a comment: the failure is not a parse error. Two call
+ * sites in `marketplaceApi.ts` read `payload?.error` bare, so a converted
+ * producer hands them an OBJECT where the surrounding type declares
+ * `error?: string`. That object reaches `toast.error()` as a React child and
+ * crashes the page — objectstack#3913's exact symptom, which the actions route
+ * already carries `interpretActionResponse` to prevent. Nothing about the types
+ * catches it: the payloads are `any`.
+ */
+
+import { describe, expect, it } from 'vitest';
+import { readApiError } from '../marketplaceApi';
+
+const res = (status = 400, statusText = 'Bad Request') => ({ status, statusText });
+
+describe('readApiError — both dialects read the same', () => {
+  it('reads the declared envelope', () => {
+    expect(readApiError(
+      { success: false, error: { code: 'ENV_NOT_FOUND', message: 'Environment not found' } },
+      res(404, 'Not Found'),
+    )).toEqual({ code: 'ENV_NOT_FOUND', message: 'Environment not found' });
+  });
+
+  it("reads `fail()`'s bare string — what these routes send today", () => {
+    expect(readApiError(
+      { success: false, error: 'environment_id is required' },
+      res(),
+    )).toEqual({ code: 'HTTP_400', message: 'environment_id is required' });
+  });
+
+  it('agrees on the message across both — the point of one reader', () => {
+    const nested = readApiError({ error: { code: 'X', message: 'same text' } }, res());
+    const bare = readApiError({ error: 'same text' }, res());
+    expect(nested.message).toBe(bare.message);
+  });
+
+  it("reads `failWithCode()`'s sibling `code`", () => {
+    // `{ success: false, error: '<message>', code }` — the third dialect, where
+    // the code sits BESIDE `error` rather than inside it.
+    expect(readApiError(
+      { success: false, error: 'Slug rename in progress', code: 'SLUG_RENAME_IN_PROGRESS' },
+      res(503, 'Service Unavailable'),
+    )).toEqual({ code: 'SLUG_RENAME_IN_PROGRESS', message: 'Slug rename in progress' });
+  });
+
+  it('prefers the nested code over a sibling one when both are present', () => {
+    expect(readApiError({ error: { code: 'INNER', message: 'm' }, code: 'OUTER' }, res()).code)
+      .toBe('INNER');
+  });
+});
+
+describe('readApiError — never hands back a non-string message', () => {
+  it('degrades an unexpected `error` shape to the code rather than passing the object on', () => {
+    // The whole reason this returns a string: an object travelling onward as
+    // `ActionResult.error` reaches a toast as a React child and crashes the page.
+    const out = readApiError({ error: { unexpected: true } }, res(500, 'Server Error'));
+    expect(typeof out.message).toBe('string');
+    expect(out.message).toBe('HTTP_500');
+  });
+
+  it('falls back to statusText when there is no body at all', () => {
+    expect(readApiError(null, res(502, 'Bad Gateway')))
+      .toEqual({ code: 'HTTP_502', message: 'Bad Gateway' });
+    expect(readApiError({}, res(502, 'Bad Gateway')))
+      .toEqual({ code: 'HTTP_502', message: 'Bad Gateway' });
+  });
+
+  it('reads a top-level `message` — a few routes put the text there', () => {
+    expect(readApiError({ message: 'quota exceeded' }, res(429, 'Too Many Requests')).message)
+      .toBe('quota exceeded');
+  });
+});

packages/app-shell/src/console/marketplace/marketplaceApi.ts

@@ -38,6 +38,48 @@ function withEnvAuth(headers: Record<string, string>): Record<string, string> {
   return token ? { ...headers, Authorization: `Bearer ${token}` } : headers;
 }
 
+/**
+ * Read a code + human message out of a failed cloud response, whichever dialect
+ * it arrived in.
+ *
+ * The routes behind this file (`service-cloud`) answer failures in two shapes,
+ * and are mid-conversion from one to the other (cloud#944):
+ *
+ *   { success: false, error: '<message>' }              today, via `fail()`
+ *   { success: false, error: { code, message } }        the declared envelope
+ *
+ * Seven call sites in this file each hand-rolled their own read of that, in
+ * four different ways, and the spread was not harmless:
+ *
+ *   - two read `payload?.error` bare, with no nested fallback. Once the
+ *     producer converts, those receive an OBJECT where the surrounding type
+ *     declares `error?: string` — and an object handed to a toast as a React
+ *     child crashes the page. That is objectstack#3913's exact symptom, which
+ *     `interpretActionResponse` already exists to prevent on the actions route.
+ *   - two read only the nested form, so against a server shipping TODAY they
+ *     find nothing and degrade a real explanation into a bare status code.
+ *
+ * So this is not only preparation for the conversion — it fixes messages that
+ * are already being swallowed. One function, so the next reader has one place
+ * to look and the next route has nothing to hand-roll.
+ *
+ * The `typeof … === 'string'` guard is what makes it safe to hand the result
+ * straight to a `string` field: an unexpected shape degrades to the code rather
+ * than travelling onward as an object.
+ */
+export function readApiError(
+  payload: any,
+  res: { status: number; statusText: string },
+): { code: string; message: string } {
+  const code = payload?.error?.code ?? payload?.code ?? `HTTP_${res.status}`;
+  const raw =
+    payload?.error?.message   // the declared envelope
+    ?? payload?.error         // `fail()`'s bare string
+    ?? payload?.message       // a few routes put the text here
+    ?? res.statusText;
+  return { code: String(code), message: typeof raw === 'string' ? raw : String(code) };
+}
+
 /**
  * Per-locale overrides for translatable package fields. Mirrors
  * `PackageTranslation` from @objectstack/spec/cloud — duplicated here
@@ -143,8 +185,7 @@ async function call<T>(path: string, init?: RequestInit): Promise<T> {
   let payload: any = null;
   try { payload = await res.json(); } catch { /* empty body */ }
   if (!res.ok) {
-    const code = payload?.error?.code ?? payload?.code ?? `HTTP_${res.status}`;
-    const message = payload?.error?.message ?? payload?.error ?? res.statusText;
+    const { code, message } = readApiError(payload, res);
     const err = new Error(typeof message === 'string' ? message : `${code}`);
     (err as any).code = code;
     (err as any).status = res.status;
@@ -221,8 +262,7 @@ export async function installPackage(input: {
     let payload: any = null;
     try { payload = await res.json(); } catch { /* empty */ }
     if (!res.ok || payload?.success === false) {
-      const code = payload?.error?.code ?? payload?.code ?? `HTTP_${res.status}`;
-      const message = payload?.error?.message ?? payload?.error ?? payload?.message ?? res.statusText;
+      const { code, message } = readApiError(payload, res);
       const err = new Error(typeof message === 'string' ? message : `${code}`);
       (err as any).code = code;
       (err as any).status = res.status;
@@ -255,13 +295,11 @@ export async function installPackage(input: {
   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}`;
-    // `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);
+    // The inner branch reads a DIFFERENT object — the 200-with-`data.success:
+    // false` shape above — so it stays hand-read; only the outer one is shared.
+    const outer = readApiError(payload, res);
+    const code = payload?.code ?? outer.code;
+    const message = innerFailed ? (inner.error ?? res.statusText) : outer.message;
     const err = new Error(typeof message === 'string' ? message : `${code}`);
     (err as any).code = code;
     (err as any).status = res.status;
@@ -488,7 +526,7 @@ export async function reseedSampleData(installationId: string): Promise<{ ok: bo
     });
     const payload: any = await res.json().catch(() => ({}));
     if (!res.ok || payload?.success === false) {
-      return { ok: false, error: payload?.error || `HTTP ${res.status}` };
+      return { ok: false, error: readApiError(payload, res).message };
     }
     return { ok: true };
   } catch (err: any) {
@@ -509,7 +547,7 @@ export async function purgeSampleData(installationId: string): Promise<{ ok: boo
     });
     const payload: any = await res.json().catch(() => ({}));
     if (!res.ok || payload?.success === false) {
-      return { ok: false, error: payload?.error || `HTTP ${res.status}` };
+      return { ok: false, error: readApiError(payload, res).message };
     }
     return { ok: true, deleted: Number(payload?.data?.deleted ?? 0) };
   } catch (err: any) {
@@ -604,8 +642,7 @@ export async function installLocal(input: {
   let payload: any = null;
   try { payload = await res.json(); } catch { /* empty */ }
   if (!res.ok) {
-    const code = payload?.error?.code ?? `HTTP_${res.status}`;
-    const message = payload?.error?.message ?? res.statusText;
+    const { code, message } = readApiError(payload, res);
     const err = new Error(typeof message === 'string' ? message : `${code}`);
     (err as any).code = code;
     (err as any).status = res.status;
@@ -683,8 +720,7 @@ export async function uninstallLocal(manifestId: string): Promise<void> {
   });
   if (!res.ok) {
     const payload: any = await res.json().catch(() => ({}));
-    const message = payload?.error?.message ?? res.statusText;
-    throw new Error(typeof message === 'string' ? message : `HTTP_${res.status}`);
+    throw new Error(readApiError(payload, res).message);
   }
 }