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..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 @@ -66,9 +67,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: @@ -554,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]: @@ -818,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. @@ -1809,14 +1841,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 +1920,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 +2067,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..850a200 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(""), []) @@ -2455,6 +2458,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 +2908,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 +4108,116 @@ 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", "maybe"): + 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", "maybe"), + ], + ) + + 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") + + +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")], + )