Skip to content

fix: support packaged memory provider skills - #18842

Closed
smarzola wants to merge 1 commit into
NousResearch:mainfrom
smarzola:fix/memory-provider-plugin-packaging
Closed

fix: support packaged memory provider skills#18842
smarzola wants to merge 1 commit into
NousResearch:mainfrom
smarzola:fix/memory-provider-plugin-packaging

Conversation

@smarzola

@smarzola smarzola commented May 2, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add hermes_agent.memory_providers entry-point discovery/loading for pip-installed memory providers
  • allow memory provider registration contexts to forward register_skill(...) into the plugin skill registry
  • lazily load only the active memory provider namespace when skill_view("provider:skill") misses the plugin registry
  • add regression tests for entry-point providers, provider-packaged skills, and inactive provider guardrails

Why

Memory providers use the exclusive plugins.memory activation path rather than the general PluginManager. That means provider packages can register a memory provider, but provider-packaged skills are not visible to skill_view() unless the provider is loaded through another path. This also makes pip-installed memory providers less complete than directory-installed plugins.

This keeps the exclusive provider model intact while letting packaged memory providers expose read-only skills such as maintenance/runbook workflows.

Test Plan

  • python -m pytest tests/test_plugin_skills.py tests/agent/test_memory_provider.py -q
  • ruff check plugins/memory/__init__.py tools/skills_tool.py tests/test_plugin_skills.py tests/agent/test_memory_provider.py
  • python -m compileall -q plugins/memory tools/skills_tool.py tests/test_plugin_skills.py tests/agent/test_memory_provider.py
  • git diff --check -- plugins/memory/__init__.py tools/skills_tool.py tests/test_plugin_skills.py tests/agent/test_memory_provider.py
  • independent reviewer subagent re-reviewed the final diff and passed it

@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins tool/skills Skills system (list, view, manage) labels May 2, 2026
@lombax85

lombax85 commented May 2, 2026

Copy link
Copy Markdown

+1

@smarzola
smarzola force-pushed the fix/memory-provider-plugin-packaging branch 2 times, most recently from 185c521 to c54ee88 Compare June 14, 2026 11:12
@smarzola

Copy link
Copy Markdown
Contributor Author

Rebased this PR onto current main to clear the stale branch.

Local verification on the rebased commit c54ee883b9ade7816ead3ce9700e7b7be5189412:

python -m pytest tests/agent/test_memory_provider.py tests/test_plugin_skills.py -q -o 'addopts='
# 126 passed, 1 warning in 5.30s

python -m ruff check plugins/memory/__init__.py tools/skills_tool.py tests/agent/test_memory_provider.py tests/test_plugin_skills.py
# All checks passed!

GitHub still reports no checks for the branch, but the PR is now mergeable (mergeable=true, mergeable_state=blocked).

@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 closing the gap between the documented pip-entry-point path and memory-provider skill registration. The current-main premise is verified: plugins/memory/__init__.py:124-205 only resolves directory providers, while CONTRIBUTING.md:72-80 already describes pip entry-point discovery.

Problems

  • The new tests do not cover the combined behavior this PR adds. tests/agent/test_memory_provider.py tests entry-point loading, and tests/test_plugin_skills.py tests a directory provider registering a skill, but neither verifies an active hermes_agent.memory_providers entry point whose register(ctx) calls register_skill() can be resolved by skill_view("provider:skill").

Suggested changes

  • Add that combined regression case using the existing fake-entry-point fixture.
  • Document the hermes_agent.memory_providers group and provider-packaged register_skill() support in website/docs/developer-guide/memory-provider-plugin.md, whose current directory-only layout begins at line 15.

Automated hermes-sweeper review.

@@ -679,6 +679,92 @@ def test_provider_load_after_cli_discovery(self, tmp_path, monkeypatch):
assert p.name == "extcliload"


class TestEntryPointMemoryProviderDiscovery:

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.

Please add an integration regression here for the actual promised combination: an active fake hermes_agent.memory_providers entry point whose register(ctx) invokes both register_memory_provider() and register_skill(), followed by skill_view("entrymem:maintenance"). The separate entry-point and directory-provider skill tests do not cover that path.

@teknium1 teknium1 added 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 12, 2026
@smarzola
smarzola force-pushed the fix/memory-provider-plugin-packaging branch from c54ee88 to bc23d41 Compare July 12, 2026 22:04
@smarzola

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review — I rebased the PR onto current main and addressed both findings in bc23d416b.

  • Added the combined packaged-provider regression: an active fake hermes_agent.memory_providers entry point whose register(ctx) registers both the memory provider and a skill, followed by a successful skill_view("entrymem:maintenance").
  • Added activation-safety coverage so inactive status/setup-style loads do not register skills and switching providers prunes stale provider skills without removing a same-qualified general-plugin override.
  • Documented the hermes_agent.memory_providers entry-point group and packaged ctx.register_skill() support in the memory-provider developer guide.

Verification: 138 focused provider/skill tests, 15 dashboard memory-provider tests, and 4 agent-initialization tests passed; Ruff and git diff --check are clean. An independent adversarial review of the final diff also returned clean.

Thanks again for catching the missing end-to-end coverage.

@teknium1

Copy link
Copy Markdown
Contributor

Resolved via PR #85527 (salvage of #80493) — your entry-point discovery for packaged memory providers and the ctx.register_skill() forwarding are on main with your authorship preserved in git history. You were the earliest submitter on this discovery gap; thanks for the original work.

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

Labels

area/memory Memory subsystem: store, providers, sync, background reviews 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 tool/skills Skills system (list, view, manage) type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants