Skip to content

Commit 53e7e6b

Browse files
KJ7LNWcannuridaniel-lxs
authored
feat: Allow escaping of context mentions (#4362)
* allow escaping of context mentions * refactor: update comment --------- Co-authored-by: cannuri <[email protected]> Co-authored-by: Daniel <[email protected]>
1 parent 8f58737 commit 53e7e6b

File tree

4 files changed

+200
-4
lines changed

4 files changed

+200
-4
lines changed
Lines changed: 186 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,186 @@
1+
import { jest } from "@jest/globals"
2+
import type { AskApproval, HandleError } from "../../../shared/tools" // Import the types
3+
4+
// Mock dependencies before importing the module under test
5+
// Explicitly type the mock functions
6+
const mockAskApproval = jest.fn<AskApproval>()
7+
const mockHandleError = jest.fn<HandleError>() // Explicitly type HandleError
8+
const mockPushToolResult = jest.fn()
9+
const mockRemoveClosingTag = jest.fn((_name: string, value: string | undefined) => value ?? "") // Simple mock
10+
const mockGetModeBySlug = jest.fn()
11+
// Define a minimal type for the resolved value
12+
type MockClineInstance = { taskId: string }
13+
// Make initClineWithTask return a mock Cline-like object with taskId, providing type hint
14+
const mockInitClineWithTask = jest
15+
.fn<() => Promise<MockClineInstance>>()
16+
.mockResolvedValue({ taskId: "mock-subtask-id" })
17+
const mockEmit = jest.fn()
18+
const mockRecordToolError = jest.fn()
19+
const mockSayAndCreateMissingParamError = jest.fn()
20+
21+
// Mock the Cline instance and its methods/properties
22+
const mockCline = {
23+
ask: jest.fn(),
24+
sayAndCreateMissingParamError: mockSayAndCreateMissingParamError,
25+
emit: mockEmit,
26+
recordToolError: mockRecordToolError,
27+
consecutiveMistakeCount: 0,
28+
isPaused: false,
29+
pausedModeSlug: "ask", // Default or mock value
30+
providerRef: {
31+
deref: jest.fn(() => ({
32+
getState: jest.fn(() => ({ customModes: [], mode: "ask" })), // Mock provider state
33+
handleModeSwitch: jest.fn(),
34+
initClineWithTask: mockInitClineWithTask,
35+
})),
36+
},
37+
}
38+
39+
// Mock other modules
40+
jest.mock("delay", () => jest.fn(() => Promise.resolve())) // Mock delay to resolve immediately
41+
jest.mock("../../../shared/modes", () => ({
42+
// Corrected path
43+
getModeBySlug: mockGetModeBySlug,
44+
defaultModeSlug: "ask",
45+
}))
46+
jest.mock("../../prompts/responses", () => ({
47+
// Corrected path
48+
formatResponse: {
49+
toolError: jest.fn((msg: string) => `Tool Error: ${msg}`), // Simple mock
50+
},
51+
}))
52+
53+
// Import the function to test AFTER mocks are set up
54+
import { newTaskTool } from "../newTaskTool"
55+
import type { ToolUse } from "../../../shared/tools"
56+
57+
describe("newTaskTool", () => {
58+
beforeEach(() => {
59+
// Reset mocks before each test
60+
jest.clearAllMocks()
61+
mockAskApproval.mockResolvedValue(true) // Default to approved
62+
mockGetModeBySlug.mockReturnValue({ slug: "code", name: "Code Mode" }) // Default valid mode
63+
mockCline.consecutiveMistakeCount = 0
64+
mockCline.isPaused = false
65+
})
66+
67+
it("should correctly un-escape \\\\@ to \\@ in the message passed to the new task", async () => {
68+
const block: ToolUse = {
69+
type: "tool_use", // Add required 'type' property
70+
name: "new_task", // Correct property name
71+
params: {
72+
mode: "code",
73+
message: "Review this: \\\\@file1.txt and also \\\\\\\\@file2.txt", // Input with \\@ and \\\\@
74+
},
75+
partial: false,
76+
}
77+
78+
await newTaskTool(
79+
mockCline as any, // Use 'as any' for simplicity in mocking complex type
80+
block,
81+
mockAskApproval, // Now correctly typed
82+
mockHandleError,
83+
mockPushToolResult,
84+
mockRemoveClosingTag,
85+
)
86+
87+
// Verify askApproval was called
88+
expect(mockAskApproval).toHaveBeenCalled()
89+
90+
// Verify the message passed to initClineWithTask reflects the code's behavior in unit tests
91+
expect(mockInitClineWithTask).toHaveBeenCalledWith(
92+
"Review this: \\@file1.txt and also \\\\\\@file2.txt", // Unit Test Expectation: \\@ -> \@, \\\\@ -> \\\\@
93+
undefined,
94+
mockCline,
95+
)
96+
97+
// Verify side effects
98+
expect(mockCline.emit).toHaveBeenCalledWith("taskSpawned", expect.any(String)) // Assuming initCline returns a mock task ID
99+
expect(mockCline.isPaused).toBe(true)
100+
expect(mockCline.emit).toHaveBeenCalledWith("taskPaused")
101+
expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Successfully created new task"))
102+
})
103+
104+
it("should not un-escape single escaped \@", async () => {
105+
const block: ToolUse = {
106+
type: "tool_use", // Add required 'type' property
107+
name: "new_task", // Correct property name
108+
params: {
109+
mode: "code",
110+
message: "This is already unescaped: \\@file1.txt",
111+
},
112+
partial: false,
113+
}
114+
115+
await newTaskTool(
116+
mockCline as any,
117+
block,
118+
mockAskApproval, // Now correctly typed
119+
mockHandleError,
120+
mockPushToolResult,
121+
mockRemoveClosingTag,
122+
)
123+
124+
expect(mockInitClineWithTask).toHaveBeenCalledWith(
125+
"This is already unescaped: \\@file1.txt", // Expected: \@ remains \@
126+
undefined,
127+
mockCline,
128+
)
129+
})
130+
131+
it("should not un-escape non-escaped @", async () => {
132+
const block: ToolUse = {
133+
type: "tool_use", // Add required 'type' property
134+
name: "new_task", // Correct property name
135+
params: {
136+
mode: "code",
137+
message: "A normal mention @file1.txt",
138+
},
139+
partial: false,
140+
}
141+
142+
await newTaskTool(
143+
mockCline as any,
144+
block,
145+
mockAskApproval, // Now correctly typed
146+
mockHandleError,
147+
mockPushToolResult,
148+
mockRemoveClosingTag,
149+
)
150+
151+
expect(mockInitClineWithTask).toHaveBeenCalledWith(
152+
"A normal mention @file1.txt", // Expected: @ remains @
153+
undefined,
154+
mockCline,
155+
)
156+
})
157+
158+
it("should handle mixed escaping scenarios", async () => {
159+
const block: ToolUse = {
160+
type: "tool_use", // Add required 'type' property
161+
name: "new_task", // Correct property name
162+
params: {
163+
mode: "code",
164+
message: "Mix: @file0.txt, \\@file1.txt, \\\\@file2.txt, \\\\\\\\@file3.txt",
165+
},
166+
partial: false,
167+
}
168+
169+
await newTaskTool(
170+
mockCline as any,
171+
block,
172+
mockAskApproval, // Now correctly typed
173+
mockHandleError,
174+
mockPushToolResult,
175+
mockRemoveClosingTag,
176+
)
177+
178+
expect(mockInitClineWithTask).toHaveBeenCalledWith(
179+
"Mix: @file0.txt, \\@file1.txt, \\@file2.txt, \\\\\\@file3.txt", // Unit Test Expectation: @->@, \@->\@, \\@->\@, \\\\@->\\\\@
180+
undefined,
181+
mockCline,
182+
)
183+
})
184+
185+
// Add more tests for error handling (missing params, invalid mode, approval denied) if needed
186+
})

src/core/tools/newTaskTool.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,9 @@ export async function newTaskTool(
4242
}
4343

4444
cline.consecutiveMistakeCount = 0
45+
// Un-escape one level of backslashes before '@' for hierarchical subtasks
46+
// Un-escape one level: \\@ -> \@ (removes one backslash for hierarchical subtasks)
47+
const unescapedMessage = message.replace(/\\\\@/g, "\\@")
4548

4649
// Verify the mode exists
4750
const targetMode = getModeBySlug(mode, (await cline.providerRef.deref()?.getState())?.customModes)
@@ -82,10 +85,10 @@ export async function newTaskTool(
8285
// Delay to allow mode change to take effect before next tool is executed.
8386
await delay(500)
8487

85-
const newCline = await provider.initClineWithTask(message, undefined, cline)
88+
const newCline = await provider.initClineWithTask(unescapedMessage, undefined, cline)
8689
cline.emit("taskSpawned", newCline.taskId)
8790

88-
pushToolResult(`Successfully created new task in ${targetMode.name} mode with message: ${message}`)
91+
pushToolResult(`Successfully created new task in ${targetMode.name} mode with message: ${unescapedMessage}`)
8992

9093
// Set the isPaused flag to true so the parent
9194
// task can wait for the sub-task to finish.

src/shared/__tests__/context-mentions.test.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,15 @@ describe("mentionRegex and mentionRegexGlobal", () => {
4141
{ input: "mention@", expected: null }, // Trailing @
4242
{ input: "@/path/trailing\\", expected: null }, // Trailing backslash (invalid escape)
4343
{ input: "@/path/to/file\\not-a-space", expected: null }, // Backslash not followed by space
44+
// Escaped mentions (should not match due to negative lookbehind)
45+
{ input: "This is not a mention: \\@/path/to/file.txt", expected: null },
46+
{ input: "Escaped \\@problems word", expected: null },
47+
{ input: "Text with \\@https://example.com", expected: null },
48+
{ input: "Another \\@a1b2c3d hash", expected: null },
49+
{ input: "Not escaped @terminal", expected: ["@terminal"] }, // Ensure non-escaped still works nearby
50+
{ input: "Double escape \\\\@/should/match", expected: null }, // Double backslash escapes the backslash, currently incorrectly fails to match
51+
{ input: "Text with \\@/escaped/path\\ with\\ spaces.txt", expected: null }, // Escaped mention with escaped spaces within the path part
4452
]
45-
4653
testCases.forEach(({ input, expected }) => {
4754
it(`should handle input: "${input}"`, () => {
4855
// Test mentionRegex (first match)

src/shared/context-mentions.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ Mention regex:
5454
5555
*/
5656
export const mentionRegex =
57-
/@((?:\/|\w+:\/\/)(?:[^\s\\]|\\ )+?|[a-f0-9]{7,40}\b|problems\b|git-changes\b|terminal\b)(?=[.,;:!?]?(?=[\s\r\n]|$))/
57+
/(?<!\\)@((?:\/|\w+:\/\/)(?:[^\s\\]|\\ )+?|[a-f0-9]{7,40}\b|problems\b|git-changes\b|terminal\b)(?=[.,;:!?]?(?=[\s\r\n]|$))/
5858
export const mentionRegexGlobal = new RegExp(mentionRegex.source, "g")
5959

6060
export interface MentionSuggestion {

0 commit comments

Comments
 (0)