refactor(mcp): server.py 1617→377 LOC via ToolDispatcher + per-domain handlers#75
Closed
emp3thy wants to merge 20 commits into
Closed
refactor(mcp): server.py 1617→377 LOC via ToolDispatcher + per-domain handlers#75emp3thy wants to merge 20 commits into
emp3thy wants to merge 20 commits into
Conversation
Spec: extract _call_tool if-chain into ToolDispatcher + ServiceContainer + per-domain handler modules. 17 tasks, single big-bang PR, ~94% avg confidence with all sub-90% tasks lifted via parallel investigation. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Bundles every long-lived service the dispatcher needs so they're built once at startup, not per-call (today: SemanticMemoryService 4x, SessionBootstrapService 2x).
- Strip unnecessary quote-string forward refs (ruff UP037) — the `from __future__ import annotations` import already defers evaluation, so quoting is redundant. - Restore `semantic: SemanticMemoryService` type (was `Any`) by adding the import to the TYPE_CHECKING block alongside the other services. No runtime circular-import risk: container.py is only imported by the MCP dispatcher and services/semantic.py only depends on services/memory_rating. - Strengthen frozen-dataclass test: assert `dataclasses.FrozenInstanceError` is raised on attribute reassignment, in addition to the existing structural checks. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replaces the 470-LOC if-chain with O(1) name lookup. Capability gate preserves the existing 'Unknown tool: ...' ValueError shape so clients don't notice.
No behaviour change. Lets handler modules import it without a circular dep on the server module.
Audit JSONL byte-shape unchanged. Test updates import path only.
…_counts Two MCP dispatcher branches were the only callers; making this public removes a leak the handler-module split would otherwise expose. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ugh create_server Legacy _call_tool if-chain still drives the actual dispatch; container is now the canonical source for every long-lived service. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…down The _wrap helper inside test_build_services_constructs_each_service_exactly_once was permanently mutating SemanticMemoryService.__init__ and SessionBootstrapService.__init__ via direct cls.__init__ = _init assignment. Any test running after this one continued executing the wrapped __init__, a slow leak that monkeypatch was already in scope to prevent. Switch to monkeypatch.setattr so pytest reverts the class patch on teardown. Also tighten the assertion comment to scope the regression statement to _build_services (the only thing this test exercises). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…etrieve_observations + record_use) Schemas + bodies lifted verbatim from _call_tool. spool->retention->backend ordering preserved. ObservationHandlers exposes one method per tool + handlers() registration list for ToolDispatcher. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ve + update + delete) SemanticMemoryService is now built ONCE on the container, not 4x per call.
Task 9 of the MCP server dispatcher refactor. Lifts the four episode lifecycle tools out of the legacy _call_tool if-chain into a domain handler module. Bodies are verbatim, including the close_episode try/except fork that converts EpisodeService.close_active's ValueError into the already_closed=True no-op payload, and the 10-field list_episodes serializer the UI depends on. _serialize_queue is duplicated as a small module-local helper to keep the handler module self-contained. After this task, all_handlers() returns 12 (4 obs + 4 sem + 4 ep). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds ReflectionHandlers covering the two capability-gated synthesis tools. Bodies lifted verbatim from the legacy _call_tool if-chain to preserve every documented invariant: audit bracketing on both tools, the SynthesisResponseError -> validation_error fork (no synth_failed_at stamp; caller can retry), the ValueError -> state_error fork (caller must refetch context), and the four serializer payload shapes the IDE-LLM consumes. Both handlers are marked requires_synthesis=True so the dispatcher hides them when backend.supports_synthesis is False. all_handlers() now returns 14 tools (4 obs + 4 sem + 4 ep + 2 ref). Task 10 of 17 of the MCP dispatcher refactor. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Task 11 of MCP server dispatcher refactor. Lifts the memory.run_retention branch out of the _call_tool if-chain into RetentionHandlers, registered via handlers/__init__.all_handlers(). Body and schema are verbatim from server.py; `retention.run(...)` is re-routed through `services.retention.run(...)` on the ServiceContainer. The serialized payload is the 4-field RetentionReport (three archived_via_* counters + pruned) — not 8 as the task description suggested; the dataclass has 4 fields. Tests cover the success serialization, default-arg fan-out, explicit overrides, and the one-tool registration. all_handlers() now returns 15 handlers. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Task 13 of the MCP server dispatcher refactor. Adds the three
rating-domain handlers as a RatingHandlers class behind the
ToolDispatcher / Handler abstraction, lifting the bodies and JSON
schemas verbatim from the legacy _call_tool if-chain.
- memory.list_session_exposures: coerces an unresolved session id to ""
so the bootstrap service can return an empty exposures payload
instead of raising.
- memory.apply_session_ratings: raises the verbatim 3-line ValueError
when no session is active (the rate-session-memories skill matches
the exact text).
- memory.credit: two payload shapes - {"applied": None,
"skipped": "no_session"} when no session, otherwise the ApplyOutcome
from MemoryRatingService.credit_one. Note the schema's class enum
intentionally omits "ignored" (apply-only).
Registered in handlers/__init__.py so all_handlers() now returns 20
handlers across 7 domain modules.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Task 14/17 of the MCP dispatcher refactor. Lands handlers/session.py with SessionHandlers exposing memory.session_bootstrap (4-tier session_id fallback: args -> CLAUDE_SESSION_ID -> CLAUDE_CODE_SESSION_ID -> uuid; 5-key payload) and memory.start_ui. Uses the services.session_bootstrap singleton from the container (built once in Task 6) instead of re-instantiating SessionBootstrapService per call. After this task all_handlers() returns 22 -- the full tool set. Task 15 will wire ToolDispatcher into create_server.
server.py drops from 1568 to 401 LOC. _call_tool is now a 1-line
delegation to dispatcher.call(name, arguments or {}) (under _diag.trace).
_tool_definitions deleted as the 550-LOC schema function; replaced by a
thin shim built from all_handlers() so tests asserting tool registration
keep working. All 22 tools route through the dispatcher.
To break a server <-> handlers import cycle introduced by importing
all_handlers() at module load, extracted _run_best_effort into
mcp/_best_effort.py. server.py re-exports it so the existing test
import path (better_memory.mcp.server._run_best_effort) still works;
test_best_effort_logging now asserts the canonical logger name
(better_memory.mcp._best_effort).
ServerContext gains a `dispatcher` field LAST so positional construction
keeps compiling. Synth serializers (_serialize_synth_*, _serialize_queue)
re-exported from handlers.reflections for test backwards compat.
_serialize_knowledge_* deleted (no callers).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
5 LOC instead of 40. Same signature, same behaviour.
…617 → 377 LOC All 22 tools route through ToolDispatcher. Handler bodies + schemas co-located in mcp/handlers/. ServiceContainer built once at startup. Final pytest: 1180 passed, 23 skipped, 0 failed (chunked across 4 junit runs because full-suite stdout buffer was truncated by the harness). Pyright clean on better_memory/mcp/ (0 errors, 0 warnings). Ruff on better_memory/mcp/ + tests/mcp/: 5 pre-existing errors (down from 6 at base 22c3d26); none introduced by this refactor. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Owner
Author
|
Superseded by PR #73 (already merged). Same target hit with a different architecture — closing to avoid duplicate work. Will file follow-up PRs for the architectural extras (frozen ServiceContainer, capability-gate dataclass) against the current main if desired. |
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
1 participant
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
better_memory/mcp/server.py(1617 LOC god-module) into aToolDispatcher+ 8 per-domain handler modules + a frozenServiceContainerbuilt once at startup._call_toolis now a 2-line delegation (return await dispatcher.call(name, args))._tool_definitionsis a thin shim built fromall_handlers(). Every long-lived service (SemanticMemoryService, SessionBootstrapService, etc.) is constructed once instead of per-call.mainstill passes.ValueError("Unknown tool: ...")shape preserved for capability gating.What changed
better_memory/mcp/container.py(frozenServiceContainer, 12 fields)better_memory/mcp/dispatcher.py(Handlerdataclass +ToolDispatcher)better_memory/mcp/_session.py(resolve_session_id)better_memory/mcp/_best_effort.py(_run_best_effort— extracted to break server↔handlers import cycle)better_memory/mcp/handlers/(8 domain modules +_audit.py, totalling ~1600 LOC)ReflectionSynthesisService.read_queue_counts(was_read_queue_counts)server.py: 1617 → 377 LOC (-77%). Legacy_tool_definitions+_call_toolif-chain deleted. Thin re-export shims kept for 8 existing test files that import helpers by name.Architecture
Test plan
tests/mcp/: 158 passed, 6 skippedtest_service_container.py(constructed-exactly-once),test_tool_dispatcher.py(capability gate + unknown-tool ValueError shape), 8 per-domain handler test filesuv run ruff check better_memory/mcp/— cleanuv run pyright better_memory/mcp/— 0 errors, 0 warningscreate_server()returnsctx.dispatcherwith 22 toolsFollow-ups (separate PRs)
_tool_definitions,_serialize_*,_run_best_effort) by rewriting the 8 test files to import from canonical locations.website/mcp-tools.mdto point atbetter_memory/mcp/handlers/for live tool schemas.ServerContextto@dataclass(frozen=True)for consistency withServiceContainer+Handler.🤖 Generated with Claude Code