From 218d0cf6c63e2d4a5e0b56a5ae061de4b6e861dc Mon Sep 17 00:00:00 2001 From: Tam Nhu Tran Date: Wed, 29 Apr 2026 17:57:11 -0400 Subject: [PATCH] fix(cliproxy): sanitize invalid tool name chars and disambiguate collisions sanitizeToolName() fallback now forces valid characters (replaces unsupported chars with underscores, ensures leading letter/underscore) instead of only truncating. registerTools() disambiguates colliding sanitized names with numeric suffixes so restoreToolUse() always maps back to the correct original. --- .../__tests__/tool-name-mapper.test.js | 60 ++++++++++++++++++- .../__tests__/tool-name-sanitizer.test.js | 35 ++++++++++- src/cliproxy/ai-providers/tool-name-mapper.ts | 60 ++++++++++++------- .../ai-providers/tool-name-sanitizer.ts | 30 +++++++++- 4 files changed, 159 insertions(+), 26 deletions(-) diff --git a/src/cliproxy/ai-providers/__tests__/tool-name-mapper.test.js b/src/cliproxy/ai-providers/__tests__/tool-name-mapper.test.js index 7758e3a9..095c89c7 100644 --- a/src/cliproxy/ai-providers/__tests__/tool-name-mapper.test.js +++ b/src/cliproxy/ai-providers/__tests__/tool-name-mapper.test.js @@ -4,7 +4,7 @@ const assert = require('assert'); -const { ToolNameMapper } = require('../../../../dist/cliproxy/tool-name-mapper'); +const { ToolNameMapper } = require('../../../../dist/cliproxy/ai-providers/tool-name-mapper'); describe('Tool Name Mapper', () => { describe('registerTools', () => { @@ -243,4 +243,62 @@ describe('Tool Name Mapper', () => { assert.strictEqual(mapper.restoreName('foo__bar'), 'foo__bar'); }); }); + + describe('collision handling', () => { + it('disambiguates when sanitized name collides with unchanged name', () => { + const mapper = new ToolNameMapper(); + // 'abc__def__def' sanitizes to 'abc__def' (changed) + // 'abc__def' is unchanged but collides with the sanitized name + const tools = [{ name: 'abc__def__def' }, { name: 'abc__def' }]; + + const result = mapper.registerTools(tools); + + assert.strictEqual(result[0].name, 'abc__def'); + assert.strictEqual(result[1].name, 'abc__def_2'); + assert.strictEqual(mapper.hasCollisions(), true); + }); + + it('disambiguates different originals that sanitize to the same name', () => { + const mapper = new ToolNameMapper(); + const tools = [{ name: 'abc__def__def' }, { name: 'abc__def' }]; + + const result = mapper.registerTools(tools); + + // 'abc__def__def' → 'abc__def' (deduped) + // 'abc__def' → 'abc__def_2' (disambiguated, unchanged but collides) + assert.strictEqual(result[0].name, 'abc__def'); + assert.strictEqual(result[1].name, 'abc__def_2'); + }); + + it('restores both colliding tools to correct originals', () => { + const mapper = new ToolNameMapper(); + const tools = [{ name: 'a__b__b' }, { name: 'a__b' }]; + + mapper.registerTools(tools); + + const content = [ + { type: 'tool_use', id: '1', name: 'a__b', input: {} }, + { type: 'tool_use', id: '2', name: 'a__b_2', input: {} }, + ]; + + const restored = mapper.restoreToolUse(content); + + assert.strictEqual(restored[0].name, 'a__b__b'); + assert.strictEqual(restored[1].name, 'a__b'); + }); + + it('records collision metadata for logging', () => { + const mapper = new ToolNameMapper(); + const tools = [{ name: 'x__y__y' }, { name: 'x__y' }]; + + mapper.registerTools(tools); + + assert.strictEqual(mapper.hasCollisions(), true); + const collisions = mapper.getCollisions(); + assert.strictEqual(collisions.length, 1); + assert.strictEqual(collisions[0].base, 'x__y'); + assert.ok(collisions[0].originals.includes('x__y__y')); + assert.ok(collisions[0].originals.includes('x__y')); + }); + }); }); diff --git a/src/cliproxy/ai-providers/__tests__/tool-name-sanitizer.test.js b/src/cliproxy/ai-providers/__tests__/tool-name-sanitizer.test.js index 342ddbe8..5e6fa12e 100644 --- a/src/cliproxy/ai-providers/__tests__/tool-name-sanitizer.test.js +++ b/src/cliproxy/ai-providers/__tests__/tool-name-sanitizer.test.js @@ -10,7 +10,7 @@ const { smartTruncate, sanitizeToolName, GEMINI_MAX_TOOL_NAME_LENGTH, -} = require('../../../../dist/cliproxy/tool-name-sanitizer'); +} = require('../../../../dist/cliproxy/ai-providers/tool-name-sanitizer'); describe('Tool Name Sanitizer', () => { describe('GEMINI_MAX_TOOL_NAME_LENGTH', () => { @@ -170,5 +170,38 @@ describe('Tool Name Sanitizer', () => { assert.strictEqual(result.changed, true); assert.strictEqual(result.sanitized, 'gitmcp__plus-pro-components'); }); + + it('forces valid characters when name contains spaces', () => { + const result = sanitizeToolName('has space'); + + assert.strictEqual(result.sanitized, 'has_space'); + assert.strictEqual(result.changed, true); + assert.ok(isValidToolName(result.sanitized)); + }); + + it('forces valid characters when name starts with digit', () => { + const result = sanitizeToolName('123start'); + + assert.ok(result.sanitized.startsWith('_')); + assert.strictEqual(result.changed, true); + assert.ok(isValidToolName(result.sanitized)); + }); + + it('forces valid characters when name contains symbols', () => { + const result = sanitizeToolName('tool@name#here'); + + assert.ok(!result.sanitized.includes('@')); + assert.ok(!result.sanitized.includes('#')); + assert.strictEqual(result.changed, true); + assert.ok(isValidToolName(result.sanitized)); + }); + + it('produces valid name for digit-starting long name', () => { + const name = '1' + 'a'.repeat(100); + const result = sanitizeToolName(name); + + assert.ok(isValidToolName(result.sanitized), `Expected valid name, got: ${result.sanitized}`); + assert.ok(result.sanitized.length <= 64); + }); }); }); diff --git a/src/cliproxy/ai-providers/tool-name-mapper.ts b/src/cliproxy/ai-providers/tool-name-mapper.ts index 6ca21bb6..f88a7659 100644 --- a/src/cliproxy/ai-providers/tool-name-mapper.ts +++ b/src/cliproxy/ai-providers/tool-name-mapper.ts @@ -41,8 +41,12 @@ export interface SanitizationChange { /** Record of a hash collision */ export interface HashCollision { - sanitized: string; + /** The base sanitized name before disambiguation */ + base: string; + /** All original names that collided */ originals: string[]; + /** The sanitized name used (after disambiguation) */ + sanitized: string; } /** @@ -63,7 +67,8 @@ export class ToolNameMapper { /** * Register tools and sanitize their names. - * Stores mapping for later restoration. + * Stores mapping for later restoration. On collision (two originals + * sanitizing to the same name), disambiguates with a numeric suffix. * * @param tools Array of tool definitions * @returns Array of tools with sanitized names @@ -71,36 +76,49 @@ export class ToolNameMapper { registerTools(tools: Tool[]): Tool[] { return tools.map((tool) => { const result: SanitizeResult = sanitizeToolName(tool.name); + let finalName = result.sanitized; - 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], - }); + // Check for collision regardless of whether the name changed. + // An unchanged name can still collide with a previously sanitized name. + const existingOriginal = this.mapping.get(finalName); + if (existingOriginal && existingOriginal !== tool.name) { + // Record collision for logging + const existing = this.collisions.find((c) => c.base === finalName); + if (existing) { + if (!existing.originals.includes(tool.name)) { + existing.originals.push(tool.name); } + } else { + this.collisions.push({ + base: finalName, + originals: [existingOriginal, tool.name], + sanitized: finalName, + }); } - // Store mapping: sanitized → original - this.mapping.set(result.sanitized, tool.name); + // Disambiguate: append _N suffix to make unique + let suffix = 2; + let disambiguated: string; + do { + disambiguated = `${finalName}_${suffix}`; + suffix++; + } while (this.mapping.has(disambiguated) && this.mapping.get(disambiguated) !== tool.name); + + finalName = disambiguated; + } + + // Store mapping if name changed or was disambiguated + if (finalName !== tool.name) { + this.mapping.set(finalName, tool.name); this.changes.push({ original: tool.name, - sanitized: result.sanitized, + sanitized: finalName, }); } return { ...tool, - name: result.sanitized, + name: finalName, }; }); } diff --git a/src/cliproxy/ai-providers/tool-name-sanitizer.ts b/src/cliproxy/ai-providers/tool-name-sanitizer.ts index f8e48611..754c5e58 100644 --- a/src/cliproxy/ai-providers/tool-name-sanitizer.ts +++ b/src/cliproxy/ai-providers/tool-name-sanitizer.ts @@ -100,13 +100,34 @@ export function smartTruncate(name: string, maxLen: number = GEMINI_MAX_TOOL_NAM return `${prefix}_${hash}`; } +/** + * Force a string to comply with Gemini tool name character rules. + * Replaces invalid characters with underscores and ensures the name + * starts with a letter or underscore. + */ +function forceValidChars(name: string): string { + // Replace any character not in [a-zA-Z0-9_.:/-] with underscore + let fixed = name.replace(/[^a-zA-Z0-9_.:/-]/g, '_'); + + // Collapse multiple consecutive underscores from replacement + fixed = fixed.replace(/_+/g, '_'); + + // Ensure starts with letter or underscore + if (fixed.length > 0 && /^[0-9]/.test(fixed)) { + fixed = '_' + fixed; + } + + return fixed || '_'; +} + /** * Sanitize a tool name to comply with Gemini API constraints. * * Process: * 1. Remove duplicate segments (always, as duplicates are likely unintentional) * 2. Truncate with hash if >64 chars - * 3. Return result with changed flag + * 3. Force valid characters if still invalid + * 4. Truncate again if forced-valid name exceeds limit * * @param name The original tool name * @returns Sanitization result with sanitized name and changed flag @@ -121,9 +142,12 @@ export function sanitizeToolName(name: string): SanitizeResult { sanitized = smartTruncate(sanitized); } - // Step 3: If still invalid after sanitization, apply truncation to original + // Step 3: If still invalid, fix characters and truncate if needed if (!isValidToolName(sanitized)) { - sanitized = smartTruncate(name); + sanitized = forceValidChars(sanitized); + if (sanitized.length > GEMINI_MAX_TOOL_NAME_LENGTH) { + sanitized = smartTruncate(sanitized); + } } return {