Repository navigation
feat: resolve room-scoped MCP servers per connection - #896
Conversation
Source-Base: 490e2d7 Source-Head: 3b1354f Upstream-PR: band-ai#896
Source-Base: 490e2d7 Source-Head: 3b1354f Upstream-PR: band-ai#896
Source-Base: 490e2d7 Source-Head: 3b1354f Upstream-PR: band-ai#896
Source-Base: 490e2d7 Source-Head: 3b1354f Upstream-PR: band-ai#896
Source-Base: 490e2d7 Source-Head: 3b1354f Upstream-PR: band-ai#896
Source-Base: 490e2d7 Source-Head: 3b1354f Upstream-PR: band-ai#896
AlexanderZ-Band
left a comment
There was a problem hiding this comment.
Two lifecycle defects reproduced at the ACP setup boundary. The review also notes one documentation formatting nit.
|
Proposed next steps: split the shared lifecycle change into a separate core PR and keep this PR open for ACP MCP resolution.
The core design still needs to be settled before coding. This split does not add MCP resolver features to other adapters. |
3b1354f to
421c586
Compare
Copilot and Cursor through the ACP client adapter need the room's current MCP server set when a connection starts; a process-wide snapshot cannot follow refreshed credentials or per-room servers. An optional async resolver runs inside the shared, room-owned session initializer, freezing the external server snapshot per setup while keeping Band's reserved endpoint and the agent's advertised transport capabilities. Setup callers share one owned initializer; cancellation and failed close keep custody of the native runtime until cleanup settles, and an accepted caller is never cancelled by its own child's cleanup request.
46f8144 to
a2af1c4
Compare
AlexanderZ-Band
left a comment
There was a problem hiding this comment.
One verified process-closure regression at the reviewed head.
AlexanderZ-Band
left a comment
There was a problem hiding this comment.
One nonblocking duplication nit. The buffered-stdout teardown regression is already recorded in its existing thread and is not duplicated here.
|
The requested ownership split is fulfilled at The PR description matches the resulting ACP scope. INT-1906 and INT-1907 remain independent TypeScript follow-ups. Verification in this review: 25 focused ownership/setup/error tests passed, and the buffered-stdout reproduction was independently confirmed against the exact base. That inline finding remains open; this is not a readiness or merge approval. |
|
The earlier ownership-split request is complete: #911 merged into main, and #896 inherits its completion-only |
Why
ACP rooms need workspace-specific external MCP servers, resolved on the connection that will use them. Shared setup must stay owned through caller cancellation and cleanup.
What
Add optional
resolve_mcp_servers(workspace)to the ACP client adapter and its Copilot/Cursor constructors. Validate native descriptors against negotiated transports, preserve Band routing, and reuse one copied snapshot for restore/fresh-session fallback. Join shared setup throughRetainedRoom.wait_owned, observe abandoned errors once without exposing exception contents, and retain process custody until child exit and stdout EOF are confirmed.Scope
ACP resolver, session setup, process cleanup, documentation and regressions under INT-1889. Uses the shared Python retention contract merged in #911 (INT-1905), inherited from main; this PR contains no generic retention changes. TypeScript parity and planning guidance remain separate follow-ups INT-1906 and INT-1907. No idle-retention enablement or resolver rollout to other Python adapters.
Also repairs a pre-existing resync test's synchronization and async activity fixture exposed during CI validation; generic runtime behavior is unchanged.