diff --git a/packages/cli/src/services/CommandService.test.ts b/packages/cli/src/services/CommandService.test.ts index ea906a3da6..eae7ec7c40 100644 --- a/packages/cli/src/services/CommandService.test.ts +++ b/packages/cli/src/services/CommandService.test.ts @@ -17,21 +17,9 @@ const createMockCommand = (name: string, kind: CommandKind): SlashCommand => ({ action: vi.fn(), }); -const mockCommandA = createMockCommand('command-a', CommandKind.BUILT_IN); -const mockCommandB = createMockCommand('command-b', CommandKind.BUILT_IN); -const mockCommandC = createMockCommand('command-c', CommandKind.FILE); -const mockCommandB_Override = createMockCommand('command-b', CommandKind.FILE); - class MockCommandLoader implements ICommandLoader { - private commandsToLoad: SlashCommand[]; - - constructor(commandsToLoad: SlashCommand[]) { - this.commandsToLoad = commandsToLoad; - } - - loadCommands = vi.fn( - async (): Promise => Promise.resolve(this.commandsToLoad), - ); + constructor(private readonly commands: SlashCommand[]) {} + loadCommands = vi.fn(async () => Promise.resolve(this.commands)); } describe('CommandService', () => { @@ -43,424 +31,74 @@ describe('CommandService', () => { vi.restoreAllMocks(); }); - it('should load commands from a single loader', async () => { - const mockLoader = new MockCommandLoader([mockCommandA, mockCommandB]); - const service = await CommandService.create( - [mockLoader], - new AbortController().signal, - ); + describe('basic loading', () => { + it('should aggregate commands from multiple successful loaders', async () => { + const cmdA = createMockCommand('a', CommandKind.BUILT_IN); + const cmdB = createMockCommand('b', CommandKind.USER_FILE); + const service = await CommandService.create( + [new MockCommandLoader([cmdA]), new MockCommandLoader([cmdB])], + new AbortController().signal, + ); - const commands = service.getCommands(); + expect(service.getCommands()).toHaveLength(2); + expect(service.getCommands()).toEqual( + expect.arrayContaining([cmdA, cmdB]), + ); + }); - expect(mockLoader.loadCommands).toHaveBeenCalledTimes(1); - expect(commands).toHaveLength(2); - expect(commands).toEqual( - expect.arrayContaining([mockCommandA, mockCommandB]), - ); - }); + it('should handle empty loaders and failed loaders gracefully', async () => { + const cmdA = createMockCommand('a', CommandKind.BUILT_IN); + const failingLoader = new MockCommandLoader([]); + vi.spyOn(failingLoader, 'loadCommands').mockRejectedValue( + new Error('fail'), + ); - it('should aggregate commands from multiple loaders', async () => { - const loader1 = new MockCommandLoader([mockCommandA]); - const loader2 = new MockCommandLoader([mockCommandC]); - const service = await CommandService.create( - [loader1, loader2], - new AbortController().signal, - ); + const service = await CommandService.create( + [ + new MockCommandLoader([cmdA]), + new MockCommandLoader([]), + failingLoader, + ], + new AbortController().signal, + ); - const commands = service.getCommands(); + expect(service.getCommands()).toHaveLength(1); + expect(service.getCommands()[0].name).toBe('a'); + expect(debugLogger.debug).toHaveBeenCalledWith( + 'A command loader failed:', + expect.any(Error), + ); + }); - expect(loader1.loadCommands).toHaveBeenCalledTimes(1); - expect(loader2.loadCommands).toHaveBeenCalledTimes(1); - expect(commands).toHaveLength(2); - expect(commands).toEqual( - expect.arrayContaining([mockCommandA, mockCommandC]), - ); - }); + it('should return a readonly array of commands', async () => { + const service = await CommandService.create( + [new MockCommandLoader([createMockCommand('a', CommandKind.BUILT_IN)])], + new AbortController().signal, + ); + expect(() => (service.getCommands() as unknown[]).push({})).toThrow(); + }); - it('should override commands from earlier loaders with those from later loaders', async () => { - const loader1 = new MockCommandLoader([mockCommandA, mockCommandB]); - const loader2 = new MockCommandLoader([ - mockCommandB_Override, - mockCommandC, - ]); - const service = await CommandService.create( - [loader1, loader2], - new AbortController().signal, - ); - - const commands = service.getCommands(); - - expect(commands).toHaveLength(3); // Should be A, C, and the overridden B. - - // The final list should contain the override from the *last* loader. - const commandB = commands.find((cmd) => cmd.name === 'command-b'); - expect(commandB).toBeDefined(); - expect(commandB?.kind).toBe(CommandKind.FILE); // Verify it's the overridden version. - expect(commandB).toEqual(mockCommandB_Override); - - // Ensure the other commands are still present. - expect(commands).toEqual( - expect.arrayContaining([ - mockCommandA, - mockCommandC, - mockCommandB_Override, - ]), - ); - }); - - it('should handle loaders that return an empty array of commands gracefully', async () => { - const loader1 = new MockCommandLoader([mockCommandA]); - const emptyLoader = new MockCommandLoader([]); - const loader3 = new MockCommandLoader([mockCommandB]); - const service = await CommandService.create( - [loader1, emptyLoader, loader3], - new AbortController().signal, - ); - - const commands = service.getCommands(); - - expect(emptyLoader.loadCommands).toHaveBeenCalledTimes(1); - expect(commands).toHaveLength(2); - expect(commands).toEqual( - expect.arrayContaining([mockCommandA, mockCommandB]), - ); - }); - - it('should load commands from successful loaders even if one fails', async () => { - const successfulLoader = new MockCommandLoader([mockCommandA]); - const failingLoader = new MockCommandLoader([]); - const error = new Error('Loader failed'); - vi.spyOn(failingLoader, 'loadCommands').mockRejectedValue(error); - - const service = await CommandService.create( - [successfulLoader, failingLoader], - new AbortController().signal, - ); - - const commands = service.getCommands(); - expect(commands).toHaveLength(1); - expect(commands).toEqual([mockCommandA]); - expect(debugLogger.debug).toHaveBeenCalledWith( - 'A command loader failed:', - error, - ); - }); - - it('getCommands should return a readonly array that cannot be mutated', async () => { - const service = await CommandService.create( - [new MockCommandLoader([mockCommandA])], - new AbortController().signal, - ); - - const commands = service.getCommands(); - - // Expect it to throw a TypeError at runtime because the array is frozen. - expect(() => { - // @ts-expect-error - Testing immutability is intentional here. - commands.push(mockCommandB); - }).toThrow(); - - // Verify the original array was not mutated. - expect(service.getCommands()).toHaveLength(1); - }); - - it('should pass the abort signal to all loaders', async () => { - const controller = new AbortController(); - const signal = controller.signal; - - const loader1 = new MockCommandLoader([mockCommandA]); - const loader2 = new MockCommandLoader([mockCommandB]); - - await CommandService.create([loader1, loader2], signal); - - expect(loader1.loadCommands).toHaveBeenCalledTimes(1); - expect(loader1.loadCommands).toHaveBeenCalledWith(signal); - expect(loader2.loadCommands).toHaveBeenCalledTimes(1); - expect(loader2.loadCommands).toHaveBeenCalledWith(signal); - }); - - it('should rename extension commands when they conflict', async () => { - const builtinCommand = createMockCommand('deploy', CommandKind.BUILT_IN); - const userCommand = createMockCommand('sync', CommandKind.FILE); - const extensionCommand1 = { - ...createMockCommand('deploy', CommandKind.FILE), - extensionName: 'firebase', - description: '[firebase] Deploy to Firebase', - }; - const extensionCommand2 = { - ...createMockCommand('sync', CommandKind.FILE), - extensionName: 'git-helper', - description: '[git-helper] Sync with remote', - }; - - const mockLoader1 = new MockCommandLoader([builtinCommand]); - const mockLoader2 = new MockCommandLoader([ - userCommand, - extensionCommand1, - extensionCommand2, - ]); - - const service = await CommandService.create( - [mockLoader1, mockLoader2], - new AbortController().signal, - ); - - const commands = service.getCommands(); - expect(commands).toHaveLength(4); - - // Built-in command keeps original name - const deployBuiltin = commands.find( - (cmd) => cmd.name === 'deploy' && !cmd.extensionName, - ); - expect(deployBuiltin).toBeDefined(); - expect(deployBuiltin?.kind).toBe(CommandKind.BUILT_IN); - - // Extension command conflicting with built-in gets renamed - const deployExtension = commands.find( - (cmd) => cmd.name === 'firebase.deploy', - ); - expect(deployExtension).toBeDefined(); - expect(deployExtension?.extensionName).toBe('firebase'); - - // User command keeps original name - const syncUser = commands.find( - (cmd) => cmd.name === 'sync' && !cmd.extensionName, - ); - expect(syncUser).toBeDefined(); - expect(syncUser?.kind).toBe(CommandKind.FILE); - - // Extension command conflicting with user command gets renamed - const syncExtension = commands.find( - (cmd) => cmd.name === 'git-helper.sync', - ); - expect(syncExtension).toBeDefined(); - expect(syncExtension?.extensionName).toBe('git-helper'); - }); - - it('should handle user/project command override correctly', async () => { - const builtinCommand = createMockCommand('help', CommandKind.BUILT_IN); - const userCommand = createMockCommand('help', CommandKind.FILE); - const projectCommand = createMockCommand('deploy', CommandKind.FILE); - const userDeployCommand = createMockCommand('deploy', CommandKind.FILE); - - const mockLoader1 = new MockCommandLoader([builtinCommand]); - const mockLoader2 = new MockCommandLoader([ - userCommand, - userDeployCommand, - projectCommand, - ]); - - const service = await CommandService.create( - [mockLoader1, mockLoader2], - new AbortController().signal, - ); - - const commands = service.getCommands(); - expect(commands).toHaveLength(2); - - // User command overrides built-in - const helpCommand = commands.find((cmd) => cmd.name === 'help'); - expect(helpCommand).toBeDefined(); - expect(helpCommand?.kind).toBe(CommandKind.FILE); - - // Project command overrides user command (last wins) - const deployCommand = commands.find((cmd) => cmd.name === 'deploy'); - expect(deployCommand).toBeDefined(); - expect(deployCommand?.kind).toBe(CommandKind.FILE); - }); - - it('should handle secondary conflicts when renaming extension commands', async () => { - // User has both /deploy and /gcp.deploy commands - const userCommand1 = createMockCommand('deploy', CommandKind.FILE); - const userCommand2 = createMockCommand('gcp.deploy', CommandKind.FILE); - - // Extension also has a deploy command that will conflict with user's /deploy - const extensionCommand = { - ...createMockCommand('deploy', CommandKind.FILE), - extensionName: 'gcp', - description: '[gcp] Deploy to Google Cloud', - }; - - const mockLoader = new MockCommandLoader([ - userCommand1, - userCommand2, - extensionCommand, - ]); - - const service = await CommandService.create( - [mockLoader], - new AbortController().signal, - ); - - const commands = service.getCommands(); - expect(commands).toHaveLength(3); - - // Original user command keeps its name - const deployUser = commands.find( - (cmd) => cmd.name === 'deploy' && !cmd.extensionName, - ); - expect(deployUser).toBeDefined(); - - // User's dot notation command keeps its name - const gcpDeployUser = commands.find( - (cmd) => cmd.name === 'gcp.deploy' && !cmd.extensionName, - ); - expect(gcpDeployUser).toBeDefined(); - - // Extension command gets renamed with suffix due to secondary conflict - const deployExtension = commands.find( - (cmd) => cmd.name === 'gcp.deploy1' && cmd.extensionName === 'gcp', - ); - expect(deployExtension).toBeDefined(); - expect(deployExtension?.description).toBe('[gcp] Deploy to Google Cloud'); - }); - - it('should handle multiple secondary conflicts with incrementing suffixes', async () => { - // User has /deploy, /gcp.deploy, and /gcp.deploy1 - const userCommand1 = createMockCommand('deploy', CommandKind.FILE); - const userCommand2 = createMockCommand('gcp.deploy', CommandKind.FILE); - const userCommand3 = createMockCommand('gcp.deploy1', CommandKind.FILE); - - // Extension has a deploy command - const extensionCommand = { - ...createMockCommand('deploy', CommandKind.FILE), - extensionName: 'gcp', - description: '[gcp] Deploy to Google Cloud', - }; - - const mockLoader = new MockCommandLoader([ - userCommand1, - userCommand2, - userCommand3, - extensionCommand, - ]); - - const service = await CommandService.create( - [mockLoader], - new AbortController().signal, - ); - - const commands = service.getCommands(); - expect(commands).toHaveLength(4); - - // Extension command gets renamed with suffix 2 due to multiple conflicts - const deployExtension = commands.find( - (cmd) => cmd.name === 'gcp.deploy2' && cmd.extensionName === 'gcp', - ); - expect(deployExtension).toBeDefined(); - expect(deployExtension?.description).toBe('[gcp] Deploy to Google Cloud'); - }); - - it('should report conflicts via getConflicts', async () => { - const builtinCommand = createMockCommand('deploy', CommandKind.BUILT_IN); - const extensionCommand = { - ...createMockCommand('deploy', CommandKind.FILE), - extensionName: 'firebase', - }; - - const mockLoader = new MockCommandLoader([ - builtinCommand, - extensionCommand, - ]); - - const service = await CommandService.create( - [mockLoader], - new AbortController().signal, - ); - - const conflicts = service.getConflicts(); - expect(conflicts).toHaveLength(1); - - expect(conflicts[0]).toMatchObject({ - name: 'deploy', - winner: builtinCommand, - losers: [ - { - renamedTo: 'firebase.deploy', - command: expect.objectContaining({ - name: 'deploy', - extensionName: 'firebase', - }), - }, - ], + it('should pass the abort signal to all loaders', async () => { + const controller = new AbortController(); + const loader = new MockCommandLoader([]); + await CommandService.create([loader], controller.signal); + expect(loader.loadCommands).toHaveBeenCalledWith(controller.signal); }); }); - it('should report extension vs extension conflicts correctly', async () => { - // Both extensions try to register 'deploy' - const extension1Command = { - ...createMockCommand('deploy', CommandKind.FILE), - extensionName: 'firebase', - }; - const extension2Command = { - ...createMockCommand('deploy', CommandKind.FILE), - extensionName: 'aws', - }; + describe('conflict delegation', () => { + it('should delegate conflict resolution to SlashCommandResolver', async () => { + const builtin = createMockCommand('help', CommandKind.BUILT_IN); + const user = createMockCommand('help', CommandKind.USER_FILE); - const mockLoader = new MockCommandLoader([ - extension1Command, - extension2Command, - ]); + const service = await CommandService.create( + [new MockCommandLoader([builtin, user])], + new AbortController().signal, + ); - const service = await CommandService.create( - [mockLoader], - new AbortController().signal, - ); - - const conflicts = service.getConflicts(); - expect(conflicts).toHaveLength(1); - - expect(conflicts[0]).toMatchObject({ - name: 'deploy', - winner: expect.objectContaining({ - name: 'deploy', - extensionName: 'firebase', - }), - losers: [ - { - renamedTo: 'aws.deploy', // ext2 is 'aws' and it lost because it was second in the list - command: expect.objectContaining({ - name: 'deploy', - extensionName: 'aws', - }), - }, - ], + expect(service.getCommands().map((c) => c.name)).toContain('help'); + expect(service.getCommands().map((c) => c.name)).toContain('user.help'); + expect(service.getConflicts()).toHaveLength(1); }); }); - - it('should report multiple conflicts for the same command name', async () => { - const builtinCommand = createMockCommand('deploy', CommandKind.BUILT_IN); - const ext1 = { - ...createMockCommand('deploy', CommandKind.FILE), - extensionName: 'ext1', - }; - const ext2 = { - ...createMockCommand('deploy', CommandKind.FILE), - extensionName: 'ext2', - }; - - const mockLoader = new MockCommandLoader([builtinCommand, ext1, ext2]); - - const service = await CommandService.create( - [mockLoader], - new AbortController().signal, - ); - - const conflicts = service.getConflicts(); - expect(conflicts).toHaveLength(1); - expect(conflicts[0].name).toBe('deploy'); - expect(conflicts[0].losers).toHaveLength(2); - expect(conflicts[0].losers).toEqual( - expect.arrayContaining([ - expect.objectContaining({ - renamedTo: 'ext1.deploy', - command: expect.objectContaining({ extensionName: 'ext1' }), - }), - expect.objectContaining({ - renamedTo: 'ext2.deploy', - command: expect.objectContaining({ extensionName: 'ext2' }), - }), - ]), - ); - }); }); diff --git a/packages/cli/src/services/CommandService.ts b/packages/cli/src/services/CommandService.ts index bd42226a32..61f9457619 100644 --- a/packages/cli/src/services/CommandService.ts +++ b/packages/cli/src/services/CommandService.ts @@ -6,16 +6,8 @@ import { debugLogger, coreEvents } from '@google/gemini-cli-core'; import type { SlashCommand } from '../ui/commands/types.js'; -import type { ICommandLoader } from './types.js'; - -export interface CommandConflict { - name: string; - winner: SlashCommand; - losers: Array<{ - command: SlashCommand; - renamedTo: string; - }>; -} +import type { ICommandLoader, CommandConflict } from './types.js'; +import { SlashCommandResolver } from './SlashCommandResolver.js'; /** * Orchestrates the discovery and loading of all slash commands for the CLI. @@ -24,9 +16,9 @@ export interface CommandConflict { * with an array of `ICommandLoader` instances, each responsible for fetching * commands from a specific source (e.g., built-in code, local files). * - * The CommandService is responsible for invoking these loaders, aggregating their - * results, and resolving any name conflicts. This architecture allows the command - * system to be extended with new sources without modifying the service itself. + * It uses a delegating resolver to reconcile name conflicts, ensuring that + * all commands are uniquely addressable via source-specific prefixes while + * allowing built-in commands to retain their primary names. */ export class CommandService { /** @@ -42,96 +34,71 @@ export class CommandService { /** * Asynchronously creates and initializes a new CommandService instance. * - * This factory method orchestrates the entire command loading process. It - * runs all provided loaders in parallel, aggregates their results, handles - * name conflicts for extension commands by renaming them, and then returns a - * fully constructed `CommandService` instance. + * This factory method orchestrates the loading process and delegates + * conflict resolution to the SlashCommandResolver. * - * Conflict resolution: - * - Extension commands that conflict with existing commands are renamed to - * `extensionName.commandName` - * - Non-extension commands (built-in, user, project) override earlier commands - * with the same name based on loader order - * - * @param loaders An array of objects that conform to the `ICommandLoader` - * interface. Built-in commands should come first, followed by FileCommandLoader. - * @param signal An AbortSignal to cancel the loading process. - * @returns A promise that resolves to a new, fully initialized `CommandService` instance. + * @param loaders An array of loaders to fetch commands from. + * @param signal An AbortSignal to allow cancellation. + * @returns A promise that resolves to a fully initialized CommandService. */ static async create( loaders: ICommandLoader[], signal: AbortSignal, ): Promise { + const allCommands = await this.loadAllCommands(loaders, signal); + const { finalCommands, conflicts } = + SlashCommandResolver.resolve(allCommands); + + if (conflicts.length > 0) { + this.emitConflictEvents(conflicts); + } + + return new CommandService( + Object.freeze(finalCommands), + Object.freeze(conflicts), + ); + } + + /** + * Invokes all loaders in parallel and flattens the results. + */ + private static async loadAllCommands( + loaders: ICommandLoader[], + signal: AbortSignal, + ): Promise { const results = await Promise.allSettled( loaders.map((loader) => loader.loadCommands(signal)), ); - const allCommands: SlashCommand[] = []; + const commands: SlashCommand[] = []; for (const result of results) { if (result.status === 'fulfilled') { - allCommands.push(...result.value); + commands.push(...result.value); } else { debugLogger.debug('A command loader failed:', result.reason); } } + return commands; + } - const commandMap = new Map(); - const conflictsMap = new Map(); - - for (const cmd of allCommands) { - let finalName = cmd.name; - - // Extension commands get renamed if they conflict with existing commands - if (cmd.extensionName && commandMap.has(cmd.name)) { - const winner = commandMap.get(cmd.name)!; - let renamedName = `${cmd.extensionName}.${cmd.name}`; - let suffix = 1; - - // Keep trying until we find a name that doesn't conflict - while (commandMap.has(renamedName)) { - renamedName = `${cmd.extensionName}.${cmd.name}${suffix}`; - suffix++; - } - - finalName = renamedName; - - if (!conflictsMap.has(cmd.name)) { - conflictsMap.set(cmd.name, { - name: cmd.name, - winner, - losers: [], - }); - } - - conflictsMap.get(cmd.name)!.losers.push({ - command: cmd, - renamedTo: finalName, - }); - } - - commandMap.set(finalName, { - ...cmd, - name: finalName, - }); - } - - const conflicts = Array.from(conflictsMap.values()); - if (conflicts.length > 0) { - coreEvents.emitSlashCommandConflicts( - conflicts.flatMap((c) => - c.losers.map((l) => ({ - name: c.name, - renamedTo: l.renamedTo, - loserExtensionName: l.command.extensionName, - winnerExtensionName: c.winner.extensionName, - })), - ), - ); - } - - const finalCommands = Object.freeze(Array.from(commandMap.values())); - const finalConflicts = Object.freeze(conflicts); - return new CommandService(finalCommands, finalConflicts); + /** + * Formats and emits telemetry for command conflicts. + */ + private static emitConflictEvents(conflicts: CommandConflict[]): void { + coreEvents.emitSlashCommandConflicts( + conflicts.flatMap((c) => + c.losers.map((l) => ({ + name: c.name, + renamedTo: l.renamedTo, + loserExtensionName: l.command.extensionName, + winnerExtensionName: l.reason.extensionName, + loserMcpServerName: l.command.mcpServerName, + winnerMcpServerName: l.reason.mcpServerName, + loserKind: l.command.kind, + winnerKind: l.reason.kind, + })), + ), + ); } /** diff --git a/packages/cli/src/services/FileCommandLoader.ts b/packages/cli/src/services/FileCommandLoader.ts index fb27327ead..229ff0b3bc 100644 --- a/packages/cli/src/services/FileCommandLoader.ts +++ b/packages/cli/src/services/FileCommandLoader.ts @@ -37,6 +37,7 @@ import { sanitizeForDisplay } from '../ui/utils/textUtils.js'; interface CommandDirectory { path: string; + kind: CommandKind; extensionName?: string; extensionId?: string; } @@ -111,6 +112,7 @@ export class FileCommandLoader implements ICommandLoader { this.parseAndAdaptFile( path.join(dirInfo.path, file), dirInfo.path, + dirInfo.kind, dirInfo.extensionName, dirInfo.extensionId, ), @@ -151,10 +153,16 @@ export class FileCommandLoader implements ICommandLoader { const storage = this.config?.storage ?? new Storage(this.projectRoot); // 1. User commands - dirs.push({ path: Storage.getUserCommandsDir() }); + dirs.push({ + path: Storage.getUserCommandsDir(), + kind: CommandKind.USER_FILE, + }); - // 2. Project commands (override user commands) - dirs.push({ path: storage.getProjectCommandsDir() }); + // 2. Project commands + dirs.push({ + path: storage.getProjectCommandsDir(), + kind: CommandKind.WORKSPACE_FILE, + }); // 3. Extension commands (processed last to detect all conflicts) if (this.config) { @@ -165,6 +173,7 @@ export class FileCommandLoader implements ICommandLoader { const extensionCommandDirs = activeExtensions.map((ext) => ({ path: path.join(ext.path, 'commands'), + kind: CommandKind.EXTENSION_FILE, extensionName: ext.name, extensionId: ext.id, })); @@ -179,12 +188,14 @@ export class FileCommandLoader implements ICommandLoader { * Parses a single .toml file and transforms it into a SlashCommand object. * @param filePath The absolute path to the .toml file. * @param baseDir The root command directory for name calculation. + * @param kind The CommandKind. * @param extensionName Optional extension name to prefix commands with. * @returns A promise resolving to a SlashCommand, or null if the file is invalid. */ private async parseAndAdaptFile( filePath: string, baseDir: string, + kind: CommandKind, extensionName?: string, extensionId?: string, ): Promise { @@ -286,7 +297,7 @@ export class FileCommandLoader implements ICommandLoader { return { name: baseCommandName, description, - kind: CommandKind.FILE, + kind, extensionName, extensionId, action: async ( diff --git a/packages/cli/src/services/McpPromptLoader.ts b/packages/cli/src/services/McpPromptLoader.ts index f61eed9184..9afeffcdc2 100644 --- a/packages/cli/src/services/McpPromptLoader.ts +++ b/packages/cli/src/services/McpPromptLoader.ts @@ -44,6 +44,7 @@ export class McpPromptLoader implements ICommandLoader { name: commandName, description: prompt.description || `Invoke prompt ${prompt.name}`, kind: CommandKind.MCP_PROMPT, + mcpServerName: serverName, autoExecute: !prompt.arguments || prompt.arguments.length === 0, subCommands: [ { diff --git a/packages/cli/src/services/SlashCommandConflictHandler.test.ts b/packages/cli/src/services/SlashCommandConflictHandler.test.ts new file mode 100644 index 0000000000..a828923fe5 --- /dev/null +++ b/packages/cli/src/services/SlashCommandConflictHandler.test.ts @@ -0,0 +1,175 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { SlashCommandConflictHandler } from './SlashCommandConflictHandler.js'; +import { + coreEvents, + CoreEvent, + type SlashCommandConflictsPayload, + type SlashCommandConflict, +} from '@google/gemini-cli-core'; +import { CommandKind } from '../ui/commands/types.js'; + +vi.mock('@google/gemini-cli-core', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + coreEvents: { + on: vi.fn(), + off: vi.fn(), + emitFeedback: vi.fn(), + }, + }; +}); + +describe('SlashCommandConflictHandler', () => { + let handler: SlashCommandConflictHandler; + + /** + * Helper to find and invoke the registered conflict event listener. + */ + const simulateEvent = (conflicts: SlashCommandConflict[]) => { + const callback = vi + .mocked(coreEvents.on) + .mock.calls.find( + (call) => call[0] === CoreEvent.SlashCommandConflicts, + )![1] as (payload: SlashCommandConflictsPayload) => void; + callback({ conflicts }); + }; + + beforeEach(() => { + vi.useFakeTimers(); + handler = new SlashCommandConflictHandler(); + handler.start(); + }); + + afterEach(() => { + handler.stop(); + vi.clearAllMocks(); + vi.useRealTimers(); + }); + + it('should listen for conflict events on start', () => { + expect(coreEvents.on).toHaveBeenCalledWith( + CoreEvent.SlashCommandConflicts, + expect.any(Function), + ); + }); + + it('should display a descriptive message for a single extension conflict', () => { + simulateEvent([ + { + name: 'deploy', + renamedTo: 'firebase.deploy', + loserExtensionName: 'firebase', + loserKind: CommandKind.EXTENSION_FILE, + winnerKind: CommandKind.BUILT_IN, + }, + ]); + + vi.advanceTimersByTime(600); + + expect(coreEvents.emitFeedback).toHaveBeenCalledWith( + 'info', + "Extension 'firebase' command '/deploy' was renamed to '/firebase.deploy' because it conflicts with built-in command.", + ); + }); + + it('should display a descriptive message for a single MCP conflict', () => { + simulateEvent([ + { + name: 'pickle', + renamedTo: 'test-server.pickle', + loserMcpServerName: 'test-server', + loserKind: CommandKind.MCP_PROMPT, + winnerExtensionName: 'pickle-rick', + winnerKind: CommandKind.EXTENSION_FILE, + }, + ]); + + vi.advanceTimersByTime(600); + + expect(coreEvents.emitFeedback).toHaveBeenCalledWith( + 'info', + "MCP server 'test-server' command '/pickle' was renamed to '/test-server.pickle' because it conflicts with extension 'pickle-rick' command.", + ); + }); + + it('should group multiple conflicts for the same command name', () => { + simulateEvent([ + { + name: 'launch', + renamedTo: 'user.launch', + loserKind: CommandKind.USER_FILE, + winnerKind: CommandKind.WORKSPACE_FILE, + }, + { + name: 'launch', + renamedTo: 'workspace.launch', + loserKind: CommandKind.WORKSPACE_FILE, + winnerKind: CommandKind.USER_FILE, + }, + ]); + + vi.advanceTimersByTime(600); + + expect(coreEvents.emitFeedback).toHaveBeenCalledWith( + 'info', + `Conflicts detected for command '/launch': +- User command '/launch' was renamed to '/user.launch' +- Workspace command '/launch' was renamed to '/workspace.launch'`, + ); + }); + + it('should debounce multiple events within the flush window', () => { + simulateEvent([ + { + name: 'a', + renamedTo: 'user.a', + loserKind: CommandKind.USER_FILE, + winnerKind: CommandKind.BUILT_IN, + }, + ]); + + vi.advanceTimersByTime(200); + + simulateEvent([ + { + name: 'b', + renamedTo: 'user.b', + loserKind: CommandKind.USER_FILE, + winnerKind: CommandKind.BUILT_IN, + }, + ]); + + vi.advanceTimersByTime(600); + + // Should emit two feedbacks (one for each unique command name) + expect(coreEvents.emitFeedback).toHaveBeenCalledTimes(2); + }); + + it('should deduplicate already notified conflicts', () => { + const conflict = { + name: 'deploy', + renamedTo: 'firebase.deploy', + loserExtensionName: 'firebase', + loserKind: CommandKind.EXTENSION_FILE, + winnerKind: CommandKind.BUILT_IN, + }; + + simulateEvent([conflict]); + vi.advanceTimersByTime(600); + expect(coreEvents.emitFeedback).toHaveBeenCalledTimes(1); + + vi.mocked(coreEvents.emitFeedback).mockClear(); + + simulateEvent([conflict]); + vi.advanceTimersByTime(600); + expect(coreEvents.emitFeedback).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/cli/src/services/SlashCommandConflictHandler.ts b/packages/cli/src/services/SlashCommandConflictHandler.ts index 31e110732b..b51617840e 100644 --- a/packages/cli/src/services/SlashCommandConflictHandler.ts +++ b/packages/cli/src/services/SlashCommandConflictHandler.ts @@ -8,10 +8,20 @@ import { coreEvents, CoreEvent, type SlashCommandConflictsPayload, + type SlashCommandConflict, } from '@google/gemini-cli-core'; +import { CommandKind } from '../ui/commands/types.js'; +/** + * Handles slash command conflict events and provides user feedback. + * + * This handler batches multiple conflict events into a single notification + * block per command name to avoid UI clutter during startup or incremental loading. + */ export class SlashCommandConflictHandler { private notifiedConflicts = new Set(); + private pendingConflicts: SlashCommandConflict[] = []; + private flushTimeout: ReturnType | null = null; constructor() { this.handleConflicts = this.handleConflicts.bind(this); @@ -23,11 +33,18 @@ export class SlashCommandConflictHandler { stop() { coreEvents.off(CoreEvent.SlashCommandConflicts, this.handleConflicts); + if (this.flushTimeout) { + clearTimeout(this.flushTimeout); + this.flushTimeout = null; + } } private handleConflicts(payload: SlashCommandConflictsPayload) { const newConflicts = payload.conflicts.filter((c) => { - const key = `${c.name}:${c.loserExtensionName}`; + // Use a unique key to prevent duplicate notifications for the same conflict + const sourceId = + c.loserExtensionName || c.loserMcpServerName || c.loserKind; + const key = `${c.name}:${sourceId}:${c.renamedTo}`; if (this.notifiedConflicts.has(key)) { return false; } @@ -36,19 +53,119 @@ export class SlashCommandConflictHandler { }); if (newConflicts.length > 0) { - const conflictMessages = newConflicts - .map((c) => { - const winnerSource = c.winnerExtensionName - ? `extension '${c.winnerExtensionName}'` - : 'an existing command'; - return `- Command '/${c.name}' from extension '${c.loserExtensionName}' was renamed to '/${c.renamedTo}' because it conflicts with ${winnerSource}.`; - }) - .join('\n'); + this.pendingConflicts.push(...newConflicts); + this.scheduleFlush(); + } + } - coreEvents.emitFeedback( - 'info', - `Command conflicts detected:\n${conflictMessages}`, - ); + private scheduleFlush() { + if (this.flushTimeout) { + clearTimeout(this.flushTimeout); + } + // Use a trailing debounce to capture staggered reloads during startup + this.flushTimeout = setTimeout(() => this.flush(), 500); + } + + private flush() { + this.flushTimeout = null; + const conflicts = [...this.pendingConflicts]; + this.pendingConflicts = []; + + if (conflicts.length === 0) { + return; + } + + // Group conflicts by their original command name + const grouped = new Map(); + for (const c of conflicts) { + const list = grouped.get(c.name) ?? []; + list.push(c); + grouped.set(c.name, list); + } + + for (const [name, commandConflicts] of grouped) { + if (commandConflicts.length > 1) { + this.emitGroupedFeedback(name, commandConflicts); + } else { + this.emitSingleFeedback(commandConflicts[0]); + } + } + } + + /** + * Emits a grouped notification for multiple conflicts sharing the same name. + */ + private emitGroupedFeedback( + name: string, + conflicts: SlashCommandConflict[], + ): void { + const messages = conflicts + .map((c) => { + const source = this.getSourceDescription( + c.loserExtensionName, + c.loserKind, + c.loserMcpServerName, + ); + return `- ${this.capitalize(source)} '/${c.name}' was renamed to '/${c.renamedTo}'`; + }) + .join('\n'); + + coreEvents.emitFeedback( + 'info', + `Conflicts detected for command '/${name}':\n${messages}`, + ); + } + + /** + * Emits a descriptive notification for a single command conflict. + */ + private emitSingleFeedback(c: SlashCommandConflict): void { + const loserSource = this.getSourceDescription( + c.loserExtensionName, + c.loserKind, + c.loserMcpServerName, + ); + const winnerSource = this.getSourceDescription( + c.winnerExtensionName, + c.winnerKind, + c.winnerMcpServerName, + ); + + coreEvents.emitFeedback( + 'info', + `${this.capitalize(loserSource)} '/${c.name}' was renamed to '/${c.renamedTo}' because it conflicts with ${winnerSource}.`, + ); + } + + private capitalize(s: string): string { + return s.charAt(0).toUpperCase() + s.slice(1); + } + + /** + * Returns a human-readable description of a command's source. + */ + private getSourceDescription( + extensionName?: string, + kind?: string, + mcpServerName?: string, + ): string { + switch (kind) { + case CommandKind.EXTENSION_FILE: + return extensionName + ? `extension '${extensionName}' command` + : 'extension command'; + case CommandKind.MCP_PROMPT: + return mcpServerName + ? `MCP server '${mcpServerName}' command` + : 'MCP server command'; + case CommandKind.USER_FILE: + return 'user command'; + case CommandKind.WORKSPACE_FILE: + return 'workspace command'; + case CommandKind.BUILT_IN: + return 'built-in command'; + default: + return 'existing command'; } } } diff --git a/packages/cli/src/services/SlashCommandResolver.test.ts b/packages/cli/src/services/SlashCommandResolver.test.ts new file mode 100644 index 0000000000..e703028b3d --- /dev/null +++ b/packages/cli/src/services/SlashCommandResolver.test.ts @@ -0,0 +1,177 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi } from 'vitest'; +import { SlashCommandResolver } from './SlashCommandResolver.js'; +import { CommandKind, type SlashCommand } from '../ui/commands/types.js'; + +const createMockCommand = (name: string, kind: CommandKind): SlashCommand => ({ + name, + description: `Description for ${name}`, + kind, + action: vi.fn(), +}); + +describe('SlashCommandResolver', () => { + describe('resolve', () => { + it('should return all commands when there are no conflicts', () => { + const cmdA = createMockCommand('a', CommandKind.BUILT_IN); + const cmdB = createMockCommand('b', CommandKind.USER_FILE); + + const { finalCommands, conflicts } = SlashCommandResolver.resolve([ + cmdA, + cmdB, + ]); + + expect(finalCommands).toHaveLength(2); + expect(conflicts).toHaveLength(0); + }); + + it('should rename extension commands when they conflict with built-in', () => { + const builtin = createMockCommand('deploy', CommandKind.BUILT_IN); + const extension = { + ...createMockCommand('deploy', CommandKind.EXTENSION_FILE), + extensionName: 'firebase', + }; + + const { finalCommands, conflicts } = SlashCommandResolver.resolve([ + builtin, + extension, + ]); + + expect(finalCommands.map((c) => c.name)).toContain('deploy'); + expect(finalCommands.map((c) => c.name)).toContain('firebase.deploy'); + expect(conflicts).toHaveLength(1); + }); + + it('should prefix both user and workspace commands when they conflict', () => { + const userCmd = createMockCommand('sync', CommandKind.USER_FILE); + const workspaceCmd = createMockCommand( + 'sync', + CommandKind.WORKSPACE_FILE, + ); + + const { finalCommands, conflicts } = SlashCommandResolver.resolve([ + userCmd, + workspaceCmd, + ]); + + const names = finalCommands.map((c) => c.name); + expect(names).not.toContain('sync'); + expect(names).toContain('user.sync'); + expect(names).toContain('workspace.sync'); + expect(conflicts).toHaveLength(1); + expect(conflicts[0].losers).toHaveLength(2); // Both are considered losers + }); + + it('should prefix file commands but keep built-in names during conflicts', () => { + const builtin = createMockCommand('help', CommandKind.BUILT_IN); + const user = createMockCommand('help', CommandKind.USER_FILE); + + const { finalCommands } = SlashCommandResolver.resolve([builtin, user]); + + const names = finalCommands.map((c) => c.name); + expect(names).toContain('help'); + expect(names).toContain('user.help'); + }); + + it('should prefix both commands when MCP and user file conflict', () => { + const mcp = { + ...createMockCommand('test', CommandKind.MCP_PROMPT), + mcpServerName: 'test-server', + }; + const user = createMockCommand('test', CommandKind.USER_FILE); + + const { finalCommands } = SlashCommandResolver.resolve([mcp, user]); + + const names = finalCommands.map((c) => c.name); + expect(names).not.toContain('test'); + expect(names).toContain('test-server.test'); + expect(names).toContain('user.test'); + }); + + it('should prefix MCP commands with server name when they conflict with built-in', () => { + const builtin = createMockCommand('help', CommandKind.BUILT_IN); + const mcp = { + ...createMockCommand('help', CommandKind.MCP_PROMPT), + mcpServerName: 'test-server', + }; + + const { finalCommands } = SlashCommandResolver.resolve([builtin, mcp]); + + const names = finalCommands.map((c) => c.name); + expect(names).toContain('help'); + expect(names).toContain('test-server.help'); + }); + + it('should prefix both MCP commands when they conflict with each other', () => { + const mcp1 = { + ...createMockCommand('test', CommandKind.MCP_PROMPT), + mcpServerName: 'server1', + }; + const mcp2 = { + ...createMockCommand('test', CommandKind.MCP_PROMPT), + mcpServerName: 'server2', + }; + + const { finalCommands } = SlashCommandResolver.resolve([mcp1, mcp2]); + + const names = finalCommands.map((c) => c.name); + expect(names).not.toContain('test'); + expect(names).toContain('server1.test'); + expect(names).toContain('server2.test'); + }); + + it('should favor the last built-in command silently during conflicts', () => { + const builtin1 = { + ...createMockCommand('help', CommandKind.BUILT_IN), + description: 'first', + }; + const builtin2 = { + ...createMockCommand('help', CommandKind.BUILT_IN), + description: 'second', + }; + + const { finalCommands } = SlashCommandResolver.resolve([ + builtin1, + builtin2, + ]); + + expect(finalCommands).toHaveLength(1); + expect(finalCommands[0].description).toBe('second'); + }); + + it('should fallback to numeric suffixes when both prefix and kind-based prefix are missing', () => { + const cmd1 = createMockCommand('test', CommandKind.BUILT_IN); + const cmd2 = { + ...createMockCommand('test', 'unknown' as CommandKind), + }; + + const { finalCommands } = SlashCommandResolver.resolve([cmd1, cmd2]); + + const names = finalCommands.map((c) => c.name); + expect(names).toContain('test'); + expect(names).toContain('test1'); + }); + + it('should apply numeric suffixes when renames also conflict', () => { + const user1 = createMockCommand('deploy', CommandKind.USER_FILE); + const user2 = createMockCommand('gcp.deploy', CommandKind.USER_FILE); + const extension = { + ...createMockCommand('deploy', CommandKind.EXTENSION_FILE), + extensionName: 'gcp', + }; + + const { finalCommands } = SlashCommandResolver.resolve([ + user1, + user2, + extension, + ]); + + expect(finalCommands.find((c) => c.name === 'gcp.deploy1')).toBeDefined(); + }); + }); +}); diff --git a/packages/cli/src/services/SlashCommandResolver.ts b/packages/cli/src/services/SlashCommandResolver.ts new file mode 100644 index 0000000000..aad4d98fe4 --- /dev/null +++ b/packages/cli/src/services/SlashCommandResolver.ts @@ -0,0 +1,213 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { SlashCommand } from '../ui/commands/types.js'; +import { CommandKind } from '../ui/commands/types.js'; +import type { CommandConflict } from './types.js'; + +/** + * Internal registry to track commands and conflicts during resolution. + */ +class CommandRegistry { + readonly commandMap = new Map(); + readonly conflictsMap = new Map(); + readonly firstEncounters = new Map(); + + get finalCommands(): SlashCommand[] { + return Array.from(this.commandMap.values()); + } + + get conflicts(): CommandConflict[] { + return Array.from(this.conflictsMap.values()); + } +} + +/** + * Resolves name conflicts among slash commands. + * + * Rules: + * 1. Built-in commands always keep the original name. + * 2. All other types are prefixed with their source name (e.g. user.name). + * 3. If multiple non-built-in commands conflict, all of them are renamed. + */ +export class SlashCommandResolver { + /** + * Orchestrates conflict resolution by applying renaming rules to ensures + * every command has a unique name. + */ + static resolve(allCommands: SlashCommand[]): { + finalCommands: SlashCommand[]; + conflicts: CommandConflict[]; + } { + const registry = new CommandRegistry(); + + for (const cmd of allCommands) { + const originalName = cmd.name; + let finalName = originalName; + + if (registry.firstEncounters.has(originalName)) { + // We've already seen a command with this name, so resolve the conflict. + finalName = this.handleConflict(cmd, registry); + } else { + // Track the first claimant to report them as the conflict reason later. + registry.firstEncounters.set(originalName, cmd); + } + + // Store under final name, ensuring the command object reflects it. + registry.commandMap.set(finalName, { + ...cmd, + name: finalName, + }); + } + + return { + finalCommands: registry.finalCommands, + conflicts: registry.conflicts, + }; + } + + /** + * Resolves a name collision by deciding which command keeps the name and which is renamed. + * + * @param incoming The command currently being processed that has a name collision. + * @param registry The internal state of the resolution process. + * @returns The final name to be assigned to the `incoming` command. + */ + private static handleConflict( + incoming: SlashCommand, + registry: CommandRegistry, + ): string { + const collidingName = incoming.name; + const originalClaimant = registry.firstEncounters.get(collidingName)!; + + // Incoming built-in takes priority. Prefix any existing owner. + if (incoming.kind === CommandKind.BUILT_IN) { + this.prefixExistingCommand(collidingName, incoming, registry); + return collidingName; + } + + // Incoming non-built-in is renamed to its source-prefixed version. + const renamedName = this.getRenamedName( + incoming.name, + this.getPrefix(incoming), + registry.commandMap, + ); + this.trackConflict( + registry.conflictsMap, + collidingName, + originalClaimant, + incoming, + renamedName, + ); + + // Prefix current owner as well if it isn't a built-in. + this.prefixExistingCommand(collidingName, incoming, registry); + + return renamedName; + } + + /** + * Safely renames the command currently occupying a name in the registry. + * + * @param name The name of the command to prefix. + * @param reason The incoming command that is causing the prefixing. + * @param registry The internal state of the resolution process. + */ + private static prefixExistingCommand( + name: string, + reason: SlashCommand, + registry: CommandRegistry, + ): void { + const currentOwner = registry.commandMap.get(name); + + // Only non-built-in commands can be prefixed. + if (!currentOwner || currentOwner.kind === CommandKind.BUILT_IN) { + return; + } + + // Determine the new name for the owner using its source prefix. + const renamedName = this.getRenamedName( + currentOwner.name, + this.getPrefix(currentOwner), + registry.commandMap, + ); + + // Update the registry: remove the old name and add the owner under the new name. + registry.commandMap.delete(name); + const renamedOwner = { ...currentOwner, name: renamedName }; + registry.commandMap.set(renamedName, renamedOwner); + + // Record the conflict so the user can be notified of the prefixing. + this.trackConflict( + registry.conflictsMap, + name, + reason, + currentOwner, + renamedName, + ); + } + + /** + * Generates a unique name using numeric suffixes if needed. + */ + private static getRenamedName( + name: string, + prefix: string | undefined, + commandMap: Map, + ): string { + const base = prefix ? `${prefix}.${name}` : name; + let renamedName = base; + let suffix = 1; + + while (commandMap.has(renamedName)) { + renamedName = `${base}${suffix}`; + suffix++; + } + return renamedName; + } + + /** + * Returns a suitable prefix for a conflicting command. + */ + private static getPrefix(cmd: SlashCommand): string | undefined { + switch (cmd.kind) { + case CommandKind.EXTENSION_FILE: + return cmd.extensionName; + case CommandKind.MCP_PROMPT: + return cmd.mcpServerName; + case CommandKind.USER_FILE: + return 'user'; + case CommandKind.WORKSPACE_FILE: + return 'workspace'; + default: + return undefined; + } + } + + /** + * Logs a conflict event. + */ + private static trackConflict( + conflictsMap: Map, + originalName: string, + reason: SlashCommand, + displacedCommand: SlashCommand, + renamedTo: string, + ) { + if (!conflictsMap.has(originalName)) { + conflictsMap.set(originalName, { + name: originalName, + losers: [], + }); + } + + conflictsMap.get(originalName)!.losers.push({ + command: displacedCommand, + renamedTo, + reason, + }); + } +} diff --git a/packages/cli/src/services/types.ts b/packages/cli/src/services/types.ts index 13a87687ee..b583e56e39 100644 --- a/packages/cli/src/services/types.ts +++ b/packages/cli/src/services/types.ts @@ -22,3 +22,12 @@ export interface ICommandLoader { */ loadCommands(signal: AbortSignal): Promise; } + +export interface CommandConflict { + name: string; + losers: Array<{ + command: SlashCommand; + renamedTo: string; + reason: SlashCommand; + }>; +} diff --git a/packages/cli/src/ui/commands/types.ts b/packages/cli/src/ui/commands/types.ts index 2cbb9da9a7..a88457ed9e 100644 --- a/packages/cli/src/ui/commands/types.ts +++ b/packages/cli/src/ui/commands/types.ts @@ -177,7 +177,9 @@ export type SlashCommandActionReturn = export enum CommandKind { BUILT_IN = 'built-in', - FILE = 'file', + USER_FILE = 'user-file', + WORKSPACE_FILE = 'workspace-file', + EXTENSION_FILE = 'extension-file', MCP_PROMPT = 'mcp-prompt', AGENT = 'agent', } @@ -203,6 +205,9 @@ export interface SlashCommand { extensionName?: string; extensionId?: string; + // Optional metadata for MCP commands + mcpServerName?: string; + // The action to run. Optional for parent commands that only group sub-commands. action?: ( context: CommandContext, diff --git a/packages/cli/src/ui/components/InputPrompt.test.tsx b/packages/cli/src/ui/components/InputPrompt.test.tsx index ac880e4624..b8148b0bef 100644 --- a/packages/cli/src/ui/components/InputPrompt.test.tsx +++ b/packages/cli/src/ui/components/InputPrompt.test.tsx @@ -1349,7 +1349,7 @@ describe('InputPrompt', () => { it('should autocomplete custom commands from .toml files on Enter', async () => { const customCommand: SlashCommand = { name: 'find-capital', - kind: CommandKind.FILE, + kind: CommandKind.USER_FILE, description: 'Find capital of a country', action: vi.fn(), // No autoExecute flag - custom commands default to undefined diff --git a/packages/cli/src/ui/hooks/slashCommandProcessor.test.tsx b/packages/cli/src/ui/hooks/slashCommandProcessor.test.tsx index 6190d163f7..f47aa30fba 100644 --- a/packages/cli/src/ui/hooks/slashCommandProcessor.test.tsx +++ b/packages/cli/src/ui/hooks/slashCommandProcessor.test.tsx @@ -18,14 +18,11 @@ import { FileCommandLoader } from '../../services/FileCommandLoader.js'; import { McpPromptLoader } from '../../services/McpPromptLoader.js'; import { type GeminiClient, - type UserFeedbackPayload, SlashCommandStatus, MCPDiscoveryState, makeFakeConfig, coreEvents, - CoreEvent, } from '@google/gemini-cli-core'; -import { SlashCommandConflictHandler } from '../../services/SlashCommandConflictHandler.js'; const { logSlashCommand, @@ -186,26 +183,6 @@ describe('useSlashCommandProcessor', () => { mockFileLoadCommands.mockResolvedValue(Object.freeze(fileCommands)); mockMcpLoadCommands.mockResolvedValue(Object.freeze(mcpCommands)); - const conflictHandler = new SlashCommandConflictHandler(); - conflictHandler.start(); - - const handleFeedback = (payload: UserFeedbackPayload) => { - let type = MessageType.INFO; - if (payload.severity === 'error') { - type = MessageType.ERROR; - } else if (payload.severity === 'warning') { - type = MessageType.WARNING; - } - mockAddItem( - { - type, - text: payload.message, - }, - Date.now(), - ); - }; - coreEvents.on(CoreEvent.UserFeedback, handleFeedback); - let result!: { current: ReturnType }; let unmount!: () => void; let rerender!: (props?: unknown) => void; @@ -253,8 +230,6 @@ describe('useSlashCommandProcessor', () => { }); unmountHook = async () => { - conflictHandler.stop(); - coreEvents.off(CoreEvent.UserFeedback, handleFeedback); unmount(); }; @@ -336,57 +311,6 @@ describe('useSlashCommandProcessor', () => { expect(mockFileLoadCommands).toHaveBeenCalledTimes(1); expect(mockMcpLoadCommands).toHaveBeenCalledTimes(1); }); - - it('should provide an immutable array of commands to consumers', async () => { - const testCommand = createTestCommand({ name: 'test' }); - const result = await setupProcessorHook({ - builtinCommands: [testCommand], - }); - - await waitFor(() => { - expect(result.current.slashCommands).toHaveLength(1); - }); - - const commands = result.current.slashCommands; - - expect(() => { - // @ts-expect-error - We are intentionally testing a violation of the readonly type. - commands.push(createTestCommand({ name: 'rogue' })); - }).toThrow(TypeError); - }); - - it('should override built-in commands with file-based commands of the same name', async () => { - const builtinAction = vi.fn(); - const fileAction = vi.fn(); - - const builtinCommand = createTestCommand({ - name: 'override', - description: 'builtin', - action: builtinAction, - }); - const fileCommand = createTestCommand( - { name: 'override', description: 'file', action: fileAction }, - CommandKind.FILE, - ); - - const result = await setupProcessorHook({ - builtinCommands: [builtinCommand], - fileCommands: [fileCommand], - }); - - await waitFor(() => { - // The service should only return one command with the name 'override' - expect(result.current.slashCommands).toHaveLength(1); - }); - - await act(async () => { - await result.current.handleSlashCommand('/override'); - }); - - // Only the file-based command's action should be called. - expect(fileAction).toHaveBeenCalledTimes(1); - expect(builtinAction).not.toHaveBeenCalled(); - }); }); describe('Command Execution Logic', () => { @@ -731,7 +655,7 @@ describe('useSlashCommandProcessor', () => { content: [{ text: 'The actual prompt from the TOML file.' }], }), }, - CommandKind.FILE, + CommandKind.USER_FILE, ); const result = await setupProcessorHook({ @@ -866,42 +790,6 @@ describe('useSlashCommandProcessor', () => { }); describe('Command Precedence', () => { - it('should override mcp-based commands with file-based commands of the same name', async () => { - const mcpAction = vi.fn(); - const fileAction = vi.fn(); - - const mcpCommand = createTestCommand( - { - name: 'override', - description: 'mcp', - action: mcpAction, - }, - CommandKind.MCP_PROMPT, - ); - const fileCommand = createTestCommand( - { name: 'override', description: 'file', action: fileAction }, - CommandKind.FILE, - ); - - const result = await setupProcessorHook({ - fileCommands: [fileCommand], - mcpCommands: [mcpCommand], - }); - - await waitFor(() => { - // The service should only return one command with the name 'override' - expect(result.current.slashCommands).toHaveLength(1); - }); - - await act(async () => { - await result.current.handleSlashCommand('/override'); - }); - - // Only the file-based command's action should be called. - expect(fileAction).toHaveBeenCalledTimes(1); - expect(mcpAction).not.toHaveBeenCalled(); - }); - it('should prioritize a command with a primary name over a command with a matching alias', async () => { const quitAction = vi.fn(); const exitAction = vi.fn(); @@ -917,7 +805,7 @@ describe('useSlashCommandProcessor', () => { name: 'exit', action: exitAction, }, - CommandKind.FILE, + CommandKind.USER_FILE, ); // The order of commands in the final loaded array is not guaranteed, @@ -949,7 +837,7 @@ describe('useSlashCommandProcessor', () => { }); const exitCommand = createTestCommand( { name: 'exit', action: vi.fn() }, - CommandKind.FILE, + CommandKind.USER_FILE, ); const result = await setupProcessorHook({ @@ -1106,119 +994,4 @@ describe('useSlashCommandProcessor', () => { expect(result.current.slashCommands).toEqual([newCommand]), ); }); - - describe('Conflict Notifications', () => { - it('should display a warning when a command conflict occurs', async () => { - const builtinCommand = createTestCommand({ name: 'deploy' }); - const extensionCommand = createTestCommand( - { - name: 'deploy', - extensionName: 'firebase', - }, - CommandKind.FILE, - ); - - const result = await setupProcessorHook({ - builtinCommands: [builtinCommand], - fileCommands: [extensionCommand], - }); - - await waitFor(() => expect(result.current.slashCommands).toHaveLength(2)); - - expect(mockAddItem).toHaveBeenCalledWith( - expect.objectContaining({ - type: MessageType.INFO, - text: expect.stringContaining('Command conflicts detected'), - }), - expect.any(Number), - ); - - expect(mockAddItem).toHaveBeenCalledWith( - expect.objectContaining({ - type: MessageType.INFO, - text: expect.stringContaining( - "- Command '/deploy' from extension 'firebase' was renamed", - ), - }), - expect.any(Number), - ); - }); - - it('should deduplicate conflict warnings across re-renders', async () => { - const builtinCommand = createTestCommand({ name: 'deploy' }); - const extensionCommand = createTestCommand( - { - name: 'deploy', - extensionName: 'firebase', - }, - CommandKind.FILE, - ); - - const result = await setupProcessorHook({ - builtinCommands: [builtinCommand], - fileCommands: [extensionCommand], - }); - - await waitFor(() => expect(result.current.slashCommands).toHaveLength(2)); - - // First notification - expect(mockAddItem).toHaveBeenCalledWith( - expect.objectContaining({ - type: MessageType.INFO, - text: expect.stringContaining('Command conflicts detected'), - }), - expect.any(Number), - ); - - mockAddItem.mockClear(); - - // Trigger a reload or re-render - await act(async () => { - result.current.commandContext.ui.reloadCommands(); - }); - - // Wait a bit for effect to run - await new Promise((resolve) => setTimeout(resolve, 100)); - - // Should NOT have notified again - expect(mockAddItem).not.toHaveBeenCalledWith( - expect.objectContaining({ - type: MessageType.INFO, - text: expect.stringContaining('Command conflicts detected'), - }), - expect.any(Number), - ); - }); - - it('should correctly identify the winner extension in the message', async () => { - const ext1Command = createTestCommand( - { - name: 'deploy', - extensionName: 'firebase', - }, - CommandKind.FILE, - ); - const ext2Command = createTestCommand( - { - name: 'deploy', - extensionName: 'aws', - }, - CommandKind.FILE, - ); - - const result = await setupProcessorHook({ - fileCommands: [ext1Command, ext2Command], - }); - - await waitFor(() => expect(result.current.slashCommands).toHaveLength(2)); - - expect(mockAddItem).toHaveBeenCalledWith( - expect.objectContaining({ - type: MessageType.INFO, - text: expect.stringContaining("conflicts with extension 'firebase'"), - }), - expect.any(Number), - ); - }); - }); }); diff --git a/packages/cli/src/ui/hooks/useSlashCompletion.test.ts b/packages/cli/src/ui/hooks/useSlashCompletion.test.ts index ea320b80a1..9c60f6a5fd 100644 --- a/packages/cli/src/ui/hooks/useSlashCompletion.test.ts +++ b/packages/cli/src/ui/hooks/useSlashCompletion.test.ts @@ -1079,7 +1079,7 @@ describe('useSlashCompletion', () => { { name: 'custom-script', description: 'Run custom script', - kind: CommandKind.FILE, + kind: CommandKind.USER_FILE, action: vi.fn(), }, ] as SlashCommand[]; @@ -1099,7 +1099,7 @@ describe('useSlashCompletion', () => { label: 'custom-script', value: 'custom-script', description: 'Run custom script', - commandKind: CommandKind.FILE, + commandKind: CommandKind.USER_FILE, }, ]); expect(result.current.completionStart).toBe(1); diff --git a/packages/cli/src/utils/commands.test.ts b/packages/cli/src/utils/commands.test.ts index 30040a0350..85af0c624b 100644 --- a/packages/cli/src/utils/commands.test.ts +++ b/packages/cli/src/utils/commands.test.ts @@ -20,7 +20,7 @@ const mockCommands: readonly SlashCommand[] = [ name: 'commit', description: 'Commit changes', action: async () => {}, - kind: CommandKind.FILE, + kind: CommandKind.USER_FILE, }, { name: 'memory', diff --git a/packages/core/src/utils/events.ts b/packages/core/src/utils/events.ts index f1a6402e69..47c42c93ba 100644 --- a/packages/core/src/utils/events.ts +++ b/packages/core/src/utils/events.ts @@ -149,6 +149,10 @@ export interface SlashCommandConflict { renamedTo: string; loserExtensionName?: string; winnerExtensionName?: string; + loserMcpServerName?: string; + winnerMcpServerName?: string; + loserKind?: string; + winnerKind?: string; } export interface SlashCommandConflictsPayload {