From 9c2843116790202e197f328db34c5491d03bd618 Mon Sep 17 00:00:00 2001 From: claude-bot Date: Fri, 5 Jun 2026 19:32:33 +0000 Subject: [PATCH] reviewer(ET): auto-commit from reviewer run_id=124 --- docs/work-items/ORCH-017/12-review.md | 121 +++++++++++--------------- 1 file changed, 52 insertions(+), 69 deletions(-) diff --git a/docs/work-items/ORCH-017/12-review.md b/docs/work-items/ORCH-017/12-review.md index d29f834..fb10cc7 100644 --- a/docs/work-items/ORCH-017/12-review.md +++ b/docs/work-items/ORCH-017/12-review.md @@ -2,99 +2,82 @@ type: review work_item_id: ORCH-017 verdict: REQUEST_CHANGES -version: 3 +version: 4 --- # Review ORCH-017 ## Summary Основная фича (прямые BRD-/Plane-ссылки в `notify_approve_requested`) реализована -качественно и соответствует ТЗ, ADR-001 и всем критериям приёмки — это было -независимо подтверждено в review v2 (изменения только в `src/config.py` и -`src/notifications.py`). +качественно и соответствует ТЗ, 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. +P0 из review v3 (правка разделяемого гейта `check_tests_passed` коммитом `e62d51a`, +нарушавшая ADR-001 Р-5 и ТЗ §7) **снят**: коммит `d615747` откатил изменение +`src/qg/checks.py` (вынесено в отдельный work item ORCH-47 со своим ADR). Код гейта +теперь идентичен `main` (читает только `verdict:`/`status:`); ADR-001 Р-5 и ТЗ §7 +снова консистентны с кодом. ✔ + +Однако откат кода **не сопровождён откатом документации**: `CHANGELOG.md` и +`docs/architecture/README.md` всё ещё описывают откаченную правку гейта и ссылаются +на не существующие в этом PR тесты `tests/test_qg.py`. Это новый doc↔code конфликт +(golden source). → REQUEST_CHANGES (P1). ## Соответствие ТЗ -- §3.1–§3.2, §4–§6 (фича уведомления) — выполнено. BRD-/Plane-ссылки строятся - независимо (`_build_brd_link`, `_build_plane_issue_link`), встроены в текст - одного сообщения; призыв «Переведите задачу в статус Approved …» сохранён; +- §3.1–§3.2, §4–§6 (фича уведомления) — выполнено. `_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) при неполноте ТЗ — возврат на Анализ, а не - тихое расширение объёма. ✘ +- §7 — соблюдено. Реестр `QG_CHECKS`, стадии и машинные вердикты в коде не меняются + (правка гейта откачена в `d615747`). ✔ ## Соответствие 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. ✘ +- ADR-001 (Р-1…Р-5) — соблюдён. Ссылки HTML-`` в тексте, `send_telegram` не + тронута; полный Plane-URL по uuid; `ORCH_PLANE_WEB_URL` + loopback-guard + (`_is_loopback_base`); graceful degradation; «одно сообщение, без дублей». ✔ +- ADR-001 Р-5 vs код — конфликт снят откатом гейта. ✔ ## Качество кода -Сама правка `_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). +Фича `notifications.py`/`config.py` — без замечаний. Чтение полей задачи +(`_get_task_link_fields`) и обе сборки ссылок защищены try/except и никогда не +роняют alert (AC-6); loopback-guard корректно опускает неклика­бельный Plane-URL +(AC-2/AC-3); `html.escape(..., quote=True)` на href и `html.escape(work_item_id)` +на подписи (AC-7). Тесты `tests/test_notify_approve_links.py`, +`tests/test_analysis_approve_flow_links.py` присутствуют и содержательны. ## 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 утверждает обратное. +- [ ] **Документация описывает откаченный код (doc↔code конфликт).** После + revert-коммита `d615747` код `src/qg/checks.py` НЕ читает `result:` (только + `verdict:`/`status:`), но документация осталась от состояния `e62d51a`: + - `docs/architecture/README.md:61` утверждает, что `check_tests_passed` + читает `verdict:`/`status:`/`result:` — это ложно для текущего кода и + вводит в заблуждение по поведению разделяемого прод-гейта (self-hosting: + tester, написавший только `result: PASS`, реально провалит гейт). + - `CHANGELOG.md:24` (секция Fixed) содержит запись о правке гейта + `check_tests_passed` под тегом ORCH-017 и ссылается на отсутствующие в PR + тесты `tests/test_qg.py::TestCheckTestsPassed::test_result_pass_only_passes` + / `…::test_result_fail_only_fails`. + **Резолюция:** убрать из ORCH-017 PR обе записи (откатить README:61 к + формулировке `main` и удалить CHANGELOG-entry про гейт) — правка гейта + принадлежит ORCH-47 и должна документироваться там вместе с её кодом. ### P2 — Should fix -- [ ] `13-test-report.md` устарел относительно правки гейта: отчёт покрывает только - нотификационные TC и фиксирует «434 passed» из прогона, предшествующего - `e62d51a`; новые кейсы `tests/test_qg.py` и сама правка гейта в отчёте не - отражены. (Канонический ре-тест — на стадии testing после устранения P0/P1.) +- [ ] `13-test-report.md` (`result: PASS`) относится к прогону, включавшему + откаченную правку гейта; после устранения P1 канонический ре-тест — на + стадии testing (отчёт не должен ссылаться на снятые из PR изменения). ## Документация Правило «изменён `src/` → обновлена документация в том же PR» по фиче уведомления — -выполнено (`CHANGELOG.md`, `.env.example`, `docs/operations/INFRA.md`, ADR-001). ✔ +выполнено: `CHANGELOG.md` (Added), `.env.example` (`ORCH_PLANE_WEB_URL`), +`docs/operations/INFRA.md` (env-карта), ADR-001. ✔ -Для правки гейта документация **неконсистентна**: CHANGELOG (Fixed) и -`docs/architecture/README.md` обновлены, НО ADR-001 Р-5 и ТЗ §7 продолжают -утверждать, что QG/машинные вердикты не меняются — прямое противоречие коду того же -PR. Требуется: либо вынос правки в отдельный work item с собственным ADR, либо -(при одобрении Owner) приведение ADR-001/ТЗ §7 в соответствие и отдельный ADR на -изменение семантики разделяемого testing-гейта. +Неконсистентность (P1): документация про откаченную правку гейта `check_tests_passed` +осталась в `CHANGELOG.md` (Fixed) и `docs/architecture/README.md`, хотя +соответствующий код отозван (`d615747`) и перенесён в ORCH-47. Доку нужно привести в +соответствие с кодом этого PR.