Add default shell timeout policy - #17
Merged
Merged
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a safe default shell timeout policy so host shell commands no longer run forever when
timeout_msis omitted.Host shell commands now default to a 120 second timeout unless the caller explicitly passes
timeout_ms: 0to opt out.What changed
Added
DEFAULT_SHELL_TIMEOUT_MS = 120_000Added shell timeout resolution logic in
exec_shellUpdated shell tool documentation to mention the default
Preserved explicit
timeout_ms > 0override behaviorPreserved explicit
timeout_ms: 0as the unbounded/back-compatible opt-outPreserved DockerTarget behavior:
timeout_ms > 0still returns a structured unsupported-target errortimeout_msmaps to unbounded on Docker and is not accidentally rejectedTimeout semantics
timeout_ms > 0timeout_ms == 0timeout_msmissingWhy
After PR #15, LocalAgent supported explicit shell timeouts, but commands could still run forever when the model omitted
timeout_ms.This closes that gap for host execution while preserving a deliberate opt-out path.
Validation
cargo fmt --checkcargo clippy --all-targets -- -D warningscargo test --lib target::cargo test --lib tools::cargo test --test tool_call_accuracy_cicargo test --lib reprocargo test --test artifact_goldenpython scripts/ci_release_readiness.pyAll validation passed locally.
Notes
The default is currently a documented compile-time constant, not a CLI/profile setting. That keeps this PR small and avoids profile/golden churn. A future PR can wire this into reliability profiles or a
--shell-timeout-msflag.Follow-ups