diff --git a/src/openhuman/agent/harness/session/turn/core.rs b/src/openhuman/agent/harness/session/turn/core.rs index 158d6e5ecc..3ee5ff72c3 100644 --- a/src/openhuman/agent/harness/session/turn/core.rs +++ b/src/openhuman/agent/harness/session/turn/core.rs @@ -640,7 +640,16 @@ impl Agent { ); } - if self.auto_save { + // `auto_save` says the workspace keeps its chat in memory; the origin says + // whether this 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, not + // anything the user said. Live, that stored `memory_goals::enrich`'s + // prompt as a `Conversation` document keyed `user_msg:…`, where it then + // competed for slots in every later recall (#5312). Gating here rather + // than at each caller keeps a new internal agent from having to remember + // to opt out, which is a thing nobody notices forgetting. + if self.auto_save && crate::openhuman::agent::turn_origin::current_is_user_authored() { // Fire-and-forget: persisting the user message to the memory store // does an embedding round-trip (Voyage) + memory-tree write that the // in-flight turn never reads back. Awaiting it delayed the start of diff --git a/src/openhuman/agent/tests.rs b/src/openhuman/agent/tests.rs index c7c1d85c4d..3ce1814420 100644 --- a/src/openhuman/agent/tests.rs +++ b/src/openhuman/agent/tests.rs @@ -655,7 +655,20 @@ async fn auto_save_stores_messages_in_memory() { true, // auto_save enabled ); - let _ = agent.turn("Remember this fact").await.unwrap(); + // Scoped like a real chat turn. The autosave only stores what a person sent + // (`turn_origin::current_is_user_authored`), and production entry points + // scope an origin — web chat `WebChat`, channels `ExternalChannel` — so a + // test that skipped it would be asserting a shape no caller produces. + let _ = crate::openhuman::agent::turn_origin::with_origin( + crate::openhuman::agent::turn_origin::AgentTurnOrigin::WebChat { + thread_id: "t-autosave".into(), + client_id: "c-autosave".into(), + request_id: None, + }, + agent.turn("Remember this fact"), + ) + .await + .unwrap(); // Both user message and assistant response should be saved. The assistant // reply is persisted synchronously, but the user message is saved @@ -676,6 +689,91 @@ async fn auto_save_stores_messages_in_memory() { ); } +/// Poll for a `user_msg:` conversation memory over the same one-second window +/// the positive autosave test uses, and report whether one ever landed. +/// +/// The user message is stored fire-and-forget (`tokio::spawn` in +/// `turn/core.rs`, #3610), so both directions need the *same* window or the +/// negative tests are the weaker assertion: a broken guard whose spawned store +/// lands after a short fixed sleep would pass them while failing in production. +/// Returns as soon as one appears, so a genuinely broken guard fails fast +/// instead of costing the full second. +async fn poll_for_stored_user_message(mem: &Arc) -> Vec { + let mut keys: Vec = Vec::new(); + for _ in 0..50 { + keys = mem + .list(None, None, None) + .await + .expect( + "memory list must succeed; an empty list here would let the \ + autosave assertions below pass without reading storage", + ) + .into_iter() + .map(|e| e.key) + .collect(); + if keys.iter().any(|k| k.starts_with("user_msg:")) { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(20)).await; + } + keys +} + +/// `ExternalChannel` is in the user-authored allowlist alongside `WebChat`, so +/// it needs its own positive case: a Telegram/Discord/Slack message is a person +/// talking, and the allowlist would be half-tested if only the web thread had a +/// stored-message assertion. +#[tokio::test] +async fn an_external_channel_turn_stores_the_user_message() { + use crate::openhuman::agent::turn_origin::{with_origin, AgentTurnOrigin}; + + let (mem, _tmp) = make_sqlite_memory(); + let provider = Arc::new(ScriptedProvider::new(vec![text_response("got it")])); + let (mut agent, _tmp2) = build_agent_with_memory(provider, vec![], mem.clone(), true); + + let origin = AgentTurnOrigin::ExternalChannel { + channel: "telegram".into(), + sender: Some("user-42".into()), + reply_target: "chat-7".into(), + message_id: "m-1".into(), + }; + let _ = with_origin(origin, agent.turn("remember my flight is at nine")) + .await + .unwrap(); + + let keys = poll_for_stored_user_message(&mem).await; + assert!( + keys.iter().any(|k| k.starts_with("user_msg:")), + "a channel message is a person talking and must be stored: {keys:?}" + ); +} + +/// The desktop Settings agent-chat panel calls `openhuman.agent_chat`, which +/// scopes `DirectChat`. That is a person typing, so it stores — the origin +/// exists precisely because `Cli` (its previous label, chosen for the approval +/// gate) would have dropped these messages. +#[tokio::test] +async fn a_direct_chat_turn_stores_the_user_message() { + use crate::openhuman::agent::turn_origin::{with_origin, AgentTurnOrigin}; + + let (mem, _tmp) = make_sqlite_memory(); + let provider = Arc::new(ScriptedProvider::new(vec![text_response("noted")])); + let (mut agent, _tmp2) = build_agent_with_memory(provider, vec![], mem.clone(), true); + + let _ = with_origin( + AgentTurnOrigin::DirectChat, + agent.turn("my dog is called Pip"), + ) + .await + .unwrap(); + + let keys = poll_for_stored_user_message(&mem).await; + assert!( + keys.iter().any(|k| k.starts_with("user_msg:")), + "a direct-chat message is a person typing and must be stored: {keys:?}" + ); +} + #[tokio::test] async fn auto_save_disabled_does_not_store() { let (mem, _tmp) = make_sqlite_memory(); @@ -694,6 +792,63 @@ async fn auto_save_disabled_does_not_store() { assert_eq!(count, 0, "Expected 0 memory entries with auto_save off"); } +/// An internal agent runs on the same config as the chat, so it inherits +/// `auto_save` — but its "user message" is the prompt the host wrote for it. +/// Storing that as a conversation memory puts prompt boilerplate where the +/// user's own words belong, and it competes for slots in every later recall +/// (#5312). A `TrustedAutomation` turn therefore saves nothing. +#[tokio::test] +async fn an_automation_turn_does_not_store_its_prompt_as_the_users_memory() { + use crate::openhuman::agent::turn_origin::{ + with_origin, AgentTurnOrigin, TrustedAutomationSource, + }; + + let (mem, _tmp) = make_sqlite_memory(); + let provider = Arc::new(ScriptedProvider::new(vec![text_response("goals updated")])); + let (mut agent, _tmp2) = build_agent_with_memory( + provider, + vec![], + mem.clone(), + true, // auto_save enabled, exactly as a config-built internal agent gets it + ); + + let origin = AgentTurnOrigin::TrustedAutomation { + job_id: "memory_goals:enrich:1".into(), + source: TrustedAutomationSource::Subconscious, + }; + let _ = with_origin(origin, agent.turn("Maintain the existing goals list.")) + .await + .unwrap(); + + // The fire-and-forget store would land shortly after the turn returns, so + // give it the same one-second window the positive tests poll for before + // concluding it never happened. + let keys = poll_for_stored_user_message(&mem).await; + assert!( + !keys.iter().any(|k| k.starts_with("user_msg:")), + "an automation prompt must not be stored as a user message: {keys:?}" + ); +} + +/// An unscoped turn is not a user turn either. `turn_origin` documents that +/// every entry point scopes an origin and that an unlabelled one fails closed; +/// the autosave follows the same allowlist, so a caller that forgets cannot +/// quietly write host text into the user's memory. +#[tokio::test] +async fn an_unscoped_turn_stores_no_user_message() { + let (mem, _tmp) = make_sqlite_memory(); + let provider = Arc::new(ScriptedProvider::new(vec![text_response("ok")])); + let (mut agent, _tmp2) = build_agent_with_memory(provider, vec![], mem.clone(), true); + + let _ = agent.turn("who wrote this?").await.unwrap(); + + let keys = poll_for_stored_user_message(&mem).await; + assert!( + !keys.iter().any(|k| k.starts_with("user_msg:")), + "an unscoped turn must not be credited to the user: {keys:?}" + ); +} + // ═══════════════════════════════════════════════════════════════════════════ // 10. Native vs XML dispatcher integration // ═══════════════════════════════════════════════════════════════════════════ diff --git a/src/openhuman/agent/turn_origin.rs b/src/openhuman/agent/turn_origin.rs index 758056bce2..d320e76800 100644 --- a/src/openhuman/agent/turn_origin.rs +++ b/src/openhuman/agent/turn_origin.rs @@ -63,6 +63,21 @@ pub enum AgentTurnOrigin { }, /// Command-line / sub-agent / one-off internal invocation. Cli, + /// A person typing into a direct chat surface that is not the web thread — + /// today the `openhuman.agent_chat` RPC behind the desktop Settings agent + /// chat panel, and an operator running the same RPC by hand. + /// + /// Split out of [`Cli`](Self::Cli) rather than folded into it because the + /// two answer different questions with the same variant. `Cli` was chosen + /// for this RPC to tell the **approval gate** "trusted caller, do not fail + /// closed"; it says nothing about who wrote the text, and its own + /// documentation covers sub-agent and internal invocations too. Reusing it + /// for [`is_user_authored`](Self::is_user_authored) would have to answer + /// "did a person write this" with a trust answer, and would drop a real + /// person's message on the floor. + /// + /// Trust-wise this is exactly `Cli` — the gate treats them identically. + DirectChat, /// Unlabelled — gate fails closed. Every entry point MUST scope a real /// origin before invoking the agent. Unknown, @@ -121,9 +136,46 @@ impl AgentTurnOrigin { format!("TrustedAutomation({source:?})") } AgentTurnOrigin::Cli => "Cli".to_string(), + AgentTurnOrigin::DirectChat => "DirectChat".to_string(), AgentTurnOrigin::Unknown => "Unknown".to_string(), } } + + /// Whether the turn's text was written by a **person**. + /// + /// `WebChat`, `ExternalChannel`, and `DirectChat` carry what a human sent. + /// Every other origin carries text the host wrote for an agent to act on: a + /// `TrustedAutomation` prompt (cron, subconscious, goal continuation, + /// workflow), a `Cli` invocation — which this module documents as + /// "command-line / **sub-agent** / one-off internal" — or an unscoped + /// `Unknown`. + /// + /// An allowlist, not a denylist, and for the same reason the permission + /// gate uses one: a new origin is a turn nobody has classified yet, and + /// mistaking a host-written prompt for a user message writes it into the + /// user's memory, where it is indistinguishable from something they said. + /// A caller that genuinely relays a person's text scopes one of the three + /// origins above. + /// + /// This is a **different question** from the one the approval gate asks, + /// and the two must not be collapsed onto one variant. The gate asks how + /// far to trust the caller; this asks who wrote the words. `DirectChat` + /// exists because `agent_chat` needs the first answer to be "trusted" and + /// the second to be "a person" — see that variant's note. + pub fn is_user_authored(&self) -> bool { + matches!( + self, + AgentTurnOrigin::WebChat { .. } + | AgentTurnOrigin::ExternalChannel { .. } + | AgentTurnOrigin::DirectChat + ) + } +} + +/// Whether the current turn's text was written by a person — `false` outside +/// any origin scope, matching [`AgentTurnOrigin::is_user_authored`]'s allowlist. +pub fn current_is_user_authored() -> bool { + current().is_some_and(|origin| origin.is_user_authored()) } tokio::task_local! { diff --git a/src/openhuman/inference/local/ops.rs b/src/openhuman/inference/local/ops.rs index 1b466c3abf..a901280b01 100644 --- a/src/openhuman/inference/local/ops.rs +++ b/src/openhuman/inference/local/ops.rs @@ -102,11 +102,16 @@ pub async fn agent_chat( config.default_temperature = temp; } let mut agent = Agent::from_config(config).map_err(|e| e.to_string())?; - // Direct `agent_chat` RPC — invoked by trusted clients (desktop UI, - // operator CLI). Label as CLI so the approval gate doesn't fail - // closed on an unlabelled call site. + // Direct `agent_chat` RPC — invoked by trusted clients (the desktop + // Settings agent-chat panel, an operator running the RPC by hand). The + // approval gate treats `DirectChat` exactly like `Cli`, so it still doesn't + // fail closed here; the difference is that `message` is something a person + // typed, so the turn is user-authored and its text belongs in conversation + // memory. Labelling it `Cli` — whose own docs cover sub-agent and internal + // invocations — would answer that second question with a trust answer and + // silently drop a real user message. let run = crate::openhuman::agent::turn_origin::with_origin( - crate::openhuman::agent::turn_origin::AgentTurnOrigin::Cli, + crate::openhuman::agent::turn_origin::AgentTurnOrigin::DirectChat, agent.run_single(message), ); let response = match thread_id.as_deref() { diff --git a/src/openhuman/security/approval/gate.rs b/src/openhuman/security/approval/gate.rs index dbe2313377..d3f4701d7c 100644 --- a/src/openhuman/security/approval/gate.rs +++ b/src/openhuman/security/approval/gate.rs @@ -816,9 +816,15 @@ impl ApprovalGate { // TTL-denies, the conservative fail-closed default for a // user-forced HITL gate. } - AgentTurnOrigin::Cli => { + // Same trust decision for both: a local operator invoking the core + // directly. They differ only in whether the turn's text was written + // by a person (`turn_origin::is_user_authored`), which this gate + // does not ask. Kept as one arm so the two can never drift apart on + // the trust axis, which is the axis this gate owns. + AgentTurnOrigin::Cli | AgentTurnOrigin::DirectChat => { tracing::debug!( tool = tool_name, + origin = %origin.class(), "[approval::gate] CLI / sub-agent caller — allowing without prompt" ); return (GateOutcome::Allow, None); @@ -2931,6 +2937,31 @@ mod tests { assert!(matches!(outcome, GateOutcome::Allow)); } + #[tokio::test] + async fn intercept_with_direct_chat_origin_allows_without_prompt() { + // `DirectChat` shares the CLI arm: both are a local operator invoking + // the core directly, and this gate decides on trust, not on whether the + // turn's text was person-written. The arm is only sound while both + // origins are covered — pinning `Cli` alone would let a future split + // send `DirectChat` down the park-and-prompt path (or the TTL-deny one) + // with nothing failing. + let (gate, _dir) = test_gate(); + let outcome = turn_origin::with_origin( + AgentTurnOrigin::DirectChat, + gate.intercept("shell", "run ls", serde_json::json!({})), + ) + .await; + assert!( + matches!(outcome, GateOutcome::Allow), + "DirectChat must allow unprompted, got {outcome:?}" + ); + // Allowed without prompting means nothing was parked for a decision. + assert!( + gate.list_pending().unwrap().is_empty(), + "an allow-without-prompt must not persist a pending approval" + ); + } + #[tokio::test] async fn intercept_with_external_channel_origin_persists_and_ttl_denies() { // Non-web channel inbound (Telegram / Discord / Slack / etc.):