From 9c21caa17262d51464f97f59d9e46e485b37966e Mon Sep 17 00:00:00 2001 From: andig Date: Fri, 24 Jul 2026 13:21:37 +0200 Subject: [PATCH] fix(ctx_shell): stop reporting a requested background cancel as a tool error Cancelling a background job answered with the same '[background: running]' string a status poll returns, so the cancel looked like a no-op and the natural next move was to cancel again. That second call then surfaced the process's own SIGINT exit as the tool's exit code, tripping the client's failure hook and instructing the agent to fix something it had deliberately done. A cancel now acknowledges itself distinctly, reports the terminal state as data ('[cancelled: , exit 130]') with exit 0, and is idempotent: cancelling an already-cancelled, already-finished or already-pruned job is equally benign. The cancelled marker in the captured output loses its 'ERROR:' prefix for the same reason - a caller-requested stop is not a failure. Fixes #1246 Co-Authored-By: Claude Opus 4.8 (1M context) --- rust/src/server/background_shell.rs | 2 +- rust/src/server/execute.rs | 3 +- rust/src/tools/registered/ctx_shell.rs | 154 ++++++++++++++++++------- 3 files changed, 117 insertions(+), 42 deletions(-) diff --git a/rust/src/server/background_shell.rs b/rust/src/server/background_shell.rs index 99305c94bb..2aecf10604 100644 --- a/rust/src/server/background_shell.rs +++ b/rust/src/server/background_shell.rs @@ -475,7 +475,7 @@ mod tests { assert!(matches!(cancel(&id), Some(JobState::Running { .. }))); for _ in 0..40 { if let Some(JobState::Cancelled { output }) = status(&id) { - assert!(output.contains("command cancelled")); + assert!(output.contains("[cancelled: command stopped on request]")); return; } std::thread::sleep(Duration::from_millis(25)); diff --git a/rust/src/server/execute.rs b/rust/src/server/execute.rs index c484c0e988..ad2a57a73e 100644 --- a/rust/src/server/execute.rs +++ b/rust/src/server/execute.rs @@ -266,7 +266,8 @@ pub(crate) fn execute_command_with_env_cancellable( if !text.ends_with('\n') && !text.is_empty() { text.push('\n'); } - text.push_str("ERROR: command cancelled"); + // #1246: a cancel is always caller-requested, so it is not an error. + text.push_str("[cancelled: command stopped on request]"); } (text, code) diff --git a/rust/src/tools/registered/ctx_shell.rs b/rust/src/tools/registered/ctx_shell.rs index 4936cfd019..86a8c92535 100644 --- a/rust/src/tools/registered/ctx_shell.rs +++ b/rust/src/tools/registered/ctx_shell.rs @@ -53,6 +53,7 @@ impl McpTool for CtxShellTool { let id = get_str(args, "job_id").ok_or_else(|| { ErrorData::invalid_params("job_id is required with background_action", None) })?; + let is_cancel = action == "cancel"; let state = match action.as_str() { "status" => crate::server::background_shell::status(&id), "cancel" => crate::server::background_shell::cancel(&id), @@ -63,45 +64,7 @@ impl McpTool for CtxShellTool { )); } }; - let Some(state) = state else { - return Ok(ToolOutput { - shell_outcome: Some(ShellOutcome::Exit(1)), - content_blocks: None, - ..ToolOutput::simple(format!("[background:{id} not found]")) - }); - }; - let (text, exit_code) = match state { - crate::server::background_shell::JobState::Running { output } => { - // #1217: show the captured-so-far output so a poll of a - // long-running job reflects progress instead of a bare - // "running" with no signal of whether it is advancing. - let body = redact_shell_output_secrets(&output); - if body.trim().is_empty() { - (format!("[background:{id} running]"), 0) - } else { - (format!("[background:{id} running]\n{body}"), 0) - } - } - crate::server::background_shell::JobState::Completed { output, exit_code } => ( - format!( - "[background:{id} completed]\n{}{}", - redact_shell_output_secrets(&output), - if exit_code == 0 { - String::new() - } else { - format!("\n[exit:{exit_code}]") - } - ), - exit_code, - ), - crate::server::background_shell::JobState::Cancelled { output } => ( - format!( - "[background:{id} cancelled]\n{}\n[exit:130]", - redact_shell_output_secrets(&output) - ), - 130, - ), - }; + let (text, exit_code) = format_background_state(&id, is_cancel, state); return Ok(ToolOutput { shell_outcome: Some(ShellOutcome::Exit(exit_code)), content_blocks: None, @@ -638,6 +601,72 @@ fn warn_shell_secret_paths(command: &str) { } } +/// Render a `background_action` result. +/// +/// #1246: a caller-requested cancel is a success, not a tool failure. The +/// process's own SIGINT exit (130) used to be reported as the tool's exit code, +/// which tripped the client's failure hook and told the agent to fix something +/// it had deliberately done. A cancel therefore never reports a non-zero exit, +/// and is idempotent: cancelling an already-cancelled, already-finished or +/// already-pruned job is equally benign. The first cancel also gets its own +/// wording so it cannot be mistaken for a status poll that did nothing. +fn format_background_state( + id: &str, + is_cancel: bool, + state: Option, +) -> (String, i32) { + use crate::server::background_shell::JobState; + let Some(state) = state else { + return if is_cancel { + ( + format!("[background:{id} not found — already finished or cancelled]"), + 0, + ) + } else { + (format!("[background:{id} not found]"), 1) + }; + }; + match state { + JobState::Running { output } => { + // #1217: show the captured-so-far output so a poll of a + // long-running job reflects progress instead of a bare + // "running" with no signal of whether it is advancing. + let body = redact_shell_output_secrets(&output); + let head = if is_cancel { + format!( + "[background:{id} cancel requested — job is stopping; poll status for the final output]" + ) + } else { + format!("[background:{id} running]") + }; + if body.trim().is_empty() { + (head, 0) + } else { + (format!("{head}\n{body}"), 0) + } + } + JobState::Completed { output, exit_code } => ( + format!( + "[background:{id} completed]\n{}{}", + redact_shell_output_secrets(&output), + if exit_code == 0 { + String::new() + } else { + format!("\n[exit:{exit_code}]") + } + ), + if is_cancel { 0 } else { exit_code }, + ), + JobState::Cancelled { output } => ( + format!( + "[background:{id} cancelled]\n{}\n[cancelled: {id}, exit 130]", + redact_shell_output_secrets(&output) + ), + 0, + ), + } +} + fn redact_shell_output_secrets(output: &str) -> String { let cfg = crate::core::config::Config::load(); if !cfg.secret_detection.enabled { @@ -708,7 +737,52 @@ fn detect_bare_cat_file(command: &str) -> Option { #[cfg(test)] mod tests { - use super::{is_timeout_notice_only, should_auto_background}; + use super::{format_background_state, is_timeout_notice_only, should_auto_background}; + use crate::server::background_shell::JobState; + + /// #1246: a cancel must never come back as a tool error, and must not read + /// like a status poll that did nothing. + #[test] + fn cancel_is_acknowledged_and_never_reports_a_failure() { + let running = JobState::Running { + output: String::new(), + }; + let (text, exit) = format_background_state("shell_x", true, Some(running.clone())); + assert_eq!(exit, 0); + assert!(text.contains("cancel requested"), "{text}"); + + // A status poll of the same state keeps the old wording. + let (text, exit) = format_background_state("shell_x", false, Some(running)); + assert_eq!(exit, 0); + assert!(text.contains("[background:shell_x running]"), "{text}"); + + // The terminal state is data, not an error — no exit 130 leaks out. + let (text, exit) = format_background_state( + "shell_x", + true, + Some(JobState::Cancelled { + output: "[cancelled: command stopped on request]".to_string(), + }), + ); + assert_eq!(exit, 0); + assert!(text.contains("[cancelled: shell_x, exit 130]"), "{text}"); + + // Idempotent: already finished, or finished and pruned. + let finished = JobState::Completed { + output: "boom".to_string(), + exit_code: 1, + }; + assert_eq!( + format_background_state("shell_x", true, Some(finished.clone())).1, + 0 + ); + assert_eq!( + format_background_state("shell_x", false, Some(finished)).1, + 1 + ); + assert_eq!(format_background_state("shell_x", true, None).1, 0); + assert_eq!(format_background_state("shell_x", false, None).1, 1); + } #[test] fn long_cargo_test_is_auto_backgrounded() {