fix(tui): start MCP discovery for websocket sessions (#38945) - #54161
Conversation
🔎 Lint report:
|
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: LGTM (approve-ready, COMMENT due to token permissions)
Fix for #38945: starts background MCP discovery for websocket sessions. The desktop app and dashboard chat reach the agent through the WS sidecar, not through the stdio TUI path — without this, MCP tools are never discovered.
Changes
- handle_ws() now calls start_background_mcp_discovery() with a dedicated thread name
- 1 test verifying the call is made with correct kwargs
Looks Good
- Idempotent, config-gated — safe to call multiple times
- The comment explains the architectural reason clearly
- One concern: the test creates a real asyncio.run() call which could leak, but the try/finally cleanup handles it
Reviewed by Hermes Agent
The desktop app and dashboard chat reach the agent through the /api/ws JSON-RPC sidecar (tui_gateway.ws.handle_ws), NOT through tui_gateway.entry.main() — the stdio-TUI path that spawns the background MCP discovery thread. In the WS process discovery was therefore never started: _make_agent only *waits* (wait_for_mcp_discovery), which no-ops when the thread was never created, so the agent snapshotted an MCP-less tool list. The only discovery trigger reachable was a manual /reload-mcp, which is why tools appeared after a reload but vanished on restart. Start the shared, idempotent, config-gated background discovery in handle_ws right after accept() and before gateway.ready, so the first agent build picks up already-spawning servers (and the existing late-binding refresh handles slow ones). Fixes #38945.
55b92ad to
ff2dcb8
Compare
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary\n\nVerdict: LGTM\n\nStarts MCP discovery for websocket sessions. Without this, the desktop app and dashboard chat never run discovery, so the agent snapshots an MCP-less tool list. Clean implementation with idempotent start. Regression test for #38945.\n\n---\nReviewed by Hermes Agent
Profile-blind MCP discovery:
|
|
This fix is incomplete. The MCP discovery still reads from the default profile because the Desktop doesn't transmit the active profile - tracked in #71527. Even with this PR merged, per-profile MCP servers (e.g. bluesky_sheepyr) are not discovered in Desktop WebSocket sessions. The actual root cause is the profile routing gap in #71527. |
|
Thanks for the detailed report. This makes sense — the current MCP
discovery fix addresses WebSocket startup discovery, but the profile
routing issue appears to happen earlier when discovery reads config before
the target profile HERMES_HOME is applied.
I’m looking into this as a follow-up to the MCP discovery work. I’ll verify
whether passing the profile through discovery or deferring discovery into
the profile-scoped subprocess is the cleaner approach.
…On Sun, Jul 26, 2026 at 10:50 AM Karlesnine ***@***.***> wrote:
*karlesnine* left a comment (NousResearch/hermes-agent#54161)
<#54161 (comment)>
This fix is incomplete. The MCP discovery still reads from the default
profile because the Desktop doesn't transmit the active profile - tracked
in #71527 <#71527>.
Even with this PR merged, per-profile MCP servers (e.g. bluesky_sheepyr)
are not discovered in Desktop WebSocket sessions. The actual root cause is
the profile routing gap in #71527
<#71527>.
—
Reply to this email directly, view it on GitHub
<#54161?email_source=notifications&email_token=AXOQMVTYBUHOEZL7BW3I7735GYLDPA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMBYGM4TSMZXGI42M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5083993729>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AXOQMVSIEP7MXK7ARJGUIND5GYLDPAVCNFSNUABGKJSXA33TNF2G64TZHMYTAMRUGU2TIMRWG45US43TOVSTWNBXGYYTQMZSGM3TDILWAI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Hi, I updated the MCP discovery PR with the rebased profile-scoped version.
The conflicts are resolved, tests are passing, and the PR is ready for
review. Thanks again for your guidance.
On Sun, Jul 26, 2026 at 1:51 PM Antonio Vidal ***@***.***>
wrote:
… Thanks for the detailed report. This makes sense — the current MCP
discovery fix addresses WebSocket startup discovery, but the profile
routing issue appears to happen earlier when discovery reads config before
the target profile HERMES_HOME is applied.
I’m looking into this as a follow-up to the MCP discovery work. I’ll
verify whether passing the profile through discovery or deferring discovery
into the profile-scoped subprocess is the cleaner approach.
On Sun, Jul 26, 2026 at 10:50 AM Karlesnine ***@***.***>
wrote:
> *karlesnine* left a comment (NousResearch/hermes-agent#54161)
> <#54161 (comment)>
>
> This fix is incomplete. The MCP discovery still reads from the default
> profile because the Desktop doesn't transmit the active profile - tracked
> in #71527 <#71527>.
> Even with this PR merged, per-profile MCP servers (e.g. bluesky_sheepyr)
> are not discovered in Desktop WebSocket sessions. The actual root cause is
> the profile routing gap in #71527
> <#71527>.
>
> —
> Reply to this email directly, view it on GitHub
> <#54161?email_source=notifications&email_token=AXOQMVTYBUHOEZL7BW3I7735GYLDPA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMBYGM4TSMZXGI42M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5083993729>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AXOQMVSIEP7MXK7ARJGUIND5GYLDPAVCNFSNUABGKJSXA33TNF2G64TZHMYTAMRUGU2TIMRWG45US43TOVSTWNBXGYYTQMZSGM3TDILWAI>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
|
|
@LionGateOS thanks for the follow-up and for pushing the profile-scoped version forward. Two notes so we don't diverge:
If your profile-scoped fix (or #64266) doesn't touch that singleton, the per-profile MCP discovery will still be blind in Desktop websocket sessions. Worth aligning the two threads. |
|
Thanks for the careful follow-up. To clarify: #72135 was closed because the MCP discovery work was carried forward through #72219, per Teknium's note that the commits were cherry-picked onto current main with authorship preserved. #72295 is separate. It only covers the live-session lookup/reuse path by scoping with . It does not pick up the MCP discovery-side fix, and it does not address the singleton behavior. I agree those should not be conflated. I'll treat the remaining per-profile MCP discovery singleton issue as a separate follow-up area and first check whether #72219, #64266, or another existing branch already covers it before proposing anything new. |
|
Small correction to my previous comment: the stripped code identifiers were Restated: #72295 only covers the live-session lookup/reuse path by scoping |
Summary
MCP tools now appear in Desktop/dashboard chat sessions on connect, matching the CLI — no manual
/reload-mcp, no loss on restart.Root cause: The desktop app and dashboard chat reach the agent through the
/api/wsJSON-RPC sidecar (tui_gateway.ws.handle_ws), not throughtui_gateway.entry.main()— the stdio-TUI path that spawns the background MCP discovery thread. In the WS process discovery was therefore never started:_make_agentonly waits (wait_for_mcp_discovery), which no-ops when the thread was never created, so the agent snapshotted an MCP-less tool list. The only discovery trigger reachable there was a manual/reload-mcp, which is why tools showed up after a reload but vanished on every restart.Changes
tui_gateway/ws.py: start the shared, idempotent, config-gated background MCP discovery inhandle_ws()right afteraccept()and beforegateway.ready, so the first agent build picks up already-spawning servers (the existing late-binding refresh handles slow ones).tests/test_tui_gateway_ws.py: regression test assertinghandle_wsinvokesstart_background_mcp_discovery(thread_name="tui-ws-mcp-discovery").Validation
tests/test_tui_gateway_ws.py4/4 passhandle_ws+ realmcp_startup, MCP server in config)discover_mcp_toolsnever calleddiscover_mcp_toolscalledSalvaged from #38301 (@sweetcornna, earliest of the cluster). Only the
ws.pyhunk remained relevant — the rest of #38301 (migrating the thread slotentry.py→mcp_startup.py) already landed onmain. Duplicate #42794 (@LionGateOS) fixed the same site.Fixes #38945.
Infographic