Python: make FoundryToolbox.as_skills_provider() disable_caching effective#7135
Python: make FoundryToolbox.as_skills_provider() disable_caching effective#7135giles17 wants to merge 2 commits into
Conversation
…ctive 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
There was a problem hiding this comment.
Pull request overview
This PR fixes FoundryToolbox.as_skills_provider() in the Python agent-framework-foundry-hosting package so the existing disable_caching flag actually affects toolbox skill discovery, avoiding an extra skill://index.json MCP round-trip on every agent run for hosted agents. It does this by composing caching at the toolbox layer (since SkillsProvider intentionally does not auto-cache caller-supplied SkillsSources) and adds a refresh interval option plus targeted unit tests.
Changes:
- Compose
DeduplicatingSkillsSource(CachingSkillsSource(...))insideFoundryToolbox.as_skills_provider()when caching is enabled, and stop forwardingdisable_cachingtoSkillsProvider(it’s a no-op for caller-supplied sources). - Add
cache_refresh_interval: timedelta | Noneto allow periodic re-discovery when toolbox-attached skills change over process lifetime. - Add unit tests validating default caching (read once), disabled caching (read every run), and refresh-interval behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| python/packages/foundry_hosting/agent_framework_foundry_hosting/_toolbox.py | Implements effective discovery caching by wrapping the toolbox skills source in CachingSkillsSource (plus dedup), adds cache_refresh_interval, and updates the docstring to match real behavior. |
| python/packages/foundry_hosting/tests/test_toolbox.py | Adds tests that stub MCPSkillsSource to count discovery calls and verify cached/uncached/refresh behaviors. |
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
|
Flagged issue
Source: automated DevFlow PR review |
![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: 5 | Confidence: 91%
✓ Correctness
The PR correctly fixes the ineffective
disable_cachingparameter by composing caching wrappers directly inas_skills_provider(). The API usage ofCachingSkillsSourceandDeduplicatingSkillsSourcematches their actual constructors. Removingdisable_cachingfrom theSkillsProvider()call is correct since SkillsProvider explicitly does not auto-cache caller-supplied sources. The safety argument (context-independence of_FoundryToolboxSkillsSource) is valid — itsget_skillsignoresSkillsSourceContextand always returns the same toolbox skills. No correctness issues found.
✓ Security Reliability
This PR correctly fixes the ineffective
disable_cachingparameter by composingCachingSkillsSourceandDeduplicatingSkillsSourcedirectly inas_skills_provider(). The security argument is sound:_FoundryToolboxSkillsSource.get_skills()ignores theSkillsSourceContext(only readsself._toolbox.session), so a shared unkeyed cache cannot leak skills across agents/tenants. The concurrency model inCachingSkillsSource(per-key asyncio locks with double-check) is correct. No injection risks, resource leaks, or unhandled failure modes introduced.
✓ Test Coverage
The PR adds three well-targeted unit tests covering the three key behaviors: default caching (index read once), disabled caching (re-read every call), and cache_refresh_interval threading (zero interval causes re-reads). Tests correctly monkeypatch MCPSkillsSource with a counting stub to verify composition logic without network I/O. The test helper structure is clean and reusable. Coverage is adequate for the scope of the change — the core CachingSkillsSource and DeduplicatingSkillsSource classes have their own independent test suites, so the toolbox tests rightly focus on verifying correct composition/wiring rather than re-testing caching semantics.
✓ Failure Modes
The change correctly composes caching at the call site where it has effect, rather than relying on SkillsProvider which deliberately skips auto-caching for caller-supplied sources. Exception propagation through CachingSkillsSource and DeduplicatingSkillsSource is clean (no swallowed errors). The removed
disable_cachingargument to SkillsProvider was verified as a no-op for caller-supplied SkillsSource instances. No silent failure modes, lost errors, or operational hazards introduced.
✗ Design Approach
I found one blocking design issue: the new default cache is shared across all callers even though toolbox skill discovery still depends on Foundry’s per-request hosting context, so the first caller’s
skill://index.jsoncan be replayed to later callers instead of being re-evaluated for them.
Flagged Issues
-
FoundryToolbox.as_skills_provider()now wraps_FoundryToolboxSkillsSourcein a default sharedCachingSkillsSource, but_ToolboxAuthinjectsget_request_context().platform_headers()on every outbound MCP request and documents that the forwarded per-request call ID lets the Foundry MCP proxy 'resolve the caller context server-side' (python/packages/foundry_hosting/agent_framework_foundry_hosting/_toolbox.py:76-80,:106-110). CoreCachingSkillsSourceuses one shared bucket unless given an isolation selector (python/packages/core/agent_framework/_skills.py:3775-3782,:3850-3856), so once one hosted request populates the cache, later callers silently skip re-discovery and can see the wrong skill set. The existing provider contract explicitly avoids auto-caching caller-suplied sources for exactly this cross-context leak risk (python/packages/core/agent_framework/_skills.py:2047-2054,python/packages/core/tests/core/test_skills.py:603-6052).
Automated review by giles17's agents
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
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 93%
✓ Correctness
The PR correctly composes caching at the
as_skills_provider()level by wrapping the source inDeduplicatingSkillsSource(CachingSkillsSource(...))when caching is enabled. The constructor signatures ofCachingSkillsSource(acceptsrefresh_interval: timedelta | None) andDeduplicatingSkillsSource(acceptsinner_source: SkillsSource) match the PR's usage. Omittingdisable_cachingfrom theSkillsProvidercall is safe since it defaults toFalseand is a no-op for caller-supplied sources anyway. No correctness issues found.
✓ Security Reliability
The PR correctly fixes the ineffective
disable_cachingparameter by composing caching directly inas_skills_provider(). The implementation is sound: CachingSkillsSource properly handles refresh_interval with monotonic timestamps and per-key async locks, DeduplicatingSkillsSource wraps it correctly, and the safety argument for shared caching (context-independence of _FoundryToolboxSkillsSource) is valid. The removal ofdisable_cachingfrom the SkillsProvider call is correct since it was always a no-op for caller-supplied sources. No security or reliability issues identified.
✓ Test Coverage
The test coverage for this PR is good. Three new tests cover the main behavioral scenarios: default caching (index read once), disabled caching (read every call), and cache_refresh_interval wiring (zero interval forces re-reads). Assertions are meaningful (exact invocation counts), and the monkeypatch approach appropriately isolates the unit under test. The tests verify the wiring of
as_skills_provider()without redundantly re-testingCachingSkillsSourceinternals.
✓ Failure Modes
The PR correctly composes caching at the call site where it can actually take effect, rather than forwarding a no-op parameter to SkillsProvider. The failure-mode surface is minimal: CachingSkillsSource handles refresh_interval=None (never-expire) correctly, the session-None check in _FoundryToolboxSkillsSource is preserved, and the removed disable_caching kwarg to SkillsProvider was always inert for caller-supplied sources. No silent failures, lost errors, or operational failure paths introduced.
✓ Design Approach
The new caching layer makes
disable_cachingobservable, but it also cachesMCPSkillobjects that are bound to a specific MCP session. Because the MCP tool can reconnect by replacingtool.session, later runs may reuse cached skills that still point at the old closed session, breakingload_skill/read_skill_resourceafter reconnects.
Automated review by giles17's agents
| # so we compose the caching ourselves. | ||
| source: SkillsSource = _FoundryToolboxSkillsSource(self) | ||
| if not disable_caching: | ||
| source = DeduplicatingSkillsSource(CachingSkillsSource(source, refresh_interval=cache_refresh_interval)) |
There was a problem hiding this comment.
This caches concrete MCPSkill objects that capture the MCP session at construction time and use that stored client for get_content/get_resource. The MCP tool supports reconnect by replacing tool.session with a new object, but after such a reconnect this cache will still hand out skills bound to the old closed session, causing load_skill or read_skill_resource to fail. Consider storing reconnect-safe discovery data instead, or rebuilding session-bound MCPSkill wrappers from the current toolbox session on each cache hit.
Motivation & Context
FoundryToolbox.as_skills_provider()accepts adisable_cachingparameter, but it had no effect: the toolbox's skill discovery was re-read fromskill://index.jsonover MCP on every agent run, adding an avoidable round-trip per request for hosted agents.The cause is that
as_skills_provider()forwardeddisable_cachingtoSkillsProvider, which deliberately does not auto-wrap a caller-suppliedSkillsSourcein caching (to avoid leaking skills across agents/tenants for context-aware sources). So the flag was silently ignored, and skills were never cached. The docstring also claimed "caching after the first discovery", which was inaccurate.Surfaced during review of #7099.
Description & Review Guide
as_skills_provider()now composes caching itself, wrapping the source inDeduplicatingSkillsSource(CachingSkillsSource(...))when caching is enabled, sodisable_cachinghas a real, observable effect.disable_cachingtoSkillsProvider(it was a no-op there for a caller-supplied source).cache_refresh_interval: timedelta | Noneparameter, threaded intoCachingSkillsSource, so callers can re-query when a toolbox's attached skills change over the process lifetime.skill://index.jsonread once instead of per run). This is safe because_FoundryToolboxSkillsSourceis context-independent: itsget_skillsignores theSkillsSourceContextand returns the same toolbox skills for every caller. No public API is removed;disable_cachingkeeps working and now actually does something.Related Issue
Fixes #7100
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.