From ab3ad02cee1b6f0c181047ceaa958d722cfb6b34 Mon Sep 17 00:00:00 2001 From: Giles Odigwe Date: Wed, 15 Jul 2026 14:33:50 -0700 Subject: [PATCH 1/4] Python: make FoundryToolbox.as_skills_provider() disable_caching effective as_skills_provider() forwarded disable_caching to SkillsProvider, which ignores it for a caller-supplied SkillsSource, so it was a no-op and the toolbox re-read skill://index.json on every agent run. Compose caching in as_skills_provider() instead: wrap the context-independent _FoundryToolboxSkillsSource in DeduplicatingSkillsSource(CachingSkillsSource(...)). Add a cache_refresh_interval param, fix the docstring, and add tests covering cached, disabled, and refresh-interval behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 84150ec4-6f7c-4ef8-b9fb-12fa11652773 --- .../_toolbox.py | 26 +++++- .../foundry_hosting/tests/test_toolbox.py | 84 ++++++++++++++++++- 2 files changed, 105 insertions(+), 5 deletions(-) diff --git a/python/packages/foundry_hosting/agent_framework_foundry_hosting/_toolbox.py b/python/packages/foundry_hosting/agent_framework_foundry_hosting/_toolbox.py index a3848a75be..bf25af57ef 100644 --- a/python/packages/foundry_hosting/agent_framework_foundry_hosting/_toolbox.py +++ b/python/packages/foundry_hosting/agent_framework_foundry_hosting/_toolbox.py @@ -9,6 +9,8 @@ import httpx from agent_framework import ( + CachingSkillsSource, + DeduplicatingSkillsSource, MCPSkillsSource, MCPStreamableHTTPTool, SkillsProvider, @@ -19,6 +21,7 @@ if TYPE_CHECKING: from collections.abc import Generator + from datetime import timedelta from agent_framework import Skill from azure.core.credentials import TokenCredential @@ -195,6 +198,7 @@ def as_skills_provider( source_id: str | None = None, instruction_template: str | None = None, disable_caching: bool = False, + cache_refresh_interval: timedelta | None = None, disable_load_skill_approval: bool = False, disable_read_skill_resource_approval: bool = False, disable_run_skill_script_approval: bool = False, @@ -215,8 +219,17 @@ def as_skills_provider( source_id: Unique identifier for the provider instance. instruction_template: Custom system-prompt template for advertising skills; see :class:`~agent_framework.SkillsProvider`. - disable_caching: Re-query the toolbox on every agent run instead of - caching after the first discovery. + disable_caching: When ``True``, re-query the toolbox on every agent run, + re-reading ``skill://index.json`` each time. When ``False`` (the + default), the toolbox's skill discovery is cached after the first run + so the index is read once. The toolbox's skills are the same for every + caller (discovery ignores the per-run :class:`SkillsSourceContext`), so + caching them here is safe. + cache_refresh_interval: Optional duration after which the cached skill + discovery is considered stale and re-read from the toolbox on the next + agent run. Useful when a toolbox's attached skills change over the + process lifetime. When ``None`` (the default), the cache never expires. + Ignored when ``disable_caching=True``. disable_load_skill_approval: When ``True``, register the provider's ``load_skill`` tool with ``approval_mode="never_require"`` so loading a skill body needs no host approval. Set this for unattended agents @@ -250,11 +263,16 @@ def as_skills_provider( ) await ResponsesHostServer(agent).run_async() """ + # _FoundryToolboxSkillsSource is context-independent (get_skills ignores the + # SkillsSourceContext), so caching it here can't leak skills across callers. + # SkillsProvider won't auto-cache a caller source, so we compose it ourselves. + source: SkillsSource = _FoundryToolboxSkillsSource(self) + if not disable_caching: + source = DeduplicatingSkillsSource(CachingSkillsSource(source, refresh_interval=cache_refresh_interval)) return SkillsProvider( - _FoundryToolboxSkillsSource(self), + source, source_id=source_id, instruction_template=instruction_template, - disable_caching=disable_caching, disable_load_skill_approval=disable_load_skill_approval, disable_read_skill_resource_approval=disable_read_skill_resource_approval, disable_run_skill_script_approval=disable_run_skill_script_approval, diff --git a/python/packages/foundry_hosting/tests/test_toolbox.py b/python/packages/foundry_hosting/tests/test_toolbox.py index 005eefa576..1208d33f22 100644 --- a/python/packages/foundry_hosting/tests/test_toolbox.py +++ b/python/packages/foundry_hosting/tests/test_toolbox.py @@ -4,7 +4,8 @@ from __future__ import annotations -from datetime import datetime, timezone +from datetime import datetime, timedelta, timezone +from types import SimpleNamespace from typing import cast from unittest.mock import AsyncMock @@ -229,3 +230,84 @@ async def get_skills(self, context: SkillsSourceContext) -> list[str]: assert result == ["skill-a"] assert captured["client"] is sentinel_session + + +class _FakeSkill: + """Minimal stand-in for a :class:`~agent_framework.Skill` for caching tests.""" + + def __init__(self, name: str) -> None: + self.frontmatter = SimpleNamespace(name=name) + + +def _patch_counting_mcp_source(monkeypatch: pytest.MonkeyPatch) -> list[int]: + """Patch ``MCPSkillsSource`` with a stub that counts index reads. + + Returns a single-element list whose value tracks how many times + ``get_skills`` (i.e. a ``skill://index.json`` read) has been invoked. + """ + read_count = [0] + + class _CountingSkillsSource: + def __init__(self, *, client: object) -> None: + self._client = client + + async def get_skills(self, context: SkillsSourceContext) -> list[_FakeSkill]: + read_count[0] += 1 + return [_FakeSkill("skill-a")] + + monkeypatch.setattr("agent_framework_foundry_hosting._toolbox.MCPSkillsSource", _CountingSkillsSource) + return read_count + + +async def test_as_skills_provider_caches_by_default(monkeypatch: pytest.MonkeyPatch) -> None: + toolbox = FoundryToolbox( + _FakeCredential(), # type: ignore + url="https://h/toolboxes/tb/mcp", + ) + toolbox.session = object() # type: ignore + read_count = _patch_counting_mcp_source(monkeypatch) + + provider = toolbox.as_skills_provider() + context = _source_context() + for _ in range(3): + await provider._source.get_skills(context) # pyright: ignore[reportPrivateUsage] + + # By default the toolbox index is read once and reused across agent runs. + assert read_count[0] == 1 + + +async def test_as_skills_provider_disable_caching_rereads_every_run(monkeypatch: pytest.MonkeyPatch) -> None: + toolbox = FoundryToolbox( + _FakeCredential(), # type: ignore + url="https://h/toolboxes/tb/mcp", + ) + toolbox.session = object() # type: ignore + read_count = _patch_counting_mcp_source(monkeypatch) + + provider = toolbox.as_skills_provider(disable_caching=True) + context = _source_context() + for _ in range(3): + await provider._source.get_skills(context) # pyright: ignore[reportPrivateUsage] + + # With caching disabled the index is re-read on every agent run. + assert read_count[0] == 3 + + +async def test_as_skills_provider_cache_refresh_interval_rereads_after_staleness( + monkeypatch: pytest.MonkeyPatch, +) -> None: + toolbox = FoundryToolbox( + _FakeCredential(), # type: ignore + url="https://h/toolboxes/tb/mcp", + ) + toolbox.session = object() # type: ignore + read_count = _patch_counting_mcp_source(monkeypatch) + + # A zero interval makes every cached result immediately stale, so each run + # re-reads the index -- proving cache_refresh_interval is wired through. + provider = toolbox.as_skills_provider(cache_refresh_interval=timedelta(0)) + context = _source_context() + for _ in range(3): + await provider._source.get_skills(context) # pyright: ignore[reportPrivateUsage] + + assert read_count[0] == 3 From 8abb655903e22ba771af75fff28305df7dd81aef Mon Sep 17 00:00:00 2001 From: Giles Odigwe Date: Wed, 15 Jul 2026 15:11:48 -0700 Subject: [PATCH 2/4] Clarify caller-invariant skill-set wording in as_skills_provider docs Emphasize that the toolbox advertises the same skill set to every caller (the per-request call-id governs execution/authorization, not which skills are listed) rather than leaning on 'ignores SkillsSourceContext'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 84150ec4-6f7c-4ef8-b9fb-12fa11652773 --- .../agent_framework_foundry_hosting/_toolbox.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/python/packages/foundry_hosting/agent_framework_foundry_hosting/_toolbox.py b/python/packages/foundry_hosting/agent_framework_foundry_hosting/_toolbox.py index bf25af57ef..042077283b 100644 --- a/python/packages/foundry_hosting/agent_framework_foundry_hosting/_toolbox.py +++ b/python/packages/foundry_hosting/agent_framework_foundry_hosting/_toolbox.py @@ -222,9 +222,10 @@ def as_skills_provider( disable_caching: When ``True``, re-query the toolbox on every agent run, re-reading ``skill://index.json`` each time. When ``False`` (the default), the toolbox's skill discovery is cached after the first run - so the index is read once. The toolbox's skills are the same for every - caller (discovery ignores the per-run :class:`SkillsSourceContext`), so - caching them here is safe. + so the index is read once. The toolbox's advertised skill set is the + same for every caller (the per-request call-id governs execution/ + authorization, not which skills are listed), so a single shared cache + is safe. cache_refresh_interval: Optional duration after which the cached skill discovery is considered stale and re-read from the toolbox on the next agent run. Useful when a toolbox's attached skills change over the @@ -263,9 +264,10 @@ def as_skills_provider( ) await ResponsesHostServer(agent).run_async() """ - # _FoundryToolboxSkillsSource is context-independent (get_skills ignores the - # SkillsSourceContext), so caching it here can't leak skills across callers. - # SkillsProvider won't auto-cache a caller source, so we compose it ourselves. + # The toolbox advertises the same skill set to every caller (the per-request + # call-id governs execution/authorization, not which skills are listed), so a + # single shared cache is safe. SkillsProvider won't auto-cache a caller source, + # so we compose the caching ourselves. source: SkillsSource = _FoundryToolboxSkillsSource(self) if not disable_caching: source = DeduplicatingSkillsSource(CachingSkillsSource(source, refresh_interval=cache_refresh_interval)) From dccb3e4c63dc0269a0caf3c7c96d85b852e81c1b Mon Sep 17 00:00:00 2001 From: Giles Odigwe Date: Fri, 17 Jul 2026 14:14:21 -0700 Subject: [PATCH 3/4] Make MCP skills reconnect-safe via session_provider Cached MCPSkill objects captured the MCP ClientSession at construction, so after a FoundryToolbox reconnect (which replaces its session) load_skill and read_skill_resource would fail against the closed session. This regressed once as_skills_provider() started caching discovery by default. Add an optional session_provider callable to MCPSkillsSource and MCPSkill (exactly one of client or session_provider). When supplied, the session is resolved on every fetch, mirroring how MCPTool resolves self.session live at call time. _FoundryToolboxSkillsSource now passes a provider that returns the toolbox's current session, so cached skills always use the live session. The fixed client= path is unchanged and backward-compatible. Update core tests, foundry_hosting tests, and core AGENTS.md. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 84150ec4-6f7c-4ef8-b9fb-12fa11652773 --- python/packages/core/AGENTS.md | 2 +- .../packages/core/agent_framework/_skills.py | 94 ++++++++++++++++--- .../core/tests/core/test_mcp_skills.py | 72 ++++++++++++++ .../_toolbox.py | 18 +++- .../foundry_hosting/tests/test_toolbox.py | 31 +++++- 5 files changed, 196 insertions(+), 21 deletions(-) diff --git a/python/packages/core/AGENTS.md b/python/packages/core/AGENTS.md index 0f1be08ae8..fb2132fdc2 100644 --- a/python/packages/core/AGENTS.md +++ b/python/packages/core/AGENTS.md @@ -91,7 +91,7 @@ agent_framework/ - **`SkillScriptRunner`** - Protocol for file-based script execution. Any callable matching `(skill, script, args) -> Any` satisfies it. Code-defined scripts do not use a runner. - **`SkillScriptArgumentParser`** - Public type alias for an optional callable `(raw args: dict | list[str] | str | None) -> dict | None` that converts the raw `args` value before an `InlineSkillScript` runs (applied before the inline list-args guard). It is an opt-in customization hook (port of .NET PR #6498) that lets callers support backends sending tool-call arguments in a non-conforming shape (e.g. vLLM JSON strings). The output is constrained to a `dict` (named keyword arguments) or `None`, because inline scripts bind arguments by keyword name. Supply it via the `argument_parser=` constructor arg on `InlineSkillScript`, `InlineSkill` (default for scripts added via `@skill.script`), or `ClassSkill` (default for scripts discovered via `@ClassSkill.script`). When `None` (the default), the raw value is used unchanged. File-based scripts are unaffected (their runner owns arg handling). - **`SkillsProvider`** - Context provider (extends `ContextProvider`) that discovers file-based skills from `SKILL.md` files and/or accepts code-defined `Skill` instances. Follows progressive disclosure: advertise → load → read resources / run scripts. By default all three tools it exposes (`load_skill`, `read_skill_resource`, `run_skill_script`) are registered with `approval_mode="always_require"`, so every skill operation needs approval. To run unattended, pass one of the static auto-approval rules to `ToolApprovalMiddleware` (via `auto_approval_rules`): `SkillsProvider.read_only_tools_auto_approval_rule` approves only the read-only tools (`load_skill`, `read_skill_resource`) while still prompting for `run_skill_script`, and `SkillsProvider.all_tools_auto_approval_rule` approves every skill tool including script execution. Both rules reject any call carrying a `server_label` so they stay scoped to this provider's local tools and never auto-approve a same-named hosted tool. Alternatively, for trusted skills, the constructor / `from_paths` kwargs `disable_load_skill_approval`, `disable_read_skill_resource_approval`, and `disable_run_skill_script_approval` (all default `False`) opt individual tools out of approval entirely by registering them with `approval_mode="never_require"` (the auto-approval rules only apply to tools that still require approval). The tool names are also exposed as class constants (`LOAD_SKILL_TOOL_NAME`, `READ_SKILL_RESOURCE_TOOL_NAME`, `RUN_SKILL_SCRIPT_TOOL_NAME`). -- **`SkillsSource` decorators** - Skill sources are composable: `SkillsSource` is the abstract base, with concrete sources (`InMemorySkillsSource`, `FileSkillsSource`, `MCPSkillsSource`) and decorators that wrap an inner source — `AggregatingSkillsSource` (concatenate several sources), `FilteringSkillsSource` (predicate filter), `DeduplicatingSkillsSource` (first-wins by name), and `CachingSkillsSource` (cache the inner source's skills list). `DelegatingSkillsSource` is the abstract base for decorators. **`get_skills` takes a `SkillsSourceContext`**: every source/decorator implements `async def get_skills(self, context: SkillsSourceContext) -> list[Skill]` and forwards `context` to inner sources. `SkillsSourceContext` (frozen) carries the invoking `agent` (`SupportsAgentRun`) and optional `session` (`AgentSession | None`); `SkillsProvider` builds it from `before_run`'s `agent`/`session` and passes it into the pipeline. `FilteringSkillsSource`'s predicate is context-aware: `Callable[[Skill, SkillsSourceContext], bool]` (port of .NET #6797). **Default caching is applied only to the built-in, context-independent leaf sources**: for the `Skill` / sequence-of-skills / `from_paths` constructors, `SkillsProvider` builds `DeduplicatingSkillsSource(CachingSkillsSource())` so expensive filesystem/network discovery runs once. A **caller-supplied `SkillsSource` is used as-is — never auto-wrapped in caching or deduplication** — because auto-caching a context-aware caller source in a single shared bucket would replay the first invocation's skills for later `SkillsSourceContext`s and leak skills across agents/tenants (matches .NET, whose custom-source constructor also adds no caching/dedup). Callers who want caching on a custom pipeline compose `CachingSkillsSource(inner, cache_isolation_key_selector=...)` themselves. `disable_caching=True` only affects the built-in leaf caching (it has no effect on a caller-supplied source, which is never cached). `CachingSkillsSource` shares a single in-flight fetch across concurrent callers (per cache key) and does not update its cache on a failed fetch, so the next call retries (an initial failure leaves the cache empty; a refresh failure keeps the previously cached list). By default all callers share one cache bucket; pass `cache_isolation_key_selector=Callable[[SkillsSourceContext], str | None]` to cache separately per key (e.g. per agent name) for context-aware inner sources — the key should be low-cardinality and stable, and returning `None` (or leaving the selector `None`) uses the shared bucket. By default a cached list never expires; pass `refresh_interval=timedelta(...)` (port of .NET `CachingAgentSkillsSourceOptions.RefreshInterval`) to treat a cached list as stale once it is older than the interval so the next call re-queries the inner source (useful when an inner source such as `MCPSkillsSource` changes over the process lifetime; a zero/negative interval makes every result immediately stale, and a failed refresh keeps the prior list and retries). Freshness is measured with a monotonic clock (`time.monotonic()`). `SkillsProvider.__init__` / `from_paths` expose a `cache_refresh_interval` kwarg that is threaded into the built-in `CachingSkillsSource` (it has no effect on a caller-supplied source or when `disable_caching=True`). +- **`SkillsSource` decorators** - Skill sources are composable: `SkillsSource` is the abstract base, with concrete sources (`InMemorySkillsSource`, `FileSkillsSource`, `MCPSkillsSource`) and decorators that wrap an inner source — `AggregatingSkillsSource` (concatenate several sources), `FilteringSkillsSource` (predicate filter), `DeduplicatingSkillsSource` (first-wins by name), and `CachingSkillsSource` (cache the inner source's skills list). `DelegatingSkillsSource` is the abstract base for decorators. **`get_skills` takes a `SkillsSourceContext`**: every source/decorator implements `async def get_skills(self, context: SkillsSourceContext) -> list[Skill]` and forwards `context` to inner sources. `SkillsSourceContext` (frozen) carries the invoking `agent` (`SupportsAgentRun`) and optional `session` (`AgentSession | None`); `SkillsProvider` builds it from `before_run`'s `agent`/`session` and passes it into the pipeline. `FilteringSkillsSource`'s predicate is context-aware: `Callable[[Skill, SkillsSourceContext], bool]` (port of .NET #6797). **Default caching is applied only to the built-in, context-independent leaf sources**: for the `Skill` / sequence-of-skills / `from_paths` constructors, `SkillsProvider` builds `DeduplicatingSkillsSource(CachingSkillsSource())` so expensive filesystem/network discovery runs once. A **caller-supplied `SkillsSource` is used as-is — never auto-wrapped in caching or deduplication** — because auto-caching a context-aware caller source in a single shared bucket would replay the first invocation's skills for later `SkillsSourceContext`s and leak skills across agents/tenants (matches .NET, whose custom-source constructor also adds no caching/dedup). Callers who want caching on a custom pipeline compose `CachingSkillsSource(inner, cache_isolation_key_selector=...)` themselves. `disable_caching=True` only affects the built-in leaf caching (it has no effect on a caller-supplied source, which is never cached). `CachingSkillsSource` shares a single in-flight fetch across concurrent callers (per cache key) and does not update its cache on a failed fetch, so the next call retries (an initial failure leaves the cache empty; a refresh failure keeps the previously cached list). By default all callers share one cache bucket; pass `cache_isolation_key_selector=Callable[[SkillsSourceContext], str | None]` to cache separately per key (e.g. per agent name) for context-aware inner sources — the key should be low-cardinality and stable, and returning `None` (or leaving the selector `None`) uses the shared bucket. By default a cached list never expires; pass `refresh_interval=timedelta(...)` (port of .NET `CachingAgentSkillsSourceOptions.RefreshInterval`) to treat a cached list as stale once it is older than the interval so the next call re-queries the inner source (useful when an inner source such as `MCPSkillsSource` changes over the process lifetime; a zero/negative interval makes every result immediately stale, and a failed refresh keeps the prior list and retries). Freshness is measured with a monotonic clock (`time.monotonic()`). `SkillsProvider.__init__` / `from_paths` expose a `cache_refresh_interval` kwarg that is threaded into the built-in `CachingSkillsSource` (it has no effect on a caller-supplied source or when `disable_caching=True`). **`MCPSkillsSource` and `MCPSkill` accept exactly one of `client` (a fixed `ClientSession`) or `session_provider` (`Callable[[], ClientSession]`, resolved on every fetch); providing both/neither raises `ValueError`.** Use `session_provider` when the underlying session may be swapped over time — e.g. a reconnecting `MCPTool`/`FoundryToolbox` whose `session` is replaced on reconnect — so cached `MCPSkill`s keep fetching against the live session instead of a closed one (`MCPSkillsSource` forwards its provider to every `MCPSkill` it creates). A fixed `client` is safe only when the session outlives the skills. ### Model Context Protocol (`_mcp.py`) diff --git a/python/packages/core/agent_framework/_skills.py b/python/packages/core/agent_framework/_skills.py index 41f69ff889..2202da9a25 100644 --- a/python/packages/core/agent_framework/_skills.py +++ b/python/packages/core/agent_framework/_skills.py @@ -4054,6 +4054,38 @@ def _parse_mcp_skill_index(text: str) -> _McpSkillIndex: return _McpSkillIndex(schema=raw.get("$schema"), skills=entries) +def _resolve_mcp_session_provider( + client: ClientSession | None, + session_provider: Callable[[], ClientSession] | None, +) -> Callable[[], ClientSession]: + """Normalize the two MCP session inputs into a single session resolver. + + Callers supply **exactly one** of a fixed ``client`` or a + ``session_provider`` callable. A fixed client is wrapped in a provider that + always returns it; a provider is used as-is so the session is resolved on + every call (reconnect-safe for sources whose underlying session is replaced + over time, e.g. a reconnecting :class:`~agent_framework.MCPTool`). + + Args: + client: A fixed MCP client session, or ``None``. + session_provider: A callable returning the current MCP client session, + or ``None``. + + Returns: + A callable that returns the MCP client session to use. + + Raises: + ValueError: If both or neither of *client* and *session_provider* are + provided. + """ + if (client is None) == (session_provider is None): + raise ValueError("Provide exactly one of 'client' or 'session_provider'.") + if session_provider is not None: + return session_provider + resolved = client + return lambda: cast("ClientSession", resolved) + + @experimental(feature_id=ExperimentalFeature.MCP_SKILLS) class MCPSkillResource(SkillResource): """A :class:`SkillResource` backed by content fetched from an MCP server. @@ -4116,21 +4148,39 @@ def __init__( self, frontmatter: SkillFrontmatter, skill_md_uri: str, - client: ClientSession, + client: ClientSession | None = None, + *, + session_provider: Callable[[], ClientSession] | None = None, ) -> None: """Initialize an MCPSkill. + Provide **exactly one** of *client* or *session_provider*. + Args: frontmatter: The parsed frontmatter metadata for this skill. skill_md_uri: The full MCP resource URI of the ``SKILL.md`` resource (e.g. ``skill://unit-converter/SKILL.md``). The skill's root URI is derived by stripping the trailing ``SKILL.md`` segment. - client: The MCP client session used to fetch resources on demand. + client: A fixed MCP client session used to fetch resources on demand. + Use this when the session outlives the skill (e.g. a caller-owned + long-lived session). + + Keyword Args: + session_provider: A callable returning the current MCP client session, + resolved on every fetch. Prefer this over *client* when the + underlying session may be replaced over the skill's lifetime (for + example, a reconnecting :class:`~agent_framework.MCPTool` whose + ``session`` is swapped on reconnect), so a cached skill keeps + using the live session instead of a closed one. + + Raises: + ValueError: If both or neither of *client* and *session_provider* are + provided. """ self._frontmatter = frontmatter self._skill_md_uri = skill_md_uri self._skill_root_uri = self._compute_skill_root_uri(skill_md_uri) - self._client = client + self._session_provider = _resolve_mcp_session_provider(client, session_provider) self._content: str | None = None @property @@ -4154,7 +4204,7 @@ async def get_content(self) -> str: if self._content is not None: return self._content - result = await self._client.read_resource(_mcp_any_url(self._skill_md_uri)) + result = await self._session_provider().read_resource(_mcp_any_url(self._skill_md_uri)) text = _mcp_join_text(result) if not text: raise ValueError(f"The MCP server returned no text content for SKILL.md resource '{self._skill_md_uri}'.") @@ -4184,7 +4234,7 @@ async def get_resource(self, name: str) -> SkillResource | None: uri = self._skill_root_uri + normalized try: - result = await self._client.read_resource(_mcp_any_url(uri)) + result = await self._session_provider().read_resource(_mcp_any_url(uri)) except Exception as ex: if _is_mcp_resource_not_found(ex): logger.debug("MCP resource '%s' not available: %s", uri, ex) @@ -4276,14 +4326,36 @@ class MCPSkillsSource(SkillsSource): _INDEX_URI: Final[str] = "skill://index.json" _SKILL_MD_TYPE: Final[str] = "skill-md" - def __init__(self, client: ClientSession) -> None: + def __init__( + self, + client: ClientSession | None = None, + *, + session_provider: Callable[[], ClientSession] | None = None, + ) -> None: """Initialize an MCPSkillsSource. + Provide **exactly one** of *client* or *session_provider*. + Args: - client: An MCP client session connected to a server that - exposes Agent Skills resources. + client: A fixed MCP client session connected to a server that exposes + Agent Skills resources. Use this when the session outlives the + source (e.g. a caller-owned long-lived session). + + Keyword Args: + session_provider: A callable returning the current MCP client session, + resolved on every discovery and on each skill's on-demand fetch. + Prefer this over *client* when the underlying session may be + replaced over time (for example, a reconnecting + :class:`~agent_framework.MCPTool` whose ``session`` is swapped on + reconnect), so cached skills keep using the live session. The + provider is forwarded to every :class:`MCPSkill` this source + creates. + + Raises: + ValueError: If both or neither of *client* and *session_provider* are + provided. """ - self._client = client + self._session_provider = _resolve_mcp_session_provider(client, session_provider) async def get_skills(self, context: SkillsSourceContext) -> list[Skill]: """Discover and return skills from the MCP server. @@ -4327,7 +4399,7 @@ async def _try_read_index(self) -> _McpSkillIndex | None: absent, empty, or malformed. """ try: - result = await self._client.read_resource(_mcp_any_url(self._INDEX_URI)) + result = await self._session_provider().read_resource(_mcp_any_url(self._INDEX_URI)) except Exception as ex: if _is_mcp_resource_not_found(ex): logger.debug("No skill://index.json resource available on MCP server: %s", ex) @@ -4382,7 +4454,7 @@ def _try_create_skill(self, entry: _McpSkillIndexEntry) -> MCPSkill | None: logger.debug("Skipping entry '%s': invalid metadata: %s", entry.name, ex) return None - return MCPSkill(frontmatter=fm, skill_md_uri=entry.url, client=self._client) + return MCPSkill(frontmatter=fm, skill_md_uri=entry.url, session_provider=self._session_provider) # endregion diff --git a/python/packages/core/tests/core/test_mcp_skills.py b/python/packages/core/tests/core/test_mcp_skills.py index fce28d3c0c..ad68c0dead 100644 --- a/python/packages/core/tests/core/test_mcp_skills.py +++ b/python/packages/core/tests/core/test_mcp_skills.py @@ -350,6 +350,47 @@ def test_compute_skill_root_uri_trailing_slash(self) -> None: def test_compute_skill_root_uri_no_suffix_adds_slash(self) -> None: assert MCPSkill._compute_skill_root_uri("skill://unit-converter") == "skill://unit-converter/" + @pytest.mark.asyncio + async def test_session_provider_resolves_live_session(self) -> None: + # A session_provider is resolved on every fetch, so a skill built against + # one session follows a reconnect that swaps the session object. + from agent_framework import SkillFrontmatter + + old_client = _make_client(**{"skill://unit-converter/SKILL.md": _make_text_result("# Old\nold body")}) + new_client = _make_client(**{"skill://unit-converter/SKILL.md": _make_text_result("# New\nnew body")}) + current = {"session": old_client} + + fm = SkillFrontmatter(name="unit-converter", description="Convert between common units.") + skill = MCPSkill( + frontmatter=fm, + skill_md_uri="skill://unit-converter/SKILL.md", + session_provider=lambda: current["session"], + ) + + # Swap the session (as a reconnect would) before the first fetch. + current["session"] = new_client + content = await skill.get_content() + + assert "new body" in content + old_client.read_resource.assert_not_called() + new_client.read_resource.assert_called_once() + + def test_requires_exactly_one_of_client_or_session_provider(self) -> None: + from agent_framework import SkillFrontmatter + + fm = SkillFrontmatter(name="unit-converter", description="Convert between common units.") + client = _make_client() + + with pytest.raises(ValueError, match="exactly one"): + MCPSkill(frontmatter=fm, skill_md_uri="skill://x/SKILL.md") + with pytest.raises(ValueError, match="exactly one"): + MCPSkill( + frontmatter=fm, + skill_md_uri="skill://x/SKILL.md", + client=client, + session_provider=lambda: client, + ) + # --------------------------------------------------------------------------- # MCPSkillsSource tests @@ -507,6 +548,37 @@ async def test_sibling_binary_resource(self) -> None: content = await resource.read() assert content == data + @pytest.mark.asyncio + async def test_session_provider_resolves_live_session(self) -> None: + # Discovery and the resulting skills' on-demand fetches both resolve the + # provider, so a source built before a reconnect follows the swapped session. + old_client = _make_client(**{ + "skill://index.json": _make_text_result(SAMPLE_SKILL_INDEX, uri="skill://index.json"), + "skill://unit-converter/SKILL.md": _make_text_result("# Old\nold body"), + }) + new_client = _make_client(**{ + "skill://index.json": _make_text_result(SAMPLE_SKILL_INDEX, uri="skill://index.json"), + "skill://unit-converter/SKILL.md": _make_text_result("# New\nnew body"), + }) + current = {"session": old_client} + + source = MCPSkillsSource(session_provider=lambda: current["session"]) + skills = await source.get_skills(_SOURCE_CTX) + assert len(skills) == 1 + + # A reconnect swaps the session; the already-discovered skill must fetch + # its content from the new session, not the closed one. + current["session"] = new_client + content = await skills[0].get_content() + assert "new body" in content + + def test_requires_exactly_one_of_client_or_session_provider(self) -> None: + client = _make_client() + with pytest.raises(ValueError, match="exactly one"): + MCPSkillsSource() + with pytest.raises(ValueError, match="exactly one"): + MCPSkillsSource(client=client, session_provider=lambda: client) + # --------------------------------------------------------------------------- # McpError code branching tests diff --git a/python/packages/foundry_hosting/agent_framework_foundry_hosting/_toolbox.py b/python/packages/foundry_hosting/agent_framework_foundry_hosting/_toolbox.py index 042077283b..5a52d78184 100644 --- a/python/packages/foundry_hosting/agent_framework_foundry_hosting/_toolbox.py +++ b/python/packages/foundry_hosting/agent_framework_foundry_hosting/_toolbox.py @@ -25,6 +25,7 @@ from agent_framework import Skill from azure.core.credentials import TokenCredential + from mcp.client.session import ClientSession logger = logging.getLogger(__name__) @@ -285,14 +286,17 @@ class _FoundryToolboxSkillsSource(SkillsSource): """Discovers skills from a connected :class:`FoundryToolbox` MCP session. The toolbox's MCP ``session`` is established lazily when the toolbox connects - (via the agent or an ``async with`` block), so the session is resolved at - discovery time rather than captured at construction. + (via the agent or an ``async with`` block) and is **replaced** with a new + object whenever the toolbox reconnects. Skills are therefore bound to a + ``session_provider`` that resolves the toolbox's current session on every + fetch, so cached skills keep using the live session instead of a closed one. """ def __init__(self, toolbox: FoundryToolbox) -> None: self._toolbox = toolbox - async def get_skills(self, context: SkillsSourceContext) -> list[Skill]: + def _require_session(self) -> ClientSession: + """Return the toolbox's current MCP session, or raise if not connected.""" session = self._toolbox.session if session is None: raise RuntimeError( @@ -300,4 +304,10 @@ async def get_skills(self, context: SkillsSourceContext) -> list[Skill]: "Pass the toolbox to the agent (tools=...) or enter it as an async " "context manager before the agent runs." ) - return await MCPSkillsSource(client=session).get_skills(context) + return session + + async def get_skills(self, context: SkillsSourceContext) -> list[Skill]: + # Fail fast at discovery if not connected, then hand the source a provider + # (not a fixed session) so skills survive a reconnect that swaps the session. + self._require_session() + return await MCPSkillsSource(session_provider=self._require_session).get_skills(context) diff --git a/python/packages/foundry_hosting/tests/test_toolbox.py b/python/packages/foundry_hosting/tests/test_toolbox.py index 1208d33f22..eee547ad44 100644 --- a/python/packages/foundry_hosting/tests/test_toolbox.py +++ b/python/packages/foundry_hosting/tests/test_toolbox.py @@ -218,8 +218,8 @@ async def test_skills_source_uses_connected_session(monkeypatch: pytest.MonkeyPa captured: dict[str, object] = {} class _StubSkillsSource: - def __init__(self, *, client: object) -> None: - captured["client"] = client + def __init__(self, *, session_provider: object) -> None: + captured["session_provider"] = session_provider async def get_skills(self, context: SkillsSourceContext) -> list[str]: return ["skill-a"] @@ -229,7 +229,28 @@ async def get_skills(self, context: SkillsSourceContext) -> list[str]: result = await _FoundryToolboxSkillsSource(toolbox).get_skills(_source_context()) assert result == ["skill-a"] - assert captured["client"] is sentinel_session + # The source hands MCPSkillsSource a provider (not a fixed session) that resolves + # the toolbox's current session, so it survives a reconnect that swaps it. + provider = captured["session_provider"] + assert callable(provider) + assert provider() is sentinel_session + new_session = object() + toolbox.session = new_session # type: ignore + assert provider() is new_session + + +async def test_skills_source_requires_connection_via_provider() -> None: + toolbox = FoundryToolbox( + _FakeCredential(), # type: ignore + url="https://h/toolboxes/tb/mcp", + ) + toolbox.session = object() # type: ignore + source = _FoundryToolboxSkillsSource(toolbox) + # Discovery captures the bound provider; a later reconnect gap (session is None) + # surfaces the same clear error when the provider is resolved. + toolbox.session = None # type: ignore + with pytest.raises(RuntimeError, match="not connected"): + source._require_session() # pyright: ignore[reportPrivateUsage] class _FakeSkill: @@ -248,8 +269,8 @@ def _patch_counting_mcp_source(monkeypatch: pytest.MonkeyPatch) -> list[int]: read_count = [0] class _CountingSkillsSource: - def __init__(self, *, client: object) -> None: - self._client = client + def __init__(self, *, session_provider: object) -> None: + self._session_provider = session_provider async def get_skills(self, context: SkillsSourceContext) -> list[_FakeSkill]: read_count[0] += 1 From ddcf09ed552db286e01d9e9ebcbcf4d7a76570e8 Mon Sep 17 00:00:00 2001 From: Giles Odigwe Date: Fri, 17 Jul 2026 15:07:51 -0700 Subject: [PATCH 4/4] Fix ty error: type captured session_provider as Callable in test ty could not call the provider narrowed from \object\ (Top callable). Type the captured value as Callable[[], object] and drop the redundant callable() assert. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 84150ec4-6f7c-4ef8-b9fb-12fa11652773 --- python/packages/foundry_hosting/tests/test_toolbox.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/python/packages/foundry_hosting/tests/test_toolbox.py b/python/packages/foundry_hosting/tests/test_toolbox.py index eee547ad44..4990213a1a 100644 --- a/python/packages/foundry_hosting/tests/test_toolbox.py +++ b/python/packages/foundry_hosting/tests/test_toolbox.py @@ -4,6 +4,7 @@ from __future__ import annotations +from collections.abc import Callable from datetime import datetime, timedelta, timezone from types import SimpleNamespace from typing import cast @@ -215,10 +216,10 @@ async def test_skills_source_uses_connected_session(monkeypatch: pytest.MonkeyPa sentinel_session = object() toolbox.session = sentinel_session # type: ignore - captured: dict[str, object] = {} + captured: dict[str, Callable[[], object]] = {} class _StubSkillsSource: - def __init__(self, *, session_provider: object) -> None: + def __init__(self, *, session_provider: Callable[[], object]) -> None: captured["session_provider"] = session_provider async def get_skills(self, context: SkillsSourceContext) -> list[str]: @@ -232,7 +233,6 @@ async def get_skills(self, context: SkillsSourceContext) -> list[str]: # The source hands MCPSkillsSource a provider (not a fixed session) that resolves # the toolbox's current session, so it survives a reconnect that swaps it. provider = captured["session_provider"] - assert callable(provider) assert provider() is sentinel_session new_session = object() toolbox.session = new_session # type: ignore