automaton/tasks/add-loop-runner/CODE_REVIEW.md

CODE_REVIEW: add-loop-runner

Reviewed against SPEC.md R1–R8.

R1–R8 checklist

Req	Status	Notes
R1 entrypoint	✅	argparse --mode required choices; cmd_tick returns summary dict, never raises; exits 0 on unknown loop
R2 tick flow	✅	11 steps match technical.md §7 precisely
R3 daemon	✅	cmd_daemon loops on cmd_tick + time.sleep; KeyboardInterrupt = DAEMON_STOPPED; --max-iterations honored
R4 harness substitution	✅	_substitute handles 7 tokens; missing tokens left literal; default command matches D8 (opencode)
R5 context-floor guard	✅	_context_floor_ok before any Implement call; halts human_intervention on loop_mode_eligible=False; best-effort allows tick if vram_detect itself unavailable
R6 idempotence	✅	state writes only after verdict parse + orchestrator both succeed; pre-step-10 crashes leave .state.loop untouched
R7 tests	✅	18 tests, 7 classes; all subprocess stubbed
R8 out-of-scope	✅	audit/backlog/worktree-creation/prompts deferred to tasks 4–7

Edge cases checked

1. Subprocess failure in --check-gate — _run_json returns None, cmd_tick skips with gate_subprocess_failed. No crash. ✅
2. Subprocess failure in vram_detect --loop-mode — best-effort allows tick (avoids a broken vram_detect tool from halting every loop on a platform where it isn't installed). ✅
3. Empty verifier stdout — parse_verdict returns None; cmd_tick halts verifier_failed without advancing state. ✅
4. Fenced JSON verdict — handled by _FENCE_RE regex, tries fenced body before raw text. ✅
5. Line-commented JSON verdict — stripped by _strip_comments. ✅
6. Missing pass key — parse_verdict requires it; returns None. ✅
7. Score history shorter than window — no capping until length > window; oldest dropped. ✅
8. No roles configured in loop.json — _role_prompt returns None or ""; harness gets empty prompt-path token. User's config responsibility; runtime refuses on empty cwd (Path resolve) if _find_project_dir fails. ✅
9. Worktree declared but missing — runner uses project_root as cwd and logs nothing (per R8 deferred to task 5). ✅
10. KeyboardInterrupt mid-tick — bubbles up; no state write happens; next tick starts fresh. ✅
11. KeyboardInterrupt in daemon mode — _append_tick_log(DAEMON_STOPPED) then exit 0. ✅

Code-quality observations

1. _run_json parses the last stdout line only — correct for --check-gate --json (last-line contract per AGENTS.md), but assumes the harness never emits JSON mid-session. For the harness-substitution roles (Implement/Verify/Orchestrate), the runner captures full stdout (not _run_json), so the constraint only applies to --check-gate and vram_detect --loop-mode. Safe.
2. Token substitution is string-only — {verdict} gets json.dumps(verdict). Not shell-escaped. The harness command is parsed with shlex by opencode's own runner; subprocess.run with list argv means no shell injection. Safe as long as harness.command stays list-typed (it does — the cfg loader rejects non-list commands via the if not command: command = [...default...] fallback). ✅
3. No timeout on harness invocations — explicitly per SPEC ("v1 has no timeout; harness owns its timeout policy"). Fine. Worth revisiting if a loop's harness hangs and the OS unit keeps scheduling — but the scheduler interval provides natural rate-limiting.
4. _invoke_harness passes cwd=cwd to subprocess.run — if cwd doesn't exist, subprocess.run raises FileNotFoundError. Caught by the outer except (OSError, subprocess.SubprocessError) which emits stderr and returns empty — fine. ✅
5. _read_state_loop swallows JSONDecodeError — returns None. Caller treats as untracked. A corrupt .state.loop becomes an untracked loop. Acceptable for v1; --audit flags untracked. ✅
6. _write_state_loop uses tmp.replace(f) atomic write — same pattern as status.py; crash-safe. ✅

Verdict

APPROVE. Ready for bug_find.