reviewer(ET): auto-commit from reviewer run_id=321

docs/work-items/ORCH-065/12-review.md

@@ -2,7 +2,7 @@
 type: review
 work_item_id: ORCH-065
 verdict: REQUEST_CHANGES
-version: 1
+version: 2
 ---
 
 # Review ORCH-065
@@ -11,20 +11,24 @@ version: 1
 
 Задача закрывает три связанных класса отказов «процесс/поток умер, а ресурс остался
 захваченным навсегда»: zombie jobs (A), залипший merge-lease (B), неидемпотентная
-финализация merge (C). Реализация качественная: новый daemon-поток `src/job_reaper.py`
+финализация merge (C). Реализация в целом качественная: новый daemon-поток
-по образцу `reconciler` (never-raise, kill-switch, снимок в `/queue`), трёхуровневая
+`src/job_reaper.py` по образцу `reconciler` (never-raise, kill-switch, снимок в
-liveness (Tier-1 мёртвый pid + streak, Tier-2 exit_code, Tier-3 backstop), атомарный
+`/queue`), трёхуровневая liveness, атомарный `reap_running_job(... WHERE status='running')`,
-reap-claim `reap_running_job(... WHERE status='running')`, gate-driven advance через
+проактивный реклейм lease (`pid_alive` + `reclaim_stale_lease`), идемпотентный guard
-неизменный `_try_advance_stage`, проактивный реклейм lease (`pid_alive` +
+`pr_already_merged`, колонка `jobs.pid` через идемпотентный `_ensure_column`. Инварианты
-`reclaim_stale_lease`), колонка `jobs.pid` через идемпотентный `_ensure_column`.
+сохранены: `STAGE_TRANSITIONS`/`QG_CHECKS`/`check_*`/БАГ-8/exit-коды хука не тронуты
-Инварианты соблюдены: `STAGE_TRANSITIONS`/`QG_CHECKS`/`check_*`/БАГ-8/exit-коды хука —
+(коммиты ORCH-065 не касаются `stages.py`/`qg/checks.py`/`stage_engine.py`); реклейм
-без изменений; реклейм lease — только удаление файла, без git-операций; прод-контейнер
+lease — только удаление файла, без git-операций. Документация (README, internals, ADR,
-не трогается. Документация (README, internals, ADR, глобальный adr-0011, CHANGELOG,
+глобальный adr-0011, CHANGELOG, .env.example) обновлена в этом же PR. `pytest tests/ -q`
-.env.example) обновлена в этом же PR. `pytest tests/ -q` — **742 passed**.
+— **743 passed**.
 
-Блокер один: дефект Проблемы C (FR-3.2 / AC-11) — детерминированный guard
+Прошлый блокер v1 (guard `pr_already_merged` не подключён к пути merge) **устранён**
-`pr_already_merged` реализован и юнит-протестирован, но **не подключён ни к одному
+коммитом `aa46e5d`: промпт `.openclaw/agents/deployer.md` теперь предписывает консультировать
-реальному пути merge**, при этом ADR и golden-source-документация утверждают обратное.
+`pr_already_merged` ПЕРЕД любым (повторным) merge — AC-11 wiring на месте.
+
+Новый блокер: гонка Tier-2 reaper'а с живым monitor-потоком при штатной финализации —
+порядок «side effects → atomic claim» нарушает собственный контракт ADR-001 Р-1 и может
+дать дубль-advance / дубль-enqueue следующей стадии (FR-1.2/FR-1.3/AC-3/AC-4).
 
 ## Findings
 
@@ -32,48 +36,70 @@ reap-claim `reap_running_job(... WHERE status='running')`, gate-driven advance
 - нет
 
 ### P1 — Must fix
