diff --git a/agent/agent_init.py b/agent/agent_init.py index f982d038d1..ca5996636b 100644 --- a/agent/agent_init.py +++ b/agent/agent_init.py @@ -1815,13 +1815,17 @@ def init_agent( agent._memory_nudge_interval = 10 agent._turns_since_memory = 0 agent._iters_since_skill = 0 - # A flush/background agent may pass skip_memory=True to avoid spinning up an - # external memory *provider*, but if the caller also explicitly enables the - # "memory" toolset it still needs the built-in file-backed store — otherwise - # the memory tool dispatches with store=None and every call fails (#65429). - # So the built-in store is created unless memory is globally disabled, while - # the external-provider block below stays gated on skip_memory. - _memory_toolset_requested = "memory" in (agent.enabled_toolsets or []) + # skip_memory=True skips the external memory *provider*. Flush/background + # agents can still pass enabled_toolsets=["memory"] so the built-in file + # store exists and the memory tool does not fail with store=None (#65429). + # A toolset on disabled_toolsets is not a request. Cron always denylists + # memory, but the default cron toolset still names it, so an enabled-only + # check would load MEMORY.md into an auto-approve job. + _enabled_toolsets = agent.enabled_toolsets or [] + _disabled_toolsets = agent.disabled_toolsets or [] + _memory_toolset_requested = ( + "memory" in _enabled_toolsets and "memory" not in _disabled_toolsets + ) if not skip_memory or _memory_toolset_requested: try: from tools.memory_tool import ( diff --git a/cron/scheduler.py b/cron/scheduler.py index 53704dbcf4..59ce8fafa8 100644 --- a/cron/scheduler.py +++ b/cron/scheduler.py @@ -361,8 +361,10 @@ def _resolve_cron_disabled_toolsets(cfg: dict) -> list[str]: Three toolsets are always disabled in cron context regardless of config: - ``messaging`` — interactive, needs a live gateway session - ``clarify`` — interactive, blocks waiting for user input - - ``memory`` — cron agents are constructed with ``skip_memory=True``, so - exposing this tool only gives the model an unbacked tool that fails + - ``memory`` — cron agents run with ``skip_memory=True``. The tool is + hidden, and ``memory`` is stripped from enabled_toolsets so the + built-in store is not created either (MEMORY.md would otherwise + land in the cron system prompt). ``cronjob`` is policy-denied by default (loop prevention, not a security boundary) and config-gated: setting ``cron.allow_agent_scheduling: true`` @@ -436,6 +438,10 @@ def _resolve_cron_enabled_toolsets(job: dict, cfg: dict) -> list[str] | None: 3. ``None`` on any lookup failure — AIAgent loads the full default set (legacy behavior before this change, preserved as the safety net). + ``memory`` is always stripped. Cron denylists that toolset and passes + ``skip_memory=True``. Leaving it in enabled_toolsets still constructs + the built-in MemoryStore and injects MEMORY.md into the job prompt. + _DEFAULT_OFF_TOOLSETS ({moa, homeassistant, rl}) are removed by ``_get_platform_tools`` for unconfigured platforms, so fresh installs get cron WITHOUT ``moa`` by default (issue reported by Norbert — @@ -443,10 +449,12 @@ def _resolve_cron_enabled_toolsets(job: dict, cfg: dict) -> list[str] | None: """ per_job = job.get("enabled_toolsets") if per_job: - return _merge_mcp_into_per_job_toolsets(list(per_job), cfg or {}) + return _strip_cron_memory_toolset( + _merge_mcp_into_per_job_toolsets(list(per_job), cfg or {}) + ) try: from hermes_cli.tools_config import _get_platform_tools # lazy: avoid heavy import at cron module load - return sorted(_get_platform_tools(cfg or {}, "cron")) + return _strip_cron_memory_toolset(sorted(_get_platform_tools(cfg or {}, "cron"))) except Exception as exc: logger.warning( "Cron toolset resolution failed, falling back to full default toolset: %s", @@ -455,6 +463,17 @@ def _resolve_cron_enabled_toolsets(job: dict, cfg: dict) -> list[str] | None: return None +def _strip_cron_memory_toolset(enabled: list[str] | None) -> list[str] | None: + """Drop ``memory`` from a cron enabled-toolset list. + + ``None`` means "full default set" and is left alone. skip_memory=True plus + the memory denylist still keep the store off on that fallback path. + """ + if enabled is None: + return None + return [name for name in enabled if name != "memory"] + + def _resolve_job_reasoning_config(job: dict, cfg: dict, model: str) -> dict | None: """Resolve the effective reasoning config for a cron run. @@ -496,6 +515,7 @@ def _resolve_job_reasoning_config(job: dict, cfg: dict, model: str) -> dict | No ) return resolve_reasoning_config(cfg if isinstance(cfg, dict) else {}, str(model)) + # Valid delivery platforms — used to validate user-supplied platform names # in cron delivery targets, preventing env var enumeration via crafted names. _KNOWN_DELIVERY_PLATFORMS = frozenset({ diff --git a/tests/agent/test_skip_memory_store_65429.py b/tests/agent/test_skip_memory_store_65429.py index f6966cd423..c84dbab0da 100644 --- a/tests/agent/test_skip_memory_store_65429.py +++ b/tests/agent/test_skip_memory_store_65429.py @@ -24,7 +24,9 @@ class _FakeOpenAI: pass -def _make_agent(monkeypatch, enabled_toolsets=None, skip_memory=True): +def _make_agent( + monkeypatch, enabled_toolsets=None, disabled_toolsets=None, skip_memory=True +): monkeypatch.setattr("run_agent.get_tool_definitions", lambda **kw: []) monkeypatch.setattr("run_agent.check_toolset_requirements", lambda: {}) monkeypatch.setattr("run_agent.OpenAI", _FakeOpenAI) @@ -38,6 +40,7 @@ def _make_agent(monkeypatch, enabled_toolsets=None, skip_memory=True): skip_context_files=True, skip_memory=skip_memory, enabled_toolsets=enabled_toolsets, + disabled_toolsets=disabled_toolsets, ) @@ -96,3 +99,36 @@ def test_skip_memory_memory_tool_handler_works_and_provider_skipped( memory_md = tmp_path / "hm" / "memories" / "MEMORY.md" assert memory_md.exists() assert "User prefers concise answers." in memory_md.read_text() + + +def test_skip_memory_disabled_toolset_does_not_load_store(monkeypatch, tmp_path): + """Cron shape: skip_memory=True, memory named in enabled AND disabled. + + #65429 must not load MEMORY.md just because the default cron toolset + still lists memory while the denylist hides the tool. + """ + home = tmp_path / "hm" + monkeypatch.setenv("HERMES_HOME", str(home)) + mem_dir = home / "memories" + mem_dir.mkdir(parents=True) + secret = "cron-should-never-see-this-memory" + (mem_dir / "MEMORY.md").write_text(secret + "\n") + (mem_dir / "USER.md").write_text("cron-should-never-see-this-profile\n") + + agent = _make_agent( + monkeypatch, + enabled_toolsets=["memory", "file"], + disabled_toolsets=["memory"], + skip_memory=True, + ) + assert agent._memory_store is None + assert agent._memory_manager is None + assert agent._memory_enabled is False + assert agent._user_profile_enabled is False + + from agent.system_prompt import build_system_prompt_parts + + parts = build_system_prompt_parts(agent) + blob = " ".join(str(v) for v in parts.values()) + assert secret not in blob + assert "cron-should-never-see-this-profile" not in blob diff --git a/tests/cron/test_agent_scheduling_gate.py b/tests/cron/test_agent_scheduling_gate.py index 6e80dc6823..abbe6a9fb4 100644 --- a/tests/cron/test_agent_scheduling_gate.py +++ b/tests/cron/test_agent_scheduling_gate.py @@ -21,7 +21,7 @@ from cron.scheduler import _resolve_cron_disabled_toolsets # The toolsets that must be denied in cron context no matter what the # agent-scheduling gate says: messaging/clarify are interactive-only, -# memory is unbacked in cron runs (skip_memory=True). +# memory stays off in cron runs (skip_memory=True, toolset denylisted). ALWAYS_DISABLED = ["messaging", "clarify", "memory"] diff --git a/tests/cron/test_scheduler.py b/tests/cron/test_scheduler.py index 00db344c5e..4bfe7b27c0 100644 --- a/tests/cron/test_scheduler.py +++ b/tests/cron/test_scheduler.py @@ -118,6 +118,24 @@ class TestPerJobToolsetMcpMerge: assert m_platform.call_args[0][1] == "cron" assert set(result) == set(sentinel) + def test_resolver_strips_memory_from_per_job_list(self): + result = _resolve_cron_enabled_toolsets( + {"enabled_toolsets": ["memory", "file"]}, + {"mcp_servers": {}}, + ) + assert "memory" not in result + assert "file" in result + + def test_resolver_strips_memory_from_platform_fallback(self): + job = {"enabled_toolsets": None} + with patch( + "hermes_cli.tools_config._get_platform_tools", + return_value={"web", "memory", "file"}, + ): + result = _resolve_cron_enabled_toolsets(job, {}) + assert result == ["file", "web"] + assert "memory" not in result + class TestResolveOrigin: def test_full_origin(self): @@ -613,10 +631,9 @@ class TestRunJobSessionPersistence: def test_run_job_memory_toolset_disabled_in_cron(self, tmp_path): """memory toolset must be disabled in cron sessions — issue #38129. - Cron agents are constructed with skip_memory=True, so the memory - backend is not initialised. Exposing the memory tool only gives the - model an unbacked tool that fails at runtime with - "Memory is not available." Hiding it from the schema prevents that. + Cron agents are constructed with skip_memory=True. The memory tool + stays off the schema, and memory is stripped from enabled_toolsets + so MEMORY.md is not loaded into the job prompt. """ job = { "id": "memory-hide-job", @@ -634,10 +651,9 @@ class TestRunJobSessionPersistence: def test_run_job_disables_memory_even_when_per_job_enables_it(self, tmp_path): """Cron runs pass skip_memory=True, so memory must not be exposed. - A cron job can request the memory tool through enabled_toolsets, but - there is no MemoryStore injected for cron agents. Keep memory in the - disabled set so AIAgent filters the unbacked tool out before the model - can call it and receive "Memory is not available" failures. + A cron job can name the memory toolset in enabled_toolsets. The + resolver drops it, and the denylist still lists it, so the model + never gets the tool and init never builds MemoryStore. """ job = { "id": "memory-toolset-job", @@ -650,7 +666,8 @@ class TestRunJobSessionPersistence: kwargs = mock_agent_cls.call_args.kwargs assert kwargs["skip_memory"] is True - assert kwargs["enabled_toolsets"] == ["memory", "file"] + assert kwargs["enabled_toolsets"] == ["file"] + assert "memory" not in (kwargs["enabled_toolsets"] or []) assert "memory" in kwargs["disabled_toolsets"] def test_tick_skips_due_jobs_while_dispatch_is_paused(self, tmp_path):