Skip to content

fix(memory): hide built-in memory tool when both memory stores are disabled - #86540

Closed
rumotoshino wants to merge 1 commit into
NousResearch:mainfrom
rumotoshino:fix/memory-tool-config-gate
Closed

rumotoshino wants to merge 1 commit into
NousResearch:mainfrom
rumotoshino:fix/memory-tool-config-gate

Conversation

@rumotoshino

Copy link
Copy Markdown

Problem

When a profile disables both built-in memory stores (memory.memory_enabled: false and memory.user_profile_enabled: false) — the Honcho-primary setup — agent_init never constructs a MemoryStore, yet the built-in memory tool schema is still advertised to the model. Every call to it then fails with Memory is not available, wasting a tool round-trip and confusing the model into retrying.

Fix

Gate the built-in memory tool on the active profile's config:

  • check_memory_requirements() returns False only when both built-in stores are disabled; external provider tools (e.g. honcho_*) are injected separately and are unaffected.
  • get_tool_definitions() discards memory from the resolved toolset under the same condition.
  • Fails open: DEFAULT_CONFIG enables both stores, and any config-read error keeps the tool available, so normal installs are untouched.
  • load_config is imported lazily at call time so tests can monkeypatch it.

Test plan

pytest tests/tools/test_memory_tool.py -q   # 43 passed (6 new gate tests)
pytest tests/test_model_tools.py -q         # all pass
ruff check <touched files>                  # clean

Verified against current main (50d98fc1f).

…sabled

When a profile disables both built-in memory stores (memory.memory_enabled:
false and memory.user_profile_enabled: false) — the Honcho-primary setup —
agent_init never constructs a MemoryStore, yet the built-in `memory` tool
schema is still advertised. Every call to it then fails with
"Memory is not available", wasting a tool round-trip and confusing the model.

Gate the tool on the active profile's config:

- check_memory_requirements() returns False only when BOTH built-in stores
  are disabled; external provider tools (honcho_*) are injected separately
  and are unaffected. Fails open: DEFAULT_CONFIG enables both stores, and
  any config-read error keeps the tool available, so normal installs are
  untouched.
- get_tool_definitions() discards "memory" from the resolved toolset under
  the same condition.
- load_config is imported lazily at call time so tests can monkeypatch it.

Test plan:
  pytest tests/tools/test_memory_tool.py -q   # 43 passed
  pytest tests/test_model_tools.py -q         # included, all pass
  ruff check tools/memory_tool.py model_tools.py tests/tools/test_memory_tool.py  # clean
@alt-glitch alt-glitch added type/bug Something isn't working comp/tools Tool registry, model_tools, toolsets tool/memory Memory tool and memory providers area/config Config system, migrations, profiles P3 Low — cosmetic, nice to have labels Aug 15, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(memory): hide built-in memory tool when both memory stores are disabled

  1. Duplicated gate logic: model_tools.py::_builtin_memory_tools_disabled() and tools/memory_tool.py::check_memory_requirements() implement the same config predicate in two places and can drift (e.g. if a third store flag is added to one but not the other). Consider a single source of truth — e.g. _compute_tool_definitions delegating to check_memory_requirements() (or a shared helper) rather than re-deriving the config state.

  2. Two config reads per tool-definition rebuild: the registry's check_fn path (TTL-cached) plus the new load_config() call inside _compute_tool_definitions on every rebuild. If load_config is uncached, that is repeated YAML parsing for the same decision; if it is cached, the two paths can disagree on staleness timing. Worth a brief comment on which one is authoritative.

  3. Minor: bool(mem_cfg.get("memory_enabled", True) or mem_cfg.get("user_profile_enabled", True)) fails open on truthy non-bool values — e.g. a hand-written config with the string "false" would keep the tool visible. PyYAML parses real YAML booleans so this is an edge case only, but a strict is False check would match the model_tools version's semantics exactly.

@teknium1

Copy link
Copy Markdown
Collaborator

Fixed on main via #90559 (salvage of #90413) — the same both-stores-off gate via check_memory_requirements(), plus the MEMORY_GUIDANCE drop and provider-survival coverage. Yours was one of three near-identical fixes for this (submitted Aug 15); #90413 was salvaged as the most complete of the set, but the analysis in all three matched. Thanks for the contribution; closing as covered on main.

@teknium1 teknium1 closed this Aug 20, 2026
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/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have 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.

4 participants