diff --git a/cmd/opencodereview/review_cmd.go b/cmd/opencodereview/review_cmd.go index f4d8244a..a04c6380 100644 --- a/cmd/opencodereview/review_cmd.go +++ b/cmd/opencodereview/review_cmd.go @@ -45,6 +45,7 @@ type reviewOptions struct { maxGitProcs int maxTokens int maxTokensBudget int + noFilter bool preview bool } @@ -207,6 +208,7 @@ func executeReview(opts reviewOptions) error { GitRunner: cc.GitRunner, Resume: resumeState, MaxTokensBudget: int64(opts.maxTokensBudget), + SkipFilter: opts.noFilter, RuntimeConfig: rt.RuntimeConfig, }) diff --git a/cmd/opencodereview/shared_flags.go b/cmd/opencodereview/shared_flags.go index 9c961592..1fc349a2 100644 --- a/cmd/opencodereview/shared_flags.go +++ b/cmd/opencodereview/shared_flags.go @@ -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) } diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 4a11c571..0091c7d5 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -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 @@ -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 diff --git a/internal/agent/coverage_test.go b/internal/agent/coverage_test.go index 166801ff..5bff00aa 100644 --- a/internal/agent/coverage_test.go +++ b/internal/agent/coverage_test.go @@ -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"})