Skip to content

Commit dcea4d3

Browse files
shenxianpengclaude
shenxianpeng
and
claude
committed
feat: report a skipped check as skipped, not as passed
A rule that never ran was reported as a pass. commit-check-action#258 is the visible cost: every check on it rendered as a green tick and the summary announced "All 5 checks passed", when in fact nothing had been validated -- the author is dependabot[bot], which the org config lists in ignore_authors. A bypassed policy was indistinguishable from an enforced one, in the JSON, in the Python API, and in anything rendering them. Measured on that exact case before the change: every rule came back "status": "pass" with "value": "", the empty value being the only trace that a skip had happened, and an incidental one at that. Adds ValidationResult.SKIP and returns it from the guards that already decide this -- _should_skip_commit_validation, _should_skip_branch_ validation, and the ignored-author branch of _validate_author. Those helpers are named for skipping; they were simply reporting it as PASS. validate_all_detailed maps SKIP to "skip" and forces the value empty, since a rule that did not run examined nothing. Overall status is "skip" only when every check skipped; one real verdict still yields "pass" or "fail". Only "fail" is an error, so the exit code is unchanged for existing callers and code branching on status == "fail" keeps working. The overall-status rule was duplicated between the CLI's --format json and the Python API, which is how the CLI kept printing "pass" for a fully skipped run after the API had been fixed. It now lives once, in engine.overall_status(), used by both. That also fixes a latent bug in the CLI's exit code: `0 if overall == "pass" else 1` would have turned a skipped run into a failure. Verified end to end in a repository shaped like #258 -- same repo, same config, only the author differing: dependabot[bot] -> overall "skip", every check "skip", exit 0 a human -> overall "pass", values reported, exit 0 a human, bad msg -> exit 1 The twelve existing tests that asserted PASS on these paths are all named for skipping (ignored_author, skips_validation, skip_conditions); they now assert SKIP. Four new API tests pin the behaviour, including a control that only the author differs so the skip test cannot pass by the rules having quietly stopped running for everyone. Reverting the skip reporting turns the first of them red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn
1 parent 90c5abe commit dcea4d3

6 files changed

Lines changed: 219 additions & 35 deletions

File tree

‎README.md‎

Lines changed: 52 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -401,12 +401,12 @@ print(result["status"]) # "fail" — 'docs' not in allowed types
401401

402402
```python
403403
{
404-
"status": "pass" | "fail",
404+
"status": "pass" | "fail" | "skip",
405405
"checks": [
406406
{
407407
"rule_id": "<rule identifier, e.g. CC001>",
408408
"check": "<rule name>",
409-
"status": "pass" | "fail",
409+
"status": "pass" | "fail" | "skip",
410410
"value": "<actual value that was checked>",
411411
"error": "<human-readable error description>",
412412
"suggest": "<how to fix>",
@@ -417,6 +417,56 @@ print(result["status"]) # "fail" — 'docs' not in allowed types
417417
}
418418
```
419419

420+
`skip` means the rule never ran — the author matched `ignore_authors`, or
421+
there was nothing to check. It is deliberately not `pass`: a skipped rule
422+
validated nothing, so reporting it as a pass makes a bypassed policy
423+
indistinguishable from an enforced one. A skipped check carries no `value`,
424+
since nothing was examined.
425+
426+
The top-level `status` is `skip` only when **every** check skipped; one real
427+
verdict makes it `pass` or `fail` as before. Only `fail` is an error, and the
428+
CLI exit code follows that — a fully skipped run still exits `0`, so code
429+
branching on `status == "fail"` is unaffected.
430+
431+
```bash
432+
echo "chore(deps): bump commit-check" | CCHK_IGNORE_AUTHORS="dependabot[bot]" commit-check -m --format json
433+
```
434+
435+
```json
436+
{
437+
"status": "skip",
438+
"checks": [
439+
{
440+
"rule_id": "CC001",
441+
"check": "message",
442+
"status": "skip",
443+
"value": "",
444+
"error": "",
445+
"suggest": "",
446+
"docs_url": "https://commit-check.com/rules/#cc001"
447+
},
448+
{
449+
"rule_id": "CC004",
450+
"check": "subject_max_length",
451+
"status": "skip",
452+
"value": "",
453+
"error": "",
454+
"suggest": "",
455+
"docs_url": "https://commit-check.com/rules/#cc004"
456+
},
457+
{
458+
"rule_id": "CC005",
459+
"check": "subject_min_length",
460+
"status": "skip",
461+
"value": "",
462+
"error": "",
463+
"suggest": "",
464+
"docs_url": "https://commit-check.com/rules/#cc005"
465+
}
466+
]
467+
}
468+
```
469+
420470
Available API functions:
421471

422472
- `validate_message(message, *, config=None)` — validate a commit message string

