fix: namespace resource links in tool results#4156
Conversation
jlowin
left a comment
There was a problem hiding this comment.
Thanks for the focused regression test here. I think this needs one small correction before merge: the ResourceLink copy currently bypasses Pydantic validation and stores the rewritten URI as a plain string.
In fastmcp_slim/fastmcp/server/transforms/namespace.py, _transform_content_block() does:
return block.model_copy(update={"uri": self._transform_uri(str(block.uri))})model_copy(update=...) does not validate the updated value, so ResourceLink.uri becomes str instead of AnyUrl. Pydantic then emits serializer warnings when the block is dumped. Please wrap the transformed value with mcp.types.AnyUrl(...) or otherwise rebuild the ResourceLink through validation so the field keeps the expected type.
|
It looks like this introduced some significant test regressions as well. Please make sure to run tests and check all behavior before opening a PR |
|
tl;dr: 8 unit tests fail across every matrix job (3.10/3.13, Linux/Windows, lowest-direct). The new Root Cause: In
Fix: The URI rewrite needs to happen without replacing the tool's execution model. Options to consider:
Either way, the regression test from this PR ( Failure summary (Python 3.10, ubuntu-latest)Same failure set on Relevant files
🤖 Generated with Claude Code |
83bd911 to
d331588
Compare
|
Updated the PR to address the review/CI regression. What changed:
Validation run locally on Windows:
Note: on this Windows environment, pytest needs |
72e30e8 to
fc1c64f
Compare
|
Rebased onto current Local validation on Windows:
|
fc1c64f to
33d17cf
Compare
|
Rebased onto current Local validation on Windows:
The latest branch keeps the |
33d17cf to
574b4bc
Compare
|
Rebased onto current Local validation on Windows:
|
|
Thanks for chasing this and for iterating after the CI feedback. The behavior is right, but the implementation is solving too much of the framework inside The amount of code here is out of proportion to the use case because it is filling an abstraction gap. This should either be a reusable result-transforming tool abstraction, or an extension of The regression test is useful and should carry forward. I am closing this PR so the fix can be shaped around that abstraction instead of continuing with a one-off wrapper. |
Summary
Fixes #4154.
To verify