refactor(gemini): prevent session swap race conditions, dedupe history, unify model resolution

Scoteezy · Scoteezy · commit 3960d0209c74 · 2026-01-16T13:06:08.000+03:00
diff --git a/src/agent/acp/AcpBackend.ts b/src/agent/acp/AcpBackend.ts
@@ -997,12 +997,24 @@ export class AcpBackend implements AgentBackend {
     }
   }
 
+  /**
+   * Emit permission response event for UI/logging purposes.
+   *
+   * **IMPORTANT:** For ACP backends, this method does NOT send the actual permission
+   * response to the agent. The ACP protocol requires synchronous permission handling,
+   * which is done inside the `requestPermission` RPC handler via `this.options.permissionHandler`.
+   *
+   * This method only emits a `permission-response` event for:
+   * - UI updates (e.g., closing permission dialogs)
+   * - Logging and debugging
+   * - Other parts of the CLI that need to react to permission decisions
+   *
+   * @param requestId - The ID of the permission request
+   * @param approved - Whether the permission was granted
+   */
   async respondToPermission(requestId: string, approved: boolean): Promise<void> {
-    logger.debug(`[AcpBackend] Permission response: ${requestId} = ${approved}`);
+    logger.debug(`[AcpBackend] Permission response event (UI only): ${requestId} = ${approved}`);
     this.emit({ type: 'permission-response', id: requestId, approved });
-    // IMPORTANT: The actual ACP permission response is handled synchronously
-    // within the `requestPermission` method via `this.options.permissionHandler`.
-    // This method only emits an internal event for other parts of the CLI to react to.
   }
 
   async dispose(): Promise<void> {
diff --git a/src/agent/core/AgentBackend.ts b/src/agent/core/AgentBackend.ts
@@ -141,7 +141,16 @@ export interface AgentBackend {
   
   /**
    * Respond to a permission request.
-   * 
+   *
+   * **Implementation Note for ACP backends:**
+   * For ACP-based agents (Gemini, Codex via ACP), permission handling is done
+   * synchronously within the `requestPermission` RPC handler via `AcpPermissionHandler`.
+   * This method only emits an internal `permission-response` event for UI/logging purposes.
+   * The actual ACP response is already sent by the time this method is called.
+   *
+   * For non-ACP backends, this method should actually send the permission response
+   * to the agent.
+   *
    * @param requestId - The ID of the permission request
    * @param approved - Whether the permission was granted
    */
diff --git a/src/agent/factories/gemini.ts b/src/agent/factories/gemini.ts
@@ -50,17 +50,28 @@ export interface GeminiBackendOptions extends AgentFactoryOptions {
   permissionHandler?: AcpPermissionHandler;
 }
 
+/**
+ * Result of creating a Gemini backend
+ */
+export interface GeminiBackendResult {
+  /** The created AgentBackend instance */
+  backend: AgentBackend;
+  /** The resolved model that will be used (single source of truth) */
+  model: string;
+  /** Source of the model selection for logging */
+  modelSource: 'explicit' | 'env-var' | 'local-config' | 'default';
+}
+
 /**
  * Create a Gemini backend using ACP (official SDK).
- * 
+ *
  * The Gemini CLI must be installed and available in PATH.
  * Uses the --experimental-acp flag to enable ACP mode.
- * 
+ *
  * @param options - Configuration options
- * @returns AgentBackend instance for Gemini
+ * @returns GeminiBackendResult with backend and resolved model (single source of truth)
  */
-
-export function createGeminiBackend(options: GeminiBackendOptions): AgentBackend {
+export function createGeminiBackend(options: GeminiBackendOptions): GeminiBackendResult {
 
   // Resolve API key from multiple sources (in priority order):
   // 1. Happy cloud OAuth token (via 'happy connect gemini') - highest priority
@@ -145,7 +156,7 @@ export function createGeminiBackend(options: GeminiBackendOptions): AgentBackend
 
   // Determine model source for logging
   const modelSource = getGeminiModelSource(options.model, localConfig);
-  
+
   logger.debug('[Gemini] Creating ACP SDK backend with options:', {
     cwd: backendOptions.cwd,
     command: backendOptions.command,
@@ -156,7 +167,11 @@ export function createGeminiBackend(options: GeminiBackendOptions): AgentBackend
     mcpServerCount: options.mcpServers ? Object.keys(options.mcpServers).length : 0,
   });
 
-  return new AcpBackend(backendOptions);
+  return {
+    backend: new AcpBackend(backendOptions),
+    model,
+    modelSource,
+  };
 }
 
 /**
@@ -166,7 +181,7 @@ export function createGeminiBackend(options: GeminiBackendOptions): AgentBackend
  * to make the Gemini agent available for use.
  */
 export function registerGeminiAgent(): void {
-  agentRegistry.register('gemini', (opts) => createGeminiBackend(opts));
+  agentRegistry.register('gemini', (opts) => createGeminiBackend(opts).backend);
   logger.debug('[Gemini] Registered with agent registry');
 }
 
diff --git a/src/agent/factories/index.ts b/src/agent/factories/index.ts
@@ -12,6 +12,7 @@ export {
   createGeminiBackend,
   registerGeminiAgent,
   type GeminiBackendOptions,
+  type GeminiBackendResult,
 } from './gemini';
 
 // Future factories:
diff --git a/src/agent/transport/handlers/GeminiTransport.ts b/src/agent/transport/handlers/GeminiTransport.ts
@@ -20,6 +20,7 @@ import type {
   ToolNameContext,
 } from '../TransportHandler';
 import type { AgentMessage } from '../../core';
+import { logger } from '@/ui/logger';
 
 /**
  * Gemini-specific timeout values (in milliseconds)
@@ -313,6 +314,16 @@ export class GeminiTransport implements TransportHandler {
     }
 
     // Return original tool name if we couldn't determine it
+    // Log unknown patterns so developers can add them to GEMINI_TOOL_PATTERNS
+    if (toolName === 'other' || toolName === 'Unknown tool') {
+      const inputKeys = input && typeof input === 'object' ? Object.keys(input) : [];
+      logger.debug(
+        `[GeminiTransport] Unknown tool pattern - toolCallId: "${toolCallId}", ` +
+        `toolName: "${toolName}", inputKeys: [${inputKeys.join(', ')}]. ` +
+        `Consider adding a new pattern to GEMINI_TOOL_PATTERNS if this tool appears frequently.`
+      );
+    }
+
     return toolName;
   }
 }
diff --git a/src/gemini/runGemini.ts b/src/gemini/runGemini.ts
@@ -40,11 +40,10 @@ import { GeminiDiffProcessor } from '@/gemini/utils/diffProcessor';
 import type { GeminiMode, CodexMessagePayload } from '@/gemini/types';
 import type { PermissionMode } from '@/api/types';
 import { GEMINI_MODEL_ENV, DEFAULT_GEMINI_MODEL, CHANGE_TITLE_INSTRUCTION } from '@/gemini/constants';
-import { 
-  readGeminiLocalConfig, 
-  determineGeminiModel, 
+import {
+  readGeminiLocalConfig,
   saveGeminiModelToConfig,
-  getInitialGeminiModel 
+  getInitialGeminiModel
 } from '@/gemini/utils/config';
 import {
   parseOptionsFromText,
@@ -137,17 +136,45 @@ export async function runGemini(opts: {
   // Permission handler declared here so it can be updated in onSessionSwap callback
   // (assigned later after Happy server setup)
   let permissionHandler: GeminiPermissionHandler;
+
+  // Session swap synchronization to prevent race conditions during message processing
+  // When a swap is requested during processing, it's queued and applied after the current cycle
+  let isProcessingMessage = false;
+  let pendingSessionSwap: ApiSessionClient | null = null;
+
+  /**
+   * Apply a pending session swap. Called between message processing cycles.
+   * This ensures session swaps happen at safe points, not during message processing.
+   */
+  const applyPendingSessionSwap = () => {
+    if (pendingSessionSwap) {
+      logger.debug('[gemini] Applying pending session swap');
+      session = pendingSessionSwap;
+      if (permissionHandler) {
+        permissionHandler.updateSession(pendingSessionSwap);
+      }
+      pendingSessionSwap = null;
+    }
+  };
+
   const { session: initialSession, reconnectionHandle } = setupOfflineReconnection({
     api,
     sessionTag,
     metadata,
     state,
     response,
     onSessionSwap: (newSession) => {
-      session = newSession;
-      // Update permission handler with new session to avoid stale reference
-      if (permissionHandler) {
-        permissionHandler.updateSession(newSession);
+      // If we're processing a message, queue the swap for later
+      // This prevents race conditions where session changes mid-processing
+      if (isProcessingMessage) {
+        logger.debug('[gemini] Session swap requested during message processing - queueing');
+        pendingSessionSwap = newSession;
+      } else {
+        // Safe to swap immediately
+        session = newSession;
+        if (permissionHandler) {
+          permissionHandler.updateSession(newSession);
+        }
       }
     }
   });
@@ -911,10 +938,10 @@ export async function runGemini(opts: {
           await geminiBackend.dispose();
           geminiBackend = null;
         }
-        
+
         // Create new backend with new model
         const modelToUse = message.mode?.model === undefined ? undefined : (message.mode.model || null);
-        geminiBackend = createGeminiBackend({
+        const backendResult = createGeminiBackend({
           cwd: process.cwd(),
           mcpServers,
           permissionHandler,
@@ -924,16 +951,14 @@ export async function runGemini(opts: {
           // If explicitly null, will skip local config and use env/default
           model: modelToUse,
         });
-        
+        geminiBackend = backendResult.backend;
+
         // Set up message handler again
         setupGeminiMessageHandler(geminiBackend);
-        
-        // Start new session
-        // Determine actual model that will be used (from backend creation logic)
-        // Replicate backend logic: message model > env var > local config > default
-        const localConfigForModel = readGeminiLocalConfig();
-        const actualModel = determineGeminiModel(modelToUse, localConfigForModel);
-        logger.debug(`[gemini] Model change - modelToUse=${modelToUse}, actualModel=${actualModel}`);
+
+        // Use model from factory result (single source of truth - no duplicate resolution)
+        const actualModel = backendResult.model;
+        logger.debug(`[gemini] Model change - modelToUse=${modelToUse}, actualModel=${actualModel} (from ${backendResult.modelSource})`);
         
         // Update conversation history with new model
         conversationHistory.setCurrentModel(actualModel);
@@ -961,12 +986,15 @@ export async function runGemini(opts: {
       const userMessageToShow = message.mode?.originalUserMessage || message.message;
       messageBuffer.addMessage(userMessageToShow, 'user');
 
+      // Mark that we're processing a message to synchronize session swaps
+      isProcessingMessage = true;
+
       try {
         if (first || !wasSessionCreated) {
           // First message or session not created yet - create backend and start session
           if (!geminiBackend) {
             const modelToUse = message.mode?.model === undefined ? undefined : (message.mode.model || null);
-            geminiBackend = createGeminiBackend({
+            const backendResult = createGeminiBackend({
               cwd: process.cwd(),
               mcpServers,
               permissionHandler,
@@ -976,25 +1004,14 @@ export async function runGemini(opts: {
               // If explicitly null, will skip local config and use env/default
               model: modelToUse,
             });
-            
+            geminiBackend = backendResult.backend;
+
             // Set up message handler
             setupGeminiMessageHandler(geminiBackend);
-            
-            // Determine actual model that will be used
-            // Backend will determine model from: message model > env var > local config > default
-            // We need to replicate this logic here to show correct model in UI
-            const localConfigForModel = readGeminiLocalConfig();
-            const actualModel = determineGeminiModel(modelToUse, localConfigForModel);
-            
-            const modelSource = modelToUse !== undefined 
-              ? 'message' 
-              : process.env[GEMINI_MODEL_ENV] 
-                ? 'env-var' 
-                : localConfigForModel.model 
-                  ? 'local-config' 
-                  : 'default';
-            
-            logger.debug(`[gemini] Backend created, model will be: ${actualModel} (from ${modelSource})`);
+
+            // Use model from factory result (single source of truth - no duplicate resolution)
+            const actualModel = backendResult.model;
+            logger.debug(`[gemini] Backend created, model will be: ${actualModel} (from ${backendResult.modelSource})`);
             logger.debug(`[gemini] Calling updateDisplayedModel with: ${actualModel}`);
             updateDisplayedModel(actualModel, false); // Don't save - this is backend initialization
             
@@ -1258,7 +1275,11 @@ export async function runGemini(opts: {
         
         // Use same logic as Codex - emit ready if idle (no pending operations, no queue)
         emitReadyIfIdle();
-        
+
+        // Message processing complete - safe to apply any pending session swap
+        isProcessingMessage = false;
+        applyPendingSessionSwap();
+
         logger.debug(`[gemini] Main loop: turn completed, continuing to next iteration (queue size: ${messageQueue.size()})`);
       }
     }
diff --git a/src/gemini/utils/conversationHistory.ts b/src/gemini/utils/conversationHistory.ts
@@ -46,37 +46,76 @@ export class ConversationHistory {
     this.currentModel = model;
   }
 
+  /**
+   * Check if content is a duplicate of the last message with the same role.
+   * Deduplication prevents inflating history when the same message is sent multiple times.
+   */
+  private isDuplicate(role: 'user' | 'assistant', content: string): boolean {
+    if (this.messages.length === 0) return false;
+
+    // Find the last message with the same role
+    for (let i = this.messages.length - 1; i >= 0; i--) {
+      const msg = this.messages[i];
+      if (msg.role === role) {
+        // Check if content matches (normalize whitespace for comparison)
+        const normalizedNew = content.trim().replace(/\s+/g, ' ');
+        const normalizedExisting = msg.content.replace(/\s+/g, ' ');
+        return normalizedNew === normalizedExisting;
+      }
+    }
+
+    return false;
+  }
+
   /**
    * Add a user message to history
+   * Skips duplicate messages to prevent history inflation
    */
   addUserMessage(content: string): void {
     if (!content.trim()) return;
-    
+
+    const trimmedContent = content.trim();
+
+    // Skip duplicate messages
+    if (this.isDuplicate('user', trimmedContent)) {
+      logger.debug(`[ConversationHistory] Skipping duplicate user message (${trimmedContent.length} chars)`);
+      return;
+    }
+
     this.messages.push({
       role: 'user',
-      content: content.trim(),
+      content: trimmedContent,
       timestamp: Date.now(),
     });
-    
+
     this.trimHistory();
-    logger.debug(`[ConversationHistory] Added user message (${content.length} chars), total: ${this.messages.length}`);
+    logger.debug(`[ConversationHistory] Added user message (${trimmedContent.length} chars), total: ${this.messages.length}`);
   }
 
   /**
    * Add an assistant response to history
+   * Skips duplicate messages to prevent history inflation
    */
   addAssistantMessage(content: string): void {
     if (!content.trim()) return;
-    
+
+    const trimmedContent = content.trim();
+
+    // Skip duplicate messages
+    if (this.isDuplicate('assistant', trimmedContent)) {
+      logger.debug(`[ConversationHistory] Skipping duplicate assistant message (${trimmedContent.length} chars)`);
+      return;
+    }
+
     this.messages.push({
       role: 'assistant',
-      content: content.trim(),
+      content: trimmedContent,
       timestamp: Date.now(),
       model: this.currentModel,
     });
-    
+
     this.trimHistory();
-    logger.debug(`[ConversationHistory] Added assistant message (${content.length} chars), total: ${this.messages.length}`);
+    logger.debug(`[ConversationHistory] Added assistant message (${trimmedContent.length} chars), total: ${this.messages.length}`);
   }
 
   /**
