From 8d9973cc85d0b21401fd4e265fc8a7ee7675e115 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Thu, 31 Jul 2025 04:47:15 +0000 Subject: [PATCH 1/3] feat: skip interpolation for non-existent slash commands - Modified parseMentions to check command existence before text replacement - Non-existent commands are now left unchanged in the original text - Updated tests to reflect new behavior where non-existent commands remain as-is - Added error handling for command existence checks --- src/__tests__/command-mentions.spec.ts | 75 ++++++++++++++++++++++---- src/core/mentions/index.ts | 37 +++++++++---- 2 files changed, 91 insertions(+), 21 deletions(-) diff --git a/src/__tests__/command-mentions.spec.ts b/src/__tests__/command-mentions.spec.ts index d4de0bbba7..11b718d0d3 100644 --- a/src/__tests__/command-mentions.spec.ts +++ b/src/__tests__/command-mentions.spec.ts @@ -62,16 +62,31 @@ describe("Command Mentions", () => { }) it("should handle multiple commands in message", async () => { + const setupContent = "# Setup Environment\n\nRun the following commands:\n```bash\nnpm install\n```" + const deployContent = "# Deploy Environment\n\nRun the following commands:\n```bash\nnpm run deploy\n```" + mockGetCommand .mockResolvedValueOnce({ name: "setup", - content: "# Setup instructions", + content: setupContent, source: "project", filePath: "/project/.roo/commands/setup.md", }) .mockResolvedValueOnce({ name: "deploy", - content: "# Deploy instructions", + content: deployContent, + source: "project", + filePath: "/project/.roo/commands/deploy.md", + }) + .mockResolvedValueOnce({ + name: "setup", + content: setupContent, + source: "project", + filePath: "/project/.roo/commands/setup.md", + }) + .mockResolvedValueOnce({ + name: "deploy", + content: deployContent, source: "project", filePath: "/project/.roo/commands/deploy.md", }) @@ -82,31 +97,53 @@ describe("Command Mentions", () => { expect(mockGetCommand).toHaveBeenCalledWith("/test/cwd", "setup") expect(mockGetCommand).toHaveBeenCalledWith("/test/cwd", "deploy") - expect(mockGetCommand).toHaveBeenCalledTimes(2) // Both commands called + expect(mockGetCommand).toHaveBeenCalledTimes(4) // Each command called twice (existence check + processing) expect(result).toContain('') - expect(result).toContain("# Setup instructions") + expect(result).toContain("# Setup Environment") expect(result).toContain('') - expect(result).toContain("# Deploy instructions") + expect(result).toContain("# Deploy Environment") }) - it("should handle non-existent command gracefully", async () => { + it("should leave non-existent commands unchanged", async () => { mockGetCommand.mockResolvedValue(undefined) const input = "/nonexistent command" const result = await callParseMentions(input) expect(mockGetCommand).toHaveBeenCalledWith("/test/cwd", "nonexistent") - expect(result).toContain('') - expect(result).toContain("Command 'nonexistent' not found") - expect(result).toContain("") + // The command should remain unchanged in the text + expect(result).toBe("/nonexistent command") + // Should not contain any command tags + expect(result).not.toContain('') + expect(result).not.toContain("Command 'nonexistent' not found") }) - it("should handle command loading errors", async () => { + it("should handle command loading errors during existence check", async () => { mockGetCommand.mockRejectedValue(new Error("Failed to load command")) const input = "/error-command test" const result = await callParseMentions(input) + // When getCommand throws an error during existence check, + // the command is treated as non-existent and left unchanged + expect(result).toBe("/error-command test") + expect(result).not.toContain('') + }) + + it("should handle command loading errors during processing", async () => { + // First call (existence check) succeeds, second call (processing) fails + mockGetCommand + .mockResolvedValueOnce({ + name: "error-command", + content: "# Error command", + source: "project", + filePath: "/project/.roo/commands/error-command.md", + }) + .mockRejectedValueOnce(new Error("Failed to load command")) + + const input = "/error-command test" + const result = await callParseMentions(input) + expect(result).toContain('') expect(result).toContain("Error loading command") expect(result).toContain("") @@ -246,13 +283,29 @@ npm install }) describe("command mention text transformation", () => { - it("should transform command mentions at start of message", async () => { + it("should transform existing command mentions at start of message", async () => { + mockGetCommand.mockResolvedValue({ + name: "setup", + content: "# Setup instructions", + source: "project", + filePath: "/project/.roo/commands/setup.md", + }) + const input = "/setup the project" const result = await callParseMentions(input) expect(result).toContain("Command 'setup' (see below for command content)") }) + it("should leave non-existent command mentions unchanged", async () => { + mockGetCommand.mockResolvedValue(undefined) + + const input = "/nonexistent the project" + const result = await callParseMentions(input) + + expect(result).toBe("/nonexistent the project") + }) + it("should process multiple commands in message", async () => { mockGetCommand .mockResolvedValueOnce({ diff --git a/src/core/mentions/index.ts b/src/core/mentions/index.ts index b6a9dd4d0d..085674a42c 100644 --- a/src/core/mentions/index.ts +++ b/src/core/mentions/index.ts @@ -86,13 +86,30 @@ export async function parseMentions( maxReadFileLine?: number, ): Promise { const mentions: Set = new Set() - const commandMentions: Set = new Set() + const validCommandMentions: Set = new Set() - // First pass: extract command mentions (starting with /) - let parsedText = text.replace(commandRegexGlobal, (match, commandName) => { - commandMentions.add(commandName) - return `Command '${commandName}' (see below for command content)` - }) + // First pass: check which command mentions exist before replacing text + const commandMatches = Array.from(text.matchAll(commandRegexGlobal)) + const commandExistenceChecks = await Promise.all( + commandMatches.map(async ([match, commandName]) => { + try { + const command = await getCommand(cwd, commandName) + return { match, commandName, exists: !!command } + } catch (error) { + // If there's an error checking command existence, treat it as non-existent + return { match, commandName, exists: false } + } + }), + ) + + // Only replace text for commands that actually exist + let parsedText = text + for (const { match, commandName, exists } of commandExistenceChecks) { + if (exists) { + validCommandMentions.add(commandName) + parsedText = parsedText.replace(match, `Command '${commandName}' (see below for command content)`) + } + } // Second pass: handle regular mentions parsedText = parsedText.replace(mentionRegexGlobal, (match, mention) => { @@ -213,8 +230,8 @@ export async function parseMentions( } } - // Process command mentions - for (const commandName of commandMentions) { + // Process valid command mentions only + for (const commandName of validCommandMentions) { try { const command = await getCommand(cwd, commandName) if (command) { @@ -224,9 +241,9 @@ export async function parseMentions( } commandOutput += command.content parsedText += `\n\n\n${commandOutput}\n` - } else { - parsedText += `\n\n\nCommand '${commandName}' not found. Available commands can be found in .roo/commands/ or ~/.roo/commands/\n` } + // Note: We don't add error messages for non-existent commands anymore + // since we only process commands that we've already verified exist } catch (error) { parsedText += `\n\n\nError loading command '${commandName}': ${error.message}\n` } From 568eb1063804697a35bbb974aaa3bab7df508737 Mon Sep 17 00:00:00 2001 From: Matt Rubens Date: Thu, 31 Jul 2025 00:50:50 -0400 Subject: [PATCH 2/3] Update src/core/mentions/index.ts --- src/core/mentions/index.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/core/mentions/index.ts b/src/core/mentions/index.ts index 085674a42c..ac16fae760 100644 --- a/src/core/mentions/index.ts +++ b/src/core/mentions/index.ts @@ -242,8 +242,6 @@ export async function parseMentions( commandOutput += command.content parsedText += `\n\n\n${commandOutput}\n` } - // Note: We don't add error messages for non-existent commands anymore - // since we only process commands that we've already verified exist } catch (error) { parsedText += `\n\n\nError loading command '${commandName}': ${error.message}\n` } From 3ab8dfea0f2dc859360c9602efe5c22f8ad661d4 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Thu, 31 Jul 2025 04:58:41 +0000 Subject: [PATCH 3/3] perf: optimize command mention processing with caching - Reduce duplicate getCommand calls by caching results during existence checks - Eliminate redundant command loading in processing phase - Optimize regex matching by deduplicating command names - Update tests to reflect new optimized behavior - Maintain existing functionality while improving performance --- src/__tests__/command-mentions.spec.ts | 22 ++++++------- src/core/mentions/index.ts | 43 ++++++++++++++------------ 2 files changed, 35 insertions(+), 30 deletions(-) diff --git a/src/__tests__/command-mentions.spec.ts b/src/__tests__/command-mentions.spec.ts index 11b718d0d3..b120f3720c 100644 --- a/src/__tests__/command-mentions.spec.ts +++ b/src/__tests__/command-mentions.spec.ts @@ -97,7 +97,7 @@ describe("Command Mentions", () => { expect(mockGetCommand).toHaveBeenCalledWith("/test/cwd", "setup") expect(mockGetCommand).toHaveBeenCalledWith("/test/cwd", "deploy") - expect(mockGetCommand).toHaveBeenCalledTimes(4) // Each command called twice (existence check + processing) + expect(mockGetCommand).toHaveBeenCalledTimes(2) // Each unique command called once (optimized) expect(result).toContain('') expect(result).toContain("# Setup Environment") expect(result).toContain('') @@ -105,6 +105,7 @@ describe("Command Mentions", () => { }) it("should leave non-existent commands unchanged", async () => { + mockGetCommand.mockReset() mockGetCommand.mockResolvedValue(undefined) const input = "/nonexistent command" @@ -119,6 +120,7 @@ describe("Command Mentions", () => { }) it("should handle command loading errors during existence check", async () => { + mockGetCommand.mockReset() mockGetCommand.mockRejectedValue(new Error("Failed to load command")) const input = "/error-command test" @@ -131,21 +133,19 @@ describe("Command Mentions", () => { }) it("should handle command loading errors during processing", async () => { - // First call (existence check) succeeds, second call (processing) fails - mockGetCommand - .mockResolvedValueOnce({ - name: "error-command", - content: "# Error command", - source: "project", - filePath: "/project/.roo/commands/error-command.md", - }) - .mockRejectedValueOnce(new Error("Failed to load command")) + // With optimization, command is loaded once and cached + mockGetCommand.mockResolvedValue({ + name: "error-command", + content: "# Error command", + source: "project", + filePath: "/project/.roo/commands/error-command.md", + }) const input = "/error-command test" const result = await callParseMentions(input) expect(result).toContain('') - expect(result).toContain("Error loading command") + expect(result).toContain("# Error command") expect(result).toContain("") }) diff --git a/src/core/mentions/index.ts b/src/core/mentions/index.ts index ac16fae760..ed3060859b 100644 --- a/src/core/mentions/index.ts +++ b/src/core/mentions/index.ts @@ -18,7 +18,7 @@ import { UrlContentFetcher } from "../../services/browser/UrlContentFetcher" import { FileContextTracker } from "../context-tracking/FileContextTracker" import { RooIgnoreController } from "../ignore/RooIgnoreController" -import { getCommand } from "../../services/command/commands" +import { getCommand, type Command } from "../../services/command/commands" import { t } from "../../i18n" @@ -86,27 +86,35 @@ export async function parseMentions( maxReadFileLine?: number, ): Promise { const mentions: Set = new Set() - const validCommandMentions: Set = new Set() + const validCommands: Map = new Map() - // First pass: check which command mentions exist before replacing text + // First pass: check which command mentions exist and cache the results const commandMatches = Array.from(text.matchAll(commandRegexGlobal)) + const uniqueCommandNames = new Set(commandMatches.map(([, commandName]) => commandName)) + const commandExistenceChecks = await Promise.all( - commandMatches.map(async ([match, commandName]) => { + Array.from(uniqueCommandNames).map(async (commandName) => { try { const command = await getCommand(cwd, commandName) - return { match, commandName, exists: !!command } + return { commandName, command } } catch (error) { // If there's an error checking command existence, treat it as non-existent - return { match, commandName, exists: false } + return { commandName, command: undefined } } }), ) + // Store valid commands for later use + for (const { commandName, command } of commandExistenceChecks) { + if (command) { + validCommands.set(commandName, command) + } + } + // Only replace text for commands that actually exist let parsedText = text - for (const { match, commandName, exists } of commandExistenceChecks) { - if (exists) { - validCommandMentions.add(commandName) + for (const [match, commandName] of commandMatches) { + if (validCommands.has(commandName)) { parsedText = parsedText.replace(match, `Command '${commandName}' (see below for command content)`) } } @@ -230,18 +238,15 @@ export async function parseMentions( } } - // Process valid command mentions only - for (const commandName of validCommandMentions) { + // Process valid command mentions using cached results + for (const [commandName, command] of validCommands) { try { - const command = await getCommand(cwd, commandName) - if (command) { - let commandOutput = "" - if (command.description) { - commandOutput += `Description: ${command.description}\n\n` - } - commandOutput += command.content - parsedText += `\n\n\n${commandOutput}\n` + let commandOutput = "" + if (command.description) { + commandOutput += `Description: ${command.description}\n\n` } + commandOutput += command.content + parsedText += `\n\n\n${commandOutput}\n` } catch (error) { parsedText += `\n\n\nError loading command '${commandName}': ${error.message}\n` }