reviewer(ET): auto-commit from reviewer run_id=581
This commit is contained in:
@@ -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` по оси документации отсутствует.
|
||||
|
||||
Reference in New Issue
Block a user