Init client and show errors in all clients #108
Conversation
mcp_proxy_for_aws/proxy.py
Outdated
| async def __aexit__(self, exc_type, exc_val, exc_tb): | ||
| """The MCP Proxy for AWS project is a proxy from stdio to http (sigv4). | ||
| We want the client to remain connected in the until the stdio connection is closed. |
| else: | ||
| raise http_error | ||
|
|
||
| async def __aexit__(self, exc_type, exc_val, exc_tb): |
There was a problem hiding this comment.
not necessary imo, __aexit__ is async context manager protocol method, not a method specific to the parent class.
mcp_proxy_for_aws/proxy.py
Outdated
| There is no equivalent of the streamble-http DELETE concept in stdio to terminate a session. | ||
| Hence the connection will be terminated only at program exit. | ||
| """ | ||
| # return await super().__aexit__(exc_type, exc_val, exc_tb) |
There was a problem hiding this comment.
will remove the commented line.
| self._client_factory.set_init_params(context.message) | ||
| return await call_next(context) | ||
| except Exception: | ||
| logger.exception('Initialize failed in middleware.') |
There was a problem hiding this comment.
do we want to log the exact error?
There was a problem hiding this comment.
exception logs the latest error automatically
| body = await response.aread() | ||
| jsonrpc_msg = JSONRPCMessage.model_validate_json(body).root | ||
| except Exception: | ||
| logger.debug('HTTP error is not a valid MCP message.') |
There was a problem hiding this comment.
Do we want to log the exception here?
mcp_proxy_for_aws/proxy.py
Outdated
| """Initialize a client factory with transport.""" | ||
| self._transport = transport | ||
| self._client: AWSMCPProxyClient | None = None | ||
| self._clients: list[AWSMCPProxyClient] = [] |
There was a problem hiding this comment.
When would we have >1 client?
There was a problem hiding this comment.
So this is part of the fix by @arnewouters, when client is in a disconnect state, we will create a new client and push to the list.
There was a problem hiding this comment.
I don't think we need to keep track of this list, can't we immediately disconnect the old client and replace it with the new one in get_client?
There was a problem hiding this comment.
There was a problem hiding this comment.
The old client might be used by a concurrent request.
| async def run_proxy(args) -> None: | ||
| """Set up the server in MCP mode.""" | ||
| logger.info('Setting up server in MCP mode') | ||
| logger.info('Setting up mcp proxy server to %s', args.endpoint) |
There was a problem hiding this comment.
Since you are already editing this, can you also log the current version of the proxy, should help with debugging when logs are shared.
4 participants
Summary
This PR refactors the MCP proxy initialization logic to improve error handling and code organization. The main changes extract proxy logic into a dedicated module and implement initialization as middleware.
The way error message is surfaced is by sending JSONRPCError back to the client. Then wait for client to close the stdin stream and terminate the proxy process. This is the stdio transport specification.
Changes
User experience
Customer is able to see error message in Kiro CLI.
Checklist
If your change doesn't seem to apply, please leave them unchecked.
Is this a breaking change? (Y/N)
Please add details about how this change was tested.
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.