Reject tools/call for tools hidden from tools/list - #6216
Conversation
The aggregator keeps every backend tool in the routing table, including ones withheld from tools/list by excludeAllTools, per-workload excludeAll, or filter, so composite-tool workflow steps can still reach them (#3636). core.CallTool resolved a direct call against that same table, so a hidden tool was callable by name -- documented as returning ErrNotFound for an unadvertised name, but never enforced. On the Modern (2026-07-28) path, where tools/call goes straight to core.CallTool, this was reachable off the wire whenever no Cedar policy was configured. Legacy was incidentally protected: the SDK only registers advertised names on the session. Hold the call to the advertised view in the core, so every consumer gets the contract, and classify the refusal as -32602 on the Modern path to match what Legacy already answers for an unregistered tool. Composite tools are unaffected: a workflow enters CallTool once under its own advertised name, and its steps then bypass the core entirely.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6216 +/- ##
==========================================
- Coverage 72.51% 72.46% -0.05%
==========================================
Files 739 739
Lines 76719 76727 +8
==========================================
- Hits 55629 55603 -26
- Misses 17107 17161 +54
+ Partials 3983 3963 -20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-actions
Bot
added
size/M
There was a problem hiding this comment.
Pull request overview
This PR closes an access-control gap in vMCP where Modern (stateless) tools/call could invoke tools that were intentionally hidden by aggregation tool filtering, aligning behavior with the Legacy (session/SDK) path and with core.CallTool’s documented contract.
Changes:
- Enforce “directly callable == advertised” inside
core.CallTool(after authz) so all callers/transports share the same boundary. - Map
vmcp.ErrNotFoundfor Moderntools/callto JSON-RPC-32602/ HTTP 400 to match Legacy’s “unknown tool” classification. - Add regression/integration/unit tests to assert both halves of the invariant: hidden tools are not directly callable, but remain routable for composite workflow steps.
vMCP Review (anti-pattern scan; touched files under pkg/vmcp/)
Clean
- No issues found for vMCP anti-patterns #1–#10 in the changed code paths (no new context-value coupling, no repeated body read/restore patterns, no new caching/abstraction leakage in core/serve boundaries, etc.).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/vmcp/core/core_calls.go | Enforces advertised-view gating for direct tool calls (after authz), preserving composite-step routing via the routing table. |
| pkg/vmcp/server/modern_dispatch.go | Reclassifies ErrNotFound from tools/call to -32602/HTTP 400 for Modern parity with Legacy. |
| pkg/vmcp/server/hidden_tool_regression_test.go | Adds two-direction regression test covering Modern + Legacy and composite-step counterweight. |
| pkg/vmcp/server/session_management_integration_test.go | Extends integration harness to support routing-table-only hidden tools and backend-call recording. |
| pkg/vmcp/core/core_calls_test.go | Adds core-level guard test + updates rename/alias expectations to match new advertised-only direct-call behavior. |
| pkg/vmcp/core/core_checks_test.go | Adds ordering test to ensure authz denial takes precedence over not-found for hidden+denied tools. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
Reviewed this alongside a second independent review pass, then had the disputed findings adjudicated before writing anything up. The fix itself I'm happy with: holding the call to the advertised view in the core rather than in the Modern dispatcher is the right call, the composite-steps-never-re-enter-CallTool reasoning holds up (I traced it), and the both-directions test is the part I'd want to keep — a single-direction test is exactly what let this ship. -32602 is also right, and not just SDK-faithful: it's the literal example in the 2026-07-28 spec's tools error-handling section.
Everything below is comments, docs, or test coverage. Nothing blocks merge.
Two things I want to flag that aren't inline, because they're on files this diff doesn't touch:
pkg/vmcp/core/admission.go:322—findAdvertisedTool's doc comment still ends "...and routing remains the authority on whether the call resolves." That's now false, and it's the helper this fix is built on. Worth the one-line fix while you're here.docs/arch/10-virtual-mcp-architecture.md:222— the whole### Tool Filteringsection is one sentence. The invariant this PR establishes (advertised == directly callable, routing table deliberately wider and reachable only from inside a composite) isn't recorded anywhere indocs/arch/. Right now the only place it's written down is the new test comments, which means the next person who "fixes" this by pruning the routing table has nothing telling them not to. Three or four sentences in that section would do it.
Smaller, also not inline:
aggregator/default_aggregator.go:635—shouldAdvertiseTool's "controls advertising, not routing" now understates things, since this gate is what decides callability. One clause. (Its neighbours are fine — they describe routing-table contents, which genuinely didn't change.)buildTestServerWithOptionsnever callssrv.Stop, andcore.Newstarts the workflow-state cleanup ticker in its constructor, so every fixture leaks a core. Pre-existing — main already has 7 leaking call sites and this PR adds 2 — so fixing it once in the helper (registersrv.Stopbeforet.Cleanup(ts.Close)) is the smaller diff and gets all 9. Your call whether that belongs here or separately.resources/readstill answers-32603for a nonexistent URI on Modern, which is a spec MUST violation (§Resources mandates-32602), andprompts/getdiverges from Legacy the same way. You flagged the inconsistency yourself and scoping it out is right — but it's a real protocol bug rather than a cosmetic nit, so it's worth an issue rather than just a note.
One thing I checked and want to explicitly not ask you to change: the guard uses the pre-admission aggregation view rather than ListTools' post-FilterTools result. That's correct as written. Making it match ListTools would delete the args-aware call-side decision and turn an authorization outcome into -32602, which recreates the enumeration oracle your own TestCallTool_DenialPrecedesNotFound guards against. Only the comment needs narrowing, not the code.
| // "Unadvertised" is enforced against the same view ListTools returns, NOT the | ||
| // routing table — which intentionally holds more (see the advertised-view check | ||
| // below). A tool hidden from tools/list is therefore not directly callable, | ||
| // while composite workflow steps may still reach it. |
There was a problem hiding this comment.
"the same view ListTools returns" isn't quite true, and the second sentence is broader than what the code does. ListTools is admission.FilterTools(advertisedTools(agg)) (core_vmcp.go:281), so it returns a narrower set than the one this guard checks — FilterTools authorizes with nil args (admission.go:159) while AllowToolCall gets the real ones (admission.go:186). So a tool hidden from tools/list by an argument-conditional Cedar policy is still callable when the call carries permitted arguments.
That's fine and deliberate — authorizeToolCall is the enforcement point for that case and it runs first. It just means this comment is promising something wider than the guard delivers. The commit message and PR body both get this right (they scope to excludeAllTools/excludeAll/filter); it's only here that it's unqualified.
| // "Unadvertised" is enforced against the same view ListTools returns, NOT the | |
| // routing table — which intentionally holds more (see the advertised-view check | |
| // below). A tool hidden from tools/list is therefore not directly callable, | |
| // while composite workflow steps may still reach it. | |
| // "Unadvertised" here means the AGGREGATION view that ListTools filters — NOT | |
| // the routing table, which intentionally holds more (see the advertised-view | |
| // check below). Admission narrowing on top of that view is enforced separately, | |
| // by authorizeToolCall. So a tool hidden by excludeAllTools, per-workload | |
| // excludeAll, or filter is not directly callable, while composite workflow steps | |
| // may still reach it. |
There was a problem hiding this comment.
Correct, and the distinction matters — thanks. ListTools is FilterTools(advertisedTools(agg)) (core_vmcp.go:281), and FilterTools authorizes with nil args (admission.go:161) while AllowToolCall gets the real ones (admission.go:187), so the argument-conditional case you describe is real: a tool absent from tools/list under a when {} clause is still callable with permitted arguments, via authorizeToolCall.
Applied your wording verbatim in 5beec43.
| // (router/session_router.go:105) for a direct call: an alias is never an | ||
| // advertised name. That alias exists for workflow step definitions and keeps | ||
| // working there, inside the composer. | ||
| if findAdvertisedTool(c.advertisedTools(agg), name) == nil { |
There was a problem hiding this comment.
authorizeToolCall already did this exact lookup four lines up — core_checks.go:79 is also findAdvertisedTool(c.advertisedTools(agg), name), it just throws away the result and substitutes a bare &vmcp.Tool{Name: name} when it misses. And line 85 then calls accessibleComposites(agg) a third time, which advertisedTools already called internally.
So on a view with composites, every tools/call now runs FilterWorkflowDefsForSession + ValidateNoToolConflicts + ConvertWorkflowDefsToTools three times, allocates the concatenated tool slice twice, and linear-scans it twice.
Cheapest fix is to compute both once at the top of CallTool and thread them through — authorizeToolCall taking the precomputed *vmcp.Tool (or the slice) instead of agg, and line 85 reusing the same composites map. That also removes the possibility of the two lookups ever disagreeing.
There was a problem hiding this comment.
Good catch — three passes, and I'd missed that advertisedTools calls accessibleComposites internally too.
Took the approach you suggested in 5beec43. CallTool now resolves both once:
composites := c.accessibleComposites(agg)
advertised := advertisedToolsWith(agg, composites)
tool := findAdvertisedTool(advertised, name)then threads them through: authorizeToolCall takes the resolved *vmcp.Tool instead of agg (nil-checks internally and substitutes the bare stub, so its fail-closed behaviour for an absent name is unchanged), the guard becomes if tool == nil, and the composite branch reuses composites[name].
advertisedTools is split into a thin method plus advertisedToolsWith(agg, defs) taking a pre-resolved set, so ListTools/Discover/authorizedBackends keep their existing one-line call and only CallTool passes the shared map.
That collapses it to one pass, one allocation, one scan. And as you note, it also makes the drift you flagged structurally impossible — admission and the guard now read the same tool variable rather than performing independent lookups that could disagree.
CheckToolCall still does its own lookup, which is correct: it has no composite dispatch to share with, and the anti-drift guarantee is that both routes resolve the tool the same way, not that they share a call.
| // never enters CallTool (see TestCallTool_CompositeWorkflow, whose step targets | ||
| // "be1.echo"). |
There was a problem hiding this comment.
This pointer doesn't hold up, and it's hiding a coverage gap. TestCallTool_CompositeWorkflow doesn't exercise the alias fallback: it registers be1.echo as an exact routing-table key (line 148), so RouteTool's fast-path exact match answers and the dot-convention fallback at session_router.go:110-118 is never reached.
Which means this PR removes the only call in the suite that ever drove that fallback and replaces it with an assertion that it's rejected. The guarantee the alias exists for — a workflow step written as be1.echo still resolving when the routing table is keyed be1_echo after conflict resolution — now has no coverage at all, and a future prune of the fallback would go unnoticed.
Worth a third leg: routing key be1_echo with OriginalCapabilityName: "echo", a workflow step targeting be1.echo, asserting the step reaches backendClient.CallTool. That's the case session_router.go:87-95 says the alias is for.
| // never enters CallTool (see TestCallTool_CompositeWorkflow, whose step targets | |
| // "be1.echo"). | |
| // never enters CallTool. Note that TestCallTool_CompositeWorkflow does NOT cover | |
| // the alias fallback: it registers "be1.echo" as an exact routing-table key, so | |
| // RouteTool's fast path answers and the fallback never runs. |
There was a problem hiding this comment.
You're right, and this was the most valuable comment in the review — I asserted that pointer without checking it. TestCallTool_CompositeWorkflow registers be1.echo at line 148 as an exact key, so RouteTool's fast path (session_router.go:101-103) answers and the fallback never executes. My PR would have removed the suite's only alias-driving call and replaced it with a rejection.
Added your third leg in 5beec43 — routing key be1_echo with OriginalCapabilityName: "echo", workflow step targeting be1.echo, asserting the step reaches backendClient.CallTool.
Verified it actually drives the fallback rather than passing incidentally, by short-circuiting the dot-convention branch:
if dotIdx := strings.Index(toolName, "."); false && dotIdx > 0 { // probeOnly the new leg fails:
--- FAIL: TestCallTool_ResolvesRenamedTool/composite_step_reaches_the_backend_through_the_dot-alias_fallback
missing call(s) to MockBackendClient.CallTool(..., is equal to be1.echo, ...)
The other two legs pass with the fallback disabled, confirming they only ever used the fast path — exactly as you said.
Also corrected the comment to your suggested text, so it now warns about the fast-path shadowing instead of citing that test as coverage.
| // filter -- matching what the Legacy SDK path already returns for a tool it | ||
| // never registered on the session (-32602 "unknown tool", go-sdk | ||
| // server.go:957), which writeModernError maps to HTTP 400. |
There was a problem hiding this comment.
The citation is off. vMCP's Legacy path doesn't reach go-sdk's callTool — it goes through toolhive-core/mcpcompat, which produces the -32602 at mcpcompat/server/request_handler.go:146 and then rewrites the message in translateUnknownToolError (mcpcompat/server/server.go:1165-1179). So what a Legacy client actually sees is tool "X" not found, not unknown tool "X".
Same code either way, so the parity claim survives — but the two eras answer with different text, and this comment reads as though they're identical.
| // filter -- matching what the Legacy SDK path already returns for a tool it | |
| // never registered on the session (-32602 "unknown tool", go-sdk | |
| // server.go:957), which writeModernError maps to HTTP 400. | |
| // filter -- matching what the Legacy path already returns for a tool it | |
| // never registered on the session (-32602, via toolhive-core mcpcompat's | |
| // translateUnknownToolError, which rewrites the message to `tool "X" not | |
| // found`), which writeModernError maps to HTTP 400. |
There was a problem hiding this comment.
Confirmed and fixed in 5beec43. I traced it: translateUnknownToolError at mcpcompat/server/server.go:1284 matches on "unknown tool" and rewrites to mcp-go's tool %q not found wording, preserving InvalidParams — called from the dispatch path at server.go:858.
Applied your suggestion, and added a sentence making the residual difference explicit ("Same code either way; the two eras differ only in message text"), since that's the part the original comment obscured.
Related: this is why the Legacy leg of the regression test asserts on the decoded error.code rather than message text — see your comment on hidden_tool_regression_test.go, which I've also applied. Matching on the message would have coupled the test to whichever era's wording it happened to be written against.
| // not-found classification is a separate decision. The message names no | ||
| // tool: an authorization denial is classified ahead of this (in | ||
| // core.CallTool, which authorizes before checking the advertised view), and | ||
| // echoing the name back here would turn the pair of answers into a probe | ||
| // for which denied tools exist. |
There was a problem hiding this comment.
The omission is a reasonable choice, but it doesn't buy what this says it buys. A denial answers 403 + JSONRPCCodeDenied; this answers 400 + -32602. Those are already trivially distinguishable by status and code, whether or not the name is in the message — so withholding it doesn't close a probe. (Legacy echoes the name, and so does the spec's own example for this error.)
Keep the behaviour, just don't claim it's a mitigation:
| // not-found classification is a separate decision. The message names no | |
| // tool: an authorization denial is classified ahead of this (in | |
| // core.CallTool, which authorizes before checking the advertised view), and | |
| // echoing the name back here would turn the pair of answers into a probe | |
| // for which denied tools exist. | |
| // not-found classification is a separate decision. The message names no | |
| // tool: an authorization denial is classified ahead of this (in | |
| // core.CallTool, which authorizes before checking the advertised view). | |
| // Omitting the name is a conservative choice rather than a mitigation -- | |
| // a denial already answers 403 + JSONRPCCodeDenied against this 400 + | |
| // -32602, so the two are distinguishable either way. |
There was a problem hiding this comment.
Fair — I over-claimed. 403 + JSONRPCCodeDenied vs. 400 + -32602 are trivially distinguishable regardless of the message, so withholding the name closes nothing. Applied your wording in 5beec43; behaviour unchanged, the justification is now honest about being conservatism rather than a mitigation.
| // core.CallTool, which authorizes before checking the advertised view), and | ||
| // echoing the name back here would turn the pair of answers into a probe | ||
| // for which denied tools exist. | ||
| if errors.Is(err, vmcp.ErrNotFound) { |
There was a problem hiding this comment.
Minor hardening, your call. authorizeToolCall wraps with a double %w — fmt.Errorf("%w: tool %q: %w", vmcp.ErrAuthorizationFailed, name, err) at core_checks.go:84 — so errors.Is matches everything in both branches of that tree. Testing the derived sentinel before the security-classifying one means an authorizer error that happened to wrap ErrNotFound would answer 400 instead of 403.
Not reachable today: I checked, and pkg/authz has no import of pkg/vmcp at all, so neither the Cedar nor the HTTP/remote-PDP authorizer can produce an error carrying vmcp.ErrNotFound. But ErrNotFound is exported and Admission is a public interface, so a future implementation could trip this without knowing about the coupling. One line to make it order-independent:
| if errors.Is(err, vmcp.ErrNotFound) { | |
| if errors.Is(err, vmcp.ErrNotFound) && !errors.Is(err, vmcp.ErrAuthorizationFailed) { |
There was a problem hiding this comment.
Taken — applied in 5beec43. Confirmed your reachability analysis independently: pkg/authz imports pkg/vmcp/optimizer, pkg/vmcp/schema and pkg/vmcp/session/optimizerdec, but never the pkg/vmcp root where ErrNotFound lives, so no current authorizer can produce an error carrying it.
Worth doing anyway for the reason you give: Admission is a public interface and ErrNotFound is exported, so the coupling is invisible to a future implementer. One && to make a security classification order-independent is the right trade — a mistake here fails open toward 400-instead-of-403, which is the wrong direction to leave load-bearing on statement order.
Documented the reasoning inline so the extra clause doesn't read as redundant to someone who checks the same import graph later and concludes it's dead.
| assert.Contains(t, hiddenBody, "-32602", | ||
| "Legacy must also refuse a hidden tool as an unknown tool: %s", hiddenBody) |
There was a problem hiding this comment.
This is weaker than the Modern leg's check for the same invariant — Contains on the raw body passes if -32602 shows up anywhere in it (an unrelated frame, an echoed message), and never verifies it's error.code for request id 3.
The stated reason for staying at string level ("the SDK's streamable transport may answer as SSE") is already contradicted in this package: listToolNames in session_management_integration_test.go:635-660 decodes Legacy tools/list responses just fine. Since this is the assertion keeping the two eras from drifting, it'd be worth decoding and asserting error.code == -32602 the way the Modern leg does at line 143.
There was a problem hiding this comment.
Right on both points, and the SSE justification was wrong — listToolNames (session_management_integration_test.go:635) decodes Legacy responses with a plain json.NewDecoder, so the transport answers JSON here.
Fixed in 5beec43:
tools/listnow goes through the existinglistToolNameshelper and asserts on the returned names, rather than substring-matching the body.tools/callgoes through a newlegacyCallErrorthat decodes and returns theerrorobject, so the assertion isassert.Equal(t, float64(-32602), hiddenErr["code"])— the same shape as the Modern leg at line 143.
legacyCall survives only for the composite counterweight, where the assertion is on the backend recorder and the body is irrelevant; its doc comment now says so instead of carrying the bogus SSE claim.
As you say, this is the assertion holding the two eras together, so it needed to be as strong as the Modern one. It's also now robust to the message-text difference between eras that you flagged on modern_dispatch.go — code only, no wording.
Resolve the advertised view and composite set once in CallTool and thread both through admission, the not-found guard and composite dispatch. Each of accessibleComposites/advertisedTools re-runs workflow filtering and conversion, so the previous code did that work three times per call and allocated the concatenated slice twice. Add the leg that drives RouteTool's dot-alias fallback: rejecting the alias for direct calls otherwise left it with no coverage, since TestCallTool_CompositeWorkflow registers the aliased name as an exact routing-table key and never reaches the fallback. Assert the Legacy refusal on the decoded error.code rather than a substring of the response body, matching the Modern leg. Correct three comments: the guard checks the aggregation view ListTools filters (admission narrows further, separately); Legacy's -32602 comes from mcpcompat's translateUnknownToolError, not go-sdk directly; and omitting the tool name from the error is conservatism, not a mitigation, since a denial is already distinguishable by status and code. Exclude ErrAuthorizationFailed from the not-found branch so the classification cannot depend on branch order.
github-actions
Bot
added
size/L
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
Re-checked all of it against 5beec43. Every inline point is addressed, and the dedupe went further than what I suggested — pulling out advertisedToolsWith and changing authorizeToolCall to take a resolved *vmcp.Tool means the admission lookup and the not-found guard now share one result and can't drift apart, which is better than just hoisting the call. The third leg on TestCallTool_ResolvesRenamedTool is exactly the case I was worried about losing: it's now the only test in the suite that drives RouteTool's dot-convention fallback rather than its exact-key fast path. legacyCallError also brings the Legacy assertion up to the Modern leg's strength.
Approving. Four leftovers, all one-line comment/doc edits on files outside the diff, none blocking:
pkg/vmcp/core/admission.go:322—findAdvertisedTool's doc still ends "...and routing remains the authority on whether the call resolves." This is the one I'd actually still like fixed: it's now false, and after this refactor it's the helper three consumers route through. Easy to have missed since it couldn't be an inline comment.docs/arch/10-virtual-mcp-architecture.md:222—### Tool Filteringis still one sentence; the advertised-vs-routing invariant is only recorded in test comments.pkg/vmcp/aggregator/default_aggregator.go:635—shouldAdvertiseTool's "controls advertising, not routing" now understates, since that gate decides callability.- The
srv.Stopgap inbuildTestServerWithOptions— pre-existing and your call, as I said before.
Happy either way on whether those ride along here or go in a follow-up. Note I didn't run tests or lint on this revision.
Summary
vMCP tool filtering (
aggregation.tools,filter,excludeAll,excludeAllTools) is enforced on the Legacy path but not on the Modern one, so a Modern client that knows the name of a filtered-out tool can call it successfully. Filtering is understood as an access-control boundary — #6073 uses a filtered vMCP to constrain unsupervised agents — so the same config silently means something different depending on which revision a client speaks.The aggregator deliberately puts every backend tool in the routing table — including ones withheld from
tools/list— so composite-tool workflow steps can still reach them (#3636,default_aggregator.go:349). Legacy registers one SDK handler per advertised tool, so registration is the gate and a filtered name is rejected as unknown. Modern is stateless with no registration step:tools/callgoes from the request tocore.CallTool(modern_dispatch.go:297), which resolves the name against the unfiltered table (core_calls.go:64). Nothing narrows it.CallTool's doc comment already promisedErrNotFoundfor an unadvertised name; nothing enforced it.tools/callto the advertised view in the core, not in the Modern dispatcher, so every consumer inherits the contract (Modern,codemode,ratelimit, any futureVMCPdecorator) rather than one transport being patched — see the reviewer note below on why this is broader than the issue proposed. The check reusesadvertisedTools/findAdvertisedToolagainst theaggalready in hand — no extra aggregation or backend fan-out.authorizeToolCall, so a denied tool keeps returningErrAuthorizationFailed. If the order were reversed,ErrNotFoundvs. denial would tell an unauthorized caller which hidden tools exist.-32602(HTTP 400) on the Modern path, at thetools/callsite. Without this,ErrNotFoundlaunders into-32603/HTTP 200 and the two eras still disagree on the same input — Legacy answers-32602"unknown tool" (go-sdkserver.go:957).Composite tools are unaffected, with no exemption code. A workflow enters
CallToolonce under its own (advertised) name; its steps then runcomposer → router.RouteTool → backendClient.CallTool(workflow_engine.go:428,486) and never re-enter the core. #3636 is preserved exactly.Fixes #6217
Type of change
Test plan
task test)task test-e2e)task lint-fix)task lint-fixreports 4 issues (3 gosec, 1 staticcheck), all pre-existing in files this PR does not touch — verified identical on a cleanmaincheckout.Both failure directions were verified by deliberately breaking the code, because this invariant has two halves that must move in opposite directions:
hidden_toolreturnsfake result, i.e. a successful filtered-tool call (Legacy still passes, reproducing the asymmetry the issue describes)a composite workflow step must still reach a hidden backend tool (#3636)That second row is the point: pruning the routing table would satisfy every "filtered tool is rejected" assertion while silently re-breaking #3636. A single-direction test is what let this bug ship in the first place.
Changes
pkg/vmcp/core/core_calls.goCallToolresolves against the advertised view, after authz; doc comment now states which view "unadvertised" meanspkg/vmcp/server/modern_dispatch.goErrNotFoundontools/call→-32602/HTTP 400 instead of-32603pkg/vmcp/server/hidden_tool_regression_test.gopkg/vmcp/server/session_management_integration_test.goserverOptions.hiddenTools(routing-table-only names) andonBackendCall(records forwarded tool names)pkg/vmcp/core/core_calls_test.gopkg/vmcp/core/core_checks_test.goDoes this introduce a user-facing change?
Yes — two related narrowings, both deliberate:
tools/listis no longer directly callable. This is the point of the fix: filtering now holds identically on both revisions. Anyone relying on invoking anexcludeAll/filter-hidden tool by name loses that. Because the fix is in the core rather than the Modern dispatcher, it applies to every path (Modern,codemode,ratelimit), not just Modern. Composite tools calling filtered tools continue to work, which is the supported way to reach one.RouteTool's{workloadID}.{toolName}alias is no longer accepted for a direct call. An alias is by definition not an advertised name, so accepting one would leave a second bypass of exactly the boundary this PR closes. It exists so composite workflow step definitions survive conflict-resolution renames and keeps working there, inside the composer. Nothing in-tree called the core with an alias (codemodebindsinnerTools; the optimizer and Legacy bind advertised names), so this affects only an external core-library consumer that constructed one by hand.Special notes for reviewers
Two scrutiny requests:
Why the core and not the Modern dispatcher. Modern path: filtered-out vMCP tools are still callable #6217 proposes refusing to route a Modern call; this fixes it one layer down. A dispatcher-only guard was considered and rejected: it would cost an extra aggregation per
tools/call(viaLookupTool), leavecore.CallTool's documentedErrNotFoundcontract false for library consumers, and — given the issue frames filtering as an access-control boundary — fix only the transport that happens to expose the gap today, leaving the next stateless consumer to re-open it. The work item that prompted this assumed a core guard would need an explicit composite-step exemption; it does not, because steps never reachcore.CallTool. That's the load-bearing fact — if it's wrong, this design is wrong.-32602is scoped totools/callon purpose. It is written at the call site rather than added towriteModernDispatchError, which is shared withresources/read,prompts/getandcompletion/complete— their not-found classification is a separate decision, out of scope here. Consequence: those verbs still answer-32603for a not-found, so Modern is briefly inconsistent across verbs. Worth a follow-up, but bundling it would mix scopes.Deliberate rewrite worth a close look:
TestCallTool_ResolvesRenamedToolpreviously asserted thatCallTool("be1.echo")succeeds while onlybe1_echois advertised — it pinned the dot alias as callable through the core. It is now split into a resolved-name success leg and an alias-rejection leg. This is the one existing assertion this PR intentionally inverts.The error message deliberately does not name the requested tool. Echoing it back would turn the not-found/denied pair into a probe for which denied tools exist;
TestCallTool_DenialPrecedesNotFoundpins that ordering.No e2e run: the existing e2e coverage (
test/e2e/vmcp_cli_features_test.go:248-347) only asserts filtered tools are absent fromtools/listand never that they are uncallable — which is precisely the blind spot that let this ship. Adding a Modern callability leg there is worth a follow-up; the new integration test covers both eras in-process in the meantime.Generated with Claude Code