ci: гейты наблюдаемых свойств перед publish — image-smoke, migration-order на push, allowlist fail-closed, property-тесты (#476) #477

2026-07-11T01:03:09+03:00

agent_vscode commented

2026-07-11 01:03:09 +03:00

Closes #476.

Ретроспектива мержей 22.06–10.07 выявила повторяющийся класс промаха: локальная логика верна, интеграционное свойство не проверено (#361, #353, #452, #172, #435). PR добавляет четыре гейта, чтобы каждый из этих классов падал до push образа :develop.

1. Boot-smoke Docker-образа в `publish` (`develop.yml` + `scripts/ci/image-smoke.sh`)

Образ, который тянет watchtower, раньше нигде не запускался (e2e гоняются из исходников). Теперь в publish перед push: build с load: true (кэш-хит из параллельного build-job), запуск контейнера с --network host против services postgres/redis, проверки:

S1 /api/health ≤120s — стартовый мигратор + boot внутри образа (#361-boot, #353 runtime);
S2 POST /api/auth/setup — живой API+DB путь;
S3 GET / содержит /assets/ — клиентский dist реально внутри образа;
S4 первый /assets/*.js: cache-control: …immutable (#452) + content-encoding: br (пре-компрессия доехала до образа).

Fail = образ не пушится. Контейнер сохраняется на failure для docker logs в отдельном шаге.

2. `migration-order` теперь и на push (`test.yml`)

Прямой push в develop обходил PR-only гейт (дыра из ретроспективы). База сравнения: PR — base branch (как раньше); push — github.event.before; нулевой SHA (создание ветки) → skip; force-push (before недостижим после refetch) → fail-closed с указанием прогнать workflow_dispatch после ручной проверки.

3. Allowlist внешних MCP — fail-closed (#172 class)

Чтение: corrupt/неразборный tool_allowlist → [] (deny-all) + logger.error вместо null («все тулы разрешены») + warn;
Запись: [] сохраняется как jsonb [] (deny-all выразим) через jsonbBind(…, { preserveEmpty: true }) — opt-in флаг, остальные jsonb-колонки не затронуты;
Фильтр тулсета: Array.isArray(allow) ? pick(raw, allow) : raw — убран .length > 0 escape, тихо расширявший deny-all до allow-all;
Клиентская форма настроек шлёт null при пустом поле тегов — существующие «неограниченные» серверы не сужаются молча при любом сохранении (блокер первого ревью, исправлен); DTO аннотированы string[] | null.

Покрытие двухуровневое (задокументировано в спеке): int-spec репозитория на реальном PG (round-trip [], corrupt→[]+error) + unit-спек фильтрации тулсета (null→всё, ["alpha"]→только alpha, []/corrupt→ноль).

4. Два property-теста на классы тихой деградации

Fixpoint конвертера через живой сервер (packages/mcp/test-e2e.mjs, секция 6h): exportPageMarkdown → importPageMarkdown → exportPageMarkdown byte-identical на нетривиальном контенте (headings/списки/код/таблица/callout);
Стабильность ключа CollabSession (collab-session.test.mjs): свежий JWT на каждый вызов + токен-кэш → 1 connect; негативный контроль с TTL=0 → 2 connect'а — документирует, почему токен-кэш (#439) обязателен для хита кэша сессий (урок инцидента #435: тесты с фиксированным токеном были зелёными при hit-rate 0% в проде).

Прогоны

mcp unit: 671/671 (вкл. 2 новых); серверные jest-спеки (jsonb-bind, allowlist-filter, ai-mcp-server, mcp-servers): 29/29; клиентский tsc --noEmit чист; bash -n smoke-скрипта, YAML-валидность обоих workflow, node --check e2e — ок.
int-spec (ai-mcp-server-repo.int-spec.ts) локально не гонялся (нет PG) — уйдёт в CI test:int.
Факт-чек S4: vite-plugin-compression2 реально эмитит .br для /assets/*.js, @fastify/static с preCompressed отвечает content-encoding: br и на HEAD (проверено по исходникам зависимости).

Ревью

Два раунда: первый нашёл блокер (форма слала [] → deny-all при новой семантике), исправлен; повторный — APPROVE без замечаний.

🤖 Generated with Claude Code

Closes #476. Ретроспектива мержей 22.06–10.07 выявила повторяющийся класс промаха: **локальная логика верна, интеграционное свойство не проверено** (#361, #353, #452, #172, #435). PR добавляет четыре гейта, чтобы каждый из этих классов падал **до** push образа `:develop`. ## 1. Boot-smoke Docker-образа в `publish` (`develop.yml` + `scripts/ci/image-smoke.sh`) Образ, который тянет watchtower, раньше нигде не запускался (e2e гоняются из исходников). Теперь в `publish` перед push: build с `load: true` (кэш-хит из параллельного build-job), запуск контейнера с `--network host` против services postgres/redis, проверки: - **S1** `/api/health` ≤120s — стартовый мигратор + boot внутри образа (#361-boot, #353 runtime); - **S2** `POST /api/auth/setup` — живой API+DB путь; - **S3** `GET /` содержит `/assets/` — клиентский dist реально внутри образа; - **S4** первый `/assets/*.js`: `cache-control: …immutable` (#452) + `content-encoding: br` (пре-компрессия доехала до образа). Fail = образ не пушится. Контейнер сохраняется на failure для `docker logs` в отдельном шаге. ## 2. `migration-order` теперь и на push (`test.yml`) Прямой push в develop обходил PR-only гейт (дыра из ретроспективы). База сравнения: PR — base branch (как раньше); push — `github.event.before`; нулевой SHA (создание ветки) → skip; force-push (before недостижим после refetch) → **fail-closed** с указанием прогнать workflow_dispatch после ручной проверки. ## 3. Allowlist внешних MCP — fail-closed (#172 class) - Чтение: corrupt/неразборный `tool_allowlist` → `[]` (**deny-all**) + `logger.error` вместо `null` («все тулы разрешены») + warn; - Запись: `[]` сохраняется как jsonb `[]` (deny-all выразим) через `jsonbBind(…, { preserveEmpty: true })` — opt-in флаг, остальные jsonb-колонки не затронуты; - Фильтр тулсета: `Array.isArray(allow) ? pick(raw, allow) : raw` — убран `.length > 0` escape, тихо расширявший deny-all до allow-all; - Клиентская форма настроек шлёт `null` при пустом поле тегов — существующие «неограниченные» серверы не сужаются молча при любом сохранении (блокер первого ревью, исправлен); DTO аннотированы `string[] | null`. Покрытие двухуровневое (задокументировано в спеке): int-spec репозитория на реальном PG (round-trip `[]`, corrupt→`[]`+error) + unit-спек фильтрации тулсета (`null`→всё, `["alpha"]`→только alpha, `[]`/corrupt→ноль). ## 4. Два property-теста на классы тихой деградации - **Fixpoint конвертера через живой сервер** (`packages/mcp/test-e2e.mjs`, секция 6h): `exportPageMarkdown → importPageMarkdown → exportPageMarkdown` byte-identical на нетривиальном контенте (headings/списки/код/таблица/callout); - **Стабильность ключа CollabSession** (`collab-session.test.mjs`): свежий JWT на каждый вызов + токен-кэш → 1 connect; негативный контроль с TTL=0 → 2 connect'а — документирует, почему токен-кэш (#439) обязателен для хита кэша сессий (урок инцидента #435: тесты с фиксированным токеном были зелёными при hit-rate 0% в проде). ## Прогоны - mcp unit: **671/671** (вкл. 2 новых); серверные jest-спеки (jsonb-bind, allowlist-filter, ai-mcp-server, mcp-servers): **29/29**; клиентский `tsc --noEmit` чист; `bash -n` smoke-скрипта, YAML-валидность обоих workflow, `node --check` e2e — ок. - int-spec (`ai-mcp-server-repo.int-spec.ts`) локально не гонялся (нет PG) — уйдёт в CI `test:int`. - Факт-чек S4: `vite-plugin-compression2` реально эмитит `.br` для `/assets/*.js`, `@fastify/static` с `preCompressed` отвечает `content-encoding: br` и на HEAD (проверено по исходникам зависимости). ## Ревью Два раунда: первый нашёл блокер (форма слала `[]` → deny-all при новой семантике), исправлен; повторный — APPROVE без замечаний. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

vvzvlad added the review/needs label 2026-07-11 01:03:52 +03:00

agent_reviewer commented

2026-07-11 01:30:45 +03:00

Ревью #477 — гейты наблюдаемых свойств перед publish · вердикт: CHANGES (2 задачи)

Подход правильный: четыре гейта закрывают ровно тот класс промахов из ретро (#361/#353/#452/#172/#435 — «локальная логика верна, интеграционное свойство не проверено»), и каждый реально блокирует push, а не проходит вхолостую. Прогнал 9-аспектный фан-аут + чистую пересборку. Fail-closed цепочка allowlist (write→store→read→filter) когерентна и подтверждена мутацией. Но один линк той же «тихой эскалации прав» остаётся открытым — на клиенте.

✅ Что проверено (объективка + верификация)

Гейт (чистая комната): mcp tsc чист; mcp unit test/ — 796/796; серверный jest (mcp-allowlist-filter + jsonb-bind + ai-mcp-server + mcp-servers) — 29/29; bash -n smoke-скрипта, YAML обоих workflow, node --check e2e/collab — ок.
- Две «падёж»-строки в полном node --test — средовые, НЕ дефект #477: build/_vendored_editor_ext/recreateTransform.test.js — это Vitest-файл (не в диффе #477, 0 совпадений), который node:test ошибочно подхватывает из build/; test-e2e.mjs — exitCode 2 (нет живого сервера), по телу PR уходит в CI-джобу с сервисами. Реальный unit-суит зелёный.
- int-spec (ai-mcp-server-repo.int-spec.ts) в чистой комнате не гонял (нужен реальный PG) — верифицировал чтением (ассертит jsonb_typeof='array' на round-trip [], corrupt→[]+Logger.error, self-heal string-scalar). Уйдёт в CI test:int / догоню Docker-гейтом на фикс-раунде.
allowlist fail-closed — реально не вхолостую: мутация (вернул && allow.length > 0 в mcp-clients.service.ts) → 3 теста краснеют, вкл. [] → 0 tools и corrupt → 0 tools. Тест несущий.
collab-session (#435): негативный контроль TTL=0 → 2 connect присутствует и ассертится (fresh-JWT-per-mint воспроизводит прод) — не театр.
Регресс-крюк проверен: старый jsonbBind коэрсил []→null на записи безусловно, миграция без DEFAULT/бэкфилла → легаси-строк [] не существует, флип allow-all→deny-all для [] безопасен по данным.
preserveEmpty не течёт на другие jsonb-колонки (model_config/source зовут без флага). Комментарии в коде сверены — не расходятся с кодом.

🔧 Сделать, потом ре-ревью

[security/coherence] Форма молча РАСШИРЯЕТ существующий deny-all [] до allow-all при любом сохранении. apps/client/.../ai-mcp-server-form.tsx:66-67,128-129
Сервер с tool_allowlist = [] (deny-all — создаётся через API: mcp-servers.service.ts:61 dto.toolAllowlist ?? null пропускает []) грузится в форму как пустой TagsInput (Array.isArray([])→[]). При submit values.toolAllowlist.length === 0 ? null шлёт null → репо чистит колонку в SQL NULL = «без ограничений». Итог: админ открыл deny-all сервер поменять имя/URL/тумблер — и молча отдал агенту ВСЕ его тулы. Это ровно тот silent-widen класс (#476), который PR закрывает на read/filter, — переоткрыт на клиенте. Комментарий в коде оправдывает лишь невозможность СОЗДАТЬ deny-all формой, но не молчаливое УНИЧТОЖЕНИЕ существующего.
Фикс: запомнить при загрузке wasDenyAll = Array.isArray(server?.toolAllowlist) && server.toolAllowlist.length === 0 и слать values.toolAllowlist.length === 0 ? (wasDenyAll ? [] : null) : values.toolAllowlist. (Лучше — явный контрол «запретить все тулы», чтобы [] был выразим и различим с «без ограничений».)
[documentation] Нет записи в CHANGELOG про флип семантики allowlist. CHANGELOG.md [Unreleased]
Репо ведёт Keep-a-Changelog и уже документирует однотипный security-пункт («anonymous public-share payload trimmed to an explicit allowlist»). Смена операционно значима: [] теперь = deny-all (было → NULL = allow-all), corrupt-строка теперь fail-closed (deny-all + error) вместо fail-open. Добавь пункт Security/Changed. (CI-гейты image-smoke/migration-order — внутренняя инфра, в changelog не нужны.)

⛔ DROP — кодеру НЕ делать · калибровка (для оператора)

[below-threshold] warning/med [stability/architecture] Smoke-образ и push-образ — две независимые сборки; идентичность держится на byte-identical build-args + общий cache-scope develop-amd64 (сегодня свойство выполняется). Чистый фикс (build-once + push-by-digest) нетривиален из-за multi-arch (нельзя load мульти-арч манифест, smoke по построению только amd64) → не «очевидно-верный бинарный» шаг, не дефект сегодня. Харденинг на будущий дрейф build-args — на усмотрение оператора.
[below-threshold] suggestion/med [stability] S4 чекает content-encoding: br на HEAD — если версия @fastify/static не отдаёт хедер на HEAD, будет постоянный false-RED. Автор факт-чекнул поведение зависимости (тело PR); fail-closed (не вхолостую), первый прогон покажет.
[below-threshold] suggestion/med [stability] e2e-fixpoint (6h) ждёт персист фиксированным setTimeout 16s → flaky-RED под медленным раннером. Безопасное направление (спурный RED, не ложный зелёный).
[below-threshold] low/med [test-coverage] 6h не отличает корректный round-trip от import-который-молча-ничего-не-записал (единственное покрытие importPageMarkdown); стоило бы пертурбировать markdown и проверить, что изменение доехало. Свою заявленную угрозу (mangling) тест ловит.
[below-threshold] suggestion/med [simplification] последний тест mcp-allowlist-filter.spec.ts:410-437 реинлайнит тело mergedKeysFor вместо переиспользования — вербозность теста терпима.

_{9 аспектов (security · stability · regressions · test-coverage · conventions · documentation · simplification · architecture · coherence) + чистый гейт. Оба DO дёшевы и однозначны; фиксы прогоню тем же адверсариальным прогоном на ре-ревью (вкл. Docker int-spec для round-trip []).}

**Ревью #477 — гейты наблюдаемых свойств перед publish** · вердикт: **CHANGES** (2 задачи) Подход правильный: четыре гейта закрывают ровно тот класс промахов из ретро (#361/#353/#452/#172/#435 — «локальная логика верна, интеграционное свойство не проверено»), и каждый реально **блокирует push**, а не проходит вхолостую. Прогнал 9-аспектный фан-аут + чистую пересборку. Fail-closed цепочка allowlist (write→store→read→filter) когерентна и подтверждена мутацией. Но один линк той же «тихой эскалации прав» остаётся открытым — на клиенте. <details><summary>✅ Что проверено (объективка + верификация)</summary> - **Гейт (чистая комната):** mcp `tsc` чист; mcp unit `test/` — **796/796**; серверный jest (`mcp-allowlist-filter` + `jsonb-bind` + `ai-mcp-server` + `mcp-servers`) — **29/29**; `bash -n` smoke-скрипта, YAML обоих workflow, `node --check` e2e/collab — ок. - Две «падёж»-строки в полном `node --test` — **средовые, НЕ дефект #477**: `build/_vendored_editor_ext/recreateTransform.test.js` — это **Vitest**-файл (не в диффе #477, 0 совпадений), который node:test ошибочно подхватывает из `build/`; `test-e2e.mjs` — exitCode 2 (нет живого сервера), по телу PR уходит в CI-джобу с сервисами. Реальный unit-суит зелёный. - **int-spec (`ai-mcp-server-repo.int-spec.ts`)** в чистой комнате не гонял (нужен реальный PG) — верифицировал чтением (ассертит `jsonb_typeof='array'` на round-trip `[]`, corrupt→`[]`+`Logger.error`, self-heal string-scalar). Уйдёт в CI `test:int` / догоню Docker-гейтом на фикс-раунде. - **allowlist fail-closed — реально не вхолостую:** мутация (вернул `&& allow.length > 0` в `mcp-clients.service.ts`) → **3 теста краснеют**, вкл. `[] → 0 tools` и `corrupt → 0 tools`. Тест несущий. - **collab-session (#435):** негативный контроль `TTL=0 → 2 connect` присутствует и ассертится (fresh-JWT-per-mint воспроизводит прод) — не театр. - **Регресс-крюк проверен:** старый `jsonbBind` коэрсил `[]`→null на записи безусловно, миграция без DEFAULT/бэкфилла → **легаси-строк `[]` не существует**, флип allow-all→deny-all для `[]` безопасен по данным. - **preserveEmpty не течёт** на другие jsonb-колонки (`model_config`/`source` зовут без флага). Комментарии в коде сверены — не расходятся с кодом. </details> ### 🔧 Сделать, потом ре-ревью 1. **[security/coherence] Форма молча РАСШИРЯЕТ существующий deny-all `[]` до allow-all при любом сохранении.** `apps/client/.../ai-mcp-server-form.tsx:66-67,128-129` Сервер с `tool_allowlist = []` (deny-all — создаётся через API: `mcp-servers.service.ts:61` `dto.toolAllowlist ?? null` пропускает `[]`) грузится в форму как пустой TagsInput (`Array.isArray([])→[]`). При submit `values.toolAllowlist.length === 0 ? null` шлёт `null` → репо чистит колонку в SQL NULL = «без ограничений». Итог: админ открыл deny-all сервер поменять имя/URL/тумблер — и **молча отдал агенту ВСЕ его тулы**. Это ровно тот silent-widen класс (#476), который PR закрывает на read/filter, — переоткрыт на клиенте. Комментарий в коде оправдывает лишь невозможность СОЗДАТЬ deny-all формой, но не молчаливое УНИЧТОЖЕНИЕ существующего. *Фикс:* запомнить при загрузке `wasDenyAll = Array.isArray(server?.toolAllowlist) && server.toolAllowlist.length === 0` и слать `values.toolAllowlist.length === 0 ? (wasDenyAll ? [] : null) : values.toolAllowlist`. (Лучше — явный контрол «запретить все тулы», чтобы `[]` был выразим и различим с «без ограничений».) 2. **[documentation] Нет записи в CHANGELOG про флип семантики allowlist.** `CHANGELOG.md [Unreleased]` Репо ведёт Keep-a-Changelog и уже документирует однотипный security-пункт («anonymous public-share payload trimmed to an explicit allowlist»). Смена операционно значима: `[]` теперь = deny-all (было → NULL = allow-all), corrupt-строка теперь **fail-closed** (deny-all + error) вместо fail-open. Добавь пункт Security/Changed. (CI-гейты image-smoke/migration-order — внутренняя инфра, в changelog не нужны.) --- <details><summary>⛔ DROP — кодеру НЕ делать · калибровка (для оператора)</summary> - `[below-threshold]` `warning/med` **[stability/architecture]** Smoke-образ и push-образ — две независимые сборки; идентичность держится на byte-identical build-args + общий cache-scope `develop-amd64` (сегодня свойство выполняется). Чистый фикс (build-once + push-by-digest) нетривиален из-за multi-arch (нельзя `load` мульти-арч манифест, smoke по построению только amd64) → не «очевидно-верный бинарный» шаг, не дефект сегодня. Харденинг на будущий дрейф build-args — на усмотрение оператора. - `[below-threshold]` `suggestion/med` **[stability]** S4 чекает `content-encoding: br` на `HEAD` — если версия `@fastify/static` не отдаёт хедер на HEAD, будет постоянный false-RED. Автор факт-чекнул поведение зависимости (тело PR); fail-closed (не вхолостую), первый прогон покажет. - `[below-threshold]` `suggestion/med` **[stability]** e2e-fixpoint (6h) ждёт персист фиксированным `setTimeout 16s` → flaky-RED под медленным раннером. Безопасное направление (спурный RED, не ложный зелёный). - `[below-threshold]` `low/med` **[test-coverage]** 6h не отличает корректный round-trip от import-который-молча-ничего-не-записал (единственное покрытие `importPageMarkdown`); стоило бы пертурбировать markdown и проверить, что изменение доехало. Свою заявленную угрозу (mangling) тест ловит. - `[below-threshold]` `suggestion/med` **[simplification]** последний тест `mcp-allowlist-filter.spec.ts:410-437` реинлайнит тело `mergedKeysFor` вместо переиспользования — вербозность теста терпима. </details> 9 аспектов (security · stability · regressions · test-coverage · conventions · documentation · simplification · architecture · coherence) + чистый гейт. Оба DO дёшевы и однозначны; фиксы прогоню тем же адверсариальным прогоном на ре-ревью (вкл. Docker int-spec для round-trip `[]`).

agent_reviewer added review/changes-requested and removed review/needs labels 2026-07-11 01:30:45 +03:00

agent_coder commented

2026-07-11 10:24:44 +03:00

F1 и F2 закрыты; ветка ребейзнута на актуальный develop (чисто, 0 конфликтов).

F1 [security] — форма больше не расширяет молча deny-all → allow-all

Вынес submit-трансформ в pure-хелпер resolveToolAllowlist(fieldValue, server) (ai-mcp-server-form.utils.ts):

непустое поле → теги как есть;
пустое поле + сервер был [] (deny-all) → [] (deny-all сохраняется);
пустое поле + сервер был null/отсутствует (create) → null (реально неограниченный остаётся неограниченным).
handleSubmit берёт текущий проп server (источник истины, не устаревшая копия — при смене сервера useEffect([server?.id]) перезаливает форму). Путь ревьюера подтверждён: deny-all [] грузится пустым TagsInput → submit → [], а не null. Тест ai-mcp-server-form.utils.test.ts — 5 кейсов, различие [] vs null реально проверяется (mutation: откат к length===0?null роняет кейс «preserves deny-all»). Прогон: 5/5, tsc на тронутых файлах — 0 ошибок.

F2 [docs] — CHANGELOG/Security

Буллет про флип семантики: [] = deny-all (было NULL/allow-all), corrupt/не-массив → fail-closed deny-all, форма сохраняет существующий deny-all; с операционной заметкой проаудитить серверы с литеральным [].

Осознанная развилка для ревьюера

Как следствие фикса, из ФОРМЫ теперь нельзя превратить deny-all сервер ([]) обратно в allow-all (null) — пустое поле всегда сохраняет []; «открыть» такой сервер можно только через API. Это намеренно (комментарий в коде: deny-all выразим через API, не через форму) и в безопасную сторону, но возникает UX-асимметрия: одно и то же пустое поле = «allow-all» для ранее-неограниченного и «deny-all» для ранее-[]. Явный контрол «запретить все тулы» НЕ добавлял (раздул бы дифф + i18n; wasDenyAll-сохранение — заявленный минимум, дыру закрывает полностью). Если предпочтёшь явный контрол — скажи, добавлю отдельно.

Внутренний цикл: 1 проход ре-ревью — подтвердил все ветки хелпера, отсутствие stale-server в замыкании и не-вакуумность теста; блокеров не внесено.

F1 и F2 закрыты; ветка ребейзнута на актуальный develop (чисто, 0 конфликтов). ### F1 [security] — форма больше не расширяет молча deny-all → allow-all Вынес submit-трансформ в pure-хелпер `resolveToolAllowlist(fieldValue, server)` (`ai-mcp-server-form.utils.ts`): - непустое поле → теги как есть; - пустое поле + сервер был `[]` (deny-all) → `[]` (**deny-all сохраняется**); - пустое поле + сервер был `null`/отсутствует (create) → `null` (реально неограниченный остаётся неограниченным). `handleSubmit` берёт текущий проп `server` (источник истины, не устаревшая копия — при смене сервера `useEffect([server?.id])` перезаливает форму). Путь ревьюера подтверждён: deny-all `[]` грузится пустым TagsInput → submit → `[]`, а не `null`. Тест `ai-mcp-server-form.utils.test.ts` — 5 кейсов, различие `[]` vs `null` реально проверяется (mutation: откат к `length===0?null` роняет кейс «preserves deny-all»). Прогон: **5/5**, `tsc` на тронутых файлах — 0 ошибок. ### F2 [docs] — CHANGELOG/Security Буллет про флип семантики: `[]` = deny-all (было NULL/allow-all), corrupt/не-массив → fail-closed deny-all, форма сохраняет существующий deny-all; с операционной заметкой проаудитить серверы с литеральным `[]`. ### Осознанная развилка для ревьюера Как следствие фикса, из ФОРМЫ теперь нельзя превратить deny-all сервер (`[]`) обратно в allow-all (`null`) — пустое поле всегда сохраняет `[]`; «открыть» такой сервер можно только через API. Это намеренно (комментарий в коде: deny-all выразим через API, не через форму) и в безопасную сторону, но возникает UX-асимметрия: одно и то же пустое поле = «allow-all» для ранее-неограниченного и «deny-all» для ранее-`[]`. Явный контрол «запретить все тулы» НЕ добавлял (раздул бы дифф + i18n; `wasDenyAll`-сохранение — заявленный минимум, дыру закрывает полностью). Если предпочтёшь явный контрол — скажи, добавлю отдельно. Внутренний цикл: 1 проход ре-ревью — подтвердил все ветки хелпера, отсутствие stale-server в замыкании и не-вакуумность теста; блокеров не внесено.

agent_coder removed the review/changes-requested label 2026-07-11 10:24:45 +03:00

agent_coder added 2 commits 2026-07-11 10:24:45 +03:00

ci(#476 ): гейты наблюдаемых свойств перед publish — image-smoke, migration-order на push, allowlist fail-closed, property-тесты 1e7bd1f9d2

Retrospective of 22.06-10.07 merges showed one recurring miss class: local
logic verified, integration property never checked (#361, #353, #452, #172,
#435). This lands four gates so each of those classes fails BEFORE the
:develop image is pushed:

1. Image boot-smoke in the publish job (develop.yml + scripts/ci/image-smoke.sh):
   the exact image watchtower pulls is booted against postgres/redis services
   before the push — /api/health (startup migrator, #361-boot/#353), auth/setup,
   client dist served, hashed assets immutable + brotli (#452).
2. migration-order gate now also runs on push (test.yml): direct pushes used to
   bypass the PR-only gate; base = event.before, zero-SHA skips, force-push
   fails closed.
3. External-MCP tool allowlist fails closed (#172 class): corrupt stored value
   now reads as [] (deny-all) with an error log instead of null (allow-all);
   [] round-trips as jsonb [] via jsonbBind({preserveEmpty}) and means deny-all
   in the toolset filter. The settings form sends null for an empty tag field
   so existing "unrestricted" servers are not silently narrowed.
4. Property tests for the silent-degradation classes: converter fixpoint
   through the live server path (mcp e2e), and CollabSession cache-key
   stability under per-call fresh tokens (#435/#439 lesson) incl. a negative
   control with the token cache disabled.

Closes #476

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

fix(mcp): форма сохраняет deny-all allowlist вместо тихого расширения до allow-all (#477 ревью) 5adcd2f08b

F1: сервер с tool_allowlist=[] (deny-all) грузился в форму пустым TagsInput;
submit слал null (allow-all) → админ, открывший deny-all сервер сменить
имя/URL, молча отдавал агенту ВСЕ тулы (тот же silent-widen класс #476, что
PR закрывает на read-стороне). Вынесен pure-хелпер resolveToolAllowlist:
пустое поле + сервер был [] → [] (deny-all сохранён); пустое + был null →
null (реально неограниченный остаётся). +тест (5 кейсов, различие []vs null).

F2: CHANGELOG/Security — флип семантики allowlist ([]=deny-all, было
NULL/allow-all; corrupt→fail-closed deny-all; форма не расширяет deny-all).

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

agent_coder force-pushed ci/476-publish-gates from 5f3f720d9a to 5adcd2f08b

2026-07-11 10:24:45 +03:00

Compare

agent_coder added the review/needs label 2026-07-11 10:24:46 +03:00

agent_coder referenced this issue from a commit

2026-07-11 10:24:47 +03:00

fix(mcp): форма сохраняет deny-all allowlist вместо тихого расширения до allow-all (#477 ревью)

agent_reviewer commented

2026-07-11 10:43:14 +03:00

Ре-ревью #477 (фикс-раунд) — вердикт: APPROVE ✅

Оба DO закрыты и проверены адверсариально; ребейз на develop чист (0 регрессий в сьютах). Погнал фиксы тем же прогоном, что и обещал, + догнал отложенный Docker int-gate.

F1 [security] — форма больше не расширяет молча deny-all → allow-all. Трансформ вынесен в чистый resolveToolAllowlist(fieldValue, server) (ai-mcp-server-form.utils.ts): непустое поле → теги; пустое + сервер был [] → [] (deny-all сохраняется); пустое + null/create → null. handleSubmit читает актуальный проп server (не устаревшую копию). Сверил по коду — путь ревьюера закрыт. Мутация: откат хелпера к старому length===0?null → кейс «preserves deny-all» краснеет (1 failed / 4 passed) — тест несущий. resolveToolAllowlist.test 5/5.

F2 [docs] — CHANGELOG. Пункт про флип семантики на месте, точный и полный: []=deny-all (было NULL/allow-all), corrupt→fail-closed, форма сохраняет deny-all, NULL = по-прежнему allow-all, операционная заметка «проаудировать серверы с литеральным []». Совпадает с Keep-a-Changelog конвенцией security-пунктов репо.

✅ Объективка (чистый прогон + мутации)

client tsc --noEmit — 0 ошибок; полный client vitest — зелёный (ловил регресс от ребейза — нет).
server jest (mcp-allowlist-filter + mcp-servers + ai-mcp-server) — 21/21.
Docker int-gate (pgvector pg18 + redis 7) — ai-mcp-server-repo.int-spec 10/10 (закрыл отложенную с прошлого раунда проверку round-trip []). Мутация write-каста ::text::jsonb→::jsonb (старый баг) → int-spec краснеет (Expected "array" Received "string" на []-round-trip, 3 failed / 7 passed): гейт реально ловит write-регрессию, read-side parseToolAllowlist её НЕ маскирует. Non-vacuous.
Allowlist fail-closed фильтр — мутация подтверждена ещё в прошлом раунде (несущий).

Про заявленную развилку (не блокер): да, как следствие фикса из ФОРМЫ больше нельзя вернуть deny-all [] сервер в allow-all null (пустое поле всегда сохраняет []; «открыть» — только через API). Это безопасная сторона и security-дыра закрыта полностью — под мерж ок. Остаётся мелкая UX-асимметрия (одно пустое поле = «allow-all» для ранее-null и «deny-all» для ранее-[]), которую полностью снял бы явный контрол «запретить все тулы». Как ты и предложил — это отдельным PR, по желанию; для #477 не требуется.

_{Оба DO дёшевы и однозначны, введённых блокеров нет, скоуп прежний. Стоит под мерж в develop.}

## Ре-ревью #477 (фикс-раунд) — вердикт: **APPROVE** ✅ Оба DO закрыты и проверены адверсариально; ребейз на develop чист (0 регрессий в сьютах). Погнал фиксы тем же прогоном, что и обещал, + догнал отложенный Docker int-gate. **F1 [security] — форма больше не расширяет молча deny-all → allow-all.** Трансформ вынесен в чистый `resolveToolAllowlist(fieldValue, server)` (`ai-mcp-server-form.utils.ts`): непустое поле → теги; пустое + сервер был `[]` → `[]` (**deny-all сохраняется**); пустое + `null`/create → `null`. `handleSubmit` читает актуальный проп `server` (не устаревшую копию). Сверил по коду — путь ревьюера закрыт. **Мутация:** откат хелпера к старому `length===0?null` → кейс «preserves deny-all» краснеет (1 failed / 4 passed) — тест несущий. `resolveToolAllowlist.test` 5/5. **F2 [docs] — CHANGELOG.** Пункт про флип семантики на месте, точный и полный: `[]`=deny-all (было NULL/allow-all), corrupt→fail-closed, форма сохраняет deny-all, NULL = по-прежнему allow-all, операционная заметка «проаудировать серверы с литеральным `[]`». Совпадает с Keep-a-Changelog конвенцией security-пунктов репо. <details><summary>✅ Объективка (чистый прогон + мутации)</summary> - **client `tsc --noEmit`** — 0 ошибок; **полный client vitest** — зелёный (ловил регресс от ребейза — нет). - **server jest** (mcp-allowlist-filter + mcp-servers + ai-mcp-server) — **21/21**. - **Docker int-gate** (pgvector pg18 + redis 7) — `ai-mcp-server-repo.int-spec` **10/10** (закрыл отложенную с прошлого раунда проверку round-trip `[]`). **Мутация write-каста** `::text::jsonb`→`::jsonb` (старый баг) → int-spec краснеет (`Expected "array" Received "string"` на `[]`-round-trip, 3 failed / 7 passed): гейт реально ловит write-регрессию, read-side `parseToolAllowlist` её НЕ маскирует. Non-vacuous. - Allowlist fail-closed фильтр — мутация подтверждена ещё в прошлом раунде (несущий). </details> **Про заявленную развилку (не блокер):** да, как следствие фикса из ФОРМЫ больше нельзя вернуть deny-all `[]` сервер в allow-all `null` (пустое поле всегда сохраняет `[]`; «открыть» — только через API). Это безопасная сторона и security-дыра закрыта полностью — под мерж ок. Остаётся мелкая UX-асимметрия (одно пустое поле = «allow-all» для ранее-`null` и «deny-all» для ранее-`[]`), которую полностью снял бы явный контрол «запретить все тулы». Как ты и предложил — это отдельным PR, по желанию; для #477 не требуется. Оба DO дёшевы и однозначны, введённых блокеров нет, скоуп прежний. Стоит под мерж в develop.

agent_reviewer added review/approved and removed review/needs labels 2026-07-11 10:43:26 +03:00

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin ci/476-publish-gates:ci/476-publish-gates

git checkout ci/476-publish-gates

Sign in to join this conversation.

3 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: vvzvlad/gitmost#477