From 820e534e7712acf310de4c551b29b265a849619a Mon Sep 17 00:00:00 2001 From: claude-bot Date: Tue, 16 Jun 2026 08:52:22 +0300 Subject: [PATCH] reviewer(ET): auto-commit from reviewer run_id=751 --- docs/work-items/ORCH-123/12-review.md | 153 ++++++++++++++++++++++++++ 1 file changed, 153 insertions(+) create mode 100644 docs/work-items/ORCH-123/12-review.md diff --git a/docs/work-items/ORCH-123/12-review.md b/docs/work-items/ORCH-123/12-review.md new file mode 100644 index 0000000..21beee4 --- /dev/null +++ b/docs/work-items/ORCH-123/12-review.md @@ -0,0 +1,153 @@ +--- +verdict: APPROVED +work_item: ORCH-123 +stage: review +author_agent: reviewer +status: approved +created_at: 2026-06-16 +model_used: claude-opus-4-8 +type: review +work_item_id: ORCH-123 +version: 1 +--- + +# Review ORCH-123 + +## Summary + +Фикс инцидента **ORCH-116**: детерминированный staging-runner (ORCH-115) исполнял `docker exec` +**изнутри** прод-контейнера `orchestrator`, где нет docker CLI (`Dockerfile:11` — только +`openssh-client git curl`) → `FileNotFoundError` → постоянный environment-дефект ложно +маршрутизировался как код-фейл-откат `deploy-staging → development` (с расходом developer-retry). +Задача — `Bug`, эскалирована `escalate: full-cycle` (полный пакет analysis+architecture+ADR). + +Изменение точное и минимальное: 9 файлов в реальном скоупе (`65c17d8..HEAD`) — +`src/staging_runner.py` (+343), `src/config.py` (новый флаг), `src/main.py` (preflight), +`.env.example`, `CHANGELOG.md`, `CLAUDE.md`, `docs/architecture/README.md`, +`docs/operations/INFRA.md`, + тесты `tests/test_orch123_staging_runner_exec.py` (503), +`tests/test_orch115_staging_runner.py` (anti-drift). + +Реализация байт-в-байт соответствует ADR-001 / adr-0049 (D1 host-side ssh-wrap, D3 трёхсторонняя +`classify_staging_outcome`, D4 инвариант «инфра ≠ код-фейл» = infra-HOLD, D5 preflight, D6 флаг, +D7 сохранность контракта, D8 наблюдаемость). Все четыре оси проверки пройдены; P0/P1 findings нет. + +**Проверено фактически:** +- Полный прогон `pytest tests/ -q` → **2131 passed** (AC-11). +- Целевой набор `test_orch123_staging_runner_exec.py` + `test_orch115_staging_runner.py` + + `test_llm_determinization_docs.py` → **53 passed**. +- Структурный анти-регресс `test_system_docs.py` + `test_no_host_hardcodes.py` + + `test_config.py` → **61 passed**. + +## Соответствие ТЗ (02-trz.md / 03-acceptance-criteria.md) + +Все функциональные требования и критерии приёмки реализованы и покрыты тестами: + +| AC | FR/BR | Статус | Где проверено | +|----|-------|--------|---------------| +| AC-1, AC-2 — сюита реально исполняется host-side | FR-1 | ✅ | `build_staging_command` ssh-wrap; TC-02, TC-05 | +| AC-3 — env/tool-error ≠ код-фейл-откат | FR-2/FR-3 | ✅ | `classify_staging_outcome` + `_handle_permanent_env`/`_handle_transient_infra`; TC-01, TC-03 | +| AC-4 — анти-over-tolerance (исполнившаяся упавшая сюита → откат) | FR-2/BR-3 | ✅ | check 3 «trust recognised exit-code first»; TC-04, TC-04-exit1-marker | +| AC-5 — prod-like preflight | FR-4 | ✅ | `preflight()` в `main.lifespan` (best-effort, never-blocks); TC-07 | +| AC-6 — обязательный регресс red→green | FR-1/FR-2 | ✅ | TC-01 (in-container shape + spawn-error → permanent-env, без advance/defer/retry) | +| AC-7 — гейт/контракт/стадии/схема без регресса | NFR-1 | ✅ | TC-10, TC-11 (`STAGE_TRANSITIONS`/`QG_CHECKS`/`check_staging_status`/схема БД целы) | +| AC-8 — self-hosting safety | NFR-2 | ✅ | TC-08 (нет `compose`/`--build`/`8500`/`force`/`push`/`.env`/`restart` в argv) | +| AC-9 — security-review socket/CLI | NFR-3 | ✅ | ADR D2: выбран host-side ssh, docker CLI/SDK НЕ добавлен, `docker.sock` не используется изнутри | +| AC-10 — kill-switch/область | NFR-4/NFR-6 | ✅ | TC-09 (`staging_runner_exec_host_side` default True; enduro no-op) | +| AC-11 — never-raise / очередь не клинится | NFR-5 | ✅ | TC-12; полный suite зелёный | +| AC-12 — документация границы исполнения | BR-5/FR-6 | ✅ | TC-13 (INFRA.md + README.md содержат host-side/ssh/ORCH-123) | + +Регресс R-2 (held `deploy-staging`-задача не откатывается reconciler'ом) — отдельный тест +`test_r2_held_deploy_staging_not_rolled_back`: при отсутствии `15-staging-log.md` (infra-HOLD) +`advance_if_gate_passed` отдаёт `None`, задача держится на `deploy-staging`, `advance_stage` не +зовётся. Закрывает риск реинтродукции дефекта (10-tech-risks R-2). + +## Соответствие ADR (06-adr/ADR-001 + сквозной adr-0049) + +- **D1** — `build_staging_command` оборачивает ту же `docker exec … staging_check.py … --mode stub` + в `ssh -o StrictHostKeyChecking=no ''` (зеркало `self_deploy.build_deploy_command`/ + `image_freshness.image_revision`); inner-команда `shlex.quote`-ится; fallback на in-container при + выключенном флаге ИЛИ пустом ssh-target. ✅ +- **D3** — `classify_staging_outcome` различает три класса с корректным порядком проверок: + env-маркер в stderr → `permanent-env` (дизамбигуация `exit=1` коллизии «No such container» vs + реальный фейл, R-3); 126/127 → `permanent-env`; распознанный int≠255 без маркера → `suite-ran` + (проверяется **до** channel-guards → реальный фейл не маскируется, BR-3); timeout/255 → + `transient-infra`; нет ssh-target/`rc=None` → `permanent-env`; неизвестно → fail-safe + `transient-infra`. Pure, never-raise. ✅ +- **D4** — инвариант «инфра ≠ код-фейл»: `permanent-env` → немедленный infra-HOLD (без `_advance`, + без FAILED-лога, без developer-retry); `transient-infra` → bounded DEFER, на исчерпании — тоже + infra-HOLD (суперсед терминала ORCH-115 D5 fail-closed-FAILED+rollback). ✅ +- **D5** — `preflight()` зондирует host-side канал коротким bounded ssh-probe, пишет + `_PREFLIGHT_STATE`, алертит; вызван best-effort из `main.lifespan` после lease-recovery, + никогда не блокирует старт. ✅ +- **D6** — `staging_runner_exec_host_side: bool = True` (env `ORCH_STAGING_RUNNER_EXEC_HOST_SIDE`), + дефолт = боевое; откат двумя флагами. ✅ +- **D7** — `STAGE_TRANSITIONS`/`QG_CHECKS`/`check_staging_status`/`_parse_staging_status`/ + machine-verdict `staging_status:`/схема БД — байт-в-байт (TC-10/TC-11). ✅ +- **D8** — `snapshot()` различает три не-успешных класса (`failed`=code-fail / `deferred`=transient / + `permanent_env`=infra-HOLD) + `exec_host_side` + `preflight`. ✅ + +**Трассировка маркеров (CLAUDE.md §9 / TRACEABILITY.md):** ORCH-123 правит блоки ORCH-115 +(staging-runner, прямой объект), ORCH-036 (self_deploy ssh-паттерн), ORCH-110 (proc_group/classify +/infra-tolerance), ORCH-058 (host-side docker). ADR явно сверяется с их инвариантами и не ломает +их; анти-дрейф ORCH-115 — TC-14 (`should_intercept`/`map_exit_code_to_status` shared-контракт, +kill-switch → LLM) зелёный. + +## Качество кода + +- Чистый leaf-стиль по образцу `merge_gate`/`self_deploy`/`image_freshness`; на импорте только + `config`/`shlex`/`subprocess`, тяжёлое — лениво. Все публичные функции с docstrings. +- `never-raise` соблюдён на всех путях (classify, preflight, snapshot, run_staging_gate, alert'ы). +- Старое имя `_handle_tool_error` чисто переименовано в `_handle_transient_infra` — висячих ссылок + нет (grep пуст). +- Тесты содержательные (поведенческие, не тривиальные): мокается subprocess/advance/notifications, + spawn-error воспроизводится несуществующим бинарём — без живого ssh/docker/сети. +- **Багфикс-трек (ORCH-019 BR-4):** обязательный регресс-тест-фиксатор дефекта присутствует — + TC-01 красный на коде до фикса (нет класса/счётчика `permanent_env`, путь advance'ит + DEFER), + зелёный после. Требование выполнено. + +## Документация + +`src/` изменён → документация обновлена **в том же PR** (golden source): +- `docs/operations/INFRA.md` — новый раздел «Граница исполнения docker-операций — host-side» + + правило 4 для агентов (FR-6 / AC-12). ✅ +- `docs/architecture/README.md` — секция Staging-runner дополнена host-side стратегией + + трёхсторонней классификацией + ссылкой на adr-0049. ✅ +- `CLAUDE.md` — раздел ORCH-123 (host-side exec + классификация environment-дефекта). ✅ +- `CHANGELOG.md` — запись `fix(staging)`. ✅ +- `.env.example` — новый ключ `ORCH_STAGING_RUNNER_EXEC_HOST_SIDE=true` + описание. ✅ +- ADR: `06-adr/ADR-001-…` + сквозной `docs/architecture/adr/adr-0049-…`. ✅ + +**Витрина системы `docs/overview/` (ORCH-011)** — обновление НЕ требуется: канал исполнения +(host-side vs in-container) — внутренняя деталь реализации; машинно-проверяемые факты витрины +(детерминированный раннер заменил LLM-`deployer`, kill-switch, скоуп) ORCH-123 не меняет; +`test_system_docs.py` зелёный. + +**Обзорные доки `README.md` «Известные ограничения» (ORCH-079)** — обновление НЕ требуется: +ORCH-123 закрывает внутренний регресс ORCH-116, а не задокументированное ограничение; релевантного +пункта в списке нет. + +## Findings + +### P0 — Blocker +- Нет. + +### P1 — Must fix +- Нет. + +### P2 — Should fix +- Нет. + +### P3 — Nice-to-have (не блокирует) +- `preflight()` использует прямой `subprocess.run(timeout=20)` вместо `proc_group` — для короткого + read-only ssh-зонда приемлемо (тот же паттерн, что у host-side probe'ов `image_freshness`); + при таймауте локальный ssh-клиент завершается subprocess'ом без tree-kill, что для 20-секундного + inspect незначимо. Можно унифицировать в будущем, не обязательно. +- `_channel_ssh_configured()` вызывается дважды в `run_staging_gate` (для classify и для лог-строки + `permanent-env`) — микро-избыточность, never-raise, влияния нет. + +## Вердикт + +**APPROVED.** Реализация полно и точно покрывает ТЗ/AC, соответствует ADR-001/adr-0049, не трогает +гейты/стадии/контракт/схему БД, безопасна для self-hosting, документация (включая витрину-проверку +и обзорные доки) в порядке, обязательный багфикс-регресс присутствует и зелёный, полный suite +(2131) зелёный. P0/P1 отсутствуют.