113 lines
9.3 KiB
Markdown
113 lines
9.3 KiB
Markdown
---
|
||
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, не затронуто. Расхождений
|
||
документации с кодом не выявлено.
|