| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
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@shenxianpeng, you've reached your PR review limit, so we couldn't start this review. 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR. 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: 0a83c2e0-b0b6-4db3-a1fb-ffcb97d2b5f5 📥 CommitsReviewing files that changed from the base of the PR and between 7d31a5d and 2377b45. 📒 Files selected for processing (1)
WalkthroughThe 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: tests 🚥 Pre-merge checks | ✅ 5 ✅ Passed checks (5 passed)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify 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.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ade9a11-e7c2-47a0-bb3d-3b0b7a032096
📥 CommitsReviewing files that changed from the base of the PR and between 98fb97c and 7d31a5d.
📒 Files selected for processing (2)
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ 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.
|
Sorry, something went wrong.
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
|
Sorry, something went wrong.
Merging this PR will regress 1 benchmark⚡ 3 improved benchmarks Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent. Comparing claude/fix-empty-message-602anc (2377b45) with main (7315edf)2 Footnotes
|
Sorry, something went wrong.
|
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 commit_check/engine.py, none of them in the merge-base path: $ git diff origin/main...HEAD -- commit_check/engine.py | grep -iE "^[+-].*(merge_base|MergeBase)" (no matches) Its twin moved the opposite way on that same untouched path. test_merge_base_validator_invalid improved +20.06% while test_merge_base_validator_valid regressed -34.76%. One code path cannot get both faster and slower in one commit; at 3–5 ms, dominated by subprocess spawning, that spread is the noise floor. The comparison base is known-bad. CodSpeed's own footnote says it: "No successful run was found on main (98fb97c) during the generation of this report, so 7315edf was used instead. There might be some changes unrelated to this pull request in this report." That missing run is the red main this PR exists to fix — so the report is measuring against the wrong baseline by its own admission. The two benchmarks that are on the changed path moved the way the change predicts: test_empty_message_passes ×27 faster (9,269 µs → 342 µs) and test_empty_message_returns_fail +33%, both from skipping repository lookups when the message came from stdin. Also worth noting the run was on 7d31a5d; head is now 2377b45. Once main is green again the baseline regenerates and this comparison stops being meaningful. Happy to acknowledge it on CodSpeed rather than chase it. Generated by Claude Code |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Why this exists
main went red on the push run immediately after #532 merged, on test_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_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. 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:
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 in AiAttributionValidator.
What changed
One line makes main green — _get_commit_body splits on None, matching _should_skip_validation and _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, and BranchValidator. The intent was already recorded upstream — api.validate_author distinguishes with name is not None — only these readers had not followed it.
ForcePushValidator deliberately keeps its truth test, and says so in a comment: 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 — the rejecting branch was dead code. A supplied message now reaches the rule even when empty; one git never gave us still returns early:
validate_message("") -> pass (default: empty allowed) validate_message("", allow_empty_commits=false) -> fail CC008The other validators keep their early return. BodyValidator documents whitespace-only input as "no commit message at all" and has tests asserting it; allow_empty_commits is 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 — so commit-check-mcp's validate_commit_message("") validated whatever the server's working directory had committed last. It now reports value=''.
The CLI is unaffected either way: _resolve_commit_message_source already normalises empty stdin to None.
Verification
🤖 Generated with Claude Code
https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn
Generated by Claude Code
Summary by CodeRabbit