From 98b7f6f4545daca18b3c76619bd7c6a6496cc1ed Mon Sep 17 00:00:00 2001
From: kaitranntt <kaitran.ntt@gmail.com>
Date: Thu, 29 Jan 2026 17:49:32 -0500
Subject: [PATCH] fix(cliproxy): address PR #399 review suggestions

- Move require('os') to top-level import
- Add debug logging for log dir creation failures
- Add hash collision detection and warning in ToolNameMapper
- Clarify proxy chain order comments in cliproxy-executor
---
 src/cliproxy/cliproxy-executor.ts       |  4 ++-
 src/cliproxy/tool-name-mapper.ts        | 42 +++++++++++++++++++++++++
 src/cliproxy/tool-sanitization-proxy.ts | 20 ++++++++++--
 3 files changed, 63 insertions(+), 3 deletions(-)

diff --git a/src/cliproxy/cliproxy-executor.ts b/src/cliproxy/cliproxy-executor.ts
index 3842ee6b..18e4faa4 100644
--- a/src/cliproxy/cliproxy-executor.ts
+++ b/src/cliproxy/cliproxy-executor.ts
@@ -844,7 +844,9 @@ export async function execClaudeWithCLIProxy(
 
   // Tool sanitization proxy - applies to ALL CLIProxy providers.
   // Sanitizes MCP tool names exceeding Gemini's 64-char limit.
-  // Chain: Claude CLI → ToolSanitizationProxy → [CodexReasoningProxy] → CLIProxy → Gemini
+  // Proxy chain order (request flow):
+  //   Claude CLI → ToolSanitizationProxy → [CodexReasoningProxy if codex] → CLIProxy → Backend
+  // Response flow is reversed, with each proxy transforming data back.
   let toolSanitizationProxy: ToolSanitizationProxy | null = null;
   let toolSanitizationPort: number | null = null;
 
diff --git a/src/cliproxy/tool-name-mapper.ts b/src/cliproxy/tool-name-mapper.ts
index 8cc67288..6ca21bb6 100644
--- a/src/cliproxy/tool-name-mapper.ts
+++ b/src/cliproxy/tool-name-mapper.ts
@@ -39,6 +39,12 @@ export interface SanitizationChange {
   sanitized: string;
 }
 
+/** Record of a hash collision */
+export interface HashCollision {
+  sanitized: string;
+  originals: string[];
+}
+
 /**
  * Bidirectional mapper for tool name sanitization.
  *
@@ -52,6 +58,9 @@ export class ToolNameMapper {
   /** List of all changes made during registration */
   private changes: SanitizationChange[] = [];
 
+  /** List of detected hash collisions */
+  private collisions: HashCollision[] = [];
+
   /**
    * Register tools and sanitize their names.
    * Stores mapping for later restoration.
@@ -64,6 +73,23 @@ export class ToolNameMapper {
       const result: SanitizeResult = sanitizeToolName(tool.name);
 
       if (result.changed) {
+        // Check for collision: sanitized name already maps to different original
+        const existingOriginal = this.mapping.get(result.sanitized);
+        if (existingOriginal && existingOriginal !== tool.name) {
+          // Record collision
+          const existing = this.collisions.find((c) => c.sanitized === result.sanitized);
+          if (existing) {
+            if (!existing.originals.includes(tool.name)) {
+              existing.originals.push(tool.name);
+            }
+          } else {
+            this.collisions.push({
+              sanitized: result.sanitized,
+              originals: [existingOriginal, tool.name],
+            });
+          }
+        }
+
         // Store mapping: sanitized → original
         this.mapping.set(result.sanitized, tool.name);
         this.changes.push({
@@ -143,11 +169,27 @@ export class ToolNameMapper {
     return this.changes.length;
   }
 
+  /**
+   * Check if any hash collisions were detected.
+   * Collisions occur when multiple original names map to the same sanitized name.
+   */
+  hasCollisions(): boolean {
+    return this.collisions.length > 0;
+  }
+
+  /**
+   * Get all detected hash collisions for logging/warning.
+   */
+  getCollisions(): HashCollision[] {
+    return [...this.collisions];
+  }
+
   /**
    * Clear all mappings. Call between requests.
    */
   clear(): void {
     this.mapping.clear();
     this.changes = [];
+    this.collisions = [];
   }
 }
diff --git a/src/cliproxy/tool-sanitization-proxy.ts b/src/cliproxy/tool-sanitization-proxy.ts
index 3e6164e8..5bfde38f 100644
--- a/src/cliproxy/tool-sanitization-proxy.ts
+++ b/src/cliproxy/tool-sanitization-proxy.ts
@@ -13,6 +13,7 @@ import * as http from 'http';
 import * as https from 'https';
 import * as fs from 'fs';
 import * as path from 'path';
+import * as os from 'os';
 import { URL } from 'url';
 import { ToolNameMapper, type Tool, type ContentBlock } from './tool-name-mapper';
 import { getCcsDir } from '../utils/config-manager';
@@ -64,9 +65,13 @@ export class ToolSanitizationProxy {
       if (!fs.existsSync(logsDir)) {
         fs.mkdirSync(logsDir, { recursive: true });
       }
-    } catch {
+    } catch (err) {
       // Fallback to temp directory if logs dir creation fails
-      const os = require('os');
+      if (this.debugMode) {
+        console.error(
+          `[tool-sanitization-proxy] Failed to create logs dir: ${(err as Error).message}`
+        );
+      }
       return path.join(os.tmpdir(), 'tool-sanitization-proxy.log');
     }
 
@@ -213,6 +218,17 @@ export class ToolSanitizationProxy {
           }
           this.log(`Sanitized ${changes.length} tool name(s)`);
         }
+
+        // Warn about hash collisions (multiple originals → same sanitized)
+        if (mapper.hasCollisions()) {
+          const collisions = mapper.getCollisions();
+          for (const collision of collisions) {
+            this.writeLog(
+              'warn',
+              `[tool-sanitization-proxy] Hash collision detected: ${collision.originals.join(', ')} → "${collision.sanitized}"`
+            );
+          }
+        }
       }
 
       // Check if streaming is requested