Skip to content

Commit 14ba09a

Browse files
shenxianpengclaude
andcommitted
fix: resolve the pull request head from the event, skip authors without it
pull_request.head.sha names the branch tip for pull_request and pull_request_target alike, whatever was checked out, so the author checks read it first. HEAD^2 stays as the fallback on a pull_request checkout only: on pull_request_target HEAD is the base branch, and its second parent, when it has one, belongs to some unrelated merge. When neither resolves, the author scopes are reported as skipped instead of being run on HEAD, whose author on a pull request is GitHub's merge commit or the base branch, never the contributor. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018xYa7m3qup5wyN5MaXFgf6
1 parent 8f55aab commit 14ba09a

2 files changed

Lines changed: 257 additions & 31 deletions

File tree

main.py

Lines changed: 82 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -215,23 +215,87 @@ def warn_shallow_checkout(problem: str, consequence: str) -> None:
215215
print(f"::warning title=commit-check::{_annotation_escape(text)}")
216216

217217

218-
def pr_head_rev() -> str | None:
219-
"""Return ``HEAD^2`` when it resolves, ``None`` on a shallow clone.
218+
def get_pr_head_sha() -> str | None:
219+
"""The pull request's head commit, from the event payload."""
220+
if not is_pr_event():
221+
return None
222+
event_path = os.getenv("GITHUB_EVENT_PATH")
223+
if not event_path:
224+
return None
225+
try:
226+
with open(event_path, "r", encoding="utf-8") as f:
227+
event = json.load(f)
228+
return event.get("pull_request", {}).get("head", {}).get("sha") or None
229+
except Exception as e:
230+
print(f"::warning::Failed to read PR head from event: {e}", file=sys.stderr)
231+
return None
220232

