fix(workflow): don't replay a finished run on the next turn - #637
Conversation
AmaadMartin
left a comment
AmaadMartin
left a comment
There was a problem hiding this comment.
The diagnosis is right and the reasoning for not porting upstream's invocation-id filter is sound — I checked runner.ts:299 and the runner does mint a fresh id per turn, so that filter would discard the outputs resume depends on.
I verified the two assumptions the new scoping rests on, and both hold. The turn's user event carries the current invocation id, so currentStart anchors where you expect. enrichEvent (node_runner.ts:363) stamps nodeInfo.path unconditionally, so real interrupt events always carry it — the workflow_agent_test.ts fixture change corrects the fixture rather than papering over a gap.
One real problem in raisedInterrupt, inline. Also note CI has not run here: only cla/google has reported, so the suite result in the description is unverified.
| function raisedInterrupt(event: Event): boolean { | ||
| return ( | ||
| (event.longRunningToolIds?.length ?? 0) > 0 || | ||
| hasRequestInputFunctionCall(event) || | ||
| hasAuthRequestFunctionCall(event) | ||
| ); |
There was a problem hiding this comment.
Not a nit. This treats "used a long-running tool" as "paused for a human", and the replay bug comes back for workflows that do the former.
(event.longRunningToolIds?.length ?? 0) > 0 ||getLongRunningFunctionCalls (agents/functions.ts:57) adds the id of any tool with isLongRunning, not just the HITL ones. A node whose agent calls such a tool emits an event with nodeInfo.path (stamped by enrichEvent) and a non-empty longRunningToolIds. That invocation is recorded paused, so on the next turn its finished run is kept and every node is fast-forwarded — exactly what this PR fixes, for those workflows.
You cannot just delete the clause: adk_request_confirmation has no predicate in core/src/workflow, so this is the only thing covering confirmation pauses today.
Test the three interrupt call names instead. Your #633 adds that predicate as requiresUserInput in agents/user_input_request.ts, where it currently has no caller — this is the caller.
Worth noting the unit tests here only ever mark a pause with longRunningToolIds, never with a real interrupt call, so none of them would catch this.
There was a problem hiding this comment.
You're right, and the fix is in 0515d1c.
raisedInterrupt is now requiresUserInput(event) from #633, which landed while this sat. That also picks up adk_request_confirmation properly — as you say, it was only ever reachable here through the longRunningToolIds clause, and nothing tested it.
Your last line was the useful part. Swapping the predicate with nothing else changed broke exactly four tests, all of them the fixtures in this file, all of them for the same reason: they build a pause as a bare longRunningToolIds with no adk_request_* call, which no engine event looks like. Every end-to-end test — auth gate, dynamic resume, HITL flow, workflow_advanced — stayed green, which is the confirmation that real interrupt events do carry the call.
So the fixtures now go through a pauseEvent() helper shaped like createRequestInputEvent, and there are two new cases for the gap:
- a completed run that merely used a long-running tool is dropped (fails on the old predicate);
- a run paused on
adk_request_confirmationis kept.
unit:core green: 204 files, 2804 tests.
Worth noting the same conflation lives one level deeper: node_runner.ts:241 turns every longRunningToolIds entry into a node interrupt, so a node whose agent calls a long-running tool is treated as waiting regardless. That is beyond this diff — I'll file it.
| lastInvocationId = event.invocationId; | ||
| } | ||
| if (invocationId === currentInvocationId) { | ||
| currentStart = Math.min(currentStart, i); |
There was a problem hiding this comment.
Nit, optional. Two guards in here defend against something that cannot happen.
currentStart = Math.min(currentStart, i);
...
return boundary <= 0 ? events : events.slice(boundary);i only increases, so the first match is already the smallest and Math.min never changes the result. boundary is never negative, and events.slice(0) already returns everything, so the ternary only decides whether the caller gets the original array or a copy — an inconsistency rather than a saving.
if (currentStart === events.length) currentStart = i;
...
return events.slice(boundary);There was a problem hiding this comment.
Both taken in 0515d1c. Math.min is now a first-write guard and the ternary is a plain events.slice(boundary).
|
CI has now run, which my review said it had not. Both failures are unrelated to this change, so no action needed from you on them. Neither touches Both belong to the same family: an integration test that spawns a real process and waits on a fixed timeout. The same family is red elsewhere right now. #618, whose diff is an export-condition fix, fails on I have re-run the failed jobs here. Worth a separate look at those timeouts, in the spirit of #548, #549 and #550; they are costing every PR a re-run. My review comment above still stands on its own. |
|
We should take a step back and reconsider. I understand a direct port of Python's functionality broke tests, but we should take a closer look at why rather than skip the idea entirely and re-implement a parallel system of pausing and resuming. It only took a few minutes of prompting to get to a spot where we can just re-use invocation ID. The change is much smaller and simpler and all tests pass. This patch implements the simpler version: |
ScottMansfield
left a comment
ScottMansfield
left a comment
There was a problem hiding this comment.
See above comment with patch
|
Thanks for writing the patch out — it made this checkable. I applied it to It still replays, permanently, after a plain-text resumeTwo-node HITL workflow, four turns, replies typed as plain text — what
That is the case the With a structured
|
| site | effect |
|---|---|
runner.ts:498 artifact_${invocationId}_${i} |
inline artifacts overwrite the previous turn's |
code_executor_context.ts:143/159, via code_execution_request_processor.ts:206,330 |
the code-retry error budget carries across turns |
skill_toolset.ts:118 contextKey = invocationId |
skill cache bleeds between turns |
telemetry/tracing.ts:224,300 |
separate turns merge into one trace |
llm_agent_wrapper.ts:259 event.invocationId !== ctx.invocationId |
a stale turn's event now matches |
Not covered by the suite either.
On "a parallel system of pausing and resuming"
That objection is fair and I'd rather not own one. Two things though. The codebase already infers resumption from the event stream instead of from an id — determineAgentForResumption (runner.ts:587) resumes by finding the agent behind the last function response, gated on resumabilityConfig. And the reason python's filter can't be ported isn't incidental: there a resumed invocation keeps its id because the client hands it back, so the id is the run. Here there is no invocation-level resume, so it isn't.
A smaller shape, if you want one
I agree the scan is the least attractive part of this. Rather than infer the boundary we can mark it. WorkflowAgent already emits a final event only when the run completed (workflow_agent.ts:96, interruptIds.length === 0) — a paused run emits none. Make that emission unconditional, tag it, and the helper collapses to:
export function eventsForCurrentRun(events: Event[]): Event[] {
for (let i = events.length - 1; i >= 0; i--) {
if (events[i].runCompleted) return events.slice(i + 1);
}
return events;
}No invocation ids, no interrupt predicate, no carry-forward, exact instead of inferred. I prototyped it: identical behaviour on every probe above, and unit:core green apart from this PR's unit tests of the old helper's semantics, which would be rewritten. Persistence is fine — the DB service stores the whole event as JSON through the camel/snake transform, and Vertex round-trips via rawEvent.
It costs a field on Event and an event-stream change (a completion event even when the output is undefined). Workflow is @experimental so that's affordable, but it's a wider blast radius than what's here.
My suggestion: land this one, which is contained in core/src/workflow and touches neither event contents nor invocation ids, and I'll follow up with the marker as a separate PR that deletes most of eventsForCurrentRun. Happy to do it the other way round if you'd rather review the marker version first — say which and I'll push it.
I've pushed Amaad's raisedInterrupt fix in the meantime (0515d1c): the long-running-tool clause is gone, replaced by requiresUserInput from #633. unit:core 204 files, 2804 tests.
|
CI is red here, and it isn't this PR:
Bisected locally, same result: Cause: the sample harness fingerprints a model call over Note this is prompt-change brittleness in the harness, not a one-off: any future edit to the system instruction breaks all seven again. Recording keyed on Nothing to do here; |
AmaadMartin
left a comment
AmaadMartin
left a comment
There was a problem hiding this comment.
Approved. Both comments are addressed at head 0515d1c3, and I checked the one way the swap to requiresUserInput could have made things worse — it does not. Details inline.
CI red here is not yours. validation fails on main at 09e23757 with the same seven suites, by name: agent_in_workflow, dynamic_nodes, loop, node_output, parallel_worker, request_input_advanced, route. Same count, same set, and your merge commit pulled that main in. Nothing in this diff can produce Cannot read properties of undefined (reading 'grade') in a recorded-response sample.
Worth raising separately: main is red, and these are the workflow sample suites, so the whole workflow surface is currently unverified on every PR that branches from it. That is a bigger problem than any of the four workflow PRs in flight.
| /** | ||
| * Whether an event paused the run for a human: it carries one of the three | ||
| * `adk_request_*` calls (input, credential, or tool confirmation). | ||
| * | ||
| * Deliberately NOT `longRunningToolIds`, which marks any tool declared | ||
| * `isLongRunning` — a run that merely used one is not waiting on a person. | ||
| */ | ||
| function raisedInterrupt(event: Event): boolean { |
There was a problem hiding this comment.
Verified, including the narrowing risk this swap carries.
requiresUserInput only counts a part once it resolves an interrupt id (functionCall.id ?? args.interruptId ?? args.functionCallId), so a pause whose call carried no id would now go undetected — worse than the false positive it replaces. All three producers set one: createRequestInputEvent uses id: requestInput.interruptId, and both generateAuthEvent and the confirmation branch use generateClientFunctionCallId() (functions.ts:101, :155). So there is no under-detection.
drops a completed run that merely used a long-running tool is the right test — it pins the false positive directly rather than the symptom. And pauseEvent building an adk_request_input call whose id is also a long-running tool id fixes what was actually wrong with the old fixtures: they only ever exercised the catch-all.
|
Sharpening what I said about CI, because the cause changes what to do about it.
The first two are the flake family that has been costing every PR a re-run — The seven workflow suites are different, and they look deterministic. The only change between the green If that is right, the fix is |
|
Fix for the inherited CI failure is up as #647 — all checks green there, including the seven samples that are red on |
Re-invoking a workflow that already completed did not run it. Every node was fast-forwarded from the previous turn's cached output, so the workflow re-emitted its previous answer and the new user message was ignored: turn 1 "first" -> n1(first), n2 -> "n2:n1:first" turn 2 "second" -> (nothing ran) -> "n2:n1:first" Rehydration exists to resume a run that paused, but `reconstructNodeStates` was handed every event in the session, so a finished run looked exactly like one waiting to be resumed. `google/adk-python` scopes this by invocation id — there a resumed invocation keeps its id. The TypeScript runner mints a fresh invocation id for every turn, so a run that spans a pause covers several invocations and an id filter would discard the outputs resume needs (it breaks the auth-gate, HITL and dynamic resume paths). Runs are delimited by pausing instead: an invocation that raised an interrupt ended paused and belongs to the run in progress; one that emitted node events without raising an interrupt ran to completion and becomes the boundary. `eventsForCurrentRun` applies that at the three places that scan history: the graph rehydration, the dynamic scheduler, and the plain-text resume scan in WorkflowAgent — the last one mattered because a plain-text resume never writes a resolving function response, so a finished run's interrupt stayed "pending" forever and swallowed later messages. Engine-minted events (a RequestInput interrupt) carry no invocation id, so the scan carries the last id seen forward; otherwise such an event opens a phantom run and the boundary lands mid-run. Two fixtures were updated to match how real events look: a hand-built prior event now carries the invocation of the run in progress, and a synthetic pending interrupt now carries the node path the engine always stamps.
`raisedInterrupt` counted any event with `longRunningToolIds`, but that marks every tool declared `isLongRunning`, not a pause for a human. A run that called one and then finished was recorded as paused, so the next turn kept it and fast-forwarded every node — the replay bug this change set fixes, for exactly those workflows. Test the three interrupt call names instead, via `requiresUserInput` (#633), which also covers `adk_request_confirmation` — until now only reachable through the `longRunningToolIds` clause, and untested. The unit fixtures marked a pause with a bare `longRunningToolIds` and no `adk_request_*` call, which no engine event looks like, so none of them caught this. They now build the interrupt the engine actually emits. Adds the two cases that were missing: a completed run that merely used a long-running tool is dropped, and a run paused on a tool confirmation is kept. Also drops two guards that defend against nothing: `i` only grows, so `Math.min` never changes `currentStart`, and `boundary` is never negative, so the `slice` ternary only chose between the array and a copy of it.
0515d1c to
0d43835
Compare
Review finding: eslint, Prettier, check_license.sh and the new `tsc` step all read `samples/`, so a syntax, style, license or type error in these 26 files fails CI. Nothing ran them, which left the failure they are most exposed to uncovered: a `WorkflowAgent` validates its graph in its constructor, so a rename or a semantics change in the `@experimental` workflow API can turn a sample into a load-time error that still type-checks -- and #635, #637 and #647 all moved that API while this branch was open. Every sample is now constructed, and the 18 that call no model are also run end-to-end through a real `InMemoryRunner`. Reuses the existing sample harness in `offline` mode, which installs the record/replay model over an empty response set, so an "offline" sample that starts calling a model throws rather than reaching the network. The 8 model-backed samples are constructed only: driving them means a checked-in fixture each, and what they add over the sibling `tests/integration/workflows/` set is prompt wording, not graph shape. One table drives it, and a guard test asserts the table matches the directories on disk -- otherwise a new sample silently gets no coverage, which is the hole this closes. Checked against all three failures it is meant to catch, rather than assuming a passing suite means a working one: a duplicate node name (constructor validation) fails the sample's case, an unregistered new directory fails the guard, and an LlmAgent spliced into an offline graph fails on the missing fixture.
Review finding: eslint, Prettier, check_license.sh and the new `tsc` step all read `samples/`, so a syntax, style, license or type error in these 26 files fails CI. Nothing ran them, which left the failure they are most exposed to uncovered: a `WorkflowAgent` validates its graph in its constructor, so a rename or a semantics change in the `@experimental` workflow API can turn a sample into a load-time error that still type-checks -- and #635, #637 and #647 all moved that API while this branch was open. Every sample is now constructed, and the 18 that call no model are also run end-to-end through a real `InMemoryRunner`. Reuses the existing sample harness in `offline` mode, which installs the record/replay model over an empty response set, so an "offline" sample that starts calling a model throws rather than reaching the network. The 8 model-backed samples are constructed only: driving them means a checked-in fixture each, and what they add over the sibling `tests/integration/workflows/` set is prompt wording, not graph shape. One table drives it, and a guard test asserts the table matches the directories on disk -- otherwise a new sample silently gets no coverage, which is the hole this closes. Checked against all three failures it is meant to catch, rather than assuming a passing suite means a working one: a duplicate node name (constructor validation) fails the sample's case, an unregistered new directory fails the guard, and an LlmAgent spliced into an offline graph fails on the missing fixture.
…oc snippets (#634) * docs(workflow): add runnable ports of the graph-workflow doc snippets The Python snippets on https://adk.dev/graphs/ have no TypeScript counterpart, and they are fragments: they reference helpers they never define (`condition()`, `task_A_node`, …), so they cannot be run as written even in Python. A TS reader has nothing to copy from and no way to check that the concept behaves the way the page claims. Adds 26 runnable ports, one directory per snippet, grouped by the docs page it comes from so a directory maps 1:1 to a section anchor on adk.dev: graphs/ get_started, process_pipeline routes/ sequence, branches, function_node, fan_out_join, loop_escalation, nested_workflow data_handling/ node_output, routing_output, schemas, session_state, structured_access, structured_output, user_message dynamic/ get_started, nodes, custom_run_ids, data_handling, human_input, loop_route, parallel_route, sequence_route human_input/ get_started, initial_prompt, payload_and_schema Each fills in the undefined helpers with the smallest plausible implementation and says so in its header. Where TypeScript genuinely diverges from the Python API the file comments say why, so a reader porting from the docs is not left guessing — for example Python's `Event(message=...)` has no TS equivalent, and a graph's validating schema belongs on the node wrapping an agent rather than on the agent itself. 18 of the 26 run with no API key, which keeps the concepts (routing, loops, fan-out/join, dynamic dispatch, human-in-the-loop) explorable offline. * ci(samples): type-check samples/ in CI samples/ is not an npm workspace, so "npm run build" never compiled it, and the lint job uses tseslint's non-type-aware recommended config. That left the sample sources backing the docs pages with nothing in CI that would catch a renamed type or a removed export as the @experimental workflow API moves. Add samples/tsconfig.json (the same extends-the-root pattern core, dev and integrations use), a "ts:check:samples" script, and a validation.yaml step that runs it after the build. Scoped to samples rather than the existing repo-wide "ts:check", which currently reports 288 pre-existing errors across 44 test files. * docs(workflow): correct two wrong claims in the sample comments Both were review findings, and both were wrong about the framework rather than about the samples. The dynamic HITL sample said the `rerun_on_resume=False` handoff -- "do not re-run on resume; complete with the human's reply as my output" -- was implemented for static graph nodes only, so its leaf used a re-entry form instead: a stable `interruptId` plus a `ctx.resumeInputs[id]` lookup that returns the reply on the second pass. #635 added that handoff for dynamic `ctx.runNode` children (`dynamic_node_scheduler.ts:134`, `resumeHandoff`), so the claim went stale in the same branch that now carries the sample. The leaf is the doc's `rerun_on_resume=False` one-liner again, which is both the faithful port and four fewer concepts to explain. The node_output sample cautioned that a node may emit only ONE event carrying `output`. Nothing enforces that: `node_runner.ts:234` assigns `child.output = event.output` for every event, so the last one silently wins and the successor never sees the rest. That is worth stating precisely, because the Python page gives two accounts and neither is what happens here -- each `yield` "adds to a list of data objects on the Event" under Node output, and two yields carrying `Event.output` are "a runtime error" under the structured-data caution. Recorded as a Python-to-TypeScript difference in the README rather than only in the sample. Verified both by running them, not by reading: a node yielding two `output` events hands the successor the second and raises nothing, and the reworked HITL leaf pauses on turn 1 and resolves "yes" to "Approved" on turn 2. * docs(workflow): stop coercing inputs that are already typed as strings Review finding: the samples were split on how they treat the workflow input. Eleven files wrapped it in `String(...)`; eight called `.trim()` or `.toUpperCase()` straight on a parameter already declared `string`. `extractWorkflowInput` (`workflow_agent.ts:187`) returns the message text for a text-only turn and the raw `Content` for anything else, so neither form is sound for a non-text turn -- but they fail differently. `String()` turns a `Content` into `"[object Object]"` and carries it happily through the graph; the bare call throws where the mistake is. Keep the one that fails loudly, and say so in the README so a reader copying a sample knows what it assumes. Coercion stays where the value genuinely is untyped: `ctx.runNode(...).output` and a `ctx.resumeInputs[id]` reply are both `unknown`, and the samples that read them keep converting explicitly at the point of use. * test(workflow): execute the docs samples instead of only compiling them Review finding: eslint, Prettier, check_license.sh and the new `tsc` step all read `samples/`, so a syntax, style, license or type error in these 26 files fails CI. Nothing ran them, which left the failure they are most exposed to uncovered: a `WorkflowAgent` validates its graph in its constructor, so a rename or a semantics change in the `@experimental` workflow API can turn a sample into a load-time error that still type-checks -- and #635, #637 and #647 all moved that API while this branch was open. Every sample is now constructed, and the 18 that call no model are also run end-to-end through a real `InMemoryRunner`. Reuses the existing sample harness in `offline` mode, which installs the record/replay model over an empty response set, so an "offline" sample that starts calling a model throws rather than reaching the network. The 8 model-backed samples are constructed only: driving them means a checked-in fixture each, and what they add over the sibling `tests/integration/workflows/` set is prompt wording, not graph shape. One table drives it, and a guard test asserts the table matches the directories on disk -- otherwise a new sample silently gets no coverage, which is the hole this closes. Checked against all three failures it is meant to catch, rather than assuming a passing suite means a working one: a duplicate node name (constructor validation) fails the sample's case, an unregistered new directory fails the guard, and an LlmAgent spliced into an offline graph fails on the missing fixture. * docs(workflow): restore the state-based counter now that #636 fixed the read The third stale claim of this review, and the same shape as the other two: a sample working around a framework bug that has since been fixed on main. The session_state port carried a "do not read-modify-write ONE key from several nodes" gotcha, and routed `attempts` along the edges as node output to avoid it. #636 landed that fix — node reads are now served from a per-invocation write overlay — so the warning describes a bug that no longer exists and the workaround is no longer buying anything. `attempts` goes back to being a state key that one node initializes, another increments and a third reads, which is what the Python snippet does and what its inline comment claims it prints. Confirmed against both sides of the fix rather than assuming: reverting #636's `node_context.ts` makes the third node read 0, and with it in place the sample prints `attempts state: 1` — the snippet's own documented output. Drops the README gotcha section with it, and keeps the surviving half of the advice — prefer an edge when only the next node needs the value — as guidance in the sample rather than as a warning about a defect. * ci(samples): keep samples resolving @google/adk through node_modules The samples config inherits the root one, so once #648 adds the `@google/adk` -> `core/src` aliases there, `npm run ts:check:samples` would start checking the samples against the workspace sources instead of the published types — the one thing a sample should not do, since a user's project resolves the package through `node_modules`. `"paths": {}` pins that, the same reset `core`, `dev` and `integrations` already carry. No-op against the root config as it stands today: the check resolves to `core/dist/types/index.d.ts` and passes either way. * ci(samples): keep the repo-wide type check out of samples/ Fallout from rebasing onto #648, which landed the repo-wide `ts:check` while this branch was open. The root config names no `include` and excludes only `node_modules` and `**/dist`, so `tsc --noEmit` now picks up all 26 sample files — and resolves their `@google/adk` imports through the root `paths` aliases, against `core/src`. That is the one resolution a sample must not use, which is the whole point of the `"paths": {}` reset in `samples/tsconfig.json`: a sample is a consumer of the published package, so it has to resolve the way a user's project does, through `node_modules` and against the built types. With both checks running, the scoped one did that and the repo-wide one quietly did the opposite over the same files. Excluding `samples` from the root config leaves one owner. Verified on both sides: `tsc --noEmit --listFiles` now reports 0 files under `samples/` and still passes, while `tsc -p samples --listFiles` reports all 26 and resolves `@google/adk` to `core/dist/types/index.d.ts`. The `validation.yaml` collision #648 was warned about resolved as both steps, not one: `ts:check` for the repo, `ts:check:samples` for the samples. The zizmor hardening on that file (`permissions`, `persist-credentials`, the three SHA pins) came in with #648, so that commit dropped out of this branch as already upstream.
* fix(workflow): don't replay a finished run on the next turn Re-invoking a workflow that already completed did not run it. Every node was fast-forwarded from the previous turn's cached output, so the workflow re-emitted its previous answer and the new user message was ignored: turn 1 "first" -> n1(first), n2 -> "n2:n1:first" turn 2 "second" -> (nothing ran) -> "n2:n1:first" Rehydration exists to resume a run that paused, but `reconstructNodeStates` was handed every event in the session, so a finished run looked exactly like one waiting to be resumed. `google/adk-python` scopes this by invocation id — there a resumed invocation keeps its id. The TypeScript runner mints a fresh invocation id for every turn, so a run that spans a pause covers several invocations and an id filter would discard the outputs resume needs (it breaks the auth-gate, HITL and dynamic resume paths). Runs are delimited by pausing instead: an invocation that raised an interrupt ended paused and belongs to the run in progress; one that emitted node events without raising an interrupt ran to completion and becomes the boundary. `eventsForCurrentRun` applies that at the three places that scan history: the graph rehydration, the dynamic scheduler, and the plain-text resume scan in WorkflowAgent — the last one mattered because a plain-text resume never writes a resolving function response, so a finished run's interrupt stayed "pending" forever and swallowed later messages. Engine-minted events (a RequestInput interrupt) carry no invocation id, so the scan carries the last id seen forward; otherwise such an event opens a phantom run and the boundary lands mid-run. Two fixtures were updated to match how real events look: a hand-built prior event now carries the invocation of the run in progress, and a synthetic pending interrupt now carries the node path the engine always stamps. * fix(workflow): only a real HITL pause keeps a run open `raisedInterrupt` counted any event with `longRunningToolIds`, but that marks every tool declared `isLongRunning`, not a pause for a human. A run that called one and then finished was recorded as paused, so the next turn kept it and fast-forwarded every node — the replay bug this change set fixes, for exactly those workflows. Test the three interrupt call names instead, via `requiresUserInput` (google#633), which also covers `adk_request_confirmation` — until now only reachable through the `longRunningToolIds` clause, and untested. The unit fixtures marked a pause with a bare `longRunningToolIds` and no `adk_request_*` call, which no engine event looks like, so none of them caught this. They now build the interrupt the engine actually emits. Adds the two cases that were missing: a completed run that merely used a long-running tool is dropped, and a run paused on a tool confirmation is kept. Also drops two guards that defend against nothing: `i` only grows, so `Math.min` never changes `currentStart`, and `boundary` is never negative, so the `slice` ternary only chose between the array and a copy of it.
…oc snippets (google#634) * docs(workflow): add runnable ports of the graph-workflow doc snippets The Python snippets on https://adk.dev/graphs/ have no TypeScript counterpart, and they are fragments: they reference helpers they never define (`condition()`, `task_A_node`, …), so they cannot be run as written even in Python. A TS reader has nothing to copy from and no way to check that the concept behaves the way the page claims. Adds 26 runnable ports, one directory per snippet, grouped by the docs page it comes from so a directory maps 1:1 to a section anchor on adk.dev: graphs/ get_started, process_pipeline routes/ sequence, branches, function_node, fan_out_join, loop_escalation, nested_workflow data_handling/ node_output, routing_output, schemas, session_state, structured_access, structured_output, user_message dynamic/ get_started, nodes, custom_run_ids, data_handling, human_input, loop_route, parallel_route, sequence_route human_input/ get_started, initial_prompt, payload_and_schema Each fills in the undefined helpers with the smallest plausible implementation and says so in its header. Where TypeScript genuinely diverges from the Python API the file comments say why, so a reader porting from the docs is not left guessing — for example Python's `Event(message=...)` has no TS equivalent, and a graph's validating schema belongs on the node wrapping an agent rather than on the agent itself. 18 of the 26 run with no API key, which keeps the concepts (routing, loops, fan-out/join, dynamic dispatch, human-in-the-loop) explorable offline. * ci(samples): type-check samples/ in CI samples/ is not an npm workspace, so "npm run build" never compiled it, and the lint job uses tseslint's non-type-aware recommended config. That left the sample sources backing the docs pages with nothing in CI that would catch a renamed type or a removed export as the @experimental workflow API moves. Add samples/tsconfig.json (the same extends-the-root pattern core, dev and integrations use), a "ts:check:samples" script, and a validation.yaml step that runs it after the build. Scoped to samples rather than the existing repo-wide "ts:check", which currently reports 288 pre-existing errors across 44 test files. * docs(workflow): correct two wrong claims in the sample comments Both were review findings, and both were wrong about the framework rather than about the samples. The dynamic HITL sample said the `rerun_on_resume=False` handoff -- "do not re-run on resume; complete with the human's reply as my output" -- was implemented for static graph nodes only, so its leaf used a re-entry form instead: a stable `interruptId` plus a `ctx.resumeInputs[id]` lookup that returns the reply on the second pass. google#635 added that handoff for dynamic `ctx.runNode` children (`dynamic_node_scheduler.ts:134`, `resumeHandoff`), so the claim went stale in the same branch that now carries the sample. The leaf is the doc's `rerun_on_resume=False` one-liner again, which is both the faithful port and four fewer concepts to explain. The node_output sample cautioned that a node may emit only ONE event carrying `output`. Nothing enforces that: `node_runner.ts:234` assigns `child.output = event.output` for every event, so the last one silently wins and the successor never sees the rest. That is worth stating precisely, because the Python page gives two accounts and neither is what happens here -- each `yield` "adds to a list of data objects on the Event" under Node output, and two yields carrying `Event.output` are "a runtime error" under the structured-data caution. Recorded as a Python-to-TypeScript difference in the README rather than only in the sample. Verified both by running them, not by reading: a node yielding two `output` events hands the successor the second and raises nothing, and the reworked HITL leaf pauses on turn 1 and resolves "yes" to "Approved" on turn 2. * docs(workflow): stop coercing inputs that are already typed as strings Review finding: the samples were split on how they treat the workflow input. Eleven files wrapped it in `String(...)`; eight called `.trim()` or `.toUpperCase()` straight on a parameter already declared `string`. `extractWorkflowInput` (`workflow_agent.ts:187`) returns the message text for a text-only turn and the raw `Content` for anything else, so neither form is sound for a non-text turn -- but they fail differently. `String()` turns a `Content` into `"[object Object]"` and carries it happily through the graph; the bare call throws where the mistake is. Keep the one that fails loudly, and say so in the README so a reader copying a sample knows what it assumes. Coercion stays where the value genuinely is untyped: `ctx.runNode(...).output` and a `ctx.resumeInputs[id]` reply are both `unknown`, and the samples that read them keep converting explicitly at the point of use. * test(workflow): execute the docs samples instead of only compiling them Review finding: eslint, Prettier, check_license.sh and the new `tsc` step all read `samples/`, so a syntax, style, license or type error in these 26 files fails CI. Nothing ran them, which left the failure they are most exposed to uncovered: a `WorkflowAgent` validates its graph in its constructor, so a rename or a semantics change in the `@experimental` workflow API can turn a sample into a load-time error that still type-checks -- and google#635, google#637 and google#647 all moved that API while this branch was open. Every sample is now constructed, and the 18 that call no model are also run end-to-end through a real `InMemoryRunner`. Reuses the existing sample harness in `offline` mode, which installs the record/replay model over an empty response set, so an "offline" sample that starts calling a model throws rather than reaching the network. The 8 model-backed samples are constructed only: driving them means a checked-in fixture each, and what they add over the sibling `tests/integration/workflows/` set is prompt wording, not graph shape. One table drives it, and a guard test asserts the table matches the directories on disk -- otherwise a new sample silently gets no coverage, which is the hole this closes. Checked against all three failures it is meant to catch, rather than assuming a passing suite means a working one: a duplicate node name (constructor validation) fails the sample's case, an unregistered new directory fails the guard, and an LlmAgent spliced into an offline graph fails on the missing fixture. * docs(workflow): restore the state-based counter now that google#636 fixed the read The third stale claim of this review, and the same shape as the other two: a sample working around a framework bug that has since been fixed on main. The session_state port carried a "do not read-modify-write ONE key from several nodes" gotcha, and routed `attempts` along the edges as node output to avoid it. google#636 landed that fix — node reads are now served from a per-invocation write overlay — so the warning describes a bug that no longer exists and the workaround is no longer buying anything. `attempts` goes back to being a state key that one node initializes, another increments and a third reads, which is what the Python snippet does and what its inline comment claims it prints. Confirmed against both sides of the fix rather than assuming: reverting google#636's `node_context.ts` makes the third node read 0, and with it in place the sample prints `attempts state: 1` — the snippet's own documented output. Drops the README gotcha section with it, and keeps the surviving half of the advice — prefer an edge when only the next node needs the value — as guidance in the sample rather than as a warning about a defect. * ci(samples): keep samples resolving @google/adk through node_modules The samples config inherits the root one, so once google#648 adds the `@google/adk` -> `core/src` aliases there, `npm run ts:check:samples` would start checking the samples against the workspace sources instead of the published types — the one thing a sample should not do, since a user's project resolves the package through `node_modules`. `"paths": {}` pins that, the same reset `core`, `dev` and `integrations` already carry. No-op against the root config as it stands today: the check resolves to `core/dist/types/index.d.ts` and passes either way. * ci(samples): keep the repo-wide type check out of samples/ Fallout from rebasing onto google#648, which landed the repo-wide `ts:check` while this branch was open. The root config names no `include` and excludes only `node_modules` and `**/dist`, so `tsc --noEmit` now picks up all 26 sample files — and resolves their `@google/adk` imports through the root `paths` aliases, against `core/src`. That is the one resolution a sample must not use, which is the whole point of the `"paths": {}` reset in `samples/tsconfig.json`: a sample is a consumer of the published package, so it has to resolve the way a user's project does, through `node_modules` and against the built types. With both checks running, the scoped one did that and the repo-wide one quietly did the opposite over the same files. Excluding `samples` from the root config leaves one owner. Verified on both sides: `tsc --noEmit --listFiles` now reports 0 files under `samples/` and still passes, while `tsc -p samples --listFiles` reports all 26 and resolves `@google/adk` to `core/dist/types/index.d.ts`. The `validation.yaml` collision google#648 was warned about resolved as both steps, not one: `ts:check` for the repo, `ts:check:samples` for the samples. The zizmor hardening on that file (`permissions`, `persist-credentials`, the three SHA pins) came in with google#648, so that commit dropped out of this branch as already upstream.
4 participants
Summary
Re-invoking a workflow that already completed does not run it. Every node is fast-forwarded from the previous turn's cached output, so the workflow re-emits its previous answer and the new user message is silently ignored.
Probe on current
main, a two-node workflow over two turns:Any multi-turn conversational workflow answers the first question forever.
Root cause
Rehydration exists to resume a run that paused, but
reconstructNodeStateswas handed every event in the session (workflow.ts:192), so a finished run looked identical to one waiting to be resumed — cached outputs present,isFastForwardabletrue, every node skipped.Why not upstream's fix
google/adk-pythonscopes this by invocation id:That works there because a resumed invocation keeps its id. The TypeScript runner mints a fresh invocation id on every turn (
Runner.runAsync,runner.ts:299) — there is no invocation-level resumption. A run that spans a pause therefore covers several invocations, and an id filter discards exactly the outputs resume needs.Measured, not assumed — porting the filter verbatim fixes the replay bug but breaks four existing tests:
Fix
Delimit runs by pausing rather than by invocation id:
A pause is an event carrying one of the three
adk_request_*calls, tested viarequiresUserInput(#633). Deliberately notlongRunningToolIds, which marks any tool declaredisLongRunning: a run that merely used one is not waiting on a person, and counting it as a pause brings the replay bug straight back for those workflows.eventsForCurrentRunapplies that at the three places that scan history:workflow.tsgraph rehydrationdynamic_node_scheduler.tsctx.runNodechildrenworkflow_agent.tsplain-text resumefunctionResponse, so a finished run's interrupt stayed "pending" forever and swallowed later messagesOne subtlety
Engine-minted events carry no invocation id —
createRequestInputEventproduces an interrupt event thatenrichEventstamps with author, path and branch but no id. Left alone it opens a phantom run and the boundary lands mid-run, re-executing completed nodes. The scan carries the last id seen forward. There is a test for it; arguably the underlying gap is thatenrichEventshould stampinvocationIdtoo — happy to do that separately, since it changes event contents.Tests
New
core/test/workflow/rehydration_scope_test.ts:eventsForCurrentRun: completed run dropped; paused run kept; multi-pause run kept across all its invocations; a completed run before a paused one cut correctly; id-less engine event attributed to its surrounding invocation; empty/no-op inputadk_request_confirmationis kept — the two ends of the pause predicateRunnerPauses in the fixtures are built the way the engine builds them (
createRequestInputEvent: anadk_request_*call whose id is also a long-running tool id), not as a barelongRunningToolIds, which no real event looks like.Two other fixtures updated to match how real events look: a hand-built prior event now carries the invocation of the run in progress, and a synthetic pending interrupt now carries the
nodeInfo.paththe engine always stamps.unit:coregreen: 204 files, 2804 tests.Stacking
#635 has landed and is merged in here. Independent of #636 (disjoint files).