-
Notifications
You must be signed in to change notification settings - Fork 790
Bound direct subagent waits #311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,8 +11,11 @@ use serde::Deserialize; | |
| use serde_json::{Value, json}; | ||
| use std::collections::{HashMap, HashSet}; | ||
| use std::sync::{Arc, Mutex}; | ||
| use std::time::Duration; | ||
| use tokio::sync::broadcast; | ||
|
|
||
| const DEFAULT_SUBAGENT_TIMEOUT_SECS: u64 = 300; | ||
|
|
||
| pub struct SubagentTool { | ||
| provider: Arc<dyn Provider>, | ||
| registry: Registry, | ||
|
|
@@ -55,6 +58,8 @@ struct SubagentInput { | |
| session_id: Option<String>, | ||
| #[serde(default)] | ||
| output_mode: SubagentOutputMode, | ||
| #[serde(default)] | ||
| timeout_secs: Option<u64>, | ||
| #[serde(rename = "command", default)] | ||
| _command: Option<String>, | ||
| } | ||
|
|
@@ -115,6 +120,12 @@ impl Tool for SubagentTool { | |
| "enum": ["answer", "compact", "full_transcript"], | ||
| "description": "Return mode. 'answer' returns the final answer only, 'compact' adds a user-visible transcript, and 'full_transcript' adds raw persisted messages. Defaults to 'answer'." | ||
| }, | ||
| "timeout_secs": { | ||
| "type": "integer", | ||
| "minimum": 1, | ||
| "default": DEFAULT_SUBAGENT_TIMEOUT_SECS, | ||
| "description": "Maximum seconds to wait for the subagent before returning a timeout error. Defaults to 300." | ||
| }, | ||
| "command": { | ||
| "type": "string", | ||
| "description": "Source command." | ||
|
|
@@ -201,8 +212,8 @@ impl Tool for SubagentTool { | |
| }); | ||
|
|
||
| logging::info(&format!( | ||
| "Subagent starting: {} (type: {})", | ||
| params.description, params.subagent_type | ||
| "Subagent starting: {} (type: {}) session_id={} model={}", | ||
| params.description, params.subagent_type, session.id, resolved_model | ||
| )); | ||
|
|
||
| // Run subagent on an isolated provider fork so model/session changes do not | ||
|
|
@@ -215,18 +226,39 @@ impl Tool for SubagentTool { | |
| ); | ||
|
|
||
| let start = std::time::Instant::now(); | ||
| let final_text = agent.run_once_capture(¶ms.prompt).await.map_err(|err| { | ||
| logging::warn(&format!( | ||
| "[tool:subagent] subagent failed description={} type={} session_id={} model={} error={}", | ||
| params.description, | ||
| params.subagent_type, | ||
| agent.session_id(), | ||
| resolved_model, | ||
| err | ||
| )); | ||
| err | ||
| })?; | ||
| let sub_session_id = agent.session_id().to_string(); | ||
| let timeout_secs = params | ||
| .timeout_secs | ||
| .unwrap_or(DEFAULT_SUBAGENT_TIMEOUT_SECS) | ||
| .max(1); | ||
| let final_text = match tokio::time::timeout( | ||
| Duration::from_secs(timeout_secs), | ||
| agent.run_once_capture(¶ms.prompt), | ||
| ) | ||
| .await | ||
| { | ||
| Ok(result) => result.map_err(|err| { | ||
| logging::warn(&format!( | ||
| "[tool:subagent] subagent failed description={} type={} session_id={} model={} error={}", | ||
| params.description, | ||
| params.subagent_type, | ||
| sub_session_id, | ||
| resolved_model, | ||
| err | ||
| )); | ||
| err | ||
| })?, | ||
| Err(_) => { | ||
| listener.abort(); | ||
| let message = | ||
| subagent_timeout_error(¶ms.description, &sub_session_id, timeout_secs); | ||
| logging::warn(&format!( | ||
| "[tool:subagent] {} type={} model={}", | ||
| message, params.subagent_type, resolved_model | ||
| )); | ||
| return Err(anyhow::anyhow!(message)); | ||
|
ritamgh marked this conversation as resolved.
|
||
| } | ||
| }; | ||
|
Comment on lines
+234
to
+261
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. The goal of this PR is to bound the parent agent’s direct wait and return the child |
||
| let history = if params.output_mode == SubagentOutputMode::Compact { | ||
| Some(agent.get_history()) | ||
| } else { | ||
|
|
@@ -288,6 +320,13 @@ fn subagent_display_title(params: &SubagentInput, model: &str) -> String { | |
| ) | ||
| } | ||
|
|
||
| fn subagent_timeout_error(description: &str, sub_session_id: &str, timeout_secs: u64) -> String { | ||
| format!( | ||
| "Subagent timed out after {timeout_secs}s before returning a final answer: {description}. \ | ||
| Child session `{sub_session_id}` was created and saved, so inspect or resume that session instead of waiting on this direct subagent call. For long-running work, prefer durable `swarm spawn`/`swarm await_members`." | ||
| ) | ||
| } | ||
|
|
||
| impl SubagentOutputMode { | ||
| fn as_str(self) -> &'static str { | ||
| match self { | ||
|
|
@@ -369,7 +408,7 @@ fn format_compact_subagent_history(messages: &[HistoryMessage]) -> String { | |
| mod tests { | ||
| use super::{ | ||
| SubagentInput, SubagentOutputMode, format_compact_subagent_history, format_subagent_output, | ||
| subagent_display_title, | ||
| subagent_display_title, subagent_timeout_error, | ||
| }; | ||
| use crate::protocol::HistoryMessage; | ||
|
|
||
|
|
@@ -382,6 +421,7 @@ mod tests { | |
| model: None, | ||
| session_id: None, | ||
| output_mode: SubagentOutputMode::Answer, | ||
| timeout_secs: None, | ||
| _command: None, | ||
| }; | ||
|
|
||
|
|
@@ -391,6 +431,17 @@ mod tests { | |
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn subagent_timeout_error_includes_recovery_details() { | ||
| let message = subagent_timeout_error("Review the plan", "session_child", 300); | ||
|
|
||
| assert!(message.contains("timed out after 300s")); | ||
| assert!(message.contains("Review the plan")); | ||
| assert!(message.contains("session_child")); | ||
| assert!(message.contains("resume that session")); | ||
| assert!(message.contains("swarm spawn")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn resolve_model_prefers_explicit_then_existing_then_parent_then_provider() { | ||
| assert_eq!( | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.