Add shell command timeout and cleanup - #15
Merged
Conversation
Replace the unbounded `command.output().await` host shell execution with a managed child: piped stdout/stderr drained into shared buffers, `kill_on_drop` enabled, and an optional per-call `timeout_ms`. - ShellReq gains `timeout_ms` (u64; 0 = unbounded, preserving prior behavior). Exposed as an optional `timeout_ms` arg on the `shell` tool (catalog + schema + strict validation; integer >= 0 so negatives are unrepresentable). - On timeout the direct child is killed and reaped, drain tasks get a short grace then abort (so a surviving grandchild holding the pipe can never block return), and a structured result is returned: ok=false, timed_out=true, timeout_ms, partial stdout/stderr, and an actionable hint. - New ShellExecTimeout tool error code; classify_shell_target_error detects the timeout envelope before other shell error classification. - kill_on_drop(true) means a cancelled turn no longer silently orphans the direct child. Limitation: termination is best-effort on the direct child only; commands run through a shell wrapper (sh -c / cmd /C) may leave grandchildren until they exit. Robust process-tree cleanup (unix process groups / Windows job objects) and docker-target timeout enforcement are follow-ups. Tests: host command under timeout succeeds; exceeding timeout returns promptly (<2s for a 200ms budget) with timed_out + hint; timeout_ms=0 stays unbounded.
Make the shell auto-repair test deterministic and give the docker target a clear answer for timeout requests. - tools/tests: the windows builtin auto-repair test used `echo`, which is frequently shadowed by an MSYS/Git `echo.exe` on PATH; the direct spawn then succeeds and the repair path is never exercised (the test failed on this machine and on base main). Switch to `ver`, a cmd builtin with no standalone executable counterpart, so the initial spawn deterministically fails and forces windows_cmd_c repair regardless of host PATH. Assertion still proves repair_attempted + windows_cmd_c strategy and real command output. - target: DockerTarget::exec_shell now rejects timeout_ms > 0 with a structured, classifiable error (error=timeout_unsupported) instead of silently ignoring a requested bound. Returns before any docker invocation, so the new test is deterministic without a docker daemon. Shared run_container read/write/list/ patch behavior is untouched. - New ShellExecTimeoutUnsupported tool error code; classify_shell_target_error maps the docker rejection envelope to it with an actionable message.
CalvinSturm
added a commit
that referenced
this pull request
Jul 1, 2026
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 bounded shell command execution for LocalAgent so shell tool calls can fail safely instead of hanging indefinitely.
This PR adds optional
timeout_mssupport to shell execution, returns a structured timeout failure with partial output, and uses managed child execution with best-effort direct-child cleanup.What changed
Added
ShellReq.timeout_msAdvertised optional
timeout_msin the shell tool schemaReplaced
command.output().awaitwith managed child executionAdded timeout handling via
tokio::time::timeoutAdded direct-child cleanup via
kill_on_drop/start_killAdded structured timeout responses with:
timed_out: truetimeout_msAdded
ToolErrorCode::ShellExecTimeoutAdded
ToolErrorCode::ShellExecTimeoutUnsupportedRejects
timeout_ms > 0on DockerTarget with a structured unsupported-target error instead of silently ignoring itStabilized the Windows shell auto-repair test by using
verinstead ofechoWhy
Previously,
HostTarget::exec_shellusedcommand.output().awaitwith no timeout or managed lifecycle. A hanging command could block the agent indefinitely, and cancelling a TUI turn could orphan the child process.This PR fixes the highest-priority harness reliability issue from the opencode capability audit.
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 required validation passed.
Known limitations
sh -c,cmd /C, or similar wrappers may continue until they exit.timeout_ms > 0clearly instead of ignoring it.Follow-ups