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

[Copilot]

Summary

When permissions.write is absent, the "Execute safe outputs (Stage 3)" bash step emitted a bare env: key with no children — invalid ADO pipeline YAML.

Root cause: generate_executor_ado_env returned an empty string, but the templates unconditionally wrapped it in an env: key, producing:

        env:
          # ← nothing here → invalid

Fix:

• generate_executor_ado_env — returns the full env: block when a write service connection is set, empty string otherwise. replace_with_indent handles base-indentation alignment automatically:

  Some(_) => "env:\n  SYSTEM_ACCESSTOKEN: $(SC_WRITE_TOKEN)".to_string(),
  None => String::new(),

• base.yml / 1es-base.yml — removed the bare env: line; {{ executor_ado_env }} is now the sole marker on that line.
• docs/template-markers.md — updated to document that {{ executor_ado_env }} now emits the entire block or nothing.

Test plan

• Updated unit tests for generate_executor_ado_env to assert the env: key is included in the non-empty output.
• Two new integration tests in compiler_tests.rs:
  • test_executor_step_no_empty_env_block_without_write_permissions — compiles minimal-agent.md (no write perms), asserts no env: key is present on the executor step. Note: a bare env: with no children is valid YAML (parsed as null), so assert_valid_yaml alone does not catch this regression — the structural !executor_step_text.contains("env:") assertion is what guards against it.
  • test_executor_step_has_env_block_with_write_permissions — compiles complete-agent.md (with write perms), asserts SYSTEM_ACCESSTOKEN: $(SC_WRITE_TOKEN) appears under the executor step.

[jamesadevine]

/rust-review

[github-actions]

✅ Rust PR Reviewer completed successfully!

[github-actions]

🔍 Rust PR Review

Summary: Looks good — correct fix, well-tested, one misleading test comment worth noting.

Findings

⚠️ Suggestions

• tests/compiler_tests.rs:3493 — The comment "serde_yaml rejects env: with no children in strict mode and the assert_valid_yaml call above already exercises this" is incorrect. env: null (bare env: key with no children) is valid YAML and serde_yaml will parse it without error. The assert_valid_yaml call does not catch this case. The structural assertion that follows (!executor_step_text.contains("env:")) is what actually validates the fix. The comment should be removed or corrected to avoid misleading future contributors.

✅ What Looks Good

• replace_with_indent interaction is correct: The function correctly handles the multiline "env:\n SYSTEM_ACCESSTOKEN: $(SC_WRITE_TOKEN)" replacement — the env: key gets the template's base indentation, and subsequent lines get base_indent + " ", producing correctly-structured YAML for both base.yml (8-space indent) and 1es-base.yml (18-space indent).
• Empty-string case is clean: When None, the replacement is "" which replace_with_indent handles by leaving a whitespace-only line — valid YAML, no env: null artifact.
• Tests are well-scoped: The two new integration tests clearly cover both the absent and present write-permissions cases, and the find("- bash:") bounding approach is reliable for these fixtures.
• Doc update is accurate: docs/template-markers.md correctly describes the new contract ("emits the entire block or nothing").

Generated by Rust PR Reviewer for issue #407 · ● 493.2K · ◷

[jamesadevine]

@copilot tests/compiler_tests.rs:3493 — The comment "serde_yaml rejects env: with no children in strict mode and the assert_valid_yaml call above already exercises this" is incorrect. env: null (bare env: key with no children) is valid YAML and serde_yaml will parse it without error. The assert_valid_yaml call does not catch this case. The structural assertion that follows (!executor_step_text.contains("env:")) is what actually validates the fix. The comment should be removed or corrected to avoid misleading future contributors.

[Copilot]

@copilot tests/compiler_tests.rs:3493 — The comment "serde_yaml rejects env: with no children in strict mode and the assert_valid_yaml call above already exercises this" is incorrect. env: null (bar...

Fixed in 69ab847. The comment now correctly states that a bare env: is valid YAML (parsed as null) and assert_valid_yaml would not catch the regression — the !executor_step_text.contains("env:") assertion is what actually guards against it.