reviewer(ET): auto-commit from reviewer run_id=677
This commit is contained in:
121
docs/work-items/ORCH-111/12-review.md
Normal file
121
docs/work-items/ORCH-111/12-review.md
Normal file
@@ -0,0 +1,121 @@
|
||||
---
|
||||
verdict: APPROVED
|
||||
work_item: ORCH-111
|
||||
stage: review
|
||||
author_agent: reviewer
|
||||
status: approved
|
||||
created_at: 2026-06-15
|
||||
model_used: claude-opus-4-8
|
||||
type: review
|
||||
work_item_id: ORCH-111
|
||||
version: 1
|
||||
---
|
||||
|
||||
# Review ORCH-111
|
||||
|
||||
## Summary
|
||||
|
||||
PR добавляет в sidecar-watchdog новый opt-in сигнал `proc_blocking` — алерт на долго живущий
|
||||
осиротевший тест-процесс (pytest), репарентированный на хост и грузящий CPU (инцидент:
|
||||
`test_install_lite_script.py` жил >2 суток без единого алерта, валил merge-gate re-test на ORCH-109).
|
||||
|
||||
Изменение **строго внутри наблюдателя** (`watchdog/**` + сервис watchdog в `docker-compose.yml`).
|
||||
Проверено по всем 4 осям — **замечаний уровня P0/P1 нет**. Реализация 1:1 соответствует ТЗ и ADR-001
|
||||
(D1–D7), полный `pytest tests/` зелёный (**1933 passed**), watchdog+key-sync тесты зелёные
|
||||
(125 passed), `src/**` байт-в-байт не тронут, конвейерные контракты сохранены, документация
|
||||
обновлена в том же PR. **Вердикт: APPROVED.**
|
||||
|
||||
### Сверка по осям
|
||||
|
||||
**1. Соответствие ТЗ / Acceptance Criteria — PASS.**
|
||||
- FR-1 (чистый builder `proc_signals`, per-entity key `("proc_blocking", pid)`, active ⇔
|
||||
`age_s > cfg.proc_age_s`, действенный RU-`detail`: PID + возраст + усечённый cmdline + CPU) — ✓.
|
||||
- FR-2 (коллектор `watchdog/collectors/proc.py`, stdlib-only, read-only, never-raise → `[]`, чистый
|
||||
разбор отделён от I/O) — ✓.
|
||||
- FR-3 (4 ключа `WATCHDOG_PROC_*`, never-raise парсеры, дефолт-off, гейт на `proc_enabled`) — ✓.
|
||||
- FR-4 (только наблюдение — нет `os.kill`/сигналов/`subprocess`/чтения `/proc/<pid>/environ`),
|
||||
подтверждено AST-тестом `test_tc04_collector_source_is_read_only` — ✓.
|
||||
- FR-5 (диспетч через существующую `decision.decide()`/`AlertState`; RECOVERY синтезируется без новой
|
||||
анти-спам-логики) — ✓.
|
||||
- FR-6 (без дубля с `agent_hung` — по построению: cmdline-скоуп + порог возраста) — ✓.
|
||||
- AC-1…AC-10 покрыты тестами (TC-01 regress-якорь, TC-02 порог/граница, TC-03 конфиг/kill-switch,
|
||||
TC-04 never-raise/read-only, TC-05 anti-spam→realert→recovery, TC-06 partition vs agent_hung,
|
||||
TC-07 tick→dispatch + flag-off + never-raise, TC-12 compose `pid: host`).
|
||||
- §4 (API): выбран watchdog-side путь, `GET /metrics`/`schema_version` не тронуты — ✓.
|
||||
- §7 (порог): дефолт `WATCHDOG_PROC_AGE_MIN=60` (=3600s) > `max(merge_retest_timeout_s=600,
|
||||
coverage_run_timeout_s=900)=900s` (4× запас) — кросс-инвариант D2 соблюдён, анти-false-positive — ✓.
|
||||
|
||||
**2. Соответствие ADR — PASS.**
|
||||
- Реализация соответствует ADR-001 D1 (watchdog-side `/proc` под `pid: host`, НЕ orch-side `/metrics`),
|
||||
D2 (анти-FP/анти-дубль по построению), D3 (коллектор), D4 (builder + синтез RECOVERY), D5 (конфиг
|
||||
дефолт-off), D6 (`pid: host` только на `orchestrator-watchdog`), D7 (инварианты конвейера).
|
||||
- Сквозной ADR `docs/architecture/adr/adr-0041-...` создан (архитектором, commit `1d87ae5`).
|
||||
- **Трассировка (TRACEABILITY.md):** правки `watchdog/{core,signals,config}.py` и сервиса watchdog в
|
||||
compose — **аддитивные врезки**, маркированные инварианты предшественников (ORCH-100 sidecar,
|
||||
read-only-маунты) не сломаны. Существующий тест `test_host_paths_mounted_read_only` остаётся зелёным
|
||||
(`pid` — не volume), плюс добавлен позитивный `test_watchdog_shares_host_pid_namespace` (и проверка,
|
||||
что прод `orchestrator` НЕ получает `pid`).
|
||||
- Глобальные инварианты (AC-9): `STAGE_TRANSITIONS` / `QG_CHECKS` / `check_*` / machine-verdict /
|
||||
схема БД / `/metrics`+`schema_version` — **байт-в-байт не тронуты** (подтверждено: `git diff` по
|
||||
`src/` пуст).
|
||||
|
||||
**3. Качество кода — PASS.**
|
||||
- Docstrings на всех публичных функциях (коллектор/builder/synth) — содержательные, со ссылками на
|
||||
D-решения ADR.
|
||||
- Тесты содержательные, не тривиальные: red→green regress-якоря (TC-01 builder, TC-07 tick→dispatch
|
||||
«алерт даже когда ни одна стадия не stuck»), граничные (strict `>`), патологический `comm` со
|
||||
скобками/пробелами, гонка «процесс умер mid-scan», AST-скан read-only-контракта, полная
|
||||
последовательность alert→none→realert→recovery→no-repeat.
|
||||
- **Регресс-тест дефекта (ORCH-019 BR-4 / усиление):** задача эскалирована `escalate: full-cycle`
|
||||
(прошла `architecture`), но как баг несёт явные тест-фиксаторы дефекта — требование выполнено.
|
||||
- Безопасность: read-only, never-raise (per-pid + top-level), `/proc/<pid>/environ` не читается
|
||||
(секреты), cmdline усечена в алерте (анти-утечка аргументов, R-2). Привилегия `pid: host` —
|
||||
read-only, только у наблюдателя, обоснована в `07-infra-requirements.md`.
|
||||
|
||||
**4. Документация — PASS.**
|
||||
- `src/**` НЕ изменён → жёсткое правило «src изменён, доки нет → P0» не активируется; доки наблюдателя
|
||||
обновлены в том же PR независимо:
|
||||
`CHANGELOG.md` (детальная запись), `docs/architecture/README.md` (раздел `proc_blocking`),
|
||||
`docs/deployment/LITE_SETUP.md` (opt-in блок), ADR work-item `06-adr/ADR-001` + сквозной `adr-0041`,
|
||||
`.env.watchdog.example` ↔ блок `WATCHDOG_*` `.env.example` (key-sync тест `test_lite_setup_doc.py`
|
||||
зелёный).
|
||||
- Стадийное владение артефактами соблюдено (rule 3/4): TRZ/AC/test-plan — analyst (`a0218a2`),
|
||||
ADR/infra/risks/architecture-README — architect (`1d87ae5`), код/CHANGELOG/env/compose/LITE_SETUP —
|
||||
developer (`6c83191`). Ретроактивных правок ТЗ/ADR нет.
|
||||
|
||||
## Findings
|
||||
|
||||
### P0 — Blocker
|
||||
- Нет.
|
||||
|
||||
### P1 — Must fix
|
||||
- Нет.
|
||||
|
||||
### P2 — Should fix
|
||||
- Нет.
|
||||
|
||||
### P3 — Nice to have (не блокируют)
|
||||
- [ ] Опечатка в комментарии `watchdog/collectors/proc.py` (top-level `except`): «one signal tih» →
|
||||
«one signal **тих**» (косметика).
|
||||
- [ ] Витрина `docs/overview/` (ORCH-011): описывает sidecar-watchdog абстрактно («опрашивает
|
||||
`/metrics`, шлёт алерты»), каталог сигналов не перечисляет. Новый opt-in (дефолт-off) сигнал
|
||||
абстрактную формулировку не противоречит, машинно-проверяемые факты не меняет
|
||||
(`tests/test_system_docs.py` зелёный) → обновление витрины **не требуется**. Опционально можно
|
||||
упомянуть новую способность наблюдателя (видимость хост-процессов) — не блокирует.
|
||||
- [ ] Индекс `docs/architecture/adr/README.md` не содержит `adr-0041` — но это **предсуществующий
|
||||
долг** (отсутствуют также `adr-0038`/`0039`/`0040` из 3 ранее влитых PR), не свойство этого PR и не
|
||||
per-PR-контракт. Не вменяется ORCH-111.
|
||||
|
||||
## Документация
|
||||
|
||||
Документация обновлена корректно и в полном объёме **в том же PR**:
|
||||
- `CHANGELOG.md` — запись `[Unreleased]` с разбивкой по D-решениям. ✓
|
||||
- `docs/architecture/README.md` — раздел `proc_blocking` в описании sidecar-watchdog. ✓
|
||||
- `docs/deployment/LITE_SETUP.md` — opt-in блок с проверкой (NFR-5). ✓
|
||||
- `docs/work-items/ORCH-111/06-adr/ADR-001-...` + сквозной `docs/architecture/adr/adr-0041-...`. ✓
|
||||
- `.env.watchdog.example` ↔ `.env.example` (блок `WATCHDOG_*`) — синхронизированы, key-sync тест
|
||||
зелёный. ✓
|
||||
|
||||
Изменения функционала наблюдателя задокументированы, конвейерные контракты не тронуты, выкат
|
||||
пересобирает только `orchestrator-watchdog` (прод `orchestrator` не рестартится, NFR-3).
|
||||
**Документация = golden source: требование выполнено.**
|
||||
Reference in New Issue
Block a user