Skip to content

Fix bridge server crash with newer FastMCP versions#38

Open
SteffenPL wants to merge 2 commits into
royerlab:mainfrom
SteffenPL:fix/bridge-server-remove-tool-compat
Open

Fix bridge server crash with newer FastMCP versions#38
SteffenPL wants to merge 2 commits into
royerlab:mainfrom
SteffenPL:fix/bridge-server-remove-tool-compat

Conversation

@SteffenPL

@SteffenPL SteffenPL commented Apr 8, 2026

Copy link
Copy Markdown

Summary

  • NapariBridgeServer.__init__ crashes with AttributeError: 'FastMCP' object has no attribute '_tool_manager' on newer versions of FastMCP
  • Uses the public remove_tool() API instead, with a fallback to the old _tool_manager._tools.pop() for backward compatibility
  • If both methods fail, silently passes (the tools may simply not exist)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Refactor
    • Made lifecycle tool removal during startup more robust: now uses the public removal mechanism with safe fallbacks and quieter error handling to reduce initialization disruptions and noisy logs.

FastMCP no longer exposes `_tool_manager._tools` as a public attribute.
Use the public `remove_tool()` API with a fallback to the old internal
API for backward compatibility.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Apr 8, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0891d2d8-572b-459c-b70e-a9ad8b1ad0cf

📥 Commits

Reviewing files that changed from the base of the PR and between 1a70065 and d2693c6.

📒 Files selected for processing (1)
  • src/napari_mcp/bridge_server.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/napari_mcp/bridge_server.py

📝 Walkthrough

Walkthrough

The NapariBridgeServer.__init__ lifecycle tool removal for close_viewer and init_viewer now first calls the public self.server.remove_tool(name). If that method is missing it falls back to self.server._tool_manager._tools.pop(name, None). If remove_tool exists but raises, the exception is caught and debug-logged instead of propagating.

Changes

Cohort / File(s) Summary
Tool Removal Robustness
src/napari_mcp/bridge_server.py
Replace direct internal deletion with a preferred call to self.server.remove_tool(name); fall back to _tool_manager._tools.pop(name, None) when remove_tool is absent; catch-and-debug-log any other exceptions from remove_tool instead of letting them surface.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 I tried the door that’s meant to be,
A gentle knock, a public key.
If locks refuse or answers stray,
I slip a window, hop away.
Debugging carrots pave the way. 🥕✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title clearly and concisely describes the main change: fixing a bridge server crash with newer FastMCP versions, which directly matches the core issue and solution in the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/napari_mcp/bridge_server.py (1)

122-128: Narrow exception handling and consider adding debug logging.

The fallback logic is reasonable for backward compatibility, but can be improved:

  1. (AttributeError, Exception) is redundant—Exception already covers AttributeError. The second catch on line 124 should catch only AttributeError since the _tool_manager._tools.pop() call won't raise exceptions with the default None.
  2. Catching broad Exception in the first try-except (line 123) could mask unexpected errors. Since remove_tool() is now a stable API in FastMCP v2.3.4+, consider catching only AttributeError (for older versions) or letting other exceptions propagate to aid troubleshooting.
  3. Silent failure provides no feedback if removal fails unexpectedly. Adding a debug log when both approaches fail would help with future troubleshooting.
♻️ Optional: Improved exception handling with logging
+        import logging
+        logger = logging.getLogger(__name__)
+
         # Remove lifecycle tools that should not be available in bridge mode
         for name in ("close_viewer", "init_viewer"):
             try:
                 self.server.remove_tool(name)
-            except (AttributeError, Exception):
+            except AttributeError:
+                # remove_tool() not available in FastMCP < v2.3.4
                 try:
                     self.server._tool_manager._tools.pop(name, None)
-                except (AttributeError, Exception):
-                    pass
+                except AttributeError:
+                    logger.debug("Could not remove tool %r using any method", name)
+            except Exception as e:
+                logger.debug("remove_tool(%r) failed unexpectedly: %s", name, e)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/napari_mcp/bridge_server.py` around lines 122 - 128, Replace the broad
exception handling around tool removal so only AttributeError is caught for
backward-compatibility: attempt self.server.remove_tool(name) and except
AttributeError try the fallback self.server._tool_manager._tools.pop(name,
None); if that also raises AttributeError or returns None/logically indicates
failure, emit a debug log (via self.logger or module logger) describing the tool
name and failure; do not catch Exception globally so other errors from
remove_tool propagate for visibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/napari_mcp/bridge_server.py`:
- Around line 122-128: Replace the broad exception handling around tool removal
so only AttributeError is caught for backward-compatibility: attempt
self.server.remove_tool(name) and except AttributeError try the fallback
self.server._tool_manager._tools.pop(name, None); if that also raises
AttributeError or returns None/logically indicates failure, emit a debug log
(via self.logger or module logger) describing the tool name and failure; do not
catch Exception globally so other errors from remove_tool propagate for
visibility.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8560aa5b-f1a6-46d3-9d7e-e6a3eb97111d

📥 Commits

Reviewing files that changed from the base of the PR and between 44b90a7 and 1a70065.

📒 Files selected for processing (1)
  • src/napari_mcp/bridge_server.py

Address review feedback: catch only AttributeError for backward
compatibility instead of broad Exception, and log debug messages
when tool removal fails.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@SteffenPL

Copy link
Copy Markdown
Author

CodeRabbit feedback is integrated, ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant