Skip to content

fix(cli): await plugin discovery before toolset warning - #84499

Open
qixuancao wants to merge 2 commits into
NousResearch:mainfrom
qixuancao:fix/plugin-toolset-startup-validation
Open

qixuancao wants to merge 2 commits into
NousResearch:mainfrom
qixuancao:fix/plugin-toolset-startup-validation

Conversation

@qixuancao

Copy link
Copy Markdown

What does this PR do?

Fixes false Unknown toolsets warnings when configured plugin toolsets are selected while plugin discovery is still in flight.

HermesCLI could validate a persisted plugin toolset before its plugin finished registering. The normal hermes entry point tracks its background discovery thread, but direct cmd_chat / python cli.py -w can also start discovery through the untracked tool-prewarm path. In that path, _discovered previously doubled as both a re-entry guard and a completion signal, allowing validation to observe a half-loaded registry.

This change:

  • re-validates initially unknown CLI toolsets after plugin discovery completes;
  • gives PluginManager a single serialized discovery owner and completion condition;
  • keeps same-thread registration re-entry safe;
  • wakes concurrent waiters after failure so later discovery can retry;
  • avoids redundant background discovery and preserves non-blocking built-in-only startup;
  • treats cached plugin toolset keys as known during config diagnostics.

Prior work / attribution

This is a current-main follow-up to #25714 by @rewasa. That PR identified the original validation-before-discovery bug and proposed the initial discover-and-revalidate fix. Thank you to @rewasa for the diagnosis and first patch.

#25714 is currently conflicting with main; this follow-up retains its core fix and adds coverage for the later tool-prewarm concurrency path, discovery ownership, same-thread re-entry, failure/retry, and startup performance semantics.

Related Issue

Follow-up / replacement for #25714. No separate issue exists.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • cli.py: wait for completed plugin discovery before declaring configured toolsets unknown.
  • hermes_cli/plugins.py: serialize discovery ownership, concurrent waiting, re-entry, failure, retry, and background-start semantics.
  • hermes_cli/tools_config.py: recognize persisted plugin toolset keys during config validation without forcing startup to block.
  • tests/cli/test_plugin_toolset_startup_validation.py: add deterministic Event-based regressions for tracked and direct-prewarm startup paths.
  • tests/hermes_cli/test_plugins.py: cover concurrent discovery, same-thread re-entry, failure wake/retry, and waiter-cycle behavior.
  • contributors/emails/qixuancao36@gmail.com: map the commit-author email for attribution CI.

How to Test

  1. Enable a user plugin that registers a toolset and save that toolset for CLI use.
  2. Run hermes chat -Q --source tool --max-turns 1 -q 'Reply exactly OK without calling tools.' and confirm no false Unknown toolsets warning appears.
  3. Run the direct prewarm path with python cli.py -w --quiet --max-turns=1 ... and confirm it also returns without the false warning.
  4. Configure a genuinely missing toolset and confirm it still warns.
  5. Run:
.venv/bin/python scripts/run_tests_parallel.py tests/cli
.venv/bin/pytest -q \
  tests/cli/test_plugin_toolset_startup_validation.py \
  tests/hermes_cli/test_plugins.py \
  tests/scripts/test_contributor_map.py

Local results on Linux x86_64:

  • full tests/cli: 950 passed, 8 skipped, 0 failed;
  • focused plugin/startup/attribution tests: 55 passed;
  • real configured-plugin chat canary: exit 0, OK, no false warning;
  • fresh-process plugin health dispatch: valid JSON;
  • direct python cli.py -w canary: exit 0, OK, no false warning;
  • Ruff, byte compilation, attribution audit, and git diff --check: passed.

Checklist

Code

Documentation & Housekeeping

  • Documentation changes are N/A; behavior and concurrency contracts are documented in code and tests
  • cli-config.yaml.example changes are N/A; no config keys changed
  • CONTRIBUTING.md / AGENTS.md changes are N/A; no workflow changed
  • I've considered cross-platform impact; coordination uses Python threading primitives only
  • Tool descriptions/schemas changes are N/A

Screenshots / Logs

$ hermes chat -Q --source tool --max-turns 1 -q 'Reply exactly OK without calling tools.'
OK

$ python cli.py -w --quiet --max-turns=1 -q 'Reply exactly OK without calling tools.'
OK

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard comp/plugins Plugin system and bundled plugins area/config Config system, migrations, profiles P3 Low — cosmetic, nice to have labels Aug 12, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference, author can ignore or act on any point.

fix(cli): await plugin discovery before toolset warning

  • cli.py (~L4526): the first validation pass only knows validate_toolset(t) and mcp_names. When background discovery has already completed before CLI startup validation runs, a legitimate plugin toolset still fails the first pass and forces a redundant synchronous discover_plugins() join + re-validation. Consider consulting get_plugin_toolset_keys_nowait() in the first pass so the join only happens when discovery is genuinely still in flight.
  • hermes_cli/plugins.py discover_and_load(): the same-thread re-entry guard (if self._discovery_owner == current_thread: return) makes a plugin's register()-time discover_plugins() return before the sweep completes. Transitively-triggered callers that read plugin state (e.g. toolset keys) will observe partial registration. The docstring documents this as intentional, but it is a subtle contract future callers can easily trip over.
  • cli.py: if discover_plugins() raises, the exception is swallowed at debug level and the "Unknown toolsets" warning is still emitted. Since that warning may then be misleading, consider logging at warning level so a failed discovery isn't mistaken for a genuine unknown-toolset config.

qixuancao added 2 commits August 17, 2026 07:42
…iscovery failure

- First validation pass now also accepts toolsets from
  get_plugin_toolset_keys_nowait(), so a toolset whose background
  discovery already completed no longer forces a redundant synchronous
  discover_plugins() join and re-validation.
- Log discovery failure at warning level so the subsequent Unknown
  toolsets warning is not mistaken for a genuine unknown-toolset config.
- Add regression test: completed background discovery skips the join.

This branch has not been deployed

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

Labels

area/config Config system, migrations, profiles comp/cli CLI entry point, hermes_cli/, setup wizard comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants