diff --git a/cmd/opencodereview/output.go b/cmd/opencodereview/output.go index 99c3f2ad..cefd8964 100644 --- a/cmd/opencodereview/output.go +++ b/cmd/opencodereview/output.go @@ -25,13 +25,17 @@ func outputText(comments []model.LlmComment) { func hasSubtaskErrors(warnings []agent.AgentWarning) bool { for _, w := range warnings { - if w.Type == "subtask_error" { + if isSubtaskErrorType(w.Type) { return true } } return false } +func isSubtaskErrorType(warningType string) bool { + return warningType == "subtask_error" || warningType == "scan_subtask_error" +} + func outputTextWithWarnings(comments []model.LlmComment, warnings []agent.AgentWarning) { if len(comments) == 0 { if hasSubtaskErrors(warnings) { @@ -45,7 +49,7 @@ func outputTextWithWarnings(comments []model.LlmComment, warnings []agent.AgentW } } for _, w := range warnings { - if w.Type == "subtask_error" { + if isSubtaskErrorType(w.Type) { continue } fmt.Fprintf(os.Stderr, "[ocr] WARNING [%s] %s: %s\n", w.Type, sanitizeTerminal(w.File), sanitizeTerminal(w.Message)) diff --git a/cmd/opencodereview/output_helpers_test.go b/cmd/opencodereview/output_helpers_test.go index 2d2f97e8..9f4b63d8 100644 --- a/cmd/opencodereview/output_helpers_test.go +++ b/cmd/opencodereview/output_helpers_test.go @@ -22,6 +22,7 @@ func TestHasSubtaskErrors(t *testing.T) { {"empty", []agent.AgentWarning{}, false}, {"no subtask errors", []agent.AgentWarning{{Type: "other", Message: "msg"}}, false}, {"has subtask error", []agent.AgentWarning{{Type: "subtask_error", Message: "fail"}}, true}, + {"has scan subtask error", []agent.AgentWarning{{Type: "scan_subtask_error", Message: "fail"}}, true}, {"mixed", []agent.AgentWarning{{Type: "warn"}, {Type: "subtask_error"}}, true}, } for _, tc := range tests { diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 1f8d8743..0d1b154d 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -685,7 +685,7 @@ func (a *Agent) executeSubtask(ctx context.Context, d model.Diff) (bool, string, return false, "", err } if !mainCompleted { - return false, "main_task did not complete before stopping", nil + return false, "", fmt.Errorf("main_task did not complete before stopping") } return true, "", nil } diff --git a/internal/agent/agent_test.go b/internal/agent/agent_test.go index c746e103..d08ec928 100644 --- a/internal/agent/agent_test.go +++ b/internal/agent/agent_test.go @@ -695,12 +695,16 @@ func TestDispatchSubtasks_MainTaskWithoutTaskDoneIsNotReusableCheckpoint(t *test a.currentDate = "2025-06-26 10:00" _, err := a.dispatchSubtasks(context.Background()) - if err != nil { - t.Fatalf("dispatchSubtasks: %v", err) + if err == nil || !strings.Contains(err.Error(), "all 1 file review(s) failed") { + t.Fatalf("expected all-failed error, got %v", err) } if client.calls != 1 { t.Fatalf("LLM calls = %d, want 1", client.calls) } + warnings := a.Warnings() + if len(warnings) != 1 || warnings[0].Type != "subtask_error" || warnings[0].File != diff.NewPath { + t.Fatalf("warnings = %+v, want one subtask_error for %s", warnings, diff.NewPath) + } sess.Finalize() state, err := session.LoadResumeState(repoDir, sess.SessionID) @@ -726,6 +730,46 @@ func TestDispatchSubtasks_MainTaskWithoutTaskDoneIsNotReusableCheckpoint(t *test } } +func TestDispatchSubtasks_IncompleteMainTaskMarksPartialFailure(t *testing.T) { + emptyContent := "" + client := &fakeAgentClient{responses: []*llm.ChatResponse{ + agentTaskDoneResponse(), + { + Choices: []llm.Choice{{Message: llm.ResponseMessage{Content: &emptyContent}}}, + Model: "fake", + Usage: &llm.UsageInfo{PromptTokens: 10, CompletionTokens: 1}, + }, + }} + a := New(Args{ + From: "main", + To: "feature", + LLMClient: client, + Model: "fake", + MaxConcurrency: 1, + Template: template.Template{ + MaxTokens: 100000, + MaxToolRequestTimes: 1, + MainTask: template.LlmConversation{ + Messages: []template.ChatMessage{{Role: "user", Content: "Review {{diff}}"}}, + }, + }, + }) + a.diffs = []model.Diff{ + {NewPath: "complete.go", OldPath: "complete.go", Diff: "+x", Insertions: 1}, + {NewPath: "incomplete.go", OldPath: "incomplete.go", Diff: "+y", Insertions: 1}, + } + a.currentDate = "2025-06-26 10:00" + + _, err := a.dispatchSubtasks(context.Background()) + if err != nil { + t.Fatalf("one completed file should keep the review partial, got %v", err) + } + warnings := a.Warnings() + if len(warnings) != 1 || warnings[0].Type != "subtask_error" || warnings[0].File != "incomplete.go" { + t.Fatalf("warnings = %+v, want one subtask_error for incomplete.go", warnings) + } +} + func TestDispatchSubtasks_AllDeleted(t *testing.T) { client := &fakeAgentClient{} a := New(Args{ diff --git a/internal/llmloop/loop.go b/internal/llmloop/loop.go index 3b080b3e..e6368a29 100644 --- a/internal/llmloop/loop.go +++ b/internal/llmloop/loop.go @@ -145,7 +145,7 @@ func (r *Runner) CollectPendingComments() []model.LlmComment { // tool calls returned by the model, and collects review comments until // task_done is called or limits are reached. Token usage and warnings // are aggregated on the Runner across all files. The returned bool is true -// only when the model explicitly calls task_done. +// only when the model explicitly calls task_done with a successful state. func (r *Runner) RunPerFile(ctx context.Context, messages []llm.Message, newPath string) (bool, error) { toolReqCount := r.deps.Template.MaxToolRequestTimes const maxConsecutiveEmptyRounds = 3 @@ -217,7 +217,9 @@ func (r *Runner) RunPerFile(ctx context.Context, messages []llm.Message, newPath for _, call := range calls { cp := r.executeToolCall(ctx, newPath, call, rec) - if cp.Completed { + if cp.Failed { + return false, fmt.Errorf("task failed: %s", cp.Data) + } else if cp.Completed { results = append(results, tool.ToolCallResult{ ToolCallID: call.ID, Name: call.Function.Name, @@ -307,7 +309,26 @@ func (r *Runner) executeToolCall(ctx context.Context, newPath string, call llm.T } if t == tool.TaskDone { - return tool.Complete() + args, err := parseToolArgs(call.Function.Arguments) + if err != nil { + return tool.Of(fmt.Sprintf("Error parsing tool arguments for %s: %v", t.Name(), err)) + } + rawState, hasState := args["state"] + if !hasState { + return tool.Complete() + } + state, ok := rawState.(string) + if !ok { + return tool.Of("Error: task_done state must be DONE or FAILED.") + } + switch state { + case "DONE": + return tool.Complete() + case "FAILED": + return tool.Fail("task_done reported FAILED") + default: + return tool.Of(fmt.Sprintf("Error: invalid task_done state %q; expected DONE or FAILED.", state)) + } } p := lookupTool(r.deps.Tools, t) diff --git a/internal/llmloop/loop_test.go b/internal/llmloop/loop_test.go index cb426287..000742d3 100644 --- a/internal/llmloop/loop_test.go +++ b/internal/llmloop/loop_test.go @@ -30,7 +30,7 @@ func (f *fakeClient) CompletionsWithCtx(_ context.Context, _ llm.ChatRequest) (* return resp, nil } -func taskDoneResponse() *llm.ChatResponse { +func taskDoneResponseWithArguments(arguments string) *llm.ChatResponse { content := "" return &llm.ChatResponse{ Choices: []llm.Choice{{ @@ -41,7 +41,7 @@ func taskDoneResponse() *llm.ChatResponse { Type: "function", Function: llm.FunctionCall{ Name: "task_done", - Arguments: `{}`, + Arguments: arguments, }, }}, }, @@ -51,6 +51,10 @@ func taskDoneResponse() *llm.ChatResponse { } } +func taskDoneResponse() *llm.ChatResponse { + return taskDoneResponseWithArguments(`{}`) +} + func fileReadToolCallResponse(callID, args string) *llm.ChatResponse { content := "" return &llm.ChatResponse{ @@ -118,6 +122,84 @@ func TestRunPerFile_TaskDoneImmediately(t *testing.T) { } } +func TestRunPerFile_TaskDoneExplicitDone(t *testing.T) { + client := &fakeClient{responses: []*llm.ChatResponse{ + taskDoneResponseWithArguments(`{"state":"DONE"}`), + }} + runner := NewRunner(newTestDeps(client)) + + completed, err := runner.RunPerFile( + context.Background(), + []llm.Message{llm.NewTextMessage("user", "review this file")}, + "main.go", + ) + if err != nil { + t.Fatalf("RunPerFile: %v", err) + } + if !completed { + t.Fatal("expected task_done DONE to complete RunPerFile") + } +} + +func TestRunPerFile_TaskDoneFailed(t *testing.T) { + client := &fakeClient{responses: []*llm.ChatResponse{ + taskDoneResponseWithArguments(`{"state":"FAILED"}`), + }} + runner := NewRunner(newTestDeps(client)) + + completed, err := runner.RunPerFile( + context.Background(), + []llm.Message{llm.NewTextMessage("user", "review this file")}, + "main.go", + ) + if err == nil || !strings.Contains(err.Error(), "task_done reported FAILED") { + t.Fatalf("expected task_done FAILED error, got %v", err) + } + if completed { + t.Fatal("task_done FAILED must not complete RunPerFile") + } + if client.calls != 1 { + t.Fatalf("expected terminal failure after 1 LLM call, got %d", client.calls) + } +} + +func TestRunPerFile_InvalidTaskDoneStateRetries(t *testing.T) { + tests := []struct { + name string + arguments string + }{ + {name: "unknown state", arguments: `{"state":"UNKNOWN"}`}, + {name: "empty state", arguments: `{"state":""}`}, + {name: "non-string state", arguments: `{"state":1}`}, + {name: "malformed arguments", arguments: `{"state":`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + client := &fakeClient{responses: []*llm.ChatResponse{ + taskDoneResponseWithArguments(tt.arguments), + taskDoneResponseWithArguments(`{"state":"DONE"}`), + }} + runner := NewRunner(newTestDeps(client)) + + completed, err := runner.RunPerFile( + context.Background(), + []llm.Message{llm.NewTextMessage("user", "review this file")}, + "main.go", + ) + if err != nil { + t.Fatalf("RunPerFile: %v", err) + } + if !completed { + t.Fatal("expected retry to complete with task_done DONE") + } + if client.calls != 2 { + t.Fatalf("expected invalid state to be retried, got %d LLM calls", client.calls) + } + }) + } +} + func TestRunPerFile_ToolCallThenDone(t *testing.T) { client := &fakeClient{responses: []*llm.ChatResponse{ fileReadToolCallResponse("call_1", `{"path":"main.go"}`), diff --git a/internal/scan/agent.go b/internal/scan/agent.go index 240fac74..0a770866 100644 --- a/internal/scan/agent.go +++ b/internal/scan/agent.go @@ -572,8 +572,14 @@ func (a *Agent) executeSubtask(ctx context.Context, it model.ScanItem) error { return nil } - _, err := a.runner.RunPerFile(ctx, messages, it.Path) - return err + completed, err := a.runner.RunPerFile(ctx, messages, it.Path) + if err != nil { + return err + } + if !completed { + return fmt.Errorf("main_task did not complete before stopping") + } + return nil } // maybeRunPlan invokes PLAN_TASK on the file and returns a human-readable diff --git a/internal/scan/coverage_test.go b/internal/scan/coverage_test.go index aa7c8e1d..d109afdb 100644 --- a/internal/scan/coverage_test.go +++ b/internal/scan/coverage_test.go @@ -648,6 +648,47 @@ func TestDispatchSubtasks_AllFailed(t *testing.T) { } } +func TestDispatchSubtasks_WithoutTaskDoneIsAllFailed(t *testing.T) { + empty := "" + client := &fakeScanClient{responses: []*llm.ChatResponse{{ + Choices: []llm.Choice{{Message: llm.ResponseMessage{Content: &empty}}}, + Model: "test", + Usage: &llm.UsageInfo{PromptTokens: 10, CompletionTokens: 1}, + }}} + + tpl := makeTemplateWithFullScan() + tpl.MaxTokens = 100000 + tpl.MaxToolRequestTimes = 1 + + a := NewAgent(Args{ + Template: tpl, + LLMClient: client, + Model: "test", + CommentCollector: tool.NewCommentCollector(), + Tools: tool.NewRegistry(), + MaxConcurrency: 1, + SkipPlan: true, + SkipDedup: true, + SkipSummary: true, + Session: session.New(t.TempDir(), "main", "test", session.SessionOptions{ + ReviewMode: session.ReviewModeFullScan, + }), + }) + a.items = []model.ScanItem{{Path: "a.go", Content: "x", LineCount: 1}} + a.currentDate = "2026-06-26" + a.args.Tools.Freeze() + + _, err := a.dispatchSubtasks(context.Background()) + if err == nil || !strings.Contains(err.Error(), "all 1 file scan(s) failed") { + t.Fatalf("expected all-failed error, got %v", err) + } + warnings := a.Warnings() + if len(warnings) != 1 || warnings[0].Type != "scan_subtask_error" || + !strings.Contains(warnings[0].Message, "main_task did not complete") { + t.Fatalf("warnings = %+v, want one incomplete scan subtask error", warnings) + } +} + func TestPhaseEnabled(t *testing.T) { tpl := makeTemplateWithFullScan() a := newAgentForTest(t, tpl) diff --git a/internal/tool/response_message.go b/internal/tool/response_message.go index 2e1f83c6..e4ae7ca8 100644 --- a/internal/tool/response_message.go +++ b/internal/tool/response_message.go @@ -7,15 +7,19 @@ type ToolCallResult struct { Result string // output from the tool } -// TaskCheckpoint signals completion or carries data back to the LLM. +// TaskCheckpoint signals terminal completion or failure, or carries data back to the LLM. type TaskCheckpoint struct { Data string Completed bool + Failed bool } // Complete returns a checkpoint signaling task completion. func Complete() TaskCheckpoint { return TaskCheckpoint{Completed: true} } +// Fail returns a checkpoint signaling terminal task failure. +func Fail(data string) TaskCheckpoint { return TaskCheckpoint{Data: data, Failed: true} } + // Of returns a checkpoint with data. func Of(data string) TaskCheckpoint { return TaskCheckpoint{Data: data, Completed: false} } diff --git a/internal/tool/response_message_test.go b/internal/tool/response_message_test.go index 0fa89d65..2ad6c2aa 100644 --- a/internal/tool/response_message_test.go +++ b/internal/tool/response_message_test.go @@ -7,16 +7,35 @@ func TestComplete(t *testing.T) { if !cp.Completed { t.Error("Complete() should set Completed=true") } + if cp.Failed { + t.Error("Complete() should set Failed=false") + } if cp.Data != "" { t.Errorf("Complete() Data = %q, want empty", cp.Data) } } +func TestFail(t *testing.T) { + cp := Fail("task failed") + if cp.Completed { + t.Error("Fail() should set Completed=false") + } + if !cp.Failed { + t.Error("Fail() should set Failed=true") + } + if cp.Data != "task failed" { + t.Errorf("Fail() Data = %q, want %q", cp.Data, "task failed") + } +} + func TestOf(t *testing.T) { cp := Of("hello") if cp.Completed { t.Error("Of() should set Completed=false") } + if cp.Failed { + t.Error("Of() should set Failed=false") + } if cp.Data != "hello" { t.Errorf("Of() Data = %q, want %q", cp.Data, "hello") } diff --git a/internal/viewer/store.go b/internal/viewer/store.go index dd60fe0a..9955279a 100644 --- a/internal/viewer/store.go +++ b/internal/viewer/store.go @@ -386,7 +386,7 @@ func LoadSession(root, encodedRepo, sessionID string) (*ViewSession, error) { args, _ := tm["arguments"].(string) info := ToolCallInfo{Name: name, Arguments: args} if name == "task_done" { - info.Ok = true + info.Ok = taskDoneSucceeded(args) } card.ToolCalls = append(card.ToolCalls, info) } @@ -414,6 +414,7 @@ func LoadSession(root, encodedRepo, sessionID string) (*ViewSession, error) { } case "tool_call": + toolName, _ := rec["tool_name"].(string) result, _ := rec["result"].(string) okVal := true if b, hasOk := rec["ok"].(bool); hasOk { @@ -432,7 +433,10 @@ func LoadSession(root, encodedRepo, sessionID string) (*ViewSession, error) { if len(cards) > 0 { card := cards[len(cards)-1] for ti := range card.ToolCalls { - if card.ToolCalls[ti].Result == "" && !card.ToolCalls[ti].Ok { + // Older session records omitted tool_name, so retain + // positional matching only for those records. + if (toolName == "" || card.ToolCalls[ti].Name == toolName) && + card.ToolCalls[ti].Result == "" && !card.ToolCalls[ti].Ok { card.ToolCalls[ti].Result = result card.ToolCalls[ti].Ok = okVal card.ToolCalls[ti].DurationMs = durationMs @@ -494,3 +498,16 @@ func LoadSession(root, encodedRepo, sessionID string) (*ViewSession, error) { vs.Summary.SessionID = sessionID return vs, scanner.Err() } + +func taskDoneSucceeded(arguments string) bool { + var args map[string]any + if err := json.Unmarshal([]byte(arguments), &args); err != nil { + return false + } + state, hasState := args["state"] + if !hasState { + return true + } + stateString, ok := state.(string) + return ok && stateString == "DONE" +} diff --git a/internal/viewer/store_load_test.go b/internal/viewer/store_load_test.go index a4da1dbf..ef18bbb3 100644 --- a/internal/viewer/store_load_test.go +++ b/internal/viewer/store_load_test.go @@ -162,6 +162,58 @@ func TestLoadSession_FullParse(t *testing.T) { } } +func TestLoadSession_TaskDoneStates(t *testing.T) { + root := t.TempDir() + repoDir := filepath.Join(root, "repo") + if err := os.MkdirAll(repoDir, 0755); err != nil { + t.Fatal(err) + } + + writeJSONL(t, filepath.Join(repoDir, "terminal-states.jsonl"), + `{"type":"llm_request","filePath":"main.go","taskType":"main_task","request_no":1,"messages":[]}`, + `{"type":"llm_response","filePath":"main.go","taskType":"main_task","tool_calls":[{"name":"task_done","arguments":"{}"}]}`, + `{"type":"llm_request","filePath":"main.go","taskType":"main_task","request_no":2,"messages":[]}`, + `{"type":"llm_response","filePath":"main.go","taskType":"main_task","tool_calls":[{"name":"task_done","arguments":"{\"state\":\"DONE\"}"}]}`, + `{"type":"llm_request","filePath":"main.go","taskType":"main_task","request_no":3,"messages":[]}`, + `{"type":"llm_response","filePath":"main.go","taskType":"main_task","tool_calls":[{"name":"task_done","arguments":"{\"state\":\"FAILED\"}"}]}`, + `{"type":"llm_request","filePath":"main.go","taskType":"main_task","request_no":4,"messages":[]}`, + `{"type":"llm_response","filePath":"main.go","taskType":"main_task","tool_calls":[{"name":"task_done","arguments":"{\"state\":\"\"}"}]}`, + `{"type":"llm_request","filePath":"main.go","taskType":"main_task","request_no":5,"messages":[]}`, + `{"type":"llm_response","filePath":"main.go","taskType":"main_task","tool_calls":[{"name":"task_done","arguments":"{\"state\":\"\"}"},{"name":"file_read","arguments":"{\"path\":\"main.go\"}"}]}`, + `{"type":"tool_call","filePath":"main.go","taskType":"main_task","tool_name":"file_read","result":"package main","ok":true,"duration_ms":20}`, + ) + + vs, err := LoadSession(root, "repo", "terminal-states") + if err != nil { + t.Fatal(err) + } + if len(vs.Files) != 1 { + t.Fatalf("files = %d, want 1", len(vs.Files)) + } + cards := vs.Files[0].Tasks[MainTask] + if len(cards) != 5 { + t.Fatalf("main_task cards = %d, want 5", len(cards)) + } + wantOK := []bool{true, true, false, false} + for i, want := range wantOK { + if len(cards[i].ToolCalls) != 1 { + t.Fatalf("card %d tool calls = %d, want 1", i, len(cards[i].ToolCalls)) + } + if got := cards[i].ToolCalls[0].Ok; got != want { + t.Errorf("card %d task_done Ok = %v, want %v", i, got, want) + } + } + if len(cards[4].ToolCalls) != 2 { + t.Fatalf("card 4 tool calls = %d, want 2", len(cards[4].ToolCalls)) + } + if cards[4].ToolCalls[0].Ok || cards[4].ToolCalls[0].Result != "" { + t.Errorf("invalid task_done received another tool's result: %+v", cards[4].ToolCalls[0]) + } + if !cards[4].ToolCalls[1].Ok || cards[4].ToolCalls[1].Result != "package main" { + t.Errorf("file_read result was not matched by name: %+v", cards[4].ToolCalls[1]) + } +} + func TestLoadSession_MissingFile(t *testing.T) { root := t.TempDir() repoDir := filepath.Join(root, "repo")