fix(mcp): verify hook-free multi-host support - #341
Conversation
cdeust
left a comment
There was a problem hiding this comment.
Review — verified against the installed dependency, not the diff alone
CI is green at the time of writing (18 checks passed, Test (Python 3.12) still running).
What holds up
The root cause is correctly identified and fixed at its source. In the installed fastmcp==3.4.5, log_server_banner (utilities/cli.py:201) calls check_for_newer_version() as its first statement. The only other call sites are in fastmcp/cli/cli.py, which is not on this server's path. Suppressing the banner therefore does remove the sole network probe from the startup path — this is a fix at the cause, not at the throw site (coding-standards.md §6.1).
The signature is sound. show_banner is keyword-only and typed bool | None, so partial(run_stdio_drained, mcp, show_banner=False) yields the zero-argument callable anyio.run expects.
The new contract test is severe rather than accommodating. _environment() reproduces the actual failure (ALL_PROXY=socks5://127.0.0.1:9 with no socks extra) and — the detail that matters — pops FASTMCP_SHOW_SERVER_BANNER and FASTMCP_CHECK_FOR_UPDATES from the child environment. The test cannot pass by accident through ambient configuration. That is properly constructed falsifiability, and it is the strongest part of this PR.
The rename is clean. agents/ → claude-agents/ is detected as a 100% rename, plugin.json declares the new path, the wiki_groomer.py docstring follows, and no reference to the old path survives anywhere in the tree.
The new CI job is hardened: pinned action SHAs, persist-credentials: false, permissions: contents: read, a pinned lockfile, and a verified justification for permitting install scripts (--ignore-scripts was confirmed to leave claude plugin validate unusable). The rationale is documented at the right place.
Findings
1 — The fix is one level broader than its own stated diagnosis. (main finding)
The inline comment argues: "Version discovery belongs in an explicit maintenance/doctor path, never in the protocol handshake." That is an argument for disabling the version check, not the banner. fastmcp exposes exactly that lever: settings.check_for_updates (settings.py:354, FASTMCP_CHECK_FOR_UPDATES).
The concrete consequence: _resolve_show_banner exists precisely to honour fastmcp.settings.show_server_banner, carries a sourced citation, and was re-verified against the installed package on 2026-07-30. Its only production caller now passes a hard-coded False. That resolution branch becomes unreachable in production — it is kept alive only by its tests (tests_py/infrastructure/test_stdio_transport_wiring.py:166,182). Sourced, tested, no real caller: coding-standards.md §9.
The side effect is also silent: FASTMCP_SHOW_SERVER_BANNER=1 now does nothing, on every host, including hosts where the banner never caused a problem. The banner also carried server name/version diagnostics on stderr; that is gone for everyone.
Both resolutions are defensible, but the choice should be explicit:
- target
check_for_updatesand leave banner resolution intact; or - accept "banner always off" and say so — in which case
_resolve_show_bannershould be removed or marked vestigial, not left in place implying a behaviour it no longer has.
2 — The lean profile has no protocol coverage.
MIN_TOOL_COUNT = 52 is a floor, and this PR's own evidence reports 54 tools. The assertion passes at 52, 54, or 200 — it guards against catastrophic loss, not against drift. More importantly, verify_mcp_hosts.py exercises only the full profile (no occurrence of lean or profile in the script), yet tool_profiles.py is modified here and LEAN_INSTRUCTIONS is part of the host-neutral contract this PR establishes. A lean server exposing ~10 tools would fail the floor outright. In a PR whose subject is the multi-host contract, that is the most notable gap.
3 — Supply-chain surface on fork PRs.
ci.yml triggers on pull_request, and the new job runs npm ci with install scripts enabled. A fork PR that edits package-lock.json executes arbitrary code on the runner. The mitigations are real (read-only token, no secrets on fork PRs, no persisted credentials), so the blast radius is compute abuse and cache poisoning rather than credential theft. But it is a surface introduced by this PR. Cheap hardening: gate the job on github.event.pull_request.head.repo.fork == false, or assert the lockfile is unmodified before npm ci.
4 — Two minor points in verify_mcp_hosts.py.
resources != []freezes "no MCP resources" as a permanent contract. If that is intended, it deserves a comment; the day a resource is legitimately exposed, this fails with a message that does not explain why._responses()silently skips unparseable lines. A malformed frame surfaces asmissing response for request id=N, pointing the diagnosis at the wrong place.
5 — A product-policy change ships inside a comment.
pyproject.toml moves PyPI from "a deprecated, best-effort secondary channel" to "the best-effort, hook-free compatibility channel for Gemini CLI, Codex CLI, and other hosts". That is consistent with the PR's intent, but it changes a support commitment. It belongs in CHANGELOG.md, not only in a source comment.
Verdict
Mergeable on substance: the fix is correct, minimal, placed at the cause, and proven by a real reproduction rather than a mock. Finding 1 warrants an explicit decision before publishing — not because it is wrong, but because the PR renders unreachable a path it documents as correct, without saying so. Findings 2 and 3 are legitimate follow-ups. Findings 4 and 5 are cosmetic.
|
Merci pour la revue. Les cinq points sont traités dans
Validation locale après correction : 49 tests ciblés, Ruff check/format, contrôles documentation/YAML, Pyright ciblé, et 6 cas stdio réels (Claude/Gemini/Codex × full/lean) passent. |
Summary
Make Cortex's local stdio server reliably usable without Claude lifecycle hooks across Claude Code, Gemini CLI, and Codex CLI. The server now skips FastMCP's banner-time network update probe so a SOCKS proxy without the optional HTTP SOCKS extra cannot abort startup before MCP
initialize. The change also isolates Claude-only agent metadata, aligns host manifests and documentation, and adds protocol plus pinned vendor-parser contracts in CI.Type of change
Test plan
cc32fc56.)Evidence run locally:
pytest tests_py/test_main.py tests_py/test_tool_profiles.py tests_py/infrastructure/test_stdio_transport_wiring.py tests_py/scripts/test_mcp_json_contract.py tests_py/scripts/test_cross_host_manifests.py tests_py/scripts/test_verify_mcp_hosts.py -q— 49 passed.ruff check .andruff format --check .— passed.scripts/check_doc_claims.py,scripts/generate_repo_badges.py --check, workflow YAML parsing, andgit diff --check— passed.scripts/verify_mcp_hosts.py -- hypermnesia-mcpunder a deliberately unusable SOCKS proxy — Claude, Gemini, and Codex client identities each completed initialize, discovery, and a real SQLitememory_statscall for both the full (54 tools locally) and exact lean (10 tools) profiles.Audit notes
cc32fc56: preserve the banner and its setting while disabling only FastMCP's update lookup; verify all three hosts against both profiles and the exact 10-tool lean set; gate the vendor-parser job on same-repository PRs; reject malformed stdout at the parse boundary and document the resource contract; state the policy change in the changelog.Coding-standards compliance
any(oras any, or untyped dicts at boundaries) in production code.Breaking changes
None.
Screenshots / logs
Representative smoke output:
Reviewer checklist