feat(backend): add conditional branch node - #27
Conversation
200f5d8 to
24d5dac
Compare
Implements the "Conditional Branch" control-flow node end to end: a new
condition node kind with a single input and two labeled outputs ("true"/
"false"), whose config (left template, operator, right value) the executor
evaluates at run time to decide which single downstream branch actually runs.
Design decisions:
- Edge/handle persistence: WorkflowEdge gains a new sourceHandle field
(backend model + frontend type) recording which output handle an edge
originates from. The existing WorkflowNodeData.condition / EdgeData.Condition
fields are a separate, still-unimplemented free-form per-node/per-edge gate
and are left untouched rather than repurposed — their shape (an unstructured
expression string) doesn't match this node's structured {left, operator,
right} comparison.
- Traversal: orderedNodes' static BFS is folded into Run's loop so branch
selection can be evaluated with the vars available at that point; every node
kind except "condition" still follows every outgoing edge unconditionally.
- No-matching-edge behavior: an evaluated branch with no wired outgoing edge is
a clean dead end, not an error — a condition node with only one branch
connected (e.g. "if spam, delete; otherwise do nothing") is a valid shape.
- Operators: equals/notEquals are exact string comparison, contains/
notContains are substring checks, matches is regexp.MatchString; an invalid
regex pattern is surfaced as a normal node-failed execution error.
Signed-off-by: Lukas Hirt <info@hirt.cz>
24d5dac to
24b33cc
Compare
dj4oC
left a comment
There was a problem hiding this comment.
Review: feat(backend): add conditional branch node
Stats: +785/-63 across 12 files (backend + frontend) — the largest of the recent batch of new-node-type PRs, and it earns the size.
Overview
Adds a condition node with one input and two labeled outputs, evaluated at run time (equals/notEquals/contains/notContains/matches) against templated vars, with traversal continuing down only the matching branch. This required actually restructuring Run's traversal — the old orderedNodes() precomputed a static execution order once and looped over it; that can't work once which edge to follow depends on a runtime comparison, so this PR folds the BFS directly into Run's loop, evaluating each node as it's dequeued and only then choosing its outgoing edges via the new targetsFor. I traced through the new traversal logic (queue/visited handling, the matchedHandle *string nil-means-unconditional convention, error-path short-circuiting) against several graph shapes — including the specific failure modes this kind of refactor tends to introduce (double-execution on reconvergence, premature execution of a shared downstream node, skipped error propagation) — and didn't find a correctness bug in it.
What I specifically checked
- Reconvergence after a branch (
cond → true → A → merge,cond → false → B → merge): since only one branch's edge is ever enqueued,mergeis only ever reached once, from whichever branch actually ran — no double-execution risk here, which is the shape this feature will actually be used for most. - The known general weakness of BFS-with-a-visited-set traversal (a node reachable via two paths of different length can be visited via the shorter path before the longer path's intermediate nodes have run) is real, but it's a pre-existing characteristic inherited unchanged from the old
orderedNodes()(same queue/visited-set shape), not something this PR introduces. It's only reachable today via plain multi-edge fan-out (not through condition branches, which are mutually exclusive), so it's not currently a live concern — flagging only because conditional branching makes "diamond-shaped" graphs a much more natural thing for users to build next, so it's worth keeping in mind if a future PR adds an explicit "merge/join" node. - Error propagation: confirmed
matchedHandlestayingnilon arunConditionerror is irrelevant, since the outererr != nilcheck already breaks the loop beforetargetsForis ever called with it — no path where an error is silently swallowed or a wrong branch gets taken after a failure. regexp.MatchString(right, left)argument order matches the PR's own stated design (right = pattern, left = subject) — verified against thematchestest cases directly.- Go's
regexppackage (RE2-based) doesn't have catastrophic-backtracking exposure the way PCRE-style engines do, so a condition node comparing large template-rendered values (e.g.{{file.content}}) against a user-supplied pattern isn't a ReDoS vector the way it might be in other languages — no new concern there.
Code quality & style
- The design-decisions section in the PR body is genuinely good practice — it documents why
sourceHandleis a new field rather than repurposing the existing free-formcondition/EdgeData.Conditionfields, and why dead-ending on an unwired branch is a deliberate non-error rather than a gap. That reasoning is exactly what's needed to evaluate the design, and it holds up. ConditionNode.vue'soperatorSymbollookup is typedRecord<ConditionOperator, string>(not the looserRecord<string, ...>I've flagged on a couple of the other node-type PRs in this batch) — this one gets compile-time exhaustiveness for free if a new operator is ever added. Good instinct.targetsFor's doc comment explaining thenilvs&"true"/&"false"convention is clear enough that I didn't need to cross-reference the call site to understand it.
Minor suggestions (non-blocking)
- Chained conditions are untested. Every test wires a single condition node between two leaf actions; a
condition → conditionchain isn't exercised. I traced through the logic and don't see a reason it wouldn't work (each condition independently evaluates and returns its own handle), but given this PR's own thoroughness elsewhere (table-driven operator tests, explicit dead-end and regression tests), a quick test locking this in would fit the pattern and cheaply rule out a corner case inindexGraph/targetsFor's interaction that I might have missed by inspection alone. - Nothing else stood out as worth raising — the CSS handle-positioning approach (hardcoded 32%/68% offsets) is a pragmatic, well-commented fix for a real Vue Flow rendering quirk (overlapping same-position handles), not a design concern.
Test coverage
Strong, and well-targeted at the specific risk this refactor carries: TestRunFansOutFromNonConditionNode is exactly the right regression test to lock in (multi-edge fan-out from a non-condition node must keep working unchanged), and TestRunConditionDeadEndsCleanlyWhenOnlyOneBranchIsWired / TestRunConditionInvalidRegexIsAClearErrorNotAPanic both target the two failure-shaped edge cases (not-wired branch, invalid input) that are easy to get wrong in this kind of change. Frontend coverage (mounting ConditionNode inside a real <VueFlow> instance rather than in isolation, since it needs a registered node) shows real attention to how Vue Flow actually behaves rather than testing around it.
Security
Nothing new. left/right go through the same render() templating every other action param already uses, with the same (pre-existing, shared-across-all-node-types) lack of output sanitization — not specific to this PR.
Summary
Well-designed, well-tested, and the trickiest part (changing traversal from precomputed-order to evaluate-as-you-go) holds up under scrutiny. The one thing worth adding before or shortly after merge is a chained-condition test; everything else here is in good shape.
🤖 Generated with Claude Code
Summary
Implements the "Conditional Branch" control-flow node end to end (previously just a proposal in
docs/superpowers/specs/2026-07-24-new-workflow-node-types-design.md, flagged there as needing its own design pass before implementation). Adds anodeKind: 'condition'canvas node with a single input and two labeled outputs ("true"/"false"). Its config is one comparison — a left-hand template, an operator, and a right-hand value — evaluated at run time againstvars(via the existingrender()function, exactly like every action param already is), and traversal continues down only the matching outgoing edge.Design decisions (resolved, not left open)
1. Edge/handle persistence.
WorkflowEdgegains a newsourceHandlefield (backend/pkg/model/workflow.go,frontend/src/types/workflow.ts) recording which of the source node's output handles an edge originates from — populated only for edges leaving a condition node ("true"/"false"), empty/absent for every other edge. I considered reusing the existingWorkflowNodeData.condition/EdgeData.Conditionfields instead of adding anything new, but rejected it: those are a free-form, unstructured expression string with no defined grammar and no interpreter, while the condition node needs a structured{left, operator, right}comparison this PR fully implements and evaluates. Repurposing a differently-shaped, still-undefined field would have been more confusing than adding one small, purpose-built, Vue-Flow-native field (sourceHandlemirrors Vue Flow's ownsourceHandleconcept directly). The old fields are left completely untouched — not removed, not repurposed — to avoid any scope creep into other in-flight work that may still reference them.2. Traversal semantics.
orderedNodesused to precompute a flat, static BFS order once, thenRunlooped over it. That doesn't work once traversal can depend on a runtime comparison, so I folded the BFS directly intoRun's loop: each node is evaluated as it's dequeued, and then its outgoing edges are chosen. For every node kind exceptcondition, this behaves exactly as before — follow every outgoing edge unconditionally (this is whattrigger/llm/actionnodes have always done, and still only ever have one output). For aconditionnode, afterrunConditionevaluates the comparison, only the outgoing edge(s) whoseSourceHandlematches the boolean result are enqueued. A regression test (TestRunFansOutFromNonConditionNode) explicitly locks in that non-condition multi-edge fan-out still works unchanged, and the full existing executor suite (TestRunTriggerLLMAction,TestRunStopsOnNodeFailure, etc.) passes unmodified.3. No-matching-edge behavior. If the evaluated branch ("true" or "false") has no outgoing edge wired up at all, execution simply stops for that branch — a clean dead end, not an error. This is deliberate: a condition node with only one branch connected (e.g. "if spam, delete; otherwise do nothing") is a completely normal, valid workflow shape, not a misconfiguration. Covered by
TestRunConditionDeadEndsCleanlyWhenOnlyOneBranchIsWired.4. Operator implementation.
equals/notEqualsare exact string comparison post-render;contains/notContainsare substring checks (strings.Contains);matchesrunsregexp.MatchString(right, left)(right-hand value is the pattern). An invalid regex pattern is caught and surfaced as a normalnodeFailedexecution error (same shape as any other node failure) rather than propagating a panic — covered byTestRunConditionInvalidRegexIsAClearErrorNotAPanic.What's implemented
Backend (
backend/pkg/executor/executor.go,backend/pkg/model/workflow.go):runConditionalongsiderunLLM/runAction, evaluating the five operators above.Run's traversal loop now evaluates nodes as it dequeues them (via newindexGraph/targetsForhelpers) instead of walking a precomputed static order, so it can consultrunCondition's result before choosing which edge(s) to follow next.WorkflowEdge.SourceHandle(new field,omitempty), with a doc comment clarifying it's unrelated to the pre-existingEdgeData.Condition.Frontend:
frontend/src/components/nodes/ConditionNode.vue— new node component with one targetHandleand two sourceHandles (id="true"/id="false"), each with its own "+ True" / "+ False" add-next button; positioned via new rules in the sharedfrontend/src/styles/canvas.css(Vue Flow centers same-position handles by default, so the two are pinned to 32%/68% top offsets to avoid overlapping).frontend/src/nodeTypes.ts— newconditionnode kind under a newLogiccategory.frontend/src/views/WorkflowBuilder.vue— wired into the#node-conditionslot;openPicker/onPickNodeTypenow thread an optional source handle through so edges leaving a condition node get the rightsourceHandle.frontend/src/components/NodeDetailsPanel.vue— newleft/operator/rightconfig fields for condition nodes. Also hides the generic legacy "Run only if (optional condition)" field specifically on condition nodes (showing an unrelated dead field labeled "condition" on a node whose entire purpose is evaluating a condition would be actively confusing); it's unchanged for every other node type.frontend/src/types/workflow.tsgainsConditionOperator, the condition-node data fields,'condition'inWorkflowNode.type, andWorkflowEdge.sourceHandle.Out of scope (explicitly, per the design doc): truly cyclic graphs (loops back to an earlier node) — this only prunes which single path through an otherwise-still-linear-per-branch DAG gets taken.
Test plan
go build ./...,go vet ./...,go test ./...— all pass, including new tests inbackend/pkg/executor/executor_condition_test.go:TestRunConditionOperators)TestRunConditionTrueBranchOnlyExecutesTrueBranch,TestRunConditionFalseBranchOnlyExecutesFalseBranch)TestRunConditionDeadEndsCleanlyWhenOnlyOneBranchIsWired)TestRunConditionInvalidRegexIsAClearErrorNotAPanic)TestRunFansOutFromNonConditionNode), full pre-existing suite still greenpnpm test:unit(17/17 passing) — new suitesnodeTypes.spec.ts,ConditionNode.spec.ts(mounted inside a real<VueFlow>instance, asserting both handles render and the add-next buttons emit the right branch id),NodeDetailsPanel.spec.ts(condition fields render/patch correctly, and don't leak into other node types)pnpm check:types,pnpm lint,pnpm buildall cleanreuse lintandnode scripts/check-licenses.mjsclean (no license/header issues from new files)🤖 Generated with Claude Code