From 727cab11ffbb2e87cbff18132a37f0847e8aca22 Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Thu, 10 Sep 2026 17:54:36 -0600 Subject: [PATCH] fix(graph): validate the whole max_concurrency template string A string max_concurrency passed the load-time check whenever it merely contained a {{...}} pair, so forms like "n={{k}}" or "{{ key }}" that can never resolve at runtime were only caught after the map's inputs had been computed. The check now requires the trimmed string to be exactly one template reference, matching what interpolate_raw resolves as a pure reference; padded forms like " {{budget}} " stay valid. Also: the bash tool build-failure context names the source file instead of the destination path, and the map-subgraph validator no longer shadows its member id list with the resolved member nodes. --- src/graph/state.rs | 42 +++++++++++++++++++++++++++ src/graph/validator.rs | 66 ++++++++++++++++++++++++++++++++++-------- src/parsers/bash.rs | 2 +- 3 files changed, 97 insertions(+), 13 deletions(-) diff --git a/src/graph/state.rs b/src/graph/state.rs index e16f16f..824a8fe 100644 --- a/src/graph/state.rs +++ b/src/graph/state.rs @@ -320,6 +320,16 @@ pub(super) fn template_root_keys(template: &str) -> Vec { .collect() } +// Whether `s` is exactly one `{{key}}` reference and nothing else, after the +// same trim `interpolate_raw` applies, so this accepts exactly the strings the +// runtime resolves as a pure reference. +pub(super) fn is_lone_template(s: &str) -> bool { + let s = s.trim(); + TEMPLATE_VAR_RE + .find(s) + .is_ok_and(|m| m.is_some_and(|m| m.start() == 0 && m.end() == s.len())) +} + fn value_to_string(value: &Value) -> String { match value { Value::String(s) => s.clone(), @@ -931,6 +941,38 @@ mod tests { assert_eq!(result, json!("v")); } + #[test] + fn is_lone_template_cases() { + for accepted in [ + "{{budget}}", + "{{cfg.limits[0]}}", + " {{budget}} ", + "\n{{k}}\t", + ] { + assert!( + is_lone_template(accepted), + "{accepted:?} should be a lone template" + ); + } + for rejected in [ + "", + "4", + "{{}}", + "{{ key }}", + "n={{k}}", + "{{k}}x", + "{{a}} {{b", + "{{a}}{{b}}", + "{{a-b}}", + "{{k", + ] { + assert!( + !is_lone_template(rejected), + "{rejected:?} must not be a lone template" + ); + } + } + #[test] fn interpolate_raw_inner_spaces_treated_as_mixed() { let manager = manager_with(&[("k", json!("v"))]); diff --git a/src/graph/validator.rs b/src/graph/validator.rs index e325f86..c6e5097 100644 --- a/src/graph/validator.rs +++ b/src/graph/validator.rs @@ -1,4 +1,4 @@ -use super::state::template_root_keys; +use super::state::{is_lone_template, template_root_keys}; use super::types::{ConcurrencyCap, Graph, NextTargets, Node, NodeType}; use crate::client::{Model, ModelType}; use crate::config; @@ -557,12 +557,12 @@ impl GraphValidator { for (node_id, node) in &graph.nodes { if let NodeType::Map(m) = &node.node_type && let Some(ConcurrencyCap::Template(t)) = &m.max_concurrency - && !contains_template(t) + && !is_lone_template(t) { result.error(ValidationError::with_node( node_id, "map node's `max_concurrency` is a string but not a template; write \ - an integer or a `{{key}}` template", + an integer or exactly one `{{key}}` with nothing around it", )); } } @@ -654,12 +654,13 @@ impl GraphValidator { )); } // A missing entry is reported by `validate_node_references`. - let mut members: Vec = branch_subgraph(graph, &m.branch).into_iter().collect(); - if members.is_empty() { + let mut member_ids: Vec = + branch_subgraph(graph, &m.branch).into_iter().collect(); + if member_ids.is_empty() { continue; } - members.sort(); - let members: Vec<(&str, &Node)> = members + member_ids.sort(); + let members: Vec<(&str, &Node)> = member_ids .iter() .filter_map(|id| graph.get_node(id).map(|n| (id.as_str(), n))) .collect(); @@ -1156,11 +1157,6 @@ fn primary_templated_fields(node: &Node) -> Vec { } } -fn contains_template(s: &str) -> bool { - s.find("{{") - .is_some_and(|open| s[open + 2..].contains("}}")) -} - fn node_state_updates_map(node: &Node) -> Option<&std::collections::HashMap> { match &node.node_type { NodeType::Llm(n) => n.state_updates.as_ref(), @@ -3628,6 +3624,52 @@ mod tests { ); } + #[test] + fn map_max_concurrency_malformed_templates_error() { + for cap in ["{{}}", "{{ key }}", "n={{k}}", "{{a}} {{b", "{{a-b}}"] { + let map = map_with_cap("m", "br", Some("end"), cap); + 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's `max_concurrency` is a string but not a template; write \ + an integer or exactly one `{{key}}` with nothing around it" + && e.node_id.as_deref() == Some("m")), + "cap {cap:?} should fail the whole-string template check: {:?}", + result.errors + ); + } + } + + #[test] + fn map_max_concurrency_lone_template_forms_pass() { + for cap in ["{{budget}}", "{{cfg.limits[0]}}", " {{budget}} "] { + let map = map_with_cap("m", "br", Some("end"), cap); + 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("is a string but not a template")), + "cap {cap:?} is a lone template and must pass: {:?}", + result.errors + ); + } + } + #[test] fn map_max_concurrency_template_reading_sibling_write_errors() { let mut start = end_node("start"); diff --git a/src/parsers/bash.rs b/src/parsers/bash.rs index 019eaec..040a156 100644 --- a/src/parsers/bash.rs +++ b/src/parsers/bash.rs @@ -19,7 +19,7 @@ pub fn generate_bash_declarations( debug!("Building script at '{tool_file:?}'"); let (build_script, declarations) = build_bash_tool(&src, file_name) - .with_context(|| format!("Failed to build script at '{tools_file_path:?}'"))?; + .with_context(|| format!("Failed to build script '{file_name}'"))?; function::write_file_atomic(tools_file_path, &build_script, Some(0o755)) .with_context(|| format!("Failed to write built script to '{tools_file_path:?}'"))?;