Skip to content

fix(memory): enforce independent built-in store controls - #90550

Closed
kshitijk4poor wants to merge 3 commits into
NousResearch:mainfrom
kshitijk4poor:salvage/90413-memory-guidance
Closed

kshitijk4poor wants to merge 3 commits into
NousResearch:mainfrom
kshitijk4poor:salvage/90413-memory-guidance

Conversation

@kshitijk4poor

@kshitijk4poor kshitijk4poor commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Built-in memory and the user profile now behave as genuinely independent stores across tool availability, schemas, initialization, and writes.

Current main already contains the original both-disabled fix from #90413 plus profile-only USER_PROFILE_GUIDANCE from 481bc9391e. This PR adds the remaining runtime hardening.

Changes

  • Keep the built-in memory tool when either memory_enabled or user_profile_enabled is enabled; remove it only when both are off.
  • Use one normalized config/flag source for tool discovery, live-agent stores, and fresh CLI/gateway approval stores.
  • Parse quoted boolean values such as "false" through Hermes's shared truthy-value helper.
  • Reflect config changes immediately instead of retaining the generic external-probe availability TTL.
  • Narrow the model-facing target enum and description to the enabled store in one-store-only configurations.
  • Freeze per-store write permissions on MemoryStore; reject direct and staged writes to a disabled file.
  • Normalize malformed memory: sections so tool availability and store initialization cannot diverge.
  • Atomically reuse one availability snapshot for the dynamic schema, isolated per concurrent context.

Validation

  • Focused canonical suite: 257 passed, 1 skipped.
  • Real-import E2E: both-disabled removal, one-store schema narrowing, quoted booleans, malformed config, direct/staged/fresh approval write gates.
  • Ruff, Python compilation, diff check, and contributor-attribution checks passed.

Credit

Builds on HexLab98's original diagnosis and implementation in #90413. The original both-disabled behavior is already present on current main; this PR closes the residual independent-store gaps found during salvage review.

@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 comp/tools Tool registry, model_tools, toolsets tool/memory Memory tool and memory providers area/memory Memory subsystem: store, providers, sync, background reviews 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 needs-decision Awaiting maintainer decision before any implementation labels Aug 20, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Related: #90413, #53486, and #45548 address overlapping built-in-memory tool and guidance semantics through different mechanisms. This PR has a broader configuration and external-provider delta, so it is linked rather than marked duplicate.

Reuse the built-in store predicate during agent initialization and evaluate the config-backed memory tool check immediately after edits instead of applying the generic external-probe TTL.
Use Hermes's shared truthy-value parser so quoted false memory flags disable both built-in stores as expected.
Normalize malformed memory config during initialization and bind per-target write permissions to the session MemoryStore so direct and staged writes cannot update a disabled built-in store.
@kshitijk4poor
kshitijk4poor force-pushed the salvage/90413-memory-guidance branch from e106b0a to 269ebeb Compare August 20, 2026 06:29
@kshitijk4poor kshitijk4poor changed the title fix(memory): separate built-in memory and profile guidance fix(memory): enforce independent built-in store controls Aug 20, 2026

@HexLab98 HexLab98 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks right to me. The flag unification is a good catch — agent_init defaulted missing flags to False while check_memory_requirements defaulted them to True, so a failed config read still advertised the tool with store=None. The quoted-"false" fix and the per-store write gate both close real holes. One minor thing inline, non-blocking.

Comment thread tools/memory_tool.py
def _memory_target_error(store: "MemoryStore", target: str) -> Optional[Dict[str, Any]]:
"""Return a shared validation error for an invalid or disabled target."""
if target not in {"memory", "user"}:
return {"success": False, "error": f"Invalid memory target '{target}'."}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: this path no longer goes through tool_error(), so the error body loses _bound_error_text()’s 2048-char cap (tools/registry.py:33). target is model-supplied and interpolated directly. It also drops the "Use 'memory' or 'user'" hint that told the model how to recover.

@helix4u

helix4u commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Had a look through this against the current main behavior and the one-store-only cases. This is a solid follow-up to #90559. The shared flag handling, narrowed tool schema, and write gates fix real inconsistencies that were still left after the original issue was resolved. I also ran the new targeted coverage locally on the current head and all 20 tests passed.

HexLab's note about keeping the recovery hint and using the standard error path is worth addressing, but I don't see it as merge-blocking. This looks good to merge.

@teknium1

Copy link
Copy Markdown
Collaborator

Merged via PR #91262 with your three commits cherry-picked onto current main and your authorship preserved in git log. Also folded in HexLab98's review note on top (bounded invalid-target error + restored recovery hint). Thanks for the thorough follow-up to #90413 — the flag unification and per-store write gates closed real holes.

@teknium1 teknium1 closed this Aug 21, 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 area/memory Memory subsystem: store, providers, sync, background reviews comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/tools Tool registry, model_tools, toolsets needs-decision Awaiting maintainer decision before any implementation 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.

5 participants