fix: honor per-file review terminal states

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))

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 {

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
 }

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{

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)

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"}`),

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

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)

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} }
 

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")
 	}