From 55f4e27b1d05615cba3fc568810ab7951d6d211f Mon Sep 17 00:00:00 2001 From: shenxianpeng Date: Sun, 6 Sep 2026 02:36:16 +0300 Subject: [PATCH 01/11] feat: render a warned rule as a warning, not a failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit commit-check's CLI reports a rule listed under the config's top-level warn as status "warn" rather than "fail" (commit-check#565). The action's report follows: a warned scope gets its own row in the Warnings table (alongside the existing Failed checks table), its own ⚠ line in the step log and details block, and a ::warning annotation instead of ::error — but it never turns the workflow red. It counts toward "passed" in the verdict instead, e.g. "3 of 4 checks passed, 1 warning". A real failure still fails the run even when a warning sits in the same scope (CC202 stands in for branch when the two disagree), and the verdict then names both: "N of M checks failed, K warnings". ScopeResult.status and .warnings, overall_status/exit_code_for (warn already fell through to "pass", now documented), _render_scopes, render_step_log's annotations, and render_report/_markdown_table all follow. Against an older engine, or a config with no warn list, no check can ever be a warning, and every existing golden-output test keeps passing unchanged. Co-Authored-By: Claude Fable 5.1 --- README.md | 50 +++++++++++ main.py | 233 +++++++++++++++++++++++++++++++++++++++------------ main_test.py | 139 ++++++++++++++++++++++++++++++ 3 files changed, 367 insertions(+), 55 deletions(-) diff --git a/README.md b/README.md index 7b0e7e4..26c9164 100644 --- a/README.md +++ b/README.md @@ -203,6 +203,13 @@ for all available options. > [Optional Inputs](#optional-inputs), so env vars and config files are the > recommended way to customize. +The config's top-level `warn` reports a rule without failing the run — see +[Warning Job Summary](#warning-job-summary) for what that looks like: + +```toml +warn = ["branch", "CC003"] +``` + ## Outputs ### `result` @@ -338,6 +345,49 @@ This needs commit-check 2.13.4 or newer, which reports `"status": "skip"` in its JSON. Against an older engine every check is `pass` or `fail` as before, and the report is unchanged. +### Warning Job Summary + +A rule listed under the config's [top-level `warn`](#via-configuration-file) +still runs and is reported in full — its own table row, its own entry in the +details block — but it never fails the workflow. It counts toward "passed": + +> **Commit Check** +> +> ✅ **3 of 4 checks passed**, 1 warning +> +> | Scope | Checked value | Warnings | +> |---|---|---| +> | Branch | `jsmith/fix-x` | [CC201 branch](https://commit-check.com/rules/#cc201) | +> +>
+> Show all 4 checks +> +> ```text +> Commit message +> ✔ PR title (feat: add login page) +> ✔ Commit 1/2 (feat: add login page) +> Branch +> ⚠ Branch (1 warning) +> CC201 branch +> value: jsmith/fix-x +> The branch should follow Conventional Branch. +> Suggest: Use / with allowed types +> ``` +> +>
+> +> _commit-check <version> · [Rules reference](https://commit-check.com/rules/)_ + +A warned scope is marked `⚠`, never `✖`, and a real failure elsewhere still +fails the run — the verdict then reads `❌ **N of M checks failed**, K +warnings` and both tables appear. In the step log, a warning becomes a +`::warning` annotation rather than `::error`, so it never counts toward the +run's error count. + +This needs commit-check 2.17.0 or newer, which reports `"status": "warn"` in +its JSON. Against an older engine, or a config with no `warn` list, no check +can ever be a warning, and the report is unchanged. + ## GitHub Pull Request Comments With `pr-comments: true` the same report is posted as a pull request comment. diff --git a/main.py b/main.py index 649c3bf..5ad0ca6 100755 --- a/main.py +++ b/main.py @@ -101,7 +101,7 @@ class ScopeResult: @property def status(self) -> str: - """Overall status: ``pass``, ``fail``, or ``skip``. + """Overall status: ``pass``, ``fail``, ``warn``, or ``skip``. ``skip`` means every rule in this scope declined to run — the author is on an ``ignore_authors`` list, or there was nothing to check. It @@ -109,13 +109,19 @@ def status(self) -> str: validated nothing, and rendering the two identically let a bypassed policy read as an enforced one. - A single real verdict outranks the skips: a scope is ``skip`` only - when *all* of its checks skipped. + ``warn`` means a rule found something but is listed under the + config's top-level ``warn``, so commit-check reports it without + failing the run. A real failure still outranks a warning in the same + scope (CC202 stands in for ``branch`` when the two disagree), and a + single real verdict — failing or warning — outranks the skips: a + scope is ``skip`` only when *all* of its checks skipped. """ if self.raw_text and not self.checks: return "fail" if any(c["status"] == "fail" for c in self.checks): return "fail" + if any(c["status"] == "warn" for c in self.checks): + return "warn" if self.checks and all(c["status"] == "skip" for c in self.checks): return "skip" return "pass" @@ -125,6 +131,11 @@ def failures(self) -> list[dict[str, str]]: """The checks that failed in this scope.""" return [c for c in self.checks if c["status"] == "fail"] + @property + def warnings(self) -> list[dict[str, str]]: + """The checks reported as warnings in this scope.""" + return [c for c in self.checks if c["status"] == "warn"] + def overall_status(results: list[ScopeResult]) -> str: """Reduce scope statuses to one of ``pass``/``fail``/``skip``. @@ -134,7 +145,9 @@ def overall_status(results: list[ScopeResult]) -> str: correct only while exactly two statuses existed. The moment ``skip`` appeared they all silently reclassified a skipped run as a failure. - ``skip`` requires at least one scope and all of them skipped. + ``skip`` requires at least one scope and all of them skipped. A ``warn`` + scope is neither a failure nor a skip, so it falls through to ``pass`` + here — reported in full, but it never fails the workflow. """ if any(scope.status == "fail" for scope in results): return "fail" @@ -410,6 +423,27 @@ def _grouped(results: list[ScopeResult]) -> list[tuple[str, list[ScopeResult]]]: return groups +def _render_findings(checks: list[dict[str, str]], include_docs: bool) -> list[str]: + """Render the indented detail lines for a list of check entries. + + Shared by the failure and warning branches of ``_render_scopes`` — the + same rule label / value / error / suggestion / docs layout, whichever + list it is called with. + """ + lines: list[str] = [] + for check in checks: + lines.append(f" {_rule_label(check)}") + if check.get("value"): + lines.append(f" value: {check['value']}") + for line in check.get("error", "").splitlines(): + lines.append(f" {line}") + if check.get("suggest"): + lines.append(f" Suggest: {check['suggest']}") + if include_docs and check.get("docs_url"): + lines.append(f" Docs: {check['docs_url']}") + return lines + + def _render_scopes(scopes: list[ScopeResult], include_docs: bool) -> list[str]: """Render the indented listing for one group of scopes, without its header. @@ -418,9 +452,9 @@ def _render_scopes(scopes: list[ScopeResult], include_docs: bool) -> list[str]: docs link, which the Markdown report already carries on the rule ID in the table above it. - A failing scope shows its value in full rather than truncated. It is the one - value the reader has to act on, and the table's 60-character cap can cut off - the part that explains the failure. + A failing or warned scope shows its value in full rather than truncated. + It is the one value the reader has to act on, and the table's 60-character + cap can cut off the part that explains it. """ lines: list[str] = [] for scope in scopes: @@ -433,6 +467,15 @@ 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}") @@ -441,16 +484,7 @@ def _render_scopes(scopes: list[ScopeResult], include_docs: bool) -> list[str]: failures = scope.failures count = f" ({len(failures)} failure{'s' if len(failures) != 1 else ''})" lines.append(f" ✖ {scope.label}{count}") - for check in failures: - lines.append(f" {_rule_label(check)}") - if check.get("value"): - lines.append(f" value: {check['value']}") - for line in check.get("error", "").splitlines(): - lines.append(f" {line}") - if check.get("suggest"): - lines.append(f" Suggest: {check['suggest']}") - if include_docs and check.get("docs_url"): - lines.append(f" Docs: {check['docs_url']}") + lines.extend(_render_findings(failures, include_docs)) return lines @@ -473,15 +507,19 @@ def _annotation_escape(text: str) -> str: def render_step_log(results: list[ScopeResult]) -> None: - """Print results to the step log, then emit one annotation per failure. - - The two are separated deliberately. An ``::error`` command renders as a - line of its own wherever it is printed, so emitting one inside the indented - listing broke the tree apart, and its ``title=`` \u2014 which is what carries the - rule ID \u2014 is only shown in the annotations UI, never inline. Printing the - detail once in the listing and the annotations after all the groups keeps - the log readable and still surfaces failures in the run summary and on the - Files changed tab. + """Print results to the step log, then emit one annotation per finding. + + The tree and the annotations are separated deliberately. An ``::error`` or + ``::warning`` command renders as a line of its own wherever it is printed, + so emitting one inside the indented listing broke the tree apart, and its + ``title=`` \u2014 which is what carries the rule ID \u2014 is only shown in the + annotations UI, never inline. Printing the detail once in the listing and + the annotations after all the groups keeps the log readable and still + surfaces findings in the run summary and on the Files changed tab. + + 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. """ # 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. @@ -491,34 +529,53 @@ def render_step_log(results: list[ScopeResult]) -> None: print(line) print("::endgroup::") - annotations: list[tuple[str, str]] = [] + errors: list[tuple[str, str]] = [] + warnings: list[tuple[str, str]] = [] for scope in results: - if scope.status == "pass": + if scope.status in ("pass", "skip"): continue if scope.raw_text and not scope.checks: - annotations.append( + errors.append( (f"commit-check: {scope.label}", "output could not be parsed") ) continue for check in scope.failures: error = check.get("error", "") first_line = error.splitlines()[0] if error else "check failed" - annotations.append((_rule_label(check), f"{scope.label}: {first_line}")) + errors.append((_rule_label(check), f"{scope.label}: {first_line}")) + for check in scope.warnings: + error = check.get("error", "") + first_line = error.splitlines()[0] if error else "check warning" + warnings.append((_rule_label(check), f"{scope.label}: {first_line}")) - for title, message in annotations: + for title, message in errors: print( f"::error title={_annotation_escape(title)}" f"::{_annotation_escape(message)}" ) + for title, message in warnings: + print( + f"::warning title={_annotation_escape(title)}" + f"::{_annotation_escape(message)}" + ) - if not annotations: - skipped, total = _skip_count(results), len(results) + if not errors: + skipped, warned, total = ( + _skip_count(results), + _warn_count(results), + len(results), + ) if total and skipped == total: print("\u2298 commit-check: all checks skipped, nothing was validated") - elif skipped: + elif warned or skipped: + passed = total - skipped - warned + tail = [] + if warned: + tail.append(f"{warned} warning{'s' if warned != 1 else ''}") + if skipped: + tail.append(f"{skipped} skipped") print( - f"\u2714 commit-check: {total - skipped} of {total} checks passed, " - f"{skipped} skipped" + f"\u2714 commit-check: {passed} of {total} checks passed, {', '.join(tail)}" ) else: print("\u2714 commit-check: all checks passed") @@ -558,31 +615,42 @@ def _skip_count(results: list[ScopeResult]) -> int: return sum(1 for scope in results if scope.status == "skip") -def _markdown_table(results: list[ScopeResult]) -> str: - """Render the failure table shared by summary and PR comment. +def _warn_count(results: list[ScopeResult]) -> int: + """Number of scopes reported as warnings. - Only failed scopes appear, so a per-row result column would read ``\u274c`` on - every row and carry no information; the pass/fail picture for everything - else lives in the details block. + 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. + """ + return sum(1 for scope in results if scope.status == "warn") + + +def _markdown_table( + results: list[ScopeResult], status: str = "fail", header: str = "Failed checks" +) -> 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. """ rows = [ - "| Scope | Checked value | Failed checks |", + f"| Scope | Checked value | {header} |", "|---|---|---|", ] for scope in results: - # Only failures belong in this table. 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". - if scope.status != "fail": + # 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: continue value = _scope_value(scope) value_display = f"`{value}`" if value else "\u2014" if scope.raw_text and not scope.checks: links = "_output could not be parsed \u2014 see details_" else: - links = " \u00b7 ".join( - _rule_markdown_link(check) for check in scope.failures - ) + 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) @@ -677,6 +745,44 @@ def _scope_value(scope: ScopeResult, max_len: int = 60) -> str: # examined, so there is no value to report and no pass to claim. When only some # scopes skip, the verdict reads "✅ **3 of 5 checks passed**, 2 skipped". # +# Warning (the repository's config lists a rule under the top-level `warn`): +# +# +# ## Commit Check +# +# ✅ **3 of 4 checks passed**, 1 warning +# +# | Scope | Checked value | Warnings | +# |---|---|---| +# | Branch | `jsmith/fix-x` | [CC201 branch](https://commit-check.com/rules/#cc201) | +# +#
+# Show all 4 checks +# +# ```text +# Commit message +# ✔ PR title (feat: add login page) +# Branch +# ⚠ Branch (1 warning) +# CC201 branch +# value: jsmith/fix-x +# The branch should follow Conventional Branch. +# Suggest: Use / +# Author +# ✔ Author name (Jane Doe) +# ✔ Author email (jane@example.com) +# ``` +# +#
+# +# _commit-check 2.17.0 · [Rules reference](https://commit-check.com/rules/)_ +# +# A warned scope is reported exactly like a failing one — its own table row, +# its own entry in the details block — except the marker is ⚠ rather than ✖, +# it counts toward "passed" rather than "failed", and it never turns the +# workflow red. A real failure elsewhere still fails the run; the verdict +# then reads "❌ **N of M checks failed**, K warnings" and both tables appear. +# # Failure: # # @@ -748,26 +854,43 @@ def render_report(results: list[ScopeResult]) -> str: Opens with the hidden marker and the title, then a one-line verdict — ``✅ **All N checks passed**`` or ``❌ **N of M checks failed**`` — then the - failure table (failures only) and the collapsible per-scope details. + failure table (failures only), a warnings table when the config listed + any rule under ``warn``, and the collapsible per-scope details. + + A warned scope counts toward "passed", never toward "failed": it ran, + found something, and is reported in full, but the config asked for it to + be surfaced rather than enforced. """ failed, total = _check_counts(results) + warned = _warn_count(results) skipped = _skip_count(results) unit = "check" if total == 1 else "checks" lines = [COMMENT_MARKER, REPORT_TITLE, ""] if failed: - lines.append(f"❌ **{failed} of {total} {unit} failed**") + verdict = f"❌ **{failed} of {total} {unit} failed**" + if warned: + verdict += f", {warned} warning{'s' if warned != 1 else ''}" + lines.append(verdict) lines.extend(["", _markdown_table(results), ""]) + if warned: + lines.extend([_markdown_table(results, "warn", "Warnings"), ""]) elif total and skipped == total: # Nothing ran, so there is no success to announce. Saying "all # checks passed" here is the defect this branch exists to prevent. lines.append(f"⊘ **All {total} {unit} skipped** — nothing was validated") lines.append("") - elif skipped: - lines.append( - f"✅ **{total - skipped} of {total} {unit} passed**, {skipped} skipped" - ) + elif warned or skipped: + passed = total - skipped - warned + tail = [] + if warned: + tail.append(f"{warned} warning{'s' if warned != 1 else ''}") + if skipped: + tail.append(f"{skipped} skipped") + lines.append(f"✅ **{passed} of {total} {unit} passed**, {', '.join(tail)}") lines.append("") + if warned: + lines.extend([_markdown_table(results, "warn", "Warnings"), ""]) else: lines.append(f"✅ **All {total} {unit} passed**") lines.append("") diff --git a/main_test.py b/main_test.py index 9fe0a34..12bbf8b 100644 --- a/main_test.py +++ b/main_test.py @@ -1435,6 +1435,145 @@ def test_older_engine_without_skip_is_unaffected(self): self.assertIn("✅ **All 1 check passed**", main.render_report(results)) +def warn_scope(label: str = "Branch", value: str = "jsmith/fix-x") -> main.ScopeResult: + """A scope with one rule the config lists under the top-level ``warn``.""" + return main.ScopeResult( + label=label, + checks=[ + make_check( + "branch", + status="warn", + rule_id="CC201", + value=value, + error="The branch should follow Conventional Branch.", + suggest="Use /", + docs_url="https://commit-check.com/rules/#cc201", + ) + ], + ) + + +class TestWarnedScopes(unittest.TestCase): + """A rule listed under the config's top-level ``warn`` is reported in + full, but it must never read as a failure — or disappear like a pass.""" + + def test_scope_status_is_warn_not_fail_or_pass(self): + self.assertEqual(warn_scope().status, "warn") + + def test_a_real_failure_outranks_a_warning_in_the_same_scope(self): + """CC2xx groups both branch and merge_base; the two can disagree.""" + mixed = main.ScopeResult( + label="Branch", + checks=[ + make_check("branch", status="warn", rule_id="CC201"), + make_check("merge_base", status="fail", rule_id="CC202"), + ], + ) + self.assertEqual(mixed.status, "fail") + + def test_a_warning_outranks_a_skip_in_the_same_scope(self): + mixed = main.ScopeResult( + label="Author", + checks=[ + make_check("author_name", status="skip"), + make_check("author_email", status="warn", rule_id="CC102"), + ], + ) + self.assertEqual(mixed.status, "warn") + + def test_warnings_property_lists_only_the_warned_checks(self): + mixed = main.ScopeResult( + label="Branch", + checks=[ + make_check("branch", status="warn", rule_id="CC201"), + make_check("merge_base", status="pass", rule_id="CC202"), + ], + ) + self.assertEqual([c["rule_id"] for c in mixed.warnings], ["CC201"]) + + def test_overall_status_and_exit_code_treat_a_warning_as_a_pass(self): + results = [pass_scope("PR title"), warn_scope()] + self.assertEqual(main.overall_status(results), "pass") + self.assertEqual(main.exit_code_for(results), 0) + + def test_a_real_failure_still_fails_the_run_alongside_a_warning(self): + results = [fail_scope("Commit 1/1"), warn_scope()] + self.assertEqual(main.overall_status(results), "fail") + self.assertEqual(main.exit_code_for(results), 1) + + @pin_version + def test_warning_only_golden_output(self): + """Pin the warning report: passes, but the finding is fully visible.""" + results = [ + pass_scope("PR title", value="feat: add login page"), + warn_scope("Branch"), + ] + body = main.render_report(results) + self.assertEqual( + body, + f"{main.COMMENT_MARKER}\n" + f"{main.REPORT_TITLE}\n" + "\n" + "✅ **1 of 2 checks passed**, 1 warning\n" + "\n" + "| Scope | Checked value | Warnings |\n" + "|---|---|---|\n" + "| Branch | `jsmith/fix-x` | " + "[CC201 branch](https://commit-check.com/rules/#cc201) |\n" + "\n" + "
\n" + "Show all 2 checks\n" + "\n" + "```text\n" + "Commit message\n" + " ✔ PR title (feat: add login page)\n" + "Branch\n" + " ⚠ Branch (1 warning)\n" + " CC201 branch\n" + " value: jsmith/fix-x\n" + " The branch should follow Conventional Branch.\n" + " Suggest: Use /\n" + "```\n" + "\n" + "
\n" + "\n" + f"{FOOTER}", + ) + + def test_warning_and_skip_both_fold_into_the_title_in_order(self): + results = [pass_scope("PR title"), warn_scope(), skip_scope("Author")] + body = main.render_report(results) + self.assertIn("✅ **1 of 3 checks passed**, 1 warning, 1 skipped", body) + + def test_a_failure_still_fails_and_names_the_warning_too(self): + results = [fail_scope("Commit 1/1"), warn_scope()] + body = main.render_report(results) + self.assertIn("❌ **1 of 2 checks failed**, 1 warning", body) + self.assertIn("| Scope | Checked value | Failed checks |", body) + self.assertIn("| Scope | Checked value | Warnings |", body) + self.assertLess( + body.index("Failed checks"), + body.index("| Scope | Checked value | Warnings |"), + ) + + def test_step_log_prints_a_warning_annotation_not_an_error(self): + buf = io.StringIO() + with patch("sys.stdout", buf): + main.render_step_log([warn_scope()]) + out = buf.getvalue() + self.assertIn("::warning title=CC201 branch::Branch: The branch should ", out) + self.assertNotIn("::error", out) + self.assertIn(" ⚠ Branch (1 warning)", out) + + def test_step_log_friendly_line_mentions_the_warning(self): + buf = io.StringIO() + with patch("sys.stdout", buf): + main.render_step_log([pass_scope("PR title"), warn_scope()]) + out = buf.getvalue() + self.assertIn("1 of 2 checks passed, 1 warning", out) + self.assertNotIn("all checks passed", out) + + class TestSkipCompletionSemantics(unittest.TestCase): """A skipped run must not be treated as a failing one. From 192d6f7191d8f0eab512284313970ec77592aad1 Mon Sep 17 00:00:00 2001 From: shenxianpeng Date: Sun, 6 Sep 2026 02:44:19 +0300 Subject: [PATCH 02/11] fix: report a warning even when its scope also fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CC2xx groups both branch and merge_base, so the two can disagree: one warns while the other fails, and the scope's status names only the worse of the two. Every warning surface filtered on that single status, so a scope's warning vanished the moment a sibling rule in the same scope failed — the warnings count undercounted it, neither table showed it, and the details block only printed the failure. The step log's ::warning annotation was the one place it still appeared, because that loop already read scope.warnings directly. _warn_count and _markdown_table now check scope.failures/scope.warnings directly rather than scope.status, so a scope with both gets a row in both tables and counts toward both. _render_scopes renders whichever of the two lists is non-empty instead of branching once on status, so the same scope's failure and warning both reach the folded listing. Co-Authored-By: Claude Fable 5.1 --- main.py | 62 ++++++++++++++++++++++++++++++---------------------- main_test.py | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 26 deletions(-) diff --git a/main.py b/main.py index 5ad0ca6..5e0210b 100755 --- a/main.py +++ b/main.py @@ -467,24 +467,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 @@ -616,13 +618,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 +634,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) diff --git a/main_test.py b/main_test.py index 12bbf8b..4b1c042 100644 --- a/main_test.py +++ b/main_test.py @@ -1471,6 +1471,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", From bf2042fb9865765d4d9090b7954d67a3b89f3f33 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen Date: Sun, 6 Sep 2026 19:32:48 +0000 Subject: [PATCH 03/11] chore(deps): bump commit-check to 2.17.0 for the warn status The warn rendering merged in #276 needs commit-check to report "status": "warn", which it does from 2.17.0. The pin stayed at 2.16.0, which never emits it, so the feature was unreachable in every run. Add a test that asserts both the installed package and the requirements.txt pin are at least 2.17.0, so the pin cannot silently lag a feature that depends on the engine again. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018xYa7m3qup5wyN5MaXFgf6 --- main_test.py | 40 ++++++++++++++++++++++++++++++++++++++++ requirements.txt | 2 +- 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/main_test.py b/main_test.py index 12bbf8b..e039d13 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 @@ -592,6 +594,44 @@ def fake_other_checks(args): self.assertIn("--branch", captured_args) +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): buffer = io.StringIO() 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 From 4c84bff1009dfba86d2e5a5a1182937c37ffd722 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen Date: Sun, 6 Sep 2026 19:32:52 +0000 Subject: [PATCH 04/11] fix: warn when a shallow clone hides the pull request's commits On a pull_request event with the default fetch-depth: 1, neither HEAD^1..HEAD^2 nor origin/..HEAD can be listed, so get_pr_commit_messages() returns nothing and run_commit_check() falls back to validating HEAD. HEAD is the synthetic "Merge X into Y" commit, which passes CC001 by default: every pull request went green without a word about the commits that were never checked. Keep the HEAD fallback but emit a workflow warning naming the fix (actions/checkout with fetch-depth: 0), and correct the README comment on fetch-depth, which claimed it was for merge-base checks. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018xYa7m3qup5wyN5MaXFgf6 --- README.md | 2 +- main.py | 23 +++++++++++++++++++++++ main_test.py | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 26c9164..a1f7b0f 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 diff --git a/main.py b/main.py index 5ad0ca6..c3dd656 100755 --- a/main.py +++ b/main.py @@ -190,6 +190,22 @@ 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?" + + +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} ({SHALLOW_CHECKOUT_HINT}); {consequence}" + print(f"::warning title=commit-check::{_annotation_escape(text)}") + + def get_pr_title() -> str | None: """Read PR title from GitHub event payload.""" if not is_pr_event(): @@ -371,6 +387,13 @@ 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. diff --git a/main_test.py b/main_test.py index e039d13..f0e57d7 100644 --- a/main_test.py +++ b/main_test.py @@ -593,6 +593,55 @@ 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) + class TestCommitCheckVersionPin(unittest.TestCase): """The warn rendering is inert against an engine that never emits it. From 9703fabb84ba5f958d32adfdd55ff0793733b772 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen Date: Sun, 6 Sep 2026 19:32:56 +0000 Subject: [PATCH 05/11] fix: check the PR author on HEAD^2, not GitHub's merge commit On refs/pull/N/merge HEAD is a merge commit that GitHub authored, so author-name and author-email validated "GitHub " whatever the contributor had configured. In a PR event, pass --rev HEAD^2 (commit-check >= 2.16.0 reads that commit's recorded author) to the author checks. When HEAD^2 does not resolve, which is what a fetch-depth: 1 clone looks like, fall back to the previous behaviour and emit the same shallow-checkout warning as the commit-message path. The branch check is unaffected. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018xYa7m3qup5wyN5MaXFgf6 --- main.py | 54 +++++++++++++++++++++++++-- main_test.py | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 152 insertions(+), 5 deletions(-) diff --git a/main.py b/main.py index c3dd656..d477870 100755 --- a/main.py +++ b/main.py @@ -193,6 +193,15 @@ def is_pr_event() -> bool: #: The one fix for every "history is too shallow" finding below. SHALLOW_CHECKOUT_HINT = "is actions/checkout using fetch-depth: 0?" +#: 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. @@ -206,6 +215,25 @@ def warn_shallow_checkout(problem: str, consequence: str) -> None: print(f"::warning title=commit-check::{_annotation_escape(text)}") +def pr_head_rev() -> str | None: + """Return ``HEAD^2`` when it resolves, ``None`` on a shallow clone. + + With ``fetch-depth: 1`` the merge commit's parents are not fetched and + ``git rev-parse HEAD^2`` fails, so the caller has to settle for HEAD. + """ + try: + result = subprocess.run( + ["git", "rev-parse", "--verify", "--quiet", PR_HEAD_REV], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + encoding="utf-8", + check=False, + ) + except OSError: + return None + return PR_HEAD_REV if result.returncode == 0 else None + + def get_pr_title() -> str | None: """Read PR title from GitHub event payload.""" if not is_pr_event(): @@ -339,13 +367,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 @@ -400,7 +437,16 @@ def run_commit_check() -> tuple[int, list[ScopeResult]]: 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: + warn_shallow_checkout( + f"Could not resolve {PR_HEAD_REV} for the author checks", + "HEAD's author was checked instead, which on a pull request " + "is GitHub's merge commit", + ) + results.extend(run_other_checks(args, rev=rev)) exit_code = exit_code_for(results) return exit_code, results diff --git a/main_test.py b/main_test.py index f0e57d7..65a1fb4 100644 --- a/main_test.py +++ b/main_test.py @@ -576,7 +576,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 [] @@ -642,6 +642,107 @@ def test_push_without_pr_commits_does_not_warn(self): _rc, _results, output = self._run_capturing_stdout() self.assertNotIn("::warning", output) + @staticmethod + def _fake_git_and_cli(head2_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 head2_resolves else 1, + stdout="abc123\n" if head2_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(head2_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("main.is_pr_event", return_value=True), + 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"], 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_fall_back_to_head_on_shallow_clone(self): + run, commands = self._fake_git_and_cli(head2_resolves=False) + with ( + patch("main.MESSAGE_ENABLED", False), + patch("main.BRANCH_ENABLED", False), + patch("main.AUTHOR_NAME_ENABLED", True), + patch("main.AUTHOR_EMAIL_ENABLED", False), + patch("main.is_pr_event", return_value=True), + patch("main.subprocess.run", side_effect=run), + ): + rc, results, output = self._run_capturing_stdout() + self.assertEqual(rc, 0) + self.assertIn(["commit-check", "--format", "json", "--author-name"], commands) + self.assertFalse([c for c in commands if "--rev" in c], 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 HEAD^2", warning[0]) + self.assertIn("is actions/checkout using fetch-depth: 0?", warning[0]) + + def test_push_author_checks_never_pass_rev(self): + run, commands = self._fake_git_and_cli(head2_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_resolving_head2_returns_the_revision(self): + with 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"], + ) + + def test_shallow_clone_returns_none(self): + with patch("main.subprocess.run", return_value=MagicMock(returncode=1)): + self.assertIsNone(main.pr_head_rev()) + + def test_missing_git_returns_none(self): + with patch("main.subprocess.run", side_effect=OSError("no git")): + self.assertIsNone(main.pr_head_rev()) + class TestCommitCheckVersionPin(unittest.TestCase): """The warn rendering is inert against an engine that never emits it. From 8d638caa90de0ca30030771c38bd8f9e00ba2fca Mon Sep 17 00:00:00 2001 From: Xianpeng Shen Date: Sun, 6 Sep 2026 19:32:59 +0000 Subject: [PATCH 06/11] fix: skip the PR comment on push events without a warning pr-comments: true on a push event annotated every run with "::warning::Unable to post PR comment: Unable to determine PR number", because only PR_COMMENTS_ENABLED short-circuited add_pr_comments() and get_pr_number() raised for a ref that is not refs/pull/N/merge. Return early with a plain log line when the event is not a pull request: there is nothing to comment on, and that is not a problem worth an annotation. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018xYa7m3qup5wyN5MaXFgf6 --- main.py | 6 ++++++ main_test.py | 43 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/main.py b/main.py index d477870..dc0fe59 100755 --- a/main.py +++ b/main.py @@ -1102,6 +1102,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 65a1fb4..56afb0f 100644 --- a/main_test.py +++ b/main_test.py @@ -1153,9 +1153,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, @@ -1170,6 +1208,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), @@ -1200,6 +1239,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", }, ), @@ -1233,6 +1273,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", }, ), @@ -1263,6 +1304,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", }, ), @@ -1317,6 +1359,7 @@ def _run(self, side_effect): { "GITHUB_TOKEN": "token", "GITHUB_REPOSITORY": "owner/repo", + "GITHUB_EVENT_NAME": "pull_request", "GITHUB_REF": "refs/pull/12/merge", }, ), From 7158f3ab83179498ff7e54b001021dbe5b1372ed Mon Sep 17 00:00:00 2001 From: Xianpeng Shen Date: Sun, 6 Sep 2026 19:33:02 +0000 Subject: [PATCH 07/11] fix: report dry-run failures as warnings with a verdict line dry-run zeroed the exit code but still emitted ::error annotations for each failure, so a green step counted errors in the run summary, and log_error_and_exit() prints nothing for a zero exit, so the log ended with no verdict at all. Downgrade the per-failure annotations to ::warning (keeping the rule ID title) and print "commit-check (dry-run): N of M checks failed; not failing the job". Reword the dry-run input description in README and action.yml, which read "exit code is 0; otherwise is 1", to say what it does: failures are reported (summary, PR comment, annotations as warnings) but the job never fails. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018xYa7m3qup5wyN5MaXFgf6 --- README.md | 3 ++- action.yml | 2 +- main.py | 14 +++++++++++++- main_test.py | 27 +++++++++++++++++++++++++++ 4 files changed, 43 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index a1f7b0f..01eea15 100644 --- a/README.md +++ b/README.md @@ -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 dc0fe59..4e7c907 100755 --- a/main.py +++ b/main.py @@ -589,6 +589,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. @@ -617,9 +622,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: @@ -628,6 +634,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), diff --git a/main_test.py b/main_test.py index 56afb0f..0ef2129 100644 --- a/main_test.py +++ b/main_test.py @@ -873,6 +873,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 From 14ba09a1d030d8d5cbb2ef420993f336ccf5dbd0 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen Date: Sun, 6 Sep 2026 19:47:23 +0000 Subject: [PATCH 08/11] fix: resolve the pull request head from the event, skip authors without it pull_request.head.sha names the branch tip for pull_request and pull_request_target alike, whatever was checked out, so the author checks read it first. HEAD^2 stays as the fallback on a pull_request checkout only: on pull_request_target HEAD is the base branch, and its second parent, when it has one, belongs to some unrelated merge. When neither resolves, the author scopes are reported as skipped instead of being run on HEAD, whose author on a pull request is GitHub's merge commit or the base branch, never the contributor. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018xYa7m3qup5wyN5MaXFgf6 --- main.py | 93 +++++++++++++++++++++--- main_test.py | 195 +++++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 257 insertions(+), 31 deletions(-) diff --git a/main.py b/main.py index e61ebc8..0848922 100755 --- a/main.py +++ b/main.py @@ -215,23 +215,87 @@ def warn_shallow_checkout(problem: str, consequence: str) -> None: print(f"::warning title=commit-check::{_annotation_escape(text)}") -def pr_head_rev() -> str | None: - """Return ``HEAD^2`` when it resolves, ``None`` on a shallow clone. +def get_pr_head_sha() -> str | None: + """The pull request's head commit, from the event payload.""" + if not is_pr_event(): + return None + event_path = os.getenv("GITHUB_EVENT_PATH") + if not event_path: + return None + try: + with open(event_path, "r", encoding="utf-8") as f: + event = json.load(f) + return event.get("pull_request", {}).get("head", {}).get("sha") or None + except Exception as e: + print(f"::warning::Failed to read PR head from event: {e}", file=sys.stderr) + return None - With ``fetch-depth: 1`` the merge commit's parents are not fetched and - ``git rev-parse HEAD^2`` fails, so the caller has to settle for HEAD. - """ + +def _rev_resolves(rev: str) -> bool: + """Whether ``rev`` names a commit the clone actually has.""" try: result = subprocess.run( - ["git", "rev-parse", "--verify", "--quiet", PR_HEAD_REV], + ["git", "rev-parse", "--verify", "--quiet", f"{rev}^{{commit}}"], stdout=subprocess.PIPE, stderr=subprocess.PIPE, encoding="utf-8", check=False, ) except OSError: - return None - return PR_HEAD_REV if result.returncode == 0 else None + 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: @@ -441,11 +505,18 @@ def run_commit_check() -> tuple[int, list[ScopeResult]]: 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( - f"Could not resolve {PR_HEAD_REV} for the author checks", - "HEAD's author was checked instead, which on a pull request " - "is GitHub's merge commit", + "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) diff --git a/main_test.py b/main_test.py index 4f068e5..9493ff9 100644 --- a/main_test.py +++ b/main_test.py @@ -643,7 +643,7 @@ def test_push_without_pr_commits_does_not_warn(self): self.assertNotIn("::warning", output) @staticmethod - def _fake_git_and_cli(head2_resolves: bool): + def _fake_git_and_cli(resolves: bool): """subprocess.run stand-in: answers rev-parse and the CLI alike.""" commands: list[list[str]] = [] @@ -651,8 +651,8 @@ def run(command, **_kwargs): commands.append(command) if command[:2] == ["git", "rev-parse"]: return MagicMock( - returncode=0 if head2_resolves else 1, - stdout="abc123\n" if head2_resolves else "", + 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))) @@ -661,13 +661,14 @@ def run(command, **_kwargs): 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(head2_resolves=True) + 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("main.is_pr_event", return_value=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() @@ -675,7 +676,9 @@ def test_pr_author_checks_read_the_branch_tip(self): self.assertEqual( [s.label for s in results], ["Branch", "Author name", "Author email"] ) - self.assertIn(["git", "rev-parse", "--verify", "--quiet", "HEAD^2"], commands) + self.assertIn( + ["git", "rev-parse", "--verify", "--quiet", "HEAD^2^{commit}"], commands + ) self.assertIn( ["commit-check", "--format", "json", "--author-name", "--rev", "HEAD^2"], commands, @@ -688,28 +691,104 @@ def test_pr_author_checks_read_the_branch_tip(self): self.assertIn(["commit-check", "--format", "json", "--branch"], commands) self.assertNotIn("::warning", output) - def test_pr_author_checks_fall_back_to_head_on_shallow_clone(self): - run, commands = self._fake_git_and_cli(head2_resolves=False) + 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("main.is_pr_event", return_value=True), + 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.assertIn(["commit-check", "--format", "json", "--author-name"], commands) - self.assertFalse([c for c in commands if "--rev" in c], commands) + 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 HEAD^2", warning[0]) + 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(head2_resolves=True) + run, commands = self._fake_git_and_cli(resolves=True) with ( patch("main.MESSAGE_ENABLED", False), patch("main.BRANCH_ENABLED", False), @@ -725,25 +804,101 @@ def test_push_author_checks_never_pass_rev(self): class TestPrHeadRev(unittest.TestCase): - def test_resolving_head2_returns_the_revision(self): - with patch( - "main.subprocess.run", return_value=MagicMock(returncode=0) - ) as mock_run: + 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"], + ["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("main.subprocess.run", return_value=MagicMock(returncode=1)): + 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("main.subprocess.run", side_effect=OSError("no git")): + 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 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_not_a_pr_event_returns_none(self): + with patch.dict(os.environ, {"GITHUB_EVENT_NAME": "push"}): + 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. From 1635224cbdb47f368cd08ad6ddd6292b82711b74 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen Date: Sun, 6 Sep 2026 19:48:47 +0000 Subject: [PATCH 09/11] test: cover get_pr_head_sha without an event path Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018xYa7m3qup5wyN5MaXFgf6 --- main_test.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/main_test.py b/main_test.py index 9493ff9..228a745 100644 --- a/main_test.py +++ b/main_test.py @@ -888,6 +888,11 @@ 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()) + 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, From 3ad1e58a7c270c586bac75b7f8879e8212dfcb12 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen Date: Sun, 6 Sep 2026 20:02:27 +0000 Subject: [PATCH 10/11] fix: list the pull request's commits from the event, not HEAD^2 alone pull_request.base.sha..pull_request.head.sha names the pull request's commits whatever was checked out, so it is tried first. HEAD^1..HEAD^2 stays as the fallback on a pull_request checkout only: on pull_request_target HEAD is the base branch, and when its tip is itself a merge commit that range listed some unrelated branch's commits as the pull request's, with no warning. The "clone too shallow" hint names the right fix for each event: a deeper fetch on pull_request, checking the pull request out at all on pull_request_target. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018xYa7m3qup5wyN5MaXFgf6 --- main.py | 108 ++++++++++++++++++++++++++++++++++++--------------- main_test.py | 88 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 160 insertions(+), 36 deletions(-) diff --git a/main.py b/main.py index 0848922..ded9b96 100755 --- a/main.py +++ b/main.py @@ -193,6 +193,22 @@ def is_pr_event() -> bool: #: 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; @@ -211,24 +227,34 @@ def warn_shallow_checkout(problem: str, consequence: str) -> None: hits that same root cause, so they share one message shape that names the fix rather than only the symptom. """ - text = f"{problem} ({SHALLOW_CHECKOUT_HINT}); {consequence}" + text = f"{problem} ({checkout_hint()}); {consequence}" print(f"::warning title=commit-check::{_annotation_escape(text)}") -def get_pr_head_sha() -> str | None: - """The pull request's head commit, from the event payload.""" +def get_pr_event() -> dict[str, Any]: + """The ``pull_request`` object from the event payload, or ``{}``.""" if not is_pr_event(): - return None + return {} event_path = os.getenv("GITHUB_EVENT_PATH") if not event_path: - return None + return {} try: with open(event_path, "r", encoding="utf-8") as f: event = json.load(f) - return event.get("pull_request", {}).get("head", {}).get("sha") or None + return event.get("pull_request") or {} except Exception as e: - print(f"::warning::Failed to read PR head from event: {e}", file=sys.stderr) - return None + 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: @@ -323,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", @@ -337,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 diff --git a/main_test.py b/main_test.py index 228a745..ed22c3e 100644 --- a/main_test.py +++ b/main_test.py @@ -393,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"], @@ -410,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"]) @@ -424,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", @@ -438,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() @@ -450,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( @@ -458,6 +480,53 @@ 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_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: @@ -867,6 +936,17 @@ def test_missing_git_returns_none(self): 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: From 6c3e6405bd14f90de5297fdfb22ef83ea9f46035 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen Date: Sun, 6 Sep 2026 20:03:34 +0000 Subject: [PATCH 11/11] test: cover the base sha reader and a failing git log Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018xYa7m3qup5wyN5MaXFgf6 --- main_test.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/main_test.py b/main_test.py index ed22c3e..d2a0674 100644 --- a/main_test.py +++ b/main_test.py @@ -527,6 +527,10 @@ def test_event_range_without_a_payload_is_empty(self): 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: @@ -964,9 +968,24 @@ def test_reads_the_head_sha_from_the_event(self): 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"}):