diff --git a/docs/work-items/ORCH-114/12-review.md b/docs/work-items/ORCH-114/12-review.md index d376565..5d040aa 100644 --- a/docs/work-items/ORCH-114/12-review.md +++ b/docs/work-items/ORCH-114/12-review.md @@ -1,36 +1,68 @@ --- -verdict: REQUEST_CHANGES +verdict: APPROVED # APPROVED | REQUEST_CHANGES — строго одно из двух, UPPERCASE work_item: ORCH-114 stage: review author_agent: reviewer -status: changes-requested +status: approved created_at: 2026-06-15 model_used: claude-opus-4-8 type: review work_item_id: ORCH-114 -version: 1 +version: 2 --- # 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. +Повторное ревью (цикл 2). Прошлый вердикт был `REQUEST_CHANGES` из-за **частичной +незавершённости документации golden-source** (два P1 + три P2/P3). Кода-корректность и +инварианты конвейера уже тогда были без P0/P1. В этом цикле developer **закрыл все ранее +поднятые findings**: -Однако вердикт — **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). +- **P1 `.env.example`** → закрыт: добавлены `ORCH_TRANSITION_LEASE_ENABLED=true` / + `ORCH_TRANSITION_LEASE_REPOS=` с подробной нормативной шапкой (рядом с блоком reaper-флагов). +- **P1 `CLAUDE.md`** → закрыт: добавлена полноценная паспорт-секция «Единое владение + side-effectful переходами: durable-lease + expected-stage CAS (ORCH-114)» (механизм, два + слоя, инвариант, флаги, ADR-ссылки) — по образцу ORCH-112/113. +- **P2 витрина `docs/overview/`** → закрыта: `tech-data-model.md` (строка таблицы + `transition_lease`), `tech-observability.md` (упоминание блока `/queue`), + `tech-architecture.md` (компонент `Transition-lease`). +- **P2 расхождение код↔ADR D4 (CAS на rollback-записях)** → закрыто: введён общий хелпер + `_rollback_stage_cas`, и все четыре in-region rollback-записи (`_handle_merge_gate_rollback`/ + `_handle_security_gate`/`_handle_coverage_gate`/`_handle_image_freshness`) теперь пишут + `development` через тот же expected-stage CAS. Реализация совпала с собственным ADR D4; + добавлены целевые тесты (TC-11 `test_tc11_inregion_rollback_writes_use_cas`, + `test_tc11_rollback_cas_lost_aborts_without_overwriting_done`). +- **P2 изоляция тестов** → закрыта: убран module-level `os.environ.setdefault("ORCH_DB_PATH")`, + `test_webhooks.py` пинит собственный реестр per-test, добавлен autouse-fixture + `_disable_transition_lease` (kill-switch off для всего suite по образцу `_disable_merge_verify`). + +Архитектура реализована **в точном соответствии с ADR-001 D1–D10**: durable-lease на входе +(`acquire`/`release` в `try/finally`) + expected-stage CAS на записи (включая 6 путей в обход +`advance_stage`), liveness по `pid`+`boot_id` без heartbeat, реклейм при рестарте +(`recover_on_startup` в `main.lifespan`), reaper/reconciler/webhook defer при живом владельце, +Tier-3 backstop добивает зависшего. `src/transition_lease.py` — чистый never-raise leaf +(импортирует только `db`+`config`, лениво `merge_gate.pid_alive`/`qg.checks`/`notifications`). + +**Инварианты конвейера (AC-11):** `src/stages.py` и `src/qg/checks.py` **не тронуты** (нет в +диффе), `STAGE_TRANSITIONS`/`QG_CHECKS` встречаются в src-диффе только в комментарии; +machine-verdict ключи не тронуты; БД аддитивна (одна таблица `transition_lease`, +`CREATE TABLE IF NOT EXISTS`, без epoch-колонки). hot-path `claim_next_job` lease не +консультирует (AC-10/fail-open). + +**Багфикс-трек (ORCH-019/BR-4):** задача `bug→escalate full-cycle`. Регресс-тест-фиксатор +**присутствует**: `test_tc01_concurrent_entry_no_double_effect` (зелёный с lease) + +`test_tc01_red_before_fix_demonstration` (красный при kill-switch off — второй актор +переисполняет все под-гейты, воспроизводя класс ORCH-111). Требование выполнено. + +**Прогоны (verified):** +- `pytest tests/test_orch114_transition_ownership.py tests/test_webhooks.py` — **50 passed** + (именно та комбинация двух модулей, что в прошлом цикле давала 4 падения — изоляция + подтверждённо починена). +- `pytest tests/` (полный) — **2052 passed** в детерминированном порядке → AC-9 (байт-в-байт + при kill-switch off) и CI-green выполняются операционно; ORCH-113-тесты в наборе зелёные + (контракт предшественника не сломан, ORCH-078). ## Findings @@ -38,90 +70,50 @@ ADR-001 + сквозной adr-0045) — образцовы, но **`.env.exampl - _(нет)_ ### 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`-гард ради нулевого оверхеда. +- [ ] **Merge-lease не освобождается на (практически недостижимой) ветке CAS-lost в + rollback'е coverage/image-freshness.** В `_handle_coverage_gate`/`_handle_image_freshness` + `release_merge_lease` стоит **после** `_rollback_stage_cas`, поэтому при проигранном CAS + (`return True`) штатное освобождение merge-lease пропускается. Под держимым transition-lease + эта ветка практически недостижима (единственный владелец → CAS почти всегда выигрывает; чтобы + проиграть, нужен аномальный bypass-писатель, сдвинувший стадию с `deploy-staging`). Даже в + этом крайнем случае утечка **ограничена** собственным TTL+reclaim merge-lease (ORCH-043/065). + Корректность недвоения не нарушена. При желании — продублировать holder-aware + `release_merge_lease` до CAS-проверки (или задокументировать намеренный аборт). Не блокер. + _Ссылка: ADR-001 D1/D4, ORCH-027/058 rollback._ +- [ ] **`reconciler`/`plane._try_advance_stage` зовут `is_held_by_live_owner(task_id)` без + предварительного `applies(repo)`** (в отличие от reaper). Для out-of-scope, но включённого + репо (enduro при дефолте) это безвредный лишний `SELECT` (строк нет → `False`). Функционально + корректно; ради нулевого оверхеда можно добавить дешёвый `applies`-гард. (Перенос P3 из цикла 1 + — остаётся косметикой.) ## Документация -**Обновлено (образцово):** -- `docs/architecture/README.md` — новый компонент в списке, отдельная секция «Единое владение - side-effectful переходами», строка таблицы БД `transition_lease`, API-таблица - (`POST /transition-lease/release`), описание блока `/queue`. -- `docs/architecture/internals.md` — DDL `transition_lease`, lifespan-recovery, секция reaper. +**Обновлено и проверено (полно):** +- `.env.example` — `ORCH_TRANSITION_LEASE_ENABLED` / `ORCH_TRANSITION_LEASE_REPOS` (канон ключей + старта, ORCH-101). +- `CLAUDE.md` — паспорт-секция ORCH-114. +- `docs/overview/` — `tech-data-model.md` / `tech-observability.md` / `tech-architecture.md` + (витрина системы, ORCH-011). - `CHANGELOG.md` — подробная запись ORCH-114 в `[Unreleased]`. +- `docs/architecture/README.md` + `internals.md` — компонент, секция, таблица БД, API-эндпоинт + `POST /transition-lease/release`, блок `/queue`. - 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` — на месте. +- Work-item доки `02-trz`/`03-acceptance-criteria`/`04-test-plan`/`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)**. +**Необновлённой документации при изменении `src/` не выявлено.** Пунктов README «Известные +ограничения», закрываемых этим PR, нет. ## Verdict rationale -Любой P1 → `REQUEST_CHANGES`. Кода-корректность и инварианты конвейера — без P0/P1; блокирует -**частичная незавершённость документации golden-source** (`.env.example`, `CLAUDE.md`). После -закрытия двух P1 (и желательно P2) задача готова к повторному ревью — реализация по существу -зрелая. +Все P0/P1/P2 прошлого цикла закрыты; новых P0/P1 не выявлено по всем четырём осям +(ТЗ/AC, ADR, качество кода + обязательный регресс-тест багфикс-трека, документация +golden-source). Остаются только два P3-наблюдения (косметика/недостижимая граница), не +блокирующие приёмку. Полный `pytest tests/` зелёный (2052 passed), инварианты конвейера и +схемы существующих таблиц — байт-в-байт. → **APPROVED**.