diff --git a/.changes/source-control/fix-force-push-read-pages-1473d48c.md b/.changes/source-control/fix-force-push-read-pages-1473d48c.md new file mode 100644 index 00000000000..e4e83f235b0 --- /dev/null +++ b/.changes/source-control/fix-force-push-read-pages-1473d48c.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- The babysit merge gate reads a pull request's force pushes 100 at a time up to 1,000, so a pull request rebased more than 100 times no longer fails its risk gate with `force pushes: unread or past one page` on every run and never arms. A listing past 1,000, a missing cursor or a count the pages do not reach still raises. 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 1207ab918b3..5ce6abba42a 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -3337,19 +3337,26 @@ def _main_reviews( _FORCE_PUSHES_QUERY = ( - "query($o:String!,$r:String!,$n:Int!){repository(owner:$o,name:$r){" - "pullRequest(number:$n){timelineItems(first:100,itemTypes:[HEAD_REF_FORCE_PUSHED_EVENT])" - "{totalCount nodes{... on HeadRefForcePushedEvent{beforeCommit{oid}}}}}}}" + "query($o:String!,$r:String!,$n:Int!,$c:String){repository(owner:$o,name:$r){" + "pullRequest(number:$n){timelineItems(first:100,after:$c,itemTypes:[HEAD_REF_FORCE_PUSHED_EVENT])" + "{totalCount pageInfo{hasNextPage endCursor}" + "nodes{... on HeadRefForcePushedEvent{beforeCommit{oid}}}}}}}" ) +# A pull request rebased more often than this is past what the gate reads. +_FORCE_PUSH_PAGES = 10 def _force_pushed_heads(repo: str, number: int) -> list[str]: """The heads force pushes replaced, which the commits list no longer - names but whose recorded counts still hold. Any doubt raises, so the - evidence is unbound rather than missing a count.""" + names but whose recorded counts still hold, read a page of 100 at a time + up to _FORCE_PUSH_PAGES. Any doubt raises, so the evidence is unbound + rather than missing a count.""" owner, name = repo.split("/", 1) - data = gh_json_graphql( - [ + nodes: list[Any] = [] + cursor = None + total = None + for _ in range(_FORCE_PUSH_PAGES): + args = [ "api", "graphql", "-f", @@ -3360,19 +3367,32 @@ def _force_pushed_heads(repo: str, number: int) -> list[str]: f"r={name}", "-F", f"n={number}", - ], - label=f"#{number} force pushes", - ) - items = json_object( - json_object( + ] + if cursor is not None: + args += ["-f", f"c={cursor}"] + data = gh_json_graphql(args, label=f"#{number} force pushes") + items = json_object( json_object( - json_object(json_object(data).get("data")).get("repository") - ).get("pullRequest") - ).get("timelineItems") - ) - nodes = items.get("nodes") - if not is_json_array(nodes) or items.get("totalCount") != len(nodes): - raise RuntimeError(f"#{number} force pushes: unread or past one page") + json_object( + json_object(json_object(data).get("data")).get("repository") + ).get("pullRequest") + ).get("timelineItems") + ) + page = items.get("nodes") + info = json_object(items.get("pageInfo")) + if not is_json_array(page): + raise RuntimeError(f"#{number} force pushes: unread") + total = items.get("totalCount") + nodes.extend(page) + if info.get("hasNextPage") is not True: + break + cursor = info.get("endCursor") + if not isinstance(cursor, str) or not cursor: + raise RuntimeError(f"#{number} force pushes: unread") + else: + raise RuntimeError(f"#{number} force pushes: past {_FORCE_PUSH_PAGES} pages") + if total != len(nodes): + raise RuntimeError(f"#{number} force pushes: unread or incomplete") heads = [] for node in nodes: oid = json_object(json_object(node).get("beforeCommit")).get("oid") diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py index 5030cb12abe..376ac940746 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py @@ -2502,7 +2502,14 @@ def collect(repo, names, workflow, branch, env): ), ): reviews, context = merge._main_reviews( - "owner/repo", 5, HEAD, changed, None, None, None, [HEAD], + "owner/repo", + 5, + HEAD, + changed, + None, + None, + None, + [HEAD], floor=merge.babysit_risk.floor_for(), ) calls.extend(read.call_args_list) @@ -2620,32 +2627,62 @@ def test_any_other_pull_request_lists_every_lane(self) -> None: class ForcePushedHeadsTests(unittest.TestCase): """The heads force pushes replaced, whose recorded review counts still - hold; a listing that is not whole raises.""" + hold, read across pages; a listing that is not whole raises.""" - def heads(self, total: int, oids: list[Any]) -> list[str]: + @staticmethod + def page(total: int, oids: list[Any], cursor: str | None = None) -> dict: nodes = [ {"beforeCommit": None if oid is None else {"oid": oid}} for oid in oids ] - data = { + info = {"hasNextPage": cursor is not None, "endCursor": cursor} + return { "data": { "repository": { "pullRequest": { - "timelineItems": {"totalCount": total, "nodes": nodes} + "timelineItems": { + "totalCount": total, + "pageInfo": info, + "nodes": nodes, + } } } } } - with mock.patch.object(merge, "gh_json_graphql", return_value=data): - return merge._force_pushed_heads("owner/repo", 5) + + def heads(self, *pages: dict) -> tuple[list[str], mock.MagicMock]: + with mock.patch.object( + merge, "gh_json_graphql", side_effect=list(pages) + ) as read: + return merge._force_pushed_heads("owner/repo", 5), read def test_names_each_replaced_head(self) -> None: - self.assertEqual( - self.heads(3, ["a" * 40, None, "b" * 40]), ["a" * 40, "b" * 40] + heads, _ = self.heads(self.page(3, ["a" * 40, None, "b" * 40])) + self.assertEqual(heads, ["a" * 40, "b" * 40]) + + def test_reads_every_page_by_its_cursor(self) -> None: + heads, read = self.heads( + self.page(101, ["a" * 40] * 100, cursor="next"), + self.page(101, ["b" * 40]), ) + self.assertEqual(heads, ["a" * 40] * 100 + ["b" * 40]) + self.assertNotIn("c=next", read.call_args_list[0].args[0]) + self.assertIn("c=next", read.call_args_list[1].args[0]) + + def test_a_count_the_pages_do_not_reach_raises(self) -> None: + with self.assertRaises(RuntimeError): + self.heads(self.page(101, ["a" * 40])) + + def test_a_next_page_with_no_cursor_raises(self) -> None: + with self.assertRaises(RuntimeError): + self.heads(self.page(200, ["a" * 40], cursor="")) - def test_a_listing_past_one_page_raises(self) -> None: + def test_a_listing_past_the_page_cap_raises(self) -> None: + pages = [ + self.page(9999, ["a" * 40], cursor=f"c{i}") + for i in range(merge._FORCE_PUSH_PAGES) + ] with self.assertRaises(RuntimeError): - self.heads(101, ["a" * 40]) + self.heads(*pages) if __name__ == "__main__":