From 1b4f01f8d9c8ef03f06a9dd8d24667365d68c18d Mon Sep 17 00:00:00 2001 From: shenxianpeng Date: Fri, 7 Aug 2026 07:10:23 +0000 Subject: [PATCH 1/3] 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 --- commit_check/engine.py | 7 ++++++- tests/engine_test.py | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/commit_check/engine.py b/commit_check/engine.py index f3d200e5..d5b93757 100644 --- a/commit_check/engine.py +++ b/commit_check/engine.py @@ -183,7 +183,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: diff --git a/tests/engine_test.py b/tests/engine_test.py index eb0f7c8b..38a9f9c1 100644 --- a/tests/engine_test.py +++ b/tests/engine_test.py @@ -2444,3 +2444,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 7d31a5dda688ff5c894a4f4046b9cb978dabc7a5 Mon Sep 17 00:00:00 2001 From: shenxianpeng Date: Fri, 7 Aug 2026 07:24:09 +0000 Subject: [PATCH 2/3] 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 --- commit_check/engine.py | 31 ++++++++++++++++++++++++++----- tests/engine_test.py | 27 +++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 5 deletions(-) diff --git a/commit_check/engine.py b/commit_check/engine.py index d5b93757..5166eacb 100644 --- a/commit_check/engine.py +++ b/commit_check/engine.py @@ -138,10 +138,20 @@ 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 handed us a message 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. + """ + 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: @@ -290,7 +300,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: @@ -401,7 +411,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 = { @@ -451,7 +461,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 @@ -635,6 +647,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() @@ -750,7 +766,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 38a9f9c1..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.""" From 2377b456346ffddaa4f9ab5ddee585029ccd5766 Mon Sep 17 00:00:00 2001 From: shenxianpeng Date: Fri, 7 Aug 2026 07:34:31 +0000 Subject: [PATCH 3/3] 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 --- commit_check/engine.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/commit_check/engine.py b/commit_check/engine.py index 5166eacb..1ea2c5cd 100644 --- a/commit_check/engine.py +++ b/commit_check/engine.py @@ -140,11 +140,17 @@ def _resolve_current_author(context: ValidationContext) -> str: @staticmethod def _message_was_supplied(context: ValidationContext) -> bool: - """Whether the caller handed us a message rather than leaving it to git. + """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