mirror of
https://github.com/tiennm99/ccs.git
synced 2026-10-12 16:15:13 +00:00
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
This commit is contained in:
1 parent
149dfc3d6d
commit
98b7f6f454
3 files changed
+63
-3
No files matched your search
@@ -844,7 +844,9 @@ export async function execClaudeWithCLIProxy(
|
|||||||
|
|
||||||
// Tool sanitization proxy - applies to ALL CLIProxy providers.
|
// Tool sanitization proxy - applies to ALL CLIProxy providers.
|
||||||
// Sanitizes MCP tool names exceeding Gemini's 64-char limit.
|
// 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 toolSanitizationProxy: ToolSanitizationProxy | null = null;
|
||||||
let toolSanitizationPort: number | null = null;
|
let toolSanitizationPort: number | null = null;
|
||||||
|
|
||||||
|
|||||||
@@ -39,6 +39,12 @@ export interface SanitizationChange {
|
|||||||
sanitized: string;
|
sanitized: string;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/** Record of a hash collision */
|
||||||
|
export interface HashCollision {
|
||||||
|
sanitized: string;
|
||||||
|
originals: string[];
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Bidirectional mapper for tool name sanitization.
|
* Bidirectional mapper for tool name sanitization.
|
||||||
*
|
*
|
||||||
@@ -52,6 +58,9 @@ export class ToolNameMapper {
|
|||||||
/** List of all changes made during registration */
|
/** List of all changes made during registration */
|
||||||
private changes: SanitizationChange[] = [];
|
private changes: SanitizationChange[] = [];
|
||||||
|
|
||||||
|
/** List of detected hash collisions */
|
||||||
|
private collisions: HashCollision[] = [];
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Register tools and sanitize their names.
|
* Register tools and sanitize their names.
|
||||||
* Stores mapping for later restoration.
|
* Stores mapping for later restoration.
|
||||||
@@ -64,6 +73,23 @@ export class ToolNameMapper {
|
|||||||
const result: SanitizeResult = sanitizeToolName(tool.name);
|
const result: SanitizeResult = sanitizeToolName(tool.name);
|
||||||
|
|
||||||
if (result.changed) {
|
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
|
// Store mapping: sanitized → original
|
||||||
this.mapping.set(result.sanitized, tool.name);
|
this.mapping.set(result.sanitized, tool.name);
|
||||||
this.changes.push({
|
this.changes.push({
|
||||||
@@ -143,11 +169,27 @@ export class ToolNameMapper {
|
|||||||
return this.changes.length;
|
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 all mappings. Call between requests.
|
||||||
*/
|
*/
|
||||||
clear(): void {
|
clear(): void {
|
||||||
this.mapping.clear();
|
this.mapping.clear();
|
||||||
this.changes = [];
|
this.changes = [];
|
||||||
|
this.collisions = [];
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -13,6 +13,7 @@ import * as http from 'http';
|
|||||||
import * as https from 'https';
|
import * as https from 'https';
|
||||||
import * as fs from 'fs';
|
import * as fs from 'fs';
|
||||||
import * as path from 'path';
|
import * as path from 'path';
|
||||||
|
import * as os from 'os';
|
||||||
import { URL } from 'url';
|
import { URL } from 'url';
|
||||||
import { ToolNameMapper, type Tool, type ContentBlock } from './tool-name-mapper';
|
import { ToolNameMapper, type Tool, type ContentBlock } from './tool-name-mapper';
|
||||||
import { getCcsDir } from '../utils/config-manager';
|
import { getCcsDir } from '../utils/config-manager';
|
||||||
@@ -64,9 +65,13 @@ export class ToolSanitizationProxy {
|
|||||||
if (!fs.existsSync(logsDir)) {
|
if (!fs.existsSync(logsDir)) {
|
||||||
fs.mkdirSync(logsDir, { recursive: true });
|
fs.mkdirSync(logsDir, { recursive: true });
|
||||||
}
|
}
|
||||||
} catch {
|
} catch (err) {
|
||||||
// Fallback to temp directory if logs dir creation fails
|
// 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');
|
return path.join(os.tmpdir(), 'tool-sanitization-proxy.log');
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -213,6 +218,17 @@ export class ToolSanitizationProxy {
|
|||||||
}
|
}
|
||||||
this.log(`Sanitized ${changes.length} tool name(s)`);
|
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
|
// Check if streaming is requested
|
||||||
|
|||||||
Reference in new issue
Block a user