fix(graph): make turn-cap and template checks consistent across validation modes
This commit is contained in:
4 files changed
+98
-9
No files matched your search
+7
-1
@@ -262,7 +262,7 @@ async fn run_with_retries(
|
|||||||
|
|
||||||
/// Whether `turn` (0-based) is the final turn the node's cap allows. A cap of 0 means no cap.
|
/// Whether `turn` (0-based) is the final turn the node's cap allows. A cap of 0 means no cap.
|
||||||
pub(crate) fn is_last_turn(turn: u32, max_iterations: u32) -> bool {
|
pub(crate) fn is_last_turn(turn: u32, max_iterations: u32) -> bool {
|
||||||
max_iterations > 0 && turn + 1 == max_iterations
|
max_iterations > 0 && turn == max_iterations - 1
|
||||||
}
|
}
|
||||||
|
|
||||||
async fn run_chat_loop(
|
async fn run_chat_loop(
|
||||||
@@ -768,6 +768,12 @@ mod tests {
|
|||||||
assert!(!is_last_turn(u32::MAX, 0));
|
assert!(!is_last_turn(u32::MAX, 0));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn is_last_turn_at_u32_max_does_not_overflow() {
|
||||||
|
assert!(is_last_turn(u32::MAX - 1, u32::MAX));
|
||||||
|
assert!(!is_last_turn(u32::MAX, u32::MAX));
|
||||||
|
}
|
||||||
|
|
||||||
/// The turn-top abort check runs before any client is created, so an
|
/// The turn-top abort check runs before any client is created, so an
|
||||||
/// already-aborted graph never issues a model call and the node fails
|
/// already-aborted graph never issues a model call and the node fails
|
||||||
/// with the abort reason rather than a connection error.
|
/// with the abort reason rather than a connection error.
|
||||||
|
|||||||
+32
-2
@@ -1,5 +1,5 @@
|
|||||||
use super::executor::{PeerRetireGuard, StepContext, StepResult, TaskCancelGuard, step};
|
use super::executor::{PeerRetireGuard, StepContext, StepResult, TaskCancelGuard, step};
|
||||||
use super::state::StateManager;
|
use super::state::{StateManager, is_lone_template};
|
||||||
use super::types::{ConcurrencyCap, Graph, MapNode, Node, NodeType};
|
use super::types::{ConcurrencyCap, Graph, MapNode, Node, NodeType};
|
||||||
use super::validator::branch_subgraph;
|
use super::validator::branch_subgraph;
|
||||||
use crate::config::{RenderMode, RequestContext};
|
use crate::config::{RenderMode, RequestContext};
|
||||||
@@ -324,7 +324,8 @@ fn wants_peer_identity(node: &Node) -> bool {
|
|||||||
/// accepted; anything else is the author's bug and surfaces as an error
|
/// accepted; anything else is the author's bug and surfaces as an error
|
||||||
/// rather than being clamped. A literal 0 is rejected the same way, so a
|
/// rather than being clamped. A literal 0 is rejected the same way, so a
|
||||||
/// graph loaded with validation off fails here instead of silently running
|
/// graph loaded with validation off fails here instead of silently running
|
||||||
/// one item at a time. Only the absent-cap default is clamped.
|
/// one item at a time, and a string that is not exactly one `{{key}}` is
|
||||||
|
/// rejected before interpolation for the same reason. Only the absent-cap default is clamped.
|
||||||
fn resolve_max_concurrency(
|
fn resolve_max_concurrency(
|
||||||
node: &MapNode,
|
node: &MapNode,
|
||||||
state: &StateManager,
|
state: &StateManager,
|
||||||
@@ -340,6 +341,13 @@ fn resolve_max_concurrency(
|
|||||||
Some(ConcurrencyCap::Template(t)) => t,
|
Some(ConcurrencyCap::Template(t)) => t,
|
||||||
};
|
};
|
||||||
|
|
||||||
|
if !is_lone_template(template) {
|
||||||
|
bail!(
|
||||||
|
"map node '{node_id}': max_concurrency \"{template}\" is a string but not a template; \
|
||||||
|
write an integer or exactly one `{{{{key}}}}` with nothing around it"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
let value = state
|
let value = state
|
||||||
.interpolate_raw(template)
|
.interpolate_raw(template)
|
||||||
.with_context(|| format!("map node '{node_id}': evaluating `max_concurrency` template"))?;
|
.with_context(|| format!("map node '{node_id}': evaluating `max_concurrency` template"))?;
|
||||||
@@ -662,6 +670,28 @@ mod tests {
|
|||||||
);
|
);
|
||||||
assert!(chain.contains("budget"), "{chain}");
|
assert!(chain.contains("budget"), "{chain}");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn resolve_max_concurrency_rejects_non_template_string() {
|
||||||
|
let state = state_with("budget", Value::from(3));
|
||||||
|
let cap = Some(ConcurrencyCap::Template("4".into()));
|
||||||
|
let msg = resolve(cap, &state).unwrap_err().to_string();
|
||||||
|
assert!(
|
||||||
|
msg.contains(
|
||||||
|
"is a string but not a template; write an integer or exactly one `{{key}}` \
|
||||||
|
with nothing around it"
|
||||||
|
),
|
||||||
|
"{msg}"
|
||||||
|
);
|
||||||
|
assert!(msg.contains("map node 'm': max_concurrency \"4\""), "{msg}");
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn resolve_max_concurrency_accepts_padded_template() {
|
||||||
|
let state = state_with("budget", Value::from(3));
|
||||||
|
let cap = Some(ConcurrencyCap::Template(" {{budget}} ".into()));
|
||||||
|
assert_eq!(resolve(cap, &state).unwrap(), 3);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
|
|||||||
+38
-6
@@ -320,14 +320,24 @@ pub(super) fn template_root_keys(template: &str) -> Vec<String> {
|
|||||||
.collect()
|
.collect()
|
||||||
}
|
}
|
||||||
|
|
||||||
// Whether `s` is exactly one `{{key}}` reference and nothing else, after the
|
// Whether `s` is exactly one `{{key}}` reference and nothing else (after the
|
||||||
// same trim `interpolate_raw` applies, so this accepts exactly the strings the
|
// same trim `interpolate_raw` applies) whose path parses with the grammar
|
||||||
// runtime resolves as a pure reference.
|
// `get_nested_value` uses, so this accepts exactly the strings the runtime
|
||||||
|
// can resolve as a pure reference.
|
||||||
pub(super) fn is_lone_template(s: &str) -> bool {
|
pub(super) fn is_lone_template(s: &str) -> bool {
|
||||||
let s = s.trim();
|
let s = s.trim();
|
||||||
TEMPLATE_VAR_RE
|
let Ok(Some(m)) = TEMPLATE_VAR_RE.captures(s) else {
|
||||||
.find(s)
|
return false;
|
||||||
.is_ok_and(|m| m.is_some_and(|m| m.start() == 0 && m.end() == s.len()))
|
};
|
||||||
|
let whole = m.get(0).expect("group 0 always present");
|
||||||
|
if whole.start() != 0 || whole.end() != s.len() {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
let path = m.get(1).expect("template regex has one group").as_str();
|
||||||
|
let mut parts = path.split('.');
|
||||||
|
let first = parts.next().and_then(split_indices);
|
||||||
|
first.is_some_and(|(name, _)| !name.is_empty())
|
||||||
|
&& parts.all(|part| split_indices(part).is_some())
|
||||||
}
|
}
|
||||||
|
|
||||||
fn value_to_string(value: &Value) -> String {
|
fn value_to_string(value: &Value) -> String {
|
||||||
@@ -973,6 +983,28 @@ mod tests {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn is_lone_template_matches_runtime_path_grammar() {
|
||||||
|
for accepted in [
|
||||||
|
"{{a.b[0][1].c}}",
|
||||||
|
"{{a[0][1]}}",
|
||||||
|
"{{a.}}",
|
||||||
|
"{{a..b}}",
|
||||||
|
"{{a.[0]}}",
|
||||||
|
] {
|
||||||
|
assert!(
|
||||||
|
is_lone_template(accepted),
|
||||||
|
"{accepted:?} resolves at run time and must be accepted"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
for rejected in ["{{limits[foo]}}", "{{a[]}}", "{{a[0}}", "{{.a}}"] {
|
||||||
|
assert!(
|
||||||
|
!is_lone_template(rejected),
|
||||||
|
"{rejected:?} never resolves at run time and must be rejected"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn interpolate_raw_inner_spaces_treated_as_mixed() {
|
fn interpolate_raw_inner_spaces_treated_as_mixed() {
|
||||||
let manager = manager_with(&[("k", json!("v"))]);
|
let manager = manager_with(&[("k", json!("v"))]);
|
||||||
|
|||||||
@@ -3647,6 +3647,27 @@ mod tests {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn map_max_concurrency_non_numeric_index_errors() {
|
||||||
|
let map = map_with_cap("m", "br", Some("end"), "{{limits[foo]}}");
|
||||||
|
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")),
|
||||||
|
"a non-numeric index can never resolve and must fail at load: {:?}",
|
||||||
|
result.errors
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn map_max_concurrency_lone_template_forms_pass() {
|
fn map_max_concurrency_lone_template_forms_pass() {
|
||||||
for cap in ["{{budget}}", "{{cfg.limits[0]}}", " {{budget}} "] {
|
for cap in ["{{budget}}", "{{cfg.limits[0]}}", " {{budget}} "] {
|
||||||
|
|||||||
Reference in new issue
Block a user