Conversation
arnewouters
left a comment
There was a problem hiding this comment.
Don't forget to run/install the pre-commit hooks :)
mcp_proxy_for_aws/server.py
Outdated
| proxy.add_middleware(InitializeMiddleware(client_factory)) | ||
| proxy.add_middleware( | ||
| ConnectionErrorMiddleware( | ||
| tool_call_timeout=args.read_timeout, |
There was a problem hiding this comment.
Do we want to create a new variable for tool call timeouts? Is there another place where read_timeout is used? Trying to understand if this is a new feature or a miss that we are fixing now
There was a problem hiding this comment.
Yes we will need a new variable, the read timeout and the tool timeout are 2 different things
- Add optional --tool-timeout CLI flag to cap tool call duration - Rename error_handling middleware to ToolTimeoutMiddleware - Return graceful isError=True response instead of hanging - Suggest long-lived credentials on 401/403 errors - Document --tool-timeout in README troubleshooting
d7280aa to
092b144
Compare
- Rename class, files, CLI flag (--tool-error-timeout), and all references - Default tool-error-timeout to 300s
mcp_proxy_for_aws/cli.py
Outdated
| ) | ||
|
|
||
| parser.add_argument( | ||
| '--tool-error-timeout', |
There was a problem hiding this comment.
| '--tool-error-timeout', | |
| '--tool-timeout', |
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class _FailedToolResult(ToolResult): |
There was a problem hiding this comment.
Do we really have to define a new class?
There was a problem hiding this comment.
yes
fastmcp's ToolResult doesn't expose isError
Subclassing and overriding to_mcp_result() is the only way to set isError=True
# ToolResult.__init__ — no isError parameter
def __init__(
self,
content: list[ContentBlock] | Any | None = None,
structured_content: dict[str, Any] | Any | None = None,
meta: dict[str, Any] | None = None,
):
# ToolResult.to_mcp_result() — never sets isError
def to_mcp_result(self):
if self.meta is not None:
return CallToolResult(content=self.content, _meta=self.meta) # no isError
if self.structured_content is None:
return self.content # bare list, not even a CallToolResult
return self.content, self.structured_content # tuple, not a CallToolResult
There was a problem hiding this comment.
I believe we can raise a ToolError instead of creating a new class and returning that. It should have isError set.
async def test_specific_tool_errors_are_sent_to_client(self):
mcp = FastMCP("TestServer")
@mcp.tool
def custom_error_tool():
raise ToolError("This is a test error (abc)")
client = Client(transport=FastMCPTransport(mcp))
async with client:
result = await client.call_tool_mcp("custom_error_tool", {})
assert result.isError
assert isinstance(result.content[0], TextContent)
assert "test error" in result.content[0].text
assert "abc" in result.content[0].text
Summary
Changes
ToolErrorMiddlewarethat wraps every tool call with a timeout and error handler, ensuring the proxy never hangs on stale credentials or unresponsive endpoints--tool-timeoutCLI flag (default: 300s) to configure the maximum duration for a tool call--profileoraws sso login--tool-timeoutflagUser experience
When a user's credentials expire mid-session, they now get an actionable error message suggesting
--profileoraws sso logininstead of the client hanging indefinitely. The server will need to be restarted for new credential configuration to take effect.Testing
Testing
isErrorpropagation:Output:
Checklist
Is this a breaking change?
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.