Fix: resolve the exit_loop built-in tool in YAML agent configs - #206
Closed
AmaadMartin wants to merge 9 commits into
Closed
Fix: resolve the exit_loop built-in tool in YAML agent configs#206AmaadMartin wants to merge 9 commits into
AmaadMartin wants to merge 9 commits into
Conversation
* docs: document minimum supported Node.js version in README Add a short prerequisite note under the Installation section stating that ADK for TypeScript requires Node.js 18 or newer, so new users know which Node.js runtime they need before running npm install @google/adk. The version reflects the mandated fallback: no engines.node field is declared in any package.json in the repo. * docs: reference current Node.js LTS instead of a fixed version Node.js 18 is EOL and any hard-coded minimum version goes stale over time. Reword the installation prerequisite to point readers at the current Node.js LTS releases, which stays accurate without future edits. Addresses PR review feedback on #526. --------- Co-authored-by: Amaad Martin <amaadmartin@google.com>
…#536) `ToolAuthHandler` accepted an `authCredential` and then never read it. When no auth response was present it went straight to `requestCredential()`, so a credential handed to `OpenAPIToolset`/`RestApiTool` at construction time was ignored and the tool returned `{pending: true}` on every call. For `apiKey`, `http` and `serviceAccount` schemes nothing ever resolves that request — no user interaction is involved — so the tool could never complete. Fall back to the configured credential when there is no auth response, which mirrors `_get_auth_response() or self.auth_credential` in adk-python. Also narrow what gets written to session state. The credential store exists to avoid repeating work that either cannot be repeated (an auth response is readable once) or is expensive (an exchange costs a round trip). A static credential that needed no exchange is neither, so it is no longer persisted — that would only copy the developer's secret into the session store.
…hon parity (#542) * Feat: add LoadMcpResourceTool and MCPToolset resource access Port adk-python's LoadMcpResourceTool to adk-js for cross-language parity. - Add listResources/getResourceInfo/readResource to MCPToolset, following the existing create -> try -> closeSession-in-finally session idiom. - Add LoadMcpResourceTool (mirrors the in-repo LoadArtifactsTool idiom): declares load_mcp_resource({resource_names}), and processLlmRequest injects resolved resource contents (text + base64 binary, no decode step) into the LlmRequest. - Export the tool from core/src/index.ts (@google/adk public API). * test: cover LoadMcpResourceTool and MCPToolset resource access Add full unit coverage (100% line + branch of the new code): - load_mcp_resource_tool_test.ts: init, declaration, runAsync (incl. default), list injection (incl. empty + swallowed list errors), text/binary/unknown content, base64 blob passthrough + default mime type, swallowed read errors, and all no-op guard paths (non-matching/absent function response, missing parts). - mcp_toolset_test.ts: listResources/getResourceInfo/readResource happy paths and error paths (unknown name, missing URI), plus session-cleanup assertions for success and failure (closeSession in finally, no leaked sessions). * test(e2e): exercise LoadMcpResourceTool against a real MCP server Add a no-mock end-to-end test that spawns a real MCP server over stdio (mcp_resource_server.mjs, exposing a text and a binary resource) and drives the real MCPToolset + LoadMcpResourceTool: listing/resolving/reading resources and injecting their contents (text + base64 binary) into an LlmRequest. --------- Co-authored-by: Amaad Martin <amaadmartin@google.com>
* feat(tools): add ExampleTool for few-shot examples Port adk-python's ExampleTool to adk-js. The tool accepts a static Example[] or a BaseExampleProvider and, on each outgoing LLM request, appends a few-shot <EXAMPLES> block (built via buildExampleSi from the latest user query) to the system instruction. It is never declared to the model (mirrors PreloadMemoryTool) and is a no-op when no user text is present. Exported from the public @google/adk API. * test(tools): cover ExampleTool unit and end-to-end paths Add Vitest coverage for ExampleTool: static list and provider paths, model-style passthrough, no-op branches (missing user content, empty parts, text-less first part), runAsync throwing, and the public export. Includes an end-to-end block that drives processLlmRequest through a real Context/InvocationContext (no mocks). 100% line/branch coverage of the new tool. * refactor(tools): apply simplicity audit feedback Use a constructor parameter property for `examples` (repo convention), and drop the redundant provider end-to-end test whose only unique aspect was a spy — keeping the no-mock e2e block strictly mock-free. The provider selection path stays fully covered by the unit tests; the tool retains 100% line/branch coverage. --------- Co-authored-by: Amaad Martin <amaadmartin@google.com>
* feat(agents): support clone() for RoutedAgent RoutedAgent derives its routing targets from config.agents rather than subAgents, so the inherited BaseAgent.clone() rebuilt the agent from the already-parented originals and threw "already has a parent agent". Add a RoutedAgent.clone() override that deep-clones the routing targets (via a private cloneRoutingTargets helper) and passes them through the agents override, so super.clone() rebuilds the constructor with fresh, detached copies that are re-parented onto the clone. The array-vs-record shape and record keys are preserved so the clone routes identically, and parent-override rejection plus the detached-root guarantee are still enforced by the base implementation. Remove the now-obsolete "documented limitation" test (and its unused RoutedAgent import) from base_agent_test; positive coverage lives in routed_agent_test. * test(agents): cover RoutedAgent.clone() Add a clone describe suite exercising the new override and the cloneRoutingTargets helper: array and record forms, deep-clone and re-parenting of targets, originals left untouched, functional routing on the clone (record form), verbatim agents override, non-agents overrides, and parentAgent-override rejection. Includes a no-mock end-to-end case that clones a RoutedAgent whose targets are real LlmAgents. --------- Co-authored-by: Amaad Martin <amaadmartin@google.com>
* Feat(tools): add SSRF-safe load_web_page tool for adk-python parity Ports the adk-python load_web_page tool to adk-js. Fetches a URL and returns its extracted, readable text, hardened against SSRF: - only http/https schemes are fetched - localhost-style hostnames and hosts resolving to non-global IPs (private, loopback, link-local, shared/CGNAT, reserved, multicast, IPv4-mapped IPv6) are rejected before any connection - redirects are never followed (redirect: 'manual') - a configurable timeout (default 30s) bounds every request - expected failures return the parity string "Failed to fetch url: <url>" instead of throwing Exposes loadWebPage(), the LOAD_WEB_PAGE FunctionTool, and the LoadWebPageOptions type via the @google/adk public API. * Refactor(tools): inline single-use failure prefix in load_web_page Addresses simplicity-audit feedback: the FAILURE_PREFIX constant had a single caller, so its literal is inlined into failedToFetchMessage, which remains the sole formatter of the parity failure string. --------- Co-authored-by: Amaad Martin <amaadmartin@google.com>
The conformance agent registry mapped every name in BUILTIN_TOOLS to
undefined, so a YAML agent declaring 'tools: [{name: exit_loop}]' was
instantiated with no tools at all. Split the list into built-ins that
resolve to a real tool object (currently just exit_loop -> EXIT_LOOP,
mirroring adk-python's LlmAgent._resolve_tools) and the server-side
built-ins that must keep being dropped because their processLlmRequest
rejects the replay harness' DummyLlm model name.
Returning the recorded tool response short-circuits callToolAsync, so a tool whose only observable effect is on EventActions never runs during a replay. The plugin already replicated that effect for transfer_to_agent; do the same for exit_loop so escalate and skipSummarization are set, which is what stops the LoopAgent and ends the LlmAgent step loop.
ExitLoopTool is implemented and now resolved from YAML configs, so the skip reason no longer holds. Add a TestRunner suite that replays an in-memory equivalent of the case (LoopAgent -> LlmAgent calling exit_loop) as executable proof, since the conformance corpus itself is not vendored in this repo.
AmaadMartin
force-pushed
the
fix/conformance-exit-loop-builtin-tool
branch
from
July 29, 2026 18:32
730a54d to
a519152
Compare
Owner
Author
|
Automated: ported to upstream as google#572. |
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.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
Problem:
ExitLoopTooland theEXIT_LOOPsingleton already exist in core and are exported from@google/adk, but the conformance harness in thedevpackage still behaves as if they did not.dev/src/integration/agent_registry.tslistedexit_loopin aBUILTIN_TOOLSarray whose every entry was mapped toundefinedand filtered out, so a YAML agent declaring:was instantiated with zero tools. During a conformance replay the recorded
exit_loopfunction call then fails withFunction exit_loop is not found in the toolsDict., which is whyworkflow/loop_001was permanently listed inSKIPPED_TESTSwith the (now false) reasonExitLoopTool is not implemented yet.Solution:
Two changes are required, and both are needed for the case to actually pass.
Registry resolution (
dev/src/integration/agent_registry.ts).BUILTIN_TOOLSbecomes aRecord<string, BaseTool>of built-ins that resolve to a real tool object — currently justexit_loop -> EXIT_LOOP— mirroringadk-python'sLlmAgent._resolve_tools, which resolves a bare, dot-less built-in name to the real tool object. The three purely server-side built-ins (google_search,url_context,google_maps_grounding) move to a separateSKIPPED_BUILTIN_TOOLSlist and keep being dropped on purpose: they are executed by the Gemini backend and theirprocessLlmRequestthrows for a non-Gemini model name, and the replay harness injectsDummyLlm(model namedummy-llm). The table is read throughObject.hasOwnso that a YAML string naming an inherited member such asconstructorfalls through tofindToolOrThrowinstead of resolving toObject.prototype.constructor;Object.hasOwnis already the idiom used elsewhere in this package (dev/src/integration/test_runner.ts).Replay side effects (
dev/src/integration/replay_plugin.ts). This is the non-obvious half — reviewers will otherwise read it as unrelated.ReplayPlugin.beforeToolCallbackreturns the recorded tool response, and incore/src/agents/functions.tsa non-null plugin response short-circuitscallToolAsync. The response recorded for a Python tool returningNoneis the dict{result: None}— a non-null object — soEXIT_LOOP.runAsyncwould never execute and neitherescalatenorskipSummarizationwould be set. WithoutescalatetheLoopAgentnever stops; withoutskipSummarizationthe sub-agent asks the model for one more turn and the replay throwsNo LLM recording found .... Both flags are also deep-compared byvalidateSession. The plugin already replicated this class of side effect fortransfer_to_agent; theifbecomes a smallswitchwith anexit_looparm that sets both flags. (adk-pythonsolves this differently — its replay plugin runs the real tool before returning the recording — but porting that would change execution for every replayed tool, so it is deliberately out of scope here.)With both in place,
workflow/loop_001is removed fromSKIPPED_TESTS; that list now holds exactlytool/example_tool_001,core/multi_005andtool/long_running_tool_001.No
core/change, no public API change, and no new dependency:AgentRegistryandReplayPluginare internal to the devtools conformance harness.Testing Plan
Note on the conformance corpus: the replay corpus (
spec.yaml/generated-recordings.yaml/generated-session.yamltriplets) is not vendored in this repo and is not exercised by.github/workflows/validation.yaml. The realadk integration conformancereplay was NOT executed — no corpus was available in the environment.dev/test/integration/test_runner_test.tswas added as the in-repo, CI-runnable substitute: it drives the realTestRunner,AgentRegistry,ReplayPlugin,LoopAgentandLlmAgentwith no mocks, over an in-memory equivalent of theloop_001case, and asserts the produced session deep-equals the expected one — includingescalate/skipSummarizationon the final function-response event, which is only reachable when both halves of this fix are present.Unit Tests:
I have added or updated unit tests for my change.
All unit tests pass locally.
dev/test/integration/agent_registry_test.ts—exit_loopnow resolves to the sharedEXIT_LOOPinstance; the three server-side built-ins are still dropped; an inherited member name such asconstructoris not resolved as a built-in.should skip built-in toolscase assertedexpect(retrieved.tools.length).toBe(0)forexit_loop— it pinned exactly the behaviour this PR fixes, so it becameshould resolve exit_loop to the EXIT_LOOP built-in tool. Its original assertion is not lost: it survives verbatim in the newshould skip server-side built-in toolscase, which applies it togoogle_search/url_context/google_maps_grounding. No other existing test was changed or removed.dev/test/integration/replay_plugin_test.ts(new) — bothswitcharms, the no-side-effect arm, the not-found rejection, and single-consumption of a recording.dev/test/integration/test_runner_test.ts(new) — the end-to-end loop replay described above.Commands run locally on the pushed commit:
Each half of the fix was verified to be load-bearing by reverting it in isolation and re-running
dev/test/integration/test_runner_test.ts:agent_registry.tsresolution, the run fails withTool exit_loop not found in registry;replay_plugin.tsexit_looparm, the loop never terminates and the run fails withNo LLM recording found for agent refiner_agent at turn 0.Pre-existing failures unrelated to this change: the
tests/integration/build_setup,tests/integration/app_loader,tests/integration/agent_loaderande2esuites fail in this sandbox because they shell out to real builds and to the live Gemini API. They fail on the base commit too, and repeated runs on an unchanged tree produce a different subset each time, so they are environmental rather than caused by this diff.Manual End-to-End (E2E) Tests:
Write a YAML agent config for a refiner agent that exits a loop:
Load it via
AgentRegistry.registerAgentConfig+getAgentand confirm the resultingLlmAgent.toolsis[EXIT_LOOP](before this change it was[]).If you have a conformance corpus available:
workflow/loop_001should now run and pass instead of being reported as skipped.Checklist
Additional context
The two unchecked boxes are accurate rather than oversights: the full end-to-end path is the
adk integration conformancereplay, whose corpus is not vendored in this repo and was not available in this environment, and this change has no dependent downstream modules.