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..c30c55425ce --- /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 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 150d98f15a9..7d254c255a6 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -4316,7 +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. + # 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( @@ -4350,6 +4352,11 @@ def judge(n: int) -> str | None: if verifier is not None and n != number else None ) + 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 1d1db2f6e1f..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 @@ -5511,6 +5511,62 @@ def tearDownModule() -> None: _role_label.stop() +class AnUnreadableVerdictKeepsAQueuedEntry(unittest.TestCase): + """The queued-entry sweep dequeues an entry its verdict holds, but keeps + 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.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, 9], 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", 9: merge.verify_evidence.UNREADABLE} + ) + self.assertEqual(dequeued, [{5, 9}]) + 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 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."""