reviewer(ET): auto-commit from reviewer run_id=741
This commit is contained in:
135
docs/work-items/ORCH-116/12-review.md
Normal file
135
docs/work-items/ORCH-116/12-review.md
Normal file
@@ -0,0 +1,135 @@
|
||||
---
|
||||
verdict: APPROVED
|
||||
work_item: ORCH-116
|
||||
stage: review
|
||||
author_agent: reviewer
|
||||
status: approved
|
||||
created_at: 2026-06-16
|
||||
model_used: claude-opus-4-8
|
||||
type: review
|
||||
work_item_id: ORCH-116
|
||||
version: 1
|
||||
---
|
||||
|
||||
# Review ORCH-116 — детерминированный test-раннер вместо LLM-тестера
|
||||
|
||||
## Summary
|
||||
|
||||
PR заменяет LLM-агента `tester` на стадии `testing` (self-hosting `orchestrator`)
|
||||
детерминированным leaf `src/test_runner.py`, перехватываемым в `launch_job` **до `_spawn`** —
|
||||
точное зеркало уже работающего в проде ORCH-115/`staging_runner`. Проверено по всем четырём осям:
|
||||
соответствие ТЗ/AC, соответствие ADR, качество кода, документация. **Все 15 критериев приёмки
|
||||
выполнены**, критический инвариант NFR-1 («замена *продюсера* артефакта, не гейта») соблюдён
|
||||
байт-в-байт, документация (включая витрину `docs/overview/` и LLM-карту/roadmap/политику) обновлена
|
||||
в том же PR. Полный регресс зелёный (`2137 passed`), новый `tests/test_orch116_test_runner.py`
|
||||
зелёный (32 теста), LLM-анти-дрейф зелёный. **P0/P1 findings отсутствуют → `APPROVED`.**
|
||||
|
||||
## Оси проверки
|
||||
|
||||
### 1. Соответствие ТЗ / Acceptance Criteria — PASS
|
||||
|
||||
Верифицировано по коду + тестам каждое AC:
|
||||
- **AC-1** (перехват до `_spawn`) — `launcher.launch_job`: `if job.get("agent")=="tester" and
|
||||
test_runner.should_intercept(job): return self._run_test_runner_job(job)`; `_run_test_runner_job`
|
||||
ведёт `jobs` через `mark_job`, возвращает `None` (нет `agent_runs`). TC-05 воспроизводит без живого CLI.
|
||||
- **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`); второй маппинг не введён (TC-02).
|
||||
- **AC-4** (эквивалентность маршрутизации) — `_advance(finished_agent="tester", current_stage="testing")`,
|
||||
без новых рёбер; PASS→advance, FAIL→существующий откат `testing→development` (TC-07/TC-10).
|
||||
- **AC-5** (two-level outcome) — `run_test_gate`: `suite_ran = returncode is not None and not timed_out`;
|
||||
tool-error → bounded DEFER re-queue **`tester`**-джоба, не код-фейл-откат (TC-10).
|
||||
- **AC-6** (анти-дрейф) — `git diff` по `src/` затрагивает только `launcher.py`/`config.py`/`main.py`/
|
||||
`test_runner.py`; `src/stages.py` (STAGE_TRANSITIONS) и `src/qg/checks.py` (`check_tests_passed`/
|
||||
`_parse_tests_verdict`) **не тронуты**; новых таблиц нет (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` не роняет воркер (TC-11).
|
||||
- **AC-10** (self-hosting safety) — `build_test_command` без литералов рестарта/compose/build/force-push/
|
||||
`.env`; smoke строго read-only GET (TC-13).
|
||||
- **AC-11** (proc_group/worktree/timeout) — pytest в `get_worktree_path` через `run_in_process_group`,
|
||||
`_resolve_timeout` дефолтит на малформе (TC-12).
|
||||
- **AC-12/AC-13/AC-14/AC-15** — гибрид off-control-path, блок `test_runner` в `/queue`, обновление
|
||||
LLM-карты/политики/витрины, полный регресс — все зелёные.
|
||||
|
||||
### 2. Соответствие ADR — PASS
|
||||
|
||||
Реализация дословно следует ADR-001 (D1–D12) и сквозному adr-0049. Ключевые места сверены:
|
||||
- **D6.1 (анти-коллизия 52c-`status:` ↔ `_parse_tests_verdict`)** — корректно: `PASS → status: success`
|
||||
(`SUCCESS` не позитивный и не негативный токен, позитив `PASS` берётся из `result:`), `FAIL → status:
|
||||
failed` (`FAILED` — негативный токен, согласован). Прибито TC-04/`test_tc04_status_field_never_false_
|
||||
negatives_a_pass` через неизменённый парсер. Это была самая тонкая мина задачи — снята верно.
|
||||
- **D5 DEFER** — побайтовое зеркало `staging_runner._handle_tool_error`, но re-queue **`tester`**-джоба
|
||||
(не `deployer`) — проверено TC-10 (`assert agent == "tester"`). Счётчик restart-safe по маркеру в
|
||||
`task_content`, без правки схемы БД.
|
||||
- **Трассировка маркеров (TRACEABILITY):** правки рядом с ORCH-115 (`launcher`), ORCH-114
|
||||
(`advance_stage`/transition-lease — берётся **внутри** `advance_stage`, раннер не трогает, граница O1),
|
||||
ORCH-110 (`proc_group` переиспользуется как есть) — ни один зафиксированный инвариант не сломан.
|
||||
- Глобальные ADR (INV-4 «мерж только через PR», запрет рестарта прод-контейнера) — соблюдены: лог
|
||||
пушится только в фичеветку, никакой работы с `main`.
|
||||
|
||||
### 3. Качество кода — PASS
|
||||
|
||||
- Docstrings на всех публичных функциях; контракт never-raise выдержан сквозно (каждый внешний вызов
|
||||
обёрнут, наблюдательные счётчики через `_bump` не ломают решение).
|
||||
- Тесты содержательные (32 TC, не тривиальные): покрыты kill-switch/scope/contract, маппинг по классам
|
||||
входа, рендер+парсер, перехват, two-level outcome, never-raise, timeout, smoke read-only,
|
||||
анти-дрейф, наблюдаемость.
|
||||
- Сигнатуры зависимостей сверены: `db.enqueue_job(..., available_at_delay_s=)`, `db.mark_job`,
|
||||
`self_deploy.map_exit_code_to_status`, `config.post_deploy_base_url` (дефолт `http://localhost:8500`)
|
||||
— все существуют, AttributeError-риск smoke отсутствует.
|
||||
- Security: smoke — только `urllib` GET к локальному оркестратору; мутирующих запросов нет.
|
||||
- Это **не** багфикс-трек (стадия `architecture` отработана, full-cycle) → требование регресс-фиксатора
|
||||
ORCH-019/BR-4 неприменимо; тем не менее покрытие исчерпывающее.
|
||||
|
||||
### 4. Документация — PASS (обязательная проверка)
|
||||
|
||||
`src/` изменён → документация обновлена **в том же PR**, проверено явно:
|
||||
- **Паспорт** `CLAUDE.md` — новый раздел «Детерминированный test-раннер (ORCH-116)».
|
||||
- **Компонент-карта** `docs/architecture/README.md` — запись **Test-runner** + блок roadmap «второй
|
||||
срез реализован».
|
||||
- **Внутренности** `docs/architecture/internals.md` — примечание о перехвате на `testing`.
|
||||
- **LLM-карта/политика/roadmap** — `llm-call-sites.md` (A5), `llm-determinization-roadmap.md` (rank 2 ✅),
|
||||
`llm-usage-policy.md` (§5); инвариант «ровно один `first_slice=yes`» цел; анти-дрейф-тесты зелёные.
|
||||
- **Витрина системы `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».
|
||||
- **`CHANGELOG.md`** — запись `[Unreleased]`.
|
||||
- **ADR** заведён (`06-adr/ADR-001` + сквозной `adr-0049`).
|
||||
|
||||
Обзорные доки / «Известные ограничения» README не затронуты этим PR (раннер — новая способность, не
|
||||
закрытие документированного ограничения) → дополнительных правок не требуется.
|
||||
|
||||
## Findings
|
||||
|
||||
### P0 — Blocker
|
||||
- Нет.
|
||||
|
||||
### P1 — Must fix
|
||||
- Нет.
|
||||
|
||||
### P2 — Should fix
|
||||
- Нет.
|
||||
|
||||
### P3 — Nice-to-have (не блокирует)
|
||||
- [ ] `_http_get` читает только первые 8192 байта тела (`resp.read(8192)`), а smoke проверяет наличие
|
||||
`serial_gate` в ответе `/queue`. Эмпирически `serial_gate` лежит на офсете ~1481 байт (запас ~6.7 КБ
|
||||
до границы) — **сейчас корректно**, но при будущем разрастании блоков `/queue` *перед* `serial_gate`
|
||||
это латентная хрупкость (ложный smoke-FAIL здорового прогона). Опционально: поднять лимит чтения для
|
||||
`/queue` или искать ключ потоково. Не блокирует — текущий запас комфортный.
|
||||
- [ ] `build_test_command` добавляет флаг `-q`, отсутствующий в литеральной команде ADR (`python -m
|
||||
pytest <target>`). Безвредно и согласовано с прежним промптом tester (`pytest tests/ -q`); фиксирую
|
||||
лишь как расхождение текст↔код.
|
||||
|
||||
## Документация
|
||||
Обновлена в полном объёме в том же 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-0049. Анти-дрейф-тесты документации (`test_llm_call_site_
|
||||
inventory.py`, `test_llm_determinization_docs.py`, `test_system_docs.py`) — зелёные. Требований к
|
||||
обновлению документации, оставшихся невыполненными, **нет**.
|
||||
|
||||
## Вердикт
|
||||
Нет P0/P1 findings; критический инвариант NFR-1 соблюдён байт-в-байт; документация = golden source
|
||||
обновлена синхронно с кодом; полный регресс зелёный. **`verdict: APPROVED`.**
|
||||
Reference in New Issue
Block a user