Skip to content

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

Open
AmaadMartin wants to merge 9 commits into
google:mainfrom
AmaadMartin:fix/long-running-tool-actions-loss
Open

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

Conversation

@AmaadMartin

Copy link
Copy Markdown
Collaborator

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):

No existing issue.

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 (core/src/agents/functions.ts) 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 that 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 — it is not added to the package's public exports in core/src/common.ts 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 they consume is already built with. No new validation or throw sites were added.
  • LlmAgent's isEmptyMetadataEvent check (core/src/agents/llm_agent.ts) 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

Unit Tests:

  • I have added or updated unit tests for my change.

  • 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 this 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
# -> Test Files 4 passed (4), Tests 92 passed (92)

npx vitest run --project unit:core --project unit:dev
# -> Test Files 177 passed, Tests 2524 passed
# The only non-passing test, dev/test/cli/cli_create_test.ts "should handle
# Vertex AI selection with gcloud defaults", fails identically on an unmodified
# main: it reads the developer machine's real gcloud project/region instead of
# the mocked values. This PR touches no file under dev/.

npm run build
npm run lint
npm run format:check
npm run docs:check
npx secretlint "**/*"
# -> all clean

Manual End-to-End (E2E) Tests:

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. This scenario is also asserted automatically in core/test/agents/long_running_tool_actions_integration_test.ts.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • I have manually tested my changes end-to-end.
  • Any dependent changes have been merged and published in downstream modules.

Additional context

This change is behavior-preserving for every tool that already returns a response, and for long-running tools that record no actions: in both cases the emitted events are byte-for-byte what they were before. The only new event shape is a content-less event, which the existing content assembly (getContents) and parallel-response merge (mergeParallelFunctionResponseEvents) already tolerate.

Amaad Martin added 5 commits July 29, 2026 11:04
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.
Amaad Martin added 4 commits July 31, 2026 15:04
Temporarily restores the files this branch touches to their state at the
merge base so the following merge of upstream/main applies without
conflicts. Reapplied in the commit after the merge.
Restores the parked changes on top of upstream's normalized callback
handling. The long-running skip now tests `functionResponse == null`
rather than falsiness, so the actions-only event is emitted from inside
that nullish branch; a falsy-but-present response keeps taking the
normal response path introduced upstream.

Drops the branch's null-response and undefined-response regression tests:
upstream now covers both branches directly.
Replaces the last `as unknown as Session` this branch added with the
repo's own factory, so the helper carries a real Session.
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.

1 participant