prompt: reduce false positives + add pre-push hook - #31
Conversation
- Add precision penalty: when in doubt, DO NOT flag it - Add ZERO IS FINE: empty findings array is valid - Add FOCUS ON ADDITIONS: concentrate on new code, not deletions - Tighten MEDIUM severity: exclude style preferences and theoretical concerns - Restrict scope category to intent-driven findings only (no 'split your MR') - Add suggestion guardrail: never reference imports/APIs not in the diff - Add table-driven test covering all 10 new prompt rules
- Add 'code-reviewer hook install' to set up a pre-push git hook - Add 'code-reviewer hook uninstall' to remove it - Hook runs 'code-reviewer --diff --min-severity high' before push - Refuses to overwrite foreign hooks (safety check) - Add .pre-commit-hooks.yaml for pre-commit framework integration - Add tests for install, uninstall, foreign hook protection, re-install
|
Warning Review limit reached
Next review available in: 37 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds CLI commands and package logic for installing and removing a guarded Git pre-push hook. It also tightens review prompt instructions and adds tests for hook behavior and prompt precision. ChangesGit hook integration
Review prompt precision
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant code-reviewer
participant Git
Developer->>code-reviewer: run hook install
code-reviewer->>Git: resolve repository hooks directory
code-reviewer->>Git: write executable managed pre-push hook
Git-->>Developer: hook installed
Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
internal/hook/hook_test.go (1)
67-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTable-drive the hook policy cases.
TestInstall_ExistingForeignHook,TestUninstall_ForeignHook, andTestUninstall_NoHookcan share repository setup through cases containing setup, operation, and expected-error fields. Keep the install/reinstall/uninstall lifecycle test separate.As per coding guidelines, “
**/*_test.go: Prefer table-driven tests.”🤖 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 `@internal/hook/hook_test.go` around lines 67 - 121, The hook policy tests should be consolidated into a table-driven test covering foreign-hook install, foreign-hook uninstall, and missing-hook uninstall cases. Reuse repository setup and define per-case setup, operation, and expected-error assertions, while keeping the install/reinstall/uninstall lifecycle test separate.Source: Coding guidelines
🤖 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 @.pre-commit-hooks.yaml:
- Around line 14-15: Update the hook description associated with the
code-reviewer entry to state that it performs an AI-powered code review before
push, removing the inaccurate reference to staged changes; leave the entry
command unchanged.
In `@internal/hook/hook_test.go`:
- Around line 28-30: Check and handle every setup error in
internal/hook/hook_test.go at lines 28-30, 70-77, and 92-99: fail tests for
os.Getwd, os.Chdir, os.MkdirAll, and os.WriteFile errors, and report failures
when cleanup restores the working directory. Apply the checks consistently in
each affected test setup while preserving existing cleanup behavior.
In `@internal/hook/hook.go`:
- Around line 43-44: Replace substring-based ownership checks in
internal/hook/hook.go lines 43-44 and 82-83 with a shared isManagedHook helper
that requires the exact dedicated managed-hook sentinel/header. Use this helper
before overwriting during installation and before removing during
uninstallation, and add coverage showing a foreign hook that invokes
code-reviewer is preserved.
- Around line 23-25: Update the push-review hook’s code-reviewer invocation to
use the same explicit @{push}-equivalent base as the preceding git diff guard,
rather than its default origin/HEAD; add a defined fallback for branches without
an upstream or first-push state, preserving the existing high-severity and
no-color options.
- Around line 37-38: The hook path resolution in findGitDir must honor Git’s
configured core.hooksPath instead of always using gitDir/hooks. Resolve the
hooks directory via git rev-parse --git-path hooks, store that resolved path and
its pre-push child for Install and Uninstall, and add coverage for a repository
with core.hooksPath configured; update both affected sites in
internal/hook/hook.go at lines 37-38 and 71-71.
- Around line 96-100: Update the error handling around cmd.Output() in
findGitDir to wrap and preserve the underlying err while retaining the existing
repository-context message. Ensure Install() and Uninstall() can distinguish a
missing git executable from other Git command failures when they propagate this
error.
In `@internal/model/prompt_test.go`:
- Line 253: Add a Go doc comment immediately before the exported test function
TestBasePrompt_PrecisionRules, starting with the function name and briefly
describing what the test verifies.
In `@internal/model/prompt.go`:
- Line 26: Update the review finding schema and validation rules so findings
cannot target deleted diff lines when the output requires a new_line number.
Remove the exception permitting deletion-line findings, or explicitly support
deleted locations with an old-line/side representation; ensure all emitted
findings remain valid under the chosen contract.
---
Nitpick comments:
In `@internal/hook/hook_test.go`:
- Around line 67-121: The hook policy tests should be consolidated into a
table-driven test covering foreign-hook install, foreign-hook uninstall, and
missing-hook uninstall cases. Reuse repository setup and define per-case setup,
operation, and expected-error assertions, while keeping the
install/reinstall/uninstall lifecycle test separate.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 820b5f7c-5202-4ad0-8b80-6441c5b45c30
📒 Files selected for processing (6)
.pre-commit-hooks.yamlcmd/code-reviewer/main.gointernal/hook/hook.gointernal/hook/hook_test.gointernal/model/prompt.gointernal/model/prompt_test.go
| if ! git diff --quiet @{push} 2>/dev/null; then | ||
| echo "🔍 code-reviewer: reviewing changes before push..." | ||
| code-reviewer --diff --min-severity high --no-color |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)hook\.go$|hook|\.code-reviewer|README|go.mod' || true
echo "== hook.go outline =="
if [ -f internal/hook/hook.go ]; then
ast-grep outline internal/hook/hook.go || true
echo "== hook.go lines 1-140 =="
cat -n internal/hook/hook.go | sed -n '1,140p'
else
fd -a 'hook\.go$' . | sed -n '1,20p'
fi
echo "== searches for diff ref handling and flags =="
rg -n "code-reviewer|--diff|@\\{push\\}|origin/HEAD|push|first|fallback|no-first|first-push|diff ref|first" . git 2>/dev/null | head -200 || true
echo "== git config references =="
rg -n "core\.hooksPath|pre-push|hooksPath|code-reviewer" . -g '!vendor' -g '!node_modules' -g '!dist' -g '!build' | head -250Repository: OpticDiff/code-reviewer
Length of output: 48354
Review against the same base used by the push guard.
The guard checks @{push}, but code-reviewer --diff without an explicit ref compares against its documented default, origin/HEAD. A branch tracking a different remote branch can therefore fail on unrelated findings, and a first-push scenario needs an explicit fallback. Pass an explicit @{push}-equivalent ref and define a first-push behavior.
🤖 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 `@internal/hook/hook.go` around lines 23 - 25, Update the push-review hook’s
code-reviewer invocation to use the same explicit @{push}-equivalent base as the
preceding git diff guard, rather than its default origin/HEAD; add a defined
fallback for branches without an upstream or first-push state, preserving the
existing high-severity and no-color options.
| STRICTLY follow these rules for review comments: | ||
|
|
||
| * LOCATION: You MUST only provide comments on lines that represent actual changes in the diff. This means your comments must refer ONLY to lines beginning with '+' or '-'. DO NOT comment on context lines (lines starting with a space). | ||
| * FOCUS ON ADDITIONS: Concentrate on lines starting with '+' (new code). Lines starting with '-' (deleted code) rarely need comments unless the deletion itself introduces a bug (e.g., removing a necessary nil check or error handler). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Deleted-line findings cannot satisfy the new-line anchor contract.
Line 26 permits comments on deleted lines when their removal introduces a bug, but Line 74 requires every finding’s line to be a new_line number. Deleted diff lines have no new_line, so such findings may be rejected or attached incorrectly. Either disallow deletion-line findings or add an explicit old-line/side field to the output schema.
Also applies to: 74-74
🤖 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 `@internal/model/prompt.go` at line 26, Update the review finding schema and
validation rules so findings cannot target deleted diff lines when the output
requires a new_line number. Remove the exception permitting deletion-line
findings, or explicitly support deleted locations with an old-line/side
representation; ensure all emitted findings remain valid under the chosen
contract.
CI fix:
- Fix all 7 errcheck lint violations in hook_test.go
CodeRabbit findings addressed:
- Use sentinel-based ownership ('# managed-by: code-reviewer') instead of
loose substring match; foreign hook mentioning code-reviewer is preserved
- Wrap underlying error in findGitDir for distinguishable failure modes
- Support core.hooksPath via 'git rev-parse --git-path hooks'
- Fix pre-commit-hooks.yaml description (pre-push, not staged changes)
- Resolve deleted-line/new_line schema contradiction in prompt
- Add doc comment on TestBasePrompt_PrecisionRules
- Consolidate hook policy tests into table-driven format with setup callbacks
- Add test case for foreign hook that mentions code-reviewer but lacks sentinel
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/hook/hook_test.go (1)
96-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the configured hooks-path lifecycle.
All cases assume
.git/hooks; add a table case that setscore.hooksPathand verifies install and uninstall use that directory. Otherwise a regression in the new resolver passes these tests.🤖 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 `@internal/hook/hook_test.go` around lines 96 - 142, Extend TestHookPolicy with a table case that configures core.hooksPath to a custom directory, then verifies Install and Uninstall operate on that configured hook location rather than .git/hooks. Set up the repository configuration and assert the hook lifecycle succeeds in the custom directory while preserving the existing cases.
🤖 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 `@internal/hook/hook.go`:
- Line 70: Add a Go doc comment immediately above the exported Uninstall
function, starting with “Uninstall” and briefly describing its behavior. Do not
alter the existing resolveHooksDir flow.
---
Nitpick comments:
In `@internal/hook/hook_test.go`:
- Around line 96-142: Extend TestHookPolicy with a table case that configures
core.hooksPath to a custom directory, then verifies Install and Uninstall
operate on that configured hook location rather than .git/hooks. Set up the
repository configuration and assert the hook lifecycle succeeds in the custom
directory while preserving the existing cases.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 43e2e36a-d80b-4393-be7d-f1e0a944962b
📒 Files selected for processing (5)
.pre-commit-hooks.yamlinternal/hook/hook.gointernal/hook/hook_test.gointernal/model/prompt.gointernal/model/prompt_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- .pre-commit-hooks.yaml
- internal/model/prompt_test.go
|
|
||
| // Uninstall removes the pre-push hook if it was installed by code-reviewer. | ||
| func Uninstall() error { | ||
| hooksDir, err := resolveHooksDir() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document Uninstall.
Add a doc comment immediately above this exported function. As per coding guidelines, “All exported Go functions and types MUST have doc comments.”
🤖 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 `@internal/hook/hook.go` at line 70, Add a Go doc comment immediately above the
exported Uninstall function, starting with “Uninstall” and briefly describing
its behavior. Do not alter the existing resolveHooksDir flow.
Source: Coding guidelines
- Add TestInstallWithCustomHooksPath: sets git core.hooksPath, verifies Install writes to custom dir and .git/hooks is untouched, verifies Uninstall cleans up from custom dir
1 participant
Prompt Quality Pass (5 improvements)
+lines, not deleted codePre-push Hook
code-reviewer hook install— sets up a git pre-push hookcode-reviewer hook uninstall— removes itcode-reviewer --diff --min-severity highbefore push.pre-commit-hooks.yamlfor the pre-commit frameworkTests
Summary by CodeRabbit
New Features
pre-pushreview hook.Bug Fixes
Tests