fix(mcp): refuse MCP tools that shadow reserved framework names

[sushant-me]

Refuse a tool name that is already owned by something the model dispatches

Fixes #1513.

What this does

An agent that carries an in-model tool (google_search, url_context, load_artifacts,
list_skills, …) and also a function tool of the same name ends up with two things answering
to one name. The in-model tool is written into the request's config tools and never enters
LlmRequest.tools(), so nothing rejects the duplicate; the model chooses between them per
call, and nobody decided that.

This adds the error in BaseLlmFlow.getRequestProcessorFromTools — the one path every tool
passes through, including each tool unwrapped from a toolset — so it covers FunctionTool,
AgentTool, McpToolset and McpAsyncToolset alike.

Behaviour change to note before upgrading

An agent that carries, for example, GoogleSearchTool and its own google_search function
tool will now fail where it previously ran, with Duplicate tool name: google_search. This
is the intended behaviour, but it is a change for anyone in that configuration, and the fix is
to rename one of the two.

Only that direction is rejected. Two tools that both lack a declaration may still share a name,
so setups such as two default-named ExampleTools keep working.

Why the check lives here and not in McpToolset

An earlier revision of this PR put a reserved-name list in McpToolset. That cannot work,
because McpToolset cannot see the agent's other tools, so it cannot tell a real collision from
a harmless name: it rejected a server whose tool merely shared a name with an in-model tool the
agent did not even have. Moving the check to the request processor makes the decision where the
full tool list is visible.

How the collision is detected

A tool without a declaration does not appear in the built request, so the two halves are read
separately:

• declaration-less names come from agent.toolsUnion() plus each baseToolset.getTools(...),
  filtered on declaration().isEmpty();
• function-tool names come from the built request's tools() map.

The check fails on the intersection.

Testing Plan

core: 1856 tests, 0 failures, 0 errors, 24 skipped   (./mvnw -pl core test)

New cases in BaseLlmFlowTest:

• getRequestProcessorFromTools_rejectsDeclarationlessNameCollision — exercises the in-model
  tool both before and after the clashing function tool, since the order must not matter.
• getRequestProcessorFromTools_allowsTwoDeclarationlessToolsSharingAName — the negative case.

Each new test was checked to fail with the guard reverted, so it pins behaviour rather
than passing vacuously:

[ERROR] BaseLlmFlowTest.getRequestProcessorFromTools_rejectsDeclarationlessNameCollision:989
        expected java.lang.IllegalArgumentException to be thrown, but nothing was thrown

Built and tested locally with ./mvnw on JDK 25. CI pins JDK 17.

Revision history

• 6d3088bc — original: a reserved-name list inside McpToolset.
• 04534436 — corrected the set to names this framework actually defines.
• 65c409b2 — added the skill and loop tools.
• 5635996e — moved the check to run after tool selection. Insufficient: this kept the check
  inside McpToolset, so both review objections still held.
• 9c0ae01b — reworked as requested, into BaseLlmFlow.getRequestProcessorFromTools, with
  the order-independence test and the behaviour-change note above.

[hemasekhar-p]

Hi @sushant-me , thank you for your contribution. We appreciate you taking the time to submit this pull request. Currently this PR is under review by our team, we will keep you posted if any additional information is required. thank you.

[sushant-me]

One scoping note on the residual, so the trade-off is on the record — the same class of note as the one on the Go port, but the reasoning differs here and I did not want to copy it across.

This guard closes the path where a remote MCP server advertises a reserved name. It does not change the underlying property that an in-model tool never occupies its name in the tool map:

• GoogleSearchTool.processLlmRequest (and the other in-model tools) only append to configBuilder.tools(...); they never put the name into llmRequestBuilder.tools().
• Nothing else does either — I could not find a duplicate-name check at the tools() map level in the flows (the only duplicate check is for sub-agent names in BaseAgent).

So an in-process tool registered under google_search — a FunctionTool or an AgentTool — is accepted alongside the in-model tool. In this repository that requires the application author to pick the name, so I treated it as a footgun rather than the adversarial case and kept this PR to the server-controlled path.

Where Java differs from Go: in adk-go the equivalent map is map[string]any and consumers type-assert to tool.Tool, so occupying a name with a sentinel breaks them — that is why I left the general fix alone there. Here LlmRequest.tools() is already Map<String, BaseTool> (LlmRequest.java:89), so registering a sentinel or a real holder under the in-model name is type-safe. The open question is only whether a sentinel value is acceptable to advertise, which is a design decision rather than a typing constraint.

Happy to prepare that broader change if you would prefer the invariant enforced in one place instead of at each boundary — it touches high-fan-in classes, so I did not want to fold it in unasked.

[sushant-me]

Thanks, @hemasekhar-p — appreciated.

One thing that may help while it is in review: RESERVED_TOOL_NAMES currently carries all 13 names Java puts on the wire, and I checked it is neither short nor over-broad — every entry is a super("...") literal in a non-test source, plus set_model_response and transfer_to_agent which the framework contributes outside a tool class. Two names an earlier revision of my notes carried over from the Go port (finish_task, task_completed) are not Java names and are deliberately absent.

The two halves of #1513 fail differently in this code, which is why the guard covers both:

• In-model built-ins (google_search, google_maps, url_context, vertex_ai_search, code_execution) append only to config.Tools and never reach appendTools, so a callable tool takes the name and Functions.handleFunctionCalls resolves it to the server tool.
• Callables (set_model_response and friends) are already fail-closed — appendTools throws Duplicate tool name — so there the guard converts a run-aborting exception into a clean registration error.

Happy to adjust the list, the error type, or the placement if your team prefers a different shape.

[MiloszSobczyk]

Thank you for the detailed report, and for tracking down the loadMemory naming. That was useful.

I'd like to go with the alternative you describe at the end of the PR description, rather than a reserved list in McpToolset. The main reason is that McpToolset can't see the agent's other tools, so it can't tell a real collision from a harmless name. As written, the check:

• rejects servers even when nothing collides. An agent with no GoogleSearchTool can no longer load a server that exposes a google_search tool. The same goes for code_execution, load_skill, list_skills, exit_loop and the rest.
• does not cover McpAsyncToolset. initTools() still wraps every server tool without a check, and #1513 lists it as a place to fix.

For the function tools in the list, including set_model_response, LlmRequest.Builder.appendTools already throws Duplicate tool name on a real collision. So those cases already fail with a clear error, and the list only adds errors where nothing collides.

For the 5 in-model tools, ADK never dispatches them through the tool map, because the model runs them itself. So a server tool with the same name does not take over their dispatch. It adds a second tool with the same name. That is confusing, and a clear error for it is a good idea.

I think the simplest place for that error is BaseLlmFlow.getRequestProcessorFromTools, since every tool passes through it, including each tool from a toolset. It can fail with Duplicate tool name when a tool with no function declaration (like the in-model tools) has the same name as a function tool in the request. That covers MCP sync and async, FunctionTool and AgentTool, and it doesn't change the tool map used for dispatch. Checking only that case keeps setups like two ExampleTools with the default name working. Since an agent with, for example, GoogleSearchTool plus its own google_search function would start failing, please mention this in the PR description.

Could you please rework the PR in that direction? A test with the in-model tool both before and after the clashing tool would be great, since the order shouldn't matter. Please also keep code comments short and leave the revision history in the PR description.

[sushant-me]

Done — thank you for reading the control flow closely enough to find this. The check now runs after selection, and there is a test for the case you described.

What changed

The reserved-name check sat inside the mapping step, so it ran before isToolSelected:

.map(tool -> {
  if (RESERVED_TOOL_NAMES.contains(tool.name())) throw new IllegalArgumentException(...);
  return new McpTool(tool, ...);
})
.filter(tool -> isToolSelected(tool, toolFilter, readonlyContext)));

It now runs in the flow, after selection:

.map(tool -> new McpTool(tool, this.mcpSession, this.mcpSessionManager, this.objectMapper))
.filter(tool -> isToolSelected(tool, toolFilter, readonlyContext))
.map(tool -> {
  if (RESERVED_TOOL_NAMES.contains(tool.name())) throw new IllegalArgumentException(...);
  return tool;
});

One note on the ordering, in case it looks arbitrary: the McpTool has to be constructed before the filter rather than after it, because isToolSelected takes a BaseTool and dispatches on either a ToolPredicate or a list of names. So "wrap → filter → check" is the only order that keeps the existing filter semantics.

The test

getTools_toolFilterExcludesReservedName_loadsSelectedToolsInsteadOfFailing — the server advertises a reserved name plus tool1, and the caller's toolFilter is ["tool1"]. It asserts the toolset yields exactly ["tool1"].

I ran it against the old ordering, because an assertion that has never failed is not evidence:

OLD ordering:
  McpToolsetTest…:399 » McpToolLoading
  Invalid argument encountered during tool loading.        <- your bug, reproduced
NEW ordering:
  Tests run: 27, Failures: 0, Errors: 0

getTools_refusesReservedToolName still passes unchanged, so a reserved name that is selected is still a fatal, non-retried error. The guard still guards — it just no longer over-fires.

Checks: ./mvnw -pl core test -Dtest=McpToolsetTest → 27 tests, 0 failures. google-java-format clean.

One question, since you know the code better than I do: is "after selection" the placement you had in mind, or would you rather the check live inside isToolSelected itself so every toolset subclass inherits it? I put it in getTools because the error is meant to be fatal and non-retried, and that is expressed by where it sits relative to retryWhen — but if you would prefer it owned by the base class, that is a different and probably better factoring, and I would rather do it your way than argue for mine.

[sushant-me]

I need to correct my previous comment. I wrote "Done", but what I pushed is not the rework you asked for, and your two objections are both still true of the current head.

You asked for the error to move to BaseLlmFlow.getRequestProcessorFromTools, so that it fires only on a real name collision and covers McpAsyncToolset, FunctionTool and AgentTool through the one path every tool passes through.

What 5635996e actually did was keep the check inside McpToolset and change when it runs — after the tool filter rather than before. That is a narrower change than it sounds:

• Your first objection stands. McpToolset still cannot see the agent's other tools, so it still rejects a server whose tool merely shares a name with an in-model tool the agent does not even have. An agent with no GoogleSearchTool still cannot load a server exposing google_search.
• Your second objection stands. McpAsyncToolset is still unchecked; initTools() wraps every server tool without a check, exactly as MCP toolset: in-model built-ins (e.g. google_search) can be shadowed by a server tool; a server tool named set_model_response aborts the run #1513 describes.

So moving the check later within McpToolset did not address the reason you asked for the move. I read my own change as satisfying the review when it only softened the symptom, and "Done" was the wrong word for it.

I have read getRequestProcessorFromTools and LlmRequest.Builder.appendTools (the Duplicate tool name throw at LlmRequest.java:219), and I understand the rule you want: after the processors have run and the request is built, fail when a tool with no function declaration shares a name with a function tool in the request — and only in that direction, so two ExampleTools with the default name keep working.

What I have not yet worked out is how to read "a tool with no function declaration" off the built request reliably, which is the part the check depends on. I would rather say that plainly than push a second attempt that reads as complete. This is not done, and I will rework it in the direction you described, with the test that exercises the in-model tool both before and after the clashing tool, and with the PR description updated to note that an agent carrying both GoogleSearchTool and its own google_search function would begin to fail.

If you would prefer to close this while I do that, that is reasonable — nothing here is worth leaving in a state that looks answered when it is not.

[sushant-me]

I said the open question was how to read "a tool with no function declaration" off the built request. I have worked that out, and the answer changes where the check can live. Recording it so the rework has a concrete route.

A declaration-less tool never enters LlmRequest.tools(). BaseTool.processLlmRequest returns before the only appendTools call:

