diff --git a/README.md b/README.md index f1ec9590..0c37d438 100644 --- a/README.md +++ b/README.md @@ -256,8 +256,33 @@ echo "feat: add streaming support" | commit-check -m --format json { "status": "pass", "checks": [ - { "check": "message", "status": "pass", "value": "", "error": "", "suggest": "" }, - { "check": "subject_imperative", "status": "pass", "value": "", "error": "", "suggest": "" } + { + "rule_id": "CC001", + "check": "message", + "status": "pass", + "value": "feat: add streaming support", + "error": "", + "suggest": "", + "docs_url": "https://commit-check.com/rules/#cc001" + }, + { + "rule_id": "CC004", + "check": "subject_max_length", + "status": "pass", + "value": "feat: add streaming support", + "error": "", + "suggest": "", + "docs_url": "https://commit-check.com/rules/#cc004" + }, + { + "rule_id": "CC005", + "check": "subject_min_length", + "status": "pass", + "value": "feat: add streaming support", + "error": "", + "suggest": "", + "docs_url": "https://commit-check.com/rules/#cc005" + } ] } ``` @@ -274,11 +299,31 @@ echo "wip bad commit" | commit-check -m --format json "status": "fail", "checks": [ { - "check": "message", - "status": "fail", - "value": "wip bad commit", - "error": "The commit message should follow Conventional Commits. See https://www.conventionalcommits.org", - "suggest": "Use (): , where is one of: feat, fix, docs, ..." + "rule_id": "CC001", + "check": "message", + "status": "fail", + "value": "wip bad commit", + "error": "The commit message should follow Conventional Commits. See https://www.conventionalcommits.org", + "suggest": "Use (): , where is one of: feat, fix, docs, style, refactor, test, chore, perf, build, ci", + "docs_url": "https://commit-check.com/rules/#cc001" + }, + { + "rule_id": "CC004", + "check": "subject_max_length", + "status": "pass", + "value": "wip bad commit", + "error": "", + "suggest": "", + "docs_url": "https://commit-check.com/rules/#cc004" + }, + { + "rule_id": "CC005", + "check": "subject_min_length", + "status": "pass", + "value": "wip bad commit", + "error": "", + "suggest": "", + "docs_url": "https://commit-check.com/rules/#cc005" } ] } @@ -299,10 +344,10 @@ echo "wip bad commit" | commit-check -m --no-banner ``` ```text -Type message check failed ==> wip bad commit -It doesn't match regex: ^(build|chore|ci|docs|feat|fix|perf|refactor|revert|style|test){1}(\([\w\-\.]+\))?(!)?: ([\w ])+([\s\S]*)|(Merge).*|(fixup!.*) +CC001 message check failed ==> wip bad commit The commit message should follow Conventional Commits. See https://www.conventionalcommits.org -Suggest: Use (): , where is one of: feat, fix, docs, ... +Suggest: Use (): , where is one of: feat, fix, docs, style, refactor, test, chore, perf, build, ci +Docs: https://commit-check.com/rules/#cc001 ``` ```bash @@ -310,7 +355,7 @@ echo "wip bad commit" | commit-check -m --compact ``` ```text -[FAIL] message: wip bad commit +[FAIL] CC001 message: wip bad commit ``` ### Python API (no subprocess required) @@ -356,20 +401,72 @@ print(result["status"]) # "fail" — 'docs' not in allowed types ```python { - "status": "pass" | "fail", + "status": "pass" | "fail" | "skip", "checks": [ { - "check": "", - "status": "pass" | "fail", - "value": "", - "error": "", - "suggest": "", + "rule_id": "", + "check": "", + "status": "pass" | "fail" | "skip", + "value": "", + "error": "", + "suggest": "", + "docs_url": "", }, # ... one entry per active rule ] } ``` +`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 @@ -397,9 +494,10 @@ Commit rejected by Commit-Check. Commit rejected. -Type message check failed ==> test commit message check +CC001 message check failed ==> test commit message check The commit message should follow Conventional Commits. See https://www.conventionalcommits.org -Suggest: Use (): , where is one of: feat, fix, docs, style, refactor, test, chore, ci +Suggest: Use (): , where is one of: feat, fix, docs, style, refactor, test, chore, perf, build, ci +Docs: https://commit-check.com/rules/#cc001 ``` ### Check Branch Naming Failed @@ -418,9 +516,10 @@ Commit rejected by Commit-Check. Commit rejected. -Type branch check failed ==> test-branch +CC201 branch check failed ==> test-branch The branch should follow Conventional Branch. See https://conventionalbranch.org Suggest: Use / with allowed types or add branch name to allow_branch_names in config, or use ignore_authors in config branch section to bypass +Docs: https://commit-check.com/rules/#cc201 ``` For more examples, see the [example documentation](https://commit-check.com/example/). diff --git a/assets/demo.gif b/assets/demo.gif index 09de3032..506fa044 100644 Binary files a/assets/demo.gif and b/assets/demo.gif differ diff --git a/assets/demo.tape b/assets/demo.tape index e2e2fd84..e8d450cd 100644 --- a/assets/demo.tape +++ b/assets/demo.tape @@ -1,39 +1,80 @@ -# demo.tape — Generate demo.gif with: brew install vhs && vhs docs/demo.tape +# demo.tape — regenerate with `vhs assets/demo.tape` from the repository root. +# +# Needs vhs (https://github.com/charmbracelet/vhs) and commit-check on PATH. +# +# The hidden block below records inside a throwaway repository, so running this +# never leaves stray branches behind in the repository you invoke it from. -Output demo.gif +Output assets/demo.gif Set Shell "zsh" Set FontSize 16 -Set Width 1000 -Set Height 550 +# The longest line a failure prints is the CC001 suggestion, at 131 columns. +# This width measures 132 columns (checked with `tput cols` in the recorder, +# not calculated — the glyph advance is 10.26px, not the 9.6px you get from +# assuming 0.6em). Narrower breaks that line, and the URL above it, mid-word. +Set Width 1400 +Set Height 600 Set Theme "Dracula" Set Padding 24 Set TypingSpeed 50ms Set PlaybackSpeed 1.0 -# --- Scene 1: Invalid commit message --- -Type "echo 'add user login' | commit-check -m" +Hide +# The identity matters: without it the initial commit fails, the repository is +# left with no commits at all, and every branch check then silently passes — +# which is exactly how this demo once recorded a failing branch as clean. +# ${DEMO_REPO:?} rather than a bare cd: when a command substitution produces +# nothing, zsh reads `cd $(...)` as plain `cd` and goes to $HOME — so a failing +# mktemp would have this git init a maintainer's home directory. The guard +# aborts the chain instead. +Type "DEMO_REPO=$(mktemp -d) && cd ${DEMO_REPO:?} && git init -q -b main . && git config user.name 'Dev' && git config user.email 'dev@example.com'" +Enter +Type "git commit -q --allow-empty -m 'feat: initial commit'" +Enter +Type "export PS1='%F{cyan}❯%f '" +Enter +Type "clear" Enter Sleep 2s +Show -# --- Scene 2: Valid commit message --- -Type "echo 'feat: add user login' | commit-check -m" +# --- A commit message that does not follow Conventional Commits --- +Type "echo 'add user login' | commit-check -m" Enter -Sleep 2s +Sleep 4s -# --- Scene 4: Invalid branch name --- -Type "git checkout -b user-login" +# --- The same message, fixed. A passing check prints nothing and exits 0, so +# the demo reports the result rather than showing an empty line. --- +Type "echo 'feat: add user login' | commit-check -m && echo 'passed'" Enter -Sleep 2s -Type "commit-check -b" +Sleep 3s + +Hide +Type "clear" Enter -Sleep 2s +Show -# --- Scene 5: Valid branch name --- -Type "git checkout -b feature/user-login" +# --- A branch name that does not follow Conventional Branch --- +Type "git switch -c user-login" Enter -Sleep 2s +Sleep 1500ms Type "commit-check -b" Enter -Sleep 2s +Sleep 4s + +# --- The same branch, fixed --- +Type "git switch -c feature/user-login" +Enter +Sleep 1500ms +Type "commit-check -b && echo 'passed'" +Enter +Sleep 3s + +Hide +# Leave no throwaway repository behind. The same guard applies: an unset or +# empty DEMO_REPO aborts rather than handing rm -rf a bare path. +Type "cd / && rm -rf ${DEMO_REPO:?}" +Enter +Show 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 d06ac03d..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 @@ -21,15 +22,28 @@ get_upstream_remote_sha, has_commits, git_merge_base, + git_rev_parse_verify, ) from commit_check.imperatives import IMPERATIVES 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) @@ -54,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. @@ -77,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.""" @@ -137,10 +180,26 @@ def _resolve_current_author(context: ValidationContext) -> str: return get_git_config_value("user.name") or get_commit_info("an") return get_commit_info("an") or get_git_config_value("user.name") + @staticmethod + def _message_was_supplied(context: ValidationContext) -> bool: + """Whether the caller named a message source rather than leaving it to git. + + Distinguishes "you asked me about this empty message" from "git had + nothing to give me", which decide opposite answers: the first is a + message that fails, the second is nothing to check. + + A commit_file that cannot be read counts as named even though the text + then comes from git. That stays correct where it matters: the only way + to reach an empty message from there is a HEAD commit whose message is + genuinely empty, and rejecting that under allow_empty_commits = false + is the verdict the rule exists to give. + """ + return context.stdin_text is not None or context.commit_file is not None + @staticmethod def _get_commit_message(context: ValidationContext) -> str: """Get commit message from context or git.""" - if context.stdin_text: + if context.stdin_text is not None: return context.stdin_text.strip() if context.commit_file: @@ -182,7 +241,12 @@ def _author_in_ignore_list(self, context: ValidationContext) -> bool: @staticmethod def _get_commit_body(context: ValidationContext) -> str: """Retrieve the commit message body from context or git.""" - if context.stdin_text: + # An empty string is a message the caller supplied, not an absent one. + # Reading it as absent sends the check off to the repository's HEAD + # commit instead, so a caller asking about "" is answered about + # whatever was committed last. The skip logic above already draws the + # line at None; this follows it. + if context.stdin_text is not None: return context.stdin_text if context.commit_file: try: @@ -250,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: @@ -272,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: @@ -284,7 +348,7 @@ def validate(self, context: ValidationContext) -> ValidationResult: def _get_subject(self, context: ValidationContext) -> str: """Extract subject from commit message.""" - if context.stdin_text: + if context.stdin_text is not None: return context.stdin_text.strip().split("\n")[0] if context.commit_file: @@ -379,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: @@ -395,7 +459,7 @@ def _get_author_value(self, context: ValidationContext) -> str: Checks git config first (for pre-commit validation of the configured identity), then falls back to the last commit's author info. """ - if context.stdin_text: + if context.stdin_text is not None: return context.stdin_text.strip() git_config_map = { @@ -433,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 @@ -443,9 +508,11 @@ 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 else get_branch_name() + context.stdin_text.strip() + if context.stdin_text is not None + else get_branch_name() ) self._checked_value = branch_name @@ -466,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 @@ -481,6 +548,24 @@ def validate(self, context: ValidationContext) -> ValidationResult: return ValidationResult.PASS result = git_merge_base(target_branch, current_branch) + if result == 128: + # 128 is git failing to resolve a name, not an answer about + # ancestry. A CI checkout of a pull request leaves a detached HEAD + # with no local branch created, while get_branch_name() still + # reports a name from GITHUB_HEAD_REF — so the name here refers to + # nothing on disk. The remote-tracking ref is the real branch. + result = git_merge_base(target_branch, f"origin/{current_branch}") + if result == 128: + # Last resort, when the branch is unresolvable under either name. + # On a pull_request event HEAD is GitHub's synthetic merge commit, + # whose first parent IS the target tip, so asking about HEAD would + # pass every branch, rebased or not. Its *second* parent is the + # pull request head — the commit actually under review — so ask + # about that instead whenever HEAD is a merge. Where HEAD has a + # single parent it is the branch commit itself (a push event, or a + # branch that was never pushed) and answers for itself. + source = "HEAD^2" if git_rev_parse_verify("HEAD^2") else "HEAD" + result = git_merge_base(target_branch, source) if result == 0: return ValidationResult.PASS @@ -527,7 +612,14 @@ def _find_target_branch(self, pattern: str) -> str | None: stderr=subprocess.DEVNULL, check=True, ) - return branch_name + # Qualified with the remote, because that is the ref that was just + # verified. Returning the bare name here made the caller run + # ``git merge-base --is-ancestor main HEAD`` in a checkout that has + # only ``origin/main``; git exits 128 on the unresolvable name and + # the branch was reported as "not rebased onto target branch" when + # it was correctly based all along. A CI checkout of a pull request + # is exactly that shape. + return f"origin/{branch_name}" except subprocess.CalledProcessError: pass @@ -539,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: @@ -561,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: @@ -604,6 +696,10 @@ class ForcePushValidator(BaseValidator): ZERO_SHA = "0000000000000000000000000000000000000000" def validate(self, context: ValidationContext) -> ValidationResult: + # Emptiness, not absence, is the question here: unlike a message or a + # branch name, stdin_text carries a *list* of refs, and no refs means + # there is nothing to check either way. So this one stays a truth test + # while the single-value readers above distinguish "" from None. if not context.stdin_text: if context.push_upstream_fallback: return self._check_current_branch_against_upstream() @@ -714,12 +810,17 @@ 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) - if not message: + # allow_empty_commits is the rule that exists to judge an empty + # message, so returning early on one made it unreachable: the branch + # in _is_empty_commit_allowed that rejects an empty message could + # never run. A message the caller supplied goes to the rule even when + # it is empty; an empty one from git is still nothing to check. + if not message and not self._message_was_supplied(context): return ValidationResult.PASS self._checked_value = message @@ -793,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: @@ -933,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/imperatives.py b/commit_check/imperatives.py index 3704ee79..f3af9e08 100644 --- a/commit_check/imperatives.py +++ b/commit_check/imperatives.py @@ -1,20 +1,38 @@ +# Imperative forms of verbs, seeded from # https://github.com/crate-ci/imperative/blob/master/assets/imperatives.txt -# Imperative forms of verbs +# and extended since. # -# This file contains the imperative form of frequently encountered -# docstring verbs. Some of these may be more commonly encountered as -# nouns, but blacklisting them for this may cause false positives. +# Some of these are more commonly encountered as nouns, but leaving them out +# rejects a subject that is written correctly, which is the worse failure: the +# contributor has to reword something that was never wrong, and the only way +# they learn which words are acceptable is by trial and error. +# +# For the same reason both spellings of every -ize/-ise verb are listed. A +# project writing British English is not making a mistake. +# +# Additions are welcome and cheap. The list can only ever approximate "is this +# an English imperative verb", so treat a rejected-but-correct subject as a bug +# in this file rather than as something the author should work around. IMPERATIVES = { "abort", "absorb", + "abstract", "accept", "access", + "accommodate", + "acknowledge", + "activate", + "adapt", "add", + "address", "adjust", + "advertise", "aggregate", "align", "allow", + "amend", + "annotate", "append", "apply", "archive", @@ -22,38 +40,53 @@ "assign", "attach", "attempt", + "audit", "authenticate", + "authorise", "authorize", "auto", + "automate", + "avoid", + "await", "backport", + "balance", "batch", + "be", "bind", "block", "break", "broadcast", + "broaden", "build", "bump", + "bypass", "cache", "calculate", "call", "cancel", "capture", + "centralise", + "centralize", "change", "check", + "clarify", "clean", "clear", "close", + "collapse", "collect", "combine", "comment", "commit", "compare", + "complete", "compose", "compress", "compute", "configure", "confirm", "connect", + "consolidate", "construct", "consume", "control", @@ -62,36 +95,49 @@ "correct", "count", "create", + "customise", "customize", "debug", "declare", "decode", "decompress", "decorate", - "decrypt", "decrease", + "decrypt", + "deduplicate", + "defer", "define", "delegate", "delete", + "demonstrate", "deprecate", "derive", "describe", + "deselect", "detach", "detect", "determine", + "diagnose", + "die", + "differentiate", "disable", + "disallow", "discard", "disconnect", "dispatch", "display", "dispose", + "distinguish", "distribute", + "do", "document", - "download", "downgrade", + "download", "drop", - "duplicate", "dump", + "duplicate", + "elaborate", + "eliminate", "embed", "emit", "empty", @@ -114,24 +160,31 @@ "exit", "expand", "expect", + "expire", + "explain", "export", "expose", "extend", "extract", + "factor", "feed", "fetch", "fill", "filter", + "finalise", "finalize", "find", "fire", "fix", "flag", + "flatten", "flush", "fold", + "forbid", "force", "format", "forward", + "free", "freeze", "generate", "get", @@ -139,15 +192,21 @@ "go", "grant", "group", + "guard", "halt", "handle", + "harden", "hash", "help", "hide", "highlight", + "hoist", "hold", + "honor", + "honour", "identify", "ignore", + "illustrate", "implement", "import", "improve", @@ -162,9 +221,11 @@ "initialize", "initiate", "inject", + "inline", "input", "insert", "instantiate", + "instrument", "integrate", "intercept", "introduce", @@ -175,6 +236,8 @@ "join", "keep", "launch", + "let", + "lift", "link", "list", "listen", @@ -192,19 +255,32 @@ "marshal", "mask", "match", + "maximise", "maximize", "measure", + "memoise", + "memoize", + "mention", "merge", "migrate", + "minimise", "minimize", "mirror", "mock", + "modernise", + "modernize", "modify", + "modularise", + "modularize", "monitor", "mount", "move", "name", + "narrow", "navigate", + "neutralise", + "neutralize", + "normalise", "normalize", "note", "notify", @@ -212,45 +288,69 @@ "offset", "omit", "open", + "optimise", "optimize", + "orchestrate", + "organise", + "organize", "outline", "output", - "organize", - "orchestrate", + "overhaul", "override", "overwrite", "package", "pad", + "paginate", + "parameterise", "parameterize", "parse", "partial", "pass", "pause", "perform", + "permit", "persist", "pick", + "pin", "ping", "pipe", "plot", + "plug", + "pluralise", + "pluralize", "poll", "polyfill", "populate", + "port", "post", + "postpone", + "precompute", + "prefer", "prefix", "prepare", + "prepend", + "preserve", "prevent", "print", + "prioritise", + "prioritize", "process", "produce", - "prune", + "propagate", "provide", + "proxy", + "prune", "publish", "pull", "purge", "push", "put", + "qualify", + "quarantine", "query", + "quote", "raise", + "randomise", "randomize", "rank", "read", @@ -259,11 +359,13 @@ "rebuild", "recall", "receive", + "reclaim", "recommend", "reconcile", "reconnect", "record", "recover", + "redact", "redesign", "redirect", "reduce", @@ -272,35 +374,49 @@ "reformat", "refresh", "register", + "reinstate", "reject", "relate", + "relax", "release", "reload", "relocate", + "remap", "remove", "rename", "render", "reorder", + "reorganise", "reorganize", "repeat", + "rephrase", "replace", "replay", "reply", "report", "represent", + "repurpose", "request", "require", + "rescue", "reset", "resolve", + "respect", + "restore", "restrict", "resume", + "retarget", + "rethrow", + "retire", "retrieve", "retry", "return", "reuse", + "revalidate", "revamp", "revert", "revoke", + "rewire", "rework", "rewrite", "roll", @@ -309,10 +425,13 @@ "route", "run", "sample", + "sanitise", "sanitize", "save", "scan", "schedule", + "scope", + "seal", "search", "select", "send", @@ -321,9 +440,13 @@ "serialize", "serve", "set", + "settle", "setup", + "shard", + "shorten", "show", "shuffle", + "silence", "simplify", "simulate", "skip", @@ -332,42 +455,56 @@ "source", "spawn", "specify", + "speed", + "spell", "split", "spread", "squash", + "stabilise", + "stabilize", + "standardise", "standardize", "start", "step", "stop", "store", + "streamline", "strip", + "stub", + "subclass", "submit", "subscribe", "substitute", "suggest", "sum", - "suppress", + "supersede", "support", + "suppress", "suspend", "swap", "switch", "sync", "synchronise", "synchronize", - "terminate", + "tag", "take", + "teach", "tear", + "terminate", "test", - "throw", "throttle", + "throw", + "tighten", "time", "toggle", + "tolerate", "trace", "track", "transfer", "transform", "translate", "transmit", + "treat", "trigger", "trim", "truncate", @@ -382,10 +519,12 @@ "unmarshal", "unpack", "unsubscribe", + "untangle", "unwind", "unwrap", "update", "upgrade", + "uphold", "upload", "use", "validate", @@ -396,8 +535,11 @@ "walk", "warm", "warn", + "weaken", + "widen", "wire", "withdraw", + "work", "wrap", "write", "yield", 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/commit_check/rules_catalog.py b/commit_check/rules_catalog.py index 29717b2e..93faf529 100644 --- a/commit_check/rules_catalog.py +++ b/commit_check/rules_catalog.py @@ -24,6 +24,22 @@ RULES_DOCS_URL = "https://commit-check.com/rules/" +def display_name(check: str) -> str: + """Human-readable form of a check name, e.g. ``subject-imperative``. + + Config files and the JSON output carry the snake_case key, because that is + what a reader sets in ``cchk.toml`` and what a consumer maps back to an + option. Text written for a person uses the kebab-case form instead: it is + how the rules reference titles each rule, so a name printed to a terminal + can be searched for there verbatim. + + Every text surface goes through here so the two forms cannot drift apart + again — the compact output once printed the config key while the default + output printed this one. + """ + return check.replace("_", "-") + + @dataclass(frozen=True) class RuleCatalogEntry: check: str @@ -35,7 +51,7 @@ class RuleCatalogEntry: @property def name(self) -> str: """Human-readable rule name, e.g. ``subject-imperative``.""" - return self.check.replace("_", "-") + return display_name(self.check) @property def docs_url(self) -> str | None: diff --git a/commit_check/util.py b/commit_check/util.py index 8053d8c3..4a78ecf7 100644 --- a/commit_check/util.py +++ b/commit_check/util.py @@ -11,6 +11,7 @@ import sys from subprocess import CalledProcessError from commit_check import RED, GREEN, YELLOW, RESET_COLOR +from commit_check.rules_catalog import display_name def _print_failure( @@ -23,7 +24,8 @@ def _print_failure( rule_id = check.get("rule_id", "") if compact: compact_value = actual.splitlines()[0] if actual else actual - label = f"{rule_id} {check['check']}" if rule_id else check["check"] + name = display_name(check["check"]) + label = f"{rule_id} {name}" if rule_id else name print(f"[FAIL] {label}: {compact_value}") return if not no_banner and not print_error_header.has_been_called: @@ -193,6 +195,24 @@ def has_commits() -> bool: return False +def git_rev_parse_verify(rev: str) -> bool: + """Check whether a revision resolves in the current repository. + :param rev: any revision expression, e.g. ``HEAD^2`` + + :returns: `True` if the revision resolves, `False` otherwise. + """ + try: + subprocess.run( + ["git", "rev-parse", "--verify", "--quiet", f"{rev}^{{commit}}"], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + check=True, + ) + return True + except subprocess.CalledProcessError: + return False + + def get_commit_info(format_string: str, sha: str = "HEAD") -> str: """Get latest commits information :param format_string: could be @@ -364,9 +384,7 @@ def print_error_message( :returns: Give error messages to user """ - # The kebab-case form is what the rules reference uses as its headings, so - # the name printed here can be searched for there verbatim. - name = check_type.replace("_", "-") + name = display_name(check_type) label = rule_id if rule_id and docs_url and supports_hyperlinks(): label = hyperlink(rule_id, docs_url) 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 f096cedb..3f3ec867 100644 --- a/tests/engine_test.py +++ b/tests/engine_test.py @@ -35,6 +35,84 @@ USE_CONVENTIONAL_FORMAT = "Use conventional format" +def _pull_request_shaped_clone(tmp_path): + """Build a clone shaped like a CI checkout of a pull request. + + One commit on origin/main, one commit of work on top, then every local + branch removed and HEAD detached — so the target exists only as a + remote-tracking ref and the branch name resolves to nothing. + + Returns the clone path. + """ + import subprocess as sp + + def git(*args, cwd): + return sp.run( + ["git", *args], + cwd=cwd, + check=True, + stdout=sp.PIPE, + stderr=sp.PIPE, + text=True, + ) + + origin = tmp_path / "origin" + origin.mkdir() + git("init", "-q", "-b", "main", ".", cwd=origin) + git("config", "user.name", "Dev", cwd=origin) + git("config", "user.email", "dev@example.com", cwd=origin) + git("commit", "-q", "--allow-empty", "-m", "feat: base", cwd=origin) + + clone = tmp_path / "clone" + git("clone", "-q", str(origin), str(clone), cwd=tmp_path) + git("config", "user.name", "Dev", cwd=clone) + git("config", "user.email", "dev@example.com", cwd=clone) + git("checkout", "-q", "-b", "feat/work", cwd=clone) + git("commit", "-q", "--allow-empty", "-m", "feat: work", cwd=clone) + return clone, git + + +def _diverged_merge_ref_clone(tmp_path): + """Extend the pull-request shape into the one that hides false passes. + + Publishes feat/work, moves main past it so the branch is genuinely behind, + then detaches at a merge of the two — the shape of GitHub's synthetic merge + commit, whose first parent is the target tip. Every local branch is removed, + so the branch resolves only through refs/remotes/origin/feat/work. + """ + clone, git = _pull_request_shaped_clone(tmp_path) + git("push", "-q", "origin", "feat/work", cwd=clone) + origin = tmp_path / "origin" + git("commit", "-q", "--allow-empty", "-m", "feat: main moved on", cwd=origin) + git("fetch", "-q", "origin", cwd=clone) + git("checkout", "-q", "--detach", "origin/main", cwd=clone) + git( + "merge", + "-q", + "--no-ff", + "-m", + "Merge feat/work into main", + "feat/work", + cwd=clone, + ) + git("branch", "-q", "-D", "main", "feat/work", cwd=clone) + return clone, git + + +def _validate_merge_base(clone, branch="feat/work", target="main"): + """Run MergeBaseValidator inside ``clone`` as a CI checkout would.""" + cwd = os.getcwd() + try: + os.chdir(clone) + with patch.dict(os.environ, {"GITHUB_HEAD_REF": branch}): + validator = MergeBaseValidator( + ValidationRule(check="merge_base", regex=target) + ) + return validator.validate(ValidationContext(no_banner=True)) + finally: + os.chdir(cwd) + + class TestValidationResult: @pytest.mark.benchmark def test_validation_result_enum(self): @@ -260,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): @@ -434,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: @@ -500,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): @@ -537,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): @@ -608,6 +686,33 @@ def test_default_name_pattern_uses_builtin_regex(self): class TestCommitTypeValidator: + def test_supplied_empty_message_reaches_the_empty_commit_rule(self): + """allow_empty_commits=False must actually reject an empty message. + + The early return on a falsy message used to make this unreachable, so + the rejecting branch of _is_empty_commit_allowed was dead code. Patches + get_commit_info to prove the verdict comes from the supplied message + and not from the repository's own HEAD commit. + """ + rule = ValidationRule(check="allow_empty_commits", value=False) + validator = CommitTypeValidator(rule) + with patch("commit_check.engine.get_commit_info") as mock_commit_info: + mock_commit_info.return_value = "feat: something from git" + result = validator.validate( + ValidationContext(stdin_text="", no_banner=True) + ) + assert result == ValidationResult.FAIL + mock_commit_info.assert_not_called() + + def test_absent_message_still_skips_the_empty_commit_rule(self): + """A message git never supplied is nothing to check, not a failure.""" + rule = ValidationRule(check="allow_empty_commits", value=False) + validator = CommitTypeValidator(rule) + with patch("commit_check.engine.get_commit_info", return_value=""): + with patch("commit_check.engine.has_commits", return_value=True): + result = validator.validate(ValidationContext(no_banner=True)) + assert result == ValidationResult.PASS + @pytest.mark.benchmark def test_commit_type_validator_merge_commits(self): """Test CommitTypeValidator with merge commits.""" @@ -646,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 @@ -873,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): @@ -1045,22 +1150,38 @@ def test_validate_with_whitespace_only_message(self): class TestMergeBaseValidator: - @patch("commit_check.util.git_merge_base") + @patch("commit_check.engine.git_merge_base") @pytest.mark.benchmark def test_merge_base_validator_valid(self, mock_git_merge_base): - """Test MergeBaseValidator with valid merge base.""" + """Test MergeBaseValidator with valid merge base. + + Patched on commit_check.engine, not commit_check.util: the engine + imported the name directly, so patching the util module rebound + nothing and these tests were running real git against the checkout + they happened to be executed in. + """ mock_git_merge_base.return_value = 0 - rule = ValidationRule(check="merge_base") + # With no regex, validate() returns PASS at the "no target configured" + # exit without ever calling git_merge_base — the assertion below would + # hold no matter what the mock returned. Give it a target so the + # merge-base path actually runs. + rule = ValidationRule(check="merge_base", regex=r"^main$") validator = MergeBaseValidator(rule) context = ValidationContext() - result = validator.validate(context) + with ( + patch.object(validator, "_find_target_branch", return_value="origin/main"), + patch("commit_check.engine.get_branch_name", return_value="feature/test"), + patch("commit_check.engine.has_commits", return_value=True), + ): + result = validator.validate(context) assert result == ValidationResult.PASS + mock_git_merge_base.assert_called_once_with("origin/main", "feature/test") @patch("commit_check.engine.has_commits") @patch("commit_check.engine.get_branch_name") - @patch("commit_check.util.git_merge_base") + @patch("commit_check.engine.git_merge_base") @pytest.mark.benchmark def test_merge_base_validator_invalid( self, mock_git_merge_base, mock_get_branch_name, mock_has_commits @@ -1099,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 @@ -1122,14 +1243,21 @@ def test_find_target_branch_local_found(self, mock_run): @patch("subprocess.run") def test_find_target_branch_local_missing_remote_found(self, mock_run): - """Local missing, remote tracking exists: returns the stripped branch name.""" + """Local missing, remote tracking exists: returns the remote-qualified name. + + Qualified, not bare: the caller feeds this straight to ``git merge-base + --is-ancestor``, and a checkout holding only ``origin/develop`` cannot + resolve ``develop``. Git exits 128 there, which the validator reports as + "not rebased onto target branch" — a clean branch failing for a name it + could not look up. + """ mock_run.side_effect = [ subprocess.CalledProcessError(1, []), # local fails subprocess.CompletedProcess(args=[], returncode=0), # remote succeeds ] validator = MergeBaseValidator(ValidationRule(check="merge_base")) result = validator._find_target_branch("develop") - assert result == "develop" + assert result == "origin/develop" assert mock_run.call_args_list[1][0][0][:5] == [ "git", "rev-parse", @@ -1148,6 +1276,80 @@ def test_find_target_branch_not_found(self, mock_run): result = validator._find_target_branch("nonexistent-branch") assert result is None + def test_merge_base_against_a_real_pull_request_shaped_checkout(self, tmp_path): + """A branch based on origin/main passes when no local main exists. + + Mocked subprocess is what let this through: every call was asserted + against the arguments the code happened to pass, so a target name git + could not resolve looked correct. This drives real git instead. + """ + from commit_check.util import git_merge_base + + clone, git = _pull_request_shaped_clone(tmp_path) + git("branch", "-q", "-D", "main", cwd=clone) + + cwd = os.getcwd() + try: + os.chdir(clone) + validator = MergeBaseValidator(ValidationRule(check="merge_base")) + target = validator._find_target_branch("main") + assert target == "origin/main" + # The point of the qualification: this is what the caller runs. + assert git_merge_base(target, "feat/work") == 0 + finally: + os.chdir(cwd) + + def test_merge_base_on_a_detached_checkout_with_no_local_branch(self, tmp_path): + """A detached checkout passes when the branch name resolves to nothing. + + The other half of the same mistake: get_branch_name() falls back to + GITHUB_HEAD_REF, so on a CI checkout of a pull request it reports a + branch that was never created locally. git exits 128 on the name, and + 128 was read as "not an ancestor" rather than "could not look that up". + """ + clone, git = _pull_request_shaped_clone(tmp_path) + git("checkout", "-q", "--detach", "HEAD", cwd=clone) + git("branch", "-q", "-D", "main", cwd=clone) + git("branch", "-q", "-D", "feat/work", cwd=clone) + + cwd = os.getcwd() + try: + os.chdir(clone) + with patch.dict(os.environ, {"GITHUB_HEAD_REF": "feat/never-created"}): + validator = MergeBaseValidator( + ValidationRule(check="merge_base", regex="main") + ) + result = validator.validate(ValidationContext()) + finally: + os.chdir(cwd) + assert result == ValidationResult.PASS + + def test_merge_base_fails_a_diverged_branch_on_a_merge_ref_checkout(self, tmp_path): + """A branch that is NOT rebased must fail, even on a merge-ref checkout. + + On a pull_request event the runner checks out GitHub's synthetic merge + commit, whose first parent IS the target tip — so answering the + ancestry question from HEAD passes every branch, rebased or not. The + branch must be resolved through its remote-tracking ref instead; this + test pins that, and fails if the HEAD fallback is consulted first. + """ + clone, _ = _diverged_merge_ref_clone(tmp_path) + assert _validate_merge_base(clone) == ValidationResult.FAIL + + def test_merge_base_fails_a_diverged_branch_with_no_remote_ref(self, tmp_path): + """The same diverged branch must still fail when even the remote ref is + gone, which is where the fallback chain runs out of names. + + Measured in this shape: origin/main vs feat/work is 128, vs + origin/feat/work is 128, and vs HEAD is 0 -- so falling through to HEAD + would pass a branch that is genuinely behind. HEAD's *second* parent is + the pull request head and answers 1, the truth. + """ + clone, git = _diverged_merge_ref_clone(tmp_path) + # Leave the branch unresolvable under every name. + git("update-ref", "-d", "refs/remotes/origin/feat/work", cwd=clone) + assert _validate_merge_base(clone) == ValidationResult.FAIL + @patch("subprocess.run") def test_find_target_branch_empty_pattern(self, mock_run): """Empty or anchor-only pattern: returns None without calling subprocess.""" @@ -1420,6 +1622,58 @@ def test_validate_with_common_imperative_subjects(self, subject): assert result == ValidationResult.PASS + @pytest.mark.parametrize( + "subject", + [ + # Rejected before the list was extended, every one of them written + # in correct imperative mood. + "feat: settle the report format", + "fix: avoid a second lookup", + "docs: clarify the default value", + "refactor: factor out the helper", + "chore: teach the parser about tabs", + "fix: free the buffer on the error path", + "refactor: inline the wrapper", + "fix: restore the previous behaviour", + "chore: retire the legacy flag", + # British spelling is not a mistake. The last pair is the case the + # file used to get wrong most often: the -ize form was listed and + # the -ise one was not, so only half of a spelling pair worked. + "refactor: normalise the path separators", + "chore: prioritise the queue", + "feat: customise the template", + "feat: customize the template", + ], + ) + def test_correct_imperative_subjects_are_not_rejected(self, subject): + """Words a contributor would have had to reword around must pass. + + A whitelist can only approximate "is this an imperative verb", and the + cost of a gap falls on someone who wrote the subject correctly. + """ + rule = ValidationRule(check="subject_imperative") + validator = SubjectImperativeValidator(rule) + context = ValidationContext(stdin_text=subject) + + assert validator.validate(context) == ValidationResult.PASS + + @pytest.mark.parametrize( + "subject", + [ + "fix: updated the parser", + "feat: adding a new flag", + "fix: fixes the crash", + "chore: removed the dead code", + ], + ) + def test_wrong_verb_forms_still_fail(self, subject): + """Extending the list must not weaken what the rule is there to catch.""" + rule = ValidationRule(check="subject_imperative") + validator = SubjectImperativeValidator(rule) + context = ValidationContext(stdin_text=subject) + + assert validator.validate(context) == ValidationResult.FAIL + @pytest.mark.benchmark def test_validate_with_imperative_subject(self): """Test validation with proper imperative subject.""" @@ -1496,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): @@ -1545,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) @@ -1618,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( @@ -1650,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: @@ -2201,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): @@ -2217,3 +2471,92 @@ def test_empty_message_passes(self): result = validator.validate(context) assert result == ValidationResult.PASS + + def test_empty_message_is_not_read_from_git(self): + """An empty stdin_text must not fall through to the HEAD commit. + + The assertion above only holds while the checkout's own HEAD carries no + AI trailers, so it passed on pull request runs — where HEAD is GitHub's + synthetic merge commit with an empty body — and went red on main the + moment a commit with a Co-authored-by trailer landed. This pins the + behaviour itself, independent of whatever the repository last + committed. + """ + with patch("commit_check.engine.get_commit_info") as mock_commit_info: + mock_commit_info.return_value = "Co-authored-by: Claude " + body = AiAttributionValidator._get_commit_body( + ValidationContext(stdin_text="") + ) + 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 diff --git a/tests/main_test.py b/tests/main_test.py index 9cc43b41..d1b5f59c 100644 --- a/tests/main_test.py +++ b/tests/main_test.py @@ -728,6 +728,35 @@ def test_compact_shows_one_line_per_failure(self, mocker, capsys, monkeypatch): assert all(line.startswith("[FAIL]") for line in lines) assert len(lines) >= 1 + @pytest.mark.benchmark + def test_compact_names_checks_the_way_the_default_output_does( + self, mocker, capsys, monkeypatch + ): + """--compact prints the kebab-case name, not the config key. + + Both are text written for a person, so they have to agree. This + assertion is the one the suite was missing: --compact shipped + printing ``subject_imperative`` while the default output printed + ``subject-imperative``, and nothing here noticed. + """ + mocker.patch("sys.stdin.isatty", return_value=False) + mocker.patch("sys.stdin.read", return_value="docs: revamped the profile\n") + mocker.patch("commit_check.engine.get_commit_info", return_value="test-author") + + # A check whose name contains an underscore, so the two forms differ. + monkeypatch.setattr( + "sys.argv", [CMD, "-m", "--compact", "--subject-imperative=true"] + ) + main() + + out, _ = capsys.readouterr() + assert "CC003 subject-imperative:" in out, ( + f"--compact should print the display name: {out!r}" + ) + assert "subject_imperative" not in out, ( + f"--compact printed the config key: {out!r}" + ) + @pytest.mark.benchmark def test_compact_no_suggestions(self, mocker, capsys, monkeypatch): """--compact output must not include 'Suggest:' lines."""