fix(memory): stop copying the conversation into memory; let recall answer with memories - #5315
Open
yh928 wants to merge 3 commits into
Open
fix(memory): stop copying the conversation into memory; let recall answer with memories#5315yh928 wants to merge 3 commits into
yh928 wants to merge 3 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change stops session and channel turns from copying raw conversation text into memory. Namespace recall filters legacy verbatim conversation copies while retaining extracted memories and unrelated global memories. Tests cover autosave, session exclusion, and legacy-row filtering. ChangesConversation memory deduplication
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
agent
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/channels/tests/memory.rs`:
- Around line 201-209: Update the test around the first channel-processing call
to assert that runtime_ctx.memory.count().await is zero before clearing
in-process history, directly verifying process_channel_message does not store
the raw inbound message. Keep the existing recall assertion afterward to
preserve coverage that prior messages are not replayed from memory.
In `@src/openhuman/memory_store/namespace_store/query_tests.rs`:
- Around line 1013-1156: Add a regression test alongside the existing recall
tests that uses UnifiedMemory::recall_namespace_memories("") instead of
query_namespace_ranked. Insert legacy conversation rows, including an
assistant_resp entry, plus a global memory with category Core, then assert the
legacy rows are absent while the Core memory remains available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cf982dbe-9034-4780-8f7f-82fd94591bbd
📒 Files selected for processing (8)
src/openhuman/agent/harness/session/turn/core.rssrc/openhuman/agent/tests.rssrc/openhuman/channels/context.rssrc/openhuman/channels/runtime/dispatch/processor.rssrc/openhuman/channels/tests/memory.rssrc/openhuman/memory_store/memory_trait.rssrc/openhuman/memory_store/namespace_store/query.rssrc/openhuman/memory_store/namespace_store/query_tests.rs
💤 Files with no reviewable changes (1)
- src/openhuman/channels/context.rs
|
| Filename | Overview |
|---|---|
| src/openhuman/memory/store/namespace_store/query.rs | Core of the fix: adds drop_verbatim_conversation_copies and applies it in both the query-with-session-exclusion and query-less recall paths; filter is correctly scoped to the global namespace so extracted conversation memories in dedicated namespaces are untouched. |
| src/openhuman/agent/harness/session/turn/core.rs | Removes the fire-and-forget user-message write and the synchronous assistant-reply snapshot; replaces both with explanatory comments. Clean deletion, no lingering writes. |
| src/openhuman/channels/runtime/dispatch/processor.rs | Removes the channel-message autosave (and notes the previously missing session_id tagging as an additional fix). Import of conversation_memory_key cleaned up. |
| src/openhuman/memory/store/namespace_store/query_tests.rs | Adds five targeted tests covering all three legacy document shapes plus the over-filtering guard; retargets session-exclusion fixtures from global to conversation_memory namespace to match where transcript_ingest actually writes. |
| src/openhuman/channels/tests/memory.rs | Removes now-deleted autosave tests; renames and inverts the key test to assert the message does NOT replay out of memory; changes a test fixture category from Conversation to Core to avoid the new global-namespace filter. |
| src/openhuman/memory/store/memory_trait.rs | Updates session-exclusion tests to use conversation_memory namespace instead of global; updates assertion message to reference transcript_ingest rather than the removed autosave. |
| src/openhuman/channels/context.rs | Deletes conversation_memory_key helper function and its one test; conversation_history_key is preserved and unaffected. |
| src/openhuman/agent/tests.rs | Renames and inverts auto_save_stores_messages_in_memory to assert zero writes happen; uses the same 25-iteration polling pattern to catch any stray fire-and-forget writes that might arrive late. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph Before["Before (writes raw chat to memory)"]
A1[User message] -->|fire-and-forget spawn| W1["memory.store() → user_msg:{uuid}\ncategory=Conversation, global ns"]
A2[Agent turn ends] -->|sync write| W2["memory.store() → assistant_resp\ncategory=Daily, global ns\n(fixed key: overwrites every turn)"]
A3[Channel message] -->|sync write| W3["memory.store() → {ch}_{sender}_{id}\ncategory=Conversation, global ns\n(session_id=None: no self-echo guard)"]
W1 & W2 & W3 -->|pollute| VEC["Vector search space\n(fact recall)"]
end
subgraph After["After (writes removed; filter cleans up legacy rows)"]
B1[User message] --> T1["Transcript\n(memory_conversations)"]
B2[Channel message] --> T1
B3[Agent turn] --> T1
T1 -->|reads| TS["transcript_search"]
MEM["memory_docs\n(facts only)"] -->|query path| F["drop_verbatim_conversation_copies\n(global ns only)\n• drops category=conversation\n• drops key=assistant_resp"]
F --> RC["recall / memory_hybrid_search\n(returns memories, not chat lines)"]
INGEST["learning::transcript_ingest\n(extracts facts → conversation_memory /\nconversation_reflections)"] --> MEM
end
Reviews (3): Last reviewed commit: "fix(memory): stop copying the conversati..." | Re-trigger Greptile
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 513112549c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
yh928
force-pushed
the
fix/conversation-copies-out-of-recall
branch
from
August 2, 2026 13:03
5131125 to
0d595ec
Compare
…swer with memories Three writers copied chat text verbatim into `memory_docs`: - the chat turn stored the user's message as a `conversation` document, - the channel runtime stored each inbound message the same way, - and every turn overwrote a single `assistant_resp` row holding a 100-character truncation of the reply, mislabelled `Daily`. All three duplicated text the transcript already holds, and all three landed in the vector space the agent searches for *facts*, so raw chat lines competed with real memories for every recall slot (tinyhumansai#5312). The user's message cost an embedding round-trip per turn to produce a row no reader wanted. The roles are now split cleanly. `transcript_search` reads the conversation — it searches the thread transcripts, which still receive every message (`memory_conversations` persists both web chat and `ChannelMessageReceived`). `memory_recall` / `memory_hybrid_search` answer with memories. What is worth keeping from a conversation is extracted rather than copied: `learning::transcript_ingest` distils preferences, decisions, commitments, facts, and reflections into the `conversation_memory` / `conversation_reflections` namespaces, and those stay fully recallable. Existing installs already carry the old rows, so recall filters them out (`drop_verbatim_conversation_copies`). The filter sits in `query_namespace_hits_excluding_session`, which is the one layer BOTH `Memory::recall` and `memory_hybrid_search` pass through — the hybrid tool does not go through `recall`, so a filter there would have missed the tool the context scout and flow memory agent actually call. `recall_namespace_memories` (query-less recall) gets it too. The filter is scoped to the **global** namespace deliberately. The extracted memories above carry the same `conversation` category, so filtering on category alone would have deleted the useful half of the feature. In the global namespace that category has only ever meant a raw copy. Tests: - the chat turn now asserts it writes NOTHING, and the channel test asserts a prior message is not replayed out of memory, - new query tests cover each legacy shape (user autosave key, channel key with no prefix, the fixed `assistant_resp` key) and pin that extracted memories in their own namespaces survive, and that a non-conversation global memory is untouched, - the same-session exclusion tests move to `conversation_memory`, where session-tagged documents actually live now that the autosave is gone. Also drops `channels::context::conversation_memory_key`, whose only caller was the deleted channel write. One of its two tests carried no `#[test]` attribute and had never run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
yh928
force-pushed
the
fix/conversation-copies-out-of-recall
branch
from
August 5, 2026 01:55
0d595ec to
56a6312
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
12 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/memory/store/namespace_store/query.rs`:
- Around line 151-171: Update drop_verbatim_conversation_copies to follow the
domain logging contract: use the stable [domain] prefix and add debug logs for
function entry, the non-global namespace early-return branch, and function exit.
Include stable count fields such as the input and remaining document counts,
while preserving the existing filtering behavior and dropped-count logging.
- Line 215: Update query_namespace_hits_excluding_session and the
memory_hybrid_search path so episodic transcript hits are excluded from global
hybrid-search results, while remaining available to transcript-specific
searches. Make episodic inclusion explicit at the relevant query boundary,
preserve query_namespace_ranked’s existing non-document filtering, and add a
regression covering a matching global episodic row to verify
memory_hybrid_search never renders it.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4db66f00-c577-4034-859f-67f98d3be49f
📒 Files selected for processing (8)
src/openhuman/agent/harness/session/turn/core.rssrc/openhuman/agent/tests.rssrc/openhuman/channels/context.rssrc/openhuman/channels/runtime/dispatch/processor.rssrc/openhuman/channels/tests/memory.rssrc/openhuman/memory/store/memory_trait.rssrc/openhuman/memory/store/namespace_store/query.rssrc/openhuman/memory/store/namespace_store/query_tests.rs
💤 Files with no reviewable changes (1)
- src/openhuman/channels/context.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/openhuman/agent/tests.rs
- src/openhuman/channels/runtime/dispatch/processor.rs
- src/openhuman/channels/tests/memory.rs
…call Review found the same leak twice, from two directions, and both were right. Dropping the verbatim `[conversation]` documents closed one door; the episodic FTS5 merge a hundred lines further down was the other. `memory_hybrid_search` renders every hit it is handed, so a global query still came back with raw chat turns — truncated to 500 characters, scored, and competing with extracted memories for the same slots. That is exactly what tinyhumansai#5312 is about. The merge is removed rather than filtered at each renderer: `transcript_search` reads the thread transcript directly, which is complete and current where the episodic mirror is neither, and nothing else consumes `fts5::episodic_search`. Filtering per caller would have left the next caller to rediscover this. The episodic-present reweighting goes with it, since the branch that applied it can no longer be taken. Tests: the three that pinned the merge are replaced by one that asserts the opposite — an episodic row `episodic_search` finds directly must not appear in `query_namespace_hits("global", ..)`. The fixture is checked for a direct match first, so the assertion cannot pass vacuously. namespace_store 162 pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
yh928 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/memory/store/namespace_store/query.rs (1)
117-164: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit
query.rsto meet the Rust file-size limit.The final file extends to Line 1381. Line 117 adds retrieval behavior to a file that exceeds the 500-line limit. Split the retrieval logic into focused modules and keep
src/openhuman/memory/store/namespace_store/query.rsat or below 500 lines.As per coding guidelines, “Keep file size to ≤ 500 lines of code.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/memory/store/namespace_store/query.rs` around lines 117 - 164, Split the retrieval logic containing drop_verbatim_conversation_copies and its related query helpers out of namespace_store/query.rs into focused Rust module(s), preserving behavior and visibility at existing call sites. Update module declarations and imports as needed, and ensure query.rs is no longer than 500 lines.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/openhuman/memory/store/namespace_store/query.rs`:
- Around line 117-164: Split the retrieval logic containing
drop_verbatim_conversation_copies and its related query helpers out of
namespace_store/query.rs into focused Rust module(s), preserving behavior and
visibility at existing call sites. Update module declarations and imports as
needed, and ensure query.rs is no longer than 500 lines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: db43ad85-187f-4137-96c5-a1ec133357ad
📒 Files selected for processing (2)
src/openhuman/memory/store/namespace_store/query.rssrc/openhuman/memory/store/namespace_store/query_tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/openhuman/memory/store/namespace_store/query_tests.rs
… recall Two gaps the review found, both of the same kind: an assertion that holds for the wrong reason. The channel test only proved recall hides the prior message — it would have passed just as well if `process_channel_message` started writing raw messages again, since the new filter would hide those too. It now reads `memory_docs` directly and asserts the row count for the secret is zero, which is the property the PR actually claims. `recall_namespace_memories` carries its own copy of the filter and nothing called it. `query_namespace_ranked` passing says nothing about it — the two load the same documents but neither delegates to the other, so either could lose the filter alone. Adds a regression that drops a `user_msg:` copy and keeps a real memory through the query-less path. Also completes the logging contract on the filter: entry, the non-global early return, and one exit either way — a run that drops nothing is as much a fact about this filter as one that drops. Counts only; the documents are recalled memory and their content must not reach the log. namespace_store 163, channels::tests::memory pass. Reported by CodeRabbit on tinyhumansai#5315. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
There was a problem hiding this comment.
yh928 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
This was referenced Aug 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
memory_docs. All three writes are removed.transcript_searchnow owns reading the conversation;memory_recall/memory_hybrid_searchanswer with memories. The transcript itself is unchanged — every message is still persisted.learning::transcript_ingest) are untouched and stay fully recallable.Problem
Three writers put raw chat text into the semantic memory store:
session::turn::coreuser_msg:{uuid}conversationchannels::runtime::dispatch::processor{channel}_{sender}_{id}conversationsession::turn::core(post-turn)assistant_resp(fixed)dailyEach is a duplicate of something the thread transcript already holds, and each lands in the same vector space the agent searches for facts — so raw chat lines competed with real memories for every recall slot (#5312). The user's message additionally cost an embedding round-trip (Voyage) per turn to produce a row no reader wanted.
The assistant copy was the weakest of the three: a 100-character truncation under a fixed key.
upsert_documentkeys by(namespace, key), so every turn in the workspace overwrote the one before it — a single perpetually-stale row, mislabelledDaily, that could not serve as history for anything.The two tools had also drifted into doing the same job.
transcript_searchalready searches every thread's messages with recency ranking and active-thread exclusion, which is exactly what the copies were trying to provide, less well.Solution
Stop writing. The three writes are removed. Nothing about transcript retention changes:
memory_conversationspersists both web chat and everyChannelMessageReceivedinto the thread transcript, and that is whattranscript_searchreads.Hide what is already stored.
drop_verbatim_conversation_copiesfilters the legacy rows out of recall. Two design points:query_namespace_hits_excluding_session, the single layer bothMemory::recallandmemory_hybrid_searchpass through. The hybrid tool does not go throughrecall— it calls the store directly — so a filter placed inrecallalone would have missed the toolcontext_scoutandflow_memory_agentare actually pointed at.recall_namespace_memories(query-less recall) gets it too.MemoryCategory::Conversationalone would have been wrong:learning::transcript_ingestwrites distilled preferences, decisions, commitments, facts, and reflections under that same category, into the dedicatedconversation_memory/conversation_reflectionsnamespaces. Those are memories, and recall should return them. The filter is therefore scoped to the global namespace, where the category has only ever meant a raw copy.The legacy
assistant_resprow is matched by key, since its category isdailyand no category test would catch it.channels::context::conversation_memory_keyis dropped — its only caller was the deleted channel write.Submission Checklist
query.rsis exercised by the newquery_tests.rscases; the deleted write paths are covered by the inverted assertions inagent/tests.rsandchannels/tests/memory.rsCloses #NNNin the## RelatedsectionTesting
New (
memory_store/namespace_store/query_tests.rs) — one per legacy shape, plus the two that guard against over-filtering:recall_drops_a_legacy_user_message_copyrecall_drops_a_legacy_channel_message_copy— the channel key has no prefix to match on, which is why the filter keys off categoryrecall_drops_the_legacy_assistant_reply_snapshot— fixed key,dailycategoryrecall_keeps_extracted_conversation_memories— the failure case that matters: proves the filter does not take thetranscript_ingestoutput with ita_non_conversation_global_memory_is_untouchedRewritten to assert the new behaviour:
agent::tests::a_turn_does_not_copy_the_conversation_into_memory— wasauto_save_stores_messages_in_memorychannels::tests::memory::process_channel_message_does_not_replay_a_prior_message_from_memory— was..._uses_autosaved_memory_after_history_is_clearedRetargeted: the same-session exclusion tests (
memory_trait.rs,query_tests.rs) move their fixtures toconversation_memory. That guard was written for the chat autosave, which is gone; session-tagged documents now come fromtranscript_ingest, so the tests exercise it where it still applies.cargo test --lib: 12553 passed. The 3 remaining failures (credentials::ops,tinyplace::manifest×2) reproduce identically on the base commit with no changes applied.Impact
memory_recall/memory_hybrid_search. To recall what was said, calltranscript_search. Agents that had both tools already had the better route.memory_forgetand the memory UI, so a user who wants them gone can still remove them.memory.auto_saveno longer gates any write in the turn path. The flag is left in place rather than removed in this PR — retiring it touches the config schema and the settings UI, and is better done on its own.Related
Closes #5312
Summary by CodeRabbit
Bug Fixes
Improvements