fix: keep one-shot MCP resources alive through tool calls - #66047
Conversation
…75-apply-audited-one-shot-mcp-lifetime-fix
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: LGTM / Comment
Fix ensures MCP resources discovered in one-shot mode stay alive through tool call execution, then shut down cleanly afterward. The wait_for_mcp_discovery() call at the top and shutdown_mcp_servers() in a finally block are correct. Includes a dedicated test. No issues found.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing the one-shot MCP startup race. The premise is present on current main: hermes_cli/main.py:12866-12879 starts bounded background discovery for chat/one-shot launches, while hermes_cli/oneshot.py:393 constructs AIAgent without the CLI wrapper's discovery wait. The proposed wait therefore closes a real tool-snapshot gap.
Problems
- The new
try/finallystarts only afterAIAgent(...)is constructed (hermes_cli/oneshot.py:426in the PR diff). If construction raises after discovery has opened MCP servers,shutdown_mcp_servers()is never reached.
Suggested changes
- Start the protected region before
AIAgent(...), so construction, callback setup, and the conversation all share the shutdown guarantee. Add a constructor-failure ordering test alongside the current happy-path test intests/hermes_cli/test_mcp_startup.py.
This is an automated hermes-sweeper review.
| @@ -422,8 +426,13 @@ def _run_agent( | |||
| agent.stream_delta_callback = None | |||
There was a problem hiding this comment.
This try begins after AIAgent(...) construction. Please begin the protected region before construction so an initialization failure after MCP discovery still calls shutdown_mcp_servers().
SummaryTwenty-three PRs address or reference the MCP discovery/tool-snapshot complex through timeout changes, pre-build readiness barriers, late refresh, worker-specific waits, or adjacent lifecycle work. Merged #49208 provides the multi-turn refresh foundation and merged #75933 implements the non-interactive discovery barrier, while target #66047 adds a distinct one-shot MCP resource-lifetime change. Related pull requests
Duplicates#41630 and #43685 are the same three-second CLI/TUI timeout bump; #68168, #69174, and #71277 duplicate #68162's one-shot pre-build wait, and that chain is superseded by merged #75933; #38456, #38620, #51322, #60017, and the readiness portion of #61944 overlap #75933, while #43348 and #54695 share the Kanban-specific long-wait approach. Suggested consolidationKeep #66047 open with a salvage path: retain only its distinct MCP resource-lifetime guarantee, move the Complex graphflowchart TD
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I38448(["issue #38448 (closed)"])
I51316(["issue #51316 (closed)"])
I68137(["issue #68137 (closed)"])
subgraph Dup36882 ["PRs duplicating each other"]
P36882["PR #36882 (open)"]
P38456["PR #38456 (open)"]
P38620["PR #38620 (open)"]
P51322["PR #51322 (open)"]
P60017["PR #60017 (open)"]
P61944["PR #61944 (open)"]
P66047["PR #66047 (open)"]
P68162["PR #68162 (closed)"]
P68168["PR #68168 (closed)"]
P69174["PR #69174 (open)"]
P71277["PR #71277 (closed)"]
P75933["PR #75933 (merged)"]
end
P66047 -.->|partial| I38448
P66047 -.->|partial| I51316
P66047 -->|fixes| I68137
class I38448 closed
class I51316 closed
class I68137 closed
class P36882 open
class P38456 open
class P38620 open
class P51322 open
class P60017 open
class P61944 open
class P66047 open
class P68162 closed
class P68168 closed
class P69174 open
class P71277 closed
class P75933 merged
class P36882 best
class P51322 best
class P51322 best
class P51322 best
class P61944 best
class P61944 best
class P68162 best
class P75933 best
class P75933 best
class P75933 best
class P75933 best
class P75933 best
class P66047 target
click I38448 "https://github.com/NousResearch/hermes-agent/issues/38448"
click I51316 "https://github.com/NousResearch/hermes-agent/issues/51316"
click I68137 "https://github.com/NousResearch/hermes-agent/issues/68137"
click P36882 "https://github.com/NousResearch/hermes-agent/pull/36882"
click P38456 "https://github.com/NousResearch/hermes-agent/pull/38456"
click P38620 "https://github.com/NousResearch/hermes-agent/pull/38620"
click P51322 "https://github.com/NousResearch/hermes-agent/pull/51322"
click P60017 "https://github.com/NousResearch/hermes-agent/pull/60017"
click P61944 "https://github.com/NousResearch/hermes-agent/pull/61944"
click P66047 "https://github.com/NousResearch/hermes-agent/pull/66047"
click P68162 "https://github.com/NousResearch/hermes-agent/pull/68162"
click P68168 "https://github.com/NousResearch/hermes-agent/pull/68168"
click P69174 "https://github.com/NousResearch/hermes-agent/pull/69174"
click P71277 "https://github.com/NousResearch/hermes-agent/pull/71277"
click P75933 "https://github.com/NousResearch/hermes-agent/pull/75933"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 23 pull requests and 8 issues in this complex. Each diff was read against this issue; Assessment working set: 270 kB of PR diffs, 78 kB of issue/PR text, 72 kB of discussion (55 comments), 123 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
Summary
Verification
scripts/run_tests.sh tests/hermes_cli/test_mcp_startup.py tests/tools/test_mcp_tool.py tests/tools/test_mcp_probe.py -qgit diff --checkpassedSafety
No provider, authentication, gateway, user configuration, or GBrain data changes.