From 63628cb012c031a5d03448c0fde5a0bdd12aaf1d Mon Sep 17 00:00:00 2001 From: calvinsturm Date: Tue, 30 Jun 2026 17:47:07 -0700 Subject: [PATCH] Add default shell timeout policy Previously an omitted timeout_ms meant unbounded shell execution, so an autonomous/unattended run could hang forever on a runaway command. Introduce a target-aware default in resolve_shell_timeout_ms: - explicit timeout_ms > 0: use it (host enforces; docker still rejects per #15) - explicit timeout_ms == 0: unbounded opt-out (unchanged) - missing timeout_ms on host: apply DEFAULT_SHELL_TIMEOUT_MS (120000 ms) - missing timeout_ms on docker: resolve to 0 (unbounded), NOT a rejection The docker branch is deliberate: DockerTarget cannot enforce timeouts and rejects timeout_ms > 0, so a *missing* timeout must stay unbounded there rather than silently becoming a rejection. Explicit docker requests are unchanged. The default lives as a documented module constant in tools/exec_shell.rs (easy to adjust; no reliability-profile/RunArgs/golden churn) and is reflected in the shell tool description. Tests: resolver covers missing->host-default, explicit>0 override, explicit 0 unbounded, and missing-on-docker stays unbounded; a behavioral execute_tool test confirms a fast host command with no timeout still succeeds under the default. --- src/tools/catalog.rs | 2 +- src/tools/exec_shell.rs | 73 +++++++++++++++++++++++++++++++++++++++-- src/tools/tests.rs | 46 ++++++++++++++++++++++++++ 3 files changed, 117 insertions(+), 4 deletions(-) diff --git a/src/tools/catalog.rs b/src/tools/catalog.rs index d69f695..94577c6 100644 --- a/src/tools/catalog.rs +++ b/src/tools/catalog.rs @@ -68,7 +68,7 @@ pub fn builtin_tools_enabled(enable_write_tools: bool, enable_shell_tool: bool) if enable_shell_tool { tools.push(ToolDef { name: "shell".to_string(), - description: "Run a shell command with optional args, cwd, and timeout_ms. timeout_ms bounds the command's wall-clock runtime in milliseconds; omit or use 0 for no timeout.".to_string(), + description: "Run a shell command with optional args, cwd, and timeout_ms. timeout_ms bounds the command's wall-clock runtime in milliseconds. If omitted, a safe default timeout (120000 ms) is applied on the host target; pass 0 to opt out and run unbounded.".to_string(), parameters: json!({ "type":"object", "properties":{ diff --git a/src/tools/exec_shell.rs b/src/tools/exec_shell.rs index 0477095..6dc1a54 100644 --- a/src/tools/exec_shell.rs +++ b/src/tools/exec_shell.rs @@ -6,6 +6,31 @@ use crate::types::SideEffects; use super::exec_support::{failed_exec, ToolExecution}; use super::{minimal_builtin_example, ToolErrorCode, ToolErrorDetail, ToolRuntime}; +/// Default wall-clock timeout (ms) applied to a shell command on the host target +/// when the caller omits `timeout_ms`. This keeps unattended/autonomous runs +/// from hanging forever on a runaway command. Adjust here to change the policy +/// globally; callers can always override per-call, and `timeout_ms: 0` opts out +/// entirely (unbounded). +pub(super) const DEFAULT_SHELL_TIMEOUT_MS: u64 = 120_000; + +/// Resolve the effective shell timeout from the request args and execution +/// target. +/// +/// - Explicit `timeout_ms` (including `0` = unbounded opt-out): honored as-is. +/// - Missing `timeout_ms`: the host target applies [`DEFAULT_SHELL_TIMEOUT_MS`]. +/// The docker target instead resolves to `0` (unbounded) because it cannot +/// enforce timeouts (`DockerTarget::exec_shell` rejects `timeout_ms > 0`), so a +/// *missing* timeout must not silently turn into a rejection. +pub(super) fn resolve_shell_timeout_ms(args: &Value, target: ExecTargetKind) -> u64 { + match args.get("timeout_ms").and_then(|v| v.as_u64()) { + Some(explicit) => explicit, + None => match target { + ExecTargetKind::Host => DEFAULT_SHELL_TIMEOUT_MS, + ExecTargetKind::Docker => 0, + }, + } +} + pub(super) async fn run_shell(rt: &ToolRuntime, args: &Value) -> ToolExecution { let shell_allowed = rt.allow_shell || (rt.allow_shell_in_workdir_only && shell_cwd_is_workdir_scoped(args)); @@ -39,9 +64,11 @@ pub(super) async fn run_shell(rt: &ToolRuntime, args: &Value) -> ToolExecution { .get("cwd") .and_then(|v| v.as_str()) .map(ToString::to_string); - // `timeout_ms` is optional; absent or 0 preserves unbounded behavior. The - // `as_u64` parse makes negative values unrepresentable. - let timeout_ms = args.get("timeout_ms").and_then(|v| v.as_u64()).unwrap_or(0); + // `timeout_ms` is optional. When omitted, the host target applies a safe + // default so unattended runs cannot hang forever; an explicit `0` opts out + // (unbounded). The `as_u64` parse makes negative values unrepresentable. + // Target-aware so a missing timeout never turns into a docker rejection. + let timeout_ms = resolve_shell_timeout_ms(args, rt.exec_target_kind); let req = ShellReq { workdir: rt.workdir.clone(), cmd: cmd.to_string(), @@ -322,3 +349,43 @@ fn shell_cwd_is_workdir_scoped(args: &Value) -> bool { ) }) } + +#[cfg(test)] +mod timeout_policy_tests { + use super::{resolve_shell_timeout_ms, DEFAULT_SHELL_TIMEOUT_MS}; + use crate::target::ExecTargetKind; + use serde_json::json; + + #[test] + fn missing_timeout_uses_host_default() { + let args = json!({ "cmd": "echo", "args": ["hi"] }); + assert_eq!( + resolve_shell_timeout_ms(&args, ExecTargetKind::Host), + DEFAULT_SHELL_TIMEOUT_MS + ); + assert_eq!(DEFAULT_SHELL_TIMEOUT_MS, 120_000); + } + + #[test] + fn explicit_positive_timeout_overrides_default() { + let args = json!({ "cmd": "echo", "timeout_ms": 500 }); + assert_eq!(resolve_shell_timeout_ms(&args, ExecTargetKind::Host), 500); + // Explicit values are honored on docker too (DockerTarget rejects > 0). + assert_eq!(resolve_shell_timeout_ms(&args, ExecTargetKind::Docker), 500); + } + + #[test] + fn explicit_zero_timeout_stays_unbounded() { + let args = json!({ "cmd": "echo", "timeout_ms": 0 }); + assert_eq!(resolve_shell_timeout_ms(&args, ExecTargetKind::Host), 0); + assert_eq!(resolve_shell_timeout_ms(&args, ExecTargetKind::Docker), 0); + } + + #[test] + fn missing_timeout_on_docker_stays_unbounded_not_rejected() { + // Critical: a missing timeout must not become a docker timeout + // rejection. Docker resolves missing -> 0 (unbounded), unlike host. + let args = json!({ "cmd": "echo" }); + assert_eq!(resolve_shell_timeout_ms(&args, ExecTargetKind::Docker), 0); + } +} diff --git a/src/tools/tests.rs b/src/tools/tests.rs index b94a01c..eb2fe9e 100644 --- a/src/tools/tests.rs +++ b/src/tools/tests.rs @@ -852,6 +852,52 @@ async fn shell_auto_repair_uses_sh_lc_for_embedded_command() { ); } +#[tokio::test] +async fn shell_without_timeout_still_runs_fast_command_under_default_policy() { + // A host shell call that omits timeout_ms now gets the 120s default. A fast + // command must still complete normally (regression guard: the default must + // not break ordinary usage), proving missing -> default is wired into the + // execute_tool path without turning fast commands into timeouts. + let tmp = tempdir().expect("tempdir"); + let rt = ToolRuntime { + workdir: tmp.path().to_path_buf(), + allow_shell: true, + allow_shell_in_workdir_only: false, + allow_write: false, + max_tool_output_bytes: 200_000, + max_read_bytes: 200_000, + unsafe_bypass_allow_flags: false, + tool_args_strict: ToolArgsStrict::On, + exec_target_kind: ExecTargetKind::Host, + exec_target: std::sync::Arc::new(HostTarget), + }; + let arguments = if cfg!(windows) { + json!({"cmd":"cmd","args":["/C","echo default-policy-ok"]}) + } else { + json!({"cmd":"sh","args":["-c","echo default-policy-ok"]}) + }; + let tc = ToolCall { + id: "tc_shell_default_timeout".to_string(), + name: "shell".to_string(), + arguments, + }; + let msg = execute_tool(&rt, &tc).await; + let envelope: Value = serde_json::from_str(&msg.content.expect("content")).expect("json"); + assert_eq!(envelope.get("ok").and_then(|v| v.as_bool()), Some(true)); + let inner = envelope + .get("content") + .and_then(|v| v.as_str()) + .and_then(|s| serde_json::from_str::(s).ok()) + .expect("inner shell json"); + let stdout = inner + .get("stdout") + .and_then(|v| v.as_str()) + .unwrap_or_default(); + assert!(stdout.contains("default-policy-ok")); + // Fast command must not be reported as timed out. + assert!(inner.get("timed_out").is_none()); +} + #[tokio::test] async fn read_file_rejects_path_traversal() { let tmp = tempdir().expect("tempdir");