fix: distinguish an absent commit message from an empty one - #534
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn
|
Warning Review limit reached
Next review available in: 50 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe engine now distinguishes explicitly supplied empty values from absent inputs. Commit, body, subject, author, and branch validation preserve empty stdin values. Commit-type and AI-attribution validation use these values without reading repository data. ChangesEmpty Input Validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@commit_check/engine.py`:
- Around line 141-149: Update commit_check/engine.py at lines 141-149 in
_message_was_supplied to base the supplied flag on successful message
resolution, so an unreadable commit_file is not treated as explicit input;
update lines 769-774 to use that resolved-source flag when evaluating empty
messages, preserving the no-CC008 behavior for a missing commit file with an
empty Git message, and add a regression test covering that case.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ade9a11-e7c2-47a0-bb3d-3b0b7a032096
📒 Files selected for processing (2)
commit_check/engine.pytests/engine_test.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #534 +/- ##
==========================================
+ Coverage 97.61% 97.72% +0.10%
==========================================
Files 12 12
Lines 1258 1272 +14
==========================================
+ Hits 1228 1243 +15
+ Misses 30 29 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn
|
Merging this PR will regress 1 benchmark
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
|
Not acting on the CodSpeed regression — the evidence says it is measurement noise, not this branch. The regressed benchmark exercises code this branch does not touch. The diff is 44 lines in Its twin moved the opposite way on that same untouched path. The comparison base is known-bad. CodSpeed's own footnote says it: "No successful run was found on The two benchmarks that are on the changed path moved the way the change predicts: Also worth noting the run was on Generated by Claude Code |
1 participant
Why this exists
mainwent red on the push run immediately after #532 merged, ontest_empty_message_passes— a test that had been green on the pull request. Nothing regressed. The merge was simply the first time that test met a real commit.The bug
_get_commit_bodytestedstdin_textfor truth, so an empty string read as not provided and the check fell through toget_commit_info("b")— the repository's HEAD commit. The test named "empty message passes" therefore never measured an empty message. Measured on a real checkout, its "empty" message resolved to 6694 characters.That is why the timing looks strange, and it takes all three:
pull_requestrun, HEAD is GitHub's synthetic merge commit, whose body is empty (git log -1 --format=%b→ 1 character). Empty body → earlyreturn PASS→ the test passed for the wrong reason.main, HEAD became the squashed commit, which carries aCo-authored-by:trailer.The commit that set it off was #532's own AI attribution.
This is the same shape as the merge-base defect #532 fixed: a test that looks isolated but reads ambient git state, in a repository where the PR checkout and the main checkout are structurally different commits. #532 cleaned up three such tests in
MergeBaseValidator; the pattern was also sitting inAiAttributionValidator.What changed
One line makes
maingreen —_get_commit_bodysplits onNone, matching_should_skip_validationand_resolve_current_author, which already did.The same looseness was in four more readers, so they follow the same rule now:
_get_commit_message,_get_subject,_get_author_value, andBranchValidator. The intent was already recorded upstream —api.validate_authordistinguishes withname is not None— only these readers had not followed it.ForcePushValidatordeliberately keeps its truth test, and says so in a comment: itsstdin_textcarries 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_allowedexists to reject an empty message underallow_empty_commits = false, butCommitTypeValidatorreturnedPASSon a falsy message before ever reaching it — the rejecting branch was dead code. A supplied message now reaches the rule even when empty; one git never gave us still returns early:The other validators keep their early return.
BodyValidatordocuments whitespace-only input as "no commit message at all" and has tests asserting it;allow_empty_commitsis the rule that owns that judgement, so the rest defer to it.Impact beyond the red build
The public API was answering the wrong question.
validate_message("")reported on the last commit rather than on the empty message it was handed — socommit-check-mcp'svalidate_commit_message("")validated whatever the server's working directory had committed last. It now reportsvalue=''.The CLI is unaffected either way:
_resolve_commit_message_sourcealready normalises empty stdin toNone.Verification
514 passed(python -m pytest tests/), ruff clean and formatted.get_commit_infoand asserting it is never consulted, so the verdict provably comes from the supplied message and not from the repository's HEAD. The pre-existingtest_empty_message_passesonly holds while the checkout's own HEAD carries no AI trailers — which is exactly what made it fragile — so the replacements pin the behaviour instead of the environment.test_empty_message_returns_failinapi_test.pywas vacuous for the same reason: it patchedget_commit_infoto"test-user"and validated that string. It now genuinely exercises the empty message.🤖 Generated with Claude Code
https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn
Generated by Claude Code
Summary by CodeRabbit