From e1844b2e0593a304abaa6ff5d69fc4cdf24337c9 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 24 Oct 2026 03:35:35 -0400 Subject: [PATCH 1/2] perf(source-control): cut pr-merge gate reads per candidate The post-merge check compares the local clone's copy of the branch with a moved live tip once, so lane records on that copy need no compare each. The unattended gate stops a closed or merged pull request right after its REST read when no state directory can hold a merge request read-back. Co-Authored-By: Claude Opus 5.5 --- ...-pr-merge-gate-candidate-reads-5e1b7c3a.md | 8 + .../babysit-prs/scripts/babysit_merge.py | 8 +- .../scripts/babysit_merge_policy.py | 18 ++- .../babysit-prs/scripts/babysit_postmerge.py | 71 ++++++-- .../tests/test_babysit_merge_policy.py | 40 +++++ .../tests/test_babysit_merge_unattended.py | 64 +++++++- .../tests/test_babysit_postmerge_contains.py | 151 ++++++++++++++++++ 7 files changed, 340 insertions(+), 20 deletions(-) create mode 100644 .changes/source-control/perf-pr-merge-gate-candidate-reads-5e1b7c3a.md diff --git a/.changes/source-control/perf-pr-merge-gate-candidate-reads-5e1b7c3a.md b/.changes/source-control/perf-pr-merge-gate-candidate-reads-5e1b7c3a.md new file mode 100644 index 0000000000..4429bf04f7 --- /dev/null +++ b/.changes/source-control/perf-pr-merge-gate-candidate-reads-5e1b7c3a.md @@ -0,0 +1,8 @@ +--- +bump: patch +--- + +### Changed + +- The post-merge check asks the compare API once per branch tip when the branch moved after the gate's local clone was made, instead of once for every lane record. It compares the clone's copy of the branch with the live tip, and a record whose run head is on that copy is on the tip with no further call. A record off that copy, a diverged tip or an unreadable compare still gets its own compare, so every verdict is unchanged and every failed read still refuses. +- In the pr-merge lane (no `--state-dir`), the unattended gate stops a pull request the REST record reads as closed or merged right after that read, on the blocker it already gave it (`state=MERGED (not OPEN)` or `state=CLOSED (not OPEN)`), instead of running the post-merge check first. The verdict is unchanged; the gate skips the post-merge check and the queue sweep for that candidate. With a state directory, where a recorded merge request's read-back needs the full policy, the gate reads it in full as before. diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py index f84bd6be48..6ddad98a0f 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -1620,7 +1620,7 @@ def evaluate( if owner not in allowed: blockers.append(f"owner {owner!r} out of scope") if pr.get("state") != "OPEN": - blockers.append(f"state={pr.get('state')} (not OPEN)") + blockers.append(merge_policy.not_open_refusal(pr.get("state"))) if pr.get("isDraft"): blockers.append("PR is a draft -- mark ready first") if BODY_MERGE_HOLD_RE.search(str(pr.get("body") or "")): @@ -3278,6 +3278,7 @@ def assess_unattended( lane_rung_cap: str | None = None, shared: merge_policy.SharedPostMerge | None = None, share: Callable[[merge_policy.SharedPostMerge], object] | None = None, + stop_when_closed: bool = False, ) -> merge_policy.UnattendedPolicy: """The unattended decision for one pull request, in the wiring order: identity, the credential chokepoint, the kill switch, a default-branch @@ -3321,6 +3322,7 @@ def promotion(window_start: datetime | None) -> merge_policy.Promotion: lane_rung_cap=lane_rung_cap, shared=shared, share=share, + stop_when_closed=stop_when_closed, ) return score_unattended(repo, number, policy, self_logins) @@ -4945,6 +4947,10 @@ def _deadline_stop() -> int: lane_kill_switch=args.lane_kill_switch, lane_rung_cap=args.lane_rung_cap, shared=shared, + # With no state directory there is no recorded merge + # request whose read-back needs the policy of a closed + # pull request, so a closed one stops at its first read. + stop_when_closed=pending_path is None, # Each recompute goes back to the file for the next candidate. share=( None diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py index db97518715..99100abd90 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py @@ -4067,6 +4067,11 @@ def shared_refusal( return None +def not_open_refusal(state: object) -> str: + """The gate's blocker for a pull request whose GraphQL state is not OPEN.""" + return f"state={state} (not OPEN)" + + def assess( repo: str, number: int, @@ -4081,6 +4086,7 @@ def assess( shared: SharedPostMerge | None = None, share: Callable[[SharedPostMerge], object] | None = None, human_gated: str | None = None, + stop_when_closed: bool = False, ) -> UnattendedPolicy: """The policy readers for one pull request, in the wiring order. @@ -4119,13 +4125,21 @@ def assess( adopted pull request in the lane (`adoption_refusal`); None adopts none. `promotion(window_start)` reads the promotion evidence with the newest demotion record's creation as the window restart; without it nothing is - promoted. `self_logins` names only lane or bot self identities (see - `is_human`). Raises `PolicyReadError` when the pull request or either + promoted. With `stop_when_closed`, a pull request the REST record reads + as closed refuses right after that read (`not_open_refusal`), before any + config or post-merge read: the gate holds it on the same blocker. + `self_logins` names only lane or bot self identities (see `is_human`). Raises `PolicyReadError` when the pull request or either pipeline config cannot be read, because the agent identity is then unknown. A default branch head config that cannot be parsed, or names another identity, is a stop (`StopReadError`). """ pr = pull_request(repo, number) + if stop_when_closed and pr.get("state") == "closed": + return UnattendedPolicy( + not_open_refusal("MERGED" if pr.get("merged") is True else "CLOSED"), + enabled=False, + inputs={"pr": pr, "authority": authority}, + ) base_ref = str(json_object(pr.get("base")).get("ref")) base_sha = str(json_object(pr.get("base")).get("sha")) config = read_config_at(repo, base_sha) diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py index 293f8dc5a4..5beebd5b00 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py @@ -1047,6 +1047,9 @@ class _Context: checks_cache: dict[tuple[int, int], frozenset[str]] = field(default_factory=dict) # Full-history clones `contains` asks first; None until first resolved. local_clones: list[Path] | None = None + # Per live branch tip, the clone's copy of the branch when the compare + # API read it as that tip's ancestor, else None (`_tip_bridge`). + bridge_cache: dict[str, str | None] = field(default_factory=dict) baseline_cache: dict[tuple[str, datetime], frozenset[str] | None] = field( default_factory=dict ) @@ -1664,27 +1667,71 @@ def contains(self, ancestor: str, sha: str) -> bool: """Whether `sha` is `ancestor` or has it in its history: from a local full-history clone when one holds both commits, else from the compare API, whose `ahead` or `identical` status means `ancestor` is reachable - from `sha`, which is what `git merge-base --is-ancestor` tests.""" + from `sha`, which is what `git merge-base --is-ancestor` tests. For the + branch tip the clone lacks (the branch moved after it was made), an + ancestor of the clone's copy of the branch is an ancestor of the tip + when that copy is (`_tip_bridge`), so one compare answers them all.""" key = (ancestor, sha) if key not in self.contains_cache: if ancestor == sha: self.contains_cache[key] = True elif (local := self._local_contains(ancestor, sha)) is not None: self.contains_cache[key] = local + elif ( + sha == self.branch_tip + and (bridge := self._tip_bridge(sha)) is not None + and self._local_contains(ancestor, bridge) is True + ): + self.contains_cache[key] = True else: - compare = _read( - [ - "api", - f"repos/{self.owner}/{self.name}/compare/{ancestor}...{sha}", - ], - f"compare {ancestor[:12]}...{sha[:12]}", - ) - self.contains_cache[key] = json_object(compare).get("status") in ( - "ahead", - "identical", - ) + self.contains_cache[key] = self._compare(ancestor, sha) return self.contains_cache[key] + def _compare(self, ancestor: str, sha: str) -> bool: + """The compare API's answer to `contains`; a failed read raises.""" + compare = _read( + ["api", f"repos/{self.owner}/{self.name}/compare/{ancestor}...{sha}"], + f"compare {ancestor[:12]}...{sha[:12]}", + ) + return json_object(compare).get("status") in ("ahead", "identical") + + def _tip_bridge(self, tip: str) -> str | None: + """The first full-history clone's copy of the branch, read from + `refs/remotes/origin/` or else `refs/heads/`, when the + compare API reads it as an ancestor of `tip`; else None. Read once per + tip. A failed compare is None, not an error: each record then gets + its own compare, which raises on its own failure.""" + if tip not in self.bridge_cache: + bridge = None + if self.local_clones is None: + self.local_clones = _full_history_clones() + refs = (f"refs/remotes/origin/{self.branch}", f"refs/heads/{self.branch}") + local = next( + ( + out.strip() + for root in self.local_clones + for ref in refs + if ( + out := policy._git( + root, + "rev-parse", + "--verify", + "--quiet", + f"{ref}^{{commit}}", + ) + ) + and _FULL_SHA.fullmatch(out.strip()) + ), + None, + ) + if local is not None: + try: + bridge = local if self._compare(local, tip) else None + except PostMergeReadError: + bridge = None + self.bridge_cache[tip] = bridge + return self.bridge_cache[tip] + def _local_contains(self, ancestor: str, sha: str) -> bool | None: """`git merge-base --is-ancestor` in the first local clone that can decide (exit 0 or 1), or None. Only full SHA-1 ids are asked: a ref diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_policy.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_policy.py index 81c42ea2c6..c6308a8720 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_policy.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_policy.py @@ -2247,6 +2247,46 @@ def test_every_input_good_permits_the_merge(self) -> None: self.assertIs(result.promoted, True) self.assertEqual(result.report()["credentials"], "identity-only") + def test_a_closed_pull_request_stops_before_any_later_read(self) -> None: + gh = self.readable() + later = mock.Mock(side_effect=AssertionError("read after the refusal")) + for merged, state in ((True, "MERGED"), (False, "CLOSED")): + with self.subTest(state=state): + gh.calls.clear() + pr = gh.rest[f"repos/{REPO}/pulls/1"] + pr.update(state="closed", merged=merged) + result = self.assess( + post_merge=later, promotion=later, stop_when_closed=True + ) + later.assert_not_called() + self.assertEqual(gh.calls, [["api", f"repos/{REPO}/pulls/1"]]) + # The gate's own blocker for a pull request that is not open. + self.assertEqual(result.reason, f"state={state} (not OPEN)") + self.assertIs(result.enabled, False) + self.assertNotIn("identity", result.inputs) + + def test_a_closed_pull_request_is_read_in_full_without_the_stop(self) -> None: + gh = self.readable() + gh.rest[f"repos/{REPO}/pulls/1"].update(state="closed", merged=True) + self.all_clear() + seen: list[Any] = [] + self.assess( + post_merge=lambda floor, who: seen.append(who) or gate(), + promotion=lambda s: PROMOTED, + ) + self.assertEqual(seen, [IDENTITY]) + + def test_an_open_pull_request_is_read_in_full_with_the_stop(self) -> None: + gh = self.readable() + gh.rest[f"repos/{REPO}/pulls/1"].update(state="open", merged=False) + self.all_clear() + result = self.assess( + post_merge=lambda floor, who: gate(), + promotion=lambda s: PROMOTED, + stop_when_closed=True, + ) + self.assertIsNone(result.reason) + def test_config_is_read_at_the_pull_requests_base_sha(self) -> None: gh = self.readable() self.all_clear() diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_unattended.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_unattended.py index 88b5b83668..11520b59c6 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_unattended.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_unattended.py @@ -417,7 +417,11 @@ class PrMergeHoldsOnAHumanVeto(unittest.TestCase): ) def _vetoes(self, result: dict[str, Any]) -> list[str]: - return [b for b in result["blockers"] if "human blocking comment" in b or "human merge veto" in b] + return [ + b + for b in result["blockers"] + if "human blocking comment" in b or "human merge veto" in b + ] def test_a_human_do_not_merge_comment_holds(self) -> None: result = _evaluate( @@ -471,7 +475,9 @@ def test_blocking_words_without_a_veto_hold_nothing(self) -> None: def test_a_later_comment_from_the_same_person_lifts_the_veto(self) -> None: veto = _comment( - "maintainer", "Do not merge until the release is cut.", at="2026-10-12T10:00:00Z" + "maintainer", + "Do not merge until the release is cut.", + at="2026-10-12T10:00:00Z", ) lifted = _comment( "maintainer", "Release is out, go ahead.", at="2026-10-12T11:00:00Z" @@ -533,8 +539,13 @@ def test_only_owner_member_or_collaborator_can_veto(self) -> None: "body": "Do not merge.", "created_at": "2026-10-12T10:00:00Z", } - for kwargs in ({"issue_comments": [veto]}, {"review_comments": [inline]}): - result = _evaluate(fixtures._pr(), HEAD, None, self.PR_MERGE, **kwargs) + for kwargs in ( + {"issue_comments": [veto]}, + {"review_comments": [inline]}, + ): + result = _evaluate( + fixtures._pr(), HEAD, None, self.PR_MERGE, **kwargs + ) self.assertEqual( len(self._vetoes(result)), int(holds), result["blockers"] ) @@ -810,6 +821,8 @@ def setUp(self) -> None: self.pr_head_ref = "feat" # REST `mergeable_state`; "dirty" is a conflict with the base. self.pr_mergeable_state: str | None = None + # The REST `state` and `merged` fields; None leaves them out. + self.pr_rest_state: tuple[str, bool] | None = None # The pull request's commits and the compare answers, by endpoint, # for the Dependabot rebase request; None leaves the reads unfaked. self.pr_commits: list[dict[str, Any]] | None = None @@ -899,6 +912,11 @@ def run_main( "mergeable_state": self.pr_mergeable_state, "user": self.pr_user, "labels": self.pr_labels, + **( + {"state": self.pr_rest_state[0], "merged": self.pr_rest_state[1]} + if self.pr_rest_state + else {} + ), } kills = list(self.kill) self.capture = mock.Mock(side_effect=self._capture) @@ -1871,6 +1889,39 @@ def test_a_landed_entry_read_back_at_its_pin_dispatches_the_full_run(self) -> No self.assertIs(out["unattended"]["dispatched"], True) self.assertEqual(code, 10) + def test_a_landed_entry_read_back_dispatches_with_the_pr_read_as_merged( + self, + ) -> None: + # The REST record says merged too: a state directory can hold a + # recorded request, so the closed stop stays off and the dispatch runs. + self.flight = policy.InFlight(False, f"unverified-auto-merge:#{PR}") + self.pr_view = fixtures._pr(state="MERGED") + self.pr_rest_state = ("closed", True) + self.prior = { + "queued": True, + "head": HEAD, + "status": "merged", + "verification": {"verified": True, "mismatches": []}, + "requestedAt": "2026-10-12T10:00:00Z", + } + with tempfile.TemporaryDirectory() as state: + code, out = self.run_main("--state-dir", state) + self.assertIs(out["merged"], True) + self.dispatch.assert_called_once_with("owner/repo", ref="main") + self.post_check.assert_called_once() + self.assertEqual(code, 10) + + def test_a_merged_pr_with_no_state_dir_skips_the_post_merge_check(self) -> None: + self.pr_view = fixtures._pr(state="MERGED") + self.pr_rest_state = ("closed", True) + code, out = self.run_main() + self.post_check.assert_not_called() + self.assertEqual(code, 10) + self.assertIs(out["merged"], False) + self.assertIn("state=MERGED (not OPEN)", out["blockers"]) + self.assertEqual(self.log, []) + self.dequeue.assert_not_called() + def test_a_landed_entry_read_back_under_a_cap_with_room_exits_zero(self) -> None: # At a cap of 5 the landed merge leaves room, so no refusal stands. self.config = dataclasses.replace(self.config, max_in_flight=5) @@ -4337,7 +4388,10 @@ def answer(self, endpoint: str) -> Any: ] } ] - if endpoint == f"repos/owner/repo/check-runs/{job}/annotations?per_page=100": + if ( + endpoint + == f"repos/owner/repo/check-runs/{job}/annotations?per_page=100" + ): message = ( f"claude-lane-review-complete lane={lane} sha={HEAD}" " findings=0 inline=0 scope=whole" diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge_contains.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge_contains.py index 89fb0f8e47..7a228ae1ac 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge_contains.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge_contains.py @@ -304,5 +304,156 @@ def test_local_and_compare_agree_on_every_pair(self) -> None: self.assertEqual(set(local_verdicts), {True, False}) +class LiveRepo: + """The compare and branch-ref endpoints, answered from a live repository's + real ancestry: a SHA it lacks is a 404, as GitHub answers one. `failing` + names compare bases that fail, and `all_fail` fails every compare.""" + + def __init__(self, root: pathlib.Path, branch: str = "main") -> None: + self.root = root + self.branch = branch + self.failing: set[str] = set() + self.all_fail = False + self.calls: list[list[str]] = [] + + def compares(self) -> list[list[str]]: + return [c for c in self.calls if "/compare/" in c[1]] + + def known(self, sha: str) -> bool: + return ( + subprocess.run( + ["git", "-C", str(self.root), "cat-file", "-e", f"{sha}^{{commit}}"], + capture_output=True, + env=GIT_ENV, + ).returncode + == 0 + ) + + def __call__( + self, args: list[str], **_: object + ) -> subprocess.CompletedProcess[str]: + self.calls.append(args) + if args[1].endswith(f"/git/ref/heads/{self.branch}"): + tip = git(self.root, "rev-parse", f"refs/heads/{self.branch}") + return subprocess.CompletedProcess( + args, 0, json.dumps({"object": {"sha": tip}}), "" + ) + base, head = args[1].split("/compare/")[1].split("...") + if ( + self.all_fail + or base in self.failing + or not (self.known(base) and self.known(head)) + ): + return subprocess.CompletedProcess(args, 1, "", "gh: Not Found (HTTP 404)") + if base == head: + status = "identical" + elif base in set(git(self.root, "rev-list", head).split()): + status = "ahead" + elif head in set(git(self.root, "rev-list", base).split()): + status = "behind" + else: + status = "diverged" + return subprocess.CompletedProcess(args, 0, json.dumps({"status": status}), "") + + +class TipMoved(unittest.TestCase): + """The branch moved after the local clone was made: its live tip is not in + the clone, so every lane record's `_lane_sender` check asks whether its + run's head is on a tip only the compare API knows. Fixture: live main + m0..m19 with side commit s1 off m3; the clone is made at m19; then live + main gains n1 and n2. The verdict for each record must equal a + compare-only judgment.""" + + REPO = "example-org/example-repo" + EVENTS = frozenset({"workflow_dispatch"}) + + def setUp(self) -> None: + self.tmp = tempfile.TemporaryDirectory() + self.addCleanup(self.tmp.cleanup) + live = self.live = pathlib.Path(self.tmp.name) / "live" + live.mkdir() + git(live, "init", "-q", "-b", "main") + self.mains = [commit(live, f"m{i}") for i in range(20)] + git(live, "checkout", "-q", "-b", "side", self.mains[3]) + self.s1 = commit(live, "s1") + git(live, "checkout", "-q", "main") + self.clone = pathlib.Path(self.tmp.name) / "clone" + subprocess.run( + ["git", "clone", "-q", live.as_uri(), str(self.clone)], + check=True, + env=GIT_ENV, + ) + self.n1 = commit(live, "n1") + self.n2 = commit(live, "n2") + + def record(self, sha: str) -> dict[str, object]: + return { + "path": pm.LANE_DISPATCH_WORKFLOW, + "event": "workflow_dispatch", + "head_branch": "main", + "repository": {"full_name": self.REPO}, + "head_repository": {"full_name": self.REPO}, + "head_sha": sha, + } + + def judge( + self, shas: list[str], clones: list[pathlib.Path], api: LiveRepo + ) -> list[bool]: + with ( + mock.patch.object(pm, "gh_capture", api), + mock.patch.object(pm.policy, "_history_checkouts", return_value=clones), + ): + ctx = pm._Context( + owner="example-org", name="example-repo", identity="x", branch="main" + ) + return [ctx._lane_sender(self.record(s), self.EVENTS) for s in shas] + + def compare_only(self, shas: list[str]) -> list[bool]: + return self.judge(shas, [], LiveRepo(self.live)) + + def test_records_on_the_old_tip_cost_one_compare_in_all(self) -> None: + shas = [*self.mains, self.n1, self.s1] + api = LiveRepo(self.live) + verdicts = self.judge(shas, [self.clone], api) + self.assertEqual(verdicts, self.compare_only(shas)) + self.assertEqual(verdicts, [True] * 21 + [False]) + # One compare from the clone's main to the live tip answers every + # record on the old tip; n1 (not in the clone) and s1 (not on the + # old tip) are asked on their own. + self.assertEqual(len(api.compares()), 3, api.compares()) + self.assertIn(f"compare/{self.mains[-1]}...{self.n2}", api.compares()[0][1]) + + def test_a_force_moved_tip_falls_back_to_one_compare_per_record(self) -> None: + git(self.live, "reset", "-q", "--hard", self.mains[5]) + self.x1 = commit(self.live, "x1") + shas = list(self.mains) + api = LiveRepo(self.live) + verdicts = self.judge(shas, [self.clone], api) + self.assertEqual(verdicts, self.compare_only(shas)) + self.assertEqual(verdicts, [True] * 6 + [False] * 14) + # The bridge compare reads diverged, so it carries no answer. + self.assertEqual(len(api.compares()), 1 + len(shas)) + + def test_an_unreadable_bridge_falls_back_to_one_compare_per_record(self) -> None: + shas = list(self.mains) + api = LiveRepo(self.live) + api.failing.add(self.mains[-1]) + verdicts = self.judge(shas[:-1], [self.clone], api) + self.assertEqual(verdicts, [True] * 19) + self.assertEqual(len(api.compares()), 1 + 19) + + def test_a_failed_record_compare_still_raises(self) -> None: + api = LiveRepo(self.live) + api.all_fail = True + with self.assertRaises(pm.PostMergeReadError): + self.judge([self.mains[0]], [self.clone], api) + + def test_no_clone_asks_every_record(self) -> None: + shas = [*self.mains, self.n1, self.s1] + api = LiveRepo(self.live) + self.judge(shas, [], api) + self.assertEqual(len(api.compares()), len(shas)) + + if __name__ == "__main__": unittest.main() From 14da5c5cf790ec5c7cef4cde811093402cddfb39 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 24 Oct 2026 03:54:30 -0400 Subject: [PATCH 2/2] perf(source-control): keep the queue sweep for a merged candidate Stop a merged pull request after the identity and queue readers instead of right after the REST read, so its gate still dequeues a held entry when it is the run's last candidate. Only a merge stops early, since a closed pull request can reopen before the gate's own state read. Spend no bridge compare when the clone already holds the live tip. Co-Authored-By: Claude Opus 5.5 --- ...-pr-merge-gate-candidate-reads-5e1b7c3a.md | 2 +- .../babysit-prs/scripts/babysit_merge.py | 4 +- .../scripts/babysit_merge_policy.py | 15 +++--- .../babysit-prs/scripts/babysit_postmerge.py | 3 +- .../tests/test_babysit_merge_policy.py | 49 +++++++++++++------ .../tests/test_babysit_merge_unattended.py | 13 +++++ .../tests/test_babysit_postmerge_contains.py | 32 ++++++++++++ 7 files changed, 91 insertions(+), 27 deletions(-) diff --git a/.changes/source-control/perf-pr-merge-gate-candidate-reads-5e1b7c3a.md b/.changes/source-control/perf-pr-merge-gate-candidate-reads-5e1b7c3a.md index 4429bf04f7..94d089f624 100644 --- a/.changes/source-control/perf-pr-merge-gate-candidate-reads-5e1b7c3a.md +++ b/.changes/source-control/perf-pr-merge-gate-candidate-reads-5e1b7c3a.md @@ -5,4 +5,4 @@ bump: patch ### Changed - The post-merge check asks the compare API once per branch tip when the branch moved after the gate's local clone was made, instead of once for every lane record. It compares the clone's copy of the branch with the live tip, and a record whose run head is on that copy is on the tip with no further call. A record off that copy, a diverged tip or an unreadable compare still gets its own compare, so every verdict is unchanged and every failed read still refuses. -- In the pr-merge lane (no `--state-dir`), the unattended gate stops a pull request the REST record reads as closed or merged right after that read, on the blocker it already gave it (`state=MERGED (not OPEN)` or `state=CLOSED (not OPEN)`), instead of running the post-merge check first. The verdict is unchanged; the gate skips the post-merge check and the queue sweep for that candidate. With a state directory, where a recorded merge request's read-back needs the full policy, the gate reads it in full as before. +- An unattended gate run with no `--state-dir` (how the pr-merge lane runs) stops a pull request the REST record reads as merged before its demotion and post-merge reads, on the blocker it already gave it (`state=MERGED (not OPEN)`), instead of running the post-merge check first. The identity and queue readers are still read, so its queue sweep still dequeues a held entry. The verdict is unchanged. A closed pull request that was not merged, which can reopen, and any run with a state directory, where a recorded merge request's read-back needs the full policy, are read in full as before. diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py index 1b65afd50e..60a70bb0d9 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -4941,8 +4941,8 @@ def _deadline_stop() -> int: lane_rung_cap=args.lane_rung_cap, shared=shared, # With no state directory there is no recorded merge - # request whose read-back needs the policy of a closed - # pull request, so a closed one stops at its first read. + # request whose read-back needs the policy of a merged + # pull request, so a merged one skips the post-merge check. stop_when_closed=pending_path is None, # Each recompute goes back to the file for the next candidate. share=( diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py index 4aefa139f5..4038b5efa5 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py @@ -4128,20 +4128,15 @@ def assess( `promotion(window_start)` reads the promotion evidence with the newest demotion record's creation as the window restart; without it nothing is promoted. With `stop_when_closed`, a pull request the REST record reads - as closed refuses right after that read (`not_open_refusal`), before any - config or post-merge read: the gate holds it on the same blocker. + as merged refuses (`not_open_refusal`) once the identity and the readers + the queue sweep needs are read, before the demotion and post-merge + reads: the gate holds it on the same blocker. `self_logins` names only lane or bot self identities (see `is_human`). Raises `PolicyReadError` when the pull request or either pipeline config cannot be read, because the agent identity is then unknown. A default branch head config that cannot be parsed, or names another identity, is a stop (`StopReadError`). """ pr = pull_request(repo, number) - if stop_when_closed and pr.get("state") == "closed": - return UnattendedPolicy( - not_open_refusal("MERGED" if pr.get("merged") is True else "CLOSED"), - enabled=False, - inputs={"pr": pr, "authority": authority}, - ) base_ref = str(json_object(pr.get("base")).get("ref")) base_sha = str(json_object(pr.get("base")).get("sha")) config = read_config_at(repo, base_sha) @@ -4250,6 +4245,10 @@ def assess( "permission": permission, } inputs.update(permission=permission, self_logins=self_logins) + if stop_when_closed and pr.get("state") == "closed" and pr.get("merged") is True: + # The queue sweep still runs on what is read so far: this may be the + # run's last candidate. A merge is final; a closed one can reopen. + return replace(stopped, reason=not_open_refusal("MERGED")) # The run's shared answer stands in for the demotion, floor and post-merge # reads when it is bound to this run; otherwise they are read here. reuse = None diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py index ac209d7191..bf524c8e09 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py @@ -1744,7 +1744,8 @@ def _tip_bridge(self, tip: str) -> str | None: ), None, ) - if local is not None: + # A local tip leaves only the record's commit missing: no bridge. + if local is not None and local != tip: try: bridge = local if self._compare(local, tip) else None except PostMergeReadError: diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_policy.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_policy.py index c6308a8720..b4ae50f50e 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_policy.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_policy.py @@ -2247,23 +2247,42 @@ def test_every_input_good_permits_the_merge(self) -> None: self.assertIs(result.promoted, True) self.assertEqual(result.report()["credentials"], "identity-only") - def test_a_closed_pull_request_stops_before_any_later_read(self) -> None: + def test_a_merged_pull_request_stops_before_the_post_merge_reads(self) -> None: gh = self.readable() + gh.rest[f"repos/{REPO}/pulls/1"].update(state="closed", merged=True) later = mock.Mock(side_effect=AssertionError("read after the refusal")) - for merged, state in ((True, "MERGED"), (False, "CLOSED")): - with self.subTest(state=state): - gh.calls.clear() - pr = gh.rest[f"repos/{REPO}/pulls/1"] - pr.update(state="closed", merged=merged) - result = self.assess( - post_merge=later, promotion=later, stop_when_closed=True - ) - later.assert_not_called() - self.assertEqual(gh.calls, [["api", f"repos/{REPO}/pulls/1"]]) - # The gate's own blocker for a pull request that is not open. - self.assertEqual(result.reason, f"state={state} (not OPEN)") - self.assertIs(result.enabled, False) - self.assertNotIn("identity", result.inputs) + with ( + mock.patch.object(policy, "read_demotion", later), + mock.patch.object(policy, "in_flight", later), + mock.patch.object(policy, "eligibility", later), + ): + result = self.assess( + post_merge=later, promotion=later, stop_when_closed=True + ) + later.assert_not_called() + # The gate's own blocker for a pull request that is not open. + self.assertEqual(result.reason, "state=MERGED (not OPEN)") + self.assertIs(result.enabled, False) + self.assertIs(result.report()["reportOnly"], False) + # What the queue sweep reads: the identity, branch and readers. + for key in ("identity", "branch", "config", "permission", "self_logins"): + self.assertIn(key, result.inputs) + + def test_a_closed_unmerged_pull_request_is_read_in_full_with_the_stop( + self, + ) -> None: + # Closed but not merged can reopen before the gate's own state read, + # so only a merge, which is final, stops early. + gh = self.readable() + gh.rest[f"repos/{REPO}/pulls/1"].update(state="closed", merged=False) + self.all_clear() + seen: list[Any] = [] + self.assess( + post_merge=lambda floor, who: seen.append(who) or gate(), + promotion=lambda s: PROMOTED, + stop_when_closed=True, + ) + self.assertEqual(seen, [IDENTITY]) def test_a_closed_pull_request_is_read_in_full_without_the_stop(self) -> None: gh = self.readable() diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_unattended.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_unattended.py index 11520b59c6..0256c354db 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_unattended.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_unattended.py @@ -1911,6 +1911,19 @@ def test_a_landed_entry_read_back_dispatches_with_the_pr_read_as_merged( self.post_check.assert_called_once() self.assertEqual(code, 10) + def test_a_merged_pr_with_no_state_dir_still_sweeps_the_queue(self) -> None: + # The run's last candidate may be the merged one: its gate is then the + # only one left to dequeue a held entry the identity queued. + self.pr_view = fixtures._pr(state="MERGED") + self.pr_rest_state = ("closed", True) + self.queued = [5] + self.holds = {5: "do-not-merge"} + code, out = self.run_main() + self.post_check.assert_not_called() + self.assertEqual(code, 10) + self.assertEqual(self.log, ["dequeue:[5]"]) + self.assertEqual(out["unattended"]["writes"]["holds"], {"5": "do-not-merge"}) + def test_a_merged_pr_with_no_state_dir_skips_the_post_merge_check(self) -> None: self.pr_view = fixtures._pr(state="MERGED") self.pr_rest_state = ("closed", True) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge_contains.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge_contains.py index 7a228ae1ac..fad50eb33f 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge_contains.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge_contains.py @@ -448,6 +448,38 @@ def test_a_failed_record_compare_still_raises(self) -> None: with self.assertRaises(pm.PostMergeReadError): self.judge([self.mains[0]], [self.clone], api) + def test_a_record_off_the_old_tip_but_on_the_live_tip_is_true(self) -> None: + # Live main merges side after the clone was made: s1 is off the + # clone's copy of main, so only its own compare can answer True. + git(self.live, "merge", "-q", "--no-ff", "-m", "merge side", "side") + shas = [self.mains[0], self.s1, self.n1] + api = LiveRepo(self.live) + verdicts = self.judge(shas, [self.clone], api) + self.assertEqual(verdicts, self.compare_only(shas)) + self.assertEqual(verdicts, [True, True, True]) + + def test_a_tip_the_clone_holds_spends_no_bridge_compare(self) -> None: + # The tip is local; only the record's commit is missing from the clone. + tmp = pathlib.Path(self.tmp.name) / "older" + subprocess.run( + ["git", "clone", "-q", self.live.as_uri(), str(tmp)], + check=True, + env=GIT_ENV, + ) + api = LiveRepo(self.live) + with ( + mock.patch.object(pm, "gh_capture", api), + mock.patch.object(pm.policy, "_history_checkouts", return_value=[tmp]), + ): + ctx = pm._Context( + owner="example-org", name="example-repo", identity="x", branch="main" + ) + tip = ctx._branch_tip() + with self.assertRaises(pm.PostMergeReadError): + ctx.contains(UNKNOWN, tip) + # One compare for the unknown record (a 404), none for a bridge. + self.assertEqual(len(api.compares()), 1, api.compares()) + def test_no_clone_asks_every_record(self) -> None: shas = [*self.mains, self.n1, self.s1] api = LiveRepo(self.live)