-
Notifications
You must be signed in to change notification settings - Fork 22
write initialize error to stdout #95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
312c93d to
9b58283
Compare
mcp_proxy_for_aws/server.py
Outdated
| logger.error('Failed to start server: %s', e) | ||
| cause = e.__cause__ | ||
| if isinstance(cause, McpProxy) and cause.payload: | ||
| print(cause.payload, file=sys.stdout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to print the initialization error to the process stdout so that the client can receive it and show the error message properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit/qq: Maybe we want to do this also via the logger but point it to sys.stdout only in this case.
9b58283 to
10b44b3
Compare
10b44b3 to
8d26bf9
Compare
mcp_proxy_for_aws/exceptions.py
Outdated
| """Error which will be handled by the proxy writing the payload to stdout.""" | ||
|
|
||
| def __init__(self, payload: str | None = None, *args: object) -> None: | ||
| """Consturct a MCP proxy error with payload.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo here.
mcp_proxy_for_aws/sigv4_helper.py
Outdated
| if response.status_code == 401 or response.status_code == 403: | ||
| request = await response.request.aread() | ||
| request_body = json.loads(request.decode()) | ||
| if isinstance(request_body, dict) and request_body.get('method') == 'initialize': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this thrown only on initialize? I presume because other MCP operations would normally return 200 if they are implemented according to the spec. But we can't make that assumption especially as the proxy can be leveraged by any non-AWS service specific MCP server (for example an AgentCore Runtime MCP server would be feasible to use with the proxy).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would normally return 200 if they are implemented according to the spec.
Note, this is wrong. If done by spec, e.g. when a SessionId is expired the server must return 404 if its passed.
However I do agree this is not really respected properly 😓
| name='MCP Proxy', | ||
| instructions=( | ||
| 'MCP Proxy for AWS Server that provides access to backend servers through a single interface. ' | ||
| 'This proxy handles authentication and request routing to the appropriate backend services.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: SigV4 authentication and we don't really do any routing.
mcp_proxy_for_aws/server.py
Outdated
| logger.error('Failed to start server: %s', e) | ||
| cause = e.__cause__ | ||
| if isinstance(cause, McpProxy) and cause.payload: | ||
| print(cause.payload, file=sys.stdout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit/qq: Maybe we want to do this also via the logger but point it to sys.stdout only in this case.
mcp_proxy_for_aws/server.py
Outdated
| cause = e.__cause__ | ||
| if isinstance(cause, McpProxyError) and cause.payload: | ||
| print(cause.payload, file=sys.stdout) | ||
| raise e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: raise
| raise e | |
| raise |
mcp_proxy_for_aws/sigv4_helper.py
Outdated
| if response.status_code == 401 or response.status_code == 403: | ||
| request = await response.request.aread() | ||
| request_body = json.loads(request.decode()) | ||
| if isinstance(request_body, dict) and request_body.get('method') == 'initialize': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would normally return 200 if they are implemented according to the spec.
Note, this is wrong. If done by spec, e.g. when a SessionId is expired the server must return 404 if its passed.
However I do agree this is not really respected properly 😓
mcp_proxy_for_aws/sigv4_helper.py
Outdated
| error_details = response.json() | ||
| logger.log(log_level, 'HTTP %d Error Details: %s', response.status_code, error_details) | ||
| except Exception: | ||
| if response.status_code == 401 or response.status_code == 403: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we do this on any 4xx / 5xx errors per default and ignore failures for maybe GET requests?
e.g. allowlist for know "ok" failures like GET requests instad of this surgical way?
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the error handling outside of sigv4 helper
8d26bf9 to
db7e54e
Compare
b540a25 to
d777d6d
Compare
d777d6d to
c6a9177
Compare
| # line = sys.stdin.readline() | ||
| # logger.debug('First line from kiro %s', line) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # line = sys.stdin.readline() | |
| # logger.debug('First line from kiro %s', line) |
Co-authored-by: Leonardo Araneda Freccero <arangatang@users.noreply.github.com> Signed-off-by: wzxxing <169175349+wzxxing@users.noreply.github.com>
Summary
Changes
When client fails, we want to the see the failure reason in the MCP client. This was possible with v1.0 where every request is independent, but with v1.1.1, to reuse the MCP session for every request, proxy takes an already connected mcp client, so the client initialization happens outside of the MCP client life cycle.
User experience
The user should see a more detailed error message such as below, immediately after the connection is closed:
Previously v1.1.1 make the error message very unclear:
Testing
update MCP config as
Expired credentials (Authn failure):
Credentials without permission (Authz faillure)
Tool call failure (Authz), normal without terminating the transport
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.