Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions cmd/opencodereview/review_cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ type reviewOptions struct {
maxGitProcs int
maxTokens int
maxTokensBudget int
noFilter bool
preview bool
}

Expand Down Expand Up @@ -207,6 +208,7 @@ func executeReview(opts reviewOptions) error {
GitRunner: cc.GitRunner,
Resume: resumeState,
MaxTokensBudget: int64(opts.maxTokensBudget),
SkipFilter: opts.noFilter,
RuntimeConfig: rt.RuntimeConfig,
})

Expand Down
1 change: 1 addition & 0 deletions cmd/opencodereview/shared_flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ func registerReviewFlags(cmd *cobra.Command, opts *reviewOptions) {
addBackgroundFlags(cmd, &opts.background, &opts.backgroundFile)
addProviderFlag(cmd, &opts.provider)
addModelFlag(cmd, &opts.model)
cmd.Flags().BoolVar(&opts.noFilter, "no-filter", false, "skip the per-file REVIEW_FILTER_TASK")
addPreviewFlag(cmd, &opts.preview)
}

Expand Down
10 changes: 10 additions & 0 deletions internal/agent/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,10 @@ type Args struct {
// would exceed it. 0 = unlimited. Mirrors scan.Args.MaxTokensBudget.
MaxTokensBudget int64

// SkipFilter disables the REVIEW_FILTER_TASK even when the template
// defines one. Set via the --no-filter CLI flag.
SkipFilter bool

// RuntimeConfig carries the non-secret, allowlisted runtime settings that
// identify how this run was configured, for the manifest's
// runtime_config_sha256. It is populated by the cmd layer from the resolved
Expand Down Expand Up @@ -1238,6 +1242,12 @@ func (a *Agent) executeReviewFilter(ctx context.Context, d model.Diff, newPath s
return
}

if a.args.SkipFilter {
telemetry.SetAttr(span, "skipped", true)
fmt.Fprintf(stdout.Writer(), "[ocr] Review filter skipped for %s (--no-filter)\n", newPath)
return
}

comments := a.args.CommentCollector.CommentsForPath(newPath)
if len(comments) == 0 {
return
Expand Down
176 changes: 176 additions & 0 deletions internal/agent/coverage_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,182 @@ func TestExecuteReviewFilter_LLMError(t *testing.T) {
}
}

func TestExecuteReviewFilter_SkipFilter(t *testing.T) {
t.Run("AC-1: SkipFilter disables the filter", func(t *testing.T) {
tmpDir := t.TempDir()
sess := session.New(tmpDir, "main", "test", session.SessionOptions{ReviewMode: "diff"})
client := &fakeAgentClient{}
collector := tool.NewCommentCollector()
collector.Add(model.LlmComment{Path: "a.go", Content: "comment"})

a := New(Args{
LLMClient: client,
Model: "test",
Session: sess,
SkipFilter: true,
CommentCollector: collector,
Template: template.Template{
ReviewFilterTask: &template.LlmConversation{
Messages: []template.ChatMessage{{Role: "user", Content: "Filter: {{comments}}"}},
},
MaxTokens: 10000,
MaxToolRequestTimes: 5,
MainTask: template.LlmConversation{Messages: []template.ChatMessage{{Role: "user", Content: "t"}}},
},
})

a.executeReviewFilter(context.Background(), model.Diff{NewPath: "a.go", Diff: "+code"}, "a.go")

if client.calls != 0 {
t.Errorf("no LLM calls expected when SkipFilter is true, got %d", client.calls)
}
comments := collector.CommentsForPath("a.go")
if len(comments) != 1 {
t.Errorf("comments should be unchanged when filter is skipped, got %d", len(comments))
}
})

t.Run("AC-2: All comments preserved when skipped", func(t *testing.T) {
tmpDir := t.TempDir()
sess := session.New(tmpDir, "main", "test", session.SessionOptions{ReviewMode: "diff"})
client := &fakeAgentClient{}
collector := tool.NewCommentCollector()
collector.Add(model.LlmComment{Path: "a.go", Content: "comment 1"})
collector.Add(model.LlmComment{Path: "a.go", Content: "comment 2"})
collector.Add(model.LlmComment{Path: "a.go", Content: "comment 3"})

a := New(Args{
LLMClient: client,
Model: "test",
Session: sess,
SkipFilter: true,
CommentCollector: collector,
Template: template.Template{
ReviewFilterTask: &template.LlmConversation{
Messages: []template.ChatMessage{{Role: "user", Content: "Filter: {{comments}}"}},
},
MaxTokens: 10000,
MaxToolRequestTimes: 5,
MainTask: template.LlmConversation{Messages: []template.ChatMessage{{Role: "user", Content: "t"}}},
},
})

a.executeReviewFilter(context.Background(), model.Diff{NewPath: "a.go", Diff: "+code"}, "a.go")

comments := collector.CommentsForPath("a.go")
if len(comments) != 3 {
t.Fatalf("expected 3 comments when filter is skipped, got %d", len(comments))
}
})

t.Run("AC-3: Default (no SkipFilter) still runs filter", func(t *testing.T) {
tmpDir := t.TempDir()
sess := session.New(tmpDir, "main", "test", session.SessionOptions{ReviewMode: "diff"})

filterResp := `["c-1"]`
client := &fakeAgentClient{
responses: []*llm.ChatResponse{{
Choices: []llm.Choice{{
Message: llm.ResponseMessage{Content: &filterResp},
}},
Usage: &llm.UsageInfo{PromptTokens: 10, CompletionTokens: 5},
}},
}

collector := tool.NewCommentCollector()
collector.Add(model.LlmComment{Path: "a.go", Content: "keep this"})
collector.Add(model.LlmComment{Path: "a.go", Content: "remove this"})

a := New(Args{
LLMClient: client,
Model: "test",
Session: sess,
CommentCollector: collector,
Template: template.Template{
ReviewFilterTask: &template.LlmConversation{
Messages: []template.ChatMessage{{Role: "user", Content: "Filter: {{comments}} path={{path}} diff={{diff}}"}},
},
MaxTokens: 10000,
MaxToolRequestTimes: 5,
MainTask: template.LlmConversation{Messages: []template.ChatMessage{{Role: "user", Content: "t"}}},
},
})

a.executeReviewFilter(context.Background(), model.Diff{NewPath: "a.go", Diff: "+code"}, "a.go")

if client.calls == 0 {
t.Error("LLM client should have been called when SkipFilter is false (default)")
}
comments := collector.CommentsForPath("a.go")
if len(comments) != 1 {
t.Errorf("expected 1 comment after filter, got %d", len(comments))
}
})

t.Run("AC-4: SkipFilter is reached when ReviewFilterTask is non-nil", func(t *testing.T) {
// After the nil-template guard, SkipFilter is the next early-return.
// With a non-nil ReviewFilterTask + zero comments, the function would
// normally fall through to the LLM call; SkipFilter must short-circuit it.
tmpDir := t.TempDir()
sess := session.New(tmpDir, "main", "test", session.SessionOptions{ReviewMode: "diff"})
client := &fakeAgentClient{}
collector := tool.NewCommentCollector()
collector.Add(model.LlmComment{Path: "a.go", Content: "comment"})

a := New(Args{
LLMClient: client,
Model: "test",
Session: sess,
SkipFilter: true,
CommentCollector: collector,
Template: template.Template{
ReviewFilterTask: &template.LlmConversation{
Messages: []template.ChatMessage{{Role: "user", Content: "Filter: {{comments}}"}},
},
MaxTokens: 10000,
MaxToolRequestTimes: 5,
MainTask: template.LlmConversation{Messages: []template.ChatMessage{{Role: "user", Content: "t"}}},
},
})

a.executeReviewFilter(context.Background(), model.Diff{NewPath: "a.go", Diff: "+x"}, "a.go")

if client.calls != 0 {
t.Errorf("no LLM calls expected when SkipFilter is true, got %d", client.calls)
}
if len(collector.CommentsForPath("a.go")) != 1 {
t.Errorf("comments should be unchanged when filter is skipped")
}
})

t.Run("AC-5: Skip takes priority over no comments", func(t *testing.T) {
tmpDir := t.TempDir()
sess := session.New(tmpDir, "main", "test", session.SessionOptions{ReviewMode: "diff"})
client := &fakeAgentClient{}

a := New(Args{
LLMClient: client,
Model: "test",
Session: sess,
SkipFilter: true,
Template: template.Template{
ReviewFilterTask: &template.LlmConversation{
Messages: []template.ChatMessage{{Role: "user", Content: "Filter: {{comments}}"}},
},
MaxTokens: 10000,
MaxToolRequestTimes: 5,
MainTask: template.LlmConversation{Messages: []template.ChatMessage{{Role: "user", Content: "t"}}},
},
})

a.executeReviewFilter(context.Background(), model.Diff{NewPath: "a.go", Diff: "+x"}, "a.go")

if client.calls != 0 {
t.Errorf("no LLM calls expected when SkipFilter is true, got %d", client.calls)
}
})
}

func TestExecutePlanPhase(t *testing.T) {
tmpDir := t.TempDir()
sess := session.New(tmpDir, "main", "test", session.SessionOptions{ReviewMode: "diff"})
Expand Down