Skip to content

fix(memory): drop dead memory tool and MEMORY_GUIDANCE when built-in stores are disabled - #90413

Closed
HexLab98 wants to merge 2 commits into
NousResearch:mainfrom
HexLab98:fix/builtin-memory-disabled-surface
Closed

HexLab98 wants to merge 2 commits into
NousResearch:mainfrom
HexLab98:fix/builtin-memory-disabled-surface

Conversation

@HexLab98

@HexLab98 HexLab98 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Problem

With both built-in stores off:

memory:
  memory_enabled: false
  user_profile_enabled: false
  provider: hindsight

agent_init never builds a MemoryStore (agent/agent_init.py only constructs one when memory_enabled or user_profile_enabled), yet the surface it backs stays fully wired:

  • check_memory_requirements() returned True unconditionally, so the memory tool shipped in every request's schema. Every call answers "Memory is not available. It may be disabled in config or this environment."
  • MEMORY_GUIDANCE was gated only on "memory" in agent.valid_tool_names, so the system prompt still told the model to "Save durable facts using the memory tool" — a multi-paragraph block paid for on every API call, instructing the model to use a tool that cannot work.

There was no way out via config. agent.disabled_toolsets: [memory] drops the dead tool and the guidance, but memory_provider_tools_enabled() returns False for that same list, so it takes the external provider's tools down with it. Users on a third-party backend (Hindsight, Mem0, Honcho, …) had to keep the built-in dead surface to keep their provider alive.

Fix

Two gates, one predicate — "is either built-in store actually enabled":

tools/memory_tool.pycheck_memory_requirements() now consults the config flags via a new builtin_memory_stores_enabled(). Either flag alone still backs the tool (user_profile_enabled: true keeps target="user" working), so only turning both off removes it. Reads through load_config_readonly() (mtime-cached) and fails open on any read error, so an unreadable config can never strip a working tool.

agent/system_prompt.pyMEMORY_GUIDANCE is gated on the already-resolved agent._memory_enabled / agent._user_profile_enabled, not on a fresh config read. Config is resolved once at init, so the prompt stays byte-stable for the life of the conversation and prompt caching is untouched. getattr(..., True) keeps the rare code paths that build an agent view without agent_init on the old behavior.

The external provider is deliberately left alone. Hiding the built-in tool flips memory_tool_present to False in memory_provider_tools_enabled(), which moves the decision onto the toolset gate — enabled_toolsets naming memory (or None, or a bundle resolving to it) still injects the provider's tools. disabled_toolsets: [memory] keeps its documented "hide everything" meaning. No change to that function was needed; the new tests pin the behavior so it cannot silently regress.

Docs: website/docs/user-guide/features/memory.md now states what both-flags-off does and how it differs from the disabled_toolsets switch.

Test plan

New tests/agent/test_builtin_memory_disabled_surface.py walks the real chain — config.yaml on a temp HERMES_HOMEcheck_memory_requirementsget_tool_definitions — rather than mocking the availability check, since the bug was in how the flags reach the schema:

  • both flags off → memory absent from the schema
  • either flag alone → memory present
  • no config file at all → memory present (no behavior change by default)
  • config read raises → available (fails open)
  • provider tools still enabled with memory_tool_present=False for ["memory", ...] and for None
  • disabled_toolsets: ["memory"] still suppresses provider tools
  • real inject_memory_provider_tools injects a provider tool with no built-in memory tool in the surface

tests/run_agent/test_run_agent.py: the existing test_memory_guidance_when_memory_tool_loaded built a skip_memory=True agent whose flags were both false, so it was asserting the old presence-only behavior; it now states its precondition and gains a false-case mirror plus a user-profile-only case.

scripts/run_tests.sh tests/agent/test_builtin_memory_disabled_surface.py
scripts/run_tests.sh tests/run_agent/test_run_agent.py tests/agent/test_memory_provider.py \
  tests/agent/test_prompt_builder.py tests/agent/test_skip_memory_store_65429.py \
  tests/agent/test_context_breakdown.py tests/hermes_cli/test_memory_status.py

423 passed, 0 failed. tests/agent/ and tests/cron/ are clean. The one unrelated failure I hit locally is tests/tools/test_video_generation_tool_surface_matrix.py, which fails on an unmodified main checkout after 2dea073a1c ("dispatch image/video FAL strictly on the stored hermes tools selection") and touches nothing in this diff.

Related

Narrower than #30814 (which also adds write-time guards) and overlapping the prompt half of #36085 / #45548, neither merged. This PR adds the tool-schema half and the explicit non-regression coverage for the external provider, which is what made the built-in surface unremovable in the first place.

…are off

With memory.memory_enabled and memory.user_profile_enabled both false,
agent_init never builds a MemoryStore -- but check_memory_requirements()
returned True unconditionally and MEMORY_GUIDANCE was gated only on the
tool being present in valid_tool_names. So the tool shipped in every
request's schema while answering "Memory is not available" on every call,
and the system prompt still told the model to save durable facts there.

Gate both on the config flags, using the store predicate for the tool and
the already-resolved agent state for the guidance (config is not re-read
mid-conversation, so the prompt stays byte-stable). Either flag alone
still backs the tool, so only turning both off removes it.

This lets a user running a third-party provider (Hindsight, Mem0, ...)
turn the built-in files off without paying for the dead surface on every
API call. The provider's own tools are unaffected: hiding the built-in
tool moves the decision onto the toolset gate, and listing memory under
agent.disabled_toolsets remains the only switch that takes those down.
Walks the real resolution chain -- config.yaml on a temp HERMES_HOME ->
check_memory_requirements -> get_tool_definitions -- rather than mocking
the availability check, since the bug was in how the flags reach the
schema. Covers both flags off, either one alone, no config file at all,
and a config read that raises (must fail open).

Also asserts the external provider's tools survive with the built-in tool
gone, so the fix cannot regress into taking Hindsight/Mem0 down with it,
while disabled_toolsets keeps its documented "hide everything" meaning.

The existing MEMORY_GUIDANCE test built a skip_memory agent whose flags
were both false, so it was asserting the old tool-presence-only behavior;
it now states its precondition and gains the false-case mirror.
@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint tool/memory Memory tool and memory providers area/config Config system, migrations, profiles P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 20, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Merged via #90559 — your two commits were cherry-picked onto current main with your authorship preserved in git log (merge commit 481bc93).

On top of your fix we split MEMORY_GUIDANCE per store: with memory_enabled: false but user_profile_enabled: true, a narrower profile-only guidance block is injected instead of the full memory block, addressing the follow-up raised in the support thread. Thanks for the most complete fix in this cluster — real resolution-chain tests, provider survival coverage, fail-open, and docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades tool/memory Memory tool and memory providers type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants