168 lines
17 KiB
Markdown
168 lines
17 KiB
Markdown
---
|
||
verdict: APPROVED
|
||
work_item: ORCH-009
|
||
stage: review
|
||
author_agent: reviewer
|
||
status: approved
|
||
created_at: 2026-06-10
|
||
model_used: claude-fable-5
|
||
type: review
|
||
work_item_id: ORCH-009
|
||
version: 2
|
||
---
|
||
|
||
# Review ORCH-009 — Turnkey-онбординг проектов (kit + CLI + runbook) — re-review (цикл 2)
|
||
|
||
Ветка: `feature/ORCH-009-turnkey-plane` · Diff vs `origin/main`: 46 файлов, +5478/−12.
|
||
Состав: kit `onboarding/repo-skeleton/` (28 файлов), CLI `scripts/onboard_project.py` (1090 строк),
|
||
runbook `docs/operations/ONBOARDING.md`, 3 онбординг-тест-модуля (83 теста), golden-source доки,
|
||
ADR×2 + индекс.
|
||
|
||
**Контекст цикла:** review v1 (`APPROVED`, 3×P2/2×P3) → testing `PASS` → re-test merge-gate упал на
|
||
**средовых** не-герметичных тестах ORCH-41-эры (прод-env `ORCH_AGENT_FALLBACK_MODEL`/
|
||
`ORCH_AGENT_MODEL_DEFAULT`) → откат на development → фикс `e903818` (герметизация
|
||
`tests/test_resolve_agent_{model,effort}.py`) + регенерация `17-security-report.md` (`b26a391`).
|
||
Этот review: независимая проверка дельты цикла + выборочная верификация клеймов v1.
|
||
|
||
## Summary
|
||
|
||
**APPROVED.** P0/P1 нет. Дельта цикла (фикс герметичности тестов) корректна, трассирована к
|
||
ORCH-074 ADR-001 Решение 3 и сохраняет его инвариант; полный регресс теперь зелёный **под
|
||
фактическим прод-env** (перепроверено мной: 1713 passed, exit 0 — ровно та среда, что валила
|
||
merge-gate до фикса). Клеймы review v1 выборочно перепроверены и подтверждены. Переносятся 3×P2
|
||
(харднинг краевых путей CLI, не фикшены — легитимно, follow-up) + 3×P3. Документация обновлена в
|
||
том же PR по всем точкам, включая отдельную CHANGELOG-запись про сам фикс тестов.
|
||
|
||
## Оси проверки
|
||
|
||
### Ось 1 — Соответствие ТЗ (`02-trz.md`, `03-acceptance-criteria.md`) — ✅
|
||
|
||
| Требование | Статус | Чем подтверждено |
|
||
|---|---|---|
|
||
| FR-1 состав kit (19 элементов, анти-форк канона) | ✅ | TC-01/02 зелёные; `docs/_templates|_standards` в kit не хранятся — live-copy в `materialize_kit` (`LIVE_COPY_DIRS`, прочитан код) |
|
||
| FR-2 канон 52d/92 промптов (5 секций, ❌→✅, `<escalation>`, 52c-схема, verdict-ключи байт-в-байт, плейсхолдерные даты/модели) | ✅ | TC-03…06 зелёные; verdict-ключи в kit-промптах сверены grep'ом (`verdict:`/`staging_status:`/`deploy_status:`/`security_status:` на месте) |
|
||
| FR-2 reviewer-gate доки (AC-3) | ✅ | kit `reviewer.md:65`: «документация НЕ обновлена → вердикт ОБЯЗАТЕЛЬНО `REQUEST_CHANGES`» — прочитано лично |
|
||
| FR-3 INFRA.md шаблон | ✅ | TC-19 зелёный (топология/env/границы/риски общего хоста); INFRA орка не тронут (diff пуст) |
|
||
| FR-4 CLI plan/apply/verify | ✅ | Код прочитан полностью: `plan` GET-only (рендер in-memory), `apply` идемпотентный ensure без delete, 22 статуса из `_PLANE_NAME_TO_KEY` + `STATE_GROUPS` 1:1 c ADR D5, CE-отказ → `ManualStep` → `manual-step` (fail-safe); TC-13…18 зелёные |
|
||
| FR-5 verify | ✅ | round-trip фактическим парсером, резолв всех 22 имён, лейблы, webhook active, полнота kit (`VERIFY_KIT_FILES`), скан `{{…}}`, канон ≥16/≥3 |
|
||
| FR-6 runbook | ✅ | `ONBOARDING.md` прочитан: слои 0→6 в порядке BR-1, каждый 🖐-шаг с командой проверки, self-hosting-предупреждение «групповое окно» (§4.2), workspace-webhook — «существует, только проверка» (§1.5), откат §6 |
|
||
| §4/§5 нет API/БД изменений | ✅ | diff `src/**` пуст (проверено лично) |
|
||
| §9 инварианты | ✅ | `git diff origin/main...HEAD -- src/ .openclaw/ docs/_templates/ docs/_standards/ docs/operations/INFRA.md requirements.txt` — **пусто**; сетевых вызовов в тестах нет (фейк-клиенты); новых pip-зависимостей нет |
|
||
| AC-12 полный регресс | ✅ | **1713 passed, 0 failed, exit 0** — мой прогон в worktree ветки под фактическим хост-env (до фикса здесь было 2 failed) |
|
||
| AC-13 операторский smoke | ⏳ | По построению операторский (ADR D8); «Журнал smoke-прогонов» — плейсхолдер. Обязателен ДО `Confirm Deploy` — см. handoff |
|
||
|
||
### Ось 2 — Соответствие ADR (`06-adr/ADR-001` D1–D11, сквозной `adr-0035`) — ✅
|
||
|
||
- **D1** top-level `onboarding/` ✅; **D2** `{{NAME}}` + `str.replace` + обязательный пост-скан
|
||
(`PLACEHOLDER_RE`, ValueError в `materialize_kit`) + биекция словаря тестом ✅; **D3**
|
||
live-copy verbatim, существующие файлы не перезаписываются ✅; **D4** закрытый список импортов
|
||
`src` — в скрипте ровно три (`settings`, `_PLANE_NAME_TO_KEY`, `_parse_projects_json`),
|
||
загвожден AST-тестом TC-21 ✅; **D5** `STATE_GROUPS` 1:1 с таблицей ADR (22 имени, set-равенство
|
||
с `_PLANE_NAME_TO_KEY` тестом; `STOP`→`cancelled`; терминальные группы только
|
||
Done/Cancelled/STOP; `Rejected`→`started`) ✅; **D6** `auto_init=False`, переиспользование
|
||
глобального HMAC-секрета, push только в свежесозданный/пустой репо ✅; **D7** merged-full-array
|
||
+ round-trip + `.env` read-only ✅; **D8** smoke на staging 8501, журнал в runbook ✅; **D9**
|
||
5 ru + deployer en с «Do NOT translate»-гардом и рамкой shared-host-гардрейлов (прочитано
|
||
лично) ✅; **D10** runbook §2.3 «branch protection НЕ включать» ✅; **D11** plan/apply/verify,
|
||
чистый `build_plan`, инжектируемые клиенты, отчёт `created/skipped(exists)/manual-step/planned/
|
||
error`, exit-коды 0/2/1 ✅.
|
||
- **Трассировка (`docs/_standards/TRACEABILITY.md`) — дельта цикла:**
|
||
- `tests/test_resolve_agent_{model,effort}.py` несут маркеры **ORCH-41/ORCH-074** — сверено с
|
||
`docs/work-items/ORCH-074/06-adr/ADR-001-model-name-validation.md` **Решение 3 (G4)**:
|
||
инвариант = «**shipped-дефолт** `agent_fallback_model` остаётся `""`». Фикс переводит ассерт
|
||
с env-backed singleton на **класс-дефолт поля** (`model_fields[...].default == ""`) — это
|
||
и есть подлинный инвариант ADR (заводской дефолт, а не рантайм-конфиг оператора);
|
||
never-break ассерты `is_valid_model` — байт-в-байт. Инвариант **сохранён и уточнён**,
|
||
обоснование — в коммит-месседже и инлайн-комментариях со ссылкой на ADR. Чужой инвариант
|
||
не сломан → finding нет.
|
||
- `docs/architecture/adr/README.md` — бэкфилл строк 0032–0035 сверен: все 4 файла
|
||
(`adr-0032-bug-fast-track`, `adr-0033-sidecar-watchdog`, `adr-0034-lessons-journal`,
|
||
`adr-0035-turnkey-project-onboarding`) существуют, привязки задач корректны, «текущий
|
||
максимум 0035» верен.
|
||
- `docs/operations/SETUP_WEBHOOKS.md` — обобщение per-repo **усиливает** инвариант одного
|
||
глобального HMAC-секрета (явное предупреждение про ротацию на всех репо разом).
|
||
- **Инварианты NFR-1/INV-4:** снапшот-тесты `STAGE_TRANSITIONS`/`QG_CHECKS` зелёные; push —
|
||
только initial в пустой репо вне конвейера; PR-merge API не затрагивается.
|
||
|
||
### Ось 3 — Качество кода — ✅ (с переносными P2 ниже)
|
||
|
||
- CLI: чистое разделение слоёв (ядро без I/O / тонкие клиенты / режимы), docstrings на всех
|
||
публичных функциях, единственная точка subprocess (только `git`, токен в логе маскируется
|
||
`://***@`), `ManualStep` fail-safe вместо падений, delete-операций нет вовсе, секрет в отчёте
|
||
`***` + тест non-leak. Тесты содержательные: AST-проверка закрытого списка импортов,
|
||
monkeypatch-мины на мутации в dry-run, негативные CE-сценарии, set-равенство против дрейфа
|
||
констант — не тривиальные.
|
||
- **Фикс герметичности (дельта цикла) — корректен:** autouse-фикстуры пиняют shipped-дефолты
|
||
(зеркально между файлами-сиблингами), в чистом env поведение байт-эквивалентно; класс среды
|
||
merge-gate re-test (прод-env) теперь покрыт. Перепроверено прогоном: 1713 passed под хост-env.
|
||
Правка существующих тестов вне инвентаря ТЗ §2 — легитимна: инвентарь «рабочее предложение»,
|
||
ни один инвариант §9 не запрещает правку тестов; без неё PR непроходим через merge-gate
|
||
(латентная мина `main`, детонированная сменой прод-env).
|
||
- Багфикс-трек (ORCH-019, BR-4): не применим — задача не `Bug`, маршрут полный.
|
||
|
||
### Ось 4 — Документация — ✅ ОБНОВЛЕНА В ТОМ ЖЕ PR
|
||
|
||
| Точка | Статус |
|
||
|---|---|
|
||
| `CLAUDE.md` — раздел «Turnkey-онбординг проектов (ORCH-009)» | ✅ |
|
||
| `docs/architecture/README.md` — раздел + ссылки на оба ADR | ✅ (diff прочитан, фактам соответствует) |
|
||
| `CHANGELOG.md` — детальная `feat`-запись **+ отдельная под-запись про фикс герметичности тестов** | ✅ (дельта цикла задокументирована — образцово) |
|
||
| ADR per-WI `06-adr/ADR-001` + сквозной `adr-0035` + индекс `adr/README.md` | ✅ |
|
||
| `docs/operations/ONBOARDING.md` (новый runbook) | ✅ |
|
||
| `docs/operations/SETUP_WEBHOOKS.md` — обобщён per-repo | ✅ |
|
||
| `onboarding/README.md` — устройство kit, словарь, анти-форк | ✅ |
|
||
| README «Известные ограничения» (ORCH-079) | **N/A — проверено лично:** открыты 3 пункта (Telegram 48h / intra-repo deps ORCH-026 / пакетный автоном Этап 1) — ни один этим PR не закрывается |
|
||
| `17-security-report.md` | ✅ `security_status: PASS` (0 secrets, 0 blocking) |
|
||
| `08-data-requirements.md` отсутствует | Легитимно: гейт `check_analysis_complete` требует 01–04; ТЗ §5 «изменений БД нет» |
|
||
|
||
## Findings
|
||
|
||
### P0 — Blocker
|
||
- (нет)
|
||
|
||
### P1 — Must fix
|
||
- (нет)
|
||
|
||
### P2 — Should fix (перенос из review v1 — не фикшены, перепроверены: всё ещё в коде; follow-up, не блокируют)
|
||
- [ ] **Quoted-значение в `.env` → тихая потеря существующих записей в merged-выводе.**
|
||
`read_existing_registry` (строка ~355) возвращает значение после `=` как есть; кавычки →
|
||
`json.loads` в `merged_projects_json` молча даёт `existing=[]` → merged-массив только с новым
|
||
проектом, а runbook §4.1 велит «заменить строку». Доминирующий путь безопасен (pydantic
|
||
снимает кавычки), потому P2. Рекомендация: `strip("'\"")` в фоллбеке + GAP-warning, если строка
|
||
в `.env` есть, а existing пуст. (ADR D7 «существующие не теряются».)
|
||
- [ ] **`GiteaClient.create_repo`: фоллбек `POST /user/repos` может создать репо в чужом
|
||
namespace** (строки ~474–477): owner не org и не юзер токена → репо рождается под юзером
|
||
токена, последующие шаги по `owner/repo` дают 404/manual-step. Рекомендация: сверять
|
||
`owner.login` ответа с запрошенным; расхождение → `manual-step`.
|
||
- [ ] **CE-деградация Plane + успешный Gitea в одном apply запекает литерал
|
||
`<assigned-on-apply>` в запушенный паспорт** (`build_params` → `PLANE_PROJECT_ID`); скан ловит
|
||
только `{{…}}`. Рекомендация: при неразрешённом `PLANE_PROJECT_ID` деградировать
|
||
`kit.materialize`/`kit.push` в `manual-step` ИЛИ добавить `<assigned-on-apply>` в скан verify.
|
||
|
||
### P3 — Nice to have
|
||
- [ ] `--env-file` игнорируется в `plan` (`run_plan` → `_registry_instructions(report, params,
|
||
None)`; `main()` его в `run_plan` и не передаёт): превью merged-массива может расходиться с apply.
|
||
- [ ] Push-URL с `oauth2:<token>@` остаётся в `.git/config` temp-каталога после успешного apply
|
||
(cleanup нет). Рекомендация: чистить на успехе, на ошибке сохранять для дебага.
|
||
- [ ] *(новое)* `run_apply`: шаг `registry.emit` добавляется со статусом `CREATED` **до**
|
||
`_registry_instructions`, который на ошибке round-trip добавляет второй шаг `registry.emit`
|
||
со статусом `ERROR` → дубль step-id в отчёте (exit-код при этом честный — 1). Косметика отчёта.
|
||
|
||
## Документация
|
||
|
||
Обновлена полностью в том же PR (таблица оси 4). Несоответствий «код изменён — дока молчит» нет;
|
||
дельта цикла (фикс тестов) получила собственную CHANGELOG-запись с диагнозом и обоснованием;
|
||
обзорная витрина README задачей не затрагивается (проверено: открытые ограничения не про онбординг).
|
||
|
||
## Для следующей стадии (testing) — handoff
|
||
|
||
1. **AC-13 (операторский smoke, ADR D8)** — единственный непокрытый pytest'ом AC: прогон по
|
||
runbook §5.2 (staging 8501, sandbox `SMK`) должен быть выполнен и запротоколирован в «Журнале
|
||
smoke-прогонов» `ONBOARDING.md`, ссылка — из `13-test-report.md`. Обязателен **до**
|
||
`Confirm Deploy` (человеческий гейт — точка контроля сохраняется).
|
||
2. Средовая мина merge-gate обезврежена фиксом `e903818`: полный регресс зелёный и в чистом env,
|
||
и под прод-env (1713 passed, проверено в этом review) — спец-обвязка прогона больше не нужна.
|
||
3. `13-test-report.md` в дереве — от прошлого цикла (до `e903818`): его строка «PR эти файлы не
|
||
трогает» про `tests/test_resolve_agent_*` устарела. Перегенерировать отчёт штатно (артефакт
|
||
чужой стадии — в этом review не правился).
|