diff --git a/.changes/pr-pipeline/fix-pr-merge-gate-late-pass-reread-8b2f5c61.md b/.changes/pr-pipeline/fix-pr-merge-gate-late-pass-reread-8b2f5c61.md new file mode 100644 index 00000000000..8037d89c740 --- /dev/null +++ b/.changes/pr-pipeline/fix-pr-merge-gate-late-pass-reread-8b2f5c61.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- The README's verifier-verdict section states the pr-merge gate's one exception to a refusal standing on the run's saved listing: a candidate's `verify:missing` there reads the head's two pass names by exact name (`pr-verify---pass` and `-pass-notes`), two requests and no listing, so a pass uploaded after that listing is seen in the same run. Its allow is judged again on a listing of the gate's own, and a failed read or no pass that counts leaves `verify:missing`. diff --git a/.changes/source-control/fix-pr-merge-gate-late-pass-reread-4d7a1e93.md b/.changes/source-control/fix-pr-merge-gate-late-pass-reread-4d7a1e93.md new file mode 100644 index 00000000000..2192fe05ee8 --- /dev/null +++ b/.changes/source-control/fix-pr-merge-gate-late-pass-reread-4d7a1e93.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- The pr-merge gate's verdict read (`babysit_verify_evidence.py` `Reader.evidence`, provisional) no longer refuses `verify:missing` for a pass uploaded after the run's saved artifact listing was read. When that refusal still stands on the saved listing, the reader reads the head's two pass names by exact name (`pr-verify---pass` and `-pass-notes`), two requests and no listing. A pass found there is judged with the saved listing in memory, and an allow stays provisional, so the gate judges it again on a listing of its own that must hold the live count. A failed read, a pass whose run does not count, or none found leaves `verify:missing`. Each find prints one `babysit-late-pass` line. diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 98ef310f40e..0155576f859 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -2640,6 +2640,11 @@ listing there, and later candidates and their queued-entry checks read it instea No judgment, of a candidate or a queued entry, uses a listing (saved or in memory) begun more than two minutes before it: an older one is read again and saved. A saved listing can only lack artifacts uploaded since, so a refusal on it stands; a candidate it would allow is judged again on a listing of its own before it is allowed, which is saved in turn. +The one exception is a candidate's `verify:missing` on the saved listing: its gate reads the head's +two pass names by exact name (`pr-verify---pass` and `-pass-notes`), two requests and no +listing, and judges a pass found there with the saved listing, so a pass uploaded after that listing +is seen in the same run. Its allow is judged again on a listing of its own like any other; a failed +read, or no pass that counts, leaves `verify:missing`. A listing that got a 5xx or a fast network error is read again, three reads in all with backoff, all inside the one gh timeout a single read would get; any other failure, a timeout, a 429 or a rate-limit 403 included, is not. A failed listing is never 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 be91281c10e..0987e06d3b6 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 @@ -194,6 +194,11 @@ refusal no later upload clears (`STANDING`: a FAIL at the head, the FAIL cap). Any other refusal is judged again on a recent listing, so none dequeues the candidate's queued entry on an older one. A +`verify:missing` that still stands on the run's saved listing reads the head's +two pass names by exact name (`Reader._late_pass`), two requests and no +listing, so a pass uploaded after that listing was read is seen in the same +run: it is judged with that listing in memory, and an allow stays provisional, +judged again on the gate's own listing. A candidate the gate refuses for anything else lists nothing more; queued entries are judged on a recent listing. A listing that got a 5xx or a fast network error is read again after each of `LISTING_WAITS`, every read inside one @@ -433,6 +438,9 @@ def no_incremental_waits() -> bool: # The prefix of the stderr line `Reader._incremental` prints for each read # that missed the live count, like babysit_gh's `babysit-gh-calls`. LISTING_MISS_PREFIX = "babysit-listing-miss" +# The stderr line `Reader._late_pass` prints when its name reads find a pass +# the saved listing lacked: how many, and whether the judgment then allows. +LATE_PASS_PREFIX = "babysit-late-pass" class _CountMissed: @@ -2596,17 +2604,84 @@ def evidence( (babysit_merge.py `refresh()`), takes the listing at hand at any age: an allow or a refusal no later upload clears (`STANDING`) stands on it, and any other refusal on an older listing is judged again on a - recent one.""" + recent one. A provisional `verify:missing` still on the run's saved + listing reads this pull request's pass names (`_late_pass`).""" judged = self._judge(number, head, pr, any_age=provisional) + if not provisional: + return judged if ( - not provisional - or judged.refusal is None - or judged.refusal in STANDING - or self._fetched_at is None - or _recent(self._fetched_at, self.listing_max_age) + judged.refusal is not None + and judged.refusal not in STANDING + and self._fetched_at is not None + and not _recent(self._fetched_at, self.listing_max_age) ): - return judged - return self._judge(number, head, pr, any_age=False) + judged = self._judge(number, head, pr, any_age=False) + if judged.refusal == MISSING and self._saved: + return self._late_pass(number, head, pr, judged) + return judged + + def _late_pass( + self, number: int, head: str, pr: dict[str, Any] | None, missing: Evidence + ) -> Evidence: + """The provisional judgment at `head` with any pass uploaded since the + run's saved listing was read, else `missing`. Only the exact names + write-verdict.mjs `artifactName` writes for a pass at the head are + read, `pr-verify---` for each of `PASSES`, one + `?name=` read each, as resolve-pr.mjs `judged` looks them up; no + listing. The writer names no pass with the head-ref digest yet; when + it moves to one, such a pass leaves `missing` here until this reads + that name too. An unexpired one the saved listing lacks is judged + with it, in memory: an allow stands only as a provisional one, so + babysit_merge.py `refresh()` judges it again on a listing of the + gate's own, which must hold the live count. A refusal on it, a failed + or partial read, or no such artifact leaves `missing`, and nothing is + noted for the job.""" + held = {a.get("id") for a in self._artifacts or ()} + transient = self._page_transient + deadline = _listing_clock() + listing_cap()[0] + found: list[dict[str, Any]] = [] + try: + for verdict in PASSES: + name = f"pr-verify-{number}-{head}-{verdict}" + read = self._api_object( + f"repos/{self.repo}/actions/artifacts" + f"?name={name}&per_page={LISTING_PAGE_SIZE}", + deadline, + ) + if not isinstance(read, tuple): + return missing + items = read[0].get("artifacts") + if ( + not is_json_array(items) + or read[0].get("total_count") != len(items) + or not all(map(_listed_entry, items)) + ): + return missing + found += [ + a + for a in items + if a.get("name") == name + and a["expired"] is False + and a["id"] not in held + ] + finally: + self._page_transient = transient + if not found: + return missing + listing = self._artifacts or [] + self._artifacts = [*found, *listing] + try: + late = self._judge(number, head, pr, any_age=True) + finally: + self._artifacts = listing + print( + f"{LATE_PASS_PREFIX} " + + json.dumps( + {"pr": number, "found": len(found), "allowed": late.refusal is None} + ), + file=sys.stderr, + ) + return late if late.refusal is None else missing def _judge( self, number: int, head: str, pr: dict[str, Any] | None, *, any_age: bool 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 f75639534f5..00f70dc273f 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 @@ -466,6 +466,13 @@ def rest( # One artifact by id, as GitHub's "Get an artifact" answers. held = [a for a in self.artifacts if a.get("id") == int(found[1])] return ok(held[0]) if held else missing() + by_name = re.fullmatch( + rf"repos/{REPO}/actions/artifacts\?name=([^&]+)&per_page=100", path + ) + if by_name: + # Every artifact with exactly that name, as GitHub's filter answers. + held = [a for a in self.artifacts if a.get("name") == by_name[1]] + return ok({"total_count": len(held), "artifacts": held}) prefix = f"repos/{REPO}/actions/artifacts?per_page=" if path.startswith(prefix) and "&page=" in path: if self.page_fail: @@ -2977,6 +2984,11 @@ def listings(fake: FakeGitHub) -> int: return sum(any(a.endswith("&page=1") for a in c) for c in fake.calls) +def name_reads(fake: FakeGitHub) -> int: + """Reads of artifacts by exact name (`Reader._late_pass`).""" + return sum(any("actions/artifacts?name=" in a for a in c) for c in fake.calls) + + class TheListingIsReadOncePerRun(unittest.TestCase): """pr-merge.yml runs one gate, so one `Reader`, per candidate and names a file in `BABYSIT_VERIFY_LISTING`: the run pages the artifact listing once, @@ -3122,6 +3134,68 @@ def test_a_clearable_refusal_on_an_older_saved_listing_is_judged_again( self.assertFalse(reader.saved) self.assertEqual(listings(fake), 1) + def late_notes(self, **overrides: Any) -> FakeGitHub: + """GitHub after the run saved a listing with no verdict: a pass-notes + at the head uploaded since, its run `run(1, **overrides)`.""" + name = named(HEAD, "pass-notes") + return FakeGitHub( + [artifact(name, 1)], + {1: run(1, **overrides)}, + {name: record(verdict="pass-notes")}, + ) + + def test_a_pass_uploaded_after_the_saved_listing_is_read_by_name(self) -> None: + # #11854: the pass-notes landed after the run's listing was read. + self.gate(FakeGitHub([], {})) + fake = self.late_notes() + reader, evidence = self.provisional(fake) + self.assertIsNone(evidence.refusal) + self.assertTrue(reader.saved) + self.assertEqual((listings(fake), name_reads(fake)), (0, 2)) + # babysit_merge.py allows only on a listing of the gate's own. + with mock.patch.object(postmerge, "gh_capture", side_effect=fake): + reader.refresh() + self.assertIsNone(reader.evidence(PR, HEAD, pull()).refusal) + self.assertEqual((listings(fake), name_reads(fake)), (1, 2)) + + def test_a_late_pass_that_does_not_count_leaves_missing(self) -> None: + for label, fake in ( + ("run failed", self.late_notes(conclusion="failure")), + ("run going", self.late_notes(status="in_progress", conclusion=None)), + ("expired", self.late_notes()), + ): + with self.subTest(label): + if label == "expired": + fake.artifacts[0]["expired"] = True + self.gate(FakeGitHub([], {})) + reader, evidence = self.provisional(fake) + self.assertEqual(evidence.refusal, "verify:missing") + self.assertTrue(reader.saved) + self.assertEqual(listings(fake), 0) + + def test_a_failed_name_read_leaves_missing_and_notes_nothing(self) -> None: + self.gate(FakeGitHub([], {})) + fake = self.late_notes() + fake.fail = {"?name="} + _, evidence = self.provisional(fake) + self.assertEqual(evidence.refusal, "verify:missing") + # The first name, read again after each wait, and no other read. + self.assertEqual(name_reads(fake), len(ve.LISTING_WAITS) + 1) + self.assertEqual(listings(fake), 0) + for suffix in (ve.FAILED_LISTING_SUFFIX, ve.OVERCOUNT_FALLBACK_SUFFIX): + self.assertFalse(pathlib.Path(f"{self.path}{suffix}").exists()) + + def test_only_a_provisional_missing_on_the_saved_listing_reads_names(self) -> None: + # The first candidate read its listing just now; a judgment that is + # not the gate's own (`evidence`, queued entries) reads no name. + fake = self.late_notes() + fake.artifacts.clear() + _, evidence = self.provisional(fake) + self.assertEqual(evidence.refusal, "verify:missing") + _, evidence = self.gate(fake) + self.assertEqual(evidence.refusal, "verify:missing") + self.assertEqual(name_reads(fake), 0) + def test_a_listing_in_memory_past_the_bound_is_read_again(self) -> None: clock = [datetime(2026, 10, 16, 12, 0, tzinfo=UTC)] fake = passing()