feat(cli): consolidate Phase 1 pipeline-lifecycle commands (disable/remove/list/status/run/secrets)

[jamesadevine]

Summary

Consolidates the six in-flight PRs from the Phase 1 pipeline-lifecycle CLI overhaul (#591 disable, #592 remove, #594 list, #595 run, #596 status, #600 secrets) onto a single branch with linear history.

Once this PR merges, the six original PRs will be closed (a comment on each pointing at this PR). The branch is structured as seven commits so reviewers can either look at the per-feature diff or the consolidated final state.

Commits

#	Commit	Replaces
1	feat(cli): add ado-aw disable	#591
2	feat(cli): add ado-aw remove	#592
3	feat(cli): add ado-aw list	#594
4	feat(cli): add ado-aw status	#596 (initial)
5	fix(cli): avoid duplicate ADO definition fetch in list and status	#596 (PR-review fix; introduces match_definitions_in)
6	feat(cli): add ado-aw run	#595
7	feat(cli): rename configure to secrets with subcommands	#600

CLI surface added

ado-aw disable [PATH] --paused --dry-run …auth…
ado-aw remove  [PATH] --yes --dry-run …auth…
ado-aw list    [PATH] --all --json …auth…
ado-aw status  [PATH] --json …auth…
ado-aw run     [PATH] --branch --parameters k=v --wait --poll-interval --timeout --dry-run …auth…
ado-aw secrets set <name> [<value>] [PATH] --allow-override --value-stdin --dry-run …auth…
ado-aw secrets list [PATH] --json …auth…
ado-aw secrets delete <name> [PATH] --dry-run …auth…

ado-aw configure is retained as a hidden alias that prints a stderr deprecation warning and forwards to secrets set GITHUB_TOKEN.

Plumbing

• src/ado/mod.rs — fills in the four remaining stubs from PR refactor(cli): extract shared ADO REST helpers into src/ado/mod.rs #580 (delete_definition, queue_build, get_build, get_latest_build); extends DefinitionSummary with queue_status + path; extends MatchedDefinition with queue_status; splits match_definitions into a sync match_definitions_in(definitions, detected) for callers that already have the definition list and an async wrapper for legacy callers (avoids the duplicate list_definitions fetch in list and status).
• src/configure.rs — shrinks from ~140 lines of orchestration to a ~30-line forwarder to secrets::run_set_github_token.
• src/main.rs — adds the six new Commands::* variants + the SecretsCmd subcommand group; hides Commands::Configure from --help; routes each variant to its module.
• docs/cli.md + AGENTS.md — section per command and an updated src/ inventory.

Tests

70+ new tests across the eight modules (full transition matrices, snapshot renderers, decision functions, name validation, deprecation-warning emission, clap requires constraints) plus 13 integration-test files. All run alongside the existing suite — cargo test reports 1567 passing unit tests on the main binary plus the integration crates.

Verified

• cargo build ✅
• cargo clippy --all-targets --all-features ✅ (no new findings)
• cargo test ✅ — full suite green
• Manual: ado-aw --help shows the eight new commands and hides configure. Running ado-aw configure --token x --dry-run against this repo's GitHub remote still emits the deprecation warning before bailing.

Closes

• Closes feat(cli): add ado-aw disable #591
• Closes feat(cli): add ado-aw remove #592
• Closes feat(cli): add ado-aw list #594
• Closes feat(cli): add ado-aw run #595
• Closes feat(cli): add ado-aw status #596
• Closes feat(cli): rename configure to secrets with subcommands #600

[github-actions]

🔍 Rust PR Review

Summary: One real bug (inconsistent URL percent-encoding) worth fixing before merge; everything else looks solid.

---

Findings

🐛 Bugs / Logic Issues

• src/ado/mod.rs — queue_build (line ~1102), get_build (~1159), get_latest_build (~1200) and src/secrets.rs — put_definition (~118): ctx.project is interpolated raw into the URL path, while every other HTTP helper in ado/mod.rs (including delete_definition, added in this very PR) wraps it with percent_encoding::utf8_percent_encode(&ctx.project, PATH_SEGMENT). An ADO project named "My Project" (common) silently produces a broken URL (/My Project/_apis/...). Fix is the same one-liner used everywhere else:

  percent_encoding::utf8_percent_encode(&ctx.project, PATH_SEGMENT),

  Four call sites need it: queue_build, get_build, get_latest_build in ado/mod.rs, and put_definition in secrets.rs.

⚠️ Suggestions

• src/run.rs — poll_until_complete: The started.elapsed() >= timeout guard fires at the top of the loop, before each round of get_build calls. With N in-flight builds and a 30 s per-request HTTP timeout, the real worst-case wait is timeout + N × 30 s, not just timeout. This is unlikely to bite in practice but worth noting if the poll interval ever becomes much shorter than the HTTP timeout. One option: check elapsed after each individual get_build call and break early.

✅ What Looks Good

• The match_definitions_in / match_definitions split is clean — avoids the duplicate list_definitions fetch in list + status without any API surface change.
• list_variables correctly projects VariableInfo with no value field, and the guardrail test (variable_info_has_no_value_field_in_debug_repr) is a nice belt-and-suspenders touch.
• decide_action and decide_confirm are pure functions with thorough transition-matrix tests — easy to reason about without touching the network.
• parse_parameters handles split_once('=') (values with = preserved), empty pairs, and repeated vs comma-separated flags correctly.
• Error handling throughout is consistent with project conventions: anyhow::bail! with actionable messages, .with_context() on every fallible I/O call, no unwrap() on user-facing paths.

Generated by Rust PR Reviewer for issue #602 · ● 1.1M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — solid structure, good test coverage, and the refactoring is clean. One real bug and a couple of minor suggestions.

Findings

🐛 Bugs / Logic Issues

• src/secrets.rs ~line 130 (put_definition) — ctx.project is interpolated directly into the URL path segment without percent-encoding, unlike every other URL builder in src/ado/mod.rs (which this PR explicitly fixes for get_definition_name, update_pipeline_variable, delete_definition, queue_build, get_build, get_latest_build). Project names containing spaces or other URL-unsafe characters will produce a 400/404 from the ADO API. The fix is the same one applied elsewhere in this PR:

  // Before
  let url = format!(
      "{}/{}/_apis/build/definitions/{}?api-version=7.1",
      ctx.org_url.trim_end_matches('/'),
      ctx.project,
      id
  );
  // After
  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
  );

