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

- `AdoAwMarkerExtension`: neutralise `##vso[` and `##[` logging-command
  prefixes in the source path before embedding it in the marker step's
  runtime `echo` line. Without this, a markdown filename like
  `agents/##vso[task.setvariable variable=FOO]value.md` would echo a
  literal `##vso[...]` sequence that the ADO build agent's stdout
  scanner treats as a task command — a logging-command-injection
  primitive any attacker controlling a filename could trigger. New
  `sanitize_for_vso_logging` helper mirrors the existing convention in
  `crate::agent_stats::sanitize_for_markdown` (`[vso-filtered][` /
  `[filtered][`). The `# ado-aw-metadata:` JSON line keeps the raw
  value (it's a YAML comment, not echoed to stdout). Two new tests:
  the sanitiser unit test and an end-to-end attack-payload roundtrip
  asserting the echo line is neutralised.

- `resolve_definitions_via_discovery`: the previous skip-counter
  implementation counted `UnknownRequiredParams` / `Forbidden` /
  `PreviewFailed` failures *before* applying `source_filter`, so under
  `--source agents/foo.md` the warnings would tell the user "N
  definitions skipped requiring template parameters" for definitions
  that had nothing to do with `agents/foo.md`. Split the counting:

  * without `--source`: per-status counts are honest (we're operating
    on every ado-aw pipeline) and the existing three warnings stand;
  * with `--source`: a single conservative `uninspectable` counter,
    surfaced as one warning that explicitly acknowledges we can't tell
    whether any of those skipped definitions would have been consumers
    of the filtered template.

- `src/ado/discovery.rs`: drop the file-level `#![allow(dead_code)]`.
  `resolve_definitions_via_discovery` and `discovered_to_matched` are
  now wired into `secrets.rs`; the suppression was hiding future
  dead-code regressions. Build is clean without it.

- `src/main.rs` (`SecretsCmd`): clarified `--source` help text — calls
  out that **without `--all-repos`, only the current repository is
  searched**. Saves the user-confusion case "I ran `secrets set
  GITHUB_TOKEN --source agents/foo.md` and got zero results" when
  they're in a different repo than the consumer pipelines.

All 1743 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

@@ -31,8 +31,6 @@
 //! `src/compile/extensions/ado_aw_marker.rs` solves the stripping
 //! problem by embedding the marker inside a bash heredoc.
 
-#![allow(dead_code)] // Wired into CLI commands in workstream S; tests below cover it now.
-
 use anyhow::{Context, Result};
 use log::{debug, warn};
 use serde::Deserialize;
@@ -509,11 +507,22 @@ pub fn discovered_to_matched(d: &DiscoveredPipeline) -> Option<MatchedDefinition
 /// markers reference that source path are kept. Match is by exact
 /// equality on the normalized source string in the marker JSON.
 ///
-/// Definitions whose Preview call failed in a known-recoverable way
-/// (`UnknownRequiredParams` / `UnknownForbidden` / `PreviewFailed`) are
-/// counted and surfaced as a `warn!` so the operator can see that
-/// some pipelines were skipped — silently dropping them would be a
-/// nasty surprise for `secrets set --all-repos`.
+/// Skip-summary warnings are emitted differently depending on whether
+/// `source_filter` is active:
+///
+/// - **Unfiltered (`--all-repos` alone)**: every `UnknownRequiredParams`
+///   / `UnknownForbidden` / `PreviewFailed` definition is counted —
+///   under `--all-repos` the user is operating on every ado-aw pipeline
+///   in scope, so each failure represents a real skip.
+///
+/// - **Filtered (`--source <path>`)**: we can't tell whether a failed
+///   definition would have been a consumer of `path` because we never
+///   got markers out of it. Emitting per-status counts would mislead
+///   the user into thinking they're missing consumers of their
+///   template. Instead, emit a single conservative warning ("N
+///   definitions could not be inspected; consumers of `<path>` among
+///   them may have been silently skipped") so the operator is informed
+///   without being told false specifics.
 pub async fn resolve_definitions_via_discovery(
     client: &reqwest::Client,
     ctx: &AdoContext,
@@ -527,18 +536,39 @@ pub async fn resolve_definitions_via_discovery(
     let mut skipped_required_params = 0usize;
     let mut skipped_forbidden = 0usize;
     let mut skipped_failed = 0usize;
+    let mut uninspectable = 0usize;
 
     let kept: Vec<_> = discovered
         .into_iter()
         .filter(|d| {
-            match &d.status {
-                DiscoveryStatus::UnknownRequiredParams => skipped_required_params += 1,
-                DiscoveryStatus::UnknownForbidden => skipped_forbidden += 1,
-                DiscoveryStatus::PreviewFailed(_) => skipped_failed += 1,
-                _ => {}
+            let matches_filter = match source_filter {
+                Some(src) => d.markers.iter().any(|m| m.source == src),
+                None => true,
+            };
+
+            // Count toward skip totals only when the failure is
+            // relevant to the requested operation:
+            //  - unfiltered: every failure is relevant (we're operating
+            //    on every ado-aw pipeline in scope);
+            //  - filtered: we can't attribute uninspectable definitions
+            //    to a specific source, so use a single combined counter.
+            if source_filter.is_none() {
+                match &d.status {
+                    DiscoveryStatus::UnknownRequiredParams => skipped_required_params += 1,
+                    DiscoveryStatus::UnknownForbidden => skipped_forbidden += 1,
+                    DiscoveryStatus::PreviewFailed(_) => skipped_failed += 1,
+                    _ => {}
+                }
+            } else if matches!(
+                d.status,
+                DiscoveryStatus::UnknownRequiredParams
+                    | DiscoveryStatus::UnknownForbidden
+                    | DiscoveryStatus::PreviewFailed(_)
+            ) {
+                uninspectable += 1;
             }
-            let Some(src) = source_filter else { return true };
-            d.markers.iter().any(|m| m.source == src)
+
+            matches_filter
         })
         .collect();
 
@@ -561,6 +591,15 @@ pub async fn resolve_definitions_via_discovery(
              an unexpected error. Re-run with --debug to see details.",
         );
     }
+    if uninspectable > 0
+        && let Some(src) = source_filter
+    {
+        warn!(
+            "Discovery could not inspect {uninspectable} definition(s) (Preview failure, \
+             forbidden, or required-parameters); any consumers of `{src}` among them have \
+             been silently skipped. Re-run with --debug for per-definition reasons.",
+        );
+    }
 
     Ok(kept.iter().filter_map(discovered_to_matched).collect())
 }

src/compile/extensions/ado_aw_marker.rs

@@ -69,12 +69,21 @@ impl CompilerExtension for AdoAwMarkerExtension {
         // the build log at runtime, which is a free human-discoverability
         // bonus and costs nothing because the step runs in milliseconds.
         //
-        // The echo uses single quotes to keep the literal intact at
-        // runtime; we apply the bash `'\''` idiom to any `'` inside the
-        // source path so a markdown filename like `agents/foo's.md`
-        // doesn't produce broken bash. `version` and `target` are
-        // controlled inputs and can't contain `'`.
-        let echo_source = bash_single_quote_escape(&source);
+        // The echo's source value goes through two sanitisations:
+        //
+        //  1. `sanitize_for_vso_logging` neutralises `##vso[` and `##[`
+        //     prefixes. The ADO build agent scans stdout for those
+        //     sequences and treats them as logging commands (e.g.
+        //     `task.setvariable`). An attacker who controls a markdown
+        //     filename could otherwise inject a logging command into
+        //     the build log via the echoed source path. Same convention
+        //     used by `agent_stats::sanitize_for_markdown`.
+        //
+        //  2. `bash_single_quote_escape` applies the `'\''` idiom so a
+        //     filename containing `'` (e.g. `agents/foo's.md`) doesn't
+        //     produce syntactically broken bash. `version` and `target`
+        //     are controlled inputs and can't contain either.
+        let echo_source = bash_single_quote_escape(&sanitize_for_vso_logging(&source));
         let step = format!(
             "- bash: |\n    \
                 # ado-aw-metadata: {metadata}\n    \
@@ -97,6 +106,15 @@ fn bash_single_quote_escape(s: &str) -> String {
     s.replace('\'', "'\\''")
 }
 
+/// Neutralise ADO build-agent logging-command prefixes (`##vso[`, `##[`).
+/// Mirrors `crate::agent_stats::sanitize_for_markdown` so a malicious
+/// filename can't smuggle a `task.setvariable` (or similar) through the
+/// runtime `echo` line in the marker step.
+fn sanitize_for_vso_logging(s: &str) -> String {
+    s.replace("##vso[", "[vso-filtered][")
+        .replace("##[", "[filtered][")
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -207,4 +225,60 @@ mod tests {
             "JSON marker should carry raw source unchanged:\n{step}",
         );
     }
+
+    #[test]
+    fn sanitize_for_vso_logging_neutralises_known_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");
+        assert_eq!(sanitize_for_vso_logging(""), "");
+    }
+
+    #[test]
+    fn echo_line_neutralises_vso_injection_attempt() {
+        // An attacker who controls a markdown filename must not be able
+        // to inject ADO logging commands into the build log via the
+        // echoed source path. The ADO agent scans stdout for `##vso[`
+        // and `##[` prefixes and treats matching sequences as task
+        // commands (setvariable, setoutput, etc.).
+        let fm = parse_fm("name: t\ndescription: x\n");
+        let input_path = Path::new("agents/##vso[task.setvariable variable=FOO]value.md");
+        let ctx = CompileContext {
+            agent_name: &fm.name,
+            front_matter: &fm,
+            ado_context: None,
+            engine: crate::engine::Engine::Copilot,
+            compile_dir: None,
+            input_path: Some(input_path),
+        };
+        let steps = AdoAwMarkerExtension.setup_steps(&ctx).unwrap();
+        assert_eq!(steps.len(), 1);
+        let step = &steps[0];
+
+        // Find the `echo` line specifically — the `# ado-aw-metadata`
+        // JSON line is allowed to carry the raw source (it's not echoed
+        // to stdout by ADO; it's a comment in the bash heredoc, not
+        // output at runtime). The JSON line *does* get written to the
+        // build log when ADO renders the step body, but as `# ...`
+        // comments inside the rendered yaml; those don't trip the
+        // logging-command scanner.
+        let echo_line = step
+            .lines()
+            .find(|l| l.trim_start().starts_with("echo 'ado-aw metadata:"))
+            .expect("must have echo line");
+        assert!(
+            !echo_line.contains("##vso["),
+            "raw ##vso[ leaked into echo line: {echo_line}"
+        );
+        assert!(
+            echo_line.contains("[vso-filtered]["),
+            "expected `##vso[` neutralised to `[vso-filtered][` in echo line: {echo_line}"
+        );
+    }
 }

