fix: drop configurable dedupe from AggregateProvider, always warn#3877
fix: drop configurable dedupe from AggregateProvider, always warn#3877
Conversation
#3827 let FastMCP's on_duplicate kwarg control cross-provider duplicate behavior in AggregateProvider, but that setting was designed for LocalProvider registration: the author wrote both decorators and can react to an error at code time. AggregateProvider composition is different — mounted providers can be dynamic, so list_tools() raising at runtime based on a static kwarg would crash production with no actionable recovery. Collisions across mounted providers now always log a warning and never raise. The identity key used for detection is MCP-native per component type (uri for resources, uri_template for templates, name for tools and prompts), so two providers exposing the same resource URI under different display names get flagged while two providers exposing different URIs under the same display name no longer false-positive. LocalProvider's registration-time on_duplicate behavior is unchanged.
marvin-context-protocol
Bot
added
bug
Replace the ad-hoc name-or-uri-or-uri_template fallback with the canonical FastMCPComponent.key property, which encodes type, identifier, and version. Version variants across providers are no longer flagged as duplicates — consistent with _get_highest_version_result on the get_* side.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05c08c4dbd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| key = getattr(item, "key", None) | ||
| if key is not None: | ||
| first = seen_keys.setdefault(key, i) | ||
| if first != i: |
There was a problem hiding this comment.
Duplicate detection now compares raw item.key strings, but keys embed the version text without normalization, so semantically equivalent versions like 1 and 1.0 are treated as different and won't warn. In the same class, _get_highest_version_result uses version_sort_key (PEP 440 parsing), which treats those versions as equal and then silently picks the first provider. That creates a false negative in the collision warning path for mounted providers and hides ambiguous cross-provider resolution in production.
Useful? React with 👍 / 👎.
Test Failure AnalysisSummary: One test failed on Windows only — a tight performance benchmark missed its 100ms threshold by ~0.2ms (got 0.1002s). This is unrelated to the PR changes. Root Cause: uses a hard-coded 100ms threshold with zero tolerance. The test averaged 0.1002s on the Windows CI runner — barely over the limit. This PR only touches Suggested Solution: The test at
The test is a pre-existing issue and not caused by this PR. Detailed AnalysisFailing job: Tests: Python 3.10 on windows-latest (job ID 70993692507) Failure log excerpt: Test logic (lines 198–223): def test_server_initialization_performance(self, comprehensive_spec):
num_iterations = 5
times = []
for _ in range(num_iterations):
client = httpx.AsyncClient(base_url="https://api.example.com")
start_time = time.time()
server = create_openapi_server(...)
end_time = time.time()
times.append(end_time - start_time)
avg_time = sum(times) / len(times)
max_acceptable_time = 0.1 # 100ms ← no platform tolerance
assert avg_time < max_acceptable_time, ...The margin of failure is ~0.2ms — well within normal CI timing jitter on Windows. All other jobs (Linux Python 3.10, Linux Python 3.13, integration tests, MCP conformance) passed. PR diff summary: Only Related Files
|
CLAUDE.md now tells contributors (and LLM agents) to read the base component class before writing cross-component logic, and to prefer .key over name/uri fallbacks. Expanded the .key docstring with when-to-use guidance so the same hint surfaces in LSP hover.
1 participant
Follow-up to #3827.
#3827 wired FastMCP's
on_duplicatekwarg into AggregateProvider so that cross-provider collisions could raiseValueErroronlist_*calls. That overloaded the kwarg:on_duplicatewas designed for LocalProvider's registration-time behavior, where the author writes both decorators and can meaningfully react to an error at code time. AggregateProvider composition is different — mounted providers can be dynamic (file-system watchers, remote mirrors, etc.), so a strict-mode runtime error would crash production with no actionable recovery path for the author.This change makes the cross-provider signal warn-only and non-configurable.
on_duplicatestill controls LocalProvider registration as before; AggregateProvider just emits a warning when two mounted providers return the same MCP component. Collision detection now usesFastMCPComponent.key, which is the canonical MCP identity — it encodes type (tool:/resource:/template:/prompt:), identifier (name for tools/prompts,urifor resources,uri_templatefor templates), and version. That replaces the ad-hocname or uri or uri_templatefallback from #3827 and gives version-awareness for free:[email protected]from provider A and[email protected]from provider B are variants, not duplicates — matching what_get_highest_version_resultalready does on theget_*side.