diff --git a/README.md b/README.md index f1ec9590..6e840143 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) @@ -359,11 +404,13 @@ print(result["status"]) # "fail" — 'docs' not in allowed types "status": "pass" | "fail", "checks": [ { - "check": "", - "status": "pass" | "fail", - "value": "", - "error": "", - "suggest": "", + "rule_id": "", + "check": "", + "status": "pass" | "fail", + "value": "", + "error": "", + "suggest": "", + "docs_url": "", }, # ... one entry per active rule ] @@ -397,9 +444,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 +466,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/engine.py b/commit_check/engine.py index d06ac03d..1ea2c5cd 100644 --- a/commit_check/engine.py +++ b/commit_check/engine.py @@ -21,6 +21,7 @@ get_upstream_remote_sha, has_commits, git_merge_base, + git_rev_parse_verify, ) from commit_check.imperatives import IMPERATIVES @@ -137,10 +138,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 +199,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: @@ -284,7 +306,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: @@ -395,7 +417,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 = { @@ -445,7 +467,9 @@ def validate(self, context: ValidationContext) -> ValidationResult: if self._should_skip_branch_validation(context): return ValidationResult.PASS 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 @@ -481,6 +505,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 +569,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 @@ -604,6 +653,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() @@ -719,7 +772,12 @@ def validate(self, context: ValidationContext) -> ValidationResult: return ValidationResult.PASS 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 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/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/engine_test.py b/tests/engine_test.py index f096cedb..8070246c 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): @@ -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.""" @@ -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 @@ -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.""" @@ -2217,3 +2471,21 @@ 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() 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."""