fix(chatbot): read the agent catalog in the declared envelope too (objectstack#4053)

.changeset/agent-catalog-envelope-tolerance.md

@@ -0,0 +1,33 @@
+---
+'@object-ui/plugin-chatbot': patch
+---
+
+**Read the agent catalog in the declared envelope too, before the server converts.**
+
+`GET /api/v1/ai/agents` is served by two producers — the framework dispatcher's
+degraded fallback when no AI service is registered, and cloud's `service-ai` — and
+it is one of the last SDK-addressable routes still answering outside the platform's
+declared `{ success: true, data }` envelope (objectstack#4053). `useAgents` read
+only `{ agents }` and a bare array, so the day either producer converts, the parse
+would miss.
+
+That miss is unusually dangerous on this particular route, which is why it is worth
+getting ahead of rather than fixing after. The catalog is not just data:
+`useAiSurfaceEnabled` gates the **entire AI surface** on `agents.length > 0`,
+because the route is access-filtered per caller and is therefore the only signal
+that is both edition- and user-aware (ADR-0068). An empty list is the correct
+answer for a seat-less user or a Community-Edition deployment with no `service-ai`
+— so a parse miss and the legitimate hidden state are **indistinguishable**: no
+error, no 403, no log, just the FAB, the top-bar link and the designer's "Ask AI"
+quietly gone for everyone.
+
+`extractAgentList` now folds all four shapes to the same list — a bare array,
+`{ agents }`, `{ success: true, data: [...] }`, and `{ success: true, data:
+{ agents } }` — detecting the envelope the way `ObjectStackClient.unwrapResponse`
+does (a **boolean** `success`), so the two readers cannot disagree about what
+counts as one. Nine tests cover it; reverting to the previous two-shape read fails
+five of them.
+
+No behaviour change against any server shipping today: the shapes that worked
+before still parse identically. This only removes the lockstep requirement, so the
+server side can convert on its own schedule.

packages/plugin-chatbot/src/__tests__/agentListShapes.test.ts

@@ -0,0 +1,96 @@
+// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * `GET /api/v1/ai/agents` shape tolerance (objectstack#4053).
+ *
+ * Two producers serve this route — the framework dispatcher's degraded fallback
+ * when no AI service is registered, and cloud's `service-ai` — and it is
+ * mid-migration onto the platform's declared `{ success: true, data }` envelope.
+ *
+ * Why this file exists rather than a comment: an unrecognised shape here does not
+ * throw, warn, or log. It yields an empty list, and `useAiSurfaceEnabled` turns an
+ * empty list into "hide the entire AI surface". That is ALSO the correct behaviour
+ * for a seat-less user (ADR-0068) or a Community-Edition deployment with no
+ * `service-ai` — so a parse miss looks exactly like the legitimate hidden state.
+ * There is no downstream signal that would catch it: no failing request, no 403,
+ * no visible difference. The tests are the only thing standing between an envelope
+ * conversion on the server and the AI UI quietly vanishing for every user.
+ *
+ * Teaching the reader all three shapes BEFORE any producer converts is what lets
+ * the server side move on its own schedule instead of landing in lockstep with a
+ * console release.
+ */
+
+import { describe, expect, it } from 'vitest';
+import { extractAgentList } from '../useAgents';
+
+const ASK = { name: 'ask', label: 'Ask' };
+const BUILD = { name: 'build', label: 'Build' };
+
+describe('extractAgentList — every shape this route answers in', () => {
+  it('reads `{ agents }` — what both producers send today', () => {
+    expect(extractAgentList({ agents: [ASK, BUILD] })).toEqual([ASK, BUILD]);
+  });
+
+  it('reads a bare array', () => {
+    expect(extractAgentList([ASK, BUILD])).toEqual([ASK, BUILD]);
+  });
+
+  it('reads the declared envelope with `data` as the array', () => {
+    // The shape objectstack#3983 set the precedent for: `data` carries the
+    // payload directly. This is the variant that silently emptied the list.
+    expect(extractAgentList({ success: true, data: [ASK, BUILD] })).toEqual([ASK, BUILD]);
+  });
+
+  it('reads the declared envelope with `data: { agents }`', () => {
+    // The other plausible conversion — relocating the existing payload under
+    // `data` rather than flattening it. Both must read the same.
+    expect(extractAgentList({ success: true, data: { agents: [ASK, BUILD] } })).toEqual([ASK, BUILD]);
+  });
+
+  it('agrees across all four shapes — the point of one extractor', () => {
+    const shapes: unknown[] = [
+      { agents: [ASK] },
+      [ASK],
+      { success: true, data: [ASK] },
+      { success: true, data: { agents: [ASK] } },
+    ];
+    for (const s of shapes) expect(extractAgentList(s)).toEqual([ASK]);
+  });
+});
+
+describe('extractAgentList — empty is a real answer, not a fallback', () => {
+  it('an empty catalog stays empty in every shape', () => {
+    // A seat-less user gets this legitimately (ADR-0068), so it must not be
+    // conflated with a miss — but it must not throw either.
+    expect(extractAgentList({ agents: [] })).toEqual([]);
+    expect(extractAgentList([])).toEqual([]);
+    expect(extractAgentList({ success: true, data: [] })).toEqual([]);
+    expect(extractAgentList({ success: true, data: { agents: [] } })).toEqual([]);
+  });
+
+  it('never throws on a shape it does not recognise', () => {
+    // Callers gate the whole AI surface on this; a throw would surface as a
+    // load error rather than a hidden surface, but neither is worth a crash.
+    for (const junk of [null, undefined, 0, 'nope', {}, { data: null }, { agents: 'no' }]) {
+      expect(extractAgentList(junk)).toEqual([]);
+    }
+  });
+});
+
+describe('extractAgentList — envelope detection matches unwrapResponse', () => {
+  it('keys on a BOOLEAN `success`, as the SDK does', () => {
+    // `ObjectStackClient.unwrapResponse` treats a body as an envelope iff
+    // `typeof body.success === 'boolean'`. Diverging from that rule is how two
+    // readers of one route end up disagreeing about which shape they got.
+    expect(extractAgentList({ success: true, data: [ASK] })).toEqual([ASK]);
+    expect(extractAgentList({ success: false, data: [ASK] })).toEqual([ASK]);
+  });
+
+  it('does NOT treat a non-boolean `success` as an envelope', () => {
+    // A payload that happens to carry a truthy `success` field is data, not an
+    // envelope — unwrapping it would hide the real agents.
+    expect(extractAgentList({ success: 'yes', agents: [ASK] })).toEqual([ASK]);
+    expect(extractAgentList({ success: 1, agents: [ASK] })).toEqual([ASK]);
+  });
+});

packages/plugin-chatbot/src/useAgents.ts

@@ -149,6 +149,43 @@ export function resolveDefaultAgentName(
   return agents[0].name;
 }
 
+/**
+ * Pull the agent list out of whatever `GET /api/v1/ai/agents` answered.
+ *
+ * This route is served by more than one producer and is mid-migration onto the
+ * platform's declared `{ success: true, data }` envelope
+ * (objectstack#4053), so three shapes have to read the same:
+ *
+ *   [ … ]                                  a bare array
+ *   { agents: [ … ] }                      today's shape, both producers
+ *   { success: true, data: … }             the declared envelope, whose `data`
+ *                                          may be the array or `{ agents }`
+ *
+ * The envelope is detected the way `ObjectStackClient.unwrapResponse` detects
+ * it — a **boolean** `success` — so the two agree on what counts as one.
+ *
+ * Reading the envelope BEFORE any producer emits it is deliberate, and it is the
+ * whole point of doing this ahead of the conversion. An unrecognised shape here
+ * does not throw or warn: it yields an empty list, and `useAiSurfaceEnabled`
+ * turns an empty list into "hide the entire AI surface". That is also the
+ * CORRECT behaviour for a seat-less user or a Community-Edition deployment with
+ * no `service-ai` — so a parse miss is indistinguishable from the legitimate
+ * hidden state, with no error, no 403 and no log to notice it by. Teaching the
+ * consumer first means the producer can convert on its own schedule instead of
+ * having to land in lockstep with this file.
+ */
+export function extractAgentList(payload: unknown): RawAgent[] {
+  const isEnvelope =
+    !!payload && typeof payload === 'object' && !Array.isArray(payload) &&
+    typeof (payload as { success?: unknown }).success === 'boolean';
+
+  const body = isEnvelope ? (payload as { data?: unknown }).data : payload;
+
+  if (Array.isArray(body)) return body as RawAgent[];
+  const agents = (body as { agents?: unknown } | null | undefined)?.agents;
+  return Array.isArray(agents) ? (agents as RawAgent[]) : [];
+}
+
 function normalize(raw: RawAgent[]): AgentDescriptor[] {
   return raw
     .filter((a) => typeof a?.name === 'string' && a.name.length > 0)
@@ -196,9 +233,8 @@ function fetchAgentsCached(
   })
     .then(async (res) => {
       if (!res.ok) throw new Error(`Failed to load agents (${res.status})`);
-      const payload = (await res.json()) as { agents?: RawAgent[] } | RawAgent[];
-      const list = Array.isArray(payload) ? payload : payload?.agents ?? [];
-      return normalize(list);
+      // Bare array, `{ agents }`, or the declared envelope — see `extractAgentList`.
+      return normalize(extractAgentList(await res.json()));
     })
     .then((normalized) => {
       agentsCache.set(apiBase, { data: normalized, timestamp: Date.now() });