fix!(tools): align virtual-server tool names and aliases

[lucarlig]

🐛 Bug-fix PR

📌 Summary

Virtual servers now expose exact custom tool names while routing calls to upstream original names. Dataplane backends use stable gateway UUIDs so gateways with the same name or slug cannot overwrite each other. Tool-name validation now follows the MCP 2025-11-25 recommendation.

Companion dataplane consumer: contextforge-org/contextforge-data-plane#56

🔁 Reproduction Steps

1. Register two visible gateways whose names produce the same slug and attach both to one virtual server.
2. Publish the dataplane configuration. Previously, one backend replaced the other because the backend dictionary was keyed by slug.
3. Give an attached tool a custom name such as Public.Tool and list it through the virtual-server MCP endpoint. Previously, protocol-visible naming was derived from the URL-oriented slug instead of exposing the exact custom name.
4. Call the exact custom name through the virtual server. Previously, scoped lookup did not consistently resolve it back to the upstream original name.

🐞 Root Cause

• Dataplane publishing conflated mutable presentation slugs with backend identity.
• Server-scoped tool listing and invocation used computed or slugged names instead of separating the exposed custom name from the upstream original name.
• Tool-name validation reused URL-oriented restrictions rather than the MCP tool-name character set and length bound.

💡 Fix Description

• Key dataplane backend maps by immutable gateway UUID.
• Publish tool_name_aliases as {exposed_name: original_name} while retaining upstream originals in allowed_tool_names.
• Expose exact custom_name values for virtual-server tool listings and resolve scoped calls back to original_name.
• Validate names with ^[A-Za-z0-9_.-]+$ and enforce the 1–128 character bound.
• Add regression coverage for same-slug gateway collisions, exact aliases, scoped invocation, and validation boundaries.
• Warn when a virtual server exposes duplicate tool names and keep the first alias deterministically when two tools on one backend share an exposed name.

⚠️ Compatibility Note

Server-scoped MCP clients that cached the previously exposed namespaced tool names (<gateway-slug>-<tool-slug>) must re-list tools: scoped invocation now resolves the exact custom name instead. Clients that list-then-call are unaffected.

📏 Reviewability

• This PR has one clear purpose
• The linked issue is not labeled triage
• Unrelated bugs or improvements are tracked in separate issues/PRs
• Tests are included with the code they validate

🧪 Verification

Check	Command	Status
Affected unit suites	.venv/bin/python -m pytest -q tests/unit/mcpgateway/services/test_dataplane_publisher.py tests/unit/mcpgateway/services/test_metrics_exception_coverage.py tests/unit/mcpgateway/services/test_tool_service_coverage.py tests/unit/mcpgateway/services/test_tool_service.py tests/unit/mcpgateway/transports/test_streamablehttp_transport.py	✅ Passed
Ruff	make ruff	✅ Passed
Secret scan	make detect-secrets-scan	✅ Passed
GitHub Actions	Required PR checks	✅ Passed
Coverage ≥ 80%	make coverage	Not run locally
Container regression	Docker or Podman SSL run	Not run locally

📐 MCP Compliance (if relevant)

• Matches current MCP spec
• No breaking change to MCP clients

✅ Checklist

• Code formatted and commit hooks passed
• No secrets/credentials committed

[msureshkumar88]

Thanks for this one — the backend-keying fix and the custom-name exposure/alias resolution are both real, well-scoped fixes, and the test coverage for the primary scenarios (same-slug gateway collision, duplicate alias on one backend, scoped alias routing) is solid. A few things worth addressing before merge:

1. Missing regression test for a new ambiguity surface
_load_invocable_tools now OR-matches custom_name, name, and original_name for scoped lookups (previously 2 columns). I traced the existing multiple_found disambiguation logic and it does fail closed (raises ToolInvocationError("...ambiguous") when tied), so there's no correctness bug — but there's no test exercising the specific case where one tool's custom_name collides with another tool's name/original_name on the same server. Worth adding given this PR is what introduces the 3-way collision surface.

2. Stale doc reference
docs/docs/testing/acceptance.md (around the "Input Validation Testing" table) still asserts the old error message ("Tool name must start with a letter, number, or underscore...") and a 255-char length reference for the tool name field. Your own test-file changes confirm both are now incorrect for this validator path.

