fix(tools): raise on failure so isError=true on the MCP wire#300
Open
sanjibani wants to merge 1 commit into
Open
fix(tools): raise on failure so isError=true on the MCP wire#300sanjibani wants to merge 1 commit into
sanjibani wants to merge 1 commit into
Conversation
6 @mcp.tool() handlers in src/windows_mcp/tools/{clipboard,filesystem,
notification,process,registry,shell}.py caught
and returned a formatted "Error: ..." string. FastMCP wraps the
return value as success content with isError=false, so MCP clients
(Claude Code, Cursor, etc.) treat the failure as data and the LLM
often proceeds as if the call had succeeded.
This is the same isError-compliance gap flagged in the recent MCP
security audit (Dayna Blackwell, 'Tool Poisoning, Rug Pulls, and
Prompt Injections — oh my!').
Each is replaced with bare so the
original exception propagates and FastMCP sets isError=true on the
wire while preserving the formatted message in content for the LLM.
The pre-validation returns (e.g. 'Error: content parameter is required
for write mode.') are kept as-is — those run before any try/except and
represent input-validation rejections, not exception swallowing.
Regression test: tests/test_iserror_compliance.py uses monkeypatch to
force the underlying Windows service to raise, then asserts each tool
handler raises ToolError. Test will run on Windows CI (this codebase
depends on pywin32 which is Windows-only).
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Member
|
Can you show a demo without this fix and after the fix |
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
2 participants
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
6
@mcp.tool()handlers insrc/windows_mcp/tools/{clipboard,filesystem,notification,process,registry,shell}.pycaughtexcept Exception as e:and returned a formatted"Error: ..."string. FastMCP wraps the return value as success content withisError=false, so MCP clients treat the failure as data and the LLM often proceeds as if the call had succeeded.This is the same
isError-compliance gap flagged in the recent MCP security audit (Dayna Blackwell, Tool Poisoning, Rug Pulls, and Prompt Injections — oh my!).Changes
Each
return f'Error ...'is replaced with bareraiseso the original exception propagates and FastMCP setsisError=trueon the wire while preserving the formatted message incontentfor the LLM.The pre-validation returns (e.g.
"Error: content parameter is required for write mode.") are kept as-is — those run before any try/except and represent input-validation rejections, not exception swallowing.Tests
tests/test_iserror_compliance.pyusesmonkeypatchto force the underlying Windows service to raise, then asserts each tool handler raisesToolError. The test will run on Windows CI (this codebase depends onpywin32which is Windows-only; the test module is skipped whenfastmcpis unavailable on non-Windows).Trade-off
Bare
raisekeeps the FastMCPisError=truewire semantics. The exception type is preserved so any upstream exception handler (logging, metrics) that was relying onisinstance(e, SomeError)continues to work.Reference: https://composio.dev/blog/mcp-security-vulnerabilities