From fd1b4e6f3fd06ffa3ab15e4e36447ff20c2a960c Mon Sep 17 00:00:00 2001 From: obchain <167975049+obchain@users.noreply.github.com> Date: Mon, 22 Jun 2026 23:47:30 +0530 Subject: [PATCH] feat(skills): retract uninstalled skills from mid-session catalogue (#3745) --- .../agent/harness/session/builder/setters.rs | 1 + src/openhuman/agent/harness/session/tests.rs | 106 ++++++++++++++++++ .../agent/harness/session/turn/core.rs | 12 +- .../agent/harness/session/turn/mod.rs | 16 +++ .../agent/harness/session/turn/tools.rs | 43 +++++-- .../agent/harness/session/turn_tests.rs | 23 ++++ src/openhuman/agent/harness/session/types.rs | 7 ++ 7 files changed, 200 insertions(+), 8 deletions(-) diff --git a/src/openhuman/agent/harness/session/builder/setters.rs b/src/openhuman/agent/harness/session/builder/setters.rs index a89f19c9d..996e05cce 100644 --- a/src/openhuman/agent/harness/session/builder/setters.rs +++ b/src/openhuman/agent/harness/session/builder/setters.rs @@ -589,6 +589,7 @@ impl AgentBuilder { pending_mcp_announcement: Vec::new(), announced_skills: std::collections::HashSet::new(), pending_skill_announcement: Vec::new(), + pending_skill_retraction: Vec::new(), archivist_hook: self.archivist_hook, synthesized_tool_names: std::collections::HashSet::new(), pending_synthesized_tools_mask: std::collections::HashSet::new(), diff --git a/src/openhuman/agent/harness/session/tests.rs b/src/openhuman/agent/harness/session/tests.rs index 2076a89e7..e7637a7e7 100644 --- a/src/openhuman/agent/harness/session/tests.rs +++ b/src/openhuman/agent/harness/session/tests.rs @@ -533,6 +533,112 @@ fn refresh_workflows_picks_up_skill_installed_on_disk() { ); } +#[test] +fn refresh_workflows_retracts_skill_removed_from_disk() { + use crate::openhuman::workflows::ops_types::{SKILL_MD, TRUST_MARKER}; + + let ws = tempfile::TempDir::new().expect("temp workspace"); + let wsp = ws.path().to_path_buf(); + std::fs::create_dir_all(wsp.join(".openhuman")).unwrap(); + std::fs::write(wsp.join(".openhuman").join(TRUST_MARKER), "").unwrap(); + + // Write a skill to disk. + let skill_dir = wsp + .join(".openhuman") + .join("skills") + .join("zz-retract-test"); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write( + skill_dir.join(SKILL_MD), + "---\nname: zz-retract-test\ndescription: a retraction test skill\n---\n# body\n", + ) + .unwrap(); + + let memory_cfg = crate::openhuman::config::MemoryConfig { + backend: "none".into(), + ..crate::openhuman::config::MemoryConfig::default() + }; + let mem: Arc = + Arc::from(crate::openhuman::memory_store::create_memory(&memory_cfg, &wsp).unwrap()); + let provider = Box::new(MockProvider { + responses: Mutex::new(vec![]), + }); + let mut agent = Agent::builder() + .provider(provider) + .tools(vec![Box::new(MockTool)]) + .memory(mem) + .tool_dispatcher(Box::new(NativeToolDispatcher)) + .workspace_dir(wsp.clone()) + .build() + .expect("agent build should succeed"); + + // First refresh: picks up the installed skill. + assert!(agent.refresh_workflows("test-install")); + assert!( + agent + .test_workflow_ids() + .iter() + .any(|id| id == "zz-retract-test"), + "skill should be in catalogue after first refresh" + ); + assert!( + agent + .test_pending_skill_announcement() + .iter() + .any(|id| id == "zz-retract-test"), + "skill should be parked for announcement" + ); + // Now remove the skill from disk. + std::fs::remove_dir_all(&skill_dir).unwrap(); + + // Second refresh: detects the removal, parks the retraction. + assert!( + agent.refresh_workflows("test-remove"), + "removing a skill should change the set" + ); + assert!( + !agent + .test_workflow_ids() + .iter() + .any(|id| id == "zz-retract-test"), + "skill should be gone from catalogue after removal" + ); + assert!( + agent + .test_pending_skill_retraction() + .iter() + .any(|id| id == "zz-retract-test"), + "removed skill should be parked for retraction" + ); + // Retraction should have cleared it from announced_skills; re-install will + // be announced fresh (not silently re-added). Verify by re-adding the skill + // and confirming it gets announced again. + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write( + skill_dir.join(SKILL_MD), + "---\nname: zz-retract-test\ndescription: a retraction test skill\n---\n# body\n", + ) + .unwrap(); + assert!(agent.refresh_workflows("test-reinstall")); + assert!( + agent + .test_pending_skill_announcement() + .iter() + .any(|id| id == "zz-retract-test"), + "re-installed skill should be announced again after retraction cleared it from announced set" + ); + // Re-install must also cancel the still-pending retraction so the user turn + // never carries a contradictory "installed" + "retracted" pair for the same + // skill. + assert!( + !agent + .test_pending_skill_retraction() + .iter() + .any(|id| id == "zz-retract-test"), + "re-install should cancel the pending retraction for the same skill" + ); +} + #[tokio::test] async fn turn_without_tools_returns_text() { let workspace = tempfile::TempDir::new().expect("temp workspace"); diff --git a/src/openhuman/agent/harness/session/turn/core.rs b/src/openhuman/agent/harness/session/turn/core.rs index ddbeb21ea..2a8b5b4ea 100644 --- a/src/openhuman/agent/harness/session/turn/core.rs +++ b/src/openhuman/agent/harness/session/turn/core.rs @@ -5,7 +5,7 @@ use super::super::turn_engine_adapter::{AgentCheckpoint, AgentObserver, AgentToo use super::super::types::Agent; use super::{ integration_announcement_note, mcp_announcement_note, newly_connected_slugs, - skill_announcement_note, + skill_announcement_note, skill_retraction_note, }; use crate::openhuman::agent::harness; use crate::openhuman::agent::harness::definition::TriggerMemoryAgent; @@ -388,6 +388,16 @@ impl Agent { None => enriched, }; + // Same one-shot treatment for skills uninstalled mid-session (parked by + // `refresh_workflows`). The model must know the skill is gone so it does + // not attempt `run_skill` on a removed entry. Rides the user turn for + // the same KV-cache reason as the install note above. + let pending_retracted = std::mem::take(&mut self.pending_skill_retraction); + let enriched = match skill_retraction_note(&pending_retracted) { + Some(note) => format!("{note}\n\n{enriched}"), + None => enriched, + }; + // Pin the main agent to its configured model for the lifetime of // the session. Per-turn classification used to run here, but it // would flip `effective_model` mid-conversation (e.g. reasoning → diff --git a/src/openhuman/agent/harness/session/turn/mod.rs b/src/openhuman/agent/harness/session/turn/mod.rs index 04ae83850..47625d919 100644 --- a/src/openhuman/agent/harness/session/turn/mod.rs +++ b/src/openhuman/agent/harness/session/turn/mod.rs @@ -136,6 +136,22 @@ They are in your `## Installed Skills` list — run one with `run_skill` immedia )) } +/// One-shot note prepended to the next user turn when skills are uninstalled +/// mid-session. Symmetric to [`skill_announcement_note`]: tells the model the +/// listed skills are no longer present and `run_skill` will fail for them, so +/// it does not attempt to invoke them. Rides the user turn (not the system +/// prompt) to keep the KV-cache prefix stable. +pub(super) fn skill_retraction_note(skill_ids: &[String]) -> Option { + if skill_ids.is_empty() { + return None; + } + Some(format!( + "[skills retracted] These skill(s) were uninstalled during this conversation and are no longer available: {}. \ +Do not attempt to run them with `run_skill` — they have been removed. Tell the user to reinstall if they want to use them again.", + skill_ids.join(", ") + )) +} + /// Wrapper around /// [`crate::openhuman::memory_tree::tree_runtime::store::collect_root_summaries_with_caps`] /// that takes user-resolved per-namespace and total caps. The actual diff --git a/src/openhuman/agent/harness/session/turn/tools.rs b/src/openhuman/agent/harness/session/turn/tools.rs index e1dc4fd2d..97703c2a8 100644 --- a/src/openhuman/agent/harness/session/turn/tools.rs +++ b/src/openhuman/agent/harness/session/turn/tools.rs @@ -391,27 +391,49 @@ impl Agent { return false; } // Newly-present skills (on disk now, absent from the prior snapshot), - // announced at most once this session. Removals are detected by the - // id-set diff above but are NOT yet retracted from the frozen - // catalogue (symmetric to the integration/MCP announcements, which - // also only announce additions) — tracked in - // tinyhumansai/openhuman#3738. + // announced at most once this session. let newly: Vec = latest_ids .difference(¤t_ids) .filter(|id| self.announced_skills.insert((*id).clone())) .cloned() .collect(); + // Skills removed from disk since the last snapshot: retract them so the + // model stops routing `run_skill` calls to skills that no longer exist. + // The frozen `## Installed Skills` system-prompt block cannot be updated + // mid-session (KV-cache stability), so the retraction note on the user + // turn is the only signal the model gets — mirrors the install path. + // Clear from `announced_skills` so a re-install later is announced fresh. + let removed: Vec = current_ids.difference(&latest_ids).cloned().collect(); + for id in &removed { + self.announced_skills.remove(id); + } log::info!( - "[agent_loop] installed-skills set changed ({trigger}): {} -> {} skills; updating tracked set + parking announcement (system-prompt catalogue is frozen mid-session; the user-turn note surfaces the change)", + "[agent_loop] installed-skills set changed ({trigger}): {} -> {} skills (new={} removed={}); updating tracked set + parking notes (system-prompt catalogue frozen for KV cache)", self.workflows.len(), - latest.len() + latest.len(), + newly.len(), + removed.len(), ); self.workflows = latest; for id in newly { + // A re-install after a still-pending retraction cancels the + // retraction: the skill is present again, so drop the stale "gone" + // note and announce it instead. + self.pending_skill_retraction.retain(|p| p != &id); if !self.pending_skill_announcement.contains(&id) { self.pending_skill_announcement.push(id); } } + for id in removed { + // If the skill was installed and uninstalled before its + // announcement ever surfaced, the model never saw it as available — + // drop the pending announcement so we don't emit a contradictory + // "installed" + "retracted" pair on the same user turn. + self.pending_skill_announcement.retain(|p| p != &id); + if !self.pending_skill_retraction.contains(&id) { + self.pending_skill_retraction.push(id); + } + } true } @@ -439,6 +461,13 @@ impl Agent { &self.pending_skill_announcement } + /// Test-only: skill ids parked for the next-turn `[skills retracted]` + /// retraction note by `refresh_workflows`. + #[cfg(test)] + pub(in super::super) fn test_pending_skill_retraction(&self) -> &[String] { + &self.pending_skill_retraction + } + /// Test-only: inject a specific skill-events receiver (e.g. one whose /// sender has been dropped) so `drain_skill_events`' `Closed` arm is /// reachable without the global bus singleton. diff --git a/src/openhuman/agent/harness/session/turn_tests.rs b/src/openhuman/agent/harness/session/turn_tests.rs index f06b126e9..5bcda3bba 100644 --- a/src/openhuman/agent/harness/session/turn_tests.rs +++ b/src/openhuman/agent/harness/session/turn_tests.rs @@ -1889,3 +1889,26 @@ fn skill_announcement_note_mentions_ids_and_run_skill() { "note must steer the model to run_skill: {note}" ); } + +#[test] +fn skill_retraction_note_empty_yields_none() { + assert!(super::skill_retraction_note(&[]).is_none()); +} + +#[test] +fn skill_retraction_note_names_removed_skills_and_warns_against_run_skill() { + let note = + super::skill_retraction_note(&["ascii-art".to_string(), "github-issues".to_string()]) + .expect("non-empty input should yield a note"); + assert!(note.contains("[skills retracted]")); + assert!(note.contains("ascii-art")); + assert!(note.contains("github-issues")); + assert!( + note.contains("run_skill"), + "note must mention run_skill so the model knows not to invoke it: {note}" + ); + assert!( + !note.contains("[skills update]"), + "retraction note must not look like an install announcement: {note}" + ); +} diff --git a/src/openhuman/agent/harness/session/types.rs b/src/openhuman/agent/harness/session/types.rs index fa9c54e0c..c950ff879 100644 --- a/src/openhuman/agent/harness/session/types.rs +++ b/src/openhuman/agent/harness/session/types.rs @@ -245,6 +245,13 @@ pub struct Agent { /// next user message is built so the note rides the user turn (NOT the /// system prompt) and the KV-cache prefix stays byte-identical. pub(super) pending_skill_announcement: Vec, + /// Skill ids removed mid-session (uninstalled after session build) that + /// still need retracting on the next user message. Symmetric to + /// [`Self::pending_skill_announcement`]: parked by `refresh_workflows`, + /// rendered + cleared when the next user message is built so the retraction + /// note rides the user turn (NOT the system prompt) and the KV-cache prefix + /// stays byte-identical. + pub(super) pending_skill_retraction: Vec, /// Skill ids already surfaced to the model as installed this session, so /// each newly-installed skill is announced exactly once and never /// re-announced per turn. Seeded from the session-build catalogue.