fix(mcp): address greptile review on short tool prefix

- server.py: drop the redundant server_id append in
  _get_filtered_mcp_servers_from_mcp_server_names. iter_known_server_prefixes
  already yields server_id unconditionally, so the manual append (and its
  misleading comment) was a no-op duplicate.
- utils.py: rewrite the SHORT_MCP_TOOL_PREFIX docstring to accurately
  describe the collision behaviour. The previous wording said collisions
  were 'cosmetic only', but a natural-hash collision IS a routing-correctness
  issue, which is precisely why we already added _assign_unique_short_prefix
  to rehash deterministically. The new comment cross-references that path.
- utils.py: restrict the first character of the short prefix to [A-Za-z]
  via a 52-char alphabet for position 0 only. The remaining two positions
  still use the full base62 alphabet. This keeps prefixes valid identifiers
  on every backend and gives 52*62*62 = 199_888 distinct prefixes (still
  comfortably more than any realistic deployment).
- tests: add coverage proving the first character of the prefix is always
  alphabetic across many server_ids and rehash attempts.

Co-authored-by: Mateo Wang <mateo-berri@users.noreply.github.com>

litellm/proxy/_experimental/mcp_server/server.py

@@ -714,11 +714,6 @@ if MCP_AVAILABLE:
                         match_list = [
                             s.lower() for s in iter_known_server_prefixes(server) if s
                         ]
-                        # Always accept server_id even if it isn't part of the
-                        # current prefix form (iter_known_server_prefixes only
-                        # yields it when no other identifier exists).
-                        if server.server_id:
-                            match_list.append(server.server_id.lower())
 
                         if server_or_group.lower() in match_list:
                             filtered_server[server.server_id] = server

litellm/proxy/_experimental/mcp_server/utils.py

@@ -21,27 +21,39 @@ MCP_TOOL_PREFIX_FORMAT = "{server_name}{separator}{tool_name}"
 # When LITELLM_USE_SHORT_MCP_TOOL_PREFIX is truthy the prefix attached to MCP
 # tool / prompt / resource / resource-template names switches from the
 # (potentially long) human-readable server name to a deterministic three
-# character base62 ID derived from the server's ``server_id``.
+# character ID derived from the server's ``server_id``.
 #
-# Why three characters and base62 ([0-9A-Za-z])?
-#   * 62**3 = 238_328 distinct IDs — the chance of a real local tool name
-#     happening to begin with the exact prefix LiteLLM assigned to a given
-#     MCP server is negligible in practice.
-#   * The IDs are short enough that prefixed tool names stay well under the
-#     60-character upper bound enforced by some model APIs (Anthropic etc.)
-#     even for long upstream tool names.
-#   * The mapping is deterministic (SHA-256 of ``server_id`` → first three
-#     base62 chars), which means the prefix is stable across processes,
-#     workers and restarts without any persistence layer.  Two servers with
-#     different ``server_id`` values can in principle hash to the same
-#     three chars, but for the reverse-lookup path we register every known
-#     form of the prefix anyway, so a collision only affects the cosmetic
-#     emitted name, not routing correctness.
+# Why three characters?
+#   * The first character is restricted to 52 alphabetic characters
+#     ([A-Za-z]) and the remaining two characters use the full base62
+#     alphabet ([0-9A-Za-z]).  That guarantees the prefix never starts
+#     with a digit so it remains a valid identifier for every model API
+#     (some providers historically required a leading alphabetic char).
+#   * 52 * 62 * 62 = 199_888 distinct IDs.  The chance of a real local
+#     tool name happening to begin with the exact prefix LiteLLM assigned
+#     to a given MCP server is negligible in practice.
+#   * The IDs are short enough that prefixed tool names stay well under
+#     the 60-character upper bound enforced by some model APIs (Anthropic
+#     etc.) even for long upstream tool names.
+#   * The mapping is deterministic (SHA-256 of ``server_id`` → three
+#     characters drawn from the alphabets above), so the prefix is stable
+#     across processes, workers and restarts without any persistence
+#     layer.  Two servers with different ``server_id`` values can in
+#     principle hash to the same three chars; that natural-hash collision
+#     IS a routing-correctness issue (the second registrant would otherwise
+#     have its tools misrouted to the first), so registration goes through
+#     ``MCPServerManager._assign_unique_short_prefix`` which rehashes with
+#     a deterministic attempt counter until it finds an unused prefix and
+#     caches the result on ``MCPServer.short_prefix``.  A collision is
+#     logged at INFO when it happens.
 #
 # This flag is intentionally opt-in for the first release so customers can
 # migrate.  It will become the default in a future release.
 SHORT_MCP_TOOL_PREFIX_LENGTH = 3
 _BASE62_ALPHABET = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
