From 5c7e1f1fa2bf24d89c2228b3834c7f9a970e47ea Mon Sep 17 00:00:00 2001 From: swackhamer Date: Tue, 11 Aug 2026 14:24:31 -0500 Subject: [PATCH] fix(fleet): scope the sweep duplicate-check to the paths this round touched Follow-up to #691. Measured 2026-08-11 in production: PR #692 duplicated #690 (byte-identical diffs, md5-verified) -- the #691 fix's own full-tree comparison missed it, because #690's branch was cut BEFORE #691 itself landed on origin/main, so it legitimately lacks the scripts/ changes #691 added. #692's branch, cut after #691 merged, carries those changes. A raw `diff --quiet ..HEAD` sees that unrelated drift as "different" and can never again match #690, no matter how many more times the same tag gap gets re-solved identically -- and the same defeat recurs for every future round whenever ANY unrelated commit lands on main while a sweep PR sits open, which given "the fleet publishes, it never merges" is routine. Fix: scope the comparison to `git diff --name-only origin_ref..HEAD` -- the paths THIS round's own squad merges actually touched -- instead of a full-tree diff. That isolates the tag-fix content from incidental history the two branches don't share. New regression test reproduces the exact #692-vs-#690 shape: an unrelated commit lands on main between when the open PR's branch was cut and when the new round's branch is cut, with the identical tag fix on both. Confirmed the test fails against the unversioned (pre-fix) code and passes with it. Full suite: 102 + 240 tests green. --- scripts/overlord_sweep.py | 21 ++++++++++-- scripts/test_overlord_sweep.py | 62 ++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 2 deletions(-) diff --git a/scripts/overlord_sweep.py b/scripts/overlord_sweep.py index f44277459..6efbc2c1c 100644 --- a/scripts/overlord_sweep.py +++ b/scripts/overlord_sweep.py @@ -1691,7 +1691,23 @@ def clear_parks(squads_to_clear): # open_sweep_prs_fn is optional (None in every existing test and any # caller that predates this check) so this is additive: skip # entirely rather than fail a sweep over a `gh` hiccup. + # + # Scoped to the PATHS this round's own squad merges touched, not a raw + # full-tree diff. Measured 2026-08-11: the raw full-tree version of this + # check missed a real duplicate (PR #692 vs #690, since confirmed + # byte-identical by md5) because THIS branch was cut from an origin/main + # that had since gained an unrelated commit (this very fix, #691) that + # PR #690's branch -- opened before #691 merged -- does not carry. Every + # future round inherits any commit that lands on origin/main in the + # meantime, so a full-tree compare against an older open PR sees THAT + # unrelated drift as "different" and never again matches, no matter how + # many times the same gap gets re-solved identically. Comparing only the + # paths this round actually changed isolates the tag-fix content from + # incidental history the two branches don't share. if open_sweep_prs_fn is not None: + changed_rc, changed_out, _changed_err = run_git( + ["diff", "--name-only", f"{origin_ref}..HEAD"], repo_root) + changed_paths = [p for p in changed_out.splitlines() if p.strip()] if changed_rc == 0 else [] # origin_ref is "/" in production (ORIGIN_MAIN = # "origin/main") and a bare local branch name ("main") in tests # that want no real remote at all -- same split every other @@ -1703,7 +1719,7 @@ def clear_parks(squads_to_clear): open_prs = open_sweep_prs_fn() or [] for pr in open_prs: head = pr.get("headRefName") if isinstance(pr, dict) else None - if not head or head == branch: + if not head or head == branch or not changed_paths: continue candidate_ref = head if remote: @@ -1716,7 +1732,8 @@ def clear_parks(squads_to_clear): # checking that candidate, never the whole duplicate scan. continue candidate_ref = f"refs/remotes/{remote}/{head}" - cmp_rc, _out2, _err2 = run_git(["diff", "--quiet", f"{candidate_ref}..HEAD"], repo_root) + cmp_rc, _out2, _err2 = run_git( + ["diff", "--quiet", f"{candidate_ref}..HEAD", "--", *changed_paths], repo_root) if cmp_rc == 0: pr_ref = pr.get("url") or pr.get("number") or head log_fn(f"{branch} is tree-identical to already-open {pr_ref} ({head}) -- skipping " diff --git a/scripts/test_overlord_sweep.py b/scripts/test_overlord_sweep.py index c727841ad..a12a2256c 100644 --- a/scripts/test_overlord_sweep.py +++ b/scripts/test_overlord_sweep.py @@ -1701,6 +1701,68 @@ def test_content_identical_to_an_already_open_pr_is_a_duplicate_not_a_second_pr( # re-collecting the stamp forever would spin every round. self.assertIn("canon", cursor["squads"]) + def test_an_unrelated_commit_landing_on_main_meanwhile_does_not_defeat_the_duplicate_check(self): + """Measured 2026-08-11: PR #692 duplicated #690 (byte-identical + diffs, md5-verified) and the FIRST version of this fix -- a raw + full-tree compare -- missed it. #690's branch was cut before this + very fix (#691) landed on main; #692's branch was cut after, so it + legitimately carries #691's scripts/ changes that #690's branch + does not. A full-tree diff sees that as "different" forever, no + matter how many times the SAME gap gets re-solved identically -- + every future round inherits whatever unrelated commits landed on + main in the meantime. The fix: scope the comparison to the paths + THIS round's own squad merges touched, so incidental history the + two branches don't share can never mask a real duplicate. + """ + repo = self.make_repo() + # An earlier round already published this exact fix as an open PR, + # cut from main BEFORE the unrelated commit below landed. + git(repo, "branch", "sweep/tags-earlier", "main") + git(repo, "checkout", "-q", "sweep/tags-earlier") + self.commit_file(repo, "src/a.rs", "fn a() {}\n", "sweep: fix JPEG:Foo") + git(repo, "checkout", "-q", "main") + + # A completely unrelated PR (infra, docs, anything) lands on main in + # between -- e.g. this very fix. + self.commit_file(repo, "scripts/unrelated.py", "# unrelated infra change\n", "infra: unrelated fix") + + # THIS round's squad branch is cut from the NEW main tip (so it + # carries the unrelated commit #690 never saw) and produces the + # identical fix under a fresh sha. + git(repo, "branch", "squad/canon", "main") + git(repo, "checkout", "-q", "squad/canon") + canon_sha = self.commit_file( + repo, "src/a.rs", "fn a() {}\n", "fix JPEG:Foo", + trailers=[("Format", "JPEG"), ("Tag", "MakerNotes:Foo")], + ) + git(repo, "checkout", "-q", "main") + + with tempfile.TemporaryDirectory() as tmpdir: + home = Path(tmpdir) / "home" + config_toml = self._config_toml(Path(tmpdir), ["canon"]) + squad_merge_loop.record_head( + squad_merge_loop.squad_status_file(home, "canon"), "workerhead", status="consumed", + patch_id="p1", format_name="JPEG", squad_sha=canon_sha, now_fn=lambda: 100, + ) + tested, pushed, prs = [], [], [] + result = overlord_sweep.run_sweep( + repo_root=repo, home=home, cache_dir="/unused", + comparison_fn=self._passing_comparison_fn, checkout_fn=self._checkout_fn, + config_path=config_toml, sweep_state_path=home / "sweep-state.json", origin_ref="main", + dispatcher_lock_path=home / "logs" / "dispatcher.lock", + cargo_test_workspace_fn=lambda repo_root: tested.append(1) or (True, "ok"), + push_branch_fn=lambda repo_root, branch: pushed.append(branch) or (True, "pushed"), + create_pr_fn=lambda *a, **kw: prs.append(a) or {"ok": True, "url": "u"}, + fmt_fn=self._reformatting_fmt_fn, lint_fn=lambda repo_root: (True, ""), log_fn=lambda *a: None, + open_sweep_prs_fn=lambda: [ + {"headRefName": "sweep/tags-earlier", "number": 42, "url": "https://example/pull/42"}, + ], + ) + self.assertEqual(result["status"], "duplicate_of_open_pr") + self.assertEqual(tested, []) + self.assertEqual(pushed, []) + self.assertEqual(prs, []) + def test_a_content_DIFFERENT_open_pr_does_not_veto_a_genuinely_new_fix(self): """The duplicate gate must not become a second, accidental zero_delta check: an open sweep PR fixing a DIFFERENT tag must