From ebe22790b549c8d1514ef94a9975eda922d65aca Mon Sep 17 00:00:00 2001 From: pmenic Date: Tue, 28 Apr 2026 17:28:36 +0200 Subject: [PATCH] fix(cli): reset slash-command conflict dedupe when conflicts reappear Closes #24333. SlashCommandConflictHandler now treats each conflict event as the current active-conflict snapshot instead of keeping a process-lifetime history. Conflicts that remain active stay deduped, conflicts absent from the latest snapshot are dropped from dedupe state, and reintroduced conflicts notify again. Pending notifications are also filtered against each new snapshot so a conflict that resolves and reappears inside the debounce window is emitted once instead of producing stale or duplicate feedback. CommandService emits the conflict snapshot even when it is empty, which lets the handler observe that all conflicts have resolved. Tests cover continuous dedupe, re-notification after resolution, partial reintroduction, the debounce snapshot path, and the pre-flush resolve/reappear edge case. --- packages/cli/src/services/CommandService.ts | 8 +- .../SlashCommandConflictHandler.test.ts | 122 +++++++++++++++--- .../services/SlashCommandConflictHandler.ts | 40 ++++-- 3 files changed, 140 insertions(+), 30 deletions(-) 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);