Skip to content

fix!: bind the room to the Band MCP connection so models never supply chat_id - #723

Merged
AlexanderZ-Band merged 20 commits into
mainfrom
fix/fix-bind-the-room-server-side-so-models-never-supp-INT-1607
Oct 5, 2026
Merged

AlexanderZ-Band merged 20 commits into
mainfrom
fix/fix-bind-the-room-server-side-so-models-never-supp-INT-1607

Conversation

@AlexanderZ-Band

@AlexanderZ-Band AlexanderZ-Band commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

Why

Band MCP tools took a model-supplied chat_id, but the ACP client stated the room only once, in the first prompt. Models sometimes left it out (band_store_memory ... chat_id: is required), which broke test_memory_survives_adapter_rehydration[omp_acp] nightly.

What

  • Each room gets its own Band MCP endpoint (/rooms/<id>/mcp or /sse). The connection carries the room, so tools no longer advertise or accept chat_id.
  • The ACP clients (base, OMP, Copilot, Cursor, generic) and claude_sdk connect through these per-room endpoints. claude_sdk moves off the in-process SDK MCP server onto the HTTP backend.
  • The [chat_id: …] prompt prefixes and chat_id guidance are removed, including from copilot_sdk.
  • opencode, letta, the band-mcp CLI and the desktop room view stay on the multi-room endpoint, since one registration there serves every room.

Follow-ups: INT-1636 (per-endpoint auth for LocalMCPServer).

Closes INT-1607
Closes INT-1265

BREAKING CHANGE: the "sdk" Band MCP backend kind and the kind argument/field are removed. Use create_band_mcp_backend(room_bound=True) and BandMCPBackend.endpoint(transport, room_id). BandMCPBackendKind is replaced by BandMCPTransport (HTTP, SSE), and BandMCPBackend is frozen. BandMCPBackend.server is removed (use local_server), and create_band_mcp_backend drops get_participant_handles/tool_result_hook. In band.integrations.mcp.engine, build_custom_tool_registration(room_bound=...) becomes advertise_chat_id=..., and MCPToolRegistration.structured_output is removed: engine tools are served unstructured, with no structuredContent. The Claude SDK tool builders and their aliases (ToolResolver, ParticipantHandlesResolver, ToolResultHook) in band.integrations.claude_sdk.tools are removed.

🤖 Generated with Claude Code

https://claude.ai/code/session_0199vfrhPRYDZbhHtPmdLcZo

… chat_id

A room-bound LocalMCPServer mounts its engine under /rooms/{room_id}; its
tools advertise no chat_id and each call takes its room from the request
path. ACP client sessions and claude_sdk dial their room's endpoint via
BandMCPBackend.endpoint(); opencode and letta keep the multi-room endpoint.
claude_sdk moves off the in-process SDK MCP server to HTTP, and the
chat_id prompt text is dropped wherever the tools are bound.

BREAKING CHANGE: the "sdk" Band MCP backend kind is removed; use
kind="http" (with room_bound=True for per-room endpoints). BandMCPBackend
drops its `server` field (use `local_server`), create_band_mcp_backend drops
get_participant_handles/tool_result_hook, and the Claude SDK tool builders
in band.integrations.claude_sdk.tools are removed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017VPm1kcnGmxdLivQ6BGJj5
@linear-code

linear-code Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

MCPTransportKind aliases BandMCPBackendKind; the ACP prompt builds its
room block under one conditional; tests address room endpoints through
room_endpoint_path and start backends through one started_backend helper.
The copilot_sdk prompt test now asserts the prompt carries no room id.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017VPm1kcnGmxdLivQ6BGJj5
@AlexanderZ-Band

Copy link
Copy Markdown
Collaborator Author

Results, first attempt each, no retries:

  • E2E test_memory_survives_adapter_rehydration: claude_sdk (core) PASSED, omp_acp (backends) PASSED, copilot_acp (backends) PASSED.
  • Unit suite: the first run had 1 failure. tests/adapters/copilot_sdk/test_reply.py::test_prompt_contains_room_context_and_message asserted the [chat_id: …] prefix that this PR removes. The test now asserts the prompt carries no room id. After that change and the /simplify cleanup: 6291 passed, 150 skipped.
  • ruff check, ruff format, pyrefly check: clean.

