feat(compile): support Azure DevOps variable group imports

[jamesadevine]

Summary

Adds source-level support for Azure DevOps variable group imports so generated pipeline locks can consume project-level (often Key Vault-backed) secrets — such as a shared GitHub App private key — without hand-editing the generated YAML.

A new front-matter list declares the imports:

variable-groups:
  - Agentic Workflows
  - Shared Secrets

which compiles to a top-level import per entry, in declaration order:

variables:
  - group: Agentic Workflows
  - group: Shared Secrets

Why

In Azure DevOps, a variable group being authorized for a pipeline definition is not enough — the YAML must also explicitly import the group. ado-aw already lets engine.github-app-token.private-key reference an ADO secret variable and emits $(NAME) macros, but never imported the group, so a source-clean lock could not consume a project-level secret without hand-patching (which breaks lock integrity). This PR provides the missing YAML import.

Changes

• IR (compile/ir/mod.rs, compile/ir/lower.rs): PipelineVar is now an enum modelling - group: imports alongside literal - name/value entries, with matching lowering.
• Front matter (compile/types.rs): new variable-groups: Vec<String> field.
• Validation (compile/common.rs + validate.rs, wired at the shared build_pipeline_context chokepoint): group names are validated as safe references (reject $(, $[, ${{, ##vso[, ##[, control characters, newlines); a non-empty variable-groups: on target: job / target: stage is a hard compile-time error (ADO templates cannot carry pipeline-level variables: — the parent pipeline owns them). Names with leading/trailing whitespace, and duplicate imports (case-insensitive), are also rejected — padded names are emitted verbatim, and ADO group names are case-insensitive and a repeated - group: has unspecified collision behaviour.
• Builders (compile/standalone_ir.rs, compile/onees_ir.rs): emit the imports at the pipeline root (valid alongside the 1ES extends: block).
• Docs: docs/front-matter.md, docs/engine.md, docs/targets.md, and the authoring prompt prompts/create-ado-agentic-workflow.md.

Security

• Source carries only group names — never secret values.
• ado-aw never resolves, prints, logs, or serialises a group's variable values.
• Existing secret handling is unchanged: steps still reference secrets by macro ($(VAR_NAME)).

Design decisions

• Shape: ado-aw-specific variable-groups: list (rather than ADO-native variables: - group:), to avoid a future collision with literal name/value variables: support. The list handles multiple groups natively.
• job/stage targets: fail fast with a clear compile-time error pointing at the parent pipeline, rather than silently emitting nothing.

Testing

• cargo check ✓, cargo clippy --all-targets --all-features ✓ (clean)
• Full suite: 2506 unit + all integration tests pass, 0 failures
• New coverage: IR lowering (single/multiple/ordered group imports), front-matter validation (accept standalone/1es; reject job/stage; reject unsafe names), and end-to-end compile tests (standalone + 1ES output shape, job/stage errors, injection rejection, and github-app-token + variable group compiling without a lock patch)
• Lock round-trip tests confirm existing pipelines are byte-unchanged

Follow-up

• Accept hyphenated ADO variable names for engine.github-app-token.private-key (Key Vault-backed variable groups) #1416 tracks accepting hyphenated private-key names (Key Vault-backed groups commonly expose hyphenated secret names), recommending a dedicated ADO-variable-name validator.

Closes #1385

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — clean design, correct security validation, proper YAML lowering. Three minor findings worth noting before merge.

Findings

⚠️ Suggestions

• src/validate.rs:505 — redundant contains_newline check.
  is_valid_variable_group_name already calls !name.chars().any(|c| c.is_control()), and both \n (U+000A) and \r (U+000D) are control characters, so !contains_newline(name) on the next line can never catch anything new. It's dead logic. The doc comment reinforces the confusion by listing "control characters and newlines" as two distinct things. Suggest removing the !contains_newline(name) call (or updating the doc to just say "control characters including newlines").

• docs/front-matter.md:88 — validation rejection list is incomplete.
  The docs say group names are rejected if they contain ${{, $(, $[, ##vso[, ##[, control characters, or newlines — but the actual validator also rejects the compiler's own {{ template marker via contains_template_marker. A user reading only the docs would think {{ my group }} is a legal name.

• src/compile/onees_ir.rs:115 — inline vs. helper inconsistency.
  standalone_ir.rs extracts a named variable_group_vars helper. onees_ir.rs inlines the equivalent .iter().cloned().map(PipelineVar::Group).collect() chain. Semantically identical. The helper should either be promoted to common.rs and shared, or onees_ir.rs should call it directly — mixing the two approaches makes future readers unsure which is canonical.

• tests/compiler_tests.rs — stage target rejection not covered at integration level.
  variable_groups_rejected_for_job_target only exercises target: job. The target: stage path is covered by the unit test in common.rs, but an integration-level twin would be consistent with the pattern already established for the job case.

✅ What Looks Good

• Validation is wired at the shared build_pipeline_context chokepoint — all targets get it without per-target duplication.
• lower_variables correctly emits - group: <name> with no name:/value: keys; the IR unit test asserts this explicitly.
• Key ordering in lower_with_graph inserts variables: before extends: for 1ES — correct ADO YAML semantics.
• Format string escaping in the bail! error message is correct ({{{{ → {{).
• No unwrap()/expect() in non-test paths; all fallible functions return Result.

Generated by Rust PR Reviewer for #1426 · 70.2 AIC · ⌖ 6.45 AIC · ⊞ 6.2K · ◷

[jamesadevine]

@copilot implement suggestions

[Copilot]

@copilot implement suggestions

Done in 04d9a3a:

1. Redundant contains_newline check — removed from is_valid_variable_group_name; !name.chars().any(|c| c.is_control()) already subsumes newlines. Doc comment updated to "control characters (including \n / \r)".
2. Docs rejection list — docs/front-matter.md now lists the compiler's {{ template marker alongside the ADO expression/command patterns.
3. variable_group_vars helper — promoted from standalone_ir.rs to common.rs; both standalone_ir.rs and onees_ir.rs now call the shared helper.
4. Stage target integration test — variable_groups_rejected_for_stage_target was already present in the original commit; no change needed.

[jamesadevine]

/rust-review

[github-actions]

✅ Rust PR Reviewer completed successfully!

[github-actions]

🔍 Rust PR Review

Summary: One real regression bundled with an otherwise clean feature — needs a fix before merge.

Findings

🐛 Bugs / Logic Issues

• src/compile/types.rs:512 + src/validate.rs — private-key validator silently regressed.
  On main, GithubAppTokenConfig::validate() uses is_valid_ado_variable_name, which accepts [A-Za-z0-9._-] — covering the hyphenated Key Vault-backed names (e.g. AGENTIC-WORKFLOWS-GITHUB-APP-PRIVATE-KEY) that this very PR is designed to enable. The PR quietly removes is_valid_ado_variable_name and substitutes is_valid_env_var_name, which only accepts [A-Za-z_][A-Za-z0-9_]*. Any user with private-key: MY-KEYVAULT-SECRET-NAME in an existing agent file will now get a hard compile error where none existed before. The test test_github_app_token_validate_accepts_hyphenated_private_key_override (which explicitly guarded this case) was deleted alongside the function. The PR notes issue Accept hyphenated ADO variable names for engine.github-app-token.private-key (Key Vault-backed variable groups) #1416 as a follow-up for hyphenated names, but the regression ships immediately in this PR.
  Fix: Restore is_valid_ado_variable_name (or an equivalent) for private-key until Accept hyphenated ADO variable names for engine.github-app-token.private-key (Key Vault-backed variable groups) #1416 lands. The variable-groups: feature does not require this change.

⚠️ Suggestions

• src/compile/common.rs:validate_variable_groups — duplicate group names not detected.
  variable-groups: [A, A] passes validation and emits two - group: A entries. ADO's duplicate-import behavior is not specified; a deduplicate-with-warning pass (or even a lint error) would prevent surprising runtime behavior.

✅ What Looks Good

• Validation wired at the shared build_pipeline_context chokepoint — all targets protected without per-target duplication.
• PipelineVar::Group lowering emits only group: with no stray name:/value: keys; asserted by the unit test.
• variables: is inserted before extends: in 1ES lowering by construction (correct ADO YAML key order).
• is_valid_variable_group_name correctly rejects ADO expressions, pipeline commands, template markers, and control characters.
• No unwrap()/expect() in non-test paths; all fallible functions return Result.
• Error messages from the target-restriction check are actionable and name the offending target.
• Previous review round feedback (redundant contains_newline, docs rejection list, inline vs. helper, stage integration test) fully addressed.

Generated by Rust PR Reviewer for #1426 · 98.4 AIC · ⌖ 6.42 AIC · ⊞ 6.3K · ◷
_{Comment /rust-review to run again}

[jamesadevine]

Addressed the latest Rust PR Reviewer round in 26d3fa9:

Duplicate group names (suggestion) — fixed. validate_variable_groups now fails closed on duplicate imports. Detection is case-insensitive and whitespace-trimmed (ADO variable group names are case-insensitive display names), so [A, A], [Foo, foo], and [X, " X "] are all rejected with an actionable message instead of emitting redundant - group: entries. Added validate_variable_groups_rejects_duplicates and documented the rule in docs/front-matter.md.

private-key validator "regression" — not a regression (no change made). This finding is a false positive. main already validates private-key with is_valid_env_var_name at src/compile/types.rs:512; is_valid_ado_variable_name does not exist on main (verified via git grep and git show main:...), and the referenced test_..._accepts_hyphenated_private_key_override test never existed. The types.rs diff for this branch only adds the variable_groups field — it does not touch private-key validation at all. Hyphenated Key Vault names remain tracked under #1416; this PR neither changes nor blocks that behaviour.

cargo test (unit + compiler_tests) and cargo clippy pass locally.

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — solid validation design, correct lowering, and good test coverage. One validation gap worth closing before merge.

Findings

⚠️ Suggestions

• src/validate.rs + src/compile/common.rs — leading/trailing whitespace in group names is accepted but not normalized

  is_valid_variable_group_name allows names that pass only because of name.trim():

  !name.trim().is_empty()   // trims for the empty-check...
  && !name.chars().any(|c| c.is_control())  // ...but the full name is checked here

  A name like " Shared Secrets " (with surrounding spaces) passes validation, yet the raw (un-trimmed) string is emitted directly into the YAML as - group: ' Shared Secrets '.

  The duplicate check does trim for comparison (group.trim().to_ascii_lowercase()), so two entries that differ only by surrounding whitespace are caught. But a single entry with extraneous whitespace silently makes it through — and ADO's runtime group lookup behaviour with padded names is unspecified, making this a silent runtime failure rather than a compile-time error.

  Suggested fix: add name == name.trim() (or name.trim() == name) to the validator so the error is surfaced at compile time:

  pub fn is_valid_variable_group_name(name: &str) -> bool {
      !name.trim().is_empty()
          && name == name.trim()          // <-- reject leading/trailing whitespace
          && !name.chars().any(|c| c.is_control())
          // ...
  }

• src/compile/ir/lower.rs — test is missing a name: assertion

  lower_variables_emits_group_imports_in_order asserts !yaml.contains("value:") to prove no name:/value: keys bleed through from the old struct form, but doesn't also assert !yaml.contains("name:"). Both are worth checking to pin the exact shape of a Group entry.

✅ What Looks Good

• Validation chokepoint — validate_variable_groups is wired inside build_pipeline_context, which is the single entry point for all four targets. The Job | Stage denylist (with only four total variants) is effectively equivalent to an allowlist and the error message is user-actionable.
• Defense in depth — rejecting ${{ / $( / $[ expressions, ##vso[ pipeline commands, the compiler's own {{ template marker, and control characters is the right set of injections to block, matching the patterns used elsewhere in validate.rs.
• Empty-list fast-path — lower.rs line 142 gates on !p.variables.is_empty() before emitting variables:, so existing pipelines without groups produce byte-identical output (confirmed by the lock round-trip tests).
• Test coverage — standalone, 1ES, job-target rejection, stage-target rejection, injection name rejection, and the github-app-token + group combination all exercised end-to-end.

Generated by Rust PR Reviewer for #1426 · 58.6 AIC · ⌖ 6.39 AIC · ⊞ 6.2K · ◷

[jamesadevine]

Addressed the latest Rust PR Reviewer round in 5750569:

Leading/trailing whitespace in group names (suggestion) — fixed. is_valid_variable_group_name now includes name == name.trim(), so a single padded entry like " Shared Secrets " is rejected at compile time instead of being emitted verbatim as - group: ' Shared Secrets '. Added positive/negative cases to test_is_valid_variable_group_name and a validate_variable_groups_rejects_padded_name integration-style unit test. (The existing duplicate check already trimmed for comparison; the padded case now fails the name validator first, so I split it out of the duplicate test.) Docs updated in docs/front-matter.md.

Missing name: assertion in lower_variables_emits_group_imports_in_order (suggestion) — fixed, with a tweak. A blanket !yaml.contains("name:") fails because the top-level pipeline name: P mapping key is always present. Since a literal variable lowers to a - name: / value: sequence item, I asserted !yaml.contains("- name:") instead — this pins that a Group entry carries no name key without the false collision.

cargo test (unit + compiler_tests) and cargo clippy pass locally.

[github-actions]

🔍 Rust PR Review

Summary: Looks good — clean design, strong security validation, and good test coverage. Two minor items worth addressing.

Findings

⚠️ Suggestions

• src/compile/common.rs — redundant .trim() in duplicate-check key

  let key = group.trim().to_ascii_lowercase();

  is_valid_variable_group_name already rejects any name where name != name.trim() (the name == name.trim() guard), so by the time we reach the duplicate check, .trim() is a no-op. The comment on the error message ("whitespace-trimmed") perpetuates the confusion. This won't cause a bug today, but it's a trap for anyone who later relaxes the whitespace check in the validator without realising the duplicate key would need to change too. Simplify to let key = group.to_ascii_lowercase();.

• docs/ir.md / site/src/content/docs/reference/ir.mdx — PipelineVar shape not updated

  Both documents still show PipelineVar as a struct with name, value, is_secret fields. The PR changes it to an enum (NameValue { ... } / Group(String)). These files weren't touched in the diff, so they're now stale. Not a compile-time issue but will mislead anyone reading the IR reference.

✅ What Looks Good

• Injection surface correctly covered. is_valid_variable_group_name checks contains_ado_expression (${{, $(, $[), contains_pipeline_command (##vso[, ##[), contains_template_marker ({{), and control characters including newlines. The union of these is exactly what a group name can't carry without breaking the emitted YAML or injecting pipeline logic.
• Validation at the correct chokepoint. validate_variable_groups is called at the top of build_pipeline_context, which every compile target flows through, so job/stage rejection is guaranteed regardless of which target builder is invoked.
• lower_variables guard is correct. The block is only emitted when !p.variables.is_empty(), so pipelines without variable groups produce no top-level variables: key — byte-identical to before.
• Test coverage is solid. Unit tests cover all validation branches (accept, reject by target, reject by name, reject duplicates, reject padded names) and integration tests cover the full compile round-trip for standalone, 1ES, job, stage, injection, and the github-app-token combination.

Generated by Rust PR Reviewer for #1426 · 62.4 AIC · ⌖ 6.19 AIC · ⊞ 6.2K · ◷

[jamesadevine]

Addressed the latest Rust PR Reviewer round in c4ccfc2:

Redundant .trim() in the duplicate-check key (suggestion) — fixed. Since is_valid_variable_group_name now rejects any name where name != name.trim(), the padded case can never reach the duplicate check, so the key is simplified to group.to_ascii_lowercase(). The comment and error message no longer claim whitespace normalisation (they now say only "differ only by case"), removing the trap for anyone who later relaxes the validator.

Stale PipelineVar shape in the IR reference (suggestion) — fixed. Both docs/ir.md and site/src/content/docs/reference/ir.mdx now document the PipelineVar enum (NameValue { name, value, is_secret } / Group(String)) right after the Pipeline struct. (Note: neither doc previously showed a PipelineVar struct literal — they only referenced Vec<PipelineVar> — and the is_secret at ir.mdx:521 is the step-output summary, unrelated to variables. The real gap was that the new enum was undocumented, which is now closed.)

cargo test (variable-group unit + validator) and cargo clippy pass locally.

[github-actions]

🔍 Rust PR Review

Summary: Looks good — all findings from previous rounds have been correctly addressed; the code is in clean shape for merge.

Findings

✅ What Looks Good

• src/compile/ir/mod.rs — struct→enum refactoring is clean. All call sites updated, the Rust compiler enforced exhaustive match coverage (confirmed by the clean cargo clippy pass). The two variants model the ADO YAML shapes precisely.

• src/validate.rs — is_valid_variable_group_name is comprehensive. Post-fixup the function correctly blocks: empty/whitespace-only names, leading/trailing whitespace (via name == name.trim()), control characters (subsumes \n/\r/\t since is_control() covers the full C0+C1 range), ADO expression markers ($(, ${{, $[), pipeline commands (##vso[, ##[), and template injection ({{). The serde_yaml-based lowering handles remaining YAML special characters (:, #, quotes) safely by quoting them at serialization time.

• src/compile/common.rs — validation chokepoint is well-placed. validate_variable_groups is called inside build_pipeline_context, which is the shared entry point for all compile targets. Job/stage targets get the hard compile-time error before any IR is constructed; standalone/1ES get the group vars inserted cleanly.

• src/compile/ir/lower.rs — lowering is correct. lower_variables now handles both variants: NameValue emits name/value/(optional isSecret), Group emits only the group key — no variable values leak into the output.

• Test coverage is thorough. The test pyramid covers: unit (name validator, duplicate detection, lowering order), integration (standalone + 1ES compile, job + stage rejection, injection rejection, github-app-token combo), and lock round-trips confirm zero regression on existing pipelines.

⚠️ Suggestions

• src/validate.rs — duplicate detection uses to_ascii_lowercase(). ADO variable group names are practically ASCII-only, so this is fine. Just worth noting for the record: if ADO ever supports non-ASCII display names (e.g. Ü vs ü), to_ascii_lowercase() wouldn't normalize them and case-only duplicates would slip through. The existing code comment acknowledges ADO names are case-insensitive display names — adding a brief note that the check is ASCII-scoped would make the intent explicit for future readers. Non-blocking.

Generated by Rust PR Reviewer for #1426 · 93.3 AIC · ⌖ 6.68 AIC · ⊞ 6.2K · ◷