fix(tool): apply WithConfirmation to streaming tools - #1409
Open
venkat-uk wants to merge 2 commits into
Open
Conversation
confirmationToolset.Tools wrapped a tool only when it satisfied the unexported runnableTool interface, which is Declaration plus Run. The flow can execute two shapes: toolinternal.FunctionTool and toolinternal.StreamingFunctionTool, and it dispatches on the streaming one first. A streaming tool has Declaration and RunStream but no Run, so it fell through the else branch and was returned unwrapped. The toolset-level confirmation setting never reached it, and the only gate left was the tool's own Config.RequireConfirmation. Add a streaming counterpart to the wrapper and check the streaming shape first, matching the flow's dispatch order. The wrapper embeds the streamingRunnableTool interface rather than the concrete tool, so a tool that also has a Run method cannot reach the flow through an unguarded promoted method. Tools that ADK does not execute itself, such as the model-side built-ins in tool/geminitool, still pass through unwrapped: there is nothing to confirm. The godoc now says which shapes are covered instead of naming one internal interface. The confirmation decision itself moves into confirmCall so the streaming and non-streaming paths cannot drift. Fixes google#1408
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
Problem:
tool.WithConfirmationwrapped a tool only when it satisfied the unexportedrunnableToolinterface, which isDeclarationplusRun. The flow can execute two shapes, not one:toolinternal.FunctionToolandtoolinternal.StreamingFunctionTool, andinternal/llminternal/base_flow.godispatches on the streaming shape first (line 1156) and the non-streaming one second (line 1211).A streaming tool has
DeclarationandRunStreambut noRun, so it fell through theelsebranch and was appended unchanged. A caller who wrapped a toolset to gate every tool in it behind human-in-the-loop confirmation got no gate at all on the streaming tools, and no signal that this had happened. The only remaining gate was the tool's ownConfig.RequireConfirmation, which is a different control set by whoever wrote the tool rather than by whoever wrapped the toolset.Solution:
Add a streaming counterpart to the wrapper and check the streaming shape first, so the wrapper matches the order in which the flow will dispatch.
Two details are deliberate:
confirmationStreamingToolembeds thestreamingRunnableToolinterface rather than the concrete tool. Go promotes the methods of the embedded static type, so a tool that also has aRunmethod cannot reach the flow through an unguarded promoted method. The existingconfirmationToolembedsrunnableToolfor the same reason, so this keeps the two symmetric.tool/geminitoolare run by the model, so there is nothing to confirm. Thedefaultbranch now says that, instead of describing the skipped tools as ones lacking a specific internal interface.The confirmation decision moves into one
confirmCallhelper used by both paths, so the streaming and non-streaming semantics cannot drift apart later. Behaviour for non-streaming tools is unchanged.The godoc no longer names
runnableTool, which was not a useful thing to tell a caller: it described the skipped tools as ones that do not "provide a FunctionDeclaration and a Run method", and a streaming tool does provide a FunctionDeclaration, so the sentence pointed a reader away from the gap.Testing Plan
Unit Tests:
TestWithConfirmation_StreamingToolis a table test that mirrors the existingTestWithConfirmationcases against a streaming tool built withRequireConfirmation: false, so only the toolset-level setting is in play: confirmation required with nothing on the context, confirmed, rejected, not required, provider true, andrequireConfirmation=truewith a provider returning false.TestWithConfirmation_ToolWithBothRunAndRunStreamcovers the second path. It uses a tool implementing both shapes and asserts the wrapper presents the streaming shape, does not also satisfyFunctionTool, and that neither method ran without confirmation.Both fail on
main:and pass with the change, alongside the existing suite:
go vet ./tool/is clean.Manual End-to-End (E2E) Tests:
The behaviour is observable without a model, which is why the reproduction is a test rather than a runner transcript. Before the change,
cts.Tools(nil)returns the same pointer that went in (tools[0] == streamToolistrue), the handler runs, andRequestConfirmationis never called. After it, the same call returnsErrConfirmationRequired, setsSkipSummarization, and the handler does not run until a confirmation arrives on the context.Checklist
Additional context
WithConfirmationis marked EXPERIMENTAL and out of scope for the v1.0 API, so this is a fix within an experimental surface rather than a public API change. The exported signature is unchanged.Adjacent but deliberately not in this PR: #1153 item 2 tracks test coverage for the tool's own
RunStreamconfirmation gate, which is a different control and already works. Happy to split anything here if you would rather see the wrapper and the helper extraction land separately.