Skip to content

fix(http): stop SSE fallback from masking primary transport errors - #311

Merged
steipete merged 2 commits into
openclaw:mainfrom
SebTardif:fix/310-sse-fallback-primary-error
Aug 13, 2026
Merged

fix(http): stop SSE fallback from masking primary transport errors#311
steipete merged 2 commits into
openclaw:mainfrom
SebTardif:fix/310-sse-fallback-primary-error

Conversation

@SebTardif

@SebTardif SebTardif commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

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.

  • Ordinary Streamable HTTP errors now surface unchanged and do not trigger legacy SSE.
  • Explicit 404/405 transport mismatches still select the legacy SSE transport.
  • Existing 401/OAuth promotion and fallback paths remain intact.
  • Once a confirmed mismatch selects SSE, a later SSE failure remains the actionable error.

Real behavior proof

Built from this PR head and exercised against real local HTTP endpoints through the public runtime API:

  1. A server that resets the Streamable HTTP POST and would return 405 to a fallback GET:
    • origin/main: request trace was POST /mcp, then GET /mcp; surfaced SSE error: Non-200 status code (405).
    • this PR: request trace was only POST /mcp; surfaced the original TypeError: fetch failed with native cause other side closed.
    • repeated twice with the same POST-only result.
  2. A legacy endpoint that returns 405 to Streamable HTTP and 503 to SSE:
    • request trace was POST /mcp, then GET /mcp;
    • surfaced 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:

$ pnpm exec vitest run tests/cli-idle-sse.integration.test.ts --reporter=verbose
Test Files  1 passed (1)
Tests       2 passed (2)

$ pnpm exec vitest run tests/e2e-fixture-servers.test.ts --reporter=verbose --testTimeout=30000 -t 'headers arrive after the startup grace'
Test Files  1 passed (1)
Tests       1 passed | 14 skipped (15)

Focused transport and composability coverage:

$ pnpm exec vitest run tests/runtime-transport.test.ts tests/runtime-compose.test.ts --reporter=verbose
Test Files  2 passed (2)
Tests       63 passed (63)

Full repository gates:

$ pnpm check
# format, OXLint, and TypeScript checks passed

$ pnpm test
Test Files  191 passed | 4 skipped (195)
Tests       1598 passed | 26 skipped (1624)

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>
@clawsweeper

clawsweeper Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@SebTardif

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@clawsweeper

clawsweeper Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed August 12, 2026, 8:03 PM ET / August 13, 2026, 00:03 UTC.

ClawSweeper review

What this changes

The 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
Reviewed head: cadbe14581c07630cd0809d2529363833cea9e5a

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The patch has a clear target and focused coverage, but two compatibility/diagnostic defects and mock-only proof keep it below merge readiness.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The PR provides focused mocked transport tests and test output, but no after-fix real MCP endpoint run or production-boundary client/server trace. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦐 gold shrimp (3/6) 2 actionable review findings remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The PR provides focused mocked transport tests and test output, but no after-fix real MCP endpoint run or production-boundary client/server trace. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 6 items Existing compatibility contract: Current documentation says HTTP connections try Streamable HTTP first while retaining legacy SSE compatibility.
Current regression coverage: Current main explicitly expects a generic primary connection error to fall back successfully to SSE, and separately tests creation of a legacy client after a generic streamable-transport failure.
Compatibility regression in proposed branch: The new ordinary-path return value aborts fallback for every error except 404/405, while the PR changes the prior generic-failure compatibility tests to assert that fallback no longer occurs.
Findings 2 actionable findings [P1] Preserve fallback for generic primary failures
[P2] Surface the SSE error after a confirmed mismatch
Security None None.

How this fits together

mcporter 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]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The PR provides focused mocked transport tests and test output, but no after-fix real MCP endpoint run or production-boundary client/server trace. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Preserve fallback for generic primary failures (P1) - Current main documents automatic legacy-SSE compatibility and tests successful SSE fallback after generic connection errors. Restricting it to parsed 404/405 responses makes legacy endpoints whose Streamable probe fails with a reset, timeout, or other unclassified error unreachable; retain the fallback and fix the error returned when it fails.
  • Surface the SSE error after a confirmed mismatch (P2) - Once a 404/405 has established that Streamable HTTP is the wrong transport, that error is expected. If the selected SSE connection then fails for a distinct reason, rethrowing the primary mismatch hides the operational failure; return the SSE error with the primary mismatch retained as context instead.
  • Resolve merge risk (P1) - Merging would make some legacy SSE servers unreachable whenever their Streamable HTTP probe fails without a parsed 404/405, despite the documented automatic compatibility path.
  • Resolve merge risk (P2) - After a valid 404/405 negotiation fallback, an operational SSE failure would be presented as the expected primary mismatch rather than the failure that needs remediation.
  • Resolve merge risk (P1) - The supplied validation is mock-only; it does not show the corrected behavior against a real MCP endpoint or production-boundary client/server trace.
  • Complete next step (P2) - The two blockers are a narrow transport error-selection repair with clear existing contract tests; real-endpoint proof remains contributor-provided before merge.

