fix: never run the checkout's pip.py, reject non-boolean inputs, check empty messages

.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

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:

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()

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")],
+        )