84 lines
5.8 KiB
Markdown
84 lines
5.8 KiB
Markdown
---
|
||
type: review
|
||
work_item_id: ORCH-017
|
||
verdict: REQUEST_CHANGES
|
||
version: 4
|
||
---
|
||
|
||
# Review ORCH-017
|
||
|
||
## Summary
|
||
Основная фича (прямые BRD-/Plane-ссылки в `notify_approve_requested`) реализована
|
||
качественно и соответствует ТЗ, ADR-001 и всем критериям приёмки (подтверждено в
|
||
review v2: изменения по фиче — только `src/config.py` и `src/notifications.py`).
|
||
|
||
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 (фича уведомления) — выполнено. `_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 — соблюдено. Реестр `QG_CHECKS`, стадии и машинные вердикты в коде не меняются
|
||
(правка гейта откачена в `d615747`). ✔
|
||
|
||
## Соответствие ADR
|
||
- ADR-001 (Р-1…Р-5) — соблюдён. Ссылки HTML-`<a>` в тексте, `send_telegram` не
|
||
тронута; полный Plane-URL по uuid; `ORCH_PLANE_WEB_URL` + loopback-guard
|
||
(`_is_loopback_base`); graceful degradation; «одно сообщение, без дублей». ✔
|
||
- ADR-001 Р-5 vs код — конфликт снят откатом гейта. ✔
|
||
|
||
## Качество кода
|
||
Фича `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
|
||
- (нет)
|
||
|
||
### P1 — Must fix
|
||
- [ ] **Документация описывает откаченный код (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` (`result: PASS`) относится к прогону, включавшему
|
||
откаченную правку гейта; после устранения P1 канонический ре-тест — на
|
||
стадии testing (отчёт не должен ссылаться на снятые из PR изменения).
|
||
|
||
## Документация
|
||
Правило «изменён `src/` → обновлена документация в том же PR» по фиче уведомления —
|
||
выполнено: `CHANGELOG.md` (Added), `.env.example` (`ORCH_PLANE_WEB_URL`),
|
||
`docs/operations/INFRA.md` (env-карта), ADR-001. ✔
|
||
|
||
Неконсистентность (P1): документация про откаченную правку гейта `check_tests_passed`
|
||
осталась в `CHANGELOG.md` (Fixed) и `docs/architecture/README.md`, хотя
|
||
соответствующий код отозван (`d615747`) и перенесён в ORCH-47. Доку нужно привести в
|
||
соответствие с кодом этого PR.
|