From d6d97d037cb39f198e17cffce85c39ba0d51c211 Mon Sep 17 00:00:00 2001 From: Alessandro <155005371+3clyp50@users.noreply.github.com> Date: Sat, 2 May 2026 20:14:49 +0200 Subject: [PATCH] Fix skills selector unloading Remove dynamically loaded skills when they are deactivated from the Skills selector. Treat skill names and paths as aliases so scoped defaults, chat overrides, and loaded-skill state resolve consistently. --- helpers/skills.py | 83 +++++++++++++++++++++++---- plugins/_skills/api/skills_catalog.py | 11 +--- tests/test_skills_runtime.py | 73 +++++++++++++++++++++++ tools/skills_tool.py | 2 +- 4 files changed, 148 insertions(+), 21 deletions(-) diff --git a/helpers/skills.py b/helpers/skills.py index ab713f5ac..76cd77722 100644 --- a/helpers/skills.py +++ b/helpers/skills.py @@ -20,6 +20,7 @@ except Exception: # pragma: no cover MAX_ACTIVE_SKILLS = 20 ACTIVE_SKILLS_PLUGIN_NAME = "_skills" +AGENT_DATA_NAME_LOADED_SKILLS = "loaded_skills" CONTEXT_DATA_NAME_CHAT_ACTIVE_SKILLS = "skills_chat_active" CONTEXT_DATA_NAME_CHAT_DISABLED_SKILLS = "skills_chat_disabled" @@ -703,6 +704,48 @@ def get_active_skills(agent: Agent | None) -> list[ActiveSkillEntry]: return _build_active_skills(agent, limit=get_max_active_skills()) +def get_loaded_skill_entries(agent: Agent | None) -> list[ActiveSkillEntry]: + if not agent: + return [] + + loaded = getattr(agent, "data", {}).get(AGENT_DATA_NAME_LOADED_SKILLS) + if not isinstance(loaded, list): + return [] + + return [ + {"name": str(skill_name).strip()} + for skill_name in loaded + if str(skill_name).strip() + ] + + +def unload_agent_skill(agent: Agent | None, entry: Any) -> bool: + normalized = _normalize_active_skill_entry(entry) + if not agent or not normalized: + return False + + data = getattr(agent, "data", None) + if not isinstance(data, dict): + return False + + loaded = data.get(AGENT_DATA_NAME_LOADED_SKILLS) + if not isinstance(loaded, list): + return False + + next_loaded: list[str] = [] + removed = False + for skill_name in loaded: + loaded_entry = _normalize_active_skill_entry(str(skill_name)) + if loaded_entry and _entries_match(loaded_entry, normalized): + removed = True + continue + next_loaded.append(skill_name) + + if removed: + data[AGENT_DATA_NAME_LOADED_SKILLS] = next_loaded + return removed + + def activate_chat_skill(agent: Agent, entry: Any) -> list[ActiveSkillEntry]: normalized = _normalize_active_skill_entry(entry) if not normalized: @@ -712,18 +755,19 @@ def activate_chat_skill(agent: Agent, entry: Any) -> list[ActiveSkillEntry]: if not context: raise ValueError("A chat context is required.") - key = _entry_key(normalized) scope_entries = get_scope_active_skills(agent) chat_entries = [ - item for item in get_chat_active_skills(context) if _entry_key(item) != key + item + for item in get_chat_active_skills(context) + if not _entries_match(item, normalized) ] disabled_entries = [ item for item in get_chat_disabled_skills(context) - if _entry_key(item) != key + if not _entries_match(item, normalized) ] - if not any(_entry_key(item) == key for item in scope_entries): + if not any(_entries_match(item, normalized) for item in scope_entries): chat_entries.append(normalized) merged_entries = _build_active_skills( @@ -759,18 +803,19 @@ def deactivate_chat_skill(agent: Agent, entry: Any) -> list[ActiveSkillEntry]: if not context: raise ValueError("A chat context is required.") - key = _entry_key(normalized) chat_entries = [ - item for item in get_chat_active_skills(context) if _entry_key(item) != key + item + for item in get_chat_active_skills(context) + if not _entries_match(item, normalized) ] disabled_entries = [ item for item in get_chat_disabled_skills(context) - if _entry_key(item) != key + if not _entries_match(item, normalized) ] is_scope_default = any( - _entry_key(item) == key for item in get_scope_active_skills(agent) + _entries_match(item, normalized) for item in get_scope_active_skills(agent) ) if is_scope_default: disabled_entries.append(normalized) @@ -877,6 +922,19 @@ def _entry_key(entry: ActiveSkillEntry) -> str: return str(entry.get("path") or entry.get("name") or "").strip().lower() +def _entry_keys(entry: ActiveSkillEntry) -> set[str]: + keys: set[str] = set() + for value in (entry.get("path"), entry.get("name")): + key = str(value or "").strip().lower() + if key: + keys.add(key) + return keys + + +def _entries_match(left: ActiveSkillEntry, right: ActiveSkillEntry) -> bool: + return bool(_entry_keys(left) & _entry_keys(right)) + + def _get_agent_project_name(agent: Agent | None) -> str: context = getattr(agent, "context", None) if not context: @@ -925,14 +983,17 @@ def _merge_active_skill_entries( ) -> list[ActiveSkillEntry]: merged: list[ActiveSkillEntry] = [] seen: set[str] = set() - disabled_keys = {_entry_key(entry) for entry in disabled_entries if _entry_key(entry)} + disabled_keys = { + key for entry in disabled_entries for key in _entry_keys(entry) + } for entry in [*scope_entries, *dynamic_entries]: + keys = _entry_keys(entry) key = _entry_key(entry) - if not key or key in seen or key in disabled_keys: + if not key or keys & seen or keys & disabled_keys: continue - seen.add(key) + seen.update(keys) merged.append(entry) if limit is not None and limit >= 0 and len(merged) >= limit: break diff --git a/plugins/_skills/api/skills_catalog.py b/plugins/_skills/api/skills_catalog.py index 24d87b3dc..04ceb9008 100644 --- a/plugins/_skills/api/skills_catalog.py +++ b/plugins/_skills/api/skills_catalog.py @@ -41,6 +41,7 @@ class SkillsCatalog(ApiHandler): context = self._require_context(context_id) skill_entry = self._require_skill_entry(input) skills.deactivate_chat_skill(context.get_agent(), skill_entry) + skills.unload_agent_skill(context.get_agent(), skill_entry) save_tmp_chat(context) return self._build_state(context_id=context.id) @@ -198,15 +199,7 @@ class SkillsCatalog(ApiHandler): if not agent: return [] - loaded = getattr(agent, "data", {}).get("loaded_skills") - if not isinstance(loaded, list): - return [] - - return [ - {"name": str(skill_name).strip()} - for skill_name in loaded - if str(skill_name).strip() - ] + return skills.get_loaded_skill_entries(agent) def _merge_entries( self, diff --git a/tests/test_skills_runtime.py b/tests/test_skills_runtime.py index 009d20803..8a56906fa 100644 --- a/tests/test_skills_runtime.py +++ b/tests/test_skills_runtime.py @@ -136,6 +136,79 @@ def test_chat_activation_can_override_scope_defaults(monkeypatch): assert runtime.get_chat_disabled_skills(agent.context) == [] +def test_chat_deactivation_hides_name_only_scope_default_by_path(monkeypatch): + monkeypatch.setattr( + runtime.plugin_helpers, + "get_plugin_config", + lambda *args, **kwargs: _scope_config([{"name": "Pinned"}]), + ) + agent = DummyAgent() + + runtime.deactivate_chat_skill( + agent, + {"name": "Pinned", "path": "/a0/usr/skills/custom/pinned"}, + ) + + assert runtime.get_active_skills(agent) == [] + assert runtime.get_chat_disabled_skills(agent.context) == [ + {"name": "Pinned", "path": "/a0/usr/skills/custom/pinned"} + ] + + +def test_reactivating_name_only_scope_default_by_path_clears_hidden_override(monkeypatch): + monkeypatch.setattr( + runtime.plugin_helpers, + "get_plugin_config", + lambda *args, **kwargs: _scope_config([{"name": "Pinned"}]), + ) + agent = DummyAgent() + + runtime.deactivate_chat_skill( + agent, + {"name": "Pinned", "path": "/a0/usr/skills/custom/pinned"}, + ) + runtime.activate_chat_skill( + agent, + {"name": "Pinned", "path": "/a0/usr/skills/custom/pinned"}, + ) + + assert runtime.get_active_skills(agent) == [{"name": "Pinned"}] + assert runtime.get_chat_active_skills(agent.context) == [] + assert runtime.get_chat_disabled_skills(agent.context) == [] + + +def test_loaded_skill_entries_come_from_agent_data(): + agent = DummyAgent() + agent.data[runtime.AGENT_DATA_NAME_LOADED_SKILLS] = [ + "computer-use-remote", + "", + "a0-development", + ] + + assert runtime.get_loaded_skill_entries(agent) == [ + {"name": "computer-use-remote"}, + {"name": "a0-development"}, + ] + + +def test_unload_agent_skill_removes_loaded_skill_by_name(): + agent = DummyAgent() + agent.data[runtime.AGENT_DATA_NAME_LOADED_SKILLS] = [ + "computer-use-remote", + "a0-development", + ] + + removed = runtime.unload_agent_skill( + agent, + {"name": "computer-use-remote", "path": "/a0/skills/computer-use-remote"}, + ) + + assert removed is True + assert agent.data[runtime.AGENT_DATA_NAME_LOADED_SKILLS] == [ + "a0-development" + ] + + def test_clearing_chat_overrides_restores_scope_defaults(monkeypatch): monkeypatch.setattr( runtime.plugin_helpers, diff --git a/tools/skills_tool.py b/tools/skills_tool.py index 8cee30443..8cb2f23bf 100644 --- a/tools/skills_tool.py +++ b/tools/skills_tool.py @@ -7,7 +7,7 @@ from helpers import skills as skills_helper from helpers.print_style import PrintStyle -DATA_NAME_LOADED_SKILLS = "loaded_skills" +DATA_NAME_LOADED_SKILLS = skills_helper.AGENT_DATA_NAME_LOADED_SKILLS class SkillsTool(Tool):