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

- `is_direct_match`: return `false` (not `markers.is_empty()`) for the
  marker-less case. The previous code returned `true` for 0 markers,
  which would misclassify a non-ado-aw definition as `Direct` if any
  future caller hit that path. Belt-and-braces — the current callsite
  in `classify_definition` already guards against it.

- `discovered_to_matched`: doc-comment claimed `UnknownRequiredParams`
  was propagated; implementation drops it. Keep the safer drop
  behaviour (we can't act on a marker-less definition) but surface a
  `warn!` summary from `resolve_definitions_via_discovery` so
  `secrets set --all-repos` operators can see when pipelines were
  skipped because of required-template-parameters / 403 / other
  preview failures. Doc comment updated to match.

- `AdoContext::repo_url`: percent-encode `project` and `repo_name` so
  `DiscoveryScope::CurrentRepo` works for projects whose names contain
  reserved characters (e.g. spaces). The lowercase normalize step
  can't reconcile a decoded local name with the encoded form ADO
  returns in `repository.url`.

- `AdoAwMarkerExtension`: bash-quote-escape the source path embedded
  in the runtime `echo` line. A markdown filename containing `'`
  (e.g. `agents/foo's.md`) would otherwise produce syntactically
  broken bash. New `bash_single_quote_escape` helper applies the
  canonical `'\''` idiom; the JSON marker line keeps the raw value
  because JSON has no quoting concern with `'`. Two new tests cover
  the idiom and a `foo's-agent.md` path.

- `src/detect.rs`: drop the now-stale `#[allow(dead_code)]` attrs on
  `MARKER_STEP_PREFIX`, `MarkerMetadata`, and `parse_marker_step`.
  All three are actively consumed by `src/ado/discovery.rs`.

- `resolve_for_command`: thread local-lock-file paths into discovery
  so the `process.yamlFilename` fast-path can skip Preview calls for
  locally-compiled pipelines. Best-effort scan — failures fall back
  to Preview-for-everything cleanly.

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

@@ -399,11 +399,21 @@ async fn classify_definition(
 /// markdown referenced by the marker has the same stem as the root
 /// YAML, the definition is the direct owner. Anything else is a
 /// consumer that pulls the template in via `template:` indirection.
+///
+/// Returns `false` for the marker-less case — `classify_definition`
+/// only routes here when at least one marker was found, but defensive
+/// against future callers.
 fn is_direct_match(def: &DefinitionSummary, markers: &[MarkerMetadata]) -> bool {
-    if markers.len() != 1 {
+    if markers.is_empty() {
+        // 0 markers means "not ado-aw at all", which is neither direct
+        // nor consumer. Belt-and-braces — `classify_definition`
+        // currently guards this, but the guard could move.
+        return false;
+    }
+    if markers.len() > 1 {
         // Multiple markers means a consumer pulling in more than one
         // template; can't be a direct ado-aw pipeline.
-        return markers.is_empty();
+        return false;
     }
     let marker = &markers[0];
     let Some(yaml_filename) = def
@@ -459,16 +469,17 @@ async fn parse_local_lock(path: &Path) -> Option<MarkerMetadata> {
 /// etc.). This keeps the rest of the codebase unchanged when commands
 /// opt into discovery via `--all-repos` / `--source`.
 ///
-/// Returns `None` for non-ado-aw definitions and for failures we don't
-/// want to silently surface as matched (forbidden, preview failed).
-/// `UnknownRequiredParams` is propagated *with* a marker-less
-/// MatchedDefinition because the user explicitly opted in and may
-/// want to act on it.
+/// Returns `None` for any classification that isn't safely actionable
+/// by a write command. In particular `UnknownRequiredParams`,
+/// `UnknownForbidden`, and `PreviewFailed` are dropped because we
+/// have no markers to attach (so we can't even tell the user which
+/// template a write would affect); `NotAdoAw` is dropped because it
+/// isn't ado-aw at all. Callers that want a richer summary (e.g. a
+/// future `list --all-repos`) should inspect `DiscoveredPipeline`
+/// directly rather than going through this adapter.
 pub fn discovered_to_matched(d: &DiscoveredPipeline) -> Option<MatchedDefinition> {
     match d.status {
         DiscoveryStatus::Direct | DiscoveryStatus::Consumer => {}
-        // Skip non-actionable classifications. The caller can inspect
-        // d.status directly for `list`-style reporting if needed.
         DiscoveryStatus::NotAdoAw
         | DiscoveryStatus::UnknownForbidden
         | DiscoveryStatus::UnknownRequiredParams
@@ -497,6 +508,12 @@ pub fn discovered_to_matched(d: &DiscoveredPipeline) -> Option<MatchedDefinition
 /// `source_filter` filters discovery results so only definitions whose
 /// 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`.
 pub async fn resolve_definitions_via_discovery(
     client: &reqwest::Client,
     ctx: &AdoContext,
@@ -507,21 +524,57 @@ pub async fn resolve_definitions_via_discovery(
 ) -> Result<Vec<MatchedDefinition>> {
     let discovered = discover_ado_aw_pipelines(client, ctx, auth, scope, local_lock_paths).await?;
 
+    let mut skipped_required_params = 0usize;
+    let mut skipped_forbidden = 0usize;
+    let mut skipped_failed = 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 Some(src) = source_filter else { return true };
             d.markers.iter().any(|m| m.source == src)
         })
         .collect();
 
+    if skipped_required_params > 0 {
+        warn!(
+            "Discovery skipped {skipped_required_params} definition(s) whose Pipeline Preview \
+             requires templateParameters with no defaults. Use --definition-ids to act on them \
+             directly.",
+        );
+    }
+    if skipped_forbidden > 0 {
+        warn!(
+            "Discovery skipped {skipped_forbidden} definition(s) the calling identity lacks \
+             read access to. Check your PAT or AAD permissions.",
+        );
+    }
+    if skipped_failed > 0 {
+        warn!(
+            "Discovery skipped {skipped_failed} definition(s) whose Pipeline Preview returned \
+             an unexpected error. Re-run with --debug to see details.",
+        );
+    }
+
     Ok(kept.iter().filter_map(discovered_to_matched).collect())
 }
 
 // AdoContext helper: derive the resolved git remote URL for
 // `CurrentRepo` scoping. Lives here (rather than on `AdoContext`)
 // because the context only stores org+project+repo_name today;
 // reconstructing the URL is a local detail of discovery.
+//
+// Percent-encodes `project` and `repo_name` to match the form ADO
+// returns in `repository.url` — without this, projects whose names
+// contain spaces or other reserved chars would silently match nothing
+// because the lowercase comparison can't reconcile e.g. `my project`
+// with `my%20project`.
 impl AdoContext {
     fn repo_url(&self) -> Option<String> {
         if self.repo_name.is_empty() {
@@ -530,8 +583,8 @@ impl AdoContext {
         Some(format!(
             "{}/{}/_git/{}",
             self.org_url.trim_end_matches('/'),
-            self.project,
-            self.repo_name
+            percent_encoding::utf8_percent_encode(&self.project, super::PATH_SEGMENT),
+            percent_encoding::utf8_percent_encode(&self.repo_name, super::PATH_SEGMENT),
         ))
     }
 }

src/compile/extensions/ado_aw_marker.rs

@@ -69,16 +69,19 @@ 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.
         //
-        // Single quotes around the echo argument keep the JSON literal
-        // intact; the metadata contains no single quotes (JSON strings
-        // escape them via \u0027 if ever needed).
+        // 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);
         let step = format!(
             "- bash: |\n    \
                 # ado-aw-metadata: {metadata}\n    \
-                echo 'ado-aw metadata: source={source} version={version} target={target}'\n  \
+                echo 'ado-aw metadata: source={echo_source} version={version} target={target}'\n  \
             displayName: \"ado-aw\"\n",
             metadata = metadata_json,
-            source = source,
+            echo_source = echo_source,
             version = version,
             target = target,
         );
@@ -87,6 +90,13 @@ impl CompilerExtension for AdoAwMarkerExtension {
     }
 }
 
+/// Escape any `'` in `s` so it can be safely embedded inside a single-quoted
+/// bash string. Replaces each `'` with `'\''` (close-quote, escaped quote,
+/// reopen-quote — the canonical idiom).
+fn bash_single_quote_escape(s: &str) -> String {
+    s.replace('\'', "'\\''")
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -158,4 +168,43 @@ mod tests {
             );
         }
     }
+
+    #[test]
+    fn bash_single_quote_escape_idiom_is_correct() {
+        // Standard bash idiom: close-quote, escaped quote, reopen.
+        assert_eq!(bash_single_quote_escape("a'b"), "a'\\''b");
+        assert_eq!(bash_single_quote_escape("''"), "'\\'''\\''");
+        assert_eq!(bash_single_quote_escape("plain"), "plain");
+        assert_eq!(bash_single_quote_escape(""), "");
+    }
+
+    #[test]
+    fn echo_line_handles_single_quote_in_source_path() {
+        // A markdown filename with `'` in it must produce syntactically
+        // valid bash. Without the escape, the generated step would
+        // break with "unexpected EOF while looking for matching `''".
+        let fm = parse_fm("name: t\ndescription: x\n");
+        let input_path = Path::new("agents/foo's-agent.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];
+        assert!(
+            step.contains("echo 'ado-aw metadata: source=agents/foo'\\''s-agent.md "),
+            "single-quote in source should be escaped via the '\\'' idiom; got:\n{step}",
+        );
+        // The JSON marker line should still carry the raw (un-bash-escaped)
+        // source — JSON has no quoting concern with `'`.
+        assert!(
+            step.contains("\"source\":\"agents/foo's-agent.md\""),
+            "JSON marker should carry raw source unchanged:\n{step}",
+        );
+    }
 }

src/detect.rs

@@ -133,15 +133,13 @@ async fn try_detect_pipeline(
 /// (unlike top-of-file YAML comments) survives ADO Pipeline Preview
 /// expansion, so this prefix is the canonical surface project-scope
 /// discovery searches for in expanded YAML.
-#[allow(dead_code)] // Consumed by upcoming Preview-driven discovery (workstream P).
 pub const MARKER_STEP_PREFIX: &str = "# ado-aw-metadata:";
 
 /// Parsed metadata from a `# ado-aw-metadata: {…}` marker step line.
 ///
 /// The schema is forward-compatible: unknown JSON fields are ignored,
 /// and missing fields fall through to defaults (empty string / zero).
 /// Callers that need a specific field should check it explicitly.
-#[allow(dead_code)] // Consumed by upcoming Preview-driven discovery (workstream P).
 #[derive(Debug, Clone, Deserialize, PartialEq, Eq)]
 pub struct MarkerMetadata {
     /// Schema version; `1` for the initial release.
@@ -172,7 +170,6 @@ pub struct MarkerMetadata {
 /// - Raw compiled-on-disk lock-file YAML, or
 /// - The `finalYaml` returned by ADO's Pipeline Preview API (which
 ///   strips top-of-file comments but preserves step bodies).
-#[allow(dead_code)] // Consumed by upcoming Preview-driven discovery (workstream P).
 pub fn parse_marker_step(yaml: &str) -> Vec<MarkerMetadata> {
     let mut results = Vec::new();
 

src/secrets.rs

@@ -205,9 +205,37 @@ async fn resolve_for_command(
         } else {
             DiscoveryScope::CurrentRepo
         };
-        let matched =
-            resolve_definitions_via_discovery(client, ado_ctx, auth, scope, None, source_filter)
-                .await?;
+
+        // Feed local lock files into discovery so its yamlFilename
+        // fast-path can skip a Preview call per locally-compiled
+        // pipeline. Best-effort: scan failures aren't fatal — discovery
+        // simply falls back to Preview for everything.
+        let local_lock_paths: Vec<PathBuf> = match crate::detect::detect_pipelines(repo_path).await
+        {
+            Ok(detected) => detected.into_iter().map(|p| p.yaml_path).collect(),
+            Err(e) => {
+                log::debug!(
+                    "Local-fixture scan failed during discovery ({e}); falling back to Preview \
+                     for every definition."
+                );
+                Vec::new()
+            }
+        };
+        let local_lock_slice = if local_lock_paths.is_empty() {
+            None
+        } else {
+            Some(local_lock_paths.as_slice())
+        };
+
+        let matched = resolve_definitions_via_discovery(
+            client,
+            ado_ctx,
+            auth,
+            scope,
+            local_lock_slice,
+            source_filter,
+        )
+        .await?;
         return Ok(Some(matched));
     }