Python: Changed Skills Resources to Use Type-Safe Dependencies#4564
Python: Changed Skills Resources to Use Type-Safe Dependencies#4564suneetnangia wants to merge 4 commits intomicrosoft:mainfrom
Conversation
Signed-off-by: Suneet Nangia <suneetnangia@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces an experimental SkillContext[DepsT] API to enable typed dependency injection into code-defined skill resource functions, and updates the skills provider/tests/samples/docs accordingly.
Changes:
- Added
SkillContext[DepsT]and detection logic soSkillsProvidercan inject typed deps into resource functions. - Updated
SkillsProviderto accept adepsobject and pass it into resource functions that opt intoSkillContext. - Updated tests and the code-defined skills sample/docs to demonstrate typed deps.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| python/packages/core/agent_framework/_skills.py | Adds SkillContext, detects whether a resource takes it, and injects provider deps into resource execution. |
| python/packages/core/agent_framework/init.py | Re-exports SkillContext from the package root. |
| python/packages/core/tests/core/test_skills.py | Adds tests for SkillContext injection/mutation and related detection behavior. |
| python/samples/02-agents/skills/code_skill/code_skill.py | Updates sample to use SkillContext + provider-level deps instead of runtime kwargs. |
| python/samples/02-agents/skills/code_skill/README.md | Updates sample documentation to describe the SkillContext pattern. |
| self._deps = deps | ||
| self._skills = _load_skills(skill_paths, skills, resource_extensions or DEFAULT_RESOURCE_EXTENSIONS) |
There was a problem hiding this comment.
deps is stored on the SkillsProvider instance and passed by reference into every SkillContext. Since the API explicitly allows mutating ctx.deps, this makes dependency/state mutations global to the provider and potentially shared across concurrent sessions/runs. If this is intended, it should be documented clearly; otherwise consider supporting per-session/per-run deps (e.g., a deps_factory or storing deps in state) to avoid cross-request state bleed.
There was a problem hiding this comment.
This is intended, deps implementer will take care of concurrency using Double-Checked Locking pattern etc.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback. Document that this is intended, deps implementer must take care of concurrency using Double-Checked Locking pattern etc.
| async def _read_skill_resource(self, skill_name: str, resource_name: str) -> str: | ||
| """Read a named resource from a skill. | ||
|
|
||
| Resolves the resource by case-insensitive name lookup. Static | ||
| ``content`` is returned directly; callable resources are invoked | ||
| (awaited if async). | ||
| (awaited if async). Resource functions that declare a | ||
| :class:`SkillContext` first parameter receive a context carrying | ||
| the provider's ``deps``. | ||
|
|
There was a problem hiding this comment.
Changing SkillsProvider._read_skill_resource to no longer accept **kwargs removes the mechanism by which agent.run(..., **kwargs) runtime kwargs are forwarded into the read_skill_resource tool (via FunctionTool’s runtime kwargs forwarding). This is a breaking behavioral change for existing code-defined resources that relied on request-scoped kwargs (even if the tool schema didn’t declare them). Consider keeping **kwargs on _read_skill_resource for backward compatibility (and optionally exposing them via SkillContext, e.g., ctx.kwargs), or mark/document this as a breaking change and provide a migration path.
There was a problem hiding this comment.
Use of **kwargs is not released as a package yet, so we may not want to keep it?
|
CC: @SergeyMenshykh |
Signed-off-by: Suneet Nangia <suneetnangia@gmail.com>
Motivation and Context
Description
This pull request introduces a new, experimental
SkillContextAPI for code-defined skill resources in the agent framework, enabling typed dependency injection into resource functions. It replaces the previous pattern of passing**kwargsto resource functions, offering a more robust and type-safe way to provide shared dependencies (such as database clients or configuration) to skill resources.SkillContext API and Dependency Injection:
Introduced the
SkillContext[DepsT]class inagent_framework._skills, allowing resource functions to declare a first parameter that receives a typed context with dependencies injected by theSkillsProvider. This is detected automatically and is mutable, enabling state sharing between resources. [1] [2] [3] [4]Updated the
SkillsProviderto accept adepsargument, which is passed to all resource functions that declare aSkillContextparameter. The provider now detects whether a resource expects aSkillContextand injects it accordingly. [1] [2] [3] [4]Testing and Backward Compatibility:
SkillContextpattern, including sync and async resources, mutation of dependencies, and backward compatibility for resources that do not useSkillContext.Documentation and Samples:
README.md,code_skill.py) to demonstrate the new typed dependency injection pattern usingSkillContext, replacing references to the old**kwargspattern. [1] [2] [3] [4] [5] [6]These changes make skill resource functions more robust, type-safe, and easier to test.
Contribution Checklist