diff --git a/src/config/request_context.rs b/src/config/request_context.rs index 3b6f5d9..ef04078 100644 --- a/src/config/request_context.rs +++ b/src/config/request_context.rs @@ -8184,6 +8184,22 @@ mod tests { .collect() } + fn shadowed(names: &[&str]) -> Vec { + 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_` env is unaffected)" + ) + }) + .collect() + } + let _guard = TestConfigDirGuard::new(); Agent::install_builtin_agents(false).unwrap(); @@ -8218,8 +8234,9 @@ mod tests { ); } - // Warning parity with the pre-subgraph validator: the only warnings any - // bundled graph emits are the `_next`-routed unreachable nodes. A new + // Warning baseline for the shipped graphs: `_next`-routed unreachable + // 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 // recorded here deliberately. let expected_warnings = BTreeMap::from([ @@ -8227,19 +8244,23 @@ mod tests { ("code-reviewer".to_string(), Vec::new()), ( "coder".to_string(), - unreachable(&[ - "analyze_request", - "end_rejected", - "end_success", - "fix_loop_gate", - "gate_approval", - "implement", - "route_complexity", - "route_review_result", - "self_review", - "verify_build", - "verify_tests", - ]), + [ + shadowed(&["project_dir"]), + unreachable(&[ + "analyze_request", + "end_rejected", + "end_success", + "fix_loop_gate", + "gate_approval", + "implement", + "route_complexity", + "route_review_result", + "self_review", + "verify_build", + "verify_tests", + ]), + ] + .concat(), ), ("deep-research".to_string(), unreachable(&["ask_topic"])), ("finding-verifier".to_string(), Vec::new()), @@ -8247,26 +8268,30 @@ mod tests { ("review-gauntlet".to_string(), Vec::new()), ( "step-runner".to_string(), - unreachable(&[ - "check_handoff", - "edge_case_sweep", - "end_blocked", - "end_rejected", - "end_success", - "fix_loop_gate", - "gate_blocked", - "gate_deviation", - "gate_user_review", - "get_revision", - "independent_review", - "revise_from_choice", - "route_review", - "route_sweep", - "verify_build", - "verify_format_lint", - "verify_tests", - "write_handoff", - ]), + [ + shadowed(&["plans_dir", "project_dir"]), + unreachable(&[ + "check_handoff", + "edge_case_sweep", + "end_blocked", + "end_rejected", + "end_success", + "fix_loop_gate", + "gate_blocked", + "gate_deviation", + "gate_user_review", + "get_revision", + "independent_review", + "revise_from_choice", + "route_review", + "route_sweep", + "verify_build", + "verify_format_lint", + "verify_tests", + "write_handoff", + ]), + ] + .concat(), ), ]); assert_eq!( diff --git a/src/graph/validator.rs b/src/graph/validator.rs index cf61276..08188fe 100644 --- a/src/graph/validator.rs +++ b/src/graph/validator.rs @@ -180,6 +180,7 @@ impl GraphValidator { self.validate_max_concurrency_template(graph, &mut result); self.validate_orchestration_limits(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_parallel_user_interaction(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_` env is unaffected)", + var.name + ))); + } + } + } + /// Explicit `0` opt-outs on `settings` and node bounds: a `timeout: 0` /// 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 @@ -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> { result .warnings