From 60b7be35346496b19009b3a2c80284eba07525a7 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Thu, 25 Jun 2026 04:19:12 +0300 Subject: [PATCH] fix(db): jsonb double-encoding follow-ups from PR #172 review (#173) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #172 fixed the jsonb double-encoding for `tool_allowlist` but the same class of bug, and the same re-derived workaround, remained elsewhere. 1. model_config (agent roles): jsonbObject still used the buggy `::jsonb` bind, so `ai_agent_roles.model_config` round-tripped as a jsonb STRING SCALAR. The read-path `typeof === 'object'` check then failed and the model override was SILENTLY dropped (role fell back to the default model). Fixed to `::text::jsonb` and added `parseModelConfig` + `normalizeRow` so every read self-heals already-corrupted rows (no migration). 2. Centralized the write workaround as `jsonbBind()` in database/utils.ts — one implementation with one explanation of the quirk — replacing the per-repo `jsonbArray` (mcp) and `jsonbObject` (roles). 3. Integration coverage (the fix is a DB round-trip a unit test cannot see; the read-side parser MASKS a write regression): new ai-mcp-server-repo.int-spec asserts `jsonb_typeof(tool_allowlist)='array'` after insert + heals a seeded string-scalar row; ai-agent-roles-repo int-spec gains the same for `model_config` (`'object'` + heal). 4. Updated the stale `ai-mcp-servers.types.ts` comment (the driver returns a JSON string for legacy rows; the repo normalizes every read). 5. Fail-open logging: a corrupt tool_allowlist degrades to "no restriction" (agent gets ALL tools) — normalizeRow now warns (server id only, never contents) so the silent widening leaves a trace. 6. Simplified parseToolAllowlist (normalize the string once, then a single array-of-strings check) — identical behaviour, all 12 cases still pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../core/ai-chat/roles/jsonb-object.spec.ts | 30 ----- apps/server/src/database/jsonb-bind.spec.ts | 38 ++++++ .../ai-agent-roles.repo.spec.ts | 8 +- .../ai-agent-roles/ai-agent-roles.repo.ts | 67 ++++++++--- .../ai-agent-roles/parse-model-config.spec.ts | 46 ++++++++ .../repos/ai-chat/ai-mcp-server.repo.ts | 78 ++++++------- .../database/types/ai-mcp-servers.types.ts | 4 +- apps/server/src/database/utils.ts | 33 ++++++ .../ai-agent-roles-repo.int-spec.ts | 108 ++++++++++++++++-- .../ai-mcp-server-repo.int-spec.ts | 94 +++++++++++++++ 10 files changed, 402 insertions(+), 104 deletions(-) delete mode 100644 apps/server/src/core/ai-chat/roles/jsonb-object.spec.ts create mode 100644 apps/server/src/database/jsonb-bind.spec.ts create mode 100644 apps/server/src/database/repos/ai-agent-roles/parse-model-config.spec.ts create mode 100644 apps/server/test/integration/ai-mcp-server-repo.int-spec.ts diff --git a/apps/server/src/core/ai-chat/roles/jsonb-object.spec.ts b/apps/server/src/core/ai-chat/roles/jsonb-object.spec.ts deleted file mode 100644 index 96875748..00000000 --- a/apps/server/src/core/ai-chat/roles/jsonb-object.spec.ts +++ /dev/null @@ -1,30 +0,0 @@ -import { jsonbObject } from '@docmost/db/repos/ai-agent-roles/ai-agent-roles.repo'; - -/** - * Unit tests for jsonbObject: the repo helper that encodes a model_config object - * as a jsonb bind (or null when there is nothing to persist). It is the last - * line of defence before the column write, so the null-vs-bind decision is what - * matters here. We assert only null vs non-null because the non-null value is a - * kysely `sql` template fragment whose internal shape is an implementation - * detail of the SQL tag. - */ -describe('jsonbObject', () => { - it('returns null for null', () => { - expect(jsonbObject(null)).toBeNull(); - }); - - it('returns null for undefined', () => { - expect(jsonbObject(undefined)).toBeNull(); - }); - - it('returns null for an empty object (nothing to persist)', () => { - expect(jsonbObject({})).toBeNull(); - }); - - it('returns a (non-null) jsonb bind for a non-empty object', () => { - const out = jsonbObject({ driver: 'gemini', chatModel: 'gemini-2.0-flash' }); - // A real sql fragment is produced, never null/undefined. - expect(out).not.toBeNull(); - expect(out).toBeDefined(); - }); -}); diff --git a/apps/server/src/database/jsonb-bind.spec.ts b/apps/server/src/database/jsonb-bind.spec.ts new file mode 100644 index 00000000..4e9d3ffa --- /dev/null +++ b/apps/server/src/database/jsonb-bind.spec.ts @@ -0,0 +1,38 @@ +import { jsonbBind } from './utils'; + +/** + * Unit tests for jsonbBind: THE shared helper that encodes a JS array/object as + * a jsonb bind (or null when there is nothing to persist). It is the last line + * of defence before a jsonb column write, so the null-vs-bind decision is what + * matters here. We assert only null vs non-null because the non-null value is a + * kysely `sql` template fragment whose internal shape is an implementation + * detail of the SQL tag (the `::text::jsonb` double-encoding fix is verified + * end-to-end by the repo integration specs, where a real DB round-trip can + * actually observe `jsonb_typeof`). + */ +describe('jsonbBind', () => { + it('returns null for null / undefined', () => { + expect(jsonbBind(null)).toBeNull(); + expect(jsonbBind(undefined)).toBeNull(); + }); + + it('returns null for an empty array (nothing to persist)', () => { + expect(jsonbBind([])).toBeNull(); + }); + + it('returns null for an empty object (nothing to persist)', () => { + expect(jsonbBind({})).toBeNull(); + }); + + it('returns a (non-null) bind for a non-empty array', () => { + const out = jsonbBind(['search', 'crawl']); + expect(out).not.toBeNull(); + expect(out).toBeDefined(); + }); + + it('returns a (non-null) bind for a non-empty object', () => { + const out = jsonbBind({ driver: 'gemini', chatModel: 'gemini-2.0-flash' }); + expect(out).not.toBeNull(); + expect(out).toBeDefined(); + }); +}); diff --git a/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.spec.ts b/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.spec.ts index 723c7627..3f1d2ede 100644 --- a/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.spec.ts +++ b/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.spec.ts @@ -35,7 +35,13 @@ describe('AiAgentRoleRepo.findLiveEnabled', () => { const result = await repo.findLiveEnabled('r-1', 'ws-1'); - expect(result).toBe(role); + // The repo normalizes the row (modelConfig parse), so it returns a COPY, not + // the same reference; assert the row's fields are carried through. + expect(result).toMatchObject({ + id: 'r-1', + workspaceId: 'ws-1', + enabled: true, + }); expect(db.selectFrom).toHaveBeenCalledWith('aiAgentRoles'); // Every security filter must be present. expect(where).toHaveBeenCalledWith('id', '=', 'r-1'); diff --git a/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.ts b/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.ts index fb950585..1621b3e5 100644 --- a/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.ts +++ b/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.ts @@ -1,8 +1,7 @@ import { Injectable } from '@nestjs/common'; import { InjectKysely } from 'nestjs-kysely'; -import { sql } from 'kysely'; import { KyselyDB, KyselyTransaction } from '../../types/kysely.types'; -import { dbOrTx } from '../../utils'; +import { dbOrTx, jsonbBind } from '../../utils'; import { AiAgentRole } from '@docmost/db/types/entity.types'; /** The jsonb shape persisted in `model_config` (loosely typed for the column). */ @@ -23,13 +22,14 @@ export class AiAgentRoleRepo { id: string, workspaceId: string, ): Promise { - return this.db + const row = await this.db .selectFrom('aiAgentRoles') .selectAll('aiAgentRoles') .where('id', '=', id) .where('workspaceId', '=', workspaceId) .where('deletedAt', 'is', null) .executeTakeFirst(); + return row ? normalizeRow(row) : row; } /** @@ -45,7 +45,7 @@ export class AiAgentRoleRepo { id: string, workspaceId: string, ): Promise { - return this.db + const row = await this.db .selectFrom('aiAgentRoles') .selectAll('aiAgentRoles') .where('id', '=', id) @@ -53,17 +53,19 @@ export class AiAgentRoleRepo { .where('deletedAt', 'is', null) .where('enabled', '=', true) .executeTakeFirst(); + return row ? normalizeRow(row) : row; } /** All live roles for the workspace (management list + chat picker). */ async listByWorkspace(workspaceId: string): Promise { - return this.db + const rows = await this.db .selectFrom('aiAgentRoles') .selectAll('aiAgentRoles') .where('workspaceId', '=', workspaceId) .where('deletedAt', 'is', null) .orderBy('createdAt', 'asc') .execute(); + return rows.map(normalizeRow); } async insert( @@ -83,7 +85,7 @@ export class AiAgentRoleRepo { trx?: KyselyTransaction, ): Promise { const db = dbOrTx(this.db, trx); - return db + const row = await db .insertInto('aiAgentRoles') .values({ workspaceId: values.workspaceId, @@ -92,7 +94,11 @@ export class AiAgentRoleRepo { emoji: values.emoji ?? null, description: values.description ?? null, instructions: values.instructions, - modelConfig: jsonbObject(values.modelConfig), + // Cast: the generated `model_config` column type is the broad JsonValue + // union, which the concrete RawBuilder is not structurally + // assignable to (same reason the old jsonbObject cast to any). + // eslint-disable-next-line @typescript-eslint/no-explicit-any + modelConfig: jsonbBind(values.modelConfig) as any, enabled: values.enabled ?? true, autoStart: values.autoStart ?? true, // Empty string is treated as "no custom text" => null. @@ -100,6 +106,7 @@ export class AiAgentRoleRepo { }) .returningAll() .executeTakeFirst(); + return normalizeRow(row); } async update( @@ -127,7 +134,7 @@ export class AiAgentRoleRepo { if (patch.description !== undefined) set.description = patch.description; if (patch.instructions !== undefined) set.instructions = patch.instructions; if (patch.modelConfig !== undefined) { - set.modelConfig = jsonbObject(patch.modelConfig); + set.modelConfig = jsonbBind(patch.modelConfig); } if (patch.enabled !== undefined) set.enabled = patch.enabled; if (patch.autoStart !== undefined) set.autoStart = patch.autoStart; @@ -163,16 +170,40 @@ export class AiAgentRoleRepo { } /** - * Encode an object as a jsonb bind for the `model_config` column. The postgres - * driver would otherwise need an explicit cast; bind the JSON text and cast it. - * Returns null for null/undefined/empty objects. Cast to `any` because the - * generated column type is the broad `JsonValue` union, which a concrete object - * type is not structurally assignable to. + * Parse the `model_config` value read from the DB into the object the entity + * type promises. Rows written by the old double-encoding bind (`::jsonb` instead + * of `::text::jsonb`) round-trip as a JSON STRING, so the driver hands back e.g. + * `'{"driver":"gemini"}'` rather than an object; the read-path check + * `typeof cfg === 'object'` then failed and the model override was SILENTLY + * dropped (the role fell back to the default model). Be tolerant: a JSON string + * is parsed; an already-parsed object passes through; null / a non-object (incl. + * an array) / unparseable value becomes null (= no override). This self-heals + * already-corrupted rows on read, no migration required. */ -export function jsonbObject(value: ModelConfigValue | undefined) { - if (value === null || value === undefined || Object.keys(value).length === 0) { - return null; +export function parseModelConfig( + value: unknown, +): Record | null { + let v: unknown = value; + if (typeof v === 'string') { + try { + v = JSON.parse(v); // legacy double-encoded read + } catch { + return null; + } } - // eslint-disable-next-line @typescript-eslint/no-explicit-any - return sql`${JSON.stringify(value)}::jsonb` as any; + return v !== null && typeof v === 'object' && !Array.isArray(v) + ? (v as Record) + : null; +} + +/** Normalize a DB row so `modelConfig` is always an object or null. The cast + * bridges parseModelConfig's concrete `Record | null` to the column's broad + * generated `JsonValue` type (an object is a valid JsonValue at runtime). */ +function normalizeRow(row: AiAgentRole): AiAgentRole { + return { + ...row, + modelConfig: parseModelConfig( + row.modelConfig, + ) as AiAgentRole['modelConfig'], + }; } diff --git a/apps/server/src/database/repos/ai-agent-roles/parse-model-config.spec.ts b/apps/server/src/database/repos/ai-agent-roles/parse-model-config.spec.ts new file mode 100644 index 00000000..16392305 --- /dev/null +++ b/apps/server/src/database/repos/ai-agent-roles/parse-model-config.spec.ts @@ -0,0 +1,46 @@ +import { parseModelConfig } from './ai-agent-roles.repo'; + +/** + * Unit tests for parseModelConfig: the read-side normalizer that repairs the + * jsonb double-encoding regression on `model_config`. Rows written by the old + * `::jsonb` bind round-trip as a JSON STRING, which the read path's + * `typeof === 'object'` check rejected — silently dropping the model override. + * parseModelConfig accepts an already-parsed object, parses a legacy JSON + * string, and rejects everything that is not an object (null = no override). + */ +describe('parseModelConfig', () => { + it('passes an already-parsed object through', () => { + expect(parseModelConfig({ driver: 'gemini' })).toEqual({ + driver: 'gemini', + }); + }); + + it('parses a legacy double-encoded JSON string into an object', () => { + expect(parseModelConfig('{"driver":"gemini","chatModel":"x"}')).toEqual({ + driver: 'gemini', + chatModel: 'x', + }); + }); + + it('returns null for null / undefined', () => { + expect(parseModelConfig(null)).toBeNull(); + expect(parseModelConfig(undefined)).toBeNull(); + }); + + it('returns null for a non-object JSON value (string/number/array)', () => { + expect(parseModelConfig('"justastring"')).toBeNull(); + expect(parseModelConfig('42')).toBeNull(); + // An array is an object in JS but not a valid model_config shape. + expect(parseModelConfig('["a","b"]')).toBeNull(); + expect(parseModelConfig(['a', 'b'])).toBeNull(); + }); + + it('returns null for an unparseable string', () => { + expect(parseModelConfig('not json at all')).toBeNull(); + }); + + it('returns null for a raw non-object primitive', () => { + expect(parseModelConfig(42 as unknown)).toBeNull(); + expect(parseModelConfig(true as unknown)).toBeNull(); + }); +}); diff --git a/apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.ts b/apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.ts index a0f2da50..f17d7485 100644 --- a/apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.ts +++ b/apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.ts @@ -1,10 +1,11 @@ -import { Injectable } from '@nestjs/common'; +import { Injectable, Logger } from '@nestjs/common'; import { InjectKysely } from 'nestjs-kysely'; -import { sql } from 'kysely'; import { KyselyDB, KyselyTransaction } from '../../types/kysely.types'; -import { dbOrTx } from '../../utils'; +import { dbOrTx, jsonbBind } from '../../utils'; import { AiMcpServer } from '@docmost/db/types/entity.types'; +const logger = new Logger('AiMcpServerRepo'); + /** * Repository for per-workspace external MCP servers the agent may use (§5.4). * @@ -75,7 +76,7 @@ export class AiMcpServerRepo { headersEnc: values.headersEnc ?? null, // jsonb column: the postgres driver would otherwise encode a JS array as // a Postgres array literal. Bind the JSON text and cast it to jsonb. - toolAllowlist: jsonbArray(values.toolAllowlist), + toolAllowlist: jsonbBind(values.toolAllowlist), enabled: values.enabled ?? true, }) .returningAll() @@ -104,7 +105,7 @@ export class AiMcpServerRepo { if (patch.url !== undefined) set.url = patch.url; if (patch.headersEnc !== undefined) set.headersEnc = patch.headersEnc; if (patch.toolAllowlist !== undefined) { - set.toolAllowlist = jsonbArray(patch.toolAllowlist); + set.toolAllowlist = jsonbBind(patch.toolAllowlist); } if (patch.enabled !== undefined) set.enabled = patch.enabled; await db @@ -129,58 +130,43 @@ 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 `[]`). - */ -function jsonbArray(value: string[] | null | undefined) { - if (value === null || value === undefined || value.length === 0) { - return null; - } - // Typed as string[] so it is assignable to the toolAllowlist column. - return sql`${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). + * STRING (rows written by the old double-encoding bind before the `::text::jsonb` + * fix), so the driver hands back a string like `'["a","b"]'` rather than an + * array. Be tolerant: normalize a JSON string to its value, then accept it only + * if it is an array of strings; null / a non-array / unparseable value / an + * array with a non-string element all become 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') { + let v: unknown = value; + if (typeof v === 'string') { try { - const parsed = JSON.parse(value); - return Array.isArray(parsed) && - parsed.every((v) => typeof v === 'string') - ? (parsed as string[]) - : null; + v = JSON.parse(v); // legacy double-encoded read } catch { return null; } } - return null; + return Array.isArray(v) && v.every((x) => typeof x === 'string') + ? (v as string[]) + : null; } -/** Normalize a DB row so `toolAllowlist` is always `string[] | null`. */ +/** + * Normalize a DB row so `toolAllowlist` is always `string[] | null`. + * + * FAIL-OPEN logging: a stored value that is present but cannot be parsed into a + * string[] (corrupt JSON, a non-array, non-string elements) degrades to `null` = + * "no restriction", so the agent silently gets ALL of the server's tools. Log + * one line (server id only, never the contents) so that widening is not silent. + */ function normalizeRow(row: AiMcpServer): AiMcpServer { - return { ...row, toolAllowlist: parseToolAllowlist(row.toolAllowlist) }; + const parsed = parseToolAllowlist(row.toolAllowlist); + if (parsed === null && row.toolAllowlist != null) { + logger.warn( + `Corrupt tool_allowlist for MCP server ${row.id}; ignoring it (no tool restriction applied)`, + ); + } + return { ...row, toolAllowlist: parsed }; } diff --git a/apps/server/src/database/types/ai-mcp-servers.types.ts b/apps/server/src/database/types/ai-mcp-servers.types.ts index 677f45fe..c0d75622 100644 --- a/apps/server/src/database/types/ai-mcp-servers.types.ts +++ b/apps/server/src/database/types/ai-mcp-servers.types.ts @@ -20,7 +20,9 @@ export interface AiMcpServers { // Encrypted JSON of the auth headers. Nullable (a server may need no auth). headersEnc: string | null; // Optional allowlist of remote tool names to expose; null = expose all. - // Stored as jsonb; reads come back as a string[] from the postgres driver. + // Stored as jsonb. The postgres driver may return a JSON string for legacy + // double-encoded rows; `AiMcpServerRepo` normalizes every read to + // `string[] | null` via `parseToolAllowlist`. toolAllowlist: string[] | null; enabled: Generated; createdAt: Generated; diff --git a/apps/server/src/database/utils.ts b/apps/server/src/database/utils.ts index 6c11339c..c493798c 100644 --- a/apps/server/src/database/utils.ts +++ b/apps/server/src/database/utils.ts @@ -1,3 +1,4 @@ +import { sql, RawBuilder } from 'kysely'; import { KyselyDB, KyselyTransaction } from './types/kysely.types'; /* @@ -31,3 +32,35 @@ export function dbOrTx( return db; // Use normal database instance } } + +/** + * Bind a JS array/object as a `jsonb` column value, working around a postgres + * driver double-encoding quirk. THE single implementation — repos that persist + * jsonb (`tool_allowlist`, `model_config`, ...) call this instead of re-deriving + * the cast. + * + * THE QUIRK: with the `kysely-postgres-js` / postgres.js driver, casting a bound + * parameter straight to `::jsonb` makes the driver infer the param type as jsonb + * and JSON-stringify the (already-JSON) text a SECOND time, so the column ends + * up holding a jsonb STRING SCALAR (`"[\"a\"]"` / `"{\"k\":1}"`) instead of a + * real jsonb array/object. Read paths then see a string, not the structure, and + * silently fall back (an allowlist becomes "unrestricted", a model override is + * ignored). Forcing the param through `::text` first binds it as text (sent + * verbatim); `::jsonb` then parses it into a real array/object. Read-side + * parsers repair rows written the old buggy way without a migration. + * + * Returns `null` for null/undefined and for "empty" values (an empty array, or + * an object with no own enumerable keys) — callers treat empty as "clear/unset", + * so an empty allowlist/config never round-trips as `[]`/`{}`. + */ +export function jsonbBind( + value: T | null | undefined, +): RawBuilder | null { + if (value === null || value === undefined) return null; + if (Array.isArray(value)) { + if (value.length === 0) return null; + } else if (typeof value === 'object') { + if (Object.keys(value as object).length === 0) return null; + } + return sql`${JSON.stringify(value)}::text::jsonb`; +} diff --git a/apps/server/test/integration/ai-agent-roles-repo.int-spec.ts b/apps/server/test/integration/ai-agent-roles-repo.int-spec.ts index 9129dd75..454a6e1d 100644 --- a/apps/server/test/integration/ai-agent-roles-repo.int-spec.ts +++ b/apps/server/test/integration/ai-agent-roles-repo.int-spec.ts @@ -1,4 +1,5 @@ -import { Kysely } from 'kysely'; +import { Kysely, sql } from 'kysely'; +import { randomUUID } from 'node:crypto'; import { AiAgentRoleRepo } from '@docmost/db/repos/ai-agent-roles/ai-agent-roles.repo'; import { getTestDb, destroyTestDb, createWorkspace } from './db'; @@ -25,8 +26,16 @@ describe('AiAgentRoleRepo isolation + partial unique index [integration]', () => }); it('findById / listByWorkspace exclude soft-deleted rows', async () => { - const live = await repo.insert({ workspaceId: w1, name: 'Live', instructions: 'x' }); - const dead = await repo.insert({ workspaceId: w1, name: 'Dead', instructions: 'x' }); + const live = await repo.insert({ + workspaceId: w1, + name: 'Live', + instructions: 'x', + }); + const dead = await repo.insert({ + workspaceId: w1, + name: 'Dead', + instructions: 'x', + }); await repo.softDelete(dead.id, w1); expect(await repo.findById(live.id, w1)).toBeDefined(); @@ -38,7 +47,11 @@ describe('AiAgentRoleRepo isolation + partial unique index [integration]', () => }); it('findById of a W2 role from W1 context returns undefined (tenant isolation)', async () => { - const w2role = await repo.insert({ workspaceId: w2, name: 'W2Role', instructions: 'x' }); + const w2role = await repo.insert({ + workspaceId: w2, + name: 'W2Role', + instructions: 'x', + }); expect(await repo.findById(w2role.id, w2)).toBeDefined(); // Same id, wrong workspace context -> not visible. @@ -58,21 +71,100 @@ describe('AiAgentRoleRepo isolation + partial unique index [integration]', () => }); it('same name is reusable after softDelete (partial unique index WHERE deleted_at IS NULL)', async () => { - const first = await repo.insert({ workspaceId: w1, name: 'Reusable', instructions: 'x' }); + const first = await repo.insert({ + workspaceId: w1, + name: 'Reusable', + instructions: 'x', + }); await repo.softDelete(first.id, w1); // Now inserting the same name must succeed because the soft-deleted row is // excluded from the partial unique index. - const second = await repo.insert({ workspaceId: w1, name: 'Reusable', instructions: 'x' }); + const second = await repo.insert({ + workspaceId: w1, + name: 'Reusable', + instructions: 'x', + }); expect(second.id).toBeDefined(); expect(second.id).not.toBe(first.id); }); it('same name in W1 and W2 is allowed (unique is per-workspace)', async () => { - const a = await repo.insert({ workspaceId: w1, name: 'CrossTenant', instructions: 'x' }); - const b = await repo.insert({ workspaceId: w2, name: 'CrossTenant', instructions: 'x' }); + const a = await repo.insert({ + workspaceId: w1, + name: 'CrossTenant', + instructions: 'x', + }); + const b = await repo.insert({ + workspaceId: w2, + name: 'CrossTenant', + instructions: 'x', + }); expect(a.id).toBeDefined(); expect(b.id).toBeDefined(); expect(a.id).not.toBe(b.id); }); + + // model_config jsonb round-trip (issue #173 §1): the same double-encoding bug + // PR #172 fixed for tool_allowlist lived in jsonbObject. A DB round-trip is the + // only way to observe it — the write must land as a real jsonb OBJECT, and a + // legacy string-scalar row must self-heal on read (else the model override is + // silently dropped and the role falls back to the default model). + const jsonbTypeof = async (id: string): Promise => { + const res = await sql<{ t: string | null }>` + SELECT jsonb_typeof(model_config) AS t + FROM ai_agent_roles WHERE id = ${id} + `.execute(db); + return res.rows[0]?.t ?? null; + }; + + it('insert stores model_config as a jsonb OBJECT and reads it back as an object', async () => { + const role = await repo.insert({ + workspaceId: w1, + name: `Model-${randomUUID()}`, + instructions: 'x', + modelConfig: { driver: 'gemini', chatModel: 'gemini-2.0-flash' }, + }); + expect(await jsonbTypeof(role.id)).toBe('object'); + // The returned row is already normalized to an object. + expect(role.modelConfig).toEqual({ + driver: 'gemini', + chatModel: 'gemini-2.0-flash', + }); + const found = await repo.findById(role.id, w1); + expect(found?.modelConfig).toEqual({ + driver: 'gemini', + chatModel: 'gemini-2.0-flash', + }); + }); + + it('an empty model_config is normalized to null (no override)', async () => { + const role = await repo.insert({ + workspaceId: w1, + name: `Empty-${randomUUID()}`, + instructions: 'x', + modelConfig: {}, + }); + // The column is SQL NULL, so jsonb_typeof returns SQL NULL (JS null). + expect(await jsonbTypeof(role.id)).toBeNull(); + expect((await repo.findById(role.id, w1))?.modelConfig).toBeNull(); + }); + + it('repairs a legacy double-encoded (string scalar) model_config on read', async () => { + const id = randomUUID(); + // Seed the corrupt string-scalar shape the old `::jsonb` bind produced. + await sql` + INSERT INTO ai_agent_roles (id, workspace_id, name, instructions, model_config) + VALUES ( + ${id}, ${w1}, ${`Legacy-${id}`}, 'x', + to_jsonb(${'{"driver":"openai","chatModel":"gpt"}'}::text) + ) + `.execute(db); + expect(await jsonbTypeof(id)).toBe('string'); // sanity: really corrupt + + expect((await repo.findById(id, w1))?.modelConfig).toEqual({ + driver: 'openai', + chatModel: 'gpt', + }); + }); }); diff --git a/apps/server/test/integration/ai-mcp-server-repo.int-spec.ts b/apps/server/test/integration/ai-mcp-server-repo.int-spec.ts new file mode 100644 index 00000000..c1949a57 --- /dev/null +++ b/apps/server/test/integration/ai-mcp-server-repo.int-spec.ts @@ -0,0 +1,94 @@ +import { Kysely, sql } from 'kysely'; +import { randomUUID } from 'node:crypto'; +import { AiMcpServerRepo } from '@docmost/db/repos/ai-chat/ai-mcp-server.repo'; +import { getTestDb, destroyTestDb, createWorkspace } from './db'; + +/** + * AiMcpServerRepo `tool_allowlist` jsonb round-trip (PR #172 / issue #173 §3). + * + * The fix under test is a DB round-trip, so a unit test cannot observe it: the + * write must land as a real jsonb ARRAY (not a double-encoded string scalar), + * and the read must repair any legacy string-scalar rows. The read-side + * `parseToolAllowlist` MASKS a write regression (it parses the string back), so + * without this integration check, reverting `::text::jsonb` to `::jsonb` would + * keep every unit test green while silently corrupting the column again. + */ +describe('AiMcpServerRepo tool_allowlist jsonb round-trip [integration]', () => { + let db: Kysely; + let repo: AiMcpServerRepo; + let ws: string; + + beforeAll(async () => { + db = getTestDb(); + repo = new AiMcpServerRepo(db as any); + ws = (await createWorkspace(db)).id; + }); + + afterAll(async () => { + await destroyTestDb(); + }); + + const jsonbTypeof = async (id: string): Promise => { + const res = await sql<{ t: string | null }>` + SELECT jsonb_typeof(tool_allowlist) AS t + FROM ai_mcp_servers WHERE id = ${id} + `.execute(db); + return res.rows[0]?.t ?? null; + }; + + it('insert stores the allowlist as a jsonb ARRAY (not a string scalar)', async () => { + const row = await repo.insert({ + workspaceId: ws, + name: `srv-${randomUUID()}`, + transport: 'http', + url: 'https://example.com/mcp', + toolAllowlist: ['search', 'crawl'], + }); + + // The column holds a real jsonb array — the whole point of ::text::jsonb. + expect(await jsonbTypeof(row.id)).toBe('array'); + + // And the read returns a genuine string[], not a JSON string. + const found = await repo.findById(row.id, ws); + expect(found?.toolAllowlist).toEqual(['search', 'crawl']); + expect(Array.isArray(found?.toolAllowlist)).toBe(true); + }); + + it('an empty allowlist is normalized to null (no restriction), not []', async () => { + const row = await repo.insert({ + workspaceId: ws, + name: `srv-${randomUUID()}`, + transport: 'http', + url: 'https://example.com/mcp', + toolAllowlist: [], + }); + // The column is SQL NULL, so jsonb_typeof returns SQL NULL (JS null). + expect(await jsonbTypeof(row.id)).toBeNull(); + expect((await repo.findById(row.id, ws))?.toolAllowlist).toBeNull(); + }); + + it('repairs a legacy double-encoded (string scalar) row on read (self-heal)', async () => { + // Seed a row whose tool_allowlist is a jsonb STRING SCALAR holding the JSON + // text — exactly what the old `::jsonb` double-encoding produced. + const id = randomUUID(); + await sql` + INSERT INTO ai_mcp_servers (id, workspace_id, name, transport, url, tool_allowlist) + VALUES ( + ${id}, ${ws}, ${`srv-${id}`}, 'http', 'https://example.com/mcp', + to_jsonb(${'["alpha","beta"]'}::text) + ) + `.execute(db); + + // Sanity: the seeded column really IS the corrupt string-scalar shape. + expect(await jsonbTypeof(id)).toBe('string'); + + // The repo read heals it back to a real string[]. + expect((await repo.findById(id, ws))?.toolAllowlist).toEqual([ + 'alpha', + 'beta', + ]); + const enabled = await repo.listEnabled(ws); + const healed = enabled.find((r) => r.id === id); + expect(healed?.toolAllowlist).toEqual(['alpha', 'beta']); + }); +});