fix: terminalize interrupted tool executions
This commit is contained in:
@@ -1,5 +1,6 @@
|
||||
use std::collections::{HashMap, HashSet};
|
||||
use std::path::PathBuf;
|
||||
use std::sync::Arc;
|
||||
use std::sync::{Arc, Mutex};
|
||||
|
||||
use agen::tool::{Tool, ToolDefinition, ToolError, ToolMeta, ToolOutput};
|
||||
use async_trait::async_trait;
|
||||
@@ -20,15 +21,28 @@ struct BashParams {
|
||||
|
||||
pub(crate) struct BashTool {
|
||||
session: WorkdirSessionHandle,
|
||||
state: Arc<Mutex<BashExecutionState>>,
|
||||
}
|
||||
|
||||
#[derive(Default)]
|
||||
struct BashExecutionState {
|
||||
active: HashMap<String, CommandHandle>,
|
||||
cancellation_requested: HashSet<String>,
|
||||
}
|
||||
|
||||
struct CommandGuard {
|
||||
session: WorkdirSessionHandle,
|
||||
state: Arc<Mutex<BashExecutionState>>,
|
||||
call_id: String,
|
||||
handle: Option<CommandHandle>,
|
||||
}
|
||||
|
||||
impl Drop for CommandGuard {
|
||||
fn drop(&mut self) {
|
||||
let mut state = self.state.lock().unwrap();
|
||||
state.active.remove(&self.call_id);
|
||||
state.cancellation_requested.remove(&self.call_id);
|
||||
drop(state);
|
||||
if let Some(handle) = self.handle.take() {
|
||||
let workdir = self.session.clone();
|
||||
tokio::spawn(async move {
|
||||
@@ -52,20 +66,35 @@ impl Tool for BashTool {
|
||||
.unwrap_or(DEFAULT_TIMEOUT_SECS)
|
||||
.clamp(1, MAX_TIMEOUT_SECS);
|
||||
let cmd_summary = truncate_for_summary(¶ms.command);
|
||||
let call_id = ctx.call_id;
|
||||
let mut guard = CommandGuard {
|
||||
session: self.session.clone(),
|
||||
state: self.state.clone(),
|
||||
call_id: call_id.clone(),
|
||||
handle: None,
|
||||
};
|
||||
let handle = self
|
||||
.session
|
||||
.start_command(CommandRequest {
|
||||
command: params.command,
|
||||
timeout_secs,
|
||||
output_limit: INLINE_BYTE_BUDGET,
|
||||
tool_call_id: Some(ctx.call_id),
|
||||
tool_call_id: Some(call_id.clone()),
|
||||
})
|
||||
.await
|
||||
.map_err(crate::ToolsError::from)?;
|
||||
let mut guard = CommandGuard {
|
||||
session: self.session.clone(),
|
||||
handle: Some(handle.clone()),
|
||||
let cancel_after_start = {
|
||||
let mut state = self.state.lock().unwrap();
|
||||
state.active.insert(call_id.clone(), handle.clone());
|
||||
state.cancellation_requested.contains(&call_id)
|
||||
};
|
||||
guard.handle = Some(handle.clone());
|
||||
if cancel_after_start {
|
||||
self.session
|
||||
.cancel_command(handle.clone())
|
||||
.await
|
||||
.map_err(crate::ToolsError::from)?;
|
||||
}
|
||||
let output = self
|
||||
.session
|
||||
.command_output(CommandOutputRequest {
|
||||
@@ -76,9 +105,17 @@ impl Tool for BashTool {
|
||||
})
|
||||
.await
|
||||
.map_err(crate::ToolsError::from)?;
|
||||
let cancellation_requested = {
|
||||
let mut state = self.state.lock().unwrap();
|
||||
state.active.remove(&call_id);
|
||||
state.cancellation_requested.remove(&call_id)
|
||||
};
|
||||
guard.handle = None;
|
||||
|
||||
let summary = if output.timed_out {
|
||||
let timed_out = output.timed_out;
|
||||
let summary = if cancellation_requested {
|
||||
format!("$ {cmd_summary} (cancelled)")
|
||||
} else if output.timed_out {
|
||||
format!("$ {cmd_summary} (timed out after {timeout_secs}s)")
|
||||
} else {
|
||||
match output.exit_code {
|
||||
@@ -97,11 +134,33 @@ impl Tool for BashTool {
|
||||
} else {
|
||||
Some(output.content)
|
||||
};
|
||||
Ok(ToolOutput {
|
||||
let output = ToolOutput {
|
||||
summary,
|
||||
content,
|
||||
attachments: Vec::new(),
|
||||
})
|
||||
};
|
||||
if cancellation_requested {
|
||||
Err(ToolError::Cancelled(output))
|
||||
} else if timed_out {
|
||||
Err(ToolError::Interrupted(output))
|
||||
} else {
|
||||
Ok(output)
|
||||
}
|
||||
}
|
||||
|
||||
async fn cancel(&self, call_id: &str) -> Result<(), ToolError> {
|
||||
let handle = {
|
||||
let mut state = self.state.lock().unwrap();
|
||||
state.cancellation_requested.insert(call_id.to_string());
|
||||
state.active.get(call_id).cloned()
|
||||
};
|
||||
if let Some(handle) = handle {
|
||||
self.session
|
||||
.cancel_command(handle)
|
||||
.await
|
||||
.map_err(crate::ToolsError::from)?;
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
@@ -123,6 +182,7 @@ pub fn bash_tool(session: WorkdirSessionHandle, _output_dir: PathBuf) -> ToolDef
|
||||
.input_schema(serde_json::to_value(schema).expect("Bash schema serialization"));
|
||||
let tool: Arc<dyn Tool> = Arc::new(BashTool {
|
||||
session: session.clone(),
|
||||
state: Arc::new(Mutex::new(BashExecutionState::default())),
|
||||
});
|
||||
(meta, tool)
|
||||
})
|
||||
|
||||
@@ -7,7 +7,7 @@
|
||||
use std::path::Path;
|
||||
use std::sync::Arc;
|
||||
|
||||
use agen::tool::{Tool, ToolDefinition, ToolMeta};
|
||||
use agen::tool::{Tool, ToolDefinition, ToolError, ToolMeta};
|
||||
use manifest::{Permission, Scope, ScopeConfig, ScopeRule};
|
||||
use serde_json::json;
|
||||
use tempfile::TempDir;
|
||||
@@ -401,5 +401,45 @@ async fn bash_provider_output_does_not_expose_internal_paths() {
|
||||
assert_eq!(std::fs::read_dir(spill.path()).unwrap().count(), 0);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn bash_cancellation_returns_bounded_progress_as_terminal_output() {
|
||||
let (_dir, _spill, reg) = setup();
|
||||
let bash = reg.get("Bash");
|
||||
let executing = bash.clone();
|
||||
let execution = tokio::spawn(async move {
|
||||
executing
|
||||
.execute(
|
||||
r#"{"command":"printf 'before\\n'; printf 'err-before\\n' >&2; sleep 5; printf 'after\\n'"}"#,
|
||||
Default::default(),
|
||||
)
|
||||
.await
|
||||
});
|
||||
|
||||
tokio::time::sleep(std::time::Duration::from_millis(100)).await;
|
||||
bash.cancel("direct").await.expect("signal cancellation");
|
||||
let error = tokio::time::timeout(std::time::Duration::from_secs(2), execution)
|
||||
.await
|
||||
.expect("cancelled Bash should terminate inside the Engine grace budget")
|
||||
.expect("Bash task join");
|
||||
|
||||
let ToolError::Cancelled(output) = error.expect_err("cancelled command is non-success") else {
|
||||
panic!("expected typed cancellation result");
|
||||
};
|
||||
let content = output.content.expect("bounded progress output");
|
||||
assert!(
|
||||
content.contains("before"),
|
||||
"missing pre-cancel stdout: {content}"
|
||||
);
|
||||
assert!(
|
||||
content.contains("err-before"),
|
||||
"missing pre-cancel stderr: {content}"
|
||||
);
|
||||
assert!(
|
||||
!content.contains("after"),
|
||||
"post-cancel output leaked: {content}"
|
||||
);
|
||||
assert!(content.len() <= 16 * 1024, "output must remain bounded");
|
||||
}
|
||||
|
||||
// Sanity: unused Path import guard
|
||||
const _: fn() -> &'static Path = || Path::new("/");
|
||||
|
||||
Reference in New Issue
Block a user