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 1/5] [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 2/5] .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 3/5] .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 4/5] 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 From eb4053543659fdc53c584e5e0f6eb2b60a75b81e Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Thu, 14 May 2026 09:36:38 -0400 Subject: [PATCH 5/5] .NET: Add Executor RouteBuilder Unit Tests (#5824) * Add RouteBuilder unit tests Agent-Logs-Url: https://github.com/microsoft/agent-framework/sessions/012f3b3b-acb9-4869-9084-b767cbe1885b Co-authored-by: lokitoth <6936551+lokitoth@users.noreply.github.com> * Address RouteBuilder test review feedback Agent-Logs-Url: https://github.com/microsoft/agent-framework/sessions/012f3b3b-acb9-4869-9084-b767cbe1885b Co-authored-by: lokitoth <6936551+lokitoth@users.noreply.github.com> * Fix RouteBuilder test nullability warning Agent-Logs-Url: https://github.com/microsoft/agent-framework/sessions/012f3b3b-acb9-4869-9084-b767cbe1885b Co-authored-by: lokitoth <6936551+lokitoth@users.noreply.github.com> * Refine RouteBuilder test helpers Agent-Logs-Url: https://github.com/microsoft/agent-framework/sessions/012f3b3b-acb9-4869-9084-b767cbe1885b Co-authored-by: lokitoth <6936551+lokitoth@users.noreply.github.com> * Refactor overload int constants to HandlerOverload enum Agent-Logs-Url: https://github.com/microsoft/agent-framework/sessions/19397f58-a88a-41cf-bd85-588f520e0d0f Co-authored-by: lokitoth <6936551+lokitoth@users.noreply.github.com> * Fix ValueTask compatibility with .NET Framework 4.7.2 Agent-Logs-Url: https://github.com/microsoft/agent-framework/sessions/a8437809-0898-43a6-a950-09eb3417f58a Co-authored-by: lokitoth <6936551+lokitoth@users.noreply.github.com> * Fix IDE0001 format errors - simplify generic type names Agent-Logs-Url: https://github.com/microsoft/agent-framework/sessions/8573214e-ec42-4969-ba94-76bdc8ad3e59 Co-authored-by: lokitoth <6936551+lokitoth@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: lokitoth <6936551+lokitoth@users.noreply.github.com> Co-authored-by: Jacob Alber --- .../RouteBuilderTests.cs | 546 ++++++++++++++++++ 1 file changed, 546 insertions(+) create mode 100644 dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/RouteBuilderTests.cs diff --git a/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/RouteBuilderTests.cs b/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/RouteBuilderTests.cs new file mode 100644 index 0000000000..a734b82b66 --- /dev/null +++ b/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/RouteBuilderTests.cs @@ -0,0 +1,546 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using FluentAssertions; +using Microsoft.Agents.AI.Workflows.Execution; + +namespace Microsoft.Agents.AI.Workflows.UnitTests; + +public sealed class RouteBuilderTests +{ + public enum HandlerOverload + { + SyncWithCancellation = 0, + SyncWithoutCancellation = 1, + AsyncWithCancellation = 2, + AsyncWithoutCancellation = 3, + } + + private sealed record TestPayload(string Value); + + private sealed class HandlerInvocation + { + public object? Message { get; private set; } + + public IWorkflowContext? Context { get; private set; } + + public CancellationToken CancellationToken { get; private set; } + + public int InvocationCount { get; private set; } + + public void Capture(object? message, IWorkflowContext context, CancellationToken cancellationToken = default) + { + this.Message = message; + this.Context = context; + this.CancellationToken = cancellationToken; + this.InvocationCount++; + } + } + + private sealed class TestExternalRequestContext : IExternalRequestContext, IExternalRequestSink + { + public List RegisteredPorts { get; } = []; + + public List PostedRequests { get; } = []; + + public IExternalRequestSink RegisterPort(RequestPort port) + { + this.RegisteredPorts.Add(port); + return this; + } + + public ValueTask PostAsync(ExternalRequest request) + { + this.PostedRequests.Add(request); + return default; + } + } + + [Theory] + [InlineData(HandlerOverload.SyncWithCancellation)] + [InlineData(HandlerOverload.SyncWithoutCancellation)] + [InlineData(HandlerOverload.AsyncWithCancellation)] + [InlineData(HandlerOverload.AsyncWithoutCancellation)] + public async Task AddHandler_VoidOverloads_RouteExpectedMessageAsync(HandlerOverload overload) + { + // Arrange + RouteBuilder routeBuilder = new(null); + HandlerInvocation invocation = new(); + CancellationToken cancellationToken = new CancellationTokenSource().Token; + RegisterVoidHandler(routeBuilder, invocation, overload); + MessageRouter router = routeBuilder.Build(); + TestWorkflowContext context = new("executor"); + + // Act + CallResult? result = await router.RouteMessageAsync("hello", context, cancellationToken: cancellationToken); + + // Assert + result.Should().NotBeNull(); + result!.IsSuccess.Should().BeTrue(); + result.IsVoid.Should().BeTrue(); + result.Result.Should().BeNull(); + invocation.InvocationCount.Should().Be(1); + invocation.Message.Should().Be("hello"); + invocation.Context.Should().BeSameAs(context); + + if (UsesCancellationToken(overload)) + { + invocation.CancellationToken.Should().Be(cancellationToken); + } + } + + [Theory] + [InlineData(HandlerOverload.SyncWithCancellation)] + [InlineData(HandlerOverload.SyncWithoutCancellation)] + [InlineData(HandlerOverload.AsyncWithCancellation)] + [InlineData(HandlerOverload.AsyncWithoutCancellation)] + public async Task AddHandler_ResultOverloads_RouteExpectedMessageAsync(HandlerOverload overload) + { + // Arrange + RouteBuilder routeBuilder = new(null); + HandlerInvocation invocation = new(); + CancellationToken cancellationToken = new CancellationTokenSource().Token; + RegisterResultHandler(routeBuilder, invocation, overload); + MessageRouter router = routeBuilder.Build(); + TestWorkflowContext context = new("executor"); + + // Act + CallResult? result = await router.RouteMessageAsync("hello", context, cancellationToken: cancellationToken); + + // Assert + result.Should().NotBeNull(); + result!.IsSuccess.Should().BeTrue(); + result.IsVoid.Should().BeFalse(); + result.Result.Should().Be("HELLO"); + router.DefaultOutputTypes.Should().Contain(typeof(string)); + invocation.InvocationCount.Should().Be(1); + invocation.Message.Should().Be("hello"); + invocation.Context.Should().BeSameAs(context); + + if (UsesCancellationToken(overload)) + { + invocation.CancellationToken.Should().Be(cancellationToken); + } + } + + [Theory] + [InlineData(HandlerOverload.SyncWithCancellation)] + [InlineData(HandlerOverload.SyncWithoutCancellation)] + [InlineData(HandlerOverload.AsyncWithCancellation)] + [InlineData(HandlerOverload.AsyncWithoutCancellation)] + public async Task AddCatchAll_VoidOverloads_RouteUnexpectedMessageAsync(HandlerOverload overload) + { + // Arrange + RouteBuilder routeBuilder = new(null); + HandlerInvocation invocation = new(); + CancellationToken cancellationToken = new CancellationTokenSource().Token; + TestPayload payload = new("hello"); + RegisterVoidCatchAll(routeBuilder, invocation, overload); + MessageRouter router = routeBuilder.Build(); + TestWorkflowContext context = new("executor"); + + // Act + CallResult? result = await router.RouteMessageAsync(payload, context, cancellationToken: cancellationToken); + + // Assert + result.Should().NotBeNull(); + result!.IsSuccess.Should().BeTrue(); + result.IsVoid.Should().BeTrue(); + result.Result.Should().BeNull(); + invocation.InvocationCount.Should().Be(1); + invocation.Message.Should().BeEquivalentTo(new PortableValue(payload)); + invocation.Context.Should().BeSameAs(context); + + if (UsesCancellationToken(overload)) + { + invocation.CancellationToken.Should().Be(cancellationToken); + } + } + + [Theory] + [InlineData(HandlerOverload.SyncWithCancellation)] + [InlineData(HandlerOverload.SyncWithoutCancellation)] + [InlineData(HandlerOverload.AsyncWithCancellation)] + [InlineData(HandlerOverload.AsyncWithoutCancellation)] + public async Task AddCatchAll_ResultOverloads_RouteUnexpectedMessageAsync(HandlerOverload overload) + { + // Arrange + RouteBuilder routeBuilder = new(null); + HandlerInvocation invocation = new(); + CancellationToken cancellationToken = new CancellationTokenSource().Token; + TestPayload payload = new("hello"); + RegisterResultCatchAll(routeBuilder, invocation, overload); + MessageRouter router = routeBuilder.Build(); + TestWorkflowContext context = new("executor"); + + // Act + CallResult? result = await router.RouteMessageAsync(payload, context, cancellationToken: cancellationToken); + + // Assert + result.Should().NotBeNull(); + result!.IsSuccess.Should().BeTrue(); + result.IsVoid.Should().BeFalse(); + result.Result.Should().Be("HELLO"); + invocation.InvocationCount.Should().Be(1); + invocation.Message.Should().BeEquivalentTo(new PortableValue(payload)); + invocation.Context.Should().BeSameAs(context); + + if (UsesCancellationToken(overload)) + { + invocation.CancellationToken.Should().Be(cancellationToken); + } + } + + [Fact] + public async Task AddHandlerUntyped_VoidAndResultOverloads_RouteExpectedMessageAsync() + { + // Arrange + RouteBuilder routeBuilder = new(null); + HandlerInvocation voidInvocation = new(); + HandlerInvocation resultInvocation = new(); + CancellationToken cancellationToken = new CancellationTokenSource().Token; + routeBuilder.AddHandlerUntyped(typeof(string), (message, context, token) => + { + voidInvocation.Capture(message, context, token); + return default; + }); + routeBuilder.AddHandlerUntyped(typeof(int), (message, context, token) => + { + resultInvocation.Capture(message, context, token); + return new((int)message + 1); + }); + MessageRouter router = routeBuilder.Build(); + TestWorkflowContext context = new("executor"); + + // Act + CallResult? voidResult = await router.RouteMessageAsync("hello", context, cancellationToken: cancellationToken); + CallResult? typedResult = await router.RouteMessageAsync(41, context, cancellationToken: cancellationToken); + + // Assert + voidResult.Should().NotBeNull(); + voidResult!.IsVoid.Should().BeTrue(); + voidInvocation.Message.Should().Be("hello"); + voidInvocation.Context.Should().BeSameAs(context); + voidInvocation.CancellationToken.Should().Be(cancellationToken); + + typedResult.Should().NotBeNull(); + typedResult!.Result.Should().Be(42); + router.DefaultOutputTypes.Should().Contain(typeof(int)); + resultInvocation.Message.Should().Be(41); + resultInvocation.Context.Should().BeSameAs(context); + resultInvocation.CancellationToken.Should().Be(cancellationToken); + } + + [Fact] + public void AddHandler_ForPortableValue_ThrowsInvalidOperationException() + { + // Arrange + RouteBuilder routeBuilder = new(null); + + // Act + Action act = () => routeBuilder.AddHandler((message, context) => { }); + + // Assert + act.Should().Throw() + .WithMessage("*Use AddCatchAll()*"); + } + + [Fact] + public void AddHandler_DuplicateRegistrationWithoutOverwrite_ThrowsArgumentException() + { + // Arrange + RouteBuilder routeBuilder = new(null); + routeBuilder.AddHandler((message, context) => { }); + + // Act + Action act = () => routeBuilder.AddHandler((message, context) => { }); + + // Assert + act.Should().Throw() + .WithMessage("*already registered*"); + } + + [Fact] + public void AddHandler_OverwriteWithoutExistingRegistration_ThrowsArgumentException() + { + // Arrange + RouteBuilder routeBuilder = new(null); + + // Act + Action act = () => routeBuilder.AddHandler((message, context) => { }, overwrite: true); + + // Assert + act.Should().Throw() + .WithMessage("*has not yet been registered*"); + } + + [Fact] + public async Task AddHandler_OverwriteExistingRegistration_RoutesUpdatedHandlerAsync() + { + // Arrange + RouteBuilder routeBuilder = new(null); + routeBuilder.AddHandler((message, context) => context.SendMessageAsync("first")); + routeBuilder.AddHandler((message, context) => context.SendMessageAsync("second"), overwrite: true); + MessageRouter router = routeBuilder.Build(); + TestWorkflowContext context = new("executor"); + + // Act + _ = await router.RouteMessageAsync("hello", context); + + // Assert + context.SentMessages.Should().ContainSingle().Which.Should().Be("second"); + } + + [Fact] + public void AddCatchAll_DuplicateRegistrationWithoutOverwrite_ThrowsInvalidOperationException() + { + // Arrange + RouteBuilder routeBuilder = new(null); + routeBuilder.AddCatchAll((message, context) => { }); + + // Act + Action act = () => routeBuilder.AddCatchAll((message, context) => { }); + + // Assert + act.Should().Throw() + .WithMessage("*already registered*"); + } + + [Fact] + public async Task AddCatchAll_OverwriteExistingRegistration_RoutesUpdatedHandlerAsync() + { + // Arrange + RouteBuilder routeBuilder = new(null); + routeBuilder.AddCatchAll((message, context) => context.SendMessageAsync("first")); + routeBuilder.AddCatchAll((message, context) => context.SendMessageAsync("second"), overwrite: true); + MessageRouter router = routeBuilder.Build(); + TestWorkflowContext context = new("executor"); + + // Act + _ = await router.RouteMessageAsync(new TestPayload("hello"), context); + + // Assert + context.SentMessages.Should().ContainSingle().Which.Should().Be("second"); + } + + [Fact] + public void AddPortHandler_WithoutExternalRequestContext_ThrowsInvalidOperationException() + { + // Arrange + RouteBuilder routeBuilder = new(null); + + // Act + Action act = () => routeBuilder.AddPortHandler("port", (response, context, cancellationToken) => default, out _); + + // Assert + act.Should().Throw() + .WithMessage("*external request context is required*"); + } + + [Fact] + public async Task AddPortHandler_RoutesMatchingExternalResponseAsync() + { + // Arrange + TestExternalRequestContext externalRequestContext = new(); + RouteBuilder routeBuilder = new(externalRequestContext); + HandlerInvocation invocation = new(); + routeBuilder.AddPortHandler("port", (response, context, cancellationToken) => + { + invocation.Capture(response, context, cancellationToken); + return default; + }, out PortBinding portBinding); + await portBinding.PostRequestAsync("request", requestId: "req-1"); + MessageRouter router = routeBuilder.Build(); + TestWorkflowContext context = new("executor"); + CancellationToken cancellationToken = new CancellationTokenSource().Token; + ExternalResponse response = externalRequestContext.PostedRequests.Single().CreateResponse(42); + + // Act + CallResult? result = await router.RouteMessageAsync(response, context, cancellationToken: cancellationToken); + + // Assert + externalRequestContext.RegisteredPorts.Should().ContainSingle(port => port.Id == "port"); + externalRequestContext.PostedRequests.Should().ContainSingle(request => request.RequestId == "req-1"); + result.Should().NotBeNull(); + result!.IsSuccess.Should().BeTrue(); + result.Result.Should().BeSameAs(response); + invocation.InvocationCount.Should().Be(1); + invocation.Message.Should().Be(42); + invocation.Context.Should().BeSameAs(context); + invocation.CancellationToken.Should().Be(cancellationToken); + } + + [Fact] + public async Task AddPortHandler_UnknownPort_ReturnsExceptionResultAsync() + { + // Arrange + TestExternalRequestContext externalRequestContext = new(); + RouteBuilder routeBuilder = new(externalRequestContext); + routeBuilder.AddPortHandler("port", (response, context, cancellationToken) => default, out _); + MessageRouter router = routeBuilder.Build(); + ExternalRequest request = ExternalRequest.Create(RequestPort.Create("other"), "request", requestId: "req-1"); + + // Act + CallResult? result = await router.RouteMessageAsync(request.CreateResponse(42), new TestWorkflowContext("executor")); + + // Assert + result.Should().NotBeNull(); + result!.IsSuccess.Should().BeFalse(); + result.Exception.Should().BeOfType(); + result.Exception!.Message.Should().Contain("Unknown port"); + } + + private static void RegisterVoidHandler(RouteBuilder routeBuilder, HandlerInvocation invocation, HandlerOverload overload) + { + switch (overload) + { + case HandlerOverload.SyncWithCancellation: + routeBuilder.AddHandler((message, context, cancellationToken) => invocation.Capture(message, context, cancellationToken)); + break; + case HandlerOverload.SyncWithoutCancellation: + routeBuilder.AddHandler((message, context) => invocation.Capture(message, context)); + break; + case HandlerOverload.AsyncWithCancellation: + routeBuilder.AddHandler((message, context, cancellationToken) => + { + invocation.Capture(message, context, cancellationToken); + return default; + }); + break; + case HandlerOverload.AsyncWithoutCancellation: + routeBuilder.AddHandler((message, context) => + { + invocation.Capture(message, context); + return default; + }); + break; + default: + throw new ArgumentOutOfRangeException(nameof(overload)); + } + } + + private static void RegisterResultHandler(RouteBuilder routeBuilder, HandlerInvocation invocation, HandlerOverload overload) + { + switch (overload) + { + case HandlerOverload.SyncWithCancellation: + routeBuilder.AddHandler((message, context, cancellationToken) => + { + invocation.Capture(message, context, cancellationToken); + return NormalizeHandlerResult(message); + }); + break; + case HandlerOverload.SyncWithoutCancellation: + routeBuilder.AddHandler((message, context) => + { + invocation.Capture(message, context); + return NormalizeHandlerResult(message); + }); + break; + case HandlerOverload.AsyncWithCancellation: + Func> asyncHandlerWithCancellation = (message, context, cancellationToken) => + { + invocation.Capture(message, context, cancellationToken); + return new ValueTask(NormalizeHandlerResult(message)); + }; + routeBuilder.AddHandler(asyncHandlerWithCancellation); + break; + case HandlerOverload.AsyncWithoutCancellation: + Func> asyncHandler = (message, context) => + { + invocation.Capture(message, context); + return new ValueTask(NormalizeHandlerResult(message)); + }; + routeBuilder.AddHandler(asyncHandler); + break; + default: + throw new ArgumentOutOfRangeException(nameof(overload)); + } + } + + private static void RegisterVoidCatchAll(RouteBuilder routeBuilder, HandlerInvocation invocation, HandlerOverload overload) + { + switch (overload) + { + case HandlerOverload.SyncWithCancellation: + routeBuilder.AddCatchAll((message, context, cancellationToken) => invocation.Capture(message, context, cancellationToken)); + break; + case HandlerOverload.SyncWithoutCancellation: + routeBuilder.AddCatchAll((message, context) => invocation.Capture(message, context)); + break; + case HandlerOverload.AsyncWithCancellation: + routeBuilder.AddCatchAll((message, context, cancellationToken) => + { + invocation.Capture(message, context, cancellationToken); + return default; + }); + break; + case HandlerOverload.AsyncWithoutCancellation: + routeBuilder.AddCatchAll((message, context) => + { + invocation.Capture(message, context); + return default; + }); + break; + default: + throw new ArgumentOutOfRangeException(nameof(overload)); + } + } + + private static void RegisterResultCatchAll(RouteBuilder routeBuilder, HandlerInvocation invocation, HandlerOverload overload) + { + switch (overload) + { + case HandlerOverload.SyncWithCancellation: + routeBuilder.AddCatchAll((message, context, cancellationToken) => + { + invocation.Capture(message, context, cancellationToken); + return NormalizeCatchAllResult(message); + }); + break; + case HandlerOverload.SyncWithoutCancellation: + routeBuilder.AddCatchAll((message, context) => + { + invocation.Capture(message, context); + return NormalizeCatchAllResult(message); + }); + break; + case HandlerOverload.AsyncWithCancellation: + Func> asyncCatchAllWithCancellation = (message, context, cancellationToken) => + { + invocation.Capture(message, context, cancellationToken); + return new ValueTask(NormalizeCatchAllResult(message)); + }; + routeBuilder.AddCatchAll(asyncCatchAllWithCancellation); + break; + case HandlerOverload.AsyncWithoutCancellation: + Func> asyncCatchAll = (message, context) => + { + invocation.Capture(message, context); + return new ValueTask(NormalizeCatchAllResult(message)); + }; + routeBuilder.AddCatchAll(asyncCatchAll); + break; + default: + throw new ArgumentOutOfRangeException(nameof(overload)); + } + } + + private static bool UsesCancellationToken(HandlerOverload overload) => + overload is HandlerOverload.SyncWithCancellation or HandlerOverload.AsyncWithCancellation; + + private static string NormalizeHandlerResult(string message) => message.ToUpperInvariant(); + + private static string NormalizeCatchAllResult(PortableValue message) => GetPayloadValue(message).ToUpperInvariant(); + + private static string GetPayloadValue(PortableValue message) + { + return message.As() is TestPayload payload + ? payload.Value + : throw new InvalidOperationException("Expected catch-all message payload to deserialize as TestPayload."); + } +}