From e2e461dcfdbe2179e5beb34ecb25f18ac55b4270 Mon Sep 17 00:00:00 2001 From: mingsing <1072118803@qq.com> Date: Mon, 27 Jul 2026 21:00:00 +0800 Subject: [PATCH 1/3] fix: honor lower max-tools overrides --- cmd/opencodereview/shared.go | 4 ++-- cmd/opencodereview/shared_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/cmd/opencodereview/shared.go b/cmd/opencodereview/shared.go index 071b5184..3244b9f1 100644 --- a/cmd/opencodereview/shared.go +++ b/cmd/opencodereview/shared.go @@ -40,7 +40,7 @@ type commonContext struct { } // loadCommonContext validates the working directory, loads the embedded -// template, raises MaxToolRequestTimes when maxTools exceeds the default, +// template, overrides MaxToolRequestTimes when maxTools is explicitly set, // resolves the absolute repo path, loads system review rules, and creates // the global git subprocess limiter. Both review and scan callers go // through this so the startup sequence stays consistent. @@ -53,7 +53,7 @@ func loadCommonContext(repoDirInput, rulePath string, maxTools, maxGitProcs int, if err != nil { return nil, fmt.Errorf("load default template: %w", err) } - if maxTools > tpl.MaxToolRequestTimes { + if maxTools > 0 { tpl.MaxToolRequestTimes = maxTools } if err := tpl.Validate(); err != nil { diff --git a/cmd/opencodereview/shared_test.go b/cmd/opencodereview/shared_test.go index 4db02273..0e904f61 100644 --- a/cmd/opencodereview/shared_test.go +++ b/cmd/opencodereview/shared_test.go @@ -17,6 +17,34 @@ func TestApplyCLIExcludes_Empty(t *testing.T) { } } +func TestLoadCommonContext_MaxToolsOverride(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + repoDir := t.TempDir() + + tests := []struct { + name string + maxTools int + want int + }{ + {name: "template default", maxTools: 0, want: 30}, + {name: "lower bound", maxTools: 10, want: 10}, + {name: "lower than default", maxTools: 15, want: 15}, + {name: "higher than default", maxTools: 40, want: 40}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cc, err := loadCommonContext(repoDir, "", tt.maxTools, 0, false) + if err != nil { + t.Fatalf("loadCommonContext() error: %v", err) + } + if got := cc.Template.MaxToolRequestTimes; got != tt.want { + t.Errorf("MaxToolRequestTimes = %d, want %d", got, tt.want) + } + }) + } +} + func TestApplyCLIExcludes_AppendsPatterns(t *testing.T) { cc := &commonContext{FileFilter: &rules.FileFilter{Exclude: []string{"a"}}} applyCLIExcludes(cc, []string{"b", "c"}) From 6d4649b0bd8be3d3618d15710e2e0c90f098ce9a Mon Sep 17 00:00:00 2001 From: Mingsing <1072118803@qq.com> Date: Wed, 5 Aug 2026 18:41:43 +0800 Subject: [PATCH 2/3] perf: bound per-file review latency --- cmd/opencodereview/flags_test.go | 17 +++++++++++ cmd/opencodereview/review_cmd.go | 2 ++ cmd/opencodereview/shared_flags.go | 4 +++ internal/agent/agent.go | 11 +++++++ internal/agent/coverage_test.go | 33 +++++++++++++++++++++ internal/config/template/task_template.json | 2 +- internal/config/template/template_test.go | 4 +-- 7 files changed, 70 insertions(+), 3 deletions(-) diff --git a/cmd/opencodereview/flags_test.go b/cmd/opencodereview/flags_test.go index 303a5361..52213701 100644 --- a/cmd/opencodereview/flags_test.go +++ b/cmd/opencodereview/flags_test.go @@ -66,6 +66,23 @@ func TestParseReviewFlags_NegativeMaxTools(t *testing.T) { } } +func TestParseReviewFlags_PlanTimeout(t *testing.T) { + opts, err := parseReviewFlags([]string{"--plan-timeout", "120"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if opts.planTimeoutSecs != 120 { + t.Errorf("planTimeoutSecs = %d, want 120", opts.planTimeoutSecs) + } +} + +func TestParseReviewFlags_NegativePlanTimeout(t *testing.T) { + _, err := parseReviewFlags([]string{"--plan-timeout", "-1"}) + if err == nil { + t.Fatal("expected error for negative plan-timeout") + } +} + func TestParseReviewFlags_MaxToolsBelowMin(t *testing.T) { opts, err := parseReviewFlags([]string{"--max-tools", "5"}) if err != nil { diff --git a/cmd/opencodereview/review_cmd.go b/cmd/opencodereview/review_cmd.go index 268218a9..c4f2f4a3 100644 --- a/cmd/opencodereview/review_cmd.go +++ b/cmd/opencodereview/review_cmd.go @@ -36,6 +36,7 @@ type reviewOptions struct { model string concurrency int perFileTimeout int + planTimeoutSecs int maxTools int maxGitProcs int maxTokensBudget int @@ -179,6 +180,7 @@ func executeReview(opts reviewOptions) error { CommentWorkerPool: agent.NewCommentWorkerPool(opts.concurrency), MaxConcurrency: opts.concurrency, ConcurrentTaskTimeout: opts.perFileTimeout, + PlanTaskTimeout: time.Duration(opts.planTimeoutSecs) * time.Second, Model: rt.Model, Provider: rt.Provider, Background: opts.background, diff --git a/cmd/opencodereview/shared_flags.go b/cmd/opencodereview/shared_flags.go index f2eac76d..543643da 100644 --- a/cmd/opencodereview/shared_flags.go +++ b/cmd/opencodereview/shared_flags.go @@ -101,6 +101,9 @@ func validateReviewOptions(opts *reviewOptions) error { if opts.preview && opts.resume != "" { return fmt.Errorf("--preview and --resume cannot be used together") } + if opts.planTimeoutSecs < 0 { + return fmt.Errorf("--plan-timeout must be a non-negative integer (0 means no separate timeout)") + } if err := validateAudience(opts.audience); err != nil { return err } @@ -151,6 +154,7 @@ func registerReviewFlags(cmd *cobra.Command, opts *reviewOptions) { addExcludeFlag(cmd, &opts.excludes) addOutputFlags(cmd, &opts.outputFormat, &opts.audience) addConcurrencyFlags(cmd, &opts.concurrency, &opts.perFileTimeout, &opts.maxTools, &opts.maxGitProcs, &opts.maxTokensBudget) + cmd.Flags().IntVar(&opts.planTimeoutSecs, "plan-timeout", 0, "per-file plan task timeout in seconds (0 = use the file timeout only)") addBackgroundFlags(cmd, &opts.background, &opts.backgroundFile) addModelFlag(cmd, &opts.model) addPreviewFlag(cmd, &opts.preview) diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 9f53fbbd..d8d6eed2 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -100,6 +100,11 @@ type Args struct { // Concurrent task timeout in minutes. 0 means no timeout. ConcurrentTaskTimeout int + // PlanTaskTimeout bounds the optional plan LLM call. When it expires, + // executeSubtask continues with the main task without plan guidance. + // A non-positive value means the per-file context is the only deadline. + PlanTaskTimeout time.Duration + // CommentCollector collects review comments generated by the code_comment tool. CommentCollector *tool.CommentCollector @@ -1460,6 +1465,12 @@ func (a *Agent) extFromPath(path string) string { // executePlanPhase runs the plan task for a single file, sending template messages // with resolved placeholders and collecting the LLM response as plan guidance. func (a *Agent) executePlanPhase(ctx context.Context, newPath, rawDiff, changeFiles, rule string) (string, error) { + if a.args.PlanTaskTimeout > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, a.args.PlanTaskTimeout) + defer cancel() + } + ctx, span := telemetry.StartSpan(ctx, "plan.execute") defer span.End() telemetry.SetAttr(span, "file.path", newPath) diff --git a/internal/agent/coverage_test.go b/internal/agent/coverage_test.go index 5baf11bd..17c7d800 100644 --- a/internal/agent/coverage_test.go +++ b/internal/agent/coverage_test.go @@ -6,6 +6,7 @@ import ( "fmt" "strings" "testing" + "time" "github.com/alibaba/open-code-review/internal/config/rules" "github.com/alibaba/open-code-review/internal/config/template" @@ -15,6 +16,13 @@ import ( "github.com/alibaba/open-code-review/internal/tool" ) +type contextBlockingClient struct{} + +func (contextBlockingClient) CompletionsWithCtx(ctx context.Context, _ llm.ChatRequest) (*llm.ChatResponse, error) { + <-ctx.Done() + return nil, ctx.Err() +} + func TestAgent_Getters(t *testing.T) { tmpDir := t.TempDir() sess := session.New(tmpDir, "main", "test-model", session.SessionOptions{ReviewMode: "diff"}) @@ -414,6 +422,31 @@ func TestExecutePlanPhase_LLMError(t *testing.T) { } } +func TestExecutePlanPhase_Timeout(t *testing.T) { + tmpDir := t.TempDir() + sess := session.New(tmpDir, "main", "test", session.SessionOptions{ReviewMode: "diff"}) + + a := New(Args{ + LLMClient: contextBlockingClient{}, + Model: "test", + Session: sess, + PlanTaskTimeout: 20 * time.Millisecond, + Template: template.Template{ + PlanTask: &template.LlmConversation{ + Messages: []template.ChatMessage{{Role: "user", Content: "{{diff}}"}}, + }, + MaxTokens: 10000, + MaxToolRequestTimes: 5, + MainTask: template.LlmConversation{Messages: []template.ChatMessage{{Role: "user", Content: "t"}}}, + }, + }) + + _, err := a.executePlanPhase(context.Background(), "a.go", "+x", "", "") + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("executePlanPhase error = %v, want context deadline exceeded", err) + } +} + func TestExecuteSubtask_EmptyMainTask(t *testing.T) { tmpDir := t.TempDir() sess := session.New(tmpDir, "main", "test", session.SessionOptions{ReviewMode: "diff"}) diff --git a/internal/config/template/task_template.json b/internal/config/template/task_template.json index 8a9c1dd3..5738798f 100644 --- a/internal/config/template/task_template.json +++ b/internal/config/template/task_template.json @@ -31,5 +31,5 @@ }, "MAX_TOOL_REQUEST_TIMES": 30, "PLAN_MODE_LINE_THRESHOLD": 50, - "MAX_TOKENS": 58888 + "MAX_TOKENS": 30000 } diff --git a/internal/config/template/template_test.go b/internal/config/template/template_test.go index 44cddae0..04583b90 100644 --- a/internal/config/template/template_test.go +++ b/internal/config/template/template_test.go @@ -80,8 +80,8 @@ func TestLoadDefault_FieldsPopulated(t *testing.T) { if tpl.ReviewFilterTask == nil { t.Fatal("ReviewFilterTask is nil, expected non-nil") } - if tpl.MaxTokens != 58888 { - t.Errorf("MaxTokens = %d, want 58888", tpl.MaxTokens) + if tpl.MaxTokens != 30000 { + t.Errorf("MaxTokens = %d, want 30000", tpl.MaxTokens) } if tpl.MaxToolRequestTimes != 30 { t.Errorf("MaxToolRequestTimes = %d, want 30", tpl.MaxToolRequestTimes) From 4f3971da579b5d73ea16338f252961ecde12f118 Mon Sep 17 00:00:00 2001 From: Mingsing <1072118803@qq.com> Date: Sun, 9 Aug 2026 14:56:32 +0800 Subject: [PATCH 3/3] feat: expose review max tokens override --- cmd/opencodereview/flags_test.go | 17 +++++++++++++++++ cmd/opencodereview/review_cmd.go | 4 ++++ cmd/opencodereview/shared_flags.go | 4 ++++ 3 files changed, 25 insertions(+) diff --git a/cmd/opencodereview/flags_test.go b/cmd/opencodereview/flags_test.go index 52213701..fda965b1 100644 --- a/cmd/opencodereview/flags_test.go +++ b/cmd/opencodereview/flags_test.go @@ -83,6 +83,23 @@ func TestParseReviewFlags_NegativePlanTimeout(t *testing.T) { } } +func TestParseReviewFlags_MaxTokens(t *testing.T) { + opts, err := parseReviewFlags([]string{"--max-tokens", "30000"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if opts.maxTokens != 30000 { + t.Errorf("maxTokens = %d, want 30000", opts.maxTokens) + } +} + +func TestParseReviewFlags_NegativeMaxTokens(t *testing.T) { + _, err := parseReviewFlags([]string{"--max-tokens", "-1"}) + if err == nil { + t.Fatal("expected error for negative max-tokens") + } +} + func TestParseReviewFlags_MaxToolsBelowMin(t *testing.T) { opts, err := parseReviewFlags([]string{"--max-tools", "5"}) if err != nil { diff --git a/cmd/opencodereview/review_cmd.go b/cmd/opencodereview/review_cmd.go index c4f2f4a3..da1529bf 100644 --- a/cmd/opencodereview/review_cmd.go +++ b/cmd/opencodereview/review_cmd.go @@ -37,6 +37,7 @@ type reviewOptions struct { concurrency int perFileTimeout int planTimeoutSecs int + maxTokens int maxTools int maxGitProcs int maxTokensBudget int @@ -99,6 +100,9 @@ func executeReview(opts reviewOptions) error { if err != nil { return err } + if opts.maxTokens > 0 { + cc.Template.MaxTokens = opts.maxTokens + } applyCLIExcludes(cc, splitPaths(opts.excludes)) // Security (#112): reject ref-option injection before any git invocation. diff --git a/cmd/opencodereview/shared_flags.go b/cmd/opencodereview/shared_flags.go index 543643da..69b6fc2f 100644 --- a/cmd/opencodereview/shared_flags.go +++ b/cmd/opencodereview/shared_flags.go @@ -104,6 +104,9 @@ func validateReviewOptions(opts *reviewOptions) error { if opts.planTimeoutSecs < 0 { return fmt.Errorf("--plan-timeout must be a non-negative integer (0 means no separate timeout)") } + if opts.maxTokens < 0 { + return fmt.Errorf("--max-tokens must be a non-negative integer (0 means use template default)") + } if err := validateAudience(opts.audience); err != nil { return err } @@ -155,6 +158,7 @@ func registerReviewFlags(cmd *cobra.Command, opts *reviewOptions) { addOutputFlags(cmd, &opts.outputFormat, &opts.audience) addConcurrencyFlags(cmd, &opts.concurrency, &opts.perFileTimeout, &opts.maxTools, &opts.maxGitProcs, &opts.maxTokensBudget) cmd.Flags().IntVar(&opts.planTimeoutSecs, "plan-timeout", 0, "per-file plan task timeout in seconds (0 = use the file timeout only)") + cmd.Flags().IntVar(&opts.maxTokens, "max-tokens", 0, "maximum tokens retained in each LLM conversation (0 = template default)") addBackgroundFlags(cmd, &opts.background, &opts.backgroundFile) addModelFlag(cmd, &opts.model) addPreviewFlag(cmd, &opts.preview)