diff --git a/src/openhuman/agent/harness/definition_tests.rs b/src/openhuman/agent/harness/definition_tests.rs index 9bf9c3cd9d..ed905d26dc 100644 --- a/src/openhuman/agent/harness/definition_tests.rs +++ b/src/openhuman/agent/harness/definition_tests.rs @@ -342,3 +342,99 @@ fn tier_display_matches_as_str() { assert_eq!(AgentTier::Reasoning.to_string(), "reasoning"); assert_eq!(AgentTier::Worker.to_string(), "worker"); } + +// ── Issue #4868 audit snapshot ────────────────────────────────────────────── +// +// The systemic fix in `build_session_agent_inner` makes every direct- +// invocation call site (flows_build, flows_discover, agent-node runtime, +// cron, MCP server, …) honor each agent's own `effective_max_iterations()` +// instead of the global `config.agent.max_tool_iterations` default (10). +// That changes the *runtime* iteration cap for every agent in the registry — +// some go up (extended policy), some go down (declared max_iterations < 10), +// most stay the same. This test pins the exact expected cap for every +// built-in agent so an accidental `agent.toml` edit (or a new agent landing +// with an unreviewed cap) shows up as a diff here rather than silently +// changing runtime behavior. See PLAN/PR #4868 for the full before/after +// audit table this list mirrors. +#[test] +fn all_builtin_agent_definitions_have_expected_effective_max_iterations() { + let defs = crate::openhuman::agent_registry::agents::load_builtins() + .expect("built-in agent TOML must always parse"); + + let expected: &[(&str, usize)] = &[ + // Extended policy (or high `max_iterations`) -> effective cap raised. + ("orchestrator", 15), + ("code_executor", 50), + ("context_scout", 50), + ("integrations_agent", 50), + ("mcp_agent", 50), + ("mcp_setup", 50), + ("planner", 50), + ("researcher", 50), + ("skill_creator", 50), + ("task_manager_agent", 50), + ("tools_agent", 50), + ("flow_discovery", 50), + ("workflow_builder", 50), + ("skill_executor", 50), + ("tinyplace_agent", 50), + ("subconscious", 30), + // Strict policy, declared `max_iterations` below the old global + // default (10) -> effective cap lowered. + ("agent_memory", 6), + ("account_admin_agent", 8), + ("archivist", 3), + ("critic", 5), + ("crypto_agent", 8), + ("desktop_control_agent", 8), + ("goals_agent", 5), + ("help", 6), + ("image_agent", 8), + ("markets_agent", 8), + ("morning_briefing", 8), + ("profile_memory_agent", 8), + ("scheduler_agent", 8), + ("screen_awareness_agent", 8), + ("settings_agent", 8), + ("summarizer", 1), + ("tool_maker", 2), + ("trigger_reactor", 6), + ("trigger_triage", 2), + ("video_agent", 8), + ("vision_agent", 6), + // Unchanged. + ("presentation_agent", 10), + ("skill_setup", 10), + ]; + + for (id, expected_cap) in expected { + let def = defs + .iter() + .find(|d| d.id == *id) + .unwrap_or_else(|| panic!("missing built-in agent definition: {id}")); + assert_eq!( + def.effective_max_iterations(), + *expected_cap, + "agent '{id}' effective_max_iterations() mismatch — expected {expected_cap}, got {} \ + (max_iterations={}, iteration_policy={:?}). If this agent's cap was intentionally \ + changed, update this snapshot; otherwise an agent.toml edit just silently changed \ + its runtime iteration budget.", + def.effective_max_iterations(), + def.max_iterations, + def.iteration_policy, + ); + } + + // Exhaustiveness: every built-in id must appear in the expected list + // above (and vice versa) so a newly-added agent can't slip in with an + // unreviewed cap. + let mut expected_ids: Vec<&str> = expected.iter().map(|(id, _)| *id).collect(); + expected_ids.sort_unstable(); + let mut actual_ids: Vec<&str> = defs.iter().map(|d| d.id.as_str()).collect(); + actual_ids.sort_unstable(); + assert_eq!( + actual_ids, expected_ids, + "the set of built-in agent ids changed — add/remove the new agent from this audit \ + snapshot's `expected` list with a deliberate effective_max_iterations() entry" + ); +} diff --git a/src/openhuman/agent/harness/session/builder/builder_tests.rs b/src/openhuman/agent/harness/session/builder/builder_tests.rs index b375d42978..06bbd26cc6 100644 --- a/src/openhuman/agent/harness/session/builder/builder_tests.rs +++ b/src/openhuman/agent/harness/session/builder/builder_tests.rs @@ -125,3 +125,118 @@ fn automatic_memory_policy_does_not_synthesize_delegate_tools() { "orchestrator still needs synthesized delegate tools" ); } + +// ───────────────────────────────────────────────────────────────────────────── +// Issue #4868 — `build_session_agent_inner` must resolve the iteration cap +// from the target `AgentDefinition`'s `effective_max_iterations()`, not the +// global `config.agent.max_tool_iterations` default. These tests drive +// `build_session_agent_inner` directly with a hand-picked `target_def` +// (`pub(crate)` for exactly this purpose), independent of the process-global +// `AgentDefinitionRegistry` singleton's init-once state. +// ───────────────────────────────────────────────────────────────────────────── + +fn test_config(tmp: &tempfile::TempDir) -> crate::openhuman::config::Config { + let config = crate::openhuman::config::Config { + workspace_dir: tmp.path().join("workspace"), + action_dir: tmp.path().join("workspace"), + config_path: tmp.path().join("config.toml"), + ..crate::openhuman::config::Config::default() + }; + std::fs::create_dir_all(&config.workspace_dir).unwrap(); + config +} + +/// Look up a real built-in `AgentDefinition` by id — loaded fresh from the +/// bundled TOML files, entirely independent of the global registry +/// singleton (so tests can't be poisoned by another test's +/// `AgentDefinitionRegistry::init_global*` call, and can't poison later ones). +fn builtin_def(id: &str) -> crate::openhuman::agent::harness::definition::AgentDefinition { + crate::openhuman::agent_registry::agents::load_builtins() + .unwrap() + .into_iter() + .find(|def| def.id == id) + .unwrap_or_else(|| panic!("builtin agent definition not found: {id}")) +} + +#[tokio::test] +async fn build_session_agent_applies_extended_policy_definition_cap() { + use crate::openhuman::agent::harness::session::types::Agent; + + let tmp = tempfile::TempDir::new().unwrap(); + let config = test_config(&tmp); + assert_eq!( + config.agent.max_tool_iterations, 10, + "precondition: global default must be 10 for this test to distinguish the two" + ); + + // `code_executor` declares `iteration_policy = "extended"` with + // `max_iterations = 10` in its agent.toml, so its effective cap is + // `EXTENDED_MAX_TOOL_ITERATIONS` (50) — not the raw `max_iterations`. + let def = builtin_def("code_executor"); + assert_eq!(def.effective_max_iterations(), 50); + + let agent = Agent::build_session_agent_inner( + &config, + "code_executor", + Some(&def), + None, + None, + false, + None, + ) + .expect("build_session_agent_inner should succeed for a valid extended-policy definition"); + + assert_eq!( + agent.agent_config().max_tool_iterations, + 50, + "extended-policy agent must carry its definition's effective cap (50), not the global \ + default (10)" + ); +} + +#[tokio::test] +async fn build_session_agent_applies_strict_cap_below_global_default() { + use crate::openhuman::agent::harness::session::types::Agent; + + let tmp = tempfile::TempDir::new().unwrap(); + let config = test_config(&tmp); + assert_eq!(config.agent.max_tool_iterations, 10); + + // `archivist` is strict-policy with a declared `max_iterations = 3` — + // well below the global default of 10. The definition cap must still + // win (lowering the runtime cap), not just raise it. + let def = builtin_def("archivist"); + assert_eq!(def.effective_max_iterations(), 3); + + let agent = + Agent::build_session_agent_inner(&config, "archivist", Some(&def), None, None, false, None) + .expect("build_session_agent_inner should succeed for a valid strict-low definition"); + + assert_eq!( + agent.agent_config().max_tool_iterations, + 3, + "strict-policy agent below the global default must still get its own (lower) cap" + ); +} + +#[tokio::test] +async fn build_session_agent_falls_back_to_global_default_when_no_definition() { + use crate::openhuman::agent::harness::session::types::Agent; + + let tmp = tempfile::TempDir::new().unwrap(); + let config = test_config(&tmp); + assert_eq!(config.agent.max_tool_iterations, 10); + + // No `target_def` at all (e.g. registry not yet initialised, or a + // legacy caller that never resolved one) — must fall back to the + // unmodified global `config.agent.max_tool_iterations`. + let agent = + Agent::build_session_agent_inner(&config, "orchestrator", None, None, None, false, None) + .expect("build_session_agent_inner should succeed with no definition"); + + assert_eq!( + agent.agent_config().max_tool_iterations, + 10, + "with no definition, the global config default must be used unchanged" + ); +} diff --git a/src/openhuman/agent/harness/session/builder/factory.rs b/src/openhuman/agent/harness/session/builder/factory.rs index 3d8b55bb39..e454a80b20 100644 --- a/src/openhuman/agent/harness/session/builder/factory.rs +++ b/src/openhuman/agent/harness/session/builder/factory.rs @@ -300,8 +300,13 @@ impl Agent { /// the subconscious LLM cited when it produced the spawning /// reflection (#623). Empty / `None` is the default for normal chat /// threads — the section is omitted entirely. + // `pub(crate)` (rather than private) so `builder_tests` can drive the + // definition-cap resolution logic (issue #4868) directly with a + // hand-picked `target_def`, independent of the process-global + // `AgentDefinitionRegistry` singleton's init-once state. Still not part + // of the crate's public API. #[allow(clippy::too_many_arguments)] - fn build_session_agent_inner( + pub(crate) fn build_session_agent_inner( config: &Config, agent_id: &str, target_def: Option<&crate::openhuman::agent::harness::definition::AgentDefinition>, @@ -1133,6 +1138,31 @@ impl Agent { // a host provider. The // `agent_harness_e2e` mock now serves SSE for streaming, so the crate-native // streaming path is exercised end-to-end. + // + // Issue #4868 — resolve the per-agent iteration cap. When a named + // definition is present, its `effective_max_iterations()` (which honors + // `iteration_policy = "extended"` -> 50, and the declared `max_iterations` + // for strict agents) takes priority over the global + // `config.agent.max_tool_iterations` (default 10). This is the single + // shared resolution point that closes #4868 for every direct-invocation + // path: flows_build, flows_discover, agent-node runtime, cron, MCP + // server, etc. Falls back to the global default when there is no + // definition for this agent_id. + let mut effective_agent_config = config.agent.clone(); + if let Some(def) = target_def { + let def_cap = def.effective_max_iterations(); + log::info!( + "[agent::builder] applying definition iteration cap for agent_id={}: \ + definition.max_iterations={} iteration_policy={:?} -> effective={} \ + (was global default {})", + agent_id, + def.max_iterations, + def.iteration_policy, + def_cap, + config.agent.max_tool_iterations, + ); + effective_agent_config.max_tool_iterations = def_cap; + } let mut builder = Agent::builder() .crate_native_provider(provider_role, std::sync::Arc::new(config.clone())) .tools(tools) @@ -1156,7 +1186,7 @@ impl Agent { ), )) .prompt_builder(prompt_builder) - .config(config.agent.clone()) + .config(effective_agent_config) .context_config(config.context.clone()) .model_name(model_name) .model_vision(model_vision) diff --git a/src/openhuman/agent/harness/session/runtime.rs b/src/openhuman/agent/harness/session/runtime.rs index 0697804943..805e0065cc 100644 --- a/src/openhuman/agent/harness/session/runtime.rs +++ b/src/openhuman/agent/harness/session/runtime.rs @@ -158,6 +158,21 @@ impl Agent { &self.config } + /// Override the agent's tool-iteration cap after construction. + /// + /// Issue #4868 — `build_session_agent_inner` now stamps every agent with + /// its `AgentDefinition::effective_max_iterations()`, which is the correct + /// behavior for direct-invocation call sites. A handful of callers need a + /// *different* cap than the definition's declared budget (e.g. long-running + /// workflow/task-dispatcher runs that intentionally exceed any single + /// agent's normal budget). Those callers should apply their override + /// AFTER construction via this setter, so the shared definition-cap logic + /// in the builder doesn't get silently clobbered by pre-construction + /// mutations (and vice versa). + pub fn set_max_tool_iterations(&mut self, cap: usize) { + self.config.max_tool_iterations = cap; + } + /// Returns the current conversation history. pub fn history(&self) -> &[ConversationMessage] { &self.history diff --git a/src/openhuman/agent/harness/session/tests.rs b/src/openhuman/agent/harness/session/tests.rs index 75142b77ed..6f7b655c9d 100644 --- a/src/openhuman/agent/harness/session/tests.rs +++ b/src/openhuman/agent/harness/session/tests.rs @@ -1309,3 +1309,62 @@ fn hide_tools_seeds_allowlist_when_no_filter_present() { "an absent hidden name is a harmless no-op; visible = {visible:?}" ); } + +// ── Issue #4868 — `set_max_tool_iterations` post-construction override ───── + +/// `set_max_tool_iterations` directly overrides the runtime cap, independent +/// of whatever the builder resolved it to. +#[test] +fn set_max_tool_iterations_overrides_the_builder_resolved_cap() { + let mut agent = build_minimal_agent_with_definition_name(Some("orchestrator")); + let before = agent.agent_config().max_tool_iterations; + + agent.set_max_tool_iterations(200); + + assert_eq!(agent.agent_config().max_tool_iterations, 200); + assert_ne!( + 200, before, + "sanity: the override must actually change the cap for this assertion to mean anything" + ); +} + +/// Regression for issue #4868's `skill_runtime`/`task_dispatcher` callers: +/// both build the agent via `Agent::from_config_for_agent` (which now stamps +/// the resolved agent definition's own `effective_max_iterations()` — 15 for +/// `orchestrator`), then need a much larger budget (200) for a full +/// workflow/autonomous-task run. `set_max_tool_iterations` must win over +/// whatever the session builder resolved, so the post-construction override +/// actually sticks instead of being silently re-clobbered. +#[test] +fn set_max_tool_iterations_survives_after_definition_backed_construction() { + use crate::openhuman::agent::harness::AgentDefinitionRegistry; + + AgentDefinitionRegistry::init_global_builtins().unwrap(); + + let workspace = tempfile::TempDir::new().expect("temp workspace"); + let mut config = crate::openhuman::config::Config { + workspace_dir: workspace.path().to_path_buf(), + action_dir: workspace.path().to_path_buf(), + ..crate::openhuman::config::Config::default() + }; + config.http_request.allowed_domains = vec!["*".to_string()]; + + let mut agent = + Agent::from_config_for_agent(&config, "orchestrator").expect("build orchestrator agent"); + assert_eq!( + agent.agent_config().max_tool_iterations, + 15, + "precondition: the orchestrator definition's own cap (15) is applied by the builder" + ); + + // Mirrors `skill_runtime::run_machinery`/`task_dispatcher::executor`: + // apply the much larger workflow/task-run budget AFTER construction. + const WORKFLOW_RUN_MAX_ITERATIONS: usize = 200; + agent.set_max_tool_iterations(WORKFLOW_RUN_MAX_ITERATIONS); + + assert_eq!( + agent.agent_config().max_tool_iterations, + WORKFLOW_RUN_MAX_ITERATIONS, + "post-construction override must win over the definition-resolved cap" + ); +} diff --git a/src/openhuman/agent/task_dispatcher/executor.rs b/src/openhuman/agent/task_dispatcher/executor.rs index a910c1e93a..f002d8c14a 100644 --- a/src/openhuman/agent/task_dispatcher/executor.rs +++ b/src/openhuman/agent/task_dispatcher/executor.rs @@ -135,7 +135,6 @@ pub(super) async fn run_autonomous( run_id: &str, session_thread_id: Option, ) -> Result { - config.agent.max_tool_iterations = TASK_RUN_MAX_ITERATIONS; // Match skill-run egress handling: only widen to the permissive default // when the operator hasn't configured an explicit allow-list. See the // threat-model note above on why `*` is the default here. @@ -151,6 +150,13 @@ pub(super) async fn run_autonomous( executor.profile.as_ref(), ) .map_err(|e| format!("build agent: {e:#}"))?; + // Issue #4868 — apply the autonomous task-run iteration budget AFTER + // construction. The session builder now stamps the resolved agent + // definition's own cap onto the agent; an autonomous task run + // intentionally needs a much larger budget (200), so this must be a + // post-construction override rather than a pre-set on `config` (which + // the builder would otherwise clobber). + agent.set_max_tool_iterations(TASK_RUN_MAX_ITERATIONS); agent.set_event_context(run_id.to_string(), "task"); agent.set_agent_definition_name(format!( "task-{}-{}", diff --git a/src/openhuman/cron/scheduler.rs b/src/openhuman/cron/scheduler.rs index 7c34333160..d9ef195001 100644 --- a/src/openhuman/cron/scheduler.rs +++ b/src/openhuman/cron/scheduler.rs @@ -884,7 +884,12 @@ async fn run_agent_job(config: &Config, job: &CronJob) -> (bool, String, Option< ModelSpec::Exact(name) => name.clone(), }; effective.default_model = Some(resolved_model); - effective.agent.max_tool_iterations = def.max_iterations; + // Issue #4868 — the iteration cap is no longer set here. The + // session builder (`build_session_agent_inner`) resolves it + // from `def.effective_max_iterations()` directly, which (unlike + // this cron path previously) correctly honors + // `iteration_policy = "extended"` agents (e.g. `tools_agent` + // getting 50, not the raw `max_iterations = 10`). } else { tracing::warn!( job_id = %job.id, diff --git a/src/openhuman/flows/ops.rs b/src/openhuman/flows/ops.rs index 61ec81c24e..b5843a9af9 100644 --- a/src/openhuman/flows/ops.rs +++ b/src/openhuman/flows/ops.rs @@ -3259,11 +3259,12 @@ pub async fn flows_discover( /// own `max_iterations` caps its loop, but a hung LLM/tool call must never let /// the RPC block indefinitely. /// -/// Matches [`FLOW_RUN_TIMEOUT_SECS`] (600s): since the (B31) fix below actually -/// applies the builder's `effective_max_iterations()` (50, not the global -/// default of 10) to this path, a worst-case run at ~10s/iteration can take up -/// to ~500s — the old 300s bound would have clipped a legitimate long build -/// before the iteration cap ever got a chance to. +/// Matches [`FLOW_RUN_TIMEOUT_SECS`] (600s): the session builder applies the +/// `workflow_builder` definition's `effective_max_iterations()` (50, not the +/// global default of 10) to this path (issue #4868), so a worst-case run at +/// ~10s/iteration can take up to ~500s — the old 300s bound would have +/// clipped a legitimate long build before the iteration cap ever got a +/// chance to. const FLOW_BUILD_TIMEOUT_SECS: u64 = 600; /// Tools stripped from the `workflow_builder` belt on the direct `flows_build` @@ -3307,61 +3308,6 @@ fn restrict_builder_toolset(agent: &mut crate::openhuman::agent::Agent) { agent.hide_tools(FLOWS_BUILD_HIDDEN_TOOLS); } -/// (B31) Returns a clone of `config` with `agent.max_tool_iterations` -/// overridden to the `workflow_builder` [`AgentDefinition`](crate::openhuman::agent::harness::definition::AgentDefinition)'s -/// [`effective_max_iterations()`](crate::openhuman::agent::harness::definition::AgentDefinition::effective_max_iterations), -/// for use by [`Agent::from_config_for_agent`](crate::openhuman::agent::Agent::from_config_for_agent) -/// in [`flows_build`]. -/// -/// Two independent iteration-cap systems exist in the harness: the per-agent -/// `AgentDefinition.max_iterations`/`iteration_policy` (`workflow_builder`'s -/// `agent.toml` declares `iteration_policy = "extended"`, so its effective cap -/// is `max(max_iterations, EXTENDED_MAX_TOOL_ITERATIONS)` = 50), and the -/// GLOBAL `Config.agent.max_tool_iterations` (default 10). Only the -/// **sub-agent runner** path (`spawn_subagent`, used when `workflow_builder` -/// is invoked as a chat delegate) applies the definition's effective cap. -/// `flows_build` calls `Agent::from_config_for_agent` directly — the session -/// builder stamps `config.agent.clone()` onto the session unconditionally, so -/// without this override the builder silently got the global default (10) -/// instead of the 50 its own `agent.toml` intends, burning its entire budget -/// on a handful of dry-run cycles for anything past a trivial graph. -/// -/// A missing registry or missing `workflow_builder` definition (registry not -/// yet initialised, a stripped-down test registry) falls back to `config` -/// unchanged — same behavior as before this fix, never a hard failure. -fn apply_builder_iteration_cap(config: &Config) -> Config { - let mut build_config = config.clone(); - let Some(reg) = crate::openhuman::agent::harness::AgentDefinitionRegistry::global() else { - tracing::warn!( - target: "flows", - "[flows] flows_build: agent registry not initialised; falling back to global \ - max_tool_iterations={}", - config.agent.max_tool_iterations - ); - return build_config; - }; - let Some(def) = reg.get("workflow_builder") else { - tracing::warn!( - target: "flows", - "[flows] flows_build: workflow_builder definition not found in registry; falling \ - back to global max_tool_iterations={}", - config.agent.max_tool_iterations - ); - return build_config; - }; - let effective = def.effective_max_iterations(); - tracing::debug!( - target: "flows", - toml_max_iterations = def.max_iterations, - effective_max_iterations = effective, - global_default = config.agent.max_tool_iterations, - "[flows] flows_build: overriding max_tool_iterations from the workflow_builder agent \ - definition (the session path does not apply effective_max_iterations() by default)" - ); - build_config.agent.max_tool_iterations = effective; - build_config -} - /// Runs the `workflow_builder` agent for one authoring turn and returns its /// proposal, invoking it as a first-class backend agent (exactly like the Flow /// Scout `flows_discover`) rather than routing a hand-crafted delegate prompt @@ -3404,13 +3350,11 @@ pub async fn flows_build( crate::openhuman::agent::harness::AgentDefinitionRegistry::init_global(&config.workspace_dir) .map_err(|e| format!("failed to initialise agent registry: {e}"))?; - // (B31) See `apply_builder_iteration_cap`'s doc: the session path below - // otherwise stamps the GLOBAL `config.agent.max_tool_iterations` (10) - // instead of the `workflow_builder` definition's intended - // `effective_max_iterations()` (50). - let build_config = apply_builder_iteration_cap(config); - - let mut agent = Agent::from_config_for_agent(&build_config, "workflow_builder") + // Issue #4868 — the session builder (`build_session_agent_inner`) now + // resolves the per-agent iteration cap from the `workflow_builder` + // `AgentDefinition` itself (`iteration_policy = "extended"` -> + // `effective_max_iterations()` = 50), so no override is needed here. + let mut agent = Agent::from_config_for_agent(config, "workflow_builder") .map_err(|e| format!("failed to build workflow_builder agent: {e:#}"))?; agent.set_agent_definition_name("workflow_builder".to_string()); diff --git a/src/openhuman/flows/ops_tests.rs b/src/openhuman/flows/ops_tests.rs index 95f9939891..2f0615fe95 100644 --- a/src/openhuman/flows/ops_tests.rs +++ b/src/openhuman/flows/ops_tests.rs @@ -3156,10 +3156,14 @@ async fn flows_build_hides_the_live_run_tool_from_the_builder_belt() { } } -/// Regression for B31: `flows_build` must apply the `workflow_builder` -/// `AgentDefinition`'s `effective_max_iterations()` (50, from `agent.toml`'s +/// Regression for issue #4868 (systemic fix, superseding the old B31 +/// per-caller `apply_builder_iteration_cap` override): `flows_build` must get +/// an agent carrying the `workflow_builder` `AgentDefinition`'s +/// `effective_max_iterations()` (50, from `agent.toml`'s /// `iteration_policy = "extended"`), not the global `Config::default()` -/// `agent.max_tool_iterations` (10) — see `apply_builder_iteration_cap`'s doc. +/// `agent.max_tool_iterations` (10) — and it must get this from the shared +/// resolution point in `build_session_agent_inner`, with **no** per-caller +/// override needed (that function was deleted as part of #4868). #[tokio::test] async fn flows_build_applies_the_builder_definitions_effective_iteration_cap() { let tmp = TempDir::new().unwrap(); @@ -3183,21 +3187,43 @@ async fn flows_build_applies_the_builder_definitions_effective_iteration_cap() { yielding an effective cap of EXTENDED_MAX_TOOL_ITERATIONS (50)" ); - let build_config = apply_builder_iteration_cap(&config); - assert_eq!( - build_config.agent.max_tool_iterations, expected, - "flows_build's build_config must carry the definition's effective cap, not the global \ - default" - ); + // End-to-end: the agent actually built for this path carries the + // definition's cap straight off the unmodified `config` — the session + // builder resolves it internally now, no `flows_build`-side override. + let agent = crate::openhuman::agent::Agent::from_config_for_agent(&config, "workflow_builder") + .expect("build workflow_builder agent"); + assert_eq!(agent.agent_config().max_tool_iterations, expected); assert_ne!( - build_config.agent.max_tool_iterations, config.agent.max_tool_iterations, - "sanity: the override must actually differ from the unmodified global config" + agent.agent_config().max_tool_iterations, + config.agent.max_tool_iterations, + "sanity: the resolved cap must actually differ from the unmodified global config" ); +} - // End-to-end: the agent actually built for this path carries the override. - let agent = - crate::openhuman::agent::Agent::from_config_for_agent(&build_config, "workflow_builder") - .expect("build workflow_builder agent"); +/// Regression for issue #4868: `flows_discover`'s `flow_discovery` agent must +/// also resolve to its definition's effective cap (50, `iteration_policy = +/// "extended"`), not the global default of 10. Before the systemic fix, this +/// call site had NO override at all (unlike `flows_build`'s now-deleted +/// `apply_builder_iteration_cap`), so it silently got the global 10 in +/// production. +#[tokio::test] +async fn flows_discover_applies_the_flow_discovery_definitions_effective_iteration_cap() { + let tmp = TempDir::new().unwrap(); + let config = test_config(&tmp); + assert_eq!(config.agent.max_tool_iterations, 10); + + crate::openhuman::agent::harness::AgentDefinitionRegistry::init_global(&config.workspace_dir) + .expect("agent registry init"); + let def = crate::openhuman::agent::harness::AgentDefinitionRegistry::global() + .expect("registry initialised") + .get("flow_discovery") + .expect("flow_discovery definition registered") + .clone(); + let expected = def.effective_max_iterations(); + assert_eq!(expected, 50); + + let agent = crate::openhuman::agent::Agent::from_config_for_agent(&config, "flow_discovery") + .expect("build flow_discovery agent"); assert_eq!(agent.agent_config().max_tool_iterations, expected); } diff --git a/src/openhuman/skill_runtime/run_machinery.rs b/src/openhuman/skill_runtime/run_machinery.rs index d56265b4e1..8d5dee3a72 100644 --- a/src/openhuman/skill_runtime/run_machinery.rs +++ b/src/openhuman/skill_runtime/run_machinery.rs @@ -176,7 +176,6 @@ pub async fn spawn_workflow_run_background( return; } }; - config.agent.max_tool_iterations = WORKFLOW_RUN_MAX_ITERATIONS; // Only apply the permissive wildcard default when the operator // hasn't configured an explicit allow-list — preserve any // configured egress policy instead of unconditionally widening it. @@ -196,6 +195,13 @@ pub async fn spawn_workflow_run_background( return; } }; + // Issue #4868 — apply the workflow-run iteration budget AFTER + // construction. The session builder now stamps `orchestrator`'s + // definition cap (15) onto the agent; a full workflow run + // intentionally needs a much larger budget (200), so this must be + // a post-construction override rather than a pre-set on `config` + // (which the builder would otherwise clobber). + agent.set_max_tool_iterations(WORKFLOW_RUN_MAX_ITERATIONS); agent.set_event_context(run_id.clone(), "skill"); agent.set_agent_definition_name(format!( "orchestrator-skill-{}", diff --git a/src/openhuman/subconscious/profiles/memory.rs b/src/openhuman/subconscious/profiles/memory.rs index e7c9068e28..c9f6ef7ae9 100644 --- a/src/openhuman/subconscious/profiles/memory.rs +++ b/src/openhuman/subconscious/profiles/memory.rs @@ -160,11 +160,19 @@ impl MemoryProfile { } SubconsciousMode::Off => return Ok(0), } + let mode_iteration_cap = effective.agent.max_tool_iterations; let mut agent = Agent::from_config(&effective).map_err(|e| { warn!("[subconscious:memory] agent init failed: {e}"); format!("agent init: {e}") })?; + // Issue #4868 — `Agent::from_config` builds as the `orchestrator` + // definition (max_iterations=15, strict), so the session builder + // would stamp orchestrator's cap onto this agent regardless of mode + // — silently dropping `Aggressive`/`EventDriven` mode's intended + // 30-iteration budget set above to 15. Re-apply the mode-specific + // cap post-construction so this tick keeps its previous behavior. + agent.set_max_tool_iterations(mode_iteration_cap); agent.set_event_context( format!("subconscious:tick:{}", now_secs() as u64), diff --git a/src/openhuman/subconscious/session.rs b/src/openhuman/subconscious/session.rs index 80af1448af..a524d64737 100644 --- a/src/openhuman/subconscious/session.rs +++ b/src/openhuman/subconscious/session.rs @@ -176,6 +176,7 @@ impl LongLivedSession { /// caps, seeding history from the reserved thread for cold-boot resume. fn build_agent(&self, config: &Config, current_message: &str) -> Result { let effective = effective_config(config, self.mode); + let mode_iteration_cap = effective.agent.max_tool_iterations; // Build as the `subconscious` agent (not the default orchestrator) so // the session's promoted turns get the subconscious tool surface — // memory_diff + agent_prepare_context + global to-dos/goals + the @@ -184,6 +185,14 @@ impl LongLivedSession { warn!("[subconscious::session] agent init failed: {e}"); format!("agent init: {e}") })?; + // Issue #4868 — the session builder now stamps the `subconscious` + // agent definition's own `effective_max_iterations()` (30) onto the + // agent regardless of mode, which would silently widen `Simple` + // mode's intentionally tighter 15-iteration budget set by + // `effective_config` above. Re-apply the mode-specific cap + // post-construction so `Simple` keeps 15 and `Aggressive`/ + // `EventDriven` keep 30, exactly as before this fix. + agent.set_max_tool_iterations(mode_iteration_cap); agent.set_event_context(self.thread_id.clone(), "subconscious"); // Cold-boot resume: prime history from the reserved thread. @@ -377,4 +386,65 @@ mod tests { assert_eq!(eff.autonomy.level, AutonomyLevel::Full); assert_eq!(eff.agent.max_tool_iterations, 30); } + + /// Regression for issue #4868: the `subconscious` `AgentDefinition` + /// declares `max_iterations = 30` (strict), so `build_session_agent_inner` + /// now stamps 30 onto every session agent regardless of mode — which + /// would silently widen `Simple` mode's intentionally tighter + /// 15-iteration budget set by `effective_config`. `build_agent` must + /// re-apply the mode-specific cap post-construction so `Simple` still + /// gets 15. + #[test] + fn build_agent_preserves_simple_modes_15_iteration_cap() { + use crate::openhuman::agent::harness::AgentDefinitionRegistry; + + AgentDefinitionRegistry::init_global_builtins().unwrap(); + + let tmp = tempfile::TempDir::new().unwrap(); + let workspace_dir = tmp.path().to_path_buf(); + std::fs::create_dir_all(&workspace_dir).unwrap(); + let config = Config { + workspace_dir: workspace_dir.clone(), + action_dir: workspace_dir.clone(), + ..Config::default() + }; + + let session = LongLivedSession::new(workspace_dir, SubconsciousMode::Simple); + let agent = session + .build_agent(&config, "hello") + .expect("build_agent should succeed"); + + assert_eq!( + agent.agent_config().max_tool_iterations, + 15, + "Simple mode must keep its 15-iteration cap even though the `subconscious` \ + definition's own effective_max_iterations() is 30" + ); + } + + /// Companion to the above: `Aggressive`/`EventDriven` mode's 30-iteration + /// cap must also survive (it happens to match the definition's own cap + /// here, but must come from the mode override, not incidentally). + #[test] + fn build_agent_preserves_aggressive_modes_30_iteration_cap() { + use crate::openhuman::agent::harness::AgentDefinitionRegistry; + + AgentDefinitionRegistry::init_global_builtins().unwrap(); + + let tmp = tempfile::TempDir::new().unwrap(); + let workspace_dir = tmp.path().to_path_buf(); + std::fs::create_dir_all(&workspace_dir).unwrap(); + let config = Config { + workspace_dir: workspace_dir.clone(), + action_dir: workspace_dir.clone(), + ..Config::default() + }; + + let session = LongLivedSession::new(workspace_dir, SubconsciousMode::Aggressive); + let agent = session + .build_agent(&config, "hello") + .expect("build_agent should succeed"); + + assert_eq!(agent.agent_config().max_tool_iterations, 30); + } } diff --git a/src/openhuman/tinyflows/caps.rs b/src/openhuman/tinyflows/caps.rs index f6f7930c56..c3db0c133e 100644 --- a/src/openhuman/tinyflows/caps.rs +++ b/src/openhuman/tinyflows/caps.rs @@ -656,6 +656,30 @@ pub(crate) fn clamp_run_timeout_secs(requested: Option) -> u64 { requested.map(|s| s.clamp(10, 600)).unwrap_or(240) } +/// Issue #4868 — scale `base_timeout_secs` up for agents whose effective +/// iteration cap exceeds the (until now, universal) global default of 10. +/// +/// A `tools_agent`/`code_executor`/etc. node now legitimately runs up to 50 +/// iterations (`iteration_policy = "extended"`). At a worst case of +/// ~10s/iteration that's ~500s, comfortably exceeding the 240s +/// `clamp_run_timeout_secs` default — the node would be killed by timeout +/// before it could use its own declared budget. Agents whose effective cap is +/// still at or below the old global default (10) are unaffected and keep the +/// unscaled `base_timeout_secs`. The scaled floor is capped at the existing +/// 600s maximum `clamp_run_timeout_secs` already enforces, so this can only +/// ever raise the effective timeout up to that ceiling, never past it. +pub(crate) fn scale_timeout_for_iteration_cap( + base_timeout_secs: u64, + effective_iteration_cap: usize, +) -> u64 { + if effective_iteration_cap > 10 { + let scaled = (effective_iteration_cap as u64).saturating_mul(12).min(600); + base_timeout_secs.max(scaled) + } else { + base_timeout_secs + } +} + /// Renders an agent-node completion `request` into the single user message /// [`Agent::run_single`](crate::openhuman::agent::Agent::run_single) takes: the /// `prompt` string when present and non-empty, else the `messages` array @@ -899,14 +923,27 @@ impl OpenHumanAgentRunner { let prompt = build_harness_run_prompt(&request); - let timeout_secs = + let base_timeout_secs = clamp_run_timeout_secs(request.get("timeout_secs").and_then(Value::as_u64)); + // Issue #4868 — the session builder now stamps `agent_ref`'s own + // `effective_max_iterations()` onto the agent (instead of the global + // default of 10), so `code_executor`/`tools_agent`/etc. can run up to + // 50 iterations here. Read the cap actually applied to `agent` + // (reflects the definition cap or the global fallback, whichever the + // builder resolved) and scale the timeout accordingly — see + // `scale_timeout_for_iteration_cap`. + let effective_iteration_cap = agent.agent_config().max_tool_iterations; + let timeout_secs = + scale_timeout_for_iteration_cap(base_timeout_secs, effective_iteration_cap); + tracing::debug!( target: "flows", agent_ref, node_model = node_model.as_deref().unwrap_or(""), default_model = effective.default_model.as_deref().unwrap_or(""), + effective_iteration_cap, + base_timeout_secs, timeout_secs, prompt_len = prompt.len(), "[flows] agent_runner: dispatching full harness turn" @@ -4101,6 +4138,71 @@ mod tests { assert!(escalated_origin_for_nested_harness(None).is_none()); } + // ── Issue #4868 — agent-node iteration cap + timeout scaling ─────────── + + #[test] + fn scale_timeout_for_iteration_cap_leaves_default_cap_unscaled() { + // An agent whose effective cap is at or below the old global default + // (10) doesn't need extra wall-clock time. + assert_eq!(scale_timeout_for_iteration_cap(240, 10), 240); + assert_eq!(scale_timeout_for_iteration_cap(240, 3), 240); + } + + #[test] + fn scale_timeout_for_iteration_cap_scales_extended_agents_up() { + // 50 iterations * 12s/iter = 600s, exactly the existing ceiling. + assert_eq!(scale_timeout_for_iteration_cap(240, 50), 600); + } + + #[test] + fn scale_timeout_for_iteration_cap_never_lowers_an_explicit_request() { + // A caller-requested timeout higher than the scaled floor must win. + assert_eq!(scale_timeout_for_iteration_cap(600, 50), 600); + } + + #[test] + fn scale_timeout_for_iteration_cap_caps_at_600_even_for_very_high_iteration_counts() { + assert_eq!(scale_timeout_for_iteration_cap(240, 200), 600); + } + + /// Regression for issue #4868: the agent-node runtime path + /// (`OpenHumanAgentRunner::run_via_harness`) must build an `Agent` that + /// carries `agent_ref`'s definition's effective cap (50 for an + /// extended-policy agent), not the global `config.agent.max_tool_iterations` + /// default (10). This mirrors the exact build step `run_via_harness` takes + /// before dispatching the turn (so it doesn't require a live model + /// provider to exercise). + #[test] + fn agent_node_runtime_resolves_to_the_definitions_effective_iteration_cap() { + let tmp = tempfile::TempDir::new().unwrap(); + let config = resolver_test_config(&tmp); + assert_eq!(config.agent.max_tool_iterations, 10); + + crate::openhuman::agent::harness::definition::AgentDefinitionRegistry::init_global( + &config.workspace_dir, + ) + .expect("agent registry init"); + let def = crate::openhuman::agent::harness::definition::AgentDefinitionRegistry::global() + .expect("registry initialised") + .get("code_executor") + .expect("code_executor definition registered") + .clone(); + let expected = def.effective_max_iterations(); + assert_eq!(expected, 50); + + let agent = crate::openhuman::agent::Agent::from_config_for_agent(&config, "code_executor") + .expect("build code_executor agent"); + assert_eq!(agent.agent_config().max_tool_iterations, expected); + + // And the timeout scaling this cap feeds into actually widens the + // default 240s bound for this node. + let base_timeout = clamp_run_timeout_secs(None); + assert_eq!(base_timeout, 240); + let scaled = + scale_timeout_for_iteration_cap(base_timeout, agent.agent_config().max_tool_iterations); + assert_eq!(scaled, 600); + } + // ── Phase 7: sub_workflow-by-id resolver ─────────────────────────────── fn resolver_test_config(tmp: &tempfile::TempDir) -> Config {