Skip to content

Conversation

@billyhickamzn
Copy link

@billyhickamzn billyhickamzn commented Jan 28, 2026

The proxy implementations do not currently support SSE formatting.

Description of changes:

Tests pass and run locally to ensure proxy behaviour works as expected.

Comment on lines 168 to 169
// Check if it's a notification by looking for "method" field
if (jsonData.contains("\"method\"")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Parse into a Document and then extract the "method" field. You're looking for a top-level JSON key named "method" but the string "method" can ostensibly appear anywhere in the payload.

StringBuilder dataBuffer = new StringBuilder();

for (String line : lines) {
if (line.startsWith("data: ")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The space after data: is optional

Comment on lines 146 to 150
String responseSessionId = response.headers().firstValue("Mcp-Session-Id");
if (responseSessionId != null) {
sessionId = responseSessionId;
LOG.debug("Stored session ID from response: {}", responseSessionId);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is only allowed during initialize but we are accepting it during any responses.

Comment on lines 194 to 197
if (isNotification(jsonData)) {
// This is a notification - parse as JsonRpcRequest and forward
JsonRpcRequest notification =
JSON_CODEC.deserializeShape(jsonData, JsonRpcRequest.builder());
Copy link
Contributor

Choose a reason for hiding this comment

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

We are double parsing the JSON here. You can convert the Document to JsonRpcRequest.

Billy Hickman added 2 commits February 2, 2026 11:44
- Respond to list/changed notifications by invalidation currently cached tools.
- Track session ID header values when response recieved from initialize method call.
- Forward notifications to notificaiton writers.
- Handle SSE format (when handling text/event-stream), read line by line.

for (String line : lines) {
if (line.startsWith("data:")) {
dataBuffer.append(line.substring(5).replaceFirst("^ ", ""));
Copy link
Contributor

Choose a reason for hiding this comment

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

replaceFirst compiles and evaluates a regular expression, which is expensive. Can we do a cheaper if (line.length() > 6 && Charater.isWhitespace(line.charAt(5)) check instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

I took a different approach. We can't use Charater.isWhitespace because the SSE spec only allows a U+0020 space. https://html.spec.whatwg.org/multipage/server-sent-events.html#parsing-an-event-stream

@rhernandez35 rhernandez35 enabled auto-merge (rebase) February 2, 2026 21:47
@rhernandez35 rhernandez35 merged commit 7d46bd0 into smithy-lang:main Feb 2, 2026
2 of 4 checks passed
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.

3 participants