feat(ir): add EnvValue::RuntimeExpression variant to prevent $[] in step env

[Copilot]

Summary

ADO evaluates $[ ... ] runtime expressions only in job-level variables: and condition: fields — never in step env:, where the literal string is passed verbatim. This has silently broken pipelines (build #612528, #1076). The compiler now makes that mistake structurally impossible.

New variant — EnvValue::RuntimeExpression(String) in src/compile/ir/env.rs, carrying the expression body (no $[ ] wrapper):

step.with_env("AGENT_RESULT", EnvValue::runtime_expression("dependencies.Agent.result"))

Auto-hoist on lowering (lower_job):

• Each distinct RuntimeExpression in a job's step env: is lifted to a generated job variable AwRtExpr_<sha256[..12]>: $[ body ].
• The step env: value lowers to a $(AwRtExpr_…) macro reference. The name is content-derived so the hoist pass and env lowering agree without a shared map; identical expressions collapse to one variable, and an explicit job var of the same name is never clobbered.

Guard — lower_bash/lower_task reject any EnvValue::Literal containing $[, with an error pointing to RuntimeExpression. Mirrors the existing assert_no_dollar_bracket_in_step_env test helper, now enforced at lower time.

Other — graph.rs treats the variant as carrying no OutputRef (no edges). RuntimeExpression is only valid at the top level of a step env: value: nesting it inside Coalesce or Concat is rejected at lower time. The hoist pass walks only the top level of each step env (and Concat lowers its children verbatim), so a nested occurrence would otherwise leave a dangling $(AwRtExpr_…) macro; a Literal carrying $[ nested inside a Concat is rejected for the same reason. Docs updated in docs/ir.md.

No existing step-env violations remain in the tree — prior bugs were already patched via manual hoists; this variant is preventive.

Test plan

• cargo test — added unit tests in env.rs (constructor) and lower.rs (macro emission, hoist-to-variable, dedup, literal-$[ rejection, RuntimeExpression/$[-literal nested-Concat rejection including nested Concat, and a positive test that valid macro-form Concat children still lower); full suite passes.
• cargo clippy --all-targets --all-features clean.

[jamesadevine]

/rust-review

[github-actions]

✅ Rust PR Reviewer completed successfully!

[github-actions]

🔍 Rust PR Review

Summary: Solid design with one latent bug in the hoist-collection pass — worth fixing before merging to prevent a silent footgun for future callers.

---

Findings

🐛 Bugs / Logic Issues

src/compile/ir/lower.rs — collect_runtime_expr_hoists doesn't recurse into Concat children

The hoist-collection pass only matches RuntimeExpression at the top level of each step's env map:

for v in env.values() {
    if let EnvValue::RuntimeExpression(body) = v { ... }  // top-level only
}

But lower_env_value does recurse into Concat children — if a RuntimeExpression appears nested inside a Concat, the lowerer emits $(AwRtExpr_<hash>) in the step env while the corresponding job-level variable is never created, leaving a dangling macro reference at runtime (ADO silently expands undefined macros to empty string).

The same gap applies to reject_runtime_expr_literal_in_step_env — a Literal("$[ ... ]") nested inside a Concat bypasses the guard for the same reason.

Both fixes point the same way: either recurse into Concat children in both functions, or add an explicit bail! for RuntimeExpression inside Concat (mirroring the existing Coalesce rejection in lower_env_value_as_expr_atom). The rejection approach is simpler and more consistent — Concat is documented as "macro-form concatenation" and mixing a hoisted expression inside it is a conceptual mismatch. Add a test case that exercises Concat([RuntimeExpression(...)]) to lock this in either way.

(No existing code triggers this today since RuntimeExpression is brand-new, but the gap will bite future callers.)

---

✅ What Looks Good

• Design is correct: hoisting to a hash-derived job variable and reading it via $(macro) is exactly the right workaround for ADO's limitation, and making the mistake structurally impossible at the IR level is the right approach.
• Hash-based deterministic naming: avoids threading a shared mutable map through lowering — collect_runtime_expr_hoists and lower_env_value independently agree on the same name, which is clever and clean.
• Deduplication and precedence: identical expressions collapse to a single variable; explicit job.variables win over hoists — both are correct.
• RuntimeExpression-inside-Coalesce is already rejected at lower_env_value_as_expr_atom with a clear error message pointing to the fix. Consistent treatment for Concat would complete the picture.
• Error messages are actionable — they name the right fix (EnvValue::RuntimeExpression) and cite the real build numbers where the underlying ADO bug manifested.
• Tests cover the three main scenarios (emission, hoist, dedup, literal-$[ rejection) and the env.rs unit test is correctly scoped.

Generated by Rust PR Reviewer for issue #1082 · 212.4 AIC · ⌖ 12.2 AIC · ⊞ 33.8K · ◷
_{Comment /rust-review to run again}

[jamesadevine]

@copilot address feedback

[Copilot]

@copilot address feedback

Addressed the rust-review finding: RuntimeExpression (and Literals carrying $[ ... ]) nested inside a Concat are now rejected at lower time in lower_env_value, mirroring the existing Coalesce rejection. Since Concat lowers children verbatim and the hoist pass only walks the top level of each step env:, a nested occurrence would otherwise leave a dangling $(AwRtExpr_…) macro. Added tests for the direct, literal-$[, and nested-Concat cases (plus a positive test that valid macro-form children still pass) and documented the top-level-only constraint in docs/ir.md. (e19ddc7 → latest)

[jamesadevine]

/rust-review

[github-actions]

✅ Rust PR Reviewer completed successfully!

[github-actions]

🔍 Rust PR Review

Summary: Looks good — solid structural fix with comprehensive tests. One minor guard gap worth noting.

Findings

⚠️ Suggestions

• src/compile/ir/lower.rs — reject_runtime_expr_literal_in_step_env doesn't cover RawYamlScalar

  The guard fires on EnvValue::Literal("$[..."), but EnvValue::RawYamlScalar(Value::String("$[ foo ]")) is short-circuited before lower_env_value in both lower_bash and lower_task (the RawYamlScalar branch clones the raw value directly). A RawYamlScalar carrying a runtime-expression string would silently pass verbatim into the step env — exactly the footgun being blocked.

  RawYamlScalar is documented for non-string scalars (ints, bools), so this is a misuse scenario rather than an expected path, but a belt-and-suspenders check would close the gap:

  fn reject_runtime_expr_literal_in_step_env(key: &str, v: &EnvValue) -> Result<()> {
      let raw_str = match v {
          EnvValue::Literal(s) => Some(s.as_str()),
          EnvValue::RawYamlScalar(Value::String(s)) => Some(s.as_str()),
          _ => None,
      };
      if let Some(lit) = raw_str
          && lit.contains("$[")
      { ... }
  }

• src/compile/ir/lower.rs — "explicit job var wins" path is untested

  collect_runtime_expr_hoists has the guard if !vars.contains_key(&key) to avoid clobbering an explicit JobVariable with the same AwRtExpr_... name, but no test exercises this path. Extremely unlikely in practice, but a small unit test would lock in the invariant.

✅ What Looks Good

• Structural prevention: Making RuntimeExpression a distinct variant ensures the misuse is caught at compile time rather than relying on runtime string-scanning alone. The three-layer defence (new type variant + hoist pass + literal guard) is the right approach.
• Deterministic naming: Hash-derived variable names let collect_runtime_expr_hoists and lower_env_value independently agree on the same key with no shared mutable state. IndexMap preserves insertion order so YAML output is stable across compilation runs.
• Deduplication: The HashSet<String> dedup in collect_runtime_expr_hoists collapses identical expressions to a single hoisted variable — verified by lower_job_deduplicates_identical_runtime_expressions.
• Nested rejection coverage: reject_runtime_expr_in_concat + the recursion in lower_env_value correctly catches RuntimeExpression inside arbitrarily deep Concat nesting (and tested for it).
• Error messages: Every bail! path names the offending key, the bad variant, and points to the correct fix — immediately actionable for a future internal caller.
• Test breadth: The new tests cover macro emission, hoist-to-variable, dedup, literal-$[ rejection, RuntimeExpression-in-Concat, literal-$[-in-Concat, and nested Concat — good edge-case coverage.

Warning

Firewall blocked 2 domains

The following domains were blocked by the firewall during workflow execution:

• spsprodeus21.vssps.visualstudio.com
• spsprodweu4.vssps.visualstudio.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "spsprodeus21.vssps.visualstudio.com"
    - "spsprodweu4.vssps.visualstudio.com"

See Network Configuration for more information.

Generated by Rust PR Reviewer for issue #1082 · 290.4 AIC · ⌖ 12.4 AIC · ⊞ 33.8K · ◷
_{Comment /rust-review to run again}

[jamesadevine]

/rust-review

[github-actions]

✅ Rust PR Reviewer completed successfully!

[github-actions]

🔍 Rust PR Review

Summary: Looks good — the design is sound and test coverage is thorough. One genuine bug in the Concat guard's handling of RawYamlScalar.

Findings

🐛 Bugs / Logic Issues

• src/compile/ir/lower.rs — reject_runtime_expr_in_concat misses RawYamlScalar

  reject_runtime_expr_literal_in_step_env (the top-level guard) correctly catches both Literal and RawYamlScalar(Value::String(s)) containing $[. The Concat-child guard reject_runtime_expr_in_concat only checks Literal, not RawYamlScalar:

  fn reject_runtime_expr_in_concat(v: &EnvValue) -> Result<()> {
      match v {
          EnvValue::RuntimeExpression(body) => anyhow::bail!(...),
          EnvValue::Literal(lit) if lit.contains("$[") => anyhow::bail!(...),
          // RawYamlScalar(Value::String("$[ ... ]")) slips through here ↓
          _ => Ok(()),
      }
  }

  A Concat([RawYamlScalar(Value::String("$[ deps.Agent.result ]"))]) passes the guard, then yaml_value_to_scalar_string returns the literal string which concatenates into the step env — ADO receives $[ deps.Agent.result ] verbatim (the exact footgun being patched). The fix is a third arm mirroring the top-level guard:

  EnvValue::RawYamlScalar(Value::String(lit)) if lit.contains("$[") => anyhow::bail!(...),

⚠️ Suggestions

• src/compile/ir/lower.rs — missing Task hoist test — all lowering tests use BashStep. collect_runtime_expr_hoists handles Task steps identically but there's no test that a RuntimeExpression in a TaskStep env is hoisted. Low risk given the symmetry, but a one-liner test would close the gap.

• src/compile/ir/lower.rs — runtime_expr_var_name body injection note — the body is interpolated verbatim into $[ {body} ] at hoist time. Today all call sites are compiler-internal (safe), but if a future path ever plumbs user text through EnvValue::runtime_expression(), a body containing ] would prematurely close the expression. Worth a doc note on the constructor or a validation guard (e.g. rejecting ] in the body at construction time).

✅ What Looks Good

• Hash-derived naming in runtime_expr_var_name is elegant — the hoist pass and the env lowering independently converge on the same variable name without a shared mutable map; deduplication falls out naturally.
• "explicit variable wins" semantics are correct and tested — the name collision path can't silently corrupt an author-declared variable.
• Test coverage is exceptionally thorough: macro emission, hoist, dedup, literal rejection, RawYamlScalar rejection, Concat/Coalesce nesting, nested Concat, and the explicit-variable-wins case are all covered.
• graph.rs treatment is correct — RuntimeExpression carries no OutputRef, so no graph edges are added.

Generated by Rust PR Reviewer for issue #1082 · 154.9 AIC · ⌖ 12.5 AIC · ⊞ 35.7K · ◷
_{Comment /rust-review to run again}