feat(workflow): event model extensions and shared node primitives (Part 1) - #587
feat(workflow): event model extensions and shared node primitives (Part 1)#587kalenkevich wants to merge 5 commits into
Conversation
AmaadMartin
left a comment
AmaadMartin
left a comment
There was a problem hiding this comment.
Read the whole thing against the head tree. The stacking strategy is the right call — recomposing from the final tree instead of replaying 41 commits is what makes this reviewable, and the note about the 3-way merge on event.ts saved me from misreading the 2 removed lines as a reversion. Tests are real (the retry tests inject the RNG rather than asserting on randomness), and CI is green on all three OS legs.
One real bug, then a few type-discipline points that I'm raising because they're the same standard this repo holds contributors to.
There's also a behaviour change in here that the description undersells — see the note on event.ts:135. Reviewers of Parts 2-9 will want to know it landed in Part 1.
| constructor(message = 'Node interrupted (awaiting resume input).') { | ||
| super(message); | ||
| this.name = 'NodeInterruptedError'; | ||
| // Restore prototype chain for `instanceof` across transpilation targets. |
There was a problem hiding this comment.
Nit — instanceof as the dispatch mechanism, in the one place it's known to break.
// Restore prototype chain for `instanceof` across transpilation targets.
Object.setPrototypeOf(this, NodeInterruptedError.prototype);The setPrototypeOf calls are correct for what they do. The question is the strategy: this is the objection you raised on #364 — "Problem with instanceof is that when user will have multiple adk-js packages in their runtime it will not able to mix objects from one to another" — which is why isGeminiModel-style guards exist instead.
These three errors are caught across the node/engine boundary (NodeInterruptedError by the parent's NodeRunner, DynamicNodeFailError likewise), which is exactly where two copies of the package would meet: the throw site resolves one class, the catch site another, and the instanceof silently returns false — the node hangs instead of resuming.
Exporting guards alongside the classes would keep the catch sites copy-safe:
export function isNodeInterruptedError(e: unknown): e is NodeInterruptedError {
return e instanceof Error && e.name === 'NodeInterruptedError';
}Fine to defer to Part 2 where the catch sites actually land, but better decided before three more parts are written against instanceof.
AmaadMartin
left a comment
AmaadMartin
left a comment
There was a problem hiding this comment.
Follow-up: one finding I missed on the first pass. I checked each new primitive against what already exists in core/src and EventChannel has a near-twin — details inline on event_channel.ts.
I also chased three other hypotheses and they came back clean, so recording them here rather than filing noise: the RetryConfig.exceptions string-or-class union and the exact-name (non-subclass) matching in shouldRetryNode both faithfully mirror _retry_config.py / _retry_utils.py — Python does exactly the same via field_validator and type(exception).__name__, so no parity divergence; the new Trigger collides with nothing on main (only ContextCompactionTrigger exists, unrelated); and a diff-hygiene scan is clean — no CHANGELOG/lockfile churn, no stray artifacts, zero console.log, zero .only/.skip, zero type suppressions, and event_test.ts is +45/-0 so no existing test was rewritten.
Resolves review comments on #587: - events: drop 6 unused EventActions fields (output, joinCompleted, toolExecution, requestInput, nodeExecutionReplay, route) that were dead across the whole branch and left arbitrary `unknown` payloads unpreserved; removing them also eliminates the round-trip hole and the ambiguous second `route` carrier. Add shared RouteKey/Route types for Event.route. - errors: add name-based guards (isNodeInterruptedError, isNodeTimeoutError, isDynamicNodeFailError) so catch sites stay correct across duplicate-package boundaries instead of relying on `instanceof`. - branch_path: convert createSubBranch/commonPrefixOf from static methods to standalone util functions (kept fromString/commonPrefix as factories). - async_queue: fold EventChannel into the existing AsyncQueue instead of shipping a duplicate queue — port drain-before-error, sticky failure, and error()-closes; add isClosed/size getters and a fail() method (error() retained as an alias). Delete event_channel.ts and move its tests. This also fixes the close()-then-fail() error-swallow bug. - retry: introduce prepareRetryConfig/PreparedRetryConfig so a node's exception filter is normalized+validated once at construction, not re-normalized (and potentially thrown) on every retry check; make shouldRetryNode/ getRetryDelaySeconds take a required prepared config and drop the unreachable no-config branches (and the misleading test that pinned one). Full core suite green (2338 tests).
kalenkevich
changed the title
92b4c12 to
953242b
Compare
First slice of the workflow engine split (Part 1/9). Adds the leaf
primitives the engine builds on, none of which depend on the engine core:
- workflow/errors, node_status, node_state, retry_config,
utils/retry_utils, trigger, branch_path, and utils/event_channel
- Event model extensions mirroring adk-python: Event.{output, route,
nodeInfo, isolationScope}, the NodeInfo type, CreateEventParams, an
isEvent guard, and additive optional EventActions fields; PRESERVE_KEYS
entries so node output and checkpointed state survive snake/camel
round-trips
- export the new event types from common.ts
Bundled tests: foundations, event_channel, event_model, and event.
Recomposed from the final feature/workflows tree onto current main
(3-way merged event.ts to preserve main's relocated function-call-id
helpers). Stacked PRs follow.
Address PR1 review feedback: identify Event objects via a
Symbol.for('google.adk.event') brand — set by createEvent and checked by
isEvent — matching the signature-symbol guards used across ADK (isBaseTool,
isBaseAgent, ...) instead of structural duck-typing.
The brand is declared optional on the Event interface and is a
non-serializable runtime marker, so events reconstructed from storage/session
payloads are intentionally unbranded (documented on the interface and covered
by a round-trip test). Adds isEvent unit tests (branded event, impostor
rejection, non-object rejection, round-trip brand drop).
Resolves review comments on #587: - events: drop 6 unused EventActions fields (output, joinCompleted, toolExecution, requestInput, nodeExecutionReplay, route) that were dead across the whole branch and left arbitrary `unknown` payloads unpreserved; removing them also eliminates the round-trip hole and the ambiguous second `route` carrier. Add shared RouteKey/Route types for Event.route. - errors: add name-based guards (isNodeInterruptedError, isNodeTimeoutError, isDynamicNodeFailError) so catch sites stay correct across duplicate-package boundaries instead of relying on `instanceof`. - branch_path: convert createSubBranch/commonPrefixOf from static methods to standalone util functions (kept fromString/commonPrefix as factories). - async_queue: fold EventChannel into the existing AsyncQueue instead of shipping a duplicate queue — port drain-before-error, sticky failure, and error()-closes; add isClosed/size getters and a fail() method (error() retained as an alias). Delete event_channel.ts and move its tests. This also fixes the close()-then-fail() error-swallow bug. - retry: introduce prepareRetryConfig/PreparedRetryConfig so a node's exception filter is normalized+validated once at construction, not re-normalized (and potentially thrown) on every retry check; make shouldRetryNode/ getRetryDelaySeconds take a required prepared config and drop the unreachable no-config branches (and the misleading test that pinned one). Full core suite green (2338 tests).
typedoc `docs:check` runs with --treatWarningsAsErrors and flagged the [EVENT_SIGNATURE_SYMBOL] doc comment (inherited by Event, CreateEventParams and CompactedEvent) for linking to `isEvent`, which is not part of the exported/ documented API. Reference it as inline code instead of a doc link. No API or behavior change.
e8abbf4 to
9aee489
Compare
Aligns the Part 1 shared primitives with the workflow conventions applied across the stack. - branch_path: convert the BranchPath.fromString / BranchPath.commonPrefix static methods to plain functions (branchPathFromString / commonPrefixOfPaths); keep the instance methods. Internal callers (createSubBranch, commonPrefixOf) updated. - retry_utils: shouldRetryNode and getRetryDelaySeconds now take a single params object; errorName is duck-typed (no `instanceof Error`) so it works for error-like values across package copies. - Add branch_path unit coverage (previously untested) and update the retry_utils call sites in foundations_test to the params-object form. Error type guards keep the name-based `instanceof Error` form by design (the one sanctioned use of instanceof).
AmaadMartin
left a comment
AmaadMartin
left a comment
There was a problem hiding this comment.
Re-checked at 3e927fd. All ten items from the last round are addressed, and the EventChannel one landed the right way round: the file is gone and AsyncQueue picked up drain-before-error, sticky failure, fail-after-close and the six tests. I verified the PRESERVE_KEYS hole is actually closed (the five untyped actions.* fields were deleted rather than papered over), route now has a single carrier with a shared Route/RouteKey type, and prepareRetryConfig moved normalization to construction time so the retry path can't throw over the node's real error. Two small things below.
One housekeeping item: the description is now stale — it still lists utils/event_channel.ts (EventChannel) as included, claims event_actions.ts adds output/route, and the test plan reports an event_channel_test.ts run. None of those exist at head.
| * @param obj The object to check. | ||
| * @returns True if the object matches the Event structure. | ||
| */ | ||
| export function isEvent(obj: unknown): obj is Event { |
There was a problem hiding this comment.
Nit. The brand answers "was this object made by createEvent in this process", which is narrower than "is this an Event".
EVENT_SIGNATURE_SYMBOL in obj &&Three paths in the repo hand back Event-typed objects with no brand: transformToCamelCaseEvent below (toCamelCase(...) as Event), sessions/vertex_ai_session_service.ts:455 (JSON.parse(JSON.stringify(rawEvent)) as Event), and the plugin callback return cast in plugins/plugin_manager.ts:180. Your own test pins the first one — "drops the (non-serializable) brand across a snake/camel round-trip".
Nothing is broken today: Part 2's only caller, BaseNode.toEvent, only ever sees events it just created. But it fails in the unhelpful direction — a node that yields an event it loaded from a session takes the else path and gets re-wrapped as the output of a new event instead of being emitted as-is, and that's silent.
Cheapest fix is to brand at the rehydration boundary, since that is where events legitimately re-enter the process:
export function transformToCamelCaseEvent(event: Record<string, unknown>): Event {
const restored = toCamelCase(event, PRESERVE_KEYS_SNAKE_CASE) as Event;
return {...restored, [EVENT_SIGNATURE_SYMBOL]: true};
}That inverts the expectation in your round-trip test (isEvent(rehydrated) becomes true), which I think is the behaviour you want. I didn't check every deep-equality assertion on rehydrated events for symbol-key sensitivity, so worth a full run rather than just that file.
| } | ||
|
|
||
| /** @deprecated Alias for {@link fail}; kept for existing callers. */ | ||
| error(err: unknown) { |
There was a problem hiding this comment.
Nit, optional. The alias has exactly one caller, so the deprecation could just be the fix.
/** @deprecated Alias for {@link fail}; kept for existing callers. */
error(err: unknown) {AsyncQueue isn't exported from common.ts, and the only .error( call site in the repo is models/google_llm.ts:326 in the live onerror callback. Flipping that one line to fail(error) lets you delete the alias instead of shipping a deprecated method on day one.
Worth noting either way that this call site is the reason the AsyncQueue change is a real fix and not just a refactor: onerror followed by onclose used to clear the error after the first rejection and drop buffered messages ahead of it. Both are now correct.
AmaadMartin
left a comment
AmaadMartin
left a comment
There was a problem hiding this comment.
Re-reviewed at 3e927fd. Independently verified the five load-bearing items from my earlier rounds rather than taking the prior pass on trust: utils/event_channel.ts is gone and AsyncQueue absorbed the drain-before-error / sticky-failure / fail-after-close fixes plus the getters; the five untyped actions.* fields were deleted rather than added to PRESERVE_KEYS, which closes the round-trip hole at the source; EventActions.route is removed so routing has a single carrier behind a shared Route/RouteKey type; and isEvent is now brand-based rather than the typeof actions === 'object' check that accepted null.
The AsyncQueue outcome is the one I want to call out: I asked for the duplicate to be resolved and it landed the right way round — the better semantics were ported into the shared utility and the copy deleted, so google_llm.ts picks up the ordering fix for free.
CI is green across ubuntu/macOS/Windows (verified the individual check list, not the rollup). The two open nits from the last pass are non-blocking. LGTM.
One housekeeping item, non-blocking: the PR description still lists utils/event_channel.ts in the included-files section, which no longer exists — worth a quick edit so the merge commit message is accurate.
3 participants
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
Problem:
The
feature/workflowsbranch is a single, very large change (41 commits, ~15k lines across ~155 files) that is impractical to review or merge as one unit. It also carries an initial workflow implementation that a later rewrite fully replaced, so a straight commit-by-commit review would surface dead code.Solution:
Split the work into small, stacked, dependency-ordered PRs recomposed from the final tree state (not a replay of the 41 commits), so each PR is coherent and reviewable and reviewers never see the superseded implementation.
This is Part 1 of 9. It adds only the leaf primitives the engine builds on — none depend on the engine core — plus the additive event-model changes the engine needs. There is no orchestration, node, or runner wiring yet, so there is nothing user-facing to exercise beyond the new types.
Included:
core/src/workflow/):errors.ts(NodeInterruptedError,NodeTimeoutError,DynamicNodeFailError),node_status.ts(NodeStatus),node_state.ts(NodeState,createNodeState,isNodeState),retry_config.ts(RetryConfig,ErrorClass,normalizeRetryExceptions),utils/retry_utils.ts(shouldRetryNode,getRetryDelaySeconds),trigger.ts(Trigger),branch_path.ts(BranchPath),utils/event_channel.ts(EventChannel).adk-python):events/event.tsaddsEvent.{output, route, nodeInfo, isolationScope}, theNodeInfotype,CreateEventParams, anisEventguard, andPRESERVE_KEYSentries so nodeoutputand checkpointedagentStatesurvive snake/camel round-trips;events/event_actions.tsadds optional fields (output,agentState,endOfAgent,route, …);common.tsexports the new event types.Intentionally deferred to later parts: engine core (Part 2), node types (Part 3), parallelism (Part 4), dynamic scheduling (Part 5), workflow orchestrator + runner + public barrel export (Part 6), LLM-as-node + task mode (Part 7), HITL (Part 8), samples (Part 9). The
finish_task_toolexport incommon.tsand theworkflow/index.jsexport incore/src/index.tsare deferred to the PRs that introduce those files.Testing Plan
Unit Tests:
Bundled tests:
workflow/foundations_test.ts,workflow/event_channel_test.ts,workflow/event_model_test.ts,events/event_test.ts.Typecheck is clean:
npx tsc --noEmit -p core/tsconfig.json.Manual End-to-End (E2E) Tests:
N/A — this part adds only leaf primitives and additive types with no user-facing behavior. End-to-end coverage arrives with the runner integration in Part 6.
Checklist
Additional context