Comment thread src/band/adapters/claude_sdk.py Outdated
AlexanderZ-Band and others added 5 commits October 2, 2026 10:59
…rt type

Every backend is one LocalMCPServer serving both transports, so `kind` was
never read; consumers pick the transport in `endpoint()`. The one transport
vocabulary is now `BandMCPTransport` (replacing `BandMCPBackendKind` and the
ACP-only `MCPTransportKind`). Docs and examples describe the room-bound
endpoints ACP and claude_sdk now use.

BREAKING CHANGE: `create_band_mcp_backend` and `BandMCPBackend` no longer
take or carry `kind`; `BandMCPBackendKind` is renamed `BandMCPTransport`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017VPm1kcnGmxdLivQ6BGJj5
…tured wrapper

Every engine tool returns its JSON as text, but FastMCP inferred a
`{"result": string}` output schema from the handler's `-> str` and sent the
same text again as `structuredContent: {"result": "<json>"}`. Clients that
prefer structured content (the Claude CLI; Copilot, which the ACP echo
unwrap exists for) showed the model double-encoded JSON. claude_sdk moved
onto the engine in this PR, so its roster lookups started reaching the
model escaped. The engine now builds every tool unstructured, which also
drops the per-registration `structured_output` field it no longer needs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017VPm1kcnGmxdLivQ6BGJj5
- create_band_mcp_backend types get_tools as RoomToolResolver (was Any)
  and takes any Sequence of tool definitions.
- BandMCPBackend is frozen; endpoint()'s room_id defaults to None.
- build_custom_tool_registration takes one advertise_chat_id flag instead
  of a room_bound/room_from_connection pair that described one choice.
- The connection-room reader is private to the engine.
- ClaudeSessionManager's mcp_servers_factory returns the SDK's own
  McpServerConfig mapping (was dict[str, Any]).
- LocalMCPServer's wrong-kind URL error names the accessor to use.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017VPm1kcnGmxdLivQ6BGJj5
…ound trip fits

The deadline is adapter-wide, so the turn after the held one must finish
within it too. Its Band tool calls now cross a real loopback HTTP server,
which a 50 ms deadline did not leave room for on CI runners.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017VPm1kcnGmxdLivQ6BGJj5
Comment thread src/band/adapters/opencode/adapter.py Outdated
A stalled approval turn timed out with a bare TimeoutError: the outer
deadline always beat the inner one and swallowed its message, so a silent
agent and a frame the WebSocket capture missed looked the same. The timeout
now outlines, without message contents, each streamed and durable agent
message's role (request, notice, closing reply, other text) and whether the
durable copy closed. Phase logs mark the captured requests and the moment
the stream settles.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017VPm1kcnGmxdLivQ6BGJj5

@AlexanderZ-Band AlexanderZ-Band left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cycle 1: 3 findings (1 issue / 1 suggestion / 1 nit). Would be REQUEST_CHANGES for the Test Quality gap (missing claude_sdk prompt regression guard for absent chat_id guidance), but GitHub blocks request_changes on own PRs — treating it as blocking for the fix pass anyway.

Comment thread src/band/integrations/claude_sdk/session_manager.py Outdated
Comment thread src/band/adapters/claude_sdk.py Outdated
Comment thread src/band/integrations/claude_sdk/prompts.py
Cycle-1 review: log unexpected engine dispatch failures (FastMCP drops the stack), pin the claude_sdk prompt drift guard, and finish the mcp_servers_factory docstring.

Co-authored-by: Cursor <cursoragent@cursor.com>

@AlexanderZ-Band AlexanderZ-Band left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cycle 2: 3 findings (1 issue / 2 nits). Blocking for the fix pass: kind-mismatch test assertion is too loose.

Comment thread src/band/integrations/mcp/engine.py
Comment thread src/band/integrations/mcp/engine.py Outdated
Comment thread tests/integrations/mcp/test_room_bound_server.py Outdated
Cycle-2 review: pin each endpoint-kind assertion to its expected error fragment, and describe pin_existing_chat_id by what the schema does for both callers.

Co-authored-by: Cursor <cursoragent@cursor.com>

@AlexanderZ-Band AlexanderZ-Band left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cycle 3: 0 new findings. Stopped early — PR clean under this review's criteria.

