From 0bc9662288cf555b7dbf80ba3ebf953fcf521b05 Mon Sep 17 00:00:00 2001 From: claude-bot Date: Mon, 15 Jun 2026 21:24:17 +0300 Subject: [PATCH] reviewer(ET): auto-commit from reviewer run_id=719 --- docs/work-items/ORCH-117/12-review.md | 107 ++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) create mode 100644 docs/work-items/ORCH-117/12-review.md diff --git a/docs/work-items/ORCH-117/12-review.md b/docs/work-items/ORCH-117/12-review.md new file mode 100644 index 0000000..64b2b24 --- /dev/null +++ b/docs/work-items/ORCH-117/12-review.md @@ -0,0 +1,107 @@ +--- +verdict: APPROVED +work_item: ORCH-117 +stage: review +author_agent: reviewer +status: approved +created_at: 2026-06-15 +model_used: claude-opus-4-8 +type: review +work_item_id: ORCH-117 +version: 1 +--- + +# Review ORCH-117 — sandbox-only fail-closed изоляция записи в Plane + +## Summary +Багфикс-трек (bug → escalate full-cycle) закрывает корневой класс инцидента **ORCH-114**: тест/ +worktree-процесс выполнял РЕАЛЬНУЮ запись (`PATCH …/issues/… state=` + комментарий) против +**боевого** Plane-проекта, наследуя живой боевой токен. Реализован чистый never-raise leaf +`src/plane_write_guard.py` (`decide()` + `audit_*` + `snapshot`), врезанный через тонкий хелпер +`_guard_allows_write` в **3 (все) примитива записи** `plane_sync` (`update_issue_state`/`add_comment`/ +`_set_issue_state_direct`) на момент вызова — сразу после `_resolve_project_id` и до любого сетевого +шага. Второй sandbox-bound слой — autouse-floor `tests/conftest.py::_plane_sandbox_only`. + +Реализация **точно** соответствует ADR-001 (D1–D7) и сквозному adr-0046. Все четыре оси проверки +пройдены, P0/P1-findings нет. + +**Проведённая верификация (фактический прогон):** +- `pytest tests/test_orch117_plane_write_isolation.py` → **16 passed** (TC-01…TC-14). +- **Обязательный регресс ORCH-019 BR-4 подтверждён вручную:** откатил врезку `plane_sync.py` на + версию `origin/main` → **TC-01 КРАСНЫЙ** (`httpx.patch` уходит на боевой проект + `7a79f0a9-…` с `LIVE-PROD-TOKEN` — воспроизводит инцидент); с фиксом — **ЗЕЛЁНЫЙ**. Тест-фиксатор + дефекта содержателен. +- `pytest tests/ -q` → **2068 passed** (AC-11 — нулевая регрессия, включая `test_system_docs.py`). +- `grep httpx.(patch|post|put|delete)` по `plane_sync.py` → ровно 3 write-call-site, **все** + под гардом; необёрнутых примитивов записи нет. +- `git diff --name-only src/` → изменены **только** `config.py`/`plane_sync.py`/`plane_write_guard.py`; + `stages.py`/`qg/`/`db.py`/`stage_engine.py` — **не тронуты**. + +## Findings + +### P0 — Blocker +- Нет. + +### P1 — Must fix +- Нет. + +### P2 — Should fix +- Нет. + +### P3 — Nice-to-have (не блокирует) +- [ ] Четыре существующих теста (`test_plane_author.py`, `test_plane_status_model.py`, + `test_plane_sync_labels.py`, `test_stage_visibility.py`) обходят гард монкипатчем + `decide → (True, "test-bypass")`. Это легитимно (per-test autouse, авто-revert; `httpx` в этих + тестах замокан → реальной записи быть не может; сам гард покрыт отдельным сьютом; паттерн — + аналог `_disable_merge_verify`). Чуть более хирургичной альтернативой был бы opt-in + добавление + целевого проекта в allowlist, но тесты проверяют маршрутизацию именно в НЕ-sandbox проекты, так + что полный bypass оправдан и явно задокументирован в каждом docstring. Менять не требуется. + +## Ось 1 — Соответствие ТЗ (02-trz / 03-acceptance-criteria) +- **FR-1** (fail-closed блок прод-записи из теста) — ✅ TC-01/02/03/04/09. +- **FR-2** (разрешено только sandbox + opt-in) — ✅ TC-06/07; `decide` шаги 3–5 1:1 с таблицей ADR. +- **FR-3** (дефолтная поза fail-closed через conftest-floor) — ✅ TC-05/13. +- **FR-4** (детект тест-процесса; no-op в боевом/staging) — ✅ TC-10/11; `_in_test_process` = + `"pytest" in sys.modules` / `PYTEST_CURRENT_TEST`, call-time. +- **FR-5** (аудит блок ERROR / allow INFO с полями) — ✅ TC-12. +- **FR-6** (never-raise в боевом пути; громко в тесте) — ✅ live-path возвращает ALLOW ДО try-блока; + in-test исключение → fail-CLOSED `guard-error`. +- **FR-7** (kill-switch без чёрного хода) — ✅ TC-14 (ассерт отсутствия `plane_write_guard_enabled`). +- **AC-1…AC-11** — все выполнены; AC-1 (обязательный регресс) и AC-11 (полный регресс) подтверждены + фактическим прогоном (см. Summary). + +## Ось 2 — Соответствие ADR (06-adr/ADR-001 + adr-0046) +- D1 чокпоинт (после `_resolve_project_id`, до сети, 3 примитива) — ✅ сверено по diff. +- D2 детект тест-процесса — ✅. D3 `decide` default-deny/sandbox-allowlist/opt-in — ✅ 1:1 с таблицей. +- D4 **умышленно без kill-switch прод-блока** — ✅ соблюдено (важный анти-дрейф: добавление общего + выключателя реинтродуцировало бы дефект ORCH-114; reviewer ловил бы как ≥P1 — здесь его нет). +- D5 conftest-floor — ✅. D6 конфиг-ключи (`plane_test_write_enabled=False`, + `plane_test_sandbox_projects=8c5a3025-…`) — ✅. D7 аудит — ✅. +- **Инварианты не сломаны:** `STAGE_TRANSITIONS`/реестр `QG_CHECKS`/семантика и имена `check_*`/ + machine-verdict-ключи/схема БД — байт-в-байт (диф трогает только 3 src-файла; гейтовые модули + не изменены). Трассировка (ORCH-078): врезка в 3 примитива не ломает чужих маркированных + инвариантов. + +## Ось 3 — Качество кода +- never-raise дисциплина корректна: внешний (боевой) путь физически не может упасть из-за гарда + (return до try); внутренний тест-путь fail-CLOSED. +- Полные docstrings на всех публичных функциях; стабильные reason-слаги вынесены константами и + проверяются тестами. +- **Багфикс-регресс (ORCH-019 BR-4):** TC-01 — фиксатор дефекта, красный до фикса / зелёный после + (проверено вручную). Требование выполнено. +- Покрытие всех 3 точек записи; необёрнутых write-примитивов не осталось. + +## Документация +Обновлена полностью (golden source наравне с кодом), AC-10 выполнен: +- `CLAUDE.md` — новый раздел «Sandbox-only fail-closed изоляция записи в Plane (ORCH-117)». ✅ +- `docs/architecture/README.md` — новый компонент «Plane write guard». ✅ +- `docs/operations/INFRA.md` — таблица env (`ORCH_PLANE_TEST_WRITE_ENABLED`/ + `ORCH_PLANE_TEST_SANDBOX_PROJECTS`) + раздел «что изолировано». ✅ +- `.env.example` — оба ключа с безопасными дефолтами. ✅ +- `CHANGELOG.md` — запись `[Unreleased]`. ✅ +- ADR: `06-adr/ADR-001-sandbox-only-plane-write-guard.md` + сквозной + `docs/architecture/adr/adr-0046-sandbox-only-plane-write-guard.md`. ✅ +- **Обзорные доки (ORCH-079) / витрина `docs/overview/` (ORCH-011):** обновлять не требуется — + ORCH-117 это защитный тест-изоляционный гард, а не ранее задокументированное «Известное + ограничение» README и не изменение функциональности конвейера/стадий/гейтов/агентов/интеграций, + показываемой в витрине; `test_system_docs.py` зелёный в полном прогоне.