Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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"
Expand Down
10 changes: 10 additions & 0 deletions src/openhuman/agent/harness/subagent_runner/ops/graph_part_02.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
106 changes: 106 additions & 0 deletions src/openhuman/agent/harness/subagent_runner/ops/graph_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> = 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);
}
Loading