reviewer(ET): auto-commit from reviewer run_id=790
This commit is contained in:
@@ -8,88 +8,101 @@ created_at: 2026-06-17
|
||||
model_used: claude-opus-4-8
|
||||
type: review
|
||||
work_item_id: ORCH-119
|
||||
version: 2
|
||||
version: 3
|
||||
---
|
||||
|
||||
# Review ORCH-119 — source-backed `00-business-request.md` вместо хардкода `TBD`
|
||||
|
||||
## Summary
|
||||
|
||||
Чистый, узко-скоупленный багфикс (Bug-трек). Раздел «Description» файла
|
||||
`00-business-request.md` теперь несёт **фактический текст запроса** из Plane-issue вместо
|
||||
литерала `TBD` — на **обоих** путях создания (прямой A + отложенный срез ветки B, ORCH-088,
|
||||
доминирует на self-hosting). Реализация **точно зеркалирует** прецедент `tasks.title`, как и
|
||||
Узко-скоупленный багфикс (Bug-трек). Раздел «Description» файла `00-business-request.md`
|
||||
теперь несёт **фактический текст запроса** из Plane-issue вместо литерала `TBD` — на **обоих**
|
||||
путях создания: прямой (путь A) и **отложенный срез ветки** (путь B, ORCH-088, доминирует на
|
||||
self-hosting `orchestrator`). Реализация **точно зеркалирует** прецедент `tasks.title`, как и
|
||||
предписал ADR-001: аддитивная колонка `tasks.description` через `_ensure_column`, записываемая
|
||||
**внутри того же атомарного INSERT** `create_task_atomic` (race-safe vs ORCH-053), читаемая в
|
||||
`_spawn` → `_materialize_deferred_branch` на момент claim (без сети в горячем пути, NFR-4).
|
||||
Тело рендерится чистым unit-тестируемым хелпером `_render_business_request` с fail-safe
|
||||
`_spawn` → `_materialize_deferred_branch` на момент claim (без сети в горячем пути, NFR-4). Тело
|
||||
рендерится чистым unit-тестируемым хелпером `_render_business_request` с fail-safe
|
||||
fallback-маркером.
|
||||
|
||||
**Это повторный review (v2).** v1 выдал APPROVED с единственным P2: полный прогон был красным
|
||||
из-за **вне-скоупного flake чужой смерженной задачи** `test_orch123_staging_runner_exec.py::test_r2_…`
|
||||
(переиспользовал смерженный в `main` id `ORCH-123`; staging-гейт через `origin/main`-fallback
|
||||
находил реальный `staging_status: SUCCESS`-лог ORCH-123 и делал намеренно-красный гейт зелёным,
|
||||
вызывая `advance`). Developer закрыл это **тест-гермеичностью** (commit `26d6f31`: пин `repos_dir`
|
||||
в изолированный пустой tmp-каталог в autouse-фикстуре `fresh_db`, зеркало уже пиннимого
|
||||
`worktrees_dir`). **Прежний P2 устранён** — повторно проверено локально: red-ранее тест
|
||||
`test_r2_held_deploy_staging_not_rolled_back` теперь зелёный, инвариант ORCH-123 R-2 проверяем
|
||||
order-independently.
|
||||
|
||||
**Скоуп PR (против `origin/main`, merge-base `39fe1a5`):** `src/` — ровно 3 файла, предписанных
|
||||
ТЗ §2 (`webhooks/plane.py`, `agents/launcher.py`, `db.py`); тесты —
|
||||
`test_orch119_business_request.py` (новый, +263), `test_serial_gate_branch.py` (+spy-арг),
|
||||
`test_orch123_staging_runner_exec.py` (+гермеичность, +11); `CHANGELOG.md` + work-item доки.
|
||||
(Гигантский `main...HEAD`-diff — артефакт **устаревшего локального** `main`-рефа `64ba121`;
|
||||
истинный скоуп считается против `origin/main`.)
|
||||
**Это независимый повторный review (v3).** Все 4 оси перепроверены лично против `origin/main`
|
||||
(`f50f61c`, = merge-base): скоуп `src/` — ровно 3 файла, предписанных ТЗ §2 (`webhooks/plane.py`,
|
||||
`agents/launcher.py`, `db.py`); тесты — `test_orch119_business_request.py` (новый, +263),
|
||||
`test_serial_gate_branch.py` (+spy-арг); `CHANGELOG.md` + work-item доки. **Полный прогон
|
||||
`pytest tests/ -q` — зелёный (2215 passed, изолированно от конкурентных сессий).**
|
||||
|
||||
## Соответствие ТЗ / AC (ось 1) — PASS
|
||||
|
||||
| AC / FR | Статус | Подтверждение |
|
||||
|---------|--------|---------------|
|
||||
| AC-1 / FR-1 — реальное описание вместо `TBD`, header/`Work Item ID` целы | ✅ | `_render_business_request` рендерит тело из `description`; TC-01 (genuine red→green). |
|
||||
| AC-2 / FR-2 — прямой путь A | ✅ | `start_pipeline` передаёт `description` в `_create_initial_docs` (`plane.py:716`); TC-04 (ассертит отсутствие `## Description\n\nTBD\n`). |
|
||||
| AC-3 / FR-3 — отложенный путь B / durable-персист | ✅ | персист в атомарном INSERT (`plane.py:645`); чтение `SELECT …, description` в `_spawn` (`launcher.py:570`) → 5-й арг `_materialize_deferred_branch` (`launcher.py:594`); TC-03. |
|
||||
| AC-4 / FR-4 — fail-safe fallback | ✅ | `_BUSINESS_REQUEST_FALLBACK`, `try/except` → never-raise; TC-02 (`""`/`" "`/`"\n\t"`/`None`). |
|
||||
| AC-2 / FR-2 — прямой путь A | ✅ | `start_pipeline` передаёт `description` в `_create_initial_docs` (`plane.py:736`); TC-04 ассертит отсутствие `## Description\n\nTBD\n`. |
|
||||
| AC-3 / FR-3 — отложенный путь B / durable-персист | ✅ | персист в атомарном INSERT `create_task_atomic` (`plane.py:665`, `db.py`); чтение `SELECT …, description` в `_spawn` → 5-й арг `_materialize_deferred_branch` (`_br_row[3]`); TC-03. |
|
||||
| AC-4 / FR-4 — fail-safe fallback | ✅ | `_BUSINESS_REQUEST_FALLBACK` + `try/except` → never-raise; TC-02 (`""`/`" "`/`"\n\t"`/`None`). |
|
||||
| AC-5 — гейты/схема не тронуты | ✅ | коммит не касается `stages.py`/`qg/checks.py`; единственная схема — аддитивная `tasks.description`, базовый `CREATE TABLE tasks` цел; TC-07. |
|
||||
| AC-6 — идемпотентность / BC | ✅ | Gitea 422 → no-op (тело не перезаписывается); `_ensure_column` no-op; TC-05/TC-06. |
|
||||
| AC-7 — регресс red→green + полный прогон зелёный | ✅ | TC-01 red до фикса (функция отсутствовала + `TBD` хардкод на `main`), green после; **полный прогон теперь зелёный** (прежний P2-flake устранён `26d6f31`; commit-протокол: 2173 passed). |
|
||||
| AC-6 — идемпотентность / BC | ✅ | Gitea `422` → `return` (тело не перезаписывается, `plane.py:1006`); `_ensure_column` no-op; TC-05/TC-06. |
|
||||
| AC-7 — регресс red→green + полный прогон зелёный | ✅ | На `origin/main` `_render_business_request` **отсутствует** (grep=0) и `TBD` хардкоден (`plane.py:945`) → TC-01 настоящий red; green после. Полный прогон **2215 passed** (изолированно). |
|
||||
| AC-8 — сопровождение | ✅ | `CHANGELOG.md` содержит запись ORCH-119. |
|
||||
|
||||
`description` определён в scope `start_pipeline` (`plane.py:518`, обогащается из Plane API
|
||||
`:540–545`) **до** обоих call-site (`:645`, `:716`) — NameError исключён; персистится тот же
|
||||
enriched-текст, что уходит analyst-job (`:731`) — консистентно.
|
||||
`description` определён в scope `start_pipeline` (`plane.py:538`) и **обогащается** из Plane API
|
||||
(`plane.py:560–568`) **до** обоих call-site (`:665`, `:736`) — NameError исключён; персистится тот
|
||||
же enriched-текст, что уходит analyst-job (`:751`) — консистентно.
|
||||
|
||||
## Соответствие ADR (ось 2) — PASS
|
||||
|
||||
- **D1** (аддитивная колонка в атомарном INSERT) — реализовано буквально: `_ensure_column(conn,"tasks","description","TEXT")` рядом с `tasks.title`; запись встроена в существующий INSERT `create_task_atomic`, не отдельным UPDATE → нет race-окна vs ORCH-053. ✅
|
||||
- **D2** (чистый рендер-хелпер + проброс на обоих путях) — `_render_business_request` чистый/network-free; проброс A и B. ✅
|
||||
- **D1** (аддитивная колонка в атомарном INSERT) — реализовано буквально:
|
||||
`_ensure_column(conn,"tasks","description","TEXT")` рядом с `tasks.title`; запись встроена в
|
||||
существующий INSERT `create_task_atomic`, не отдельным UPDATE → нет race-окна vs ORCH-053. ✅
|
||||
- **D2** (чистый рендер-хелпер + проброс на обоих путях) — `_render_business_request`
|
||||
чистый/network-free; проброс A и B. ✅
|
||||
- **D3** (fail-safe fallback + идемпотентность) — маркер + never-raise + 422-no-op. ✅
|
||||
- **D4** (инварианты) — ORCH-088 момент/условие отложенного среза не меняются (дополняется только источник данных), ORCH-053 семантика `(row, created)` цела, гейты байт-в-байт, без kill-switch (обосновано — фикс дефекта). ✅
|
||||
- **Трассировка (TRACEABILITY):** правки блоков с маркерами **ORCH-088** (`_materialize_deferred_branch`) и **ORCH-053** (`create_task_atomic`) сверены — аддитивные, с дефолтами; зафиксированные инварианты не сломаны; комментарии кода корректно ссылаются на оба ADR. ✅
|
||||
- **D4** (инварианты) — ORCH-088 момент/условие отложенного среза не меняются (дополняется только
|
||||
источник данных), ORCH-053 семантика `(row, created)` цела, гейты байт-в-байт, без kill-switch
|
||||
(обосновано — фикс дефекта). ✅
|
||||
- **Трассировка (TRACEABILITY):** правки блоков с маркерами **ORCH-088**
|
||||
(`_materialize_deferred_branch`) и **ORCH-053** (`create_task_atomic`) сверены с их `06-adr` —
|
||||
аддитивные, все новые параметры с дефолтами; зафиксированные инварианты не сломаны; комментарии
|
||||
кода корректно ссылаются на оба ADR. ✅
|
||||
- Сквозной (global) ADR не требуется — локальное аддитивное решение по прецеденту. ✅
|
||||
|
||||
## Качество кода (ось 3) — PASS
|
||||
|
||||
- Чистый, network-free, unit-тестируемый хелпер; docstrings на всех новых/изменённых функциях. ✅
|
||||
- never-raise контракт соблюдён (`try/except` в рендере; создание задачи не падает). ✅
|
||||
- BC: все добавленные параметры аддитивны с дефолтами; единственный прочий потребитель сигнатуры (serial-gate spy в `test_serial_gate_branch.py`) синхронно обновлён. ✅
|
||||
- **Багфикс-регресс (ORCH-019 / BR-4) — фиксатор присутствует:** TC-01 настоящий red до фикса (`_render_business_request` отсутствовал + `_create_initial_docs` хардкодил `TBD` на `main`), green после; усилен TC-04 (бьёт реальный баг-путь `_create_initial_docs`). ✅
|
||||
- Тесты содержательные: 7 TC покрывают регресс, fallback (4 параметра), оба пути, schema-BC, идемпотентность 422, multiline/спецсимволы verbatim, анти-регресс гейтов. ✅
|
||||
- Гермеичность-фикс `test_orch123` (`26d6f31`) — test-only, продкод/семантика гейтов не тронуты; делает чужой тест order-independent.
|
||||
- **Локальная проверка:** `tests/test_orch119_business_request.py` — **10 passed**; ранее красный `test_orch123…::test_r2_held_deploy_staging_not_rolled_back` — **1 passed**. ✅
|
||||
- BC: все добавленные параметры аддитивны с дефолтами; единственный прочий потребитель сигнатуры
|
||||
(`_docs_spy` в `test_serial_gate_branch.py`) синхронно обновлён под аддитивный `description=None`. ✅
|
||||
- **Багфикс-регресс (ORCH-019 / BR-4) — фиксатор присутствует:** TC-01 настоящий red до фикса
|
||||
(`_render_business_request` отсутствовал + `_create_initial_docs` хардкодил `TBD` на `main`),
|
||||
green после; усилен TC-04 (бьёт реальный баг-путь `_create_initial_docs`). ✅
|
||||
- Тесты содержательные: 7 TC покрывают регресс, fallback (4 параметра), оба пути, schema-BC,
|
||||
идемпотентность 422, multiline/спецсимволы verbatim, анти-регресс гейтов. ✅
|
||||
- **Личная проверка:** `test_orch119_business_request.py` + `test_serial_gate_branch.py` — 13 passed;
|
||||
`test_stage_engine.py` — 52 passed (изолированно); полный прогон — 2215 passed. ✅
|
||||
|
||||
## Документация (ось 4) — PASS
|
||||
|
||||
Изменён `src/` → проверка обязательна. Документация обновлена в том же PR:
|
||||
- **`CHANGELOG.md`** — подробная запись ORCH-119 (AC-8). ✅
|
||||
- **ADR-001** (`06-adr/ADR-001-source-backed-business-request-doc.md`) + **`08-data-requirements.md`** (документирует схему `tasks.description`) — присутствуют, исчерпывающи. ✅
|
||||
- **API** — изменений нет (ТЗ §4) → таблица API в `docs/architecture/README.md` обновления не требует. ✅
|
||||
- **Стадии/QG** — не тронуты → `docs/architecture/README.md` / `internals.md` обновления не требуют. ✅
|
||||
- **Витрина `docs/overview/` (ORCH-011):** `tech-agents.md` по-прежнему верно указывает `00-business-request.md` как вход аналитика; `tech-data-model.md` описывает `tasks` концептуально (без перечня колонок) — не устаревает добавлением колонки; смена **тела** артефакта (внутренняя деталь генерации) описанных стадий/гейтов/агентов/способностей не меняет. Обновление не требуется. ✅
|
||||
- **`README.md` «Известные ограничения» (ORCH-079):** TBD-баг `00-business-request.md` **не** числится среди открытых ограничений (Telegram-48h / intra-repo deps / batch-автоном) — закрывать/снимать нечего, обновление README не требуется. ✅
|
||||
- **Центрального schema-дока колонок `tasks` нет** — конвенция проекта: per-task скалярные колонки документируются ADR задачи + data-requirements + CHANGELOG (так же оформлены `tasks.title` / `tasks.track` / `tasks.cancelled_at`). ORCH-119 ей следует. ✅
|
||||
- **ADR-001** (`06-adr/ADR-001-source-backed-business-request-doc.md`) + **`08-data-requirements.md`**
|
||||
(документирует схему `tasks.description`) — присутствуют, исчерпывающи. ✅
|
||||
- **API** — изменений нет (ТЗ §4) → таблица API в `docs/architecture/README.md` обновления не
|
||||
требует. ✅
|
||||
- **Стадии/QG** — не тронуты → `docs/architecture/README.md` / `internals.md` обновления не
|
||||
требуют. ✅
|
||||
- **Витрина `docs/overview/` (ORCH-011):** `tech-agents.md` по-прежнему верно указывает
|
||||
`00-business-request.md` как вход аналитика; `tech-data-model.md` описывает `tasks`
|
||||
концептуально (без перечня колонок) — добавление колонки его не устаревает; смена **тела**
|
||||
артефакта (внутренняя деталь генерации) описанных стадий/гейтов/агентов/способностей не меняет.
|
||||
Обновление не требуется. ✅
|
||||
- **`README.md` «Известные ограничения» (ORCH-079):** проверено — TBD-баг `00-business-request.md`
|
||||
**не** числится среди 3 открытых ограничений (Telegram-48h / intra-repo deps / batch-автоном) —
|
||||
закрывать/снимать нечего, обновление README не требуется. ✅
|
||||
- **Центрального schema-дока колонок `tasks` нет** — конвенция проекта: per-task скалярные колонки
|
||||
документируются ADR задачи + data-requirements + CHANGELOG (так же оформлены `tasks.title` /
|
||||
`tasks.track` / `tasks.cancelled_at`). ORCH-119 ей следует. ✅
|
||||
|
||||
**Итог по документации:** при изменении `src/` документация обновлена в том же PR — P0 «`src/` изменён, документация не обновлена» **не наступает**.
|
||||
**Итог по документации:** при изменении `src/` документация обновлена в том же PR — P0 «`src/`
|
||||
изменён, документация не обновлена» **не наступает**.
|
||||
|
||||
## Findings
|
||||
|
||||
@@ -100,11 +113,22 @@ enriched-текст, что уходит analyst-job (`:731`) — консист
|
||||
- Нет.
|
||||
|
||||
### P2 — Should fix
|
||||
- Нет. *(Прежний P2 v1 — красный полный прогон из-за вне-скоупного flake `test_orch123` — **устранён** в этом же PR гермеичность-фиксом `26d6f31`; полный прогон зелёный.)*
|
||||
- Нет.
|
||||
|
||||
### P3 — Nice-to-have (не блокирует)
|
||||
- [ ] `tests/test_orch119_business_request.py` мутирует `os.environ` (`ORCH_DB_PATH`/`ORCH_REPOS_DIR`) на уровне модуля — добавляет к общей import-pollution-поверхности сьюта. Соответствует существующей конвенции многих тест-файлов проекта (не новый грех), на корректность не влияет; на будущее — скоупить через фикстуру.
|
||||
- [ ] `_render_business_request`: `try/except Exception` вокруг `(description or "").strip()` — `.strip()` на строке не бросает, ветка недостижима. Намеренный never-break «belt-and-suspenders» в стиле кодбейза; оставить как есть допустимо.
|
||||
- [ ] **`CHANGELOG.md` over-claim после rebase.** Хвост записи ORCH-119 описывает «Дополнительно в
|
||||
том же PR починена тест-гермеичность `tests/test_orch123_staging_runner_exec.py`…». Этот файл
|
||||
**отсутствует** в diff против `origin/main` — фикс (`fresh_db` пинит `repos_dir`) уже в `main`
|
||||
(проверено: `git show origin/main:tests/test_orch123_staging_runner_exec.py` уже содержит пин).
|
||||
Описывает изменение вне фактического скоупа PR (артефакт rebase). Безвредно (ядро ORCH-119
|
||||
задокументировано точно), но для аккуратности — подрезать хвост. На корректность фикса не влияет.
|
||||
- [ ] `tests/test_orch119_business_request.py` мутирует `os.environ` (`ORCH_DB_PATH`/
|
||||
`ORCH_REPOS_DIR`) на уровне модуля — добавляет к общей import-pollution-поверхности сьюта.
|
||||
Соответствует существующей конвенции многих тест-файлов проекта (не новый грех), на корректность
|
||||
не влияет.
|
||||
- [ ] `_render_business_request`: `try/except Exception` вокруг `(description or "").strip()` —
|
||||
`.strip()` на строке не бросает, ветка недостижима. Намеренный never-break «belt-and-suspenders» в
|
||||
стиле кодбейза; оставить как есть допустимо.
|
||||
|
||||
## Документация
|
||||
|
||||
@@ -113,14 +137,15 @@ enriched-текст, что уходит analyst-job (`:731`) — консист
|
||||
`docs/overview/`, обзорный `README.md` («Известные ограничения»),
|
||||
`PIPELINE_DOCS.md`/`HANDOFF_PROTOCOL.md` проверены явно — не устаревают (артефакт остаётся
|
||||
информационным, продюсер/владелец/гейт-статус не изменились; TBD-баг не числился среди открытых
|
||||
ограничений). Центрального schema-дока колонок `tasks` нет — аддитивная колонка документирована
|
||||
по конвенции (ADR + data-requirements + CHANGELOG, как `tasks.title`/`track`/`cancelled_at`).
|
||||
**Необновлённой обязательной документации не найдено.**
|
||||
ограничений). Центрального schema-дока колонок `tasks` нет — аддитивная колонка документирована по
|
||||
конвенции (ADR + data-requirements + CHANGELOG, как `tasks.title`/`track`/`cancelled_at`).
|
||||
**Необновлённой обязательной документации не найдено.** (Единственный документ-нюанс — P3-over-claim
|
||||
в `CHANGELOG.md` про вне-скоупный `test_orch123`, не блокирует.)
|
||||
|
||||
## Вердикт
|
||||
|
||||
**APPROVED.** Нет P0/P1/P2. Все AC/FR/ADR удовлетворены; реализация — точное зеркало
|
||||
одобренного прецедента с нулевым касанием конвейерной машинерии и сохранёнными инвариантами
|
||||
ORCH-088/ORCH-053; обязательный регресс-тест-фиксатор присутствует (red→green). Прежний
|
||||
вне-скоупный flake (ORCH-123) устранён в этом же PR, полный прогон зелёный. Документация
|
||||
синхронна. Готово к стадии `testing`.
|
||||
**APPROVED.** Нет P0/P1/P2. Все AC/FR/ADR удовлетворены; реализация — точное зеркало одобренного
|
||||
прецедента `tasks.title` с нулевым касанием конвейерной машинерии и сохранёнными инвариантами
|
||||
ORCH-088/ORCH-053; обязательный регресс-тест-фиксатор присутствует (genuine red→green, проверено
|
||||
против `origin/main`); полный прогон зелёный (2215 passed). Документация синхронна. Готово к стадии
|
||||
`testing`.
|
||||
|
||||
Reference in New Issue
Block a user