From feb8a802dee46f1e4f0a5d7b1e86c7e4e8c4b89b Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Thu, 10 Sep 2026 17:08:48 +0530 Subject: [PATCH 1/2] fix(subagent): stop mirrored tool results rendering as the user's own message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A sub-agent's worker-thread mirror writes each tool result with `sender: "user"` and no visibility flag. The renderer maps every non-`"agent"` sender to `role: 'user'` (`assistantUiMessages.ts:536`) and filters only on `extraMetadata.hidden` (`:664`, plus `ChatThreadView.tsx:312` and `timeline/selectors.ts:50`), and worker threads are openable chats — `create_worker_thread` stamps `labels: ["tasks"]` and `threadFilter.ts` lists that tab. So a tool's raw JSON output is painted in a right-aligned bubble as something the human typed. Set `hidden` in `append_worker_message` for any non-`"agent"` sender rather than at the two `ToolResults` call sites: both mirrors route through it, and the typed path (`mirror_worker_thread`) and the error-recovery path (`mirror_worker_thread_from_history`) had the same defect. The row stays in the JSONL 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 the delegation prompt still shows. The sibling arms are correctly distinguished and unchanged: `AssistantToolCalls` text, the assistant `Chat` arm and the trailing `extra_final` all send `"agent"` — the sub-agent speaking, not a mirrored record. Item 2 of #5934 (narration rendered as plain text) was fixed by #6169. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016ipSkHi47nsBmnxX5dC4dA --- .../subagent_runner/ops/graph_part_02.rs | 10 ++ .../subagent_runner/ops/graph_tests.rs | 106 ++++++++++++++++++ 2 files changed, 116 insertions(+) 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); +} From c9abb1d515ade4057bc75b9396eccaafb5cc950e Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Thu, 10 Sep 2026 18:55:01 +0530 Subject: [PATCH 2/2] test(agent): resolve the orchestrator from the bundled TOML, not the global registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `memory_access_instruction_is_present_with_learning_disabled` passed run alone and failed inside the full `openhuman::agent` run, which is what the `Rust Core Coverage` lane executes when a PR touches `agent/`. Reproduced on `upstream/main` with no other change present, so it is pre-existing rather than caused by the mirror fix in this branch. Cause: the test passed `None` for the agent definition, so the factory resolved `"orchestrator"` from `AgentDefinitionRegistry`'s `static GLOBAL: OnceLock<_>` (`harness/definition_part_02.rs:24`) — first-write-wins, never reset. It was therefore asserting against whichever definition set another test in the binary installed first. The section is gated on `memory_recall` being registered *and* visible after tool filtering, and the visible set comes from the resolved definition's tool scope, so a poisoned registry silently dropped the section. Fix: pass `builtin_def("orchestrator")`, the helper written for exactly this hazard — it loads fresh from the bundled TOML, "entirely independent of the global registry singleton". This is what the sibling write-side test in `builder_tests_part_03_tests.rs` already does, which is why that one never flaked. Not weakened and not ignored: `learning.enabled` is still forced off and the assertion is unchanged. Verified by disabling `MemoryAccessSection`'s registration in `helpers.rs`, which makes the test fail on this same assertion. Full `openhuman::agent` scope under the product feature set: 2391 passed, 0 failed (was 2390 passed, 1 failed). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016ipSkHi47nsBmnxX5dC4dA --- .../builder/builder_tests_part_01_tests.rs | 33 +++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) 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"