⚠️ Suggestions

• src/secrets.rs run_set / main.rs SecretsCmd::Set — When both a positional value and --value-stdin are supplied, resolve_value silently ignores --value-stdin (explicit value wins). There's no clap conflicts_with guard. This is technically correct per the code's comment, but surprising to users. Adding #[arg(long, conflicts_with = "value")] to value_stdin in SecretsCmd::Set would make the conflict explicit and produce a cleaner error.

• src/run.rs ~line 170 (display URL) — The queued-build URL printed to stdout uses ado_ctx.project unencoded: {}/{project}/_build/results?buildId={}. This is cosmetic-only (it's never used as an HTTP target) but produces a broken/unclickable link for projects with spaces. Consistent with the encoding changes elsewhere, applying percent_encoding (or at minimum str::replace(' ', "%20")) would be good hygiene.

✅ What Looks Good

• match_definitions_in refactor — Clean extraction of the pure sync function to avoid the duplicate list_definitions fetch; the async wrapper preserves backward compatibility perfectly.
• decide_action / decide_confirm — Both are pure functions with comprehensive unit-test matrices. This is exactly the right pattern for decision logic.
• list_variables never surfaces values — The VariableInfo struct has no value field by design, and the guardrail comment + test make this invariant explicit. Good security hygiene.
• parse_parameters with split_once('=') — Correctly handles values that themselves contain = (e.g. base64 strings). Well tested.
• Error propagation — All new async functions return anyhow::Result with .context() at every fallible site. No silent unwraps in non-test code.

Generated by Rust PR Reviewer for issue #602 · ● 715.5K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — solid design with a few issues worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/status.rs:78 — unencoded project in synthesized build URL
  The fallback URL construction in render_blocks passes ado_project verbatim:

  format!(
      "{}/{}/_build/results?buildId={}",
      ado_org_url.trim_end_matches('/'),
      ado_project,   // ← no percent-encoding
      lr.id,
  )

  Every other URL construction in this PR correctly uses percent_encoding::utf8_percent_encode(&ctx.project, PATH_SEGMENT) (e.g. run.rs:197, ado/mod.rs throughout). Project names with spaces or special chars (e.g. My Project) will produce broken URLs in status text output. Easy fix: pass ado_project through PATH_SEGMENT encoding here.

• src/list.rs:288 and src/status.rs:117 — detect_pipelines errors are silently swallowed
  Both read-only commands use .unwrap_or_default() on the detection result:

  let detected = detect::detect_pipelines(&repo_path).await.unwrap_or_default();

  If detection fails for any reason other than an empty directory, users see zero matched definitions with no diagnostic, which looks identical to a "no pipelines compiled here" state. Contrast with disable and remove which propagate the error. At minimum a warn!()/eprintln!() on the Err arm would help users distinguish a detection failure from a genuinely empty workspace.

⚠️ Suggestions

• src/run.rs:28 — comma-separated parameter values are silently truncated
  parse_parameters splits every raw argument on , before splitting on =, so --parameters redirect_uri=(a.com/redacted),extra=2 silently produces {"redirect_uri": "(a.com/redacted), "extra": "2"}. There's no error or warning. A doc comment on parse_parameters noting "values must not contain commas; use a separate --parameters flag per pair" would prevent silent mis-parses. The test suite covers the expected happy path but not this edge case.

• src/configure.rs:20-33 — configure_invocation_still_works_and_warns depends on warning-before-canonicalize ordering
  The integration test passes --path /definitely-does-not-exist and relies on the deprecation warning being emitted before the path is checked. This is currently true because run_set_github_token prints to stderr as its very first statement, but if the forwarding call site ever moves (e.g. lazy-init), the test would stop catching regressions. A // IMPORTANT: warning must be emitted before any fallible IO comment in run_set_github_token would make the constraint explicit for future refactors.

✅ What Looks Good

• match_definitions_in refactor is clean — the split into a pure sync fn + async wrapper eliminates the double list_definitions fetch in list and status with zero behaviour change.
• decide_action / decide_confirm as pure functions with comprehensive transition-matrix tests is exactly the right pattern for this kind of CLI logic.
• secrets list never surfaces values — enforced structurally (no value field on VariableInfo) and verified by the variable_info_has_no_value_field_in_debug_repr test. Nicely done.
• Percent-encoding of ctx.project in all four new ado/mod.rs URL constructions is consistent and correct.
• Mid-round timeout check in poll_until_complete (re-checking elapsed between each per-build HTTP call) is a thoughtful improvement over a naive per-round check.
• The deprecation alias emitting the warning before doing any real work is the right UX for a hidden command.

Generated by Rust PR Reviewer for issue #602 · ● 700.7K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — solid architecture, thorough test coverage, and consistent error handling. One doc bug and one minor resilience suggestion worth addressing.

---

Findings

🐛 Bugs / Logic Issues

• src/run.rs ~L36-40 — The first bullet in the parse_parameters doc comment is marked ✅ but describes a pattern that doesn't work:

  /// - ✅ `--parameters 'urls=a,b' --parameters mode=fast`  → two flags
  ///   still split `urls=a,b` on the comma. Use the workaround below.
  

  The phrase "still split urls=a,b on the comma" tells you the ✅ example is broken — urls=a is stored and b fails with "no '='", identical to the ❌ case below it. Both the first and third bullets should be ❌. The test at the bottom of the module (parse_parameters_values_with_commas_split_pre_equals) confirms this is an error. The second bullet (one pair per --parameters flag, no commas in values) is the only actually working workaround and should remain ✅.

⚠️ Suggestions

• src/run.rs poll_until_complete — A permanent poll error (e.g. a 404 if the build is deleted mid-poll, or an auth failure) is indistinguishable from a transient one; both push the target back onto next_pending and retry silently until --timeout. A consecutive-error counter per build (e.g. bail out of that specific build after 3 consecutive poll failures and count it as failed) would surface persistent failures minutes earlier, especially with a long --timeout. Not blocking, but worth a follow-up issue.

• src/status.rs vs src/disable.rs — no-fixtures behaviour — status swallows fixture-detection failures with unwrap_or_else(..., Vec::new()) and renders (no matched definitions) silently, while disable calls anyhow::bail! on the same condition. Both choices are defensible (read-only vs write), but the asymmetry may confuse users who see status return successfully with no rows after a misconfigured path. A terse eprintln!("warning: no local fixtures found under {path}") on the empty-match path in status::run would be consistent with the existing "failed to scan local pipelines" warning it already emits.

✅ What Looks Good

• The match_definitions_in refactor cleanly eliminates the duplicate list_definitions fetch in list and status without changing callers that don't need it.
• Every new ADO REST URL correctly applies percent_encoding::utf8_percent_encode to ctx.project — the PATH_SEGMENT set going public makes that pattern easy to follow elsewhere.
• VariableInfo having no value field is a well-designed safety property, and the variable_info_has_no_value_field_in_debug_repr compile-time guardrail test is excellent.
• The configure deprecation alias is structured to emit its eprintln! warning before any fallible I/O — the invariant comment and the integration test that validates it are exactly the right approach.
• Pure decision functions (decide_action, decide_confirm, classify_build, parse_parameters) extracted from the async command runners, each with a complete transition-matrix test suite.

Generated by Rust PR Reviewer for issue #602 · ● 1.8M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — well-structured consolidation with strong test coverage, consistent error handling, and correct security properties. A few items worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/run.rs:261 — silent swallow in dry-run JSON printer

  println!("{}", serde_json::to_string_pretty(&body).unwrap_or_default());

  unwrap_or_default() on Result<String, _> silently prints nothing if serialization fails. The value constructed is trivially safe so it can't error in practice, but this is the only callsite in the new code that doesn't propagate errors. Given this is the dry-run feedback path, a user getting blank output here would be confused. Prefer unwrap_or_else(|e| format!("<serialization error: {e}>")) or just unwrap() since it's provably infallible.

• src/run.rs — --wait with partial queue failure proceeds without early exit
  When some builds fail to queue and --wait is set, poll_until_complete is called on the partial targets slice (the successfully queued ones), and queue_failure is only added to non_success at the end. This is intentional and actually correct — you want to wait for the builds that did queue. However the code path where all builds fail to queue (empty targets) causes poll_until_complete to be called with an empty slice, which harmlessly returns immediately. No bug, but a comment explaining the deliberate "proceed with partial targets" design would help future maintainers.

⚠️ Suggestions

• src/secrets.rs:apply_variable_set — silent allowOverride overwrite

  definition["variables"][name] = serde_json::json!({
      "value": value,
      "isSecret": true,
      "allowOverride": allow_override,
  });
  ```
  If the variable already exists with `allowOverride: true`, running `secrets set <name> <value>` (without `--allow-override`) silently downgrades it to `allowOverride: false`. A user rotating a secret (most common use case) would not expect this side effect. Consider either (a) reading and preserving the existing `allowOverride` when `--allow-override` is not explicitly passed, or (b) calling this out in the command help string so operators can make an informed choice.

• src/run.rs:parse_parameters — comma-in-value error message is misleading
  When a user passes --parameters urls=(a/redacted),b, the error they see is:

  Invalid --parameters pair 'b': expected key=value (no '=' found).
  

  The docstring explains the limitation well, but the error message gives no hint that a comma in the value caused the split. Since the docstring exists, threading a hint like "(values must not contain commas — use one --parameters flag per pair)" into the bail message would make this self-diagnosable without consulting docs.

• src/list.rs / src/status.rs — sequential get_latest_build fanout
  Already acknowledged in the comment "bounded fanout via JoinSet is a straightforward future improvement". Worth tracking as a follow-up issue since even 10 matched definitions → 10 serial HTTP round-trips with the 30s client timeout creates a worst-case 5-minute wall time.

✅ What Looks Good

• match_definitions_in refactor is clean — factoring out the sync function and making match_definitions a thin async wrapper eliminates the redundant list_definitions fetch in list and status without changing any observable behaviour.
• Security properties of secrets list — zero code paths read or surface variable values. The list_variables pure function explicitly projects only name + flags and is unit-tested.
• decide_confirm / remove::run safety model is solid: bulk deletes unconditionally require --yes, single-match on a non-tty unconditionally requires --yes, interactive prompt only on a tty single-match. The full matrix is tested.
• Consistent percent-encoding of ctx.project across all four new ADO REST helpers (delete_definition, queue_build, get_build, get_latest_build) via the now-pub PATH_SEGMENT constant. No regressions on the existing encode points either.
• MAX_CONSECUTIVE_POLL_ERRORS = 3 bounds transient poll error damage without masking persistent failures — nice resilience pattern.

Generated by Rust PR Reviewer for issue #602 · ● 2.2M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-structured, consistently error-handled, comprehensively tested. One behavioral inconsistency worth fixing before merge and one minor concern to be aware of.

Findings

🐛 Bugs / Logic Issues

• src/remove.rs:113 — When no local fixtures are detected, remove silently returns Ok(()) and exits 0. disable bails with a non-zero exit code in the same situation (line 129 of disable.rs). For a destructive command this is the wrong behavior: running ado-aw remove in the wrong directory currently prints "No agentic pipelines found." and exits success, giving no signal that nothing happened. It should mirror disable and call anyhow::bail! with the repo-path and compile hint.

⚠️ Suggestions

• src/run.rs:26-54 (parse_parameters) — The limitation that values may not contain commas is documented in the module doc-comment but not surfaced in --help text. A user who writes --parameters redirect_uri=(a.com/redacted),b.com will get a confusing "no '=' found" error on the fragment b.com. The diagnostic hint added when raw_has_comma is present is good, but consider adding a short one-liner to the clap #[arg(help)] annotation so it appears at ado-aw run --help as well.

✅ What Looks Good

• match_definitions_in refactor neatly eliminates the redundant list_definitions HTTP round-trip in list and status without changing behaviour for other callers.
• apply_variable_set allowOverride preservation is a subtle correctness win — the None → preserve / Some(b) → force split correctly avoids silently downgrading an existing allowOverride=true during secret rotation.
• list_variables never reads the value field — verified. The JSON shape in run_list also only emits name, isSecret, and allowOverride.
• run_set_github_token deprecation ordering — emitting the eprintln! before any fallible I/O is the right call and the integration-test invariant note in the comment makes the intent clear to future maintainers.
• Error handling across all six new modules: consistent anyhow::bail! / .context() patterns, no unwrap/expect outside #[cfg(test)], and URL encoding with PATH_SEGMENT is uniformly applied to ctx.project in all new REST calls.
• MAX_CONSECUTIVE_POLL_ERRORS + mid-round timeout recheck in poll_until_complete is solid defensive engineering.

Generated by Rust PR Reviewer for issue #602 · ● 2.2M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-structured consolidation with thorough testing and clean security-conscious design.

Findings

✅ What Looks Good

• Pure-function architecture: Every command separates decision logic (decide_action, decide_confirm, parse_parameters, apply_variable_set, build_rows) from IO, enabling the tight unit-test coverage seen across all modules. This is the right pattern.

• allowOverride preservation on rotation (src/secrets.rs:76–84): The None sentinel correctly preserves existing allowOverride=true when rotating secrets without --allow-override, preventing a silent security downgrade. This is an improvement over the old configure path.

• PATH_SEGMENT encoding applied consistently: All six new URL-building call sites in ado/mod.rs and secrets.rs encode ctx.project through PATH_SEGMENT, matching the existing patch_queue_status / list_definitions pattern.

• secrets list never surfaces values: The list_variables function (src/secrets.rs:120–140) projects only name, isSecret, and allowOverride — the value field is structurally absent from the output, not just filtered at print time.

• configure deprecation wires to the same code path (src/secrets.rs:471–497): The forwarding avoids a parallel implementation, so there's no divergence risk. The allow_override: false → None path correctly preserves existing flags through apply_variable_set, meaning the deprecated command now has better rotation semantics than the old implementation.

• Fail-safe destructive operations (src/remove.rs, src/disable.rs): Both commands bail with actionable messages when no local fixtures are found, refusing to exit success with no signal that nothing happened.

⚠️ Suggestions

• src/run.rs — --parameters comma limitation is surprising for URL values: The doc-comment and --help text call this out clearly, but the current split-on-comma behaviour means --parameters redirect_uri=(example.com/redacted) silently works (no commas), but --parameters redirect_uri=(a.com/redacted),b.com/yfails with a confusing fragment error. Consider clamping the comma-split to only activate when the raw argument contains a,*and* at least one comma-separated token also contains=(i.e., the optimistic "this looks like multiple pairs" case) — otherwise treat the whole argument as a singlekey=value`. This would preserve the convenience shorthand while not rejecting valid single-pair values that happen to have a comma on the right-hand side.

• src/list.rs:302 — sequential get_latest_build fetches: The code comment already acknowledges this; noting it here for visibility. For teams with 20+ matched pipelines this could add meaningful latency. A tokio::task::JoinSet fan-out (bounded at, say, 8) would be a straightforward improvement whenever this becomes a pain point.

• src/secrets.rs — ADO GET→PUT round-trip swaps *** back for all other secrets (pre-existing, not introduced here): When secrets set FOO bar fetches the full definition and PUTs it back, every other secret variable's value is *** (ADO masks them on GET). The PUT stores those literal *** strings, overwriting the real values. This is the same behaviour as the old configure path and is an ADO API constraint, but operators using secrets set to rotate one of several secrets should be aware of it. A note in docs/cli.md under secrets set would save a future debugging session.

Generated by Rust PR Reviewer for issue #602 · ● 1.6M · ◷