Let FastMCPError propagate unchanged from managers#2697
Conversation
|
Merging, this needs the uvicorn 0.39 fixes in #2696 |
WalkthroughFastMCPError is added to the public exception imports across three manager modules: prompt_manager, resource_manager, and tool_manager. Exception handling logic in each module is updated to specifically re-raise FastMCPError without custom processing, while general exception handling paths remain intact with existing error masking behavior preserved. This change unifies exception propagation behavior across the managers. Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/fastmcp/tools/tool_manager.py (1)
161-164: LGTM: Exception handling correctly preserves FastMCPError and pydantic validation errors.The exception handling logic correctly:
- Re-raises all
FastMCPErrorsubclasses (includingfastmcp.exceptions.ValidationError) unchanged- Re-raises
pydantic.ValidationError(imported at line 8) unchanged- Preserves existing error masking behavior for all other exceptions
The dual handling ensures both FastMCP-specific errors and pydantic validation errors propagate with their original messages intact, which aligns with the PR objectives.
Optional: Add clarifying comment
To make the intent more explicit, consider adding a brief comment distinguishing the pydantic ValidationError:
except FastMCPError: raise + # pydantic ValidationError (distinct from fastmcp.exceptions.ValidationError) except ValidationError: raise
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/fastmcp/prompts/prompt_manager.pysrc/fastmcp/resources/resource_manager.pysrc/fastmcp/tools/tool_manager.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use Python ≥ 3.10 with full type annotations
Never use bareexcept- be specific with exception types
Files:
src/fastmcp/resources/resource_manager.pysrc/fastmcp/prompts/prompt_manager.pysrc/fastmcp/tools/tool_manager.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-21T21:37:55.031Z
Learning: Applies to src/fastmcp/__init__.py : All module exports should be intentional - only re-export to `fastmcp.*` for fundamental types like `FastMCP` and `Client`, prefer users importing from specific submodules for specialized features
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: .cursor/rules/core-mcp-objects.mdc:0-0
Timestamp: 2025-11-26T21:51:44.174Z
Learning: Review and update related Manager classes (ToolManager, ResourceManager, PromptManager) when modifying MCP object definitions
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-21T21:37:55.031Z
Learning: Applies to src/fastmcp/**/__init__.py : Core types that define a module's purpose should be exported (e.g., `Middleware` from `fastmcp.server.middleware`), while specialized features can live in submodules
📚 Learning: 2025-12-21T21:37:55.031Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-21T21:37:55.031Z
Learning: Applies to src/fastmcp/__init__.py : All module exports should be intentional - only re-export to `fastmcp.*` for fundamental types like `FastMCP` and `Client`, prefer users importing from specific submodules for specialized features
Applied to files:
src/fastmcp/resources/resource_manager.pysrc/fastmcp/prompts/prompt_manager.pysrc/fastmcp/tools/tool_manager.py
📚 Learning: 2025-11-26T21:51:44.174Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: .cursor/rules/core-mcp-objects.mdc:0-0
Timestamp: 2025-11-26T21:51:44.174Z
Learning: Review and update related Manager classes (ToolManager, ResourceManager, PromptManager) when modifying MCP object definitions
Applied to files:
src/fastmcp/resources/resource_manager.pysrc/fastmcp/prompts/prompt_manager.pysrc/fastmcp/tools/tool_manager.py
📚 Learning: 2025-12-21T21:37:55.031Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-21T21:37:55.031Z
Learning: Applies to src/fastmcp/**/__init__.py : Core types that define a module's purpose should be exported (e.g., `Middleware` from `fastmcp.server.middleware`), while specialized features can live in submodules
Applied to files:
src/fastmcp/prompts/prompt_manager.py
🧬 Code graph analysis (3)
src/fastmcp/resources/resource_manager.py (1)
src/fastmcp/exceptions.py (3)
FastMCPError(6-7)NotFoundError(34-35)ResourceError(14-15)
src/fastmcp/prompts/prompt_manager.py (1)
src/fastmcp/exceptions.py (3)
FastMCPError(6-7)NotFoundError(34-35)PromptError(22-23)
src/fastmcp/tools/tool_manager.py (2)
src/fastmcp/server/context.py (1)
fastmcp(169-174)src/fastmcp/exceptions.py (3)
FastMCPError(6-7)ToolError(18-19)ValidationError(10-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run tests: Python 3.13 on ubuntu-latest
- GitHub Check: Run tests: Python 3.10 on windows-latest
- GitHub Check: Run tests: Python 3.10 on ubuntu-latest
- GitHub Check: Run tests with lowest-direct dependencies
🔇 Additional comments (7)
src/fastmcp/tools/tool_manager.py (1)
11-11: LGTM: FastMCPError import added.The addition of
FastMCPErrorto the imports enables the enhanced exception propagation behavior described in the PR objectives.src/fastmcp/prompts/prompt_manager.py (2)
10-10: LGTM: FastMCPError import added.The addition of
FastMCPErrorto the imports is consistent with the pattern applied across all manager classes in this PR.
110-111: LGTM: Exception handling correctly preserves FastMCPError.The exception handling logic correctly re-raises all
FastMCPErrorsubclasses (includingPromptError) unchanged, while preserving the existing error masking behavior for other exceptions. This ensures that FastMCP-specific errors propagate with their original messages intact.src/fastmcp/resources/resource_manager.py (4)
13-13: LGTM: FastMCPError import added.The addition of
FastMCPErrorto the imports enables consistent exception propagation behavior across the resource manager.
271-273: LGTM: Exception handling correctly preserves FastMCPError in template creation.The exception handling logic correctly re-raises all
FastMCPErrorsubclasses unchanged when creating resources from templates, while preserving the existing error masking behavior for other exceptions.
301-303: LGTM: Exception handling correctly preserves FastMCPError in resource reads.The exception handling logic correctly re-raises all
FastMCPErrorsubclasses unchanged when reading resources directly, while preserving the existing error masking behavior for other exceptions.
323-324: LGTM: Exception handling correctly preserves FastMCPError in template reads.The exception handling logic correctly re-raises all
FastMCPErrorsubclasses unchanged when reading resources from templates, while preserving the existing error masking behavior for other exceptions. This completes the consistent exception propagation pattern across all resource manager code paths.
Test Failure AnalysisSummary: The test failure in workflow run 20473894870 is unrelated to the changes in this PR. Root Cause: The test Evidence:
This aligns with @jlowin's comment mentioning "uvicorn 0.39 fixes in #2696" - the test failure is related to that dependency issue, not the FastMCPError propagation changes in this PR. Suggested Solution: Merge #2696 first (uvicorn 0.39 fixes), then this PR should pass CI cleanly. Detailed AnalysisTest Failure DetailsThe failing test: Error: The test itself passes, but uvicorn's lifespan manager has a pending task that prevents clean shutdown: VerificationI tested both the PR branch and the parent commit:
This confirms the issue existed before this PR. Files Changed by This PRThe PR only modified exception handling in three manager files:
None of these changes affect HTTP server teardown or uvicorn lifecycle management. Related FilesFailing test: Manager files (modified by this PR):
Relevance: All manager tests pass; changes are working as intended |
Previously, managers only re-raised their specific error types (e.g.,
ToolErrorin ToolManager,ResourceErrorin ResourceManager). This meant that otherFastMCPErrorsubclasses likefastmcp.exceptions.ValidationErrorwould get wrapped, losing their original message.Now all managers catch
FastMCPError(the base class) and re-raise it unchanged, allowing any FastMCP error type to propagate with its original message intact.