From 3a13efa14a836eed4906b0d3f3be51b067af48b9 Mon Sep 17 00:00:00 2001 From: sprooty Date: Wed, 5 Aug 2026 01:18:59 +0000 Subject: [PATCH 1/2] fix: one review rubric, and a session retry that knows why it failed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two items were rejected today for artefacts that had already been found, fixed and closed — in the other copy of the prompt. **#167 — the rubric was duplicated.** `executor.py` and `session_executor.py` each carried a REVIEW_PROMPT, and every correction made from measurement landed in the headless one: that a diff is the whole change, that the harness ran the checks and which ones, that the files the diff touched are supplied, that scope belongs to the task. The session reviewer had none of them. So R4 added a paragraph to a diagnostics panel and was rejected on "the diff does not demonstrate that it is specifically integrated into the existing diagnostics/support surface rather than merely adjacent" which is exactly the failure #156 documented and closed: a diff cannot show its surroundings, and the fix was to supply them. R3's rejection carried the same shape. Both also contained a real point — R3 widened a repository trait's return type, R4's wording claimed everything absent from the bundle was deliberate — so the reviews were not wrong. Half of each objection was simply a solved problem in the wrong file. The rubric is now imported rather than copied, and the two reviewer helpers are module-level functions both executors call, so a correction cannot land in one and not the other. Two tests guard it: the prompts must be the same object, because "roughly the same" is what lets drift back; and `_review` must actually pass the fields, because identical prompts with empty fields would keep the reviewers unequal while passing the first test. **#166 — a session retry was blind.** The agent's prompt carried the title, the brief and the checks. Not the reviewer's verdict, not the last failure. A session attempt is minutes of a real agent rather than one API call, so repeating one blind is the most expensive avoidable thing here — and R3's retry would have been sent the identical brief with no mention of the specific, actionable criticism it had just received. Not resumption: the item is re-read from the current brief, no prior diff is fed back, and a guided attempt consumes an attempt exactly as an unguided one does. A first attempt reads exactly as it did before. One thing worth recording because tests did not catch it: the first version of this borrowed `Executor._review_context` as an unbound method on a `SessionExecutor`, which has no `context_policy` and would have raised at runtime. The suite passed anyway, because the test that exercises that path lives on another branch. Extracting real shared functions removes the class of mistake rather than the instance. Co-Authored-By: Claude Opus 5 (1M context) --- src/agent_harness/executor.py | 100 ++++++++++++------------ src/agent_harness/session_executor.py | 108 +++++++++++++++----------- tests/test_reviewer.py | 72 +++++++++++++++++ 3 files changed, 188 insertions(+), 92 deletions(-) diff --git a/src/agent_harness/executor.py b/src/agent_harness/executor.py index 9691786..c9f503a 100644 --- a/src/agent_harness/executor.py +++ b/src/agent_harness/executor.py @@ -1350,6 +1350,56 @@ def is_disk_exhaustion(detail: str) -> bool: """ +def review_context(repo: Path, diff: str, budget: int) -> str: + """The touched files as they now stand, for the reviewer. + + The reviewer is asked whether a change is wired in where it should be + and whether anything unrelated moved, and was given only the change. + Measured on rdpapp: two of the three reasons in a rejection were "the + diff does not show whether …", which no diff ever can. That is a gate + rejecting for the shape of its own prompt. + + The same budget bounds it, and a file too large to include is **named + as absent** rather than quietly left out — a reviewer that believes a + partial view is complete is worse than one that knows it is partial. + """ + paths: list[str] = [] + for line in diff.splitlines(): + if line.startswith("+++ ") and not line.startswith("+++ /dev/null"): + path = line[4:].strip() + path = path[2:] if path.startswith("b/") else path + if path and path not in paths: + paths.append(path) + if not paths: + return "" + + blocks: list[str] = [] + missing: list[str] = [] + spent = 0 + for path in paths: + try: + body = (repo / path).read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + missing.append(f" {path} — could not be read") + continue + block = f"--- {path} ---\n{body}\n" + if spent + len(block) > budget: + missing.append(f" {path} — {len(body)} characters, too large to include") + continue + blocks.append(block) + spent += len(block) + omitted = "\nNot included, so you have not seen them:\n" + "\n".join(missing) if missing else "" + return REVIEW_CONTEXT_PROMPT.format(files="\n".join(blocks), omitted=omitted) + + +def review_checks_prompt(commands: Sequence[Sequence[str]]) -> str: + """Which commands passed, and that the harness ran them.""" + commands = [" ".join(command) for command in commands if command] + if not commands: + return REVIEW_NO_CHECKS_PROMPT + return REVIEW_CHECKS_PROMPT.format(commands="\n".join(f" {command}" for command in commands)) + + class Executor: """Drives work items through the model roles.""" @@ -2430,56 +2480,10 @@ def _base_for(self, record: WorkRecord) -> tuple[str, str | None]: return first.branch, note def _review_context(self, diff: str) -> str: - """The touched files as they now stand, for the reviewer. - - The reviewer is asked whether a change is wired in where it should be - and whether anything unrelated moved, and was given only the change. - Measured on rdpapp: two of the three reasons in a rejection were "the - diff does not show whether …", which no diff ever can. That is a gate - rejecting for the shape of its own prompt. - - The same budget bounds it, and a file too large to include is **named - as absent** rather than quietly left out — a reviewer that believes a - partial view is complete is worse than one that knows it is partial. - """ - paths: list[str] = [] - for line in diff.splitlines(): - if line.startswith("+++ ") and not line.startswith("+++ /dev/null"): - path = line[4:].strip() - path = path[2:] if path.startswith("b/") else path - if path and path not in paths: - paths.append(path) - if not paths: - return "" - - blocks: list[str] = [] - missing: list[str] = [] - spent = 0 - for path in paths: - try: - body = (self.repo / path).read_text(encoding="utf-8") - except (OSError, UnicodeDecodeError): - missing.append(f" {path} — could not be read") - continue - block = f"--- {path} ---\n{body}\n" - if spent + len(block) > self.context_policy.budget: - missing.append(f" {path} — {len(body)} characters, too large to include") - continue - blocks.append(block) - spent += len(block) - omitted = ( - "\nNot included, so you have not seen them:\n" + "\n".join(missing) if missing else "" - ) - return REVIEW_CONTEXT_PROMPT.format(files="\n".join(blocks), omitted=omitted) + return review_context(self.repo, diff, self.context_policy.budget) def _review_checks_prompt(self) -> str: - """Which commands passed, and that the harness ran them.""" - commands = [" ".join(command) for command in self.checks.commands if command] - if not commands: - return REVIEW_NO_CHECKS_PROMPT - return REVIEW_CHECKS_PROMPT.format( - commands="\n".join(f" {command}" for command in commands) - ) + return review_checks_prompt(self.checks.commands) def _starved_prompt(self, starved: Sequence[str]) -> str: """Supporting targets that did not fit, named so they are not guessed at.""" diff --git a/src/agent_harness/session_executor.py b/src/agent_harness/session_executor.py index 7a1687f..f43f85e 100644 --- a/src/agent_harness/session_executor.py +++ b/src/agent_harness/session_executor.py @@ -36,7 +36,15 @@ from pathlib import Path from typing import Any -from .executor import APPROVED, REJECTED, Checks, Outcome, is_disk_exhaustion, run_git +from .executor import ( + APPROVED, + DEFAULT_CONTEXT_BUDGET, + REJECTED, + Checks, + Outcome, + is_disk_exhaustion, + run_git, +) from .graph import LOCAL_WORK from .model_client import CapExhausted, ModelClient, RequestRefused, RetryExhausted from .outcomes import ( @@ -77,6 +85,25 @@ #: prompt an agent was given stays on disk next to its result. DEFAULT_AGENT_COMMAND = ("claude", "-p", "{prompt_file}", "--permission-mode", "acceptEdits") +#: Why the last attempt was refused, for the attempt replacing it. A session +#: attempt is minutes of a real agent rather than one API call, so repeating +#: one blind is the most expensive avoidable thing here — and until now the +#: agent's prompt carried the title, the brief and the checks, and nothing +#: else. Measured: an item was rejected for widening a trait's return type, +#: and a retry would have been sent the identical brief with no mention of it. +#: +#: **Not resumption.** The item is re-read from the current brief, no prior +#: diff is fed back, and a guided attempt consumes an attempt exactly as an +#: unguided one does. +PRIOR_FAILURE_PROMPT = """ +## What happened last time + +A previous attempt at this item was refused. You are starting again from the +brief above, not continuing that attempt — but do not reproduce the fault: + +{error} +""" + PROMPT_TEMPLATE = """\ You are working one item from a plan. Work only on this item. @@ -92,7 +119,7 @@ {checks_description} A reviewer then reads your diff against the item above and can reject it. - +{prior} ## Rules - Change only what this item asks for. Unrelated edits will be rejected. @@ -104,44 +131,14 @@ around it is not. """ -REVIEW_PROMPT = """\ -Review this change. You did not write it, and your job is not to be agreeable. - -Assume it is wrong until the diff shows otherwise. Most changes that fail -review fail because they do something *adjacent* to what was asked, or claim -more than they did — not because they are obviously broken. - -The task: -{brief} - -The diff: -```diff -{diff} -``` - -Checks: {checks} - -## Answer - -First line exactly APPROVED or REJECTED. Then, in order: - -1. **What I verified** — the specific things in the diff you actually checked - against the task. If you cannot name any, that is a REJECTED. -2. **What I could not verify** — anything the diff claims that the diff alone - does not show. Say it, do not assume it. -3. **Why** — one paragraph. - -## Reject if - -- It does not do what the task asked, or does more than the task asked. -- It claims an effect the diff does not demonstrate. -- It changes something unrelated, however small. -- The task cannot be judged from what you were given. - -Approving work that does not do what was asked is the expensive failure here: -it reaches a pull request, a human reads it as reviewed, and the cost lands -much later. An unnecessary rejection costs one retry. -""" +#: The review rubric lives with the headless executor and is imported, not +#: copied. It used to be copied, and every correction made from measurement +#: landed in one of the two: the session reviewer was never told that a diff is +#: the whole change, never told the harness ran the checks, never given the +#: files the diff touched, and never told that scope belongs to the task. Two +#: items were rejected for exactly the artefacts those fixes had already +#: retired — in the other file (#167). +from .executor import REVIEW_PROMPT as REVIEW_PROMPT # noqa: E402 @dataclass @@ -484,6 +481,7 @@ def _execute(self, record: WorkRecord) -> Outcome: title=record.title, brief=record.brief, checks_description=self._describe_checks(), + prior=self._prior_failure(record), ) ) @@ -634,7 +632,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,17 +742,39 @@ def _on_waiting(self, record: WorkRecord, session: Session) -> None: url=url, ) - def _review(self, record: WorkRecord, tree: Path, passed: bool, failure: str) -> str: + def _prior_failure(self, record: WorkRecord) -> str: + """Why the last attempt was refused, bounded. + + `requeue` keeps `last_error` deliberately — "the only record of why the + item failed" — and until now nothing passed it to the agent about to + repeat the mistake (#166). + """ + error = (record.last_error or "").strip() + return PRIOR_FAILURE_PROMPT.format(error=error[:4000]) if error else "" + + 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` asks "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. Every session-mode reviewer was shown an empty + # diff and rejected on it, so nothing could ever be approved (#162). + diff = run_git(tree, "diff", f"{base}...HEAD") if base else run_git(tree, "diff", "HEAD") + from .executor import review_checks_prompt, review_context + prompt = REVIEW_PROMPT.format( brief=record.brief, diff=diff[:20000], - checks="passed" if passed else failure, + # The same helpers the headless reviewer uses, so a correction to + # either cannot land in one executor and not the other (#167). + context=review_context(tree, diff, DEFAULT_CONTEXT_BUDGET), + checks=review_checks_prompt(self.checks.commands), ) try: response = self.reviewer.call("reviewer", [{"role": "user", "content": prompt}]) diff --git a/tests/test_reviewer.py b/tests/test_reviewer.py index e31c2b8..643daa2 100644 --- a/tests/test_reviewer.py +++ b/tests/test_reviewer.py @@ -98,3 +98,75 @@ 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_both_executors_review_against_one_rubric() -> None: + """Two copies drifted, and two items were rejected for it. + + The rubric used to be duplicated in `executor.py` and + `session_executor.py`. Every correction made from measurement landed in + the headless one: that a diff is the whole change, that the harness ran + the checks, that the touched files are supplied, that scope belongs to the + task. The session reviewer had none of them, and rejected real work for + precisely the artefacts those fixes had already retired — in the other + file. + + Identity, not similarity: a test that allows "roughly the same" is a test + that allows the drift back. + """ + from agent_harness import executor, session_executor + + assert session_executor.REVIEW_PROMPT is executor.REVIEW_PROMPT + + +def test_the_session_reviewer_is_given_what_the_headless_one_is() -> None: + """The rubric names fields; supplying an empty string for them would keep + the two prompts identical and the two reviewers unequal.""" + import inspect + + from agent_harness import session_executor + + source = inspect.getsource(session_executor.SessionExecutor._review) + assert "review_context(" in source, "the touched files must reach the session reviewer" + assert "review_checks_prompt(" in source, "and what actually ran the checks" + assert "...HEAD" in source, "and the diff must be against the base, not the working tree" + + +def test_a_session_retry_is_told_why_the_last_attempt_was_refused(tmp_path: Any) -> None: + """A session attempt is minutes of a real agent, not one API call, so + repeating one blind is the most expensive avoidable thing here. + + Measured: an item was rejected for widening a repository trait's return + type — a specific, actionable criticism — and the retry would have been + sent the identical brief with no mention of it. + """ + from agent_harness.session_executor import PROMPT_TEMPLATE, SessionExecutor + from agent_harness.work import WorkQueue, WorkRecord + + queue = WorkQueue(str(tmp_path / "w.sqlite")) + executor = SessionExecutor(queue, type("Host", (), {})(), tmp_path) + + refused = WorkRecord( + item_id="R3", + title="t", + brief="b", + last_error="review rejected: it widens the repository trait's return type", + ) + prompt = PROMPT_TEMPLATE.format( + title=refused.title, + brief=refused.brief, + checks_description="none", + prior=executor._prior_failure(refused), + ) + + assert "widens the repository trait" in prompt + assert "not continuing that attempt" in prompt, "a new attempt, not a resumption" + + # A first attempt must read exactly as it did before this existed. + fresh = PROMPT_TEMPLATE.format( + title="t", + brief="b", + checks_description="none", + prior=executor._prior_failure(WorkRecord(item_id="R3", title="t", brief="b")), + ) + assert "What happened last time" not in fresh From 5432363bad371dc7fd97b5bea5ae5b72f1eed0be Mon Sep 17 00:00:00 2001 From: sprooty Date: Wed, 5 Aug 2026 01:21:45 +0000 Subject: [PATCH 2/2] test: keep the empty-diff regression when folding #162 into this branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I branched this from main, which does not carry #162, and re-applied the empty-diff fix here while rewriting _review. That left the same change in two open pull requests — my error in branching, not a second bug. Folding them: this branch already had the fix and was missing the test that proves it stays fixed, which is the half worth keeping. #162 closes as superseded rather than both being merged. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test_reviewer.py | 70 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/tests/test_reviewer.py b/tests/test_reviewer.py index 643daa2..8e044b3 100644 --- a/tests/test_reviewer.py +++ b/tests/test_reviewer.py @@ -170,3 +170,73 @@ def test_a_session_retry_is_told_why_the_last_attempt_was_refused(tmp_path: Any) prior=executor._prior_failure(WorkRecord(item_id="R3", title="t", brief="b")), ) assert "What happened last time" not in fresh + + +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"