feat: consolidate band-mcp into one SDK-owned MCP engine (INT-1096) - #552
Open
AlexanderZ-Band wants to merge 66 commits into
Open
feat: consolidate band-mcp into one SDK-owned MCP engine (INT-1096)#552AlexanderZ-Band wants to merge 66 commits into
AlexanderZ-Band wants to merge 66 commits into
Conversation
AlexanderZ-Band
marked this pull request as ready for review
August 18, 2026 08:18
AlexanderZ-Band
force-pushed
the
feat/keep-the-mcp-server-in-the-sdk-repo-not-a-separate-INT-1096
branch
from
August 18, 2026 12:16
4841a13 to
4bff619
Compare
Gates the INT-1096 migration plan's design: proves end-to-end that a bare FastMCP instance's sse_app() and streamable_http_app() routes can be mounted onto a custom-managed Starlette app (the shape LocalMCPServer already uses), with the host lifespan entering session_manager.run() itself since a mounted sub-app's own lifespan is never invoked by the ASGI server. Also proves the start->stop->start rebuild requirement (session managers are single-use) and that FastMCP's auto-enabled DNS-rebinding protection on loopback binds accepts real MCP clients while still rejecting a spoofed Host header. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
Adds [tool.uv.workspace] with packages/* members, and a placeholder packages/band-mcp package (pyproject.toml, minimal __init__.py/server.py, README.md stub). The real source lands in step 3's mechanical copy. band-mcp's mcp[cli] floor is pinned to >=1.28.1,<2, not the latest 1.x: it's an unconditional workspace member (not an extra), so it can't be forked away from the dev-crewai extra the way [tool.uv] conflicts already forks crewai from parlant/pydantic-ai -- and crewai 1.15.x unconditionally pins mcp~=1.28.1. A tighter floor here made the shared uv.lock unsolvable whenever crewai is in the graph. A standalone `pip install band-mcp` is unaffected, since nothing else constrains it there. Verified: uv lock resolves; uv sync --all-packages with --extra dev, --extra dev-crewai, and --extra dev-parlant all succeed (dev-crewai's runtime crewai import crash under Python 3.14 is pre-existing on main, unrelated to this change); full unit suite green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
…1096) Copies src/band_mcp/* (config.py, server.py, shared.py, tools/registrar.py), README.md, and mcp_config_example.json from the standalone band-mcp repo unchanged, replacing step 2's placeholders. Only dependency metadata (step 2) and the new package's pins differ from the old repo -- this is code+docs. One type-annotation-only fix beyond the copy: config.py's resolve_config() called Mapping.get() twice per credential slot (once in an isinstance check, once in the guarded value), relying on a mypy-specific `# type: ignore[arg-type]` that this repo's pyrefly doesn't recognize (different error code). Rewrote each as a single `.get()` into a local variable so the isinstance narrowing is real and the ignore comment is no longer needed -- no behavior change (verified: resolve_config()/validate() produce identical results before and after). Verified end-to-end against the real band-sdk in the workspace: register_tools() against a live band_mcp.shared.mcp FastMCP instance advertises the same 7 agent tools as published band-mcp 1.3.2. Full unit suite green; ruff/pyrefly clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
… (INT-1096)
Steps 4-5 of the migration plan (test migration + get-it-green), landed
together: the fixes needed to run under this repo's environment are baked
into the same files as the copy, so a clean "raw copy, zero changes" commit
wasn't possible without redoing the work.
Moved, unchanged:
- tests/unit/{test_shared,test_registrar,test_server,test_config}.py,
tests/test_transport_security.py, tests/conftest.py -> tests/mcp/
(124 tests, all pass unchanged against the packages/band-mcp source
copied in the previous commit).
- tests/integration/test_forwarding.py -> tests/integration/mcp/
(in-process, fully mocked -- no live API).
Moved with a real naming collision resolved: band-mcp's top-level
tests/conftest_integration.py would have collided with this repo's own
unrelated tests/conftest_integration.py (SDK-wide live fixtures, different
shape). Inlined its content directly into tests/integration/mcp/conftest.py
instead, dropping the old repo's separate re-export shim; fixed
test_full_workflow.py's now-dangling import.
Mechanical fixes to actually run here (Python 3.14 / this repo's
pytest-asyncio config, not behavior changes):
- agent_room fixture used the deprecated `asyncio.get_event_loop()` pattern,
which raises under Python 3.14 outside a running loop -- converted to a
plain async fixture.
- That conversion needs `@pytest.mark.asyncio(loop_scope="session")` on the
three tests consuming `agent_room`, matching this repo's own
asyncio_default_fixture_loop_scope="session" convention (already used
elsewhere under tests/integration/) -- otherwise the AppContext's
asyncio.Lock, first touched inside the fixture's loop, raises "bound to a
different event loop" when the test body reuses it.
- _extract_id() only handled dict-shaped tool results; band_create_chatroom
returns a bare room-id string (AgentTools.create_chatroom() -> str), so
agent_room was silently skip()ing on every real run. Fixed to handle both.
Two real pre-existing bugs found by actually running this live suite
(reproduce identically against the unmodified old registrar.py + unmodified
old test -- not introduced by this migration, and this suite is excluded
from CI, gated behind `requires_api`/BAND_API_KEY):
- test_agent_lookup_peers_returns_list assumed band_lookup_peers is
room-less; it isn't (AgentTools is constructor-scoped per room, and
lookup_peers() filters to peers not already in that room) -- fixed by
passing chat_id via the now-working agent_room fixture.
- test_agent_create_room_send_and_read_back and
test_agent_send_message_accepts_room_id_alias call band_send_message on a
freshly created room with no other participant to mention, but
band_send_message requires a non-empty `mentions` list. Not a mechanical
fix -- marked xfail with a reason citing the real cause; left for the
engine work to resolve deliberately rather than guessing a design choice.
Verified: tests/mcp/ 124 passed; tests/integration/mcp/ 15 passed, 4 skipped,
2 xfailed, 0 failed (run live against platform.dev.band.ai, a dev
environment, using the .env.test legacy BAND_API_KEY the SDK's own
pre-existing integration tests already auto-load); full unit suite 4702
passed (was 4578); ruff/ruff format/pyrefly clean.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
Step 6: dependency refresh, within the mcp<2 cap. - band-client-rest 0.0.26 -> 0.0.27. Diffed the two wheels: the only change is the User-Agent/X-Fern-SDK-Version header literal -- no workaround needed, no adaptation required. - desktop/opencode/letta/acp extras' mcp floor: >=1.25.0,<2 -> >=1.28.1,<2 (also applied to packages/band-mcp, landed in an earlier commit). Not >=1.29.0 as the plan's own text suggested: bumping band-sdk's own published extras to >=1.29.0 makes `pip install band-sdk[crewai,acp]` (or any crewai + desktop/opencode/letta/acp combo) genuinely unsolvable for real downstream installs, not just this repo's dev lock -- crewai 1.15.x (latest as of 2026-08-18) unconditionally pins mcp~=1.28.1. >=1.28.1 is the newest floor that keeps every extra combination installable; it's still within the load-bearing <2 cap (mcp 2.0.0 dropped the lowlevel Server decorator registration these extras build on). Used `uv lock --upgrade-package mcp --upgrade-package band-client-rest` rather than a blanket `uv lock --upgrade`: the latter also bumped dev-only tooling (ruff 0.15->0.16, pyrefly 0.61->1.2) and surfaced ~1348 pre-existing lint findings across the whole repo, unrelated to this PR's scope. band-testing-python==0.1.4 already latest -- no change needed. Verified: uv lock resolves (two mcp forks: 1.28.1 for the crewai fork, 1.29.0 elsewhere); dev/dev-crewai/dev-parlant syncs all succeed; full unit suite green (4702 passed); ruff/ruff format/pyrefly clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
Step 7: the fixtures-first toolkit the plan wants engine.py written against
(step 8), built as far as possible before engine.py/local_server.py exist.
- FakeHumanTools (tests/mcp/conftest.py): behavioral fake for the human
surface, in FakeAgentTools' style -- observable state, not a MagicMock.
Covers every HumanTools method. Test-local for now; promote to
band.testing only if a second consumer appears. Proven against the real
registrar + a real FastMCP dispatch (tests/mcp/test_fake_human_tools.py),
not just type-checked.
- advertised_schemas(session): projects a real list_tools() round trip into
a snapshot-comparable dict.
- Wire-schema snapshot test (tests/mcp/test_wire_schema_snapshot.py):
real in-memory-transport protocol round trip against band-mcp's current
registrar, diffed against checked-in JSON (tests/fixtures/wire_schemas/
{full,pinned}.json, generated now). Locks in the published 1.3.2 contract
before the engine consolidation (steps 8-9) touches anything -- any
accidental wire change during that work fails loudly here.
Deliberately deferred, not dropped: the engine_session / local_server_session
fixtures the plan also lists under step 7 need EngineSpec/build_engine
(step 8) and local_server.py (step 9) to exist -- writing them now would
just be dead code against nothing. They land with the step that makes them
real. Same for the CLI subprocess contract tests (--help/--version/stdio
purity/etc.): only meaningful once the CLI is testing the actual
consolidated engine, so they land with steps 11-12.
Verified: 132 tests pass in tests/mcp/ (was 124); full unit suite 4710
passed (was 4702); ruff/ruff format/pyrefly clean.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
Step 8: src/band/integrations/mcp/engine.py -- collapses band-mcp's
FastMCP-based registrar and LocalMCPServer's hand-rolled lowlevel-Server
registration into one engine, consumed by two front-door factories
(packages/band-mcp's standalone_spec, step 11; local_server.py's
embedded_spec, step 9). Neither front door exists yet -- this is the shared
core they'll both call.
Design realized concretely from the plan's decisions:
- EngineSpec/MCPToolRegistration/CustomToolSpec/ToolsResolver are themselves
framework-neutral (no mcp-package type in their own fields), even though
engine.py itself is an allowlisted mcp-import module -- so INT-1150's v2
migration only touches this module's FastMCP-translation internals.
- ToolsResolver has exactly one method (invoke()); EmbeddedResolver is the
SDK-owned implementation (calls adapter-owned tools via a room-lookup
callback, no cache/lock -- row 11). StandaloneResolver (CLI-side locking/
caching/participant-refresh) lands with the CLI factory in step 11.
- Room-field injection unifies onto one mechanism for both doors:
extend_with_chat_id()/pin_existing_chat_id() build a real extended
Pydantic model (chat_id + AliasChoices("chat_id","room_id"), hidden via
SkipJsonSchema when pinned) -- replacing LocalMCPServer's old hand-rolled
schema-dict surgery. FastMCP has no API to hand a tool an explicit schema
override; it only derives one from a real function's signature, which is
why this goes through a model, not a dict.
- Dropped the ctx/AppContext/lifespan threading entirely: resolvers are
self-contained objects a factory captures directly in each registration's
execute closure, so build_engine()'s dynamic handlers need no Context
parameter at all -- a real simplification (declarative-first, per the
plan's decisions log), not just a port.
- SendEventWideInput (row 6) is a deliberate independent model, not a
SendEventInput subclass: pyrefly correctly flagged that widening a
mutable Pydantic field's type via subclassing is unsound (Liskov). Its
Literal is WideEventMessageType, added to band.core.types next to the
existing EventMessageType -- same single-source-of-truth pattern already
used there, just widened.
- _serialize() (row 15) is the CLI's str-passthrough shape, now universal
for both doors -- flagged in this commit as the one intentional,
LLM-visible behavior change for embedded consumers (was a {"result": x}
dict-wrap), to be verified by the e2e backends lane per the plan.
- validate_unique_tool_names() is the one engine-level duplicate check
(row 8), covering every surface and custom tools together.
- AGENT_ROOM_BOUND_TOOL_NAMES + classify_room_binding() land in
runtime/tools.py next to ROOM_POSTING_TOOL_NAMES/is_room_posting_tool
(tool-schema concerns, framework-neutral) -- the CLI factory's
classifier; the embedded factory doesn't call it, since its uniform wrap
applies to every agent tool regardless (row 2). Also fixed the stale
tools.py comment referencing band-mcp as an external repo path.
- Added the MCP-import-boundary test now (INT-1150's acceptance criterion,
made executable early): scans src/band and packages/band-mcp/src for
module-level mcp-package imports outside an explicit allowlist.
Verified end-to-end via real MCP protocol round trips (SDK in-memory
transport, mcp.shared.memory.create_connected_server_and_client_session) --
not just unit-tested in isolation: embedded uniform-wrap dispatch, pin
overriding a client-sent chat_id, send_message error enrichment with
available handles, room_id alias routing, human room-bound pinned/unpinned
dispatch, custom tools (both CustomToolSpec and the bare tuple contract).
Full unit suite 4727 passed (was 4710); ruff/ruff format/pyrefly clean.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
Step 9: src/band/integrations/mcp/local_server.py replaces runtime/mcp_server.py -- keeps LocalMCPServer's hard-won lifecycle shell (port scanning, EmbeddedUvicornServer's signal-capture-disabling, bounded graceful shutdown) verbatim, replaces its internals (raw lowlevel Server with hand-rolled list_tools/call_tool) with mounting engine.py's FastMCP app via the step-1 spike's proven recipe. Two lifecycle bugs fixed, not ported: - stop() used to skip socket close and state reset when the serve task crashed with anything but CancelledError (the bare `await self._serve_task` re-raised past the cleanup code below it). Fixed with cleanup in `finally`. - start()/stop() had no concurrency guard. Fixed with one asyncio.Lock serializing every lifecycle transition (start() keeps its existing idempotent no-op-if-already-running check, now race-free). - Gained __aenter__/__aexit__; start()/stop() remain as the escape hatch for non-lexical lifetimes (acp/client_adapter.py holds its server across method scopes) -- the context manager's own halves, not a second path. build_band_mcp_tool_registrations()/build_resolved_band_mcp_tool_registrations() now build on engine.py's shared build_tool_registration() + EmbeddedResolver instead of hand-rolling -- same public signatures (band-sdk is published API), different internals. Both now advertise "chat_id" (not "room_id") on every registration, per the uniform wrap the embedded door has always used (divergence-matrix row 2) -- the actual field-name rename is intentional and happens here, not step 10 (which ripples the *prompt text* teaching models the old name, a separate concern from the schema itself). Rewired every caller per the plan's blast-radius table: backends.py, letta/config.py, letta/mcp.py, acp/client_adapter.py (the one direct LocalMCPServer import, not just the stable create_band_mcp_backend API). runtime/mcp_server.py becomes a pure re-export shim (band-sdk is published, so the module move alone would break an external `band.runtime.mcp_server` import) -- dropped from the import-boundary allowlist since it no longer imports the mcp package itself. Migrated tests/runtime/test_mcp_server.py -> tests/integrations/mcp/test_local_server.py (import paths, logger-name string, MCPToolRegistration.to_mcp_tool() -> input_model.model_json_schema() since that method no longer exists -- the engine derives schemas from a real function signature now, not a hand-built Tool). Fixed two tests whose hand-built MCPToolRegistration returned a raw dict: the dynamic handler build_engine() creates always declares -> str (row 15, universal now), so FastMCP's structured-output validation now rejects a non-string return that the old lowlevel-Server path never checked. Same fix applied to test_e2e_codex_acp.py's equivalent (gated, not run here). Added tests for both fixed lifecycle bugs directly, plus a real start/stop/start cycle against a live server. Verified: 1166 passed across tests/mcp, tests/integrations/mcp, tests/runtime, tests/integrations/test_mcp_backends.py, tests/adapters/test_letta_mcp.py, tests/integrations/acp (8 skipped -- codex-acp E2E, opt-in). Full unit suite 4730 passed (was 4727); ruff/ruff format/pyrefly clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
…-1096)
Step 10: the embedded door's advertised schema field renamed room_id ->
chat_id in step 9 (build_resolved_band_mcp_tool_registrations's uniform
wrap) -- this updates what every embedded consumer *tells the model* to
match, so the model doesn't keep reaching for a field name the schema no
longer has. All 7 sites the plan enumerated, plus their test assertions:
1. opencode/adapter.py: per-turn "Current room_id" system block ->
"Current chat_id".
2. integrations/letta/prompts.py: "Every tool call REQUIRES a `room_id`
argument" -> `chat_id`.
3. adapters/letta.py: self-host/shared-mode rejoin prompt -> `chat_id`.
4. acp/client_adapter.py: room context "Current room_id" -> "Current
chat_id".
5. integrations/claude_sdk/prompts.py: the full tool-call JSON example
block (every "room_id" key and the [room_id: ...] message-format
example) -> chat_id, throughout.
6. adapters/claude_sdk.py: the per-message room_context marker itself,
`f"[room_id: {room_id}]"` -> `f"[chat_id: {room_id}]"` (the Python
variable name stays room_id -- only the model-facing marker text and
its schema field name change).
7. runtime/custom_tools.py: custom_tool_to_mcp_schema's include_room_id
param and injected key -> include_chat_id / "chat_id". Confirmed this
function has zero callers anywhere in the repo -- renaming outright
rather than adding a compat alias for a function nothing calls.
One additional site the plan's table pointed at but didn't fully resolve:
integrations/claude_sdk/tools.py's _build_sdk_schema hand-spliced a
"room_id" key into a schema dict (the "sdk" in-process backend kind, C in
the plan's usage-impact table). Switched it to call the engine's
extend_with_chat_id() directly -- same canonical field-injection helper
every other embedded consumer now uses, instead of a fourth hand-rolled
copy of "splice a room field into a schema". Its own dispatch (room_id/
chat_id key extraction in the two tool handlers) updated to match.
Checked and left alone as genuinely out of scope (verified, not assumed):
copilot_sdk.py's own `[room_id: ...]` marker (doesn't use LocalMCPServer or
backends.py at all -- an unrelated mechanism); crewai's tools.py and
runtime/oneshot.py's "room_id" mentions (CrewAI's own separate tool-wrapping
layer, and the bridge's HTTP forwarding envelope, respectively -- neither
touches band-mcp/engine.py).
Deferred to step 13 as the plan specifies: the
examples/acp/copilot_docker/compose/README.md paragraph explaining the old
chat_id-vs-room_id split (now false) gets rewritten with the rest of the
docs pass.
Full unit suite 4730 passed (text/assertion changes only, no behavior
change in test count); ruff/ruff format/pyrefly clean.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
…096) Step 11: packages/band-mcp/tools/registrar.py deleted entirely (fully absorbed into engine.py, per the plan). server.py/shared.py/config.py rewritten to build an EngineSpec (standalone_spec) and hand it to the shared engine, instead of hand-registering tools via FastMCP's own decorator API. Legacy thnv_* key prefixes dropped (user decision, INT-1096): config.py's _legacy_key_capabilities() now only recognizes band_u_/band_a_/band_ -- a surviving thnv_* key serves neither scope, same as any other unrecognized key. Locked in with a new test; every other test's thnv_* fixture values swapped for band_* equivalents (same intent, still-recognized prefix). Design change beyond a mechanical port: dropped the AppContext/FastMCP- Context/lifespan threading entirely. The old registrar needed it because free functions (get_agent_tools, get_human_tools, ...) had no other way to reach per-room state from inside a dynamically-built handler. The new StandaloneResolver (shared.py) is the concrete ToolsResolver for this door: a self-contained object built once, synchronously, before the engine exists (REST client construction does no I/O, so there's nothing to defer to a lifespan). standalone_spec(config, resolver) then builds the CLI's EngineSpec: per-tool room classification (classify_room_binding, unlike the embedded door's uniform wrap), band_send_event widened to SendEventWideInput, extend_with_chat_id/pin_existing_chat_id per tool. health_check is a module-level function taking the resolver explicitly (testable in isolation), wrapped by a thin zero-arg closure at registration time. Every divergence-matrix row this door owns, carried forward in StandaloneResolver: per-room AgentTools LRU cache (128) + 64 lock stripes (row 11), room-less None-key/""-sentinel (row 24), send_message pre-flight participant refresh + discard-on-failure (row 9), the CLI's str-shaped result serialization (row 15, via the engine's now-universal _serialize()). Caught by a real subprocess smoke test, not by any unit test: health_check is registered by run() itself, outside standalone_spec, so the wire-schema snapshot never covered it -- a wrapper function named _health_check_tool leaked into the advertised schema's auto-derived "title" field (FastMCP derives it from the function's own __name__, independent of the tool() name= override). Fixed by naming the wrapper health_check directly, and added tests/mcp/test_cli_contract.py (real `python -m band_mcp.server` subprocess: --version, --help, missing-credential exit 2, stdio stdout purity, and this exact regression) to close the gap going forward. A fuller subprocess contract battery (per-config schema/validation-text parity) remains step 12's job, per the plan. Test suite changes: - Deleted tests/mcp/test_registrar.py (tested the now-deleted module). - Rewrote tests/mcp/test_shared.py against StandaloneResolver (same invariants: LRU eviction, lock stripes, room-less sentinel, refresh/ discard, human dispatch). Dropped SDK-import-failure tests outright (row 21: AgentTools/HumanTools import unconditionally now -- band-sdk is this same package, no failure mode left to test). - Added tests/mcp/test_standalone_spec.py: the CLI-door integration coverage (scope/tools filtering, duplicate-name ConfigError, per-tool room classification, pinning) that test_registrar.py used to carry, ported to the new factory -- a real gap the raw pass-count would have hidden otherwise. - Fixed tests/mcp/test_fake_human_tools.py and test_wire_schema_snapshot.py (both built FastMCP via the now-deleted register_tools) to build via standalone_spec + build_engine instead -- the wire-schema-snapshot fix is the real proof this step preserves the published contract: it caught one genuine bug (band_send_event's widened model carried a rewritten docstring mentioning tool_call/tool_result, not the original SendEventInput docstring the old registrar's model.__doc__ = original.__doc__ preserved) before this commit, fixed by reusing SendEventInput.__doc__ verbatim. - Fixed tests/mcp/test_transport_security.py's two integration tests (built the engine fresh via standalone_spec + build_engine instead of importing a now-nonexistent module-level FastMCP singleton). Verified: real `band-mcp --version`/`--help` subprocess output unchanged; real stdio initialize+tools/list round trip (7 agent tools + health_check, stdout pure JSON-RPC, chat_id schema correct); wire-schema snapshot passes (byte-for-byte published contract preserved for the "full" and "pinned" CLI profiles). Full unit suite 4723 passed; ruff/ruff format/pyrefly clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
Adds --all-packages to every uv invocation in ci.yml so the workspace member is installed and checked (lint, test, test-crewai, test-parlant), replaces the packaging job's now-broken LocalMCPServer._build_server() smoke with a real start/stop cycle against the new engine, and adds dedicated band-mcp wheel-build + import + CLI verification steps. Adds band-mcp-publish.yml mirroring band-publish.yml's trusted-publishing layout, with a bounded wait for band-mcp's declared band-sdk floor to be indexed on PyPI before the install-check (two-phase release: band-sdk ships the engine first, then band-mcp's floor is bumped to match). Gives band-publish.yml's build job a matching tag-prefix guard so a band-mcp release doesn't also run (and fail) the band-sdk publish job. Registers packages/band-mcp as its own release-please component (package-relative changelog/extra-files, exclude-paths on the root component so band-mcp-only commits don't bump band-sdk), and adds the tools/auth/transport PR-title scopes band-mcp's old repo used. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
Adds an AGENTS.md/CLAUDE.md section covering the one-engine/two-front-door design (packages/band-mcp CLI + the embedded LocalMCPServer), the EmbeddedResolver/StandaloneResolver split, the MCP-import allowlist, and the wire-schema snapshot tests that pin the published CLI's contract. Fixes two docs that still described chat_id and room_id as different model-facing argument names across the two doors -- both now advertise chat_id, per INT-1096's room-field unification. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
… MCP engine Closes several magic-string/parallel-list duplications the migration introduced or inherited: - `CHAT_ID_FIELD_NAME` (band.runtime.tools) is the one definition of the model-facing room-identifier argument name; engine.py's schema builders and every adapter prompt that tells the model to use it (opencode, letta, acp, claude_sdk) now reference the constant instead of re-typing "chat_id". - `Surface` (StrEnum, band.runtime.tools) replaces the bare `Literal["agent", "human"]` ToolDefinition.surface carried, so classify_room_binding/iter_tool_definitions dispatch on named members via match/case instead of re-typed string literals. - `Scope`/`ToolGroup`/`Transport` (StrEnum, band_mcp.config) replace band-mcp's own Literal types plus their hand-maintained parallel VALID_SCOPES/VALID_TOOLS lists -- one definition each, derived lists. - `CliArgs` (TypedDict) replaces `Mapping[str, object]` for resolve_config's cli parameter, removing the isinstance-renarrowing dance and every `# type: ignore[arg-type]` that boundary needed -- the two `cast()` calls in Config's field defaults are gone too, now that DEFAULT_SCOPE/DEFAULT_TOOLS are already concretely typed. Behavior is unchanged; the wire contract (band-mcp's advertised schemas, CLI flags, exit codes) is untouched -- confirmed by the full unit suite, the wire-schema snapshot tests, and the CLI subprocess contract tests all staying green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
tools/auth/transport were band-mcp's own scopes when it was a standalone repo, where those words were unambiguous. In this monorepo they collide with existing broader concerns: transport reads as websocket's territory, tools spans every adapter's runtime tool surface, and auth is generic across the whole SDK. One mcp scope is precise and collision-free. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
…ippets python3 -c "<newline>code<newline>" only works because YAML's `run: |` block strips the block's common indentation before bash sees it -- correct today, but silently depends on every line staying at the same indent level, and the double-quoted string form performs $var/backtick/backslash expansion, so a future snippet using any of those characters would break or misbehave silently. A quoted heredoc (`<<'PYEOF' ... PYEOF`) disables shell expansion entirely regardless of content, and is the idiomatic way to embed a multi-line script in bash. Behavior is unchanged -- verified by extracting every affected step's real (YAML-dedented) script and bash -n-checking it, and by running the two package-version-extraction snippets directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
Found live via the Letta lane smoke: a containerized Letta calling back into a LocalMCPServer bound to 0.0.0.0 (the class's own documented Docker-callback support) got a 421 Misdirected Request on every call. build_engine() never told FastMCP the caller's real bind host, so FastMCP's own constructor always saw its hardcoded default host="127.0.0.1" and took its "auto-enable loopback-only DNS-rebinding protection" branch regardless of what the caller actually bound to -- locking allowed_hosts to 127.0.0.1/localhost/::1 even when LocalMCPServer had explicitly bound 0.0.0.0 for a remote/containerized client. A Host: host.docker.internal request then failed that allowlist unconditionally. build_engine now takes a host param and LocalMCPServer.start() passes its own self._host, so FastMCP's auto-detection sees the truth: loopback binds keep the existing strict default unchanged, non-loopback binds get protection off (matching the already-accepted risk model documented on LocalMCPServer's own class docstring -- only bind non-loopback on an isolated/trusted host). The CLI door is unaffected either way, since it always passes its own explicit transport_security. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
BREAKING CHANGE: band-mcp no longer accepts BAND_API_KEY. Set BAND_USER_KEY (human scope) and/or BAND_AGENT_KEY (agent scope) explicitly -- there is no unscoped credential or prefix-inference fallback any more. Removes _legacy_key_capabilities(), Config.legacy_key, the "legacy-key-ignored" warning kind, server.py's pure-legacy escape hatch (_is_pure_legacy_invocation and its scope write-back), and the dead Settings.band_api_key field (nothing ever read it). resolve_credential_for_ scope() is now a plain per-scope lookup with no fallback branch. Updates the README, mcp_config_example.json, and the CLI's --help env-var list to match, and drops the now-legacy-specific tests from test_config.py/ test_server.py/test_cli_contract.py. Incidentally found and fixed while touching this code: tests/integration/ mcp/conftest.py (and test_forwarding.py) still imported band_mcp.tools. registrar and band_mcp.shared.build_app_context, both deleted in step 11 of the INT-1096 migration -- the whole tests/integration/mcp/ live-API suite has been silently uncollectable since then. Rewrote conftest.py's live_config/harness fixtures on the current standalone_spec()/ build_engine()/StandaloneResolver API (LiveHarness's public interface is unchanged, so test_smoke.py/test_full_workflow.py/test_error_cases.py needed no changes), and deleted test_forwarding.py outright -- it tested the now-deleted registrar module directly and its coverage is already superseded by tests/mcp/test_standalone_spec.py + test_engine.py against the real engine. Verified live: 4 passed, 4 skipped (no human-scope credential), 2 xfailed (pre-existing, unrelated), 0 failures/errors. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
…t 2) The rest of the previous commit's intended changes -- a bad pathspec in my own `git add -A` invocation aborted that add before it staged anything but the test_forwarding.py deletion, so the previous commit's message described changes this repo didn't actually contain yet. This commit is those changes. BREAKING CHANGE: band-mcp no longer accepts BAND_API_KEY. Set BAND_USER_KEY (human scope) and/or BAND_AGENT_KEY (agent scope) explicitly -- there is no unscoped credential or prefix-inference fallback any more. Removes _legacy_key_capabilities(), Config.legacy_key, the "legacy-key-ignored" warning kind, server.py's pure-legacy escape hatch (_is_pure_legacy_invocation and its scope write-back), and the dead Settings.band_api_key field (nothing ever read it). resolve_credential_for_ scope() is now a plain per-scope lookup with no fallback branch. Updates the README, mcp_config_example.json, and the CLI's --help env-var list to match, and drops the now-legacy-specific tests from test_config.py/ test_server.py/test_cli_contract.py. Also rewrites tests/integration/mcp/conftest.py's live_config/harness fixtures onto the current standalone_spec()/build_engine()/ StandaloneResolver API -- they still imported band_mcp.tools.registrar and band_mcp.shared.build_app_context, both deleted in step 11 of the INT-1096 migration, so the whole tests/integration/mcp/ live-API suite has been silently uncollectable since then (LiveHarness's public interface is unchanged, so test_smoke.py/test_full_workflow.py/test_error_cases.py needed no further changes beyond what's already in the previous commit). Verified live against the real dev Band platform: 4 passed, 4 skipped (no human-scope credential), 2 xfailed (pre-existing, unrelated), 0 failures/errors. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
… types
Comments should state why the code is the way it is, not reference a
tracker ID or narrate what changed between versions -- strip every
INT-1096/INT-1150/INT-338/step-N mention across the MCP engine, band-mcp,
and their tests.
Also tightens test_start_forwards_real_host_to_build_engine's spy to
build_engine's real keyword-only signature instead of *args/**kwargs:
object, which both fixes a genuine pyrefly no-matching-overload error on
the forwarding call (verified: 10 -> 3 pyrefly errors on this file, the
remaining 3 pre-existing and unrelated) and resolves ty's matching
complaint. The create_model(**{...}) and __signature__ assignment ty
flagged elsewhere are confirmed-necessary escape hatches (removing their
ignore comments reproduces the same errors under pyrefly, the repo's
actual gate) -- pydantic's create_model overloads and CPython's
FunctionType stub don't model dynamic model/signature construction, so
these stay as-is.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
Introduced when these assertions replaced structuredContent dict checks: content is a five-member union (TextContent | ImageContent | AudioContent | ResourceLink | EmbeddedResource) and only TextContent has .text, so pyrefly flags every .text access as missing-attribute -- invisible to the gate today only because project_excludes drops tests/** from pyrefly's project-level check. Added _text_of(), an isinstance-narrowing helper used at all three call sites; also fails loudly (not silently) if a tool ever returns non-text content. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
Comparing the full advertised schema (title/description prose included) made every snapshot diff noisy -- a reviewer can't tell an intentional wire-contract change from a reworded description at a glance. Project each tool down to what a real call's acceptance actually depends on: required-ness, JSON type, enum values (resolving $ref/anyOf so a ref'd enum's allowed values are still covered), array item type, and string-length bounds. full.json shrinks from ~43KB to ~18KB. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
…racts A diff against a 45-tool JSON blob is not reviewable -- nothing forces a human to actually read it before regenerating and committing whatever comes out. Replace it with small, hand-written ToolContract entries for only the 8 tools that carry a real non-obvious wire invariant (enum values, array item type, a required-set beyond boilerplate); the other ~37 tools are plain string/int/bool CRUD fields already covered by the engine/converter/dispatch unit tests. Enum values are reflected off their real StrEnum/Literal source (band.core.memory_types, WideEventMessageType, the input models themselves via a small _field_literal_values helper) rather than hand-copied, so the test can't silently drift from the validator's own source of truth. chat_id room-binding (required unpinned, hidden pinned) is checked once generically against AGENT_ROOM_BOUND_TOOL_NAMES instead of being repeated per tool. Verified the new test actually fails loudly on a real break (renamed a required field) before committing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
… gaps Review pass over the MCP engine refactor: - engine.py: _build_handler_signature dropped every ge/le/max_length/ pattern constraint from the advertised wire schema; fixed via Pydantic's own FieldInfo.rebuild_annotation(). The chat_id field description no longer names the room_id alias (model-facing text stays chat_id-only). - Extracted resolve_tool_method/dispatch_tool in engine.py, shared by EmbeddedResolver and StandaloneResolver: a method-not-found registry mistake now raises one actionable message instead of a raw AttributeError at whichever call site hit it first. - shared.py: StandaloneResolver never passed agent_id into AgentTools, so a failed send_message on the CLI door hinted the agent's own handle as mentionable. Resolves and caches it via agent_api_identity.get_agent_me(). Lock stripes bumped 64->128 to match cache size (removes cross-room false contention). - config.py: unified --scope/--tools explicit-empty detection into one _is_explicit_empty() helper (--scope "" had no equivalent handling before). - server.py: _health_check probes human/agent connectivity concurrently. - Test-quality pass: replaced weak assertions (`is not None`, bare `isinstance(profile, dict)`) with pinned values verified against real response shapes; test_concurrent_start_calls_are_serialized now counts actual socket-reservation calls instead of just checking a bind succeeded; test_invoke_human_dispatches_to_singleton uses the real FakeHumanTools instead of a MagicMock; added match= to ConfigError assertions; removed remaining stray ticket-ID references. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
test_loopback_bind_auto_dns_rebinding_protection_accepts_real_clients hangs the full 30s pytest-timeout on windows-latest CI, stuck inside asyncio's Proactor _poll waiting on a completion that never arrives. This is a real, isolated bug in the spike's own throwaway _RunningApp helper, not in production code: LocalMCPServer.start() uses the identical raw-socket-to-uvicorn handoff (_reserve_socket + uvicorn.Server(sockets= [...])), and its own real-client tests (test_local_server.py's test_serves_sse_tools_on_localhost / test_serves_streamable_http_tools_ on_localhost) already exercise that exact path successfully on Windows CI today (verified against main via PR #545/#547's green windows runs) -- so the production path isn't broken, only this spike's copy of it. The spike's own docstring already says it's disposable once local_server.py lands ("this test either moves onto it or is deleted -- it is a feasibility gate, not permanent product code"). That already happened; this test's unique coverage (a real client isn't 421'd on loopback) is now fully redundant with test_local_server.py's tests against the real class, so delete it rather than debug a Windows Proactor quirk in disposable spike code. The sibling rejects_spoofed_host test stays -- it has no live-server equivalent elsewhere. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
AlexanderZ-Band
force-pushed
the
feat/keep-the-mcp-server-in-the-sdk-repo-not-a-separate-INT-1096
branch
from
August 18, 2026 13:05
121c233 to
f6bdf3b
Compare
…nfig resolvers /simplify pass (4 parallel review agents: reuse/simplification/efficiency/ altitude) over the full MCP-engine migration diff. Efficiency and altitude came back clean. Reuse found one finding already explicitly deferred by the diff's own comment (a "[System]: " prefix duplicated across 9 other adapters -- fixing it means touching every one of them, out of scope here). Applied the 5 simplification findings: - engine.py: extend_with_chat_id's pinned branch and pin_existing_chat_id built the identical create_model field spec; extracted _pinned_chat_id_field(). - local_server.py: build_band_mcp_tool_registrations duplicated build_resolved_band_mcp_tool_registrations wholesale; now delegates to it with a single-room get_tools closure. - band_mcp/config.py: resolve_config's --scope and --tools blocks repeated the same resolve-then-partition shape; extracted _resolve_and_partition(). - claude_sdk.py: _on_assistant_message and _on_user_message duplicated the ToolUseBlock/ToolResultBlock match cases; extracted _dispatch_tool_block() (single-pass semantics preserved -- each block is still visited exactly once, in original order). - client_types.py: BandACPClient.__init__ was a pure passthrough adding no behavior over its parent; removed. Also dropped a few more stray "the ticket"/"divergence-matrix row N" references picked up while touching these functions, per the no-ticket- refs convention already applied elsewhere this session. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
Dependency Conflicts table and the band-client-rest pin example had drifted from pyproject.toml (crewai/pydantic/opentelemetry/fastmcp versions). Also collapsed three near-identical gh api examples and three restatements of the same ValidationError/required-config rule down to one each. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SecysJC5LKmM3RGY9CcV2v
~16 tests each hand-rolled the identical mechanism: a local MockMessage class, a nonlocal-capturing callback, then a call to client._handle_events. Extracted dispatch() -- feeds one event through _handle_events via a single registered callback and returns what it received (None if never called). Left tests with a genuinely different shape (custom raising callbacks, multi-handler routing, no-handler-registered cases, a different method under test) untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
task_statuses/task_error_codes extracted: 9 tests hand-rolled the identical "pull one metadata_namespace field out of every task event" comprehension this repo's own CLAUDE.md names as the anti-pattern to avoid. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
- register_pending_approval() extracted: 17 tests hand-built the identical _PendingApproval registration (varying only token/tool_name/summary/ created_at), each duplicating asyncio.get_running_loop().create_future() and the _pending_approvals dict-poke. - _tool_result_payload() extracted alongside the file's existing _error_events/_narrated_message_types, for the one call site with the same "pull the sole matching event's content" shape. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
Demonstrates the published band-mcp CLI's stdio transport driven by three different consumers, each proving a distinct capability: a raw mcp client composing a room at runtime (agent scope), a vanilla Claude Agent SDK script proving durable cross-process memory (--tools memory), and a LangGraph agent using band-mcp's human scope as a personal assistant. All three live-verified against the dev platform. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018vHeRvLouZegJmdmgyue1o
Extracted started_tools fixture: ~30 tests hand-rolled the identical "construct adapter, on_started, reach into crewai_mocks.Agent.call_args[1] ['tools'], look up by name" sequence, most also duplicating asyncio.run() for sync test methods. started_tools(**adapter_kwargs) does the construct+start and returns the registered tools keyed by name, so tests read as arrange (kwargs) -> act (call) -> assert (lookup/membership). Left the Agent-constructor-kwarg checks (role/goal/verbose/max_rpm/ allow_delegation) untouched -- a different concern, not the tools lookup this fixture targets, and each is already a short, undusplicated arrange/act/assert. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
Extracted message_types() for the two TestExecutionReporting sites that hand-rolled a count/loop over mock_tools.send_event.call_args_list instead of reusing the projection this file's sibling files already extract this shape into. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
… credential gating band_send_message requires a non-empty mentions list, but a freshly created agent room has no other participant and self-mention is disallowed by design. ensure_mentionable_participant() adds the room-owning human (or a given identifier) as a real participant before sending, so both previously xfail tests now pass for real. Also replaces every skip that was masking a real failure or a missing credential with a hard failure: live_config now requires both BAND_AGENT_KEY and BAND_USER_KEY (aliased to the already-present BAND_API_KEY_USER), agent_room asserts instead of skipping when room creation fails, and the now-dead "scope not served" skips are removed since scope is always [AGENT, HUMAN]. Adds a genuine two-agent scenario (test_two_agents_collaborate_in_shared_room) backed by a second real agent identity (BAND_AGENT_KEY_2, aliased to the existing BAND_API_KEY_2/TEST_AGENT_ID_2), sharing the harness-building code via _build_harness() to keep the two identities' fixtures DRY. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
AlexanderZ-Band
commented
Aug 19, 2026
local_server.py mixed three concerns: an sse_starlette shutdown-bug workaround, tool-registration building (RoomToolResolver, build_band_mcp_tool_registrations, build_resolved_band_mcp_tool_registrations), and the actual LocalMCPServer lifecycle. The registration builders never touched sockets/uvicorn/starlette and depended entirely on primitives engine.py already owns, so they move there -- engine.py already builds every other Band MCP tool registration. Updated all in-repo import sites (backends.py, the mcp_server.py compat shim, three test files); both files were already on the MCP import-boundary allowlist, so no allowlist change needed. Also collapses the sse_starlette shutdown-bug explanation, previously told three times (module docstring, a standalone comment, and EmbeddedUvicornServer's docstring), down to one authoritative spot, and dedupes _reserve_socket's twice-repeated socket setup into two small helpers -- which also narrows the range-scan's try/except to just bind(), no longer silently swallowing a listen()/setblocking() failure as "port taken." No behavior change: LocalMCPServer's public surface (including the pre-existing .url property) is untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
AlexanderZ-Band
commented
Aug 19, 2026
…ework swap - core/types.py: WideEventMessageType had exactly one consumer (engine.py's SendEventWideInput) and retyped EventMessageType's members instead of composing them. Moved it next to that consumer and derived it from EventMessageType, closing both the misplaced-indirection and duplicated-taxonomy complaints in one fix. - claude_sdk.py/copilot_sdk.py: both hand-roll a per-message room-context label instead of using the existing CHAT_ID_FIELD_NAME constant (already used correctly by opencode.py and acp/client_adapter.py). copilot_sdk.py's version said "room_id", violating the documented invariant that model-facing text always says "chat_id" -- a real bug, not just a style gap. Both now derive from CHAT_ID_FIELD_NAME; fixed the test that had encoded the wrong string. - tests/integration/mcp/conftest.py: renamed live_config_2/harness_2 to second_agent_config/second_agent_harness (a numeric suffix named nothing about the fixture's role). Replaced ensure_mentionable_participant's dual-mode "identifier=None means discover the owner instead" design with a single-purpose add_room_owner -- adding a *known* second agent is a one-line band_add_participant call, no helper needed. - test_full_workflow.py: merged test_agent_create_room_send_and_read_back and test_two_agents_collaborate_in_shared_room (near-duplicate scenarios, one per participant kind) into one test_agent_room_with_human_and_second_agent covering both a human and a second real agent identity in the same room, including a mention with more than one target. - packages/band-mcp: replaced argparse with Typer. Typer is already a real transitive dependency (mcp[cli] requires it), so this adds no new install footprint -- just promoted from implicit to an explicit direct dependency. Native Enum support replaces the manual `type=Transport, choices=list(Transport)` wiring; _cli_mapping's Namespace-to-dict flatten is gone entirely since Typer hands the command function typed parameters directly. resolve_config/ validate in config.py are untouched -- they're deliberately pure and framework-agnostic, and stay the single place CLI>env precedence and credential validation happen. Verified live: --version, --help, the missing-credential exit(2) path, and a real stdio tools/list round trip all match the pre-swap contract (tests/mcp/test_cli_contract.py, 5/5). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
- runtime/tools.py: TOOL_DEFINITIONS had every tool's name typed twice -- once as the dict key, once as the ToolDefinition's own name= field -- with nothing structural stopping the two from drifting apart. Now built from a tuple of ToolDefinition entries (name typed once) with the dict derived by keying on .name. - band_mcp/server.py: main()'s "Order of operations: 1-6" docstring was narrating six responsibilities glued into one function. Extracted three of them into named steps (_resolve_validated_config, _exit_on_config_error, _run_transport) -- also killing a duplicated exit(2)+log ConfigError pattern that appeared twice. main() now reads as its own step list via the calls themselves; no docstring narration needed. - engine.py: the WideEventMessageType move in the last commit left its new rationale comment immediately followed by the old comment's now-redundant first paragraph restating the same thing. Removed the duplicate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
create_room() and main() each hand-rolled the same stdio_client(...)/ClientSession(...)/initialize() nesting to get a ready session. Factored into open_session(), an async context manager -- both call sites now just state what they need (a session, optionally pinned to a room) instead of repeating how to get one. The other two band_mcp examples (02, 03) were already this shape -- each helper used once or twice with nothing to extract. Left the per-file logging/base_url boilerplate duplicated across all three examples as-is: these are standalone, copy-paste-able PEP 723 scripts by design (this repo's own example convention), so a shared helper module would break that -- a reader copying just one file shouldn't need a sibling module to run it. Verified live against the real platform: room creation, tool listing, peer discovery, participant add, and message send all still work end-to-end through the refactored open_session. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
AlexanderZ-Band
commented
Aug 19, 2026
… output
Leading underscore is a convention for module-private functions -- not
classes, which Python has no equivalent privacy convention for and where
band-sdk's own style is plain PascalCase. Renamed every underscore-prefixed
class across the codebase (17 total): TimeoutNotSet, ResyncRequest,
BacklogProcessResult, PendingToolCall, SessionCommand, SlackTeeingTools,
SeenEvents, CodexClientProtocol, PendingApproval (both codex.py and
claude_sdk.py have their own, unrelated to each other), TurnResult,
RoomCacheEntry, RoomLockEntry, AmbiguousReply, RoomContext, RoomState.
langchain.py's own _ParsedToolResult is renamed to LangChainParsedToolResult,
not the bare name -- band.converters.parsing already has a real, different
ParsedToolResult, and this file only imports parse_tool_result/parse_tool_call
from there, not that type, so the two could otherwise be confused for the
same shape. No filenames needed the same fix -- the __init__.py/__main__.py
hits are Python's own required convention, not a style choice.
Also fixes a real CI failure this surfaced while re-verifying: band_mcp's
Typer app left rich_markup_mode at its default, which lets Typer's
Rich-based --help renderer emit ANSI color codes -- inserted *inside*
option names ("--user-key" splits into several separately-colored spans)
whenever Rich's terminal-capability detection decides the output stream
supports color. That detection is environment-dependent: plain locally
(macOS), colored on Ubuntu CI for the identical piped-subprocess call,
which broke test_cli_contract.py's plain substring match on Ubuntu only
(Windows/macOS both passed). Setting rich_markup_mode=None forces plain,
deterministic help output -- verified with FORCE_COLOR=1 set explicitly,
not just by re-running the previously-flaky case.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
AlexanderZ-Band
commented
Aug 19, 2026
AlexanderZ-Band
commented
Aug 19, 2026
…ion in shared.py runtime/tools.py: moved SEND_MESSAGE_TOOL_NAME earlier so ROOM_POSTING_TOOL_NAMES and AGENT_ROOM_BOUND_TOOL_NAMES reference it instead of repeating the literal. Consolidated every other standalone "is this the send-message tool" check/constant to the same constant: crewai/tools.py's own redundant _SEND_MESSAGE_TOOL local constant is gone entirely, claude_sdk/tools.py's mention-hint-enrichment check, letta/ prompts.py's SEND_MESSAGE_TOOL_NAMES tuple, and agno.py's reply-detection check. Left literals alone where they're one member of an already-uniform enumerated collection (crewai's per-tool category dict, the format-success- payload if/elif chain, slack's write-tool set) or docstring prose -- converting just one member there would be a mixed-style regression, not an improvement. band_mcp/shared.py: `assert self._agent_rest is not None` replaced with an explicit if/raise (asserts vanish under `python -O`, so a real invariant in production code needs a real check) -- matches the sibling guard three lines below it. AGENT_TOOLS_LOCK_STRIPES was a second name for exactly AGENT_TOOLS_CACHE_MAX_SIZE's value with no independent existence; removed it and reference the real constant directly at its one use site, moving the explanatory comment there. Also dismissed two CodeQL alerts (py/bind-socket-all-network-interfaces, local_server.py:311,327) as won't-fix: LocalMCPServer's own docstring already documents 0.0.0.0 as an intentional, opt-in, explicitly-warned-about Docker-bridge configuration, not a default or an accident. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
Both describe the same field (name, max length); splitting them across runtime/tools.py and engine.py meant a reader had to know to look in two files for one field's canonical properties. Moved CHAT_ID_MAX_LENGTH next to CHAT_ID_FIELD_NAME; engine.py imports it like it already imports the field name. Verified it isn't duplicated anywhere else in this repo or in the band-client-rest dependency -- it's a standalone choice for this schema-extension code, not mirroring an existing backend constraint. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
… live coverage An audit of tests/mcp/, tests/integrations/mcp/, and tests/integration/mcp/ found several tests reaching for MagicMock/AsyncMock where a real FakeAgentTools already existed and would exercise more production code, and no agent-surface test chained multiple tool calls into one realistic session the way the human-surface tests already do. - Deleted test_engine_mount_spike.py: its own docstring says it's a feasibility gate superseded once local_server.py was built, which happened earlier this session -- confirmed the module exists and is real product code before removing the spike. - test_standalone_spec.py / test_shared.py: replaced bare MagicMock/AsyncMock agent-tools doubles with real FakeAgentTools (subclassed where a test needs order-tracking, a forced refresh failure, or a bare BandToolError with no hint pre-applied). The load-bearing fix is in test_shared.py's error-enrichment test: the old mock raised a bare ValueError, so engine.py's `except (ValueError, BandToolError)` branch for the real exception type was never actually exercised. - test_local_server.py: replaced a MagicMock(data=[]) participants response with the real ListAgentChatParticipantsResponse Fern model, and narrowed a `pytest.raises(Exception)` to the real BandToolError production code raises. Verified live: the MagicMock stand-in silently tolerates an attribute-name typo in get_participants() that the real Pydantic model correctly rejects with AttributeError. - test_engine.py: three new fake-backed unit tests exercising this codebase's own dispatch/caching/registration logic -- test_agent_multi_step_room_lifecycle (add_participant -> send_message mentioning that participant -> get_participants, each step depending on the prior's real mutated state), test_custom_tool_alongside_builtin_tools_in_one_session (a custom tool's handler mutates the same FakeAgentTools a built-in tool then reads), and test_concurrent_dispatch_through_one_engine (drives StandaloneResolver's real identity resolution, per-room caching, and lock striping through a real engine's `_tool_manager.call_tool`, asserting no cross-room leakage and that the LRU cache stays bounded). No fake-backed test for cross-identity isolation was added -- that invariant depends on real backend behavior and stays live-only, per this session's existing test_full_workflow.py::test_agent_room_with_human_and_second_agent. - test_full_workflow.py: added test_agent_send_message_retry_after_lookup_peers, a new live test against platform.dev.band.ai. Mentioning a real peer found via band_lookup_peers who isn't yet a room participant is observed to fail live with `Unknown participant '<id>'`; band_add_participant then makes the identical retry succeed. Every new/fixed test was verified to fail for the right reason (by reverting the fix or breaking the real production code path it guards) before being confirmed green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
AlexanderZ-Band
commented
Aug 19, 2026
AlexanderZ-Band
commented
Aug 19, 2026
AlexanderZ-Band
commented
Aug 19, 2026
Adds a bump-mcp-sdk-floor job to release.yml, mirroring bump-add-band's shape, so packages/band-mcp/pyproject.toml's band-sdk>= floor always tracks the most recently published SDK version instead of relying on a manual bump once the right version number is known. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
AlexanderZ-Band
commented
Aug 19, 2026
AlexanderZ-Band
commented
Aug 19, 2026
…e, imports - test_reply.py: assert the chat_id room-context marker via CHAT_ID_FIELD_NAME instead of a hardcoded "[chat_id: ...]" literal that duplicates the constant copilot_sdk.py itself builds the prompt from. - test_client.py: replace dispatch()'s nonlocal-closure callback with a plain AsyncMock, reading the received payload off its await_args. - test_transport_security.py: hoisted every function-local band_mcp/ engine import to the top of the file; dropped TestDnsRebindingProtectionBehavior, which characterized the mcp SDK's own TransportSecurityMiddleware in isolation rather than anything band-mcp builds -- the "our config reaches the SDK correctly" claim it made is already covered by TestMcpTransportSecurityIntegration, which drives band-mcp's real factories. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
release-please only bumps a component's own version off a bump-worthy commit type (feat/fix/breaking, not chore). A chore:-typed floor bump would merge into main without ever triggering a release-please PR for packages/band-mcp, leaving its pyproject.toml version stuck at the last release -- which band-mcp-publish.yml's tag-must-match-version check then rejects on the next band-mcp-v* tag. Retyped the bump-mcp-sdk-floor job's commit/PR title to fix: (branch prefix to match) so merging it actually opens band-mcp's next release-please PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfY2tLM1peE7YwhYDsMs1M
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Migrates
band-mcpinto this repo as a sibling workspace package (packages/band-mcp), and builds one MCP-framework-neutral tool-registration engine consumed by two front doors, per the plan attached to INT-1096 ("Migration plan: one MCP engine (v5)").All 13 execution steps are now complete, all 20 review threads addressed and resolved. One user-owned action item remains before a
band-mcprelease can actually ship (see below); the repo-archiving item is done whenever you get to it post-merge.Progress (one commit per step)
packages/band-mcpas a uv workspace member. Found and resolved a real crewai/mcp dependency conflict (band-mcp's own lock).band_lookup_peersroom-scoping,band_create_chatroom's string return).FakeHumanTools, wire-schema snapshot test locking in band-mcp 1.3.2's published contract before the engine touches anything.src/band/integrations/mcp/engine.py— the one engine, verified via real MCP protocol round trips (SDK in-memory transport).src/band/integrations/mcp/local_server.py(LocalMCPServer) — the embedded front door, rebuilt on the engine.src/band/runtime/mcp_server.pyis now a pure re-export shim for backward compat.chat_idrename through prompts,claude_sdk, and custom tools — the Python-side variable is stillroom_ideverywhere; only model-facing text (schemas, prompts) sayschat_idnow, uniformly across both doors.standalone_spec()(per-tool room classification,SendEventWideInputwidening, chat_id pinning) +StandaloneResolver(human singleton, LRU-cached/lock-striped per-roomAgentTools). Legacythnv_*key prefixes dropped per your decision — no compatibility shim.--all-packageseverywhere inci.yml, fixed the packaging job's now-brokenLocalMCPServersmoke (real start/stop cycle instead), added band-mcp wheel/import/CLI verification steps, newband-mcp-publish.yml(mirrorsband-publish.yml's trusted-publishing layout, waits for the declared band-sdk floor to land on PyPI before its install-check), tag-prefix guard onband-publish.ymlso a band-mcp release doesn't also fire (and fail) the band-sdk publish job,packages/band-mcpregistered as its own release-please component withexclude-pathson the root component,mcpadded topr-title.yml's scopes.AGENTS.md/CLAUDE.md, fixed two now-staleroom_id-vs-chat_idmentions.CHAT_ID_FIELD_NAME(the model-facing room-id argument name, now referenced by the engine's schema builders and every adapter prompt that mentions it),Surface(replacesToolDefinition.surface's bareLiteral["agent","human"]), andScope/ToolGroup/Transport(replace band-mcp's ownLiteraltypes plus their hand-maintained parallelVALID_SCOPES/VALID_TOOLSlists). Also removed the# type: ignore[arg-type]/cast()escape hatchesresolve_config'sMapping[str, object]boundary needed, by giving it a concreteCliArgsTypedDictinstead.python3 -c "..."snippet inci.yml/band-mcp-publish.ymlwith a quoted heredoc (<<'PYEOF') — the old form only worked because YAML strips the block's indentation, and silently allowed shell$/backtick/backslash expansion inside what's meant to be inert Python source.build_engine()never told FastMCP the caller's real bind host, so FastMCP always assumed loopback and locked DNS-rebinding protection to127.0.0.1/localhostonly — rejecting ahost.docker.internalcaller with a 421 even whenLocalMCPServerwas explicitly bound to0.0.0.0for exactly that documented Docker-callback case. Fixed by threading the real host through; added regression tests intest_engine.py/test_local_server.py.BAND_API_KEYsupport entirely (breaking change) —_legacy_key_capabilities,Config.legacy_key, the pure-legacy escape hatch inserver.py, and the deadSettings.band_api_keyfield are all gone. SetBAND_USER_KEY/BAND_AGENT_KEYexplicitly now.tests/integration/mcp/(the live-API integration suite) has been silently uncollectable since step 11 — itsconftest.pystill importedband_mcp.tools.registrar/band_mcp.shared.build_app_context, both deleted that step. Rewrote the fixtures onto the current engine API and deletedtest_forwarding.py(superseded bytest_standalone_spec.py/test_engine.py). Verified live against the dev platform: 4 passed, 4 skipped, 2 xfailed (pre-existing, unrelated), 0 failures.MagicMock/AsyncMockstanding in forAgentTools) for the realFakeAgentToolsfixture acrosstests/mcp/test_standalone_spec.pyandtests/mcp/test_shared.py, so the participant-refresh/discard-on-failure/mention-error paths exercise real dispatch logic instead of a hand-typed assumption about it. Added a multi-step room-lifecycle test, a custom-tool-alongside-builtin-tools test, and a concurrent-dispatch-across-rooms test totest_engine.py; added a live retry test (test_agent_send_message_retry_after_lookup_peers) totests/integration/mcp/test_full_workflow.py. Deletedtest_engine_mount_spike.py(superseded feasibility spike, docstring said as much).tests/fixtures/wire_schemas/*.json) withtests/mcp/test_wire_contract.py— declarative, hand-written per-tool contracts checked against a realtools/listround trip, instead of an opaque diff against a checked-in blob a reviewer can't meaningfully read.bump-mcp-sdk-floorjob torelease.yml(mirrors the existingbump-add-bandjob's shape) that opens a PR bumpingpackages/band-mcp/pyproject.toml'sband-sdk>=floor to match every future band-sdk release, so this never needs a human to remember it again.Decisions made along the way
thnv_*key prefixes: the plan's one open decision (keep vs. drop). User decided: drop — no compatibility retained. Applied in step 11.mcppin and this migration'smcpfloors, resolved by capping atmcp>=1.28.1,<2instead of>=1.29.0,<2everywhere — confirmed with the user both times.SendEventWideInput(the CLI door's widened event-type input) is an independent model, not a subclass of the SDK'sSendEventInput— pyrefly correctly flagged that widening a mutable Pydantic field's type via subclassing is unsound.async with LocalMCPServer(...)start/stop cycle (the old_build_server()it called no longer exists on the new class) — this exercises the actual FastMCP-mount + uvicorn serve/shutdown recipe against a freshly resolvedmcp, which is the failure mode that step originally existed to catch.User-owned action items
PyPI trusted-publisher binding for— done.band-mcpband-mcp-publish.ymlis registered as a trusted publisher on theband-mcpPyPI project (environmentrelease), same asband-publish.ymlis forband-sdk. The project's old trusted-publisher entry (from the standaloneband-mcprepo) is intentionally left in place until the new one is confirmed working, then should be removed.band-mcprepo — still open. Once this PR merges and aband-mcp-v*release ships from here, the standaloneband-mcprepo should be archived (or at minimum READMEd to point here) so it doesn't keep accepting PRs against dead code.packages/band-mcp/pyproject.toml'sband-sdkfloor before tagging the firstband-mcp-v*release — now automated end to end, but not yet correctable. It's currently>=1.6.0— a real, already-published version, but one that predates this PR and lackssrc/band/integrations/mcp/engine.py(confirmed live:band-sdk>=1.6.0+band-client-rest==0.0.27is unsatisfiable today, since publishedband-sdk==1.6.0pinsband-client-rest==0.0.10). This is a deliberate placeholder — the real floor is the band-sdk version this PR's merge actually ships as, which isn't known until release-please cuts that release. Full automated sequence: band-sdk releases → the newbump-mcp-sdk-floorjob (release.yml) opens afix:-typed PR correcting the floor (typedfix:, notchore:, specifically so merging it also triggers release-please's own version bump for thepackages/band-mcpcomponent — achore:would've merged silently and leftpyproject.toml's version stuck, whichband-mcp-publish.yml's tag-must-match-version check would then reject) → merge that PR → release-please opens band-mcp's own version-bump release PR → merge that → tagband-mcp-v*.band-mcp-publish.yml's build job also fails loudly if any of this is skipped (its install-check step hits the exact resolver conflict above before anything reaches PyPI), so there's no risk of a silently-broken publish either way.Test plan
ruff check,ruff format --check,pyrefly checkclean at every step.standalone_spec, no mocking of engine internals.tests/mcp/test_cli_contract.py) against the realband-mcpCLI — stdio stdout purity,--version/--help, published tool names, thehealth_checktitle-leak regression.band-mcpover real stdio against a real dev-environment Band room —initialize→tools/list(all 8 tools incl.health_check) →health_checkOK →band_create_chatroom→band_get_participants→band_lookup_peers, all real HTTP round trips, all succeeded.claude_sdkcore lane — 27 passed, 1 skipped, 1 rerun (transient timing flake, not a regression), 0 failures, against real Anthropic API + real Band rooms. Covers the exact code this migration touched (chat_id/room_idhandling inclaude_sdk/tools.py/prompts.py,extend_with_chat_idin the rebuilt engine).Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com