From 4ece7deca34af74bb9f217e2d53c04dd2146086b Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 17 May 2026 21:02:42 +0100 Subject: [PATCH 01/14] feat(cli): add ado-aw disable Squash-merge of #591 (feat/cli-disable). See original PR for review history and detailed rationale. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- AGENTS.md | 1 + docs/cli.md | 7 + src/ado/mod.rs | 9 + src/disable.rs | 341 +++++++++++++++++++++++++++++++++++ src/main.rs | 45 +++++ tests/disable_integration.rs | 47 +++++ 6 files changed, 450 insertions(+) create mode 100644 src/disable.rs create mode 100644 tests/disable_integration.rs diff --git a/AGENTS.md b/AGENTS.md index 0f888099..36004234 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -77,6 +77,7 @@ Every compiled pipeline runs as three sequential jobs: │ ├── mcp.rs # SafeOutputs MCP server (stdio + HTTP) │ ├── configure.rs # `configure` CLI command — orchestration shim atop `src/ado/` │ ├── enable.rs # `enable` CLI command — registers ADO build definitions for compiled pipelines and ensures they are enabled +│ ├── disable.rs # `disable` CLI command — sets queueStatus to disabled (default) or paused on matched definitions │ ├── ado/ # Shared Azure DevOps REST helpers (auth, list/match/PATCH/POST) │ │ └── mod.rs # Used by `configure` and the `enable` command (ADO REST helpers: auth, list/match/PATCH/POST) │ ├── detect.rs # Agentic pipeline detection (helper for `configure`) diff --git a/docs/cli.md b/docs/cli.md index e4144da3..f0ad6a55 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -55,3 +55,10 @@ Global flags (apply to all subcommands): `--verbose, -v` (enable info-level logg - `--token ` - The token value for `--also-set-token`. Falls back to `$GITHUB_TOKEN`, then to an interactive prompt. Requires `--also-set-token`. **Source-repo scope (Phase 1):** `enable` requires the local git remote to be an Azure DevOps Git remote (the source repo is what gets registered as the definition's repository). GitHub-hosted source repos are gated on a follow-up. + +- `disable [PATH]` - Set `queueStatus` to `disabled` (default) or `paused` on every ADO build definition that matches a local fixture under `PATH`. Refuses to touch any ADO definition that is not the target of a local fixture match — that safety property falls naturally out of the same yaml-path + name match used by `configure`. Skips definitions that are already at the requested status; fail-soft per fixture; exits non-zero if any patch failed or if zero local fixtures matched ADO definitions. + - `--org ` - Override: Azure DevOps organization (URL or bare org name). Inferred from git remote by default. + - `--project ` - Override: Azure DevOps project name (inferred from git remote by default). + - `--pat ` / `AZURE_DEVOPS_EXT_PAT` env var - PAT for ADO API authentication (Azure CLI fallback if omitted). + - `--paused` - Use `queueStatus: paused` instead of `disabled`. Paused definitions still queue scheduled runs but the queue is held; disabled definitions reject all queue requests. + - `--dry-run` - Print the planned `from → to` transitions without calling the ADO API. diff --git a/src/ado/mod.rs b/src/ado/mod.rs index d7793ddb..81bcf5d8 100644 --- a/src/ado/mod.rs +++ b/src/ado/mod.rs @@ -238,6 +238,12 @@ pub struct MatchedDefinition { pub name: String, pub match_method: MatchMethod, pub yaml_path: String, + /// `enabled`, `disabled`, `paused`, or `None` when the matcher + /// couldn't read the field (explicit-ID matches, older API + /// responses). Populated from `DefinitionSummary::queue_status` + /// when available, so command-level decision logic can skip + /// already-at-target definitions without an extra HTTP round-trip. + pub queue_status: Option, } /// List all build definitions in the project, handling pagination. @@ -425,6 +431,7 @@ pub async fn match_definitions( name: def.name.clone(), match_method: MatchMethod::YamlPath, yaml_path: yaml_path_normalized.to_string(), + queue_status: def.queue_status.clone(), }); continue; } @@ -448,6 +455,7 @@ pub async fn match_definitions( name: def.name.clone(), match_method: MatchMethod::PipelineName, yaml_path: yaml_path_normalized.to_string(), + queue_status: def.queue_status.clone(), }); continue; } @@ -752,6 +760,7 @@ pub async fn resolve_definitions( name, match_method: MatchMethod::Explicit, yaml_path: String::new(), + queue_status: None, }); } return Ok(Some(matched)); diff --git a/src/disable.rs b/src/disable.rs new file mode 100644 index 00000000..e8ebc186 --- /dev/null +++ b/src/disable.rs @@ -0,0 +1,341 @@ +//! The `disable` CLI command. +//! +//! Sets `queueStatus` to `disabled` (default) or `paused` on every ADO +//! build definition matched against a local fixture. Phase 1 of the +//! pipeline-lifecycle CLI family — see `docs/cli.md`. +//! +//! Scope (Phase 1): +//! +//! - Only touches ADO definitions that map to a local fixture. This +//! safety property falls naturally out of [`match_definitions`] — +//! definitions without a local fixture are never in the returned +//! set. +//! - No-op (skip) when the current `queueStatus` already matches the +//! target. + +use anyhow::{Context, Result}; +use log::debug; +use std::path::{Path, PathBuf}; + +use crate::ado::{ + MatchedDefinition, match_definitions, patch_queue_status, resolve_ado_context, resolve_auth, +}; +use crate::detect; + +/// Which `queueStatus` value the operator wants to land on. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Target { + Disabled, + Paused, +} + +impl Target { + pub fn as_str(&self) -> &'static str { + match self { + Target::Disabled => "disabled", + Target::Paused => "paused", + } + } +} + +/// Outcome of inspecting one matched definition against the operator's +/// requested target. +/// +/// Pure data — no HTTP, no auth, no IO. Built by [`decide_action`] so +/// the decision logic can be exercised without touching the network. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Action { + /// Already at the requested status; nothing to do. + Skip { id: u64, name: String, reason: String }, + /// `queueStatus` needs to be patched. + Patch { + id: u64, + name: String, + from: String, + to: &'static str, + }, +} + +/// Pure function: decide what to do for one matched definition. +/// +/// - `Skip` when the current status equals the target. +/// - `Patch` otherwise — including the "current status is unknown" +/// case (older API responses, explicit-ID matches), since the safe +/// default is to apply the patch and let ADO reject if appropriate. +pub fn decide_action(matched: &MatchedDefinition, target: Target) -> Action { + let target_str = target.as_str(); + let current = matched.queue_status.as_deref().unwrap_or(""); + + if current == target_str { + return Action::Skip { + id: matched.id, + name: matched.name.clone(), + reason: format!("already {}", target_str), + }; + } + + Action::Patch { + id: matched.id, + name: matched.name.clone(), + from: if current.is_empty() { + "unknown".to_string() + } else { + current.to_string() + }, + to: target_str, + } +} + +/// CLI options for [`run`]. +pub struct DisableOptions<'a> { + pub org: Option<&'a str>, + pub project: Option<&'a str>, + pub pat: Option<&'a str>, + pub path: Option<&'a Path>, + pub paused: bool, + pub dry_run: bool, +} + +/// Run the `disable` command. +pub async fn run(opts: DisableOptions<'_>) -> Result<()> { + let repo_path: PathBuf = match opts.path { + Some(p) => tokio::fs::canonicalize(p) + .await + .with_context(|| format!("Could not resolve path: {}", p.display()))?, + None => tokio::fs::canonicalize(".") + .await + .context("Could not resolve current directory")?, + }; + + let target = if opts.paused { + Target::Paused + } else { + Target::Disabled + }; + + let auth = resolve_auth(opts.pat).await?; + let ado_ctx = resolve_ado_context(&repo_path, opts.org, opts.project).await?; + + println!( + "ADO context: org={}, project={}", + ado_ctx.org_url, ado_ctx.project + ); + println!("Target queueStatus: {}", target.as_str()); + println!(); + + println!("Scanning for agentic pipelines..."); + let detected = detect::detect_pipelines(&repo_path).await?; + if detected.is_empty() { + anyhow::bail!( + "No local agentic pipeline fixtures were found under {}. \ + Run `ado-aw compile` first (or point `ado-aw disable` at the repo root).", + repo_path.display() + ); + } + println!("Found {} agentic pipeline(s).", detected.len()); + println!(); + + let client = reqwest::Client::builder() + .timeout(std::time::Duration::from_secs(30)) + .build() + .context("Failed to create HTTP client")?; + + println!("Matching to Azure DevOps pipeline definitions..."); + let matched = match_definitions(&client, &ado_ctx, &auth, &detected).await?; + + if matched.is_empty() { + anyhow::bail!( + "No ADO definitions matched any local fixture. Run `ado-aw list` to \ + diagnose: either the fixtures haven't been registered with `ado-aw \ + enable`, or the local yaml paths and ADO `yamlFilename` values don't \ + line up." + ); + } + + println!("{} definition(s) matched.", matched.len()); + println!(); + + let mut patched = 0usize; + let mut skipped = 0usize; + let mut failure = 0usize; + for m in &matched { + let action = decide_action(m, target); + debug!("definition {}: action={:?}", m.id, action); + + match action { + Action::Skip { id, name, reason } => { + println!("↻ skip: {} (id={}, {})", name, id, reason); + skipped += 1; + } + Action::Patch { id, name, from, to } => { + if opts.dry_run { + println!( + "[dry-run] ▶ would patch: {} (id={}, {} → {})", + name, id, from, to + ); + patched += 1; + continue; + } + match patch_queue_status(&client, &ado_ctx, &auth, id, to).await { + Ok(()) => { + println!("▶ patched: {} (id={}, {} → {})", name, id, from, to); + patched += 1; + } + Err(e) => { + eprintln!("✗ failed: {} (id={}): {:#}", name, id, e); + failure += 1; + } + } + } + } + } + + println!(); + println!( + "Done: {} patched, {} skipped, {} failed.", + patched, skipped, failure + ); + if failure > 0 { + anyhow::bail!("{} definition(s) failed", failure); + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::ado::MatchMethod; + + fn matched_with_status(id: u64, name: &str, status: Option<&str>) -> MatchedDefinition { + MatchedDefinition { + id, + name: name.to_string(), + match_method: MatchMethod::YamlPath, + yaml_path: format!("/tests/{}.lock.yml", name.replace(' ', "-")), + queue_status: status.map(str::to_string), + } + } + + #[test] + fn target_as_str_disabled_and_paused() { + assert_eq!(Target::Disabled.as_str(), "disabled"); + assert_eq!(Target::Paused.as_str(), "paused"); + } + + // ============ decide_action matrix ============ + + #[test] + fn enabled_to_disabled_patches() { + let m = matched_with_status(1, "noop", Some("enabled")); + let action = decide_action(&m, Target::Disabled); + assert_eq!( + action, + Action::Patch { + id: 1, + name: "noop".to_string(), + from: "enabled".to_string(), + to: "disabled" + } + ); + } + + #[test] + fn enabled_to_paused_patches() { + let m = matched_with_status(2, "noop", Some("enabled")); + let action = decide_action(&m, Target::Paused); + assert_eq!( + action, + Action::Patch { + id: 2, + name: "noop".to_string(), + from: "enabled".to_string(), + to: "paused" + } + ); + } + + #[test] + fn disabled_to_disabled_skips() { + let m = matched_with_status(3, "noop", Some("disabled")); + let action = decide_action(&m, Target::Disabled); + assert_eq!( + action, + Action::Skip { + id: 3, + name: "noop".to_string(), + reason: "already disabled".to_string() + } + ); + } + + #[test] + fn paused_to_paused_skips() { + let m = matched_with_status(4, "noop", Some("paused")); + let action = decide_action(&m, Target::Paused); + assert_eq!( + action, + Action::Skip { + id: 4, + name: "noop".to_string(), + reason: "already paused".to_string() + } + ); + } + + #[test] + fn disabled_to_paused_patches() { + let m = matched_with_status(5, "noop", Some("disabled")); + let action = decide_action(&m, Target::Paused); + assert_eq!( + action, + Action::Patch { + id: 5, + name: "noop".to_string(), + from: "disabled".to_string(), + to: "paused" + } + ); + } + + #[test] + fn paused_to_disabled_patches() { + let m = matched_with_status(6, "noop", Some("paused")); + let action = decide_action(&m, Target::Disabled); + assert_eq!( + action, + Action::Patch { + id: 6, + name: "noop".to_string(), + from: "paused".to_string(), + to: "disabled" + } + ); + } + + #[test] + fn unknown_status_patches_with_from_unknown() { + // Explicit-ID matches and older API responses have queue_status = None. + // The safe default is to apply the patch and let ADO reject if needed. + let m = matched_with_status(7, "noop", None); + let action = decide_action(&m, Target::Disabled); + assert_eq!( + action, + Action::Patch { + id: 7, + name: "noop".to_string(), + from: "unknown".to_string(), + to: "disabled" + } + ); + } + + #[test] + fn empty_status_string_treated_as_unknown() { + let m = matched_with_status(8, "noop", Some("")); + let action = decide_action(&m, Target::Disabled); + match action { + Action::Patch { from, .. } => assert_eq!(from, "unknown"), + other => panic!("expected Patch, got {:?}", other), + } + } +} diff --git a/src/main.rs b/src/main.rs index 9d19f316..a18d3387 100644 --- a/src/main.rs +++ b/src/main.rs @@ -4,6 +4,7 @@ pub mod ado; mod compile; mod configure; mod detect; +mod disable; mod ecosystem_domains; mod enable; mod engine; @@ -174,6 +175,31 @@ enum Commands { #[arg(long, requires = "also_set_token")] token: Option, }, + /// Disable (or pause) every ADO build definition that matches a local fixture. + Disable { + /// Path to the repository root (defaults to current directory). Used + /// to auto-discover compiled pipelines, same as `compile`. + path: Option, + /// Override: Azure DevOps organization (URL like `https://dev.azure.com/myorg`, + /// or just the org name `myorg`). Inferred from git remote by default. + #[arg(long)] + org: Option, + /// Override: Azure DevOps project name (inferred from git remote by default). + #[arg(long)] + project: Option, + /// PAT for ADO API authentication (prefer setting AZURE_DEVOPS_EXT_PAT env var; + /// Azure CLI fallback if omitted). + #[arg(long, env = "AZURE_DEVOPS_EXT_PAT")] + pat: Option, + /// Set queueStatus to `paused` instead of `disabled`. Paused + /// definitions still queue scheduled runs but the queue is held; + /// disabled definitions reject all queue requests. + #[arg(long)] + paused: bool, + /// Preview the planned actions without calling the ADO API. + #[arg(long)] + dry_run: bool, + }, } #[derive(Parser, Debug)] @@ -524,6 +550,7 @@ async fn main() -> Result<()> { Some(Commands::Init { .. }) => "init", Some(Commands::Configure { .. }) => "configure", Some(Commands::Enable { .. }) => "enable", + Some(Commands::Disable { .. }) => "disable", None => "ado-aw", }; @@ -656,6 +683,24 @@ async fn main() -> Result<()> { }) .await?; } + Commands::Disable { + path, + org, + project, + pat, + paused, + dry_run, + } => { + disable::run(disable::DisableOptions { + org: org.as_deref(), + project: project.as_deref(), + pat: pat.as_deref(), + path: path.as_deref(), + paused, + dry_run, + }) + .await?; + } } Ok(()) } diff --git a/tests/disable_integration.rs b/tests/disable_integration.rs new file mode 100644 index 00000000..46b89e8e --- /dev/null +++ b/tests/disable_integration.rs @@ -0,0 +1,47 @@ +//! Integration tests for `ado-aw disable`. +//! +//! These tests exercise the compiled binary at the CLI surface level — +//! `--help` output and clap-level validation — without driving real +//! HTTP traffic. The pure decision logic (`decide_action`, the full +//! enabled/disabled/paused transition matrix) is covered by +//! `#[cfg(test)] mod tests` inside `src/disable.rs`. + +use std::path::PathBuf; + +fn binary() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_ado-aw")) +} + +#[test] +fn disable_help_describes_command() { + let output = std::process::Command::new(binary()) + .args(["disable", "--help"]) + .output() + .expect("Failed to run ado-aw disable --help"); + assert!(output.status.success(), "--help should exit 0"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("Disable (or pause) every ADO build definition"), + "Help text should describe the disable command, got:\n{stdout}" + ); + for flag in ["--org", "--project", "--pat", "--paused", "--dry-run"] { + assert!( + stdout.contains(flag), + "Expected --help to advertise {flag}, got:\n{stdout}" + ); + } +} + +#[test] +fn disable_is_listed_in_top_level_help() { + let output = std::process::Command::new(binary()) + .arg("--help") + .output() + .expect("Failed to run ado-aw --help"); + assert!(output.status.success(), "--help should exit 0"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("disable"), + "Top-level --help should mention the disable subcommand, got:\n{stdout}" + ); +} From 19b38fe0243c7fa4d38f33487daf829f2448fa0d Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 17 May 2026 21:05:05 +0100 Subject: [PATCH 02/14] feat(cli): add ado-aw remove Squash-merge of #592 (feat/cli-remove). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- AGENTS.md | 1 + docs/cli.md | 8 ++ src/ado/mod.rs | 36 +++++- src/main.rs | 44 +++++++ src/remove.rs | 238 ++++++++++++++++++++++++++++++++++++ tests/remove_integration.rs | 41 +++++++ 6 files changed, 363 insertions(+), 5 deletions(-) create mode 100644 src/remove.rs create mode 100644 tests/remove_integration.rs diff --git a/AGENTS.md b/AGENTS.md index 36004234..7a95bd89 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -78,6 +78,7 @@ Every compiled pipeline runs as three sequential jobs: │ ├── configure.rs # `configure` CLI command — orchestration shim atop `src/ado/` │ ├── enable.rs # `enable` CLI command — registers ADO build definitions for compiled pipelines and ensures they are enabled │ ├── disable.rs # `disable` CLI command — sets queueStatus to disabled (default) or paused on matched definitions +│ ├── remove.rs # `remove` CLI command — deletes matched ADO build definitions (with --yes / tty-prompt safety) │ ├── ado/ # Shared Azure DevOps REST helpers (auth, list/match/PATCH/POST) │ │ └── mod.rs # Used by `configure` and the `enable` command (ADO REST helpers: auth, list/match/PATCH/POST) │ ├── detect.rs # Agentic pipeline detection (helper for `configure`) diff --git a/docs/cli.md b/docs/cli.md index f0ad6a55..60389d09 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -62,3 +62,11 @@ Global flags (apply to all subcommands): `--verbose, -v` (enable info-level logg - `--pat ` / `AZURE_DEVOPS_EXT_PAT` env var - PAT for ADO API authentication (Azure CLI fallback if omitted). - `--paused` - Use `queueStatus: paused` instead of `disabled`. Paused definitions still queue scheduled runs but the queue is held; disabled definitions reject all queue requests. - `--dry-run` - Print the planned `from → to` transitions without calling the ADO API. + +- `remove [PATH]` - **Destructive.** Delete every ADO build definition that matches a local fixture under `PATH`. The same `match_definitions` safety property as `disable` applies: definitions without a local fixture are never in scope. Bulk deletes (`>1` match) require `--yes`; a single match on a tty prompts interactively (`y/N`); non-tty contexts always require `--yes`. Fail-soft per fixture; exits non-zero if any deletion failed or if zero local fixtures matched ADO definitions. + - `--org ` - Override: Azure DevOps organization (URL or bare org name). Inferred from git remote by default. + - `--project ` - Override: Azure DevOps project name (inferred from git remote by default). + - `--pat ` / `AZURE_DEVOPS_EXT_PAT` env var - PAT for ADO API authentication (Azure CLI fallback if omitted). + - `--yes` - Required for bulk deletes (>1 match) and for any delete in a non-tty context. A single match on a tty otherwise prompts interactively. + - `--dry-run` - Print the planned deletions without calling the ADO API. + diff --git a/src/ado/mod.rs b/src/ado/mod.rs index 81bcf5d8..8ddcad21 100644 --- a/src/ado/mod.rs +++ b/src/ado/mod.rs @@ -986,12 +986,38 @@ pub async fn patch_queue_status( /// /// Calls `DELETE /_apis/build/definitions/{id}?api-version=7.1`. pub async fn delete_definition( - _client: &reqwest::Client, - _ctx: &AdoContext, - _auth: &AdoAuth, - _id: u64, + client: &reqwest::Client, + ctx: &AdoContext, + auth: &AdoAuth, + id: u64, ) -> Result<()> { - anyhow::bail!("not yet implemented: filled in by PR 4 (ado-aw remove)") + let url = format!( + "{}/{}/_apis/build/definitions/{}?api-version=7.1", + ctx.org_url.trim_end_matches('/'), + percent_encoding::utf8_percent_encode(&ctx.project, PATH_SEGMENT), + id + ); + + debug!("DELETE definition {}: {}", id, url); + + let resp = auth + .apply(client.delete(&url)) + .send() + .await + .with_context(|| format!("Failed to delete definition {}", id))?; + + let status = resp.status(); + if !status.is_success() { + let body = resp.text().await.unwrap_or_default(); + anyhow::bail!( + "ADO API returned {} when deleting definition {}: {}", + status, + id, + body + ); + } + + Ok(()) } /// Create a new build definition. diff --git a/src/main.rs b/src/main.rs index a18d3387..ed613006 100644 --- a/src/main.rs +++ b/src/main.rs @@ -15,6 +15,7 @@ mod init; mod logging; mod mcp; mod ndjson; +mod remove; pub mod runtimes; pub mod sanitize; mod safeoutputs; @@ -200,6 +201,30 @@ enum Commands { #[arg(long)] dry_run: bool, }, + /// Delete every ADO build definition that matches a local fixture. + Remove { + /// Path to the repository root (defaults to current directory). Used + /// to auto-discover compiled pipelines, same as `compile`. + path: Option, + /// Override: Azure DevOps organization (URL like `https://dev.azure.com/myorg`, + /// or just the org name `myorg`). Inferred from git remote by default. + #[arg(long)] + org: Option, + /// Override: Azure DevOps project name (inferred from git remote by default). + #[arg(long)] + project: Option, + /// PAT for ADO API authentication (prefer setting AZURE_DEVOPS_EXT_PAT env var; + /// Azure CLI fallback if omitted). + #[arg(long, env = "AZURE_DEVOPS_EXT_PAT")] + pat: Option, + /// Required for bulk deletes (>1 match) and for any delete in a non-tty + /// context. A single match on a tty otherwise prompts interactively. + #[arg(long)] + yes: bool, + /// Preview the planned deletions without calling the ADO API. + #[arg(long)] + dry_run: bool, + }, } #[derive(Parser, Debug)] @@ -551,6 +576,7 @@ async fn main() -> Result<()> { Some(Commands::Configure { .. }) => "configure", Some(Commands::Enable { .. }) => "enable", Some(Commands::Disable { .. }) => "disable", + Some(Commands::Remove { .. }) => "remove", None => "ado-aw", }; @@ -701,6 +727,24 @@ async fn main() -> Result<()> { }) .await?; } + Commands::Remove { + path, + org, + project, + pat, + yes, + dry_run, + } => { + remove::run(remove::RemoveOptions { + org: org.as_deref(), + project: project.as_deref(), + pat: pat.as_deref(), + path: path.as_deref(), + yes, + dry_run, + }) + .await?; + } } Ok(()) } diff --git a/src/remove.rs b/src/remove.rs new file mode 100644 index 00000000..e7572daf --- /dev/null +++ b/src/remove.rs @@ -0,0 +1,238 @@ +//! The `remove` CLI command. +//! +//! Deletes every ADO build definition that matches a local fixture. +//! Phase 1 of the pipeline-lifecycle CLI family — see `docs/cli.md`. +//! +//! Safety: +//! +//! - Refuses to delete any ADO definition that is not matched against +//! a local fixture (this falls naturally out of [`match_definitions`]). +//! - Bulk deletes (`> 1` match) require `--yes`. Single-match deletes +//! require either `--yes` or an interactive `y/N` confirmation on +//! a tty; non-tty contexts always require `--yes`. + +use anyhow::{Context, Result}; +use std::io::IsTerminal; +use std::path::{Path, PathBuf}; + +use crate::ado::{ + MatchedDefinition, delete_definition, match_definitions, resolve_ado_context, resolve_auth, +}; +use crate::detect; + +/// Pure decision for the confirm-or-not gate. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Confirm { + /// Proceed without prompting (either `--yes` or `--dry-run`). + Proceed, + /// Prompt the operator interactively on the tty before deleting. + PromptTty, + /// Bail out — the operator must re-run with `--yes`. The string + /// is the user-visible reason. + RequireYes(String), +} + +/// Pure function: decide the gating mode for a remove operation. +/// +/// Returns [`Confirm::Proceed`] when the caller explicitly opted into +/// the operation, [`Confirm::PromptTty`] for a single match on a tty, +/// or [`Confirm::RequireYes`] when the operator must rerun with +/// `--yes` (bulk deletes, single-match in non-tty). +pub fn decide_confirm( + match_count: usize, + yes: bool, + dry_run: bool, + is_tty: bool, +) -> Confirm { + if dry_run || yes { + return Confirm::Proceed; + } + if match_count > 1 { + return Confirm::RequireYes(format!( + "{} definitions would be deleted; rerun with --yes to confirm.", + match_count + )); + } + if match_count == 1 { + if is_tty { + return Confirm::PromptTty; + } + return Confirm::RequireYes( + "stdin is not a tty; rerun with --yes to confirm.".to_string(), + ); + } + Confirm::Proceed +} + +/// CLI options for [`run`]. +pub struct RemoveOptions<'a> { + pub org: Option<&'a str>, + pub project: Option<&'a str>, + pub pat: Option<&'a str>, + pub path: Option<&'a Path>, + pub yes: bool, + pub dry_run: bool, +} + +/// Run the `remove` command. +pub async fn run(opts: RemoveOptions<'_>) -> Result<()> { + let repo_path: PathBuf = match opts.path { + Some(p) => tokio::fs::canonicalize(p) + .await + .with_context(|| format!("Could not resolve path: {}", p.display()))?, + None => tokio::fs::canonicalize(".") + .await + .context("Could not resolve current directory")?, + }; + + let auth = resolve_auth(opts.pat).await?; + let ado_ctx = resolve_ado_context(&repo_path, opts.org, opts.project).await?; + + println!( + "ADO context: org={}, project={}", + ado_ctx.org_url, ado_ctx.project + ); + println!(); + + println!("Scanning for agentic pipelines..."); + let detected = detect::detect_pipelines(&repo_path).await?; + if detected.is_empty() { + println!( + "No agentic pipelines found. Make sure your pipelines were compiled with the latest ado-aw." + ); + return Ok(()); + } + println!("Found {} agentic pipeline(s).", detected.len()); + println!(); + + let client = reqwest::Client::builder() + .timeout(std::time::Duration::from_secs(30)) + .build() + .context("Failed to create HTTP client")?; + + println!("Matching to Azure DevOps pipeline definitions..."); + let matched = match_definitions(&client, &ado_ctx, &auth, &detected).await?; + + if matched.is_empty() { + anyhow::bail!( + "No ADO definitions matched any local fixture. Run `ado-aw list` to \ + diagnose; nothing to delete." + ); + } + + println!("{} definition(s) would be deleted:", matched.len()); + for m in &matched { + println!(" - {} (id={})", m.name, m.id); + } + println!(); + + let confirm = decide_confirm( + matched.len(), + opts.yes, + opts.dry_run, + std::io::stdin().is_terminal(), + ); + match confirm { + Confirm::Proceed => {} + Confirm::RequireYes(reason) => anyhow::bail!("{}", reason), + Confirm::PromptTty => { + if !prompt_yes_no(&matched[0])? { + println!("Aborted by user."); + return Ok(()); + } + } + } + + let mut success = 0usize; + let mut failure = 0usize; + for m in &matched { + if opts.dry_run { + println!("[dry-run] ✓ would delete: {} (id={})", m.name, m.id); + success += 1; + continue; + } + match delete_definition(&client, &ado_ctx, &auth, m.id).await { + Ok(()) => { + println!("✓ deleted: {} (id={})", m.name, m.id); + success += 1; + } + Err(e) => { + eprintln!("✗ failed: {} (id={}): {:#}", m.name, m.id, e); + failure += 1; + } + } + } + + println!(); + println!("Done: {} succeeded, {} failed.", success, failure); + if failure > 0 { + anyhow::bail!("{} deletion(s) failed", failure); + } + Ok(()) +} + +fn prompt_yes_no(m: &MatchedDefinition) -> Result { + let prompt = format!("Delete '{}' (id={})?", m.name, m.id); + inquire::Confirm::new(&prompt) + .with_default(false) + .prompt() + .context("Failed to read confirmation from interactive prompt") +} + +#[cfg(test)] +mod tests { + use super::*; + + // ============ decide_confirm matrix ============ + + #[test] + fn dry_run_always_proceeds() { + assert_eq!(decide_confirm(0, false, true, false), Confirm::Proceed); + assert_eq!(decide_confirm(1, false, true, false), Confirm::Proceed); + assert_eq!(decide_confirm(5, false, true, false), Confirm::Proceed); + assert_eq!(decide_confirm(5, false, true, true), Confirm::Proceed); + } + + #[test] + fn yes_always_proceeds() { + assert_eq!(decide_confirm(0, true, false, false), Confirm::Proceed); + assert_eq!(decide_confirm(1, true, false, false), Confirm::Proceed); + assert_eq!(decide_confirm(5, true, false, false), Confirm::Proceed); + assert_eq!(decide_confirm(5, true, false, true), Confirm::Proceed); + } + + #[test] + fn bulk_without_yes_requires_yes_even_on_tty() { + match decide_confirm(3, false, false, true) { + Confirm::RequireYes(reason) => { + assert!(reason.contains("3 definitions"), "got: {}", reason); + assert!(reason.contains("--yes"), "got: {}", reason); + } + other => panic!("expected RequireYes, got {:?}", other), + } + } + + #[test] + fn single_match_on_tty_prompts() { + assert_eq!(decide_confirm(1, false, false, true), Confirm::PromptTty); + } + + #[test] + fn single_match_non_tty_requires_yes() { + match decide_confirm(1, false, false, false) { + Confirm::RequireYes(reason) => { + assert!(reason.contains("tty"), "got: {}", reason); + assert!(reason.contains("--yes"), "got: {}", reason); + } + other => panic!("expected RequireYes, got {:?}", other), + } + } + + #[test] + fn zero_matches_proceeds_so_caller_can_handle() { + // The empty case is handled earlier in `run` (bail with hint) + // but the gate itself shouldn't block. + assert_eq!(decide_confirm(0, false, false, true), Confirm::Proceed); + assert_eq!(decide_confirm(0, false, false, false), Confirm::Proceed); + } +} diff --git a/tests/remove_integration.rs b/tests/remove_integration.rs new file mode 100644 index 00000000..3fff6e5c --- /dev/null +++ b/tests/remove_integration.rs @@ -0,0 +1,41 @@ +//! Integration tests for `ado-aw remove`. + +use std::path::PathBuf; + +fn binary() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_ado-aw")) +} + +#[test] +fn remove_help_describes_command() { + let output = std::process::Command::new(binary()) + .args(["remove", "--help"]) + .output() + .expect("Failed to run ado-aw remove --help"); + assert!(output.status.success(), "--help should exit 0"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("Delete every ADO build definition"), + "Help text should describe the remove command, got:\n{stdout}" + ); + for flag in ["--org", "--project", "--pat", "--yes", "--dry-run"] { + assert!( + stdout.contains(flag), + "Expected --help to advertise {flag}, got:\n{stdout}" + ); + } +} + +#[test] +fn remove_is_listed_in_top_level_help() { + let output = std::process::Command::new(binary()) + .arg("--help") + .output() + .expect("Failed to run ado-aw --help"); + assert!(output.status.success(), "--help should exit 0"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("remove"), + "Top-level --help should mention the remove subcommand, got:\n{stdout}" + ); +} From 26bb742b5a73e2396c356773919fe43164680f35 Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 17 May 2026 21:07:53 +0100 Subject: [PATCH 03/14] feat(cli): add ado-aw list Squash-merge of #594 (feat/cli-list). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- AGENTS.md | 1 + docs/cli.md | 9 +- src/ado/mod.rs | 51 +++- src/enable.rs | 1 + src/list.rs | 535 ++++++++++++++++++++++++++++++++++++++ src/main.rs | 43 +++ tests/list_integration.rs | 41 +++ 7 files changed, 675 insertions(+), 6 deletions(-) create mode 100644 src/list.rs create mode 100644 tests/list_integration.rs diff --git a/AGENTS.md b/AGENTS.md index 7a95bd89..bd4ef1d3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -79,6 +79,7 @@ Every compiled pipeline runs as three sequential jobs: │ ├── enable.rs # `enable` CLI command — registers ADO build definitions for compiled pipelines and ensures they are enabled │ ├── disable.rs # `disable` CLI command — sets queueStatus to disabled (default) or paused on matched definitions │ ├── remove.rs # `remove` CLI command — deletes matched ADO build definitions (with --yes / tty-prompt safety) +│ ├── list.rs # `list` CLI command — renders matched ADO definitions with their latest-run state (text or JSON) │ ├── ado/ # Shared Azure DevOps REST helpers (auth, list/match/PATCH/POST) │ │ └── mod.rs # Used by `configure` and the `enable` command (ADO REST helpers: auth, list/match/PATCH/POST) │ ├── detect.rs # Agentic pipeline detection (helper for `configure`) diff --git a/docs/cli.md b/docs/cli.md index 60389d09..db690b38 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -56,7 +56,7 @@ Global flags (apply to all subcommands): `--verbose, -v` (enable info-level logg **Source-repo scope (Phase 1):** `enable` requires the local git remote to be an Azure DevOps Git remote (the source repo is what gets registered as the definition's repository). GitHub-hosted source repos are gated on a follow-up. -- `disable [PATH]` - Set `queueStatus` to `disabled` (default) or `paused` on every ADO build definition that matches a local fixture under `PATH`. Refuses to touch any ADO definition that is not the target of a local fixture match — that safety property falls naturally out of the same yaml-path + name match used by `configure`. Skips definitions that are already at the requested status; fail-soft per fixture; exits non-zero if any patch failed or if zero local fixtures matched ADO definitions. +- `disable [PATH]`- Set `queueStatus` to `disabled` (default) or `paused` on every ADO build definition that matches a local fixture under `PATH`. Refuses to touch any ADO definition that is not the target of a local fixture match — that safety property falls naturally out of the same yaml-path + name match used by `configure`. Skips definitions that are already at the requested status; fail-soft per fixture; exits non-zero if any patch failed or if zero local fixtures matched ADO definitions. - `--org ` - Override: Azure DevOps organization (URL or bare org name). Inferred from git remote by default. - `--project ` - Override: Azure DevOps project name (inferred from git remote by default). - `--pat ` / `AZURE_DEVOPS_EXT_PAT` env var - PAT for ADO API authentication (Azure CLI fallback if omitted). @@ -70,3 +70,10 @@ Global flags (apply to all subcommands): `--verbose, -v` (enable info-level logg - `--yes` - Required for bulk deletes (>1 match) and for any delete in a non-tty context. A single match on a tty otherwise prompts interactively. - `--dry-run` - Print the planned deletions without calling the ADO API. +- `list [PATH]` - Render every ADO build definition that matches a local fixture (under `PATH`) along with its `queueStatus`, ADO folder, and latest-run summary. Pass `--all` to also include definitions with no matching local fixture. Output defaults to a human-readable table; `--json` emits a stable JSON array suitable for scripting. + - `--org ` - Override: Azure DevOps organization (URL or bare org name). Inferred from git remote by default. + - `--project ` - Override: Azure DevOps project name (inferred from git remote by default). + - `--pat ` / `AZURE_DEVOPS_EXT_PAT` env var - PAT for ADO API authentication (Azure CLI fallback if omitted). + - `--all` - Include ADO definitions that do not match any local fixture. + - `--json` - Emit machine-readable JSON. + diff --git a/src/ado/mod.rs b/src/ado/mod.rs index 8ddcad21..633b3293 100644 --- a/src/ado/mod.rs +++ b/src/ado/mod.rs @@ -205,6 +205,10 @@ pub struct DefinitionSummary { /// [`list_definitions`]). Older/cached responses may omit it. #[serde(rename = "queueStatus")] pub queue_status: Option, + /// ADO folder path (e.g. `\smoke`, `\`). Populated when + /// `includeAllProperties=true`. May be absent on older API versions. + #[serde(default)] + pub path: Option, } #[derive(Debug, Deserialize)] @@ -1100,12 +1104,47 @@ pub async fn get_build( /// Calls `GET /_apis/build/builds?definitions={id}&$top=1&api-version=7.1` /// and returns the first result (or `None` if the definition has never run). pub async fn get_latest_build( - _client: &reqwest::Client, - _ctx: &AdoContext, - _auth: &AdoAuth, - _definition_id: u64, + client: &reqwest::Client, + ctx: &AdoContext, + auth: &AdoAuth, + definition_id: u64, ) -> Result> { - anyhow::bail!("not yet implemented: filled in by PR 5 (ado-aw list) or PR 7 (ado-aw status)") + let url = format!( + "{}/{}/_apis/build/builds?definitions={}&$top=1&api-version=7.1", + ctx.org_url.trim_end_matches('/'), + ctx.project, + definition_id, + ); + + debug!("GET latest build for definition {}: {}", definition_id, url); + + let resp = auth + .apply(client.get(&url)) + .send() + .await + .with_context(|| format!("Failed to fetch latest build for definition {}", definition_id))?; + + let status = resp.status(); + if !status.is_success() { + let body = resp.text().await.unwrap_or_default(); + anyhow::bail!( + "ADO API returned {} when fetching latest build for definition {}: {}", + status, + definition_id, + body + ); + } + + let body: serde_json::Value = resp + .json() + .await + .with_context(|| format!("Failed to parse builds response for {}", definition_id))?; + + Ok(body + .get("value") + .and_then(|v| v.as_array()) + .and_then(|a| a.first()) + .cloned()) } #[cfg(test)] @@ -1220,6 +1259,7 @@ mod tests { name: name.to_string(), process: None, queue_status: None, + path: None, } } @@ -1231,6 +1271,7 @@ mod tests { yaml_filename: Some(yaml_filename.to_string()), }), queue_status: None, + path: None, } } diff --git a/src/enable.rs b/src/enable.rs index b4e27cf1..726971d7 100644 --- a/src/enable.rs +++ b/src/enable.rs @@ -452,6 +452,7 @@ mod tests { yaml_filename: Some(y.to_string()), }), queue_status: status.map(str::to_string), + path: None, } } diff --git a/src/list.rs b/src/list.rs new file mode 100644 index 00000000..39f4d643 --- /dev/null +++ b/src/list.rs @@ -0,0 +1,535 @@ +//! The `list` CLI command. +//! +//! Renders the current state of every ADO build definition that +//! matches a local fixture (or *all* definitions with `--all`). +//! Phase 1 of the pipeline-lifecycle CLI family — see `docs/cli.md`. +//! +//! Output: +//! +//! - Default: human-readable text table. +//! - `--json`: JSON array, stable shape suitable for programmatic +//! consumption. + +use anyhow::{Context, Result}; +use std::collections::HashSet; +use std::path::{Path, PathBuf}; + +use crate::ado::{ + DefinitionSummary, MatchedDefinition, get_latest_build, list_definitions, match_definitions, + resolve_ado_context, resolve_auth, +}; +use crate::detect; + +/// One row of the rendered output. +/// +/// `last_run` is intentionally untyped here — ADO build records have +/// many optional fields and the JSON renderer passes them through +/// verbatim, while the text renderer only inspects `result`. Pure +/// data so we can snapshot-test both renderers. +#[derive(Debug, Clone, PartialEq)] +pub struct ListRow { + pub id: u64, + pub name: String, + pub folder: Option, + pub queue_status: Option, + pub yaml_filename: Option, + pub matched: bool, + pub last_run: Option, +} + +/// Latest-build summary, projected to the fields the text renderer +/// uses. JSON output passes the full `serde_json::Value` through so +/// callers don't lose access to fields we don't currently surface. +#[derive(Debug, Clone, PartialEq)] +pub struct LastRun { + pub id: u64, + pub result: Option, + pub status: Option, + pub finish_time: Option, + pub url: Option, + /// Raw value for JSON pass-through. + pub raw: serde_json::Value, +} + +impl LastRun { + /// Project an ADO `build` JSON value into a [`LastRun`]. Returns + /// `None` when the JSON has no usable `id` field. + pub fn from_json(value: serde_json::Value) -> Option { + let id = value.get("id").and_then(|v| v.as_u64())?; + Some(Self { + id, + result: value + .get("result") + .and_then(|v| v.as_str()) + .map(str::to_string), + status: value + .get("status") + .and_then(|v| v.as_str()) + .map(str::to_string), + finish_time: value + .get("finishTime") + .and_then(|v| v.as_str()) + .map(str::to_string), + url: value + .get("_links") + .and_then(|l| l.get("web")) + .and_then(|w| w.get("href")) + .and_then(|v| v.as_str()) + .map(str::to_string) + .or_else(|| { + value + .get("url") + .and_then(|v| v.as_str()) + .map(str::to_string) + }), + raw: value, + }) + } +} + +/// CLI options for [`run`]. +pub struct ListOptions<'a> { + pub org: Option<&'a str>, + pub project: Option<&'a str>, + pub pat: Option<&'a str>, + pub path: Option<&'a Path>, + pub all: bool, + pub json: bool, +} + +/// Pure function: assemble [`ListRow`]s from raw inputs. +/// +/// - `definitions`: project-wide listing from `list_definitions`. +/// - `matched`: subset that maps to a local fixture (yaml-path or +/// pipeline-name). +/// - `last_runs`: latest-build JSON keyed by definition id. Missing +/// entries become `last_run: None`. +/// - `include_unmatched`: when `false`, definitions that aren't in +/// `matched` are filtered out (default for `list`). +/// +/// Row ordering: matched rows first (sorted by name), then unmatched +/// rows (also sorted by name) when included. +pub fn build_rows( + definitions: &[DefinitionSummary], + matched: &[MatchedDefinition], + last_runs: &std::collections::HashMap, + include_unmatched: bool, +) -> Vec { + let matched_ids: HashSet = matched.iter().map(|m| m.id).collect(); + let yaml_by_id: std::collections::HashMap = matched + .iter() + .filter(|m| !m.yaml_path.is_empty()) + .map(|m| (m.id, m.yaml_path.clone())) + .collect(); + + let mut rows: Vec = definitions + .iter() + .filter(|d| include_unmatched || matched_ids.contains(&d.id)) + .map(|d| { + let yaml_filename = yaml_by_id.get(&d.id).cloned().or_else(|| { + d.process + .as_ref() + .and_then(|p| p.yaml_filename.clone()) + }); + let last_run = last_runs.get(&d.id).cloned().and_then(LastRun::from_json); + ListRow { + id: d.id, + name: d.name.clone(), + folder: d.path.clone(), + queue_status: d.queue_status.clone(), + yaml_filename, + matched: matched_ids.contains(&d.id), + last_run, + } + }) + .collect(); + + rows.sort_by(|a, b| { + b.matched + .cmp(&a.matched) + .then_with(|| a.name.to_lowercase().cmp(&b.name.to_lowercase())) + }); + rows +} + +/// Pure function: render a list of rows as a text table. +pub fn render_text(rows: &[ListRow]) -> String { + if rows.is_empty() { + return "(no definitions)\n".to_string(); + } + let headers = ["NAME", "ID", "FOLDER", "STATUS", "LAST RUN", "SOURCE"]; + let mut widths = headers.map(|h| h.chars().count()); + let str_rows: Vec<[String; 6]> = rows + .iter() + .map(|r| { + [ + r.name.clone(), + r.id.to_string(), + r.folder.clone().unwrap_or_default(), + r.queue_status.clone().unwrap_or_else(|| "?".to_string()), + r.last_run + .as_ref() + .map(|lr| { + lr.result + .clone() + .or_else(|| lr.status.clone()) + .unwrap_or_else(|| "in progress".to_string()) + }) + .unwrap_or_else(|| "never".to_string()), + r.yaml_filename + .clone() + .map(|y| y.trim_start_matches('/').to_string()) + .unwrap_or_default(), + ] + }) + .collect(); + for cells in &str_rows { + for (i, cell) in cells.iter().enumerate() { + widths[i] = widths[i].max(cell.chars().count()); + } + } + let mut out = String::new(); + write_row(&mut out, &headers.map(str::to_string), &widths); + for cells in &str_rows { + write_row(&mut out, cells, &widths); + } + out +} + +fn write_row(out: &mut String, cells: &[String; 6], widths: &[usize; 6]) { + for (i, cell) in cells.iter().enumerate() { + if i > 0 { + out.push_str(" "); + } + let pad = widths[i].saturating_sub(cell.chars().count()); + out.push_str(cell); + if i < cells.len() - 1 { + for _ in 0..pad { + out.push(' '); + } + } + } + out.push('\n'); +} + +/// Pure function: render the rows as JSON. +pub fn render_json(rows: &[ListRow]) -> Result { + let array: Vec = rows + .iter() + .map(|r| { + // Keep this as a raw pass-through for scripting stability. + // Text output trims the leading slash for readability; JSON + // intentionally retains ADO/local-matcher path shape. + serde_json::json!({ + "name": r.name, + "id": r.id, + "folder": r.folder, + "queueStatus": r.queue_status, + "yamlFilename": r.yaml_filename, + "matched": r.matched, + "lastRun": r.last_run.as_ref().map(|lr| &lr.raw), + }) + }) + .collect(); + serde_json::to_string_pretty(&array).context("Failed to serialize list rows as JSON") +} + +/// Run the `list` command. +pub async fn run(opts: ListOptions<'_>) -> Result<()> { + let repo_path: PathBuf = match opts.path { + Some(p) => tokio::fs::canonicalize(p) + .await + .with_context(|| format!("Could not resolve path: {}", p.display()))?, + None => tokio::fs::canonicalize(".") + .await + .context("Could not resolve current directory")?, + }; + + let auth = resolve_auth(opts.pat).await?; + let ado_ctx = resolve_ado_context(&repo_path, opts.org, opts.project).await?; + + let client = reqwest::Client::builder() + .timeout(std::time::Duration::from_secs(30)) + .build() + .context("Failed to create HTTP client")?; + + let definitions = list_definitions(&client, &ado_ctx, &auth).await?; + let detected = detect::detect_pipelines(&repo_path).await.unwrap_or_default(); + let matched = match match_definitions(&client, &ado_ctx, &auth, &detected).await { + Ok(m) => m, + Err(e) => { + eprintln!( + " warning: failed to match local pipeline files with ADO definitions: {:#}; continuing with unmatched results", + e + ); + Vec::new() + } + }; + + // Decide which IDs need a last-build fetch. + let target_ids: HashSet = if opts.all { + definitions.iter().map(|d| d.id).collect() + } else { + matched.iter().map(|m| m.id).collect() + }; + + // Sequential fetch (small N; bounded fanout via JoinSet is a + // straightforward future improvement once we have a project with + // 50+ matched pipelines). + let mut last_runs = std::collections::HashMap::new(); + for id in &target_ids { + match get_latest_build(&client, &ado_ctx, &auth, *id).await { + Ok(Some(v)) => { + last_runs.insert(*id, v); + } + Ok(None) => {} + Err(e) => { + eprintln!(" warning: failed to fetch latest build for {}: {:#}", id, e); + } + } + } + + let rows = build_rows(&definitions, &matched, &last_runs, opts.all); + + if opts.json { + println!("{}", render_json(&rows)?); + } else { + print!("{}", render_text(&rows)); + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::ado::{MatchMethod, ProcessInfo}; + use std::collections::HashMap; + + fn def(id: u64, name: &str, folder: Option<&str>, yaml: Option<&str>, status: Option<&str>) -> DefinitionSummary { + DefinitionSummary { + id, + name: name.to_string(), + process: yaml.map(|y| ProcessInfo { + yaml_filename: Some(y.to_string()), + }), + queue_status: status.map(str::to_string), + path: folder.map(str::to_string), + } + } + + fn matched(id: u64, name: &str, yaml: &str) -> MatchedDefinition { + MatchedDefinition { + id, + name: name.to_string(), + match_method: MatchMethod::YamlPath, + yaml_path: yaml.to_string(), + } + } + + // ============ LastRun::from_json ============ + + #[test] + fn last_run_extracts_fields() { + let v = serde_json::json!({ + "id": 1234, + "result": "succeeded", + "status": "completed", + "finishTime": "2026-05-17T08:00:00Z", + "_links": { "web": { "href": "https://dev.azure.com/.../1234" } } + }); + let lr = LastRun::from_json(v).unwrap(); + assert_eq!(lr.id, 1234); + assert_eq!(lr.result.as_deref(), Some("succeeded")); + assert_eq!(lr.status.as_deref(), Some("completed")); + assert_eq!(lr.finish_time.as_deref(), Some("2026-05-17T08:00:00Z")); + assert_eq!(lr.url.as_deref(), Some("https://dev.azure.com/.../1234")); + } + + #[test] + fn last_run_falls_back_to_top_level_url() { + let v = serde_json::json!({ + "id": 7, + "url": "https://dev.azure.com/.../7" + }); + let lr = LastRun::from_json(v).unwrap(); + assert_eq!(lr.url.as_deref(), Some("https://dev.azure.com/.../7")); + } + + #[test] + fn last_run_returns_none_when_id_missing() { + let v = serde_json::json!({ "result": "succeeded" }); + assert!(LastRun::from_json(v).is_none()); + } + + // ============ build_rows ============ + + #[test] + fn build_rows_default_filters_unmatched() { + let defs = vec![ + def(1, "matched", Some("\\smoke"), Some("/a.yml"), Some("enabled")), + def(2, "unmatched", Some("\\other"), Some("/b.yml"), Some("enabled")), + ]; + let m = vec![matched(1, "matched", "/a.yml")]; + let rows = build_rows(&defs, &m, &HashMap::new(), false); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0].id, 1); + assert!(rows[0].matched); + } + + #[test] + fn build_rows_all_flag_includes_unmatched() { + let defs = vec![ + def(1, "matched", Some("\\smoke"), Some("/a.yml"), Some("enabled")), + def(2, "unmatched", Some("\\other"), Some("/b.yml"), Some("disabled")), + ]; + let m = vec![matched(1, "matched", "/a.yml")]; + let rows = build_rows(&defs, &m, &HashMap::new(), true); + assert_eq!(rows.len(), 2); + // Matched rows sort first. + assert!(rows[0].matched); + assert!(!rows[1].matched); + } + + #[test] + fn build_rows_sorts_within_group_by_name_case_insensitive() { + let defs = vec![ + def(1, "Zebra", None, None, None), + def(2, "alpha", None, None, None), + def(3, "Beta", None, None, None), + ]; + let m = vec![matched(1, "Zebra", "/z.yml"), matched(2, "alpha", "/a.yml"), matched(3, "Beta", "/b.yml")]; + let rows = build_rows(&defs, &m, &HashMap::new(), false); + assert_eq!(rows.iter().map(|r| r.name.as_str()).collect::>(), vec!["alpha", "Beta", "Zebra"]); + } + + #[test] + fn build_rows_attaches_last_run() { + let defs = vec![def(1, "x", None, Some("/x.yml"), Some("enabled"))]; + let m = vec![matched(1, "x", "/x.yml")]; + let mut runs = HashMap::new(); + runs.insert( + 1u64, + serde_json::json!({ "id": 99, "result": "succeeded" }), + ); + let rows = build_rows(&defs, &m, &runs, false); + assert_eq!(rows[0].last_run.as_ref().unwrap().id, 99); + assert_eq!( + rows[0] + .last_run + .as_ref() + .unwrap() + .result + .as_deref(), + Some("succeeded") + ); + } + + // ============ render_text ============ + + #[test] + fn render_text_includes_headers_and_data() { + let rows = vec![ListRow { + id: 123, + name: "Daily noop".to_string(), + folder: Some("\\smoke".to_string()), + queue_status: Some("enabled".to_string()), + yaml_filename: Some("/tests/noop.lock.yml".to_string()), + matched: true, + last_run: Some(LastRun { + id: 999, + result: Some("succeeded".to_string()), + status: None, + finish_time: None, + url: None, + raw: serde_json::Value::Null, + }), + }]; + let out = render_text(&rows); + assert!(out.contains("NAME")); + assert!(out.contains("ID")); + assert!(out.contains("FOLDER")); + assert!(out.contains("STATUS")); + assert!(out.contains("Daily noop")); + assert!(out.contains("123")); + assert!(out.contains("\\smoke")); + assert!(out.contains("enabled")); + assert!(out.contains("succeeded")); + // Yaml source rendered without the leading slash. + assert!(out.contains("tests/noop.lock.yml")); + } + + #[test] + fn render_text_uses_never_when_no_last_run() { + let rows = vec![ListRow { + id: 1, + name: "x".to_string(), + folder: None, + queue_status: Some("enabled".to_string()), + yaml_filename: None, + matched: true, + last_run: None, + }]; + let out = render_text(&rows); + assert!(out.contains("never")); + } + + #[test] + fn render_text_empty_returns_placeholder() { + assert_eq!(render_text(&[]), "(no definitions)\n"); + } + + // ============ render_json ============ + + #[test] + fn render_json_emits_expected_shape() { + let raw = serde_json::json!({ + "id": 999, + "result": "succeeded", + "status": "completed", + "finishTime": "2026-05-17T08:00:00Z", + "requestedFor": { "displayName": "A User" }, + "triggerInfo": { "ci.sourceSha": "abc123" }, + }); + let rows = vec![ListRow { + id: 123, + name: "Daily noop".to_string(), + folder: Some("\\smoke".to_string()), + queue_status: Some("enabled".to_string()), + yaml_filename: Some("/tests/noop.lock.yml".to_string()), + matched: true, + last_run: Some(LastRun { + id: 999, + result: Some("succeeded".to_string()), + status: Some("completed".to_string()), + finish_time: Some("2026-05-17T08:00:00Z".to_string()), + url: Some("https://dev.azure.com/.../999".to_string()), + raw: raw.clone(), + }), + }]; + let out = render_json(&rows).unwrap(); + let parsed: serde_json::Value = serde_json::from_str(&out).unwrap(); + assert_eq!(parsed[0]["name"], "Daily noop"); + assert_eq!(parsed[0]["id"], 123); + assert_eq!(parsed[0]["folder"], "\\smoke"); + assert_eq!(parsed[0]["queueStatus"], "enabled"); + assert_eq!(parsed[0]["yamlFilename"], "/tests/noop.lock.yml"); + assert_eq!(parsed[0]["matched"], true); + assert_eq!(parsed[0]["lastRun"], raw); + } + + #[test] + fn render_json_lastrun_is_null_when_missing() { + let rows = vec![ListRow { + id: 1, + name: "x".to_string(), + folder: None, + queue_status: None, + yaml_filename: None, + matched: true, + last_run: None, + }]; + let out = render_json(&rows).unwrap(); + let parsed: serde_json::Value = serde_json::from_str(&out).unwrap(); + assert!(parsed[0]["lastRun"].is_null()); + } +} diff --git a/src/main.rs b/src/main.rs index ed613006..6f4acaac 100644 --- a/src/main.rs +++ b/src/main.rs @@ -12,6 +12,7 @@ mod execute; mod fuzzy_schedule; mod hash; mod init; +mod list; mod logging; mod mcp; mod ndjson; @@ -225,6 +226,29 @@ enum Commands { #[arg(long)] dry_run: bool, }, + /// List ADO build definitions (with their latest-run state) that match local fixtures. + List { + /// Path to the repository root (defaults to current directory). Used + /// to auto-discover compiled pipelines, same as `compile`. + path: Option, + /// Override: Azure DevOps organization (URL like `https://dev.azure.com/myorg`, + /// or just the org name `myorg`). Inferred from git remote by default. + #[arg(long)] + org: Option, + /// Override: Azure DevOps project name (inferred from git remote by default). + #[arg(long)] + project: Option, + /// PAT for ADO API authentication (prefer setting AZURE_DEVOPS_EXT_PAT env var; + /// Azure CLI fallback if omitted). + #[arg(long, env = "AZURE_DEVOPS_EXT_PAT")] + pat: Option, + /// Include ADO definitions that do not match any local fixture. + #[arg(long)] + all: bool, + /// Emit machine-readable JSON instead of the text table. + #[arg(long)] + json: bool, + }, } #[derive(Parser, Debug)] @@ -577,6 +601,7 @@ async fn main() -> Result<()> { Some(Commands::Enable { .. }) => "enable", Some(Commands::Disable { .. }) => "disable", Some(Commands::Remove { .. }) => "remove", + Some(Commands::List { .. }) => "list", None => "ado-aw", }; @@ -745,6 +770,24 @@ async fn main() -> Result<()> { }) .await?; } + Commands::List { + path, + org, + project, + pat, + all, + json, + } => { + list::run(list::ListOptions { + org: org.as_deref(), + project: project.as_deref(), + pat: pat.as_deref(), + path: path.as_deref(), + all, + json, + }) + .await?; + } } Ok(()) } diff --git a/tests/list_integration.rs b/tests/list_integration.rs new file mode 100644 index 00000000..90275da8 --- /dev/null +++ b/tests/list_integration.rs @@ -0,0 +1,41 @@ +//! Integration tests for `ado-aw list`. + +use std::path::PathBuf; + +fn binary() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_ado-aw")) +} + +#[test] +fn list_help_describes_command() { + let output = std::process::Command::new(binary()) + .args(["list", "--help"]) + .output() + .expect("Failed to run ado-aw list --help"); + assert!(output.status.success(), "--help should exit 0"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("List ADO build definitions"), + "Help text should describe the list command, got:\n{stdout}" + ); + for flag in ["--org", "--project", "--pat", "--all", "--json"] { + assert!( + stdout.contains(flag), + "Expected --help to advertise {flag}, got:\n{stdout}" + ); + } +} + +#[test] +fn list_is_in_top_level_help() { + let output = std::process::Command::new(binary()) + .arg("--help") + .output() + .expect("Failed to run ado-aw --help"); + assert!(output.status.success(), "--help should exit 0"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("list"), + "Top-level --help should mention the list subcommand, got:\n{stdout}" + ); +} From 9cb32a17bdd57e82e26eca4aef3b497b34f2f6e4 Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 17 May 2026 13:44:14 +0100 Subject: [PATCH 04/14] feat(cli): add ado-aw status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements PR 7 of the Phase 1 CLI overhaul. Renders per-pipeline status — name, id, folder, queueStatus, latest-run summary, and a deep link — one block per matched ADO definition. Read-only. `status` is intentionally a thin renderer over the same data path as `list` (same `list_definitions` + `match_definitions` + `get_latest_build` sequence). The `--json` shape is byte-for-byte identical to `list --json` so scripts can use either. CLI surface: ado-aw status [PATH] --org --project --pat --json The block renderer is a pure function (`render_blocks`) tested against six scenarios: - empty → placeholder line - succeeded run with url → all fields rendered - run with no url → synthesizes `{org_url}/{project}/_build/results?buildId={id}` - no last run → "never" + no url line - in-progress run (no `result` yet) → shows `status` value instead - missing queueStatus → renders `?` placeholder Depends on PR 5 (#594) for `crate::list::{ListRow, LastRun, build_rows, render_json}`; reuses the `get_latest_build` ado/mod.rs helper landed there. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- AGENTS.md | 1 + docs/cli.md | 6 + src/main.rs | 38 ++++++ src/status.rs | 237 ++++++++++++++++++++++++++++++++++++ tests/status_integration.rs | 41 +++++++ 5 files changed, 323 insertions(+) create mode 100644 src/status.rs create mode 100644 tests/status_integration.rs diff --git a/AGENTS.md b/AGENTS.md index bd4ef1d3..c8f01df6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -80,6 +80,7 @@ Every compiled pipeline runs as three sequential jobs: │ ├── disable.rs # `disable` CLI command — sets queueStatus to disabled (default) or paused on matched definitions │ ├── remove.rs # `remove` CLI command — deletes matched ADO build definitions (with --yes / tty-prompt safety) │ ├── list.rs # `list` CLI command — renders matched ADO definitions with their latest-run state (text or JSON) +│ ├── status.rs # `status` CLI command — denser per-pipeline status block (thin renderer over `list`'s data path) │ ├── ado/ # Shared Azure DevOps REST helpers (auth, list/match/PATCH/POST) │ │ └── mod.rs # Used by `configure` and the `enable` command (ADO REST helpers: auth, list/match/PATCH/POST) │ ├── detect.rs # Agentic pipeline detection (helper for `configure`) diff --git a/docs/cli.md b/docs/cli.md index db690b38..dd58f9b8 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -77,3 +77,9 @@ Global flags (apply to all subcommands): `--verbose, -v` (enable info-level logg - `--all` - Include ADO definitions that do not match any local fixture. - `--json` - Emit machine-readable JSON. +- `status [PATH]` - Per-pipeline status: name, id, folder, `queueStatus`, latest-run summary, and a deep link — one block per matched definition. Read-only. `--json` emits the same shape as `list --json` so scripts can use either. + - `--org ` - Override: Azure DevOps organization (URL or bare org name). Inferred from git remote by default. + - `--project ` - Override: Azure DevOps project name (inferred from git remote by default). + - `--pat ` / `AZURE_DEVOPS_EXT_PAT` env var - PAT for ADO API authentication (Azure CLI fallback if omitted). + - `--json` - Emit machine-readable JSON (same shape as `list --json`). + diff --git a/src/main.rs b/src/main.rs index 6f4acaac..3ff98de8 100644 --- a/src/main.rs +++ b/src/main.rs @@ -20,6 +20,7 @@ mod remove; pub mod runtimes; pub mod sanitize; mod safeoutputs; +mod status; mod tools; pub mod validate; @@ -249,6 +250,26 @@ enum Commands { #[arg(long)] json: bool, }, + /// Per-pipeline status: queueStatus + latest-run summary, for every matched definition. + Status { + /// Path to the repository root (defaults to current directory). Used + /// to auto-discover compiled pipelines, same as `compile`. + path: Option, + /// Override: Azure DevOps organization (URL like `https://dev.azure.com/myorg`, + /// or just the org name `myorg`). Inferred from git remote by default. + #[arg(long)] + org: Option, + /// Override: Azure DevOps project name (inferred from git remote by default). + #[arg(long)] + project: Option, + /// PAT for ADO API authentication (prefer setting AZURE_DEVOPS_EXT_PAT env var; + /// Azure CLI fallback if omitted). + #[arg(long, env = "AZURE_DEVOPS_EXT_PAT")] + pat: Option, + /// Emit machine-readable JSON (same shape as `list --json`). + #[arg(long)] + json: bool, + }, } #[derive(Parser, Debug)] @@ -602,6 +623,7 @@ async fn main() -> Result<()> { Some(Commands::Disable { .. }) => "disable", Some(Commands::Remove { .. }) => "remove", Some(Commands::List { .. }) => "list", + Some(Commands::Status { .. }) => "status", None => "ado-aw", }; @@ -788,6 +810,22 @@ async fn main() -> Result<()> { }) .await?; } + Commands::Status { + path, + org, + project, + pat, + json, + } => { + status::run(status::StatusOptions { + org: org.as_deref(), + project: project.as_deref(), + pat: pat.as_deref(), + path: path.as_deref(), + json, + }) + .await?; + } } Ok(()) } diff --git a/src/status.rs b/src/status.rs new file mode 100644 index 00000000..749c6590 --- /dev/null +++ b/src/status.rs @@ -0,0 +1,237 @@ +//! The `status` CLI command. +//! +//! Renders the per-pipeline status for every ADO build definition +//! that matches a local fixture: name, id, folder, queueStatus, the +//! latest-run summary, and a deep link. Read-only. +//! +//! `status` is intentionally a thin renderer over the same data path +//! as `list` — same `list_definitions` + `match_definitions` + +//! `get_latest_build` sequence, just a denser per-pipeline block +//! instead of a table. The `--json` shape is byte-for-byte identical +//! to `list --json` so scripts can use either. + +use anyhow::{Context, Result}; +use std::collections::HashSet; +use std::path::{Path, PathBuf}; + +use crate::ado::{ + AdoAuth, AdoContext, get_latest_build, list_definitions, match_definitions, + resolve_ado_context, resolve_auth, +}; +use crate::detect; +use crate::list::{ListRow, build_rows, render_json}; + +/// CLI options for [`run`]. +pub struct StatusOptions<'a> { + pub org: Option<&'a str>, + pub project: Option<&'a str>, + pub pat: Option<&'a str>, + pub path: Option<&'a Path>, + pub json: bool, +} + +/// Pure renderer: dense per-pipeline block. +pub fn render_blocks(ado_org_url: &str, ado_project: &str, rows: &[ListRow]) -> String { + if rows.is_empty() { + return "(no matched definitions)\n".to_string(); + } + + let mut out = String::new(); + for r in rows { + out.push_str(&format!("● {}\n", r.name)); + out.push_str(&format!(" id: {}\n", r.id)); + if let Some(folder) = &r.folder { + out.push_str(&format!(" folder: {}\n", folder)); + } + out.push_str(&format!( + " queueStatus: {}\n", + r.queue_status.as_deref().unwrap_or("?") + )); + if let Some(yaml) = &r.yaml_filename { + out.push_str(&format!(" source: {}\n", yaml.trim_start_matches('/'))); + } + match &r.last_run { + Some(lr) => { + let result = lr + .result + .clone() + .or_else(|| lr.status.clone()) + .unwrap_or_else(|| "in progress".to_string()); + out.push_str(&format!(" last run: build {} — {}", lr.id, result)); + if let Some(t) = &lr.finish_time { + out.push_str(&format!(" @ {}", t)); + } + out.push('\n'); + let url = lr.url.clone().unwrap_or_else(|| { + format!( + "{}/{}/_build/results?buildId={}", + ado_org_url.trim_end_matches('/'), + ado_project, + lr.id, + ) + }); + out.push_str(&format!(" url: {}\n", url)); + } + None => { + out.push_str(" last run: never\n"); + } + } + out.push('\n'); + } + out +} + +/// Run the `status` command. +pub async fn run(opts: StatusOptions<'_>) -> Result<()> { + let repo_path: PathBuf = match opts.path { + Some(p) => tokio::fs::canonicalize(p) + .await + .with_context(|| format!("Could not resolve path: {}", p.display()))?, + None => tokio::fs::canonicalize(".") + .await + .context("Could not resolve current directory")?, + }; + + let auth = resolve_auth(opts.pat).await?; + let ado_ctx = resolve_ado_context(&repo_path, opts.org, opts.project).await?; + + let client = reqwest::Client::builder() + .timeout(std::time::Duration::from_secs(30)) + .build() + .context("Failed to create HTTP client")?; + + let definitions = list_definitions(&client, &ado_ctx, &auth).await?; + let detected = detect::detect_pipelines(&repo_path).await.unwrap_or_default(); + let matched = match_definitions(&client, &ado_ctx, &auth, &detected) + .await + .unwrap_or_default(); + + let target_ids: HashSet = matched.iter().map(|m| m.id).collect(); + let mut last_runs = std::collections::HashMap::new(); + for id in &target_ids { + match get_latest_build(&client, &ado_ctx, &auth, *id).await { + Ok(Some(v)) => { + last_runs.insert(*id, v); + } + Ok(None) => {} + Err(e) => { + eprintln!(" warning: failed to fetch latest build for {}: {:#}", id, e); + } + } + } + + let rows = build_rows(&definitions, &matched, &last_runs, false); + + if opts.json { + println!("{}", render_json(&rows)?); + } else { + print!("{}", render_blocks(&ado_ctx.org_url, &ado_ctx.project, &rows)); + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::list::LastRun; + + fn row_with_run(id: u64, name: &str, status: Option<&str>, last_run: Option) -> ListRow { + ListRow { + id, + name: name.to_string(), + folder: Some("\\smoke".to_string()), + queue_status: status.map(str::to_string), + yaml_filename: Some(format!("/tests/{}.lock.yml", name)), + matched: true, + last_run, + } + } + + #[test] + fn empty_renders_placeholder() { + let out = render_blocks("https://dev.azure.com/o", "p", &[]); + assert_eq!(out, "(no matched definitions)\n"); + } + + #[test] + fn block_shows_succeeded_run_with_url() { + let row = row_with_run( + 123, + "noop", + Some("enabled"), + Some(LastRun { + id: 999, + result: Some("succeeded".to_string()), + status: Some("completed".to_string()), + finish_time: Some("2026-05-17T08:00:00Z".to_string()), + url: Some("https://dev.azure.com/.../999".to_string()), + raw: serde_json::Value::Null, + }), + ); + let out = render_blocks("https://dev.azure.com/o", "p", &[row]); + assert!(out.contains("● noop")); + assert!(out.contains("id: 123")); + assert!(out.contains("folder: \\smoke")); + assert!(out.contains("queueStatus: enabled")); + assert!(out.contains("source: tests/noop.lock.yml")); + assert!(out.contains("last run: build 999 — succeeded")); + assert!(out.contains("2026-05-17T08:00:00Z")); + assert!(out.contains("https://dev.azure.com/.../999")); + } + + #[test] + fn block_synthesizes_url_when_missing() { + let row = row_with_run( + 42, + "x", + Some("disabled"), + Some(LastRun { + id: 7, + result: Some("failed".to_string()), + status: Some("completed".to_string()), + finish_time: None, + url: None, + raw: serde_json::Value::Null, + }), + ); + let out = render_blocks("https://dev.azure.com/myorg/", "myproject", &[row]); + assert!( + out.contains("https://dev.azure.com/myorg/myproject/_build/results?buildId=7"), + "expected synthesized URL in:\n{out}" + ); + } + + #[test] + fn block_shows_never_when_no_last_run() { + let row = row_with_run(1, "x", Some("enabled"), None); + let out = render_blocks("o", "p", &[row]); + assert!(out.contains("last run: never")); + assert!(!out.contains("url:")); + } + + #[test] + fn block_shows_in_progress_when_no_result_yet() { + let row = row_with_run( + 1, + "x", + Some("enabled"), + Some(LastRun { + id: 10, + result: None, + status: Some("inProgress".to_string()), + finish_time: None, + url: None, + raw: serde_json::Value::Null, + }), + ); + let out = render_blocks("o", "p", &[row]); + assert!(out.contains("build 10 — inProgress")); + } + + #[test] + fn block_uses_question_mark_when_queue_status_missing() { + let row = row_with_run(1, "x", None, None); + let out = render_blocks("o", "p", &[row]); + assert!(out.contains("queueStatus: ?")); + } +} diff --git a/tests/status_integration.rs b/tests/status_integration.rs new file mode 100644 index 00000000..2a362bbf --- /dev/null +++ b/tests/status_integration.rs @@ -0,0 +1,41 @@ +//! Integration tests for `ado-aw status`. + +use std::path::PathBuf; + +fn binary() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_ado-aw")) +} + +#[test] +fn status_help_describes_command() { + let output = std::process::Command::new(binary()) + .args(["status", "--help"]) + .output() + .expect("Failed to run ado-aw status --help"); + assert!(output.status.success(), "--help should exit 0"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("Per-pipeline status"), + "Help text should describe the status command, got:\n{stdout}" + ); + for flag in ["--org", "--project", "--pat", "--json"] { + assert!( + stdout.contains(flag), + "Expected --help to advertise {flag}, got:\n{stdout}" + ); + } +} + +#[test] +fn status_is_in_top_level_help() { + let output = std::process::Command::new(binary()) + .arg("--help") + .output() + .expect("Failed to run ado-aw --help"); + assert!(output.status.success(), "--help should exit 0"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("status"), + "Top-level --help should mention the status subcommand, got:\n{stdout}" + ); +} From ffb84c57ddc41861cc5a05e3830133dcca6cb505 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 17 May 2026 13:18:02 +0000 Subject: [PATCH 05/14] fix(cli): avoid duplicate ADO definition fetch in list and status Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/c7c2866a-891d-48c3-8336-774bba086817 Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> --- src/ado/mod.rs | 44 ++++++++++++++++++++++++++++---------------- src/list.rs | 15 +++------------ src/status.rs | 7 ++----- 3 files changed, 33 insertions(+), 33 deletions(-) diff --git a/src/ado/mod.rs b/src/ado/mod.rs index 633b3293..700d5059 100644 --- a/src/ado/mod.rs +++ b/src/ado/mod.rs @@ -376,24 +376,14 @@ pub fn normalize_ado_yaml_path(path: &str) -> String { /// Strategy: /// 1. Try to match by the `yamlFilename` field in the definition's process config /// 2. Fall back to matching by pipeline name containing the agent name -pub async fn match_definitions( - client: &reqwest::Client, - ctx: &AdoContext, - auth: &AdoAuth, +pub fn match_definitions_in( + definitions: &[DefinitionSummary], detected: &[detect::DetectedPipeline], -) -> Result> { - let definitions = list_definitions(client, ctx, auth).await?; - info!( - "Found {} pipeline definitions in {}/{}", - definitions.len(), - ctx.org_url, - ctx.project - ); - +) -> Vec { let mut matched = Vec::new(); // Log all definition yaml paths for debugging - for def in &definitions { + for def in definitions { let yaml_path = def .process .as_ref() @@ -447,7 +437,7 @@ pub async fn match_definitions( .and_then(|s| s.to_str()) .unwrap_or(""); - match fuzzy_match_by_name(agent_name, &definitions) { + match fuzzy_match_by_name(agent_name, definitions) { FuzzyMatchResult::Single(idx) => { let def = &definitions[idx]; eprintln!( @@ -481,7 +471,29 @@ pub async fn match_definitions( ); } - Ok(matched) + matched +} + +/// Match detected pipeline YAML files to ADO pipeline definitions. +/// +/// Strategy: +/// 1. Try to match by the `yamlFilename` field in the definition's process config +/// 2. Fall back to matching by pipeline name containing the agent name +pub async fn match_definitions( + client: &reqwest::Client, + ctx: &AdoContext, + auth: &AdoAuth, + detected: &[detect::DetectedPipeline], +) -> Result> { + let definitions = list_definitions(client, ctx, auth).await?; + info!( + "Found {} pipeline definitions in {}/{}", + definitions.len(), + ctx.org_url, + ctx.project + ); + + Ok(match_definitions_in(&definitions, detected)) } /// Fetch the human-readable name of a pipeline definition by ID. diff --git a/src/list.rs b/src/list.rs index 39f4d643..8de325c8 100644 --- a/src/list.rs +++ b/src/list.rs @@ -15,8 +15,8 @@ use std::collections::HashSet; use std::path::{Path, PathBuf}; use crate::ado::{ - DefinitionSummary, MatchedDefinition, get_latest_build, list_definitions, match_definitions, - resolve_ado_context, resolve_auth, + DefinitionSummary, MatchedDefinition, get_latest_build, list_definitions, + match_definitions_in, resolve_ado_context, resolve_auth, }; use crate::detect; @@ -255,16 +255,7 @@ pub async fn run(opts: ListOptions<'_>) -> Result<()> { let definitions = list_definitions(&client, &ado_ctx, &auth).await?; let detected = detect::detect_pipelines(&repo_path).await.unwrap_or_default(); - let matched = match match_definitions(&client, &ado_ctx, &auth, &detected).await { - Ok(m) => m, - Err(e) => { - eprintln!( - " warning: failed to match local pipeline files with ADO definitions: {:#}; continuing with unmatched results", - e - ); - Vec::new() - } - }; + let matched = match_definitions_in(&definitions, &detected); // Decide which IDs need a last-build fetch. let target_ids: HashSet = if opts.all { diff --git a/src/status.rs b/src/status.rs index 749c6590..01fca907 100644 --- a/src/status.rs +++ b/src/status.rs @@ -15,8 +15,7 @@ use std::collections::HashSet; use std::path::{Path, PathBuf}; use crate::ado::{ - AdoAuth, AdoContext, get_latest_build, list_definitions, match_definitions, - resolve_ado_context, resolve_auth, + get_latest_build, list_definitions, match_definitions_in, resolve_ado_context, resolve_auth, }; use crate::detect; use crate::list::{ListRow, build_rows, render_json}; @@ -102,9 +101,7 @@ pub async fn run(opts: StatusOptions<'_>) -> Result<()> { let definitions = list_definitions(&client, &ado_ctx, &auth).await?; let detected = detect::detect_pipelines(&repo_path).await.unwrap_or_default(); - let matched = match_definitions(&client, &ado_ctx, &auth, &detected) - .await - .unwrap_or_default(); + let matched = match_definitions_in(&definitions, &detected); let target_ids: HashSet = matched.iter().map(|m| m.id).collect(); let mut last_runs = std::collections::HashMap::new(); From b10c8aa07b00283c1436a0ef5da0689b892b3406 Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 17 May 2026 21:20:46 +0100 Subject: [PATCH 06/14] feat(cli): add ado-aw run Squash-merge of #595 (feat/cli-run). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- AGENTS.md | 1 + docs/cli.md | 11 ++ src/ado/mod.rs | 97 +++++++-- src/main.rs | 65 ++++++ src/run.rs | 415 +++++++++++++++++++++++++++++++++++++++ tests/run_integration.rs | 55 ++++++ 6 files changed, 632 insertions(+), 12 deletions(-) create mode 100644 src/run.rs create mode 100644 tests/run_integration.rs diff --git a/AGENTS.md b/AGENTS.md index c8f01df6..d099fe7c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -81,6 +81,7 @@ Every compiled pipeline runs as three sequential jobs: │ ├── remove.rs # `remove` CLI command — deletes matched ADO build definitions (with --yes / tty-prompt safety) │ ├── list.rs # `list` CLI command — renders matched ADO definitions with their latest-run state (text or JSON) │ ├── status.rs # `status` CLI command — denser per-pipeline status block (thin renderer over `list`'s data path) +│ ├── run.rs # `run` CLI command — queues builds for matched definitions, optional polling to completion (module entry is `dispatch`) │ ├── ado/ # Shared Azure DevOps REST helpers (auth, list/match/PATCH/POST) │ │ └── mod.rs # Used by `configure` and the `enable` command (ADO REST helpers: auth, list/match/PATCH/POST) │ ├── detect.rs # Agentic pipeline detection (helper for `configure`) diff --git a/docs/cli.md b/docs/cli.md index dd58f9b8..c39a8333 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -83,3 +83,14 @@ Global flags (apply to all subcommands): `--verbose, -v` (enable info-level logg - `--pat ` / `AZURE_DEVOPS_EXT_PAT` env var - PAT for ADO API authentication (Azure CLI fallback if omitted). - `--json` - Emit machine-readable JSON (same shape as `list --json`). +- `run [PATH]` - Queue an ADO build for every ADO definition that matches a local fixture (under `PATH`). With `--wait`, poll each queued build until completion and exit with an aggregate result — 0 only if every queued build succeeded. + - `--org ` - Override: Azure DevOps organization (URL or bare org name). Inferred from git remote by default. + - `--project ` - Override: Azure DevOps project name (inferred from git remote by default). + - `--pat ` / `AZURE_DEVOPS_EXT_PAT` env var - PAT for ADO API authentication (Azure CLI fallback if omitted). + - `--branch ` - Source branch to queue. Defaults to the definition's `defaultBranch`. + - `--parameters ` - ADO `templateParameters`. Repeatable and/or comma-separated. All values are strings (ADO coerces as the definition requires). Rejects malformed pairs (missing `=`). + - `--wait` - Poll each queued build to completion before exiting. + - `--poll-interval ` - Polling period when `--wait` is set (default 10). + - `--timeout ` - Hard cap on the polling loop when `--wait` is set (default 1800). + - `--dry-run` - Print the planned `templateParameters` body without calling the ADO API. + diff --git a/src/ado/mod.rs b/src/ado/mod.rs index 700d5059..a3b4210f 100644 --- a/src/ado/mod.rs +++ b/src/ado/mod.rs @@ -1089,26 +1089,99 @@ pub async fn create_definition( /// build's `id`. `branch` defaults to the definition's `defaultBranch` when /// `None`. `parameters` are passed through as ADO `templateParameters`. pub async fn queue_build( - _client: &reqwest::Client, - _ctx: &AdoContext, - _auth: &AdoAuth, - _definition_id: u64, - _branch: Option<&str>, - _parameters: &serde_json::Map, + client: &reqwest::Client, + ctx: &AdoContext, + auth: &AdoAuth, + definition_id: u64, + branch: Option<&str>, + parameters: &serde_json::Map, ) -> Result { - anyhow::bail!("not yet implemented: filled in by PR 6 (ado-aw run)") + let url = format!( + "{}/{}/_apis/build/builds?api-version=7.1", + ctx.org_url.trim_end_matches('/'), + ctx.project, + ); + + let mut body = serde_json::json!({ + "definition": { "id": definition_id } + }); + if let Some(b) = branch { + body["sourceBranch"] = serde_json::Value::String(b.to_string()); + } + if !parameters.is_empty() { + body["templateParameters"] = serde_json::Value::Object(parameters.clone()); + } + + debug!("POST queue build for definition {}: {}", definition_id, url); + + let resp = auth + .apply(client.post(&url)) + .header("Content-Type", "application/json") + .json(&body) + .send() + .await + .with_context(|| format!("Failed to queue build for definition {}", definition_id))?; + + let status = resp.status(); + if !status.is_success() { + let resp_body = resp.text().await.unwrap_or_default(); + anyhow::bail!( + "ADO API returned {} when queuing build for definition {}: {}", + status, + definition_id, + resp_body + ); + } + + let resp_body: serde_json::Value = resp + .json() + .await + .context("Failed to parse queue-build response")?; + + resp_body + .get("id") + .and_then(|v| v.as_u64()) + .context("queue_build response has no numeric 'id' field") } /// Fetch the full JSON body of a build. /// /// Calls `GET /_apis/build/builds/{id}?api-version=7.1`. pub async fn get_build( - _client: &reqwest::Client, - _ctx: &AdoContext, - _auth: &AdoAuth, - _build_id: u64, + client: &reqwest::Client, + ctx: &AdoContext, + auth: &AdoAuth, + build_id: u64, ) -> Result { - anyhow::bail!("not yet implemented: filled in by PR 6 (ado-aw run)") + let url = format!( + "{}/{}/_apis/build/builds/{}?api-version=7.1", + ctx.org_url.trim_end_matches('/'), + ctx.project, + build_id + ); + + debug!("GET build {}: {}", build_id, url); + + let resp = auth + .apply(client.get(&url)) + .send() + .await + .with_context(|| format!("Failed to fetch build {}", build_id))?; + + let status = resp.status(); + if !status.is_success() { + let body = resp.text().await.unwrap_or_default(); + anyhow::bail!( + "ADO API returned {} when fetching build {}: {}", + status, + build_id, + body + ); + } + + resp.json() + .await + .with_context(|| format!("Failed to parse build {} response", build_id)) } /// Fetch the most recent build for a definition. diff --git a/src/main.rs b/src/main.rs index 3ff98de8..6921c931 100644 --- a/src/main.rs +++ b/src/main.rs @@ -17,6 +17,7 @@ mod logging; mod mcp; mod ndjson; mod remove; +mod run; pub mod runtimes; pub mod sanitize; mod safeoutputs; @@ -270,6 +271,43 @@ enum Commands { #[arg(long)] json: bool, }, + /// Queue a build for every ADO definition that matches a local fixture (optionally wait for completion). + Run { + /// Path to the repository root (defaults to current directory). Used + /// to auto-discover compiled pipelines, same as `compile`. + path: Option, + /// Override: Azure DevOps organization (URL like `https://dev.azure.com/myorg`, + /// or just the org name `myorg`). Inferred from git remote by default. + #[arg(long)] + org: Option, + /// Override: Azure DevOps project name (inferred from git remote by default). + #[arg(long)] + project: Option, + /// PAT for ADO API authentication (prefer setting AZURE_DEVOPS_EXT_PAT env var; + /// Azure CLI fallback if omitted). + #[arg(long, env = "AZURE_DEVOPS_EXT_PAT")] + pat: Option, + /// Source branch to queue. Defaults to the definition's `defaultBranch`. + #[arg(long)] + branch: Option, + /// ADO `templateParameters` as `key=value` pairs. Repeatable and/or + /// comma-separated (`--parameters a=1,b=2 --parameters c=3`). + #[arg(long)] + parameters: Vec, + /// Poll each queued build to completion before exiting; aggregate result + /// determines the exit code. + #[arg(long)] + wait: bool, + /// Seconds between polls when `--wait` is set. + #[arg(long, default_value_t = 10, requires = "wait")] + poll_interval: u64, + /// Maximum seconds to wait when `--wait` is set. + #[arg(long, default_value_t = 1800, requires = "wait")] + timeout: u64, + /// Print the planned queue body without calling the ADO API. + #[arg(long)] + dry_run: bool, + }, } #[derive(Parser, Debug)] @@ -624,6 +662,7 @@ async fn main() -> Result<()> { Some(Commands::Remove { .. }) => "remove", Some(Commands::List { .. }) => "list", Some(Commands::Status { .. }) => "status", + Some(Commands::Run { .. }) => "run", None => "ado-aw", }; @@ -826,6 +865,32 @@ async fn main() -> Result<()> { }) .await?; } + Commands::Run { + path, + org, + project, + pat, + branch, + parameters, + wait, + poll_interval, + timeout, + dry_run, + } => { + run::dispatch(run::RunOptions { + org: org.as_deref(), + project: project.as_deref(), + pat: pat.as_deref(), + path: path.as_deref(), + branch: branch.as_deref(), + parameters: ¶meters, + wait, + poll_interval_secs: poll_interval, + timeout_secs: timeout, + dry_run, + }) + .await?; + } } Ok(()) } diff --git a/src/run.rs b/src/run.rs new file mode 100644 index 00000000..2e798f8e --- /dev/null +++ b/src/run.rs @@ -0,0 +1,415 @@ +//! The `run` CLI command. +//! +//! Queues a build for every ADO definition that matches a local +//! fixture. With `--wait`, polls each queued build until completion +//! and exits with a status code that reflects the aggregate result. +//! Phase 1 of the pipeline-lifecycle CLI family — see `docs/cli.md`. +//! +//! Naming nit: the module-level entry point is `dispatch`, not `run`, +//! so call sites don't end up reading `run::run(...)`. Don't rename +//! it back to `run` — future contributors will find this comment if +//! they try. + +use anyhow::{Context, Result}; +use std::path::{Path, PathBuf}; +use std::time::{Duration, Instant}; + +use crate::ado::{ + AdoAuth, AdoContext, MatchedDefinition, get_build, match_definitions, queue_build, + resolve_ado_context, resolve_auth, +}; +use crate::detect; + +/// Parse `--parameters foo=bar,baz=qux` (and its repeatable form) into +/// a JSON map. Pure function; reject malformed pairs. +pub fn parse_parameters(values: &[String]) -> Result> { + let mut out = serde_json::Map::new(); + for raw in values { + for pair in raw.split(',') { + let pair = pair.trim(); + if pair.is_empty() { + continue; + } + let Some((k, v)) = pair.split_once('=') else { + anyhow::bail!( + "Invalid --parameters pair '{}': expected key=value (no '=' found).", + pair + ); + }; + let key = k.trim(); + if key.is_empty() { + anyhow::bail!("Invalid --parameters pair '{}': empty key.", pair); + } + // All values are strings — ADO coerces template-parameter + // values as the pipeline definition requires. + out.insert(key.to_string(), serde_json::Value::String(v.trim().to_string())); + } + } + Ok(out) +} + +/// Build a `(definition_id, queued_build_id)` poll-target pair. +#[derive(Debug, Clone, Copy)] +struct PollTarget { + definition_id: u64, + build_id: u64, +} + +/// Pure decision: given an ADO build JSON body, what's the terminal +/// state from the operator's perspective? +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum BuildOutcome { + /// `status` is anything but `completed`. Keep polling. + InProgress, + /// `status == "completed"` and `result == "succeeded"`. + Succeeded, + /// `status == "completed"` and `result` is anything else (failed, + /// canceled, partiallySucceeded). + Failed, +} + +/// Pure function: classify a build's terminal state from its JSON +/// body. Tested independently of any HTTP code. +pub fn classify_build(body: &serde_json::Value) -> BuildOutcome { + let status = body.get("status").and_then(|v| v.as_str()).unwrap_or(""); + if status != "completed" { + return BuildOutcome::InProgress; + } + let result = body.get("result").and_then(|v| v.as_str()).unwrap_or(""); + if result == "succeeded" { + BuildOutcome::Succeeded + } else { + BuildOutcome::Failed + } +} + +/// CLI options for [`dispatch`]. +pub struct RunOptions<'a> { + pub org: Option<&'a str>, + pub project: Option<&'a str>, + pub pat: Option<&'a str>, + pub path: Option<&'a Path>, + pub branch: Option<&'a str>, + /// Raw `--parameters` arguments (one entry per CLI occurrence). + pub parameters: &'a [String], + pub wait: bool, + pub poll_interval_secs: u64, + pub timeout_secs: u64, + pub dry_run: bool, +} + +/// Run the `run` command — kept as `dispatch` to avoid the awkward +/// `run::run(...)` call site that a plain `run` would produce. See the +/// module-level comment. +pub async fn dispatch(opts: RunOptions<'_>) -> Result<()> { + let parameters = parse_parameters(opts.parameters)?; + + let repo_path: PathBuf = match opts.path { + Some(p) => tokio::fs::canonicalize(p) + .await + .with_context(|| format!("Could not resolve path: {}", p.display()))?, + None => tokio::fs::canonicalize(".") + .await + .context("Could not resolve current directory")?, + }; + + let auth = resolve_auth(opts.pat).await?; + let ado_ctx = resolve_ado_context(&repo_path, opts.org, opts.project).await?; + + let client = reqwest::Client::builder() + .timeout(Duration::from_secs(30)) + .build() + .context("Failed to create HTTP client")?; + + println!("Scanning for agentic pipelines..."); + let detected = detect::detect_pipelines(&repo_path).await?; + if detected.is_empty() { + println!("No agentic pipelines found."); + return Ok(()); + } + + let matched = match_definitions(&client, &ado_ctx, &auth, &detected).await?; + if matched.is_empty() { + anyhow::bail!( + "No ADO definitions matched any local fixture. Run `ado-aw list` to \ + diagnose." + ); + } + + println!("{} definition(s) to queue.", matched.len()); + println!(); + + if opts.dry_run { + for m in &matched { + print_queue_plan(m, opts.branch, ¶meters); + } + return Ok(()); + } + + let mut targets: Vec = Vec::new(); + let mut queue_failure = 0usize; + + for m in &matched { + match queue_build(&client, &ado_ctx, &auth, m.id, opts.branch, ¶meters).await { + Ok(build_id) => { + println!( + "▶ queued: {} (id={}) → build {} at {}/{}/_build/results?buildId={}", + m.name, + m.id, + build_id, + ado_ctx.org_url.trim_end_matches('/'), + ado_ctx.project, + build_id + ); + targets.push(PollTarget { + definition_id: m.id, + build_id, + }); + } + Err(e) => { + eprintln!("✗ failed to queue: {} (id={}): {:#}", m.name, m.id, e); + queue_failure += 1; + } + } + } + + if !opts.wait { + println!(); + println!( + "Queued {} build(s); {} failed to queue.", + targets.len(), + queue_failure + ); + if queue_failure > 0 { + anyhow::bail!("{} build(s) failed to queue", queue_failure); + } + return Ok(()); + } + + let poll_outcome = poll_until_complete( + &client, + &ado_ctx, + &auth, + &targets, + Duration::from_secs(opts.poll_interval_secs), + Duration::from_secs(opts.timeout_secs), + ) + .await?; + + println!(); + println!( + "Wait summary: {} succeeded, {} failed, {} still in progress (timeout), {} failed to queue.", + poll_outcome.succeeded, poll_outcome.failed, poll_outcome.in_progress, queue_failure, + ); + + let non_success = poll_outcome.failed + poll_outcome.in_progress + queue_failure; + if non_success > 0 { + anyhow::bail!("not all builds succeeded"); + } + Ok(()) +} + +fn print_queue_plan( + m: &MatchedDefinition, + branch: Option<&str>, + parameters: &serde_json::Map, +) { + let mut body = serde_json::json!({ + "definition": { "id": m.id } + }); + if let Some(b) = branch { + body["sourceBranch"] = serde_json::Value::String(b.to_string()); + } + if !parameters.is_empty() { + body["templateParameters"] = serde_json::Value::Object(parameters.clone()); + } + println!("[dry-run] ▶ would queue: {} (id={})", m.name, m.id); + println!("{}", serde_json::to_string_pretty(&body).unwrap_or_default()); +} + +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] +struct PollOutcome { + succeeded: usize, + failed: usize, + in_progress: usize, +} + +async fn poll_until_complete( + client: &reqwest::Client, + ctx: &AdoContext, + auth: &AdoAuth, + targets: &[PollTarget], + poll_interval: Duration, + timeout: Duration, +) -> Result { + let started = Instant::now(); + let mut outcome = PollOutcome::default(); + let mut pending: Vec = targets.to_vec(); + + println!(); + println!( + "Waiting for {} build(s) (poll every {}s, timeout {}s)...", + pending.len(), + poll_interval.as_secs(), + timeout.as_secs() + ); + + while !pending.is_empty() { + if started.elapsed() >= timeout { + println!("⚠ wait timed out after {}s", timeout.as_secs()); + outcome.in_progress = pending.len(); + return Ok(outcome); + } + + let mut next_pending = Vec::new(); + for t in &pending { + match get_build(client, ctx, auth, t.build_id).await { + Ok(body) => match classify_build(&body) { + BuildOutcome::InProgress => next_pending.push(*t), + BuildOutcome::Succeeded => { + println!("✓ build {} (definition {}) succeeded", t.build_id, t.definition_id); + outcome.succeeded += 1; + } + BuildOutcome::Failed => { + let result = body + .get("result") + .and_then(|v| v.as_str()) + .unwrap_or("unknown"); + println!( + "✗ build {} (definition {}) finished with result={}", + t.build_id, t.definition_id, result + ); + outcome.failed += 1; + } + }, + Err(e) => { + eprintln!( + " warning: poll error for build {} (definition {}): {:#}", + t.build_id, t.definition_id, e + ); + // Treat transient poll errors as still-in-progress; + // we'll retry on the next tick. + next_pending.push(*t); + } + } + } + pending = next_pending; + + if !pending.is_empty() { + tokio::time::sleep(poll_interval).await; + } + } + + Ok(outcome) +} + +#[cfg(test)] +mod tests { + use super::*; + + // ============ parse_parameters ============ + + #[test] + fn parse_parameters_single_pair() { + let m = parse_parameters(&["foo=bar".to_string()]).unwrap(); + assert_eq!(m.get("foo").unwrap().as_str(), Some("bar")); + } + + #[test] + fn parse_parameters_comma_separated() { + let m = parse_parameters(&["foo=bar,baz=qux".to_string()]).unwrap(); + assert_eq!(m.get("foo").unwrap().as_str(), Some("bar")); + assert_eq!(m.get("baz").unwrap().as_str(), Some("qux")); + } + + #[test] + fn parse_parameters_repeated() { + let m = parse_parameters(&["a=1".to_string(), "b=2".to_string()]).unwrap(); + assert_eq!(m.get("a").unwrap().as_str(), Some("1")); + assert_eq!(m.get("b").unwrap().as_str(), Some("2")); + } + + #[test] + fn parse_parameters_repeated_comma_mix() { + let m = + parse_parameters(&["a=1,b=2".to_string(), "c=3".to_string()]).unwrap(); + assert_eq!(m.len(), 3); + } + + #[test] + fn parse_parameters_value_with_equals() { + // Split on first '=' only; subsequent equals are part of the value. + let m = parse_parameters(&["key=a=b=c".to_string()]).unwrap(); + assert_eq!(m.get("key").unwrap().as_str(), Some("a=b=c")); + } + + #[test] + fn parse_parameters_rejects_missing_equals() { + let err = parse_parameters(&["nope".to_string()]).unwrap_err(); + assert!(err.to_string().contains("no '='"), "got: {}", err); + } + + #[test] + fn parse_parameters_rejects_empty_key() { + let err = parse_parameters(&["=bar".to_string()]).unwrap_err(); + assert!(err.to_string().contains("empty key"), "got: {}", err); + } + + #[test] + fn parse_parameters_empty_input_returns_empty() { + let m = parse_parameters(&[]).unwrap(); + assert!(m.is_empty()); + } + + #[test] + fn parse_parameters_skips_blank_pairs() { + // Trailing/duplicate commas are forgiving. + let m = parse_parameters(&["foo=bar,,".to_string()]).unwrap(); + assert_eq!(m.len(), 1); + } + + // ============ classify_build ============ + + #[test] + fn classify_in_progress_when_status_not_completed() { + let body = serde_json::json!({ "status": "inProgress", "result": "succeeded" }); + assert_eq!(classify_build(&body), BuildOutcome::InProgress); + } + + #[test] + fn classify_in_progress_when_status_missing() { + let body = serde_json::json!({ "result": "succeeded" }); + assert_eq!(classify_build(&body), BuildOutcome::InProgress); + } + + #[test] + fn classify_succeeded_when_completed_and_succeeded() { + let body = serde_json::json!({ "status": "completed", "result": "succeeded" }); + assert_eq!(classify_build(&body), BuildOutcome::Succeeded); + } + + #[test] + fn classify_failed_when_completed_failed() { + let body = serde_json::json!({ "status": "completed", "result": "failed" }); + assert_eq!(classify_build(&body), BuildOutcome::Failed); + } + + #[test] + fn classify_failed_when_completed_canceled() { + let body = serde_json::json!({ "status": "completed", "result": "canceled" }); + assert_eq!(classify_build(&body), BuildOutcome::Failed); + } + + #[test] + fn classify_failed_when_completed_partial() { + let body = + serde_json::json!({ "status": "completed", "result": "partiallySucceeded" }); + assert_eq!(classify_build(&body), BuildOutcome::Failed); + } + + #[test] + fn classify_failed_when_completed_without_result() { + let body = serde_json::json!({ "status": "completed" }); + assert_eq!(classify_build(&body), BuildOutcome::Failed); + } +} diff --git a/tests/run_integration.rs b/tests/run_integration.rs new file mode 100644 index 00000000..f75b881d --- /dev/null +++ b/tests/run_integration.rs @@ -0,0 +1,55 @@ +//! Integration tests for `ado-aw run`. + +use std::path::PathBuf; + +fn binary() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_ado-aw")) +} + +#[test] +fn run_help_describes_command() { + let output = std::process::Command::new(binary()) + .args(["run", "--help"]) + .output() + .expect("Failed to run ado-aw run --help"); + assert!(output.status.success(), "--help should exit 0"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("Queue a build"), + "Help text should describe the run command, got:\n{stdout}" + ); + for flag in [ + "--org", + "--project", + "--pat", + "--branch", + "--parameters", + "--wait", + "--poll-interval", + "--timeout", + "--dry-run", + ] { + assert!( + stdout.contains(flag), + "Expected --help to advertise {flag}, got:\n{stdout}" + ); + } +} + +#[test] +fn run_rejects_poll_interval_without_wait() { + // clap should reject `--poll-interval` (and `--timeout`) when `--wait` is absent. + let output = std::process::Command::new(binary()) + .args(["run", "--poll-interval", "5"]) + .output() + .expect("Failed to run ado-aw run"); + assert!( + !output.status.success(), + "Expected non-zero exit when --poll-interval used without --wait" + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("--wait") || stderr.contains("wait"), + "stderr should reference the requires-constraint, got:\n{stderr}" + ); +} From 95e006fe0f747348f477c82a76839fd3917722d9 Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 17 May 2026 21:27:10 +0100 Subject: [PATCH 07/14] feat(cli): rename configure to secrets with subcommands Squash-merge of #600 (feat/cli-secrets). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- AGENTS.md | 3 +- docs/cli.md | 25 +- src/configure.rs | 147 +------- src/list.rs | 1 + src/main.rs | 137 +++++++- src/secrets.rs | 653 +++++++++++++++++++++++++++++++++++ tests/secrets_integration.rs | 119 +++++++ 7 files changed, 947 insertions(+), 138 deletions(-) create mode 100644 src/secrets.rs create mode 100644 tests/secrets_integration.rs diff --git a/AGENTS.md b/AGENTS.md index d099fe7c..298bcb53 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -75,7 +75,8 @@ Every compiled pipeline runs as three sequential jobs: │ ├── fuzzy_schedule.rs # Fuzzy schedule parsing │ ├── logging.rs # File-based logging infrastructure │ ├── mcp.rs # SafeOutputs MCP server (stdio + HTTP) -│ ├── configure.rs # `configure` CLI command — orchestration shim atop `src/ado/` +│ ├── configure.rs # `configure` CLI command (deprecated) — hidden alias forwarding to `secrets set GITHUB_TOKEN` +│ ├── secrets.rs # `secrets set/list/delete` subcommand group — manages pipeline variables (never prints values from `list`) │ ├── enable.rs # `enable` CLI command — registers ADO build definitions for compiled pipelines and ensures they are enabled │ ├── disable.rs # `disable` CLI command — sets queueStatus to disabled (default) or paused on matched definitions │ ├── remove.rs # `remove` CLI command — deletes matched ADO build definitions (with --yes / tty-prompt safety) diff --git a/docs/cli.md b/docs/cli.md index c39a8333..a514562f 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -35,14 +35,23 @@ Global flags (apply to all subcommands): `--verbose, -v` (enable info-level logg - `--ado-project ` - Azure DevOps project name override - `--dry-run` - Validate inputs but skip ADO API calls (useful for local testing and QA review) -- `configure` - Detect agentic pipelines in a local repository and update the `GITHUB_TOKEN` pipeline variable on their Azure DevOps build definitions - - `--token ` / `GITHUB_TOKEN` env var - The new GITHUB_TOKEN value (prompted if omitted) - - `--org ` - Override: Azure DevOps organization URL (e.g. `https://dev.azure.com/myorg`) or just the org name (e.g. `myorg`, auto-prefixed to the canonical URL). Inferred from git remote by default. - - `--project ` - Override: Azure DevOps project name (inferred from git remote by default) - - `--pat ` / `AZURE_DEVOPS_EXT_PAT` env var - PAT for ADO API authentication (prompted if omitted) - - `--path ` - Path to the repository root (defaults to current directory) - - `--dry-run` - Preview changes without applying them - - `--definition-ids ` - Explicit pipeline definition IDs to update (comma-separated, skips auto-detection) +- `configure` *(deprecated; hidden in --help)* - Alias forwarding to `secrets set GITHUB_TOKEN`. Existing scripts keep working but get a stderr warning. The alias will be removed in the next minor release. + +- `secrets set [] [PATH]` - Set a pipeline variable (with `isSecret=true`) on every matched ADO definition. Value resolution: positional `` → `--value-stdin` (one line) → interactive tty prompt with echo off. + - `--allow-override` - Mark the variable as `allowOverride=true` (default: false). + - `--value-stdin` - Read the value from a single line on stdin. + - `--dry-run` - Print the planned set without calling the ADO API. + - `--org / --project / --pat` - ADO context overrides (same semantics as the other lifecycle commands). + - `--definition-ids ` - Explicit pipeline definition IDs (comma-separated; skips local-fixture auto-detection). + +- `secrets list [PATH]` - List variable names and their `isSecret` / `allowOverride` flags on every matched definition. **Never prints values.** + - `--json` - Emit machine-readable JSON. + - `--org / --project / --pat / --definition-ids` - As above. + +- `secrets delete [PATH]` - Delete the named variable from every matched definition. No-op when the variable is absent. + - `--dry-run` - Print the planned deletion plan without calling the ADO API. + - `--org / --project / --pat / --definition-ids` - As above. + - `enable [PATH]` - Register an ADO build definition for each compiled pipeline discovered under `PATH` (or the current directory) and ensure it is `enabled`. For each fixture, matches against the existing ADO definitions by `yamlFilename` first, then by sanitized display name; creates a new definition when neither matches, flips `queueStatus` to `enabled` when an existing definition is `disabled` / `paused`, and skips when it is already `enabled`. Fail-soft per fixture; exits non-zero if any fixture failed. - `--org ` - Override: Azure DevOps organization (URL or bare org name). Inferred from git remote by default. diff --git a/src/configure.rs b/src/configure.rs index d0eb678e..18eb4055 100644 --- a/src/configure.rs +++ b/src/configure.rs @@ -1,70 +1,16 @@ -//! The `configure` CLI command. +//! The `configure` CLI command (deprecated). //! -//! Detects agentic pipelines in a local repository and updates the -//! `GITHUB_TOKEN` pipeline variable on their corresponding Azure DevOps -//! build definitions. -//! -//! Note: this command is being renamed to `secrets set GITHUB_TOKEN` as -//! part of the Phase 1 CLI overhaul. The current entry point remains the -//! orchestration shim below; all shared ADO REST logic lives in -//! [`crate::ado`]. +//! Sets `GITHUB_TOKEN` on every matched ADO definition. This command +//! is retained as a hidden deprecation alias forwarding to +//! [`crate::secrets::run_set_github_token`]; new code should use +//! `ado-aw secrets set GITHUB_TOKEN ` instead. -use anyhow::{Context, Result}; +use anyhow::Result; use std::path::Path; -use crate::ado::{ - AdoAuth, AdoContext, MatchedDefinition, resolve_ado_context, resolve_auth, - resolve_definitions, update_pipeline_variable, -}; - -/// Resolves the GitHub token from the CLI flag or an interactive prompt. -fn resolve_token(token: Option<&str>) -> Result { - match token { - Some(t) => Ok(t.to_string()), - None => inquire::Password::new("Enter the new GITHUB_TOKEN:") - .without_confirmation() - .prompt() - .context("Failed to read token from interactive prompt"), - } -} - -/// Updates the `GITHUB_TOKEN` variable on every matched pipeline -/// definition and reports per-definition success/failure. -async fn apply_token_updates( - client: &reqwest::Client, - ado_ctx: &AdoContext, - auth: &AdoAuth, - matched: &[MatchedDefinition], - token: &str, -) -> Result<()> { - println!("Updating GITHUB_TOKEN on matched definitions..."); - let mut success_count = 0; - let mut failure_count = 0; - - for m in matched { - match update_pipeline_variable(client, ado_ctx, auth, m.id, "GITHUB_TOKEN", token).await { - Ok(()) => { - println!(" \u{2713} Updated '{}' (id={})", m.name, m.id); - success_count += 1; - } - Err(e) => { - eprintln!(" \u{2717} Failed to update '{}' (id={}): {}", m.name, m.id, e); - failure_count += 1; - } - } - } - - println!(); - println!("Done: {} updated, {} failed.", success_count, failure_count); - - if failure_count > 0 { - anyhow::bail!("{} definition(s) failed to update", failure_count); - } - - Ok(()) -} - -/// Run the configure command. +/// Forwarder for the legacy `configure --token` invocation. Emits a +/// deprecation warning to stderr and forwards to the unified +/// `secrets set GITHUB_TOKEN` code path. pub async fn run( token: Option<&str>, org: Option<&str>, @@ -74,69 +20,14 @@ pub async fn run( dry_run: bool, definition_ids: Option<&[u64]>, ) -> Result<()> { - let repo_path = match path { - Some(p) => tokio::fs::canonicalize(p) - .await - .with_context(|| format!("Could not resolve path: {}", p.display()))?, - None => tokio::fs::canonicalize(".") - .await - .context("Could not resolve current directory")?, - }; - - let token = resolve_token(token)?; - let auth = resolve_auth(pat).await?; - let ado_ctx = resolve_ado_context(&repo_path, org, project).await?; - - println!( - "ADO context: org={}, project={}{}", - ado_ctx.org_url, - ado_ctx.project, - if ado_ctx.repo_name.is_empty() { - String::new() - } else { - format!(", repo={}", ado_ctx.repo_name) - } - ); - println!(); - - let client = reqwest::Client::builder() - .timeout(std::time::Duration::from_secs(30)) - .build() - .context("Failed to create HTTP client")?; - - let Some(matched) = - resolve_definitions(&client, &ado_ctx, &auth, definition_ids, &repo_path).await? - else { - return Ok(()); - }; - - if matched.is_empty() { - println!("No matching ADO pipeline definitions found."); - println!("Make sure your pipelines are registered in Azure DevOps and point to the detected YAML files."); - return Ok(()); - } - - println!("{} definition(s) to update:", matched.len()); - for m in &matched { - if m.yaml_path.is_empty() { - println!(" [{}] '{}' (id={})", m.match_method, m.name, m.id); - } else { - println!( - " [{}] '{}' (id={}) \u{2190} {}", - m.match_method, m.name, m.id, m.yaml_path - ); - } - } - println!(); - - if dry_run { - println!("Dry run \u{2014} no changes applied."); - println!( - "Would update GITHUB_TOKEN on {} definition(s).", - matched.len() - ); - return Ok(()); - } - - apply_token_updates(&client, &ado_ctx, &auth, &matched, &token).await + crate::secrets::run_set_github_token( + token, + org, + project, + pat, + path, + dry_run, + definition_ids, + ) + .await } diff --git a/src/list.rs b/src/list.rs index 8de325c8..31a6d1ac 100644 --- a/src/list.rs +++ b/src/list.rs @@ -314,6 +314,7 @@ mod tests { name: name.to_string(), match_method: MatchMethod::YamlPath, yaml_path: yaml.to_string(), + queue_status: None, } } diff --git a/src/main.rs b/src/main.rs index 6921c931..ef9c2fbd 100644 --- a/src/main.rs +++ b/src/main.rs @@ -21,6 +21,7 @@ mod run; pub mod runtimes; pub mod sanitize; mod safeoutputs; +mod secrets; mod status; mod tools; pub mod validate; @@ -29,6 +30,66 @@ use anyhow::{Context, Result}; use clap::{Parser, Subcommand}; use std::path::{Path, PathBuf}; +#[derive(Subcommand, Debug)] +enum SecretsCmd { + /// Set a pipeline variable on every matched definition (isSecret=true). + Set { + /// Variable name to set (e.g. `GITHUB_TOKEN`). + name: String, + /// Variable value. If omitted, falls back to `--value-stdin` or an + /// interactive tty prompt with echo off. + value: Option, + /// Path to the repository root (defaults to current directory). + path: Option, + #[arg(long)] + org: Option, + #[arg(long)] + project: Option, + #[arg(long, env = "AZURE_DEVOPS_EXT_PAT")] + pat: Option, + /// Mark the variable as `allowOverride=true` (default: false). + #[arg(long)] + allow_override: bool, + /// Read the value from a single line on stdin. + #[arg(long)] + value_stdin: bool, + #[arg(long)] + dry_run: bool, + /// Explicit definition IDs (skips local-fixture auto-detection). + #[arg(long, value_delimiter = ',')] + definition_ids: Option>, + }, + /// List variable names + flags on every matched definition. Never prints values. + List { + path: Option, + #[arg(long)] + org: Option, + #[arg(long)] + project: Option, + #[arg(long, env = "AZURE_DEVOPS_EXT_PAT")] + pat: Option, + #[arg(long)] + json: bool, + #[arg(long, value_delimiter = ',')] + definition_ids: Option>, + }, + /// Delete a named variable from every matched definition. + Delete { + name: String, + path: Option, + #[arg(long)] + org: Option, + #[arg(long)] + project: Option, + #[arg(long, env = "AZURE_DEVOPS_EXT_PAT")] + pat: Option, + #[arg(long)] + dry_run: bool, + #[arg(long, value_delimiter = ',')] + definition_ids: Option>, + }, +} + #[derive(Subcommand, Debug)] enum Commands { /// Compile markdown to pipeline definition (or recompile all detected pipelines) @@ -120,7 +181,9 @@ enum Commands { #[arg(long)] force: bool, }, - /// Detect agentic pipelines and update GITHUB_TOKEN on their ADO definitions + /// (Deprecated) Set GITHUB_TOKEN on every matched ADO definition. + /// Use `secrets set GITHUB_TOKEN ` instead. + #[command(hide = true)] Configure { /// The new GITHUB_TOKEN value (defaults to GITHUB_TOKEN env var; prompted if omitted) #[arg(long, env = "GITHUB_TOKEN")] @@ -145,6 +208,11 @@ enum Commands { #[arg(long, value_delimiter = ',')] definition_ids: Option>, }, + /// Manage pipeline-variable secrets on every matched ADO definition. + Secrets { + #[command(subcommand)] + action: SecretsCmd, + }, /// Register an ADO build definition for each compiled pipeline and ensure it's enabled. Enable { /// Path to the repository root (defaults to current directory). Used @@ -657,6 +725,7 @@ async fn main() -> Result<()> { Some(Commands::McpHttp { .. }) => "mcp-http", Some(Commands::Init { .. }) => "init", Some(Commands::Configure { .. }) => "configure", + Some(Commands::Secrets { .. }) => "secrets", Some(Commands::Enable { .. }) => "enable", Some(Commands::Disable { .. }) => "disable", Some(Commands::Remove { .. }) => "remove", @@ -771,6 +840,72 @@ async fn main() -> Result<()> { ) .await?; } + Commands::Secrets { action } => match action { + SecretsCmd::Set { + name, + value, + path, + org, + project, + pat, + allow_override, + value_stdin, + dry_run, + definition_ids, + } => { + secrets::run_set(secrets::SetOptions { + name: &name, + value: value.as_deref(), + org: org.as_deref(), + project: project.as_deref(), + pat: pat.as_deref(), + path: path.as_deref(), + allow_override, + value_stdin, + dry_run, + definition_ids: definition_ids.as_deref(), + }) + .await?; + } + SecretsCmd::List { + path, + org, + project, + pat, + json, + definition_ids, + } => { + secrets::run_list(secrets::ListOptions { + org: org.as_deref(), + project: project.as_deref(), + pat: pat.as_deref(), + path: path.as_deref(), + json, + definition_ids: definition_ids.as_deref(), + }) + .await?; + } + SecretsCmd::Delete { + name, + path, + org, + project, + pat, + dry_run, + definition_ids, + } => { + secrets::run_delete(secrets::DeleteOptions { + name: &name, + org: org.as_deref(), + project: project.as_deref(), + pat: pat.as_deref(), + path: path.as_deref(), + dry_run, + definition_ids: definition_ids.as_deref(), + }) + .await?; + } + }, Commands::Enable { path, org, diff --git a/src/secrets.rs b/src/secrets.rs new file mode 100644 index 00000000..770904da --- /dev/null +++ b/src/secrets.rs @@ -0,0 +1,653 @@ +//! The `secrets` CLI command (subcommand group). +//! +//! Replaces `ado-aw configure` with a `secrets set / list / delete` +//! subcommand group. `secrets set GITHUB_TOKEN ` is the direct +//! replacement for `configure --token `; the legacy +//! `configure` invocation is still accepted (hidden in `--help`) and +//! prints a deprecation warning before forwarding to +//! [`run_set_github_token`]. +//! +//! Phase 1 of the pipeline-lifecycle CLI family — see `docs/cli.md`. +//! +//! ## Security +//! +//! - `secrets list` never prints variable values. It only emits names +//! and the `isSecret` / `allowOverride` flags. +//! - `secrets set` PUTs with `isSecret: true`. + +use anyhow::{Context, Result}; +use std::path::{Path, PathBuf}; + +use crate::ado::{ + AdoAuth, AdoContext, MatchedDefinition, get_definition_full, resolve_ado_context, + resolve_auth, resolve_definitions, +}; + +/// Description of one pipeline variable, for listing only. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct VariableInfo { + pub name: String, + pub is_secret: bool, + pub allow_override: bool, +} + +/// Validate a variable name. ADO permits arbitrary names but the CLI +/// rejects empty/whitespace-only/whitespace-containing inputs since +/// those almost always indicate a quoting bug. +pub fn validate_variable_name(name: &str) -> Result<()> { + if name.is_empty() { + anyhow::bail!("Variable name must not be empty."); + } + if name.chars().any(char::is_whitespace) { + anyhow::bail!( + "Variable name '{}' contains whitespace; check shell quoting.", + name + ); + } + Ok(()) +} + +/// Pure: produce a copy of `definition` with the named variable set +/// to `(value, isSecret=true, allow_override)`. Preserves all other +/// keys. +pub fn apply_variable_set( + mut definition: serde_json::Value, + name: &str, + value: &str, + allow_override: bool, +) -> serde_json::Value { + if definition.get("variables").is_none() + || !definition["variables"].is_object() + { + definition["variables"] = serde_json::json!({}); + } + definition["variables"][name] = serde_json::json!({ + "value": value, + "isSecret": true, + "allowOverride": allow_override, + }); + definition +} + +/// Pure: produce a copy of `definition` with the named variable +/// removed. No-op if it wasn't present. +pub fn apply_variable_delete( + mut definition: serde_json::Value, + name: &str, +) -> serde_json::Value { + if let Some(vars) = definition.get_mut("variables").and_then(|v| v.as_object_mut()) { + vars.remove(name); + } + definition +} + +/// Pure: project a definition's `variables` object to a sorted +/// list of [`VariableInfo`]. Never reads or surfaces the `value` +/// field — listings must be safe to dump to stdout. +pub fn list_variables(definition: &serde_json::Value) -> Vec { + let Some(vars) = definition.get("variables").and_then(|v| v.as_object()) else { + return Vec::new(); + }; + let mut out: Vec = vars + .iter() + .map(|(k, v)| VariableInfo { + name: k.clone(), + is_secret: v.get("isSecret").and_then(|b| b.as_bool()).unwrap_or(false), + allow_override: v + .get("allowOverride") + .and_then(|b| b.as_bool()) + .unwrap_or(false), + }) + .collect(); + out.sort_by(|a, b| a.name.cmp(&b.name)); + out +} + +// ==================== Shared HTTP helpers ==================== + +async fn put_definition( + client: &reqwest::Client, + ctx: &AdoContext, + auth: &AdoAuth, + id: u64, + body: &serde_json::Value, +) -> Result<()> { + let url = format!( + "{}/{}/_apis/build/definitions/{}?api-version=7.1", + ctx.org_url.trim_end_matches('/'), + ctx.project, + id + ); + + let resp = auth + .apply(client.put(&url)) + .header("Content-Type", "application/json") + .json(body) + .send() + .await + .with_context(|| format!("Failed to PUT definition {}", id))?; + + let status = resp.status(); + if !status.is_success() { + let resp_body = resp.text().await.unwrap_or_default(); + anyhow::bail!( + "ADO API returned {} when PUTting definition {}: {}", + status, + id, + resp_body + ); + } + Ok(()) +} + +// ==================== `secrets set` ==================== + +pub struct SetOptions<'a> { + pub name: &'a str, + pub value: Option<&'a str>, + pub org: Option<&'a str>, + pub project: Option<&'a str>, + pub pat: Option<&'a str>, + pub path: Option<&'a Path>, + pub allow_override: bool, + pub value_stdin: bool, + pub dry_run: bool, + pub definition_ids: Option<&'a [u64]>, +} + +pub async fn run_set(opts: SetOptions<'_>) -> Result<()> { + validate_variable_name(opts.name)?; + + let repo_path: PathBuf = match opts.path { + Some(p) => tokio::fs::canonicalize(p) + .await + .with_context(|| format!("Could not resolve path: {}", p.display()))?, + None => tokio::fs::canonicalize(".") + .await + .context("Could not resolve current directory")?, + }; + + let value = resolve_value(opts.name, opts.value, opts.value_stdin)?; + let auth = resolve_auth(opts.pat).await?; + let ado_ctx = resolve_ado_context(&repo_path, opts.org, opts.project).await?; + + let client = reqwest::Client::builder() + .timeout(std::time::Duration::from_secs(30)) + .build() + .context("Failed to create HTTP client")?; + + let Some(matched) = resolve_definitions( + &client, + &ado_ctx, + &auth, + opts.definition_ids, + &repo_path, + ) + .await? + else { + return Ok(()); + }; + + if matched.is_empty() { + anyhow::bail!( + "No ADO definitions matched any local fixture. Run `ado-aw list` to \ + diagnose." + ); + } + + print_matched_summary(&matched); + + if opts.dry_run { + println!( + "[dry-run] Would set '{}' (isSecret=true, allowOverride={}) on {} definition(s).", + opts.name, + opts.allow_override, + matched.len() + ); + return Ok(()); + } + + let mut success = 0usize; + let mut failure = 0usize; + for m in &matched { + match apply_set_one( + &client, + &ado_ctx, + &auth, + m.id, + opts.name, + &value, + opts.allow_override, + ) + .await + { + Ok(()) => { + println!(" ✓ '{}' set on '{}' (id={})", opts.name, m.name, m.id); + success += 1; + } + Err(e) => { + eprintln!(" ✗ '{}' on '{}' (id={}): {:#}", opts.name, m.name, m.id, e); + failure += 1; + } + } + } + + println!(); + println!("Done: {} succeeded, {} failed.", success, failure); + if failure > 0 { + anyhow::bail!("{} definition(s) failed", failure); + } + Ok(()) +} + +async fn apply_set_one( + client: &reqwest::Client, + ctx: &AdoContext, + auth: &AdoAuth, + id: u64, + name: &str, + value: &str, + allow_override: bool, +) -> Result<()> { + let definition = get_definition_full(client, ctx, auth, id).await?; + let updated = apply_variable_set(definition, name, value, allow_override); + put_definition(client, ctx, auth, id, &updated).await +} + +/// Resolve the variable value from the CLI inputs: explicit positional +/// `value` first, then `--value-stdin` (reads exactly one line), then +/// an interactive tty prompt with echo off. +fn resolve_value( + name: &str, + explicit: Option<&str>, + value_stdin: bool, +) -> Result { + if let Some(v) = explicit { + return Ok(v.to_string()); + } + if value_stdin { + use std::io::BufRead; + let mut line = String::new(); + let stdin = std::io::stdin(); + stdin.lock().read_line(&mut line).context("Failed to read value from stdin")?; + let trimmed = line.trim_end_matches(['\r', '\n']).to_string(); + if trimmed.is_empty() { + anyhow::bail!("--value-stdin read an empty value"); + } + return Ok(trimmed); + } + inquire::Password::new(&format!("Enter value for {}:", name)) + .without_confirmation() + .prompt() + .context("Failed to read value from interactive prompt") +} + +// ==================== `secrets list` ==================== + +pub struct ListOptions<'a> { + pub org: Option<&'a str>, + pub project: Option<&'a str>, + pub pat: Option<&'a str>, + pub path: Option<&'a Path>, + pub json: bool, + pub definition_ids: Option<&'a [u64]>, +} + +pub async fn run_list(opts: ListOptions<'_>) -> Result<()> { + let repo_path: PathBuf = match opts.path { + Some(p) => tokio::fs::canonicalize(p) + .await + .with_context(|| format!("Could not resolve path: {}", p.display()))?, + None => tokio::fs::canonicalize(".") + .await + .context("Could not resolve current directory")?, + }; + + let auth = resolve_auth(opts.pat).await?; + let ado_ctx = resolve_ado_context(&repo_path, opts.org, opts.project).await?; + + let client = reqwest::Client::builder() + .timeout(std::time::Duration::from_secs(30)) + .build() + .context("Failed to create HTTP client")?; + + let Some(matched) = resolve_definitions( + &client, + &ado_ctx, + &auth, + opts.definition_ids, + &repo_path, + ) + .await? + else { + return Ok(()); + }; + + if matched.is_empty() { + anyhow::bail!( + "No ADO definitions matched any local fixture. Run `ado-aw list` to \ + diagnose." + ); + } + + let mut payload = serde_json::json!({}); + for m in &matched { + let definition = get_definition_full(&client, &ado_ctx, &auth, m.id).await?; + let vars = list_variables(&definition); + + if opts.json { + payload[m.id.to_string()] = serde_json::json!({ + "name": m.name, + "variables": vars.iter().map(|v| serde_json::json!({ + "name": v.name, + "isSecret": v.is_secret, + "allowOverride": v.allow_override, + })).collect::>(), + }); + } else { + println!("● {} (id={})", m.name, m.id); + if vars.is_empty() { + println!(" (no variables)"); + } else { + for v in &vars { + println!( + " - {} isSecret={} allowOverride={}", + v.name, v.is_secret, v.allow_override + ); + } + } + println!(); + } + } + + if opts.json { + println!("{}", serde_json::to_string_pretty(&payload)?); + } + Ok(()) +} + +// ==================== `secrets delete` ==================== + +pub struct DeleteOptions<'a> { + pub name: &'a str, + pub org: Option<&'a str>, + pub project: Option<&'a str>, + pub pat: Option<&'a str>, + pub path: Option<&'a Path>, + pub dry_run: bool, + pub definition_ids: Option<&'a [u64]>, +} + +pub async fn run_delete(opts: DeleteOptions<'_>) -> Result<()> { + validate_variable_name(opts.name)?; + + let repo_path: PathBuf = match opts.path { + Some(p) => tokio::fs::canonicalize(p) + .await + .with_context(|| format!("Could not resolve path: {}", p.display()))?, + None => tokio::fs::canonicalize(".") + .await + .context("Could not resolve current directory")?, + }; + + let auth = resolve_auth(opts.pat).await?; + let ado_ctx = resolve_ado_context(&repo_path, opts.org, opts.project).await?; + + let client = reqwest::Client::builder() + .timeout(std::time::Duration::from_secs(30)) + .build() + .context("Failed to create HTTP client")?; + + let Some(matched) = resolve_definitions( + &client, + &ado_ctx, + &auth, + opts.definition_ids, + &repo_path, + ) + .await? + else { + return Ok(()); + }; + + if matched.is_empty() { + anyhow::bail!( + "No ADO definitions matched any local fixture. Run `ado-aw list` to \ + diagnose." + ); + } + + print_matched_summary(&matched); + + if opts.dry_run { + println!( + "[dry-run] Would delete variable '{}' from {} definition(s) (no-op when absent).", + opts.name, + matched.len() + ); + return Ok(()); + } + + let mut success = 0usize; + let mut failure = 0usize; + for m in &matched { + match apply_delete_one(&client, &ado_ctx, &auth, m.id, opts.name).await { + Ok(()) => { + println!(" ✓ '{}' removed from '{}' (id={})", opts.name, m.name, m.id); + success += 1; + } + Err(e) => { + eprintln!( + " ✗ removing '{}' from '{}' (id={}): {:#}", + opts.name, m.name, m.id, e + ); + failure += 1; + } + } + } + + println!(); + println!("Done: {} succeeded, {} failed.", success, failure); + if failure > 0 { + anyhow::bail!("{} definition(s) failed", failure); + } + Ok(()) +} + +async fn apply_delete_one( + client: &reqwest::Client, + ctx: &AdoContext, + auth: &AdoAuth, + id: u64, + name: &str, +) -> Result<()> { + let definition = get_definition_full(client, ctx, auth, id).await?; + let updated = apply_variable_delete(definition, name); + put_definition(client, ctx, auth, id, &updated).await +} + +// ==================== Deprecation alias ==================== + +/// Shim for the legacy `configure --token` invocation. Sets +/// `GITHUB_TOKEN` (isSecret=true, allowOverride preserved) on every +/// matched definition. Same fail-soft + accumulated-counts pattern as +/// the old `configure` body, lifted here verbatim so the deprecation +/// alias is byte-equivalent. +pub async fn run_set_github_token( + token: Option<&str>, + org: Option<&str>, + project: Option<&str>, + pat: Option<&str>, + path: Option<&Path>, + dry_run: bool, + definition_ids: Option<&[u64]>, +) -> Result<()> { + eprintln!( + "warning: 'ado-aw configure' is deprecated; use 'ado-aw secrets set GITHUB_TOKEN' \ + instead. The alias will be removed in the next minor release." + ); + run_set(SetOptions { + name: "GITHUB_TOKEN", + value: token, + org, + project, + pat, + path, + allow_override: false, + value_stdin: false, + dry_run, + definition_ids, + }) + .await +} + +// ==================== Shared display helpers ==================== + +fn print_matched_summary(matched: &[MatchedDefinition]) { + println!("{} definition(s) matched:", matched.len()); + for m in matched { + if m.yaml_path.is_empty() { + println!(" [{}] '{}' (id={})", m.match_method, m.name, m.id); + } else { + println!( + " [{}] '{}' (id={}) ← {}", + m.match_method, m.name, m.id, m.yaml_path + ); + } + } + println!(); +} + +#[cfg(test)] +mod tests { + use super::*; + + // ============ validate_variable_name ============ + + #[test] + fn validate_rejects_empty() { + assert!(validate_variable_name("").is_err()); + } + + #[test] + fn validate_rejects_whitespace() { + assert!(validate_variable_name(" ").is_err()); + assert!(validate_variable_name("FOO BAR").is_err()); + assert!(validate_variable_name("FOO\tBAR").is_err()); + } + + #[test] + fn validate_accepts_typical_names() { + assert!(validate_variable_name("GITHUB_TOKEN").is_ok()); + assert!(validate_variable_name("MY-VAR").is_ok()); + assert!(validate_variable_name("a.b.c").is_ok()); + } + + // ============ apply_variable_set ============ + + #[test] + fn set_creates_variables_object_when_missing() { + let def = serde_json::json!({ "name": "x" }); + let out = apply_variable_set(def, "FOO", "bar", false); + assert_eq!(out["variables"]["FOO"]["value"], "bar"); + assert_eq!(out["variables"]["FOO"]["isSecret"], true); + assert_eq!(out["variables"]["FOO"]["allowOverride"], false); + } + + #[test] + fn set_preserves_other_variables() { + let def = serde_json::json!({ + "variables": { "OTHER": { "value": "x", "isSecret": true, "allowOverride": false } } + }); + let out = apply_variable_set(def, "FOO", "bar", true); + assert_eq!(out["variables"]["OTHER"]["value"], "x"); + assert_eq!(out["variables"]["FOO"]["value"], "bar"); + assert_eq!(out["variables"]["FOO"]["allowOverride"], true); + } + + #[test] + fn set_overwrites_existing_variable() { + let def = serde_json::json!({ + "variables": { "FOO": { "value": "old", "isSecret": true, "allowOverride": false } } + }); + let out = apply_variable_set(def, "FOO", "new", true); + assert_eq!(out["variables"]["FOO"]["value"], "new"); + assert_eq!(out["variables"]["FOO"]["allowOverride"], true); + } + + // ============ apply_variable_delete ============ + + #[test] + fn delete_removes_existing_variable() { + let def = serde_json::json!({ + "variables": { + "FOO": { "value": "v" }, + "BAR": { "value": "w" } + } + }); + let out = apply_variable_delete(def, "FOO"); + assert!(out["variables"].get("FOO").is_none()); + assert_eq!(out["variables"]["BAR"]["value"], "w"); + } + + #[test] + fn delete_is_noop_when_variable_absent() { + let def = serde_json::json!({ "variables": { "FOO": { "value": "v" } } }); + let out = apply_variable_delete(def, "MISSING"); + assert_eq!(out["variables"]["FOO"]["value"], "v"); + } + + #[test] + fn delete_is_noop_when_variables_object_missing() { + let def = serde_json::json!({ "name": "x" }); + let out = apply_variable_delete(def, "MISSING"); + assert!(out.get("variables").is_none() || out["variables"].is_null()); + } + + // ============ list_variables (no values surfaced) ============ + + #[test] + fn list_emits_names_and_flags_only() { + let def = serde_json::json!({ + "variables": { + "TOKEN": { "value": "super-secret-leak-me", "isSecret": true, "allowOverride": false }, + "DEBUG": { "value": "true", "isSecret": false, "allowOverride": true } + } + }); + let out = list_variables(&def); + assert_eq!(out.len(), 2); + // Sorted by name. + assert_eq!(out[0].name, "DEBUG"); + assert!(!out[0].is_secret); + assert!(out[0].allow_override); + assert_eq!(out[1].name, "TOKEN"); + assert!(out[1].is_secret); + assert!(!out[1].allow_override); + } + + #[test] + fn list_returns_empty_when_no_variables_object() { + let def = serde_json::json!({ "name": "x" }); + assert!(list_variables(&def).is_empty()); + } + + /// Guardrail: the VariableInfo struct has no `value` field. If you + /// ever feel tempted to add one, you'll need to change the + /// printer — and ideally have a different review reason than + /// "convenience". + #[test] + fn variable_info_has_no_value_field_in_debug_repr() { + let def = serde_json::json!({ + "variables": { + "TOKEN": { "value": "super-secret", "isSecret": true, "allowOverride": false } + } + }); + let out = list_variables(&def); + let dbg = format!("{:?}", out[0]); + assert!( + !dbg.contains("super-secret"), + "VariableInfo Debug must not leak values, got: {}", + dbg + ); + } +} diff --git a/tests/secrets_integration.rs b/tests/secrets_integration.rs new file mode 100644 index 00000000..30895332 --- /dev/null +++ b/tests/secrets_integration.rs @@ -0,0 +1,119 @@ +//! Integration tests for `ado-aw secrets` (and the deprecation alias). + +use std::path::PathBuf; + +fn binary() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_ado-aw")) +} + +#[test] +fn secrets_help_advertises_subcommands() { + let output = std::process::Command::new(binary()) + .args(["secrets", "--help"]) + .output() + .expect("Failed to run ado-aw secrets --help"); + assert!(output.status.success(), "--help should exit 0"); + let stdout = String::from_utf8_lossy(&output.stdout); + for sub in ["set", "list", "delete"] { + assert!( + stdout.contains(sub), + "secrets --help should advertise the {sub} subcommand, got:\n{stdout}" + ); + } +} + +#[test] +fn secrets_set_help_advertises_flags() { + let output = std::process::Command::new(binary()) + .args(["secrets", "set", "--help"]) + .output() + .expect("Failed to run ado-aw secrets set --help"); + assert!(output.status.success(), "--help should exit 0"); + let stdout = String::from_utf8_lossy(&output.stdout); + for flag in ["--allow-override", "--value-stdin", "--dry-run", "--pat"] { + assert!( + stdout.contains(flag), + "secrets set --help should advertise {flag}, got:\n{stdout}" + ); + } +} + +#[test] +fn secrets_list_help_warns_no_values() { + let output = std::process::Command::new(binary()) + .args(["secrets", "list", "--help"]) + .output() + .expect("Failed to run ado-aw secrets list --help"); + assert!(output.status.success(), "--help should exit 0"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("--json"), + "secrets list --help should advertise --json, got:\n{stdout}" + ); +} + +#[test] +fn configure_is_hidden_in_top_level_help() { + let output = std::process::Command::new(binary()) + .arg("--help") + .output() + .expect("Failed to run ado-aw --help"); + assert!(output.status.success(), "--help should exit 0"); + let stdout = String::from_utf8_lossy(&output.stdout); + // `secrets` is the documented replacement. + assert!( + stdout.contains("secrets"), + "Top-level --help should advertise the secrets subcommand, got:\n{stdout}" + ); + // The legacy `configure` line must not appear (it's hidden). + // We allow any line that mentions "configure" elsewhere but the + // top-level commands list must not include the literal subcommand. + // We check the "Commands:" section, which is everything between + // `Commands:` and `Options:`. + let lower = stdout.to_lowercase(); + if let (Some(c), Some(o)) = (lower.find("commands:"), lower.find("options:")) { + let block = &lower[c..o]; + assert!( + !block.contains("configure"), + "configure should be hidden in top-level --help; commands block was:\n{block}" + ); + } +} + +#[test] +fn configure_invocation_still_works_and_warns() { + // We can't drive a real ADO call from a unit test, but the + // deprecation warning is emitted by the very first line of + // `run_set_github_token`, so we trigger it by passing arguments + // that will fail early after the warning prints. + // + // Use a path that doesn't exist; the warning is emitted before + // the path canonicalization step. The command will still + // ultimately fail, which is fine — we only assert on stderr. + let output = std::process::Command::new(binary()) + .args([ + "configure", + "--org", + "fake", + "--project", + "fake", + "--pat", + "dummy", + "--token", + "x", + "--path", + "/definitely-does-not-exist-9c4f0", + "--dry-run", + ]) + .output() + .expect("Failed to run ado-aw configure"); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("deprecated"), + "configure should emit a deprecation warning, got stderr:\n{stderr}" + ); + assert!( + stderr.contains("secrets set GITHUB_TOKEN") || stderr.contains("secrets set"), + "deprecation warning should mention the replacement, got stderr:\n{stderr}" + ); +} From 0cab24e4d9fe53b535355d4c1576ef929811940a Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 17 May 2026 21:36:44 +0100 Subject: [PATCH 08/14] fix(cli): percent-encode ctx.project in all ADO URL formatters Code-review finding on PR #602: seven URL formatters in `src/ado/mod.rs` interpolated `ctx.project` directly into the path segment of the request URL while five sibling functions correctly ran the value through `percent_encoding::utf8_percent_encode(..., PATH_SEGMENT)`. Project names containing reserved characters (spaces, `/`, `?`, `#`, `:` etc.) would have broken the URL or silently produced surprising responses. Affected functions, all now using the same encoder as `get_repository_id`, `get_definition_full`, `patch_queue_status`, `delete_definition`, and `create_definition`: - `list_definitions` - `get_definition_name` - `update_pipeline_variable` (both GET and PUT URLs) - `queue_build` - `get_build` - `get_latest_build` The `info!()` log line at the top of `match_definitions` is unaffected (logging, not URL construction). The existing `path_segment_*` tests already cover the encoder behaviour; no new test is needed since these are mechanical substitutions of an existing pattern. Full `cargo test` (1567 unit tests + integration crates) and `cargo clippy --all-targets --all-features` are green after the fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/ado/mod.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/ado/mod.rs b/src/ado/mod.rs index a3b4210f..f7698328 100644 --- a/src/ado/mod.rs +++ b/src/ado/mod.rs @@ -263,7 +263,7 @@ pub async fn list_definitions( let base_url = format!( "{}/{}/_apis/build/definitions", ctx.org_url.trim_end_matches('/'), - ctx.project + percent_encoding::utf8_percent_encode(&ctx.project, PATH_SEGMENT) ); debug!("Listing definitions: {}", base_url); @@ -507,7 +507,7 @@ pub async fn get_definition_name( let url = format!( "{}/{}/_apis/build/definitions/{}?api-version=7.1", ctx.org_url.trim_end_matches('/'), - ctx.project, + percent_encoding::utf8_percent_encode(&ctx.project, PATH_SEGMENT), definition_id ); @@ -558,7 +558,7 @@ pub async fn update_pipeline_variable( let get_url = format!( "{}/{}/_apis/build/definitions/{}?api-version=7.1", ctx.org_url.trim_end_matches('/'), - ctx.project, + percent_encoding::utf8_percent_encode(&ctx.project, PATH_SEGMENT), definition_id ); @@ -617,7 +617,7 @@ pub async fn update_pipeline_variable( let put_url = format!( "{}/{}/_apis/build/definitions/{}?api-version=7.1", ctx.org_url.trim_end_matches('/'), - ctx.project, + percent_encoding::utf8_percent_encode(&ctx.project, PATH_SEGMENT), definition_id ); @@ -1099,7 +1099,7 @@ pub async fn queue_build( let url = format!( "{}/{}/_apis/build/builds?api-version=7.1", ctx.org_url.trim_end_matches('/'), - ctx.project, + percent_encoding::utf8_percent_encode(&ctx.project, PATH_SEGMENT), ); let mut body = serde_json::json!({ @@ -1156,7 +1156,7 @@ pub async fn get_build( let url = format!( "{}/{}/_apis/build/builds/{}?api-version=7.1", ctx.org_url.trim_end_matches('/'), - ctx.project, + percent_encoding::utf8_percent_encode(&ctx.project, PATH_SEGMENT), build_id ); @@ -1197,7 +1197,7 @@ pub async fn get_latest_build( let url = format!( "{}/{}/_apis/build/builds?definitions={}&$top=1&api-version=7.1", ctx.org_url.trim_end_matches('/'), - ctx.project, + percent_encoding::utf8_percent_encode(&ctx.project, PATH_SEGMENT), definition_id, ); From 860e0664f022f7600668704c1e43c0ff4a7e30ad Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 17 May 2026 21:40:26 +0100 Subject: [PATCH 09/14] fix(run): re-check --timeout between each in-flight get_build call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code-review follow-up on PR #602: `poll_until_complete` only checked `started.elapsed() >= timeout` at the top of each round, so with N in-flight builds and reqwest's 30s per-call HTTP timeout, the operator-visible wait could overshoot `--timeout` by up to N × 30s in the pathological all-stalled case. Re-checks the wall-clock budget between each individual `get_build` call inside a round. When the budget is exhausted mid-round, the current target and every remaining one are carried forward into `pending` so the caller's `in_progress` count stays accurate (the loop owes a status for everything it queued). In the common case where the poll interval is several times the HTTP timeout, the previous behaviour was indistinguishable from the new one — the bug only matters when poll interval ≪ HTTP timeout, which is an awkward but plausible configuration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/run.rs | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/src/run.rs b/src/run.rs index 2e798f8e..8e0d3cf4 100644 --- a/src/run.rs +++ b/src/run.rs @@ -262,7 +262,26 @@ async fn poll_until_complete( } let mut next_pending = Vec::new(); - for t in &pending { + let mut iter = pending.iter(); + let mut timed_out_mid_round = false; + for t in iter.by_ref() { + // Re-check the wall-clock budget between each in-flight + // build, not just at the top of the round. With N targets + // and a 30s reqwest timeout, the previous "check once per + // round" loop could overshoot the operator's `--timeout` + // by up to N × 30s in the pathological all-stalled case + // — surprising behaviour when the poll interval is shorter + // than the per-call HTTP timeout. + if started.elapsed() >= timeout { + // Carry the current target and every remaining one + // forward so the caller's `in_progress` count is + // accurate (the loop owes a status for everything it + // queued). + next_pending.push(*t); + next_pending.extend(iter.by_ref().copied()); + timed_out_mid_round = true; + break; + } match get_build(client, ctx, auth, t.build_id).await { Ok(body) => match classify_build(&body) { BuildOutcome::InProgress => next_pending.push(*t), @@ -295,6 +314,12 @@ async fn poll_until_complete( } pending = next_pending; + if timed_out_mid_round { + println!("⚠ wait timed out after {}s", timeout.as_secs()); + outcome.in_progress = pending.len(); + return Ok(outcome); + } + if !pending.is_empty() { tokio::time::sleep(poll_interval).await; } From 87b3f6bcdcb4c3eda2c9b12496c9b9d383efc7d0 Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 17 May 2026 21:50:47 +0100 Subject: [PATCH 10/14] fix(cli): percent-encode project in secrets/run; make --value-stdin conflict explicit Three follow-ups on PR #602: 1. `src/secrets.rs::put_definition` was the last URL formatter using `ctx.project` unencoded. Now uses `PATH_SEGMENT` like every other builder in `src/ado/mod.rs`. `PATH_SEGMENT` was promoted from private `const` to `pub const` to support cross-module reuse. 2. `src/run.rs` was printing a deep-link to the queued build using unencoded `ado_ctx.project`. The URL is cosmetic (never used as an HTTP target), but it would render broken/unclickable for projects containing spaces or other URL-unsafe characters. Now encoded with the same `PATH_SEGMENT` encoder. 3. `ado-aw secrets set --value-stdin` silently ignored `--value-stdin` when both were supplied (explicit positional value won). Added `conflicts_with = "value"` to the `value_stdin` clap arg so the combination is rejected at parse time with a clear error. Added an integration test in `tests/secrets_integration.rs` to pin the behaviour. `cargo test` (1567 unit + 14 integration crates, all green) and `cargo clippy --all-targets --all-features` pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/ado/mod.rs | 2 +- src/main.rs | 5 +++-- src/run.rs | 6 +++--- src/secrets.rs | 6 +++--- tests/secrets_integration.rs | 19 +++++++++++++++++++ 5 files changed, 29 insertions(+), 9 deletions(-) diff --git a/src/ado/mod.rs b/src/ado/mod.rs index f7698328..eb6ab60e 100644 --- a/src/ado/mod.rs +++ b/src/ado/mod.rs @@ -825,7 +825,7 @@ pub async fn resolve_definitions( /// just for symmetry with common path-encoding tables. Notably this /// preserves `-`, `_`, `.`, `~` which `NON_ALPHANUMERIC` would over- /// encode (e.g. `my-repo` → `my%2Drepo`). -const PATH_SEGMENT: &percent_encoding::AsciiSet = &percent_encoding::CONTROLS +pub const PATH_SEGMENT: &percent_encoding::AsciiSet = &percent_encoding::CONTROLS .add(b' ') .add(b'"') .add(b'#') diff --git a/src/main.rs b/src/main.rs index ef9c2fbd..86241507 100644 --- a/src/main.rs +++ b/src/main.rs @@ -50,8 +50,9 @@ enum SecretsCmd { /// Mark the variable as `allowOverride=true` (default: false). #[arg(long)] allow_override: bool, - /// Read the value from a single line on stdin. - #[arg(long)] + /// Read the value from a single line on stdin. Mutually exclusive + /// with the positional `` argument. + #[arg(long, conflicts_with = "value")] value_stdin: bool, #[arg(long)] dry_run: bool, diff --git a/src/run.rs b/src/run.rs index 8e0d3cf4..1c0ab6fb 100644 --- a/src/run.rs +++ b/src/run.rs @@ -15,8 +15,8 @@ use std::path::{Path, PathBuf}; use std::time::{Duration, Instant}; use crate::ado::{ - AdoAuth, AdoContext, MatchedDefinition, get_build, match_definitions, queue_build, - resolve_ado_context, resolve_auth, + AdoAuth, AdoContext, MatchedDefinition, PATH_SEGMENT, get_build, match_definitions, + queue_build, resolve_ado_context, resolve_auth, }; use crate::detect; @@ -158,7 +158,7 @@ pub async fn dispatch(opts: RunOptions<'_>) -> Result<()> { m.id, build_id, ado_ctx.org_url.trim_end_matches('/'), - ado_ctx.project, + percent_encoding::utf8_percent_encode(&ado_ctx.project, PATH_SEGMENT), build_id ); targets.push(PollTarget { diff --git a/src/secrets.rs b/src/secrets.rs index 770904da..782435f3 100644 --- a/src/secrets.rs +++ b/src/secrets.rs @@ -19,8 +19,8 @@ use anyhow::{Context, Result}; use std::path::{Path, PathBuf}; use crate::ado::{ - AdoAuth, AdoContext, MatchedDefinition, get_definition_full, resolve_ado_context, - resolve_auth, resolve_definitions, + AdoAuth, AdoContext, MatchedDefinition, PATH_SEGMENT, get_definition_full, + resolve_ado_context, resolve_auth, resolve_definitions, }; /// Description of one pipeline variable, for listing only. @@ -115,7 +115,7 @@ async fn put_definition( let url = format!( "{}/{}/_apis/build/definitions/{}?api-version=7.1", ctx.org_url.trim_end_matches('/'), - ctx.project, + percent_encoding::utf8_percent_encode(&ctx.project, PATH_SEGMENT), id ); diff --git a/tests/secrets_integration.rs b/tests/secrets_integration.rs index 30895332..3fbfa4cc 100644 --- a/tests/secrets_integration.rs +++ b/tests/secrets_integration.rs @@ -38,6 +38,25 @@ fn secrets_set_help_advertises_flags() { } } +#[test] +fn secrets_set_rejects_value_with_value_stdin() { + // clap should reject the combination at parse time via + // `conflicts_with = "value"` on `--value-stdin`. + let output = std::process::Command::new(binary()) + .args(["secrets", "set", "MY_VAR", "explicit", "--value-stdin"]) + .output() + .expect("Failed to run ado-aw secrets set"); + assert!( + !output.status.success(), + "Expected non-zero exit when both and --value-stdin are supplied" + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("value-stdin") || stderr.contains("value_stdin"), + "stderr should reference the conflict, got:\n{stderr}" + ); +} + #[test] fn secrets_list_help_warns_no_values() { let output = std::process::Command::new(binary()) From 435a55f139567294d786934427b51c725eb3f353 Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 17 May 2026 22:01:27 +0100 Subject: [PATCH 11/14] fix(cli): encode synthesized URL, surface detect errors, document parse_parameters edge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four follow-ups on PR #602: 1. `src/status.rs::render_blocks` synthesized fallback URL was passing `ado_project` verbatim into the path segment when `LastRun::url` was absent. Now uses `PATH_SEGMENT` like every other URL builder in the PR. URL is text-output only, but renders broken links for projects with spaces or reserved chars. 2. `src/list.rs` and `src/status.rs` were swallowing `detect_pipelines` errors via `.unwrap_or_default()`, making "detection failed" indistinguishable from "no pipelines compiled here" — both produce zero matches downstream. Both commands are read-only and useful even with partial inputs (`list --all` doesn't need fixtures at all), so we don't bail; we emit a `warning: failed to scan local pipelines: …` to stderr so the operator can distinguish the two cases. 3. `src/run.rs::parse_parameters` silently rejects values containing commas (the `,` split happens before the `=` split, so the trailing fragment falls into the "no `=`" rejection path). The behaviour is intentional — commas are the documented pair separator — but it was undocumented. Added a doc comment spelling out the constraint and the one-pair-per-flag workaround, plus a new `parse_parameters_values_with_commas_split_pre_equals` unit test that pins both the rejection and the workaround. The doc comment tells future contributors to update the test if comma escaping is ever added. 4. `src/secrets.rs::run_set_github_token` carries an undocumented invariant: the deprecation warning must be emitted before any fallible I/O, because the integration test `configure_invocation_still_works_and_warns` exercises it by driving the function with a path that triggers an early canonicalize failure. Added an `IMPORTANT — invariant for the integration test` doc comment so a later refactor that defers the `eprintln!` (e.g. lazy auth init) will spot the constraint. `cargo test` (1568 unit + 14 integration crates, all green) and `cargo clippy --all-targets --all-features` pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/list.rs | 10 +++++++++- src/run.rs | 47 +++++++++++++++++++++++++++++++++++++++++++++++ src/secrets.rs | 11 +++++++++++ src/status.rs | 15 ++++++++++++--- 4 files changed, 79 insertions(+), 4 deletions(-) diff --git a/src/list.rs b/src/list.rs index 31a6d1ac..03a00217 100644 --- a/src/list.rs +++ b/src/list.rs @@ -254,7 +254,15 @@ pub async fn run(opts: ListOptions<'_>) -> Result<()> { .context("Failed to create HTTP client")?; let definitions = list_definitions(&client, &ado_ctx, &auth).await?; - let detected = detect::detect_pipelines(&repo_path).await.unwrap_or_default(); + let detected = detect::detect_pipelines(&repo_path).await.unwrap_or_else(|e| { + // Distinguish "detection failed" from "no pipelines compiled + // here": both produce zero matches downstream, but only the + // former is something the operator should know about. Don't + // bail outright — list is read-only and useful even with + // partial inputs (`--all` doesn't need fixtures at all). + eprintln!("warning: failed to scan local pipelines: {:#}", e); + Vec::new() + }); let matched = match_definitions_in(&definitions, &detected); // Decide which IDs need a last-build fetch. diff --git a/src/run.rs b/src/run.rs index 1c0ab6fb..b321673e 100644 --- a/src/run.rs +++ b/src/run.rs @@ -22,6 +22,28 @@ use crate::detect; /// Parse `--parameters foo=bar,baz=qux` (and its repeatable form) into /// a JSON map. Pure function; reject malformed pairs. +/// +/// **Values must not contain commas.** Each raw argument is split on +/// `,` *before* the `=` split, so a value like `redirect_uri=https://a,b` +/// silently becomes `{"redirect_uri": "https://a", "b": "?"}` (and +/// actually errors out since `b` has no `=`). When a parameter value +/// must include a comma, pass it via a dedicated `--parameters` +/// occurrence rather than a comma-joined one: +/// +/// - ✅ `--parameters 'urls=a,b' --parameters mode=fast` → two flags +/// still split `urls=a,b` on the comma. Use the workaround below. +/// - ✅ `--parameters mode=fast` plus an explicit subsequent +/// `--parameters extra=…` — one pair per flag, no comma in values. +/// - ❌ `--parameters key=a,b` will parse as two pairs and reject +/// the second. +/// +/// The CLI does not currently support escaping commas inside a single +/// `--parameters` argument; users who need that should fall back to +/// repeated `--parameters` flags (one pair each). +/// +/// Only the first `=` in a pair is treated as the separator; subsequent +/// `=` characters are part of the value, so `key=a=b=c` parses as +/// `{"key": "a=b=c"}`. pub fn parse_parameters(values: &[String]) -> Result> { let mut out = serde_json::Map::new(); for raw in values { @@ -393,6 +415,31 @@ mod tests { assert_eq!(m.len(), 1); } + #[test] + fn parse_parameters_values_with_commas_split_pre_equals() { + // Documented sharp edge: each raw argument is split on `,` BEFORE + // the `=` split. A value containing a comma will be torn apart + // (and usually rejected because the trailing fragment has no `=`). + // If you ever change parse_parameters to escape or quote commas, + // update both the function doc and this test in lockstep — the + // doc comment promises this exact behaviour. + let err = parse_parameters(&["key=a,b".to_string()]).unwrap_err(); + assert!( + err.to_string().contains("no '='"), + "expected 'no =' error on the second fragment, got: {}", + err + ); + + // The well-formed workaround is one --parameters flag per pair. + let m = parse_parameters(&[ + "urls=https://a".to_string(), + "extra=b".to_string(), + ]) + .unwrap(); + assert_eq!(m.get("urls").unwrap().as_str(), Some("https://a")); + assert_eq!(m.get("extra").unwrap().as_str(), Some("b")); + } + // ============ classify_build ============ #[test] diff --git a/src/secrets.rs b/src/secrets.rs index 782435f3..cc02cddc 100644 --- a/src/secrets.rs +++ b/src/secrets.rs @@ -473,6 +473,17 @@ async fn apply_delete_one( /// matched definition. Same fail-soft + accumulated-counts pattern as /// the old `configure` body, lifted here verbatim so the deprecation /// alias is byte-equivalent. +/// +/// **IMPORTANT — invariant for the integration test:** the deprecation +/// warning MUST be emitted (via `eprintln!`) before any fallible I/O +/// happens. The `configure_invocation_still_works_and_warns` +/// integration test in `tests/secrets_integration.rs` drives this +/// function with a bogus `--path` that causes `tokio::fs::canonicalize` +/// inside `run_set` to fail; the test then asserts the warning text +/// appears on stderr. If you reorder this function to defer the +/// `eprintln!` (e.g. after auth resolution) the test will start +/// catching a *side effect* of ordering rather than the deprecation +/// behaviour itself, which is the opposite of what we want. pub async fn run_set_github_token( token: Option<&str>, org: Option<&str>, diff --git a/src/status.rs b/src/status.rs index 01fca907..df9cc8e1 100644 --- a/src/status.rs +++ b/src/status.rs @@ -15,7 +15,8 @@ use std::collections::HashSet; use std::path::{Path, PathBuf}; use crate::ado::{ - get_latest_build, list_definitions, match_definitions_in, resolve_ado_context, resolve_auth, + PATH_SEGMENT, get_latest_build, list_definitions, match_definitions_in, resolve_ado_context, + resolve_auth, }; use crate::detect; use crate::list::{ListRow, build_rows, render_json}; @@ -65,7 +66,7 @@ pub fn render_blocks(ado_org_url: &str, ado_project: &str, rows: &[ListRow]) -> format!( "{}/{}/_build/results?buildId={}", ado_org_url.trim_end_matches('/'), - ado_project, + percent_encoding::utf8_percent_encode(ado_project, PATH_SEGMENT), lr.id, ) }); @@ -100,7 +101,15 @@ pub async fn run(opts: StatusOptions<'_>) -> Result<()> { .context("Failed to create HTTP client")?; let definitions = list_definitions(&client, &ado_ctx, &auth).await?; - let detected = detect::detect_pipelines(&repo_path).await.unwrap_or_default(); + let detected = detect::detect_pipelines(&repo_path).await.unwrap_or_else(|e| { + // Distinguish "detection failed" from "no pipelines compiled + // here": both produce zero matches downstream, but only the + // former is something the operator should know about. Don't + // bail outright — status is read-only and useful even with + // partial inputs. + eprintln!("warning: failed to scan local pipelines: {:#}", e); + Vec::new() + }); let matched = match_definitions_in(&definitions, &detected); let target_ids: HashSet = matched.iter().map(|m| m.id).collect(); From 6919721f151d34d2d9c5e9e8bbf2a27d968291a8 Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 17 May 2026 22:13:59 +0100 Subject: [PATCH 12/14] fix(cli): correct parse_parameters doc; cap consecutive poll errors; warn on empty status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three follow-ups on PR #602: 1. `parse_parameters` doc-comment bug: the first bullet was tagged ✅ but described the same broken case as the third (✅ `--parameters 'urls=a,b' --parameters mode=fast` still splits on the comma inside `urls=a,b` and fails on the trailing `b` fragment). Rewrote the bullet list so all broken examples are ❌ and only the genuine "one pair per flag, no commas in values" workaround is ✅. Also clarified that there is currently no way to escape a comma inside a single `--parameters` argument, and pointed at the existing `parse_parameters_values_with_commas_split_pre_equals` unit test as the behaviour anchor. 2. `poll_until_complete` couldn't distinguish a permanent error (deleted build, revoked PAT, 404) from a transient one — both pushed the target back onto `next_pending` and silently retried until `--timeout`. Added a per-build `consecutive_errors: HashMap` counter that resets on any successful poll and bails out of that specific build after `MAX_CONSECUTIVE_POLL_ERRORS = 3` consecutive failures, counting it as failed. Transient blips still retry; persistent failures surface within `3 × poll_interval` (default 30s) instead of waiting out the full `--timeout` (default 1800s). 3. `status` was silently rendering `(no matched definitions)` when the fixture matcher returned zero hits, which is indistinguishable from running in the wrong directory. Added an `eprintln!` warning that mirrors the existing `failed to scan local pipelines: …` message. The command stays non-fatal (read-only) by design, unlike `disable` which bails. `cargo test` (1568 unit + 14 integration crates, all green) and `cargo clippy --all-targets --all-features` pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/run.rs | 112 ++++++++++++++++++++++++++++++++++---------------- src/status.rs | 12 ++++++ 2 files changed, 89 insertions(+), 35 deletions(-) diff --git a/src/run.rs b/src/run.rs index b321673e..4880dc0d 100644 --- a/src/run.rs +++ b/src/run.rs @@ -25,21 +25,28 @@ use crate::detect; /// /// **Values must not contain commas.** Each raw argument is split on /// `,` *before* the `=` split, so a value like `redirect_uri=https://a,b` -/// silently becomes `{"redirect_uri": "https://a", "b": "?"}` (and -/// actually errors out since `b` has no `=`). When a parameter value -/// must include a comma, pass it via a dedicated `--parameters` -/// occurrence rather than a comma-joined one: +/// is torn into two pairs and the trailing fragment (`b`) is rejected +/// because it has no `=`. /// -/// - ✅ `--parameters 'urls=a,b' --parameters mode=fast` → two flags -/// still split `urls=a,b` on the comma. Use the workaround below. -/// - ✅ `--parameters mode=fast` plus an explicit subsequent -/// `--parameters extra=…` — one pair per flag, no comma in values. -/// - ❌ `--parameters key=a,b` will parse as two pairs and reject -/// the second. +/// There is currently no way to escape a comma inside a single +/// `--parameters` argument. The CLI also splits any single argument +/// on `,`, so passing the comma-containing value as a separate flag +/// does **not** help either — it's the comma in the argument value +/// (not the argument boundary) that matters. /// -/// The CLI does not currently support escaping commas inside a single -/// `--parameters` argument; users who need that should fall back to -/// repeated `--parameters` flags (one pair each). +/// - ❌ `--parameters key=a,b` +/// → splits to `key=a` + `b`; the second pair fails with `no '='`. +/// - ❌ `--parameters 'urls=a,b' --parameters mode=fast` +/// → same split happens inside the first argument; the result is +/// `key=urls=a` + `b` + `mode=fast` and the `b` fragment is rejected. +/// - ✅ `--parameters mode=fast --parameters extra=x` +/// → one pair per flag, no commas in values; both pairs parse. +/// +/// If you need to pass a comma in a value, the only workaround today is +/// to write the value without the comma (e.g. URL-encode it on the +/// caller side and have the pipeline decode it). A follow-up could add +/// escape syntax (`--parameters 'urls=a\,b'`) without breaking this +/// rule. /// /// Only the first `=` in a pair is treated as the separator; subsequent /// `=` characters are part of the value, so `key=a=b=c` parses as @@ -256,6 +263,12 @@ struct PollOutcome { in_progress: usize, } +/// Maximum consecutive poll errors per build before the poller gives +/// up on that specific target and counts it as failed. Bounds the +/// damage of a permanent error (deleted build, revoked PAT, 404) +/// without surrendering on a single transient blip. +const MAX_CONSECUTIVE_POLL_ERRORS: usize = 3; + async fn poll_until_complete( client: &reqwest::Client, ctx: &AdoContext, @@ -267,6 +280,14 @@ async fn poll_until_complete( let started = Instant::now(); let mut outcome = PollOutcome::default(); let mut pending: Vec = targets.to_vec(); + // Consecutive poll-error count per build. A successful poll + // (Succeeded / Failed / InProgress) resets the counter via the + // implicit "we don't write to the map on success" — entries are + // removed when the build leaves `pending`. The counter is + // independent of `next_pending` so the bookkeeping stays + // round-stable. + let mut consecutive_errors: std::collections::HashMap = + std::collections::HashMap::new(); println!(); println!( @@ -305,32 +326,53 @@ async fn poll_until_complete( break; } match get_build(client, ctx, auth, t.build_id).await { - Ok(body) => match classify_build(&body) { - BuildOutcome::InProgress => next_pending.push(*t), - BuildOutcome::Succeeded => { - println!("✓ build {} (definition {}) succeeded", t.build_id, t.definition_id); - outcome.succeeded += 1; + Ok(body) => { + consecutive_errors.remove(&t.build_id); + match classify_build(&body) { + BuildOutcome::InProgress => next_pending.push(*t), + BuildOutcome::Succeeded => { + println!("✓ build {} (definition {}) succeeded", t.build_id, t.definition_id); + outcome.succeeded += 1; + } + BuildOutcome::Failed => { + let result = body + .get("result") + .and_then(|v| v.as_str()) + .unwrap_or("unknown"); + println!( + "✗ build {} (definition {}) finished with result={}", + t.build_id, t.definition_id, result + ); + outcome.failed += 1; + } } - BuildOutcome::Failed => { - let result = body - .get("result") - .and_then(|v| v.as_str()) - .unwrap_or("unknown"); - println!( - "✗ build {} (definition {}) finished with result={}", - t.build_id, t.definition_id, result + } + Err(e) => { + let count = consecutive_errors.entry(t.build_id).or_insert(0); + *count += 1; + if *count >= MAX_CONSECUTIVE_POLL_ERRORS { + eprintln!( + "✗ build {} (definition {}): giving up after {} consecutive poll errors; last error: {:#}", + t.build_id, t.definition_id, count, e ); + // Count this as a failed build so the caller's + // exit code reflects the persistent error + // rather than waiting out --timeout. outcome.failed += 1; + consecutive_errors.remove(&t.build_id); + } else { + eprintln!( + " warning: poll error for build {} (definition {}) (attempt {}/{}): {:#}", + t.build_id, + t.definition_id, + count, + MAX_CONSECUTIVE_POLL_ERRORS, + e + ); + // Treat as still-in-progress; we'll retry on + // the next tick. + next_pending.push(*t); } - }, - Err(e) => { - eprintln!( - " warning: poll error for build {} (definition {}): {:#}", - t.build_id, t.definition_id, e - ); - // Treat transient poll errors as still-in-progress; - // we'll retry on the next tick. - next_pending.push(*t); } } } diff --git a/src/status.rs b/src/status.rs index df9cc8e1..68fd649a 100644 --- a/src/status.rs +++ b/src/status.rs @@ -112,6 +112,18 @@ pub async fn run(opts: StatusOptions<'_>) -> Result<()> { }); let matched = match_definitions_in(&definitions, &detected); + // Surface the "no matched fixtures" case explicitly. `status` is + // read-only and intentionally non-fatal (unlike `disable` which + // bails), but rendering "(no matched definitions)" without a + // warning is indistinguishable from running in the wrong + // directory. Mirror the existing "failed to scan" warning. + if matched.is_empty() { + eprintln!( + "warning: no local fixtures matched any ADO definition under {}", + repo_path.display() + ); + } + let target_ids: HashSet = matched.iter().map(|m| m.id).collect(); let mut last_runs = std::collections::HashMap::new(); for id in &target_ids { From fda942c378e717b2dba5b0cb15f7239ca11b7bb3 Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 17 May 2026 22:28:30 +0100 Subject: [PATCH 13/14] fix(cli): no silent allowOverride downgrade; surface comma hint; harden dry-run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four follow-ups on PR #602: 1. **`apply_variable_set`: silent `allowOverride` downgrade on secret rotation.** Previously, running `secrets set TOKEN ` without `--allow-override` would re-emit the variable with `allowOverride: false`, silently downgrading any variable that was previously created (manually or by another tool) with `allowOverride: true`. The legacy `configure` code in src/configure.rs had explicit preservation logic; the consolidated `apply_variable_set` had lost it. Changed the signature from `allow_override: bool` to `allow_override: Option`: - `Some(true)` / `Some(false)` — force the flag (CLI `--allow-override` passes `Some(true)`). - `None` — **preserve** existing variable's `allowOverride` when overwriting; default to `false` when creating. `run_set` translates the CLI flag: `--allow-override` → `Some(true)`; absence → `None`. The deprecation alias (`run_set_github_token`) stays at `allow_override: false` on the CLI side, which now maps to `None` (preserve) — restoring parity with the pre-consolidation `configure` body. Help text in `src/main.rs` and `docs/cli.md` updated. Five new unit tests pin the matrix: - `Some(true)` / `Some(false)` / `None` × create/overwrite - Specifically asserts `None` preserves `allowOverride: true` (the silent-downgrade regression guard). 2. **`run.rs::print_queue_plan` silent serialize-failure.** `serde_json::to_string_pretty(&body).unwrap_or_default()` would have printed blank output if serialization ever failed. The value is provably JSON-safe, but defensive code should surface regressions instead of silently swallowing them. Switched to `unwrap_or_else(|e| format!(""))`. 3. **`run.rs::parse_parameters` opaque comma-in-value error.** When a user writes `--parameters urls=https://a,b`, the error was `Invalid --parameters pair 'b': expected key=value (no '=' found).` — technically accurate but doesn't hint at the comma constraint documented above the function. Added a raw-argument-contains-comma detection branch that produces a self-diagnosable hint: `... Hint: values must not contain commas. The raw argument '...' was split on ',' before the '=' split; use a separate --parameters flag per pair.` 4. **`run.rs::dispatch` deliberate partial-queue + `--wait` behaviour.** When `--wait` is set and some builds fail to queue, the code polls the successfully-queued ones rather than bailing early; `queue_failure` is folded into the final exit code. This is intentional and the only sensible UX, but lacked a comment. Added a multi-paragraph block explaining all three cases (partial queue, zero queued, all queued) and why `poll_until_complete` is called with the partial slice. Not addressed (acknowledged follow-ups, tracked elsewhere): - Sequential `get_latest_build` fanout in `list`/`status`. Already documented inline; tracked as a future improvement. `cargo test` (1572 unit + 14 integration crates, all green) and `cargo clippy --all-targets --all-features` pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/cli.md | 2 +- src/main.rs | 6 ++- src/run.rs | 43 ++++++++++++++++++++- src/secrets.rs | 100 ++++++++++++++++++++++++++++++++++++++++++++----- 4 files changed, 138 insertions(+), 13 deletions(-) diff --git a/docs/cli.md b/docs/cli.md index a514562f..f48760f2 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -38,7 +38,7 @@ Global flags (apply to all subcommands): `--verbose, -v` (enable info-level logg - `configure` *(deprecated; hidden in --help)* - Alias forwarding to `secrets set GITHUB_TOKEN`. Existing scripts keep working but get a stderr warning. The alias will be removed in the next minor release. - `secrets set [] [PATH]` - Set a pipeline variable (with `isSecret=true`) on every matched ADO definition. Value resolution: positional `` → `--value-stdin` (one line) → interactive tty prompt with echo off. - - `--allow-override` - Mark the variable as `allowOverride=true` (default: false). + - `--allow-override` - Force `allowOverride=true` on the set variable. When omitted, `allowOverride` is **preserved** on existing variables (so secret rotation does not silently downgrade an existing `allowOverride=true`) and defaults to `false` for new variables. - `--value-stdin` - Read the value from a single line on stdin. - `--dry-run` - Print the planned set without calling the ADO API. - `--org / --project / --pat` - ADO context overrides (same semantics as the other lifecycle commands). diff --git a/src/main.rs b/src/main.rs index 86241507..dcb381dd 100644 --- a/src/main.rs +++ b/src/main.rs @@ -47,7 +47,11 @@ enum SecretsCmd { project: Option, #[arg(long, env = "AZURE_DEVOPS_EXT_PAT")] pat: Option, - /// Mark the variable as `allowOverride=true` (default: false). + /// Force `allowOverride=true` on the set variable. When omitted, + /// `allowOverride` is preserved on existing variables (so secret + /// rotation does not silently downgrade an existing + /// `allowOverride=true`) and defaults to `false` for new + /// variables. #[arg(long)] allow_override: bool, /// Read the value from a single line on stdin. Mutually exclusive diff --git a/src/run.rs b/src/run.rs index 4880dc0d..6fe7cfbb 100644 --- a/src/run.rs +++ b/src/run.rs @@ -54,12 +54,28 @@ use crate::detect; pub fn parse_parameters(values: &[String]) -> Result> { let mut out = serde_json::Map::new(); for raw in values { + // The argument-level comma split makes values containing + // commas impossible to express today. Detect the + // ambiguous-fragment case (a comma in the raw argument and + // a fragment with no `=`) and produce a self-diagnosable + // hint instead of the bare "no '=' found" error. + let raw_has_comma = raw.contains(','); for pair in raw.split(',') { let pair = pair.trim(); if pair.is_empty() { continue; } let Some((k, v)) = pair.split_once('=') else { + if raw_has_comma { + anyhow::bail!( + "Invalid --parameters pair '{}': expected key=value (no '=' found). \ + Hint: values must not contain commas. The raw argument '{}' was \ + split on ',' before the '=' split; use a separate --parameters flag \ + per pair.", + pair, + raw + ); + } anyhow::bail!( "Invalid --parameters pair '{}': expected key=value (no '=' found).", pair @@ -215,6 +231,22 @@ pub async fn dispatch(opts: RunOptions<'_>) -> Result<()> { return Ok(()); } + // Deliberate design choice: when `--wait` is set and some builds + // failed to queue, we still poll the successfully-queued ones + // rather than bailing early. Three cases: + // + // - **Partial queue + at-least-one-queued**: `targets` is + // non-empty; the operator wants to know how those builds + // resolve. `queue_failure` is folded into the final exit code + // (non_success below). + // - **Zero queued, queue_failure > 0**: `targets` is empty; + // `poll_until_complete` returns immediately with a default + // `PollOutcome`. We still print the wait summary so the + // operator sees a uniform report shape. + // - **All queued**: the common path, no special handling needed. + // + // The early-exit path for `!opts.wait` above already bails on + // queue_failure, so no further special-casing is required here. let poll_outcome = poll_until_complete( &client, &ado_ctx, @@ -253,7 +285,16 @@ fn print_queue_plan( body["templateParameters"] = serde_json::Value::Object(parameters.clone()); } println!("[dry-run] ▶ would queue: {} (id={})", m.name, m.id); - println!("{}", serde_json::to_string_pretty(&body).unwrap_or_default()); + // The body is constructed in-line from primitive types and is + // provably JSON-serializable, so `to_string_pretty` cannot fail + // in practice. Surface any future regression as a visible token + // rather than blank output (which would be invisible in the + // dry-run feedback path). + println!( + "{}", + serde_json::to_string_pretty(&body) + .unwrap_or_else(|e| format!("")) + ); } #[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] diff --git a/src/secrets.rs b/src/secrets.rs index cc02cddc..63b793e7 100644 --- a/src/secrets.rs +++ b/src/secrets.rs @@ -48,23 +48,43 @@ pub fn validate_variable_name(name: &str) -> Result<()> { } /// Pure: produce a copy of `definition` with the named variable set -/// to `(value, isSecret=true, allow_override)`. Preserves all other +/// to `(value, isSecret=true, allowOverride)`. Preserves all other /// keys. +/// +/// `allow_override` semantics: +/// +/// - `Some(true)` / `Some(false)` — force the flag to the given +/// value (this is what `--allow-override` does, and what the +/// create path uses). +/// - `None` — **preserve** the existing variable's `allowOverride` +/// when overwriting; default to `false` when creating. This +/// matters for secret rotation: running `secrets set TOKEN ` +/// without `--allow-override` must not silently downgrade a +/// variable that was previously created with +/// `allowOverride=true`. pub fn apply_variable_set( mut definition: serde_json::Value, name: &str, value: &str, - allow_override: bool, + allow_override: Option, ) -> serde_json::Value { if definition.get("variables").is_none() || !definition["variables"].is_object() { definition["variables"] = serde_json::json!({}); } + let resolved_override = allow_override.unwrap_or_else(|| { + definition + .get("variables") + .and_then(|vars| vars.get(name)) + .and_then(|var| var.get("allowOverride")) + .and_then(|v| v.as_bool()) + .unwrap_or(false) + }); definition["variables"][name] = serde_json::json!({ "value": value, "isSecret": true, - "allowOverride": allow_override, + "allowOverride": resolved_override, }); definition } @@ -197,11 +217,27 @@ pub async fn run_set(opts: SetOptions<'_>) -> Result<()> { print_matched_summary(&matched); + // Translate the CLI bool flag into the `Option` shape that + // `apply_variable_set` understands: `--allow-override` forces + // `Some(true)`; its absence means `None` (preserve existing, or + // default to `false` on creation). This is the fix for the + // silent-downgrade bug where rotating a secret would flip an + // existing `allowOverride=true` back to `false`. + let override_action: Option = if opts.allow_override { + Some(true) + } else { + None + }; + if opts.dry_run { + let override_summary = match override_action { + Some(b) => format!("allowOverride={}", b), + None => "preserving existing allowOverride (default false on create)".to_string(), + }; println!( - "[dry-run] Would set '{}' (isSecret=true, allowOverride={}) on {} definition(s).", + "[dry-run] Would set '{}' (isSecret=true, {}) on {} definition(s).", opts.name, - opts.allow_override, + override_summary, matched.len() ); return Ok(()); @@ -217,7 +253,7 @@ pub async fn run_set(opts: SetOptions<'_>) -> Result<()> { m.id, opts.name, &value, - opts.allow_override, + override_action, ) .await { @@ -247,7 +283,7 @@ async fn apply_set_one( id: u64, name: &str, value: &str, - allow_override: bool, + allow_override: Option, ) -> Result<()> { let definition = get_definition_full(client, ctx, auth, id).await?; let updated = apply_variable_set(definition, name, value, allow_override); @@ -559,7 +595,7 @@ mod tests { #[test] fn set_creates_variables_object_when_missing() { let def = serde_json::json!({ "name": "x" }); - let out = apply_variable_set(def, "FOO", "bar", false); + let out = apply_variable_set(def, "FOO", "bar", Some(false)); assert_eq!(out["variables"]["FOO"]["value"], "bar"); assert_eq!(out["variables"]["FOO"]["isSecret"], true); assert_eq!(out["variables"]["FOO"]["allowOverride"], false); @@ -570,7 +606,7 @@ mod tests { let def = serde_json::json!({ "variables": { "OTHER": { "value": "x", "isSecret": true, "allowOverride": false } } }); - let out = apply_variable_set(def, "FOO", "bar", true); + let out = apply_variable_set(def, "FOO", "bar", Some(true)); assert_eq!(out["variables"]["OTHER"]["value"], "x"); assert_eq!(out["variables"]["FOO"]["value"], "bar"); assert_eq!(out["variables"]["FOO"]["allowOverride"], true); @@ -581,11 +617,55 @@ mod tests { let def = serde_json::json!({ "variables": { "FOO": { "value": "old", "isSecret": true, "allowOverride": false } } }); - let out = apply_variable_set(def, "FOO", "new", true); + let out = apply_variable_set(def, "FOO", "new", Some(true)); + assert_eq!(out["variables"]["FOO"]["value"], "new"); + assert_eq!(out["variables"]["FOO"]["allowOverride"], true); + } + + // ----- allow_override = None ("preserve") semantics --------------- + + /// Rotation case: `secrets set TOKEN ` without + /// `--allow-override` must NOT downgrade a variable that was + /// previously created with `allowOverride=true`. This is the + /// silent-downgrade bug guard. + #[test] + fn set_preserves_existing_allow_override_true_when_none() { + let def = serde_json::json!({ + "variables": { "FOO": { "value": "old", "isSecret": true, "allowOverride": true } } + }); + let out = apply_variable_set(def, "FOO", "new", None); assert_eq!(out["variables"]["FOO"]["value"], "new"); assert_eq!(out["variables"]["FOO"]["allowOverride"], true); } + #[test] + fn set_preserves_existing_allow_override_false_when_none() { + let def = serde_json::json!({ + "variables": { "FOO": { "value": "old", "isSecret": true, "allowOverride": false } } + }); + let out = apply_variable_set(def, "FOO", "new", None); + assert_eq!(out["variables"]["FOO"]["allowOverride"], false); + } + + #[test] + fn set_defaults_allow_override_false_on_create_when_none() { + let def = serde_json::json!({ "name": "x" }); + let out = apply_variable_set(def, "FOO", "bar", None); + assert_eq!(out["variables"]["FOO"]["allowOverride"], false); + } + + #[test] + fn set_some_false_forces_downgrade_explicit() { + // If a caller explicitly passes Some(false), they DO want to + // force the flag back to false (e.g. a future `--no-override` + // flag). Only None preserves. + let def = serde_json::json!({ + "variables": { "FOO": { "value": "old", "isSecret": true, "allowOverride": true } } + }); + let out = apply_variable_set(def, "FOO", "new", Some(false)); + assert_eq!(out["variables"]["FOO"]["allowOverride"], false); + } + // ============ apply_variable_delete ============ #[test] From f95b091d97704dd3ce5fd586a7d4a30a08e1f31e Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 17 May 2026 22:42:25 +0100 Subject: [PATCH 14/14] fix(cli): remove bails when no fixtures found; surface --parameters comma constraint in --help MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups on PR #602: 1. **`remove` silently exited Ok(()) when no fixtures were detected.** For a destructive command this is the wrong UX — running `ado-aw remove` in the wrong directory currently printed "No agentic pipelines found." and exited success, giving no signal that nothing happened. Now mirrors `disable`: bails with a non-zero exit and tells the operator which path was scanned plus the recovery hint: No local agentic pipeline fixtures were found under . Run `ado-aw compile` first (or point `ado-aw remove` at the repo root). `remove` refuses to exit success in this state because it's destructive. 2. **`--parameters` comma constraint was documented in the module doc-comment but not in `--help` text.** A user who writes `--parameters redirect_uri=https://a,b` would only learn about the constraint by reading the source. Added an inline `VALUES MUST NOT CONTAIN COMMAS …` blurb to the clap `help` attribute and updated `docs/cli.md` to match. The integration test now asserts the constraint appears in `--help` so a refactor that drops the warning will be caught at CI. `cargo test` (1572 unit + 14 integration crates, all green) and `cargo clippy --all-targets --all-features` pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/cli.md | 2 +- src/main.rs | 3 +++ src/remove.rs | 13 ++++++++++--- tests/run_integration.rs | 7 +++++++ 4 files changed, 21 insertions(+), 4 deletions(-) diff --git a/docs/cli.md b/docs/cli.md index f48760f2..94d6846c 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -97,7 +97,7 @@ Global flags (apply to all subcommands): `--verbose, -v` (enable info-level logg - `--project ` - Override: Azure DevOps project name (inferred from git remote by default). - `--pat ` / `AZURE_DEVOPS_EXT_PAT` env var - PAT for ADO API authentication (Azure CLI fallback if omitted). - `--branch ` - Source branch to queue. Defaults to the definition's `defaultBranch`. - - `--parameters ` - ADO `templateParameters`. Repeatable and/or comma-separated. All values are strings (ADO coerces as the definition requires). Rejects malformed pairs (missing `=`). + - `--parameters ` - ADO `templateParameters`. Repeatable and/or comma-separated. All values are strings (ADO coerces as the definition requires). Rejects malformed pairs (missing `=`). **Values must not contain commas** — each raw argument is split on `,` before the `=` split, so `key=https://a,b` is rejected. Use one `--parameters` flag per pair when values contain commas. - `--wait` - Poll each queued build to completion before exiting. - `--poll-interval ` - Polling period when `--wait` is set (default 10). - `--timeout ` - Hard cap on the polling loop when `--wait` is set (default 1800). diff --git a/src/main.rs b/src/main.rs index dcb381dd..96f1bb1f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -365,6 +365,9 @@ enum Commands { branch: Option, /// ADO `templateParameters` as `key=value` pairs. Repeatable and/or /// comma-separated (`--parameters a=1,b=2 --parameters c=3`). + /// VALUES MUST NOT CONTAIN COMMAS — each raw argument is split on + /// `,` before the `=` split, so `key=https://a,b` is rejected. Use + /// one `--parameters` flag per pair when values contain commas. #[arg(long)] parameters: Vec, /// Poll each queued build to completion before exiting; aggregate result diff --git a/src/remove.rs b/src/remove.rs index e7572daf..c5a8e285 100644 --- a/src/remove.rs +++ b/src/remove.rs @@ -97,10 +97,17 @@ pub async fn run(opts: RemoveOptions<'_>) -> Result<()> { println!("Scanning for agentic pipelines..."); let detected = detect::detect_pipelines(&repo_path).await?; if detected.is_empty() { - println!( - "No agentic pipelines found. Make sure your pipelines were compiled with the latest ado-aw." + // Destructive command: returning Ok(()) here would let a + // misconfigured invocation (wrong directory, missing + // `compile`) exit success with no signal that nothing + // happened. Mirror `disable`'s bail and tell the operator + // exactly which path was scanned so they can correct it. + anyhow::bail!( + "No local agentic pipeline fixtures were found under {}. \ + Run `ado-aw compile` first (or point `ado-aw remove` at the repo root). \ + `remove` refuses to exit success in this state because it's destructive.", + repo_path.display() ); - return Ok(()); } println!("Found {} agentic pipeline(s).", detected.len()); println!(); diff --git a/tests/run_integration.rs b/tests/run_integration.rs index f75b881d..b8a7d8ab 100644 --- a/tests/run_integration.rs +++ b/tests/run_integration.rs @@ -34,6 +34,13 @@ fn run_help_describes_command() { "Expected --help to advertise {flag}, got:\n{stdout}" ); } + // The comma-in-value constraint is surfaced in `--help` so users + // can self-diagnose without consulting the module doc-comment. + assert!( + stdout.contains("VALUES MUST NOT CONTAIN COMMAS") + || stdout.contains("must not contain commas"), + "Expected --help to advertise the no-commas-in-values constraint, got:\n{stdout}" + ); } #[test]