Skip to content
8 changes: 6 additions & 2 deletions packages/server/src/utils/buildAgentflow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -829,10 +829,14 @@ async function determineNodesToIgnore(
.filter((index) => index !== -1)

// Find nodes to ignore based on unfulfilled conditions
// Note: a single condition output can fan out to multiple target nodes
// (e.g. parallel branches sharing the same `-output-${index}` handle), so we
// must collect ALL matching edges. Using find() here would only ignore the
// first target and let the remaining parallel nodes leak into other branches.
for (const index of unfulfilledIndexes) {
const ignoreEdge = edges.find((edge) => edge.source === nodeId && edge.sourceHandle === `${nodeId}-output-${index}`)
const ignoreEdges = edges.filter((edge) => edge.source === nodeId && edge.sourceHandle === `${nodeId}-output-${index}`)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix. I think this deserves a focused regression test because this branch execution bug is easy to bring back accidentally.

The case I would like to highlight in is a condition node where one unfulfilled output handle fans out to two target nodes, while another output is fulfilled. The test should assert that both targets connected to the unfulfilled handle are ignored and neither is enqueued/executed so that would specifically protect the difference between the old find(...) behavior and the new filter(...) behavior here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review, and I appreciate the suggestion. You're right that this branch execution bug is easy to reintroduce, so a focused regression test makes a lot of sense.

I looked into adding one, but determineNodesToIgnore currently lives inside buildAgentflow.ts, which imports the full execution and runtime dependency graph (storage providers and so on) at module load time. Because of that, unit testing the function in isolation would mean either extracting it into its own module or mocking a lot of unrelated internals in the test. Both of those go well beyond the small fix in this PR.

Since that's a more structural change, I'd rather not expand the scope here on my own. @HenryHengZJ, would you prefer I extract the function and add the regression test in this PR, or keep this one minimal and follow up separately? I'm happy to go either way based on your call.


if (ignoreEdge) {
for (const ignoreEdge of ignoreEdges) {
ignoreNodeIds.push(ignoreEdge.target)
}
}
Expand Down