221-
With ``fetch-depth: 1`` the merge commit's parents are not fetched and
222-
``git rev-parse HEAD^2`` fails, so the caller has to settle for HEAD.
223-
"""
233+
234+
def _rev_resolves(rev: str) -> bool:
235+
"""Whether ``rev`` names a commit the clone actually has."""
224236
try:
225237
result = subprocess.run(
226-
["git", "rev-parse", "--verify", "--quiet", PR_HEAD_REV],
238+
["git", "rev-parse", "--verify", "--quiet", f"{rev}^{{commit}}"],
227239
stdout=subprocess.PIPE,
228240
stderr=subprocess.PIPE,
229241
encoding="utf-8",
230242
check=False,
231243
)
232244
except OSError:
233-
return None
234-
return PR_HEAD_REV if result.returncode == 0 else None
245+
return False
246+
return result.returncode == 0
247+
248+
249+
def pr_head_rev() -> str | None:
250+
"""The commit whose recorded author the PR's author checks read.
251+
252+
First choice is ``pull_request.head.sha`` from the event payload: it
253+
names the branch tip for ``pull_request`` and ``pull_request_target``
254+
alike, whatever was checked out, as long as the clone has it. Failing
255+
that, ``HEAD^2`` on a ``pull_request`` checkout, where HEAD is the
256+
merge ref and its second parent is that same tip. Never ``HEAD^2`` on
257+
``pull_request_target``: there HEAD is the base branch, so ``HEAD^2``
258+
is nothing, or the parent of some unrelated merge on it.
259+
260+
``None`` when the clone is too shallow to hold either.
261+
"""
262+
sha = get_pr_head_sha()
263+
if sha and _rev_resolves(sha):
264+
return sha
265+
if os.getenv("GITHUB_EVENT_NAME") == "pull_request" and _rev_resolves(PR_HEAD_REV):
266+
return PR_HEAD_REV
267+
return None
268+
269+
270+
#: The rule each author check runs, for a scope that had to be skipped.
271+
AUTHOR_RULES = {
272+
"--author-name": ("CC101", "author_name"),
273+
"--author-email": ("CC102", "author_email"),
274+
}
275+
276+
277+
def skipped_author_scope(flag: str) -> ScopeResult:
278+
"""A scope recording that an author check could not run at all.
279+
280+
Reported as ``skip``, never as a pass: nothing was validated, and the
281+
one commit the clone does hold (HEAD) has the wrong author for a pull
282+
request, GitHub's merge commit or the base branch.
283+
"""
284+
rule_id, check = AUTHOR_RULES[flag]
285+
return ScopeResult(
286+
label=CHECK_LABELS[flag],
287+
checks=[
288+
{
289+
"rule_id": rule_id,
290+
"check": check,
291+
"status": "skip",
292+
"value": "",
293+
"error": "",
294+
"suggest": "",
295+
"docs_url": "",
296+
}
297+
],
298+
)
235299

236300

237301
def get_pr_title() -> str | None:
@@ -441,11 +505,18 @@ def run_commit_check() -> tuple[int, list[ScopeResult]]:
441505
if is_pr_event() and any(flag in AUTHOR_FLAGS for flag in args):
442506
rev = pr_head_rev()
443507
if rev is None:
508+
# HEAD's author is GitHub's merge commit on a pull_request
509+
# checkout and the base branch on pull_request_target: checking
510+
# it would grade the wrong person either way. Say so, and skip.
444511
warn_shallow_checkout(
445-
f"Could not resolve {PR_HEAD_REV} for the author checks",
446-
"HEAD's author was checked instead, which on a pull request "
447-
"is GitHub's merge commit",
512+
"Could not resolve the pull request's head commit for the "
513+
"author checks",
514+
"they were skipped",
448515
)
516+
for flag in args:
517+
if flag in AUTHOR_FLAGS:
518+
results.append(skipped_author_scope(flag))
519+
args = [a for a in args if a not in AUTHOR_FLAGS]
449520
results.extend(run_other_checks(args, rev=rev))
450521

451522
exit_code = exit_code_for(results)

main_test.py

Lines changed: 175 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -643,16 +643,16 @@ def test_push_without_pr_commits_does_not_warn(self):
643643
self.assertNotIn("::warning", output)
644644

645645
@staticmethod
646-
def _fake_git_and_cli(head2_resolves: bool):
646+
def _fake_git_and_cli(resolves: bool):
647647
"""subprocess.run stand-in: answers rev-parse and the CLI alike."""
648648
commands: list[list[str]] = []
649649

650650
def run(command, **_kwargs):
651651
commands.append(command)
652652
if command[:2] == ["git", "rev-parse"]:
653653
return MagicMock(
654-
returncode=0 if head2_resolves else 1,
655-
stdout="abc123\n" if head2_resolves else "",
654+
returncode=0 if resolves else 1,
655+
stdout="abc123\n" if resolves else "",
656656
)
657657
check = command[3].lstrip("-").replace("-", "_")
658658
return MagicMock(returncode=0, stdout=json_output(make_check(check)))
@@ -661,21 +661,24 @@ def run(command, **_kwargs):
661661

662662
def test_pr_author_checks_read_the_branch_tip(self):
663663
"""On refs/pull/N/merge HEAD's author is GitHub, not the contributor."""
664-
run, commands = self._fake_git_and_cli(head2_resolves=True)
664+
run, commands = self._fake_git_and_cli(resolves=True)
665665
with (
666666
patch("main.MESSAGE_ENABLED", False),
667667
patch("main.BRANCH_ENABLED", True),
668668
patch("main.AUTHOR_NAME_ENABLED", True),
669669
patch("main.AUTHOR_EMAIL_ENABLED", True),
670-
patch("main.is_pr_event", return_value=True),
670+
patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}),
671+
patch("main.get_pr_head_sha", return_value=None),
671672
patch("main.subprocess.run", side_effect=run),
672673
):
673674
rc, results, output = self._run_capturing_stdout()
674675
self.assertEqual(rc, 0)
675676
self.assertEqual(
676677
[s.label for s in results], ["Branch", "Author name", "Author email"]
677678
)
678-
self.assertIn(["git", "rev-parse", "--verify", "--quiet", "HEAD^2"], commands)
679+
self.assertIn(
680+
["git", "rev-parse", "--verify", "--quiet", "HEAD^2^{commit}"], commands
681+
)
679682
self.assertIn(
680683
["commit-check", "--format", "json", "--author-name", "--rev", "HEAD^2"],
681684
commands,
@@ -688,28 +691,104 @@ def test_pr_author_checks_read_the_branch_tip(self):
688691
self.assertIn(["commit-check", "--format", "json", "--branch"], commands)
689692
self.assertNotIn("::warning", output)
690693

691-
def test_pr_author_checks_fall_back_to_head_on_shallow_clone(self):
692-
run, commands = self._fake_git_and_cli(head2_resolves=False)
694+
def test_pr_author_checks_prefer_the_payload_head_sha(self):
695+
"""pull_request.head.sha names the tip for either PR event type."""
696+
run, commands = self._fake_git_and_cli(resolves=True)
693697
with (
694698
patch("main.MESSAGE_ENABLED", False),
695699
patch("main.BRANCH_ENABLED", False),
696700
patch("main.AUTHOR_NAME_ENABLED", True),
697701
patch("main.AUTHOR_EMAIL_ENABLED", False),
698-
patch("main.is_pr_event", return_value=True),
702+
patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request_target"}),
703+
patch("main.get_pr_head_sha", return_value="deadbeefcafe"),
699704
patch("main.subprocess.run", side_effect=run),
700705
):
701706
rc, results, output = self._run_capturing_stdout()
702707
self.assertEqual(rc, 0)
703-
self.assertIn(["commit-check", "--format", "json", "--author-name"], commands)
704-
self.assertFalse([c for c in commands if "--rev" in c], commands)
708+
self.assertEqual([s.status for s in results], ["pass"])
709+
self.assertIn(
710+
["git", "rev-parse", "--verify", "--quiet", "deadbeefcafe^{commit}"],
711+
commands,
712+
)
713+
self.assertIn(
714+
[
715+
"commit-check",
716+
"--format",
717+
"json",
718+
"--author-name",
719+
"--rev",
720+
"deadbeefcafe",
721+
],
722+
commands,
723+
)
724+
self.assertNotIn("::warning", output)
725+
726+
def test_pr_author_checks_are_skipped_on_a_shallow_clone(self):
727+
"""HEAD's author is GitHub's merge commit: skip rather than grade it."""
728+
run, commands = self._fake_git_and_cli(resolves=False)
729+
with (
730+
patch("main.MESSAGE_ENABLED", False),
731+
patch("main.BRANCH_ENABLED", True),
732+
patch("main.AUTHOR_NAME_ENABLED", True),
733+
patch("main.AUTHOR_EMAIL_ENABLED", True),
734+
patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}),
735+
patch("main.get_pr_head_sha", return_value=None),
736+
patch("main.subprocess.run", side_effect=run),
737+
):
738+
rc, results, output = self._run_capturing_stdout()
739+
self.assertEqual(rc, 0)
740+
self.assertEqual(
741+
[(s.label, s.status) for s in results],
742+
[("Author name", "skip"), ("Author email", "skip"), ("Branch", "pass")],
743+
)
744+
self.assertEqual(
745+
results[0].checks,
746+
[
747+
{
748+
"rule_id": "CC101",
749+
"check": "author_name",
750+
"status": "skip",
751+
"value": "",
752+
"error": "",
753+
"suggest": "",
754+
"docs_url": "",
755+
}
756+
],
757+
)
758+
self.assertEqual(results[1].checks[0]["rule_id"], "CC102")
759+
self.assertFalse([c for c in commands if "--author-name" in c], commands)
760+
self.assertFalse([c for c in commands if "--author-email" in c], commands)
761+
self.assertIn(["commit-check", "--format", "json", "--branch"], commands)
705762
warning = [ln for ln in output.splitlines() if ln.startswith("::warning")]
706763
self.assertEqual(len(warning), 1, output)
707764
self.assertTrue(warning[0].startswith("::warning title=commit-check::"))
708-
self.assertIn("Could not resolve HEAD^2", warning[0])
765+
self.assertIn("Could not resolve the pull request's head commit", warning[0])
709766
self.assertIn("is actions/checkout using fetch-depth: 0?", warning[0])
767+
self.assertIn("they were skipped", warning[0])
768+
769+
def test_pull_request_target_never_uses_head2(self):
770+
"""On pull_request_target HEAD is the base branch; HEAD^2 is unrelated."""
771+
run, commands = self._fake_git_and_cli(resolves=True)
772+
with (
773+
patch("main.MESSAGE_ENABLED", False),
774+
patch("main.BRANCH_ENABLED", False),
775+
patch("main.AUTHOR_NAME_ENABLED", True),
776+
patch("main.AUTHOR_EMAIL_ENABLED", False),
777+
patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request_target"}),
778+
patch("main.get_pr_head_sha", return_value=None),
779+
patch("main.subprocess.run", side_effect=run),
780+
):
781+
rc, results, output = self._run_capturing_stdout()
782+
self.assertEqual(rc, 0)
783+
self.assertEqual(
784+
[(s.label, s.status) for s in results], [("Author name", "skip")]
785+
)
786+
self.assertFalse([c for c in commands if "HEAD^2^{commit}" in c], commands)
787+
self.assertFalse([c for c in commands if c[0] == "commit-check"], commands)
788+
self.assertIn("::warning title=commit-check::", output)
710789