src/main.rs

@@ -70,8 +70,9 @@ enum SecretsCmd {
         all_repos: bool,
         /// Filter discovered definitions to consumers of one specific
         /// ado-aw template (e.g. `agents/security-scan.md`). Activates
-        /// the discovery code path; pairs with `--all-repos` to scope
-        /// across the project.
+        /// the discovery code path. **Without `--all-repos`, only
+        /// definitions in the current repository are searched** — pair
+        /// with `--all-repos` to search the full project.
         #[arg(long, conflicts_with = "definition_ids")]
         source: Option<String>,
     },
@@ -93,7 +94,8 @@ enum SecretsCmd {
         #[arg(long, conflicts_with = "definition_ids")]
         all_repos: bool,
         /// Filter discovered definitions to consumers of one specific
-        /// ado-aw template.
+        /// ado-aw template. **Without `--all-repos`, only definitions
+        /// in the current repository are searched.**
         #[arg(long, conflicts_with = "definition_ids")]
         source: Option<String>,
     },
@@ -116,7 +118,8 @@ enum SecretsCmd {
         #[arg(long, conflicts_with = "definition_ids")]
         all_repos: bool,
         /// Filter discovered definitions to consumers of one specific
-        /// ado-aw template.
+        /// ado-aw template. **Without `--all-repos`, only definitions
+        /// in the current repository are searched.**
         #[arg(long, conflicts_with = "definition_ids")]
         source: Option<String>,
     },