diff --git a/engine/hooks/llm-judge/README.md b/engine/hooks/llm-judge/README.md index fe5ee99b..31eba814 100644 --- a/engine/hooks/llm-judge/README.md +++ b/engine/hooks/llm-judge/README.md @@ -94,6 +94,25 @@ most 300 characters, taken from the end of stderr or the error text. Tests use it to plug in small fake runners. If it is set but not that shape, `ask` raises `ValueError` instead of quietly falling back to the real runners. +## Investigate mode + +A job opts in with `"mode": "investigate"`. It uses a read-only runner set: + +1. **codex**: `codex exec --skip-git-repo-check --sandbox read-only -c notify=[] PROMPT` +2. **claude**: `claude -p --model haiku --settings '{"disableAllHooks": true}' --allowedTools Read Grep Glob --disallowedTools Write Edit NotebookEdit Bash -- PROMPT` + +`cursor-agent` is not used because it has no read-only switch. + +An investigate job may carry `timeout_seconds`. The judge caps it at 600 +seconds. If it is missing or not a number, the runner gets 60 seconds. + +An investigate job may carry `cwd`. The judge uses it only when it is an +absolute path to a folder that exists. Otherwise the runner uses a fresh temp +folder, and `judge.log` gets a line saying the cwd was refused. + +`CATSTACK_LLM_JUDGE_RUNNERS` still replaces the selected runner set. A job that +gets no answer from any runner still comes back `unchecked`. + ## Three outcomes `verdict(job, result)` turns an `ask` result into one of: diff --git a/engine/hooks/llm-judge/judge.py b/engine/hooks/llm-judge/judge.py index a81f19b7..12696205 100644 --- a/engine/hooks/llm-judge/judge.py +++ b/engine/hooks/llm-judge/judge.py @@ -14,6 +14,7 @@ import uuid TIMEOUT_SECONDS = 60 +INVESTIGATE_TIMEOUT_CAP = 600 KILL_GRACE_SECONDS = 5 REASON_LIMIT = 300 PROMPT_SLOT = "{prompt}" @@ -25,6 +26,10 @@ ("claude", ["claude", "-p", "--model", "haiku", "--settings", '{"disableAllHooks": true}', PROMPT_SLOT]), ("cursor", ["cursor-agent", "-p", "--output-format", "text", PROMPT_SLOT]), ) +INVESTIGATE_RUNNERS = ( + ("codex", ["codex", "exec", "--skip-git-repo-check", "--sandbox", "read-only", "-c", "notify=[]", PROMPT_SLOT]), + ("claude", ["claude", "-p", "--model", "haiku", "--settings", '{"disableAllHooks": true}', "--allowedTools", "Read", "Grep", "Glob", "--disallowedTools", "Write", "Edit", "NotebookEdit", "Bash", "--", PROMPT_SLOT]), +) def state_root() -> str: @@ -49,10 +54,11 @@ def valid_runner(entry: object) -> bool: ) -def runners() -> list[tuple[str, list[str]]]: +def runners(mode: object = None) -> list[tuple[str, list[str]]]: + default = INVESTIGATE_RUNNERS if mode == "investigate" else DEFAULT_RUNNERS raw = os.environ.get(RUNNERS_ENV) if not raw: - return [(name, list(argv)) for name, argv in DEFAULT_RUNNERS] + return [(name, list(argv)) for name, argv in default] try: parsed = json.loads(raw) except ValueError as exc: @@ -96,13 +102,26 @@ def stop_group(proc: subprocess.Popen) -> str: return stderr or "" -def run_runner(name: str, argv: list[str], prompt: str) -> tuple[dict, dict | None]: +def bounded_timeout(timeout_seconds: object) -> int | float: + if isinstance(timeout_seconds, bool) or not isinstance(timeout_seconds, (int, float)): + timeout_seconds = TIMEOUT_SECONDS + return min(timeout_seconds, INVESTIGATE_TIMEOUT_CAP) + + +def run_runner(name: str, argv: list[str], prompt: str, timeout_seconds: object = TIMEOUT_SECONDS, cwd: object = None) -> tuple[dict, dict | None]: if shutil.which(argv[0]) is None: return failed(name, "not installed"), None command = [prompt if item == PROMPT_SLOT else item for item in argv] env = dict(os.environ) env[CHILD_ENV] = "1" - with tempfile.TemporaryDirectory(prefix="llm-judge-") as cwd: + timeout = bounded_timeout(timeout_seconds) + with tempfile.TemporaryDirectory(prefix="llm-judge-") as temp_cwd: + runner_cwd = temp_cwd + if cwd is not None: + if isinstance(cwd, str) and os.path.isabs(cwd) and os.path.isdir(cwd): + runner_cwd = cwd + else: + log(f"runner {name}: refused cwd {cwd!r}") try: proc = subprocess.Popen( command, @@ -110,16 +129,16 @@ def run_runner(name: str, argv: list[str], prompt: str) -> tuple[dict, dict | No stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, - cwd=cwd, + cwd=runner_cwd, env=env, start_new_session=True, ) except OSError as exc: return failed(name, clip(type(exc).__name__, str(exc))), None try: - stdout, stderr = proc.communicate(timeout=TIMEOUT_SECONDS) + stdout, stderr = proc.communicate(timeout=timeout) except subprocess.TimeoutExpired: - return failed(name, clip(f"timed out after {TIMEOUT_SECONDS}s", stop_group(proc))), None + return failed(name, clip(f"timed out after {timeout}s", stop_group(proc))), None if proc.returncode != 0: return failed(name, clip(f"exit {proc.returncode}", stderr)), None answer = last_json_object(stdout) @@ -128,10 +147,12 @@ def run_runner(name: str, argv: list[str], prompt: str) -> tuple[dict, dict | No return {"runner": name, "ok": True, "reason": "answered"}, answer -def ask(prompt: str) -> dict: +def ask(prompt: str, mode: object = None, timeout_seconds: object = None, cwd: object = None) -> dict: + if timeout_seconds is None: + timeout_seconds = TIMEOUT_SECONDS attempts = [] - for name, argv in runners(): - attempt, answer = run_runner(name, argv, prompt) + for name, argv in runners(mode): + attempt, answer = run_runner(name, argv, prompt, timeout_seconds=timeout_seconds, cwd=cwd) attempts.append(attempt) if answer is not None: return {"outcome": "answered", "runner": name, "answer": answer, "attempts": attempts} @@ -216,7 +237,7 @@ def run_job(path: str) -> dict: raise ValueError(f"job file holds a JSON {type(loaded).__name__}, not an object") job = dict(loaded) job.setdefault("id", stem) - result = verdict(job, ask(str(job["prompt"]))) + result = verdict(job, ask(str(job["prompt"]), mode=job.get("mode"), timeout_seconds=job.get("timeout_seconds", TIMEOUT_SECONDS), cwd=job.get("cwd"))) except Exception as exc: print(f"catstack-hook-error llm-judge: {type(exc).__name__}: {exc}", file=sys.stderr) log(f"job {job.get('id')} failed: {type(exc).__name__}: {exc}\n{traceback.format_exc()}") diff --git a/engine/hooks/llm-judge/tests/test_judge.py b/engine/hooks/llm-judge/tests/test_judge.py index 08d61c0d..8465f0b2 100644 --- a/engine/hooks/llm-judge/tests/test_judge.py +++ b/engine/hooks/llm-judge/tests/test_judge.py @@ -124,6 +124,90 @@ def test_malformed_runners_env_refuses_instead_of_running_defaults(self): def test_default_runner_order_is_codex_then_claude_then_cursor(self): self.assertEqual([name for name, _ in judge.runners()], ["codex", "claude", "cursor"]) + def test_investigate_runner_argv_is_read_only_and_excludes_cursor(self): + self.assertEqual( + judge.runners("investigate"), + [ + ( + "codex", + [ + "codex", + "exec", + "--skip-git-repo-check", + "--sandbox", + "read-only", + "-c", + "notify=[]", + judge.PROMPT_SLOT, + ], + ), + ( + "claude", + [ + "claude", + "-p", + "--model", + "haiku", + "--settings", + '{"disableAllHooks": true}', + "--allowedTools", + "Read", + "Grep", + "Glob", + "--disallowedTools", + "Write", + "Edit", + "NotebookEdit", + "Bash", + "--", + judge.PROMPT_SLOT, + ], + ), + ], + ) + + def test_investigate_runners_env_replaces_investigate_defaults(self): + custom = ["probe", [PY, "-c", "print('{}')", "{prompt}"]] + self.use_runners(custom) + self.assertEqual(judge.runners("investigate"), [("probe", custom[1])]) + + def test_investigate_job_threads_timeout_and_cwd_to_runner(self): + path = os.path.join(self.state.name, "jobs", "investigate-job.json") + with tempfile.TemporaryDirectory() as cwd: + judge.write_json_atomic(path, self.job(id="investigate-job", mode="investigate", timeout_seconds=123, cwd=cwd)) + calls = [] + + def capture(name, argv, prompt, timeout_seconds=judge.TIMEOUT_SECONDS, cwd=None): + calls.append((name, timeout_seconds, cwd)) + return {"runner": name, "ok": True, "reason": "answered"}, {"match": True} + + with patch.object(judge, "run_runner", side_effect=capture): + result = judge.run_job(path) + + self.assertEqual(result["outcome"], "hit") + self.assertEqual(calls, [("codex", 123, cwd)]) + + def test_investigate_timeout_is_capped_at_600_seconds(self): + self.assertEqual(judge.bounded_timeout(999), 600) + + def test_non_investigate_job_still_gets_default_timeout_and_empty_temp_cwd(self): + self.use_runners(runner("env", "import json, os; print(json.dumps({'cwd': os.getcwd(), 'entries': os.listdir('.')}))")) + path = os.path.join(self.state.name, "jobs", "default-job.json") + judge.write_json_atomic(path, self.job(id="default-job")) + calls = [] + original = judge.run_runner + + def capture(name, argv, prompt, timeout_seconds=judge.TIMEOUT_SECONDS, cwd=None): + calls.append((timeout_seconds, cwd)) + return original(name, argv, prompt, timeout_seconds=timeout_seconds, cwd=cwd) + + with patch.object(judge, "run_runner", side_effect=capture): + result = judge.run_job(path) + + self.assertEqual(calls, [(judge.TIMEOUT_SECONDS, None)]) + self.assertEqual(result["answer"]["entries"], []) + self.assertFalse(os.path.exists(result["answer"]["cwd"])) + class TestVerdict(JudgeTestCase): def test_hit_when_every_hit_key_is_true(self): diff --git a/product/skills/admin-bypass-sweep/SKILL.md b/product/skills/admin-bypass-sweep/SKILL.md index becb9df3..70ec49a7 100644 --- a/product/skills/admin-bypass-sweep/SKILL.md +++ b/product/skills/admin-bypass-sweep/SKILL.md @@ -141,10 +141,43 @@ diffs; stop and re-derive the safe approach first. Both requirements in the STOP section must be satisfied before this step runs. -Skim `gh pr diff ` for each PR before merging it, even under consent — -this is the only review most of these PRs get, since the merge bypasses -required checks entirely. The human's consent authorizes bypassing CI; it -does not stand in for having actually looked at what's being merged. +Before any `gh pr merge --admin`, write that PR's diff to a file, record +the file's total line count with `wc -l`, read the whole file, and assert +that the number of lines read equals the recorded total. This is the only +review most of these PRs get, since the merge bypasses required checks +entirely. The human's consent authorizes bypassing CI; it does not stand in +for having actually looked at what's being merged. + +Use this read-completeness precondition for each PR: + +```bash +pr= +diff_file="$(mktemp -t admin-bypass-pr-${pr}.diff.XXXXXX)" +gh pr diff "$pr" --repo / > "$diff_file" +diff_lines="$(wc -l < "$diff_file" | tr -d ' ')" +nl -ba "$diff_file" +lines_read= +test "$lines_read" = "$diff_lines" +``` + +Classify the PR before merging it, using the same three outcomes as the +rest of this procedure: + +- `reviewed` — the diff file was read line-complete, `lines_read` equals + `diff_lines`, and no review finding was found. +- `flagged` — the diff file was read line-complete, `lines_read` equals + `diff_lines`, and one or more review findings were recorded for the + human. +- `unchecked` — the diff read was narrowed, filtered, or truncated, so + line completeness was not established. + +A read through `head`, `tail`, `grep`, `awk`, or `sed` is a narrowed read: +mark that PR `unchecked`, never `reviewed`, even if the visible lines look +fine. The operator may not report such a PR as reviewed. This check reports +the PR's review state; it does not create a new authorization path or block +the human from deciding to proceed with the admin merge anyway. If an +`unchecked` PR is merged, the final report must still list it as +`unchecked`, not reviewed. For a single-PR stack: @@ -193,32 +226,27 @@ human: These are mechanically distinguishable, and only the first one is the "real conflict" Step 5 means. Before recording a `CONFLICTING` PR as -blocked, check which case it is, in a disposable worktree outside the -human's main checkout — never touch their primary working tree's branch or -uncommitted state to do this: +blocked, run the rebase probe outside the human's main checkout — never +touch their primary working tree's branch or uncommitted state to do this: ```bash git fetch origin master -git worktree add /tmp//pr- origin/ -cd /tmp//pr- -git checkout -b fix/pr--rebase -git rebase origin/master +scripts/probe_branch_rebase.sh origin/ origin/master ``` -- **Rebase applies clean** (no conflict markers, `git status` clean) — this - was stale mergeability, not a real conflict. Force-push the rebased - branch back to the PR's head with `--force-with-lease` pinned to the - known old SHA, wait for GitHub to recompute (`sleep 5`), confirm - `mergeable` now reads `MERGEABLE`, then continue this PR (and its - children) through Step 4 as normal. -- **Rebase stops with conflict markers** — this is Step 5's real-conflict - case. Run `git rebase --abort`, remove the scratch worktree, and follow - Step 5 as written: stop the chain, record as blocked, move on. Do not - attempt to resolve the markers by picking a side — that part of Step 5 - still applies. - -Remove the scratch worktree (`git worktree remove --force`) once the PR's -fate — merged or genuinely blocked — is decided. +- **Exit 0, `OK`** — the PR rebases cleanly onto current master, so this was + stale mergeability rather than Step 5's real-conflict case. Wait for + GitHub to recompute (`sleep 5`), confirm `mergeable` now reads + `MERGEABLE`, then continue this PR (and its children) through Step 4 as + normal. +- **Exit 1, `FAIL`** — the probe ran and found a real content conflict. + Follow Step 5 as written: stop the chain, record as blocked, and move on. + Do not attempt to resolve the markers by picking a side — that part of + Step 5 still applies. +- **Exit 3, `UNCHECKED`** — the probe could not run, so nothing is proven. + Do not treat this as stale mergeability and do not treat it as Step 5's + real-conflict case; report the unchecked PR separately for manual retry + or setup repair. ## Step 6: Prove the final state diff --git a/product/skills/admin-bypass-sweep/tests/fires_step5a_names_probe.md b/product/skills/admin-bypass-sweep/tests/fires_step5a_names_probe.md new file mode 100644 index 00000000..f5767180 --- /dev/null +++ b/product/skills/admin-bypass-sweep/tests/fires_step5a_names_probe.md @@ -0,0 +1,4 @@ +Fixture assertion: the Step 5a passage in +`product/skills/admin-bypass-sweep/SKILL.md` must name +`scripts/probe_branch_rebase.sh`, must name all three probe outcomes +`OK`, `FAIL`, and `UNCHECKED`, and must not contain `rm -rf`. diff --git a/product/skills/admin-bypass-sweep/tests/fixture_complete_diff_read.md b/product/skills/admin-bypass-sweep/tests/fixture_complete_diff_read.md new file mode 100644 index 00000000..b3baba78 --- /dev/null +++ b/product/skills/admin-bypass-sweep/tests/fixture_complete_diff_read.md @@ -0,0 +1,20 @@ +User invokes `/admin-bypass-sweep` with the required consent sentence and the +operator reaches Step 4 for PR 42. + +The operator writes the full diff to a file: + +```bash +pr=42 +diff_file="$(mktemp -t admin-bypass-pr-${pr}.diff.XXXXXX)" +gh pr diff "$pr" --repo neko/example > "$diff_file" +diff_lines="$(wc -l < "$diff_file" | tr -d ' ')" +nl -ba "$diff_file" +lines_read=184 +test "$lines_read" = "$diff_lines" +``` + +The recorded `diff_lines` value is 184, and the complete `nl -ba` output ends +at line 184. No review finding is found. + +Expected outcome: PR 42 may be reported as `reviewed` before the operator runs +`gh pr merge 42 --repo neko/example --admin --squash`. diff --git a/product/skills/admin-bypass-sweep/tests/fixture_truncated_diff_read.md b/product/skills/admin-bypass-sweep/tests/fixture_truncated_diff_read.md new file mode 100644 index 00000000..3c1a1646 --- /dev/null +++ b/product/skills/admin-bypass-sweep/tests/fixture_truncated_diff_read.md @@ -0,0 +1,23 @@ +User invokes `/admin-bypass-sweep` with the required consent sentence and the +operator reaches Step 4 for PR 77. + +The operator writes the diff to a file, but reads it through a truncating +filter: + +```bash +pr=77 +diff_file="$(mktemp -t admin-bypass-pr-${pr}.diff.XXXXXX)" +gh pr diff "$pr" --repo neko/example > "$diff_file" +diff_lines="$(wc -l < "$diff_file" | tr -d ' ')" +head -200 "$diff_file" +lines_read=200 +test "$lines_read" = "$diff_lines" +``` + +The recorded `diff_lines` value is 913, and only the first 200 lines were read +through `head`. + +Expected outcome: PR 77 is `unchecked`, not `reviewed`. The operator may not +report PR 77 as reviewed, even if the visible lines look fine. If the human +decides to proceed with `gh pr merge 77 --repo neko/example --admin --squash`, +the final report must still list PR 77 as `unchecked`. diff --git a/product/skills/admin-bypass-sweep/tests/test_diff_read_gate.py b/product/skills/admin-bypass-sweep/tests/test_diff_read_gate.py new file mode 100644 index 00000000..254c27e1 --- /dev/null +++ b/product/skills/admin-bypass-sweep/tests/test_diff_read_gate.py @@ -0,0 +1,60 @@ +from pathlib import Path +import re +import unittest + + +ROOT = Path(__file__).resolve().parents[1] +SKILL = ROOT / "SKILL.md" +TESTS = ROOT / "tests" + + +class DiffReadGateTests(unittest.TestCase): + def setUp(self): + self.skill = SKILL.read_text() + step4_match = re.search( + r"## Step 4: Merge each stack, bottom-up(?P.*?)## Step 5:", + self.skill, + re.S, + ) + self.assertIsNotNone(step4_match, "Step 4 section is present") + self.step4 = step4_match.group("body") + + def test_step4_requires_recorded_line_count_and_full_read_before_admin_merge(self): + merge_index = self.step4.index("gh pr merge ") + pre_merge = self.step4[:merge_index] + + self.assertIn("Before any `gh pr merge --admin`", pre_merge) + self.assertIn("gh pr diff", pre_merge) + self.assertIn("> \"$diff_file\"", pre_merge) + self.assertIn("wc -l", pre_merge) + self.assertIn("nl -ba \"$diff_file\"", pre_merge) + self.assertIn('test "$lines_read" = "$diff_lines"', pre_merge) + self.assertIn("number of lines read equals the recorded total", pre_merge) + + def test_step4_documents_narrowed_reads_as_unchecked_not_reviewed(self): + for command in ("head", "tail", "grep", "awk", "sed"): + self.assertIn(f"`{command}`", self.step4) + + self.assertIn("mark that PR `unchecked`, never `reviewed`", self.step4) + self.assertIn("may not report such a PR as reviewed", self.step4) + + def test_complete_read_fixture_marks_pr_reviewed(self): + fixture = (TESTS / "fixture_complete_diff_read.md").read_text() + + self.assertIn("wc -l", fixture) + self.assertIn("nl -ba", fixture) + self.assertIn("lines_read=184", fixture) + self.assertIn("Expected outcome: PR 42 may be reported as `reviewed`", fixture) + + def test_truncated_read_fixture_marks_pr_unchecked(self): + fixture = (TESTS / "fixture_truncated_diff_read.md").read_text() + fixture_lower = fixture.lower() + + self.assertIn("head -200", fixture) + self.assertIn("diff_lines` value is 913", fixture) + self.assertIn("PR 77 is `unchecked`, not `reviewed`", fixture) + self.assertIn("operator may not\nreport pr 77 as reviewed", fixture_lower) + + +if __name__ == "__main__": + unittest.main() diff --git a/product/skills/admin-bypass-sweep/tests/test_step5a_probe_passage.py b/product/skills/admin-bypass-sweep/tests/test_step5a_probe_passage.py new file mode 100644 index 00000000..ba7aabef --- /dev/null +++ b/product/skills/admin-bypass-sweep/tests/test_step5a_probe_passage.py @@ -0,0 +1,38 @@ +#!/usr/bin/env python3 +from __future__ import annotations + +import re +import unittest +from pathlib import Path + +SKILL_DIR = Path(__file__).resolve().parents[1] +SKILL = (SKILL_DIR / "SKILL.md").read_text(encoding="utf-8") +FIXTURE = (SKILL_DIR / "tests" / "fires_step5a_names_probe.md").read_text(encoding="utf-8") + + +def step5a_passage() -> str: + match = re.search(r"## Step 5a:.*?(?=\n## Step 6:)", SKILL, re.S) + if not match: + raise AssertionError("Step 5a passage not found") + return match.group(0) + + +class TestStep5aProbePassage(unittest.TestCase): + def test_fixture_names_the_assertions(self): + self.assertIn("scripts/probe_branch_rebase.sh", FIXTURE) + for outcome in ("OK", "FAIL", "UNCHECKED"): + self.assertIn(outcome, FIXTURE) + self.assertIn("rm -rf", FIXTURE) + + def test_step5a_names_probe_and_all_three_outcomes(self): + passage = step5a_passage() + self.assertIn("scripts/probe_branch_rebase.sh", passage) + for outcome in ("OK", "FAIL", "UNCHECKED"): + self.assertRegex(passage, rf"`{outcome}`") + + def test_step5a_no_longer_contains_improvised_removal(self): + self.assertNotIn("rm -rf", step5a_passage()) + + +if __name__ == "__main__": + unittest.main()