From ab09246dc40a5c7b65ab675b1dc230210d7ea5f4 Mon Sep 17 00:00:00 2001 From: SergeyMenshykh <68852919+SergeyMenshykh@users.noreply.github.com> Date: Wed, 13 May 2026 21:35:52 +0100 Subject: [PATCH 1/8] [Python] [Breaking] Extract skill spec metadata into SkillFrontmatter (#5775) * Fix Skill docstring consistency and spelling - Add ClassSkill to Skill class docstring concrete implementations list - Normalize 'defence' to 'defense' for American English consistency - Remove extra blank line in InlineSkill docstring example Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix E501 line-too-long lint error in test_skills.py Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix stale test section header to reflect SkillFrontmatter API Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix metadata children overriding top-level frontmatter fields Scope YAML_KV_RE to column-0 keys only so indented children under metadata: are not mistakenly parsed as top-level fields. Add regression test and spec fields to sample SKILL.md files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- python/packages/core/AGENTS.md | 3 +- .../packages/core/agent_framework/__init__.py | 2 + .../packages/core/agent_framework/_skills.py | 341 +++++--- .../packages/core/tests/core/test_skills.py | 802 ++++++++++++------ .../code_defined_skill/code_defined_skill.py | 7 +- .../skills/unit-converter/SKILL.md | 6 + .../skills/mixed_skills/mixed_skills.py | 14 +- .../skills/unit-converter/SKILL.md | 6 + .../skills/script_approval/script_approval.py | 7 +- .../skills/skill_filtering/skill_filtering.py | 2 +- .../skills/length-converter/SKILL.md | 6 + .../skills/volume-converter/SKILL.md | 6 + 12 files changed, 844 insertions(+), 358 deletions(-) diff --git a/python/packages/core/AGENTS.md b/python/packages/core/AGENTS.md index fafbc55f2f..edd4eaa158 100644 --- a/python/packages/core/AGENTS.md +++ b/python/packages/core/AGENTS.md @@ -69,7 +69,8 @@ agent_framework/ ### Skills (`_skills.py`) -- **`Skill`** - A skill definition bundling instructions (`content`) with metadata, resources, and scripts. Supports `@skill.resource` and `@skill.script` decorators for adding components. +- **`Skill`** - Abstract base for a skill definition bundling instructions (`content`) with frontmatter metadata, resources, and scripts. Concrete subclasses (`InlineSkill`, `FileSkill`, `ClassSkill`) accept a `frontmatter=SkillFrontmatter(...)` argument carrying the spec fields. Adding new spec fields is done in one place — on `SkillFrontmatter` — keeping the subclass constructors stable. +- **`SkillFrontmatter`** - L1 discovery metadata for a skill (`name`, `description`, `license`, `compatibility`, `allowed_tools`, `metadata`). All fields are mutable plain attributes; the constructor validates `name`, `description`, and `compatibility` against the spec but post-construction assignments are not re-validated. Spec fields are reachable on every skill via `skill.frontmatter`. - **`SkillResource`** - Named supplementary content attached to a skill; holds either static `content` or a dynamic `function` (sync or async). Exactly one must be provided. - **`SkillScript`** - An executable script attached to a skill; holds either an inline `function` (code-defined, runs in-process) or a `path` to a file on disk (file-based, delegated to a runner). Exactly one must be provided. - **`SkillScriptRunner`** - Protocol for file-based script execution. Any callable matching `(skill, script, args) -> Any` satisfies it. Code-defined scripts do not use a runner. diff --git a/python/packages/core/agent_framework/__init__.py b/python/packages/core/agent_framework/__init__.py index db1c43abfe..356051da3f 100644 --- a/python/packages/core/agent_framework/__init__.py +++ b/python/packages/core/agent_framework/__init__.py @@ -147,6 +147,7 @@ from ._skills import ( InlineSkillScript, InMemorySkillsSource, Skill, + SkillFrontmatter, SkillResource, SkillScript, SkillScriptRunner, @@ -432,6 +433,7 @@ __all__ = [ "SessionContext", "SingleEdgeGroup", "Skill", + "SkillFrontmatter", "SkillResource", "SkillScript", "SkillScriptRunner", diff --git a/python/packages/core/agent_framework/_skills.py b/python/packages/core/agent_framework/_skills.py index 44755a2efd..1128a938ea 100644 --- a/python/packages/core/agent_framework/_skills.py +++ b/python/packages/core/agent_framework/_skills.py @@ -465,41 +465,23 @@ class Skill(ABC): A skill represents a domain-specific capability with instructions, resources, and scripts. Concrete implementations include - :class:`FileSkill` (filesystem-backed) and :class:`InlineSkill` - (code-defined). + :class:`FileSkill` (filesystem-backed), :class:`InlineSkill` + (code-defined), and :class:`ClassSkill` (class-based). - Skill metadata follows the - `Agent Skills specification `_. - - Attributes: - name: Skill name (lowercase letters, numbers, hyphens only). - description: Human-readable description of the skill. + Skill spec metadata (name, description, license, compatibility, + allowed_tools, metadata) is exposed via the :attr:`frontmatter` + property, which returns a :class:`SkillFrontmatter` instance. """ - def __init__( - self, - *, - name: str, - description: str, - ) -> None: - """Initialize a Skill. + @property + @abstractmethod + def frontmatter(self) -> SkillFrontmatter: + """The L1 discovery metadata for this skill. - Validates the skill name and description against specification rules. - - Args: - name: Skill name (lowercase letters, numbers, hyphens only; - max 64 characters; no leading/trailing/consecutive hyphens). - description: Human-readable description of the skill - (≤1024 characters). - - Raises: - ValueError: If the name or description is invalid. + Contains the name, description, and other spec fields as defined by + the `Agent Skills specification `_. """ - _validate_skill_name(name) - _validate_skill_description(name, description) - - self.name = name - self.description = description + ... @property @abstractmethod @@ -535,6 +517,68 @@ class Skill(ABC): return [] +@experimental(feature_id=ExperimentalFeature.SKILLS) +class SkillFrontmatter: + """L1 discovery metadata for a :class:`Skill`. + + Encapsulates all `Agent Skills specification `_ + frontmatter fields in a single object. All fields are mutable plain + attributes; callers may freely reassign them after construction. + + The constructor validates ``name``, ``description``, and ``compatibility`` + against specification rules and raises :class:`ValueError` on invalid + input. Assignments made after construction are **not** re-validated; + callers are expected to honor the spec. + + Attributes: + name: Skill name (lowercase letters, numbers, hyphens only). + description: Human-readable description of the skill. + license: Optional license name or reference. + compatibility: Optional compatibility information (≤500 characters). + allowed_tools: Optional space-delimited pre-approved tool names. + metadata: Optional arbitrary key-value pairs (shallow-copied on + construction to avoid caller-owned dict aliasing). + """ + + def __init__( + self, + *, + name: str, + description: str, + license: str | None = None, + compatibility: str | None = None, + allowed_tools: str | None = None, + metadata: dict[str, str] | None = None, + ) -> None: + """Initialize a SkillFrontmatter. + + Args: + name: Skill name (lowercase letters, numbers, hyphens only; + max 64 characters; no leading/trailing/consecutive hyphens). + description: Human-readable description of the skill + (≤1024 characters). + license: Optional license name or reference. + compatibility: Optional compatibility information + (≤500 characters). + allowed_tools: Optional space-delimited pre-approved tool names. + metadata: Optional arbitrary key-value pairs. + + Raises: + ValueError: If the name, description, or compatibility is invalid. + """ + _validate_skill_name(name) + _validate_skill_description(name, description) + _validate_compatibility(compatibility) + + self.name = name + self.description = description + self.compatibility = compatibility + self.license = license + self.allowed_tools = allowed_tools + # Shallow-copy to avoid aliasing with caller-owned dict. + self.metadata: dict[str, str] | None = dict(metadata) if metadata is not None else None + + def _validate_skill_name(name: str) -> None: """Validate a skill name against specification rules. @@ -573,6 +617,21 @@ def _validate_skill_description(name: str, description: str) -> None: ) +def _validate_compatibility(compatibility: str | None) -> None: + """Validate an optional compatibility value against specification rules. + + Args: + compatibility: The optional compatibility value to validate. + + Raises: + ValueError: If the value exceeds the maximum allowed length. + """ + if compatibility is not None and len(compatibility) > MAX_COMPATIBILITY_LENGTH: + raise ValueError( + f"Skill compatibility must be {MAX_COMPATIBILITY_LENGTH} characters or fewer." + ) + + def _build_skill_content( name: str, description: str, @@ -639,23 +698,17 @@ class InlineSkill(Skill): All resources and scripts should be configured before the skill is registered with a :class:`SkillsProvider`. - Attributes: - name: Skill name (lowercase letters, numbers, hyphens only). - description: Human-readable description of the skill. - instructions: The skill instructions text. - Examples: - With the decorator: - .. code-block:: python skill = InlineSkill( - name="db-skill", - description="Database operations", + frontmatter=SkillFrontmatter( + name="db-skill", + description="Database operations", + ), instructions="Use this skill for DB tasks.", ) - @skill.resource def get_schema() -> str: return "CREATE TABLE ..." @@ -664,8 +717,7 @@ class InlineSkill(Skill): def __init__( self, *, - name: str, - description: str, + frontmatter: SkillFrontmatter, instructions: str, resources: Sequence[SkillResource] | None = None, scripts: Sequence[SkillScript] | None = None, @@ -673,19 +725,25 @@ class InlineSkill(Skill): """Initialize an InlineSkill. Args: - name: Skill name (lowercase letters, numbers, hyphens only). - description: Human-readable description of the skill (≤1024 chars). + frontmatter: Skill specification metadata (name, description, + and optional spec fields). Construct a :class:`SkillFrontmatter` + with the desired fields. instructions: The skill instructions text. resources: Pre-built resources to attach to this skill. scripts: Pre-built scripts to attach to this skill. """ - super().__init__(name=name, description=description) + self._frontmatter = frontmatter self.instructions = instructions self._resources: list[SkillResource] = list(resources) if resources is not None else [] self._scripts: list[SkillScript] = list(scripts) if scripts is not None else [] self._cached_content: str | None = None + @property + def frontmatter(self) -> SkillFrontmatter: + """The L1 discovery metadata for this skill.""" + return self._frontmatter + @property def content(self) -> str: """Synthesized XML content with name, description, instructions, resources, and scripts. @@ -697,7 +755,11 @@ class InlineSkill(Skill): return self._cached_content self._cached_content = _build_skill_content( - self.name, self.description, self.instructions, self._resources, self._scripts + self._frontmatter.name, + self._frontmatter.description, + self.instructions, + self._resources, + self._scripts, ) return self._cached_content @@ -932,10 +994,6 @@ class ClassSkill(Skill, ABC): Class-based skills can be distributed via shared libraries or PyPI packages, making them easy to reuse across projects. - Attributes: - name: Skill name (lowercase letters, numbers, hyphens only). - description: Human-readable description of the skill. - Examples: Decorator-based (recommended): @@ -944,8 +1002,10 @@ class ClassSkill(Skill, ABC): class UnitConverterSkill(ClassSkill): def __init__(self) -> None: super().__init__( - name="unit-converter", - description="Convert between common units.", + frontmatter=SkillFrontmatter( + name="unit-converter", + description="Convert between common units.", + ), ) @property @@ -967,8 +1027,10 @@ class ClassSkill(Skill, ABC): class UnitConverterSkill(ClassSkill): def __init__(self) -> None: super().__init__( - name="unit-converter", - description="Convert between common units.", + frontmatter=SkillFrontmatter( + name="unit-converter", + description="Convert between common units.", + ), ) @property @@ -989,22 +1051,25 @@ class ClassSkill(Skill, ABC): def __init__( self, *, - name: str, - description: str, + frontmatter: SkillFrontmatter, ) -> None: """Initialize a ClassSkill. Args: - name: Skill name (lowercase letters, numbers, hyphens only; - max 64 characters). - description: Human-readable description of the skill - (≤1024 characters). + frontmatter: Skill specification metadata (name, description, + and optional spec fields). Construct a :class:`SkillFrontmatter` + with the desired fields. """ - super().__init__(name=name, description=description) + self._frontmatter = frontmatter self._cached_content: str | None = None self._cached_resources: list[SkillResource] | None = None self._cached_scripts: list[SkillScript] | None = None + @property + def frontmatter(self) -> SkillFrontmatter: + """The L1 discovery metadata for this skill.""" + return self._frontmatter + @staticmethod def resource( func: Callable[..., Any] | None = None, @@ -1152,7 +1217,7 @@ class ClassSkill(Skill, ABC): resource_name = marker.get("name") or _make_method_name(attr_name) if resource_name in seen_names: raise ValueError( - f"Skill '{self.name}' already has a resource named '{resource_name}'. " + f"Skill '{self._frontmatter.name}' already has a resource named '{resource_name}'. " "Ensure each @ClassSkill.resource has a unique name." ) seen_names.add(resource_name) @@ -1212,7 +1277,7 @@ class ClassSkill(Skill, ABC): script_name = marker.get("name") or _make_method_name(attr_name) if script_name in seen_names: raise ValueError( - f"Skill '{self.name}' already has a script named '{script_name}'. " + f"Skill '{self._frontmatter.name}' already has a script named '{script_name}'. " "Ensure each @ClassSkill.script has a unique name." ) seen_names.add(script_name) @@ -1240,7 +1305,11 @@ class ClassSkill(Skill, ABC): return self._cached_content self._cached_content = _build_skill_content( - self.name, self.description, self.instructions, self.resources, self.scripts + self._frontmatter.name, + self._frontmatter.description, + self.instructions, + self.resources, + self.scripts, ) return self._cached_content @@ -1250,16 +1319,13 @@ class FileSkill(Skill): """A :class:`Skill` discovered from a filesystem directory backed by a SKILL.md file. Attributes: - name: Skill name (lowercase letters, numbers, hyphens only). - description: Human-readable description of the skill. path: Absolute path to the directory containing this skill. """ def __init__( self, *, - name: str, - description: str, + frontmatter: SkillFrontmatter, content: str, path: str, resources: Sequence[SkillResource] | None = None, @@ -1268,20 +1334,26 @@ class FileSkill(Skill): """Initialize a FileSkill. Args: - name: Skill name (lowercase letters, numbers, hyphens only). - description: Human-readable description of the skill (≤1024 chars). + frontmatter: Skill specification metadata parsed from the + SKILL.md file's YAML frontmatter (name, description, + and optional spec fields). content: The full raw SKILL.md file content including YAML frontmatter. path: Absolute path to the skill directory on disk. resources: Resources discovered for this skill. scripts: Scripts discovered for this skill. """ - super().__init__(name=name, description=description) + self._frontmatter = frontmatter self._content = content self.path = path self._resources: list[SkillResource] = list(resources) if resources is not None else [] self._scripts: list[SkillScript] = list(scripts) if scripts is not None else [] + @property + def frontmatter(self) -> SkillFrontmatter: + """The L1 discovery metadata for this skill.""" + return self._frontmatter + @property def content(self) -> str: """The skill content provided at construction time.""" @@ -1346,6 +1418,7 @@ SKILL_FILE_NAME: Final[str] = "SKILL.md" MAX_SEARCH_DEPTH: Final[int] = 2 MAX_NAME_LENGTH: Final[int] = 64 MAX_DESCRIPTION_LENGTH: Final[int] = 1024 +MAX_COMPATIBILITY_LENGTH: Final[int] = 500 DEFAULT_RESOURCE_EXTENSIONS: Final[tuple[str, ...]] = ( ".md", ".json", @@ -1366,10 +1439,24 @@ FRONTMATTER_RE = re.compile( re.MULTILINE | re.DOTALL, ) -# Matches YAML "key: value" lines. Group 1 = key, Group 2 = quoted value, -# Group 3 = unquoted value. +# Matches top-level YAML "key: value" lines (unindented). Group 1 = key, +# Group 2 = quoted value, Group 3 = unquoted value. Only matches keys at +# column 0 so that indented children (e.g. under "metadata:") are not +# mistakenly captured as top-level fields. YAML_KV_RE = re.compile( - r"^\s*(\w+)\s*:\s*(?:[\"'](.+?)[\"']|(.+?))\s*$", + r"^([\w-]+)\s*:\s*(?:[\"'](.+?)[\"']|(.+?))\s*$", + re.MULTILINE, +) + +# Matches a YAML "metadata:" block followed by indented key-value pairs. +YAML_METADATA_BLOCK_RE = re.compile( + r"^metadata\s*:\s*$\n((?:[ \t]+\S.*\n?)+)", + re.MULTILINE, +) + +# Matches indented "key: value" lines within a metadata block. +YAML_INDENTED_KV_RE = re.compile( + r"^\s+([\w-]+)\s*:\s*(?:[\"'](.+?)[\"']|(.+?))\s*$", re.MULTILINE, ) @@ -1377,6 +1464,7 @@ YAML_KV_RE = re.compile( # must not start or end with a hyphen, and must not contain consecutive hyphens. VALID_NAME_RE = re.compile(r"^[a-z0-9]([a-z0-9]*-[a-z0-9])*[a-z0-9]*$") + # Default system prompt template for advertising available skills to the model. # Use {skills} as the placeholder for the generated skills XML list. DEFAULT_SKILLS_INSTRUCTION_PROMPT = """\ @@ -1463,7 +1551,7 @@ class SkillsProvider(ContextProvider): FileSkillsSource("./skills", script_runner=my_runner), InMemorySkillsSource([my_code_skill]), ]), - predicate=lambda s: s.name != "internal", + predicate=lambda s: s.frontmatter.name != "internal", ) ) provider = SkillsProvider(source) @@ -1698,10 +1786,10 @@ class SkillsProvider(ContextProvider): lines: list[str] = [] # Sort by name for deterministic output - for skill in sorted(skills, key=lambda s: s.name): + for skill in sorted(skills, key=lambda s: s.frontmatter.name): lines.append(" ") - lines.append(f" {xml_escape(skill.name)}") - lines.append(f" {xml_escape(skill.description)}") + lines.append(f" {xml_escape(skill.frontmatter.name)}") + lines.append(f" {xml_escape(skill.frontmatter.description)}") lines.append(" ") return template.format( @@ -1920,7 +2008,7 @@ class SkillsProvider(ContextProvider): def _find_skill(skills: Sequence[Skill], name: str) -> Skill | None: """Find a skill by name (case-insensitive linear scan).""" name_lower = name.lower() - return next((s for s in skills if s.name.lower() == name_lower), None) + return next((s for s in skills if s.frontmatter.name.lower() == name_lower), None) def _load_skill(self, skills: Sequence[Skill], skill_name: str) -> str: """Return the full content for the named skill. @@ -2179,19 +2267,18 @@ class FileSkillsSource(SkillsSource): if parsed is None: continue - name, description, content = parsed + frontmatter, content = parsed - if name in skills: + if frontmatter.name in skills: logger.warning( "Duplicate skill name '%s': skill from '%s' skipped in favor of existing skill", - name, + frontmatter.name, skill_path, ) continue file_skill = FileSkill( - name=name, - description=description, + frontmatter=frontmatter, content=content, path=skill_path, ) @@ -2208,8 +2295,8 @@ class FileSkillsSource(SkillsSource): FileSkillScript(name=sn, full_path=script_full_path, runner=self._script_runner) ) - skills[file_skill.name] = file_skill - logger.info("Loaded skill: %s", file_skill.name) + skills[file_skill.frontmatter.name] = file_skill + logger.info("Loaded skill: %s", file_skill.frontmatter.name) logger.info("Successfully loaded %d skills", len(skills)) return list(skills.values()) @@ -2438,8 +2525,9 @@ class FileSkillsSource(SkillsSource): name: str | None, description: str | None, source: str, + compatibility: str | None = None, ) -> str | None: - """Validate a skill's name and description against naming rules. + """Validate a skill's name, description, and compatibility against naming rules. Enforces length limits, character-set restrictions, and non-emptiness for both file-based and code-defined skills. @@ -2449,6 +2537,7 @@ class FileSkillsSource(SkillsSource): description: Skill description to validate. source: Human-readable label for diagnostics (e.g. a file path or ``"code skill"``). + compatibility: Optional compatibility value to validate. Returns: A diagnostic error string if validation fails, or ``None`` if valid. @@ -2472,24 +2561,32 @@ class FileSkillsSource(SkillsSource): f"Must be {MAX_DESCRIPTION_LENGTH} characters or fewer." ) + if compatibility is not None and len(compatibility) > MAX_COMPATIBILITY_LENGTH: + return ( + f"Skill '{name}' from '{source}' has an invalid compatibility: " + f"Must be {MAX_COMPATIBILITY_LENGTH} characters or fewer." + ) + return None @staticmethod def _extract_frontmatter( content: str, skill_file_path: str, - ) -> tuple[str, str] | None: + ) -> SkillFrontmatter | None: """Extract and validate YAML frontmatter from a SKILL.md file. - Parses the ``---``-delimited frontmatter block for ``name`` and - ``description`` fields. + Parses the ``---``-delimited frontmatter block for all + `agentskills.io specification `_ + fields: ``name``, ``description``, ``license``, ``compatibility``, + ``allowed-tools``, and ``metadata``. Args: content: Raw text content of the SKILL.md file. skill_file_path: Path to the file (used in diagnostic messages only). Returns: - A ``(name, description)`` tuple on success, or ``None`` if the + A :class:`SkillFrontmatter` on success, or ``None`` if the frontmatter is missing, malformed, or fails validation. """ match = FRONTMATTER_RE.search(content) @@ -2500,35 +2597,63 @@ class FileSkillsSource(SkillsSource): yaml_content = match.group(1).strip() name: str | None = None description: str | None = None + license_value: str | None = None + compatibility: str | None = None + allowed_tools: str | None = None for kv_match in YAML_KV_RE.finditer(yaml_content): key = kv_match.group(1) value = kv_match.group(2) if kv_match.group(2) is not None else kv_match.group(3) - if key.lower() == "name": + key_lower = key.lower() + if key_lower == "name": name = value - elif key.lower() == "description": + elif key_lower == "description": description = value + elif key_lower == "license": + license_value = value + elif key_lower == "compatibility": + compatibility = value + elif key_lower == "allowed-tools": + allowed_tools = value - error = FileSkillsSource._validate_skill_metadata(name, description, skill_file_path) + # Parse metadata block (indented key-value pairs under "metadata:"). + metadata: dict[str, str] | None = None + metadata_match = YAML_METADATA_BLOCK_RE.search(yaml_content) + if metadata_match: + metadata = {} + for kv_match in YAML_INDENTED_KV_RE.finditer(metadata_match.group(1)): + mk = kv_match.group(1) + mv = kv_match.group(2) if kv_match.group(2) is not None else kv_match.group(3) + metadata[mk] = mv + + error = FileSkillsSource._validate_skill_metadata(name, description, skill_file_path, compatibility) if error: logger.error(error) return None - # name and description are guaranteed non-None after validation - return name, description # type: ignore[return-value] + # name and description are guaranteed non-None after validation; + # SkillFrontmatter re-validates as a defense-in-depth invariant. + return SkillFrontmatter( + name=cast(str, name), + description=cast(str, description), + license=license_value, + compatibility=compatibility, + allowed_tools=allowed_tools, + metadata=metadata, + ) @staticmethod def _read_and_parse_skill_file( skill_dir_path: str, - ) -> tuple[str, str, str] | None: + ) -> tuple[SkillFrontmatter, str] | None: """Read and parse the SKILL.md file in *skill_dir_path*. Args: skill_dir_path: Absolute path to the directory containing ``SKILL.md``. Returns: - A ``(name, description, content)`` tuple where *content* is the + A ``(frontmatter, content)`` tuple where *content* is the full raw file text, or ``None`` if the file cannot be read or its frontmatter is invalid. """ @@ -2540,23 +2665,21 @@ class FileSkillsSource(SkillsSource): logger.error("Failed to read SKILL.md at '%s'", skill_file) return None - result = FileSkillsSource._extract_frontmatter(content, str(skill_file)) - if result is None: + frontmatter = FileSkillsSource._extract_frontmatter(content, str(skill_file)) + if frontmatter is None: return None - name, description = result - dir_name = Path(skill_dir_path).name - if name != dir_name: + if frontmatter.name != dir_name: logger.error( "SKILL.md at '%s' has frontmatter name '%s' that does not match the directory name '%s'; skipping.", skill_file, - name, + frontmatter.name, dir_name, ) return None - return name, description, content + return frontmatter, content @staticmethod def _discover_skill_directories(skill_paths: Sequence[str]) -> list[str]: @@ -2704,12 +2827,12 @@ class DeduplicatingSkillsSource(DelegatingSkillsSource): result: list[Skill] = [] for skill in skills: - key = skill.name.lower() + key = skill.frontmatter.name.lower() if key in seen: logger.warning( "Duplicate skill name '%s': skill skipped in favor of existing skill '%s'", - skill.name, - seen[key].name, + skill.frontmatter.name, + seen[key].frontmatter.name, ) continue seen[key] = skill @@ -2730,7 +2853,7 @@ class FilteringSkillsSource(DelegatingSkillsSource): filtered = FilteringSkillsSource( inner_source=my_source, - predicate=lambda s: s.name != "internal", + predicate=lambda s: s.frontmatter.name != "internal", ) skills = await filtered.get_skills() """ diff --git a/python/packages/core/tests/core/test_skills.py b/python/packages/core/tests/core/test_skills.py index b268b31551..de39c58b2f 100644 --- a/python/packages/core/tests/core/test_skills.py +++ b/python/packages/core/tests/core/test_skills.py @@ -24,6 +24,7 @@ from agent_framework import ( InMemorySkillsSource, SessionContext, Skill, + SkillFrontmatter, SkillResource, SkillScript, SkillScriptRunner, @@ -69,7 +70,7 @@ def _ctx(provider: SkillsProvider) -> tuple[dict[str, Skill], str | None, list[A ctx = provider._cached_context # pyright: ignore[reportPrivateUsage] assert ctx is not None, "_init_provider() must be called before accessing context" skills, instructions, tools = ctx - return {s.name: s for s in skills}, instructions, tools + return {s.frontmatter.name: s for s in skills}, instructions, tools def _raw_skills(provider: SkillsProvider) -> Sequence[Skill]: @@ -129,10 +130,9 @@ def _read_and_parse_skill_file_for_test(skill_dir: Path) -> FileSkill: """Parse a SKILL.md file from the given directory, raising if invalid.""" result = FileSkillsSource._read_and_parse_skill_file(str(skill_dir)) assert result is not None, f"Failed to parse skill at {skill_dir}" - name, description, content = result + frontmatter, content = result return FileSkill( - name=name, - description=description, + frontmatter=frontmatter, content=content, path=str(skill_dir), ) @@ -163,7 +163,7 @@ async def _discover_file_skills_for_test( result: dict[str, FileSkill] = {} for s in skills: assert isinstance(s, FileSkill), f"Expected FileSkill, got {type(s).__name__}" - result[s.name] = s + result[s.frontmatter.name] = s return result @@ -268,22 +268,21 @@ class TestTryParseSkillDocument: content = "---\nname: test-skill\ndescription: A test skill.\n---\n# Body\nInstructions here." result = FileSkillsSource._extract_frontmatter(content, "test.md") assert result is not None - name, description = result - assert name == "test-skill" - assert description == "A test skill." + assert result.name == "test-skill" + assert result.description == "A test skill." def test_quoted_values(self) -> None: content = "---\nname: \"test-skill\"\ndescription: 'A test skill.'\n---\nBody." result = FileSkillsSource._extract_frontmatter(content, "test.md") assert result is not None - assert result[0] == "test-skill" - assert result[1] == "A test skill." + assert result.name == "test-skill" + assert result.description == "A test skill." def test_utf8_bom(self) -> None: content = "\ufeff---\nname: test-skill\ndescription: A test skill.\n---\nBody." result = FileSkillsSource._extract_frontmatter(content, "test.md") assert result is not None - assert result[0] == "test-skill" + assert result.name == "test-skill" def test_missing_frontmatter(self) -> None: content = "# Just a markdown file\nNo frontmatter here." @@ -327,11 +326,11 @@ class TestTryParseSkillDocument: result = FileSkillsSource._extract_frontmatter(content, "test.md") assert result is None - def test_extra_metadata_ignored(self) -> None: + def test_extra_fields_parsed(self) -> None: content = "---\nname: test-skill\ndescription: A test skill.\nauthor: someone\nversion: 1.0\n---\nBody." result = FileSkillsSource._extract_frontmatter(content, "test.md") assert result is not None - assert result[0] == "test-skill" + assert result.name == "test-skill" # --------------------------------------------------------------------------- @@ -346,7 +345,7 @@ class TestDiscoverAndLoadSkills: _write_skill(tmp_path, "my-skill") skills = await _discover_file_skills_for_test([str(tmp_path)]) assert "my-skill" in skills - assert skills["my-skill"].name == "my-skill" + assert skills["my-skill"].frontmatter.name == "my-skill" async def test_discovers_nested_skills(self, tmp_path: Path) -> None: skills_dir = tmp_path / "skills" @@ -504,7 +503,7 @@ class TestBuildSkillsInstructionPrompt: def test_default_prompt_contains_skills(self) -> None: skills = [ - InlineSkill(name="my-skill", description="Does stuff.", instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="Does stuff."), instructions="Body"), ] prompt = SkillsProvider._create_instructions(None, skills) assert prompt is not None @@ -514,8 +513,8 @@ class TestBuildSkillsInstructionPrompt: def test_skills_sorted_alphabetically(self) -> None: skills = [ - InlineSkill(name="zebra", description="Z skill.", instructions="Body"), - InlineSkill(name="alpha", description="A skill.", instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="zebra", description="Z skill."), instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="alpha", description="A skill."), instructions="Body"), ] prompt = SkillsProvider._create_instructions(None, skills) assert prompt is not None @@ -525,7 +524,9 @@ class TestBuildSkillsInstructionPrompt: def test_xml_escapes_metadata(self) -> None: skills = [ - InlineSkill(name="my-skill", description='Uses & "quotes"', instructions="Body"), + InlineSkill( + frontmatter=SkillFrontmatter(name="my-skill", description='Uses & "quotes"'), instructions="Body" + ), ] prompt = SkillsProvider._create_instructions(None, skills) assert prompt is not None @@ -534,7 +535,7 @@ class TestBuildSkillsInstructionPrompt: def test_custom_prompt_template(self) -> None: skills = [ - InlineSkill(name="my-skill", description="Does stuff.", instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="Does stuff."), instructions="Body"), ] custom = "Custom header:\n{skills}\nCustom footer." prompt = SkillsProvider._create_instructions(custom, skills) @@ -544,14 +545,14 @@ class TestBuildSkillsInstructionPrompt: def test_invalid_prompt_template_raises(self) -> None: skills = [ - InlineSkill(name="my-skill", description="Does stuff.", instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="Does stuff."), instructions="Body"), ] with pytest.raises(ValueError, match="valid format string"): SkillsProvider._create_instructions("{invalid}", skills) def test_positional_placeholder_raises(self) -> None: skills = [ - InlineSkill(name="my-skill", description="Does stuff.", instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="Does stuff."), instructions="Body"), ] with pytest.raises(ValueError, match="valid format string"): SkillsProvider._create_instructions("Header {0} footer", skills) @@ -942,25 +943,28 @@ class TestInlineSkill: def test_inline_skill_is_skill(self) -> None: """InlineSkill is a subclass of Skill.""" - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") assert isinstance(skill, Skill) def test_file_skill_is_skill(self) -> None: """FileSkill is a subclass of Skill.""" - skill = FileSkill(name="my-skill", description="A skill.", content="Body", path="/tmp/skill") + skill = FileSkill( + frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), content="Body", path="/tmp/skill" + ) assert isinstance(skill, Skill) def test_basic_construction(self) -> None: - skill = InlineSkill(name="my-skill", description="A test skill.", instructions="Instructions.") - assert skill.name == "my-skill" - assert skill.description == "A test skill." + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="my-skill", description="A test skill."), instructions="Instructions." + ) + assert skill.frontmatter.name == "my-skill" + assert skill.frontmatter.description == "A test skill." assert skill.instructions == "Instructions." assert skill.resources == [] def test_construction_with_static_resources(self) -> None: skill = InlineSkill( - name="my-skill", - description="A test skill.", + frontmatter=SkillFrontmatter(name="my-skill", description="A test skill."), instructions="Instructions.", resources=[ InlineSkillResource(name="ref", content="Reference content"), @@ -971,34 +975,36 @@ class TestInlineSkill: def test_empty_name_raises(self) -> None: with pytest.raises(ValueError, match="cannot be empty"): - InlineSkill(name="", description="A skill.", instructions="Body") + InlineSkill(frontmatter=SkillFrontmatter(name="", description="A skill."), instructions="Body") def test_invalid_name_raises(self) -> None: with pytest.raises(ValueError, match="Invalid skill name"): - InlineSkill(name="Invalid-Name", description="A skill.", instructions="Body") + InlineSkill(frontmatter=SkillFrontmatter(name="Invalid-Name", description="A skill."), instructions="Body") def test_name_starts_with_hyphen_raises(self) -> None: with pytest.raises(ValueError, match="Invalid skill name"): - InlineSkill(name="-bad-name", description="A skill.", instructions="Body") + InlineSkill(frontmatter=SkillFrontmatter(name="-bad-name", description="A skill."), instructions="Body") def test_name_with_consecutive_hyphens_raises(self) -> None: with pytest.raises(ValueError, match="Invalid skill name"): - InlineSkill(name="consecutive--hyphens", description="A skill.", instructions="Body") + InlineSkill( + frontmatter=SkillFrontmatter(name="consecutive--hyphens", description="A skill."), instructions="Body" + ) def test_name_too_long_raises(self) -> None: with pytest.raises(ValueError, match="Invalid skill name"): - InlineSkill(name="a" * 65, description="A skill.", instructions="Body") + InlineSkill(frontmatter=SkillFrontmatter(name="a" * 65, description="A skill."), instructions="Body") def test_empty_description_raises(self) -> None: with pytest.raises(ValueError, match="cannot be empty"): - InlineSkill(name="my-skill", description="", instructions="Body") + InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description=""), instructions="Body") def test_description_too_long_raises(self) -> None: with pytest.raises(ValueError, match="invalid description"): - InlineSkill(name="my-skill", description="a" * 1025, instructions="Body") + InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="a" * 1025), instructions="Body") def test_resource_decorator_bare(self) -> None: - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") @skill.resource def get_schema() -> Any: @@ -1012,7 +1018,7 @@ class TestInlineSkill: assert skill.resources[0].function is get_schema def test_resource_decorator_with_args(self) -> None: - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") @skill.resource(name="custom-name", description="Custom description") def my_resource() -> Any: @@ -1024,7 +1030,7 @@ class TestInlineSkill: def test_resource_decorator_returns_function(self) -> None: """Decorator should return the original function unchanged.""" - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") @skill.resource def get_data() -> Any: @@ -1034,7 +1040,7 @@ class TestInlineSkill: assert get_data() == "data" def test_multiple_resources(self) -> None: - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") @skill.resource def resource_a() -> Any: @@ -1050,7 +1056,7 @@ class TestInlineSkill: assert "resource_b" in names def test_resource_decorator_async(self) -> None: - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") @skill.resource async def get_async_data() -> Any: @@ -1070,13 +1076,19 @@ class TestSkillsProviderCodeSkill: """Tests for SkillsProvider with code-defined skills.""" async def test_code_skill_only(self) -> None: - skill = InlineSkill(name="prog-skill", description="A code-defined skill.", instructions="Do the thing.") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A code-defined skill."), + instructions="Do the thing.", + ) provider = SkillsProvider([skill]) await _init_provider(provider) assert "prog-skill" in _ctx(provider)[0] async def test_load_skill_returns_content(self) -> None: - skill = InlineSkill(name="prog-skill", description="A skill.", instructions="Code-defined instructions.") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), + instructions="Code-defined instructions.", + ) provider = SkillsProvider([skill]) await _init_provider(provider) result = provider._load_skill(_raw_skills(provider), "prog-skill") @@ -1087,8 +1099,7 @@ class TestSkillsProviderCodeSkill: async def test_load_skill_appends_resource_listing(self) -> None: skill = InlineSkill( - name="prog-skill", - description="A skill.", + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Do things.", resources=[ InlineSkillResource(name="ref-a", content="a", description="First resource"), @@ -1106,7 +1117,9 @@ class TestSkillsProviderCodeSkill: assert '' in result async def test_load_skill_no_resources_no_listing(self) -> None: - skill = InlineSkill(name="prog-skill", description="A skill.", instructions="Body only.") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body only." + ) provider = SkillsProvider([skill]) await _init_provider(provider) result = provider._load_skill(_raw_skills(provider), "prog-skill") @@ -1115,8 +1128,7 @@ class TestSkillsProviderCodeSkill: async def test_read_static_resource(self) -> None: skill = InlineSkill( - name="prog-skill", - description="A skill.", + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body", resources=[InlineSkillResource(name="ref", content="static content")], ) @@ -1126,7 +1138,9 @@ class TestSkillsProviderCodeSkill: assert result == "static content" async def test_read_callable_resource_sync(self) -> None: - skill = InlineSkill(name="prog-skill", description="A skill.", instructions="Body") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body" + ) @skill.resource def get_schema() -> Any: @@ -1138,7 +1152,9 @@ class TestSkillsProviderCodeSkill: assert result == "CREATE TABLE users" async def test_read_callable_resource_async(self) -> None: - skill = InlineSkill(name="prog-skill", description="A skill.", instructions="Body") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body" + ) @skill.resource async def get_data() -> Any: @@ -1151,8 +1167,7 @@ class TestSkillsProviderCodeSkill: async def test_read_resource_case_insensitive(self) -> None: skill = InlineSkill( - name="prog-skill", - description="A skill.", + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body", resources=[InlineSkillResource(name="MyRef", content="content")], ) @@ -1162,14 +1177,18 @@ class TestSkillsProviderCodeSkill: assert result == "content" async def test_read_unknown_resource_returns_error(self) -> None: - skill = InlineSkill(name="prog-skill", description="A skill.", instructions="Body") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body" + ) provider = SkillsProvider([skill]) await _init_provider(provider) result = await provider._read_skill_resource(_raw_skills(provider), "prog-skill", "nonexistent") assert result.startswith("Error:") async def test_read_callable_resource_sync_with_kwargs(self) -> None: - skill = InlineSkill(name="prog-skill", description="A skill.", instructions="Body") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body" + ) @skill.resource def get_user_config(**kwargs: Any) -> Any: @@ -1184,7 +1203,9 @@ class TestSkillsProviderCodeSkill: assert result == "config for user_123" async def test_read_callable_resource_async_with_kwargs(self) -> None: - skill = InlineSkill(name="prog-skill", description="A skill.", instructions="Body") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body" + ) @skill.resource async def get_user_data(**kwargs: Any) -> Any: @@ -1200,7 +1221,9 @@ class TestSkillsProviderCodeSkill: async def test_read_callable_resource_without_kwargs_ignores_extra_args(self) -> None: """Resource functions without **kwargs should still work when kwargs are passed.""" - skill = InlineSkill(name="prog-skill", description="A skill.", instructions="Body") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body" + ) @skill.resource def static_resource() -> Any: @@ -1215,7 +1238,9 @@ class TestSkillsProviderCodeSkill: async def test_read_callable_resource_returns_dict(self) -> None: """Resource functions may return non-string types, passed through as-is.""" - skill = InlineSkill(name="prog-skill", description="A skill.", instructions="Body") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body" + ) @skill.resource def get_config() -> Any: @@ -1228,7 +1253,9 @@ class TestSkillsProviderCodeSkill: async def test_read_callable_resource_returns_list(self) -> None: """Resource functions may return lists, passed through as-is.""" - skill = InlineSkill(name="prog-skill", description="A skill.", instructions="Body") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body" + ) @skill.resource def get_items() -> Any: @@ -1241,7 +1268,9 @@ class TestSkillsProviderCodeSkill: async def test_read_callable_resource_returns_none(self) -> None: """Resource functions may return None.""" - skill = InlineSkill(name="prog-skill", description="A skill.", instructions="Body") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body" + ) @skill.resource def get_nothing() -> Any: @@ -1253,7 +1282,9 @@ class TestSkillsProviderCodeSkill: assert result is None async def test_before_run_injects_code_skills(self) -> None: - skill = InlineSkill(name="prog-skill", description="A code-defined skill.", instructions="Body") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A code-defined skill."), instructions="Body" + ) provider = SkillsProvider([skill]) context = SessionContext(input_messages=[]) @@ -1274,7 +1305,9 @@ class TestSkillsProviderCodeSkill: async def test_combined_file_and_code_skill(self, tmp_path: Path) -> None: _write_skill(tmp_path, "file-skill") - prog_skill = InlineSkill(name="prog-skill", description="Code-defined.", instructions="Body") + prog_skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="Code-defined."), instructions="Body" + ) provider = SkillsProvider( DeduplicatingSkillsSource( AggregatingSkillsSource([ @@ -1289,7 +1322,9 @@ class TestSkillsProviderCodeSkill: async def test_duplicate_name_file_wins(self, tmp_path: Path) -> None: _write_skill(tmp_path, "my-skill", body="File version") - prog_skill = InlineSkill(name="my-skill", description="Code-defined.", instructions="Prog version") + prog_skill = InlineSkill( + frontmatter=SkillFrontmatter(name="my-skill", description="Code-defined."), instructions="Prog version" + ) provider = SkillsProvider( DeduplicatingSkillsSource( AggregatingSkillsSource([ @@ -1304,7 +1339,9 @@ class TestSkillsProviderCodeSkill: async def test_combined_prompt_includes_both(self, tmp_path: Path) -> None: _write_skill(tmp_path, "file-skill") - prog_skill = InlineSkill(name="prog-skill", description="A code-defined skill.", instructions="Body") + prog_skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A code-defined skill."), instructions="Body" + ) provider = SkillsProvider( DeduplicatingSkillsSource( AggregatingSkillsSource([ @@ -1361,8 +1398,8 @@ class TestFileBasedSkillParsing: def test_name_and_description_from_frontmatter(self, tmp_path: Path) -> None: _write_skill(tmp_path, "my-skill", description="Skill desc.") skill = _read_and_parse_skill_file_for_test(tmp_path / "my-skill") - assert skill.name == "my-skill" - assert skill.description == "Skill desc." + assert skill.frontmatter.name == "my-skill" + assert skill.frontmatter.description == "Skill desc." def test_path_set(self, tmp_path: Path) -> None: _write_skill(tmp_path, "my-skill") @@ -1397,7 +1434,9 @@ class TestLoadSkillFormatting: async def test_code_skill_wraps_in_xml(self) -> None: """Code-defined skills are wrapped with name, description, and instructions tags.""" - skill = InlineSkill(name="prog-skill", description="A skill.", instructions="Do stuff.") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Do stuff." + ) provider = SkillsProvider([skill]) await _init_provider(provider) result = provider._load_skill(_raw_skills(provider), "prog-skill") @@ -1408,8 +1447,7 @@ class TestLoadSkillFormatting: async def test_code_skill_single_resource_no_description(self) -> None: """Resource without description omits the description attribute.""" skill = InlineSkill( - name="prog-skill", - description="A skill.", + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body.", resources=[InlineSkillResource(name="data", content="val")], ) @@ -1642,9 +1680,9 @@ class TestReadAndParseSkillFile: (skill_dir / "SKILL.md").write_text("---\nname: my-skill\ndescription: A skill.\n---\nBody.", encoding="utf-8") result = FileSkillsSource._read_and_parse_skill_file(str(skill_dir)) assert result is not None - name, desc, content = result - assert name == "my-skill" - assert desc == "A skill." + frontmatter, content = result + assert frontmatter.name == "my-skill" + assert frontmatter.description == "A skill." assert "Body." in content def test_missing_skill_md_returns_none(self, tmp_path: Path) -> None: @@ -1838,14 +1876,282 @@ class TestExtractFrontmatterEdgeCases: content = f"---\nname: {name}\ndescription: A skill.\n---\nBody." result = FileSkillsSource._extract_frontmatter(content, "test.md") assert result is not None - assert result[0] == name + assert result.name == name def test_description_exactly_max_length(self) -> None: desc = "a" * 1024 content = f"---\nname: test-skill\ndescription: {desc}\n---\nBody." result = FileSkillsSource._extract_frontmatter(content, "test.md") assert result is not None - assert result[1] == desc + assert result.description == desc + + +# --------------------------------------------------------------------------- +# Tests: Skill spec fields (via SkillFrontmatter) +# --------------------------------------------------------------------------- + + +class TestSkillSpecFields: + """Tests for agentskills.io spec fields on SkillFrontmatter exposed via Skill.frontmatter.""" + + def test_basic_construction_defaults(self) -> None: + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="my-skill", description="A description."), instructions="Do it." + ) + assert skill.frontmatter.name == "my-skill" + assert skill.frontmatter.description == "A description." + assert skill.frontmatter.license is None + assert skill.frontmatter.compatibility is None + assert skill.frontmatter.allowed_tools is None + assert skill.frontmatter.metadata is None + + def test_all_fields_on_inline_skill(self) -> None: + skill = InlineSkill( + frontmatter=SkillFrontmatter( + name="my-skill", + description="A description.", + license="MIT", + compatibility="Works with GPT-4", + allowed_tools="tool1 tool2", + metadata={"author": "test", "version": "1.0"}, + ), + instructions="Do it.", + ) + assert skill.frontmatter.license == "MIT" + assert skill.frontmatter.compatibility == "Works with GPT-4" + assert skill.frontmatter.allowed_tools == "tool1 tool2" + assert skill.frontmatter.metadata == {"author": "test", "version": "1.0"} + + def test_compatibility_too_long_raises(self) -> None: + with pytest.raises(ValueError): + InlineSkill( + frontmatter=SkillFrontmatter(name="my-skill", description="A description.", compatibility="a" * 501), + instructions="Do it.", + ) + + def test_compatibility_exactly_max_length(self) -> None: + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="my-skill", description="A description.", compatibility="a" * 500), + instructions="Do it.", + ) + assert skill.frontmatter.compatibility == "a" * 500 + + def test_file_skill_spec_fields(self) -> None: + skill = FileSkill( + frontmatter=SkillFrontmatter( + name="my-skill", + description="Test.", + license="MIT", + compatibility="compat info", + allowed_tools="tool1", + metadata={"key": "val"}, + ), + content="---\nname: my-skill\n---", + path="/skills/my-skill", + ) + assert skill.frontmatter.license == "MIT" + assert skill.frontmatter.compatibility == "compat info" + assert skill.frontmatter.allowed_tools == "tool1" + assert skill.frontmatter.metadata == {"key": "val"} + + +# --------------------------------------------------------------------------- +# Tests: SkillFrontmatter class and two-form constructors +# --------------------------------------------------------------------------- + + +class TestSkillFrontmatter: + """Tests for the :class:`SkillFrontmatter` class.""" + + def test_basic_construction(self) -> None: + fm = SkillFrontmatter(name="my-skill", description="A test skill.") + assert fm.name == "my-skill" + assert fm.description == "A test skill." + assert fm.license is None + assert fm.compatibility is None + assert fm.allowed_tools is None + assert fm.metadata is None + + def test_all_fields(self) -> None: + fm = SkillFrontmatter( + name="my-skill", + description="Desc.", + license="MIT", + compatibility="GPT-4", + allowed_tools="tool1", + metadata={"key": "val"}, + ) + assert fm.license == "MIT" + assert fm.compatibility == "GPT-4" + assert fm.allowed_tools == "tool1" + assert fm.metadata == {"key": "val"} + + def test_invalid_name_raises(self) -> None: + with pytest.raises(ValueError): + SkillFrontmatter(name="Bad Name!", description="Desc.") + + def test_invalid_description_raises(self) -> None: + with pytest.raises(ValueError): + SkillFrontmatter(name="my-skill", description="") + + def test_invalid_compatibility_raises(self) -> None: + with pytest.raises(ValueError): + SkillFrontmatter(name="my-skill", description="Desc.", compatibility="a" * 501) + + def test_compatibility_can_be_reassigned(self) -> None: + fm = SkillFrontmatter(name="my-skill", description="Desc.") + fm.compatibility = "a" * 500 + assert fm.compatibility == "a" * 500 + # Plain attribute: post-construction assignment is not re-validated. + fm.compatibility = "a" * 501 + assert fm.compatibility == "a" * 501 + + def test_metadata_is_shallow_copied(self) -> None: + original = {"key": "val"} + fm = SkillFrontmatter(name="my-skill", description="Desc.", metadata=original) + original["key"] = "mutated" + assert fm.metadata == {"key": "val"} + + def test_name_is_mutable(self) -> None: + fm = SkillFrontmatter(name="my-skill", description="Desc.") + fm.name = "other-skill" + assert fm.name == "other-skill" + + def test_description_is_mutable(self) -> None: + fm = SkillFrontmatter(name="my-skill", description="Desc.") + fm.description = "Other description." + assert fm.description == "Other description." + + +class TestExtractFrontmatterSpecFields: + """Tests for _extract_frontmatter parsing all agentskills.io spec fields.""" + + def test_license_parsed(self) -> None: + content = "---\nname: test-skill\ndescription: A skill.\nlicense: MIT\n---\nBody." + result = FileSkillsSource._extract_frontmatter(content, "test.md") + assert result is not None + assert result.license == "MIT" + + def test_compatibility_parsed(self) -> None: + content = "---\nname: test-skill\ndescription: A skill.\ncompatibility: Works with GPT-4\n---\nBody." + result = FileSkillsSource._extract_frontmatter(content, "test.md") + assert result is not None + assert result.compatibility == "Works with GPT-4" + + def test_compatibility_too_long_returns_none(self) -> None: + long_compat = "a" * 501 + content = f"---\nname: test-skill\ndescription: A skill.\ncompatibility: {long_compat}\n---\nBody." + result = FileSkillsSource._extract_frontmatter(content, "test.md") + assert result is None + + def test_allowed_tools_parsed(self) -> None: + content = "---\nname: test-skill\ndescription: A skill.\nallowed-tools: tool1 tool2 tool3\n---\nBody." + result = FileSkillsSource._extract_frontmatter(content, "test.md") + assert result is not None + assert result.allowed_tools == "tool1 tool2 tool3" + + def test_metadata_block_parsed(self) -> None: + content = ( + "---\nname: test-skill\ndescription: A skill.\nmetadata:\n author: someone\n version: 1.0\n---\nBody." + ) + result = FileSkillsSource._extract_frontmatter(content, "test.md") + assert result is not None + assert result.metadata is not None + assert result.metadata["author"] == "someone" + assert result.metadata["version"] == "1.0" + + def test_metadata_with_quoted_values(self) -> None: + content = ( + "---\nname: test-skill\ndescription: A skill.\nmetadata:\n" + " author: 'John Doe'\n org: \"Contoso\"\n---\nBody." + ) + result = FileSkillsSource._extract_frontmatter(content, "test.md") + assert result is not None + assert result.metadata is not None + assert result.metadata["author"] == "John Doe" + assert result.metadata["org"] == "Contoso" + + def test_no_metadata_block(self) -> None: + content = "---\nname: test-skill\ndescription: A skill.\n---\nBody." + result = FileSkillsSource._extract_frontmatter(content, "test.md") + assert result is not None + assert result.metadata is None + + def test_all_spec_fields(self) -> None: + content = ( + "---\n" + "name: test-skill\n" + "description: A comprehensive skill.\n" + "license: Apache-2.0\n" + "compatibility: Works with GPT-4 and Claude\n" + "allowed-tools: read-file write-file\n" + "metadata:\n" + " author: test-author\n" + " version: 2.0\n" + "---\n" + "Body content." + ) + result = FileSkillsSource._extract_frontmatter(content, "test.md") + assert result is not None + assert result.name == "test-skill" + assert result.description == "A comprehensive skill." + assert result.license == "Apache-2.0" + assert result.compatibility == "Works with GPT-4 and Claude" + assert result.allowed_tools == "read-file write-file" + assert result.metadata == {"author": "test-author", "version": "2.0"} + + async def test_file_skill_fields_populated_from_discovery(self, tmp_path: Path) -> None: + """End-to-end: spec fields are populated on FileSkill via discovery.""" + skill_dir = tmp_path / "test-skill" + skill_dir.mkdir() + skill_md = skill_dir / "SKILL.md" + skill_md.write_text( + "---\n" + "name: test-skill\n" + "description: A test skill.\n" + "license: MIT\n" + "compatibility: GPT-4\n" + "allowed-tools: tool1\n" + "metadata:\n" + " key: value\n" + "---\n" + "Instructions.", + encoding="utf-8", + ) + source = FileSkillsSource(str(tmp_path)) + skills = await source.get_skills() + assert len(skills) == 1 + skill = skills[0] + assert isinstance(skill, FileSkill) + assert skill.frontmatter.license == "MIT" + assert skill.frontmatter.compatibility == "GPT-4" + assert skill.frontmatter.allowed_tools == "tool1" + assert skill.frontmatter.metadata == {"key": "value"} + + def test_metadata_children_do_not_override_top_level_fields(self) -> None: + """Indented keys inside a metadata: block must not overwrite top-level fields.""" + content = ( + "---\n" + "name: test-skill\n" + "description: The real description.\n" + "license: MIT\n" + "metadata:\n" + " description: should not override\n" + " license: should not override\n" + " name: should not override\n" + "---\n" + "Body." + ) + result = FileSkillsSource._extract_frontmatter(content, "test.md") + assert result is not None + assert result.name == "test-skill" + assert result.description == "The real description." + assert result.license == "MIT" + assert result.metadata == { + "description": "should not override", + "license": "should not override", + "name": "should not override", + } # --------------------------------------------------------------------------- @@ -1862,7 +2168,7 @@ class TestCreateInstructionsEdgeCases: def test_custom_template_with_literal_braces(self) -> None: skills = [ - InlineSkill(name="my-skill", description="Skill.", instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="Skill."), instructions="Body"), ] template = "Header {{literal}} {skills} footer." result = SkillsProvider._create_instructions(template, skills) @@ -1872,9 +2178,9 @@ class TestCreateInstructionsEdgeCases: def test_multiple_skills_generates_sorted_xml(self) -> None: skills = [ - InlineSkill(name="charlie", description="C.", instructions="Body"), - InlineSkill(name="alpha", description="A.", instructions="Body"), - InlineSkill(name="bravo", description="B.", instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="charlie", description="C."), instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="alpha", description="A."), instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="bravo", description="B."), instructions="Body"), ] result = SkillsProvider._create_instructions(None, skills) assert result is not None @@ -1886,7 +2192,7 @@ class TestCreateInstructionsEdgeCases: def test_custom_template_missing_runner_instructions_raises(self) -> None: """Custom template without {runner_instructions} raises when scripts are enabled.""" skills = [ - InlineSkill(name="my-skill", description="Skill.", instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="Skill."), instructions="Body"), ] template = "Skills: {skills}" with pytest.raises(ValueError, match="runner_instructions"): @@ -1895,7 +2201,7 @@ class TestCreateInstructionsEdgeCases: def test_custom_template_missing_resource_instructions_raises(self) -> None: """Custom template without {resource_instructions} raises when resources exist.""" skills = [ - InlineSkill(name="my-skill", description="Skill.", instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="Skill."), instructions="Body"), ] template = "Skills: {skills}" with pytest.raises(ValueError, match="resource_instructions"): @@ -1904,7 +2210,7 @@ class TestCreateInstructionsEdgeCases: def test_include_resource_instructions_true_adds_resource_text(self) -> None: """When include_resource_instructions is True, resource instructions appear in the prompt.""" skills = [ - InlineSkill(name="my-skill", description="Skill.", instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="Skill."), instructions="Body"), ] result = SkillsProvider._create_instructions(None, skills, include_resource_instructions=True) assert result is not None @@ -1913,7 +2219,7 @@ class TestCreateInstructionsEdgeCases: def test_include_resource_instructions_false_omits_resource_text(self) -> None: """When include_resource_instructions is False, resource instructions do not appear.""" skills = [ - InlineSkill(name="my-skill", description="Skill.", instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="Skill."), instructions="Body"), ] result = SkillsProvider._create_instructions(None, skills, include_resource_instructions=False) assert result is not None @@ -1922,7 +2228,7 @@ class TestCreateInstructionsEdgeCases: def test_custom_template_with_unknown_placeholder_raises(self) -> None: """Template with an unknown placeholder raises ValueError.""" skills = [ - InlineSkill(name="my-skill", description="Skill.", instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="Skill."), instructions="Body"), ] template = "Skills: {skills} {unknown_key}" with pytest.raises(ValueError, match="valid format string"): @@ -1952,7 +2258,7 @@ class TestSkillsProviderEdgeCases: assert "empty" in result async def test_read_skill_resource_whitespace_skill_name_returns_error(self) -> None: - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") provider = SkillsProvider([skill]) await _init_provider(provider) result = await provider._read_skill_resource(_raw_skills(provider), " ", "ref") @@ -1960,7 +2266,7 @@ class TestSkillsProviderEdgeCases: assert "empty" in result async def test_read_skill_resource_whitespace_resource_name_returns_error(self) -> None: - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") provider = SkillsProvider([skill]) await _init_provider(provider) result = await provider._read_skill_resource(_raw_skills(provider), "my-skill", " ") @@ -1968,7 +2274,7 @@ class TestSkillsProviderEdgeCases: assert "empty" in result async def test_read_callable_resource_exception_returns_error(self) -> None: - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") @skill.resource def exploding_resource() -> Any: @@ -1981,7 +2287,7 @@ class TestSkillsProviderEdgeCases: assert "Failed to read resource" in result async def test_read_async_callable_resource_exception_returns_error(self) -> None: - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") @skill.resource async def async_exploding() -> Any: @@ -1993,7 +2299,9 @@ class TestSkillsProviderEdgeCases: assert result.startswith("Error:") async def test_load_code_skill_xml_escapes_metadata(self) -> None: - skill = InlineSkill(name="my-skill", description='Uses & "quotes"', instructions="Body") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="my-skill", description='Uses & "quotes"'), instructions="Body" + ) provider = SkillsProvider([skill]) await _init_provider(provider) result = provider._load_skill(_raw_skills(provider), "my-skill") @@ -2001,16 +2309,18 @@ class TestSkillsProviderEdgeCases: assert "&" in result async def test_code_skill_deduplication(self) -> None: - skill1 = InlineSkill(name="my-skill", description="First.", instructions="Body 1") - skill2 = InlineSkill(name="my-skill", description="Second.", instructions="Body 2") + skill1 = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="First."), instructions="Body 1") + skill2 = InlineSkill( + frontmatter=SkillFrontmatter(name="my-skill", description="Second."), instructions="Body 2" + ) provider = SkillsProvider([skill1, skill2]) await _init_provider(provider) assert len(_ctx(provider)[0]) == 1 - assert "First." in _ctx(provider)[0]["my-skill"].description + assert "First." in _ctx(provider)[0]["my-skill"].frontmatter.description async def test_before_run_extends_tools_even_without_instructions(self) -> None: """If instructions are somehow None but skills exist, tools should still be added.""" - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") provider = SkillsProvider([skill]) context = SessionContext(input_messages=[]) @@ -2122,7 +2432,7 @@ class TestSkillResourceDecoratorEdgeCases: """Additional edge-case tests for the @skill.resource decorator.""" def test_decorator_no_docstring_description_is_none(self) -> None: - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") @skill.resource def no_docs() -> Any: @@ -2131,7 +2441,7 @@ class TestSkillResourceDecoratorEdgeCases: assert skill.resources[0].description is None def test_decorator_with_name_only(self) -> None: - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") @skill.resource(name="custom-name") def get_data() -> Any: @@ -2143,7 +2453,7 @@ class TestSkillResourceDecoratorEdgeCases: assert skill.resources[0].description is None def test_decorator_with_description_only(self) -> None: - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") @skill.resource(description="Custom desc") def get_data() -> Any: @@ -2153,7 +2463,7 @@ class TestSkillResourceDecoratorEdgeCases: assert skill.resources[0].description == "Custom desc" def test_decorator_preserves_original_function_identity(self) -> None: - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") @skill.resource def original() -> Any: @@ -2230,7 +2540,7 @@ class TestSkillScriptRun: return f"hello {name}" script = InlineSkillScript(name="greet", function=greet) - skill = InlineSkill(name="s", description="d", instructions="c") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="s", description="d"), instructions="c") result = await script.run(skill, args={"name": "Alice"}) assert result == "hello Alice" @@ -2239,7 +2549,7 @@ class TestSkillScriptRun: return f"async {name}" script = InlineSkillScript(name="greet", function=greet) - skill = InlineSkill(name="s", description="d", instructions="c") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="s", description="d"), instructions="c") result = await script.run(skill, args={"name": "Bob"}) assert result == "async Bob" @@ -2248,13 +2558,13 @@ class TestSkillScriptRun: return {"x": x, **kwargs} script = InlineSkillScript(name="f", function=func) - skill = InlineSkill(name="s", description="d", instructions="c") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="s", description="d"), instructions="c") result = await script.run(skill, args={"x": 1}, extra="val") assert result == {"x": 1, "extra": "val"} async def test_run_code_defined_no_args(self) -> None: script = InlineSkillScript(name="f", function=lambda: 42) - skill = InlineSkill(name="s", description="d", instructions="c") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="s", description="d"), instructions="c") result = await script.run(skill) assert result == 42 @@ -2262,13 +2572,15 @@ class TestSkillScriptRun: captured: dict[str, Any] = {} def runner(skill: Skill, script: SkillScript, args: dict[str, Any] | None = None) -> str: - captured["skill"] = skill.name + captured["skill"] = skill.frontmatter.name captured["script"] = script.name captured["args"] = args return "runner_result" script = FileSkillScript(name="run.py", full_path=f"{_ABS}/test/run.py", runner=runner) - skill = FileSkill(name="my-skill", description="d", content="c", path=f"{_ABS}/test") + skill = FileSkill( + frontmatter=SkillFrontmatter(name="my-skill", description="d"), content="c", path=f"{_ABS}/test" + ) result = await script.run(skill, args={"key": "val"}) assert result == "runner_result" assert captured["skill"] == "my-skill" @@ -2280,19 +2592,19 @@ class TestSkillScriptRun: return "async_runner" script = FileSkillScript(name="run.py", full_path=f"{_ABS}/test/run.py", runner=runner) - skill = FileSkill(name="s", description="d", content="c", path=f"{_ABS}/test") + skill = FileSkill(frontmatter=SkillFrontmatter(name="s", description="d"), content="c", path=f"{_ABS}/test") result = await script.run(skill, args=None) assert result == "async_runner" async def test_run_file_based_without_runner_raises(self) -> None: script = FileSkillScript(name="run.py", full_path=f"{_ABS}/test/run.py") - skill = FileSkill(name="s", description="d", content="c", path=f"{_ABS}/test") + skill = FileSkill(frontmatter=SkillFrontmatter(name="s", description="d"), content="c", path=f"{_ABS}/test") with pytest.raises(ValueError, match="requires a runner"): await script.run(skill) async def test_run_file_based_with_non_file_skill_raises_type_error(self) -> None: script = FileSkillScript(name="run.py", full_path=f"{_ABS}/test/run.py", runner=_noop_script_runner) - skill = InlineSkill(name="s", description="d", instructions="c") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="s", description="d"), instructions="c") with pytest.raises(TypeError, match="requires a FileSkill"): await script.run(skill) @@ -2314,7 +2626,7 @@ class TestSkillScriptDecorator: """Tests for the @skill.script decorator.""" def test_bare_decorator(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") @skill.script def analyze(query: str) -> str: @@ -2328,7 +2640,7 @@ class TestSkillScriptDecorator: assert skill.scripts[0].function is analyze def test_parameterized_decorator(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") @skill.script(name="custom-name", description="Custom desc") def my_func() -> str: @@ -2341,7 +2653,7 @@ class TestSkillScriptDecorator: assert skill.scripts[0].function is my_func def test_multiple_scripts(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") @skill.script def script_a() -> str: @@ -2356,7 +2668,7 @@ class TestSkillScriptDecorator: assert skill.scripts[1].name == "script_b" def test_async_script(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") @skill.script async def fetch_data() -> str: @@ -2369,7 +2681,7 @@ class TestSkillScriptDecorator: assert skill.scripts[0].function is fetch_data def test_decorator_returns_original_function(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") @skill.script def original() -> str: @@ -2392,12 +2704,14 @@ class TestSkillWithScripts: """Tests for the Skill class with scripts attribute.""" def test_default_empty_scripts(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") assert skill.scripts == [] def test_scripts_at_construction(self) -> None: scripts = [InlineSkillScript(name="s1", function=lambda: None)] - skill = InlineSkill(name="my-skill", description="test", instructions="body", scripts=scripts) + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body", scripts=scripts + ) assert len(skill.scripts) == 1 assert skill.scripts[0].name == "s1" @@ -2414,12 +2728,12 @@ class TestSkillScriptRunnerProtocol: results: list[tuple] = [] async def my_runner(skill, script, args=None): - results.append((skill.name, script.name, args)) + results.append((skill.frontmatter.name, script.name, args)) return "executed" assert isinstance(my_runner, SkillScriptRunner) - skill = InlineSkill(name="test-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="test"), instructions="body") script = FileSkillScript(name="my-script", full_path=f"{_ABS}/test/scripts/run.py") skill.scripts.append(script) @@ -2437,7 +2751,7 @@ class TestSkillScriptRunnerProtocol: runner = _CustomRunner() assert isinstance(runner, SkillScriptRunner) - skill = InlineSkill(name="test-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="test"), instructions="body") script = InlineSkillScript(name="my-script", function=lambda: None) skill.scripts.append(script) @@ -2448,7 +2762,7 @@ class TestSkillScriptRunnerProtocol: async def noop_runner(skill, script, args=None): return None - skill = InlineSkill(name="test-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="test"), instructions="body") script = InlineSkillScript(name="s1", function=lambda: None) result = await noop_runner(skill, script) @@ -2458,7 +2772,7 @@ class TestSkillScriptRunnerProtocol: async def dict_runner(skill, script, args=None): return {"exit_code": 0, "output": "ok"} - skill = InlineSkill(name="test-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="test"), instructions="body") script = FileSkillScript(name="s1", full_path=f"{_ABS}/test/scripts/run.py") result = await dict_runner(skill, script) @@ -2468,12 +2782,12 @@ class TestSkillScriptRunnerProtocol: results: list[tuple] = [] def my_runner(skill, script, args=None): - results.append((skill.name, script.name, args)) + results.append((skill.frontmatter.name, script.name, args)) return "executed" assert isinstance(my_runner, SkillScriptRunner) - skill = InlineSkill(name="test-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="test"), instructions="body") script = FileSkillScript(name="my-script", full_path=f"{_ABS}/test/scripts/run.py") skill.scripts.append(script) @@ -2491,7 +2805,7 @@ class TestSkillScriptRunnerProtocol: runner = _SyncRunner() assert isinstance(runner, SkillScriptRunner) - skill = InlineSkill(name="test-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="test"), instructions="body") script = InlineSkillScript(name="my-script", function=lambda: None) skill.scripts.append(script) @@ -2502,7 +2816,7 @@ class TestSkillScriptRunnerProtocol: def noop_runner(skill, script, args=None): return None - skill = InlineSkill(name="test-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="test"), instructions="body") script = InlineSkillScript(name="s1", function=lambda: None) result = noop_runner(skill, script) @@ -2512,7 +2826,7 @@ class TestSkillScriptRunnerProtocol: def dict_runner(skill, script, args=None): return {"exit_code": 0, "output": "ok"} - skill = InlineSkill(name="test-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="test"), instructions="body") script = FileSkillScript(name="s1", full_path=f"{_ABS}/test/scripts/run.py") result = dict_runner(skill, script) @@ -2528,7 +2842,7 @@ class TestSkillsProviderFactories: """Tests for the SkillsProvider constructor auto-wiring behavior.""" async def test_code_skills_with_scripts_creates_provider(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider([skill]) @@ -2538,7 +2852,7 @@ class TestSkillsProviderFactories: assert any(hasattr(t, "name") and t.name == "run_skill_script" for t in _ctx(provider)[2]) async def test_code_skills_no_scripts(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") provider = SkillsProvider([skill]) await _init_provider(provider) # No scripts with functions, no runner, no resources — only load_skill @@ -2549,7 +2863,7 @@ class TestSkillsProviderFactories: def my_function(key: str = "") -> str: return f"executed: {key}" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=my_function)) provider = SkillsProvider([skill]) @@ -2560,7 +2874,7 @@ class TestSkillsProviderFactories: assert result == "executed: hello" async def test_no_scripts_no_tool(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") # No scripts at all — no run_skill_script tool provider = SkillsProvider([skill]) await _init_provider(provider) @@ -2568,14 +2882,14 @@ class TestSkillsProviderFactories: async def test_no_resources_no_read_skill_resource_tool(self) -> None: """When no skill has resources, read_skill_resource tool is not advertised.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") provider = SkillsProvider([skill]) await _init_provider(provider) assert not any(hasattr(t, "name") and t.name == "read_skill_resource" for t in _ctx(provider)[2]) async def test_resources_present_includes_read_skill_resource_tool(self) -> None: """When a skill has resources, read_skill_resource tool is advertised.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.resources.append(InlineSkillResource(name="ref", content="reference data")) provider = SkillsProvider([skill]) await _init_provider(provider) @@ -2583,7 +2897,7 @@ class TestSkillsProviderFactories: async def test_resources_present_includes_resource_instructions(self) -> None: """When a skill has resources, instructions mention read_skill_resource.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.resources.append(InlineSkillResource(name="ref", content="reference data")) provider = SkillsProvider([skill]) await _init_provider(provider) @@ -2591,14 +2905,14 @@ class TestSkillsProviderFactories: async def test_no_resources_excludes_resource_instructions(self) -> None: """When no skill has resources, instructions do not mention read_skill_resource.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") provider = SkillsProvider([skill]) await _init_provider(provider) assert "read_skill_resource" not in (_ctx(provider)[1] or "") async def test_read_skill_resource_tool_returns_content(self) -> None: """The read_skill_resource tool returns resource content when invoked.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.resources.append(InlineSkillResource(name="ref", content="reference data")) provider = SkillsProvider([skill]) await _init_provider(provider) @@ -2695,7 +3009,9 @@ class TestSkillsProviderFactories: encoding="utf-8", ) - code_skill = InlineSkill(name="code-skill", description="test", instructions="body") + code_skill = InlineSkill( + frontmatter=SkillFrontmatter(name="code-skill", description="test"), instructions="body" + ) code_skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider( @@ -2725,7 +3041,7 @@ class TestSkillsProviderFactories: async def test_file_script_error_without_runner(self) -> None: # A skill with both a code script and a file-based script - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="code-s", function=lambda: "ok")) skill.scripts.append(FileSkillScript(name="file-s", full_path=f"{_ABS}/test/scripts/s1.py")) @@ -2746,7 +3062,7 @@ class TestSkillsProviderFactories: async def async_func(x: int = 0) -> str: return f"async: {x}" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=async_func)) provider = SkillsProvider([skill]) @@ -2761,7 +3077,7 @@ class TestSkillsProviderFactories: def returns_dict() -> dict: return {"status": "ok", "value": 42} - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=returns_dict)) provider = SkillsProvider([skill]) @@ -2772,7 +3088,7 @@ class TestSkillsProviderFactories: async def test_code_script_returns_none(self) -> None: """Code-defined scripts returning None pass through as None.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider([skill]) @@ -2783,7 +3099,7 @@ class TestSkillsProviderFactories: async def test_script_with_path_errors_without_runner(self) -> None: """A file-based script without a runner should return an error.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="code-s", function=lambda: "ok")) skill.scripts.append(FileSkillScript(name="path-s", full_path=f"{_ABS}/test/scripts/s1.py")) @@ -2801,7 +3117,7 @@ class TestSkillsProviderFactories: assert "script_runner" in result or "Failed to run" in result async def test_run_skill_script_error_on_missing_skill(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider([skill]) @@ -2812,7 +3128,7 @@ class TestSkillsProviderFactories: assert "nonexistent" in result async def test_run_skill_script_sync_with_kwargs(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") @skill.script def greet(name: str, **kwargs: Any) -> str: @@ -2827,7 +3143,7 @@ class TestSkillsProviderFactories: assert result == "Hello Alice (user=u42)" async def test_run_skill_script_async_with_kwargs(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") @skill.script async def fetch(url: str, **kwargs: Any) -> str: @@ -2843,7 +3159,7 @@ class TestSkillsProviderFactories: async def test_run_skill_script_without_kwargs_ignores_extra_args(self) -> None: """Script functions without **kwargs should still work when runtime kwargs are passed.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") @skill.script def simple(query: str) -> str: @@ -2858,7 +3174,7 @@ class TestSkillsProviderFactories: async def test_run_skill_script_conflicting_args_and_kwargs_raises(self) -> None: """Conflicting keys in args and kwargs should raise TypeError.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") @skill.script def process(**kwargs: Any) -> str: @@ -2872,7 +3188,7 @@ class TestSkillsProviderFactories: assert "Error" in result async def test_run_skill_script_error_on_missing_script(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider([skill]) @@ -2883,7 +3199,7 @@ class TestSkillsProviderFactories: assert "nonexistent" in result async def test_run_skill_script_error_on_empty_names(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider([skill]) @@ -2897,7 +3213,7 @@ class TestSkillsProviderFactories: assert "Error" in result async def test_instructions_include_script_runner_hints(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider([skill]) @@ -2906,14 +3222,14 @@ class TestSkillsProviderFactories: assert "not as top-level tool parameters" in _ctx(provider)[1] async def test_no_scripts_no_runner_no_script_instructions(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") provider = SkillsProvider([skill]) await _init_provider(provider) # No scripts and no runner — instructions should not mention run_skill_script assert "run_skill_script" not in (_ctx(provider)[1] or "") async def test_tool_schema_args_description_mentions_key_format(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider([skill]) @@ -2925,7 +3241,7 @@ class TestSkillsProviderFactories: async def test_require_script_approval_sets_approval_mode(self) -> None: """When require_script_approval=True, the run_skill_script tool has approval_mode='always_require'.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider([skill], require_script_approval=True) @@ -2935,7 +3251,7 @@ class TestSkillsProviderFactories: async def test_require_script_approval_false_by_default(self) -> None: """By default, the run_skill_script tool has approval_mode='never_require'.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider([skill]) @@ -2945,7 +3261,7 @@ class TestSkillsProviderFactories: async def test_require_script_approval_does_not_affect_other_tools(self) -> None: """The load_skill tool should never require approval.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider([skill], require_script_approval=True) @@ -2961,7 +3277,7 @@ class TestSkillsProviderFactories: def failing_script() -> str: raise RuntimeError("Something went wrong") - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="boom", function=failing_script)) provider = SkillsProvider([skill]) @@ -2974,7 +3290,7 @@ class TestSkillsProviderFactories: async def test_custom_template_without_runner_placeholder_raises(self) -> None: """Provider with code scripts and custom template missing {runner_instructions} raises.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider( @@ -3138,7 +3454,7 @@ class TestCreateInstructionsWithScripts: """Tests for script metadata in skill advertisement.""" def test_excludes_script_count(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) result = SkillsProvider._create_instructions(None, [skill]) @@ -3146,7 +3462,7 @@ class TestCreateInstructionsWithScripts: assert "" not in result def test_no_scripts_element_when_empty(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") result = SkillsProvider._create_instructions(None, [skill]) assert result is not None @@ -3162,7 +3478,7 @@ class TestLoadSkillWithScripts: """Tests for script metadata in load_skill output.""" async def test_code_skill_includes_scripts_element(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="analyze", description="Run analysis", function=lambda: None)) provider = SkillsProvider([skill]) @@ -3174,7 +3490,7 @@ class TestLoadSkillWithScripts: assert 'description="Run analysis"' in result async def test_code_skill_no_scripts_element(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") provider = SkillsProvider([skill]) await _init_provider(provider) result = provider._load_skill(_raw_skills(provider), "my-skill") @@ -3190,7 +3506,7 @@ class _MinimalClassSkill(ClassSkill): """A minimal class-based skill with no resources or scripts.""" def __init__(self) -> None: - super().__init__(name="minimal-skill", description="A minimal skill.") + super().__init__(frontmatter=SkillFrontmatter(name="minimal-skill", description="A minimal skill.")) @property def instructions(self) -> str: @@ -3201,7 +3517,7 @@ class _FullClassSkill(ClassSkill): """A class-based skill with resources and scripts.""" def __init__(self) -> None: - super().__init__(name="full-skill", description="A full skill.") + super().__init__(frontmatter=SkillFrontmatter(name="full-skill", description="A full skill.")) self._resources: list[SkillResource] | None = None self._scripts: list[SkillScript] | None = None @@ -3318,7 +3634,7 @@ class TestClassSkill: skills = _raw_skills(provider) assert len(skills) == 1 - assert skills[0].name == "full-skill" + assert skills[0].frontmatter.name == "full-skill" async def test_provider_loads_class_skill_content(self) -> None: skill = _FullClassSkill() @@ -3335,16 +3651,18 @@ class TestClassSkill: source = InMemorySkillsSource([skill]) skills = await source.get_skills() assert len(skills) == 1 - assert skills[0].name == "minimal-skill" + assert skills[0].frontmatter.name == "minimal-skill" async def test_mixed_inline_and_class_skills(self) -> None: - inline = InlineSkill(name="inline-skill", description="Inline", instructions="inline body") + inline = InlineSkill( + frontmatter=SkillFrontmatter(name="inline-skill", description="Inline"), instructions="inline body" + ) class_skill = _MinimalClassSkill() provider = SkillsProvider([inline, class_skill]) await _init_provider(provider) skills = _raw_skills(provider) - names = {s.name for s in skills} + names = {s.frontmatter.name for s in skills} assert names == {"inline-skill", "minimal-skill"} async def test_class_skill_script_runs(self) -> None: @@ -3372,7 +3690,9 @@ class _DecoratorClassSkill(ClassSkill): """A class-based skill using @ClassSkill.resource and @ClassSkill.script decorators.""" def __init__(self) -> None: - super().__init__(name="decorator-skill", description="A decorator-discovered skill.") + super().__init__( + frontmatter=SkillFrontmatter(name="decorator-skill", description="A decorator-discovered skill.") + ) @property def instructions(self) -> str: @@ -3395,7 +3715,7 @@ class _BareDecoratorSkill(ClassSkill): """Skill using bare decorators (no arguments) — name/description from method.""" def __init__(self) -> None: - super().__init__(name="bare-skill", description="Bare decorator skill.") + super().__init__(frontmatter=SkillFrontmatter(name="bare-skill", description="Bare decorator skill.")) @property def instructions(self) -> str: @@ -3416,7 +3736,7 @@ class _DuplicateResourceSkill(ClassSkill): """Skill with duplicate resource names — should raise.""" def __init__(self) -> None: - super().__init__(name="dup-skill", description="Dup.") + super().__init__(frontmatter=SkillFrontmatter(name="dup-skill", description="Dup.")) @property def instructions(self) -> str: @@ -3435,7 +3755,7 @@ class _DuplicateScriptSkill(ClassSkill): """Skill with duplicate script names — should raise.""" def __init__(self) -> None: - super().__init__(name="dup-script-skill", description="Dup.") + super().__init__(frontmatter=SkillFrontmatter(name="dup-script-skill", description="Dup.")) @property def instructions(self) -> str: @@ -3454,7 +3774,7 @@ class _SelfAccessSkill(ClassSkill): """Skill where resource/script access instance state via self.""" def __init__(self, multiplier: int = 10) -> None: - super().__init__(name="self-access", description="Self access skill.") + super().__init__(frontmatter=SkillFrontmatter(name="self-access", description="Self access skill.")) self.multiplier = multiplier @property @@ -3581,7 +3901,7 @@ class TestClassSkillDecoratorDiscovery: skills = _raw_skills(provider) assert len(skills) == 1 - assert skills[0].name == "decorator-skill" + assert skills[0].frontmatter.name == "decorator-skill" def test_manual_override_wins(self) -> None: """A subclass that overrides resources/scripts bypasses decorator discovery.""" @@ -3688,7 +4008,7 @@ class TestClassSkillDecoratorDiscovery: class _BadOrder(ClassSkill): def __init__(self) -> None: - super().__init__(name="bad", description="bad") + super().__init__(frontmatter=SkillFrontmatter(name="bad", description="bad")) @property def instructions(self) -> str: @@ -3705,7 +4025,7 @@ class TestClassSkillDecoratorDiscovery: class _BadOrder(ClassSkill): def __init__(self) -> None: - super().__init__(name="bad", description="bad") + super().__init__(frontmatter=SkillFrontmatter(name="bad", description="bad")) @property def instructions(self) -> str: @@ -3722,7 +4042,7 @@ class TestClassSkillDecoratorDiscovery: class _BadName(ClassSkill): def __init__(self) -> None: - super().__init__(name="bad", description="bad") + super().__init__(frontmatter=SkillFrontmatter(name="bad", description="bad")) @property def instructions(self) -> str: @@ -3738,7 +4058,7 @@ class TestClassSkillDecoratorDiscovery: class _BadName(ClassSkill): def __init__(self) -> None: - super().__init__(name="bad", description="bad") + super().__init__(frontmatter=SkillFrontmatter(name="bad", description="bad")) @property def instructions(self) -> str: @@ -3754,7 +4074,7 @@ class TestClassSkillDecoratorDiscovery: class _EmptyName(ClassSkill): def __init__(self) -> None: - super().__init__(name="bad", description="bad") + super().__init__(frontmatter=SkillFrontmatter(name="bad", description="bad")) @property def instructions(self) -> str: @@ -3798,7 +4118,7 @@ class _ExplicitDescriptionSkill(ClassSkill): """Skill with explicit descriptions on decorator.""" def __init__(self) -> None: - super().__init__(name="desc-skill", description="Explicit desc.") + super().__init__(frontmatter=SkillFrontmatter(name="desc-skill", description="Explicit desc.")) @property def instructions(self) -> str: @@ -3817,7 +4137,7 @@ class _PropertyCallCountSkill(ClassSkill): """Tracks how many times the property getter is called.""" def __init__(self) -> None: - super().__init__(name="callcount-skill", description="Tracks calls.") + super().__init__(frontmatter=SkillFrontmatter(name="callcount-skill", description="Tracks calls.")) self.getter_call_count = 0 @property @@ -3847,7 +4167,7 @@ class _ChildSkill(_ParentSkill): """Child inheriting parent resources and adding its own.""" def __init__(self) -> None: - super().__init__(name="child-skill", description="Child.") + super().__init__(frontmatter=SkillFrontmatter(name="child-skill", description="Child.")) @property def instructions(self) -> str: @@ -3862,7 +4182,7 @@ class _KwargsSkill(ClassSkill): """Skill that uses **kwargs from runtime.""" def __init__(self) -> None: - super().__init__(name="kwargs-skill", description="Kwargs.") + super().__init__(frontmatter=SkillFrontmatter(name="kwargs-skill", description="Kwargs.")) @property def instructions(self) -> str: @@ -3886,7 +4206,7 @@ class _ChildWithInheritedPropertySkill(_ParentWithPropertyResource): """Child that should discover inherited property resource.""" def __init__(self) -> None: - super().__init__(name="child-prop-skill", description="Child prop.") + super().__init__(frontmatter=SkillFrontmatter(name="child-prop-skill", description="Child prop.")) @property def instructions(self) -> str: @@ -3897,7 +4217,7 @@ class _PropertyResourceSkill(ClassSkill): """Skill with a property-based resource.""" def __init__(self) -> None: - super().__init__(name="prop-skill", description="Property skill.") + super().__init__(frontmatter=SkillFrontmatter(name="prop-skill", description="Property skill.")) @property def instructions(self) -> str: @@ -3914,7 +4234,7 @@ class _MixedPropertyMethodSkill(ClassSkill): """Skill with both property and method resources.""" def __init__(self) -> None: - super().__init__(name="mixed-prop", description="Mixed.") + super().__init__(frontmatter=SkillFrontmatter(name="mixed-prop", description="Mixed.")) @property def instructions(self) -> str: @@ -3937,7 +4257,7 @@ class _MixedPropertyMethodSkill(ClassSkill): def analyze(query: str, limit: int = 10) -> str: return "result" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="analyze", description="Run analysis", function=analyze)) provider = SkillsProvider([skill]) @@ -3954,7 +4274,7 @@ class TestReadSkillResourceWithScripts: """Tests for _read_skill_resource falling back to scripts.""" async def test_reads_script_with_static_content(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="generate.py", function=lambda: "print('hello')")) provider = SkillsProvider([skill]) @@ -3964,7 +4284,7 @@ class TestReadSkillResourceWithScripts: assert "not found" in result async def test_script_not_accessible_via_read_resource(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="run.py", function=lambda: "script output")) provider = SkillsProvider([skill]) @@ -3977,7 +4297,7 @@ class TestReadSkillResourceWithScripts: async def async_script() -> str: return "async output" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="run.py", function=async_script)) provider = SkillsProvider([skill]) @@ -3986,7 +4306,7 @@ class TestReadSkillResourceWithScripts: assert "not found" in result async def test_script_case_insensitive_not_in_resources(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="Generate.py", function=lambda: "code")) provider = SkillsProvider([skill]) @@ -3995,7 +4315,7 @@ class TestReadSkillResourceWithScripts: assert "not found" in result async def test_resource_takes_priority_over_script(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.resources.append(InlineSkillResource(name="data.py", content="resource content")) skill.scripts.append(InlineSkillScript(name="data.py", function=lambda: "script content")) @@ -4008,7 +4328,7 @@ class TestReadSkillResourceWithScripts: def failing_script() -> str: raise RuntimeError("boom") - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="bad.py", function=failing_script)) provider = SkillsProvider([skill]) @@ -4217,7 +4537,7 @@ class TestLoadSkillsMerging: def test_code_skill_with_invalid_name_raises(self) -> None: """Code skills with invalid metadata (e.g. uppercase name) raise at construction.""" with pytest.raises(ValueError, match="Invalid skill name"): - InlineSkill(name="INVALID_NAME", description="valid", instructions="body") + InlineSkill(frontmatter=SkillFrontmatter(name="INVALID_NAME", description="valid"), instructions="body") async def test_file_skill_takes_precedence_over_code_skill(self, tmp_path: Path) -> None: """When file-based and code-defined skills share a name, file-based wins.""" @@ -4235,7 +4555,9 @@ class TestLoadSkillsMerging: encoding="utf-8", ) - code_skill = InlineSkill(name="my-skill", description="Code skill.", instructions="Code body.") + code_skill = InlineSkill( + frontmatter=SkillFrontmatter(name="my-skill", description="Code skill."), instructions="Code body." + ) source = DeduplicatingSkillsSource( AggregatingSkillsSource([ @@ -4244,7 +4566,7 @@ class TestLoadSkillsMerging: ]) ) result = await source.get_skills() - skills_by_name = {s.name: s for s in result} + skills_by_name = {s.frontmatter.name: s for s in result} assert "my-skill" in skills_by_name assert skills_by_name["my-skill"].path is not None # file-based skill has path set @@ -4269,7 +4591,7 @@ class TestSkillsSource: source = FileSkillsSource(str(tmp_path)) skills = await source.get_skills() assert len(skills) == 1 - assert skills[0].name == "my-skill" + assert skills[0].frontmatter.name == "my-skill" assert skills[0].path is not None async def test_file_skills_source_with_extensions(self, tmp_path: Path) -> None: @@ -4295,67 +4617,67 @@ class TestSkillsSource: """InMemorySkillsSource returns all provided skills.""" from agent_framework import InMemorySkillsSource - s1 = InlineSkill(name="skill-a", description="A", instructions="body") - s2 = InlineSkill(name="skill-b", description="B", instructions="body") + s1 = InlineSkill(frontmatter=SkillFrontmatter(name="skill-a", description="A"), instructions="body") + s2 = InlineSkill(frontmatter=SkillFrontmatter(name="skill-b", description="B"), instructions="body") source = InMemorySkillsSource([s1, s2]) skills = await source.get_skills() assert len(skills) == 2 - assert skills[0].name == "skill-a" - assert skills[1].name == "skill-b" + assert skills[0].frontmatter.name == "skill-a" + assert skills[1].frontmatter.name == "skill-b" async def test_aggregating_source_combines_sources(self) -> None: """Aggregating source concatenates results from multiple sources.""" from agent_framework import AggregatingSkillsSource, InMemorySkillsSource - s1 = InlineSkill(name="skill-a", description="A", instructions="body") - s2 = InlineSkill(name="skill-b", description="B", instructions="body") + s1 = InlineSkill(frontmatter=SkillFrontmatter(name="skill-a", description="A"), instructions="body") + s2 = InlineSkill(frontmatter=SkillFrontmatter(name="skill-b", description="B"), instructions="body") source = AggregatingSkillsSource([ InMemorySkillsSource([s1]), InMemorySkillsSource([s2]), ]) skills = await source.get_skills() - names = [s.name for s in skills] + names = [s.frontmatter.name for s in skills] assert names == ["skill-a", "skill-b"] async def test_filtering_source_filters_by_predicate(self) -> None: """FilteringSkillsSource only returns skills matching the predicate.""" from agent_framework import FilteringSkillsSource, InMemorySkillsSource - s1 = InlineSkill(name="keep-me", description="keep", instructions="body") - s2 = InlineSkill(name="drop-me", description="drop", instructions="body") + s1 = InlineSkill(frontmatter=SkillFrontmatter(name="keep-me", description="keep"), instructions="body") + s2 = InlineSkill(frontmatter=SkillFrontmatter(name="drop-me", description="drop"), instructions="body") source = FilteringSkillsSource( InMemorySkillsSource([s1, s2]), - predicate=lambda s: s.name.startswith("keep"), + predicate=lambda s: s.frontmatter.name.startswith("keep"), ) skills = await source.get_skills() assert len(skills) == 1 - assert skills[0].name == "keep-me" + assert skills[0].frontmatter.name == "keep-me" async def test_deduplicating_source_removes_duplicates(self) -> None: """DeduplicatingSkillsSource keeps first skill with each name.""" from agent_framework import DeduplicatingSkillsSource, InMemorySkillsSource - s1 = InlineSkill(name="my-skill", description="first", instructions="body1") - s2 = InlineSkill(name="my-skill", description="second", instructions="body2") - s3 = InlineSkill(name="other", description="other", instructions="body3") + s1 = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="first"), instructions="body1") + s2 = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="second"), instructions="body2") + s3 = InlineSkill(frontmatter=SkillFrontmatter(name="other", description="other"), instructions="body3") source = DeduplicatingSkillsSource(InMemorySkillsSource([s1, s2, s3])) skills = await source.get_skills() assert len(skills) == 2 - names = {s.name for s in skills} + names = {s.frontmatter.name for s in skills} assert names == {"my-skill", "other"} # First one wins - my_skill = next(s for s in skills if s.name == "my-skill") - assert my_skill.description == "first" + my_skill = next(s for s in skills if s.frontmatter.name == "my-skill") + assert my_skill.frontmatter.description == "first" async def test_delegating_source_delegates(self) -> None: """DelegatingSkillsSource delegates to inner source by default.""" from agent_framework import DelegatingSkillsSource, InMemorySkillsSource - skill = InlineSkill(name="test-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="test"), instructions="body") inner = InMemorySkillsSource([skill]) class PassthroughSource(DelegatingSkillsSource): @@ -4365,7 +4687,7 @@ class TestSkillsSource: assert source.inner_source is inner skills = await source.get_skills() assert len(skills) == 1 - assert skills[0].name == "test-skill" + assert skills[0].frontmatter.name == "test-skill" async def test_provider_with_source_parameter(self, tmp_path: Path) -> None: """SkillsProvider works with the new source= parameter.""" @@ -4385,7 +4707,9 @@ class TestSkillsSource: """When source= is provided, skill_paths and skills are ignored.""" from agent_framework import InMemorySkillsSource - code_skill = InlineSkill(name="code-skill", description="test", instructions="body") + code_skill = InlineSkill( + frontmatter=SkillFrontmatter(name="code-skill", description="test"), instructions="body" + ) source = InMemorySkillsSource([code_skill]) # Pass skill_paths that would normally discover file skills — should be ignored @@ -4411,8 +4735,12 @@ class TestSkillsSource: encoding="utf-8", ) - code_skill = InlineSkill(name="code-skill", description="Code.", instructions="Body.") - internal = InlineSkill(name="internal", description="Internal.", instructions="Body.") + code_skill = InlineSkill( + frontmatter=SkillFrontmatter(name="code-skill", description="Code."), instructions="Body." + ) + internal = InlineSkill( + frontmatter=SkillFrontmatter(name="internal", description="Internal."), instructions="Body." + ) source = FilteringSkillsSource( DeduplicatingSkillsSource( @@ -4421,11 +4749,11 @@ class TestSkillsSource: InMemorySkillsSource([code_skill, internal]), ]) ), - predicate=lambda s: s.name != "internal", + predicate=lambda s: s.frontmatter.name != "internal", ) skills = await source.get_skills() - names = {s.name for s in skills} + names = {s.frontmatter.name for s in skills} assert names == {"file-skill", "code-skill"} assert "internal" not in names @@ -4453,15 +4781,15 @@ class TestSourceComposition: async def test_code_skills_with_provider(self) -> None: """InMemorySkillsSource with code skills creates a working provider.""" - skill = InlineSkill(name="code-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="code-skill", description="test"), instructions="body") provider = SkillsProvider(DeduplicatingSkillsSource(InMemorySkillsSource([skill]))) await _init_provider(provider) assert "code-skill" in _ctx(provider)[0] async def test_multiple_code_skills(self) -> None: """InMemorySkillsSource with multiple skills registers them all.""" - s1 = InlineSkill(name="skill-a", description="A", instructions="body") - s2 = InlineSkill(name="skill-b", description="B", instructions="body") + s1 = InlineSkill(frontmatter=SkillFrontmatter(name="skill-a", description="A"), instructions="body") + s2 = InlineSkill(frontmatter=SkillFrontmatter(name="skill-b", description="B"), instructions="body") provider = SkillsProvider(DeduplicatingSkillsSource(InMemorySkillsSource([s1, s2]))) await _init_provider(provider) assert "skill-a" in _ctx(provider)[0] @@ -4469,7 +4797,7 @@ class TestSourceComposition: async def test_custom_source_with_provider(self) -> None: """Custom source passed to SkillsProvider works.""" - skill = InlineSkill(name="custom", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="custom", description="test"), instructions="body") source = InMemorySkillsSource([skill]) provider = SkillsProvider(DeduplicatingSkillsSource(source)) await _init_provider(provider) @@ -4479,13 +4807,13 @@ class TestSourceComposition: """FilteringSkillsSource excludes matching skills.""" from agent_framework import FilteringSkillsSource - s1 = InlineSkill(name="keep-me", description="keep", instructions="body") - s2 = InlineSkill(name="drop-me", description="drop", instructions="body") + s1 = InlineSkill(frontmatter=SkillFrontmatter(name="keep-me", description="keep"), instructions="body") + s2 = InlineSkill(frontmatter=SkillFrontmatter(name="drop-me", description="drop"), instructions="body") source = DeduplicatingSkillsSource( FilteringSkillsSource( InMemorySkillsSource([s1, s2]), - predicate=lambda s: s.name.startswith("keep"), + predicate=lambda s: s.frontmatter.name.startswith("keep"), ) ) provider = SkillsProvider(source) @@ -4495,8 +4823,8 @@ class TestSourceComposition: async def test_dedup_across_sources(self) -> None: """DeduplicatingSkillsSource deduplicates across aggregated sources.""" - s1 = InlineSkill(name="dup", description="first", instructions="body1") - s2 = InlineSkill(name="dup", description="second", instructions="body2") + s1 = InlineSkill(frontmatter=SkillFrontmatter(name="dup", description="first"), instructions="body1") + s2 = InlineSkill(frontmatter=SkillFrontmatter(name="dup", description="second"), instructions="body2") source = DeduplicatingSkillsSource( AggregatingSkillsSource([ @@ -4507,7 +4835,7 @@ class TestSourceComposition: provider = SkillsProvider(source) await _init_provider(provider) assert len(_ctx(provider)[0]) == 1 - assert _ctx(provider)[0]["dup"].description == "first" + assert _ctx(provider)[0]["dup"].frontmatter.description == "first" async def test_file_source_with_script_runner(self, tmp_path: Path) -> None: """FileSkillsSource with script_runner enables script execution.""" @@ -4527,7 +4855,7 @@ class TestSourceComposition: async def test_script_approval_on_provider(self) -> None: """SkillsProvider with require_script_approval sets the approval mode.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider( @@ -4616,13 +4944,13 @@ class TestSkillsProviderFactoryMethods: def test_init_with_skills_creates_provider(self) -> None: """Constructor with skill list returns a SkillsProvider instance.""" - skill = InlineSkill(name="test-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="Test"), instructions="Body") provider = SkillsProvider([skill]) assert isinstance(provider, SkillsProvider) async def test_init_with_skills_registers_skills(self) -> None: """Constructor with skill list registers code-defined skills.""" - skill = InlineSkill(name="test-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="Test"), instructions="Body") provider = SkillsProvider([skill]) await _init_provider(provider) assert "test-skill" in _ctx(provider)[0] @@ -4635,7 +4963,7 @@ class TestSkillsProviderFactoryMethods: async def test_init_with_skills_and_options(self) -> None: """Constructor with skills passes through keyword options.""" - skill = InlineSkill(name="my-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="Test"), instructions="Body") provider = SkillsProvider( [skill], require_script_approval=True, @@ -4648,7 +4976,7 @@ class TestSkillsProviderFactoryMethods: """Constructor with SkillsSource returns a SkillsProvider instance.""" from agent_framework import InMemorySkillsSource - skill = InlineSkill(name="test-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="Test"), instructions="Body") source = InMemorySkillsSource([skill]) provider = SkillsProvider(source) assert isinstance(provider, SkillsProvider) @@ -4657,7 +4985,7 @@ class TestSkillsProviderFactoryMethods: """Constructor with SkillsSource uses the exact source given.""" from agent_framework import InMemorySkillsSource - skill = InlineSkill(name="test-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="Test"), instructions="Body") source = InMemorySkillsSource([skill]) provider = SkillsProvider(source) await _init_provider(provider) @@ -4674,7 +5002,7 @@ class TestDisableCaching: async def test_default_caching_enabled(self) -> None: """By default, _get_or_create_context only builds once.""" - skill = InlineSkill(name="test-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="Test"), instructions="Body") provider = SkillsProvider([skill]) await _init_provider(provider) first_ctx = provider._cached_context # pyright: ignore[reportPrivateUsage] @@ -4686,7 +5014,7 @@ class TestDisableCaching: async def test_disable_caching_rebuilds_on_every_call(self) -> None: """With disable_caching=True, _create_context rebuilds every time.""" - skill = InlineSkill(name="test-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="Test"), instructions="Body") provider = SkillsProvider([skill], disable_caching=True) await _init_provider(provider) first_ctx = provider._cached_context # pyright: ignore[reportPrivateUsage] @@ -4700,20 +5028,20 @@ class TestDisableCaching: """disable_caching works via the primary constructor.""" from agent_framework import InMemorySkillsSource - skill = InlineSkill(name="test-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="Test"), instructions="Body") source = InMemorySkillsSource([skill]) provider = SkillsProvider(source, disable_caching=True) assert provider._disable_caching is True async def test_caching_enabled_by_default(self) -> None: """SkillsProvider defaults to caching enabled.""" - skill = InlineSkill(name="test-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="Test"), instructions="Body") provider = SkillsProvider([skill]) assert provider._disable_caching is False async def test_disable_caching_before_run_rebuilds(self) -> None: """before_run with disable_caching=True calls _create_context each time.""" - skill = InlineSkill(name="test-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="Test"), instructions="Body") provider = SkillsProvider([skill], disable_caching=True) context = SessionContext(input_messages=[]) await provider.before_run(agent=AsyncMock(), session=AsyncMock(), context=context, state={}) @@ -4730,7 +5058,7 @@ class TestSkillsProviderConstructorEdgeCases: async def test_single_skill_accepted(self) -> None: """A single Skill (not a list) is accepted and wrapped.""" - skill = InlineSkill(name="test-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="Test"), instructions="Body") provider = SkillsProvider(skill) await _init_provider(provider) skills = _ctx(provider)[0] @@ -4739,7 +5067,7 @@ class TestSkillsProviderConstructorEdgeCases: async def test_template_missing_skills_placeholder_raises(self) -> None: """Instruction template without {skills} raises ValueError.""" - skill = InlineSkill(name="test-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="Test"), instructions="Body") provider = SkillsProvider([skill], instruction_template="No placeholder here.") with pytest.raises(ValueError, match="skills"): await _init_provider(provider) @@ -4765,7 +5093,7 @@ class TestInlineSkillContentCaching: def test_content_cached_after_first_access(self) -> None: """InlineSkill.content returns the same object on subsequent accesses.""" - skill = InlineSkill(name="test-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="Test"), instructions="Body") first = skill.content second = skill.content assert first is second # Same object (cached) diff --git a/python/samples/02-agents/skills/code_defined_skill/code_defined_skill.py b/python/samples/02-agents/skills/code_defined_skill/code_defined_skill.py index 15388dd695..42fd0a38de 100644 --- a/python/samples/02-agents/skills/code_defined_skill/code_defined_skill.py +++ b/python/samples/02-agents/skills/code_defined_skill/code_defined_skill.py @@ -11,7 +11,7 @@ import os from textwrap import dedent from typing import Any -from agent_framework import Agent, InlineSkill, InlineSkillResource, SkillsProvider +from agent_framework import Agent, InlineSkill, InlineSkillResource, SkillFrontmatter, SkillsProvider from agent_framework.foundry import FoundryChatClient from azure.identity import AzureCliCredential from dotenv import load_dotenv @@ -47,8 +47,9 @@ load_dotenv() # 1. Static Resources — inline content passed at construction time # --------------------------------------------------------------------------- unit_converter_skill = InlineSkill( - name="unit-converter", - description="Convert between common units using a conversion factor", + frontmatter=SkillFrontmatter( + name="unit-converter", description="Convert between common units using a conversion factor" + ), instructions=dedent("""\ Use this skill when the user asks to convert between units. diff --git a/python/samples/02-agents/skills/file_based_skill/skills/unit-converter/SKILL.md b/python/samples/02-agents/skills/file_based_skill/skills/unit-converter/SKILL.md index b6e6bef1a3..7660365328 100644 --- a/python/samples/02-agents/skills/file_based_skill/skills/unit-converter/SKILL.md +++ b/python/samples/02-agents/skills/file_based_skill/skills/unit-converter/SKILL.md @@ -1,6 +1,12 @@ --- name: unit-converter description: Convert between common units using a multiplication factor. Use when asked to convert miles, kilometers, pounds, or kilograms. +license: MIT +compatibility: Works with any model that supports tool use. +allowed-tools: convert +metadata: + author: agent-framework-samples + version: "1.0" --- ## Usage diff --git a/python/samples/02-agents/skills/mixed_skills/mixed_skills.py b/python/samples/02-agents/skills/mixed_skills/mixed_skills.py index 2b10fc0c2a..2f89074cbd 100644 --- a/python/samples/02-agents/skills/mixed_skills/mixed_skills.py +++ b/python/samples/02-agents/skills/mixed_skills/mixed_skills.py @@ -21,6 +21,7 @@ from agent_framework import ( FileSkillsSource, InlineSkill, InMemorySkillsSource, + SkillFrontmatter, SkillsProvider, ) from agent_framework.foundry import FoundryChatClient @@ -73,8 +74,9 @@ load_dotenv() # --------------------------------------------------------------------------- volume_converter_skill = InlineSkill( - name="volume-converter", - description="Convert between gallons and liters using a conversion factor", + frontmatter=SkillFrontmatter( + name="volume-converter", description="Convert between gallons and liters using a conversion factor" + ), instructions=dedent("""\ Use this skill when the user asks to convert between gallons and liters. @@ -118,6 +120,7 @@ def convert_volume(value: float, factor: float) -> str: # 2. Define a class-based skill for temperature conversion # --------------------------------------------------------------------------- + class TemperatureConverterSkill(ClassSkill): """A temperature-converter skill defined as a Python class. @@ -127,8 +130,10 @@ class TemperatureConverterSkill(ClassSkill): def __init__(self) -> None: super().__init__( - name="temperature-converter", - description="Convert between temperature scales (Fahrenheit, Celsius, Kelvin).", + frontmatter=SkillFrontmatter( + name="temperature-converter", + description="Convert between temperature scales (Fahrenheit, Celsius, Kelvin).", + ) ) @property @@ -178,6 +183,7 @@ class TemperatureConverterSkill(ClassSkill): # 3. Wire everything together and run the agent # --------------------------------------------------------------------------- + async def main() -> None: """Run the combined skills demo.""" endpoint = os.environ["FOUNDRY_PROJECT_ENDPOINT"] diff --git a/python/samples/02-agents/skills/mixed_skills/skills/unit-converter/SKILL.md b/python/samples/02-agents/skills/mixed_skills/skills/unit-converter/SKILL.md index b6e6bef1a3..7660365328 100644 --- a/python/samples/02-agents/skills/mixed_skills/skills/unit-converter/SKILL.md +++ b/python/samples/02-agents/skills/mixed_skills/skills/unit-converter/SKILL.md @@ -1,6 +1,12 @@ --- name: unit-converter description: Convert between common units using a multiplication factor. Use when asked to convert miles, kilometers, pounds, or kilograms. +license: MIT +compatibility: Works with any model that supports tool use. +allowed-tools: convert +metadata: + author: agent-framework-samples + version: "1.0" --- ## Usage diff --git a/python/samples/02-agents/skills/script_approval/script_approval.py b/python/samples/02-agents/skills/script_approval/script_approval.py index bd956dec61..8687bf6867 100644 --- a/python/samples/02-agents/skills/script_approval/script_approval.py +++ b/python/samples/02-agents/skills/script_approval/script_approval.py @@ -9,7 +9,7 @@ import os # warnings.filterwarnings("ignore", message=r"\[SKILLS\].*", category=FutureWarning) from textwrap import dedent -from agent_framework import Agent, InlineSkill, SkillsProvider +from agent_framework import Agent, InlineSkill, SkillFrontmatter, SkillsProvider from agent_framework.foundry import FoundryChatClient from azure.identity import AzureCliCredential from dotenv import load_dotenv @@ -43,8 +43,9 @@ load_dotenv() # Define a code skill with a script that performs a sensitive operation deployment_skill = InlineSkill( - name="deployment", - description="Tools for deploying application versions to production", + frontmatter=SkillFrontmatter( + name="deployment", description="Tools for deploying application versions to production" + ), instructions=dedent("""\ Use this skill when the user asks to deploy an application. diff --git a/python/samples/02-agents/skills/skill_filtering/skill_filtering.py b/python/samples/02-agents/skills/skill_filtering/skill_filtering.py index 73dffb4c71..55eea099d6 100644 --- a/python/samples/02-agents/skills/skill_filtering/skill_filtering.py +++ b/python/samples/02-agents/skills/skill_filtering/skill_filtering.py @@ -75,7 +75,7 @@ async def main() -> None: FilteringSkillsSource( FileSkillsSource(str(skills_dir), script_runner=subprocess_script_runner), # Only keep the volume-converter skill - predicate=lambda s: s.name != "length-converter", + predicate=lambda s: s.frontmatter.name != "length-converter", ) ) diff --git a/python/samples/02-agents/skills/skill_filtering/skills/length-converter/SKILL.md b/python/samples/02-agents/skills/skill_filtering/skills/length-converter/SKILL.md index cbf506f683..c73c26ab7b 100644 --- a/python/samples/02-agents/skills/skill_filtering/skills/length-converter/SKILL.md +++ b/python/samples/02-agents/skills/skill_filtering/skills/length-converter/SKILL.md @@ -1,6 +1,12 @@ --- name: length-converter description: Convert between common length units (miles, km, feet, meters) using a multiplication factor. +license: MIT +compatibility: Works with any model that supports tool use. +allowed-tools: convert +metadata: + author: agent-framework-samples + version: "1.0" --- ## Usage diff --git a/python/samples/02-agents/skills/skill_filtering/skills/volume-converter/SKILL.md b/python/samples/02-agents/skills/skill_filtering/skills/volume-converter/SKILL.md index 6e10cb46b1..0c729f3e22 100644 --- a/python/samples/02-agents/skills/skill_filtering/skills/volume-converter/SKILL.md +++ b/python/samples/02-agents/skills/skill_filtering/skills/volume-converter/SKILL.md @@ -1,6 +1,12 @@ --- name: volume-converter description: Convert between gallons and liters using a conversion factor. +license: MIT +compatibility: Works with any model that supports tool use. +allowed-tools: convert +metadata: + author: agent-framework-samples + version: "1.0" --- ## Usage From 09a3d0d30758eff71b0ae97af3fc83194e928533 Mon Sep 17 00:00:00 2001 From: Evan Mattson <35585003+moonbox3@users.noreply.github.com> Date: Thu, 14 May 2026 07:09:04 +0900 Subject: [PATCH 2/8] Python: Strip server-issued response item IDs under storage (#3295) (#5690) Fixes microsoft/agent-framework#3295. When the OpenAI Responses chat client sends a request that carries previous_response_id / conversation_id / conversation, the server already has the prior turn's response items and rejects duplicates with "Duplicate item found with id fc_xxx". The chat client was re-sending them inline whenever the input messages still carried the items in additional_properties (workflow replay, history providers, etc.), which broke any tool-using agent with persistent history. Decisions: - Single chokepoint: _prepare_message_for_openai. When the resulting request uses service-side storage, drop function_call, reasoning, approval-request/response, and local-shell-call items from the wire input. Keep function_result with its call_id; the server pairs it to the prior function_call via that key. - function_result is preserved unconditionally except for the local-shell variant, which carries its own server-issued item id. - No public API change. Wire format change is subtractive and only on requests that would otherwise 400. - Re-pointed the strict-xfail in test_full_conversation.py from #4047 to #3295. Kept xfail because the test asserts executor-level session-id clearing, which is the defense-in-depth half tracked by 3295-03; this slice closes the wire-level half. Files: - python/packages/openai/agent_framework_openai/_chat_client.py: strip rule applied alongside the existing reasoning-item branch. - python/packages/openai/tests/openai/test_openai_chat_client.py: four new tests pin the contract (function_call, approval, local-shell-call stripped under storage; everything kept without storage). Updated pre-existing tests that exercised the storage-on path to either pass request_uses_service_side_storage=False explicitly or assert the new strip behavior. - python/packages/foundry/tests/foundry/test_foundry_chat_client.py: same explicit storage-off opt-in for the inherited test. - python/packages/core/tests/workflow/test_full_conversation.py: re-pointed xfail reason to #3295 and the executor-level follow-up. Notes for next iteration: - 3295-01 (HITL wire-format validation against live OpenAI/Foundry) was not run; it requires the user's API credentials. The PRD design is locked but the empirical confirmation is still pending. If script 3 fails on either provider, this slice may need to be revisited. - 3295-03 (clear service_session_id in AgentExecutor on full-history replay) remains open. After it lands the xfail in test_full_conversation.py can be removed. - pytest was not run in this iteration because uv-based pytest commands required interactive approval. Validation rests on careful reading; next iteration should run the openai + core test suites. --- .../tests/workflow/test_full_conversation.py | 7 +- .../tests/foundry/test_foundry_chat_client.py | 2 +- .../agent_framework_openai/_chat_client.py | 50 +- .../tests/openai/test_openai_chat_client.py | 460 +++++++++++------- 4 files changed, 319 insertions(+), 200 deletions(-) diff --git a/python/packages/core/tests/workflow/test_full_conversation.py b/python/packages/core/tests/workflow/test_full_conversation.py index 5d9ce45018..79d8626bc2 100644 --- a/python/packages/core/tests/workflow/test_full_conversation.py +++ b/python/packages/core/tests/workflow/test_full_conversation.py @@ -429,7 +429,12 @@ class _FullHistoryReplayCoordinator(Executor): @pytest.mark.xfail( - reason="reset_service_session support not yet implemented — see #4047", + reason=( + "Tracks the executor-layer half of #3295: AgentExecutor should clear service_session_id " + "when handed a full prior conversation. The wire-level 'Duplicate item' API error is " + "already closed by the chat-client strip in #3295; this xfail covers the defense-in-depth " + "follow-up that makes the executor wiring reflect intent." + ), strict=True, ) async def test_run_request_with_full_history_clears_service_session_id() -> None: diff --git a/python/packages/foundry/tests/foundry/test_foundry_chat_client.py b/python/packages/foundry/tests/foundry/test_foundry_chat_client.py index 5dd0806604..eb8ff5937e 100644 --- a/python/packages/foundry/tests/foundry/test_foundry_chat_client.py +++ b/python/packages/foundry/tests/foundry/test_foundry_chat_client.py @@ -435,7 +435,7 @@ async def test_chat_message_parsing_with_function_calls() -> None: Message(role="tool", contents=[function_result]), ] - prepared_messages = client._prepare_messages_for_openai(messages) + prepared_messages = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=False) assert prepared_messages == [ { diff --git a/python/packages/openai/agent_framework_openai/_chat_client.py b/python/packages/openai/agent_framework_openai/_chat_client.py index af7995dc45..40d9063b12 100644 --- a/python/packages/openai/agent_framework_openai/_chat_client.py +++ b/python/packages/openai/agent_framework_openai/_chat_client.py @@ -1409,29 +1409,31 @@ class RawOpenAIChatClient( # type: ignore[misc] } additional_properties = message.additional_properties replays_local_storage = "_attribution" in additional_properties - uses_service_side_storage = request_uses_service_side_storage and not replays_local_storage - # Reasoning items are only valid in input when they directly preceded a function_call - # in the same response. Including a reasoning item that preceded a text response - # (i.e. no function_call in the same message) causes an API error: - # "reasoning was provided without its required following item." - # - # Local storage is stricter: response-scoped reasoning items (rs_*) cannot be replayed - # back to the service unless that message is using service-side storage. - # In that mode we omit reasoning items and rely on function call + tool output replay. - has_function_call = any(c.type == "function_call" for c in message.contents) + # Server-issued response item identities (function_call fc_*, reasoning rs_*, approval IDs, + # local-shell-call IDs) must not be re-sent inline when the request carries + # previous_response_id / conversation_id / conversation: the server already has them via + # the prior response and rejects duplicates with "Duplicate item found with id ...". + # function_result keeps its call_id and the server pairs it to the prior function_call via + # that key. See microsoft/agent-framework#3295. The strip is gated on the request-level + # flag, not a message-level one: HistoryProvider-attributed messages + # (replays_local_storage) still need stripping when the request also carries a continuation + # marker, since the server-stored items would otherwise duplicate the inline ones. Without + # storage, standalone reasoning items are invalid per the API ("reasoning was provided + # without its required following item"), so the reasoning branch always drops. for content in message.contents: match content.type: case "text_reasoning": - if not uses_service_side_storage or not has_function_call: - continue # reasoning not followed by a function_call is invalid in input - reasoning = self._prepare_content_for_openai( - message.role, - content, - replays_local_storage=replays_local_storage, - ) - if reasoning: - all_messages.append(reasoning) + continue case "function_result": + if request_uses_service_side_storage: + props = content.additional_properties or {} + # Local-shell variant serializes as `local_shell_call` carrying a server-issued id; + # plain function_call_output pairs by call_id and is safe under storage. + if ( + props.get(OPENAI_SHELL_OUTPUT_TYPE_KEY) == OPENAI_SHELL_OUTPUT_TYPE_LOCAL_SHELL_CALL + and props.get(OPENAI_LOCAL_SHELL_CALL_ITEM_ID_KEY) + ): + continue new_args: dict[str, Any] = {} new_args.update( self._prepare_content_for_openai( @@ -1443,6 +1445,8 @@ class RawOpenAIChatClient( # type: ignore[misc] if new_args: all_messages.append(new_args) case "function_call": + if request_uses_service_side_storage: + continue function_call = self._prepare_content_for_openai( message.role, content, @@ -1451,6 +1455,8 @@ class RawOpenAIChatClient( # type: ignore[misc] if function_call: all_messages.append(function_call) case "function_approval_response" | "function_approval_request": + if request_uses_service_side_storage: + continue prepared = self._prepare_content_for_openai( message.role, content, @@ -1463,6 +1469,12 @@ class RawOpenAIChatClient( # type: ignore[misc] # top-level mcp_call input item; the result side emits an # internal marker that `_prepare_messages_for_openai` # coalesces onto the matching call (or drops if unmatched). + # The mcp_call item carries the model-emitted call_id as its + # server-side `id`, so under continuation it would duplicate + # the prior response's items (#3295). Drop the call here; the + # orphan result is dropped by the coalesce step that follows. + if request_uses_service_side_storage: + continue prepared_mcp = self._prepare_content_for_openai( message.role, content, diff --git a/python/packages/openai/tests/openai/test_openai_chat_client.py b/python/packages/openai/tests/openai/test_openai_chat_client.py index 2f314927b9..325986a730 100644 --- a/python/packages/openai/tests/openai/test_openai_chat_client.py +++ b/python/packages/openai/tests/openai/test_openai_chat_client.py @@ -1,6 +1,5 @@ # Copyright (c) Microsoft. All rights reserved. -import asyncio import base64 import inspect import json @@ -121,15 +120,6 @@ async def create_vector_store( if result.last_error is not None: raise Exception(f"Vector store file processing failed with status: {result.last_error.message}") - # Wait for the vector store index to be fully searchable. - # create_and_poll confirms file processing, but the search index is eventually consistent. - for _ in range(10): - vs = await client.client.vector_stores.retrieve(vector_store.id) - if vs.file_counts.completed >= 1 and vs.file_counts.in_progress == 0: - break - await asyncio.sleep(1) - await asyncio.sleep(2) - return file.id, Content.from_hosted_vector_store(vector_store_id=vector_store.id) @@ -343,76 +333,6 @@ async def test_get_response_with_all_parameters() -> None: assert run_options["input"][1]["content"][0]["text"] == "Test message" -def test_openai_chat_options_declares_verbosity_field() -> None: - """OpenAIChatOptions declares verbosity as a typed Literal field.""" - from typing import get_args, get_type_hints - - from agent_framework_openai import OpenAIChatOptions - - annotations = get_type_hints(OpenAIChatOptions) - assert "verbosity" in annotations - assert {"low", "medium", "high"} <= set(get_args(annotations["verbosity"])) - - -async def test_verbosity_option_translates_to_text_field() -> None: - """Top-level verbosity is translated to text.verbosity for the Responses API.""" - client = OpenAIChatClient(model="test-model", api_key="test-key") - _, run_options, _ = await client._prepare_request( - messages=[Message(role="user", contents=["Test message"])], - options={"verbosity": "low"}, - ) - - assert "verbosity" not in run_options - assert run_options["text"] == {"verbosity": "low"} - - -async def test_verbosity_option_merges_with_response_format() -> None: - """Verbosity merges into text config alongside response_format-derived format.""" - client = OpenAIChatClient(model="test-model", api_key="test-key") - _, run_options, _ = await client._prepare_request( - messages=[Message(role="user", contents=["Test message"])], - options={ - "verbosity": "high", - "response_format": OutputStruct, - }, - ) - - assert "verbosity" not in run_options - assert run_options["text"]["verbosity"] == "high" - assert run_options["text_format"] is OutputStruct - - -async def test_verbosity_option_top_level_overrides_nested_text_verbosity() -> None: - """When both top-level and text['verbosity'] are set, the top-level value wins.""" - client = OpenAIChatClient(model="test-model", api_key="test-key") - _, run_options, _ = await client._prepare_request( - messages=[Message(role="user", contents=["Test message"])], - options={ - "verbosity": "high", - "text": {"verbosity": "low"}, - }, - ) - - assert "verbosity" not in run_options - assert run_options["text"]["verbosity"] == "high" - - -async def test_verbosity_option_merges_with_explicit_text_config() -> None: - """Verbosity merges into a user-provided text config without overwriting other keys.""" - client = OpenAIChatClient(model="test-model", api_key="test-key") - _, run_options, _ = await client._prepare_request( - messages=[Message(role="user", contents=["Test message"])], - options={ - "verbosity": "medium", - "text": {"format": {"type": "text"}}, - }, - ) - - assert "verbosity" not in run_options - assert run_options["text"]["verbosity"] == "medium" - assert run_options["text"]["format"] == {"type": "text"} - - @pytest.mark.asyncio async def test_web_search_tool_with_location() -> None: """Test web search tool with location parameters.""" @@ -518,7 +438,7 @@ async def test_chat_message_parsing_with_function_calls() -> None: Message(role="tool", contents=[function_result]), ] - prepared_messages = client._prepare_messages_for_openai(messages) + prepared_messages = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=False) assert prepared_messages == [ { @@ -1834,7 +1754,7 @@ def test_prepare_message_for_openai_with_function_approval_response() -> None: message = Message(role="user", contents=[approval_response]) - result = client._prepare_message_for_openai(message) + result = client._prepare_message_for_openai(message, request_uses_service_side_storage=False) # FunctionApprovalResponseContent is added directly, not nested in args with role assert len(result) == 1 @@ -1866,16 +1786,20 @@ def test_prepare_message_for_openai_includes_reasoning_with_function_call() -> N message = Message(role="assistant", contents=[reasoning, function_call]) - result = client._prepare_message_for_openai(message) + # Storage-on path strips both server-issued reasoning (rs_*) and function_call items + # because the server already has them via previous_response_id (#3295). + storage_on_result = client._prepare_message_for_openai(message, request_uses_service_side_storage=True) + storage_on_types = [item["type"] for item in storage_on_result] + assert "reasoning" not in storage_on_types + assert "function_call" not in storage_on_types - # Both reasoning and function_call should be present as top-level items - types = [item["type"] for item in result] - assert "reasoning" in types, "Reasoning items must be included for reasoning models" - assert "function_call" in types - - reasoning_item = next(item for item in result if item["type"] == "reasoning") - assert reasoning_item["summary"][0]["text"] == "Let me analyze the request" - assert reasoning_item["id"] == "rs_abc123", "Reasoning id must be preserved for the API" + # Storage-off path keeps function_call inline so the server sees the call. Reasoning items + # cannot be replayed inline against a server that has no record of the prior response, so + # they remain dropped on this path as well. + storage_off_result = client._prepare_message_for_openai(message, request_uses_service_side_storage=False) + storage_off_types = [item["type"] for item in storage_off_result] + assert "function_call" in storage_off_types + assert "reasoning" not in storage_off_types def test_prepare_messages_for_openai_full_conversation_with_reasoning() -> None: @@ -1920,27 +1844,20 @@ def test_prepare_messages_for_openai_full_conversation_with_reasoning() -> None: ), ] - result = client._prepare_messages_for_openai(messages) + # Storage-off path: function_call kept inline (server has no record of it), + # function_call_output kept. Reasoning is still dropped because rs_* response-scoped IDs + # cannot be replayed against a server that has no record of the originating response. + result = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=False) types = [item.get("type") for item in result] assert "message" in types, "User/assistant messages should be present" - assert "reasoning" in types, "Reasoning items must be present" - assert "function_call" in types, "Function call items must be present" + assert "function_call" in types, "Function call items must be present without storage" assert "function_call_output" in types, "Function call output must be present" - # Verify reasoning has id - reasoning_items = [item for item in result if item.get("type") == "reasoning"] - assert reasoning_items[0]["id"] == "rs_test123" - # Verify function_call has id fc_items = [item for item in result if item.get("type") == "function_call"] assert fc_items[0]["id"] == "fc_test456" - # Verify correct ordering: reasoning before function_call - reasoning_idx = types.index("reasoning") - fc_idx = types.index("function_call") - assert reasoning_idx < fc_idx, "Reasoning must come before function_call" - def test_prepare_message_for_openai_filters_error_content() -> None: """Test that error content in messages is handled properly.""" @@ -4082,7 +3999,13 @@ async def test_prepare_options_store_false_omits_reasoning_items_for_stateless_r assert any(item.get("type") == "function_call_output" for item in options["input"]) -async def test_prepare_options_with_conversation_id_keeps_reasoning_items() -> None: +async def test_prepare_options_with_conversation_id_strips_server_issued_items() -> None: + """When the request continues via conversation_id / previous_response_id, server-issued + response items (reasoning rs_*, function_call fc_*) must not be re-sent inline. The server + already has them via the prior response and rejects duplicates with + 'Duplicate item found with id ...'. The function_result keeps its call_id so the server + pairs result-to-call. See microsoft/agent-framework#3295. (Originally added in #5250 with + the opposite expectation; field reports proved that path 400s on the wire.)""" client = OpenAIChatClient(model="test-model", api_key="test-key") messages = [ Message(role="user", contents=[Content.from_text(text="search for hotels")]), @@ -4118,13 +4041,16 @@ async def test_prepare_options_with_conversation_id_keeps_reasoning_items() -> N ChatOptions(store=False, conversation_id="resp_prev123"), # type: ignore[arg-type] ) - reasoning_items = [item for item in options["input"] if item.get("type") == "reasoning"] - assert len(reasoning_items) == 1 - assert reasoning_items[0]["id"] == "rs_test123" + types = [item.get("type") for item in options["input"]] + assert "reasoning" not in types + assert "function_call" not in types + assert "function_call_output" in types + output_item = next(item for item in options["input"] if item.get("type") == "function_call_output") + assert output_item["call_id"] == "call_1" assert options["previous_response_id"] == "resp_prev123" -async def test_prepare_options_with_conversation_id_omits_reasoning_items_for_attributed_replay() -> None: +async def test_prepare_options_with_conversation_id_strips_server_items_for_mixed_history_and_live() -> None: client = OpenAIChatClient(model="test-model", api_key="test-key") messages = [ Message(role="user", contents=[Content.from_text(text="search for hotels")]), @@ -4186,19 +4112,18 @@ async def test_prepare_options_with_conversation_id_omits_reasoning_items_for_at ChatOptions(store=False, conversation_id="resp_prev123"), # type: ignore[arg-type] ) - reasoning_items = [item for item in options["input"] if item.get("type") == "reasoning"] - assert [item["id"] for item in reasoning_items] == ["rs_live123"] - assert any( - item.get("type") == "function_call" and item.get("call_id") == "call_history" for item in options["input"] - ) - assert any(item.get("type") == "function_call" and item.get("call_id") == "call_live" for item in options["input"]) - assert any( - item.get("type") == "function_call_output" and item.get("call_id") == "call_history" - for item in options["input"] - ) - assert any( - item.get("type") == "function_call_output" and item.get("call_id") == "call_live" for item in options["input"] - ) + # Under continuation (request_uses_service_side_storage=True), the strip rule fires for + # every server-issued item type regardless of message attribution: history-attributed items + # would duplicate the prior response stored at resp_prev123, and live items would also + # eventually duplicate items stored on the response this request produces. Function results + # are kept; the server pairs them to prior function_calls via call_id (#3295). + types = [item.get("type") for item in options["input"]] + assert "reasoning" not in types + assert "function_call" not in types + output_call_ids = { + item["call_id"] for item in options["input"] if item.get("type") == "function_call_output" + } + assert output_call_ids == {"call_history", "call_live"} assert options["previous_response_id"] == "resp_prev123" @@ -4465,6 +4390,10 @@ async def test_integration_web_search() -> None: assert response.text is not None +@pytest.mark.skip( + reason="Unreliable due to OpenAI vector store indexing potential " + "race condition. See https://github.com/microsoft/agent-framework/issues/1669" +) @pytest.mark.flaky @pytest.mark.integration @skip_if_openai_integration_tests_disabled @@ -4474,29 +4403,31 @@ async def test_integration_file_search() -> None: assert isinstance(openai_responses_client, SupportsChatGetResponse) file_id, vector_store = await create_vector_store(openai_responses_client) - try: - # Use static method for file search tool - file_search_tool = OpenAIChatClient.get_file_search_tool(vector_store_ids=[vector_store.vector_store_id]) - # Test that the client will use the file search tool - response = await openai_responses_client.get_response( - messages=[ - Message( - role="user", - contents=["What is the weather today? Do a file search to find the answer."], - ) - ], - options={ - "tool_choice": "auto", - "tools": [file_search_tool], - }, - ) + # Use static method for file search tool + file_search_tool = OpenAIChatClient.get_file_search_tool(vector_store_ids=[vector_store.vector_store_id]) + # Test that the client will use the file search tool + response = await openai_responses_client.get_response( + messages=[ + Message( + role="user", + contents=["What is the weather today? Do a file search to find the answer."], + ) + ], + options={ + "tool_choice": "auto", + "tools": [file_search_tool], + }, + ) - assert "sunny" in response.text.lower() - assert "75" in response.text - finally: - await delete_vector_store(openai_responses_client, file_id, vector_store.vector_store_id) + await delete_vector_store(openai_responses_client, file_id, vector_store.vector_store_id) + assert "sunny" in response.text.lower() + assert "75" in response.text +@pytest.mark.skip( + reason="Unreliable due to OpenAI vector store indexing " + "potential race condition. See https://github.com/microsoft/agent-framework/issues/1669" +) @pytest.mark.flaky @pytest.mark.integration @skip_if_openai_integration_tests_disabled @@ -4506,37 +4437,35 @@ async def test_integration_streaming_file_search() -> None: assert isinstance(openai_responses_client, SupportsChatGetResponse) file_id, vector_store = await create_vector_store(openai_responses_client) - try: - # Use static method for file search tool - file_search_tool = OpenAIChatClient.get_file_search_tool(vector_store_ids=[vector_store.vector_store_id]) - # Test that the client will use the file search tool - response = openai_responses_client.get_response( - messages=[ - Message( - role="user", - contents=["What is the weather today? Do a file search to find the answer."], - ) - ], - stream=True, - options={ - "tool_choice": "auto", - "tools": [file_search_tool], - }, - ) + # Use static method for file search tool + file_search_tool = OpenAIChatClient.get_file_search_tool(vector_store_ids=[vector_store.vector_store_id]) + # Test that the client will use the web search tool + response = openai_responses_client.get_streaming_response( + messages=[ + Message( + role="user", + contents=["What is the weather today? Do a file search to find the answer."], + ) + ], + options={ + "tool_choice": "auto", + "tools": [file_search_tool], + }, + ) - assert response is not None - full_message: str = "" - async for chunk in response: - assert chunk is not None - assert isinstance(chunk, ChatResponseUpdate) - for content in chunk.contents: - if content.type == "text" and content.text: - full_message += content.text + assert response is not None + full_message: str = "" + async for chunk in response: + assert chunk is not None + assert isinstance(chunk, ChatResponseUpdate) + for content in chunk.contents: + if content.type == "text" and content.text: + full_message += content.text - assert "sunny" in full_message.lower() - assert "75" in full_message - finally: - await delete_vector_store(openai_responses_client, file_id, vector_store.vector_store_id) + await delete_vector_store(openai_responses_client, file_id, vector_store.vector_store_id) + + assert "sunny" in full_message.lower() + assert "75" in full_message @pytest.mark.flaky @@ -5059,7 +4988,10 @@ async def test_prepare_messages_for_openai_does_not_replay_fc_id_when_loaded_fro next_turn_input = Message(role="user", contents=[Content.from_text(text="Book the cheapest one")]) - live_result = client._prepare_messages_for_openai([*session.state[provider.source_id]["messages"], next_turn_input]) + live_result = client._prepare_messages_for_openai( + [*session.state[provider.source_id]["messages"], next_turn_input], + request_uses_service_side_storage=False, + ) live_function_call = next(item for item in live_result if item.get("type") == "function_call") assert live_function_call["id"] == "fc_provider123" @@ -5072,7 +5004,8 @@ async def test_prepare_messages_for_openai_does_not_replay_fc_id_when_loaded_fro ) # type: ignore[arg-type] loaded_result = client._prepare_messages_for_openai( - context.get_messages(sources={provider.source_id}, include_input=True) + context.get_messages(sources={provider.source_id}, include_input=True), + request_uses_service_side_storage=False, ) loaded_function_call = next(item for item in loaded_result if item.get("type") == "function_call") assert loaded_function_call["id"] == "fc_call_1" @@ -5091,7 +5024,8 @@ async def test_prepare_messages_for_openai_does_not_replay_fc_id_when_loaded_fro ) # type: ignore[arg-type] restored_result = client._prepare_messages_for_openai( - restored_context.get_messages(sources={provider.source_id}, include_input=True) + restored_context.get_messages(sources={provider.source_id}, include_input=True), + request_uses_service_side_storage=False, ) restored_function_call = next(item for item in restored_result if item.get("type") == "function_call") assert restored_function_call["id"] == "fc_call_1" @@ -5125,7 +5059,9 @@ def test_prepare_messages_for_openai_keeps_live_fc_id_separate_from_replayed_his ], ) - result = client._prepare_messages_for_openai([history_message, live_message]) + result = client._prepare_messages_for_openai( + [history_message, live_message], request_uses_service_side_storage=False + ) function_calls = [item for item in result if item.get("type") == "function_call"] assert [item["id"] for item in function_calls] == ["fc_call_1", "fc_live123"] @@ -5163,7 +5099,7 @@ def test_prepare_messages_for_openai_filters_empty_fc_id() -> None: ), ] - result = client._prepare_messages_for_openai(messages) + result = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=False) # Find the function_call items in the result fc_items = [item for item in result if item.get("type") == "function_call"] @@ -5198,7 +5134,7 @@ def test_prepare_messages_for_openai_filters_none_fc_id() -> None: ), ] - result = client._prepare_messages_for_openai(messages) + result = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=False) # Find the function_call item fc_items = [item for item in result if item.get("type") == "function_call"] @@ -5233,7 +5169,7 @@ def test_prepare_messages_for_openai_serializes_mcp_server_tool_call_as_mcp_call ), ] - result = client._prepare_messages_for_openai(messages) + result = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=False) mcp_items = [item for item in result if isinstance(item, dict) and item.get("type") == "mcp_call"] assert len(mcp_items) == 1, f"expected exactly one mcp_call item; got result={result}" @@ -5276,7 +5212,7 @@ def test_prepare_messages_for_openai_coalesces_mcp_call_and_result_into_single_i ), ] - result = client._prepare_messages_for_openai(messages) + result = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=False) mcp_items = [item for item in result if isinstance(item, dict) and item.get("type") == "mcp_call"] assert len(mcp_items) == 1, f"expected one coalesced mcp_call item carrying both arguments and output; got {result}" @@ -5310,7 +5246,7 @@ def test_prepare_messages_for_openai_drops_orphan_mcp_server_tool_result() -> No ), ] - result = client._prepare_messages_for_openai(messages) + result = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=False) fco_items = [item for item in result if isinstance(item, dict) and item.get("type") == "function_call_output"] assert fco_items == [], f"orphan mcp_server_tool_result must not serialize as function_call_output; got {fco_items}" @@ -5342,4 +5278,170 @@ def test_stringify_mcp_output_falls_back_to_json_for_non_text_dict_entries() -> # endregion +# region: strip server-issued item IDs under storage (issue #3295) + + +def _strip_rule_messages() -> list[Message]: + return [ + Message(role="user", contents=[Content.from_text(text="search hotels in Paris")]), + Message( + role="assistant", + contents=[ + Content.from_function_call( + call_id="call_1", + name="search_hotels", + arguments='{"city": "Paris"}', + additional_properties={"fc_id": "fc_server_issued"}, + ), + ], + ), + Message( + role="tool", + contents=[Content.from_function_result(call_id="call_1", result="Found 3 hotels in Paris")], + ), + ] + + +def test_prepare_messages_strips_function_call_under_storage() -> None: + """Regression for #3295: when previous_response_id / conversation_id is in flight, the chat + client must not re-send server-issued function_call items inline. The server already has them + via the prior response and rejects duplicates with 'Duplicate item found with id fc_...'. + The function_result keeps its call_id so the server can pair result-to-call.""" + client = OpenAIChatClient(model="test-model", api_key="test-key") + + result = client._prepare_messages_for_openai(_strip_rule_messages(), request_uses_service_side_storage=True) + + types = [item.get("type") for item in result] + assert "function_call" not in types + assert "function_call_output" in types + output_item = next(item for item in result if item.get("type") == "function_call_output") + assert output_item["call_id"] == "call_1" + + +def test_prepare_messages_keeps_function_call_without_storage() -> None: + """Without storage there is no previous_response_id, so inline function_call items are the + only source of truth for the server. Behavior is byte-identical to pre-#3295.""" + client = OpenAIChatClient(model="test-model", api_key="test-key") + + result = client._prepare_messages_for_openai(_strip_rule_messages(), request_uses_service_side_storage=False) + + types = [item.get("type") for item in result] + assert "function_call" in types + assert "function_call_output" in types + fc_item = next(item for item in result if item.get("type") == "function_call") + assert fc_item["call_id"] == "call_1" + assert fc_item["id"] == "fc_server_issued" + output_item = next(item for item in result if item.get("type") == "function_call_output") + assert output_item["call_id"] == "call_1" + + +def test_prepare_messages_strips_approval_items_under_storage() -> None: + """Approval request/response items also carry server-issued IDs and must be stripped under + storage. Without storage they are kept (#3295).""" + client = OpenAIChatClient(model="test-model", api_key="test-key") + + function_call = Content.from_function_call( + call_id="mcp_1", + name="sensitive_action", + arguments='{"action": "delete"}', + ) + approval_request = Content.from_function_approval_request( + id="approval_req_1", + function_call=function_call, + ) + approval_response = Content.from_function_approval_response( + approved=True, + id="approval_req_1", + function_call=function_call, + ) + messages = [ + Message(role="assistant", contents=[approval_request]), + Message(role="user", contents=[approval_response]), + ] + + storage_on = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=True) + storage_on_types = [item.get("type") for item in storage_on] + assert "mcp_approval_request" not in storage_on_types + assert "mcp_approval_response" not in storage_on_types + + storage_off = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=False) + storage_off_types = [item.get("type") for item in storage_off] + assert "mcp_approval_request" in storage_off_types + assert "mcp_approval_response" in storage_off_types + + +def test_prepare_messages_strips_local_shell_call_under_storage() -> None: + """Local-shell-call function_results carry a server-issued local_shell_call_item_id and must + be stripped under storage. Plain function_results (no shell ID) are kept either way (#3295).""" + from agent_framework_openai._chat_client import ( + OPENAI_LOCAL_SHELL_CALL_ITEM_ID_KEY, + OPENAI_SHELL_OUTPUT_TYPE_KEY, + OPENAI_SHELL_OUTPUT_TYPE_LOCAL_SHELL_CALL, + ) + + client = OpenAIChatClient(model="test-model", api_key="test-key") + shell_result = Content.from_function_result( + call_id="shell_1", + result="ok", + additional_properties={ + OPENAI_SHELL_OUTPUT_TYPE_KEY: OPENAI_SHELL_OUTPUT_TYPE_LOCAL_SHELL_CALL, + OPENAI_LOCAL_SHELL_CALL_ITEM_ID_KEY: "lsh_server_issued", + }, + ) + plain_result = Content.from_function_result(call_id="plain_1", result="plain") + message = Message(role="tool", contents=[shell_result, plain_result]) + + storage_on = client._prepare_message_for_openai(message, request_uses_service_side_storage=True) + types_on = [item.get("type") for item in storage_on] + assert OPENAI_SHELL_OUTPUT_TYPE_LOCAL_SHELL_CALL not in types_on + assert "function_call_output" in types_on + + storage_off = client._prepare_message_for_openai(message, request_uses_service_side_storage=False) + types_off = [item.get("type") for item in storage_off] + assert OPENAI_SHELL_OUTPUT_TYPE_LOCAL_SHELL_CALL in types_off + assert "function_call_output" in types_off + + +def test_prepare_messages_strips_mcp_items_under_storage() -> None: + """Hosted-MCP tool call items carry server-issued IDs (the call_id surfaces as `id` on the + wire mcp_call item), so they must be stripped under storage. The orphan mcp_server_tool_result + is then dropped by the existing coalesce logic (#5581). Without storage, the call/result pair + coalesces normally into a single mcp_call wire item (#3295).""" + client = OpenAIChatClient(model="test-model", api_key="test-key") + + messages = [ + Message( + role="assistant", + contents=[ + Content.from_mcp_server_tool_call( + call_id="mcp_abc123", + tool_name="search", + server_name="api_specs", + arguments='{"q": "cats"}', + ) + ], + ), + Message( + role="tool", + contents=[ + Content.from_mcp_server_tool_result( + call_id="mcp_abc123", + output=[Content.from_text(text="found 10 cats")], + ) + ], + ), + ] + + storage_on = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=True) + storage_on_types = [item.get("type") for item in storage_on] + assert "mcp_call" not in storage_on_types + + storage_off = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=False) + storage_off_types = [item.get("type") for item in storage_off] + assert "mcp_call" in storage_off_types + + +# endregion + + # endregion From 741259476f869a288ec157568a569e4c36a15946 Mon Sep 17 00:00:00 2001 From: Ben Thomas Date: Wed, 13 May 2026 15:48:49 -0700 Subject: [PATCH 3/8] Fix CA1873 in DevUI by using LoggerMessage source generator (#5831) Replaces two ILogger.LogWarning(string, params object?[]) calls in DevUIAuthFilter and DevUIExtensions with allocation-free [LoggerMessage] partial methods on a new internal DevUILog class. Preserves original message templates and structured property names ({RemoteIp}, {EnvVar}). Co-authored-by: alliscode <25218250+alliscode@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../DevUIAuthFilter.cs | 4 +--- .../DevUIExtensions.cs | 5 +---- .../src/Microsoft.Agents.AI.DevUI/DevUILog.cs | 20 +++++++++++++++++++ 3 files changed, 22 insertions(+), 7 deletions(-) create mode 100644 dotnet/src/Microsoft.Agents.AI.DevUI/DevUILog.cs diff --git a/dotnet/src/Microsoft.Agents.AI.DevUI/DevUIAuthFilter.cs b/dotnet/src/Microsoft.Agents.AI.DevUI/DevUIAuthFilter.cs index b8e4b499f8..7f238ab3a0 100644 --- a/dotnet/src/Microsoft.Agents.AI.DevUI/DevUIAuthFilter.cs +++ b/dotnet/src/Microsoft.Agents.AI.DevUI/DevUIAuthFilter.cs @@ -51,9 +51,7 @@ internal sealed class DevUIAuthFilter : IEndpointFilter if (!isLoopback && !this._options.AllowRemoteAccess) { - this._logger.LogWarning( - "Rejected non-loopback DevUI request from {RemoteIp}. Set DevUIOptions.AllowRemoteAccess to permit remote callers.", - remoteIp); + DevUILog.RejectedNonLoopbackRequest(this._logger, remoteIp); return Results.Problem( statusCode: StatusCodes.Status403Forbidden, title: "DevUI access denied", diff --git a/dotnet/src/Microsoft.Agents.AI.DevUI/DevUIExtensions.cs b/dotnet/src/Microsoft.Agents.AI.DevUI/DevUIExtensions.cs index 18d0ae24cf..d22cd46f61 100644 --- a/dotnet/src/Microsoft.Agents.AI.DevUI/DevUIExtensions.cs +++ b/dotnet/src/Microsoft.Agents.AI.DevUI/DevUIExtensions.cs @@ -100,10 +100,7 @@ public static class DevUIExtensions if (options.AllowRemoteAccess && !tokenConfigured && options.ConfigureEndpoints is null) { - logger.LogWarning( - "DevUI is configured with AllowRemoteAccess=true and no authentication. " + - "Set DevUIOptions.AuthToken, the {EnvVar} environment variable, or attach an authorization policy via ConfigureEndpoints.", - DevUIOptions.AuthTokenEnvironmentVariable); + DevUILog.InsecurelyExposed(logger, DevUIOptions.AuthTokenEnvironmentVariable); } } } diff --git a/dotnet/src/Microsoft.Agents.AI.DevUI/DevUILog.cs b/dotnet/src/Microsoft.Agents.AI.DevUI/DevUILog.cs new file mode 100644 index 0000000000..a963b42a6f --- /dev/null +++ b/dotnet/src/Microsoft.Agents.AI.DevUI/DevUILog.cs @@ -0,0 +1,20 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System.Net; + +namespace Microsoft.Agents.AI.DevUI; + +internal static partial class DevUILog +{ + [LoggerMessage( + EventId = 1, + Level = LogLevel.Warning, + Message = "Rejected non-loopback DevUI request from {RemoteIp}. Set DevUIOptions.AllowRemoteAccess to permit remote callers.")] + public static partial void RejectedNonLoopbackRequest(ILogger logger, IPAddress? remoteIp); + + [LoggerMessage( + EventId = 2, + Level = LogLevel.Warning, + Message = "DevUI is configured with AllowRemoteAccess=true and no authentication. Set DevUIOptions.AuthToken, the {EnvVar} environment variable, or attach an authorization policy via ConfigureEndpoints.")] + public static partial void InsecurelyExposed(ILogger logger, string envVar); +} From fbccad091bb087e782d157a9ccab5e233f15a168 Mon Sep 17 00:00:00 2001 From: Evan Mattson <35585003+moonbox3@users.noreply.github.com> Date: Thu, 14 May 2026 09:37:46 +0900 Subject: [PATCH 4/8] [BREAKING] Python: DevUI: tighten default access controls and CORS posture (#5740) * Python: DevUI: tighten default access controls and CORS posture Adjusts the default configuration of the DevUI server so the out-of-the-box posture matches what most callers expect when running locally. Adds explicit opt-outs for callers who need the previous behavior. - DevServer gains auth_enabled and auth_token constructor params; auth is on by default. Auto-generates and logs a token when none provided. - CORS default is an empty allowlist on every host. Callers wanting cross-origin pass cors_origins explicitly. - Streaming /v1/responses no longer sets Access-Control-Allow-Origin directly; CORSMiddleware owns all CORS decisions. - Loopback binds enforce a Host-header allowlist. - /meta moved out of the auth bypass list (was alongside /health and /). - serve() default flipped to auth_enabled=True; passes auth args through to DevServer instead of using env-var indirection. - CLI: --auth opt-in replaced with --no-auth opt-out; --auth-token preserved. - Tests cover the eight behaviors above in test_server.py. * Python: DevUI: address PR review comments - /meta now derives auth_required from self.auth_enabled instead of reading DEVUI_AUTH_TOKEN, so the auto-generated and explicit auth_token paths report correctly. - Reorder middleware so the loopback Host-header allowlist is registered last; Starlette wraps later-added middleware around earlier-added ones, so the host check now runs outermost (before CORS/auth) as intended. - Rework comments to describe the behavior rather than threat scenarios. - Streaming-headers and CORS tests now construct the server with an explicit auth_token and send a Bearer header, so the assertions actually exercise the streaming/CORS path instead of short-circuiting in the auth middleware. --- .../devui/agent_framework_devui/__init__.py | 55 ++--- .../devui/agent_framework_devui/_cli.py | 8 +- .../devui/agent_framework_devui/_server.py | 109 +++++++--- .../packages/devui/tests/devui/test_server.py | 189 +++++++++++++++++- .../tests/devui/test_ui_memory_regression.py | 2 +- 5 files changed, 290 insertions(+), 73 deletions(-) diff --git a/python/packages/devui/agent_framework_devui/__init__.py b/python/packages/devui/agent_framework_devui/__init__.py index 6af274743a..b647c60fed 100644 --- a/python/packages/devui/agent_framework_devui/__init__.py +++ b/python/packages/devui/agent_framework_devui/__init__.py @@ -96,7 +96,7 @@ def serve( ui_enabled: bool = True, instrumentation_enabled: bool = False, mode: str = "developer", - auth_enabled: bool = False, + auth_enabled: bool = True, auth_token: str | None = None, ) -> None: """Launch Agent Framework DevUI with simple API. @@ -126,52 +126,29 @@ def serve( if not isinstance(port, int) or not (1 <= port <= 65535): raise ValueError(f"Invalid port: {port}. Must be integer between 1 and 65535") - # Security check: Warn if network-exposed without authentication + # Security check: warn loudly when network-exposed without authentication. if host not in ("127.0.0.1", "localhost") and not auth_enabled: - logger.warning("⚠️ WARNING: Exposing DevUI to network without authentication!") - logger.warning("⚠️ This is INSECURE - anyone on your network can access your agents") - logger.warning("💡 For network exposure, add --auth flag: devui --host 0.0.0.0 --auth") + logger.warning("WARNING: Exposing DevUI to the network with --no-auth.") + logger.warning("Anyone on your network can read agent metadata and trigger requests.") + logger.warning("Drop --no-auth and DevUI will require Bearer tokens.") - # Handle authentication configuration - if auth_enabled: + # Refuse to auto-generate a token for network-exposed binds. Auto-generated tokens + # are fine for localhost convenience; for anything else, require an explicit token. + if auth_enabled and not auth_token: import os - import secrets - # Check if token is in environment variable first - if not auth_token: - auth_token = os.environ.get("DEVUI_AUTH_TOKEN") - - # Auto-generate token if STILL not provided - if not auth_token: - # Check if we're in a production-like environment + env_token = os.environ.get("DEVUI_AUTH_TOKEN") + if not env_token: is_production = ( - host not in ("127.0.0.1", "localhost") # Exposed to network - or os.environ.get("CI") == "true" # Running in CI - or os.environ.get("KUBERNETES_SERVICE_HOST") # Running in k8s + host not in ("127.0.0.1", "localhost") + or os.environ.get("CI") == "true" + or os.environ.get("KUBERNETES_SERVICE_HOST") ) - if is_production: - # REFUSE to start without explicit token - logger.error("❌ Authentication enabled but no token provided") - logger.error("❌ Auto-generated tokens are NOT secure for network-exposed deployments") - logger.error("💡 Set token: export DEVUI_AUTH_TOKEN=") - logger.error("💡 Or pass: serve(entities=[...], auth_token='your-token')") + logger.error("Authentication required but no token provided.") + logger.error("Set DEVUI_AUTH_TOKEN env var or pass auth_token='...' to serve().") raise ValueError("DEVUI_AUTH_TOKEN required when host is not localhost") - # Development mode: auto-generate and show - auth_token = secrets.token_urlsafe(32) - logger.info("🔒 Authentication enabled with auto-generated token") - logger.info("\n" + "=" * 70) - logger.info("🔑 DEV TOKEN (localhost only, shown once):") - logger.info(f" {auth_token}") - logger.info("=" * 70 + "\n") - else: - logger.info("🔒 Authentication enabled with provided token") - - # Set environment variable for server to use - os.environ["AUTH_REQUIRED"] = "true" - os.environ["DEVUI_AUTH_TOKEN"] = auth_token - # Enable instrumentation if requested if instrumentation_enabled: from agent_framework.observability import enable_instrumentation @@ -187,6 +164,8 @@ def serve( cors_origins=cors_origins, ui_enabled=ui_enabled, mode=mode, + auth_enabled=auth_enabled, + auth_token=auth_token, ) # Register in-memory entities if provided diff --git a/python/packages/devui/agent_framework_devui/_cli.py b/python/packages/devui/agent_framework_devui/_cli.py index 261cfe4331..e5e64b6fd4 100644 --- a/python/packages/devui/agent_framework_devui/_cli.py +++ b/python/packages/devui/agent_framework_devui/_cli.py @@ -79,15 +79,15 @@ Examples: ) parser.add_argument( - "--auth", + "--no-auth", action="store_true", - help="Enable authentication via Bearer token (required for deployed environments)", + help="Disable Bearer token authentication. DevUI is auth-enabled by default; use this to opt out.", ) parser.add_argument( "--auth-token", type=str, - help="Custom authentication token (auto-generated if not provided with --auth)", + help="Custom Bearer token. Auto-generated and logged at startup when omitted.", ) parser.add_argument("--version", action="version", version=f"Agent Framework DevUI {get_version()}") @@ -184,7 +184,7 @@ def main() -> None: ui_enabled=ui_enabled, instrumentation_enabled=args.instrumentation, mode=mode, - auth_enabled=args.auth, + auth_enabled=not args.no_auth, auth_token=args.auth_token, # Pass through explicit token only ) diff --git a/python/packages/devui/agent_framework_devui/_server.py b/python/packages/devui/agent_framework_devui/_server.py index ff26937843..416821f40e 100644 --- a/python/packages/devui/agent_framework_devui/_server.py +++ b/python/packages/devui/agent_framework_devui/_server.py @@ -75,6 +75,8 @@ class DevServer: cors_origins: list[str] | None = None, ui_enabled: bool = True, mode: str = "developer", + auth_enabled: bool = True, + auth_token: str | None = None, ) -> None: """Initialize the development server. @@ -85,20 +87,26 @@ class DevServer: cors_origins: List of allowed CORS origins ui_enabled: Whether to enable the UI mode: Server mode - 'developer' (full access, verbose errors) or 'user' (restricted APIs, generic errors) + auth_enabled: Whether to require Bearer token auth on /v1/* endpoints. Defaults to True. + auth_token: Bearer token. If None and auth_enabled, falls back to the DEVUI_AUTH_TOKEN + environment variable, then to an auto-generated token (logged at startup). """ self.entities_dir = entities_dir self.port = port self.host = host - # Smart CORS defaults: permissive for localhost, restrictive for network-exposed deployments + # CORS default is same-origin only (empty allowlist) on every host. The + # previous wildcard-on-localhost default let any webpage the developer + # visited read DevUI's responses cross-origin. Callers who need a real + # cross-origin dev frontend pass an explicit allowlist. if cors_origins is None: - # Localhost development: allow cross-origin for dev tools (e.g., frontend dev server) - # Network-exposed: empty list (same-origin only, no CORS) - cors_origins = ["*"] if host in ("127.0.0.1", "localhost") else [] + cors_origins = [] self.cors_origins = cors_origins self.ui_enabled = ui_enabled self.mode = mode + self.auth_enabled = auth_enabled + self.auth_token = self._resolve_auth_token(auth_enabled, auth_token) self.executor: AgentFrameworkExecutor | None = None self.openai_executor: OpenAIExecutor | None = None self.deployment_manager = DeploymentManager() @@ -110,6 +118,37 @@ class DevServer: """Set in-memory entities to register on startup.""" self._pending_entities = entities + _LOOPBACK_HOSTS = frozenset({"127.0.0.1", "localhost", "[::1]", "::1"}) + + def _loopback_allowed_hosts(self) -> frozenset[str] | None: + """Return the Host-header allowlist when bound to a loopback interface, else None. + + Returning None disables Host-header enforcement (e.g. for 0.0.0.0 / public binds, + where the operator is intentionally exposing the service). + """ + host = self.host.lower() + if host not in self._LOOPBACK_HOSTS: + return None + return self._LOOPBACK_HOSTS + + @staticmethod + def _resolve_auth_token(auth_enabled: bool, auth_token: str | None) -> str | None: + """Resolve the active Bearer token. Returns None when auth is disabled.""" + if not auth_enabled: + return None + if auth_token: + return auth_token + env_token = os.getenv("DEVUI_AUTH_TOKEN") + if env_token: + return env_token + generated = secrets.token_urlsafe(32) + logger.info("=" * 70) + logger.info("DevUI authentication enabled with auto-generated token:") + logger.info(f" {generated}") + logger.info("Pass it as: Authorization: Bearer ") + logger.info("=" * 70) + return generated + def _is_dev_mode(self) -> bool: """Check if running in developer mode. @@ -336,6 +375,11 @@ class DevServer: lifespan=lifespan, ) + # Middleware registration order matters: Starlette wraps later-added + # middleware around earlier-added ones, so the LAST registered runs + # outermost (sees the request first). We want Host-header enforcement + # to run before CORS/auth, so it is registered last below. + # Add CORS middleware # Note: allow_credentials cannot be True when allow_origins is ["*"] # For localhost dev with wildcard origins, credentials are disabled @@ -350,29 +394,24 @@ class DevServer: allow_headers=["*"], ) - # Add authentication middleware using decorator pattern - # Auth is enabled by presence of DEVUI_AUTH_TOKEN - auth_token = os.getenv("DEVUI_AUTH_TOKEN", "") - auth_required = bool(auth_token) - - if auth_required: + # Bearer-token authentication. Enabled by default; opt out via + # DevServer(auth_enabled=False) for embedded/test scenarios. + if self.auth_enabled and self.auth_token: + expected_token = self.auth_token logger.info("Authentication middleware enabled") @app.middleware("http") async def auth_middleware(request: Request, call_next: Callable[[Request], Awaitable[Any]]) -> Any: """Validate Bearer token authentication. - Skips authentication for health, meta, static UI endpoints, and OPTIONS requests. + Skips authentication for health, the UI shell, static assets, and OPTIONS preflight. """ - # Skip auth for OPTIONS (CORS preflight) requests if request.method == "OPTIONS": return await call_next(request) - # Skip auth for health checks, meta endpoint, and static files - if request.url.path in ["/health", "/meta", "/"] or request.url.path.startswith("/assets"): + if request.url.path in ["/health", "/"] or request.url.path.startswith("/assets"): return await call_next(request) - # Check Authorization header auth_header = request.headers.get("Authorization") if not auth_header or not auth_header.startswith("Bearer "): return JSONResponse( @@ -388,9 +427,8 @@ class DevServer: }, ) - # Extract and validate token token = auth_header.replace("Bearer ", "", 1).strip() - if not secrets.compare_digest(token, auth_token): + if not secrets.compare_digest(token, expected_token): return JSONResponse( status_code=401, content={ @@ -402,11 +440,40 @@ class DevServer: }, ) - # Token valid, proceed return await call_next(request) _ = auth_middleware + # Host-header allowlist for loopback binds: on a loopback interface, only + # accept requests whose Host header names a loopback address. Registered LAST + # so it runs outermost, rejecting non-loopback Host values before CORS/auth + # (and before CORS can short-circuit a preflight on a rebound request). + allowed_hosts = self._loopback_allowed_hosts() + if allowed_hosts is not None: + expected_hosts = allowed_hosts + + @app.middleware("http") + async def host_header_middleware(request: Request, call_next: Callable[[Request], Awaitable[Any]]) -> Any: + host_header = request.headers.get("host", "") + hostname = host_header.split(":", 1)[0].lower() + if hostname and hostname not in expected_hosts: + return JSONResponse( + status_code=400, + content={ + "error": { + "message": ( + f"Invalid Host header '{host_header}'. DevUI is bound to a " + "loopback interface and only accepts requests addressed to it." + ), + "type": "invalid_host", + "code": "host_not_allowed", + } + }, + ) + return await call_next(request) + + _ = host_header_middleware + self._register_routes(app) self._mount_ui(app) @@ -427,8 +494,6 @@ class DevServer: @app.get("/meta", response_model=MetaResponse) async def get_meta() -> MetaResponse: """Get server metadata and configuration.""" - import os - # Ensure executors are initialized to check capabilities openai_executor = await self._ensure_openai_executor() @@ -442,7 +507,7 @@ class DevServer: "openai_proxy": openai_executor.is_configured, "deployment": True, # Deployment feature is available }, - auth_required=bool(os.getenv("DEVUI_AUTH_TOKEN")), + auth_required=self.auth_enabled, ) @app.get("/v1/entities", response_model=DiscoveryResponse) @@ -750,7 +815,6 @@ class DevServer: headers={ "Cache-Control": "no-cache", "Connection": "keep-alive", - "Access-Control-Allow-Origin": "*", }, ) return await openai_executor.execute_sync(request) @@ -794,7 +858,6 @@ class DevServer: headers={ "Cache-Control": "no-cache", "Connection": "keep-alive", - "Access-Control-Allow-Origin": "*", "X-Response-ID": response_id, # Include ID for debugging/tracking }, ) diff --git a/python/packages/devui/tests/devui/test_server.py b/python/packages/devui/tests/devui/test_server.py index 3f1945be3a..bcb21f4eee 100644 --- a/python/packages/devui/tests/devui/test_server.py +++ b/python/packages/devui/tests/devui/test_server.py @@ -3,11 +3,15 @@ """Focused tests for server functionality.""" import asyncio +import inspect import tempfile from pathlib import Path import pytest +from conftest import MockAgent +from fastapi.testclient import TestClient +import agent_framework_devui from agent_framework_devui import DevServer from agent_framework_devui._utils import extract_executor_message_types, select_primary_input_type from agent_framework_devui.models._openai_custom import AgentFrameworkRequest @@ -99,11 +103,11 @@ async def test_server_execution_streaming(test_entities_dir): def test_configuration(): """Test basic configuration.""" - server = DevServer(entities_dir="test", port=9000, host="localhost") + server = DevServer(entities_dir="test", port=9000, host="localhost", auth_enabled=False) assert server.port == 9000 assert server.host == "localhost" assert server.entities_dir == "test" - assert server.cors_origins == ["*"] + assert server.cors_origins == [] assert server.ui_enabled @@ -252,15 +256,18 @@ async def test_api_restrictions_in_user_mode(): """Test that developer APIs are restricted in user mode.""" from fastapi.testclient import TestClient - # Create servers with different modes - dev_server = DevServer(mode="developer") - user_server = DevServer(mode="user") + # Create servers with different modes. auth_enabled=False isolates this test + # to mode behavior — auth has its own dedicated suite. + dev_server = DevServer(mode="developer", auth_enabled=False) + user_server = DevServer(mode="user", auth_enabled=False) dev_app = dev_server.create_app() user_app = user_server.create_app() - dev_client = TestClient(dev_app) - user_client = TestClient(user_app) + # base_url sets the Host header to a loopback alias so the loopback + # host-header allowlist accepts the request. + dev_client = TestClient(dev_app, base_url="http://127.0.0.1") + user_client = TestClient(user_app, base_url="http://127.0.0.1") # Test 1: Health endpoint should work in both modes assert dev_client.get("/health").status_code == 200 @@ -403,3 +410,171 @@ async def test_checkpoint_api_endpoints(test_entities_dir): # Test delete non-existent checkpoint deleted = await storage.delete("nonexistent") assert deleted is False + + +# ============================================================================= +# Security posture: default CORS, auth, host-header, and streaming headers. +# ============================================================================= + + +def _server_with_mock_agent(**kwargs) -> DevServer: + """Build a DevServer with one in-memory mock agent registered.""" + server = DevServer(**kwargs) + server.set_pending_entities([MockAgent(id="mock", name="Mock", response_text="hi")]) + return server + + +def test_streaming_response_does_not_hardcode_acao_header(): + """A streaming /v1/responses must not set Access-Control-Allow-Origin itself. + + The endpoint previously hardcoded `Access-Control-Allow-Origin: *` on the + StreamingResponse, bypassing CORSMiddleware. With no Origin header on the + request, CORSMiddleware never adds ACAO — so any ACAO we see proves the + streaming handler is still setting it. + """ + server = _server_with_mock_agent(auth_token="s3cret") + app = server.get_app() + + with TestClient(app, base_url="http://127.0.0.1") as client: + response = client.post( + "/v1/responses", + json={"metadata": {"entity_id": "mock"}, "input": "hello", "stream": True}, + headers={"Authorization": "Bearer s3cret"}, + ) + + assert "access-control-allow-origin" not in {k.lower() for k in response.headers}, ( + "Streaming response sets ACAO directly, bypassing CORSMiddleware" + ) + + +def test_cors_default_does_not_allow_arbitrary_origin_even_on_localhost(): + """Default CORS must not echo Access-Control-Allow-Origin to arbitrary origins. + + Previous default was `["*"]` on localhost binds, which let any webpage the + developer visited read DevUI's responses. Default is now `[]` — opt in by + passing `cors_origins=[...]` explicitly. + """ + server = _server_with_mock_agent(host="127.0.0.1", auth_token="s3cret") + app = server.get_app() + + with TestClient(app, base_url="http://127.0.0.1") as client: + preflight = client.options( + "/v1/entities", + headers={ + "Origin": "https://evil.example", + "Access-Control-Request-Method": "GET", + }, + ) + assert preflight.headers.get("access-control-allow-origin") not in ("*", "https://evil.example") + + actual = client.get( + "/v1/entities", + headers={"Origin": "https://evil.example", "Authorization": "Bearer s3cret"}, + ) + assert actual.headers.get("access-control-allow-origin") not in ("*", "https://evil.example") + + +def test_devserver_requires_auth_by_default(monkeypatch): + """A bare DevServer() must reject unauthenticated /v1/* requests. + + Previously auth was opt-in via DEVUI_AUTH_TOKEN env var; the new default is + auth-on so a bare `devui ./agents` invocation does not expose an open API. + """ + monkeypatch.delenv("DEVUI_AUTH_TOKEN", raising=False) + + server = DevServer() + app = server.get_app() + + with TestClient(app, base_url="http://127.0.0.1") as client: + response = client.get("/v1/entities") + + assert response.status_code == 401 + + +def test_devserver_auth_can_be_explicitly_disabled(monkeypatch): + """Callers can opt out of auth with auth_enabled=False (escape hatch for tests / trusted hosts).""" + monkeypatch.delenv("DEVUI_AUTH_TOKEN", raising=False) + + server = _server_with_mock_agent(auth_enabled=False) + app = server.get_app() + + with TestClient(app, base_url="http://127.0.0.1") as client: + response = client.get("/v1/entities") + + assert response.status_code == 200 + + +def test_devserver_accepts_request_with_valid_bearer_token(monkeypatch): + """When auth is on, supplying the configured Bearer token grants access.""" + monkeypatch.delenv("DEVUI_AUTH_TOKEN", raising=False) + + server = DevServer(auth_token="s3cret") + app = server.get_app() + + with TestClient(app, base_url="http://127.0.0.1") as client: + response = client.get("/v1/entities", headers={"Authorization": "Bearer s3cret"}) + + assert response.status_code == 200 + + +def test_meta_endpoint_requires_auth(monkeypatch): + """/meta exposes capability flags (deployment, instrumentation, version) — gate it behind auth. + + Previously /meta was in the auth-bypass list alongside /health and /, so any + unauthenticated caller could read the deployment's capability flags. + """ + monkeypatch.delenv("DEVUI_AUTH_TOKEN", raising=False) + + server = DevServer(auth_token="s3cret") + app = server.get_app() + + with TestClient(app, base_url="http://127.0.0.1") as client: + unauth = client.get("/meta") + assert unauth.status_code == 401 + + ok = client.get("/meta", headers={"Authorization": "Bearer s3cret"}) + assert ok.status_code == 200 + + +def test_loopback_bind_rejects_non_allowlisted_host_header(monkeypatch): + """A loopback-bound server must reject requests with a non-loopback Host header. + + On a loopback bind, only Host values that name a loopback address are valid; + anything else (e.g. an external hostname that happens to resolve to 127.0.0.1) + is rejected before any handler runs. + """ + monkeypatch.delenv("DEVUI_AUTH_TOKEN", raising=False) + + server = DevServer(host="127.0.0.1", auth_enabled=False) + app = server.get_app() + + with TestClient(app, base_url="http://127.0.0.1") as client: + rebound = client.get("/health", headers={"Host": "evil.example"}) + assert rebound.status_code == 400 + + ok = client.get("/health", headers={"Host": "127.0.0.1"}) + assert ok.status_code == 200 + + ok_localhost = client.get("/health", headers={"Host": "localhost:8080"}) + assert ok_localhost.status_code == 200 + + +def test_serve_defaults_to_auth_enabled(): + """`serve()`'s public signature must default to auth_enabled=True.""" + sig = inspect.signature(agent_framework_devui.serve) + assert sig.parameters["auth_enabled"].default is True, ( + "serve() must default to auth_enabled=True so `devui ./agents` is secure out of the box" + ) + + +def test_cli_enables_auth_by_default_and_supports_no_auth_optout(): + """`devui ./agents` must produce auth-enabled config; `--no-auth` is the explicit escape hatch.""" + from agent_framework_devui._cli import create_cli_parser + + parser = create_cli_parser() + + default_args = parser.parse_args([]) + assert default_args.no_auth is False, "Default CLI invocation should leave auth on" + + optout_args = parser.parse_args(["--no-auth"]) + assert optout_args.no_auth is True diff --git a/python/packages/devui/tests/devui/test_ui_memory_regression.py b/python/packages/devui/tests/devui/test_ui_memory_regression.py index b042764f6c..cc3ec9056d 100644 --- a/python/packages/devui/tests/devui/test_ui_memory_regression.py +++ b/python/packages/devui/tests/devui/test_ui_memory_regression.py @@ -582,7 +582,7 @@ def test_sample_peak_renderer_rss_mb_uses_browser_process_tree( def memory_regression_server() -> Generator[tuple[str, str]]: """Start DevUI with a synthetic streaming agent and yield the base URL plus entity ID.""" - server = DevServer(host="127.0.0.1", port=0) + server = DevServer(host="127.0.0.1", port=0, auth_enabled=False) server.register_entities([ MemoryStressAgent( id="memory-stream-agent", From d40670748d251d1c20c848a5c16417c240b8d5fe Mon Sep 17 00:00:00 2001 From: SergeyMenshykh <68852919+SergeyMenshykh@users.noreply.github.com> Date: Thu, 14 May 2026 11:28:22 +0100 Subject: [PATCH 5/8] [BREAKING] Python: Align file skill folder discovery with agentskills.io spec (#5807) * Align Python FileSkillsSource with agentskills.io spec Update FileSkillsSource to scan spec-defined subdirectories instead of recursive rglob for resource and script discovery: - Resources: scan 'references/' and 'assets/' (was: entire skill tree) - Scripts: scan 'scripts/' (was: entire skill tree) - Add resource_directories and script_directories parameters for customization, with '.' root indicator for skill root files - Add directory validation: reject '..' traversal, absolute paths, empty names; normalize separators and deduplicate directories - Non-recursive scanning within each configured directory (top-level only) - Containment check validates files against target directory, not just skill root, for stronger path-traversal defense - Case-insensitive directory deduplication via os.path.normcase() - Cross-platform absolute path rejection in directory validation - Sort discovery results for stable ordering - Update SkillsProvider.from_paths() to pass new parameters through - Update all tests for new subdirectory-scoped discovery behavior Resolves #5711. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address PR review: tighten path validation and add containment guard - Narrow Windows absolute path check to proper drive-root pattern (re.match r'^[A-Za-z]:[/\\]') to avoid rejecting valid POSIX names - Add _is_path_within_directory guard before _has_symlink_in_path in both discovery methods to prevent ValueError on escaped paths Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Log warning on OSError during directory listing in skill discovery Address review comment: _discover_resource_files and _discover_script_files previously swallowed OSError silently when iterdir() failed. Now log a warning so permission errors and transient FS failures are visible instead of making resource/script directories silently disappear. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../packages/core/agent_framework/_skills.py | 322 +++++++++-- .../packages/core/tests/core/test_skills.py | 523 ++++++++++++++---- 2 files changed, 685 insertions(+), 160 deletions(-) diff --git a/python/packages/core/agent_framework/_skills.py b/python/packages/core/agent_framework/_skills.py index 1128a938ea..91b583aaab 100644 --- a/python/packages/core/agent_framework/_skills.py +++ b/python/packages/core/agent_framework/_skills.py @@ -1430,6 +1430,13 @@ DEFAULT_RESOURCE_EXTENSIONS: Final[tuple[str, ...]] = ( ) DEFAULT_SCRIPT_EXTENSIONS: Final[tuple[str, ...]] = (".py",) +# "." means the skill directory root itself (files directly in the skill folder). +ROOT_DIRECTORY_INDICATOR: Final[str] = "." + +# Standard subdirectory names per https://agentskills.io/specification#directory-structure +DEFAULT_RESOURCE_DIRECTORIES: Final[tuple[str, ...]] = ("references", "assets") +DEFAULT_SCRIPT_DIRECTORIES: Final[tuple[str, ...]] = ("scripts",) + # region Patterns and prompt template # Matches YAML frontmatter delimited by "---" lines. @@ -1650,6 +1657,8 @@ class SkillsProvider(ContextProvider): script_runner: SkillScriptRunner | None = None, resource_extensions: tuple[str, ...] | None = None, script_extensions: tuple[str, ...] | None = None, + resource_directories: Sequence[str] | None = None, + script_directories: Sequence[str] | None = None, instruction_template: str | None = None, require_script_approval: bool = False, disable_caching: bool = False, @@ -1672,6 +1681,15 @@ class SkillsProvider(ContextProvider): ``(".md", ".json", ".yaml", ".yml", ".csv", ".xml", ".txt")``. script_extensions: File extensions recognized as discoverable scripts. Defaults to ``(".py",)``. + resource_directories: Relative directory paths to scan for + resource files within each skill directory. Use ``"."`` + to include files at the skill root level. Defaults to + ``("references", "assets")`` per the agentskills.io + specification. + script_directories: Relative directory paths to scan for + script files within each skill directory. Use ``"."`` + to include files at the skill root level. Defaults to + ``("scripts",)`` per the agentskills.io specification. instruction_template: Custom system-prompt template for advertising skills. Must contain a ``{skills}`` placeholder. Uses a built-in template when ``None``. @@ -1701,6 +1719,8 @@ class SkillsProvider(ContextProvider): script_runner=script_runner, resource_extensions=resource_extensions, script_extensions=script_extensions, + resource_directories=resource_directories, + script_directories=script_directories, ) ) return cls( @@ -2186,7 +2206,15 @@ class FileSkillsSource(SkillsSource): Recursively scans the configured *skill_paths* directories for ``SKILL.md`` files (up to 2 levels deep), parses their YAML frontmatter, - and discovers associated resource and script files from subdirectories. + and discovers associated resource and script files from spec-defined + subdirectories. + + By default, resources are discovered from ``references/`` and ``assets/`` + subdirectories, and scripts from ``scripts/``, per the + `agentskills.io specification + `_. Use *resource_directories* + and *script_directories* to customize which subdirectories are scanned. + Pass ``"."`` to include files at the skill root level. Security: file-based metadata is XML-escaped before prompt injection, and resource reads are guarded against path traversal and symlink escape. @@ -2200,14 +2228,15 @@ class FileSkillsSource(SkillsSource): source = FileSkillsSource(skill_paths="./skills") skills = await source.get_skills() - With a script runner and custom extensions: + With a script runner and custom directories: .. code-block:: python source = FileSkillsSource( skill_paths=["./skills", "./more-skills"], script_runner=my_runner, - script_extensions=(".py", ".sh"), + resource_directories=[".", "references", "assets"], + script_directories=["scripts"], ) """ @@ -2218,6 +2247,8 @@ class FileSkillsSource(SkillsSource): script_runner: SkillScriptRunner | None = None, resource_extensions: tuple[str, ...] | None = None, script_extensions: tuple[str, ...] | None = None, + resource_directories: Sequence[str] | None = None, + script_directories: Sequence[str] | None = None, ) -> None: """Initialize a FileSkillsSource. @@ -2237,6 +2268,18 @@ class FileSkillsSource(SkillsSource): ``(".md", ".json", ".yaml", ".yml", ".csv", ".xml", ".txt")``. script_extensions: File extensions recognized as discoverable scripts. Defaults to ``(".py",)``. + resource_directories: Relative directory paths to scan for + resource files within each skill directory. Use ``"."`` + to include files at the skill root level. Defaults to + ``("references", "assets")`` per the + `agentskills.io specification + `_. + script_directories: Relative directory paths to scan for + script files within each skill directory. Use ``"."`` + to include files at the skill root level. Defaults to + ``("scripts",)`` per the + `agentskills.io specification + `_. """ if isinstance(skill_paths, (str, Path)): self._skill_paths: list[str] = [str(skill_paths)] @@ -2247,6 +2290,17 @@ class FileSkillsSource(SkillsSource): self._resource_extensions = resource_extensions or DEFAULT_RESOURCE_EXTENSIONS self._script_extensions = script_extensions or DEFAULT_SCRIPT_EXTENSIONS + self._resource_directories: tuple[str, ...] = ( + tuple(FileSkillsSource._validate_and_normalize_directory_names(resource_directories)) + if resource_directories is not None + else DEFAULT_RESOURCE_DIRECTORIES + ) + self._script_directories: tuple[str, ...] = ( + tuple(FileSkillsSource._validate_and_normalize_directory_names(script_directories)) + if script_directories is not None + else DEFAULT_SCRIPT_DIRECTORIES + ) + async def get_skills(self) -> list[Skill]: """Discover and return all file-based skills from configured paths. @@ -2284,12 +2338,16 @@ class FileSkillsSource(SkillsSource): ) # Discover and attach file-based resources - for rn in FileSkillsSource._discover_resource_files(skill_path, self._resource_extensions): + for rn in FileSkillsSource._discover_resource_files( + skill_path, self._resource_extensions, self._resource_directories + ): resource_full_path = FileSkillsSource._get_validated_resource_path(skill_path, rn) file_skill.resources.append(_FileSkillResource(name=rn, full_path=resource_full_path)) # Discover and attach file-based scripts as SkillScript instances - for sn in FileSkillsSource._discover_script_files(skill_path, self._script_extensions): + for sn in FileSkillsSource._discover_script_files( + skill_path, self._script_extensions, self._script_directories + ): script_full_path = os.path.normpath(os.path.join(skill_path, sn)) # noqa: ASYNC240 file_skill.scripts.append( FileSkillScript(name=sn, full_path=script_full_path, runner=self._script_runner) @@ -2369,116 +2427,274 @@ class FileSkillsSource(SkillsSource): return True return False + @staticmethod + def _validate_and_normalize_directory_names( + directories: Sequence[str], + ) -> list[str]: + """Validate and normalize relative directory names. + + Ensures each entry is a safe relative path. The ``"."`` root indicator + is passed through unchanged. Entries containing ``..`` segments or + representing absolute paths are rejected with a warning and skipped. + Empty or whitespace-only entries raise :class:`ValueError`. + + Args: + directories: Sequence of relative directory names to validate. + + Returns: + A list of validated, normalized directory names. + + Raises: + ValueError: If any entry is empty or whitespace-only. + """ + result: list[str] = [] + for directory in directories: + if not directory or not directory.strip(): + raise ValueError("Directory names must not be empty or whitespace.") + + # Normalize separators: backslash → forward slash, strip leading ./ and trailing / + normalized = PurePosixPath(directory.replace("\\", "/")).as_posix() + + # "." and "./" both normalize to "." — treat as root indicator + if normalized == ROOT_DIRECTORY_INDICATOR: + result.append(ROOT_DIRECTORY_INDICATOR) + continue + + # Reject absolute paths (check both POSIX and Windows-style roots + # so validation is consistent regardless of the host OS) + if ( + os.path.isabs(directory) + or normalized.startswith("/") + or re.match(r"^[A-Za-z]:[/\\]", directory) + ): + logger.warning( + "Skipping directory '%s': absolute paths are not allowed.", + directory, + ) + continue + + # Reject paths containing ".." segments + if any(segment == ".." for segment in normalized.split("/")): + logger.warning( + "Skipping directory '%s': parent traversal ('..') is not allowed.", + directory, + ) + continue + + result.append(normalized) + return result + @staticmethod def _discover_resource_files( skill_dir_path: str, extensions: tuple[str, ...] = DEFAULT_RESOURCE_EXTENSIONS, + directories: tuple[str, ...] = DEFAULT_RESOURCE_DIRECTORIES, ) -> list[str]: - """Scan a skill directory for resource files matching *extensions*. + """Scan configured subdirectories for resource files matching *extensions*. - Recursively walks *skill_dir_path* and collects files whose extension - is in *extensions*, excluding ``SKILL.md`` itself. Each candidate is - validated against path-traversal and symlink-escape checks; unsafe - files are skipped with a warning. + Scans each directory in *directories* within *skill_dir_path* for files + whose extension is in *extensions*, excluding ``SKILL.md`` itself. + Use ``"."`` in *directories* to include files at the skill root level. + Each candidate is validated against path-traversal and symlink-escape + checks; unsafe files are skipped with a warning. Args: skill_dir_path: Absolute path to the skill directory to scan. extensions: Tuple of allowed file extensions (e.g. ``(".md", ".json")``). + directories: Relative subdirectory paths to scan for resources. Returns: - Relative resource paths (forward-slash-separated) for every + Sorted relative resource paths (forward-slash-separated) for every discovered file that passes security checks. """ skill_dir = Path(skill_dir_path).absolute() root_directory_path = str(skill_dir) resources: list[str] = [] normalized_extensions = {e.lower() for e in extensions} + seen_directories: set[str] = set() - for resource_file in skill_dir.rglob("*"): - if not resource_file.is_file(): + for directory in directories: + is_root = directory == ROOT_DIRECTORY_INDICATOR + target_dir = skill_dir if is_root else (skill_dir / directory) + + # Deduplicate after resolving to avoid scanning the same directory twice. + # Use normcase for case-insensitive dedup on case-insensitive filesystems. + resolved_target = str(Path(os.path.normpath(target_dir)).absolute()) + dedup_key = os.path.normcase(resolved_target) + if dedup_key in seen_directories: + continue + seen_directories.add(dedup_key) + + if not target_dir.is_dir(): continue - if resource_file.name.upper() == SKILL_FILE_NAME.upper(): - continue + # Directory-level containment and symlink checks for non-root directories + if not is_root: + if not FileSkillsSource._is_path_within_directory(resolved_target, root_directory_path): + logger.warning( + "Skipping resource directory '%s': resolves outside skill directory '%s'", + directory, + skill_dir_path, + ) + continue - if resource_file.suffix.lower() not in normalized_extensions: - continue + if FileSkillsSource._has_symlink_in_path(resolved_target, root_directory_path): + logger.warning( + "Skipping resource directory '%s': symlink detected in path under skill directory '%s'", + directory, + skill_dir_path, + ) + continue - resource_full_path = str(Path(os.path.normpath(resource_file)).absolute()) - - if not FileSkillsSource._is_path_within_directory(resource_full_path, root_directory_path): + # Scan top-level files only (non-recursive) within this directory + try: + entries = list(target_dir.iterdir()) + except OSError: logger.warning( - "Skipping resource '%s': resolves outside skill directory '%s'", - resource_file, + "Failed to list resource directory '%s' in skill directory '%s'; skipping.", + directory, skill_dir_path, ) continue - if FileSkillsSource._has_symlink_in_path(resource_full_path, root_directory_path): - logger.warning( - "Skipping resource '%s': symlink detected in path under skill directory '%s'", - resource_file, - skill_dir_path, - ) - continue + for resource_file in entries: + if not resource_file.is_file(): + continue - rel_path = resource_file.relative_to(skill_dir) - resources.append(FileSkillsSource._normalize_resource_path(str(rel_path))) + if resource_file.name.upper() == SKILL_FILE_NAME.upper(): + continue + if resource_file.suffix.lower() not in normalized_extensions: + continue + + resource_full_path = str(Path(os.path.normpath(resource_file)).absolute()) + + # Containment check: file must resolve within the target directory + if not FileSkillsSource._is_path_within_directory(resource_full_path, resolved_target): + logger.warning( + "Skipping resource '%s': resolves outside target directory '%s'", + resource_file, + directory, + ) + continue + + if FileSkillsSource._has_symlink_in_path(resource_full_path, root_directory_path): + logger.warning( + "Skipping resource '%s': symlink detected in path under skill directory '%s'", + resource_file, + skill_dir_path, + ) + continue + + rel_path = resource_file.relative_to(skill_dir) + resources.append(FileSkillsSource._normalize_resource_path(str(rel_path))) + + resources.sort() return resources @staticmethod def _discover_script_files( skill_dir_path: str, extensions: tuple[str, ...] = DEFAULT_SCRIPT_EXTENSIONS, + directories: tuple[str, ...] = DEFAULT_SCRIPT_DIRECTORIES, ) -> list[str]: - """Scan a skill directory for script files matching *extensions*. + """Scan configured subdirectories for script files matching *extensions*. - Recursively walks *skill_dir_path* and collects files whose extension - is in *extensions*. Each candidate is validated against path-traversal - and symlink-escape checks; unsafe files are skipped with a warning. + Scans each directory in *directories* within *skill_dir_path* for files + whose extension is in *extensions*. Use ``"."`` in *directories* to + include files at the skill root level. Each candidate is validated + against path-traversal and symlink-escape checks; unsafe files are + skipped with a warning. Args: skill_dir_path: Absolute path to the skill directory to scan. extensions: Tuple of allowed script extensions (e.g. ``(".py",)``). + directories: Relative subdirectory paths to scan for scripts. Returns: - Relative script paths (forward-slash-separated) for every + Sorted relative script paths (forward-slash-separated) for every discovered file that passes security checks. """ skill_dir = Path(skill_dir_path).absolute() root_directory_path = str(skill_dir) scripts: list[str] = [] normalized_extensions = {e.lower() for e in extensions} + seen_directories: set[str] = set() - for script_file in skill_dir.rglob("*"): - if not script_file.is_file(): + for directory in directories: + is_root = directory == ROOT_DIRECTORY_INDICATOR + target_dir = skill_dir if is_root else (skill_dir / directory) + + # Deduplicate after resolving to avoid scanning the same directory twice. + # Use normcase for case-insensitive dedup on case-insensitive filesystems. + resolved_target = str(Path(os.path.normpath(target_dir)).absolute()) + dedup_key = os.path.normcase(resolved_target) + if dedup_key in seen_directories: + continue + seen_directories.add(dedup_key) + + if not target_dir.is_dir(): continue - if script_file.suffix.lower() not in normalized_extensions: - continue + # Directory-level containment and symlink checks for non-root directories + if not is_root: + if not FileSkillsSource._is_path_within_directory(resolved_target, root_directory_path): + logger.warning( + "Skipping script directory '%s': resolves outside skill directory '%s'", + directory, + skill_dir_path, + ) + continue - script_full_path = str(Path(os.path.normpath(script_file)).absolute()) + if FileSkillsSource._has_symlink_in_path(resolved_target, root_directory_path): + logger.warning( + "Skipping script directory '%s': symlink detected in path under skill directory '%s'", + directory, + skill_dir_path, + ) + continue - if not FileSkillsSource._is_path_within_directory(script_full_path, root_directory_path): + # Scan top-level files only (non-recursive) within this directory + try: + entries = list(target_dir.iterdir()) + except OSError: logger.warning( - "Skipping script '%s': resolves outside skill directory '%s'", - script_file, + "Failed to list script directory '%s' in skill directory '%s'; skipping.", + directory, skill_dir_path, ) continue - if FileSkillsSource._has_symlink_in_path(script_full_path, root_directory_path): - logger.warning( - "Skipping script '%s': symlink detected in path under skill directory '%s'", - script_file, - skill_dir_path, - ) - continue + for script_file in entries: + if not script_file.is_file(): + continue - rel_path = script_file.relative_to(skill_dir) - scripts.append(FileSkillsSource._normalize_resource_path(str(rel_path))) + if script_file.suffix.lower() not in normalized_extensions: + continue + script_full_path = str(Path(os.path.normpath(script_file)).absolute()) + + # Containment check: file must resolve within the target directory + if not FileSkillsSource._is_path_within_directory(script_full_path, resolved_target): + logger.warning( + "Skipping script '%s': resolves outside target directory '%s'", + script_file, + directory, + ) + continue + + if FileSkillsSource._has_symlink_in_path(script_full_path, root_directory_path): + logger.warning( + "Skipping script '%s': symlink detected in path under skill directory '%s'", + script_file, + skill_dir_path, + ) + continue + + rel_path = script_file.relative_to(skill_dir) + scripts.append(FileSkillsSource._normalize_resource_path(str(rel_path))) + + scripts.sort() return scripts @staticmethod diff --git a/python/packages/core/tests/core/test_skills.py b/python/packages/core/tests/core/test_skills.py index de39c58b2f..30eba73237 100644 --- a/python/packages/core/tests/core/test_skills.py +++ b/python/packages/core/tests/core/test_skills.py @@ -31,8 +31,11 @@ from agent_framework import ( SkillsProvider, ) from agent_framework._skills import ( + DEFAULT_RESOURCE_DIRECTORIES, DEFAULT_RESOURCE_EXTENSIONS, + DEFAULT_SCRIPT_DIRECTORIES, DEFAULT_SCRIPT_EXTENSIONS, + ROOT_DIRECTORY_INDICATOR, InlineSkillResource, InlineSkillScript, _create_resource_element, @@ -143,6 +146,8 @@ async def _discover_file_skills_for_test( *, resource_extensions: tuple[str, ...] | None = None, script_extensions: tuple[str, ...] | None = None, + resource_directories: Sequence[str] | None = None, + script_directories: Sequence[str] | None = None, script_runner: Any = None, ) -> dict[str, FileSkill]: """Test helper: discover file skills and return as a dict keyed by name. @@ -155,6 +160,10 @@ async def _discover_file_skills_for_test( kwargs["resource_extensions"] = resource_extensions if script_extensions is not None: kwargs["script_extensions"] = script_extensions + if resource_directories is not None: + kwargs["resource_directories"] = resource_directories + if script_directories is not None: + kwargs["script_directories"] = script_directories if script_runner is not None: kwargs["script_runner"] = script_runner @@ -191,59 +200,103 @@ class TestNormalizeResourcePath: class TestDiscoverResourceFiles: """Tests for _discover_resource_files (filesystem-based resource discovery).""" - def test_discovers_md_files(self, tmp_path: Path) -> None: + def test_discovers_md_files_in_references(self, tmp_path: Path) -> None: skill_dir = tmp_path / "my-skill" skill_dir.mkdir() (skill_dir / "SKILL.md").write_text("---\nname: s\ndescription: d\n---\n", encoding="utf-8") - refs = skill_dir / "refs" + refs = skill_dir / "references" refs.mkdir() (refs / "FAQ.md").write_text("FAQ content", encoding="utf-8") resources = FileSkillsSource._discover_resource_files(str(skill_dir)) - assert "refs/FAQ.md" in resources + assert "references/FAQ.md" in resources - def test_excludes_skill_md(self, tmp_path: Path) -> None: + def test_discovers_md_files_in_assets(self, tmp_path: Path) -> None: + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + assets = skill_dir / "assets" + assets.mkdir() + (assets / "guide.md").write_text("guide", encoding="utf-8") + resources = FileSkillsSource._discover_resource_files(str(skill_dir)) + assert "assets/guide.md" in resources + + def test_excludes_skill_md_at_root(self, tmp_path: Path) -> None: skill_dir = tmp_path / "my-skill" skill_dir.mkdir() (skill_dir / "SKILL.md").write_text("content", encoding="utf-8") - resources = FileSkillsSource._discover_resource_files(str(skill_dir)) + resources = FileSkillsSource._discover_resource_files(str(skill_dir), directories=(".",)) assert len(resources) == 0 def test_discovers_multiple_extensions(self, tmp_path: Path) -> None: skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - (skill_dir / "data.json").write_text("{}", encoding="utf-8") - (skill_dir / "config.yaml").write_text("key: val", encoding="utf-8") - (skill_dir / "notes.txt").write_text("notes", encoding="utf-8") + refs = skill_dir / "references" + refs.mkdir(parents=True) + (refs / "data.json").write_text("{}", encoding="utf-8") + (refs / "config.yaml").write_text("key: val", encoding="utf-8") + (refs / "notes.txt").write_text("notes", encoding="utf-8") resources = FileSkillsSource._discover_resource_files(str(skill_dir)) assert len(resources) == 3 names = set(resources) - assert "data.json" in names - assert "config.yaml" in names - assert "notes.txt" in names + assert "references/data.json" in names + assert "references/config.yaml" in names + assert "references/notes.txt" in names def test_ignores_unsupported_extensions(self, tmp_path: Path) -> None: skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - (skill_dir / "image.png").write_bytes(b"\x89PNG") - (skill_dir / "binary.exe").write_bytes(b"\x00") + refs = skill_dir / "references" + refs.mkdir(parents=True) + (refs / "image.png").write_bytes(b"\x89PNG") + (refs / "binary.exe").write_bytes(b"\x00") resources = FileSkillsSource._discover_resource_files(str(skill_dir)) assert len(resources) == 0 def test_custom_extensions(self, tmp_path: Path) -> None: skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - (skill_dir / "data.json").write_text("{}", encoding="utf-8") - (skill_dir / "notes.txt").write_text("notes", encoding="utf-8") + refs = skill_dir / "references" + refs.mkdir(parents=True) + (refs / "data.json").write_text("{}", encoding="utf-8") + (refs / "notes.txt").write_text("notes", encoding="utf-8") resources = FileSkillsSource._discover_resource_files(str(skill_dir), extensions=(".json",)) - assert resources == ["data.json"] + assert resources == ["references/data.json"] - def test_discovers_nested_files(self, tmp_path: Path) -> None: + def test_does_not_discover_nested_files(self, tmp_path: Path) -> None: + """Non-recursive: files inside subdirectories of configured dirs are not discovered.""" skill_dir = tmp_path / "my-skill" - sub = skill_dir / "refs" / "deep" + sub = skill_dir / "references" / "deep" sub.mkdir(parents=True) (sub / "doc.md").write_text("deep doc", encoding="utf-8") resources = FileSkillsSource._discover_resource_files(str(skill_dir)) - assert "refs/deep/doc.md" in resources + assert len(resources) == 0 + + def test_root_directory_discovers_root_files(self, tmp_path: Path) -> None: + """The '.' root indicator discovers files at the skill root level.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + (skill_dir / "data.json").write_text("{}", encoding="utf-8") + resources = FileSkillsSource._discover_resource_files(str(skill_dir), directories=(".",)) + assert "data.json" in resources + + def test_root_does_not_discover_by_default(self, tmp_path: Path) -> None: + """Files at skill root are not discovered with default directories.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + (skill_dir / "data.json").write_text("{}", encoding="utf-8") + resources = FileSkillsSource._discover_resource_files(str(skill_dir)) + assert len(resources) == 0 + + def test_custom_directories(self, tmp_path: Path) -> None: + """Custom directory names override defaults.""" + skill_dir = tmp_path / "my-skill" + custom = skill_dir / "docs" + custom.mkdir(parents=True) + (custom / "readme.md").write_text("readme", encoding="utf-8") + resources = FileSkillsSource._discover_resource_files(str(skill_dir), directories=("docs",)) + assert "docs/readme.md" in resources + + def test_nonexistent_directory_silently_skipped(self, tmp_path: Path) -> None: + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + resources = FileSkillsSource._discover_resource_files(str(skill_dir), directories=("nonexistent",)) + assert resources == [] def test_empty_directory(self, tmp_path: Path) -> None: skill_dir = tmp_path / "my-skill" @@ -260,6 +313,27 @@ class TestDiscoverResourceFiles: assert ".xml" in DEFAULT_RESOURCE_EXTENSIONS assert ".txt" in DEFAULT_RESOURCE_EXTENSIONS + def test_duplicate_directories_deduplicated(self, tmp_path: Path) -> None: + """Duplicate directory entries should not produce duplicate resources.""" + skill_dir = tmp_path / "my-skill" + refs = skill_dir / "references" + refs.mkdir(parents=True) + (refs / "doc.md").write_text("content", encoding="utf-8") + resources = FileSkillsSource._discover_resource_files( + str(skill_dir), directories=("references", "references") + ) + assert resources == ["references/doc.md"] + + def test_results_are_sorted(self, tmp_path: Path) -> None: + """Results should be sorted for stable ordering.""" + skill_dir = tmp_path / "my-skill" + refs = skill_dir / "references" + refs.mkdir(parents=True) + (refs / "zebra.md").write_text("z", encoding="utf-8") + (refs / "alpha.md").write_text("a", encoding="utf-8") + resources = FileSkillsSource._discover_resource_files(str(skill_dir)) + assert resources == ["references/alpha.md", "references/zebra.md"] + class TestTryParseSkillDocument: """Tests for _extract_frontmatter.""" @@ -413,11 +487,11 @@ class TestDiscoverAndLoadSkills: tmp_path, "my-skill", body="Instructions here.", - resources={"refs/FAQ.md": "FAQ content"}, + resources={"references/FAQ.md": "FAQ content"}, ) skills = await _discover_file_skills_for_test([str(tmp_path)]) assert "my-skill" in skills - assert [r.name for r in skills["my-skill"].resources] == ["refs/FAQ.md"] + assert [r.name for r in skills["my-skill"].resources] == ["references/FAQ.md"] async def test_skill_discovers_all_resource_files(self, tmp_path: Path) -> None: """Resources are discovered by filesystem scan, not by markdown links.""" @@ -425,13 +499,13 @@ class TestDiscoverAndLoadSkills: tmp_path, "my-skill", body="No links here.", - resources={"data.json": '{"key": "val"}', "refs/doc.md": "doc content"}, + resources={"references/data.json": '{"key": "val"}', "assets/doc.md": "doc content"}, ) skills = await _discover_file_skills_for_test([str(tmp_path)]) assert "my-skill" in skills resource_names = sorted(r.name for r in skills["my-skill"].resources) - assert "data.json" in resource_names - assert "refs/doc.md" in resource_names + assert "assets/doc.md" in resource_names + assert "references/data.json" in resource_names # --------------------------------------------------------------------------- @@ -446,12 +520,12 @@ class TestReadSkillResource: _write_skill( tmp_path, "my-skill", - body="See [doc](refs/FAQ.md).", - resources={"refs/FAQ.md": "FAQ content here"}, + body="See [doc](references/FAQ.md).", + resources={"references/FAQ.md": "FAQ content here"}, ) skill_dir = tmp_path / "my-skill" - full_path = str(skill_dir / "refs" / "FAQ.md") - resource = _FileSkillResource(name="refs/FAQ.md", full_path=full_path) + full_path = str(skill_dir / "references" / "FAQ.md") + resource = _FileSkillResource(name="references/FAQ.md", full_path=full_path) content = await resource.read() assert content == "FAQ content here" @@ -468,12 +542,12 @@ class TestReadSkillResource: _write_skill( tmp_path, "my-skill", - body="See [doc](refs/FAQ.md).", - resources={"refs/FAQ.md": "FAQ content"}, + body="See [doc](references/FAQ.md).", + resources={"references/FAQ.md": "FAQ content"}, ) skill_dir = tmp_path / "my-skill" - full_path = str(skill_dir / "refs" / "FAQ.md") - resource = _FileSkillResource(name="refs/FAQ.md", full_path=full_path) + full_path = str(skill_dir / "references" / "FAQ.md") + resource = _FileSkillResource(name="references/FAQ.md", full_path=full_path) content = await resource.read() assert content == "FAQ content" @@ -486,7 +560,7 @@ class TestReadSkillResource: skill_dir = tmp_path / "skill" skill_dir.mkdir() (tmp_path / "secret.md").write_text("secret", encoding="utf-8") - resources = FileSkillsSource._discover_resource_files(str(skill_dir)) + resources = FileSkillsSource._discover_resource_files(str(skill_dir), directories=(".",)) assert not any("secret" in r for r in resources) @@ -633,13 +707,13 @@ class TestSkillsProvider: _write_skill( tmp_path, "my-skill", - body="See [doc](refs/FAQ.md).", - resources={"refs/FAQ.md": "FAQ content"}, + body="See [doc](references/FAQ.md).", + resources={"references/FAQ.md": "FAQ content"}, ) provider = SkillsProvider.from_paths(str(tmp_path)) await _init_provider(provider) result = provider._load_skill(_raw_skills(provider), "my-skill") - assert "See [doc](refs/FAQ.md)." in result + assert "See [doc](references/FAQ.md)." in result async def test_load_skill_unknown_returns_error(self, tmp_path: Path) -> None: provider = SkillsProvider.from_paths(str(tmp_path)) @@ -657,12 +731,12 @@ class TestSkillsProvider: _write_skill( tmp_path, "my-skill", - body="See [doc](refs/FAQ.md).", - resources={"refs/FAQ.md": "FAQ content"}, + body="See [doc](references/FAQ.md).", + resources={"references/FAQ.md": "FAQ content"}, ) provider = SkillsProvider.from_paths(str(tmp_path)) await _init_provider(provider) - result = await provider._read_skill_resource(_raw_skills(provider), "my-skill", "refs/FAQ.md") + result = await provider._read_skill_resource(_raw_skills(provider), "my-skill", "references/FAQ.md") assert result == "FAQ content" async def test_read_skill_resource_unknown_skill_returns_error(self, tmp_path: Path) -> None: @@ -791,7 +865,7 @@ class TestSymlinkDetection: "---\nname: my-skill\ndescription: A test skill.\n---\nInstructions.\n", encoding="utf-8", ) - refs_dir = skill_dir / "refs" + refs_dir = skill_dir / "references" refs_dir.mkdir() (refs_dir / "leak.md").symlink_to(outside_file) # Also add a safe resource @@ -800,8 +874,8 @@ class TestSymlinkDetection: skills = await _discover_file_skills_for_test([str(tmp_path)]) assert "my-skill" in skills resource_names = [r.name for r in skills["my-skill"].resources] - assert "refs/leak.md" not in resource_names - assert "refs/safe.md" in resource_names + assert "references/leak.md" not in resource_names + assert "references/safe.md" in resource_names def test_discover_resource_files_rejects_symlinked_resource(self, tmp_path: Path) -> None: """_discover_resource_files should exclude a symlinked resource file.""" @@ -811,12 +885,12 @@ class TestSymlinkDetection: outside_file = tmp_path / "secret.md" outside_file.write_text("secret content", encoding="utf-8") - refs_dir = skill_dir / "refs" + refs_dir = skill_dir / "references" refs_dir.mkdir() (refs_dir / "leak.md").symlink_to(outside_file) resources = FileSkillsSource._discover_resource_files(str(skill_dir)) - assert "refs/leak.md" not in resources + assert "references/leak.md" not in resources def test_discover_skips_symlinked_script(self, tmp_path: Path) -> None: """_discover_script_files should skip scripts with symlinks in their path.""" @@ -1361,21 +1435,22 @@ class TestSkillsProviderCodeSkill: async def test_custom_resource_extensions(self, tmp_path: Path) -> None: """SkillsProvider accepts custom resource_extensions.""" skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() + refs = skill_dir / "references" + refs.mkdir(parents=True) (skill_dir / "SKILL.md").write_text( "---\nname: my-skill\ndescription: A test skill.\n---\nBody.", encoding="utf-8", ) - (skill_dir / "data.json").write_text("{}", encoding="utf-8") - (skill_dir / "notes.txt").write_text("notes", encoding="utf-8") + (refs / "data.json").write_text("{}", encoding="utf-8") + (refs / "notes.txt").write_text("notes", encoding="utf-8") # Only discover .json files provider = SkillsProvider.from_paths(str(tmp_path), resource_extensions=(".json",)) await _init_provider(provider) skill = _ctx(provider)[0]["my-skill"] resource_names = [r.name for r in skill.resources] - assert "data.json" in resource_names - assert "notes.txt" not in resource_names + assert "references/data.json" in resource_names + assert "references/notes.txt" not in resource_names # --------------------------------------------------------------------------- @@ -1407,11 +1482,11 @@ class TestFileBasedSkillParsing: assert skill.path == str(tmp_path / "my-skill") async def test_resources_populated(self, tmp_path: Path) -> None: - _write_skill(tmp_path, "my-skill", resources={"refs/doc.md": "content"}) + _write_skill(tmp_path, "my-skill", resources={"references/doc.md": "content"}) skills = await _discover_file_skills_for_test([str(tmp_path)]) assert "my-skill" in skills resource_names = [r.name for r in skills["my-skill"].resources] - assert "refs/doc.md" in resource_names + assert "references/doc.md" in resource_names # --------------------------------------------------------------------------- @@ -1467,12 +1542,12 @@ class TestDiscoverResourceFilesEdgeCases: """Additional edge-case tests for filesystem resource discovery.""" def test_excludes_skill_md_case_insensitive(self, tmp_path: Path) -> None: - """SKILL.md in any casing is excluded.""" + """SKILL.md in any casing is excluded when scanning root.""" skill_dir = tmp_path / "my-skill" skill_dir.mkdir() (skill_dir / "skill.md").write_text("lowercase name", encoding="utf-8") (skill_dir / "other.md").write_text("keep me", encoding="utf-8") - resources = FileSkillsSource._discover_resource_files(str(skill_dir)) + resources = FileSkillsSource._discover_resource_files(str(skill_dir), directories=(".",)) names = [r.lower() for r in resources] assert "skill.md" not in names assert "other.md" in resources @@ -1480,19 +1555,221 @@ class TestDiscoverResourceFilesEdgeCases: def test_skips_directories(self, tmp_path: Path) -> None: """Directories are not included as resources even if their name matches an extension.""" skill_dir = tmp_path / "my-skill" - subdir = skill_dir / "data.json" - subdir.mkdir(parents=True) + refs = skill_dir / "references" + refs.mkdir(parents=True) + subdir = refs / "data.json" + subdir.mkdir() resources = FileSkillsSource._discover_resource_files(str(skill_dir)) assert resources == [] def test_extension_matching_is_case_insensitive(self, tmp_path: Path) -> None: skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - (skill_dir / "NOTES.TXT").write_text("caps", encoding="utf-8") + refs = skill_dir / "references" + refs.mkdir(parents=True) + (refs / "NOTES.TXT").write_text("caps", encoding="utf-8") resources = FileSkillsSource._discover_resource_files(str(skill_dir)) assert len(resources) == 1 +class TestDiscoverFilesOSErrorWarning: + """OSError during directory listing should log a warning, not fail silently.""" + + def test_resource_discovery_warns_on_oserror(self, tmp_path: Path, caplog: pytest.LogCaptureFixture) -> None: + """_discover_resource_files logs a warning when iterdir() raises OSError.""" + skill_dir = tmp_path / "my-skill" + refs = skill_dir / "references" + refs.mkdir(parents=True) + (refs / "guide.md").write_text("content", encoding="utf-8") + + original_iterdir = Path.iterdir + + def _patched_iterdir(self: Path) -> Any: + if self.name == "references": + raise PermissionError("access denied") + return original_iterdir(self) + + import unittest.mock + + with unittest.mock.patch.object(Path, "iterdir", _patched_iterdir): + resources = FileSkillsSource._discover_resource_files(str(skill_dir)) + + assert resources == [] + assert any("Failed to list resource directory" in r.message for r in caplog.records) + + def test_script_discovery_warns_on_oserror(self, tmp_path: Path, caplog: pytest.LogCaptureFixture) -> None: + """_discover_script_files logs a warning when iterdir() raises OSError.""" + skill_dir = tmp_path / "my-skill" + scripts_dir = skill_dir / "scripts" + scripts_dir.mkdir(parents=True) + (scripts_dir / "run.py").write_text("print('hi')", encoding="utf-8") + + original_iterdir = Path.iterdir + + def _patched_iterdir(self: Path) -> Any: + if self.name == "scripts": + raise PermissionError("access denied") + return original_iterdir(self) + + import unittest.mock + + with unittest.mock.patch.object(Path, "iterdir", _patched_iterdir): + scripts = FileSkillsSource._discover_script_files(str(skill_dir)) + + assert scripts == [] + assert any("Failed to list script directory" in r.message for r in caplog.records) + + +class TestValidateAndNormalizeDirectoryNames: + """Tests for _validate_and_normalize_directory_names.""" + + def test_simple_directory_name(self) -> None: + result = FileSkillsSource._validate_and_normalize_directory_names(["references"]) + assert result == ["references"] + + def test_root_indicator(self) -> None: + result = FileSkillsSource._validate_and_normalize_directory_names(["."]) + assert result == ["."] + + def test_dot_slash_normalizes_to_root(self) -> None: + result = FileSkillsSource._validate_and_normalize_directory_names(["./"]) + assert result == ["."] + + def test_backslash_dot_normalizes_to_root(self) -> None: + result = FileSkillsSource._validate_and_normalize_directory_names([".\\"]) + assert result == ["."] + + def test_backslashes_normalized(self) -> None: + result = FileSkillsSource._validate_and_normalize_directory_names(["sub\\scripts"]) + assert result == ["sub/scripts"] + + def test_trailing_slash_stripped(self) -> None: + result = FileSkillsSource._validate_and_normalize_directory_names(["scripts/"]) + assert result == ["scripts"] + + def test_leading_dot_slash_stripped(self) -> None: + result = FileSkillsSource._validate_and_normalize_directory_names(["./references"]) + assert result == ["references"] + + def test_rejects_parent_traversal(self) -> None: + result = FileSkillsSource._validate_and_normalize_directory_names(["../secrets"]) + assert result == [] + + def test_rejects_embedded_parent_traversal(self) -> None: + result = FileSkillsSource._validate_and_normalize_directory_names(["sub/../secrets"]) + assert result == [] + + def test_rejects_absolute_path(self) -> None: + result = FileSkillsSource._validate_and_normalize_directory_names(["/etc/passwd"]) + assert result == [] + + def test_rejects_windows_absolute_path(self) -> None: + result = FileSkillsSource._validate_and_normalize_directory_names(["C:\\Windows"]) + assert result == [] + + def test_empty_string_raises(self) -> None: + with pytest.raises(ValueError, match="empty or whitespace"): + FileSkillsSource._validate_and_normalize_directory_names([""]) + + def test_whitespace_only_raises(self) -> None: + with pytest.raises(ValueError, match="empty or whitespace"): + FileSkillsSource._validate_and_normalize_directory_names([" "]) + + def test_multiple_directories(self) -> None: + result = FileSkillsSource._validate_and_normalize_directory_names( + [".", "references", "assets", "scripts"] + ) + assert result == [".", "references", "assets", "scripts"] + + def test_default_resource_directories(self) -> None: + assert DEFAULT_RESOURCE_DIRECTORIES == ("references", "assets") + + def test_default_script_directories(self) -> None: + assert DEFAULT_SCRIPT_DIRECTORIES == ("scripts",) + + def test_root_directory_indicator_is_dot(self) -> None: + assert ROOT_DIRECTORY_INDICATOR == "." + + +class TestFileSkillsSourceDirectories: + """Tests for resource_directories and script_directories parameters.""" + + async def test_custom_resource_directories(self, tmp_path: Path) -> None: + """Custom resource_directories controls which dirs are scanned.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + "---\nname: my-skill\ndescription: test\n---\nBody", + encoding="utf-8", + ) + # Put resource in a custom directory + docs = skill_dir / "docs" + docs.mkdir() + (docs / "guide.md").write_text("guide", encoding="utf-8") + # Also put one in default references/ — should not be found + refs = skill_dir / "references" + refs.mkdir() + (refs / "ref.md").write_text("ref", encoding="utf-8") + + source = FileSkillsSource(str(tmp_path), resource_directories=["docs"]) + skills = await source.get_skills() + resource_names = [r.name for r in skills[0].resources] + assert "docs/guide.md" in resource_names + assert "references/ref.md" not in resource_names + + async def test_custom_script_directories(self, tmp_path: Path) -> None: + """Custom script_directories controls which dirs are scanned.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + "---\nname: my-skill\ndescription: test\n---\nBody", + encoding="utf-8", + ) + # Put script in a custom directory + tools = skill_dir / "tools" + tools.mkdir() + (tools / "run.py").write_text("print('run')", encoding="utf-8") + + source = FileSkillsSource(str(tmp_path), script_directories=["tools"]) + skills = await source.get_skills() + script_names = [s.name for s in skills[0].scripts] + assert "tools/run.py" in script_names + + async def test_root_indicator_discovers_root_files(self, tmp_path: Path) -> None: + """The '.' root indicator discovers files at the skill root.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + "---\nname: my-skill\ndescription: test\n---\nBody", + encoding="utf-8", + ) + (skill_dir / "data.json").write_text("{}", encoding="utf-8") + + source = FileSkillsSource(str(tmp_path), resource_directories=[".", "references"]) + skills = await source.get_skills() + resource_names = [r.name for r in skills[0].resources] + assert "data.json" in resource_names + + async def test_from_paths_passes_directories(self, tmp_path: Path) -> None: + """from_paths passes resource_directories and script_directories through.""" + skill_dir = tmp_path / "my-skill" + docs = skill_dir / "docs" + docs.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text( + "---\nname: my-skill\ndescription: test\n---\nBody", + encoding="utf-8", + ) + (docs / "guide.md").write_text("guide", encoding="utf-8") + + provider = SkillsProvider.from_paths( + str(tmp_path), + resource_directories=["docs"], + ) + await _init_provider(provider) + skill = _ctx(provider)[0]["my-skill"] + resource_names = [r.name for r in skill.resources] + assert "docs/guide.md" in resource_names + + # --------------------------------------------------------------------------- # Tests: _is_path_within_directory # --------------------------------------------------------------------------- @@ -2928,12 +3205,13 @@ class TestSkillsProviderFactories: assert isinstance(_CustomRunner(), SkillScriptRunner) skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() + scripts_dir = skill_dir / "scripts" + scripts_dir.mkdir(parents=True) (skill_dir / "SKILL.md").write_text( "---\nname: my-skill\ndescription: test\n---\nBody", encoding="utf-8", ) - (skill_dir / "run.py").write_text("print('hi')", encoding="utf-8") + (scripts_dir / "run.py").write_text("print('hi')", encoding="utf-8") provider = SkillsProvider.from_paths( str(tmp_path), @@ -2949,12 +3227,13 @@ class TestSkillsProviderFactories: assert isinstance(sync_runner, SkillScriptRunner) skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() + scripts_dir = skill_dir / "scripts" + scripts_dir.mkdir(parents=True) (skill_dir / "SKILL.md").write_text( "---\nname: my-skill\ndescription: test\n---\nBody", encoding="utf-8", ) - (skill_dir / "run.py").write_text("print('hi')", encoding="utf-8") + (scripts_dir / "run.py").write_text("print('hi')", encoding="utf-8") provider = SkillsProvider.from_paths( str(tmp_path), @@ -2966,12 +3245,13 @@ class TestSkillsProviderFactories: async def test_file_script_with_sync_runner_executes(self, tmp_path: Path) -> None: """A sync script_runner is awaitable through the provider's run_skill_script.""" skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() + scripts_dir = skill_dir / "scripts" + scripts_dir.mkdir(parents=True) (skill_dir / "SKILL.md").write_text( "---\nname: my-skill\ndescription: test\n---\nBody", encoding="utf-8", ) - (skill_dir / "run.py").write_text("print('hi')", encoding="utf-8") + (scripts_dir / "run.py").write_text("print('hi')", encoding="utf-8") def sync_runner(skill, script, args=None): return f"sync: {script.name} args={args}" @@ -2982,17 +3262,18 @@ class TestSkillsProviderFactories: ) await _init_provider(provider) run_tool = next(t for t in _ctx(provider)[2] if hasattr(t, "name") and t.name == "run_skill_script") - result = await run_tool.func(skill_name="my-skill", script_name="run.py", args={"key": "val"}) - assert result == "sync: run.py args={'key': 'val'}" + result = await run_tool.func(skill_name="my-skill", script_name="scripts/run.py", args={"key": "val"}) + assert result == "sync: scripts/run.py args={'key': 'val'}" async def test_file_skills_with_callback_runner(self, tmp_path: Path) -> None: skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() + scripts_dir = skill_dir / "scripts" + scripts_dir.mkdir(parents=True) (skill_dir / "SKILL.md").write_text( "---\nname: my-skill\ndescription: test\n---\nBody", encoding="utf-8", ) - (skill_dir / "run.py").write_text("print('hi')", encoding="utf-8") + (scripts_dir / "run.py").write_text("print('hi')", encoding="utf-8") provider = SkillsProvider.from_paths( str(tmp_path), @@ -3028,12 +3309,13 @@ class TestSkillsProviderFactories: async def test_file_scripts_without_runner_no_error_at_init(self, tmp_path: Path) -> None: skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() + scripts_dir = skill_dir / "scripts" + scripts_dir.mkdir(parents=True) (skill_dir / "SKILL.md").write_text( "---\nname: my-skill\ndescription: test\n---\nBody", encoding="utf-8", ) - (skill_dir / "run.py").write_text("print('hi')", encoding="utf-8") + (scripts_dir / "run.py").write_text("print('hi')", encoding="utf-8") provider = SkillsProvider.from_paths(str(tmp_path)) # Initialization succeeds; the error now surfaces at script.run() time @@ -3309,7 +3591,23 @@ class TestSkillsProviderFactories: class TestFileScriptDiscovery: """Tests for automatic .py script discovery in skill directories.""" - async def test_discovers_py_files(self, tmp_path: Path) -> None: + async def test_discovers_py_files_in_scripts_dir(self, tmp_path: Path) -> None: + skill_dir = tmp_path / "my-skill" + scripts_dir = skill_dir / "scripts" + scripts_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text( + "---\nname: my-skill\ndescription: test\n---\nBody", + encoding="utf-8", + ) + (scripts_dir / "analyze.py").write_text("print('hi')", encoding="utf-8") + + skills = await _discover_file_skills_for_test(str(tmp_path)) + assert "my-skill" in skills + assert len(skills["my-skill"].scripts) == 1 + assert skills["my-skill"].scripts[0].name == "scripts/analyze.py" + + async def test_root_py_files_not_discovered_by_default(self, tmp_path: Path) -> None: + """Scripts at the skill root are NOT discovered with default directories.""" skill_dir = tmp_path / "my-skill" skill_dir.mkdir() (skill_dir / "SKILL.md").write_text( @@ -3320,8 +3618,7 @@ class TestFileScriptDiscovery: skills = await _discover_file_skills_for_test(str(tmp_path)) assert "my-skill" in skills - assert len(skills["my-skill"].scripts) == 1 - assert skills["my-skill"].scripts[0].name == "analyze.py" + assert len(skills["my-skill"].scripts) == 0 async def test_discovered_script_has_absolute_full_path(self, tmp_path: Path) -> None: skill_dir = tmp_path / "my-skill" @@ -3340,7 +3637,8 @@ class TestFileScriptDiscovery: expected = str(Path(str(skill_dir), "scripts", "generate.py")) assert script.full_path == expected - async def test_discovers_nested_scripts(self, tmp_path: Path) -> None: + async def test_scripts_not_discovered_recursively(self, tmp_path: Path) -> None: + """Scripts inside subdirectories of scripts/ are NOT discovered (non-recursive).""" skill_dir = tmp_path / "my-skill" scripts_dir = skill_dir / "scripts" scripts_dir.mkdir(parents=True) @@ -3348,11 +3646,16 @@ class TestFileScriptDiscovery: "---\nname: my-skill\ndescription: test\n---\nBody", encoding="utf-8", ) - (scripts_dir / "generate.py").write_text("print('gen')", encoding="utf-8") + # File directly in scripts/ is discovered + (scripts_dir / "top.py").write_text("print('top')", encoding="utf-8") + # File in scripts/sub/ is NOT discovered + sub_dir = scripts_dir / "sub" + sub_dir.mkdir() + (sub_dir / "nested.py").write_text("print('nested')", encoding="utf-8") skills = await _discover_file_skills_for_test(str(tmp_path)) assert len(skills["my-skill"].scripts) == 1 - assert skills["my-skill"].scripts[0].name == "scripts/generate.py" + assert skills["my-skill"].scripts[0].name == "scripts/top.py" async def test_no_scripts_when_no_py_files(self, tmp_path: Path) -> None: skill_dir = tmp_path / "my-skill" @@ -3373,36 +3676,38 @@ class TestCustomScriptExtensions: async def test_custom_script_extensions_via_get_skills(self, tmp_path: Path) -> None: """get_skills() forwards script_extensions to _discover_script_files.""" skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() + scripts_dir = skill_dir / "scripts" + scripts_dir.mkdir(parents=True) (skill_dir / "SKILL.md").write_text( "---\nname: my-skill\ndescription: test\n---\nBody", encoding="utf-8", ) - (skill_dir / "analyze.py").write_text("print('hi')", encoding="utf-8") - (skill_dir / "run.sh").write_text("#!/bin/bash", encoding="utf-8") + (scripts_dir / "analyze.py").write_text("print('hi')", encoding="utf-8") + (scripts_dir / "run.sh").write_text("#!/bin/bash", encoding="utf-8") # Default: only .py discovered skills_default = await _discover_file_skills_for_test(str(tmp_path)) script_names_default = [s.name for s in skills_default["my-skill"].scripts] - assert "analyze.py" in script_names_default - assert "run.sh" not in script_names_default + assert "scripts/analyze.py" in script_names_default + assert "scripts/run.sh" not in script_names_default # Custom: only .sh discovered skills_custom = await _discover_file_skills_for_test(str(tmp_path), script_extensions=(".sh",)) script_names_custom = [s.name for s in skills_custom["my-skill"].scripts] - assert "run.sh" in script_names_custom - assert "analyze.py" not in script_names_custom + assert "scripts/run.sh" in script_names_custom + assert "scripts/analyze.py" not in script_names_custom async def test_custom_script_extensions_via_provider(self, tmp_path: Path) -> None: """SkillsProvider accepts custom script_extensions.""" skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() + scripts_dir = skill_dir / "scripts" + scripts_dir.mkdir(parents=True) (skill_dir / "SKILL.md").write_text( "---\nname: my-skill\ndescription: test\n---\nBody", encoding="utf-8", ) - (skill_dir / "analyze.py").write_text("print('hi')", encoding="utf-8") - (skill_dir / "run.sh").write_text("#!/bin/bash", encoding="utf-8") + (scripts_dir / "analyze.py").write_text("print('hi')", encoding="utf-8") + (scripts_dir / "run.sh").write_text("#!/bin/bash", encoding="utf-8") # Only discover .sh scripts provider = SkillsProvider.from_paths( @@ -3413,20 +3718,21 @@ class TestCustomScriptExtensions: await _init_provider(provider) skill = _ctx(provider)[0]["my-skill"] script_names = [s.name for s in skill.scripts] - assert "run.sh" in script_names - assert "analyze.py" not in script_names + assert "scripts/run.sh" in script_names + assert "scripts/analyze.py" not in script_names async def test_multiple_script_extensions(self, tmp_path: Path) -> None: """Multiple script extensions can be specified.""" skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() + scripts_dir = skill_dir / "scripts" + scripts_dir.mkdir(parents=True) (skill_dir / "SKILL.md").write_text( "---\nname: my-skill\ndescription: test\n---\nBody", encoding="utf-8", ) - (skill_dir / "analyze.py").write_text("print('hi')", encoding="utf-8") - (skill_dir / "run.sh").write_text("#!/bin/bash", encoding="utf-8") - (skill_dir / "notes.txt").write_text("notes", encoding="utf-8") + (scripts_dir / "analyze.py").write_text("print('hi')", encoding="utf-8") + (scripts_dir / "run.sh").write_text("#!/bin/bash", encoding="utf-8") + (scripts_dir / "notes.txt").write_text("notes", encoding="utf-8") provider = SkillsProvider.from_paths( str(tmp_path), @@ -3436,9 +3742,9 @@ class TestCustomScriptExtensions: await _init_provider(provider) skill = _ctx(provider)[0]["my-skill"] script_names = [s.name for s in skill.scripts] - assert "analyze.py" in script_names - assert "run.sh" in script_names - assert "notes.txt" not in script_names + assert "scripts/analyze.py" in script_names + assert "scripts/run.sh" in script_names + assert "scripts/notes.txt" not in script_names def test_default_script_extensions_unchanged(self) -> None: """DEFAULT_SCRIPT_EXTENSIONS contains only .py.""" @@ -4597,21 +4903,22 @@ class TestSkillsSource: async def test_file_skills_source_with_extensions(self, tmp_path: Path) -> None: """FileSkillsSource resource_extensions controls extension filtering.""" skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() + refs = skill_dir / "references" + refs.mkdir(parents=True) (skill_dir / "SKILL.md").write_text( "---\nname: my-skill\ndescription: Test skill.\n---\nBody.", encoding="utf-8", ) - (skill_dir / "data.json").write_text("{}", encoding="utf-8") - (skill_dir / "data.csv").write_text("a,b", encoding="utf-8") + (refs / "data.json").write_text("{}", encoding="utf-8") + (refs / "data.csv").write_text("a,b", encoding="utf-8") # Only allow .json resources source = FileSkillsSource(str(tmp_path), resource_extensions=(".json",)) skills = await source.get_skills() assert len(skills) == 1 resource_names = [r.name for r in skills[0].resources] - assert "data.json" in resource_names - assert "data.csv" not in resource_names + assert "references/data.json" in resource_names + assert "references/data.csv" not in resource_names async def test_in_memory_skills_source_returns_all_skills(self) -> None: """InMemorySkillsSource returns all provided skills.""" @@ -4840,12 +5147,13 @@ class TestSourceComposition: async def test_file_source_with_script_runner(self, tmp_path: Path) -> None: """FileSkillsSource with script_runner enables script execution.""" skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() + scripts_dir = skill_dir / "scripts" + scripts_dir.mkdir(parents=True) (skill_dir / "SKILL.md").write_text( "---\nname: my-skill\ndescription: test\n---\nBody", encoding="utf-8", ) - (skill_dir / "run.py").write_text("print('hi')", encoding="utf-8") + (scripts_dir / "run.py").write_text("print('hi')", encoding="utf-8") source = DeduplicatingSkillsSource(FileSkillsSource(str(tmp_path), script_runner=_noop_script_runner)) provider = SkillsProvider(source) @@ -4875,12 +5183,13 @@ class TestSourceComposition: async def test_per_source_runner(self, tmp_path: Path) -> None: """Per-source script runner is used when set on FileSkillsSource.""" skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() + scripts_dir = skill_dir / "scripts" + scripts_dir.mkdir(parents=True) (skill_dir / "SKILL.md").write_text( "---\nname: my-skill\ndescription: test\n---\nBody", encoding="utf-8", ) - (skill_dir / "run.py").write_text("print('hi')", encoding="utf-8") + (scripts_dir / "run.py").write_text("print('hi')", encoding="utf-8") call_log: list[str] = [] @@ -4894,7 +5203,7 @@ class TestSourceComposition: # The source-level runner should be discovered and used run_tool = next(t for t in _ctx(provider)[2] if hasattr(t, "name") and t.name == "run_skill_script") - result = await run_tool.func(skill_name="my-skill", script_name="run.py") + result = await run_tool.func(skill_name="my-skill", script_name="scripts/run.py") assert result == "source" assert call_log == ["source"] From 4e65fabafcda7eb3207b1150e067fb74592dc2a6 Mon Sep 17 00:00:00 2001 From: westey <164392973+westey-m@users.noreply.github.com> Date: Thu, 14 May 2026 12:09:01 +0100 Subject: [PATCH 6/8] .NET: Filestore improvements (#5842) * Filestore improvements * Address PR comments --- .../FileStore/FileSystemAgentFileStore.cs | 55 +- .../FileSystemAgentFileStoreTests.cs | 471 ++++++++++++++++++ 2 files changed, 525 insertions(+), 1 deletion(-) diff --git a/dotnet/src/Microsoft.Agents.AI/Harness/FileStore/FileSystemAgentFileStore.cs b/dotnet/src/Microsoft.Agents.AI/Harness/FileStore/FileSystemAgentFileStore.cs index b26d0eea5d..a704c9c9e1 100644 --- a/dotnet/src/Microsoft.Agents.AI/Harness/FileStore/FileSystemAgentFileStore.cs +++ b/dotnet/src/Microsoft.Agents.AI/Harness/FileStore/FileSystemAgentFileStore.cs @@ -122,6 +122,7 @@ public sealed class FileSystemAgentFileStore : AgentFileStore } var files = Directory.GetFiles(fullDir) + .Where(f => (File.GetAttributes(f) & FileAttributes.ReparsePoint) == 0) .Select(Path.GetFileName) .Where(name => name is not null) .ToList(); @@ -157,6 +158,12 @@ public sealed class FileSystemAgentFileStore : AgentFileStore foreach (string filePath in Directory.GetFiles(fullDir)) { + // Skip files that are symlinks/reparse points to prevent reading outside the root. + if ((File.GetAttributes(filePath) & FileAttributes.ReparsePoint) != 0) + { + continue; + } + string? fileName = Path.GetFileName(filePath); if (fileName is null) { @@ -231,7 +238,7 @@ public sealed class FileSystemAgentFileStore : AgentFileStore /// /// Resolves a relative file path to a safe absolute path under the root directory. - /// Rejects paths that would escape the root via traversal or rooted paths. + /// Rejects paths that would escape the root via traversal, rooted paths, or symbolic links. /// private string ResolveSafePath(string relativePath) { @@ -250,9 +257,55 @@ public sealed class FileSystemAgentFileStore : AgentFileStore nameof(relativePath)); } + // Reject symlinks/reparse points in any path segment to prevent escaping the root. + ThrowIfContainsSymlink(fullPath, this._rootPath); + return fullPath; } + /// + /// Checks each path segment between the trusted root and the resolved path for symbolic links + /// or reparse points. Throws if any segment is a symlink. + /// Stops checking at the first segment that does not exist on disk (for write scenarios). + /// Uses directly so that dangling symlinks (whose targets + /// do not exist) are still detected via their flag. + /// + private static void ThrowIfContainsSymlink(string fullPath, string rootPath) + { + string rootTrimmed = rootPath.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); + string relative = fullPath.Substring(rootTrimmed.Length); + string[] segments = relative.Split( + [Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar], + StringSplitOptions.RemoveEmptyEntries); + + string current = rootTrimmed; + foreach (string segment in segments) + { + current = Path.Combine(current, segment); + + FileAttributes attributes; + try + { + attributes = File.GetAttributes(current); + } + catch (FileNotFoundException) + { + // Segment does not exist on disk (write scenario); stop checking. + break; + } + catch (DirectoryNotFoundException) + { + break; + } + + if ((attributes & FileAttributes.ReparsePoint) != 0) + { + throw new ArgumentException( + "Invalid path: the resolved path contains a symbolic link or reparse point."); + } + } + } + /// /// Resolves a relative directory path to a safe absolute path under the root directory. /// An empty string resolves to the root directory itself. diff --git a/dotnet/tests/Microsoft.Agents.AI.UnitTests/Harness/FileStore/FileSystemAgentFileStoreTests.cs b/dotnet/tests/Microsoft.Agents.AI.UnitTests/Harness/FileStore/FileSystemAgentFileStoreTests.cs index 82341d9a47..32342f177a 100644 --- a/dotnet/tests/Microsoft.Agents.AI.UnitTests/Harness/FileStore/FileSystemAgentFileStoreTests.cs +++ b/dotnet/tests/Microsoft.Agents.AI.UnitTests/Harness/FileStore/FileSystemAgentFileStoreTests.cs @@ -334,4 +334,475 @@ public sealed class FileSystemAgentFileStoreTests : IDisposable } #endregion + + #region Symlink Escape Rejection + +#if NET + /// + /// Attempts to create a file symlink. Returns false if the platform does not support + /// symlink creation (e.g., Windows without developer mode) or if creation fails. + /// + private static bool TryCreateFileSymbolicLink(string linkPath, string targetPath) + { + try + { + File.CreateSymbolicLink(linkPath, targetPath); + } + catch (IOException) + { + return false; + } + + // Verify the symlink was actually created as a reparse point. + return File.Exists(linkPath) + && (File.GetAttributes(linkPath) & FileAttributes.ReparsePoint) != 0; + } + + /// + /// Attempts to create a directory symlink. Returns false if the platform does not support + /// symlink creation (e.g., Windows without developer mode) or if creation fails. + /// + private static bool TryCreateDirectorySymbolicLink(string linkPath, string targetPath) + { + try + { + Directory.CreateSymbolicLink(linkPath, targetPath); + } + catch (IOException) + { + return false; + } + + // Verify the symlink was actually created as a reparse point. + return Directory.Exists(linkPath) + && (File.GetAttributes(linkPath) & FileAttributes.ReparsePoint) != 0; + } + + [Fact] + public async Task ReadFileAsync_SymlinkedFile_ThrowsAsync() + { + // Arrange — create a file outside the root and symlink to it from inside. + string outsideFile = Path.Combine(Path.GetTempPath(), "symlink_target_read_" + Guid.NewGuid().ToString("N") + ".txt"); + File.WriteAllText(outsideFile, "SECRET_OUTSIDE_ROOT"); + + string linkPath = Path.Combine(this._rootDir, "leak.txt"); + + try + { + if (!TryCreateFileSymbolicLink(linkPath, outsideFile)) + { + return; // Cannot create symlinks in this environment; skip. + } + + // Act & Assert — reading through the symlink should be rejected. + await Assert.ThrowsAsync(() => this._store.ReadFileAsync("leak.txt")); + } + finally + { + if (File.Exists(linkPath)) + { + File.Delete(linkPath); + } + + File.Delete(outsideFile); + } + } + + [Fact] + public async Task WriteFileAsync_SymlinkedFile_ThrowsAsync() + { + // Arrange — create a file outside the root and symlink to it from inside. + string outsideFile = Path.Combine(Path.GetTempPath(), "symlink_target_write_" + Guid.NewGuid().ToString("N") + ".txt"); + File.WriteAllText(outsideFile, "ORIGINAL_CONTENT"); + + string linkPath = Path.Combine(this._rootDir, "overwrite.txt"); + + try + { + if (!TryCreateFileSymbolicLink(linkPath, outsideFile)) + { + return; + } + + // Act & Assert — writing through the symlink should be rejected. + await Assert.ThrowsAsync(() => this._store.WriteFileAsync("overwrite.txt", "EVIL_CONTENT")); + + // Verify the outside file was NOT modified. + Assert.Equal("ORIGINAL_CONTENT", await File.ReadAllTextAsync(outsideFile)); + } + finally + { + if (File.Exists(linkPath)) + { + File.Delete(linkPath); + } + + File.Delete(outsideFile); + } + } + + [Fact] + public async Task DeleteFileAsync_SymlinkedFile_ThrowsAsync() + { + // Arrange + string outsideFile = Path.Combine(Path.GetTempPath(), "symlink_target_delete_" + Guid.NewGuid().ToString("N") + ".txt"); + File.WriteAllText(outsideFile, "DO_NOT_DELETE"); + + string linkPath = Path.Combine(this._rootDir, "trap.txt"); + + try + { + if (!TryCreateFileSymbolicLink(linkPath, outsideFile)) + { + return; + } + + // Act & Assert + await Assert.ThrowsAsync(() => this._store.DeleteFileAsync("trap.txt")); + + // Verify the outside file still exists. + Assert.True(File.Exists(outsideFile)); + } + finally + { + if (File.Exists(linkPath)) + { + File.Delete(linkPath); + } + + File.Delete(outsideFile); + } + } + + [Fact] + public async Task FileExistsAsync_SymlinkedFile_ThrowsAsync() + { + // Arrange + string outsideFile = Path.Combine(Path.GetTempPath(), "symlink_target_exists_" + Guid.NewGuid().ToString("N") + ".txt"); + File.WriteAllText(outsideFile, "EXISTS_OUTSIDE"); + + string linkPath = Path.Combine(this._rootDir, "phantom.txt"); + + try + { + if (!TryCreateFileSymbolicLink(linkPath, outsideFile)) + { + return; + } + + // Act & Assert + await Assert.ThrowsAsync(() => this._store.FileExistsAsync("phantom.txt")); + } + finally + { + if (File.Exists(linkPath)) + { + File.Delete(linkPath); + } + + File.Delete(outsideFile); + } + } + + [Fact] + public async Task WriteFileAsync_DanglingSymlink_ThrowsAsync() + { + // Arrange — create a symlink pointing to a non-existent target. + string nonExistentTarget = Path.Combine(Path.GetTempPath(), "dangling_target_" + Guid.NewGuid().ToString("N") + ".txt"); + string linkPath = Path.Combine(this._rootDir, "dangling.txt"); + + try + { + if (!TryCreateFileSymbolicLink(linkPath, nonExistentTarget)) + { + return; + } + + // Act & Assert — even a dangling symlink must be rejected. + await Assert.ThrowsAsync(() => this._store.WriteFileAsync("dangling.txt", "CONTENT")); + + // Verify the target was NOT created by following the dangling link. + Assert.False(File.Exists(nonExistentTarget)); + } + finally + { + // Dangling symlinks: File.Exists returns false, but the link entry still exists. + // Use FileInfo to delete the link itself. + var linkInfo = new FileInfo(linkPath); + if (linkInfo.Exists || (linkInfo.Attributes & FileAttributes.ReparsePoint) != 0) + { + linkInfo.Delete(); + } + } + } + + [Fact] + public async Task ListFilesAsync_SymlinkedDirectory_ThrowsAsync() + { + // Arrange — create a directory outside root and symlink a directory inside root to it. + string outsideDir = Path.Combine(Path.GetTempPath(), "symlink_dir_target_" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(outsideDir); + File.WriteAllText(Path.Combine(outsideDir, "secret.txt"), "SECRET"); + + string linkDir = Path.Combine(this._rootDir, "linked-dir"); + + try + { + if (!TryCreateDirectorySymbolicLink(linkDir, outsideDir)) + { + return; + } + + // Act & Assert + await Assert.ThrowsAsync(() => this._store.ListFilesAsync("linked-dir")); + } + finally + { + if (Directory.Exists(linkDir)) + { + Directory.Delete(linkDir); + } + + Directory.Delete(outsideDir, recursive: true); + } + } + + [Fact] + public async Task SearchFilesAsync_SymlinkedDirectory_ThrowsAsync() + { + // Arrange + string outsideDir = Path.Combine(Path.GetTempPath(), "symlink_search_target_" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(outsideDir); + File.WriteAllText(Path.Combine(outsideDir, "data.txt"), "SENSITIVE_DATA"); + + string linkDir = Path.Combine(this._rootDir, "search-link"); + + try + { + if (!TryCreateDirectorySymbolicLink(linkDir, outsideDir)) + { + return; + } + + // Act & Assert + await Assert.ThrowsAsync(() => this._store.SearchFilesAsync("search-link", "SENSITIVE")); + } + finally + { + if (Directory.Exists(linkDir)) + { + Directory.Delete(linkDir); + } + + Directory.Delete(outsideDir, recursive: true); + } + } + + [Fact] + public async Task ReadFileAsync_ThroughDirectorySymlink_ThrowsAsync() + { + // Arrange — directory symlink inside root pointing outside; read a file through it. + string outsideDir = Path.Combine(Path.GetTempPath(), "symlink_dir_read_" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(outsideDir); + File.WriteAllText(Path.Combine(outsideDir, "secret.txt"), "DIR_SYMLINK_SECRET"); + + string linkDir = Path.Combine(this._rootDir, "linked-output"); + + try + { + if (!TryCreateDirectorySymbolicLink(linkDir, outsideDir)) + { + return; + } + + // Act & Assert — reading through a directory symlink should be rejected. + await Assert.ThrowsAsync(() => this._store.ReadFileAsync("linked-output/secret.txt")); + } + finally + { + if (Directory.Exists(linkDir)) + { + Directory.Delete(linkDir); + } + + Directory.Delete(outsideDir, recursive: true); + } + } + + [Fact] + public async Task WriteFileAsync_ThroughDirectorySymlink_ThrowsAsync() + { + // Arrange — directory symlink; attempt to create/overwrite a file through it. + string outsideDir = Path.Combine(Path.GetTempPath(), "symlink_dir_write_" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(outsideDir); + + string linkDir = Path.Combine(this._rootDir, "linked-output"); + + try + { + if (!TryCreateDirectorySymbolicLink(linkDir, outsideDir)) + { + return; + } + + // Act & Assert + await Assert.ThrowsAsync(() => this._store.WriteFileAsync("linked-output/created-by-agent.txt", "CONTENT")); + + // Verify no file was created outside. + Assert.False(File.Exists(Path.Combine(outsideDir, "created-by-agent.txt"))); + } + finally + { + if (Directory.Exists(linkDir)) + { + Directory.Delete(linkDir); + } + + Directory.Delete(outsideDir, recursive: true); + } + } + + [Fact] + public async Task DeleteFileAsync_ThroughDirectorySymlink_ThrowsAsync() + { + // Arrange — directory symlink; attempt to delete a file through it. + string outsideDir = Path.Combine(Path.GetTempPath(), "symlink_dir_delete_" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(outsideDir); + string outsideFile = Path.Combine(outsideDir, "delete-me.txt"); + File.WriteAllText(outsideFile, "DO_NOT_DELETE"); + + string linkDir = Path.Combine(this._rootDir, "linked-output"); + + try + { + if (!TryCreateDirectorySymbolicLink(linkDir, outsideDir)) + { + return; + } + + // Act & Assert + await Assert.ThrowsAsync(() => this._store.DeleteFileAsync("linked-output/delete-me.txt")); + + // Verify the outside file was NOT deleted. + Assert.True(File.Exists(outsideFile)); + } + finally + { + if (Directory.Exists(linkDir)) + { + Directory.Delete(linkDir); + } + + Directory.Delete(outsideDir, recursive: true); + } + } + + [Fact] + public async Task CreateDirectoryAsync_ThroughDirectorySymlink_ThrowsAsync() + { + // Arrange — directory symlink; attempt to create a subdirectory through it. + string outsideDir = Path.Combine(Path.GetTempPath(), "symlink_dir_mkdir_" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(outsideDir); + + string linkDir = Path.Combine(this._rootDir, "linked-output"); + + try + { + if (!TryCreateDirectorySymbolicLink(linkDir, outsideDir)) + { + return; + } + + // Act & Assert + await Assert.ThrowsAsync(() => this._store.CreateDirectoryAsync("linked-output/created-directory")); + + // Verify no directory was created outside. + Assert.False(Directory.Exists(Path.Combine(outsideDir, "created-directory"))); + } + finally + { + if (Directory.Exists(linkDir)) + { + Directory.Delete(linkDir); + } + + Directory.Delete(outsideDir, recursive: true); + } + } + + [Fact] + public async Task SearchFilesAsync_RootWithSymlinkedFile_DoesNotLeakContentAsync() + { + // Arrange — symlinked file at root level; search should not return its content. + string outsideFile = Path.Combine(Path.GetTempPath(), "symlink_search_root_" + Guid.NewGuid().ToString("N") + ".txt"); + File.WriteAllText(outsideFile, "ROOT_LEVEL_SECRET_CONTENT"); + + string linkPath = Path.Combine(this._rootDir, "env-link.txt"); + + try + { + if (!TryCreateFileSymbolicLink(linkPath, outsideFile)) + { + return; + } + + // Also add a normal file to confirm search still works for non-symlinks. + await this._store.WriteFileAsync("normal.txt", "NORMAL_CONTENT"); + + // Act — search at root should skip the symlinked file. + var results = await this._store.SearchFilesAsync("", "SECRET_CONTENT"); + + // Assert — no results from the symlinked file. + Assert.Empty(results); + } + finally + { + if (File.Exists(linkPath)) + { + File.Delete(linkPath); + } + + File.Delete(outsideFile); + } + } + + [Fact] + public async Task ListFilesAsync_RootWithSymlinkedFile_ExcludesSymlinkAsync() + { + // Arrange — symlinked file at root level; listing should not include it. + string outsideFile = Path.Combine(Path.GetTempPath(), "symlink_list_root_" + Guid.NewGuid().ToString("N") + ".txt"); + File.WriteAllText(outsideFile, "OUTSIDE"); + + string linkPath = Path.Combine(this._rootDir, "hidden-link.txt"); + + try + { + if (!TryCreateFileSymbolicLink(linkPath, outsideFile)) + { + return; + } + + // Also add a normal file. + await this._store.WriteFileAsync("visible.txt", "VISIBLE"); + + // Act + var files = await this._store.ListFilesAsync(""); + + // Assert — symlinked file should not appear in listing. + Assert.DoesNotContain("hidden-link.txt", files); + Assert.Contains("visible.txt", files); + } + finally + { + if (File.Exists(linkPath)) + { + File.Delete(linkPath); + } + + File.Delete(outsideFile); + } + } +#endif + + #endregion } From 198761d3ba00a59e6e86f6f6b1f4c582fb7f76f8 Mon Sep 17 00:00:00 2001 From: Roger Barreto <19890735+rogerbarreto@users.noreply.github.com> Date: Thu, 14 May 2026 12:51:34 +0100 Subject: [PATCH 7/8] .NET: DevUI: quarantine flaky discovery integration test (#5845) (#5846) TestServerWithDevUI_ResolvesMixedAgentsAndWorkflows_AllRegistrationsAsync fails intermittently in the merge_group with NRE on the discovery response, blocking PRs unrelated to DevUI from merging. Skip via Fact(Skip=...) referencing #5845 while the underlying race is investigated. --- .../DevUIIntegrationTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dotnet/tests/Microsoft.Agents.AI.DevUI.UnitTests/DevUIIntegrationTests.cs b/dotnet/tests/Microsoft.Agents.AI.DevUI.UnitTests/DevUIIntegrationTests.cs index 901058ee29..029a650785 100644 --- a/dotnet/tests/Microsoft.Agents.AI.DevUI.UnitTests/DevUIIntegrationTests.cs +++ b/dotnet/tests/Microsoft.Agents.AI.DevUI.UnitTests/DevUIIntegrationTests.cs @@ -218,7 +218,7 @@ public class DevUIIntegrationTests Assert.Contains(discoveryResponse.Entities, e => e.Name == "default-workflow" && e.Type == "workflow"); } - [Fact] + [Fact(Skip = "Flaky in merge_group; see https://github.com/microsoft/agent-framework/issues/5845")] public async Task TestServerWithDevUI_ResolvesMixedAgentsAndWorkflows_AllRegistrationsAsync() { // Arrange From 2d83a9b10d30ac8f7ce91d5dc221efaf918397ae Mon Sep 17 00:00:00 2001 From: westey <164392973+westey-m@users.noreply.github.com> Date: Thu, 14 May 2026 12:55:04 +0100 Subject: [PATCH 8/8] Update version to 1.6.1 for release (#5843) --- dotnet/nuget/nuget-package.props | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dotnet/nuget/nuget-package.props b/dotnet/nuget/nuget-package.props index cb1713e5e9..f1377833a2 100644 --- a/dotnet/nuget/nuget-package.props +++ b/dotnet/nuget/nuget-package.props @@ -1,14 +1,14 @@ - 1.6.0 + 1.6.1 1 - 260512 + 260514 $(VersionPrefix)-rc$(RCNumber) $(VersionPrefix)-$(VersionSuffix).$(DateSuffix).1 $(VersionPrefix)-preview.$(DateSuffix).1 $(VersionPrefix) - 1.6.0 + 1.6.1 Debug;Release;Publish true