+# Subset of _BASE62_ALPHABET used for the *first* character only, to
+# guarantee the prefix never starts with a digit.
+_BASE52_ALPHA_ALPHABET = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
 
 
 def is_short_mcp_tool_prefix_enabled() -> bool:
@@ -55,15 +67,17 @@ def is_short_mcp_tool_prefix_enabled() -> bool:
 
 
 def compute_short_server_prefix(server_id: str, attempt: int = 0) -> str:
-    """Derive the deterministic three-character base62 prefix for a server.
+    """Derive the deterministic three-character prefix for a server.
 
     Uses SHA-256 of ``f"{server_id}#{attempt}"`` and folds the first eight
-    bytes into a base62 string.  Pass ``attempt > 0`` to rehash to a
-    different prefix when the natural hash collides with a prefix already
-    assigned to another server (see
-    ``MCPServerManager._assign_unique_short_prefix``).  An empty server_id
-    raises ValueError — short prefixes require a stable identifier to be
-    deterministic.
+    bytes into a fixed-length string whose first character is drawn from
+    ``_BASE52_ALPHA_ALPHABET`` (so the prefix never starts with a digit)
+    and whose remaining characters are drawn from the full base62
+    alphabet.  Pass ``attempt > 0`` to rehash to a different prefix when
+    the natural hash collides with a prefix already assigned to another
+    server (see ``MCPServerManager._assign_unique_short_prefix``).  An
+    empty ``server_id`` raises ``ValueError`` — short prefixes require a
+    stable identifier to be deterministic.
     """
     if not server_id:
         raise ValueError("compute_short_server_prefix requires a non-empty server_id")
@@ -71,10 +85,17 @@ def compute_short_server_prefix(server_id: str, attempt: int = 0) -> str:
     seed = server_id if attempt == 0 else f"{server_id}#{attempt}"
     digest = hashlib.sha256(seed.encode("utf-8")).digest()
     value = int.from_bytes(digest[:8], "big")
+
+    # Build chars from least-significant to most-significant; we reverse
+    # at the end so the first emitted char comes from the high-order
+    # bits of the digest (which is the position we constrain to be
+    # alphabetic).
     chars = []
-    for _ in range(SHORT_MCP_TOOL_PREFIX_LENGTH):
-        value, idx = divmod(value, len(_BASE62_ALPHABET))
-        chars.append(_BASE62_ALPHABET[idx])
+    for position in range(SHORT_MCP_TOOL_PREFIX_LENGTH):
+        is_first_char = position == SHORT_MCP_TOOL_PREFIX_LENGTH - 1
+        alphabet = _BASE52_ALPHA_ALPHABET if is_first_char else _BASE62_ALPHABET
+        value, idx = divmod(value, len(alphabet))
+        chars.append(alphabet[idx])
     return "".join(reversed(chars))
 
 

tests/test_litellm/proxy/_experimental/mcp_server/test_short_mcp_tool_prefix.py

@@ -57,6 +57,20 @@ class TestShortPrefixHelpers:
         assert len(prefix) == SHORT_MCP_TOOL_PREFIX_LENGTH
         assert prefix.isalnum() and prefix.isascii()
 
+    def test_short_prefix_first_char_is_alphabetic(self):
+        """The first char must be [A-Za-z] so the prefix is a valid identifier
+        on every model API (some providers historically required the first
+        character of a function name to be alphabetic)."""
+        # Sweep many server_ids and rehash attempts to give us coverage of
+        # every position the high-order bits can land on.
+        for i in range(200):
+            for attempt in range(4):
+                prefix = compute_short_server_prefix(f"server-{i}", attempt=attempt)
+                assert prefix[0].isalpha(), (
+                    f"prefix {prefix!r} for server-{i} (attempt={attempt}) "
+                    f"starts with a non-alphabetic character"
+                )
+
     def test_short_prefix_is_deterministic(self):
         assert compute_short_server_prefix("abc") == compute_short_server_prefix("abc")
         assert compute_short_server_prefix("abc") != compute_short_server_prefix("abd")