reviewer(ET): auto-commit from reviewer run_id=754
This commit is contained in:
@@ -8,7 +8,7 @@ created_at: 2026-06-16
|
||||
model_used: claude-opus-4-8
|
||||
type: review
|
||||
work_item_id: ORCH-116
|
||||
version: 2
|
||||
version: 3
|
||||
---
|
||||
|
||||
# Review ORCH-116 — детерминированный test-раннер вместо LLM-тестера
|
||||
@@ -17,101 +17,117 @@ version: 2
|
||||
|
||||
PR заменяет LLM-агента `tester` на стадии `testing` (self-hosting `orchestrator`) детерминированным
|
||||
leaf `src/test_runner.py`, перехватываемым в `launch_job` **до `_spawn`** — точное зеркало уже
|
||||
работающего в проде ORCH-115/`staging_runner`. Проверено независимо по всем четырём осям
|
||||
(соответствие ТЗ/AC, соответствие ADR, качество кода, документация) с самостоятельным прогоном
|
||||
тестов. **Все 15 критериев приёмки выполнены**; критический инвариант NFR-1 («замена *продюсера*
|
||||
артефакта, не гейта») соблюдён байт-в-байт (только 4 файла в `src/`, `stages.py`/`qg/checks.py` не
|
||||
тронуты); документация = golden source обновлена в том же PR. Полный регресс **проверен лично**:
|
||||
`pytest tests/ -q` → **2137 passed**; ORCH-116 + LLM-анти-дрейф + system-docs → 74 passed. **P0/P1
|
||||
findings отсутствуют → `APPROVED`.**
|
||||
работающего в проде 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-0049). Полный регресс
|
||||
**проверен лично**: `pytest tests/ -q` → **2137 passed**; ORCH-116 + LLM-анти-дрейф + system-docs →
|
||||
**74 passed**. **P0/P1/P2 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-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, лично подтверждено).
|
||||
+ полную 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 проверяет `agent ==
|
||||
"tester"`), не код-фейл-откат, не жжёт developer-retry.
|
||||
`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` лично); новых таблиц нет (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).
|
||||
`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 (TC-13).
|
||||
- **AC-11** (proc_group/worktree/timeout) — pytest в `get_worktree_path` через
|
||||
`run_in_process_group`, `_resolve_timeout` дефолтит на малформе (TC-12).
|
||||
рестарта/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-карты/политики/витрины, полный регресс — все зелёные (AC-15 проверен личным прогоном).
|
||||
LLM-карты/политики/roadmap/витрины, полный регресс — все зелёные (AC-15: 2137 passed, проверено
|
||||
лично).
|
||||
|
||||
### 2. Соответствие ADR — PASS
|
||||
|
||||
Реализация дословно следует ADR-001 (D1–D12) и сквозному adr-0049. Ключевые места сверены:
|
||||
- **D6.1 (анти-коллизия 52c-`status:` ↔ `_parse_tests_verdict`)** — корректно и проверено против
|
||||
**неизменённого** парсера (`src/qg/checks.py:263-277`): `result: PASS → status: success`
|
||||
(конкатенация `"SUCCESS PASS"` — нет негативного токена, позитив `PASS` из `result:` → `True`);
|
||||
`result: FAIL → status: failed` (`"FAILED FAIL"` — негативный токен → `False`). В `build_test_report`
|
||||
жёстко: `sub_status = "success" if result == "PASS" else "failed"`. Прибито
|
||||
TC-04/`test_tc04_status_field_never_false_negatives_a_pass`. Самая тонкая мина задачи — снята верно.
|
||||
Реализация дословно следует ADR-001 (D1–D12) и сквозному adr-0049. Сверено критическое:
|
||||
- **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`) — проверено TC-10. Счётчик restart-safe по маркеру `test-runner infra-retry` в
|
||||
`task_content`, без правки схемы БД.
|
||||
- **Трассировка маркеров (TRACEABILITY):** правки рядом с ORCH-115 (`launcher`), ORCH-114
|
||||
`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», запрет рестарта прод-контейнера) — соблюдены: лог
|
||||
пушится только в фичеветку, никакой работы с `main`.
|
||||
- **Глобальные ADR** (INV-4 «мерж только через PR», запрет рестарта прод-контейнера) — соблюдены:
|
||||
лог пушится только в фичеветку (`git push origin <branch>`, не force, не `main`); смоук read-only.
|
||||
|
||||
### 3. Качество кода — PASS
|
||||
|
||||
- Docstrings на всех публичных функциях; контракт never-raise выдержан сквозно (каждый внешний вызов
|
||||
обёрнут; наблюдательные счётчики через `_bump` не ломают решение).
|
||||
- Тесты содержательные (32 тест-функции, не тривиальные): kill-switch/scope/contract, маппинг по
|
||||
классам входа, рендер+парсер, перехват, two-level outcome, never-raise, timeout, smoke read-only
|
||||
(включая транзиент-ретрай и missing-serial_gate), анти-дрейф, наблюдаемость.
|
||||
- **Docstrings** на всех публичных функциях; контракт **never-raise** выдержан сквозно (каждый
|
||||
внешний вызов обёрнут; счётчики через `_bump` не ломают решение).
|
||||
- **Сигнатуры зависимостей сверены лично** (риск latent false-FAIL устранён):
|
||||
`db.enqueue_job(..., task_id=, available_at_delay_s=)`, `db.mark_job`,
|
||||
`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`) —
|
||||
все существуют ровно как используются.
|
||||
- Security: smoke — только `urllib` GET к локальному оркестратору; мутирующих запросов нет.
|
||||
- Это **не** багфикс-трек (стадия `architecture` отработана, full-cycle) → требование
|
||||
регресс-фиксатора ORCH-019/BR-4 неприменимо; тем не менее покрытие исчерпывающее.
|
||||
`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 не применяется; покрытие тем не менее
|
||||
исчерпывающее.
|
||||
|
||||
### 4. Документация — PASS (обязательная проверка)
|
||||
|
||||
`src/` изменён → документация обновлена **в том же PR**, проверено явно по `git diff`:
|
||||
- **Паспорт** `CLAUDE.md` — новый раздел «Детерминированный test-раннер (ORCH-116)».
|
||||
- **Компонент-карта** `docs/architecture/README.md` — запись **Test-runner** + блок roadmap «второй
|
||||
срез реализован».
|
||||
- **Внутренности** `docs/architecture/internals.md` — примечание о перехвате на `testing`.
|
||||
`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 ✅), `llm-usage-policy.md` (§5); инвариант «ровно один `first_slice=yes`» цел (у rank 1
|
||||
deployer); анти-дрейф-тесты `test_llm_call_site_inventory.py`/`test_llm_determinization_docs.py` —
|
||||
зелёные (лично прогнаны).
|
||||
(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` зелёный (лично прогнан).
|
||||
`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`).
|
||||
fallback под выключенным флагом / для репо без контракта»; канон 52d (5 секций, ключ `result:`) цел.
|
||||
- **`CHANGELOG.md`** — развёрнутая запись `[Unreleased]`.
|
||||
- **ADR** заведён (`06-adr/ADR-001-deterministic-test-runner.md` + сквозной
|
||||
`adr/adr-0049-deterministic-test-runner.md`).
|
||||
|
||||
Обзорные доки / «Известные ограничения» README не затронуты этим PR (раннер — новая способность, не
|
||||
закрытие документированного ограничения) → дополнительных правок не требуется.
|
||||
Обзорные доки / README «Известные ограничения» (ORCH-079): раннер — **новая способность**, не
|
||||
закрытие документированного ограничения → дополнительных правок README не требуется.
|
||||
|
||||
## Findings
|
||||
|
||||
@@ -125,20 +141,20 @@ findings отсутствуют → `APPROVED`.**
|
||||
- Нет.
|
||||
|
||||
### P3 — Nice-to-have (не блокирует)
|
||||
- [ ] `_http_get` читает только первые 8192 байта тела (`resp.read(8192)`), а smoke проверяет наличие
|
||||
`serial_gate` в ответе `/queue`. `serial_gate` — 11-й ключ ответа (`src/main.py:261`), идёт **до**
|
||||
основной массы аддитивных блоков (coverage/fs_ownership/auto_labels/stop/bug_fast_track/
|
||||
checkout_hygiene/transition_lease/staging_runner/test_runner/lessons) → запас комфортный, **сейчас
|
||||
корректно**. Латентная хрупкость при разрастании ранних блоков — опционально поднять лимит чтения
|
||||
для `/queue` или искать ключ потоково. Не блокирует.
|
||||
- [ ] `build_test_command` добавляет флаг `-q`, отсутствующий в литеральной команде ADR (`python -m
|
||||
pytest <target>`). Безвредно и согласовано с прежним промптом tester (`pytest tests/ -q`); фиксирую
|
||||
лишь как расхождение текст↔код.
|
||||
- [ ] `_http_get` читает первые 8192 байта тела; smoke ищет `serial_gate` в ответе `/queue`. **Лично
|
||||
измерено:** `serial_gate` на смещении **1471 байт** при полном ответе ~3.6 КБ → запас огромный,
|
||||
**сейчас корректно**. Латентная хрупкость лишь при экстремальном разрастании ранних блоков; можно
|
||||
опционально поднять лимит чтения для `/queue` или искать ключ потоково.
|
||||
- [ ] `build_test_command` добавляет `-q`, отсутствующий в литеральной команде ADR/CHANGELOG (`python
|
||||
-m pytest <target>`). Безвредно (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 и
|
||||
Это исход стадии **deploy-staging** (downstream от review), относится к staging-раннеру ORCH-115 и
|
||||
является **инфра-сбоем, а не дефектом кода ORCH-116** — вне осей код-ревью. Поведение гейта
|
||||
корректно (fail-closed на исчерпании инфра-бюджета); правок в коде задачи не требует. Отмечено как
|
||||
контекст для последующих стадий.
|
||||
@@ -149,9 +165,9 @@ findings отсутствуют → `APPROVED`.**
|
||||
`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 соблюдён байт-в-байт (`stages.py`/`qg/checks.py` не
|
||||
тронуты); документация = golden source обновлена синхронно с кодом; полный регресс зелёный лично
|
||||
проверен (2137 passed). **`verdict: APPROVED`.**
|
||||
Нет P0/P1/P2 findings; критический инвариант NFR-1 соблюдён байт-в-байт (`stages.py`/`qg/checks.py`
|
||||
не тронуты — проверено лично); документация = golden source обновлена синхронно с кодом; полный
|
||||
регресс зелёный лично проверен (2137 passed). **`verdict: APPROVED`.**
|
||||
|
||||
Reference in New Issue
Block a user