From bd6e16d41be73e3c7ceaf69c5dfab3fbeefd872b Mon Sep 17 00:00:00 2001 From: tanzhenxin Date: Wed, 10 Dec 2025 17:18:44 +0800 Subject: [PATCH] draft version of skill tool feature --- packages/core/src/config/config.ts | 9 + packages/core/src/config/storage.ts | 4 + packages/core/src/index.ts | 4 + packages/core/src/skills/index.ts | 31 ++ .../core/src/skills/skill-manager.test.ts | 463 ++++++++++++++++++ packages/core/src/skills/skill-manager.ts | 452 +++++++++++++++++ packages/core/src/skills/types.ts | 105 ++++ packages/core/src/telemetry/constants.ts | 1 + packages/core/src/telemetry/index.ts | 2 + packages/core/src/telemetry/loggers.test.ts | 2 + packages/core/src/telemetry/loggers.ts | 22 + .../src/telemetry/qwen-logger/qwen-logger.ts | 15 + packages/core/src/telemetry/types.ts | 39 +- packages/core/src/tools/ls.test.ts | 7 +- packages/core/src/tools/ls.ts | 9 +- packages/core/src/tools/read-file.test.ts | 1 + packages/core/src/tools/read-file.ts | 18 +- packages/core/src/tools/skill.test.ts | 442 +++++++++++++++++ packages/core/src/tools/skill.ts | 264 ++++++++++ packages/core/src/tools/task.ts | 4 + packages/core/src/tools/tool-names.ts | 2 + packages/core/src/utils/paths.ts | 4 + 22 files changed, 1891 insertions(+), 9 deletions(-) create mode 100644 packages/core/src/skills/index.ts create mode 100644 packages/core/src/skills/skill-manager.test.ts create mode 100644 packages/core/src/skills/skill-manager.ts create mode 100644 packages/core/src/skills/types.ts create mode 100644 packages/core/src/tools/skill.test.ts create mode 100644 packages/core/src/tools/skill.ts diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 6383cb17..04ff2153 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -54,6 +54,7 @@ import { canUseRipgrep } from '../utils/ripgrepUtils.js'; import { RipGrepTool } from '../tools/ripGrep.js'; import { ShellTool } from '../tools/shell.js'; import { SmartEditTool } from '../tools/smart-edit.js'; +import { SkillTool } from '../tools/skill.js'; import { TaskTool } from '../tools/task.js'; import { TodoWriteTool } from '../tools/todoWrite.js'; import { ToolRegistry } from '../tools/tool-registry.js'; @@ -65,6 +66,7 @@ import { WriteFileTool } from '../tools/write-file.js'; import { ideContextStore } from '../ide/ideContext.js'; import { InputFormat, OutputFormat } from '../output/types.js'; import { PromptRegistry } from '../prompts/prompt-registry.js'; +import { SkillManager } from '../skills/skill-manager.js'; import { SubagentManager } from '../subagents/subagent-manager.js'; import type { SubagentConfig } from '../subagents/types.js'; import { @@ -387,6 +389,7 @@ export class Config { private toolRegistry!: ToolRegistry; private promptRegistry!: PromptRegistry; private subagentManager!: SubagentManager; + private skillManager!: SkillManager; private fileSystemService: FileSystemService; private contentGeneratorConfig!: ContentGeneratorConfig; private contentGenerator!: ContentGenerator; @@ -635,6 +638,7 @@ export class Config { } this.promptRegistry = new PromptRegistry(); this.subagentManager = new SubagentManager(this); + this.skillManager = new SkillManager(this); // Load session subagents if they were provided before initialization if (this.sessionSubagents.length > 0) { @@ -1285,6 +1289,10 @@ export class Config { return this.subagentManager; } + getSkillManager(): SkillManager { + return this.skillManager; + } + async createToolRegistry( sendSdkMcpMessage?: SendSdkMcpMessage, ): Promise { @@ -1327,6 +1335,7 @@ export class Config { }; registerCoreTool(TaskTool, this); + registerCoreTool(SkillTool, this); registerCoreTool(LSTool, this); registerCoreTool(ReadFileTool, this); diff --git a/packages/core/src/config/storage.ts b/packages/core/src/config/storage.ts index 8e598787..9b5af2d4 100644 --- a/packages/core/src/config/storage.ts +++ b/packages/core/src/config/storage.ts @@ -121,6 +121,10 @@ export class Storage { return path.join(this.getExtensionsDir(), 'qwen-extension.json'); } + getUserSkillsDir(): string { + return path.join(Storage.getGlobalQwenDir(), 'skills'); + } + getHistoryFilePath(): string { return path.join(this.getProjectTempDir(), 'shell_history'); } diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 738aca57..fdb6effc 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -85,6 +85,9 @@ export * from './tools/tool-registry.js'; // Export subagents (Phase 1) export * from './subagents/index.js'; +// Export skills +export * from './skills/index.js'; + // Export prompt logic export * from './prompts/mcp-prompts.js'; @@ -106,6 +109,7 @@ export * from './tools/mcp-client-manager.js'; export * from './tools/mcp-tool.js'; export * from './tools/sdk-control-client-transport.js'; export * from './tools/task.js'; +export * from './tools/skill.js'; export * from './tools/todoWrite.js'; export * from './tools/exitPlanMode.js'; diff --git a/packages/core/src/skills/index.ts b/packages/core/src/skills/index.ts new file mode 100644 index 00000000..94d5869f --- /dev/null +++ b/packages/core/src/skills/index.ts @@ -0,0 +1,31 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * @fileoverview Skills feature implementation + * + * This module provides the foundation for the skills feature, which allows + * users to define reusable skill configurations that can be loaded by the + * model via a dedicated Skills tool. + * + * Skills are stored as directories in `.qwen/skills/` (project-level) or + * `~/.qwen/skills/` (user-level), with each directory containing a SKILL.md + * file with YAML frontmatter for metadata. + */ + +// Core types and interfaces +export type { + SkillConfig, + SkillLevel, + SkillValidationResult, + ListSkillsOptions, + SkillErrorCode, +} from './types.js'; + +export { SkillError } from './types.js'; + +// Main management class +export { SkillManager } from './skill-manager.js'; diff --git a/packages/core/src/skills/skill-manager.test.ts b/packages/core/src/skills/skill-manager.test.ts new file mode 100644 index 00000000..076816f8 --- /dev/null +++ b/packages/core/src/skills/skill-manager.test.ts @@ -0,0 +1,463 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { vi, describe, it, expect, beforeEach, afterEach } from 'vitest'; +import * as fs from 'fs/promises'; +import * as path from 'path'; +import * as os from 'os'; +import { SkillManager } from './skill-manager.js'; +import { type SkillConfig, SkillError } from './types.js'; +import type { Config } from '../config/config.js'; +import { makeFakeConfig } from '../test-utils/config.js'; + +// Mock file system operations +vi.mock('fs/promises'); +vi.mock('os'); + +// Mock yaml parser - use vi.hoisted for proper hoisting +const mockParseYaml = vi.hoisted(() => vi.fn()); + +vi.mock('../utils/yaml-parser.js', () => ({ + parse: mockParseYaml, + stringify: vi.fn(), +})); + +describe('SkillManager', () => { + let manager: SkillManager; + let mockConfig: Config; + + beforeEach(() => { + // Create mock Config object using test utility + mockConfig = makeFakeConfig({}); + + // Mock the project root method + vi.spyOn(mockConfig, 'getProjectRoot').mockReturnValue('/test/project'); + + // Mock os.homedir + vi.mocked(os.homedir).mockReturnValue('/home/user'); + + // Reset and setup mocks + vi.clearAllMocks(); + + // Setup yaml parser mocks with sophisticated behavior + mockParseYaml.mockImplementation((yamlString: string) => { + // Handle different test cases based on YAML content + if (yamlString.includes('allowedTools:')) { + return { + name: 'test-skill', + description: 'A test skill', + allowedTools: ['read_file', 'write_file'], + }; + } + if (yamlString.includes('name: skill1')) { + return { name: 'skill1', description: 'First skill' }; + } + if (yamlString.includes('name: skill2')) { + return { name: 'skill2', description: 'Second skill' }; + } + if (yamlString.includes('name: skill3')) { + return { name: 'skill3', description: 'Third skill' }; + } + if (!yamlString.includes('name:')) { + return { description: 'A test skill' }; // Missing name case + } + if (!yamlString.includes('description:')) { + return { name: 'test-skill' }; // Missing description case + } + // Default case + return { + name: 'test-skill', + description: 'A test skill', + }; + }); + + manager = new SkillManager(mockConfig); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + const validSkillConfig: SkillConfig = { + name: 'test-skill', + description: 'A test skill', + level: 'project', + filePath: '/test/project/.qwen/skills/test-skill/SKILL.md', + body: 'You are a helpful assistant with this skill.', + }; + + const validMarkdown = `--- +name: test-skill +description: A test skill +--- + +You are a helpful assistant with this skill. +`; + + describe('parseSkillContent', () => { + it('should parse valid markdown content', () => { + const config = manager.parseSkillContent( + validMarkdown, + validSkillConfig.filePath, + 'project', + ); + + expect(config.name).toBe('test-skill'); + expect(config.description).toBe('A test skill'); + expect(config.body).toBe('You are a helpful assistant with this skill.'); + expect(config.level).toBe('project'); + expect(config.filePath).toBe(validSkillConfig.filePath); + }); + + it('should parse content with allowedTools', () => { + const markdownWithTools = `--- +name: test-skill +description: A test skill +allowedTools: + - read_file + - write_file +--- + +You are a helpful assistant with this skill. +`; + + const config = manager.parseSkillContent( + markdownWithTools, + validSkillConfig.filePath, + 'project', + ); + + expect(config.allowedTools).toEqual(['read_file', 'write_file']); + }); + + it('should determine level from file path', () => { + const projectPath = '/test/project/.qwen/skills/test-skill/SKILL.md'; + const userPath = '/home/user/.qwen/skills/test-skill/SKILL.md'; + + const projectConfig = manager.parseSkillContent( + validMarkdown, + projectPath, + 'project', + ); + const userConfig = manager.parseSkillContent( + validMarkdown, + userPath, + 'user', + ); + + expect(projectConfig.level).toBe('project'); + expect(userConfig.level).toBe('user'); + }); + + it('should throw error for invalid frontmatter format', () => { + const invalidMarkdown = `No frontmatter here +Just content`; + + expect(() => + manager.parseSkillContent( + invalidMarkdown, + validSkillConfig.filePath, + 'project', + ), + ).toThrow(SkillError); + }); + + it('should throw error for missing name', () => { + const markdownWithoutName = `--- +description: A test skill +--- + +You are a helpful assistant. +`; + + expect(() => + manager.parseSkillContent( + markdownWithoutName, + validSkillConfig.filePath, + 'project', + ), + ).toThrow(SkillError); + }); + + it('should throw error for missing description', () => { + const markdownWithoutDescription = `--- +name: test-skill +--- + +You are a helpful assistant. +`; + + expect(() => + manager.parseSkillContent( + markdownWithoutDescription, + validSkillConfig.filePath, + 'project', + ), + ).toThrow(SkillError); + }); + }); + + describe('validateConfig', () => { + it('should validate valid configuration', () => { + const result = manager.validateConfig(validSkillConfig); + + expect(result.isValid).toBe(true); + expect(result.errors).toHaveLength(0); + }); + + it('should report error for missing name', () => { + const invalidConfig = { ...validSkillConfig, name: '' }; + const result = manager.validateConfig(invalidConfig); + + expect(result.isValid).toBe(false); + expect(result.errors).toContain('"name" cannot be empty'); + }); + + it('should report error for missing description', () => { + const invalidConfig = { ...validSkillConfig, description: '' }; + const result = manager.validateConfig(invalidConfig); + + expect(result.isValid).toBe(false); + expect(result.errors).toContain('"description" cannot be empty'); + }); + + it('should report error for invalid allowedTools type', () => { + const invalidConfig = { + ...validSkillConfig, + allowedTools: 'not-an-array' as unknown as string[], + }; + const result = manager.validateConfig(invalidConfig); + + expect(result.isValid).toBe(false); + expect(result.errors).toContain('"allowedTools" must be an array'); + }); + + it('should warn for empty body', () => { + const configWithEmptyBody = { ...validSkillConfig, body: '' }; + const result = manager.validateConfig(configWithEmptyBody); + + expect(result.isValid).toBe(true); // Still valid + expect(result.warnings).toContain('Skill body is empty'); + }); + }); + + describe('loadSkill', () => { + it('should load skill from project level first', async () => { + vi.mocked(fs.readdir).mockResolvedValue([ + { name: 'test-skill', isDirectory: () => true, isFile: () => false }, + ] as unknown as Awaited>); + vi.mocked(fs.access).mockResolvedValue(undefined); + vi.mocked(fs.readFile).mockResolvedValue(validMarkdown); + + const config = await manager.loadSkill('test-skill'); + + expect(config).toBeDefined(); + expect(config!.name).toBe('test-skill'); + }); + + it('should fall back to user level if project level fails', async () => { + vi.mocked(fs.readdir) + .mockRejectedValueOnce(new Error('Project dir not found')) // project level fails + .mockResolvedValueOnce([ + { name: 'test-skill', isDirectory: () => true, isFile: () => false }, + ] as unknown as Awaited>); // user level succeeds + vi.mocked(fs.access).mockResolvedValue(undefined); + vi.mocked(fs.readFile).mockResolvedValue(validMarkdown); + + const config = await manager.loadSkill('test-skill'); + + expect(config).toBeDefined(); + expect(config!.name).toBe('test-skill'); + }); + + it('should return null if not found at either level', async () => { + vi.mocked(fs.readdir).mockRejectedValue(new Error('Directory not found')); + + const config = await manager.loadSkill('nonexistent'); + + expect(config).toBeNull(); + }); + }); + + describe('loadSkillForRuntime', () => { + it('should load skill for runtime', async () => { + vi.mocked(fs.readdir).mockResolvedValueOnce([ + { name: 'test-skill', isDirectory: () => true, isFile: () => false }, + ] as unknown as Awaited>); + + vi.mocked(fs.access).mockResolvedValue(undefined); + vi.mocked(fs.readFile).mockResolvedValue(validMarkdown); // SKILL.md + + const config = await manager.loadSkillForRuntime('test-skill'); + + expect(config).toBeDefined(); + expect(config!.name).toBe('test-skill'); + }); + + it('should return null if skill not found', async () => { + vi.mocked(fs.readdir).mockRejectedValue(new Error('Directory not found')); + + const config = await manager.loadSkillForRuntime('nonexistent'); + + expect(config).toBeNull(); + }); + }); + + describe('listSkills', () => { + beforeEach(() => { + // Mock directory listing for skills directories (with Dirent objects) + vi.mocked(fs.readdir) + .mockResolvedValueOnce([ + { name: 'skill1', isDirectory: () => true, isFile: () => false }, + { name: 'skill2', isDirectory: () => true, isFile: () => false }, + { + name: 'not-a-dir.txt', + isDirectory: () => false, + isFile: () => true, + }, + ] as unknown as Awaited>) + .mockResolvedValueOnce([ + { name: 'skill3', isDirectory: () => true, isFile: () => false }, + { name: 'skill1', isDirectory: () => true, isFile: () => false }, + ] as unknown as Awaited>); + + vi.mocked(fs.access).mockResolvedValue(undefined); + + // Mock file reading for valid skills + vi.mocked(fs.readFile).mockImplementation((filePath) => { + const pathStr = String(filePath); + if (pathStr.includes('skill1')) { + return Promise.resolve(`--- +name: skill1 +description: First skill +--- +Skill 1 content`); + } else if (pathStr.includes('skill2')) { + return Promise.resolve(`--- +name: skill2 +description: Second skill +--- +Skill 2 content`); + } else if (pathStr.includes('skill3')) { + return Promise.resolve(`--- +name: skill3 +description: Third skill +--- +Skill 3 content`); + } + return Promise.reject(new Error('File not found')); + }); + }); + + it('should list skills from both levels', async () => { + const skills = await manager.listSkills(); + + expect(skills).toHaveLength(3); // skill1 (project takes precedence), skill2, skill3 + expect(skills.map((s) => s.name).sort()).toEqual([ + 'skill1', + 'skill2', + 'skill3', + ]); + }); + + it('should prioritize project level over user level', async () => { + const skills = await manager.listSkills(); + const skill1 = skills.find((s) => s.name === 'skill1'); + + expect(skill1!.level).toBe('project'); + }); + + it('should filter by level', async () => { + const projectSkills = await manager.listSkills({ + level: 'project', + }); + + expect(projectSkills).toHaveLength(2); // skill1, skill2 + expect(projectSkills.every((s) => s.level === 'project')).toBe(true); + }); + + it('should handle empty directories', async () => { + vi.mocked(fs.readdir).mockReset(); + vi.mocked(fs.readdir).mockResolvedValue( + [] as unknown as Awaited>, + ); + + const skills = await manager.listSkills({ force: true }); + + expect(skills).toHaveLength(0); + }); + + it('should handle directory read errors', async () => { + vi.mocked(fs.readdir).mockReset(); + vi.mocked(fs.readdir).mockRejectedValue(new Error('Directory not found')); + + const skills = await manager.listSkills({ force: true }); + + expect(skills).toHaveLength(0); + }); + }); + + describe('getSkillsBaseDir', () => { + it('should return project-level base dir', () => { + const baseDir = manager.getSkillsBaseDir('project'); + + expect(baseDir).toBe(path.join('/test/project', '.qwen', 'skills')); + }); + + it('should return user-level base dir', () => { + const baseDir = manager.getSkillsBaseDir('user'); + + expect(baseDir).toBe(path.join('/home/user', '.qwen', 'skills')); + }); + }); + + describe('change listeners', () => { + it('should notify listeners when cache is refreshed', async () => { + const listener = vi.fn(); + manager.addChangeListener(listener); + + vi.mocked(fs.readdir).mockResolvedValue( + [] as unknown as Awaited>, + ); + + await manager.refreshCache(); + + expect(listener).toHaveBeenCalled(); + }); + + it('should remove listener when cleanup function is called', async () => { + const listener = vi.fn(); + const removeListener = manager.addChangeListener(listener); + + removeListener(); + + vi.mocked(fs.readdir).mockResolvedValue( + [] as unknown as Awaited>, + ); + + await manager.refreshCache(); + + expect(listener).not.toHaveBeenCalled(); + }); + }); + + describe('parse errors', () => { + it('should track parse errors', async () => { + vi.mocked(fs.readdir).mockResolvedValue([ + { name: 'bad-skill', isDirectory: () => true, isFile: () => false }, + ] as unknown as Awaited>); + vi.mocked(fs.access).mockResolvedValue(undefined); + vi.mocked(fs.readFile).mockResolvedValue( + 'invalid content without frontmatter', + ); + + await manager.listSkills({ force: true }); + + const errors = manager.getParseErrors(); + expect(errors.size).toBeGreaterThan(0); + }); + }); +}); diff --git a/packages/core/src/skills/skill-manager.ts b/packages/core/src/skills/skill-manager.ts new file mode 100644 index 00000000..77cec15f --- /dev/null +++ b/packages/core/src/skills/skill-manager.ts @@ -0,0 +1,452 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import * as fs from 'fs/promises'; +import * as path from 'path'; +import * as os from 'os'; +import { parse as parseYaml } from '../utils/yaml-parser.js'; +import type { + SkillConfig, + SkillLevel, + ListSkillsOptions, + SkillValidationResult, +} from './types.js'; +import { SkillError, SkillErrorCode } from './types.js'; +import type { Config } from '../config/config.js'; + +const QWEN_CONFIG_DIR = '.qwen'; +const SKILLS_CONFIG_DIR = 'skills'; +const SKILL_MANIFEST_FILE = 'SKILL.md'; + +/** + * Manages skill configurations stored as directories containing SKILL.md files. + * Provides discovery, parsing, validation, and caching for skills. + */ +export class SkillManager { + private skillsCache: Map | null = null; + private readonly changeListeners: Set<() => void> = new Set(); + private parseErrors: Map = new Map(); + + constructor(private readonly config: Config) {} + + /** + * Adds a listener that will be called when skills change. + * @returns A function to remove the listener. + */ + addChangeListener(listener: () => void): () => void { + this.changeListeners.add(listener); + return () => { + this.changeListeners.delete(listener); + }; + } + + /** + * Notifies all registered change listeners. + */ + private notifyChangeListeners(): void { + for (const listener of this.changeListeners) { + try { + listener(); + } catch (error) { + console.warn('Skill change listener threw an error:', error); + } + } + } + + /** + * Gets any parse errors that occurred during skill loading. + * @returns Map of skill paths to their parse errors. + */ + getParseErrors(): Map { + return new Map(this.parseErrors); + } + + /** + * Lists all available skills. + * + * @param options - Filtering options + * @returns Array of skill configurations + */ + async listSkills(options: ListSkillsOptions = {}): Promise { + const skills: SkillConfig[] = []; + const seenNames = new Set(); + + const levelsToCheck: SkillLevel[] = options.level + ? [options.level] + : ['project', 'user']; + + // Check if we should use cache or force refresh + const shouldUseCache = !options.force && this.skillsCache !== null; + + // Initialize cache if it doesn't exist or we're forcing a refresh + if (!shouldUseCache) { + await this.refreshCache(); + } + + // Collect skills from each level (project takes precedence over user) + for (const level of levelsToCheck) { + const levelSkills = this.skillsCache?.get(level) || []; + + for (const skill of levelSkills) { + // Skip if we've already seen this name (precedence: project > user) + if (seenNames.has(skill.name)) { + continue; + } + + skills.push(skill); + seenNames.add(skill.name); + } + } + + // Sort by name for consistent ordering + skills.sort((a, b) => a.name.localeCompare(b.name)); + + return skills; + } + + /** + * Loads a skill configuration by name. + * If level is specified, only searches that level. + * If level is omitted, searches project-level first, then user-level. + * + * @param name - Name of the skill to load + * @param level - Optional level to limit search to + * @returns SkillConfig or null if not found + */ + async loadSkill( + name: string, + level?: SkillLevel, + ): Promise { + if (level) { + return this.findSkillByNameAtLevel(name, level); + } + + // Try project level first + const projectSkill = await this.findSkillByNameAtLevel(name, 'project'); + if (projectSkill) { + return projectSkill; + } + + // Try user level + return this.findSkillByNameAtLevel(name, 'user'); + } + + /** + * Loads a skill with its full content, ready for runtime use. + * This includes loading additional files from the skill directory. + * + * @param name - Name of the skill to load + * @param level - Optional level to limit search to + * @returns SkillConfig or null if not found + */ + async loadSkillForRuntime( + name: string, + level?: SkillLevel, + ): Promise { + const skill = await this.loadSkill(name, level); + if (!skill) { + return null; + } + + return skill; + } + + /** + * Validates a skill configuration. + * + * @param config - Configuration to validate + * @returns Validation result + */ + validateConfig(config: Partial): SkillValidationResult { + const errors: string[] = []; + const warnings: string[] = []; + + // Check required fields + if (typeof config.name !== 'string') { + errors.push('Missing or invalid "name" field'); + } else if (config.name.trim() === '') { + errors.push('"name" cannot be empty'); + } + + if (typeof config.description !== 'string') { + errors.push('Missing or invalid "description" field'); + } else if (config.description.trim() === '') { + errors.push('"description" cannot be empty'); + } + + // Validate allowedTools if present + if (config.allowedTools !== undefined) { + if (!Array.isArray(config.allowedTools)) { + errors.push('"allowedTools" must be an array'); + } else { + for (const tool of config.allowedTools) { + if (typeof tool !== 'string') { + errors.push('"allowedTools" must contain only strings'); + break; + } + } + } + } + + // Warn if body is empty + if (!config.body || config.body.trim() === '') { + warnings.push('Skill body is empty'); + } + + return { + isValid: errors.length === 0, + errors, + warnings, + }; + } + + /** + * Refreshes the skills cache by loading all skills from disk. + */ + async refreshCache(): Promise { + const skillsCache = new Map(); + this.parseErrors.clear(); + + const levels: SkillLevel[] = ['project', 'user']; + + for (const level of levels) { + const levelSkills = await this.listSkillsAtLevel(level); + skillsCache.set(level, levelSkills); + } + + this.skillsCache = skillsCache; + this.notifyChangeListeners(); + } + + /** + * Parses a SKILL.md file and returns the configuration. + * + * @param filePath - Path to the SKILL.md file + * @param level - Storage level + * @returns SkillConfig + * @throws SkillError if parsing fails + */ + parseSkillFile(filePath: string, level: SkillLevel): Promise { + return this.parseSkillFileInternal(filePath, level); + } + + /** + * Internal implementation of skill file parsing. + */ + private async parseSkillFileInternal( + filePath: string, + level: SkillLevel, + ): Promise { + let content: string; + + try { + content = await fs.readFile(filePath, 'utf8'); + } catch (error) { + const skillError = new SkillError( + `Failed to read skill file: ${error instanceof Error ? error.message : 'Unknown error'}`, + SkillErrorCode.FILE_ERROR, + ); + this.parseErrors.set(filePath, skillError); + throw skillError; + } + + return this.parseSkillContent(content, filePath, level); + } + + /** + * Parses skill content from a string. + * + * @param content - File content + * @param filePath - File path for error reporting + * @param level - Storage level + * @returns SkillConfig + * @throws SkillError if parsing fails + */ + parseSkillContent( + content: string, + filePath: string, + level: SkillLevel, + ): SkillConfig { + try { + // Split frontmatter and content + const frontmatterRegex = /^---\n([\s\S]*?)\n---\n([\s\S]*)$/; + const match = content.match(frontmatterRegex); + + if (!match) { + throw new Error('Invalid format: missing YAML frontmatter'); + } + + const [, frontmatterYaml, body] = match; + + // Parse YAML frontmatter + const frontmatter = parseYaml(frontmatterYaml) as Record; + + // Extract required fields + const nameRaw = frontmatter['name']; + const descriptionRaw = frontmatter['description']; + + if (nameRaw == null || nameRaw === '') { + throw new Error('Missing "name" in frontmatter'); + } + + if (descriptionRaw == null || descriptionRaw === '') { + throw new Error('Missing "description" in frontmatter'); + } + + // Convert to strings + const name = String(nameRaw); + const description = String(descriptionRaw); + + // Extract optional fields + const allowedToolsRaw = frontmatter['allowedTools'] as + | unknown[] + | undefined; + let allowedTools: string[] | undefined; + + if (allowedToolsRaw !== undefined) { + if (Array.isArray(allowedToolsRaw)) { + allowedTools = allowedToolsRaw.map(String); + } else { + throw new Error('"allowedTools" must be an array'); + } + } + + const config: SkillConfig = { + name, + description, + allowedTools, + level, + filePath, + body: body.trim(), + }; + + // Validate the parsed configuration + const validation = this.validateConfig(config); + if (!validation.isValid) { + throw new Error(`Validation failed: ${validation.errors.join(', ')}`); + } + + return config; + } catch (error) { + const skillError = new SkillError( + `Failed to parse skill file: ${error instanceof Error ? error.message : 'Unknown error'}`, + SkillErrorCode.PARSE_ERROR, + ); + this.parseErrors.set(filePath, skillError); + throw skillError; + } + } + + /** + * Gets the base directory for skills at a specific level. + * + * @param level - Storage level + * @returns Absolute directory path + */ + getSkillsBaseDir(level: SkillLevel): string { + const baseDir = + level === 'project' + ? path.join( + this.config.getProjectRoot(), + QWEN_CONFIG_DIR, + SKILLS_CONFIG_DIR, + ) + : path.join(os.homedir(), QWEN_CONFIG_DIR, SKILLS_CONFIG_DIR); + + return baseDir; + } + + /** + * Lists skills at a specific level. + * + * @param level - Storage level to scan + * @returns Array of skill configurations + */ + private async listSkillsAtLevel(level: SkillLevel): Promise { + const projectRoot = this.config.getProjectRoot(); + const homeDir = os.homedir(); + const isHomeDirectory = path.resolve(projectRoot) === path.resolve(homeDir); + + // If project level is requested but project root is same as home directory, + // return empty array to avoid conflicts between project and global skills + if (level === 'project' && isHomeDirectory) { + return []; + } + + const baseDir = this.getSkillsBaseDir(level); + + try { + const entries = await fs.readdir(baseDir, { withFileTypes: true }); + const skills: SkillConfig[] = []; + + for (const entry of entries) { + // Only process directories (each skill is a directory) + if (!entry.isDirectory()) continue; + + const skillDir = path.join(baseDir, entry.name); + const skillManifest = path.join(skillDir, SKILL_MANIFEST_FILE); + + try { + // Check if SKILL.md exists + await fs.access(skillManifest); + + const config = await this.parseSkillFileInternal( + skillManifest, + level, + ); + skills.push(config); + } catch (error) { + // Skip directories without valid SKILL.md + if (error instanceof SkillError) { + // Parse error was already recorded + console.warn( + `Failed to parse skill at ${skillDir}: ${error.message}`, + ); + } + continue; + } + } + + return skills; + } catch (_error) { + // Directory doesn't exist or can't be read + return []; + } + } + + /** + * Finds a skill by name at a specific level. + * + * @param name - Name of the skill to find + * @param level - Storage level to search + * @returns SkillConfig or null if not found + */ + private async findSkillByNameAtLevel( + name: string, + level: SkillLevel, + ): Promise { + await this.ensureLevelCache(level); + + const levelSkills = this.skillsCache?.get(level) || []; + + // Find the skill with matching name + return levelSkills.find((skill) => skill.name === name) || null; + } + + /** + * Ensures the cache is populated for a specific level without loading other levels. + */ + private async ensureLevelCache(level: SkillLevel): Promise { + if (!this.skillsCache) { + this.skillsCache = new Map(); + } + + if (!this.skillsCache.has(level)) { + const levelSkills = await this.listSkillsAtLevel(level); + this.skillsCache.set(level, levelSkills); + } + } +} diff --git a/packages/core/src/skills/types.ts b/packages/core/src/skills/types.ts new file mode 100644 index 00000000..75dfe014 --- /dev/null +++ b/packages/core/src/skills/types.ts @@ -0,0 +1,105 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Represents the storage level for a skill configuration. + * - 'project': Stored in `.qwen/skills/` within the project directory + * - 'user': Stored in `~/.qwen/skills/` in the user's home directory + */ +export type SkillLevel = 'project' | 'user'; + +/** + * Core configuration for a skill as stored in SKILL.md files. + * Each skill directory contains a SKILL.md file with YAML frontmatter + * containing metadata, followed by markdown content describing the skill. + */ +export interface SkillConfig { + /** Unique name identifier for the skill */ + name: string; + + /** Human-readable description of what this skill provides */ + description: string; + + /** + * Optional list of tool names that this skill is allowed to use. + * For v1, this is informational only (no gating). + */ + allowedTools?: string[]; + + /** + * Storage level - determines where the configuration file is stored + */ + level: SkillLevel; + + /** + * Absolute path to the skill directory containing SKILL.md + */ + filePath: string; + + /** + * The markdown body content from SKILL.md (after the frontmatter) + */ + body: string; +} + +/** + * Runtime configuration for a skill when it's being actively used. + * Extends SkillConfig with additional runtime-specific fields. + */ +export type SkillRuntimeConfig = SkillConfig; + +/** + * Result of a validation operation on a skill configuration. + */ +export interface SkillValidationResult { + /** Whether the configuration is valid */ + isValid: boolean; + + /** Array of error messages if validation failed */ + errors: string[]; + + /** Array of warning messages (non-blocking issues) */ + warnings: string[]; +} + +/** + * Options for listing skills. + */ +export interface ListSkillsOptions { + /** Filter by storage level */ + level?: SkillLevel; + + /** Force refresh from disk, bypassing cache. Defaults to false. */ + force?: boolean; +} + +/** + * Error thrown when a skill operation fails. + */ +export class SkillError extends Error { + constructor( + message: string, + readonly code: SkillErrorCode, + readonly skillName?: string, + ) { + super(message); + this.name = 'SkillError'; + } +} + +/** + * Error codes for skill operations. + */ +export const SkillErrorCode = { + NOT_FOUND: 'NOT_FOUND', + INVALID_CONFIG: 'INVALID_CONFIG', + INVALID_NAME: 'INVALID_NAME', + FILE_ERROR: 'FILE_ERROR', + PARSE_ERROR: 'PARSE_ERROR', +} as const; + +export type SkillErrorCode = + (typeof SkillErrorCode)[keyof typeof SkillErrorCode]; diff --git a/packages/core/src/telemetry/constants.ts b/packages/core/src/telemetry/constants.ts index bc654637..acbf5602 100644 --- a/packages/core/src/telemetry/constants.ts +++ b/packages/core/src/telemetry/constants.ts @@ -33,6 +33,7 @@ export const EVENT_MALFORMED_JSON_RESPONSE = export const EVENT_FILE_OPERATION = 'qwen-code.file_operation'; export const EVENT_MODEL_SLASH_COMMAND = 'qwen-code.slash_command.model'; export const EVENT_SUBAGENT_EXECUTION = 'qwen-code.subagent_execution'; +export const EVENT_SKILL_LAUNCH = 'qwen-code.skill_launch'; export const EVENT_AUTH = 'qwen-code.auth'; // Performance Events diff --git a/packages/core/src/telemetry/index.ts b/packages/core/src/telemetry/index.ts index 9adb2d32..d0feb020 100644 --- a/packages/core/src/telemetry/index.ts +++ b/packages/core/src/telemetry/index.ts @@ -44,6 +44,7 @@ export { logRipgrepFallback, logNextSpeakerCheck, logAuth, + logSkillLaunch, } from './loggers.js'; export type { SlashCommandEvent, ChatCompressionEvent } from './types.js'; export { @@ -63,6 +64,7 @@ export { RipgrepFallbackEvent, NextSpeakerCheckEvent, AuthEvent, + SkillLaunchEvent, } from './types.js'; export { makeSlashCommandEvent, makeChatCompressionEvent } from './types.js'; export type { TelemetryEvent } from './types.js'; diff --git a/packages/core/src/telemetry/loggers.test.ts b/packages/core/src/telemetry/loggers.test.ts index 1167cc6a..0a2d57d4 100644 --- a/packages/core/src/telemetry/loggers.test.ts +++ b/packages/core/src/telemetry/loggers.test.ts @@ -206,6 +206,8 @@ describe('loggers', () => { mcp_tools: undefined, mcp_tools_count: undefined, output_format: 'json', + skills: undefined, + subagents: undefined, }, }); }); diff --git a/packages/core/src/telemetry/loggers.ts b/packages/core/src/telemetry/loggers.ts index b7039a55..19616e25 100644 --- a/packages/core/src/telemetry/loggers.ts +++ b/packages/core/src/telemetry/loggers.ts @@ -38,6 +38,7 @@ import { EVENT_MALFORMED_JSON_RESPONSE, EVENT_INVALID_CHUNK, EVENT_AUTH, + EVENT_SKILL_LAUNCH, } from './constants.js'; import { recordApiErrorMetrics, @@ -85,6 +86,7 @@ import type { MalformedJsonResponseEvent, InvalidChunkEvent, AuthEvent, + SkillLaunchEvent, } from './types.js'; import type { UiEvent } from './uiTelemetry.js'; import { uiTelemetryService } from './uiTelemetry.js'; @@ -127,6 +129,8 @@ export function logStartSession( mcp_tools: event.mcp_tools, mcp_tools_count: event.mcp_tools_count, output_format: event.output_format, + skills: event.skills, + subagents: event.subagents, }; const logger = logs.getLogger(SERVICE_NAME); @@ -869,3 +873,21 @@ export function logAuth(config: Config, event: AuthEvent): void { }; logger.emit(logRecord); } + +export function logSkillLaunch(config: Config, event: SkillLaunchEvent): void { + if (!isTelemetrySdkInitialized()) return; + + const attributes: LogAttributes = { + ...getCommonAttributes(config), + ...event, + 'event.name': EVENT_SKILL_LAUNCH, + 'event.timestamp': new Date().toISOString(), + }; + + const logger = logs.getLogger(SERVICE_NAME); + const logRecord: LogRecord = { + body: `Skill launch: ${event.skill_name}. Success: ${event.success}.`, + attributes, + }; + logger.emit(logRecord); +} diff --git a/packages/core/src/telemetry/qwen-logger/qwen-logger.ts b/packages/core/src/telemetry/qwen-logger/qwen-logger.ts index b6a97a2e..0a2b8b74 100644 --- a/packages/core/src/telemetry/qwen-logger/qwen-logger.ts +++ b/packages/core/src/telemetry/qwen-logger/qwen-logger.ts @@ -38,6 +38,7 @@ import type { ModelSlashCommandEvent, ExtensionDisableEvent, AuthEvent, + SkillLaunchEvent, RipgrepFallbackEvent, EndSessionEvent, } from '../types.js'; @@ -388,6 +389,8 @@ export class QwenLogger { telemetry_enabled: event.telemetry_enabled, telemetry_log_user_prompts_enabled: event.telemetry_log_user_prompts_enabled, + skills: event.skills, + subagents: event.subagents, }, }); @@ -824,6 +827,18 @@ export class QwenLogger { this.flushIfNeeded(); } + logSkillLaunchEvent(event: SkillLaunchEvent): void { + const rumEvent = this.createActionEvent('misc', 'skill_launch', { + properties: { + skill_name: event.skill_name, + success: event.success ? 1 : 0, + }, + }); + + this.enqueueLogEvent(rumEvent); + this.flushIfNeeded(); + } + logChatCompressionEvent(event: ChatCompressionEvent): void { const rumEvent = this.createActionEvent('misc', 'chat_compression', { properties: { diff --git a/packages/core/src/telemetry/types.ts b/packages/core/src/telemetry/types.ts index cfe4a2a0..4158c905 100644 --- a/packages/core/src/telemetry/types.ts +++ b/packages/core/src/telemetry/types.ts @@ -18,6 +18,9 @@ import { import type { FileOperation } from './metrics.js'; export { ToolCallDecision }; import type { OutputFormat } from '../output/types.js'; +import { ToolNames } from '../tools/tool-names.js'; +import type { SkillTool } from '../tools/skill.js'; +import type { TaskTool } from '../tools/task.js'; export interface BaseTelemetryEvent { 'event.name': string; @@ -47,6 +50,8 @@ export class StartSessionEvent implements BaseTelemetryEvent { mcp_tools_count?: number; mcp_tools?: string; output_format: OutputFormat; + skills?: string; + subagents?: string; constructor(config: Config) { const generatorConfig = config.getContentGeneratorConfig(); @@ -79,6 +84,7 @@ export class StartSessionEvent implements BaseTelemetryEvent { config.getFileFilteringRespectGitIgnore(); this.mcp_servers_count = mcpServers ? Object.keys(mcpServers).length : 0; this.output_format = config.getOutputFormat(); + if (toolRegistry) { const mcpTools = toolRegistry .getAllTools() @@ -87,6 +93,22 @@ export class StartSessionEvent implements BaseTelemetryEvent { this.mcp_tools = mcpTools .map((tool) => (tool as DiscoveredMCPTool).name) .join(','); + + const skillTool = toolRegistry.getTool(ToolNames.SKILL) as + | SkillTool + | undefined; + const skillNames = skillTool?.getAvailableSkillNames?.(); + if (skillNames && skillNames.length > 0) { + this.skills = skillNames.join(','); + } + + const taskTool = toolRegistry.getTool(ToolNames.TASK) as + | TaskTool + | undefined; + const subagentNames = taskTool?.getAvailableSubagentNames?.(); + if (subagentNames && subagentNames.length > 0) { + this.subagents = subagentNames.join(','); + } } } } @@ -721,6 +743,20 @@ export class AuthEvent implements BaseTelemetryEvent { } } +export class SkillLaunchEvent implements BaseTelemetryEvent { + 'event.name': 'skill_launch'; + 'event.timestamp': string; + skill_name: string; + success: boolean; + + constructor(skill_name: string, success: boolean) { + this['event.name'] = 'skill_launch'; + this['event.timestamp'] = new Date().toISOString(); + this.skill_name = skill_name; + this.success = success; + } +} + export type TelemetryEvent = | StartSessionEvent | EndSessionEvent @@ -749,7 +785,8 @@ export type TelemetryEvent = | ExtensionUninstallEvent | ToolOutputTruncatedEvent | ModelSlashCommandEvent - | AuthEvent; + | AuthEvent + | SkillLaunchEvent; export class ExtensionDisableEvent implements BaseTelemetryEvent { 'event.name': 'extension_disable'; diff --git a/packages/core/src/tools/ls.test.ts b/packages/core/src/tools/ls.test.ts index 8eea72cb..3a3f3a0d 100644 --- a/packages/core/src/tools/ls.test.ts +++ b/packages/core/src/tools/ls.test.ts @@ -31,6 +31,8 @@ describe('LSTool', () => { tempSecondaryDir, ]); + const userSkillsBase = path.join(os.homedir(), '.qwen', 'skills'); + mockConfig = { getTargetDir: () => tempRootDir, getWorkspaceContext: () => mockWorkspaceContext, @@ -39,6 +41,9 @@ describe('LSTool', () => { respectGitIgnore: true, respectQwenIgnore: true, }), + storage: { + getUserSkillsDir: () => userSkillsBase, + }, } as unknown as Config; lsTool = new LSTool(mockConfig); @@ -288,7 +293,7 @@ describe('LSTool', () => { }; const invocation = lsTool.build(params); const description = invocation.getDescription(); - const expected = path.relative(tempRootDir, params.path); + const expected = path.resolve(params.path); expect(description).toBe(expected); }); }); diff --git a/packages/core/src/tools/ls.ts b/packages/core/src/tools/ls.ts index 2aefe443..6310a682 100644 --- a/packages/core/src/tools/ls.ts +++ b/packages/core/src/tools/ls.ts @@ -9,6 +9,7 @@ import path from 'node:path'; import type { ToolInvocation, ToolResult } from './tools.js'; import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; +import { isSubpath } from '../utils/paths.js'; import type { Config } from '../config/config.js'; import { DEFAULT_FILE_FILTERING_OPTIONS } from '../config/constants.js'; import { ToolErrorType } from './tool-error.js'; @@ -311,8 +312,14 @@ export class LSTool extends BaseDeclarativeTool { return `Path must be absolute: ${params.path}`; } + const userSkillsBase = this.config.storage.getUserSkillsDir(); + const isUnderUserSkills = isSubpath(userSkillsBase, params.path); + const workspaceContext = this.config.getWorkspaceContext(); - if (!workspaceContext.isPathWithinWorkspace(params.path)) { + if ( + !workspaceContext.isPathWithinWorkspace(params.path) && + !isUnderUserSkills + ) { const directories = workspaceContext.getDirectories(); return `Path must be within one of the workspace directories: ${directories.join( ', ', diff --git a/packages/core/src/tools/read-file.test.ts b/packages/core/src/tools/read-file.test.ts index aaca9923..01568eed 100644 --- a/packages/core/src/tools/read-file.test.ts +++ b/packages/core/src/tools/read-file.test.ts @@ -40,6 +40,7 @@ describe('ReadFileTool', () => { getWorkspaceContext: () => createMockWorkspaceContext(tempRootDir), storage: { getProjectTempDir: () => path.join(tempRootDir, '.temp'), + getUserSkillsDir: () => path.join(os.homedir(), '.qwen', 'skills'), }, getTruncateToolOutputThreshold: () => 2500, getTruncateToolOutputLines: () => 500, diff --git a/packages/core/src/tools/read-file.ts b/packages/core/src/tools/read-file.ts index e4b41c23..6bd0ddb6 100644 --- a/packages/core/src/tools/read-file.ts +++ b/packages/core/src/tools/read-file.ts @@ -20,6 +20,7 @@ import { FileOperation } from '../telemetry/metrics.js'; import { getProgrammingLanguage } from '../telemetry/telemetry-utils.js'; import { logFileOperation } from '../telemetry/loggers.js'; import { FileOperationEvent } from '../telemetry/types.js'; +import { isSubpath } from '../utils/paths.js'; /** * Parameters for the ReadFile tool @@ -183,15 +184,20 @@ export class ReadFileTool extends BaseDeclarativeTool< const workspaceContext = this.config.getWorkspaceContext(); const projectTempDir = this.config.storage.getProjectTempDir(); + const userSkillsDir = this.config.storage.getUserSkillsDir(); const resolvedFilePath = path.resolve(filePath); - const resolvedProjectTempDir = path.resolve(projectTempDir); - const isWithinTempDir = - resolvedFilePath.startsWith(resolvedProjectTempDir + path.sep) || - resolvedFilePath === resolvedProjectTempDir; + const isWithinTempDir = isSubpath(projectTempDir, resolvedFilePath); + const isWithinUserSkills = isSubpath(userSkillsDir, resolvedFilePath); - if (!workspaceContext.isPathWithinWorkspace(filePath) && !isWithinTempDir) { + if ( + !workspaceContext.isPathWithinWorkspace(filePath) && + !isWithinTempDir && + !isWithinUserSkills + ) { const directories = workspaceContext.getDirectories(); - return `File path must be within one of the workspace directories: ${directories.join(', ')} or within the project temp directory: ${projectTempDir}`; + return `File path must be within one of the workspace directories: ${directories.join( + ', ', + )} or within the project temp directory: ${projectTempDir}`; } if (params.offset !== undefined && params.offset < 0) { return 'Offset must be a non-negative number'; diff --git a/packages/core/src/tools/skill.test.ts b/packages/core/src/tools/skill.test.ts new file mode 100644 index 00000000..da0e9a19 --- /dev/null +++ b/packages/core/src/tools/skill.test.ts @@ -0,0 +1,442 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { SkillTool, type SkillParams } from './skill.js'; +import type { PartListUnion } from '@google/genai'; +import type { ToolResultDisplay } from './tools.js'; +import type { Config } from '../config/config.js'; +import { SkillManager } from '../skills/skill-manager.js'; +import type { SkillConfig } from '../skills/types.js'; +import { partToString } from '../utils/partUtils.js'; + +// Type for accessing protected methods in tests +type SkillToolWithProtectedMethods = SkillTool & { + createInvocation: (params: SkillParams) => { + execute: ( + signal?: AbortSignal, + updateOutput?: (output: ToolResultDisplay) => void, + ) => Promise<{ + llmContent: PartListUnion; + returnDisplay: ToolResultDisplay; + }>; + getDescription: () => string; + shouldConfirmExecute: () => Promise; + }; +}; + +// Mock dependencies +vi.mock('../skills/skill-manager.js'); +vi.mock('../telemetry/index.js', () => ({ + logSkillLaunch: vi.fn(), + SkillLaunchEvent: class { + constructor( + public skill_name: string, + public success: boolean, + ) {} + }, +})); + +const MockedSkillManager = vi.mocked(SkillManager); + +describe('SkillTool', () => { + let config: Config; + let skillTool: SkillTool; + let mockSkillManager: SkillManager; + let changeListeners: Array<() => void>; + + const mockSkills: SkillConfig[] = [ + { + name: 'code-review', + description: 'Specialized skill for reviewing code quality', + level: 'project', + filePath: '/project/.qwen/skills/code-review/SKILL.md', + body: 'Review code for quality and best practices.', + }, + { + name: 'testing', + description: 'Skill for writing and running tests', + level: 'user', + filePath: '/home/user/.qwen/skills/testing/SKILL.md', + body: 'Help write comprehensive tests.', + allowedTools: ['read_file', 'write_file', 'shell'], + }, + ]; + + beforeEach(async () => { + // Setup fake timers + vi.useFakeTimers(); + + // Create mock config + config = { + getProjectRoot: vi.fn().mockReturnValue('/test/project'), + getSessionId: vi.fn().mockReturnValue('test-session-id'), + getSkillManager: vi.fn(), + getGeminiClient: vi.fn().mockReturnValue(undefined), + } as unknown as Config; + + changeListeners = []; + + // Setup SkillManager mock + mockSkillManager = { + listSkills: vi.fn().mockResolvedValue(mockSkills), + loadSkill: vi.fn(), + loadSkillForRuntime: vi.fn(), + addChangeListener: vi.fn((listener: () => void) => { + changeListeners.push(listener); + return () => { + const index = changeListeners.indexOf(listener); + if (index >= 0) { + changeListeners.splice(index, 1); + } + }; + }), + getParseErrors: vi.fn().mockReturnValue(new Map()), + } as unknown as SkillManager; + + MockedSkillManager.mockImplementation(() => mockSkillManager); + + // Make config return the mock SkillManager + vi.mocked(config.getSkillManager).mockReturnValue(mockSkillManager); + + // Create SkillTool instance + skillTool = new SkillTool(config); + + // Allow async initialization to complete + await vi.runAllTimersAsync(); + }); + + afterEach(() => { + vi.useRealTimers(); + vi.clearAllMocks(); + }); + + describe('initialization', () => { + it('should initialize with correct name and properties', () => { + expect(skillTool.name).toBe('skill'); + expect(skillTool.displayName).toBe('Skill'); + expect(skillTool.kind).toBe('read'); + }); + + it('should load available skills during initialization', () => { + expect(mockSkillManager.listSkills).toHaveBeenCalled(); + }); + + it('should subscribe to skill manager changes', () => { + expect(mockSkillManager.addChangeListener).toHaveBeenCalledTimes(1); + }); + + it('should update description with available skills', () => { + expect(skillTool.description).toContain('code-review'); + expect(skillTool.description).toContain( + 'Specialized skill for reviewing code quality', + ); + expect(skillTool.description).toContain('testing'); + expect(skillTool.description).toContain( + 'Skill for writing and running tests', + ); + }); + + it('should handle empty skills list gracefully', async () => { + vi.mocked(mockSkillManager.listSkills).mockResolvedValue([]); + + const emptySkillTool = new SkillTool(config); + await vi.runAllTimersAsync(); + + expect(emptySkillTool.description).toContain( + 'No skills are currently configured', + ); + }); + + it('should handle skill loading errors gracefully', async () => { + vi.mocked(mockSkillManager.listSkills).mockRejectedValue( + new Error('Loading failed'), + ); + + const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + new SkillTool(config); + await vi.runAllTimersAsync(); + + expect(consoleSpy).toHaveBeenCalledWith( + 'Failed to load skills for Skills tool:', + expect.any(Error), + ); + consoleSpy.mockRestore(); + }); + }); + + describe('schema generation', () => { + it('should expose static schema without dynamic enums', () => { + const schema = skillTool.schema; + const properties = schema.parametersJsonSchema as { + properties: { + skill: { + type: string; + description: string; + enum?: string[]; + }; + }; + }; + expect(properties.properties.skill.type).toBe('string'); + expect(properties.properties.skill.description).toBe( + 'The skill name (no arguments). E.g., "pdf" or "xlsx"', + ); + expect(properties.properties.skill.enum).toBeUndefined(); + }); + + it('should keep schema static even when no skills available', async () => { + vi.mocked(mockSkillManager.listSkills).mockResolvedValue([]); + + const emptySkillTool = new SkillTool(config); + await vi.runAllTimersAsync(); + + const schema = emptySkillTool.schema; + const properties = schema.parametersJsonSchema as { + properties: { + skill: { + type: string; + description: string; + enum?: string[]; + }; + }; + }; + expect(properties.properties.skill.type).toBe('string'); + expect(properties.properties.skill.description).toBe( + 'The skill name (no arguments). E.g., "pdf" or "xlsx"', + ); + expect(properties.properties.skill.enum).toBeUndefined(); + }); + }); + + describe('validateToolParams', () => { + it('should validate valid parameters', () => { + const result = skillTool.validateToolParams({ skill: 'code-review' }); + expect(result).toBeNull(); + }); + + it('should reject empty skill', () => { + const result = skillTool.validateToolParams({ skill: '' }); + expect(result).toBe('Parameter "skill" must be a non-empty string.'); + }); + + it('should reject non-existent skill', () => { + const result = skillTool.validateToolParams({ + skill: 'non-existent', + }); + expect(result).toBe( + 'Skill "non-existent" not found. Available skills: code-review, testing', + ); + }); + + it('should show appropriate message when no skills available', async () => { + vi.mocked(mockSkillManager.listSkills).mockResolvedValue([]); + + const emptySkillTool = new SkillTool(config); + await vi.runAllTimersAsync(); + + const result = emptySkillTool.validateToolParams({ + skill: 'non-existent', + }); + expect(result).toBe( + 'Skill "non-existent" not found. No skills are currently available.', + ); + }); + }); + + describe('refreshSkills', () => { + it('should refresh when change listener fires', async () => { + const newSkills: SkillConfig[] = [ + { + name: 'new-skill', + description: 'A brand new skill', + level: 'project', + filePath: '/project/.qwen/skills/new-skill/SKILL.md', + body: 'New skill content.', + }, + ]; + + vi.mocked(mockSkillManager.listSkills).mockResolvedValueOnce(newSkills); + + const listener = changeListeners[0]; + expect(listener).toBeDefined(); + + listener?.(); + await vi.runAllTimersAsync(); + + expect(skillTool.description).toContain('new-skill'); + expect(skillTool.description).toContain('A brand new skill'); + }); + + it('should refresh available skills and update description', async () => { + const newSkills: SkillConfig[] = [ + { + name: 'test-skill', + description: 'A test skill', + level: 'project', + filePath: '/project/.qwen/skills/test-skill/SKILL.md', + body: 'Test content.', + }, + ]; + + vi.mocked(mockSkillManager.listSkills).mockResolvedValue(newSkills); + + await skillTool.refreshSkills(); + + expect(skillTool.description).toContain('test-skill'); + expect(skillTool.description).toContain('A test skill'); + }); + }); + + describe('SkillToolInvocation', () => { + const mockRuntimeConfig: SkillConfig = { + ...mockSkills[0], + }; + + beforeEach(() => { + vi.mocked(mockSkillManager.loadSkillForRuntime).mockResolvedValue( + mockRuntimeConfig, + ); + }); + + it('should execute skill load successfully', async () => { + const params: SkillParams = { + skill: 'code-review', + }; + + const invocation = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation(params); + const result = await invocation.execute(); + + expect(mockSkillManager.loadSkillForRuntime).toHaveBeenCalledWith( + 'code-review', + ); + + const llmText = partToString(result.llmContent); + expect(llmText).toContain( + 'Base directory for this skill: /project/.qwen/skills/code-review', + ); + expect(llmText.trim()).toContain( + 'Review code for quality and best practices.', + ); + + expect(result.returnDisplay).toBe('Launching skill: code-review'); + }); + + it('should include allowedTools in result when present', async () => { + const skillWithTools: SkillConfig = { + ...mockSkills[1], + }; + vi.mocked(mockSkillManager.loadSkillForRuntime).mockResolvedValue( + skillWithTools, + ); + + const params: SkillParams = { + skill: 'testing', + }; + + const invocation = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation(params); + const result = await invocation.execute(); + + const llmText = partToString(result.llmContent); + expect(llmText).toContain('testing'); + // Base description is omitted from llmContent; ensure body is present. + expect(llmText).toContain('Help write comprehensive tests.'); + + expect(result.returnDisplay).toBe('Launching skill: testing'); + }); + + it('should handle skill not found error', async () => { + vi.mocked(mockSkillManager.loadSkillForRuntime).mockResolvedValue(null); + + const params: SkillParams = { + skill: 'non-existent', + }; + + const invocation = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation(params); + const result = await invocation.execute(); + + const llmText = partToString(result.llmContent); + expect(llmText).toContain('Skill "non-existent" not found'); + }); + + it('should handle execution errors gracefully', async () => { + vi.mocked(mockSkillManager.loadSkillForRuntime).mockRejectedValue( + new Error('Loading failed'), + ); + + const consoleSpy = vi + .spyOn(console, 'error') + .mockImplementation(() => {}); + + const params: SkillParams = { + skill: 'code-review', + }; + + const invocation = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation(params); + const result = await invocation.execute(); + + const llmText = partToString(result.llmContent); + expect(llmText).toContain('Failed to load skill'); + expect(llmText).toContain('Loading failed'); + + consoleSpy.mockRestore(); + }); + + it('should not require confirmation', async () => { + const params: SkillParams = { + skill: 'code-review', + }; + + const invocation = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation(params); + const shouldConfirm = await invocation.shouldConfirmExecute(); + + expect(shouldConfirm).toBe(false); + }); + + it('should provide correct description', () => { + const params: SkillParams = { + skill: 'code-review', + }; + + const invocation = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation(params); + const description = invocation.getDescription(); + + expect(description).toBe('Launching skill: "code-review"'); + }); + + it('should handle skill without additional files', async () => { + vi.mocked(mockSkillManager.loadSkillForRuntime).mockResolvedValue( + mockSkills[0], + ); + + const params: SkillParams = { + skill: 'code-review', + }; + + const invocation = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation(params); + const result = await invocation.execute(); + + const llmText = partToString(result.llmContent); + expect(llmText).not.toContain('## Additional Files'); + + expect(result.returnDisplay).toBe('Launching skill: code-review'); + }); + }); +}); diff --git a/packages/core/src/tools/skill.ts b/packages/core/src/tools/skill.ts new file mode 100644 index 00000000..d0a1fce6 --- /dev/null +++ b/packages/core/src/tools/skill.ts @@ -0,0 +1,264 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; +import { ToolNames, ToolDisplayNames } from './tool-names.js'; +import type { ToolResult, ToolResultDisplay } from './tools.js'; +import type { Config } from '../config/config.js'; +import type { SkillManager } from '../skills/skill-manager.js'; +import type { SkillConfig } from '../skills/types.js'; +import { logSkillLaunch, SkillLaunchEvent } from '../telemetry/index.js'; +import path from 'path'; + +export interface SkillParams { + skill: string; +} + +/** + * Skill tool that enables the model to access skill definitions. + * The tool dynamically loads available skills and includes them in its description + * for the model to choose from. + */ +export class SkillTool extends BaseDeclarativeTool { + static readonly Name: string = ToolNames.SKILL; + + private skillManager: SkillManager; + private availableSkills: SkillConfig[] = []; + + constructor(private readonly config: Config) { + // Initialize with a basic schema first + const initialSchema = { + type: 'object', + properties: { + skill: { + type: 'string', + description: 'The skill name (no arguments). E.g., "pdf" or "xlsx"', + }, + }, + required: ['skill'], + additionalProperties: false, + $schema: 'http://json-schema.org/draft-07/schema#', + }; + + super( + SkillTool.Name, + ToolDisplayNames.SKILL, + 'Execute a skill within the main conversation. Loading available skills...', // Initial description + Kind.Read, + initialSchema, + true, // isOutputMarkdown + false, // canUpdateOutput + ); + + this.skillManager = config.getSkillManager(); + this.skillManager.addChangeListener(() => { + void this.refreshSkills(); + }); + + // Initialize the tool asynchronously + this.refreshSkills(); + } + + /** + * Asynchronously initializes the tool by loading available skills + * and updating the description and schema. + */ + async refreshSkills(): Promise { + try { + this.availableSkills = await this.skillManager.listSkills(); + this.updateDescriptionAndSchema(); + } catch (error) { + console.warn('Failed to load skills for Skills tool:', error); + this.availableSkills = []; + this.updateDescriptionAndSchema(); + } finally { + // Update the client with the new tools + const geminiClient = this.config.getGeminiClient(); + if (geminiClient && geminiClient.isInitialized()) { + await geminiClient.setTools(); + } + } + } + + /** + * Updates the tool's description and schema based on available skills. + */ + private updateDescriptionAndSchema(): void { + let skillDescriptions = ''; + if (this.availableSkills.length === 0) { + skillDescriptions = + 'No skills are currently configured. Skills can be created by adding directories with SKILL.md files to .qwen/skills/ or ~/.qwen/skills/.'; + } else { + skillDescriptions = this.availableSkills + .map( + (skill) => ` + +${skill.name} + + +${skill.description} (${skill.level}) + + +${skill.level} + +`, + ) + .join('\n'); + } + + const baseDescription = `Execute a skill within the main conversation + + +When users ask you to perform tasks, check if any of the available skills below can help complete the task more effectively. Skills provide specialized capabilities and domain knowledge. + +How to invoke: +- Use this tool with the skill name only (no arguments) +- Examples: + - \`skill: "pdf"\` - invoke the pdf skill + - \`skill: "xlsx"\` - invoke the xlsx skill + - \`skill: "ms-office-suite:pdf"\` - invoke using fully qualified name + +Important: +- When a skill is relevant, you must invoke this tool IMMEDIATELY as your first action +- NEVER just announce or mention a skill in your text response without actually calling this tool +- This is a BLOCKING REQUIREMENT: invoke the relevant Skill tool BEFORE generating any other response about the task +- Only use skills listed in below +- Do not invoke a skill that is already running +- Do not use this tool for built-in CLI commands (like /help, /clear, etc.) + + + +${skillDescriptions} + +`; + // Update description using object property assignment + (this as { description: string }).description = baseDescription; + } + + override validateToolParams(params: SkillParams): string | null { + // Validate required fields + if ( + !params.skill || + typeof params.skill !== 'string' || + params.skill.trim() === '' + ) { + return 'Parameter "skill" must be a non-empty string.'; + } + + // Validate that the skill exists + const skillExists = this.availableSkills.some( + (skill) => skill.name === params.skill, + ); + + if (!skillExists) { + const availableNames = this.availableSkills.map((s) => s.name); + if (availableNames.length === 0) { + return `Skill "${params.skill}" not found. No skills are currently available.`; + } + return `Skill "${params.skill}" not found. Available skills: ${availableNames.join(', ')}`; + } + + return null; + } + + protected createInvocation(params: SkillParams) { + return new SkillToolInvocation(this.config, this.skillManager, params); + } + + getAvailableSkillNames(): string[] { + return this.availableSkills.map((skill) => skill.name); + } +} + +class SkillToolInvocation extends BaseToolInvocation { + constructor( + private readonly config: Config, + private readonly skillManager: SkillManager, + params: SkillParams, + ) { + super(params); + } + + getDescription(): string { + return `Launching skill: "${this.params.skill}"`; + } + + override async shouldConfirmExecute(): Promise { + // Skill loading is a read-only operation, no confirmation needed + return false; + } + + async execute( + _signal?: AbortSignal, + _updateOutput?: (output: ToolResultDisplay) => void, + ): Promise { + try { + // Load the skill with runtime config (includes additional files) + const skill = await this.skillManager.loadSkillForRuntime( + this.params.skill, + ); + + if (!skill) { + // Log failed skill launch + logSkillLaunch( + this.config, + new SkillLaunchEvent(this.params.skill, false), + ); + + // Get parse errors if any + const parseErrors = this.skillManager.getParseErrors(); + const errorMessages: string[] = []; + + for (const [filePath, error] of parseErrors) { + if (filePath.includes(this.params.skill)) { + errorMessages.push(`Parse error at ${filePath}: ${error.message}`); + } + } + + const errorDetail = + errorMessages.length > 0 + ? `\nErrors:\n${errorMessages.join('\n')}` + : ''; + + return { + llmContent: `Skill "${this.params.skill}" not found.${errorDetail}`, + returnDisplay: `Skill "${this.params.skill}" not found.${errorDetail}`, + }; + } + + // Log successful skill launch + logSkillLaunch( + this.config, + new SkillLaunchEvent(this.params.skill, true), + ); + + const baseDir = path.dirname(skill.filePath); + + // Build markdown content for LLM (show base dir, then body) + const llmContent = `Base directory for this skill: ${baseDir}\n\n${skill.body}\n`; + + return { + llmContent: [{ text: llmContent }], + returnDisplay: `Launching skill: ${skill.name}`, + }; + } catch (error) { + const errorMessage = + error instanceof Error ? error.message : String(error); + console.error(`[SkillsTool] Error launching skill: ${errorMessage}`); + + // Log failed skill launch + logSkillLaunch( + this.config, + new SkillLaunchEvent(this.params.skill, false), + ); + + return { + llmContent: `Failed to load skill "${this.params.skill}": ${errorMessage}`, + returnDisplay: `Failed to load skill "${this.params.skill}": ${errorMessage}`, + }; + } + } +} diff --git a/packages/core/src/tools/task.ts b/packages/core/src/tools/task.ts index 67f03f5f..e8fd64d5 100644 --- a/packages/core/src/tools/task.ts +++ b/packages/core/src/tools/task.ts @@ -252,6 +252,10 @@ assistant: "I'm going to use the Task tool to launch the with the greeting-respo protected createInvocation(params: TaskParams) { return new TaskToolInvocation(this.config, this.subagentManager, params); } + + getAvailableSubagentNames(): string[] { + return this.availableSubagents.map((subagent) => subagent.name); + } } class TaskToolInvocation extends BaseToolInvocation { diff --git a/packages/core/src/tools/tool-names.ts b/packages/core/src/tools/tool-names.ts index 22be9c1b..8cd1de54 100644 --- a/packages/core/src/tools/tool-names.ts +++ b/packages/core/src/tools/tool-names.ts @@ -20,6 +20,7 @@ export const ToolNames = { TODO_WRITE: 'todo_write', MEMORY: 'save_memory', TASK: 'task', + SKILL: 'skill', EXIT_PLAN_MODE: 'exit_plan_mode', WEB_FETCH: 'web_fetch', WEB_SEARCH: 'web_search', @@ -42,6 +43,7 @@ export const ToolDisplayNames = { TODO_WRITE: 'TodoWrite', MEMORY: 'SaveMemory', TASK: 'Task', + SKILL: 'Skill', EXIT_PLAN_MODE: 'ExitPlanMode', WEB_FETCH: 'WebFetch', WEB_SEARCH: 'WebSearch', diff --git a/packages/core/src/utils/paths.ts b/packages/core/src/utils/paths.ts index 0bdfaf83..a4c22010 100644 --- a/packages/core/src/utils/paths.ts +++ b/packages/core/src/utils/paths.ts @@ -120,6 +120,10 @@ export function makeRelative( const resolvedTargetPath = path.resolve(targetPath); const resolvedRootDirectory = path.resolve(rootDirectory); + if (!isSubpath(resolvedRootDirectory, resolvedTargetPath)) { + return resolvedTargetPath; + } + const relativePath = path.relative(resolvedRootDirectory, resolvedTargetPath); // If the paths are the same, path.relative returns '', return '.' instead