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>

Author: jamesadevine

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<PollTarget> = 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<u64, usize> =
+        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);
                 }
             }
         }

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<u64> = matched.iter().map(|m| m.id).collect();
     let mut last_runs = std::collections::HashMap::new();
     for id in &target_ids {