fix: prefer git config user.name for author validation in BaseValidator - #461
Conversation
✅ Deploy Preview for commit-check ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesAuthor resolution and validator integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #461 +/- ##
==========================================
+ Coverage 97.23% 97.24% +0.01%
==========================================
Files 12 12
Lines 1157 1163 +6
==========================================
+ Hits 1125 1131 +6
Misses 32 32 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Merging this PR will not alter performance
|
github-actions
Bot
added
bug
…sion
The original fix replaced get_commit_info("an") unconditionally with
get_git_config_value("user.name") or get_commit_info("an") in both
_author_in_ignore_list and _should_skip_branch_validation. This fixed
the reported bug (piped stdin silently skipped when HEAD is a bot) but
introduced a regression: when validating an existing bot commit with no
stdin, the local git config user.name (a human) was used instead of the
commit's own author, so bots like dependabot/renovate were no longer
recognized as ignored.
Instead, add a mode-aware _resolve_current_author helper:
- Prospective message (stdin or commit_file): resolve from git config
user.name first, falling back to the last commit's author. The last
commit's author is unrelated to the pending commit.
- Existing commit (no stdin, no commit_file): resolve from the last
commit's author first, falling back to git config. The commit being
validated is the last commit.
Fixes the reported bug without breaking dependabot/renovate skip.
fix: add test for _resolve_current_author fallback path (coverage)
397a374 to
1f3ec2a
Compare
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/engine_test.py (1)
383-438: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the existing
GIT_CONFIG_VALUEconstant instead of hardcoding the patch target.The file already defines
GIT_CONFIG_VALUEfor"commit_check.engine.get_git_config_value"(used at lines 442, 460, 550), but these new tests (and similarly at lines 248, 489, 1295-1397, 1852-1855) hardcode the raw string repeatedly instead. SonarCloud also flags related literal duplication in this hunk (dependabot[bot],Alice Developer,pre-commit-ci[bot],get_commit_info,get_branch_name,has_commits). Consider using the established constant/fixtures for consistency.♻️ Example fix
- patch( - "commit_check.engine.get_git_config_value", - return_value="Alice Developer", - ), + patch(GIT_CONFIG_VALUE, return_value="Alice Developer"),🤖 Prompt for 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. In `@tests/engine_test.py` around lines 383 - 438, Replace hardcoded patch targets in the affected tests with the existing GIT_CONFIG_VALUE constant, including both BranchValidator tests and the similarly flagged occurrences in this test module. Reuse established constants or fixtures for repeated author values and Git helper patch targets such as get_commit_info, get_branch_name, and has_commits where available, without changing test behavior.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@tests/engine_test.py`:
- Around line 383-438: Replace hardcoded patch targets in the affected tests
with the existing GIT_CONFIG_VALUE constant, including both BranchValidator
tests and the similarly flagged occurrences in this test module. Reuse
established constants or fixtures for repeated author values and Git helper
patch targets such as get_commit_info, get_branch_name, and has_commits where
available, without changing test behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f07abb15-fa41-4180-b123-a2578ad2748e
📒 Files selected for processing (2)
commit_check/engine.pytests/engine_test.py
1 participant
I have an issue when I am running the following command
It looks like my latest commit from the main branch is a bot commit which user has been ignored by the cchk.toml config file
Summary by CodeRabbit