From a153c8ef3dc8f3e100dce3edb98ca9d517921bcf Mon Sep 17 00:00:00 2001 From: claude-bot Date: Wed, 10 Jun 2026 09:28:03 +0300 Subject: [PATCH] reviewer(ET): auto-commit from reviewer run_id=570 --- docs/work-items/ORCH-100/12-review.md | 155 +++++++++++++++----------- 1 file changed, 88 insertions(+), 67 deletions(-) diff --git a/docs/work-items/ORCH-100/12-review.md b/docs/work-items/ORCH-100/12-review.md index 2b5b9e7..3753e88 100644 --- a/docs/work-items/ORCH-100/12-review.md +++ b/docs/work-items/ORCH-100/12-review.md @@ -8,27 +8,36 @@ created_at: 2026-06-10 model_used: claude-opus-4-8 type: review work_item_id: ORCH-100 -version: 1 +version: 2 --- -# Review ORCH-100 — FND/F1b: sidecar-watchdog +# Review ORCH-100 — FND/F1b: sidecar-watchdog (re-review) ## Summary Аддитивная реализация sidecar-наблюдателя в отдельном контейнере `orchestrator-watchdog` -(папка `watchdog/`, тонкий Python-3.12-stdlib-only демон). Проверено по 4 осям. Реализация -**точно** соответствует ТЗ (FR-1…FR-11) и ADR-001 (D1…D9): отдельный контейнер, толерантный -парсинг `/metrics`, debounce `orch_down`, read-only `docker.sock` (GET-only по построению), -обобщённая чистая `decide`, независимый Telegram-канал, структурный анти-дубль диск-алерта (D6), -трёхуровневый never-raise, kill-switch. +(папка `watchdog/`, тонкий Python-3.12-stdlib-only демон). Это **повторное ревью** после цикла +`testing → development`: предыдущий прогон тестера дал `result: FAIL` из-за единственного красного +теста `tests/test_queue.py::TestRetry::test_finalize_job_requeue_then_fail`; развработчик закрыл +причину тест-only фиксом (коммит `2040de3` — autouse-фикстура `_isolate_runs_dir` в +`tests/conftest.py`, **без правок `src/**`**). -**Корневой инвариант соблюдён:** PR не трогает `src/**` ни одной строкой (подтверждено -`git diff --stat -- 'src/**'` → пусто) ⇒ `STAGE_TRANSITIONS` / `QG_CHECKS` / `check_*` / -machine-verdict ключи / схема БД орка — байт-в-байт прежние. Документация обновлена -исчерпывающе. 66 тестов `tests/watchdog/` зелёные. +Проверено по 4 осям. Реализация **точно** соответствует ТЗ (FR-1…FR-11) и ADR-001 (D1…D9): +отдельный контейнер, толерантный парсинг `/metrics` (D9), debounce `orch_down` (FR-3, порог +`orch_down_ticks`), read-only `docker.sock` (`_get` хардкодит `GET` — read-only **по построению** + +mount `:ro`), обобщённая чистая `decide` (D4, 1:1 семантика `disk_watchdog`), независимый +Telegram-канал (свои токены, ноль импортов `src/**`), структурный анти-дубль диск-алерта (D6, +opt-in потолок), трёхуровневый never-raise (per-source/per-tick/per-send), kill-switch (idle-loop, +не exit). -**Вердикт: APPROVED.** P0/P1, относящихся к этому PR, нет. Ниже — P2-замечания (не блокируют) -и одна эскалация по pre-existing красному тесту вне области F1b. +**Корневой инвариант соблюдён:** PR не трогает `src/**` ни одной строкой за всю ветку, включая +fix-коммит (`git diff origin/main...HEAD --stat -- 'src/**'` → пусто) ⇒ `STAGE_TRANSITIONS` / +`QG_CHECKS` / `check_*` / machine-verdict ключи / схема БД орка — байт-в-байт прежние. + +**Блокер тестирования снят.** Полный регресс `pytest tests/` теперь **зелёный (1617 passed)`**, +профильная сюита `tests/watchdog/` — **66/66 PASS**. Документация обновлена исчерпывающе. + +**Вердикт: APPROVED.** P0/P1 нет. Ниже — анализ снятого блокера и P2/P3-замечания (не блокируют). ## Findings @@ -39,79 +48,91 @@ machine-verdict ключи / схема БД орка — байт-в-байт - (нет) ### P2 — Should fix -- [ ] **Заявление «полный регресс `tests/ -q` зелёный» неточно (CHANGELOG / AC-7).** Полный прогон - `pytest tests/` даёт **1 падение**: `tests/test_queue.py::TestRetry::test_finalize_job_requeue_then_fail`. - **Важно:** этот тест **байт-в-байт идентичен `origin/main`** (`git diff origin/main...HEAD -- - tests/test_queue.py` → пусто), а PR не меняет `src/**` — падение **pre-existing на main**, НЕ - вызвано ORCH-100 и **структурно неустранимо в рамках F1b** (ТЗ §2 прямо запрещает правки `src/**`). - Поэтому это **не основание для REQUEST_CHANGES** этого PR. Действие: скорректировать формулировку - «полный tests/ зелёный» → «`tests/watchdog/` зелёные (66); один pre-existing красный тест на main - вне области F1b» и завести отдельную баг-задачу (см. Escalation). Источник: AC-7 - (`03-acceptance-criteria.md`), CHANGELOG-запись ORCH-100. -- [ ] **FR-4: host inode/CPU не реализованы как сигналы.** ТЗ FR-4 упоминает «диск (% и, где - доступно, inode), память, CPU»; ADR-001 D5 (реестр сигналов) сузил host-метрики до `host_mem` + - opt-in `host_disk_crit`, и этот реестр реализован полностью. Host-CPU (loadavg) и inode в сигналы - не превращены — `watchdog/collectors/host.py` их не собирает (host-CPU/«завис» покрыт через - `agent_hung` из `/metrics`). Это документированное сужение на стадии архитектуры (FR-4 - формулирует inode как «где доступно»), не нарушение. Замечание: docstring ADR D3 для `host.py` - упоминает «CPU (loadavg)», которого в реализации нет — расхождение комментария и кода; либо - реализовать host-CPU/inode-сигнал, либо снять упоминание из docstring D3. Источник: `02-trz.md` - FR-4, `ADR-001` D3/D5. +- [ ] **ADR-001 D3: docstring-блок структуры `host.py` упоминает «CPU (loadavg)», которого в + реализации нет.** ADR D3 (строка с `host.py # ... / CPU (loadavg)`) перечисляет CPU/inode среди + host-метрик, но реестр сигналов D5 сознательно сузил host до `host_mem` + opt-in `host_disk_crit`, + а host-CPU/«завис» покрыт через `agent_hung` из `/metrics`. Сам `watchdog/collectors/host.py` + внутренне консистентен (его docstring явно пишет «CPU ... computed from the /metrics envelope, not + here»), inode FR-4 оговорён как «где доступно» — это документированное сужение на стадии + архитектуры, **не нарушение ТЗ**. Замечание косметическое: привести строку D3 в соответствие с D5 + (снять «CPU (loadavg)»/inode из блока структуры). Источник: `ADR-001` D3/D5, `02-trz.md` FR-4. ### P3 — Nice-to-have -- [ ] **CLAUDE.md не обновлён.** Паспорт проекта не получил запись о F1b. Прецедент: парная задача - F1a (ORCH-099) также отсутствует в CLAUDE.md (grep → 0) — это семейство наблюдаемости в паспорте - не трекается, а золотой архитектурный док (`docs/architecture/README.md`) покрывает F1b - исчерпывающе (компонентная строка + полная секция §«Sidecar-watchdog F1b»). Опционально для - единообразия с другими операционными демонами (`disk_watchdog`/`reaper`) — добавить краткую - TL;DR-строку. -- [ ] **`watchdog/collectors/containers.py::DockerSockReader.list_containers` не вызывается** в - `core.tick` (используется только `inspect(name)` по списку `cfg.containers`). Публичный read-метод - оставлен для полноты API/тестов — безвреден; при желании отметить как explicit-API или удалить. +- [ ] **CLAUDE.md не обновлён.** Паспорт проекта не получил TL;DR-запись о F1b. Прецедент: парная + задача F1a (ORCH-099) также отсутствует в CLAUDE.md (`grep` → 0) — семейство наблюдаемости в + паспорте не трекается, а золотой архитектурный док (`docs/architecture/README.md`) покрывает F1b + исчерпывающе. Опционально для единообразия с операционными демонами (`disk_watchdog`/`reaper`). +- [ ] **`DockerSockReader.list_containers` не вызывается** в `core.tick` (используется только + `inspect(name)` по `cfg.containers`). Публичный read-метод оставлен для полноты API/тестов + (`test_docker_readonly.py`) — безвреден; при желании пометить как explicit-API. + +## Анализ снятого блокера (testing FAIL → development fix) + +- **Причина прежнего FAIL:** `test_finalize_job_requeue_then_fail` (run_id=1/2) читал хвост + `/.log`. Дефолтный `runs_dir` указывал на прод-каталог + `/app/data/runs`, где на self-hosting-хосте лежат реальные накопленные `*.log`; реальный `2.log` + с токеном «429» переключал классификацию `permanent → transient` (requeue вместо `failed`). Это + **ambient prod-pollution окружения, не дефект кода** — сам тест байт-в-байт идентичен + `origin/main`, а `src/**` ORCH-100 не трогает. +- **Фикс (коммит `2040de3`):** autouse-фикстура `_isolate_runs_dir` редиректит `settings.runs_dir` + на per-test `tmp_path` ⇒ `_run_log_path()` резолвится в несуществующий файл ⇒ + `classify_log_file()` возвращает документированный дефолт `permanent` ⇒ детерминированный, + не зависящий от окружения результат для всей сюиты. Зеркалит существующие autouse-фикстуры + `_no_telegram`/`_disable_merge_verify`/`_reset_webhook_secrets`. +- **Это НЕ «подгонка теста под код»:** тело теста не изменено; добавлена только изоляция окружения + (test-infra). Фикс улучшает гигиену всей сюиты и устраняет скрытую env-зависимость. Прежний + диагноз тестера («реальное красное, ловящее расхождение requeue→finalize в launcher») оказался + ошибочным — корень был в загрязнении прод-логами; артефакт тестера (`13-test-report.md`) не правлю + (чужая стадия), фиксирую факт здесь. +- **Верификация (независимо):** `git diff origin/main...HEAD --stat -- 'src/**'` → пусто (включая + fix-коммит); изолированный прогон `test_finalize_job_requeue_then_fail` → **1 passed**; полный + `pytest tests/` → **1617 passed**; `tests/watchdog/` → **66 passed**. +- **Багфикс-трек (ORCH-019 BR-4):** задача — `feat`/FND (не `Bug`) ⇒ требование + регресс-теста-фиксатора не применяется. Фикс окружения, тем не менее, детерминирует поведение + всей сюиты. ## Документация **Обновлена исчерпывающе — golden source синхронизирован с кодом:** - ✅ `docs/architecture/README.md` — новая компонентная строка (Sidecar-watchdog F1b) + полная - секция «## Sidecar-watchdog F1b (ORCH-100 — design)» + перекрёстная ссылка в секции F1a. + секция дизайна F1b + перекрёстная ссылка из секции F1a. - ✅ `CHANGELOG.md` — детальная запись F1b (стек D1 / топология D2 / decide D4 / реестр сигналов D5 / - анти-дубль D6 / транспорт D7 / never-raise D8). + анти-дубль D6 / транспорт D7 / never-raise D8) **+** отдельная строка fix-коммита `2040de3` + (`_isolate_runs_dir`). - ✅ `docs/work-items/ORCH-100/06-adr/ADR-001-sidecar-watchdog.md` + сквозной `docs/architecture/adr/adr-0033-sidecar-watchdog.md` (оба с корректным frontmatter). - ✅ `docs/work-items/ORCH-100/07-infra-requirements.md` — разовое инфра-предусловие (сервис в compose, bot/chat watchdog, `.env.watchdog`, первый запуск). -- ✅ `.env.example` — канон всех `WATCHDOG_*` ключей, **без реальных секретов** (токены пустые). -- ⚠️ **CLAUDE.md** — не обновлён (P3, см. выше; прецедент F1a — допустимо). +- ✅ `.env.example` — канон всех `WATCHDOG_*` ключей, **без реальных секретов** (`TG_BOT_TOKEN`/ + `TG_CHAT_ID` пустые). +- ⚠️ **CLAUDE.md** — не обновлён (P3, прецедент F1a — допустимо). - ✅ **README «Известные ограничения» (ось ORCH-079):** F1b — новая способность (внешний - наблюдатель), **ни один** из 3 открытых пунктов витрины не закрывается этим PR ⇒ обновления - обзорной витрины не требуется (ось удовлетворена). + наблюдатель); **ни один** из 3 открытых пунктов витрины (Telegram-48h / intra-repo task-deps / + пакетный автоном Этап 1) не закрывается этим PR ⇒ обновления обзорной витрины не требуется. -**Поскольку `src/**` НЕ изменён, P0 «src изменён, документация не обновлена» не активируется**; -при этом документация всё равно обновлена сверх минимума. +**`src/**` НЕ изменён ⇒ P0 «src изменён, документация не обновлена» не активируется**; документация +при этом обновлена сверх минимума. ## Проверки инвариантов (явно) -- `git diff origin/main...HEAD --stat -- 'src/**'` → **пусто** (src нетронут; STAGE_TRANSITIONS / - QG_CHECKS / check_* / схема БД — байт-в-байт). +- `git diff origin/main...HEAD --stat -- 'src/**'` → **пусто** за всю ветку, включая fix-коммит + (STAGE_TRANSITIONS / QG_CHECKS / check_* / схема БД — байт-в-байт). - `docker.sock` смонтирован `:ro` (compose) И код GET-only по построению (`_get` хардкодит `GET`, - нет ни одного мутирующего метода) — двойная гарантия read-only (AC-6). -- Нет импорта `src/**` из `watchdog/**` (C-1 / BR-8) — независимый Telegram-транспорт со своими - токенами; падение орка не утянет алерт-канал. -- never-raise: per-source (коллекторы), per-tick (`__main__` + `core._dispatch`), per-send - (`notify`) — все три уровня присутствуют. -- kill-switch `WATCHDOG_ENABLED=false` → idle-loop (НЕ exit) — restart-policy не крутит петлю. + ни одного мутирующего метода/`POST`/start/stop/restart/exec) — двойная гарантия read-only (AC-6). +- Нет импорта `src/**` из `watchdog/**` (`grep` → пусто; C-1 / BR-8) — независимый Telegram-транспорт + со своими токенами; падение орка не утянет алерт-канал. +- never-raise: per-source (коллекторы `_collect_*`), per-tick (`__main__.run` + `core._dispatch`), + per-send (`notify`/`_send`) — все три уровня присутствуют (TC-06). +- kill-switch `WATCHDOG_ENABLED=false` → idle-loop (НЕ exit) — restart-policy не крутит петлю (TC-07). - `mem_limit: 128m` + `mem_reservation: 32m`; stdlib-only (нет requirements/pip-дерева) — тонкость - C-3 соблюдена. -- Багфикс-трек (ORCH-019 BR-4): задача — `feat`/FND, не `Bug` ⇒ требование регресс-теста-фиксатора - не применяется. + C-3 соблюдена; compose-сервис изолирован (деплой watchdog НЕ пересобирает/рестартит `orchestrator`). +- Анти-дубль диска (D6/AC-5): `host_disk_crit` opt-in (`disk_crit_enabled=False` по умолчанию) на + более высоком потолке (97%) — структурно один владелец 85%-события (`disk_watchdog`/ORCH-063). ## Escalation -- **Pre-existing красный тест на `main`:** `tests/test_queue.py::TestRetry::test_finalize_job_requeue_then_fail` - падает стабильно (3/3 изолированных прогона) и идентичен `origin/main`. Он **не вызван** ORCH-100 - и **неустраним в области F1b** (правки `src/**` запрещены ТЗ §2). Однако он **завалит** downstream - merge-gate re-test (ORCH-043 гоняет `pytest tests/` на догнанной ветке) и нарушает AC-7 «полный - tests/ зелёный» на уровне репозитория. **Рекомендация:** Owner/планировщику завести отдельную - баг-задачу (метка `Bug`, маршрут ORCH-019) на починку `test_finalize_job_requeue_then_fail` в - `src/**`; без неё прод-выкат ORCH-100 упрётся в merge-gate по причине, не относящейся к F1b. Это - эскалация, а не finding против данного PR. +- Нет открытых эскалаций. Прежняя эскалация ревью v1 / тест-репорта (pre-existing красный тест) — + **закрыта** fix-коммитом `2040de3` (test-only изоляция окружения, `src/**` не тронут). Полный + регресс `pytest tests/` зелёный (1617 passed) ⇒ downstream merge-gate re-test (ORCH-043) по этой + причине более не упрётся. Отдельная баг-задача на `test_finalize_job_requeue_then_fail` **не + требуется**: корнем было загрязнение прод-логами, а не дефект `src/**`.