From 08d14c188a91e709b601ad5b05340c68e40228b9 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen Date: Fri, 7 Aug 2026 12:23:06 +0300 Subject: [PATCH 1/2] fix: distinguish an absent commit message from an empty one (#534) * fix: treat an empty commit message as supplied, not absent main went red on the push run right after #532 merged, on a test that had been green on the pull request. Nothing regressed -- the merge was the first time the test met a real commit. _get_commit_body tested stdin_text for truth, so an empty string read as "not provided" and the check fell through to get_commit_info("b"), the repository's HEAD commit. test_empty_message_passes therefore never measured an empty message: on a pull_request run HEAD is GitHub's synthetic merge commit, whose body is empty, so it passed for the wrong reason; on main HEAD became the squashed commit carrying a Co-authored-by trailer, CC013 detected it, and the test failed. Measured on this checkout, the "empty" message resolved to 6694 characters. The same looseness reaches the public API: validate_message("") answers about the last commit rather than the empty message it was given. The skip logic in this file already draws the line at None (_should_skip_validation, _resolve_current_author); _get_commit_body now follows it. The CLI is unaffected -- _resolve_commit_message_source already normalises empty stdin to None. Adds a hermetic regression test: the existing one only holds while the checkout's own HEAD carries no AI trailers, which is what made it fragile in the first place. The new one patches get_commit_info and asserts it is never consulted. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn * fix: distinguish an absent commit message from an empty one Follows the one-line fix on _get_commit_body by applying the same rule to the readers that were still testing stdin_text for truth, so an empty string is no longer read as "the caller said nothing". _get_commit_message, _get_subject, _get_author_value and BranchValidator now split on None, matching _should_skip_validation and _resolve_current_author, which already did. api.validate_author draws the same line with `name is not None`, so the intent was there; only these readers had not followed it. ForcePushValidator deliberately keeps a truth test: its stdin_text carries a *list* of refs, where empty genuinely means nothing to check rather than a value to judge. That surfaced a rule that could never fire. _is_empty_commit_allowed exists to reject an empty message under allow_empty_commits = false, but CommitTypeValidator returned PASS on a falsy message before ever reaching it, so the rejecting branch was dead code. A supplied message now reaches the rule even when empty; one git never gave us still returns early. Measured after the change: validate_message("") -> pass (default) validate_message("", allow_empty_commits=off) -> fail CC008 The other validators keep their early return: BodyValidator documents whitespace-only input as "no commit message at all", and allow_empty_commits is the rule that owns that judgement. Adds two tests pinning both directions, each patching get_commit_info to prove the verdict comes from the supplied message rather than the repository's HEAD. Restoring the early return fails them. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn * docs: record why an unreadable commit file still counts as named Review asked whether _message_was_supplied should drop to False when a commit_file cannot be read, since the text then comes from git. Measured the only reachable case: a HEAD commit whose message is genuinely empty, where allow_empty_commits = false makes CC008 the correct verdict. Deriving the flag from successful resolution would restore the miss this branch fixes, so the behaviour stands and the docstring now says why. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn --------- Co-authored-by: Claude Fable 5 --- commit_check/engine.py | 44 +++++++++++++++++++++++++++++++++++------ tests/engine_test.py | 45 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 6 deletions(-) diff --git a/commit_check/engine.py b/commit_check/engine.py index f3d200e5..1ea2c5cd 100644 --- a/commit_check/engine.py +++ b/commit_check/engine.py @@ -138,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: @@ -183,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: @@ -285,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: @@ -396,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 = { @@ -446,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 @@ -630,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() @@ -745,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/tests/engine_test.py b/tests/engine_test.py index eb0f7c8b..8070246c 100644 --- a/tests/engine_test.py +++ b/tests/engine_test.py @@ -686,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.""" @@ -2444,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() From 90c5abe941f9758b55922057b28b95d946d389ff Mon Sep 17 00:00:00 2001 From: Xianpeng Shen Date: Fri, 7 Aug 2026 12:43:06 +0300 Subject: [PATCH 2/2] docs: refresh README sample output to match what commit-check prints (#535) The README's output blocks predate rule IDs, so they showed neither the CCxxx identifiers nor the Docs links that every failure now prints. Same staleness #530 fixed in the demo GIF; the docs site was already current, only the README had been left behind. Measured by running each documented command against the checkout: Type message check failed ==> ... -> CC001 message check failed ==> ... Type branch check failed ==> ... -> CC201 branch check failed ==> ... plus a trailing Docs: https://commit-check.com/rules/#ccNNN line on both, and two commit types the list had never picked up (perf, build). Four more blocks were stale the same way: --no-banner carried an "It doesn't match regex:" line that no longer exists in the source, --compact now prints the rule id, both --format json examples were missing rule_id and docs_url and named a subject_imperative check the default run does not emit (it reports subject_max_length and subject_min_length), and the Python API return-value schema was missing rule_id and docs_url. Every block was re-captured and compared byte for byte against the committed text, so these are transcripts rather than transcriptions. --- README.md | 87 +++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 68 insertions(+), 19 deletions(-) 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/).