fix(compile): normalize legacy Azure Repos remotes (#1495)

* Initial plan

* fix: normalize legacy Azure Repos remotes

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

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>

Author: Copilot

src/ado/mod.rs

@@ -106,6 +106,8 @@ impl AdoContext {
 /// Supports:
 /// - HTTPS: `https://dev.azure.com/{org}/{project}/_git/{repo}`
 /// - HTTPS (legacy): `https://{org}.visualstudio.com/{project}/_git/{repo}`
+/// - HTTPS (legacy collection):
+///   `https://{org}.visualstudio.com/DefaultCollection/{project}/_git/{repo}`
 /// - SSH: `git@ssh.dev.azure.com:v3/{org}/{project}/{repo}`
 /// - SSH (legacy): `git@vs-ssh.visualstudio.com:v3/{org}/{project}/{repo}`
 pub fn parse_ado_remote(remote_url: &str) -> Result<AdoContext> {
@@ -158,11 +160,21 @@ pub fn parse_ado_remote(remote_url: &str) -> Result<AdoContext> {
             .unwrap_or_default();
 
         // Expected: /{project}/_git/{repo}
-        if segments.len() >= 3 && segments[1] == "_git" {
-            let repo_name = segments[2].trim_end_matches(".git");
+        // Also accepts the legacy collection-prefixed shape:
+        // /DefaultCollection/{project}/_git/{repo}
+        let repo_start = if segments
+            .first()
+            .is_some_and(|segment| segment.eq_ignore_ascii_case("DefaultCollection"))
+        {
+            1
+        } else {
+            0
+        };
+        if segments.len() >= repo_start + 3 && segments[repo_start + 1] == "_git" {
+            let repo_name = segments[repo_start + 2].trim_end_matches(".git");
             return Ok(AdoContext {
                 org_url: format!("https://dev.azure.com/{}", org),
-                project: segments[0].to_string(),
+                project: segments[repo_start].to_string(),
                 repo_name: repo_name.to_string(),
             });
         }
@@ -2107,6 +2119,15 @@ mod tests {
         assert_eq!(ctx.repo_name, "myrepo");
     }
 
+    #[test]
+    fn test_parse_ado_remote_legacy_visualstudio_default_collection() {
+        let url = "https://myorg.visualstudio.com/DefaultCollection/MyProject/_git/myrepo.git";
+        let ctx = parse_ado_remote(url).unwrap();
+        assert_eq!(ctx.org_url, "https://dev.azure.com/myorg");
+        assert_eq!(ctx.project, "MyProject");
+        assert_eq!(ctx.repo_name, "myrepo");
+    }
+
     #[test]
     fn test_parse_ado_remote_legacy_ssh() {
         let url = "git@vs-ssh.visualstudio.com:v3/myorg/myproject/myrepo";
@@ -2152,6 +2173,18 @@ mod tests {
         assert_eq!(source.project.as_deref(), Some("MyProject"));
     }
 
+    #[test]
+    fn parse_git_remote_ado_legacy_visualstudio_default_collection() {
+        let source = parse_git_remote(
+            "https://myorg.visualstudio.com/DefaultCollection/MyProject/_git/myrepo.git",
+        )
+        .unwrap();
+        assert_eq!(source.provider, RepoProvider::AdoGit);
+        assert_eq!(source.owner, "myorg");
+        assert_eq!(source.repo, "myrepo");
+        assert_eq!(source.project.as_deref(), Some("MyProject"));
+    }
+
     #[test]
     fn parse_git_remote_github_https() {
         let source = parse_git_remote("https://github.com/githubnext/ado-aw").unwrap();

src/compile/extensions/exec_context/ci_push.rs

@@ -7,8 +7,8 @@
 //! the contributor does ADO REST + git fetch deepening work that
 //! adds startup latency, so most agents shouldn't pay for it.
 //!
-//! Runtime gate: `or(eq(Build.Reason, 'IndividualCI'),
-//! eq(Build.Reason, 'BatchedCI'))`. Skips PRs, scheduled runs, and
+//! Runtime gate: `and(succeeded(), or(eq(Build.Reason, 'IndividualCI'),
+//! eq(Build.Reason, 'BatchedCI')))`. Skips PRs, scheduled runs, and
 //! resource triggers at zero cost.
 //!
 //! ## Artefacts (staged by the bundle on success)
@@ -52,7 +52,7 @@ use crate::compile::ir::condition::{Condition, Expr};
 use crate::compile::ir::step::{BashStep, Step};
 use crate::compile::types::CiPushContextConfig;
 
-use super::contributor::ContextContributor;
+use super::contributor::{ContextContributor, succeeded_and};
 
 /// CI-push-context contributor.
 pub(super) struct CiPushContextContributor {
@@ -99,7 +99,7 @@ impl ContextContributor for CiPushContextContributor {
                 "Stage ci-push execution context (aw-context/ci-push/*)",
                 script,
             )
-            .with_condition(Condition::Or(vec![
+            .with_condition(succeeded_and(Condition::Or(vec![
                 Condition::Eq(
                     Expr::Variable("Build.Reason".to_string()),
                     Expr::Literal("IndividualCI".to_string()),
@@ -108,7 +108,7 @@ impl ContextContributor for CiPushContextContributor {
                     Expr::Variable("Build.Reason".to_string()),
                     Expr::Literal("BatchedCI".to_string()),
                 ),
-            ])),
+            ]))),
             Bundle::ExecContextCiPush,
             TokenSource::SystemAccessToken,
         );
@@ -177,10 +177,14 @@ mod tests {
             Step::Bash(b) => b,
             other => panic!("expected Bash, got {other:?}"),
         };
-        // Condition: or(eq(Build.Reason, IndividualCI),
-        //              eq(Build.Reason, BatchedCI))
+        // Condition: and(succeeded(), or(eq(Build.Reason, IndividualCI),
+        //                                eq(Build.Reason, BatchedCI)))
         match &bash.condition {
-            Some(Condition::Or(clauses)) => {
+            Some(Condition::And(parts)) => {
+                assert!(matches!(parts.as_slice(), [Condition::Succeeded, _]));
+                let Condition::Or(clauses) = &parts[1] else {
+                    panic!("expected Or trigger condition, got {:?}", parts[1]);
+                };
                 assert_eq!(clauses.len(), 2);
                 let reasons: Vec<&str> = clauses
                     .iter()
@@ -192,7 +196,7 @@ mod tests {
                 assert!(reasons.contains(&"IndividualCI"));
                 assert!(reasons.contains(&"BatchedCI"));
             }
-            other => panic!("expected Or condition, got {other:?}"),
+            other => panic!("expected succeeded And condition, got {other:?}"),
         }
 
         // Trust boundary: bearer present, projected as a secret via the

src/compile/extensions/exec_context/contributor.rs

@@ -20,6 +20,12 @@
 //! and lets us evolve the contract freely.
 
 use crate::compile::extensions::CompileContext;
+use crate::compile::ir::condition::Condition;
+
+/// Preserve ADO's implicit step success gate when adding a runtime trigger predicate.
+pub(super) fn succeeded_and(condition: Condition) -> Condition {
+    Condition::And(vec![Condition::Succeeded, condition])
+}
 
 /// A unit of per-trigger execution-context generation.
 ///

src/compile/extensions/exec_context/manual.rs

@@ -3,7 +3,7 @@
 //!
 //! Activates whenever the agent declares any `parameters:` block (and
 //! the `execution-context.manual.enabled` switch is not `false`).
-//! Runtime gate: `eq(variables['Build.Reason'], 'Manual')`.
+//! Runtime gate: `and(succeeded(), eq(variables['Build.Reason'], 'Manual'))`.
 //!
 //! ## Artefacts (staged by the bundle on success)
 //!
@@ -56,7 +56,7 @@ use crate::compile::types::ManualContextConfig;
 #[cfg(test)]
 use crate::compile::types::FrontMatter;
 
-use super::contributor::ContextContributor;
+use super::contributor::{ContextContributor, succeeded_and};
 
 /// Manual-context contributor.
 pub(super) struct ManualContextContributor {
@@ -152,10 +152,10 @@ impl ContextContributor for ManualContextContributor {
             "Stage manual execution context (aw-context/manual/*)",
             script,
         )
-        .with_condition(Condition::Eq(
+        .with_condition(succeeded_and(Condition::Eq(
             Expr::Variable("Build.Reason".to_string()),
             Expr::Literal("Manual".to_string()),
-        ))
+        )))
         .with_env(
             "BUILD_REQUESTEDFOR",
             EnvValue::ado_macro("Build.RequestedFor")?,
@@ -297,13 +297,17 @@ mod tests {
             other => panic!("expected Step::Bash, got {other:?}"),
         };
 
-        // Condition: gated on Build.Reason == 'Manual'.
+        // Condition: preserve the default success gate and then gate on Manual.
         match &bash.condition {
-            Some(Condition::Eq(Expr::Variable(v), Expr::Literal(l))) => {
+            Some(Condition::And(parts)) => {
+                assert!(matches!(parts.as_slice(), [Condition::Succeeded, _]));
+                let Condition::Eq(Expr::Variable(v), Expr::Literal(l)) = &parts[1] else {
+                    panic!("expected eq(Build.Reason, 'Manual'), got {:?}", parts[1]);
+                };
                 assert_eq!(v, "Build.Reason");
                 assert_eq!(l, "Manual");
             }
-            other => panic!("expected eq(Build.Reason, 'Manual'), got {other:?}"),
+            other => panic!("expected succeeded And condition, got {other:?}"),
         }
 
         // Requestor identity env vars present.

src/compile/extensions/exec_context/pipeline.rs

@@ -4,7 +4,7 @@
 //! Activates whenever the agent declares an `on.pipeline` resource
 //! trigger (and the `execution-context.pipeline.enabled` switch is
 //! not `false`). Runtime gate:
-//! `eq(variables['Build.Reason'], 'ResourceTrigger')`.
+//! `and(succeeded(), eq(variables['Build.Reason'], 'ResourceTrigger'))`.
 //!
 //! ## Artefacts (staged by the bundle on success)
 //!
@@ -32,7 +32,7 @@
 //!   bearer for the Build REST API. The token is never written to
 //!   disk, never logged, never passed in argv.
 //! - The step is gated by
-//!   `condition: eq(variables['Build.Reason'], 'ResourceTrigger')`
+//!   `condition: and(succeeded(), eq(variables['Build.Reason'], 'ResourceTrigger'))`
 //!   so it never runs on non-pipeline-completion builds.
 //! - All staged artefacts are short, structured ADO REST output —
 //!   no user-controlled HTML, no free-text fields (the upstream
@@ -46,7 +46,7 @@ use crate::compile::ir::condition::{Condition, Expr};
 use crate::compile::ir::step::{BashStep, Step};
 use crate::compile::types::PipelineContextConfig;
 
-use super::contributor::ContextContributor;
+use super::contributor::{ContextContributor, succeeded_and};
 
 /// Pipeline-context contributor.
 pub(super) struct PipelineContextContributor {
@@ -98,10 +98,10 @@ impl ContextContributor for PipelineContextContributor {
                 "Stage pipeline execution context (aw-context/pipeline/*)",
                 script,
             )
-            .with_condition(Condition::Eq(
+            .with_condition(succeeded_and(Condition::Eq(
                 Expr::Variable("Build.Reason".to_string()),
                 Expr::Literal("ResourceTrigger".to_string()),
-            )),
+            ))),
             Bundle::ExecContextPipeline,
             TokenSource::SystemAccessToken,
         );
@@ -181,13 +181,20 @@ mod tests {
             other => panic!("expected Step::Bash, got {other:?}"),
         };
 
-        // Runtime gate.
+        // Runtime gate preserves the default success gate.
         match &bash.condition {
-            Some(Condition::Eq(Expr::Variable(v), Expr::Literal(l))) => {
+            Some(Condition::And(parts)) => {
+                assert!(matches!(parts.as_slice(), [Condition::Succeeded, _]));
+                let Condition::Eq(Expr::Variable(v), Expr::Literal(l)) = &parts[1] else {
+                    panic!(
+                        "expected eq(Build.Reason, 'ResourceTrigger'), got {:?}",
+                        parts[1]
+                    );
+                };
                 assert_eq!(v, "Build.Reason");
                 assert_eq!(l, "ResourceTrigger");
             }
-            other => panic!("expected eq(Build.Reason, 'ResourceTrigger'), got {other:?}"),
+            other => panic!("expected succeeded And condition, got {other:?}"),
         }
 
         // Bearer present.

src/compile/extensions/exec_context/pr.rs

@@ -42,8 +42,9 @@
 //!   regex validation, etc.) on top of the same Node-step exposure.
 //! - The token is never written to `.git/config`; `persistCredentials`
 //!   is never `true`; no checkout override is emitted.
-//! - The step is gated by `condition: eq(variables['Build.Reason'],
-//!   'PullRequest')` so it never runs on non-PR builds.
+//! - The step is gated by `condition: and(succeeded(),
+//!   eq(variables['Build.Reason'], 'PullRequest'))` so it never runs
+//!   after an earlier failure or on non-PR builds.
 //!
 //! ## Wiring
 //!
@@ -66,7 +67,7 @@ use crate::compile::ir::env::EnvValue;
 use crate::compile::ir::step::{BashStep, Step};
 use crate::compile::types::PrContextConfig;
 
-use super::contributor::ContextContributor;
+use super::contributor::{ContextContributor, succeeded_and};
 
 /// PR-context contributor. Activates when `on.pr` is configured
 /// (unless explicitly disabled via `execution-context.pr.enabled: false`).
@@ -138,10 +139,10 @@ impl ContextContributor for PrContextContributor {
         } else {
             (
                 "",
-                Condition::Eq(
+                succeeded_and(Condition::Eq(
                     Expr::Variable("Build.Reason".to_string()),
                     Expr::Literal("PullRequest".to_string()),
-                ),
+                )),
             )
         };
         let script = format!("set -euo pipefail\n{prelude}node '{EXEC_CONTEXT_PR_PATH}'\n");
@@ -286,7 +287,7 @@ mod tests {
 
     /// Synth-inactive: the auto-injected `SYSTEM_PULLREQUEST_*` and
     /// `SYSTEM_TEAMPROJECT` / `BUILD_*` context vars are NOT re-projected;
-    /// condition is the typed `Eq(Variable("Build.Reason"), Literal("PullRequest"))`.
+    /// condition preserves the default success gate and then gates on PullRequest.
     #[test]
     fn prepare_step_typed_synth_inactive_uses_plain_macros_and_narrow_condition() {
         let contributor = PrContextContributor::new(PrContextConfig::default(), false);
@@ -323,11 +324,18 @@ mod tests {
         assert!(!bash.env.contains_key("AW_SYNTHETIC_PR"));
 
         match bash.condition.as_ref().expect("condition required") {
-            Condition::Eq(Expr::Variable(name), Expr::Literal(lit)) => {
+            Condition::And(parts) => {
+                assert!(matches!(parts.as_slice(), [Condition::Succeeded, _]));
+                let Condition::Eq(Expr::Variable(name), Expr::Literal(lit)) = &parts[1] else {
+                    panic!(
+                        "expected Condition::Eq(Variable, Literal), got {:?}",
+                        parts[1]
+                    );
+                };
                 assert_eq!(name, "Build.Reason");
                 assert_eq!(lit, "PullRequest");
             }
-            other => panic!("expected Condition::Eq(Variable, Literal), got {other:?}"),
+            other => panic!("expected succeeded And condition, got {other:?}"),
         }
     }
 }

src/compile/extensions/exec_context/pr_checks.rs

@@ -27,7 +27,7 @@ use crate::compile::ir::env::EnvValue;
 use crate::compile::ir::step::{BashStep, Step};
 use crate::compile::types::PrChecksContextConfig;
 
-use super::contributor::ContextContributor;
+use super::contributor::{ContextContributor, succeeded_and};
 
 pub(super) struct PrChecksContextContributor {
     config: PrChecksContextConfig,
@@ -88,10 +88,10 @@ impl ContextContributor for PrChecksContextContributor {
             )
         } else {
             (
-                Condition::Eq(
+                succeeded_and(Condition::Eq(
                     Expr::Variable("Build.Reason".to_string()),
                     Expr::Literal("PullRequest".to_string()),
-                ),
+                )),
                 "",
             )
         };
@@ -199,8 +199,8 @@ mod tests {
 
     #[test]
     fn prepare_step_carries_bearer_condition_and_pr_id() {
-        // Non-synth path: condition must be eq(Build.Reason, 'PullRequest'),
-        // PR ID env must be the plain ADO macro (not a pipeline var).
+        // Non-synth path: condition must preserve succeeded() and gate on
+        // PullRequest; PR ID env must not be re-projected.
         let c = PrChecksContextContributor::new(
             PrChecksContextConfig {
                 enabled: Some(true),
@@ -220,15 +220,20 @@ mod tests {
             bash.env.get("SYSTEM_ACCESSTOKEN"),
             Some(EnvValue::Secret(v)) if v == "System.AccessToken"
         ));
-        // Runtime gate: step must only fire on PR builds.
+        // Runtime gate: step must only fire on PR builds after prior success.
         match bash.condition.as_ref().expect("condition required") {
-            Condition::Eq(Expr::Variable(v), Expr::Literal(l)) => {
+            Condition::And(parts) => {
+                assert!(matches!(parts.as_slice(), [Condition::Succeeded, _]));
+                let Condition::Eq(Expr::Variable(v), Expr::Literal(l)) = &parts[1] else {
+                    panic!(
+                        "expected Condition::Eq(Variable(Build.Reason), Literal(PullRequest)), got {:?}",
+                        parts[1]
+                    );
+                };
                 assert_eq!(v, "Build.Reason");
                 assert_eq!(l, "PullRequest");
             }
-            other => panic!(
-                "expected Condition::Eq(Variable(Build.Reason), Literal(PullRequest)), got {other:?}"
-            ),
+            other => panic!("expected succeeded And condition, got {other:?}"),
         }
         // Non-synth path: the bundle reads the auto-injected
         // SYSTEM_PULLREQUEST_PULLREQUESTID (and other context vars) directly,