From bd590bbde66f9aa7695940635b816d17f80a05ed Mon Sep 17 00:00:00 2001 From: "N. Taylor Mullen" Date: Sat, 14 Mar 2026 16:33:14 -0700 Subject: [PATCH] fix(cli): improve command conflict handling for skills (#21942) --- packages/cli/src/config/extension-manager.ts | 14 ++++++----- .../src/services/SkillCommandLoader.test.ts | 12 +++++++++ .../cli/src/services/SkillCommandLoader.ts | 1 + .../SlashCommandConflictHandler.test.ts | 19 ++++++++++++++ .../services/SlashCommandConflictHandler.ts | 4 +++ .../src/services/SlashCommandResolver.test.ts | 25 +++++++++++++++++++ .../cli/src/services/SlashCommandResolver.ts | 2 +- .../cli/src/ui/hooks/slashCommandProcessor.ts | 2 +- packages/core/src/code_assist/oauth2.test.ts | 3 ++- packages/core/src/skills/skillLoader.ts | 2 ++ .../core/src/telemetry/memory-monitor.test.ts | 1 + 11 files changed, 76 insertions(+), 9 deletions(-) diff --git a/packages/cli/src/config/extension-manager.ts b/packages/cli/src/config/extension-manager.ts index 68617bcbcd..974cb1b83e 100644 --- a/packages/cli/src/config/extension-manager.ts +++ b/packages/cli/src/config/extension-manager.ts @@ -898,9 +898,10 @@ Would you like to attempt to install via "git clone" instead?`, let skills = await loadSkillsFromDir( path.join(effectiveExtensionPath, 'skills'), ); - skills = skills.map((skill) => - recursivelyHydrateStrings(skill, hydrationContext), - ); + skills = skills.map((skill) => ({ + ...recursivelyHydrateStrings(skill, hydrationContext), + extensionName: config.name, + })); let rules: PolicyRule[] | undefined; let checkers: SafetyCheckerRule[] | undefined; @@ -923,9 +924,10 @@ Would you like to attempt to install via "git clone" instead?`, const agentLoadResult = await loadAgentsFromDirectory( path.join(effectiveExtensionPath, 'agents'), ); - agentLoadResult.agents = agentLoadResult.agents.map((agent) => - recursivelyHydrateStrings(agent, hydrationContext), - ); + agentLoadResult.agents = agentLoadResult.agents.map((agent) => ({ + ...recursivelyHydrateStrings(agent, hydrationContext), + extensionName: config.name, + })); // Log errors but don't fail the entire extension load for (const error of agentLoadResult.errors) { diff --git a/packages/cli/src/services/SkillCommandLoader.test.ts b/packages/cli/src/services/SkillCommandLoader.test.ts index 15a2ebec18..51cc098536 100644 --- a/packages/cli/src/services/SkillCommandLoader.test.ts +++ b/packages/cli/src/services/SkillCommandLoader.test.ts @@ -122,4 +122,16 @@ describe('SkillCommandLoader', () => { const actionResult = (await commands[0].action!({} as any, '')) as any; expect(actionResult.toolArgs).toEqual({ name: 'my awesome skill' }); }); + + it('should propagate extensionName to the generated slash command', async () => { + const mockSkills = [ + { name: 'skill1', description: 'desc', extensionName: 'ext1' }, + ]; + mockSkillManager.getDisplayableSkills.mockReturnValue(mockSkills); + + const loader = new SkillCommandLoader(mockConfig); + const commands = await loader.loadCommands(new AbortController().signal); + + expect(commands[0].extensionName).toBe('ext1'); + }); }); diff --git a/packages/cli/src/services/SkillCommandLoader.ts b/packages/cli/src/services/SkillCommandLoader.ts index 85f1884299..e264da2e31 100644 --- a/packages/cli/src/services/SkillCommandLoader.ts +++ b/packages/cli/src/services/SkillCommandLoader.ts @@ -41,6 +41,7 @@ export class SkillCommandLoader implements ICommandLoader { description: skill.description || `Activate the ${skill.name} skill`, kind: CommandKind.SKILL, autoExecute: true, + extensionName: skill.extensionName, action: async (_context, args) => ({ type: 'tool', toolName: ACTIVATE_SKILL_TOOL_NAME, diff --git a/packages/cli/src/services/SlashCommandConflictHandler.test.ts b/packages/cli/src/services/SlashCommandConflictHandler.test.ts index a828923fe5..5527188a04 100644 --- a/packages/cli/src/services/SlashCommandConflictHandler.test.ts +++ b/packages/cli/src/services/SlashCommandConflictHandler.test.ts @@ -172,4 +172,23 @@ describe('SlashCommandConflictHandler', () => { vi.advanceTimersByTime(600); expect(coreEvents.emitFeedback).not.toHaveBeenCalled(); }); + + it('should display a descriptive message for a skill conflict', () => { + simulateEvent([ + { + name: 'chat', + renamedTo: 'google-workspace.chat', + loserExtensionName: 'google-workspace', + loserKind: CommandKind.SKILL, + winnerKind: CommandKind.BUILT_IN, + }, + ]); + + vi.advanceTimersByTime(600); + + expect(coreEvents.emitFeedback).toHaveBeenCalledWith( + 'info', + "Extension 'google-workspace' skill '/chat' was renamed to '/google-workspace.chat' because it conflicts with built-in command.", + ); + }); }); diff --git a/packages/cli/src/services/SlashCommandConflictHandler.ts b/packages/cli/src/services/SlashCommandConflictHandler.ts index b51617840e..7da4e53842 100644 --- a/packages/cli/src/services/SlashCommandConflictHandler.ts +++ b/packages/cli/src/services/SlashCommandConflictHandler.ts @@ -154,6 +154,10 @@ export class SlashCommandConflictHandler { return extensionName ? `extension '${extensionName}' command` : 'extension command'; + case CommandKind.SKILL: + return extensionName + ? `extension '${extensionName}' skill` + : 'skill command'; case CommandKind.MCP_PROMPT: return mcpServerName ? `MCP server '${mcpServerName}' command` diff --git a/packages/cli/src/services/SlashCommandResolver.test.ts b/packages/cli/src/services/SlashCommandResolver.test.ts index e703028b3d..43d1c310a8 100644 --- a/packages/cli/src/services/SlashCommandResolver.test.ts +++ b/packages/cli/src/services/SlashCommandResolver.test.ts @@ -173,5 +173,30 @@ describe('SlashCommandResolver', () => { expect(finalCommands.find((c) => c.name === 'gcp.deploy1')).toBeDefined(); }); + + it('should prefix skills with extension name when they conflict with built-in', () => { + const builtin = createMockCommand('chat', CommandKind.BUILT_IN); + const skill = { + ...createMockCommand('chat', CommandKind.SKILL), + extensionName: 'google-workspace', + }; + + const { finalCommands } = SlashCommandResolver.resolve([builtin, skill]); + + const names = finalCommands.map((c) => c.name); + expect(names).toContain('chat'); + expect(names).toContain('google-workspace.chat'); + }); + + it('should NOT prefix skills with "skill" when extension name is missing', () => { + const builtin = createMockCommand('chat', CommandKind.BUILT_IN); + const skill = createMockCommand('chat', CommandKind.SKILL); + + const { finalCommands } = SlashCommandResolver.resolve([builtin, skill]); + + const names = finalCommands.map((c) => c.name); + expect(names).toContain('chat'); + expect(names).toContain('chat1'); + }); }); }); diff --git a/packages/cli/src/services/SlashCommandResolver.ts b/packages/cli/src/services/SlashCommandResolver.ts index d4e7efc7bb..4947e6545a 100644 --- a/packages/cli/src/services/SlashCommandResolver.ts +++ b/packages/cli/src/services/SlashCommandResolver.ts @@ -174,6 +174,7 @@ export class SlashCommandResolver { private static getPrefix(cmd: SlashCommand): string | undefined { switch (cmd.kind) { case CommandKind.EXTENSION_FILE: + case CommandKind.SKILL: return cmd.extensionName; case CommandKind.MCP_PROMPT: return cmd.mcpServerName; @@ -185,7 +186,6 @@ export class SlashCommandResolver { return undefined; } } - /** * Logs a conflict event. */ diff --git a/packages/cli/src/ui/hooks/slashCommandProcessor.ts b/packages/cli/src/ui/hooks/slashCommandProcessor.ts index 6f3ecd7b96..d070840f2d 100644 --- a/packages/cli/src/ui/hooks/slashCommandProcessor.ts +++ b/packages/cli/src/ui/hooks/slashCommandProcessor.ts @@ -325,9 +325,9 @@ export const useSlashCommandProcessor = ( (async () => { const commandService = await CommandService.create( [ + new BuiltinCommandLoader(config), new SkillCommandLoader(config), new McpPromptLoader(config), - new BuiltinCommandLoader(config), new FileCommandLoader(config), ], controller.signal, diff --git a/packages/core/src/code_assist/oauth2.test.ts b/packages/core/src/code_assist/oauth2.test.ts index 2405e3307c..afe35ce665 100644 --- a/packages/core/src/code_assist/oauth2.test.ts +++ b/packages/core/src/code_assist/oauth2.test.ts @@ -480,6 +480,7 @@ describe('oauth2', () => { expect(fs.existsSync(googleAccountPath)).toBe(true); if (fs.existsSync(googleAccountPath)) { const cachedGoogleAccount = fs.readFileSync(googleAccountPath, 'utf-8'); + expect(JSON.parse(cachedGoogleAccount)).toEqual({ active: 'test-user-code-account@gmail.com', old: [], @@ -1349,7 +1350,7 @@ describe('oauth2', () => { let dataHandler: ((data: Buffer) => void) | undefined; await vi.waitFor(() => { const dataCall = stdinOnSpy.mock.calls.find( - (call: [string, ...unknown[]]) => call[0] === 'data', + (call: [string | symbol, ...unknown[]]) => call[0] === 'data', ); dataHandler = dataCall?.[1] as ((data: Buffer) => void) | undefined; if (!dataHandler) throw new Error('stdin handler not registered yet'); diff --git a/packages/core/src/skills/skillLoader.ts b/packages/core/src/skills/skillLoader.ts index e746caa179..7f6d3c11d0 100644 --- a/packages/core/src/skills/skillLoader.ts +++ b/packages/core/src/skills/skillLoader.ts @@ -27,6 +27,8 @@ export interface SkillDefinition { disabled?: boolean; /** Whether the skill is a built-in skill. */ isBuiltin?: boolean; + /** The name of the extension that provided this skill, if any. */ + extensionName?: string; } export const FRONTMATTER_REGEX = diff --git a/packages/core/src/telemetry/memory-monitor.test.ts b/packages/core/src/telemetry/memory-monitor.test.ts index fce8119753..8ad0d45595 100644 --- a/packages/core/src/telemetry/memory-monitor.test.ts +++ b/packages/core/src/telemetry/memory-monitor.test.ts @@ -89,6 +89,7 @@ const mockHeapStatistics = { total_global_handles_size: 8192, used_global_handles_size: 4096, external_memory: 2097152, + total_allocated_bytes: 31457280, }; const mockHeapSpaceStatistics = [