From 6b5347d9038ca07b47a93f4e02c57573aa43c9fa Mon Sep 17 00:00:00 2001 From: Stefan Obradovic Date: Thu, 12 Mar 2026 01:08:25 +1000 Subject: [PATCH] fix: prevent deletion of instance-level skills via agent API SkillSet::remove() now rejects instance-level skills with an error instead of deleting them from disk. The API endpoint returns 403 for this case. Instance skills are shared across all agents and should not be removable through the per-agent endpoint. Also fixes pre-existing clippy collapsible_if lint in signal adapter. Closes #365 --- src/api/skills.rs | 10 ++- src/skills.rs | 91 +++++++++++++++++++- src/tools/send_message_to_another_channel.rs | 54 ++++++------ 3 files changed, 123 insertions(+), 32 deletions(-) diff --git a/src/api/skills.rs b/src/api/skills.rs index de9e75853..506216d18 100644 --- a/src/api/skills.rs +++ b/src/api/skills.rs @@ -252,8 +252,14 @@ pub(super) async fn remove_skill( crate::skills::SkillSet::load(&instance_skills_dir, &workspace_skills_dir).await; let removed_path = skills.remove(&req.name).await.map_err(|error| { - tracing::warn!(%error, skill = %req.name, "failed to remove skill"); - StatusCode::INTERNAL_SERVER_ERROR + let msg = error.to_string(); + if msg.contains("instance-level skill") { + tracing::warn!(skill = %req.name, "rejected removal of instance-level skill"); + StatusCode::FORBIDDEN + } else { + tracing::warn!(%error, skill = %req.name, "failed to remove skill"); + StatusCode::INTERNAL_SERVER_ERROR + } })?; state.send_event(ApiEvent::ConfigReloaded); diff --git a/src/skills.rs b/src/skills.rs index 33136a429..b0c64c3f4 100644 --- a/src/skills.rs +++ b/src/skills.rs @@ -174,13 +174,29 @@ impl SkillSet { /// Remove a skill by name. /// + /// Only workspace-level skills can be removed via this method. Instance-level + /// skills are shared across all agents and must not be deleted through the + /// per-agent API. + /// /// Returns the base directory path if the skill was found and removed. pub async fn remove(&mut self, name: &str) -> anyhow::Result> { - let skill = match self.skills.remove(&name.to_lowercase()) { + let key = name.to_lowercase(); + let skill = match self.skills.get(&key) { Some(s) => s, None => return Ok(None), }; + if skill.source == SkillSource::Instance { + anyhow::bail!( + "cannot remove instance-level skill '{}' via the agent API; \ + instance skills are shared across all agents and must be \ + removed from the instance skills directory directly", + name + ); + } + + let skill = self.skills.remove(&key).unwrap(); + // Remove the skill directory from disk if skill.base_dir.exists() { tokio::fs::remove_dir_all(&skill.base_dir) @@ -516,4 +532,77 @@ mod tests { let prompt = empty_set.render_worker_skills(&[], &engine).unwrap(); assert!(prompt.is_empty()); } + + fn make_skill(name: &str, source: SkillSource) -> Skill { + Skill { + name: name.into(), + description: format!("{name} skill"), + file_path: PathBuf::from(format!("/skills/{name}/SKILL.md")), + base_dir: PathBuf::from(format!("/tmp/test-skills-{}", uuid::Uuid::new_v4())), + content: format!("# {name}"), + source, + source_repo: None, + } + } + + #[tokio::test] + async fn remove_instance_skill_is_rejected() { + let mut set = SkillSet::default(); + set.skills.insert( + "my-skill".into(), + make_skill("my-skill", SkillSource::Instance), + ); + + let result = set.remove("my-skill").await; + assert!(result.is_err()); + let msg = result.unwrap_err().to_string(); + assert!( + msg.contains("instance-level skill"), + "unexpected error: {msg}" + ); + + // Skill should still be in the set (not removed) + assert!(set.skills.contains_key("my-skill")); + } + + #[tokio::test] + async fn remove_workspace_skill_succeeds() { + let mut set = SkillSet::default(); + let skill = make_skill("my-skill", SkillSource::Workspace); + // Don't create the directory on disk - remove should still return the path + let expected_dir = skill.base_dir.clone(); + set.skills.insert("my-skill".into(), skill); + + let result = set.remove("my-skill").await; + assert!(result.is_ok()); + let path = result.unwrap(); + assert_eq!(path, Some(expected_dir)); + assert!(!set.skills.contains_key("my-skill")); + } + + #[tokio::test] + async fn remove_nonexistent_skill_returns_none() { + let mut set = SkillSet::default(); + let result = set.remove("nonexistent").await; + assert!(result.is_ok()); + assert!(result.unwrap().is_none()); + } + + #[tokio::test] + async fn remove_is_case_insensitive() { + let mut set = SkillSet::default(); + set.skills.insert( + "my-skill".into(), + make_skill("my-skill", SkillSource::Instance), + ); + + let result = set.remove("MY-SKILL").await; + assert!(result.is_err()); + assert!( + result + .unwrap_err() + .to_string() + .contains("instance-level skill") + ); + } } diff --git a/src/tools/send_message_to_another_channel.rs b/src/tools/send_message_to_another_channel.rs index abf72e239..bb0681fab 100644 --- a/src/tools/send_message_to_another_channel.rs +++ b/src/tools/send_message_to_another_channel.rs @@ -150,14 +150,13 @@ impl Tool for SendMessageTool { // If explicit prefix returned default "signal" adapter but we're in a named // Signal adapter conversation (e.g., signal:gvoice1), use the current adapter // to ensure the message goes through the correct account. - if target.adapter == "signal" { - if let Some(current_adapter) = self + if target.adapter == "signal" + && let Some(current_adapter) = self .current_adapter .as_ref() .filter(|adapter| adapter.starts_with("signal:")) - { - target.adapter = current_adapter.clone(); - } + { + target.adapter = current_adapter.clone(); } self.messaging_manager @@ -189,31 +188,28 @@ impl Tool for SendMessageTool { .current_adapter .as_ref() .filter(|adapter| adapter.starts_with("signal")) + && let Some(target) = parse_implicit_signal_shorthand(&args.target, current_adapter) { - if let Some(target) = parse_implicit_signal_shorthand(&args.target, current_adapter) { - self.messaging_manager - .broadcast( - &target.adapter, - &target.target, - crate::OutboundResponse::Text(args.message), - ) - .await - .map_err(|error| { - SendMessageError(format!("failed to send message: {error}")) - })?; - - tracing::info!( - adapter = %target.adapter, - broadcast_target = %"[REDACTED]", - "message sent via implicit Signal shorthand" - ); - - return Ok(SendMessageOutput { - success: true, - target: target.target, - platform: target.adapter, - }); - } + self.messaging_manager + .broadcast( + &target.adapter, + &target.target, + crate::OutboundResponse::Text(args.message), + ) + .await + .map_err(|error| SendMessageError(format!("failed to send message: {error}")))?; + + tracing::info!( + adapter = %target.adapter, + broadcast_target = %"[REDACTED]", + "message sent via implicit Signal shorthand" + ); + + return Ok(SendMessageOutput { + success: true, + target: target.target, + platform: target.adapter, + }); } // Check for explicit email target