89 lines
6.2 KiB
Markdown
89 lines
6.2 KiB
Markdown
---
|
||
type: review
|
||
work_item_id: ORCH-053
|
||
verdict: APPROVED
|
||
version: 1
|
||
---
|
||
|
||
# Review ORCH-053 — Sweeper потерянных webhook (реконсиляция застрявших стадий)
|
||
|
||
## Summary
|
||
PR реализует фоновый reconciler застрявших стадий ровно в объёме ТЗ (`02-trz.md`) и
|
||
ADR (`06-adr/ADR-001`, глобальный `adr-0007`). Все 17 acceptance-criteria покрыты
|
||
кодом и тестами; полный прогон `pytest` — **563 passed**. Реализация строго следует
|
||
ключевым инвариантам: продвижение только через неизменный `advance_stage(...,
|
||
finished_agent=None)`, никакой дублирующей advance/rollback-логики в `reconciler.py`,
|
||
структурная невозможность спама нотификаций, never-raise на единицу работы,
|
||
restart-safe daemon-поток, kill-switch'и. Схема БД и реестры `STAGE_TRANSITIONS` /
|
||
`QG_CHECKS` не тронуты. Документация обновлена в этом же PR. Рекомендация: **APPROVED**.
|
||
|
||
## Соответствие ТЗ
|
||
- `src/reconciler.py` (НОВЫЙ): F-1 `reconcile_gate_once` + F-2 `reconcile_plane_once`, класс
|
||
`Reconciler` + module-singleton по образцу `queue_worker`. ✓
|
||
- `src/config.py`: все 6 `reconcile_*` настроек с дефолтами по таблице §5. ✓
|
||
- `src/main.py`: старт после `worker.start()`, стоп перед `worker.stop()`, блок `reconcile`
|
||
в `GET /queue`. ✓
|
||
- `src/stage_engine.py`: тонкий `advance_if_gate_passed` — read-only пред-оценка гейта,
|
||
advance только через `advance_stage`, на красном гейте `advance_stage` не вызывается
|
||
вовсе (подавление спама без изменения общего критпути). ✓
|
||
- `src/plane_sync.py`: `list_issues_by_state` с курсорной пагинацией и never-raise → `[]`. ✓
|
||
- `src/webhooks/gitea.py`: F-3 БД-fallback `sha→branch` (`_resolve_branch_via_db`),
|
||
однозначность обязательна, `debug→info`. ✓
|
||
- `src/webhooks/plane.py` + `src/db.py`: F-2 переиспользует `handle_status_start` /
|
||
`handle_verdict` без дублирования; анти-дубль `create_task_atomic` под process-wide Lock,
|
||
`start_pipeline` рефакторен на atomic-claim первым DB-действием. ✓
|
||
- Схема БД и реестры не менялись (§6/§8 ТЗ). ✓
|
||
|
||
## Соответствие ADR
|
||
- §2 (источник истины — гейт; продвижение только через `advance_stage`): соблюдено —
|
||
в `reconciler.py` нет собственного `update_task_stage`/`enqueue_job` для advance (AC-2).
|
||
- §3 (async-обработчики из sync-потока через `asyncio.run`): реализовано в `_dispatch`.
|
||
- §4 (atomic-claim под `threading.Lock`, без миграции): `db.create_task_atomic`.
|
||
- §6 (F-1 не трогает `analysis`): ранний возврат в `advance_if_gate_passed` и в
|
||
`_reconcile_gate_task` (AC-16).
|
||
- §7 (grace «потерян, а не задержан»): F-1 по `tasks.updated_at` (SQL `age_s`), F-2 по
|
||
`issue.updated_at` (`_age_seconds_iso`).
|
||
- Нарушений глобальных ADR нет; `adr-0007` заведён и внесён в `docs/architecture/adr/README.md`.
|
||
|
||
## Качество кода
|
||
- Контракт never-raise выдержан на всех уровнях: outer loop, per-task, per-project, per-issue,
|
||
`_parse_grace_overrides`, `list_issues_by_state`, `_resolve_branch_via_db`, телеграм-нотификация.
|
||
- Идемпотентность: active-job guard в F-1 и F-2; самозатухание через обновление `updated_at`
|
||
после advance; `max_concurrency=1`. Подтверждено анализом — F-2 на approved/rejected всегда
|
||
меняет состояние (analysis approved-via-status всегда проходит; rollback всегда срабатывает),
|
||
поэтому петли спама нотификаций структурно не возникает.
|
||
- Защита от ложного матча в F-3 (только при единственной development-задаче repo).
|
||
- Docstrings содержательные на всех публичных функциях; тесты не тривиальные (мапятся на
|
||
TC-01…TC-21 из `04-test-plan.yaml`).
|
||
|
||
## Документация
|
||
Обновлена в этом же PR (AC-17 выполнен):
|
||
- `docs/architecture/README.md` — компонент Reconciler, раздел resilience, строка в таблице API
|
||
(`/queue` … + reconcile), footer-пометка. ✓
|
||
- `docs/work-items/ORCH-053/06-adr/ADR-001-stuck-task-reconciler.md` — заведён. ✓
|
||
- `docs/architecture/adr/adr-0007-reconciler.md` + строка в `adr/README.md`. ✓
|
||
- `CHANGELOG.md` — запись в `[Unreleased]/Added`. ✓
|
||
- `docs/operations/INFRA.md` — kill-switch'и и env-карта (self-hosting). ✓
|
||
- `README.md` и `.env.example` — env-таблица `ORCH_RECONCILE_*`. ✓
|
||
|
||
## Findings
|
||
|
||
### P0 — Blocker
|
||
- Нет.
|
||
|
||
### P1 — Must fix
|
||
- Нет.
|
||
|
||
### P2 — Should fix
|
||
- Нет.
|
||
|
||
### P3 — Nice-to-have
|
||
- Несоответствие статуса ADR: `06-adr/ADR-001` помечен `Статус: Proposed`, тогда как
|
||
`docs/architecture/adr/README.md` указывает `adr-0007` как `accepted`. Косметика —
|
||
привести к одному значению при следующем касании.
|
||
- `get_project_states(pid)` теоретически может вернуть словарь без ключей
|
||
`approved`/`rejected` при частичном резолве состояний проекта → `KeyError` в
|
||
`_reconcile_plane_project`. Сейчас изолировано per-project `try/except` (never-raise
|
||
держится, эффект — пропуск F-2 для проекта). Можно усилить `.get(...)`-доступом ради
|
||
явности; не блокер.
|