Skip to content

fix(oneshot): join MCP discovery before the one-shot tool snapshot (bounded, gated, warns) - #61944

Closed
100yenadmin wants to merge 1 commit into
NousResearch:mainfrom
100yenadmin:fix/oneshot-mcp-discovery-wait
Closed

fix(oneshot): join MCP discovery before the one-shot tool snapshot (bounded, gated, warns)#61944
100yenadmin wants to merge 1 commit into
NousResearch:mainfrom
100yenadmin:fix/oneshot-mcp-discovery-wait

Conversation

@100yenadmin

Copy link
Copy Markdown
Contributor

The bug

hermes -z starts MCP discovery on a background thread (_prepare_agent_startupstart_background_mcp_discovery) but never joins it — the one-shot tool snapshot is built immediately, so configured mcp_servers tools are silently absent from the turn. The model then answers without them; with capable models this produces convincing fake tool-call output (we reproduced this — the tell is zero tool-role messages in the session).

Every other entrypoint already joins: the interactive CLI (cli.py wait_for_mcp_discovery()), the agent-setup mixin, and the TUI (bounded 30s join). One-shot is the only gap — and it's the mode where the user can't notice mid-run.

The fix

In run_oneshot, before the tool snapshot and before stderr is redirected: if any enabled mcp_servers are configured, wait_for_mcp_discovery(timeout=30.0) (the TUI's bound — one-shot has no late-binding refresh, so it's this turn or nothing). On timeout, one stderr warning names the servers that may still be pending, and the run proceeds. Zero cost when no MCP servers are configured — the wait is gated on the same config read the --toolsets resolver uses (now factored into a shared _read_mcp_server_names() helper instead of two inline copies).

Relationship to the open PRs in this area

#38456, #43544, and #60017 attack overlapping symptoms; posting this because the approaches differ on points we think matter: this one is bounded (30s, TUI-parity — not an unbounded wait or a raised global timeout), gated (no cost on the no-MCP path), placed before the stderr redirect so the pending-server warning actually reaches the terminal (warnings emitted after run_oneshot's redirect_stderr(devnull) are swallowed — worth checking in any variant), and covered by targeted tests (wait-called / not-called / timeout-warns-and-proceeds). Happy to consolidate with any of the other authors — one of these four should land, this is a sharp-edged silent failure.

Tests

tests/hermes_cli/test_oneshot_mcp_wait.py (3) + test_oneshot_usage_file + test_mcp_startup (14 total) + test_tui_resume_flow (48, covers the resolver refactor) — green via scripts/run_tests.sh, re-verified after rebasing onto today's main.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard tool/mcp MCP client and OAuth labels Jul 10, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating the one-shot startup race; current main confirms the missing join before the agent's tool snapshot (hermes_cli/main.py:12442-12449, hermes_cli/oneshot.py:206-235, agent/agent_init.py:1159-1163).

Problems

  • hermes_cli/oneshot.py:102 pins the join to 30 seconds, but the shared MCP startup policy is configurable and defaults to 1.5 seconds (hermes_cli/mcp_startup.py:53-71, hermes_cli/config.py:1372). TUI also calls the shared wait without overriding it (tui_gateway/server.py:4481-4489).
  • The wait is gated by configured MCP names rather than the effective one-shot toolset (hermes_cli/oneshot.py:94-96,259). Explicit non-MCP --toolsets and config no_mcp can therefore wait for tools that will not be exposed (hermes_cli/tools_config.py:1875-1893).

Suggested changes

  • Reuse the configured shared timeout and gate the wait on the resolved effective MCP selection.
  • Add coverage for explicit non-MCP toolsets, no_mcp, and a temp-HERMES_HOME one-shot snapshot regression.

Automated hermes-sweeper review.

Comment thread hermes_cli/oneshot.py Outdated
Comment thread hermes_cli/oneshot.py Outdated
@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 11, 2026
@100yenadmin
100yenadmin force-pushed the fix/oneshot-mcp-discovery-wait branch from 852779b to 535bed1 Compare July 15, 2026 05:33
@100yenadmin

Copy link
Copy Markdown
Contributor Author

