From 097563b53228a62cf89421e1613d4921644a0d65 Mon Sep 17 00:00:00 2001 From: cannuri <91494156+cannuri@users.noreply.github.com> Date: Thu, 1 May 2025 10:00:10 +0200 Subject: [PATCH 1/2] allow escaping of context mentions --- src/core/tools/__tests__/newTaskTool.test.ts | 186 ++++++++++++++++++ src/core/tools/newTaskTool.ts | 7 +- src/shared/__tests__/context-mentions.test.ts | 9 +- src/shared/context-mentions.ts | 2 +- 4 files changed, 200 insertions(+), 4 deletions(-) create mode 100644 src/core/tools/__tests__/newTaskTool.test.ts diff --git a/src/core/tools/__tests__/newTaskTool.test.ts b/src/core/tools/__tests__/newTaskTool.test.ts new file mode 100644 index 0000000000..1a9e497df3 --- /dev/null +++ b/src/core/tools/__tests__/newTaskTool.test.ts @@ -0,0 +1,186 @@ +import { jest } from "@jest/globals" +import type { AskApproval, HandleError } from "../../../shared/tools" // Import the types + +// Mock dependencies before importing the module under test +// Explicitly type the mock functions +const mockAskApproval = jest.fn() +const mockHandleError = jest.fn() // Explicitly type HandleError +const mockPushToolResult = jest.fn() +const mockRemoveClosingTag = jest.fn((_name: string, value: string | undefined) => value ?? "") // Simple mock +const mockGetModeBySlug = jest.fn() +// Define a minimal type for the resolved value +type MockClineInstance = { taskId: string } +// Make initClineWithTask return a mock Cline-like object with taskId, providing type hint +const mockInitClineWithTask = jest + .fn<() => Promise>() + .mockResolvedValue({ taskId: "mock-subtask-id" }) +const mockEmit = jest.fn() +const mockRecordToolError = jest.fn() +const mockSayAndCreateMissingParamError = jest.fn() + +// Mock the Cline instance and its methods/properties +const mockCline = { + ask: jest.fn(), + sayAndCreateMissingParamError: mockSayAndCreateMissingParamError, + emit: mockEmit, + recordToolError: mockRecordToolError, + consecutiveMistakeCount: 0, + isPaused: false, + pausedModeSlug: "ask", // Default or mock value + providerRef: { + deref: jest.fn(() => ({ + getState: jest.fn(() => ({ customModes: [], mode: "ask" })), // Mock provider state + handleModeSwitch: jest.fn(), + initClineWithTask: mockInitClineWithTask, + })), + }, +} + +// Mock other modules +jest.mock("delay", () => jest.fn(() => Promise.resolve())) // Mock delay to resolve immediately +jest.mock("../../../shared/modes", () => ({ + // Corrected path + getModeBySlug: mockGetModeBySlug, + defaultModeSlug: "ask", +})) +jest.mock("../../prompts/responses", () => ({ + // Corrected path + formatResponse: { + toolError: jest.fn((msg: string) => `Tool Error: ${msg}`), // Simple mock + }, +})) + +// Import the function to test AFTER mocks are set up +import { newTaskTool } from "../newTaskTool" +import type { ToolUse } from "../../../shared/tools" + +describe("newTaskTool", () => { + beforeEach(() => { + // Reset mocks before each test + jest.clearAllMocks() + mockAskApproval.mockResolvedValue(true) // Default to approved + mockGetModeBySlug.mockReturnValue({ slug: "code", name: "Code Mode" }) // Default valid mode + mockCline.consecutiveMistakeCount = 0 + mockCline.isPaused = false + }) + + it("should correctly un-escape \\\\@ to \\@ in the message passed to the new task", async () => { + const block: ToolUse = { + type: "tool_use", // Add required 'type' property + name: "new_task", // Correct property name + params: { + mode: "code", + message: "Review this: \\\\@file1.txt and also \\\\\\\\@file2.txt", // Input with \\@ and \\\\@ + }, + partial: false, + } + + await newTaskTool( + mockCline as any, // Use 'as any' for simplicity in mocking complex type + block, + mockAskApproval, // Now correctly typed + mockHandleError, + mockPushToolResult, + mockRemoveClosingTag, + ) + + // Verify askApproval was called + expect(mockAskApproval).toHaveBeenCalled() + + // Verify the message passed to initClineWithTask reflects the code's behavior in unit tests + expect(mockInitClineWithTask).toHaveBeenCalledWith( + "Review this: \\@file1.txt and also \\\\\\@file2.txt", // Unit Test Expectation: \\@ -> \@, \\\\@ -> \\\\@ + undefined, + mockCline, + ) + + // Verify side effects + expect(mockCline.emit).toHaveBeenCalledWith("taskSpawned", expect.any(String)) // Assuming initCline returns a mock task ID + expect(mockCline.isPaused).toBe(true) + expect(mockCline.emit).toHaveBeenCalledWith("taskPaused") + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Successfully created new task")) + }) + + it("should not un-escape single escaped \@", async () => { + const block: ToolUse = { + type: "tool_use", // Add required 'type' property + name: "new_task", // Correct property name + params: { + mode: "code", + message: "This is already unescaped: \\@file1.txt", + }, + partial: false, + } + + await newTaskTool( + mockCline as any, + block, + mockAskApproval, // Now correctly typed + mockHandleError, + mockPushToolResult, + mockRemoveClosingTag, + ) + + expect(mockInitClineWithTask).toHaveBeenCalledWith( + "This is already unescaped: \\@file1.txt", // Expected: \@ remains \@ + undefined, + mockCline, + ) + }) + + it("should not un-escape non-escaped @", async () => { + const block: ToolUse = { + type: "tool_use", // Add required 'type' property + name: "new_task", // Correct property name + params: { + mode: "code", + message: "A normal mention @file1.txt", + }, + partial: false, + } + + await newTaskTool( + mockCline as any, + block, + mockAskApproval, // Now correctly typed + mockHandleError, + mockPushToolResult, + mockRemoveClosingTag, + ) + + expect(mockInitClineWithTask).toHaveBeenCalledWith( + "A normal mention @file1.txt", // Expected: @ remains @ + undefined, + mockCline, + ) + }) + + it("should handle mixed escaping scenarios", async () => { + const block: ToolUse = { + type: "tool_use", // Add required 'type' property + name: "new_task", // Correct property name + params: { + mode: "code", + message: "Mix: @file0.txt, \\@file1.txt, \\\\@file2.txt, \\\\\\\\@file3.txt", + }, + partial: false, + } + + await newTaskTool( + mockCline as any, + block, + mockAskApproval, // Now correctly typed + mockHandleError, + mockPushToolResult, + mockRemoveClosingTag, + ) + + expect(mockInitClineWithTask).toHaveBeenCalledWith( + "Mix: @file0.txt, \\@file1.txt, \\@file2.txt, \\\\\\@file3.txt", // Unit Test Expectation: @->@, \@->\@, \\@->\@, \\\\@->\\\\@ + undefined, + mockCline, + ) + }) + + // Add more tests for error handling (missing params, invalid mode, approval denied) if needed +}) diff --git a/src/core/tools/newTaskTool.ts b/src/core/tools/newTaskTool.ts index bdb6d9a009..80335f111b 100644 --- a/src/core/tools/newTaskTool.ts +++ b/src/core/tools/newTaskTool.ts @@ -42,6 +42,9 @@ export async function newTaskTool( } cline.consecutiveMistakeCount = 0 + // Un-escape one level of backslashes before '@' for hierarchical subtasks + // Un-escape one level: \\@ -> @ (This seems to be the observed behavior) + const unescapedMessage = message.replace(/\\\\@/g, "\\@") // Verify the mode exists const targetMode = getModeBySlug(mode, (await cline.providerRef.deref()?.getState())?.customModes) @@ -82,10 +85,10 @@ export async function newTaskTool( // Delay to allow mode change to take effect before next tool is executed. await delay(500) - const newCline = await provider.initClineWithTask(message, undefined, cline) + const newCline = await provider.initClineWithTask(unescapedMessage, undefined, cline) cline.emit("taskSpawned", newCline.taskId) - pushToolResult(`Successfully created new task in ${targetMode.name} mode with message: ${message}`) + pushToolResult(`Successfully created new task in ${targetMode.name} mode with message: ${unescapedMessage}`) // Set the isPaused flag to true so the parent // task can wait for the sub-task to finish. diff --git a/src/shared/__tests__/context-mentions.test.ts b/src/shared/__tests__/context-mentions.test.ts index cb070d4717..04246945ab 100644 --- a/src/shared/__tests__/context-mentions.test.ts +++ b/src/shared/__tests__/context-mentions.test.ts @@ -41,8 +41,15 @@ describe("mentionRegex and mentionRegexGlobal", () => { { input: "mention@", expected: null }, // Trailing @ { input: "@/path/trailing\\", expected: null }, // Trailing backslash (invalid escape) { input: "@/path/to/file\\not-a-space", expected: null }, // Backslash not followed by space + // Escaped mentions (should not match due to negative lookbehind) + { input: "This is not a mention: \\@/path/to/file.txt", expected: null }, + { input: "Escaped \\@problems word", expected: null }, + { input: "Text with \\@https://example.com", expected: null }, + { input: "Another \\@a1b2c3d hash", expected: null }, + { input: "Not escaped @terminal", expected: ["@terminal"] }, // Ensure non-escaped still works nearby + { input: "Double escape \\\\@/should/match", expected: null }, // Double backslash escapes the backslash, currently incorrectly fails to match + { input: "Text with \\@/escaped/path\\ with\\ spaces.txt", expected: null }, // Escaped mention with escaped spaces within the path part ] - testCases.forEach(({ input, expected }) => { it(`should handle input: "${input}"`, () => { // Test mentionRegex (first match) diff --git a/src/shared/context-mentions.ts b/src/shared/context-mentions.ts index fb7ba4723c..2edb99de6a 100644 --- a/src/shared/context-mentions.ts +++ b/src/shared/context-mentions.ts @@ -54,7 +54,7 @@ Mention regex: */ export const mentionRegex = - /@((?:\/|\w+:\/\/)(?:[^\s\\]|\\ )+?|[a-f0-9]{7,40}\b|problems\b|git-changes\b|terminal\b)(?=[.,;:!?]?(?=[\s\r\n]|$))/ + /(? Date: Sat, 7 Jun 2025 12:46:02 -0500 Subject: [PATCH 2/2] refactor: update comment --- src/core/tools/newTaskTool.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/tools/newTaskTool.ts b/src/core/tools/newTaskTool.ts index 80335f111b..25d5766d5d 100644 --- a/src/core/tools/newTaskTool.ts +++ b/src/core/tools/newTaskTool.ts @@ -43,7 +43,7 @@ export async function newTaskTool( cline.consecutiveMistakeCount = 0 // Un-escape one level of backslashes before '@' for hierarchical subtasks - // Un-escape one level: \\@ -> @ (This seems to be the observed behavior) +// Un-escape one level: \\@ -> \@ (removes one backslash for hierarchical subtasks) const unescapedMessage = message.replace(/\\\\@/g, "\\@") // Verify the mode exists