docs(workflow): workflow samples (Part 8) - #595
Conversation
AmaadMartin
left a comment
There was a problem hiding this comment.
Read the full diff and checked every symbol the 22 samples import against the barrel at c6a678e: all of them (node, NodeContext, Workflow, WorkflowAgent, JoinNode, RequestInput, DEFAULT_ROUTE, createEvent, FunctionTool, LlmAgent, AuthConfig/AuthScheme/AuthCredential/AuthCredentialTypes) are exported from @google/adk, and there is not a single deep import into core/src/... — the public-surface claim in the PR body holds. I also spot-checked the option shapes against source and they match: BuildNodeOptions (name/description/inputSchema/authConfig/rerunOnResume/parallelWorker/retryConfig), RunNodeOptions (useSubBranch/runId/useAsOutput), RequestInputParams, RetryConfig.maxAttempts/initialDelay, LlmAgentSchema (zod object or genai Schema), ToolUnion = BaseTool | BaseToolset | BaseNode (so node_as_tool passing a Workflow and a node in tools is valid), and the temp:<credentialKey> state key the two auth samples read — that is exactly what AuthHandler writes (core/src/auth/auth_handler.ts:23). No hardcoded secrets. Comments below are mostly doc/consistency; the unbounded-loop one is the only substantive one.
| | `parallel_worker` | Map a node across a list with bounded concurrency | ✅ | | ||
| | `dynamic_nodes` | Imperative `dynamicEntry` driving `ctx.runNode()` | ✅ | |
There was a problem hiding this comment.
Not a nit. Two rows in this table advertise API surface that no sample in the set actually exercises.
| `parallel_worker` | Map a node across a list with bounded concurrency | ✅ |
| `dynamic_nodes` | Imperative `dynamicEntry` driving `ctx.runNode()` | ✅ |dynamicEntry appears exactly once in the whole diff — in this line. dynamic_nodes/agent.ts uses edges: [['START', orchestrate]] with an ordinary function node, not the WorkflowConfig.dynamicEntry field (core/src/workflow/workflow.ts:48, which is mutually exclusive with edges). Likewise maxParallelWorkers appears zero times in the diff, so parallel_worker/agent.ts leaves it undefined — unbounded — and does not demonstrate the bounding this row claims.
Either wire the samples to those fields, or reword the rows (e.g. "imperative orchestrator node driving ctx.runNode()" and "map a node across a list"). Since the PR positions this directory as covering the full workflow surface, dynamicEntry having no sample at all is worth calling out in the Feature coverage section too.
c6a678e to
98a0b29
Compare
AmaadMartin
left a comment
There was a problem hiding this comment.
Status, not a review: this one is blocked before the code can be re-read. Head is still 98a0b290 (unchanged since my last pass), the branch is now conflicted against feat/workflows_part7, and CI fails in 42s at npm run build rather than in the tests.
The five build errors are all API drift from the parts that moved during the review round, not sample bugs:
registerNodeBuilder({...})is missing the now-requiredid—nodes/function_node.ts:208,nodes/llm_agent_wrapper.ts:317,nodes/tool_node.ts:114(thatidis the registry dedup that came out of the Part 2 review).Graph.fromEdgeItemsno longer exists —workflow.ts:109, and:110falls out of the same line.
Rebase onto the current part7 and I'll recheck the five findings from last round against a tree that compiles.
- dynamic_nodes now uses a real `WorkflowConfig.dynamicEntry` (driving children
via `ctx.runNode()`) instead of a static `edges` graph, so it actually
demonstrates what the README row and Feature-coverage section claim — and the
imperative loop is bounded by `MAX_ATTEMPTS` instead of `for (;;)`, so an
off-topic input can't spin forever on live model calls.
- parallel_worker sets `maxParallelWorkers: 2`, demonstrating the bounded
concurrency the README advertises.
- Normalize all nine sample headers that still used the raw
`node dev/dist/esm/cli_entrypoint.js run ...` form to `npm run sample -- ...`,
matching the README and the other samples.
- Unify how the four HITL samples parse a human reply: normalize with
`.trim().toLowerCase()` (so "Approve"/"approve " no longer fall through) and
share one affirmative vocabulary, instead of three different idioms.
- README: note that `loop`'s graph cycle is intentionally uncapped and can
iterate many times; add a dynamicEntry bullet to Feature coverage.
- Fix a prompt typo ("relates the the" -> "relates to the") in parallel_worker.
98a0b29 to
8c574cc
Compare
- dynamic_nodes now uses a real `WorkflowConfig.dynamicEntry` (driving children
via `ctx.runNode()`) instead of a static `edges` graph, so it actually
demonstrates what the README row and Feature-coverage section claim — and the
imperative loop is bounded by `MAX_ATTEMPTS` instead of `for (;;)`, so an
off-topic input can't spin forever on live model calls.
- parallel_worker sets `maxParallelWorkers: 2`, demonstrating the bounded
concurrency the README advertises.
- Normalize all nine sample headers that still used the raw
`node dev/dist/esm/cli_entrypoint.js run ...` form to `npm run sample -- ...`,
matching the README and the other samples.
- Unify how the four HITL samples parse a human reply: normalize with
`.trim().toLowerCase()` (so "Approve"/"approve " no longer fall through) and
share one affirmative vocabulary, instead of three different idioms.
- README: note that `loop`'s graph cycle is intentionally uncapped and can
iterate many times; add a dynamicEntry bullet to Feature coverage.
- Fix a prompt typo ("relates the the" -> "relates to the") in parallel_worker.
8c574cc to
1dc9d71
Compare
AmaadMartin
left a comment
There was a problem hiding this comment.
Re-reviewed at 1dc9d719. All five findings are addressed, and the two substantive ones were handled with judgement rather than mechanically.
The README no longer advertises API that no sample exercises. dynamicEntry and maxParallelWorkers now appear in actual sample code rather than only in a feature table — that gap was the one thing in a samples PR that actively misleads, since a table row is a promise a reader will go looking for.
The unbounded model-call loop is bounded where it should be and deliberately not where it shouldn't. dynamic_nodes now caps at MAX_ATTEMPTS = 5 with a stated reason ("an off-topic input can't spin forever"), while loop is left uncapped on purpose and says so, because an uncapped cycle is the thing that sample exists to demonstrate. Distinguishing those two cases is exactly right — capping both would have made the second sample teach the wrong lesson, and capping neither leaves a cost hazard in a sample people copy.
The run-command idiom is unified: all 26 sample headers use npm run sample -- …. The two remaining raw node dev/dist/esm/cli_entrypoint.js references are the package.json script definition and the one README line explaining what the npm alias maps to — both correct as-is, so nothing to change there. Typo fixed.
One leftover, non-blocking: the HITL reply parsing is now mostly unified behind normalizeDecision (with a comment pointing the other samples at it), but one sample still does its own inline reply === 'approve'. Worth folding into the shared helper next time you touch that file, so the four samples don't drift apart again — but not worth another round on a docs PR.
CI: the macOS failure is app_loader_test.ts timing out at 40s, which is a known intermittent on this repo and nothing a samples PR could cause; Windows was fail-fast cancelled by it. I'm re-running it, so no action needed from you.
That closes out the stack — Parts 1-8 all reviewed. LGTM.
- dynamic_nodes now uses a real `WorkflowConfig.dynamicEntry` (driving children
via `ctx.runNode()`) instead of a static `edges` graph, so it actually
demonstrates what the README row and Feature-coverage section claim — and the
imperative loop is bounded by `MAX_ATTEMPTS` instead of `for (;;)`, so an
off-topic input can't spin forever on live model calls.
- parallel_worker sets `maxParallelWorkers: 2`, demonstrating the bounded
concurrency the README advertises.
- Normalize all nine sample headers that still used the raw
`node dev/dist/esm/cli_entrypoint.js run ...` form to `npm run sample -- ...`,
matching the README and the other samples.
- Unify how the four HITL samples parse a human reply: normalize with
`.trim().toLowerCase()` (so "Approve"/"approve " no longer fall through) and
share one affirmative vocabulary, instead of three different idioms.
- README: note that `loop`'s graph cycle is intentionally uncapped and can
iterate many times; add a dynamicEntry bullet to Feature coverage.
- Fix a prompt typo ("relates the the" -> "relates to the") in parallel_worker.
1dc9d71 to
e388b0c
Compare
- dynamic_nodes now uses a real `WorkflowConfig.dynamicEntry` (driving children
via `ctx.runNode()`) instead of a static `edges` graph, so it actually
demonstrates what the README row and Feature-coverage section claim — and the
imperative loop is bounded by `MAX_ATTEMPTS` instead of `for (;;)`, so an
off-topic input can't spin forever on live model calls.
- parallel_worker sets `maxParallelWorkers: 2`, demonstrating the bounded
concurrency the README advertises.
- Normalize all nine sample headers that still used the raw
`node dev/dist/esm/cli_entrypoint.js run ...` form to `npm run sample -- ...`,
matching the README and the other samples.
- Unify how the four HITL samples parse a human reply: normalize with
`.trim().toLowerCase()` (so "Approve"/"approve " no longer fall through) and
share one affirmative vocabulary, instead of three different idioms.
- README: note that `loop`'s graph cycle is intentionally uncapped and can
iterate many times; add a dynamicEntry bullet to Feature coverage.
- Fix a prompt typo ("relates the the" -> "relates to the") in parallel_worker.
5773943 to
6ddd0f6
Compare
- dynamic_nodes now uses a real `WorkflowConfig.dynamicEntry` (driving children
via `ctx.runNode()`) instead of a static `edges` graph, so it actually
demonstrates what the README row and Feature-coverage section claim — and the
imperative loop is bounded by `MAX_ATTEMPTS` instead of `for (;;)`, so an
off-topic input can't spin forever on live model calls.
- parallel_worker sets `maxParallelWorkers: 2`, demonstrating the bounded
concurrency the README advertises.
- Normalize all nine sample headers that still used the raw
`node dev/dist/esm/cli_entrypoint.js run ...` form to `npm run sample -- ...`,
matching the README and the other samples.
- Unify how the four HITL samples parse a human reply: normalize with
`.trim().toLowerCase()` (so "Approve"/"approve " no longer fall through) and
share one affirmative vocabulary, instead of three different idioms.
- README: note that `loop`'s graph cycle is intentionally uncapped and can
iterate many times; add a dynamicEntry bullet to Feature coverage.
- Fix a prompt typo ("relates the the" -> "relates to the") in parallel_worker.
109a3e8 to
5773943
Compare
Part 8/9 (final) of the feature/workflows split. Runnable examples covering the workflow API surface: - basics: sequence, loop, loop_self, route, multi_triggers, state, node_output, use_as_output, message - parallelism & dynamic: fan_out_fan_in, parallel_worker, dynamic_fan_out_fan_in, dynamic_nodes, nested_workflow - HITL & auth: request_input, request_input_advanced, request_input_rerun, auth_api_key, auth_oauth - agents & tools: agent_in_workflow, node_as_tool, retry - samples/workflows/README.md and a root `sample` script to run them Samples import only the public `@google/adk` surface and typecheck cleanly against source.
- dynamic_nodes now uses a real `WorkflowConfig.dynamicEntry` (driving children
via `ctx.runNode()`) instead of a static `edges` graph, so it actually
demonstrates what the README row and Feature-coverage section claim — and the
imperative loop is bounded by `MAX_ATTEMPTS` instead of `for (;;)`, so an
off-topic input can't spin forever on live model calls.
- parallel_worker sets `maxParallelWorkers: 2`, demonstrating the bounded
concurrency the README advertises.
- Normalize all nine sample headers that still used the raw
`node dev/dist/esm/cli_entrypoint.js run ...` form to `npm run sample -- ...`,
matching the README and the other samples.
- Unify how the four HITL samples parse a human reply: normalize with
`.trim().toLowerCase()` (so "Approve"/"approve " no longer fall through) and
share one affirmative vocabulary, instead of three different idioms.
- README: note that `loop`'s graph cycle is intentionally uncapped and can
iterate many times; add a dynamicEntry bullet to Feature coverage.
- Fix a prompt typo ("relates the the" -> "relates to the") in parallel_worker.
…folders Add integration tests that run the real workflow samples end-to-end with only the model mocked, and reorganize tests/integration/workflows so every test lives in its own subfolder. - Harness (tests/integration/workflows/_harness/): a RecordReplayModel registered into LLMRegistry mocks the model boundary for every agent — including ones captured inside a dynamicEntry/ctx.runNode closure — matching recorded responses to requests by a stable, id-normalized fingerprint (concurrency/order independent). sample_harness runs the real sample rootAgent through an InMemoryRunner; record mode (RECORD_MODEL_RESPONSES=1) calls the live model and writes the fixture, replay is offline. rng provides a seeded PRNG for the model-free non-deterministic samples (retry, loop_self). - 21 of 22 samples covered, one folder each: agent.ts (vendored from the sample) + <sample>_test.ts + model_responses.json where model-backed; offline samples need no fixture. auth_oauth is skipped (needs a live OAuth provider). - Add `npm run record:samples` to re-record the model-backed fixtures. - Move the existing Part 6 workflow integration tests into per-test subfolders (workflow_test_utils.ts -> _harness/; node_as_tool_test.ts -> node_as_tool_llm/ to avoid colliding with the sample's node_as_tool/ folder), fixing relative imports only. No Part 6 test logic changed.
5773943 to
5370380
Compare
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Problem: The workflow API needs runnable, discoverable examples.
Solution — Part 8 of 8 (final). Stacked on Part 7. Adds
samples/workflows/covering the API surface:samples/workflows/README.mdand a rootsamplescript to run them.Testing Plan
@google/adksurface and typecheck cleanly against source (verified with a temporary tsconfig aliasing@google/adk→core/src).Manual E2E: Each sample is runnable via
npm run sample -- samples/workflows/<name>.Checklist
Additional context
Final part of the stacked split (…Part 7 → Part 8). Diff: 24 files, +1,973.