diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 36823b6..37365ae 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -84,5 +84,3 @@ jobs: - name: Publish to npm run: npm publish --access public --provenance - env: - NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} diff --git a/cmd/opencodereview/review_cmd.go b/cmd/opencodereview/review_cmd.go index 80bf979..d29d2d8 100644 --- a/cmd/opencodereview/review_cmd.go +++ b/cmd/opencodereview/review_cmd.go @@ -45,7 +45,7 @@ func runReview(args []string) error { return fmt.Errorf("resolve repo: %w", err) } - resolver, err := rules.NewResolver(repoDir, opts.rulePath) + resolver, fileFilter, err := rules.NewResolver(repoDir, opts.rulePath) if err != nil { return fmt.Errorf("load rules: %w", err) } @@ -92,6 +92,7 @@ func runReview(args []string) error { DiffMap: diffMap, Template: *tpl, SystemRule: resolver, + FileFilter: fileFilter, LLMClient: llmClient, Tools: tools, PlanToolDefs: planToolDefs, diff --git a/internal/agent/agent.go b/internal/agent/agent.go index f6fd404..b5836c0 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -41,6 +41,10 @@ type Args struct { // SystemRule holds path-based review rules loaded from a JSON config. SystemRule rules.Resolver + // FileFilter holds user-configured include/exclude path patterns from rule.json. + // When nil, only the default extension and path filters apply. + FileFilter *rules.FileFilter + // LLM client for model inference. LLMClient llm.LLMClient @@ -230,8 +234,7 @@ func (a *Agent) Run(ctx context.Context) ([]model.LlmComment, error) { reviewCount := a.countReviewable(a.diffs) fmt.Fprintf(stdout.Writer(), "[ocr] %d file(s) changed, reviewing %d in %s\n", totalChanged, reviewCount, a.args.RepoDir) - a.diffs = a.filterUnsupportedExts(a.diffs) - a.diffs = a.filterExcludedPaths(a.diffs) + a.diffs = a.filterDiffs(a.diffs) if len(a.diffs) == 0 { fmt.Fprintln(stdout.Writer(), "[ocr] No supported files changed. Skipping review.") @@ -568,7 +571,7 @@ func (a *Agent) filterLargeDiffs(diffs []model.Diff) []model.Diff { return kept } -// countReviewable counts diffs that will survive both filters and are not pure deletions. +// countReviewable counts diffs that will survive all filters and are not pure deletions. func (a *Agent) countReviewable(diffs []model.Diff) int { count := 0 for _, d := range diffs { @@ -576,11 +579,7 @@ func (a *Agent) countReviewable(diffs []model.Diff) int { if path == "/dev/null" { path = d.OldPath } - ext := a.extFromPath(path) - if ext != "" && !allowedext.IsAllowedExt(ext) { - continue - } - if allowedext.IsExcludedPath(path) { + if !a.shouldReview(path) { continue } if d.IsDeleted { @@ -591,33 +590,39 @@ func (a *Agent) countReviewable(diffs []model.Diff) int { return count } -// filterUnsupportedExts drops diffs whose file extensions are not in the supported types allowlist. -func (a *Agent) filterUnsupportedExts(diffs []model.Diff) []model.Diff { - var kept []model.Diff - skipped := 0 +// shouldReview applies the four-gate filter algorithm: +// 1. User exclude → skip +// 2. Default extension check → skip if not allowed +// 3. User include → keep (penetrates default path exclusion) +// 4. Default path exclusion → skip +// +// If none of the gates trigger, the file is kept for review. +func (a *Agent) shouldReview(path string) bool { + f := a.args.FileFilter - for _, d := range diffs { - path := d.NewPath - if path == "/dev/null" { - path = d.OldPath - } - ext := a.extFromPath(path) - if ext != "" && !allowedext.IsAllowedExt(ext) { - fmt.Fprintf(stdout.Writer(), "[ocr] Skipping %s — extension %q not in supported file types\n", d.NewPath, ext) - skipped++ - continue - } - kept = append(kept, d) + if f != nil && f.IsUserExcluded(path) { + return false } - if skipped > 0 { - fmt.Fprintf(stdout.Writer(), "[ocr] Skipped %d file(s) with unsupported extensions\n", skipped) + ext := a.extFromPath(path) + if ext != "" && !allowedext.IsAllowedExt(ext) { + return false } - return kept + + if f != nil && f.HasInclude() && f.IsUserIncluded(path) { + return true + } + + if allowedext.IsExcludedPath(path) { + return false + } + + return true } -// filterExcludedPaths drops diffs whose file paths match any default exclude pattern. -func (a *Agent) filterExcludedPaths(diffs []model.Diff) []model.Diff { +// filterDiffs drops diffs that should not be reviewed based on user-configured +// include/exclude patterns and default extension/path filters. +func (a *Agent) filterDiffs(diffs []model.Diff) []model.Diff { var kept []model.Diff skipped := 0 @@ -626,8 +631,8 @@ func (a *Agent) filterExcludedPaths(diffs []model.Diff) []model.Diff { if path == "/dev/null" { path = d.OldPath } - if allowedext.IsExcludedPath(path) { - fmt.Fprintf(stdout.Writer(), "[ocr] Skipping %s — matches exclude pattern\n", path) + if !a.shouldReview(path) { + fmt.Fprintf(stdout.Writer(), "[ocr] Skipping %s — filtered by path/extension rules\n", path) skipped++ continue } @@ -635,7 +640,7 @@ func (a *Agent) filterExcludedPaths(diffs []model.Diff) []model.Diff { } if skipped > 0 { - fmt.Fprintf(stdout.Writer(), "[ocr] Excluded %d file(s) by pattern\n", skipped) + fmt.Fprintf(stdout.Writer(), "[ocr] Filtered %d file(s) by include/exclude rules\n", skipped) } return kept } diff --git a/internal/config/rules/system_rules.go b/internal/config/rules/system_rules.go index 8370f8a..914cea5 100644 --- a/internal/config/rules/system_rules.go +++ b/internal/config/rules/system_rules.go @@ -142,7 +142,53 @@ type ProjectRuleEntry struct { // ProjectRule holds rules loaded from /.open-code-review/rule.json. type ProjectRule struct { - Rules []ProjectRuleEntry `json:"rules"` + Rules []ProjectRuleEntry `json:"rules"` + Include []string `json:"include,omitempty"` + Exclude []string `json:"exclude,omitempty"` +} + +// FileFilter holds the merged user-configured include/exclude glob patterns +// collected from all rule.json layers (custom, project, global). +type FileFilter struct { + Include []string + Exclude []string +} + +// HasInclude reports whether any include patterns are configured. +func (f *FileFilter) HasInclude() bool { + return len(f.Include) > 0 +} + +// IsUserExcluded reports whether the given path matches any user exclude pattern. +func (f *FileFilter) IsUserExcluded(path string) bool { + lowerPath := strings.ToLower(path) + for _, pattern := range f.Exclude { + expanded := expandBraces(pattern) + for _, p := range expanded { + if matched, _ := doublestar.Match(p, lowerPath); matched { + return true + } + } + } + return false +} + +// IsUserIncluded reports whether the given path matches any user include pattern. +// Returns false when Include is empty (no user include restriction defined). +func (f *FileFilter) IsUserIncluded(path string) bool { + if !f.HasInclude() { + return false + } + lowerPath := strings.ToLower(path) + for _, pattern := range f.Include { + expanded := expandBraces(pattern) + for _, p := range expanded { + if matched, _ := doublestar.Match(p, lowerPath); matched { + return true + } + } + } + return false } // composedResolver implements Resolver with layered priority. @@ -158,17 +204,19 @@ type composedResolver struct { // 2. Project-local .open-code-review/rule.json (first match wins) // 3. Global ~/.open-code-review/rule.json (first match wins) // 4. Embedded system default rules -func NewResolver(repoDir, customRulePath string) (Resolver, error) { +// +// It also returns a FileFilter with the merged include/exclude patterns from all layers. +func NewResolver(repoDir, customRulePath string) (Resolver, *FileFilter, error) { sysRule, err := LoadDefault() if err != nil { - return nil, err + return nil, nil, err } var customRule *ProjectRule if customRulePath != "" { cr, err := loadRuleFile(customRulePath) if err != nil { - return nil, err + return nil, nil, err } customRule = cr } @@ -177,17 +225,41 @@ func NewResolver(repoDir, customRulePath string) (Resolver, error) { if repoDir != "" { pr, err := loadProjectRule(repoDir) if err != nil { - return nil, err + return nil, nil, err } projectRule = pr } globalRule, err := loadGlobalRule() if err != nil { - return nil, err + return nil, nil, err } - return &composedResolver{custom: customRule, project: projectRule, global: globalRule, system: sysRule}, nil + filter := buildFileFilter(customRule, projectRule, globalRule) + + return &composedResolver{custom: customRule, project: projectRule, global: globalRule, system: sysRule}, filter, nil +} + +// buildFileFilter picks the highest-priority layer that has any include/exclude +// configured. Priority order: custom (--rule) > project > global. +func buildFileFilter(layers ...*ProjectRule) *FileFilter { + for _, pr := range layers { + if pr == nil { + continue + } + if len(pr.Include) == 0 && len(pr.Exclude) == 0 { + continue + } + f := &FileFilter{} + for _, p := range pr.Include { + f.Include = append(f.Include, strings.ToLower(p)) + } + for _, p := range pr.Exclude { + f.Exclude = append(f.Exclude, strings.ToLower(p)) + } + return f + } + return nil } func loadGlobalRule() (*ProjectRule, error) { diff --git a/internal/config/rules/system_rules_test.go b/internal/config/rules/system_rules_test.go index 5958671..572f1e0 100644 --- a/internal/config/rules/system_rules_test.go +++ b/internal/config/rules/system_rules_test.go @@ -176,7 +176,8 @@ func truncate(s string, maxLen int) string { } func TestNewResolver_DefaultOnly(t *testing.T) { - resolver, err := NewResolver(t.TempDir(), "") + resolver, _, err := NewResolver(t.TempDir(), "") + if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -187,7 +188,8 @@ func TestNewResolver_DefaultOnly(t *testing.T) { } func TestNewResolver_ProjectFileMissing(t *testing.T) { - resolver, err := NewResolver(t.TempDir(), "") + resolver, _, err := NewResolver(t.TempDir(), "") + if err != nil { t.Fatalf("NewResolver should not fail when project rule is missing: %v", err) } @@ -208,7 +210,7 @@ func TestNewResolver_ProjectRuleHighestPriority(t *testing.T) { t.Fatalf("write rule.json: %v", err) } - resolver, err := NewResolver(dir, "") + resolver, _, err := NewResolver(dir, "") if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -241,7 +243,7 @@ func TestNewResolver_ProjectRuleFallsBackToSystem(t *testing.T) { t.Fatalf("write rule.json: %v", err) } - resolver, err := NewResolver(dir, "") + resolver, _, err := NewResolver(dir, "") if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -260,7 +262,7 @@ func TestNewResolver_CustomRuleOverridesDefault(t *testing.T) { t.Fatalf("write custom rule: %v", err) } - resolver, err := NewResolver(t.TempDir(), customPath) + resolver, _, err := NewResolver(t.TempDir(), customPath) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -296,7 +298,7 @@ func TestNewResolver_CustomOverridesProject(t *testing.T) { t.Fatalf("write rule.json: %v", err) } - resolver, err := NewResolver(repoDir, customPath) + resolver, _, err := NewResolver(repoDir, customPath) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -330,12 +332,195 @@ func TestNewResolver_ProjectFileMalformed(t *testing.T) { t.Fatalf("write: %v", err) } - _, err := NewResolver(dir, "") + _, _, err := NewResolver(dir, "") if err == nil { t.Errorf("expected error for malformed project rule.json") } } +func TestFileFilter_IsUserExcluded(t *testing.T) { + f := &FileFilter{ + Exclude: []string{"**/generated/**", "**/*.pb.go", "vendor/**/*.{go,js}"}, + } + + tests := []struct { + path string + want bool + }{ + {"src/generated/api.java", true}, + {"pkg/foo.pb.go", true}, + {"vendor/lib/util.go", true}, + {"vendor/lib/util.js", true}, + {"src/main.go", false}, + {"src/generated.go", false}, + } + for _, tt := range tests { + t.Run(tt.path, func(t *testing.T) { + if got := f.IsUserExcluded(tt.path); got != tt.want { + t.Errorf("IsUserExcluded(%q) = %v, want %v", tt.path, got, tt.want) + } + }) + } +} + +func TestFileFilter_IsUserIncluded(t *testing.T) { + f := &FileFilter{ + Include: []string{"src/**/*.java", "src/**/*.{kt,kts}"}, + } + + tests := []struct { + path string + want bool + }{ + {"src/main/foo.java", true}, + {"src/main/bar.kt", true}, + {"src/build.kts", true}, + {"test/main.java", false}, + {"src/main/util.go", false}, + } + for _, tt := range tests { + t.Run(tt.path, func(t *testing.T) { + if got := f.IsUserIncluded(tt.path); got != tt.want { + t.Errorf("IsUserIncluded(%q) = %v, want %v", tt.path, got, tt.want) + } + }) + } +} + +func TestFileFilter_IsUserIncluded_EmptyInclude(t *testing.T) { + f := &FileFilter{} + if f.IsUserIncluded("anything.java") { + t.Errorf("expected false when include is empty") + } +} + +func TestFileFilter_CaseInsensitive(t *testing.T) { + f := &FileFilter{ + Include: []string{"src/**/*.java"}, + Exclude: []string{"**/generated/**"}, + } + + if !f.IsUserIncluded("SRC/Main/Foo.Java") { + t.Errorf("expected case-insensitive include match") + } + if !f.IsUserExcluded("SRC/Generated/Api.java") { + t.Errorf("expected case-insensitive exclude match") + } +} + +func TestNewResolver_FileFilterMerged(t *testing.T) { + repoDir := t.TempDir() + ocrDir := filepath.Join(repoDir, ".open-code-review") + if err := os.MkdirAll(ocrDir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + projJSON := `{"rules":[],"include":["src/**/*.java"],"exclude":["**/generated/**"]}` + if err := os.WriteFile(filepath.Join(ocrDir, "rule.json"), []byte(projJSON), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + + _, filter, err := NewResolver(repoDir, "") + if err != nil { + t.Fatalf("NewResolver: %v", err) + } + if filter == nil { + t.Fatal("expected non-nil FileFilter") + } + if !filter.HasInclude() { + t.Error("expected HasInclude to be true") + } + if !filter.IsUserIncluded("src/main/foo.java") { + t.Error("expected src/main/foo.java to be included") + } + if !filter.IsUserExcluded("src/generated/api.java") { + t.Error("expected src/generated/api.java to be excluded") + } +} + +func TestNewResolver_FileFilterNilWhenEmpty(t *testing.T) { + _, filter, err := NewResolver(t.TempDir(), "") + if err != nil { + t.Fatalf("NewResolver: %v", err) + } + if filter != nil { + t.Errorf("expected nil FileFilter when no include/exclude configured, got %+v", filter) + } +} + +func TestNewResolver_FileFilterPriorityOverride(t *testing.T) { + repoDir := t.TempDir() + ocrDir := filepath.Join(repoDir, ".open-code-review") + if err := os.MkdirAll(ocrDir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + projJSON := `{"rules":[],"include":["src/**/*.java"],"exclude":["**/gen/**"]}` + if err := os.WriteFile(filepath.Join(ocrDir, "rule.json"), []byte(projJSON), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + + customDir := t.TempDir() + customJSON := `{"rules":[],"include":["lib/**/*.kt"],"exclude":["**/tmp/**"]}` + customPath := filepath.Join(customDir, "custom.json") + if err := os.WriteFile(customPath, []byte(customJSON), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + + _, filter, err := NewResolver(repoDir, customPath) + if err != nil { + t.Fatalf("NewResolver: %v", err) + } + if filter == nil { + t.Fatal("expected non-nil FileFilter") + } + + // Custom (--rule) has highest priority, so only its patterns take effect + if !filter.IsUserIncluded("lib/util.kt") { + t.Error("expected custom include to be active") + } + if !filter.IsUserExcluded("lib/tmp/cache.kt") { + t.Error("expected custom exclude to be active") + } + + // Project patterns should NOT be active since custom overrides + if filter.IsUserIncluded("src/main/foo.java") { + t.Error("project include should not be active when custom is present") + } + if filter.IsUserExcluded("src/gen/api.java") { + t.Error("project exclude should not be active when custom is present") + } +} + +func TestNewResolver_FileFilterFallsToProject(t *testing.T) { + repoDir := t.TempDir() + ocrDir := filepath.Join(repoDir, ".open-code-review") + if err := os.MkdirAll(ocrDir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + projJSON := `{"rules":[],"include":["src/**/*.java"],"exclude":["**/gen/**"]}` + if err := os.WriteFile(filepath.Join(ocrDir, "rule.json"), []byte(projJSON), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + + // Custom rule has no include/exclude — should fall through to project + customDir := t.TempDir() + customJSON := `{"rules":[{"path":"**/*.go","rule":"custom-go"}]}` + customPath := filepath.Join(customDir, "custom.json") + if err := os.WriteFile(customPath, []byte(customJSON), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + + _, filter, err := NewResolver(repoDir, customPath) + if err != nil { + t.Fatalf("NewResolver: %v", err) + } + if filter == nil { + t.Fatal("expected non-nil FileFilter from project layer") + } + if !filter.IsUserIncluded("src/main/foo.java") { + t.Error("expected project include to take effect when custom has none") + } +} + func TestNewResolver_BraceExpansionInProjectRule(t *testing.T) { dir := t.TempDir() ocrDir := filepath.Join(dir, ".open-code-review") @@ -347,7 +532,7 @@ func TestNewResolver_BraceExpansionInProjectRule(t *testing.T) { t.Fatalf("write: %v", err) } - resolver, err := NewResolver(dir, "") + resolver, _, err := NewResolver(dir, "") if err != nil { t.Fatalf("NewResolver: %v", err) }