diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 4d79a7a..ca8d676 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -574,7 +574,7 @@ func (a *Agent) filterLargeDiffs(diffs []model.Diff) []model.Diff { func (a *Agent) countReviewable(diffs []model.Diff) int { count := 0 for _, d := range diffs { - if !a.shouldReview(effectivePath(d)) { + if !a.shouldReview(d) { continue } if d.IsDeleted { @@ -585,9 +585,9 @@ func (a *Agent) countReviewable(diffs []model.Diff) int { return count } -// shouldReview applies the four-gate filter algorithm via whyExcluded. -func (a *Agent) shouldReview(path string) bool { - return a.whyExcluded(path) == ExcludeNone +// shouldReview applies the filter algorithm via whyExcluded. +func (a *Agent) shouldReview(d model.Diff) bool { + return a.whyExcluded(d) == ExcludeNone } // filterDiffs drops diffs that should not be reviewed based on user-configured @@ -598,8 +598,12 @@ func (a *Agent) filterDiffs(diffs []model.Diff) []model.Diff { for _, d := range diffs { path := effectivePath(d) - if !a.shouldReview(path) { - fmt.Fprintf(stdout.Writer(), "[ocr] Skipping %s — filtered by path/extension rules\n", path) + if !a.shouldReview(d) { + if d.IsBinary { + fmt.Fprintf(stdout.Writer(), "[ocr] Skipping %s — binary file\n", path) + } else { + fmt.Fprintf(stdout.Writer(), "[ocr] Skipping %s — filtered by path/extension rules\n", path) + } skipped++ continue } diff --git a/internal/agent/preview.go b/internal/agent/preview.go index 1447752..2966333 100644 --- a/internal/agent/preview.go +++ b/internal/agent/preview.go @@ -16,6 +16,7 @@ const ( ExcludeExtension ExcludeReason = "unsupported_ext" ExcludeDefaultPath ExcludeReason = "default_path" ExcludeDeleted ExcludeReason = "deleted" + ExcludeBinary ExcludeReason = "binary" ) // DiffPreviewEntry is one file's preview record. @@ -38,9 +39,14 @@ type DiffPreview struct { ExcludedCount int `json:"excluded_count"` } -// whyExcluded applies the same four-gate algorithm as shouldReview but +// whyExcluded applies the filter algorithm as shouldReview but // returns the specific reason a file is excluded. -func (a *Agent) whyExcluded(path string) ExcludeReason { +func (a *Agent) whyExcluded(d model.Diff) ExcludeReason { + if d.IsBinary { + return ExcludeBinary + } + + path := effectivePath(d) f := a.args.FileFilter if f != nil && f.IsUserExcluded(path) { @@ -85,7 +91,7 @@ func (a *Agent) Preview() (*DiffPreview, error) { Status: diffStatus(d), } - reason := a.whyExcluded(path) + reason := a.whyExcluded(d) if reason == ExcludeNone && d.IsDeleted { reason = ExcludeDeleted } diff --git a/internal/agent/preview_test.go b/internal/agent/preview_test.go new file mode 100644 index 0000000..274a013 --- /dev/null +++ b/internal/agent/preview_test.go @@ -0,0 +1,552 @@ +package agent + +import ( + "testing" + + "github.com/open-code-review/open-code-review/internal/config/rules" + "github.com/open-code-review/open-code-review/internal/model" +) + +func TestWhyExcluded_BinaryFile(t *testing.T) { + agent := New(Args{}) + tests := []struct { + name string + diff model.Diff + expected ExcludeReason + }{ + { + name: "binary file returns ExcludeBinary", + diff: model.Diff{ + NewPath: "image.png", + IsBinary: true, + }, + expected: ExcludeBinary, + }, + { + name: "non-binary go file returns ExcludeNone", + diff: model.Diff{ + NewPath: "main.go", + }, + expected: ExcludeNone, + }, + { + name: "binary file with valid extension still excluded", + diff: model.Diff{ + NewPath: "document.pdf", + IsBinary: true, + }, + expected: ExcludeBinary, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := agent.whyExcluded(tt.diff) + if got != tt.expected { + t.Errorf("whyExcluded() = %v, want %v", got, tt.expected) + } + }) + } +} + +func TestWhyExcluded_UserExcludePattern(t *testing.T) { + agent := New(Args{ + FileFilter: &rules.FileFilter{ + Exclude: []string{"vendor/**", "*.gen.go"}, + }, + }) + + tests := []struct { + name string + diff model.Diff + expected ExcludeReason + }{ + { + name: "file matching exclude pattern", + diff: model.Diff{ + NewPath: "vendor/foo/bar.go", + }, + expected: ExcludeUserRule, + }, + { + name: "generated file excluded", + diff: model.Diff{ + NewPath: "api.gen.go", + }, + expected: ExcludeUserRule, + }, + { + name: "regular file not excluded", + diff: model.Diff{ + NewPath: "main.go", + }, + expected: ExcludeNone, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := agent.whyExcluded(tt.diff) + if got != tt.expected { + t.Errorf("whyExcluded() = %v, want %v", got, tt.expected) + } + }) + } +} + +func TestWhyExcluded_ExtensionFilter(t *testing.T) { + agent := New(Args{}) + + tests := []struct { + name string + diff model.Diff + expected ExcludeReason + }{ + { + name: "unsupported extension txt", + diff: model.Diff{ + NewPath: "README.txt", + }, + expected: ExcludeExtension, + }, + { + name: "unsupported extension md", + diff: model.Diff{ + NewPath: "docs/guide.md", + }, + expected: ExcludeExtension, + }, + { + name: "supported extension go", + diff: model.Diff{ + NewPath: "main.go", + }, + expected: ExcludeNone, + }, + { + name: "supported extension java", + diff: model.Diff{ + NewPath: "src/Main.java", + }, + expected: ExcludeNone, + }, + { + name: "supported extension ts", + diff: model.Diff{ + NewPath: "app.ts", + }, + expected: ExcludeNone, + }, + { + name: "file without extension", + diff: model.Diff{ + NewPath: "Makefile", + }, + expected: ExcludeNone, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := agent.whyExcluded(tt.diff) + if got != tt.expected { + t.Errorf("whyExcluded() = %v, want %v", got, tt.expected) + } + }) + } +} + +func TestWhyExcluded_DefaultPathFilter(t *testing.T) { + agent := New(Args{}) + + tests := []struct { + name string + diff model.Diff + expected ExcludeReason + }{ + { + name: "test file excluded by default path", + diff: model.Diff{ + NewPath: "foo_test.go", + }, + expected: ExcludeDefaultPath, + }, + { + name: "java test file excluded", + diff: model.Diff{ + NewPath: "src/test/java/com/example/FooTest.java", + }, + expected: ExcludeDefaultPath, + }, + { + name: "regular source file not excluded", + diff: model.Diff{ + NewPath: "src/main/java/com/example/Foo.java", + }, + expected: ExcludeNone, + }, + { + name: "go source file not excluded", + diff: model.Diff{ + NewPath: "handler.go", + }, + expected: ExcludeNone, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := agent.whyExcluded(tt.diff) + if got != tt.expected { + t.Errorf("whyExcluded() = %v, want %v", got, tt.expected) + } + }) + } +} + +func TestWhyExcluded_UserIncludePattern(t *testing.T) { + agent := New(Args{ + FileFilter: &rules.FileFilter{ + Include: []string{"src/**/*.go", "pkg/**/*.go"}, + }, + }) + + tests := []struct { + name string + diff model.Diff + expected ExcludeReason + }{ + // --- Files matching include patterns bypass default-path checks --- + { + name: "file matching first include pattern is reviewed", + diff: model.Diff{ + NewPath: "src/foo/bar.go", + }, + expected: ExcludeNone, + }, + { + name: "file matching second include pattern is reviewed", + diff: model.Diff{ + NewPath: "pkg/util/helper.go", + }, + expected: ExcludeNone, + }, + { + name: "include pattern bypasses default-path exclusion for test files", + diff: model.Diff{ + NewPath: "src/foo/bar_test.go", + }, + // IMPORTANT: Even though *_test.go is excluded by IsExcludedPath, + // matching an include pattern returns ExcludeNone before that check. + expected: ExcludeNone, + }, + // --- Include is additive, NOT exclusive --- + // When include patterns are configured, files that do NOT match them + // still fall through to the default checks. If extension is valid and + // path is not default-excluded, they are still reviewed. + { + name: "non-included file with valid extension still reviewed (additive semantics)", + diff: model.Diff{ + NewPath: "vendor/baz.go", + }, + // .go is a supported extension and vendor/baz.go does not hit + // IsExcludedPath, so it falls through to ExcludeNone. + expected: ExcludeNone, + }, + { + name: "non-included file in non-excluded directory still reviewed", + diff: model.Diff{ + NewPath: "internal/handler.go", + }, + expected: ExcludeNone, + }, + // --- Extension filter still takes precedence over include logic --- + { + name: "unsupported extension excluded before include check", + diff: model.Diff{ + NewPath: "docs/readme.md", + }, + expected: ExcludeExtension, + }, + { + name: "unsupported extension even if path looks like include dir", + diff: model.Diff{ + NewPath: "src/notes.txt", + }, + expected: ExcludeExtension, + }, + // --- Default-path exclusion still applies to non-included files --- + { + name: "non-included test file excluded by default path", + diff: model.Diff{ + NewPath: "internal/handler_test.go", + }, + // Does not match include patterns, falls through. + // IsExcludedPath matches *_test.go → ExcludeDefaultPath. + expected: ExcludeDefaultPath, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := agent.whyExcluded(tt.diff) + if got != tt.expected { + t.Errorf("whyExcluded() = %q, want %q", got, tt.expected) + } + }) + } +} + +// TestWhyExcluded_IncludeBypassesDefaultPath verifies that a file matching +// an include pattern is reviewed even when it would normally be excluded by +// the default-path filter (e.g. test files, generated code patterns). +func TestWhyExcluded_IncludeBypassesDefaultPath(t *testing.T) { + agent := New(Args{ + FileFilter: &rules.FileFilter{ + Include: []string{"**/*_test.go"}, + }, + }) + + tests := []struct { + name string + diff model.Diff + expected ExcludeReason + }{ + { + name: "test file explicitly included overrides default-path exclusion", + diff: model.Diff{ + NewPath: "foo_test.go", + }, + expected: ExcludeNone, + }, + { + name: "non-test file still reviewed via default checks", + diff: model.Diff{ + NewPath: "main.go", + }, + expected: ExcludeNone, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := agent.whyExcluded(tt.diff) + if got != tt.expected { + t.Errorf("whyExcluded() = %q, want %q", got, tt.expected) + } + }) + } +} + +// TestWhyExcluded_IncludeAndExcludeInteraction verifies priority: user +// exclude patterns take precedence over include patterns. +func TestWhyExcluded_IncludeAndExcludeInteraction(t *testing.T) { + agent := New(Args{ + FileFilter: &rules.FileFilter{ + Include: []string{"src/**/*.go"}, + Exclude: []string{"src/generated/**"}, + }, + }) + + tests := []struct { + name string + diff model.Diff + expected ExcludeReason + }{ + { + name: "included file is reviewed", + diff: model.Diff{ + NewPath: "src/handler.go", + }, + expected: ExcludeNone, + }, + { + name: "file matching both include and exclude is excluded (exclude wins)", + diff: model.Diff{ + NewPath: "src/generated/api.go", + }, + expected: ExcludeUserRule, + }, + { + name: "file outside include with valid ext still reviewed (additive)", + diff: model.Diff{ + NewPath: "lib/utils.go", + }, + expected: ExcludeNone, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := agent.whyExcluded(tt.diff) + if got != tt.expected { + t.Errorf("whyExcluded() = %q, want %q", got, tt.expected) + } + }) + } +} + +func TestWhyExcluded_PriorityOrder(t *testing.T) { + agent := New(Args{ + FileFilter: &rules.FileFilter{ + Exclude: []string{"vendor/**"}, + }, + }) + + // Binary check should happen first, even if excluded by user pattern + diff := model.Diff{ + NewPath: "vendor/image.png", + IsBinary: true, + } + + got := agent.whyExcluded(diff) + if got != ExcludeBinary { + t.Errorf("whyExcluded() = %v, want %v (binary should be checked first)", got, ExcludeBinary) + } +} + +func TestShouldReview(t *testing.T) { + agent := New(Args{}) + + tests := []struct { + name string + diff model.Diff + expected bool + }{ + { + name: "binary file should not be reviewed", + diff: model.Diff{ + NewPath: "image.png", + IsBinary: true, + }, + expected: false, + }, + { + name: "regular go file should be reviewed", + diff: model.Diff{ + NewPath: "main.go", + }, + expected: true, + }, + { + name: "test file should not be reviewed", + diff: model.Diff{ + NewPath: "main_test.go", + }, + expected: false, + }, + { + name: "unsupported extension should not be reviewed", + diff: model.Diff{ + NewPath: "README.md", + }, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := agent.shouldReview(tt.diff) + if got != tt.expected { + t.Errorf("shouldReview() = %v, want %v", got, tt.expected) + } + }) + } +} + +func TestEffectivePath(t *testing.T) { + tests := []struct { + name string + diff model.Diff + expected string + }{ + { + name: "normal new path", + diff: model.Diff{ + OldPath: "old.go", + NewPath: "new.go", + }, + expected: "new.go", + }, + { + name: "new path is dev/null (deleted file)", + diff: model.Diff{ + OldPath: "deleted.go", + NewPath: "/dev/null", + }, + expected: "deleted.go", + }, + { + name: "renamed file uses new path", + diff: model.Diff{ + OldPath: "old_name.go", + NewPath: "new_name.go", + }, + expected: "new_name.go", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := effectivePath(tt.diff) + if got != tt.expected { + t.Errorf("effectivePath() = %v, want %v", got, tt.expected) + } + }) + } +} + +func TestDiffStatus(t *testing.T) { + tests := []struct { + name string + diff model.Diff + expected string + }{ + { + name: "binary file", + diff: model.Diff{ + IsBinary: true, + }, + expected: "binary", + }, + { + name: "new file", + diff: model.Diff{ + IsNew: true, + }, + expected: "added", + }, + { + name: "deleted file", + diff: model.Diff{ + IsDeleted: true, + }, + expected: "deleted", + }, + { + name: "renamed file", + diff: model.Diff{ + OldPath: "old.go", + NewPath: "new.go", + }, + expected: "renamed", + }, + { + name: "modified file", + diff: model.Diff{ + OldPath: "main.go", + NewPath: "main.go", + }, + expected: "modified", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := diffStatus(tt.diff) + if got != tt.expected { + t.Errorf("diffStatus() = %v, want %v", got, tt.expected) + } + }) + } +}