apply the default extension timeout to developer shell tool calls also (#10348)
Signed-off-by: Evanfeenstra <evanfeenstra@gmail.com>
This commit is contained in:
@@ -171,6 +171,8 @@ struct TruncationInfo {
|
||||
#[derive(Debug, Deserialize, JsonSchema)]
|
||||
pub struct ShellParams {
|
||||
pub command: String,
|
||||
/// Maximum time in seconds to allow the command to run before it is killed.
|
||||
/// If omitted, defaults to DEFAULT_EXTENSION_TIMEOUT.
|
||||
#[serde(default)]
|
||||
pub timeout_secs: Option<u64>,
|
||||
}
|
||||
@@ -437,14 +439,10 @@ impl ShellTool {
|
||||
.collect();
|
||||
|
||||
let is_error = if execution.timed_out {
|
||||
if let Some(timeout_secs) = params.timeout_secs {
|
||||
rendered.push_str(&format!(
|
||||
"\n\nCommand timed out after {} seconds",
|
||||
timeout_secs
|
||||
));
|
||||
} else {
|
||||
rendered.push_str("\n\nCommand timed out");
|
||||
}
|
||||
rendered.push_str(&format!(
|
||||
"\n\nCommand timed out after {} seconds",
|
||||
resolve_shell_timeout(params.timeout_secs)
|
||||
));
|
||||
true
|
||||
} else {
|
||||
execution.exit_code.unwrap_or(1) != 0
|
||||
@@ -509,6 +507,14 @@ struct ExecutionOutput {
|
||||
output_collection_error: Option<String>,
|
||||
}
|
||||
|
||||
fn resolve_shell_timeout(timeout_secs: Option<u64>) -> u64 {
|
||||
timeout_secs.unwrap_or_else(|| {
|
||||
crate::config::Config::global()
|
||||
.get_goose_default_extension_timeout()
|
||||
.unwrap_or(crate::config::DEFAULT_EXTENSION_TIMEOUT)
|
||||
})
|
||||
}
|
||||
|
||||
async fn run_command(
|
||||
command_line: &str,
|
||||
timeout_secs: Option<u64>,
|
||||
@@ -516,6 +522,8 @@ async fn run_command(
|
||||
login_path: Option<&str>,
|
||||
cancellation_token: CancellationToken,
|
||||
) -> Result<ExecutionOutput, String> {
|
||||
let timeout_secs = Some(resolve_shell_timeout(timeout_secs));
|
||||
|
||||
let mut command = build_shell_command(command_line, working_dir, login_path);
|
||||
|
||||
command.stdout(Stdio::piped());
|
||||
@@ -1104,4 +1112,42 @@ mod tests {
|
||||
"should capture output after background cmd"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn resolve_shell_timeout_prefers_explicit_value() {
|
||||
assert_eq!(resolve_shell_timeout(Some(42)), 42);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn resolve_shell_timeout_falls_back_to_a_bound_when_absent() {
|
||||
// The key behavioral guarantee: an omitted timeout no longer means
|
||||
// "run forever" — it resolves to the default extension timeout.
|
||||
assert!(resolve_shell_timeout(None) > 0);
|
||||
}
|
||||
|
||||
#[cfg(not(windows))]
|
||||
#[tokio::test]
|
||||
async fn shell_kills_hanging_command_after_explicit_timeout() {
|
||||
let tool = ShellTool::new_for_test().unwrap();
|
||||
let start = std::time::Instant::now();
|
||||
let result = tool
|
||||
.shell(ShellParams {
|
||||
command: "sleep 30".to_string(),
|
||||
timeout_secs: Some(1),
|
||||
})
|
||||
.await;
|
||||
|
||||
assert!(
|
||||
start.elapsed().as_secs() < 10,
|
||||
"shell should return shortly after the timeout, not wait for the command"
|
||||
);
|
||||
assert_eq!(result.is_error, Some(true));
|
||||
let shell_output = extract_shell_output(&result);
|
||||
assert!(shell_output.timed_out, "command should be marked timed_out");
|
||||
assert!(
|
||||
shell_output.exit_code.is_none(),
|
||||
"killed process should have no exit code"
|
||||
);
|
||||
assert!(extract_text(&result).contains("Command timed out after 1 seconds"));
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user