Skip to content

fix(cli): clarify memory status label to avoid built-in store confusion - #18491

Closed
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:fix/issue-18404-memory-status-confusing-text
Closed

fix(cli): clarify memory status label to avoid built-in store confusion#18491
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:fix/issue-18404-memory-status-confusing-text

Conversation

@liuhao1024

@liuhao1024 liuhao1024 commented May 1, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Change the misleading hermes memory status label from "Built-in: always active" to "Memory subsystem: active" to distinguish the memory framework/injection pipeline from the built-in storage backend.

Root Cause

hermes_cli/memory_setup.py:cmd_status() unconditionally printed Built-in: always active, which users interpreted as the built-in storage backend still running alongside their configured external provider (e.g. mnemosyne, honcho). This caused confusion when troubleshooting memory duplication — users thought hermes tools disable memory didn't take effect.

The "built-in" label actually refers to the memory subsystem framework (the pipeline that loads providers and injects context into prompts), not the storage backend. When a non-built-in provider is configured, memories go to that provider, not the built-in store.

Related Issue

N/A

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • See commit messages for detailed changes

How to Test

  1. Run pytest tests/ -q — all tests should pass
  2. Verify the specific scenario described above is resolved

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 26.4.1

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture and workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard tool/memory Memory tool and memory providers labels May 1, 2026
@Tranquil-Flow

Copy link
Copy Markdown
Contributor

Hi @liuhao1024 — small thing on the new test in tests/hermes_cli/test_memory_status.py. The assertion assert "active" in out.lower() would still pass against the old output ("Built-in: always active") because that string also contains "active" — so a regression that reverted the label change would slip past silently.

Suggest tightening to something like:

assert "Memory subsystem: active" in out
assert "Built-in" not in out

That way the test actually guards the new label rather than just checking for any substring containing "active".

liuhao1024 added a commit to liuhao1024/hermes-agent that referenced this pull request May 2, 2026
Add the two commit emails used by liuhao1024:
- liuhao1024@users.noreply.github.com (GitHub noreply)
- sunsky.lau@gmail.com (personal)

These were flagged by the Contributor Attribution Check CI on PRs NousResearch#18734,
NousResearch#18491, NousResearch#18009, and NousResearch#17761.
@liuhao1024

Copy link
Copy Markdown
Contributor Author

@Tranquil-Flow Good catch — the old assert "active" in out.lower() would pass against "Built-in: always active" too, so a regression that reverted the label change would slip past silently.

Fixed: replaced with assert "Memory subsystem: active" in out which locks in the exact label+status combination. The negative guard (assert "Built-in" not in out) was already there.

Pushed: 6327687c3.

@teknium1 teknium1 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.

Thanks for addressing a real source of confusion. The current status line is indeed unconditional, but the proposed replacement does not report the state that matters to the issue.

Problems

  • The changed Memory subsystem: active line remains unconditional (hermes_cli/memory_setup.py:396 in this PR). Current runtime loads MEMORY.md/USER.md only when memory.memory_enabled or memory.user_profile_enabled is true (agent/agent_init.py:1333-1345), including the disabled-built-in setup described by issue #18404.
  • The new tests require that Built-in never appear, although the documented enabled configuration keeps built-in MEMORY.md/USER.md active alongside an external provider (website/docs/user-guide/features/memory-providers.md:9).

Suggested changes

  • Derive the displayed built-in-store state from the two memory configuration flags, and test both enabled and disabled states with an external provider.
  • Preserve a built-in-store label when it is enabled; avoid an unconditional replacement label.

Automated hermes-sweeper review.

Comment thread hermes_cli/memory_setup.py Outdated
@@ -393,7 +393,7 @@ def cmd_status(args) -> None:
provider_name = mem_config.get("provider", "")

print(f"\nMemory status\n" + "─" * 40)
print(f" Built-in: always active")
print(f" Memory subsystem: active")

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.

agent/agent_init.py:1333-1345 only loads the built-in MEMORY.md/USER.md store when memory_enabled or user_profile_enabled is true. This unconditional active label remains wrong for the disabled-built-in configuration in #18404; please derive and report the built-in state from those flags instead.

Addresses teknium1's review on PR NousResearch#18491:
- The status message should reflect actual built-in store state,
  not be unconditional.
- When memory.memory_enabled or memory.user_profile_enabled is true,
  show 'Built-in store: active'. Otherwise show 'Built-in store: disabled'.
- This correctly reflects the runtime behavior: MEMORY.md/USER.md are
  loaded only when those flags are true (agent/agent_init.py:1350-1360).

Tests added:
- test_builtin_store_disabled_by_default
- test_builtin_store_active_when_memory_enabled
- test_builtin_store_active_when_profile_enabled
- test_provider_with_builtin_store_active
- test_provider_with_builtin_store_disabled

Fixes NousResearch#18404 (original issue) and addresses review feedback.
@liuhao1024
liuhao1024 force-pushed the fix/issue-18404-memory-status-confusing-text branch from 6327687 to 959c57f Compare July 12, 2026 21:09
@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users area/memory Memory subsystem: store, providers, sync, background reviews labels Jul 12, 2026
@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Merged via #70065 which uses a more thorough approach from #23630 by @thirstycrow. Your PR was submitted first and your test approach was adapted. Both contributors credited. Thanks!

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

Labels

area/memory Memory subsystem: store, providers, sync, background reviews comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users 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