fix(secrets): three correctness fixes on Preview-driven discovery

1. `src/ado/discovery.rs:732` — skip-warning text overclaimed
   ado-aw membership.

   In `--all-repos` mode the per-status counters
   (`skipped_required_params` / `_forbidden` / `_failed`) fired
   for every failed Preview, including pipelines that aren't ado-aw
   at all. A project with hundreds of non-ado-aw pipelines that
   legitimately require `templateParameters` would emit
   "Discovery skipped N definitions whose Pipeline Preview requires
   templateParameters" when none of them were ado-aw consumers in
   the first place. We literally cannot tell which is which without
   successful Preview output.

   Consolidated the three separate `warn!` messages into one that's
   honest about uncertainty ("any ado-aw pipelines among them have
   been silently skipped"), with the per-status breakdown demoted to
   `debug!` for operators investigating. Counter variables renamed
   (`uninspectable_*`) to reflect what they actually count.

2. `src/compile/extensions/mod.rs:130` — `Path::parent()` returns
   `Some(Path::new(""))` for bare filenames, not `None`, so the
   `unwrap_or(Path::new("."))` fallback never fired for inputs like
   `foo.md`. Empty paths passed to `git -C "" remote get-url`
   behave differently from `git -C "."`. Match-arm now normalises
   both the `None` and empty-`Some` cases to `.`.

3. `src/secrets.rs:202` + `src/ado/discovery.rs` —
   `--all-repos --source` could silently exclude every strict
   marker when `ctx.org_name()` returned `None` or
   `ctx.repo_name` was empty. The existing guard only fired in the
   `!all_repos` branch. Extended the guard to fire for any
   `--source` invocation when the current `(org, repo)` is
   unresolvable, with an actionable error pointing the user to
   `--definition-ids` as the escape hatch. New test:
   `source_with_all_repos_bails_when_org_url_unresolvable`.

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

Author: jamesadevine

src/ado/discovery.rs

@@ -702,10 +702,19 @@ pub async fn resolve_definitions_via_discovery(
     // counters while deciding inclusion — explicit two-pass form keeps
     // the counts honestly derived from the same iteration and makes it
     // obvious what ends up in the returned vec.
-    let mut skipped_required_params = 0usize;
-    let mut skipped_forbidden = 0usize;
-    let mut skipped_failed = 0usize;
-    let mut uninspectable = 0usize;
+    //
+    // The per-status counters (`uninspectable_required_params` /
+    // `_forbidden` / `_failed`) tally Preview failures by reason. They
+    // intentionally do NOT distinguish "ado-aw consumer" from
+    // "unrelated pipeline" — Preview failed for these, so we have no
+    // markers to tell which is which. A non-ado-aw project may have
+    // hundreds of definitions that legitimately require
+    // templateParameters; we can't claim any of them were ado-aw
+    // consumers without inspecting them, so the warning text below is
+    // written to be honest about that uncertainty.
+    let mut uninspectable_required_params = 0usize;
+    let mut uninspectable_forbidden = 0usize;
+    let mut uninspectable_failed = 0usize;
     let mut selected: Vec<DiscoveredPipeline> = Vec::with_capacity(discovered.len());
 
     for d in discovered {
@@ -716,22 +725,10 @@ pub async fn resolve_definitions_via_discovery(
             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.
-        match (&d.status, normalized_filter.is_some()) {
-            (DiscoveryStatus::UnknownRequiredParams, false) => skipped_required_params += 1,
-            (DiscoveryStatus::UnknownForbidden, false) => skipped_forbidden += 1,
-            (DiscoveryStatus::PreviewFailed(_), false) => skipped_failed += 1,
-            (
-                DiscoveryStatus::UnknownRequiredParams
-                | DiscoveryStatus::UnknownForbidden
-                | DiscoveryStatus::PreviewFailed(_),
-                true,
-            ) => uninspectable += 1,
+        match d.status {
+            DiscoveryStatus::UnknownRequiredParams => uninspectable_required_params += 1,
+            DiscoveryStatus::UnknownForbidden => uninspectable_forbidden += 1,
+            DiscoveryStatus::PreviewFailed(_) => uninspectable_failed += 1,
             _ => {}
         }
 
@@ -740,37 +737,38 @@ pub async fn resolve_definitions_via_discovery(
         }
     }
 
-    // Pass 2: emit warnings and convert the selected items into
-    // `MatchedDefinition`. `discovered_to_matched` further filters out
-    // non-actionable statuses (NotAdoAw / NotFound / UnknownForbidden /
-    // UnknownRequiredParams / PreviewFailed); the count warnings above
-    // are what tells the operator why those drops happened.
-    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.",
-        );
-    }
-    if uninspectable > 0
-        && let Some(src) = normalized_filter.as_deref()
-    {
-        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.",
+    let uninspectable =
+        uninspectable_required_params + uninspectable_forbidden + uninspectable_failed;
+
+    // Pass 2: emit a single warning that's honest about uncertainty,
+    // and surface the per-status breakdown at debug level for
+    // operators who want to know whether the misses were
+    // permission-related or template-parameter-related.
+    //
+    // Previously we emitted three separate warn-level messages keyed
+    // on the per-status counts (e.g. "Discovery skipped N definitions
+    // whose Pipeline Preview requires templateParameters") — but in
+    // `--all-repos` mode that's misleading: a project with hundreds of
+    // non-ado-aw pipelines that legitimately require parameters would
+    // make the operator think they'd missed N ado-aw consumers, when
+    // none of them were ado-aw in the first place. We can't tell
+    // which is which without successful Preview output.
+    if uninspectable > 0 {
+        match normalized_filter.as_deref() {
+            Some(src) => 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.",
+            ),
+            None => warn!(
+                "Discovery could not inspect {uninspectable} definition(s) (Preview failure, \
+                 forbidden, or required-parameters); any ado-aw pipelines among them have been \
+                 silently skipped. Re-run with --debug for per-definition reasons.",
+            ),
+        }
+        debug!(
+            "Uninspectable breakdown: {uninspectable_required_params} required-parameters, \
+             {uninspectable_forbidden} forbidden, {uninspectable_failed} other Preview errors.",
         );
     }
 

src/compile/extensions/mod.rs

@@ -123,7 +123,17 @@ impl<'a> CompileContext<'a> {
     /// context from the git remote in the directory containing `input_path`.
     /// Returns an error if the engine identifier is unsupported.
     pub async fn new(front_matter: &'a FrontMatter, input_path: &'a Path) -> Result<Self> {
-        let compile_dir = input_path.parent().unwrap_or(Path::new("."));
+        // `Path::parent()` is subtle: for a bare filename like `foo.md`
+        // it returns `Some(Path::new(""))` rather than `None`, so the
+        // `unwrap_or(Path::new("."))` fallback wouldn't catch it. An
+        // empty path passed to `git -C ""` behaves differently from
+        // `git -C "."` (some platforms reject it, others quietly use
+        // the parent process's cwd), so we normalise both the
+        // `None` and empty-`Some` cases to `.`.
+        let compile_dir = match input_path.parent() {
+            Some(p) if !p.as_os_str().is_empty() => p,
+            _ => Path::new("."),
+        };
         let engine = engine::get_engine(front_matter.engine.engine_id())?;
         let ado_context = Self::infer_ado_context(compile_dir).await;
         Ok(Self {

src/secrets.rs

@@ -214,6 +214,29 @@ async fn resolve_for_command(
             );
         }
 
+        // `--source` requires an identifiable current (org, repo) so
+        // the marker-origin filter in discovery can disambiguate
+        // same-named source paths across repos. Without it, a strict
+        // marker (one carrying `org` / `repo`) would silently fail the
+        // origin check and the operator would see "no pipelines
+        // matched" with no explanation. The `!all_repos` guard above
+        // covers the missing-`repo_name` case for current-repo scope;
+        // here we also catch `--all-repos --source` paired with a
+        // missing or malformed `org_url` (e.g. `org_name()` resolves
+        // to `None`).
+        if source_filter.is_some()
+            && (ado_ctx.org_name().is_none() || ado_ctx.repo_name.is_empty())
+        {
+            anyhow::bail!(
+                "--source needs the current repository's Azure DevOps org and repo to \
+                 disambiguate same-named source paths across the project, but neither \
+                 could be resolved from `{}`.\n\
+                 Run from a checkout of an ADO repo, or use --definition-ids to act on \
+                 specific pipelines directly.",
+                repo_path.display()
+            );
+        }
+
         let scope = if all_repos {
             DiscoveryScope::AllRepos
         } else {
@@ -746,6 +769,48 @@ mod tests {
         );
     }
 
+    #[tokio::test]
+    async fn source_with_all_repos_bails_when_org_url_unresolvable() {
+        // `--all-repos --source` still needs the current (org, repo)
+        // to disambiguate same-named source paths via the marker's
+        // origin fields. If `org_url` doesn't yield an org slug (or
+        // `repo_name` is empty), the marker-origin filter would
+        // silently exclude every strict marker — surface a targeted
+        // error instead.
+        //
+        // Empty `org_url` is the realistic failure mode: a hand-built
+        // AdoContext from `--org "" --project p` or a corrupted ADO
+        // remote that parsed past `parse_ado_remote` would land here.
+        let ctx = AdoContext {
+            org_url: String::new(), // org_name() resolves to None
+            project: "p".to_string(),
+            repo_name: "some-repo".to_string(),
+        };
+        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,
+            true, // --all-repos
+            Some("agents/foo.md"),
+            tmp.path(),
+        )
+        .await
+        .expect_err("expected bail");
+
+        let msg = format!("{err}");
+        assert!(
+            msg.contains("--source needs the current repository's"),
+            "error should explain the root cause; got: {msg}"
+        );
+    }
+
     #[test]
     fn set_preserves_other_variables() {
         let def = serde_json::json!({