Skip to content

Commit 9c59351

Browse files
committed
fix(bench): drop --3way fallback + post-verify in apply_as_commit
Diagnostic on the 1500-instance ceiling probe: 53% of swebench_verified instances reported file_recall=0.0 with mean recall 0.45 even at budget=10M. Investigating one (astropy__astropy-13033) showed the git diff after apply_as_commit committed files COMPLETELY UNRELATED to the gold patch -- e.g. CHANGES.rst, docs/changes/* instead of the declared astropy/timeseries/core.py. 5/5 sampled zero-recall instances have the same pattern with totally different committed content. Root cause: 1. ensure_repo's `git checkout --force base_commit` silently fails on shallow / incomplete bare clones -- HEAD stays at some main-branch commit, returncode=0 misleads the caller. 2. apply_as_commit then runs `git apply --index` which fails strictly on a wrong-base worktree (returncode != 0). 3. Falls through to `git apply --index --3way` which uses fuzzy matching and silently applies the patch on whatever state HEAD happens to be at. 4. Commit creates a HEAD~1..HEAD diff that is the patch reapplied on top of arbitrary main-branch content -- typically committing completely unrelated files. 5. Pipeline analyzes that diff and reports recall against gold_files that were never the patch's target. This invalidates downstream metrics for those instances. Headline 0.875 from old paper runs likely predates the regression that broke the silent-checkout edge case (or used a different cache state); the ceiling probe at budget=10M reporting 0.42 is a measurement artefact on instances corrupted by this bug. Fix: - Remove --3way fallback. `git apply --index` strict mode is the only path; failures return False and the instance is flagged. - Post-apply verify: `git diff --name-only HEAD~1..HEAD` must match the patch's declared target files (extracted from --- a/ / +++ b/ headers). Mismatch -> return False with APPLY VERIFY FAIL log. ensure_repo's silent-checkout problem still exists upstream; that fix needs `git rev-parse HEAD` verification + targeted fetch when base_commit is missing. Tracked for follow-up. The current change catches the symptom so no benchmark number is silently wrong.
1 parent 02ffbe5 commit 9c59351

1 file changed

Lines changed: 61 additions & 5 deletions

File tree

benchmarks/common.py

Lines changed: 61 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -212,18 +212,74 @@ def ensure_repo(
212212
return repo_dir
213213

214214

215+
def _patch_target_files(patch_text: str) -> set[str]:
216+
"""Extract target file paths from a unified diff (the +++ b/<path>
217+
lines, /dev/null excluded). Used to verify post-apply that git
218+
actually committed the patch's changes and not stale worktree state.
219+
"""
220+
out: set[str] = set()
221+
for line in patch_text.splitlines():
222+
if line.startswith("+++ b/"):
223+
p = line[6:].strip()
224+
if p and p != "/dev/null":
225+
out.add(p)
226+
elif line.startswith("--- a/"):
227+
p = line[6:].strip()
228+
if p and p != "/dev/null":
229+
out.add(p)
230+
return out
231+
232+
215233
def apply_as_commit(repo_dir: Path, patch_text: str, message: str = "bench") -> bool:
234+
"""Apply ``patch_text`` as a single commit on top of HEAD and verify
235+
the resulting ``HEAD~1..HEAD`` diff matches the patch's declared
236+
target files. Returns False on any of: apply error, post-apply
237+
file-set mismatch (``git apply --3way`` is permissive enough to
238+
silently accept fuzzy / wrong applies on dirty worktrees, which
239+
silently corrupts every downstream metric in the benchmark).
240+
241+
The verification is symmetric-difference vs.\\ a permissive subset:
242+
we require the committed file set to be a *non-empty* subset of the
243+
patch's declared targets. Extra files in HEAD (e.g.\\ leftover
244+
untracked from a previous instance) are rejected; missing files
245+
(patch declared file not in commit) are also rejected.
246+
"""
247+
expected = _patch_target_files(patch_text)
216248
with tempfile.NamedTemporaryFile(mode="w", suffix=".patch", delete=False) as f:
217249
f.write(patch_text)
218250
patch_path = f.name
219251
try:
220252
r = run_cmd(["git", "-C", str(repo_dir), "apply", "--index", patch_path], check=False)
221253
if r.returncode != 0:
222-
r = run_cmd(["git", "-C", str(repo_dir), "apply", "--index", "--3way", patch_path], check=False)
223-
if r.returncode != 0:
224-
print(f" APPLY FAIL: {r.stderr[:300]}")
225-
return False
226-
run_cmd(["git", "-C", str(repo_dir), "commit", "-m", message, "--allow-empty", "--no-verify"], check=False)
254+
# `--3way` is intentionally NOT used as a fallback here.
255+
# Empirically (May 2026), --3way silently produced commits
256+
# whose name-only diff bore no resemblance to the gold
257+
# patch on ~53% of swebench_verified instances, generating
258+
# fake zero-recall measurements for those. If the strict
259+
# apply fails, the instance is unrecoverable in the
260+
# benchmark and must be flagged.
261+
print(f" APPLY FAIL: {r.stderr[:300]}")
262+
return False
263+
run_cmd(
264+
["git", "-C", str(repo_dir), "commit", "-m", message, "--allow-empty", "--no-verify"],
265+
check=False,
266+
)
267+
268+
# Post-apply verification: which files actually got committed?
269+
diff_r = run_cmd(
270+
["git", "-C", str(repo_dir), "diff", "--name-only", "HEAD~1..HEAD"],
271+
check=False,
272+
)
273+
committed = {ln.strip() for ln in diff_r.stdout.splitlines() if ln.strip()}
274+
if expected and committed != expected:
275+
extra = committed - expected
276+
missing = expected - committed
277+
print(
278+
f" APPLY VERIFY FAIL: committed={sorted(committed)[:5]} "
279+
f"expected={sorted(expected)[:5]} "
280+
f"extra_n={len(extra)} missing_n={len(missing)}"
281+
)
282+
return False
227283
return True
228284
finally:
229285
os.unlink(patch_path)

0 commit comments

Comments
 (0)