fix(memory): isolate long-term memory per user and per group - #52903
Jabberwocky238 wants to merge 6 commits into
Conversation
The built-in memory store wrote MEMORY.md/USER.md to a single profile-scoped
directory ($HERMES_HOME/memories/), keyed only off HERMES_HOME/profile. On
multi-user messaging platforms (WeCom with several allowed users, Telegram
group bots, …) every user therefore shared one MEMORY.md/USER.md: one user's
"remember X" bled into every other user's system-prompt snapshot — a
cross-user memory leak. The short-term transcript was already isolated per
session_key, but long-term memory was not.
get_memory_dir now resolves the bucket from the session identity threaded
into init_agent (agent._user_id / _chat_type / _chat_id):
- group chat (chat_type == "group" with a chat_id) → memories/groups/<slug>/
so all members of a shared group session (group_sessions_per_user=false)
read/write the same memory, instead of it being attributed to whichever
user happened to create the cached agent first.
- DM / single user (user_id given) → memories/<slug>/, isolated per user.
- no identity (CLI / cron / bare scripts) → memories/ (the historical,
single-user layout — byte-identical, so existing installs and every
positional reader are unaffected).
MemoryStore / load_on_disk_store forward chat_type+chat_id alongside
user_id; agent_init passes agent._chat_type/_chat_id, and the gateway
/memory slash command passes event.source.chat_type/chat_id so an approval
lands in the same per-user/per-group store the live agent uses.
user_ids are untrusted free-form strings, so _user_slug collapses every
non [A-Za-z0-9_.-] char to _ (capped at 128, sha256 fallback for
empty/traversal-shaped results) before joining into the path — a value
like ../../etc cannot traverse out of the store root.
…ch sigs get_memory_dir gained user_id/chat_type/chat_id params; the existing "lambda: tmp_path" monkeypatches had a zero-arg signature and broke under the new signature. Switch them to "lambda *a, **k: tmp_path". Add tests/tools/test_memory_per_user_group.py pinning the resolution (DM per-user, group shared by chat_id, no-identity root fallback), the traversal-hardening slug, and the cross-user isolation / group-sharing end-to-end behavior this fix is about.
…roup-bucketing # Conflicts: # scripts/release.py
|
Some related history for reviewers, since this PR changes the memory-isolation boundary. Root-cause lineage — this isn't a regression from a single PR; the built-in store has been single-bucket by design since memory was introduced:
Why it surfaced as a security issue: the gateway independently grew per-user session keys (e.g. Design-intent note (per One thing worth calling out for review: the no-identity fallback to the historical root |
|
Thanks for the thorough work here — the implementation is clean and the tests are solid. But we're going to close this. Hermes doesn't support or expect privacy isolation between users, sessions, or profiles, and the shared Where isolation IS wanted, the boundary is profiles — each profile has its own Appreciate the contribution regardless. |
What does this PR do?
The built-in long-term
MemoryStorewroteMEMORY.md/USER.mdto a single profile-scoped directory (get_hermes_home()/memories/). On multi-user messaging platforms (e.g. WeCom/Enterprise WeChat, Telegram, Discord) every end user shared one store — user A's saved preferences and facts bled into user B's context, and any user could read another's memory via/memory. This is a cross-user memory bleed.get_memory_dir()now resolves the memory bucket from the session identity:memories/<user_slug>/(one bucket per user)memories/groups/<chat_id>/(shared by all members of that group — a team's collective memory, matching the existinggroup_sessions_per_usersharing semantics)memories/The identity (
user_id,chat_type,chat_id) is threaded from the platform event throughagent_initintoMemoryStore, and the/memoryslash command reads the same bucket the agent wrote to.A filename-safe slug (
_user_slug) guards the path:..//collapse to_, length is capped at 128, and values that strip to empty fall back to a stablesha256prefix instead of resolving to the shared root (which would silently re-bleed users).This is Route A: the fix lives entirely in the built-in
tools/memory_tool.py— no new plugin, no new config keys, no behavior change for single-user / CLI use.Related Issue
Fixes #52900
Type of Change
Changes Made
tools/memory_tool.py—get_memory_dir(user_id, chat_type, chat_id)resolves per-user / per-group / root buckets; added_user_slug()path-traversal hardening;MemoryStore.__init__+load_on_disk_storeaccept and forward the identity;_path_foris now an instance method using the store's identity.agent/agent_init.py— assignagent._user_id/agent._chat_type/agent._chat_idfrom the session before constructingMemoryStore, and pass them through.gateway/slash_commands.py—/memoryreadsuser_id/chat_type/chat_idoff the event source so it operates on the caller's own bucket.scripts/release.py— added the contributor email → username mapping for AUTHOR_MAP attribution.tests/tools/test_memory_per_user_group.py— new: 16 tests covering bucket resolution, slug hardening, store plumbing, and end-to-end cross-user isolation (the bug this fixes).tests/tools/test_memory_tool.py,tests/tools/test_memory_tool_import_fallback.py— updatedget_hermes_homemonkeypatch signatures to accept the new args.How to Test
.venv/bin/python -m pytest tests/tools/test_memory_per_user_group.py tests/tools/test_memory_tool.py tests/tools/test_memory_tool_import_fallback.py -q→93 passed.main, then confirm the fix: twoMemoryStoreinstances with differentuser_idmust not see each other's entries (tests/tools/test_memory_per_user_group.py::TestCrossUserIsolation::test_one_user_cannot_read_another).chat_id+chat_type="group"resolve to the same bucket (test_group_members_share).user_id="../../etc/passwd"stays undermemories/(TestUserSlug)..venv/bin/ruff check tools/memory_tool.py agent/agent_init.py gateway/slash_commands.py→ clean.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass (only memory-related subset run locally; 93 passed)Documentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/A (no config keys added/changed)CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/Apathlib-based, slug uses[^A-Za-z0-9_.-]— platform-agnostic)For New Skills
(N/A — not a skill)
Screenshots / Logs