Merge 51290a86ea into 3cc7e5b096

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),

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([
       {

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<string>();
   private pendingConflicts: SlashCommandConflict[] = [];
   private flushTimeout: ReturnType<typeof setTimeout> | 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<string>();
+    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);