Return more compatible SSE endpoint#455
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates the SSE endpoint format to be more standards-compliant by returning /sse?sessionId=<id> instead of just ?sessionId=<id>. This change improves compatibility with clients that may not handle the previous endpoint format well.
Key Changes:
- Implemented URL construction logic to create proper SSE endpoints with full path and query parameters
- Added utility function
modify_urlfor converting between URIs and URLs to manipulate query parameters - Updated SSE service to use the new endpoint format when establishing connections
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/agentgateway/src/mcp/sse.rs | Updates SSE endpoint construction to use full path with query parameters |
| crates/agentgateway/src/http/mod.rs | Adds utility function for URL manipulation and URI/URL conversion |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| anyhow::bail!("no authority"); | ||
| } | ||
| if !url.has_host() { | ||
| anyhow::bail!("no host"); |
There was a problem hiding this comment.
The error messages 'no authority' and 'no host' are not descriptive enough for debugging. Consider including the URL in the error message, e.g., 'URL has no authority: {url}' and 'URL has no host: {url}'.
| anyhow::bail!("no authority"); | |
| } | |
| if !url.has_host() { | |
| anyhow::bail!("no host"); | |
| anyhow::bail!("URL has no authority: {}", url.as_str()); | |
| } | |
| if !url.has_host() { | |
| anyhow::bail!("URL has no host: {}", url.as_str()); |
| }) { | ||
| return http_error( | ||
| StatusCode::INTERNAL_SERVER_ERROR, | ||
| format!("fail to create SSE url: {e}"), |
There was a problem hiding this comment.
The error message contains a grammatical error. 'fail to create' should be 'failed to create'.
| format!("fail to create SSE url: {e}"), | |
| format!("failed to create SSE url: {e}"), |
Before: `?sessionId=23cab33d-8f1e-46cf-902a-9793335ab90f` After: `/sse?sessionId=23cab33d-8f1e-46cf-902a-9793335ab90f` A proper SDK supports both... but some clients are not great. The latter is more standard.
58fc400 to
9c81525
Compare
Before: `?sessionId=23cab33d-8f1e-46cf-902a-9793335ab90f` After: `/sse?sessionId=23cab33d-8f1e-46cf-902a-9793335ab90f` A proper SDK supports both... but some clients are not great. The latter is more standard.
Before:
?sessionId=23cab33d-8f1e-46cf-902a-9793335ab90fAfter:
/sse?sessionId=23cab33d-8f1e-46cf-902a-9793335ab90fA proper SDK supports both... but some clients are not great. The latter is more standard.
Fixes #166