fix: make _is_memory_provider_dir detect re-exported MemoryProvider s… - #53393
fix: make _is_memory_provider_dir detect re-exported MemoryProvider s…#53393j-zly wants to merge 2 commits into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Clean simplification of the memory provider detection logic. Removes the synthetic _USER_NAMESPACE package registration in favor of direct importlib-based detection. The code removal is net-negative (22 additions, 51 deletions) which is a good sign — simplification.
Looks Good
- Removes unnecessary complexity (synthetic package registration)
- Simpler detection logic for re-exported MemoryProvider
- No security concerns
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying the re-export discovery gap; current plugins/memory/__init__.py:74-85 does only a literal text scan, so a thin provider root can be missed.
Problems
- The patch removes the synthetic
_hermes_user_memorypackage registrations. Currentplugins/memory/__init__.py:253-256and:395-406install those parents before loading user-provider modules andcli.py. Their removal would reintroduce the relative-import failure guarded bytests/agent/test_memory_provider.py:540-571and:610-640. - Please add a regression that has neither
MemoryProvidernorregister_memory_providerin the root__init__.py. The current nested-reexport test attests/agent/test_memory_provider.py:573-607still containsregister_memory_provider, so it takes the existing fast path.
Suggested changes
- Preserve
_USER_NAMESPACE/_register_synthetic_package()and their provider/CLI call sites while adding the fallback detection. - Cover discovery and loading for the marker-free re-export layout.
Automated hermes-sweeper review.
|
|
||
| # Fallback: import and check for MemoryProvider subclass | ||
| try: | ||
| provider = _load_provider_from_dir(path) |
There was a problem hiding this comment.
This fallback needs to retain the synthetic _hermes_user_memory parent-package setup removed elsewhere in this patch. _load_provider_from_dir() executes user providers under that namespace; without the registrations, relative imports regress (tests/agent/test_memory_provider.py:540-571 covers this on current main).
…ports - Restore _USER_NAMESPACE / _register_synthetic_package() removed in previous iteration - Call _register_synthetic_package before loading user provider modules in _load_provider_from_dir and discover_plugin_cli_commands - Add regression test for _is_memory_provider_dir fallback path when __init__.py lacks literal MemoryProvider/register_memory_provider
…ubclasses
What does this PR do?
Related Issue
Fixes #
Type of Change
Changes Made
How to Test
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AFor New Skills
hermes --toolsets skills -q "Use the X skill to do Y"Screenshots / Logs