From eb102e1374ecd7acbea533b78a4024e1aa22c90f Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Thu, 4 Jun 2026 09:09:55 -0600 Subject: [PATCH] fix: don't silently fail on skill role composition extraction in llm nodes --- src/config/input.rs | 62 +++++++++++++++++------------------ src/config/macros.rs | 2 +- src/config/request_context.rs | 42 +++++++++++++----------- src/config/session.rs | 8 ++--- src/function/supervisor.rs | 6 ++-- src/graph/llm.rs | 10 +++--- src/graph/structured.rs | 2 +- src/main.rs | 4 +-- src/repl/mod.rs | 12 +++---- 9 files changed, 74 insertions(+), 74 deletions(-) diff --git a/src/config/input.rs b/src/config/input.rs index 83dff33..89aa8ee 100644 --- a/src/config/input.rs +++ b/src/config/input.rs @@ -38,10 +38,10 @@ pub struct Input { } impl Input { - pub fn from_str(ctx: &RequestContext, text: &str, role: Option) -> Self { - let (role, with_session, with_agent) = resolve_role(ctx, role); + pub fn from_str(ctx: &RequestContext, text: &str, role: Option) -> Result { + let (role, with_session, with_agent) = resolve_role(ctx, role)?; let captured = capture_input_config(ctx, &role); - Self { + Ok(Self { app_config: Arc::clone(&ctx.app.config), stream_enabled: captured.stream_enabled, session: captured.session, @@ -60,7 +60,7 @@ impl Input { rag_name: None, with_session, with_agent, - } + }) } pub async fn from_files( @@ -111,7 +111,7 @@ impl Input { )); } } - let (role, with_session, with_agent) = resolve_role(ctx, role); + let (role, with_session, with_agent) = resolve_role(ctx, role)?; let captured = capture_input_config(ctx, &role); Ok(Self { app_config: Arc::clone(&ctx.app.config), @@ -398,14 +398,14 @@ impl Input { } } -fn resolve_role(ctx: &RequestContext, role: Option) -> (Role, bool, bool) { +fn resolve_role(ctx: &RequestContext, role: Option) -> Result<(Role, bool, bool)> { match role { - Some(v) => (v, false, false), - None => ( - ctx.extract_role(ctx.app.config.as_ref()), + Some(v) => Ok((v, false, false)), + None => Ok(( + ctx.extract_role(ctx.app.config.as_ref())?, ctx.session.is_some(), ctx.agent.is_some(), - ), + )), } } @@ -600,7 +600,7 @@ mod tests { fn resolve_role_with_explicit_role() { let ctx = create_test_ctx(); let role = Role::new("custom", "be helpful"); - let (resolved, with_session, with_agent) = resolve_role(&ctx, Some(role)); + let (resolved, with_session, with_agent) = resolve_role(&ctx, Some(role)).unwrap(); assert_eq!(resolved.name(), "custom"); assert!(!with_session); assert!(!with_agent); @@ -609,7 +609,7 @@ mod tests { #[test] fn resolve_role_without_role_no_session_no_agent() { let ctx = create_test_ctx(); - let (resolved, with_session, with_agent) = resolve_role(&ctx, None); + let (resolved, with_session, with_agent) = resolve_role(&ctx, None).unwrap(); assert_eq!(resolved.name(), ""); assert!(!with_session); assert!(!with_agent); @@ -619,7 +619,7 @@ mod tests { fn resolve_role_without_role_with_session() { let mut ctx = create_test_ctx(); ctx.session = Some(Session::default()); - let (_resolved, with_session, with_agent) = resolve_role(&ctx, None); + let (_resolved, with_session, with_agent) = resolve_role(&ctx, None).unwrap(); assert!(with_session); assert!(!with_agent); } @@ -629,7 +629,7 @@ mod tests { let mut ctx = create_test_ctx(); ctx.session = Some(Session::default()); let role = Role::new("explicit", "prompt"); - let (_resolved, with_session, _with_agent) = resolve_role(&ctx, Some(role)); + let (_resolved, with_session, _with_agent) = resolve_role(&ctx, Some(role)).unwrap(); assert!(!with_session); } @@ -695,7 +695,7 @@ mod tests { #[test] fn input_from_str_captures_text() { let ctx = create_test_ctx(); - let input = Input::from_str(&ctx, "hello world", None); + let input = Input::from_str(&ctx, "hello world", None).unwrap(); assert_eq!(input.text(), "hello world"); } @@ -703,7 +703,7 @@ mod tests { fn input_from_str_with_explicit_role() { let ctx = create_test_ctx(); let role = Role::new("pirate", "you are a pirate"); - let input = Input::from_str(&ctx, "ahoy", Some(role)); + let input = Input::from_str(&ctx, "ahoy", Some(role)).unwrap(); assert_eq!(input.role().name(), "pirate"); assert!(!input.with_agent()); } @@ -715,28 +715,28 @@ mod tests { config.stream = false; state.config = Arc::new(config); let ctx = RequestContext::new(Arc::new(state), WorkingMode::Cmd); - let input = Input::from_str(&ctx, "test", None); + let input = Input::from_str(&ctx, "test", None).unwrap(); assert!(!input.stream_enabled); } #[test] fn input_is_empty_with_no_text_and_no_medias() { let ctx = create_test_ctx(); - let input = Input::from_str(&ctx, "", None); + let input = Input::from_str(&ctx, "", None).unwrap(); assert!(input.is_empty()); } #[test] fn input_is_not_empty_with_text() { let ctx = create_test_ctx(); - let input = Input::from_str(&ctx, "hello", None); + let input = Input::from_str(&ctx, "hello", None).unwrap(); assert!(!input.is_empty()); } #[test] fn input_set_text_changes_text() { let ctx = create_test_ctx(); - let mut input = Input::from_str(&ctx, "original", None); + let mut input = Input::from_str(&ctx, "original", None).unwrap(); input.set_text("modified".to_string()); assert_eq!(input.text(), "modified"); } @@ -744,7 +744,7 @@ mod tests { #[test] fn input_text_returns_patched_when_set() { let ctx = create_test_ctx(); - let mut input = Input::from_str(&ctx, "original", None); + let mut input = Input::from_str(&ctx, "original", None).unwrap(); input.patched_text = Some("patched".to_string()); assert_eq!(input.text(), "patched"); } @@ -752,7 +752,7 @@ mod tests { #[test] fn input_clear_patch_restores_original() { let ctx = create_test_ctx(); - let mut input = Input::from_str(&ctx, "original", None); + let mut input = Input::from_str(&ctx, "original", None).unwrap(); input.patched_text = Some("patched".to_string()); input.clear_patch(); assert_eq!(input.text(), "original"); @@ -761,7 +761,7 @@ mod tests { #[test] fn input_set_continue_output_accumulates() { let ctx = create_test_ctx(); - let mut input = Input::from_str(&ctx, "test", None); + let mut input = Input::from_str(&ctx, "test", None).unwrap(); assert!(input.continue_output().is_none()); input.set_continue_output("first "); assert_eq!(input.continue_output(), Some("first ")); @@ -772,7 +772,7 @@ mod tests { #[test] fn input_set_regenerate_sets_flag_and_clears_tool_calls() { let ctx = create_test_ctx(); - let mut input = Input::from_str(&ctx, "test", None); + let mut input = Input::from_str(&ctx, "test", None).unwrap(); let role = input.role().clone(); assert!(!input.regenerate()); input.set_regenerate(role); @@ -784,7 +784,7 @@ mod tests { fn input_summary_truncates_long_text() { let ctx = create_test_ctx(); let long_text = "a".repeat(200); - let input = Input::from_str(&ctx, &long_text, None); + let input = Input::from_str(&ctx, &long_text, None).unwrap(); let summary = input.summary(); assert!(summary.len() < 200); assert!(summary.ends_with("...")); @@ -793,35 +793,35 @@ mod tests { #[test] fn input_summary_preserves_short_text() { let ctx = create_test_ctx(); - let input = Input::from_str(&ctx, "short", None); + let input = Input::from_str(&ctx, "short", None).unwrap(); assert_eq!(input.summary(), "short"); } #[test] fn input_raw_with_no_files() { let ctx = create_test_ctx(); - let input = Input::from_str(&ctx, "hello", None); + let input = Input::from_str(&ctx, "hello", None).unwrap(); assert_eq!(input.raw(), "hello"); } #[test] fn input_render_with_no_medias() { let ctx = create_test_ctx(); - let input = Input::from_str(&ctx, "hello", None); + let input = Input::from_str(&ctx, "hello", None).unwrap(); assert_eq!(input.render(), "hello"); } #[test] fn input_with_agent_false_when_no_agent() { let ctx = create_test_ctx(); - let input = Input::from_str(&ctx, "test", None); + let input = Input::from_str(&ctx, "test", None).unwrap(); assert!(!input.with_agent()); } #[test] fn input_session_returns_none_when_with_session_false() { let ctx = create_test_ctx(); - let input = Input::from_str(&ctx, "test", Some(Role::new("r", "p"))); + let input = Input::from_str(&ctx, "test", Some(Role::new("r", "p"))).unwrap(); let session = Some(Session::default()); assert!(input.session(&session).is_none()); } @@ -830,7 +830,7 @@ mod tests { fn input_session_returns_some_when_with_session_true() { let mut ctx = create_test_ctx(); ctx.session = Some(Session::default()); - let input = Input::from_str(&ctx, "test", None); + let input = Input::from_str(&ctx, "test", None).unwrap(); let session = Some(Session::default()); assert!(input.session(&session).is_some()); } diff --git a/src/config/macros.rs b/src/config/macros.rs index d4ea27b..179e52f 100644 --- a/src/config/macros.rs +++ b/src/config/macros.rs @@ -29,7 +29,7 @@ pub async fn macro_execute( let variables = macro_value .resolve_variables(&new_args) .map_err(|err| anyhow!("{err}. Usage: {}", macro_value.usage(name)))?; - let role = ctx.extract_role(ctx.app.config.as_ref()); + let role = ctx.extract_role(ctx.app.config.as_ref())?; let mut app_config = (*ctx.app.config).clone(); app_config.temperature = role.temperature(); app_config.top_p = role.top_p(); diff --git a/src/config/request_context.rs b/src/config/request_context.rs index bf63b65..2a23710 100644 --- a/src/config/request_context.rs +++ b/src/config/request_context.rs @@ -37,6 +37,7 @@ use gman::providers::SupportedProvider; use indexmap::IndexMap; use indoc::formatdoc; use inquire::{Confirm, MultiSelect, Text, list_option::ListOption, validator::Validation}; +use log::warn; use parking_lot::RwLock; use std::collections::{BTreeSet, HashMap, HashSet}; use std::fs::{File, OpenOptions, read_dir, read_to_string, remove_dir_all, remove_file}; @@ -601,7 +602,7 @@ impl RequestContext { } } - pub fn extract_role(&self, app: &AppConfig) -> Role { + pub fn extract_role(&self, app: &AppConfig) -> Result { let mut role = if let Some(session) = self.session.as_ref() { session.to_role() } else if let Some(agent) = self.agent.as_ref() { @@ -627,15 +628,13 @@ impl RequestContext { } } - match SkillPolicy::effective( + let policy = SkillPolicy::effective( app, self.role.as_ref(), self.agent.as_ref(), self.session.as_ref(), - ) { - Ok(policy) => self.skill_registry.effective_role(&role, &policy), - Err(_) => role, - } + )?; + Ok(self.skill_registry.effective_role(&role, &policy)) } pub fn auto_continue_config(&self) -> AutoContinueConfig { @@ -852,7 +851,7 @@ impl RequestContext { Some(rag) => rag.get_config(), None => (app.rag_reranker_model.clone(), app.rag_top_k), }; - let role = self.extract_role(app); + let role = self.extract_role(app)?; let mut items = vec![ ("model", role.model().id()), ( @@ -1025,7 +1024,10 @@ impl RequestContext { pub fn generate_prompt_context(&self, app: &AppConfig) -> HashMap<&str, String> { let mut output = HashMap::new(); - let role = self.extract_role(app); + let role = self.extract_role(app).unwrap_or_else(|err| { + warn!("failed to compute effective role for prompt rendering: {err}"); + Role::default() + }); output.insert("model", role.model().id()); output.insert("client_name", role.model().client_name().to_string()); output.insert("model_name", role.model().name().to_string()); @@ -2364,12 +2366,12 @@ impl RequestContext { format!("Failed to cleanup previous '{TEMP_SESSION_NAME}' session") })?; } - session = Some(Session::new_from_ctx(self, app, TEMP_SESSION_NAME)); + session = Some(Session::new_from_ctx(self, app, TEMP_SESSION_NAME)?); } Some(name) => { let session_path = self.session_file(name); if !session_path.exists() { - session = Some(Session::new_from_ctx(self, app, name)); + session = Some(Session::new_from_ctx(self, app, name)?); } else { session = Some(Session::load_from_ctx(self, app, name, &session_path)?); } @@ -2788,7 +2790,7 @@ impl RequestContext { .summarization_prompt .clone() .unwrap_or_else(|| SUMMARIZATION_PROMPT.into()); - let input = Input::from_str(self, &prompt, None); + let input = Input::from_str(self, &prompt, None)?; let summary = input.fetch_chat_text().await?; let summary_context_prompt = self .app @@ -2823,7 +2825,7 @@ impl RequestContext { None => bail!("No chat history"), }; let role = self.retrieve_role(app, CREATE_TITLE_ROLE)?; - let input = Input::from_str(self, &text, Some(role)); + let input = Input::from_str(self, &text, Some(role))?; let text = input.fetch_chat_text().await?; if let Some(session) = self.session.as_mut() { session.set_autoname(&text); @@ -3077,7 +3079,7 @@ mod tests { let app = ctx.app.config.clone(); let role = Role::new("myrole", "my prompt"); ctx.use_role_obj(role).unwrap(); - let extracted = ctx.extract_role(&app); + let extracted = ctx.extract_role(&app).unwrap(); assert_eq!(extracted.name(), "myrole"); } @@ -3085,7 +3087,7 @@ mod tests { fn extract_role_returns_default_when_nothing_active() { let ctx = create_test_ctx(); let app = ctx.app.config.clone(); - let extracted = ctx.extract_role(&app); + let extracted = ctx.extract_role(&app).unwrap(); assert_eq!(extracted.name(), ""); } @@ -3576,7 +3578,7 @@ mod tests { #[test] fn discontinuous_last_message_sets_continuous_false() { let mut ctx = create_test_ctx(); - let input = Input::from_str(&ctx, "test", None); + let input = Input::from_str(&ctx, "test", None).unwrap(); ctx.last_message = Some(LastMessage::new(input, "reply".to_string())); assert!(ctx.last_message.as_ref().unwrap().continuous); ctx.discontinuous_last_message(); @@ -3594,7 +3596,7 @@ mod tests { #[test] fn before_chat_completion_sets_last_message() { let mut ctx = create_test_ctx(); - let input = Input::from_str(&ctx, "hello", None); + let input = Input::from_str(&ctx, "hello", None).unwrap(); ctx.before_chat_completion(&input).unwrap(); assert!(ctx.last_message.is_some()); let lm = ctx.last_message.as_ref().unwrap(); @@ -3618,7 +3620,7 @@ mod tests { ctx.skill_registry.insert(ephemeral).unwrap(); ctx.skill_registry.insert(persistent).unwrap(); - let input = Input::from_str(&ctx, "hello", None); + let input = Input::from_str(&ctx, "hello", None).unwrap(); let app = Arc::clone(&ctx.app.config); ctx.after_chat_completion(app.as_ref(), &input, "response", &[]) .unwrap(); @@ -3641,7 +3643,7 @@ mod tests { let ephemeral = Skill::new("ephemeral", "---\nauto_unload: true\n---\nbody"); ctx.skill_registry.insert(ephemeral).unwrap(); - let input = Input::from_str(&ctx, "hello", None); + let input = Input::from_str(&ctx, "hello", None).unwrap(); let app = Arc::clone(&ctx.app.config); let tool_result = ToolResult::new(crate::function::ToolCall::default(), serde_json::json!({})); @@ -3803,7 +3805,7 @@ mod tests { fn session_new_from_ctx_captures_state() { let _guard = TestConfigDirGuard::new(); let ctx = create_test_ctx(); - let session = Session::new_from_ctx(&ctx, &ctx.app.config, "test-session"); + let session = Session::new_from_ctx(&ctx, &ctx.app.config, "test-session").unwrap(); assert_eq!(session.name(), "test-session"); assert!(session.is_empty()); } @@ -3813,7 +3815,7 @@ mod tests { fn session_save_creates_file() { let _guard = TestConfigDirGuard::new(); let ctx = create_test_ctx(); - let mut session = Session::new_from_ctx(&ctx, &ctx.app.config, "save-test"); + let mut session = Session::new_from_ctx(&ctx, &ctx.app.config, "save-test").unwrap(); let session_path = ctx.session_file("save-test"); ensure_parent_exists(&session_path).unwrap(); diff --git a/src/config/session.rs b/src/config/session.rs index d2cb53c..060ebef 100644 --- a/src/config/session.rs +++ b/src/config/session.rs @@ -106,8 +106,8 @@ impl Session { } } - pub fn new_from_ctx(ctx: &RequestContext, app: &AppConfig, name: &str) -> Self { - let role = ctx.extract_role(app); + pub fn new_from_ctx(ctx: &RequestContext, app: &AppConfig, name: &str) -> Result { + let role = ctx.extract_role(app)?; let mut session = Self { name: name.to_string(), save_session: app.save_session, @@ -115,7 +115,7 @@ impl Session { }; session.set_role(role); session.dirty = false; - session + Ok(session) } pub fn load_from_ctx( @@ -817,7 +817,7 @@ mod tests { functions: Functions::default(), }); let ctx = RequestContext::new(app_state, WorkingMode::Cmd); - let session = Session::new_from_ctx(&ctx, &app_config, "test-session"); + let session = Session::new_from_ctx(&ctx, &app_config, "test-session").unwrap(); assert_eq!(session.name(), "test-session"); assert_eq!(session.save_session(), app_config.save_session); diff --git a/src/function/supervisor.rs b/src/function/supervisor.rs index fa76afd..c15841f 100644 --- a/src/function/supervisor.rs +++ b/src/function/supervisor.rs @@ -469,7 +469,7 @@ pub async fn run_agent_for_graph( child_ctx.init_agent_shared_variables()?; } - let input = Input::from_str(&child_ctx, prompt, None); + let input = Input::from_str(&child_ctx, prompt, None)?; debug!("Spawning agent '{agent_name}' for graph node as '{agent_id}'"); @@ -635,7 +635,7 @@ async fn handle_spawn(ctx: &mut RequestContext, args: &Value) -> Result { child_ctx.init_agent_shared_variables()?; } - let input = Input::from_str(&child_ctx, &prompt, None); + let input = Input::from_str(&child_ctx, &prompt, None)?; debug!("Spawning child agent '{agent_name}' as '{agent_id}'"); @@ -1228,7 +1228,7 @@ async fn summarize_output(ctx: &RequestContext, agent_name: &str, output: &str) "Summarize the following sub-agent output from '{}':\n\n{}", agent_name, output ); - let input = Input::from_str(ctx, &user_message, Some(role)); + let input = Input::from_str(ctx, &user_message, Some(role))?; let summary = input.fetch_chat_text().await?; diff --git a/src/graph/llm.rs b/src/graph/llm.rs index 6aa9e6e..6207194 100644 --- a/src/graph/llm.rs +++ b/src/graph/llm.rs @@ -115,15 +115,13 @@ async fn run( let saved_agent_skill_state = swap_in_node_skill_policy(node, parent_ctx); - let composed_role = match SkillPolicy::effective( + let policy = SkillPolicy::effective( &parent_ctx.app.config, parent_ctx.role.as_ref(), parent_ctx.agent.as_ref(), parent_ctx.session.as_ref(), - ) { - Ok(policy) => parent_ctx.skill_registry.effective_role(&role, &policy), - Err(_) => role, - }; + )?; + let composed_role = parent_ctx.skill_registry.effective_role(&role, &policy); let saved_role = parent_ctx.role.clone(); parent_ctx.role = Some(composed_role); @@ -203,7 +201,7 @@ async fn run_chat_loop(node: &LlmNode, prompt: &str, ctx: &mut RequestContext) - let abort = create_abort_signal(); let app_cfg = Arc::clone(&ctx.app.config); let role_for_input = ctx.role.clone(); - let mut input = Input::from_str(ctx, prompt, role_for_input); + let mut input = Input::from_str(ctx, prompt, role_for_input)?; let mut accumulated = String::new(); for turn in 0..node.max_iterations { diff --git a/src/graph/structured.rs b/src/graph/structured.rs index 587db87..9db94ac 100644 --- a/src/graph/structured.rs +++ b/src/graph/structured.rs @@ -76,7 +76,7 @@ async fn run_one_shot(prompt: &str, ctx: &mut RequestContext) -> Result let abort = create_abort_signal(); let app_cfg = Arc::clone(&ctx.app.config); let role_for_input = ctx.role.clone(); - let input = Input::from_str(ctx, prompt, role_for_input); + let input = Input::from_str(ctx, prompt, role_for_input)?; let client = input.create_client()?; ctx.before_chat_completion(&input)?; let (output, tool_results) = diff --git a/src/main.rs b/src/main.rs index d054b19..4cdf493 100644 --- a/src/main.rs +++ b/src/main.rs @@ -457,7 +457,7 @@ async fn shell_execute( } 'd' => { let role = ctx.retrieve_role(app.as_ref(), EXPLAIN_SHELL_ROLE)?; - let input = Input::from_str(ctx, &eval_str, Some(role)); + let input = Input::from_str(ctx, &eval_str, Some(role))?; if input.stream() { call_chat_completions_streaming( &input, @@ -502,7 +502,7 @@ async fn create_input( ) -> Result { let text = text.unwrap_or_default(); let input = if file.is_empty() { - Input::from_str(ctx, &text, None) + Input::from_str(ctx, &text, None)? } else { Input::from_files_with_spinner(ctx, &text, file.to_vec(), None, abort_signal).await? }; diff --git a/src/repl/mod.rs b/src/repl/mod.rs index e17e6b1..8788539 100644 --- a/src/repl/mod.rs +++ b/src/repl/mod.rs @@ -503,7 +503,7 @@ pub async fn run_repl_command( Some((name, text)) => { let app = Arc::clone(&ctx.app.config); let role = ctx.retrieve_role(app.as_ref(), name.trim())?; - let input = Input::from_str(ctx, text, Some(role)); + let input = Input::from_str(ctx, text, Some(role))?; ask(ctx, abort_signal.clone(), input, false).await?; } None => { @@ -654,7 +654,7 @@ pub async fn run_repl_command( match text { Some(text) => { println!("{}", dimmed_text(&format!(">> {text}"))); - let input = Input::from_str(ctx, &text, None); + let input = Input::from_str(ctx, &text, None)?; ask(ctx, abort_signal.clone(), input, true).await?; } None => { @@ -822,7 +822,7 @@ pub async fn run_repl_command( None => bail!("Unable to regenerate the response"), }; let app = Arc::clone(&ctx.app.config); - input.set_regenerate(ctx.extract_role(&app)); + input.set_regenerate(ctx.extract_role(&app)?); ask(ctx, abort_signal.clone(), input, true).await?; } ".set" => match args { @@ -944,7 +944,7 @@ pub async fn run_repl_command( }, None => { reset_continuation(ctx); - let input = Input::from_str(ctx, line, None); + let input = Input::from_str(ctx, line, None)?; ask(ctx, abort_signal.clone(), input, true).await?; } } @@ -1040,7 +1040,7 @@ async fn ask( format!("{prompt}\n\n{todo_state}") }; - let continuation_input = Input::from_str(ctx, &full_prompt, None); + let continuation_input = Input::from_str(ctx, &full_prompt, None)?; ask(ctx, abort_signal, continuation_input, false).await } else { reset_continuation(ctx); @@ -1113,7 +1113,7 @@ async fn ask( format!("{prompt}\n\n{todo_state}") }; - let continuation_input = Input::from_str(ctx, &full_prompt, None); + let continuation_input = Input::from_str(ctx, &full_prompt, None)?; return ask(ctx, abort_signal, continuation_input, false).await; } }