Fix CLA checks for pull requests over 250 commits (#830)

actions/__init__.py

@@ -28,4 +28,4 @@
 #     ├── test_summarize_pr.py
 #     └── ...
 
-__version__ = "0.2.32"
+__version__ = "0.2.33"

actions/cla.py

@@ -15,7 +15,7 @@ import json
 import os
 import time
 
-from actions.utils import GITHUB_API_URL, GITHUB_GRAPHQL_URL, Action
+from actions.utils import GITHUB_API_URL, Action
 
 CLA_REPOSITORY = "ultralytics/cla"
 CLA_PATH = "signatures/version1/cla.json"
@@ -27,27 +27,6 @@ LEGACY_MARKER = "CLA Assistant Lite bot"
 BOT_LOGIN = "github-actions[bot]"
 ALLOWLIST = frozenset(("dependabot[bot]", "github-actions[bot]", "pre-commit-ci[bot]"))
 TRANSIENT_STATUS = (429, 500, 502, 503, 504)
-COMMITS_QUERY = """
-query($owner: String!, $name: String!, $number: Int!, $cursor: String) {
-  repository(owner: $owner, name: $name) {
-    pullRequest(number: $number) {
-      commits(first: 100, after: $cursor) {
-        totalCount
-        nodes {
-          commit {
-            author {
-              name
-              email
-              user { databaseId login }
-            }
-          }
-        }
-        pageInfo { endCursor hasNextPage }
-      }
-    }
-  }
-}
-"""
 
 
 def _allowed(login: str) -> bool:
@@ -67,12 +46,13 @@ def _read(action: Action, method: str, url: str, **kwargs):
     response.raise_for_status()
 
 
-def _paginate(action: Action, url: str) -> list[dict]:
+def _paginate(action: Action, url: str, key=None) -> list[dict]:
     """Fetch every page from a GitHub REST collection."""
     items = []
     for page in range(1, 101):
         response = _read(action, "get", url, params={"per_page": 100, "page": page})
-        page_items = response.json()
+        payload = response.json()
+        page_items = payload[key] if key else payload
         items.extend(page_items)
         if len(page_items) < 100:
             return items
@@ -87,37 +67,19 @@ def _contributors(action: Action, number: int) -> list[dict]:
     if not _allowed(opener["login"]):
         contributors[opener["id"]] = {"id": opener["id"], "name": opener["login"]}
 
-    owner, name = action.repository.split("/", 1)
-    cursor = None
-    count = 0
-    for _ in range(100):
-        response = _read(
-            action,
-            "post",
-            GITHUB_GRAPHQL_URL,
-            json={
-                "query": COMMITS_QUERY,
-                "variables": {"owner": owner, "name": name, "number": number, "cursor": cursor},
-            },
-        ).json()
-        if response.get("errors"):
-            raise RuntimeError(f"Could not read PR commit authors: {response['errors']}")
-        commits = response["data"]["repository"]["pullRequest"]["commits"]
-        for node in commits["nodes"]:
-            author = node["commit"].get("author") or {}
-            user = author.get("user")
-            if user and not _allowed(user["login"]):
-                contributors[user["databaseId"]] = {"id": user["databaseId"], "name": user["login"]}
-            elif not user and author.get("name"):
-                key = f"unknown:{author['name']}:{author.get('email', '')}"
-                contributors[key] = {"id": None, "name": author["name"]}
-        count += len(commits["nodes"])
-        if not commits["pageInfo"]["hasNextPage"]:
-            if count != commits["totalCount"]:
-                raise RuntimeError(f"GitHub returned {count} of {commits['totalCount']} PR commits")
-            return list(contributors.values())
-        cursor = commits["pageInfo"]["endCursor"]
-    raise RuntimeError("Pull request exceeded 10,000 commits")
+    url = f"{GITHUB_API_URL}/repos/{action.repository}/compare/{pr['base']['sha']}...{pr['head']['sha']}"
+    commits = _paginate(action, url, "commits")
+    if len(commits) != pr["commits"]:
+        raise RuntimeError(f"GitHub returned {len(commits)} of {pr['commits']} PR commits")
+    for commit in commits:
+        author = commit["commit"].get("author") or {}
+        user = commit.get("author")
+        if user and not _allowed(user["login"]):
+            contributors[user["id"]] = {"id": user["id"], "name": user["login"]}
+        elif not user and author.get("name"):
+            key = f"unknown:{author['name']}:{author.get('email', '')}"
+            contributors[key] = {"id": None, "name": author["name"]}
+    return list(contributors.values())
 
 
 def _ledger(action: Action) -> tuple[dict, str]:

tests/test_cla.py

@@ -35,15 +35,23 @@ def ledger_response(rows, sha="old-sha"):
     return response(data={"content": content, "sha": sha})
 
 
-def commits_response(authors, total=None, has_next=False, cursor=None):
-    """Create a GraphQL response containing PR commit authors."""
-    nodes = [{"commit": {"author": author}} for author in authors]
-    commits = {
-        "totalCount": len(nodes) if total is None else total,
-        "nodes": nodes,
-        "pageInfo": {"hasNextPage": has_next, "endCursor": cursor},
-    }
-    return response(data={"data": {"repository": {"pullRequest": {"commits": commits}}}})
+def pr_response(user, commits):
+    """Create a pull request response."""
+    return response(data={"user": user, "base": {"sha": "base"}, "head": {"sha": "head"}, "commits": commits})
+
+
+def commits_response(authors):
+    """Create a compare response containing commit authors."""
+    commits = []
+    for author in authors:
+        user = author.get("user")
+        commits.append(
+            {
+                "author": {"id": user["databaseId"], "login": user["login"]} if user else None,
+                "commit": {"author": {key: value for key, value in author.items() if key != "user"}},
+            }
+        )
+    return response(data={"commits": commits})
 
 
 def test_contributors_paginates_and_requires_verified_github_identity():
@@ -64,8 +72,7 @@ def test_contributors_paginates_and_requires_verified_github_identity():
         {"user": {"databaseId": 4, "login": "other[bot]"}},
         {"user": {"databaseId": 5, "login": "bot-attacker"}},
     ]
