fix(agent): resolve iteration cap from the agent definition, not the global default (#4868)

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"
+    );
+}

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"
+    );
+}

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)

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

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"
+    );
+}

src/openhuman/agent/task_dispatcher/executor.rs

@@ -135,7 +135,6 @@ pub(super) async fn run_autonomous(
     run_id: &str,
     session_thread_id: Option<String>,
 ) -> Result<String, String> {
-    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-{}-{}",

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,