diff --git a/crates/buzz-acp/src/config.rs b/crates/buzz-acp/src/config.rs index 3d4e67d0f55..e0e424f0185 100644 --- a/crates/buzz-acp/src/config.rs +++ b/crates/buzz-acp/src/config.rs @@ -20,11 +20,11 @@ use crate::filter::SubscriptionRule; /// /// Sized for slow turns where the agent may go silent on its outer ACP channel /// while running long sub-tools (e.g. a buzz-agent running another agent, or -/// codex/claude doing multi-minute single tool calls). 900s gives 300s of -/// breathing room above the 600s max shell timeout, so legitimate long-running +/// codex/claude doing multi-minute single tool calls). 1500s gives 300s of +/// breathing room above the 1200s max shell timeout, so legitimate long-running /// tool calls don't race the idle deadline. /// Override via `--idle-timeout` / `BUZZ_ACP_IDLE_TIMEOUT`. -pub(crate) const DEFAULT_IDLE_TIMEOUT_SECS: u64 = 900; +pub(crate) const DEFAULT_IDLE_TIMEOUT_SECS: u64 = 1_500; /// Default absolute wall-clock cap per agent turn (2 hours). /// Override via `--max-turn-duration` / `BUZZ_ACP_MAX_TURN_DURATION`. @@ -2757,9 +2757,9 @@ channels = "ALL" // ── Idle timeout constant + guard (PR #935) ─────────────────────────────── #[test] - fn default_idle_timeout_is_900_seconds() { + fn default_idle_timeout_is_1500_seconds() { // Lock the constant value so accidental changes are caught. - assert_eq!(DEFAULT_IDLE_TIMEOUT_SECS, 900); + assert_eq!(DEFAULT_IDLE_TIMEOUT_SECS, 1_500); } #[test] @@ -2779,6 +2779,45 @@ channels = "ALL" } } + #[test] + fn budget_ordering_invariant_shell_cap_plus_headroom_fits_within_idle_timeout() { + // Asserts the three-layer budget relationship introduced in PR #7185: + // buzz-dev-mcp MAX_TIMEOUT_MS (1 200 000 ms = 1 200s) + // ≤ buzz-agent BUZZ_AGENT_TOOL_TIMEOUT_SECS default (1 260s) + // < buzz-acp DEFAULT_IDLE_TIMEOUT_SECS (1 500s) + // + // The idle deadline must strictly outlast the agent tool timeout so a + // legitimately long-running tool call is killed by buzz-agent first (at + // 1 260s) rather than the ACP idle watchdog. The 240s gap gives the agent + // time to handle the timeout, emit a response, and reset the idle clock + // before the ACP connection dies. + // + // If any of these constants change the compiler catches the inversion here. + // Cross-crate constants are mirrored as literals; grep for PR #7185 to + // find the authoritative source if you need to update them. + const SHELL_CAP_MS: u64 = 1_200_000; // buzz-dev-mcp MAX_TIMEOUT_MS + const SHELL_CAP_SECS: u64 = SHELL_CAP_MS / 1_000; + const AGENT_TOOL_TIMEOUT_SECS: u64 = 1_260; // buzz-agent BUZZ_AGENT_TOOL_TIMEOUT_SECS default + + const { + // Shell cap must not exceed the agent's per-tool-call timeout. + assert!( + SHELL_CAP_SECS <= AGENT_TOOL_TIMEOUT_SECS, + "shell cap must be <= agent tool timeout" + ); + // Agent tool timeout must be strictly less than the ACP idle deadline. + assert!( + AGENT_TOOL_TIMEOUT_SECS < DEFAULT_IDLE_TIMEOUT_SECS, + "agent tool timeout must be < ACP idle timeout" + ); + // ACP idle timeout must remain below the max turn duration. + assert!( + DEFAULT_IDLE_TIMEOUT_SECS < DEFAULT_MAX_TURN_DURATION_SECS, + "ACP idle timeout must be < max turn duration" + ); + } + } + // --- BUZZ_ACP_ALLOWED_RESPOND_TO gate --- fn parse_allowed_respond_to(raw: &[&str]) -> Result, ConfigError> { diff --git a/crates/buzz-agent/README.md b/crates/buzz-agent/README.md index 56e62cf9e79..f2d68d4d8cd 100644 --- a/crates/buzz-agent/README.md +++ b/crates/buzz-agent/README.md @@ -159,7 +159,7 @@ Everything is environment variables. No flags, no config files. (We are a subpro | `BUZZ_AGENT_MAX_CONTEXT_TOKENS` | `200000` | Provider context window used by the handoff gate. | | `BUZZ_AGENT_MAX_HANDOFFS` | `10` | Max context handoffs per session before falling back to truncation. | | `BUZZ_AGENT_LLM_TIMEOUT_SECS` | `240` | Max seconds with no response bytes before abandoning an LLM call (per-read inactivity, not wall-clock). | -| `BUZZ_AGENT_TOOL_TIMEOUT_SECS` | `660` | Per-tool call timeout in seconds | +| `BUZZ_AGENT_TOOL_TIMEOUT_SECS` | `1260` | Per-tool call timeout in seconds | | `BUZZ_AGENT_MAX_PARALLEL_TOOLS` | `8` | Max concurrent tool calls per turn (1 = sequential) | | `BUZZ_AGENT_MAX_SESSIONS` | unlimited | Max concurrent ACP sessions. Sessions are cheap; default has no cap. | | `BUZZ_AGENT_MAX_LINE_BYTES` | `4194304` | 4 MiB. Hard cap on inbound JSON-RPC frames. | @@ -326,7 +326,7 @@ The trust boundary is **the operator who launched the agent**. The harness, MCP | Tool calls per turn | 64 | `MAX_TOOL_CALLS_PER_TURN` | | Loop rounds | 0 (unlimited) | `BUZZ_AGENT_MAX_ROUNDS` | | LLM read inactivity timeout | 240 s | `BUZZ_AGENT_LLM_TIMEOUT_SECS` | -| Tool call timeout | 660 s | `BUZZ_AGENT_TOOL_TIMEOUT_SECS` | +| Tool call timeout | 1260 s | `BUZZ_AGENT_TOOL_TIMEOUT_SECS` | ## What This Is NOT diff --git a/crates/buzz-agent/src/config.rs b/crates/buzz-agent/src/config.rs index 202d73e5548..5b2f1d659d2 100644 --- a/crates/buzz-agent/src/config.rs +++ b/crates/buzz-agent/src/config.rs @@ -709,7 +709,7 @@ impl Config { max_output_tokens: parse_env("BUZZ_AGENT_MAX_OUTPUT_TOKENS", 65_536)?, max_token_recoveries: parse_env("BUZZ_AGENT_MAX_TOKEN_RECOVERIES", 3u32)?, llm_timeout: Duration::from_secs(parse_env("BUZZ_AGENT_LLM_TIMEOUT_SECS", 240)?), - tool_timeout: Duration::from_secs(parse_env("BUZZ_AGENT_TOOL_TIMEOUT_SECS", 660)?), + tool_timeout: Duration::from_secs(parse_env("BUZZ_AGENT_TOOL_TIMEOUT_SECS", 1_260)?), mcp_init_timeout: Duration::from_secs(parse_env( "BUZZ_AGENT_MCP_INIT_TIMEOUT_SECS", 30, @@ -2462,4 +2462,24 @@ mod tests { assert_eq!(pricing_authority("https://api.databricks.com/v1"), None); assert_eq!(pricing_authority("https://custom.llm.corp/v1"), None); } + + #[test] + fn default_tool_timeout_is_1260_seconds() { + // Lock the production default so accidental regressions are caught. + // This value must remain >= buzz-dev-mcp's MAX_TIMEOUT_MS (1_200s) to + // give every shell(timeout_ms=1_200_000) call time to complete before + // buzz-agent kills the MCP server. See PR #7185 for the full budget chain. + // + // 1_260s is the literal default passed to parse_env in Config::from_env(). + // Update here if and only if you update that literal; the test name makes + // "grep for old value" reliable. + const DEFAULT_TOOL_TIMEOUT_SECS: u64 = 1_260; + const { + // Shell cap (1_200_000 ms = 1_200s) must fit inside the agent timeout. + assert!( + 1_200u64 <= DEFAULT_TOOL_TIMEOUT_SECS, + "agent tool timeout must be >= dev-mcp shell cap (1200s)" + ); + } + } } diff --git a/crates/buzz-dev-mcp/src/lib.rs b/crates/buzz-dev-mcp/src/lib.rs index 87c3a119317..d555b6ea542 100644 --- a/crates/buzz-dev-mcp/src/lib.rs +++ b/crates/buzz-dev-mcp/src/lib.rs @@ -39,7 +39,7 @@ impl DevMcp { #[tool( name = "shell", - description = "Run a shell command (bash by default; set `BUZZ_SHELL` to use cmd, PowerShell, or another shell). Ephemeral process per call. Output tail-truncated to ~8KB for the LLM; full output (first 10MB) saved to artifact file. timeout_ms defaults to 120000 (2 min) if omitted; capped at 600000 (10 min). For long-running commands (git push with hooks, cargo build, test suites), use 300000+. On PATH: rg (prefer over grep; flags: -n -i -l -g -C --files), tree (flags: -d ; shows line counts), and buzz (Buzz relay CLI — run buzz --help for commands)." + description = "Run a shell command (bash by default; set `BUZZ_SHELL` to use cmd, PowerShell, or another shell). Ephemeral process per call. Output tail-truncated to ~8KB for the LLM; full output (first 10MB) saved to artifact file. timeout_ms defaults to 120000 (2 min) if omitted; capped at 1,200,000 (20 min). For long-running commands (git push with hooks, cargo build, test suites), use 300000+. On PATH: rg (prefer over grep; flags: -n -i -l -g -C --files), tree (flags: -d ; shows line counts), and buzz (Buzz relay CLI — run buzz --help for commands)." )] async fn shell( &self, diff --git a/crates/buzz-dev-mcp/src/shell.rs b/crates/buzz-dev-mcp/src/shell.rs index 7aa95b1d879..140d3c44cc9 100644 --- a/crates/buzz-dev-mcp/src/shell.rs +++ b/crates/buzz-dev-mcp/src/shell.rs @@ -14,7 +14,7 @@ use tokio::process::Command; use tokio_util::sync::CancellationToken; const DEFAULT_TIMEOUT_MS: u64 = 120_000; -const MAX_TIMEOUT_MS: u64 = 600_000; +const MAX_TIMEOUT_MS: u64 = 1_200_000; const MAX_COMMAND_BYTES: usize = 1_000_000; const CAPTURE_CAP: usize = 10 * 1024 * 1024; const MAX_BYTES: usize = 50 * 1024; @@ -121,12 +121,16 @@ pub struct ShellParams { pub command: String, #[serde(default)] pub workdir: Option, - /// Defaults to 120000 ms (2 min) if omitted; capped at 600000 ms (10 min). + /// Defaults to 120000 ms (2 min) if omitted; capped at 1,200,000 ms (20 min). /// For long-running commands (git push with hooks, cargo build, test suites), use 300000+. #[serde(default)] pub timeout_ms: Option, } +fn effective_timeout_ms(requested: Option) -> u64 { + requested.unwrap_or(DEFAULT_TIMEOUT_MS).min(MAX_TIMEOUT_MS) +} + pub async fn run( state: &SharedState, p: ShellParams, @@ -138,10 +142,7 @@ pub async fn run( None, )); } - let timeout_ms = p - .timeout_ms - .unwrap_or(DEFAULT_TIMEOUT_MS) - .min(MAX_TIMEOUT_MS); + let timeout_ms = effective_timeout_ms(p.timeout_ms); let workdir: PathBuf = p .workdir .as_deref() @@ -1002,6 +1003,15 @@ mod tests { serde_json::from_str(&text).expect("json") } + #[test] + fn timeout_bounds_preserve_default_and_cap_requests_at_twenty_minutes() { + assert_eq!(effective_timeout_ms(None), 120_000); + assert_eq!(effective_timeout_ms(Some(120_000)), 120_000); + assert_eq!(effective_timeout_ms(Some(1_200_000)), 1_200_000); + assert_eq!(effective_timeout_ms(Some(1_200_001)), 1_200_000); + assert_eq!(effective_timeout_ms(Some(u64::MAX)), 1_200_000); + } + #[tokio::test(flavor = "current_thread")] async fn basic_echo() { let dir = tempdir().expect("tempdir");