From ec932264db6f4b376c47e895925a8824a096df2b Mon Sep 17 00:00:00 2001 From: claude-bot Date: Tue, 16 Jun 2026 20:24:55 +0300 Subject: [PATCH] reviewer(ET): auto-commit from reviewer run_id=768 --- docs/work-items/ORCH-124/12-review.md | 152 ++++++++++++++++---------- 1 file changed, 96 insertions(+), 56 deletions(-) diff --git a/docs/work-items/ORCH-124/12-review.md b/docs/work-items/ORCH-124/12-review.md index fd86a0b..55440b5 100644 --- a/docs/work-items/ORCH-124/12-review.md +++ b/docs/work-items/ORCH-124/12-review.md @@ -1,14 +1,14 @@ --- -verdict: APPROVED +verdict: REQUEST_CHANGES work_item: ORCH-124 stage: review author_agent: reviewer -status: approved +status: changes-requested created_at: 2026-06-16 model_used: claude-opus-4-8 type: review work_item_id: ORCH-124 -version: 1 +version: 2 --- # Review ORCH-124 — serial-gate «пауза без блокировки» (per-task park-сигнал) @@ -18,69 +18,86 @@ version: 1 Багфикс (метка `Bug`, эскалирован в full-cycle) инцидента ORCH-116/ORCH-123: serial-gate считал «активной» любую задачу со стадией вне `{done,cancelled}`, а Plane-статусы Backlog/Blocked/Needs-Input (слой B) не меняют `tasks.stage` (слой A) ⇒ приостановленный предшественник держал FIFO-гейт закрытым -против срочного успешника. Решение — **новая ортогональная ось планировщика «пауза»**: аддитивная -колонка `tasks.paused_at TEXT` + терм `AND paused_at IS NULL` во всех 3 точках определения «активной -задачи», операторские эндпоинты `POST /serial-gate/pause|resume`, под независимым под-флагом +против срочного успешника. Решение — **ортогональная ось планировщика «пауза»**: аддитивная колонка +`tasks.paused_at TEXT` + терм `AND paused_at IS NULL` во всех 3 точках определения «активной задачи», +операторские эндпоинты `POST /serial-gate/pause|resume`, под независимым под-флагом `serial_gate_pause_enabled`. -Реализация **точно соответствует** ADR-001 (D1–D9) и закрывает все FR/AC из ТЗ. Проверено по 4 осям — -**замечаний уровня P0/P1 нет**. Все тесты зелёные (15 новых TC + 51 существующий serial-gate + -151 db/queue/claim/task_deps — без регресса). Документация обновлена полностью. **Вердикт: APPROVED.** +**Ядро реализации (код + ТЗ + ADR + тесты) — высокого качества и P0/P1-замечаний по сути фикса не +имеет.** Реализация 1:1 соответствует ADR-001 (D1–D9), закрывает все FR/AC, машинные инварианты не +тронуты, полный прогон `pytest tests/` зелёный (**2178 passed** на текущем HEAD; ранее красный +order-dependent `test_orch123::test_r2…` исправлен изоляцией `repos_dir` в коммите `3a19728`). + +**Но есть один P1 по оси документации:** PR меняет описанную в **витрине системы** +(`docs/overview/tech-pipeline.md`) функциональность serial-gate (маршрут задач внутри репо), но +витрину **не обновил** — нарушение норматива сопровождения `docs/overview/README.md` и правила агентов +§6 (ORCH-011/ORCH-079). Предыдущий ревью (run 766) ошибочно заключил «serial-gate не фигурирует в +витрине»; по факту serial-gate описан в 4 файлах витрины, и `tech-pipeline.md` несёт **абсолютное +утверждение FIFO**, которое ORCH-124 делает неполным. **Вердикт: REQUEST_CHANGES** (исправляется +обновлением витрины в этом же PR). ## Проверка по осям ### 1. Соответствие ТЗ (`02-trz.md` / `03-acceptance-criteria.md`) — ✅ -- FR-1 (пауза исключает из горячего SQL): `build_claim_clause` — терм внутри существующего `EXISTS`, - offline, fail-OPEN сохранён. ✓ -- FR-2 (зеркало + снапшот согласованы): один предикат во всех 3 точках; анти-дрейф проверен **TC-11**. ✓ +- FR-1 (пауза исключает из горячего SQL): `build_claim_clause` — терм `AND t2.paused_at IS NULL` внутри + существующего `EXISTS`, offline, fail-OPEN сохранён (внутри того же try/except). ✓ +- FR-2 (зеркало + снапшот согласованы): один предикат во всех 3 точках; анти-дрейф — **TC-11**. ✓ - FR-3 (явное durable DB-намерение): колонка `paused_at` + `set/clear/is_task_paused` + эндпоинты; durable через рестарт — **TC-06**. ✓ - FR-4 (анти-stale-base при resume): через существующие механизмы (D8), без новой rebase-машинерии. ✓ - FR-5 (причина ожидания): `_waiting_reason` (`freeze`→`dependency`→`active-task`→`null`) + ключ - `paused`; проверено **TC-10**. ✓ -- FR-6 (явные блоки сохранены): пауза НЕ обходит `repo_freeze`/`task_deps` — **TC-08/TC-09**. ✓ + `paused` — **TC-10**. ✓ +- FR-6 (явные блоки сохранены): пауза НЕ обходит `repo_freeze`/`task_deps` — **TC-08/TC-09** + (подтверждено: `task_deps` читает только терминал `{done,cancelled}`, не `paused_at`). ✓ - FR-7 (условность/нейтральность): под-флаг + scope `serial_gate_repos`; kill-switch off байт-в-байт — - **TC-14** (enduro вне scope не затронут). ✓ -- AC-1…AC-10 — все покрыты TC-01…TC-15 буквально по файлам/тестам. + **TC-14**. ✓ +- AC-1…AC-10 покрыты TC-01…TC-15 буквально по файлам/тестам; прогон зелёный. -### 2. Соответствие ADR (`06-adr/ADR-001`, глобальный `adr-0051`) — ✅ +### 2. Соответствие ADR (`06-adr/ADR-001`, сквозной `adr-0051`) — ✅ Реализация = ADR 1:1: D1 (явный per-task флаг), D2 (`tasks.paused_at` через `_ensure_column`, паттерн `cancelled_at`/`track`), D3 (ортогональная ось — терминал `{done,cancelled}` **байт-в-байт**), D4 (3 точки согласованно), D5 (наблюдаемость), D6 (`serial_gate_pause_enabled`), D7 (эндпоинты), D8 (анти-stale-base реюзом), D9 (never-raise/fail-directions). - **Трассировка (ORCH-078/TRACEABILITY):** правка горячего SQL с маркерами `ORCH-088`/`ORCH-090` — - инвариант FIFO (`t2.id < jobs.task_id`, R-7) и терминал-множество `{done,cancelled}` (adr-0026) - **не сломаны** (подтверждено структурным **TC-15** + сверкой `task_deps`/`stages.py` не читают - `paused_at`). Маркер `ORCH-124` размещён рядом; сводный сквозной `adr-0051` заведён. ✓ + инвариант FIFO (`t2.id < jobs.task_id`, R-7) и терминал-множество `{done,cancelled}` (adr-0026) **не + сломаны** (структурный **TC-15** + сверка: `src/task_deps.py`/`src/stages.py` `paused_at` не читают). + Маркер `ORCH-124` размещён рядом; сводный сквозной `adr-0051` заведён. ✓ - Нет нарушений глобальных ADR. `STAGE_TRANSITIONS`/`QG_CHECKS`/`check_*`/machine-verdict/схемы - существующих таблиц — не тронуты (`src/stages.py`, `src/qg/`, `src/frontmatter.py` вне диффа). ✓ + существующих таблиц — не тронуты (`src/stages.py`, `src/qg/`, `src/frontmatter.py` вне диффа — + подтверждено по `git diff --name-only`). ✓ ### 3. Качество кода — ✅ - Чисто, читаемо; docstrings на всех новых публичных функциях; маркеры/ссылки на ADR в коде. - never-raise на всех публичных функциях `serial_gate`/`db`-хелперах; hot-claim fail-OPEN, - freeze fail-CLOSED — направления не инвертированы (**TC-13**). -- **Багфикс-трек (ORCH-019 BR-4):** обязательный регресс-тест-фиксатор **TC-01** воспроизводит - инцидент ORCH-116/ORCH-123 (красный до фикса — опирается на отсутствовавшие `set_task_paused`/ - `paused_at`; зелёный после). ✓ -- Тесты содержательные (не тривиальные): 15 TC покрывают регресс, анти-регресс ORCH-088, durable/ - restart, resume, сохранность freeze/dependency, анти-дрейф 3 точек, offline hot-path, never-raise, + freeze fail-CLOSED — направления не инвертированы (**TC-13**, инъекция ошибки в `_pause_layer_enabled` + → `build_claim_clause()==""` → claim не падает). +- Helper-сигнатуры выверены по коду: `db.get_task_by_work_item_id` существует (`SELECT *` ⇒ `paused_at` + попадает в `dict`), `notifications.link_for`/`send_telegram` существуют. `resume`-эндпоинт сознательно + НЕ гейтится `_pause_layer_enabled()` (позволяет снять «залипшую» паузу после выключения слоя) — + корректное решение, не дефект. +- **Багфикс-трек (ORCH-019 BR-4) — выполнен:** обязательный регресс-тест-фиксатор **TC-01** + воспроизводит инцидент (красный до фикса — опирается на отсутствовавшие `set_task_paused`/`paused_at`; + зелёный после); «до-фикса»-поведение бага дополнительно зафиксировано в **TC-14** (kill-switch off → + паузнутая задача снова блокирует). ✓ +- Тесты содержательные (15 TC, не тривиальные): регресс, анти-регресс ORCH-088, durable/restart, + resume, сохранность freeze/dependency, анти-дрейф 3 точек, offline hot-path, never-raise, kill-switch-нейтральность, структурный анти-регресс реестров/схем. -- Helper-сигнатуры выверены: `get_task_by_work_item_id` → `dict|None` (совместимо с `.get()`), - `link_for`/`send_telegram` существуют; `claim_next_job` сплайсит `build_claim_clause()` через - **существующую** точку (не модифицирована). +- **Тест-гигиена (коммит `3a19728`):** изоляция `settings.repos_dir` в фикстуре + `tests/test_orch123_staging_runner_exec.py` устранила order-dependent FAIL `test_r2…` (из-за фолбэка + `check_staging_status` на реальный `/orchestrator/.../15-staging-log.md` после мержа + ORCH-123). Инвариант ORCH-123 R-2 сохранён; правка только теста — корректно. Чуть вне строгой области + ORCH-124, но прозрачно задокументирована в CHANGELOG. Не блокирует. -### 4. Документация (обязательная ось) — ✅ обновлена в том же PR -- `docs/architecture/README.md` — раздел serial-gate + ось «пауза», таблица БД (`paused_at`), - таблица API (`/serial-gate/pause|resume`). ✓ -- `docs/architecture/internals.md` — ось «пауза» ⊥ оси «терминальность». ✓ -- `CHANGELOG.md` — развёрнутая запись (механизм/хранилище/ось/3 точки/эндпоинты/анти-stale-base/ - наблюдаемость/откат/покрытие/доки). ✓ -- `.env.example` — `ORCH_SERIAL_GATE_PAUSE_ENABLED`. ✓ -- ADR: `06-adr/ADR-001` + сквозной `adr/adr-0051` + `08-data-requirements.md`. ✓ -- **Обзорные доки (ORCH-079):** в `README.md` «Известные ограничения» нет открытого пункта, - закрываемого этим PR (п.3 — эпик ORCH-088 batch-autonomy, шире данного багфикса) → обновление не - требуется. **Витрина `docs/overview/` (ORCH-011):** serial-gate не фигурирует в витрине (внутренний - планировщик, не бизнес-способность) → обновление не требуется. ✓ +### 4. Документация (обязательная ось) — ⚠️ обновлена НЕПОЛНО (см. P1) +- ✅ `docs/architecture/README.md` (раздел serial-gate + ось «пауза», таблица БД `paused_at`, таблица + API `/serial-gate/pause|resume`), `docs/architecture/internals.md` (ось «пауза» ⊥ «терминальность»), + `CHANGELOG.md` (развёрнуто), `.env.example` (`ORCH_SERIAL_GATE_PAUSE_ENABLED`), ADR `06-adr/ADR-001` + + сквозной `adr-0051`, `08-data-requirements.md` — обновлены качественно. +- ❌ **`docs/overview/` (витрина системы) НЕ обновлена** при изменении описанного в ней маршрута задач + serial-gate → **P1** (детали ниже). serial-gate присутствует в витрине (grep: `tech-pipeline.md`, + `tech-data-model.md`, `tech-observability.md`, `tech-quality-security.md`). +- ✅ Root `README.md` «Известные ограничения»: п.3 — про эпик ORCH-088 (пакетный автоном), данным + багфиксом не закрывается → обновления не требует. ## Findings @@ -88,23 +105,46 @@ D8 (анти-stale-base реюзом), D9 (never-raise/fail-directions). - Нет. ### P1 — Must fix -- Нет. +- [ ] **Витрина системы `docs/overview/` не обновлена при изменении описанной в ней функциональности + serial-gate (ORCH-011/ORCH-079, правило агентов §6 + норматив `docs/overview/README.md`).** + `docs/overview/tech-pipeline.md` §«Последовательность внутри репозитория (serial gate)» (стр. 103–108) + несёт **абсолютное** утверждение маршрута: *«Новая задача репозитория не входит в работу, пока не + завершена более ранняя (FIFO)»* и уже **перечисляет исключение freeze** («следующие задачи ждут»). + ORCH-124 вводит **второе намеренное исключение** — оператор может поставить предшественника на паузу, + и срочный успешник его **обгоняет**. После фикса утверждение FIFO неполно/вводит в заблуждение, а + норматив сопровождения витрины (`docs/overview/README.md`: «маршруты задач → `tech-pipeline.md`») + требует синхронного обновления в том же PR. + **Как исправить:** добавить в `tech-pipeline.md` (раздел serial gate) краткую фразу про ось «пауза + без блокировки» (оператор паузит предшественника → срочный успешник обгоняет; пауза ≠ cancel, не + обходит freeze/dependency), рядом с уже описанным исключением freeze. Рекомендуется заодно (та же ось, + во избежание повторного REQUEST_CHANGES): `tech-data-model.md` — упомянуть durable-сигнал + `tasks.paused_at` рядом с `repo_freeze`; `tech-observability.md` — ключ `paused`/`reason` в блоке + `serial_gate` `GET /queue` и операторские эндпоинты `pause|resume`. Машинные доковые тесты + (`test_system_docs.py`) сейчас зелёные — это пробел **точности прозы**, который машинно не ловится. ### P2 — Should fix -- Нет. +- [ ] **Мусорные хвостовые теги `` / `` в 4 golden-source доках этого PR.** + Литерально закоммичены в конце файлов: `docs/work-items/ORCH-124/06-adr/ADR-001-serial-gate-pause-without-blocking.md` + (стр. 299–300), `docs/architecture/adr/adr-0051-serial-gate-pause-without-blocking.md` (стр. 111), + `docs/work-items/ORCH-124/08-data-requirements.md` (стр. 54), `docs/work-items/ORCH-124/10-tech-risks.md` + (стр. 41) — протёкшие в контент закрывающие XML-теги tool-call'а архитектора. Frontmatter цел, машинный + парсинг не страдает, но golden-source доки не должны нести этот мусор. (Системный pre-existing паттерн — + 52 файла по репо, в т.ч. в уже смерженном ORCH-114 → отдельная гигиеническая зачистка уместна; но 4 + файла **этого** PR следует почистить здесь.) ### P3 — Nice-to-have (не блокирует) -- [ ] HTTP-уровневых тестов для `POST /serial-gate/pause|resume` нет (терминальный no-op / - под-флаг-off no-op / success+telegram). Не блокирует: это тонкая обёртка над **полностью - покрытыми** `db.set/clear/is_task_paused`, а ветвь гейта (SQL-предикат) протестирована исчерпывающе; - соответствует конвенции репо — соседние операторские эндпоинты (`/serial-gate/unfreeze`, - `/transition-lease/release`) также не покрыты HTTP-тестами. Опционально: добавить TestClient-кейсы. -- [ ] `.task-dev.md` (dev-скретч с метаданными задачи) закоммичен с содержимым ORCH-124 — pre-existing - трекаемый паттерн, вне области ORCH-124; кандидат на `.gitignore` отдельной гигиенической задачей. +- [ ] HTTP-уровневых тестов для `POST /serial-gate/pause|resume` нет (терминальный no-op / под-флаг-off + no-op / success+telegram). Не блокирует: тонкая обёртка над полностью покрытыми `db.set/clear/is_task_paused`, + а SQL-предикат гейта протестирован исчерпывающе; соответствует конвенции репо (соседние операторские + эндпоинты `/serial-gate/unfreeze`, `/transition-lease/release` тоже без HTTP-тестов). +- [ ] `.task-dev.md` (dev-скретч) закоммичен с метаданными ORCH-124 — pre-existing трекаемый паттерн, + вне области ORCH-124; кандидат на `.gitignore` отдельной задачей. ## Документация -**Обновлена полностью в том же PR.** `src/` изменён (config/db/main/serial_gate) — и golden-source доки -(`README.md` архитектуры, `internals.md`, `CHANGELOG.md`, `.env.example`), оба ADR (work-item + сквозной -`adr-0051`) и `08-data-requirements.md` синхронно обновлены. Обзорная витрина (`README.md` ограничения, -`docs/overview/`) обновления не требует (нет затронутого пункта/способности). Структурные доковые тесты -(`test_system_docs.py`, `test_no_host_hardcodes.py`) зелёные. Замечаний по документации нет. +`src/` изменён (`config`/`db`/`main`/`serial_gate`). Golden-source **инженерные** доки (`README.md` +архитектуры, `internals.md`, `CHANGELOG.md`, `.env.example`), оба ADR (work-item + сквозной `adr-0051`) +и `08-data-requirements.md` обновлены синхронно и качественно. **Однако обзорная витрина `docs/overview/` +(`tech-pipeline.md`, обязательно; `tech-data-model.md`/`tech-observability.md`, рекомендуется) НЕ +обновлена**, хотя ORCH-124 меняет описанный в ней маршрут задач serial-gate — это P1 (ORCH-011/ORCH-079). +Документация = golden source наравне с кодом; до устранения P1 — `REQUEST_CHANGES`. После добавления +оси «пауза» в витрину (и зачистки P2-тегов) ядро фикса готово к приёмке.