fix(graph): reject as == output_key on map nodes
A map node whose item binding shares its name with output_key would have the binding collected as the chain's result whenever the chain never wrote it, while the missing-writer warning stayed silent. The collision is now a validator error attributed to the map node and, with validation off, a runtime error in run_map before any item forks.
This commit is contained in:
2 files changed
+116
No files matched your search
@@ -55,6 +55,13 @@ async fn run_map(
|
|||||||
node.branch
|
node.branch
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
if node.as_name == node.output_key {
|
||||||
|
bail!(
|
||||||
|
"map node '{node_id}': `as` and `output_key` are both '{}'; the item binding \
|
||||||
|
would be collected as the chain's result",
|
||||||
|
node.as_name
|
||||||
|
);
|
||||||
|
}
|
||||||
let subgraph = Arc::new(branch_subgraph(&step_ctx.graph, &node.branch));
|
let subgraph = Arc::new(branch_subgraph(&step_ctx.graph, &node.branch));
|
||||||
|
|
||||||
let max_conc = resolve_max_concurrency(node, state, step_ctx.max_concurrency, node_id)?;
|
let max_conc = resolve_max_concurrency(node, state, step_ctx.max_concurrency, node_id)?;
|
||||||
@@ -810,6 +817,55 @@ nodes:
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// With validation off, the collision is still caught before any item
|
||||||
|
/// forks: the branch never runs.
|
||||||
|
#[tokio::test]
|
||||||
|
async fn map_as_equal_output_key_fails_at_runtime() {
|
||||||
|
if !cmd_available("bash") {
|
||||||
|
eprintln!("skipping: bash not available");
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
let ws = TestWorkspace::new();
|
||||||
|
let sentinel = ws.dir.join("ran");
|
||||||
|
ws.write_script(
|
||||||
|
"mark.sh",
|
||||||
|
&format!("#!/bin/bash\ntouch '{}'\necho '{{}}'\n", sentinel.display()),
|
||||||
|
);
|
||||||
|
|
||||||
|
let yaml = r#"
|
||||||
|
name: as_output_collision_test
|
||||||
|
start: fan_out
|
||||||
|
settings:
|
||||||
|
validate_before_run: false
|
||||||
|
initial_state:
|
||||||
|
items: [1, 2]
|
||||||
|
nodes:
|
||||||
|
fan_out:
|
||||||
|
type: map
|
||||||
|
over: "{{items}}"
|
||||||
|
as: output
|
||||||
|
branch: mark
|
||||||
|
collect_into: results
|
||||||
|
next: done
|
||||||
|
mark:
|
||||||
|
type: script
|
||||||
|
script: mark.sh
|
||||||
|
done:
|
||||||
|
type: end
|
||||||
|
output: "{{results}}"
|
||||||
|
"#;
|
||||||
|
let chain = error_chain(run_graph(yaml, &ws).await);
|
||||||
|
|
||||||
|
assert!(
|
||||||
|
chain.contains(
|
||||||
|
"map node 'fan_out': `as` and `output_key` are both 'output'; the item binding \
|
||||||
|
would be collected as the chain's result"
|
||||||
|
),
|
||||||
|
"{chain}"
|
||||||
|
);
|
||||||
|
assert!(!sentinel.exists(), "no item chain may start");
|
||||||
|
}
|
||||||
|
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn map_chain_two_script_steps_route_via_next() {
|
async fn map_chain_two_script_steps_route_via_next() {
|
||||||
if !cmd_available("python3") {
|
if !cmd_available("python3") {
|
||||||
|
|||||||
@@ -643,6 +643,16 @@ impl GraphValidator {
|
|||||||
let NodeType::Map(m) = &node.node_type else {
|
let NodeType::Map(m) = &node.node_type else {
|
||||||
continue;
|
continue;
|
||||||
};
|
};
|
||||||
|
if m.as_name == m.output_key {
|
||||||
|
result.error(ValidationError::with_node(
|
||||||
|
map_id,
|
||||||
|
format!(
|
||||||
|
"map node '{map_id}': `as` and `output_key` are both '{}'; the item \
|
||||||
|
binding would be collected as the chain's result",
|
||||||
|
m.as_name
|
||||||
|
),
|
||||||
|
));
|
||||||
|
}
|
||||||
// A missing entry is reported by `validate_node_references`.
|
// A missing entry is reported by `validate_node_references`.
|
||||||
let mut members: Vec<String> = branch_subgraph(graph, &m.branch).into_iter().collect();
|
let mut members: Vec<String> = branch_subgraph(graph, &m.branch).into_iter().collect();
|
||||||
if members.is_empty() {
|
if members.is_empty() {
|
||||||
@@ -3505,6 +3515,56 @@ mod tests {
|
|||||||
map
|
map
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn map_with_as(id: &str, branch: &str, next: Option<&str>, as_name: &str) -> Node {
|
||||||
|
let mut map = map_node_basic(id, branch, next);
|
||||||
|
if let NodeType::Map(ref mut mm) = map.node_type {
|
||||||
|
mm.as_name = as_name.into();
|
||||||
|
}
|
||||||
|
map
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn map_as_equal_output_key_errors() {
|
||||||
|
let map = map_with_as("m", "br", Some("end"), "output");
|
||||||
|
let branch = llm_with_state_updates("br", &[("output", "{{output}}")], None);
|
||||||
|
let graph = graph_with(
|
||||||
|
vec![("m", map), ("br", branch), ("end", end_node("end"))],
|
||||||
|
"m",
|
||||||
|
);
|
||||||
|
|
||||||
|
let result = validator().validate(&graph);
|
||||||
|
|
||||||
|
assert!(
|
||||||
|
result.errors.iter().any(|e| e.message
|
||||||
|
== "map node 'm': `as` and `output_key` are both 'output'; the item binding \
|
||||||
|
would be collected as the chain's result"
|
||||||
|
&& e.node_id.as_deref() == Some("m")),
|
||||||
|
"expected as/output_key collision error: {:?}",
|
||||||
|
result.errors
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn map_as_distinct_from_output_key_passes() {
|
||||||
|
let map = map_with_as("m", "br", Some("end"), "item");
|
||||||
|
let branch = llm_with_state_updates("br", &[("output", "{{output}}")], None);
|
||||||
|
let graph = graph_with(
|
||||||
|
vec![("m", map), ("br", branch), ("end", end_node("end"))],
|
||||||
|
"m",
|
||||||
|
);
|
||||||
|
|
||||||
|
let result = validator().validate(&graph);
|
||||||
|
|
||||||
|
assert!(
|
||||||
|
!result
|
||||||
|
.errors
|
||||||
|
.iter()
|
||||||
|
.any(|e| e.message.contains("`as` and `output_key`")),
|
||||||
|
"distinct as/output_key must not error: {:?}",
|
||||||
|
result.errors
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn map_max_concurrency_non_template_string_errors() {
|
fn map_max_concurrency_non_template_string_errors() {
|
||||||
let map = map_with_cap("m", "br", Some("end"), "4");
|
let map = map_with_cap("m", "br", Some("end"), "4");
|
||||||
|
|||||||
Reference in new issue
Block a user