mirror of
https://github.com/google/adk-go.git
synced 2026-10-28 16:44:38 +08:00
* fix(llmagent): resolve agent mode per placement instead of mutating the agent
An LlmAgent that declares no Mode took one from where it was placed, and
that default was written back onto the shared agent: chat for a sub-agent
or a runner root, single_turn for a workflow node. Five sites wrote to
llminternal.State, three of them on the run path.
Because every write was guarded on ModeUnset, the first placement to be
constructed won and later placements silently inherited its mode. The
same agent instance used both as a graph node and as a chat sub-agent
gave a coordinator a different tool set depending on construction order.
The run-path writes were also a data race: one agent instance is
documented to serve many concurrent invocations, and a ParallelWorker
mapped over a node races activations of the same agent.
State.Mode now stays the agent's own declaration and is never written to
after construction. Placement publishes its default on the context
(runner root => chat, AgentNode => single_turn), the declared mode still
wins over it, and the mode that wins is published for the request
processors downstream. IncludeContents is no longer forced to "none" for
single_turn; the contents processor derives that from the resolved mode,
so the setting no longer leaks into the agent's later placements.
* fix(llmagent): bind the resolved mode to its agent, and migrate every reader
Removing the mode mutation left three readers behind. identity_request_
processor and agent_transfer still read State.Mode, so an agent that declares
no mode and runs at a graph node looked undeclared to them: it gained the "You
are an agent" preamble and the transfer_to_agent instruction block that the
mutation used to suppress. That is the shape examples/workflow/complex ships.
The context also carried the mode ambiently, so it governed agents it was never
resolved for. A chat coordinator at a graph node pushed single_turn onto the
peer it transferred to, and the getter preferred the context over an agent's own
declaration, so a declared chat agent could silently lose its history.
The binding now names the agent it describes: a placement resolves the mode for
one agent and binds it under that agent's name, and a reader only accepts a
binding made for the agent it is looking at. A peer or child therefore falls
back to its own declaration. Placements bind (runner root => chat, AgentNode =>
single_turn) and the wrapper falls back to chat, which is what an undeclared
mode already means to every reader.
Hiding history and shaping a single turn are separated, because they were
separate before: history follows the placement alone, so a declared single_turn
agent run outside one keeps its conversation, as it did when the wrapper set
IncludeContents="none" only on the path it drove.
Tests pin the mechanism: removing either bind, reverting either reader, or
dropping the agent-name check in BoundMode now fails the suite.
* fix(llmagent): honour an explicit IncludeContents at a single_turn node
A single_turn placement seeds the agent with one synthetic turn and hides the
conversation, which is right when the caller expressed no preference but not
when they asked for history outright. adk-python gates the same override on
include_contents being absent from model_fields_set; the Go port dropped that
gate and overrode the field either way.
The zero value already distinguishes the two cases: "" is unset, "default" is
an explicit request. The placement now hides history only for the former.
* fix(llmagent): place a nameless agent, and pin the checks that were passing either way
The mode binding is keyed by agent name, and it skipped an empty one. That
turned a missing Config.Name — documented as required, rejected by nothing —
into the difference between a one-shot graph node and a chat agent carrying
the whole transcript, reported nowhere. Bind "" like any other name so the
placement still applies, and leave the missing name to whatever validates
names.
Three checks were pinned by tests that passed with the check removed. Two mode
fixtures ended on the agent's own reply, which makes the backward scan that
isolates the current turn pivot at index 0 and return the whole history either
way; they now carry a third event, and go red when the per-agent name check or
the placement-only history gate is removed. The wrapper's re-bind, load-bearing
when findAgentToRun picks a declared single_turn agent, and the resolved mode
behind the single-turn nudge, each get a test that fails without them.
Also cover the race this replaces on the run path rather than at construction,
the explicit-IncludeContents override for a declared single_turn agent, and
say in the runner why a chat root records a binding no reader can currently
distinguish from none.
* fix(llminternal): resolve the mode in the compaction estimate, and pin two checks that passed either way
The prompt-token estimator read the raw declaration to decide whether a turn
carries the single-turn nudge, while the contents processor resolves the mode.
For an agent whose placement resolved single_turn, the estimate therefore
described a prompt the flow does not build, and that estimate is what decides
when a turn is compacted. It was the one single_turn reader the migration
missed. The comment in mode.go claimed the only readers left on the declaration
were the task-only ones, which is what made the gap easy to miss, so it now
names both kinds and says why each is exempt.
Two tests did not pin what their names claim. The chat-coordinator placement
test had a one-event history, and the backward scan that isolates the current
turn skips the agent's own events, so it pivoted at index 0 and returned
everything whether history was hidden or not; the whole workflow package stayed
green with the history gate widened. The construction-order test compared an
empty tool list against an empty tool list, so restoring the write to a shared
sub-agent in installTaskTools left it passing — a write of exactly the kind
this change exists to remove, and one absent from the mutation battery.
That write is now covered three ways: the order test asserts a non-empty tool
list, a new test asserts llmagent.New leaves a sub-agent's declaration alone,
and a second builds coordinators concurrently over one sub-agent so the
detector sees it.
* fix(llminternal): key the mode binding by agent so a nested placement cannot erase it
A placement stored one {agent, mode} pair under one context key, which gave the
binding the property it needed on the way out and not the one it needed on the
way back. A single_turn graph node that transferred to a chat peer had its slot
overwritten by the peer's own binding. The return hop is a fresh activation on
the peer's context rather than a return to the outer frame, so when the node's
agent was re-entered its binding was gone and it fell back to chat, collecting
the identity preamble, the transfer instructions and the whole conversation
while still sitting at the same one-shot node.
The merge base survived this by accident: its sticky write to State.Mode kept
the agent single_turn on re-entry. That write is what this change removes, so
the new mechanism has to cover the case deliberately.
Putting the agent name in the context KEY rather than in the value gives both
properties at once. A binding still never governs an agent it was not resolved
for, because the lookup is by exact key, and it now also survives a nested
activation that binds someone else, because that writes a different key.
Reported by karolpiotrowicz, whose reproduction is the new test. It is red on
the parent commit and green here. Also corrects the design comment's account of
installTaskTools, which branches on single_turn too and is exempt for a
different reason: it runs at construction, where there is no invocation.
* fix(workflow): stop AgentNode.Run crashing on a tool context, and pin two rules nothing held
agent.Context.WithAgentContext returns nil for a tool context and for a callback
context, logging rather than erroring. AgentNode.Run assigned that straight back
into ctx and dereferenced it a few lines down, so a node driven from inside a
tool panicked where the merge base drained events. Every symbol on that path is
exported. Nothing in the repository reaches it, because workflow.RunNode
dispatches through SubScheduler, which the tool wrapper delegates.
The obvious nil check is not the fix: keeping the old ctx silences the panic and
drops the placement, and an undeclared agent would then run as chat at a
single_turn node. The binding only has to reach the context passed DOWN, which
NewInvocationContext takes as a plain context.Context, so it no longer travels
through WithAgentContext at all. The wrapper's re-bind had the same latent nil
and takes the same shape, guarded because its chat path needs an agent.Context.
Two invariants had no test. Re-binding the same agent shadows the outer binding
— inverting that rule left the whole root-module suite green, because no
in-tree path binds two values for one name. And workflow.New now accepts an
undeclared sub-agent mid-chain, which is a public constructor's error surface
changing; every other subtest there passes an explicit mode.
Also corrects the design comment's reason for exempting isUntransferableMode
(bindings nest, so the caller's binding IS live when a callee computes transfer
targets — it is looked past deliberately), the NewTransferToAgentTool doc (the
mode gates the instructions, not the declaration), and a subtest label that
described a mode-unset agent at a node as chat.
Reported by karolpiotrowicz.
* fix(llmagent): keep WithAgentContext off the single_turn path, and cover the guard
Round-4 review follow-up. The wrapper's re-bind now sits inside the chat branch,
which is its only consumer: runChat needs the agent.Context itself, for the
sub-scheduler its task delegations dispatch through. The single_turn and task
branches take the binding from the context they pass down and never call
WithAgentContext, so a tool or callback context cannot nil them out and neither
logs an unsupported-operation line per invocation.
The guard was unpinned before this: restoring the unguarded shape left the whole
suite green. It is now covered on the single_turn branch. The chat branch stays
uncovered on purpose, and the test says why — a chat run cannot be driven from a
tool context on the merge base either, and nor can a seeded single_turn one,
because a tool context reports no session and wrappedSession wraps that nil.
Both panic identically without this change, so neither is this change's to fix.
Also drops two comments that still describe history suppression as
IncludeContents="none", which nothing writes any more, and corrects one that
named transfer_to_agent as the route by which a declared single_turn agent
keeps its history — isUntransferableMode keeps it off the target list, so the
real route is a composite agent calling subAgent.Run directly.
* fix(llminternal): treat only "default" as an explicit opt-out from the placement
IncludeContents is an unvalidated string, and the gate asked whether it was
EMPTY. So any value the package does not recognise — "None", "defualt", a value
read from a config file — read as "the caller explicitly asked for history" and
handed a one-shot node the whole transcript. The merge base could not be
misconfigured this way, because it forced "none" onto a single_turn agent
outright, so this was a disclosure regression introduced by making the override
possible at all. The gate now asks whether the value IS "default", so anything
unrecognised falls back to the placement.
Also gives the concurrent-invocation race test a reason to be trusted. It
asserted only that no error came back, so it would have stayed green if the runs
had short-circuited before reaching the contents processor, having opened no
race window at all. It now counts model calls, and builds its contexts on the
test goroutine, since the helper calls t.Fatalf and a fixture failure inside a
worker would not have been reported as a failure.
Four comments were wrong rather than merely stale. The contents processor
claimed a declared single_turn agent reached by transfer keeps its history, but
the wrapper re-binds the declaration, so it does not. A test comment claimed
isUntransferableMode keeps such an agent off the target list entirely, but the
parent is appended without that check. The complex workflow example still said
wrapping an agent defaults it to single-turn. And the exported RunLLMAgentAsNode
never stated how it picks a mode, which now matters because an out-of-module
caller cannot bind one.
Records the precondition the binding key rests on: it is the agent name alone,
so it is only as well-scoped as names are unique within one context chain, and
neither runner.New nor workflow.New covers a node agent's own descendants.
* docs(llminternal): correct four comments that state rules the code does not have
All four were found by re-reviewing the previous round's own output, which is
where this change keeps producing defects: the code is right and the prose
beside it describes a different design.
The contents processor said hiding history follows the placement alone. An
explicit IncludeContentsNone hides it with no placement at all, through the
disjunct on the next line. The `fn` comment beside it enumerated `"" or
"default"`, which stopped being the set the moment that branch started turning
on the difference between those two values.
raceFreeLLM was documented as holding no mutable state, one line above the
atomic counter added in the previous commit. The counter is what lets the test
prove it reached the model, so it earns its place, but atomics order the
goroutines that touch them and a reader deciding whether this test can still
detect a race needs to know that.
RunLLMAgentAsNode's doc named the runner and the workflow engine as the only
binders, in a function that is itself the third.
No behaviour change. A behaviour change was tried here and reverted: making the
unbound fallback single_turn when nodeInput is non-nil, to restore what an
out-of-module caller driving its own node used to get, breaks TestAgentTransfer.
runner/agent_node.go passes the user message as nodeInput on every non-resume
turn, so that fallback would flip every transfer-selected sub-agent from chat to
single_turn — the case adk-python's build_node also resolves to chat.
* fix(llmagent): reject a nil context instead of dereferencing it, and correct the second binder list
RunLLMAgentAsNode resolves the mode before it validates it, and resolving reads
ctx. So a nil context panicked on an exported entry point, where the merge base
reached its mode check without touching ctx at all. It now errors, which is
better than either.
The binder list in PrepareLLMAgentInput's doc was the second copy of a claim
corrected 119 lines above it last round, and its stated consequence was wrong in
the other direction too: a callback or tool running under a graph node holds a
context that already carries a binding, so an out-of-module caller can get a
seed for an agent that declares nothing.
* fix(llminternal): stop a placement resolved for one agent governing a same-named other
The binding is keyed by agent name, and names are unique only across
SubAgents(). A graph node's agent is not in SubAgents() and parentmap.New
recurses nothing else, so runner.New accepts a tree holding two distinct agents
with one name. When it does, the wider binding wins and the nested agent's own
declaration is discarded.
Reachable end to end with no hand-built state: a chat root "coordinator"
transfers to a workflow whose node wraps a SequentialAgent — a composite, so the
node binds nothing — whose child is a different agent, also "coordinator",
declaring single_turn. Both the runner root and the wrapper's chat branch bind
chat under that name, either one alone being enough, so the child was handed the
identity preamble and the transfer instructions. The merge base read the
declaration at those sites and withheld both, which makes this a regression this
PR introduced rather than one it inherited.
A placement only ever supplies a DEFAULT: every binder binds
ResolveMode(declared, placementDefault), which equals the declaration whenever
there is one. A binding that contradicts an explicit declaration therefore
cannot have been resolved for that agent, and is now ignored — PlacedMode for
the history gate, ModeFor for the rest. Two same-named agents that declare the
same mode, or that both declare nothing, are still indistinguishable; nothing
keyed by name can separate them.
Suppressing chat bindings instead was tried and rejected: it fixes only the
direction measured here, leaving a single_turn binding free to hide history from
a same-named chat agent.
TestModeFor's "the binding wins over the declaration" pinned the opposite
precedence over a state no binder can produce — ResolveMode(chat, single_turn)
is chat — so it asserted the collision was correct. Replaced by the two rules
that hold.
Also: TestRunLLMAgentAsNode_SingleTurnAcceptsAToolContext built a recordingLLM
and never read it, so a branch that quietly stopped running the agent passed it.
It now requires the model to have been reached.
* test(llminternal): cover the half of the same-name fix that nothing exercised
No production change. The previous commit's fix has two halves — PlacedMode's
contradiction guard for the history gate, and ModeFor's declaration-first
precedence for every other reader — and only the second was covered end to end.
Reverting the call site at contents_processor.go from PlacedMode back to
BoundMode left the entire suite green. The mutation battery missed it because it
mutates the guard inside PlacedMode, which TestPlacedMode pins, rather than the
one call that consumes it. The direction that separates them is a chat-declaring
agent reading a foreign single_turn binding: without the guard its conversation
is taken away. Every existing row is on the other side of that — declaring
nothing, binding another name, or binding nothing at all — where the two
functions agree. The new contents-processor test fails against the reverted call
site and passes here.
The end-to-end collision test asserted two absences on a string that is empty
when the request carries no system instruction, so it would have kept passing if
instruction assembly stopped running. It now requires the agent's own
instruction to be present first.
The runner's root bind is kept on the argument that a chat binding cannot be
told from an absent one. That argument was already falsified once for an agent
that DECLARES a mode, and the contradiction guard cannot fire for one that
declares nothing, so the remaining case rested on nothing measured. It is now a
test: the same collision with an undeclared nested agent must produce byte-
identical output to the same tree with the collision renamed away, comparing
with the agent's own name substituted out, since that name is the variable being
varied and legitimately appears in the identity preamble. Binding single_turn at
the root instead of the resolved mode fails it, so it can fail.
TestPlacedMode gains the cell every undeclared agent outside a placement hits.
* fix(llminternal): identify a mode binding by the agent it was resolved for
The binding was keyed by agent name and, since the previous commit, guarded by
comparing it against the reading agent's declaration. That guard is a heuristic
and it has a hole exactly where it cannot fire: an agent that declares nothing.
Reproduced end to end through the real runner. A graph node binds single_turn
for an outer agent named "worker". That agent transfers into a workflow whose
node wraps a SequentialAgent — a composite, so nothing rebinds — and the
composite's child is a different agent, also named "worker", also undeclared. As
a composite's child with no placement of its own it is a chat agent, and the
merge base gave it the identity preamble and the transfer instructions because
it read that agent's own State. Here it was reading the outer agent's placement
and losing both. The previous commit fixed the mirror image of this for an agent
that DECLARES a mode, and documented this case as unobservable. It is not.
The binding now carries the *State of the agent it was resolved for, and
BoundMode reports nothing to any other agent — including a same-named one. That
is exact where the declaration comparison was a proxy, so PlacedMode and the
declaration-first precedence both go away and ModeFor is back to preferring the
binding, which is now safe to prefer.
Identity has to be the *State and not the agent.Agent. Reveal hands every binder
and every reader the same pointer for one agent, a pointer is always comparable,
and comparing two interface values panics when the dynamic type is not — which
for a caller-supplied Agent implementation would turn a misconfiguration into a
runtime panic on the request path.
The name stays in the context key. It is what scopes a binding to one slot and
lets bindings nest, which is what carries an agent's placement across a
transfer round trip. Identity decides whose the binding is; the name decides
where it lives.
This also closes the two-nameless-agents case, which no name-keyed scheme could:
they shared the empty-string slot outright.
* fix(llminternal): give each agent its own binding slot, not a shared one
Identity was in the binding's VALUE and the name alone was the context key. That
stopped a placement governing a same-named other agent, but not the mirror case:
binding a second, same-named agent REPLACED the first in the lookup chain, and
the identity check then rejected what it found, so the agent that owned the
placement silently lost it and fell back to chat.
Sx, Sy := &State{}, &State{} // two agents, both "w"
ctx := WithBoundMode(t.Context(), "w", Sx, ModeSingleTurn)
ctx2 := WithBoundMode(ctx, "w", Sy, ModeSingleTurn)
BoundMode(ctx2, "w", Sx) // was (unset, false); want (single_turn, true)
Moving the identity into the KEY gives the two agents separate slots, so neither
can read the other's placement and neither can destroy it. That is both halves
of the property in one mechanism, and the value goes back to being the mode.
The name stays in the key alongside the identity, and still earns its place:
it is what makes re-binding the SAME agent shadow its outer binding rather than
sit beside it.
I could not ground a runtime path that re-enters the first agent inside the
second's binding, so the user-visible consequence is unproven — FindAgent
resolves a name lookup from inside the second agent to the second agent. The
invariant is stated on WithBoundMode either way, and it was false.
Also, all found by re-reading what the previous rounds left behind:
- RunLLMAgentAsNode and PrepareLLMAgentInput both documented the mode
precedence in the order it had BEFORE the previous commit inverted it.
- A test comment still argued about the contradiction guard, deleted a commit
ago, and still said the case it covers was unmeasured. The test immediately
below it measures exactly that case.
- TestContentsRequestProcessor_BoundModeIsScopedToItsAgent varied the agent
name and the identity at once, so it could not fail on the name alone. It
now shares the running agent's state and varies only the name.
- One arm of the "another agent's binding loses" table bound the same mode the
agent declared, so it passed whatever the code did. Every arm now binds a
mode the agent did not declare.
The root-bind test's comment overstated what it establishes: it fails if the
root binds a mode readers distinguish, but not if the bind is deleted, because
for an undeclared agent chat and absent are the same answer. It says so now.
* docs(llminternal): correct what the name in the binding key actually does
No behaviour change. Round eight of review found nothing above NIT in the code
and three statements about it that are wrong.
The key comment claimed the agent name is "what makes re-binding the SAME agent
shadow its outer binding rather than sit beside it". It is not. Re-binding one
agent reuses its state pointer, so the key is equal with or without the name,
and shadowing is the identity's doing. Every binder and every reader pairs an
agent's name with that same agent's state, which makes the name a function of
the identity and redundant for lookup. It is kept as a guard against a future
binder or reader that pairs the two inconsistently — the mistake behind all
three regressions in this mechanism — and because a key naming the agent reads
better in a dump than a bare pointer. The comment says that now.
The scoping test still described the previous shape, identity in the value and
checked by the reader, rather than identity in the key.
ModeFor dereferences state, and only ctx was documented as needing to be
non-nil.
* test(runner): pin that a run does not mutate the root agent's mode
A confirmation pass with three lanes that had not seen this change before found
one real gap and two statements that contradict the code.
Of the five writes to shared agent state this PR removes, four are pinned by a
test that fails if they come back. The fifth — runner.Run stamping ModeChat onto
an undeclared root's State — is pinned by nothing. Every reader treats chat and
unset alike, so no value assertion can tell the write from its absence, and no
concurrent test drives runner.Run, so the race detector cannot either.
Reintroducing that exact write ships green today. The new test asserts the agent
object is unchanged by a run, which is the property #1137 is about, and it fails
when the write is restored.
Two comments still described the binding as scoped by the agent's NAME, in the
same commit that established the name adds nothing to a lookup and the identity
is what scopes it. For the same-named peer this design exists for, the name
scopes nothing at all.
RunLLMAgentAsNode's chat branch assigned the rebound context back to ctx, which
is the closure's captured parameter, so ranging one returned iterator twice
would write state shared between the two ranges. No caller does, and the merge
base never wrote ctx here at all, so this is a new hazard rather than an
inherited one. It uses a local now.
* test(llmagent): pin transfer-target reachability against construction order
The shared-state write this PR removes did two things, and only one of them was
tested. Wrapping an undeclared agent in an AgentNode stamped it single_turn;
that changed which delegation tool its coordinator installed, which
TestLlmAgent_SubAgentModeResolution_IsConstructionOrderIndependent covers, and
it also dropped the agent off the coordinator's transfer-target list, which
nothing covered. isUntransferableMode reads the declaration, and the target list
is assembled in the request processor rather than in state.Tools, so no
assertion on tool names can see it.
On the merge base the two construction orders produce a coordinator whose entire
transfer block is present in one and absent in the other. The new test drives
both orders through the real runner and compares the system instruction the
coordinator's model actually received. It fails on the merge base and passes
here.
* test(llminternal): assert a run leaves IncludeContents alone, and localise the root binding
CONTRACT 1 is about State.Mode and State.IncludeContents, and only the Mode half
was ever asserted. The wrapper used to write state.IncludeContents = "none" onto
the shared agent whenever it ran a single_turn placement, which is why a second
placement of the same instance then hid history everywhere.
Measured rather than assumed: reintroducing that write in its guarded form —
only when the field is still empty — leaves the entire suite green. The history
tests cannot see it because they set IncludeContents explicitly, and the race
detector only catches it under concurrency. With the new assertion it is the
sole failure, and without it there are none at all.
The runner's root binding is now kept in a local, matching what the wrapper
already does. The previous shape assigned the closure's captured ctx, and unlike
the writes further down that function it was reached on the LlmAgent-root path,
which returns before them — so it was a new instance of that shape, not the
pre-existing one it had been filed under. No behaviour change either way: the
binding is idempotent and no caller ranges the iterator twice.
* test(workflow): make the tool-context test see a dropped placement, not just a panic
Karol's review, all of it agreed with and none of it disputed.
The tool-context test declared the agent chat, and chat is the same answer as no
binding at every reader — so it could see the panic it was written for and could
not see the failure that panic's obvious fix causes. It now declares nothing, so
the placement is the only thing making the agent single_turn, and it asserts the
system instruction carries neither the identity preamble nor the transfer
instructions. Green as it stands, red when the binding is routed back through
ctx.WithAgentContext behind a nil guard.
Four comments described the code inaccurately.
AgentNode.Run said nothing in the repository drives a node from inside a tool.
SingleTurnTool does. The true claim is the stronger one: no tool context reaches
that function, because RunNode takes only the SubScheduler from the caller's
context and the child context comes from the scheduler's own parent.
The key comment said an inconsistent name/state pairing is the mistake behind
the three earlier regressions. It is not — those were all errors in the key's
design. What the name buys is that such a pairing becomes a miss rather than a
wrong hit, which is the safer failure.
runner.Run claimed its branch was the only writer of the captured ctx; four more
writes follow in the sibling branch. The clause was wrong and unnecessary, since
the branches are mutually exclusive.
The nil-context test said the merge base errored at the mode check. For a
single_turn agent it panicked at ctx.UserContent() instead. Verified on the base
before rewording.
The chat bullet never said nodeInput is ignored, which matters to a caller whose
input is not also in the session.
TestRun_DoesNotMutateTheRootAgentsMode moves into package runner. Karol flagged
the placement without asking for a change, but his reason not to — that splitting
would duplicate fixtures — does not apply here: package runner already has
scriptedModel and can read State directly. It matters because with the write
restored, `go test ./runner/` alone was green, and that is the loop someone
editing runner.go runs.
* test(llmagent): pin IncludeContents from the package that owns the write
The regression check on the reviewer's round turned up the same defect one layer
out. His point was that a test pinning runner behaviour from another directory
leaves `go test ./runner/` green when the write comes back. Two of the five
removed writes live in agent/llmagent and were pinned only from workflow_test,
so the same held there.
Measured, each write restored on its own in a clean tree:
Mode write restored ./agent/llmagent/ 10 failures
IncludeContents write restored ./agent/llmagent/ green
So the mode half was already covered locally and the IncludeContents half was
not. It is now, and the new test fails when that write is put back.
Four comments were inaccurate.
"The sibling of the test above" no longer named the right test after the runner
one moved out of that file, so it names it explicitly.
The moved runner test said a version of it "in another package" would leave
`go test ./runner/` green. False for package runner_test, which compiles into
the same binary. The property is about the directory.
The agent_node.go rationale had been narrowed to tool contexts when it was
rewritten. Callback contexts return nil from WithAgentContext too and are
equally unreachable, so it covers both again.
"An error is better than either" named an error as one of the two alternatives.
One thing I tried and reverted: the tool-context test used to pass "hello" and
now passes nil, which does drop the seeded path. Restoring it is not possible —
seeding under a tool context panics, because the context reports no session,
and the merge base panics identically. That is a known merge-base defect rather
than something this test can assert, and the test now says so.
* test(llmagent): pin the third removed write from the package that owns it
The previous round said two of the five removed writes were pinned only from
another package and closed both. It is three. installTaskTools stamps chat onto
an undeclared sub-agent while building a coordinator's tool list, it lives in
agent/llmagent/llmagent.go, and its only pin was in workflow_test.
Measured on a clean tree with that write restored:
./agent/llmagent/ green
./workflow/ 2 failures
So the suite catches it and the package that owns it does not. The new test
fails when the write returns.
That closes the pattern for all five: two were already local, and the three that
were not now are.
* fix(workflow): reject a nil context at AgentNode.Run, as the wrapper does
AgentNode.Run is exported and resolves a mode, which reads ctx. A nil one
reached context.WithValue and panicked "cannot create context from nil
parent". The merge base panicked too, a few lines lower while building
params, so this turns a crash into an error rather than changing behaviour
— and it makes the two exported mode-resolving entry points agree, since
RunLLMAgentAsNode already rejected nil after review.
Also corrects two claims the code made about itself. IncludeContentsDefault's
doc said it was merely what the unset field does, which stopped being true
when an explicit "default" became the way to keep history at a single_turn
placement. And the same-named-nested-agent test now records that its power
depends on the runner's root bind, which is inert to every reader but not to
that test, so the mutation survivor is not a licence to delete it.
The transfer-round-trip test gains the vacuity guard its four siblings
already carry: two of its three checks are absences.
* test(workflow): cover the callback context, and replace a wrong count with the tests it names
Two code comments named the callback context as the hazard that shaped this
design, and nothing drove it. It reaches further than the tool context does —
Session, Memory, RunConfig and IsolationScope all return nil for it — and the
placement survives it. The test fails when the binding is removed, so it is
load-bearing rather than a smoke test.
The mode write's local coverage was justified by "fails ten tests here". It
fails three: TestCompactionE2E, TestAgentTransfer and TestToolCallbacksAgent.
Naming them makes the claim checkable without re-running a mutation.
* test(llmagent): pin the branch PrepareLLMAgentInput's change actually created
Every row of that table declared a mode, so none could tell the resolved mode
from the declaration — which is the only thing the change to that function did.
Two rows now cover it: an undeclared agent with no binding returns nil, and one
a placement bound single_turn seeds. Reverting the reader to the declaration
fails the second and nothing else in the package.
Three comments claimed more than they could.
The tool-context test said it pinned the single_turn branch's binding. It does
not: the agent declares single_turn, so deleting the bind selects the same
branch and the test still passes. The trick its workflow sibling uses does not
transfer, because an undeclared agent with no binding resolves to chat here and
a chat run over a tool context panics on the merge base too. It now says what it
pins, and names the test that pins the rest.
The nil guard on AgentNode.Run moves below nodeInputToContent. Above it, a nil
context with an unmarshalable input reported the nil-context error where the
merge base reported a marshaling error; that call never touches ctx, so below it
the base's error survives. The comment also no longer claims a nil ctx always
panicked on the base, because for that input it did not.
The callback-context test claimed to reach further than the tool context. It
does not — the two wrappers nil the same accessors, and IsolationScope returns
empty rather than nil. It pins that they stay equivalent, which is worth having
since two design comments name the callback context and nothing drove one.
agentNodeMode's resolve call is inert for both of today's callers, and now says
so and why it is written that way.
* fix(llmagent): report an unusable context on the chat branch instead of dying in it
An UNDECLARED agent driven from a tool or callback context worked on the merge
base and panicked here. The base stamped it single_turn, and that branch builds
its own InvocationContext, which a tool context survives. Resolving to chat
instead sends it to runChat, which hands the tool context to agent.Run, whose
ctx.WithContext returns nil for that wrapper, and the flow dereferences it.
Measured both ways with the same call: the merge base completes, head panicked
with a nil dereference.
The mode flip stays — it is deliberate and documented, and the alternatives were
weighed when it was made. What was wrong is that the branch noticed the context
was unusable, shrugged, and carried on into a crash several frames down. It now
says so and stops. A caller that needs this agent over a tool context must
declare single_turn or place it at a node.
That also makes the guard testable for the first time: its failure path used to
be indistinguishable from its success path, both ending in the same panic, and
it was carried in the mutation battery as an untestable survivor for six rounds.
The tool-context test's header claimed this case was considered and cleared. It
described a chat run, which does die on both sides — not this call, which did
not.
* docs(llminternal): name the one reader that deliberately does not resolve
The design block said every reader outside its three listed exceptions resolves
the mode. The history half of the contents processor does not: it asks BoundMode
directly, because it is the only reader that must tell a single_turn placement
from a single_turn declaration, and resolving flattens exactly that. Its other
half does resolve. The code was right and the block describing it was not.
* fix(llmagent): report a missing session on the seeded path, and correct eight claims
A second cell of the round-15 class, found by asking where else removing a write
changes which BRANCH a call takes. An undeclared sub-agent ADOPTED by a chat
coordinator was stamped chat by installTaskTools, and chat ignores nodeInput, so
a node driving it from a tool context never seeded. Nothing stamps it now, the
node places it single_turn, and the seeded path wraps a session the tool context
reports as nil.
Measured with the same call on both sides: the merge base completes, head
panicked. The panic underneath predates this change and stays out of scope; the
set of callers reaching it did not, so the seeded path now says what is missing.
A 192-cell sweep of entry point x context kind x declared mode x nodeInput,
run against the merge base and against head, reports zero remaining cells that
completed on the base and panic here.
Eight claims did not survive checking, none of them a defect in the code:
The chat-branch comment called itself "the one place" an undeclared agent
notices the fallback. A composite's undeclared child re-entered by a
transfer-back notices it too, over an ordinary context and with no error.
Two copies of "writing it would make the returned iterator stateful for a
caller that ranges it twice" named the benign scenario. Re-deriving a binding
sequentially yields the same key and value; the hazard is two goroutines. In
the runner that sentence was also self-refuting, since the sibling branch
assigns four wrappers back onto the same captured parameter.
The runner no-mutation test said every reader treats chat and unset alike. On
the run path they do. agentNodeMode does not, which is how the removed stamp
used to make a later graph fail chat-mode wiring.
The root-bind companion test claimed to guard against a future reader telling
chat from unset. Identity is in the key, so a foreign binding is invisible
whatever its value, and that guard cannot fire.
The estimator said it resolves the mode the way the contents processor does. It
mirrors the nudge half and not the history half, so for a hidden-history
placement it still counts turns the prompt will not carry.
The mode.go note on isUntransferableMode argued from a live caller binding. The
caller is the one agent that filter is never applied to.
The scoping unit test said its properties were unreachable elsewhere; nesting is
driven end to end by the transfer round trip. The declared-single_turn test said
nil nodeInput is the runner path; it is the resume shape of it.
The concurrency test's scope is now stated: no tools, nothing shared but the
agent and the model, and one placement shape rather than two.
* refactor(workflow): fold the two wrapper-context tests, and cut comment mass
The callback-context test was a 54-line copy of the tool-context one. Its own
header already conceded that the two wrappers behave identically for everything
AgentNode.Run touches, and the round-13 gate note recorded that both die to the
same mutant. One table, two rows, 51 fewer lines, and both rows still fail when
the binding is removed.
Four comments were arguing with a reviewer rather than informing a maintainer.
The chat branch explained what an earlier unshipped revision did and insisted
its behaviour change was "not an accident"; mode.go spent fourteen lines
defending the name half of the context key and another six narrating a
regression git already records; a test header named a sibling test by
identifier, which rots on the first rename. All trimmed to the fact a
maintainer needs.
The chat-branch error named tool and callback contexts as the cause. The actual
condition is that WithAgentContext returned nil, and agent.Context is exported,
so a third-party implementation can reach it too. The message now says what was
missing and names those two as the usual source.
* test(workflow): pin the consequence, not just the five writes
The individual writes are each pinned field by field, and the PR description
promises what removing them buys: one placement no longer decides what the agent
is for every placement after it. Nothing exercised that.
An undeclared agent is run at a graph node, then the same instance is used as a
runner root. On the merge base the node run stamped Mode=single_turn and
IncludeContents=none onto the object permanently, and the runner then rejected
it with "root agent worker must be a chat LlmAgent, but has mode single_turn".
Restoring the write reproduces that error exactly.
Measured rather than argued: after one node run the base leaves Mode
""->"single_turn" and IncludeContents ""->"none" on the shared object, and head
leaves both untouched. That is the dimension a single-call comparison cannot
see, since the merge base's writes were persistent.
* test(workflow): delete a test that no mutation can kill, and correct the claim it rested on
TestAgentNode_ASameNamedUndeclaredNestedAgentIsUnaffectedByTheRootBind was 100
lines and two full runner runs, and it is killed by zero of the battery's 27
mutations. Its declared sibling is killed by two. The reason is structural: the
only binding in that chain is chat, and no reader a binding can reach tells
chat from unset, so the colliding and renamed arms agree under a name-only key
exactly as they do under the real one. The comment claimed a discriminating
power twice — the original claim was corrected last round, and the correction
was wrong the same way.
That investigation refuted a second claim I had repeated in three places. The
declared sibling was said to discriminate only because the runner's root bind
exists. Measured by removing that bind and dropping identity from the key at
once: it still fails. The binding a name-only key hands the child comes from the
wrapper re-binding the outer coordinator under the same name. So the root bind
is inert to the tests as well as to every reader, which is simpler than what was
written and is now what the code, the battery label and the PR description say.
* docs: correct four more claims, three of them last round's corrections
The pattern this loop keeps producing, once more. Every item below is a comment
written or rewritten in the last two rounds, and each was checked against the
code rather than reasoned about.
runner.Run's root-bind comment argued that keeping the binding in a local
changes nothing, because the sibling branch assigns four wrappers back onto the
same captured parameter. That branch is unreachable from here: the LlmAgent path
returns before it. The truth is the stronger one, so the comment now says it.
mode.go claimed isUntransferableMode is never applied to the caller. It is, in
two of the three transfer directions: a caller transferring up is filtered as
one of the callee's sub-agents, and a caller transferring sideways is filtered
as a peer. The design decision is unaffected and the reason for it is now the
real one.
The concurrency test said a toolless agent never enters the tool processor. It
enters it and appends nothing, which is what makes the hazard unexercised.
And the test added earlier today attributed both merge-base writes to the run.
Measured: NewAgentNode stamps Mode at construction with no run at all, and only
IncludeContents comes from the run. That test also lacked the reached-the-model
guard every sibling in its file carries, so it now has one.
* test(workflow): drive the transfer-back case the description enumerates
The only transfer round-trip test in these packages runs a node-wrapped agent,
which the merge base stamped at construction — so it takes the same branch on
both sides and cannot distinguish them. The case that does differ, a composite's
undeclared child re-entered as a transfer target, was enumerated in the PR
description and driven nowhere.
Measured on that tree, base against head, at the re-entry:
identity preamble base no head yes
transfer block base no head yes
conversation shown base no head yes
Those three are the visible cost of the chat fallback there, and the description
listed only the two quiet ones (Event.Output and NodeInfo.MessageAsOutput, plus
OutputSchema validation). Both the test and the description now carry all five.
526 lines
16 KiB
Go
526 lines
16 KiB
Go
// Copyright 2026 Google LLC
|
|
//
|
|
// Licensed under the Apache License, Version 2.0 (the "License");
|
|
// you may not use this file except in compliance with the License.
|
|
// You may obtain a copy of the License at
|
|
//
|
|
// http://www.apache.org/licenses/LICENSE-2.0
|
|
//
|
|
// Unless required by applicable law or agreed to in writing, software
|
|
// distributed under the License is distributed on an "AS IS" BASIS,
|
|
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
|
|
// See the License for the specific language governing permissions and
|
|
// limitations under the License.
|
|
|
|
package workflow
|
|
|
|
import (
|
|
"bytes"
|
|
"encoding/json"
|
|
"errors"
|
|
"fmt"
|
|
"slices"
|
|
"strings"
|
|
|
|
"github.com/google/jsonschema-go/jsonschema"
|
|
|
|
"google.golang.org/adk/v2/internal/llminternal"
|
|
"google.golang.org/adk/v2/internal/typeutil"
|
|
"google.golang.org/adk/v2/internal/utils"
|
|
"google.golang.org/adk/v2/session"
|
|
)
|
|
|
|
// ErrDuplicateNodeName is returned when an edge set contains two
|
|
// distinct Node instances that share the same Name.
|
|
var ErrDuplicateNodeName = errors.New("duplicate node name")
|
|
|
|
// ErrNoStartNode is returned when no start node is found in the edge set.
|
|
var ErrNoStartNode = errors.New("no start node found")
|
|
|
|
// ErrNodePointsToStart is returned when a node points to the start node.
|
|
var ErrNodePointsToStart = errors.New("node points to start node")
|
|
|
|
// ErrDuplicateEdge is returned when an edge set contains two identical edges.
|
|
// Two edges with the same (From, To) are rejected regardless of Route; use
|
|
// MultiRoute to express alternatives to the same target.
|
|
var ErrDuplicateEdge = errors.New("duplicate edge")
|
|
|
|
// ErrMultipleDefaultRoutes is returned when a node has more than one default route.
|
|
var ErrMultipleDefaultRoutes = errors.New("node has more than one default route")
|
|
|
|
// ErrNodesNotReachable is returned when some nodes are not reachable from the start node.
|
|
var ErrNodesNotReachable = errors.New("nodes not reachable from start node")
|
|
|
|
// ErrUnconditionalCycle is returned when a cycle is detected that does not
|
|
// contain any conditional edges.
|
|
var ErrUnconditionalCycle = errors.New("unconditional cycle detected")
|
|
|
|
// ErrSubWorkflowNameCollision is returned when a sub-workflow has the same name as the parent workflow.
|
|
var ErrSubWorkflowNameCollision = errors.New("sub-workflow name collision")
|
|
|
|
// ErrUnsupportedFanIn is returned when a non-JoinNode has two or more
|
|
// unconditional incoming edges. Such a fan-in target would be activated
|
|
// once per predecessor, which the scheduler does not yet serialize
|
|
// safely. Use a JoinNode to converge multiple branches.
|
|
var ErrUnsupportedFanIn = errors.New("non-JoinNode fan-in is not yet supported")
|
|
|
|
// validateNodes executes a set of edges validation checks.
|
|
func validateNodes(edges []Edge) error {
|
|
if err := validateUniqueNames(edges); err != nil {
|
|
return err
|
|
}
|
|
if err := validateStartNodePresent(edges); err != nil {
|
|
return err
|
|
}
|
|
if err := validateStartNodeNoIncoming(edges); err != nil {
|
|
return err
|
|
}
|
|
if err := validateNoTaskModeGraphNodes(edges); err != nil {
|
|
return err
|
|
}
|
|
if err := validateChatModeWiring(edges); err != nil {
|
|
return err
|
|
}
|
|
return nil
|
|
}
|
|
|
|
// validateSubWorkflowNames checks that no sub-workflow has the same name as the parent workflow.
|
|
func validateSubWorkflowNames(workflowName string, edges []Edge) error {
|
|
for _, edge := range edges {
|
|
if err := checkSubWorkflowName(edge.From, workflowName); err != nil {
|
|
return err
|
|
}
|
|
if err := checkSubWorkflowName(edge.To, workflowName); err != nil {
|
|
return err
|
|
}
|
|
}
|
|
return nil
|
|
}
|
|
|
|
// checkSubWorkflowName checks if the node is a WorkflowNode and if its sub-workflow has the same name as the parent workflow.
|
|
func checkSubWorkflowName(node Node, workflowName string) error {
|
|
if wfNode, ok := node.(*WorkflowNode); ok {
|
|
if wfNode.subWorkflow.Name() == workflowName {
|
|
return fmt.Errorf("%w: %q", ErrSubWorkflowNameCollision, workflowName)
|
|
}
|
|
}
|
|
return nil
|
|
}
|
|
|
|
// validateUniqueNames checks that all nodes in the edge set have unique names.
|
|
// If duplicate node names are found, it returns an error. The equality between
|
|
// nodes is checked by comparing the nodes directly.
|
|
func validateUniqueNames(edges []Edge) error {
|
|
names := make(map[string]Node)
|
|
checkNode := func(node Node) error {
|
|
if storedNode, ok := names[node.Name()]; ok {
|
|
if storedNode != node {
|
|
return fmt.Errorf("%w: %s", ErrDuplicateNodeName, node.Name())
|
|
}
|
|
} else {
|
|
names[node.Name()] = node
|
|
}
|
|
return nil
|
|
}
|
|
for _, edge := range edges {
|
|
if err := checkNode(edge.From); err != nil {
|
|
return err
|
|
}
|
|
if err := checkNode(edge.To); err != nil {
|
|
return err
|
|
}
|
|
}
|
|
return nil
|
|
}
|
|
|
|
// validateStartNodePresent checks that there is at least one edge starting from the start node.
|
|
func validateStartNodePresent(edges []Edge) error {
|
|
for _, edge := range edges {
|
|
if edge.From == Start {
|
|
return nil
|
|
}
|
|
}
|
|
return ErrNoStartNode
|
|
}
|
|
|
|
// validateStartNodeNoIncoming checks that no node points to the start node.
|
|
func validateStartNodeNoIncoming(edges []Edge) error {
|
|
for _, edge := range edges {
|
|
if edge.To == Start {
|
|
return fmt.Errorf("%w: %s", ErrNodePointsToStart, edge.From.Name())
|
|
}
|
|
}
|
|
return nil
|
|
}
|
|
|
|
// validateWorkflow executes a set of workflow validation checks.
|
|
func validateWorkflow(workflow *graph, schema *jsonschema.Resolved) error {
|
|
if err := validateUniqueEdges(workflow); err != nil {
|
|
return err
|
|
}
|
|
if err := validateDefaultRoute(workflow); err != nil {
|
|
return err
|
|
}
|
|
if err := validateConnectivity(workflow); err != nil {
|
|
return err
|
|
}
|
|
if err := validateCycles(workflow); err != nil {
|
|
return err
|
|
}
|
|
if err := validateFanIn(workflow); err != nil {
|
|
return err
|
|
}
|
|
if err := validateStaticSchemas(workflow); err != nil {
|
|
return err
|
|
}
|
|
if err := validateStateSchemaConsistency(workflow, schema); err != nil {
|
|
return err
|
|
}
|
|
return nil
|
|
}
|
|
|
|
// validateNoTaskModeGraphNodes rejects task-mode LlmAgents that appear
|
|
// as static workflow graph nodes.
|
|
//
|
|
// Task-mode agents are multi-turn — they pause for user replies and
|
|
// expect the original node_input (the task brief) to remain visible
|
|
// across re-dispatches. The workflow scheduler currently overwrites
|
|
// node_input with the latest user message on every re-entry, so the
|
|
// task brief is lost and the agent loses context. Until the scheduler
|
|
// preserves the originating node_input on resume, task agents may only
|
|
// be used:
|
|
//
|
|
// - as chat sub-agents of an LlmAgent coordinator (FC delegation via
|
|
// workflowinternal.TaskAgentTool / dispatchTaskFC), or
|
|
// - dispatched dynamically via workflow.RunNode from a function/
|
|
// dynamic node — never as static graph nodes.
|
|
func validateNoTaskModeGraphNodes(edges []Edge) error {
|
|
allNodes := make(map[Node]bool)
|
|
for _, e := range edges {
|
|
allNodes[e.From] = true
|
|
allNodes[e.To] = true
|
|
}
|
|
|
|
for node := range allNodes {
|
|
if mode, ok := agentNodeMode(node); ok && mode == llminternal.ModeTask {
|
|
return fmt.Errorf(
|
|
"Agent %q has mode='task' and cannot be used as a workflow graph node. Use a chat coordinator with task sub-agents, or "+
|
|
"dispatch dynamically via RunNode from a function node",
|
|
node.Name(),
|
|
)
|
|
}
|
|
}
|
|
|
|
return nil
|
|
}
|
|
|
|
// validateChatModeWiring rejects a chat-mode LlmAgent that is reached from
|
|
// any node other than Start. A chat agent builds its prompt from the
|
|
// conversation history (session events) and ignores the node input handed
|
|
// down an edge, so feeding it from a predecessor silently drops that
|
|
// predecessor's output. Mirrors adk-python's _validate_chat_agent_wiring.
|
|
func validateChatModeWiring(edges []Edge) error {
|
|
for _, e := range edges {
|
|
if e.From == Start {
|
|
continue
|
|
}
|
|
if mode, ok := agentNodeMode(e.To); ok && mode == llminternal.ModeChat {
|
|
return fmt.Errorf(
|
|
"Agent %q has mode='chat' and cannot follow node %q: chat agents rely on conversation history and cannot consume a predecessor's node input. Use mode='single_turn', or wire the agent directly from Start",
|
|
e.To.Name(), e.From.Name(),
|
|
)
|
|
}
|
|
}
|
|
return nil
|
|
}
|
|
|
|
// agentNodeMode returns the mode the LlmAgent wrapped by node runs under
|
|
// as a graph node, or ok=false when node is not an AgentNode wrapping an
|
|
// LlmAgent.
|
|
//
|
|
// Resolving rather than reading the declaration makes no difference to either
|
|
// caller today: both compare against task and chat, and resolving only ever
|
|
// turns unset into single_turn. It is written this way because the answer this
|
|
// returns is what the agent runs under at that placement, which is the whole
|
|
// point of the change around it, and because a caller added later that compares
|
|
// against single_turn would otherwise be silently wrong for an undeclared agent.
|
|
func agentNodeMode(node Node) (llminternal.Mode, bool) {
|
|
agentNode, ok := node.(*AgentNode)
|
|
if !ok {
|
|
return llminternal.ModeUnset, false
|
|
}
|
|
llmA, ok := agentNode.agent.(llminternal.Agent)
|
|
if !ok || llmA == nil {
|
|
return llminternal.ModeUnset, false
|
|
}
|
|
return llminternal.ResolveMode(llminternal.Reveal(llmA).Mode, llminternal.ModeSingleTurn), true
|
|
}
|
|
|
|
// validateUniqueEdges checks that there are no duplicate edges in the workflow.
|
|
// Two edges with the same (From, To) are rejected regardless of Route; use
|
|
// MultiRoute to express alternatives to the same target.
|
|
func validateUniqueEdges(workflow *graph) error {
|
|
for node, edges := range workflow.successors {
|
|
uniqueEdges := make(map[Node]struct{})
|
|
for _, edge := range edges {
|
|
if _, ok := uniqueEdges[edge.To]; ok {
|
|
return fmt.Errorf("%w: from %q to %q", ErrDuplicateEdge, node.Name(), edge.To.Name())
|
|
}
|
|
uniqueEdges[edge.To] = struct{}{}
|
|
}
|
|
}
|
|
return nil
|
|
}
|
|
|
|
// validateDefaultRoute checks that there are no multiple default routes for one node.
|
|
func validateDefaultRoute(workflow *graph) error {
|
|
for node, edges := range workflow.successors {
|
|
hasDefault := false
|
|
for _, edge := range edges {
|
|
if edge.Route == Default && !hasDefault {
|
|
hasDefault = true
|
|
} else if edge.Route == Default && hasDefault {
|
|
return fmt.Errorf("%w: %q", ErrMultipleDefaultRoutes, node.Name())
|
|
}
|
|
}
|
|
}
|
|
return nil
|
|
}
|
|
|
|
// validateConnectivity checks that all nodes in the edge set are reachable from the start node.
|
|
func validateConnectivity(workflow *graph) error {
|
|
if len(workflow.successors) == 0 {
|
|
return nil
|
|
}
|
|
|
|
visited := make(map[Node]bool)
|
|
var traverse func(n Node)
|
|
traverse = func(n Node) {
|
|
visited[n] = true
|
|
for _, neighbor := range workflow.successors[n] {
|
|
if !visited[neighbor.To] {
|
|
traverse(neighbor.To)
|
|
}
|
|
}
|
|
}
|
|
|
|
traverse(Start)
|
|
|
|
var unreachable []string
|
|
for _, node := range workflow.allNodes() {
|
|
if !visited[node] {
|
|
unreachable = append(unreachable, node.Name())
|
|
}
|
|
}
|
|
slices.Sort(unreachable)
|
|
|
|
if len(unreachable) > 0 {
|
|
return fmt.Errorf("%w: %q", ErrNodesNotReachable, strings.Join(unreachable, ", "))
|
|
}
|
|
|
|
return nil
|
|
}
|
|
|
|
// validateCycles checks that there are no unconditional cycles in the workflow.
|
|
// It performs a depth first search for every node in the workflow and checks
|
|
// for cycles where all edges in the cycle have nil routes.
|
|
// Default routes (where Route == Default) are treated as conditional edges
|
|
// and are ignored during unconditional cycle detection.
|
|
func validateCycles(workflow *graph) error {
|
|
visited := make(map[Node]struct{})
|
|
|
|
var traverse func(n Node, inStack map[Node]struct{}) error
|
|
traverse = func(n Node, inStack map[Node]struct{}) error {
|
|
if _, ok := inStack[n]; ok {
|
|
return fmt.Errorf("%w: %q", ErrUnconditionalCycle, n.Name())
|
|
}
|
|
|
|
if _, ok := visited[n]; ok {
|
|
return nil
|
|
}
|
|
|
|
inStack[n] = struct{}{}
|
|
visited[n] = struct{}{}
|
|
|
|
for _, edge := range workflow.successors[n] {
|
|
if edge.Route == nil {
|
|
if err := traverse(edge.To, inStack); err != nil {
|
|
return err
|
|
}
|
|
}
|
|
}
|
|
|
|
delete(inStack, n)
|
|
return nil
|
|
}
|
|
|
|
for node := range workflow.successors {
|
|
if _, ok := visited[node]; !ok {
|
|
inStack := make(map[Node]struct{})
|
|
if err := traverse(node, inStack); err != nil {
|
|
return err
|
|
}
|
|
}
|
|
}
|
|
|
|
return nil
|
|
}
|
|
|
|
// validateFanIn rejects a non-JoinNode target that has two or more
|
|
// unconditional incoming edges. Such a node would be activated once per
|
|
// completed predecessor, colliding the scheduler's per-name bookkeeping
|
|
// (mixed outputs, lost cancel funcs). JoinNode handles fan-in via a
|
|
// barrier; everything else must converge through one. Only unconditional
|
|
// (Route == nil) edges are counted so conditional fan-in and loop-back
|
|
// back-edges — where the predecessors don't all fire together — are not
|
|
// rejected.
|
|
func validateFanIn(workflow *graph) error {
|
|
for node, edges := range workflow.predecessors {
|
|
if _, isJoin := node.(*JoinNode); isJoin {
|
|
continue
|
|
}
|
|
unconditional := 0
|
|
for _, edge := range edges {
|
|
if edge.Route == nil {
|
|
unconditional++
|
|
}
|
|
}
|
|
if unconditional > 1 {
|
|
return fmt.Errorf("%w: node %q has %d unconditional incoming edges; "+
|
|
"use a JoinNode to converge branches", ErrUnsupportedFanIn, node.Name(), unconditional)
|
|
}
|
|
}
|
|
return nil
|
|
}
|
|
|
|
// defaultValidateInput validates data against schema. When data is a
|
|
// string and the schema expects a non-string shape, it first tries to
|
|
// parse the string as JSON. If parsing or validation fails, it falls
|
|
// back to validating the raw string (useful for enum/literal schemas).
|
|
// If both attempts fail, it returns the error from standard schema
|
|
// validation.
|
|
func defaultValidateInput(data any, schema *jsonschema.Resolved) (any, error) {
|
|
if schema == nil {
|
|
return data, nil
|
|
}
|
|
if data == nil {
|
|
return nil, nil
|
|
}
|
|
// Bypasses ConvertToWithJSONSchema for all string inputs since ConvertToWithJSONSchema
|
|
// expects the value to marshal into map[string]any for validation, which fails for string types.
|
|
if text, ok := data.(string); ok {
|
|
if !schemaIsString(schema) {
|
|
// Step 1: try JSON parse
|
|
var parsed any
|
|
if err := json.Unmarshal([]byte(text), &parsed); err == nil {
|
|
if err := schema.Validate(parsed); err == nil {
|
|
return parsed, nil
|
|
}
|
|
}
|
|
// Step 2: raw string may match an enum/literal schema
|
|
if err := schema.Validate(text); err == nil {
|
|
return text, nil
|
|
}
|
|
// Step 3: fall through to standard validation (which will return an error)
|
|
} else {
|
|
// If schema expects string, validate the raw string directly.
|
|
if err := schema.Validate(text); err != nil {
|
|
return nil, err
|
|
}
|
|
return text, nil
|
|
}
|
|
}
|
|
return typeutil.ConvertToWithJSONSchema[any, any](data, schema)
|
|
}
|
|
|
|
// schemaIsString reports whether the resolved schema expects a JSON
|
|
// string at the top level.
|
|
func schemaIsString(s *jsonschema.Resolved) bool {
|
|
if s == nil {
|
|
return false
|
|
}
|
|
schema := s.Schema()
|
|
if schema == nil {
|
|
return false
|
|
}
|
|
if schema.Type == "string" {
|
|
return true
|
|
}
|
|
for _, t := range schema.Types {
|
|
if t == "string" {
|
|
return true
|
|
}
|
|
}
|
|
return false
|
|
}
|
|
|
|
// validateStateSchemaConsistency checks that all nodes in the graph that reference state fields
|
|
// have those fields declared in the workflow's state schema.
|
|
func validateStateSchemaConsistency(g *graph, schema *jsonschema.Resolved) error {
|
|
if schema == nil {
|
|
return nil
|
|
}
|
|
schemaFields := extractFieldNames(schema)
|
|
|
|
for _, n := range g.allNodes() {
|
|
spa, ok := n.(StateParamsAware)
|
|
if !ok {
|
|
continue
|
|
}
|
|
for _, fieldName := range spa.StateFieldNames() {
|
|
if strings.HasPrefix(fieldName, session.KeyPrefixApp) ||
|
|
strings.HasPrefix(fieldName, session.KeyPrefixUser) ||
|
|
strings.HasPrefix(fieldName, session.KeyPrefixTemp) {
|
|
continue
|
|
}
|
|
if !slices.Contains(schemaFields, fieldName) {
|
|
return fmt.Errorf("node %q references state field %q which is not declared in StateSchema (declared: %v)", n.Name(), fieldName, schemaFields)
|
|
}
|
|
}
|
|
}
|
|
return nil
|
|
}
|
|
|
|
func extractFieldNames(schema *jsonschema.Resolved) []string {
|
|
var fields []string
|
|
if schema != nil && schema.Schema() != nil && schema.Schema().Properties != nil {
|
|
for k := range schema.Schema().Properties {
|
|
fields = append(fields, k)
|
|
}
|
|
}
|
|
slices.Sort(fields)
|
|
return fields
|
|
}
|
|
|
|
func validateStaticSchemas(g *graph) error {
|
|
for _, edge := range g.allEdges() {
|
|
outResolved := edge.From.OutputSchema()
|
|
inResolved := edge.To.InputSchema()
|
|
if outResolved == nil || inResolved == nil {
|
|
continue // validate only when both ends declare schemas (Python parity)
|
|
}
|
|
eq, err := schemasEqualCanonical(outResolved.Schema(), inResolved.Schema())
|
|
if err != nil {
|
|
return fmt.Errorf("comparing schemas on edge %s->%s: %w",
|
|
edge.From.Name(), edge.To.Name(), err)
|
|
}
|
|
if !eq {
|
|
return fmt.Errorf("graph validation failed: schema mismatch on edge %s -> %s",
|
|
edge.From.Name(), edge.To.Name())
|
|
}
|
|
}
|
|
return nil
|
|
}
|
|
|
|
func schemasEqualCanonical(a, b *jsonschema.Schema) (bool, error) {
|
|
ac, err := utils.CanonicalSchemaJSON(a)
|
|
if err != nil {
|
|
return false, err
|
|
}
|
|
bc, err := utils.CanonicalSchemaJSON(b)
|
|
if err != nil {
|
|
return false, err
|
|
}
|
|
return bytes.Equal(ac, bc), nil
|
|
}
|