fix(secrets): address fourth-round Rust PR review feedback on #624

- `normalize_repo_url`: percent-decode before comparing, so a project
  named "My Project" matches whether ADO returns the encoded form
  (`My%20Project`) or the decoded form. The previous implementation
  assumed ADO always returns percent-encoded URLs; that assumption is
  documented in code now and the comparison is encoding-independent.
  New unit tests cover the encoded/decoded equivalence and the
  case-insensitive/trailing-slash behaviour.

- `discovered_to_matched`: stop silently truncating consumers that
  include multiple ado-aw templates. The `yaml_path` field used by
  `print_matched_summary` now joins every marker source with `, ` so
  e.g. `agents/a.md, agents/b.md` shows up honestly in the CLI
  summary. New unit test asserts both sources are surfaced.

- `##vso[` defence-in-depth: the marker step's runtime echo already
  neutralises `##vso[` and `##[` prefixes, but the same raw source
  string was flowing through `MarkerMetadata` -> `MatchedDefinition::yaml_path`
  -> `print_matched_summary` (which writes to stdout). When the CLI
  is invoked from inside an ADO pipeline step, the agent's stdout
  scanner would still pick up an attacker-controlled `##vso[...]`
  payload. New `sanitize_for_vso_logging` helper in the discovery
  module applies the same convention (`##vso[` -> `[vso-filtered][`,
  `##[` -> `[filtered][`) when building the `yaml_path`. New unit test
  asserts the sanitisation.

- `ADO_AW_PREVIEW_CONCURRENCY=0` now emits a `warn!` before clamping
  to 1, instead of silently masking the typo. Operators who set `=0`
  will see the warning and can correct the env value rather than
  wondering why their concurrency tuning had no effect.

- New unit test for the `--source` + no-git-remote bail in
  `secrets::resolve_for_command`: previously the helpful "no Azure
  DevOps git remote was detected; try --all-repos" error path was
  untested. Now asserted via a `tokio::test` that constructs an empty
  AdoContext and verifies the error message contains both the cause
  and the suggested mitigation.

All 1753 tests pass; clippy clean on touched files.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>

Author: jamesadevine

src/ado/discovery.rs

@@ -241,10 +241,24 @@ pub async fn discover_ado_aw_pipelines(
     // `match_definitions_in`.
     let lock_map = build_lock_path_map(local_lock_paths);
 
-    let permits = std::env::var("ADO_AW_PREVIEW_CONCURRENCY")
+    // Resolve concurrency from env; warn (not silently clamp) when an
+    // operator sets `=0`, since the deadlock-avoidance `.max(1)` would
+    // mask the typo and leave the user wondering why throughput hasn't
+    // changed.
+    let raw_permits = std::env::var("ADO_AW_PREVIEW_CONCURRENCY")
         .ok()
-        .and_then(|v| v.parse().ok())
-        .unwrap_or(DEFAULT_PREVIEW_CONCURRENCY);
+        .and_then(|v| v.parse::<usize>().ok());
+    let permits = match raw_permits {
+        Some(0) => {
+            warn!(
+                "ADO_AW_PREVIEW_CONCURRENCY=0 would deadlock the Preview semaphore; \
+                 clamping to 1. Set a positive integer to control concurrency.",
+            );
+            1
+        }
+        Some(n) => n,
+        None => DEFAULT_PREVIEW_CONCURRENCY,
+    };
     let semaphore = Arc::new(Semaphore::new(permits.max(1)));
 
     let mut handles = Vec::with_capacity(filtered.len());
@@ -310,11 +324,28 @@ fn apply_scope_filter(
     }
 }
 