…one definition

Callers spelled the transport as a bare "http"/"sse" literal at every site
(opencode, claude_sdk, ACP's capability selection, the tests). They now
reference BandMCPTransport.HTTP/.SSE, and the endpoint and ACP config
dispatch match on the enum.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017VPm1kcnGmxdLivQ6BGJj5
@AlexanderZ-Band
AlexanderZ-Band requested a review from a team October 2, 2026 11:35
…ver-supp-INT-1607

Takes main's ApprovalRoom, which moved to smoke/samples/approvalroom.py.
The stall-report diagnostic this branch added to the old copy is dropped;
#726 adds the equivalent close diagnostic in the new location.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013rPu5kStSAEAxctEv6Z9uN

@amit-gazal-band amit-gazal-band left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for two must items. Everything else is marked not a must (follow-ups or nits) and doesn't block.

Must

  1. claude_sdk never checks that its local Band MCP server is alive and never restarts it (ACP does), so a dead server breaks every room until the process restarts.
  2. The engine reads the room from request_ctx, which is internal to the mcp package. Please use FastMCP's public Context.

Not a must: local unauthenticated exposure for claude_sdk (INT-1636), chat_id dropped when an external room-routed server is also configured, proxy env vars breaking the loopback connection, two room-binding flags that must agree, a match with no fallback, cleanup if startup fails, and URL quoting of room ids.

What I ran (at 7b29118; the later merge of main doesn't touch the files these comments are on), set up like CI with uv sync --all-packages --locked --extra dev:

  • ruff check / ruff format --check: clean. pyrefly check: 0 errors. Markdown doc snippets: 23 passed.
  • Unit suite (without integration and e2e): 6292 passed, 151 skipped, 1 failed. The failure is test_strands_adapter::test_turn_agent_runs_the_configured_model_id, from a local botocore [crt] credential dependency; it fails on main too, so it's unrelated.
  • E2E not run (needs live credentials).

🤖 Generated with Claude Code

Comment thread src/band/adapters/claude_sdk.py
Comment thread src/band/integrations/mcp/engine.py Outdated
Comment thread src/band/adapters/claude_sdk.py Outdated
Comment thread src/band/integrations/acp/client_adapter.py
Comment thread src/band/adapters/claude_sdk.py Outdated
Comment thread src/band/integrations/mcp/local_server.py
Comment thread src/band/integrations/mcp/backends.py
Comment thread src/band/adapters/claude_sdk.py Outdated
Comment thread src/band/integrations/mcp/local_server.py
Stop importing mcp's low-level request_ctx ContextVar; inject ctx: Context
into room-bound dispatch so upgrades can't silently break room binding.

Co-authored-by: Cursor <cursoragent@cursor.com>

@AlexanderZ-Band AlexanderZ-Band left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed the two must items from the request-changes review:

  1. Dead Band MCP restart — deferred to stacked/approved #727 (INT-1642), which owns crash replacement for every adapter that hosts a local Band MCP server.
  2. request_ctx internals — fixed in 14ab516: room-bound tools now take FastMCP ctx: Context and read the room from ctx.request_context.request.

Non-must threads answered in-line (deferrals / pushbacks with reasoning). Ready for another look.

#727)

* fix: Restart a Band MCP server in place on a new port

BandMCPBackend.restart() stops and starts the same LocalMCPServer; the
server tries the port it last served on only after every other one in its
range, so consumers holding the old URL can tell a restart happened.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013rPu5kStSAEAxctEv6Z9uN

* fix: Restart claude_sdk's crashed Band MCP server and recycle stale sessions

Each message checks the backend under a lock and restarts it in place when
its serve task died; cleanup_all detaches it under the same lock so nothing
restarts after shutdown. ClaudeSessionManager replaces a cached client whose
MCP servers no longer match the room's, resuming its session on the new
URL, and fails requests queued behind stop() instead of leaving them hung.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013rPu5kStSAEAxctEv6Z9uN

* fix: Heal ACP rooms whose session dials a crashed Band MCP server

The crash branch restarts the backend in place, and every message with
injected Band tools checks it. A room whose session was built against a URL
the backend no longer serves gets a fresh session on the live one, with the
transcript replayed, instead of failing until a turn tears it down.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013rPu5kStSAEAxctEv6Z9uN

* fix: Re-register OpenCode's Band MCP server after it crashes

Every turn now checks the backend before the already-registered
short-circuit; a dead one restarts in place on a new port and is
re-registered under the same name, which OpenCode treats as a replacement.
A failed restart propagates instead of letting the turn run without tools.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013rPu5kStSAEAxctEv6Z9uN

* refactor: Heal every Band MCP backend through one restart_if_crashed()

The liveness check, warning and in-place restart lived in each adapter;
BandMCPBackend now owns them and reports whether it restarted, which
OpenCode uses to drop its stale registration. The OpenCode restart test
waits for the first turn to finish before crashing the server.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013rPu5kStSAEAxctEv6Z9uN

* fix: Repoint Letta's registration at a restarted Band MCP server

A self-hosted backend whose serve task died no longer counts as ready.
The next message restarts it and updates the registration's URL in place:
Letta reads that URL on every tool call, so attached tools reconnect with
no new ids. A row still pointing at this process's own last URL is adopted
rather than rejected, so a fixed server_name survives a crash after
release. letta-client's floor rises to 1.0.0, the first release with the
mcp_servers API the bridge already calls.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013rPu5kStSAEAxctEv6Z9uN

* fix: Clean up a cancelled LocalMCPServer start

start() only stopped what it had begun on Exception, so a cancellation
mid-start left the serve task running and its port bound with no caller
holding the server to stop it.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013rPu5kStSAEAxctEv6Z9uN

* refactor: Own every adapter's Band MCP backend through one SharedBandMCPBackend

claude_sdk, ACP, OpenCode and Letta each hand-rolled starting, healing,
locking and shutting down their Band MCP server, so a fifth adapter could
copy one and miss the crash check. SharedBandMCPBackend now owns all of it:
ensure() starts or heals the backend and refuses after a final close,
close()/detach() stop it outside the lock, and reopen() re-arms it on agent
restart. Adapters declare BandMCPBackendSettings and keep only their own
reconnect step, keyed on the backend's URL changing.

A guard test fails any module outside band.integrations.mcp that creates,
constructs or heals a Band MCP server itself.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013rPu5kStSAEAxctEv6Z9uN

* refactor: Replace a dead Band MCP backend instead of restarting it in place

A restarted-in-place backend kept its identity while its port changed, so
correctness leaned on ordering rules (read the endpoint before any await)
that held only because nothing could run while the port was unset. The
owner now stops a dead backend and starts a new one off its port, so a
BandMCPBackend's URLs are fixed for its whole life and a stopped server
still reports the URLs it served. restart_if_crashed() and LocalMCPServer's
remembered previous port give way to an explicit avoid_port.

ACP records each room's Band MCP URL on the RoomSession it belongs to,
written once the session exists and retired with it, replacing a parallel
dict kept in step at three sites.

Also: OpenCode compares registrations by value, Letta builds its
registration config and URL once, and tests share one fake backend plus
hold/crash/served-tools helpers in tests/mcpclient.py.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013rPu5kStSAEAxctEv6Z9uN

* refactor: Decide reuse where it happens for ACP sessions and Letta's registration

ACP decides whether a room's session is still current inside
_get_or_create_session, from one Band URL it also builds the session with,
instead of a separate stale check on_message had to run first. Letta's
ready compares the registered URL with the live server's, so a repoint
interrupted mid-update is retried rather than trusted; its repoint and
forget-registration steps each live in one place.

Also: stale docstrings and the OpenCode shutdown comment now match the
replace-don't-restart design, the restart test files and OpenCode's
backend fixture are renamed for it, and two tests that never dial their
server run on a fake.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013rPu5kStSAEAxctEv6Z9uN

* fix: Stop restarting a stopped session manager and leaking retired Cursor sessions

A ClaudeSessionManager stop is now final: the adapter builds a new manager
to start again, so a session requested after stop() raises
ClaudeSessionManagerStoppedError, which on_message passes through instead
of retrying as a failed resume that restarted the stopped loop.

ACP releases a session through one _release_session hook, from room
cleanup and from replacing a session whose Band MCP URL went stale, so
CursorACPAdapter forgets the retired session's todo state either way.

Also from review: ACP builds a session's MCP servers once per reuse
decision; backend fakes move to tests/mcpbackends.py; one shared
SHIPPED_SOURCE_ROOTS for the AST guards; a replacement logs the dead port;
tests assert observable outcomes, spec Letta's API from letta-client, make
the concurrent-start test actually overlap, and drive the post-shutdown
ACP turn through the harness.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013rPu5kStSAEAxctEv6Z9uN

* fix: Share one session-manager shutdown and release ACP sessions after their runtime stops

- ClaudeSessionManager.stop() runs one shielded shutdown that every caller
  awaits, so overlapping or cancelled stops no longer fail or abandon it;
  lifecycle state is the loop task and the shutdown task, not two flags.
- ClaudeSDKAdapter's resume fallback lets a stopped manager pass through
  quietly instead of posting a failure mid-shutdown.
- ACPClientAdapter.on_cleanup releases the session after runtime.stop(), so
  a detached turn's late Cursor todo updates can't outlive the cleanup.
- The backend-ownership guard moves to its own module and also catches
  module-qualified calls; tests pin the drain, the retired session's runtime
  reset, and the fixes above.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013rPu5kStSAEAxctEv6Z9uN

* fix: Forget an ACP session's bootstrap under the lock and its subclass state even on a cancelled stop

- ACPClientAdapter drops a session's bootstrap mark under _session_lock with
  no await before it again (on_cleanup and the stale-session branch), and
  calls the new _forget_session hook in a finally once the session can
  deliver nothing more: after on_cleanup's runtime.stop() and after every
  _close_session. Cursor overrides _forget_session.
- ClaudeSessionManager's shutdown queues a future-less stop and awaits the
  loop task; the stop future and _queue_stop are gone.
- SharedBandMCPBackend refuses with BandMCPBackendStoppedError.
- The ownership guard also catches an aliased LocalMCPServer import.
- Tests: a cleanup cancelled mid-stop, a manager without an MCP factory
  reusing its session; the fake ACP agent can hold its exit and has one
  Cursor-todos sender.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013rPu5kStSAEAxctEv6Z9uN

* fix: Give an ACP room whose Band URL went stale a fresh session, never a reload of the retired one

A bootstrap turn names the room's persisted session; when that session's
Band MCP URL is stale, the initializer restored the very id being retired
and the background close then killed it. The stale branch now drops the
restore, so the room gets the fresh session the retire intends.

Also pins the session manager's no-loop guards: cleanup, invalidate and
cleanup_all return at once on a never-started or stopped manager.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013rPu5kStSAEAxctEv6Z9uN

* fix: Settle teardown commands queued behind a session-manager stop, keep Letta's registration on a transient repoint failure, and drop a retired ACP session's finished setup

- ClaudeSessionManager: a failing cleanup no longer keeps the loop alive past
  stop, and teardown commands queued behind stop resolve instead of raising;
  only a create fails.
- Letta MCP bridge: a repoint failure forgets the registration only on a 404,
  so a transient error retries the same row instead of registering a second one.
- ACP client adapter: a finished setup that built the session just retired is
  never handed to the next turn.

Co-authored-by: Cursor <cursoragent@cursor.com>

---------

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
AlexanderZ-Band and others added 2 commits October 4, 2026 21:55
Keep #714's expected_runtime cleanup guard and this branch's
RoomSession + _forget_session path.

Co-authored-by: Cursor <cursoragent@cursor.com>
Fixture teardown runs on the real clock after looptime tests, so a
uvicorn tick scheduled at virtual T blocked LocalMCPServer.stop() for
~T wall seconds on fresh CI runners.

Co-authored-by: Cursor <cursoragent@cursor.com>
AlexanderZ-Band and others added 3 commits October 5, 2026 00:23
Keep AbstractEventLoop typing happy without casting the running loop.

Co-authored-by: Cursor <cursoragent@cursor.com>
@AlexanderZ-Band
AlexanderZ-Band merged commit d99f642 into main Oct 5, 2026
27 checks passed
@AlexanderZ-Band
AlexanderZ-Band deleted the fix/fix-bind-the-room-server-side-so-models-never-supp-INT-1607 branch October 5, 2026 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants