From 1ed478ac0865b2cd6242d979c1e7f5913ca398c2 Mon Sep 17 00:00:00 2001 From: Sushant Poudel <215148952+sushant-me@users.noreply.github.com> Date: Fri, 9 Oct 2026 21:48:09 +0545 Subject: [PATCH] fix(flows): reject a function tool that shares a name with a built-in tool A function tool whose name collides with a built-in (in-model) tool was accepted, so two tools answered to one name and which one ran depended on lookup order. The collision is now rejected where the tools are assembled, rather than inside McpToolset, and a declaration-less config tool that adds no entry is recorded in the existing pass so it is not skipped. Scope, stated because it differs by framework: this rejects a *function* tool that shares a name with a built-in. `load_artifacts` and `list_skills` declare functions, so they are not in-model tools and are not part of the reserved set. ADK Python permits an in-model tool and a same-named function tool to coexist; this change does not, which is deliberate for this repository. Tests assert on the processed request rather than the raw config, and use the real search tool, so a passing test means the collision was actually refused. Rebased on main and squashed to the single commit CONTRIBUTING.md requires. --- .../adk/flows/llmflows/BaseLlmFlow.java | 68 ++++++- .../adk/flows/llmflows/BaseLlmFlowTest.java | 180 ++++++++++++++++++ 2 files changed, 240 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/com/google/adk/flows/llmflows/BaseLlmFlow.java b/core/src/main/java/com/google/adk/flows/llmflows/BaseLlmFlow.java index b576a299e..39b26db30 100644 --- a/core/src/main/java/com/google/adk/flows/llmflows/BaseLlmFlow.java +++ b/core/src/main/java/com/google/adk/flows/llmflows/BaseLlmFlow.java @@ -45,6 +45,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.genai.types.FunctionResponse; +import com.google.genai.types.GenerateContentConfig; import io.opentelemetry.api.trace.Span; import io.opentelemetry.api.trace.StatusCode; import io.opentelemetry.context.Context; @@ -56,7 +57,9 @@ import io.reactivex.rxjava3.disposables.Disposable; import io.reactivex.rxjava3.observers.DisposableCompletableObserver; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; @@ -124,15 +127,17 @@ private Flowable preprocess( RequestProcessor getRequestProcessorFromTools(LlmAgent agent) { return (context, request) -> { ReadonlyContext readonlyContext = new ReadonlyContext(context); + // In-model built-in names, recorded as each tool's request processor runs. They are + // collected here because this is the point at which both the assembled config tools and + // the tools' own contributions are visible, so no separate pass over the toolsets is + // needed to find them. + Set declarationlessConfigNames = new HashSet<>(); List> processors = new ArrayList<>(); for (Object toolOrToolset : agent.toolsUnion()) { if (toolOrToolset instanceof BaseTool baseTool) { processors.add( - (builder, ctx) -> { - Completable c = baseTool.processLlmRequest(builder, ctx); - return c == null ? Completable.complete() : c; - }); + (builder, ctx) -> applyTool(baseTool, builder, ctx, declarationlessConfigNames)); } else if (toolOrToolset instanceof BaseToolset baseToolset) { // First apply the toolset's own request processor, then unwrap all tools from the toolset // and apply each individual tool's request processor sequentially. @@ -143,10 +148,7 @@ RequestProcessor getRequestProcessorFromTools(LlmAgent agent) { return toolsetProcessor .andThen(baseToolset.getTools(readonlyContext)) .concatMapCompletable( - b -> { - Completable tc = b.processLlmRequest(builder, ctx); - return tc == null ? Completable.complete() : tc; - }); + b -> applyTool(b, builder, ctx, declarationlessConfigNames)); }); } else { throw new IllegalArgumentException( @@ -159,12 +161,62 @@ RequestProcessor getRequestProcessorFromTools(LlmAgent agent) { ToolContext toolContext = ToolContext.builder(context).build(); return Flowable.fromIterable(processors) .concatMapCompletable(f -> f.apply(builder, toolContext)) + .andThen( + Completable.fromAction( + () -> { + Map declared = builder.build().tools(); + for (String name : declarationlessConfigNames) { + if (declared.containsKey(name)) { + throw new IllegalArgumentException("Duplicate tool name: " + name); + } + } + })) .andThen( Single.fromCallable( () -> RequestProcessingResult.create(builder.build(), ImmutableList.of()))); }; } + /** + * Runs one tool's request processor, recording the tool's name when it adds an entry to the + * request's config tools without adding one to {@link LlmRequest#tools()}. + * + *

Those are the in-model built-ins such as {@code google_search}: they never reach {@link + * LlmRequest#tools()}, so a function tool of the same name sits beside them and the model chooses + * between two things answering to one name. A tool that adds no config entry, such as {@code + * ExampleTool}, is not one of them and is left alone. + * + *

The condition is the two counts changing differently rather than the absence of a + * declaration. {@code VertexAiRagRetrieval} on Vertex declares a function yet still adds to the + * config without reaching {@code tools()}, so testing for a missing declaration misses it; and a + * tool that only contributed the shared function-declarations entry would be counted by a + * config-count test alone. + */ + private static Completable applyTool( + BaseTool tool, + LlmRequest.Builder builder, + ToolContext toolContext, + Set declarationlessConfigNames) { + int configToolsBefore = configToolCount(builder); + int requestToolsBefore = builder.build().tools().size(); + Completable result = tool.processLlmRequest(builder, toolContext); + Completable nonNullResult = result == null ? Completable.complete() : result; + return nonNullResult.doOnComplete( + () -> { + // Grew the config's tools but contributed nothing to the request's own tool map: an + // in-model built-in. Both counts are compared after the processor has run. + if (configToolCount(builder) > configToolsBefore + && builder.build().tools().size() == requestToolsBefore) { + declarationlessConfigNames.add(tool.name()); + } + }); + } + + /** Number of tools currently set on the request's config. */ + private static int configToolCount(LlmRequest.Builder builder) { + return builder.build().config().flatMap(GenerateContentConfig::tools).map(List::size).orElse(0); + } + /** * Post-processes the LLM response after receiving it from the LLM. Executes all registered {@link * ResponseProcessor} instances. Emits events for the model response and any subsequent function diff --git a/core/src/test/java/com/google/adk/flows/llmflows/BaseLlmFlowTest.java b/core/src/test/java/com/google/adk/flows/llmflows/BaseLlmFlowTest.java index 7da99cef0..ddeba56c4 100644 --- a/core/src/test/java/com/google/adk/flows/llmflows/BaseLlmFlowTest.java +++ b/core/src/test/java/com/google/adk/flows/llmflows/BaseLlmFlowTest.java @@ -40,6 +40,7 @@ import com.google.adk.testing.TestLlm; import com.google.adk.tools.BaseTool; import com.google.adk.tools.BaseToolset; +import com.google.adk.tools.GoogleSearchTool; import com.google.adk.tools.ToolContext; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -983,6 +984,185 @@ public void close() {} .containsExactly("toolset-instruction\n\ntool-instruction"); } + @Test + public void getRequestProcessorFromTools_rejectsDeclarationlessNameCollision_inModelFirst() { + assertDeclarationlessCollisionRejected( + ImmutableList.of(GoogleSearchTool.INSTANCE, googleSearchFunctionTool())); + } + + @Test + public void getRequestProcessorFromTools_rejectsDeclarationlessNameCollision_functionToolFirst() { + assertDeclarationlessCollisionRejected( + ImmutableList.of(googleSearchFunctionTool(), GoogleSearchTool.INSTANCE)); + } + + /** A function tool answering to the same name as the built-in search tool. */ + private static BaseTool googleSearchFunctionTool() { + return new BaseTool("google_search", "function search") { + @Override + public Optional declaration() { + return Optional.of(FunctionDeclaration.builder().name("google_search").build()); + } + }; + } + + @Test + public void getRequestProcessorFromTools_allowsDeclarationlessToolAddingNoConfigEntry() { + // A declaration-less tool that adds nothing to the request's config tools is not a built-in + // answer, so it does not collide with a function tool of the same name. ExampleTool is the + // real-world case; a plain declaration-less BaseTool stands in for it here. + BaseTool declarationless = new BaseTool("lookup", "adds no config entry") {}; + + BaseTool functionTool = + new BaseTool("lookup", "function lookup") { + @Override + public Optional declaration() { + return Optional.of(FunctionDeclaration.builder().name("lookup").build()); + } + }; + + LlmAgent agent = + createTestAgentBuilder(createTestLlm(LlmResponse.builder().build())) + .tools(ImmutableList.of(declarationless, functionTool)) + .build(); + + InvocationContext invocationContext = createInvocationContext(agent); + BaseLlmFlow baseLlmFlow = createBaseLlmFlowWithoutProcessors(); + RequestProcessor requestProcessor = baseLlmFlow.getRequestProcessorFromTools(agent); + + LlmRequest processedRequest = + requestProcessor + .processRequest(invocationContext, LlmRequest.builder().build()) + .map(RequestProcessingResult::updatedRequest) + .blockingGet(); + + // The function tool is the only one that reaches the request's tools. + assertThat(processedRequest.tools()).containsKey("lookup"); + } + + private void assertDeclarationlessCollisionRejected(List tools) { + // GoogleSearchTool.INSTANCE makes no network calls, so it is safe to use directly here. + // The caller passes the ordered tool list so the order under test is visible at the call. + LlmAgent agent = + createTestAgentBuilder(createTestLlm(LlmResponse.builder().build())).tools(tools).build(); + + InvocationContext invocationContext = createInvocationContext(agent); + BaseLlmFlow baseLlmFlow = createBaseLlmFlowWithoutProcessors(); + RequestProcessor requestProcessor = baseLlmFlow.getRequestProcessorFromTools(agent); + + IllegalArgumentException thrown = + assertThrows( + IllegalArgumentException.class, + () -> + requestProcessor + .processRequest(invocationContext, LlmRequest.builder().build()) + .blockingGet()); + assertThat(thrown).hasMessageThat().isEqualTo("Duplicate tool name: google_search"); + } + + @Test + public void getRequestProcessorFromTools_allowsBuiltInBesideDifferentlyNamedFunctionTool() { + // Positive control for the two rejection tests above. Those tests would all still pass if the + // guard rejected EVERY built-in, because each of them expects an exception. This one fails in + // that case: the names differ, so nothing collides and the request must go through. + BaseTool inModel = GoogleSearchTool.INSTANCE; + + BaseTool functionTool = + new BaseTool("lookup", "function lookup") { + @Override + public Optional declaration() { + return Optional.of(FunctionDeclaration.builder().name("lookup").build()); + } + }; + + LlmAgent agent = + createTestAgentBuilder(createTestLlm(LlmResponse.builder().build())) + .tools(ImmutableList.of(inModel, functionTool)) + .build(); + + InvocationContext invocationContext = createInvocationContext(agent); + BaseLlmFlow baseLlmFlow = createBaseLlmFlowWithoutProcessors(); + RequestProcessor requestProcessor = baseLlmFlow.getRequestProcessorFromTools(agent); + + LlmRequest processedRequest = + requestProcessor + .processRequest(invocationContext, LlmRequest.builder().build()) + .map(RequestProcessingResult::updatedRequest) + .blockingGet(); + + // The differently named function tool reaches the request's tools; the built-in does not. + assertThat(processedRequest.tools()).containsKey("lookup"); + } + + @Test + public void getRequestProcessorFromTools_rejectsCollisionFromToolsetServedBuiltIn() { + // Covers the toolset branch of applyTool. A toolset that serves a built-in must be counted as + // a built-in exactly like an agent-level one, so a same-named agent-level function tool still + // collides. Without this test a toolset branch that skips applyTool passes every other test. + BaseToolset toolset = + new BaseToolset() { + @Override + public Flowable getTools(ReadonlyContext readonlyContext) { + return Flowable.just(GoogleSearchTool.INSTANCE); + } + + @Override + public void close() {} + }; + + BaseTool agentLevelFunctionTool = + new BaseTool("google_search", "function search") { + @Override + public Optional declaration() { + return Optional.of(FunctionDeclaration.builder().name("google_search").build()); + } + }; + + LlmAgent agent = + createTestAgentBuilder(createTestLlm(LlmResponse.builder().build())) + .tools(toolset, agentLevelFunctionTool) + .build(); + + InvocationContext invocationContext = createInvocationContext(agent); + BaseLlmFlow baseLlmFlow = createBaseLlmFlowWithoutProcessors(); + RequestProcessor requestProcessor = baseLlmFlow.getRequestProcessorFromTools(agent); + + IllegalArgumentException thrown = + assertThrows( + IllegalArgumentException.class, + () -> + requestProcessor + .processRequest(invocationContext, LlmRequest.builder().build()) + .blockingGet()); + assertThat(thrown).hasMessageThat().isEqualTo("Duplicate tool name: google_search"); + } + + @Test + public void getRequestProcessorFromTools_allowsTwoDeclarationlessToolsSharingAName() { + // Both are declaration-less, so nothing is dispatched by name and no name is taken. Two + // default-named ExampleTools must keep working. + BaseTool first = new BaseTool("same_name", "first") {}; + BaseTool second = new BaseTool("same_name", "second") {}; + + LlmAgent agent = + createTestAgentBuilder(createTestLlm(LlmResponse.builder().build())) + .tools(ImmutableList.of(first, second)) + .build(); + + InvocationContext invocationContext = createInvocationContext(agent); + BaseLlmFlow baseLlmFlow = createBaseLlmFlowWithoutProcessors(); + RequestProcessor requestProcessor = baseLlmFlow.getRequestProcessorFromTools(agent); + + LlmRequest processedRequest = + requestProcessor + .processRequest(invocationContext, LlmRequest.builder().build()) + .map(RequestProcessingResult::updatedRequest) + .blockingGet(); + + // Neither tool declares anything, so neither contributes an entry to the request's tools. + assertThat(processedRequest.tools()).isEmpty(); + } + @Test public void getRequestProcessorFromTools_throwsOnUnsupportedType() { LlmAgent agent =