diff --git a/docs/work-items/ORCH-098/12-review.md b/docs/work-items/ORCH-098/12-review.md new file mode 100644 index 0000000..0560cb4 --- /dev/null +++ b/docs/work-items/ORCH-098/12-review.md @@ -0,0 +1,92 @@ +--- +verdict: APPROVED +work_item: ORCH-098 +stage: review +author_agent: reviewer +status: approved +created_at: 2026-06-10 +model_used: claude-opus-4-8 +type: review +work_item_id: ORCH-098 +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. + +## Соответствие 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'ах, существующая логика не переписана). Слом инвариантов отсутствует. + +## Качество кода (ось 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 + +### 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`. Сигнал редкий (только на исчерпании бюджета), поэтому влияние ограничено. + +## Документация +Обновлена в том же 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-0033-lessons-journal.md` (существует). +- `README.md` «Известные ограничения» — данный PR не закрывает ни одного пункта витрины (обзорные + доки обновлять не требуется). + +Документация = golden source: изменения `src/` сопровождены полным обновлением доков → P0 «src +изменён, документация не обновлена» **не применяется**.