Skip to content

Commit f814a84

Browse files
milesjclaude
andauthored
internal: Improve code quality of the moon_process crate (#2567)
* internal: Improve code quality of the moon_process crate. - Fix command output being lost entirely when it contains invalid UTF-8 (now lossy converted). - Fix cache key collisions in `Command::get_cache_key` by length-prefixing hashed fields, sorting env vars, and including the env variant discriminant. - Fix command line being logged after streamed output in `exec_stream_and_capture_output`. - Remove `unsafe` from `get_bin_name`. - Return captured lines through join handles instead of `Arc<RwLock<Vec>>`, and dedupe the stream reader loops. - Remove dead code: `output_stream` module and commented-out exec method. - Replace magic signal/errno numbers with libc constants. - Move the arg quoting test harness from `src/main.rs` to `examples/args.rs` so it no longer builds as a default target. - Clear the process registry after force-kill tasks complete, not before. - Declare all used tokio features; hoist `libc` to workspace deps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Hoist windows-sys to workspace dependencies. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Add byte-level exec_stream_and_capture_output_bytes variant. Tees raw chunks instead of lines: partial lines and carriage return redraws render in real time, non-UTF-8 output is preserved, and capture is byte-exact with redraw frames collapsed for clean cache replay. Prefixes are inserted statefully at line starts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Add test coverage for the moon_process crate. Covers the Command builder API, cache key stability and collision resistance, command line formatting, Output status/error conversion, lossy output strings, SharedChild signal handling, the process registry lifecycle, and all four exec methods (capture, continuous, stream, stream+capture) against real child processes. Process spawning tests are unix-gated. Also derives Debug for Output and ChildExit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fix relative working dir test on Windows. Build the working dir with join so it uses the native path separator; a forward slash literal is preserved as-is on Windows and never matches the expected backslash output. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent e2036bd commit f814a84

21 files changed

Lines changed: 1247 additions & 490 deletions

Cargo.toml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ futures = "0.3.31"
3030
hex = "0.4.3"
3131
indexmap = "2.13.0"
3232
iocraft = "0.8.3"
33+
libc = "0.2.186"
3334
md5 = "0.8.0"
3435
miette = "7.6.0"
3536
num_cpus = "1.17.0"
@@ -87,6 +88,7 @@ tower = "0.5.3"
8788
tracing = "0.1.44"
8889
url = "2.5.8"
8990
uuid = { version = "1.23.2", features = ["v4"] }
91+
windows-sys = { version = "0.61.2", default-features = false }
9092

9193
# proto/plugin related
9294
extism = "=1.21.0"

crates/cli/Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ wiggle = "~41.0.4"
5252
liblzma = { version = "0.4.6", features = ["static"] }
5353

5454
[target.'cfg(unix)'.dependencies]
55-
libc = "0.2.186"
55+
libc = { workspace = true }
5656

5757
[dev-dependencies]
5858
moon_affected = { path = "../affected" }

crates/console/src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ use std::ops::DerefMut;
77

88
pub use reporter::*;
99
pub use starbase_console::ConsoleError;
10+
pub use starbase_console::ConsoleStream;
1011
pub use starbase_console::ui;
1112
pub use theme::*;
1213

crates/daemon-utils/Cargo.toml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,15 +10,15 @@ tokio = { workspace = true, features = ["net"] }
1010
tracing = { workspace = true }
1111

1212
[target.'cfg(unix)'.dependencies]
13-
libc = "0.2.186"
13+
libc = { workspace = true }
1414
tokio-stream = { version = "0.1.18", features = ["net"] }
1515

1616
[target.'cfg(windows)'.dependencies]
1717
async-stream = { workspace = true }
1818
futures-core = "0.3.32"
1919
md5 = { workspace = true }
2020
tonic = { workspace = true, features = ["transport"] }
21-
windows-sys = { version = "0.61.2", default-features = false, features = [
21+
windows-sys = { workspace = true, features = [
2222
"Win32_Foundation",
2323
"Win32_System_Threading",
2424
] }

crates/process/Cargo.toml

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,21 @@ starbase_shell = { workspace = true }
1818
system_env = { workspace = true }
1919
thiserror = { workspace = true }
2020
tracing = { workspace = true }
21-
tokio = { workspace = true, features = ["io-util", "signal", "sync"] }
21+
tokio = { workspace = true, features = [
22+
"io-util",
23+
"macros",
24+
"process",
25+
"rt",
26+
"signal",
27+
"sync",
28+
"time",
29+
] }
2230

2331
[target.'cfg(unix)'.dependencies]
24-
libc = "0.2.186"
32+
libc = { workspace = true }
2533

2634
[target.'cfg(windows)'.dependencies]
27-
windows-sys = { version = "0.61.2", default-features = false, features = [
35+
windows-sys = { workspace = true, features = [
2836
# "Win32_System_Console",
2937
"Win32_System_Threading",
3038
] }

crates/process/src/command.rs

Lines changed: 39 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -408,26 +408,11 @@ impl Command {
408408
match &self.exe {
409409
CommandExecutable::Binary(bin) => bin.value.to_string_lossy().to_string(),
410410
CommandExecutable::Script(script) => {
411-
if let Some(inner) = script.to_str() {
412-
match inner.find(' ') {
413-
Some(index) => &inner[0..index],
414-
None => inner,
415-
}
416-
.into()
417-
} else {
418-
let mut bytes = vec![];
419-
420-
for ch in script.as_encoded_bytes() {
421-
if *ch == b' ' {
422-
break;
423-
}
424-
425-
bytes.push(*ch);
426-
}
427-
428-
unsafe { OsString::from_encoded_bytes_unchecked(bytes) }
429-
.to_string_lossy()
430-
.to_string()
411+
let script = script.to_string_lossy();
412+
413+
match script.find(' ') {
414+
Some(index) => script[0..index].to_string(),
415+
None => script.into_owned(),
431416
}
432417
}
433418
}
@@ -436,49 +421,69 @@ impl Command {
436421
pub fn get_cache_key(&self) -> String {
437422
let mut hasher = FxHasher::default();
438423

439-
let mut write = |value: &OsString| {
440-
hasher.write(value.as_os_str().as_encoded_bytes());
441-
};
424+
// Length-prefix each field, otherwise consecutive values hash
425+
// ambiguously, like ("ab", "c") and ("a", "bc")
426+
fn write(hasher: &mut FxHasher, value: &OsStr) {
427+
let bytes = value.as_encoded_bytes();
428+
hasher.write_usize(bytes.len());
429+
hasher.write(bytes);
430+
}
431+
432+
// Sort env vars, as map iteration order is not guaranteed,
433+
// and the key must be stable for identical commands
434+
let mut env = self.env.iter().collect::<Vec<_>>();
435+
env.sort_by(|a, b| a.0.cmp(b.0));
442436

443-
for (key, value) in &self.env {
444-
write(key);
437+
for (key, value) in env {
438+
write(&mut hasher, key);
445439

446440
match value {
447-
Env::Set(value) => write(value),
448-
Env::SetIfMissing(value) => write(value),
449-
Env::Unset => {}
441+
Env::Set(value) => {
442+
hasher.write_u8(1);
443+
write(&mut hasher, value);
444+
}
445+
Env::SetIfMissing(value) => {
446+
hasher.write_u8(2);
447+
write(&mut hasher, value);
448+
}
449+
Env::Unset => {
450+
hasher.write_u8(0);
451+
}
450452
};
451453
}
452454

453455
match &self.exe {
454456
CommandExecutable::Binary(exe) => {
455-
write(&exe.value);
457+
write(&mut hasher, &exe.value);
456458
}
457459
CommandExecutable::Script(exe) => {
458-
write(exe);
460+
write(&mut hasher, exe);
459461
}
460462
};
461463

462464
for arg in &self.args {
463-
write(&arg.value);
465+
write(&mut hasher, &arg.value);
464466
}
465467

466468
if let Some(cwd) = &self.cwd {
467-
write(cwd);
469+
write(&mut hasher, cwd);
468470
}
469471

470472
for arg in &self.input {
471-
write(arg);
473+
write(&mut hasher, arg);
472474
}
473475

474-
format!("{}", hasher.finish())
476+
hasher.finish().to_string()
475477
}
476478

477479
pub fn get_command_line(&self, with_shell: bool, with_input: bool) -> String {
478480
let shell = self.shell.unwrap_or_default().build();
479481
let use_shell = with_shell && (self.shell.is_some() || self.exe.requires_shell());
480482
let mut line = OsString::new();
481483

484+
// Curly quotes are intentional, so that the shell wrapper is
485+
// distinguishable from real quoting within the command itself.
486+
// This output is only used for logs, never executed!
482487
if use_shell {
483488
line.push(shell.to_string());
484489
line.push(" -c “");

0 commit comments

Comments
 (0)