diff --git a/packages/cli/src/config/extension-manager-skills.test.ts b/packages/cli/src/config/extension-manager-skills.test.ts new file mode 100644 index 0000000000..495336e4a8 --- /dev/null +++ b/packages/cli/src/config/extension-manager-skills.test.ts @@ -0,0 +1,138 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import * as os from 'node:os'; +import { ExtensionManager } from './extension-manager.js'; +import { loadSettings } from './settings.js'; +import { createExtension } from '../test-utils/createExtension.js'; +import { EXTENSIONS_DIRECTORY_NAME } from './extensions/variables.js'; +import { coreEvents } from '@google/gemini-cli-core'; + +const mockHomedir = vi.hoisted(() => vi.fn()); + +vi.mock('os', async (importOriginal) => { + const mockedOs = await importOriginal(); + return { + ...mockedOs, + homedir: mockHomedir, + }; +}); + +describe('ExtensionManager skills validation', () => { + let tempHomeDir: string; + let tempWorkspaceDir: string; + let userExtensionsDir: string; + let extensionManager: ExtensionManager; + + beforeEach(() => { + tempHomeDir = fs.mkdtempSync( + path.join(os.tmpdir(), 'gemini-cli-skills-test-home-'), + ); + tempWorkspaceDir = fs.mkdtempSync( + path.join(tempHomeDir, 'gemini-cli-skills-test-workspace-'), + ); + userExtensionsDir = path.join(tempHomeDir, EXTENSIONS_DIRECTORY_NAME); + fs.mkdirSync(userExtensionsDir, { recursive: true }); + + mockHomedir.mockReturnValue(tempHomeDir); + + extensionManager = new ExtensionManager({ + workspaceDir: tempWorkspaceDir, + requestConsent: vi.fn().mockResolvedValue(true), + requestSetting: vi.fn().mockResolvedValue(''), + settings: loadSettings(tempWorkspaceDir).merged, + }); + vi.spyOn(coreEvents, 'emitFeedback'); + }); + + afterEach(() => { + fs.rmSync(tempHomeDir, { recursive: true, force: true }); + vi.restoreAllMocks(); + }); + + it('should emit a warning during install if skills directory is not empty but no skills are loaded', async () => { + const sourceExtDir = createExtension({ + extensionsDir: tempHomeDir, + name: 'skills-ext', + version: '1.0.0', + }); + + const skillsDir = path.join(sourceExtDir, 'skills'); + fs.mkdirSync(skillsDir); + fs.writeFileSync(path.join(skillsDir, 'not-a-skill.txt'), 'hello'); + + await extensionManager.loadExtensions(); + const extension = await extensionManager.installOrUpdateExtension({ + source: sourceExtDir, + type: 'local', + }); + + expect(extension.name).toBe('skills-ext'); + expect(coreEvents.emitFeedback).toHaveBeenCalledWith( + 'warning', + expect.stringContaining('Failed to load skills from'), + ); + }); + + it('should emit a warning during load if skills directory is not empty but no skills are loaded', async () => { + const extDir = createExtension({ + extensionsDir: userExtensionsDir, + name: 'load-skills-ext', + version: '1.0.0', + }); + + const skillsDir = path.join(extDir, 'skills'); + fs.mkdirSync(skillsDir); + fs.writeFileSync(path.join(skillsDir, 'not-a-skill.txt'), 'hello'); + + await extensionManager.loadExtensions(); + + expect(coreEvents.emitFeedback).toHaveBeenCalledWith( + 'warning', + expect.stringContaining('Failed to load skills from'), + ); + expect(coreEvents.emitFeedback).toHaveBeenCalledWith( + 'warning', + expect.stringContaining( + 'The directory is not empty but no valid skills were discovered', + ), + ); + }); + + it('should succeed if skills are correctly loaded', async () => { + const sourceExtDir = createExtension({ + extensionsDir: tempHomeDir, + name: 'good-skills-ext', + version: '1.0.0', + }); + + const skillsDir = path.join(sourceExtDir, 'skills'); + const skillSubdir = path.join(skillsDir, 'test-skill'); + fs.mkdirSync(skillSubdir, { recursive: true }); + fs.writeFileSync( + path.join(skillSubdir, 'SKILL.md'), + '---\nname: test-skill\ndescription: test desc\n---\nbody', + ); + + await extensionManager.loadExtensions(); + const extension = await extensionManager.installOrUpdateExtension({ + source: sourceExtDir, + type: 'local', + }); + + expect(extension.skills).toHaveLength(1); + expect(extension.skills![0].name).toBe('test-skill'); + // It might be called for other reasons during startup, but shouldn't be called for our skills loading success + // Actually, it shouldn't be called with our warning message + expect(coreEvents.emitFeedback).not.toHaveBeenCalledWith( + 'warning', + expect.stringContaining('Failed to load skills from'), + ); + }); +}); diff --git a/packages/cli/src/config/extension-manager.ts b/packages/cli/src/config/extension-manager.ts index adc7743cf1..3dcd71dac4 100644 --- a/packages/cli/src/config/extension-manager.ts +++ b/packages/cli/src/config/extension-manager.ts @@ -38,6 +38,7 @@ import { logExtensionInstallEvent, logExtensionUninstall, logExtensionUpdateEvent, + loadSkillsFromDir, type ExtensionEvents, type MCPServerConfig, type ExtensionInstallMetadata, @@ -262,10 +263,17 @@ Would you like to attempt to install via "git clone" instead?`, const newHasHooks = fs.existsSync( path.join(localSourcePath, 'hooks', 'hooks.json'), ); - let previousHasHooks = false; - if (isUpdate && previous && previous.hooks) { - previousHasHooks = Object.keys(previous.hooks).length > 0; - } + const previousHasHooks = !!( + isUpdate && + previous && + previous.hooks && + Object.keys(previous.hooks).length > 0 + ); + + const newSkills = await loadSkillsFromDir( + path.join(localSourcePath, 'skills'), + ); + const previousSkills = previous?.skills ?? []; await maybeRequestConsentOrFail( newExtensionConfig, @@ -273,6 +281,8 @@ Would you like to attempt to install via "git clone" instead?`, newHasHooks, previousExtensionConfig, previousHasHooks, + newSkills, + previousSkills, ); const extensionId = getExtensionId(newExtensionConfig, installMetadata); const destinationPath = new ExtensionStorage( @@ -551,6 +561,10 @@ Would you like to attempt to install via "git clone" instead?`, }); } + const skills = await loadSkillsFromDir( + path.join(effectiveExtensionPath, 'skills'), + ); + const extension: GeminiCLIExtension = { name: config.name, version: config.version, @@ -567,6 +581,7 @@ Would you like to attempt to install via "git clone" instead?`, id: getExtensionId(config, installMetadata), settings: config.settings, resolvedSettings, + skills, }; this.loadedExtensions = [...this.loadedExtensions, extension]; @@ -721,6 +736,12 @@ Would you like to attempt to install via "git clone" instead?`, output += `\n ${tool}`; }); } + if (extension.skills && extension.skills.length > 0) { + output += `\n Agent skills:`; + extension.skills.forEach((skill) => { + output += `\n ${skill.name}: ${skill.description}`; + }); + } const resolvedSettings = extension.resolvedSettings; if (resolvedSettings && resolvedSettings.length > 0) { output += `\n Settings:`; diff --git a/packages/cli/src/config/extensions/consent.test.ts b/packages/cli/src/config/extensions/consent.test.ts index ef57161132..72a0b79fb6 100644 --- a/packages/cli/src/config/extensions/consent.test.ts +++ b/packages/cli/src/config/extensions/consent.test.ts @@ -5,15 +5,20 @@ */ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import chalk from 'chalk'; +import * as fs from 'node:fs/promises'; +import * as path from 'node:path'; +import * as os from 'node:os'; import { requestConsentNonInteractive, requestConsentInteractive, maybeRequestConsentOrFail, INSTALL_WARNING_MESSAGE, + SKILLS_WARNING_MESSAGE, } from './consent.js'; import type { ConfirmationRequest } from '../../ui/types.js'; import type { ExtensionConfig } from '../extension.js'; -import { debugLogger } from '@google/gemini-cli-core'; +import { debugLogger, type SkillDefinition } from '@google/gemini-cli-core'; const mockReadline = vi.hoisted(() => ({ createInterface: vi.fn().mockReturnValue({ @@ -40,11 +45,18 @@ vi.mock('@google/gemini-cli-core', async (importOriginal) => { }); describe('consent', () => { - beforeEach(() => { + let tempDir: string; + + beforeEach(async () => { vi.clearAllMocks(); + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'consent-test-')); }); - afterEach(() => { + + afterEach(async () => { vi.restoreAllMocks(); + if (tempDir) { + await fs.rm(tempDir, { recursive: true, force: true }); + } }); describe('requestConsentNonInteractive', () => { @@ -250,6 +262,102 @@ describe('consent', () => { ); expect(requestConsent).toHaveBeenCalledTimes(1); }); + + it('should request consent if skills change', async () => { + const skill1Dir = path.join(tempDir, 'skill1'); + const skill2Dir = path.join(tempDir, 'skill2'); + await fs.mkdir(skill1Dir, { recursive: true }); + await fs.mkdir(skill2Dir, { recursive: true }); + await fs.writeFile(path.join(skill1Dir, 'SKILL.md'), 'body1'); + await fs.writeFile(path.join(skill1Dir, 'extra.txt'), 'extra'); + await fs.writeFile(path.join(skill2Dir, 'SKILL.md'), 'body2'); + + const skill1: SkillDefinition = { + name: 'skill1', + description: 'desc1', + location: path.join(skill1Dir, 'SKILL.md'), + body: 'body1', + }; + const skill2: SkillDefinition = { + name: 'skill2', + description: 'desc2', + location: path.join(skill2Dir, 'SKILL.md'), + body: 'body2', + }; + + const config: ExtensionConfig = { + ...baseConfig, + mcpServers: { + server1: { command: 'npm', args: ['start'] }, + server2: { httpUrl: 'https://remote.com' }, + }, + contextFileName: 'my-context.md', + excludeTools: ['tool1', 'tool2'], + }; + const requestConsent = vi.fn().mockResolvedValue(true); + await maybeRequestConsentOrFail( + config, + requestConsent, + false, + undefined, + false, + [skill1, skill2], + ); + + const expectedConsentString = [ + 'Installing extension "test-ext".', + INSTALL_WARNING_MESSAGE, + 'This extension will run the following MCP servers:', + ' * server1 (local): npm start', + ' * server2 (remote): https://remote.com', + 'This extension will append info to your gemini.md context using my-context.md', + 'This extension will exclude the following core tools: tool1,tool2', + '', + chalk.bold('Agent Skills:'), + SKILLS_WARNING_MESSAGE, + 'This extension will install the following agent skills:', + ` * ${chalk.bold('skill1')}: desc1`, + ` (Location: ${skill1.location}) (2 items in directory)`, + ` * ${chalk.bold('skill2')}: desc2`, + ` (Location: ${skill2.location}) (1 items in directory)`, + '', + ].join('\n'); + + expect(requestConsent).toHaveBeenCalledWith(expectedConsentString); + }); + + it('should show a warning if the skill directory cannot be read', async () => { + const lockedDir = path.join(tempDir, 'locked'); + await fs.mkdir(lockedDir, { recursive: true, mode: 0o000 }); + + const skill: SkillDefinition = { + name: 'locked-skill', + description: 'A skill in a locked dir', + location: path.join(lockedDir, 'SKILL.md'), + body: 'body', + }; + + const requestConsent = vi.fn().mockResolvedValue(true); + try { + await maybeRequestConsentOrFail( + baseConfig, + requestConsent, + false, + undefined, + false, + [skill], + ); + + expect(requestConsent).toHaveBeenCalledWith( + expect.stringContaining( + ` (Location: ${skill.location}) ${chalk.red('⚠️ (Could not count items in directory)')}`, + ), + ); + } finally { + // Restore permissions so cleanup works + await fs.chmod(lockedDir, 0o700); + } + }); }); }); }); diff --git a/packages/cli/src/config/extensions/consent.ts b/packages/cli/src/config/extensions/consent.ts index 36297e6386..47391fd9e6 100644 --- a/packages/cli/src/config/extensions/consent.ts +++ b/packages/cli/src/config/extensions/consent.ts @@ -4,14 +4,22 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { debugLogger } from '@google/gemini-cli-core'; +import * as fs from 'node:fs/promises'; +import * as path from 'node:path'; +import { debugLogger, type SkillDefinition } from '@google/gemini-cli-core'; +import chalk from 'chalk'; import type { ConfirmationRequest } from '../../ui/types.js'; import { escapeAnsiCtrlCodes } from '../../ui/utils/textUtils.js'; import type { ExtensionConfig } from '../extension.js'; -export const INSTALL_WARNING_MESSAGE = - '**The extension you are about to install may have been created by a third-party developer and sourced from a public repository. Google does not vet, endorse, or guarantee the functionality or security of extensions. Please carefully inspect any extension and its source code before installing to understand the permissions it requires and the actions it may perform.**'; +export const INSTALL_WARNING_MESSAGE = chalk.yellow( + 'The extension you are about to install may have been created by a third-party developer and sourced from a public repository. Google does not vet, endorse, or guarantee the functionality or security of extensions. Please carefully inspect any extension and its source code before installing to understand the permissions it requires and the actions it may perform.', +); + +export const SKILLS_WARNING_MESSAGE = chalk.yellow( + "Agent skills inject specialized instructions and domain-specific knowledge into the agent's system prompt. This can change how the agent interprets your requests and interacts with your environment. Review the skill definitions at the location(s) provided below to ensure they meet your security standards.", +); /** * Requests consent from the user to perform an action, by reading a Y/n @@ -38,7 +46,7 @@ export async function requestConsentNonInteractive( * This should not be called from non-interactive mode as it will not work. * * @param consentDescription The description of the thing they will be consenting to. - * @param setExtensionUpdateConfirmationRequest A function to actually add a prompt to the UI. + * @param addExtensionUpdateConfirmationRequest A function to actually add a prompt to the UI. * @returns boolean, whether they consented or not. */ export async function requestConsentInteractive( @@ -82,7 +90,7 @@ async function promptForConsentNonInteractive( * This should not be called from non-interactive mode as it will break the CLI. * * @param prompt A markdown prompt to ask the user - * @param setExtensionUpdateConfirmationRequest Function to update the UI state with the confirmation request. + * @param addExtensionUpdateConfirmationRequest Function to update the UI state with the confirmation request. * @returns Whether or not the user answers yes. */ async function promptForConsentInteractive( @@ -103,10 +111,11 @@ async function promptForConsentInteractive( * Builds a consent string for installing an extension based on it's * extensionConfig. */ -function extensionConsentString( +async function extensionConsentString( extensionConfig: ExtensionConfig, hasHooks: boolean, -): string { + skills: SkillDefinition[] = [], +): Promise { const sanitizedConfig = escapeAnsiCtrlCodes(extensionConfig); const output: string[] = []; const mcpServerEntries = Object.entries(sanitizedConfig.mcpServers || {}); @@ -138,6 +147,24 @@ function extensionConsentString( '⚠️ This extension contains Hooks which can automatically execute commands.', ); } + if (skills.length > 0) { + output.push(`\n${chalk.bold('Agent Skills:')}`); + output.push(SKILLS_WARNING_MESSAGE); + output.push('This extension will install the following agent skills:'); + for (const skill of skills) { + output.push(` * ${chalk.bold(skill.name)}: ${skill.description}`); + const skillDir = path.dirname(skill.location); + let fileCountStr = ''; + try { + const skillDirItems = await fs.readdir(skillDir); + fileCountStr = ` (${skillDirItems.length} items in directory)`; + } catch { + fileCountStr = ` ${chalk.red('⚠️ (Could not count items in directory)')}`; + } + output.push(` (Location: ${skill.location})${fileCountStr}`); + } + output.push(''); + } return output.join('\n'); } @@ -156,12 +183,19 @@ export async function maybeRequestConsentOrFail( hasHooks: boolean, previousExtensionConfig?: ExtensionConfig, previousHasHooks?: boolean, + skills: SkillDefinition[] = [], + previousSkills: SkillDefinition[] = [], ) { - const extensionConsent = extensionConsentString(extensionConfig, hasHooks); + const extensionConsent = await extensionConsentString( + extensionConfig, + hasHooks, + skills, + ); if (previousExtensionConfig) { - const previousExtensionConsent = extensionConsentString( + const previousExtensionConsent = await extensionConsentString( previousExtensionConfig, previousHasHooks ?? false, + previousSkills, ); if (previousExtensionConsent === extensionConsent) { return; diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 2871e51f80..cceae41b14 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -97,7 +97,7 @@ import { DELEGATE_TO_AGENT_TOOL_NAME } from '../tools/tool-names.js'; import { getExperiments } from '../code_assist/experiments/experiments.js'; import { ExperimentFlags } from '../code_assist/experiments/flagNames.js'; import { debugLogger } from '../utils/debugLogger.js'; -import { SkillManager } from '../skills/skillManager.js'; +import { SkillManager, type SkillDefinition } from '../skills/skillManager.js'; import { startupProfiler } from '../telemetry/startupProfiler.js'; import { ApprovalMode } from '../policy/types.js'; @@ -175,6 +175,7 @@ export interface GeminiCLIExtension { hooks?: { [K in HookEventName]?: HookDefinition[] }; settings?: ExtensionSetting[]; resolvedSettings?: ResolvedExtensionSetting[]; + skills?: SkillDefinition[]; } export interface ExtensionInstallMetadata { @@ -732,7 +733,10 @@ export class Config { // Discover skills if enabled if (this.skillsSupport) { - await this.getSkillManager().discoverSkills(this.storage); + await this.getSkillManager().discoverSkills( + this.storage, + this.getExtensions(), + ); this.getSkillManager().setDisabledSkills(this.disabledSkills); // Re-register ActivateSkillTool to update its schema with the discovered enabled skill enums diff --git a/packages/core/src/skills/skillLoader.test.ts b/packages/core/src/skills/skillLoader.test.ts index 5988b853b4..3a42253f9f 100644 --- a/packages/core/src/skills/skillLoader.test.ts +++ b/packages/core/src/skills/skillLoader.test.ts @@ -9,6 +9,7 @@ import * as fs from 'node:fs/promises'; import * as os from 'node:os'; import * as path from 'node:path'; import { loadSkillsFromDir } from './skillLoader.js'; +import { coreEvents } from '../utils/events.js'; describe('skillLoader', () => { let testRootDir: string; @@ -17,6 +18,7 @@ describe('skillLoader', () => { testRootDir = await fs.mkdtemp( path.join(os.tmpdir(), 'skill-loader-test-'), ); + vi.spyOn(coreEvents, 'emitFeedback'); }); afterEach(async () => { @@ -40,18 +42,45 @@ describe('skillLoader', () => { expect(skills[0].description).toBe('A test skill'); expect(skills[0].location).toBe(skillFile); expect(skills[0].body).toBe('# Instructions\nDo something.'); + expect(coreEvents.emitFeedback).not.toHaveBeenCalled(); + }); + + it('should emit feedback when no valid skills are found in a non-empty directory', async () => { + const notASkillDir = path.join(testRootDir, 'not-a-skill'); + await fs.mkdir(notASkillDir, { recursive: true }); + await fs.writeFile(path.join(notASkillDir, 'some-file.txt'), 'hello'); + + const skills = await loadSkillsFromDir(testRootDir); + + expect(skills).toHaveLength(0); + expect(coreEvents.emitFeedback).toHaveBeenCalledWith( + 'warning', + expect.stringContaining('Failed to load skills from'), + ); + }); + + it('should ignore empty directories and not emit feedback', async () => { + const skills = await loadSkillsFromDir(testRootDir); + + expect(skills).toHaveLength(0); + expect(coreEvents.emitFeedback).not.toHaveBeenCalled(); }); it('should ignore directories without SKILL.md', async () => { const notASkillDir = path.join(testRootDir, 'not-a-skill'); await fs.mkdir(notASkillDir, { recursive: true }); + // With a subdirectory, even if empty, it might still trigger readdir + // But my current logic is if discoveredSkills.length === 0, then check readdir + // If readdir is empty, it's fine. + const skills = await loadSkillsFromDir(testRootDir); expect(skills).toHaveLength(0); + // If notASkillDir is empty, no warning. }); - it('should ignore SKILL.md without valid frontmatter', async () => { + it('should ignore SKILL.md without valid frontmatter and emit warning if directory is not empty', async () => { const skillDir = path.join(testRootDir, 'invalid-skill'); await fs.mkdir(skillDir, { recursive: true }); const skillFile = path.join(skillDir, 'SKILL.md'); @@ -60,10 +89,15 @@ describe('skillLoader', () => { const skills = await loadSkillsFromDir(testRootDir); expect(skills).toHaveLength(0); + expect(coreEvents.emitFeedback).toHaveBeenCalledWith( + 'warning', + expect.stringContaining('Failed to load skills from'), + ); }); it('should return empty array for non-existent directory', async () => { const skills = await loadSkillsFromDir('/non/existent/path'); expect(skills).toEqual([]); + expect(coreEvents.emitFeedback).not.toHaveBeenCalled(); }); }); diff --git a/packages/core/src/skills/skillLoader.ts b/packages/core/src/skills/skillLoader.ts index 9dc86fd170..e9de2db2f0 100644 --- a/packages/core/src/skills/skillLoader.ts +++ b/packages/core/src/skills/skillLoader.ts @@ -9,6 +9,7 @@ import * as path from 'node:path'; import { glob } from 'glob'; import yaml from 'js-yaml'; import { debugLogger } from '../utils/debugLogger.js'; +import { coreEvents } from '../utils/events.js'; /** * Represents the definition of an Agent Skill. @@ -55,8 +56,22 @@ export async function loadSkillsFromDir( discoveredSkills.push(metadata); } } + + if (discoveredSkills.length === 0) { + const files = await fs.readdir(absoluteSearchPath); + if (files.length > 0) { + coreEvents.emitFeedback( + 'warning', + `Failed to load skills from ${absoluteSearchPath}. The directory is not empty but no valid skills were discovered. Please ensure SKILL.md files are present in subdirectories and have valid frontmatter.`, + ); + } + } } catch (error) { - debugLogger.log(`Error discovering skills in ${dir}:`, error); + coreEvents.emitFeedback( + 'warning', + `Error discovering skills in ${dir}:`, + error, + ); } return discoveredSkills; diff --git a/packages/core/src/skills/skillManager.test.ts b/packages/core/src/skills/skillManager.test.ts index 47e2ae39b2..4bd200e4d7 100644 --- a/packages/core/src/skills/skillManager.test.ts +++ b/packages/core/src/skills/skillManager.test.ts @@ -10,6 +10,7 @@ import * as os from 'node:os'; import * as path from 'node:path'; import { SkillManager } from './skillManager.js'; import { Storage } from '../config/storage.js'; +import { type GeminiCLIExtension } from '../config/config.js'; describe('SkillManager', () => { let testRootDir: string; @@ -25,39 +26,116 @@ describe('SkillManager', () => { vi.restoreAllMocks(); }); - it('should discover skills from Storage with project precedence', async () => { + it('should discover skills from extensions, user, and project with precedence', async () => { const userDir = path.join(testRootDir, 'user'); const projectDir = path.join(testRootDir, 'project'); await fs.mkdir(path.join(userDir, 'skill-a'), { recursive: true }); - await fs.mkdir(path.join(projectDir, 'skill-a'), { recursive: true }); + await fs.mkdir(path.join(projectDir, 'skill-b'), { recursive: true }); await fs.writeFile( path.join(userDir, 'skill-a', 'SKILL.md'), `--- -name: skill-a +name: skill-user description: user-desc --- `, ); await fs.writeFile( - path.join(projectDir, 'skill-a', 'SKILL.md'), + path.join(projectDir, 'skill-b', 'SKILL.md'), `--- -name: skill-a +name: skill-project description: project-desc --- `, ); + const mockExtension: GeminiCLIExtension = { + name: 'test-ext', + version: '1.0.0', + isActive: true, + path: '/ext', + contextFiles: [], + id: 'ext-id', + skills: [ + { + name: 'skill-extension', + description: 'ext-desc', + location: '/ext/skills/SKILL.md', + body: 'body', + }, + ], + }; + vi.spyOn(Storage, 'getUserSkillsDir').mockReturnValue(userDir); const storage = new Storage('/dummy'); vi.spyOn(storage, 'getProjectSkillsDir').mockReturnValue(projectDir); const service = new SkillManager(); - await service.discoverSkills(storage); + await service.discoverSkills(storage, [mockExtension]); + + const skills = service.getSkills(); + expect(skills).toHaveLength(3); + const names = skills.map((s) => s.name); + expect(names).toContain('skill-extension'); + expect(names).toContain('skill-user'); + expect(names).toContain('skill-project'); + }); + + it('should respect precedence: Project > User > Extension', async () => { + const userDir = path.join(testRootDir, 'user'); + const projectDir = path.join(testRootDir, 'project'); + await fs.mkdir(path.join(userDir, 'skill'), { recursive: true }); + await fs.mkdir(path.join(projectDir, 'skill'), { recursive: true }); + + await fs.writeFile( + path.join(userDir, 'skill', 'SKILL.md'), + `--- +name: same-name +description: user-desc +--- +`, + ); + await fs.writeFile( + path.join(projectDir, 'skill', 'SKILL.md'), + `--- +name: same-name +description: project-desc +--- +`, + ); + + const mockExtension: GeminiCLIExtension = { + name: 'test-ext', + version: '1.0.0', + isActive: true, + path: '/ext', + contextFiles: [], + id: 'ext-id', + skills: [ + { + name: 'same-name', + description: 'ext-desc', + location: '/ext/skills/SKILL.md', + body: 'body', + }, + ], + }; + + vi.spyOn(Storage, 'getUserSkillsDir').mockReturnValue(userDir); + const storage = new Storage('/dummy'); + vi.spyOn(storage, 'getProjectSkillsDir').mockReturnValue(projectDir); + + const service = new SkillManager(); + await service.discoverSkills(storage, [mockExtension]); const skills = service.getSkills(); expect(skills).toHaveLength(1); expect(skills[0].description).toBe('project-desc'); + + // Test User > Extension + vi.spyOn(storage, 'getProjectSkillsDir').mockReturnValue('/non-existent'); + await service.discoverSkills(storage, [mockExtension]); + expect(service.getSkills()[0].description).toBe('user-desc'); }); it('should filter disabled skills in getSkills but not in getAllSkills', async () => { diff --git a/packages/core/src/skills/skillManager.ts b/packages/core/src/skills/skillManager.ts index a8e14ba388..22e0858cc8 100644 --- a/packages/core/src/skills/skillManager.ts +++ b/packages/core/src/skills/skillManager.ts @@ -6,6 +6,9 @@ import { Storage } from '../config/storage.js'; import { type SkillDefinition, loadSkillsFromDir } from './skillLoader.js'; +import type { GeminiCLIExtension } from '../config/config.js'; + +export { type SkillDefinition }; export class SkillManager { private skills: SkillDefinition[] = []; @@ -19,17 +22,27 @@ export class SkillManager { } /** - * Discovers skills from standard user and project locations. - * Project skills take precedence over user skills. + * Discovers skills from standard user and project locations, as well as extensions. + * Precedence: Extensions (lowest) -> User -> Project (highest). */ - async discoverSkills(storage: Storage): Promise { + async discoverSkills( + storage: Storage, + extensions: GeminiCLIExtension[] = [], + ): Promise { this.clearSkills(); - // 1. User skills + // 1. Extension skills (lowest precedence) + for (const extension of extensions) { + if (extension.isActive && extension.skills) { + this.addSkillsWithPrecedence(extension.skills); + } + } + + // 2. User skills const userSkills = await loadSkillsFromDir(Storage.getUserSkillsDir()); this.addSkillsWithPrecedence(userSkills); - // 2. Project skills (highest precedence) + // 3. Project skills (highest precedence) const projectSkills = await loadSkillsFromDir( storage.getProjectSkillsDir(), );