-/// Normalize a repo URL for equality comparison. Strips trailing slash
-/// and lowercases the scheme/host portion (ADO is case-insensitive on
-/// org/project/repo names).
+/// Normalize a repo URL for equality comparison.
+///
+/// Two normalizations are applied so the comparison is robust to the
+/// shape ADO returns:
+///
+///  1. **Percent-decode** the URL so a project named e.g. `My Project`
+///     compares equal whether ADO returned `My%20Project` or the (rare
+///     but legal) decoded `My Project`. Lossy decoding on invalid UTF-8
+///     keeps us forward-compatible — anything ADO can return, we can
+///     compare.
+///  2. **ASCII-lowercase** because ADO is case-insensitive on org /
+///     project / repo identifiers, and trim any trailing `/`.
+///
+/// Without (1), the comparison would silently fail for any project
+/// name containing a percent-reserved character if `ado_ctx.repo_url()`
+/// emitted the encoded form and `repository.url` returned the decoded
+/// form (or vice-versa).
 fn normalize_repo_url(url: &str) -> String {
-    url.trim_end_matches('/').to_ascii_lowercase()
+    let decoded = percent_encoding::percent_decode_str(url)
+        .decode_utf8_lossy()
+        .into_owned();
+    decoded.trim_end_matches('/').to_ascii_lowercase()
 }
 
 /// Build a `(normalized yamlFilename → local lock path)` lookup table
@@ -518,21 +549,47 @@ pub fn discovered_to_matched(d: &DiscoveredPipeline) -> Option<MatchedDefinition
         | DiscoveryStatus::PreviewFailed(_) => return None,
     }
 
+    // Join every marker's source path so consumers that include
+    // multiple templates show up honestly in the CLI summary instead
+    // of silently truncating to whichever marker happened to be
+    // first. Also apply `sanitize_for_vso_logging` here: the
+    // `yaml_path` ends up in `print_matched_summary` (which writes to
+    // stdout), and if `ado-aw secrets set --all-repos` is ever invoked
+    // from inside an ADO pipeline step, an attacker-controlled marker
+    // source path containing `##vso[` would otherwise be processed by
+    // the agent's logging-command scanner.
+    let yaml_path = if d.markers.is_empty() {
+        String::new()
+    } else {
+        let joined = d
+            .markers
+            .iter()
+            .map(|m| m.source.as_str())
+            .collect::<Vec<_>>()
+            .join(", ");
+        sanitize_for_vso_logging(&joined)
+    };
+
     Some(MatchedDefinition {
         id: d.definition_id,
         name: d.definition_name.clone(),
         match_method: MatchMethod::Discovery,
-        // Prefer the first marker's source path so downstream summaries
-        // can show "→ agents/foo.md" without further lookup.
-        yaml_path: d
-            .markers
-            .first()
-            .map(|m| m.source.clone())
-            .unwrap_or_default(),
+        yaml_path,
         queue_status: d.queue_status.clone(),
     })
 }
 
+/// Neutralise ADO build-agent logging-command prefixes (`##vso[`,
+/// `##[`). Mirrors `crate::compile::extensions::ado_aw_marker` and
+/// `crate::agent_stats::sanitize_for_markdown` so that data flowing
+/// from a Preview-discovered marker through the CLI's own stdout
+/// cannot smuggle a `task.setvariable` (or similar) when the CLI is
+/// invoked from inside an ADO pipeline step.
+fn sanitize_for_vso_logging(s: &str) -> String {
+    s.replace("##vso[", "[vso-filtered][")
+        .replace("##[", "[filtered][")
+}
+
 /// CLI-facing wrapper: run Preview-driven discovery with the given
 /// scope, optionally filter to consumers of a single template source,
 /// and return the result as `Vec<MatchedDefinition>`.
@@ -915,4 +972,95 @@ mod tests {
         assert!(discovered_to_matched(&discovered(DiscoveryStatus::Direct)).is_some());
         assert!(discovered_to_matched(&discovered(DiscoveryStatus::Consumer)).is_some());
     }
