fix: respect memory_enabled flag for MEMORY_GUIDANCE injection - #36085
fix: respect memory_enabled flag for MEMORY_GUIDANCE injection#36085kjames2001 wants to merge 1 commit into
Conversation
The stable tier injected MEMORY_GUIDANCE whenever the memory tool was registered, ignoring the memory_enabled config flag. This caused contradictory instructions when users set memory_enabled=false (e.g. with third-party memory providers like MemPalace) — the system prompt told the agent to 'Save durable facts using the memory tool' even though memory was explicitly disabled. Now checks agent._memory_enabled before injecting MEMORY_GUIDANCE, consistent with how the volatile tier already gates memory content. Closes #XXXX
|
Related to #30814 which also gates MEMORY_GUIDANCE injection (plus additional dead-write guards). This PR is a narrower subset of that fix. |
mxnstrexgl
left a comment
There was a problem hiding this comment.
LGTM — automated review passed. No security, quality, or test coverage issues detected.
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅
Review
fix: respect memory_enabled flag for MEMORY_GUIDANCE injection
Clean, well-reasoned fix. Key observations:
- Problem: The stable tier injected MEMORY_GUIDANCE based solely on
valid_tool_namesmembership, ignoring thememory_enabledconfig flag. This created contradictory instructions when users had third-party memory providers withmemory_enabled: false. - Fix: One-line change adding
getattr(agent, "_memory_enabled", True)to the MEMORY_GUIDANCE injection condition. Thegetattrwith defaultTruepreserves backward compatibility for code paths that don't initialize_memory_enabled. - Consistency: Matches how the volatile tier already checks
agent._memory_enabledbefore injecting memory content. - Tests: Updated existing test to explicitly set
_memory_enabled = True, added new test for theFalsecase.
Looks Good
- Minimal change with clear motivation
- Backward compatible
- Well-documented in PR description
Reviewed by Hermes Agent
|
Thanks for the focused fix. The premise still holds on current main: The proposed condition is narrowly scoped, preserves the missing-attribute compatibility behavior requested in the PR, and the added false-case test directly covers the regression. The related broader work in #30814 does not make this narrower prompt-guidance correction redundant. Automated hermes-sweeper review. |
|
Fixed on main via #90559 (salvage of #90413). You were the earliest submitter in this cluster (May 31) to gate MEMORY_GUIDANCE on the config flags — thank you, and credited here. The landed fix also gates the tool itself via |
Problem
The stable tier injected
MEMORY_GUIDANCEwhenever thememorytool was registered invalid_tool_names, ignoring thememory_enabledconfig flag. This caused contradictory instructions when users setmemory_enabled: false— the system prompt told the agent to "Save durable facts using the memory tool" even though memory was explicitly disabled.This is inconsistent with the volatile tier, which already checks
agent._memory_enabledbefore injecting memory content (MEMORY.md, USER.md blocks).Use Case
Users running third-party memory providers (MemPalace, Mem0, etc.) via the plugin system set
memory_enabled: falseto disable the built-in memory. The built-in MEMORY_GUIDANCE instructions contradict the plugin's own instructions, confusing the agent.Fix
Added
and getattr(agent, "_memory_enabled", True)check to the MEMORY_GUIDANCE injection inbuild_system_prompt_parts(), matching the existing pattern in the volatile tier.getattrwith defaultTruepreserves backward compatibility for code paths that don't initialize_memory_enabled.Tests
test_memory_guidance_when_memory_tool_loadedto explicitly set_memory_enabled = Truetest_no_memory_guidance_when_memory_disabledto verify the fix