Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
187 changes: 187 additions & 0 deletions src/core/mentions/__tests__/parseMentions-skills.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,187 @@
import { describe, it, expect, vi, beforeEach } from "vitest"
import { parseMentions } from "../index"
import type { SkillsManager } from "../../../services/skills/SkillsManager"

describe("parseMentions - Skill Resolution", () => {
let mockSkillsManager: Partial<SkillsManager>
let mockUrlContentFetcher: any

beforeEach(() => {
mockUrlContentFetcher = {
launchBrowser: vi.fn(),
urlToMarkdown: vi.fn(),
closeBrowser: vi.fn(),
}

mockSkillsManager = {
getSkillContent: vi.fn(),
}
})

it("should replace $skill-name tokens with placeholders", async () => {
vi.mocked(mockSkillsManager.getSkillContent!).mockResolvedValue({
name: "pdf-processing",
description: "Extract text from PDFs",
path: "/path/to/skill/SKILL.md",
source: "global",
instructions: "# PDF Processing\n\nInstructions here",
})

const result = await parseMentions(
"Please help with $pdf-processing task",
"/workspace",
mockUrlContentFetcher,
undefined,
undefined,
false,
true,
50,
undefined,
mockSkillsManager as SkillsManager,
"code",
)

expect(result.text).toContain("Skill '$pdf-processing' (see below for skill content)")
expect(result.text).toContain('<skill name="pdf-processing">')
expect(result.text).toContain("# PDF Processing")
expect(result.text).toContain("Instructions here")
expect(result.text).toContain("</skill>")
})

it("should handle multiple skills in one message", async () => {
vi.mocked(mockSkillsManager.getSkillContent!).mockImplementation(async (name: string) => {
const skills: Record<string, any> = {
"pdf-processing": {
name: "pdf-processing",
description: "Extract text from PDFs",
path: "/path/to/pdf/SKILL.md",
source: "global",
instructions: "# PDF Processing",
},
"code-review": {
name: "code-review",
description: "Review code",
path: "/path/to/review/SKILL.md",
source: "global",
instructions: "# Code Review",
},
}
return skills[name] || null
})

const result = await parseMentions(
"Use $pdf-processing and $code-review",
"/workspace",
mockUrlContentFetcher,
undefined,
undefined,
false,
true,
50,
undefined,
mockSkillsManager as SkillsManager,
"code",
)

expect(result.text).toContain("Skill '$pdf-processing'")
expect(result.text).toContain("Skill '$code-review'")
expect(result.text).toContain('<skill name="pdf-processing">')
expect(result.text).toContain('<skill name="code-review">')
})

it("should handle invalid skill names gracefully", async () => {
vi.mocked(mockSkillsManager.getSkillContent!).mockResolvedValue(null)

const result = await parseMentions(
"Use $nonexistent skill",
"/workspace",
mockUrlContentFetcher,
undefined,
undefined,
false,
true,
50,
undefined,
mockSkillsManager as SkillsManager,
"code",
)

// Should not replace invalid skills
expect(result.text).toBe("Use $nonexistent skill")
expect(result.text).not.toContain("<skill")
})

it("should work without skillsManager", async () => {
const result = await parseMentions(
"Use $pdf-processing",
"/workspace",
mockUrlContentFetcher,
undefined,
undefined,
false,
true,
50,
undefined,
undefined,
"code",
)

// Should not process skills without manager
expect(result.text).toBe("Use $pdf-processing")
expect(result.text).not.toContain("<skill")
})

it("should handle skills with @ mentions and / commands", async () => {
vi.mocked(mockSkillsManager.getSkillContent!).mockResolvedValue({
name: "pdf-processing",
description: "Extract text from PDFs",
path: "/path/to/skill/SKILL.md",
source: "global",
instructions: "# PDF Processing",
})

const result = await parseMentions(
"Use $pdf-processing on @/test.pdf",
"/workspace",
mockUrlContentFetcher,
undefined,
undefined,
false,
true,
50,
undefined,
mockSkillsManager as SkillsManager,
"code",
)

// Both should be processed
expect(result.text).toContain("Skill '$pdf-processing'")
expect(result.text).toContain("'test.pdf' (see below for file content)")
})

it("should handle skill names with hyphens and underscores", async () => {
vi.mocked(mockSkillsManager.getSkillContent!).mockResolvedValue({
name: "my-special_skill",
description: "A test skill",
path: "/path/to/skill/SKILL.md",
source: "global",
instructions: "# My Skill",
})

const result = await parseMentions(
"Use $my-special_skill",
"/workspace",
mockUrlContentFetcher,
undefined,
undefined,
false,
true,
50,
undefined,
mockSkillsManager as SkillsManager,
"code",
)

expect(result.text).toContain("Skill '$my-special_skill'")
})
})
51 changes: 50 additions & 1 deletion src/core/mentions/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import * as path from "path"
import * as vscode from "vscode"
import { isBinaryFile } from "isbinaryfile"

import { mentionRegexGlobal, commandRegexGlobal, unescapeSpaces } from "../../shared/context-mentions"
import { mentionRegexGlobal, commandRegexGlobal, skillRegexGlobal, unescapeSpaces } from "../../shared/context-mentions"

import { getCommitInfo, getWorkingState } from "../../utils/git"

Expand All @@ -18,6 +18,7 @@ import { FileContextTracker } from "../context-tracking/FileContextTracker"

import { RooIgnoreController } from "../ignore/RooIgnoreController"
import { getCommand, type Command } from "../../services/command/commands"
import { SkillsManager } from "../../services/skills/SkillsManager"

import { t } from "../../i18n"

Expand Down Expand Up @@ -86,9 +87,12 @@ export async function parseMentions(
includeDiagnosticMessages: boolean = true,
maxDiagnosticMessages: number = 50,
maxReadFileLine?: number,
skillsManager?: SkillsManager,
currentMode?: string,
): Promise<ParseMentionsResult> {
const mentions: Set<string> = new Set()
const validCommands: Map<string, Command> = new Map()
const validSkills: Map<string, { name: string; path: string }> = new Map()
let commandMode: string | undefined // Track mode from the first slash command that has one

// First pass: check which command mentions exist and cache the results
Expand Down Expand Up @@ -118,6 +122,31 @@ export async function parseMentions(
}
}

// Check which skill mentions exist and cache the results
const skillMatches = Array.from(text.matchAll(skillRegexGlobal))
const uniqueSkillNames = new Set(skillMatches.map(([, skillName]) => skillName))

if (skillsManager && currentMode) {
const skillExistenceChecks = await Promise.all(
Array.from(uniqueSkillNames).map(async (skillName) => {
try {
const skillContent = await skillsManager.getSkillContent(skillName, currentMode)
return { skillName, skillContent }
} catch (error) {
// If there's an error checking skill existence, treat it as non-existent
return { skillName, skillContent: undefined }
}
}),
)

// Store valid skills for later use
for (const { skillName, skillContent } of skillExistenceChecks) {
if (skillContent) {
validSkills.set(skillName, { name: skillContent.name, path: skillContent.path })
}
}
}

// Only replace text for commands that actually exist
let parsedText = text
for (const [match, commandName] of commandMatches) {
Expand All @@ -126,6 +155,13 @@ export async function parseMentions(
}
}

// Replace skill mentions with placeholders
for (const [match, skillName] of skillMatches) {
if (validSkills.has(skillName)) {
parsedText = parsedText.replace(match, `Skill '$${skillName}' (see below for skill content)`)
}
}

