feat(cli): add ado-aw disable#591
Closed
jamesadevine wants to merge 2 commits into
Closed
Conversation
Implements PR 3 of the Phase 1 CLI overhaul: sets `queueStatus` to
`disabled` (default) or `paused` on every ADO build definition that
matches a local fixture.
Behaviour:
- Discover local fixtures from `PATH` (same auto-discovery as
`compile`), then run `match_definitions` to map them to ADO
definition IDs. Definitions without a local fixture are never in
the returned set — `disable` therefore cannot accidentally touch an
unrelated definition. This safety property falls naturally out of
the existing matcher rather than from any new explicit check.
- For each match: read current `queueStatus`; skip when already at
target; otherwise patch via the existing `patch_queue_status`
helper.
- If `match_definitions` returns zero results, exit non-zero with a
hint pointing at `ado-aw list` to diagnose. Fail-soft per fixture;
any failed patch returns non-zero overall.
Plumbing change: `MatchedDefinition` gains a
`queue_status: Option<String>` field, populated by the two
list-based branches of `match_definitions`. Explicit-ID matches
remain `None` since the explicit path doesn't fetch the full
definition; `decide_action` patches in that case (treats `None` as
"not confirmed at target"), which is the safe default.
CLI surface:
ado-aw disable [PATH] --org --project --pat --paused --dry-run
Tests:
- Unit (disable.rs): full transition matrix —
enabled→disabled, enabled→paused, disabled→disabled (skip),
paused→paused (skip), disabled→paused, paused→disabled, plus
`None`/empty `queue_status` → patch-with-from-unknown.
- Integration (tests/disable_integration.rs): `--help` advertises the
documented surface, and top-level `--help` lists the subcommand.
Reuses the four ado/mod.rs stubs landed in PR 2 (#583); no new
network helpers required.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
🔍 Rust PR ReviewSummary: Looks good — clean implementation with solid test coverage; two minor UX clarity points worth considering. Findings
|
Collaborator
Author
|
@copilot address pr feedback and suggestions |
Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/a1d24ebd-0544-4978-a88b-b66cdb74d407 Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Contributor
Addressed the PR feedback in f8811c9: Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Collaborator
Author
jamesadevine
added a commit
that referenced
this pull request
May 17, 2026
…emove/list/status/run/secrets) (#602) * 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> * feat(cli): add ado-aw remove Squash-merge of #592 (feat/cli-remove). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * feat(cli): add ado-aw list Squash-merge of #594 (feat/cli-list). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * feat(cli): add ado-aw status 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> * 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> * feat(cli): add ado-aw run Squash-merge of #595 (feat/cli-run). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * 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> * 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> * fix(run): re-check --timeout between each in-flight get_build call 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> * 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> --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> * fix(cli): encode synthesized URL, surface detect errors, document parse_parameters edge 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> * fix(cli): correct parse_parameters doc; cap consecutive poll errors; warn on empty status 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<u64, usize>` 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> * fix(cli): no silent allowOverride downgrade; surface comma hint; harden dry-run Four follow-ups on PR #602: 1. **`apply_variable_set`: silent `allowOverride` downgrade on secret rotation.** Previously, running `secrets set TOKEN <new>` 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<bool>`: - `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!("<serialization error: {e}>"))`. 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> * fix(cli): remove bails when no fixtures found; surface --parameters comma constraint in --help 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 <path>. 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> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements PR 3 of the Phase 1 CLI overhaul. Adds
ado-aw disable [PATH]— setsqueueStatustodisabled(default) orpausedon every ADO build definition that matches a local fixture.Builds on the
MatchedDefinitionmatcher introduced forconfigureand refined forenable(#583). The four ado/mod.rs network helpers landed in PR 2 (get_repository_id,create_definition,patch_queue_status,get_definition_full) cover everythingdisableneeds — this PR only consumespatch_queue_statusand adds no new wire-level code.CLI surface
Behaviour
PATH(same auto-discovery ascompile).match_definitionsto map fixtures → ADO definition IDs. Definitions without a local fixture are never in the returned set;disabletherefore cannot accidentally touch an unrelated definition. The safety property falls naturally out of the existing matcher rather than from any new explicit check.queueStatus. Skip when already at target.patch_queue_status(GET → mutate → PUT) helper.match_definitionsreturns zero matches: exit non-zero with a hint pointing atado-aw listto diagnose. Fail-soft per fixture; any failed patch returns non-zero overall.--pausedvs defaultADO distinguishes the two queue statuses:
disabledpauseddisabledefaults todisabled(the destructive choice you usually want for "turn this off");--pausedis for the "stop new runs but let scheduled queue items pile up while I diagnose" case.Plumbing change
MatchedDefinitiongains aqueue_status: Option<String>field, populated by the two list-based branches ofmatch_definitions. Explicit-ID matches stayNonesince the explicit path doesn't fetch the full definition;decide_actionpatches in that case (treatsNoneas "not confirmed at target") — the safe default.Verified
cargo build✅cargo clippy --all-targets --all-features✅ (no new findings)cargo test✅ — 9 new unit tests indisable.rs(full transition matrix incl.None/emptyqueue_status→ patch-with-from-unknown) + 2 integration tests intests/disable_integration.rs. One flakymcp_http_tests::test_health_endpoint_no_auth_requiredfailure observed under load that passes cleanly in isolation; unrelated to this PR.ado-aw disable --helpadvertises the documented surface.Out of scope
PRs 4–8 (
remove/list/run/status/secrets), PR 9 (help-text grouping), and PR 10 (doc overhaul).