Skip to content

Fix: preserve EventActions when a long-running tool returns no response - #253

Closed
AmaadMartin wants to merge 11 commits into
mainfrom
fix/long-running-tool-actions-loss
Closed

Fix: preserve EventActions when a long-running tool returns no response#253
AmaadMartin wants to merge 11 commits into
mainfrom
fix/long-running-tool-actions-loss

Conversation

@AmaadMartin

Copy link
Copy Markdown
Owner

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: When a long-running tool returns no response, every EventActions mutation it recorded on its ToolContext is silently discarded.

In handleFunctionCallList the per-call loop short-circuits on tool.isLongRunning && !functionResponse before the function-response event is built, and that createEvent(...) is the only place actions: toolContext.actions is ever attached. So a tool that calls toolContext.state.set(...), saveArtifact(...), requestCredential(...), requestConfirmation(...), or sets skipSummarization / escalate / transferToAgent and then returns nothing loses all of it:

  • Nothing is persisted — session state is only applied from event.actions.stateDelta when an event is appended.
  • If every call in the batch is such a tool, no event is produced at all, so LlmAgent.postprocess returns early and no auth or confirmation request ever reaches the client.
  • In a mixed batch the merged event only folds in actions from events that were pushed, so the silent tool's actions are still lost.

Solution: Emit a content-less (actions-only) event carrying just those actions, matching the Python SDK's intended behavior and the content-less event shape ADK JS already supports (getContents skips events without content.role).

  • isDefaultEventActions (core/src/events/event_actions.ts) reports whether an EventActions is still entirely at its defaults. An explicitly set falsy scalar such as escalate: false counts as non-default; that keeps the predicate an honest object-vs-default comparison and is harmless, since the resulting event has no content and changes no loop or escalation behavior. It is kept module-internal — no public export until a caller outside the package needs it.
  • handleFunctionCallList contributes a content-less event with the tool's actions when they are non-default, and still contributes nothing (returning null for a lone call) when they are not, so the existing "no event for a pending long-running call" contract is preserved.
  • generateAuthEvent and generateRequestConfirmationEvent now read content?.role ?? 'user' instead of content!.role. Those are the two functions postprocess calls on the returned event, i.e. exactly the auth / confirmation paths this fix unblocks, and they would otherwise throw a TypeError on a content-less event. 'user' is the role every function-response event these consume is already built with. No new validation or throw sites were added.
  • LlmAgent's isEmptyMetadataEvent check is narrowed with isDefaultEventActions(lastEvent.actions). The actions-only event matches the shape of the trailing empty streaming STOP chunk that clause exists for (agent-authored, not partial, no content parts, in a step that had tool calls), so without this the loop would suppress the break and issue an extra model turn while the long-running call is still pending. A trailing empty STOP chunk carries default actions, so the streaming behavior the clause was added for is unchanged. Setting endInvocation in postprocess was rejected as an alternative because it would kill the transferToAgent follow-up that this fix makes reachable for long-running tools.

mergeParallelFunctionResponseEvents needed no change — it already guards on event.content && event.content.parts and merges every event's actions — so that tolerance is pinned by test rather than by edit.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

core/test/events/event_actions_test.ts covers isDefaultEventActions for default actions and for every non-default field, including the explicitly-false scalar case.

core/test/agents/functions_test.ts covers: a silent long-running tool that touches nothing still yields null; one that records a stateDelta, skipSummarization, transferToAgent or a tool confirmation yields a content-less event carrying it; a mixed batch merges the silent tool's actions into an event whose content holds only the responding tool's part; a long-running tool that does respond and a non-long-running tool returning undefined both behave exactly as before; and both event generators produce role: 'user' from a content-less event.

core/test/agents/llm_agent_test.ts drives the agent with a turn-counting stub model and asserts the step loop stops after the actions-only event (model called exactly once, no second-turn text), and that a trailing empty chunk with default actions still lets the loop continue.

core/test/agents/long_running_tool_actions_integration_test.ts runs the whole path through InMemoryRunner: the state delta of a silent long-running tool is persisted to the session, and its requestCredential call surfaces an adk_request_credential function call to the client — neither of which happened before.

Commands run locally on the pushed commit:

npx vitest run --project unit:core core/test/agents/functions_test.ts core/test/events/event_actions_test.ts core/test/agents/llm_agent_test.ts core/test/agents/long_running_tool_actions_integration_test.ts core/test/agents/processors core/test/auth
npm run build
npm run lint
npm run format:check
npm run docs:check

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

Register a long-running tool that mutates its tool context and returns nothing, then run an agent that calls it:

const startJob = new LongRunningFunctionTool({
  name: 'startJob',
  description: 'starts a background job',
  execute: async (_args, toolContext) => {
    toolContext!.state.set('pendingJob', 'job-123');
    return undefined;
  },
});

const runner = new InMemoryRunner({
  agent: new LlmAgent({
    name: 'job_agent',
    model: '<your model>',
    tools: [startJob],
  }),
  appName: 'demo',
});

Ask the agent to start the job and iterate the events. Before this change the run produced no tool event and session.state['pendingJob'] was undefined; now the run emits one event with content === undefined and actions.stateDelta = {pendingJob: 'job-123'}, the session state contains pendingJob, and the agent does not take an extra model turn while the call is pending.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

AmaadMartin and others added 11 commits July 28, 2026 14:48
* 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>
A long-running tool that returns no response had every mutation it
recorded on its tool context (state/artifact deltas, auth or
confirmation requests, transfer, escalation, skipSummarization)
silently discarded, because the per-call loop skipped straight past the
only place actions are attached to an event.

Emit a content-less event carrying just those actions when the tool
left them non-default, keep emitting nothing when it did not, and make
the auth / confirmation event generators tolerate a content-less event.
Narrow the step loop's empty-metadata escape hatch to genuinely empty
events so the actions-only event still terminates the step.
Unit tests for the isDefaultEventActions predicate, the actions-only
event (single, mixed and all-silent batches, both call orders), the
content-less path through the auth and confirmation event generators,
and the step-loop termination guard. Adds a runner-level integration
test proving the state delta is persisted and the credential request
reaches the client.
Fold the per-field actions-only assertions into one parameterised case,
drop the batch and auth-guard tests that re-exercise an already covered
path, inline a single-use fixture, and make the counting mock fail loudly
instead of replaying its last turn forever.
Parameterise the isDefaultEventActions non-default cases and keep a
single call order for the mixed batch, which selects the same code path
either way.
Drop the public barrel re-export until an out-of-package caller exists,
fold the stateDelta case into the parameterised table, and trim the
comment lines that restated the code.
@AmaadMartin
AmaadMartin force-pushed the fix/long-running-tool-actions-loss branch from 1b5d460 to b395f37 Compare July 29, 2026 18:05
@AmaadMartin

Copy link
Copy Markdown
Owner Author

Automated: ported to upstream as google#571.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants