From a757c90b7d2a735cad61505c3191eb8dda5b31e2 Mon Sep 17 00:00:00 2001 From: t Date: Sun, 5 Jul 2026 14:05:26 -0500 Subject: [PATCH] fix(stibbons): harden labels sync arg handling and subprocess safety Deferred hardening follow-ups from the adversarial review of #289 / PR #693, all confined to crates/stibbons/src/labels/ and shipped together. Arg-injection hardening (#694): - LabelDef::validate() rejects name/color/description beginning with `-` (which gh/glab could parse as a flag) and enforces a 6-hex-digit color. - Backends build argv with `=`-form flags (--color=..., --description=...) and, for gh, a `--` terminator before the positional name, so a leading-dash value can never be parsed as an option even if validation is bypassed. Reliability guards (#695): - New exec::run_with_timeout wraps gh/glab/git in a std-only deadline (spawn + thread-drained pipes + try_wait poll + kill), so a wedged interactive re-auth or stalled network fails the run instead of hanging. backend::run() and platform::origin_remote_url() route through it. - find_metadata_files tracks canonicalized visited dirs and caps recursion depth, terminating on directory symlink cycles instead of exhausting fds/stack; oversized metadata.yml is skipped with a warning before parsing. Test coverage (#696): - gh/glab create/update arg construction asserted (incl. GitLab --label-id update path and the gh `--` terminator); validate() accept/reject cases. - classify_remote bare-substring fallback branch; resolve_skill_roots empty-roots error and default_skill_roots existence; exec timeout/success. - Hermetic integration tests for `stibbons setup` real execution and the orphaned-reference warning reaching CLI stderr. No new dependency (Cargo.lock untouched); no behavior change for well-formed metadata. Closes #694 Closes #695 Closes #696 Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/stibbons/src/labels/backend.rs | 184 ++++++++++++++------ crates/stibbons/src/labels/exec.rs | 148 ++++++++++++++++ crates/stibbons/src/labels/metadata.rs | 184 +++++++++++++++++++- crates/stibbons/src/labels/mod.rs | 65 ++++++- crates/stibbons/src/labels/platform.rs | 30 +++- crates/stibbons/tests/labels_integration.rs | 45 +++++ 6 files changed, 585 insertions(+), 71 deletions(-) create mode 100644 crates/stibbons/src/labels/exec.rs diff --git a/crates/stibbons/src/labels/backend.rs b/crates/stibbons/src/labels/backend.rs index 43b97e63..0e7a1612 100644 --- a/crates/stibbons/src/labels/backend.rs +++ b/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, Box> { - 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> { - 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 { + 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> { - 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 { + 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 { + 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`). +fn run_owned(program: &str, args: &[String]) -> Result, Box> { + 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")); + } } diff --git a/crates/stibbons/src/labels/exec.rs b/crates/stibbons/src/labels/exec.rs new file mode 100644 index 00000000..61ab950a --- /dev/null +++ b/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 { + 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(mut reader: R) -> std::thread::JoinHandle> { + 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 { + 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); + } +} diff --git a/crates/stibbons/src/labels/metadata.rs b/crates/stibbons/src/labels/metadata.rs index c01e621d..1f1ecb55 100644 --- a/crates/stibbons/src/labels/metadata.rs +++ b/crates/stibbons/src/labels/metadata.rs @@ -24,9 +24,23 @@ //! sync with a warning — otherwise a name-only entry would push an empty color //! onto the tracker and blank out the real label. -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; use std::path::{Path, PathBuf}; +/// Maximum directory depth [`find_metadata_files`] will descend. +/// +/// Skill roots are shallow (`skills//metadata.yml` or +/// `plugins//skills//metadata.yml`), so a real tree never nears +/// this. It is a backstop against a pathological or hostile directory tree. +const MAX_WALK_DEPTH: usize = 32; + +/// Maximum size of a `metadata.yml` we will read and parse (256 KiB). +/// +/// A real skill metadata file is well under a kilobyte; anything this large is +/// either corrupt or hostile, and parsing it just wastes memory. Oversized +/// files are skipped with a warning rather than parsed. +const MAX_METADATA_BYTES: u64 = 256 * 1024; + /// A single label definition as read from a skill `metadata.yml`. #[derive(Debug, Clone, PartialEq, Eq, serde::Deserialize)] pub struct LabelDef { @@ -40,6 +54,49 @@ pub struct LabelDef { pub description: String, } +impl LabelDef { + /// Validate the fields before they are handed to a tracker CLI as argv. + /// + /// `metadata.yml` files come from the externally-populated + /// `/opt/librarian/plugins` tree and any `--skills-dir` the caller passes, + /// so their contents are not fully trusted. `Command::args` avoids the + /// shell, but a value beginning with `-` could still be parsed as a *flag* + /// by the downstream `gh`/`glab` (classic argument injection), and `color` + /// should be a hex string. The backends additionally use `=`-form flags and + /// a `--` terminator, so this is a defense-in-depth check. + /// + /// # Errors + /// + /// Returns a human-readable message when `name`, `color`, or `description` + /// begins with `-`, or when `color` is not a 6-digit hex string (with an + /// optional leading `#`). + pub(crate) fn validate(&self) -> Result<(), String> { + for (field, value) in + [("name", &self.name), ("color", &self.color), ("description", &self.description)] + { + if value.starts_with('-') { + return Err(format!( + "label {field} `{value}` starts with `-`; refusing to pass it to the tracker \ + CLI where it could be parsed as a flag" + )); + } + } + if !is_hex_color(&self.color) { + return Err(format!( + "label `{}` has invalid color `{}` (expected 6 hex digits, optional leading `#`)", + self.name, self.color + )); + } + Ok(()) + } +} + +/// True when `s` is a 6-digit hex color with an optional leading `#`. +fn is_hex_color(s: &str) -> bool { + let hex = s.strip_prefix('#').unwrap_or(s); + hex.len() == 6 && hex.bytes().all(|b| b.is_ascii_hexdigit()) +} + /// The subset of a skill `metadata.yml` we parse. Everything except `labels` /// is ignored (serde drops unknown fields by default). #[derive(Debug, Default, serde::Deserialize)] @@ -65,20 +122,42 @@ pub struct Aggregate { /// `plugins//skills//metadata.yml`), so a simple recursive walk /// is fine. Unreadable directories are skipped silently — a permission blip on /// one subtree shouldn't abort the whole sync. +/// +/// A directory symlink cycle (possible under the externally-populated +/// `/opt/librarian/plugins` or an attacker-controlled `--skills-dir`) would +/// otherwise recurse until stack or fd exhaustion, so the walk tracks the +/// canonical path of every directory it enters and skips any it has already +/// visited, and caps recursion at [`MAX_WALK_DEPTH`] as a final backstop. fn find_metadata_files(root: &Path) -> Vec { let mut out = Vec::new(); - let Ok(entries) = std::fs::read_dir(root) else { - return out; + let mut visited = std::collections::BTreeSet::new(); + walk(root, 0, &mut visited, &mut out); + out +} + +/// Recursion helper for [`find_metadata_files`], carrying cycle-detection and +/// depth-limit state. +fn walk(dir: &Path, depth: usize, visited: &mut BTreeSet, out: &mut Vec) { + if depth > MAX_WALK_DEPTH { + return; + } + // Canonicalize so two paths reaching the same directory via a symlink + // resolve to one key; if canonicalization fails, fall back to the raw path. + let key = std::fs::canonicalize(dir).unwrap_or_else(|_| dir.to_path_buf()); + if !visited.insert(key) { + return; // already walked this directory — a symlink cycle. + } + let Ok(entries) = std::fs::read_dir(dir) else { + return; }; for entry in entries.flatten() { let path = entry.path(); if path.is_dir() { - out.extend(find_metadata_files(&path)); + walk(&path, depth + 1, visited, out); } else if path.file_name().is_some_and(|n| n == "metadata.yml") { out.push(path); } } - out } /// Load and aggregate labels from every `metadata.yml` under the given roots. @@ -111,6 +190,20 @@ pub fn load_labels(roots: &[PathBuf]) -> Result = std::collections::BTreeSet::new(); for file in &files { + // Skip an implausibly large file rather than reading it into memory and + // handing it to the YAML parser — corrupt or hostile input shouldn't be + // able to make the sync allocate unboundedly. + if let Ok(meta_fs) = std::fs::metadata(file) + && meta_fs.len() > MAX_METADATA_BYTES + { + warnings.push(format!( + "skipping {} — {} bytes exceeds the {} byte metadata limit", + file.display(), + meta_fs.len(), + MAX_METADATA_BYTES + )); + continue; + } let text = std::fs::read_to_string(file)?; let meta: SkillMetadata = serde_yaml::from_str(&text) .map_err(|e| format!("failed to parse {}: {e}", file.display()))?; @@ -311,4 +404,85 @@ mod tests { assert_eq!(agg.labels.len(), 1); assert_eq!(agg.labels[0].name, "status/on-hold"); } + + #[test] + fn validate_accepts_well_formed() { + let ok = LabelDef { + name: "status/in-progress".into(), + color: "0E8A16".into(), + description: "An agent is working".into(), + }; + // A leading `#` on the color is allowed. + let hashed = LabelDef { color: "#0E8A16".into(), ..ok.clone() }; + assert!(ok.validate().is_ok()); + assert!(hashed.validate().is_ok()); + } + + #[test] + fn validate_rejects_leading_dash_fields() { + for bad in [ + LabelDef { name: "--force".into(), color: "0E8A16".into(), description: "x".into() }, + LabelDef { name: "n".into(), color: "-0E8A16".into(), description: "x".into() }, + LabelDef { + name: "n".into(), + color: "0E8A16".into(), + description: "--repo=evil".into(), + }, + ] { + assert!(bad.validate().is_err(), "should reject leading-dash field: {bad:?}"); + } + } + + #[test] + fn validate_rejects_malformed_color() { + for color in ["", "0E8A1", "0E8A16Z", "not-a-color", "#12345"] { + let l = LabelDef { name: "n".into(), color: color.into(), description: "x".into() }; + assert!(l.validate().is_err(), "should reject color `{color}`"); + } + } + + #[test] + #[cfg(unix)] + fn symlink_cycle_terminates_and_still_finds_real_files() { + use std::os::unix::fs::symlink; + let tmp = TempDir::new().unwrap(); + // A real skill with a label... + write_skill( + tmp.path(), + "real", + "labels:\n - name: type/bug\n color: \"D73A4A\"\n description: Bug\n", + ); + // ...plus a directory that symlinks back to the root, forming a cycle. + let loop_dir = tmp.path().join("loop"); + fs::create_dir_all(&loop_dir).unwrap(); + symlink(tmp.path(), loop_dir.join("back")).unwrap(); + + // If the walk didn't detect the cycle this would recurse until fd/stack + // exhaustion; instead it terminates and still returns the real label. + let agg = load_labels(&[tmp.path().to_path_buf()]).unwrap(); + assert_eq!(agg.labels.len(), 1); + assert_eq!(agg.labels[0].name, "type/bug"); + } + + #[test] + fn oversized_metadata_is_skipped_with_warning() { + let tmp = TempDir::new().unwrap(); + // A valid-looking file padded past the size cap with a huge comment. + let padding = "#".repeat(usize::try_from(MAX_METADATA_BYTES).unwrap() + 1); + write_skill( + tmp.path(), + "huge", + &format!( + "{padding}\nlabels:\n - name: type/bug\n color: \"D73A4A\"\n description: Bug\n" + ), + ); + let agg = load_labels(&[tmp.path().to_path_buf()]).unwrap(); + assert!(agg.labels.is_empty(), "oversized file must not contribute labels"); + assert_eq!(agg.source_files, 0); + assert!( + agg.warnings.iter().any(|w| w.contains("exceeds") && w.contains("metadata limit")), + "expected an oversize warning, got: {:?}", + agg.warnings + ); + } } diff --git a/crates/stibbons/src/labels/mod.rs b/crates/stibbons/src/labels/mod.rs index c546c192..deac0618 100644 --- a/crates/stibbons/src/labels/mod.rs +++ b/crates/stibbons/src/labels/mod.rs @@ -11,6 +11,7 @@ //! `gh`/`glab` CLIs behind a trait. This module wires them together. mod backend; +mod exec; mod metadata; mod plan; mod platform; @@ -59,6 +60,31 @@ fn default_skill_roots() -> Vec { candidates.into_iter().filter(|p| p.exists()).collect() } +/// Resolve the skill roots to scan: the caller's `--skills-dir` values when any +/// were given, otherwise `defaults`. Errors when the result is empty (nothing +/// to read labels from). +/// +/// Split out (with `defaults` injected rather than calling +/// [`default_skill_roots`] directly) so the empty-roots error branch is +/// unit-testable without depending on whether the host has +/// `/opt/librarian/plugins` installed. +/// +/// # Errors +/// +/// Returns an error when both `skills_dirs` and `defaults` are empty. +fn resolve_skill_roots( + skills_dirs: &[PathBuf], + defaults: Vec, +) -> Result, Box> { + let roots = if skills_dirs.is_empty() { defaults } else { skills_dirs.to_vec() }; + if roots.is_empty() { + return Err("no skill directories found to read labels from; \ + pass --skills-dir " + .into()); + } + Ok(roots) +} + /// Run the label sync. /// /// # Errors @@ -80,13 +106,7 @@ pub fn run_sync(opts: &SyncOptions) -> Result<(), Box> { }; // 2. Resolve skill roots and aggregate labels. - let roots = - if opts.skills_dirs.is_empty() { default_skill_roots() } else { opts.skills_dirs.clone() }; - if roots.is_empty() { - return Err("no skill directories found to read labels from; \ - pass --skills-dir " - .into()); - } + let roots = resolve_skill_roots(&opts.skills_dirs, default_skill_roots())?; let agg = metadata::load_labels(&roots)?; for warning in &agg.warnings { eprintln!("warning: {warning}"); @@ -201,6 +221,37 @@ mod tests { assert!(err.to_string().contains("unknown platform")); } + #[test] + fn resolve_skill_roots_prefers_explicit_dirs() { + let explicit = vec![PathBuf::from("/tmp/a"), PathBuf::from("/tmp/b")]; + // Explicit dirs are used verbatim and defaults are ignored. + let roots = resolve_skill_roots(&explicit, vec![PathBuf::from("/default")]).unwrap(); + assert_eq!(roots, explicit); + } + + #[test] + fn resolve_skill_roots_falls_back_to_defaults() { + let defaults = vec![PathBuf::from("/opt/librarian/plugins")]; + let roots = resolve_skill_roots(&[], defaults.clone()).unwrap(); + assert_eq!(roots, defaults); + } + + #[test] + fn resolve_skill_roots_empty_is_error() { + // No explicit dirs and no defaults (e.g. a host without librarian) is + // the "no skill directories found" branch. + let err = resolve_skill_roots(&[], vec![]).unwrap_err(); + assert!(err.to_string().contains("no skill directories found"), "got: {err}"); + } + + #[test] + fn default_skill_roots_returns_only_existing_paths() { + // Whatever it returns, every entry must actually exist on disk. + for root in default_skill_roots() { + assert!(root.exists(), "default_skill_roots returned a nonexistent path: {root:?}"); + } + } + fn label(name: &str, color: &str, desc: &str) -> LabelDef { LabelDef { name: name.into(), color: color.into(), description: desc.into() } } diff --git a/crates/stibbons/src/labels/platform.rs b/crates/stibbons/src/labels/platform.rs index 07dcf20f..35122310 100644 --- a/crates/stibbons/src/labels/platform.rs +++ b/crates/stibbons/src/labels/platform.rs @@ -7,6 +7,8 @@ use std::fmt; use std::process::Command; +use super::exec::{self, DEFAULT_CLI_TIMEOUT, ExecError}; + /// Which issue tracker a repo is hosted on. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Platform { @@ -84,10 +86,15 @@ fn host_of(url: &str) -> Option<&str> { /// Returns an error when `git` cannot be spawned or the command fails (e.g. not /// a git repo, or no `origin` remote configured). pub fn origin_remote_url() -> Result> { - let output = Command::new("git") - .args(["remote", "get-url", "origin"]) - .output() - .map_err(|e| format!("failed to run git: {e}"))?; + let mut cmd = Command::new("git"); + cmd.args(["remote", "get-url", "origin"]); + // Bounded so a wedged git (e.g. a credential prompt) can't hang detection. + let output = exec::run_with_timeout(cmd, DEFAULT_CLI_TIMEOUT).map_err(|e| match e { + ExecError::Spawn(io) => format!("failed to run git: {io}"), + ExecError::Timeout(d) => { + format!("`git remote get-url origin` timed out after {}s", d.as_secs()) + } + })?; if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr); return Err(format!("git remote get-url origin failed: {}", stderr.trim()).into()); @@ -132,6 +139,21 @@ mod tests { assert_eq!(classify_remote("https://example.com/x/y.git"), None); } + #[test] + fn bare_substring_host_hits_fallback_branch() { + // Host is neither `github.com` nor `ghe.`/`.ghe.`-prefixed and doesn't + // start with `gitlab.`, so it only matches via the bare-substring + // fallback (`host.contains("github")`). + assert_eq!( + classify_remote("git@mycompany-github.example.com:o/r.git"), + Some(Platform::GitHub) + ); + assert_eq!( + classify_remote("https://internal-gitlab-host.example.com/o/r.git"), + Some(Platform::GitLab) + ); + } + #[test] fn ssh_url_scheme_form() { assert_eq!( diff --git a/crates/stibbons/tests/labels_integration.rs b/crates/stibbons/tests/labels_integration.rs index 3a31ba22..efb54209 100644 --- a/crates/stibbons/tests/labels_integration.rs +++ b/crates/stibbons/tests/labels_integration.rs @@ -79,6 +79,51 @@ fn setup_command_exists() { assert!(stdout.contains("--platform")); } +#[test] +fn setup_real_execution_reports_complete() { + // Exercise `run_setup` itself (not just --help): an empty label set makes + // the underlying sync short-circuit at "Nothing to sync" before any + // gh/glab call, so this stays hermetic while still reaching the + // "Setup complete." tail that only real execution prints. + let tmp = TempDir::new().unwrap(); + let skills = tmp.path().join("skills"); + fs::create_dir_all(&skills).unwrap(); + write_skill(&skills, "docker-development", "name: docker-development\nlabels: []\n"); + + let out = run( + tmp.path(), + &["setup", "--platform", "github", "--skills-dir", skills.to_str().unwrap()], + ); + assert!(out.status.success(), "stderr: {}", String::from_utf8_lossy(&out.stderr)); + let stdout = String::from_utf8_lossy(&out.stdout); + assert!(stdout.contains("Nothing to sync"), "got: {stdout}"); + assert!(stdout.contains("Setup complete. (Future steps:"), "got: {stdout}"); +} + +#[test] +fn orphaned_reference_warning_reaches_stderr() { + // A name-only label (no color) is a *reference*, never a definition. When a + // name is only ever referenced, load_labels emits a "never defined" warning + // and drops it — so the desired set is empty and sync short-circuits at + // "Nothing to sync" (hermetic, no backend). This asserts that module-level + // warning is wired through to the CLI's stderr. + let tmp = TempDir::new().unwrap(); + let skills = tmp.path().join("skills"); + fs::create_dir_all(&skills).unwrap(); + write_skill(&skills, "orchestrate", "labels:\n - name: status/orphan\n"); + + let out = run( + tmp.path(), + &["labels", "sync", "--platform", "github", "--skills-dir", skills.to_str().unwrap()], + ); + assert!(out.status.success(), "stderr: {}", String::from_utf8_lossy(&out.stderr)); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!(stderr.contains("warning:"), "expected a warning on stderr, got: {stderr}"); + assert!(stderr.contains("never defined"), "expected orphan warning, got: {stderr}"); + let stdout = String::from_utf8_lossy(&out.stdout); + assert!(stdout.contains("Nothing to sync"), "got: {stdout}"); +} + #[test] fn empty_skills_dir_reports_nothing_to_sync() { let tmp = TempDir::new().unwrap();