Improve messages emitted for exec failures (#1659)
1. Emit call_id to exec approval elicitations for mcp client convenience 2. Remove the `-retry` from the call id for the same reason as above but upstream the reset behavior to the mcp client
This commit is contained in:
@@ -253,6 +253,7 @@ impl Session {
|
||||
pub async fn request_command_approval(
|
||||
&self,
|
||||
sub_id: String,
|
||||
call_id: String,
|
||||
command: Vec<String>,
|
||||
cwd: PathBuf,
|
||||
reason: Option<String>,
|
||||
@@ -261,6 +262,7 @@ impl Session {
|
||||
let event = Event {
|
||||
id: sub_id.clone(),
|
||||
msg: EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent {
|
||||
call_id,
|
||||
command,
|
||||
cwd,
|
||||
reason,
|
||||
@@ -1393,6 +1395,7 @@ async fn handle_container_exec_with_params(
|
||||
let rx_approve = sess
|
||||
.request_command_approval(
|
||||
sub_id.clone(),
|
||||
call_id.clone(),
|
||||
params.command.clone(),
|
||||
params.cwd.clone(),
|
||||
None,
|
||||
@@ -1520,6 +1523,7 @@ async fn handle_sandbox_error(
|
||||
let rx_approve = sess
|
||||
.request_command_approval(
|
||||
sub_id.clone(),
|
||||
call_id.clone(),
|
||||
params.command.clone(),
|
||||
params.cwd.clone(),
|
||||
Some("command failed; retry without sandbox?".to_string()),
|
||||
@@ -1537,9 +1541,7 @@ async fn handle_sandbox_error(
|
||||
sess.notify_background_event(&sub_id, "retrying command without sandbox")
|
||||
.await;
|
||||
|
||||
// Emit a fresh Begin event so progress bars reset.
|
||||
let retry_call_id = format!("{call_id}-retry");
|
||||
sess.notify_exec_command_begin(&sub_id, &retry_call_id, ¶ms)
|
||||
sess.notify_exec_command_begin(&sub_id, &call_id, ¶ms)
|
||||
.await;
|
||||
|
||||
// This is an escalated retry; the policy will not be
|
||||
@@ -1562,14 +1564,8 @@ async fn handle_sandbox_error(
|
||||
duration,
|
||||
} = retry_output;
|
||||
|
||||
sess.notify_exec_command_end(
|
||||
&sub_id,
|
||||
&retry_call_id,
|
||||
&stdout,
|
||||
&stderr,
|
||||
exit_code,
|
||||
)
|
||||
.await;
|
||||
sess.notify_exec_command_end(&sub_id, &call_id, &stdout, &stderr, exit_code)
|
||||
.await;
|
||||
|
||||
let is_success = exit_code == 0;
|
||||
let content = format_exec_output(
|
||||
|
||||
@@ -422,6 +422,8 @@ pub struct ExecCommandEndEvent {
|
||||
|
||||
#[derive(Debug, Clone, Deserialize, Serialize)]
|
||||
pub struct ExecApprovalRequestEvent {
|
||||
/// Identifier for the associated exec call, if available.
|
||||
pub call_id: String,
|
||||
/// The command to be executed.
|
||||
pub command: Vec<String>,
|
||||
/// The command's working directory.
|
||||
|
||||
@@ -156,6 +156,7 @@ async fn run_codex_tool_session_inner(
|
||||
EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent {
|
||||
command,
|
||||
cwd,
|
||||
call_id: _,
|
||||
reason: _,
|
||||
}) => {
|
||||
handle_exec_approval_request(
|
||||
|
||||
@@ -314,6 +314,7 @@ impl ChatWidget<'_> {
|
||||
self.bottom_pane.set_task_running(false);
|
||||
}
|
||||
EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent {
|
||||
call_id: _,
|
||||
command,
|
||||
cwd,
|
||||
reason,
|
||||
@@ -362,7 +363,7 @@ impl ChatWidget<'_> {
|
||||
cwd: _,
|
||||
}) => {
|
||||
self.conversation_history
|
||||
.add_active_exec_command(call_id, command);
|
||||
.reset_or_add_active_exec_command(call_id, command);
|
||||
self.request_redraw();
|
||||
}
|
||||
EventMsg::PatchApplyBegin(PatchApplyBeginEvent {
|
||||
|
||||
@@ -235,6 +235,30 @@ impl ConversationHistoryWidget {
|
||||
self.add_to_history(HistoryCell::new_active_exec_command(call_id, command));
|
||||
}
|
||||
|
||||
/// If an ActiveExecCommand with the same call_id already exists, replace
|
||||
/// it with a fresh one (resetting start time and view). Otherwise, add a new entry.
|
||||
pub fn reset_or_add_active_exec_command(&mut self, call_id: String, command: Vec<String>) {
|
||||
// Find the most recent matching ActiveExecCommand.
|
||||
let maybe_idx = self.entries.iter().rposition(|entry| {
|
||||
if let HistoryCell::ActiveExecCommand { call_id: id, .. } = &entry.cell {
|
||||
id == &call_id
|
||||
} else {
|
||||
false
|
||||
}
|
||||
});
|
||||
|
||||
if let Some(idx) = maybe_idx {
|
||||
let width = self.cached_width.get();
|
||||
self.entries[idx].cell = HistoryCell::new_active_exec_command(call_id.clone(), command);
|
||||
if width > 0 {
|
||||
let height = self.entries[idx].cell.height(width);
|
||||
self.entries[idx].line_count.set(height);
|
||||
}
|
||||
} else {
|
||||
self.add_active_exec_command(call_id, command);
|
||||
}
|
||||
}
|
||||
|
||||
pub fn add_active_mcp_tool_call(
|
||||
&mut self,
|
||||
call_id: String,
|
||||
|
||||
Reference in New Issue
Block a user