fix(compile): address pool review feedback (#541)

Author: jamesadevine

src/compile/codemods/0002_pool_object_form.rs

@@ -45,13 +45,20 @@ fn apply_codemod(fm: &mut Mapping, ctx: &CodemodContext) -> Result<bool> {
     let key = Value::String("pool".to_string());
 
     let Some(pool_value) = fm.get(&key).cloned() else {
-        // Pool absent — only inject the legacy default when the
-        // compiler version is at or above the release that changed
-        // the implicit default. Older binaries still carry the old
-        // default in `resolve_pool_block`, so no rewrite is needed.
+        // Pool absent — only inject the legacy default for 1ES
+        // targets where the old implicit default was the self-hosted
+        // pool. Non-1ES (standalone/job/stage) targets now default to
+        // `vmImage: ubuntu-latest`, which is the desired behaviour
+        // for new pipelines that omit `pool:`.
         if !version_gte(ctx.compiler_version, INTRODUCED_IN) {
             return Ok(false);
         }
+        let target = fm
+            .get(&Value::String("target".to_string()))
+            .and_then(|v| v.as_str());
+        if target != Some("1es") {
+            return Ok(false);
+        }
         let mut mapped = Mapping::new();
         mapped.insert(
             Value::String("name".to_string()),
@@ -108,8 +115,9 @@ mod tests {
     }
 
     #[test]
-    fn inserts_legacy_default_when_pool_absent_and_version_gte() {
-        let mut fm: Mapping = serde_yaml::from_str("name: x\ndescription: y").unwrap();
+    fn inserts_legacy_default_when_pool_absent_1es_and_version_gte() {
+        let mut fm: Mapping =
+            serde_yaml::from_str("name: x\ndescription: y\ntarget: 1es").unwrap();
         let changed = apply_codemod(&mut fm, &ctx("0.30.0")).expect("apply");
         assert!(changed);
         assert_eq!(
@@ -119,16 +127,37 @@ mod tests {
     }
 
     #[test]
-    fn noops_when_pool_absent_and_version_below() {
+    fn noops_when_pool_absent_standalone_and_version_gte() {
+        // Standalone pipelines without pool should get the new
+        // vmImage: ubuntu-latest default, not the legacy 1ES pool.
         let mut fm: Mapping = serde_yaml::from_str("name: x\ndescription: y").unwrap();
+        let changed = apply_codemod(&mut fm, &ctx("0.30.0")).expect("apply");
+        assert!(!changed);
+        assert!(!fm.contains_key(Value::String("pool".into())));
+    }
+
+    #[test]
+    fn noops_when_pool_absent_explicit_standalone_and_version_gte() {
+        let mut fm: Mapping =
+            serde_yaml::from_str("name: x\ndescription: y\ntarget: standalone").unwrap();
+        let changed = apply_codemod(&mut fm, &ctx("0.30.0")).expect("apply");
+        assert!(!changed);
+        assert!(!fm.contains_key(Value::String("pool".into())));
+    }
+
+    #[test]
+    fn noops_when_pool_absent_1es_and_version_below() {
+        let mut fm: Mapping =
+            serde_yaml::from_str("name: x\ndescription: y\ntarget: 1es").unwrap();
         let changed = apply_codemod(&mut fm, &ctx("0.29.0")).expect("apply");
         assert!(!changed);
         assert!(!fm.contains_key(Value::String("pool".into())));
     }
 
     #[test]
     fn idempotent_after_inserting_legacy_default() {
-        let mut fm: Mapping = serde_yaml::from_str("name: x\ndescription: y").unwrap();
+        let mut fm: Mapping =
+            serde_yaml::from_str("name: x\ndescription: y\ntarget: 1es").unwrap();
         let changed1 = apply_codemod(&mut fm, &ctx("0.30.0")).expect("first apply");
         assert!(changed1);
         let changed2 = apply_codemod(&mut fm, &ctx("0.30.0")).expect("second apply");

src/compile/common.rs

@@ -1055,6 +1055,8 @@ fn resolve_pool_block(target: CompileTarget, pool: Option<&PoolConfig>) -> Resul
                     ),
                     (Some(name), None) => Ok(format!("name: {}", name)),
                     (None, Some(vm_image)) => Ok(format!("vmImage: {}", vm_image)),
+                    // `pool: {}` (empty object) — fall back to the
+                    // Microsoft-hosted default, same as omitting pool.
                     (None, None) => Ok(format!("vmImage: {}", DEFAULT_VM_IMAGE_POOL)),
                 },
             }
@@ -1815,15 +1817,14 @@ pub fn generate_setup_job(
     let user_steps = steps_parts.join("\n\n");
 
     // Build the job YAML with markers for proper indentation
-    let mut template = format!(
-        r#"- job: Setup
+    let mut template = r#"- job: Setup
   displayName: "Setup"
   pool:
-    {pool}
+    {{ pool }}
   steps:
     - checkout: self
 "#
-    );
+    .to_string();
 
     if !ext_steps_combined.is_empty() {
         template.push_str("    {{ ext_setup_steps }}\n");
@@ -1832,7 +1833,8 @@ pub fn generate_setup_job(
         template.push_str("    {{ user_setup_steps }}\n");
     }
 
-    let yaml = replace_with_indent(&template, "{{ ext_setup_steps }}", &ext_steps_combined);
+    let yaml = replace_with_indent(&template, "{{ pool }}", pool);
+    let yaml = replace_with_indent(&yaml, "{{ ext_setup_steps }}", &ext_steps_combined);
     let yaml = replace_with_indent(&yaml, "{{ user_setup_steps }}", &user_steps);
 
     Ok(yaml)
@@ -1849,18 +1851,20 @@ pub fn generate_teardown_job(
 
     let steps_yaml = format_steps_yaml_indented(teardown_steps, 4);
 
-    format!(
+    let template = format!(
         r#"- job: Teardown
   displayName: "Teardown"
   dependsOn: Execution
   pool:
-    {}
+    {{{{ pool }}}}
   steps:
     - checkout: self
 {}
 "#,
-        pool, steps_yaml
-    )
+        steps_yaml
+    );
+
+    replace_with_indent(&template, "{{ pool }}", pool)
 }
 
 /// Generate prepare steps (inline), including extension steps and user-defined steps.
@@ -2944,7 +2948,7 @@ pub async fn compile_template_target(
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::compile::types::{McpConfig, McpOptions, Repository};
+    use crate::compile::types::{McpConfig, McpOptions, PoolConfigFull, Repository};
     use crate::compile::extensions::{CompileContext, collect_extensions};
     use std::collections::HashMap;
 
@@ -6218,6 +6222,33 @@ mod tests {
         assert!(out.contains("echo td"), "out: {out}");
     }
 
+    #[test]
+    fn test_generate_setup_job_multiline_pool_indentation() {
+        // 1ES pool resolves to a multi-line string; verify all lines
+        // are properly indented under `pool:`.
+        let fm: FrontMatter = serde_yaml::from_str("name: t\ndescription: t").unwrap();
+        let ctx = CompileContext::for_test(&fm);
+        let step: serde_yaml::Value = serde_yaml::from_str("bash: echo setup").unwrap();
+        let pool = "name: AZS-1ES-L-MMS-ubuntu-22.04\nos: linux";
+        let out = generate_setup_job(&[step], pool, None, None, &[], &ctx).unwrap();
+        // Both pool lines must be indented at the same level (4 spaces)
+        assert!(
+            out.contains("    name: AZS-1ES-L-MMS-ubuntu-22.04\n    os: linux"),
+            "multi-line pool must be indented correctly:\n{out}"
+        );
+    }
+
+    #[test]
+    fn test_generate_teardown_job_multiline_pool_indentation() {
+        let step: serde_yaml::Value = serde_yaml::from_str("bash: echo td").unwrap();
+        let pool = "name: AZS-1ES-L-MMS-ubuntu-22.04\nos: linux";
+        let out = generate_teardown_job(&[step], pool);
+        assert!(
+            out.contains("    name: AZS-1ES-L-MMS-ubuntu-22.04\n    os: linux"),
+            "multi-line pool must be indented correctly:\n{out}"
+        );
+    }
+
     #[test]
     fn test_resolve_pool_block_non_onees_defaults_to_vm_image() {
         let block = resolve_pool_block(CompileTarget::Standalone, None).expect("pool block");
@@ -6253,6 +6284,18 @@ mod tests {
         assert_eq!(block, "name: CustomPool\nos: windows");
     }
 
+    #[test]
+    fn test_resolve_pool_block_non_onees_empty_object_defaults_to_vm_image() {
+        let pool = PoolConfig::Full(PoolConfigFull {
+            name: None,
+            vm_image: None,
+            os: None,
+        });
+        let block =
+            resolve_pool_block(CompileTarget::Standalone, Some(&pool)).expect("pool block");
+        assert_eq!(block, "vmImage: ubuntu-latest");
+    }
+
     #[test]
     fn test_generate_agentic_depends_on_empty_steps() {
         assert!(generate_agentic_depends_on(&[], false, false, &[]).is_empty());