Skip to content

Commit be8ae65

Browse files
authored
fix(validate): reject condition edges routed on the wrong port field (#8)
B24: routing is keyed exclusively on `from_port` (see `engine::outgoing_by_port` / `handler_routing`), but nothing in `validate::validate` checked that a `condition` node's outgoing edges actually emit on `from_port` "true"/"false". An edge authored with the branch label on `to_port` instead (e.g. `{from_port:"main", to_port:"true"}` and `{from_port:"main", to_port:"false"}`) puts both edges in the same `from_port` group, which `handler_routing` classifies as a parallel `FanOut` — silently driving BOTH branches unconditionally, with no validation error or routing-divergence warning to flag the mistake. Add a hard structural check: every outgoing edge from a `condition` node must have `from_port` in {"true", "false"}, or `validate` now rejects the graph with a new `ValidationError::InvalidConditionRouting` naming the offending node and port. A condition with only one branch wired (a common intentional "gate/filter" shape) is still accepted.
1 parent db4165f commit be8ae65

2 files changed

Lines changed: 208 additions & 0 deletions

File tree

src/error.rs

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,31 @@ pub enum ValidationError {
7474
/// The highest schema version this crate understands.
7575
supported: u32,
7676
},
77+
78+
/// A `condition` node has an outgoing edge whose `from_port` is not one of
79+
/// its two declared branch ports (`"true"` / `"false"`).
80+
///
81+
/// Routing is keyed EXCLUSIVELY on `from_port` (see `engine::outgoing_by_port`
82+
/// / `handler_routing`) — `to_port` is never consulted to decide which
83+
/// successor fires. A condition node authored with the branch label on
84+
/// `to_port` instead (e.g. `{from_port:"main", to_port:"true"}` and
85+
/// `{from_port:"main", to_port:"false"}`) puts both edges in the SAME
86+
/// `from_port` group, which `handler_routing` classifies as a parallel
87+
/// `FanOut` — silently driving BOTH branches unconditionally instead of
88+
/// gating on the condition's actual result. This is a HARD authoring
89+
/// mistake, not a runtime data issue, so it is rejected here rather than
90+
/// left as a silent no-op condition.
91+
#[error(
92+
"condition node {node} has an outgoing edge with from_port {from_port:?} — condition \
93+
edges must emit on from_port \"true\" or \"false\" (the branch label belongs on \
94+
from_port, not to_port; routing is keyed exclusively on from_port)"
95+
)]
96+
InvalidConditionRouting {
97+
/// The offending condition node's id.
98+
node: String,
99+
/// The edge's actual (invalid) `from_port` value.
100+
from_port: String,
101+
},
77102
}
78103

79104
/// Errors produced while compiling or running a workflow.
@@ -160,6 +185,16 @@ mod tests {
160185
"schema_version 5 is newer than this crate supports (max 1); \
161186
upgrade tinyflows to load this graph"
162187
);
188+
assert_eq!(
189+
ValidationError::InvalidConditionRouting {
190+
node: "gate".to_string(),
191+
from_port: "main".to_string(),
192+
}
193+
.to_string(),
194+
"condition node gate has an outgoing edge with from_port \"main\" — condition \
195+
edges must emit on from_port \"true\" or \"false\" (the branch label belongs on \
196+
from_port, not to_port; routing is keyed exclusively on from_port)"
197+
);
163198
}
164199

165200
#[test]

src/validate.rs

Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,32 @@ pub fn validate(graph: &WorkflowGraph) -> Result<(), ValidationError> {
113113
}
114114
}
115115

116+
// A `condition` node's outgoing edges must emit on `from_port` "true" or
117+
// "false" — routing is keyed EXCLUSIVELY on `from_port` (see
118+
// `engine::outgoing_by_port` / `handler_routing`), so any other value
119+
// (most commonly the default `"main"`, from an authoring mistake that put
120+
// the branch label on `to_port` instead) is a hard authoring bug: it
121+
// silently degrades to a parallel `FanOut` that drives BOTH branches
122+
// unconditionally, with no runtime error or warning to point at the
123+
// mistake. Caught here, at the door, instead.
124+
let condition_node_ids: HashSet<&str> = graph
125+
.nodes
126+
.iter()
127+
.filter(|n| n.kind == NodeKind::Condition)
128+
.map(|n| n.id.as_str())
129+
.collect();
130+
for edge in &graph.edges {
131+
if condition_node_ids.contains(edge.from_node.as_str())
132+
&& edge.from_port != "true"
133+
&& edge.from_port != "false"
134+
{
135+
return Err(ValidationError::InvalidConditionRouting {
136+
node: edge.from_node.clone(),
137+
from_port: edge.from_port.clone(),
138+
});
139+
}
140+
}
141+
116142
Ok(())
117143
}
118144

@@ -457,6 +483,153 @@ mod tests {
457483
assert_eq!(validate(&graph), Ok(()));
458484
}
459485

