From 6bb445c098b1141d0069590d5a75403243bdac50 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 21:33:41 -0400 Subject: [PATCH 1/3] fix(source-control): post-merge remediation disarms the lane's armed pull requests babysit_postmerge.py --remediate now turns off native auto-merge on every open pull request into the judged branch that the agent identity both opened and armed, before it dequeues the identity's merge-queue entries. A pull request a person armed, or one the identity did not open, is left armed. Failures are reported as disarm: errors and do not stop the dequeue or the demotion issue. Closes #9518 Co-Authored-By: Claude Opus 5.5 --- ...518-remediate-disarms-lane-prs-3c7e19a4.md | 7 ++ .../babysit-prs/reference/guard-contract.md | 4 +- .../babysit-prs/scripts/babysit_postmerge.py | 85 +++++++++++++-- .../scripts/tests/guard_contract.py | 6 +- .../scripts/tests/test_babysit_postmerge.py | 102 ++++++++++++++++++ 5 files changed, 194 insertions(+), 10 deletions(-) create mode 100644 .changes/source-control/fix-9518-remediate-disarms-lane-prs-3c7e19a4.md diff --git a/.changes/source-control/fix-9518-remediate-disarms-lane-prs-3c7e19a4.md b/.changes/source-control/fix-9518-remediate-disarms-lane-prs-3c7e19a4.md new file mode 100644 index 0000000000..df6deebc04 --- /dev/null +++ b/.changes/source-control/fix-9518-remediate-disarms-lane-prs-3c7e19a4.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- `babysit_postmerge.py --remediate` now turns off native auto-merge on every open pull request into the judged branch that the agent identity both opened and armed, before it dequeues the identity's merge-queue entries, so GitHub cannot enqueue an armed lane pull request after a demotion. A pull request a person armed, or one the identity did not open, stays armed. Its report lists them as `disarmed`, and a failed read or disarm is a `disarm:` error that does not stop the dequeue or the demotion issue. diff --git a/plugins/source-control/skills/babysit-prs/reference/guard-contract.md b/plugins/source-control/skills/babysit-prs/reference/guard-contract.md index 164e6146f1..edd55bcc26 100644 --- a/plugins/source-control/skills/babysit-prs/reference/guard-contract.md +++ b/plugins/source-control/skills/babysit-prs/reference/guard-contract.md @@ -26,7 +26,7 @@ This table scopes `mutation` to DOMAIN state -- GitHub, the queue state file, wo | `skills/babysit-prs/scripts/manage_babysit_lease.py` | -- | mutating | writes, renews, and deletes lease files under --state-dir | none for acquire / heartbeat / release -- they write unconditionally; --apply gates reap's deletion ONLY | Local filesystem writes only, no GitHub write. The --apply flag does not make this script read-only in its other three actions. | `lease.acquire-writes-without-apply`, `lease.reap-without-apply-is-a-dry-run` | | `skills/babysit-prs/scripts/manage_feedback_ledger.py` | -- | conditionally mutating | records feedback dispositions, fix rounds, and check reruns in queue state | --apply, additionally requiring --lease-token | Local state only, no GitHub write. The lease token requirement means an --apply invocation still refuses without proof of lease ownership. | `ledger.apply-requires-lease-token`, `ledger.advisory-round-requires-finding-class`, `ledger.finding-class-requires-an-advisory-round`, `ledger.rerun-requires-check-id`, `ledger.check-id-must-be-hex`, `ledger.check-id-requires-a-rerun` | | `skills/babysit-prs/scripts/prune_babysit_worktrees.py` | -- | conditionally mutating | runs `git worktree remove` on eligible worktrees under --root | --apply (absent: dry run) | The most destructive local helper in the lane. --root is required, so it cannot sweep a directory the caller did not name. | `prune.root-is-required`, `prune.removes-worktrees-via-git` | -| `skills/babysit-prs/scripts/babysit_postmerge.py` | -- | conditionally mutating | re-runs failed jobs of a red full run; dispatches a full run when none covers an unverified auto-merge; dequeues the agent identity's merge-queue entries; files, reopens, or comments on the demotion issue; files or comments on a quarantine issue | --remediate, --file-quarantine | Read-only without --remediate or --file-quarantine. | `postmerge.remediate-gates-every-write`, `postmerge.file-quarantine-gates-its-writes`, `postmerge.parser-refuses-abbreviation` | +| `skills/babysit-prs/scripts/babysit_postmerge.py` | -- | conditionally mutating | re-runs failed jobs of a red full run; dispatches a full run when none covers an unverified auto-merge; turns off native auto-merge the agent identity armed on its own pull requests; dequeues the agent identity's merge-queue entries; files, reopens, or comments on the demotion issue; files or comments on a quarantine issue | --remediate, --file-quarantine | Read-only without --remediate or --file-quarantine. | `postmerge.remediate-gates-every-write`, `postmerge.file-quarantine-gates-its-writes`, `postmerge.parser-refuses-abbreviation` | | `skills/babysit-prs/scripts/refresh_pr_branch.py` | -- | conditionally mutating | disables an armed auto-merge, then asks GitHub to update the PR branch server-side; writes queue state | --apply | Mutates the PR branch on GitHub WITHOUT pushing: the update is a pinned server-side update-branch API call. | `refresh.uses-server-side-update-branch` | | `skills/babysit-prs/scripts/request_review.py` | -- | conditionally mutating | POSTs an issue comment requesting an AI re-review; writes queue state | --apply | A GitHub write despite the read-sounding name. | `review.posts-a-comment` | | `skills/babysit-prs/scripts/pr_queue_snapshot.py` | -- | conditionally mutating | writes the queue snapshot to local state | --write-state (absent: reports the snapshot without persisting it) | No GitHub write on any path. The only domain-state mutation is the local snapshot file. | `snapshot.write-state-gates-the-only-write` | @@ -130,7 +130,7 @@ Asserted against the entry point's source, because the mechanism -- not just the | `merge.parser-refuses-abbreviation` | `skills/babysit-prs/scripts/babysit_merge.py` | babysit_merge.py builds its parser with allow_abbrev=False. Argparse's default would resolve any unambiguous prefix, so a permission rule written against a flag's exact spelling would not cover the prefixes that reach the same flag. | | `resolve.parser-refuses-abbreviation` | `skills/babysit-prs/scripts/babysit_resolve_thread.py` | babysit_resolve_thread.py sets allow_abbrev=False for the same reason, and it carries more weight here: the resolve wrapper filters nothing, so the parser is the only layer refusing an abbreviated --allow-unpinned-thread. | | `resolve.wrapper-carries-no-filter` | `scripts/source-control-babysit-resolve-thread` | ASYMMETRY, stated at the source: the resolve wrapper contains no argument filter loop. Adding one here without a matching row is the drift this contract exists to catch. | -| `postmerge.remediate-gates-every-write` | `skills/babysit-prs/scripts/babysit_postmerge.py` | babysit_postmerge.py calls its writers (the attribution re-run, the full-run dispatch, the dequeue and the demotion issue) only inside one branch of main() that needs --remediate, a readable verdict and a known branch. Without the flag it reads GitHub and prints the verdict, whatever the verdict says. | +| `postmerge.remediate-gates-every-write` | `skills/babysit-prs/scripts/babysit_postmerge.py` | babysit_postmerge.py calls its writers (the attribution re-run, the full-run dispatch, the disarm, the dequeue and the demotion issue) only inside one branch of main() that needs --remediate, a readable verdict and a known branch. Without the flag it reads GitHub and prints the verdict, whatever the verdict says. | | `postmerge.file-quarantine-gates-its-writes` | `skills/babysit-prs/scripts/babysit_postmerge.py` | babysit_postmerge.py --judge-run files or comments on a quarantine issue only inside one branch of its --judge-run path that needs --file-quarantine; without it --judge-run reads GitHub and prints its judgment. | | `postmerge.parser-refuses-abbreviation` | `skills/babysit-prs/scripts/babysit_postmerge.py` | babysit_postmerge.py sets allow_abbrev=False, so a permission rule written against the exact spelling --remediate also covers every prefix of it. | 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 2459697a96..e99a8639c1 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py @@ -27,8 +27,9 @@ same event before the merge (`_Context.regressions`); with none left it is reported as unattributed and passes like a green run. -`remediate()` acts on failures in order: dequeue the identity's merge-queue -entries, file or comment on the demotion issue, then record the telemetry +`remediate()` acts on failures in order: disarm the native auto-merge the +identity armed on its own open pull requests, dequeue the identity's +merge-queue entries, file or comment on the demotion issue, then record the telemetry count. `attribute()` triggers the one re-run, and `dispatch_full_run()` the dispatch. The CLI is read-only unless `--remediate` is given; with it, a cycle also dispatches one full run on the branch tip whenever an unverified @@ -505,6 +506,18 @@ def _graphql( "enqueuer{__typename login} pullRequest{id number}}}}}}" ) DEQUEUE_MUTATION = "mutation PostMergeDequeue($id:ID!){dequeuePullRequest(input:{id:$id}){clientMutationId}}" +ARMED_QUERY = ( + "query PostMergeArmed($o:String!,$r:String!,$b:String!,$c:String){" + "repository(owner:$o,name:$r){pullRequests(states:OPEN,baseRefName:$b," + "first:100,after:$c){pageInfo{hasNextPage endCursor} nodes{id number " + "author{__typename login} autoMergeRequest{enabledBy{__typename login}}}}}}" +) +# Open pull request pages the armed read walks: 1000, as enable-auto-merge.mjs. +ARMED_PAGE_LIMIT = 10 +DISARM_MUTATION = ( + "mutation PostMergeDisarm($id:ID!){" + "disablePullRequestAutoMerge(input:{pullRequestId:$id}){clientMutationId}}" +) def native_auto_merge(pr: dict[str, Any], merge: AutoMerge, identity: str) -> bool: @@ -2268,6 +2281,55 @@ def dequeue_identity_entries( return unknown +def disarm_identity_pulls( + repo: str, branch: str, identity: str, *, disarmed: list[int] | None = None +) -> None: + """Turn off native auto-merge on every open pull request into `branch` + that the agent identity both opened and armed, appending each number to + `disarmed` as it goes. A pull request a person armed, or one the identity + did not open, is left armed. Every one is tried; a failed one does not + stop the rest. Raises `PostMergeReadError` on a read that is not every + open pull request, or `PostMergeWriteError` naming every failed one.""" + owner, name = _split_repo(repo) + key = _login_key(identity) + armed: list[tuple[str, int]] = [] + cursor = None + for _ in range(ARMED_PAGE_LIMIT): + data = _graphql( + ARMED_QUERY, "armed pull requests", o=owner, r=name, b=branch, c=cursor + ) + pulls = json_object(json_object(data.get("repository")).get("pullRequests")) + for pr in (json_object(n) for n in json_array(pulls.get("nodes"))): + request = pr.get("autoMergeRequest") + if ( + is_json_object(request) + and _actor_key(pr.get("author")) == key + and _actor_key(request.get("enabledBy")) == key + ): + armed.append((str(pr.get("id")), pr.get("number"))) + page = json_object(pulls.get("pageInfo")) + if not page.get("hasNextPage"): + break + cursor = page.get("endCursor") + else: + raise PostMergeReadError( + f"armed pull requests: more than {ARMED_PAGE_LIMIT} pages of open pull requests" + ) + failed: list[str] = [] + for pr_id, number in armed: + try: + _graphql( + DISARM_MUTATION, f"disarm #{number}", PostMergeWriteError, id=pr_id + ) + except PostMergeWriteError as exc: + failed.append(str(exc)) + continue + if disarmed is not None: + disarmed.append(number) + if failed: + raise PostMergeWriteError("; ".join(failed)) + + # The pinned-head record: an identity-authored comment the wrapper posts on the # pull request immediately before it enqueues, so the read-back has a source no # host stores. @@ -2654,14 +2716,17 @@ def remediate( label: str | None = None, record_telemetry: Callable[[int], None] | None = None, ) -> dict[str, Any]: - """Act on the verdict's failures: dequeue every merge-queue entry the agent - identity enqueued, then file one demotion issue per (signal, merge SHA) or + """Act on the verdict's failures: turn off native auto-merge on every open + pull request the agent identity opened and armed, so GitHub enqueues none + of them, then dequeue every merge-queue entry the agent identity enqueued, + then file one demotion issue per (signal, merge SHA) or failing run, or comment on the existing one, also found by its failing run (reopening it first when closed and the sighting is new; only the filers' own comments count as a prior sighting), then record the number of new issues as `demotion_events`. Each step runs even when an earlier one failed; errors are listed in the report. No failure, no write.""" report: dict[str, Any] = { + "disarmed": [], "dequeued": [], "issues": [], "comments": [], @@ -2688,9 +2753,17 @@ def remediate( report["errors"].append(f"quarantine issue: {exc}") if not verdict.failures: return report + # The queue is the judged branch's unless the caller names another. + queue_branch = branch or verdict.branch + try: + if not queue_branch: + raise PostMergeReadError("no branch to read the pull requests of") + disarm_identity_pulls( + repo, queue_branch, agent_identity, disarmed=report["disarmed"] + ) + except (PostMergeReadError, PostMergeWriteError) as exc: + report["errors"].append(f"disarm: {exc}") try: - # The queue is the judged branch's unless the caller names another. - queue_branch = branch or verdict.branch if not queue_branch: raise PostMergeReadError("no branch to read the merge queue of") unknown = dequeue_identity_entries( diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/guard_contract.py b/plugins/source-control/skills/babysit-prs/scripts/tests/guard_contract.py index 9ebbc6428f..35d301d9f5 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/guard_contract.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/guard_contract.py @@ -1332,7 +1332,8 @@ def _thread( id="postmerge.remediate-gates-every-write", claim=( "babysit_postmerge.py calls its writers (the attribution re-run, the " - "full-run dispatch, the dequeue and the demotion issue) only inside one " + "full-run dispatch, the disarm, the dequeue and the demotion issue) only " + "inside one " "branch of main() that needs --remediate, a readable verdict and a known " "branch. Without the flag it reads GitHub and prints the verdict, whatever " "the verdict says." @@ -1478,7 +1479,8 @@ def _thread( mutation=CONDITIONAL, mutates_what=( "re-runs failed jobs of a red full run; dispatches a full run when none " - "covers an unverified auto-merge; dequeues the agent identity's " + "covers an unverified auto-merge; turns off native auto-merge the agent " + "identity armed on its own pull requests; dequeues the agent identity's " "merge-queue entries; files, reopens, or comments on the demotion issue; " "files or comments on a quarantine issue" ), diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge.py index def7ac76c7..7155658671 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge.py @@ -158,6 +158,15 @@ def failure_annotation(title: str = "") -> dict[str, Any]: } +def armed_pull( + pr_id: str, number: int, *, author: dict[str, str], armer: dict[str, str] | None +) -> dict[str, Any]: + """An open pull request as the armed read returns it; `armer` None is one + with auto-merge off.""" + request = None if armer is None else {"enabledBy": armer} + return {"id": pr_id, "number": number, "author": author, "autoMergeRequest": request} + + def completed( stdout: Any = None, *, returncode: int = 0, stderr: str = "" ) -> subprocess.CompletedProcess[str]: @@ -183,6 +192,8 @@ def __init__(self) -> None: # branch, a set stands in for a same-named tag on another history. self.bare_main: set[str] | None = None self.queue: list[dict[str, Any]] = [] + # Open pull requests into the branch, as the armed read returns them. + self.open_pulls: list[dict[str, Any]] = [] self.issues: list[dict[str, Any]] = [] self.comments: dict[int, list[dict[str, Any]]] = {} self.fail_reads: set[str] = set() @@ -387,6 +398,16 @@ def _graphql(self, joined: str) -> subprocess.CompletedProcess[str]: return completed( {"data": {"dequeuePullRequest": {"clientMutationId": None}}} ) + if "disablePullRequestAutoMerge" in joined: + return completed( + {"data": {"disablePullRequestAutoMerge": {"clientMutationId": None}}} + ) + if "PostMergeArmed" in joined: + page = { + "pageInfo": {"hasNextPage": False, "endCursor": None}, + "nodes": self.open_pulls, + } + return completed({"data": {"repository": {"pullRequests": page}}}) if "PostMergeQueue" in joined: entries = {"totalCount": len(self.queue), "nodes": self.queue} return completed( @@ -2360,6 +2381,87 @@ def test_an_entry_with_an_unknown_enqueuer_is_a_remediation_error(self) -> None: self.assertEqual(len(dequeues), 1) self.assertIn("id=PR_a", dequeues[0]) + def armed_pulls(self) -> None: + # #9518: only a lanes App pull request the lanes App armed is disarmed. + self.gh.open_pulls = [ + armed_pull("PR_lane", 7020, author=APP_ACTOR, armer=APP_ACTOR), + armed_pull("PR_person_armed", 7021, author=APP_ACTOR, armer=HUMAN), + armed_pull("PR_person_pr", 7022, author=HUMAN, armer=APP_ACTOR), + armed_pull("PR_unarmed", 7023, author=APP_ACTOR, armer=None), + ] + + def disarms(self) -> list[list[str]]: + return [ + w for w in self.gh.writes() if "disablePullRequestAutoMerge" in " ".join(w) + ] + + def test_remediate_disarms_only_the_lanes_own_armed_pull_requests(self) -> None: + verdict = self.failing_verdict() + self.armed_pulls() + self.gh.queue = [ + {"enqueuer": APP_ACTOR, "pullRequest": {"id": "PR_a", "number": 7010}} + ] + report = pm.remediate(REPO, verdict, agent_identity=IDENTITY) + self.assertEqual(report["disarmed"], [7020]) + self.assertEqual(report["dequeued"], [7010]) + self.assertEqual(report["errors"], []) + disarms = self.disarms() + self.assertEqual(len(disarms), 1) + self.assertIn("id=PR_lane", disarms[0]) + # Disarmed before the dequeue, so none enters the queue after its read. + writes = [" ".join(w) for w in self.gh.writes()] + self.assertIn("disablePullRequestAutoMerge", writes[0]) + self.assertIn("dequeuePullRequest", writes[1]) + + def test_a_failed_disarm_is_an_error_and_the_rest_still_run(self) -> None: + verdict = self.failing_verdict() + self.gh.open_pulls = [ + armed_pull("PR_x", 7030, author=APP_ACTOR, armer=APP_ACTOR), + armed_pull("PR_y", 7031, author=APP_ACTOR, armer=APP_ACTOR), + ] + self.gh.queue = [ + {"enqueuer": APP_ACTOR, "pullRequest": {"id": "PR_a", "number": 7010}} + ] + self.gh.fail_reads = {"id=PR_x"} + report = pm.remediate(REPO, verdict, agent_identity=IDENTITY) + self.assertEqual(report["disarmed"], [7031]) + self.assertEqual(len(report["errors"]), 1) + self.assertTrue(report["errors"][0].startswith("disarm: disarm #7030")) + self.assertEqual(report["dequeued"], [7010]) + self.assertEqual(report["issues"], [9001]) + + def test_a_failed_dequeue_read_still_disarms(self) -> None: + verdict = self.failing_verdict() + self.armed_pulls() + self.gh.fail_reads = {"PostMergeQueue"} + report = pm.remediate(REPO, verdict, agent_identity=IDENTITY) + self.assertEqual(report["disarmed"], [7020]) + self.assertTrue(any(e.startswith("dequeue:") for e in report["errors"])) + + def test_an_unreadable_armed_listing_is_an_error_and_still_dequeues( + self, + ) -> None: + verdict = self.failing_verdict() + self.gh.queue = [ + {"enqueuer": APP_ACTOR, "pullRequest": {"id": "PR_a", "number": 7010}} + ] + self.gh.fail_reads = {"PostMergeArmed"} + report = pm.remediate(REPO, verdict, agent_identity=IDENTITY) + self.assertEqual(report["disarmed"], []) + self.assertTrue(report["errors"][0].startswith("disarm: armed pull requests")) + self.assertEqual(report["dequeued"], [7010]) + + def test_no_failure_disarms_nothing(self) -> None: + self.gh.merged = [identity_merge()] + self.all_green() + verdict = self.check() + self.assertFalse(verdict.failures) + self.armed_pulls() + report = pm.remediate(REPO, verdict, agent_identity=IDENTITY) + self.assertEqual(report["disarmed"], []) + self.assertFalse(any("PostMergeArmed" in " ".join(c) for c in self.gh.calls)) + self.assertEqual(self.disarms(), []) + def test_a_second_sighting_under_one_marker_reopens_a_closed_record( self, ) -> None: From 6a70105e3c4ec3ec8a68c1bd0155d9b0a046d4e8 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 21:37:23 -0400 Subject: [PATCH 2/3] fix(source-control): report a failed disarm as a lane merge error The "Record a failed lane merge" step now errors on a disarm: entry in the remediation report, as it does for demotion issue: and dequeue: entries. Also drops an issue number from a test comment. Co-Authored-By: Claude Opus 5.5 --- .github/scripts/pr-merge/merge-job.test.mjs | 2 +- .github/workflows/pr-merge.yml | 4 ++-- .../babysit-prs/scripts/tests/test_babysit_postmerge.py | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/scripts/pr-merge/merge-job.test.mjs b/.github/scripts/pr-merge/merge-job.test.mjs index 26b7f8d4e1..00eb3c6aea 100644 --- a/.github/scripts/pr-merge/merge-job.test.mjs +++ b/.github/scripts/pr-merge/merge-job.test.mjs @@ -586,7 +586,7 @@ test("the record step warns on refused re-runs and errors when the record was no assert.equal(refused.status, 0); assert.match(refused.stdout, /^::warning::the post-merge remediation exited 4 /m); assert.doesNotMatch(refused.stdout, /::error::/); - for (const error of ["demotion issue: HTTP 403", "dequeue: HTTP 502"]) { + for (const error of ["demotion issue: HTTP 403", "dequeue: HTTP 502", "disarm: HTTP 502"]) { const failed = runRecord(4, { remediation: { errors: ["re-run: HTTP 403", error] } }); assert.equal(failed.status, 0); assert.match(failed.stdout, /^::error::the post-merge remediation could not file the demotion record/m); diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index 9efdc2aae5..ea0dab71b1 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -825,8 +825,8 @@ jobs: # the report, not the exit code, says whether the record was filed. if ! jq -e '.remediation.errors | type == "array"' "$report" >/dev/null 2>&1; then echo "::error::the post-merge remediation exited $code with no remediation report; no demotion record was filed" - elif jq -e '[.remediation.errors[] | select(startswith("demotion issue:") or startswith("dequeue:"))] | length > 0' "$report" >/dev/null; then - echo "::error::the post-merge remediation could not file the demotion record or dequeue the lane's entries; see its report above" + elif jq -e '[.remediation.errors[] | select(startswith("demotion issue:") or startswith("dequeue:") or startswith("disarm:"))] | length > 0' "$report" >/dev/null; then + echo "::error::the post-merge remediation could not file the demotion record, dequeue the lane's entries or disarm its pull requests; see its report above" elif [ "$code" -eq 4 ]; then echo "::warning::the post-merge remediation exited $code (re-run or dispatch refused: the lane token has no Actions permission)" fi diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge.py index 7155658671..9893a91196 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge.py @@ -2382,7 +2382,7 @@ def test_an_entry_with_an_unknown_enqueuer_is_a_remediation_error(self) -> None: self.assertIn("id=PR_a", dequeues[0]) def armed_pulls(self) -> None: - # #9518: only a lanes App pull request the lanes App armed is disarmed. + # Only a lanes App pull request the lanes App armed is disarmed. self.gh.open_pulls = [ armed_pull("PR_lane", 7020, author=APP_ACTOR, armer=APP_ACTOR), armed_pull("PR_person_armed", 7021, author=APP_ACTOR, armer=HUMAN), From 0c6293044fbbcc421ef057658e7e7c7bb076fd4f Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 21:49:01 -0400 Subject: [PATCH 3/3] fix(source-control): harden the remediation's armed pull request read A malformed page of the armed listing is now a read error instead of an empty listing, and a listing past the page limit still disarms what it read before reporting the error. babysit_merge.py reuses the postmerge disarm mutation, and the pr-merge.yml comment names the disarm. New tests cover the cursor, the judged branch, a malformed page and the page limit. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/pr-merge.yml | 6 +-- .../babysit-prs/scripts/babysit_merge.py | 5 +- .../babysit-prs/scripts/babysit_postmerge.py | 23 +++++--- .../scripts/tests/test_babysit_postmerge.py | 53 ++++++++++++++++++- 4 files changed, 72 insertions(+), 15 deletions(-) diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index 4d776f8d38..415e65bf31 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -797,9 +797,9 @@ jobs: # hears of a full run this workflow dispatched only when the push job # sends it, and not of one the merge job or post-merge-dispatch started: # a run started with GITHUB_TOKEN raises no workflow_run event. So the check runs again - # with --remediate under the lane token, which files the record (or - # comments on it, reopening it on a new sighting) and dequeues the - # lane's merge-queue entries. The lane token holds no Actions + # with --remediate under the lane token, which disarms the lane's armed + # pull requests, dequeues the lane's merge-queue entries and files the + # record (or comments on it, reopening it on a new sighting). The lane token holds no Actions # permission, so the re-run and dispatch --remediate also attempts fail # here and are reported as a write failure (exit 4); the # post-merge-dispatch job starts those. Live mode only. 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 8aa37191fd..d298786dbf 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -2921,10 +2921,7 @@ def queued_entry_hold( "mergeQueue(branch:$b){entries(first:100){totalCount nodes{enqueuedAt position " "enqueuer{__typename login} pullRequest{id number}}}}}}" ) -DISARM_MUTATION = ( - "mutation($id:ID!){disablePullRequestAutoMerge(input:{pullRequestId:$id})" - "{clientMutationId}}" -) +DISARM_MUTATION = postmerge.DISARM_MUTATION _CONTROL = re.compile(r"[\x00-\x08\x0b-\x1f\x7f-\x9f

]") 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 e99a8639c1..00a0b14c0b 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py @@ -2288,18 +2288,24 @@ def disarm_identity_pulls( that the agent identity both opened and armed, appending each number to `disarmed` as it goes. A pull request a person armed, or one the identity did not open, is left armed. Every one is tried; a failed one does not - stop the rest. Raises `PostMergeReadError` on a read that is not every - open pull request, or `PostMergeWriteError` naming every failed one.""" + stop the rest. A read past the page limit still disarms what it found, + then raises `PostMergeReadError`; otherwise `PostMergeWriteError` names + every failed one.""" owner, name = _split_repo(repo) key = _login_key(identity) armed: list[tuple[str, int]] = [] cursor = None + overflow = False for _ in range(ARMED_PAGE_LIMIT): data = _graphql( ARMED_QUERY, "armed pull requests", o=owner, r=name, b=branch, c=cursor ) pulls = json_object(json_object(data.get("repository")).get("pullRequests")) - for pr in (json_object(n) for n in json_array(pulls.get("nodes"))): + nodes = pulls.get("nodes") + page = pulls.get("pageInfo") + if not is_json_array(nodes) or not is_json_object(page): + raise PostMergeReadError("armed pull requests: the listing did not read") + for pr in (json_object(n) for n in nodes): request = pr.get("autoMergeRequest") if ( is_json_object(request) @@ -2307,14 +2313,11 @@ def disarm_identity_pulls( and _actor_key(request.get("enabledBy")) == key ): armed.append((str(pr.get("id")), pr.get("number"))) - page = json_object(pulls.get("pageInfo")) if not page.get("hasNextPage"): break cursor = page.get("endCursor") else: - raise PostMergeReadError( - f"armed pull requests: more than {ARMED_PAGE_LIMIT} pages of open pull requests" - ) + overflow = True failed: list[str] = [] for pr_id, number in armed: try: @@ -2326,6 +2329,12 @@ def disarm_identity_pulls( continue if disarmed is not None: disarmed.append(number) + if overflow: + failed.insert( + 0, + f"armed pull requests: more than {ARMED_PAGE_LIMIT} pages of open pull requests", + ) + raise PostMergeReadError("; ".join(failed)) if failed: raise PostMergeWriteError("; ".join(failed)) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge.py index 9893a91196..95b6854c55 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge.py @@ -194,6 +194,10 @@ def __init__(self) -> None: self.queue: list[dict[str, Any]] = [] # Open pull requests into the branch, as the armed read returns them. self.open_pulls: list[dict[str, Any]] = [] + # True: every armed page says another follows. + self.open_pulls_endless = False + # Armed pages served in order, one per read, when set. + self.armed_pages: list[dict[str, Any]] = [] self.issues: list[dict[str, Any]] = [] self.comments: dict[int, list[dict[str, Any]]] = {} self.fail_reads: set[str] = set() @@ -402,9 +406,13 @@ def _graphql(self, joined: str) -> subprocess.CompletedProcess[str]: return completed( {"data": {"disablePullRequestAutoMerge": {"clientMutationId": None}}} ) + if "PostMergeArmed" in joined and self.armed_pages: + return completed( + {"data": {"repository": {"pullRequests": self.armed_pages.pop(0)}}} + ) if "PostMergeArmed" in joined: page = { - "pageInfo": {"hasNextPage": False, "endCursor": None}, + "pageInfo": {"hasNextPage": self.open_pulls_endless, "endCursor": "c"}, "nodes": self.open_pulls, } return completed({"data": {"repository": {"pullRequests": page}}}) @@ -2451,6 +2459,49 @@ def test_an_unreadable_armed_listing_is_an_error_and_still_dequeues( self.assertTrue(report["errors"][0].startswith("disarm: armed pull requests")) self.assertEqual(report["dequeued"], [7010]) + def test_an_armed_listing_past_the_page_limit_still_disarms_what_it_read( + self, + ) -> None: + verdict = self.failing_verdict() + self.armed_pulls() + self.gh.open_pulls_endless = True + report = pm.remediate(REPO, verdict, agent_identity=IDENTITY) + self.assertTrue(len(self.disarms()) >= 1) + self.assertIn(7020, report["disarmed"]) + self.assertTrue( + report["errors"][0].startswith("disarm: armed pull requests: more than") + ) + + def test_the_armed_listing_follows_the_cursor_on_the_judged_branch( + self, + ) -> None: + verdict = self.failing_verdict() + self.gh.armed_pages = [ + { + "pageInfo": {"hasNextPage": True, "endCursor": "c1"}, + "nodes": [armed_pull("PR_p1", 7040, author=HUMAN, armer=HUMAN)], + }, + { + "pageInfo": {"hasNextPage": False, "endCursor": None}, + "nodes": [armed_pull("PR_p2", 7041, author=APP_ACTOR, armer=APP_ACTOR)], + }, + ] + report = pm.remediate(REPO, verdict, agent_identity=IDENTITY) + self.assertEqual(report["disarmed"], [7041]) + reads = [c for c in self.gh.calls if "PostMergeArmed" in " ".join(c)] + self.assertEqual(len(reads), 2) + self.assertIn("c=c1", reads[1]) + self.assertIn(f"b={verdict.branch}", reads[0]) + + def test_a_malformed_armed_page_is_an_error(self) -> None: + verdict = self.failing_verdict() + self.gh.armed_pages = [{"pageInfo": {"hasNextPage": False}, "nodes": None}] + report = pm.remediate(REPO, verdict, agent_identity=IDENTITY) + self.assertEqual(report["disarmed"], []) + self.assertTrue( + report["errors"][0].startswith("disarm: armed pull requests: the listing") + ) + def test_no_failure_disarms_nothing(self) -> None: self.gh.merged = [identity_merge()] self.all_green()