108 lines
8.3 KiB
Markdown
108 lines
8.3 KiB
Markdown
---
|
||
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=<Done>` + комментарий) против
|
||
**боевого** 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` зелёный в полном прогоне.
|