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..8e044b3 100644 --- a/tests/test_reviewer.py +++ b/tests/test_reviewer.py @@ -98,3 +98,145 @@ 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 + + +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"