fix(workflow): stop run_llm_agent_as_node mutating the shared node agent - #7173
Open
prasanna8585 wants to merge 1 commit into
Open
prasanna8585 wants to merge 1 commit into
prasanna8585 wants to merge 1 commit into
Conversation
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.
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
2 participants
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.
Problem
run_llm_agent_as_node'ssingle_turnbranch defaulted a node'sinclude_contentsto'none'when left unset, by settingagent.include_contents = 'none'directly on theagentparameter. That parameter is not per-invocation:LlmAgent._run_implcallsrun_llm_agent_as_node(self, ...), andselfis the sameLlmAgentinstancebuild_nodeplaced in the workflow graph — the one every future run of that node reuses.The first invocation to run the node without an explicit
include_contentspermanently 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_contentscontrols whether prior conversation history is included in the LLM request, so this is content-inclusion behavior silently locked in by whichever invocation happened to run first.Found while auditing #<f8282da's PR number> ("prevent in-place agent mutation"), which fixed the identical shared-mutation shape in
code_execution.py'scfc_agent.code_executor = BuiltInCodeExecutor()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.pyfor unrelated cleanup.A companion mutation two lines above (
if agent.mode is None: agent.mode = 'single_turn') has the same shape but isn't exploitable in practice — traced every call site and confirmedbuild_node's existing clone-and-set-mode logic always runs first, soagent.modeis already non-Noneby the time this check executes. Left as-is; fixing something already unreachable would add risk without closing a real gap.Fix
Clone the agent for this one invocation when the default would otherwise apply, mirroring the pattern
build_nodealready uses for the identical need — including restoringparent_agent(clone()drops it by design;build_noderestores it for the same reason). Skip the clone entirely wheninclude_contentswas set explicitly.Testing
test_single_turn_defaults_include_contents_only_when_unset) was directly asserting the bug as the intended contract (assert wrapper.include_contents == expected_include_contents— the shared node's permanent mutation). Its mock also capturedwrapperby closure rather than the actual calling object, so it wouldn't have caught this either way. Rewrote it to patchLlmAgent.run_asyncat the class level and assert the two things that matter: the effective value used during the run, and that the shared node itself is never mutated.tests/unittests/workflow/suite: 872 passed, 1 skipped, 5 xfailed, 0 failed — both before and after.