mirror of
https://github.com/tinyhumansai/openhuman.git
synced 2026-07-27 21:08:00 +00:00
fix(tinyflows): preserve explicit node timeouts, align discover timeout, log cap pins (#4877)
This commit is contained in:
@@ -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<React.ComponentProps<typeof FlowCanvas>> = {}) {
|
||||
return render(
|
||||
const ref = createRef<EditableFlowCanvasHandle>();
|
||||
const onSaveMetaChange = vi.fn<(meta: EditorSaveMeta) => void>();
|
||||
const utils = render(
|
||||
<FlowCanvas
|
||||
ref={ref}
|
||||
editable
|
||||
nodes={[triggerNode()]}
|
||||
edges={[]}
|
||||
meta={META}
|
||||
onSave={vi.fn().mockResolvedValue(undefined)}
|
||||
onSaveMetaChange={onSaveMetaChange}
|
||||
{...props}
|
||||
/>
|
||||
);
|
||||
return { ...utils, ref, onSaveMetaChange };
|
||||
}
|
||||
|
||||
/** Latest `{ dirty, hasErrors, saving }` the canvas reported to its host. */
|
||||
function lastSaveMeta(onSaveMetaChange: ReturnType<typeof vi.fn<(meta: EditorSaveMeta) => 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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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,
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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-{}-{}",
|
||||
|
||||
@@ -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.)
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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-{}",
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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<u64>,
|
||||
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("<definition-default>"),
|
||||
default_model = effective.default_model.as_deref().unwrap_or("<config-default>"),
|
||||
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
|
||||
|
||||
Reference in New Issue
Block a user