mcp: enforce 64-char tool name limit after server ID prefixing

[birdayz]

What

Enforce the 64-character tool name limit in the MCP client's namespaceTool() by mangling names that exceed the limit after the serverID__ prefix is applied.

Why

namespaceTool() concatenates serverID__toolName without checking length. LLM providers (at minimum Bedrock) reject tool names exceeding 64 characters. protoc-gen-go-mcp already mangles proto-generated names to 64 chars, but the server ID prefix pushes them right back over:

• servicenow-catalog__delete_api_sn_sc_servicecatalog_cart_by_sys_id_empty (73 chars)
• servicenow-catalog__post_api_sn_sc_servicecatalog_items_by_sys_id_versioning_checkout (85 chars)

Implementation details

Reuses the same mangling algorithm from protoc-gen-go-mcp: SHA-256 the full name, take 10 base-36 chars as a deterministic hash prefix, keep as much of the tail (the most human-readable part) as fits within 64 chars.

Names within the limit pass through unchanged -- zero behavioral change for short names.

Since mangled names cannot be reversed via strings.TrimPrefix, toolWrapper now stores the original server tool name. ExecuteTool() looks it up from the wrapper, falling back to TrimPrefix for direct API callers.

References

AI-1106

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

_{Tip: disable this comment in your organization's Code Review settings.}

[birdayz]

@claude plx review

[claude]

Code Review

• Read changed files (tool/mcp/mangle.go, tool/mcp/mangle_test.go, tool/mcp/tools.go)
• Analyze the mangling algorithm for correctness
• Review tool wrapper changes for ExecuteTool lookup
• Check tests for completeness and edge cases

Overall this looks solid — small, surgical change that addresses a real provider-side problem with a deterministic, well-tested mangling scheme. The wrapper-based reverse lookup is the right call given mangled names are not trivially invertible. A few notes:

---

1. Entropy comment is off by ~18 bits (cosmetic but misleading)

tool/mcp/mangle.go:39:

// The hash is derived from the full original name, so the mapping is stable // and collision-resistant (~31 bits of entropy, birthday bound ~46k).

10 base36 chars carry up to log2(36^10) ≈ 51.7 bits. Even accounting for leading-digit bias in big.Int.Text(36) against SHA-256 input (256 - 40·log2(36) ≈ 49), real entropy is about ~49 bits, not 31. Birthday bound for 50% collision is ~2^24.5 ≈ ~24M, not 46k. 46k is the bound for ~31 bits, which would correspond to a 6-char base36 prefix — perhaps a leftover from an earlier variant. Worth correcting so the comment isn't reassuring readers with the wrong order of magnitude. Fix this →

2. Silent collision in prepareTools if two server names mangle to the same namespaced key

In tool/mcp/tools.go:228:

prepared[namespaced] = &preparedTool{...}

If two different mcpTool.Names ever collide under the mangle (astronomically unlikely with ~49 bits, but not impossible across a 50k-tool-population universe), one silently overwrites the other and one server tool becomes unreachable. Worth at least a debug/warn log here when prepared[namespaced] already exists, so this would be diagnosable rather than mysterious. Not a blocker.

3. ListTools doc is now slightly inaccurate

tool/mcp/tools.go:34:

// Tool names are always namespaced with serverID (e.g., "github__create-issue").

After this PR, names that exceed 64 chars no longer literally start with serverID__. Suggest:

// Tool names are namespaced with serverID (e.g., "github__create-issue").
// Names that would exceed 64 chars are deterministically mangled (a hash
// prefix replaces the head); the original server tool name is preserved
// internally and used when forwarding the call to the MCP server.

4. ExecuteTool's TrimPrefix fallback is now effectively dead for namespaced calls

tool/mcp/tools.go:80-91: when a caller comes from ListTools() (the documented path), wrapper is never nil and the TrimPrefix branch never runs. The branch is only reachable when (a) a tool was unregistered between ListTools and ExecuteTool, or (b) a caller hand-constructs a tool name. In case (a), looking up a stale name and prefix-stripping it to call a server tool that may also no longer exist seems worse than just erroring out (ErrToolNotFound or similar). I'd consider removing the fallback and returning an explicit error — but if you prefer the defensive behavior, that's fine; just add a one-line comment about why it exists.

5. mangleHeadIfTooLong UTF-8 boundary

tool/mcp/mangle.go:66: tail := name[len(name)-available:] will slice mid-rune if a tool name ever contains multi-byte UTF-8. MCP tool names are constrained to a pattern that excludes non-ASCII in practice, so this is theoretical, but a one-line note in the godoc ("assumes ASCII; callers in this package only pass server-id-prefixed MCP tool names") would prevent future misuse if someone repurposes the helper.

6. Tests — coverage is good, one suggestion

The unit tests cover the algorithm well (boundary, determinism, tail preservation, tiny maxLen). One gap: there's no test that exercises ExecuteTool against a mangled name end-to-end via the wrapper lookup. The change to serverToolName storage is the most behavior-sensitive part of this PR and isn't currently exercised by a unit test — could be covered with a fake clientImpl.tools map and a stub session, similar to existing patterns. Not required, but would lock in the contract.

7. Tiny nit

mangle.go:57-59: when maxLen <= len(hashPrefix), you return hashPrefix[:maxLen]. With maxLen very small (e.g., 5), this gives a result that contains no entropy from the tail. That's fine and tested, but it also means a 5-char maxLen is effectively useless for collision avoidance. Worth noting (in a comment) that maxLen should be ≥ ~12 to be meaningful — or document a sensible minimum.

---

Summary

LGTM with non-blocking comments. The two I'd actually act on are #1 (correct the entropy comment — it's misleading) and #3 (ListTools doc). #2/#4 are nice-to-haves, #5/#7 are godoc clarity.

View job run • Branch: jb/mangle-long-tool-names