-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: allow conversational responses in Ask mode without forcing tool use #6582
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1325,8 +1325,16 @@ export class Task extends EventEmitter<TaskEvents> { | |||||||||
| // the user hits max requests and denies resetting the count. | ||||||||||
| break | ||||||||||
| } else { | ||||||||||
| nextUserContent = [{ type: "text", text: formatResponse.noToolsUsed() }] | ||||||||||
| this.consecutiveMistakeCount++ | ||||||||||
| // Check if we're in Ask mode before forcing tool use | ||||||||||
| const currentMode = await this.getTaskMode() | ||||||||||
| if (currentMode === "ask") { | ||||||||||
| // In Ask mode, allow the conversation to end without forcing tool use | ||||||||||
| break | ||||||||||
| } else { | ||||||||||
| // For other modes, maintain the existing behavior | ||||||||||
| nextUserContent = [{ type: "text", text: formatResponse.noToolsUsed() }] | ||||||||||
| this.consecutiveMistakeCount++ | ||||||||||
| } | ||||||||||
| } | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
@@ -1740,8 +1748,17 @@ export class Task extends EventEmitter<TaskEvents> { | |||||||||
| const didToolUse = this.assistantMessageContent.some((block) => block.type === "tool_use") | ||||||||||
|
|
||||||||||
| if (!didToolUse) { | ||||||||||
| this.userMessageContent.push({ type: "text", text: formatResponse.noToolsUsed() }) | ||||||||||
| this.consecutiveMistakeCount++ | ||||||||||
| // Check if we're in Ask mode - if so, allow conversational responses without tools | ||||||||||
| const currentMode = await this.getTaskMode() | ||||||||||
| if (currentMode === "ask") { | ||||||||||
| // In Ask mode, we don't force tool use for conversational responses | ||||||||||
| // This prevents the repetitive response issue | ||||||||||
| return true // End the loop successfully | ||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The return value could benefit from a clarifying comment:
Suggested change
|
||||||||||
| } else { | ||||||||||
| // For other modes, maintain the existing behavior | ||||||||||
| this.userMessageContent.push({ type: "text", text: formatResponse.noToolsUsed() }) | ||||||||||
| this.consecutiveMistakeCount++ | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| const recDidEndLoop = await this.recursivelyMakeClineRequests(this.userMessageContent) | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -17,6 +17,7 @@ import { processUserContentMentions } from "../../mentions/processUserContentMen | |||
| import { MultiSearchReplaceDiffStrategy } from "../../diff/strategies/multi-search-replace" | ||||
| import { MultiFileSearchReplaceDiffStrategy } from "../../diff/strategies/multi-file-search-replace" | ||||
| import { EXPERIMENT_IDS } from "../../../shared/experiments" | ||||
| import { formatResponse } from "../../prompts/responses" | ||||
|
|
||||
| // Mock delay before any imports that might use it | ||||
| vi.mock("delay", () => ({ | ||||
|
|
@@ -1493,5 +1494,142 @@ describe("Cline", () => { | |||
| expect(noModelTask.apiConfiguration.apiProvider).toBe("openai") | ||||
| }) | ||||
| }) | ||||
|
|
||||
| describe("Ask mode conversational responses", () => { | ||||
| it("should allow conversational responses in Ask mode without forcing tool use", async () => { | ||||
| // Mock provider with Ask mode | ||||
| const askModeProvider = { | ||||
| ...mockProvider, | ||||
| getState: vi.fn().mockResolvedValue({ | ||||
| mode: "ask", | ||||
| }), | ||||
| } | ||||
|
|
||||
| // Create task with history item that has ask mode | ||||
| const askTask = new Task({ | ||||
| provider: askModeProvider, | ||||
| apiConfiguration: mockApiConfig, | ||||
| historyItem: { | ||||
| id: "test-ask-task", | ||||
| number: 1, | ||||
| ts: Date.now(), | ||||
| task: "What is TypeScript?", | ||||
| tokensIn: 0, | ||||
| tokensOut: 0, | ||||
| cacheWrites: 0, | ||||
| cacheReads: 0, | ||||
| totalCost: 0, | ||||
| mode: "ask", // This sets the task mode | ||||
| }, | ||||
| startTask: false, | ||||
| }) | ||||
|
|
||||
| // Mock the API stream response without tool use | ||||
| const mockStream = { | ||||
| async *[Symbol.asyncIterator]() { | ||||
| yield { type: "text", text: "TypeScript is a typed superset of JavaScript..." } | ||||
| }, | ||||
| async next() { | ||||
| return { | ||||
| done: true, | ||||
| value: { type: "text", text: "TypeScript is a typed superset of JavaScript..." }, | ||||
| } | ||||
| }, | ||||
| async return() { | ||||
| return { done: true, value: undefined } | ||||
| }, | ||||
| async throw(e: any) { | ||||
| throw e | ||||
| }, | ||||
| [Symbol.asyncDispose]: async () => {}, | ||||
| } as AsyncGenerator<ApiStreamChunk> | ||||
|
|
||||
| vi.spyOn(askTask.api, "createMessage").mockReturnValue(mockStream) | ||||
|
|
||||
| // Mock assistant message content without tool use | ||||
| askTask.assistantMessageContent = [ | ||||
| { | ||||
| type: "text", | ||||
| content: "TypeScript is a typed superset of JavaScript...", | ||||
| partial: false, | ||||
| }, | ||||
| ] | ||||
|
|
||||
| // Mock userMessageContentReady | ||||
| askTask.userMessageContentReady = true | ||||
|
|
||||
| // Spy on recursivelyMakeClineRequests to check if it returns true (ends loop) | ||||
| const recursiveSpy = vi.spyOn(askTask, "recursivelyMakeClineRequests") | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The spy on 'recursivelyMakeClineRequests' is created but not asserted anywhere. Consider either removing it or adding an assertion (e.g. verifying call count or arguments) to increase test clarity.
Suggested change
|
||||
|
|
||||
| // Execute the request | ||||
| const result = await askTask.recursivelyMakeClineRequests([ | ||||
| { type: "text", text: "What is TypeScript?" }, | ||||
| ]) | ||||
|
|
||||
| // Verify that the loop ends successfully without forcing tool use | ||||
| expect(result).toBe(true) | ||||
|
|
||||
| // Verify that no "noToolsUsed" error was added | ||||
| expect(askTask.userMessageContent).not.toContainEqual( | ||||
| expect.objectContaining({ | ||||
| type: "text", | ||||
| text: expect.stringContaining("You did not use a tool"), | ||||
| }), | ||||
| ) | ||||
|
|
||||
| // Verify consecutive mistake count was not incremented | ||||
| expect(askTask.consecutiveMistakeCount).toBe(0) | ||||
| }) | ||||
|
|
||||
| it("should still enforce tool use in non-Ask modes", async () => { | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the test for non-Ask modes, the logic is manually duplicated (checking current mode and updating nextUserContent) rather than invoking the actual task loop. Consider refactoring the test to exercise the real behavior to reduce test fragility. |
||||
| // Test the actual logic in initiateTaskLoop | ||||
| const testUserContent = [{ type: "text" as const, text: "test" }] | ||||
|
|
||||
| // Test code mode | ||||
| const codeTask = new Task({ | ||||
| provider: mockProvider, | ||||
| apiConfiguration: mockApiConfig, | ||||
| historyItem: { | ||||
| id: "test-code-task", | ||||
| number: 2, | ||||
| ts: Date.now(), | ||||
| task: "Write a function", | ||||
| tokensIn: 0, | ||||
| tokensOut: 0, | ||||
| cacheWrites: 0, | ||||
| cacheReads: 0, | ||||
| totalCost: 0, | ||||
| mode: "code", | ||||
| }, | ||||
| startTask: false, | ||||
| }) | ||||
|
|
||||
| // Directly test the logic from initiateTaskLoop | ||||
| let nextUserContent = testUserContent | ||||
| const didEndLoop = false // Simulating recursivelyMakeClineRequests returning false | ||||
|
|
||||
| if (!didEndLoop) { | ||||
| // This is the actual code from initiateTaskLoop | ||||
| const currentMode = await codeTask.getTaskMode() | ||||
| if (currentMode === "ask") { | ||||
| // In Ask mode, allow the conversation to end without forcing tool use | ||||
| // This would break the loop | ||||
| } else { | ||||
| // For other modes, maintain the existing behavior | ||||
| nextUserContent = [{ type: "text" as const, text: formatResponse.noToolsUsed() }] | ||||
| codeTask.consecutiveMistakeCount++ | ||||
| } | ||||
| } | ||||
|
|
||||
| // Verify the behavior for code mode | ||||
| expect(nextUserContent).toContainEqual( | ||||
| expect.objectContaining({ | ||||
| type: "text", | ||||
| text: expect.stringContaining("You did not use a tool"), | ||||
| }), | ||||
| ) | ||||
| expect(codeTask.consecutiveMistakeCount).toBe(1) | ||||
| }) | ||||
| }) | ||||
| }) | ||||
| }) | ||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a comment here explaining why Ask mode is special-cased. Something like:
This would help future maintainers understand the design decision.