Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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, "keep all review comments without LLM post-filtering")
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