feat: fetch transport #1209
feat: fetch transport #1209
Conversation
commit: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
mattzcarey
force-pushed
the
feat-fetch-transport
branch
from
62eb340 to
cbaa269
Compare
…with fetch-based transport
- Fix IsomorphicHeaders type mismatch in capturedHeaders variable - Fix replayEventsAfter mock signature to match EventStore interface - Remove unused variables (closeSSEStreamFn, mcpServer1)
mattzcarey
force-pushed
the
feat-fetch-transport
branch
from
cbaa269 to
31b824d
Compare
- Reorder devDependencies alphabetically in package.json - Update imports to use experimental/index.js instead of nested paths - Move EventStore import from streamableHttp.js to stores.js - Relocate FetchStreamableHTTPServerTransport tests to /test directory
KKonstantinov
left a comment
KKonstantinov
left a comment
There was a problem hiding this comment.
Hi, left a few comments when diffing with the original streamableHttp
| return; | ||
| } | ||
|
|
||
| const primingEventId = await this._eventStore.storeEvent(streamId, {} as JSONRPCMessage); |
There was a problem hiding this comment.
Hi, the protocol version check is missing here - this will break legacy clients - needs to be carried over to this transport.
| // Validate Origin header if allowedOrigins is configured | ||
| if (this._allowedOrigins && this._allowedOrigins.length > 0) { | ||
| const originHeader = req.headers.get('origin'); | ||
| if (!originHeader || !this._allowedOrigins.includes(originHeader)) { |
There was a problem hiding this comment.
This appears to differ from what is in the original streamableHttp -> on there, this only applies if originHeader exists.
| /** | ||
| * Helper to create a JSON error response | ||
| */ | ||
| private createJsonErrorResponse(status: number, code: number, message: string, headers?: Record<string, string>): Response { |
There was a problem hiding this comment.
The original streamableHttp appears to also add data to some errors, like below:
res.writeHead(400).end(
JSON.stringify({
jsonrpc: '2.0',
error: {
code: -32700,
message: 'Parse error',
data: String(error)
},
id: null
})
);Are we accidentally losing this functionality here?
|
|
||
| let rawMessage; | ||
| try { | ||
| rawMessage = await req.json(); |
There was a problem hiding this comment.
On the original streamableHttp, we have:
const body = await getRawBody(req, {
limit: MAXIMUM_MESSAGE_SIZE,
encoding: parsedCt.parameters.charset ?? 'utf-8'
});E.g. we're losing the 4mb maximum message size here, let's discuss if we want to keep it or get rid of it, but it is a difference in behavior
| return this.createJsonErrorResponse(415, -32000, 'Unsupported Media Type: Content-Type must be application/json'); | ||
| } | ||
|
|
||
| const requestInfo: RequestInfo = { |
There was a problem hiding this comment.
We have omitted authInfo here. I think let's keep it for backwards compatibility if we merge now, and remove it in v2 with a breaking change? Else if we want to remove it now in this transport, document it very explicitly so our users don't expect it when switching to this transport.
felixweinberger
left a comment
felixweinberger
left a comment
There was a problem hiding this comment.
+1 to @KKonstantinov's comments to be sure what we provide matches spec and existing features.
A more high level point: I'm a bit worried about introducing an additional transport (even if marked "experimental" - people are still going to import it) that is IIUC mostly duplicated code from the existing streamableHttpServerTransport with some changes at the edges to use the fetch API instead of Node API.
I think the motivation is valid - we want to meet people where they are if they want to use Web Standards. I wonder if this duplication is the right strategy though, as this essentially duplicates maintenance effort for any changes to the streamable http spec (and any bug fixes, e.g. some of the stuff @KKonstantinov pointed out).
I see we added nodeRequestToWebRequest() and webResponseToNodeResponse() adapters in the express example - have we considered going with an "adapters only" approach rather than duplicating much of the transport code for example?
| export interface SessionState { | ||
| /** Whether the session has completed initialization */ | ||
| initialized: boolean; | ||
| } |
There was a problem hiding this comment.
I see the motivation for this, but I think this might be somewhat pre-empting what decisions come out of modelcontextprotocol/modelcontextprotocol#1442 which has a lot of trade-offs to consider on how to make the protocol stateless by default. A SessionStore concept is only one of many options that might end up being the right solution:
Again, I guess experimental is our way out of this, but this might literally need to be deleted depending on what comes out of that. If we ship this and people start depending on it we'll lock ourselves in a bit in terms of what breaking changes we can realistically make on the next spec release?
web standards based streamable http transport.
marked experimental
closes #260
Motivation and Context
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context