diff --git a/config.agent.example.yaml b/config.agent.example.yaml index 14a7324..6691b00 100644 --- a/config.agent.example.yaml +++ b/config.agent.example.yaml @@ -26,6 +26,9 @@ auto_continue: false # Enable automatic continuation when incomplete max_auto_continues: 10 # Maximum number of automatic continuations before stopping inject_todo_instructions: true # Inject the default todo tool usage instructions into the agent's system prompt continuation_prompt: null # Custom prompt used when auto-continuing (optional; uses default if null) +inject_skill_instructions: true # Inject a short hint pointing the model at `skill__list` when skills are enabled + # (default: true). Suppressed automatically when no skills are available. +skill_instructions: null # Custom text for the skill hint (optional; uses built-in default if null) # Sub-Agent Spawning System # Enable this agent to spawn and manage child agents in parallel. # See https://github.com/Dark-Alex-17/coyote/wiki/Agents for detailed documentation. diff --git a/config.example.yaml b/config.example.yaml index 4068864..29e5cb2 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -162,6 +162,10 @@ auto_continue: false # Enable automatic continuation when incomplet max_auto_continues: 10 # Maximum number of automatic continuations before stopping (default: 10) inject_todo_instructions: true # Inject default todo usage instructions into the system prompt (default: true) continuation_prompt: null # Custom prompt used when auto-continuing. If null, uses built-in default +inject_skill_instructions: true # Inject a short hint pointing the model at `skill__list` when skills are enabled in + # this context. Only injected if `function_calling_support`, `skills_enabled`, and the + # effective enabled skill set is non-empty (default: true). +skill_instructions: null # Custom text used for the skill hint when injected. If null, uses built-in default. # ---- Session ---- # See the [Session documentation](https://github.com/Dark-Alex-17/coyote/wiki/Sessions) for more information diff --git a/config.role.example.md b/config.role.example.md index 84ffef7..9846e1a 100644 --- a/config.role.example.md +++ b/config.role.example.md @@ -30,5 +30,8 @@ auto_continue: false # Enable automatic continuation when incom max_auto_continues: 10 # Maximum number of automatic continuations before stopping (default: 10) inject_todo_instructions: true # Inject default todo tool usage instructions into the system prompt (default: true) continuation_prompt: null # Custom prompt used when auto-continuing. If null, uses built-in default +inject_skill_instructions: true # Inject a short hint pointing the model at `skill__list` when skills are enabled + # (default: true). Suppressed automatically when no skills are available. +skill_instructions: null # Custom text for the skill hint (optional; uses built-in default if null) --- You are an expert at doing things. This is where you write the instructions for the role. diff --git a/graph.example.yaml b/graph.example.yaml index fa7b262..21e5ab3 100644 --- a/graph.example.yaml +++ b/graph.example.yaml @@ -63,6 +63,9 @@ enabled_skills: - code-review - git-master - ai-slop-remover +inject_skill_instructions: true # Inject a hint pointing the model at `skill__list`. Defaults to true; suppressed + # automatically when no skills are available. +skill_instructions: null # Custom text for the skill hint (optional; uses the built-in default if omitted). conversation_starters: # Suggested prompts surfaced in the UI - "Research the current state of WebAssembly outside the browser" @@ -176,6 +179,9 @@ nodes: skills_enabled: true # Whether skills are enabled on this llm node; defaults to 'true' enabled_skills: - ai-slop-remover + inject_skill_instructions: true # Override skill-hint injection for just this node. Falls back to + # agent/graph/global default when omitted. + skill_instructions: null # Per-node skill-hint text override; uses the built-in default when omitted. output_schema: # Optional JSON Schema. The output is parsed to JSON type: object # and its top-level object keys auto-merge into state properties: # (so `topic` / `needs_deep_dive` become {{topic}} etc). diff --git a/src/config/agent.rs b/src/config/agent.rs index c04106d..c70750f 100644 --- a/src/config/agent.rs +++ b/src/config/agent.rs @@ -464,6 +464,14 @@ impl Agent { self.config.continuation_prompt.clone() } + pub fn inject_skill_instructions(&self) -> bool { + self.config.inject_skill_instructions + } + + pub fn skill_instructions_value(&self) -> Option { + self.config.skill_instructions.clone() + } + pub fn can_spawn_agents(&self) -> bool { self.config.can_spawn_agents } @@ -625,6 +633,10 @@ pub struct AgentConfig { pub inject_todo_instructions: bool, #[serde(default = "default_true")] pub inject_spawn_instructions: bool, + #[serde(default = "default_true")] + pub inject_skill_instructions: bool, + #[serde(skip_serializing_if = "Option::is_none")] + pub skill_instructions: Option, #[serde(skip_serializing_if = "Option::is_none")] pub compression_threshold: Option, #[serde(default)] @@ -704,6 +716,8 @@ impl AgentConfig { mcp_servers: graph.mcp_servers.clone(), skills_enabled: graph.skills_enabled, enabled_skills: graph.enabled_skills.clone(), + inject_skill_instructions: graph.inject_skill_instructions.unwrap_or(true), + skill_instructions: graph.skill_instructions.clone(), conversation_starters: graph.conversation_starters.clone(), variables: graph.variables.clone(), can_spawn_agents: graph.has_agent_node(), diff --git a/src/config/app_config.rs b/src/config/app_config.rs index c19f68b..6274860 100644 --- a/src/config/app_config.rs +++ b/src/config/app_config.rs @@ -52,6 +52,8 @@ pub struct AppConfig { pub max_auto_continues: usize, pub inject_todo_instructions: bool, pub continuation_prompt: Option, + pub inject_skill_instructions: bool, + pub skill_instructions: Option, pub repl_prelude: Option, pub cmd_prelude: Option, @@ -118,6 +120,8 @@ impl Default for AppConfig { max_auto_continues: 10, inject_todo_instructions: true, continuation_prompt: None, + inject_skill_instructions: true, + skill_instructions: None, repl_prelude: None, cmd_prelude: None, @@ -185,6 +189,8 @@ impl AppConfig { max_auto_continues: config.max_auto_continues, inject_todo_instructions: config.inject_todo_instructions, continuation_prompt: config.continuation_prompt, + inject_skill_instructions: config.inject_skill_instructions, + skill_instructions: config.skill_instructions, repl_prelude: config.repl_prelude, cmd_prelude: config.cmd_prelude, diff --git a/src/config/mod.rs b/src/config/mod.rs index 3860f43..5b54539 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -6,7 +6,7 @@ mod install_remote; mod macros; mod mcp_factory; pub(crate) mod paths; -mod prompts; +pub(crate) mod prompts; mod rag_cache; mod request_context; mod role; @@ -28,7 +28,7 @@ pub use self::app_state::AppState; pub use self::input::Input; pub use self::install_remote::{install_remote, install_remote_from_repl_args}; #[allow(unused_imports)] -pub use self::request_context::{RenderMode, RequestContext}; +pub use self::request_context::{RenderMode, RequestContext, should_inject_skill_instructions}; pub use self::role::{ CODE_ROLE, CREATE_TITLE_ROLE, EXPLAIN_SHELL_ROLE, Role, RoleLike, SHELL_ROLE, }; @@ -214,6 +214,8 @@ pub struct Config { pub max_auto_continues: usize, pub inject_todo_instructions: bool, pub continuation_prompt: Option, + pub inject_skill_instructions: bool, + pub skill_instructions: Option, pub repl_prelude: Option, pub cmd_prelude: Option, @@ -280,6 +282,8 @@ impl Default for Config { max_auto_continues: 10, inject_todo_instructions: true, continuation_prompt: None, + inject_skill_instructions: true, + skill_instructions: None, repl_prelude: None, cmd_prelude: None, diff --git a/src/config/prompts.rs b/src/config/prompts.rs index 0a3553c..d230543 100644 --- a/src/config/prompts.rs +++ b/src/config/prompts.rs @@ -1,5 +1,13 @@ use indoc::indoc; +pub(crate) const DEFAULT_SKILL_INSTRUCTIONS: &str = indoc! {" + ## Skills + Specialized skills may be available in this context. Call `skill__list` early in a task to + discover any that match the work, then `skill__load` the relevant ones. Their instructions and + granted tools will become active for subsequent turns. Call `skill__unload` when their work is + complete to keep the context lean." +}; + pub(in crate::config) const DEFAULT_TODO_INSTRUCTIONS: &str = indoc! {" ## Task Tracking You have built-in task tracking tools. Use them to track your progress: diff --git a/src/config/request_context.rs b/src/config/request_context.rs index 9d0996d..84af488 100644 --- a/src/config/request_context.rs +++ b/src/config/request_context.rs @@ -39,6 +39,7 @@ use indoc::formatdoc; use inquire::{Confirm, MultiSelect, Text, list_option::ListOption, validator::Validation}; use log::warn; use parking_lot::RwLock; +use prompts::DEFAULT_SKILL_INSTRUCTIONS; use std::collections::{BTreeSet, HashMap, HashSet}; use std::fs::{File, OpenOptions, read_dir, read_to_string, remove_dir_all, remove_file}; use std::io::Write; @@ -53,6 +54,20 @@ pub struct AutoContinueConfig { pub continuation_prompt: Option, } +pub struct SkillInstructionsConfig { + pub inject: bool, + pub instructions: Option, +} + +/// Must stay in sync with the predicate that registers `skill__*` tools in `rebuild_tool_scope` +/// (and in `graph::llm::run_llm_node`). Telling the model to call tools that are not exposed +/// is a footgun. `compatible_enabled` is the post-filter universe that `skill__list` would +/// actually return (cascade-allowed AND surviving `Skill::is_compatible` for current +/// `mcp_server_support`), so an empty set means the hint has nothing to point at. +pub fn should_inject_skill_instructions(app: &AppConfig, policy: &SkillPolicy) -> bool { + app.function_calling_support && policy.skills_enabled && !policy.compatible_enabled.is_empty() +} + #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] pub enum RenderMode { #[default] @@ -634,9 +649,62 @@ impl RequestContext { self.agent.as_ref(), self.session.as_ref(), )?; + + if should_inject_skill_instructions(app, &policy) { + let config = self.skill_instructions_config(); + + if config.inject { + let separator = if role.is_empty_prompt() { "" } else { "\n\n" }; + + role.append_to_prompt(separator); + role.append_to_prompt( + config + .instructions + .as_deref() + .unwrap_or(DEFAULT_SKILL_INSTRUCTIONS), + ); + } + } + Ok(self.skill_registry.effective_role(&role, &policy)) } + pub fn skill_instructions_config(&self) -> SkillInstructionsConfig { + if let Some(agent) = &self.agent { + return SkillInstructionsConfig { + inject: agent.inject_skill_instructions(), + instructions: agent.skill_instructions_value(), + }; + } + + let app = &self.app.config; + let inject = self + .session + .as_ref() + .and_then(|s| s.inject_skill_instructions()) + .or_else(|| { + self.role + .as_ref() + .and_then(|r| r.inject_skill_instructions()) + }) + .unwrap_or(app.inject_skill_instructions); + let instructions = self + .session + .as_ref() + .and_then(|s| s.skill_instructions().map(|v| v.to_string())) + .or_else(|| { + self.role + .as_ref() + .and_then(|r| r.skill_instructions().map(|v| v.to_string())) + }) + .or_else(|| app.skill_instructions.clone()); + + SkillInstructionsConfig { + inject, + instructions, + } + } + pub fn auto_continue_config(&self) -> AutoContinueConfig { if let Some(agent) = &self.agent { return AutoContinueConfig { @@ -1707,7 +1775,7 @@ impl RequestContext { } let value = match key { - "continuation_prompt" => raw_value, + "continuation_prompt" | "skill_instructions" => raw_value, _ => { if raw_value.contains(char::is_whitespace) { bail!("Usage: .set . If value is null, unset key."); @@ -1907,6 +1975,22 @@ impl RequestContext { self.update_app_config(|app| app.continuation_prompt = value); } } + "inject_skill_instructions" => { + let value: bool = value.parse().with_context(|| "Invalid value")?; + if let Some(session) = self.session.as_mut() { + session.set_inject_skill_instructions(Some(value)); + } else { + self.update_app_config(|app| app.inject_skill_instructions = value); + } + } + "skill_instructions" => { + let value: Option = super::parse_value(value)?; + if let Some(session) = self.session.as_mut() { + session.set_skill_instructions(value); + } else { + self.update_app_config(|app| app.skill_instructions = value); + } + } _ => bail!("Unknown key '{key}'"), } Ok(()) @@ -2006,6 +2090,8 @@ impl RequestContext { "enabled_tools", "enabled_mcp_servers", "inject_todo_instructions", + "inject_skill_instructions", + "skill_instructions", "max_auto_continues", "save_session", "compression_threshold", @@ -2172,6 +2258,11 @@ impl RequestContext { super::complete_bool(config.inject_instructions) } "continuation_prompt" => vec!["null".to_string()], + "inject_skill_instructions" => { + let config = self.skill_instructions_config(); + super::complete_bool(config.inject) + } + "skill_instructions" => vec!["null".to_string()], _ => vec![], }; values = candidates.into_iter().map(|v| (v, None)).collect(); @@ -3123,6 +3214,108 @@ mod tests { assert_eq!(extracted.name(), ""); } + #[test] + fn should_inject_skill_instructions_requires_function_calling() { + let app = AppConfig { + function_calling_support: false, + ..AppConfig::default() + }; + + let policy = SkillPolicy { + skills_enabled: true, + enabled: ["a".to_string()].into_iter().collect(), + compatible_enabled: ["a".to_string()].into_iter().collect(), + }; + + assert!(!should_inject_skill_instructions(&app, &policy)); + } + + #[test] + fn should_inject_skill_instructions_requires_skills_enabled() { + let app = AppConfig { + function_calling_support: true, + ..AppConfig::default() + }; + + let policy = SkillPolicy { + skills_enabled: false, + enabled: ["a".to_string()].into_iter().collect(), + compatible_enabled: ["a".to_string()].into_iter().collect(), + }; + + assert!(!should_inject_skill_instructions(&app, &policy)); + } + + #[test] + fn should_inject_skill_instructions_suppresses_when_no_compatible_skills() { + let app = AppConfig { + function_calling_support: true, + ..AppConfig::default() + }; + + // `enabled` has names, but none survive the compatibility filter — hint must suppress. + let policy = SkillPolicy { + skills_enabled: true, + enabled: ["a".to_string()].into_iter().collect(), + compatible_enabled: Default::default(), + }; + + assert!(!should_inject_skill_instructions(&app, &policy)); + } + + #[test] + fn should_inject_skill_instructions_when_all_conditions_met() { + let app = AppConfig { + function_calling_support: true, + ..AppConfig::default() + }; + + let policy = SkillPolicy { + skills_enabled: true, + enabled: ["a".to_string()].into_iter().collect(), + compatible_enabled: ["a".to_string()].into_iter().collect(), + }; + + assert!(should_inject_skill_instructions(&app, &policy)); + } + + #[test] + fn skill_instructions_config_falls_back_to_app_default() { + let ctx = create_test_ctx(); + + let cfg = ctx.skill_instructions_config(); + + assert!(cfg.inject); + assert!(cfg.instructions.is_none()); + } + + #[test] + fn skill_instructions_config_respects_role_disable() { + let mut ctx = create_test_ctx(); + let role = Role::new("r", "---\ninject_skill_instructions: false\n---\nhello"); + ctx.use_role_obj(role).unwrap(); + + let cfg = ctx.skill_instructions_config(); + + assert!(!cfg.inject); + } + + #[test] + fn skill_instructions_config_session_overrides_role() { + let mut ctx = create_test_ctx(); + let role = Role::new("r", "---\ninject_skill_instructions: false\n---\nhello"); + ctx.use_role_obj(role).unwrap(); + let mut session = Session::default(); + session.set_inject_skill_instructions(Some(true)); + session.set_skill_instructions(Some("custom hint".into())); + ctx.session = Some(session); + + let cfg = ctx.skill_instructions_config(); + + assert!(cfg.inject); + assert_eq!(cfg.instructions.as_deref(), Some("custom hint")); + } + #[test] fn exit_session_clears_session() { let mut ctx = create_test_ctx(); diff --git a/src/config/role.rs b/src/config/role.rs index 8f229eb..9508953 100644 --- a/src/config/role.rs +++ b/src/config/role.rs @@ -79,6 +79,10 @@ pub struct Role { inject_todo_instructions: Option, #[serde(skip_serializing_if = "Option::is_none")] continuation_prompt: Option, + #[serde(skip_serializing_if = "Option::is_none")] + inject_skill_instructions: Option, + #[serde(skip_serializing_if = "Option::is_none")] + skill_instructions: Option, #[serde(skip)] model: Model, @@ -124,6 +128,10 @@ impl Role { "continuation_prompt" => { role.continuation_prompt = value.as_str().map(|v| v.to_string()) } + "inject_skill_instructions" => role.inject_skill_instructions = value.as_bool(), + "skill_instructions" => { + role.skill_instructions = value.as_str().map(|v| v.to_string()) + } _ => (), } } @@ -189,6 +197,14 @@ impl Role { if let Some(continuation_prompt) = &self.continuation_prompt { metadata.push(format!("continuation_prompt: {continuation_prompt}")); } + if let Some(inject_skill_instructions) = self.inject_skill_instructions { + metadata.push(format!( + "inject_skill_instructions: {inject_skill_instructions}" + )); + } + if let Some(skill_instructions) = &self.skill_instructions { + metadata.push(format!("skill_instructions: {skill_instructions}")); + } if metadata.is_empty() { format!("{}\n", self.prompt) } else if self.prompt.is_empty() { @@ -299,6 +315,14 @@ impl Role { self.continuation_prompt.as_deref() } + pub fn inject_skill_instructions(&self) -> Option { + self.inject_skill_instructions + } + + pub fn skill_instructions(&self) -> Option<&str> { + self.skill_instructions.as_deref() + } + pub fn skills_enabled(&self) -> Option { self.skills_enabled } diff --git a/src/config/session.rs b/src/config/session.rs index 060ebef..f18772f 100644 --- a/src/config/session.rs +++ b/src/config/session.rs @@ -56,6 +56,10 @@ pub struct Session { inject_todo_instructions: Option, #[serde(skip_serializing_if = "Option::is_none")] continuation_prompt: Option, + #[serde(skip_serializing_if = "Option::is_none")] + inject_skill_instructions: Option, + #[serde(skip_serializing_if = "Option::is_none")] + skill_instructions: Option, #[serde(skip_serializing_if = "Option::is_none")] role_name: Option, @@ -227,6 +231,12 @@ impl Session { if let Some(continuation_prompt) = self.continuation_prompt() { data["continuation_prompt"] = continuation_prompt.into(); } + if let Some(inject_skill_instructions) = self.inject_skill_instructions() { + data["inject_skill_instructions"] = inject_skill_instructions.into(); + } + if let Some(skill_instructions) = self.skill_instructions() { + data["skill_instructions"] = skill_instructions.into(); + } let (tokens, percent) = self.tokens_usage(); data["total_tokens"] = tokens.into(); if let Some(max_input_tokens) = self.model().max_input_tokens() { @@ -305,6 +315,15 @@ impl Session { if let Some(continuation_prompt) = self.continuation_prompt() { items.push(("continuation_prompt", continuation_prompt.to_string())); } + if let Some(inject_skill_instructions) = self.inject_skill_instructions() { + items.push(( + "inject_skill_instructions", + inject_skill_instructions.to_string(), + )); + } + if let Some(skill_instructions) = self.skill_instructions() { + items.push(("skill_instructions", skill_instructions.to_string())); + } if let Some(max_input_tokens) = self.model().max_input_tokens() { items.push(("max_input_tokens", max_input_tokens.to_string())); @@ -446,6 +465,14 @@ impl Session { self.continuation_prompt.as_deref() } + pub fn inject_skill_instructions(&self) -> Option { + self.inject_skill_instructions + } + + pub fn skill_instructions(&self) -> Option<&str> { + self.skill_instructions.as_deref() + } + pub fn set_inject_todo_instructions(&mut self, value: Option) { if self.inject_todo_instructions != value { self.inject_todo_instructions = value; @@ -460,6 +487,20 @@ impl Session { } } + pub fn set_inject_skill_instructions(&mut self, value: Option) { + if self.inject_skill_instructions != value { + self.inject_skill_instructions = value; + self.dirty = true; + } + } + + pub fn set_skill_instructions(&mut self, value: Option) { + if self.skill_instructions != value { + self.skill_instructions = value; + self.dirty = true; + } + } + pub fn needs_compression(&self, global_compression_threshold: usize) -> bool { if self.compressing { return false; diff --git a/src/config/skill_policy.rs b/src/config/skill_policy.rs index 7411a91..3f99855 100644 --- a/src/config/skill_policy.rs +++ b/src/config/skill_policy.rs @@ -3,14 +3,16 @@ use super::app_config::AppConfig; use super::paths; use super::role::Role; use super::session::Session; +use super::skill::Skill; use anyhow::{Result, anyhow, bail}; -use std::collections::HashSet; +use std::collections::{BTreeSet, HashSet}; #[derive(Debug)] pub struct SkillPolicy { pub skills_enabled: bool, pub enabled: HashSet, + pub compatible_enabled: BTreeSet, } impl SkillPolicy { @@ -27,20 +29,27 @@ impl SkillPolicy { session, &paths::has_skill, &paths::list_skills, + &|name, mcp_on| { + Skill::load(name) + .map(|s| s.is_compatible(mcp_on)) + .unwrap_or(false) + }, ) } - fn effective_with( + fn effective_with( global: &AppConfig, role: Option<&Role>, agent: Option<&Agent>, session: Option<&Session>, skill_exists: &F, list_installed: &G, + skill_is_compatible: &H, ) -> Result where F: Fn(&str) -> bool, G: Fn() -> Vec, + H: Fn(&str, bool) -> bool, { let mut skills_enabled = global.skills_enabled; if let Some(r) = role @@ -104,9 +113,21 @@ impl SkillPolicy { }, }; + let compatible_enabled: BTreeSet = if skills_enabled { + let mcp_on = global.mcp_server_support; + enabled + .iter() + .filter(|name| skill_is_compatible(name, mcp_on)) + .cloned() + .collect() + } else { + BTreeSet::new() + }; + Ok(Self { skills_enabled, enabled, + compatible_enabled, }) } @@ -128,6 +149,10 @@ mod tests { Vec::new() } + fn all_compatible(_: &str, _: bool) -> bool { + true + } + fn make_app_config( skills_enabled: bool, enabled: Option<&str>, @@ -145,9 +170,16 @@ mod tests { fn defaults_yield_skills_enabled_with_empty_universe() { let global = AppConfig::default(); - let policy = - SkillPolicy::effective_with(&global, None, None, None, &always_true, &empty_installed) - .unwrap(); + let policy = SkillPolicy::effective_with( + &global, + None, + None, + None, + &always_true, + &empty_installed, + &all_compatible, + ) + .unwrap(); assert!(policy.skills_enabled); assert!(policy.enabled.is_empty()); @@ -158,9 +190,16 @@ mod tests { let global = AppConfig::default(); let installed = || vec!["alpha".to_string(), "beta".to_string()]; - let policy = - SkillPolicy::effective_with(&global, None, None, None, &always_true, &installed) - .unwrap(); + let policy = SkillPolicy::effective_with( + &global, + None, + None, + None, + &always_true, + &installed, + &all_compatible, + ) + .unwrap(); assert_eq!(policy.enabled.len(), 2); assert!(policy.enabled.contains("alpha")); @@ -171,9 +210,16 @@ mod tests { fn falls_back_to_visible_when_visible_set_but_no_enabled() { let global = make_app_config(true, None, Some(&["alpha", "beta"])); - let policy = - SkillPolicy::effective_with(&global, None, None, None, &always_true, &empty_installed) - .unwrap(); + let policy = SkillPolicy::effective_with( + &global, + None, + None, + None, + &always_true, + &empty_installed, + &all_compatible, + ) + .unwrap(); assert_eq!(policy.enabled.len(), 2); assert!(policy.enabled.contains("alpha")); @@ -184,9 +230,16 @@ mod tests { fn global_enabled_skills_is_effective_when_no_other_levels() { let global = make_app_config(true, Some("alpha,beta"), Some(&["alpha", "beta", "gamma"])); - let policy = - SkillPolicy::effective_with(&global, None, None, None, &always_true, &empty_installed) - .unwrap(); + let policy = SkillPolicy::effective_with( + &global, + None, + None, + None, + &always_true, + &empty_installed, + &all_compatible, + ) + .unwrap(); assert!(policy.enabled.contains("alpha")); assert!(policy.enabled.contains("beta")); @@ -205,6 +258,7 @@ mod tests { None, &always_true, &empty_installed, + &all_compatible, ) .unwrap(); @@ -224,6 +278,7 @@ mod tests { None, &always_true, &empty_installed, + &all_compatible, ) .unwrap(); @@ -237,9 +292,15 @@ mod tests { ..AppConfig::default() }; - let policy = SkillPolicy::effective_with(&global, None, None, None, &always_true, &|| { - vec!["alpha".to_string()] - }) + let policy = SkillPolicy::effective_with( + &global, + None, + None, + None, + &always_true, + &|| vec!["alpha".to_string()], + &all_compatible, + ) .unwrap(); assert!(!policy.allows("alpha")); @@ -249,9 +310,16 @@ mod tests { fn allows_returns_true_when_skill_in_enabled_set() { let global = make_app_config(true, Some("alpha"), None); - let policy = - SkillPolicy::effective_with(&global, None, None, None, &always_true, &empty_installed) - .unwrap(); + let policy = SkillPolicy::effective_with( + &global, + None, + None, + None, + &always_true, + &empty_installed, + &all_compatible, + ) + .unwrap(); assert!(policy.allows("alpha")); assert!(!policy.allows("beta")); @@ -261,9 +329,16 @@ mod tests { fn validation_rejects_uninstalled_skill_reference() { let global = make_app_config(true, Some("ghost"), None); - let err = - SkillPolicy::effective_with(&global, None, None, None, &|_| false, &empty_installed) - .unwrap_err(); + let err = SkillPolicy::effective_with( + &global, + None, + None, + None, + &|_| false, + &empty_installed, + &all_compatible, + ) + .unwrap_err(); assert!(err.to_string().contains("not installed")); assert!(err.to_string().contains("ghost")); @@ -273,9 +348,16 @@ mod tests { fn validation_rejects_skill_not_in_visible_set() { let global = make_app_config(true, Some("beta"), Some(&["alpha"])); - let err = - SkillPolicy::effective_with(&global, None, None, None, &always_true, &empty_installed) - .unwrap_err(); + let err = SkillPolicy::effective_with( + &global, + None, + None, + None, + &always_true, + &empty_installed, + &all_compatible, + ) + .unwrap_err(); assert!( err.to_string() @@ -288,9 +370,16 @@ mod tests { fn validation_skipped_when_no_explicit_enabled_skills() { let global = make_app_config(true, None, None); - let policy = - SkillPolicy::effective_with(&global, None, None, None, &|_| false, &empty_installed) - .unwrap(); + let policy = SkillPolicy::effective_with( + &global, + None, + None, + None, + &|_| false, + &empty_installed, + &all_compatible, + ) + .unwrap(); assert!(policy.enabled.is_empty()); } @@ -307,9 +396,142 @@ mod tests { None, &always_true, &empty_installed, + &all_compatible, ) .unwrap(); assert!(policy.enabled.is_empty()); } + + #[test] + fn compatible_enabled_is_empty_when_skills_disabled() { + let global = AppConfig { + skills_enabled: false, + enabled_skills: Some(vec!["alpha".into()]), + visible_skills: Some(vec!["alpha".into()]), + ..AppConfig::default() + }; + + let policy = SkillPolicy::effective_with( + &global, + None, + None, + None, + &always_true, + &empty_installed, + &all_compatible, + ) + .unwrap(); + + assert!(!policy.skills_enabled); + assert!(policy.compatible_enabled.is_empty()); + } + + #[test] + fn compatible_enabled_short_circuits_callback_when_skills_disabled() { + use std::cell::Cell; + let global = AppConfig { + skills_enabled: false, + enabled_skills: Some(vec!["alpha".into()]), + visible_skills: Some(vec!["alpha".into()]), + ..AppConfig::default() + }; + let invoked = Cell::new(0u32); + let counting = |_: &str, _: bool| { + invoked.set(invoked.get() + 1); + true + }; + + SkillPolicy::effective_with( + &global, + None, + None, + None, + &always_true, + &empty_installed, + &counting, + ) + .unwrap(); + + assert_eq!( + invoked.get(), + 0, + "skill_is_compatible callback must not run when skills are disabled" + ); + } + + #[test] + fn compatible_enabled_includes_all_when_callback_passes() { + let global = make_app_config(true, Some("alpha,beta"), Some(&["alpha", "beta"])); + + let policy = SkillPolicy::effective_with( + &global, + None, + None, + None, + &always_true, + &empty_installed, + &all_compatible, + ) + .unwrap(); + + assert_eq!(policy.compatible_enabled.len(), 2); + assert!(policy.compatible_enabled.contains("alpha")); + assert!(policy.compatible_enabled.contains("beta")); + } + + #[test] + fn compatible_enabled_excludes_incompatible_skills() { + let global = make_app_config(true, Some("alpha,beta"), Some(&["alpha", "beta"])); + let only_alpha_compat = |name: &str, _: bool| name == "alpha"; + + let policy = SkillPolicy::effective_with( + &global, + None, + None, + None, + &always_true, + &empty_installed, + &only_alpha_compat, + ) + .unwrap(); + + assert!(policy.compatible_enabled.contains("alpha")); + assert!(!policy.compatible_enabled.contains("beta")); + assert_eq!(policy.compatible_enabled.len(), 1); + } + + #[test] + fn compatible_enabled_passes_mcp_flag_to_callback() { + use std::cell::Cell; + let global = AppConfig { + skills_enabled: true, + mcp_server_support: false, + enabled_skills: Some(vec!["alpha".into()]), + visible_skills: Some(vec!["alpha".into()]), + ..AppConfig::default() + }; + let observed_mcp = Cell::new(None::); + let capture = |_: &str, mcp_on: bool| { + observed_mcp.set(Some(mcp_on)); + true + }; + + SkillPolicy::effective_with( + &global, + None, + None, + None, + &always_true, + &empty_installed, + &capture, + ) + .unwrap(); + + assert_eq!( + observed_mcp.get(), + Some(false), + "callback must receive mcp_server_support flag from AppConfig" + ); + } } diff --git a/src/config/skill_registry.rs b/src/config/skill_registry.rs index bb97089..63706c3 100644 --- a/src/config/skill_registry.rs +++ b/src/config/skill_registry.rs @@ -116,6 +116,7 @@ impl SkillRegistry { let policy = SkillPolicy { skills_enabled: true, enabled: self.loaded.keys().cloned().collect(), + compatible_enabled: self.loaded.keys().cloned().collect(), }; self.effective_role(base, &policy) } diff --git a/src/function/skill.rs b/src/function/skill.rs index a90bf46..5d0e65c 100644 --- a/src/function/skill.rs +++ b/src/function/skill.rs @@ -14,9 +14,11 @@ pub fn skill_function_declarations() -> Vec { FunctionDeclaration { name: format!("{SKILL_FUNCTION_PREFIX}list"), description: - "List skills available in this context. Returns each skill's name, description, \ - what tools and MCP servers it grants on load, and whether it is currently loaded. \ - Call this to discover skills before using skill__load." + "List skills available in this context. Call this early in any non-trivial task to \ + discover specialized skills that may apply to the work before deciding on an \ + approach. Returns each skill's name, description, what tools and MCP servers it \ + grants on load, and whether it is currently loaded. Pair with `skill__load` to \ + activate the skills you choose." .to_string(), parameters: JsonSchema { type_value: Some("object".to_string()), @@ -28,9 +30,10 @@ pub fn skill_function_declarations() -> Vec { FunctionDeclaration { name: format!("{SKILL_FUNCTION_PREFIX}load"), description: - "Load a skill module into the current context. The skill's instructions and any \ - tools or MCP servers it grants become active for subsequent turns. Call \ - skill__unload when the skill's work is complete to keep the context lean." + "Load a skill module into the current context after confirming via `skill__list` \ + that it applies to the task at hand. The skill's instructions and any tools or \ + MCP servers it grants become active for subsequent turns. Call `skill__unload` \ + when the skill's work is complete to keep the context lean." .to_string(), parameters: JsonSchema { type_value: Some("object".to_string()), @@ -102,8 +105,6 @@ pub async fn handle_skill_tool( } fn handle_list(ctx: &RequestContext, policy: &SkillPolicy) -> Result { - let mcp_on = ctx.app.config.mcp_server_support; - let visible_names: Vec = match ctx.app.config.visible_skills.as_deref() { Some(list) => list.to_vec(), None => paths::list_skills(), @@ -111,7 +112,7 @@ fn handle_list(ctx: &RequestContext, policy: &SkillPolicy) -> Result { let mut entries = Vec::new(); for name in visible_names { - if !policy.allows(&name) { + if !policy.compatible_enabled.contains(&name) { continue; } @@ -122,12 +123,6 @@ fn handle_list(ctx: &RequestContext, policy: &SkillPolicy) -> Result { continue; } }; - if !skill.is_compatible(mcp_on) { - warn!( - "Skill '{name}' filtered from list: declares MCP servers but MCP support is disabled" - ); - continue; - } entries.push(json!({ "name": skill.name(), diff --git a/src/graph/llm.rs b/src/graph/llm.rs index b433141..d25d52f 100644 --- a/src/graph/llm.rs +++ b/src/graph/llm.rs @@ -2,7 +2,10 @@ use super::state::StateManager; use super::structured; use super::types::LlmNode; use crate::client::{Model, ModelType, call_chat_completions}; -use crate::config::{Input, RequestContext, Role, RoleLike, SkillPolicy}; +use crate::config::prompts::DEFAULT_SKILL_INSTRUCTIONS; +use crate::config::{ + Input, RequestContext, Role, RoleLike, SkillPolicy, should_inject_skill_instructions, +}; use crate::function::skill::skill_function_declarations; use crate::utils::create_abort_signal; use anyhow::{Context, Error, Result, anyhow, bail}; @@ -139,6 +142,31 @@ async fn run( role.set_enabled_tools(Some(tools)); } + if should_inject_skill_instructions(&parent_ctx.app.config, &policy) { + let app = &parent_ctx.app.config; + let agent = parent_ctx.agent.as_ref(); + let inject = node + .inject_skill_instructions + .or_else(|| agent.map(|a| a.inject_skill_instructions())) + .unwrap_or(app.inject_skill_instructions); + + if inject { + let instructions = node + .skill_instructions + .clone() + .or_else(|| agent.and_then(|a| a.skill_instructions_value())) + .or_else(|| app.skill_instructions.clone()); + let separator = if role.is_empty_prompt() { "" } else { "\n\n" }; + + role.append_to_prompt(separator); + role.append_to_prompt( + instructions + .as_deref() + .unwrap_or(DEFAULT_SKILL_INSTRUCTIONS), + ); + } + } + let composed_role = parent_ctx.skill_registry.effective_role(&role, &policy); let saved_role = parent_ctx.role.clone(); @@ -456,6 +484,8 @@ mod tests { timeout: None, skills_enabled: None, enabled_skills: None, + inject_skill_instructions: None, + skill_instructions: None, } } diff --git a/src/graph/types.rs b/src/graph/types.rs index 35fd52c..b06aabf 100644 --- a/src/graph/types.rs +++ b/src/graph/types.rs @@ -37,6 +37,12 @@ pub struct Graph { #[serde(default, skip_serializing_if = "Option::is_none")] pub enabled_skills: Option>, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub inject_skill_instructions: Option, + + #[serde(default, skip_serializing_if = "Option::is_none")] + pub skill_instructions: Option, + #[serde(default)] pub conversation_starters: Vec, @@ -305,6 +311,12 @@ pub struct LlmNode { #[serde(default, skip_serializing_if = "Option::is_none")] pub enabled_skills: Option>, + + #[serde(default, skip_serializing_if = "Option::is_none")] + pub inject_skill_instructions: Option, + + #[serde(default, skip_serializing_if = "Option::is_none")] + pub skill_instructions: Option, } fn default_llm_max_attempts() -> u32 { diff --git a/src/graph/validator.rs b/src/graph/validator.rs index 4580a98..c04309c 100644 --- a/src/graph/validator.rs +++ b/src/graph/validator.rs @@ -950,6 +950,8 @@ mod tests { mcp_servers: Vec::new(), skills_enabled: None, enabled_skills: None, + inject_skill_instructions: None, + skill_instructions: None, conversation_starters: Vec::new(), variables: Vec::new(), settings: GraphSettings::default(), @@ -1051,6 +1053,8 @@ mod tests { timeout: None, skills_enabled: None, enabled_skills: None, + inject_skill_instructions: None, + skill_instructions: None, }), next: next.map(NextTargets::from), }