diff --git a/packages/cli/src/services/CommandService.ts b/packages/cli/src/services/CommandService.ts index 61f9457619..b3c2afddce 100644 --- a/packages/cli/src/services/CommandService.ts +++ b/packages/cli/src/services/CommandService.ts @@ -49,9 +49,11 @@ export class CommandService { const { finalCommands, conflicts } = SlashCommandResolver.resolve(allCommands); - if (conflicts.length > 0) { - this.emitConflictEvents(conflicts); - } + // Always emit, even with no conflicts: the handler treats each event as + // the current conflict snapshot and uses absence to detect resolution. + // Skipping empty emissions would leave stale dedup state that suppresses + // legitimate notifications when a conflict reappears (issue #24333). + this.emitConflictEvents(conflicts); return new CommandService( Object.freeze(finalCommands), diff --git a/packages/cli/src/services/SlashCommandConflictHandler.test.ts b/packages/cli/src/services/SlashCommandConflictHandler.test.ts index 5527188a04..37a6586f69 100644 --- a/packages/cli/src/services/SlashCommandConflictHandler.test.ts +++ b/packages/cli/src/services/SlashCommandConflictHandler.test.ts @@ -126,26 +126,25 @@ describe('SlashCommandConflictHandler', () => { ); }); - it('should debounce multiple events within the flush window', () => { - simulateEvent([ - { - name: 'a', - renamedTo: 'user.a', - loserKind: CommandKind.USER_FILE, - winnerKind: CommandKind.BUILT_IN, - }, - ]); + it('should debounce newly active conflicts within the flush window', () => { + const conflictA = { + name: 'a', + renamedTo: 'user.a', + loserKind: CommandKind.USER_FILE, + winnerKind: CommandKind.BUILT_IN, + }; + const conflictB = { + name: 'b', + renamedTo: 'user.b', + loserKind: CommandKind.USER_FILE, + winnerKind: CommandKind.BUILT_IN, + }; + + simulateEvent([conflictA]); vi.advanceTimersByTime(200); - simulateEvent([ - { - name: 'b', - renamedTo: 'user.b', - loserKind: CommandKind.USER_FILE, - winnerKind: CommandKind.BUILT_IN, - }, - ]); + simulateEvent([conflictA, conflictB]); vi.advanceTimersByTime(600); @@ -173,6 +172,95 @@ describe('SlashCommandConflictHandler', () => { expect(coreEvents.emitFeedback).not.toHaveBeenCalled(); }); + it('should re-notify when a previously-resolved conflict reappears', () => { + const conflict = { + name: 'deploy', + renamedTo: 'firebase.deploy', + loserExtensionName: 'firebase', + loserKind: CommandKind.EXTENSION_FILE, + winnerKind: CommandKind.BUILT_IN, + }; + + // Initial occurrence triggers a notification. + simulateEvent([conflict]); + vi.advanceTimersByTime(600); + expect(coreEvents.emitFeedback).toHaveBeenCalledTimes(1); + vi.mocked(coreEvents.emitFeedback).mockClear(); + + // Conflict resolved: the next emission is empty (no conflicts active). + simulateEvent([]); + vi.advanceTimersByTime(600); + expect(coreEvents.emitFeedback).not.toHaveBeenCalled(); + + // Same conflict reintroduced later: a fresh notification must fire, + // because the dedup state was cleared when the conflict went away. + simulateEvent([conflict]); + vi.advanceTimersByTime(600); + expect(coreEvents.emitFeedback).toHaveBeenCalledTimes(1); + }); + + it('should not duplicate pending notifications when a conflict resolves and reappears before flush', () => { + const conflict = { + name: 'deploy', + renamedTo: 'firebase.deploy', + loserExtensionName: 'firebase', + loserKind: CommandKind.EXTENSION_FILE, + winnerKind: CommandKind.BUILT_IN, + }; + + simulateEvent([conflict]); + vi.advanceTimersByTime(200); + + simulateEvent([]); + vi.advanceTimersByTime(200); + + simulateEvent([conflict]); + vi.advanceTimersByTime(600); + + expect(coreEvents.emitFeedback).toHaveBeenCalledTimes(1); + expect(coreEvents.emitFeedback).toHaveBeenCalledWith( + 'info', + "Extension 'firebase' command '/deploy' was renamed to '/firebase.deploy' because it conflicts with built-in command.", + ); + }); + + it('should re-notify only the conflicts that were resolved and reappeared', () => { + const conflictA = { + name: 'deploy', + renamedTo: 'firebase.deploy', + loserExtensionName: 'firebase', + loserKind: CommandKind.EXTENSION_FILE, + winnerKind: CommandKind.BUILT_IN, + }; + const conflictB = { + name: 'launch', + renamedTo: 'workspace.launch', + loserKind: CommandKind.WORKSPACE_FILE, + winnerKind: CommandKind.USER_FILE, + }; + + // Both active: each gets a notification. + simulateEvent([conflictA, conflictB]); + vi.advanceTimersByTime(600); + expect(coreEvents.emitFeedback).toHaveBeenCalledTimes(2); + vi.mocked(coreEvents.emitFeedback).mockClear(); + + // Only A is still active. B should be dropped from dedup. + simulateEvent([conflictA]); + vi.advanceTimersByTime(600); + expect(coreEvents.emitFeedback).not.toHaveBeenCalled(); + + // Both reported again. A is still deduped (continuously active); + // B re-fires because it was resolved between events. + simulateEvent([conflictA, conflictB]); + vi.advanceTimersByTime(600); + expect(coreEvents.emitFeedback).toHaveBeenCalledTimes(1); + expect(coreEvents.emitFeedback).toHaveBeenCalledWith( + 'info', + "Workspace command '/launch' was renamed to '/workspace.launch' because it conflicts with user command.", + ); + }); + it('should display a descriptive message for a skill conflict', () => { simulateEvent([ { diff --git a/packages/cli/src/services/SlashCommandConflictHandler.ts b/packages/cli/src/services/SlashCommandConflictHandler.ts index 7da4e53842..dc2ddde57f 100644 --- a/packages/cli/src/services/SlashCommandConflictHandler.ts +++ b/packages/cli/src/services/SlashCommandConflictHandler.ts @@ -19,6 +19,12 @@ import { CommandKind } from '../ui/commands/types.js'; * block per command name to avoid UI clutter during startup or incremental loading. */ export class SlashCommandConflictHandler { + /** + * Keys of conflicts the user has already been notified about and that are + * still active in the most recent payload. A conflict that disappears from + * the payload is treated as resolved and dropped here, so a fresh + * notification fires when the same conflict reappears later. + */ private notifiedConflicts = new Set(); private pendingConflicts: SlashCommandConflict[] = []; private flushTimeout: ReturnType | null = null; @@ -40,17 +46,25 @@ export class SlashCommandConflictHandler { } private handleConflicts(payload: SlashCommandConflictsPayload) { - const newConflicts = payload.conflicts.filter((c) => { - // 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; + const activeKeys = new Set(); + const newConflicts: SlashCommandConflict[] = []; + + for (const c of payload.conflicts) { + const key = this.getConflictKey(c); + activeKeys.add(key); + if (!this.notifiedConflicts.has(key)) { + newConflicts.push(c); } - this.notifiedConflicts.add(key); - return true; - }); + } + + // Replace dedup state with the current snapshot. Conflicts no longer in + // the payload (resolved) are dropped so they re-trigger a notification if + // they reappear; conflicts still present remain deduped to avoid spam. + this.notifiedConflicts = activeKeys; + + this.pendingConflicts = this.pendingConflicts.filter((c) => + activeKeys.has(this.getConflictKey(c)), + ); if (newConflicts.length > 0) { this.pendingConflicts.push(...newConflicts); @@ -58,6 +72,12 @@ export class SlashCommandConflictHandler { } } + private getConflictKey(c: SlashCommandConflict): string { + const sourceId = + c.loserExtensionName || c.loserMcpServerName || c.loserKind; + return `${c.name}:${sourceId}:${c.renamedTo}`; + } + private scheduleFlush() { if (this.flushTimeout) { clearTimeout(this.flushTimeout);