From 461c277bf2de4057a704505315d138cad9ab7ead Mon Sep 17 00:00:00 2001 From: "N. Taylor Mullen" Date: Fri, 9 Jan 2026 22:26:58 -0800 Subject: [PATCH] Support for Built-in Agent Skills (#16045) --- packages/cli/src/commands/skills/list.test.ts | 59 +- packages/cli/src/commands/skills/list.ts | 29 +- .../config/extension-manager-skills.test.ts | 141 ++-- .../cli/src/ui/commands/skillsCommand.test.ts | 47 +- packages/cli/src/ui/commands/skillsCommand.ts | 20 +- .../cli/src/ui/components/Composer.test.tsx | 1 + .../src/ui/components/StatusDisplay.test.tsx | 1 + .../cli/src/ui/components/StatusDisplay.tsx | 4 +- .../ui/components/views/SkillsList.test.tsx | 21 + .../src/ui/components/views/SkillsList.tsx | 32 +- .../src/ui/hooks/useQuotaAndFallback.test.ts | 23 +- .../cli/src/ui/utils/rewindFileOps.test.ts | 717 +++++++++--------- packages/core/src/skills/skillLoader.ts | 2 + packages/core/src/skills/skillManager.test.ts | 41 + packages/core/src/skills/skillManager.ts | 36 +- .../core/src/tools/activate-skill.test.ts | 28 + packages/core/src/tools/activate-skill.ts | 4 + 17 files changed, 755 insertions(+), 451 deletions(-) diff --git a/packages/cli/src/commands/skills/list.test.ts b/packages/cli/src/commands/skills/list.test.ts index ce5a5d0cf8..81230b33a2 100644 --- a/packages/cli/src/commands/skills/list.test.ts +++ b/packages/cli/src/commands/skills/list.test.ts @@ -65,7 +65,7 @@ describe('skills list command', () => { }; mockLoadCliConfig.mockResolvedValue(mockConfig as unknown as Config); - await handleList(); + await handleList({}); expect(emitConsoleLog).toHaveBeenCalledWith( 'log', @@ -96,7 +96,7 @@ describe('skills list command', () => { }; mockLoadCliConfig.mockResolvedValue(mockConfig as unknown as Config); - await handleList(); + await handleList({}); expect(emitConsoleLog).toHaveBeenCalledWith( 'log', @@ -120,10 +120,63 @@ describe('skills list command', () => { ); }); + it('should filter built-in skills by default and show them with { all: true }', async () => { + const skills = [ + { + name: 'regular', + description: 'desc1', + disabled: false, + location: '/loc1', + }, + { + name: 'builtin', + description: 'desc2', + disabled: false, + location: '/loc2', + isBuiltin: true, + }, + ]; + const mockConfig = { + initialize: vi.fn().mockResolvedValue(undefined), + getSkillManager: vi.fn().mockReturnValue({ + getAllSkills: vi.fn().mockReturnValue(skills), + }), + }; + mockLoadCliConfig.mockResolvedValue(mockConfig as unknown as Config); + + // Default + await handleList({ all: false }); + expect(emitConsoleLog).toHaveBeenCalledWith( + 'log', + expect.stringContaining('regular'), + ); + expect(emitConsoleLog).not.toHaveBeenCalledWith( + 'log', + expect.stringContaining('builtin'), + ); + + vi.clearAllMocks(); + + // With all: true + await handleList({ all: true }); + expect(emitConsoleLog).toHaveBeenCalledWith( + 'log', + expect.stringContaining('regular'), + ); + expect(emitConsoleLog).toHaveBeenCalledWith( + 'log', + expect.stringContaining('builtin'), + ); + expect(emitConsoleLog).toHaveBeenCalledWith( + 'log', + expect.stringContaining(chalk.gray(' [Built-in]')), + ); + }); + it('should throw an error when listing fails', async () => { mockLoadCliConfig.mockRejectedValue(new Error('List failed')); - await expect(handleList()).rejects.toThrow('List failed'); + await expect(handleList({})).rejects.toThrow('List failed'); }); }); diff --git a/packages/cli/src/commands/skills/list.ts b/packages/cli/src/commands/skills/list.ts index 29b234df98..17f225c510 100644 --- a/packages/cli/src/commands/skills/list.ts +++ b/packages/cli/src/commands/skills/list.ts @@ -11,7 +11,7 @@ import { loadCliConfig, type CliArgs } from '../../config/config.js'; import { exitCli } from '../utils.js'; import chalk from 'chalk'; -export async function handleList() { +export async function handleList(args: { all?: boolean }) { const workspaceDir = process.cwd(); const settings = loadSettings(workspaceDir); @@ -28,7 +28,17 @@ export async function handleList() { await config.initialize(); const skillManager = config.getSkillManager(); - const skills = skillManager.getAllSkills(); + const skills = args.all + ? skillManager.getAllSkills() + : skillManager.getAllSkills().filter((s) => !s.isBuiltin); + + // Sort skills: non-built-in first, then alphabetically by name + skills.sort((a, b) => { + if (a.isBuiltin === b.isBuiltin) { + return a.name.localeCompare(b.name); + } + return a.isBuiltin ? 1 : -1; + }); if (skills.length === 0) { debugLogger.log('No skills discovered.'); @@ -43,7 +53,9 @@ export async function handleList() { ? chalk.red('[Disabled]') : chalk.green('[Enabled]'); - debugLogger.log(`${chalk.bold(skill.name)} ${status}`); + const builtinSuffix = skill.isBuiltin ? chalk.gray(' [Built-in]') : ''; + + debugLogger.log(`${chalk.bold(skill.name)} ${status}${builtinSuffix}`); debugLogger.log(` Description: ${skill.description}`); debugLogger.log(` Location: ${skill.location}`); debugLogger.log(''); @@ -53,9 +65,14 @@ export async function handleList() { export const listCommand: CommandModule = { command: 'list', describe: 'Lists discovered agent skills.', - builder: (yargs) => yargs, - handler: async () => { - await handleList(); + builder: (yargs) => + yargs.option('all', { + type: 'boolean', + description: 'Show all skills, including built-in ones.', + default: false, + }), + handler: async (argv) => { + await handleList({ all: argv['all'] as boolean }); await exitCli(); }, }; diff --git a/packages/cli/src/config/extension-manager-skills.test.ts b/packages/cli/src/config/extension-manager-skills.test.ts index 585f2adc51..526db275e2 100644 --- a/packages/cli/src/config/extension-manager-skills.test.ts +++ b/packages/cli/src/config/extension-manager-skills.test.ts @@ -4,26 +4,27 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } 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 { debugLogger, coreEvents } from '@google/gemini-cli-core'; +import { type Settings } from './settings.js'; import { createExtension } from '../test-utils/createExtension.js'; import { EXTENSIONS_DIRECTORY_NAME } from './extensions/variables.js'; -import { coreEvents, debugLogger } from '@google/gemini-cli-core'; -const mockHomedir = vi.hoisted(() => vi.fn()); +const mockHomedir = vi.hoisted(() => vi.fn(() => '/tmp/mock-home')); -vi.mock('os', async (importOriginal) => { - const mockedOs = await importOriginal(); +vi.mock('node:os', async (importOriginal) => { + const actual = await importOriginal(); return { - ...mockedOs, + ...actual, homedir: mockHomedir, }; }); +// Mock @google/gemini-cli-core vi.mock('@google/gemini-cli-core', async (importOriginal) => { const actual = await importOriginal(); @@ -34,92 +35,130 @@ vi.mock('@google/gemini-cli-core', async (importOriginal) => { }); describe('ExtensionManager skills validation', () => { - let tempHomeDir: string; - let tempWorkspaceDir: string; - let userExtensionsDir: string; let extensionManager: ExtensionManager; + let tempDir: string; + let extensionsDir: string; 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.clearAllMocks(); vi.spyOn(coreEvents, 'emitFeedback'); vi.spyOn(debugLogger, 'debug').mockImplementation(() => {}); + + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gemini-test-')); + mockHomedir.mockReturnValue(tempDir); + + // Create the extensions directory that ExtensionManager expects + extensionsDir = path.join(tempDir, '.gemini', EXTENSIONS_DIRECTORY_NAME); + fs.mkdirSync(extensionsDir, { recursive: true }); + + extensionManager = new ExtensionManager({ + settings: { + telemetry: { enabled: false }, + trustedFolders: [tempDir], + } as unknown as Settings, + requestConsent: vi.fn().mockResolvedValue(true), + requestSetting: vi.fn(), + workspaceDir: tempDir, + }); }); afterEach(() => { - fs.rmSync(tempHomeDir, { recursive: true, force: true }); - vi.restoreAllMocks(); + try { + fs.rmSync(tempDir, { recursive: true, force: true }); + } catch { + // ignore + } }); it('should emit a warning during install if skills directory is not empty but no skills are loaded', async () => { - const sourceExtDir = createExtension({ - extensionsDir: tempHomeDir, + // Create a source extension + const sourceDir = path.join(tempDir, 'source-ext'); + createExtension({ + extensionsDir: sourceDir, // createExtension appends name name: 'skills-ext', version: '1.0.0', + installMetadata: { + type: 'local', + source: path.join(sourceDir, 'skills-ext'), + }, }); + const extensionPath = path.join(sourceDir, 'skills-ext'); - const skillsDir = path.join(sourceExtDir, 'skills'); + // Add invalid skills content + const skillsDir = path.join(extensionPath, 'skills'); fs.mkdirSync(skillsDir); fs.writeFileSync(path.join(skillsDir, 'not-a-skill.txt'), 'hello'); await extensionManager.loadExtensions(); - const extension = await extensionManager.installOrUpdateExtension({ - source: sourceExtDir, + + await extensionManager.installOrUpdateExtension({ type: 'local', + source: extensionPath, }); - expect(extension.name).toBe('skills-ext'); expect(debugLogger.debug).toHaveBeenCalledWith( 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', + // 1. Create a source extension + const sourceDir = path.join(tempDir, 'source-ext-load'); + createExtension({ + extensionsDir: sourceDir, + name: 'skills-ext-load', version: '1.0.0', }); + const sourceExtPath = path.join(sourceDir, 'skills-ext-load'); - const skillsDir = path.join(extDir, 'skills'); + // Add invalid skills content + const skillsDir = path.join(sourceExtPath, 'skills'); fs.mkdirSync(skillsDir); fs.writeFileSync(path.join(skillsDir, 'not-a-skill.txt'), 'hello'); + // 2. Install it to ensure correct disk state await extensionManager.loadExtensions(); + await extensionManager.installOrUpdateExtension({ + type: 'local', + source: sourceExtPath, + }); + + // Clear the spy + vi.mocked(debugLogger.debug).mockClear(); + + // 3. Create a fresh ExtensionManager to force loading from disk + const newExtensionManager = new ExtensionManager({ + settings: { + telemetry: { enabled: false }, + trustedFolders: [tempDir], + } as unknown as Settings, + requestConsent: vi.fn().mockResolvedValue(true), + requestSetting: vi.fn(), + workspaceDir: tempDir, + }); + + // 4. Load extensions + await newExtensionManager.loadExtensions(); expect(debugLogger.debug).toHaveBeenCalledWith( expect.stringContaining('Failed to load skills from'), ); - expect(debugLogger.debug).toHaveBeenCalledWith( - 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, + const sourceDir = path.join(tempDir, 'source-ext-good'); + createExtension({ + extensionsDir: sourceDir, name: 'good-skills-ext', version: '1.0.0', + installMetadata: { + type: 'local', + source: path.join(sourceDir, 'good-skills-ext'), + }, }); + const extensionPath = path.join(sourceDir, 'good-skills-ext'); - const skillsDir = path.join(sourceExtDir, 'skills'); + const skillsDir = path.join(extensionPath, 'skills'); const skillSubdir = path.join(skillsDir, 'test-skill'); fs.mkdirSync(skillSubdir, { recursive: true }); fs.writeFileSync( @@ -128,15 +167,13 @@ describe('ExtensionManager skills validation', () => { ); await extensionManager.loadExtensions(); + const extension = await extensionManager.installOrUpdateExtension({ - source: sourceExtDir, type: 'local', + source: extensionPath, }); - 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(extension.name).toBe('good-skills-ext'); expect(debugLogger.debug).not.toHaveBeenCalledWith( expect.stringContaining('Failed to load skills from'), ); diff --git a/packages/cli/src/ui/commands/skillsCommand.test.ts b/packages/cli/src/ui/commands/skillsCommand.test.ts index e90c695e0c..3bcfa6ba06 100644 --- a/packages/cli/src/ui/commands/skillsCommand.test.ts +++ b/packages/cli/src/ui/commands/skillsCommand.test.ts @@ -6,7 +6,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { skillsCommand } from './skillsCommand.js'; -import { MessageType } from '../types.js'; +import { MessageType, type HistoryItemSkillsList } from '../types.js'; import { createMockCommandContext } from '../../test-utils/mockCommandContext.js'; import type { CommandContext } from './types.js'; import type { Config, SkillDefinition } from '@google/gemini-cli-core'; @@ -136,6 +136,51 @@ describe('skillsCommand', () => { ); }); + it('should filter built-in skills by default and show them with "all"', async () => { + const skillManager = context.services.config!.getSkillManager(); + const mockSkills = [ + { + name: 'regular', + description: 'desc1', + location: '/loc1', + body: 'body1', + }, + { + name: 'builtin', + description: 'desc2', + location: '/loc2', + body: 'body2', + isBuiltin: true, + }, + ]; + vi.mocked(skillManager.getAllSkills).mockReturnValue(mockSkills); + + const listCmd = skillsCommand.subCommands!.find((s) => s.name === 'list')!; + + // By default, only regular skills + await listCmd.action!(context, ''); + let lastCall = vi + .mocked(context.ui.addItem) + .mock.calls.at(-1)![0] as HistoryItemSkillsList; + expect(lastCall.skills).toHaveLength(1); + expect(lastCall.skills[0].name).toBe('regular'); + + // With "all", show both + await listCmd.action!(context, 'all'); + lastCall = vi + .mocked(context.ui.addItem) + .mock.calls.at(-1)![0] as HistoryItemSkillsList; + expect(lastCall.skills).toHaveLength(2); + expect(lastCall.skills.map((s) => s.name)).toContain('builtin'); + + // With "--all", show both + await listCmd.action!(context, '--all'); + lastCall = vi + .mocked(context.ui.addItem) + .mock.calls.at(-1)![0] as HistoryItemSkillsList; + expect(lastCall.skills).toHaveLength(2); + }); + describe('disable/enable', () => { beforeEach(() => { context.services.settings.merged.skills = { disabled: [] }; diff --git a/packages/cli/src/ui/commands/skillsCommand.ts b/packages/cli/src/ui/commands/skillsCommand.ts index d769b9941d..ca476a32ea 100644 --- a/packages/cli/src/ui/commands/skillsCommand.ts +++ b/packages/cli/src/ui/commands/skillsCommand.ts @@ -23,12 +23,18 @@ async function listAction( context: CommandContext, args: string, ): Promise { - const subCommand = args.trim(); + const subArgs = args.trim().split(/\s+/); // Default to SHOWING descriptions. The user can hide them with 'nodesc'. let useShowDescriptions = true; - if (subCommand === 'nodesc') { - useShowDescriptions = false; + let showAll = false; + + for (const arg of subArgs) { + if (arg === 'nodesc' || arg === '--nodesc') { + useShowDescriptions = false; + } else if (arg === 'all' || arg === '--all') { + showAll = true; + } } const skillManager = context.services.config?.getSkillManager(); @@ -43,7 +49,9 @@ async function listAction( return; } - const skills = skillManager.getAllSkills(); + const skills = showAll + ? skillManager.getAllSkills() + : skillManager.getAllSkills().filter((s) => !s.isBuiltin); const skillsListItem: HistoryItemSkillsList = { type: MessageType.SKILLS_LIST, @@ -53,6 +61,7 @@ async function listAction( disabled: skill.disabled, location: skill.location, body: skill.body, + isBuiltin: skill.isBuiltin, })), showDescriptions: useShowDescriptions, }; @@ -278,7 +287,8 @@ export const skillsCommand: SlashCommand = { subCommands: [ { name: 'list', - description: 'List available agent skills. Usage: /skills list [nodesc]', + description: + 'List available agent skills. Usage: /skills list [nodesc] [all]', kind: CommandKind.BUILT_IN, action: listAction, }, diff --git a/packages/cli/src/ui/components/Composer.test.tsx b/packages/cli/src/ui/components/Composer.test.tsx index b48e18cc00..e5e6e02830 100644 --- a/packages/cli/src/ui/components/Composer.test.tsx +++ b/packages/cli/src/ui/components/Composer.test.tsx @@ -154,6 +154,7 @@ const createMockConfig = (overrides = {}) => ({ }), getSkillManager: () => ({ getSkills: () => [], + getDisplayableSkills: () => [], }), getMcpClientManager: () => ({ getMcpServers: () => ({}), diff --git a/packages/cli/src/ui/components/StatusDisplay.test.tsx b/packages/cli/src/ui/components/StatusDisplay.test.tsx index e5a64bd3f3..8e3bdff68c 100644 --- a/packages/cli/src/ui/components/StatusDisplay.test.tsx +++ b/packages/cli/src/ui/components/StatusDisplay.test.tsx @@ -45,6 +45,7 @@ const createMockConfig = (overrides = {}) => ({ })), getSkillManager: vi.fn().mockImplementation(() => ({ getSkills: vi.fn(() => ['skill1', 'skill2']), + getDisplayableSkills: vi.fn(() => ['skill1', 'skill2']), })), ...overrides, }); diff --git a/packages/cli/src/ui/components/StatusDisplay.tsx b/packages/cli/src/ui/components/StatusDisplay.tsx index 367be17593..40925fa5ba 100644 --- a/packages/cli/src/ui/components/StatusDisplay.tsx +++ b/packages/cli/src/ui/components/StatusDisplay.tsx @@ -25,7 +25,7 @@ export const StatusDisplay: React.FC = ({ const config = useConfig(); if (process.env['GEMINI_SYSTEM_MD']) { - return |⌐■_■| ; + return |⌐■_■|; } if (uiState.ctrlCPressedOnce) { @@ -69,7 +69,7 @@ export const StatusDisplay: React.FC = ({ blockedMcpServers={ config.getMcpClientManager()?.getBlockedMcpServers() ?? [] } - skillCount={config.getSkillManager().getSkills().length} + skillCount={config.getSkillManager().getDisplayableSkills().length} /> ); } diff --git a/packages/cli/src/ui/components/views/SkillsList.test.tsx b/packages/cli/src/ui/components/views/SkillsList.test.tsx index e04958cc9a..d12871803e 100644 --- a/packages/cli/src/ui/components/views/SkillsList.test.tsx +++ b/packages/cli/src/ui/components/views/SkillsList.test.tsx @@ -105,4 +105,25 @@ describe('SkillsList Component', () => { unmount(); }); + + it('should render [Built-in] tag for built-in skills', () => { + const builtinSkill: SkillDefinition = { + name: 'builtin-skill', + description: 'A built-in skill', + disabled: false, + location: 'loc', + body: 'body', + isBuiltin: true, + }; + + const { lastFrame, unmount } = render( + , + ); + const output = lastFrame(); + + expect(output).toContain('builtin-skill'); + expect(output).toContain('Built-in'); + + unmount(); + }); }); diff --git a/packages/cli/src/ui/components/views/SkillsList.tsx b/packages/cli/src/ui/components/views/SkillsList.tsx index ebb3c8519b..64e2d3efd7 100644 --- a/packages/cli/src/ui/components/views/SkillsList.tsx +++ b/packages/cli/src/ui/components/views/SkillsList.tsx @@ -18,24 +18,32 @@ export const SkillsList: React.FC = ({ skills, showDescriptions, }) => { - const enabledSkills = skills - .filter((s) => !s.disabled) - .sort((a, b) => a.name.localeCompare(b.name)); + const sortSkills = (a: SkillDefinition, b: SkillDefinition) => { + if (a.isBuiltin === b.isBuiltin) { + return a.name.localeCompare(b.name); + } + return a.isBuiltin ? 1 : -1; + }; - const disabledSkills = skills - .filter((s) => s.disabled) - .sort((a, b) => a.name.localeCompare(b.name)); + const enabledSkills = skills.filter((s) => !s.disabled).sort(sortSkills); + + const disabledSkills = skills.filter((s) => s.disabled).sort(sortSkills); const renderSkill = (skill: SkillDefinition) => ( {' '}- - - {skill.name} - + + + {skill.name} + + {skill.isBuiltin && ( + {' [Built-in]'} + )} + {showDescriptions && skill.description && ( { vi.spyOn(mockConfig, 'setQuotaErrorOccurred'); vi.spyOn(mockConfig, 'setModel'); vi.spyOn(mockConfig, 'setActiveModel'); + vi.spyOn(mockConfig, 'activateFallbackMode'); }); afterEach(() => { @@ -165,8 +166,10 @@ describe('useQuotaAndFallback', () => { const intent = await promise!; expect(intent).toBe('retry_always'); - // Verify setActiveModel was called - expect(mockConfig.setActiveModel).toHaveBeenCalledWith('gemini-flash'); + // Verify activateFallbackMode was called + expect(mockConfig.activateFallbackMode).toHaveBeenCalledWith( + 'gemini-flash', + ); // The pending request should be cleared from the state expect(result.current.proQuotaRequest).toBeNull(); @@ -279,8 +282,10 @@ describe('useQuotaAndFallback', () => { const intent = await promise!; expect(intent).toBe('retry_always'); - // Verify setActiveModel was called - expect(mockConfig.setActiveModel).toHaveBeenCalledWith('model-B'); + // Verify activateFallbackMode was called + expect(mockConfig.activateFallbackMode).toHaveBeenCalledWith( + 'model-B', + ); // The pending request should be cleared from the state expect(result.current.proQuotaRequest).toBeNull(); @@ -337,8 +342,8 @@ To disable gemini-3-pro-preview, disable "Preview features" in /settings.`, const intent = await promise!; expect(intent).toBe('retry_always'); - // Verify setActiveModel was called - expect(mockConfig.setActiveModel).toHaveBeenCalledWith( + // Verify activateFallbackMode was called + expect(mockConfig.activateFallbackMode).toHaveBeenCalledWith( 'gemini-2.5-pro', ); @@ -425,8 +430,10 @@ To disable gemini-3-pro-preview, disable "Preview features" in /settings.`, expect(intent).toBe('retry_always'); expect(result.current.proQuotaRequest).toBeNull(); - // Verify setActiveModel was called - expect(mockConfig.setActiveModel).toHaveBeenCalledWith('gemini-flash'); + // Verify activateFallbackMode was called + expect(mockConfig.activateFallbackMode).toHaveBeenCalledWith( + 'gemini-flash', + ); // Verify quota error flags are reset expect(mockSetModelSwitchedFromQuotaError).toHaveBeenCalledWith(false); diff --git a/packages/cli/src/ui/utils/rewindFileOps.test.ts b/packages/cli/src/ui/utils/rewindFileOps.test.ts index a70dd0337f..fa0a1df51d 100644 --- a/packages/cli/src/ui/utils/rewindFileOps.test.ts +++ b/packages/cli/src/ui/utils/rewindFileOps.test.ts @@ -4,219 +4,204 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import fs from 'node:fs/promises'; import { calculateTurnStats, calculateRewindImpact, revertFileChanges, } from './rewindFileOps.js'; -import type { - ConversationRecord, - MessageRecord, - ToolCallRecord, +import { + coreEvents, + type ConversationRecord, + type MessageRecord, + type ToolCallRecord, } from '@google/gemini-cli-core'; -import { coreEvents } from '@google/gemini-cli-core'; -import fs from 'node:fs/promises'; -import path from 'node:path'; -vi.mock('node:fs/promises'); +// Mock fs/promises +vi.mock('node:fs/promises', () => ({ + default: { + readFile: vi.fn(), + writeFile: vi.fn(), + rm: vi.fn(), + unlink: vi.fn(), + }, +})); + +// Mock @google/gemini-cli-core vi.mock('@google/gemini-cli-core', async (importOriginal) => { const actual = await importOriginal(); return { ...actual, - coreEvents: { - emitFeedback: vi.fn(), + debugLogger: { + log: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), }, + getFileDiffFromResultDisplay: vi.fn(), + computeAddedAndRemovedLines: vi.fn(), }; }); describe('rewindFileOps', () => { - const mockConversation: ConversationRecord = { - sessionId: 'test-session', - projectHash: 'hash', - startTime: 'time', - lastUpdated: 'time', - messages: [], - }; - beforeEach(() => { vi.clearAllMocks(); - }); - - afterEach(() => { - vi.restoreAllMocks(); + vi.spyOn(coreEvents, 'emitFeedback'); }); describe('calculateTurnStats', () => { it('returns null if no edits found after user message', () => { - const userMsg: MessageRecord = { - type: 'user', - content: 'hello', - id: '1', - timestamp: '1', + const userMsg = { type: 'user' } as unknown as MessageRecord; + const conversation = { + messages: [ + userMsg, + { type: 'gemini', text: 'Hello' } as unknown as MessageRecord, + ], }; - const geminiMsg: MessageRecord = { - type: 'gemini', - content: 'hi', - id: '2', - timestamp: '2', - }; - mockConversation.messages = [userMsg, geminiMsg]; - - const stats = calculateTurnStats(mockConversation, userMsg); - expect(stats).toBeNull(); + const result = calculateTurnStats( + conversation as unknown as ConversationRecord, + userMsg, + ); + expect(result).toBeNull(); }); - it('calculates stats for single turn correctly', () => { - const userMsg: MessageRecord = { - type: 'user', - content: 'hello', - id: '1', - timestamp: '1', - }; - const toolMsg: MessageRecord = { - type: 'gemini', - id: '2', - timestamp: '2', - content: '', - toolCalls: [ + it('calculates stats for single turn correctly', async () => { + const { getFileDiffFromResultDisplay, computeAddedAndRemovedLines } = + await import('@google/gemini-cli-core'); + vi.mocked(getFileDiffFromResultDisplay).mockReturnValue({ + filePath: 'test.ts', + fileName: 'test.ts', + originalContent: 'old', + newContent: 'new', + isNewFile: false, + diffStat: { + model_added_lines: 0, + model_removed_lines: 0, + model_added_chars: 0, + model_removed_chars: 0, + user_added_lines: 0, + user_removed_lines: 0, + user_added_chars: 0, + user_removed_chars: 0, + }, + fileDiff: 'diff', + }); + vi.mocked(computeAddedAndRemovedLines).mockReturnValue({ + addedLines: 3, + removedLines: 3, + }); + + const userMsg = { type: 'user' } as unknown as MessageRecord; + const conversation = { + messages: [ + userMsg, { - name: 'replace', - id: 'tool-call-1', - status: 'success', - timestamp: '2', - args: {}, - resultDisplay: { - fileName: 'file1.ts', - filePath: '/file1.ts', - originalContent: 'old', - newContent: 'new', - isNewFile: false, - diffStat: { - model_added_lines: 5, - model_removed_lines: 2, - user_added_lines: 0, - user_removed_lines: 0, - model_added_chars: 100, - model_removed_chars: 20, - user_added_chars: 0, - user_removed_chars: 0, + type: 'gemini', + toolCalls: [ + { + name: 'replace', + args: {}, + resultDisplay: 'diff', }, - }, - }, - ] as unknown as ToolCallRecord[], + ], + } as unknown as MessageRecord, + ], }; - const userMsg2: MessageRecord = { - type: 'user', - content: 'next', - id: '3', - timestamp: '3', - }; - - mockConversation.messages = [userMsg, toolMsg, userMsg2]; - - const stats = calculateTurnStats(mockConversation, userMsg); - expect(stats).toEqual({ - addedLines: 5, - removedLines: 2, + const result = calculateTurnStats( + conversation as unknown as ConversationRecord, + userMsg, + ); + expect(result).toEqual({ fileCount: 1, + addedLines: 3, + removedLines: 3, }); }); }); describe('calculateRewindImpact', () => { - it('calculates cumulative stats across multiple turns', () => { - const userMsg1: MessageRecord = { - type: 'user', - content: 'start', - id: '1', - timestamp: '1', - }; - const toolMsg1: MessageRecord = { - type: 'gemini', - id: '2', - timestamp: '2', - content: '', - toolCalls: [ - { - name: 'replace', - id: 'tool-call-1', - status: 'success', - timestamp: '2', - args: {}, - resultDisplay: { - fileName: 'file1.ts', - filePath: '/file1.ts', - fileDiff: 'diff1', - originalContent: 'old', - newContent: 'new', - isNewFile: false, - diffStat: { - model_added_lines: 5, - model_removed_lines: 2, - user_added_lines: 0, - user_removed_lines: 0, - model_added_chars: 0, - model_removed_chars: 0, - user_added_chars: 0, - user_removed_chars: 0, - }, - }, + it('calculates cumulative stats across multiple turns', async () => { + const { getFileDiffFromResultDisplay, computeAddedAndRemovedLines } = + await import('@google/gemini-cli-core'); + vi.mocked(getFileDiffFromResultDisplay) + .mockReturnValueOnce({ + filePath: 'file1.ts', + fileName: 'file1.ts', + originalContent: '123', + newContent: '12345', + isNewFile: false, + diffStat: { + model_added_lines: 0, + model_removed_lines: 0, + model_added_chars: 0, + model_removed_chars: 0, + user_added_lines: 0, + user_removed_lines: 0, + user_added_chars: 0, + user_removed_chars: 0, }, - ] as unknown as ToolCallRecord[], - }; - - const userMsg2: MessageRecord = { - type: 'user', - content: 'next', - id: '3', - timestamp: '3', - }; - - const toolMsg2: MessageRecord = { - type: 'gemini', - id: '4', - timestamp: '4', - content: '', - toolCalls: [ - { - name: 'replace', - id: 'tool-call-2', - status: 'success', - timestamp: '4', - args: {}, - resultDisplay: { - fileName: 'file2.ts', - filePath: '/file2.ts', - fileDiff: 'diff2', - originalContent: 'old', - newContent: 'new', - isNewFile: false, - diffStat: { - model_added_lines: 3, - model_removed_lines: 1, - user_added_lines: 0, - user_removed_lines: 0, - model_added_chars: 0, - model_removed_chars: 0, - user_added_chars: 0, - user_removed_chars: 0, - }, - }, + fileDiff: 'diff1', + }) + .mockReturnValueOnce({ + filePath: 'file2.ts', + fileName: 'file2.ts', + originalContent: 'abc', + newContent: 'abcd', + isNewFile: true, + diffStat: { + model_added_lines: 0, + model_removed_lines: 0, + model_added_chars: 0, + model_removed_chars: 0, + user_added_lines: 0, + user_removed_lines: 0, + user_added_chars: 0, + user_removed_chars: 0, }, - ] as unknown as ToolCallRecord[], + fileDiff: 'diff2', + }); + + vi.mocked(computeAddedAndRemovedLines) + .mockReturnValueOnce({ addedLines: 5, removedLines: 3 }) + .mockReturnValueOnce({ addedLines: 4, removedLines: 0 }); + + const userMsg = { type: 'user' } as unknown as MessageRecord; + const conversation = { + messages: [ + userMsg, + { + type: 'gemini', + toolCalls: [ + { + resultDisplay: 'd1', + } as unknown as ToolCallRecord, + ], + } as unknown as MessageRecord, + { + type: 'user', + } as unknown as MessageRecord, + { + type: 'gemini', + toolCalls: [ + { + resultDisplay: 'd2', + } as unknown as ToolCallRecord, + ], + } as unknown as MessageRecord, + ], }; - mockConversation.messages = [userMsg1, toolMsg1, userMsg2, toolMsg2]; - - const stats = calculateRewindImpact(mockConversation, userMsg1); - - expect(stats).toEqual({ - addedLines: 8, // 5 + 3 - removedLines: 3, // 2 + 1 + const result = calculateRewindImpact( + conversation as unknown as ConversationRecord, + userMsg, + ); + expect(result).toEqual({ fileCount: 2, + addedLines: 9, // 5 + 4 + removedLines: 3, // 3 + 0 details: [ { fileName: 'file1.ts', diff: 'diff1' }, { fileName: 'file2.ts', diff: 'diff2' }, @@ -226,246 +211,264 @@ describe('rewindFileOps', () => { }); describe('revertFileChanges', () => { - const mockDiffStat = { - model_added_lines: 1, - model_removed_lines: 1, - user_added_lines: 0, - user_removed_lines: 0, - model_added_chars: 1, - model_removed_chars: 1, - user_added_chars: 0, - user_removed_chars: 0, - }; - it('does nothing if message not found', async () => { - mockConversation.messages = []; - await revertFileChanges(mockConversation, 'missing-id'); + await revertFileChanges( + { messages: [] } as unknown as ConversationRecord, + 'missing', + ); expect(fs.writeFile).not.toHaveBeenCalled(); }); it('reverts exact match', async () => { - const userMsg: MessageRecord = { + const { getFileDiffFromResultDisplay } = await import( + '@google/gemini-cli-core' + ); + vi.mocked(getFileDiffFromResultDisplay).mockReturnValue({ + filePath: '/abs/path/test.ts', + fileName: 'test.ts', + originalContent: 'ORIGINAL_CONTENT', + newContent: 'NEW_CONTENT', + isNewFile: false, + diffStat: { + model_added_lines: 0, + model_removed_lines: 0, + model_added_chars: 0, + model_removed_chars: 0, + user_added_lines: 0, + user_removed_lines: 0, + user_added_chars: 0, + user_removed_chars: 0, + }, + fileDiff: 'diff', + }); + + const userMsg = { type: 'user', - content: 'start', - id: '1', - timestamp: '1', - }; - const toolMsg: MessageRecord = { - type: 'gemini', - id: '2', - timestamp: '2', - content: '', - toolCalls: [ + id: 'target', + } as unknown as MessageRecord; + const conversation = { + messages: [ + userMsg, { - name: 'replace', - id: 'tool-call-1', - status: 'success', - timestamp: '2', - args: {}, - resultDisplay: { - fileName: 'file.txt', - filePath: path.resolve('/root/file.txt'), - originalContent: 'old', - newContent: 'new', - isNewFile: false, - diffStat: mockDiffStat, - }, - }, - ] as unknown as ToolCallRecord[], + type: 'gemini', + toolCalls: [{ resultDisplay: 'diff' } as unknown as ToolCallRecord], + } as unknown as MessageRecord, + ], }; - mockConversation.messages = [userMsg, toolMsg]; + vi.mocked(fs.readFile).mockResolvedValue('NEW_CONTENT'); - vi.mocked(fs.readFile).mockResolvedValue('new'); - - await revertFileChanges(mockConversation, '1'); + await revertFileChanges( + conversation as unknown as ConversationRecord, + 'target', + ); expect(fs.writeFile).toHaveBeenCalledWith( - path.resolve('/root/file.txt'), - 'old', + '/abs/path/test.ts', + 'ORIGINAL_CONTENT', ); }); it('deletes new file on revert', async () => { - const userMsg: MessageRecord = { + const { getFileDiffFromResultDisplay } = await import( + '@google/gemini-cli-core' + ); + vi.mocked(getFileDiffFromResultDisplay).mockReturnValue({ + filePath: '/abs/path/new.ts', + fileName: 'new.ts', + originalContent: '', + newContent: 'SOME_CONTENT', + isNewFile: true, + diffStat: { + model_added_lines: 0, + model_removed_lines: 0, + model_added_chars: 0, + model_removed_chars: 0, + user_added_lines: 0, + user_removed_lines: 0, + user_added_chars: 0, + user_removed_chars: 0, + }, + fileDiff: 'diff', + }); + + const userMsg = { type: 'user', - content: 'start', - id: '1', - timestamp: '1', - }; - const toolMsg: MessageRecord = { - type: 'gemini', - id: '2', - timestamp: '2', - content: '', - toolCalls: [ + id: 'target', + } as unknown as MessageRecord; + const conversation = { + messages: [ + userMsg, { - name: 'write_file', - id: 'tool-call-2', - status: 'success', - timestamp: '2', - args: {}, - resultDisplay: { - fileName: 'file.txt', - filePath: path.resolve('/root/file.txt'), - originalContent: null, - newContent: 'content', - isNewFile: true, - diffStat: mockDiffStat, - }, - }, - ] as unknown as ToolCallRecord[], + type: 'gemini', + toolCalls: [{ resultDisplay: 'diff' } as unknown as ToolCallRecord], + } as unknown as MessageRecord, + ], }; - mockConversation.messages = [userMsg, toolMsg]; + vi.mocked(fs.readFile).mockResolvedValue('SOME_CONTENT'); - vi.mocked(fs.readFile).mockResolvedValue('content'); + await revertFileChanges( + conversation as unknown as ConversationRecord, + 'target', + ); - await revertFileChanges(mockConversation, '1'); - - expect(fs.unlink).toHaveBeenCalledWith(path.resolve('/root/file.txt')); + expect(fs.unlink).toHaveBeenCalledWith('/abs/path/new.ts'); }); it('handles smart revert (patching) successfully', async () => { - const original = Array.from( - { length: 20 }, - (_, i) => `line${i + 1}`, - ).join('\n'); - // Agent changes line 2 - const agentModifiedLines = original.split('\n'); - agentModifiedLines[1] = 'line2-modified'; - const agentModified = agentModifiedLines.join('\n'); + const { getFileDiffFromResultDisplay } = await import( + '@google/gemini-cli-core' + ); + vi.mocked(getFileDiffFromResultDisplay).mockReturnValue({ + filePath: '/abs/path/test.ts', + fileName: 'test.ts', + originalContent: 'LINE1\nLINE2\nLINE3', + newContent: 'LINE1\nEDITED\nLINE3', + isNewFile: false, + diffStat: { + model_added_lines: 0, + model_removed_lines: 0, + model_added_chars: 0, + model_removed_chars: 0, + user_added_lines: 0, + user_removed_lines: 0, + user_added_chars: 0, + user_removed_chars: 0, + }, + fileDiff: 'diff', + }); - // User changes line 18 (far away from line 2) - const userModifiedLines = [...agentModifiedLines]; - userModifiedLines[17] = 'line18-modified'; - const userModified = userModifiedLines.join('\n'); - - const toolMsg: MessageRecord = { - type: 'gemini', - id: '2', - timestamp: '2', - content: '', - toolCalls: [ + const userMsg = { + type: 'user', + id: 'target', + } as unknown as MessageRecord; + const conversation = { + messages: [ + userMsg, { - name: 'replace', - id: 'tool-call-1', - status: 'success', - timestamp: '2', - args: {}, - resultDisplay: { - fileName: 'file.txt', - filePath: path.resolve('/root/file.txt'), - originalContent: original, - newContent: agentModified, - isNewFile: false, - diffStat: mockDiffStat, - }, - }, - ] as unknown as ToolCallRecord[], + type: 'gemini', + toolCalls: [{ resultDisplay: 'diff' } as unknown as ToolCallRecord], + } as unknown as MessageRecord, + ], }; - mockConversation.messages = [ - { type: 'user', content: 'start', id: '1', timestamp: '1' }, - toolMsg, - ]; - vi.mocked(fs.readFile).mockResolvedValue(userModified); + // Current content has FURTHER changes + vi.mocked(fs.readFile).mockResolvedValue('LINE1\nEDITED\nLINE3\nNEWLINE'); - await revertFileChanges(mockConversation, '1'); - - // Expect line 2 to be reverted to original, but line 18 to keep user modification - const expectedLines = original.split('\n'); - expectedLines[17] = 'line18-modified'; - const expectedContent = expectedLines.join('\n'); + await revertFileChanges( + conversation as unknown as ConversationRecord, + 'target', + ); + // Should have successfully patched it back to ORIGINAL state but kept the NEWLINE expect(fs.writeFile).toHaveBeenCalledWith( - path.resolve('/root/file.txt'), - expectedContent, + '/abs/path/test.ts', + 'LINE1\nLINE2\nLINE3\nNEWLINE', ); }); it('emits warning on smart revert failure', async () => { - const original = 'line1\nline2\nline3'; - const agentModified = 'line1\nline2-modified\nline3'; - // User modification conflicts with the agent's change. - const userModified = 'line1\nline2-usermodified\nline3'; + const { getFileDiffFromResultDisplay } = await import( + '@google/gemini-cli-core' + ); + vi.mocked(getFileDiffFromResultDisplay).mockReturnValue({ + filePath: '/abs/path/test.ts', + fileName: 'test.ts', + originalContent: 'OLD', + newContent: 'NEW', + isNewFile: false, + diffStat: { + model_added_lines: 0, + model_removed_lines: 0, + model_added_chars: 0, + model_removed_chars: 0, + user_added_lines: 0, + user_removed_lines: 0, + user_added_chars: 0, + user_removed_chars: 0, + }, + fileDiff: 'diff', + }); - const toolMsg: MessageRecord = { - type: 'gemini', - id: '2', - timestamp: '2', - content: '', - toolCalls: [ + const userMsg = { + type: 'user', + id: 'target', + } as unknown as MessageRecord; + const conversation = { + messages: [ + userMsg, { - name: 'replace', - id: 'tool-call-1', - status: 'success', - timestamp: '2', - args: {}, - resultDisplay: { - fileName: 'file.txt', - filePath: path.resolve('/root/file.txt'), - originalContent: original, - newContent: agentModified, - isNewFile: false, - diffStat: mockDiffStat, - }, - }, - ] as unknown as ToolCallRecord[], + type: 'gemini', + toolCalls: [{ resultDisplay: 'diff' } as unknown as ToolCallRecord], + } as unknown as MessageRecord, + ], }; - mockConversation.messages = [ - { type: 'user', content: 'start', id: '1', timestamp: '1' }, - toolMsg, - ]; - vi.mocked(fs.readFile).mockResolvedValue(userModified); + // Current content is completely unrelated - diff won't apply + vi.mocked(fs.readFile).mockResolvedValue('UNRELATED'); - await revertFileChanges(mockConversation, '1'); + await revertFileChanges( + conversation as unknown as ConversationRecord, + 'target', + ); expect(fs.writeFile).not.toHaveBeenCalled(); expect(coreEvents.emitFeedback).toHaveBeenCalledWith( 'warning', - expect.stringContaining('Smart revert for file.txt failed'), + expect.stringContaining('Smart revert for test.ts failed'), ); }); it('emits error if fs.readFile fails with a generic error', async () => { - const toolMsg: MessageRecord = { - type: 'gemini', - id: '2', - timestamp: '2', - content: '', - toolCalls: [ + const { getFileDiffFromResultDisplay } = await import( + '@google/gemini-cli-core' + ); + vi.mocked(getFileDiffFromResultDisplay).mockReturnValue({ + filePath: '/abs/path/test.ts', + fileName: 'test.ts', + originalContent: 'OLD', + newContent: 'NEW', + isNewFile: false, + diffStat: { + model_added_lines: 0, + model_removed_lines: 0, + model_added_chars: 0, + model_removed_chars: 0, + user_added_lines: 0, + user_removed_lines: 0, + user_added_chars: 0, + user_removed_chars: 0, + }, + fileDiff: 'diff', + }); + + const userMsg = { + type: 'user', + id: 'target', + } as unknown as MessageRecord; + const conversation = { + messages: [ + userMsg, { - name: 'replace', - id: 'tool-call-1', - status: 'success', - timestamp: '2', - args: {}, - resultDisplay: { - fileName: 'file.txt', - filePath: path.resolve('/root/file.txt'), - originalContent: 'old', - newContent: 'new', - isNewFile: false, - diffStat: mockDiffStat, - }, - }, - ] as unknown as ToolCallRecord[], + type: 'gemini', + toolCalls: [{ resultDisplay: 'diff' } as unknown as ToolCallRecord], + } as unknown as MessageRecord, + ], }; - mockConversation.messages = [ - { type: 'user', content: 'start', id: '1', timestamp: '1' }, - toolMsg, - ]; - // Simulate a generic file read error - vi.mocked(fs.readFile).mockRejectedValue(new Error('Permission denied')); + vi.mocked(fs.readFile).mockRejectedValue(new Error('disk failure')); + + await revertFileChanges( + conversation as unknown as ConversationRecord, + 'target', + ); - await revertFileChanges(mockConversation, '1'); - expect(fs.writeFile).not.toHaveBeenCalled(); expect(coreEvents.emitFeedback).toHaveBeenCalledWith( 'error', - 'Error reading file.txt during revert: Permission denied', + expect.stringContaining( + 'Error reading test.ts during revert: disk failure', + ), expect.any(Error), ); }); diff --git a/packages/core/src/skills/skillLoader.ts b/packages/core/src/skills/skillLoader.ts index cbd1f238bd..be962eaa67 100644 --- a/packages/core/src/skills/skillLoader.ts +++ b/packages/core/src/skills/skillLoader.ts @@ -25,6 +25,8 @@ export interface SkillDefinition { body: string; /** Whether the skill is currently disabled. */ disabled?: boolean; + /** Whether the skill is a built-in skill. */ + isBuiltin?: boolean; } const FRONTMATTER_REGEX = /^---\r?\n([\s\S]*?)\r?\n---\r?\n([\s\S]*)/; diff --git a/packages/core/src/skills/skillManager.test.ts b/packages/core/src/skills/skillManager.test.ts index 4bd200e4d7..293115267c 100644 --- a/packages/core/src/skills/skillManager.test.ts +++ b/packages/core/src/skills/skillManager.test.ts @@ -163,4 +163,45 @@ description: desc1 expect(service.getAllSkills()).toHaveLength(1); expect(service.getAllSkills()[0].disabled).toBe(true); }); + + it('should filter built-in skills in getDisplayableSkills', async () => { + const service = new SkillManager(); + + // @ts-expect-error accessing private property for testing + service.skills = [ + { + name: 'regular-skill', + description: 'regular', + location: 'loc1', + body: 'body', + isBuiltin: false, + }, + { + name: 'builtin-skill', + description: 'builtin', + location: 'loc2', + body: 'body', + isBuiltin: true, + }, + { + name: 'disabled-builtin', + description: 'disabled builtin', + location: 'loc3', + body: 'body', + isBuiltin: true, + disabled: true, + }, + ]; + + const displayable = service.getDisplayableSkills(); + expect(displayable).toHaveLength(1); + expect(displayable[0].name).toBe('regular-skill'); + + const all = service.getAllSkills(); + expect(all).toHaveLength(3); + + const enabled = service.getSkills(); + expect(enabled).toHaveLength(2); + expect(enabled.map((s) => s.name)).toContain('builtin-skill'); + }); }); diff --git a/packages/core/src/skills/skillManager.ts b/packages/core/src/skills/skillManager.ts index 22e0858cc8..0279df5a65 100644 --- a/packages/core/src/skills/skillManager.ts +++ b/packages/core/src/skills/skillManager.ts @@ -31,24 +31,36 @@ export class SkillManager { ): Promise { this.clearSkills(); - // 1. Extension skills (lowest precedence) + // 1. Built-in skills (lowest precedence) + await this.discoverBuiltinSkills(); + + // 2. Extension skills for (const extension of extensions) { if (extension.isActive && extension.skills) { this.addSkillsWithPrecedence(extension.skills); } } - // 2. User skills + // 3. User skills const userSkills = await loadSkillsFromDir(Storage.getUserSkillsDir()); this.addSkillsWithPrecedence(userSkills); - // 3. Project skills (highest precedence) + // 4. Project skills (highest precedence) const projectSkills = await loadSkillsFromDir( storage.getProjectSkillsDir(), ); this.addSkillsWithPrecedence(projectSkills); } + /** + * Discovers built-in skills. + */ + private async discoverBuiltinSkills(): Promise { + // Built-in skills can be added here. + // For now, this is a placeholder for where built-in skills will be loaded from. + // They could be loaded from a specific directory within the package. + } + private addSkillsWithPrecedence(newSkills: SkillDefinition[]): void { const skillMap = new Map(); for (const skill of [...this.skills, ...newSkills]) { @@ -64,6 +76,14 @@ export class SkillManager { return this.skills.filter((s) => !s.disabled); } + /** + * Returns the list of enabled discovered skills that should be displayed in the UI. + * This excludes built-in skills. + */ + getDisplayableSkills(): SkillDefinition[] { + return this.skills.filter((s) => !s.disabled && !s.isBuiltin); + } + /** * Returns all discovered skills, including disabled ones. */ @@ -82,8 +102,11 @@ export class SkillManager { * Sets the list of disabled skill names. */ setDisabledSkills(disabledNames: string[]): void { + const lowercaseDisabledNames = disabledNames.map((n) => n.toLowerCase()); for (const skill of this.skills) { - skill.disabled = disabledNames.includes(skill.name); + skill.disabled = lowercaseDisabledNames.includes( + skill.name.toLowerCase(), + ); } } @@ -91,7 +114,10 @@ export class SkillManager { * Reads the full content (metadata + body) of a skill by name. */ getSkill(name: string): SkillDefinition | null { - return this.skills.find((s) => s.name === name) ?? null; + const lowercaseName = name.toLowerCase(); + return ( + this.skills.find((s) => s.name.toLowerCase() === lowercaseName) ?? null + ); } /** diff --git a/packages/core/src/tools/activate-skill.test.ts b/packages/core/src/tools/activate-skill.test.ts index b997a67bdc..553a34dd43 100644 --- a/packages/core/src/tools/activate-skill.test.ts +++ b/packages/core/src/tools/activate-skill.test.ts @@ -76,6 +76,34 @@ describe('ActivateSkillTool', () => { expect(details.prompt).toContain('Mock folder structure'); }); + it('should skip confirmation for built-in skills', async () => { + const builtinSkill = { + name: 'builtin-skill', + description: 'A built-in skill', + location: '/path/to/builtin/SKILL.md', + isBuiltin: true, + body: 'Built-in instructions', + }; + vi.mocked(mockConfig.getSkillManager().getSkill).mockReturnValue( + builtinSkill, + ); + vi.mocked(mockConfig.getSkillManager().getSkills).mockReturnValue([ + builtinSkill, + ]); + + const params = { name: 'builtin-skill' }; + const toolWithBuiltin = new ActivateSkillTool(mockConfig, mockMessageBus); + const invocation = toolWithBuiltin.build(params); + + const details = await ( + invocation as unknown as { + getConfirmationDetails: (signal: AbortSignal) => Promise; + } + ).getConfirmationDetails(new AbortController().signal); + + expect(details).toBe(false); + }); + it('should activate a valid skill and return its content in XML tags', async () => { const params = { name: 'test-skill' }; const invocation = tool.build(params); diff --git a/packages/core/src/tools/activate-skill.ts b/packages/core/src/tools/activate-skill.ts index e6f1a9e6b7..381ad66976 100644 --- a/packages/core/src/tools/activate-skill.ts +++ b/packages/core/src/tools/activate-skill.ts @@ -80,6 +80,10 @@ class ActivateSkillToolInvocation extends BaseToolInvocation< return false; } + if (skill.isBuiltin) { + return false; + } + const folderStructure = await this.getOrFetchFolderStructure( skill.location, );