Skip to content

Skip interpolation for non-existent slash commands #6475

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 31, 2025
Merged

Skip interpolation for non-existent slash commands #6475

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8d9973c
feat: skip interpolation for non-existent slash commands
roomote Jul 31, 2025
568eb10
Update src/core/mentions/index.ts
mrubens Jul 31, 2025
3ab8dfe
perf: optimize command mention processing with caching
roomote Jul 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 64 additions & 11 deletions src/__tests__/command-mentions.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
})
Expand All @@ -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)
Copy link
Contributor Author

@roomote roomote bot Jul 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test efficiency: The mock is being called 4 times for the same commands, which reflects the duplicate API calls in the production code. This suggests the performance issue mentioned in the main implementation.

expect(result).toContain('<command name="setup">')
expect(result).toContain("# Setup instructions")
expect(result).toContain("# Setup Environment")
expect(result).toContain('<command name="deploy">')
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('<command name="nonexistent">')
expect(result).toContain("Command 'nonexistent' not found")
expect(result).toContain("</command>")
// The command should remain unchanged in the text
expect(result).toBe("/nonexistent command")
// Should not contain any command tags
expect(result).not.toContain('<command name="nonexistent">')
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('<command name="error-command">')
})

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('<command name="error-command">')
expect(result).toContain("Error loading command")
expect(result).toContain("</command>")
Expand Down Expand Up @@ -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({
Expand Down
37 changes: 27 additions & 10 deletions src/core/mentions/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,13 +86,30 @@ export async function parseMentions(
maxReadFileLine?: number,
): Promise<string> {
const mentions: Set<string> = new Set()
const commandMentions: Set<string> = new Set()
const validCommandMentions: Set<string> = new Set()
Copy link
Contributor Author

@roomote roomote bot Jul 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider renaming this variable to be more descriptive, such as 'existingCommandMentions' or 'verifiedCommandMentions' to better indicate that these commands have been verified to exist.


// 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 }
Copy link
Contributor Author

@roomote roomote bot Jul 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inefficient data usage: The command data fetched during existence checks is discarded here. Could we store the actual command objects instead of just boolean existence flags to reuse them later?

} 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) => {
Expand Down Expand Up @@ -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)
Copy link
Contributor Author

@roomote roomote bot Jul 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the second getCommand() call for the same command. Since we already fetched this data during existence checking (line 96), could we reuse that data instead of making another API call?

if (command) {
Expand All @@ -224,9 +241,9 @@ export async function parseMentions(
}
commandOutput += command.content
parsedText += `\n\n<command name="${commandName}">\n${commandOutput}\n</command>`
} else {
parsedText += `\n\n<command name="${commandName}">\nCommand '${commandName}' not found. Available commands can be found in .roo/commands/ or ~/.roo/commands/\n</command>`
}
// Note: We don't add error messages for non-existent commands anymore
Copy link
Contributor Author

@roomote roomote bot Jul 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment could be more specific about the reasoning. Perhaps: 'Non-existent commands are left unchanged in the original text as they failed the existence check during the first pass.'

// since we only process commands that we've already verified exist
} catch (error) {
Copy link
Contributor Author

@roomote roomote bot Jul 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error handling inconsistency: If getCommand() fails here after passing the existence check, we show an error message. This creates a scenario where a 'verified existing' command can still fail. Is this the intended behavior?

parsedText += `\n\n<command name="${commandName}">\nError loading command '${commandName}': ${error.message}\n</command>`
}
Expand Down