feat(mcp): serve over a host-supplied transport - #716
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
PR SummaryMedium Risk Overview Lifecycle and shutdown are tightened for embedding: a serving generation ( New Reviewed by Cursor Bugbot for commit 2569466. Bugbot is set up for automated code reviews on this repo. Configure here. |
Add `PeekabooMCPServer.serve(transport: any Transport)` for hosts that embed the server instead of spawning the CLI — an application that links `PeekabooCore` and speaks MCP over a connection it owns. The built-in stdio path and the new entry point share one lifecycle: the server runs until the transport completes and the tool context is released on the way out, on success and on failure alike. `serve(transport: TransportType, port:)` keeps its behaviour, including teardown on the not-implemented transports.
528347c to
d8ae61b
Compare
|
Codex review: needs maintainer review before merge. Reviewed September 12, 2026, 9:45 PM ET / September 13, 2026, 01:45 UTC (Revision 6). ClawSweeper reviewWhat this changesAdds a host-supplied MCP transport entry point with shared stdio lifecycle handling, cancellation and cleanup safeguards, regression tests, and embedding documentation. Merge readiness✅ Ready for maintainer review This PR supplies a useful embedding capability absent from current main and v4.3.4. The lifecycle implementation, focused regressions, and recorded native proof support keeping it as a landing candidate; no blocking defect was found. Priority: P2 Review scores
Verification
How this fits togetherPeekaboo’s MCP server exposes desktop automation tools to external clients. The changed lifecycle accepts either standard input/output or a host-owned connection, dispatches tool requests, and releases connection-scoped resources when serving ends. flowchart TD
A[Standard input and output] --> C[MCP server lifecycle]
B[Host-owned connection] --> C
C --> D[Tool-call admission]
D --> E[Desktop automation tools]
C --> F[Completion or cancellation]
F --> G[Disconnect and drain calls]
G --> H[Release tool context]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep the additive embedding entry point on the shared lifecycle, preserving stdio response draining and the documented host-owned connection policy. Do we have a high-confidence way to reproduce the issue? Not applicable as a feature request; source confirms the public embedding entry point is absent from main, and the supplied native transcript demonstrates the added transport lifecycle. Is this the best way to solve the issue? Yes. An additive overload using the SDK transport contract and a shared cleanup path avoids duplicating the MCP server or adding a competing built-in listener. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 6916848ab2fa. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (5 earlier review cycles)
|
Reconcile the contributor transport entry point with current main, prove server-owned cleanup on success and startup failure, and isolate wire-decoder permission fixtures. Co-authored-by: Semyon Kotliarenko <semako.ua@gmail.com>
SDK completion only awaits the receive loop and retains its transport. Stop the SDK session before releasing tool context on both completion and startup failure, with disconnect regressions and live TCP/stdio EOF proof.
Co-authored-by: Semyon Kotliarenko <semako.ua@gmail.com>
Allow applications embedding
PeekabooCoreto serve MCP over a connection they own withPeekabooMCPServer.serve(transport: any Transport). The supplied transport and existing stdio path share startup, completion waiting, SDK disconnection, and tool-context cleanup. Built-in HTTP/SSE transports remain unimplemented.The SDK receive loop does not own incoming tool tasks, so Peekaboo now owns accepted calls explicitly. Shutdown closes admission, cancels and awaits startup, disconnects the transport once, and cancels/drains accepted calls before releasing their context. Competing EOF/cancellation/error paths await one shutdown task. A serving-generation task-local prevents delayed old calls from entering a new lifetime; concurrent
servecalls refuse before connecting a second transport. Hosts must provide cooperative transports and tool services, and use one server per connection.The server retains its isolated snapshot namespace. Tests seed the actual server owner and verify cleanup on completion/startup failure. The wire-decoder fixture uses a deterministic permission provider. In-memory test peers connect before clients initialize because the SDK otherwise drops early messages.
Validation:
PeekabooMCPServerTests,PeekabooMCPHostTransportTests, andEOFDrainingTransportTestspassed. Held-call tests verify cancellation and drain; held-startup and held-disconnect tests verify ordering and exactly one disconnect. The earlier disconnect and accepted-call regressions failed before their repairs.NWListeneraccepted a real TCP connection, then passedNetworkTransportinto the new API. A separate client initialized, listed all 24 tools, and disconnected; both processes exited 0. Accepted connections disable SDK reconnection so peer disconnect ends the serving lifetime.Unreleasedincludes contributor credit. Exact-head hosted checks must pass before merge.Co-authored-by: Semyon Kotliarenko semako.ua@gmail.com