Skip to content

fix: use the endpoint announced by MCP SSE servers - #509

Open
AnKiT-GaRG2 wants to merge 2 commits into
CraftOS-dev:devfrom
AnKiT-GaRG2:fix/mcp-sse-endpoint-discovery
Open

AnKiT-GaRG2 wants to merge 2 commits into
CraftOS-dev:devfrom
AnKiT-GaRG2:fix/mcp-sse-endpoint-discovery

Conversation

@AnKiT-GaRG2

Copy link
Copy Markdown

Fixes #508

Problem

SSETransport POSTed requests to self.url.replace("/sse", "/message") and ignored the endpoint event the MCP SSE spec says a server sends first. Servers with a different message path, such as the official Python SDK (GET /sse, POST /messages/?session_id=...), returned 404, so initialize timed out after 30s and the server failed to connect.

Changes (agent_core/core/impl/mcp/server.py)

  • _listen_sse now tracks event: lines and reads the endpoint event. The announced URL is resolved against the SSE URL and ignored with a warning if its origin differs.
  • connect() waits for the endpoint (ENDPOINT_TIMEOUT, 10s) before sending initialize. If none arrives it falls back to the previous guessed URL, so servers that never sent an endpoint event keep working.
  • Sends notifications/initialized after initialize, as the stdio and websocket transports already do.

Not in scope: headers and streamable HTTP (#185).

Testing

  • New tests/test_mcp_sse_transport.py (4 tests, in-process spec-style server): POSTs to the announced endpoint, sends notifications/initialized, falls back to the guess when no endpoint is announced, and ignores a cross-origin endpoint. The first one fails on the old code.
  • ruff check passes on the changed files.
  • Manual: MCPServerConnection against the official MCP Python SDK server (mcp==1.30.0).
    • Before: connect() -> False in 30.1s, server saw POST /message 404.
    • After: connect() -> True in 0.2s, tool add discovered, server saw POST /messages/?session_id=... 202.
  • Not tested: MCP SDK 2.x servers, and a server that rejects requests until it receives notifications/initialized.

@ahmad-ajmal ahmad-ajmal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Two interoperability issues with endpoint discovery are noted inline.

Comment thread agent_core/core/impl/mcp/server.py Resolved
Comment thread agent_core/core/impl/mcp/server.py Resolved

@ahmad-ajmal ahmad-ajmal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Both findings are addressed in 8013ab0. SSE events are buffered until the blank line, and origin comparison now handles hostname case and default ports.

Validation: 43 tests passed, 8 skipped, and Ruff passed. Additional checks for multiline JSON, event state reset, and endpoint acceptance passed. No further actionable findings.

@AnKiT-GaRG2

Copy link
Copy Markdown
Author

Thanks @ahmad-ajmal for the review and the extra checks on multiline JSON and event state reset. Both findings are fixed in 8013ab0. Nothing is outstanding on my side, and the branch is mergeable. Let me know if you want any changes before it goes in.

@ahmad-ajmal

Copy link
Copy Markdown
Collaborator

Thanks @ahmad-ajmal for the review and the extra checks on multiline JSON and event state reset. Both findings are fixed in 8013ab0. Nothing is outstanding on my side, and the branch is mergeable. Let me know if you want any changes before it goes in.

Thank you for contributing. This will be merged to dev when the development cycle of V1.4.5 is completed.

SSETransport POSTed requests to self.url.replace('/sse', '/message')
and ignored the server's first 'endpoint' event. Servers whose message
path differs (e.g. the official Python SDK: /sse and /messages/?session_id=...)
returned 404, so initialize timed out after 30s.

- Read the 'endpoint' event and POST to it. The URL is resolved against the
  SSE URL and ignored if its origin differs.
- Wait for the endpoint before sending initialize; if none arrives within
  ENDPOINT_TIMEOUT, fall back to the previous guessed URL.
- Send notifications/initialized after initialize, as the stdio and
  websocket transports already do.
- Add tests with a spec-style SSE server.

Fixes CraftOS-dev#508
Process an event when its blank line arrives so data: may precede event:,
and treat https://host and https://host:443 as the same origin.
@AnKiT-GaRG2
AnKiT-GaRG2 force-pushed the fix/mcp-sse-endpoint-discovery branch from 8013ab0 to 1704455 Compare November 7, 2026 21:53
@AnKiT-GaRG2

Copy link
Copy Markdown
Author

Rebased onto current dev: the branch had been cut from an older base and had picked up unrelated release commits, which is why it showed conflicts. The PR is now just my two commits (server.py and tests/test_mcp_sse_transport.py), no conflicts. The code is unchanged from the version you approved at 8013ab0. The SSE and stdio MCP tests pass and Ruff is clean. Thanks for the review, @ahmad-ajmal.

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.

2 participants