Skip to content

fix: scope plugin manager by profile home - #28109

Open
jbcrane13 wants to merge 2 commits into
NousResearch:mainfrom
jbcrane13:fix/profile-lcm-plugin-manager
Open

fix: scope plugin manager by profile home#28109
jbcrane13 wants to merge 2 commits into
NousResearch:mainfrom
jbcrane13:fix/profile-lcm-plugin-manager

Conversation

@jbcrane13

Copy link
Copy Markdown

Summary\n- Scope the plugin manager cache to the active Hermes home/profile instead of one process-global singleton\n- Prevent context-engine plugins like hermes-lcm from reusing another profile's engine and lcm.db\n- Add regression coverage for switching HERMES_HOME between profiles\n- Document the decision in docs/ADR.md\n\n## Verification\n- venv/bin/python -m pytest tests/hermes_cli/test_plugins.py tests/agent/test_context_engine.py tests/hermes_cli/test_plugins_cmd.py -q\n- venv/bin/python -m pytest /Users/blake/.hermes/plugins/hermes-lcm/tests -q\n- Manual smoke: daneel/friend/quentin/argus/dessin each resolve LCM to their own profile lcm.db\n

Copilot AI review requested due to automatic review settings May 18, 2026 17:21

Copilot AI 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.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Scopes plugin discovery/loading (and context-engine registration) to the active HERMES_HOME so profile switches in long-lived processes don’t leak plugin/context-engine state across profiles.

Changes:

  • Key the global PluginManager singleton by resolved Hermes home and recreate it when the active home changes.
  • Add a regression test ensuring context engines differ across different HERMES_HOME values.
  • Document the decision and consequences in an ADR.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
hermes_cli/plugins.py Reworks get_plugin_manager() to be home/profile-scoped and adds bookkeeping helpers.
tests/hermes_cli/test_plugins.py Adds coverage for profile-switch behavior to prevent context-engine reuse across homes.
docs/ADR.md Records the architectural decision to scope plugin manager state by Hermes home/profile.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread hermes_cli/plugins.py
Comment thread hermes_cli/plugins.py
Comment on lines +1420 to 1424
if _plugin_manager is None or _plugin_manager_home != current_home:
_plugin_manager = PluginManager()
_plugin_manager_home = current_home
_plugin_manager_obj_id = id(_plugin_manager)
return _plugin_manager

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

Comment thread hermes_cli/plugins.py
Comment on lines +1411 to +1418
# Tests and embedders historically monkeypatch ``_plugin_manager``
# directly. If that happened, adopt the injected manager for the current
# home instead of throwing it away because our profile-scope bookkeeping is
# stale.
if _plugin_manager is not None and current_obj_id != _plugin_manager_obj_id:
_plugin_manager_home = current_home
_plugin_manager_obj_id = current_obj_id
return _plugin_manager

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

Comment thread tests/hermes_cli/test_plugins.py Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins comp/cli CLI entry point, hermes_cli/, setup wizard labels May 18, 2026

@outsourc-e outsourc-e 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.

I like the profile-scoped cache direction, but I can’t get the touched plugin test file clean locally yet. Running python -m pytest tests/hermes_cli/test_plugins.py -q on this branch still fails test_plugin_context_engine_is_scoped_by_hermes_home, with engine_a is None where the new regression expects the first profile-specific context engine to load. Since that’s the exact surface this PR is trying to fix, I’d want that passing before merge.

@terry197913

Copy link
Copy Markdown
Contributor

Hi — just wanted to clarify the relationship between this PR and #50346 (ctx.profile_name), since they might look related at a glance.

#50346 exposes the current profile name inside a plugin's execution context — useful for plugins that need to know which profile they're running under.

This PR (#28109) addresses a different problem: the plugin manager itself is a process-global singleton, so when two profiles share the same process (e.g. gateway running multiple profiles), they also share the same plugin manager instance and its internal state, including the lcm.db path. This causes context-engine plugins like hermes-lcm to silently reuse the wrong profile's database.

The fix scopes the plugin manager cache to HERMES_HOME so each profile gets its own isolated instance. This is a complementary change to #50346, not a duplicate.

Would appreciate a review when you get a chance — happy to add more tests or adjust the approach if needed. Thanks!

@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 identifying the remaining profile-isolation gap; current main still has a single process-global manager at hermes_cli/plugins.py:2027-2035, while discovery reads user plugins from the active home at hermes_cli/plugins.py:1347-1352.

Problems

  • The regression switches HERMES_HOME in the process environment, but the multi-profile gateway uses set_hermes_home_override() (gateway/run.py:1438-1444), whose contract deliberately avoids mutating os.environ (hermes_constants.py:23-30). It does not cover the production mechanism.
  • _make_plugin_dir() writes plugins.enabled using the ambient HERMES_HOME when present (tests/hermes_cli/test_plugins.py:67-84); the new test invokes that helper before selecting either target home. This aligns with the existing failing-test review reporting engine_a is None.
  • A fresh manager alone does not clear hermes_plugins.<slug>.* submodules. _load_plugin() replaces only the package module in sys.modules (hermes_cli/plugins.py:1843-1865), so relative imports can reuse code/state from the prior profile.

Suggested changes

  • Test with set_hermes_home_override() and explicit per-profile config, and purge or home-qualify plugin submodules. Use a keyed manager cache rather than repeatedly replacing one global slot.

Automated hermes-sweeper review.

Comment thread hermes_cli/plugins.py
_plugin_manager = PluginManager()
_plugin_manager_home = current_home
_plugin_manager_obj_id = id(_plugin_manager)
return _plugin_manager

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.

Creating a fresh manager does not clear hermes_plugins.<slug>.*; _load_plugin() only replaces the package parent, so relative imports can reuse profile A submodules under profile B. Purge the package and its prefixed submodules, or use a home-qualified import namespace, with a relative-import regression.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
@terry197913

Copy link
Copy Markdown
Contributor

A cleaner re-implementation of the same fix is now available at PR #63702 (luna/pr-28109-profile-isolation-fix).

This version:

  • Has clean commit history (single squashed commit)
  • Adds full regression tests (test_profile_isolation_separate_managers_and_state, test_force_rediscover_clears_all_caches)
  • 118 plugin tests pass
  • All 3 original issues resolved (multi-profile cache isolation, test helper home= param, module cache eviction)

Feel free to close this PR in favor of #63702, or cherry-pick as needed. Happy to help with anything needed for merge.


Comment from terry197913 (Luna)

@teknium1 teknium1 added the area/profiles Multi-profile isolation, HERMES_HOME scoping label Jul 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/profiles Multi-profile isolation, HERMES_HOME scoping comp/cli CLI entry point, hermes_cli/, setup wizard comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants