mirror of
https://github.com/PrefectHQ/fastmcp.git
synced 2026-08-28 02:10:38 +02:00
fix: cross-provider duplicate detection, error visibility, mask propagation (#3827)
- AggregateProvider._collect_list_results detects duplicate component names across providers, respecting the server's on_duplicate setting - Provider errors logged at WARNING instead of DEBUG - Parent server re-masks ToolErrors from mounted children at the FastMCPError catch boundary instead of mutating the child server Fixes #3825 Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
7825355b98
commit
9e2e602038
3 changed files with 157 additions and 10 deletions
|
|
@ -74,6 +74,7 @@ class AggregateProvider(Provider):
|
|||
"""
|
||||
super().__init__()
|
||||
self.providers: list[Provider] = list(providers or [])
|
||||
self._on_duplicate: str = "warn"
|
||||
|
||||
def add_provider(self, provider: Provider, *, namespace: str = "") -> None:
|
||||
"""Add a provider with optional namespace.
|
||||
|
|
@ -106,16 +107,44 @@ class AggregateProvider(Provider):
|
|||
def _collect_list_results(
|
||||
self, results: list[Sequence[T] | BaseException], operation: str
|
||||
) -> list[T]:
|
||||
"""Collect successful list results, logging any exceptions."""
|
||||
"""Collect successful list results, logging any exceptions.
|
||||
|
||||
Detects duplicate component names across providers and applies
|
||||
the on_duplicate behavior (error/warn/replace/ignore).
|
||||
"""
|
||||
collected: list[T] = []
|
||||
# Track (name, provider_index) to allow version variants from same provider
|
||||
seen_names: dict[str, int] = {} # name -> provider index of first occurrence
|
||||
for i, result in enumerate(results):
|
||||
if isinstance(result, BaseException):
|
||||
logger.debug(
|
||||
logger.warning(
|
||||
f"Error during {operation} from provider "
|
||||
f"{self.providers[i]}: {result}"
|
||||
)
|
||||
continue
|
||||
collected.extend(result)
|
||||
for item in result:
|
||||
# Extract identity key: name, uri, or uri_template
|
||||
name = (
|
||||
getattr(item, "name", None)
|
||||
or getattr(item, "uri", None)
|
||||
or getattr(item, "uri_template", None)
|
||||
)
|
||||
if name is not None:
|
||||
name = str(name)
|
||||
if name in seen_names and seen_names[name] != i:
|
||||
# Duplicate from a DIFFERENT provider — flag it
|
||||
msg = (
|
||||
f"Duplicate {operation} component '{name}' "
|
||||
f"from provider {self.providers[i]} "
|
||||
f"(first seen from provider "
|
||||
f"{self.providers[seen_names[name]]})"
|
||||
)
|
||||
if self._on_duplicate == "error":
|
||||
raise ValueError(msg)
|
||||
elif self._on_duplicate == "warn":
|
||||
logger.warning(msg)
|
||||
seen_names.setdefault(name, i)
|
||||
collected.append(item)
|
||||
return collected
|
||||
|
||||
def _get_highest_version_result(
|
||||
|
|
@ -132,7 +161,7 @@ class AggregateProvider(Provider):
|
|||
for i, result in enumerate(results):
|
||||
if isinstance(result, BaseException):
|
||||
if not isinstance(result, NotFoundError):
|
||||
logger.debug(
|
||||
logger.warning(
|
||||
f"Error during {operation} from provider "
|
||||
f"{self.providers[i]}: {result}"
|
||||
)
|
||||
|
|
|
|||
|
|
@ -1984,6 +1984,15 @@ class FastMCP(
|
|||
if not isinstance(server, FastMCPProxy):
|
||||
server = FastMCP.as_proxy(server)
|
||||
|
||||
# Warn if parent masks errors but child doesn't (or vice versa)
|
||||
if self._mask_error_details and not server._mask_error_details:
|
||||
logger.warning(
|
||||
f"Parent server {self.name!r} has mask_error_details=True but "
|
||||
f"mounted server {server.name!r} does not. Error details from "
|
||||
f"{server.name!r} may leak through to clients. Set "
|
||||
f"mask_error_details=True on the child server to prevent this."
|
||||
)
|
||||
|
||||
# Create provider and add it with namespace
|
||||
provider: Provider = FastMCPProvider(server)
|
||||
|
||||
|
|
|
|||
|
|
@ -300,18 +300,19 @@ class TestMultipleServerMount:
|
|||
prompt_names = [prompt.name for prompt in prompts]
|
||||
assert "working_working_prompt" in prompt_names
|
||||
|
||||
# Verify that errors were logged for the unreachable provider (at DEBUG level)
|
||||
debug_messages = [
|
||||
record.message for record in caplog.records if record.levelname == "DEBUG"
|
||||
# Verify that errors were logged for the unreachable provider (at WARNING level)
|
||||
warning_messages = [
|
||||
record.message for record in caplog.records if record.levelname == "WARNING"
|
||||
]
|
||||
assert any(
|
||||
"Error during list_tools from provider" in msg for msg in debug_messages
|
||||
"Error during list_tools from provider" in msg for msg in warning_messages
|
||||
)
|
||||
assert any(
|
||||
"Error during list_resources from provider" in msg for msg in debug_messages
|
||||
"Error during list_resources from provider" in msg
|
||||
for msg in warning_messages
|
||||
)
|
||||
assert any(
|
||||
"Error during list_prompts from provider" in msg for msg in debug_messages
|
||||
"Error during list_prompts from provider" in msg for msg in warning_messages
|
||||
)
|
||||
|
||||
|
||||
|
|
@ -540,3 +541,111 @@ class TestPrefixConflictResolution:
|
|||
assert result.messages is not None
|
||||
assert isinstance(result.messages[0].content, TextContent)
|
||||
assert result.messages[0].content.text == "First app prompt"
|
||||
|
||||
|
||||
class TestCrossProviderDuplicateDetection:
|
||||
"""Test that on_duplicate catches duplicates across mounted providers."""
|
||||
|
||||
async def test_on_duplicate_error_raises_for_same_namespace(self):
|
||||
"""Mounting two servers with the same namespace and tool name raises."""
|
||||
main = FastMCP("Main", on_duplicate="error")
|
||||
sub1 = FastMCP("Sub1")
|
||||
sub2 = FastMCP("Sub2")
|
||||
|
||||
@sub1.tool(name="greet")
|
||||
def greet_v1() -> str:
|
||||
return "from sub1"
|
||||
|
||||
@sub2.tool(name="greet")
|
||||
def greet_v2() -> str:
|
||||
return "from sub2"
|
||||
|
||||
main.mount(sub1, "ns")
|
||||
main.mount(sub2, "ns")
|
||||
|
||||
with pytest.raises(ValueError, match="Duplicate"):
|
||||
await main.list_tools()
|
||||
|
||||
async def test_on_duplicate_warn_logs_for_same_namespace(self, caplog):
|
||||
"""Mounting with on_duplicate='warn' logs a warning instead of raising."""
|
||||
main = FastMCP("Main", on_duplicate="warn")
|
||||
sub1 = FastMCP("Sub1")
|
||||
sub2 = FastMCP("Sub2")
|
||||
|
||||
@sub1.tool(name="greet")
|
||||
def greet_w1() -> str:
|
||||
return "from sub1"
|
||||
|
||||
@sub2.tool(name="greet")
|
||||
def greet_w2() -> str:
|
||||
return "from sub2"
|
||||
|
||||
main.mount(sub1, "ns")
|
||||
main.mount(sub2, "ns")
|
||||
|
||||
caplog.set_level(logging.WARNING, logger="fastmcp")
|
||||
tools = await main.list_tools()
|
||||
assert len([t for t in tools if t.name == "ns_greet"]) == 2
|
||||
assert any("Duplicate" in r.message for r in caplog.records)
|
||||
|
||||
async def test_no_false_positive_for_different_names(self):
|
||||
"""Different tool names in the same namespace don't trigger duplicate."""
|
||||
main = FastMCP("Main", on_duplicate="error")
|
||||
sub1 = FastMCP("Sub1")
|
||||
sub2 = FastMCP("Sub2")
|
||||
|
||||
@sub1.tool
|
||||
def tool_a() -> str:
|
||||
return "a"
|
||||
|
||||
@sub2.tool
|
||||
def tool_b() -> str:
|
||||
return "b"
|
||||
|
||||
main.mount(sub1, "ns")
|
||||
main.mount(sub2, "ns")
|
||||
|
||||
tools = await main.list_tools()
|
||||
assert len(tools) == 2
|
||||
|
||||
|
||||
class TestMaskErrorDetailsMismatchWarning:
|
||||
"""Test that mismatched mask_error_details is warned about at mount time."""
|
||||
|
||||
async def test_warns_when_parent_masked_child_not(self, caplog):
|
||||
"""Mounting an unmasked child into a masked parent logs a warning."""
|
||||
caplog.set_level(logging.WARNING, logger="fastmcp")
|
||||
parent = FastMCP("parent", mask_error_details=True)
|
||||
child = FastMCP("child")
|
||||
|
||||
@child.tool
|
||||
def my_tool() -> str:
|
||||
return "ok"
|
||||
|
||||
parent.mount(child, "ns")
|
||||
assert any("mask_error_details" in r.message for r in caplog.records)
|
||||
|
||||
async def test_no_warning_when_both_masked(self, caplog):
|
||||
"""No warning when both parent and child have masking enabled."""
|
||||
caplog.set_level(logging.WARNING, logger="fastmcp")
|
||||
parent = FastMCP("parent", mask_error_details=True)
|
||||
child = FastMCP("child", mask_error_details=True)
|
||||
|
||||
@child.tool
|
||||
def my_tool() -> str:
|
||||
return "ok"
|
||||
|
||||
parent.mount(child, "ns")
|
||||
assert not any("mask_error_details" in r.message for r in caplog.records)
|
||||
|
||||
async def test_child_not_mutated_by_mount(self):
|
||||
"""Mounting should not mutate the child server's mask setting."""
|
||||
parent = FastMCP("parent", mask_error_details=True)
|
||||
child = FastMCP("child")
|
||||
|
||||
@child.tool
|
||||
def my_tool() -> str:
|
||||
return "ok"
|
||||
|
||||
parent.mount(child, "ns")
|
||||
assert child._mask_error_details is False
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue