fix(compile): address pool review feedback

[jamesadevine]

Summary

Addresses review feedback on the pool front-matter changes. Three fixes:

🐛 Bug: Setup/teardown job multi-line pool indentation

generate_setup_job and generate_teardown_job used format! to embed the pool string. For 1ES targets, the pool resolves to a multi-line string ("name: AZS-1ES-L-MMS-ubuntu-22.04\nos: linux"), causing broken YAML:

pool:
  name: AZS-1ES-L-MMS-ubuntu-22.04
os: linux # ← wrong indent, invalid YAML

Fix: Switched to replace_with_indent which correctly indents continuation lines.

🐛 Bug: Absent-pool codemod injected for all targets

The pool_object_form codemod force-injected pool: { name: AZS-1ES-L-MMS-ubuntu-22.04 } whenever pool: was absent, regardless of target. This broke new standalone pipelines that intentionally omit pool: to get the new vmImage: ubuntu-latest default.

Fix: Gate absent-pool injection on target: 1es in the front matter. Non-1ES targets now correctly inherit the vmImage: ubuntu-latest default.

⚠️ Suggestion: Empty pool object edge case

The (None, None) branch in resolve_pool_block (reached via pool: {}) silently fell back to vmImage: ubuntu-latest with no documentation.

Fix: Added a clarifying comment and a dedicated test for this edge case.

Test Plan

• All 1505 existing tests pass
• New tests added for:
  • Multi-line pool indentation in setup and teardown jobs
  • Target-gated absent-pool injection (1es vs standalone)
  • Empty pool object fallback (pool: {})
  • Version-gated behaviour (below/at/above threshold)

[github-actions]

🔍 Rust PR Review

Summary: Looks good — all three fixes are correct and well-tested.

Findings

✅ What Looks Good

• Multi-line pool indentation fix (generate_setup_job / generate_teardown_job): The switch from format!("{pool}") to replace_with_indent is the right approach. replace_with_indent correctly detects the 4-space indent of the {{ pool }} placeholder and applies it to all continuation lines, producing valid YAML for the 1ES two-line pool block.

• Brace escaping in generate_teardown_job: {{{{ pool }}}} inside format! correctly double-escapes to the literal {{ pool }} string, which is then consumed by replace_with_indent. The approach is sound.

• Codemod target gate: Reading target directly from the raw Mapping is consistent with how other codemods in this file work (pre-typed-deserialization context). The None case — absent target field — correctly falls through the target != Some("1es") check and returns Ok(false), matching the "absent = standalone" semantic.

• Tests: Coverage is thorough. The three new codemod tests (1es-at-version, standalone-no-target, explicit-standalone) and the idempotent_after_inserting_legacy_default fix cover the interesting cases. The multi-line indentation unit tests directly assert the 4-space alignment.

⚠️ Suggestions

• src/compile/codemods/0002_pool_object_form.rs:58 — The target comparison is case-sensitive (Some("1es")). A user who writes target: 1ES in their front matter would bypass the injection silently. Given that 1es appears to be the canonical lowercase form used throughout the codebase this is low risk, but a one-line map(|s| s.eq_ignore_ascii_case("1es")) would future-proof it with zero cost.

• src/compile/common.rs — generate_teardown_job return type: The function still returns a bare String (not Result<String>). This is fine for now since replace_with_indent is infallible, but it's a minor inconsistency with generate_setup_job which returns Result<String>. No action required — just flagging for awareness.

Generated by Rust PR Reviewer for issue #541 · ● 249K · ◷