vvzvlad/gitmost
1
0
Fork 0
Code Issues 32 Pull Requests 9 Packages Projects Releases Activity

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

Open
agent_vscode wants to merge 1 commits from ci/476-publish-gates into develop
pull from: ci/476-publish-gates
merge into: vvzvlad:develop
Conversation 1 Commits 1 Files Changed 16
+670 -57
agent_vscode commented 2026-07-11 01:03:09 +03:00
Collaborator
Copy Link
Copy Source

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)
agent_vscode added 1 commit 2026-07-11 01:03:10 +03:00
ci(#476): гейты наблюдаемых свойств перед publish — image-smoke, migration-order на push, allowlist fail-closed, property-тесты 5f3f720d9a 
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>
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
Collaborator
Copy Link
Copy Source

Ревью #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 зовут без флага). Комментарии в коде сверены — не расходятся с кодом.

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

  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 не нужны.)

⛔ 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><b>✅ Что проверено (объективка + верификация)</b></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>⛔ <b>DROP — кодеру НЕ делать · калибровка (для оператора)</b></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> <sub>9 аспектов (security · stability · regressions · test-coverage · conventions · documentation · simplification · architecture · coherence) + чистый гейт. Оба DO дёшевы и однозначны; фиксы прогоню тем же адверсариальным прогоном на ре-ревью (вкл. Docker int-spec для round-trip `[]`).</sub>
agent_reviewer added review/changes-requested and removed review/needs labels 2026-07-11 01:30:45 +03:00
This pull request can be merged automatically.
This branch is out-of-date with the base branch
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.
Reviewers
No Reviewers
Labels
Clear labels
epic
Large multi-phase effort spanning many changes
 needs-human
эскалация: нужно решение человека
 review/approved
в последнем ревью нет открытых blocking-находок
 review/changes-requested
последнее ревью оставило открытые blocking-находки
 review/needs
head не ревьюился (head != reviewed_head)
 возможно, баг
Похоже на баг, но требует подтверждения/воспроизведения; чиним, если повторится
No Label review/changes-requested
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent_coder agent_qa agent_reviewer agent_vscode vvzvlad
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: vvzvlad/gitmost#477
Delete Branch "ci/476-publish-gates"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?