From a2591e03a942ae244b50539d4b9d26da3a0b3d58 Mon Sep 17 00:00:00 2001
From: yuchou87 <yuchou87@gmail.com>
Date: Sun, 1 Mar 2026 12:00:26 +0800
Subject: [PATCH] fix: improve MCP tool name collision safety and registry
 overwrite warning

- MCPTool.Name(): append FNV-32a hash of original (unsanitized) server+tool
  names whenever sanitization is lossy or total length exceeds 64 chars,
  ensuring names that differ only in disallowed characters remain distinct
- ToolRegistry.Register(): emit warn log when a tool registration overwrites
  an existing tool with the same name, making collisions observable
- scripts/test-docker-mcp.sh: switch shebang from #/bin/bash /Users/yuchou/Work/klook-calendar/klook-google-cal-sync/src/googlecalconversrv/bin/start.sh to #  for portability on minimal distros and Nix environments
---
 pkg/tools/mcp_tool.go      | 30 ++++++++++++++++++++++++++++--
 pkg/tools/registry.go      |  7 ++++++-
 scripts/test-docker-mcp.sh |  2 +-
 3 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/pkg/tools/mcp_tool.go b/pkg/tools/mcp_tool.go
index 1bdf248f7..6e53cf354 100644
--- a/pkg/tools/mcp_tool.go
+++ b/pkg/tools/mcp_tool.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"hash/fnv"
 	"strings"
 
 	"github.com/modelcontextprotocol/go-sdk/mcp"
@@ -90,12 +91,37 @@ func sanitizeIdentifierComponent(s string) string {
 	return result
 }
 
-// Name returns the tool name, prefixed with the server name
+// Name returns the tool name, prefixed with the server name.
+// The total length is capped at 64 characters (OpenAI-compatible API limit).
+// A short hash of the original (unsanitized) server and tool names is appended
+// whenever sanitization is lossy or the name is truncated, ensuring that two
+// names which differ only in disallowed characters remain distinct after sanitization.
 func (t *MCPTool) Name() string {
 	// Prefix with server name to avoid conflicts, and sanitize components
 	sanitizedServer := sanitizeIdentifierComponent(t.serverName)
 	sanitizedTool := sanitizeIdentifierComponent(t.tool.Name)
-	return fmt.Sprintf("mcp_%s_%s", sanitizedServer, sanitizedTool)
+	full := fmt.Sprintf("mcp_%s_%s", sanitizedServer, sanitizedTool)
+
+	// Check if sanitization was lossless (only lowercasing, no char replacement/truncation)
+	lossless := strings.ToLower(t.serverName) == sanitizedServer &&
+		strings.ToLower(t.tool.Name) == sanitizedTool
+
+	const maxTotal = 64
+	if lossless && len(full) <= maxTotal {
+		return full
+	}
+
+	// Sanitization was lossy or name too long: append hash of the ORIGINAL names
+	// (not the sanitized names) so different originals always yield different hashes.
+	h := fnv.New32a()
+	_, _ = h.Write([]byte(t.serverName + "\x00" + t.tool.Name))
+	suffix := fmt.Sprintf("%08x", h.Sum32()) // 8 chars
+
+	base := full
+	if len(base) > maxTotal-9 {
+		base = strings.TrimRight(full[:maxTotal-9], "_")
+	}
+	return base + "_" + suffix
 }
 
 // Description returns the tool description
diff --git a/pkg/tools/registry.go b/pkg/tools/registry.go
index d37a093a8..0ba983e02 100644
--- a/pkg/tools/registry.go
+++ b/pkg/tools/registry.go
@@ -25,7 +25,12 @@ func NewToolRegistry() *ToolRegistry {
 func (r *ToolRegistry) Register(tool Tool) {
 	r.mu.Lock()
 	defer r.mu.Unlock()
-	r.tools[tool.Name()] = tool
+	name := tool.Name()
+	if _, exists := r.tools[name]; exists {
+		logger.WarnCF("tools", "Tool registration overwrites existing tool",
+			map[string]any{"name": name})
+	}
+	r.tools[name] = tool
 }
 
 func (r *ToolRegistry) Get(name string) (Tool, bool) {
diff --git a/scripts/test-docker-mcp.sh b/scripts/test-docker-mcp.sh
index 4eb77dceb..9d582ffa0 100755
--- a/scripts/test-docker-mcp.sh
+++ b/scripts/test-docker-mcp.sh
@@ -1,4 +1,4 @@
-#!/bin/bash
+#!/bin/sh
 # Test script for MCP tools in Docker (full-featured image)
 
 set -e