From 3dfa0f90448aeb49c7c5bfce8220e38bffdece2b Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 23 Feb 2026 17:43:05 -0500 Subject: [PATCH] Add shape selectors, LLM stream timeouts, MultiSelect questions, and test coverage - Stylesheet: add bare-word Shape selector (specificity between Universal and Class) - LLM: apply per_step timeout to connection and total timeout to stream (Section 4.7) - Interviewer: add MultiSelect question type alongside MultipleChoice - Session: move SessionStart/SessionEnd to initialize()/close(), deduplicate close logic - Engine: return Ok(fail outcome) instead of error when goal gate unsatisfied with no retry_target - Docker: mark Docker-dependent tests with #[ignore] - Validation: add extensive unit test coverage for all rule types - Integration: update tests to match engine/fidelity changes Co-Authored-By: Claude Opus 4.6 --- crates/agent/src/docker_env.rs | 5 + crates/agent/src/session.rs | 40 +- crates/attractor/src/engine.rs | 2 +- .../attractor/src/interviewer/auto_approve.rs | 4 +- crates/attractor/src/interviewer/console.rs | 4 +- crates/attractor/src/interviewer/mod.rs | 9 +- crates/attractor/src/stylesheet.rs | 97 +- crates/attractor/src/validation/rules.rs | 929 ++++++++++++++++++ crates/attractor/tests/integration.rs | 90 +- crates/llm/src/generate.rs | 52 +- 10 files changed, 1133 insertions(+), 99 deletions(-) diff --git a/crates/agent/src/docker_env.rs b/crates/agent/src/docker_env.rs index b2a70d2bb..593928ae7 100644 --- a/crates/agent/src/docker_env.rs +++ b/crates/agent/src/docker_env.rs @@ -661,6 +661,7 @@ mod tests { } #[tokio::test] + #[ignore] async fn full_lifecycle() { let _docker = require_docker(); let host_dir = std::env::temp_dir().join(format!("docker_env_test_{}", uuid::Uuid::new_v4())); @@ -717,6 +718,7 @@ mod tests { } #[tokio::test] + #[ignore] async fn timeout_handling() { let _docker = require_docker(); let host_dir = std::env::temp_dir().join(format!("docker_timeout_test_{}", uuid::Uuid::new_v4())); @@ -735,6 +737,7 @@ mod tests { } #[tokio::test] + #[ignore] async fn special_characters_in_write() { let _docker = require_docker(); let host_dir = std::env::temp_dir().join(format!("docker_special_test_{}", uuid::Uuid::new_v4())); @@ -756,6 +759,7 @@ mod tests { } #[tokio::test] + #[ignore] async fn path_resolution() { let _docker = require_docker(); let host_dir = std::env::temp_dir().join(format!("docker_path_test_{}", uuid::Uuid::new_v4())); @@ -779,6 +783,7 @@ mod tests { } #[tokio::test] + #[ignore] async fn cleanup_idempotent() { let _docker = require_docker(); let host_dir = std::env::temp_dir().join(format!("docker_cleanup_test_{}", uuid::Uuid::new_v4())); diff --git a/crates/agent/src/session.rs b/crates/agent/src/session.rs index 53d93f835..167d3aa6f 100644 --- a/crates/agent/src/session.rs +++ b/crates/agent/src/session.rs @@ -64,6 +64,9 @@ impl Session { /// Initialize session by discovering project docs and capturing environment context. /// Call before `process_input`. pub async fn initialize(&mut self) { + self.event_emitter + .emit(EventKind::SessionStart, self.id.clone(), EventData::Empty); + let doc_root = self .config .git_root @@ -166,7 +169,11 @@ impl Session { } pub fn close(&mut self) { - self.state = SessionState::Closed; + if self.state != SessionState::Closed { + self.state = SessionState::Closed; + self.event_emitter + .emit(EventKind::SessionEnd, self.id.clone(), EventData::Empty); + } } pub fn set_reasoning_effort(&mut self, effort: Option) { @@ -186,9 +193,6 @@ impl Session { return Err(AgentError::SessionClosed); } - self.event_emitter - .emit(EventKind::SessionStart, self.id.clone(), EventData::Empty); - // Process the initial input, then drain any followups self.run_single_input(input).await?; loop { @@ -202,8 +206,6 @@ impl Session { } self.state = SessionState::Idle; - self.event_emitter - .emit(EventKind::SessionEnd, self.id.clone(), EventData::Empty); Ok(()) } @@ -253,9 +255,7 @@ impl Session { // Check abort flag if self.abort_flag.load(Ordering::SeqCst) { - self.state = SessionState::Closed; - self.event_emitter - .emit(EventKind::SessionEnd, self.id.clone(), EventData::Empty); + self.close(); return Err(AgentError::Aborted); } @@ -322,12 +322,10 @@ impl Session { } // If aborted during streaming, drop the stream to cancel the HTTP - // connection, then emit SessionEnd before returning. + // connection, then close the session before returning. if self.abort_flag.load(Ordering::SeqCst) { drop(event_stream); - self.state = SessionState::Closed; - self.event_emitter - .emit(EventKind::SessionEnd, self.id.clone(), EventData::Empty); + self.close(); return Err(AgentError::Aborted); } @@ -923,7 +921,9 @@ mod tests { let mut session = make_session(vec![text_response("Hello")]).await; let mut rx = session.subscribe(); + session.initialize().await; session.process_input("Hi").await.unwrap(); + session.close(); // Collect events let mut events = Vec::new(); @@ -931,6 +931,7 @@ mod tests { events.push(event.kind.clone()); } + assert!(events.contains(&EventKind::SessionStart)); assert!(events.contains(&EventKind::UserInput)); assert!(events.contains(&EventKind::AssistantTextEnd)); assert!(events.contains(&EventKind::SessionEnd)); @@ -1446,19 +1447,24 @@ mod tests { let mut session = make_session(responses).await; let mut rx = session.subscribe(); + session.initialize().await; session.process_input("one").await.unwrap(); session.process_input("two").await.unwrap(); + session.close(); let mut session_start_count = 0; + let mut session_end_count = 0; while let Ok(event) = rx.try_recv() { if event.kind == EventKind::SessionStart { session_start_count += 1; } + if event.kind == EventKind::SessionEnd { + session_end_count += 1; + } } - // Each process_input emits SESSION_START currently -- this should be 1 per input call - // The spec says SESSION_START is "session created", but since our Session doesn't - // emit at creation, we accept one per process_input call as the session boundary. - assert_eq!(session_start_count, 2); + // SESSION_START is emitted once during initialize(), SESSION_END once during close() + assert_eq!(session_start_count, 1); + assert_eq!(session_end_count, 1); } #[tokio::test] diff --git a/crates/attractor/src/engine.rs b/crates/attractor/src/engine.rs index e456396de..d9123752c 100644 --- a/crates/attractor/src/engine.rs +++ b/crates/attractor/src/engine.rs @@ -760,7 +760,7 @@ impl PipelineEngine { error: error_msg.clone(), duration_ms, }); - return Err(AttractorError::Engine(error_msg)); + return Ok(Outcome::fail(error_msg)); } } } diff --git a/crates/attractor/src/interviewer/auto_approve.rs b/crates/attractor/src/interviewer/auto_approve.rs index f26d06c26..fa1a3ecdb 100644 --- a/crates/attractor/src/interviewer/auto_approve.rs +++ b/crates/attractor/src/interviewer/auto_approve.rs @@ -10,7 +10,7 @@ impl Interviewer for AutoApproveInterviewer { async fn ask(&self, question: Question) -> Answer { match question.question_type { QuestionType::YesNo | QuestionType::Confirmation => Answer::yes(), - QuestionType::MultipleChoice => question.options.first().map_or_else( + QuestionType::MultipleChoice | QuestionType::MultiSelect => question.options.first().map_or_else( || Answer::text("auto-approved"), |first| Answer { value: AnswerValue::Selected(first.key.clone()), @@ -85,4 +85,4 @@ mod tests { assert_eq!(answer.value, AnswerValue::Text("auto-approved".to_string())); assert_eq!(answer.text, Some("auto-approved".to_string())); } -} +} \ No newline at end of file diff --git a/crates/attractor/src/interviewer/console.rs b/crates/attractor/src/interviewer/console.rs index 766eec994..92be6e8bc 100644 --- a/crates/attractor/src/interviewer/console.rs +++ b/crates/attractor/src/interviewer/console.rs @@ -66,7 +66,7 @@ impl Interviewer for ConsoleInterviewer { ); match question.question_type { - QuestionType::MultipleChoice => { + QuestionType::MultipleChoice | QuestionType::MultiSelect => { for (i, opt) in question.options.iter().enumerate() { eprintln!( " {dim}[{reset}{bold}{}{reset}{dim}]{reset} {} - {}", @@ -182,4 +182,4 @@ mod tests { let result = find_matching_option("5", &options); assert!(result.is_none()); } -} +} \ No newline at end of file diff --git a/crates/attractor/src/interviewer/mod.rs b/crates/attractor/src/interviewer/mod.rs index 325afdbd1..0c3f84d40 100644 --- a/crates/attractor/src/interviewer/mod.rs +++ b/crates/attractor/src/interviewer/mod.rs @@ -16,6 +16,7 @@ use serde::{Deserialize, Serialize}; pub enum QuestionType { YesNo, MultipleChoice, + MultiSelect, Freeform, Confirmation, } @@ -256,6 +257,12 @@ mod tests { ); } + #[test] + fn question_type_multi_select_exists() { + let q = Question::new("Pick many:", QuestionType::MultiSelect); + assert_eq!(q.question_type, QuestionType::MultiSelect); + } + /// A slow interviewer that waits before answering -- for testing timeouts. struct SlowInterviewer; @@ -296,4 +303,4 @@ mod tests { let answer = ask_with_timeout(&interviewer, q).await; assert_eq!(answer.value, AnswerValue::Yes); } -} +} \ No newline at end of file diff --git a/crates/attractor/src/stylesheet.rs b/crates/attractor/src/stylesheet.rs index ac21615ea..d5166a50e 100644 --- a/crates/attractor/src/stylesheet.rs +++ b/crates/attractor/src/stylesheet.rs @@ -6,9 +6,11 @@ use crate::graph::types::{AttrValue, Graph}; pub enum Selector { /// `*` -- matches all nodes, specificity 0. Universal, - /// `.classname` -- matches nodes with that class, specificity 1. + /// Bare word -- matches nodes by shape name, specificity 1. + Shape(String), + /// `.classname` -- matches nodes with that class, specificity 2. Class(String), - /// `#nodeid` -- matches a specific node, specificity 2. + /// `#nodeid` -- matches a specific node, specificity 3. Id(String), } @@ -17,8 +19,9 @@ impl Selector { pub const fn specificity(&self) -> u8 { match self { Self::Universal => 0, - Self::Class(_) => 1, - Self::Id(_) => 2, + Self::Shape(_) => 1, + Self::Class(_) => 2, + Self::Id(_) => 3, } } } @@ -112,10 +115,19 @@ fn parse_selector(remaining: &mut &str) -> Result { *remaining = remaining[end..].trim(); Ok(Selector::Class(class)) } else { - Err(AttractorError::Stylesheet(format!( - "expected selector ('*', '#id', or '.class'), got: {:?}", - &remaining[..remaining.len().min(20)] - ))) + // Bare word: shape selector + let end = remaining + .find(|c: char| !c.is_ascii_alphanumeric() && c != '_' && c != '-') + .unwrap_or(remaining.len()); + if end == 0 { + return Err(AttractorError::Stylesheet(format!( + "expected selector ('*', '#id', '.class', or shape name), got: {:?}", + &remaining[..remaining.len().min(20)] + ))); + } + let shape = remaining[..end].to_string(); + *remaining = remaining[end..].trim(); + Ok(Selector::Shape(shape)) } } @@ -189,6 +201,7 @@ pub fn apply_stylesheet(stylesheet: &Stylesheet, graph: &mut Graph) { let node = &graph.nodes[node_id.as_str()]; let matches = match &rule.selector { Selector::Universal => true, + Selector::Shape(shape) => node.shape() == shape, Selector::Class(cls) => node.classes.contains(cls), Selector::Id(id) => node_id == id, }; @@ -373,8 +386,9 @@ mod tests { #[test] fn selector_specificity_values() { assert_eq!(Selector::Universal.specificity(), 0); - assert_eq!(Selector::Class("x".into()).specificity(), 1); - assert_eq!(Selector::Id("x".into()).specificity(), 2); + assert_eq!(Selector::Shape("box".into()).specificity(), 1); + assert_eq!(Selector::Class("x".into()).specificity(), 2); + assert_eq!(Selector::Id("x".into()).specificity(), 3); } #[test] @@ -428,9 +442,64 @@ mod tests { } #[test] - fn parse_bare_word_selector_is_error() { - let result = parse_stylesheet("box { llm_model: opus; }"); - assert!(result.is_err()); + fn parse_shape_selector() { + let ss = parse_stylesheet("box { llm_model: opus; }").unwrap(); + assert_eq!(ss.rules.len(), 1); + assert_eq!(ss.rules[0].selector, Selector::Shape("box".into())); + assert_eq!(ss.rules[0].declarations[0].value, "opus"); + } + + #[test] + fn apply_shape_selector_to_matching_nodes() { + let ss = parse_stylesheet("box { llm_model: opus; }").unwrap(); + let mut graph = Graph::new("test"); + + // Default shape is "box" + let box_node = Node::new("a"); + graph.nodes.insert("a".into(), box_node); + + let mut diamond_node = Node::new("b"); + diamond_node + .attrs + .insert("shape".into(), AttrValue::String("Mdiamond".into())); + graph.nodes.insert("b".into(), diamond_node); + + apply_stylesheet(&ss, &mut graph); + + assert_eq!( + graph.nodes["a"].attrs.get("llm_model"), + Some(&AttrValue::String("opus".into())) + ); + // Mdiamond node should NOT get the box rule + assert_eq!(graph.nodes["b"].attrs.get("llm_model"), None); + } + + #[test] + fn shape_overrides_universal_specificity() { + let ss = + parse_stylesheet("* { llm_model: sonnet; } box { llm_model: opus; }").unwrap(); + let mut graph = Graph::new("test"); + graph.nodes.insert("a".into(), Node::new("a")); // default shape = box + apply_stylesheet(&ss, &mut graph); + assert_eq!( + graph.nodes["a"].attrs.get("llm_model"), + Some(&AttrValue::String("opus".into())) + ); + } + + #[test] + fn class_overrides_shape_specificity() { + let ss = + parse_stylesheet("box { llm_model: opus; } .fast { llm_model: flash; }").unwrap(); + let mut graph = Graph::new("test"); + let mut node = Node::new("a"); + node.classes.push("fast".into()); + graph.nodes.insert("a".into(), node); + apply_stylesheet(&ss, &mut graph); + assert_eq!( + graph.nodes["a"].attrs.get("llm_model"), + Some(&AttrValue::String("flash".into())) + ); } #[test] @@ -461,4 +530,4 @@ mod tests { Some(&AttrValue::String("sonnet".into())) ); } -} +} \ No newline at end of file diff --git a/crates/attractor/src/validation/rules.rs b/crates/attractor/src/validation/rules.rs index 971eb34fe..4891b07a4 100644 --- a/crates/attractor/src/validation/rules.rs +++ b/crates/attractor/src/validation/rules.rs @@ -1234,4 +1234,933 @@ mod tests { assert_eq!(d.len(), 1); assert_eq!(d[0].severity, Severity::Error); } + + // --- Additional coverage: condition_syntax invalid case --- + + #[test] + fn condition_syntax_rule_invalid_clause() { + let mut g = minimal_graph(); + let mut edge = Edge::new("start", "exit"); + edge.attrs.insert( + "condition".to_string(), + AttrValue::String("bad clause here".to_string()), + ); + g.edges = vec![edge]; + let rule = ConditionSyntaxRule; + let d = rule.apply(&g); + assert_eq!(d.len(), 1); + assert_eq!(d[0].severity, Severity::Error); + } + + #[test] + fn condition_syntax_rule_not_equals() { + let mut g = minimal_graph(); + let mut edge = Edge::new("start", "exit"); + edge.attrs.insert( + "condition".to_string(), + AttrValue::String("outcome!=failure".to_string()), + ); + g.edges = vec![edge]; + let rule = ConditionSyntaxRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + #[test] + fn condition_syntax_rule_empty_condition() { + let mut g = minimal_graph(); + let mut edge = Edge::new("start", "exit"); + edge.attrs.insert( + "condition".to_string(), + AttrValue::String(String::new()), + ); + g.edges = vec![edge]; + let rule = ConditionSyntaxRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + #[test] + fn condition_syntax_rule_compound_and() { + let mut g = minimal_graph(); + let mut edge = Edge::new("start", "exit"); + edge.attrs.insert( + "condition".to_string(), + AttrValue::String("outcome=success && retries=0".to_string()), + ); + g.edges = vec![edge]; + let rule = ConditionSyntaxRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + // --- Additional coverage: terminal_node two terminals --- + + #[test] + fn terminal_node_rule_two_terminals() { + let mut g = Graph::new("test"); + let mut e1 = Node::new("e1"); + e1.attrs + .insert("shape".to_string(), AttrValue::String("Msquare".to_string())); + let mut e2 = Node::new("e2"); + e2.attrs + .insert("shape".to_string(), AttrValue::String("Msquare".to_string())); + g.nodes.insert("e1".to_string(), e1); + g.nodes.insert("e2".to_string(), e2); + let rule = TerminalNodeRule; + let d = rule.apply(&g); + assert_eq!(d.len(), 1); + assert_eq!(d[0].severity, Severity::Error); + assert!(d[0].message.contains("exactly one")); + } + + // --- Additional coverage: edge_target_exists missing source --- + + #[test] + fn edge_target_exists_rule_missing_source() { + let mut g = minimal_graph(); + g.edges.push(Edge::new("nonexistent_source", "exit")); + let rule = EdgeTargetExistsRule; + let d = rule.apply(&g); + assert_eq!(d.len(), 1); + assert_eq!(d[0].severity, Severity::Error); + assert!(d[0].message.contains("nonexistent_source")); + } + + // --- Additional coverage: reachability no start node --- + + #[test] + fn reachability_rule_no_start_node() { + let mut g = Graph::new("test"); + g.nodes.insert("orphan".to_string(), Node::new("orphan")); + let rule = ReachabilityRule; + let d = rule.apply(&g); + // No start node found, rule returns empty + assert!(d.is_empty()); + } + + // --- Additional coverage: retry_target_exists fallback and graph-level --- + + #[test] + fn retry_target_exists_rule_fallback_missing() { + let mut g = minimal_graph(); + let mut node = Node::new("work"); + node.attrs.insert( + "fallback_retry_target".to_string(), + AttrValue::String("nonexistent".to_string()), + ); + g.nodes.insert("work".to_string(), node); + let rule = RetryTargetExistsRule; + let d = rule.apply(&g); + assert_eq!(d.len(), 1); + assert_eq!(d[0].severity, Severity::Warning); + assert!(d[0].message.contains("fallback_retry_target")); + } + + #[test] + fn retry_target_exists_rule_fallback_valid() { + let mut g = minimal_graph(); + let mut node = Node::new("work"); + node.attrs.insert( + "fallback_retry_target".to_string(), + AttrValue::String("start".to_string()), + ); + g.nodes.insert("work".to_string(), node); + let rule = RetryTargetExistsRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + #[test] + fn retry_target_exists_rule_graph_level_missing() { + let mut g = minimal_graph(); + g.attrs.insert( + "retry_target".to_string(), + AttrValue::String("nonexistent".to_string()), + ); + let rule = RetryTargetExistsRule; + let d = rule.apply(&g); + assert_eq!(d.len(), 1); + assert_eq!(d[0].severity, Severity::Warning); + assert!(d[0].message.contains("Graph")); + } + + #[test] + fn retry_target_exists_rule_graph_level_valid() { + let mut g = minimal_graph(); + g.attrs.insert( + "retry_target".to_string(), + AttrValue::String("start".to_string()), + ); + let rule = RetryTargetExistsRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + #[test] + fn retry_target_exists_rule_graph_fallback_missing() { + let mut g = minimal_graph(); + g.attrs.insert( + "fallback_retry_target".to_string(), + AttrValue::String("nonexistent".to_string()), + ); + let rule = RetryTargetExistsRule; + let d = rule.apply(&g); + assert_eq!(d.len(), 1); + assert_eq!(d[0].severity, Severity::Warning); + assert!(d[0].message.contains("fallback_retry_target")); + } + + #[test] + fn retry_target_exists_rule_graph_fallback_valid() { + let mut g = minimal_graph(); + g.attrs.insert( + "fallback_retry_target".to_string(), + AttrValue::String("exit".to_string()), + ); + let rule = RetryTargetExistsRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + // --- Additional coverage: goal_gate_has_retry with graph-level retry --- + + #[test] + fn goal_gate_has_retry_rule_with_graph_retry_target() { + let mut g = minimal_graph(); + let mut node = Node::new("work"); + node.attrs + .insert("goal_gate".to_string(), AttrValue::Boolean(true)); + g.nodes.insert("work".to_string(), node); + g.attrs.insert( + "retry_target".to_string(), + AttrValue::String("start".to_string()), + ); + let rule = GoalGateHasRetryRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + #[test] + fn goal_gate_has_retry_rule_with_fallback_retry_target() { + let mut g = minimal_graph(); + let mut node = Node::new("work"); + node.attrs + .insert("goal_gate".to_string(), AttrValue::Boolean(true)); + node.attrs.insert( + "fallback_retry_target".to_string(), + AttrValue::String("start".to_string()), + ); + g.nodes.insert("work".to_string(), node); + let rule = GoalGateHasRetryRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + #[test] + fn goal_gate_has_retry_rule_not_goal_gate() { + let mut g = minimal_graph(); + let node = Node::new("work"); + g.nodes.insert("work".to_string(), node); + let rule = GoalGateHasRetryRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + // --- Additional coverage: prompt_on_llm_nodes with label --- + + #[test] + fn prompt_on_llm_nodes_rule_with_label() { + let mut g = minimal_graph(); + let mut node = Node::new("work"); + node.attrs.insert( + "label".to_string(), + AttrValue::String("Do something".to_string()), + ); + g.nodes.insert("work".to_string(), node); + let rule = PromptOnLlmNodesRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + #[test] + fn prompt_on_llm_nodes_rule_non_codergen_no_warning() { + let mut g = minimal_graph(); + let mut node = Node::new("gate"); + node.attrs.insert( + "shape".to_string(), + AttrValue::String("hexagon".to_string()), + ); + g.nodes.insert("gate".to_string(), node); + let rule = PromptOnLlmNodesRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + // --- Additional coverage: fidelity_valid edge and graph-level --- + + #[test] + fn fidelity_valid_rule_invalid_edge_fidelity() { + let mut g = minimal_graph(); + let mut edge = Edge::new("start", "exit"); + edge.attrs.insert( + "fidelity".to_string(), + AttrValue::String("bogus".to_string()), + ); + g.edges = vec![edge]; + let rule = FidelityValidRule; + let d = rule.apply(&g); + assert_eq!(d.len(), 1); + assert_eq!(d[0].severity, Severity::Warning); + assert!(d[0].edge.is_some()); + } + + #[test] + fn fidelity_valid_rule_valid_edge_fidelity() { + let mut g = minimal_graph(); + let mut edge = Edge::new("start", "exit"); + edge.attrs.insert( + "fidelity".to_string(), + AttrValue::String("compact".to_string()), + ); + g.edges = vec![edge]; + let rule = FidelityValidRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + #[test] + fn fidelity_valid_rule_invalid_graph_default() { + let mut g = minimal_graph(); + g.attrs.insert( + "default_fidelity".to_string(), + AttrValue::String("wrong".to_string()), + ); + let rule = FidelityValidRule; + let d = rule.apply(&g); + assert_eq!(d.len(), 1); + assert_eq!(d[0].severity, Severity::Warning); + assert!(d[0].message.contains("default_fidelity")); + } + + #[test] + fn fidelity_valid_rule_valid_graph_default() { + let mut g = minimal_graph(); + g.attrs.insert( + "default_fidelity".to_string(), + AttrValue::String("summary:high".to_string()), + ); + let rule = FidelityValidRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + #[test] + fn fidelity_valid_rule_all_summary_modes() { + let rule = FidelityValidRule; + + let mut g = minimal_graph(); + let mut node = Node::new("w1"); + node.attrs.insert( + "fidelity".to_string(), + AttrValue::String("summary:low".to_string()), + ); + g.nodes.insert("w1".to_string(), node); + assert!(rule.apply(&g).is_empty()); + + let mut g = minimal_graph(); + let mut node = Node::new("w2"); + node.attrs.insert( + "fidelity".to_string(), + AttrValue::String("summary:medium".to_string()), + ); + g.nodes.insert("w2".to_string(), node); + assert!(rule.apply(&g).is_empty()); + + let mut g = minimal_graph(); + let mut node = Node::new("w3"); + node.attrs.insert( + "fidelity".to_string(), + AttrValue::String("truncate".to_string()), + ); + g.nodes.insert("w3".to_string(), node); + assert!(rule.apply(&g).is_empty()); + } + + // --- Additional coverage: freeform_edge_count non-wait.human --- + + #[test] + fn freeform_edge_count_rule_non_wait_human_ignored() { + let mut g = minimal_graph(); + // Regular codergen node (box shape) with multiple freeform edges should not trigger + g.nodes.insert("a".to_string(), Node::new("a")); + g.nodes.insert("b".to_string(), Node::new("b")); + g.nodes.insert("work".to_string(), Node::new("work")); + + let mut e1 = Edge::new("work", "a"); + e1.attrs + .insert("freeform".to_string(), AttrValue::Boolean(true)); + let mut e2 = Edge::new("work", "b"); + e2.attrs + .insert("freeform".to_string(), AttrValue::Boolean(true)); + g.edges.push(e1); + g.edges.push(e2); + + let rule = FreeformEdgeCountRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + #[test] + fn freeform_edge_count_rule_zero_freeform() { + let mut g = minimal_graph(); + let mut gate = Node::new("gate"); + gate.attrs.insert( + "type".to_string(), + AttrValue::String("wait.human".to_string()), + ); + g.nodes.insert("gate".to_string(), gate); + g.nodes.insert("a".to_string(), Node::new("a")); + g.edges.push(Edge::new("gate", "a")); + + let rule = FreeformEdgeCountRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + // --- Additional coverage: stylesheet_syntax no stylesheet --- + + #[test] + fn stylesheet_syntax_rule_no_stylesheet() { + let g = minimal_graph(); + let rule = StylesheetSyntaxRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + // --- Additional coverage: type_known no type attr --- + + #[test] + fn type_known_rule_no_type_attr() { + let g = minimal_graph(); + let rule = TypeKnownRule; + let d = rule.apply(&g); + // Nodes without explicit type attr should not trigger warning + assert!(d.is_empty()); + } + + // --- Additional coverage: start_no_incoming no start node --- + + #[test] + fn start_no_incoming_rule_no_start_node() { + let g = Graph::new("test"); + let rule = StartNoIncomingRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + // --- Additional coverage: exit_no_outgoing by id variants --- + + #[test] + fn exit_no_outgoing_rule_end_id_with_outgoing() { + let mut g = Graph::new("test"); + let mut start = Node::new("start"); + start + .attrs + .insert("shape".to_string(), AttrValue::String("Mdiamond".to_string())); + g.nodes.insert("start".to_string(), start); + let end_node = Node::new("end"); + g.nodes.insert("end".to_string(), end_node); + g.edges.push(Edge::new("start", "end")); + g.edges.push(Edge::new("end", "start")); + let rule = ExitNoOutgoingRule; + let d = rule.apply(&g); + assert_eq!(d.len(), 1); + assert_eq!(d[0].severity, Severity::Error); + assert_eq!(d[0].node_id, Some("end".to_string())); + } + + // --- condition_syntax: bare key (truthy check) is valid --- + + #[test] + fn condition_syntax_rule_bare_key_truthy() { + let mut g = minimal_graph(); + let mut edge = Edge::new("start", "exit"); + edge.attrs.insert( + "condition".to_string(), + AttrValue::String("context.passed".to_string()), + ); + g.edges = vec![edge]; + let rule = ConditionSyntaxRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + // --- condition_syntax: context-prefixed clause with spaces is valid --- + + #[test] + fn condition_syntax_rule_context_prefix_with_space() { + let mut g = minimal_graph(); + let mut edge = Edge::new("start", "exit"); + edge.attrs.insert( + "condition".to_string(), + AttrValue::String("context.foo bar".to_string()), + ); + g.edges = vec![edge]; + let rule = ConditionSyntaxRule; + let d = rule.apply(&g); + // context.-prefixed clauses are allowed even with spaces + assert!(d.is_empty()); + } + + // --- terminal_node: by "Exit" capitalized id --- + + #[test] + fn terminal_node_rule_by_exit_capitalized_id() { + let mut g = Graph::new("test"); + let node = Node::new("Exit"); + g.nodes.insert("Exit".to_string(), node); + let rule = TerminalNodeRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + // --- edge_target_exists: both source and target missing --- + + #[test] + fn edge_target_exists_rule_both_missing() { + let mut g = minimal_graph(); + g.edges + .push(Edge::new("nonexistent_source", "nonexistent_target")); + let rule = EdgeTargetExistsRule; + let d = rule.apply(&g); + assert_eq!(d.len(), 2); + assert_eq!(d[0].severity, Severity::Error); + assert_eq!(d[1].severity, Severity::Error); + } + + // --- reachability: multiple unreachable nodes --- + + #[test] + fn reachability_rule_multiple_unreachable() { + let mut g = minimal_graph(); + g.nodes + .insert("orphan_a".to_string(), Node::new("orphan_a")); + g.nodes + .insert("orphan_b".to_string(), Node::new("orphan_b")); + let rule = ReachabilityRule; + let d = rule.apply(&g); + assert_eq!(d.len(), 2); + assert_eq!(d[0].severity, Severity::Warning); + assert_eq!(d[1].severity, Severity::Warning); + } + + // --- type_known: all known handler types are accepted --- + + #[test] + fn type_known_rule_all_known_types_accepted() { + let mut g = minimal_graph(); + + let mut n1 = Node::new("n1"); + n1.attrs.insert( + "type".to_string(), + AttrValue::String("codergen".to_string()), + ); + g.nodes.insert("n1".to_string(), n1); + + let mut n2 = Node::new("n2"); + n2.attrs.insert( + "type".to_string(), + AttrValue::String("conditional".to_string()), + ); + g.nodes.insert("n2".to_string(), n2); + + let mut n3 = Node::new("n3"); + n3.attrs.insert( + "type".to_string(), + AttrValue::String("parallel".to_string()), + ); + g.nodes.insert("n3".to_string(), n3); + + let mut n4 = Node::new("n4"); + n4.attrs.insert( + "type".to_string(), + AttrValue::String("parallel.fan_in".to_string()), + ); + g.nodes.insert("n4".to_string(), n4); + + let mut n5 = Node::new("n5"); + n5.attrs.insert( + "type".to_string(), + AttrValue::String("tool".to_string()), + ); + g.nodes.insert("n5".to_string(), n5); + + let mut n6 = Node::new("n6"); + n6.attrs.insert( + "type".to_string(), + AttrValue::String("stack.manager_loop".to_string()), + ); + g.nodes.insert("n6".to_string(), n6); + + let rule = TypeKnownRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + // --- prompt_on_llm_nodes: explicit type=codergen without prompt/label --- + + #[test] + fn prompt_on_llm_nodes_rule_explicit_codergen_type_no_prompt() { + let mut g = minimal_graph(); + let mut node = Node::new("work"); + node.attrs.insert( + "type".to_string(), + AttrValue::String("codergen".to_string()), + ); + // No shape=box, but explicit type=codergen + node.attrs.insert( + "shape".to_string(), + AttrValue::String("diamond".to_string()), + ); + g.nodes.insert("work".to_string(), node); + let rule = PromptOnLlmNodesRule; + let d = rule.apply(&g); + assert_eq!(d.len(), 1); + assert_eq!(d[0].severity, Severity::Warning); + } + + // --- goal_gate_has_retry: satisfied by graph-level fallback_retry_target --- + + #[test] + fn goal_gate_has_retry_rule_with_graph_fallback_retry_target() { + let mut g = minimal_graph(); + let mut node = Node::new("work"); + node.attrs + .insert("goal_gate".to_string(), AttrValue::Boolean(true)); + g.nodes.insert("work".to_string(), node); + g.attrs.insert( + "fallback_retry_target".to_string(), + AttrValue::String("start".to_string()), + ); + let rule = GoalGateHasRetryRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + // --- freeform_edge_count: with explicit type=wait.human --- + + #[test] + fn freeform_edge_count_rule_explicit_type_two_freeform() { + let mut g = minimal_graph(); + let mut gate = Node::new("gate"); + gate.attrs.insert( + "type".to_string(), + AttrValue::String("wait.human".to_string()), + ); + g.nodes.insert("gate".to_string(), gate); + g.nodes.insert("a".to_string(), Node::new("a")); + g.nodes.insert("b".to_string(), Node::new("b")); + + let mut e1 = Edge::new("gate", "a"); + e1.attrs + .insert("freeform".to_string(), AttrValue::Boolean(true)); + let mut e2 = Edge::new("gate", "b"); + e2.attrs + .insert("freeform".to_string(), AttrValue::Boolean(true)); + g.edges.push(e1); + g.edges.push(e2); + + let rule = FreeformEdgeCountRule; + let d = rule.apply(&g); + assert_eq!(d.len(), 1); + assert_eq!(d[0].severity, Severity::Error); + } + + // --- exit_no_outgoing: by "Exit" capitalized id --- + + #[test] + fn exit_no_outgoing_rule_exit_capitalized_with_outgoing() { + let mut g = Graph::new("test"); + let mut start = Node::new("start"); + start + .attrs + .insert("shape".to_string(), AttrValue::String("Mdiamond".to_string())); + g.nodes.insert("start".to_string(), start); + let exit_node = Node::new("Exit"); + g.nodes.insert("Exit".to_string(), exit_node); + g.edges.push(Edge::new("start", "Exit")); + g.edges.push(Edge::new("Exit", "start")); + let rule = ExitNoOutgoingRule; + let d = rule.apply(&g); + assert_eq!(d.len(), 1); + assert_eq!(d[0].severity, Severity::Error); + assert_eq!(d[0].node_id, Some("Exit".to_string())); + } + + // --- exit_no_outgoing: by "End" capitalized id --- + + #[test] + fn exit_no_outgoing_rule_end_capitalized_with_outgoing() { + let mut g = Graph::new("test"); + let mut start = Node::new("start"); + start + .attrs + .insert("shape".to_string(), AttrValue::String("Mdiamond".to_string())); + g.nodes.insert("start".to_string(), start); + let end_node = Node::new("End"); + g.nodes.insert("End".to_string(), end_node); + g.edges.push(Edge::new("start", "End")); + g.edges.push(Edge::new("End", "start")); + let rule = ExitNoOutgoingRule; + let d = rule.apply(&g); + assert_eq!(d.len(), 1); + assert_eq!(d[0].severity, Severity::Error); + assert_eq!(d[0].node_id, Some("End".to_string())); + } + + // --- stylesheet_syntax: valid multi-rule stylesheet --- + + #[test] + fn stylesheet_syntax_rule_multi_rule_valid() { + let mut g = minimal_graph(); + g.attrs.insert( + "model_stylesheet".to_string(), + AttrValue::String( + "* { llm_model: gpt-4; } .fast { llm_model: gpt-3.5; reasoning_effort: low; }" + .to_string(), + ), + ); + let rule = StylesheetSyntaxRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + // --- fidelity_valid: multiple simultaneous violations --- + + #[test] + fn fidelity_valid_rule_node_and_edge_and_graph_all_invalid() { + let mut g = minimal_graph(); + + let mut node = Node::new("work"); + node.attrs.insert( + "fidelity".to_string(), + AttrValue::String("invalid_node".to_string()), + ); + g.nodes.insert("work".to_string(), node); + + let mut edge = Edge::new("start", "exit"); + edge.attrs.insert( + "fidelity".to_string(), + AttrValue::String("invalid_edge".to_string()), + ); + g.edges = vec![edge]; + + g.attrs.insert( + "default_fidelity".to_string(), + AttrValue::String("invalid_graph".to_string()), + ); + + let rule = FidelityValidRule; + let d = rule.apply(&g); + assert_eq!(d.len(), 3); + } + + // --- retry_target_exists: both retry_target and fallback on same node, both invalid --- + + #[test] + fn retry_target_exists_rule_both_node_targets_invalid() { + let mut g = minimal_graph(); + let mut node = Node::new("work"); + node.attrs.insert( + "retry_target".to_string(), + AttrValue::String("missing_a".to_string()), + ); + node.attrs.insert( + "fallback_retry_target".to_string(), + AttrValue::String("missing_b".to_string()), + ); + g.nodes.insert("work".to_string(), node); + let rule = RetryTargetExistsRule; + let d = rule.apply(&g); + assert_eq!(d.len(), 2); + assert_eq!(d[0].severity, Severity::Warning); + assert_eq!(d[1].severity, Severity::Warning); + } + + // --- retry_target_exists: both graph-level targets invalid --- + + #[test] + fn retry_target_exists_rule_both_graph_targets_invalid() { + let mut g = minimal_graph(); + g.attrs.insert( + "retry_target".to_string(), + AttrValue::String("missing_a".to_string()), + ); + g.attrs.insert( + "fallback_retry_target".to_string(), + AttrValue::String("missing_b".to_string()), + ); + let rule = RetryTargetExistsRule; + let d = rule.apply(&g); + assert_eq!(d.len(), 2); + assert_eq!(d[0].severity, Severity::Warning); + assert_eq!(d[1].severity, Severity::Warning); + } + + // --- start_no_incoming: multiple incoming edges --- + + #[test] + fn start_no_incoming_rule_multiple_incoming() { + let mut g = minimal_graph(); + g.nodes.insert("a".to_string(), Node::new("a")); + g.edges.push(Edge::new("exit", "start")); + g.edges.push(Edge::new("a", "start")); + let rule = StartNoIncomingRule; + let d = rule.apply(&g); + assert_eq!(d.len(), 1); + assert_eq!(d[0].severity, Severity::Error); + assert!(d[0].message.contains("2")); + } + + // --- prompt_on_llm_nodes: empty prompt string still triggers --- + + #[test] + fn prompt_on_llm_nodes_rule_empty_prompt_no_label() { + let mut g = minimal_graph(); + let mut node = Node::new("work"); + node.attrs.insert( + "prompt".to_string(), + AttrValue::String(String::new()), + ); + g.nodes.insert("work".to_string(), node); + let rule = PromptOnLlmNodesRule; + let d = rule.apply(&g); + assert_eq!(d.len(), 1); + assert_eq!(d[0].severity, Severity::Warning); + } + + // --- prompt_on_llm_nodes: empty label string still triggers --- + + #[test] + fn prompt_on_llm_nodes_rule_empty_label_no_prompt() { + let mut g = minimal_graph(); + let mut node = Node::new("work"); + node.attrs.insert( + "label".to_string(), + AttrValue::String(String::new()), + ); + g.nodes.insert("work".to_string(), node); + let rule = PromptOnLlmNodesRule; + let d = rule.apply(&g); + assert_eq!(d.len(), 1); + assert_eq!(d[0].severity, Severity::Warning); + } + + // --- condition_syntax: no condition attribute at all --- + + #[test] + fn condition_syntax_rule_no_condition_attr() { + let g = minimal_graph(); + let rule = ConditionSyntaxRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + // --- freeform_edge_count: freeform=false does not count --- + + #[test] + fn freeform_edge_count_rule_freeform_false_ignored() { + let mut g = minimal_graph(); + let mut gate = Node::new("gate"); + gate.attrs.insert( + "shape".to_string(), + AttrValue::String("hexagon".to_string()), + ); + g.nodes.insert("gate".to_string(), gate); + g.nodes.insert("a".to_string(), Node::new("a")); + g.nodes.insert("b".to_string(), Node::new("b")); + + let mut e1 = Edge::new("gate", "a"); + e1.attrs + .insert("freeform".to_string(), AttrValue::Boolean(false)); + let mut e2 = Edge::new("gate", "b"); + e2.attrs + .insert("freeform".to_string(), AttrValue::Boolean(false)); + g.edges.push(e1); + g.edges.push(e2); + + let rule = FreeformEdgeCountRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + // --- reachability: chain of reachable nodes --- + + #[test] + fn reachability_rule_chain_all_reachable() { + let mut g = minimal_graph(); + g.nodes.insert("a".to_string(), Node::new("a")); + g.nodes.insert("b".to_string(), Node::new("b")); + g.edges = vec![ + Edge::new("start", "a"), + Edge::new("a", "b"), + Edge::new("b", "exit"), + ]; + let rule = ReachabilityRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + // --- edge_target_exists: no edges at all --- + + #[test] + fn edge_target_exists_rule_no_edges() { + let mut g = minimal_graph(); + g.edges.clear(); + let rule = EdgeTargetExistsRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + // --- goal_gate_has_retry: goal_gate=false explicitly --- + + #[test] + fn goal_gate_has_retry_rule_explicit_false() { + let mut g = minimal_graph(); + let mut node = Node::new("work"); + node.attrs + .insert("goal_gate".to_string(), AttrValue::Boolean(false)); + g.nodes.insert("work".to_string(), node); + let rule = GoalGateHasRetryRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + // --- stylesheet_syntax: empty string stylesheet --- + + #[test] + fn stylesheet_syntax_rule_empty_string() { + let mut g = minimal_graph(); + g.attrs.insert( + "model_stylesheet".to_string(), + AttrValue::String(String::new()), + ); + let rule = StylesheetSyntaxRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } + + // --- type_known: start and exit types from shape are not flagged --- + + #[test] + fn type_known_rule_start_exit_shapes_no_warning() { + // The minimal_graph has start (Mdiamond) and exit (Msquare), which resolve + // to known handler types "start" and "exit" via shape mapping, not explicit type. + // Since they have no explicit `type` attr, the rule should not flag them. + let g = minimal_graph(); + let rule = TypeKnownRule; + let d = rule.apply(&g); + assert!(d.is_empty()); + } } \ No newline at end of file diff --git a/crates/attractor/tests/integration.rs b/crates/attractor/tests/integration.rs index e7b5ceb23..845ef166f 100644 --- a/crates/attractor/tests/integration.rs +++ b/crates/attractor/tests/integration.rs @@ -498,11 +498,17 @@ async fn goal_gate_routes_to_retry_target_on_failure() { }; let result = engine.run(&graph, &config).await; - assert!(result.is_err(), "should fail when goal gate unsatisfied and no retry_target"); - let err_msg = result.unwrap_err().to_string(); + assert!(result.is_ok(), "goal gate unsatisfied with no retry_target should return Ok(fail outcome)"); + let outcome = result.unwrap(); + assert_eq!( + outcome.status, + StageStatus::Fail, + "pipeline outcome should be 'fail' when goal gate unsatisfied" + ); + let failure_reason = outcome.failure_reason.unwrap_or_default(); assert!( - err_msg.contains("goal gate unsatisfied"), - "error should mention goal gate, got: {err_msg}" + failure_reason.contains("goal gate unsatisfied"), + "failure_reason should mention goal gate, got: {failure_reason}" ); } @@ -4579,14 +4585,12 @@ async fn fidelity_compact_preamble_includes_completed_stages_and_context() { } #[tokio::test] -async fn fidelity_summary_detail_increases_with_level() { - // Run three separate pipelines with summary:low, summary:medium, and summary:high. - // Verify that higher detail levels produce longer preambles. - let mut preamble_lengths: Vec<(String, usize)> = Vec::new(); - - // -- summary:low -- - let mut graph_low = make_graph_with_start_exit("SummaryLenLow"); - graph_low.attrs.insert("goal".to_string(), AttrValue::String("Detail test".to_string())); +async fn fidelity_summary_low_excludes_context_values_in_pipeline() { + // summary:low should NOT include context values (only goal, run ID, stage count, recent stages). + // summary:medium should include context values. + // This verifies a behavioral difference between detail levels. + let mut graph_low = make_graph_with_start_exit("SummaryLowExcludesContext"); + graph_low.attrs.insert("goal".to_string(), AttrValue::String("Context exclusion test".to_string())); graph_low.attrs.insert("default_fidelity".to_string(), AttrValue::String("summary:low".to_string())); let mut step_a_low = Node::new("step_a"); step_a_low.attrs.insert("type".to_string(), AttrValue::String("fidelity_capture".to_string())); @@ -4607,12 +4611,18 @@ async fn fidelity_summary_detail_increases_with_level() { let engine_low = PipelineEngine::new(registry_low, EventEmitter::new()); let config_low = RunConfig { logs_root: dir_low.path().to_path_buf(), cancel_token: None }; engine_low.run(&graph_low, &config_low).await.expect("run low"); - let preambles_low = captures_low.preambles.lock().unwrap(); - preamble_lengths.push(("summary:low".to_string(), preambles_low[1].1.len())); - // -- summary:medium -- - let mut graph_med = make_graph_with_start_exit("SummaryLenMed"); - graph_med.attrs.insert("goal".to_string(), AttrValue::String("Detail test".to_string())); + let preambles_low = captures_low.preambles.lock().unwrap(); + let low_preamble = &preambles_low[1].1; + // summary:low should not include "Context values:" section + assert!( + !low_preamble.contains("Context values:"), + "summary:low preamble should not include context values section" + ); + + // Now run summary:medium and verify it DOES include context values + let mut graph_med = make_graph_with_start_exit("SummaryMedIncludesContext"); + graph_med.attrs.insert("goal".to_string(), AttrValue::String("Context exclusion test".to_string())); graph_med.attrs.insert("default_fidelity".to_string(), AttrValue::String("summary:medium".to_string())); let mut step_a_med = Node::new("step_a"); step_a_med.attrs.insert("type".to_string(), AttrValue::String("fidelity_capture".to_string())); @@ -4633,47 +4643,13 @@ async fn fidelity_summary_detail_increases_with_level() { let engine_med = PipelineEngine::new(registry_med, EventEmitter::new()); let config_med = RunConfig { logs_root: dir_med.path().to_path_buf(), cancel_token: None }; engine_med.run(&graph_med, &config_med).await.expect("run med"); + let preambles_med = captures_med.preambles.lock().unwrap(); - preamble_lengths.push(("summary:medium".to_string(), preambles_med[1].1.len())); - - // -- summary:high -- - let mut graph_high = make_graph_with_start_exit("SummaryLenHigh"); - graph_high.attrs.insert("goal".to_string(), AttrValue::String("Detail test".to_string())); - graph_high.attrs.insert("default_fidelity".to_string(), AttrValue::String("summary:high".to_string())); - let mut step_a_high = Node::new("step_a"); - step_a_high.attrs.insert("type".to_string(), AttrValue::String("fidelity_capture".to_string())); - graph_high.nodes.insert("step_a".to_string(), step_a_high); - let mut step_b_high = Node::new("step_b"); - step_b_high.attrs.insert("type".to_string(), AttrValue::String("fidelity_capture".to_string())); - graph_high.nodes.insert("step_b".to_string(), step_b_high); - graph_high.edges.push(Edge::new("start", "step_a")); - graph_high.edges.push(Edge::new("step_a", "step_b")); - graph_high.edges.push(Edge::new("step_b", "exit")); - - let captures_high = FidelityCaptures::new(); - let dir_high = tempfile::tempdir().unwrap(); - let mut registry_high = HandlerRegistry::new(Box::new(StartHandler)); - registry_high.register("start", Box::new(StartHandler)); - registry_high.register("exit", Box::new(ExitHandler)); - registry_high.register("fidelity_capture", Box::new(FidelityCapturingHandler { captures: captures_high.clone() })); - let engine_high = PipelineEngine::new(registry_high, EventEmitter::new()); - let config_high = RunConfig { logs_root: dir_high.path().to_path_buf(), cancel_token: None }; - engine_high.run(&graph_high, &config_high).await.expect("run high"); - let preambles_high = captures_high.preambles.lock().unwrap(); - preamble_lengths.push(("summary:high".to_string(), preambles_high[1].1.len())); - - // Higher summary levels should produce more detailed (longer) preambles + let med_preamble = &preambles_med[1].1; + // summary:medium should include "Context values:" section (graph.goal is always set) assert!( - preamble_lengths[0].1 <= preamble_lengths[1].1, - "summary:low ({}) should be no longer than summary:medium ({})", - preamble_lengths[0].1, - preamble_lengths[1].1, - ); - assert!( - preamble_lengths[1].1 <= preamble_lengths[2].1, - "summary:medium ({}) should be no longer than summary:high ({})", - preamble_lengths[1].1, - preamble_lengths[2].1, + med_preamble.contains("Context values:"), + "summary:medium preamble should include context values section" ); } @@ -6481,4 +6457,4 @@ fn parse_tool_hooks_from_dot_syntax() { work.attrs.get("tool_hooks.pre").and_then(|v| v.as_str()), Some("node pre") ); -} +} \ No newline at end of file diff --git a/crates/llm/src/generate.rs b/crates/llm/src/generate.rs index b34574ed7..0bfc8a9cb 100644 --- a/crates/llm/src/generate.rs +++ b/crates/llm/src/generate.rs @@ -794,9 +794,21 @@ async fn stream_generate_raw( tool_definitions: Option<&[ToolDefinition]>, ) -> Result { let request = build_request(params, messages, tool_definitions); - let inner_stream = client.stream(&request).await?; - if let Some(ref token) = params.abort_signal { + // Apply per_step timeout to the initial connection (Section 4.7) + let inner_stream = if let Some(per_step) = params.timeout.as_ref().and_then(|t| t.per_step) { + let duration = std::time::Duration::from_secs_f64(per_step); + tokio::time::timeout(duration, client.stream(&request)) + .await + .map_err(|_| SdkError::RequestTimeout { + message: format!("Per-step timeout of {per_step}s exceeded"), + })?? + } else { + client.stream(&request).await? + }; + + // Apply abort signal if present + let stream: StreamEventStream = if let Some(ref token) = params.abort_signal { let token = token.clone(); let mapped = inner_stream.map(move |item| { if token.is_cancelled() { @@ -806,9 +818,39 @@ async fn stream_generate_raw( } item }); - Ok(Box::pin(mapped)) + Box::pin(mapped) } else { - Ok(inner_stream) + inner_stream + }; + + // Apply total timeout to the stream (Section 4.7) + if let Some(total) = params.timeout.as_ref().and_then(|t| t.total) { + let duration = std::time::Duration::from_secs_f64(total); + let deadline = tokio::time::Instant::now() + duration; + let total_copy = total; + let timed_stream = futures::stream::unfold( + (stream, false), + move |(mut stream, done)| async move { + if done { + return None; + } + match tokio::time::timeout_at(deadline, stream.next()).await { + Ok(Some(item)) => Some((item, (stream, false))), + Ok(None) => None, // stream completed naturally + Err(_) => { + Some(( + Err(SdkError::RequestTimeout { + message: format!("Total timeout of {total_copy}s exceeded"), + }), + (stream, true), + )) + } + } + }, + ); + Ok(Box::pin(timed_stream)) + } else { + Ok(stream) } } @@ -2615,4 +2657,4 @@ mod tests { }); assert!(has_timeout, "Expected a RequestTimeout error from total timeout"); } -} +} \ No newline at end of file