486+
fn condition_node(id: &str) -> Node {
487+
node(id, NodeKind::Condition)
488+
}
489+
490+
#[test]
491+
fn accepts_condition_with_branch_label_on_from_port() {
492+
// The CORRECT shape (B23/B24): the branch label lives on `from_port`,
493+
// `to_port` stays `"main"`.
494+
let graph = WorkflowGraph {
495+
nodes: vec![
496+
node("t", NodeKind::Trigger),
497+
condition_node("gate"),
498+
node("yes", NodeKind::Agent),
499+
node("no", NodeKind::Agent),
500+
],
501+
edges: vec![
502+
Edge {
503+
from_node: "t".to_string(),
504+
from_port: "main".to_string(),
505+
to_node: "gate".to_string(),
506+
to_port: "main".to_string(),
507+
},
508+
Edge {
509+
from_node: "gate".to_string(),
510+
from_port: "true".to_string(),
511+
to_node: "yes".to_string(),
512+
to_port: "main".to_string(),
513+
},
514+
Edge {
515+
from_node: "gate".to_string(),
516+
from_port: "false".to_string(),
517+
to_node: "no".to_string(),
518+
to_port: "main".to_string(),
519+
},
520+
],
521+
..Default::default()
522+
};
523+
assert_eq!(validate(&graph), Ok(()));
524+
}
525+
526+
#[test]
527+
fn accepts_condition_with_only_one_branch_wired() {
528+
// Wiring only the `true` (or only the `false`) branch is legal — the
529+
// other simply dead-ends.
530+
let graph = WorkflowGraph {
531+
nodes: vec![
532+
node("t", NodeKind::Trigger),
533+
condition_node("gate"),
534+
node("yes", NodeKind::Agent),
535+
],
536+
edges: vec![
537+
Edge {
538+
from_node: "t".to_string(),
539+
from_port: "main".to_string(),
540+
to_node: "gate".to_string(),
541+
to_port: "main".to_string(),
542+
},
543+
Edge {
544+
from_node: "gate".to_string(),
545+
from_port: "true".to_string(),
546+
to_node: "yes".to_string(),
547+
to_port: "main".to_string(),
548+
},
549+
],
550+
..Default::default()
551+
};
552+
assert_eq!(validate(&graph), Ok(()));
553+
}
554+
555+
#[test]
556+
fn rejects_condition_with_branch_label_on_to_port_instead_of_from_port() {
557+
// The BAD shape (B23/B24 — the exact bug the workflow_builder agent
558+
// produced live): both edges share `from_port: "main"` with the branch
559+
// label on `to_port` instead. Without this check, `handler_routing`
560+
// would see one `from_port` group with two targets and classify it as
561+
// a parallel `FanOut`, silently driving BOTH branches unconditionally.
562+
let graph = WorkflowGraph {
563+
nodes: vec![
564+
node("t", NodeKind::Trigger),
565+
condition_node("gate"),
566+
node("yes", NodeKind::Agent),
567+
node("no", NodeKind::Agent),
568+
],
569+
edges: vec![
570+
Edge {
571+
from_node: "t".to_string(),
572+
from_port: "main".to_string(),
573+
to_node: "gate".to_string(),
574+
to_port: "main".to_string(),
575+
},
576+
Edge {
577+
from_node: "gate".to_string(),
578+
from_port: "main".to_string(),
579+
to_node: "yes".to_string(),
580+
to_port: "true".to_string(),
581+
},
582+
Edge {
583+
from_node: "gate".to_string(),
584+
from_port: "main".to_string(),
585+
to_node: "no".to_string(),
586+
to_port: "false".to_string(),
587+
},
588+
],
589+
..Default::default()
590+
};
591+
assert_eq!(
592+
validate(&graph),
593+
Err(ValidationError::InvalidConditionRouting {
594+
node: "gate".to_string(),
595+
from_port: "main".to_string(),
596+
})
597+
);
598+
}
599+
600+
#[test]
601+
fn rejects_condition_with_unrecognized_from_port() {
602+
let graph = WorkflowGraph {
603+
nodes: vec![
604+
node("t", NodeKind::Trigger),
605+
condition_node("gate"),
606+
node("other", NodeKind::Agent),
607+
],
608+
edges: vec![
609+
Edge {
610+
from_node: "t".to_string(),
611+
from_port: "main".to_string(),
612+
to_node: "gate".to_string(),
613+
to_port: "main".to_string(),
614+
},
615+
Edge {
616+
from_node: "gate".to_string(),
617+
from_port: "maybe".to_string(),
618+
to_node: "other".to_string(),
619+
to_port: "main".to_string(),
620+
},
621+
],
622+
..Default::default()
623+
};
624+
assert_eq!(
625+
validate(&graph),
626+
Err(ValidationError::InvalidConditionRouting {
627+
node: "gate".to_string(),
628+
from_port: "maybe".to_string(),
629+
})
630+
);
631+
}
632+
460633
#[test]
461634
fn duplicate_id_is_reported_before_trigger_checks() {
462635
// Two agents sharing an id and no trigger: the duplicate-id check runs

0 commit comments

Comments
 (0)