chore: upgrade fastmcp to 3.4.2#29
Open
mskry wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughUpgrades ChangesFastMCP 3.4.2 upgrade and server re-wiring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/diagrams_mcp/tools/plantuml.py (1)
7-7: Align ToolError import with coding guideline for tool functions.Line 7 imports
ToolErrorviadiagrams_mcp.fastmcp_compat, but the coding guideline requiresfastmcp.exceptions.ToolErrorfor error handling in tool functions. While the compat layer works, all tool modules in this package currently use this pattern (mermaid.py, equivalence.py, discovery.py, render.py). Consider refactoring to import directly fromfastmcp.exceptionsto align with the specified guideline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/diagrams_mcp/tools/plantuml.py` at line 7, The import statement on line 7 currently imports ToolError from diagrams_mcp.fastmcp_compat, but the coding guideline requires importing ToolError directly from fastmcp.exceptions to align with the pattern used in other tool modules (mermaid.py, equivalence.py, discovery.py, render.py). Refactor the import statement to separate the imports: keep Image imported from diagrams_mcp.fastmcp_compat but import ToolError directly from fastmcp.exceptions instead.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/diagrams_mcp/image_store.py`:
- Around line 333-334: The PDF inline path using binary_file() at line 333-334
is unreachable because the condition at line 294 forces download_link = True for
all non-PNG formats, causing execution to return early at line 318. To fix this,
modify the condition at line 294 that sets download_link = True to exclude PDF
format (or conditionally skip the early return at line 318 for PDF), so that PDF
requests can proceed to the binary_file() call instead of taking the forced
download path.
In `@src/diagrams_mcp/server.py`:
- Around line 57-83: The expensive rendering tools render_diagram,
render_mermaid, and render_plantuml are registered with mcp.add_tool() without
any rate limiting protection, leaving them vulnerable to saturation and abuse.
Add server-side throttling by implementing a middleware-based rate limiting
layer or configuring an upstream policy to restrict request frequency for these
resource-intensive operations. Ensure the throttling mechanism is enforced
before these tools execute to prevent abuse and maintain system stability.
In `@tests/test_sandbox.py`:
- Line 260: The assertion on line 260 performs inconsistent case handling where
"timed out" is checked against msg.lower() but "Diagram code failed" is checked
against the original msg without case normalization. To fix this, normalize the
second alternative by changing the check to also use msg.lower(), so both "timed
out" and "diagram code failed" (in lowercase) are checked against the lowercased
message string. This ensures the assertion handles all message variations
consistently regardless of capitalization.
---
Nitpick comments:
In `@src/diagrams_mcp/tools/plantuml.py`:
- Line 7: The import statement on line 7 currently imports ToolError from
diagrams_mcp.fastmcp_compat, but the coding guideline requires importing
ToolError directly from fastmcp.exceptions to align with the pattern used in
other tool modules (mermaid.py, equivalence.py, discovery.py, render.py).
Refactor the import statement to separate the imports: keep Image imported from
diagrams_mcp.fastmcp_compat but import ToolError directly from
fastmcp.exceptions instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c9920bfa-22bc-4649-8963-297bd902ff24
📒 Files selected for processing (22)
pyproject.tomlsrc/diagrams_mcp/fastmcp_compat.pysrc/diagrams_mcp/image_store.pysrc/diagrams_mcp/prompts.pysrc/diagrams_mcp/renderer.pysrc/diagrams_mcp/resources.pysrc/diagrams_mcp/sandbox.pysrc/diagrams_mcp/server.pysrc/diagrams_mcp/tools/discovery.pysrc/diagrams_mcp/tools/equivalence.pysrc/diagrams_mcp/tools/mermaid.pysrc/diagrams_mcp/tools/plantuml.pysrc/diagrams_mcp/tools/render.pytests/test_discovery.pytests/test_equivalence.pytests/test_image_store.pytests/test_mermaid.pytests/test_plantuml.pytests/test_render.pytests/test_renderer.pytests/test_sandbox.pytests/test_server.py
Comment on lines
+333
to
+334
| if fmt == "pdf": | ||
| return binary_file(data, f"{filename}.pdf", _FORMAT_MAP[fmt]["mime"]) |
There was a problem hiding this comment.
PDF inline path is unreachable due earlier forced download mode.
At Line 294, any non-PNG format sets download_link = True, so PDF requests return at Line 318 and never hit the new binary_file(...) branch. This breaks the new EmbeddedResource contract for PDF responses.
Suggested fix
- if fmt != "png":
+ if fmt == "svg":
download_link = True
- elif len(data) > ANTHROPIC_INLINE_IMAGE_MAX_BYTES:
+ elif fmt == "png" and len(data) > ANTHROPIC_INLINE_IMAGE_MAX_BYTES:
download_link = True🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/diagrams_mcp/image_store.py` around lines 333 - 334, The PDF inline path
using binary_file() at line 333-334 is unreachable because the condition at line
294 forces download_link = True for all non-PNG formats, causing execution to
return early at line 318. To fix this, modify the condition at line 294 that
sets download_link = True to exclude PDF format (or conditionally skip the early
return at line 318 for PDF), so that PDF requests can proceed to the
binary_file() call instead of taking the forced download path.
Comment on lines
+57
to
+83
| mcp.add_tool(render_diagram, annotations={"readOnlyHint": True}, structured_output=False) | ||
| mcp.add_tool(list_providers, annotations={"readOnlyHint": True, "idempotentHint": True}) | ||
| mcp.add_tool(list_services, annotations={"readOnlyHint": True, "idempotentHint": True}) | ||
| mcp.add_tool(list_nodes, annotations={"readOnlyHint": True, "idempotentHint": True}) | ||
| mcp.add_tool(search_nodes, annotations={"readOnlyHint": True, "idempotentHint": True}) | ||
| mcp.add_tool(find_equivalent, annotations={"readOnlyHint": True, "idempotentHint": True}) | ||
| mcp.add_tool(list_categories, annotations={"readOnlyHint": True, "idempotentHint": True}) | ||
| mcp.add_tool(render_mermaid, annotations={"readOnlyHint": True}, structured_output=False) | ||
| mcp.add_tool(render_plantuml, annotations={"readOnlyHint": True}, structured_output=False) | ||
|
|
||
| mcp.resource("diagrams://reference/diagram", mime_type="text/markdown")(diagram_reference) | ||
| mcp.resource("diagrams://reference/edge", mime_type="text/markdown")(edge_reference) | ||
| mcp.resource("diagrams://reference/cluster", mime_type="text/markdown")(cluster_reference) | ||
| mcp.resource("diagrams://reference/mermaid", mime_type="text/markdown")(mermaid_reference) | ||
| mcp.resource("diagrams://reference/plantuml", mime_type="text/markdown")(plantuml_reference) | ||
|
|
||
| mcp.mount(render) | ||
| mcp.mount(discovery) | ||
| mcp.mount(references) | ||
| mcp.mount(mermaid) | ||
| mcp.mount(plantuml) | ||
| mcp.mount(equivalence) | ||
| mcp.mount(prompts_app) | ||
| mcp.prompt(description="Guide the user through building a cloud architecture diagram")( | ||
| architecture_diagram | ||
| ) | ||
| mcp.prompt(description="Guide creation of a sequence or flow diagram")(sequence_flow) | ||
| mcp.prompt(description="Walk through multi-cloud service comparison")(compare_providers) | ||
| mcp.prompt( | ||
| description=( | ||
| "Minimal-friction path: describe what to visualize and the best engine is" | ||
| " picked automatically" | ||
| ) | ||
| )(quick_sketch) |
There was a problem hiding this comment.
Reintroduce request throttling after the rewiring.
The previous middleware/mount-based rate limiting was removed, and this registration path does not add a replacement. That leaves expensive render tools (render_diagram, render_mermaid, render_plantuml) unthrottled and increases saturation/abuse risk. Please add server-side throttling (middleware or enforced upstream policy) before release.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/diagrams_mcp/server.py` around lines 57 - 83, The expensive rendering
tools render_diagram, render_mermaid, and render_plantuml are registered with
mcp.add_tool() without any rate limiting protection, leaving them vulnerable to
saturation and abuse. Add server-side throttling by implementing a
middleware-based rate limiting layer or configuring an upstream policy to
restrict request frequency for these resource-intensive operations. Ensure the
throttling mechanism is enforced before these tools execute to prevent abuse and
maintain system stability.
| msg = str(exc_info.value) | ||
| assert "Diagram code failed" in msg, f"Expected CPU-limit kill, got: {msg}" | ||
| assert "timed out" not in msg.lower(), f"Got wall-clock timeout instead of CPU limit: {msg}" | ||
| assert "timed out" in msg.lower() or "Diagram code failed" in msg, msg |
There was a problem hiding this comment.
Normalize both alternatives to lowercase in the assertion.
Line 260 mixes case-normalized and case-sensitive checks, which makes this test fail on harmless capitalization changes.
Proposed fix
- msg = str(exc_info.value)
- assert "timed out" in msg.lower() or "Diagram code failed" in msg, msg
+ msg = str(exc_info.value)
+ lowered = msg.lower()
+ assert "timed out" in lowered or "diagram code failed" in lowered, msg🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_sandbox.py` at line 260, The assertion on line 260 performs
inconsistent case handling where "timed out" is checked against msg.lower() but
"Diagram code failed" is checked against the original msg without case
normalization. To fix this, normalize the second alternative by changing the
check to also use msg.lower(), so both "timed out" and "diagram code failed" (in
lowercase) are checked against the lowercased message string. This ensures the
assertion handles all message variations consistently regardless of
capitalization.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Summary by CodeRabbit
Chores
fastmcpdependency from version 3.2.3 to 3.4.2.Improvements