Both review objections are fixed and pushed (rebased onto current main, single commit 535bed1a):

  1. Config-resolved timeout: the wait no longer hardcodes 30s — hermes_cli/oneshot.py now resolves mcp_discovery_timeout through the shared mcp_startup._resolve_discovery_timeout, exactly like the TUI path.
  2. Effective-toolset gating: the wait now keys on which enabled MCP servers can actually contribute to this invocation's resolved one-shot toolset — explicit non-MCP --toolsets and the no_mcp sentinel skip the wait entirely; all and selected MCP server names preserve it. The pending-warning copy reports the resolved timeout and only the selected servers.

Tests: the reviewer-requested cases are all in tests/hermes_cli/test_oneshot_mcp_wait.py (non-MCP toolsets skip, no_mcp skip, non-default config timeout reaches the wait, temp-HERMES_HOME wait-before-snapshot regression) — 7 passed, and 431 passed / 0 failed across the 11 adjacent suites via scripts/run_tests.sh.

Also re-verified none of #38456 / #43544 / #60017 landed an equivalent join on current main — the race is still live there. Consolidation offer stands.

@100yenadmin

Copy link
Copy Markdown
Contributor Author

Refreshed onto current main dd241cf at head 5ce9c7a448b62eddf730a727a7ee2da10622a38d. Both existing review findings remain fixed: the one-shot wait uses the shared configured MCP discovery timeout, and it is gated by the invocation effective MCP selection (including explicit non-MCP toolsets and no_mcp). The stale late-binding-refresh docstring claim was also corrected because one-shot has no interactive refresh. Proof: scripts/run_tests.sh tests/hermes_cli/test_oneshot_mcp_wait.py tests/hermes_cli/test_mcp_startup.py tests/hermes_cli/test_mcp_tools_config.py -q (11 passed); git diff --check passes. The two inline threads are resolved/outdated on this head.

Join background MCP discovery before the one-shot tool snapshot only when the invocation's effective toolset can use an enabled MCP server. Resolve the shared mcp_discovery_timeout from config, report that bound in warnings, and cover explicit, no_mcp, timeout, and snapshot-order regressions.
@100yenadmin
100yenadmin force-pushed the fix/oneshot-mcp-discovery-wait branch from 5ce9c7a to 770dddc Compare July 30, 2026 10:02
@100yenadmin

Copy link
Copy Markdown
Contributor Author

Rebased the reviewed one-commit patch without conflicts from old base dd241cf / old head 5ce9c7a448b62eddf730a727a7ee2da10622a38d onto exact fetched base c581ad4. New head: 770dddc.

Proof on the new head:

  • git range-diff: 5ce9c7a44 = 770dddc; stable patch ID unchanged at 5a52e5f6b335ccaa0acc607d29ab5a9bf556223e; original author Eva eva@100yen.org and authored date preserved.
  • scripts/run_tests.sh tests/hermes_cli/test_oneshot_mcp_wait.py tests/hermes_cli/test_mcp_startup.py tests/hermes_cli/test_mcp_tools_config.py -q: 11 passed, 0 failed.
  • git diff --check c581ad4..770dddc: passed.
  • Force-pushed with a lease pinned to the verified old fork head; local, fork, and PR now resolve to 770dddc.

Exact-head CI at posting: GitHub reports no check runs yet and mergeStateStatus BLOCKED. The PR base branch has since advanced to 9650f55, but this requested rebase intentionally used the already-fetched exact c581ad4 and did not fetch again.

@100yenadmin

Copy link
Copy Markdown
Contributor Author

Current-main follow-up: after the exact rebase base c581ad402e942c8ab5ce7eb0be325b33061d17bb, main advanced to 9650f555d072d89f51b2c659360bc99799d4bc3c. The later delta touches neither hermes_cli/oneshot.py nor tests/hermes_cli/test_oneshot_mcp_wait.py, and GitHub reports this PR mergeable. No additional rebase is needed for that delta.

Remaining gate: exact-head CI is action_required until an upstream maintainer approves the external-fork run: https://github.com/NousResearch/hermes-agent/actions/runs/30533155393

@100yenadmin

Copy link
Copy Markdown
Contributor Author

Closing — superseded by 3572d4b (ensure_mcp_discovery_before_agent_build), which landed the same fix at the same seam: one-shot now starts and joins MCP discovery before the tool snapshot, with a dedicated mcp_single_query_discovery_timeout (default 15s) replacing the interactive 1.5s bound. That covers both findings this PR carried (config-resolved timeout, no hardcoded wait). Nothing here survives that isn't already on main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades tool/mcp MCP client and OAuth type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants