-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: add config parameter to new_task tool (#6839) #6840
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 |
|---|---|---|
|
|
@@ -26,6 +26,7 @@ const mockInitClineWithTask = vi.fn<() => Promise<MockClineInstance>>().mockReso | |
| const mockEmit = vi.fn() | ||
| const mockRecordToolError = vi.fn() | ||
| const mockSayAndCreateMissingParamError = vi.fn() | ||
| const mockHasConfig = vi.fn() | ||
|
|
||
| // Mock the Cline instance and its methods/properties | ||
| const mockCline = { | ||
|
|
@@ -41,6 +42,9 @@ const mockCline = { | |
| getState: vi.fn(() => ({ customModes: [], mode: "ask" })), | ||
| handleModeSwitch: vi.fn(), | ||
| initClineWithTask: mockInitClineWithTask, | ||
| providerSettingsManager: { | ||
| hasConfig: mockHasConfig, | ||
| }, | ||
| })), | ||
| }, | ||
| } | ||
|
|
@@ -63,6 +67,7 @@ describe("newTaskTool", () => { | |
| }) // Default valid mode | ||
| mockCline.consecutiveMistakeCount = 0 | ||
| mockCline.isPaused = false | ||
| mockHasConfig.mockResolvedValue(true) // Default to config exists | ||
| }) | ||
|
|
||
| it("should correctly un-escape \\\\@ to \\@ in the message passed to the new task", async () => { | ||
|
|
@@ -93,6 +98,7 @@ describe("newTaskTool", () => { | |
| "Review this: \\@file1.txt and also \\\\\\@file2.txt", // Unit Test Expectation: \\@ -> \@, \\\\@ -> \\\\@ | ||
| undefined, | ||
| mockCline, | ||
| undefined, // No config parameter for this test | ||
| ) | ||
|
|
||
| // Verify side effects | ||
|
|
@@ -126,6 +132,7 @@ describe("newTaskTool", () => { | |
| "This is already unescaped: \\@file1.txt", // Expected: \@ remains \@ | ||
| undefined, | ||
| mockCline, | ||
| undefined, // No config parameter for this test | ||
| ) | ||
| }) | ||
|
|
||
|
|
@@ -153,6 +160,7 @@ describe("newTaskTool", () => { | |
| "A normal mention @file1.txt", // Expected: @ remains @ | ||
| undefined, | ||
| mockCline, | ||
| undefined, // No config parameter for this test | ||
| ) | ||
| }) | ||
|
|
||
|
|
@@ -180,8 +188,188 @@ describe("newTaskTool", () => { | |
| "Mix: @file0.txt, \\@file1.txt, \\@file2.txt, \\\\\\@file3.txt", // Unit Test Expectation: @->@, \@->\@, \\@->\@, \\\\@->\\\\@ | ||
| undefined, | ||
| mockCline, | ||
| undefined, // No config parameter for this test | ||
| ) | ||
| }) | ||
|
|
||
| // Tests for the new config parameter functionality | ||
| describe("config parameter", () => { | ||
| it("should pass config parameter to initClineWithTask when valid config is provided", async () => { | ||
| const block: ToolUse = { | ||
| type: "tool_use", | ||
| name: "new_task", | ||
| params: { | ||
| mode: "code", | ||
| message: "Test message", | ||
| config: "fast-model", | ||
| }, | ||
| partial: false, | ||
| } | ||
|
|
||
| mockHasConfig.mockResolvedValue(true) | ||
|
|
||
| await newTaskTool( | ||
| mockCline as any, | ||
| block, | ||
| mockAskApproval, | ||
| mockHandleError, | ||
| mockPushToolResult, | ||
| mockRemoveClosingTag, | ||
| ) | ||
|
|
||
| // Verify hasConfig was called to validate the config | ||
| expect(mockHasConfig).toHaveBeenCalledWith("fast-model") | ||
|
|
||
| // Verify initClineWithTask was called with the config parameter | ||
| expect(mockInitClineWithTask).toHaveBeenCalledWith( | ||
| "Test message", | ||
| undefined, | ||
| mockCline, | ||
| "fast-model", // The config parameter should be passed | ||
| ) | ||
|
|
||
| // Verify success message includes config name | ||
| expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("configuration 'fast-model'")) | ||
| }) | ||
|
|
||
| it("should continue without config when invalid config is provided", async () => { | ||
| const block: ToolUse = { | ||
| type: "tool_use", | ||
| name: "new_task", | ||
| params: { | ||
| mode: "code", | ||
| message: "Test message", | ||
| config: "non-existent-config", | ||
| }, | ||
| partial: false, | ||
| } | ||
|
|
||
| mockHasConfig.mockResolvedValue(false) | ||
|
|
||
| await newTaskTool( | ||
| mockCline as any, | ||
| block, | ||
| mockAskApproval, | ||
| mockHandleError, | ||
| mockPushToolResult, | ||
| mockRemoveClosingTag, | ||
| ) | ||
|
|
||
| // Verify hasConfig was called | ||
| expect(mockHasConfig).toHaveBeenCalledWith("non-existent-config") | ||
|
|
||
| // Verify error message was pushed | ||
| expect(mockPushToolResult).toHaveBeenCalledWith( | ||
| expect.stringContaining("Configuration profile 'non-existent-config' not found"), | ||
| ) | ||
|
|
||
| // Verify initClineWithTask was called without the config parameter | ||
| expect(mockInitClineWithTask).toHaveBeenCalledWith( | ||
| "Test message", | ||
| undefined, | ||
| mockCline, | ||
| undefined, // No config should be passed | ||
| ) | ||
|
|
||
| // Verify success message doesn't include config | ||
| expect(mockPushToolResult).toHaveBeenCalledWith( | ||
| expect.stringContaining("Successfully created new task in Code Mode mode with message: Test message"), | ||
| ) | ||
| }) | ||
|
|
||
| it("should work without config parameter (backward compatibility)", async () => { | ||
| const block: ToolUse = { | ||
| type: "tool_use", | ||
| name: "new_task", | ||
| params: { | ||
| mode: "code", | ||
| message: "Test message", | ||
| // No config parameter | ||
| }, | ||
| partial: false, | ||
| } | ||
|
|
||
| await newTaskTool( | ||
| mockCline as any, | ||
| block, | ||
| mockAskApproval, | ||
| mockHandleError, | ||
| mockPushToolResult, | ||
| mockRemoveClosingTag, | ||
| ) | ||
|
|
||
| // Verify hasConfig was NOT called | ||
| expect(mockHasConfig).not.toHaveBeenCalled() | ||
|
|
||
| // Verify initClineWithTask was called without config | ||
| expect(mockInitClineWithTask).toHaveBeenCalledWith( | ||
| "Test message", | ||
| undefined, | ||
| mockCline, | ||
| undefined, // No config parameter | ||
| ) | ||
|
|
||
| // Verify success message doesn't include config | ||
| expect(mockPushToolResult).toHaveBeenCalledWith( | ||
| expect.stringContaining("Successfully created new task in Code Mode mode with message: Test message"), | ||
| ) | ||
| }) | ||
|
|
||
| it("should include config in approval message when config is provided", async () => { | ||
| const block: ToolUse = { | ||
| type: "tool_use", | ||
| name: "new_task", | ||
| params: { | ||
| mode: "code", | ||
| message: "Test message", | ||
| config: "accurate-model", | ||
| }, | ||
| partial: false, | ||
| } | ||
|
|
||
| mockHasConfig.mockResolvedValue(true) | ||
|
|
||
| await newTaskTool( | ||
| mockCline as any, | ||
| block, | ||
| mockAskApproval, | ||
| mockHandleError, | ||
| mockPushToolResult, | ||
| mockRemoveClosingTag, | ||
| ) | ||
|
|
||
| // Verify askApproval was called with a message containing the config | ||
| expect(mockAskApproval).toHaveBeenCalledWith("tool", expect.stringContaining('"config":"accurate-model"')) | ||
| }) | ||
|
|
||
| it("should handle partial messages with config parameter", async () => { | ||
| const block: ToolUse = { | ||
| type: "tool_use", | ||
| name: "new_task", | ||
| params: { | ||
| mode: "code", | ||
| message: "Test message", | ||
| config: "fast-model", | ||
| }, | ||
| partial: true, | ||
| } | ||
|
|
||
| await newTaskTool( | ||
| mockCline as any, | ||
| block, | ||
| mockAskApproval, | ||
| mockHandleError, | ||
| mockPushToolResult, | ||
| mockRemoveClosingTag, | ||
| ) | ||
|
|
||
| // Verify ask was called with partial message including config | ||
| expect(mockCline.ask).toHaveBeenCalledWith("tool", expect.stringContaining('"config":"fast-model"'), true) | ||
|
|
||
| // Verify initClineWithTask was NOT called for partial message | ||
| expect(mockInitClineWithTask).not.toHaveBeenCalled() | ||
| }) | ||
| }) | ||
|
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. Great test coverage for the main functionality! Consider adding edge case tests for:
|
||
|
|
||
| // Add more tests for error handling (missing params, invalid mode, approval denied) if needed | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,13 +18,15 @@ export async function newTaskTool( | |
| ) { | ||
| const mode: string | undefined = block.params.mode | ||
| const message: string | undefined = block.params.message | ||
| const config: string | undefined = block.params.config | ||
|
|
||
| try { | ||
| if (block.partial) { | ||
| const partialMessage = JSON.stringify({ | ||
| tool: "newTask", | ||
| mode: removeClosingTag("mode", mode), | ||
| content: removeClosingTag("message", message), | ||
| config: config ? removeClosingTag("config", config) : undefined, | ||
| }) | ||
|
|
||
| await cline.ask("tool", partialMessage, block.partial).catch(() => {}) | ||
|
|
@@ -57,10 +59,33 @@ export async function newTaskTool( | |
| return | ||
| } | ||
|
|
||
| // If a config was specified, verify it exists | ||
| let configName: string | undefined | ||
| if (config) { | ||
| const provider = cline.providerRef.deref() | ||
| if (!provider) { | ||
| return | ||
| } | ||
|
|
||
| // Check if the specified config exists | ||
| const hasConfig = await provider.providerSettingsManager.hasConfig(config) | ||
| if (!hasConfig) { | ||
| pushToolResult( | ||
| formatResponse.toolError( | ||
| `Configuration profile '${config}' not found. Using default configuration.`, | ||
| ), | ||
| ) | ||
| // Continue without the config rather than failing completely | ||
| } else { | ||
| configName = config | ||
| } | ||
| } | ||
|
|
||
| const toolMessage = JSON.stringify({ | ||
| tool: "newTask", | ||
| mode: targetMode.name, | ||
| content: message, | ||
| ...(configName && { config: configName }), | ||
| }) | ||
|
|
||
| const didApprove = await askApproval("tool", toolMessage) | ||
|
|
@@ -83,7 +108,7 @@ export async function newTaskTool( | |
| cline.pausedModeSlug = (await provider.getState()).mode ?? defaultModeSlug | ||
|
|
||
| // Create new task instance first (this preserves parent's current mode in its history) | ||
| const newCline = await provider.initClineWithTask(unescapedMessage, undefined, cline) | ||
| const newCline = await provider.initClineWithTask(unescapedMessage, undefined, cline, configName) | ||
| if (!newCline) { | ||
| pushToolResult(t("tools:newTask.errors.policy_restriction")) | ||
| return | ||
|
|
@@ -97,7 +122,10 @@ export async function newTaskTool( | |
|
|
||
| cline.emit(RooCodeEventName.TaskSpawned, newCline.taskId) | ||
|
|
||
| pushToolResult(`Successfully created new task in ${targetMode.name} mode with message: ${unescapedMessage}`) | ||
| const successMessage = configName | ||
|
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. User-facing success messages are constructed as plain strings. Consider using the translation function (e.g., t()) for these messages to support internationalization. This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX. |
||
| ? `Successfully created new task in ${targetMode.name} mode with configuration '${configName}' and message: ${unescapedMessage}` | ||
| : `Successfully created new task in ${targetMode.name} mode with message: ${unescapedMessage}` | ||
| pushToolResult(successMessage) | ||
|
|
||
| // Set the isPaused flag to true so the parent | ||
| // task can wait for the sub-task to finish. | ||
|
|
||
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.
The tool description could mention that invalid configs fall back to default gracefully. This would set proper expectations for users.