-- [ ] **`pr_already_merged` — мёртвый код в проде; ADR/доки утверждают, что он
+- [ ] **Tier-2 реапит живой, штатно финализирующийся job; side-effects идут ДО
-  консультируется (FR-3.2 / AC-11, ADR-001 Р-3).** Функция `merge_gate.pr_already_merged`
+  атомарного claim (нарушение ADR-001 Р-1, риск дубль-advance/дубль-enqueue).**
-  определена (`src/merge_gate.py:447`) и покрыта unit-тестами (TC-16/TC-17), но grep по
+  В `JobReaper._reap_job` (`src/job_reaper.py:177`) ветка Tier-2 срабатывает на ЛЮБОЙ
-  `src/` показывает **ноль вызовов** из продакшен-кода: ни в `stage_engine.py`, ни в
+  `running`-job, у которого в `agent_runs` уже записан `exit_code` — **без grace и без
-  `webhooks/gitea.py`, ни в `qg/checks.py`, ни в промпте `.openclaw/agents/deployer.md`.
+  какого-либо признака смерти monitor'а**. Но именно это состояние («exit_code записан,
-  Фактический merge PR в `main` выполняет LLM-агент deployer, и его промпт guard не
+  job ещё running») — нормальное окно финализации живого monitor-потока: `_monitor_agent`
-  упоминает. Тем временем:
+  пишет `exit_code`, затем выполняет git commit/push (+PR), БАГ-8-проверку, **сетевые
-  - ADR-001 Р-3: «Путь слияния (deployer/merge) **консультируется** с этим guard ПЕРЕД
+  usage-комментарии в Plane** (секунды-десятки секунд), и лишь потом `_try_advance_stage`
-    повторным merge: PR уже слит → no-op (без второго merge и без ошибки)»;
+  → `_finalize_job`. pid агента (`jobs.pid`) к этому моменту уже мёртв (процесс завершён
-  - `docs/architecture/README.md:226`: «добавлен never-raise guard `pr_already_merged` …
+  до записи exit_code), поэтому отличить «monitor умер» от «monitor жив и финализирует»
-    уже слит = no-op»;
+  по pid невозможно, а reaper тикает каждые `reaper_interval_s` (60с). Доступный для
-  - CHANGELOG: «`pr_already_merged(...)` — guard **перед повторным merge** при re-drive».
+  grace `finished_at_run` запрашивается в `get_running_jobs`, но **не используется**
+  (помечен «debug only», `src/db.py`).
 
-  Следствие: при re-drive (reaper → `queued` → переисполнение стадии `deploy`) с уже
+  Усугубляет дефект порядок в `_reap_known_outcome` (`src/job_reaper.py:206`): для exit0
-  слитым PR гарантия AC-11 «без второго слияния» **не обеспечена детерминированно** —
+  сначала вызывается `_gate_driven_advance(job)` (побочные эффекты — `_try_advance_stage`
-  deployer-агент повторно попытается слить уже слитый PR (Gitea вернёт ошибку
+  → `advance_stage` → `enqueue_job` следующей стадии), и **только потом** атомарный
-  merge → риск ложного БАГ-8 отката). `branch_is_behind_main`-короткое замыкание
+  `reap_running_job(..., "done")`. Это прямо противоречит ADR-001 Р-1: «Атомарный
-  (TC-17) делает идемпотентным лишь дорогой rebase+re-test merge-гейта, но НЕ сам акт
+  reap-claim. **Перед любым действием с побочными эффектами** reaper атомарно
-  слияния. Это прямое расхождение реализации с ADR и golden-source.
+  «застолбляет» строку тем же приёмом, что `claim_next_job`». Поскольку claim стоит
+  ПОСЛЕ side-effects, его guard `WHERE status='running'` не сериализует advance: даже
+  проигравший гонку reaper (rowcount==0) уже успел выполнить `advance_stage`.
 
