diff --git a/crates/toolkit/src/fetch.rs b/crates/toolkit/src/fetch.rs index 868410c..af122aa 100644 --- a/crates/toolkit/src/fetch.rs +++ b/crates/toolkit/src/fetch.rs @@ -80,7 +80,16 @@ pub fn fetch_with(curl: &Path, args: &HttpFetchArgs) -> Outcome { Some(r) => r, None => return Outcome::tool_error("http_fetch: curl has no standard error".to_string()), }; - let stderr_handle = std::thread::spawn(move || read_capped(stderr_reader)); + // `Builder`, not `spawn`, which panics when the system refuses a thread. + let stderr_handle = match std::thread::Builder::new().spawn(move || read_capped(stderr_reader)) + { + Ok(handle) => handle, + Err(e) => { + let _ = child.kill(); + let _ = child.wait(); + return Outcome::tool_error(format!("http_fetch: cannot start a thread: {e}")); + } + }; let mut body = Vec::new(); let mut stdout = match child.stdout.take() { diff --git a/crates/toolkit/src/files.rs b/crates/toolkit/src/files.rs index e73424a..71cbc9a 100644 --- a/crates/toolkit/src/files.rs +++ b/crates/toolkit/src/files.rs @@ -35,7 +35,10 @@ pub fn read_file(args: &ReadFileArgs) -> Outcome { }; let mut buf = Vec::new(); - let n = match file.take(MAX_READ as u64 + 1).read_to_end(&mut buf) { + let n = match file + .take(u64::try_from(MAX_READ).map_or(u64::MAX, |n| n.saturating_add(1))) + .read_to_end(&mut buf) + { Err(e) => return tool_err("read_file", path, &e.to_string()), Ok(n) => n, }; diff --git a/crates/toolkit/src/input.rs b/crates/toolkit/src/input.rs index aba7f25..9f57141 100644 --- a/crates/toolkit/src/input.rs +++ b/crates/toolkit/src/input.rs @@ -33,7 +33,9 @@ impl From for InputError { /// (`Read::take`); more than MAX_INPUT is TooLarge. pub fn read_input(stdin: &mut dyn std::io::Read) -> Result { let mut buf = Vec::new(); - let n = stdin.take(MAX_INPUT as u64 + 1).read_to_end(&mut buf)?; + let n = stdin + .take(u64::try_from(MAX_INPUT).map_or(u64::MAX, |n| n.saturating_add(1))) + .read_to_end(&mut buf)?; if n > MAX_INPUT { return Err(InputError::TooLarge); } diff --git a/crates/toolkit/src/main.rs b/crates/toolkit/src/main.rs index acd35fd..aa41d48 100644 --- a/crates/toolkit/src/main.rs +++ b/crates/toolkit/src/main.rs @@ -18,7 +18,8 @@ fn main() -> ExitCode { /// The `egress-proxy --socket --allow ` form. Anything else, including a program name /// that is not `egress-proxy`, returns `None` so the tool form handles it. fn parse_egress_proxy(args: &[OsString]) -> Option { - if args.first()?.as_bytes() != b"egress-proxy" { + // Exactly five words: a longer list is not this form, and goes to the tool form (exit 2). + if args.len() != 5 || args.first()?.as_bytes() != b"egress-proxy" { return None; } // The form is `egress-proxy --socket --allow `, in that order. diff --git a/crates/toolkit/tests/egress_form.rs b/crates/toolkit/tests/egress_form.rs new file mode 100644 index 0000000..033168f --- /dev/null +++ b/crates/toolkit/tests/egress_form.rs @@ -0,0 +1,42 @@ +//! `toolkit egress-proxy` takes exactly `--socket --allow `: with anything after +//! them it is not the proxy, and exits 2 at once rather than listening (M3b review). Do not edit. + +use std::process::{Command, Stdio}; +use std::time::{Duration, Instant}; + +#[test] +fn trailing_arguments_are_not_the_proxy_form() { + let dir = std::env::temp_dir().join(format!("tk-egress-form-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let socket = dir.join("egress.sock"); + let mut child = Command::new(env!("CARGO_BIN_EXE_toolkit")) + .args([ + "egress-proxy", + "--socket", + socket.to_str().unwrap(), + "--allow", + "example.com", + "extra", + ]) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .unwrap(); + let until = Instant::now() + Duration::from_secs(3); + let status = loop { + if let Some(status) = child.try_wait().unwrap() { + break status; + } + if Instant::now() > until { + let _ = child.kill(); + let _ = child.wait(); + panic!("it is listening: trailing arguments were accepted"); + } + std::thread::sleep(Duration::from_millis(20)); + }; + assert_eq!(status.code(), Some(2)); + assert!(!socket.exists(), "no socket was made"); + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/docs/implementer-log.md b/docs/implementer-log.md index e0a6dd4..d3a90d5 100644 --- a/docs/implementer-log.md +++ b/docs/implementer-log.md @@ -6,6 +6,7 @@ reviewer adds findings under "Reviews" once per milestone. | Task | Date | Status | Gate runs | First gate | Deviations | Notes | Model | |---|---|---|---|---|---|---|---| +| M3b/17-toolkit-nits | 2026-09-23 | done | 1 | pass | none | Three small fixes. `fetch.rs`: replaced `std::thread::spawn` with a `Builder::new().spawn` match that kills and waits on a spawn error and returns `Outcome::tool_error("http_fetch: cannot start a thread: {e}")`. `input.rs` and `files.rs`: replaced `MAX_INPUT as u64 + 1` / `MAX_READ as u64 + 1` with `u64::try_from(MAX_*).map_or(u64::MAX, |n| n.saturating_add(1))`. `main.rs` `parse_egress_proxy`: the first check is now `args.len() != 5 || args.first()?.as_bytes() != b"egress-proxy"` so a longer list goes to the tool form (exit 2). Copied `tests/egress_form.rs`; the one test failed before the fix and passed in 0.02s after. `grep "thread::spawn\| as u64" crates/toolkit/src/` prints nothing. `make gate` prints `gate: ok` first run. | ? | | M3b/16-brokerd-log-escaping | 2026-09-23 | done | 1 | pass | none | Escaped the container's standard error before logging it (M3b review finding 5) and prefixed/quoted three small texts (finding 7). `container.rs` `answer`: exit 2 logs `brokerd: {name}: the tool could not run: {err:?}` instead of the raw stderr; exit 125..=127 logs `brokerd: podman could not start {name}: {err:?}\n{RUNBOOK}` (was `{err}\n{RUNBOOK}`); the `_` arm logs `brokerd: container {name} exited {status}: {err:?}` (the trailing `\n{err}` moved inside the debug format). `start_egress`: the proxy's `podman run -d` failure now logs `{stderr:?}`. `main.rs`: the runtime notice prints `brokerd: {runtime_notice}`. `config.rs`: the image and memory `[runner]` errors use `{:?}` so the bad value is quoted. Copied `tests/container_log.rs` and `tests/notices.rs`; 4, 2, 2 and 7 passed; `make gate` prints `gate: ok` first run. | ? | | M3b/15-brokerd-pipes-grace | 2026-09-23 | done | 1 | pass | none | Fixed the two M3b review findings in `pipes.rs`. Added `pub const GRACE: Duration = Duration::from_secs(2)`; replaced the `JoinHandle`-holding `Io` with one holding `Option, bool)>>` for stdout/stderr and added the `Finished { out, truncated, err, open }` struct. `Io::start` now returns `std::io::Result`: the stdin writer is started with `Builder::new().spawn(...)` and its handle dropped (never joined, a spawn error returned with `?`); a new private `reader(pipe, cap) -> io::Result>` starts one detached reader per pipe and returns a spawn error, used for both stdout (`cap`) and stderr (`err_cap`). `Io::finish(grace)` sets one `until = Instant::now() + grace` and calls a private `collect(rx, until)` per receiver: `recv_timeout(until.saturating_duration_since(now))` → `Ok` keeps bytes, `Timeout` sets `open`, `Disconnected` (panicked reader) counts closed; `open` is `stdout_open || stderr_open`. `read_capped` unchanged in behavior, now `cap.saturating_sub(kept.len())` and `chunk.get(..take).unwrap_or_default()` (no indexing, no wrapping subtraction). `container.rs`: added `pub const OUTPUT_OPEN` after `TIMED_OUT`, `use crate::pipes::{GRACE, Io}`, and replaced the `run_container` tail with the task's `Io::start` match (on spawn error, `podman kill`/`rm -f`, `child.kill()`/`wait()`, log + `Unavailable(CANNOT_START)`), then `io.finish(GRACE)` and the `done.open && status.is_some()` check returning `Failed(OUTPUT_OPEN)` (a timed-out call, `status` None, still answers `TIMED_OUT`). All `std::thread::spawn` gone from `crates/brokerd/src/`. Copied `tests/container_grace.rs`; 4 passed in ~4.4s, container 11, container_egress 6, all five runs; `make gate` prints `gate: ok` first run. | ? | | M3b/14-brokerd-pipes-module | 2026-09-23 | done | 1 | pass | none | Pure move: cut `struct Io`, `impl Io { start, finish }`, and `fn read_capped` from `container.rs` and pasted them into `crates/brokerd/src/pipes.rs` with bodies unchanged and `pub(crate)` visibility; `Io::start` now calls `crate::container::STDERR_KEPT` (the constant stays in `container.rs` since the tests import it). Added the module doc comment and the `use std::io::{Read, Write}`, `use std::process::Child`, `use std::thread::JoinHandle` lines. `lib.rs` gained `pub mod pipes;` between `ledger` and `podman`. `container.rs` gained `use crate::pipes::Io;` and lost the `Read, Write` and `JoinHandle` imports the compiler reported unused; nothing else changed. `cargo fmt --all` reflowed the `STDERR_KEPT` call line in `start` to wrap. `cargo check`/clippy clean; container 11, container_egress 6, serve_runner 2 pass; `grep "struct Io\|fn read_capped" container.rs` prints nothing and `container.rs` is 350 lines. `make gate` prints `gate: ok` on the first run. | ? |