diff --git a/src/openhuman/agent/harness/session/builder/builder_tests_part_01_tests.rs b/src/openhuman/agent/harness/session/builder/builder_tests_part_01_tests.rs index 31231e8249..a9ae92c319 100644 --- a/src/openhuman/agent/harness/session/builder/builder_tests_part_01_tests.rs +++ b/src/openhuman/agent/harness/session/builder/builder_tests_part_01_tests.rs @@ -525,6 +525,26 @@ async fn build_session_agent_uses_profile_memory_instead_of_root_memory() { /// #6040 — the memory-access instruction is about the memory tools, not the /// learning subsystem, so it must be in the prompt with `learning.enabled` /// off (the default) whenever a retrieval tool is registered and visible. +/// +/// Passes the definition explicitly via [`builtin_def`] rather than letting the +/// factory resolve `"orchestrator"` from the registry, and that is load-bearing +/// rather than ceremony. +/// +/// The section is gated on `memory_recall` being **registered and visible after +/// tool filtering** (`any_tool_offered`), and the visible set comes from the +/// resolved definition's tool scope. With `None` here the factory reads +/// `AgentDefinitionRegistry`'s `static GLOBAL: OnceLock<…>` +/// (`harness/definition_part_02.rs:24`) — first-write-wins and never reset — so +/// the test was asserting against whichever definition set some *other* test in +/// the binary had installed first. That is exactly the hazard `builtin_def` +/// was written for: it loads fresh from the bundled TOML, "entirely independent +/// of the global registry singleton". +/// +/// It is why this passed run alone and failed inside the full +/// `openhuman::agent` run (`ci-lite` scopes the Rust lane per changed domain, +/// so the whole scope only runs when a PR touches `agent/`), and why the +/// sibling write-side test in `builder_tests_part_03_tests.rs` never flaked — +/// it already supplied `builtin_def("orchestrator")`. #[tokio::test] async fn memory_access_instruction_is_present_with_learning_disabled() { use crate::openhuman::agent::context::prompt::LearnedContextData; @@ -535,11 +555,20 @@ async fn memory_access_instruction_is_present_with_learning_disabled() { let mut config = test_config(&tmp); config.learning.enabled = false; - let agent = Agent::build_session_agent_inner(&config, "orchestrator", None, None, false, None) - .expect("build session agent"); + let orchestrator = builtin_def("orchestrator"); + let agent = Agent::build_session_agent_inner( + &config, + "orchestrator", + Some(&orchestrator), + None, + false, + None, + ) + .expect("build session agent"); let prompt = agent .build_system_prompt(LearnedContextData::default()) .expect("build_system_prompt"); + assert!( prompt.contains(MEMORY_ACCESS_INSTRUCTION.trim()), "the memory-access section must not be gated on learning.enabled" diff --git a/src/openhuman/agent/harness/subagent_runner/ops/graph_part_02.rs b/src/openhuman/agent/harness/subagent_runner/ops/graph_part_02.rs index 0d01d0e379..3ccf7d3fcc 100644 --- a/src/openhuman/agent/harness/subagent_runner/ops/graph_part_02.rs +++ b/src/openhuman/agent/harness/subagent_runner/ops/graph_part_02.rs @@ -76,6 +76,16 @@ fn append_worker_message( "agent_id": agent_id, "task_id": task_id, "mode": "typed", + // #5934: a mirror row that is not the sub-agent speaking is a tool + // result, and the renderer maps every non-`"agent"` sender to + // `role: 'user'` (`assistantUiMessages.ts`) — so without this flag a + // tool's raw output is painted, in an openable worker thread, as + // something the human typed. `hidden` keeps the row in the log for the + // process rail and the process-source view; it only stops being chat. + // Genuine worker-thread user turns are written by + // `worker_thread::{create_worker_thread, append_worker_user_message}`, + // not here, so they are unaffected. Overridable by `metadata` below. + "hidden": sender != "agent", }); if let (Some(base), Some(extra_fields)) = (extra.as_object_mut(), metadata.as_object()) { for (k, v) in extra_fields { diff --git a/src/openhuman/agent/harness/subagent_runner/ops/graph_tests.rs b/src/openhuman/agent/harness/subagent_runner/ops/graph_tests.rs index cbcadb68a3..f7ecc431b3 100644 --- a/src/openhuman/agent/harness/subagent_runner/ops/graph_tests.rs +++ b/src/openhuman/agent/harness/subagent_runner/ops/graph_tests.rs @@ -636,3 +636,109 @@ async fn an_allowlist_that_readmits_a_spawn_tool_is_refused_loudly() { "the readmitted spawn tool warns exactly once: {warnings:?}" ); } + +/// #5934 (item 1): a mirrored sub-agent **tool result** must not be painted as +/// something the human typed. +/// +/// Both worker-thread mirrors write tool output with `sender: "user"`, and the +/// renderer maps every non-`"agent"` sender to `role: 'user'` +/// (`app/src/providers/assistantUiMessages.ts:536`) while keying visibility +/// only on `extraMetadata.hidden` (`:664`, and the same flag in +/// `ChatThreadView.tsx:312` / `timeline/selectors.ts:50`). Worker threads are +/// openable chats — `create_worker_thread` stamps `labels: ["tasks"]` and +/// `threadFilter.ts` lists that tab — so an unflagged mirror row shows the +/// tool's raw output in a right-aligned user bubble. +/// +/// The invariant: every non-`"agent"` row these mirrors write is `hidden`. The +/// record stays in the log for the process rail; it just stops being chat. +#[test] +fn mirrored_tool_results_are_hidden_from_the_worker_thread_chat() { + use crate::openhuman::memory::conversations::{self as store, CreateConversationThread}; + + let dir = std::env::temp_dir().join(format!("wt-5934-{}", uuid::Uuid::new_v4())); + std::fs::create_dir_all(&dir).unwrap(); + let now = chrono::Utc::now().to_rfc3339(); + for id in ["worker-typed", "worker-recovered"] { + store::ensure_thread( + dir.clone(), + CreateConversationThread { + id: id.to_string(), + title: "task".to_string(), + created_at: now.clone(), + parent_thread_id: Some("parent-1".to_string()), + labels: Some(vec!["tasks".to_string()]), + personality_id: None, + }, + ) + .unwrap(); + } + + const RAW: &str = "{\"events\":[{\"title\":\"raw tool output the human never typed\"}]}"; + + // The typed path (`ConversationMessage::ToolResults`), used on a normal run. + mirror_worker_thread( + &dir, + "worker-typed", + "researcher", + "task-1", + &[ + ConversationMessage::AssistantToolCalls { + text: Some("checking the calendar".to_string()), + tool_calls: vec![crate::openhuman::inference::provider::ToolCall { + id: "call-1".to_string(), + name: "list_events".to_string(), + arguments: "{}".to_string(), + extra_content: None, + }], + reasoning_content: None, + extra_metadata: None, + }, + ConversationMessage::ToolResults(vec![ + crate::openhuman::agent::messages::ToolResultMessage { + tool_call_id: "call-1".to_string(), + content: RAW.to_string(), + }, + ]), + ], + Some("Here is your week."), + ); + + // The error-recovery path (`role: "tool"`), used when a run fails mid-turn. + mirror_worker_thread_from_history( + &dir, + "worker-recovered", + "researcher", + "task-1", + &[ + ChatMessage::assistant("checking the calendar"), + ChatMessage::tool(RAW), + ], + Some("[subagent run failed before completion]"), + ); + + // Collected, not asserted per row: both mirrors must be reported, so a + // failure names every path that is still painting a tool result as chat. + let mut painted_as_user_chat: Vec = Vec::new(); + for thread_id in ["worker-typed", "worker-recovered"] { + let rows = store::get_messages(dir.clone(), thread_id).unwrap(); + assert!( + rows.iter().any(|r| r.content == RAW), + "{thread_id}: the mirror wrote no tool-result row to assert on" + ); + for row in rows.iter().filter(|r| r.sender != "agent") { + if row.extra_metadata.get("hidden").and_then(|v| v.as_bool()) != Some(true) { + painted_as_user_chat.push(format!( + "{thread_id}: sender={:?} not hidden: {}", + row.sender, row.content + )); + } + } + } + assert!( + painted_as_user_chat.is_empty(), + "mirrored tool results render in a user chat bubble:\n{}", + painted_as_user_chat.join("\n") + ); + + let _ = std::fs::remove_dir_all(&dir); +}