feat(compile): support named pool demands

[Copilot]

Summary

Standalone/job/stage workflows could select a named Azure Pipelines pool but could not express standard pool.demands, forcing lock-file edits that recompilation erased.

• Add typed pool.demands support for named pools.
• Preserve demand ordering when lowering to Azure Pipelines YAML.
• Reject demands with vmImage / default hosted pools.
• Emit 1ES demands on shared extends.parameters.pool.
• Document front matter and target behavior.

pool:
  name: CustomPool
  demands:
    - CustomCapability -equals required-value

Test plan

• cargo build
• cargo test
• cargo clippy --all-targets --all-features
• Manual compile checks for standalone, 1ES, and invalid vmImage + demands

[jamesadevine]

/rust-review

[github-actions]

✅ Rust PR Reviewer completed successfully!

[github-actions]

🔍 Rust PR Review

Summary: Looks good — clean, well-structured feature addition with one validation gap and a few minor items worth noting.

Findings

🐛 Bugs / Logic Issues

• src/compile/common.rs:1246 — The (Some(name), Some(vm_image)) match arm bails with "pool cannot specify both name and vmImage" but does not check for demands. If a user writes { name: X, vmImage: Y, demands: [...] }, the error message omits any mention that demands are also incompatible here. This is minor (the compile still fails), but the error could be clearer.

• src/compile/common.rs:1239 — PoolConfig::Name(name) (string form) gets demands: Vec::new() correctly. No issue here, just confirming it's handled.

⚠️ Suggestions

• src/compile/types.rs:103 — PoolConfig::demands() is gated #[cfg(test)]. Given demands are now a documented, first-class field, this accessor may be useful to non-test code (e.g., display/lint commands). Consider removing the attribute.

• src/compile/ir/lower.rs (lower_job_emits_named_pool_demands test) — The test asserts an exact YAML substring including indentation, which is fragile against serializer changes. A structural assertion (parse the YAML back and check the sequence) would be more resilient.

• No fixture-based integration test — The tests/fixtures/ pattern isn't exercised here. A fixture with a named pool + demands would catch regressions in the full compile path (types → IR → lowering → YAML). Consistent with the project's test conventions.

✅ What Looks Good

• Validation covers all invalid states: vmImage + demands, default pool + demands — clear error messages.
• PoolSummary updated in lockstep — MCP author tooling will expose demands correctly.
• serde(default) on demands ensures full backward compatibility.
• 1ES path correctly threads demands to extends.parameters.pool.
• Lowering order: name → demands → image → os matches ADO's documented pool object structure.

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

[jamesadevine]

@copilot implement feedback and suggestions and beef up tests

[Copilot]

@copilot implement feedback and suggestions and beef up tests

Implemented in 67dd606: clarified the combined name/vmImage/demands validation error, exposed PoolConfig::demands(), switched the lowerer test to structural YAML assertions, and added fixture coverage for standalone/job/stage/1ES named pool demands. Validation passed: cargo build, cargo test, cargo clippy --all-targets --all-features, secret scan, and CodeQL.

[jamesadevine]

/rust-review

[github-actions]

✅ Rust PR Reviewer completed successfully!

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — well-structured feature with good test coverage and proper validation. One logic inconsistency in the 1ES path and a minor ordering note.

Findings

🐛 Bugs / Logic Issues

• src/compile/common.rs:1200–1236 — 1ES path silently accepts demands on the default pool

  In the standalone/job/stage arm (_ branch), the code correctly rejects (None, None) if !full.demands.is_empty() with an explicit error:

  "pool.demands requires pool.name and cannot be used with the default pool.vmImage"

  But in the 1ES arm, the match (full.name.as_deref(), full.vm_image.as_deref()) block has no such guard — (None, None) falls through to _ => {}, and then line 1236 includes the demands on DEFAULT_ONEES_POOL:

  demands: pool.map(PoolConfig::demands).unwrap_or_default().to_vec(),

  So for target: 1es, a user who writes:

  pool:
    demands:
      - CustomCapability -equals x

  ...gets the demands silently applied to the default 1ES pool (AZS-1ES-L-MMS-ubuntu-22.04) rather than a helpful error. Whether demands on the default named 1ES pool is intentionally allowed is unclear from the PR, but the asymmetric behavior is a footgun either way — it should either be explicitly accepted (with a test) or explicitly rejected (with a (None, None) if !full.demands.is_empty() arm matching the standalone behavior).