Findings

  • [P1] Preserve fallback for generic primary failures — src/runtime/http-transport.ts:156
  • [P2] Surface the SSE error after a confirmed mismatch — src/runtime/http-transport.ts:360
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus test delta production +39/-5; tests +50/-4 The branch changes the core transport-selection policy and rewrites its compatibility expectations across three files.

Merge-risk options

Maintainer options:

  1. Restore the compatibility matrix (recommended)
    Keep generic primary-error fallback, return the primary error with SSE context only when that fallback also fails, and return the SSE failure after a confirmed 404/405 mismatch.
  2. Pause the behavior change
    Do not merge the narrowed fallback policy unless maintainers intentionally revise the documented legacy-SSE compatibility contract.

Technical review

Best 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:

  • [P1] Preserve fallback for generic primary failures — src/runtime/http-transport.ts:156
    Current main documents automatic legacy-SSE compatibility and tests successful SSE fallback after generic connection errors. Restricting it to parsed 404/405 responses makes legacy endpoints whose Streamable probe fails with a reset, timeout, or other unclassified error unreachable; retain the fallback and fix the error returned when it fails.
    Confidence: 0.98
  • [P2] Surface the SSE error after a confirmed mismatch — src/runtime/http-transport.ts:360
    Once a 404/405 has established that Streamable HTTP is the wrong transport, that error is expected. If the selected SSE connection then fails for a distinct reason, rethrowing the primary mismatch hides the operational failure; return the SSE error with the primary mismatch retained as context instead.
    Confidence: 0.93

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 4fdb2916720b.

Labels

Label justifications:

  • P2: This is a bounded HTTP transport compatibility regression affecting users of legacy SSE servers, without evidence of an emergency-wide outage.
  • merge-risk: 🚨 compatibility: The PR changes the shipped automatic Streamable HTTP-to-SSE fallback contract and can make previously reachable legacy endpoints fail.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR provides focused mocked transport tests and test output, but no after-fix real MCP endpoint run or production-boundary client/server trace. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

Acceptance criteria:

  • [P1] pnpm check.
  • [P1] pnpm exec vitest run tests/runtime-transport.test.ts tests/runtime-compose.test.ts.
  • [P1] pnpm test.

What I checked:

  • Existing compatibility contract: Current documentation says HTTP connections try Streamable HTTP first while retaining legacy SSE compatibility. (docs/protocols.md:13, 4fdb2916720b)
  • Current regression coverage: Current main explicitly expects a generic primary connection error to fall back successfully to SSE, and separately tests creation of a legacy client after a generic streamable-transport failure. (tests/runtime-transport.test.ts:199, 4fdb2916720b)
  • Compatibility regression in proposed branch: The new ordinary-path return value aborts fallback for every error except 404/405, while the PR changes the prior generic-failure compatibility tests to assert that fallback no longer occurs. (src/runtime/http-transport.ts:156, cadbe14581c0)
  • Fallback error remains masked after a confirmed mismatch: After classifying the primary 404/405 as an expected legacy mismatch, the branch always rethrows that expected primary error when SSE fails; a distinct SSE failure is only attached as cause rather than surfaced. (src/runtime/http-transport.ts:360, cadbe14581c0)
  • History and release provenance: Blame attributes the current fallback helper to the v0.13.4 release commit, whose source includes the established fallback behavior; the provided repository context identifies v0.13.4 as the latest release. (src/runtime/http-transport.ts:146, 9b0e998a18bc)
  • Review continuity: The current head is unchanged from the prior completed review, whose two findings were preserving generic-error fallback and reporting the actual SSE failure after a mismatch. (src/runtime/http-transport.ts:156, cadbe14581c0)

Likely related people:

  • Peter Steinberger: Current fallback behavior is attributed by blame to the v0.13.4 release commit, which records this transport implementation. (role: recent transport-area contributor; confidence: medium; commits: 9b0e998a18bc; files: src/runtime/http-transport.ts, tests/runtime-transport.test.ts)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Restore generic-error SSE compatibility and add a focused error-selection matrix for generic versus 404/405 primary failures.
  • Add redacted after-fix terminal, log, or trace evidence from a real MCP endpoint or production-boundary client/server path.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (2 earlier review cycles)
  • reviewed 2026-08-12T22:40:21.203Z sha cadbe14 :: needs real behavior proof before merge. :: [P1] Preserve the existing generic-error SSE fallback
  • reviewed 2026-08-12T22:43:55.188Z sha cadbe14 :: needs real behavior proof before merge. :: [P1] Preserve the generic-error SSE fallback | [P2] Report the actual SSE failure after a mismatch fallback

@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 12, 2026
@clawsweeper

clawsweeper Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

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.

@steipete
steipete merged commit 97a5ee4 into openclaw:main Aug 13, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SSE fallback fires on any primary error, replacing the real cause with a misleading HTTP 405 on streamable-HTTP-only servers

2 participants