From 14f240a5f0fa1287a8c2fdadc6541d01379b6b05 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 2 Nov 2026 13:57:39 -0500 Subject: [PATCH 1/2] fix(source-control): keep a queued entry whose verdict the sweep cannot read The merge job's queued-entry sweep read verify:unreadable as a hold and dequeued the entry. When the job's whole artifact listing failed, it removed lanes App PRs that enable-auto-merge had just armed on a pass verdict, and they cycled in and out of the queue (#12432, #12435 in run 41496022715). The merge queue's own reader refuses an unreadable verdict fail-closed, so the sweep now leaves the entry and does not count it as kept. Co-Authored-By: Claude Opus 5.5 --- ...x-sweep-unreadable-keeps-entry-3e8a51c0.md | 7 +++ .../babysit-prs/scripts/babysit_merge.py | 12 +++-- .../tests/test_babysit_merge_unattended.py | 47 +++++++++++++++++++ 3 files changed, 63 insertions(+), 3 deletions(-) create mode 100644 .changes/source-control/fix-sweep-unreadable-keeps-entry-3e8a51c0.md diff --git a/.changes/source-control/fix-sweep-unreadable-keeps-entry-3e8a51c0.md b/.changes/source-control/fix-sweep-unreadable-keeps-entry-3e8a51c0.md new file mode 100644 index 00000000000..6edc13e4f5a --- /dev/null +++ b/.changes/source-control/fix-sweep-unreadable-keeps-entry-3e8a51c0.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- `babysit-prs`: the pr-merge merge job's queued-entry sweep no longer dequeues an entry whose verifier verdict it could not read (`verify:unreadable`). The merge queue's own verdict reader refuses that entry fail-closed, so the sweep leaves it queued and does not count it as kept. When the merge job's whole artifact listing failed, the sweep used to remove lanes App entries that enable-auto-merge had just armed on a pass, and they cycled in and out of the queue. 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 150d98f15a9..4ceef806d7c 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -4254,8 +4254,8 @@ def queue_ejection_body(head: str, count: int, runs: list[str] | None) -> str: def _queued_verify_hold(verifier: verify_evidence.Reader, number: int) -> str | None: - """One queued entry's verifier hold. An unexpected error refuses that - entry, so it leaves the queue and the sweep goes on to the next.""" + """One queued entry's verifier hold. An unexpected error reads as + unreadable for that entry alone, and the sweep goes on to the next.""" try: return verifier.queued_hold(number) except Exception: # noqa: BLE001 -- any crash refuses this entry alone @@ -4316,7 +4316,8 @@ def unattended_writes( else: queued, _ = postmerge.identity_queue(repo, branch, identity) # In the pr-merge lane a queued entry also leaves without a - # passing verifier verdict at its head now. + # passing verifier verdict at its head now; one whose verdict + # cannot be read stays for the merge queue's own reader. config = inputs.get("config") verifier = inputs.get("verify_reader") or ( verify_evidence.Reader( @@ -4350,6 +4351,11 @@ def judge(n: int) -> str | None: if verifier is not None and n != number else None ) + if hold == verify_evidence.UNREADABLE: + # A verdict this run could not read is no hold: the merge + # queue's own reader refuses it fail-closed, so the entry + # stays, neither dequeued nor counted as kept. + return None kept_at[n] = min([began, *taken]) return hold 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 1d1db2f6e1f..8faa4e458f4 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 @@ -5511,6 +5511,53 @@ def tearDownModule() -> None: _role_label.stop() +class AnUnreadableVerdictKeepsAQueuedEntry(unittest.TestCase): + """The queued-entry sweep dequeues an entry its verdict holds, but keeps + one whose verdict it could not read: the merge queue's own reader refuses + that fail-closed. Run 41496022715 dequeued #12432 and #12435 on an + unreadable listing while enable-auto-merge re-armed them on a pass.""" + + REPO = "owner/repo" + + def test_only_a_read_hold_dequeues(self) -> None: + holds = {5: "verify:fail", 6: merge.verify_evidence.UNREADABLE, 8: None} + reader = mock.Mock() + reader.queued_hold.side_effect = holds.get + dequeued: list[set[int] | None] = [] + kept: list[str] = [] + with ( + mock.patch.object( + postmerge, "identity_queue", return_value=([5, 6, 8], 0) + ), + mock.patch.object( + postmerge, + "dequeue_identity_entries", + side_effect=lambda *a, numbers, **k: dequeued.append(numbers) or 0, + ), + mock.patch.object(policy, "entry_hold", return_value=None), + mock.patch.object( + run_cache, "put", side_effect=lambda ns, key, at: kept.append(key) + ), + ): + out = merge.unattended_writes( + self.REPO, + 7, + policy.UnattendedPolicy( + "ineligible:x", + "agent-lanes[bot]", + {}, + inputs={ + "identity": "agent-lanes[bot]", + "branch": "main", + "verify_reader": reader, + }, + ), + ) + self.assertEqual(out["holds"], {5: "verify:fail"}) + self.assertEqual(dequeued, [{5}]) + self.assertEqual([k.rsplit("|", 1)[1] for k in kept], ["8"]) + + class DependabotMajorEscalation(unittest.TestCase): """A Dependabot major held under `merge.dependabot.majors: hold` gets the escalation contract: one marked comment, then the tracker's human-gated From cca22b9e65dd06c8fc99e503599fc56aeb268be3 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 2 Nov 2026 14:04:10 -0500 Subject: [PATCH 2/2] fix(source-control): keep only lanes App entries the queue re-reads The merge queue's ci-status reads the verdict only for lanes App pull requests and exits before any read for every other author, so keeping every unreadable entry let an adopted pull request merge unread. queued_hold now answers verify:queue-reads only for an unreadable verdict on a lanes App entry (id and Bot type, as ci-status matches it), and the sweep keeps only that one; every other unreadable entry and a crash still dequeue. Co-Authored-By: Claude Opus 5.5 --- ...x-sweep-unreadable-keeps-entry-3e8a51c0.md | 2 +- .../babysit-prs/scripts/babysit_merge.py | 17 ++++---- .../scripts/babysit_verify_evidence.py | 24 +++++++++++- .../tests/test_babysit_merge_unattended.py | 23 +++++++---- .../tests/test_babysit_verify_evidence.py | 39 +++++++++++++++++++ 5 files changed, 87 insertions(+), 18 deletions(-) diff --git a/.changes/source-control/fix-sweep-unreadable-keeps-entry-3e8a51c0.md b/.changes/source-control/fix-sweep-unreadable-keeps-entry-3e8a51c0.md index 6edc13e4f5a..c30c55425ce 100644 --- a/.changes/source-control/fix-sweep-unreadable-keeps-entry-3e8a51c0.md +++ b/.changes/source-control/fix-sweep-unreadable-keeps-entry-3e8a51c0.md @@ -4,4 +4,4 @@ bump: patch ### Fixed -- `babysit-prs`: the pr-merge merge job's queued-entry sweep no longer dequeues an entry whose verifier verdict it could not read (`verify:unreadable`). The merge queue's own verdict reader refuses that entry fail-closed, so the sweep leaves it queued and does not count it as kept. When the merge job's whole artifact listing failed, the sweep used to remove lanes App entries that enable-auto-merge had just armed on a pass, and they cycled in and out of the queue. +- `babysit-prs`: the pr-merge merge job's queued-entry sweep no longer dequeues a lanes App entry just because this run could not read its pr-verify verdict. The merge queue's ci-status reads a lanes App pull request's verdict itself and refuses it fail-closed, so the reader answers `verify:queue-reads` and the entry stays, not counted as kept. Before, when the merge job's whole artifact listing failed, the sweep removed lanes App entries that enable-auto-merge had just armed on a pass, and they cycled in and out of the queue. Every other unreadable entry still leaves, adopted pull requests included, since the queue reads no verdict for them. 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 4ceef806d7c..7d254c255a6 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -4254,8 +4254,8 @@ def queue_ejection_body(head: str, count: int, runs: list[str] | None) -> str: def _queued_verify_hold(verifier: verify_evidence.Reader, number: int) -> str | None: - """One queued entry's verifier hold. An unexpected error reads as - unreadable for that entry alone, and the sweep goes on to the next.""" + """One queued entry's verifier hold. An unexpected error refuses that + entry, so it leaves the queue and the sweep goes on to the next.""" try: return verifier.queued_hold(number) except Exception: # noqa: BLE001 -- any crash refuses this entry alone @@ -4316,8 +4316,9 @@ def unattended_writes( else: queued, _ = postmerge.identity_queue(repo, branch, identity) # In the pr-merge lane a queued entry also leaves without a - # passing verifier verdict at its head now; one whose verdict - # cannot be read stays for the merge queue's own reader. + # passing verifier verdict at its head now; a lanes App entry + # whose verdict cannot be read stays for the merge queue's own + # reader (`verify_evidence.QUEUE_READS`). config = inputs.get("config") verifier = inputs.get("verify_reader") or ( verify_evidence.Reader( @@ -4351,10 +4352,10 @@ def judge(n: int) -> str | None: if verifier is not None and n != number else None ) - if hold == verify_evidence.UNREADABLE: - # A verdict this run could not read is no hold: the merge - # queue's own reader refuses it fail-closed, so the entry - # stays, neither dequeued nor counted as kept. + if hold == verify_evidence.QUEUE_READS: + # A lanes App entry whose verdict this run could not read: + # the merge queue's own reader refuses it fail-closed, so + # it stays, neither dequeued nor counted as kept. return None kept_at[n] = min([began, *taken]) return hold diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py index b888571f0c5..8f0bb2b4f3a 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py @@ -576,6 +576,9 @@ def _overcount_refusal(outcome: str) -> str: REPEATED_FAIL = "verify:repeated-fail" MISSING = "verify:missing" UNREADABLE = "verify:unreadable" +# `Reader.queued_hold`: a lanes App entry whose verdict this run could not +# read. The merge queue's own reader judges it fail-closed, so it is no hold. +QUEUE_READS = "verify:queue-reads" TOO_OLD = "verify:too-old" UNTRUSTED_SPEC = "verify:untrusted-spec" SPEC_CHANGED = "verify:spec-changed" @@ -3953,7 +3956,10 @@ def _pass_refusal( def queued_hold(self, number: int) -> str | None: """Why a queued entry must leave: no allowing verdict at its live - head now. An unreadable pull request is a hold. A dependency + head now. An unreadable pull request is a hold, but an unreadable + verdict on a lanes App entry is `QUEUE_READS`: the merge queue's + ci-status reads that entry's verdict itself and refuses it + fail-closed, while it reads none for any other author. A dependency manager's entry is the lane's Dependabot class, which pr-verify never judges, so it is not held here.""" try: @@ -3963,7 +3969,21 @@ def queued_hold(self, number: int) -> str | None: if dependency_bot(pr.get("user")): return None head = str(json_object(pr.get("head")).get("sha")) - return self.evidence(number, head, pr).refusal + refusal = self.evidence(number, head, pr).refusal + if refusal == UNREADABLE and self._queue_reads(pr): + return QUEUE_READS + return refusal + + def _queue_reads(self, pr: dict[str, Any]) -> bool: + """Whether the merge queue's ci-status reads this pull request's + verdict: the lanes App's, matched by id and type as ci-status + matches it. A lanes App id that cannot be read is a no.""" + user = json_object(pr.get("user")) + try: + app_id = self._lane_app_id() + except Exception: # noqa: BLE001 -- any failure keeps the hold + return False + return app_id is not None and user.get("id") == app_id and user.get("type") == "Bot" def lane_risk_refusal( 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 8faa4e458f4..41faa72053b 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 @@ -5513,21 +5513,28 @@ def tearDownModule() -> None: class AnUnreadableVerdictKeepsAQueuedEntry(unittest.TestCase): """The queued-entry sweep dequeues an entry its verdict holds, but keeps - one whose verdict it could not read: the merge queue's own reader refuses - that fail-closed. Run 41496022715 dequeued #12432 and #12435 on an - unreadable listing while enable-auto-merge re-armed them on a pass.""" + a lanes App entry whose verdict it could not read (`QUEUE_READS`): the + merge queue's own reader refuses that fail-closed. Any other unreadable + entry still leaves, since the queue reads no verdict for it. Run + 41496022715 dequeued #12432 and #12435 on an unreadable listing while + enable-auto-merge re-armed them on a pass.""" REPO = "owner/repo" def test_only_a_read_hold_dequeues(self) -> None: - holds = {5: "verify:fail", 6: merge.verify_evidence.UNREADABLE, 8: None} + holds = { + 5: "verify:fail", + 6: merge.verify_evidence.QUEUE_READS, + 8: None, + 9: merge.verify_evidence.UNREADABLE, + } reader = mock.Mock() reader.queued_hold.side_effect = holds.get dequeued: list[set[int] | None] = [] kept: list[str] = [] with ( mock.patch.object( - postmerge, "identity_queue", return_value=([5, 6, 8], 0) + postmerge, "identity_queue", return_value=([5, 6, 8, 9], 0) ), mock.patch.object( postmerge, @@ -5553,8 +5560,10 @@ def test_only_a_read_hold_dequeues(self) -> None: }, ), ) - self.assertEqual(out["holds"], {5: "verify:fail"}) - self.assertEqual(dequeued, [{5}]) + self.assertEqual( + out["holds"], {5: "verify:fail", 9: merge.verify_evidence.UNREADABLE} + ) + self.assertEqual(dequeued, [{5, 9}]) self.assertEqual([k.rsplit("|", 1)[1] for k in kept], ["8"]) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_evidence.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_evidence.py index ba084ca7251..03789aa8145 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_evidence.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_evidence.py @@ -2884,6 +2884,45 @@ def test_a_record_whose_issues_are_not_a_list_refuses(self) -> None: self.assertEqual(read(fake).refusal, "verify:unreadable") +class AnUnreadableQueuedLanesEntryIsLeftToTheQueue(unittest.TestCase): + """An unreadable verdict on a queued lanes App entry is `QUEUE_READS`, + since the merge queue's ci-status reads that entry's verdict itself; on + any other author, or when the App's id cannot be read, it stays a hold.""" + + APP = 4242 + + def hold(self, user: dict[str, Any], app: Any = APP) -> str | None: + unreadable = ve.Evidence(ve.UNREADABLE, HEAD, (), 0) + lane_id = ( + mock.patch.object(ve.Reader, "_lane_app_id", side_effect=app) + if isinstance(app, BaseException) + else mock.patch.object(ve.Reader, "_lane_app_id", return_value=app) + ) + with ( + mock.patch.object(policy, "pull_request", return_value=pull(user=user)), + mock.patch.object(ve.Reader, "evidence", return_value=unreadable), + lane_id, + ): + return ve.Reader(REPO, "main", now=NOW).queued_hold(PR) + + def test_only_the_lanes_apps_entry_is_left_to_the_queue(self) -> None: + cases: dict[str, tuple[dict[str, Any], Any, str]] = { + "lanes App": ({"id": self.APP, "type": "Bot"}, self.APP, ve.QUEUE_READS), + "owner": ({"id": 7, "type": "User"}, self.APP, ve.UNREADABLE), + "same id, not a Bot": ({"id": self.APP, "type": "User"}, self.APP, ve.UNREADABLE), + "other App": ({"id": 99, "type": "Bot"}, self.APP, ve.UNREADABLE), + "no App id": ({"id": self.APP, "type": "Bot"}, None, ve.UNREADABLE), + "App id unreadable": ( + {"id": self.APP, "type": "Bot"}, + RuntimeError("trusted list"), + ve.UNREADABLE, + ), + } + for label, (user, app, want) in cases.items(): + with self.subTest(label): + self.assertEqual(self.hold(user, app), want) + + class AQueuedDependabotEntryNeedsNoVerdict(unittest.TestCase): """The pr-merge lane enqueues Dependabot pull requests in their own class (babysit_dependabot), which pr-verify never judges."""