diff --git a/docs/work-items/ORCH-110/12-review.md b/docs/work-items/ORCH-110/12-review.md new file mode 100644 index 0000000..a56c501 --- /dev/null +++ b/docs/work-items/ORCH-110/12-review.md @@ -0,0 +1,112 @@ +--- +verdict: APPROVED # APPROVED | REQUEST_CHANGES — строго одно из двух, UPPERCASE +work_item: ORCH-110 +stage: review +author_agent: reviewer +status: approved +created_at: 2026-06-15 +model_used: claude-opus-4-8 +type: review +work_item_id: ORCH-110 +version: 1 +--- + +# Review ORCH-110 + +> Машинный вердикт читается ТОЛЬКО из `verdict:` во frontmatter. `APPROVED` → дальше по конвейеру. + +## Summary +Багфикс инцидента ORCH-109/PR#129 (bug → escalate full-cycle): локальный re-test merge-gate падал по +**таймауту** при зелёных CI+tester+staging → ложно маршрутизировался как код-фейл (откат +`deploy-staging → development` + расход developer-retry) → manual-gate. Реализация **полностью +соответствует ТЗ (FR-1…FR-6, AC-1…AC-10) и ADR-001 (D1…D6)**, аддитивна, под 5 независимыми +kill-switch, never-raise, скоуп self-hosting. Инварианты конвейера и self-hosting — байт-в-байт целы; +документация (golden source) обновлена в том же PR; багфикс несёт регресс-тест-фиксатор (ORCH-019 BR-4 / +AC-10). **Вердикт: APPROVED** — P0/P1/P2 findings нет. + +> Замечание по ревью-процессу (не finding): локальный ref `main` в worktree устарел (6a04d0a); +> истинная цель мержа — `origin/main` (b6c9d27, уже содержит ORCH-111). Диф против `origin/main` +> добавляет ровно 4 ORCH-110-коммита (fix + analysis + architect + business-request); посторонних +> изменений нет. Ревью проведено против `origin/main`. + +## Оси проверки + +### 1. Соответствие ТЗ (02-trz / 03-acceptance-criteria) — PASS +- **FR-1/AC-1/AC-2** (толерантность к инфра-таймауту, отдельная классификация): `merge_gate.classify_retest_failure` + (`timeout`/`red`/`lock-busy`/`other`) + `stage_engine._handle_merge_gate_infra_retry` (зеркало + `_handle_merge_gate_defer`: задача остаётся на `deploy-staging`, БЕЗ отката/developer-retry). ✓ +- **FR-2/AC-4** (tree-kill): новый stdlib-only leaf `src/proc_group.py::run_in_process_group` + (`start_new_session` → `os.killpg` SIGTERM→grace→SIGKILL), интегрирован в `retest_branch` и + `measure_coverage`; реальный grandchild-тест TC-01 доказывает «сирота не переживает таймаут». ✓ +- **FR-3/AC-5** (бюджет): `merge_retest_timeout_s` 600→900 + `_resolve_retest_timeout` (малформ/непозитив + → дефолт 900 + WARNING); сквозной инвариант `reaper_max_running_s (5400) > Σ≈4460 + grace` проверен + тестом TC-08 **без** правки `reaper_max_running_s`. ✓ +- **FR-4/AC-6/AC-3 (BR-6)** (контракт необходимости re-test): пропуск re-test при доказанном no-op rebase + (`head_sha` до==после, обе непусты) в `check_branch_mergeable`; fail-safe — на любой неопределённости + re-test выполняется; красный re-test/конфликт по-прежнему откатывают (тест + `test_tc10_real_catchup_red_retest_still_rolls_back`). ✓ +- **FR-5/AC-7/AC-8** (kill-switch + нулевая регрессия + инварианты): 5 независимых флагов (дефолт = боевое); + `STAGE_TRANSITIONS`/реестр `QG_CHECKS`/семантика и имя `check_branch_mergeable`/machine-verdict/схема БД — + статически подтверждено «не тронуты»; INV-4 (никогда push/force-push `main`) соблюдён. ✓ +- **FR-6/AC-9** (наблюдаемость + anti-loop): счётчики `_MERGE_GATE_COUNTERS` + read-only блок `merge_gate` + в `GET /queue`; ограниченность по попыткам (`merge_retest_infra_max_retries=2`, restart-safe счётчик по + маркеру в `task_content`) и времени; исчерпание → один инфра-alert (явно «НЕ дефект кода»). ✓ +- **AC-10** (регресс red-before/green-after): `tests/test_orch110_false_rollback_regression.py` гоняет + РЕАЛЬНЫЙ `check_branch_mergeable` через `advance_stage`; на до-ORCH-110-коде оба сценария упали бы + (Scenario A гоняла бы обречённый re-test, Scenario B откатилась бы на `development`). ✓ + +### 2. Соответствие ADR + трассировка маркеров (ORCH-078) — PASS +- D1…D6 реализованы как зафиксировано в `06-adr/ADR-001` + сквозном `adr-0042`. ✓ +- Директива developer'у выполнена: `("ORCH-110", "classify_retest_failure", "src/merge_gate.py")` + дописан в `MAIN_REGRESSION_MARKERS` (append-only, существующие маркеры целы). ✓ +- Затронутые маркеры ORCH-043/071/073/093/027/065/109 сверены, зафиксированные инварианты **не сломаны**: + лиз held-on-success / released-on-failure (контракт `check_branch_mergeable` сохранён, skip-путь + возвращает `(True, …)` с HELD lease), anti-phantom pre-merge rebase не тронут, coverage fail-open `None` + сохранён, reaper-инвариант проверен численно. + +### 3. Качество кода — PASS +- never-raise по всему пути (leaf `proc_group`, классификатор, врезка `_handle_merge_gate_infra_retry` + обёрнута в `_handle_merge_gate_infra_retry`/`_merge_gate_infra_retry_impl` с проглатыванием исключения). +- Docstrings на всех публичных функциях; чёткое разделение чистого предиката и I/O. +- Тесты содержательные (реальный kill дерева процессов, реальная маршрутизация через `advance_stage`, + валидация бюджета, kill-switch-параметризация, наблюдаемость). Прогон зелёный: + 141 (ORCH-110 + изменённые merge/coverage/config) + 575 (широкая зона stage_engine/qg/merge/deploy/reaper) + + 37 (docs-инварианты, ORCH-011 + host-hardcodes) — все PASS. +- **Багфикс-трек (ORCH-019 BR-4):** исправление несёт тест-фиксатор дефекта (AC-10) — требование выполнено. + +### 4. Документация — PASS (приоритетная проверка) +`src/` изменён → документация обновлена в **том же PR**: +- `CLAUDE.md` — добавлена секция ORCH-110 (паспорт). ✓ +- `CHANGELOG.md` — запись `[Unreleased]` с разбивкой D1…D6 + 5 ключей. ✓ +- `.env.example` — 5 новых ключей + бамп `ORCH_MERGE_RETEST_TIMEOUT_S=900` с врезкой про сквозной + инвариант. ✓ +- `docs/architecture/README.md` — поведение merge-gate re-test (D1…D6) + ссылки на ADR. ✓ +- `docs/overview/tech-pipeline.md` — **витрина системы (ORCH-011)** обновлена: исключение «инфра-таймаут = + транзиент, не откат»; `tests/test_system_docs.py` зелёный. ✓ +- Глобальный сквозной ADR `docs/architecture/adr/adr-0042-…md` + work-item `06-adr/ADR-001-…md` заведены. ✓ + +## Findings + +### P0 — Blocker +- (нет) + +### P1 — Must fix +- (нет) + +### P2 — Should fix +- (нет) + +### P3 — Nice-to-have (не блокирует) +- В `check_branch_mergeable` (`src/qg/checks.py:750`) причина FAIL раскладывается локальной подстрокой + `"timeout" in t_reason`, тогда как введён точный предикат `merge_gate.classify_retest_failure`. Поведение + безопасно (авторитетная маршрутизация в `stage_engine._handle_merge_gate` использует классификатор; редкий + `"re-test error: …"` со словом «timeout» в детали безопасно уйдёт в `other`→rollback), но единая точка + классификации читалась бы чище. Косметика, на усмотрение. + +## Документация +**Обновлена полностью в том же PR** (golden source наравне с кодом): `CLAUDE.md`, `CHANGELOG.md`, +`.env.example`, `docs/architecture/README.md`, витрина `docs/overview/tech-pipeline.md` (ORCH-011), +глобальный ADR `adr-0042`, work-item ADR `06-adr/ADR-001`. Машинно-проверяемые факты витрины и +запрет хост-хардкодов — зелёные (`tests/test_system_docs.py`, `tests/test_no_host_hardcodes.py`). +Пунктов `README.md` «Известные ограничения», требующих снятия этим PR, не затронуто. Расхождений +документации с кодом не выявлено.