-    source.get.return_value = response(data={"user": {"id": 1, "login": "person"}})
-    source.post.return_value = commits_response(authors)
+    source.get.side_effect = [pr_response({"id": 1, "login": "person"}, len(authors)), commits_response(authors)]
 
     assert cla._contributors(source, 7) == [
         {"id": 1, "name": "person"},
@@ -92,25 +99,28 @@ def test_ledger_preserves_existing_schema_and_never_creates_missing_file():
 def test_contributors_fails_when_github_truncates_commits():
     """Fail instead of silently skipping commit authors beyond an API limit."""
     source = action()
-    source.get.return_value = response(data={"user": {"id": 1, "login": "person"}})
-    source.post.return_value = commits_response([{"name": "Unknown", "user": None}], total=2)
+    source.get.side_effect = [
+        pr_response({"id": 1, "login": "person"}, 2),
+        commits_response([{"name": "Unknown", "user": None}]),
+    ]
 
     with pytest.raises(RuntimeError, match="returned 1 of 2"):
         cla._contributors(source, 7)
 
 
-def test_contributors_paginates_graphql_commits():
-    """Follow GraphQL cursors beyond one hundred PR commits."""
+def test_contributors_paginates_compare_commits_beyond_pull_request_limit():
+    """Collect every author from a pull request with more than 250 commits."""
     source = action()
-    source.get.return_value = response(data={"user": {"id": 1, "login": "opener"}})
-    first = [{"user": {"databaseId": i, "login": f"user-{i}"}} for i in range(2, 102)]
-    source.post.side_effect = [
-        commits_response(first, total=101, has_next=True, cursor="next"),
-        commits_response([{"user": {"databaseId": 102, "login": "user-102"}}], total=101),
+    authors = [{"user": {"databaseId": i, "login": f"user-{i}"}} for i in range(2, 258)]
+    source.get.side_effect = [
+        pr_response({"id": 1, "login": "opener"}, len(authors)),
+        commits_response(authors[:100]),
+        commits_response(authors[100:200]),
+        commits_response(authors[200:]),
     ]
 
-    assert len(cla._contributors(source, 7)) == 102
-    assert source.post.call_args_list[1].kwargs["json"]["variables"]["cursor"] == "next"
+    assert len(cla._contributors(source, 7)) == 257
+    assert source.get.call_args_list[-1].kwargs["params"] == {"per_page": 100, "page": 3}
 
 
 def test_persist_rereads_and_merges_after_conflict():
@@ -171,10 +181,10 @@ def test_run_records_exact_sentence_and_updates_legacy_comment():
     }
     bot = {"id": 40, "body": "Posted by the CLA Assistant Lite bot", "user": {"login": cla.BOT_LOGIN}}
     source.get.side_effect = [
-        response(data={"user": {"id": 2, "login": "new"}}),
+        pr_response({"id": 2, "login": "new"}, 1),
+        commits_response([{"user": {"databaseId": 2, "login": "new"}}]),
         response(data=[signing, bot]),
     ]
-    source.post.return_value = commits_response([{"user": {"databaseId": 2, "login": "new"}}])
     store.get.side_effect = [ledger_response([]), ledger_response([])]
     store.put.return_value = response(200)
 
@@ -193,15 +203,15 @@ def test_run_stays_silent_when_all_contributors_already_signed():
     """Skip the status comment entirely when nobody needed to sign."""
     source, store = action(), action()
     source.get.side_effect = [
-        response(data={"user": {"id": 1, "login": "signed"}}),
+        pr_response({"id": 1, "login": "signed"}, 1),
+        commits_response([{"user": {"databaseId": 1, "login": "signed"}}]),
         response(data=[]),
     ]
-    source.post.return_value = commits_response([{"user": {"databaseId": 1, "login": "signed"}}])
     store.get.return_value = ledger_response([{"id": 1}])
 
     cla.run(source, store)
 
-    assert source.post.call_count == 1  # GraphQL commits query only, no comment created
+    source.post.assert_not_called()
     source.patch.assert_not_called()
 
 
@@ -211,14 +221,11 @@ def test_run_rejects_similar_sentence_and_keeps_hard_failure(body):
     source, store = action(), action()
     user = {"id": 2, "login": "new", "type": "User"}
     source.get.side_effect = [
-        response(data={"user": {"id": 2, "login": "new"}}),
+        pr_response({"id": 2, "login": "new"}, 1),
+        commits_response([{"user": {"databaseId": 2, "login": "new"}}]),
         response(data=[{"id": 30, "body": body, "created_at": "date", "user": user}]),
         response(data=[{"id": 40, "body": cla.COMMENT_MARKER, "user": {"login": cla.BOT_LOGIN}}]),
     ]
-    source.post.side_effect = [
-        commits_response([{"user": {"databaseId": 2, "login": "new"}}]),
-        response(201),
-    ]
     store.get.return_value = ledger_response([])
     source.post.return_value = response(201)
 
@@ -233,14 +240,11 @@ def test_run_fails_unlinked_email_author():
     """Require commit authors with unlinked emails to link a GitHub account."""
     source, store = action(), action()
     source.get.side_effect = [
-        response(data={"user": {"id": 2, "login": "new"}}),
+        pr_response({"id": 2, "login": "new"}, 1),
+        commits_response([{"name": "Unknown", "email": "[email protected]", "user": None}]),
         response(data=[]),
         response(data=[{"id": 40, "body": cla.COMMENT_MARKER, "user": {"login": cla.BOT_LOGIN}}]),
     ]
-    source.post.side_effect = [
-        commits_response([{"name": "Unknown", "email": "[email protected]", "user": None}]),
-        response(201),
-    ]
     store.get.return_value = ledger_response([])
     source.post.return_value = response(201)
 
@@ -254,14 +258,11 @@ def test_run_requires_pr_opener_when_commit_identity_is_already_signed():
     """Require the submitter to sign even when forged commit metadata names a signer."""
     source, store = action(), action()
     source.get.side_effect = [
-        response(data={"user": {"id": 9, "login": "submitter"}}),
+        pr_response({"id": 9, "login": "submitter"}, 1),
+        commits_response([{"user": {"databaseId": 1, "login": "signed-author"}}]),
         response(data=[]),
         response(data=[{"id": 40, "body": cla.COMMENT_MARKER, "user": {"login": cla.BOT_LOGIN}}]),
     ]
-    source.post.side_effect = [
-        commits_response([{"user": {"databaseId": 1, "login": "signed-author"}}]),
-        response(201),
-    ]
     store.get.return_value = ledger_response([{"id": 1}])
 
     with pytest.raises(RuntimeError, match="must sign"):