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

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

@@ -1,34 +1,47 @@
 ---
-verdict: REQUEST_CHANGES
+verdict: APPROVED
 work_item: ORCH-090
 stage: review
 author_agent: reviewer
-status: changes-requested
+status: approved
 created_at: 2026-06-09
 model_used: claude-opus-4-8
 type: review
 work_item_id: ORCH-090
-version: 1
+version: 2
 ---
 
-# Review ORCH-090 — Механизм отмены задачи: статус STOP
+# Review ORCH-090 — Механизм отмены задачи: статус STOP (re-review, attempt 2)
 
 ## 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.
+Повторный review после фикса блокирующего P1 из предыдущей итерации (`12-review.md` v1).
+Реализация 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.
 
-**Однако** обнаружен **P1** дефект в логике безопасного прерывания (AC-7 / D7): «критическое окно»
-определено слишком широко и **отменённая задача self-hosting-репо застревает (wedge)** в самом
-вероятном сценарии STOP — отмене задачи, ожидающей `Confirm Deploy`. Это блокирует приёмку.
+**Оба ранее блокировавших/важных дефекта закрыты и покрыты содержательными тестами:**
+- **P1 (был blocker) — ИСПРАВЛЕН.** `cancel.in_critical_window` сужен: удержание merge-lease без
+  бегущего актора (`_task_has_running_actor`) на стадии `deploy` в ожидании `Confirm Deploy` теперь
+  НЕ считается критическим окном → немедленный полный сброс, который сам отпускает lease (шаг 3c).
+  Тесты `test_d7_lease_held_idle_parking_is_not_critical`,
+  `test_d7_lease_held_with_running_actor_still_critical`,
+  `test_d7_stop_on_deploy_awaiting_confirm_full_resets` (последний прямо проверяет
+  `stage='cancelled'` + удалённую ветку + `current_lease_holder is None`). Сверено по коду
+  `src/cancel.py::in_critical_window` (стр. 100–158) и `stage_engine.cancel_task` — wedge
+  self-hosting-репо устранён.
+- **P2 (был should-fix) — ИСПРАВЛЕН.** Deferred-ветка `cancel_task` шлёт алерт только при первом
+  переходе (`first = set_task_cancel_requested(...)`, далее `if first:`); повторный STOP в
+  критическом окне даёт `deferred-already-pending` без повторного уведомления. Тест
+  `test_d7_repeated_stop_in_critical_window_no_duplicate_notify` (ровно 1 notify).
 
-Оси проверки: ✅ ТЗ (кроме AC-7 — см. P1) · ⚠️ ADR (D7 реализован у́же, чем намеренная семантика) ·
-✅ качество кода · ✅ документация.
+Полный регресс `pytest tests/` зелёный (**1349 passed**); `tests/test_stop_status.py` — 30 кейсов
+(TC-01…TC-14 + D7), покрывают AC-1…AC-10 и оба фикса.
+
+Оси проверки: ✅ ТЗ/AC (AC-1…AC-10, включая ранее проваленный AC-7) · ✅ ADR (соответствие
+adr-0026/ADR-001; см. P2-нит ниже) · ✅ качество кода · ✅ документация.
 
 ## Findings
 
@@ -36,100 +49,66 @@ internals, CHANGELOG, CLAUDE.md, .env.example, оба ADR, все номерны
 - (нет)
 
 ### 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`, возвращая факт первой простановки).
+- [ ] **Work-item ADR-001 §D7 не синхронизирован с фиксом P1 (running-actor-уточнение).**
+  `docs/work-items/ORCH-090/06-adr/ADR-001-stop-cancel-task.md` §D7 (стр. 189–201) по-прежнему
+  определяет критическое окно как «задача держит merge-lease … / merge в процессе» — **без**
+  оговорки «И активно бегущий актор», которую фактически реализует код
+  (`cancel.in_critical_window` + `_task_has_running_actor`) после фикса P1. Авторитетные
+  golden-source доки уже синхронизированы (`CLAUDE.md` — абзац «Уточнение P1 (ORCH-090 review)»;
+  `docs/architecture/README.md` стр. 316–317 «P1-уточнение»; `CHANGELOG.md` — буллет «Фикс P1»),
+  поэтому витрина проекта корректна и это **не** P0 «src изменён, доки не обновлены». Но per
+  «documentation = golden source» работа-айтемный ADR (запись именно этого архитектурного решения)
+  должен честно отражать итоговую семантику — как это уже сделано для уточнения D4. Предложение:
+  добавить в §D7 строку-уточнение «merge-lease критичен ТОЛЬКО при бегущем акторе; припаркованное
+  ожидание `Confirm Deploy` обратимо → немедленный сброс» (ссылка на review P1). Не блокирует.
 
 ### 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 после завершения.
