fix(workflow): say which node failed, and when a route goes nowhere - #650
Conversation
|
The red #652 pins those three actions and its The test matrix on this PR is passing on all three OSes. |
AmaadMartin
left a comment
AmaadMartin
left a comment
There was a problem hiding this comment.
The core is sound: cause is preserved (errors.ts:155), getNextPendingNodes control flow is unchanged, and no code reads ZodError.issues, so the wrap breaks no caller. The message construction is not repeated across the six files, so a shared helper is not warranted. One point needs a decision: the warn level fires on the loop-exit idiom that tests/integration/workflows/loop/agent.ts uses. zizmor-output is red on other open PRs too, so it is not from this change.
| logger.warn( | ||
| `Node '${nodeName}' emitted route ${shown}, which matches no ` + | ||
| `outgoing edge, so this branch stops here. Edges from this node ` + | ||
| `are keyed to: ${known}.`, | ||
| ); |
There was a problem hiding this comment.
Not a nit. Make this logger.debug.
The loop-exit idiom emits an unmatched route on each successful exit. tests/integration/workflows/loop/agent.ts:77 wires only {unrelated: generateHeadline}, so a correct run warns. warn prints at the default level (core/src/utils/logger.ts sets INFO), and this is the only log call in core/src/workflow/. Your own repro ran at --log_level debug and saw nothing, so debug answers the reported problem without noise on a working graph.
| const isConditionalContinue = | ||
| emitted.length > 0 && emitted.every((r) => r === false); |
There was a problem hiding this comment.
Nit. The false exemption is too broad, and it skips the string normalization this function uses everywhere else.
const isConditionalContinue =
emitted.length > 0 && emitted.every((r) => r === false);It also silences a node whose only edge is keyed to 'x', where false is a real mistake. Line 189 matches with String(r), so an emitted 'false' warns but false does not.
const isConditionalContinue =
availableRoutes.includes('true') &&
emitted.length > 0 &&
emitted.every((r) => String(r) === 'false');The boolean test at graph_test.ts:122 still passes: its edge is keyed to true.
| expect(err.message).toContain("Node 'squared'"); | ||
| expect(err.message).toContain('inputSchema'); | ||
| // The original validation error is preserved for field-level detail. | ||
| expect(err.cause).toBeDefined(); |
There was a problem hiding this comment.
Nit. toBeDefined() does not test what the wrapper promises.
expect(err.cause).toBeDefined();It passes for any non-undefined value. The output test at line 59 checks no cause at all, and neither test checks that the message keeps the field detail. Assert both, in both tests:
expect(err.cause).toBeInstanceOf(Error);
expect(err.message).toContain((err.cause as Error).message);
|
can we instead distinguish between an empty list and one that has a node that does not exist? |
3db2a5a to
2a5f85d
Compare
|
Rebased onto latest main (8611219) and applied all three points.
One thing you should know, because it undercuts the stated rationale: There is a second, deeper gap that #658 does not fix:
Core suite: 2853 passing, |
|
Good question — I want to fix the right thing here, and I think you're asking for a design change rather than a bug fix, because "a node that does not exist" isn't reachable today. Two versions of it, and they differ a lot in scope. Where the split already is. The guard in
Where it is not. Inside that second bucket, a deliberate loop exit and a typo'd route key are identical states. On "a node that does not exist". A routing-map target can't currently be missing from the graph. Two ways to introduce it:
I lean towards (1) — it is the only one that lets a correct graph stay quiet while a typo gets a loud error. Happy to do it here or as a follow-up, but I would rather confirm before rewiring the samples. Which did you have in mind? |
AmaadMartin
left a comment
AmaadMartin
left a comment
There was a problem hiding this comment.
Holding: the PR does not merge cleanly (mergeable_state: dirty). Commits dd1c08b and e0b7281 duplicate PRs #651 and #658, both already merged to main. My three earlier findings are resolved: the route log is now logger.debug, the false exemption requires a true edge, and both schema tests assert the cause. The isolation scope feature (commit b596ea3) is absent from the description but is wired and CI is green on all three runners. Rebase onto main to clear the conflict.
| * These paths come from the command line, so an absolute one has to be | ||
| * honoured as given. | ||
| */ | ||
| export function getAbsolutePath(p: string): string { |
There was a problem hiding this comment.
Not a nit. This duplicates merged PR #651.
getAbsolutePath already exists on main at dev/src/utils/file_utils.ts:19; PR #651 merged it on 2026-08-12. Commit dd1c08b re-adds it here, and commit e0b7281 re-adds the merged #658 ?? LogLevel.INFO fix at dev/src/cli/cli.ts:49. Both now conflict with main.
Rebase onto main. The rebase drops both CLI commits, and the diff returns to the workflow change plus the isolation scope.
Two failures in a graph workflow gave the user nothing to act on. A node whose input or output misses its schema threw a bare ZodError. It names the offending field but not the node, and the stack is bundler internals, so in a graph of any size there is no way to tell which node produced the bad value. Wrap it in NodeSchemaValidationError, which names the node and the side that failed and keeps the original error as `cause`. A node that emits a route no outgoing edge is keyed to schedules nothing, so the invocation can end having produced no events at all — with no diagnostic. Behaviour is deliberately unchanged (it matches adk-python's _get_next_pending_nodes, and routing back on one value while letting the other fall through is how the loop samples exit), but it now logs the node, the emitted route and the routes that do have edges. That log is at debug rather than warn: the loop-exit idiom is correct code, and at warn every successful run of it would complain. A bare `false` against a `true` edge is skipped entirely as the conditional-continue idiom, compared as a string like every other route match so `false` and 'false' behave alike. Found while bug-bashing the graph-workflow docs samples.
b596ea3 to
5cbef44
Compare
AmaadMartin
left a comment
AmaadMartin
left a comment
There was a problem hiding this comment.
Re-review at 5cbef44. All four prior findings are fixed: warn is now logger.debug, the false exemption keys on a true edge, both wrapper tests assert cause, and the dev/ CLI duplication is gone after the rebase. CI is green on ubuntu, macOS, and Windows. One new blocker below holds approval.
| // --- Errors --- | ||
| export {NodeTimeoutError} from './errors.js'; | ||
| export { | ||
| NodeSchemaValidationError, |
There was a problem hiding this comment.
Not a nit. Export the new error and its guard from core/src/common.ts.
common.ts:361 re-exports the workflow surface and says to keep it in sync with workflow/index.js. This PR adds NodeSchemaValidationError, isNodeSchemaValidationError, and isNodeTimeoutError here, but not there. The package has one entry point (core/package.json exports has only .), so a user of @google/adk cannot import them. The catchable, named error is the point of this change.
Add the three names to the common.ts export block.
There was a problem hiding this comment.
Done in 6d3387a — all three names added to the common.ts block, keeping it sorted with the rest.
You were right that this was the gap that mattered: index.ts:41 re-exports ./common.js and that is the only route to the package's single . entry, so the error was readable in a log line but not catchable by name, which is the entire point of naming it. Verified against the built entry rather than the source:
$ node -e "import('./core/dist/esm/index.js')..."
message: Node 'n' output does not match its outputSchema: boom
guard: true | timeout guard: true
I took isNodeTimeoutError along with the two you named, as you listed. Worth flagging why it was missing in the first place: NodeTimeoutError was already exported here without its guard, so catching a node timeout from outside the package meant an instanceof check — precisely the cross-package case the name-based guards were written to survive. That one predates this PR; it just surfaced because the two arrive together.
index_web.ts:7 also re-exports common.js, so the web build picks them up with no second change.
The other four threads are already addressed on the current branch and show as outdated:
logger.debug(graph.ts:236) —debug, notwarn. Your measurement was right, and there is a wrinkle in the rationale I noted above:--log_level debugwas itself inert, so my repro saw nothing because the flag was broken, not because the message was absent. fix(cli): make --log_level debug actually set the debug level #658 fixes the flag.falseexemption (graph.ts:217) — your version verbatim:availableRoutes.includes('true')plusString(r) === 'false', so it matches by string like every other comparison in the function.toBeDefined()—toBeInstanceOf(Error)andexpect(err.message).toContain((err.cause as Error).message), in both the input and the output test.- Rebase — done; the branch is on
2e58422and the diff is back to the six workflow files. Both duplicated CLI commits are gone.
Core suite green: 210 files, 2919 tests. tsc --noEmit clean, prettier and eslint clean on the changed file.
ScottMansfield's question about distinguishing an empty list from a missing node is still open on my side — I put two options here and would rather settle that before rewiring the loop samples.
`NodeSchemaValidationError` was added to `workflow/index.ts` but not to `common.ts`, which is the only path to the package's single entry point (`core/package.json` `exports` has just `.`, and `index.ts` re-exports `./common.js`). A user of `@google/adk` therefore could not import it, so the error could be read in a log message but not caught by name -- which is the whole point of naming it. `isNodeTimeoutError` goes with them: `NodeTimeoutError` was already exported here without its guard, so catching a node timeout meant an `instanceof` check that breaks across package boundaries, the case the name-based guards exist for. Keeps the block in sync with `workflow/index.js`, as the comment above it asks.
AmaadMartin
left a comment
AmaadMartin
left a comment
There was a problem hiding this comment.
Approve. I verified all five earlier findings against the source at 6d3387a:
graph.ts:236now logs atdebug.graph.ts:217gates onavailableRoutes.includes('true')andString(r) === 'false'.schema_validation_test.ts:56,75asserttoBeInstanceOf(Error)and the cause message, in both tests.common.ts:371,385,387export the three names;index.ts:82matches.- The branch is rebased; the duplicate CLI commits are gone.
The new error and guard follow the name-based guard pattern in errors.ts. No any, subclass instanceof, or suppression was added. All three run-tests jobs pass on ubuntu, macOS, and windows. ScottMansfield's question about an empty route list versus a missing node stays open, but it does not block this diff.
AmaadMartin
left a comment
AmaadMartin
left a comment
There was a problem hiding this comment.
All three findings are closed in the source at 6d3387a: the call is logger.debug (graph.ts:236), the exemption now requires a true edge and compares with String(r) (graph.ts:217-220), and both schema tests assert the cause and its message survive. The rebase changed parseWithSchema so a genai Schema is validated too; the wrapper still names the node and keeps cause, so it covers that path as well. The common.ts additions match the sort order and the sync note at common.ts:358. zizmor-output now skips because main pinned its actions, so my earlier note about it no longer applies.
| } | ||
| return parseWithSchema(this.inputSchema, input); | ||
| try { | ||
| return parseWithSchema(this.inputSchema, input); |
There was a problem hiding this comment.
Nit, optional. The comment above this line is wrong, and it was already wrong before your change.
* enforced for Zod schemas; a genai `Schema` is left unvalidated (see
* `parseWithSchema`).
parseWithSchema validates a genai Schema at utils/schema.ts:106. Your new error fires for that path too, which this text denies. The same two lines sit above validateOutput.
…oogle#650) * fix(workflow): say which node failed, and when a route goes nowhere Two failures in a graph workflow gave the user nothing to act on. A node whose input or output misses its schema threw a bare ZodError. It names the offending field but not the node, and the stack is bundler internals, so in a graph of any size there is no way to tell which node produced the bad value. Wrap it in NodeSchemaValidationError, which names the node and the side that failed and keeps the original error as `cause`. A node that emits a route no outgoing edge is keyed to schedules nothing, so the invocation can end having produced no events at all — with no diagnostic. Behaviour is deliberately unchanged (it matches adk-python's _get_next_pending_nodes, and routing back on one value while letting the other fall through is how the loop samples exit), but it now logs the node, the emitted route and the routes that do have edges. That log is at debug rather than warn: the loop-exit idiom is correct code, and at warn every successful run of it would complain. A bare `false` against a `true` edge is skipped entirely as the conditional-continue idiom, compared as a string like every other route match so `false` and 'false' behave alike. Found while bug-bashing the graph-workflow docs samples. * fix(workflow): export the schema validation error from the package entry `NodeSchemaValidationError` was added to `workflow/index.ts` but not to `common.ts`, which is the only path to the package's single entry point (`core/package.json` `exports` has just `.`, and `index.ts` re-exports `./common.js`). A user of `@google/adk` therefore could not import it, so the error could be read in a log message but not caught by name -- which is the whole point of naming it. `isNodeTimeoutError` goes with them: `NodeTimeoutError` was already exported here without its guard, so catching a node timeout meant an `instanceof` check that breaks across package boundaries, the case the name-based guards exist for. Keeps the block in sync with `workflow/index.js`, as the comment above it asks.
4 participants
Link to Issue or Description of Change
2. Or, if no issue exists, describe the change:
Problem:
Two failure modes in a graph workflow leave the user with nothing to act on.
1. A schema failure does not say which node failed. A node whose input or
output misses its schema throws a bare
ZodError. It names the offendingfield, but not the node, and the stack is bundler internals:
In a graph of any size there is no way to tell which node produced the bad
value. The node name is available as
this.nameright at the throw site.2. A route that matches no edge dead-ends in total silence. A node emitting
a route no outgoing edge is keyed to, with no
DEFAULT_ROUTE, schedulesnothing — so the invocation can end having produced no events at all. No
output, no error, and nothing at
--log_level debugeither. A typo in a routekey is indistinguishable from a workflow that legitimately finished.
Solution:
Wrap validation failures in a
NodeSchemaValidationErrorthat names the nodeand the side that failed, keeping the original error as
cause:For the unmatched route, log a warning naming the node, the route it emitted,
and the routes that do have edges:
Behaviour is deliberately unchanged here — this is a diagnostic only. I
first implemented it as a thrown error, which broke exactly one thing:
tests/integration/workflows/loop/agent.tsroutes back to the generator on'unrelated'and wires nothing for'tech-related', so the unmatched route isthe loop's exit condition.
adk-pythonconfirms the silent stop is intendedcross-language behaviour rather than an oversight:
So control flow stays byte-for-byte what it was; only the missing diagnostic is
added. A bare boolean
falseis exempt — an edge keyed totruewith nothingfor
falseis the conditional-continue idiom, where stopping is the intent.Known trade-off: a graph relying on the loop-exit idiom now emits this
warning on each successful exit. A deliberate exit and a typo'd route key are
indistinguishable at that point, so this is the cost of having any diagnostic at
all. Wiring the terminating value to an explicit node or to
DEFAULT_ROUTEsilences it. Happy to drop the warning to
debugif reviewers would rather havesilence by default.
Testing Plan
Unit Tests:
Seven new tests: two in
schema_validation_test.tsasserting the node name,the failing side and the preserved
cause; five ingraph_test.tscovering thewarning, the array case, and the three cases that must stay quiet
(
DEFAULT_ROUTEpresent, no conditional edges, no route emitted). The existingboolean test now also asserts no warning fires.
Those 24 integration failures are pre-existing on
mainat5742875—identical count and set with these changes stashed (a2a, build_setup,
app_loader, webui, skills). Zero new failures.
Manual End-to-End (E2E) Tests:
Against the repros that turned these up, using the workflow samples:
And a loop workflow whose routes are all mapped still runs clean, with no
spurious warning:
Checklist
Additional context
Both findings came out of a bug bash against the graph-workflow pages on
adk.dev, working through the samples in #634. They are runtime fixes and
independent of that PR, hence a separate branch off
main