fix(plugins): pin discovery to the process home, not the triggering request - #331
Merged
Merged
Conversation
…equest Plugin discovery is lazy, and several tools call `_ensure_plugins_discovered()` from inside an agent turn — `tts_tool`, `web_tools`, `video_generation_tool`, `browser_tool`. The gateway's turn handler binds `set_hermes_home_override(session_profile_home)` for the duration of a turn, and `discover_and_load` resolved its user plugin dir with `get_hermes_home()`. So whichever session happened to trigger the FIRST discovery decided the whole process's plugin registry, permanently — the singleton never rescans. In a backend serving several profiles the launch profile could lose its own plugins AND inherit another profile's, depending only on which chat ran a plugin-touching tool first. Reproduced with two homes: discovery fired under a profile override, and afterwards the launch profile's own `/launch-cmd` was gone while the other profile's `/profile-cmd` answered for it. Bind the process home around the sweep. That covers the user plugin dir, the enabled/disabled allow-lists (`load_config` follows the override too, so changing only the directory would have scanned one home while gating on another), and anything a plugin's own `register()` reads — one consistent scope instead of a mix. The process home is the only coherent scope here: plugins register process-global capabilities (tools, hooks, middleware, platforms, the context engine) that every session on the backend shares, and `PluginManager` has no lock, so re-scoping per request would mean clearing registrations out from under live turns. Same rationale as the dashboard's user plugin dir in `hermes_cli/web_server.py` and the goal store in #320. This does NOT give non-launch profiles their own plugins when one backend serves many — that needs a per-profile registry and is left alone deliberately. It makes the existing scope deterministic and correct for the per-profile deployments that are the norm: a dedicated backend or slash worker exports HERMES_HOME at spawn, so the process home IS that profile's home there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Plugin discovery is lazy, and several tools call
_ensure_plugins_discovered()from inside an agent turn —
tts_tool,web_tools,video_generation_tool,browser_tool. The gateway's turn handler bindsset_hermes_home_override(session_profile_home)for the duration of a turn, anddiscover_and_loadresolved its user plugin dir withget_hermes_home().So whichever session happened to trigger the first discovery decided the whole
process's plugin registry — permanently, since the manager is a singleton that
never rescans. In a backend serving several profiles, the launch profile could
lose its own plugins and inherit another profile's, depending only on which chat
ran a plugin-touching tool first.
Reproduced with two homes, discovery fired under a profile override:
With this change, the same probe returns
launch-cmdregistered,/launch-cmdfound,
/profile-cmdnot visible.The fix binds the process home around the sweep. Binding, rather than changing
the one
get_hermes_home()call, becauseload_config()follows the overridetoo — pinning only the directory would have scanned one home while gating on
another profile's
plugins.enabledallow-list. One consistent scope covers theplugin dir, both allow-lists, and anything a plugin's own
register()reads.Why the process home is the right scope — and what this deliberately does not fix
Plugins register process-global capabilities: tools, hooks, middleware,
platforms, the context engine. Every session on the backend shares them, and
PluginManagerholds no lock. Re-scoping discovery per request would meanclearing and re-registering that state out from under live turns — a data race
that would break other profiles' in-flight sessions, not just a slow path.
So this PR makes the existing scope deterministic, in the spirit of #320
("make the goal store's launch-home scope deliberate, not accidental") and
matching the dashboard's user plugin dir, which already resolves via
get_process_hermes_home()for the same reason.It does not give non-launch profiles their own plugins when one backend serves
many. That needs a per-profile registry — a real design change — and I have left
it alone rather than half-doing it. The common deployment is unaffected either
way: a dedicated per-profile backend, and the slash worker, both export
HERMES_HOMEat spawn, so the process home is that profile's home there. Thereis a test pinning that.
Related Issue
No filed issue — found while auditing
command.dispatchfor profile scoping, theaudit behind #323, #324, #326, and #328.
Type of Change
Changes Made
hermes_cli/plugins.py—discover_and_load()bindsset_hermes_home_override(get_process_hermes_home())around_discover_and_load_inner(), released in afinallyso it is restored on boththe success and the raise path (the existing
exceptthat un-caches a failedsweep is preserved). Comment records why discovery is process-scoped.
hermes_cli/plugins.py— importsget_process_hermes_home,set_hermes_home_override,reset_hermes_home_override.hermes_cli/plugins.py—_discover_and_load_innerdocstring notes it runswith the process home bound, so the
get_hermes_home()and config reads insideit resolve to the launch home. The call at the user plugin dir is left as
get_hermes_home()deliberately: with the binding in place it resolvescorrectly, and one mechanism scoping the whole sweep is easier to reason about
than a mix of pinned and unpinned calls.
tests/hermes_cli/test_plugins.py— newTestDiscoveryIsScopedToTheProcessHome(4 tests).
How to Test
Confirm the new test fails without the fix. Revert only
hermes_cli/plugins.pyand runpytest tests/hermes_cli/test_plugins.py -k TestDiscoveryIsScopedToTheProcessHome.→ 1 failed, 3 passed.
Only
test_a_profile_scoped_request_does_not_capture_the_registryreproducesthe bug. The other three are guards and pass either way by design, which is
what you want from them:
..._override_is_restored_after_discovery— the sweep must not leak its ownbinding into the caller's scope (nothing to leak before the fix).
..._override_is_restored_when_the_sweep_raises— same on the error path,and asserts the failed sweep is still not cached as discovered.
..._dedicated_profile_process_still_gets_its_own_plugins— proves this doesnot force every process onto one shared plugin set.
Confirm all four pass with it.
pytest tests/hermes_cli/test_plugins.py -q→ 120 passed.Check for regressions across the core plugin suites:
→ 302 passed, 0 failed. Baseline on an unpatched tree: 298 passed, 0
failed. The delta is exactly the four new tests; both runs fully green.
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — not claimed. The full suite is not green on this machine independent of this change. I ran the 8 core plugin suites above matched before and after (both fully green, 298 → 302). A wider sweep over all 58 test files that touchPluginManager/discover_plugins/_ensure_plugins_discoveredis running in matched worktrees; I will post the comparison as a comment, and this should not merge before it lands.Documentation & Housekeeping
docs/, docstrings) — or N/A (rationale comment at the binding;_discover_and_load_innerdocstring notes the bound scope)cli-config.yaml.exampleif I added/changed config keys — or N/A (N/A — no config keys)CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A (N/A — no change to the plugin authoring surface;register(ctx)and the manifest format are untouched)get_process_hermes_home()is the same platform-native resolver already used for dashboard plugins)Screenshots / Logs
Included inline above — the two-home probe before and after.