fix: effective_policy unconditionally overwrote skill values for role-like structs

This commit is contained in:
2026-06-03 14:54:42 -06:00
parent 3ee80fafe5
commit a5eb19c85f
3 changed files with 43 additions and 15 deletions
+9 -1
View File
@@ -627,7 +627,15 @@ impl RequestContext {
} }
} }
self.skill_registry.effective_role(&role) match 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,
}
} }
pub fn auto_continue_config(&self) -> AutoContinueConfig { pub fn auto_continue_config(&self) -> AutoContinueConfig {
+24 -12
View File
@@ -1,5 +1,6 @@
use super::role::{Role, RoleLike}; use super::role::{Role, RoleLike};
use super::skill::Skill; use super::skill::Skill;
use super::skill_policy::SkillPolicy;
use anyhow::{Result, bail}; use anyhow::{Result, bail};
use indexmap::IndexMap; use indexmap::IndexMap;
@@ -58,8 +59,8 @@ impl SkillRegistry {
self.loaded.retain(|_, skill| !skill.auto_unload()); self.loaded.retain(|_, skill| !skill.auto_unload());
} }
pub fn effective_role(&self, base: &Role) -> Role { pub fn effective_role(&self, base: &Role, policy: &SkillPolicy) -> Role {
if self.loaded.is_empty() { if !policy.skills_enabled || self.loaded.is_empty() {
return base.clone(); return base.clone();
} }
@@ -78,7 +79,10 @@ impl SkillRegistry {
.map(|v| v.into_iter().collect()) .map(|v| v.into_iter().collect())
.unwrap_or_default(); .unwrap_or_default();
for (_, skill) in &self.loaded { for (name, skill) in &self.loaded {
if !policy.allows(name) {
continue;
}
if let Some(skill_tools) = skill.enabled_tools() { if let Some(skill_tools) = skill.enabled_tools() {
tools.extend(skill_tools.iter().cloned()); tools.extend(skill_tools.iter().cloned());
} }
@@ -113,6 +117,14 @@ impl SkillRegistry {
fn insert_for_test(&mut self, skill: Skill) { fn insert_for_test(&mut self, skill: Skill) {
self.loaded.insert(skill.name().to_string(), skill); self.loaded.insert(skill.name().to_string(), skill);
} }
fn effective_role_for_test(&self, base: &Role) -> Role {
let policy = SkillPolicy {
skills_enabled: true,
enabled: self.loaded.keys().cloned().collect(),
};
self.effective_role(base, &policy)
}
} }
#[cfg(test)] #[cfg(test)]
@@ -133,7 +145,7 @@ mod tests {
let base = Role::new("test", "You are a helper"); let base = Role::new("test", "You are a helper");
let registry = SkillRegistry::default(); let registry = SkillRegistry::default();
let effective = registry.effective_role(&base); let effective = registry.effective_role_for_test(&base);
assert_eq!(effective.prompt(), base.prompt()); assert_eq!(effective.prompt(), base.prompt());
} }
@@ -144,7 +156,7 @@ mod tests {
registry.insert_for_test(make_skill("git-master", "description: D", "Git knowledge")); registry.insert_for_test(make_skill("git-master", "description: D", "Git knowledge"));
let base = Role::new("test", "You are a helper"); let base = Role::new("test", "You are a helper");
let effective = registry.effective_role(&base); let effective = registry.effective_role_for_test(&base);
assert_eq!(effective.prompt(), "You are a helper\n\nGit knowledge"); assert_eq!(effective.prompt(), "You are a helper\n\nGit knowledge");
} }
@@ -156,7 +168,7 @@ mod tests {
registry.insert_for_test(make_skill("b", "", "Beta body")); registry.insert_for_test(make_skill("b", "", "Beta body"));
let base = Role::new("test", "Base"); let base = Role::new("test", "Base");
let effective = registry.effective_role(&base); let effective = registry.effective_role_for_test(&base);
assert_eq!(effective.prompt(), "Base\n\nAlpha body\n\nBeta body"); assert_eq!(effective.prompt(), "Base\n\nAlpha body\n\nBeta body");
} }
@@ -168,7 +180,7 @@ mod tests {
registry.insert_for_test(make_skill("b", "", "Beta")); registry.insert_for_test(make_skill("b", "", "Beta"));
let base = Role::new("test", ""); let base = Role::new("test", "");
let effective = registry.effective_role(&base); let effective = registry.effective_role_for_test(&base);
assert_eq!(effective.prompt(), "Alpha\n\nBeta"); assert_eq!(effective.prompt(), "Alpha\n\nBeta");
} }
@@ -183,7 +195,7 @@ mod tests {
)); ));
let base = Role::new("test", "Process: __INPUT__"); let base = Role::new("test", "Process: __INPUT__");
let effective = registry.effective_role(&base); let effective = registry.effective_role_for_test(&base);
assert_eq!(effective.prompt(), "Process: __INPUT__"); assert_eq!(effective.prompt(), "Process: __INPUT__");
let tools = effective.enabled_tools().expect("tools set by skill"); let tools = effective.enabled_tools().expect("tools set by skill");
@@ -196,7 +208,7 @@ mod tests {
registry.insert_for_test(make_skill("knowledge", "enabled_tools: fs", "")); registry.insert_for_test(make_skill("knowledge", "enabled_tools: fs", ""));
let base = Role::new("test", "Base"); let base = Role::new("test", "Base");
let effective = registry.effective_role(&base); let effective = registry.effective_role_for_test(&base);
assert_eq!(effective.prompt(), "Base"); assert_eq!(effective.prompt(), "Base");
} }
@@ -218,7 +230,7 @@ mod tests {
let mut base = Role::new("test", "body"); let mut base = Role::new("test", "body");
base.set_enabled_tools(Some(vec!["web_search".to_string()])); base.set_enabled_tools(Some(vec!["web_search".to_string()]));
let effective = registry.effective_role(&base); let effective = registry.effective_role_for_test(&base);
let tools_vec = effective.enabled_tools().unwrap(); let tools_vec = effective.enabled_tools().unwrap();
let tools: BTreeSet<&str> = tools_vec.iter().map(|s| s.as_str()).collect(); let tools: BTreeSet<&str> = tools_vec.iter().map(|s| s.as_str()).collect();
@@ -235,7 +247,7 @@ mod tests {
registry.insert_for_test(make_skill("knowledge", "", "Pure knowledge")); registry.insert_for_test(make_skill("knowledge", "", "Pure knowledge"));
let base = Role::new("test", "Base"); let base = Role::new("test", "Base");
let effective = registry.effective_role(&base); let effective = registry.effective_role_for_test(&base);
assert!(effective.enabled_tools().is_none()); assert!(effective.enabled_tools().is_none());
assert!(effective.enabled_mcp_servers().is_none()); assert!(effective.enabled_mcp_servers().is_none());
@@ -248,7 +260,7 @@ mod tests {
let mut base = Role::new("test", "Base"); let mut base = Role::new("test", "Base");
base.set_enabled_tools(Some(Vec::new())); base.set_enabled_tools(Some(Vec::new()));
let effective = registry.effective_role(&base); let effective = registry.effective_role_for_test(&base);
assert_eq!(effective.enabled_tools().as_deref(), Some([].as_slice())); assert_eq!(effective.enabled_tools().as_deref(), Some([].as_slice()));
} }
+10 -2
View File
@@ -2,7 +2,7 @@ use super::state::StateManager;
use super::structured; use super::structured;
use super::types::LlmNode; use super::types::LlmNode;
use crate::client::{Model, ModelType, call_chat_completions}; use crate::client::{Model, ModelType, call_chat_completions};
use crate::config::{Input, RequestContext, Role, RoleLike}; use crate::config::{Input, RequestContext, Role, RoleLike, SkillPolicy};
use crate::utils::create_abort_signal; use crate::utils::create_abort_signal;
use anyhow::{Context, Error, Result, anyhow, bail}; use anyhow::{Context, Error, Result, anyhow, bail};
use serde_json::Value; use serde_json::Value;
@@ -115,7 +115,15 @@ async fn run(
let saved_agent_skill_state = swap_in_node_skill_policy(node, parent_ctx); let saved_agent_skill_state = swap_in_node_skill_policy(node, parent_ctx);
let composed_role = parent_ctx.skill_registry.effective_role(&role); let composed_role = match 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 saved_role = parent_ctx.role.clone(); let saved_role = parent_ctx.role.clone();
parent_ctx.role = Some(composed_role); parent_ctx.role = Some(composed_role);