‎commit_check/api.py‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,18 +19,26 @@
1919
Return-value schema (all functions)::
2020
2121
{
22-
"status": "pass" | "fail",
22+
"status": "pass" | "fail" | "skip",
2323
"checks": [
2424
{
2525
"check": "<rule name>",
26-
"status": "pass" | "fail",
26+
"status": "pass" | "fail" | "skip",
2727
"value": "<actual value that was checked>",
2828
"error": "<error description>",
2929
"suggest": "<how to fix>",
3030
},
3131
...
3232
]
3333
}
34+
35+
``"skip"`` means the rule never ran — the author is on an ``ignore_authors``
36+
list, or there was nothing to check. It is deliberately not ``"pass"``: a
37+
skipped rule validated nothing, and collapsing the two makes a bypassed
38+
policy indistinguishable from an enforced one. The top-level ``status`` is
39+
``"skip"`` only when *every* check skipped; a run with any real verdict
40+
reports ``"pass"`` or ``"fail"`` as before. Only ``"fail"`` is an error, so
41+
code branching on ``status == "fail"`` keeps working unchanged.
3442
"""
3543

3644
from __future__ import annotations
@@ -43,6 +51,7 @@
4351
CheckOutcome,
4452
ValidationContext,
4553
ValidationEngine,
54+
overall_status,
4655
)
4756
from commit_check.rule_builder import RuleBuilder
4857

@@ -55,9 +64,8 @@
5564
def _build_result(outcomes: list[CheckOutcome]) -> dict[str, Any]:
5665
"""Convert a list of :class:`~commit_check.engine.CheckOutcome` into the
5766
public return-value dict."""
58-
overall = "fail" if any(o.status == "fail" for o in outcomes) else "pass"
5967
return {
60-
"status": overall,
68+
"status": overall_status(outcomes),
6169
"checks": [o.to_dict() for o in outcomes],
6270
}
6371

‎commit_check/engine.py‎

Lines changed: 52 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,22 @@
2727

2828

2929
class ValidationResult(IntEnum):
30-
"""Validation result codes."""
30+
"""Validation result codes.
31+
32+
``SKIP`` means the validator declined to run — the author is on an
33+
ignore list, or there was nothing to check — as opposed to ``PASS``,
34+
which means the rule ran and found nothing to object to. Reporting a
35+
skip as a pass makes a bypassed policy indistinguishable from an
36+
enforced one, so the two are kept apart.
37+
38+
Only ``FAIL`` is an error. ``validate_all`` returns ``PASS``/``FAIL``
39+
explicitly rather than propagating this value, so the new member never
40+
reaches an exit code.
41+
"""
3142

3243
PASS = 0
3344
FAIL = 1
45+
SKIP = 2
3446

3547

3648
@dataclass(frozen=True)
@@ -55,7 +67,11 @@ class CheckOutcome:
5567
"""
5668

5769
check: str
58-
status: str # "pass" or "fail"
70+
# "pass" (the rule ran and was satisfied), "fail" (the rule ran and was
71+
# not), or "skip" (the rule never ran — ignored author, or nothing to
72+
# check). A skip is not a pass: it means the policy was bypassed, and
73+
# collapsing the two lets a run that validated nothing report success.
74+
status: str
5975
# The concrete value that was checked (subject, branch, author, ...),
6076
# populated on both pass and fail so consumers can report what was
6177
# validated even when the check succeeded.
@@ -78,6 +94,23 @@ def to_dict(self) -> dict[str, str]:
7894
}
7995

8096

97+
def overall_status(outcomes: list[CheckOutcome]) -> str:
98+
"""Reduce per-check outcomes to one of ``"pass"``/``"fail"``/``"skip"``.
99+
100+
Lives here, and is used by both the CLI's ``--format json`` and the
101+
Python API, because two copies of this rule is how the CLI came to
102+
report ``"pass"`` for a run in which every rule had been skipped.
103+
104+
``"skip"`` requires that *every* check skipped: a single real verdict
105+
means something was actually validated. Only ``"fail"`` is an error.
106+
"""
107+
if any(o.status == "fail" for o in outcomes):
108+
return "fail"
109+
if outcomes and all(o.status == "skip" for o in outcomes):
110+
return "skip"
111+
return "pass"
112+
113+
81114
class BaseValidator(ABC):
82115
"""Abstract base validator."""
83116

@@ -272,7 +305,7 @@ class CommitMessageValidator(BaseValidator):
272305

273306
def validate(self, context: ValidationContext) -> ValidationResult:
274307
if self._should_skip_commit_validation(context):
275-
return ValidationResult.PASS
308+
return ValidationResult.SKIP
276309

277310
message = self._get_commit_message(context)
278311
if not message:
@@ -294,7 +327,7 @@ class SubjectValidator(BaseValidator):
294327

295328
def validate(self, context: ValidationContext) -> ValidationResult:
296329
if self._should_skip_commit_validation(context):
297-
return ValidationResult.PASS
330+
return ValidationResult.SKIP
298331

299332
subject = self._get_subject(context)
300333
if not subject:
@@ -401,7 +434,7 @@ class AuthorValidator(BaseValidator):
401434
def validate(self, context: ValidationContext) -> ValidationResult:
402435
# Use commit skip logic for ignore_authors
403436
if self._should_skip_commit_validation(context):
404-
return ValidationResult.PASS
437+
return ValidationResult.SKIP
405438