+
+    #[test]
+    fn discovered_to_matched_joins_multiple_marker_sources() {
+        // A consumer that includes two templates must surface both
+        // sources in the yaml_path summary, not silently truncate to
+        // whichever happened to be first.
+        let mut d = discovered(DiscoveryStatus::Consumer);
+        d.markers = vec![
+            MarkerMetadata {
+                schema: 1,
+                source: "agents/a.md".to_string(),
+                version: "1.0".to_string(),
+                target: "job".to_string(),
+            },
+            MarkerMetadata {
+                schema: 1,
+                source: "agents/b.md".to_string(),
+                version: "1.0".to_string(),
+                target: "stage".to_string(),
+            },
+        ];
+        let matched = discovered_to_matched(&d).expect("Consumer kept");
+        assert!(
+            matched.yaml_path.contains("agents/a.md")
+                && matched.yaml_path.contains("agents/b.md"),
+            "expected both marker sources in yaml_path, got: {}",
+            matched.yaml_path
+        );
+    }
+
+    #[test]
+    fn discovered_to_matched_sanitises_vso_in_yaml_path() {
+        // The yaml_path ends up in stdout via print_matched_summary.
+        // If the CLI is invoked from inside an ADO pipeline step, an
+        // attacker-controlled marker source path containing `##vso[`
+        // would otherwise be processed by the agent's logging-command
+        // scanner.
+        let mut d = discovered(DiscoveryStatus::Consumer);
+        d.markers = vec![MarkerMetadata {
+            schema: 1,
+            source: "agents/##vso[task.setvariable variable=X]value.md".to_string(),
+            version: "1.0".to_string(),
+            target: "job".to_string(),
+        }];
+        let matched = discovered_to_matched(&d).expect("Consumer kept");
+        assert!(
+            !matched.yaml_path.contains("##vso["),
+            "raw ##vso[ leaked into yaml_path: {}",
+            matched.yaml_path,
+        );
+        assert!(
+            matched.yaml_path.contains("[vso-filtered]["),
+            "expected `##vso[` neutralised to `[vso-filtered][`: {}",
+            matched.yaml_path,
+        );
+    }
+
+    // ── normalize_repo_url ───────────────────────────────────────────
+
+    #[test]
+    fn normalize_repo_url_is_encoding_independent() {
+        // ADO usually returns percent-encoded URLs (`My%20Project`),
+        // but the comparison must work whichever shape both sides
+        // happen to be in.
+        let encoded = "https://dev.azure.com/Org/My%20Project/_git/Repo";
+        let decoded = "https://dev.azure.com/Org/My Project/_git/Repo";
+        assert_eq!(normalize_repo_url(encoded), normalize_repo_url(decoded));
+    }
+
+    #[test]
+    fn normalize_repo_url_is_case_insensitive_and_trims_trailing_slash() {
+        assert_eq!(
+            normalize_repo_url("https://dev.azure.com/Org/P/_git/Repo/"),
+            normalize_repo_url("https://dev.azure.com/org/p/_git/repo")
+        );
+    }
+
+    // ── sanitize_for_vso_logging ─────────────────────────────────────
+
+    #[test]
+    fn discovery_sanitize_for_vso_logging_neutralises_prefixes() {
+        assert_eq!(
+            sanitize_for_vso_logging("##vso[task.setvariable variable=X]value"),
+            "[vso-filtered][task.setvariable variable=X]value"
+        );
+        assert_eq!(
+            sanitize_for_vso_logging("##[warning]ignore me"),
+            "[filtered][warning]ignore me"
+        );
+        assert_eq!(sanitize_for_vso_logging("agents/foo.md"), "agents/foo.md");
+    }
 }

src/secrets.rs

@@ -704,6 +704,48 @@ mod tests {
         assert_eq!(out["variables"]["FOO"]["allowOverride"], false);
     }
 
+    // ============ resolve_for_command precondition ============
+
+    #[tokio::test]
+    async fn source_without_all_repos_bails_when_no_git_remote() {
+        // CurrentRepo scope + empty repo_name = no git remote was
+        // detected. `--source` users hitting this case must get a
+        // targeted error mentioning `--all-repos`, not a generic
+        // "no pipelines found" further down the pipeline.
+        let ctx = AdoContext {
+            org_url: "https://dev.azure.com/example".to_string(),
+            project: "p".to_string(),
+            repo_name: String::new(),
+        };
+        let auth = AdoAuth::Pat("token".to_string());
+        let client = reqwest::Client::builder()
+            .build()
+            .expect("client builds");
+        let tmp = tempfile::tempdir().unwrap();
+
+        let err = resolve_for_command(
+            &client,
+            &ctx,
+            &auth,
+            None,
+            false,
+            Some("agents/foo.md"),
+            tmp.path(),
+        )
+        .await
+        .expect_err("expected bail");
+
+        let msg = format!("{err}");
+        assert!(
+            msg.contains("--all-repos"),
+            "error should suggest --all-repos; got: {msg}"
+        );
+        assert!(
+            msg.contains("no Azure DevOps git remote"),
+            "error should explain the root cause; got: {msg}"
+        );
+    }
+
     #[test]
     fn set_preserves_other_variables() {
         let def = serde_json::json!({