feat(runner): subprocess-isolated verifier + typed fail-loud run stubs (#55 phase 1) - #72
Conversation
#55 phase 1) Replace the placeholder default verifier with a real subprocess-isolated verifier: shlex-parsed argv, shell=False, cwd=workspace, 300s wall-clock timeout, errors=replace decoding. Exit 0 -> VerifyOutcome(True); nonzero or timeout -> VerifyOutcome(False, bounded 2000-char tail) - never an exception. Unlaunchable or unparseable verify commands raise the new typed VerifierExecutionError (RunnerError subclass -> CLI exit 2, no traceback, zero workspace writes). VerifierNotImplementedError narrows to missing/blank TASKS.json verify fields. New --continuous/--approve run flags refuse with typed RunModeNotImplementedError before mode validation, target checks, and any event-store access. 16 new tests in scripts/test_runner_verifier.py + 3 hardening regressions (malformed shlex string, ENOEXEC-class launch failure, non-UTF8 output); 2 default-verifier fixtures in test_runner_dispatch.py now declare blank verify commands. Kernel modules byte-unchanged. Refs #55
There was a problem hiding this comment.
Pull request overview
This PR advances the loop run command toward Phase 1 of issue #55 by implementing a subprocess-isolated default task verifier, adding typed “fail-loud” errors for unimplemented run modes, and expanding regression coverage to validate crash-safety and refusal semantics.
Changes:
- Replace the default verifier with
_subprocess_verifier()that usesshlex.split()+subprocess.run(shell=False, cwd=workspace, timeout=...)and returnsVerifyOutcomeon exit/timeout. - Introduce typed runner errors (
VerifierExecutionError,VerifierNotImplementedError,RunModeNotImplementedError) and wire CLI behavior to exit 2 with a typed message (no traceback). - Add a new regression test module covering verifier isolation, timeout behavior, typed refusal paths, and crash-injection retry semantics; update existing dispatch tests/messages accordingly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/test_runner_verifier.py | Adds end-to-end and hardening regression tests for the subprocess verifier and typed refusal paths. |
| scripts/test_runner_dispatch.py | Updates existing runner/CLI tests to reflect the new “no verify command declared” behavior. |
| reference/repo-os-contract.md | Documents the verifier subprocess isolation boundary and typed refusal cases. |
| loop/runner.py | Implements _subprocess_verifier, adds typed errors, and routes the default verifier through the subprocess implementation. |
| loop/main.py | Adds early typed refusal for unimplemented run stub flags (--continuous, --approve) before mode/target validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except subprocess.TimeoutExpired: | ||
| return VerifyOutcome(False, summary=f"verify command timed out after {_VERIFY_TIMEOUT_SECONDS}s") |
| run_id, projection = _projection(target, mode) | ||
| # Safe only because each dispatch_once invocation appends at most one event. | ||
| if projection.get("terminal") is not None: | ||
| _reconcile_legacy_terminal(target, projection) | ||
| return {"ok": True, "action": "noop_terminal", "run_id": run_id} |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f9ac86cb2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| raise VerifierExecutionError(f"cannot parse verify command {cmd!r}: {exc}") from exc | ||
| try: | ||
| proc = subprocess.run( | ||
| argv, cwd=str(workspace), shell=False, timeout=_VERIFY_TIMEOUT_SECONDS, |
There was a problem hiding this comment.
When a verifier script starts a subprocess and then waits, for example sleep 999 & wait, the timeout here only terminates the direct child process; the grandchild remains running after _subprocess_verifier returns a failed outcome. That leaves orphaned test servers/workers around to contaminate later dispatches, so the timeout is not a reliable isolation boundary unless the verifier is started in its own process group/session and that group is killed on timeout.
Useful? React with 👍 / 👎.
| ) | ||
| except subprocess.TimeoutExpired: | ||
| return VerifyOutcome(False, summary=f"verify command timed out after {_VERIFY_TIMEOUT_SECONDS}s") | ||
| except OSError as exc: |
There was a problem hiding this comment.
When a TASKS.json command contains an escaped NUL, such as "verify": "echo \\u0000", shlex.split succeeds and the launch raises ValueError: embedded null byte, not OSError; because this handler only wraps OSError, loop run bypasses the top-level RunnerError handling and prints a traceback with exit 1 instead of the typed verifier error/exit 2 expected for malformed declarations.
Useful? React with 👍 / 👎.
| counter = tmp_path / "counter" | ||
| command = _cmd(_script(tmp_path, "pass.py", f"from pathlib import Path\np=Path({str(counter)!r})\np.write_text(str(int(p.read_text()) + 1) if p.exists() else '1')\n")) | ||
| workspace, store = _ws(tmp_path, [_task(command)]) | ||
| body = "import os,signal,sqlite3,sys\nreal=sqlite3.connect\nclass Kill(sqlite3.Connection):\n def execute(self,sql,*a,**kw):\n if isinstance(sql,str) and sql.strip().upper()=='COMMIT': os.kill(os.getpid(),signal.SIGKILL)\n return super().execute(sql,*a,**kw)\nsqlite3.connect=lambda *a,**kw: real(*a,factory=Kill,**kw)\nfrom loop.runner import dispatch_once\ndispatch_once(sys.argv[1])\n" |
There was a problem hiding this comment.
When the tests run from an uninstalled checkout, this generated tmp script has sys.path[0] set to tmp_path, and cwd=ROOT in the subprocess call does not make the loop package importable; the test exits with ModuleNotFoundError before exercising crash recovery. The existing crash helper in scripts/test_runner_dispatch.py inserts os.getcwd(), so this helper needs the same setup or an explicit PYTHONPATH.
Useful? React with 👍 / 👎.
…(CI has no installed package)
Summary
S3b of the phase3 run — completes #55 phase 1 (Refs, deliberately not Closes): the
loop runverb gains a real subprocess-isolated verifier and typed fail-loud stubs for the not-yet-implemented run modes._subprocess_verifier: shlex-parsed argv,shell=False,cwd=workspace, 300s wall-clock timeout,errors="replace"decoding. Exit 0 →VerifyOutcome(True); nonzero/timeout →VerifyOutcome(False, bounded 2000-char tail)— never an exception.VerifierExecutionError(RunnerErrorsubclass): unparseable verify strings (shlexValueError) and unlaunchable commands (any launchOSError, incl. ENOEXEC) → CLI exit 2, no traceback, zero workspace writes.VerifierNotImplementedErrornarrows to missing/blankTASKS.jsonverify fields ("no verify command declared…").--continuous/--approverefuse with typedRunModeNotImplementedErrorbefore mode validation, target checks, and any event-store access.reference/repo-os-contract.md§16 documents the verifier isolation boundary.events/reducer/evidence/fsm/completion/contract/scaffold/paths/plan/runtime/emit) byte-unchanged.Governed-lane evidence
s3b-verifier(attempt 1 honest stop on a real packet contradiction → adjudicated; attempt 2 gates-green but review BLOCKER →failed_verification) + replacement packets3b-verifier-b(sha256 delta-gated directed 3-finding repair) — receiptscx_s3b_verifier_a1/cx_s3b_verifier_a2/cx_s3b_verifier_b1(accepted; codex sessions 019f673b / 019f67d6 / 019f67e5).ValueErrorblocker, governor-reproduced live) + post-repair adversarial exhaustiveness pass — PASS, zero findings (per-statement exception enumeration of_subprocess_verifier;argv==[]proven unreachable;TimeoutExpired/OSErrorclause interplay verified).events.dbtamper refused by the append-only triggers; wall-clock timeout on a zero-CPU I/O-blocked child; 5MB stdout bounded to exactly 2000 chars; out-of-workspace verify runs as declared (containment is deliberatelyevidence@1's boundary, not the runner's); combined--continuous --approve --mode stricton a nonexistent target → exactly one typed refusal.Test plan
Refs #55