From 4a6b32e61de75da5c03261b0e9943a1728cd3ee0 Mon Sep 17 00:00:00 2001 From: claude-bot Date: Mon, 15 Jun 2026 17:54:55 +0300 Subject: [PATCH] reviewer(ET): auto-commit from reviewer run_id=711 --- docs/work-items/ORCH-114/12-review.md | 127 ++++++++++++++++++++++++++ 1 file changed, 127 insertions(+) create mode 100644 docs/work-items/ORCH-114/12-review.md diff --git a/docs/work-items/ORCH-114/12-review.md b/docs/work-items/ORCH-114/12-review.md new file mode 100644 index 0000000..d376565 --- /dev/null +++ b/docs/work-items/ORCH-114/12-review.md @@ -0,0 +1,127 @@ +--- +verdict: REQUEST_CHANGES +work_item: ORCH-114 +stage: review +author_agent: reviewer +status: changes-requested +created_at: 2026-06-15 +model_used: claude-opus-4-8 +type: review +work_item_id: ORCH-114 +version: 1 +--- + +# Review ORCH-114 — Durable transition-ownership lease + expected-stage CAS + +## Summary + +Сильная, аккуратная реализация корневого класса инцидент-цепочки ORCH-110/111/112/113. +Архитектура (durable-lease на входе + expected-stage CAS на записи, liveness по `pid`+`boot_id` +без heartbeat, self-hosting-скоуп, kill-switch, never-raise) реализована **в точном соответствии +с ADR-001 D1–D10**. Код-инварианты конвейера соблюдены: `src/stages.py` и `src/qg/checks.py` +**не тронуты** (AC-11), схема БД аддитивна (одна таблица `transition_lease`, без epoch-колонки), +hot-path `claim_next_job` не консультирует lease (AC-10/fail-open). Прогон **полного** `pytest +tests/` — **2048 passed** в детерминированном порядке (плагина random-ordering нет), т.е. AC-9 +(байт-в-байт при kill-switch off) и CI-green выполняются операционно. Тесты содержательные: +TC-01a/b воспроизводят обязательный регресс класса ORCH-111 (kill-switch off = «красный», +on = «зелёный»), TC-02…TC-14 покрывают AC-2…AC-13. + +Однако вердикт — **REQUEST_CHANGES**: документация (golden source наравне с кодом) обновлена +**частично**. Архитектурные доки (`docs/architecture/README.md`, `internals.md`, `CHANGELOG.md`, +ADR-001 + сквозной adr-0045) — образцовы, но **`.env.example` и `CLAUDE.md` не обновлены** (P1), +а витрина `docs/overview/` (ORCH-011) не отражает новый механизм (P2). Плюс две незначительные +оси-уточнения (P2). + +## Findings + +### P0 — Blocker +- _(нет)_ + +### P1 — Must fix +- [ ] **`.env.example` не содержит новых ключей конфигурации** `ORCH_TRANSITION_LEASE_ENABLED` + и `ORCH_TRANSITION_LEASE_REPOS` (добавлены в `src/config.py:transition_lease_enabled`/ + `transition_lease_repos`). ORCH-101 нормативно объявил `.env.example` **«каноном 100% ключей + старта»**, и все сопоставимые kill-switch'и там присутствуют (`ORCH_SERIAL_GATE_ENABLED:220`, + `ORCH_CHECKOUT_HYGIENE_ENABLED:349`, `ORCH_COVERAGE_GATE_ENABLED:511`). Изменение конфигурации + без отражения в каноническом env-файле — нарушение оси «документация = golden source» (доступ + оператора к kill-switch'у). **Fix:** добавить обе строки в `.env.example` (с дефолтами + `true`/пусто) рядом с блоком прочих gate-флагов. + _Ссылка: правило документирования CLAUDE.md §2 («конфигурация → env-канон»), ORCH-101 (adr-0036)._ + +- [ ] **`CLAUDE.md` не дополнен паспорт-секцией ORCH-114.** ORCH-114 — крупный кросс-каттинговый + механизм (новая durable-таблица, новый leaf `src/transition_lease.py`, врезки в `advance_stage`/ + `job_reaper`/`reconciler`/обоих webhook'ах/`main.lifespan`, новый эндпоинт, новый инвариант + владения переходом). Конвенция паспорта **универсальна**: каждая сопоставимая фича несёт секцию + в `CLAUDE.md` (ORCH-112/110/098/094/093/090/089/088/027/019…), а футер прямо гласит «Поддерживается + агентами при каждой доработке». Для self-hosting это критично: будущий агент, правящий + `advance_stage`/reaper/webhooks, обязан найти инвариант ORCH-114 в первом обязательном к чтению + доке (ORCH-078 трассировка опирается на паспорт как индекс). **Fix:** добавить секцию ORCH-114 + в `CLAUDE.md` по образцу ORCH-112/113 (механизм, инвариант, флаги, ADR-ссылки). + _Ссылка: CLAUDE.md правила §1, §2 + футер; ORCH-078 (TRACEABILITY)._ + +### P2 — Should fix +- [ ] **Витрина системы `docs/overview/` не обновлена (ORCH-011).** Добавлена durable-таблица + `transition_lease` и read-only блок `transition_lease` в `GET /queue`, но: + `tech-data-model.md` (раздел «Вспомогательные таблицы», строки 46–49) перечисляет + `repo_freeze`/`coverage_baseline`/`tracker_messages`/`lessons` — без `transition_lease`; + `tech-observability.md` (перечень блоков `/queue`, стр. 22–23) и `tech-architecture.md` + (список компонентов с job-reaper, стр. 46) тоже не упоминают механизм. Индекс витрины прямо + требует «изменил функциональность платформы → обнови витрину `docs/overview/` в том же PR» + (`docs/overview/README.md:66`). Понижено до P2 (не стадия/гейт/агент/интеграция → машинные + проверки `tests/test_system_docs.py` это не ловят), но completeness-норму витрины нарушает. + **Fix:** добавить строку `transition_lease` в таблицу `tech-data-model.md`, упоминание блока + в `tech-observability.md`, компонент в `tech-architecture.md`. + +- [ ] **Расхождение код ↔ ADR D4 по CAS на rollback-записях.** ADR-001 D4 (таблица) предписывает + rollback-записям `_handle_*_rollback` **CAS** (столбец «да») как защиту rollback↔done (BR-6). + В коде rollback-записи на side-effectful рёбрах остались голым `update_task_stage(task_id, + "development")` — `_handle_merge_gate_rollback:1246`, `_handle_security_gate:1323`, + `_handle_coverage_gate:1411`, `_handle_image_freshness:1491`. **Корректность не нарушена:** эти + хендлеры вызываются строго внутри `advance_stage` (стр. 378–407) под удерживаемым lease → + единственный владелец → конкурентного противоречия нет. Но налицо буквальное расхождение + реализации с собственным ADR. **Fix (на выбор):** либо провести эти записи через + `transition_lease.commit_stage_cas(...)` (как в forward/bypass-путях), либо уточнить ADR D4, что + под lease CAS на in-region rollback избыточен (чтобы доки соответствовали коду). + _Ссылка: ADR-001 D4/D5, BR-6, AC-6._ + +- [ ] **Изоляция тестов: модуль `tests/test_orch114_transition_ownership.py` протекает в + процесс-wide `settings.db_path`.** Module-level `os.environ.setdefault("ORCH_DB_PATH", …)` + (стр. 22) при первом импорте инстанцирует `Settings` с `db_path=test_orch114.db`, а позднейший + `os.environ["ORCH_DB_PATH"]=…` в `tests/test_webhooks.py:9` уже не влияет (settings создан) → + при прогоне двух модулей вместе (`pytest tests/test_orch114_transition_ownership.py + tests/test_webhooks.py`) **4 существующих webhook-теста падают** (`assert 'architecture' == + 'analysis'` и др.). **Полный `pytest tests/` зелёный** (промежуточные модули перетирают + состояние; плагина random-ordering нет) → CI/coverage-gate **не** ломаются, поэтому P2, не + блокер. Но developer/reviewer, гоняющие подмножество, получат ложные падения. **Fix:** + изолировать db_path модуля через conftest-fixture/собственный tmp без правки процесс-wide env + (паттерн `fresh_db` уже есть — достаточно убрать module-level `setdefault`). + +### P3 — Nice to have +- [ ] `reconciler`/`plane._try_advance_stage` зовут `is_held_by_live_owner(task_id)` без + предварительного `applies(repo)` (в отличие от reaper). Для out-of-scope, но включённого репо + (enduro при дефолте) это лишний безвредный `SELECT` (строк нет → `False`, defer не возникает). + Функционально корректно; при желании — добавить дешёвый `applies`-гард ради нулевого оверхеда. + +## Документация + +**Обновлено (образцово):** +- `docs/architecture/README.md` — новый компонент в списке, отдельная секция «Единое владение + side-effectful переходами», строка таблицы БД `transition_lease`, API-таблица + (`POST /transition-lease/release`), описание блока `/queue`. +- `docs/architecture/internals.md` — DDL `transition_lease`, lifespan-recovery, секция reaper. +- `CHANGELOG.md` — подробная запись ORCH-114 в `[Unreleased]`. +- ADR: `docs/work-items/ORCH-114/06-adr/ADR-001-…` + сквозной + `docs/architecture/adr/adr-0045-…` — полные, со сверкой по коду. +- Work-item доки `02-trz`/`03-acceptance-criteria`/`08-data-requirements`/`10-tech-risks` — на месте. + +**Нужно обновить (см. findings):** +- `.env.example` — добавить `ORCH_TRANSITION_LEASE_ENABLED` / `ORCH_TRANSITION_LEASE_REPOS` **(P1)**. +- `CLAUDE.md` — добавить паспорт-секцию ORCH-114 **(P1)**. +- `docs/overview/` (`tech-data-model.md` / `tech-observability.md` / `tech-architecture.md`) — + отразить таблицу/блок/компонент `transition_lease` **(P2)**. + +## Verdict rationale +Любой P1 → `REQUEST_CHANGES`. Кода-корректность и инварианты конвейера — без P0/P1; блокирует +**частичная незавершённость документации golden-source** (`.env.example`, `CLAUDE.md`). После +закрытия двух P1 (и желательно P2) задача готова к повторному ревью — реализация по существу +зрелая.