From cdf1dce4f8395762837835c7c54a50ffab6c2a6f Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 23 Jul 2026 19:36:29 -0400 Subject: [PATCH 1/2] fix(agent): remove dead execution limits --- docs/internal/events.md | 20 ---- .../fabro-event-schema-v2-concrete-shape.md | 1 - docs/public/agents/subagents.mdx | 3 +- docs/public/changelog/2026-03-23.mdx | 2 +- docs/public/reference/sdk.mdx | 7 +- lib/components/fabro-agent/README.md | 3 - lib/components/fabro-agent/src/config.rs | 11 -- lib/components/fabro-agent/src/session.rs | 107 +++++------------- lib/components/fabro-agent/src/subagent.rs | 62 +++++----- lib/components/fabro-agent/src/types.rs | 6 - .../fabro-agent/tests/it/compaction.rs | 1 - .../fabro-agent/tests/it/parity_matrix.rs | 17 +-- lib/components/fabro-workflow/src/error.rs | 20 +--- .../fabro-workflow/src/event/convert.rs | 6 - .../fabro-workflow/src/event/names.rs | 1 - .../fabro-types/src/run_event/agent.rs | 6 - .../fabro-types/src/run_event/mod.rs | 4 - 17 files changed, 66 insertions(+), 211 deletions(-) diff --git a/docs/internal/events.md b/docs/internal/events.md index 4af5ab491..7850a5411 100644 --- a/docs/internal/events.md +++ b/docs/internal/events.md @@ -1252,26 +1252,6 @@ Emitted when the agent detects a tool-use loop. No properties. -### `agent.turn.limit` - -Emitted when the agent reaches its maximum turn count. - -```json -{ - "id": "...", "ts": "...", "run_id": "...", - "event": "agent.turn.limit", - "node_id": "code", "node_label": "code", - "session_id": "ses_abc", - "properties": { - "max_turns": 25 - } -} -``` - -| Property | Type | Description | -|----------|------|-------------| -| `max_turns` | number | Maximum turns allowed | - ### `agent.skill.expanded` ```json diff --git a/docs/internal/fabro-event-schema-v2-concrete-shape.md b/docs/internal/fabro-event-schema-v2-concrete-shape.md index 77f4fe5da..78134f619 100644 --- a/docs/internal/fabro-event-schema-v2-concrete-shape.md +++ b/docs/internal/fabro-event-schema-v2-concrete-shape.md @@ -374,7 +374,6 @@ V2 keeps the current durable family surface broadly intact. - `agent.error` - `agent.warning` - `agent.loop.detected` -- `agent.turn.limit` - `agent.steering.injected` - `agent.compaction.started` - `agent.compaction.completed` diff --git a/docs/public/agents/subagents.mdx b/docs/public/agents/subagents.mdx index 11cf30c38..597bd96e3 100644 --- a/docs/public/agents/subagents.mdx +++ b/docs/public/agents/subagents.mdx @@ -44,7 +44,6 @@ Sub-agent failures do not automatically fail the parent stage. The parent receiv Common cases: -- **Hits `max_turns`** -- returns normally with its last output - **Panics or errors** -- returned as a failed `wait` result - **`spawn_agent` fails** -- returned immediately as a tool result @@ -97,5 +96,5 @@ Sub-agents are most useful for: Use [child runs](/execution/child-runs) instead when the delegated work should be a separate Fabro run with its own workflow, lifecycle, sandbox, checkpoints, and outputs. -Sub-agents run with no turn limit by default. Pass `max_turns` when you want predictable cost or time bounds. All active sub-agents are cleaned up automatically when the parent session closes. +Sub-agents run until they complete, fail, are cancelled, or hit the session's wall-clock timeout. All active sub-agents are cleaned up automatically when the parent session closes. diff --git a/docs/public/changelog/2026-03-23.mdx b/docs/public/changelog/2026-03-23.mdx index e3d47ecda..359f5bbc1 100644 --- a/docs/public/changelog/2026-03-23.mdx +++ b/docs/public/changelog/2026-03-23.mdx @@ -5,7 +5,7 @@ date: "2026-03-23" ## Unlimited agent tool rounds -Agent stages no longer have an artificial cap on tool rounds per input or subagent turns. Previously, the default limits could cause agents to stop mid-task on complex operations. Both `max_tool_rounds_per_input` and subagent `max_turns` now default to unlimited, so agents run until they complete their work or hit the context window. +Agent stages no longer have an artificial cap on tool rounds per input or subagent turns, so agents run until they complete their work or are interrupted. ## More diff --git a/docs/public/reference/sdk.mdx b/docs/public/reference/sdk.mdx index 4332f3adc..9f88c7696 100644 --- a/docs/public/reference/sdk.mdx +++ b/docs/public/reference/sdk.mdx @@ -84,7 +84,7 @@ pub fn new( | Method | Description | |---|---| | `initialize().await` | Discovers project docs, skills, and MCP servers. Call before `process_input`. | -| `process_input(input).await` | Sends user input and runs the agent loop until the model stops or a limit is hit. | +| `process_input(input).await` | Sends user input and runs the agent loop until the model stops, the session is interrupted, or an error occurs. | | `close()` | Ends the session and emits `SessionEnded`. | | `interrupt()` | Cancels the current `process_input` call. | | `cancel_token()` | Returns a `CancellationToken` for external cancellation. | @@ -110,8 +110,6 @@ All fields are public. Key settings with their defaults: | Field | Default | Description | |---|---|---| -| `max_turns` | `0` (unlimited) | Maximum conversation turns before stopping. | -| `max_tool_rounds_per_input` | `200` | Maximum tool execution rounds per `process_input` call. | | `default_command_timeout_ms` | `10,000` | Default timeout for Bash tool commands. | | `max_command_timeout_ms` | `600,000` | Maximum allowed timeout for Bash tool commands. | | `enable_loop_detection` | `true` | Detect and break out of repetitive tool call patterns. | @@ -221,7 +219,6 @@ Key `AgentEvent` variants: | `ToolCallCompleted { tool_name, tool_call_id, output, is_error }` | A tool call finished. | | `Error { error }` | An `AgentError` occurred. | | `LoopDetected` | The agent is repeating itself. | -| `TurnLimitReached { max_turns }` | Turn limit hit. | | `CompactionStarted` / `CompactionCompleted` | Context window compaction. | | `SubAgentSpawned` / `SubAgentCompleted` | Sub-agent lifecycle. | | `McpServerReady` / `McpServerFailed` | MCP server connection status. | @@ -296,7 +293,7 @@ All fallible `Session` methods return `Result`: | `SessionClosed` | `process_input` was called on a closed session. | | `InvalidState(String)` | The session is in an unexpected state. | | `ToolExecution(String)` | A tool execution failed. | -| `Interrupted(InterruptReason)` | The session was cancelled (`Cancelled`) or timed out (`WallClockTimeout`). | +| `Interrupted(InterruptReason)` | The session was cancelled or timed out. | --- diff --git a/lib/components/fabro-agent/README.md b/lib/components/fabro-agent/README.md index 5cac1be82..69764459e 100644 --- a/lib/components/fabro-agent/README.md +++ b/lib/components/fabro-agent/README.md @@ -98,8 +98,6 @@ pub trait Sandbox: Send + Sync { ```rust pub struct SessionConfig { - pub max_turns: usize, // 0 = unlimited - pub max_tool_rounds_per_input: usize, // default: 200 pub default_command_timeout_ms: u64, // default: 10s pub max_command_timeout_ms: u64, // default: 600s pub enable_loop_detection: bool, // default: true @@ -134,7 +132,6 @@ let env = Arc::new(LocalSandbox::new( // 4. Configure the session let config = SessionConfig { - max_tool_rounds_per_input: 50, enable_loop_detection: true, user_instructions: Some("Always write tests first".into()), ..SessionConfig::default() diff --git a/lib/components/fabro-agent/src/config.rs b/lib/components/fabro-agent/src/config.rs index c54d49f01..bc22fd75c 100644 --- a/lib/components/fabro-agent/src/config.rs +++ b/lib/components/fabro-agent/src/config.rs @@ -106,8 +106,6 @@ pub struct ToolSecrets { #[derive(Clone)] pub struct SessionOptions { - pub max_turns: usize, - pub max_tool_rounds_per_input: usize, pub default_command_timeout_ms: u64, pub max_command_timeout_ms: u64, pub reasoning_effort: Option, @@ -149,8 +147,6 @@ pub struct SessionOptions { impl std::fmt::Debug for SessionOptions { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("SessionOptions") - .field("max_turns", &self.max_turns) - .field("max_tool_rounds_per_input", &self.max_tool_rounds_per_input) .field( "default_command_timeout_ms", &self.default_command_timeout_ms, @@ -196,8 +192,6 @@ impl std::fmt::Debug for SessionOptions { impl Default for SessionOptions { fn default() -> Self { Self { - max_turns: 0, - max_tool_rounds_per_input: 0, default_command_timeout_ms: 10_000, max_command_timeout_ms: 600_000, max_tokens: None, @@ -280,8 +274,6 @@ mod tests { #[test] fn default_config_values() { let config = SessionOptions::default(); - assert_eq!(config.max_turns, 0); - assert_eq!(config.max_tool_rounds_per_input, 0); assert_eq!(config.default_command_timeout_ms, 10_000); assert_eq!(config.max_command_timeout_ms, 600_000); assert!(config.reasoning_effort.is_none()); @@ -328,13 +320,10 @@ mod tests { #[test] fn config_with_custom_values() { let config = SessionOptions { - max_turns: 50, reasoning_effort: Some(ReasoningEffort::High), ..Default::default() }; - assert_eq!(config.max_turns, 50); assert_eq!(config.reasoning_effort, Some(ReasoningEffort::High)); - assert_eq!(config.max_tool_rounds_per_input, 0); } #[test] diff --git a/lib/components/fabro-agent/src/session.rs b/lib/components/fabro-agent/src/session.rs index 34986a29d..759aa8884 100644 --- a/lib/components/fabro-agent/src/session.rs +++ b/lib/components/fabro-agent/src/session.rs @@ -1195,10 +1195,6 @@ impl Session { self.config.speed = speed; } - pub const fn set_max_turns(&mut self, max_turns: usize) { - self.config.max_turns = max_turns; - } - #[must_use] pub const fn history(&self) -> &History { &self.history @@ -1210,7 +1206,14 @@ impl Session { } pub async fn process_input(&mut self, input: &str) -> Result<(), Error> { - self.process_input_with_runtime(input, AgentToolRuntime::default()) + self.process_input_with_output(input).await.map(drop) + } + + pub(crate) async fn process_input_with_output( + &mut self, + input: &str, + ) -> Result, Error> { + self.process_input_with_runtime_and_output(input, AgentToolRuntime::default()) .await } @@ -1237,6 +1240,16 @@ impl Session { input: &str, agent_tool_runtime: AgentToolRuntime, ) -> Result<(), Error> { + self.process_input_with_runtime_and_output(input, agent_tool_runtime) + .await + .map(drop) + } + + async fn process_input_with_runtime_and_output( + &mut self, + input: &str, + agent_tool_runtime: AgentToolRuntime, + ) -> Result, Error> { let mut timing = SessionInputTiming::default(); let mut usage = TokenCounts::default(); let mut cost = None; @@ -1322,7 +1335,7 @@ impl Session { timing: &mut SessionInputTiming, usage_accumulator: &mut TokenCounts, cost_accumulator: &mut Option, - ) -> Result<(), Error> { + ) -> Result, Error> { const STREAM_CONSUME_RETRIES: usize = 3; if self.state == SessionState::Closed { @@ -1360,8 +1373,6 @@ impl Session { text: expanded_input.clone(), }); - let mut round_count: usize = 0; - loop { // Top-of-loop: if the previous round's interrupt token fired, // swap in a fresh one before draining and rebuilding state. @@ -1394,26 +1405,6 @@ impl Session { self.wait_for_steer_if_needed().await?; self.drain_steering(); - // Check max_tool_rounds_per_input - if self.config.max_tool_rounds_per_input > 0 - && round_count >= self.config.max_tool_rounds_per_input - { - self.event_emitter - .emit(self.id.clone(), AgentEvent::TurnLimitReached { - max_turns: self.config.max_tool_rounds_per_input, - }); - break; - } - - // Check max_turns - if self.config.max_turns > 0 && self.history.turns().len() >= self.config.max_turns { - self.event_emitter - .emit(self.id.clone(), AgentEvent::TurnLimitReached { - max_turns: self.config.max_turns, - }); - break; - } - // Snapshot the per-round token; it stays stable for this iteration. let round_token = self .round_token @@ -1773,11 +1764,9 @@ impl Session { if should_continue { continue; } - break; + return Ok((!text.trim().is_empty()).then_some(text)); } - round_count += 1; - // Build a composite cancellation token covering both terminal // cancel and round (steer) interrupt. Tools observe it // cooperatively — they synthesize "Cancelled" results rather @@ -1859,8 +1848,6 @@ impl Session { .emit(self.id.clone(), AgentEvent::LoopDetected); } } - - Ok(()) } async fn compact_if_needed(&mut self) { @@ -2259,8 +2246,9 @@ mod tests { #[tokio::test] async fn text_only_response_natural_completion() { let mut session = make_session(vec![text_response("Hello there!")]).await; - session.process_input("Hi").await.unwrap(); + let output = session.process_input_with_output("Hi").await.unwrap(); + assert_eq!(output.as_deref(), Some("Hello there!")); assert_eq!(session.state(), SessionState::Idle); let turns = session.history().turns(); // UserTurn + AssistantTurn = 2 @@ -2445,55 +2433,12 @@ mod tests { } #[tokio::test] - async fn max_tool_rounds_enforced() { - let mut registry = ToolRegistry::new(); - registry.register(make_echo_tool()); + async fn empty_natural_completion_has_no_output() { + let mut session = make_session(vec![text_response(" ")]).await; - // Respond with tool calls indefinitely - let responses = vec![ - tool_call_response("echo", "call_1", serde_json::json!({"text": "a"})), - tool_call_response("echo", "call_2", serde_json::json!({"text": "b"})), - tool_call_response("echo", "call_3", serde_json::json!({"text": "c"})), - ]; + let output = session.process_input_with_output("Hi").await.unwrap(); - let config = SessionOptions { - max_tool_rounds_per_input: 2, - enable_loop_detection: false, - ..Default::default() - }; - - let mut session = make_session_with_tools_and_config(responses, registry, config).await; - session.process_input("Keep using tools").await.unwrap(); - - // Should stop after 2 rounds: User + (Asst+ToolResult) * 2 = 5 turns - assert_eq!(session.state(), SessionState::Idle); - let turns = session.history().turns(); - assert_eq!(turns.len(), 5); - } - - #[tokio::test] - async fn max_turns_enforced() { - let responses = vec![ - text_response("first"), - text_response("second"), - text_response("should not reach"), - ]; - - let config = SessionOptions { - max_turns: 3, - ..Default::default() - }; - - let mut session = make_session_with_config(responses, config).await; - - // First input: adds User + Assistant = 2 turns - session.process_input("one").await.unwrap(); - assert_eq!(session.history().turns().len(), 2); - - // Second input: adds User (now 3 turns), then max_turns check triggers - session.process_input("two").await.unwrap(); - // Should have 3 turns total (User + Asst + User), max_turns hit before LLM call - assert_eq!(session.history().turns().len(), 3); + assert_eq!(output, None); } #[tokio::test] diff --git a/lib/components/fabro-agent/src/subagent.rs b/lib/components/fabro-agent/src/subagent.rs index 52cec77c6..aad7b975d 100644 --- a/lib/components/fabro-agent/src/subagent.rs +++ b/lib/components/fabro-agent/src/subagent.rs @@ -10,7 +10,7 @@ use crate::error::Error; use crate::session::Session; use crate::tool_registry::{RegisteredTool, ToolSource}; use crate::tools::required_str; -use crate::types::{AgentEvent, Message, SessionEvent}; +use crate::types::{AgentEvent, SessionEvent}; pub type SessionFactory = Arc Session + Send + Sync>; @@ -112,15 +112,18 @@ impl SubAgentManager { let task_prompt_for_spawn = task_prompt.clone(); let task = tokio::spawn(async move { session.initialize().await?; - session.process_input(&task_prompt_for_spawn).await?; + let output = session + .process_input_with_output(&task_prompt_for_spawn) + .await? + .ok_or_else(|| { + Error::InvalidState( + "Subagent completed without a non-empty final response".to_string(), + ) + })?; let turns = session.history().turns(); - let last_text = turns.iter().rev().find_map(|t| match t { - Message::Assistant { content, .. } => Some(content.clone()), - _ => None, - }); Ok(SubAgentResult { - output: last_text.unwrap_or_default(), - success: true, + output, + success: true, turns_used: turns.len(), }) }); @@ -314,18 +317,6 @@ pub fn make_spawn_agent_tool( "task": { "type": "string", "description": "The task description for the subagent" - }, - "working_dir": { - "type": "string", - "description": "Working directory for the subagent" - }, - "model": { - "type": "string", - "description": "Model to use for the subagent" - }, - "max_turns": { - "type": "integer", - "description": "Maximum number of turns for the subagent" } }, "required": ["task"] @@ -337,13 +328,6 @@ pub fn make_spawn_agent_tool( Box::pin(async move { let task = required_str(&args, "task")?; - // Extract optional max_turns parameter - let max_turns = args - .get("max_turns") - .and_then(serde_json::Value::as_u64) - .map(|v| usize::try_from(v).unwrap_or(usize::MAX)); - - // Note: working_dir and model require session factory changes to wire through let mut session = session_factory(); // Inherit the parent agent's root session ID so todo tools // that scope by root (e.g. Anthropic tasks) share one list @@ -351,9 +335,6 @@ pub fn make_spawn_agent_tool( if let Some(root) = ctx.root_session_id.as_ref().or(ctx.session_id.as_ref()) { session.set_root_session_id(root.clone()); } - // Default subagent max_turns is 0 (unlimited) per spec (overridable via - // parameter) - session.set_max_turns(max_turns.unwrap_or(0)); let mut mgr = manager.lock().await; mgr.spawn(session, task.to_string(), current_depth) .map_err(|e| e.to_string()) @@ -619,7 +600,11 @@ mod tests { let spawn_tool = make_spawn_agent_tool(manager.clone(), factory, 0); assert_eq!(spawn_tool.definition.name, "spawn_agent"); - assert!(spawn_tool.definition.parameters["properties"]["task"].is_object()); + let spawn_properties = spawn_tool.definition.parameters["properties"] + .as_object() + .unwrap(); + assert_eq!(spawn_properties.len(), 1); + assert!(spawn_properties["task"].is_object()); let spawn_required = spawn_tool.definition.parameters["required"] .as_array() .unwrap(); @@ -831,6 +816,21 @@ mod tests { )); } + #[tokio::test] + async fn empty_final_response_is_not_reported_as_success() { + let mut manager = SubAgentManager::new(3); + let session = make_session(vec![text_response("")]).await; + let agent_id = manager.spawn(session, "Do something".into(), 0).unwrap(); + + let result = manager.wait(&agent_id).await; + + assert!( + matches!(result, Err(Error::InvalidState(message)) if message.contains( + "without a non-empty final response" + )) + ); + } + #[tokio::test] async fn send_input_to_completed_agent_errors() { let mut manager = SubAgentManager::new(3); diff --git a/lib/components/fabro-agent/src/types.rs b/lib/components/fabro-agent/src/types.rs index 94cb6808e..d77753519 100644 --- a/lib/components/fabro-agent/src/types.rs +++ b/lib/components/fabro-agent/src/types.rs @@ -284,9 +284,6 @@ pub enum AgentEvent { details: serde_json::Value, }, LoopDetected, - TurnLimitReached { - max_turns: usize, - }, SteeringInjected { text: String, /// Principal that authored the steer. Lifted to top-level @@ -464,9 +461,6 @@ impl AgentEvent { Self::LoopDetected => { warn!(session_id, "Loop detected"); } - Self::TurnLimitReached { max_turns } => { - warn!(session_id, max_turns, "Message limit reached"); - } Self::SteeringInjected { text, .. } => { debug!(session_id, text_len = text.len(), "Steering injected"); } diff --git a/lib/components/fabro-agent/tests/it/compaction.rs b/lib/components/fabro-agent/tests/it/compaction.rs index 084deae6c..14cb88d5e 100644 --- a/lib/components/fabro-agent/tests/it/compaction.rs +++ b/lib/components/fabro-agent/tests/it/compaction.rs @@ -53,7 +53,6 @@ fn make_openai_session(cwd: &Path, base_url: String, api_key: String) -> Session let profile: Arc = Arc::new(OpenAiProfile::new(MODEL)); let sandbox = Arc::new(LocalSandbox::new(cwd.to_path_buf())); let options = SessionOptions { - max_turns: 20, enable_context_compaction: true, compaction_threshold_percent: 80, compaction_preserve_turns: 6, diff --git a/lib/components/fabro-agent/tests/it/parity_matrix.rs b/lib/components/fabro-agent/tests/it/parity_matrix.rs index 6b60d1cf7..1bd80ed78 100644 --- a/lib/components/fabro-agent/tests/it/parity_matrix.rs +++ b/lib/components/fabro-agent/tests/it/parity_matrix.rs @@ -126,11 +126,7 @@ async fn make_session( profile.register_subagent_tools(manager, factory, 0); let profile: Arc = Arc::from(profile); - let config = SessionOptions { - max_turns: 20, - ..SessionOptions::default() - }; - Session::new(client, profile, env, config, None) + Session::new(client, profile, env, SessionOptions::default(), None) } async fn make_session_with_config( @@ -315,20 +311,17 @@ async fn openai_compatible_twin_uses_json_edit_file_tool() { "old_string": "old", "new_string": "new" }), - )), + )) + .text("Done."), ) .load(twin_openai().await) .await; - let config = SessionOptions { - max_turns: 2, - ..SessionOptions::default() - }; let mut session = make_openai_compatible_twin_session( ProviderId::new("litellm"), "gpt-5.4-mini", tmp.path(), - config, + SessionOptions::default(), &twin, ); session.initialize().await.unwrap(); @@ -767,7 +760,6 @@ macro_rules! reasoning_effort_tests { async fn $test_name() { let tmp = tempfile::tempdir().expect("failed to create tempdir"); let config = SessionOptions { - max_turns: 20, reasoning_effort: Some(fabro_llm::types::ReasoningEffort::Low), ..SessionOptions::default() }; @@ -846,7 +838,6 @@ macro_rules! loop_detection_tests { async fn $test_name() { let tmp = tempfile::tempdir().expect("failed to create tempdir"); let config = SessionOptions { - max_turns: 20, loop_detection_window: 3, ..SessionOptions::default() }; diff --git a/lib/components/fabro-workflow/src/error.rs b/lib/components/fabro-workflow/src/error.rs index 71ded36ac..75b6d317f 100644 --- a/lib/components/fabro-workflow/src/error.rs +++ b/lib/components/fabro-workflow/src/error.rs @@ -90,8 +90,6 @@ const BUDGET_EXHAUSTED_HINTS: &[&str] = &[ "context length", "budget", "quota exceeded", - "max_turns", - "max turns", "max_tokens", "max tokens", "context window exceeded", @@ -1233,7 +1231,7 @@ mod tests { #[test] fn budget_exhausted_hints_count() { - assert_eq!(BUDGET_EXHAUSTED_HINTS.len(), 12); + assert_eq!(BUDGET_EXHAUSTED_HINTS.len(), 10); } #[test] @@ -1313,22 +1311,6 @@ mod tests { ); } - #[test] - fn classify_reason_max_turns() { - assert_eq!( - classify_failure_reason("hit max_turns limit"), - FailureCategory::BudgetExhausted - ); - } - - #[test] - fn classify_reason_max_turns_space() { - assert_eq!( - classify_failure_reason("max turns reached"), - FailureCategory::BudgetExhausted - ); - } - #[test] fn classify_reason_max_tokens() { assert_eq!( diff --git a/lib/components/fabro-workflow/src/event/convert.rs b/lib/components/fabro-workflow/src/event/convert.rs index 528371880..e5eb1f41d 100644 --- a/lib/components/fabro-workflow/src/event/convert.rs +++ b/lib/components/fabro-workflow/src/event/convert.rs @@ -674,12 +674,6 @@ fn event_body_from_event(event: &Event) -> EventBody { AgentEvent::LoopDetected => { EventBody::AgentLoopDetected(fabro_types::AgentLoopDetectedProps { visit: *visit }) } - AgentEvent::TurnLimitReached { max_turns } => { - EventBody::AgentTurnLimitReached(fabro_types::AgentTurnLimitReachedProps { - max_turns: *max_turns, - visit: *visit, - }) - } AgentEvent::SteeringInjected { text, .. } => { EventBody::AgentSteeringInjected(fabro_types::AgentSteeringInjectedProps { text: text.clone(), diff --git a/lib/components/fabro-workflow/src/event/names.rs b/lib/components/fabro-workflow/src/event/names.rs index acfee89e6..5eb339ce8 100644 --- a/lib/components/fabro-workflow/src/event/names.rs +++ b/lib/components/fabro-workflow/src/event/names.rs @@ -81,7 +81,6 @@ pub fn event_name(event: &Event) -> &'static str { AgentEvent::Error { .. } => "agent.error", AgentEvent::Warning { .. } => "agent.warning", AgentEvent::LoopDetected => "agent.loop.detected", - AgentEvent::TurnLimitReached { .. } => "agent.turn.limit", AgentEvent::SteeringInjected { .. } => "agent.steering.injected", AgentEvent::CompactionStarted { .. } => "agent.compaction.started", AgentEvent::CompactionCompleted { .. } => "agent.compaction.completed", diff --git a/lib/foundation/fabro-types/src/run_event/agent.rs b/lib/foundation/fabro-types/src/run_event/agent.rs index 138344f9f..a38c07df2 100644 --- a/lib/foundation/fabro-types/src/run_event/agent.rs +++ b/lib/foundation/fabro-types/src/run_event/agent.rs @@ -195,12 +195,6 @@ pub struct AgentLoopDetectedProps { pub visit: u32, } -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct AgentTurnLimitReachedProps { - pub max_turns: usize, - pub visit: u32, -} - #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct AgentSteeringInjectedProps { pub text: String, diff --git a/lib/foundation/fabro-types/src/run_event/mod.rs b/lib/foundation/fabro-types/src/run_event/mod.rs index 4aed5b3af..46d63e531 100644 --- a/lib/foundation/fabro-types/src/run_event/mod.rs +++ b/lib/foundation/fabro-types/src/run_event/mod.rs @@ -220,8 +220,6 @@ pub enum EventBody { AgentWarning(AgentWarningProps), #[serde(rename = "agent.loop.detected")] AgentLoopDetected(AgentLoopDetectedProps), - #[serde(rename = "agent.turn.limit")] - AgentTurnLimitReached(AgentTurnLimitReachedProps), #[serde(rename = "agent.steering.injected")] AgentSteeringInjected(AgentSteeringInjectedProps), #[serde(rename = "agent.pair.user_message")] @@ -499,7 +497,6 @@ impl EventBody { Self::AgentError(_) => "agent.error", Self::AgentWarning(_) => "agent.warning", Self::AgentLoopDetected(_) => "agent.loop.detected", - Self::AgentTurnLimitReached(_) => "agent.turn.limit", Self::AgentSteeringInjected(_) => "agent.steering.injected", Self::AgentPairUserMessage(_) => "agent.pair.user_message", Self::AgentPairSystemMessage(_) => "agent.pair.system_message", @@ -671,7 +668,6 @@ fn is_known_event_name(event: &str) -> bool { | "agent.error" | "agent.warning" | "agent.loop.detected" - | "agent.turn.limit" | "agent.steering.injected" | "agent.pair.user_message" | "agent.pair.system_message" From 7c771fc5d754caf7ef72de05667a391064b97f98 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 23 Jul 2026 20:11:21 -0400 Subject: [PATCH 2/2] test(twin-openai): accept tool call histories --- test/twin/openai/src/openai/models.rs | 117 +++++++++++++++--- .../openai/tests/chat_completions_contract.rs | 38 ++++++ 2 files changed, 140 insertions(+), 15 deletions(-) diff --git a/test/twin/openai/src/openai/models.rs b/test/twin/openai/src/openai/models.rs index abd2e47e7..c6b33bc31 100644 --- a/test/twin/openai/src/openai/models.rs +++ b/test/twin/openai/src/openai/models.rs @@ -625,15 +625,34 @@ impl ChatCompletionsRequest { #[derive(Clone, Debug, Deserialize)] #[serde(deny_unknown_fields)] pub struct ChatMessage { - pub role: String, - pub content: Value, + pub role: String, + pub content: Option, + pub reasoning_content: Option, + pub tool_call_id: Option, + pub tool_calls: Option>, +} + +#[derive(Clone, Debug, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct ChatMessageToolCall { + pub id: String, + #[serde(rename = "type")] + pub kind: String, + pub function: ChatMessageToolCallFunction, +} + +#[derive(Clone, Debug, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct ChatMessageToolCallFunction { + pub name: String, + pub arguments: String, } impl ChatMessage { fn extract_texts(&self) -> Vec { - match &self.content { - Value::String(text) => vec![normalize_whitespace(text)], - Value::Array(parts) => parts + match self.content.as_ref() { + Some(Value::String(text)) => vec![normalize_whitespace(text)], + Some(Value::Array(parts)) => parts .iter() .filter_map(|part| { part.get("text") @@ -647,13 +666,21 @@ impl ChatMessage { fn contains_reasoning_content(&self) -> bool { self.role == "assistant" - && self.content.as_array().is_some_and(|parts| { - parts.iter().any(|part| { - part.get("type") - .and_then(Value::as_str) - .is_some_and(|kind| kind == "reasoning") - }) - }) + && (self + .reasoning_content + .as_deref() + .is_some_and(|reasoning| !reasoning.trim().is_empty()) + || self + .content + .as_ref() + .and_then(Value::as_array) + .is_some_and(|parts| { + parts.iter().any(|part| { + part.get("type") + .and_then(Value::as_str) + .is_some_and(|kind| kind == "reasoning") + }) + })) } } @@ -680,14 +707,24 @@ fn validate_chat_message(message: &ChatMessage) -> Result<(), OpenAiError> { )); } - match &message.content { - Value::String(_) => Ok(()), - Value::Array(parts) if !parts.is_empty() => { + validate_chat_tool_fields(message)?; + + match message.content.as_ref() { + Some(Value::String(_)) => Ok(()), + Some(Value::Array(parts)) if !parts.is_empty() => { for part in parts { validate_chat_message_part(part, &message.role)?; } Ok(()) } + None if message.role == "assistant" + && message + .tool_calls + .as_ref() + .is_some_and(|tool_calls| !tool_calls.is_empty()) => + { + Ok(()) + } _ => Err(OpenAiError::invalid_request( "messages", "unsupported message content shape", @@ -695,6 +732,56 @@ fn validate_chat_message(message: &ChatMessage) -> Result<(), OpenAiError> { } } +fn validate_chat_tool_fields(message: &ChatMessage) -> Result<(), OpenAiError> { + if message.role == "tool" { + if message + .tool_call_id + .as_deref() + .is_none_or(|tool_call_id| tool_call_id.trim().is_empty()) + { + return Err(OpenAiError::invalid_request( + "messages", + "tool messages require a tool_call_id", + )); + } + } else if message.tool_call_id.is_some() { + return Err(OpenAiError::invalid_request( + "messages", + "tool_call_id is only supported on tool messages", + )); + } + + let Some(tool_calls) = &message.tool_calls else { + return Ok(()); + }; + if message.role != "assistant" { + return Err(OpenAiError::invalid_request( + "messages", + "tool_calls are only supported on assistant messages", + )); + } + if tool_calls.is_empty() { + return Err(OpenAiError::invalid_request( + "messages", + "tool_calls must not be empty", + )); + } + for tool_call in tool_calls { + if tool_call.id.trim().is_empty() + || tool_call.kind != "function" + || tool_call.function.name.trim().is_empty() + || tool_call.function.arguments.trim().is_empty() + { + return Err(OpenAiError::invalid_request( + "messages", + "invalid assistant tool call", + )); + } + } + + Ok(()) +} + fn validate_chat_message_part(part: &Value, role: &str) -> Result<(), OpenAiError> { let Some(object) = part.as_object() else { return Err(OpenAiError::invalid_request( diff --git a/test/twin/openai/tests/chat_completions_contract.rs b/test/twin/openai/tests/chat_completions_contract.rs index db1ace170..6fe4c27fe 100644 --- a/test/twin/openai/tests/chat_completions_contract.rs +++ b/test/twin/openai/tests/chat_completions_contract.rs @@ -90,6 +90,44 @@ async fn chat_completions_accepts_supported_openai_compatible_fields() { ); } +#[tokio::test] +async fn chat_completions_accepts_tool_call_history() { + let server = common::spawn_server().await.expect("server should start"); + + let response = server + .post_chat(json!({ + "model": "gpt-test", + "messages": [ + { "role": "user", "content": "replace old with new" }, + { + "role": "assistant", + "tool_calls": [{ + "id": "call_edit", + "type": "function", + "function": { + "name": "edit_file", + "arguments": "{\"old\":\"old\",\"new\":\"new\"}" + } + }] + }, + { + "role": "tool", + "content": "Updated data.txt", + "tool_call_id": "call_edit" + } + ], + "stream": false + })) + .await; + + assert_eq!(response.status(), 200); + let body = response.json::().await.expect("json"); + assert_eq!( + body["choices"][0]["message"]["content"], + "deterministic: replace old with new" + ); +} + #[tokio::test] async fn chat_completions_supports_scripted_tool_call_and_json_schema() { let server = common::spawn_server().await.expect("server should start");