Python: [Breaking] Restructure agent skills to use multi-source architecture#5584
Open
SergeyMenshykh wants to merge 18 commits intomicrosoft:mainfrom
Open
Python: [Breaking] Restructure agent skills to use multi-source architecture#5584SergeyMenshykh wants to merge 18 commits intomicrosoft:mainfrom
SergeyMenshykh wants to merge 18 commits intomicrosoft:mainfrom
Conversation
Contributor
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Python Agent Skills system to the ADR-0021 multi-source architecture, introducing new skill/source abstractions and updating samples to use the new InlineSkill / FileSkill APIs and the builder-based composition flow.
Changes:
- Refactors core skills models to abstract
Skill/SkillResource/SkillScripttypes, plus concreteInline*andFile*implementations andSkillsSourcecomposition. - Updates
SkillsProviderto accept a composed source (or skill/skill list), addsSkillsProvider.from_paths(), and introducesSkillsProviderBuilder. - Refreshes skills samples/docs to use
InlineSkill,SkillsProviderBuilder, and typed file-script runner signatures.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| python/packages/core/agent_framework/_skills.py | Implements the new multi-source skills architecture, new skill/script/resource types, provider changes, and builder. |
| python/packages/core/agent_framework/init.py | Exposes new public API symbols for the refactored skills system. |
| python/samples/02-agents/skills/subprocess_script_runner.py | Updates subprocess runner typing/behavior for file-based scripts. |
| python/samples/02-agents/skills/script_approval/script_approval.py | Updates sample to InlineSkill and the new provider constructor API. |
| python/samples/02-agents/skills/mixed_skills/mixed_skills.py | Updates sample to compose file + inline skills via SkillsProviderBuilder. |
| python/samples/02-agents/skills/mixed_skills/README.md | Updates documentation diagram to builder-based composition. |
| python/samples/02-agents/skills/file_based_skill/file_based_skill.py | Updates sample to use SkillsProvider.from_paths(). |
| python/samples/02-agents/skills/code_defined_skill/code_defined_skill.py | Updates sample to InlineSkill / InlineSkillResource and new provider API. |
- Use anyio.Path for async file I/O in _FileSkillResource.read() - Use noqa: ASYNC240 for pure string os.path calls in async context - Restore pre-commit if/else pattern in InlineSkillScript.run() - Break long lines to fit 120-char limit in _skills.py and test_skills.py Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The pyright ignore comments only suppress errors on the same line, so multi-line lambdas left arguments on continuation lines uncovered. Collapse both lambdas to single lines matching the existing load_skill lambda pattern. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…t errors Python lambdas cannot have type annotations, so pyright reports reportUnknownLambdaType and reportUnknownArgumentType errors that cannot be suppressed with inline ignore comments. Replace the lambdas for read_skill_resource and run_skill_script with typed inner async functions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Update with_prompt_template() docstring to document the
{resource_instructions} placeholder requirement
- Remove stray backslashes after {resource_instructions} and
{runner_instructions} in DEFAULT_SKILLS_INSTRUCTION_PROMPT
- Update subprocess_script_runner docstring to reflect
FileSkillScript.full_path usage
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ider Replace internal dict-based skills storage with Sequence[Skill] to eliminate silent duplicate overwrites and simplify the code. Add _find_skill helper for case-insensitive linear lookup. Also fix pyright errors in tests by adding isinstance assertions before accessing .function on SkillResource/SkillScript base types. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move security validation (path-traversal and symlink guards) for file-based skill resources into _FileSkillsSource, restoring the read-time checks that existed in main via _read_file_skill_resource. - Add _get_validated_resource_path static method on _FileSkillsSource that validates containment, existence, and symlink safety - _FileSkillsSource.get_skills() validates resource paths at discovery time via _get_validated_resource_path before passing to _FileSkillResource - Move _normalize_resource_path, _is_path_within_directory, and _has_symlink_in_path from module-level into _FileSkillsSource as static methods (only used there) - _FileSkillResource remains a simple path-to-content reader - Add tests for _get_validated_resource_path security checks Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated Code Review
Reviewers: 3 | Confidence: 83%
✓ Correctness
No actionable issues found in this dimension.
✓ Security Reliability
No actionable issues found in this dimension.
✓ Design Approach
The test updates largely track the new Inline/File skill split, but they also lock in one design regression:
SkillsProvidernow appears to initialize successfully for file-based skills that contain scripts even when no runner is configured. That shifts what used to be a clear configuration-time validation into a runtime/tool-call failure, so the provider can advertise an unusable script capability to the model and only fail afterrun_skill_scriptis invoked. The chunk mostly looks coherent with the new source/builder direction, but one sample now blurs the intended API boundary by passing a plain inline-skill list through the newsource=abstraction. That works against the PR’s own design, which otherwise distinguishesSkillsSourcecomposition from the simpler inline-skill constructor path.
Automated review by SergeyMenshykh's agents
…Sequence ambiguity Since str is a Sequence, passing a path string to the source parameter would silently be treated as a sequence of characters instead of a file source. Add an explicit TypeError with a helpful message pointing callers to SkillsProvider.from_paths(). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TaoChenOSU
approved these changes
Apr 30, 2026
Member
eavanvalkenburg
left a comment
eavanvalkenburg
left a comment
There was a problem hiding this comment.
overall looks good, but I have some doubts on the Builder pattern used, it is not super common in python...
- Remove .NET reference from _FileSkillResource docstring - Fix inconsistent resource name example (references/FAQ.md -> references/FAQ) - Simplify SkillsProvider usage in code_defined_skill sample (pass single skill directly) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SergeyMenshykh
force-pushed
the
agent-skills-refactoring-1
branch
from
b7257ab to
42a4375
Compare
| Raises: | ||
| ValueError: If the resource file does not exist. | ||
| """ | ||
| if not await anyio.Path(self._full_path).is_file(): |
Member
There was a problem hiding this comment.
anyio is a dependency that we currently do not use explicitly (it is currently part of mcp, which means they can change that without us knowing), and we do not want to introduce new dependency, we should use a different way for this, reading with the Pathlib, wrapped in a asyncio executor.
Member
Author
There was a problem hiding this comment.
Refactored the code to use asyncio instead that is used in the repo.
| `Agent Skills specification <https://agentskills.io/>`_. | ||
| """ | ||
|
|
||
| @property |
Member
There was a problem hiding this comment.
I don't think these should be models as abstract properties, this means that people will also have to override these method when they want to define their own Skill, a small init with the name, description is a more common approach, that also allows you to embed the validation of the name directly into it, instead of relying on each dev to do that
Co-authored-by: Eduard van Valkenburg <eavanvalkenburg@users.noreply.github.com>
…ce.read() - Change await self.function() to self.function() for sync functions without **kwargs; async results are handled by inspect.isawaitable() - Remove unreachable raise ValueError since __init__ already validates Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace anyio.Path usage with asyncio.to_thread + pathlib.Path since anyio is not a direct dependency of core (transitive via mcp). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use 'return await result' instead of assigning then returning. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Replace anyio with asyncio.to_thread + pathlib.Path for file I/O - Simplify awaitable check to return directly - Remove unnecessary function None guard in InlineSkillResource.read() - Add assert for type narrowing on self.function Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Replace anyio with asyncio.to_thread + pathlib.Path for file I/O - Simplify awaitable checks to return directly - Remove unnecessary function None guard in InlineSkillResource.read() - Use typing.cast instead of assert for type narrowing - Add caching behavior note to SkillsProvider docstring Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nit__ Replace abstract properties for name and description on the Skill ABC with a base __init__ that validates and stores them as regular attributes. This simplifies custom Skill subclasses (only content remains abstract) and centralizes validation in the base class, consistent with SkillResource and SkillScript base classes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation and Context
Restructures the Python agent skills system to use the new multi-source architecture as proposed in ADR-0021. This introduces composable abstractions for discovering, filtering, deduplicating, and combining skills from multiple origins (filesystem, inline code, or custom sources) — aligning the Python implementation with the .NET implementation.
Description
New abstractions
Skill— abstract base for all skill types, withfrontmatter,instructions,resources, andscriptspropertiesSkillResource/SkillScript— abstract base types for resources and scriptsSkillsSource— abstract base for skill discovery from any origin (filesystem, in-memory, custom)Skill implementations
InlineSkill— a skill defined entirely in code with a fluent API for adding resources and scripts (replaces the oldSkillclass used for code-defined skills)InlineSkillResource— a resource wrapping static content or a callableInlineSkillScript— a script backed by a callableFileSkill— a filesystem-based skill discovered fromSKILL.mdfiles on disk (replaces the oldSkillclass used for file-based skills)FileSkillScript— a file-based script that delegates execution to a configurableSkillScriptRunnerSource implementations
_FileSkillsSource— discovers file-based skills fromSKILL.mdfiles on disk_InMemorySkillsSource— holds anySkillinstances in memory (for inline/code-based skills)_AggregatingSkillsSource— composes multiple sources into oneSource decorators
_DelegatingSkillsSource— abstract base for interceptingget_skills()_FilteringSkillsSource— applies predicate-based skill filtering_DeduplicatingSkillsSource— deduplicates by name (case-insensitive, first-wins)Provider and builder
SkillsProvider— updated to accept a singleSkillsSource(composed externally); retainsSkillsProvider.from_paths()convenience factory for file-only use casesSkillsProviderBuilder— fluent API for composing skills from multiple source types withadd_file_skills(),add_skill(), filtering, deduplication, and a file script runner