⚠️ Suggestions

• src/compile/common.rs:1201 — redundant first arm in 1ES match

  The (Some(name), Some(vm_image)) if !full.demands.is_empty() arm fires before (Some(name), Some(vm_image)) but produces a longer error message. The guard condition does nothing functionally useful — both arms call bail!. You could collapse them into one arm (the demands case) and let the non-demands case keep its current message. Minor but reduces cognitive load.

✅ What Looks Good

• Sanitization is correct — PoolConfigFull derives SanitizeConfig, which handles Vec<String> fields (strips control chars / ##vso[ injection). Demand strings like "CustomCapability -equals value" are unaffected.
• IR lowering ordering is preserved — demands are emitted before image/os in the YAML mapping, matching natural ADO convention.
• Test coverage is solid — unit tests in types.rs, common.rs, and ir/lower.rs cover the main validation branches plus the round-trip lowering. Integration tests cover standalone/job/stage and 1ES targets.
• 1ES demands plumbing via PoolConfig::demands() — clean single call-site at line 1236 avoids duplicating the logic for Name vs Full variants.

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

[jamesadevine]

@copilot fix feedback and suggestions

[Copilot]

@copilot fix feedback and suggestions

Implemented in e677a2a and follow-up 819a8ba: 1ES now rejects pool.demands unless an explicit pool.name is provided, the redundant 1ES validation branch is simplified, and unit + CLI compile regression coverage was added. Validation passed locally: cargo build, cargo test, cargo clippy --all-targets --all-features, secret scan, and final code review. CodeQL timed out on the final validation rerun after an earlier no-alert result.

[jamesadevine]

/rust-review

[github-actions]

✅ Rust PR Reviewer completed successfully!

[github-actions]

🔍 Rust PR Review

Summary: Looks good — clean, well-tested implementation with no bugs or security issues found.

Findings

✅ What Looks Good

• Type safety: demands: Vec<String> on both PoolConfigFull (front matter) and Pool::Named (IR) with #[serde(default)] — correct deserialization default, no Option<Vec> ceremony needed.
• Guard coverage: All invalid combinations are rejected at resolve_pool_typed — vmImage + demands, name + vmImage + demands, and default pool (both standalone and 1es targets) + demands. The match arm ordering is correct; more-specific guards fire before catch-all arms.
• YAML lowering: lower_pool correctly skips emitting demands: when the vec is empty, keeping clean output for the common case.
• 1ES hoisting: extends.parameters.pool goes through lower_pool, so named-pool demands flow into the shared pool block on the 1es target — confirmed by the integration test.
• Error messages: All bail! messages are actionable and user-facing, consistent with project conventions.
• Test coverage: Unit tests in common.rs and lower.rs, plus integration tests across all four targets including a binary-level rejection test for the 1ES default-pool case.

⚠️ Suggestions

• src/compile/common.rs:1233 — pool.map(PoolConfig::demands).unwrap_or_default().to_vec() works but is slightly opaque. The inner unwrap_or_default() resolves Option<&[String]> to &[], which is correct; pool.map_or(&[], PoolConfig::demands).to_vec() would read more directly.

• src/compile/types.rs — PoolConfig::demands() for Name(_) returns &[] (correct), but a brief doc comment noting the scalar name form has no demands concept would prevent a future reader from wondering whether demands are silently dropped.

• No content validation on demand strings — Values arrive from user YAML and are emitted verbatim. They're serialized through serde_yaml (no YAML injection risk), and ADO never evaluates demand values as script output (no ##vso[ risk). Fine for now; src/validate.rs would be the right place if an allowlist is ever warranted.

None of these block merging.

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