From c91eb7f82befe2a0a2d85897c1d826c5922da88d Mon Sep 17 00:00:00 2001 From: claude-bot Date: Fri, 5 Jun 2026 18:40:52 +0000 Subject: [PATCH] reviewer(ET): auto-commit from reviewer run_id=123 --- docs/work-items/ORCH-017/12-review.md | 135 +++++++++++++++----------- 1 file changed, 77 insertions(+), 58 deletions(-) diff --git a/docs/work-items/ORCH-017/12-review.md b/docs/work-items/ORCH-017/12-review.md index 5b6c533..d29f834 100644 --- a/docs/work-items/ORCH-017/12-review.md +++ b/docs/work-items/ORCH-017/12-review.md @@ -1,81 +1,100 @@ --- type: review work_item_id: ORCH-017 -verdict: APPROVED -version: 2 +verdict: REQUEST_CHANGES +version: 3 --- # Review ORCH-017 ## Summary -PR добавляет в пингующее уведомление об апруве BRD (`notify_approve_requested`) -две кликабельные HTML-``-ссылки — на `01-brd.md` в Gitea (branch-view) и на -issue в Plane. Реализация затрагивает только `src/config.py` и -`src/notifications.py`. Независимая проверка подтверждает соответствие ТЗ -(`02-trz.md`), всем 10 критериям приёмки (`03-acceptance-criteria.md`) и -принятому ADR-001. Полный прогон `pytest tests/ -q` → **434 passed**, целевые -тесты (`test_notify_approve_links.py`, `test_analysis_approve_flow_links.py`, -`test_telegram_tracker.py`, `test_notify_done_regression.py`) → 43 passed. -Документация обновлена в том же PR. Блокеров и must-fix findings нет. +Основная фича (прямые BRD-/Plane-ссылки в `notify_approve_requested`) реализована +качественно и соответствует ТЗ, ADR-001 и всем критериям приёмки — это было +независимо подтверждено в review v2 (изменения только в `src/config.py` и +`src/notifications.py`). + +Однако **после** review v2 в этот же work item внесён коммит `e62d51a` +`fix(qg): testing gate reads documented tester result: frontmatter key (ORCH-017)`, +который меняет `src/qg/checks.py` — функцию `_parse_tests_verdict` **разделяемого** +гейта `check_tests_passed`. Это правка машинного вердикта / управления конвейером, +затрагивающая ВСЕ проекты из общего прод-инстанса (self-hosting blast radius), и она +**прямо противоречит** собственным управляющим документам задачи (ТЗ §7 и ADR-001 Р-5), +которые фиксируют, что QG_CHECKS и машинные вердикты НЕ меняются. Изменение не +оформлено отдельным ADR и не проведено через возврат на Анализ. → REQUEST_CHANGES. ## Соответствие ТЗ -- §3.1 — BRD- и Plane-ссылки строятся независимо (`_build_brd_link`, - `_build_plane_issue_link`), встроены в текст того же одного сообщения; призыв - «Переведите задачу в статус Approved …» сохранён; `html.escape` на динамике. ✔ -- §3.1 порядок действий — `mark_brd_review_started → update_task_tracker → - send_telegram(msg)` (пингующий, не silent) сохранён, проверено по коду. ✔ -- §3.2 — `Settings.plane_web_url` (env `ORCH_PLANE_WEB_URL`); фолбэк - `plane_web_url → plane_api_url`. ✔ -- §4/§5/§7 — API, схема БД и реестр `QG_CHECKS` не затронуты (подтверждено - diff: в `src/` только `config.py` и `notifications.py`). ✔ +- §3.1–§3.2, §4–§6 (фича уведомления) — выполнено. BRD-/Plane-ссылки строятся + независимо (`_build_brd_link`, `_build_plane_issue_link`), встроены в текст + одного сообщения; призыв «Переведите задачу в статус Approved …» сохранён; + `html.escape` на динамике; порядок `mark_brd_review_started → update_task_tracker + → send_telegram(msg)` соблюдён; `Settings.plane_web_url` + фолбэк добавлены. ✔ +- **§7 — НАРУШЕНО.** ТЗ §7 дословно: «Требования к новым QG checks: Нет. Реестр + `QG_CHECKS`, стадии и **машинные вердикты не меняются**». Коммит `e62d51a` + меняет машинный вердикт гейта `check_tests_passed` (читает теперь `result:`). + По канону (CLAUDE.md правило 4) при неполноте ТЗ — возврат на Анализ, а не + тихое расширение объёма. ✘ -## Соответствие ADR-001 -- Р-1 — HTML-ссылки в тексте; сигнатура `send_telegram` НЕ изменена - (inline-кнопки отклонены) → минимальный blast radius, безопасно для - self-hosting. ✔ -- Р-2 — полный путь Plane по uuid - `{web_base}/{workspace}/projects/{project_id}/issues/{plane_issue_id}/`. ✔ -- Р-3 — `ORCH_PLANE_WEB_URL` + loopback-guard (`_is_loopback_base`: - `localhost/127.0.0.1/0.0.0.0/::1`, пустой/битый URL = unusable → ссылка - опускается). ✔ -- Р-4/Р-5 — graceful degradation как контракт построения ссылок (SELECT в - try/except, каждая ссылка независима); инвариант «одно сообщение, без дублей» - соблюдён. ✔ +## Соответствие ADR +- ADR-001 (Р-1…Р-4) по фиче уведомления — соблюдён. ✔ +- **ADR-001 Р-5 — НАРУШЕНО.** Р-5 фиксирует: «Реестр `QG_CHECKS`, стадии, + `:approved:`-handler, `check_analysis_approved` — без изменений (правка — + отображение, **не управление конвейером**)». Коммит `e62d51a` меняет именно + управление (логику прохождения testing-гейта). ADR-001 теперь содержит + утверждение, противоречащее коду того же PR → golden-source конфликт. ✘ +- **Нет ADR на изменение семантики гейта.** Правка разделяемого machine-verdict + гейта, влияющая на все проекты, — архитектурное решение и требует ADR + (CLAUDE.md правило 2). Заведён только ADR-001 (про ссылки); ADR на правку + `check_tests_passed` отсутствует (`docs/architecture/adr/` и `06-adr/` его не + содержат). README/CHANGELOG-строк недостаточно — они не снимают противоречие + с ADR-001 Р-5. ✘ ## Качество кода -- Вспомогательные функции снабжены содержательными docstrings со ссылками на - AC/ADR. -- Все обращения к БД и `get_project_by_repo` обёрнуты в try/except с - логированием; поток уведомлений не падает при нехватке данных (AC-6). -- Тесты содержательные, замаплены на TC из `04-test-plan.yaml`, изолируют сеть - и БД. -- Проверено наличие зависимостей: `import html`, `_get_settings`, поля Settings - `gitea_public_url/gitea_owner/gitea_url`, `plane_workspace_slug/plane_web_url/ - plane_api_url`. +Сама правка `_parse_tests_verdict` технически корректна и защитна: `result:` +добавлен как равноправное поле наряду с `verdict:`/`status:`, отрицательный токен +в любом поле остаётся авторитетным (ET-013 кейс сохранён), добавлены адресные +тесты (`test_result_pass_only_passes`, `test_result_fail_only_fails`). Претензий к +самой реализации нет — проблема не в коде, а в нарушении объёма/ADR и self-hosting +governance: изменение разделяемой прод-инфраструктуры протащено внутрь задачи про +текст уведомления вместо отдельного work item с собственным ADR. + +Фича уведомления (`notifications.py`/`config.py`) — без замечаний (см. review v2). ## Findings ### P0 — Blocker -- нет +- [ ] **Нарушен ADR-001 Р-5 и ТЗ §7.** Коммит `e62d51a` меняет машинный вердикт + разделяемого гейта `check_tests_passed` (`src/qg/checks.py::_parse_tests_verdict`), + тогда как ADR-001 Р-5 и ТЗ §7 явно фиксируют неизменность `QG_CHECKS` и + машинных вердиктов («правка — отображение, не управление конвейером»). Код + PR противоречит собственным управляющим артефактам задачи. + **Резолюция (одна из):** + (а) вынести правку гейта в отдельный work item со своим ADR (канон: + ТЗ неполно → возврат на Анализ, не scope-creep); либо + (б) при явном одобрении Owner на сохранение правки в ORCH-017 — привести + ADR-001/ТЗ §7 в соответствие с кодом и завести отдельный ADR, + документирующий изменение семантики разделяемого testing-гейта и его + self-hosting blast radius. ### P1 — Must fix -- нет +- [ ] **Нет ADR на архитектурное изменение разделяемого гейта** (CLAUDE.md правило 2). + Изменение чтения машинного вердикта `check_tests_passed` влияет на все проекты + общего прод-инстанса (enduro-trails и др.) — это архитектурное решение, + требующее ADR. Сейчас задокументировано лишь строкой README + CHANGELOG + + комментарием в коде; ADR отсутствует, а ADR-001 утверждает обратное. ### P2 — Should fix -- нет - -### P3 — Nice-to-have -- `docs/architecture/README.md` не обновлён, и это корректно: API, стадии, - реестр QG и компоненты не менялись; trigger-файлы из футера README - (`stages.py`, `qg/checks.py`, `main.py`) не затронуты. Env-карта обновлена в - `docs/operations/INFRA.md`. Замечание чисто информационное. +- [ ] `13-test-report.md` устарел относительно правки гейта: отчёт покрывает только + нотификационные TC и фиксирует «434 passed» из прогона, предшествующего + `e62d51a`; новые кейсы `tests/test_qg.py` и сама правка гейта в отчёте не + отражены. (Канонический ре-тест — на стадии testing после устранения P0/P1.) ## Документация -Правило «изменён `src/` → обновлена документация в том же PR» выполнено: -- `CHANGELOG.md` — запись о фиче (Unreleased/Added). ✔ -- `.env.example` — новая `ORCH_PLANE_WEB_URL` с описанием фолбэка и - loopback-семантики. ✔ -- `docs/operations/INFRA.md` — env-карта дополнена `ORCH_PLANE_WEB_URL`. ✔ -- `src/config.py` — поле документировано inline-комментарием. ✔ -- ADR `docs/work-items/ORCH-017/06-adr/ADR-001-telegram-approve-links.md` — - фиксирует выбор формата (HTML vs кнопки) и формат Plane-URL. ✔ +Правило «изменён `src/` → обновлена документация в том же PR» по фиче уведомления — +выполнено (`CHANGELOG.md`, `.env.example`, `docs/operations/INFRA.md`, ADR-001). ✔ + +Для правки гейта документация **неконсистентна**: CHANGELOG (Fixed) и +`docs/architecture/README.md` обновлены, НО ADR-001 Р-5 и ТЗ §7 продолжают +утверждать, что QG/машинные вердикты не меняются — прямое противоречие коду того же +PR. Требуется: либо вынос правки в отдельный work item с собственным ADR, либо +(при одобрении Owner) приведение ADR-001/ТЗ §7 в соответствие и отдельный ADR на +изменение семантики разделяемого testing-гейта.