diff --git a/README.md b/README.md index 26c9164..01eea15 100644 --- a/README.md +++ b/README.md @@ -52,7 +52,7 @@ jobs: steps: - uses: actions/checkout@v7 with: - fetch-depth: 0 # Required for merge-base checks + fetch-depth: 0 # With a shallow clone only HEAD, the merge commit, is checked - uses: commit-check/commit-check-action@v2 with: message: true @@ -116,7 +116,8 @@ jobs: ### `dry-run` -- **Description**: run checks without failing. exit code is 0; otherwise is 1. +- **Description**: report failures (job summary, PR comment, and annotations + downgraded to warnings) but always exit 0, so the job never fails. - Default: `false` ### `job-summary` diff --git a/action.yml b/action.yml index d02f20a..22441e4 100644 --- a/action.yml +++ b/action.yml @@ -22,7 +22,7 @@ inputs: required: false default: false dry-run: - description: run checks without failing + description: report failures (summary, PR comment, annotations as warnings) but always exit 0 required: false default: false job-summary: diff --git a/main.py b/main.py index 5ad0ca6..ded9b96 100755 --- a/main.py +++ b/main.py @@ -190,6 +190,140 @@ def is_pr_event() -> bool: return os.getenv("GITHUB_EVENT_NAME", "") in {"pull_request", "pull_request_target"} +#: The one fix for every "history is too shallow" finding below. +SHALLOW_CHECKOUT_HINT = "is actions/checkout using fetch-depth: 0?" + +#: On pull_request_target the default checkout is the base branch, which +#: holds none of the pull request at any depth; the fix is to check the +#: pull request out. +TARGET_CHECKOUT_HINT = ( + "is the workflow checking out the pull request, e.g. " + "ref: refs/pull//merge with fetch-depth: 0?" +) + + +def checkout_hint() -> str: + """The fix for a checkout that does not hold the pull request.""" + if os.getenv("GITHUB_EVENT_NAME") == "pull_request_target": + return TARGET_CHECKOUT_HINT + return SHALLOW_CHECKOUT_HINT + + +#: The pull request branch tip. On ``refs/pull/N/merge`` HEAD is a merge +#: commit that GitHub authored, so its recorded author is +#: ``GitHub `` whatever the contributor configured; +#: HEAD^2 is the commit the contributor actually made. +PR_HEAD_REV = "HEAD^2" + +#: The checks whose subject is a commit's recorded author. +AUTHOR_FLAGS = ("--author-name", "--author-email") + + +def warn_shallow_checkout(problem: str, consequence: str) -> None: + """Annotate a PR run whose clone is too shallow to do what was asked. + + ``actions/checkout`` defaults to ``fetch-depth: 1``, which leaves the + synthetic merge commit as the only commit in the clone. Every caller + hits that same root cause, so they share one message shape that names + the fix rather than only the symptom. + """ + text = f"{problem} ({checkout_hint()}); {consequence}" + print(f"::warning title=commit-check::{_annotation_escape(text)}") + + +def get_pr_event() -> dict[str, Any]: + """The ``pull_request`` object from the event payload, or ``{}``.""" + if not is_pr_event(): + return {} + event_path = os.getenv("GITHUB_EVENT_PATH") + if not event_path: + return {} + try: + with open(event_path, "r", encoding="utf-8") as f: + event = json.load(f) + return event.get("pull_request") or {} + except Exception as e: + print(f"::warning::Failed to read the PR from the event: {e}", file=sys.stderr) + return {} + + +def get_pr_head_sha() -> str | None: + """The pull request's head commit, from the event payload.""" + return get_pr_event().get("head", {}).get("sha") or None + + +def get_pr_base_sha() -> str | None: + """The base branch tip the pull request targets, from the event payload.""" + return get_pr_event().get("base", {}).get("sha") or None + + +def _rev_resolves(rev: str) -> bool: + """Whether ``rev`` names a commit the clone actually has.""" + try: + result = subprocess.run( + ["git", "rev-parse", "--verify", "--quiet", f"{rev}^{{commit}}"], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + encoding="utf-8", + check=False, + ) + except OSError: + return False + return result.returncode == 0 + + +def pr_head_rev() -> str | None: + """The commit whose recorded author the PR's author checks read. + + First choice is ``pull_request.head.sha`` from the event payload: it + names the branch tip for ``pull_request`` and ``pull_request_target`` + alike, whatever was checked out, as long as the clone has it. Failing + that, ``HEAD^2`` on a ``pull_request`` checkout, where HEAD is the + merge ref and its second parent is that same tip. Never ``HEAD^2`` on + ``pull_request_target``: there HEAD is the base branch, so ``HEAD^2`` + is nothing, or the parent of some unrelated merge on it. + + ``None`` when the clone is too shallow to hold either. + """ + sha = get_pr_head_sha() + if sha and _rev_resolves(sha): + return sha + if os.getenv("GITHUB_EVENT_NAME") == "pull_request" and _rev_resolves(PR_HEAD_REV): + return PR_HEAD_REV + return None + + +#: The rule each author check runs, for a scope that had to be skipped. +AUTHOR_RULES = { + "--author-name": ("CC101", "author_name"), + "--author-email": ("CC102", "author_email"), +} + + +def skipped_author_scope(flag: str) -> ScopeResult: + """A scope recording that an author check could not run at all. + + Reported as ``skip``, never as a pass: nothing was validated, and the + one commit the clone does hold (HEAD) has the wrong author for a pull + request, GitHub's merge commit or the base branch. + """ + rule_id, check = AUTHOR_RULES[flag] + return ScopeResult( + label=CHECK_LABELS[flag], + checks=[ + { + "rule_id": rule_id, + "check": check, + "status": "skip", + "value": "", + "error": "", + "suggest": "", + "docs_url": "", + } + ], + ) + + def get_pr_title() -> str | None: """Read PR title from GitHub event payload.""" if not is_pr_event(): @@ -215,10 +349,10 @@ def parse_commit_messages(output: str) -> list[str]: ] -def get_messages_from_merge_ref() -> list[str]: - """Read PR commit messages from GitHub's synthetic merge commit.""" +def _messages_in_range(revision_range: str) -> list[str]: + """Commit messages in ``revision_range``, oldest first, or ``[]``.""" result = subprocess.run( - ["git", "log", "--pretty=format:%B%x00", "--reverse", "HEAD^1..HEAD^2"], + ["git", "log", "--pretty=format:%B%x00", "--reverse", revision_range], stdout=subprocess.PIPE, stderr=subprocess.PIPE, encoding="utf-8", @@ -229,39 +363,57 @@ def get_messages_from_merge_ref() -> list[str]: return [] +def get_messages_from_event_range() -> list[str]: + """Read PR commit messages between the payload's base and head commits. + + ``pull_request.base.sha`` and ``pull_request.head.sha`` name the pull + request whatever the workflow checked out, for ``pull_request`` and + ``pull_request_target`` alike; the range is usable whenever the clone + holds both commits. + """ + base_sha, head_sha = get_pr_base_sha(), get_pr_head_sha() + if not (base_sha and head_sha): + return [] + if not (_rev_resolves(head_sha) and _rev_resolves(base_sha)): + return [] + return _messages_in_range(f"{base_sha}..{head_sha}") + + +def get_messages_from_merge_ref() -> list[str]: + """Read PR commit messages from GitHub's synthetic merge commit. + + Only meaningful on a ``pull_request`` checkout, where HEAD is + ``refs/pull/N/merge``. On ``pull_request_target`` HEAD is the base + branch: ``HEAD^2`` is then nothing, or the parent of some unrelated + merge on it, whose commits are not the pull request's. + """ + if os.getenv("GITHUB_EVENT_NAME") != "pull_request": + return [] + return _messages_in_range("HEAD^1..HEAD^2") + + def get_messages_from_head_ref(base_ref: str) -> list[str]: """Read PR commit messages when the workflow checks out the head SHA.""" - result = subprocess.run( - [ - "git", - "log", - "--pretty=format:%B%x00", - "--reverse", - f"origin/{base_ref}..HEAD", - ], - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - encoding="utf-8", - check=False, - ) - if result.returncode == 0 and result.stdout: - return parse_commit_messages(result.stdout) - return [] + return _messages_in_range(f"origin/{base_ref}..HEAD") def get_pr_commit_messages() -> list[str]: """Get all commit messages for the current PR workflow. - In pull_request-style workflows, actions/checkout checks out a synthetic merge - commit (HEAD = merge of PR branch into base). HEAD^1 is the base branch - tip, HEAD^2 is the PR branch tip. So HEAD^1..HEAD^2 gives all PR commits. - If the workflow explicitly checks out the PR head SHA instead, fall back to - diffing against origin/ when that ref is available locally. + The event payload's ``base.sha..head.sha`` is tried first: it names the + pull request exactly, whatever was checked out. On a ``pull_request`` + checkout HEAD is the synthetic merge commit, so ``HEAD^1..HEAD^2`` is + the same range. If the workflow checks out the PR head SHA instead, + diff against ``origin/`` when that ref is available locally. """ if not is_pr_event(): return [] try: + messages = get_messages_from_event_range() + if messages: + return messages + messages = get_messages_from_merge_ref() if messages: return messages @@ -323,13 +475,22 @@ def run_pr_message_checks(pr_messages: list[str]) -> list[ScopeResult]: return results -def run_other_checks(args: list[str]) -> list[ScopeResult]: - """Run each non-message check (branch, author) once, as its own scope.""" +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. + + ``rev`` goes to the author checks only: it names the commit whose + recorded author is validated (commit-check >= 2.16.0), which in a PR is + the branch tip rather than GitHub's merge commit. The branch check has + no commit to point at, so it never takes it. + """ results: list[ScopeResult] = [] for flag in args: label = CHECK_LABELS.get(flag) if label: - results.append(check_scope(label, [flag])) + cli_args = [flag] + if rev and flag in AUTHOR_FLAGS: + cli_args += ["--rev", rev] + results.append(check_scope(label, cli_args)) return results @@ -371,13 +532,36 @@ def run_commit_check() -> tuple[int, list[ScopeResult]]: # only validating the synthetic merge commit at HEAD. results.extend(run_pr_message_checks(pr_messages)) args = [a for a in args if a != "--message"] + elif is_pr_event(): + # Falling through to HEAD validates the synthetic merge commit, + # "Merge X into Y", which passes CC001 by default: a shallow + # clone used to turn every pull request green without a word. + warn_shallow_checkout( + "Could not list the pull request's commits", "only HEAD was checked" + ) # ---- 3. Remaining checks (branch, author, etc.) ----------------------- # Outside a PR, check the HEAD commit message directly. if "--message" in args: results.append(check_scope("Commit message", ["--message"])) args = [a for a in args if a != "--message"] - results.extend(run_other_checks(args)) + rev = None + if is_pr_event() and any(flag in AUTHOR_FLAGS for flag in args): + rev = pr_head_rev() + if rev is None: + # HEAD's author is GitHub's merge commit on a pull_request + # checkout and the base branch on pull_request_target: checking + # it would grade the wrong person either way. Say so, and skip. + warn_shallow_checkout( + "Could not resolve the pull request's head commit for the " + "author checks", + "they were skipped", + ) + for flag in args: + if flag in AUTHOR_FLAGS: + results.append(skipped_author_scope(flag)) + args = [a for a in args if a not in AUTHOR_FLAGS] + results.extend(run_other_checks(args, rev=rev)) exit_code = exit_code_for(results) return exit_code, results @@ -467,24 +651,26 @@ def _render_scopes(scopes: list[ScopeResult], include_docs: bool) -> list[str]: value = _scope_value(scope) lines.append(f" ✔ {scope.label}{f' ({value})' if value else ''}") continue - if scope.status == "warn": - # Reported like a failure — same detail lines — but never a ✖: - # a warned rule ran and found something, it just does not fail - # the workflow, and the marker says so at a glance. - warnings = scope.warnings - count = f" ({len(warnings)} warning{'s' if len(warnings) != 1 else ''})" - lines.append(f" ⚠ {scope.label}{count}") - lines.extend(_render_findings(warnings, include_docs)) - continue if scope.raw_text and not scope.checks: # Defensive fallback: commit-check produced unexpected output. lines.append(f" ✖ {scope.label}") lines.extend(f" {ln}" for ln in scope.raw_text.strip().splitlines()) continue + # A scope's status names its worst outcome (fail beats warn), but the + # two are not exclusive: CC2xx covers both branch and merge_base, so + # one can fail while the other only warns. Render whichever of the + # two lists is non-empty, rather than only the one the status names — + # a warning on an otherwise-failing scope is still a finding to fix. failures = scope.failures - count = f" ({len(failures)} failure{'s' if len(failures) != 1 else ''})" - lines.append(f" ✖ {scope.label}{count}") - lines.extend(_render_findings(failures, include_docs)) + if failures: + count = f" ({len(failures)} failure{'s' if len(failures) != 1 else ''})" + lines.append(f" ✖ {scope.label}{count}") + lines.extend(_render_findings(failures, include_docs)) + warnings = scope.warnings + if warnings: + count = f" ({len(warnings)} warning{'s' if len(warnings) != 1 else ''})" + lines.append(f" ⚠ {scope.label}{count}") + lines.extend(_render_findings(warnings, include_docs)) return lines @@ -520,6 +706,11 @@ def render_step_log(results: list[ScopeResult]) -> None: A failure becomes an ``::error``, a warning a ``::warning`` \u2014 GitHub renders the two differently, and only the errors count toward the friendly one-line verdict claiming nothing failed. + + Under ``dry-run`` a failure is still reported, but as a ``::warning``: + an ``::error`` annotation on a step that then exits 0 reads as a + contradiction, and the run's error count would claim a failure the job + did not have. The verdict line says the same thing in words. """ # The tree is grouped, so it is printed group by group rather than in one # block: ::group:: and ::endgroup:: have to bracket each section's lines. @@ -548,9 +739,10 @@ def render_step_log(results: list[ScopeResult]) -> None: first_line = error.splitlines()[0] if error else "check warning" warnings.append((_rule_label(check), f"{scope.label}: {first_line}")) + level = "warning" if DRY_RUN_ENABLED else "error" for title, message in errors: print( - f"::error title={_annotation_escape(title)}" + f"::{level} title={_annotation_escape(title)}" f"::{_annotation_escape(message)}" ) for title, message in warnings: @@ -559,6 +751,12 @@ def render_step_log(results: list[ScopeResult]) -> None: f"::{_annotation_escape(message)}" ) + if errors and DRY_RUN_ENABLED: + failed, total = _check_counts(results) + print( + f"commit-check (dry-run): {failed} of {total} checks failed; " + "not failing the job" + ) if not errors: skipped, warned, total = ( _skip_count(results), @@ -616,13 +814,15 @@ def _skip_count(results: list[ScopeResult]) -> int: def _warn_count(results: list[ScopeResult]) -> int: - """Number of scopes reported as warnings. + """Number of scopes that carry at least one warning. - Reported separately from the pass count, the same way skips are, so the - headline can say how many findings were surfaced without claiming they - failed anything. + Counts by ``scope.warnings``, not ``scope.status == "warn"``: a scope + whose overall status is "fail" (CC2xx covers both ``branch`` and + ``merge_base``, so one can fail while the other only warns) still has + warnings to report, and this is the count the verdict and the table use + to decide whether to show them. """ - return sum(1 for scope in results if scope.status == "warn") + return sum(1 for scope in results if scope.warnings) def _markdown_table( @@ -630,26 +830,32 @@ def _markdown_table( ) -> str: """Render the failure or warning table shared by summary and PR comment. - Only scopes of the given status appear, so a per-row result column would - read the same symbol on every row and carry no information; the pass/fail - picture for everything else lives in the details block. + A scope appears when it has an entry of the requested kind \u2014 checked via + ``scope.failures`` / ``scope.warnings``, not ``scope.status`` \u2014 so a scope + that both failed and warned gets a row in both tables. Filtering on the + scope's single overall status would silently drop its warnings once a + failure in the same scope outranked them. """ rows = [ f"| Scope | Checked value | {header} |", "|---|---|---|", ] + is_raw_only_failure = ( + status == "fail" + ) # a raw-text scope has no checks to warn about for scope in results: - # A skipped scope has no failed checks and no checked value, so it - # contributed an entirely blank row \u2014 an empty accusation in a table - # headed "Failed checks" \u2014 before this filter existed. - if scope.status != status: + entries = scope.failures if status == "fail" else scope.warnings + raw_failure = is_raw_only_failure and scope.raw_text and not scope.checks + # A skipped or clean-passing scope has no matching entries and is not + # a raw-text failure, so it contributes no row \u2014 an empty accusation + # in a table headed "Failed checks" or "Warnings" otherwise. + if not entries and not raw_failure: continue value = _scope_value(scope) value_display = f"`{value}`" if value else "\u2014" - if scope.raw_text and not scope.checks: + if raw_failure: links = "_output could not be parsed \u2014 see details_" else: - entries = scope.failures if status == "fail" else scope.warnings links = " \u00b7 ".join(_rule_markdown_link(check) for check in entries) rows.append(f"| {scope.label} | {value_display} | {links} |") return "\n".join(rows) @@ -1033,6 +1239,12 @@ def add_pr_comments(results: list[ScopeResult]) -> int: if not PR_COMMENTS_ENABLED: return 0 + # A push has no pull request to comment on. This used to fall through to + # get_pr_number(), which raised, and every push run carried a warning. + if not is_pr_event(): + print("Skipping PR comment: not a pull request event.") + return 0 + # Fork PRs triggered by the pull_request event receive a read-only token; # the GitHub API will always reject comment writes with 403. # pull_request_target events always have the configured token permissions. diff --git a/main_test.py b/main_test.py index 12bbf8b..d2a0674 100644 --- a/main_test.py +++ b/main_test.py @@ -1,8 +1,10 @@ """Unit tests for main.py.""" +import importlib.metadata import io import json import os +import re import sys import tempfile import unittest @@ -391,9 +393,26 @@ def test_non_pr_event_returns_empty(self): result = main.get_pr_commit_messages() self.assertEqual(result, []) - def test_merge_ref_is_preferred(self): + def test_event_range_is_preferred(self): with ( patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}), + patch( + "main.get_messages_from_event_range", + return_value=["fix: first", "feat: second"], + ) as mock_range, + patch("main.get_messages_from_merge_ref") as mock_merge, + patch("main.get_messages_from_head_ref") as mock_head, + ): + result = main.get_pr_commit_messages() + self.assertEqual(result, ["fix: first", "feat: second"]) + mock_range.assert_called_once() + mock_merge.assert_not_called() + mock_head.assert_not_called() + + def test_merge_ref_is_next(self): + with ( + patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}), + patch("main.get_messages_from_event_range", return_value=[]), patch( "main.get_messages_from_merge_ref", return_value=["fix: first", "feat: second"], @@ -408,7 +427,7 @@ def test_merge_ref_is_preferred(self): def test_pull_request_target_is_supported(self): with ( patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request_target"}), - patch("main.get_messages_from_merge_ref", return_value=["fix: first"]), + patch("main.get_messages_from_event_range", return_value=["fix: first"]), ): result = main.get_pr_commit_messages() self.assertEqual(result, ["fix: first"]) @@ -422,6 +441,7 @@ def test_falls_back_to_base_ref_when_merge_ref_is_unavailable(self): "GITHUB_BASE_REF": "main", }, ), + patch("main.get_messages_from_event_range", return_value=[]), patch("main.get_messages_from_merge_ref", return_value=[]), patch( "main.get_messages_from_head_ref", @@ -436,7 +456,8 @@ def test_exception_returns_empty(self): with ( patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}), patch( - "main.get_messages_from_merge_ref", side_effect=Exception("git failed") + "main.get_messages_from_event_range", + side_effect=Exception("git failed"), ), ): result = main.get_pr_commit_messages() @@ -448,7 +469,10 @@ def test_get_messages_from_merge_ref(self): mock_result = MagicMock( returncode=0, stdout="fix: first\n\x00feat: second\n\x00" ) - with patch("main.subprocess.run", return_value=mock_result) as mock_run: + with ( + patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}), + patch("main.subprocess.run", return_value=mock_result) as mock_run, + ): result = main.get_messages_from_merge_ref() self.assertEqual(result, ["fix: first", "feat: second"]) self.assertEqual( @@ -456,6 +480,57 @@ def test_get_messages_from_merge_ref(self): ["git", "log", "--pretty=format:%B%x00", "--reverse", "HEAD^1..HEAD^2"], ) + def test_merge_ref_is_never_read_on_pull_request_target(self): + """HEAD is the base branch there; HEAD^2 belongs to some other merge.""" + with ( + patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request_target"}), + patch("main.subprocess.run") as mock_run, + ): + self.assertEqual(main.get_messages_from_merge_ref(), []) + mock_run.assert_not_called() + + def test_get_messages_from_event_range(self): + commands: list[list[str]] = [] + + def run(command, **_kwargs): + commands.append(command) + if command[:2] == ["git", "rev-parse"]: + return MagicMock(returncode=0, stdout="x\n") + return MagicMock(returncode=0, stdout="fix: first\n\x00feat: second\n\x00") + + with ( + patch("main.get_pr_base_sha", return_value="base111"), + patch("main.get_pr_head_sha", return_value="head222"), + patch("main.subprocess.run", side_effect=run), + ): + result = main.get_messages_from_event_range() + self.assertEqual(result, ["fix: first", "feat: second"]) + self.assertIn( + ["git", "log", "--pretty=format:%B%x00", "--reverse", "base111..head222"], + commands, + ) + + def test_event_range_needs_both_commits_in_the_clone(self): + with ( + patch("main.get_pr_base_sha", return_value="base111"), + patch("main.get_pr_head_sha", return_value="head222"), + patch("main.subprocess.run", return_value=MagicMock(returncode=1)), + ): + self.assertEqual(main.get_messages_from_event_range(), []) + + def test_event_range_without_a_payload_is_empty(self): + with ( + patch("main.get_pr_base_sha", return_value=None), + patch("main.get_pr_head_sha", return_value=None), + patch("main.subprocess.run") as mock_run, + ): + self.assertEqual(main.get_messages_from_event_range(), []) + mock_run.assert_not_called() + + def test_a_failing_git_log_yields_no_messages(self): + with patch("main.subprocess.run", return_value=MagicMock(returncode=128)): + self.assertEqual(main.get_messages_from_head_ref("main"), []) + def test_get_messages_from_head_ref(self): mock_result = MagicMock(returncode=0, stdout="fix: first\n\x00") with patch("main.subprocess.run", return_value=mock_result) as mock_run: @@ -574,7 +649,7 @@ def test_non_pr_message_check_uses_commit_message_scope(self): def test_message_flag_removed_before_other_checks_in_pr(self): captured_args = [] - def fake_other_checks(args): + def fake_other_checks(args, rev=None): captured_args.extend(args) return [] @@ -591,6 +666,380 @@ def fake_other_checks(args): self.assertNotIn("--message", captured_args) self.assertIn("--branch", captured_args) + SHALLOW_PR_WARNING = ( + "::warning title=commit-check::Could not list the pull request's commits " + "(is actions/checkout using fetch-depth: 0?); only HEAD was checked" + ) + + def _run_capturing_stdout(self): + buffer = io.StringIO() + with patch("sys.stdout", buffer): + rc, results = main.run_commit_check() + return rc, results, buffer.getvalue() + + def test_pr_without_enumerable_commits_warns_and_checks_head(self): + """A shallow clone must not turn a pull request green silently. + + With fetch-depth: 1 neither HEAD^1..HEAD^2 nor origin/..HEAD + can be listed, and the fallback validates HEAD — the synthetic + "Merge X into Y" commit, which passes CC001 by default. + """ + with ( + patch("main.MESSAGE_ENABLED", True), + patch("main.BRANCH_ENABLED", False), + patch("main.AUTHOR_NAME_ENABLED", False), + patch("main.AUTHOR_EMAIL_ENABLED", False), + patch("main.is_pr_event", return_value=True), + patch("main.get_pr_commit_messages", return_value=[]), + patch( + "main.check_scope", return_value=pass_scope("Commit message") + ) as mock_scope, + patch("main.run_other_checks", return_value=[]), + ): + rc, results, output = self._run_capturing_stdout() + self.assertEqual(rc, 0) + self.assertIn(self.SHALLOW_PR_WARNING, output) + mock_scope.assert_called_once_with("Commit message", ["--message"]) + + def test_push_without_pr_commits_does_not_warn(self): + with ( + patch("main.MESSAGE_ENABLED", True), + patch("main.BRANCH_ENABLED", False), + patch("main.AUTHOR_NAME_ENABLED", False), + patch("main.AUTHOR_EMAIL_ENABLED", False), + patch("main.is_pr_event", return_value=False), + patch("main.get_pr_commit_messages", return_value=[]), + patch("main.check_scope", return_value=pass_scope("Commit message")), + patch("main.run_other_checks", return_value=[]), + ): + _rc, _results, output = self._run_capturing_stdout() + self.assertNotIn("::warning", output) + + @staticmethod + def _fake_git_and_cli(resolves: bool): + """subprocess.run stand-in: answers rev-parse and the CLI alike.""" + commands: list[list[str]] = [] + + def run(command, **_kwargs): + commands.append(command) + if command[:2] == ["git", "rev-parse"]: + return MagicMock( + returncode=0 if resolves else 1, + stdout="abc123\n" if resolves else "", + ) + check = command[3].lstrip("-").replace("-", "_") + return MagicMock(returncode=0, stdout=json_output(make_check(check))) + + return run, commands + + def test_pr_author_checks_read_the_branch_tip(self): + """On refs/pull/N/merge HEAD's author is GitHub, not the contributor.""" + run, commands = self._fake_git_and_cli(resolves=True) + with ( + patch("main.MESSAGE_ENABLED", False), + patch("main.BRANCH_ENABLED", True), + patch("main.AUTHOR_NAME_ENABLED", True), + patch("main.AUTHOR_EMAIL_ENABLED", True), + patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}), + patch("main.get_pr_head_sha", return_value=None), + patch("main.subprocess.run", side_effect=run), + ): + rc, results, output = self._run_capturing_stdout() + self.assertEqual(rc, 0) + self.assertEqual( + [s.label for s in results], ["Branch", "Author name", "Author email"] + ) + self.assertIn( + ["git", "rev-parse", "--verify", "--quiet", "HEAD^2^{commit}"], commands + ) + self.assertIn( + ["commit-check", "--format", "json", "--author-name", "--rev", "HEAD^2"], + commands, + ) + self.assertIn( + ["commit-check", "--format", "json", "--author-email", "--rev", "HEAD^2"], + commands, + ) + # The branch check has no commit to point at. + self.assertIn(["commit-check", "--format", "json", "--branch"], commands) + self.assertNotIn("::warning", output) + + def test_pr_author_checks_prefer_the_payload_head_sha(self): + """pull_request.head.sha names the tip for either PR event type.""" + run, commands = self._fake_git_and_cli(resolves=True) + with ( + patch("main.MESSAGE_ENABLED", False), + patch("main.BRANCH_ENABLED", False), + patch("main.AUTHOR_NAME_ENABLED", True), + patch("main.AUTHOR_EMAIL_ENABLED", False), + patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request_target"}), + patch("main.get_pr_head_sha", return_value="deadbeefcafe"), + patch("main.subprocess.run", side_effect=run), + ): + rc, results, output = self._run_capturing_stdout() + self.assertEqual(rc, 0) + self.assertEqual([s.status for s in results], ["pass"]) + self.assertIn( + ["git", "rev-parse", "--verify", "--quiet", "deadbeefcafe^{commit}"], + commands, + ) + self.assertIn( + [ + "commit-check", + "--format", + "json", + "--author-name", + "--rev", + "deadbeefcafe", + ], + commands, + ) + self.assertNotIn("::warning", output) + + def test_pr_author_checks_are_skipped_on_a_shallow_clone(self): + """HEAD's author is GitHub's merge commit: skip rather than grade it.""" + run, commands = self._fake_git_and_cli(resolves=False) + with ( + patch("main.MESSAGE_ENABLED", False), + patch("main.BRANCH_ENABLED", True), + patch("main.AUTHOR_NAME_ENABLED", True), + patch("main.AUTHOR_EMAIL_ENABLED", True), + patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}), + patch("main.get_pr_head_sha", return_value=None), + patch("main.subprocess.run", side_effect=run), + ): + rc, results, output = self._run_capturing_stdout() + self.assertEqual(rc, 0) + self.assertEqual( + [(s.label, s.status) for s in results], + [("Author name", "skip"), ("Author email", "skip"), ("Branch", "pass")], + ) + self.assertEqual( + results[0].checks, + [ + { + "rule_id": "CC101", + "check": "author_name", + "status": "skip", + "value": "", + "error": "", + "suggest": "", + "docs_url": "", + } + ], + ) + self.assertEqual(results[1].checks[0]["rule_id"], "CC102") + self.assertFalse([c for c in commands if "--author-name" in c], commands) + self.assertFalse([c for c in commands if "--author-email" in c], commands) + self.assertIn(["commit-check", "--format", "json", "--branch"], commands) + warning = [ln for ln in output.splitlines() if ln.startswith("::warning")] + self.assertEqual(len(warning), 1, output) + self.assertTrue(warning[0].startswith("::warning title=commit-check::")) + self.assertIn("Could not resolve the pull request's head commit", warning[0]) + self.assertIn("is actions/checkout using fetch-depth: 0?", warning[0]) + self.assertIn("they were skipped", warning[0]) + + def test_pull_request_target_never_uses_head2(self): + """On pull_request_target HEAD is the base branch; HEAD^2 is unrelated.""" + run, commands = self._fake_git_and_cli(resolves=True) + with ( + patch("main.MESSAGE_ENABLED", False), + patch("main.BRANCH_ENABLED", False), + patch("main.AUTHOR_NAME_ENABLED", True), + patch("main.AUTHOR_EMAIL_ENABLED", False), + patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request_target"}), + patch("main.get_pr_head_sha", return_value=None), + patch("main.subprocess.run", side_effect=run), + ): + rc, results, output = self._run_capturing_stdout() + self.assertEqual(rc, 0) + self.assertEqual( + [(s.label, s.status) for s in results], [("Author name", "skip")] + ) + self.assertFalse([c for c in commands if "HEAD^2^{commit}" in c], commands) + self.assertFalse([c for c in commands if c[0] == "commit-check"], commands) + self.assertIn("::warning title=commit-check::", output) + + def test_push_author_checks_never_pass_rev(self): + run, commands = self._fake_git_and_cli(resolves=True) + with ( + patch("main.MESSAGE_ENABLED", False), + patch("main.BRANCH_ENABLED", False), + patch("main.AUTHOR_NAME_ENABLED", True), + patch("main.AUTHOR_EMAIL_ENABLED", True), + patch("main.is_pr_event", return_value=False), + patch("main.subprocess.run", side_effect=run), + ): + _rc, _results, output = self._run_capturing_stdout() + self.assertFalse([c for c in commands if c[0] == "git"], commands) + self.assertFalse([c for c in commands if "--rev" in c], commands) + self.assertNotIn("::warning", output) + + +class TestPrHeadRev(unittest.TestCase): + def test_payload_head_sha_wins_when_the_clone_has_it(self): + with ( + patch("main.get_pr_head_sha", return_value="abc123"), + patch( + "main.subprocess.run", return_value=MagicMock(returncode=0) + ) as mock_run, + ): + self.assertEqual(main.pr_head_rev(), "abc123") + self.assertEqual( + mock_run.call_args[0][0], + ["git", "rev-parse", "--verify", "--quiet", "abc123^{commit}"], + ) + + def test_pull_request_falls_back_to_head2(self): + with ( + patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}), + patch("main.get_pr_head_sha", return_value=None), + patch( + "main.subprocess.run", return_value=MagicMock(returncode=0) + ) as mock_run, + ): + self.assertEqual(main.pr_head_rev(), "HEAD^2") + self.assertEqual( + mock_run.call_args[0][0], + ["git", "rev-parse", "--verify", "--quiet", "HEAD^2^{commit}"], + ) + + def test_pull_request_target_does_not_fall_back_to_head2(self): + with ( + patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request_target"}), + patch("main.get_pr_head_sha", return_value=None), + patch( + "main.subprocess.run", return_value=MagicMock(returncode=0) + ) as mock_run, + ): + self.assertIsNone(main.pr_head_rev()) + mock_run.assert_not_called() + + def test_unfetched_payload_sha_on_pull_request_target_returns_none(self): + with ( + patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request_target"}), + patch("main.get_pr_head_sha", return_value="abc123"), + patch("main.subprocess.run", return_value=MagicMock(returncode=1)), + ): + self.assertIsNone(main.pr_head_rev()) + + def test_shallow_clone_returns_none(self): + with ( + patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}), + patch("main.get_pr_head_sha", return_value=None), + patch("main.subprocess.run", return_value=MagicMock(returncode=1)), + ): + self.assertIsNone(main.pr_head_rev()) + + def test_missing_git_returns_none(self): + with ( + patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}), + patch("main.get_pr_head_sha", return_value=None), + patch("main.subprocess.run", side_effect=OSError("no git")), + ): + self.assertIsNone(main.pr_head_rev()) + + +class TestCheckoutHint(unittest.TestCase): + def test_pull_request_names_fetch_depth(self): + with patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}): + self.assertEqual(main.checkout_hint(), main.SHALLOW_CHECKOUT_HINT) + + def test_pull_request_target_names_the_checkout(self): + with patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request_target"}): + self.assertEqual(main.checkout_hint(), main.TARGET_CHECKOUT_HINT) + self.assertIn("refs/pull//merge", main.checkout_hint()) + + +class TestGetPrHeadSha(unittest.TestCase): + def test_reads_the_head_sha_from_the_event(self): + with tempfile.NamedTemporaryFile("w", suffix=".json", delete=False) as f: + json.dump({"pull_request": {"head": {"sha": "abc123"}}}, f) + event_path = f.name + try: + with patch.dict( + os.environ, + { + "GITHUB_EVENT_NAME": "pull_request_target", + "GITHUB_EVENT_PATH": event_path, + }, + ): + self.assertEqual(main.get_pr_head_sha(), "abc123") + finally: + os.unlink(event_path) + + def test_reads_the_base_sha_from_the_event(self): + with tempfile.NamedTemporaryFile("w", suffix=".json", delete=False) as f: + json.dump({"pull_request": {"base": {"sha": "base111"}}}, f) + event_path = f.name + try: + with patch.dict( + os.environ, + {"GITHUB_EVENT_NAME": "pull_request", "GITHUB_EVENT_PATH": event_path}, + ): + self.assertEqual(main.get_pr_base_sha(), "base111") + self.assertIsNone(main.get_pr_head_sha()) + finally: + os.unlink(event_path) + + def test_not_a_pr_event_returns_none(self): + with patch.dict(os.environ, {"GITHUB_EVENT_NAME": "push"}): + self.assertIsNone(main.get_pr_head_sha()) + self.assertIsNone(main.get_pr_base_sha()) + + def test_missing_event_path_returns_none(self): + with patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}): + os.environ.pop("GITHUB_EVENT_PATH", None) + self.assertIsNone(main.get_pr_head_sha()) + + def test_unreadable_event_returns_none(self): + with patch.dict( + os.environ, + { + "GITHUB_EVENT_NAME": "pull_request", + "GITHUB_EVENT_PATH": "/nonexistent.json", + }, + ): + self.assertIsNone(main.get_pr_head_sha()) + + +class TestCommitCheckVersionPin(unittest.TestCase): + """The warn rendering is inert against an engine that never emits it. + + commit-check reports ``"status": "warn"`` from 2.17.0; the action pinned + 2.16.0 for a release after shipping the rendering, so nobody could ever + see it. Both the installed package and the pin have to keep up. + """ + + MINIMUM = (2, 17, 0) + + @staticmethod + def _parse(version: str) -> tuple[int, ...]: + match = re.match(r"(\d+)\.(\d+)\.(\d+)", version) + assert match, f"unparsable commit-check version: {version!r}" + return tuple(int(part) for part in match.groups()) + + def test_installed_commit_check_can_report_warnings(self): + try: + version = importlib.metadata.version("commit-check") + except importlib.metadata.PackageNotFoundError: + self.skipTest("commit-check is not installed") + self.assertGreaterEqual( + self._parse(version), + self.MINIMUM, + f"installed commit-check {version} predates the warn status", + ) + + def test_requirements_pin_can_report_warnings(self): + here = os.path.dirname(os.path.abspath(main.__file__)) + with open(os.path.join(here, "requirements.txt"), encoding="utf-8") as f: + pins = dict( + line.strip().split("==", 1) + for line in f + if "==" in line and not line.startswith("#") + ) + self.assertGreaterEqual(self._parse(pins["commit-check"]), self.MINIMUM) + class TestRenderStepLog(unittest.TestCase): def _run(self, results): @@ -683,6 +1132,33 @@ def test_raw_text_fallback_is_printed(self): self.assertIn("unexpected output", output) self.assertIn("::error title=commit-check: Branch::", output) + def test_dry_run_downgrades_annotations_and_prints_verdict(self): + """Dry-run still reports every failure, but nothing may say "error". + + The exit code is forced to 0, so an ::error annotation would count + toward the run's error total on a green job, and log_error_and_exit + prints nothing for a zero exit — leaving the log with no verdict. + """ + with patch("main.DRY_RUN_ENABLED", True): + output = self._run([fail_scope("Commit 1/1"), pass_scope("Branch")]) + self.assertNotIn("::error", output) + self.assertIn( + "::warning title=CC001 message::Commit 1/1: The commit message should " + "follow Conventional Commits.", + output, + ) + self.assertIn( + "commit-check (dry-run): 1 of 2 checks failed; not failing the job", + output, + ) + self.assertNotIn("all checks passed", output) + + def test_dry_run_without_failures_prints_the_usual_verdict(self): + with patch("main.DRY_RUN_ENABLED", True): + output = self._run([pass_scope("Branch")]) + self.assertIn("✔ commit-check: all checks passed", output) + self.assertNotIn("dry-run", output) + class TestRenderJobSummary(unittest.TestCase): @pin_version @@ -963,9 +1439,47 @@ def test_disabled_returns_zero(self): rc = main.add_pr_comments([pass_scope()]) self.assertEqual(rc, 0) + def test_push_event_skips_comment_without_warning(self): + """A push has no PR to comment on, and that is not a problem. + + It used to reach get_pr_number(), which raised, so every push run + with pr-comments enabled carried a "Unable to post PR comment" + warning annotation. + """ + event_path = os.path.join(tempfile.mkdtemp(), "event.json") + with open(event_path, "w", encoding="utf-8") as f: + json.dump({"ref": "refs/heads/main", "pusher": {"name": "octocat"}}, f) + with ( + patch("main.PR_COMMENTS_ENABLED", True), + patch.dict( + os.environ, + { + "GITHUB_EVENT_NAME": "push", + "GITHUB_REF": "refs/heads/main", + "GITHUB_EVENT_PATH": event_path, + "GITHUB_TOKEN": "token", + "GITHUB_REPOSITORY": "owner/repo", + }, + ), + patch("main.get_pr_number") as mock_number, + patch("builtins.print") as mock_print, + ): + rc = main.add_pr_comments([fail_scope()]) + self.assertEqual(rc, 0) + mock_number.assert_not_called() + printed = [ + call[0][0] + for call in mock_print.call_args_list + if call[0] and isinstance(call[0][0], str) + ] + self.assertFalse( + [line for line in printed if line.startswith("::warning")], printed + ) + def test_fork_pr_skips_comment_and_warns(self): with ( patch("main.PR_COMMENTS_ENABLED", True), + patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}), patch("main.is_fork_pr", return_value=True), patch("main.JOB_SUMMARY_ENABLED", False), patch("builtins.print") as mock_print, @@ -980,6 +1494,7 @@ def test_fork_pr_writes_job_summary_hint(self): summary_path = os.path.join(tempfile.mkdtemp(), "summary.txt") with ( patch("main.PR_COMMENTS_ENABLED", True), + patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}), patch("main.is_fork_pr", return_value=True), patch("main.JOB_SUMMARY_ENABLED", True), patch("main.GITHUB_STEP_SUMMARY", summary_path), @@ -1010,6 +1525,7 @@ def test_creates_comment_with_rendered_body(self): { "GITHUB_TOKEN": "token", "GITHUB_REPOSITORY": "owner/repo", + "GITHUB_EVENT_NAME": "pull_request", "GITHUB_REF": "refs/pull/12/merge", }, ), @@ -1043,6 +1559,7 @@ def test_updates_existing_comment_when_changed(self): { "GITHUB_TOKEN": "token", "GITHUB_REPOSITORY": "owner/repo", + "GITHUB_EVENT_NAME": "pull_request", "GITHUB_REF": "refs/pull/12/merge", }, ), @@ -1073,6 +1590,7 @@ def test_skips_when_comment_is_up_to_date(self): { "GITHUB_TOKEN": "token", "GITHUB_REPOSITORY": "owner/repo", + "GITHUB_EVENT_NAME": "pull_request", "GITHUB_REF": "refs/pull/12/merge", }, ), @@ -1127,6 +1645,7 @@ def _run(self, side_effect): { "GITHUB_TOKEN": "token", "GITHUB_REPOSITORY": "owner/repo", + "GITHUB_EVENT_NAME": "pull_request", "GITHUB_REF": "refs/pull/12/merge", }, ), @@ -1471,6 +1990,68 @@ def test_a_real_failure_outranks_a_warning_in_the_same_scope(self): ) self.assertEqual(mixed.status, "fail") + def test_a_warning_survives_reporting_on_a_scope_that_also_fails(self): + """The scope's status names its worst outcome, but both findings + still belong on every surface: the count, both tables, and the + details block. Filtering any of those on ``scope.status == "warn"`` + drops this scope's warning the moment its sibling rule fails. + """ + mixed = main.ScopeResult( + label="Branch", + checks=[ + make_check( + "branch", + status="warn", + rule_id="CC201", + value="jsmith/fix-x", + error="The branch should follow Conventional Branch.", + suggest="Use /", + docs_url="https://commit-check.com/rules/#cc201", + ), + make_check( + "merge_base", + status="fail", + rule_id="CC202", + value="jsmith/fix-x", + error="Current branch is not rebased onto main.", + docs_url="https://commit-check.com/rules/#cc202", + ), + ], + ) + self.assertEqual(main._warn_count([mixed]), 1) + + body = main.render_report([mixed]) + self.assertIn("❌ **1 of 1 check failed**, 1 warning", body) + self.assertIn("| Scope | Checked value | Failed checks |", body) + self.assertIn("[CC202 merge-base](https://commit-check.com/rules/#cc202)", body) + self.assertIn("| Scope | Checked value | Warnings |", body) + self.assertIn("[CC201 branch](https://commit-check.com/rules/#cc201)", body) + self.assertIn(" ✖ Branch (1 failure)", body) + self.assertIn(" CC202 merge-base", body) + self.assertIn(" ⚠ Branch (1 warning)", body) + self.assertIn(" CC201 branch", body) + + def test_step_log_and_report_agree_on_a_mixed_scope(self): + """The step log's ::warning annotation must not be the only surface + that shows this scope's warning — the report has to as well.""" + mixed = main.ScopeResult( + label="Branch", + checks=[ + make_check("branch", status="warn", rule_id="CC201", error="e1"), + make_check("merge_base", status="fail", rule_id="CC202", error="e2"), + ], + ) + buf = io.StringIO() + with patch("sys.stdout", buf): + main.render_step_log([mixed]) + step_log = buf.getvalue() + self.assertIn("::warning title=CC201 branch::", step_log) + self.assertIn("::error title=CC202 merge-base::", step_log) + + body = main.render_report([mixed]) + self.assertIn("CC201 branch", body) + self.assertIn("CC202 merge-base", body) + def test_a_warning_outranks_a_skip_in_the_same_scope(self): mixed = main.ScopeResult( label="Author", diff --git a/requirements.txt b/requirements.txt index fc1afd7..50e4698 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,5 +1,5 @@ # Install commit-check CLI # For details please see: https://github.com/commit-check/commit-check -commit-check==2.16.0 +commit-check==2.17.0 # Interact with the GitHub API. PyGithub==2.10.0