From de009822c0538c31dd62493e29f861b66a256dcc Mon Sep 17 00:00:00 2001 From: claude-bot Date: Wed, 10 Jun 2026 10:49:49 +0300 Subject: [PATCH] reviewer(ET): auto-commit from reviewer run_id=581 --- docs/work-items/ORCH-098/12-review.md | 103 ++++++++++---------------- 1 file changed, 41 insertions(+), 62 deletions(-) diff --git a/docs/work-items/ORCH-098/12-review.md b/docs/work-items/ORCH-098/12-review.md index 0414999..21c4248 100644 --- a/docs/work-items/ORCH-098/12-review.md +++ b/docs/work-items/ORCH-098/12-review.md @@ -14,79 +14,58 @@ version: 1 # Review ORCH-098 — FND: машинный журнал уроков ## Summary -Чистая аддитивная реализация observer-leaf'а `src/lessons.py` + таблица `lessons` строго по образцу -`serial_gate`/`coverage_gate`/`metrics`. Все 9 критериев приёмки (AC-1…AC-9) выполнены, ТЗ (FR-1…FR-6) -реализовано полностью, реализация байт-в-байт соответствует ADR-001 (D1…D6). Инварианты конвейера -не тронуты, документация обновлена в том же PR. Тесты зелёные (13 в `test_lessons.py`, broader sanity -17 passed). P0/P1 findings нет → **APPROVED**. -## Соответствие ТЗ (ось 1) -- **FR-1 / AC-1, AC-2** ✅ `CREATE TABLE IF NOT EXISTS lessons` в `db.init_db()` со всеми полями + 3 индекса; - колонки атрибуции (`attribution`/`target_repo`/`target_domain`) присутствуют сразу, нуллабельны, - плюс forward-safe `_ensure_column` на старой таблице. TC-01/TC-02 подтверждают идемпотентность и - nullable-атрибуцию. -- **FR-2 / AC-6** ✅ `record()` — kill-switch-first no-op, never-raise обёртка над `db.record_lesson`. -- **FR-3 / AC-3** ✅ Реализованы **4** типа автозаписи (> минимума 2–3): `gate_failure` - (`_handle_qg_failure_rollbacks`), `merge_hold` (`_handle_merge_verify` HOLD), `transient_retry` - (`launcher._finalize_transient` на исчерпании бюджета), `deploy_degraded` (post-deploy DEGRADED). - Каждая врезка обёрнута в локальный `try/except` → ошибка импорта/записи не пробивается в горячий путь. - Переменные контекста (`work_item_id`/`repo`/`branch`/`checks_failed`/`checks_total`) проверены в - scope перед точкой врезки в `run_post_deploy_monitor`. TC-08/TC-09 — интеграционные. -- **FR-4 / AC-4** ✅ `GET /lessons` (фильтры type/status/repo/work_item/limit, всегда 200, read-only) + - read-only ключ `lessons` в `GET /queue`. -- **FR-5 / AC-5** ✅ `POST /lessons` (`source="manual"`, не дедупится) + `POST /lessons/{id}` (update со - стампом `updated_at`). TC-07/TC-11. -- **FR-6 / AC-7** ✅ `lessons_enabled=False` → все функции инертны без обращения к БД, эндпоинты отдают - `{"enabled": false}`. TC-05. +Реализация полностью соответствует ТЗ (`02-trz.md`), критериям приёмки (`03-acceptance-criteria.md`) +и ADR-001/adr-0034. Введён чистый observer-leaf `src/lessons.py` (never-raise, единственный +kill-switch `lessons_enabled`, без repo-скоупа — по решению D2), аддитивная идемпотентная таблица +`lessons` с нуллабельными колонками атрибуции сразу (NFR-6, требование Славы 10.06), 4 типа +автозаписи best-effort, дедуп для `auto`, три HTTP-эндпоинта + блок `lessons` в `GET /queue`. -## Соответствие ADR (ось 2) -- D1 (таблица/индексы/forward-safe), D2 (no repo-scope, kill-switch only), D3 (4 детектора), D4 (дедуп - один indexed-SELECT по `idx_lessons_wi_type`, только `auto`, окно `lessons_dedup_window_s`), D5 - (эндпоинты), D6 (изоляция от гейтов) — реализованы как описано. -- **AC-8 проверен диффом:** `src/stages.py` (`STAGE_TRANSITIONS`) и `src/qg/` (`QG_CHECKS`/`check_*`) — - **не затронуты**; machine-verdict-ключи и схемы существующих таблиц не тронуты. Journal не участвует - в решении гейта (TC-12). -- **Трассировка маркеров:** правок чужих `ORCH-NNN`-инвариантов нет — изменения чисто аддитивные - (новые блоки в choke-point'ах, существующая логика не переписана). Слом инвариантов отсутствует. +**Инварианты конвейера не тронуты (AC-8):** `src/stages.py` (`STAGE_TRANSITIONS`), `src/qg/checks.py` +(`QG_CHECKS`/`check_*`), `src/merge_gate.py`, machine-verdict-ключи и схемы существующих таблиц — +**диффом не затронуты** (подтверждено `git diff --name-only`). `tests/test_lessons.py` (TC-01…TC-12, +13 тестов) — **зелёный** локально. Документация обновлена в том же PR. -## Качество кода (ось 3) -- never-raise контракт соблюдён на всех публичных функциях leaf'а и во всех 4 врезках; DDL-хелперы - `db.py` корректно открывают/закрывают соединение в `finally` (паттерн `coverage_baseline`). -- Тесты содержательные: idempotency, nullable-атрибуция, never-raise с замоканной падающей БД (TC-04), - kill-switch (TC-05), дедуп vs manual-passthrough (TC-07b), интеграция автозаписи (TC-08/TC-09), - эндпоинты (TC-10/TC-11), инварианты конвейера (TC-12). `pytest tests/test_lessons.py -q` → 13 passed. -- **Регресс-тест (ORCH-019/BR-4):** N/A — задача не багфикс (foundation-feature, метки `Bug` нет, - полный маршрут с `architecture`/ADR). +Все findings — P2/P3 (advisory), блокеров нет. ## Findings ### P0 — Blocker -- нет +- Нет. ### P1 — Must fix -- нет +- Нет. ### P2 — Should fix -- [ ] **Грубость дедупа `transient_retry` в `launcher`.** Врезка в `_finalize_transient` передаёт - `task_id`, но **не** `work_item_id` и не `stage`, тогда как дедуп-ключ D4 = `(work_item_id, - lesson_type, stage)`. Для launcher-уроков оба = `NULL`, поэтому два разных задания, исчерпавших - бюджет транзиентов в окне `lessons_dedup_window_s` (дефолт 1ч), коллапсируют в один ключ - `(NULL, transient_retry, NULL)` → второй урок подавляется (потеря межзадачного сигнала). Реализация - следует букве ADR-D4, но точность дедупа на этом детекторе ниже задуманной. Рекомендация (не блокер): - передавать `work_item_id` (если доступен в `job`) либо включить `task_id` в дедуп-ключ для - `transient_retry`. Сигнал редкий (только на исчерпании бюджета), поэтому влияние ограничено. +- [ ] **Кросс-задачный дедуп `transient_retry` теряет сигнал.** Врезка в + `launcher._finalize_transient` (`src/agents/launcher.py:~1024`) передаёт `task_id`, но **не** + `work_item_id` и **не** `stage` → ключ дедупа `db.lessons_recent_dup_exists` становится + `(work_item_id IS NULL, lesson_type='transient_retry', stage IS NULL)`. В окне + `lessons_dedup_window_s` (дефолт 1ч) **разные** задачи, исчерпавшие бюджет ретраев, схлопываются в + одну запись — теряется урок про вторую задачу. Поскольку `task_id` локально доступен, дедуп-ключ + стоило бы доопределять им при `work_item_id is None` (или включать `task_id` в ключ дедупа). + Это observer/best-effort (не влияет на конвейер, AC-3 формально выполнен — 4 типа автозаписи + работают), потому не блокер, но ослабляет ценность самого сигнала, ради которого фича вводится. + Ссылка: ADR-001 D4 («ключ `work_item_id+stage+lesson_type`»). + +### P3 — Nice to have +- [ ] **Мелкая неточность ADR vs код.** `06-adr/ADR-001` (D3, таблица) и `adr-0034` указывают + choke-point `merge_hold` как `merge_gate._handle_merge_verify`, фактически `_handle_merge_verify` + живёт в `src/stage_engine.py` (туда и врезан `merge_hold`; `merge_gate.py` диффом не тронут). + Функционально корректно; рекомендуется поправить адрес в ADR для трассировки. Также + `transient_retry` в `merge_gate` (merge-retry exhausted) не реализован — но ADR формулирует это как + «**and/or** launcher», т.е. опционально; реализация через launcher достаточна. ## Документация -Обновлена в том же PR — проверено явно: -- `CLAUDE.md` — новый раздел «Машинный журнал уроков (ORCH-098)» (+38). -- `docs/architecture/README.md` — компонент **Lessons journal**, таблица `lessons` в списке схемы БД, - 3 новые строки API-таблицы (`GET /lessons`, `POST /lessons`, `POST /lessons/{id}`), `lessons` в - описании `GET /queue`. -- `CHANGELOG.md` — запись `feat` (ORCH-098) с разбивкой D1…D5 + регресс. -- ADR: локальный `06-adr/ADR-001-lessons-journal.md` (proposed) + сквозной - `docs/architecture/adr/adr-0034-lessons-journal.md` (существует). -- `README.md` «Известные ограничения» — данный PR не закрывает ни одного пункта витрины (обзорные - доки обновлять не требуется). -Документация = golden source: изменения `src/` сопровождены полным обновлением доков → P0 «src -изменён, документация не обновлена» **не применяется**. +**Обновлена полностью в том же PR — ось «документация» PASS:** +- `CLAUDE.md` — добавлен раздел «Машинный журнал уроков (ORCH-098)» (D1–D5, флаги, инвариант). +- `docs/architecture/README.md` — компонент «Lessons journal», строка таблицы `lessons` в разделе + схемы БД, три новых эндпоинта в таблице API, обновлена строка `GET /queue` (`+ lessons (ORCH-098)`). +- `docs/architecture/adr/adr-0034-lessons-journal.md` — сквозной ADR (новый). +- `docs/work-items/ORCH-098/06-adr/ADR-001-lessons-journal.md` — локальный ADR (присутствует). +- `CHANGELOG.md` — запись `[Unreleased]` с разбивкой D1–D5 + регресс. +- `README.md` «Известные ограничения» — пунктов, закрываемых этой задачей, нет (ORCH-079 N/A). + +Изменение `src/` ⇒ требование «документация = golden source» выполнено; основание для +`REQUEST_CHANGES` по оси документации отсутствует.