Skip to content

Commit e521f05

Browse files
committed
fix: never accept a partial or stale list of the pull request's commits
- A local range shorter than the payload's commit count is the history a shallow clone had room for, not the pull request: the API is asked first, and the local list is kept only when the API cannot answer, as before. - An API list is used only when it ends at the payload's head commit. A force push after the event can leave the same count and other commits. - The README names the two fallbacks separately: an unlisted pull request checks HEAD alone, an unfetched head skips the author checks. Tests cover both, and the git-missing branch of the head fetch that Codecov flagged. TestGetPrCommitMessages pins the event to {} so it never reads the CI run it happens to execute in.
1 parent 27de21e commit e521f05

3 files changed

Lines changed: 47 additions & 19 deletions

File tree

‎README.md‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,8 +63,9 @@ jobs:
6363
> `fetch-depth: 0` is recommended, not required. A shallow clone holds only
6464
> GitHub's merge commit, so the action lists the pull request's commits
6565
> through the API — up to 250, with `pull-requests: read` — and fetches the
66-
> head commit for the author checks. If neither works it warns and checks
67-
> HEAD alone.
66+
> head commit for the author checks. If the commits cannot be listed, it warns
67+
> and checks HEAD alone; if the head commit cannot be fetched, it warns and
68+
> skips the author checks.
6869

6970
Runs on `ubuntu-latest`, `macos-latest` and `windows-latest`. Self-hosted
7071
runners need a few tools — see [Good to know](#good-to-know).

‎main.py‎

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -500,7 +500,11 @@ def get_messages_from_api() -> list[Commit]:
500500
Github(auth=Auth.Token(token), base_url=api_url).get_repo(repo).get_pull(number)
501501
)
502502
commits = [(c.sha, c.commit.message) for c in pull.get_commits()]
503-
return commits if len(commits) == total else []
503+
# A force push after the event can leave the same count but other commits.
504+
head = get_pr_head_sha()
505+
if len(commits) != total or (head and commits[-1][0] != head):
506+
return []
507+
return commits
504508

505509

506510
def get_pr_commit_messages() -> list[Commit]:
@@ -511,27 +515,23 @@ def get_pr_commit_messages() -> list[Commit]:
511515
checkout HEAD is the synthetic merge commit, so ``HEAD^1..HEAD^2`` is
512516
the same range. If the workflow checks out the PR head SHA instead,
513517
diff against ``origin/<base-ref>`` when that ref is available locally.
514-
A clone that holds none of these asks the API.
518+
A clone that holds none of these, or only part of the pull request (a
519+
shallow boundary ends a range early), asks the API.
515520
"""
516521
if not is_pr_event():
517522
return []
518523

519524
try:
520-
messages = get_messages_from_event_range()
521-
if messages:
522-
return messages
523-
524-
messages = get_messages_from_merge_ref()
525-
if messages:
526-
return messages
527-
528525
base_ref = os.getenv("GITHUB_BASE_REF", "")
529-
if base_ref:
530-
messages = get_messages_from_head_ref(base_ref)
531-
if messages:
532-
return messages
533-
534-
return get_messages_from_api()
526+
messages = (
527+
get_messages_from_event_range()
528+
or get_messages_from_merge_ref()
529+
or (get_messages_from_head_ref(base_ref) if base_ref else [])
530+
)
531+
total = get_pr_event().get("commits")
532+
if messages and (not total or len(messages) == total):
533+
return messages
534+
return get_messages_from_api() or messages
535535
except Exception as e:
536536
print(
537537
f"::warning::Failed to retrieve PR commit messages: {e}",

‎main_test.py‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -458,6 +458,12 @@ def test_unknown_flag_is_skipped(self):
458458

459459

460460
class TestGetPrCommitMessages(unittest.TestCase):
461+
def setUp(self):
462+
# Never the event of whatever CI run the tests happen to be in.
463+
patcher = patch("main.get_pr_event", return_value={})
464+
patcher.start()
465+
self.addCleanup(patcher.stop)
466+
461467
def test_non_pr_event_returns_empty(self):
462468
with patch.dict(os.environ, {"GITHUB_EVENT_NAME": "push"}):
463469
result = main.get_pr_commit_messages()
@@ -502,6 +508,17 @@ def test_pull_request_target_is_supported(self):
502508
result = main.get_pr_commit_messages()
503509
self.assertEqual(result, ["fix: first"])
504510

511+
def test_a_range_cut_short_by_a_shallow_clone_asks_the_api(self):
512+
with (
513+
patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}),
514+
patch("main.get_messages_from_event_range", return_value=[("b2", "fix")]),
515+
patch("main.get_pr_event", return_value={"commits": 2}),
516+
patch("main.get_messages_from_api", return_value=[]) as api,
517+
):
518+
# The API cannot help either: keep what the clone had.
519+
self.assertEqual(main.get_pr_commit_messages(), [("b2", "fix")])
520+
api.assert_called_once()
521+
505522
def test_asks_the_api_when_the_clone_holds_none_of_them(self):
506523
with (
507524
patch.dict(
@@ -1060,11 +1077,15 @@ def test_a_head_the_clone_lacks_is_fetched_without_history(self):
10601077
["git", "fetch", "--no-tags", "--quiet", "--depth=1", "origin", "abc123"],
10611078
)
10621079

1080+
def test_a_fetch_without_git_gives_up(self):
1081+
with patch("main.subprocess.run", side_effect=OSError("no git")):
1082+
self.assertFalse(main._fetch_commit("abc123"))
1083+
10631084

10641085
class TestGetMessagesFromApi(unittest.TestCase):
10651086
"""A clone without the pull request's commits lists them through the API."""
10661087

1067-
def _run(self, total, api_commits):
1088+
def _run(self, total, api_commits, head="b2"):
10681089
pull = MagicMock()
10691090
pull.get_commits.return_value = [
10701091
MagicMock(sha=sha, commit=MagicMock(message=msg))
@@ -1082,6 +1103,7 @@ def _run(self, total, api_commits):
10821103
with (
10831104
patch.dict(os.environ, env),
10841105
patch("main.get_pr_event", return_value={"number": 7, "commits": total}),
1106+
patch("main.get_pr_head_sha", return_value=head),
10851107
patch.dict(sys.modules, {"github": github_module}),
10861108
):
10871109
return main.get_messages_from_api(), github_module
@@ -1095,6 +1117,11 @@ def test_lists_the_commits_oldest_first(self):
10951117
"https://ghe.example.com/api/v3",
10961118
)
10971119

1120+
def test_a_list_that_ends_elsewhere_is_not_used(self):
1121+
# A force push after the event: same count, other commits.
1122+
result, _ = self._run(2, [("a1", "feat: one"), ("c3", "fix: other")])
1123+
self.assertEqual(result, [])
1124+
10981125
def test_a_partial_list_is_not_used(self):
10991126
result, _ = self._run(3, [("a1", "feat: one"), ("b2", "fix: two")])
11001127
self.assertEqual(result, [])

0 commit comments

Comments
 (0)