fix(memory): honor config.yaml limits in registry-dispatched memory calls (#11665) - #11693
fix(memory): honor config.yaml limits in registry-dispatched memory calls (#11665)#11693briandevans wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes inconsistent memory tool behavior across dispatch paths by ensuring registry-dispatched memory calls also honor config.yaml memory limits and enablement flags, aligning behavior with the main AIAgent loop (Fixes #11665).
Changes:
- Add a registry-dispatch store resolver in
tools/memory_tool.pythat selectsstorevia: explicit kwarg →parent_agent._memory_store→ lazily-built config-driven singleton. - Add a test reset hook for the module-level singleton cache to support tests that mutate
HERMES_HOME. - Add a focused regression test suite covering the full store-resolution truth table and an end-to-end
registry.dispatch("memory", ...)path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/memory_tool.py | Adds fallback store-resolution logic for registry-dispatched memory calls and updates registry handler to use it. |
| tests/tools/test_memory_tool.py | Adds regression tests validating store-resolution precedence and end-to-end registry dispatch behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from hermes_cli.config import load_config | ||
| cfg = load_config() or {} | ||
| mem_cfg = cfg.get("memory", {}) or {} | ||
| if not ( | ||
| mem_cfg.get("memory_enabled", False) | ||
| or mem_cfg.get("user_profile_enabled", False) | ||
| ): |
There was a problem hiding this comment.
Calling hermes_cli.config.load_config() here will run ensure_hermes_home(), which creates ~/.hermes subdirs and seeds SOUL.md (and may print warnings on YAML parse errors). That means a registry-dispatched memory call can have filesystem side effects even when config ultimately disables memory/user profile and the resolver returns None. Consider reading just the memory section without ensure_hermes_home (e.g., read_raw_config() merged with DEFAULT_CONFIG["memory"]) and only creating directories/loading from disk once you’ve confirmed memory_enabled/user_profile_enabled is true.
| monkeypatch.setenv("HERMES_HOME", str(hermes_home)) | ||
| monkeypatch.setattr("tools.memory_tool.get_memory_dir", lambda: hermes_home) | ||
|
|
||
| # Dispatch via the registry with NO store kwarg and NO parent_agent | ||
| # — mirrors handle_function_call's path. | ||
| raw = registry.dispatch( | ||
| "memory", | ||
| {"action": "add", "target": "memory", "content": "routed via registry dispatch"}, | ||
| ) | ||
| result = json.loads(raw) | ||
| # On unpatched code this fails with "Memory is not available". | ||
| assert result.get("success") is True, f"dispatch returned: {result}" | ||
|
|
||
| # The entry should have been persisted under the tmp HERMES_HOME. | ||
| memory_md = hermes_home / "MEMORY.md" | ||
| assert memory_md.exists(), "dispatch did not persist to config-driven hermes_home" | ||
| assert "routed via registry dispatch" in memory_md.read_text(encoding="utf-8") |
There was a problem hiding this comment.
These tests monkeypatch get_memory_dir to return hermes_home directly, so they assert persistence to HERMES_HOME/MEMORY.md. In production, get_memory_dir() resolves to HERMES_HOME/memories, so the persisted file path is HERMES_HOME/memories/MEMORY.md. To keep the end-to-end registry dispatch test representative, consider patching get_memory_dir to hermes_home / "memories" and updating the assertion accordingly.
|
Addressed both Copilot review points in
Validation: |
alt-glitch
added
type/bug
|
Hey! 👋 Just wanted to gently check in on this one — it's blocking a memory limit increase that would help a lot. Currently running at 95% capacity (2,991/3,000 chars) and really looking forward to the bump to 8K once this merges. No rush at all — just wanted to make sure it hasn't fallen through the cracks. Happy to help test if needed! Thanks for all the amazing work on Hermes 🙏 |
5c2f58c to
91d3c37
Compare
|
Rebased onto current |
91d3c37 to
455b97f
Compare
|
Re-rebased onto current Diff is now strictly limited to |
|
CI audit — all 33 Failures grouped by category — all reproduce on clean
None of these tests import from |
|
Hey team! 👋 Just checking in on this one — all Copilot feedback was addressed back in April, and the branch has been rebased on latest main. This is a nice bugfix that ensures the No rush — just making sure it hasn't fallen through the cracks. |
👋 Bump — PR Needs Review + MergeHey team! Just checking in again. This PR has been open for ~23 days and the fix is solid. Status Recap
What's Needed to Merge
This is blocking a |
455b97f to
d2b0d8f
Compare
|
Hey folks! 👋 Just checking in on this one — it's been over a month and the fix looks good. The test failure is a pre-existing baseline issue (not from this PR), so all the blocking checks are actually green. Is there anything else needed to move this forward? Happy to help where needed! 🚀 |
d2b0d8f to
847aa1a
Compare
847aa1a to
d260a24
Compare
d260a24 to
69c9f4b
Compare
|
Hey team! 👋 Just a gentle nudge on this PR whenever someone has a moment. Latest checks are green and it looks review-ready from our side. No rush at all — just making sure it hasn't slipped through the cracks 🙏 |
69c9f4b to
27a0445
Compare
…alls (NousResearch#11665) The memory tool is reachable through two code paths: * ``AIAgent._invoke_tool`` / the main agent loop — explicitly passes ``store=self._memory_store``, which ``AIAgent.__init__`` built from ``config.yaml`` (``memory.memory_char_limit`` / ``memory.user_char_limit``). * Any dispatch through ``model_tools.handle_function_call`` → ``tools.registry.registry.dispatch("memory", ...)`` — the registry handler lambda read ``store=kw.get("store")``, and nothing else in the codebase passed ``store=`` through that path. The result was that code_execution sandboxes, RL training environments, reward/verifier helpers (``environments/tool_context.py``), plugin dispatches, and any future entry point that routes through the registry silently errored with ``"Memory is not available"`` or (with a locally built store) enforced the hardcoded 2200/1375 defaults instead of the user's ``config.yaml`` values. Smallest safe fix ----------------- Extend the registry handler with a ``_resolve_memory_store_from_kwargs`` fallback chain: 1. Explicit ``store=`` kwarg (main agent loop + unit tests — unchanged). 2. ``kw["parent_agent"]._memory_store`` — ``hermes_cli/plugins.py`` plumbs ``parent_agent`` into dispatch kwargs, so plugin-initiated memory calls now reuse the live agent's store. 3. A process-wide config-driven ``MemoryStore`` singleton, built lazily from ``hermes_cli.config.load_config()``. Cached so repeated dispatches don't re-read ``config.yaml`` or reload on-disk files. Returns ``None`` when both ``memory_enabled`` and ``user_profile_enabled`` are false, preserving the ``"Memory is not available"`` error for users who opted out. Also exposes ``_reset_default_store_for_tests`` so tests that mutate ``HERMES_HOME`` can force the next dispatch to rebuild the singleton. Precedence truth table ---------------------- | caller / state | store used | | --- | --- | | explicit ``store=`` kwarg passed | that store (unchanged) | | plugin dispatch with parent_agent, parent has store | parent's store | | plugin dispatch with parent_agent, parent store is None (subagent) | falls through to config default | | no store, no parent_agent, memory enabled in config | config-driven singleton (new) | | no store, no parent_agent, memory disabled in config | ``None`` → "not available" error (unchanged) | Internal state isolation ------------------------ The main agent loop continues to use ``self._memory_store`` directly in all three special-case sites (``_invoke_tool``, main tool-calling loop, flush_memories) — the fallback chain only activates when the registry handler lambda fires, so no behavior change for users on the normal CLI path. The ``memory_char_limit``/``user_char_limit`` in ``config.yaml`` is now the single source of truth for every dispatch path. Validation ---------- ``source venv/bin/activate && python -m pytest tests/tools/test_memory_tool.py -q`` -> 41 passed (33 existing + 8 new). The behavioral dispatch test (``test_registry_dispatch_uses_config_memory_limit``) fails on ``origin/main`` with ``AssertionError: dispatch returned: {'error': 'Memory is not available...'}``, and passes with the fix, confirming the regression is covered. CI-aligned broad suite: ``python -m pytest tests/ -q --ignore=tests/integration --ignore=tests/e2e --tb=line -n auto`` -> 12389 passed, 39 skipped, 7 baseline failures (``test_matrix``, ``test_approve_deny_commands`` ×2, ``test_gateway_wsl`` ×2, ``test_file_staleness`` ×2). All 7 reproduce on clean ``origin/main`` with identical assertions — none are in the touched code path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ies/ layout Addresses the Copilot review on PR NousResearch#11693: 1. ``_resolve_memory_store_from_kwargs`` now calls ``read_raw_config()`` instead of ``load_config()``. ``load_config()`` runs ``ensure_hermes_home()``, which creates ``~/.hermes`` subdirs and seeds ``SOUL.md`` — that's a visible filesystem side effect for every registry-dispatched memory call, even when memory is ultimately disabled and the resolver returns ``None``. ``read_raw_config()`` is a plain YAML read, and the memory section is merged with ``DEFAULT_CONFIG["memory"]`` so default flags/limits still resolve. 2. Updated the new regression tests in ``TestRegistryDispatchStoreResolution`` to patch ``get_memory_dir`` to ``HERMES_HOME/memories`` — matching the production layout (``tools/memory_tool.py:55``: ``get_memory_dir() -> get_hermes_home() / "memories"``) rather than ``HERMES_HOME`` directly. The end-to-end behavioral test's assertion now checks ``HERMES_HOME/memories/MEMORY.md``, which is what a real dispatch produces. 3. Strengthened ``test_default_store_returns_none_when_memory_disabled`` with an explicit assertion that the ``memories/`` subdir was NOT created — if the resolver regains ``ensure_hermes_home()``-style side effects in the future, this test will fail. Validation: ``source venv/bin/activate && python -m pytest tests/tools/test_memory_tool.py -q`` -> 41 passed. The two behavioral tests still fail on ``origin/main`` (dispatch returns ``"Memory is not available"`` / ImportError on the helper) and pass with the full fix. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
27a0445 to
4c98db8
Compare
|
Housekeeping: closing to keep my open-PR set focused on actively-reviewed work. This has been open ~45d without maintainer review and the surrounding code has continued to move, so it's unlikely to land as-is. The underlying fix still stands — happy to reopen and rebase if it would be useful. Thanks! |
|
Superseded by #37102 — same fix, fresh rebase on latest Full context: |
What does this PR do?
Fixes #11665.
The
memorytool is reachable through two code paths, and until now only one of them respectedconfig.yaml:AIAgent._invoke_tool/ the main agent loop passesstore=self._memory_store, whichAIAgent.__init__(run_agent.py:1219) builds with the user's configuredmemory.memory_char_limit/memory.user_char_limit. ✅model_tools.handle_function_call→tools.registry.registry.dispatch("memory", ...). The registry handler lambda readsstore=kw.get("store"), and nothing passesstore=through that path. ❌Code paths affected by Path B:
tools/code_execution_tool.py(execute_code sandboxes that call tools back via JSON-RPC)environments/agent_loop.py(RL training loop)environments/tool_context.py(reward / verifier helpers)hermes_cli/plugins.py(plugin dispatch — already plumbsparent_agentbut the handler wasn't using it)On pre-fix code, dispatches via Path B silently errored with
"Memory is not available"(the reporter's description of "hardcoded defaults" maps to the same underlying symptom — config.yaml values were invisible to that path). Anymemory_char_limit: 3000oruser_char_limit: …override a user set inconfig.yamlwas ignored in every dispatch site that wasn't the mainAIAgentloop.Root cause
tools/memory_tool.py:572— the registry handler:storeis never populated byhandle_function_callorregistry.dispatch, so the tool always receivedNoneand short-circuited at theif store is Noneguard (tools/memory_tool.py:475).Smallest safe fix
Extend the registry handler with
_resolve_memory_store_from_kwargs— a fallback chain:store=kwarg — main agent loop + tests, unchanged.kw[\"parent_agent\"]._memory_store—hermes_cli/plugins.pyalready plumbsparent_agentinto dispatch kwargs, so plugin-initiated memory calls now reuse the live agent's store.MemoryStoresingleton built lazily fromhermes_cli.config.load_config(). Cached so repeated dispatches don't re-readconfig.yamlor the on-disk MEMORY.md / USER.md files.memory_enabledanduser_profile_enabledarefalsein config, returnsNone— preserves the"Memory is not available"error for users who opted out.Also exports
_reset_default_store_for_testsso tests that mutateHERMES_HOMEcan force the next dispatch to rebuild the singleton. No public API change.Precedence / compat truth table
store=kwarg passedparent_agent, parent has storeparent_agent, parent store isNone(subagent)memory_enabled: truein configmemory_enabled: falseANDuser_profile_enabled: falseNone→ "not available" error (unchanged)The main agent loop still uses
self._memory_storedirectly in all three special-case sites (_invoke_tool, main tool-calling loop,flush_memories), so there is no behavior change for users on the normal CLI path. The fallback chain only activates when the registry handler lambda fires.Narrow scope — why only the registry handler
I intentionally did not touch
handle_function_call,registry.dispatch, or any of the caller sites that invoke them. Addingstore=to those signatures would ripple through RL training, sandbox RPC, plugin context, and the public plugin API. The fallback-chain approach scoped insidememory_tool.pyis local, is reversible, and keeps the single source of truth onconfig.yaml.How to test
Run the focused suite:
→ 41 passed (33 existing + 8 new).
Confirm the new behavioral test catches the bug on origin/main:
→ fails with
Restore with
git stash pop.CI-aligned broad suite:
→ 12389 passed, 39 skipped, 7 pre-existing baseline failures (
tests/gateway/test_matrix.py,tests/gateway/test_approve_deny_commands.py×2,tests/hermes_cli/test_gateway_wsl.py×2,tests/tools/test_file_staleness.py×2). Each reproduces on cleanorigin/mainwith identical assertions — none are in the touched code path.Tested on: macOS 15 (Darwin 25.5.0), Python 3.11.15.
Related Issue
Fixes #11665
Type of Change
Changes Made
tools/memory_tool.py: add_resolve_memory_store_from_kwargs+_reset_default_store_for_tests+ module-level lazy singleton; swap the registry handler to use the resolver.tests/tools/test_memory_tool.py: addTestRegistryDispatchStoreResolution— 8 tests covering the full truth table (explicit store wins, parent_agent fallback, subagent-with-None falls through, config-driven default, user_profile-only activation, memory-disabled returns None, singleton identity, end-to-end registry dispatch).Adjacent surfaces checked
run_agent.pyspecial-case branches at lines 7411 and 7894 — unchanged; they still passstore=self._memory_store.run_agent.py:1219AIAgent.__init__MemoryStore construction — unchanged.tools/delegate_tool.py:366(subagentskip_memory=True) — the new behavior is safe: subagents still don't get a store (parent_agent._memory_storeisNoneon them); the resolver falls through to the config-driven default, but the subagent's ownskip_memory=Truekeeps it from dispatching memory in the first place.hermes_cli/plugins.py:291plugin dispatch withparent_agent— now transparently uses parent's store.tools/code_execution_tool.pysandbox dispatch — now works with config-driven defaults.environments/agent_loop.pyandenvironments/tool_context.py— now work with config-driven defaults.Checklist
Code
fix(scope):)origin/mainwithout the fix, the rest pin the fallback chain)Documentation & Housekeeping
memory.memory_char_limit/memory.user_char_limitkeys are unchangedNotes for reviewers
python -m pytest tests/ -q --ignore=tests/integration --ignore=tests/e2e --tb=short -n auto(matches.github/workflows/tests.yml).hermes_cli/**changes — the Nix workflow should not trigger.