reviewer(ET): auto-commit from reviewer run_id=319
This commit is contained in:
79
docs/work-items/ORCH-065/12-review.md
Normal file
79
docs/work-items/ORCH-065/12-review.md
Normal file
@@ -0,0 +1,79 @@
|
||||
---
|
||||
type: review
|
||||
work_item_id: ORCH-065
|
||||
verdict: REQUEST_CHANGES
|
||||
version: 1
|
||||
---
|
||||
|
||||
# Review ORCH-065
|
||||
|
||||
## Summary
|
||||
|
||||
Задача закрывает три связанных класса отказов «процесс/поток умер, а ресурс остался
|
||||
захваченным навсегда»: zombie jobs (A), залипший merge-lease (B), неидемпотентная
|
||||
финализация merge (C). Реализация качественная: новый daemon-поток `src/job_reaper.py`
|
||||
по образцу `reconciler` (never-raise, kill-switch, снимок в `/queue`), трёхуровневая
|
||||
liveness (Tier-1 мёртвый pid + streak, Tier-2 exit_code, Tier-3 backstop), атомарный
|
||||
reap-claim `reap_running_job(... WHERE status='running')`, gate-driven advance через
|
||||
неизменный `_try_advance_stage`, проактивный реклейм lease (`pid_alive` +
|
||||
`reclaim_stale_lease`), колонка `jobs.pid` через идемпотентный `_ensure_column`.
|
||||
Инварианты соблюдены: `STAGE_TRANSITIONS`/`QG_CHECKS`/`check_*`/БАГ-8/exit-коды хука —
|
||||
без изменений; реклейм lease — только удаление файла, без git-операций; прод-контейнер
|
||||
не трогается. Документация (README, internals, ADR, глобальный adr-0011, CHANGELOG,
|
||||
.env.example) обновлена в этом же PR. `pytest tests/ -q` — **742 passed**.
|
||||
|
||||
Блокер один: дефект Проблемы C (FR-3.2 / AC-11) — детерминированный guard
|
||||
`pr_already_merged` реализован и юнит-протестирован, но **не подключён ни к одному
|
||||
реальному пути merge**, при этом ADR и golden-source-документация утверждают обратное.
|
||||
|
||||
## Findings
|
||||
|
||||
### P0 — Blocker
|
||||
- нет
|
||||
|
||||
### P1 — Must fix
|
||||
- [ ] **`pr_already_merged` — мёртвый код в проде; ADR/доки утверждают, что он
|
||||
консультируется (FR-3.2 / AC-11, ADR-001 Р-3).** Функция `merge_gate.pr_already_merged`
|
||||
определена (`src/merge_gate.py:447`) и покрыта unit-тестами (TC-16/TC-17), но grep по
|
||||
`src/` показывает **ноль вызовов** из продакшен-кода: ни в `stage_engine.py`, ни в
|
||||
`webhooks/gitea.py`, ни в `qg/checks.py`, ни в промпте `.openclaw/agents/deployer.md`.
|
||||
Фактический merge PR в `main` выполняет LLM-агент deployer, и его промпт guard не
|
||||
упоминает. Тем временем:
|
||||
- ADR-001 Р-3: «Путь слияния (deployer/merge) **консультируется** с этим guard ПЕРЕД
|
||||
повторным merge: PR уже слит → no-op (без второго merge и без ошибки)»;
|
||||
- `docs/architecture/README.md:226`: «добавлен never-raise guard `pr_already_merged` …
|
||||
уже слит = no-op»;
|
||||
- CHANGELOG: «`pr_already_merged(...)` — guard **перед повторным merge** при re-drive».
|
||||
|
||||
Следствие: при re-drive (reaper → `queued` → переисполнение стадии `deploy`) с уже
|
||||
слитым PR гарантия AC-11 «без второго слияния» **не обеспечена детерминированно** —
|
||||
deployer-агент повторно попытается слить уже слитый PR (Gitea вернёт ошибку
|
||||
merge → риск ложного БАГ-8 отката). `branch_is_behind_main`-короткое замыкание
|
||||
(TC-17) делает идемпотентным лишь дорогой rebase+re-test merge-гейта, но НЕ сам акт
|
||||
слияния. Это прямое расхождение реализации с ADR и golden-source.
|
||||
|
||||
Требуется одно из:
|
||||
1. **Подключить guard** к пути merge (инструкция в промпте deployer «перед merge
|
||||
проверь `pr_already_merged` — уже слит → пометь стадию успешной без повторного
|
||||
merge», либо детерминированная Python-проверка перед merge), чтобы AC-11
|
||||
выполнялся реально; **либо**
|
||||
2. если идемпотентность намеренно достигается только gate-driven advance +
|
||||
`branch_is_behind_main` (а `pr_already_merged` оставлен как helper «на будущее»),
|
||||
**исправить ADR-001 Р-3, README и CHANGELOG**, убрав утверждение, что путь merge
|
||||
консультируется с guard, и явно описав, чем именно покрывается «без второго
|
||||
слияния» из AC-11.
|
||||
|
||||
### P2 — Should fix
|
||||
- нет
|
||||
|
||||
## Документация
|
||||
|
||||
Обновлена корректно и в этом же PR (AC-17 PASS по составу артефактов):
|
||||
`docs/architecture/README.md` (раздел про job-reaper + lease-reclaim, таблицы БД и
|
||||
`/queue`), `docs/architecture/internals.md`, `docs/architecture/adr/adr-0011-*.md`
|
||||
(+ запись в `adr/README.md`), `docs/work-items/ORCH-065/06-adr/ADR-001-*.md`,
|
||||
`CHANGELOG.md`, `.env.example` (флаги `ORCH_REAPER_*` / `ORCH_LEASE_RECLAIM_ENABLED`).
|
||||
|
||||
Оговорка: содержание документации про `pr_already_merged` фактически неверно (см. P1) —
|
||||
guard описан как «консультируемый перед merge», тогда как он нигде не вызывается.
|
||||
До устранения P1 документация в этой части не является достоверным golden-source.
|
||||
Reference in New Issue
Block a user