diff --git a/docs/work-items/ORCH-116/12-review.md b/docs/work-items/ORCH-116/12-review.md index c9a8af1..246f3a1 100644 --- a/docs/work-items/ORCH-116/12-review.md +++ b/docs/work-items/ORCH-116/12-review.md @@ -8,166 +8,132 @@ created_at: 2026-06-16 model_used: claude-opus-4-8 type: review work_item_id: ORCH-116 -version: 3 +version: 4 --- # Review ORCH-116 — детерминированный test-раннер вместо LLM-тестера +> Машинный вердикт читается ТОЛЬКО из `verdict:` во frontmatter. + ## Summary -PR заменяет LLM-агента `tester` на стадии `testing` (self-hosting `orchestrator`) детерминированным -leaf `src/test_runner.py`, перехватываемым в `launch_job` **до `_spawn`** — точное зеркало уже -работающего в проде ORCH-115/`staging_runner`. Ревью выполнено **независимо** по всем четырём осям -(ТЗ/AC · ADR · качество кода · документация) с личным прогоном тестов и сверкой сигнатур -зависимостей. Критический инвариант **NFR-1** («замена *продюсера* артефакта, не гейта») соблюдён -байт-в-байт: `src/stages.py`, `src/qg/checks.py`, `src/staging_runner.py`, `src/transition_lease.py`, -`src/proc_group.py` — **не тронуты** (подтверждено `git diff --stat`); в `src/` изменены только -`test_runner.py` (новый), `launcher.py`/`config.py`/`main.py` (аддитивно). Документация = golden -source обновлена в том же PR (8 doc-файлов + CHANGELOG + ADR-001/adr-0050). Полный регресс -**проверен лично**: `pytest tests/ -q` → **2137 passed**; ORCH-116 + LLM-анти-дрейф + system-docs → -**74 passed**. **P0/P1/P2 findings отсутствуют → `APPROVED`.** +Реализация **полностью соответствует ТЗ (02-trz.md), критериям приёмки (03-acceptance-criteria.md) +и ADR-001 / adr-0050**. ORCH-116 заменяет LLM-агента `tester` на стадии `testing` детерминированным +leaf `src/test_runner.py`, перехватываемым в `launch_job` до `_spawn` — точное зеркало эталона +`src/staging_runner.py` (ORCH-115). Критический инвариант NFR-1 соблюдён байт-в-байт: гейт +`check_tests_passed` / `_parse_tests_verdict`, `STAGE_TRANSITIONS`, `QG_CHECKS`, machine-key `result:` +и схема БД **не тронуты** (это замена *продюсера* артефакта, не гейта). Тонкая мина D6.1 (52c-`status:` +↔ парсер с negative-token-priority) обработана корректно и прибита тестом через неизменённый парсер. +Документация обновлена в полном объёме (CLAUDE.md / CHANGELOG / README / internals / витрина +`docs/overview/` / LLM-карта/roadmap/policy / tester.md / .env.example). Все 4 оси — зелёные. Findings +уровня P0/P1, относящихся к ORCH-116, нет. ## Оси проверки -### 1. Соответствие ТЗ / Acceptance Criteria — PASS +### 1. Соответствие ТЗ / AC — PASS +- **FR-1 / AC-1** перехват до `_spawn`: `launcher.launch_job` врезка `if job.get("agent")=="tester" + and test_runner.should_intercept(job): return self._run_test_runner_job(job)` рядом с + D1/D2/ORCH-115; `_run_test_runner_job` — зеркало `_run_staging_runner_job`, ведёт `jobs` через + `mark_job`, возвращает `None` (нет `agent_runs`). ✔ (TC-05). +- **FR-2/3 / AC-3/AC-11** pytest в worktree через `proc_group` (tree-kill, `_resolve_timeout` + fail-safe), маппинг `map_exit_code_to_result` поверх единого `self_deploy.map_exit_code_to_status` + (без второго маппинга). ✔ (TC-02, TC-12). +- **FR-4 / AC-2** `write_test_report` — `result:` UPPERCASE + 52c-схема, `author_agent: test-runner` / + `model_used: n/a`, push только в фичеветку (без merge в `main`). ✔ (TC-03, TC-04). +- **FR-5 / AC-4** `_advance` вызывает `advance_stage(current_stage="testing", finished_agent="tester")` + — FAIL-ветвь `stage_engine.py:849` матчит по `agent=="tester"`. ✔ (TC-07). +- **FR-6 / AC-5** two-level outcome: `suite_ran=(returncode is not None) and (not timed_out)`; + tool-error → bounded DEFER (re-queue именно **`tester`**-джоба + restart-safe маркер), на исчерпании + → fail-closed `FAIL` + advance + INFRA-alert. ✔ (TC-10). +- **FR-7 / AC-7/AC-8** `applies()` = kill-switch + скоуп + `_has_test_contract` (репо без контракта → + LLM-tester даже при ручном добавлении в CSV). ✔ (TC-01, TC-08). +- **FR-8 / AC-13** блок `test_runner` в `GET /queue` + счётчики + структурный лог, различающий + code-fail / tool-error. ✔ (TC-14). +- **FR-9 / AC-12** гибрид: LLM вне control-path вердикта. ✔. -Верифицировано по коду + тестам + личному прогону каждое AC: -- **AC-1** (перехват до `_spawn`) — `launcher.launch_job`: `if job.get("agent")=="tester":` → - `if test_runner.should_intercept(job): return self._run_test_runner_job(job)` вставлено **перед** - `return self._spawn(...)`; `_run_test_runner_job` ведёт `jobs` через `mark_job(done|failed)`, - возвращает `None` (нет `agent_runs`). TC-05 воспроизводит без живого CLI (`spawn.assert_not_called`). -- **AC-2** (контракт `13-test-report.md`) — `build_test_report` эмитит `result: PASS|FAIL` UPPERCASE - + полную 52c-схему; читается **неизменённым** `_parse_tests_verdict` (TC-04, лично подтверждено - прогоном через реальный парсер). -- **AC-3** (exit→verdict) — `map_exit_code_to_result` — тонкий транслятор над единым - `self_deploy.map_exit_code_to_status` (`0→PASS`, иначе/None/нечисло→`FAIL`, fail-closed); второй - маппинг не введён (TC-02 сверяет с базовым контрактом). -- **AC-4** (эквивалентность маршрутизации) — `_advance(current_stage="testing", - finished_agent="tester")`, без новых рёбер; PASS→advance, FAIL→существующий откат - `testing→development` (TC-07 параметризован [PASS/FAIL]). -- **AC-5** (two-level outcome, анти-ORCH-110) — `suite_ran = (returncode is not None) and (not - timed_out)`; tool-error → bounded DEFER re-queue **`tester`**-джоба (TC-10 проверяет `agent == - "tester"`), не код-фейл-откат, не жжёт developer-retry; исчерпание бюджета → fail-closed FAIL + - alert. -- **AC-6** (анти-дрейф) — `git diff` по `src/` затрагивает только - `launcher.py`/`config.py`/`main.py`/`test_runner.py`; `src/stages.py`, `src/qg/checks.py` - **байт-в-байт не тронуты** (проверено `git diff --stat` лично); новых таблиц нет (TC-09). -- **AC-7/AC-8** (kill-switch / backward-compat) — `applies()` гасит при `enabled=False`, скоупит CSV; - `_has_test_contract` → `False` для не-self репо даже при ручном добавлении в CSV (TC-01/TC-08). -- **AC-9** (never-raise) — все публичные функции в `try/except`; `run_test_gate` имеет внешний - guard; `_run_test_runner_job` дополнительно оборачивает (TC-11: воркер не падает / не клинит). -- **AC-10** (self-hosting safety) — `build_test_command` без литералов - рестарта/compose/build/force-push/`.env`; smoke строго read-only GET; лог пушится только в - фичеветку, никакой прямой работы с `main` (TC-13). -- **AC-11** (proc_group/worktree/timeout) — pytest в `get_worktree_path(repo, branch)` через - `run_in_process_group` (tree-kill), `_resolve_timeout` дефолтит на малформе + WARNING; без правки - `reaper_max_running_s` (TC-12). -- **AC-12/AC-13/AC-14/AC-15** — гибрид off-control-path, блок `test_runner` в `/queue`, обновление - LLM-карты/политики/roadmap/витрины, полный регресс — все зелёные (AC-15: 2137 passed, проверено - лично). - -### 2. Соответствие ADR — PASS - -Реализация дословно следует ADR-001 (D1–D12) и сквозному adr-0050. Сверено критическое: -- **D6.1 (анти-коллизия 52c-`status:` ↔ `_parse_tests_verdict`)** — самая тонкая мина задачи. - Сверено по **неизменённому** парсеру (`src/qg/checks.py:226-282`): он читает вердикт из ТРЁХ - равноранговых полей `verdict:`/`status:`/`result:` c negative-token-priority. `build_test_report` - жёстко выравнивает `sub_status = "success" if result=="PASS" else "failed"`: для PASS конкатенация - `"SUCCESS PASS"` не содержит ни одного из `_TESTS_NEGATIVE_TOKENS` - (`BLOCKED/FAILED/FAIL/REQUEST_CHANGES/REJECT/RED`), позитив `PASS` из `result:` → `True`; для FAIL - `"FAILED FAIL"` → негативный токен → `False`. Прибито TC-04 - (`test_tc04_status_field_never_false_negatives_a_pass`) через реальный парсер. Мина снята верно. -- **D5 DEFER** — зеркало `staging_runner._handle_tool_error`, но re-queue **`tester`**-джоба (не - `deployer`!), счётчик restart-safe по маркеру `test-runner infra-retry` в `task_content` (без - правки схемы БД). Проверено TC-10. -- **D4 единый маппинг** — переиспользован `self_deploy.map_exit_code_to_status` (BR-4), второй - несогласованный маппинг не введён. -- **Трассировка маркеров (TRACEABILITY):** правки рядом с ORCH-115 (`launcher` врезка), ORCH-114 - (transition-lease берётся **внутри** `advance_stage` — раннер не трогает, граница O1), ORCH-110 - (`proc_group` переиспользуется как есть) — ни один зафиксированный инвариант не сломан. -- **Глобальные ADR** (INV-4 «мерж только через PR», запрет рестарта прод-контейнера) — соблюдены: - лог пушится только в фичеветку (`git push origin `, не force, не `main`); смоук read-only. +### 2. Соответствие ADR / инварианты — PASS +- NFR-1 / AC-6: `git diff origin/main...HEAD` НЕ затрагивает `src/stages.py`, `src/qg/`, + `src/stage_engine.py`, `src/staging_runner.py`, `src/proc_group.py`, `src/transition_lease.py`, + `src/git_worktree.py` — проверено явно (пустой diff). ✔. +- **D6.1 (ключевая мина)**: `_parse_tests_verdict` (`src/qg/checks.py:263-277`) читает вердикт из трёх + полей с приоритетом негативного токена. Раннер выравнивает `status:` по вердикту (`PASS→success` + «SUCCESS» — не позитивный и не негативный токен → `True` берётся из `result: PASS`; `FAIL→failed` + «FAILED» — негативный → `False`). Проверено вручную по логике парсера и тестом TC-04 + (`test_tc04_status_field_never_false_negatives_a_pass`) через **неизменённый** парсер. ✔. +- Граница O1: `staging_runner` (ORCH-115), `transition_lease` (ORCH-114), `checkout_hygiene` + (ORCH-112) не модифицированы — соответствует требованию. ✔. +- Трассировка: новые врезки корректно ссылаются на прецеденты D1/D2/ORCH-115; зафиксированные + инварианты конвейера не сломаны. ✔. ### 3. Качество кода — PASS - -- **Docstrings** на всех публичных функциях; контракт **never-raise** выдержан сквозно (каждый - внешний вызов обёрнут; счётчики через `_bump` не ломают решение). -- **Сигнатуры зависимостей сверены лично** (риск latent false-FAIL устранён): - `db.enqueue_job(agent, repo, task_content, task_id=, available_at_delay_s=)`, `db.mark_job`, - `self_deploy.map_exit_code_to_status`, `git_worktree.get_worktree_path(repo, branch)`, - `qg.checks.is_self_hosting_repo`, `config.post_deploy_base_url` (дефолт `http://localhost:8500`), - `notifications.send_telegram`/`link_for`, `proc_group.run_in_process_group`/`ProcResult` — все - существуют ровно как используются. -- **Тесты содержательные** (TC-01…TC-15, ~32 тест-функции, не тривиальные): kill-switch/scope/contract, - маппинг по классам входа, рендер+неизменённый парсер, перехват до `_spawn`, two-level outcome, - never-raise, timeout-resolve, smoke read-only (включая транзиент-ретрай и missing-`serial_gate`), - анти-дрейф конвейера, наблюдаемость, структурный лог. -- **Security:** smoke — только `urllib` GET к локальному оркестратору; мутирующих запросов нет. -- **Багфикс-трек неприменим:** это **не** `Bug`-задача (full-cycle, стадия `architecture` отработана, - есть ADR) → требование регресс-фиксатора ORCH-019/BR-4 не применяется; покрытие тем не менее - исчерпывающее. +- never-raise соблюдён во всех публичных функциях (`applies`/`should_intercept`/`run_test_gate`/ + `snapshot`/`write_test_report`/`run_smoke` — guarded; TC-01/TC-06/TC-11/TC-14 покрывают). +- Лениво импортируются тяжёлые модули — leaf-чистота сохранена (импорт на уровне модуля только + `config`/`logging`/`proc_group`). +- Тесты содержательные (14 TC + анти-дрейф TC-15), без живого Claude CLI/сети; покрывают happy/FAIL/ + tool-error/never-raise/kill-switch/scope/smoke/observability. +- Это feature-трек (не `Bug`), регресс-тест-фиксатор (ORCH-019 BR-4) не требуется; покрытие тем не + менее исчерпывающее. ### 4. Документация — PASS (обязательная проверка) +`src/` изменён → документация обновлена в том же PR, проверено пофайлово: +- `CLAUDE.md` — новый раздел «Детерминированный test-раннер (ORCH-116)». ✔ +- `CHANGELOG.md` — детальная запись `[Unreleased]`. ✔ +- `docs/architecture/README.md` — компонент **Test-runner** + блок roadmap «второй срез реализован». ✔ +- `docs/architecture/internals.md` — примечание о перехвате на `testing`. ✔ +- `docs/architecture/llm-call-sites.md` (A5), `llm-determinization-roadmap.md` (rank 2), + `llm-usage-policy.md` (§5) — обновлены; **инвариант «ровно один `first_slice=yes`» сохранён** + (deployer=yes, tester=no), анти-дрейф `tests/test_llm_call_site_inventory.py` / + `test_llm_determinization_docs.py` — зелёные. ✔ +- Витрина `docs/overview/` (ORCH-011): `tech-pipeline.md` / `tech-agents.md` / + `tech-quality-security.md` обновлены; `tests/test_system_docs.py` зелёный. ✔ +- `.openclaw/agents/tester.md` — LLM-ветвь помечена fallback'ом (канон 52d цел). ✔ +- `.env.example` — все 7 ключей `ORCH_TEST_RUNNER_*`. ✔ +- ADR + сквозной `adr-0050` присутствуют. ✔ -`src/` изменён → документация обновлена **в том же PR**, проверено явно по `git diff` (наличие -правок подтверждено по каждому файлу): -- **Паспорт** `CLAUDE.md` — новый раздел «Детерминированный test-раннер вместо LLM-тестера (ORCH-116)». -- **Компонент-карта** `docs/architecture/README.md` (3 hits) + **внутренности** `internals.md` - (5 hits) — перехват на `testing` рядом с ORCH-115. -- **LLM-карта/политика/roadmap** — `llm-call-sites.md` (A5), `llm-determinization-roadmap.md` - (rank 2 ✅ реализован; инвариант «ровно один `first_slice=yes`» цел — `yes` у rank 1 deployer, - `no`/`hybrid_needed=yes` у rank 2 tester), `llm-usage-policy.md` (§5); анти-дрейф-тесты - `test_llm_call_site_inventory.py`/`test_llm_determinization_docs.py` — **зелёные (лично прогнаны)**. -- **Витрина системы `docs/overview/` (ORCH-011)** — обновлены `tech-pipeline.md`, `tech-agents.md`, - `tech-quality-security.md`; `tests/test_system_docs.py` — **зелёный (лично прогнан)**. -- **Промпт** `.openclaw/agents/tester.md` — врезка «детерминированный раннер ведёт стадию; LLM — - fallback под выключенным флагом / для репо без контракта»; канон 52d (5 секций, ключ `result:`) цел. -- **`CHANGELOG.md`** — развёрнутая запись `[Unreleased]`. -- **ADR** заведён (`06-adr/ADR-001-deterministic-test-runner.md` + сквозной - `adr/adr-0050-deterministic-test-runner.md`). - -Обзорные доки / README «Известные ограничения» (ORCH-079): раннер — **новая способность**, не -закрытие документированного ограничения → дополнительных правок README не требуется. +## Тесты (AC-15) +- `tests/test_orch116_test_runner.py` + LLM анти-дрейф — **45 passed**. +- Полный регресс `pytest tests/ -q` — **2162 passed, 1 failed** (84s). Единственный фейл — + `tests/test_orch123_staging_runner_exec.py::test_r2_held_deploy_staging_not_rolled_back` (см. P2). ## Findings ### P0 — Blocker -- Нет. +- (нет) ### P1 — Must fix -- Нет. +- (нет) ### P2 — Should fix -- Нет. +- **[Вне ORCH-116, для отдельного follow-up] Pre-existing test-isolation flake в ORCH-123.** + В полном прогоне падает `tests/test_orch123_staging_runner_exec.py::test_r2_held_deploy_staging_ + not_rolled_back`. Доказано, что это **не** дефект ORCH-116: (1) тест зелёный в изоляции и в своём + файле; (2) фейл воспроизводится при прогоне полного сьюта **без** `tests/test_orch116_test_runner.py` + (`--ignore`) → 1 failed; (3) ORCH-116 не трогает `src/staging_runner.py` (граница O1, корректно). + Это загрязнение глобального состояния от соседнего теста в ORCH-123-коде (недавно влит в `main`). + AC-15 в части «ORCH-116 не краснит ранее зелёные тесты» удовлетворён. Рекомендация: завести + отдельный bug на изоляцию теста ORCH-123 — он вне области ORCH-116 и не должен блокировать этот PR. -### P3 — Nice-to-have (не блокирует) -- [ ] `_http_get` читает первые 8192 байта тела; smoke ищет `serial_gate` в ответе `/queue`. **Лично - измерено:** `serial_gate` на смещении **1471 байт** при полном ответе ~3.6 КБ → запас огромный, - **сейчас корректно**. Латентная хрупкость лишь при экстремальном разрастании ранних блоков; можно - опционально поднять лимит чтения для `/queue` или искать ключ потоково. -- [ ] `build_test_command` добавляет `-q`, отсутствующий в литеральной команде ADR/CHANGELOG (`python - -m pytest `). Безвредно (exit-код, а не парс вывода, формирует вердикт; согласовано с прежним - промптом `pytest tests/ -q`); фиксирую лишь как расхождение текст↔код. -- [ ] Tracked-scratch `.task-dev.md` перезаписан с ORCH-115 на ORCH-116 (нормальная churn dev-раннера, - предсуществующий паттерн, не введён этим PR; вне осей ревью). - -### Контекст (не finding) -- В истории ветки присутствует `15-staging-log.md` со `staging_status: FAILED` (`exit_code: None` — - staging-сюита **не исполнилась**, tool-error/инфра, бюджет инфра-ретраев исчерпан → fail-closed). - Это исход стадии **deploy-staging** (downstream от review), относится к staging-раннеру ORCH-115 и - является **инфра-сбоем, а не дефектом кода ORCH-116** — вне осей код-ревью. Поведение гейта - корректно (fail-closed на исчерпании инфра-бюджета); правок в коде задачи не требует. Отмечено как - контекст для последующих стадий. +### P3 — Nice to have +- `run_smoke` (`src/test_runner.py:291`) дублирует литерал `"http://localhost:8500"`, который уже + является дефолтом `settings.post_deploy_base_url`. Это безвредный defensive double-default + (`localhost`/`8500` НЕ в FORBIDDEN-списке `test_no_host_hardcodes` — нарушения нет); можно опереться + только на дефолт config. Косметика. +- Smoke-провал из-за транзиентного блипа прод-8500 после bounded-ретраев даёт `FAIL` → откат + + расход developer-retry (формально инфра, трактуемая как код-фейл — родственно классу ORCH-110). + Архитектор **осознанно** взвесил это в ADR (D3 + Consequences «−»): bounded smoke-ретрай + + config-gate `test_runner_smoke_enabled` + developer-retry-cap; поведение эквивалентно прежнему + LLM-tester (`curl /health`). Принято как задокументированный компромисс; не блокирует. ## Документация -Обновлена в полном объёме в том же PR (см. ось 4): `CLAUDE.md`, `docs/architecture/README.md`, -`internals.md`, `llm-call-sites.md`, `llm-determinization-roadmap.md`, `llm-usage-policy.md`, -`docs/overview/{tech-pipeline,tech-agents,tech-quality-security}.md`, `.openclaw/agents/tester.md`, -`CHANGELOG.md`, ADR-001 + сквозной adr-0050. Анти-дрейф-тесты документации -(`test_llm_call_site_inventory.py`, `test_llm_determinization_docs.py`, `test_system_docs.py`) — -зелёные (лично прогнаны). Невыполненных требований к обновлению документации **нет**. +Обновлена в полном объёме в том же PR (см. ось 4). Изменение `src/` сопровождено обновлением +паспорта, чейнджлога, архитектурной карты, internals, витрины `docs/overview/`, LLM-карты/roadmap/ +политики, промпта `tester.md`, `.env.example` и ADR (локальный + сквозной). Машинно-проверяемые факты +витрины и LLM-карты держатся зелёными анти-дрейф-тестами. Требование «документация = golden source» +выполнено → блокирующих документационных findings нет. ## Вердикт -Нет P0/P1/P2 findings; критический инвариант NFR-1 соблюдён байт-в-байт (`stages.py`/`qg/checks.py` -не тронуты — проверено лично); документация = golden source обновлена синхронно с кодом; полный -регресс зелёный лично проверен (2137 passed). **`verdict: APPROVED`.** +Нет P0/P1, относящихся к ORCH-116. Все четыре оси (ТЗ, ADR/инварианты, качество кода, документация) +пройдены; критические инварианты сохранены байт-в-байт; покрытие исчерпывающее. **APPROVED.** +Единственный красный тест полного сьюта — pre-existing flake ORCH-123 (P2), доказанно независимый от +ORCH-116 и вне его области правки.