711790
def test_push_author_checks_never_pass_rev(self):
712-
run, commands = self._fake_git_and_cli(head2_resolves=True)
791+
run, commands = self._fake_git_and_cli(resolves=True)
713792
with (
714793
patch("main.MESSAGE_ENABLED", False),
715794
patch("main.BRANCH_ENABLED", False),
@@ -725,25 +804,101 @@ def test_push_author_checks_never_pass_rev(self):
725804

726805

727806
class TestPrHeadRev(unittest.TestCase):
728-
def test_resolving_head2_returns_the_revision(self):
729-
with patch(
730-
"main.subprocess.run", return_value=MagicMock(returncode=0)
731-
) as mock_run:
807+
def test_payload_head_sha_wins_when_the_clone_has_it(self):
808+
with (
809+
patch("main.get_pr_head_sha", return_value="abc123"),
810+
patch(
811+
"main.subprocess.run", return_value=MagicMock(returncode=0)
812+
) as mock_run,
813+
):
814+
self.assertEqual(main.pr_head_rev(), "abc123")
815+
self.assertEqual(
816+
mock_run.call_args[0][0],
817+
["git", "rev-parse", "--verify", "--quiet", "abc123^{commit}"],
818+
)
819+
820+
def test_pull_request_falls_back_to_head2(self):
821+
with (
822+
patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}),
823+
patch("main.get_pr_head_sha", return_value=None),
824+
patch(
825+
"main.subprocess.run", return_value=MagicMock(returncode=0)
826+
) as mock_run,
827+
):
732828
self.assertEqual(main.pr_head_rev(), "HEAD^2")
733829
self.assertEqual(
734830
mock_run.call_args[0][0],
735-
["git", "rev-parse", "--verify", "--quiet", "HEAD^2"],
831+
["git", "rev-parse", "--verify", "--quiet", "HEAD^2^{commit}"],
736832
)
737833

