From ffbf0a25dc0088ffff856eb82e47b59ad0e821dd Mon Sep 17 00:00:00 2001 From: sprooty Date: Mon, 3 Aug 2026 22:33:48 +0000 Subject: [PATCH] fix: show the implementer the repository it is patching The prompt asks for a unified diff that "applies cleanly at the repository root". `context_provider` defaulted to `lambda _record: ""` and the CLI never passed one, so it asked for that while showing the model nothing at all. A model cannot write context lines for a file it has not seen. So it wrote hunks with none, declaring `@@ -0,0` against files that were not empty; git apply refuses those, --unidiff-zero accepts them and inserts at line one, and code landed above module docstrings and imports with every check still green. That is the root cause behind #133, and it explains the shape of every diff this path has ever produced. Two parts: **Context.** A default provider that supplies the tracked file listing, the files the brief names in full, and as much of the rest as a byte budget allows, smallest first. Binary files are listed but not read. **Read it from the base, not the tree.** The working tree still holds the previous item's branch when the implementer is called -- the branch for this item is not cut until the apply step -- so reading the tree showed the model a file its patch would never meet. The base ref is now resolved before the implementer runs and the context is read from it with `git show`, which is correct by construction and touches no checkout. Live, on a three-item backlog with a real endpoint: 0/3 completed before, **3/3 after**, each on its own branch with its tests passing, the stacked item correctly based on its dependency, and the module docstring still the first thing in the file. --- src/agent_harness/executor.py | 95 ++++++++++++++++++++++++++++++++++- tests/test_executor.py | 64 +++++++++++++++++++++++ 2 files changed, 157 insertions(+), 2 deletions(-) diff --git a/src/agent_harness/executor.py b/src/agent_harness/executor.py index 4a9c661..3562c0d 100644 --- a/src/agent_harness/executor.py +++ b/src/agent_harness/executor.py @@ -105,6 +105,80 @@ def run_git(repo: Path, *args: str, check: bool = True) -> str: return result.stdout +#: How much repository the implementer is shown. Big enough that a small +#: project arrives whole, small enough not to dominate the prompt. +DEFAULT_CONTEXT_BUDGET = 60_000 + +#: Files that are never worth spending the budget on, whatever their size. +_UNINTERESTING = (".png", ".jpg", ".jpeg", ".gif", ".ico", ".pdf", ".zip", ".gz", ".woff", ".woff2") + + +def repo_context( + repo: Path, + record: Any = None, + *, + budget: int = DEFAULT_CONTEXT_BUDGET, + ref: str | None = None, +) -> str: + """What the repository looks like, for a model that cannot read it. + + The implementer is asked for a diff that "applies cleanly at the + repository root". Without this it was asked that while being shown + nothing, so it could not write context lines -- it did not know what they + were -- and emitted hunks claiming the file was empty. `--unidiff-zero` + then inserted them at line one, above module docstrings and imports, with + every check still green. + + Files named in the brief come first and whole: they are the ones being + edited, and a patch against a file the model has only seen the name of is + a patch written blind. The rest fills the remaining budget smallest-first, + on the grounds that many small files tell a model more about a codebase's + conventions than one large one. + + `ref` is the commit the patch will actually be applied to, and reading + from it rather than the working tree is the whole point: the tree still + holds the *previous* item's branch at this stage, so a model shown the + tree writes context lines for a file the patch will never meet. + """ + read: Callable[[str], str] + try: + if ref: + tracked = [ + p for p in run_git(repo, "ls-tree", "-r", "--name-only", ref).splitlines() if p + ] + read = lambda path: run_git(repo, "show", f"{ref}:{path}") # noqa: E731 + else: + tracked = [p for p in run_git(repo, "ls-files").splitlines() if p.strip()] + read = lambda path: (repo / path).read_text(encoding="utf-8") # noqa: E731 + except GitError: + return "" + if not tracked: + return "" + + brief = " ".join(str(getattr(record, field, "") or "") for field in ("title", "brief")).lower() + candidates = [p for p in tracked if not p.lower().endswith(_UNINTERESTING)] + # Mentioned first, then smallest-first for whatever budget remains. + mentioned = [p for p in candidates if p.lower() in brief or Path(p).name.lower() in brief] + rest = sorted( + (p for p in candidates if p not in mentioned), + key=lambda p: (repo / p).stat().st_size if (repo / p).is_file() else 0, + ) + + parts = ["Files in this repository:", *(f" {p}" for p in tracked), ""] + spent = sum(len(p) for p in parts) + for path in [*mentioned, *rest]: + try: + body = read(path) + except (OSError, UnicodeDecodeError, GitError): + continue # binary or unreadable: its name in the listing is all it gets + block = f"--- {path} ---\n{body}\n" + if spent + len(block) > budget: + continue + parts.append(block) + spent += len(block) + return "\n".join(parts) + + def extract_diff(reply: str) -> str | None: """Pull a unified diff out of a model reply. @@ -463,7 +537,14 @@ def __init__( self.on_event = on_event self.push = push self.now = now - self.context_provider = context_provider or (lambda _record: "") + # Defaults to showing the repository. An empty context meant asking a + # model for a patch that "applies cleanly" against files it had never + # seen, which it cannot do and which nothing said out loud. + self.context_provider = context_provider or self._default_context + #: The ref the current item's patch will be applied to. Resolved + #: before the implementer is called, because that is what it needs to + #: be looking at. + self._base: str | None = None # Where a patch that could not be applied is kept. Supplied, never # guessed: the core owns no directory layout. Without it the reply is # gone the moment the item fails, and the only way to see what the @@ -587,10 +668,21 @@ def _keepalive(self, record: WorkRecord) -> None: "its lease expired and another worker re-claimed it" ) + def _default_context(self, record: WorkRecord) -> str: + return repo_context(self.repo, record, ref=self._base) + def _execute(self, record: WorkRecord) -> Outcome: outcome = Outcome(record.item_id, FAILED) self._emit(record, "started") + # Resolved first, and deliberately before the implementer is called: + # the working tree still holds the previous item's branch, so a model + # shown the tree writes context lines for a file its patch will never + # meet. The branch itself is still cut later, so an item that produces + # no usable diff leaves no branch behind. + base, stacked_on = self._base_for(record) + self._base = base + # 1. Plan. Cheap, once per item, and the highest-leverage call. plan = self._call(record, PLANNER, PLAN_PROMPT.format(brief=record.brief)) outcome.stages.append("plan") @@ -639,7 +731,6 @@ def _execute(self, record: WorkRecord) -> Outcome: # 4. Apply, on a branch of its own, based on whatever this item # actually depends on. branch = f"{self.branch_prefix}{record.item_id.lower()}" - base, stacked_on = self._base_for(record) self._prepare_branch(branch, base) outcome.branch = branch outcome.base = base diff --git a/tests/test_executor.py b/tests/test_executor.py index e348a45..52d1202 100644 --- a/tests/test_executor.py +++ b/tests/test_executor.py @@ -24,6 +24,7 @@ Executor, apply_diff, extract_diff, + repo_context, validate_diff, ) from agent_harness.model_client import ( @@ -948,3 +949,66 @@ def test_a_reviewer_can_tell_where_a_rescued_hunk_landed(repo: Path, tmp_path: P shown = capturing.prompts["reviewer"] # The new line appears BEFORE the pre-existing one, and the diff says so. assert shown.index("+a new first line") < shown.index("hello world") + + +# --------------------------------------------- what the implementer is shown + + +def test_the_implementer_is_shown_the_repository(repo: Path, tmp_path: Path) -> None: + """The regression for #135. + + The prompt asks for a diff that "applies cleanly at the repository root". + With an empty context that is not a hard task, it is an impossible one: a + model cannot write context lines for a file it has never seen, so it + writes hunks with none and the tolerance ladder guesses where they go. + """ + executor, queue, _ = build( + repo, + tmp_path, + {"planner": "plan", "implementer": DIFF, "reviewer": "APPROVED\nfine"}, + ) + capturing = PromptCapturingModel( + {"planner": "plan", "implementer": DIFF, "reviewer": "APPROVED\nfine"} + ) + executor.client.transport = capturing + add_item(queue) + + executor.run_once() + + shown = capturing.prompts["implementer"] + assert "hello.txt" in shown, "the file listing must reach the implementer" + assert "hello world" in shown, "and so must the contents it is being asked to patch" + + +def test_the_file_the_brief_names_is_included_whole(repo: Path) -> None: + (repo / "big.txt").write_text("x\n" * 5000) + git(repo, "add", "-A") + git(repo, "commit", "-q", "-m", "big") + record = WorkRecord(item_id="T1", title="Edit hello", brief="Change hello.txt greeting.") + + context = repo_context(repo, record, budget=200) + + # The budget is tiny, so only the file the brief names earns its place. + assert "hello world" in context + assert "--- big.txt ---" not in context + # The listing is always there: a model should know what exists even when + # the budget will not stretch to showing it. + assert "big.txt" in context + + +def test_a_repository_with_no_tracked_files_yields_no_context(tmp_path: Path) -> None: + bare = tmp_path / "bare" + bare.mkdir() + git(bare, "init", "-q", "-b", "main") + assert repo_context(bare) == "" + + +def test_binary_files_are_listed_but_not_read(repo: Path) -> None: + (repo / "logo.png").write_bytes(b"\x89PNG\r\n\x1a\n" + b"\x00" * 100) + git(repo, "add", "-A") + git(repo, "commit", "-q", "-m", "logo") + + context = repo_context(repo) + + assert "logo.png" in context + assert "--- logo.png ---" not in context