fix(workflow): stamp the invocation id on an event a node built itself - #747
fix(workflow): stamp the invocation id on an event a node built itself#747kalenkevich wants to merge 1 commit into
Conversation
AmaadMartin
left a comment
There was a problem hiding this comment.
Verified against source at head. The fix stamps child.invocationId only when the node left it unset, which matches enrichEvent's existing fill-when-unset rule. createEvent defaults the field to '' (events/event.ts:176), so the falsy guard is correct, and both tests match the harness (createIc sets 'inv-1'). CI is green on ubuntu, macOS and Windows.
| // event a node built itself is persisted with a blank invocation id while | ||
| // every sibling event of the same invocation carries the real one. Test the | ||
| // falsy value, not `undefined`. | ||
| if (!event.invocationId) { |
There was a problem hiding this comment.
Nit, optional. The docstring above (lines 562-568) still lists only author, branch and isolationScope as the fill-when-unset fields. Add invocationId to that list so the doc matches the code.
AmaadMartin
left a comment
There was a problem hiding this comment.
Re-verified at head db327be, unchanged since the prior approval. The fix stamps child.invocationId only when the event has none, which matches enrichEvent's fill-when-unset rule; createEvent defaults the field to '' (events/event.ts:176), so the falsy guard is correct. Both tests run through enrichEvent and pin the stamp and the no-clobber case. One cosmetic nit stays open: the docstring still omits invocationId. CI is green on all three runners.
`enrichEvent` stamps `author`, `nodeInfo.path`, `branch` and `isolationScope`, but never `invocationId`. An `Event` a node hands back from `createEvent()` is passed through untouched, and `createEvent` defaults the field to `''` rather than leaving it undefined — so the event is persisted with a blank invocation id while every sibling event of the same invocation carries the real one. The damage is to persisted data and to anything that groups by invocation. The Dev UI graph, for one, never highlights the edge into such a node: `getWorkflowHighlights` breaks its backward walk on the first mismatch. Reproduced on `routes/branches`, `routes/function_node`, `data_handling/node_output`, `data_handling/routing_output`, `data_handling/structured_output` and `graphs/get_started`. The carry-forward workaround in `rehydration_utils` stays: sessions written before this change still hold `''`, and engine-minted interrupt events still exist. Fixes #715
db327be to
0c19117
Compare
AmaadMartin
left a comment
There was a problem hiding this comment.
Re-verified at head 0c19117. The fix stamps child.invocationId only when the event has none, which matches enrichEvent's fill-when-unset rule, and createEvent defaults the field to '' so the falsy guard is correct. The passthrough test fails against the unfixed code. The narration comment is gone. My earlier optional nit stays open: the docstring omits invocationId from the fill-when-unset list. CI was green at review time.
enrichEvent(core/src/workflow/node_runner.ts:570) stampsauthor,nodeInfo.path,branchandisolationScope— and neverinvocationId. AnEventa node hands back fromcreateEvent()is passed through untouched (nodes/function_node.ts:222,base_node.ts:221), while every constructing branch does set it.createEventdefaults the field to''rather than leaving it undefined (events/event.ts:176), so the event is persisted with a blank invocation id while every sibling event of the same invocation carries the real one:The damage is to persisted data and to anything that groups by invocation. The Dev UI graph, for one, never highlights the edge into such a node:
getWorkflowHighlights(dev/src/server/agent_graph.ts:489) breaks its backward walk on the firstinvocationIdmismatch.Reproduced on
routes/branches,routes/function_node,data_handling/node_output,data_handling/routing_output,data_handling/structured_outputandgraphs/get_started. It is live in the repo's own fixtures too —tests/integration/workflows/route/agent.ts:44and:62both return a barecreateEvent.Notes for review
undefined, becausecreateEventwrites''.enrichEvent's own documented rule — only fill in what the node left unset — so a node can still set its own id.rehydration_utils.ts:78-90stays: sessions written before this change still hold'', and engine-minted interrupt events still exist. Its comment ("Not every node event carries an invocation id…") is this bug documented as a feature, and can be revisited separately.invocationIdis inIGNORE_FIELDS(tests/integration/test_case_utils.ts:160-171) and is already regex-asserted at :239.Tests
core/test/workflow/node_execution_test.ts— extends the existing "passes through an explicitly emitted Event" case, plus a new one asserting an id the node set itself is not clobbered. Fullunit:core(3100) and workflow + docs integration (125) green.Fixes #715