Skip to content

fix sse stream - #3

Merged
matheusfrancisco merged 1 commit into
mainfrom
fix-sse
Aug 6, 2026
Merged

fix sse stream#3
matheusfrancisco merged 1 commit into
mainfrom
fix-sse

Conversation

@matheusfrancisco

Copy link
Copy Markdown
Collaborator

No description provided.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix SSE GET-stream 405 messaging and MCP revision classification

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Return an actionable error when SSE GET stream is rejected with HTTP 405.
• Classify MCP Legacy vs Modern using _meta (not just the version header).
• Add regression tests for streamable-http hinting and protocol-version edge cases.
Diagram

graph TD
  Client["MCP client"] --> Gateway["Gateway classify()"] --> Meta{{"_meta modern?"}}
  Meta -- "no" --> Legacy["Legacy path"]
  Meta -- "yes" --> HeaderCheck{{"Header matches _meta?"}} --> Modern["Modern path"]
  Modern --> SSE["SSEBackend Start"] --> Endpoint[("Backend endpoint")]
  Endpoint -- "GET stream 405" --> ActionErr["Error: set transport=streamable-http"]
  subgraph Legend
    direction LR
    _comp["Component/handler"] ~~~ _dec{{"Decision"}} ~~~ _ext[("Endpoint")]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Auto-detect and fallback to streamable-http on 405
  • ➕ Zero-config recovery when an endpoint is actually streamable-http
  • ➕ Avoids operator intervention for a common misconfiguration
  • ➖ Surprising behavior change (transport silently changes semantics)
  • ➖ Harder to debug if both transports are reachable but behave differently
  • ➖ May mask genuine server-side routing/method issues
2. Preflight transport detection (probe capabilities)
  • ➕ Can provide a clearer error before starting the long-lived stream
  • ➕ Could validate config at startup and fail fast
  • ➖ Adds extra round-trips and complexity
  • ➖ Capability probing is not always reliable without a spec-defined endpoint

Recommendation: Keep the PR’s explicit-config approach: improving the 405 error message and tightening revision classification yields correct behavior without hidden fallbacks. Consider an opt-in auto-fallback later if operator friction remains high.

Files changed (4) +106 / -15

Bug fix (2) +36 / -15
sse.goHint streamable-http when SSE GET stream returns 405 +10/-0

Hint streamable-http when SSE GET stream returns 405

• Adds a special-case for HTTP 405 during SSE stream establishment. The returned error now explicitly indicates the endpoint is likely streamable-http and instructs setting 'transport: streamable-http', preventing confusing downstream 502-style failures.

backend/sse.go

revision.goDecide Modern vs Legacy from _meta, not version header alone +26/-15

Decide Modern vs Legacy from _meta, not version header alone

• Adjusts protocol-era classification so 'MCP-Protocol-Version' is not treated as a Modern signal by itself; only '_meta' controls Modern routing. Adds validation for Legacy-shaped requests echoing unknown versions, and simplifies the header mismatch check to compare against '_meta' when present.

gateway/revision.go

Tests (2) +70 / -0
sse_test.goTest 405 GET stream produces streamable-http hint +21/-0

Test 405 GET stream produces streamable-http hint

• Introduces a regression test that simulates a backend returning HTTP 405 with a 'use POST' JSON-RPC error body. Verifies the surfaced error includes guidance to switch to streamable-http.

backend/sse_test.go

revision_test.goAdd tests for Legacy echoed version header and modern-meta requirements +49/-0

Add tests for Legacy echoed version header and modern-meta requirements

• Adds coverage for Legacy clients that echo negotiated versions (including the claude-code scenario) to ensure they are not misclassified as Modern. Also tests that unknown echoed versions are rejected for the right reason, and that Modern routing requires '_meta' rather than a header-only claim.

gateway/revision_test.go

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

@matheusfrancisco
matheusfrancisco merged commit 9b66782 into main Aug 6, 2026
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant