From ddda1ce7f41afec93cdb314a304cae59b662d822 Mon Sep 17 00:00:00 2001 From: prasanna8585 Date: Fri, 18 Sep 2026 15:01:58 +0530 Subject: [PATCH] fix(workflow): stop run_llm_agent_as_node mutating the shared node agent run_llm_agent_as_node's single_turn branch defaulted a node's include_contents to 'none' when the caller left it unset, by setting `agent.include_contents = 'none'` directly on the `agent` parameter. That parameter is not a per-invocation object: LlmAgent._run_impl calls `run_llm_agent_as_node(self, ...)`, and `self` is the same LlmAgent instance build_node placed in the workflow graph -- the one every future run of that node reuses. The first invocation to run the node without an explicit include_contents permanently set it to 'none' on that shared instance; every later invocation of the same node then inherited that value regardless of whether it needed the default, since the write was never undone or scoped to the call that made it. include_contents controls whether prior conversation history is included in the LLM request (flows/llm_flows/context/_contents.py reads `agent.include_contents` directly, with no other place a per-invocation override could reach it), so this is content-inclusion behavior silently locked in for a shared, long-lived object by whichever invocation happened to run first, not necessarily what the node's own configuration or a later caller intended. Found while auditing a sibling commit (f8282da) that fixed the identical shared-mutation shape in code_execution.py's `cfc_agent.code_executor = BuiltInCodeExecutor()` -- a mutation of a shared Runner.agent based on one invocation's run_config, replaced there with a per-invocation resolver that never touches the agent. That fix's own commit message says "prevent in-place agent mutation" but only covers one of at least two instances of the pattern; this is the other one, in a different file, untouched by that commit despite it also editing _llm_agent_wrapper.py for unrelated cleanup. A companion mutation two lines above -- `if agent.mode is None: agent.mode = 'single_turn'` -- has the same shape but is not exploitable in practice: every path that reaches run_llm_agent_as_node does so through LlmAgent._run_impl, which is only invoked once the agent is already running as a workflow node, which requires it to have gone through build_node's `agent_node.clone(update=kwargs)` first. build_node always sets `mode` there when it is None, before the cloned agent is ever stored as the node, so `agent.mode` is already non-None by the time run_llm_agent_as_node's check runs -- confirmed by tracing every call site (grep for `run_llm_agent_as_node(` finds exactly one, passing `self`, and grep for `build_node(` finds no path that skips the clone). Left as-is; fixing something already unreachable would add risk (the two writes are adjacent and easy to conflate) without closing a real gap. Fix: clone the agent for this one invocation when the default would otherwise apply, mirroring the pattern build_node itself already uses for the identical clone-then-configure need, including restoring parent_agent (clone() drops it, since it assumes the caller is defining a new, independent agent, which is not the case here -- see build_node's own, identical restoration for the same reason). Skip the clone entirely when include_contents was set explicitly, since no override is needed in that case. Verified: - Reproduced the bug with the existing test (test_single_turn_defaults_include_contents_only_when_unset) before changing it: its own assertion, `assert wrapper.include_contents == expected_include_contents`, was directly asserting the shared node's permanent mutation as the intended contract, which is the bug. The test's mock also captured `wrapper` by closure rather than the object run_async was actually called on (it was set via `object.__setattr__` on the instance, so it never received `self`), which would have silently passed either way once the mutation was removed. Rewrote it to patch LlmAgent.run_async at the class level (so self is captured correctly regardless of which object -- the original or the per- invocation clone -- actually runs) and to assert the two things that actually matter: the effective include_contents seen during the run is still correct, and the shared node's own attribute is never changed by running it. - Reverted the production fix locally and reran the rewritten test: it fails against the vulnerable code (asserts `wrapper.include_contents` stayed at its original value; the reverted code left it permanently changed to 'none') and passes against the fix, for exactly the two parametrized cases where the implicit default applies. The two cases with an explicit include_contents pass unchanged in both versions, as expected, since no clone or mutation happens in that branch either way. - Full tests/unittests/workflow/ suite: 872 passed, 1 skipped, 5 xfailed, 0 failed, both before capturing the fix and after restoring it. --- src/google/adk/workflow/_llm_agent_wrapper.py | 23 +++++++++++++++---- .../workflow/test_llm_agent_as_node.py | 23 +++++++++++++------ 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/src/google/adk/workflow/_llm_agent_wrapper.py b/src/google/adk/workflow/_llm_agent_wrapper.py index 09c306bbe2b..6d0183b3946 100644 --- a/src/google/adk/workflow/_llm_agent_wrapper.py +++ b/src/google/adk/workflow/_llm_agent_wrapper.py @@ -410,8 +410,6 @@ async def run_llm_agent_as_node( ) include_contents_explicit = 'include_contents' in agent.model_fields_set - if agent.mode == 'single_turn' and not include_contents_explicit: - agent.include_contents = 'none' agent_ctx = prepare_llm_agent_context(agent, ctx) prepare_llm_agent_input(agent, agent_ctx, node_input) @@ -446,9 +444,26 @@ async def run_llm_agent_as_node( if agent.mode == 'single_turn': # is_live is always False here (single_turn forces non-live). - async with aclosing(agent.run_async(ic)) as run_iter: + # + # A node in single_turn mode with no explicit include_contents defaults + # to 'none'. That default is invocation-scoped, not part of the agent's + # own configuration, so it is applied on a per-invocation clone rather + # than by mutating `agent` itself: `agent` is the same node instance + # reused across every future run of this workflow, and mutating it here + # would leave the override permanently in place for every other + # invocation of that shared node, single_turn or not. + if include_contents_explicit: + run_agent = agent + else: + run_agent = agent.clone(update={'include_contents': 'none'}) + # clone() drops parent_agent (it assumes the caller is defining a new, + # independent agent); this is a same-invocation stand-in for `agent`, + # so it must keep the same parent as the original. See build_node's + # identical restoration for the same reason. + run_agent.parent_agent = agent.parent_agent + async with aclosing(run_agent.run_async(ic)) as run_iter: async for event in run_iter: - process_llm_agent_output(agent, ctx, event) + process_llm_agent_output(run_agent, ctx, event) yield event return diff --git a/tests/unittests/workflow/test_llm_agent_as_node.py b/tests/unittests/workflow/test_llm_agent_as_node.py index e714b3300d2..32de42c348e 100644 --- a/tests/unittests/workflow/test_llm_agent_as_node.py +++ b/tests/unittests/workflow/test_llm_agent_as_node.py @@ -22,6 +22,7 @@ from __future__ import annotations from typing import Any +from unittest.mock import MagicMock from google.adk.agents.context import Context from google.adk.agents.llm.task._task_models import TaskResult @@ -320,9 +321,13 @@ async def test_single_turn_defaults_include_contents_only_when_unset( agent_kwargs: dict[str, Any], expected_include_contents: str, ): - """Single-turn workflow nodes preserve explicit content inclusion.""" - from unittest.mock import MagicMock + """Single-turn nodes get the right effective include_contents per run, + without permanently mutating the shared node object itself: a node is + reused across every future invocation, so an implicit 'none' default + applied by mutating it in place would stick for every later run, + single_turn or not, explicit or not. + """ agent = LlmAgent( name='test_agent', model='gemini-2.5-flash', @@ -330,17 +335,18 @@ async def test_single_turn_defaults_include_contents_only_when_unset( **agent_kwargs, ) wrapper = build_node(agent) + original_include_contents = wrapper.include_contents seen_include_contents = [] - async def mock_run_async(*args, **kwargs): - seen_include_contents.append(wrapper.include_contents) + async def mock_run_async(self, *args, **kwargs): + seen_include_contents.append(self.include_contents) yield Event( invocation_id='inv', - author=wrapper.name, + author=self.name, content=types.Content(parts=[types.Part(text='ok')]), ) - object.__setattr__(wrapper, 'run_async', mock_run_async) + monkeypatch.setattr(LlmAgent, 'run_async', mock_run_async) monkeypatch.setattr( agent_wrapper, 'prepare_llm_agent_context', @@ -360,8 +366,11 @@ async def mock_run_async(*args, **kwargs): event async for event in wrapper._run_impl(ctx=ctx, node_input='hi') ] + # The effective value used for this run is still correct... assert seen_include_contents == [expected_include_contents] - assert wrapper.include_contents == expected_include_contents + # ...but the shared node itself is never mutated, regardless of whether + # this run needed an implicit override. + assert wrapper.include_contents == original_include_contents assert events[0].content.parts[0].text == 'ok' def test_name_override(self):