fix(stibbons): harden labels sync arg handling and subprocess safety

crates/stibbons/src/labels/backend.rs

@@ -13,6 +13,7 @@
 use std::collections::BTreeMap;
 use std::process::Command;
 
+use super::exec::{self, DEFAULT_CLI_TIMEOUT, ExecError};
 use super::metadata::LabelDef;
 
 /// Read/write access to a repository's labels.
@@ -44,14 +45,23 @@ pub trait LabelBackend {
 ///
 /// Centralizes the spawn-failure vs non-zero-exit handling both backends need,
 /// and turns a missing binary into a clear "is it installed / authenticated"
-/// message rather than a raw `NotFound`.
+/// message rather than a raw `NotFound`. The call is bounded by
+/// [`DEFAULT_CLI_TIMEOUT`] so a wedged interactive re-auth or a stalled network
+/// call fails the run instead of hanging forever (see [`super::exec`]).
 fn run(program: &str, args: &[&str]) -> Result<Vec<u8>, Box<dyn std::error::Error>> {
-    let output = Command::new(program).args(args).output().map_err(|e| {
-        if e.kind() == std::io::ErrorKind::NotFound {
+    let mut cmd = Command::new(program);
+    cmd.args(args);
+    let output = exec::run_with_timeout(cmd, DEFAULT_CLI_TIMEOUT).map_err(|e| match e {
+        ExecError::Spawn(io) if io.kind() == std::io::ErrorKind::NotFound => {
             format!("`{program}` not found on PATH — install it or use the other platform")
-        } else {
-            format!("failed to run `{program}`: {e}")
         }
+        ExecError::Spawn(io) => format!("failed to run `{program}`: {io}"),
+        ExecError::Timeout(d) => format!(
+            "`{program} {}` timed out after {}s — is it waiting on an interactive prompt or a \
+             stalled network call?",
+            args.join(" "),
+            d.as_secs()
+        ),
     })?;
     if !output.status.success() {
         let stderr = String::from_utf8_lossy(&output.stderr);
@@ -89,38 +99,34 @@ impl LabelBackend for GithubCli {
         // --force makes create idempotent at the gh layer too, but we only call
         // it for genuinely-missing labels; kept off so a surprise collision is
         // surfaced rather than silently overwriting.
-        run(
-            "gh",
-            &[
-                "label",
-                "create",
-                &label.name,
-                "--color",
-                &label.color,
-                "--description",
-                &label.description,
-            ],
-        )?;
+        label.validate()?;
+        run_owned("gh", &github_args("create", label))?;
         Ok(())
     }
 
     fn update(&self, label: &LabelDef) -> Result<(), Box<dyn std::error::Error>> {
-        run(
-            "gh",
-            &[
-                "label",
-                "edit",
-                &label.name,
-                "--color",
-                &label.color,
-                "--description",
-                &label.description,
-            ],
-        )?;
+        label.validate()?;
+        run_owned("gh", &github_args("edit", label))?;
         Ok(())
     }
 }
 
+/// Build the `gh label {create|edit}` argv for a label.
+///
+/// Uses `=`-form flags for the color/description and a `--` terminator before
+/// the positional name, so even if validation is bypassed a value beginning
+/// with `-` can never be parsed by `gh` as an option (issue #694).
+fn github_args(verb: &str, label: &LabelDef) -> Vec<String> {
+    vec![
+        "label".into(),
+        verb.into(),
+        format!("--color={}", label.color),
+        format!("--description={}", label.description),
+        "--".into(),
+        label.name.clone(),
+    ]
+}
+
 /// GitLab backend using the `glab` CLI.
 #[derive(Debug, Default)]
 pub struct GitlabCli;
@@ -144,19 +150,8 @@ impl LabelBackend for GitlabCli {
     }
 
     fn create(&self, label: &LabelDef) -> Result<(), Box<dyn std::error::Error>> {
-        run(
-            "glab",
-            &[
-                "label",
-                "create",
-                "--name",
-                &label.name,
-                "--color",
-                &with_hash(&label.color),
-                "--description",
-                &label.description,
-            ],
-        )?;
+        label.validate()?;
+        run_owned("glab", &gitlab_create_args(label))?;
         Ok(())
     }
 
@@ -165,23 +160,42 @@ impl LabelBackend for GitlabCli {
         // name — so updates go through `glab label edit`. Its `--label-id`
         // accepts the label's name (GitLab's API identifies a label by title or
         // numeric id), so no separate id lookup is needed.
-        run(
-            "glab",
-            &[
-                "label",
-                "edit",
-                "--label-id",
-                &label.name,
-                "--color",
-                &with_hash(&label.color),
-                "--description",
-                &label.description,
-            ],
-        )?;
+        label.validate()?;
+        run_owned("glab", &gitlab_update_args(label))?;
         Ok(())
     }
 }
 
+/// Build the `glab label create` argv. `=`-form flags keep a leading-dash value
+/// from being parsed as an option (issue #694); colors get a leading `#`.
+fn gitlab_create_args(label: &LabelDef) -> Vec<String> {
+    vec![
+        "label".into(),
+        "create".into(),
+        format!("--name={}", label.name),
+        format!("--color={}", with_hash(&label.color)),
+        format!("--description={}", label.description),
+    ]
+}
+
+/// Build the `glab label edit` argv, identifying the label by name via
+/// `--label-id` (GitLab accepts a title or numeric id there).
+fn gitlab_update_args(label: &LabelDef) -> Vec<String> {
+    vec![
+        "label".into(),
+        "edit".into(),
+        format!("--label-id={}", label.name),
+        format!("--color={}", with_hash(&label.color)),
+        format!("--description={}", label.description),
+    ]
+}
+
+/// [`run`] for owned `String` args (the arg builders return `Vec<String>`).
+fn run_owned(program: &str, args: &[String]) -> Result<Vec<u8>, Box<dyn std::error::Error>> {
+    let borrowed: Vec<&str> = args.iter().map(String::as_str).collect();
+    run(program, &borrowed)
+}
+
 /// GitLab wants colors with a leading `#`; metadata may omit it.
 fn with_hash(color: &str) -> String {
     if color.starts_with('#') { color.to_string() } else { format!("#{color}") }
@@ -212,4 +226,64 @@ mod tests {
         assert_eq!(rows[0].name, "bug");
         assert_eq!(rows[0].description, "");
     }
+
+    fn label(name: &str, color: &str, desc: &str) -> LabelDef {
+        LabelDef { name: name.into(), color: color.into(), description: desc.into() }
+    }
+
+    #[test]
+    fn github_create_args_use_eq_flags_and_terminator() {
+        let l = label("status/in-progress", "0E8A16", "An agent is working");
+        assert_eq!(
+            github_args("create", &l),
+            vec![
+                "label",
+                "create",
+                "--color=0E8A16",
+                "--description=An agent is working",
+                "--",
+                "status/in-progress",
+            ]
+        );
+    }
+
+    #[test]
+    fn github_edit_args_use_edit_verb() {
+        let l = label("type/bug", "D73A4A", "Bug");
+        let args = github_args("edit", &l);
+        assert_eq!(args[0], "label");
+        assert_eq!(args[1], "edit");
+        // The `--` terminator precedes the positional name.
+        assert_eq!(args[args.len() - 2], "--");
+        assert_eq!(args[args.len() - 1], "type/bug");
+    }
+
+    #[test]
+    fn gitlab_create_args_prefix_color_hash_and_use_name_flag() {
+        let l = label("type/bug", "D73A4A", "Bug");
+        assert_eq!(
+            gitlab_create_args(&l),
+            vec!["label", "create", "--name=type/bug", "--color=#D73A4A", "--description=Bug"]
+        );
+    }
+
+    #[test]
+    fn gitlab_update_args_use_label_id_by_name() {
+        let l = label("type/bug", "#D73A4A", "Bug");
+        // Identifies the label by name via --label-id; color already has `#`.
+        assert_eq!(
+            gitlab_update_args(&l),
+            vec!["label", "edit", "--label-id=type/bug", "--color=#D73A4A", "--description=Bug"]
+        );
+    }
+
+    #[test]
+    fn eq_form_keeps_leading_dash_value_as_single_arg() {
+        // Even a (hypothetical, validation-bypassing) leading-dash description
+        // stays one `--description=…` token — never a separate flag.
+        let l = label("n", "0E8A16", "--repo=evil");
+        let args = github_args("create", &l);
+        assert!(args.iter().any(|a| a == "--description=--repo=evil"));
+        assert!(!args.iter().any(|a| a == "--repo=evil"));
+    }
 }

crates/stibbons/src/labels/exec.rs

@@ -0,0 +1,148 @@
+//! Bounded subprocess execution.
+//!
+//! The label sync shells out to `gh`/`glab`/`git`. Those CLIs can block
+//! indefinitely — an expired token triggers an interactive re-auth or browser
+//! flow, and a stalled network call never returns — which would hang
+//! `labels sync` / `setup` forever with no feedback. That is especially bad in
+//! CI, where there is no human to notice and Ctrl-C the process.
+//!
+//! [`run_with_timeout`] wraps [`std::process::Command`] with a deadline: it
+//! spawns the child, drains stdout/stderr on reader threads (so a chatty child
+//! can't deadlock on a full pipe), polls for exit against the deadline, and
+//! kills the child if the timeout expires. Standard library only — no extra
+//! dependency, so the workspace `Cargo.lock` is untouched.
+
+use std::io::{self, Read};
+use std::process::{Command, Output, Stdio};
+use std::time::{Duration, Instant};
+
+/// Default wall-clock limit for a single tracker/`git` CLI invocation.
+///
+/// Generous enough for a slow-but-live network round-trip, short enough that a
+/// wedged interactive prompt fails the run in a bounded time rather than
+/// hanging until the operator intervenes.
+pub const DEFAULT_CLI_TIMEOUT: Duration = Duration::from_mins(2);
+
+/// How often [`run_with_timeout`] polls the child for completion.
+const POLL_INTERVAL: Duration = Duration::from_millis(20);
+
+/// Why a bounded subprocess run failed.
+#[derive(Debug)]
+pub enum ExecError {
+    /// The child could not be spawned (e.g. the binary is not on `PATH`).
+    Spawn(io::Error),
+    /// The child was still running when the deadline elapsed and was killed.
+    Timeout(Duration),
+}
+
+impl std::fmt::Display for ExecError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            Self::Spawn(e) => write!(f, "{e}"),
+            Self::Timeout(d) => write!(f, "timed out after {}s", d.as_secs()),
+        }
+    }
+}
+
+impl std::error::Error for ExecError {}
+
+/// Run `cmd`, returning its [`Output`] or an [`ExecError`] if it cannot be
+/// spawned or does not finish within `timeout`.
+///
+/// stdin is set to null so a child that tries to prompt reads EOF and gives up
+/// instead of blocking on the terminal. stdout and stderr are captured on
+/// separate threads to avoid a pipe-buffer deadlock with a verbose child.
+///
+/// # Errors
+///
+/// Returns [`ExecError::Spawn`] if the process cannot start, or
+/// [`ExecError::Timeout`] if it is still running when `timeout` elapses (the
+/// child is killed and reaped before returning).
+pub fn run_with_timeout(mut cmd: Command, timeout: Duration) -> Result<Output, ExecError> {
+    cmd.stdin(Stdio::null()).stdout(Stdio::piped()).stderr(Stdio::piped());
+    let mut child = cmd.spawn().map_err(ExecError::Spawn)?;
+
+    // Take the pipes and drain each on its own thread. Holding onto the pipes
+    // and reading only after the child exits would deadlock if the child fills
+    // a pipe buffer and blocks waiting for us to read.
+    let stdout_reader = child.stdout.take().map(spawn_drain);
+    let stderr_reader = child.stderr.take().map(spawn_drain);
+
+    let deadline = Instant::now() + timeout;
+    let status = loop {
+        match child.try_wait() {
+            Ok(Some(status)) => break status,
+            Ok(None) => {
+                if Instant::now() >= deadline {
+                    // Best-effort kill + reap so we don't leak a zombie.
+                    let _ = child.kill();
+                    let _ = child.wait();
+                    return Err(ExecError::Timeout(timeout));
+                }
+                std::thread::sleep(POLL_INTERVAL);
+            }
+            Err(e) => return Err(ExecError::Spawn(e)),
+        }
+    };
+
+    let stdout = stdout_reader.map(join_drain).unwrap_or_default();
+    let stderr = stderr_reader.map(join_drain).unwrap_or_default();
+    Ok(Output { status, stdout, stderr })
+}
+
+/// Spawn a thread that reads a child pipe to EOF.
+fn spawn_drain<R: Read + Send + 'static>(mut reader: R) -> std::thread::JoinHandle<Vec<u8>> {
+    std::thread::spawn(move || {
+        let mut buf = Vec::new();
+        let _ = reader.read_to_end(&mut buf);
+        buf
+    })
+}
+
+/// Join a drain thread, treating a panicked reader as empty output.
+fn join_drain(handle: std::thread::JoinHandle<Vec<u8>>) -> Vec<u8> {
+    handle.join().unwrap_or_default()
+}
+
+#[cfg(all(test, unix))]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn quick_command_succeeds() {
+        let mut cmd = Command::new("echo");
+        cmd.arg("hello");
+        let out = run_with_timeout(cmd, Duration::from_secs(5)).unwrap();
+        assert!(out.status.success());
+        assert_eq!(String::from_utf8_lossy(&out.stdout).trim(), "hello");
+    }
+
+    #[test]
+    fn slow_command_times_out() {
+        let mut cmd = Command::new("sleep");
+        cmd.arg("5");
+        let start = Instant::now();
+        let err = run_with_timeout(cmd, Duration::from_millis(100)).unwrap_err();
+        assert!(matches!(err, ExecError::Timeout(_)), "expected timeout, got {err:?}");
+        // The kill must be prompt — nowhere near the full 5s sleep.
+        assert!(start.elapsed() < Duration::from_secs(2), "kill was not prompt");
+    }
+
+    #[test]
+    fn spawn_failure_is_reported() {
+        let cmd = Command::new("stibbons-no-such-binary-xyz");
+        let err = run_with_timeout(cmd, Duration::from_secs(5)).unwrap_err();
+        assert!(matches!(err, ExecError::Spawn(_)), "expected spawn error, got {err:?}");
+    }
+
+    #[test]
+    fn large_output_does_not_deadlock() {
+        // ~256 KiB of stdout — far more than a pipe buffer holds, so a
+        // read-after-exit implementation would deadlock here.
+        let mut cmd = Command::new("sh");
+        cmd.args(["-c", "head -c 262144 /dev/zero | tr '\\0' 'a'"]);
+        let out = run_with_timeout(cmd, Duration::from_secs(10)).unwrap();
+        assert!(out.status.success());
+        assert_eq!(out.stdout.len(), 262_144);
+    }
+}