diff --git a/app/src/components/flows/canvas/__tests__/EditableFlowCanvas.validation.test.tsx b/app/src/components/flows/canvas/__tests__/EditableFlowCanvas.validation.test.tsx index 6636e9aa24..9a1a6450dc 100644 --- a/app/src/components/flows/canvas/__tests__/EditableFlowCanvas.validation.test.tsx +++ b/app/src/components/flows/canvas/__tests__/EditableFlowCanvas.validation.test.tsx @@ -1,18 +1,30 @@ /** * EditableFlowCanvas — validation UX (Phase 3c) + draft/dirty state (Phase 3d). * - * Drives the canvas through the public `FlowCanvas editable` entry point with a - * mocked `flowsApi` so `validateFlow` is deterministic. Covers: - * - an invalid graph shows the inline error banner, rings the offending node, - * and blocks Save; - * - a valid-with-warnings graph surfaces warnings distinctly and allows Save; - * - dirty tracking gates Save/Discard, Discard resets to baseline, and a - * successful Save clears the dirty flag. + * A canvas-refactor moved the Save / Discard / dirty-badge *buttons* out of + * this component and up into `FlowCanvasPage`'s header — the canvas now only + * exposes them through the `EditableFlowCanvasHandle` ref (`save()`/ + * `discard()`) and reports state up via `onSaveMetaChange` + * (`{ dirty, hasErrors, saving }`), same as `onDirtyChange`. See + * `FlowCanvasPage.test.tsx` for the header-button + confirm-dialog + RPC + * integration coverage (clicking `flow-editor-save`/`flow-editor-discard`). + * + * This file drives the canvas through the public `FlowCanvas editable` entry + * point with a mocked `flowsApi` so `validateFlow` is deterministic, and + * covers what `FlowCanvas`/`EditableFlowCanvas` itself still owns: + * - an invalid graph shows the inline error banner, rings the offending + * node, and the imperative `save()` handle refuses to fire `onSave`; + * - a valid-with-warnings graph surfaces warnings distinctly and still + * lets `save()` fire; + * - dirty tracking gates `save()`/`discard()`, `discard()` resets to + * baseline, and a successful `save()` clears the dirty flag. */ -import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { act, fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { createRef } from 'react'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import type { FlowNode } from '../../../../lib/flows/graphAdapter'; +import type { EditableFlowCanvasHandle, EditorSaveMeta } from '../EditableFlowCanvas'; import FlowCanvas from '../FlowCanvas'; const validateFlow = vi.hoisted(() => vi.fn()); @@ -38,16 +50,27 @@ function triggerNode(): FlowNode { const META = { schema_version: 1, id: 'wf_1', name: 'My flow' } as const; function renderCanvas(props: Partial> = {}) { - return render( + const ref = createRef(); + const onSaveMetaChange = vi.fn<(meta: EditorSaveMeta) => void>(); + const utils = render( ); + return { ...utils, ref, onSaveMetaChange }; +} + +/** Latest `{ dirty, hasErrors, saving }` the canvas reported to its host. */ +function lastSaveMeta(onSaveMetaChange: ReturnType void>>) { + const calls = onSaveMetaChange.mock.calls; + return calls[calls.length - 1][0]; } describe('EditableFlowCanvas — validation + dirty state', () => { @@ -57,24 +80,29 @@ describe('EditableFlowCanvas — validation + dirty state', () => { listFlowConnections.mockResolvedValue([]); }); - it('surfaces hard errors, rings the offending node, and blocks Save', async () => { + it('surfaces hard errors, rings the offending node, and blocks save()', async () => { validateFlow.mockResolvedValue({ valid: false, errors: ['invalid config for node t: missing schedule'], warnings: [], }); - const { container } = renderCanvas(); + const onSave = vi.fn().mockResolvedValue(undefined); + const { container, ref, onSaveMetaChange } = renderCanvas({ onSave }); - // Make an edit so the graph is dirty (Save is only ever enabled when dirty). + // Make an edit so the graph is dirty (Save is only ever attempted when + // dirty). Validation runs automatically on the debounce after the edit + // (the manual Validate button now lives on the selected node card). fireEvent.click(screen.getByTestId('flow-palette-item-agent')); - // Validation runs automatically on the debounce after the edit (the manual - // Validate button now lives on the selected node card). const errors = await screen.findByTestId('flow-editor-errors'); expect(errors).toHaveTextContent('invalid config for node t: missing schedule'); - // Save is blocked while there are hard errors, even though the graph is dirty. - expect(screen.getByTestId('flow-editor-save')).toBeDisabled(); + // The host header reads `hasErrors` off `onSaveMetaChange` to disable its + // Save button; the canvas itself also refuses to fire `onSave` through + // the imperative handle while hard errors exist, even though dirty. + expect(lastSaveMeta(onSaveMetaChange)).toMatchObject({ dirty: true, hasErrors: true }); + act(() => ref.current?.save()); + expect(onSave).not.toHaveBeenCalled(); // The named node ('t') is ringed with the error class on its RF wrapper. await waitFor(() => @@ -84,13 +112,14 @@ describe('EditableFlowCanvas — validation + dirty state', () => { ); }); - it('shows warnings distinctly from errors and allows Save', async () => { + it('shows warnings distinctly from errors and still lets save() fire', async () => { validateFlow.mockResolvedValue({ valid: true, errors: [], warnings: ['this trigger kind does not fire automatically yet'], }); - renderCanvas(); + const onSave = vi.fn().mockResolvedValue(undefined); + const { ref, onSaveMetaChange } = renderCanvas({ onSave }); fireEvent.click(screen.getByTestId('flow-palette-item-agent')); // Auto-validation (debounced) surfaces the warning. @@ -99,71 +128,77 @@ describe('EditableFlowCanvas — validation + dirty state', () => { expect(warnings).toHaveTextContent('does not fire automatically'); // A valid graph never renders the errors list… expect(screen.queryByTestId('flow-editor-errors')).not.toBeInTheDocument(); - // …and Save is allowed (warnings don't block). - expect(screen.getByTestId('flow-editor-save')).not.toBeDisabled(); + // …and warnings don't block Save. + expect(lastSaveMeta(onSaveMetaChange)).toMatchObject({ dirty: true, hasErrors: false }); + + act(() => ref.current?.save()); + expect(onSave).toHaveBeenCalledTimes(1); }); - it('tracks dirty state: Save/Discard gate on it, Discard resets, Save clears it', async () => { + it('tracks dirty state: save()/discard() gate on it, discard resets, save clears it', async () => { validateFlow.mockResolvedValue({ valid: true, errors: [], warnings: [] }); const onSave = vi.fn().mockResolvedValue(undefined); const onDirtyChange = vi.fn(); - renderCanvas({ onSave, onDirtyChange }); + const { ref, onSaveMetaChange } = renderCanvas({ onSave, onDirtyChange }); - // Pristine: no dirty badge, Save + Discard disabled. - expect(screen.queryByTestId('flow-editor-dirty')).not.toBeInTheDocument(); - expect(screen.getByTestId('flow-editor-save')).toBeDisabled(); - expect(screen.getByTestId('flow-editor-discard')).toBeDisabled(); + // Pristine: not dirty; save()/discard() both no-op through the imperative + // handle (the host header renders both buttons disabled in this state). + expect(lastSaveMeta(onSaveMetaChange)).toMatchObject({ dirty: false }); + act(() => ref.current?.discard()); + act(() => ref.current?.save()); + expect(onSave).not.toHaveBeenCalled(); // Edit → dirty. fireEvent.click(screen.getByTestId('flow-palette-item-agent')); - expect(screen.getByTestId('flow-editor-dirty')).toBeInTheDocument(); - expect(screen.getByTestId('flow-editor-save')).not.toBeDisabled(); - expect(screen.getByTestId('flow-editor-discard')).not.toBeDisabled(); expect(onDirtyChange).toHaveBeenLastCalledWith(true); + expect(lastSaveMeta(onSaveMetaChange)).toMatchObject({ dirty: true }); expect(screen.getAllByTestId('flow-node')).toHaveLength(2); // Discard → back to the single trigger, no longer dirty. - fireEvent.click(screen.getByTestId('flow-editor-discard')); + act(() => ref.current?.discard()); expect(screen.getAllByTestId('flow-node')).toHaveLength(1); - expect(screen.queryByTestId('flow-editor-dirty')).not.toBeInTheDocument(); expect(onDirtyChange).toHaveBeenLastCalledWith(false); + expect(lastSaveMeta(onSaveMetaChange)).toMatchObject({ dirty: false }); - // Edit again and Save → onSave called, dirty cleared once it resolves. + // Edit again and save() → onSave called, dirty cleared once it resolves. fireEvent.click(screen.getByTestId('flow-palette-item-agent')); - fireEvent.click(screen.getByTestId('flow-editor-save')); + act(() => ref.current?.save()); await waitFor(() => expect(onSave).toHaveBeenCalledTimes(1)); const graph = onSave.mock.calls[0][0]; expect(graph.nodes.map((n: { kind: string }) => n.kind).sort()).toEqual(['agent', 'trigger']); - await waitFor(() => expect(screen.queryByTestId('flow-editor-dirty')).not.toBeInTheDocument()); - expect(onDirtyChange).toHaveBeenLastCalledWith(false); + await waitFor(() => expect(onDirtyChange).toHaveBeenLastCalledWith(false)); + await waitFor(() => expect(lastSaveMeta(onSaveMetaChange)).toMatchObject({ dirty: false })); }); - it('starts dirty when the host passes initialDirty (a remount carrying unsaved content)', async () => { + it('starts dirty when the host passes initialDirty (a remount carrying unsaved content)', () => { validateFlow.mockResolvedValue({ valid: true, errors: [], warnings: [] }); + const onSave = vi.fn().mockResolvedValue(undefined); const onDirtyChange = vi.fn(); // Mirrors `FlowCanvasPage` remounting the canvas (`key={canvasVersion}`) // after accepting a copilot proposal: the incoming nodes/edges ARE the // component's "initial" graph, so without `initialDirty` the canvas would // seed its baseline from them and instantly read as clean even though // nothing was persisted (the P1 this regression test guards against). - renderCanvas({ onDirtyChange, initialDirty: true }); + const { ref, onSaveMetaChange } = renderCanvas({ onSave, onDirtyChange, initialDirty: true }); - expect(screen.getByTestId('flow-editor-dirty')).toBeInTheDocument(); - expect(screen.getByTestId('flow-editor-save')).not.toBeDisabled(); expect(onDirtyChange).toHaveBeenLastCalledWith(true); + expect(lastSaveMeta(onSaveMetaChange)).toMatchObject({ dirty: true }); + act(() => ref.current?.save()); + expect(onSave).toHaveBeenCalledTimes(1); }); it('surfaces a Save failure inline and leaves the graph dirty', async () => { validateFlow.mockResolvedValue({ valid: true, errors: [], warnings: [] }); const onSave = vi.fn().mockRejectedValue(new Error('core unreachable')); - renderCanvas({ onSave }); + const onDirtyChange = vi.fn(); + const { ref } = renderCanvas({ onSave, onDirtyChange }); fireEvent.click(screen.getByTestId('flow-palette-item-agent')); - fireEvent.click(screen.getByTestId('flow-editor-save')); + act(() => ref.current?.save()); const saveError = await screen.findByTestId('flow-editor-save-error'); expect(saveError).toHaveTextContent('core unreachable'); // Still dirty — nothing persisted. - expect(screen.getByTestId('flow-editor-dirty')).toBeInTheDocument(); + expect(onDirtyChange).toHaveBeenLastCalledWith(true); }); }); diff --git a/app/src/services/api/flowsApi.test.ts b/app/src/services/api/flowsApi.test.ts index ad7af55e1e..9733aa1a96 100644 --- a/app/src/services/api/flowsApi.test.ts +++ b/app/src/services/api/flowsApi.test.ts @@ -340,7 +340,7 @@ describe('flowsApi', () => { expect(mockCallCoreRpc).toHaveBeenCalledWith({ method: 'openhuman.flows_discover', params: {}, - timeoutMs: 310_000, + timeoutMs: 610_000, }); expect(result).toEqual([suggestion]); }); @@ -353,7 +353,7 @@ describe('flowsApi', () => { expect(mockCallCoreRpc).toHaveBeenCalledWith({ method: 'openhuman.flows_discover', params: { thread_id: 'scout-thread-1' }, - timeoutMs: 310_000, + timeoutMs: 610_000, }); }); diff --git a/app/src/services/api/flowsApi.ts b/app/src/services/api/flowsApi.ts index f23f0eb8a3..c295251b84 100644 --- a/app/src/services/api/flowsApi.ts +++ b/app/src/services/api/flowsApi.ts @@ -799,12 +799,14 @@ export async function promoteDraft(id: string, requireApproval?: boolean): Promi /** * `openhuman.flows_discover` runs the read-only Flow Scout agent, which reasons - * over the user's memory/threads/connections/flows and can take up to ~300s - * server-side (`FLOW_DISCOVER_TIMEOUT_SECS` in `src/openhuman/flows/ops.rs`). - * Give the client a matching budget so a slow discovery run doesn't time out - * client-side while the agent is still thinking. + * over the user's memory/threads/connections/flows and can take up to ~600s + * server-side (`FLOW_DISCOVER_TIMEOUT_SECS` in `src/openhuman/flows/ops.rs`, + * raised to match `FLOW_BUILD_TIMEOUT_SECS` for the same iteration cap). Give + * the client a matching budget (mirrors {@link FLOW_BUILD_TIMEOUT_MS}) so a + * slow discovery run doesn't time out client-side while the agent is still + * thinking. */ -const FLOW_DISCOVER_TIMEOUT_MS = 310_000; +const FLOW_DISCOVER_TIMEOUT_MS = 610_000; /** * Run the Flow Scout discovery agent via `openhuman.flows_discover` and return diff --git a/src/openhuman/agent/task_dispatcher/executor.rs b/src/openhuman/agent/task_dispatcher/executor.rs index f002d8c14a..daabdc7d7a 100644 --- a/src/openhuman/agent/task_dispatcher/executor.rs +++ b/src/openhuman/agent/task_dispatcher/executor.rs @@ -157,6 +157,13 @@ pub(super) async fn run_autonomous( // 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); + tracing::debug!( + agent_id = %executor.agent_id, + run_id = %run_id, + max_tool_iterations = TASK_RUN_MAX_ITERATIONS, + "[task_dispatcher] pinned autonomous task-run iteration budget post-construction \ + (overrides the session builder's per-definition cap)" + ); agent.set_event_context(run_id.to_string(), "task"); agent.set_agent_definition_name(format!( "task-{}-{}", diff --git a/src/openhuman/agent_registry/agents/loader.rs b/src/openhuman/agent_registry/agents/loader.rs index 7fd4eca00d..bcaa34a0cf 100644 --- a/src/openhuman/agent_registry/agents/loader.rs +++ b/src/openhuman/agent_registry/agents/loader.rs @@ -1039,19 +1039,41 @@ mod tests { ); match &def.tools { ToolScope::Named(names) => { + // Reconciled against `agent.toml`'s current `[tools].named` + // after the workflow-tools expansion PR widened the belt to + // agent-native editing/creation/run-control (`edit_workflow`, + // `validate_workflow`, `create_workflow`, `duplicate_flow`, + // `list_node_kinds`, `get_node_kind_contract`, + // `get_flow_history`, `list_flow_runs`, `resume_flow_run`, + // `cancel_flow_run`, `list_connectable_toolkits`) — these are + // the agent's own scoped tool surface, not the raw `flows_*` + // controller RPCs banned below, so the "no flow + // creation/enable via the raw controller" invariant still + // holds via the forbidden list. let expected = [ "propose_workflow", "revise_workflow", + "edit_workflow", + "validate_workflow", "save_workflow", "list_flows", "get_flow", + "get_flow_history", "get_flow_run", "list_flow_connections", "search_tool_catalog", "get_tool_contract", "get_tool_output_sample", "list_agent_profiles", + "list_connectable_toolkits", + "list_node_kinds", + "get_node_kind_contract", "dry_run_workflow", + "list_flow_runs", + "resume_flow_run", + "cancel_flow_run", + "create_workflow", + "duplicate_flow", "run_flow", "composio_list_toolkits", "composio_list_connections", @@ -1070,8 +1092,9 @@ mod tests { expected.len(), "workflow_builder scope must be EXACTLY the propose-or-read belt (got {names:?})" ); - // Hard exclusions: nothing that creates/enables a flow, - // executes raw integration actions, or touches the host. + // Hard exclusions: nothing that reaches the raw flow + // controller directly, executes raw integration actions, or + // touches the host. // (Persistence onto an EXISTING flow is the deliberate // `save_workflow` carve-out above; raw `flows_update` — which // could also rename/re-gate arbitrary flows — stays out.) diff --git a/src/openhuman/flows/ops.rs b/src/openhuman/flows/ops.rs index 44b73438a9..9f5bca5b12 100644 --- a/src/openhuman/flows/ops.rs +++ b/src/openhuman/flows/ops.rs @@ -3297,7 +3297,14 @@ fn notify_pending_approval(flow: &Flow, thread_id: &str, pending_approvals: &[St /// reasons read-only over the user's data and ends by emitting /// `suggest_workflows`; its own `max_iterations` caps the loop, but a hung /// LLM/tool call must never let the RPC block indefinitely. -const FLOW_DISCOVER_TIMEOUT_SECS: u64 = 300; +/// +/// Matches [`FLOW_BUILD_TIMEOUT_SECS`] (600s): the session builder applies the +/// `flow_discovery` 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 could clip a +/// legitimate long discovery run before the iteration cap ever got a chance +/// to (post-merge Codex P2 finding). +const FLOW_DISCOVER_TIMEOUT_SECS: u64 = 600; /// The canned brief handed to the `flow_discovery` agent. The agent's own /// archetype prompt teaches the read → correlate → ground → emit loop; this is diff --git a/src/openhuman/flows/schemas.rs b/src/openhuman/flows/schemas.rs index dac1f64167..feee1c1a7c 100644 --- a/src/openhuman/flows/schemas.rs +++ b/src/openhuman/flows/schemas.rs @@ -1694,6 +1694,7 @@ mod tests { "resume", "cancel_run", "list_runs", + "list_all_runs", "get_run", "prune_runs", "build", @@ -1719,7 +1720,7 @@ mod tests { #[test] fn all_registered_controllers_has_handler_per_schema() { let controllers = all_registered_controllers(); - assert_eq!(controllers.len(), 32); + assert_eq!(controllers.len(), 33); let names: Vec<_> = controllers.iter().map(|c| c.schema.function).collect(); assert_eq!( names, @@ -1738,6 +1739,7 @@ mod tests { "resume", "cancel_run", "list_runs", + "list_all_runs", "get_run", "prune_runs", "build", diff --git a/src/openhuman/skill_runtime/run_machinery.rs b/src/openhuman/skill_runtime/run_machinery.rs index 8d5dee3a72..f69bba0f1b 100644 --- a/src/openhuman/skill_runtime/run_machinery.rs +++ b/src/openhuman/skill_runtime/run_machinery.rs @@ -202,6 +202,12 @@ pub async fn spawn_workflow_run_background( // 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); + tracing::debug!( + run_id = %run_id, + max_tool_iterations = WORKFLOW_RUN_MAX_ITERATIONS, + "[skills] workflow_run: pinned iteration budget post-construction (overrides \ + the session builder's orchestrator definition cap)" + ); 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 c9f6ef7ae9..49f818066c 100644 --- a/src/openhuman/subconscious/profiles/memory.rs +++ b/src/openhuman/subconscious/profiles/memory.rs @@ -166,6 +166,13 @@ impl MemoryProfile { warn!("[subconscious:memory] agent init failed: {e}"); format!("agent init: {e}") })?; + // Stable per-tick correlation id — minted once and reused below for + // the pin log, the agent's event context, and the turn origin's + // `job_id`, so `mode`/`max_tool_iterations` (which repeat across + // ticks) don't leave concurrent/successive subconscious ticks + // indistinguishable in logs. + let tick_id = format!("subconscious:tick:{}", now_secs() as u64); + // 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 @@ -173,12 +180,16 @@ impl MemoryProfile { // 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), - "subconscious", + debug!( + tick_id = %tick_id, + "[subconscious:memory] pinned mode-specific iteration budget post-construction: \ + mode={:?} max_tool_iterations={} (overrides the session builder's orchestrator \ + definition cap)", + self.mode, mode_iteration_cap ); + agent.set_event_context(tick_id.clone(), "subconscious"); + let mode_guidance = match self.mode { SubconsciousMode::Aggressive | SubconsciousMode::EventDriven => { "\n\nYou may delegate deeper work with `spawn_async_subagent` (e.g. research \ @@ -201,10 +212,10 @@ impl MemoryProfile { ticks. Do not invent busywork.{mode_guidance}", ); - debug!("[subconscious:memory] spawning decision agent"); + debug!(tick_id = %tick_id, "[subconscious:memory] spawning decision agent"); let source = tick_origin_source(has_external_content); let origin = crate::openhuman::agent::turn_origin::AgentTurnOrigin::TrustedAutomation { - job_id: format!("subconscious:tick:{}", now_secs() as u64), + job_id: tick_id.clone(), source, }; let response = crate::openhuman::agent::turn_origin::with_origin( diff --git a/src/openhuman/tinyflows/caps.rs b/src/openhuman/tinyflows/caps.rs index c3db0c133e..486fb9f864 100644 --- a/src/openhuman/tinyflows/caps.rs +++ b/src/openhuman/tinyflows/caps.rs @@ -680,6 +680,35 @@ pub(crate) fn scale_timeout_for_iteration_cap( } } +/// Resolves the actual wall-clock timeout for one agent-node harness turn, +/// combining [`clamp_run_timeout_secs`] and [`scale_timeout_for_iteration_cap`] +/// per the post-merge Codex P2 finding on issue #4868's iteration-cap timeout +/// scaling: **an explicit `timeout_secs` the flow author set on the node must +/// never be scaled up.** +/// +/// A node's `timeout_secs` can be an intentional fast-fail/SLA bound (e.g. +/// `timeout_secs: 120` to bound a health-check-style agent call) — scaling +/// that up to match a 50-iteration-cap agent would silently defeat the +/// author's explicit choice. So the iteration-cap scaling only ever widens +/// the *default* (no `timeout_secs` supplied) 240s bound; an explicit value is +/// clamped to `10..=600` (as it always was) and returned as-is. +/// +/// `requested_timeout_secs` is the raw `request["timeout_secs"]` (before +/// clamping) so this function can distinguish "caller supplied a value" from +/// "caller supplied nothing" — [`clamp_run_timeout_secs`] alone collapses that +/// distinction into a plain `u64`. +pub(crate) fn resolve_run_timeout_secs( + requested_timeout_secs: Option, + effective_iteration_cap: usize, +) -> u64 { + let base_timeout_secs = clamp_run_timeout_secs(requested_timeout_secs); + if requested_timeout_secs.is_some() { + base_timeout_secs + } else { + scale_timeout_for_iteration_cap(base_timeout_secs, effective_iteration_cap) + } +} + /// 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 @@ -923,19 +952,24 @@ impl OpenHumanAgentRunner { let prompt = build_harness_run_prompt(&request); - let base_timeout_secs = - clamp_run_timeout_secs(request.get("timeout_secs").and_then(Value::as_u64)); + let requested_timeout_secs = request.get("timeout_secs").and_then(Value::as_u64); + let base_timeout_secs = clamp_run_timeout_secs(requested_timeout_secs); // 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 + // builder resolved) and scale the DEFAULT timeout accordingly — see // `scale_timeout_for_iteration_cap`. + // + // Post-merge Codex P2 finding: an EXPLICIT `timeout_secs` the node + // config supplied is a caller-chosen bound (e.g. a fast-fail/SLA of + // 120s) and must be honored as-is, never scaled up just because the + // agent's iteration cap is high — see `resolve_run_timeout_secs`. let effective_iteration_cap = agent.agent_config().max_tool_iterations; let timeout_secs = - scale_timeout_for_iteration_cap(base_timeout_secs, effective_iteration_cap); + resolve_run_timeout_secs(requested_timeout_secs, effective_iteration_cap); tracing::debug!( target: "flows", @@ -943,6 +977,7 @@ impl OpenHumanAgentRunner { node_model = node_model.as_deref().unwrap_or(""), default_model = effective.default_model.as_deref().unwrap_or(""), effective_iteration_cap, + explicit_timeout_secs = requested_timeout_secs.is_some(), base_timeout_secs, timeout_secs, prompt_len = prompt.len(), @@ -4165,6 +4200,29 @@ mod tests { assert_eq!(scale_timeout_for_iteration_cap(240, 200), 600); } + /// Post-merge Codex P2 finding on issue #4868: an explicit `timeout_secs` + /// the node config supplied (a caller-chosen fast-fail/SLA bound) must be + /// honored as-is — never scaled up just because the agent's iteration cap + /// is high — while the absence of one still gets the iteration-cap + /// scaling so a 50-iteration agent isn't killed by the 240s default. + #[test] + fn resolve_run_timeout_secs_preserves_an_explicit_request_even_for_a_high_cap_agent() { + assert_eq!(resolve_run_timeout_secs(Some(120), 50), 120); + } + + #[test] + fn resolve_run_timeout_secs_scales_the_default_up_for_a_high_cap_agent() { + // No explicit timeout_secs (None) -> default 240s, scaled by the + // 50-iteration cap to min(50*12, 600) = 600. + assert_eq!(resolve_run_timeout_secs(None, 50), 600); + } + + #[test] + fn resolve_run_timeout_secs_leaves_low_cap_agents_unscaled_either_way() { + assert_eq!(resolve_run_timeout_secs(None, 10), 240); + assert_eq!(resolve_run_timeout_secs(Some(120), 10), 120); + } + /// 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