Skip to content

fix: Don't break from consuming subprocess output in shell tool until both streams are done #2771

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jamadeo merged 9 commits into from
Jun 5, 2025
Merged

fix: Don't break from consuming subprocess output in shell tool until both streams are done #2771

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f09e001
Attempt to fix shell tool for Windows/WSL
jamadeo Jun 3, 2025
e1ea289
restore prior error behavior
jamadeo Jun 3, 2025
6938895
Back to an if statement
jamadeo Jun 3, 2025
485c00e
Merge branch 'main' into jackamadeo/fix-tool-truncate
jamadeo Jun 4, 2025
0933598
Also properly hide spinners
jamadeo Jun 4, 2025
61992cb
Merge remote-tracking branch 'refs/remotes/origin/jackamadeo/fix-tool…
jamadeo Jun 4, 2025
499be6a
Merge branch 'main' into jackamadeo/fix-tool-truncate
Kvadratni Jun 4, 2025
8a436f8
Use if not map
jamadeo Jun 5, 2025
d032cb8
Merge branch 'main' into jackamadeo/fix-tool-truncate
jamadeo Jun 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions crates/goose-cli/src/session/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -727,7 +727,6 @@ impl Session {
loop {
tokio::select! {
result = stream.next() => {
let _ = progress_bars.hide();
match result {
Some(Ok(AgentEvent::Message(message))) => {
// If it's a confirmation request, get approval but otherwise do not render/persist
Expand Down Expand Up @@ -865,6 +864,7 @@ impl Session {
session::persist_messages(&self.session_file, &self.messages, None).await?;

if interactive {output::hide_thinking()};
let _ = progress_bars.hide();
output::render_message(&message, self.debug);
if interactive {output::show_thinking()};
}
Expand All @@ -891,7 +891,6 @@ impl Session {
v.to_string()
},
};
// output::render_text_no_newlines(&message, None, true);
progress_bars.log(&message);
},
"notifications/progress" => {
Expand Down
6 changes: 6 additions & 0 deletions crates/goose-cli/src/session/output.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,6 +660,12 @@ impl McpSpinners {
}

pub fn hide(&mut self) -> Result<(), Error> {
self.bars.iter_mut().for_each(|(_, bar)| {
bar.disable_steady_tick();
});
if let Some(spinner) = self.log_spinner.as_mut() {
spinner.disable_steady_tick();
}
self.multi_bar.clear()
}
}
Expand Down
86 changes: 48 additions & 38 deletions crates/goose-mcp/src/developer/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,52 +531,62 @@ impl DeveloperRouter {
let mut stdout_buf = Vec::new();
let mut stderr_buf = Vec::new();

let mut stdout_done = false;
let mut stderr_done = false;

loop {
tokio::select! {
n = stdout_reader.read_until(b'\n', &mut stdout_buf) => {
n = stdout_reader.read_until(b'\n', &mut stdout_buf), if !stdout_done => {
if n? == 0 {
break;
stdout_done = true;
} else {
let line = String::from_utf8_lossy(&stdout_buf);

notifier.try_send(JsonRpcMessage::Notification(JsonRpcNotification {
jsonrpc: "2.0".to_string(),
method: "notifications/message".to_string(),
params: Some(json!({
"data": {
"type": "shell",
"stream": "stdout",
"output": line.to_string(),
}
})),
})).ok();

combined_output.push_str(&line);
stdout_buf.clear();
}
let line = String::from_utf8_lossy(&stdout_buf);

notifier.try_send(JsonRpcMessage::Notification(JsonRpcNotification {
jsonrpc: "2.0".to_string(),
method: "notifications/message".to_string(),
params: Some(json!({
"data": {
"type": "shell",
"stream": "stdout",
"output": line.to_string(),
}
})),
}))
.ok();

combined_output.push_str(&line);
stdout_buf.clear();
}
n = stderr_reader.read_until(b'\n', &mut stderr_buf) => {

n = stderr_reader.read_until(b'\n', &mut stderr_buf), if !stderr_done => {
if n? == 0 {
break;
stderr_done = true;
} else {
let line = String::from_utf8_lossy(&stderr_buf);

notifier.try_send(JsonRpcMessage::Notification(JsonRpcNotification {
jsonrpc: "2.0".to_string(),
method: "notifications/message".to_string(),
params: Some(json!({
"data": {
"type": "shell",
"stream": "stderr",
"output": line.to_string(),
}
})),
})).ok();

combined_output.push_str(&line);
stderr_buf.clear();
}
let line = String::from_utf8_lossy(&stderr_buf);

notifier.try_send(JsonRpcMessage::Notification(JsonRpcNotification {
jsonrpc: "2.0".to_string(),
method: "notifications/message".to_string(),
params: Some(json!({
"data": {
"type": "shell",
"stream": "stderr",
"output": line.to_string(),
}
})),
}))
.ok();

combined_output.push_str(&line);
stderr_buf.clear();
}

else => break,
}

if stdout_done && stderr_done {
break;
}
}
Ok::<_, std::io::Error>(combined_output)
Expand Down
Loading