Repository navigation
feat(core): watch MCP server events and wake the session - #63178
adamdotdevin wants to merge 4 commits into
Conversation
Supports MCP Events (SEP-3415) push delivery: the client declares the io.modelcontextprotocol/events extension, lists a server's event types (events/list) and follows events/stream. A new opencode.watch_mcp_event tool lists event types, or watches one for the calling session and wakes it with Session.synthetic on each event. Behind the mcp_events flag.
There was a problem hiding this comment.
Works as described: the new tests pass against the real MCP client, and core type-checks with tsc. I didn't do an end-to-end CLI run against a real MCP server.
Public API: adds the opencode_watch_mcp_event tool and the mcp_events flag. The MCP client now also declares the io.modelcontextprotocol/events extension to every server, including when the flag is off. All of this is backwards compatible. The events additions on Mcp.Interface and McpClient.Connection are internal to core.
Concerns (details inline):
- Long-lived watches are cut off by the tool-call execution timeout.
- After its single reopen, a watch can end without waking the session.
- Minor: the events capability is declared even with
mcp_eventsoff. Gating it on the flag would keep the change inert when the flag is disabled.
Related: #48855
| StreamResult, | ||
| { | ||
| signal, | ||
| timeout: execution, |
There was a problem hiding this comment.
events/stream is meant to stay open indefinitely, but it uses the tool-call execution timeout. For attached servers that's a fixed 10 minutes. For stdio and remote servers it's whatever timeout.execution is set to, and users often set a few minutes. When it expires, the SDK rejects with "Request timed out", and the watch stops and wakes the session with that error. A once: false watch, or a once watch on a slow job, dies after that long. Could the stream go without a request timeout, or treat a timeout like a clean close and reopen from the cursor?
There was a problem hiding this comment.
Good catch. events/stream now uses the longest timeout setTimeout allows (about 24.8 days) instead of the tool-call one, and there's a test that keeps a stream open past a 50 ms execution timeout. fwiw I left the capability declaration ungated: gating it means threading the flag through the MCP layer, connection and pool, and declaring it changes nothing until the tool opens a stream.
There was a problem hiding this comment.
The fix in 6c29c70 works.
- Timeout:
events/streamnow passesSTREAM_TIMEOUT = 2 ** 31 - 1(client.ts:73, used at:510). In the SDK version core uses (@modelcontextprotocol/client@2.4.0), that value goes straight into a singlesetTimeout. That timer doesn't reset on progress, and nomaxTotalTimeoutapplies unless the caller sets one, so nothing else cuts the stream off sooner. Past ~24.8 days the request still times out and the watch ends with an error instead of reopening. That limit is fine. - Test: on
pr/head, all 4 tests intest/mcp-events.test.tspass. I changed the timeout back toexecutionand only "keeps a stream open past the server's execution timeout" failed, so the test does guard the fix. - Ungated capability: I'm fine with that. Declaring the extension only tells servers the client understands
events/*. Nothing is sent and no notification is routed until the tool opens a stream, and only the tool is behind the flag.
I didn't check remote streams over a real HTTP transport. A proxy or the server's idle timeout can still drop a long-lived SSE response. That shows up as the stream closing or failing, which the reopen-from-cursor path covers (up to three closes in a row with no event), so it isn't blocking.
I ran this in a temporary worktree at pr/head and removed it afterwards. I didn't touch repo/ or its uncommitted bun.lock change.
| ) | ||
| yield* follow().pipe( | ||
| // A server that closes the stream itself, as when it restarts in place, is followed once more. | ||
| Stream.concat(Stream.suspend(follow)), |
There was a problem hiding this comment.
This follows a server-closed stream only once. If the server closes it a second time (say it restarts twice), the stream ends cleanly, nothing reaches Effect.catch, and the watch stops without waking the session, even though the session was told not to poll. A clean end should either keep reopening (with a bound or backoff) or wake the session with a "watch stopped" message like the error path does.
Related: line 114 sets cursor = event.cursor even when an event has no cursor, which clears the caller's cursor. A later reopen then starts from "now" and can skip events. Keeping the previous value when event.cursor is undefined would avoid that.
There was a problem hiding this comment.
Fixed. A clean close now reopens from the last cursor after 500 ms, and after 3 closes in a row with no event in between it stops and wakes the session with the same "watch stopped" message. An event without a cursor keeps the previous one now too.
There was a problem hiding this comment.
I checked this in 6c29c70 and it's fixed.
- Clean close:
follow()reopens after 500 ms using the last cursor. Every event setsclosesback to 0, and when the count passes 3 the watch fails with "the server kept closing the stream". That goes through the existingEffect.catch, so the session gets the "watch stopped" message. One small detail: the check is++closes > 3, so the server can close the stream 3 times and get 3 reopens, and the watch only stops on the 4th close in a row. The test asserts this behaviour too (the stop arrives aswakes[5]), so the code and the test agree. Only your description says 3 closes. - Cursor: the cursor is only updated when the event has one (
if (event.cursor !== undefined) cursor = event.cursor). The reopen test checks it: the reopen after a cursor-less event still sendsc2. - Reopen timing:
events.streamis built onStream.callback, so nothing is sent until the stream runs. That means the reopen request goes out after the 500 ms sleep, with the cursor as it stands then. - Execution timeout: you also took
events/streamoff it (STREAM_TIMEOUT = 2**31-1). That looks right: before this, a long-lived watch would have been cut off by the tool-call timeout.
I ran bun test test/mcp-events.test.ts against pr/head in a temporary worktree and all 4 tests passed. I didn't run the full mcp suite or typecheck. I didn't change anything in repo/; the bun.lock modification there was already present and I left it alone.
events/stream no longer uses the tool-call execution timeout. A watch reopens a stream the server closes from the last cursor, and stops (waking the session) after three closes in a row without an event. An event without a cursor keeps the previous one.
There was a problem hiding this comment.
Both earlier points are addressed. The stream no longer uses the execution timeout. A closed stream is now reopened from the last cursor and gives up, waking the session, after three closes in a row with no event in between. An event without a cursor keeps the previous one. I checked this with bun test test/mcp-events.test.ts (4/4 pass) and bun run typecheck:tsc in core (passes). The public API is unchanged since the last review: it adds the opencode_watch_mcp_event tool and the mcp_events flag, and declares the MCP events client extension, all backwards compatible.
One thing in the tests (inline): the reconnect case is now hard to follow and spends about 3 s in real 500 ms waits.
|
|
||
| // The server ends the stream itself, as when it restarts in place: follow it from the last cursor, until | ||
| // it has closed it three times in a row without an event. | ||
| for (const index of [0, 1, 2, 3, 4, 5]) { |
There was a problem hiding this comment.
This loop is hard to follow: an event is sent at index 3, and the expected cursor flips on index < 3. Each reopen also waits the real 500 ms, so the test spends about 3 s sleeping. Could it check just the core cases: one reopen from the last cursor, the counter resetting after an event, and giving up after three closes in a row? Making the delay something the test can shorten, or using it.effect with TestClock, would keep it fast and less timing-dependent.
There was a problem hiding this comment.
Split it out into its own it.effect test on TestClock that checks one reopen from the last cursor, the count resetting after an event, and the watch stopping on the third close in a row, which is now what the code does too.
There was a problem hiding this comment.
I checked b29040640 and 6c29c70f0 on pr/head, and the change does what you describe.
-
Code:
++closes >= 3fails the watch on the third close in a row (mcp-events.ts:113).closes = 0inrunForEachresets the count after each event. The comment above it now matches. -
Test: the new
it.effecttest walks through the steps in order:- One close reopens the stream from cursor
c1. An event with no cursor keeps the last one. - An event resets the count.
- After that, two closes reopen from
c3, and the third stops the watch with "the server kept closing the stream". - It ends with exactly 4 streams.
The 500 ms sleeps run on TestClock through
advance. The leftoverit.livetest still covers replay, session deletion and termination, so nothing from the old loop was lost. - One close reopens the stream from cursor
What I ran:
- On a temporary detached worktree of
pr/head,bun run test test/mcp-events.test.tspassed 5 out of 5, five runs in a row, in about 1.2 s. - I broke the code two ways and the new test failed both times: going back to
> 3, and removing thecloses = 0reset.
One small point you can ignore: when it fails, it fails at advance's 15 s deadline with "condition never became true", not on a specific expect.
I didn't run typecheck or the full test/mcp suite. I removed the worktree afterwards and didn't change repo/. The bun.lock modification already there is someone else's and I left it alone.
A watch now stops on the third close in a row, matching its comment.
There was a problem hiding this comment.
The test point is addressed. The reconnect case is now its own short test driven by TestClock, and the file runs in about a second. The >= 3 change matches the documented "three closes in a row" behaviour. The other two points were already fixed by the previous push. I ran bun test test/mcp-events.test.ts three times (5/5 each time) and bun run typecheck:tsc in core (passes). Public API: the regenerated client types and OpenAPI document now add the optional mcp_events boolean to the config flags. Together with the opencode_watch_mcp_event tool and the MCP events client extension, this is all backwards compatible. No new concerns.
|
Superseded: this landed in two co-authored PRs. #63254 adds the MCP Events client (with an |
Issue for this PR
None.
Type of change
What does this PR do?
MCP Events (SEP-3415, a draft extension) lets an MCP server offer event types a client can subscribe to, such as a finished job or a new message. This adds push delivery (
events/stream) to the MCP client and one tool,opencode.watch_mcp_event:event, it lists the event types connected servers offer (events/list).serverandevent(plus optionalargumentsandcursor), it opens a stream for the calling session. Each event wakes the session throughSession.synthetic, with a message ID derived from the event ID, so a replayed event is admitted once.once(the default) ends the watch after the first event.The client declares
extensions["io.modelcontextprotocol/events"]and only sendsevents/*to servers that advertise it. Stream notifications are keyed by the request's JSON-RPC id, which the SDK doesn't expose, so eachevents/streamcarries a token in_metaand the id is read from the outgoing message.Behind the
mcp_eventsflag.How did you verify your code works?
test/mcp-events.test.tsruns the real MCP client against an in-memory server: listing, aoncewatch (wake, then cancel), a replayed event, reopening from the cursor, session deletion, termination, and concurrent watches routed to the right sessions. I broke each behavior to check its assertion fails. Inpackages/core,bun typecheck,bun run typecheck:tscandbun run test test/mcp(185 tests) pass.Screenshots / recordings
Not a UI change.
Checklist
Added by the opencode bot (AI-generated).
Related: #48855
Public API: adds the
mcp_eventsconfig flag (in the client types and OpenAPI) and theopencode_watch_mcp_eventtool, and declares the MCP events client extension (backwards compatible)