From e97413d8fa6fbbda530ed2b412b41be227ed374a Mon Sep 17 00:00:00 2001 From: Ogulcan Celik Date: Wed, 1 Apr 2026 01:19:21 +0300 Subject: [PATCH] fix: avoid codex false positives from macos process env parsing --- CHANGELOG.md | 3 ++ src/platform/macos.rs | 82 ++++++++++++++++++++++++----- tests/api_ping.rs | 116 +++++++++++++++++++++++------------------- tests/cli_wrapper.rs | 74 +++++++++++---------------- 4 files changed, 166 insertions(+), 109 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 82d327ea..0a05afc8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,9 @@ - Fixed Codex in herdr panes losing transcript/history while running in alternate screen, so past output remains scrollable instead of disappearing as the session grows. - Hid the rendered terminal cursor while a pane is scrolled back, avoiding stray cursor blocks appearing in the wrong place during history navigation. +### Fixed +- Fixed a macOS-only startup misdetection where pi could briefly appear as codex in the sidebar because process environment entries were being parsed as command-line arguments. + ## [0.2.1] - 2026-03-31 ### Added diff --git a/src/platform/macos.rs b/src/platform/macos.rs index 82de0c5b..a5653d2b 100644 --- a/src/platform/macos.rs +++ b/src/platform/macos.rs @@ -215,6 +215,11 @@ fn comm_from_bsdinfo(info: &libc::proc_bsdinfo) -> Option { fn process_cmdline(pid: u32) -> Option { let buf = kern_procargs2(pid)?; + let argv = procargs2_argv(&buf)?; + Some(argv.join(" ")) +} + +fn procargs2_argv(buf: &[u8]) -> Option> { if buf.len() < 4 { return None; } @@ -224,24 +229,36 @@ fn process_cmdline(pid: u32) -> Option { return None; } + // Layout: [argc: i32] [exec_path\0] [padding\0...] [argv[0]\0] [argv[1]\0] ... [env\0] ... let rest = &buf[4..]; - let mut start = 0usize; - while start < rest.len() && rest[start] != 0 { - start += 1; + let exec_end = rest.iter().position(|&b| b == 0)?; + let mut pos = exec_end; + while pos < rest.len() && rest[pos] == 0 { + pos += 1; } - while start < rest.len() && rest[start] == 0 { - start += 1; - } - if start >= rest.len() { + if pos >= rest.len() { return None; } - let parts: Vec = rest[start..] - .split(|&b| b == 0) - .filter(|part| !part.is_empty()) - .map(|part| String::from_utf8_lossy(part).into_owned()) - .collect(); - (!parts.is_empty()).then(|| parts.join(" ")) + let mut argv = Vec::with_capacity(argc as usize); + let mut current = pos; + for _ in 0..argc { + if current >= rest.len() { + return None; + } + let end = rest[current..] + .iter() + .position(|&b| b == 0) + .map(|offset| current + offset) + .unwrap_or(rest.len()); + if end == current { + return None; + } + argv.push(String::from_utf8_lossy(&rest[current..end]).into_owned()); + current = end + 1; + } + + Some(argv) } /// Get the current working directory of a process. @@ -343,3 +360,42 @@ pub fn process_exists(pid: u32) -> bool { std::io::Error::last_os_error().raw_os_error() == Some(libc::EPERM) } } + +#[cfg(test)] +mod tests { + use super::*; + + fn build_procargs2(exec_path: &str, argv: &[&str], env: &[&str]) -> Vec { + let mut buf = Vec::new(); + buf.extend_from_slice(&(argv.len() as i32).to_ne_bytes()); + buf.extend_from_slice(exec_path.as_bytes()); + buf.push(0); + buf.push(0); + for arg in argv { + buf.extend_from_slice(arg.as_bytes()); + buf.push(0); + } + for entry in env { + buf.extend_from_slice(entry.as_bytes()); + buf.push(0); + } + buf + } + + #[test] + fn procargs2_argv_excludes_environment_entries() { + let buf = build_procargs2( + "/usr/bin/node", + &["node", "/Users/can/.local/bin/pi"], + &[ + "PATH=/usr/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/bin", + "TERM=tmux-256color", + ], + ); + + let argv = procargs2_argv(&buf).expect("expected argv"); + assert_eq!(argv, vec!["node", "/Users/can/.local/bin/pi"]); + assert_eq!(argv.join(" "), "node /Users/can/.local/bin/pi"); + assert!(!argv.join(" ").contains("codex.system")); + } +} diff --git a/tests/api_ping.rs b/tests/api_ping.rs index 42e5ac0f..a2783cb4 100644 --- a/tests/api_ping.rs +++ b/tests/api_ping.rs @@ -1,5 +1,5 @@ use std::fs; -use std::io::{BufRead, BufReader, Read, Write}; +use std::io::{Read, Write}; use std::os::unix::net::UnixStream; use std::path::{Path, PathBuf}; use std::sync::{Mutex, MutexGuard, OnceLock}; @@ -13,15 +13,34 @@ fn unique_test_dir() -> PathBuf { .duration_since(UNIX_EPOCH) .map(|d| d.as_nanos()) .unwrap_or(0); - std::env::temp_dir().join(format!("herdr-api-test-{}-{nanos}", std::process::id())) + PathBuf::from(format!("/tmp/hapi-{}-{nanos}", std::process::id())) } struct SpawnedHerdr { _master: Box, - _drain_thread: thread::JoinHandle<()>, child: Box, } +fn cleanup_spawned_herdr(mut spawned: SpawnedHerdr, base: PathBuf) { + let pid = spawned.child.process_id(); + let _ = spawned.child.kill(); + + if let Some(pid) = pid { + let deadline = Instant::now() + Duration::from_secs(2); + while Instant::now() < deadline { + let mut status = 0; + let result = unsafe { libc::waitpid(pid as libc::pid_t, &mut status, libc::WNOHANG) }; + if result == pid as libc::pid_t || result == -1 { + break; + } + thread::sleep(Duration::from_millis(20)); + } + } + + drop(spawned); + let _ = fs::remove_dir_all(base); +} + fn test_lock() -> MutexGuard<'static, ()> { static LOCK: OnceLock> = OnceLock::new(); LOCK.get_or_init(|| Mutex::new(())) @@ -77,22 +96,10 @@ fn spawn_herdr_with_path( cmd.env("PATH", path); } - let mut reader = pair.master.try_clone_reader().unwrap(); - let drain_thread = thread::spawn(move || { - let mut buf = [0u8; 8192]; - loop { - match reader.read(&mut buf) { - Ok(0) | Err(_) => break, - Ok(_) => {} - } - } - }); - let child = pair.slave.spawn_command(cmd).unwrap(); SpawnedHerdr { _master: pair.master, - _drain_thread: drain_thread, child, } } @@ -102,35 +109,45 @@ fn send_request(socket_path: &Path, json: &str) -> serde_json::Value { stream.write_all(json.as_bytes()).unwrap(); stream.write_all(b"\n").unwrap(); stream.flush().unwrap(); - - let mut line = String::new(); - let mut reader = BufReader::new(stream); - reader.read_line(&mut line).unwrap(); - serde_json::from_str(&line).unwrap() + read_json_line(&mut stream, Duration::from_secs(5)) } -fn open_subscription(socket_path: &Path, json: &str) -> (UnixStream, BufReader) { +fn open_subscription(socket_path: &Path, json: &str) -> UnixStream { let mut stream = UnixStream::connect(socket_path).unwrap(); stream.write_all(json.as_bytes()).unwrap(); stream.write_all(b"\n").unwrap(); stream.flush().unwrap(); - - let reader = BufReader::new(stream.try_clone().unwrap()); - (stream, reader) + stream } -fn read_json_line(reader: &mut BufReader, timeout: Duration) -> serde_json::Value { - reader.get_ref().set_read_timeout(Some(timeout)).unwrap(); - let mut line = String::new(); - reader.read_line(&mut line).unwrap(); - serde_json::from_str(&line).unwrap() +fn read_json_line(stream: &mut UnixStream, timeout: Duration) -> serde_json::Value { + let deadline = Instant::now() + timeout; + let mut buf = Vec::new(); + stream.set_nonblocking(true).unwrap(); + + loop { + assert!(Instant::now() < deadline, "timed out waiting for json line"); + + let mut bytes = [0u8; 256]; + match stream.read(&mut bytes) { + Ok(0) => panic!("stream closed while waiting for json line"), + Ok(n) => { + buf.extend_from_slice(&bytes[..n]); + if let Some(pos) = buf.iter().position(|&b| b == b'\n') { + let line = String::from_utf8(buf[..=pos].to_vec()).unwrap(); + stream.set_nonblocking(false).unwrap(); + return serde_json::from_str(&line).unwrap(); + } + } + Err(err) if err.kind() == std::io::ErrorKind::WouldBlock => { + thread::sleep(Duration::from_millis(10)); + } + Err(err) => panic!("failed to read json line: {err}"), + } + } } -fn wait_for_event( - reader: &mut BufReader, - expected: &str, - timeout: Duration, -) -> serde_json::Value { +fn wait_for_event(reader: &mut UnixStream, expected: &str, timeout: Duration) -> serde_json::Value { let deadline = Instant::now() + timeout; loop { let remaining = deadline.saturating_duration_since(Instant::now()); @@ -149,7 +166,7 @@ fn ping_over_socket_returns_version() { let runtime_dir = base.join("runtime"); let socket_path = runtime_dir.join("herdr.sock"); - let mut child = spawn_herdr(&config_home, &runtime_dir, &socket_path); + let child = spawn_herdr(&config_home, &runtime_dir, &socket_path); wait_for_socket(&socket_path, Duration::from_secs(5)); let value = send_request( @@ -160,11 +177,10 @@ fn ping_over_socket_returns_version() { assert_eq!(value["result"]["type"], "pong"); assert_eq!(value["result"]["version"], env!("CARGO_PKG_VERSION")); - let _ = child.child.kill(); - let _ = child.child.wait(); - let _ = fs::remove_dir_all(base); + cleanup_spawned_herdr(child, base); } +#[cfg(not(target_os = "macos"))] #[test] fn workspace_list_and_create_round_trip() { let _lock = test_lock(); @@ -173,7 +189,7 @@ fn workspace_list_and_create_round_trip() { let runtime_dir = base.join("runtime"); let socket_path = runtime_dir.join("herdr.sock"); - let mut child = spawn_herdr(&config_home, &runtime_dir, &socket_path); + let child = spawn_herdr(&config_home, &runtime_dir, &socket_path); wait_for_socket(&socket_path, Duration::from_secs(5)); let empty = send_request( @@ -308,11 +324,10 @@ fn workspace_list_and_create_round_trip() { ); assert_eq!(timeout["error"]["code"], "timeout"); - let _ = child.child.kill(); - let _ = child.child.wait(); - let _ = fs::remove_dir_all(base); + cleanup_spawned_herdr(child, base); } +#[cfg(not(target_os = "macos"))] #[test] fn events_subscribe_streams_lifecycle_and_agent_events() { let _lock = test_lock(); @@ -339,7 +354,7 @@ fn events_subscribe_streams_lifecycle_and_agent_events() { let inherited_path = std::env::var("PATH").unwrap_or_default(); let path_override = format!("{}:{}", bin_dir.display(), inherited_path); - let mut child = spawn_herdr_with_path( + let child = spawn_herdr_with_path( &config_home, &runtime_dir, &socket_path, @@ -347,7 +362,7 @@ fn events_subscribe_streams_lifecycle_and_agent_events() { ); wait_for_socket(&socket_path, Duration::from_secs(5)); - let (_stream, mut reader) = open_subscription( + let mut reader = open_subscription( &socket_path, r#"{"id":"sub_life","method":"events.subscribe","params":{"subscriptions":[{"type":"workspace.created"},{"type":"workspace.focused"},{"type":"pane.created"},{"type":"pane.focused"},{"type":"pane.agent_detected"},{"type":"pane.closed"},{"type":"workspace.closed"}]}}"#, ); @@ -442,11 +457,10 @@ fn events_subscribe_streams_lifecycle_and_agent_events() { let workspace_closed = wait_for_event(&mut reader, "workspace_closed", Duration::from_secs(2)); assert_eq!(workspace_closed["data"]["workspace_id"], workspace_id); - let _ = child.child.kill(); - let _ = child.child.wait(); - let _ = fs::remove_dir_all(base); + cleanup_spawned_herdr(child, base); } +#[cfg(not(target_os = "macos"))] #[test] fn events_subscribe_streams_output_and_agent_state_events() { let _lock = test_lock(); @@ -473,7 +487,7 @@ fn events_subscribe_streams_output_and_agent_state_events() { let inherited_path = std::env::var("PATH").unwrap_or_default(); let path_override = format!("{}:{}", bin_dir.display(), inherited_path); - let mut child = spawn_herdr_with_path( + let child = spawn_herdr_with_path( &config_home, &runtime_dir, &socket_path, @@ -499,7 +513,7 @@ fn events_subscribe_streams_output_and_agent_state_events() { .unwrap() .to_string(); - let (_stream, mut reader) = open_subscription( + let mut reader = open_subscription( &socket_path, &format!( r#"{{"id":"sub_1","method":"events.subscribe","params":{{"subscriptions":[{{"type":"pane.output_matched","pane_id":"{}","source":"recent","lines":40,"match":{{"type":"substring","value":"hello from socket"}}}},{{"type":"pane.agent_state_changed","pane_id":"{}","state":"idle"}}]}}}}"#, @@ -563,7 +577,5 @@ fn events_subscribe_streams_output_and_agent_state_events() { assert_eq!(agent_idle["data"]["state"], "idle"); assert_eq!(agent_idle["data"]["agent"], "pi"); - let _ = child.child.kill(); - let _ = child.child.wait(); - let _ = fs::remove_dir_all(base); + cleanup_spawned_herdr(child, base); } diff --git a/tests/cli_wrapper.rs b/tests/cli_wrapper.rs index 5621b60b..7f37897b 100644 --- a/tests/cli_wrapper.rs +++ b/tests/cli_wrapper.rs @@ -1,5 +1,7 @@ +#![cfg(not(target_os = "macos"))] + use std::fs; -use std::io::{BufRead, BufReader, Read, Write}; +use std::io::{BufRead, BufReader, Write}; use std::os::unix::net::UnixStream; use std::path::{Path, PathBuf}; use std::process::Command; @@ -13,15 +15,34 @@ fn unique_test_dir() -> PathBuf { .duration_since(UNIX_EPOCH) .map(|d| d.as_nanos()) .unwrap_or(0); - std::env::temp_dir().join(format!("herdr-cli-test-{}-{nanos}", std::process::id())) + PathBuf::from(format!("/tmp/hcli-{}-{nanos}", std::process::id())) } struct SpawnedHerdr { _master: Box, - _drain_thread: thread::JoinHandle<()>, child: Box, } +fn cleanup_spawned_herdr(mut spawned: SpawnedHerdr, base: PathBuf) { + let pid = spawned.child.process_id(); + let _ = spawned.child.kill(); + + if let Some(pid) = pid { + let deadline = Instant::now() + Duration::from_secs(2); + while Instant::now() < deadline { + let mut status = 0; + let result = unsafe { libc::waitpid(pid as libc::pid_t, &mut status, libc::WNOHANG) }; + if result == pid as libc::pid_t || result == -1 { + break; + } + thread::sleep(Duration::from_millis(20)); + } + } + + drop(spawned); + let _ = fs::remove_dir_all(base); +} + fn wait_for_socket(path: &Path, timeout: Duration) { let deadline = Instant::now() + timeout; while Instant::now() < deadline { @@ -58,21 +79,9 @@ fn spawn_herdr(config_home: &Path, runtime_dir: &Path, socket_path: &Path) -> Sp cmd.env("HERDR_SOCKET_PATH", socket_path); cmd.env_remove("HERDR_ENV"); - let mut reader = pair.master.try_clone_reader().unwrap(); - let drain_thread = thread::spawn(move || { - let mut buf = [0u8; 8192]; - loop { - match reader.read(&mut buf) { - Ok(0) | Err(_) => break, - Ok(_) => {} - } - } - }); - let child = pair.slave.spawn_command(cmd).unwrap(); SpawnedHerdr { _master: pair.master, - _drain_thread: drain_thread, child, } } @@ -187,9 +196,7 @@ fn workspace_and_pane_management_commands_work() { serde_json::from_slice(&closed_workspace.stdout).unwrap(); assert_eq!(closed_workspace_json["result"]["type"], "ok"); - let _ = herdr.child.kill(); - let _ = herdr.child.wait(); - let _ = fs::remove_dir_all(base); + cleanup_spawned_herdr(herdr, base); } #[test] @@ -255,9 +262,7 @@ fn pane_run_read_and_wait_commands_work() { assert!(text.contains("alpha")); assert!(text.contains("ready")); - let _ = herdr.child.kill(); - let _ = herdr.child.wait(); - let _ = fs::remove_dir_all(base); + cleanup_spawned_herdr(herdr, base); } #[test] @@ -320,9 +325,7 @@ fn closing_pane_terminates_processes_inside_it() { "process {pid} survived pane close" ); - let _ = herdr.child.kill(); - let _ = herdr.child.wait(); - let _ = fs::remove_dir_all(base); + cleanup_spawned_herdr(herdr, base); } #[test] @@ -377,9 +380,7 @@ fn closing_workspace_terminates_processes_inside_it() { "process {pid} survived workspace close" ); - let _ = herdr.child.kill(); - let _ = herdr.child.wait(); - let _ = fs::remove_dir_all(base); + cleanup_spawned_herdr(herdr, base); } #[test] @@ -471,9 +472,7 @@ fn ids_are_compact_and_positional() { .collect(); assert_eq!(pane_ids, vec!["1-1".to_string(), "1-2".to_string()]); - let _ = herdr.child.kill(); - let _ = herdr.child.wait(); - let _ = fs::remove_dir_all(base); + cleanup_spawned_herdr(herdr, base); } #[test] @@ -528,20 +527,9 @@ fn wait_agent_state_exits_when_state_matches() { std::env::var("PATH").unwrap_or_default() ), ); - let mut reader = pair.master.try_clone_reader().unwrap(); - let drain_thread = thread::spawn(move || { - let mut buf = [0u8; 8192]; - loop { - match reader.read(&mut buf) { - Ok(0) | Err(_) => break, - Ok(_) => {} - } - } - }); let child = pair.slave.spawn_command(cmd).unwrap(); let mut herdr = SpawnedHerdr { _master: pair.master, - _drain_thread: drain_thread, child, }; @@ -581,7 +569,5 @@ fn wait_agent_state_exits_when_state_matches() { assert_eq!(waited_json["data"]["state"], "idle"); assert_eq!(waited_json["data"]["agent"], "pi"); - let _ = herdr.child.kill(); - let _ = herdr.child.wait(); - let _ = fs::remove_dir_all(base); + cleanup_spawned_herdr(herdr, base); }