reviewer(ET): auto-commit from reviewer run_id=667
This commit is contained in:
@@ -1,14 +1,14 @@
|
||||
---
|
||||
verdict: REQUEST_CHANGES # APPROVED | REQUEST_CHANGES — строго одно из двух, UPPERCASE
|
||||
verdict: APPROVED # APPROVED | REQUEST_CHANGES — строго одно из двух, UPPERCASE
|
||||
work_item: ORCH-109
|
||||
stage: review
|
||||
author_agent: reviewer
|
||||
status: changes-requested
|
||||
status: approved
|
||||
created_at: 2026-06-14
|
||||
model_used: claude-opus-4-8
|
||||
type: review
|
||||
work_item_id: ORCH-109
|
||||
version: 1
|
||||
version: 2
|
||||
---
|
||||
|
||||
# Review ORCH-109
|
||||
@@ -21,35 +21,41 @@ version: 1
|
||||
`STAGE_TRANSITIONS`/`QG_CHECKS`/`check_*`/machine-verdict/схема БД — байт-в-байт (AC-9 верифицирован
|
||||
по диффу: в `src/` изменены только `launcher.py` и `config.py`, ни одной контракт/схема-строки).
|
||||
Инварианты ORCH-087 (стамп эффорта — объединён в один `UPDATE`, не сломан) и ORCH-065 (reaper
|
||||
`5400 > 3600+20`) сохранены и закреплены тестами. Покрытие исчерпывающее: новый
|
||||
`tests/test_orch109_timeout_model.py` (TC-01…TC-12, детерминированный), полный регресс
|
||||
**1899 passed** (exit 0).
|
||||
`5400 > 3600+20=3620`) сохранены и закреплены тестами. Покрытие исчерпывающее: новый
|
||||
`tests/test_orch109_timeout_model.py` (TC-01…TC-12, детерминированный, без сети/CLI), обновлены
|
||||
`tests/test_config.py` (reaper-дефолт 5400) и `tests/test_launcher.py` (лестница `_resolve_timeout`).
|
||||
Полный регресс **1899 passed** (exit 0).
|
||||
|
||||
**Блокер ровно один — документация.** PR изменил поведение тайм-аута в `src/` и корректно обновил
|
||||
инженерный справочник `docs/architecture/internals.md` (30 мин → per-role), но **пропустил
|
||||
front-page витрину `README.md`**: раздел «### Watchdog» (стр. 295) по-прежнему утверждает «Каждый
|
||||
агент имеет timeout 30 минут», что теперь фактически неверно (developer 60м / reviewer 50м / прочие
|
||||
30м). Обзорная витрина не должна выдавать устаревший факт за текущий (ORCH-079/ORCH-011) → **P1**,
|
||||
вердикт `REQUEST_CHANGES`. Правка тривиальна (1 строка), остальной PR — эталонного качества.
|
||||
**Единственный блокер прошлой итерации (P1 README front-page «### Watchdog», устаревший факт «timeout
|
||||
30 минут») закрыт** — commit `91d56fe docs(readme): sync Watchdog section with per-role timeout
|
||||
budgets`: `README.md:295` приведён в соответствие с `internals.md` (developer 60м / reviewer 50м /
|
||||
прочие 30м дефолт, `_resolve_timeout`, ORCH-109) + Tier-3 backstop `reaper_max_running_s`=90м (ORCH-065).
|
||||
Обзорная витрина `docs/overview/` конкретного числа тайм-аута не несёт (упоминания watchdog —
|
||||
концептуальные: sidecar-наблюдатель и «следит за процессом»), поэтому правки не требует. Открытых
|
||||
findings P0/P1/P2 нет → вердикт **APPROVED**.
|
||||
|
||||
## Оси проверки
|
||||
|
||||
1. **Соответствие ТЗ (02-trz / 03-acceptance):** FR-1…FR-6 реализованы; AC-1…AC-10 покрыты тестами
|
||||
TC-01…TC-12. ✅ (кроме AC-10 в части README — см. P1).
|
||||
TC-01…TC-12. AC-10 (документация) теперь закрыт полностью, включая front-page README. ✅
|
||||
2. **Соответствие ADR (06-adr ADR-001 + сквозной adr-0040):** D1–D6 реализованы дословно — объединённый
|
||||
`UPDATE agent_runs SET model=?, effort=? WHERE id=?` с `(model or None, effort or None, run_id)`;
|
||||
лестница `_resolve_timeout` (overrides_json → выделенный ключ роли → глобальный дефолт); выделенные
|
||||
ключи `agent_timeout_developer_s=3600`/`agent_timeout_reviewer_s=3000`; `reaper_max_running_s`
|
||||
3600→5400. Трассировка маркера **ORCH-087** (стамп эффорта): правка добавляет `model` в тот же
|
||||
`UPDATE`, эффорт-стамп `(effort or None)` сохранён — зафиксированный инвариант не сломан. ✅
|
||||
3. **Качество кода:** never-raise сохранён (`try/except` + WARNING вокруг стамп-UPDATE и вокруг резолва
|
||||
выделенного ключа); непозитивный/нечисловой ключ → откат на дефолт + WARNING; докстринги/комментарии
|
||||
точные; тесты содержательные (позитивные контроли `test_tc11_clean_exit_advances`,
|
||||
`test_tc09_unstamped_killed_run_drops_model_suffix` как negative-guard, параметризация
|
||||
`[0,-5,"abc"]`). ✅
|
||||
4. **Документация (приоритетная ось):** обновлены CHANGELOG / CLAUDE.md / docs/architecture/README.md /
|
||||
internals.md / .env.example / config.py-паспорт / детальный ADR + сквозной adr-0040. **Один пропуск
|
||||
во front-page витрине `README.md` → P1** (см. ниже).
|
||||
3600→5400. Трассировка маркеров **ORCH-087** (эфорт-стамп `(effort or None)` сохранён в том же
|
||||
`UPDATE`) и **ORCH-065** (reaper поднят синхронно, инвариант жив) — зафиксированные инварианты не
|
||||
сломаны. Нарушений глобальных ADR нет. ✅
|
||||
3. **Качество кода:** never-raise сохранён (`try/except` + WARNING вокруг стамп-`UPDATE`; непозитивный/
|
||||
нечисловой выделенный ключ → откат на глобальный дефолт + WARNING); докстринг `_resolve_timeout`
|
||||
и комментарии точны; тесты содержательны — позитивные контроли (`test_tc11_clean_exit_advances`),
|
||||
negative-guard (`test_tc09_unstamped_killed_run_drops_model_suffix`), параметризация `[0,-5,"abc"]`,
|
||||
изоляция стамп-сбоя (TC-05). Регресс-тест-фиксатор инцидента ORCH-104 присутствует (ORCH-019 BR-4
|
||||
удовлетворён). ✅
|
||||
4. **Документация (приоритетная ось):** `src/` изменён (`launcher.py`/`config.py`) → документация
|
||||
обновлена в том же PR: CHANGELOG / CLAUDE.md (паспорт) / docs/architecture/README.md (бюллет Agent
|
||||
Launcher + ссылка на adr-0040) / docs/architecture/internals.md (оба упоминания «30 мин») /
|
||||
.env.example (5 ключей agent-timeout + `ORCH_REAPER_MAX_RUNNING_S`=5400) / config.py-паспорт /
|
||||
детальный ADR-001 + сквозной adr-0040 / **README.md front-page «### Watchdog»** (закрытый P1). ✅
|
||||
|
||||
## Findings
|
||||
|
||||
@@ -57,44 +63,38 @@ front-page витрину `README.md`**: раздел «### Watchdog» (стр.
|
||||
- (нет)
|
||||
|
||||
### P1 — Must fix
|
||||
- [ ] **`README.md:294–295` (раздел «### Watchdog») — устаревший факт во front-page витрине.** Строка
|
||||
«Каждый агент имеет timeout 30 минут. При превышении — SIGKILL + запись exit_code=-9.» противоречит
|
||||
поведению, введённому этим PR (per-role бюджеты: developer 60м / reviewer 50м / прочие 30м дефолт,
|
||||
`_resolve_timeout`, ORCH-109). PR изменил `src/` (тайм-аут) и обновил инженерный
|
||||
`docs/architecture/internals.md`, но **обзорную витрину `README.md` не синхронизировал** — читатель
|
||||
фронт-страницы видит решённое/изменённое как старое. Нарушает правило агентов №2/№6 и ось «обзорные
|
||||
доки» (ORCH-079) / «витрина системы» (ORCH-011): необновление обзорной витрины при изменении
|
||||
описанной в ней функциональности → finding ≥ P1.
|
||||
**Fix:** привести `README.md:295` в соответствие с `internals.md` — указать per-role бюджеты
|
||||
(developer 60м / reviewer 50м / прочие 30м дефолт, `_resolve_timeout`, ORCH-109) и, при желании,
|
||||
упомянуть Tier-3 backstop `reaper_max_running_s`=90м. (Каскад SIGTERM→grace→SIGKILL — pre-existing,
|
||||
не предмет этого PR; достаточно поправить число «30 минут».)
|
||||
- (нет)
|
||||
|
||||
### P2 — Should fix
|
||||
- (нет)
|
||||
|
||||
### P3 — Nice-to-have
|
||||
- [ ] ADR-001 и adr-0040 несут `status: proposed`/`Proposed`. На merge их разумно перевести в
|
||||
`Accepted` (косметика статуса ADR; не блокер — артефакты архитектора консистентны, на гейты/код
|
||||
не влияют).
|
||||
|
||||
## Документация
|
||||
|
||||
**Обновлено в этом PR (golden source синхронизирован с кодом):**
|
||||
- `CHANGELOG.md` — детальная запись ORCH-109 (`fix`, D1/D3/D4, FR-4/FR-5 структурно). ✅
|
||||
- `CLAUDE.md` — паспорт (блок «Стек», абзац launcher). ✅
|
||||
- `docs/architecture/README.md` — бюллет Agent Launcher (ссылка на adr-0040). ✅
|
||||
- `docs/architecture/internals.md` — watchdog 30 мин → per-role (стр. 96 и 262). ✅
|
||||
- `docs/architecture/internals.md` — watchdog «30 мин» → per-role (стр. 96 и 262). ✅
|
||||
- `README.md` — front-page «### Watchdog» (стр. 295) → per-role бюджеты + Tier-3 backstop
|
||||
(закрыт P1 прошлой итерации, commit `91d56fe`). ✅
|
||||
- `.env.example` — новый блок agent-timeout (5 ключей) + `ORCH_REAPER_MAX_RUNNING_S` 3600→5400. ✅
|
||||
- `src/config.py` — паспорт-комментарий ORCH-7/ORCH-109 + reaper-инвариант. ✅
|
||||
- ADR — `docs/work-items/ORCH-109/06-adr/ADR-001-…` (детальный) + `docs/architecture/adr/adr-0040-…`
|
||||
(сквозной). ✅
|
||||
|
||||
**Требует обновления в этом же PR (блокер P1):**
|
||||
- `README.md:295` (раздел «### Watchdog») — единственный активный устаревший факт «timeout 30 минут»
|
||||
(см. P1). Прочие совпадения «30 мин/1800/3600» в дереве — замороженные историко-номерные артефакты
|
||||
work-items (ORCH-053/065/016/098) и `docs/history/*` (правке не подлежат, правило №3), либо корректно
|
||||
актуализированный `internals.md`.
|
||||
**Обзорная витрина `docs/overview/` (ORCH-011/ORCH-079):** правки не требует — упоминания watchdog
|
||||
концептуальны (sidecar-наблюдатель, «следит за процессом»), конкретного числа тайм-аута витрина не
|
||||
несёт, поэтому устаревшего факта не возникает.
|
||||
|
||||
**Прочее (не findings):**
|
||||
- Полный регресс `pytest tests/` — **1899 passed** (exit 0). Предупреждение
|
||||
`PytestUnhandledThreadExceptionWarning: no such table: agent_runs` в `_monitor_agent` (launcher.py:860)
|
||||
— pre-existing гонка тестового потока с teardown БД в **неизменённом** этим PR коде (дифф трогает
|
||||
только `_spawn` ≈566 и `_resolve_timeout` ≈670); не блокер, не регресс ORCH-109.
|
||||
- Полный регресс `pytest tests/` — **1899 passed** (exit 0, 7m25s). Предупреждение
|
||||
`PytestUnhandledThreadExceptionWarning: disk I/O error` в `_monitor_agent` (launcher.py:860) —
|
||||
pre-existing гонка тестового потока с teardown БД в **неизменённом** этим PR коде (дифф трогает
|
||||
только `_spawn` ≈566–585 и `_resolve_timeout` ≈673–724); не блокер, не регресс ORCH-109.
|
||||
- AC-9 верифицирован: в `src/` изменены только `launcher.py` и `config.py`; ни одной строки
|
||||
`STAGE_TRANSITIONS`/`QG_CHECKS`/`check_*`/machine-verdict/`CREATE TABLE`/`ALTER TABLE` в диффе нет.
|
||||
|
||||
Reference in New Issue
Block a user