diff --git a/src/openhuman/agent/harness/session/builder.rs b/src/openhuman/agent/harness/session/builder.rs index 739eb9de16..ba5ea83818 100644 --- a/src/openhuman/agent/harness/session/builder.rs +++ b/src/openhuman/agent/harness/session/builder.rs @@ -48,6 +48,7 @@ impl AgentBuilder { event_session_id: None, event_channel: None, agent_definition_name: None, + base_definition_id: None, session_parent_prefix: None, omit_profile: None, omit_memory_md: None, @@ -233,6 +234,20 @@ impl AgentBuilder { self } + /// Sets the original agent definition id as it appears in the global + /// [`AgentDefinitionRegistry`]. This is distinct from + /// [`Self::agent_definition_name`] which may later be overwritten + /// to a thread-scoped variant (e.g. `"orchestrator_thread-6ad6d"`). + /// When not set, [`AgentBuilder::build`] falls back to + /// `agent_definition_name` and ultimately to `"main"`. + /// + /// Used by [`Agent::refresh_delegation_tools`] to look up the + /// original archetype definition for delegation-tool re-synthesis. + pub fn base_definition_id(mut self, id: impl Into) -> Self { + self.base_definition_id = Some(id.into()); + self + } + /// Set the parent session-key chain for a sub-agent. Passing /// `Some("1713000000_orchestrator")` produces a sub-agent whose /// transcript filename is prefixed with the parent's session key, @@ -380,6 +395,10 @@ impl AgentBuilder { .agent_definition_name .clone() .unwrap_or_else(|| "main".to_string()), + base_definition_id: self + .base_definition_id + .or_else(|| self.agent_definition_name.clone()) + .unwrap_or_else(|| "main".to_string()), session_transcript_path: None, session_key: { let unix_ts = std::time::SystemTime::now() @@ -869,9 +888,17 @@ impl Agent { // Tauri-web code path. It does not have access to the async // Composio fetcher, so we pass an empty slice of connected // integrations here — the skill-wildcard expansion therefore - // produces zero delegation tools. That is correct behaviour: - // callers that need live integration expansion go through the - // bus-based `channels::runtime::dispatch` path instead. + // produces zero `delegate_{toolkit}` tools at construction time. + // + // The gap is closed at runtime: on the first turn, + // `fetch_connected_integrations()` populates + // `self.connected_integrations`, and the immediately-following + // `refresh_delegation_tools()` call re-runs + // `collect_orchestrator_tools` with the live integrations list + // and merges any newly-synthesised `delegate_*` tools into the + // agent's `tools`, `tool_specs`, `visible_tool_names`, and + // `visible_tool_specs` surfaces so the LLM's function-calling + // schema is consistent with the system prompt. let (delegation_tools, filter_from_scope): ( Vec>, Option>, @@ -1173,6 +1200,11 @@ impl Agent { .post_turn_hooks(post_turn_hooks) .learning_enabled(config.learning.enabled) .agent_definition_name(agent_id.to_string()) + // Freeze the original registry id so `refresh_delegation_tools` + // can look up the archetype definition even after + // `set_agent_definition_name` has mangled the display name to a + // thread-scoped variant (e.g. `"orchestrator_thread-6ad6d"`). + .base_definition_id(agent_id.to_string()) .omit_profile(effective_omit_profile) .omit_memory_md(effective_omit_memory_md); if let Some(ps) = payload_summarizer { diff --git a/src/openhuman/agent/harness/session/runtime.rs b/src/openhuman/agent/harness/session/runtime.rs index 67c106ca27..2d616beb9d 100644 --- a/src/openhuman/agent/harness/session/runtime.rs +++ b/src/openhuman/agent/harness/session/runtime.rs @@ -51,6 +51,18 @@ impl Agent { &self.agent_definition_name } + /// The original agent definition id from the global registry, frozen + /// at build time (e.g. `"orchestrator"`). Unlike + /// [`Agent::agent_definition_name`] this is never overwritten by + /// [`Agent::set_agent_definition_name`], so it always points to + /// the archetype that was looked up in the registry when the + /// session was constructed. Used by + /// [`Agent::refresh_delegation_tools`] to re-resolve the definition + /// after the display name has been mangled. + pub fn base_definition_id(&self) -> &str { + &self.base_definition_id + } + /// Returns a new `AgentBuilder`. pub fn builder() -> AgentBuilder { AgentBuilder::new() diff --git a/src/openhuman/agent/harness/session/tests.rs b/src/openhuman/agent/harness/session/tests.rs index 627cf4c7bd..2fd458b4ea 100644 --- a/src/openhuman/agent/harness/session/tests.rs +++ b/src/openhuman/agent/harness/session/tests.rs @@ -694,6 +694,182 @@ fn seed_resume_from_messages_is_noop_on_warm_agent() { assert_eq!(cached[0].content, "warm prefix"); } +// ───────────────────────────────────────────────────────────────────────────── +// refresh_delegation_tools tests +// ───────────────────────────────────────────────────────────────────────────── + +/// Build a minimal agent scoped to `agent_id` with the global builtin registry +/// initialised. Helper shared by the `refresh_delegation_tools` tests below. +fn build_agent_with_base_definition(agent_id: &str) -> Agent { + use crate::openhuman::agent::harness::AgentDefinitionRegistry; + // Idempotent — other tests may have already initialised the registry. + AgentDefinitionRegistry::init_global_builtins().unwrap(); + + let workspace = tempfile::TempDir::new().expect("temp workspace"); + let workspace_path = workspace.path().to_path_buf(); + let provider = Box::new(MockProvider { + responses: parking_lot::Mutex::new(vec![]), + }); + let memory_cfg = crate::openhuman::config::MemoryConfig { + backend: "none".into(), + ..crate::openhuman::config::MemoryConfig::default() + }; + let mem: Arc = + Arc::from(crate::openhuman::memory::create_memory(&memory_cfg, &workspace_path).unwrap()); + Agent::builder() + .provider(provider) + .tools(vec![Box::new(MockTool)]) + .memory(mem) + .tool_dispatcher(Box::new(NativeToolDispatcher)) + .workspace_dir(workspace_path) + .agent_definition_name(agent_id.to_string()) + .base_definition_id(agent_id.to_string()) + .build() + .expect("agent build should succeed") +} + +/// `refresh_delegation_tools` must add `delegate_{toolkit}` tools to the +/// agent's tool surface when `connected_integrations` is non-empty and the +/// agent's definition has a `SubagentEntry::Skills` wildcard entry. +/// +/// The orchestrator builtin definition declares `{ skills = "*" }` in its +/// `subagents` list, so passing connected gmail + github integrations should +/// produce `delegate_gmail` and `delegate_github` in both +/// `visible_tool_names` and `visible_tool_specs`. +#[test] +fn refresh_delegation_tools_populates_skill_tools() { + use crate::openhuman::context::prompt::ConnectedIntegration; + + let mut agent = build_agent_with_base_definition("orchestrator"); + + // Inject two connected integrations before calling the method. + agent.connected_integrations = vec![ + ConnectedIntegration { + toolkit: "gmail".to_string(), + description: "Gmail integration".to_string(), + tools: vec![], + connected: true, + }, + ConnectedIntegration { + toolkit: "github".to_string(), + description: "GitHub integration".to_string(), + tools: vec![], + connected: true, + }, + ]; + + agent.refresh_delegation_tools(); + + // tool_specs must include the new delegation tools. + let spec_names: Vec<&str> = agent.tool_specs.iter().map(|s| s.name.as_str()).collect(); + assert!( + spec_names.contains(&"delegate_gmail"), + "tool_specs should contain delegate_gmail after refresh; got: {spec_names:?}" + ); + assert!( + spec_names.contains(&"delegate_github"), + "tool_specs should contain delegate_github after refresh; got: {spec_names:?}" + ); + + // visible_tool_specs must also include them (rebuilt from tool_specs + // when no name filter is active). + let visible_names: Vec<&str> = agent + .visible_tool_specs + .iter() + .map(|s| s.name.as_str()) + .collect(); + assert!( + visible_names.contains(&"delegate_gmail"), + "visible_tool_specs should contain delegate_gmail; got: {visible_names:?}" + ); + assert!( + visible_names.contains(&"delegate_github"), + "visible_tool_specs should contain delegate_github; got: {visible_names:?}" + ); + + // The tools vec must also carry the new tools so dispatch works. + let tool_names: Vec<&str> = agent.tools.iter().map(|t| t.name()).collect(); + assert!( + tool_names.contains(&"delegate_gmail"), + "tools vec should contain delegate_gmail; got: {tool_names:?}" + ); +} + +/// `refresh_delegation_tools` must be a no-op when `connected_integrations` +/// is empty — the tool surface must remain unchanged. +#[test] +fn refresh_delegation_tools_noop_when_no_integrations() { + let mut agent = build_agent_with_base_definition("orchestrator"); + + // Confirm no integrations are set (the builder default). + assert!(agent.connected_integrations.is_empty()); + + let tools_before = agent.tools.len(); + let specs_before = agent.tool_specs.len(); + + agent.refresh_delegation_tools(); + + assert_eq!( + agent.tools.len(), + tools_before, + "tools vec should not grow when connected_integrations is empty" + ); + assert_eq!( + agent.tool_specs.len(), + specs_before, + "tool_specs vec should not grow when connected_integrations is empty" + ); +} + +/// `refresh_delegation_tools` must not introduce duplicates when a +/// `delegate_*` tool is already registered on the agent (e.g. from a +/// prior call or from the builder). +/// +/// Calls `refresh_delegation_tools` twice with the same integrations list — +/// the second call must not add any new tools. +#[test] +fn refresh_delegation_tools_skips_already_registered() { + use crate::openhuman::context::prompt::ConnectedIntegration; + + let mut agent = build_agent_with_base_definition("orchestrator"); + agent.connected_integrations = vec![ConnectedIntegration { + toolkit: "gmail".to_string(), + description: "Gmail integration".to_string(), + tools: vec![], + connected: true, + }]; + + // First call — should add delegate_gmail. + agent.refresh_delegation_tools(); + let tools_after_first = agent.tools.len(); + let specs_after_first = agent.tool_specs.len(); + + // Second call with the same integrations — must be a no-op. + agent.refresh_delegation_tools(); + + assert_eq!( + agent.tools.len(), + tools_after_first, + "second refresh_delegation_tools call must not duplicate tools" + ); + assert_eq!( + agent.tool_specs.len(), + specs_after_first, + "second refresh_delegation_tools call must not duplicate tool_specs" + ); + + // Confirm only one `delegate_gmail` exists. + let gmail_count = agent + .tools + .iter() + .filter(|t| t.name() == "delegate_gmail") + .count(); + assert_eq!( + gmail_count, 1, + "exactly one delegate_gmail should exist after two refresh calls; found {gmail_count}" + ); +} + /// Trailing user message that does NOT match the current incoming /// message must be preserved — the dedup heuristic only fires on /// exact match because the conversation JSONL is the source of truth diff --git a/src/openhuman/agent/harness/session/turn.rs b/src/openhuman/agent/harness/session/turn.rs index 7f66d2e21a..0103ba47b8 100644 --- a/src/openhuman/agent/harness/session/turn.rs +++ b/src/openhuman/agent/harness/session/turn.rs @@ -82,6 +82,7 @@ impl Agent { // inference backend has already tokenised. Fetching it later // would just burn memory-store reads on data we throw away. self.fetch_connected_integrations().await; + self.refresh_delegation_tools(); let learned = self.fetch_learned_context().await; let rendered_prompt = self.build_system_prompt(learned)?; log::info!("[agent] system prompt built — initialising conversation history"); @@ -1228,6 +1229,151 @@ impl Agent { } } + /// Re-synthesise skill-delegation tools (`delegate_gmail`, + /// `delegate_notion`, …) using the live `connected_integrations` list + /// and merge them into the agent's tool surface. + /// + /// Called on the first turn, right after + /// [`Agent::fetch_connected_integrations`] has populated + /// `self.connected_integrations`. The synchronous builder cannot call + /// the async Composio fetcher, so it passes `&[]` at construction time — + /// this method closes that gap before the system prompt is built, + /// ensuring that `delegate_{toolkit}` tools are present in the + /// function-calling schema wherever they are mentioned in the prompt. + pub(super) fn refresh_delegation_tools(&mut self) { + if self.connected_integrations.is_empty() { + log::debug!( + "[refresh_delegation_tools] connected_integrations is empty \ + — skipping delegation tool refresh" + ); + return; + } + + let registry = + match crate::openhuman::agent::harness::definition::AgentDefinitionRegistry::global() { + Some(r) => r, + None => { + log::warn!( + "[refresh_delegation_tools] no global agent registry \ + — skipping delegation tool refresh" + ); + return; + } + }; + + let definition = match registry.get(&self.base_definition_id) { + Some(d) => d, + None => { + log::debug!( + "[refresh_delegation_tools] no definition for '{}' in registry \ + — skipping delegation tool refresh", + self.base_definition_id + ); + return; + } + }; + + // Collect the full set of orchestrator tools with live integrations. + let all_tools = crate::openhuman::tools::orchestrator_tools::collect_orchestrator_tools( + definition, + registry, + &self.connected_integrations, + ); + + // Determine which tool names we already have so we don't duplicate. + // Borrow the Arc directly — no Clone needed. + let existing_names: std::collections::HashSet<&str> = + self.tools.iter().map(|t| t.name()).collect(); + + // Filter to only genuinely new tools (the skill-delegation ones). + let new_tools: Vec> = all_tools + .into_iter() + .filter(|t| !existing_names.contains(t.name())) + .collect(); + + if new_tools.is_empty() { + log::debug!( + "[refresh_delegation_tools] no new delegation tools to add \ + (all already registered or no Skills subagent entries)" + ); + return; + } + + log::info!( + "[refresh_delegation_tools] adding {} skill-delegation tool(s): {}", + new_tools.len(), + new_tools + .iter() + .map(|t| t.name()) + .collect::>() + .join(", ") + ); + + // Generate specs and collect names for the visibility set. + let new_specs: Vec = + new_tools.iter().map(|t| t.spec()).collect(); + let new_names: Vec = new_tools.iter().map(|t| t.name().to_string()).collect(); + + // Replace the tools Arc with a new one that includes the new tools. + // We must rebuild the Vec because Box is not Clone, so + // Arc::make_mut is unavailable. Take the existing Arc (or clone the + // pointer) and extend a fresh Vec from it. + // + // Safety note: `Arc::try_unwrap` succeeds only when this is the sole + // strong reference. In the turn loop we are always the only owner + // (sub-agents hold their own Arcs taken at turn-start via + // `tools_arc()`), but to be safe we fall back to an empty extension + // on the rare occasion it fails. + let mut tools_vec: Vec> = + match Arc::try_unwrap(std::mem::replace(&mut self.tools, Arc::new(Vec::new()))) { + Ok(v) => v, + Err(shared_arc) => { + // Another reference exists — we cannot reclaim the Vec. + // Restore the original Arc and skip the tools update. + // tool_specs and visible names are still updated below. + log::warn!( + "[refresh_delegation_tools] tools Arc is shared — \ + cannot extend tools vec in place; \ + tool_specs + visible names will still be updated" + ); + self.tools = shared_arc; + Vec::new() + } + }; + tools_vec.extend(new_tools); + self.tools = Arc::new(tools_vec); + + // Extend tool_specs (ToolSpec is Clone, so Arc::make_mut works). + Arc::make_mut(&mut self.tool_specs).extend(new_specs); + + // Extend visible_tool_names (only when the filter is active; + // an empty set means "no filter / all tools visible"). + if !self.visible_tool_names.is_empty() { + for name in &new_names { + self.visible_tool_names.insert(name.clone()); + } + } + + // Rebuild visible_tool_specs from the updated tool_specs and + // the (possibly-updated) visible_tool_names. + self.visible_tool_specs = Arc::new(if self.visible_tool_names.is_empty() { + (*self.tool_specs).clone() + } else { + self.tool_specs + .iter() + .filter(|s| self.visible_tool_names.contains(&s.name)) + .cloned() + .collect() + }); + + log::debug!( + "[refresh_delegation_tools] tool surface updated: \ + total_tools={} visible_specs={}", + self.tools.len(), + self.visible_tool_specs.len() + ); + } + /// Fetches the user's active Composio connections and populates /// `self.connected_integrations` so the system prompt can surface them. /// Also caches a [`ComposioClient`] on the session so the sub-agent diff --git a/src/openhuman/agent/harness/session/types.rs b/src/openhuman/agent/harness/session/types.rs index 67094172ca..b9050fd2b3 100644 --- a/src/openhuman/agent/harness/session/types.rs +++ b/src/openhuman/agent/harness/session/types.rs @@ -72,6 +72,17 @@ pub struct Agent { /// `"code_executor"`). Used as the `{agent}` component in session /// transcript paths: `sessions/DDMMYYYY/{agent}_{index}.md`. pub(super) agent_definition_name: String, + /// The original agent definition id as it appears in the global + /// [`AgentDefinitionRegistry`] (e.g. `"orchestrator"`). This is + /// distinct from [`Self::agent_definition_name`], which may be + /// overwritten by [`Agent::set_agent_definition_name`] to a + /// thread-scoped name like `"orchestrator_thread-6ad6d"` for + /// transcript namespacing. `base_definition_id` is frozen at + /// build time and used by [`Agent::refresh_delegation_tools`] to + /// look up the original definition — so the delegation-tool + /// refresh always targets the right archetype even after the name + /// has been mangled. + pub(super) base_definition_id: String, /// Resolved filesystem path for this session's transcript file. /// Set on first write, reused for subsequent overwrites within the /// same session. @@ -165,6 +176,11 @@ pub struct AgentBuilder { pub(super) event_session_id: Option, pub(super) event_channel: Option, pub(super) agent_definition_name: Option, + /// The original agent definition id from the registry. Frozen at + /// build time and used by [`Agent::refresh_delegation_tools`] for + /// definition lookups. Falls back to `agent_definition_name` when + /// not explicitly set, and ultimately to `"main"`. + pub(super) base_definition_id: Option, /// Directory chain of parent session keys for a sub-agent. `None` /// (default) means this is a root session — its transcript lands /// flat in `session_raw/DDMMYYYY/{session_key}.jsonl`. Populated @@ -208,6 +224,7 @@ mod tests { assert!(builder.event_session_id.is_none()); assert!(builder.event_channel.is_none()); assert!(builder.agent_definition_name.is_none()); + assert!(builder.base_definition_id.is_none()); assert!(builder.post_turn_hooks.is_empty()); } }