diff --git a/README.md b/README.md index 26c3538..1e4ddd9 100644 --- a/README.md +++ b/README.md @@ -303,6 +303,40 @@ A scope is one thing that was checked — a commit message, the branch, the auth — not one rule evaluation, so the total matches the ✔/✖ lines you can count and does not grow with the number of rules in your config. +### Skipped Job Summary + +Some runs validate nothing at all — most commonly when the commit author is +listed in `ignore_authors`, which is how Dependabot and other bots are usually +exempted. Those runs report `⊘`, never `✔`: + +> **Commit Check** +> +> ⊘ **All 3 checks skipped** — nothing was validated +> +>
+> Show all 3 checks +> +> ```text +> Commit message +> ⊘ PR title (skipped) +> ⊘ Commit 1/1 (skipped) +> Branch +> ⊘ Branch (skipped) +> ``` +> +>
+> +> _commit-check <version> · [Rules reference](https://commit-check.com/rules/)_ + +A skipped scope carries no checked value, because nothing was examined. When +only some scopes skip, the verdict counts them separately — +`✅ **3 of 5 checks passed**, 2 skipped` — so the headline never claims a pass +that did not happen. Failures still take precedence over skips. + +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. + ## 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 a915a0b..649c3bf 100755 --- a/main.py +++ b/main.py @@ -101,10 +101,24 @@ class ScopeResult: @property def status(self) -> str: - """Overall status: ``pass`` when every check passed.""" + """Overall status: ``pass``, ``fail``, 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 + is reported separately from ``pass`` because a skipped scope + 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. + """ if self.raw_text and not self.checks: return "fail" - return "fail" if any(c["status"] == "fail" for c in self.checks) else "pass" + if any(c["status"] == "fail" for c in self.checks): + return "fail" + if self.checks and all(c["status"] == "skip" for c in self.checks): + return "skip" + return "pass" @property def failures(self) -> list[dict[str, str]]: @@ -112,6 +126,32 @@ def failures(self) -> list[dict[str, str]]: return [c for c in self.checks if c["status"] == "fail"] +def overall_status(results: list[ScopeResult]) -> str: + """Reduce scope statuses to one of ``pass``/``fail``/``skip``. + + One function, used by every completion path, because the alternative + is what this replaced: four separate ``all(... == "pass")`` tests, each + 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. + """ + if any(scope.status == "fail" for scope in results): + return "fail" + if results and all(scope.status == "skip" for scope in results): + return "skip" + return "pass" + + +def exit_code_for(results: list[ScopeResult]) -> int: + """Only a failure is an error. + + A skipped run validated nothing, but it violated no policy either, so + it must not fail the workflow. + """ + return 1 if overall_status(results) == "fail" else 0 + + def log_env_vars(): """Logs the environment variables for debugging purposes. @@ -326,7 +366,7 @@ def run_commit_check() -> tuple[int, list[ScopeResult]]: args = [a for a in args if a != "--message"] results.extend(run_other_checks(args)) - exit_code = 1 if any(scope.status == "fail" for scope in results) else 0 + exit_code = exit_code_for(results) return exit_code, results @@ -384,6 +424,11 @@ def _render_scopes(scopes: list[ScopeResult], include_docs: bool) -> list[str]: """ lines: list[str] = [] for scope in scopes: + if scope.status == "skip": + # Deliberately not a ✔. Nothing was validated here, and a tick + # claiming otherwise is what made a bypassed policy look enforced. + lines.append(f" ⊘ {scope.label} (skipped)") + continue if scope.status == "pass": value = _scope_value(scope) lines.append(f" ✔ {scope.label}{f' ({value})' if value else ''}") @@ -467,7 +512,16 @@ def render_step_log(results: list[ScopeResult]) -> None: ) if not annotations: - print("\u2714 commit-check: all checks passed") + skipped, total = _skip_count(results), len(results) + if total and skipped == total: + print("\u2298 commit-check: all checks skipped, nothing was validated") + elif skipped: + print( + f"\u2714 commit-check: {total - skipped} of {total} checks passed, " + f"{skipped} skipped" + ) + else: + print("\u2714 commit-check: all checks passed") def _check_counts(results: list[ScopeResult]) -> tuple[int, int]: @@ -495,6 +549,15 @@ def _failure_count(results: list[ScopeResult]) -> int: return _check_counts(results)[0] +def _skip_count(results: list[ScopeResult]) -> int: + """Number of scopes that never ran. + + Reported separately from the pass count so the headline cannot claim + that checks passed when they were skipped. + """ + 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. @@ -507,7 +570,10 @@ def _markdown_table(results: list[ScopeResult]) -> str: "|---|---|---|", ] for scope in results: - if scope.status == "pass": + # 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": continue value = _scope_value(scope) value_display = f"`{value}`" if value else "\u2014" @@ -592,6 +658,25 @@ def _scope_value(scope: ScopeResult, max_len: int = 60) -> str: # # _commit-check 2.13.1 · [Rules reference](https://commit-check.com/rules/)_ # +# Skipped (every rule declined to run — e.g. the author is in ignore_authors): +# +# ⊘ **All 5 checks skipped** — nothing was validated +# +# ```text +# Commit message +# ⊘ PR title (skipped) +# ⊘ Commit 1/1 (skipped) +# Branch +# ⊘ Branch (skipped) +# Author +# ⊘ Author name (skipped) +# ⊘ Author email (skipped) +# ``` +# +# A skipped scope deliberately carries no ✔ and no checked value: nothing was +# 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". +# # Failure: # # @@ -666,15 +751,26 @@ def render_report(results: list[ScopeResult]) -> str: failure table (failures only) and the collapsible per-scope details. """ failed, total = _check_counts(results) + skipped = _skip_count(results) unit = "check" if total == 1 else "checks" lines = [COMMENT_MARKER, REPORT_TITLE, ""] - if failed == 0: - lines.append(f"✅ **All {total} {unit} passed**") - lines.append("") - else: + if failed: lines.append(f"❌ **{failed} of {total} {unit} failed**") lines.extend(["", _markdown_table(results), ""]) + 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" + ) + lines.append("") + else: + lines.append(f"✅ **All {total} {unit} passed**") + lines.append("") lines.extend([_markdown_details(results), "", _report_footer()]) return "\n".join(lines) @@ -702,7 +798,7 @@ def add_job_summary(results: list[ScopeResult]) -> int: with open(GITHUB_STEP_SUMMARY, "a", encoding="utf-8") as summary_file: summary_file.write(render_job_summary(results)) - return 0 if all(scope.status == "pass" for scope in results) else 1 + return exit_code_for(results) def set_result_output(results: list[ScopeResult]) -> None: @@ -714,7 +810,7 @@ def set_result_output(results: list[ScopeResult]) -> None: if not output_path: return payload = { - "status": "pass" if all(s.status == "pass" for s in results) else "fail", + "status": overall_status(results), "scopes": [ {"label": scope.label, "status": scope.status, "checks": scope.checks} for scope in results @@ -872,7 +968,7 @@ def add_pr_comments(results: list[ScopeResult]) -> int: if target is not None: if target.body == pr_comment_body: print(f"PR comment already up-to-date for PR #{pr_number}.") - return 0 if all(scope.status == "pass" for scope in results) else 1 + return exit_code_for(results) print(f"Updating the last comment on PR #{pr_number}.") target.edit(pr_comment_body) for comment in stale: @@ -882,7 +978,7 @@ def add_pr_comments(results: list[ScopeResult]) -> int: print(f"Creating a new comment on PR #{pr_number}.") pull_request.create_comment(body=pr_comment_body) - return 0 if all(scope.status == "pass" for scope in results) else 1 + return exit_code_for(results) except GithubException as e: if e.status == 403: # GithubException.data is whatever the response decoded to, which diff --git a/main_test.py b/main_test.py index 7aa330d..9fe0a34 100644 --- a/main_test.py +++ b/main_test.py @@ -1355,3 +1355,162 @@ def test_no_comments_yields_nothing_to_update(self): target, stale = main._find_own_comments([]) self.assertIsNone(target) self.assertEqual(stale, []) + + +def skip_scope(label: str = "PR title") -> main.ScopeResult: + """A scope whose every rule declined to run (e.g. author in ignore_authors).""" + return main.ScopeResult( + label=label, + checks=[make_check("message", status="skip", rule_id="CC001", value="")], + ) + + +class TestSkippedScopes(unittest.TestCase): + """A skipped scope must never render as a passing one. + + commit-check-action#258 is the case this guards: every rule was skipped + because the author is dependabot[bot], and the report announced + "All 5 checks passed" over five green ticks. + """ + + def test_scope_status_is_skip_not_pass(self): + self.assertEqual(skip_scope().status, "skip") + + def test_one_real_verdict_outranks_the_skips(self): + """A scope only counts as skipped when nothing in it ran.""" + mixed = main.ScopeResult( + label="PR title", + checks=[ + make_check("message", status="skip", value=""), + make_check("subject_max_length", status="pass", value="feat: x"), + ], + ) + self.assertEqual(mixed.status, "pass") + + @pin_version + def test_all_skipped_golden_output(self): + """Pin the fully skipped report: no ✔, no pass claim.""" + results = [skip_scope("PR title"), skip_scope("Commit 1/1")] + body = main.render_report(results) + self.assertEqual( + body, + f"{main.COMMENT_MARKER}\n" + f"{main.REPORT_TITLE}\n" + "\n" + "⊘ **All 2 checks skipped** — nothing was validated\n" + "\n" + "
\n" + "Show all 2 checks\n" + "\n" + "```text\n" + "Commit message\n" + " ⊘ PR title (skipped)\n" + " ⊘ Commit 1/1 (skipped)\n" + "```\n" + "\n" + "
\n" + "\n" + f"{FOOTER}", + ) + + def test_partial_skip_does_not_claim_all_passed(self): + results = [pass_scope("Branch", value="main"), skip_scope("PR title")] + body = main.render_report(results) + self.assertIn("✅ **1 of 2 checks passed**, 1 skipped", body) + self.assertNotIn("All 2 checks passed", body) + + def test_failure_still_wins_over_skips(self): + results = [fail_scope("Commit 1/1"), skip_scope("PR title")] + body = main.render_report(results) + self.assertIn("❌ **1 of 2 checks failed**", body) + + def test_skipped_scope_reports_no_checked_value(self): + """The ⊘ line carries no value, because nothing was examined.""" + body = main.render_report([skip_scope("PR title")]) + self.assertIn(" ⊘ PR title (skipped)", body) + + def test_older_engine_without_skip_is_unaffected(self): + """Back-compat: engines that only emit pass/fail render as before.""" + results = [pass_scope("Branch", value="main")] + self.assertIn("✅ **All 1 check passed**", main.render_report(results)) + + +class TestSkipCompletionSemantics(unittest.TestCase): + """A skipped run must not be treated as a failing one. + + run_commit_check has always failed only on "fail", but the completion + paths each asked `all(status == "pass")` instead. That was equivalent + only while pass and fail were the only statuses; with skip added, a + skipped-only run exited 1 and reported "fail" while rendering ⊘. + """ + + def test_skipped_only_run_is_not_a_failure(self): + self.assertEqual(main.exit_code_for([skip_scope(), skip_scope("Branch")]), 0) + + def test_partial_skip_is_not_a_failure(self): + self.assertEqual(main.exit_code_for([pass_scope(), skip_scope()]), 0) + + def test_a_real_failure_still_fails(self): + self.assertEqual(main.exit_code_for([fail_scope(), skip_scope()]), 1) + + def test_result_output_status_reports_skip(self): + self.assertEqual( + main.overall_status([skip_scope(), skip_scope("Branch")]), "skip" + ) + + def test_result_output_status_of_partial_skip_is_pass(self): + self.assertEqual(main.overall_status([pass_scope(), skip_scope()]), "pass") + + def test_set_result_output_emits_skip(self): + """The `result` action output must not call a skipped run failed.""" + with tempfile.NamedTemporaryFile("w+", delete=False) as f: + out_path = f.name + try: + with patch.dict(os.environ, {"GITHUB_OUTPUT": out_path}): + main.set_result_output([skip_scope(), skip_scope("Branch")]) + written = open(out_path, encoding="utf-8").read() + finally: + os.unlink(out_path) + payload = json.loads(written.split("result<