-  Требуется одно из:
+  Последствия в окне гонки (reaper-тик попал в финализацию живого monitor'а):
-  1. **Подключить guard** к пути merge (инструкция в промпте deployer «перед merge
+  - `enqueue_job` (`src/db.py:419`) — обычный INSERT **без дедупликации**, поэтому
-     проверь `pr_already_merged` — уже слит → пометь стадию успешной без повторного
+    параллельные `advance_stage` от reaper и monitor дают **две `queued`-строки
-     merge», либо детерминированная Python-проверка перед merge), чтобы AC-11
+    следующего агента** → дублирующий запуск следующей стадии (двойной commit/PR/
-     выполнялся реально; **либо**
+    комментарии, лишние токены);
-  2. если идемпотентность намеренно достигается только gate-driven advance +
+  - либо (если гейт следующего ребра ещё красный, напр. CI не позеленел) reaper уходит
-     `branch_is_behind_main` (а `pr_already_merged` оставлен как helper «на будущее»),
+    в `_reap_unknown_outcome` → `reap_running_job(..., "queued")` и **спихивает в queued
-     **исправить ADR-001 Р-3, README и CHANGELOG**, убрав утверждение, что путь merge
+    job, который вот-вот успешно завершится** monitor'ом; при `max_concurrency=1` воркер
-     консультируется с guard, и явно описав, чем именно покрывается «без второго
+    может повторно заклеймить и **перезапустить тот же агент**.
-     слияния» из AC-11.
+
+  Это противоречит FR-1.3/AC-3 (живой агент НЕ должен реапиться) и FR-1.2/AC-4
+  (никакого дубль-advance). Требуется одно из:
+  1. ввести **grace для Tier-2** — реапить только если `finished_at_run` старше
+     заведомо-большего, чем максимальное окно финализации (использовать уже
+     запрашиваемый `finished_at_run`), и/или
+  2. **claim-before-act**: атомарно «застолбить» строку (как требует ADR Р-1) ДО любого
+     `advance_stage`/`enqueue_job`, чтобы проигравший гонку reaper не выполнял побочных
+     эффектов; advance — только после выигранного claim.
+
+  (Tier-1 этим не страдает: streak `reaper_dead_ticks` + мёртвый pid; ветка
+  `_reap_unknown_outcome` без exit0 тоже безопасна — единственное действие там и есть
+  атомарный flip.)
 
 ### P2 — Should fix
 - нет
 
+### P3 — Nice to have
+- [ ] **Битая ссылка в CHANGELOG на глобальный ADR.** В записи ORCH-065 указан
+  `docs/architecture/adr/adr-0011-job-reaper-and-lease-reclaim.md`, фактический файл —
+  `adr-0011-job-reaper-lease-reclaim.md` (без `-and-`). README и `src/job_reaper.py`
+  ссылаются корректно. Поправить путь в `CHANGELOG.md`.
+
 ## Документация
 
-Обновлена корректно и в этом же PR (AC-17 PASS по составу артефактов):
+Обновлена корректно и в этом же PR (AC-17 PASS по составу): `docs/architecture/README.md`
-`docs/architecture/README.md` (раздел про job-reaper + lease-reclaim, таблицы БД и
+(раздел про job-reaper + lease-reclaim, таблицы БД и `/queue`), `docs/architecture/internals.md`,
-`/queue`), `docs/architecture/internals.md`, `docs/architecture/adr/adr-0011-*.md`
+`docs/architecture/adr/adr-0011-job-reaper-lease-reclaim.md` (+ запись в `adr/README.md`),
-(+ запись в `adr/README.md`), `docs/work-items/ORCH-065/06-adr/ADR-001-*.md`,
+`docs/work-items/ORCH-065/06-adr/ADR-001-job-reaper-and-lease-reclaim.md`, `CHANGELOG.md`,
-`CHANGELOG.md`, `.env.example` (флаги `ORCH_REAPER_*` / `ORCH_LEASE_RECLAIM_ENABLED`).
+`.env.example` (флаги `ORCH_REAPER_*` / `ORCH_LEASE_RECLAIM_ENABLED`).
 
-Оговорка: содержание документации про `pr_already_merged` фактически неверно (см. P1) —
+Оговорка: ADR-001 Р-1 описывает «claim перед побочными эффектами», но реализация exit0-пути
-guard описан как «консультируемый перед merge», тогда как он нигде не вызывается.
+этому не следует (см. P1) — при исправлении кода либо привести код в соответствие с ADR,
-До устранения P1 документация в этой части не является достоверным golden-source.
+либо синхронизировать формулировку ADR с фактическим порядком. Битая ссылка на adr-0011 в
+CHANGELOG — P3.