diff --git a/src/agent_harness/session_executor.py b/src/agent_harness/session_executor.py index 7a1687f..52e05fd 100644 --- a/src/agent_harness/session_executor.py +++ b/src/agent_harness/session_executor.py @@ -634,7 +634,7 @@ def _execute(self, record: WorkRecord) -> Outcome: record, "draft_pr_opened", detail=outcome.pr_url, session_id=session.id ) - verdict_text = self._review(record, tree, True, "") + verdict_text = self._review(record, tree, True, "", base=base) outcome.stages.append("review") verdict = APPROVED if verdict_text.strip().upper().startswith("APPROVED") else REJECTED outcome.verdict = verdict @@ -744,13 +744,26 @@ def _on_waiting(self, record: WorkRecord, session: Session) -> None: url=url, ) - def _review(self, record: WorkRecord, tree: Path, passed: bool, failure: str) -> str: + def _review( + self, record: WorkRecord, tree: Path, passed: bool, failure: str, base: str = "" + ) -> str: if self.reviewer is None: # Checked before the diff is computed: there is no point building # a diff nobody will read. Says so rather than silently treating # unreviewed work as approved. return "REJECTED\nNo reviewer is configured, so nothing has reviewed this." - diff = run_git(tree, "diff", "HEAD") + # Against the BASE, not the working tree. `git diff HEAD` answers + # "what is uncommitted?", and by the time a reviewer is called the + # answer is always "nothing": the checkpoint before the expensive + # gate has just committed all of it. So every session-mode reviewer + # was shown an empty diff, and said the only correct thing about one: + # + # "The supplied diff is empty, so it demonstrates none of that and + # cannot be judged as satisfying the request." + # + # Measured on a real 48-line change that had already passed its + # checks. No session-mode item could ever have been approved. + diff = run_git(tree, "diff", f"{base}...HEAD") if base else run_git(tree, "diff", "HEAD") prompt = REVIEW_PROMPT.format( brief=record.brief, diff=diff[:20000], diff --git a/tests/test_reviewer.py b/tests/test_reviewer.py index e31c2b8..dce9bcd 100644 --- a/tests/test_reviewer.py +++ b/tests/test_reviewer.py @@ -98,3 +98,73 @@ def test_the_prompt_demands_evidence_not_just_a_verdict() -> None: assert "What I could not verify" in REVIEW_PROMPT assert "Assume it is wrong" in REVIEW_PROMPT assert "cannot name any, that is a REJECTED" in REVIEW_PROMPT + + +def test_the_reviewer_sees_the_work_after_the_checkpoint_committed_it(tmp_path: Any) -> None: + """The measured bug: every session-mode reviewer was shown an empty diff. + + The checkpoint before the expensive gate commits the work — deliberately, + so a worker killed during review does not lose what passed the cheap + gates. `git diff HEAD` then answers "what is uncommitted?", and the + answer is always "nothing". So the reviewer received an empty diff and + said the only correct thing about one: + + "The supplied diff is empty, so it demonstrates none of that and + cannot be judged as satisfying the request." + + Measured on a real 48-line change that had already passed its checks. No + session-mode item could ever have been approved. + """ + import subprocess + + from agent_harness.session_executor import SessionExecutor + from agent_harness.work import WorkQueue, WorkRecord + + tree = tmp_path / "repo" + tree.mkdir() + for argv in ( + ["init", "-q", "-b", "main"], + ["config", "user.email", "t@t"], + ["config", "user.name", "t"], + ): + subprocess.run(["git", "-C", str(tree), *argv], check=True, capture_output=True) + (tree / "hello.txt").write_text("hello world\n") + subprocess.run(["git", "-C", str(tree), "add", "-A"], check=True, capture_output=True) + subprocess.run( + ["git", "-C", str(tree), "commit", "-q", "-m", "base"], check=True, capture_output=True + ) + # The item's own branch, as the executor cuts one, then the agent's work + # committed onto it by the pre-review checkpoint. + subprocess.run( + ["git", "-C", str(tree), "checkout", "-q", "-b", "harness/t1"], + check=True, + capture_output=True, + ) + (tree / "hello.txt").write_text("hello harness\n") + subprocess.run(["git", "-C", str(tree), "add", "-A"], check=True, capture_output=True) + subprocess.run( + ["git", "-C", str(tree), "commit", "-q", "-m", "checkpoint"], + check=True, + capture_output=True, + ) + + seen: dict[str, str] = {} + + class Reviewer: + def call(self, _role: str, messages: Any, **_: Any) -> Any: + seen["prompt"] = messages[-1]["content"] + body = {"choices": [{"message": {"content": "APPROVED\nok"}}]} + return type("R", (), {"body": body})() + + queue = WorkQueue(str(tmp_path / "w.sqlite")) + executor = SessionExecutor( + queue, + type("Host", (), {})(), + tree, + reviewer=Reviewer(), # type: ignore[arg-type] + ) + + executor._review(WorkRecord(item_id="T1", title="t", brief="b"), tree, True, "", base="main") + + assert "hello harness" in seen["prompt"], "the reviewer was not shown the committed work" + assert "-hello world" in seen["prompt"], "nor what it replaced"