public Completable processLlmRequest(LlmRequest.Builder b, ToolContext ctx) {
  if (declaration().isEmpty()) {
    return Completable.complete();          // returns here
  }
  b.appendTools(ImmutableList.of(this));    // never reached

And the in-model tools override processLlmRequest to write to the config tools instead. Verified by counting appendTools calls in each:

tool	registered name	appendTools calls
GoogleSearchTool	google_search	0
UrlContextTool	url_context	0
LoadArtifactsTool	load_artifacts	0
ListSkillsTool	list_skills	0

GoogleSearchTool adds Tool.builder().googleSearch(...) to GenerateContentConfig.tools() and returns, which is exactly the "second tool with the same name" you described — it is in the config, not in the map.

So the comparison the check needs cannot be made from the built LlmRequest alone: the declaration-less half is not in it. It has to read the tool list from the agent. In getRequestProcessorFromTools both halves are reachable — agent.toolsUnion() (plus each baseToolset.getTools(readonlyContext)) gives the declaration-less names via declaration().isEmpty(), and builder.build().tools() gives the function-tool names. Fail on the intersection, in that direction only, so two ExampleTools with the default name still work.

One thing worth knowing before it is written: an in-model tool's presence in toolsUnion() does not mean its declaration was added to the request. GoogleSearchTool.processLlmRequest errors for a non-gemini- model, so "the agent declares it" and "the request carries it" can differ. Whether the check should key on the agent's list or on the config tools the request actually ended up with is a judgement call I would rather you make than guess at — the first is simpler, the second is closer to what the model sees.

I am not pushing this yet, and I want to be explicit about why rather than silent: there is no gradlew in my checkout and no Gradle on PATH, so I cannot compile or run a test for core/ here. I have already put one unverified change in front of you on this PR and labelled it done when it was not; I would rather hand you the analysis and a working build than do that twice.

If you can point me at how you run the module's tests, or if it is easier for you to take it from here, either is fine — I will rework it against a tree I can actually build.

[sushant-me]

Reworked in 9c0ae01b, in the direction you described, and I think your reading of the mechanism was the right one — following it is what made the shape obvious.

Where it now lives. The error is in BaseLlmFlow.getRequestProcessorFromTools, so it fires only on a real collision and covers McpAsyncToolset, FunctionTool and AgentTool through the one path. The McpToolset list is gone.

Detecting the collision needed one thing I had to check first. A declaration-less tool never enters LlmRequest.tools() at all — BaseTool.processLlmRequest returns before the only appendTools call:

if (declaration().isEmpty()) {
  return Completable.complete();          // returns here
}
b.appendTools(ImmutableList.of(this));    // never reached

and the in-model tools override it to write into the request config instead (GoogleSearchTool, UrlContextTool, LoadArtifactsTool, ListSkillsTool each call appendTools zero times). So the declaration-less half cannot be read off the built request. The check reads it from agent.toolsUnion() plus each baseToolset.getTools(...), filtered on declaration().isEmpty(), and intersects with the built request's tools() map.

Only that direction is rejected, so two declaration-less tools may still share a name and default-named ExampleTools keep working.

Tests. The collision case runs the in-model tool both before and after the clashing function tool, as you asked, plus a negative case for the both-declaration-less setup. I confirmed each new test fails with the guard reverted:

expected java.lang.IllegalArgumentException to be thrown, but nothing was thrown

Full core suite: 1856 tests, 0 failures. Built and run with ./mvnw on JDK 25; CI pins 17, and I have not run it under 17.

PR description now carries the revision history and the note you asked for: an agent with both GoogleSearchTool and its own google_search function will start failing, and the fix is to rename one.

One correction to my earlier comment on this thread: I said I could not build core because there was no Gradle wrapper. That was wrong — this repo uses Maven, ./mvnw is in the tree, and it works. My apology for the detour; the rework would have been done a round earlier if I had checked which build system the repo actually uses.

[sushant-me]

Rebased onto current main and re-verified, since the branch had drifted 1286 commits behind and this change sits in a high-fan-in file that upstream has been editing (BaseLlmFlow.java moved 205 lines in the meantime).

The rebase was clean — no conflicts — and my change is unchanged in substance: still BaseLlmFlow.getRequestProcessorFromTools, still 2 files, +107/-0.

Re-ran everything against the new base rather than assuming the old results carried over:

core: 1997 tests, 0 failures, 0 errors, 0 skipped        (./mvnw -pl core test)
BaseLlmFlowTest: 29 tests, 0 failures

And the revert-check again, on the new base, to confirm the test still pins the behaviour rather than passing because the code happens to line up:

[ERROR] ...rejectsDeclarationlessNameCollision:989 expected
        java.lang.IllegalArgumentException to be thrown, but nothing was thrown

One thing I want to be explicit about: the earlier +316/-1 figure included the McpToolset guard. That is gone — McpToolset.java and McpToolsetTest.java are byte-identical to main now. The +107/-0 is the whole change.

[sushant-me]

@MiloszSobczyk — the rework landed in 8aa485df6 and the five commits under it (all pushed 2026-10-02), and I think it takes the direction you asked for rather than the reserved list.

Both points you raised:

1. The check moved out of McpToolset. It now runs in BaseLlmFlow (core/.../flows/llmflows/BaseLlmFlow.java, +46 with +61 of tests), which is the layer that can actually see the agent's other tools — the reason you gave for preferring that placement.

2. McpAsyncToolset is covered by construction, not by a second copy. Sitting in BaseLlmFlow puts it upstream of both toolsets, so initTools() no longer needs its own guard and there is no path that wraps a server tool without passing through the check.

The two commits worth reading are:

• fix(llmflows): move the reserved-name check out of McpToolset
• fix(llmflows): reject a declaration-less tool name that collides with a function tool

So the behaviour you objected to — a server refused for a name that collides with nothing in the agent — is gone, because the decision is now made where the surrounding tool set is visible.

I could not re-request your review from the API (the endpoint returns 404 for a contributor without push access), which is why I am flagging it here instead. Happy to adjust if the placement still is not what you had in mind.

[sushant-me]

Correction to my previous comment, because I described the mechanism wrong and would rather fix it than let it stand.

I said the reserved-name check "now runs in BaseLlmFlow". That is not what the diff does. There is no reserved-name list in BaseLlmFlow — the change there is a guard on the existing duplicate name:

new IllegalArgumentException("Duplicate tool name: " + name)

So the rework is the alternative you asked for, but by a different route than I stated: rather than moving the reserved list to a layer that can see the other tools, it drops the hardcoded list and lets the framework's own duplicate detection do the work. A name only fails when something actually collides — which is exactly the behaviour you wanted when you said McpToolset "can't tell a real collision from a harmless name".

The two points stand as answered:

• out of McpToolset — correct, there is no reserved list left in it;
• McpAsyncToolset — covered because the guard sits in the shared flow rather than in either toolset.

Apologies for the muddled first description. The commits are still 8aa485df6 and the five under it if you want to read them directly.

[kvmilos]

Thanks for the rework. Two blockers, inline: the new test doesn't compile in Google's internal build, and the check lists every toolset a second time on each model call.

On the description:

• The title still describes the MCP approach. Maybe fix(flows): reject a function tool that shares a name with a built-in tool.
• load_artifacts and list_skills have function declarations, so they aren't in-model tools.
• allowsTwoDeclarationlessToolsSharingAName passes without the guard, so "each new test fails with the guard reverted" isn't accurate.
• Please note that this differs from ADK Python, which lets an in-model tool and a same-named function tool coexist.

Could you also add a test where the declaration-less tool comes from a toolset?

Please rebase on main too; zizmor fails only on the old workflow files.

[sushant-me]

Thanks both — I've worked through every item. Summary of the current state:

BaseLlmFlow.java

• :194 — the check no longer makes a second pass over agent.toolsUnion(). That pass re-listed every toolset on every model call, which for McpToolset meant an extra tools/list request per step. Declaration-less names are now collected while the tools' own request processors run.
• :198 — a tool only counts when it actually adds an entry to the request's config tools. It is measured by comparing the config's tool count across each processLlmRequest call, so an ExampleTool named lookup beside a function tool lookup is no longer a collision. No class list.
• :171 — the javadoc is down to three sentences.

BaseLlmFlowTest.java

• :1044 — now asserts on the returned request instead of discarding the blockingGet() result. Thank you for catching that one; GitHub CI doesn't run Error Prone, so I would not have seen it.
• :987 — split into two tests, one per order, no boolean flag.
• :996 — uses GoogleSearchTool.INSTANCE.
• Added a regression test pinning the :198 rule.

Design — the rule now lives in the flow, and McpToolset no longer carries a reserved-name list, so McpToolset.java:311 (the check running before isToolSelected, which let toolFilter block unrelated tools) no longer applies.

Verified locally, since the validation job sits at action_required for fork PRs and so has not compiled this branch:

./mvnw -o -pl core test -Dtest=BaseLlmFlowTest
Tests run: 31, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS (Google Java Format clean)

[kvmilos]

Thanks for the update. The double listing, the compile error and the ExampleTool case are fixed. A few things are still open:

• Please rebase on main and squash this into a single commit. The repo takes one commit per PR (see "Single Commit" in CONTRIBUTING.md), and check-commit-count fails at five. For later rounds, git commit --amend plus git push --force-with-lease keeps it at one.
• The title and description still describe the earlier version: the fix(mcp) title, load_artifacts and list_skills as in-model tools, the second pass under "How the collision is detected", the old test names, and "each new test fails with the guard reverted".
• Could you add a test where GoogleSearchTool sits next to a differently named function tool and the request goes through, and one where the clashing tool comes from a toolset? Today a check that rejected every built-in, or a toolset branch that skipped applyTool, would still pass every test.