Хвосты по ревью PR #172: тот же jsonb double-encoding в jsonbObject (роли агента) + централизация и тесты #173

New Issue

vvzvlad · 2026-06-24T20:46:04+03:00

vvzvlad commented

2026-06-24 20:46:04 +03:00

Хвосты по ревью PR #172 (fix(mcp): tool allowlist stored/read as jsonb string, not array). Сам PR корректен и смержен — здесь то, что надо доделать отдельно. Пункты 1–2 — из архитектурной части ревью (взят Вариант A в обоих), остальное — замечания из того же ревью.

1. Починить тот же баг double-encoding в `jsonbObject` (роли агента) — приоритет

Тот же класс ошибки, что чинил PR #172 для jsonbArray, остался нетронутым в соседнем хелпере.

Файл: apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.ts:177 — jsonbObject пишет sql\${JSON.stringify(value)}::jsonb`, байт-в-байт баговая форма (диалект kysely-postgres-js`/postgres.js пере-сериализует параметр → в колонку ложится jsonb string scalar, а не объект).
Колонка ai_agent_roles.model_config почти наверняка уже хранит double-encoded строки. Отказ тихий: read-путь делает typeof cfg !== 'object' → строковый override молча игнорируется и роль откатывается к дефолтной модели (а toView отдаёт в UI строку вместо объекта).
Надо (Вариант A, зеркально PR #172):
- write: ::jsonb → ::text::jsonb;
- read: добавить parseModelConfig + normalizeRow (чинит уже испорченные строки на чтении, без миграции);
- интеграционный тест на round-trip: после insert проверить jsonb_typeof(model_config) = 'object' и что чтение отдаёт объект.

2. Централизовать обход jsonb double-encoding (Вариант A)

Один и тот же трюк уже переоткрыт трижды: workspace.repo.ts (::text-касты + jsonb_typeof self-heal), ai-mcp-server.repo.ts (PR #172) и ai-agent-roles.repo.ts (ещё на баговой форме, см. п.1). Дублирование уже разъехалось.

Вынести общий jsonbBind() (write) в apps/server/src/database/utils.ts, заменить per-repo jsonbArray/jsonbObject на один реализованный-один-раз хелпер с одним комментарием-объяснением квирка.
(опц.) общие read-парсеры под формы колонок (parseJsonbArray/parseJsonbObject), куда переедут юнит-тесты.
Прим.: jsonb_build_object-пути в workspace.repo.ts структурно другие — остаются отдельными. Глобальный кодек на уровне Kysely/диалекта сознательно НЕ берём (непропорциональный риск ради ~3 колонок, задевает тяжёлый ProseMirror-контент страниц).

3. Интеграционный тест на write-каст `::text::jsonb` для allowlist — надо

Добавить apps/server/test/integration/ai-mcp-server-repo.int-spec.ts: после repo.insert({ toolAllowlist: ['a','b'] }) проверить сырым запросом jsonb_typeof(tool_allowlist) = 'array' и что findById вернул настоящий string[]; опционально засеять заведомо double-encoded строку и проверить, что listEnabled/findById её чинят.
Зачем: чинимая ошибка — это DB round-trip, юнит-тест её не ловит. Хуже того, read-side parseToolAllowlist маскирует регресс: откат каста обратно на ::jsonb оставит все юнит-тесты зелёными, а колонку снова молча испортит. Харнесс уже есть (test:int / apps/server/test/jest-integration.json, рядом — другие *.int-spec.ts).

4. Обновить устаревший комментарий типа сущности — надо

apps/server/src/database/types/ai-mcp-servers.types.ts:22-24 — строка «reads come back as a string[] from the postgres driver» теперь вводит в заблуждение (драйвер для legacy-строк отдаёт JSON-строку; string[] гарантирует уже normalizeRow/parseToolAllowlist). Переписать, например: «Stored as jsonb. The pg driver may return a JSON string for legacy double-encoded rows; AiMcpServerRepo normalizes every read to string[] | null via parseToolAllowlist.»

5. Логировать fail-open при битом allowlist — надо

apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.ts (parseToolAllowlist): в «corrupt»-ветках (строка не распарсилась / распарсилась в не-массив / не-строковые элементы) писать одну строку warn с id сервера (без содержимого), прежде чем вернуть null. Сейчас битый allowlist молча означает «без ограничений» → агенту уходят все инструменты сервера, без следа в логах.

6. Упростить `parseToolAllowlist` — по желанию

Убрать дублирование проверки массива между ветками: нормализовать строку в значение, затем одна проверка (поведение для всех 8 тест-кейсов идентично):

export function parseToolAllowlist(value: unknown): string[] | null {
  let v: unknown = value;
  if (typeof v === 'string') {
    try { v = JSON.parse(v); } catch { return null; } // legacy double-encoded read
  }
  return Array.isArray(v) && v.every((x) => typeof x === 'string')
    ? (v as string[])
    : null;
}

Источник: код-ревью PR #172.

Хвосты по ревью **PR #172** (`fix(mcp): tool allowlist stored/read as jsonb string, not array`). Сам PR корректен и смержен — здесь то, что надо доделать отдельно. Пункты 1–2 — из архитектурной части ревью (взят Вариант A в обоих), остальное — замечания из того же ревью. ## 1. Починить тот же баг double-encoding в `jsonbObject` (роли агента) — приоритет Тот же класс ошибки, что чинил PR #172 для `jsonbArray`, остался нетронутым в соседнем хелпере. - Файл: `apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.ts:177` — `jsonbObject` пишет `sql\`${JSON.stringify(value)}::jsonb\``, байт-в-байт баговая форма (диалект `kysely-postgres-js`/postgres.js пере-сериализует параметр → в колонку ложится jsonb **string scalar**, а не объект). - Колонка `ai_agent_roles.model_config` почти наверняка уже хранит double-encoded строки. Отказ **тихий**: read-путь делает `typeof cfg !== 'object'` → строковый override молча игнорируется и роль откатывается к дефолтной модели (а `toView` отдаёт в UI строку вместо объекта). - Надо (Вариант A, зеркально PR #172): - [ ] write: `::jsonb` → `::text::jsonb`; - [ ] read: добавить `parseModelConfig` + `normalizeRow` (чинит уже испорченные строки на чтении, без миграции); - [ ] интеграционный тест на round-trip: после `insert` проверить `jsonb_typeof(model_config) = 'object'` и что чтение отдаёт объект. ## 2. Централизовать обход jsonb double-encoding (Вариант A) Один и тот же трюк уже переоткрыт трижды: `workspace.repo.ts` (`::text`-касты + `jsonb_typeof` self-heal), `ai-mcp-server.repo.ts` (PR #172) и `ai-agent-roles.repo.ts` (ещё на баговой форме, см. п.1). Дублирование уже разъехалось. - [ ] Вынести общий `jsonbBind()` (write) в `apps/server/src/database/utils.ts`, заменить per-repo `jsonbArray`/`jsonbObject` на один реализованный-один-раз хелпер с одним комментарием-объяснением квирка. - [ ] (опц.) общие read-парсеры под формы колонок (`parseJsonbArray`/`parseJsonbObject`), куда переедут юнит-тесты. - Прим.: `jsonb_build_object`-пути в `workspace.repo.ts` структурно другие — остаются отдельными. Глобальный кодек на уровне Kysely/диалекта сознательно НЕ берём (непропорциональный риск ради ~3 колонок, задевает тяжёлый ProseMirror-контент страниц). ## 3. Интеграционный тест на write-каст `::text::jsonb` для allowlist — надо - [ ] Добавить `apps/server/test/integration/ai-mcp-server-repo.int-spec.ts`: после `repo.insert({ toolAllowlist: ['a','b'] })` проверить сырым запросом `jsonb_typeof(tool_allowlist) = 'array'` и что `findById` вернул настоящий `string[]`; опционально засеять заведомо double-encoded строку и проверить, что `listEnabled`/`findById` её чинят. - Зачем: чинимая ошибка — это DB round-trip, юнит-тест её не ловит. Хуже того, read-side `parseToolAllowlist` **маскирует** регресс: откат каста обратно на `::jsonb` оставит все юнит-тесты зелёными, а колонку снова молча испортит. Харнесс уже есть (`test:int` / `apps/server/test/jest-integration.json`, рядом — другие `*.int-spec.ts`). ## 4. Обновить устаревший комментарий типа сущности — надо - [ ] `apps/server/src/database/types/ai-mcp-servers.types.ts:22-24` — строка «reads come back as a string[] from the postgres driver» теперь вводит в заблуждение (драйвер для legacy-строк отдаёт JSON-строку; `string[]` гарантирует уже `normalizeRow`/`parseToolAllowlist`). Переписать, например: «Stored as jsonb. The pg driver may return a JSON string for legacy double-encoded rows; `AiMcpServerRepo` normalizes every read to `string[] | null` via `parseToolAllowlist`.» ## 5. Логировать fail-open при битом allowlist — надо - [ ] `apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.ts` (`parseToolAllowlist`): в «corrupt»-ветках (строка не распарсилась / распарсилась в не-массив / не-строковые элементы) писать одну строку `warn` с id сервера (без содержимого), прежде чем вернуть `null`. Сейчас битый allowlist молча означает «без ограничений» → агенту уходят все инструменты сервера, без следа в логах. ## 6. Упростить `parseToolAllowlist` — по желанию - [ ] Убрать дублирование проверки массива между ветками: нормализовать строку в значение, затем одна проверка (поведение для всех 8 тест-кейсов идентично): ```ts export function parseToolAllowlist(value: unknown): string[] | null { let v: unknown = value; if (typeof v === 'string') { try { v = JSON.parse(v); } catch { return null; } // legacy double-encoded read } return Array.isArray(v) && v.every((x) => typeof x === 'string') ? (v as string[]) : null; } ``` --- _Источник: код-ревью PR #172._

vvzvlad added the bug refactor test labels 2026-06-24 20:47:37 +03:00

Ghost referenced this issue from a commit

2026-06-25 04:20:02 +03:00

fix(db): jsonb double-encoding follow-ups from PR #172 review (#173)

Ghost referenced a pull request that will close this issue

2026-06-25 05:17:38 +03:00

Батч: ai-chat/footnotes/mcp/db/tree + red-team (#163 #181 #164 #173 #168 #180 + #159 8/10) #185

Ghost referenced this issue from a commit

2026-06-25 05:21:02 +03:00

fix(db): jsonb double-encoding follow-ups from PR #172 review (#173)

Ghost referenced this issue from a commit

2026-06-25 11:36:59 +03:00