Repository navigation
feat(mcp): add MCP client and manager with per-server failure isolation - #26
Closed
alanwang233233 wants to merge 20 commits into
Closed
alanwang233233 wants to merge 20 commits into
alanwang233233 wants to merge 20 commits into
Conversation
Co-authored-by: alanwang233233 <107164653+alanwang233233@users.noreply.github.com>
Normalises Claude Desktop style MCP server entries into a typed McpServerConfig, validating that url and command are mutually exclusive and building the matching StreamableHttp or Stdio transport object. Constraint: Config arrives as a parameter; this layer never touches the filesystem Rejected: fastmcp-slim[client] vs fastmcp split | fastmcp-slim already ships the full server code in 4.1, the split is only extras Confidence: high Scope-risk: narrow
env and headers routinely carry static tokens, and a single malformed entry caused the whole mapping to be rendered into the exception message, which then lands in the TUI error pane and in logs. Report only the offending entry's key. Auditing the remaining helpers found the same pattern in url (a credential can sit in userinfo or the query string) and args (command-line flags such as --token <secret>), so both now report a scheme, an index or a type name instead of the value. That is also the more actionable message: 'args[3]' beats a dump of the whole argv. Constraint: headers/env must never appear by value in any exception message Rejected: type-name-only messages for timeout/cwd/command too | those fields are not credential-bearing by definition, and reporting 'got 0' or 'got -1' is what makes a numeric mistake fixable Confidence: high Scope-risk: narrow
Enumerate every validation branch of mcp_config.py and pin each one: the non-dict/blank/whitespace entry shapes, non-bool enabled, NaN and bool timeouts, and the unreachable-from-parse unknown-kind guard. Adds sentinel credentials and asserts they never appear in exception messages, and pins that parse copies the caller's lists and mappings rather than aliasing them. 89% to 100% statement and branch coverage. A 37-mutant sweep against a scratch copy killed 36; the survivor is an equivalent reflow of one message literal, which is the expected result. Constraint: src/ is read-only to the test writer; no source file changed here Rejected: hypothesis property tests | no hypothesis in the dependency set, and the sweep already covers the two invariants Confidence: high Scope-risk: narrow
Two gaps found in review of the McpServerConfig suite:
- The strict "greater than zero" bound was only pinned incidentally, by
timeout: 0 in the pre-existing rejection test and connect_timeout: -1.
Add an explicit boundary test over int 0, float 0.0 and -1 across both
timeout fields so the strictness is pinned directly rather than inferred
from a neighbouring case.
- build_transport passed headers=self.headers or None, but only the empty
case was observed. Replacing that expression with a bare headers=None
survived the whole suite, so a server silently losing its Authorization
header had no signal. Pin the populated case at the call site, where
fastmcp's own headers or {} normalisation hides the distinction.
Also record the deliberately unpinned limitations of the module in the test
module docstring: shallow immutability of the frozen dataclass, the
auto-generated repr printing url verbatim, float("inf") passing the timeout
check, "http://" with no host being accepted, and the credential-in-key
position that is echoed by design.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Force close records rather than raises a failed client.close() or stack.aclose(), so those branches needed direct coverage. Adds fakes for both handles, an idempotent second disconnect after a wedged teardown, a no-op disconnect on a never-connected client, and a connect that fails before any handle exists.
One MCP server connection per client. connect() opens the transport and caches the server's tool catalogue in one step, and records failures into state and last_error instead of raising, so one unreachable server cannot abort a batch connect. call_tool() renders a result to text because AgentRuntime calls str() on whatever a tool returns. The timeout path calls Client.close() rather than merely abandoning the await. StdioTransport spawns its subprocess in a detached task and the caller only waits on that task's ready event, so asyncio.wait_for's cancellation would leave the connect task running and the child process alive as an orphan. Handles are assigned before the context is entered, and cleanup runs outside the timed-out await so the teardown itself is never cancelled. Constraint: Result must render to a string, because AgentRuntime calls str() on tool return values Rejected: ClientGroup.from_config | its __aenter__ connects every client, so one bad server kills all tools Confidence: high Scope-risk: narrow
Two review findings on McpClient, both about the same invariant: never orphan a subprocess, never leak a credential. The ToolError path interpolated fastmcp's text verbatim, and for an argument-validation failure that text is pydantic's, whose input_value= segment is the argument itself. A rejected token reached the exception message and from there the TUI error pane and the logs, contradicting the no-leak constraint the module's own comment asserts. Payloads are now cut out by _redact_arguments; tool, server, field name, expected type and input_type all survive, so the message still says what was wrong, just not with what. Cancellation derives from BaseException, so it bypassed the except Exception handler entirely and left _client/_stack populated with stdio's detached connect task still holding the child process -- exactly the orphan the rest of connect() is built to prevent. connect() now catches BaseException, cleans up under asyncio.shield so a repeat cancellation cannot interrupt the teardown, records the failure, and re-raises for anything that is not an ordinary Exception. Constraint: connect() keeps returning False for ordinary failures so one unreachable server cannot abort a batch connect; only cancellation re-raises. Constraint: no credential value may reach an exception message. Rejected: never raising on cancellation | swallows Ctrl-C and TUI shutdown. Rejected: re-raising with a bare 'raise Exception' | loses the cancellation identity the caller needs to unwind cleanly. Confidence: high Scope-risk: narrow
…ation The suite already reported 100% statement and branch coverage of mcp_client.py, so this adds no lines to the coverage report. It pins what that report cannot see: behaviours the existing tests let through. Redaction (the credential leak shipped in c18605f): - adversarial payloads carrying a decoy ", input_type=" marker, both with and without trailing report text after the decoy - two payloads on one line, which pins the greedy match to the *last* marker -- a non-greedy pattern silently leaves the second argument in the log - a multi-error, multi-line pydantic report, so one payload cannot eat the next error's field name and expected type - empty and suffixless payloads - end-to-end through both call_tool error branches, asserting the tool name, server name, field name and real input_type= suffix all survive, so redaction cannot be "achieved" by deleting the report Teardown (the orphaned subprocess shipped in c18605f): - repeat cancellation while Client.close() is in flight: only a shielded teardown runs to completion, so this is what pins the shield itself - a repeat cancellation skips the FAILED bookkeeping entirely - a BaseException that is neither Exception nor CancelledError still tears the transport down and propagates, which is what pins the handler on BaseException rather than (Exception, CancelledError) Also pins behaviour that no test touched: the timeout keyword reaching the server call, ensure_ascii=False and default=str in result rendering, last_error being cleared by a successful retry, a teardown error being overwritten by the handshake error, the real _build_fastmcp_client wiring, and the not-connected report naming both server and tool. Verified against 24 mutants of a scratch copy of the module under /tmp; all 24 are killed. Coverage stays 100% statement and branch.
…rrow catch Three review fixes, all confined to test/test_mcp_client.py. An assertion was enforcing a credential leak. test_redaction_cuts_the_payload_up_to_the_last_injected_type_marker asserted the redacted output character-for-character, including that `sk-PART2LEAK` survives a payload that supplies its own `", input_type="` decoy. That green-lights a leak in a suite whose binding constraint is never to leak argument values, and worse, it makes a plausible fix -- requiring the suffix to authenticate as a real type token -- fail as if it were a regression. The test now asserts the security property and the documented prefix, and names the residual leak in a comment saying that closing it requires DELETING the assertion. Coverage of the decoy shape is not lost: test_redaction_keeps_the_report_after_an_injected_marker still pins the exact output and now kills the suffix-dropping and suffix-eating mutants that the weakened assertion used to catch. _force_close's handlers were untested one call frame below connect's. _mcp_client.py:205 and :210 catch Exception while connect's handler at :171 catches BaseException, and widening the teardown handlers to BaseException survived the whole suite. test_force_close_does_not_swallow_a_shutdown_ signal_raised_while_closing pins that a signal from close() escapes disconnect() and leaves last_error untouched -- which is what stops disconnect() from swallowing an in-flight cancellation. Verified: that mutation now fails the suite. test_redaction_cuts_the_last_payload_when_a_line_reports_several was misnamed; it cuts everything up to the last marker, not the last payload. Renamed to test_redaction_cuts_up_to_the_last_marker_when_a_line_reports_ several, docstring included. Also softened the builder test's docstring, which overclaimed full config wiring: fastmcp 4.1 accepts Client(timeout=) but keeps it off the client, so that keyword stays unpinned and the docstring now says so. 48 in the file, 279 repo-wide, mcp_client.py still 100% statement and branch. 14 mutants re-checked against a scratch copy under /tmp, all killed.
register() now honours a caller-supplied parameters_schema because MCP servers ship their own JSON Schema that annotation inference cannot derive. register_mcp_client() namespaces tools as mcp__<server>__<tool> and refuses to overwrite an existing registration; unregister() lets a later layer drop a server's tools again. Constraint: McpClient referenced only under TYPE_CHECKING so tool_manager imports without fastmcp installed Rejected: Registering via AgentSession's tools_map/tool_schemas kwargs | those parameters are deprecated Confidence: high Scope-risk: moderate
…voke's dispatch namespace The 18 existing tests pinned the MCP registration paths but left four things unverified, plus 15 uncovered branches in the schema derivation. - register_mcp_client's logger.warning on a name collision was never asserted. It is now pinned with the exact message, and a clean registration is pinned as quiet. - The fastmcp dependency guard was weaker than advertised in two ways: PYTHONPATH was joined with a trailing ':' and built from a relative path, so the child's cwd joined sys.path and a stray mitocode/ package would shadow the module under test; and an import executed inside a function body sailed past it. The probe now uses an absolute PYTHONPATH with no empty element and an empty tmp_path cwd, and a second probe registers a client before re-checking sys.modules. - _invoke binds its captured client and tool name as default parameters, so a server tool argument named _client or _tool_name binds over them. Both are pinned with their current (defective) behaviour and reported, not fixed. - register()'s parameters_schema meta tier is documented but unreachable: declare_tool writes only name/human_doc/agent_doc. Pinned by writing the private __mitocode_tool_meta__ attribute directly, with a comment saying so. - Closes 15 uncovered branches: _annotation_to_schema's unannotated/Union/PEP 604/ Literal/sequence/mapping paths and its unsupported-annotation raise, _build_parameters_schema's positional-only raise and *args/**kwargs skip, the omitted required key, _normalize_input_schema's non-dict properties, and register()'s precedence tiers, docstring fallbacks and display-hint defaults. Module line+branch coverage 75% -> 97%; the two remaining statements are unreachable on Python 3.10 (the types.UnionType ImportError fallback and the single-member Union unwrap).
The generated callable bound its client and upstream tool name as default arguments, putting both in the same namespace as the model's keyword arguments. A server tool advertising an argument literally named _tool_name redirected the call to a different tool on that server -- including one that was never registered, bypassing the name-collision guard register_mcp_client relies on. Replaced with a closure factory that exposes neither value as a keyword. The two tests that pinned the defective behaviour are now xfail(strict=False) asserting the secure contract, so they XPASS on the fix instead of blocking it, and are renamed to read as a known defect rather than a capability. This is the same pattern the branch already ruled against once, on a redaction test that asserted a secret survived. Also: advertise a tool in the dependency-direction probe so the loop body actually executes -- with an empty catalogue a lazy in-loop import passed both probes silently; merge the two subprocess probes, which were 80% of the file's runtime; and block the standalone mcp SDK alongside fastmcp. Constraint: McpClient stays referenced only under TYPE_CHECKING; tool_manager must import without fastmcp installed Confidence: high Scope-risk: narrow
start() connects every configured server concurrently and registers only the ones that came up, so an unreachable server costs you its own tools and nothing else. client_factory is the seam that lets the tests drive both fake and real stdio servers through the same lifecycle. Each server's tools are registered only after its connect() reports success, and released before every re-registration: register_mcp_client never overwrites a name, so a repeated start or a refresh that failed would otherwise leave tools the manager no longer owns and stop() cannot remove. A failure the client did not report itself is recorded against that server and surfaced through status(), so "connected but no tools" always has a reason next to it. Constraint: start() must run before any AgentSession is created, since AgentSession snapshots the tool registry Rejected: ClientGroup.from_config | a single failing server fails the whole group's connect Confidence: high Scope-risk: moderate Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…uarded teardown Three defects the review found, all in the manager's own file. The mcp package imported tool_manager at runtime, breaking the plan's one-way dependency and making mitocode.backend.mcp unimportable without the tool layer. The import moves under TYPE_CHECKING with the annotation quoted, matching tool_manager's own guard for McpClient; the manager only ever needed the two methods it calls. A repeated server name left a tool registered, dispatchable and reported as the other entry's — "disabled", tool_count 0. Names are refused in _normalise_servers instead, naming the position and the name but never a value. Two different servers offering the same tool name are unaffected: that collision guard keys on mcp__<server>__<tool>. stop()'s unregister loop was the module's only unguarded await, and it ran before the disconnects, so a registry that refused to give a tool back skipped teardown entirely and left every connection open. Both phases now run through _run_each, which isolates each server and inspects the gather results: return_exceptions=True also swallows a child's CancelledError, which except Exception cannot catch, so a cancelled connect was dropped with no trace. It is recorded and re-raised. unregister()'s bool now decides ownership instead of being discarded: a tool that was removed, or that was already gone, is dropped from the record, while one whose removal raised stays owned so a later stop() can finish the job. status() prefers a recorded failure over the client's own older message, and stops reporting one a successful retry resolved. Constraint: start() must still never raise for a server that fails; only a cancellation propagates Rejected: setdefault in status() | hides the collision instead of refusing a configuration that cannot describe itself consistently Rejected: disconnect before unregister | leaves a registered tool pointing at a dead client, failing in the model's hands instead of here Confidence: high Scope-risk: moderate Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_connect_one assigned register_mcp_client's result straight over the ownership record. That list omits any tool whose removal had just failed, because the tool is still in the registry and register_mcp_client skips a taken name — so a second start, or a refresh, silently forgot a dispatchable tool: counted nowhere, and reclaimable by nobody. A third stop() could not get it back either. _unregister_one now returns the names it could not release and _connect_one merges them, keeping the recorded error too, so status() and the registry agree again. stop() also propagated a child's CancelledError as soon as its first phase reported one, skipping the disconnections that release the subprocesses — the strand-everything outcome the per-server isolation exists to prevent, reached through the propagation path. Both phases now run and the cancellation is re-raised afterwards. _test hygiene: the import probe no longer takes an unused fixture, and its child has a timeout so a wedged interpreter fails the test instead of the suite. Constraint: start() must still never raise for a server that fails; only a cancellation propagates Confidence: high Scope-risk: moderate Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
CI 检查结果汇总
信息展示
📋 pytest 详细============================= test session starts ==============================
platform linux -- Python 3.14.8, pytest-9.1.1, pluggy-1.7.0
rootdir: /home/runner/work/MitoCode/MitoCode
configfile: pyproject.toml
plugins: asyncio-1.4.0, platformdirs-4.13.3, cov-7.1.0, anyio-4.15.1
asyncio: mode=Mode.STRICT, debug=False, asyncio_default_fixture_loop_scope=None, asyncio_default_test_loop_scope=function
collected 398 items
test/test_agent_runtime.py ............................ [ 7%]
test/test_agent_session.py ............................................. [ 18%]
............ [ 21%]
test/test_main.py . [ 21%]
test/test_mcp_client.py ................................................ [ 33%]
[ 33%]
test/test_mcp_config.py ................................................ [ 45%]
....................................... [ 55%]
test/test_mcp_manager.py ......................................... [ 65%]
test/test_orchestrator_and_bridge.py ............ [ 68%]
test/test_tool_manager.py .............................................. [ 80%]
....................................XX [ 89%]
test/test_workspace.py ...................................... [ 99%]
test/test_workspace_async.py .. [100%]
------ generated xml file: /home/runner/work/MitoCode/MitoCode/report.xml ------
================================ tests coverage ================================
_______________ coverage: platform linux, python 3.14.8-final-0 ________________
Name Stmts Miss Cover
--------------------------------------------------------------------------------
src/mitocode/__init__.py 4 0 100%
src/mitocode/backend/__init__.py 3 0 100%
src/mitocode/backend/agent/__init__.py 8 0 100%
src/mitocode/backend/agent/agent_node.py 10 0 100%
src/mitocode/backend/agent/agent_orchestrator.py 115 35 70%
src/mitocode/backend/agent/agent_runtime.py 91 0 100%
src/mitocode/backend/agent/agent_session.py 133 1 99%
src/mitocode/backend/agent/agent_state.py 8 0 100%
src/mitocode/backend/agent/session_factory.py 6 0 100%
src/mitocode/backend/bridge/__init__.py 2 0 100%
src/mitocode/backend/bridge/event_adapter.py 25 0 100%
src/mitocode/backend/chat_provider/__init__.py 4 0 100%
src/mitocode/backend/chat_provider/chat_provider_openai.py 50 34 32%
src/mitocode/backend/chat_provider/sample_chat_provider.py 39 18 54%
src/mitocode/backend/exception/__init__.py 2 0 100%
src/mitocode/backend/exception/mitocode_exception.py 23 0 100%
src/mitocode/backend/mcp/__init__.py 4 0 100%
src/mitocode/backend/mcp/mcp_client.py 117 0 100%
src/mitocode/backend/mcp/mcp_config.py 100 0 100%
src/mitocode/backend/mcp/mcp_manager.py 122 0 100%
src/mitocode/backend/tool/__init__.py 2 0 100%
src/mitocode/backend/tool/tool_manager.py 138 4 97%
src/mitocode/backend/workspace/__init__.py 3 0 100%
src/mitocode/backend/workspace/migration.py 77 39 49%
src/mitocode/backend/workspace/snapshot.py 0 0 100%
src/mitocode/backend/workspace/workspace.py 180 11 94%
--------------------------------------------------------------------------------
TOTAL 1266 142 89%
Coverage XML written to file coverage.xml
Required test coverage of 80% reached. Total coverage: 88.78%
======================== 396 passed, 2 xpassed in 8.97s ========================
🔒 gitleaks 详细[]📝 pyright 输出自动提交状态: 无变更可提交 由 CI 自动生成 |
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
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
McpClientwith cached tool discovery and safe failure handlingMcpManagerwith per-server failure isolationToolManageraccept MCP tool cataloguesChanges
New Features
Security
Reliability
Cleanup
experiments/mcp_agent.py,experiments/mcp_multi_thread.py,experiments/multiagent.py)Test Plan