// Second pass: handle regular mentions
parsedText = parsedText.replace(mentionRegexGlobal, (match, mention) => {
mentions.add(mention)
Expand Down Expand Up @@ -259,6 +295,19 @@ export async function parseMentions(
}
}

// Process valid skill mentions using cached results
if (skillsManager) {
for (const [skillName, skillInfo] of validSkills) {
try {
// Read the full SKILL.md content
const skillContent = await fs.readFile(skillInfo.path, "utf-8")
parsedText += `\n\n<skill name="${skillName}">\n${skillContent}\n</skill>`
} catch (error) {
parsedText += `\n\n<skill name="${skillName}">\nError loading skill '${skillName}': ${error.message}\n</skill>`
}
Comment on lines +301 to +307
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This reads the skill content from the filesystem, but getSkillContent() already reads and parses the file (returning content in its instructions field). This creates two issues:

  1. Redundant file I/O: The skill file is read twice - once in the validation loop via getSkillContent() and again here.

  2. Test mismatch: The tests mock getSkillContent to return skill metadata with instructions, but this code ignores that and reads from the path directly. Since mocked paths like /path/to/skill/SKILL.md don't exist, fs.readFile throws and the tests would fail unless fs is globally mocked.

Consider storing the skill content during validation and reusing it here, or using the instructions field from SkillContent.

Suggested change
try {
// Read the full SKILL.md content
const skillContent = await fs.readFile(skillInfo.path, "utf-8")
parsedText += `\n\n<skill name="${skillName}">\n${skillContent}\n</skill>`
} catch (error) {
parsedText += `\n\n<skill name="${skillName}">\nError loading skill '${skillName}': ${error.message}\n</skill>`
}
// Use the skill content that was already fetched during validation
const skillFullContent = await skillsManager.getSkillContent(skillName, currentMode)
if (skillFullContent) {
parsedText += `\n\n<skill name="${skillName}">\n${skillFullContent.instructions}\n</skill>`
}

Fix it with Roo Code or mention @roomote and request a fix.

}
}

if (urlMention) {
try {
await urlContentFetcher.closeBrowser()
Expand Down
11 changes: 11 additions & 0 deletions src/core/mentions/processUserContentMentions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { Anthropic } from "@anthropic-ai/sdk"
import { parseMentions, ParseMentionsResult } from "./index"
import { UrlContentFetcher } from "../../services/browser/UrlContentFetcher"
import { FileContextTracker } from "../context-tracking/FileContextTracker"
import { SkillsManager } from "../../services/skills/SkillsManager"

export interface ProcessUserContentMentionsResult {
content: Anthropic.Messages.ContentBlockParam[]
Expand All @@ -21,6 +22,8 @@ export async function processUserContentMentions({
includeDiagnosticMessages = true,
maxDiagnosticMessages = 50,
maxReadFileLine,
skillsManager,
currentMode,
}: {
userContent: Anthropic.Messages.ContentBlockParam[]
cwd: string
Expand All @@ -31,6 +34,8 @@ export async function processUserContentMentions({
includeDiagnosticMessages?: boolean
maxDiagnosticMessages?: number
maxReadFileLine?: number
skillsManager?: SkillsManager
currentMode?: string
}): Promise<ProcessUserContentMentionsResult> {
// Track the first mode found from slash commands
let commandMode: string | undefined
Expand Down Expand Up @@ -65,6 +70,8 @@ export async function processUserContentMentions({
includeDiagnosticMessages,
maxDiagnosticMessages,
maxReadFileLine,
skillsManager,
currentMode,
)
// Capture the first mode found
if (!commandMode && result.mode) {
Expand All @@ -90,6 +97,8 @@ export async function processUserContentMentions({
includeDiagnosticMessages,
maxDiagnosticMessages,
maxReadFileLine,
skillsManager,
currentMode,
)
// Capture the first mode found
if (!commandMode && result.mode) {
Expand All @@ -116,6 +125,8 @@ export async function processUserContentMentions({
includeDiagnosticMessages,
maxDiagnosticMessages,
maxReadFileLine,
skillsManager,
currentMode,
)
// Capture the first mode found
if (!commandMode && result.mode) {
Expand Down
7 changes: 6 additions & 1 deletion src/core/task/Task.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2372,6 +2372,10 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
maxReadFileLine = -1,
} = (await this.providerRef.deref()?.getState()) ?? {}

const provider = this.providerRef.deref()
const skillsManager = provider?.getSkillsManager()
const currentMode = await this.getTaskMode()

const { content: parsedUserContent, mode: slashCommandMode } = await processUserContentMentions({
userContent: currentUserContent,
cwd: this.cwd,
Expand All @@ -2382,11 +2386,12 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
includeDiagnosticMessages,
maxDiagnosticMessages,
maxReadFileLine,
skillsManager,
currentMode,
})

// Switch mode if specified in a slash command's frontmatter
if (slashCommandMode) {
const provider = this.providerRef.deref()
if (provider) {
const state = await provider.getState()
const targetMode = getModeBySlug(slashCommandMode, state?.customModes)
Expand Down
Loading
Loading