diff --git a/scripts/overlord_sweep.py b/scripts/overlord_sweep.py index 6efbc2c1c..b18abfd00 100644 --- a/scripts/overlord_sweep.py +++ b/scripts/overlord_sweep.py @@ -1635,6 +1635,33 @@ def clear_parks(squads_to_clear): "bisection": bisection_result, "failed_squads": failed_squads, "preflight": health, } + # Normalize formatting BEFORE either idempotency check below. Both + # checks are commit-to-commit diffs (`ref..HEAD`), which never see + # uncommitted working-tree changes -- so this has to actually commit, + # not just reformat the working tree. Reuses format_sweep_branch + # itself (idempotent: the LATER call, further down, finds nothing left + # to do and reports "already cargo-fmt clean"). Committing here cannot + # leak an extra row into the evidence table or judgment queue below: + # both are built from all_shas, and commits_contributed resolves each + # squad's contribution from the merge boundaries recorded in + # merge_infos (computed earlier this round, before any fmt call), not + # from "whatever sits on HEAD now" -- see commits_contributed's own + # docstring. + # + # Measured 2026-08-11: PR #694 duplicated #692 (byte-identical diffs, + # md5-verified) despite the path-scoped duplicate check from #693 -- + # the ONLY difference the scoped diff found was rustfmt line-wrapping + # (e.g. a 3-line `match` arm collapsed to one line), because #692's + # branch had already been through format_sweep_branch's fmt-and-commit + # step while THIS round's comparison ran on raw, pre-fmt worker output. + # Both idempotency checks below compare tree content; unformatted + # content can never match a PR that already went through fmt, no + # matter how many times the same gap gets re-solved with functionally + # identical code. A fmt failure here is not fatal -- format_sweep_branch + # itself logs and reports it, and this degrades to exactly today's + # pre-fix behavior (comparing unformatted content), never worse. + format_sweep_branch(repo_root, run_git, fmt_fn=fmt_fn, log_fn=log_fn) + # The repo's DURABLE idempotency rule: compare the TREE, not the SHA. # A cherry-pick or a squash gives identical content a fresh sha (fresh # committer timestamp), so a stamp whose whole contribution is already @@ -1779,14 +1806,18 @@ def clear_parks(squads_to_clear): body = build_pr_body(evidence_rows=evidence_rows, judgment_entries=judgment_entries, branch=branch) title = build_sweep_pr_title(branch, all_shas, merge_infos) - # Formatting is deliberately the LAST thing to touch the branch. + # Usually a no-op by now: the idempotency-check section above already + # ran format_sweep_branch once, before either the origin_ref or the + # open-PR duplicate check, so both compare formatted content. Kept + # here (rather than relying solely on that earlier call) because # all_shas / the evidence table / the judgment queue above are the - # TAG-FIX commits, and the fmt commit is not one of them: it carries - # no trailers, closes no gap, and must not show up as a row in the - # PR's evidence table or as an entry in the judgment queue. Running - # it after cargo_test_workspace_fn is also deliberate -- rustfmt only - # moves whitespace, so re-running a multi-minute workspace suite - # afterwards would double the sweep's wall clock for no semantic gain. + # TAG-FIX commits, and the fmt commit must never show up as a row in + # the PR's evidence table or as an entry in the judgment queue -- + # calling it again after they're built, not before, is what keeps that + # true regardless of whether the earlier call already committed + # everything or the fmt-and-commit step still needs to happen + # (fmt_fn=None in a caller that never wired the earlier normalization + # in, or a fmt failure there that a retry here might still recover). fmt_result = format_sweep_branch(repo_root, run_git, fmt_fn=fmt_fn, log_fn=log_fn) # The lint gate CI will apply, applied BEFORE the push rather than after. diff --git a/scripts/test_overlord_sweep.py b/scripts/test_overlord_sweep.py index a12a2256c..685368571 100644 --- a/scripts/test_overlord_sweep.py +++ b/scripts/test_overlord_sweep.py @@ -1157,7 +1157,13 @@ def fake_push(repo_root, branch): # branch ref -- the only thing `git push origin ` and # `gh pr create --head ` ever see -- stays behind. branch = result["branch"] - self.assertTrue(result["fmt"]["committed"]) + # result["fmt"] reflects the LATE format_sweep_branch call -- by + # design usually a no-op now, since the idempotency-check section + # runs the same helper once, earlier, so both the origin_ref and + # the open-PR duplicate check compare already-formatted content + # (see the comment above that first call). The commit itself is + # what matters here, not which of the two calls made it. + self.assertFalse(result["fmt"]["committed"]) self.assertEqual(git_out(repo, "rev-parse", "HEAD").strip(), git_out(repo, "rev-parse", branch).strip()) self.assertIn("style: cargo fmt --all (sweep publish)", @@ -1763,6 +1769,65 @@ def test_an_unrelated_commit_landing_on_main_meanwhile_does_not_defeat_the_dupli self.assertEqual(pushed, []) self.assertEqual(prs, []) + def test_formatting_alone_does_not_defeat_the_duplicate_check(self): + """Measured 2026-08-11: PR #694 duplicated #692 (byte-identical + diffs, md5-verified) despite the path-scoped fix from #693. The + ONLY difference the scoped diff found was rustfmt whitespace: #692's + branch had already been through format_sweep_branch's fmt-and-commit + step (it published in an earlier round), while THIS round's + comparison ran on raw, pre-fmt worker output -- since both + idempotency checks are commit-to-commit diffs, unformatted content + can never tree-match a PR that already went through fmt, no matter + how many times the same gap gets re-solved with functionally + identical code. Fix: run format_sweep_branch once, early, before + either check, so both compare already-formatted content. + """ + repo = self.make_repo() + # An earlier round already published this fix, POST-fmt (the "( )" + # -> "()" normalization _reformatting_fmt_fn performs, matching + # what format_sweep_branch already committed for that round). + 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") + + # THIS round's squad branch contributes the identical fix, but as + # raw worker output -- PRE-fmt, exactly like every real worker + # patch before format_sweep_branch ever touches it. + 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