diff --git a/packages/core/src/config/storage.ts b/packages/core/src/config/storage.ts index 5de57ab0c..f0b80d88c 100644 --- a/packages/core/src/config/storage.ts +++ b/packages/core/src/config/storage.ts @@ -12,6 +12,7 @@ import { getProjectHash, sanitizeCwd } from '../utils/paths.js'; export const QWEN_DIR = '.qwen'; export const GOOGLE_ACCOUNTS_FILENAME = 'google_accounts.json'; export const OAUTH_FILE = 'oauth_creds.json'; +export const SKILL_PROVIDER_CONFIG_DIRS = ['.qwen', '.agent']; const TMP_DIR_NAME = 'tmp'; const BIN_DIR_NAME = 'bin'; const PROJECT_DIR_NAME = 'projects'; @@ -138,8 +139,11 @@ export class Storage { return path.join(this.getExtensionsDir(), 'qwen-extension.json'); } - getUserSkillsDir(): string { - return path.join(Storage.getGlobalQwenDir(), 'skills'); + getUserSkillsDirs(): string[] { + const homeDir = os.homedir() || os.tmpdir(); + return SKILL_PROVIDER_CONFIG_DIRS.map((dir) => + path.join(homeDir, dir, 'skills'), + ); } getHistoryFilePath(): string { diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index 73fe770ac..e943275bd 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -57,14 +57,10 @@ function getWindowsPathFingerprint( env: NodeJS.ProcessEnv, pathKeys: string[], ): string { - return pathKeys - .map((key) => `${key}=${env[key] ?? ''}`) - .join('\0'); + return pathKeys.map((key) => `${key}=${env[key] ?? ''}`).join('\0'); } -function normalizePathEnvForWindows( - env: NodeJS.ProcessEnv, -): NodeJS.ProcessEnv { +function normalizePathEnvForWindows(env: NodeJS.ProcessEnv): NodeJS.ProcessEnv { if (os.platform() !== 'win32') { return env; } diff --git a/packages/core/src/skills/skill-manager.test.ts b/packages/core/src/skills/skill-manager.test.ts index 272d3001d..639234577 100644 --- a/packages/core/src/skills/skill-manager.test.ts +++ b/packages/core/src/skills/skill-manager.test.ts @@ -73,6 +73,14 @@ describe('SkillManager', () => { if (yamlString.includes('name: regular-skill')) { return { name: 'regular-skill', description: 'A regular skill' }; } + if (yamlString.includes('name: shared-skill')) { + const desc = yamlString.includes('From qwen dir') + ? 'From qwen dir' + : yamlString.includes('From agent dir') + ? 'From agent dir' + : 'A shared skill'; + return { name: 'shared-skill', description: desc }; + } if (!yamlString.includes('name:')) { return { description: 'A test skill' }; // Missing name case } @@ -391,42 +399,61 @@ You are a helpful assistant. describe('listSkills', () => { beforeEach(() => { - // Mock directory listing for skills directories (with Dirent objects) - vi.mocked(fs.readdir) - .mockResolvedValueOnce([ - { - name: 'skill1', - isDirectory: () => true, - isFile: () => false, - isSymbolicLink: () => false, - }, - { - name: 'skill2', - isDirectory: () => true, - isFile: () => false, - isSymbolicLink: () => false, - }, - { - name: 'not-a-dir.txt', - isDirectory: () => false, - isFile: () => true, - isSymbolicLink: () => false, - }, - ] as unknown as Awaited>) - .mockResolvedValueOnce([ - { - name: 'skill3', - isDirectory: () => true, - isFile: () => false, - isSymbolicLink: () => false, - }, - { - name: 'skill1', - isDirectory: () => true, - isFile: () => false, - isSymbolicLink: () => false, - }, - ] as unknown as Awaited>); + // Mock directory listing based on path to handle multiple base dirs per level. + // Use path.join to construct expected paths so separators match on all platforms. + const projectQwenSkillsDir = path.join( + '/test/project', + '.qwen', + 'skills', + ); + const userQwenSkillsDir = path.join('/home/user', '.qwen', 'skills'); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + vi.mocked(fs.readdir).mockImplementation((dirPath: any) => { + const pathStr = String(dirPath); + if (pathStr === projectQwenSkillsDir) { + return Promise.resolve([ + { + name: 'skill1', + isDirectory: () => true, + isFile: () => false, + isSymbolicLink: () => false, + }, + { + name: 'skill2', + isDirectory: () => true, + isFile: () => false, + isSymbolicLink: () => false, + }, + { + name: 'not-a-dir.txt', + isDirectory: () => false, + isFile: () => true, + isSymbolicLink: () => false, + }, + ] as unknown as Awaited>); + } + if (pathStr === userQwenSkillsDir) { + return Promise.resolve([ + { + name: 'skill3', + isDirectory: () => true, + isFile: () => false, + isSymbolicLink: () => false, + }, + { + name: 'skill1', + isDirectory: () => true, + isFile: () => false, + isSymbolicLink: () => false, + }, + ] as unknown as Awaited>); + } + // Other provider dirs (.agent, .cursor, .codex, .claude) return empty + return Promise.resolve( + [] as unknown as Awaited>, + ); + }); vi.mocked(fs.access).mockResolvedValue(undefined); @@ -483,6 +510,66 @@ Skill 3 content`); expect(projectSkills.every((s) => s.level === 'project')).toBe(true); }); + it('should deduplicate same-name skills across provider dirs within a level', async () => { + // Override readdir to return the same skill name from both .qwen and .agent dirs + vi.mocked(fs.readdir).mockReset(); + const projectQwenDir = path.join('/test/project', '.qwen', 'skills'); + const projectAgentDir = path.join('/test/project', '.agent', 'skills'); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + vi.mocked(fs.readdir).mockImplementation((dirPath: any) => { + const pathStr = String(dirPath); + if (pathStr === projectQwenDir) { + return Promise.resolve([ + { + name: 'shared-skill', + isDirectory: () => true, + isFile: () => false, + isSymbolicLink: () => false, + }, + ] as unknown as Awaited>); + } + if (pathStr === projectAgentDir) { + return Promise.resolve([ + { + name: 'shared-skill', + isDirectory: () => true, + isFile: () => false, + isSymbolicLink: () => false, + }, + ] as unknown as Awaited>); + } + return Promise.resolve( + [] as unknown as Awaited>, + ); + }); + + vi.mocked(fs.readFile).mockImplementation((filePath) => { + const pathStr = String(filePath); + if (pathStr.includes('.qwen') && pathStr.includes('shared-skill')) { + return Promise.resolve( + `---\nname: shared-skill\ndescription: From qwen dir\n---\nQwen content`, + ); + } + if (pathStr.includes('.agent') && pathStr.includes('shared-skill')) { + return Promise.resolve( + `---\nname: shared-skill\ndescription: From agent dir\n---\nAgent content`, + ); + } + return Promise.reject(new Error('File not found')); + }); + + const skills = await manager.listSkills({ + level: 'project', + force: true, + }); + + // Only one instance should remain, from .qwen (first in PROVIDER_CONFIG_DIRS) + expect(skills).toHaveLength(1); + expect(skills[0].name).toBe('shared-skill'); + expect(skills[0].description).toBe('From qwen dir'); + }); + it('should handle empty directories', async () => { vi.mocked(fs.readdir).mockReset(); vi.mocked(fs.readdir).mockResolvedValue( @@ -504,27 +591,33 @@ Skill 3 content`); }); }); - describe('getSkillsBaseDir', () => { - it('should return project-level base dir', () => { - const baseDir = manager.getSkillsBaseDir('project'); + describe('getSkillsBaseDirs', () => { + it('should return all project-level base dirs', () => { + const baseDirs = manager.getSkillsBaseDirs('project'); - expect(baseDir).toBe(path.join('/test/project', '.qwen', 'skills')); + expect(baseDirs).toHaveLength(2); + expect(baseDirs).toContain(path.join('/test/project', '.qwen', 'skills')); + expect(baseDirs).toContain( + path.join('/test/project', '.agent', 'skills'), + ); }); - it('should return user-level base dir', () => { - const baseDir = manager.getSkillsBaseDir('user'); + it('should return all user-level base dirs', () => { + const baseDirs = manager.getSkillsBaseDirs('user'); - expect(baseDir).toBe(path.join('/home/user', '.qwen', 'skills')); + expect(baseDirs).toHaveLength(2); + expect(baseDirs).toContain(path.join('/home/user', '.qwen', 'skills')); + expect(baseDirs).toContain(path.join('/home/user', '.agent', 'skills')); }); it('should return bundled-level base dir', () => { - const baseDir = manager.getSkillsBaseDir('bundled'); + const baseDirs = manager.getSkillsBaseDirs('bundled'); - expect(baseDir).toMatch(/skills[/\\]bundled$/); + expect(baseDirs[0]).toMatch(/skills[/\\]bundled$/); }); it('should throw for extension level', () => { - expect(() => manager.getSkillsBaseDir('extension')).toThrow( + expect(() => manager.getSkillsBaseDirs('extension')).toThrow( 'Extension skills do not have a base directory', ); }); diff --git a/packages/core/src/skills/skill-manager.ts b/packages/core/src/skills/skill-manager.ts index b6636a627..fbeb18b8d 100644 --- a/packages/core/src/skills/skill-manager.ts +++ b/packages/core/src/skills/skill-manager.ts @@ -22,6 +22,7 @@ import type { Config } from '../config/config.js'; import { validateConfig } from './skill-load.js'; import { createDebugLogger } from '../utils/debugLogger.js'; import { normalizeContent } from '../utils/textUtils.js'; +import { SKILL_PROVIDER_CONFIG_DIRS } from '../config/storage.js'; const debugLogger = createDebugLogger('SKILL_MANAGER'); @@ -428,20 +429,20 @@ export class SkillManager { * Gets the base directory for skills at a specific level. * * @param level - Storage level - * @returns Absolute directory path + * @returns Absolute directory paths */ - getSkillsBaseDir(level: SkillLevel): string { + getSkillsBaseDirs(level: SkillLevel): string[] { switch (level) { case 'project': - return path.join( - this.config.getProjectRoot(), - QWEN_CONFIG_DIR, - SKILLS_CONFIG_DIR, + return SKILL_PROVIDER_CONFIG_DIRS.map((v) => + path.join(this.config.getProjectRoot(), v, SKILLS_CONFIG_DIR), ); case 'user': - return path.join(os.homedir(), QWEN_CONFIG_DIR, SKILLS_CONFIG_DIR); + return SKILL_PROVIDER_CONFIG_DIRS.map((v) => + path.join(os.homedir(), v, SKILLS_CONFIG_DIR), + ); case 'bundled': - return this.bundledSkillsDir; + return [this.bundledSkillsDir]; case 'extension': throw new Error( 'Extension skills do not have a base directory; they are loaded from active extensions.', @@ -499,9 +500,26 @@ export class SkillManager { return skills; } - const baseDir = this.getSkillsBaseDir(level); - debugLogger.debug(`Loading ${level} level skills from: ${baseDir}`); - const skills = await this.loadSkillsFromDir(baseDir, level); + // Iterate provider directories in PROVIDER_CONFIG_DIRS order. + // The first directory that contains a skill with a given name wins, + // so the order defines implicit precedence (.qwen > .agent > .cursor > ...). + const baseDirs = this.getSkillsBaseDirs(level); + const skills: SkillConfig[] = []; + const seenNames = new Set(); + for (const baseDir of baseDirs) { + debugLogger.debug(`Loading ${level} level skills from: ${baseDir}`); + const skillsFromDir = await this.loadSkillsFromDir(baseDir, level); + for (const skill of skillsFromDir) { + if (seenNames.has(skill.name)) { + debugLogger.debug( + `Skipping duplicate skill at ${level} level: ${skill.name} from ${baseDir}`, + ); + continue; + } + seenNames.add(skill.name); + skills.push(skill); + } + } debugLogger.debug(`Loaded ${skills.length} ${level} level skills`); return skills; } @@ -624,7 +642,8 @@ export class SkillManager { private updateWatchersFromCache(): void { const watchTargets = new Set( (['project', 'user'] as const) - .map((level) => this.getSkillsBaseDir(level)) + .map((level) => this.getSkillsBaseDirs(level)) + .reduce((acc, baseDirs) => acc.concat(baseDirs), []) .filter((baseDir) => fsSync.existsSync(baseDir)), ); @@ -680,7 +699,7 @@ export class SkillManager { } private async ensureUserSkillsDir(): Promise { - const baseDir = this.getSkillsBaseDir('user'); + const baseDir = path.join(os.homedir(), QWEN_CONFIG_DIR, SKILLS_CONFIG_DIR); try { await fs.mkdir(baseDir, { recursive: true }); } catch (error) { diff --git a/packages/core/src/tools/ls.test.ts b/packages/core/src/tools/ls.test.ts index da6273eb1..204289e61 100644 --- a/packages/core/src/tools/ls.test.ts +++ b/packages/core/src/tools/ls.test.ts @@ -43,7 +43,7 @@ describe('LSTool', () => { }), getTruncateToolOutputLines: () => 1000, storage: { - getUserSkillsDir: () => userSkillsBase, + getUserSkillsDirs: () => [userSkillsBase], }, } as unknown as Config; diff --git a/packages/core/src/tools/ls.ts b/packages/core/src/tools/ls.ts index 877a1274b..a4283417b 100644 --- a/packages/core/src/tools/ls.ts +++ b/packages/core/src/tools/ls.ts @@ -9,7 +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 { isSubpaths } 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'; @@ -335,8 +335,8 @@ 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 userSkillsBases = this.config.storage.getUserSkillsDirs(); + const isUnderUserSkills = isSubpaths(userSkillsBases, params.path); const workspaceContext = this.config.getWorkspaceContext(); if ( diff --git a/packages/core/src/tools/read-file.test.ts b/packages/core/src/tools/read-file.test.ts index 1878c3805..ebc9cb27b 100644 --- a/packages/core/src/tools/read-file.test.ts +++ b/packages/core/src/tools/read-file.test.ts @@ -40,7 +40,7 @@ describe('ReadFileTool', () => { getWorkspaceContext: () => createMockWorkspaceContext(tempRootDir), storage: { getProjectTempDir: () => path.join(tempRootDir, '.temp'), - getUserSkillsDir: () => path.join(os.homedir(), '.qwen', 'skills'), + getUserSkillsDirs: () => [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 2e0a1c228..4a9c3bd9a 100644 --- a/packages/core/src/tools/read-file.ts +++ b/packages/core/src/tools/read-file.ts @@ -21,7 +21,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'; +import { isSubpaths, isSubpath } from '../utils/paths.js'; import { Storage } from '../config/storage.js'; /** @@ -187,7 +187,7 @@ export class ReadFileTool extends BaseDeclarativeTool< const workspaceContext = this.config.getWorkspaceContext(); const globalTempDir = Storage.getGlobalTempDir(); const projectTempDir = this.config.storage.getProjectTempDir(); - const userSkillsDir = this.config.storage.getUserSkillsDir(); + const userSkillsDirs = this.config.storage.getUserSkillsDirs(); const arenaDir = Storage.getGlobalArenaDir(); const resolvedFilePath = path.resolve(filePath); const osTempDir = os.tmpdir(); @@ -195,8 +195,9 @@ export class ReadFileTool extends BaseDeclarativeTool< isSubpath(projectTempDir, resolvedFilePath) || isSubpath(globalTempDir, resolvedFilePath) || isSubpath(osTempDir, resolvedFilePath); + + const isWithinUserSkills = isSubpaths(userSkillsDirs, resolvedFilePath); const isWithinArenaDir = isSubpath(arenaDir, resolvedFilePath); - const isWithinUserSkills = isSubpath(userSkillsDir, resolvedFilePath); if ( !workspaceContext.isPathWithinWorkspace(filePath) && diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index e9aa4f850..95222af5d 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -57,7 +57,7 @@ describe('ShellTool', () => { .fn() .mockReturnValue(createMockWorkspaceContext('/test/dir')), storage: { - getUserSkillsDir: vi.fn().mockReturnValue('/test/dir/.qwen/skills'), + getUserSkillsDirs: vi.fn().mockReturnValue(['/test/dir/.qwen/skills']), getProjectTempDir: vi.fn().mockReturnValue('/tmp/qwen-temp'), }, getTruncateToolOutputThreshold: vi.fn().mockReturnValue(0), diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 54ecf30f8..67cd4cc38 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -34,7 +34,7 @@ import type { import { ShellExecutionService } from '../services/shellExecutionService.js'; import { formatMemoryUsage } from '../utils/formatters.js'; import type { AnsiOutput } from '../utils/terminalSerializer.js'; -import { isSubpath } from '../utils/paths.js'; +import { isSubpaths } from '../utils/paths.js'; import { getCommandRoots, isCommandAllowed, @@ -622,10 +622,10 @@ export class ShellTool extends BaseDeclarativeTool< return 'Directory must be an absolute path.'; } - const userSkillsDir = this.config.storage.getUserSkillsDir(); + const userSkillsDirs = this.config.storage.getUserSkillsDirs(); const resolvedDirectoryPath = path.resolve(params.directory); - const isWithinUserSkills = isSubpath( - userSkillsDir, + const isWithinUserSkills = isSubpaths( + userSkillsDirs, resolvedDirectoryPath, ); if (isWithinUserSkills) { diff --git a/packages/core/src/utils/paths.ts b/packages/core/src/utils/paths.ts index dc4434ece..6e6bdfa49 100644 --- a/packages/core/src/utils/paths.ts +++ b/packages/core/src/utils/paths.ts @@ -241,6 +241,10 @@ export function isSubpath(parentPath: string, childPath: string): boolean { ); } +export function isSubpaths(parentPath: string[], childPath: string): boolean { + return parentPath.some((p) => isSubpath(p, childPath)); +} + /** * Resolves a path with tilde (~) expansion and relative path resolution. * Handles tilde expansion for home directory and resolves relative paths