diff --git a/cmd/opencodereview/flags_test.go b/cmd/opencodereview/flags_test.go index 3a9a4db57..91115619b 100644 --- a/cmd/opencodereview/flags_test.go +++ b/cmd/opencodereview/flags_test.go @@ -79,6 +79,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 f4d8244ac..31bb538a8 100644 --- a/cmd/opencodereview/review_cmd.go +++ b/cmd/opencodereview/review_cmd.go @@ -41,6 +41,7 @@ type reviewOptions struct { model string concurrency int perFileTimeout int + planTimeoutSecs int maxTools int maxGitProcs int maxTokens int @@ -201,6 +202,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.go b/cmd/opencodereview/shared.go index 88c37533c..ef1f8604c 100644 --- a/cmd/opencodereview/shared.go +++ b/cmd/opencodereview/shared.go @@ -61,7 +61,7 @@ func resolveMaxTokens(templateDefault int, cfg *Config, cliOverride int) (int, e } // 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. @@ -74,7 +74,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_flags.go b/cmd/opencodereview/shared_flags.go index 9c9615925..f87989045 100644 --- a/cmd/opencodereview/shared_flags.go +++ b/cmd/opencodereview/shared_flags.go @@ -109,6 +109,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 } @@ -175,6 +178,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.maxTokens, &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) addProviderFlag(cmd, &opts.provider) addModelFlag(cmd, &opts.model) diff --git a/cmd/opencodereview/shared_test.go b/cmd/opencodereview/shared_test.go index cc3a90dfd..413834768 100644 --- a/cmd/opencodereview/shared_test.go +++ b/cmd/opencodereview/shared_test.go @@ -50,6 +50,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"}) diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 4a11c571f..663e6a2ba 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -106,6 +106,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 166801ff1..e1911c297 100644 --- a/internal/agent/coverage_test.go +++ b/internal/agent/coverage_test.go @@ -9,6 +9,7 @@ import ( "fmt" "strings" "testing" + "time" "github.com/alibaba/open-code-review/internal/config/rules" "github.com/alibaba/open-code-review/internal/config/template" @@ -18,6 +19,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"}) @@ -417,6 +425,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 8a9c1dd38..5738798f4 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 ae3f07956..b4fa5ec3a 100644 --- a/internal/config/template/template_test.go +++ b/internal/config/template/template_test.go @@ -96,8 +96,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)