3. CHANGELOG entry
No entry added under [Unreleased], though the PR includes an explicit client-visible compatibility break (documented in the PR body's compatibility note). Might be worth adding one given repo convention for breaking changes.

4. Cache fully disabled for all server-scoped invocations (non-blocking, worth a follow-up issue)
tool_lookup_cache is now bypassed entirely (reads and writes) whenever server_id is set, to avoid cross-server cache-key collisions on custom_name (which isn't globally unique). That's the safe choice, but since virtual-server invocation is likely the primary traffic pattern, this removes caching from the hottest path. Scoping the cache key by (server_id, name) instead of disabling it outright would keep the correctness fix without the performance cost — could be a fast follow rather than blocking this PR.

Nothing here blocks the core approach — happy to re-review once 1–2 are addressed.

[lucarlig]

Thanks @msureshkumar88 — all review feedback and the server-scope CI regression are addressed in 8166ad96f, 307b13004, and cda0f163f; cac310577 merges the latest main and resolves the remaining conflict. Focused tests and lint/security checks pass; CI is rerunning.

[msureshkumar88]

Thanks for addressing the first round of feedback — the CHANGELOG entry, doc fix, and the new collision regression test all look good. Went through the branch again with fresh eyes and found two issues in the new server-scoped lookup path that are worth fixing before merge.

1. Disambiguation priority doesn't account for the new custom_name match dimension

_load_invocable_tools (tool_service.py:4651) now matches custom_name, .name (legacy gateway-slug-prefixed), or original_name. But the priority tier used to pick among multiple candidates in prepare_rust_mcp_tool_execution (tool_service.py:4218) and invoke_tool (tool_service.py:4951) still only grants priority 0 for an exact .name match:

name_priority = 0 if getattr(candidate, "name", None) == name else 1

Since custom_name accepts the same character set as the legacy slug name, a second tool's custom_name can be crafted to equal another tool's internal .name (e.g. "public-tool"). When that happens, the exact-custom_name match loses to the exact-.name match and the wrong tool executes silently — no ambiguity error. That undercuts the PR's own goal of resolving scoped calls to the exact custom name. The new regression test covers custom_name vs original_name ties but not custom_name vs .name, so this isn't caught.

Suggest giving exact custom_name matches priority 0 as well (or tying them with .name matches and failing closed on the tie), then adding a test for that specific collision.

2. zip(tools, serialized_tools) in main.py:11087 can misalign when MCP Apps filtering drops a tool

serialized_tools = serializer_func(tools)
for tool, payload in zip(tools, serialized_tools):
    ...

serializer_func calls filter_model_visible_tools() internally, which drops app-only tools when mcpgateway_mcp_apps_enabled=True. If a server's tool list mixes model-visible and app-only tools, serialized_tools ends up shorter than tools, and zip pairs by position — so a tool downstream of the dropped one gets stamped with the wrong custom_name. Dormant today (flag defaults off) but worth fixing since MCP Apps is an active feature area. A dict keyed by tool id (or applying the name override before filtering) would avoid the positional dependency. Also currently untested with a multi-tool list.

Non-blocking, for consideration:

• GET /servers/{server_id}/tools (main.py:4728) still returns the internal slug name rather than custom_name, while the RPC and streamable-HTTP paths now expose the exact custom name. Might be intentional (REST vs MCP-protocol semantics), but flagging in case it's a missed spot given the PR's stated goal of consistent custom-name exposure.
• The re.match → re.fullmatch change in validate_tool_name (validators.py:700) incidentally closes a real bypass — re.match + trailing $ let a trailing-newline value like "toolname\n" slip through validation with the newline intact. Worth a one-line CHANGELOG mention as a deliberate security hardening so it doesn't get reverted by accident later.

Happy to take another pass once 1–2 are in.

[lucarlig]

Thanks @msureshkumar88 — addressed the latest review and server-scope CI regression in cda0f163f; cac310577 merges the latest main and resolves the remaining conflict. Focused tests and checks pass, and CI is rerunning.

[msureshkumar88]

Both rounds of feedback look good now — the priority-0 fix for custom_name collisions (with the new regression test), the main.py zip pre-filtering fix, and the CHANGELOG/doc updates all check out. Nice work on top of that: the server-scoped cache got reinstated with proper (server_id, name) key scoping and full invalidation coverage, which goes beyond what the earlier feedback asked for.

One small thing to fix before merge, introduced by this PR's own validator message change:

Stale mapping in error_formatter.py

_get_user_message() (mcpgateway/utils/error_formatter.py:138) still keys on the old message:

"Tool name exceeds maximum length": f"{field.title()} is too long (maximum 255 characters)",

But validate_tool_name (validators.py:711) now raises "Tool name exceeds MCP spec limit of 128 characters (got X)". Since the match is substring-based (if pattern in technical_msg), the old key no longer matches anything real, so over-length tool names now fall through to the generic "Invalid name" fallback instead of the intended friendly message. Also the hardcoded 255 should be 128 to match the new limit.

Suggest updating the key to something like "Tool name exceeds MCP spec limit" and the count to 128 — one-line fix, same spot the PR already touched for the character-set message just above it.

Nothing else blocking — happy to approve once this is in.

[lucarlig]

Thanks @msureshkumar88 — fixed in 55c7cf24b: the friendly mapping now matches the MCP 128-character validator message, with a regression covering the real ToolCreate validation path. The 45 formatter tests, doctests, Ruff, Bandit, and commit hooks pass.

[msureshkumar88]

Did a fresh pass over the branch (post-55c7cf24b), re-checking the fixes from the last three rounds against the live code rather than just the diff, and looking for anything new across performance, security, refactoring, and test coverage.

Previously requested changes — all confirmed fixed:

• Missing 3-way collision regression test → added and passing.
• Stale acceptance.md doc reference → updated.
• Missing CHANGELOG entry → added, with the breaking-change note.
• Cache fully disabled for server-scoped invocations → reinstated with proper (server_id, name) key scoping and full invalidation coverage (registry_cache + tool_lookup_cache), including Redis set tracking. Verified ServerService.update_server/delete_server call invalidate_server() unconditionally after commit, so tool-association changes (the only path that mutates a server's tool set) are covered.
• custom_name vs .name disambiguation priority bug → fixed via _tool_name_match_priority(), applied consistently to both prepare_rust_mcp_tool_execution and invoke_tool, with a regression test proving the tie now fails closed (ToolInvocationError: ambiguous).
• zip() misalignment in main.py _handle_tools_list_rpc when MCP Apps filtering drops a tool → fixed by filtering before zipping, with a regression test covering the multi-tool case.
• Stale error_formatter.py mapping after the validator message change → fixed, key and length both updated to match validate_tool_name.

CI is green (30/30 checks).

New, non-blocking items found on this pass:

1. Cache-key delimiter relies on an external invariant (hardening note). ToolLookupCache._cache_key() builds f"server:{server_id}:{name}". This isn't exploitable today — server_id is always a colon-free UUID hex (db.py Tool.id / Server.id default to uuid.uuid4().hex), and tool names can never contain : per TOOL_NAME_PATTERN. But the cache layer itself doesn't enforce that separation — it depends entirely on those two invariants holding elsewhere. If tool-name validation is ever loosened again (as happened once already in this PR's history), a global (unscoped) lookup for a literal string like "server:<real-server-id>:<real-tool-name>" would collide with the internal representation of a legitimate scoped cache entry for that server/tool. Worth using a delimiter or key encoding that can't collide with either component regardless of validator state, so this doesn't have to be re-verified by hand every time the tool-name pattern changes.

2. Redundant/inexhaustive branch in the cache-set path. In both invoke_tool and prepare_rust_mcp_tool_execution (tool_service.py):

   if server_id:
       await tool_lookup_cache.set(name, cache_payload, gateway_id=gateway_id, server_id=server_id)
   elif server_id is None:
       await tool_lookup_cache.set(name, cache_payload, gateway_id=gateway_id)

   _cache_key() treats any falsy server_id (including "") as the global scope. This branch doesn't — an empty-string server_id satisfies neither arm, so the cache write is silently skipped. Not reachable today since every caller passes either None or a real ID, but elif server_id is None would be clearer (and match _cache_key's actual semantics) as a plain else.

3. Minor test-coverage asymmetry. The custom_name vs .name collision regression test (test_prepare_rust_mcp_tool_execution_rejects_server_custom_name_internal_name_collision) only exercises prepare_rust_mcp_tool_execution. invoke_tool received the identical _tool_name_match_priority() fix but has no direct collision test of its own — lower risk since both call sites share the same helper, but a future edit to just one of the two call sites wouldn't be caught by the existing test.

None of these block merge — flagging as follow-up/hardening items only. Nice work getting the alias-resolution and cache-scoping work through three rounds cleanly.

[msureshkumar88]

Ran a real-process e2e script against this branch (55c7cf24b) to validate the actual bug reproduction steps from the PR description — not unit tests, an actual booted gateway talking over HTTP to two real upstream MCP servers. Script: tests/live_gateway/e2e_pr5553_virtual_server_tool_aliases.sh (added in this run, not currently part of the PR diff).

Result: 20/20 checks passed. Approving.

Environment / config

• Real uvicorn process running mcpgateway.main:app, real SQLite file (fresh, migrated by the app's own bootstrap — no fixtures/seeding), real JWT auth (AUTH_REQUIRED=true).
• Two real MCP upstream servers, not stubs: mcp.server.fastmcp.FastMCP instances speaking streamable-HTTP, each exposing an echo(text) tool tagged with its own port so responses prove which upstream actually answered.
• EXPOSE_ERROR_DETAILS=true so validation-error assertions could check the actual message text, not just HTTP status.
• Admin JWT generated via create_jwt_token --admin (teams: null, is_admin: true → admin bypass per this repo's token-scoping model).

DATABASE_URL=sqlite:///$TMP/e2e.db
JWT_SECRET_KEY=<throwaway>
AUTH_ENCRYPTION_SECRET=<throwaway>
AUTH_REQUIRED=true MCPGATEWAY_UI_ENABLED=true MCPGATEWAY_ADMIN_API_ENABLED=true EXPOSE_ERROR_DETAILS=true
Gateway: 127.0.0.1:18097
Upstream A (FastMCP echo): 127.0.0.1:18091/mcp
Upstream B (FastMCP echo): 127.0.0.1:18092/mcp

What it drove, and what it proved

1. Tool-name validation now follows MCP 2025-11-25, not SEP-986

• POST /tools name "tool/name" → 422, "Tool name may contain only letters, numbers, underscores, hyphens, and periods" (slash, previously allowed, now rejected).
• POST /tools name "-e2e.tool" → 200 (leading hyphen/dot, previously rejected, now allowed).
• POST /tools name of 129 chars → 422, "Tool name exceeds MCP spec limit of 128 characters (got 129)" (limit dropped from 255 → 128).
• SecurityValidator.validate_tool_name("bad-name\n") called directly → rejected. (Noted in the script: the public POST /tools path also has str_strip_whitespace=True on ToolCreate, which strips a trailing newline before validation runs, so that specific request-smuggling angle isn't independently reachable over that endpoint — the re.match→re.fullmatch fix was exercised at the validator directly instead, which is where it matters for any caller that doesn't go through a whitespace-stripping model.)

2. Same-slug gateways don't overwrite each other; virtual server exposes the exact custom_name and resolves scoped calls to the right upstream

• Registered two gateways, "Fast Time E2E" and "Fast-Time E2E" (different names, identical slug fast-time-e2e, confirmed via slugify()), each in a different team scope so the pre-existing per-scope slug-conflict guard doesn't block the pair — both registered successfully with distinct IDs.
• Set custom_name="Public.Echo" on gateway A's echo tool (gateway B's stays defaulted to echo).
• Created a virtual server over both tools. tools/list via /rpc/ returned exact names "Public.Echo" and "echo" — not the old slug-prefixed internal names.
• tools/call with name="Public.Echo" → routed to upstream A, response "echo-from-18091:hello-A".
• tools/call with name="echo" (gateway B's custom name, which also happens to equal gateway A's original_name) → routed to upstream B only, response "echo-from-18092:hello-B", with no 18091 in the response — proving the disambiguation priority fix correctly avoids the cross-gateway mixup this PR's second review round caught.

3. Dataplane backend map keyed by stable gateway UUID, not slug

• Called DataplanePublisherService().fetch_payload() directly against the same live SQLite DB (real DB rows, no mocks) after the above registrations.
• backends for the virtual server contained both gateway IDs as separate keys, each with its own correct URL (:18091 vs :18092) and its own tool_name_aliases ({"Public.Echo": "echo"} vs {"echo": "echo"}) — the exact scenario the old slug-keyed dict would have collapsed into one overwritten entry.

Known gap, called out rather than silently skipped

mcpgateway/transports/streamablehttp_transport.py's list_tools() applies the identical custom_name-over-name substitution for the streamable-HTTP /servers/{id}/mcp transport path. That specific transport wasn't driven live here — it requires a full MCP client session handshake over streamable-HTTP against the gateway, which is materially more setup than the /rpc/ JSON-RPC path exercised above. The /rpc/ path exercises the equivalent logic in mcpgateway/main.py::_handle_tools_list_rpc; the streamable-HTTP transport's version is a one-line change identical in shape, reviewed by code inspection in an earlier pass, not re-verified live here.

[gandhipratik203]

Nice work on this — the UUID backend keying and the custom-name/alias resolution are solid, well-scoped fixes, and the cache-scoping and RBAC re-check on every hit hold up well. Approving.

A few low-severity, non-blocking notes for consideration (follow-ups, not merge blockers):

1. invalidate_all_scoped() flushes all servers' scoped cache on any gateway-less tool change — over-invalidation; a targeted server-set lookup would be tighter if this ever shows up in profiling.
2. The validator now accepts ., .., - — safe within this repo (cache keys use :, which names can't contain), but worth a glance on the companion dataplane consumer if names land in a path/namespace position.
3. zip(..., strict=True) in main.py turns any future filter divergence into a hard 500 rather than silent misalignment — defensible, just flagging the trade-off.
4. The alias "keep first" dedup relies on unordered query iteration, so it isn't strictly deterministic (only matters in the already-warned duplicate case).

One heads-up: the branch currently shows a merge conflict against main (DIRTY), so it'll need a rebase before it can land.

[msureshkumar88]

All four rounds of requested changes are in and verified against current HEAD (f9f0987):

1. Same-slug collision test, doc fix, CHANGELOG entry — present.
2. custom_name given priority-0 in disambiguation (_tool_name_match_priority, tool_service.py:159) alongside .name, with regression coverage for the collision case.
3. main.py no longer zips tools/serialized_tools positionally — filtering-before-pairing fix confirmed (no zip(tools call sites remain).
4. error_formatter.py stale message key updated to match the new "Tool name exceeds MCP spec limit" / 128-char validator message.

Also confirmed: CI is green, PR is MERGEABLE (the earlier merge-conflict note is resolved by the subsequent merge commit), and the live e2e run (20/20 checks) validated the actual bug scenarios end-to-end — UUID-keyed backends, exact custom-name exposure/routing, and the priority-fix collision case.

Approving.

[Lang-Akshay]

Thanks for the PR @lucarlig
Please address following findings.

Blocking Changes

#	Area	File	Line	Blocking reason	Required change
1	Security	mcpgateway/services/tool_service.py	4283, 5045	High CWE-863: server-scoped invocation skips the authoritative server_tool_association check when tool_selected_from_server_scope is true. A stale L1/L2 cache entry can remain usable after association revocation when best-effort Redis invalidation fails, allowing a previously authorized caller to invoke through the revoked server until expiry.	Always check server_tool_association for every server-scoped invocation and execution-plan resolution, including cache hits; alternatively require an authoritative association/version generation and fail closed when invalidation is unavailable. Add deny-path coverage for revocation and multi-worker stale-cache cases.
2	Security	mcpgateway/services/dataplane_publisher.py	53-57, 150-213, 317-353	Medium CWE-613/CWE-863: opt-in per-user dataplane authorization snapshots are retained through fetch failures and until a 130-second default TTL; omitted users/keys are not deleted. User deactivation, team/visibility changes, or revocations can remain usable in the dataplane during the stale window.	Add revocation epochs/generations, atomically replace user namespaces and delete keys absent from the current active set, and fail closed or bound authorization staleness on fetch failure. Trigger invalidation for user/team/entity authorization changes.