+  `GET /queue`** (перенесено из v1, не адресовано). При 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 как нерелевантный, либо
+  (перенесено из v1). Фактически безвреден и недостижим: post-deploy наблюдение идёт только ПОСЛЕ
+  `done`, а STOP на `done` — no-op. Рекомендация: снять пункт из adr-0026 как нерелевантный либо
   добавить дешёвый терминал-гард для строгого соответствия ADR.
-- [ ] Косметика: «рваная» строковая склейка комментария relaunch-hole в
-  `src/webhooks/plane.py` (стр. 345–350) — собрать в одну строку для читаемости.
+- [ ] **Косметика:** «рваная» строковая склейка комментария relaunch-hole в
+  `src/webhooks/plane.py` (стр. 345–351) — собрать в одну строку для читаемости.
 
 ## Документация
 
-**Обновлена полностью и качественно — отдельных findings нет.** Проверено пофайльно:
-- `README.md` — таблица env (`ORCH_STOP_STATUS_ENABLED`/`ORCH_STOP_STATUS_REPOS`), новый раздел
-  «Отмена задачи: статус STOP», обновлён список job-статусов (`cancelled`), инфра-предусловие.
-- `docs/architecture/README.md` — новый раздел «STOP / отмена задачи (реализовано)», обновлены
-  раздел «База данных» (колонки/тумбстон/статусы) и таблица API (`/queue` несёт блок `stop`).
+**Обновлена полностью и качественно — отдельных blocking-findings нет.** Проверено пофайльно:
+- `README.md` — таблица env (`ORCH_STOP_STATUS_ENABLED`/`ORCH_STOP_STATUS_REPOS`), раздел «Отмена
+  задачи: статус STOP (ORCH-090)», обновлён список job-статусов (`cancelled`), инфра-предусловие.
+- `docs/architecture/README.md` — раздел STOP со статусом «реализовано», блок `stop` в `/queue`,
+  раздел «База данных» (колонки/тумбстон/статусы) **и P1-уточнение** (стр. 316–317).
 - `docs/architecture/internals.md` — `STAGE_TRANSITIONS` (сток `cancelled`), терминал-предикат
   `{done,cancelled}`, job-статусы.
-- `CHANGELOG.md` (`feat:`), `CLAUDE.md` (раздел «Отмена задачи: статус STOP (ORCH-090)»),
-  `.env.example` — присутствуют и согласованы.
+- `CHANGELOG.md` (`feat:` + отдельный буллет «Фикс P1»), `CLAUDE.md` (раздел «Отмена задачи: статус
+  STOP (ORCH-090)» с абзацем «Уточнение P1»), `.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), нет —
-  обзорная витрина не рассинхронена.
+  отражено в коде и доках. Единственный gap — §D7 локального ADR не дотянут до running-actor-фикса
+  (P2 выше).
+- Раздела README «Известные ограничения», который ORCH-090 закрывал бы (ORCH-079), нет — обзорная
+  витрина не рассинхронена.
 
-Трассировка маркеров (TRACEABILITY.md): правки маркированных инвариантов `serial_gate`/ORCH-088 и
-`task_deps`/ORCH-026 сверены с их ADR — расширение терминал-набора до `{done,cancelled}` сохраняет
+**Трассировка маркеров (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.
+`APPROVED` — оба ранее найденных дефекта (P1 wedge при STOP в ожидании Confirm Deploy; P2
+дубль-уведомления в deferred-ветке) исправлены и покрыты содержательными тестами; полный регресс
+зелёный (1349 passed). Остаются только P2 (синхронизация §D7 локального ADR) и P3 (наблюдаемость/
+косметика) — не блокируют приёмку, желательны к устранению попутно.