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()