fix(mcp): lazy MCP servers show as lazy (not failed) in status, banner and startup discovery (#111717, slim redo of #100648) - #112324
Conversation
… failed A `lazy: true` MCP server registers its tools from the schema cache and spawns on first use. Three consumers still equated "alive" with a live session, so a healthy all-lazy startup was reported as a total failure: - `get_mcp_status()` fell through to `status: configured, tools: 0` for a lazily registered server. It now reports `lazy` with the cached tool count (`connected: False`); an in-flight or failed first-use connect still outranks it because the error is the actionable part. - `discover_mcp_tools()`'s summary counted every name absent from `_servers` as failed, logging `MCP: 0 tool(s) from 0 server(s) (2 failed)` right after registering every cached tool, and re-announced the same "failure" on every repeat discovery. Lazy servers are now reported as `(N lazy, not spawned yet)` and an already-lazy server is not re-announced. - `hermes_cli/mcp_startup.py` judged a discovery run by `connected` at two sites, so every startup logged `Background MCP discovery completed with zero connected servers` and every later call re-spawned the discovery thread as a retry. One predicate, `_discovery_registered_servers`, treats a lazy registration as a usable outcome at both sites. - `hermes_cli/banner.py` rendered the unknown `lazy` status through the red "could not connect" line; it now shows the cached tool count with `(lazy, starts on first use)`. Ported from #100648 (core hunks only; the toolsets-filter predicate branch, the Ink TUI component extraction and 13 tests were not ported). Fixes #111717
Follow-up to the ported status fix: - `tui_gateway/contracts/tools_mcp_plugins.py::McpRuntimeStatus` is a closed wire enum; `mcp.servers.status` would raise `ContractViolation` on the new `lazy` value. Declare it and regenerate the TS/OpenRPC contract files. - `ui-tui` session panel: an unknown status fell through to the red `failed` branch; render `lazy` with its cached tool count (inline branch, no component extraction). - Two invariant tests, both red on origin/main: the real discovery path yields `status: lazy` with the cached tool count and a summary without `failed` (eager control stays `configured`, live control stays `connected`); a lazy-only run neither warns nor re-arms the startup retry, while a configured-only run still does. - Document the per-server `lazy` key (undocumented until now) in `cli-config.yaml.example`, the MCP config reference and the MCP guide.
૮ >ﻌ< ა ci reviewran on a915dbe — fix: drop the dead disabled guard on the lazy MCP banner lin debug infoCI timingsCI timings · View report · View jobWall time 6m22s vs 6m1s (+5.8%). 9 job(s) slower, 3 faster, 2 unchanged.
|
get_mcp_status reports status='disabled' (never 'lazy') for a disabled
server and derives the 'disabled' flag from that same status, so the
extra 'and not srv.get("disabled")' check could never change the branch.
kvnloo
left a comment
There was a problem hiding this comment.
A status that cannot say "waiting" will call every waiter broken. The code fact: the status function, the discovery summary, and both startup checks all equated a working server with a live session or a name in the connected-servers map, and a lazily registered server has neither until its first tool call. The failure case in user terms: you set every MCP server to lazy to save memory, and every startup greets you with a red banner line, a "(2 failed)" discovery summary, and a "zero connected servers" warning for a setup that works perfectly. The feature's own happy path looked like an outage.
Verdict: approve (as COMMENT review — token cannot formally approve)
Verified against the PR: read the full diff (13 files) and the PR body, plus _resolve_server_key, _key_visible_in_scope, _server_visible_in_scope, _ensure_lazy_server_connected, _note_connect_failure, _register_lazy_from_cache, _select_new_servers, the mcp.servers.status RPC in tui_gateway/methods_tools.py, and the banner's single get_mcp_status() call site. Ran tests/tools/test_mcp_lazy_start.py and tests/hermes_cli/test_mcp_startup.py on the PR branch: 35 passed. Red-on-base confirmed: on origin/main exactly the 2 new tests fail (TestLazyMcpStatus and the lazy param of the startup test); the configured control passes. Zero existing reviews, so both findings are original. Both are non-blocking.
-
tools/mcp_tool_discovery.py,discover_mcp_tools— the already-lazy summary dedup keys purely on_lazy_server_configsmembership and never checks_server_connect_errors. A lazy server that failed on first use keeps its lazy config (the cooldown retry, pinned bytest_lazy_connect_failure_records_cooldown), so the next discovery classifies it "already lazy", drops it from the summary names, and_log_summary's all-zero gate stays silent. Butget_mcp_statusranksfailedabovelazyfor that same server. Net: status says failed, the summary says nothing, and on base the repeat discovery re-announced "(1 failed)". Non-blocking: exclude keys carrying connect errors from the lazy suppression so they keep counting as failed. -
website/docs/reference/mcp-config-reference.md,lazyrow — "Status surfaces show the server aslazy" carries no scoping caveat, butget_mcp_statusgates the entire lazy branch oninclude_runtime, andtui_gateway/methods_tools.pypassesinclude_runtime=Falsefor non-launch profiles without a multiplexer. There,mcp.servers.statusstill reports "configured" for lazy servers. Non-blocking: one clause on the visibility rule would make the doc exact.
What's good
- The wire contract was not forgotten:
McpRuntimeStatusgainslazyand the TS union plus OpenRPC are regenerated. Without it the new value would be aContractViolationonmcp.servers.statusunder test isolation. - A failed first-use connect outranks
lazyin the status precedence, and_ensure_lazy_server_connectedpops the lazy state on success, so "lazy" can never mask a broken server in status, only in the summary (finding 1). - The two summary sites (
_register_mcp_serversanddiscover_mcp_tools) now agree: both split lazy servers out before counting failures, which is exactly the "(2 failed)" misreport being fixed. - The salvage keeps @jonpol01's four core hunks under his authorship, with the dropped hunks honestly documented.
- The new tests go through the real
discover_mcp_tools()path with real registry state instead of mocking the status contract, and the startup test pins both call sites with a configured control.
Design thread: the "usable" predicate now lives in two places that must agree without anything enforcing it. get_mcp_status decides the status string through its precedence chain, and hermes_cli/mcp_startup.py::_discovery_registered_servers re-derives usability from that string (connected or status == "lazy"). If a future state ever counts as usable — a degraded connection, a needs-auth server with cached tools — both sites must change in lockstep or the startup warning and the status will disagree the way they did before this PR. Would a single is_usable predicate owned by the discovery module be worth it, so the startup check reads the policy instead of re-parsing its output?
Lazily registered MCP servers (
mcp_servers.<name>.lazy: true) now show up as lazy with their cached tool count in the banner, the TUI andmcp.servers.status— instead ofconfigured/failed, a(2 failed)discovery summary, a red banner line, azero connected serverswarning and a discovery retry on every startup.Slim redo of #100648 by @jonpol01 (the issue author): his four core hunks are ported nearly verbatim under his authorship; tests, wire contract, docs and the TUI branch are the follow-up commit.
Changes
tools/mcp_tool_discovery.py::get_mcp_status— newlazybranch (precedence connected → disabled → connecting → failed → lazy → configured) reporting the cached tool count withconnected: False; lazy state is scoped with_key_visible_in_scope(registration-level, since a lazy server never populates the adoption maps).tools/mcp_tool_discovery.py::discover_mcp_tools— the summary no longer counts lazy servers as failed:MCP: 4 tool(s) from 2 server(s) (2 lazy, not spawned yet); a repeat discovery does not re-announce already-lazy servers.hermes_cli/mcp_startup.py::_discovery_registered_servers— one predicate (connectedORstatus == "lazy") used at both sites (re-entry retry decision, post-discovery warning).hermes_cli/banner.py::_mcp_server_line— rendersalpha (stdio) — 3 tool(s) (lazy, starts on first use)instead of falling through to the red "could not connect" line.tui_gateway/contracts/tools_mcp_plugins.py::McpRuntimeStatusgainslazy(closed wire enum: the new value would otherwise be aContractViolationonmcp.servers.status); generated TS/OpenRPC regenerated.ui-tui/src/components/branding.tsx+types.ts— the Ink session panel renderslazywith its tool count instead of the redfailedfallback.lazykey was undocumented — added tocli-config.yaml.example,website/docs/reference/mcp-config-reference.mdand a "Lazy start" section inwebsite/docs/user-guide/features/mcp.md.origin/main):tests/tools/test_mcp_lazy_start.py::TestLazyMcpStatus(realdiscover_mcp_tools()path →lazy/3 tools/nofailed; eager control staysconfigured, live control staysconnected) andtests/hermes_cli/test_mcp_startup.py::test_lazy_only_discovery_counts_as_usable_at_both_startup_sites(lazy-only run: no warning, no retry; configured-only run: still warns and retries).Validation
Live probe (temp
HERMES_HOME, twolazy: truestdio servers with a pre-seeded schema cache, realdiscover_mcp_tools()→get_mcp_status()→_any_mcp_connected(); no process spawned):MCP: 0 tool(s) from 0 server(s) (2 failed)MCP: 4 tool(s) from 2 server(s) (2 lazy, not spawned yet)get_mcp_status()alphastatus: configured, tools: 0status: lazy, tools: 3, connected: False_any_mcp_connected()False(→ zero-connected warning + retry)Truealpha (stdio) — configuredalpha (stdio) — 3 tool(s) (lazy, starts on first use)discover_mcp_tools()(2 failed)againgammadisabled /deltaconfigured /liveconnected 2Root cause: lazy registration (#56832) was added without a current-state predicate — status, the startup checks and the summary all equated "alive" with a live session or
name in _servers, neither of which a lazy server has until first use.Fixes #111717
Salvages #100648 (@jonpol01)
Dropped hunks
--toolsetsfilter branch of_discovery_registered_servers(commit42c418a: "a filter that excludes every server is not a failure") — a separate mechanism from lazy registration, not part of [Bug]: lazy MCP servers are reported as "configured"/failed — red banner, "zero connected servers" warning and a discovery retry on every startup while their tools are registered and working #111717; left for its own PR.ui-tuiMcpServerLinecomponent extraction +ui-tui/src/__tests__/mcpLazyStatus.test.ts(121 lines) — thelazybranch is added inline instead.TestLazyMcpStatus×5,TestLazyDiscoverySummary×2, predicate/retry/warning ×5, banner ×1) — replaced by two invariant tests that cover the same behaviour through the real discovery path and both startup sites.vs #100648
+595/−29 over 10 files → this PR ports the ~60 core source lines verbatim (author-preserved), adds the missing wire-contract enum value (his branch would raise
ContractViolationunderHERMES_TEST_ISOLATION=1formcp.servers.status), and trims tests from 13 to 2.Infographic
Review follow-up
hermes_cli/banner.py::_mcp_server_line: dropped the deadand not srv.get("disabled")guard on the lazy branch —get_mcp_statusreportsstatus="disabled"(never"lazy") for a disabled server and derives thedisabledflag from that same status, so the guard could never change the branch.