Conversation
🦋 Changeset detectedLatest commit: 9c6fb27 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
jmorrell-cloudflare
changed the title
jmorrell-cloudflare
changed the title
|
|
||
| it("should reject invalid session ID", async () => { | ||
| const ctx = createExecutionContext(); | ||
| const sessionId = await initializeServer(ctx); |
There was a problem hiding this comment.
This is unused in the rest of the test and can be safely removed without affecting the test logic
| async setInitialized() { | ||
| await this.ctx.storage.put("initialized", true); | ||
| } | ||
|
|
||
| async isInitialized() { | ||
| return (await this.ctx.storage.get("initialized")) === true; |
There was a problem hiding this comment.
This is a little odd because this is only used by the streamable transport, and not the SSE transport.
| if (!this.ctx.storage.get("transportType")) { | ||
| await this.ctx.storage.put("transportType", "unset"); | ||
| } |
There was a problem hiding this comment.
Without this guard this would be overwritten when the DO wakes up. The complexity of our "boot" logic is higher than I would like.
| const isInitialized = await doStub.isInitialized(); | ||
|
|
||
| if (isInitializationRequest) { | ||
| await doStub._init(ctx.props); |
There was a problem hiding this comment.
I have a question about this change. ctx is no longer used in static serve. So there doesn't seem to be a way to provide additional context from the original request to init when defining your MCP tools (e.g., header values)?
Also, wasn't the OAuth flow relying on that? https://github.com/cloudflare/workers-oauth-provider/blob/main/src/oauth-provider.ts#L1857
See my comment here for additional details: #194 (comment)
There was a problem hiding this comment.
Good catch! You're absolutely right. Let me add that back in
There was a problem hiding this comment.
Note that this is only an issue if you need to access the results of auth in your MCP tools. I think the auth flow itself will still work.
3 participants
Fixes an error introduced in #186
The first the the Streamable worker does on an incoming request with a
sessionIdis ask the DO if it's been initialized. If it has not, then the DO has never received an initialize message, and we should reject it.In the current version once a DO hibernates, this always returns
false.However, we're conflating two different types of "initialization" here:
McpAgentclass to allow the user to define theMcpServerThis fixes the issue by having an explicit
setInitialized()method, and tracking the second kind of authentication in DO storage.There was also an additional error with the
this.init?.()call where it was notawait'd. Once you await you get errors because it is being run twice and trying to define the same tool twice. These were being silently ignored.