hive-priv: filter/trim before counting output-path lines, fix log level

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).

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.
     ///