fix(memory): prevent dead writes when memory_enabled is false (v0.15.1) - #30814
fix(memory): prevent dead writes when memory_enabled is false (v0.15.1)#30814Burgunthy wants to merge 1 commit into
Conversation
db01fd0 to
8a7383c
Compare
When memory_enabled is false but user_profile_enabled is true, the MemoryStore object is still created (OR logic in agent_init). This causes memory(target="memory") to silently succeed writing to MEMORY.md, but the data is never injected into the prompt — a dead write that misleads the agent into believing its writes are durable. Fix (rebuilt on v0.15.1 main): - Guard both dispatch paths (tool_executor.py sequential + agent_runtime_helpers.py invoke_tool) to reject target="memory" when memory_enabled is false. - Gate MEMORY_GUIDANCE injection in system_prompt.py on memory_enabled so the agent is not told to use a disabled feature. - Use getattr() defensive access for _memory_enabled in system_prompt.py. - 9 new tests covering all enabled/disabled combinations. - 88 existing memory tests pass with no regression. Related NousResearch#11693, Related NousResearch#28796, See also NousResearch#29020
8a7383c to
16ee1b8
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying a real configuration combination: current main still creates a MemoryStore when user_profile_enabled is true (agent/agent_init.py:1369-1375) but injects its memory target only when _memory_enabled is true (agent/system_prompt.py:460-469). Both current dispatch paths still call memory_tool without that guard (agent/tool_executor.py:1284-1296, agent/agent_runtime_helpers.py:2281-2293).
Problems
- The new raw
target == "memory"checks misstarget: null;memory_tool()normalizes null to"memory"attools/memory_tool.py:980-984, so this still produces a dead write. Cover null in both paths. getattr(agent, "_memory_enabled", False)breaks the existingskip_memory=Truememory-tool fixture, which still expectsMEMORY_GUIDANCE(tests/run_agent/test_run_agent.py:1258-1262).build_system_prompt_parts()returns a dict (agent/system_prompt.py:145-159), so the new prompt test joins dict keys; the two disabled cases useassert True.
Suggested changes
- Normalize the effective target before guarding, preserve the established absent-attribute prompt behavior, and assert
MEMORY_GUIDANCEdirectly inparts["stable"].
Automated hermes-sweeper review.
| store=agent._memory_store, | ||
| ) | ||
| # Guard: reject target="memory" writes when memory is disabled. | ||
| # See matching guard in agent_runtime_helpers.py:invoke_tool. |
There was a problem hiding this comment.
memory_tool() treats target: null as the default "memory" target (tools/memory_tool.py:980-984), but this raw equality check does not. Normalize None before this guard and add a null-target regression; otherwise a model can still make the disabled dead write.
| # Tool-aware behavioral guidance: only inject when the tools are loaded | ||
| tool_guidance = [] | ||
| if "memory" in agent.valid_tool_names: | ||
| if "memory" in agent.valid_tool_names and getattr(agent, "_memory_enabled", False): |
There was a problem hiding this comment.
Using False as the absent-attribute default breaks the existing skip_memory=True fixture that deliberately exposes the memory tool and expects MEMORY_GUIDANCE (tests/run_agent/test_run_agent.py:1258-1262). Preserve that compatibility behavior while testing an explicitly false _memory_enabled flag.
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Two PRs address the dead-write path when memory_enabled is false; #30814 and #34414 carry the same diff, adding guards to both dispatch paths and gating memory guidance, but both miss the explicit target: null case that the underlying memory tool normalizes to memory. Their prompt tests also do not actually verify guidance presence or absence, and the _memory_enabled fallback conflicts with an existing skip_memory=True fixture expectation.
Related pull requests
- #30814
related— (+246/-9) — keep open, revisions required: The diff addresses the reported cause in both sequential and concurrent dispatch and suppresses misleading guidance, but the contributor keep_open review on #30814 identifies concrete gaps:target: nullbypasses both guards, the fallback changes existing fixture behavior, and two prompt tests are no-op assertions over incorrectly joined dict keys. Merge only after those points are addressed and tested. - #34414 [closed]
duplicate— (+246/-9) — closed duplicate, still relevant as exact-diff evidence: It implements the same guards, guidance gate, and tests as #30814, so it has the same null-target, compatibility, and ineffective-test deficiencies; the author closed it specifically to update the original PR instead.
Duplicates
#34414 is an exact duplicate of #30814 in the supplied diff and was closed by its author in favor of updating #30814.
Suggested consolidation
Merge #30814 only after revising it to normalize the effective target before guarding, preserve the documented existing fixture behavior, and replace the no-op prompt assertions with checks against the returned structure. This follows, rather than overrides, the contributor keep_open review on #30814; keep #34414 closed as the duplicate superseded by #30814.
Complex graph
flowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
subgraph Dup30814 ["PRs duplicating each other"]
P30814["PR #30814 (open)"]
P34414["PR #34414 (closed)"]
end
class P30814 open
class P34414 closed
class P30814 target
click P30814 "https://github.com/NousResearch/hermes-agent/pull/30814"
click P34414 "https://github.com/NousResearch/hermes-agent/pull/34414"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).
Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 26 kB of PR diffs, 4 kB of issue/PR text, 1 kB of discussion (2 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
…-in tool on the store predicate Rebased onto current main (was 068f587, ~1400 commits behind) and renumbered the config migration from v35 to v38: main had since assigned v35 (background process notifications), v36 (delegation max_iterations 50→250), and v37 (delegation concurrency 3→10). Keeps main's ladder intact and appends the memory rename as the newest slot; DEFAULT_CONFIG._config_version bumped 37 → 38 so the migration actually fires for pre-rename configs. Addresses AI-review feedback on the original submission (comment 1. Migration dead-code risk (version not bumped): fixed — _migrate_to_38 registered and _config_version bumped to 38. Verified end-to-end: a v37 config carrying memory.memory_enabled is rewritten to builtin_enabled and stamped v38. 2. Remaining merged-path readers of the legacy key: verified none — the only runtime consumers read through builtin_memory_enabled() (which reads raw config), the agent._memory_enabled attribute set from it, or the migration itself. 3. Absent read-path alias: added — `hermes config get memory.memory_enabled` now resolves the persisted legacy value pre-migration, then falls back to the canonical memory.builtin_enabled post-migration (mirrors the existing write-path alias in set_config_value). 4. Fail-open asymmetry: documented in builtin_memory_enabled()'s docstring — on config-read errors the tool may advertise while a provider cannot exist, until the config error clears. Original 5-layer scope unchanged (rename + migration/alias, check_fn gating, state-aware tool error, docs fixes, `hermes memory status` provider-aware state line). memory_enabled now scopes to the built-in MEMORY.md/USER.md store; the recommended provider combo (builtin_enabled: false + memory.provider) reads coherently instead of as "memory disabled" (issues NousResearch#60805, NousResearch#32624). Closes NousResearch#60805, NousResearch#32624. References NousResearch#30814, NousResearch#50076, NousResearch#45548, NousResearch#5544.
Problem
When
memory_enabled: false, thememorytool is still exposed in the tool list. The model sees it and calls it — writing data that is never injected into the system prompt. The tool returns{"success": true}, so the agent believes the write succeeded. The data is effectively lost.The
memory_enabledsetting does not actually disable memory. It just stops reading it.Reproduction
memory_enabled: false(withuser_profile_enabled: trueso agent still has memory tool)memory(target="memory", action="add", content="important info"){"success": true}Fix
Three guards that make
memory_enabled: falseactually disable writes:tool_executor.py(sequential path) — rejecttarget="memory"when disabledagent_runtime_helpers.py(invoke_toolpath) — same guard for concurrent pathsystem_prompt.py— suppress MEMORY_GUIDANCE instructions when disabledAll guards check
agent._memory_enabledwithgetattr(..., False)for backward compatibility.Testing
target="user"as alternativeBuilt on v0.15.1 main (rebuilt after Velocity refactor).
Related