Skip to content

Windows: hide background Dev MCP shell windows #2488

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

Open
paritosh921 wants to merge 1 commit into
base: main
Choose a base branch
from
+40 −16
Open

Windows: hide background Dev MCP shell windows #2488

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
56 changes: 40 additions & 16 deletions crates/buzz-dev-mcp/src/shell.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ const TAIL_BYTES: usize = 8 * 1024;
const ARTIFACT_RING_SIZE: usize = 8;
const READ_CHUNK: usize = 16 * 1024;

// Windows tests below temporarily change process-wide shell environment
// variables. Keep those tests separate from tests that create a real shell.
#[cfg(all(test, windows))]
static TEST_ENV_MUTEX: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(());

pub struct SharedState {
pub cwd: PathBuf,
pub shim: Shim,
Expand Down Expand Up @@ -177,6 +182,7 @@ pub async fn run(
cmd.stderr(Stdio::piped());
cmd.kill_on_drop(true);
set_process_group(&mut cmd);
suppress_console_window(&mut cmd);

let started = Instant::now();
let mut child = match cmd.spawn() {
Expand Down Expand Up @@ -322,6 +328,19 @@ pub async fn run(
Ok(CallToolResult::success(vec![Content::text(text)]))
}

/// Shell tool calls are background work for an agent. On Windows, a console
/// application such as Git Bash otherwise creates a visible terminal window
/// for every command, even though stdout and stderr are captured by this MCP
/// server.
#[cfg(windows)]
fn suppress_console_window(cmd: &mut Command) {
const CREATE_NO_WINDOW: u32 = 0x0800_0000;
cmd.creation_flags(CREATE_NO_WINDOW);
}

#[cfg(not(windows))]
fn suppress_console_window(_cmd: &mut Command) {}

/// The flag used to pass a command string to the shell.
///
/// bash/zsh/sh: `-c`
Expand Down Expand Up @@ -952,19 +971,26 @@ mod tests {
Some(t) => t.text.clone(),
None => panic!("no text content"),
};
serde_json::from_str(&text).expect("json")
serde_json::from_str(&text)
.unwrap_or_else(|error| panic!("expected JSON result ({error}); received: {text:?}"))
}

#[tokio::test(flavor = "current_thread")]
async fn basic_echo() {
#[cfg(windows)]
let _env_guard = TEST_ENV_MUTEX.lock().await;
let dir = tempdir().expect("tempdir");
let state = make_state(dir.path());
let r = run(
&state,
ShellParams {
command: "echo hello".into(),
workdir: None,
timeout_ms: Some(5_000),
// Git Bash can take several seconds to initialise on a busy
// Windows CI worker. This test verifies output, not startup
// performance, so avoid treating that startup variance as a
// shell-tool failure.
timeout_ms: Some(15_000),
},
CancellationToken::new(),
)
Expand All @@ -978,6 +1004,8 @@ mod tests {

#[tokio::test(flavor = "current_thread")]
async fn timeout_fires() {
#[cfg(windows)]
let _env_guard = TEST_ENV_MUTEX.lock().await;
let dir = tempdir().expect("tempdir");
let state = make_state(dir.path());
let r = run(
Expand All @@ -1003,6 +1031,8 @@ mod tests {

#[tokio::test(flavor = "current_thread")]
async fn workdir_is_honored() {
#[cfg(windows)]
let _env_guard = TEST_ENV_MUTEX.lock().await;
let dir = tempdir().expect("tempdir");
let sub = dir.path().join("sub");
std::fs::create_dir(&sub).expect("mkdir sub");
Expand Down Expand Up @@ -1036,14 +1066,8 @@ mod tests {
mod windows_resolver_tests {
use super::*;
use std::env;
use std::sync::Mutex;
use tempfile::tempdir;

// Process-global env mutation guard: tests that mutate BUZZ_SHELL,
// SystemRoot, or GIT_BASH must hold this lock for the duration of the
// test so parallel test threads cannot race on these env vars.
static ENV_MUTEX: Mutex<()> = Mutex::new(());

fn touch(path: &Path) {
if let Some(parent) = path.parent() {
std::fs::create_dir_all(parent).expect("mkdir");
Expand All @@ -1053,7 +1077,7 @@ mod windows_resolver_tests {

#[test]
fn buzz_shell_override_wins_over_everything() {
let _guard = ENV_MUTEX.lock().unwrap_or_else(|p| p.into_inner());
let _guard = TEST_ENV_MUTEX.blocking_lock();
// BUZZ_SHELL pointing at a real file must be returned without probing
// the standard Git-for-Windows locations or PATH.
let dir = tempdir().expect("tempdir");
Expand All @@ -1069,7 +1093,7 @@ mod windows_resolver_tests {

#[test]
fn buzz_shell_override_skipped_when_path_absent() {
let _guard = ENV_MUTEX.lock().unwrap_or_else(|p| p.into_inner());
let _guard = TEST_ENV_MUTEX.blocking_lock();
// If BUZZ_SHELL points at a non-existent path the resolver must fall
// through rather than returning a dead path.
env::set_var("BUZZ_SHELL", r"C:\does\not\exist\bash.exe");
Expand All @@ -1091,7 +1115,7 @@ mod windows_resolver_tests {
/// exclusion — cmd/pwsh live in System32 legitimately.
#[test]
fn buzz_shell_explicit_bare_name_resolves_from_system32() {
let _guard = ENV_MUTEX.lock().unwrap_or_else(|p| p.into_inner());
let _guard = TEST_ENV_MUTEX.blocking_lock();
// Simulate cmd.exe living in a dir that would be excluded by the WSL guard.
// The explicit BUZZ_SHELL branch must NOT skip System32.
let sys32 = tempdir().expect("sys32");
Expand Down Expand Up @@ -1123,7 +1147,7 @@ mod windows_resolver_tests {
/// Implicit bash.exe scan still skips System32 (WSL guard intact).
#[test]
fn implicit_bash_scan_still_skips_system32() {
let _guard = ENV_MUTEX.lock().unwrap_or_else(|p| p.into_inner());
let _guard = TEST_ENV_MUTEX.blocking_lock();
// Same setup: bash.exe is only in a dir that is under SystemRoot.
// Without an explicit BUZZ_SHELL, the fallback scan must skip it.
let sys32 = tempdir().expect("sys32");
Expand Down Expand Up @@ -1163,7 +1187,7 @@ mod windows_resolver_tests {
/// resolved_shell whose display name appears in bootstrap_instructions.
#[test]
fn shared_state_bootstrap_hint_matches_resolved_shell() {
let _guard = ENV_MUTEX.lock().unwrap_or_else(|p| p.into_inner());
let _guard = TEST_ENV_MUTEX.blocking_lock();
let dir = tempdir().expect("tempdir");
let fake_pwsh = dir.path().join("pwsh.exe");
touch(&fake_pwsh);
Expand All @@ -1186,7 +1210,7 @@ mod windows_resolver_tests {
/// When pwsh.exe is on PATH, resolve_bash must return it and report "pwsh".
#[test]
fn buzz_shell_bare_name_resolved_through_path_when_present() {
let _guard = ENV_MUTEX.lock().unwrap_or_else(|p| p.into_inner());
let _guard = TEST_ENV_MUTEX.blocking_lock();
let dir = tempdir().expect("tempdir");
let fake_pwsh = dir.path().join("pwsh.exe");
touch(&fake_pwsh);
Expand All @@ -1205,7 +1229,7 @@ mod windows_resolver_tests {
/// report pwsh as the active shell.
#[test]
fn buzz_shell_bare_name_absent_from_path_falls_through() {
let _guard = ENV_MUTEX.lock().unwrap_or_else(|p| p.into_inner());
let _guard = TEST_ENV_MUTEX.blocking_lock();
// Set BUZZ_SHELL to a command that won't be on any real PATH.
env::set_var("BUZZ_SHELL", "buzz-shell-does-not-exist-xyz");
let result = resolve_bash("");
Expand All @@ -1226,7 +1250,7 @@ mod windows_resolver_tests {

#[test]
fn git_cmd_on_path_resolves_sibling_git_bash() {
let _guard = ENV_MUTEX.lock().unwrap_or_else(|p| p.into_inner());
let _guard = TEST_ENV_MUTEX.blocking_lock();
let dir = tempdir().expect("tempdir");
let git = dir.path().join("Git").join("cmd").join("git.exe");
let bash = dir.path().join("Git").join("bin").join("bash.exe");
Expand Down