reviewer(ET): auto-commit from reviewer run_id=208
All checks were successful
CI / test (push) Successful in 18s
All checks were successful
CI / test (push) Successful in 18s
This commit is contained in:
@@ -2,36 +2,52 @@
|
||||
type: review
|
||||
work_item_id: ORCH-036
|
||||
verdict: APPROVED
|
||||
version: 2
|
||||
version: 3
|
||||
---
|
||||
|
||||
# Review ORCH-036 — Исполняемый самодеплой стадии `deploy` (Вариант B)
|
||||
|
||||
## Summary
|
||||
|
||||
Re-review после фикса двух P1 из версии 1. Оба блокера устранены:
|
||||
Полное ревью реализации после слияния ветки с `origin/main` (merge-commit `36c1898`;
|
||||
параллельно в `main` приехал ORCH-053 reconciler). Реализация трёхфазного исполняемого
|
||||
самодеплоя соответствует ТЗ (`02-trz.md`), критериям приёмки (`03-acceptance-criteria.md`
|
||||
AC-1…AC-13) и ADR-001. Блокеров нет.
|
||||
|
||||
1. **Stale deploy-state маркеры** — добавлен `self_deploy.clear_state(repo, work_item_id)`
|
||||
(never-raise, idempotent, рекурсивное удаление `<repos_dir>/.deploy-state-<repo>/<wi>/`)
|
||||
в ветке БАГ-8-отката `check_deploy_status` FAILED (`_handle_qg_failure_rollbacks`,
|
||||
`src/stage_engine.py`) и дополнительно в начале Фазы A (`_handle_self_deploy_phase_a`)
|
||||
как belt-and-suspenders. Добавлен регрессионный тест
|
||||
`tests/test_deploy_rollback.py::test_tc11_re_deploy_after_rollback_not_wedged`,
|
||||
доказывающий, что после FAILED → откат → фикс → повторный заход на `deploy` Фаза B
|
||||
РЕАЛЬНО инициирует деплой (нет no-op по устаревшему `initiated`), плюс
|
||||
`tests/test_deploy_hook_mapping.py::test_clear_state_removes_all_markers_and_is_idempotent`.
|
||||
2. **`.env.example`** — добавлен полный блок дескрипторов `ORCH_SELF_DEPLOY_*` /
|
||||
`ORCH_DEPLOY_*` (14 настроек, плейсхолдеры, секреты не коммитятся) по образцу
|
||||
merge-gate ORCH-043, с подробными комментариями.
|
||||
Проверено по четырём осям:
|
||||
|
||||
Реализация трёхфазного исполняемого самодеплоя соответствует ADR-001 и закрывает
|
||||
критерии приёмки AC-1…AC-13. Контракты `STAGE_TRANSITIONS` / `QG_CHECKS` /
|
||||
`_parse_deploy_status` / БАГ-8 / terminal-sync / merge-gate (ORCH-43) НЕ тронуты;
|
||||
условность по репо (`self_deploy_applies`) корректна; перехваты упорядочены верно
|
||||
(Phase B после terminal-check, Phase A после merge-gate); `deploy-finalizer` —
|
||||
детерминированный no-LLM reserved-agent, перехвачен в launcher до `_spawn`. Все
|
||||
импорты (`set_issue_in_review`, `plane_add_comment`, `set_issue_blocked`,
|
||||
`send_telegram`) присутствуют. `pytest tests/` — **568 passed**.
|
||||
**1. Соответствие ТЗ.** Все требования §2.1–§2.7 закрыты: build-once `SOURCE_IMAGE`-retag
|
||||
в хуке (§2.1, обратно совместимо, exit-code-контракт цел); approve-гейт по флагу
|
||||
`deploy_require_manual_approve=true` (§2.2); detached ssh+setsid-триггер из стадии `deploy`
|
||||
(§2.3, агент себя не рестартит — BR-2); маппинг exit→`deploy_status` чистой функцией
|
||||
`map_exit_code_to_status` (§2.4); Plane+Telegram на approve-request/initiate/success/rollback
|
||||
(§2.5, BR-5); конфиг + `.env.example`-дескрипторы (§2.6); документация (§2.7).
|
||||
|
||||
**2. Соответствие ADR.** Дизайн A/B/C реализован дословно: Фаза A перехватывает на ребре
|
||||
`deploy-staging→deploy` ПОСЛЕ `check_staging_status` и merge-gate (порядок в `advance_stage`
|
||||
верный — проверено строки 203-288); Фаза B — дискриминатор `finished_agent is None` +
|
||||
идемпотентность по маркеру `initiated`; Фаза C — reserved-agent `deploy-finalizer`,
|
||||
перехвачен в `launcher.launch_job` ДО `_spawn` (R-6), drive существующих контрактов через
|
||||
`advance_stage(..., finished_agent="deployer")`. Глобальные ADR не нарушены.
|
||||
**Контракты НЕ тронуты (AC-10):** `STAGE_TRANSITIONS`, `QG_CHECKS`, `check_deploy_status`/
|
||||
`_parse_deploy_status` (frontmatter-only), terminal-sync `deploy→done`, merge-gate (ORCH-43),
|
||||
откат БАГ-8.
|
||||
|
||||
**3. Качество кода.** `src/self_deploy.py` — чистый leaf-модуль, контракт never-raise на
|
||||
каждом публичном хелпере; sentinel-state restart-safe без миграции БД; `clear_state` в
|
||||
БАГ-8-откате + belt-and-suspenders в Фазе A снимает класс «stale `initiated` → wedge при
|
||||
повторном заходе на deploy» (AC-4). Условность `self_deploy_applies` зеркалит ORCH-35/43
|
||||
(kill-switch + CSV + self-hosting fallback). Интеграционные точки сверены:
|
||||
`enqueue_job(available_at_delay_s=, task_id=)`, `mark_job`, `get_worktree_path`,
|
||||
`repos_dir`/`host_repos_dir`, импорты `set_issue_in_review`/`set_issue_blocked`/
|
||||
`plane_add_comment`/`send_telegram` — все присутствуют. Defer-бюджет finalizer считается
|
||||
из `jobs` по `task_content LIKE '%deploy-finalize defer%'` (restart-safe, начальный
|
||||
poll-job не засчитывается — корректно). Fail-closed маппинг (любой не-0 / мусор → FAILED).
|
||||
|
||||
**4. Качество тестов.** `pytest tests/` — **596 passed**. Покрытие содержательное:
|
||||
exit-mapping (TC-01/02/03), approve-гейт, routing self/non-self, rollback +
|
||||
re-deploy-after-rollback, notifications, build-once, terminal-sync, staging-precondition,
|
||||
hook-rollback-sim.
|
||||
|
||||
## Findings
|
||||
|
||||
@@ -39,26 +55,40 @@ Re-review после фикса двух P1 из версии 1. Оба блок
|
||||
- (нет)
|
||||
|
||||
### P1 — Must fix
|
||||
- (нет — оба P1 из версии 1 устранены и покрыты тестами)
|
||||
- (нет)
|
||||
|
||||
### P2 — Should fix
|
||||
- (нет блокирующих; прежний P2 про сквозную процедуру оператора частично закрыт:
|
||||
env-карта новых настроек добавлена в INFRA.md, пошаговый approve→deploy описан в
|
||||
deployer.md и DEPLOY_HOOK.md)
|
||||
- [ ] **Коллизия номера глобального ADR.** В `docs/architecture/adr/` сосуществуют
|
||||
`adr-0007-executable-self-deploy.md` (ORCH-036) и `adr-0007-reconciler.md` (ORCH-053) —
|
||||
два разных решения под одним номером 0007 (артефакт параллельного слияния ORCH-053 в
|
||||
`main`). Канон CLAUDE.md требует уникальной нумерации `adr-NNNN-slug.md`. Рекомендация:
|
||||
перенумеровать одно из решений (напр. self-deploy → `adr-0008`) и поправить ссылки в
|
||||
`docs/architecture/README.md`. Не блокер: на функционал/контракты не влияет.
|
||||
- [ ] **Дублирующийся футер `README.md`.** `docs/architecture/README.md` строки 169-170 —
|
||||
две почти идентичные строки «*Актуально на 2026-06-06…*» (merge-артефакт ORCH-036↔ORCH-053).
|
||||
Оставить одну консолидированную строку.
|
||||
|
||||
### P3 — Nice to have
|
||||
- (нет)
|
||||
|
||||
## Документация
|
||||
|
||||
Обновлена содержательно и в том же PR:
|
||||
- `.openclaw/agents/deployer.md` — стадия `deploy` переписана: self-hosting путь
|
||||
(Фазы A/B/C, явный запрет рестарта 8500 изнутри агента) vs прежний синхронный
|
||||
ssh-путь для не-self репо;
|
||||
- `docs/operations/INFRA.md` — env-карта всех новых `ORCH_SELF_DEPLOY_*` / `ORCH_DEPLOY_*`;
|
||||
Обновлена содержательно и в том же PR (ось документации — **PASS**):
|
||||
- `.openclaw/agents/deployer.md` — стадия `deploy` переписана: self-hosting путь (Фазы A/B/C,
|
||||
явный запрет рестарта 8500 изнутри агента) vs прежний синхронный ssh-путь для не-self репо;
|
||||
- `docs/architecture/README.md` — новый раздел «Исполняемый самодеплой стадии `deploy` (ORCH-36)»;
|
||||
- `docs/operations/INFRA.md` — env-карта новых `ORCH_SELF_DEPLOY_*` / `ORCH_DEPLOY_*`;
|
||||
- `docs/operations/DEPLOY_HOOK.md` — `SOURCE_IMAGE` build-once + прод-пример;
|
||||
- `docs/architecture/README.md` — раздел «Исполняемый самодеплой стадии `deploy`»;
|
||||
- `CHANGELOG.md` — запись Added (фича) + запись Fixed (review-fix: clear_state + .env.example);
|
||||
- `CHANGELOG.md` — запись Added (ORCH-036);
|
||||
- ADR `docs/work-items/ORCH-036/06-adr/ADR-001-executable-self-deploy.md` + глобальный
|
||||
`docs/architecture/adr/adr-0007-executable-self-deploy.md`;
|
||||
- **`.env.example`** — канонический шаблон (CLAUDE.md №8, ТЗ §2.6) дополнен (был пробел в v1).
|
||||
`docs/architecture/adr/adr-0007-executable-self-deploy.md` (см. P2 о номере);
|
||||
- `.env.example` — блок дескрипторов настроек (секреты не коммитятся, канон CLAUDE.md №8).
|
||||
|
||||
Документация = golden source: изменения `src/` сопровождены синхронным обновлением
|
||||
доки в том же PR. Ось документации — PASS.
|
||||
`src/` сопровождён синхронным обновлением документации в том же PR. Найденные P2 — это
|
||||
консистентность нумерации/футера, возникшая из-за параллельного слияния, а не отсутствие
|
||||
документации.
|
||||
|
||||
## Вердикт
|
||||
|
||||
Нет P0/P1 → **APPROVED**. P2 (коллизия номера ADR, дубль футера README) рекомендуется
|
||||
устранить отдельным docs-PR; конвейер не блокируют.
|
||||
|
||||
Reference in New Issue
Block a user