From 61b8abe13499783bfdd1d78d241eb9dac3525ed4 Mon Sep 17 00:00:00 2001 From: claude-bot Date: Tue, 9 Jun 2026 21:10:24 +0300 Subject: [PATCH] reviewer(ET): auto-commit from reviewer run_id=499 --- docs/work-items/ORCH-090/12-review.md | 135 ++++++++++++++++++++++++++ 1 file changed, 135 insertions(+) create mode 100644 docs/work-items/ORCH-090/12-review.md diff --git a/docs/work-items/ORCH-090/12-review.md b/docs/work-items/ORCH-090/12-review.md new file mode 100644 index 0000000..9e35400 --- /dev/null +++ b/docs/work-items/ORCH-090/12-review.md @@ -0,0 +1,135 @@ +--- +verdict: REQUEST_CHANGES +work_item: ORCH-090 +stage: review +author_agent: reviewer +status: changes-requested +created_at: 2026-06-09 +model_used: claude-opus-4-8 +type: review +work_item_id: ORCH-090 +version: 1 +--- + +# Review ORCH-090 — Механизм отмены задачи: статус STOP + +## Summary + +Реализация STOP-отмены сделана аккуратно, по канону прошлых задач (leaf `src/cancel.py` +never-raise, kill-switch `stop_status_enabled`, аддитивные миграции, fail-closed маршрутизация по +образцу `confirm_deploy`/ORCH-059). Кросс-каттинг `{done}` → `{done, cancelled}` проведён +исчерпывающе и консистентно (serial_gate / task_deps / stages / db / job_reaper / queue_worker), в +точности по adr-0026. Документация — golden-source качества (README, architecture/README, +internals, CHANGELOG, CLAUDE.md, .env.example, оба ADR, все номерные доки). Полный регресс +`pytest tests/` зелёный (1345 passed); новый `tests/test_stop_status.py` покрывает AC-1…AC-10 + D7. + +**Однако** обнаружен **P1** дефект в логике безопасного прерывания (AC-7 / D7): «критическое окно» +определено слишком широко и **отменённая задача self-hosting-репо застревает (wedge)** в самом +вероятном сценарии STOP — отмене задачи, ожидающей `Confirm Deploy`. Это блокирует приёмку. + +Оси проверки: ✅ ТЗ (кроме AC-7 — см. P1) · ⚠️ ADR (D7 реализован у́же, чем намеренная семантика) · +✅ качество кода · ✅ документация. + +## Findings + +### P0 — Blocker +- (нет) + +### P1 — Must fix + +- [ ] **Deferred-cancel НИКОГДА не применяется при STOP на стадии `deploy` в ожидании «Confirm + Deploy» → отменённая задача self-hosting-репо застревает и заклинивает очередь репо.** (AC-7 / + ADR-001 D7 / adr-0026; NFR-3 self-hosting). + - **Где:** `src/cancel.py::in_critical_window` (merge-lease ветка, стр. 107–114) + + `src/stage_engine.py::cancel_task` deferred-ветка (стр. ~1920–1937) + единственная точка + применения deferred-cancel в `run_deploy_finalizer` (стр. 1659–1679). + - **Сценарий (сверено по коду):** для self-hosting-репо merge-gate на ребре `deploy-staging → + deploy` держит merge-lease на PASS и **не отпускает** его до `deploy→done` / rollback (см. + `src/qg/checks.py::check_branch_mergeable`, return True на стр. 701 и 711 — без + `release_merge_lease`; подтверждено docstring'ами `_handle_merge_gate` стр. 906–907 и + `_handle_merge_verify` стр. 1132–1133). Phase A (`_handle_self_deploy_phase_a`) переводит + задачу на стадию `deploy` и ждёт ручного «Confirm Deploy», **продолжая держать lease**; + sentinel `INITIATED` ещё НЕ записан (он пишется в Phase B, после Confirm Deploy). + - Поэтому при STOP в этом состоянии `in_critical_window` возвращает `True` **только** по признаку + «держим merge-lease» → `cancel_task` уходит в **deferred**-ветку: ставит `cancel_requested_at`, + снимает лишь `queued`-job'ы (их нет), шлёт алерт и **возвращается, не отпустив lease** (выход + до шага 3c). Running-актора деплоя в Phase A нет, `run_deploy_finalizer` запускается **только** + после Phase B (Confirm Deploy), которого оператор намеренно не делает (он нажал STOP). Итог: + единственная точка применения deferred-cancel недостижима → **отмена не применяется никогда**. + - **Последствия:** (1) интент оператора молча теряется — задача НЕ отменяется (ровно тот класс + «ручной хирургии», который фича призвана устранить, BRD §1 / инцидент ORCH-087); (2) задача + остаётся на `deploy` (нетерминальна) и **держит merge-lease** → serial-gate (ORCH-088) считает + репо «активным» и блокирует вход новых задач в `analysis`, а merge-lease блокирует мержи всего + репо — до stale-reclaim ORCH-065 (частично) / ручного вмешательства. Это **самый частый + реальный сценарий STOP для self-hosting** («не катить в прод — отменить»), и фича в нём + ломается. + - **Причина:** «критическое окно» (необратимый шаг) переопределено как «держим merge-lease», но + Phase-A-ожидание Confirm Deploy **полностью обратимо** (ничего не смержено и не задеплоено). + По смыслу D7 необратимость = реально начатый необратимый шаг (sentinel `INITIATED` Phase B / + идущий merge), а не сам факт удержания lease в ожидании человека. + - **Как чинить (на выбор developer/architect, не предписываю реализацию):** сузить + `in_critical_window` так, чтобы удержание merge-lease на стадии `deploy` БЕЗ маркера `INITIATED` + и без идущего merge не считалось критическим окном (т.е. STOP в ожидании Confirm Deploy = + немедленный полный сброс, который сам отпустит lease в шаге 3c); ЛИБО добавить применение + deferred-cancel в точку, гарантированно достижимую и для merge-lease-окна (как и заявляет + ADR-001 D7: «`run_deploy_finalizer` Phase C / **`_handle_merge_verify`**» — второй финализатор в + коде не задействован). Любой вариант обязан: не оставлять задачу нетерминальной с удержанным + lease; покрыть тест-кейсом «STOP на `deploy` в ожидании Confirm Deploy → задача `cancelled`, + lease отпущен, репо разблокирован». + - **Тест-разрыв:** D7-кейсы (`test_d7_*`) покрывают только окно с `INITIATED`-маркером; окно + «держим lease, ждём Confirm Deploy» не покрыто — отсюда дефект и прошёл. + +### P2 — Should fix + +- [ ] **Deferred-ветка не идемпотентна по уведомлениям (AC-6 «нет Telegram-спама дублями»).** + `src/stage_engine.py::cancel_task` — терминальная идемпотентность (`stage in {done,cancelled}`) + защищает full-reset путь, но в критическом окне повторный STOP каждый раз заново вызывает + `set_task_cancel_requested` (идемпотентно) **и** `_notify_cancel` (Telegram + Plane-коммент) — + повторные STOP во время merge/deploy спамят дублирующими уведомлениями. Предложение: слать алерт + «отложено» лишь при первом переходе `cancel_requested_at IS NULL → NOW` (его уже умеет различать + `set_task_cancel_requested`, возвращая факт первой простановки). + +### P3 — Nice to have + +- [ ] **«Завис» `cancel_requested_at` на успешно задеплоенной задаче → вечный `pending` в + `GET /queue`.** При SUCCESS-деплое `run_deploy_finalizer` вызывает `cancel_task(force=True)`, + который видит `stage='done'` → «already-terminal» no-op и **не очищает** `cancel_requested_at`. + `db.cancelled_tasks_snapshot` считает `pending = cancel_requested_at IS NOT NULL AND stage != + 'cancelled'` → done-задача с бывшим deferred-STOP навсегда показывается «pending». Чисто + наблюдаемость; предложение — очищать `cancel_requested_at` при честном no-op после завершения. +- [ ] **adr-0026 п.6 (post-deploy monitor «не тикает по отменённой задаче») в коде не реализован** + (`run_post_deploy_monitor` не сверяет терминал задачи). Фактически безвреден и недостижим: + post-deploy наблюдение идёт только ПОСЛЕ `done`, а STOP на `done` — no-op, поэтому отменённая + задача в монитор не попадает. Рекомендация: либо снять пункт из adr-0026 как нерелевантный, либо + добавить дешёвый терминал-гард для строгого соответствия ADR. +- [ ] Косметика: «рваная» строковая склейка комментария relaunch-hole в + `src/webhooks/plane.py` (стр. 345–350) — собрать в одну строку для читаемости. + +## Документация + +**Обновлена полностью и качественно — отдельных findings нет.** Проверено пофайльно: +- `README.md` — таблица env (`ORCH_STOP_STATUS_ENABLED`/`ORCH_STOP_STATUS_REPOS`), новый раздел + «Отмена задачи: статус STOP», обновлён список job-статусов (`cancelled`), инфра-предусловие. +- `docs/architecture/README.md` — новый раздел «STOP / отмена задачи (реализовано)», обновлены + раздел «База данных» (колонки/тумбстон/статусы) и таблица API (`/queue` несёт блок `stop`). +- `docs/architecture/internals.md` — `STAGE_TRANSITIONS` (сток `cancelled`), терминал-предикат + `{done,cancelled}`, job-статусы. +- `CHANGELOG.md` (`feat:`), `CLAUDE.md` (раздел «Отмена задачи: статус STOP (ORCH-090)»), + `.env.example` — присутствуют и согласованы. +- ADR: локальный `06-adr/ADR-001-stop-cancel-task.md` + сквозной + `docs/architecture/adr/adr-0026-stop-cancel-task.md`; уточнение D4 (тумбстон `plane_issue_id`) + честно отражено и в коде, и в доках. Все номерные доки `01..10` на месте. +- Раздела README «Известные ограничения», который ORCH-090 закрывал бы (ORCH-079), нет — + обзорная витрина не рассинхронена. + +Трассировка маркеров (TRACEABILITY.md): правки маркированных инвариантов `serial_gate`/ORCH-088 и +`task_deps`/ORCH-026 сверены с их ADR — расширение терминал-набора до `{done,cancelled}` сохраняет +FIFO-семантику (`t2.id < jobs.task_id`) и dep-готовность (терминальный предшественник), инварианты +не сломаны. `STAGE_TRANSITIONS` exit-гейты / `QG_CHECKS` / `check_*` — не тронуты (подтверждено +анти-регресс-снапшотами, зелёные). + +## Вердикт + +`REQUEST_CHANGES` — из-за P1 (deferred-cancel недостижим при STOP в ожидании Confirm Deploy → +wedge self-hosting-репо). Остальное (P2/P3) — на усмотрение, но P1 обязателен к исправлению с +покрывающим тест-кейсом перед повторным review.