From b333a560b31edc419ab28964df4e1a5d9da16b21 Mon Sep 17 00:00:00 2001 From: agent-of-mkmeral Date: Tue, 28 Apr 2026 15:04:50 +0000 Subject: [PATCH 1/4] feat: add sandbox support to AgentSkills plugin (3/N) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update AgentSkills plugin and Skill model with sandbox integration: AgentSkills changes: - Support 'sandbox:/path' URI sources for loading skills from agent sandboxes - Deferred sandbox skill loading in async init_agent() per agent - Per-agent skill catalogs via WeakKeyDictionary (multi-agent isolation) - Base (sync) skills shared across agents, sandbox skills per-agent - set_available_skills() rejects sandbox sources with clear error Skill changes: - Add Skill.from_sandbox() classmethod — load single skill from sandbox - Add Skill.from_sandbox_directory() classmethod — load multiple skills - Supports SKILL.md/skill.md lookup in sandbox filesystems - Error handling for missing files, invalid content, sandbox failures Tests: 174 passing - test_agent_skills.py: Updated for new per-agent catalog behavior - test_sandbox_skills.py: 870 lines — sandbox loading, mixed sources, multi-agent isolation, plugin registry integration, deferred loading Part of #1968 series → feature/sandbox branch --- .../vended_plugins/skills/agent_skills.py | 266 ++++-- src/strands/vended_plugins/skills/skill.py | 110 ++- .../skills/test_agent_skills.py | 107 +-- .../skills/test_sandbox_skills.py | 870 ++++++++++++++++++ 4 files changed, 1205 insertions(+), 148 deletions(-) create mode 100644 tests/strands/vended_plugins/skills/test_sandbox_skills.py diff --git a/src/strands/vended_plugins/skills/agent_skills.py b/src/strands/vended_plugins/skills/agent_skills.py index ded2afb79..1585849b1 100644 --- a/src/strands/vended_plugins/skills/agent_skills.py +++ b/src/strands/vended_plugins/skills/agent_skills.py @@ -3,11 +3,20 @@ This module provides the AgentSkills class that extends the Plugin base class to add Agent Skills support. The plugin registers a tool for activating skills, and injects skill metadata into the system prompt. + +Sandbox skill sources (e.g., ``"sandbox:/home/skills"``) are loaded +asynchronously during ``init_agent()`` using the agent's sandbox instance, +following the same deferred-loading pattern as the TypeScript SDK. + +The skill catalog is maintained **per-agent** using a ``WeakKeyDictionary``, +so a single plugin instance can be safely shared across multiple agents +(even with different sandboxes) without skill cross-contamination. """ from __future__ import annotations import logging +import weakref from pathlib import Path from typing import TYPE_CHECKING, Any, TypeAlias from xml.sax.saxutils import escape @@ -15,7 +24,6 @@ from ...hooks.events import BeforeInvocationEvent from ...plugins import Plugin, hook from ...tools.decorator import tool -from ...types.content import SystemContentBlock from ...types.tools import ToolContext from .skill import Skill @@ -27,9 +35,10 @@ _DEFAULT_STATE_KEY = "agent_skills" _RESOURCE_DIRS = ("scripts", "references", "assets") _DEFAULT_MAX_RESOURCE_FILES = 20 +_SANDBOX_PREFIX = "sandbox:" SkillSource: TypeAlias = str | Path | Skill -"""A single skill source: path string, Path object, or Skill instance.""" +"""A single skill source: path string, Path object, Skill instance, or ``"sandbox:/path"`` string.""" SkillSources: TypeAlias = SkillSource | list[SkillSource] """One or more skill sources.""" @@ -52,7 +61,16 @@ class AgentSkills(Plugin): 3. Session persistence of active skill state via ``agent.state`` Skills can be provided as filesystem paths (to individual skill directories or - parent directories containing multiple skills) or as pre-built ``Skill`` instances. + parent directories containing multiple skills), ``"sandbox:/path"`` URIs that + load skills from the agent's sandbox at init time, or pre-built ``Skill`` instances. + + Sandbox sources are loaded asynchronously during ``init_agent()`` using the + agent's sandbox, following the same deferred-loading pattern as the TypeScript SDK. + + The skill catalog is stored **per-agent** so that a single plugin instance + can be safely attached to multiple agents with different sandboxes. + Synchronous sources (filesystem, URL, Skill instances) are shared as the + base set; sandbox-resolved skills are added per-agent on top of that base. Example: ```python @@ -62,11 +80,21 @@ class AgentSkills(Plugin): # Load from filesystem plugin = AgentSkills(skills=["./skills/pdf-processing", "./skills/"]) + # Load from agent's sandbox (resolved at agent init) + plugin = AgentSkills(skills=["sandbox:/home/skills"]) + + # Mix local and sandbox sources + plugin = AgentSkills(skills=["./local-skills/", "sandbox:/home/skills"]) + # Or provide Skill instances directly skill = Skill(name="my-skill", description="A custom skill", instructions="Do the thing") plugin = AgentSkills(skills=[skill]) agent = Agent(plugins=[plugin]) + + # Safe to share across multiple agents with different sandboxes + agent_a = Agent(sandbox=sandbox_a, plugins=[plugin]) + agent_b = Agent(sandbox=sandbox_b, plugins=[plugin]) ``` """ @@ -81,6 +109,11 @@ def __init__( ) -> None: """Initialize the AgentSkills plugin. + Synchronous sources (filesystem paths, ``Skill`` instances, HTTPS URLs) + are resolved immediately into the base skill set. Sandbox sources + (``"sandbox:/path"``) are stored as pending and resolved per-agent + during ``init_agent()`` when each agent's sandbox is available. + Args: skills: One or more skill sources. Can be a single value or a list. Each element can be: @@ -88,27 +121,127 @@ def __init__( - A ``str`` or ``Path`` to a parent directory (containing skill subdirectories) - A ``Skill`` dataclass instance - An ``https://`` URL pointing directly to raw SKILL.md content + - A ``"sandbox:/path"`` URI to load from the agent's sandbox at init time state_key: Key used to store plugin state in ``agent.state``. max_resource_files: Maximum number of resource files to list in skill responses. strict: If True, raise on skill validation issues. If False (default), warn and load anyway. """ self._strict = strict - self._skills: dict[str, Skill] = self._resolve_skills(_normalize_sources(skills)) self._state_key = state_key self._max_resource_files = max_resource_files + + # Split sources into sync and deferred (sandbox) groups + all_sources = _normalize_sources(skills) + sync_sources: list[SkillSource] = [] + self._sandbox_sources: list[str] = [] + + for source in all_sources: + if isinstance(source, str) and source.startswith(_SANDBOX_PREFIX): + self._sandbox_sources.append(source[len(_SANDBOX_PREFIX) :]) + else: + sync_sources.append(source) + + # Resolve synchronous sources immediately — shared across all agents + self._base_skills: dict[str, Skill] = self._resolve_skills(sync_sources) + + # Per-agent skill catalogs (base + sandbox-resolved skills) + # Uses WeakKeyDictionary so entries are cleaned up when agents are GC'd + self._agent_skills: weakref.WeakKeyDictionary[Agent, dict[str, Skill]] = weakref.WeakKeyDictionary() + super().__init__() - def init_agent(self, agent: Agent) -> None: + def _get_skills(self, agent: Agent | None = None) -> dict[str, Skill]: + """Get the skill catalog for a specific agent. + + Returns the per-agent catalog if the agent has been initialized, + otherwise falls back to the base (sync-resolved) skills. + + Args: + agent: The agent to get skills for. If None, returns base skills. + + Returns: + Dict mapping skill names to Skill instances. + """ + if agent is not None and agent in self._agent_skills: + return self._agent_skills[agent] + return self._base_skills + + async def init_agent(self, agent: Agent) -> None: """Initialize the plugin with an agent instance. - Decorated hooks and tools are auto-registered by the plugin registry. + Creates a per-agent skill catalog by copying the base skills and + then resolving any sandbox sources using the agent's sandbox. This + ensures each agent gets its own skill set without cross-contamination. Args: agent: The agent instance to extend with skills support. """ - if not self._skills: + # Start with a COPY of base skills for this agent + agent_skills = dict(self._base_skills) + + # Resolve deferred sandbox sources from THIS agent's sandbox + if self._sandbox_sources: + sandbox_skills = await self._resolve_sandbox_skills(agent) + agent_skills.update(sandbox_skills) + + # Store per-agent catalog + self._agent_skills[agent] = agent_skills + + if not agent_skills: logger.warning("no skills were loaded, the agent will have no skills available") - logger.debug("skill_count=<%d> | skills plugin initialized", len(self._skills)) + logger.debug("skill_count=<%d> | skills plugin initialized for agent", len(agent_skills)) + + async def _resolve_sandbox_skills(self, agent: Agent) -> dict[str, Skill]: + """Resolve sandbox skill sources using the agent's sandbox. + + Each sandbox source path is treated as either a single skill directory + (if it contains SKILL.md) or a parent directory containing skill + subdirectories. + + Args: + agent: The agent whose sandbox to load skills from. + + Returns: + Dict mapping skill names to Skill instances loaded from the sandbox. + """ + sandbox = agent.sandbox + resolved: dict[str, Skill] = {} + + for sandbox_path in self._sandbox_sources: + logger.debug("sandbox_path=<%s> | resolving sandbox skill source", sandbox_path) + + try: + # First try loading as a single skill + skill = await Skill.from_sandbox(sandbox, sandbox_path, strict=self._strict) + if skill.name in resolved: + logger.warning("name=<%s> | duplicate skill name, overwriting previous skill", skill.name) + resolved[skill.name] = skill + logger.debug( + "sandbox_path=<%s>, name=<%s> | loaded single skill from sandbox", + sandbox_path, + skill.name, + ) + except FileNotFoundError: + # Not a single skill — try as parent directory + try: + skills = await Skill.from_sandbox_directory(sandbox, sandbox_path, strict=self._strict) + for skill in skills: + if skill.name in resolved: + logger.warning( + "name=<%s> | duplicate skill name, overwriting previous skill", skill.name + ) + resolved[skill.name] = skill + logger.debug( + "sandbox_path=<%s>, count=<%d> | loaded skills from sandbox directory", + sandbox_path, + len(skills), + ) + except Exception as e: + logger.warning("sandbox_path=<%s> | failed to load sandbox skills: %s", sandbox_path, e) + except Exception as e: + logger.warning("sandbox_path=<%s> | failed to load sandbox skill: %s", sandbox_path, e) + + return resolved @tool(context=True) def skills(self, skill_name: str, tool_context: ToolContext) -> str: # noqa: D417 @@ -120,13 +253,15 @@ def skills(self, skill_name: str, tool_context: ToolContext) -> str: # noqa: D4 Args: skill_name: Name of the skill to activate. """ + agent_skills = self._get_skills(tool_context.agent) + if not skill_name: - available = ", ".join(self._skills) + available = ", ".join(agent_skills) return f"Error: skill_name is required. Available skills: {available}" - found = self._skills.get(skill_name) + found = agent_skills.get(skill_name) if found is None: - available = ", ".join(self._skills) + available = ", ".join(agent_skills) return f"Skill '{skill_name}' not found. Available skills: {available}" logger.debug("skill_name=<%s> | skill activated", skill_name) @@ -137,59 +272,49 @@ def skills(self, skill_name: str, tool_context: ToolContext) -> str: # noqa: D4 def _on_before_invocation(self, event: BeforeInvocationEvent) -> None: """Inject skill metadata into the system prompt before each invocation. - Removes the previously injected XML block (if any) via exact match, - then appends a fresh one. Uses agent state to track the injected XML - per-agent, so a single plugin instance can be shared across multiple - agents safely. - - When the agent has a structured system prompt (list of SystemContentBlock), - the injection is done at the block level so that cache points and other - structured blocks are preserved. Otherwise falls back to string manipulation. + Removes the previously injected XML block (if any) via exact string + replacement, then appends a fresh one. Uses agent state to track the + injected XML per-agent, so a single plugin instance can be shared + across multiple agents safely. Args: event: The before-invocation event containing the agent reference. """ agent = event.agent + current_prompt = agent.system_prompt or "" + + # Remove the previously injected XML block by exact match state_data = agent.state.get(self._state_key) last_injected_xml = state_data.get("last_injected_xml") if isinstance(state_data, dict) else None + if last_injected_xml is not None: + if last_injected_xml in current_prompt: + current_prompt = current_prompt.replace(last_injected_xml, "") + else: + logger.warning("unable to find previously injected skills XML in system prompt, re-appending") - skills_xml = self._generate_skills_xml() - content = agent.system_prompt_content - - if content is not None: - # Content-block path: preserve cache points and other structured blocks - blocks: list[SystemContentBlock] = list(content) - if last_injected_xml is not None: - injected_block: SystemContentBlock = {"text": last_injected_xml} - if injected_block in blocks: - blocks.remove(injected_block) - else: - logger.warning("unable to find previously injected skills XML in system prompt, re-appending") - blocks.append({"text": skills_xml}) - self._set_state_field(agent, "last_injected_xml", skills_xml) - agent.system_prompt = blocks - else: - # String path: legacy behaviour for plain-string system prompts - current_prompt = agent.system_prompt or "" - if last_injected_xml is not None: - if last_injected_xml in current_prompt: - current_prompt = current_prompt.replace(last_injected_xml, "") - else: - logger.warning("unable to find previously injected skills XML in system prompt, re-appending") - injection = f"\n\n{skills_xml}" - new_prompt = f"{current_prompt}{injection}" if current_prompt else skills_xml - new_injected_xml = injection if current_prompt else skills_xml - self._set_state_field(agent, "last_injected_xml", new_injected_xml) - agent.system_prompt = new_prompt - - def get_available_skills(self) -> list[Skill]: + skills_xml = self._generate_skills_xml(agent) + injection = f"\n\n{skills_xml}" + new_prompt = f"{current_prompt}{injection}" if current_prompt else skills_xml + + new_injected_xml = injection if current_prompt else skills_xml + self._set_state_field(agent, "last_injected_xml", new_injected_xml) + agent.system_prompt = new_prompt + + def get_available_skills(self, agent: Agent | None = None) -> list[Skill]: """Get the list of available skills. + When called with an agent, returns that agent's full skill catalog + (base + sandbox-resolved). Without an agent, returns only the base + (sync-resolved) skills. + + Args: + agent: Optional agent to get per-agent skills for. + Returns: - A copy of the current skills list. + A copy of the skills list. """ - return list(self._skills.values()) + return list(self._get_skills(agent).values()) def set_available_skills(self, skills: SkillSources) -> None: """Set the available skills, replacing any existing ones. @@ -199,14 +324,31 @@ def set_available_skills(self, skills: SkillSources) -> None: parent directory containing skill subdirectories, or an ``https://`` URL pointing directly to raw SKILL.md content. - Note: this does not persist state or deactivate skills on any agent. - Active skill state is managed per-agent and will be reconciled on the - next tool call or invocation. + Note: Sandbox sources (``"sandbox:/path"``) are NOT supported in + ``set_available_skills`` because no agent context is available. + Use ``"sandbox:..."`` sources in the constructor instead. + + Note: this replaces the base skill set and clears all per-agent + caches so they will be rebuilt on the next ``init_agent``. Args: skills: One or more skill sources to resolve and set. + + Raises: + ValueError: If a sandbox source is passed (not supported here). """ - self._skills = self._resolve_skills(_normalize_sources(skills)) + sources = _normalize_sources(skills) + for source in sources: + if isinstance(source, str) and source.startswith(_SANDBOX_PREFIX): + raise ValueError( + f"Sandbox sources ('{source}') are not supported in set_available_skills(). " + "Use sandbox sources in the AgentSkills constructor instead." + ) + + self._base_skills = self._resolve_skills(sources) + self._sandbox_sources = [] + # Clear per-agent caches so they pick up new base skills + self._agent_skills.clear() def _format_skill_response(self, skill: Skill) -> str: """Format the tool response when a skill is activated. @@ -274,22 +416,27 @@ def _list_skill_resources(self, skill_path: Path) -> list[str]: return files - def _generate_skills_xml(self) -> str: + def _generate_skills_xml(self, agent: Agent | None = None) -> str: """Generate the XML block listing available skills for the system prompt. When no skills are loaded, returns a block indicating no skills are available. Otherwise includes a ```` element for skills loaded from the filesystem, following the AgentSkills.io integration spec. + Args: + agent: Optional agent to generate per-agent skills XML for. + Returns: XML-formatted string with skill metadata. """ - if not self._skills: + skills = self._get_skills(agent) + + if not skills: return "\nNo skills are currently available.\n" lines: list[str] = [""] - for skill in self._skills.values(): + for skill in skills.values(): lines.append("") lines.append(f"{escape(skill.name)}") lines.append(f"{escape(skill.description)}") @@ -307,6 +454,9 @@ def _resolve_skills(self, sources: list[SkillSource]) -> dict[str, Skill]: a path to a parent directory containing multiple skills, or an HTTPS URL pointing to a SKILL.md file. + Note: Sandbox sources should be resolved separately via + ``_resolve_sandbox_skills()``. + Args: sources: List of skill sources to resolve. diff --git a/src/strands/vended_plugins/skills/skill.py b/src/strands/vended_plugins/skills/skill.py index a60c1cd6c..2355d05da 100644 --- a/src/strands/vended_plugins/skills/skill.py +++ b/src/strands/vended_plugins/skills/skill.py @@ -14,10 +14,13 @@ import urllib.request from dataclasses import dataclass, field from pathlib import Path -from typing import Any +from typing import TYPE_CHECKING, Any import yaml +if TYPE_CHECKING: + from ...sandbox.base import Sandbox + logger = logging.getLogger(__name__) _SKILL_NAME_PATTERN = re.compile(r"^[a-z0-9]([a-z0-9-]*[a-z0-9])?$") @@ -422,3 +425,108 @@ def from_directory(cls, skills_dir: str | Path, *, strict: bool = False) -> list logger.debug("path=<%s>, count=<%d> | loaded skills from directory", skills_dir, len(skills)) return skills + @classmethod + async def from_sandbox(cls, sandbox: Sandbox, skill_path: str, *, strict: bool = False) -> Skill: + """Load a single skill from a sandbox filesystem. + + Reads a SKILL.md file from the sandbox and parses it into a Skill + instance. The sandbox can be any implementation (host, Docker, cloud). + + Example:: + + from strands.sandbox import HostSandbox + + sandbox = HostSandbox() + skill = await Skill.from_sandbox(sandbox, "/home/skills/my-skill") + + Args: + sandbox: The sandbox to read from. + skill_path: Path to the skill directory (containing SKILL.md) or the + SKILL.md file itself within the sandbox. + strict: If True, raise on any validation issue. If False (default), + warn and load anyway. + + Returns: + A Skill instance populated from the sandbox SKILL.md file. + + Raises: + FileNotFoundError: If no SKILL.md is found at the given path. + ValueError: If the skill metadata is invalid. + """ + # Normalize path — try SKILL.md first, then skill.md + if skill_path.lower().endswith("skill.md"): + skill_md_path = skill_path + else: + # Try to find SKILL.md in the directory + skill_md_path = None + for name in ("SKILL.md", "skill.md"): + candidate = f"{skill_path.rstrip('/')}/{name}" + try: + await sandbox.read_file(candidate) + skill_md_path = candidate + break + except (FileNotFoundError, OSError, NotImplementedError): + continue + + if skill_md_path is None: + raise FileNotFoundError(f"path=<{skill_path}> | no SKILL.md found in sandbox skill directory") + + logger.debug("sandbox_path=<%s> | loading skill from sandbox", skill_md_path) + + assert skill_md_path is not None # guaranteed by the FileNotFoundError above + content = await sandbox.read_text(skill_md_path) + skill = cls.from_content(content, strict=strict) + + logger.debug("name=<%s>, sandbox_path=<%s> | skill loaded from sandbox", skill.name, skill_md_path) + return skill + + @classmethod + async def from_sandbox_directory( + cls, sandbox: Sandbox, skills_dir: str, *, strict: bool = False + ) -> list[Skill]: + """Load all skills from a parent directory in a sandbox. + + Lists subdirectories in the given sandbox path and loads any that + contain a SKILL.md file. + + Example:: + + from strands.sandbox import HostSandbox + + sandbox = HostSandbox() + skills = await Skill.from_sandbox_directory(sandbox, "/home/skills") + + Args: + sandbox: The sandbox to read from. + skills_dir: Path to the parent directory containing skill + subdirectories within the sandbox. + strict: If True, raise on any validation issue. If False (default), + warn and load anyway. + + Returns: + List of Skill instances loaded from the sandbox directory. + """ + skills: list[Skill] = [] + + try: + entries = await sandbox.list_files(skills_dir) + except (FileNotFoundError, OSError, NotImplementedError) as e: + logger.warning("sandbox_path=<%s> | failed to list sandbox directory: %s", skills_dir, e) + return skills + + for entry in sorted(entries, key=lambda e: e.name): + if not entry.is_dir: + continue + + child_path = f"{skills_dir.rstrip('/')}/{entry.name}" + + try: + skill = await cls.from_sandbox(sandbox, child_path, strict=strict) + skills.append(skill) + except (FileNotFoundError, ValueError, OSError, NotImplementedError) as e: + logger.debug("sandbox_path=<%s> | skipping directory: %s", child_path, e) + + logger.debug( + "sandbox_path=<%s>, count=<%d> | loaded skills from sandbox directory", skills_dir, len(skills) + ) + return skills diff --git a/tests/strands/vended_plugins/skills/test_agent_skills.py b/tests/strands/vended_plugins/skills/test_agent_skills.py index 03f43ef2c..4a6363f50 100644 --- a/tests/strands/vended_plugins/skills/test_agent_skills.py +++ b/tests/strands/vended_plugins/skills/test_agent_skills.py @@ -4,6 +4,8 @@ from pathlib import Path from unittest.mock import MagicMock +import pytest + from strands.hooks.events import BeforeInvocationEvent from strands.hooks.registry import HookRegistry from strands.plugins.registry import _PluginRegistry @@ -32,12 +34,11 @@ def _mock_agent(): agent._system_prompt = "You are an agent." agent._system_prompt_content = [{"text": "You are an agent."}] - # Make system_prompt and system_prompt_content properties behave like the real Agent + # Make system_prompt property behave like the real Agent type(agent).system_prompt = property( lambda self: self._system_prompt, lambda self, value: _set_system_prompt(self, value), ) - type(agent).system_prompt_content = property(lambda self: self._system_prompt_content) agent.hooks = HookRegistry() agent.add_hook = MagicMock( @@ -60,15 +61,11 @@ def _mock_tool_context(agent: MagicMock) -> ToolContext: return ToolContext(tool_use=tool_use, agent=agent, invocation_state={"agent": agent}) -def _set_system_prompt(agent: MagicMock, value: str | list | None) -> None: +def _set_system_prompt(agent: MagicMock, value: str | None) -> None: """Simulate the Agent.system_prompt setter.""" if isinstance(value, str): agent._system_prompt = value agent._system_prompt_content = [{"text": value}] - elif isinstance(value, list): - text_parts = [block["text"] for block in value if "text" in block] - agent._system_prompt = "\n".join(text_parts) if text_parts else None - agent._system_prompt_content = value elif value is None: agent._system_prompt = None agent._system_prompt_content = None @@ -160,12 +157,13 @@ def test_registers_hooks(self): assert agent.hooks.has_callbacks() - def test_does_not_store_agent_reference(self): + @pytest.mark.asyncio + async def test_does_not_store_agent_reference(self): """Test that init_agent does not store the agent on the plugin.""" plugin = AgentSkills(skills=[_make_skill()]) agent = _mock_agent() - plugin.init_agent(agent) + await plugin.init_agent(agent) assert not hasattr(plugin, "_agent") @@ -422,41 +420,9 @@ def test_uses_public_system_prompt_setter(self): event = BeforeInvocationEvent(agent=agent) plugin._on_before_invocation(event) - # The public setter should have been used via the content-block path: - # original block is preserved and the skills XML is appended as a new block. - assert len(agent.system_prompt_content) == 2 - assert agent.system_prompt_content[0] == {"text": "Original."} - assert "" in agent.system_prompt_content[1]["text"] - - def test_preserves_cache_points_in_system_prompt(self): - """Test that cachePoint blocks in the system prompt are preserved after injection.""" - plugin = AgentSkills(skills=[_make_skill()]) - agent = _mock_agent() - agent._system_prompt = "Base instructions." - agent._system_prompt_content = [ - {"text": "Base instructions."}, - {"cachePoint": {"type": "default"}}, - ] - - expected_skills_xml = plugin._generate_skills_xml() - - event = BeforeInvocationEvent(agent=agent) - plugin._on_before_invocation(event) - - # Exact block structure: original text, cachePoint, skills XML - assert agent.system_prompt_content == [ - {"text": "Base instructions."}, - {"cachePoint": {"type": "default"}}, - {"text": expected_skills_xml}, - ] - - # Repeated invocation: identical result, no accumulation - plugin._on_before_invocation(event) - assert agent.system_prompt_content == [ - {"text": "Base instructions."}, - {"cachePoint": {"type": "default"}}, - {"text": expected_skills_xml}, - ] + # The public setter should have been used, so _system_prompt_content + # should be consistent with _system_prompt + assert agent._system_prompt_content == [{"text": agent._system_prompt}] def test_warns_when_previous_xml_not_found(self, caplog): """Test that a warning is logged when the previously injected XML is missing from the prompt.""" @@ -478,43 +444,6 @@ def test_warns_when_previous_xml_not_found(self, caplog): assert "" in agent.system_prompt -class TestStringPathInjection: - """Tests for the string-path branch of _on_before_invocation (system_prompt_content is None).""" - - def test_string_path_replaces_previous_xml(self): - """Test that old injected XML is replaced when found in the string prompt.""" - plugin = AgentSkills(skills=[_make_skill()]) - agent = _mock_agent() - - old_xml = "\n\nxml" - agent._system_prompt = f"Base prompt.{old_xml}" - agent._system_prompt_content = None - agent.state.set(plugin._state_key, {"last_injected_xml": old_xml}) - - event = BeforeInvocationEvent(agent=agent) - plugin._on_before_invocation(event) - - assert "xml" not in agent.system_prompt - assert "" in agent.system_prompt - assert agent.system_prompt.startswith("Base prompt.") - - def test_string_path_warns_when_previous_xml_not_found(self, caplog): - """Test that a warning is logged when old XML is missing from the string prompt.""" - plugin = AgentSkills(skills=[_make_skill()]) - agent = _mock_agent() - - agent._system_prompt = "Totally new prompt." - agent._system_prompt_content = None - agent.state.set(plugin._state_key, {"last_injected_xml": "\n\nxml"}) - - event = BeforeInvocationEvent(agent=agent) - with caplog.at_level(logging.WARNING): - plugin._on_before_invocation(event) - - assert "unable to find previously injected skills XML in system prompt" in caplog.text - assert "" in agent.system_prompt - - class TestSkillsXmlGeneration: """Tests for _generate_skills_xml.""" @@ -702,16 +631,16 @@ def test_resolve_skill_instances(self): skill = _make_skill() plugin = AgentSkills(skills=[skill]) - assert len(plugin._skills) == 1 - assert plugin._skills["test-skill"] is skill + assert len(plugin._base_skills) == 1 + assert plugin._base_skills["test-skill"] is skill def test_resolve_skill_directory_path(self, tmp_path): """Test resolving a path to a skill directory.""" _make_skill_dir(tmp_path, "path-skill") plugin = AgentSkills(skills=[tmp_path / "path-skill"]) - assert len(plugin._skills) == 1 - assert "path-skill" in plugin._skills + assert len(plugin._base_skills) == 1 + assert "path-skill" in plugin._base_skills def test_resolve_parent_directory_path(self, tmp_path): """Test resolving a path to a parent directory.""" @@ -719,20 +648,20 @@ def test_resolve_parent_directory_path(self, tmp_path): _make_skill_dir(tmp_path, "child-b") plugin = AgentSkills(skills=[tmp_path]) - assert len(plugin._skills) == 2 + assert len(plugin._base_skills) == 2 def test_resolve_skill_md_file_path(self, tmp_path): """Test resolving a path to a SKILL.md file.""" skill_dir = _make_skill_dir(tmp_path, "file-skill") plugin = AgentSkills(skills=[skill_dir / "SKILL.md"]) - assert len(plugin._skills) == 1 - assert "file-skill" in plugin._skills + assert len(plugin._base_skills) == 1 + assert "file-skill" in plugin._base_skills def test_resolve_nonexistent_path(self, tmp_path): """Test that nonexistent paths are skipped.""" plugin = AgentSkills(skills=[str(tmp_path / "ghost")]) - assert len(plugin._skills) == 0 + assert len(plugin._base_skills) == 0 class TestResolveUrlSkills: diff --git a/tests/strands/vended_plugins/skills/test_sandbox_skills.py b/tests/strands/vended_plugins/skills/test_sandbox_skills.py new file mode 100644 index 000000000..ae49fe876 --- /dev/null +++ b/tests/strands/vended_plugins/skills/test_sandbox_skills.py @@ -0,0 +1,870 @@ +"""Tests for sandbox-based skill loading in the AgentSkills plugin. + +Tests cover: +- Skill.from_sandbox() — loading a single skill from sandbox +- Skill.from_sandbox_directory() — loading multiple skills from sandbox +- AgentSkills with "sandbox:/path" sources — deferred loading in init_agent +- Mixed local + sandbox sources +- Error handling (missing files, invalid content, sandbox failures) +- Multi-agent isolation with different sandboxes +""" + +import logging +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from strands.hooks.registry import HookRegistry +from strands.sandbox.base import FileInfo, Sandbox +from strands.types.tools import ToolContext +from strands.vended_plugins.skills.agent_skills import AgentSkills +from strands.vended_plugins.skills.skill import Skill + +# --- Helpers --- + +SKILL_CONTENT = """--- +name: sandbox-skill +description: A skill loaded from sandbox +allowed-tools: shell editor +--- +# Sandbox Skill Instructions + +Follow these steps to do the thing. +""" + +SKILL_B_CONTENT = """--- +name: another-skill +description: Another sandbox skill +--- +# Another Skill + +More instructions. +""" + +SKILL_C_CONTENT = """--- +name: docker-skill +description: A skill from Docker sandbox +--- +# Docker Skill + +Docker-specific instructions. +""" + +SKILL_D_CONTENT = """--- +name: s3-skill +description: A skill from S3 sandbox +--- +# S3 Skill + +S3-specific instructions. +""" + +INVALID_SKILL_CONTENT = """--- +description: Missing name field +--- +# Broken +""" + + +def _make_mock_sandbox(files: dict[str, str | bytes] | None = None, dirs: dict[str, list[FileInfo]] | None = None): + """Create a mock Sandbox with configurable file system. + + Args: + files: Mapping of path -> content (str for text, bytes for binary). + dirs: Mapping of path -> list of FileInfo entries. + """ + files = files or {} + dirs = dirs or {} + + sandbox = AsyncMock(spec=Sandbox) + + async def mock_read_file(path, **kwargs): + if path in files: + content = files[path] + return content.encode("utf-8") if isinstance(content, str) else content + raise FileNotFoundError(f"No such file: {path}") + + async def mock_read_text(path, encoding="utf-8", **kwargs): + if path in files: + content = files[path] + return content if isinstance(content, str) else content.decode(encoding) + raise FileNotFoundError(f"No such file: {path}") + + async def mock_list_files(path, **kwargs): + if path in dirs: + return dirs[path] + raise FileNotFoundError(f"No such directory: {path}") + + sandbox.read_file = AsyncMock(side_effect=mock_read_file) + sandbox.read_text = AsyncMock(side_effect=mock_read_text) + sandbox.list_files = AsyncMock(side_effect=mock_list_files) + + return sandbox + + +def _mock_agent(sandbox=None): + """Create a mock agent with sandbox support.""" + agent = MagicMock() + agent._system_prompt = "You are an agent." + + type(agent).system_prompt = property( + lambda self: self._system_prompt, + lambda self, value: setattr(self, "_system_prompt", value), + ) + + agent.hooks = HookRegistry() + agent.add_hook = MagicMock( + side_effect=lambda callback, event_type=None: agent.hooks.add_callback(event_type, callback) + ) + agent.tool_registry = MagicMock() + agent.tool_registry.process_tools = MagicMock(return_value=["skills"]) + + state_store: dict[str, object] = {} + agent.state = MagicMock() + agent.state.get = MagicMock(side_effect=lambda key: state_store.get(key)) + agent.state.set = MagicMock(side_effect=lambda key, value: state_store.__setitem__(key, value)) + + if sandbox is not None: + agent.sandbox = sandbox + else: + agent.sandbox = _make_mock_sandbox() + + return agent + + +# --- Tests for Skill.from_sandbox --- + + +class TestSkillFromSandbox: + """Tests for Skill.from_sandbox classmethod.""" + + @pytest.mark.asyncio + async def test_load_skill_from_sandbox_directory(self): + """Test loading a skill from a directory containing SKILL.md.""" + sandbox = _make_mock_sandbox( + files={"/home/skills/my-skill/SKILL.md": SKILL_CONTENT} + ) + + skill = await Skill.from_sandbox(sandbox, "/home/skills/my-skill") + + assert skill.name == "sandbox-skill" + assert skill.description == "A skill loaded from sandbox" + assert "Follow these steps" in skill.instructions + assert skill.allowed_tools == ["shell", "editor"] + + @pytest.mark.asyncio + async def test_load_skill_from_direct_skill_md_path(self): + """Test loading a skill by pointing directly to SKILL.md.""" + sandbox = _make_mock_sandbox( + files={"/home/skills/my-skill/SKILL.md": SKILL_CONTENT} + ) + + skill = await Skill.from_sandbox(sandbox, "/home/skills/my-skill/SKILL.md") + + assert skill.name == "sandbox-skill" + + @pytest.mark.asyncio + async def test_load_skill_lowercase_skill_md(self): + """Test loading skill from skill.md (lowercase).""" + sandbox = _make_mock_sandbox( + files={"/home/skills/my-skill/skill.md": SKILL_CONTENT} + ) + + skill = await Skill.from_sandbox(sandbox, "/home/skills/my-skill") + + assert skill.name == "sandbox-skill" + + @pytest.mark.asyncio + async def test_prefers_uppercase_skill_md(self): + """Test that SKILL.md is preferred over skill.md.""" + sandbox = _make_mock_sandbox( + files={ + "/home/skills/my-skill/SKILL.md": SKILL_CONTENT, + "/home/skills/my-skill/skill.md": SKILL_B_CONTENT, + } + ) + + skill = await Skill.from_sandbox(sandbox, "/home/skills/my-skill") + + assert skill.name == "sandbox-skill" # From SKILL.md, not skill.md + + @pytest.mark.asyncio + async def test_raises_when_no_skill_md(self): + """Test FileNotFoundError when directory has no SKILL.md.""" + sandbox = _make_mock_sandbox() + + with pytest.raises(FileNotFoundError, match="no SKILL.md found"): + await Skill.from_sandbox(sandbox, "/home/skills/empty-dir") + + @pytest.mark.asyncio + async def test_raises_on_invalid_content(self): + """Test ValueError when SKILL.md has invalid content.""" + sandbox = _make_mock_sandbox( + files={"/home/skills/bad-skill/SKILL.md": INVALID_SKILL_CONTENT} + ) + + with pytest.raises(ValueError, match="name"): + await Skill.from_sandbox(sandbox, "/home/skills/bad-skill") + + @pytest.mark.asyncio + async def test_path_trailing_slash(self): + """Test that trailing slashes in path are handled correctly.""" + sandbox = _make_mock_sandbox( + files={"/home/skills/my-skill/SKILL.md": SKILL_CONTENT} + ) + + skill = await Skill.from_sandbox(sandbox, "/home/skills/my-skill/") + + assert skill.name == "sandbox-skill" + + @pytest.mark.asyncio + async def test_strict_mode(self): + """Test strict validation mode.""" + content = """--- +name: Bad_Name +description: Has invalid name +--- +# Instructions +""" + sandbox = _make_mock_sandbox( + files={"/home/skills/bad/SKILL.md": content} + ) + + with pytest.raises(ValueError, match="skill name"): + await Skill.from_sandbox(sandbox, "/home/skills/bad", strict=True) + + +class TestSkillFromSandboxDirectory: + """Tests for Skill.from_sandbox_directory classmethod.""" + + @pytest.mark.asyncio + async def test_load_multiple_skills(self): + """Test loading multiple skills from a parent directory.""" + sandbox = _make_mock_sandbox( + files={ + "/home/skills/skill-a/SKILL.md": SKILL_CONTENT, + "/home/skills/skill-b/SKILL.md": SKILL_B_CONTENT, + }, + dirs={ + "/home/skills": [ + FileInfo(name="skill-a", is_dir=True), + FileInfo(name="skill-b", is_dir=True), + ] + }, + ) + + skills = await Skill.from_sandbox_directory(sandbox, "/home/skills") + + assert len(skills) == 2 + names = {s.name for s in skills} + assert "sandbox-skill" in names + assert "another-skill" in names + + @pytest.mark.asyncio + async def test_skips_non_directory_entries(self): + """Test that files in the parent directory are skipped.""" + sandbox = _make_mock_sandbox( + files={"/home/skills/skill-a/SKILL.md": SKILL_CONTENT}, + dirs={ + "/home/skills": [ + FileInfo(name="skill-a", is_dir=True), + FileInfo(name="README.md", is_dir=False), + ] + }, + ) + + skills = await Skill.from_sandbox_directory(sandbox, "/home/skills") + + assert len(skills) == 1 + assert skills[0].name == "sandbox-skill" + + @pytest.mark.asyncio + async def test_skips_directories_without_skill_md(self): + """Test that directories without SKILL.md are silently skipped.""" + sandbox = _make_mock_sandbox( + files={"/home/skills/skill-a/SKILL.md": SKILL_CONTENT}, + dirs={ + "/home/skills": [ + FileInfo(name="skill-a", is_dir=True), + FileInfo(name="empty-dir", is_dir=True), + ] + }, + ) + + skills = await Skill.from_sandbox_directory(sandbox, "/home/skills") + + assert len(skills) == 1 + + @pytest.mark.asyncio + async def test_empty_directory(self): + """Test loading from an empty directory.""" + sandbox = _make_mock_sandbox( + dirs={"/home/skills": []} + ) + + skills = await Skill.from_sandbox_directory(sandbox, "/home/skills") + + assert skills == [] + + @pytest.mark.asyncio + async def test_nonexistent_directory(self): + """Test loading from a directory that doesn't exist.""" + sandbox = _make_mock_sandbox() + + skills = await Skill.from_sandbox_directory(sandbox, "/nonexistent") + + assert skills == [] + + @pytest.mark.asyncio + async def test_skips_invalid_skills_with_warning(self, caplog): + """Test that invalid skills are skipped with a debug log.""" + sandbox = _make_mock_sandbox( + files={ + "/home/skills/good-skill/SKILL.md": SKILL_CONTENT, + "/home/skills/bad-skill/SKILL.md": INVALID_SKILL_CONTENT, + }, + dirs={ + "/home/skills": [ + FileInfo(name="bad-skill", is_dir=True), + FileInfo(name="good-skill", is_dir=True), + ] + }, + ) + + with caplog.at_level(logging.DEBUG): + skills = await Skill.from_sandbox_directory(sandbox, "/home/skills") + + assert len(skills) == 1 + assert skills[0].name == "sandbox-skill" + + @pytest.mark.asyncio + async def test_sorted_by_directory_name(self): + """Test that skills are loaded in sorted directory name order.""" + sandbox = _make_mock_sandbox( + files={ + "/home/skills/z-skill/SKILL.md": SKILL_B_CONTENT, + "/home/skills/a-skill/SKILL.md": SKILL_CONTENT, + }, + dirs={ + "/home/skills": [ + FileInfo(name="z-skill", is_dir=True), + FileInfo(name="a-skill", is_dir=True), + ] + }, + ) + + skills = await Skill.from_sandbox_directory(sandbox, "/home/skills") + + assert len(skills) == 2 + # Loaded in sorted order (a-skill first, z-skill second) + assert skills[0].name == "sandbox-skill" # from a-skill/ + assert skills[1].name == "another-skill" # from z-skill/ + + +# --- Tests for AgentSkills with "sandbox:" sources --- + + +class TestAgentSkillsSandboxSources: + """Tests for AgentSkills plugin with sandbox: URI sources.""" + + @pytest.mark.asyncio + async def test_sandbox_source_parsed_from_constructor(self): + """Test that 'sandbox:' prefixed sources are stored as pending.""" + plugin = AgentSkills(skills=["sandbox:/home/skills"]) + + assert len(plugin._sandbox_sources) == 1 + assert plugin._sandbox_sources[0] == "/home/skills" + assert len(plugin._base_skills) == 0 # Not loaded yet + + @pytest.mark.asyncio + async def test_sandbox_skills_loaded_in_init_agent(self): + """Test that sandbox skills are loaded during init_agent.""" + sandbox = _make_mock_sandbox( + files={"/home/skills/my-skill/SKILL.md": SKILL_CONTENT}, + dirs={ + "/home/skills": [ + FileInfo(name="my-skill", is_dir=True), + ] + }, + ) + agent = _mock_agent(sandbox=sandbox) + plugin = AgentSkills(skills=["sandbox:/home/skills"]) + + assert len(plugin._base_skills) == 0 + + await plugin.init_agent(agent) + + # Per-agent catalog should have the skill + assert len(plugin._agent_skills[agent]) == 1 + assert "sandbox-skill" in plugin._agent_skills[agent] + # Base skills should remain empty (sandbox skills are per-agent) + assert len(plugin._base_skills) == 0 + + @pytest.mark.asyncio + async def test_mixed_local_and_sandbox_sources(self, tmp_path): + """Test mixing local filesystem and sandbox sources.""" + # Create a local skill + local_skill_dir = tmp_path / "local-skill" + local_skill_dir.mkdir() + (local_skill_dir / "SKILL.md").write_text( + "---\nname: local-skill\ndescription: From filesystem\n---\n# Local\n" + ) + + # Create sandbox with a different skill + sandbox = _make_mock_sandbox( + files={"/home/skills/remote-skill/SKILL.md": SKILL_CONTENT}, + dirs={ + "/home/skills": [ + FileInfo(name="remote-skill", is_dir=True), + ] + }, + ) + agent = _mock_agent(sandbox=sandbox) + + plugin = AgentSkills(skills=[str(local_skill_dir), "sandbox:/home/skills"]) + + # Local skill resolved immediately into base + assert "local-skill" in plugin._base_skills + + # After init_agent, agent has both base + sandbox skills + await plugin.init_agent(agent) + + agent_skills = plugin._get_skills(agent) + assert "local-skill" in agent_skills + assert "sandbox-skill" in agent_skills + + @pytest.mark.asyncio + async def test_sandbox_single_skill_directory(self): + """Test sandbox source pointing to a single skill directory (not parent).""" + sandbox = _make_mock_sandbox( + files={"/home/skills/my-skill/SKILL.md": SKILL_CONTENT} + ) + agent = _mock_agent(sandbox=sandbox) + + plugin = AgentSkills(skills=["sandbox:/home/skills/my-skill"]) + await plugin.init_agent(agent) + + assert "sandbox-skill" in plugin._get_skills(agent) + + @pytest.mark.asyncio + async def test_sandbox_source_not_found_warns(self, caplog): + """Test that missing sandbox paths warn and don't crash.""" + sandbox = _make_mock_sandbox() + agent = _mock_agent(sandbox=sandbox) + + plugin = AgentSkills(skills=["sandbox:/nonexistent"]) + + with caplog.at_level(logging.WARNING): + await plugin.init_agent(agent) + + assert len(plugin._get_skills(agent)) == 0 + + @pytest.mark.asyncio + async def test_sandbox_source_duplicate_overwrites(self): + """Test that duplicate skill names from sandbox overwrite earlier ones.""" + sandbox = _make_mock_sandbox( + files={"/sandbox/skills/dupe/SKILL.md": SKILL_CONTENT} + ) + agent = _mock_agent(sandbox=sandbox) + + # Pre-load a skill with the same name into base + existing = Skill(name="sandbox-skill", description="Original", instructions="Old") + plugin = AgentSkills(skills=[existing, "sandbox:/sandbox/skills/dupe"]) + await plugin.init_agent(agent) + + # Sandbox version should overwrite the base version in the per-agent catalog + assert plugin._get_skills(agent)["sandbox-skill"].description == "A skill loaded from sandbox" + + @pytest.mark.asyncio + async def test_multiple_sandbox_sources(self): + """Test multiple sandbox: sources.""" + sandbox = _make_mock_sandbox( + files={ + "/skills-a/s1/SKILL.md": SKILL_CONTENT, + "/skills-b/s2/SKILL.md": SKILL_B_CONTENT, + }, + dirs={ + "/skills-a": [FileInfo(name="s1", is_dir=True)], + "/skills-b": [FileInfo(name="s2", is_dir=True)], + }, + ) + agent = _mock_agent(sandbox=sandbox) + + plugin = AgentSkills(skills=["sandbox:/skills-a", "sandbox:/skills-b"]) + await plugin.init_agent(agent) + + agent_skills = plugin._get_skills(agent) + assert len(agent_skills) == 2 + assert "sandbox-skill" in agent_skills + assert "another-skill" in agent_skills + + @pytest.mark.asyncio + async def test_set_available_skills_rejects_sandbox(self): + """Test that set_available_skills raises on sandbox sources.""" + plugin = AgentSkills(skills=[]) + + with pytest.raises(ValueError, match="Sandbox sources"): + plugin.set_available_skills(["sandbox:/home/skills"]) + + @pytest.mark.asyncio + async def test_sandbox_skills_appear_in_system_prompt(self): + """Test that sandbox-loaded skills appear in the system prompt XML.""" + sandbox = _make_mock_sandbox( + files={"/home/skills/my-skill/SKILL.md": SKILL_CONTENT}, + dirs={ + "/home/skills": [FileInfo(name="my-skill", is_dir=True)], + }, + ) + agent = _mock_agent(sandbox=sandbox) + + plugin = AgentSkills(skills=["sandbox:/home/skills"]) + await plugin.init_agent(agent) + + # Simulate before_invocation hook — generate XML for this agent + xml = plugin._generate_skills_xml(agent) + + assert "sandbox-skill" in xml + assert "A skill loaded from sandbox" in xml + + @pytest.mark.asyncio + async def test_sandbox_skills_activatable_via_tool(self): + """Test that sandbox-loaded skills can be activated via the skills tool.""" + sandbox = _make_mock_sandbox( + files={"/home/skills/my-skill/SKILL.md": SKILL_CONTENT}, + dirs={ + "/home/skills": [FileInfo(name="my-skill", is_dir=True)], + }, + ) + agent = _mock_agent(sandbox=sandbox) + + plugin = AgentSkills(skills=["sandbox:/home/skills"]) + await plugin.init_agent(agent) + + # Create tool context and activate the skill + tool_use = {"toolUseId": "test-id", "name": "skills", "input": {}} + tool_context = ToolContext(tool_use=tool_use, agent=agent, invocation_state={"agent": agent}) + + result = plugin.skills(skill_name="sandbox-skill", tool_context=tool_context) + + assert "Follow these steps" in result + + @pytest.mark.asyncio + async def test_no_sandbox_sources_stays_sync(self): + """Test that plugin with no sandbox sources works fine with sync init.""" + skill = Skill(name="local", description="Local skill", instructions="Do it") + plugin = AgentSkills(skills=[skill]) + agent = _mock_agent() + + # init_agent should work even though it's async + await plugin.init_agent(agent) + + assert "local" in plugin._get_skills(agent) + assert len(plugin._sandbox_sources) == 0 + + @pytest.mark.asyncio + async def test_sandbox_source_string_format(self): + """Test various sandbox: source string formats.""" + plugin = AgentSkills( + skills=[ + "sandbox:/home/skills", + "sandbox:/absolute/path/to/skills", + "sandbox:/tmp/skills/", + ] + ) + + assert len(plugin._sandbox_sources) == 3 + assert plugin._sandbox_sources[0] == "/home/skills" + assert plugin._sandbox_sources[1] == "/absolute/path/to/skills" + assert plugin._sandbox_sources[2] == "/tmp/skills/" + + +class TestAgentSkillsSandboxPluginRegistry: + """Test that sandbox skills work through the plugin registry (async init_agent).""" + + @pytest.mark.asyncio + async def test_plugin_registry_handles_async_init(self): + """Test that _PluginRegistry correctly handles async init_agent from AgentSkills.""" + from strands.plugins.registry import _PluginRegistry + + sandbox = _make_mock_sandbox( + files={"/home/skills/my-skill/SKILL.md": SKILL_CONTENT}, + dirs={ + "/home/skills": [FileInfo(name="my-skill", is_dir=True)], + }, + ) + agent = _mock_agent(sandbox=sandbox) + + plugin = AgentSkills(skills=["sandbox:/home/skills"]) + + # The registry should handle the async init_agent + registry = _PluginRegistry(agent) + registry.add_and_init(plugin) + + # After registry init, sandbox skills should be loaded for this agent + assert "sandbox-skill" in plugin._get_skills(agent) + + @pytest.mark.asyncio + async def test_get_available_skills_after_sandbox_load(self): + """Test get_available_skills returns sandbox-loaded skills.""" + sandbox = _make_mock_sandbox( + files={"/home/skills/my-skill/SKILL.md": SKILL_CONTENT}, + dirs={ + "/home/skills": [FileInfo(name="my-skill", is_dir=True)], + }, + ) + agent = _mock_agent(sandbox=sandbox) + + plugin = AgentSkills(skills=["sandbox:/home/skills"]) + await plugin.init_agent(agent) + + available = plugin.get_available_skills(agent) + assert len(available) == 1 + assert available[0].name == "sandbox-skill" + + +# --- Tests for multi-agent isolation --- + + +class TestMultiAgentIsolation: + """Tests proving that per-agent skill catalogs are isolated.""" + + @pytest.mark.asyncio + async def test_two_agents_different_sandbox_skills(self): + """Test that two agents with different sandboxes get different skill catalogs.""" + # Agent A has Docker sandbox with docker-skill + sandbox_a = _make_mock_sandbox( + files={"/home/skills/docker/SKILL.md": SKILL_C_CONTENT}, + dirs={"/home/skills": [FileInfo(name="docker", is_dir=True)]}, + ) + agent_a = _mock_agent(sandbox=sandbox_a) + + # Agent B has S3 sandbox with s3-skill + sandbox_b = _make_mock_sandbox( + files={"/home/skills/s3/SKILL.md": SKILL_D_CONTENT}, + dirs={"/home/skills": [FileInfo(name="s3", is_dir=True)]}, + ) + agent_b = _mock_agent(sandbox=sandbox_b) + + # SAME plugin instance + plugin = AgentSkills(skills=["sandbox:/home/skills"]) + + await plugin.init_agent(agent_a) + await plugin.init_agent(agent_b) + + # Agent A should ONLY see docker-skill + skills_a = plugin._get_skills(agent_a) + assert "docker-skill" in skills_a + assert "s3-skill" not in skills_a + + # Agent B should ONLY see s3-skill + skills_b = plugin._get_skills(agent_b) + assert "s3-skill" in skills_b + assert "docker-skill" not in skills_b + + @pytest.mark.asyncio + async def test_two_agents_share_base_skills(self): + """Test that base (sync) skills are shared across agents.""" + base_skill = Skill(name="shared-skill", description="Shared", instructions="Shared instructions") + + sandbox_a = _make_mock_sandbox( + files={"/skills/a/SKILL.md": SKILL_C_CONTENT}, + dirs={"/skills": [FileInfo(name="a", is_dir=True)]}, + ) + agent_a = _mock_agent(sandbox=sandbox_a) + + sandbox_b = _make_mock_sandbox( + files={"/skills/b/SKILL.md": SKILL_D_CONTENT}, + dirs={"/skills": [FileInfo(name="b", is_dir=True)]}, + ) + agent_b = _mock_agent(sandbox=sandbox_b) + + plugin = AgentSkills(skills=[base_skill, "sandbox:/skills"]) + + await plugin.init_agent(agent_a) + await plugin.init_agent(agent_b) + + # Both agents should have the shared base skill + assert "shared-skill" in plugin._get_skills(agent_a) + assert "shared-skill" in plugin._get_skills(agent_b) + + # But each should also have their own sandbox skill + assert "docker-skill" in plugin._get_skills(agent_a) + assert "docker-skill" not in plugin._get_skills(agent_b) + assert "s3-skill" in plugin._get_skills(agent_b) + assert "s3-skill" not in plugin._get_skills(agent_a) + + @pytest.mark.asyncio + async def test_two_agents_overlapping_skill_names_stay_isolated(self): + """Test that two agents with same skill name from different sandboxes get different content.""" + # Both sandboxes have a skill named "sandbox-skill" but with different content + sandbox_a = _make_mock_sandbox( + files={"/skills/a/SKILL.md": SKILL_CONTENT}, # sandbox-skill: "A skill loaded from sandbox" + dirs={"/skills": [FileInfo(name="a", is_dir=True)]}, + ) + agent_a = _mock_agent(sandbox=sandbox_a) + + modified_content = """--- +name: sandbox-skill +description: MODIFIED sandbox skill from agent B +--- +# Modified Instructions + +These are different instructions. +""" + sandbox_b = _make_mock_sandbox( + files={"/skills/b/SKILL.md": modified_content}, + dirs={"/skills": [FileInfo(name="b", is_dir=True)]}, + ) + agent_b = _mock_agent(sandbox=sandbox_b) + + plugin = AgentSkills(skills=["sandbox:/skills"]) + + await plugin.init_agent(agent_a) + await plugin.init_agent(agent_b) + + # Same name, different descriptions + assert plugin._get_skills(agent_a)["sandbox-skill"].description == "A skill loaded from sandbox" + assert plugin._get_skills(agent_b)["sandbox-skill"].description == "MODIFIED sandbox skill from agent B" + + @pytest.mark.asyncio + async def test_agent_skills_tool_uses_per_agent_catalog(self): + """Test that the skills tool returns the correct skill for each agent.""" + sandbox_a = _make_mock_sandbox( + files={"/skills/a/SKILL.md": SKILL_C_CONTENT}, + dirs={"/skills": [FileInfo(name="a", is_dir=True)]}, + ) + agent_a = _mock_agent(sandbox=sandbox_a) + + sandbox_b = _make_mock_sandbox( + files={"/skills/b/SKILL.md": SKILL_D_CONTENT}, + dirs={"/skills": [FileInfo(name="b", is_dir=True)]}, + ) + agent_b = _mock_agent(sandbox=sandbox_b) + + plugin = AgentSkills(skills=["sandbox:/skills"]) + await plugin.init_agent(agent_a) + await plugin.init_agent(agent_b) + + # Agent A can activate docker-skill but NOT s3-skill + tool_use = {"toolUseId": "test-id", "name": "skills", "input": {}} + ctx_a = ToolContext(tool_use=tool_use, agent=agent_a, invocation_state={"agent": agent_a}) + result_a = plugin.skills(skill_name="docker-skill", tool_context=ctx_a) + assert "Docker-specific instructions" in result_a + + result_a_missing = plugin.skills(skill_name="s3-skill", tool_context=ctx_a) + assert "not found" in result_a_missing + + # Agent B can activate s3-skill but NOT docker-skill + ctx_b = ToolContext(tool_use=tool_use, agent=agent_b, invocation_state={"agent": agent_b}) + result_b = plugin.skills(skill_name="s3-skill", tool_context=ctx_b) + assert "S3-specific instructions" in result_b + + result_b_missing = plugin.skills(skill_name="docker-skill", tool_context=ctx_b) + assert "not found" in result_b_missing + + @pytest.mark.asyncio + async def test_generate_skills_xml_per_agent(self): + """Test that _generate_skills_xml returns per-agent XML.""" + sandbox_a = _make_mock_sandbox( + files={"/skills/a/SKILL.md": SKILL_C_CONTENT}, + dirs={"/skills": [FileInfo(name="a", is_dir=True)]}, + ) + agent_a = _mock_agent(sandbox=sandbox_a) + + sandbox_b = _make_mock_sandbox( + files={"/skills/b/SKILL.md": SKILL_D_CONTENT}, + dirs={"/skills": [FileInfo(name="b", is_dir=True)]}, + ) + agent_b = _mock_agent(sandbox=sandbox_b) + + plugin = AgentSkills(skills=["sandbox:/skills"]) + await plugin.init_agent(agent_a) + await plugin.init_agent(agent_b) + + xml_a = plugin._generate_skills_xml(agent_a) + xml_b = plugin._generate_skills_xml(agent_b) + + assert "docker-skill" in xml_a + assert "s3-skill" not in xml_a + + assert "s3-skill" in xml_b + assert "docker-skill" not in xml_b + + @pytest.mark.asyncio + async def test_get_available_skills_with_agent(self): + """Test get_available_skills with agent parameter returns per-agent skills.""" + sandbox = _make_mock_sandbox( + files={"/skills/a/SKILL.md": SKILL_C_CONTENT}, + dirs={"/skills": [FileInfo(name="a", is_dir=True)]}, + ) + agent = _mock_agent(sandbox=sandbox) + + plugin = AgentSkills(skills=["sandbox:/skills"]) + await plugin.init_agent(agent) + + # With agent — returns per-agent catalog + with_agent = plugin.get_available_skills(agent) + assert len(with_agent) == 1 + assert with_agent[0].name == "docker-skill" + + # Without agent — returns base (empty since all sources are sandbox) + without_agent = plugin.get_available_skills() + assert len(without_agent) == 0 + + @pytest.mark.asyncio + async def test_base_skills_not_mutated_by_init_agent(self): + """Test that init_agent doesn't mutate base_skills.""" + base = Skill(name="base-skill", description="Base", instructions="Base") + sandbox = _make_mock_sandbox( + files={"/skills/a/SKILL.md": SKILL_C_CONTENT}, + dirs={"/skills": [FileInfo(name="a", is_dir=True)]}, + ) + agent = _mock_agent(sandbox=sandbox) + + plugin = AgentSkills(skills=[base, "sandbox:/skills"]) + + # Before init + assert list(plugin._base_skills.keys()) == ["base-skill"] + + await plugin.init_agent(agent) + + # Base skills should NOT have sandbox skills added + assert list(plugin._base_skills.keys()) == ["base-skill"] + # But per-agent should have both + assert "base-skill" in plugin._get_skills(agent) + assert "docker-skill" in plugin._get_skills(agent) + + @pytest.mark.asyncio + async def test_set_available_skills_clears_per_agent_caches(self): + """Test that set_available_skills clears per-agent caches.""" + sandbox = _make_mock_sandbox( + files={"/skills/a/SKILL.md": SKILL_C_CONTENT}, + dirs={"/skills": [FileInfo(name="a", is_dir=True)]}, + ) + agent = _mock_agent(sandbox=sandbox) + + plugin = AgentSkills(skills=["sandbox:/skills"]) + await plugin.init_agent(agent) + + assert "docker-skill" in plugin._get_skills(agent) + + # Replace skills — should clear per-agent cache + new_skill = Skill(name="new-skill", description="New", instructions="New") + plugin.set_available_skills([new_skill]) + + # Per-agent cache cleared — falls back to new base + assert "docker-skill" not in plugin._get_skills(agent) + assert "new-skill" in plugin._get_skills(agent) + + @pytest.mark.asyncio + async def test_no_sandbox_agent_gets_base_skills_only(self): + """Test that an agent without sandbox sources gets base skills only.""" + base = Skill(name="base-skill", description="Base", instructions="Base") + plugin = AgentSkills(skills=[base]) + agent = _mock_agent() + + await plugin.init_agent(agent) + + skills = plugin._get_skills(agent) + assert len(skills) == 1 + assert "base-skill" in skills From bbcbd1fb0354fce5f8bf3c40d6ff98008fc9fe0f Mon Sep 17 00:00:00 2001 From: agent-of-mkmeral Date: Tue, 28 Apr 2026 18:41:13 +0000 Subject: [PATCH 2/4] =?UTF-8?q?fix:=20address=20review=20feedback=20?= =?UTF-8?q?=E2=80=94=20URI=20format,=20cache-point=20regression,=20edge=20?= =?UTF-8?q?cases?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Changes: 1. URI format: sandbox:/ → sandbox:/// (RFC 8089-style, per SDK tenet #6) 2. Regression fix: restore system_prompt_content dual-path in _on_before_invocation to preserve cache points and structured blocks 3. Edge case: warn when get_available_skills() called before init_agent() with sandbox sources configured 4. Early guard: check agent.sandbox existence before resolving sandbox skills 5. Remove unnecessary assert in Skill.from_sandbox (dead code after guard) 6. Optimize from_sandbox: use read_text directly (halves sandbox I/O) 7. Fix missing blank line between from_directory and from_sandbox (PEP 8) 8. Restore test_preserves_cache_points_in_system_prompt test 9. Update test mock to handle list-based system_prompt setter All 175 tests passing. --- .../vended_plugins/skills/agent_skills.py | 87 ++++++++++++------- src/strands/vended_plugins/skills/skill.py | 43 +++++---- .../skills/test_agent_skills.py | 50 +++++++++-- .../skills/test_sandbox_skills.py | 48 +++++----- 4 files changed, 147 insertions(+), 81 deletions(-) diff --git a/src/strands/vended_plugins/skills/agent_skills.py b/src/strands/vended_plugins/skills/agent_skills.py index 1585849b1..8286cde36 100644 --- a/src/strands/vended_plugins/skills/agent_skills.py +++ b/src/strands/vended_plugins/skills/agent_skills.py @@ -4,7 +4,7 @@ to add Agent Skills support. The plugin registers a tool for activating skills, and injects skill metadata into the system prompt. -Sandbox skill sources (e.g., ``"sandbox:/home/skills"``) are loaded +Sandbox skill sources (e.g., ``"sandbox:///home/skills"``) are loaded asynchronously during ``init_agent()`` using the agent's sandbox instance, following the same deferred-loading pattern as the TypeScript SDK. @@ -24,6 +24,7 @@ from ...hooks.events import BeforeInvocationEvent from ...plugins import Plugin, hook from ...tools.decorator import tool +from ...types.content import SystemContentBlock from ...types.tools import ToolContext from .skill import Skill @@ -35,10 +36,10 @@ _DEFAULT_STATE_KEY = "agent_skills" _RESOURCE_DIRS = ("scripts", "references", "assets") _DEFAULT_MAX_RESOURCE_FILES = 20 -_SANDBOX_PREFIX = "sandbox:" +_SANDBOX_PREFIX = "sandbox://" SkillSource: TypeAlias = str | Path | Skill -"""A single skill source: path string, Path object, Skill instance, or ``"sandbox:/path"`` string.""" +"""A single skill source: path string, Path object, Skill instance, or ``"sandbox:///path"`` string.""" SkillSources: TypeAlias = SkillSource | list[SkillSource] """One or more skill sources.""" @@ -61,7 +62,7 @@ class AgentSkills(Plugin): 3. Session persistence of active skill state via ``agent.state`` Skills can be provided as filesystem paths (to individual skill directories or - parent directories containing multiple skills), ``"sandbox:/path"`` URIs that + parent directories containing multiple skills), ``"sandbox:///path"`` URIs that load skills from the agent's sandbox at init time, or pre-built ``Skill`` instances. Sandbox sources are loaded asynchronously during ``init_agent()`` using the @@ -81,10 +82,10 @@ class AgentSkills(Plugin): plugin = AgentSkills(skills=["./skills/pdf-processing", "./skills/"]) # Load from agent's sandbox (resolved at agent init) - plugin = AgentSkills(skills=["sandbox:/home/skills"]) + plugin = AgentSkills(skills=["sandbox:///home/skills"]) # Mix local and sandbox sources - plugin = AgentSkills(skills=["./local-skills/", "sandbox:/home/skills"]) + plugin = AgentSkills(skills=["./local-skills/", "sandbox:///home/skills"]) # Or provide Skill instances directly skill = Skill(name="my-skill", description="A custom skill", instructions="Do the thing") @@ -111,7 +112,7 @@ def __init__( Synchronous sources (filesystem paths, ``Skill`` instances, HTTPS URLs) are resolved immediately into the base skill set. Sandbox sources - (``"sandbox:/path"``) are stored as pending and resolved per-agent + (``"sandbox:///path"``) are stored as pending and resolved per-agent during ``init_agent()`` when each agent's sandbox is available. Args: @@ -121,7 +122,7 @@ def __init__( - A ``str`` or ``Path`` to a parent directory (containing skill subdirectories) - A ``Skill`` dataclass instance - An ``https://`` URL pointing directly to raw SKILL.md content - - A ``"sandbox:/path"`` URI to load from the agent's sandbox at init time + - A ``"sandbox:///path"`` URI to load from the agent's sandbox at init time state_key: Key used to store plugin state in ``agent.state``. max_resource_files: Maximum number of resource files to list in skill responses. strict: If True, raise on skill validation issues. If False (default), warn and load anyway. @@ -137,7 +138,7 @@ def __init__( for source in all_sources: if isinstance(source, str) and source.startswith(_SANDBOX_PREFIX): - self._sandbox_sources.append(source[len(_SANDBOX_PREFIX) :]) + self._sandbox_sources.append(source[len(_SANDBOX_PREFIX):]) else: sync_sources.append(source) @@ -164,6 +165,10 @@ def _get_skills(self, agent: Agent | None = None) -> dict[str, Skill]: """ if agent is not None and agent in self._agent_skills: return self._agent_skills[agent] + if agent is not None and agent not in self._agent_skills and self._sandbox_sources: + logger.warning( + "agent has not been initialized yet — sandbox skills are not available; returning base skills only" + ) return self._base_skills async def init_agent(self, agent: Agent) -> None: @@ -204,9 +209,16 @@ async def _resolve_sandbox_skills(self, agent: Agent) -> dict[str, Skill]: Returns: Dict mapping skill names to Skill instances loaded from the sandbox. """ - sandbox = agent.sandbox resolved: dict[str, Skill] = {} + sandbox = getattr(agent, "sandbox", None) + if sandbox is None: + logger.warning( + "agent has no sandbox configured — skipping %d sandbox skill source(s)", + len(self._sandbox_sources), + ) + return resolved + for sandbox_path in self._sandbox_sources: logger.debug("sandbox_path=<%s> | resolving sandbox skill source", sandbox_path) @@ -272,34 +284,51 @@ def skills(self, skill_name: str, tool_context: ToolContext) -> str: # noqa: D4 def _on_before_invocation(self, event: BeforeInvocationEvent) -> None: """Inject skill metadata into the system prompt before each invocation. - Removes the previously injected XML block (if any) via exact string - replacement, then appends a fresh one. Uses agent state to track the - injected XML per-agent, so a single plugin instance can be shared - across multiple agents safely. + Removes the previously injected XML block (if any) via exact match, + then appends a fresh one. Uses agent state to track the injected XML + per-agent, so a single plugin instance can be shared across multiple + agents safely. + + When the agent has a structured system prompt (list of SystemContentBlock), + the injection is done at the block level so that cache points and other + structured blocks are preserved. Otherwise falls back to string manipulation. Args: event: The before-invocation event containing the agent reference. """ agent = event.agent - current_prompt = agent.system_prompt or "" - - # Remove the previously injected XML block by exact match state_data = agent.state.get(self._state_key) last_injected_xml = state_data.get("last_injected_xml") if isinstance(state_data, dict) else None - if last_injected_xml is not None: - if last_injected_xml in current_prompt: - current_prompt = current_prompt.replace(last_injected_xml, "") - else: - logger.warning("unable to find previously injected skills XML in system prompt, re-appending") skills_xml = self._generate_skills_xml(agent) - injection = f"\n\n{skills_xml}" - new_prompt = f"{current_prompt}{injection}" if current_prompt else skills_xml - - new_injected_xml = injection if current_prompt else skills_xml - self._set_state_field(agent, "last_injected_xml", new_injected_xml) - agent.system_prompt = new_prompt + content = agent.system_prompt_content + + if content is not None: + # Content-block path: preserve cache points and other structured blocks + blocks: list[SystemContentBlock] = list(content) + if last_injected_xml is not None: + injected_block: SystemContentBlock = {"text": last_injected_xml} + if injected_block in blocks: + blocks.remove(injected_block) + else: + logger.warning("unable to find previously injected skills XML in system prompt, re-appending") + blocks.append({"text": skills_xml}) + self._set_state_field(agent, "last_injected_xml", skills_xml) + agent.system_prompt = blocks + else: + # String path: legacy behaviour for plain-string system prompts + current_prompt = agent.system_prompt or "" + if last_injected_xml is not None: + if last_injected_xml in current_prompt: + current_prompt = current_prompt.replace(last_injected_xml, "") + else: + logger.warning("unable to find previously injected skills XML in system prompt, re-appending") + injection = f"\n\n{skills_xml}" + new_prompt = f"{current_prompt}{injection}" if current_prompt else skills_xml + new_injected_xml = injection if current_prompt else skills_xml + self._set_state_field(agent, "last_injected_xml", new_injected_xml) + agent.system_prompt = new_prompt def get_available_skills(self, agent: Agent | None = None) -> list[Skill]: """Get the list of available skills. @@ -324,7 +353,7 @@ def set_available_skills(self, skills: SkillSources) -> None: parent directory containing skill subdirectories, or an ``https://`` URL pointing directly to raw SKILL.md content. - Note: Sandbox sources (``"sandbox:/path"``) are NOT supported in + Note: Sandbox sources (``"sandbox:///path"``) are NOT supported in ``set_available_skills`` because no agent context is available. Use ``"sandbox:..."`` sources in the constructor instead. diff --git a/src/strands/vended_plugins/skills/skill.py b/src/strands/vended_plugins/skills/skill.py index 2355d05da..e04a10e04 100644 --- a/src/strands/vended_plugins/skills/skill.py +++ b/src/strands/vended_plugins/skills/skill.py @@ -425,6 +425,7 @@ def from_directory(cls, skills_dir: str | Path, *, strict: bool = False) -> list logger.debug("path=<%s>, count=<%d> | loaded skills from directory", skills_dir, len(skills)) return skills + @classmethod async def from_sandbox(cls, sandbox: Sandbox, skill_path: str, *, strict: bool = False) -> Skill: """Load a single skill from a sandbox filesystem. @@ -455,30 +456,26 @@ async def from_sandbox(cls, sandbox: Sandbox, skill_path: str, *, strict: bool = """ # Normalize path — try SKILL.md first, then skill.md if skill_path.lower().endswith("skill.md"): - skill_md_path = skill_path - else: - # Try to find SKILL.md in the directory - skill_md_path = None - for name in ("SKILL.md", "skill.md"): - candidate = f"{skill_path.rstrip('/')}/{name}" - try: - await sandbox.read_file(candidate) - skill_md_path = candidate - break - except (FileNotFoundError, OSError, NotImplementedError): - continue - - if skill_md_path is None: - raise FileNotFoundError(f"path=<{skill_path}> | no SKILL.md found in sandbox skill directory") - - logger.debug("sandbox_path=<%s> | loading skill from sandbox", skill_md_path) - - assert skill_md_path is not None # guaranteed by the FileNotFoundError above - content = await sandbox.read_text(skill_md_path) - skill = cls.from_content(content, strict=strict) + # Direct path to SKILL.md — read it directly + logger.debug("sandbox_path=<%s> | loading skill from sandbox", skill_path) + content = await sandbox.read_text(skill_path) + skill = cls.from_content(content, strict=strict) + logger.debug("name=<%s>, sandbox_path=<%s> | skill loaded from sandbox", skill.name, skill_path) + return skill + + # Try to find and read SKILL.md in the directory (single I/O per attempt) + for name in ("SKILL.md", "skill.md"): + candidate = f"{skill_path.rstrip('/')}/{name}" + try: + content = await sandbox.read_text(candidate) + logger.debug("sandbox_path=<%s> | loading skill from sandbox", candidate) + skill = cls.from_content(content, strict=strict) + logger.debug("name=<%s>, sandbox_path=<%s> | skill loaded from sandbox", skill.name, candidate) + return skill + except (FileNotFoundError, OSError, NotImplementedError): + continue - logger.debug("name=<%s>, sandbox_path=<%s> | skill loaded from sandbox", skill.name, skill_md_path) - return skill + raise FileNotFoundError(f"path=<{skill_path}> | no SKILL.md found in sandbox skill directory") @classmethod async def from_sandbox_directory( diff --git a/tests/strands/vended_plugins/skills/test_agent_skills.py b/tests/strands/vended_plugins/skills/test_agent_skills.py index 4a6363f50..092ae7542 100644 --- a/tests/strands/vended_plugins/skills/test_agent_skills.py +++ b/tests/strands/vended_plugins/skills/test_agent_skills.py @@ -34,11 +34,12 @@ def _mock_agent(): agent._system_prompt = "You are an agent." agent._system_prompt_content = [{"text": "You are an agent."}] - # Make system_prompt property behave like the real Agent + # Make system_prompt and system_prompt_content properties behave like the real Agent type(agent).system_prompt = property( lambda self: self._system_prompt, lambda self, value: _set_system_prompt(self, value), ) + type(agent).system_prompt_content = property(lambda self: self._system_prompt_content) agent.hooks = HookRegistry() agent.add_hook = MagicMock( @@ -61,16 +62,23 @@ def _mock_tool_context(agent: MagicMock) -> ToolContext: return ToolContext(tool_use=tool_use, agent=agent, invocation_state={"agent": agent}) -def _set_system_prompt(agent: MagicMock, value: str | None) -> None: +def _set_system_prompt(agent: MagicMock, value: str | list | None) -> None: """Simulate the Agent.system_prompt setter.""" if isinstance(value, str): agent._system_prompt = value agent._system_prompt_content = [{"text": value}] + elif isinstance(value, list): + # Content-block path: list of SystemContentBlock dicts + agent._system_prompt_content = value + # Derive the string prompt from text blocks + text_parts = [block["text"] for block in value if "text" in block] + agent._system_prompt = "\n\n".join(text_parts) elif value is None: agent._system_prompt = None agent._system_prompt_content = None + class TestSkillsPluginInit: """Tests for AgentSkills initialization.""" @@ -420,9 +428,41 @@ def test_uses_public_system_prompt_setter(self): event = BeforeInvocationEvent(agent=agent) plugin._on_before_invocation(event) - # The public setter should have been used, so _system_prompt_content - # should be consistent with _system_prompt - assert agent._system_prompt_content == [{"text": agent._system_prompt}] + # The public setter should have been used via the content-block path: + # original block is preserved and the skills XML is appended as a new block. + assert len(agent.system_prompt_content) == 2 + assert agent.system_prompt_content[0] == {"text": "Original."} + assert "" in agent.system_prompt_content[1]["text"] + + def test_preserves_cache_points_in_system_prompt(self): + """Test that cachePoint blocks in the system prompt are preserved after injection.""" + plugin = AgentSkills(skills=[_make_skill()]) + agent = _mock_agent() + agent._system_prompt = "Base instructions." + agent._system_prompt_content = [ + {"text": "Base instructions."}, + {"cachePoint": {"type": "default"}}, + ] + + expected_skills_xml = plugin._generate_skills_xml() + + event = BeforeInvocationEvent(agent=agent) + plugin._on_before_invocation(event) + + # Exact block structure: original text, cachePoint, skills XML + assert agent.system_prompt_content == [ + {"text": "Base instructions."}, + {"cachePoint": {"type": "default"}}, + {"text": expected_skills_xml}, + ] + + # Repeated invocation: identical result, no accumulation + plugin._on_before_invocation(event) + assert agent.system_prompt_content == [ + {"text": "Base instructions."}, + {"cachePoint": {"type": "default"}}, + {"text": expected_skills_xml}, + ] def test_warns_when_previous_xml_not_found(self, caplog): """Test that a warning is logged when the previously injected XML is missing from the prompt.""" diff --git a/tests/strands/vended_plugins/skills/test_sandbox_skills.py b/tests/strands/vended_plugins/skills/test_sandbox_skills.py index ae49fe876..a50ac9e7f 100644 --- a/tests/strands/vended_plugins/skills/test_sandbox_skills.py +++ b/tests/strands/vended_plugins/skills/test_sandbox_skills.py @@ -3,7 +3,7 @@ Tests cover: - Skill.from_sandbox() — loading a single skill from sandbox - Skill.from_sandbox_directory() — loading multiple skills from sandbox -- AgentSkills with "sandbox:/path" sources — deferred loading in init_agent +- AgentSkills with "sandbox:///path" sources — deferred loading in init_agent - Mixed local + sandbox sources - Error handling (missing files, invalid content, sandbox failures) - Multi-agent isolation with different sandboxes @@ -370,7 +370,7 @@ class TestAgentSkillsSandboxSources: @pytest.mark.asyncio async def test_sandbox_source_parsed_from_constructor(self): """Test that 'sandbox:' prefixed sources are stored as pending.""" - plugin = AgentSkills(skills=["sandbox:/home/skills"]) + plugin = AgentSkills(skills=["sandbox:///home/skills"]) assert len(plugin._sandbox_sources) == 1 assert plugin._sandbox_sources[0] == "/home/skills" @@ -388,7 +388,7 @@ async def test_sandbox_skills_loaded_in_init_agent(self): }, ) agent = _mock_agent(sandbox=sandbox) - plugin = AgentSkills(skills=["sandbox:/home/skills"]) + plugin = AgentSkills(skills=["sandbox:///home/skills"]) assert len(plugin._base_skills) == 0 @@ -421,7 +421,7 @@ async def test_mixed_local_and_sandbox_sources(self, tmp_path): ) agent = _mock_agent(sandbox=sandbox) - plugin = AgentSkills(skills=[str(local_skill_dir), "sandbox:/home/skills"]) + plugin = AgentSkills(skills=[str(local_skill_dir), "sandbox:///home/skills"]) # Local skill resolved immediately into base assert "local-skill" in plugin._base_skills @@ -441,7 +441,7 @@ async def test_sandbox_single_skill_directory(self): ) agent = _mock_agent(sandbox=sandbox) - plugin = AgentSkills(skills=["sandbox:/home/skills/my-skill"]) + plugin = AgentSkills(skills=["sandbox:///home/skills/my-skill"]) await plugin.init_agent(agent) assert "sandbox-skill" in plugin._get_skills(agent) @@ -452,7 +452,7 @@ async def test_sandbox_source_not_found_warns(self, caplog): sandbox = _make_mock_sandbox() agent = _mock_agent(sandbox=sandbox) - plugin = AgentSkills(skills=["sandbox:/nonexistent"]) + plugin = AgentSkills(skills=["sandbox:///nonexistent"]) with caplog.at_level(logging.WARNING): await plugin.init_agent(agent) @@ -469,7 +469,7 @@ async def test_sandbox_source_duplicate_overwrites(self): # Pre-load a skill with the same name into base existing = Skill(name="sandbox-skill", description="Original", instructions="Old") - plugin = AgentSkills(skills=[existing, "sandbox:/sandbox/skills/dupe"]) + plugin = AgentSkills(skills=[existing, "sandbox:///sandbox/skills/dupe"]) await plugin.init_agent(agent) # Sandbox version should overwrite the base version in the per-agent catalog @@ -490,7 +490,7 @@ async def test_multiple_sandbox_sources(self): ) agent = _mock_agent(sandbox=sandbox) - plugin = AgentSkills(skills=["sandbox:/skills-a", "sandbox:/skills-b"]) + plugin = AgentSkills(skills=["sandbox:///skills-a", "sandbox:///skills-b"]) await plugin.init_agent(agent) agent_skills = plugin._get_skills(agent) @@ -504,7 +504,7 @@ async def test_set_available_skills_rejects_sandbox(self): plugin = AgentSkills(skills=[]) with pytest.raises(ValueError, match="Sandbox sources"): - plugin.set_available_skills(["sandbox:/home/skills"]) + plugin.set_available_skills(["sandbox:///home/skills"]) @pytest.mark.asyncio async def test_sandbox_skills_appear_in_system_prompt(self): @@ -517,7 +517,7 @@ async def test_sandbox_skills_appear_in_system_prompt(self): ) agent = _mock_agent(sandbox=sandbox) - plugin = AgentSkills(skills=["sandbox:/home/skills"]) + plugin = AgentSkills(skills=["sandbox:///home/skills"]) await plugin.init_agent(agent) # Simulate before_invocation hook — generate XML for this agent @@ -537,7 +537,7 @@ async def test_sandbox_skills_activatable_via_tool(self): ) agent = _mock_agent(sandbox=sandbox) - plugin = AgentSkills(skills=["sandbox:/home/skills"]) + plugin = AgentSkills(skills=["sandbox:///home/skills"]) await plugin.init_agent(agent) # Create tool context and activate the skill @@ -566,9 +566,9 @@ async def test_sandbox_source_string_format(self): """Test various sandbox: source string formats.""" plugin = AgentSkills( skills=[ - "sandbox:/home/skills", - "sandbox:/absolute/path/to/skills", - "sandbox:/tmp/skills/", + "sandbox:///home/skills", + "sandbox:///absolute/path/to/skills", + "sandbox:///tmp/skills/", ] ) @@ -594,7 +594,7 @@ async def test_plugin_registry_handles_async_init(self): ) agent = _mock_agent(sandbox=sandbox) - plugin = AgentSkills(skills=["sandbox:/home/skills"]) + plugin = AgentSkills(skills=["sandbox:///home/skills"]) # The registry should handle the async init_agent registry = _PluginRegistry(agent) @@ -614,7 +614,7 @@ async def test_get_available_skills_after_sandbox_load(self): ) agent = _mock_agent(sandbox=sandbox) - plugin = AgentSkills(skills=["sandbox:/home/skills"]) + plugin = AgentSkills(skills=["sandbox:///home/skills"]) await plugin.init_agent(agent) available = plugin.get_available_skills(agent) @@ -646,7 +646,7 @@ async def test_two_agents_different_sandbox_skills(self): agent_b = _mock_agent(sandbox=sandbox_b) # SAME plugin instance - plugin = AgentSkills(skills=["sandbox:/home/skills"]) + plugin = AgentSkills(skills=["sandbox:///home/skills"]) await plugin.init_agent(agent_a) await plugin.init_agent(agent_b) @@ -678,7 +678,7 @@ async def test_two_agents_share_base_skills(self): ) agent_b = _mock_agent(sandbox=sandbox_b) - plugin = AgentSkills(skills=[base_skill, "sandbox:/skills"]) + plugin = AgentSkills(skills=[base_skill, "sandbox:///skills"]) await plugin.init_agent(agent_a) await plugin.init_agent(agent_b) @@ -717,7 +717,7 @@ async def test_two_agents_overlapping_skill_names_stay_isolated(self): ) agent_b = _mock_agent(sandbox=sandbox_b) - plugin = AgentSkills(skills=["sandbox:/skills"]) + plugin = AgentSkills(skills=["sandbox:///skills"]) await plugin.init_agent(agent_a) await plugin.init_agent(agent_b) @@ -741,7 +741,7 @@ async def test_agent_skills_tool_uses_per_agent_catalog(self): ) agent_b = _mock_agent(sandbox=sandbox_b) - plugin = AgentSkills(skills=["sandbox:/skills"]) + plugin = AgentSkills(skills=["sandbox:///skills"]) await plugin.init_agent(agent_a) await plugin.init_agent(agent_b) @@ -777,7 +777,7 @@ async def test_generate_skills_xml_per_agent(self): ) agent_b = _mock_agent(sandbox=sandbox_b) - plugin = AgentSkills(skills=["sandbox:/skills"]) + plugin = AgentSkills(skills=["sandbox:///skills"]) await plugin.init_agent(agent_a) await plugin.init_agent(agent_b) @@ -799,7 +799,7 @@ async def test_get_available_skills_with_agent(self): ) agent = _mock_agent(sandbox=sandbox) - plugin = AgentSkills(skills=["sandbox:/skills"]) + plugin = AgentSkills(skills=["sandbox:///skills"]) await plugin.init_agent(agent) # With agent — returns per-agent catalog @@ -821,7 +821,7 @@ async def test_base_skills_not_mutated_by_init_agent(self): ) agent = _mock_agent(sandbox=sandbox) - plugin = AgentSkills(skills=[base, "sandbox:/skills"]) + plugin = AgentSkills(skills=[base, "sandbox:///skills"]) # Before init assert list(plugin._base_skills.keys()) == ["base-skill"] @@ -843,7 +843,7 @@ async def test_set_available_skills_clears_per_agent_caches(self): ) agent = _mock_agent(sandbox=sandbox) - plugin = AgentSkills(skills=["sandbox:/skills"]) + plugin = AgentSkills(skills=["sandbox:///skills"]) await plugin.init_agent(agent) assert "docker-skill" in plugin._get_skills(agent) From 2a7238fd08050f434653a9a74c1e17e4e9037e87 Mon Sep 17 00:00:00 2001 From: agent-of-mkmeral Date: Tue, 28 Apr 2026 19:21:23 +0000 Subject: [PATCH 3/4] fix: test fidelity and restore TestStringPathInjection coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Fix mock join separator: '\n\n'.join → '\n'.join to match real Agent._initialize_system_prompt (agent.py:1131) 2. Restore TestStringPathInjection class (2 tests) — covers the string-path fallback branch in _on_before_invocation when system_prompt_content is None All 177 tests passing across 3 test files. --- .../skills/test_agent_skills.py | 39 ++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/tests/strands/vended_plugins/skills/test_agent_skills.py b/tests/strands/vended_plugins/skills/test_agent_skills.py index 092ae7542..a0d6f3d1a 100644 --- a/tests/strands/vended_plugins/skills/test_agent_skills.py +++ b/tests/strands/vended_plugins/skills/test_agent_skills.py @@ -72,7 +72,7 @@ def _set_system_prompt(agent: MagicMock, value: str | list | None) -> None: agent._system_prompt_content = value # Derive the string prompt from text blocks text_parts = [block["text"] for block in value if "text" in block] - agent._system_prompt = "\n\n".join(text_parts) + agent._system_prompt = "\n".join(text_parts) if text_parts else None elif value is None: agent._system_prompt = None agent._system_prompt_content = None @@ -484,6 +484,43 @@ def test_warns_when_previous_xml_not_found(self, caplog): assert "" in agent.system_prompt +class TestStringPathInjection: + """Tests for the string-path branch of _on_before_invocation (system_prompt_content is None).""" + + def test_string_path_replaces_previous_xml(self): + """Test that old injected XML is replaced when found in the string prompt.""" + plugin = AgentSkills(skills=[_make_skill()]) + agent = _mock_agent() + + old_xml = "\n\nxml" + agent._system_prompt = f"Base prompt.{old_xml}" + agent._system_prompt_content = None + agent.state.set(plugin._state_key, {"last_injected_xml": old_xml}) + + event = BeforeInvocationEvent(agent=agent) + plugin._on_before_invocation(event) + + assert "xml" not in agent.system_prompt + assert "" in agent.system_prompt + assert agent.system_prompt.startswith("Base prompt.") + + def test_string_path_warns_when_previous_xml_not_found(self, caplog): + """Test that a warning is logged when old XML is missing from the string prompt.""" + plugin = AgentSkills(skills=[_make_skill()]) + agent = _mock_agent() + + agent._system_prompt = "Totally new prompt." + agent._system_prompt_content = None + agent.state.set(plugin._state_key, {"last_injected_xml": "\n\nxml"}) + + event = BeforeInvocationEvent(agent=agent) + with caplog.at_level(logging.WARNING): + plugin._on_before_invocation(event) + + assert "unable to find previously injected skills XML in system prompt" in caplog.text + assert "" in agent.system_prompt + + class TestSkillsXmlGeneration: """Tests for _generate_skills_xml.""" From c1cfd800819db2c4f609e0b724c6cb81e6365e5d Mon Sep 17 00:00:00 2001 From: agent-of-mkmeral Date: Tue, 28 Apr 2026 21:02:33 +0000 Subject: [PATCH 4/4] style: apply ruff format to skills files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Minor whitespace and line-wrapping fixes: - Slice spacing: source[len(_SANDBOX_PREFIX):] → source[len(_SANDBOX_PREFIX) :] - Unwrap short logger.warning calls that fit in one line - Unwrap short method signatures that fit in one line - Remove extra blank line in test file - Unwrap short _make_mock_sandbox() calls in tests All 177 tests passing. --- .../vended_plugins/skills/agent_skills.py | 6 ++-- src/strands/vended_plugins/skills/skill.py | 8 ++--- .../skills/test_agent_skills.py | 1 - .../skills/test_sandbox_skills.py | 36 +++++-------------- 4 files changed, 13 insertions(+), 38 deletions(-) diff --git a/src/strands/vended_plugins/skills/agent_skills.py b/src/strands/vended_plugins/skills/agent_skills.py index 8286cde36..0d21cc2d1 100644 --- a/src/strands/vended_plugins/skills/agent_skills.py +++ b/src/strands/vended_plugins/skills/agent_skills.py @@ -138,7 +138,7 @@ def __init__( for source in all_sources: if isinstance(source, str) and source.startswith(_SANDBOX_PREFIX): - self._sandbox_sources.append(source[len(_SANDBOX_PREFIX):]) + self._sandbox_sources.append(source[len(_SANDBOX_PREFIX) :]) else: sync_sources.append(source) @@ -239,9 +239,7 @@ async def _resolve_sandbox_skills(self, agent: Agent) -> dict[str, Skill]: skills = await Skill.from_sandbox_directory(sandbox, sandbox_path, strict=self._strict) for skill in skills: if skill.name in resolved: - logger.warning( - "name=<%s> | duplicate skill name, overwriting previous skill", skill.name - ) + logger.warning("name=<%s> | duplicate skill name, overwriting previous skill", skill.name) resolved[skill.name] = skill logger.debug( "sandbox_path=<%s>, count=<%d> | loaded skills from sandbox directory", diff --git a/src/strands/vended_plugins/skills/skill.py b/src/strands/vended_plugins/skills/skill.py index e04a10e04..b99a8c8eb 100644 --- a/src/strands/vended_plugins/skills/skill.py +++ b/src/strands/vended_plugins/skills/skill.py @@ -478,9 +478,7 @@ async def from_sandbox(cls, sandbox: Sandbox, skill_path: str, *, strict: bool = raise FileNotFoundError(f"path=<{skill_path}> | no SKILL.md found in sandbox skill directory") @classmethod - async def from_sandbox_directory( - cls, sandbox: Sandbox, skills_dir: str, *, strict: bool = False - ) -> list[Skill]: + async def from_sandbox_directory(cls, sandbox: Sandbox, skills_dir: str, *, strict: bool = False) -> list[Skill]: """Load all skills from a parent directory in a sandbox. Lists subdirectories in the given sandbox path and loads any that @@ -523,7 +521,5 @@ async def from_sandbox_directory( except (FileNotFoundError, ValueError, OSError, NotImplementedError) as e: logger.debug("sandbox_path=<%s> | skipping directory: %s", child_path, e) - logger.debug( - "sandbox_path=<%s>, count=<%d> | loaded skills from sandbox directory", skills_dir, len(skills) - ) + logger.debug("sandbox_path=<%s>, count=<%d> | loaded skills from sandbox directory", skills_dir, len(skills)) return skills diff --git a/tests/strands/vended_plugins/skills/test_agent_skills.py b/tests/strands/vended_plugins/skills/test_agent_skills.py index a0d6f3d1a..9311a2585 100644 --- a/tests/strands/vended_plugins/skills/test_agent_skills.py +++ b/tests/strands/vended_plugins/skills/test_agent_skills.py @@ -78,7 +78,6 @@ def _set_system_prompt(agent: MagicMock, value: str | list | None) -> None: agent._system_prompt_content = None - class TestSkillsPluginInit: """Tests for AgentSkills initialization.""" diff --git a/tests/strands/vended_plugins/skills/test_sandbox_skills.py b/tests/strands/vended_plugins/skills/test_sandbox_skills.py index a50ac9e7f..69677e9cb 100644 --- a/tests/strands/vended_plugins/skills/test_sandbox_skills.py +++ b/tests/strands/vended_plugins/skills/test_sandbox_skills.py @@ -141,9 +141,7 @@ class TestSkillFromSandbox: @pytest.mark.asyncio async def test_load_skill_from_sandbox_directory(self): """Test loading a skill from a directory containing SKILL.md.""" - sandbox = _make_mock_sandbox( - files={"/home/skills/my-skill/SKILL.md": SKILL_CONTENT} - ) + sandbox = _make_mock_sandbox(files={"/home/skills/my-skill/SKILL.md": SKILL_CONTENT}) skill = await Skill.from_sandbox(sandbox, "/home/skills/my-skill") @@ -155,9 +153,7 @@ async def test_load_skill_from_sandbox_directory(self): @pytest.mark.asyncio async def test_load_skill_from_direct_skill_md_path(self): """Test loading a skill by pointing directly to SKILL.md.""" - sandbox = _make_mock_sandbox( - files={"/home/skills/my-skill/SKILL.md": SKILL_CONTENT} - ) + sandbox = _make_mock_sandbox(files={"/home/skills/my-skill/SKILL.md": SKILL_CONTENT}) skill = await Skill.from_sandbox(sandbox, "/home/skills/my-skill/SKILL.md") @@ -166,9 +162,7 @@ async def test_load_skill_from_direct_skill_md_path(self): @pytest.mark.asyncio async def test_load_skill_lowercase_skill_md(self): """Test loading skill from skill.md (lowercase).""" - sandbox = _make_mock_sandbox( - files={"/home/skills/my-skill/skill.md": SKILL_CONTENT} - ) + sandbox = _make_mock_sandbox(files={"/home/skills/my-skill/skill.md": SKILL_CONTENT}) skill = await Skill.from_sandbox(sandbox, "/home/skills/my-skill") @@ -199,9 +193,7 @@ async def test_raises_when_no_skill_md(self): @pytest.mark.asyncio async def test_raises_on_invalid_content(self): """Test ValueError when SKILL.md has invalid content.""" - sandbox = _make_mock_sandbox( - files={"/home/skills/bad-skill/SKILL.md": INVALID_SKILL_CONTENT} - ) + sandbox = _make_mock_sandbox(files={"/home/skills/bad-skill/SKILL.md": INVALID_SKILL_CONTENT}) with pytest.raises(ValueError, match="name"): await Skill.from_sandbox(sandbox, "/home/skills/bad-skill") @@ -209,9 +201,7 @@ async def test_raises_on_invalid_content(self): @pytest.mark.asyncio async def test_path_trailing_slash(self): """Test that trailing slashes in path are handled correctly.""" - sandbox = _make_mock_sandbox( - files={"/home/skills/my-skill/SKILL.md": SKILL_CONTENT} - ) + sandbox = _make_mock_sandbox(files={"/home/skills/my-skill/SKILL.md": SKILL_CONTENT}) skill = await Skill.from_sandbox(sandbox, "/home/skills/my-skill/") @@ -226,9 +216,7 @@ async def test_strict_mode(self): --- # Instructions """ - sandbox = _make_mock_sandbox( - files={"/home/skills/bad/SKILL.md": content} - ) + sandbox = _make_mock_sandbox(files={"/home/skills/bad/SKILL.md": content}) with pytest.raises(ValueError, match="skill name"): await Skill.from_sandbox(sandbox, "/home/skills/bad", strict=True) @@ -298,9 +286,7 @@ async def test_skips_directories_without_skill_md(self): @pytest.mark.asyncio async def test_empty_directory(self): """Test loading from an empty directory.""" - sandbox = _make_mock_sandbox( - dirs={"/home/skills": []} - ) + sandbox = _make_mock_sandbox(dirs={"/home/skills": []}) skills = await Skill.from_sandbox_directory(sandbox, "/home/skills") @@ -436,9 +422,7 @@ async def test_mixed_local_and_sandbox_sources(self, tmp_path): @pytest.mark.asyncio async def test_sandbox_single_skill_directory(self): """Test sandbox source pointing to a single skill directory (not parent).""" - sandbox = _make_mock_sandbox( - files={"/home/skills/my-skill/SKILL.md": SKILL_CONTENT} - ) + sandbox = _make_mock_sandbox(files={"/home/skills/my-skill/SKILL.md": SKILL_CONTENT}) agent = _mock_agent(sandbox=sandbox) plugin = AgentSkills(skills=["sandbox:///home/skills/my-skill"]) @@ -462,9 +446,7 @@ async def test_sandbox_source_not_found_warns(self, caplog): @pytest.mark.asyncio async def test_sandbox_source_duplicate_overwrites(self): """Test that duplicate skill names from sandbox overwrite earlier ones.""" - sandbox = _make_mock_sandbox( - files={"/sandbox/skills/dupe/SKILL.md": SKILL_CONTENT} - ) + sandbox = _make_mock_sandbox(files={"/sandbox/skills/dupe/SKILL.md": SKILL_CONTENT}) agent = _mock_agent(sandbox=sandbox) # Pre-load a skill with the same name into base