fix(mcp): tool allowlist stored/read as jsonb string, not array

Opening the edit form for an MCP server that has a saved tool allowlist crashed
the whole settings page (`TypeError: Ke.map is not a function` in Mantine) — and,
worse, the allowlist was silently NOT enforced. Both stem from one root cause:
the `tool_allowlist` jsonb column round-trips as a JSON STRING, not an array.

Root cause: `jsonbArray` bound `JSON.stringify(value)` (already a JSON string)
straight to a `::jsonb` cast. node-postgres infers the param type as jsonb and
JSON-stringifies it a SECOND time, so the column stored a jsonb STRING SCALAR
(`"[\"a\"]"`, jsonb_typeof = string) instead of an array. On read the driver
hands back the JS string `'["a"]'`. Then:
  - the edit form's TagsInput called `.map` on a string -> page crash;
  - mcp-clients did `Array.isArray(allow)` -> false for a string -> fell through
    to "no restriction" and exposed ALL of the server's tools.

Fix (both verified on the stand):
- Write: `jsonbArray` casts `::text::jsonb` so the param is bound as text (sent
  verbatim) and parsed into a real jsonb array. New rows now store
  jsonb_typeof=array.
- Read: `normalizeRow` runs every fetched row through `parseToolAllowlist`, which
  returns `string[] | null` for both shapes (already-array passes through; a JSON
  string is parsed; null/invalid -> null). This REPAIRS existing double-encoded
  rows on read, so the UI and the allowlist enforcement work without a data
  migration. Applied in findById / listByWorkspace / listEnabled.
- Client: defensive `Array.isArray(...) ? ... : []` guard in the form so a bad
  shape can never take the settings page down again.

Tests: ai-mcp-server.repo.spec (8 cases for parseToolAllowlist — array, the
JSON-string read, null, empty, non-array json, unparseable, non-string elements,
non-string primitive). mcp-servers-to-view + mcp-namespacing still green.
Verified live: an old double-encoded row now reads as an array; a newly created
server stores jsonb_typeof=array.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

apps/client/src/features/workspace/components/settings/components/ai-mcp-server-form.tsx

@@ -56,7 +56,13 @@ function buildInitialValues(server?: IAiMcpServer): FormValues {
     transport: server?.transport ?? "http",
     url: server?.url ?? "",
     authHeader: "",
-    toolAllowlist: server?.toolAllowlist ?? [],
+    // Defensive: TagsInput calls `.map`, so a non-array here (e.g. an API that
+    // returns the jsonb column as a JSON string) would crash the whole page. The
+    // server normalizes this now, but guard anyway so a bad shape can never take
+    // the settings UI down.
+    toolAllowlist: Array.isArray(server?.toolAllowlist)
+      ? server.toolAllowlist
+      : [],
     enabled: server?.enabled ?? true,
   };
 }

apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.spec.ts

@@ -0,0 +1,48 @@
+import { parseToolAllowlist } from './ai-mcp-server.repo';
+
+/**
+ * The `tool_allowlist` jsonb column historically round-trips as a JSON STRING
+ * (rows written by the old double-encoding `jsonbArray`), so the driver hands
+ * back `'["a","b"]'` instead of an array. `parseToolAllowlist` normalizes both
+ * shapes to the `string[] | null` the entity type promises — fixing the settings
+ * UI crash (TagsInput `.map` on a string) and the tool-allowlist enforcement
+ * (which did `Array.isArray(allow)` and silently allowed ALL tools for a string).
+ */
+describe('parseToolAllowlist', () => {
+  it('passes a real string array through unchanged', () => {
+    expect(parseToolAllowlist(['search', 'crawl'])).toEqual(['search', 'crawl']);
+  });
+
+  it('parses a JSON-string array (the double-encoded read) into an array', () => {
+    // This is exactly what the DB returns for an old row: a jsonb string scalar.
+    expect(parseToolAllowlist('["alpha","beta"]')).toEqual(['alpha', 'beta']);
+  });
+
+  it('returns null for null / undefined (unrestricted)', () => {
+    expect(parseToolAllowlist(null)).toBeNull();
+    expect(parseToolAllowlist(undefined)).toBeNull();
+  });
+
+  it('returns [] for an empty array (no items, but a present allowlist)', () => {
+    expect(parseToolAllowlist([])).toEqual([]);
+  });
+
+  it('returns null for a JSON string that is not an array', () => {
+    expect(parseToolAllowlist('"justastring"')).toBeNull();
+    expect(parseToolAllowlist('{"a":1}')).toBeNull();
+  });
+
+  it('returns null for an unparseable string', () => {
+    expect(parseToolAllowlist('not json at all')).toBeNull();
+  });
+
+  it('returns null when elements are not all strings (defensive)', () => {
+    expect(parseToolAllowlist([1, 2, 3] as unknown)).toBeNull();
+    expect(parseToolAllowlist('[1,2,3]')).toBeNull();
+  });
+
+  it('returns null for a non-string, non-array primitive', () => {
+    expect(parseToolAllowlist(42 as unknown)).toBeNull();
+    expect(parseToolAllowlist(true as unknown)).toBeNull();
+  });
+});

apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.ts

@@ -21,32 +21,35 @@ export class AiMcpServerRepo {
     id: string,
     workspaceId: string,
   ): Promise<AiMcpServer | undefined> {
-    return this.db
+    const row = await this.db
       .selectFrom('aiMcpServers')
       .selectAll('aiMcpServers')
       .where('id', '=', id)
       .where('workspaceId', '=', workspaceId)
       .executeTakeFirst();
+    return row ? normalizeRow(row) : row;
   }
 
   async listByWorkspace(workspaceId: string): Promise<AiMcpServer[]> {
-    return this.db
+    const rows = await this.db
       .selectFrom('aiMcpServers')
       .selectAll('aiMcpServers')
       .where('workspaceId', '=', workspaceId)
       .orderBy('createdAt', 'asc')
       .execute();
+    return rows.map(normalizeRow);
   }
 
   /** Enabled servers only — used by the agent loop to build the toolset. */
   async listEnabled(workspaceId: string): Promise<AiMcpServer[]> {
-    return this.db
+    const rows = await this.db
       .selectFrom('aiMcpServers')
       .selectAll('aiMcpServers')
       .where('workspaceId', '=', workspaceId)
       .where('enabled', '=', true)
       .orderBy('createdAt', 'asc')
       .execute();
+    return rows.map(normalizeRow);
   }
 
   async insert(
@@ -130,6 +133,14 @@ export class AiMcpServerRepo {
  * Encode a string[] as a jsonb bind for the `tool_allowlist` column. Passing a
  * plain JS array to the postgres driver would serialize it as a Postgres array
  * literal (incompatible with jsonb), so we bind the JSON text and cast it.
+ *
+ * The cast is `::text::jsonb`, NOT `::jsonb`: if the parameter is bound straight
+ * to a jsonb cast, node-postgres infers its type as jsonb and JSON-stringifies
+ * the (already-JSON) string a SECOND time, so the column ends up holding a jsonb
+ * STRING SCALAR (`"[\"a\"]"`) instead of a jsonb ARRAY. Forcing the param through
+ * `::text` first binds it as text (sent verbatim), and `::jsonb` then parses it
+ * into a real array. (`normalizeRow` below repairs rows written the old way.)
+ *
  * Returns null for null/empty arrays (an empty allowlist means "no restriction"
  * is not intended — callers pass null to clear; an empty array is normalized to
  * null here so it never round-trips as `[]`).
@@ -139,5 +150,37 @@ function jsonbArray(value: string[] | null | undefined) {
     return null;
   }
   // Typed as string[] so it is assignable to the toolAllowlist column.
-  return sql<string[]>`${JSON.stringify(value)}::jsonb`;
+  return sql<string[]>`${JSON.stringify(value)}::text::jsonb`;
+}
+
+/**
+ * Parse the `toolAllowlist` value read from the DB into the `string[] | null`
+ * the entity type promises. The jsonb column historically round-trips as a JSON
+ * STRING (rows written by the old double-encoding `jsonbArray`, see above), so
+ * the driver hands back a string like `'["a","b"]'` rather than an array. Be
+ * tolerant: an already-parsed array passes through; a JSON string is parsed; null
+ * / a non-array / unparseable value becomes null (unrestricted).
+ */
+export function parseToolAllowlist(value: unknown): string[] | null {
+  if (value == null) return null;
+  if (Array.isArray(value)) {
+    return value.every((v) => typeof v === 'string') ? (value as string[]) : null;
+  }
+  if (typeof value === 'string') {
+    try {
+      const parsed = JSON.parse(value);
+      return Array.isArray(parsed) &&
+        parsed.every((v) => typeof v === 'string')
+        ? (parsed as string[])
+        : null;
+    } catch {
+      return null;
+    }
+  }
+  return null;
+}
+
+/** Normalize a DB row so `toolAllowlist` is always `string[] | null`. */
+function normalizeRow(row: AiMcpServer): AiMcpServer {
+  return { ...row, toolAllowlist: parseToolAllowlist(row.toolAllowlist) };
 }