Repository navigation
mcp: read a colon-less SSE line as a field with an empty value - #1374
Merged
guglielmo-san merged 2 commits intoOct 12, 2026
Merged
guglielmo-san merged 2 commits into
guglielmo-san merged 2 commits into
Conversation
The SSE parsing rules treat a line with no colon as a field name whose value is the empty string, so an unknown such line is ignored and a bare "data" line adds an empty data line. scanEventsLimited instead reported errMalformedEvent, which processStream considers terminal: one stray line failed the whole client session.
chrikrah
approved these changes
Oct 11, 2026
chrikrah
left a comment
There was a problem hiding this comment.
@gu-feng418 this is right, and I would merge it. The session-level repro from #1373 fails on the merge base and passes at f2ce77c, with the full suite green on both trees.
# go 1.27.1 linux/amd64; head f2ce77c, merge base 944ac19
$ go test ./mcp/ -run TestProbeColonlessSSELine -count=1 -v # base 944ac19
PROBE extra="heartbeat"
PROBE extra="data"
--- FAIL: TestProbeColonlessSSELine/heartbeat
--- FAIL: TestProbeColonlessSSELine/data
FAIL github.com/modelcontextprotocol/go-sdk/mcp
$ go test ./mcp/ -run 'TestProbeColonlessSSELine|TestStreamableClientConnect_ColonlessSSELine' -count=1 -v # head f2ce77c
--- PASS: TestProbeColonlessSSELine
--- PASS: TestStreamableClientConnect_ColonlessSSELine
ok github.com/modelcontextprotocol/go-sdk/mcp 0.020s
$ go build ./... && go test ./... -count=1
# head f2ce77c: exit 0, 14 packages ok
# base 944ac19: exit 0, 14 packages ok
non-blocking: SSEClientTransport shares this scanner, and the body says so, but the new session test covers only StreamableClientTransport. One case on the legacy transport would pin that half of the claim.
@guglielmo-san #1373 asked whether the scanner should follow the SSE rules, and the two cases you rewrote in mcp/event_test.go are that decision. Do you want the spec reading, or should a colon-less line keep failing?
guglielmo-san
approved these changes
Oct 12, 2026
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
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.
The SSE parsing rules read a line with no
:as a field name whose value isthe empty string, so an unknown such line is ignored and a bare
datalinecontributes an empty data line.
scanEventsLimitedinstead reportederrMalformedEventfor it, andprocessStreamtreats that error as terminaland calls
c.fail, so one stray line ended the whole client session. Thelegacy
SSEClientTransportshares the same scanner.This matters for gateways and proxies that interleave a non-standard keepalive
line into an event stream: the connection is unusable after a single line that
the spec defines as valid.
The scanner now takes the whole line as the field name and an empty value, and
the existing switch below keeps ignoring unrecognized names. Comment lines
(starting with
:) and the size cap are untouched, soerrMalformedEventisstill reported for an event that exceeds
maxEventSize.Two cases in
mcp/event_test.goasserted the old rejection, so they now assertthe spec reading;
TestStreamableClientConnect_ColonlessSSELinecovers thesession-level repro from the issue.
Fixes #1373