fix(compile): remove empty env block from executor step when no write permissions (#407)

* fix(compile): remove empty env block from executor step when no write permissions

Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/e4d2855e-c0f7-4fc9-8214-5b9942c995dd

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

* fix(test): correct misleading comment about serde_yaml and bare env: validity

Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/42b1fb99-3470-483e-acc3-99d1f8299528

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

docs/template-markers.md

@@ -444,9 +444,9 @@ If `permissions.write` is not configured, this marker is replaced with an empty
 
 ## {{ executor_ado_env }}
 
-Generates environment variable entries for the Stage 3 executor step when `permissions.write` is configured. Sets `SYSTEM_ACCESSTOKEN` to the write service connection token (`SC_WRITE_TOKEN`).
+Generates the complete `env:` block (including the `env:` key) for the Stage 3 executor step when `permissions.write` is configured. Sets `SYSTEM_ACCESSTOKEN` to the write service connection token (`SC_WRITE_TOKEN`).
 
-If `permissions.write` is not configured, this marker is replaced with an empty string. Note: `System.AccessToken` is never used directly — all ADO tokens come from explicitly configured service connections.
+If `permissions.write` is not configured, this marker is replaced with an empty string so that no `env:` block is emitted at all. Note: `System.AccessToken` is never used directly — all ADO tokens come from explicitly configured service connections.
 
 ## {{ compiler_version }}
 

src/compile/common.rs

@@ -914,7 +914,10 @@ pub fn generate_acquire_ado_token(service_connection: Option<&str>, variable_nam
 /// When not configured, omits ADO access tokens entirely.
 pub fn generate_executor_ado_env(write_service_connection: Option<&str>) -> String {
     match write_service_connection {
-        Some(_) => "SYSTEM_ACCESSTOKEN: $(SC_WRITE_TOKEN)".to_string(),
+        // The two-space indent on the value line is the YAML relative indent for a
+        // key nested under `env:`. replace_with_indent prepends the base indentation
+        // from the marker's position in the template to each subsequent line.
+        Some(_) => "env:\n  SYSTEM_ACCESSTOKEN: $(SC_WRITE_TOKEN)".to_string(),
         None => String::new(),
     }
 }
@@ -3741,6 +3744,10 @@ mod tests {
     #[test]
     fn test_generate_executor_ado_env_with_connection() {
         let result = generate_executor_ado_env(Some("my-sc"));
+        assert!(
+            result.contains("env:"),
+            "Executor env block should include the 'env:' key"
+        );
         assert!(
             result.contains("SYSTEM_ACCESSTOKEN: $(SC_WRITE_TOKEN)"),
             "Executor should use SC_WRITE_TOKEN"
@@ -3756,7 +3763,7 @@ mod tests {
     fn test_generate_executor_ado_env_none_empty() {
         assert!(
             generate_executor_ado_env(None).is_empty(),
-            "None service connection should produce empty env block"
+            "None service connection should produce empty string (no env block)"
         );
     }
 

src/data/1es-base.yml

@@ -662,8 +662,7 @@ extends:
                     exit $EXIT_CODE
                   displayName: Execute safe outputs (Stage 3)
                   workingDirectory: {{ working_directory }}
-                  env:
-                    {{ executor_ado_env }}
+                  {{ executor_ado_env }}
 
                 - bash: |
                     # Copy all logs to output directory for artifact upload

src/data/base.yml

@@ -630,8 +630,7 @@ jobs:
           exit $EXIT_CODE
         displayName: Execute safe outputs (Stage 3)
         workingDirectory: {{ working_directory }}
-        env:
-          {{ executor_ado_env }}
+        {{ executor_ado_env }}
 
       - bash: |
           # Copy all logs to output directory for artifact upload

tests/compiler_tests.rs

@@ -3481,3 +3481,60 @@ fn test_pr_filter_gate_steps_nested_in_setup_job() {
     });
     assert!(has_download, "Download step should be inside Setup job's steps list");
 }
+
+/// Test that a pipeline without `permissions.write` does not emit a bare `env:` block
+/// on the "Execute safe outputs" step (i.e. no invalid empty-mapping YAML).
+#[test]
+fn test_executor_step_no_empty_env_block_without_write_permissions() {
+    // minimal-agent.md has no permissions.write — executor env must be absent
+    let compiled = compile_fixture("minimal-agent.md");
+    assert_valid_yaml(&compiled, "minimal-agent.md");
+
+    // Verify the executor step contains no `env:` key at all.
+    // Note: a bare `env:` with no children is valid YAML (parsed as null), so
+    // assert_valid_yaml alone would not catch the regression this test guards against.
+    let execute_block_start = compiled
+        .find("Execute safe outputs (Stage 3)")
+        .expect("Should have executor step");
+    // Find the next step after the executor step (to bound our search window)
+    let after_execute = &compiled[execute_block_start..];
+    let next_step_offset = after_execute[1..]
+        .find("- bash:")
+        .map(|i| i + 1)
+        .unwrap_or(after_execute.len());
+    let executor_step_text = &after_execute[..next_step_offset];
+
+    assert!(
+        !executor_step_text.contains("env:"),
+        "Executor step should not contain an 'env:' block when write permissions are absent: {executor_step_text}"
+    );
+}
+
+/// Test that a pipeline with `permissions.write` emits a correctly-indented `env:` block
+/// on the "Execute safe outputs" step.
+#[test]
+fn test_executor_step_has_env_block_with_write_permissions() {
+    // complete-agent.md has permissions.write configured
+    let compiled = compile_fixture("complete-agent.md");
+    assert_valid_yaml(&compiled, "complete-agent.md");
+
+    assert!(
+        compiled.contains("SYSTEM_ACCESSTOKEN: $(SC_WRITE_TOKEN)"),
+        "Executor step should have SYSTEM_ACCESSTOKEN when write permissions are configured: {compiled}"
+    );
+
+    // Verify the env key and value appear in the correct block by checking both
+    // are present in the same neighbourhood.
+    let execute_block_start = compiled
+        .find("Execute safe outputs (Stage 3)")
+        .expect("Should have executor step");
+    let after_execute = &compiled[execute_block_start..];
+    assert!(
+        after_execute.contains("env:"),
+        "Executor step should contain 'env:' key after the displayName: {after_execute}"
+    );
+    assert!(
+        after_execute.contains("SYSTEM_ACCESSTOKEN: $(SC_WRITE_TOKEN)"),
+        "Executor step should include SYSTEM_ACCESSTOKEN: {after_execute}"
+    );
+}