From 73fd34f8d6f10cfd2be84967b4bc75d438078b04 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Tue, 10 Nov 2026 07:53:54 +0000 Subject: [PATCH 1/4] fix: never run the checkout's pip.py, reject non-boolean inputs, keep to own comments - Run venv and pip with python -I, so a pip.py or venv.py at the repository root no longer runs in their place with GITHUB_TOKEN. The self-test plants both files to keep it that way. - A boolean input that is neither true nor false (yes, on, a typo) now fails the step with the input's name; it used to turn the check off silently. - Only edit or delete a marked comment when a bot posted it. - Send the PR comment to GITHUB_API_URL, as the other API calls do, so it works on GitHub Enterprise Server. --- .github/workflows/commit-check.yml | 8 +++ action.yml | 12 ++-- main.py | 41 ++++++++++++-- main_test.py | 90 ++++++++++++++++++++++++++++++ 4 files changed, 141 insertions(+), 10 deletions(-) diff --git a/.github/workflows/commit-check.yml b/.github/workflows/commit-check.yml index 203d9cb..32e82ad 100644 --- a/.github/workflows/commit-check.yml +++ b/.github/workflows/commit-check.yml @@ -24,6 +24,14 @@ jobs: fetch-depth: 0 # Required for merge-base checks # Nothing after checkout needs authenticated git. persist-credentials: false + # The install step must never run a pip.py or venv.py from the + # checkout in place of pip or venv. If it did, these stop the job. + - name: Plant pip.py and venv.py in the workspace + shell: bash + run: | + for name in pip venv; do + echo 'raise SystemExit("a workspace module ran in place of pip or venv")' > "$name.py" + done - uses: ./ # self test with: message: true diff --git a/action.yml b/action.yml index 09f8894..8c829c0 100644 --- a/action.yml +++ b/action.yml @@ -73,7 +73,7 @@ runs: # The pinned commit-check needs Python 3.11+. Say so here: on an older # python3, pip would only report that no matching version exists. - if ! $PYTHON_CMD -c 'import sys; sys.exit(sys.version_info < (3, 11))'; then + if ! $PYTHON_CMD -I -c 'import sys; sys.exit(sys.version_info < (3, 11))'; then echo "::error::commit-check-action needs Python 3.11 or newer on PATH, found $($PYTHON_CMD --version 2>&1). Add actions/setup-python with python-version 3.11 or newer before this step." exit 1 fi @@ -84,7 +84,11 @@ runs: WORK="$RUNNER_TEMP/commit-check-action" rm -rf "$WORK" # a second invocation in the same job starts clean, so the wheel glob below matches exactly one file mkdir -p "$WORK/wheels" - $PYTHON_CMD -m venv "$WORK/venv" + # -I (isolated mode) everywhere below: `python -m` otherwise puts the + # current directory, the caller's checkout, first on sys.path, so a + # pip.py or venv.py at the repository root would run in place of pip + # or venv, with this step's GITHUB_TOKEN. + $PYTHON_CMD -I -m venv "$WORK/venv" if [[ "$RUNNER_OS" == "Windows" ]]; then source "$WORK/venv/Scripts/activate" else @@ -94,7 +98,7 @@ runs: # One download of the pinned closure, into the scratch dir. Wheels only: # the offline install below cannot build an sdist, so a platform without # binary wheels fails here, with pip's message, rather than later. - $PYTHON_CMD -m pip download -q --only-binary=:all: -d "$WORK/wheels" -r "$GITHUB_ACTION_PATH/requirements.txt" + $PYTHON_CMD -I -m pip download -q --only-binary=:all: -d "$WORK/wheels" -r "$GITHUB_ACTION_PATH/requirements.txt" # Verify the commit-check wheel's build provenance (PyGithub and the # transitive wheels are pinned but not attested; see README "Runner requirements"). @@ -106,7 +110,7 @@ runs: # Install offline from the wheels we already downloaded and verified: # no second trip to PyPI, and the installed set is exactly the downloaded set. - $PYTHON_CMD -m pip install -q --no-cache-dir --no-index --find-links "$WORK/wheels" -r "$GITHUB_ACTION_PATH/requirements.txt" + $PYTHON_CMD -I -m pip install -q --no-cache-dir --no-index --find-links "$WORK/wheels" -r "$GITHUB_ACTION_PATH/requirements.txt" $PYTHON_CMD "$GITHUB_ACTION_PATH/main.py" env: diff --git a/main.py b/main.py index 13c45c2..a103d1f 100755 --- a/main.py +++ b/main.py @@ -66,9 +66,22 @@ } +#: Boolean inputs whose value was neither true nor false, as ``(name, value)``. +#: ``main`` fails the step on them before any check runs. +INVALID_FLAGS: list[tuple[str, str]] = [] + + def env_flag(name: str, default: str = "false") -> bool: - """Read a GitHub Action boolean-style environment variable.""" - return os.getenv(name, default).lower() == "true" + """Read a GitHub Action boolean input: ``true`` or ``false``, any case. + + Anything else (``yes``, ``on``, a typo) used to read as false and turn + the check off without a word. It is now recorded in ``INVALID_FLAGS``, + and the step fails with the input's name. + """ + value = os.getenv(name, default).strip().lower() + if value not in ("true", "false"): + INVALID_FLAGS.append((name, os.getenv(name, default))) + return value == "true" def _reconfigure_io() -> None: @@ -1809,14 +1822,15 @@ def _is_bot(comment: Any) -> bool: def _find_own_comments(comments: list[Any]) -> tuple[Any | None, list[Any]]: """Pick the comment to update and the ones to delete. - Returns ``(target, stale)``. Only comments carrying ``COMMENT_MARKER`` are - ever deleted — those are unambiguously ours. A comment from an earlier + Returns ``(target, stale)``. Only comments a bot posted with + ``COMMENT_MARKER`` in them are ever edited or deleted: a person can paste + the marker too (quoting a report, say), and their comment is theirs. A comment from an earlier version has no marker, so it is adopted (edited, which adds the marker) when there is no marked comment yet, and only if a bot posted it: the legacy signal is a title prefix, which a person can type by accident, and editing someone's comment out from under them is not recoverable. """ - marked = [c for c in comments if COMMENT_MARKER in c.body] + marked = [c for c in comments if COMMENT_MARKER in c.body and _is_bot(c)] if marked: return marked[-1], marked[:-1] @@ -1887,7 +1901,8 @@ def add_pr_comments(results: list[ScopeResult]) -> int: if not repo_name: raise ValueError("GITHUB_REPOSITORY is not set") - g = Github(auth=Auth.Token(token)) + api_url = os.getenv("GITHUB_API_URL", "https://api.github.com") + g = Github(auth=Auth.Token(token), base_url=api_url) repo = g.get_repo(repo_name) pull_request = repo.get_issue(pr_number) @@ -2033,9 +2048,23 @@ def log_error_and_exit(ret_code: int) -> None: sys.exit(ret_code) +def fail_on_invalid_flags() -> None: + """Stop with an error when a boolean input is neither true nor false.""" + if not INVALID_FLAGS: + return + for name, value in INVALID_FLAGS: + input_name = name.lower().replace("_", "-") + print( + f"::error::Input {input_name} must be true or false, got " + f"{_log_safe(repr(value))}" + ) + sys.exit(1) + + def main(): """Main function to run commit-check and render all output surfaces.""" _reconfigure_io() + fail_on_invalid_flags() log_env_vars() ret_code, results = run_commit_check() diff --git a/main_test.py b/main_test.py index 5ddc8a3..6913850 100644 --- a/main_test.py +++ b/main_test.py @@ -2455,6 +2455,7 @@ def test_updates_existing_comment_when_changed(self): def test_skips_when_comment_is_up_to_date(self): body = main.render_pr_comment([fail_scope()]) existing = MagicMock(body=body) + existing.user.type = "Bot" mock_pull_request = MagicMock() mock_pull_request.get_comments.return_value = [existing] mock_repo = MagicMock() @@ -2904,6 +2905,14 @@ def test_bot_comment_from_an_older_version_is_adopted(self): self.assertIs(target, legacy) self.assertEqual(stale, []) + def test_human_comment_with_the_marker_is_never_touched(self): + """A person can paste the marker too, e.g. quoting a report.""" + quoted = self._comment(f"{main.COMMENT_MARKER}\nquoted", user_type="User") + mine = self._comment(f"{main.COMMENT_MARKER}\nreport") + self.assertEqual(main._find_own_comments([quoted, mine]), (mine, [])) + self.assertEqual(main._find_own_comments([mine, quoted]), (mine, [])) + self.assertEqual(main._find_own_comments([quoted]), (None, [])) + def test_no_comments_yields_nothing_to_update(self): target, stale = main._find_own_comments([]) self.assertIsNone(target) @@ -4096,3 +4105,84 @@ def test_a_missing_pygithub_warns(self): if __name__ == "__main__": unittest.main() + + +class TestBooleanInputs(unittest.TestCase): + """A boolean input is true or false; anything else fails the step.""" + + def setUp(self): + main.INVALID_FLAGS.clear() + self.addCleanup(main.INVALID_FLAGS.clear) + + def test_true_and_false_in_any_case_and_padding(self): + for value, expected in ( + ("true", True), + (" True ", True), + ("FALSE", False), + ("false", False), + ): + with patch.dict(os.environ, {"MESSAGE": value}): + self.assertIs(main.env_flag("MESSAGE"), expected, value) + self.assertEqual(main.INVALID_FLAGS, []) + + def test_unset_input_takes_the_default(self): + with patch.dict(os.environ, {}, clear=True): + self.assertFalse(main.env_flag("MESSAGE")) + self.assertEqual(main.INVALID_FLAGS, []) + + def test_other_values_are_recorded(self): + for value in ("yes", "on", "1", "ture"): + with patch.dict(os.environ, {"MESSAGE": value}): + self.assertFalse(main.env_flag("MESSAGE")) + self.assertEqual( + main.INVALID_FLAGS, + [ + ("MESSAGE", "yes"), + ("MESSAGE", "on"), + ("MESSAGE", "1"), + ("MESSAGE", "ture"), + ], + ) + + def test_invalid_input_fails_before_any_check(self): + main.INVALID_FLAGS.append(("PR_TITLE", "yes")) + out = io.StringIO() + with ( + patch("sys.stdout", out), + patch("main.run_commit_check") as run, + self.assertRaises(SystemExit) as cm, + ): + main.main() + self.assertEqual(cm.exception.code, 1) + run.assert_not_called() + self.assertIn( + "::error::Input pr-title must be true or false, got 'yes'", out.getvalue() + ) + + +class TestPrCommentApiUrl(unittest.TestCase): + """The PR comment goes to the same API host as every other call (GHES).""" + + def test_comment_client_uses_github_api_url(self): + github_module = MagicMock() + pull_request = MagicMock() + pull_request.get_comments.return_value = [] + github_module.Github.return_value.get_repo.return_value.get_issue.return_value = pull_request + with ( + patch("main.PR_COMMENTS_ENABLED", True), + patch("main.is_fork_pr_with_readonly_token", return_value=False), + patch("main.get_pr_number", return_value=7), + patch.dict( + os.environ, + { + "GITHUB_EVENT_NAME": "pull_request", + "GITHUB_TOKEN": "t", + "GITHUB_REPOSITORY": "o/r", + "GITHUB_API_URL": "https://ghe.example.com/api/v3", + }, + ), + patch.dict(sys.modules, {"github": github_module}), + ): + main.add_pr_comments([fail_scope()]) + _, kwargs = github_module.Github.call_args + self.assertEqual(kwargs["base_url"], "https://ghe.example.com/api/v3") From c8a239818d204852a57a58e3fe68e95e4dbf097c Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Tue, 10 Nov 2026 08:03:13 +0000 Subject: [PATCH 2/4] fix: check a commit with an empty message as empty, not as HEAD git log listing dropped such a commit, and piped empty text reads to the CLI as no input, so it checked HEAD (the merge commit) instead. The commit is now kept and its empty message goes to the CLI as an empty file, where allow_empty_commits judges it. --- main.py | 37 ++++++++++++++++++++++++++++--------- main_test.py | 39 +++++++++++++++++++++++++++++++++++++-- 2 files changed, 65 insertions(+), 11 deletions(-) diff --git a/main.py b/main.py index a103d1f..9130586 100755 --- a/main.py +++ b/main.py @@ -17,6 +17,7 @@ import re import subprocess import sys +import tempfile import uuid from dataclasses import dataclass, field from typing import Any @@ -567,14 +568,13 @@ def parse_commit_messages(output: str) -> list[Commit]: The fields alternate hash, message, hash, message; git puts a newline between commits, which lands on the front of the next hash and is - stripped along with the message's trailing newline. Commits are paired - before empty messages are dropped, so a blank message never shifts the - hashes of the commits after it. + stripped along with the message's trailing newline. A commit with an + empty message is kept, as ``(sha, "")``: it is a commit of the pull + request like any other, and ``allow_empty_commits`` is the rule that + judges it. """ fields = [f.strip("\n") for f in output.split(COMMIT_MESSAGE_DELIMITER)] - return [ - (sha, message) for sha, message in zip(fields[0::2], fields[1::2]) if message - ] + return [(sha, message) for sha, message in zip(fields[0::2], fields[1::2]) if sha] def _messages_in_range(revision_range: str) -> list[Commit]: @@ -831,12 +831,31 @@ def run_pr_message_checks(pr_commits: list[Commit]) -> list[ScopeResult]: 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) - ) + if msg.strip(): + scope = check_scope(label, ["--message"], input_text=msg, sha=sha, env=env) + else: + scope = check_empty_message(label, sha, env) + results.append(scope) return results +def check_empty_message( + label: str, sha: str, env: dict[str, str] | None +) -> ScopeResult: + """Check a commit whose message is empty, from an empty file. + + Piped, an empty message reads to the CLI as no input at all, and it + checks HEAD in its place: the merge commit, which passes. A file names + the message, so the empty one is judged (``allow_empty_commits``). + """ + fd, path = tempfile.mkstemp(prefix="commit-msg-", dir=os.getenv("RUNNER_TEMP")) + os.close(fd) + try: + return check_scope(label, ["--message", path], sha=sha, env=env) + finally: + os.unlink(path) + + def run_other_checks(args: list[str], rev: str | None = None) -> list[ScopeResult]: """Run each non-message check (branch, author) once, as its own scope. diff --git a/main_test.py b/main_test.py index 6913850..cda56b2 100644 --- a/main_test.py +++ b/main_test.py @@ -245,9 +245,12 @@ def test_the_format_asks_git_for_the_hash_and_the_body(self): self.assertEqual(main.COMMIT_LOG_FORMAT, "--pretty=format:%H%x00%B%x00") self.assertNotIn("\x00", main.COMMIT_LOG_FORMAT) - def test_an_empty_message_does_not_shift_the_hashes_after_it(self): + def test_an_empty_message_is_kept_and_does_not_shift_the_hashes(self): output = f"{SHA_A}\x00\n\x00\n{SHA_B}\x00feat: second\n\x00" - self.assertEqual(main.parse_commit_messages(output), [(SHA_B, "feat: second")]) + self.assertEqual( + main.parse_commit_messages(output), + [(SHA_A, ""), (SHA_B, "feat: second")], + ) def test_empty_output_is_no_commits(self): self.assertEqual(main.parse_commit_messages(""), []) @@ -4186,3 +4189,35 @@ def test_comment_client_uses_github_api_url(self): main.add_pr_comments([fail_scope()]) _, kwargs = github_module.Github.call_args self.assertEqual(kwargs["base_url"], "https://ghe.example.com/api/v3") + + +class TestEmptyCommitMessage(unittest.TestCase): + """An empty message is checked as empty, never as HEAD's message.""" + + def test_an_empty_message_goes_to_the_cli_as_an_empty_file(self): + seen: list[tuple[list[str], str | None, str]] = [] + + def run(command, **kwargs): + path = command[-1] + with open(path, encoding="utf-8") as f: + seen.append((command, kwargs.get("input"), f.read())) + return MagicMock( + returncode=1, + stdout=json_output(make_check("allow_empty_commits", status="fail")), + stderr="", + ) + + with ( + patch("main.subprocess.run", side_effect=run), + patch("main._local_author", return_value=None), + ): + results = main.run_pr_message_checks([("a1", "")]) + command, piped, content = seen[0] + self.assertEqual(command[:4], ["commit-check", "--format", "json", "--message"]) + self.assertIsNone(piped) + self.assertEqual(content, "") + self.assertFalse(os.path.exists(command[-1])) + self.assertEqual( + [(s.label, s.sha, s.status) for s in results], + [("Commit 1/1", "a1", "fail")], + ) From 3b292b1182bebd5cb9ac1a42439cb1b7969e72ef Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Tue, 10 Nov 2026 08:06:43 +0000 Subject: [PATCH 3/4] test: avoid a spelling the codespell hook flags --- main_test.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/main_test.py b/main_test.py index cda56b2..665238a 100644 --- a/main_test.py +++ b/main_test.py @@ -4134,7 +4134,7 @@ def test_unset_input_takes_the_default(self): self.assertEqual(main.INVALID_FLAGS, []) def test_other_values_are_recorded(self): - for value in ("yes", "on", "1", "ture"): + for value in ("yes", "on", "1", "tru"): with patch.dict(os.environ, {"MESSAGE": value}): self.assertFalse(main.env_flag("MESSAGE")) self.assertEqual( @@ -4143,7 +4143,7 @@ def test_other_values_are_recorded(self): ("MESSAGE", "yes"), ("MESSAGE", "on"), ("MESSAGE", "1"), - ("MESSAGE", "ture"), + ("MESSAGE", "tru"), ], ) From d3a1ead77dd0943fa7e62f59e14ccc1b36a015c1 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Tue, 10 Nov 2026 08:06:52 +0000 Subject: [PATCH 4/4] test: use a plain word for the invalid boolean input --- main_test.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/main_test.py b/main_test.py index 665238a..850a200 100644 --- a/main_test.py +++ b/main_test.py @@ -4134,7 +4134,7 @@ def test_unset_input_takes_the_default(self): self.assertEqual(main.INVALID_FLAGS, []) def test_other_values_are_recorded(self): - for value in ("yes", "on", "1", "tru"): + for value in ("yes", "on", "1", "maybe"): with patch.dict(os.environ, {"MESSAGE": value}): self.assertFalse(main.env_flag("MESSAGE")) self.assertEqual( @@ -4143,7 +4143,7 @@ def test_other_values_are_recorded(self): ("MESSAGE", "yes"), ("MESSAGE", "on"), ("MESSAGE", "1"), - ("MESSAGE", "tru"), + ("MESSAGE", "maybe"), ], )