reviewer(ET): auto-commit from reviewer run_id=366
This commit is contained in:
@@ -1,58 +1,78 @@
|
||||
---
|
||||
type: review
|
||||
work_item_id: ORCH-067
|
||||
verdict: REQUEST_CHANGES
|
||||
version: 1
|
||||
verdict: APPROVED
|
||||
version: 2
|
||||
---
|
||||
|
||||
# Review ORCH-067
|
||||
|
||||
## Summary
|
||||
|
||||
Реализация качественная и по существу соответствует ТЗ (`02-trz.md`) и ADR-001:
|
||||
оффлайн-ядро `plane_status_label` + best-effort live-overlay `_live_plane_branch_override`
|
||||
(kill-switch / TTL-кэш / короткий таймаут), единый `_plane_issue_url` с переиспользованием
|
||||
guard'ов ORCH-017, хелперы `plane_issue_link` / `link_for`, дефолт `tracker_mode → bump`,
|
||||
новые config-флаги. Контракт «never raises» выдержан, схема БД / транспорт
|
||||
`send_telegram`/`edit_telegram`/`delete_telegram` / `disable_notification` / `STAGE_TRANSITIONS`
|
||||
/ QG — не тронуты (AC-15 ✓). `pytest tests/ -q` — **907 passed** (AC-16, AC-17 ✓).
|
||||
Точки §3.3 применены корректно: `merge_gate.py`/`job_reaper.py`/`main.py`/`launcher._notify_failed`
|
||||
оставлены без ссылки осознанно — в их тексте НЕТ номера задачи (правило «только где упоминается
|
||||
work_item_id» соблюдено).
|
||||
Повторное ревью после фикса документации (коммит `7a88f39`). Реализация полностью
|
||||
соответствует ТЗ (`02-trz.md`), ADR-001 и всем acceptance criteria (`03-acceptance-criteria.md`).
|
||||
|
||||
**Блокер — неполная документация.** ТЗ §5 и AC-18 явно требуют обновления `CHANGELOG.md` и
|
||||
`CLAUDE.md` в том же PR; оба не обновлены. Это нарушает правило «документация = golden source»
|
||||
(CLAUDE.md, п.6 правил агентов) → REQUEST_CHANGES.
|
||||
**Код** (`src/notifications.py` — ядро):
|
||||
- **Req 1 (bump):** дефолт `tracker_mode` сменён `edit → bump` (`src/config.py`); логика
|
||||
`update_task_tracker`, транспорт `send/edit/delete_telegram`, `disable_notification` и
|
||||
инвариант «одна карточка на задачу» не тронуты (AC-1..AC-4, AC-15 ✓).
|
||||
- **Req 2 (статус-строка):** чистый never-raise `plane_status_label(task_row)` (offline-ядро:
|
||||
stage→статус + `⏸️ In Review` из brd-clock + `⏸️ Awaiting Deploy`, всё без сети) +
|
||||
best-effort `_live_plane_branch_override` для ветвей, неотличимых offline (Needs Input /
|
||||
Blocked / Rejected / Cancelled / Deploying / Monitoring). Kill-switch
|
||||
(`tracker_live_status`), per-issue TTL-кэш (`_LIVE_STATE_CACHE`), короткий таймаут
|
||||
(`fetch_issue_state(..., timeout=)`, дефолт 10 сохранён → нет регресса reconciler).
|
||||
Anti-false-positive guard для enduro (`_LIVE_BRANCH_BASE`: deploying/monitoring override
|
||||
только при отдельном UUID). Прецеденс In Review > overlay соблюдён. `_card_status_label`
|
||||
обёрнут в try/except → рендер никогда не падает (AC-5..AC-9 ✓).
|
||||
- **Req 3+4 (кликабельный номер):** единый `_plane_issue_url` устраняет дублирование
|
||||
резолва проекта/loopback-guard (ORCH-017); `plane_issue_link` (текст=номер) и
|
||||
`_build_plane_issue_link` (текст=«✅ Задача в Plane») оба зовут его. `link_for` fail-safe
|
||||
достаёт `repo`/`plane_issue_id` из БД. Применено в заголовке карточки и во ВСЕХ точках
|
||||
§3.3 с номером задачи (AC-10..AC-14 ✓).
|
||||
|
||||
**Точки §3.3 проверены пофайлово:** `notify_approve_requested`, `notify_error`,
|
||||
`stage_engine.py` (все alert'ы с номером), `agents/launcher.py`, `security_gate.py`,
|
||||
`reconciler.py` — номер кликабелен. `merge_gate.py`/`job_reaper.py`/`main.py` оставлены без
|
||||
ссылки **осознанно и корректно**: их тексты ссылаются на repo/job/run_id, а НЕ на
|
||||
`work_item_id` (проверено: merge_gate:432 — lease/repo, job_reaper:396 — job/agent/repo,
|
||||
main:47 — orphaned run_ids).
|
||||
|
||||
**Инварианты/нерегресс:** схема БД, `STAGE_TRANSITIONS`, QG, транспорт — не тронуты
|
||||
(AC-15 ✓). `get_db()` возвращает новое соединение на вызов, поэтому `conn.close()` в
|
||||
`link_for` корректен. `pytest tests/ -q` → **907 passed** (AC-16, AC-17 ✓).
|
||||
|
||||
**Документация (блокеры v1 закрыты):** `CHANGELOG.md`, `CLAUDE.md`, `.env.example`
|
||||
обновлены в коммите `7a88f39`; ADR-001 присутствует и полон; `README.md`/`internals.md`
|
||||
синхронизированы (AC-18 ✓).
|
||||
|
||||
## Findings
|
||||
|
||||
### P0 — Blocker
|
||||
- [ ] **`CHANGELOG.md` не обновлён.** Нет записи ORCH-067 в секции `## [Unreleased]`
|
||||
(`grep -c ORCH-067 CHANGELOG.md` → 0). Требуется ТЗ §5 и AC-18. Добавить запись о
|
||||
смене дефолта `bump`, статус-строке карточки и кликабельном номере.
|
||||
- [ ] **`CLAUDE.md` не обновлён.** ТЗ §5 (стр. 183) требует «раздел про нотификации/tracker
|
||||
(дефолт bump; статус-строка карточки; кликабельный номер в карточке и уведомлениях)».
|
||||
Файл не менялся, ORCH-067 в нём не упоминается. AC-18 → FAIL.
|
||||
- (нет)
|
||||
|
||||
### P1 — Must fix
|
||||
- [ ] **`.env.example` рассинхронизирован.** ТЗ §4 (и §1.3): «Обновить `.env.example`, если в
|
||||
нём фигурируют `ORCH_TRACKER_MODE`». Строка осталась `ORCH_TRACKER_MODE=edit`, тогда как
|
||||
дефолт сменён на `bump` — канон env вводит в заблуждение. Также отсутствуют новые флаги
|
||||
из `config.py` (`ORCH_TRACKER_LIVE_STATUS`, `ORCH_TRACKER_LIVE_STATUS_TTL_S`,
|
||||
`ORCH_TRACKER_LIVE_STATUS_TIMEOUT_S`) — желательно задокументировать там же.
|
||||
- (нет)
|
||||
|
||||
### P2 — Should fix
|
||||
- (нет)
|
||||
|
||||
### P3 — Nice to have (не блокирует)
|
||||
- [ ] Часть alert-сообщений в `stage_engine.py` (`_handle_self_deploy_phase_b`,
|
||||
`_handle_merge_verify`) встраивает «сырой» `{msg}`/`{e}`/`{reason}` рядом с новой
|
||||
`<a>`-ссылкой; под `parse_mode=HTML` редкий `<` в этих подстановках теоретически мог
|
||||
бы помешать рендеру. Это **пре-существующее поведение** (parse_mode=HTML стоял и
|
||||
раньше), не регресс данной задачи; `notify_error` свой `error` экранирует. Можно при
|
||||
случае обернуть прочие подстановки в `html.escape`.
|
||||
|
||||
## Документация
|
||||
|
||||
- `docs/architecture/README.md` — обновлён (новый компонент Notifications / Live-tracker). ✓
|
||||
- `docs/architecture/internals.md` — обновлён (§7: режимы bump/edit, строка Plane-статуса,
|
||||
кликабельный номер). Качественно и точно. ✓
|
||||
- ADR per-work-item `06-adr/ADR-001-tracker-plane-status-and-link.md` — присутствует, полный. ✓
|
||||
- `CHANGELOG.md` — **НЕ обновлён** (P0).
|
||||
- `CLAUDE.md` — **НЕ обновлён** (P0).
|
||||
- `.env.example` — **НЕ синхронизирован** с новым дефолтом и флагами (P1).
|
||||
- `docs/architecture/README.md` — обновлён (компонент Notifications / live-tracker). ✓
|
||||
- `docs/architecture/internals.md` — обновлён (§7: bump/edit, Plane-статус, кликабельный номер). ✓
|
||||
- `06-adr/ADR-001-tracker-plane-status-and-link.md` — присутствует, полный, закрывает все `[ARCH]`. ✓
|
||||
- `CHANGELOG.md` — обновлён (запись ORCH-067). ✓
|
||||
- `CLAUDE.md` — обновлён (раздел «Нотификации / Telegram live-tracker»). ✓
|
||||
- `.env.example` — синхронизирован (`ORCH_TRACKER_MODE=bump` + новые флаги live-overlay). ✓
|
||||
|
||||
После закрытия P0/P1 (CHANGELOG + CLAUDE.md + .env.example) задача готова к повторному ревью.
|
||||
Код и тесты правок не требуют.
|
||||
Документация = golden source: код и доку обновлены в одном PR. Блокеры предыдущего ревью
|
||||
(v1) закрыты. Замечаний уровня P0/P1/P2 нет → **APPROVED**.
|
||||
|
||||
Reference in New Issue
Block a user