diff --git a/.pre-commit-hooks.yaml b/.pre-commit-hooks.yaml new file mode 100644 index 0000000..c71ead5 --- /dev/null +++ b/.pre-commit-hooks.yaml @@ -0,0 +1,19 @@ +# Pre-commit hook for code-reviewer. +# See: https://pre-commit.com +# Usage in .pre-commit-config.yaml: +# +# - repo: https://github.com/OpticDiff/code-reviewer +# rev: v0.6.0 +# hooks: +# - id: code-review +# stages: [pre-push] +# args: [--min-severity, high] +# +- id: code-review + name: code-review + description: AI-powered code review before push + entry: code-reviewer --diff + language: golang + pass_filenames: false + stages: [pre-push] + always_run: true diff --git a/cmd/code-reviewer/main.go b/cmd/code-reviewer/main.go index be8219b..464c3d3 100644 --- a/cmd/code-reviewer/main.go +++ b/cmd/code-reviewer/main.go @@ -12,6 +12,7 @@ import ( "github.com/OpticDiff/code-reviewer/internal/config" ctxpkg "github.com/OpticDiff/code-reviewer/internal/context" "github.com/OpticDiff/code-reviewer/internal/gitlab" + "github.com/OpticDiff/code-reviewer/internal/hook" "github.com/OpticDiff/code-reviewer/internal/model" "github.com/OpticDiff/code-reviewer/internal/reviewer" ) @@ -28,6 +29,15 @@ func main() { } } + // Handle "hook" subcommand before config.Load() since it doesn't need model config. + if len(os.Args) >= 2 && os.Args[1] == "hook" { + if err := runHook(os.Args[2:]); err != nil { + fmt.Fprintf(os.Stderr, "error: %v\n", err) + os.Exit(1) + } + os.Exit(0) + } + slog.SetDefault(slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{ Level: slog.LevelInfo, }))) @@ -152,3 +162,18 @@ func wrapProviderError(err error) error { } return fmt.Errorf("initializing model provider: %w", err) } + +// runHook dispatches hook subcommands. +func runHook(args []string) error { + if len(args) == 0 { + return fmt.Errorf("usage: code-reviewer hook ") + } + switch args[0] { + case "install": + return hook.Install() + case "uninstall": + return hook.Uninstall() + default: + return fmt.Errorf("unknown hook command: %q (valid: install, uninstall)", args[0]) + } +} diff --git a/internal/hook/hook.go b/internal/hook/hook.go new file mode 100644 index 0000000..c37279d --- /dev/null +++ b/internal/hook/hook.go @@ -0,0 +1,127 @@ +// Package hook provides git hook installation for code-reviewer. +package hook + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" +) + +// managedSentinel is the exact marker that identifies hooks managed by code-reviewer. +// Ownership checks match this sentinel, not a loose substring, so a foreign hook +// that happens to mention "code-reviewer" in a comment is never overwritten. +const managedSentinel = "# managed-by: code-reviewer" + +const preCommitHookContent = `#!/bin/sh +` + managedSentinel + ` +# code-reviewer pre-push hook +# Installed by: code-reviewer hook install +# Remove with: code-reviewer hook uninstall +# +# This hook reviews your changes before pushing. +# To skip: git push --no-verify + +set -e + +# Only run if there are commits to push. +if ! git diff --quiet @{push} 2>/dev/null; then + echo "🔍 code-reviewer: reviewing changes before push..." + code-reviewer --diff --min-severity high --no-color +fi +` + +// Install writes the pre-push hook to the repository's hooks directory. +// If a hook already exists and wasn't installed by code-reviewer, it returns an error. +func Install() error { + hooksDir, err := resolveHooksDir() + if err != nil { + return err + } + + hookPath := filepath.Join(hooksDir, "pre-push") + + // Check for existing hook. + if data, err := os.ReadFile(hookPath); err == nil { + if !strings.Contains(string(data), managedSentinel) { + return fmt.Errorf("pre-push hook already exists at %s\n\nTo overwrite, remove it first:\n rm %s", hookPath, hookPath) + } + // Our hook — safe to overwrite. + } + + // Ensure hooks directory exists. + if err := os.MkdirAll(hooksDir, 0o755); err != nil { + return fmt.Errorf("creating hooks directory: %w", err) + } + + if err := os.WriteFile(hookPath, []byte(preCommitHookContent), 0o755); err != nil { + return fmt.Errorf("writing pre-push hook: %w", err) + } + + fmt.Printf("✅ Installed pre-push hook at %s\n", hookPath) + fmt.Println(" Reviews will run automatically on git push.") + fmt.Println(" Skip with: git push --no-verify") + return nil +} + +// Uninstall removes the pre-push hook if it was installed by code-reviewer. +func Uninstall() error { + hooksDir, err := resolveHooksDir() + if err != nil { + return err + } + + hookPath := filepath.Join(hooksDir, "pre-push") + + data, err := os.ReadFile(hookPath) + if err != nil { + if os.IsNotExist(err) { + fmt.Println("No pre-push hook found.") + return nil + } + return fmt.Errorf("reading hook: %w", err) + } + + if !strings.Contains(string(data), managedSentinel) { + return fmt.Errorf("pre-push hook at %s was not installed by code-reviewer; refusing to remove", hookPath) + } + + if err := os.Remove(hookPath); err != nil { + return fmt.Errorf("removing hook: %w", err) + } + + fmt.Printf("✅ Removed pre-push hook from %s\n", hookPath) + return nil +} + +// resolveHooksDir returns the hooks directory for the current repository, +// honoring Git's core.hooksPath configuration if set. +func resolveHooksDir() (string, error) { + // Try core.hooksPath first. + cmd := exec.Command("git", "rev-parse", "--git-path", "hooks") + out, err := cmd.Output() + if err == nil { + resolved := strings.TrimSpace(string(out)) + if resolved != "" { + return resolved, nil + } + } + + // Fallback: /hooks. + gitDir, err := findGitDir() + if err != nil { + return "", err + } + return filepath.Join(gitDir, "hooks"), nil +} + +// findGitDir locates the .git directory by running git rev-parse. +func findGitDir() (string, error) { + cmd := exec.Command("git", "rev-parse", "--git-dir") + out, err := cmd.Output() + if err != nil { + return "", fmt.Errorf("not a git repository (run this from inside a git repo): %w", err) + } + return strings.TrimSpace(string(out)), nil +} diff --git a/internal/hook/hook_test.go b/internal/hook/hook_test.go new file mode 100644 index 0000000..a4a6fff --- /dev/null +++ b/internal/hook/hook_test.go @@ -0,0 +1,215 @@ +package hook + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// initGitRepo creates a temporary git repo and returns its path. +func initGitRepo(t *testing.T) string { + t.Helper() + dir := t.TempDir() + cmd := exec.Command("git", "init", dir) + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + if err := cmd.Run(); err != nil { + t.Fatalf("git init: %v", err) + } + return dir +} + +// chdirRepo changes into dir and restores the original working directory on cleanup. +func chdirRepo(t *testing.T, dir string) { + t.Helper() + orig, err := os.Getwd() + if err != nil { + t.Fatalf("os.Getwd: %v", err) + } + t.Cleanup(func() { + if err := os.Chdir(orig); err != nil { + t.Errorf("restoring working directory: %v", err) + } + }) + if err := os.Chdir(dir); err != nil { + t.Fatalf("os.Chdir(%s): %v", dir, err) + } +} + +// writeForeignHook creates a non-code-reviewer hook at .git/hooks/pre-push. +func writeForeignHook(t *testing.T, dir string) { + t.Helper() + hookDir := filepath.Join(dir, ".git", "hooks") + if err := os.MkdirAll(hookDir, 0o755); err != nil { + t.Fatalf("os.MkdirAll: %v", err) + } + if err := os.WriteFile(filepath.Join(hookDir, "pre-push"), []byte("#!/bin/sh\necho 'foreign'"), 0o755); err != nil { + t.Fatalf("os.WriteFile: %v", err) + } +} + +// TestInstallAndUninstall verifies the full lifecycle: install, re-install (idempotent), uninstall. +func TestInstallAndUninstall(t *testing.T) { + dir := initGitRepo(t) + chdirRepo(t, dir) + + // Install. + if err := Install(); err != nil { + t.Fatalf("Install() error: %v", err) + } + + hookPath := filepath.Join(dir, ".git", "hooks", "pre-push") + data, err := os.ReadFile(hookPath) + if err != nil { + t.Fatalf("hook file not found: %v", err) + } + if !strings.Contains(string(data), managedSentinel) { + t.Error("hook content should contain managed sentinel") + } + + // Check executable permission. + info, err := os.Stat(hookPath) + if err != nil { + t.Fatalf("os.Stat: %v", err) + } + if info.Mode()&0o111 == 0 { + t.Error("hook file should be executable") + } + + // Re-install should succeed (overwrite our own hook). + if err := Install(); err != nil { + t.Fatalf("re-Install() error: %v", err) + } + + // Uninstall. + if err := Uninstall(); err != nil { + t.Fatalf("Uninstall() error: %v", err) + } + + if _, err := os.Stat(hookPath); !os.IsNotExist(err) { + t.Error("hook file should be removed after uninstall") + } +} + +// TestHookPolicy covers table-driven cases for hook protection behavior. +func TestHookPolicy(t *testing.T) { + tests := []struct { + name string + setup func(t *testing.T, dir string) // optional pre-test setup + op func() error // operation under test + wantErr bool + errSubstr string + }{ + { + name: "install refuses to overwrite foreign hook", + setup: writeForeignHook, + op: Install, + wantErr: true, + errSubstr: "already exists", + }, + { + name: "uninstall refuses to remove foreign hook", + setup: writeForeignHook, + op: Uninstall, + wantErr: true, + errSubstr: "not installed by code-reviewer", + }, + { + name: "uninstall succeeds when no hook exists", + op: Uninstall, + wantErr: false, + }, + { + name: "install preserves foreign hook that mentions code-reviewer in comment", + setup: func(t *testing.T, dir string) { + t.Helper() + hookDir := filepath.Join(dir, ".git", "hooks") + if err := os.MkdirAll(hookDir, 0o755); err != nil { + t.Fatalf("os.MkdirAll: %v", err) + } + // Foreign hook that happens to mention code-reviewer but lacks the sentinel. + content := "#!/bin/sh\n# Run code-reviewer manually if needed\necho 'my hook'" + if err := os.WriteFile(filepath.Join(hookDir, "pre-push"), []byte(content), 0o755); err != nil { + t.Fatalf("os.WriteFile: %v", err) + } + }, + op: Install, + wantErr: true, + errSubstr: "already exists", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := initGitRepo(t) + chdirRepo(t, dir) + + if tt.setup != nil { + tt.setup(t, dir) + } + + err := tt.op() + if tt.wantErr { + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), tt.errSubstr) { + t.Errorf("error should contain %q, got: %v", tt.errSubstr, err) + } + } else { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + } + }) + } +} + +// TestInstallWithCustomHooksPath verifies that Install and Uninstall honor +// the core.hooksPath git config, writing to the configured directory instead +// of .git/hooks. +func TestInstallWithCustomHooksPath(t *testing.T) { + dir := initGitRepo(t) + chdirRepo(t, dir) + + // Set core.hooksPath to a custom directory. + customHooksDir := filepath.Join(dir, "my-hooks") + if err := os.MkdirAll(customHooksDir, 0o755); err != nil { + t.Fatalf("os.MkdirAll: %v", err) + } + cmd := exec.Command("git", "config", "core.hooksPath", customHooksDir) + cmd.Dir = dir + if err := cmd.Run(); err != nil { + t.Fatalf("git config core.hooksPath: %v", err) + } + + // Install should write to custom directory. + if err := Install(); err != nil { + t.Fatalf("Install() error: %v", err) + } + + customHookPath := filepath.Join(customHooksDir, "pre-push") + data, err := os.ReadFile(customHookPath) + if err != nil { + t.Fatalf("hook not found at custom path %s: %v", customHookPath, err) + } + if !strings.Contains(string(data), managedSentinel) { + t.Error("hook at custom path should contain managed sentinel") + } + + // Default location should NOT have a hook. + defaultPath := filepath.Join(dir, ".git", "hooks", "pre-push") + if _, err := os.Stat(defaultPath); !os.IsNotExist(err) { + t.Error("hook should NOT be installed at .git/hooks when core.hooksPath is set") + } + + // Uninstall should remove from custom directory. + if err := Uninstall(); err != nil { + t.Fatalf("Uninstall() error: %v", err) + } + if _, err := os.Stat(customHookPath); !os.IsNotExist(err) { + t.Error("hook should be removed from custom path after uninstall") + } +} diff --git a/internal/model/prompt.go b/internal/model/prompt.go index a107461..7f63b60 100644 --- a/internal/model/prompt.go +++ b/internal/model/prompt.go @@ -23,12 +23,16 @@ Provide insightful feedback and concrete, ready-to-use code suggestions to maint 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). Do NOT comment on deleted lines ('-') — findings must reference new_line numbers, which deleted lines lack. * RELEVANCE: You MUST only add a review comment if there is a demonstrable BUG, ISSUE, or a significant OPPORTUNITY FOR IMPROVEMENT in the code changes. +* PRECISION: When in doubt, DO NOT flag it. A false positive wastes more developer time than a missed nit. Only flag issues you are confident are genuine problems. If you would rate your confidence below 80%, omit the finding. +* ZERO IS FINE: An empty findings array is a perfectly valid review. Not every diff has issues. Do not manufacture findings to appear thorough. * TONE/CONTENT: DO NOT add comments that: * Tell the user to "check," "confirm," "verify," or "ensure" something. * Explain what the code change does or validate its purpose. * Explain the code to the author (they are assumed to know their own code). * Comment on missing trailing newlines or other purely stylistic issues. + * Suggest the change should be split into smaller changes or comment on MR scope. * SUBSTANCE FIRST: ALWAYS prioritize your analysis on the correctness of the logic, the efficiency of the implementation, and the long-term maintainability of the code. * TECHNICAL DETAIL: * Pay meticulous attention to line numbers; they MUST be correct and correspond to the numbered lines in the provided diff. @@ -42,7 +46,7 @@ STRICTLY follow these rules for review comments: * CRITICAL: Security vulnerabilities, system-breaking bugs, complete logic failure. * HIGH: Performance bottlenecks (e.g., N+1 queries), resource leaks, major architectural violations. -* MEDIUM: Typographical errors in code, missing input validation, complex logic that could be simplified. +* MEDIUM: Missing input validation that could cause runtime errors, incorrect error handling (wrong error type, missing context), logic that will produce wrong results under specific conditions. NOT: style preferences, code that "could be simpler", or theoretical concerns. * LOW: Refactoring hardcoded values to constants, minor log message enhancements, comments on docstring expansion. ## OUTPUT FORMAT @@ -67,7 +71,7 @@ You MUST respond with a valid JSON object matching this exact schema. Do NOT inc If no issues are found, return: {"summary": "description of the change", "findings": []} -The "line" field MUST correspond to the new_line number shown in the diff. The "category" MUST be one of: bug, security, performance, style, docs, scope. +The "line" field MUST correspond to the new_line number shown in the diff. The "category" MUST be one of: bug, security, performance, style, docs, scope. Use "scope" ONLY for intent-driven findings (e.g., scope creep, missing tests for new features). Do NOT use "scope" to suggest splitting or resizing the change. ## SUGGESTION RULES @@ -78,7 +82,8 @@ When providing a "suggestion", follow these rules strictly: * Output **only the corrected code** — do NOT include explanatory text, comments like "// fix: ...", diff markers (+/-), line numbers, or markdown fencing. * Keep suggestions **minimal** — include only the lines that need to change, not the entire function or block. * If the fix requires changes across multiple non-adjacent lines, describe the fix in the "body" field instead and omit the suggestion. -* If you are unsure about the exact fix, omit the suggestion and explain the issue in the "body" field.` +* If you are unsure about the exact fix, omit the suggestion and explain the issue in the "body" field. +* NEVER suggest code that references variables, functions, or imports not visible in the provided diff or context. If the fix requires importing a new package or using an API you haven't seen, describe the fix in "body" and omit the suggestion.` // focusOverlays adds focus-specific instructions to the prompt. var focusOverlays = map[string]string{ diff --git a/internal/model/prompt_test.go b/internal/model/prompt_test.go index 5f7da01..fcc30b1 100644 --- a/internal/model/prompt_test.go +++ b/internal/model/prompt_test.go @@ -249,3 +249,34 @@ func TestBuildUserPromptWithContext_EmptySnippets(t *testing.T) { t.Error("empty snippets should not inject the Related Unchanged Code section") } } + +// TestBasePrompt_PrecisionRules verifies that the base prompt contains all +// quality-improvement rules added to reduce false positives and improve precision. +func TestBasePrompt_PrecisionRules(t *testing.T) { + prompt := BuildPrompt(nil, "") + + checks := []struct { + name string + substr string + }{ + {"precision penalty", "false positive wastes more developer time"}, + {"confidence threshold", "confidence below 80%"}, + {"zero is fine", "empty findings array is a perfectly valid review"}, + {"no manufacturing", "Do not manufacture findings"}, + {"focus on additions", "FOCUS ON ADDITIONS"}, + {"additions detail", "Concentrate on lines starting with '+'"}, + {"no deleted line findings", "Do NOT comment on deleted lines"}, + {"scope restriction", `Use "scope" ONLY for intent-driven findings`}, + {"no split suggestion", "split into smaller changes"}, + {"suggestion import guard", "references variables, functions, or imports not visible"}, + {"medium tightened", "NOT: style preferences"}, + } + + for _, c := range checks { + t.Run(c.name, func(t *testing.T) { + if !strings.Contains(prompt, c.substr) { + t.Errorf("base prompt should contain %q", c.substr) + } + }) + } +}