Skip to content

feat(backend): add conditional branch node - #27

Open
LukasHirt wants to merge 1 commit into
mainfrom
feat/conditional-branch-node
Open

feat(backend): add conditional branch node#27
LukasHirt wants to merge 1 commit into
mainfrom
feat/conditional-branch-node

Conversation

@LukasHirt

Copy link
Copy Markdown
Collaborator

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 a nodeKind: '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 against vars (via the existing render() 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. WorkflowEdge gains a new sourceHandle field (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 existing WorkflowNodeData.condition / EdgeData.Condition fields 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 (sourceHandle mirrors Vue Flow's own sourceHandle concept 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. orderedNodes used to precompute a flat, static BFS order once, then Run looped over it. That doesn't work once traversal can depend on a runtime comparison, so I folded the BFS directly into Run's loop: each node is evaluated as it's dequeued, and then its outgoing edges are chosen. For every node kind except condition, this behaves exactly as before — follow every outgoing edge unconditionally (this is what trigger/llm/action nodes have always done, and still only ever have one output). For a condition node, after runCondition evaluates the comparison, only the outgoing edge(s) whose SourceHandle matches 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/notEquals are exact string comparison post-render; contains/notContains are substring checks (strings.Contains); matches runs regexp.MatchString(right, left) (right-hand value is the pattern). An invalid regex pattern is caught and surfaced as a normal nodeFailed execution error (same shape as any other node failure) rather than propagating a panic — covered by TestRunConditionInvalidRegexIsAClearErrorNotAPanic.

What's implemented

Backend (backend/pkg/executor/executor.go, backend/pkg/model/workflow.go):

  • runCondition alongside runLLM/runAction, evaluating the five operators above.
  • Run's traversal loop now evaluates nodes as it dequeues them (via new indexGraph/targetsFor helpers) instead of walking a precomputed static order, so it can consult runCondition'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-existing EdgeData.Condition.

Frontend:

  • frontend/src/components/nodes/ConditionNode.vue — new node component with one target Handle and two source Handles (id="true"/id="false"), each with its own "+ True" / "+ False" add-next button; positioned via new rules in the shared frontend/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 — new condition node kind under a new Logic category.
  • frontend/src/views/WorkflowBuilder.vue — wired into the #node-condition slot; openPicker/onPickNodeType now thread an optional source handle through so edges leaving a condition node get the right sourceHandle.
  • frontend/src/components/NodeDetailsPanel.vue — new left/operator/right config 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.
  • Types: frontend/src/types/workflow.ts gains ConditionOperator, the condition-node data fields, 'condition' in WorkflowNode.type, and WorkflowEdge.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

  • Backend: go build ./..., go vet ./..., go test ./... — all pass, including new tests in backend/pkg/executor/executor_condition_test.go:
    • all 5 operators × true/false + invalid regex + unknown operator (table-driven TestRunConditionOperators)
    • true branch runs / false branch doesn't, and vice versa (TestRunConditionTrueBranchOnlyExecutesTrueBranch, TestRunConditionFalseBranchOnlyExecutesFalseBranch)
    • single-branch-wired dead end is clean, not an error (TestRunConditionDeadEndsCleanlyWhenOnlyOneBranchIsWired)
    • invalid regex is a clear execution error, not a panic (TestRunConditionInvalidRegexIsAClearErrorNotAPanic)
    • regression: non-condition multi-edge fan-out unchanged (TestRunFansOutFromNonConditionNode), full pre-existing suite still green
  • Frontend: pnpm test:unit (17/17 passing) — new suites nodeTypes.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 build all clean
  • reuse lint and node scripts/check-licenses.mjs clean (no license/header issues from new files)

🤖 Generated with Claude Code

@LukasHirt
LukasHirt requested a review from a team as a code owner July 24, 2026 16:27
@LukasHirt LukasHirt self-assigned this Jul 24, 2026
@LukasHirt
LukasHirt force-pushed the feat/conditional-branch-node branch from 200f5d8 to 24d5dac Compare July 24, 2026 20:59
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>
@LukasHirt
LukasHirt force-pushed the feat/conditional-branch-node branch from 24d5dac to 24b33cc Compare July 27, 2026 14:27

@dj4oC dj4oC left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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, merge is 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 matchedHandle staying nil on a runCondition error is irrelevant, since the outer err != nil check already breaks the loop before targetsFor is 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 the matches test cases directly.
  • Go's regexp package (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 sourceHandle is a new field rather than repurposing the existing free-form condition/EdgeData.Condition fields, 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's operatorSymbol lookup is typed Record<ConditionOperator, string> (not the looser Record<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 the nil vs &"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 → condition chain 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 in indexGraph/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

@dj4oC
dj4oC self-requested a review July 28, 2026 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants