fix(memory): store only what a person sent as a conversation memory - #5313
Conversation
|
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: Team Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe change adds ChangesUser-authored autosave
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change limits conversation-memory autosave to turns authored by a person and prevents internal prompts from being stored as user messages. No actionable merge-blocking risk remains, so it is merge-ready after normal checks and review. Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Title checkExplanation The pull request title 'fix(memory): store only what a person sent as a conversation memory' directly describes the main change. The changeset adds an origin-based gate to the conversation-memory autosave system to exclude internal-agent turns and unscoped turns, storing only user-authored turns as conversation memories. This title captures the core objective and is specific and concise. Full details: Linked Issues checkExplanation The pull request meets all acceptance criteria from issue Full details: Out of Scope Changes checkExplanation All changes are directly within the scope of issue Warning Your free Security trial is over. An organization admin can upgrade to Advanced for continuous pull request security review or dismiss this notice. Comment |
coderabbitai
Bot
added
agent
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/agent/tests.rs`:
- Around line 738-744: Extend the negative autosave test wait in
src/openhuman/agent/tests.rs lines 738-744 to use the positive test’s
50-iteration, one-second polling window before checking mem.list; apply the same
wait change at lines 764-767. Update both sites consistently, or replace both
waits with a deterministic store-call signal.
- Around line 658-671: Extend the autosave regression coverage in the existing
test around the WebChat-scoped turn by adding an
AgentTurnOrigin::ExternalChannel turn through turn_origin::with_origin. Verify
the resulting persisted documents include a user_msg: entry, matching the
existing positive WebChat persistence assertion.
🪄 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: fa187edd-e21a-4d0c-b775-f1071c794c07
📒 Files selected for processing (3)
src/openhuman/agent/harness/session/turn/core.rssrc/openhuman/agent/tests.rssrc/openhuman/agent/turn_origin.rs
|
| Filename | Overview |
|---|---|
| src/openhuman/agent/turn_origin.rs | Adds DirectChat variant, is_user_authored() allowlist method, and current_is_user_authored() free function. Design is sound — allowlist ensures new origins fail closed. |
| src/openhuman/agent/harness/session/turn/core.rs | Autosave gate tightened to require both auto_save and current_is_user_authored(); fix is a single-line addition at the right chokepoint. |
| src/openhuman/inference/local/ops.rs | agent_chat RPC switched from Cli to DirectChat; approval-gate behavior is unchanged, but now correctly marks the turn as user-authored so direct-chat messages are saved. |
| src/openhuman/security/approval/gate.rs | DirectChat added to the Cli match arm with no change in trust policy; origin.class() is now logged so the distinction is visible in traces. |
| src/openhuman/agent/tests.rs | Four new tests covering positive (ExternalChannel, DirectChat) and negative (TrustedAutomation, unscoped) autosave cases with a symmetric polling window. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["agent.turn(user_message)"] --> B{"self.auto_save?"}
B -- No --> C[Skip autosave]
B -- Yes --> D{"current_is_user_authored()"}
D -- false --> C
D -- true --> E[tokio::spawn autosave]
E --> F["memory.store user_msg:uuid"]
Reviews (3): Last reviewed commit: "fix(agent): keep autosave for the direct..." | 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: 6d46baecaa
ℹ️ 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".
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/agent/tests.rs`:
- Around line 704-710: Update the key collection flow around Memory::list in the
affected test helper to propagate listing failures instead of converting them
into an empty collection. Replace unwrap_or_default with unwrap, or change the
helper to return Result and propagate the error, while preserving the existing
key-mapping behavior on success.
In `@src/openhuman/approval/gate.rs`:
- Around line 819-830: Add a regression case to the existing approval tests that
invokes the gate through with_origin using AgentTurnOrigin::DirectChat and
asserts GateOutcome::Allow without parking. Keep the test setup and assertions
aligned with the existing Cli coverage to verify both origins follow the same
approval path.
🪄 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: 03baa7f7-138f-4607-a10a-8a288cfbf846
📒 Files selected for processing (4)
src/openhuman/agent/tests.rssrc/openhuman/agent/turn_origin.rssrc/openhuman/approval/gate.rssrc/openhuman/inference/local/ops.rs
6170a77 to
98aebed
Compare
|
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. |
coderabbitai
Bot
added
the
memory
`poll_for_stored_user_message` turned a failed `Memory::list` into an empty key list, so the tests that assert on which keys the autosave wrote could conclude "no `user_msg:` key was stored" from a storage error rather than from the behaviour under test — the automation and unscoped cases would pass without reading storage at all. It now expects, and says why in the message. Also adds the `DirectChat` approval case. `Cli` and `DirectChat` share one arm because this gate decides on trust, not on whether the turn's text was person-written — and an arm covered by only one of its origins is an arm that can be split without anything failing. The test asserts `Allow` and that nothing was parked, since allowing without a prompt means exactly that. approval::gate 50, agent::tests 107 pass. Reported by CodeRabbit on tinyhumansai#5313. 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.
|
Maintainer review — no changes pushed, read-only assessment. State: The bug is still live on // src/openhuman/agent/harness/session/turn/core_turn.rs:242
if self.auto_save {
...
let autosave_key = format!("user_msg:{}", uuid::Uuid::new_v4());
... memory.store(CONVERSATION_RAW_NAMESPACE, &autosave_key, &user_msg,
MemoryCategory::Conversation, session_id_for_autosave.as_deref())Still gated on What rebasing costsThe file moved: One thing that has changed under you, and is worth a decision
It does not subsume your fix: segregating the namespace and filtering the current session does not stop an internal agent's host prompt being stored as user-authored conversation, and it does not stop that row surfacing in a later session's recall. Gating the write by origin is still the correct fix, and it is upstream of the filter. Worth being explicit about it in the PR description when you rebase, though, because a reviewer looking at Related: #5315 (same author, same area) proposes removing the conversation copies outright rather than gating them. If both are still wanted they need sequencing — the two answers overlap. Not approving; a maintainer reviews and merges. |
`auto_save` says the workspace keeps its chat in memory; it did not say whether
a turn is chat at all. An internal agent is built from the same config
(`Agent::from_config_for_agent`), so it inherits the flag — and its "user
message" is the prompt the host wrote for it.
Live, that put `memory_goals::enrich`'s prompt in the `global` namespace as a
`Conversation` document keyed `user_msg:<uuid>`:
"Maintain the existing goals list. Call goals_list first, then make the
MINIMAL set of changes (goals_add / goals_edit / goals_delete)…
## Context
Recent conversation recap (segment seg-18c746e3…)…"
Prompt boilerplate then competes for slots in every later recall, which is what
made a namespace-wide search read like a transcript dump.
The distinction already existed: `AgentTurnOrigin`. `WebChat` and
`ExternalChannel` carry what a person sent; `TrustedAutomation` (cron,
subconscious, goal continuation, workflow), `Cli` — documented as "command-line
/ sub-agent / one-off internal" — and an unscoped `Unknown` carry host text.
`AgentTurnOrigin::is_user_authored` is an allowlist for the same reason the
permission gate uses one: a new origin is a turn nobody has classified, and
mistaking host text for a user message writes it where the user's own words
belong, indistinguishable afterwards.
Gated at the session, not at each caller, so a new internal agent cannot forget
to opt out — the kind of omission nothing surfaces until the store is inspected.
Existing rows are left alone; this is the write path only.
Tests: an automation turn and an unscoped turn store no `user_msg:*` document;
the existing round-trip test now scopes `WebChat`, as every production entry
point does. agent::tests 100, turn_origin 3, agent::harness::session 205,
memory_goals 7. clippy clean.
Closes tinyhumansai#5312
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
… allowlist Review follow-ups on the user-authored autosave gate. `agent_chat` scoped `AgentTurnOrigin::Cli`, and its comment says why: to tell the approval gate "trusted caller, do not fail closed". That variant also covers sub-agent and internal invocations, so reusing it to answer "did a person write this" dropped a real user message — the desktop Settings agent-chat panel calls this RPC, and with `memory.auto_save` on its messages stopped being stored. The two questions need two variants. `DirectChat` is user-authored and shares the gate's `Cli` arm, so the trust decision is unchanged and cannot drift. Tests: - `ExternalChannel` gets its own positive case; the allowlist had three members and only `WebChat` was covered by a stored-message assertion. - `DirectChat` gets one, so the regression above stays fixed. - Both negative tests now poll the same one-second window as the positive ones via `poll_for_stored_user_message`. The store is fire-and-forget, so a fixed 200 ms sleep would let a broken guard pass while failing live. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
`main` moved the conversation autosave into a dedicated `CONVERSATION_RAW_NAMESPACE`, so the helper's `list(None, …)` came back empty. That failed the two positive cases outright — and, worse, would have passed the two negative ones without ever reading storage, which is the exact false green the helper's own doc comment warns about. List the namespace explicitly so both directions read the same rows.
f1d9e62 to
1748ce6
Compare
|
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. |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0089 · 96,635 in / 1,884 out · 512 cached (1%) · openrouter/openai/text-embedding-3-small, deepseek/deepseek-v4-flash · 692 embedded
critique: $0.0043 · 47,475 in / 340 out · 0 cached (0%) · deepseek/deepseek-v4-flash
security: $0.0020 · 23,116 in / 147 out · 512 cached (2%) · deepseek/deepseek-v4-flash
tests: $0.0017 · 16,641 in / 1,320 out · 0 cached (0%) · deepseek/deepseek-v4-flash
description: $0.0008 · 9,403 in / 77 out · 0 cached (0%) · deepseek/deepseek-v4-flash
How this change flows4 changed behaviours across 22 relationships. 6 surrounding behaviours are shown (60 graph nodes walked). 37 further behaviours left out to keep the diagram readable. flowchart LR
n0["AgentTurnOrigin<br/>changed"]:::changed
n1["grant_turn_cwd<br/>changed"]:::changed
n2["grant_turn_cwd_is_the_only_mutation<br/>changed"]:::changed
n3["...ept_with_cli_origin_allows_without_prompt<br/>changed"]:::changed
n4["with_origin"]:::impacted
n5["test_gate"]:::impacted
n6["intercept_audited"]:::impacted
n7["flow_origin"]:::impacted
n8["chat_origin_park_has_no_source_context"]:::impacted
n9["..._insert_flow_trust_composes_to_auto_allow"]:::impacted
n2 -->|calls| n1
n2 -->|tests| n1
n3 -->|calls| n4
n3 -->|tests| n4
n3 -->|calls| n5
n3 -->|tests| n5
n4 -->|uses| n0
n7 -->|uses| n0
n8 -->|calls| n4
n8 -->|tests| n4
n8 -->|calls| n5
n8 -->|tests| n5
n8 -->|calls| n6
n8 -->|tests| n6
n9 -->|calls| n4
n9 -->|tests| n4
n9 -->|calls| n5
n9 -->|tests| n5
n9 -->|calls| n6
n9 -->|tests| n6
n9 -->|calls| n7
n9 -->|tests| n7
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge. |
tinysweeper
Bot
added
the
priority: p3
|
Rebased onto Where the change landed. The four moves your review flagged, plus one you couldn't have known about:
The tests went to a new part file rather than into
A real defect the rebase exposed. On your "worth a decision" point — agreed, and stated here for the next reader: Verified locally: Re #5315 (remove the conversation copies outright): still true that the two overlap and need sequencing. This one is the smaller, reversible half — it changes what gets written, not whether the feature exists — so it should be safe to land first regardless of how #5315 goes. |
`effective_agent_chat_origin` now falls back to `DirectChat` rather than `Cli`, and this test still pinned the old label — CI caught it, I did not. Also asserts `is_user_authored()`, which is the whole reason for the new variant: `Cli` answers the trust question correctly and the authorship question wrongly, so a real user message typed into the desktop Settings panel would have been dropped from conversation memory.
|
CI caught something I missed, and it was a real one — fixed in
The test now asserts Verified against the coverage lane's filter locally: |
…ns-only\n\nfix(memory): store only what a person sent as a conversation memory\n
3 participants
Summary
AgentTurnOrigin::is_user_authorednames the distinction the codebase already had:WebChat/ExternalChannelcarry what a person sent, every other origin carries host text.Problem
Session's autosave persists the turn's "user message" as aMemoryCategory::Conversationdocument keyeduser_msg:<uuid>, gated only onconfig.memory.auto_save. An internal agent built withAgent::from_config_for_agent(...)inherits that flag — and its "user message" is the prompt the host wrote for it.Found live, in the
globalnamespace of a workspace with two namespaces:That is
memory_goals::enrich's prompt, verbatim, stored as if the user had typed it. It then competes for slots in every later recall, which is what made a namespace-wide memory search read like a transcript dump.The blast radius is every config-built internal agent, not just goals: the flag comes from config, so a caller has to remember to turn it off, and forgetting is invisible until the store is inspected.
Solution
Gate the autosave on the turn's origin.
memory_goals::enrichalready runs underAgentTurnOrigin::TrustedAutomation { source: Subconscious }; a live chat turn runs underWebChat(web chat / TUI) orExternalChannel(Telegram, Discord, …).is_user_authoredis an allowlist, for the same reason the permission gate uses one: a new origin is a turn nobody has classified yet, and mistaking host text for a user message writes it where the user's own words belong, indistinguishable afterwards.Cliis excluded on the strength of its own doc comment — "command-line / sub-agent / one-off internal invocation" — and an unscopedUnknownis excluded becauseturn_originalready documents that every entry point must scope a real origin.Tradeoff: a caller that relays a person's text without scoping an origin stops autosaving. That is the same contract the permission gate enforces, and both production user paths scope one today (
web_chat/ops.rs→WebChat,channels/runtime/dispatch/processor.rs→ExternalChannel).Submission Checklist
an_automation_turn_does_not_store_its_prompt_as_the_users_memory,an_unscoped_turn_stores_no_user_message, and the existing round-trip test now scopesWebChatas production doesN/A: behaviour-only change## Related—N/A: behaviour-only changeN/A: no release-cut surfaceCloses #NNNin the## RelatedsectionImpact
Desktop / CLI / channels: unchanged for user turns. Internal automation turns (cron, subconscious, goal continuation, workflow) stop writing
user_msg:*conversation documents, so the memory store keeps only what a person sent and recall stops competing with prompt boilerplate. No migration: rows written before this change are left in place deliberately.Related
memory_recallwithout a namespace andtranscript_searchnow describe overlapping jobs to the model ("searches everywhere" vs "search your PAST conversations across all threads"). The stores differ —ConversationStore's trigram index vs the vector namespaces — but autosave copies chat into both, so a model can pull the same content twice in one turn.RecallOptsalready carries acategoryfilter if that split is worth enforcing; noted on Internal agents store their own prompts as the user's conversation memories #5312 rather than widened into this PR.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/autosave-user-turns-only6d46baecaad7ba82cd09cb40efb40321d6906657Validation Run
pnpm --filter openhuman-app format:check— N/A: no frontend files changedpnpm typecheck— N/A: no frontend files changedagent::tests100,turn_origin3,agent::harness::session205,memory_goals7 — all greencargo fmt --all,cargo clippy -p openhuman -- -D warningscleanSummary by CodeRabbit
New Features
Bug Fixes