diff --git a/packages/types/src/telemetry.ts b/packages/types/src/telemetry.ts index 68ed38fe32..f7fa48ba57 100644 --- a/packages/types/src/telemetry.ts +++ b/packages/types/src/telemetry.ts @@ -483,7 +483,7 @@ export function extractApiProviderErrorProperties(error: ApiProviderError): Reco /** * Reason why the consecutive mistake limit was reached. */ -export type ConsecutiveMistakeReason = "no_tools_used" | "tool_repetition" | "unknown" +export type ConsecutiveMistakeReason = "no_tools_used" | "tool_repetition" | "no_assistant_messages" | "unknown" /** * Error class for "Roo is having trouble" consecutive mistake scenarios. diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 9d27c4b90b..0c69bf69a7 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -51,6 +51,7 @@ import { MAX_CHECKPOINT_TIMEOUT_SECONDS, MIN_CHECKPOINT_TIMEOUT_SECONDS, ConsecutiveMistakeError, + type ConsecutiveMistakeReason, MAX_MCP_TOOLS_THRESHOLD, countEnabledMcpTools, } from "@roo-code/types" @@ -325,6 +326,7 @@ export class Task extends EventEmitter implements TaskLike { // Tool Use consecutiveMistakeCount: number = 0 + consecutiveMistakeReason: ConsecutiveMistakeReason = "unknown" consecutiveMistakeLimit: number consecutiveMistakeCountForApplyDiff: Map = new Map() consecutiveMistakeCountForEditFile: Map = new Map() @@ -2720,8 +2722,6 @@ export class Task extends EventEmitter implements TaskLike { if (this.consecutiveMistakeLimit > 0 && this.consecutiveMistakeCount >= this.consecutiveMistakeLimit) { // Track consecutive mistake errors in telemetry via event and PostHog exception tracking. - // The reason is "no_tools_used" because this limit is reached via initiateTaskLoop - // which increments consecutiveMistakeCount when the model doesn't use any tools. TelemetryService.instance.captureConsecutiveMistakeError(this.taskId) TelemetryService.instance.captureException( new ConsecutiveMistakeError( @@ -2729,7 +2729,7 @@ export class Task extends EventEmitter implements TaskLike { this.taskId, this.consecutiveMistakeCount, this.consecutiveMistakeLimit, - "no_tools_used", + this.consecutiveMistakeReason, this.apiConfiguration.apiProvider, getModelId(this.apiConfiguration), ), @@ -2752,6 +2752,7 @@ export class Task extends EventEmitter implements TaskLike { } this.consecutiveMistakeCount = 0 + this.consecutiveMistakeReason = "unknown" } // Getting verbose details is an expensive operation, it uses ripgrep to @@ -3691,15 +3692,7 @@ export class Task extends EventEmitter implements TaskLike { ) if (!didToolUse) { - // Increment consecutive no-tool-use counter - this.consecutiveNoToolUseCount++ - - // Only show error and count toward mistake limit after 2 consecutive failures - if (this.consecutiveNoToolUseCount >= 2) { - await this.say("error", "MODEL_NO_TOOLS_USED") - // Only count toward mistake limit after second consecutive failure - this.consecutiveMistakeCount++ - } + await this.onNoToolUse() // Use the task's locked protocol for consistent behavior this.userMessageContent.push({ @@ -3729,14 +3722,7 @@ export class Task extends EventEmitter implements TaskLike { // or tool_use content blocks from API which we should assume is // an error. - // Increment consecutive no-assistant-messages counter - this.consecutiveNoAssistantMessagesCount++ - - // Only show error and count toward mistake limit after 2 consecutive failures - // This provides a "grace retry" - first failure retries silently - if (this.consecutiveNoAssistantMessagesCount >= 2) { - await this.say("error", "MODEL_NO_ASSISTANT_MESSAGES") - } + await this.onEmptyAssistantOutput() // IMPORTANT: We already added the user message to // apiConversationHistory at line 1876. Since the assistant failed to respond, @@ -4488,6 +4474,30 @@ export class Task extends EventEmitter implements TaskLike { yield* iterator } + // Increment counters and show error when the model returns no text and no tool calls. + // The first empty response is a silent "grace retry"; from the second onward we show + // an error and feed into the mistake-limit machinery so auto-retry is bounded. + private async onEmptyAssistantOutput(): Promise { + this.consecutiveNoAssistantMessagesCount++ + if (this.consecutiveNoAssistantMessagesCount >= 2) { + await this.say("error", "MODEL_NO_ASSISTANT_MESSAGES") + this.consecutiveMistakeReason = "no_assistant_messages" + this.consecutiveMistakeCount++ + } + } + + // Increment counters and show error when the model returns text but no tool calls. + // Same grace-retry pattern: first occurrence is silent, second onward triggers error + // and feeds into the mistake-limit machinery. + private async onNoToolUse(): Promise { + this.consecutiveNoToolUseCount++ + if (this.consecutiveNoToolUseCount >= 2) { + await this.say("error", "MODEL_NO_TOOLS_USED") + this.consecutiveMistakeReason = "no_tools_used" + this.consecutiveMistakeCount++ + } + } + // Shared exponential backoff for retries (first-chunk and mid-stream) private async backoffAndAnnounce(retryAttempt: number, error: any): Promise { try { diff --git a/src/core/task/__tests__/grace-retry-errors.spec.ts b/src/core/task/__tests__/grace-retry-errors.spec.ts index 283b402f69..d230f97ef9 100644 --- a/src/core/task/__tests__/grace-retry-errors.spec.ts +++ b/src/core/task/__tests__/grace-retry-errors.spec.ts @@ -277,7 +277,7 @@ describe("Grace Retry Error Handling", () => { }) }) - describe("Grace Retry Pattern", () => { + describe("Grace Retry Pattern (via production helper)", () => { it("should not show error on first failure (grace retry)", async () => { const task = new Task({ provider: mockProvider, @@ -288,17 +288,10 @@ describe("Grace Retry Error Handling", () => { const saySpy = vi.spyOn(task, "say").mockResolvedValue(undefined) - // Simulate first empty response - should NOT show error - task.consecutiveNoAssistantMessagesCount = 0 - task.consecutiveNoAssistantMessagesCount++ - expect(task.consecutiveNoAssistantMessagesCount).toBe(1) - - // First failure: grace retry (silent) - if (task.consecutiveNoAssistantMessagesCount >= 2) { - await task.say("error", "MODEL_NO_ASSISTANT_MESSAGES") - } + // First call to the production helper — grace retry (silent) + await (task as any).onEmptyAssistantOutput() - // Verify error was NOT called (grace retry on first failure) + expect(task.consecutiveNoAssistantMessagesCount).toBe(1) expect(saySpy).not.toHaveBeenCalledWith("error", "MODEL_NO_ASSISTANT_MESSAGES") }) @@ -312,17 +305,11 @@ describe("Grace Retry Error Handling", () => { const saySpy = vi.spyOn(task, "say").mockResolvedValue(undefined) - // Simulate second consecutive empty response - task.consecutiveNoAssistantMessagesCount = 1 - task.consecutiveNoAssistantMessagesCount++ - expect(task.consecutiveNoAssistantMessagesCount).toBe(2) - - // Second failure: should show error - if (task.consecutiveNoAssistantMessagesCount >= 2) { - await task.say("error", "MODEL_NO_ASSISTANT_MESSAGES") - } + // Two consecutive calls to the production helper + await (task as any).onEmptyAssistantOutput() + await (task as any).onEmptyAssistantOutput() - // Verify error was called (after 2 consecutive failures) + expect(task.consecutiveNoAssistantMessagesCount).toBe(2) expect(saySpy).toHaveBeenCalledWith("error", "MODEL_NO_ASSISTANT_MESSAGES") }) @@ -336,17 +323,14 @@ describe("Grace Retry Error Handling", () => { const saySpy = vi.spyOn(task, "say").mockResolvedValue(undefined) - // Simulate third consecutive empty response - task.consecutiveNoAssistantMessagesCount = 2 - task.consecutiveNoAssistantMessagesCount++ - expect(task.consecutiveNoAssistantMessagesCount).toBe(3) + // Three consecutive calls + await (task as any).onEmptyAssistantOutput() + await (task as any).onEmptyAssistantOutput() + await (task as any).onEmptyAssistantOutput() - // Third failure: should also show error - if (task.consecutiveNoAssistantMessagesCount >= 2) { - await task.say("error", "MODEL_NO_ASSISTANT_MESSAGES") - } - - // Verify error was called + expect(task.consecutiveNoAssistantMessagesCount).toBe(3) + // Error called on 2nd and 3rd invocations + expect(saySpy).toHaveBeenCalledTimes(2) expect(saySpy).toHaveBeenCalledWith("error", "MODEL_NO_ASSISTANT_MESSAGES") }) }) @@ -408,12 +392,9 @@ describe("Grace Retry Error Handling", () => { const saySpy = vi.spyOn(task, "say").mockResolvedValue(undefined) - // Simulate the error condition (2 consecutive failures) - task.consecutiveNoAssistantMessagesCount = 2 - - if (task.consecutiveNoAssistantMessagesCount >= 2) { - await task.say("error", "MODEL_NO_ASSISTANT_MESSAGES") - } + // Call production helper twice to trigger error path + await (task as any).onEmptyAssistantOutput() + await (task as any).onEmptyAssistantOutput() // Verify the exact marker is used expect(saySpy).toHaveBeenCalledWith("error", "MODEL_NO_ASSISTANT_MESSAGES") @@ -441,4 +422,100 @@ describe("Grace Retry Error Handling", () => { expect(task.consecutiveNoToolUseCount).toBe(3) }) }) + + describe("Empty-response mistake limit bounding", () => { + it("should increment consecutiveMistakeCount after 2 consecutive empty responses", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + vi.spyOn(task, "say").mockResolvedValue(undefined) + expect(task.consecutiveMistakeCount).toBe(0) + + // Two calls to the production helper — second triggers mistake count + await (task as any).onEmptyAssistantOutput() + expect(task.consecutiveMistakeCount).toBe(0) // grace retry: no increment yet + + await (task as any).onEmptyAssistantOutput() + expect(task.consecutiveMistakeCount).toBe(1) + }) + + it("should NOT increment consecutiveMistakeCount on first empty response (grace retry)", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + const saySpy = vi.spyOn(task, "say").mockResolvedValue(undefined) + + // Single call — grace retry, no error, no mistake count + await (task as any).onEmptyAssistantOutput() + + expect(task.consecutiveMistakeCount).toBe(0) + expect(saySpy).not.toHaveBeenCalled() + }) + + it("should confirm no-tool-use path increments consecutiveMistakeCount independently", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + const saySpy = vi.spyOn(task, "say").mockResolvedValue(undefined) + expect(task.consecutiveMistakeCount).toBe(0) + + // Two calls to the no-tool-use production helper + await (task as any).onNoToolUse() + await (task as any).onNoToolUse() + + expect(saySpy).toHaveBeenCalledWith("error", "MODEL_NO_TOOLS_USED") + expect(task.consecutiveMistakeCount).toBe(1) + + // Empty-response counter is unaffected + expect(task.consecutiveNoAssistantMessagesCount).toBe(0) + }) + + it("should set consecutiveMistakeReason to no_assistant_messages after empty-output threshold", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + vi.spyOn(task, "say").mockResolvedValue(undefined) + expect(task.consecutiveMistakeReason).toBe("unknown") + + // First call — grace retry, reason stays unknown + await (task as any).onEmptyAssistantOutput() + expect(task.consecutiveMistakeReason).toBe("unknown") + + // Second call — reason set + await (task as any).onEmptyAssistantOutput() + expect(task.consecutiveMistakeReason).toBe("no_assistant_messages") + }) + + it("should set consecutiveMistakeReason to no_tools_used after no-tool-use threshold", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + vi.spyOn(task, "say").mockResolvedValue(undefined) + expect(task.consecutiveMistakeReason).toBe("unknown") + + await (task as any).onNoToolUse() + await (task as any).onNoToolUse() + expect(task.consecutiveMistakeReason).toBe("no_tools_used") + }) + }) })