From b35063cf467987980d19e1125659129ddda7aa6a Mon Sep 17 00:00:00 2001 From: damocles Date: Fri, 28 Aug 2026 18:37:08 +0200 Subject: [PATCH] hive-priv: filter/trim before counting output-path lines, fix log level --- hive-priv/README.md | 13 ++++--- hive-priv/src/main.rs | 80 +++++++++++++++++++++++++++++++------------ 2 files changed, 67 insertions(+), 26 deletions(-) diff --git a/hive-priv/README.md b/hive-priv/README.md index 21e39350..d23b44c3 100644 --- a/hive-priv/README.md +++ b/hive-priv/README.md @@ -82,8 +82,11 @@ the end — forwarding it the same way would risk interleaving a progress line into the value this function hands back as `--system-path`, trading a closure-mixup bug for a corrupted-argument one. So stdout lines are accumulated silently and only consulted after the exit status is known -to be success — and even then, exactly one line is required (`nix build ---print-out-paths` prints one line *per output*, not one line total; -`config.system.build.toplevel` is single-output today, but a bare -`.trim()` would silently hand a multi-line string to `--system-path` the -day that ever changes, so a wrong line count `bail!`s instead). +to be success — and even then, exactly one non-empty, trimmed line is +required (`nix build --print-out-paths` prints one line *per output*, +not one line total; `config.system.build.toplevel` is single-output +today, but a bare whole-buffer `.trim()` would silently hand a +multi-line string to `--system-path` the day that ever changes, and a +bare untrimmed/unfiltered `.lines()` turns a lone `"\n"` into a bogus +empty-string "path" — a wrong line count, or a blank/whitespace one, +`bail!`s instead). diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs index 98a28a1c..5038d839 100644 --- a/hive-priv/src/main.rs +++ b/hive-priv/src/main.rs @@ -20,7 +20,7 @@ use std::os::fd::{AsRawFd as _, FromRawFd as _, OwnedFd, RawFd}; use std::path::{Path, PathBuf}; -use anyhow::{Context as _, Result, bail}; +use anyhow::{Context as _, Result, anyhow, bail}; use hive_priv_sock::{ AGENT_PREFIX, AGENT_RUNTIME_ROOT, AGENT_STATE_ROOT, AgentTmpfilesEntry, BindMount, CredentialMount, InfraAction, InfraContainer, JournalQuery, META_DIR, MIGRATE_STAGING_ROOT, @@ -747,7 +747,7 @@ async fn nix_build_toplevel(name: &str, mut writer: Option<&mut OwnedWriteHalf>) // Streamed as it arrives — the progress the dashboard // and `journalctl -f` were missing. Ok(Some(l)) => { - tracing::warn!(target: "nix-build-toplevel", "{l}"); + tracing::info!(target: "nix-build-toplevel", "{l}"); if let Some(w) = writer.as_deref_mut() { write_line_event(w, PrivStream::Stderr, &l).await; } @@ -778,23 +778,36 @@ async fn nix_build_toplevel(name: &str, mut writer: Option<&mut OwnedWriteHalf>) stderr_buf.lines().last().unwrap_or("").trim() ); } - // `--print-out-paths` prints one line *per output*, not one line - // total: `nix build --no-link --print-out-paths nixpkgs#openssl` - // prints two (`…-bin`, `…-man`). `config.system.build.toplevel` is - // single-output today, so this is one line in practice — but a bare - // `.trim()` would silently hand a multi-line string on to - // `--system-path` the day that ever changes, which is the same - // corrupted-argument failure this function exists to avoid. Require - // exactly one line and error otherwise, so a future multi-output - // attr fails loudly here instead of downstream in `nixos-container`. - let lines: Vec<&str> = stdout_buf.lines().collect(); - let [path] = lines[..] else { - bail!( - "nix build {attr} produced {} output path(s), expected exactly 1: {stdout_buf:?}", - lines.len() - ); - }; - Ok(path.to_owned()) + single_output_path(&stdout_buf) + .map(str::to_owned) + .map_err(|count| { + anyhow!("nix build {attr} produced {count} output path(s), expected exactly 1: {stdout_buf:?}") + }) +} + +/// Parse `nix build --print-out-paths`' stdout down to the single output +/// path this function's caller expects. `--print-out-paths` prints one +/// line *per output*, not one line total (`nix build --no-link +/// --print-out-paths nixpkgs#openssl` prints two: `…-bin`, `…-man`); +/// `config.system.build.toplevel` is single-output today, so this is one +/// line in practice — but a bare whole-buffer `.trim()` would silently +/// hand a multi-line string on to `--system-path` the day that ever +/// changes, the same corrupted-argument failure this function exists to +/// avoid. Trims and drops empty lines *before* counting, so a lone +/// `"\n"` (or trailing whitespace on the real line) can't be mistaken +/// for a present-but-blank path — see the unit tests below for the exact +/// table this closes. `Err` carries the surviving line count, for the +/// caller's error message. +fn single_output_path(stdout: &str) -> Result<&str, usize> { + let lines: Vec<&str> = stdout + .lines() + .map(str::trim) + .filter(|l| !l.is_empty()) + .collect(); + match lines[..] { + [path] => Ok(path), + _ => Err(lines.len()), + } } /// `WriteNspawnFlags` — validate the container + every bind path + every @@ -2828,8 +2841,8 @@ mod tests { use super::{ BindMount, OwnedFd, PAUSED_MARKER_FILE, PrivRequest, check_fd_agreement, clear_runner_credentials, contains_secret_shaped_run, git_overlay_flags, - limits_dropin_body, redact_secret_line, remove_marker_in, toplevel_attr, - write_state_file_nofollow, + limits_dropin_body, redact_secret_line, remove_marker_in, single_output_path, + toplevel_attr, write_state_file_nofollow, }; use std::path::PathBuf; use std::sync::atomic::{AtomicU32, Ordering}; @@ -2855,6 +2868,31 @@ mod tests { ); } + /// `--print-out-paths`' exact per-shape table (measured against real + /// `rustc` semantics, not just reasoned about) — a lone `"\n"` and a + /// trailing-space path are the two shapes a bare `.lines().collect()` + /// gets wrong, both catchable only by trimming + dropping empties + /// before counting rather than after. + #[test] + fn single_output_path_rejects_blank_and_trims_whitespace() { + assert_eq!(single_output_path(""), Err(0)); + assert_eq!(single_output_path("\n"), Err(0)); + assert_eq!(single_output_path("\n\n"), Err(0)); + assert_eq!(single_output_path("/nix/store/abc\n"), Ok("/nix/store/abc")); + assert_eq!( + single_output_path("/nix/store/abc \n"), + Ok("/nix/store/abc") + ); + assert_eq!( + single_output_path("/nix/store/abc\r\n"), + Ok("/nix/store/abc") + ); + assert_eq!( + single_output_path("/nix/store/abc\n/nix/store/def\n"), + Err(2) + ); + } + /// Every bound git repo gets its `.git` overlaid — the knowledge tree /// and *each* config mount, an agent's own plus every child's. ///