From 48ea8486dfb714154cf8746b1bce03fe6da70758 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Sun, 8 Nov 2026 03:43:52 +0000 Subject: [PATCH 1/2] feat: check each pull request commit as itself Each PR commit's message was piped to the CLI, so the CLI never saw the commit's author: ignore_authors compared the runner's git identity or the merge commit's author, and an author-based rule had nothing to read. A commit the clone holds is now checked by its hash (--message --rev), so the CLI reads that commit's message and its own author. A commit listed only by the API (the default shallow checkout, or no checkout at all) is still piped in, with the author the API recorded passed as GIT_AUTHOR_NAME / GIT_AUTHOR_EMAIL, which is where the CLI reads the author of a message that has no commit. --- main.py | 45 +++++++++++++++++++++++------ main_test.py | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 116 insertions(+), 9 deletions(-) diff --git a/main.py b/main.py index 2843a76..bbcfd46 100755 --- a/main.py +++ b/main.py @@ -489,6 +489,10 @@ def get_messages_from_head_ref(base_ref: str) -> list[Commit]: #: The most commits GitHub's pull request commits endpoint returns. API_COMMIT_LIMIT = 250 +#: Author (name, email) of each commit listed by the API, by hash. Those +#: commits are not in the clone, so this is the only record of who wrote them. +API_AUTHORS: dict[str, tuple[str, str]] = {} + def get_messages_from_api() -> list[Commit]: """The pull request's commits from the REST API, for a clone without them. @@ -509,7 +513,12 @@ def get_messages_from_api() -> list[Commit]: pull = ( Github(auth=Auth.Token(token), base_url=api_url).get_repo(repo).get_pull(number) ) - commits = [(c.sha, c.commit.message) for c in pull.get_commits()] + listed = list(pull.get_commits()) + commits = [(c.sha, c.commit.message) for c in listed] + for c in listed: + author = c.commit.author + if author is not None: + API_AUTHORS[c.sha] = (author.name or "", author.email or "") # A force push after the event can leave the same count but other commits. head = get_pr_head_sha() if len(commits) != total or (head and commits[-1][0] != head): @@ -578,7 +587,9 @@ def _relay_cli_notices(text: str) -> None: def run_check_json( - args: list[str], input_text: str | None = None + args: list[str], + input_text: str | None = None, + env: dict[str, str] | None = None, ) -> tuple[int, dict[str, Any] | None, str]: """Run ``commit-check --format json`` and return (exit code, parsed JSON, raw output). @@ -602,6 +613,7 @@ def run_check_json( text=True, encoding="utf-8", check=False, + env={**os.environ, **env} if env else None, ) out = result.stdout or "" err = result.stderr or "" @@ -621,28 +633,45 @@ def run_check_json( def check_scope( - label: str, args: list[str], input_text: str | None = None, sha: str = "" + label: str, + args: list[str], + input_text: str | None = None, + sha: str = "", + env: dict[str, str] | None = None, ) -> ScopeResult: """Run commit-check for one scope and wrap the outcome in a ScopeResult. ``sha`` names the commit a message scope checked; it rides along on both outcomes so an unparsable CLI response still says which commit it was. """ - _rc, data, raw = run_check_json(args, input_text=input_text) + _rc, data, raw = run_check_json(args, input_text=input_text, env=env) if isinstance(data, dict): return ScopeResult(label=label, checks=data.get("checks", []), sha=sha) return ScopeResult(label=label, raw_text=raw, sha=sha) def run_pr_message_checks(pr_commits: list[Commit]) -> list[ScopeResult]: - """Check each PR commit message individually via commit-check --message.""" + """Check each PR commit individually via commit-check --message. + + A commit the clone holds is checked by its hash (``--rev``), so the CLI + reads its message and its own author: the AI author rule (CC017) and + ``ignore_authors`` then judge that commit's author, not whoever HEAD or + the runner's git config names. A commit listed only by the API is piped + in, with the author the API recorded passed as git's author variables. + """ results: list[ScopeResult] = [] total = len(pr_commits) for index, (sha, msg) in enumerate(pr_commits, start=1): + label = f"Commit {index}/{total}" + if sha and _rev_resolves(sha): + results.append(check_scope(label, ["--message", "--rev", sha], sha=sha)) + continue + env = None + if sha in API_AUTHORS: + name, email = API_AUTHORS[sha] + env = {"GIT_AUTHOR_NAME": name, "GIT_AUTHOR_EMAIL": email} results.append( - check_scope( - f"Commit {index}/{total}", ["--message"], input_text=msg, sha=sha - ) + check_scope(label, ["--message"], input_text=msg, sha=sha, env=env) ) return results diff --git a/main_test.py b/main_test.py index 8ab2f76..be5eb5b 100644 --- a/main_test.py +++ b/main_test.py @@ -390,6 +390,45 @@ def test_invalid_json_falls_back_to_raw_text(self): class TestRunPrMessageChecks(unittest.TestCase): + """Commits the clone lacks are piped in; the rest are checked by hash.""" + + def setUp(self): + resolves = patch("main._rev_resolves", return_value=False) + resolves.start() + self.addCleanup(resolves.stop) + main.API_AUTHORS.clear() + self.addCleanup(main.API_AUTHORS.clear) + + def test_a_commit_in_the_clone_is_checked_by_its_hash(self): + """The CLI then reads that commit's own author (CC017, ignore_authors).""" + mock_result = MagicMock(returncode=0, stdout=json_output(make_check("message"))) + with ( + patch("main._rev_resolves", return_value=True), + patch("main.subprocess.run", return_value=mock_result) as mock_run, + ): + scopes = main.run_pr_message_checks([(SHA_A, "fix: something")]) + self.assertEqual(scopes[0].status, "pass") + self.assertEqual(scopes[0].sha, SHA_A) + self.assertEqual( + mock_run.call_args[0][0], + ["commit-check", "--format", "json", "--message", "--rev", SHA_A], + ) + self.assertIsNone(mock_run.call_args[1]["input"]) + self.assertIsNone(mock_run.call_args[1]["env"]) + + def test_an_api_listed_commit_carries_its_author(self): + """No commit to read, so the API's author goes in git's variables.""" + main.API_AUTHORS[SHA_A] = ("google-labs-jules[bot]", "jules@example.com") + mock_result = MagicMock(returncode=0, stdout=json_output(make_check("message"))) + with patch("main.subprocess.run", return_value=mock_result) as mock_run: + main.run_pr_message_checks([(SHA_A, "fix: something")]) + self.assertEqual(mock_run.call_args[1]["input"], "fix: something") + env = mock_run.call_args[1]["env"] + self.assertEqual(env["GIT_AUTHOR_NAME"], "google-labs-jules[bot]") + self.assertEqual(env["GIT_AUTHOR_EMAIL"], "jules@example.com") + # The rest of the environment is kept: PATH finds the CLI. + self.assertEqual(env.get("PATH"), os.environ.get("PATH")) + def test_single_message_pass(self): mock_result = MagicMock(returncode=0, stdout=json_output(make_check("message"))) with patch("main.subprocess.run", return_value=mock_result) as mock_run: @@ -1158,9 +1197,17 @@ class TestGetMessagesFromApi(unittest.TestCase): def _run(self, total, api_commits, head="b2"): pull = MagicMock() pull.get_commits.return_value = [ - MagicMock(sha=sha, commit=MagicMock(message=msg)) + MagicMock( + sha=sha, + commit=MagicMock( + message=msg, + author=MagicMock(email=f"{sha}@example.com"), + ), + ) for sha, msg in api_commits ] + for listed in pull.get_commits.return_value: + listed.commit.author.name = f"author {listed.sha}" github_module = MagicMock() github_module.Github.return_value.get_repo.return_value.get_pull.return_value = pull env = { @@ -1185,6 +1232,37 @@ def test_lists_the_commits_oldest_first(self): "https://ghe.example.com/api/v3", ) + def test_records_each_commit_author(self): + main.API_AUTHORS.clear() + self.addCleanup(main.API_AUTHORS.clear) + self._run(2, [("a1", "feat: one"), ("b2", "fix: two")]) + self.assertEqual( + main.API_AUTHORS, + { + "a1": ("author a1", "a1@example.com"), + "b2": ("author b2", "b2@example.com"), + }, + ) + + def test_a_commit_without_an_author_records_nothing(self): + main.API_AUTHORS.clear() + self.addCleanup(main.API_AUTHORS.clear) + pull = MagicMock() + pull.get_commits.return_value = [ + MagicMock(sha="b2", commit=MagicMock(message="fix: two", author=None)) + ] + github_module = MagicMock() + github_module.Github.return_value.get_repo.return_value.get_pull.return_value = pull + env = {"GITHUB_TOKEN": "token", "GITHUB_REPOSITORY": "owner/repo"} + with ( + patch.dict(os.environ, env), + patch("main.get_pr_event", return_value={"number": 7, "commits": 1}), + patch("main.get_pr_head_sha", return_value="b2"), + patch.dict(sys.modules, {"github": github_module}), + ): + self.assertEqual(main.get_messages_from_api(), [("b2", "fix: two")]) + self.assertEqual(main.API_AUTHORS, {}) + def test_a_list_that_ends_elsewhere_is_not_used(self): # A force push after the event: same count, other commits. result, _ = self._run(2, [("a1", "feat: one"), ("c3", "fix: other")]) From 840a41a215e712ad7e97b9a33124f67e296b7587 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Sun, 8 Nov 2026 03:53:42 +0000 Subject: [PATCH 2/2] fix: pipe each pull request commit with its own author --- main.py | 36 ++++++++++++----- main_test.py | 112 +++++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 126 insertions(+), 22 deletions(-) diff --git a/main.py b/main.py index bbcfd46..a63907c 100755 --- a/main.py +++ b/main.py @@ -311,6 +311,23 @@ def _rev_resolves(rev: str) -> bool: return result.returncode == 0 +def _local_author(sha: str) -> tuple[str, str] | None: + """Author (name, email) of a commit the clone holds, or ``None``.""" + try: + result = subprocess.run( + ["git", "show", "-s", "--format=%an%x1f%ae", sha], + capture_output=True, + encoding="utf-8", + check=False, + ) + except OSError: + return None + name, sep, email = result.stdout.strip("\n").partition("\x1f") + if result.returncode != 0 or not sep: + return None + return name, email + + def _fetch_commit(sha: str) -> bool: """Fetch one commit, without its history, into a clone that lacks it.""" try: @@ -653,23 +670,22 @@ def check_scope( def run_pr_message_checks(pr_commits: list[Commit]) -> list[ScopeResult]: """Check each PR commit individually via commit-check --message. - A commit the clone holds is checked by its hash (``--rev``), so the CLI - reads its message and its own author: the AI author rule (CC017) and + The raw message is piped in, so every commit is read the same way + whether the clone holds it or only the API listed it. Its own author + goes with it as git's author variables, which is where the CLI reads the + author of a piped message: the AI author rule (CC017) and ``ignore_authors`` then judge that commit's author, not whoever HEAD or - the runner's git config names. A commit listed only by the API is piped - in, with the author the API recorded passed as git's author variables. + the runner's git config names. """ results: list[ScopeResult] = [] total = len(pr_commits) for index, (sha, msg) in enumerate(pr_commits, start=1): label = f"Commit {index}/{total}" - if sha and _rev_resolves(sha): - results.append(check_scope(label, ["--message", "--rev", sha], sha=sha)) - continue + author = API_AUTHORS.get(sha) or (_local_author(sha) if sha else None) env = None - if sha in API_AUTHORS: - name, email = API_AUTHORS[sha] - env = {"GIT_AUTHOR_NAME": name, "GIT_AUTHOR_EMAIL": email} + if author: + names = ("GIT_AUTHOR_NAME", "GIT_AUTHOR_EMAIL") + env = {key: value for key, value in zip(names, author) if value} or None results.append( check_scope(label, ["--message"], input_text=msg, sha=sha, env=env) ) diff --git a/main_test.py b/main_test.py index be5eb5b..2c3280a 100644 --- a/main_test.py +++ b/main_test.py @@ -1,5 +1,6 @@ """Unit tests for main.py.""" +import functools import importlib.metadata import io import json @@ -7,6 +8,7 @@ import re import runpy import shutil +import subprocess import sys import tempfile import unittest @@ -390,32 +392,55 @@ def test_invalid_json_falls_back_to_raw_text(self): class TestRunPrMessageChecks(unittest.TestCase): - """Commits the clone lacks are piped in; the rest are checked by hash.""" + """Each commit's raw message is piped in, with that commit's own author.""" def setUp(self): - resolves = patch("main._rev_resolves", return_value=False) - resolves.start() - self.addCleanup(resolves.stop) + local = patch("main._local_author", return_value=None) + self.local_author = local.start() + self.addCleanup(local.stop) main.API_AUTHORS.clear() self.addCleanup(main.API_AUTHORS.clear) - def test_a_commit_in_the_clone_is_checked_by_its_hash(self): - """The CLI then reads that commit's own author (CC017, ignore_authors).""" + def test_a_commit_in_the_clone_carries_its_own_author(self): + """The raw message is still piped, so it reads as before (no --rev).""" + self.local_author.return_value = ("Renovate Bot", "bot@renovateapp.com") mock_result = MagicMock(returncode=0, stdout=json_output(make_check("message"))) - with ( - patch("main._rev_resolves", return_value=True), - patch("main.subprocess.run", return_value=mock_result) as mock_run, - ): + with patch("main.subprocess.run", return_value=mock_result) as mock_run: scopes = main.run_pr_message_checks([(SHA_A, "fix: something")]) self.assertEqual(scopes[0].status, "pass") self.assertEqual(scopes[0].sha, SHA_A) + self.local_author.assert_called_once_with(SHA_A) self.assertEqual( mock_run.call_args[0][0], - ["commit-check", "--format", "json", "--message", "--rev", SHA_A], + ["commit-check", "--format", "json", "--message"], ) - self.assertIsNone(mock_run.call_args[1]["input"]) + self.assertEqual(mock_run.call_args[1]["input"], "fix: something") + env = mock_run.call_args[1]["env"] + self.assertEqual(env["GIT_AUTHOR_NAME"], "Renovate Bot") + self.assertEqual(env["GIT_AUTHOR_EMAIL"], "bot@renovateapp.com") + + def test_a_blank_author_field_is_left_to_git(self): + self.local_author.return_value = ("Renovate Bot", "") + mock_result = MagicMock(returncode=0, stdout=json_output(make_check("message"))) + with patch("main.subprocess.run", return_value=mock_result) as mock_run: + main.run_pr_message_checks([(SHA_A, "fix: something")]) + env = mock_run.call_args[1]["env"] + self.assertEqual(env["GIT_AUTHOR_NAME"], "Renovate Bot") + self.assertNotEqual(env.get("GIT_AUTHOR_EMAIL"), "") + + def test_no_author_at_all_passes_no_environment(self): + self.local_author.return_value = ("", "") + mock_result = MagicMock(returncode=0, stdout=json_output(make_check("message"))) + with patch("main.subprocess.run", return_value=mock_result) as mock_run: + main.run_pr_message_checks([(SHA_A, "fix: something")]) self.assertIsNone(mock_run.call_args[1]["env"]) + def test_a_commit_without_a_hash_is_not_looked_up(self): + mock_result = MagicMock(returncode=0, stdout=json_output(make_check("message"))) + with patch("main.subprocess.run", return_value=mock_result): + main.run_pr_message_checks([("", "fix: something")]) + self.local_author.assert_not_called() + def test_an_api_listed_commit_carries_its_author(self): """No commit to read, so the API's author goes in git's variables.""" main.API_AUTHORS[SHA_A] = ("google-labs-jules[bot]", "jules@example.com") @@ -490,6 +515,69 @@ def test_empty_list(self): mock_run.assert_not_called() +class TestLocalAuthor(unittest.TestCase): + """The author of a commit the clone holds, read from git.""" + + def test_reads_name_and_email(self): + result = MagicMock(returncode=0, stdout="Jane Dev\x1fjane@example.com\n") + with patch("main.subprocess.run", return_value=result) as mock_run: + self.assertEqual( + main._local_author(SHA_A), ("Jane Dev", "jane@example.com") + ) + self.assertEqual( + mock_run.call_args[0][0], + ["git", "show", "-s", "--format=%an%x1f%ae", SHA_A], + ) + + def test_a_commit_git_cannot_show_has_no_author(self): + result = MagicMock(returncode=128, stdout="") + with patch("main.subprocess.run", return_value=result): + self.assertIsNone(main._local_author(SHA_A)) + + def test_output_without_the_separator_has_no_author(self): + result = MagicMock(returncode=0, stdout="unexpected\n") + with patch("main.subprocess.run", return_value=result): + self.assertIsNone(main._local_author(SHA_A)) + + def test_missing_git_has_no_author(self): + with patch("main.subprocess.run", side_effect=OSError("no git")): + self.assertIsNone(main._local_author(SHA_A)) + + def test_reads_a_real_commit(self): + with tempfile.TemporaryDirectory() as repo: + env = { + **os.environ, + "GIT_AUTHOR_NAME": "Renovate Bot", + "GIT_AUTHOR_EMAIL": "bot@renovateapp.com", + "GIT_COMMITTER_NAME": "Renovate Bot", + "GIT_COMMITTER_EMAIL": "bot@renovateapp.com", + } + run = functools.partial( + subprocess.run, cwd=repo, env=env, check=True, capture_output=True + ) + run(["git", "init", "-q"]) + run( + [ + "git", + "-c", + "commit.gpgsign=false", + "commit", + "-q", + "--allow-empty", + "-m", + "chore: bump", + ] + ) + sha = run(["git", "rev-parse", "HEAD"], text=True).stdout.strip() + cwd = os.getcwd() + os.chdir(repo) + try: + author = main._local_author(sha) + finally: + os.chdir(cwd) + self.assertEqual(author, ("Renovate Bot", "bot@renovateapp.com")) + + class TestRunOtherChecks(unittest.TestCase): def test_empty_args_returns_no_scopes(self): with patch("main.subprocess.run") as mock_run: