From 6633bf456bcb9095a59e05e08741810af6030f39 Mon Sep 17 00:00:00 2001 From: Tam Nhu Tran Date: Wed, 29 Apr 2026 18:04:19 -0400 Subject: [PATCH] fix(cliproxy): handle all invalid prefixes and reverse-order collisions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit forceValidChars() now ensures leading char is letter or underscore (catches dot, hyphen, colon, slash — not just digits). registerTools() always registers in mapping so collisions are detected regardless of tool registration order. --- .../__tests__/tool-name-mapper.test.js | 31 +++++++++++++++++++ .../__tests__/tool-name-sanitizer.test.js | 24 ++++++++++++++ src/cliproxy/ai-providers/tool-name-mapper.ts | 7 +++-- .../ai-providers/tool-name-sanitizer.ts | 6 ++-- 4 files changed, 63 insertions(+), 5 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 095c89c7..f3f80e0f 100644 --- a/src/cliproxy/ai-providers/__tests__/tool-name-mapper.test.js +++ b/src/cliproxy/ai-providers/__tests__/tool-name-mapper.test.js @@ -300,5 +300,36 @@ describe('Tool Name Mapper', () => { assert.ok(collisions[0].originals.includes('x__y__y')); assert.ok(collisions[0].originals.includes('x__y')); }); + + it('detects collision when unchanged tool appears before sanitized one', () => { + const mapper = new ToolNameMapper(); + const tools = [ + { name: 'abc__def' }, // unchanged, but registered in mapping + { name: 'abc__def__def' }, // sanitizes to 'abc__def', collides + ]; + + 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('restores correctly for reverse-order collision', () => { + const mapper = new ToolNameMapper(); + const tools = [{ name: 'abc__def' }, { name: 'abc__def__def' }]; + + mapper.registerTools(tools); + + const content = [ + { type: 'tool_use', id: '1', name: 'abc__def', input: {} }, + { type: 'tool_use', id: '2', name: 'abc__def_2', input: {} }, + ]; + + const restored = mapper.restoreToolUse(content); + + assert.strictEqual(restored[0].name, 'abc__def'); + assert.strictEqual(restored[1].name, 'abc__def__def'); + }); }); }); 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 5e6fa12e..db608ad8 100644 --- a/src/cliproxy/ai-providers/__tests__/tool-name-sanitizer.test.js +++ b/src/cliproxy/ai-providers/__tests__/tool-name-sanitizer.test.js @@ -203,5 +203,29 @@ describe('Tool Name Sanitizer', () => { assert.ok(isValidToolName(result.sanitized), `Expected valid name, got: ${result.sanitized}`); assert.ok(result.sanitized.length <= 64); }); + + it('forces valid prefix for dot-starting name', () => { + const result = sanitizeToolName('.tool'); + + assert.ok(result.sanitized.startsWith('_'), `Expected _ prefix, got: ${result.sanitized}`); + assert.ok(isValidToolName(result.sanitized)); + assert.strictEqual(result.changed, true); + }); + + it('forces valid prefix for hyphen-starting name', () => { + const result = sanitizeToolName('-tool'); + + assert.ok(result.sanitized.startsWith('_'), `Expected _ prefix, got: ${result.sanitized}`); + assert.ok(isValidToolName(result.sanitized)); + assert.strictEqual(result.changed, true); + }); + + it('forces valid prefix for colon-starting name', () => { + const result = sanitizeToolName(':tool'); + + assert.ok(result.sanitized.startsWith('_'), `Expected _ prefix, got: ${result.sanitized}`); + assert.ok(isValidToolName(result.sanitized)); + assert.strictEqual(result.changed, true); + }); }); }); diff --git a/src/cliproxy/ai-providers/tool-name-mapper.ts b/src/cliproxy/ai-providers/tool-name-mapper.ts index f88a7659..c7b2cce7 100644 --- a/src/cliproxy/ai-providers/tool-name-mapper.ts +++ b/src/cliproxy/ai-providers/tool-name-mapper.ts @@ -107,9 +107,12 @@ export class ToolNameMapper { finalName = disambiguated; } - // Store mapping if name changed or was disambiguated + // Always register in mapping so later tools can detect collisions + // even when this tool's name was unchanged + this.mapping.set(finalName, tool.name); + + // Record change only if name actually changed if (finalName !== tool.name) { - this.mapping.set(finalName, tool.name); this.changes.push({ original: tool.name, sanitized: finalName, diff --git a/src/cliproxy/ai-providers/tool-name-sanitizer.ts b/src/cliproxy/ai-providers/tool-name-sanitizer.ts index 754c5e58..d7950c7a 100644 --- a/src/cliproxy/ai-providers/tool-name-sanitizer.ts +++ b/src/cliproxy/ai-providers/tool-name-sanitizer.ts @@ -103,7 +103,7 @@ export function smartTruncate(name: string, maxLen: number = GEMINI_MAX_TOOL_NAM /** * 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. + * starts with a letter or underscore (not a digit, dot, colon, etc.). */ function forceValidChars(name: string): string { // Replace any character not in [a-zA-Z0-9_.:/-] with underscore @@ -112,8 +112,8 @@ function forceValidChars(name: string): string { // Collapse multiple consecutive underscores from replacement fixed = fixed.replace(/_+/g, '_'); - // Ensure starts with letter or underscore - if (fixed.length > 0 && /^[0-9]/.test(fixed)) { + // Ensure starts with letter or underscore (not digit, dot, colon, hyphen, slash) + if (fixed.length > 0 && !/^[a-zA-Z_]/.test(fixed)) { fixed = '_' + fixed; }