diff --git a/src/openhuman/agent/harness/session/builder/builder_tests.rs b/src/openhuman/agent/harness/session/builder/builder_tests.rs index 06bbd26cc..7e44eebcd 100644 --- a/src/openhuman/agent/harness/session/builder/builder_tests.rs +++ b/src/openhuman/agent/harness/session/builder/builder_tests.rs @@ -240,3 +240,71 @@ async fn build_session_agent_falls_back_to_global_default_when_no_definition() { "with no definition, the global config default must be used unchanged" ); } + +// ── #5050 Fix 1: shared `Arc` for the per-build tool config ────────── + +#[test] +fn tool_config_shares_base_arc_when_ui_control_toggle_off() { + use super::factory::resolve_tool_config; + use std::sync::Arc; + + let tmp = tempfile::TempDir::new().unwrap(); + let mut cfg = test_config(&tmp); + cfg.computer_control.ax_interact_mutations = false; + let base = Arc::new(cfg); + + // No enabled tools → the App-UI-Control toggle does not fire → the tool + // registry shares the base `Arc` (a refcount bump), not a deep clone. + let resolved = resolve_tool_config(&base, &[]); + assert!( + Arc::ptr_eq(&base, &resolved), + "toggle off must reuse the base config Arc rather than deep-clone it" + ); +} + +#[test] +fn tool_config_grant_is_scoped_and_leaves_base_untouched() { + use super::factory::resolve_tool_config; + use std::sync::Arc; + + let tmp = tempfile::TempDir::new().unwrap(); + let mut cfg = test_config(&tmp); + cfg.computer_control.ax_interact_mutations = false; + let base = Arc::new(cfg); + + // Enabling `ax_interact` fires the toggle: the tool registry gets the mutation + // grant, but as a *distinct* instance — the base config (which feeds the turn + // provider + reflection hook) must stay ungranted so the grant cannot leak. + let resolved = resolve_tool_config(&base, &["ax_interact".to_string()]); + assert!( + resolved.computer_control.ax_interact_mutations, + "the tool-registry config must carry the granted mutation flag" + ); + assert!( + !Arc::ptr_eq(&base, &resolved), + "granting must produce a distinct config, not alias the shared base" + ); + assert!( + !base.computer_control.ax_interact_mutations, + "the base config must stay ungranted — the grant is scoped to the tool registry" + ); +} + +#[test] +fn tool_config_reuses_base_when_mutations_already_granted_globally() { + use super::factory::resolve_tool_config; + use std::sync::Arc; + + let tmp = tempfile::TempDir::new().unwrap(); + let mut cfg = test_config(&tmp); + cfg.computer_control.ax_interact_mutations = true; + let base = Arc::new(cfg); + + // Already granted globally (e.g. Full autonomy) → no clone even when the tool + // is enabled, since there is nothing to grant. + let resolved = resolve_tool_config(&base, &["ax_interact".to_string()]); + assert!( + Arc::ptr_eq(&base, &resolved), + "an already-granted base config must not be re-cloned" + ); +} diff --git a/src/openhuman/agent/harness/session/builder/factory.rs b/src/openhuman/agent/harness/session/builder/factory.rs index 2c02861a2..a711f138b 100644 --- a/src/openhuman/agent/harness/session/builder/factory.rs +++ b/src/openhuman/agent/harness/session/builder/factory.rs @@ -391,23 +391,17 @@ impl Agent { // (#3762). The actions stay approval-gated and bound by the // sensitive-app denylist; Full autonomy continues to grant this // independently via `app_control_enabled`. - let adjusted_config: Config; - let tool_config: &Config = if !config.computer_control.ax_interact_mutations - && tools::enables_app_ui_control_mutations(&enabled_tools) - { - let mut c = config.clone(); - c.computer_control.ax_interact_mutations = true; - log::debug!( - "[session-builder] action=grant_app_ui_control_mutations source=features_toggle" - ); - adjusted_config = c; - &adjusted_config - } else { - config - }; + // Share a single `Arc` across the heavyweight per-build consumers + // (the tool registry, the reflection hook, the turn provider) instead of + // deep-cloning the large `Config` at each site (#5050, Fix 1). `Config` is + // immutable after construction, so one refcounted instance is behaviourally + // identical to N independent clones. `resolve_tool_config` handles the one + // consumer that needs a *different* config — the App-UI-Control toggle. + let base_config: Arc = Arc::new(config.clone()); + let tool_config: Arc = resolve_tool_config(&base_config, &enabled_tools); let mut tools = tools::all_tools_with_runtime( - Arc::new(tool_config.clone()), + Arc::clone(&tool_config), &security, runtime, audit, @@ -416,7 +410,7 @@ impl Agent { &tool_config.http_request, &tool_config.action_dir, &tool_config.agents, - tool_config, + &tool_config, profile_skill_allowlist.as_ref(), profile_mcp_allowlist.as_deref(), ); @@ -694,11 +688,10 @@ impl Agent { Vec::new(); if config.learning.enabled { if config.learning.reflection_enabled { - // Only the reflection hook needs an owned snapshot of the - // full config, so create the `Arc` lazily inside this - // branch instead of paying for the clone whenever - // `learning.enabled` is true. - let full_config = Arc::new(config.clone()); + // The reflection hook needs an owned `Arc`; reuse the + // shared base config (a refcount bump) rather than a second deep + // clone of the full config (#5050, Fix 1). + let full_config = Arc::clone(&base_config); // For cloud reflection, wrap the provider in an Arc. // For local, no provider needed. let reflection_provider: Option< @@ -1164,7 +1157,7 @@ impl Agent { effective_agent_config.max_tool_iterations = def_cap; } let mut builder = Agent::builder() - .crate_native_provider(provider_role, std::sync::Arc::new(config.clone())) + .crate_native_provider(provider_role, Arc::clone(&base_config)) .tools(tools) .visible_tool_names(visible) .memory(memory) @@ -1221,6 +1214,41 @@ impl Agent { } } +/// Resolve the `Config` the tool registry is built from (#5050, Fix 1). +/// +/// Normally this is the shared `base_config` — returned as a refcount bump, not a +/// deep clone. The one exception is the App-UI-Control / App-Automation features +/// toggle (#3762): when the user enabled the `ax_interact` / `automate` tools in +/// Settings without global Full autonomy, the tool registry (and *only* the tool +/// registry) receives a copy of the config with `ax_interact_mutations` granted. +/// Scoping the grant here keeps the turn provider and reflection hook on the +/// ungranted base config, and clones at most once — only when the toggle fires. +pub(super) fn resolve_tool_config( + base_config: &Arc, + enabled_tools: &[String], +) -> Arc { + log::trace!( + "[session-builder] action=resolve_tool_config phase=enter enabled_tools_count={} base_ax_interact_mutations={}", + enabled_tools.len(), + base_config.computer_control.ax_interact_mutations + ); + if !base_config.computer_control.ax_interact_mutations + && tools::enables_app_ui_control_mutations(enabled_tools) + { + let mut granted = (**base_config).clone(); + granted.computer_control.ax_interact_mutations = true; + log::debug!( + "[session-builder] action=resolve_tool_config phase=exit outcome=granted_app_ui_control_mutations source=features_toggle" + ); + Arc::new(granted) + } else { + log::debug!( + "[session-builder] action=resolve_tool_config phase=exit outcome=reused_base_config" + ); + Arc::clone(base_config) + } +} + fn definition_disallows_tool(disallowed: &[String], name: &str) -> bool { disallowed.iter().any(|entry| { if let Some(prefix) = entry.strip_suffix('*') {