834+
def test_pull_request_target_does_not_fall_back_to_head2(self):
835+
with (
836+
patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request_target"}),
837+
patch("main.get_pr_head_sha", return_value=None),
838+
patch(
839+
"main.subprocess.run", return_value=MagicMock(returncode=0)
840+
) as mock_run,
841+
):
842+
self.assertIsNone(main.pr_head_rev())
843+
mock_run.assert_not_called()
844+
845+
def test_unfetched_payload_sha_on_pull_request_target_returns_none(self):
846+
with (
847+
patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request_target"}),
848+
patch("main.get_pr_head_sha", return_value="abc123"),
849+
patch("main.subprocess.run", return_value=MagicMock(returncode=1)),
850+
):
851+
self.assertIsNone(main.pr_head_rev())
852+
738853
def test_shallow_clone_returns_none(self):
739-
with patch("main.subprocess.run", return_value=MagicMock(returncode=1)):
854+
with (
855+
patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}),
856+
patch("main.get_pr_head_sha", return_value=None),
857+
patch("main.subprocess.run", return_value=MagicMock(returncode=1)),
858+
):
740859
self.assertIsNone(main.pr_head_rev())
741860

742861
def test_missing_git_returns_none(self):
743-
with patch("main.subprocess.run", side_effect=OSError("no git")):
862+
with (
863+
patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}),
864+
patch("main.get_pr_head_sha", return_value=None),
865+
patch("main.subprocess.run", side_effect=OSError("no git")),
866+
):
744867
self.assertIsNone(main.pr_head_rev())
745868

746869

870+
class TestGetPrHeadSha(unittest.TestCase):
871+
def test_reads_the_head_sha_from_the_event(self):
872+
with tempfile.NamedTemporaryFile("w", suffix=".json", delete=False) as f:
873+
json.dump({"pull_request": {"head": {"sha": "abc123"}}}, f)
874+
event_path = f.name
875+
try:
876+
with patch.dict(
877+
os.environ,
878+
{
879+
"GITHUB_EVENT_NAME": "pull_request_target",
880+
"GITHUB_EVENT_PATH": event_path,
881+
},
882+
):
883+
self.assertEqual(main.get_pr_head_sha(), "abc123")
884+
finally:
885+
os.unlink(event_path)
886+
887+
def test_not_a_pr_event_returns_none(self):
888+
with patch.dict(os.environ, {"GITHUB_EVENT_NAME": "push"}):
889+
self.assertIsNone(main.get_pr_head_sha())
890+
891+
def test_unreadable_event_returns_none(self):
892+
with patch.dict(
893+
os.environ,
894+
{
895+
"GITHUB_EVENT_NAME": "pull_request",
896+
"GITHUB_EVENT_PATH": "/nonexistent.json",
897+
},
898+
):
899+
self.assertIsNone(main.get_pr_head_sha())
900+
901+
747902
class TestCommitCheckVersionPin(unittest.TestCase):
748903
"""The warn rendering is inert against an engine that never emits it.
749904

0 commit comments

Comments
 (0)