diff --git a/hive-bash-mcp/src/runner.rs b/hive-bash-mcp/src/runner.rs index 21527da5..0ddb8874 100644 --- a/hive-bash-mcp/src/runner.rs +++ b/hive-bash-mcp/src/runner.rs @@ -757,16 +757,29 @@ fn done_summary(id: &str, summary: &str, output: Option<(bool, bool)>) -> String if let Some((has_stdout, has_stderr)) = output && (has_stdout || has_stderr) { - body.push_str("\n\noutput captured — read the full text with:"); - if has_stdout { - let _ = write!(body, "\n Read({})", crate::paths::task_out(id).display()); - } + // Header + pointer order are keyed on `has_stderr`, not on the exit + // code: a failed command mid-chain (`cd bad-path && rm ...`) can + // exit 0 while the real evidence sits in `.err` — an exit-code + // trigger would filter out precisely the cases where nothing looks + // wrong. `.err`'s presence is already the scarce, meaningful signal + // the pointer is built on; keying the flag on the same condition + // costs nothing on the common quiet-success path (no stderr, no + // pointer, exactly like before) and fires on every case where + // something was written to stderr, including the ones the exit + // code can't be trusted to reveal. if has_stderr { + body.push_str("\n\n⚠️ stderr present — read the full output:"); let _ = write!( body, "\n Read({}) # stderr", crate::paths::task_err(id).display() ); + if has_stdout { + let _ = write!(body, "\n Read({})", crate::paths::task_out(id).display()); + } + } else { + body.push_str("\n\noutput captured — read the full text with:"); + let _ = write!(body, "\n Read({})", crate::paths::task_out(id).display()); } } body @@ -837,4 +850,58 @@ mod tests { assert!(take_wake_suppressed(id)); assert!(!take_wake_suppressed(id)); } + + // done_summary: the stderr-present branch must trigger on has_stderr + // alone, never on the exit code — see the comment on the function. A + // completed task with exit=0 and stderr present (a swallowed failure + // mid-chain) still needs to read as a flag, not as routine bookkeeping. + + #[test] + fn done_summary_no_output_is_bare() { + use super::done_summary; + assert_eq!( + done_summary("t1", "exit=0", None), + "bash task `t1` finished: exit=0" + ); + assert_eq!( + done_summary("t1", "exit=0", Some((false, false))), + "bash task `t1` finished: exit=0", + "no captured output on either stream ⇒ no pointer block at all" + ); + } + + #[test] + fn done_summary_stdout_only_is_unflagged() { + use super::done_summary; + let body = done_summary("t1", "exit=0", Some((true, false))); + assert!(body.contains("output captured — read the full text with:")); + assert!(body.contains("Read(")); + assert!(body.contains(".out")); + assert!(!body.contains(".err")); + assert!(!body.contains('\u{26a0}'), "no flag without stderr"); + } + + #[test] + fn done_summary_stderr_present_is_flagged_regardless_of_summary_text() { + use super::done_summary; + // exit=0 in the summary — a swallowed mid-chain failure that + // still exits clean. The flag must fire anyway, keyed on + // has_stderr, not on the summary text. + let body = done_summary("t1", "exit=0", Some((true, true))); + assert!( + body.contains("⚠️ stderr present"), + "stderr present must flag even on a clean exit code" + ); + let err_pos = body.find(".err").expect("err pointer present"); + let out_pos = body.find(".out").expect("out pointer present"); + assert!(err_pos < out_pos, ".err must be listed before .out"); + } + + #[test] + fn done_summary_stderr_only_omits_out_pointer() { + use super::done_summary; + let body = done_summary("t1", "exit=1", Some((false, true))); + assert!(body.contains(".err")); + assert!(!body.contains(".out"), "no stdout ⇒ no stdout pointer"); + } }