diff --git a/docs/work-items/ORCH-009/12-review.md b/docs/work-items/ORCH-009/12-review.md index 93b8107..39c371c 100644 --- a/docs/work-items/ORCH-009/12-review.md +++ b/docs/work-items/ORCH-009/12-review.md @@ -8,138 +8,160 @@ created_at: 2026-06-10 model_used: claude-fable-5 type: review work_item_id: ORCH-009 -version: 1 +version: 2 --- -# Review ORCH-009 — Turnkey-онбординг проектов (kit + CLI + runbook) +# Review ORCH-009 — Turnkey-онбординг проектов (kit + CLI + runbook) — re-review (цикл 2) -Ветка: `feature/ORCH-009-turnkey-plane` · Diff vs `origin/main`: 41 файл, +5120/−10. +Ветка: `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. +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 -PR реализует ТЗ полностью и точно по ADR-001 (D1–D11), с нулевым касанием рантайма. Вердикт -**APPROVED**: P0/P1 findings нет; найдены 3×P2 (харднинг краевых путей CLI) и 2×P3 (косметика) — -не блокируют, рекомендованы к follow-up. Документация обновлена в том же PR по всем требуемым -точкам. +**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-запись про сам фикс тестов. -**Проверено по 4 осям:** +## Оси проверки ### Ось 1 — Соответствие ТЗ (`02-trz.md`, `03-acceptance-criteria.md`) — ✅ | Требование | Статус | Чем подтверждено | |---|---|---| -| FR-1 состав kit | ✅ | AC-1/TC-01: все 19 элементов на месте; `_templates`/`_standards` в kit НЕ хранятся (анти-форк тест) | -| FR-2 канон 52d/92 промптов | ✅ | AC-2/TC-03…06: 5 XML-секций в нормативном порядке, «❌→✅», `` у dev/reviewer/tester, `` — паритет с эталоном (architect/reviewer/tester/deployer в обоих деревьях), 52c-схема, verdict-ключи байт-в-байт, даты/модели — плейсхолдеры | -| FR-2 reviewer-gate доки | ✅ | AC-3/TC-07: «документация НЕ обновлена → `REQUEST_CHANGES`» в kit reviewer.md | -| FR-3 INFRA.md шаблон | ✅ | AC-10/TC-19: топология/контейнеры/env-карта/границы доступа/риски общего хоста/деплой | -| FR-4 CLI plan/apply/verify | ✅ | AC-7/8/9, TC-13…18: 22 статуса из `_PLANE_NAME_TO_KEY`, группы по D5, лейблы из конфига, dry-run без единой мутации (мутации materialize/push замоканы на AssertionError), идемпотентный ensure, delete-операций нет, `docker`/`systemctl`/`compose`/запись `.env` отсутствуют в исходнике (TC-18-тест по AST/grep) | -| FR-5 verify | ✅ | round-trip реестра фактическим парсером, резолв всех 22 имён (включая fail-closed `Confirm Deploy`/`STOP` — отдельный негативный тест), лейблы, webhook active, полнота kit, скан `{{…}}` | -| FR-6 runbook | ✅ | AC-11/TC-20: все слои в порядке, 🖐-маркировка ручных шагов + команды проверки, self-hosting-предупреждение о групповом окне рестарта, workspace-webhook — «существует, только проверка» | -| §4/§5 нет API/БД изменений | ✅ | diff: `src/**` пуст | -| §9 инварианты | ✅ | см. ось 2 | -| AC-12 регресс | ✅ | 1794 passed локально; 2 падения (`test_resolve_agent_model`/`_effort`) — **средовые, pre-existing**: вызваны `ORCH_AGENT_FALLBACK_MODEL`/`ORCH_AGENT_EFFORT_*` в env агент-раннера, с очищенной средой 49/49 зелёные; файлы этих тестов PR не трогает — не регресс этого PR (авторитетен CI с чистой средой) | -| AC-13 операторский smoke | ⏳ | По построению выполняется на приёмке (tester/оператор): runbook §5.2 + «Журнал smoke-прогонов» (плейсхолдер первого прогона), D8 требует ссылку из `13-test-report.md`. **Handoff-заметка стадии testing** — см. «Для следующей стадии» | +| FR-1 состав kit (19 элементов, анти-форк канона) | ✅ | TC-01/02 зелёные; `docs/_templates|_standards` в kit не хранятся — live-copy в `materialize_kit` (`LIVE_COPY_DIRS`, прочитан код) | +| FR-2 канон 52d/92 промптов (5 секций, ❌→✅, ``, 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`, сквозной `adr-0035`) — ✅ +### Ось 2 — Соответствие ADR (`06-adr/ADR-001` D1–D11, сквозной `adr-0035`) — ✅ -- **D1–D11 реализованы без отступлений**: раскладка top-level `onboarding/` (D1); `{{NAME}}` + - `str.replace` + обязательный пост-скан + биекция словаря (D2, тест); live-copy verbatim - `_templates`(≥16)/`_standards`(≥3) (D3, тест байт-сравнения); закрытый список импортов `src` — - AST-тест `test_tc21_cli_src_imports_stay_in_closed_list` (D4); таблица групп `STATE_GROUPS` 1:1 - с таблицей D5, код-критичные констрейнты загвождены тестом (`STOP`→`cancelled`; терминальные - группы == {Done, Cancelled, STOP}; set-равенство с `_PLANE_NAME_TO_KEY` ловит будущий дрейф); - `auto_init=false` + переиспользование глобального секрета + push только в пустой репо (D6 — - сверил с приёмником `src/webhooks/gitea.py:38-41`: действительно ОДИН глобальный секрет); - merged-full-array + round-trip + «.env не правим» (D7); smoke на staging 8501 (D8); 5 ru + - deployer en c «Do NOT translate»-гардом (D9, тест на кириллицу); runbook фиксирует «branch - protection НЕ включать» (D10); plan/apply/verify, чистый `build_plan`, инжектируемые клиенты, - отчёт `created/skipped(exists)/manual-step/planned/error`, exit-коды 0/2/1 (D11). -- **Инварианты NFR-1/INV-4**: `git diff origin/main...HEAD -- src/ .openclaw/ docs/_templates/ - docs/_standards/ docs/operations/INFRA.md` — пусто; снапшот-тесты `STAGE_TRANSITIONS`/`QG_CHECKS` - зелёные; push — только initial в свежесозданный пустой репо (вне конвейера до регистрации), - PR-merge API не затрагивается. -- **Трассировка (TRACEABILITY.md)**: правка `docs/operations/SETUP_WEBHOOKS.md` обобщает - enduro-хардкод, **усиливая** (не ломая) инвариант одного глобального HMAC-секрета — сверено с - кодом приёмника; правка `docs/architecture/adr/README.md` — реестр сквозных ADR (общий индекс), - бэкфилл строк 0032–0034 сверен: все три файла существуют в main, номера/задачи корректны, - «текущий максимум 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-findings ниже) +### Ось 3 — Качество кода — ✅ (с переносными P2 ниже) -Docstrings на всех публичных функциях; чистое ядро отделено от I/O; единственная точка -subprocess (только `git`, cwd = temp-материализация, токен в логе маскируется); секрет в отчёте -`***` + явный тест non-leak (`test_secret_never_leaks_into_report`); тесты содержательные -(recording-фейки мутаций, негативные сценарии CE-отказов, AST-проверка импортов, monkeypatch-мины -на мутации в dry-run) — не тривиальные. Багфикс-трек не применим (задача не `Bug`). +- 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 | ✅ | -| `CHANGELOG.md` — запись `feat` (детальная) | ✅ | +| `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 (ТЗ §2) | ✅ | +| `docs/operations/SETUP_WEBHOOKS.md` — обобщён per-repo | ✅ | | `onboarding/README.md` — устройство kit, словарь, анти-форк | ✅ | -| README «Известные ограничения» (ORCH-079) | N/A — онбординг в списке открытых ограничений не значится, обновление не требуется (проверено) | -| `08-data-requirements.md` отсутствует | Легитимно: гейт `check_analysis_complete` требует только 01–04; ТЗ §5 «изменений БД нет» | +| 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 (follow-up, не блокирует) +### P2 — Should fix (перенос из review v1 — не фикшены, перепроверены: всё ещё в коде; follow-up, не блокируют) - [ ] **Quoted-значение в `.env` → тихая потеря существующих записей в merged-выводе.** - `read_existing_registry` (fallback-парс `.env`) возвращает значение после `=` как есть; если - строка в `.env`/`--env-file` взята в кавычки (`ORCH_PROJECTS_JSON='[...]'`), `json.loads` - в `merged_projects_json` молча даёт `existing=[]` → инструкция оператору содержит массив ТОЛЬКО - с новым проектом, а runbook §4.1 велит «заменить строку» → риск выпадения enduro/orchestrator - из реестра. Доминирующий путь безопасен (pydantic `env_file=".env"` снимает кавычки, фоллбек - срабатывает только при cwd вне корня), потому P2, не P1. Рекомендация: в фоллбеке `strip("'\"")` - + предупреждение/GAP, если строка в `.env` есть, а распарсенный existing пуст. (AC-6-периметр; - ADR D7 «существующие не теряются».) + `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.** При `--gitea-owner`, не являющемся ни org, ни юзером токена, отказ org-маршрута - ведёт в `/user/repos` → репо рождается под юзером токена, последующие webhook/push по - `owner/repo` дают 404/manual-step. Не деструктивно и видимо в отчёте, но это непрошенная - мутация не туда. Рекомендация: сверять `owner.login` ответа с запрошенным owner; расхождение → - `manual-step` (комментарий в коде уже упоминает admin-маршрут — либо реализовать - `/admin/users/{owner}/repos`, либо честно деградировать). + namespace** (строки ~474–477): owner не org и не юзер токена → репо рождается под юзером + токена, последующие шаги по `owner/repo` дают 404/manual-step. Рекомендация: сверять + `owner.login` ответа с запрошенным; расхождение → `manual-step`. - [ ] **CE-деградация Plane + успешный Gitea в одном apply запекает литерал - `` в запушенный паспорт.** Если `plane.project` ушёл в `manual-step`, а репо - создан — kit рендерится с `PLANE_PROJECT_ID=""` и пушится; повторный apply - с `--plane-project-id` уже не перезапишет (репо непустой). Скан ловит только `{{…}}`-синтаксис. - Рекомендация: при неразрешённом PLANE_PROJECT_ID деградировать `kit.materialize`/`kit.push` в - `manual-step` (push после получения uuid) ИЛИ добавить `` в скан verify. + `` в запушенный паспорт** (`build_params` → `PLANE_PROJECT_ID`); скан ловит + только `{{…}}`. Рекомендация: при неразрешённом `PLANE_PROJECT_ID` деградировать + `kit.materialize`/`kit.push` в `manual-step` ИЛИ добавить `` в скан verify. ### P3 — Nice to have -- [ ] `--env-file` молча игнорируется в `plan`-режиме (`_registry_instructions(report, params, - None)`): превью merged-массива в plan может расходиться с apply при нестандартном env-файле. -- [ ] Push-URL с `oauth2:@` остаётся в `.git/config` временного каталога материализации - после успешного apply (temp-dir не чистится). Рекомендация: cleanup на успехе (на ошибке - сохранять для дебага, как сейчас). +- [ ] `--env-file` игнорируется в `plan` (`run_plan` → `_registry_instructions(report, params, + None)`; `main()` его в `run_plan` и не передаёт): превью merged-массива может расходиться с apply. +- [ ] Push-URL с `oauth2:@` остаётся в `.git/config` temp-каталога после успешного apply + (cleanup нет). Рекомендация: чистить на успехе, на ошибке сохранять для дебага. +- [ ] *(новое)* `run_apply`: шаг `registry.emit` добавляется со статусом `CREATED` **до** + `_registry_instructions`, который на ошибке round-trip добавляет второй шаг `registry.emit` + со статусом `ERROR` → дубль step-id в отчёте (exit-код при этом честный — 1). Косметика отчёта. ## Документация -Обновлена полностью в том же PR (см. таблицу оси 4): паспорт, архитектурный README, CHANGELOG, -оба ADR + индекс, новый runbook, обобщённый SETUP_WEBHOOKS, README kit. Несоответствий -«код изменён — дока молчит» не найдено; обзорная витрина README не затронута задачей по -построению (ограничение в ней не значилось). +Обновлена полностью в том же PR (таблица оси 4). Несоответствий «код изменён — дока молчит» нет; +дельта цикла (фикс тестов) получила собственную CHANGELOG-запись с диагнозом и обоснованием; +обзорная витрина README задачей не затрагивается (проверено: открытые ограничения не про онбординг). ## Для следующей стадии (testing) — handoff -1. **AC-13 (операторский smoke)**: прогон по runbook §5.2 (staging 8501, sandbox `SMK`) должен - быть выполнен и запротоколирован в «Журнале smoke-прогонов» `ONBOARDING.md`, ссылка — из - `13-test-report.md` (требование D8). Это единственный непокрытый pytest'ом AC. -2. Локальный полный регресс гонять с чистой средой (без `ORCH_AGENT_FALLBACK_MODEL`/ - `ORCH_AGENT_EFFORT_*` агент-раннера) — иначе 2 ложных средовых падения в - `test_resolve_agent_model.py`/`test_resolve_agent_effort.py` (pre-existing, к PR отношения - не имеют). +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 не правился).