406439
author_value = self._get_author_value(context)
407440
if not author_value:
@@ -455,7 +488,8 @@ def _validate_author(self, author_value: str) -> ValidationResult:
455488
return ValidationResult.FAIL
456489

457490
if self.rule.ignored and author_value in self.rule.ignored:
458-
return ValidationResult.PASS # Ignored authors pass silently
491+
# An ignored author is a deliberate bypass, not a verdict.
492+
return ValidationResult.SKIP
459493

460494
return ValidationResult.PASS
461495

@@ -465,7 +499,7 @@ class BranchValidator(BaseValidator):
465499

466500
def validate(self, context: ValidationContext) -> ValidationResult:
467501
if self._should_skip_branch_validation(context):
468-
return ValidationResult.PASS
502+
return ValidationResult.SKIP
469503
branch_name = (
470504
context.stdin_text.strip()
471505
if context.stdin_text is not None
@@ -490,7 +524,7 @@ class MergeBaseValidator(BaseValidator):
490524

491525
def validate(self, context: ValidationContext) -> ValidationResult:
492526
if self._should_skip_branch_validation(context):
493-
return ValidationResult.PASS
527+
return ValidationResult.SKIP
494528

495529
current_branch = get_branch_name()
496530
target_pattern = self.rule.regex
@@ -588,7 +622,7 @@ class SignoffValidator(BaseValidator):
588622

589623
def validate(self, context: ValidationContext) -> ValidationResult:
590624
if self._should_skip_commit_validation(context):
591-
return ValidationResult.PASS
625+
return ValidationResult.SKIP
592626

593627
message = self._get_commit_message(context)
594628
if not message:
@@ -610,7 +644,7 @@ class BodyValidator(BaseValidator):
610644

611645
def validate(self, context: ValidationContext) -> ValidationResult:
612646
if self._should_skip_commit_validation(context):
613-
return ValidationResult.PASS
647+
return ValidationResult.SKIP
614648

615649
message = self._get_commit_message(context)
616650
if not message:
@@ -767,9 +801,9 @@ def validate(self, context: ValidationContext) -> ValidationResult:
767801
self._checked_value = self._resolve_current_author(context)
768802
if self._should_skip_commit_validation(context):
769803
self._checked_value = ""
770-
return ValidationResult.PASS
804+
return ValidationResult.SKIP
771805
elif self._should_skip_commit_validation(context):
772-
return ValidationResult.PASS
806+
return ValidationResult.SKIP
773807

774808
message = self._get_commit_message(context)
775809
# allow_empty_commits is the rule that exists to judge an empty
@@ -851,7 +885,7 @@ class AiAttributionValidator(BaseValidator):
851885

852886
def validate(self, context: ValidationContext) -> ValidationResult:
853887
if self._should_skip_commit_validation(context):
854-
return ValidationResult.PASS
888+
return ValidationResult.SKIP
855889

856890
message = self._get_commit_body(context)
857891
if not message:
@@ -991,11 +1025,14 @@ def validate_all_detailed(self, context: ValidationContext) -> list[CheckOutcome
9911025
)
9921026
)
9931027
else:
1028+
# A skipped rule never ran, so it has no value to report and
1029+
# must not be reported as a pass — see ValidationResult.SKIP.
1030+
skipped = result == ValidationResult.SKIP
9941031
outcomes.append(
9951032
CheckOutcome(
9961033
check=rule.check,
997-
status="pass",
998-
value=validator._checked_value or "",
1034+
status="skip" if skipped else "pass",
1035+
value="" if skipped else (validator._checked_value or ""),
9991036
rule_id=rule.rule_id or "",
10001037
docs_url=rule.docs_url or "",
10011038
)

‎commit_check/main.py‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
ValidationContext,
1414
ValidationResult,
1515
CheckOutcome,
16+
overall_status,
1617
)
1718
from . import __version__
1819

@@ -446,7 +447,7 @@ def _get_requested_checks(args: argparse.Namespace) -> list[str]:
446447
def _run_json_output(engine: ValidationEngine, context: ValidationContext) -> int:
447448
"""Run validation and print JSON output."""
448449
outcomes: list[CheckOutcome] = engine.validate_all_detailed(context)
449-
overall = "fail" if any(o.status == "fail" for o in outcomes) else "pass"
450+
overall = overall_status(outcomes)
450451
print(
451452
json.dumps(
452453
{
@@ -456,7 +457,9 @@ def _run_json_output(engine: ValidationEngine, context: ValidationContext) -> in
456457
indent=2,
457458
)
458459
)
459-
return 0 if overall == "pass" else 1
460+
# Only a failure is an error. A skipped run validated nothing, but that
461+
# is not a policy violation, so it must not turn into a non-zero exit.
462+
return 1 if overall == "fail" else 0
460463

461464

462465
def main() -> int:

0 commit comments

Comments
 (0)