feat(graph): warn when a declared variable is shadowed by an initial_state key
This commit is contained in:
2 files changed
+113
-2
No files matched your search
@@ -8184,6 +8184,22 @@ mod tests {
|
|||||||
.collect()
|
.collect()
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn shadowed(names: &[&str]) -> Vec<String> {
|
||||||
|
names
|
||||||
|
.iter()
|
||||||
|
.map(|name| {
|
||||||
|
format!(
|
||||||
|
"-: declared variable '{name}' is shadowed by an explicit \
|
||||||
|
`initial_state` key: the `initial_state` value wins and the \
|
||||||
|
variable's resolved value (spawn-provided/CLI/default) is never \
|
||||||
|
seeded into graph state; rename one of them, or drop the \
|
||||||
|
`initial_state` key to let the variable seed it (script nodes' \
|
||||||
|
`LLM_AGENT_VAR_<NAME>` env is unaffected)"
|
||||||
|
)
|
||||||
|
})
|
||||||
|
.collect()
|
||||||
|
}
|
||||||
|
|
||||||
let _guard = TestConfigDirGuard::new();
|
let _guard = TestConfigDirGuard::new();
|
||||||
|
|
||||||
Agent::install_builtin_agents(false).unwrap();
|
Agent::install_builtin_agents(false).unwrap();
|
||||||
@@ -8218,8 +8234,9 @@ mod tests {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
// Warning parity with the pre-subgraph validator: the only warnings any
|
// Warning baseline for the shipped graphs: `_next`-routed unreachable
|
||||||
// bundled graph emits are the `_next`-routed unreachable nodes. A new
|
// nodes, plus variable-shadowing on `coder` and `step-runner` (they
|
||||||
|
// declare variables whose names are also `initial_state` keys). A new
|
||||||
// rule that fires on a shipped asset (or a new bundled graph) must be
|
// rule that fires on a shipped asset (or a new bundled graph) must be
|
||||||
// recorded here deliberately.
|
// recorded here deliberately.
|
||||||
let expected_warnings = BTreeMap::from([
|
let expected_warnings = BTreeMap::from([
|
||||||
@@ -8227,6 +8244,8 @@ mod tests {
|
|||||||
("code-reviewer".to_string(), Vec::new()),
|
("code-reviewer".to_string(), Vec::new()),
|
||||||
(
|
(
|
||||||
"coder".to_string(),
|
"coder".to_string(),
|
||||||
|
[
|
||||||
|
shadowed(&["project_dir"]),
|
||||||
unreachable(&[
|
unreachable(&[
|
||||||
"analyze_request",
|
"analyze_request",
|
||||||
"end_rejected",
|
"end_rejected",
|
||||||
@@ -8240,6 +8259,8 @@ mod tests {
|
|||||||
"verify_build",
|
"verify_build",
|
||||||
"verify_tests",
|
"verify_tests",
|
||||||
]),
|
]),
|
||||||
|
]
|
||||||
|
.concat(),
|
||||||
),
|
),
|
||||||
("deep-research".to_string(), unreachable(&["ask_topic"])),
|
("deep-research".to_string(), unreachable(&["ask_topic"])),
|
||||||
("finding-verifier".to_string(), Vec::new()),
|
("finding-verifier".to_string(), Vec::new()),
|
||||||
@@ -8247,6 +8268,8 @@ mod tests {
|
|||||||
("review-gauntlet".to_string(), Vec::new()),
|
("review-gauntlet".to_string(), Vec::new()),
|
||||||
(
|
(
|
||||||
"step-runner".to_string(),
|
"step-runner".to_string(),
|
||||||
|
[
|
||||||
|
shadowed(&["plans_dir", "project_dir"]),
|
||||||
unreachable(&[
|
unreachable(&[
|
||||||
"check_handoff",
|
"check_handoff",
|
||||||
"edge_case_sweep",
|
"edge_case_sweep",
|
||||||
@@ -8267,6 +8290,8 @@ mod tests {
|
|||||||
"verify_tests",
|
"verify_tests",
|
||||||
"write_handoff",
|
"write_handoff",
|
||||||
]),
|
]),
|
||||||
|
]
|
||||||
|
.concat(),
|
||||||
),
|
),
|
||||||
]);
|
]);
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
|
|||||||
@@ -180,6 +180,7 @@ impl GraphValidator {
|
|||||||
self.validate_max_concurrency_template(graph, &mut result);
|
self.validate_max_concurrency_template(graph, &mut result);
|
||||||
self.validate_orchestration_limits(graph, &mut result);
|
self.validate_orchestration_limits(graph, &mut result);
|
||||||
self.validate_timeouts(graph, &mut result);
|
self.validate_timeouts(graph, &mut result);
|
||||||
|
self.validate_variable_shadowing(graph, &mut result);
|
||||||
self.validate_map_subgraphs(graph, &mut result);
|
self.validate_map_subgraphs(graph, &mut result);
|
||||||
self.validate_parallel_user_interaction(graph, &mut result);
|
self.validate_parallel_user_interaction(graph, &mut result);
|
||||||
self.validate_parallel_writes(graph, &mut result);
|
self.validate_parallel_writes(graph, &mut result);
|
||||||
@@ -613,6 +614,25 @@ impl GraphValidator {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Variable seeding never overwrites an existing state key, so a declared
|
||||||
|
/// variable whose name is also an `initial_state` key never reaches graph
|
||||||
|
/// state: the explicit `initial_state` value wins. Legal; the warning only
|
||||||
|
/// makes the shadowing visible.
|
||||||
|
fn validate_variable_shadowing(&self, graph: &Graph, result: &mut ValidationResult) {
|
||||||
|
for var in &graph.variables {
|
||||||
|
if graph.initial_state.contains_key(&var.name) {
|
||||||
|
result.warning(ValidationError::new(format!(
|
||||||
|
"declared variable '{}' is shadowed by an explicit `initial_state` key: \
|
||||||
|
the `initial_state` value wins and the variable's resolved value \
|
||||||
|
(spawn-provided/CLI/default) is never seeded into graph state; rename \
|
||||||
|
one of them, or drop the `initial_state` key to let the variable seed \
|
||||||
|
it (script nodes' `LLM_AGENT_VAR_<NAME>` env is unaffected)",
|
||||||
|
var.name
|
||||||
|
)));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// Explicit `0` opt-outs on `settings` and node bounds: a `timeout: 0`
|
/// Explicit `0` opt-outs on `settings` and node bounds: a `timeout: 0`
|
||||||
/// disables the wall-clock bound, `max_loop_iterations: 0` disables the
|
/// disables the wall-clock bound, `max_loop_iterations: 0` disables the
|
||||||
/// per-node visit cap, and an llm node's `max_iterations: 0` disables its
|
/// per-node visit cap, and an llm node's `max_iterations: 0` disables its
|
||||||
@@ -3272,6 +3292,72 @@ mod tests {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn variable(name: &str) -> crate::config::AgentVariable {
|
||||||
|
crate::config::AgentVariable {
|
||||||
|
name: name.into(),
|
||||||
|
..Default::default()
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
fn shadowing_warnings(result: &ValidationResult) -> Vec<&ValidationError> {
|
||||||
|
result
|
||||||
|
.warnings
|
||||||
|
.iter()
|
||||||
|
.filter(|w| w.message.contains("shadowed"))
|
||||||
|
.collect()
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn variable_shadowed_by_initial_state_key_warns() {
|
||||||
|
let mut graph = graph_with(vec![("e", end_node("e"))], "e");
|
||||||
|
graph.variables = vec![variable("region")];
|
||||||
|
graph
|
||||||
|
.initial_state
|
||||||
|
.insert("region".into(), serde_json::json!("us-east-1"));
|
||||||
|
|
||||||
|
let result = validator().validate(&graph);
|
||||||
|
|
||||||
|
assert!(result.is_valid());
|
||||||
|
let w = shadowing_warnings(&result);
|
||||||
|
assert_eq!(w.len(), 1, "{:?}", result.warnings);
|
||||||
|
assert_eq!(w[0].node_id, None);
|
||||||
|
assert!(w[0].message.contains("'region'"), "{}", w[0].message);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn disjoint_variables_and_initial_state_do_not_warn() {
|
||||||
|
let mut graph = graph_with(vec![("e", end_node("e"))], "e");
|
||||||
|
graph.variables = vec![variable("region")];
|
||||||
|
graph
|
||||||
|
.initial_state
|
||||||
|
.insert("zone".into(), serde_json::json!("a"));
|
||||||
|
|
||||||
|
let result = validator().validate(&graph);
|
||||||
|
|
||||||
|
assert!(
|
||||||
|
shadowing_warnings(&result).is_empty(),
|
||||||
|
"disjoint names should not warn: {:?}",
|
||||||
|
result.warnings
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn each_shadowed_variable_warns_once() {
|
||||||
|
let mut graph = graph_with(vec![("e", end_node("e"))], "e");
|
||||||
|
graph.variables = vec![variable("region"), variable("tier")];
|
||||||
|
graph
|
||||||
|
.initial_state
|
||||||
|
.insert("region".into(), serde_json::json!("us"));
|
||||||
|
graph
|
||||||
|
.initial_state
|
||||||
|
.insert("tier".into(), serde_json::json!("gold"));
|
||||||
|
|
||||||
|
let result = validator().validate(&graph);
|
||||||
|
|
||||||
|
let w = shadowing_warnings(&result);
|
||||||
|
assert_eq!(w.len(), 2, "{:?}", result.warnings);
|
||||||
|
}
|
||||||
|
|
||||||
fn timeout_warnings(result: &ValidationResult) -> Vec<&ValidationError> {
|
fn timeout_warnings(result: &ValidationResult) -> Vec<&ValidationError> {
|
||||||
result
|
result
|
||||||
.warnings
|
.warnings
|
||||||
|
|||||||
Reference in new issue
Block a user