Skip to content

Commit ba3b9f3

Browse files
hannesrudolphhannesrudolph
committed
fix: resolve newTaskRequireTodos setting not working correctly
- Use dynamic Package.name instead of hardcoded namespace values - Show todos parameter as optional/required based on setting value - Remove hardcoded new_task example from shared tool use section - Update tests to use Package.name pattern The setting now works correctly for both regular and nightly builds without requiring hardcoded namespace values.
1 parent fc70012 commit ba3b9f3

File tree

5 files changed

+75
-71
lines changed

5 files changed

+75
-71
lines changed

‎src/core/prompts/sections/tool-use.ts‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -15,18 +15,5 @@ Tool uses are formatted using XML-style tags. The tool name itself becomes the X
1515
...
1616
</actual_tool_name>
1717
18-
For example, to use the new_task tool:
19-
20-
<new_task>
21-
<mode>code</mode>
22-
<message>Implement a new feature for the application.</message>
23-
<todos>
24-
[ ] Design the feature architecture
25-
[ ] Implement core functionality
26-
[ ] Add error handling
27-
[ ] Write tests
28-
</todos>
29-
</new_task>
30-
3118
Always use the actual tool name as the XML tag name for proper parsing and execution.`
3219
}

‎src/core/prompts/tools/__tests__/new-task.spec.ts‎

Lines changed: 30 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { getNewTaskDescription } from "../new-task"
33
import { ToolArgs } from "../types"
44

55
describe("getNewTaskDescription", () => {
6-
it("should not show todos parameter at all when setting is disabled", () => {
6+
it("should show todos parameter as optional when setting is disabled", () => {
77
const args: ToolArgs = {
88
cwd: "/test",
99
supportsComputerUse: false,
@@ -14,15 +14,16 @@ describe("getNewTaskDescription", () => {
1414

1515
const description = getNewTaskDescription(args)
1616

17-
// Check that todos parameter is not mentioned at all
18-
expect(description).not.toContain("todos:")
19-
expect(description).not.toContain("todo list")
20-
expect(description).not.toContain("<todos>")
21-
expect(description).not.toContain("</todos>")
17+
// Check that todos parameter is shown as optional
18+
expect(description).toContain("todos: (optional)")
19+
expect(description).toContain("The initial todo list in markdown checklist format")
2220

23-
// Should have a simple example without todos
21+
// Should have a simple example without todos in the main example
2422
expect(description).toContain("Implement a new feature for the application")
2523

24+
// Should also have an example with optional todos
25+
expect(description).toContain("Example with optional todos:")
26+
2627
// Should still have mode and message as required
2728
expect(description).toContain("mode: (required)")
2829
expect(description).toContain("message: (required)")
@@ -53,7 +54,7 @@ describe("getNewTaskDescription", () => {
5354
expect(description).toContain("Set up auth middleware")
5455
})
5556

56-
it("should not show todos parameter when settings is undefined", () => {
57+
it("should show todos parameter as optional when settings is undefined", () => {
5758
const args: ToolArgs = {
5859
cwd: "/test",
5960
supportsComputerUse: false,
@@ -62,14 +63,12 @@ describe("getNewTaskDescription", () => {
6263

6364
const description = getNewTaskDescription(args)
6465

65-
// Check that todos parameter is not shown by default
66-
expect(description).not.toContain("todos:")
67-
expect(description).not.toContain("todo list")
68-
expect(description).not.toContain("<todos>")
69-
expect(description).not.toContain("</todos>")
66+
// Check that todos parameter is shown as optional by default
67+
expect(description).toContain("todos: (optional)")
68+
expect(description).toContain("The initial todo list in markdown checklist format")
7069
})
7170

72-
it("should not show todos parameter when newTaskRequireTodos is undefined", () => {
71+
it("should show todos parameter as optional when newTaskRequireTodos is undefined", () => {
7372
const args: ToolArgs = {
7473
cwd: "/test",
7574
supportsComputerUse: false,
@@ -78,14 +77,12 @@ describe("getNewTaskDescription", () => {
7877

7978
const description = getNewTaskDescription(args)
8079

81-
// Check that todos parameter is not shown by default
82-
expect(description).not.toContain("todos:")
83-
expect(description).not.toContain("todo list")
84-
expect(description).not.toContain("<todos>")
85-
expect(description).not.toContain("</todos>")
80+
// Check that todos parameter is shown as optional by default
81+
expect(description).toContain("todos: (optional)")
82+
expect(description).toContain("The initial todo list in markdown checklist format")
8683
})
8784

88-
it("should only include todos in example when setting is enabled", () => {
85+
it("should include todos in main example only when setting is enabled", () => {
8986
const argsWithSettingOff: ToolArgs = {
9087
cwd: "/test",
9188
supportsComputerUse: false,
@@ -105,13 +102,19 @@ describe("getNewTaskDescription", () => {
105102
const descriptionOff = getNewTaskDescription(argsWithSettingOff)
106103
const descriptionOn = getNewTaskDescription(argsWithSettingOn)
107104

108-
// When setting is off, should NOT include todos in example
109-
const todosPattern = /<todos>\s*\[\s*\]\s*Set up auth middleware/s
110-
expect(descriptionOff).not.toMatch(todosPattern)
111-
expect(descriptionOff).not.toContain("<todos>")
105+
// When setting is on, should include todos in main example
106+
expect(descriptionOn).toContain("Implement user authentication")
107+
expect(descriptionOn).toContain("[ ] Set up auth middleware")
108+
109+
// When setting is on, should NOT have "Example with optional todos" section
110+
expect(descriptionOn).not.toContain("Example with optional todos:")
111+
112+
// When setting is off, main example should NOT include todos in Usage section
113+
const usagePattern = /<new_task>\s*<mode>.*<\/mode>\s*<message>.*<\/message>\s*<\/new_task>/s
114+
expect(descriptionOff).toMatch(usagePattern)
112115

113-
// When setting is on, should include todos in example
114-
expect(descriptionOn).toMatch(todosPattern)
115-
expect(descriptionOn).toContain("<todos>")
116+
// When setting is off, should have separate "Example with optional todos" section
117+
expect(descriptionOff).toContain("Example with optional todos:")
118+
expect(descriptionOff).toContain("[ ] Set up auth middleware")
116119
})
117120
})

‎src/core/prompts/tools/new-task.ts‎

Lines changed: 29 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -3,52 +3,51 @@ import { ToolArgs } from "./types"
33
export function getNewTaskDescription(args: ToolArgs): string {
44
const todosRequired = args.settings?.newTaskRequireTodos === true
55

6-
// When setting is disabled, don't show todos parameter at all
7-
if (!todosRequired) {
8-
return `## new_task
9-
Description: This will let you create a new task instance in the chosen mode using your provided message.
10-
11-
Parameters:
12-
- mode: (required) The slug of the mode to start the new task in (e.g., "code", "debug", "architect").
13-
- message: (required) The initial user message or instructions for this new task.
14-
15-
Usage:
16-
<new_task>
17-
<mode>your-mode-slug-here</mode>
18-
<message>Your initial instructions here</message>
19-
</new_task>
20-
21-
Example:
22-
<new_task>
23-
<mode>code</mode>
24-
<message>Implement a new feature for the application.</message>
25-
</new_task>
26-
`
27-
}
28-
29-
// When setting is enabled, show todos as required
6+
// Always show the todos parameter, but mark it as optional or required based on setting
307
return `## new_task
31-
Description: This will let you create a new task instance in the chosen mode using your provided message and initial todo list.
8+
Description: This will let you create a new task instance in the chosen mode using your provided message${todosRequired ? " and initial todo list" : ""}.
329
3310
Parameters:
3411
- mode: (required) The slug of the mode to start the new task in (e.g., "code", "debug", "architect").
3512
- message: (required) The initial user message or instructions for this new task.
36-
- todos: (required) The initial todo list in markdown checklist format for the new task.
13+
- todos: (${todosRequired ? "required" : "optional"}) The initial todo list in markdown checklist format for the new task.
3714
3815
Usage:
3916
<new_task>
4017
<mode>your-mode-slug-here</mode>
41-
<message>Your initial instructions here</message>
18+
<message>Your initial instructions here</message>${
19+
todosRequired
20+
? `
4221
<todos>
4322
[ ] First task to complete
4423
[ ] Second task to complete
4524
[ ] Third task to complete
46-
</todos>
25+
</todos>`
26+
: ""
27+
}
4728
</new_task>
4829
4930
Example:
5031
<new_task>
5132
<mode>code</mode>
33+
<message>${todosRequired ? "Implement user authentication" : "Implement a new feature for the application"}</message>${
34+
todosRequired
35+
? `
36+
<todos>
37+
[ ] Set up auth middleware
38+
[ ] Create login endpoint
39+
[ ] Add session management
40+
[ ] Write tests
41+
</todos>`
42+
: ""
43+
}
44+
</new_task>
45+
46+
${
47+
!todosRequired
48+
? `Example with optional todos:
49+
<new_task>
50+
<mode>code</mode>
5251
<message>Implement user authentication</message>
5352
<todos>
5453
[ ] Set up auth middleware
@@ -58,4 +57,6 @@ Example:
5857
</todos>
5958
</new_task>
6059
`
60+
: ""
61+
}`
6162
}

‎src/core/tools/__tests__/newTaskTool.spec.ts‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,16 @@ vi.mock("vscode", () => ({
1111
},
1212
}))
1313

14+
// Mock Package module
15+
vi.mock("../../../shared/package", () => ({
16+
Package: {
17+
name: "roo-cline",
18+
publisher: "RooVeterinaryInc",
19+
version: "1.0.0",
20+
outputChannel: "Roo-Code",
21+
},
22+
}))
23+
1424
// Mock other modules first - these are hoisted to the top
1525
vi.mock("../../../shared/modes", () => ({
1626
getModeBySlug: vi.fn(),
@@ -589,7 +599,7 @@ describe("newTaskTool", () => {
589599
expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Successfully created new task"))
590600
})
591601

592-
it("should check VSCode setting with correct configuration key", async () => {
602+
it("should check VSCode setting with Package.name configuration key", async () => {
593603
const mockGet = vi.fn().mockReturnValue(false)
594604
const mockGetConfiguration = vi.fn().mockReturnValue({
595605
get: mockGet,
@@ -615,7 +625,7 @@ describe("newTaskTool", () => {
615625
mockRemoveClosingTag,
616626
)
617627

618-
// Verify that VSCode configuration was accessed correctly
628+
// Verify that VSCode configuration was accessed with Package.name
619629
expect(mockGetConfiguration).toHaveBeenCalledWith("roo-cline")
620630
expect(mockGet).toHaveBeenCalledWith("newTaskRequireTodos", false)
621631
})

‎src/core/tools/newTaskTool.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { defaultModeSlug, getModeBySlug } from "../../shared/modes"
99
import { formatResponse } from "../prompts/responses"
1010
import { t } from "../../i18n"
1111
import { parseMarkdownChecklist } from "./updateTodoListTool"
12+
import { Package } from "../../shared/package"
1213

1314
export async function newTaskTool(
1415
cline: Task,
@@ -56,8 +57,10 @@ export async function newTaskTool(
5657
return
5758
}
5859
const state = await provider.getState()
60+
61+
// Use Package.name to get the correct configuration namespace
5962
const requireTodos = vscode.workspace
60-
.getConfiguration("roo-cline")
63+
.getConfiguration(Package.name)
6164
.get<boolean>("newTaskRequireTodos", false)
6265

6366
// Check if todos are required based on VSCode setting

0 commit comments

Comments
 (0)