Conversation
alt-glitch
added
type/bug
Adds a distinct
|
a30e594 to
7c82b6e
Compare
7c82b6e to
42ea452
Compare
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 42ea452589d93b0afb8adb2938c5f5f2b8c5dd73 against current main 14ca27fa0601144b6ea4af1408a3b37c22456072 (merge base 00140a85574d4fc9e42f977a167aeda899b50ca9). The PR is one commit ahead and two commits behind; the two base-only commits touch only the Email gateway/tests, so I do not see an MCP merge collision. Exact-head CI, Docker, and Nix are all green; because this PR has one surviving commit, the commit train is every-commit green. Note, however, that JS & TS checks were skipped on this head because the diff contains no TS files, which matters for blocker 1 below.
I traced the current extracted MCP owners (mcp_tool_discovery, registration/cache state, startup coordinator), the banner and TUI session-info producer, the current TypeScript wire type, the lazy/cache tests, the recent -t/--toolsets discovery filtering, and the related/superseded work. The core state model here is good: connected -> disabled -> connecting -> failed -> lazy -> configured is the right precedence, gating lazy on the surviving lazy-config owner avoids stale tool-name residue, the banner correctly preserves the cached tool count, and the discovery summary no longer re-announces an already-lazy server.
There are two landing blockers.
1. lazy is now a Python→TUI wire value, but the consumer contract still says it cannot exist. tui_gateway/server.py forwards get_mcp_status() directly as info["mcp_servers"], while exact head still declares McpServerStatus.status in ui-tui/src/types.ts as only 'configured' | 'connecting' | 'connected' | 'disabled' | 'failed'. This PR therefore changes the runtime protocol without changing its typed consumer. The green Python matrix does not cover that boundary—the JS/TS job was skipped. Please add 'lazy' to the TUI type and pin the wire/render contract with a small regression (a lazy server should remain connected: false, carry its cached tool count, and be representable by SessionInfo). This is especially important because the PR explicitly claims the dashboard/TUI mcp_servers surface as part of the fix.
2. The new startup-success predicate still mistakes an intentionally filtered discovery for a failed discovery. Current main gained the -t/--toolsets MCP spawn filter from #19000 after this PR was originally opened. On this exact head, set_mcp_server_filter("terminal") causes _discover_mcp_tools_without_interactive_oauth() to call discover_mcp_tools(allowed_mcp_names=["terminal"]); when configured MCP server names do not match, discovery correctly returns before even importing the SDK. But the post-run/re-entry checks then call unfiltered get_mcp_status(), see those untouched servers as configured, _discovery_registered_servers() returns false, the zero-connected warning fires, and a later start_background_mcp_discovery() retries the thread. That is the same defect class this PR is fixing—intentional non-connection being interpreted as discovery failure—just on the other side of the newly landed filter boundary. Please make the success/retry decision aware of the requested discovery set (or otherwise represent intentional skip distinctly), and add a regression for configured MCP + -t terminal: zero MCP spawn, no zero-connected warning, and no later retry. An all-disabled config is the same state-shape; either cover it too or deliberately separate “nothing usable was requested” from “requested discovery failed.”
Interlocks / provenance
- The lazy-start design comes from #56832; main subsequently integrated the cache/startup work in
135a2945...and1d5ecad5...with the original design credit preserved. This PR is a complementary correctness repair, not a duplicate or supersession. - The retry-after-zero path comes from #66981 and was salvaged into main. This PR is correctly tightening that retry contract rather than replacing that work.
- The
-tfilter came from #19000 and was re-derived onto current main ine73257c...,87597d3..., andaf019a3.... That newer adjacent implementation is what exposes blocker 2; credit and ownership stay distinct. - #80948 is complementary diagnostics work on the same zero-connected branch, but it still patches a connected-only guard. Merge order needs an explicit semantic rebase: if #100648 lands first, #80948 must retain
_discovery_registered_servers(...)while adding its formatter; if #80948 lands first, this PR must preserve that formatter while replacing the predicate. “Either order” is only safe if the resulting predicate is checked, not merely if Git reports a clean textual merge. - #99923 is broader profile-scoped discovery ownership. It already introduces an availability-style predicate specifically because a cache-backed lazy registration is usable without a live session, so it is adjacent/complementary rather than duplicate. #99925 is downstream test coverage and currently states/asserts that a lazy cache hit reports
configured; after this PR that expectation must becomelazywhen that stack is rebased. - The recent MCP decomposition is respected here: the implementation now lives in the extracted
tools/mcp_tool_discovery.pyowner rather than growing the oldmcp_tool.pyfacade. That part is aligned with the current architecture.
The underlying repair is strong work. Close these two boundary seams and the state model is substantially more honest end-to-end. 🚀
| # merely waiting to be spawned. | ||
| status = ("connected" if live else "disabled" if not enabled else "connecting" if name in connecting | ||
| else "failed" if name in connect_errors else "configured") | ||
| else "failed" if name in connect_errors else "lazy" if name in lazy_tool_names |
There was a problem hiding this comment.
This new lazy value crosses the TUI session-info wire unchanged (tui_gateway/server.py assigns get_mcp_status() directly to info["mcp_servers"]), but exact head still types McpServerStatus.status in ui-tui/src/types.ts as only configured | connecting | connected | disabled | failed. Please update that consumer contract and pin a lazy payload. The exact-head JS/TS job is skipped here, so green CI cannot catch this drift.
There was a problem hiding this comment.
Fixed in 3eca919852.
ui-tui/src/types.ts now types McpServerStatus.status as 'configured' | 'connecting' | 'connected' | 'disabled' | 'failed' | 'lazy', and the TUI renders a lazy server with its cached tool count instead of falling through to the red failed branch. A lazy payload is pinned by ui-tui/src/__tests__/mcpLazyStatus.test.ts, so the contract is asserted on the JS side rather than relying on a job that is skipped here.
Thank you for flagging that green CI could not catch this drift — it was the reason the consumer went unnoticed.
| for entry in status or []: | ||
| if not isinstance(entry, dict): | ||
| continue | ||
| if entry.get("connected") or entry.get("status") == "lazy": |
There was a problem hiding this comment.
This fixes the all-lazy case, but current main's -t/--toolsets spawn filter creates another intentional non-connection state. With configured MCP servers and -t terminal, discovery intentionally filters every MCP out and returns before SDK import; get_mcp_status() still reports the full config as configured, so this predicate is false, the run logs zero-connected, and the next startup call retries. Please scope this decision to the requested discovery set (or encode intentional skip) and add the no-spawn/no-warning/no-retry regression.
There was a problem hiding this comment.
Confirmed and fixed in 42c418ad52. This was a real second case and I had not covered it.
With -t terminal and MCP servers configured, discovery filters everything out and returns before the SDK import, while get_mcp_status() still reports the full config as configured — so the predicate was false, the run logged "zero connected servers", and the next call re-entered discovery. Every call, on every such oneshot.
The decision is now scoped to the requested set: when a filter is active and none of the configured server names appear in it, the run did what was asked and is not a failure. A filter that names a configured server which still did not come up is deliberately unchanged — that keeps its warning and its retry.
Three regressions, all red without the scoping:
- a filtered-out run is not a failure, while a no-filter run with the same status still is;
- a filter naming a configured server that did not come up still reports failure;
- end to end through
_any_mcp_connected(), a filtered-out run neither warns nor re-enters.
test_mcp_startup.py, test_mcp_lazy_start.py and test_banner.py together: 54 passed.
42ea452 to
baca936
Compare
|
Thanks — this was a careful read, and blocker 1 was worse than you described. Rebased onto Blocker 1 — accepted, and the render side was the real damageYou framed this as type drift. It is, but the consumer was already mis-rendering: the status chain in ) : (
<Text color={t.color.error}>failed</Text>
)so any status the chain doesn't name falls into it. With Fixed in
Two notes on how that is pinned, so the guarantee isn't overstated:
Blocker 2 — mechanism confirmed, but it is main's, not this PR'sI traced it exactly as you described and it holds: What I can't agree with is placing it on this PR. I ran the same four hops against a clean
I agree it is the same defect class and that it should be fixed. I'd rather not fix it here: the decision needs to be aware of the requested discovery set, which means either threading the filter into the success check or giving "nothing usable was requested" its own representation — and that is a second policy on a seam #99923 is already reworking (it introduces an availability-style predicate for precisely this reason). Landing a third shape in between is how these end up contradicting each other. Happy to open it as its own PR against What the rebase itself turned upWorth flagging, since it's the "clean textual merge isn't a checked merge" hazard from your own note, and it bit this PR:
The related trap: Both are pinned by Interlocks
Tests on the rebased head: |
… failed Lazy startup (NousResearch#56832) registers a server's tools from the schema cache and spawns the process on first use. Three consumers still decided whether a server was alive from `connected` / `name in _servers` alone, so a healthy all-lazy startup — the memory-saving configuration — was reported as a total failure: - get_mcp_status() had no branch for a lazily registered server (no session, not connecting, no error), so it fell through to `status: "configured", tools: 0`, the same shape as a server that never started, while its cached tools were registered and callable. The banner, `hermes mcp status` and the dashboard all showed that. - hermes_cli/mcp_startup.py judged a discovery run by `any(entry.get("connected"))` at both sites, so every startup logged "Background MCP discovery completed with zero connected servers" and every later start_background_mcp_discovery() call treated the finished run as one that connected nothing (NousResearch#66981's retry) and re-spawned the discovery thread. One install had 2,787 of those warnings and 20 retries in errors.log for two healthy lazy servers. - discover_mcp_tools()'s summary counted `name in _servers` as connected and everything else as failed, logging "MCP: 0 tool(s) from 0 server(s) (2 failed)" right after registering every cached tool. get_mcp_status() now reports such a server as `status: "lazy"` with its cached tool count; connected, disabled, connecting (first-use spawn in flight) and failed (first-use spawn failed) keep precedence over it. The startup predicate counts a lazy registration as a run that left servers usable, at both sites; a run that registered nothing still warns and still retries. The discovery summary counts newly lazy servers as servers with tools and no longer re-announces or fails a server that was already lazy before the call. The banner renders `lazy` with its tool count instead of falling into the red "failed" branch that catches unknown statuses, and cli-config.yaml.example documents the `lazy` key, which was missing from it. Verified against a live install with two lazy stdio servers and a warm cache: 21 tools registered, no child process spawned, both servers reported as lazy, and the summary reads "MCP: 21 tool(s) from 2 server(s) (2 lazy, not spawned yet)".
`get_mcp_status()` gained a `lazy` status and `tui_gateway/server.py` forwards that list verbatim as `info["mcp_servers"]`, but `McpServerStatus.status` in ui-tui still typed only the five older values, so the runtime protocol and its typed consumer had drifted. The JS/TS CI job is skipped on a Python-only diff, so green CI could not catch it. The render side was the user-visible half: the status chain in branding.tsx ends in an unconditional red "failed", so a healthy lazy server — tools registered from the schema cache, process not spawned yet — rendered as *failed* in the TUI. That is the same misreport this PR fixes in the classic CLI banner, on the second surface. Adds 'lazy' to the union, gives the row its own branch showing the cached tool count, and pins the contract: the row renders that count rather than "failed", a lazy server stays `connected: false` inside `SessionInfo`, and it is not counted in the connected "N MCP" headline (parity with banner.py's sum(s["connected"])). The row moves into an exported `McpServerLine` because the MCP accordion is collapsed by default, so `SessionPanel` never renders it.
baca936 to
3eca919
Compare
…led discovery run `-t/--toolsets` deliberately narrows which MCP servers are spawned, and when it excludes all of them discovery returns before importing the SDK and starts nothing. `get_mcp_status()` still reports the full config as `configured`, so the registered-servers predicate read that intentional skip as a run that achieved nothing: `hermes -t terminal` on a machine with MCP servers logged "Background MCP discovery completed with zero connected servers" and re-entered discovery on the next call, every call. Scope the decision to the requested set: when a filter is active and none of the configured server names are in it, the run did exactly what was asked. A filter that names a configured server which still did not come up is unchanged — that is a real failure and keeps its warning and its retry. Thanks to @andrexibiza for finding this; it is the second intentional non-connection state on this path, after the all-lazy case the PR started from. Tests: the filtered-out run is not a failure while a no-filter run with the same status still is; a filter naming a configured server that did not come up still reports failure; and end to end through `_any_mcp_connected`, a filtered-out run neither warns nor re-enters. All three fail with the scoping reverted.
…ed row by its wire entry; docs Trim twenty tests to four, each red on the pre-fix code: - lazy-start: one test walks a lazy registration through every surface (discovery summary on a first and a repeat run, get_mcp_status, the banner line), with the connecting / failed / stale-config controls folded in; the owner-scope test stays as the second. - startup: one table-driven test. An all-lazy run and a -t filter that excluded every server are successful runs (no zero-connected warning, no retry); nothing-up and a named server that did not come up still are not. - TUI: one test renders a lazy row with its cached count, never "failed", and keeps it out of the connected-MCP headline. Dropped: the per-branch unit pins of _discovery_registered_servers (now one table), a SessionInfo shape test that passes on the pre-fix code, and the whole-banner render (folded into the banner-line assertion). Fix: McpServerLine took the TUI's local McpServerStatus, which failed tsc at the SessionPanel call site; it now takes the entry SessionInfo actually carries (the generated wire model). Docs: the lazy key in the MCP config reference, including how the server shows until first use. Comments cut to the why.
… 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
|
Thanks @jonpol01 — this landed on Closing in favour of the landed change. If anything from this PR is still missing on main, say so here and we'll pick it up. |
5 participants
What does this PR do?
Lazy MCP startup (#56832,
mcp_servers.<name>.lazy: true) registers a server's tools from the schema cache and spawns the process on first use. Three consumers still decided "is this server alive?" byconnected/name in _serversalone, so a healthy all-lazy startup — the memory-saving configuration — was reported as a total failure:get_mcp_status()had no branch for a lazily registered server: no session, not connecting, no error → it fell through tostatus: "configured", tools: 0— the same shape as a server that never started — while its cached tools were registered and callable.hermes mcp status, the banner, and the dashboard'smcp_serversinfo all showed that.hermes_cli/mcp_startup.pyjudges a discovery run byany(entry.get("connected")), twice. With every server lazy, every startup logsBackground MCP discovery completed with zero connected servers, and every later call tostart_background_mcp_discovery()treats the previous run as one that connected nothing (the fix(mcp): allow background discovery retry after a run that connected nothing #66981 retry, meant for startup cancellation / OOM) and re-spawns the discovery thread.discover_mcp_tools()'s summary countedname in _serversas connected and everything else as failed, so immediately after registering every cached tool it loggedMCP: 0 tool(s) from 0 server(s) (2 failed).Change
tools/mcp_tool.py—get_mcp_status()reports a lazily registered server asstatus: "lazy"with its cached tool count (connected: False). Precedence keeps the more current state on top: connected → disabled → connecting (first-use spawn in flight) → failed (first-use spawn failed; the error is the actionable part) → lazy → configured. The discovery summary counts newly lazy servers as servers with tools (… (N lazy, not spawned yet)) instead of failures, and a server that was already lazy before the call is neither re-announced nor counted as failed.hermes_cli/mcp_startup.py— one predicate,_discovery_registered_servers(status): a live session or a lazy registration means the run left servers usable. Used at both sites (re-entry retry decision, post-discovery warning). A run that really registered nothing still warns and still retries — tests pin both directions.hermes_cli/banner.py— renderslazywith its tool count ((lazy, starts on first use)) instead of falling into the redfailedbranch that catches every unknown status.cli-config.yaml.example— documents thelazyper-server key, which was not in the example at all.Out of scope, noted for honesty: an idle-parked server (
_was_parked, session dropped, tools deregistered) also reportsconfigured; it has no current-state predicate onMCPServerTask, and its tools really are gone, so I left it.Related Issue
Fixes #111717
Follows up #56832 (lazy startup) and #66981 (retry when a run connected nothing). #80948 (open) rewords the zero-connected warning message; this PR changes only the predicate feeding it and merges cleanly either way.
Type of Change
Changes Made
tools/mcp_tool_discovery.py—get_mcp_status()reportslazy(scoped by server key; a first-use connect in flight or failed outranks it);discover_mcp_tools()counts lazy servers apart from failures and announces them oncehermes_cli/mcp_startup.py—_discovery_registered_servers(): a run succeeded when a server connected, registered lazily, or-texcluded every server on purpose; used by the zero-connected warning and the fix(mcp): allow background discovery retry after a run that connected nothing #66981 retryhermes_cli/banner.py—lazyrendering with the cached tool countui-tui/src/components/branding.tsx,ui-tui/src/types.ts—McpServerLineextracted (typed as the wire entrySessionInfocarries) with alazybranch; the status union gainslazycli-config.yaml.example,website/docs/user-guide/features/mcp.md— thelazykey, and how the server shows until first usetests/tools/test_mcp_lazy_start.py(a lazy registration read as working on every surface; lazy state scoped to its owner),tests/hermes_cli/test_mcp_startup.py(one table: all-lazy / filtered-out succeed, nothing-up / named-server-missing still fail),ui-tui/src/__tests__/mcpLazyStatus.test.ts(lazy row and headline)How to Test
scripts/run_tests.sh tests/tools/test_mcp_lazy_start.py tests/hermes_cli/test_mcp_startup.pyandcd ui-tui && npx vitest run src/__tests__/mcpLazyStatus.test.tslazy: trueon every stdio server inmcp_servers, run once to warm the cache, restart. Before: banner shows the servers as— configured, the log saysMCP: 0 tool(s) from 0 server(s) (N failed)andBackground MCP discovery completed with zero connected servers. After:MCP: 21 tool(s) from 2 server(s) (2 lazy, not spawned yet), the banner lists each server with its cached tool count, and no zero-connected warning.Verified on a real install (two stdio servers, both lazy, warm cache) with this patch applied in a throwaway worktree — no child process was spawned:
Before the patch the same run printed
MCP: 0 tool(s) from 0 server(s) (2 failed),linear: status=configured tools=0,mem0: status=configured tools=0. The install'serrors.loghad accumulated 2,787 zero-connected warnings and 20retrying discovery threadlines for those two healthy servers.Each of the four tests fails against the pre-fix source and passes with the fix; the TUI test also fails with only the
lazybranch removed (it rendersplaywright [stdio]: failed). Merged onto currentmainlocally: no conflicts, 769 tests across the MCP, startup and banner files pass underscripts/run_tests.sh, andtscis clean.ruffclean.Checklist
Code
scripts/run_tests.sh(everytests/tools/test_mcp*.py,test_mcp_startup,test_banner) plusvitest,tscandeslinton the TUI files; relying on CI for the full matrixDocumentation & Housekeeping
lazykey added to the MCP config reference (website/docs/user-guide/features/mcp.md) andcli-config.yaml.examplecli-config.yaml.example— updated (documentation only; no new config keys)CONTRIBUTING.md/AGENTS.md— N/A