diff --git a/README.md b/README.md index 6e840143..0c37d438 100644 --- a/README.md +++ b/README.md @@ -401,12 +401,12 @@ print(result["status"]) # "fail" — 'docs' not in allowed types ```python { - "status": "pass" | "fail", + "status": "pass" | "fail" | "skip", "checks": [ { "rule_id": "", "check": "", - "status": "pass" | "fail", + "status": "pass" | "fail" | "skip", "value": "", "error": "", "suggest": "", @@ -417,6 +417,56 @@ print(result["status"]) # "fail" — 'docs' not in allowed types } ``` +`skip` means the rule never ran — the author matched `ignore_authors`, or +there was nothing to check. It is deliberately not `pass`: a skipped rule +validated nothing, so reporting it as a pass makes a bypassed policy +indistinguishable from an enforced one. A skipped check carries no `value`, +since nothing was examined. + +The top-level `status` is `skip` only when **every** check skipped; one real +verdict makes it `pass` or `fail` as before. Only `fail` is an error, and the +CLI exit code follows that — a fully skipped run still exits `0`, so code +branching on `status == "fail"` is unaffected. + +```bash +echo "chore(deps): bump commit-check" | CCHK_IGNORE_AUTHORS="dependabot[bot]" commit-check -m --format json +``` + +```json +{ + "status": "skip", + "checks": [ + { + "rule_id": "CC001", + "check": "message", + "status": "skip", + "value": "", + "error": "", + "suggest": "", + "docs_url": "https://commit-check.com/rules/#cc001" + }, + { + "rule_id": "CC004", + "check": "subject_max_length", + "status": "skip", + "value": "", + "error": "", + "suggest": "", + "docs_url": "https://commit-check.com/rules/#cc004" + }, + { + "rule_id": "CC005", + "check": "subject_min_length", + "status": "skip", + "value": "", + "error": "", + "suggest": "", + "docs_url": "https://commit-check.com/rules/#cc005" + } + ] +} +``` + Available API functions: - `validate_message(message, *, config=None)` — validate a commit message string diff --git a/commit_check/api.py b/commit_check/api.py index 0458e68b..df12ce49 100644 --- a/commit_check/api.py +++ b/commit_check/api.py @@ -19,11 +19,11 @@ Return-value schema (all functions):: { - "status": "pass" | "fail", + "status": "pass" | "fail" | "skip", "checks": [ { "check": "", - "status": "pass" | "fail", + "status": "pass" | "fail" | "skip", "value": "", "error": "", "suggest": "", @@ -31,6 +31,14 @@ ... ] } + +``"skip"`` means the rule never ran — the author is on an ``ignore_authors`` +list, or there was nothing to check. It is deliberately not ``"pass"``: a +skipped rule validated nothing, and collapsing the two makes a bypassed +policy indistinguishable from an enforced one. The top-level ``status`` is +``"skip"`` only when *every* check skipped; a run with any real verdict +reports ``"pass"`` or ``"fail"`` as before. Only ``"fail"`` is an error, so +code branching on ``status == "fail"`` keeps working unchanged. """ from __future__ import annotations @@ -43,6 +51,7 @@ CheckOutcome, ValidationContext, ValidationEngine, + overall_status, ) from commit_check.rule_builder import RuleBuilder @@ -55,9 +64,8 @@ def _build_result(outcomes: list[CheckOutcome]) -> dict[str, Any]: """Convert a list of :class:`~commit_check.engine.CheckOutcome` into the public return-value dict.""" - overall = "fail" if any(o.status == "fail" for o in outcomes) else "pass" return { - "status": overall, + "status": overall_status(o.status for o in outcomes), "checks": [o.to_dict() for o in outcomes], } @@ -245,7 +253,9 @@ def validate_author( cfg, ) all_checks = name_result["checks"] + email_result["checks"] - overall = "fail" if any(c["status"] == "fail" for c in all_checks) else "pass" + # Shared reducer, not a local "fail or else pass": a combined call + # in which every nested check skipped is still a skip. + overall = overall_status(c["status"] for c in all_checks) return {"status": overall, "checks": all_checks} stdin = None @@ -303,5 +313,5 @@ def validate_all( author_result = validate_author(author_name, author_email, config=config) all_checks.extend(author_result["checks"]) - overall = "fail" if any(c["status"] == "fail" for c in all_checks) else "pass" + overall = overall_status(c["status"] for c in all_checks) return {"status": overall, "checks": all_checks} diff --git a/commit_check/engine.py b/commit_check/engine.py index 1ea2c5cd..c67cd5dc 100644 --- a/commit_check/engine.py +++ b/commit_check/engine.py @@ -2,6 +2,7 @@ from __future__ import annotations from abc import ABC, abstractmethod +from collections.abc import Iterable from dataclasses import dataclass from enum import IntEnum from dataclasses import field @@ -27,10 +28,22 @@ class ValidationResult(IntEnum): - """Validation result codes.""" + """Validation result codes. + + ``SKIP`` means the validator declined to run — the author is on an + ignore list, or there was nothing to check — as opposed to ``PASS``, + which means the rule ran and found nothing to object to. Reporting a + skip as a pass makes a bypassed policy indistinguishable from an + enforced one, so the two are kept apart. + + Only ``FAIL`` is an error. ``validate_all`` returns ``PASS``/``FAIL`` + explicitly rather than propagating this value, so the new member never + reaches an exit code. + """ PASS = 0 FAIL = 1 + SKIP = 2 @dataclass(frozen=True) @@ -55,7 +68,11 @@ class CheckOutcome: """ check: str - status: str # "pass" or "fail" + # "pass" (the rule ran and was satisfied), "fail" (the rule ran and was + # not), or "skip" (the rule never ran — ignored author, or nothing to + # check). A skip is not a pass: it means the policy was bypassed, and + # collapsing the two lets a run that validated nothing report success. + status: str # The concrete value that was checked (subject, branch, author, ...), # populated on both pass and fail so consumers can report what was # validated even when the check succeeded. @@ -78,6 +95,31 @@ def to_dict(self) -> dict[str, str]: } +def overall_status(statuses: Iterable[str]) -> str: + """Reduce per-check statuses to one of ``"pass"``/``"fail"``/``"skip"``. + + Takes plain status strings rather than a specific type so that every + caller can share it: the CLI's ``--format json`` and the API's + :class:`CheckOutcome` objects, and the API's combined paths + (``validate_author`` with both inputs, ``validate_all``) which merge + already-serialised check dicts. + + That breadth is the point. This rule had been copied into four places, + and each copy defaulted to ``"pass"`` for anything that was not a + failure — which is how a fully skipped run kept reporting success even + after the skip status existed. + + ``"skip"`` requires that *every* check skipped: a single real verdict + means something was actually validated. Only ``"fail"`` is an error. + """ + seen = list(statuses) + if any(s == "fail" for s in seen): + return "fail" + if seen and all(s == "skip" for s in seen): + return "skip" + return "pass" + + class BaseValidator(ABC): """Abstract base validator.""" @@ -272,7 +314,7 @@ class CommitMessageValidator(BaseValidator): def validate(self, context: ValidationContext) -> ValidationResult: if self._should_skip_commit_validation(context): - return ValidationResult.PASS + return ValidationResult.SKIP message = self._get_commit_message(context) if not message: @@ -294,7 +336,7 @@ class SubjectValidator(BaseValidator): def validate(self, context: ValidationContext) -> ValidationResult: if self._should_skip_commit_validation(context): - return ValidationResult.PASS + return ValidationResult.SKIP subject = self._get_subject(context) if not subject: @@ -401,7 +443,7 @@ class AuthorValidator(BaseValidator): def validate(self, context: ValidationContext) -> ValidationResult: # Use commit skip logic for ignore_authors if self._should_skip_commit_validation(context): - return ValidationResult.PASS + return ValidationResult.SKIP author_value = self._get_author_value(context) if not author_value: @@ -455,7 +497,8 @@ def _validate_author(self, author_value: str) -> ValidationResult: return ValidationResult.FAIL if self.rule.ignored and author_value in self.rule.ignored: - return ValidationResult.PASS # Ignored authors pass silently + # An ignored author is a deliberate bypass, not a verdict. + return ValidationResult.SKIP return ValidationResult.PASS @@ -465,7 +508,7 @@ class BranchValidator(BaseValidator): def validate(self, context: ValidationContext) -> ValidationResult: if self._should_skip_branch_validation(context): - return ValidationResult.PASS + return ValidationResult.SKIP branch_name = ( context.stdin_text.strip() if context.stdin_text is not None @@ -490,7 +533,7 @@ class MergeBaseValidator(BaseValidator): def validate(self, context: ValidationContext) -> ValidationResult: if self._should_skip_branch_validation(context): - return ValidationResult.PASS + return ValidationResult.SKIP current_branch = get_branch_name() target_pattern = self.rule.regex @@ -588,7 +631,7 @@ class SignoffValidator(BaseValidator): def validate(self, context: ValidationContext) -> ValidationResult: if self._should_skip_commit_validation(context): - return ValidationResult.PASS + return ValidationResult.SKIP message = self._get_commit_message(context) if not message: @@ -610,7 +653,7 @@ class BodyValidator(BaseValidator): def validate(self, context: ValidationContext) -> ValidationResult: if self._should_skip_commit_validation(context): - return ValidationResult.PASS + return ValidationResult.SKIP message = self._get_commit_message(context) if not message: @@ -767,9 +810,9 @@ def validate(self, context: ValidationContext) -> ValidationResult: self._checked_value = self._resolve_current_author(context) if self._should_skip_commit_validation(context): self._checked_value = "" - return ValidationResult.PASS + return ValidationResult.SKIP elif self._should_skip_commit_validation(context): - return ValidationResult.PASS + return ValidationResult.SKIP message = self._get_commit_message(context) # allow_empty_commits is the rule that exists to judge an empty @@ -851,7 +894,7 @@ class AiAttributionValidator(BaseValidator): def validate(self, context: ValidationContext) -> ValidationResult: if self._should_skip_commit_validation(context): - return ValidationResult.PASS + return ValidationResult.SKIP message = self._get_commit_body(context) if not message: @@ -991,11 +1034,14 @@ def validate_all_detailed(self, context: ValidationContext) -> list[CheckOutcome ) ) else: + # A skipped rule never ran, so it has no value to report and + # must not be reported as a pass — see ValidationResult.SKIP. + skipped = result == ValidationResult.SKIP outcomes.append( CheckOutcome( check=rule.check, - status="pass", - value=validator._checked_value or "", + status="skip" if skipped else "pass", + value="" if skipped else (validator._checked_value or ""), rule_id=rule.rule_id or "", docs_url=rule.docs_url or "", ) diff --git a/commit_check/main.py b/commit_check/main.py index af7d2cd6..a7a044c3 100644 --- a/commit_check/main.py +++ b/commit_check/main.py @@ -13,6 +13,7 @@ ValidationContext, ValidationResult, CheckOutcome, + overall_status, ) from . import __version__ @@ -446,7 +447,7 @@ def _get_requested_checks(args: argparse.Namespace) -> list[str]: def _run_json_output(engine: ValidationEngine, context: ValidationContext) -> int: """Run validation and print JSON output.""" outcomes: list[CheckOutcome] = engine.validate_all_detailed(context) - overall = "fail" if any(o.status == "fail" for o in outcomes) else "pass" + overall = overall_status(o.status for o in outcomes) print( json.dumps( { @@ -456,7 +457,9 @@ def _run_json_output(engine: ValidationEngine, context: ValidationContext) -> in indent=2, ) ) - return 0 if overall == "pass" else 1 + # Only a failure is an error. A skipped run validated nothing, but that + # is not a policy violation, so it must not turn into a non-zero exit. + return 1 if overall == "fail" else 0 def main() -> int: diff --git a/tests/api_test.py b/tests/api_test.py index 9d8a3745..5f973e43 100644 --- a/tests/api_test.py +++ b/tests/api_test.py @@ -334,3 +334,142 @@ def test_result_has_expected_structure(self): assert "value" in c assert "error" in c assert "suggest" in c + + +class TestSkippedStatus: + """A skipped rule must not be reported as a passing one. + + An ignored author bypasses the policy entirely. Reporting that as + ``pass`` made a run that validated nothing indistinguishable from one + that validated everything, so consumers (the GitHub Action's summary, + an agent reading the JSON) announced success for unchecked commits. + """ + + IGNORED = {"commit": {"ignore_authors": ["dependabot[bot]"]}} + + @pytest.mark.benchmark + def test_ignored_author_reports_skip_not_pass(self): + """Every rule reports 'skip', and the overall status follows.""" + with ( + patch( + "commit_check.engine.get_git_config_value", + return_value="dependabot[bot]", + ), + patch( + "commit_check.engine.get_commit_info", return_value="dependabot[bot]" + ), + ): + result = validate_message( + "chore(deps): bump commit-check", config=self.IGNORED + ) + + assert result["status"] == "skip" + assert result["checks"], "expected the rules to be reported, not dropped" + assert {c["status"] for c in result["checks"]} == {"skip"} + + @pytest.mark.benchmark + def test_skipped_checks_carry_no_value(self): + """A skipped rule checked nothing, so it reports no checked value.""" + with ( + patch( + "commit_check.engine.get_git_config_value", + return_value="dependabot[bot]", + ), + patch( + "commit_check.engine.get_commit_info", return_value="dependabot[bot]" + ), + ): + result = validate_message( + "chore(deps): bump commit-check", config=self.IGNORED + ) + + assert [c["value"] for c in result["checks"]] == [""] * len(result["checks"]) + + @pytest.mark.benchmark + def test_same_message_from_a_listed_author_still_passes(self): + """The control: only the author differs, and the verdict is real. + + Without this the skip test would pass even if the rules had simply + stopped running for everyone. + """ + with ( + patch( + "commit_check.engine.get_git_config_value", return_value="Ada Lovelace" + ), + patch("commit_check.engine.get_commit_info", return_value="Ada Lovelace"), + ): + result = validate_message( + "chore(deps): bump commit-check", config=self.IGNORED + ) + + assert result["status"] == "pass" + assert {c["status"] for c in result["checks"]} == {"pass"} + assert any(c["value"] for c in result["checks"]), ( + "a real pass reports what it checked" + ) + + @pytest.mark.benchmark + def test_a_failure_still_outranks_a_skip(self): + """Overall status is 'skip' only when nothing ran at all.""" + with ( + patch( + "commit_check.engine.get_git_config_value", return_value="Ada Lovelace" + ), + patch("commit_check.engine.get_commit_info", return_value="Ada Lovelace"), + ): + result = validate_message("wip nonsense", config=self.IGNORED) + + assert result["status"] == "fail" + + @pytest.mark.benchmark + def test_combined_author_call_preserves_skip(self): + """validate_author(name=..., email=...) merges two runs of checks. + + That merge had its own copy of the reduce-to-overall rule which + defaulted to "pass", so a fully skipped combined call reported a + pass even after the skip status existed. + """ + cfg = {"commit": {"ignore_authors": ["dependabot[bot]"]}} + with ( + patch( + "commit_check.engine.get_git_config_value", + return_value="dependabot[bot]", + ), + patch( + "commit_check.engine.get_commit_info", return_value="dependabot[bot]" + ), + ): + result = validate_author( + name="whoever", email="who@example.com", config=cfg + ) + + assert result["status"] == "skip" + assert {c["status"] for c in result["checks"]} == {"skip"} + + @pytest.mark.benchmark + def test_validate_all_preserves_skip(self): + """validate_all() merges every group and had the same private copy.""" + cfg = { + "commit": {"ignore_authors": ["dependabot[bot]"]}, + "branch": {"ignore_authors": ["dependabot[bot]"]}, + } + with ( + patch( + "commit_check.engine.get_git_config_value", + return_value="dependabot[bot]", + ), + patch( + "commit_check.engine.get_commit_info", return_value="dependabot[bot]" + ), + patch("commit_check.engine.get_branch_name", return_value="main"), + ): + result = validate_all( + message="chore(deps): bump commit-check", + branch="dependabot/pip/commit-check-2.13.3", + author_name="whoever", + author_email="who@example.com", + config=cfg, + ) + + assert result["status"] == "skip" + assert {c["status"] for c in result["checks"]} == {"skip"} diff --git a/tests/engine_test.py b/tests/engine_test.py index 8070246c..3f3ec867 100644 --- a/tests/engine_test.py +++ b/tests/engine_test.py @@ -338,7 +338,7 @@ def test_branch_validator_ignored_author( config = {"branch": {"ignore_authors": ["ignored"]}} context = ValidationContext(config=config) result = validator.validate(context) - assert result == ValidationResult.PASS + assert result == ValidationResult.SKIP @pytest.mark.benchmark def test_validate_with_stdin_text(self): @@ -512,7 +512,7 @@ def test_branch_ignored_author_uses_commit_author_when_no_stdin(self): ): result = validator.validate(context) # Skipped — the commit's author (dependabot[bot]) is in ignore_authors - assert result == ValidationResult.PASS + assert result == ValidationResult.SKIP class TestAuthorValidator: @@ -578,7 +578,7 @@ def test_author_validator_ignored_author( config = {"commit": {"ignore_authors": ["ignored"]}} context = ValidationContext(config=config) result = validator.validate(context) - assert result == ValidationResult.PASS + assert result == ValidationResult.SKIP @pytest.mark.benchmark def test_validate_author_with_allowed_list(self): @@ -615,7 +615,7 @@ def test_validate_author_in_ignored_list(self): with patch.object(validator, "_get_author_value", return_value="Bot User"): context = ValidationContext() result = validator.validate(context) - assert result == ValidationResult.PASS + assert result == ValidationResult.SKIP @pytest.mark.benchmark def test_get_author_value_with_email_format(self): @@ -751,7 +751,7 @@ def test_ignore_authors_skipped_keeps_value_empty(self): with patch("commit_check.engine.has_commits", return_value=True): result = validator.validate(context) - assert result == ValidationResult.PASS + assert result == ValidationResult.SKIP assert validator._checked_value == "" @pytest.mark.benchmark @@ -978,7 +978,7 @@ def test_default_signoff_skips_ignored_author( context = ValidationContext(stdin_text="chore: bump dep", config=config) result = validator.validate(context) - assert result == ValidationResult.PASS + assert result == ValidationResult.SKIP @pytest.mark.benchmark def test_signoff_validator_missing_signoff(self): @@ -1220,7 +1220,7 @@ def test_validate_with_merge_base_skip_conditions(self): with patch("commit_check.engine.has_commits", return_value=False): result = validator.validate(context) - assert result == ValidationResult.PASS # Skipped + assert result == ValidationResult.SKIP # the rule never ran # ------------------------------------------------------------------ # # _find_target_branch —— unit tests for the new impl @@ -1750,7 +1750,7 @@ def test_co_author_in_ignore_list_skips_validation(self): with patch("commit_check.engine.get_commit_info", return_value="other-author"): result = validator.validate(context) - assert result == ValidationResult.PASS + assert result == ValidationResult.SKIP @pytest.mark.benchmark def test_co_author_not_in_ignore_list_does_not_skip(self): @@ -1799,7 +1799,7 @@ def test_co_author_in_ignore_list_from_commit_file(self): "commit_check.engine.get_commit_info", return_value="main-author" ): result = validator.validate(context) - assert result == ValidationResult.PASS + assert result == ValidationResult.SKIP finally: os.unlink(commit_file) @@ -1872,7 +1872,7 @@ def test_author_in_ignore_list_uses_commit_author_when_no_stdin(self): ): result = validator.validate(context) # Skipped — the commit's author (dependabot[bot]) is in ignore_authors - assert result == ValidationResult.PASS + assert result == ValidationResult.SKIP @pytest.mark.benchmark def test_author_in_ignore_list_falls_back_to_git_config_when_commit_info_empty( @@ -1904,7 +1904,7 @@ def test_author_in_ignore_list_falls_back_to_git_config_when_commit_info_empty( ): result = validator.validate(context) # Skipped — fallback author (Developer Bot) is in ignore_authors - assert result == ValidationResult.PASS + assert result == ValidationResult.SKIP class TestGetGitConfigValue: @@ -2455,7 +2455,7 @@ def test_skip_when_author_ignored(self): patch("commit_check.engine.get_git_config_value", return_value=""), ): result = validator.validate(context) - assert result == ValidationResult.PASS # Skipped due to ignored author + assert result == ValidationResult.SKIP # the rule never ran @pytest.mark.benchmark def test_empty_message_passes(self): @@ -2489,3 +2489,74 @@ def test_empty_message_is_not_read_from_git(self): ) assert body == "" mock_commit_info.assert_not_called() + + +class TestSkipCoverage: + """The remaining skip guards, pinned so they cannot regress to PASS. + + Every validator routes its bypass through the same two helpers, but each + call site returns independently, so each needs its own guard. + """ + + IGNORED = {"commit": {"ignore_authors": ["dependabot[bot]"]}} + + def _as_ignored_author(self): + return ( + patch( + "commit_check.engine.get_git_config_value", + return_value="dependabot[bot]", + ), + patch( + "commit_check.engine.get_commit_info", return_value="dependabot[bot]" + ), + ) + + @pytest.mark.benchmark + def test_body_validator_skips_ignored_author(self): + """BodyValidator declines to run rather than reporting a pass.""" + validator = BodyValidator(ValidationRule(check="require_body")) + context = ValidationContext(stdin_text="feat: add feature", config=self.IGNORED) + cfg, info = self._as_ignored_author() + with cfg, info: + assert validator.validate(context) == ValidationResult.SKIP + + @pytest.mark.benchmark + def test_body_validator_still_runs_for_other_authors(self): + """Control: only the author differs, and the rule reaches a verdict.""" + validator = BodyValidator(ValidationRule(check="require_body")) + context = ValidationContext(stdin_text="feat: add feature", config=self.IGNORED) + with ( + patch( + "commit_check.engine.get_git_config_value", return_value="Ada Lovelace" + ), + patch("commit_check.engine.get_commit_info", return_value="Ada Lovelace"), + ): + assert validator.validate(context) != ValidationResult.SKIP + + @pytest.mark.benchmark + def test_commit_type_rule_skips_ignored_author(self): + """CommitTypeValidator's non-ignore_authors branch skips too. + + ``ignore_authors`` itself takes a separate path in this validator, so + a rule such as ``allow_wip_commits`` exercises the other one. + """ + rule = ValidationRule(check="allow_wip_commits", value=False) + validator = CommitTypeValidator(rule) + context = ValidationContext(stdin_text="wip: not done", config=self.IGNORED) + cfg, info = self._as_ignored_author() + with cfg, info: + assert validator.validate(context) == ValidationResult.SKIP + + @pytest.mark.benchmark + def test_commit_type_rule_still_fails_for_other_authors(self): + """Control: the same WIP message from a human is still rejected.""" + rule = ValidationRule(check="allow_wip_commits", value=False) + validator = CommitTypeValidator(rule) + context = ValidationContext(stdin_text="wip: not done", config=self.IGNORED) + with ( + patch( + "commit_check.engine.get_git_config_value", return_value="Ada Lovelace" + ), + patch("commit_check.engine.get_commit_info", return_value="Ada Lovelace"), + ): + assert validator.validate(context) == ValidationResult.FAIL