diff --git a/agent/agent_init.py b/agent/agent_init.py index 769a6469fe..b862fb5643 100644 --- a/agent/agent_init.py +++ b/agent/agent_init.py @@ -1811,9 +1811,12 @@ def init_agent( _memory_toolset_requested = "memory" in (agent.enabled_toolsets or []) if not skip_memory or _memory_toolset_requested: try: - from tools.memory_tool import get_builtin_memory_store_flags + from tools.memory_tool import ( + get_builtin_memory_config, + get_builtin_memory_store_flags, + ) - mem_config = _agent_cfg.get("memory", {}) + mem_config = get_builtin_memory_config(_agent_cfg) agent._memory_enabled, agent._user_profile_enabled = get_builtin_memory_store_flags( _agent_cfg ) @@ -1823,6 +1826,8 @@ def init_agent( agent._memory_store = MemoryStore( memory_char_limit=mem_config.get("memory_char_limit", 2200), user_char_limit=mem_config.get("user_char_limit", 1375), + memory_enabled=agent._memory_enabled, + user_profile_enabled=agent._user_profile_enabled, ) agent._memory_store.load_from_disk() except Exception: diff --git a/tests/agent/test_builtin_memory_disabled_surface.py b/tests/agent/test_builtin_memory_disabled_surface.py index 547d5ec347..90bcf6db8e 100644 --- a/tests/agent/test_builtin_memory_disabled_surface.py +++ b/tests/agent/test_builtin_memory_disabled_surface.py @@ -12,6 +12,9 @@ These tests exercise the real resolution chain (config on disk → check_fn → ``get_tool_definitions``) against a temp ``HERMES_HOME``, not mocks. """ +import json +from unittest.mock import patch + import pytest import yaml @@ -46,6 +49,14 @@ def hermes_home(tmp_path, monkeypatch): return home +def _memory_tool_definition(): + return next( + tool["function"] + for tool in get_tool_definitions(enabled_toolsets=["memory"], quiet_mode=True) + if tool["function"]["name"] == "memory" + ) + + def _memory_tool_names(): tools = get_tool_definitions(enabled_toolsets=["memory"], quiet_mode=True) return {tool["function"]["name"] for tool in tools} @@ -62,13 +73,19 @@ class TestBuiltinMemoryToolAvailability: _write_memory_config( hermes_home, memory_enabled=False, user_profile_enabled=True ) - assert "memory" in _memory_tool_names() + definition = _memory_tool_definition() + assert definition["parameters"]["properties"]["target"]["enum"] == ["user"] + assert "only 'user' is enabled" in definition["description"] + assert "only 'memory' is enabled" not in definition["description"] def test_tool_present_when_only_memory_enabled(self, hermes_home): _write_memory_config( hermes_home, memory_enabled=True, user_profile_enabled=False ) - assert "memory" in _memory_tool_names() + definition = _memory_tool_definition() + assert definition["parameters"]["properties"]["target"]["enum"] == ["memory"] + assert "only 'memory' is enabled" in definition["description"] + assert "only 'user' is enabled" not in definition["description"] def test_tool_present_by_default(self, hermes_home): """No config file at all must not strip a working tool.""" @@ -87,6 +104,42 @@ class TestBuiltinMemoryToolAvailability: {"memory": {"memory_enabled": "false", "user_profile_enabled": "false"}} ) == (False, False) + def test_schema_reuses_availability_flag_snapshot(self, monkeypatch): + """One definition pass must not reread config between check and schema.""" + from tools import memory_tool as memory_tool_module + + calls = 0 + + def _flags(): + nonlocal calls + calls += 1 + return (False, True) if calls == 1 else (True, False) + + monkeypatch.setattr(memory_tool_module, "get_builtin_memory_store_flags", _flags) + definition = _memory_tool_definition() + + assert calls == 1 + assert definition["parameters"]["properties"]["target"]["enum"] == ["user"] + + def test_unavailable_snapshot_cannot_survive_failed_recheck(self, monkeypatch): + from tools import memory_tool as memory_tool_module + + monkeypatch.setattr( + memory_tool_module, + "get_builtin_memory_store_flags", + lambda: (False, False), + ) + assert memory_tool_module.check_memory_requirements() is False + + def _boom(): + raise RuntimeError("config read failed") + + monkeypatch.setattr(memory_tool_module, "get_builtin_memory_store_flags", _boom) + with pytest.raises(RuntimeError, match="config read failed"): + memory_tool_module.check_memory_requirements() + + assert memory_tool_module._memory_surface_flags.get() is None + def test_config_flip_updates_tool_without_manual_cache_clear(self, hermes_home): _write_memory_config( hermes_home, memory_enabled=False, user_profile_enabled=False @@ -110,6 +163,63 @@ class TestBuiltinMemoryToolAvailability: ) assert memory_tool_module.check_memory_requirements() is True + def test_malformed_memory_section_normalizes_to_defaults(self): + from tools.memory_tool import ( + get_builtin_memory_config, + get_builtin_memory_store_flags, + ) + + config = {"memory": "not-a-mapping"} + assert get_builtin_memory_config(config) == {} + assert get_builtin_memory_store_flags(config) == (True, True) + + +class TestIndependentStoreWriteGates: + def _store(self, *, memory_enabled, user_profile_enabled): + from tools.memory_tool import MemoryStore + + store = MemoryStore( + memory_char_limit=500, + user_char_limit=500, + memory_enabled=memory_enabled, + user_profile_enabled=user_profile_enabled, + ) + store.load_from_disk() + return store + + def test_user_profile_only_rejects_memory_write(self, hermes_home): + from tools.memory_tool import memory_tool + + store = self._store(memory_enabled=False, user_profile_enabled=True) + denied = json.loads(memory_tool(action="add", target="memory", content="fact", store=store)) + allowed = json.loads(memory_tool(action="add", target="user", content="pref", store=store)) + + assert denied["success"] is False + assert denied["target"] == "memory" + assert allowed["success"] is True + + def test_memory_only_rejects_user_profile_write(self, hermes_home): + from tools.memory_tool import memory_tool + + store = self._store(memory_enabled=True, user_profile_enabled=False) + denied = json.loads(memory_tool(action="add", target="user", content="pref", store=store)) + allowed = json.loads(memory_tool(action="add", target="memory", content="fact", store=store)) + + assert denied["success"] is False + assert denied["target"] == "user" + assert allowed["success"] is True + + def test_staged_write_cannot_bypass_target_gate(self, hermes_home): + from tools.memory_tool import apply_memory_pending + + store = self._store(memory_enabled=False, user_profile_enabled=True) + result = apply_memory_pending( + {"action": "add", "target": "memory", "content": "fact"}, store + ) + + assert result["success"] is False + assert result["target"] == "memory" + class TestExternalProviderSurvivesBuiltinDisable: """Dropping the built-in tool must not drop the external provider's tools. diff --git a/tests/run_agent/test_run_agent.py b/tests/run_agent/test_run_agent.py index 4688e125b6..88c18fa12e 100644 --- a/tests/run_agent/test_run_agent.py +++ b/tests/run_agent/test_run_agent.py @@ -179,6 +179,37 @@ def test_direct_session_db_flushes_share_marker_claim(agent): assert db.rows == ["exactly once"] +def test_malformed_memory_config_still_builds_default_store(): + """A non-mapping memory section must not leave an advertised dead tool.""" + malformed = {"memory": "not-a-mapping"} + with ( + patch( + "hermes_cli.config.load_config_readonly", + return_value=malformed, + ), + patch( + "run_agent.get_tool_definitions", + return_value=_make_tool_defs("memory"), + ), + patch("run_agent.check_toolset_requirements", return_value={}), + patch("run_agent.OpenAI"), + ): + agent = AIAgent( + api_key="test-k...7890", + base_url="https://openrouter.ai/api/v1", + quiet_mode=True, + skip_context_files=True, + skip_memory=True, + enabled_toolsets=["memory"], + ) + + assert agent._memory_enabled is True + assert agent._user_profile_enabled is True + assert agent._memory_store is not None + assert agent._memory_store.memory_enabled is True + assert agent._memory_store.user_profile_enabled is True + + @pytest.fixture() def agent_with_memory_tool(): """Agent whose valid_tool_names includes 'memory'.""" diff --git a/tests/tools/test_write_approval.py b/tests/tools/test_write_approval.py index a0e2c572af..44db645149 100644 --- a/tests/tools/test_write_approval.py +++ b/tests/tools/test_write_approval.py @@ -108,22 +108,27 @@ def test_cli_memory_approve_without_live_agent_uses_fresh_store(hermes_home, cap assert any("remember the launch date" in e for e in reloaded.memory_entries) -def test_load_on_disk_store_honors_configured_char_limits(hermes_home, monkeypatch): - """load_on_disk_store() must read memory.memory_char_limit / - user_char_limit from config so approvals applied without a live agent - enforce the SAME caps as the live agent (agent_init.py). Falls back to - defaults when config can't be loaded. - """ +def test_load_on_disk_store_honors_configured_limits_and_permissions(hermes_home, monkeypatch): + """Fresh approval stores must match the live agent's limits and target gates.""" from tools.memory_tool import load_on_disk_store - # Config override path: helper picks up the configured limits. + # Config override path: helper picks up configured limits and store flags. monkeypatch.setattr( "hermes_cli.config.load_config", - lambda: {"memory": {"memory_char_limit": 999, "user_char_limit": 444}}, + lambda: { + "memory": { + "memory_char_limit": 999, + "user_char_limit": 444, + "memory_enabled": False, + "user_profile_enabled": True, + } + }, ) store = load_on_disk_store() assert store.memory_char_limit == 999 assert store.user_char_limit == 444 + assert store.memory_enabled is False + assert store.user_profile_enabled is True # Failure path: config raises → defaults, never blows up. def _boom(): @@ -133,6 +138,8 @@ def test_load_on_disk_store_honors_configured_char_limits(hermes_home, monkeypat fallback = load_on_disk_store() assert fallback.memory_char_limit == 2200 assert fallback.user_char_limit == 1375 + assert fallback.memory_enabled is True + assert fallback.user_profile_enabled is True # --------------------------------------------------------------------------- diff --git a/tools/memory_tool.py b/tools/memory_tool.py index 226ca67fae..4832bcd4e9 100644 --- a/tools/memory_tool.py +++ b/tools/memory_tool.py @@ -23,10 +23,12 @@ Design: - Frozen snapshot pattern: system prompt is stable, tool responses show live state """ +import copy import json import logging import time from contextlib import contextmanager +from contextvars import ContextVar from pathlib import Path from hermes_constants import get_hermes_home from typing import Dict, Any, List, Optional, Tuple @@ -47,6 +49,14 @@ except ImportError: logger = logging.getLogger(__name__) +# One tool-definition pass must use one config decision for both availability +# and the dynamic target schema. ContextVar keeps concurrent profile/session +# builds isolated while allowing the check_fn result to flow to the immediately +# following dynamic_schema_overrides call in ToolRegistry.get_definitions(). +_memory_surface_flags: ContextVar[Optional[Tuple[bool, bool]]] = ContextVar( + "memory_surface_flags", default=None +) + # Where memory files live — resolved dynamically so profile overrides # (HERMES_HOME env var changes) are always respected. The old module-level # constant was cached at import time and could go stale if a profile switch @@ -163,17 +173,30 @@ class MemoryStore: # turn to budget exhaustion and suppress the user's reply (issue #42405). _MAX_CONSOLIDATION_FAILURES_PER_TURN = 3 - def __init__(self, memory_char_limit: int = 2200, user_char_limit: int = 1375): + def __init__( + self, + memory_char_limit: int = 2200, + user_char_limit: int = 1375, + *, + memory_enabled: bool = True, + user_profile_enabled: bool = True, + ): self.memory_entries: List[str] = [] self.user_entries: List[str] = [] self.memory_char_limit = memory_char_limit self.user_char_limit = user_char_limit + self.memory_enabled = memory_enabled + self.user_profile_enabled = user_profile_enabled # Frozen snapshot for system prompt -- set once at load_from_disk() self._system_prompt_snapshot: Dict[str, str] = {"memory": "", "user": ""} # Per-turn counter of failed at-capacity consolidation attempts; reset # at each turn boundary by reset_consolidation_failures() (#42405). self._consolidation_failures = 0 + def target_enabled(self, target: str) -> bool: + """Return whether this session's selected built-in store is writable.""" + return self.user_profile_enabled if target == "user" else self.memory_enabled + def reset_consolidation_failures(self) -> None: """Reset the per-turn consolidation-failure counter (call at turn start).""" self._consolidation_failures = 0 @@ -900,10 +923,14 @@ def load_on_disk_store() -> "MemoryStore": """ memory_char_limit = 2200 user_char_limit = 1375 + memory_enabled = True + user_profile_enabled = True try: from hermes_cli.config import load_config - mem_cfg = (load_config() or {}).get("memory", {}) or {} + config = load_config() or {} + mem_cfg = get_builtin_memory_config(config) + memory_enabled, user_profile_enabled = get_builtin_memory_store_flags(config) memory_char_limit = int(mem_cfg.get("memory_char_limit", memory_char_limit)) user_char_limit = int(mem_cfg.get("user_char_limit", user_char_limit)) except Exception: @@ -912,6 +939,8 @@ def load_on_disk_store() -> "MemoryStore": store = MemoryStore( memory_char_limit=memory_char_limit, user_char_limit=user_char_limit, + memory_enabled=memory_enabled, + user_profile_enabled=user_profile_enabled, ) store.load_from_disk() return store @@ -1093,8 +1122,9 @@ def memory_tool( if target is None: target = "memory" - if target not in {"memory", "user"}: - return tool_error(f"Invalid target '{target}'. Use 'memory' or 'user'.", success=False) + target_error = _memory_target_error(store, target) + if target_error is not None: + return json.dumps(target_error) # --- Batch path ------------------------------------------------------- if operations: @@ -1144,12 +1174,13 @@ def memory_tool( return json.dumps(result, ensure_ascii=False) -def get_builtin_memory_store_flags(config: Optional[Dict[str, Any]] = None) -> Tuple[bool, bool]: - """Return ``(memory_enabled, user_profile_enabled)`` from resolved config. +def get_builtin_memory_config(config: Optional[Dict[str, Any]] = None) -> Dict[str, Any]: + """Return a normalized built-in memory config mapping. - ``agent_init`` uses this same predicate to decide whether to construct the - ``MemoryStore`` that backs the tool. Missing or unreadable config fails open - so availability checks do not remove a tool that would otherwise work. + Missing, unreadable, or malformed sections become an empty mapping, whose + missing flags resolve to the enabled defaults. ``agent_init`` consumes this + same normalized section so tool availability and store construction cannot + diverge. """ if config is None: try: @@ -1158,27 +1189,42 @@ def get_builtin_memory_store_flags(config: Optional[Dict[str, Any]] = None) -> T config = load_config_readonly() except Exception: logger.debug("Could not read memory config for availability", exc_info=True) - return True, True + return {} section = config.get("memory") if isinstance(config, dict) else None - if not isinstance(section, dict): - return True, True + return section if isinstance(section, dict) else {} + + +def get_builtin_memory_store_flags(config: Optional[Dict[str, Any]] = None) -> Tuple[bool, bool]: + """Return ``(memory_enabled, user_profile_enabled)`` from resolved config.""" + section = get_builtin_memory_config(config) return ( is_truthy_value(section.get("memory_enabled"), default=True), is_truthy_value(section.get("user_profile_enabled"), default=True), ) -def builtin_memory_stores_enabled() -> bool: - """Return whether either built-in store (MEMORY.md / USER.md) is enabled.""" - memory_enabled, user_profile_enabled = get_builtin_memory_store_flags() - return memory_enabled or user_profile_enabled - - @no_cache_check_fn def check_memory_requirements() -> bool: - """Available unless both built-in memory stores are disabled in config.""" - return builtin_memory_stores_enabled() + """Snapshot store flags and report whether the built-in tool is available.""" + _memory_surface_flags.set(None) + flags = get_builtin_memory_store_flags() + _memory_surface_flags.set(flags) + return flags[0] or flags[1] + + +def _memory_target_error(store: "MemoryStore", target: str) -> Optional[Dict[str, Any]]: + """Return a shared validation error for an invalid or disabled target.""" + if target not in {"memory", "user"}: + return {"success": False, "error": f"Invalid memory target '{target}'."} + if store.target_enabled(target): + return None + label = "USER.md" if target == "user" else "MEMORY.md" + return { + "success": False, + "error": f"Built-in {label} writes are disabled in memory config.", + "target": target, + } def apply_memory_pending(payload: Dict[str, Any], store: "MemoryStore") -> Dict[str, Any]: @@ -1189,6 +1235,9 @@ def apply_memory_pending(payload: Dict[str, Any], store: "MemoryStore") -> Dict[ """ action = payload.get("action") target = payload.get("target", "memory") + target_error = _memory_target_error(store, target) + if target_error is not None: + return target_error content = payload.get("content") or "" old_text = payload.get("old_text") or "" if action == "batch": @@ -1276,6 +1325,43 @@ MEMORY_SCHEMA = { } +def _build_memory_schema_overrides() -> Dict[str, Any]: + """Narrow the advertised target surface using the availability snapshot.""" + flags = _memory_surface_flags.get() + _memory_surface_flags.set(None) + if flags is None: + flags = get_builtin_memory_store_flags() + memory_enabled, user_profile_enabled = flags + targets = [] + if memory_enabled: + targets.append("memory") + if user_profile_enabled: + targets.append("user") + + parameters = copy.deepcopy(MEMORY_SCHEMA["parameters"]) + target_schema = parameters["properties"]["target"] + target_schema["enum"] = targets + + description = MEMORY_SCHEMA["description"] + if targets == ["memory"]: + target_schema["description"] = "The enabled built-in store: 'memory' for personal notes." + description = description.replace( + "TARGETS: 'user' = who the user is (name, role, preferences, style). 'memory' = your " + "notes (environment, conventions, tool quirks, lessons).", + "TARGET: only 'memory' is enabled for personal notes (environment, conventions, " + "tool quirks, lessons).", + ) + elif targets == ["user"]: + target_schema["description"] = "The enabled built-in store: 'user' for user profile." + description = description.replace( + "TARGETS: 'user' = who the user is (name, role, preferences, style). 'memory' = your " + "notes (environment, conventions, tool quirks, lessons).", + "TARGET: only 'user' is enabled for user profile facts (name, role, preferences, style).", + ) + + return {"description": description, "parameters": parameters} + + # --- Registry --- from tools.registry import registry, tool_error @@ -1293,6 +1379,7 @@ registry.register( store=kw.get("store")), check_fn=check_memory_requirements, emoji="🧠", + dynamic_schema_overrides=_build_memory_schema_overrides, ) diff --git a/tools/registry.py b/tools/registry.py index 472a96f41d..16fbc071a8 100644 --- a/tools/registry.py +++ b/tools/registry.py @@ -327,31 +327,29 @@ def check_fn_cache_scope() -> Optional[str]: return CHECK_FN_CACHE_BYPASS +def _run_check_fn_uncached(fn: Callable, *, unresolved_scope: bool = False) -> bool: + """Run an availability check without cache/grace handling.""" + try: + return bool(fn()) + except Exception: + detail = " while profile cache scope was unresolved" if unresolved_scope else "" + logger.warning( + "check_fn %s raised%s; dependent tools will be unavailable this turn", + getattr(fn, "__qualname__", fn), + detail, + exc_info=True, + ) + return False + + def _check_fn_cached(fn: Callable) -> bool: """Return bool(fn()), TTL-cached across calls.""" now = time.monotonic() if fn in _NO_CACHE_CHECK_FNS: - try: - return bool(fn()) - except Exception: - logger.warning( - "check_fn %s raised; dependent tools will be unavailable this turn", - getattr(fn, "__qualname__", fn), - exc_info=True, - ) - return False + return _run_check_fn_uncached(fn) scope = check_fn_cache_scope() if scope == CHECK_FN_CACHE_BYPASS: - try: - return bool(fn()) - except Exception: - logger.warning( - "check_fn %s raised while profile cache scope was unresolved; " - "dependent tools will be unavailable this turn", - getattr(fn, "__qualname__", fn), - exc_info=True, - ) - return False + return _run_check_fn_uncached(fn, unresolved_scope=True) cache_key = (fn, scope) with _check_fn_cache_lock: _prune_check_fn_caches(now) diff --git a/website/docs/user-guide/features/memory.md b/website/docs/user-guide/features/memory.md index 11a1f2376f..205f3e493b 100644 --- a/website/docs/user-guide/features/memory.md +++ b/website/docs/user-guide/features/memory.md @@ -251,8 +251,9 @@ external provider tools too. With only `memory_enabled: false` (user profile still on), the tool stays — it backs the profile store — but the system prompt swaps the full memory -guidance for a narrower profile-only block, so the model is only instructed to -save user-profile facts and never steered at the disabled notes store. +guidance for a narrower profile-only block. The tool schema advertises only the +`user` target, and direct or staged writes to disabled `MEMORY.md` are rejected. +The inverse configuration advertises only `memory` and rejects `USER.md` writes. ## Controlling memory writes (`write_approval`)