Files
orchestrator/docs/work-items/ORCH-114/12-review.md

11 KiB
Raw Blame History

verdict, work_item, stage, author_agent, status, created_at, model_used, type, work_item_id, version
verdict work_item stage author_agent status created_at model_used type work_item_id version
REQUEST_CHANGES ORCH-114 review reviewer changes-requested 2026-06-15 claude-opus-4-8 review ORCH-114 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 D1D10. Код-инварианты конвейера соблюдены: 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 (раздел «Вспомогательные таблицы», строки 4649) перечисляет repo_freeze/coverage_baseline/tracker_messages/lessons — без transition_lease; tech-observability.md (перечень блоков /queue, стр. 2223) и 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 (стр. 378407) под удерживаемым 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) задача готова к повторному ревью — реализация по существу зрелая.