fix(http): stop SSE fallback from masking primary transport errors - #311
Conversation
Gate ordinary-path Streamable HTTP -> SSE fallback on 404/405 transport mismatch only, so generic network failures on streamable-only servers no longer trigger a legacy GET that reports a misleading 405. When a legitimate SSE fallback still fails, rethrow the primary error with the SSE failure as cause. Preserve 401/OAuth promote paths. Closes openclaw#310 Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Codex review: needs real behavior proof before merge. Reviewed August 12, 2026, 8:03 PM ET / August 13, 2026, 00:03 UTC. ClawSweeper reviewWhat this changesThe PR limits Streamable HTTP-to-legacy-SSE fallback to HTTP 404/405 mismatches and attaches failed fallback diagnostics to the initial connection error. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 7 items remain Keep open: the PR fixes the reported masking case but regresses the documented and tested generic-error SSE compatibility path, and it still hides the actual SSE failure after a confirmed mismatch. Priority: P2 Review scores
Verification
How this fits togethermcporter connects HTTP-configured MCP servers by trying Streamable HTTP first, then using legacy SSE for compatible older servers. That connection decision determines both which protocol client reaches the server and which failure operators see in the CLI. flowchart LR
A[Configured HTTP MCP server] --> B[Streamable HTTP connection]
B --> C{Primary result}
C -->|Connects| D[Usable MCP client]
C -->|Legacy-compatible failure| E[Legacy SSE connection]
C -->|Other failure| F[Primary diagnostic]
E --> D
E --> G[Fallback diagnostic]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep generic-error fallback for existing legacy compatibility, preserve the primary error only when that fallback also fails, and surface the SSE error when a confirmed 404/405 mismatch makes SSE the selected transport. Do we have a high-confidence way to reproduce the issue? Yes—source is sufficient to reproduce the pre-PR contract: current main’s focused transport tests make a generic primary failure succeed through SSE, whereas the proposed branch stops after one attempt. Is this the best way to solve the issue? No—the requested diagnostic fix can preserve generic fallback compatibility; only error selection after a failed fallback needs tightening, with a distinct mismatch-versus-SSE-failure test. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 4fdb2916720b. LabelsLabel justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (2 earlier review cycles)
|
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix(http): stop SSE fallback from masking primary transport errors This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
What Problem This Solves
When Streamable HTTP connect fails for a generic network reason (
fetch failed, DNS stall, connect timeout), mcporter still fell back to legacy SSE. Against streamable-HTTP-only servers, that fallback GET commonly returns 405 and replaced the real primary error with a misleading SSE failure.Summary
Closes #310.
Real behavior proof
Built from this PR head and exercised against real local HTTP endpoints through the public runtime API:
origin/main: request trace wasPOST /mcp, thenGET /mcp; surfacedSSE error: Non-200 status code (405).POST /mcp; surfaced the originalTypeError: fetch failedwith native causeother side closed.POST /mcp, thenGET /mcp;SSE error: Non-200 status code (503), proving legitimate mismatch fallback remains active and its operational failure is preserved.Regression proof
The pending-header and delayed-header proofs from #282 remain green:
Focused transport and composability coverage:
Full repository gates: