From ceb863eb25a03893775f0358329fdc038d54bf6d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 17 Jun 2026 22:38:01 +0000 Subject: [PATCH 1/6] Initial plan From e19ddc7aff8189559f1a021cda40540ceef90b4b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 17 Jun 2026 22:54:19 +0000 Subject: [PATCH 2/6] feat(ir): add EnvValue::RuntimeExpression with auto-hoist + step-env guard Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> --- docs/ir.md | 2 +- src/compile/ir/env.rs | 47 +++++++++ src/compile/ir/graph.rs | 1 + src/compile/ir/lower.rs | 224 +++++++++++++++++++++++++++++++++++++++- 4 files changed, 269 insertions(+), 5 deletions(-) diff --git a/docs/ir.md b/docs/ir.md index 45406e1a..6a5ecbde 100644 --- a/docs/ir.md +++ b/docs/ir.md @@ -22,7 +22,7 @@ Those wrappers are the only place per-target shape (top-level `PipelineShape`, t - `tasks.rs` — typed factory helpers for built-in ADO tasks that return preconfigured `TaskStep` values with required inputs set. Prefer extending these helpers for compiler-generated tasks rather than open-coding `TaskStep::new(...)` with raw string keys at each call site. - `job.rs` — `Job`, `Pool`, job variables, 1ES `templateContext` support, and target-job external `dependsOn` / `condition` wrapping. - `stage.rs` — `Stage` plus target-stage external `dependsOn` / `condition` wrapping. -- `env.rs` — typed environment values (`EnvValue`) including ADO macros, pipeline variables, secrets, `OutputRef`s, `Coalesce`, and macro-form `Concat`. +- `env.rs` — typed environment values (`EnvValue`) including ADO macros, pipeline variables, secrets, `OutputRef`s, `Coalesce`, macro-form `Concat`, and `RuntimeExpression` (a `$[ ... ]` ADO runtime expression that the lowering pass auto-hoists to a job-level `variables:` entry — ADO does not evaluate `$[ ... ]` inside step `env:`). - `condition.rs` — the `Condition` / `Expr` AST and code generation to ADO condition syntax. - `output.rs` — `OutputDecl`, `OutputRef`, and the output-reference lowering rules. - `graph.rs` — graph construction, `dependsOn` derivation, output validation, `isOutput=true` promotion, and cycle detection. diff --git a/src/compile/ir/env.rs b/src/compile/ir/env.rs index b04aac9a..ccefaa69 100644 --- a/src/compile/ir/env.rs +++ b/src/compile/ir/env.rs @@ -36,6 +36,11 @@ //! same-job consumers, where macro form is the only form that //! resolves correctly (see `src/compile/filter_ir.rs` for the //! underlying bug history). +//! - [`EnvValue::RuntimeExpression`] — a hand-written ADO `$[ ... ]` +//! runtime expression. ADO does not evaluate these inside step +//! `env:`, so the lowering pass hoists each one to a +//! compiler-generated job-level `variables:` entry and emits a +//! `$(name)` macro reference in the step `env:`. use super::output::OutputRef; @@ -86,6 +91,25 @@ pub enum EnvValue { /// yields the live value — the `prGate` step's /// `$(System.PullRequest.X)$(synthPr.X)` exclusive-OR. Concat(Vec), + /// A hand-written ADO **runtime expression** (`$[ ... ]`), carried + /// as a distinct type so it can never be confused with a + /// [`EnvValue::Literal`]. + /// + /// The stored string is the expression **body** only — the + /// `$[ ` / ` ]` wrapper is added during lowering. For example + /// `RuntimeExpression("coalesce(dependencies.Agent.result, '')")` + /// lowers to the variable value `$[ coalesce(dependencies.Agent.result, '') ]`. + /// + /// ADO only evaluates `$[ ... ]` runtime expressions inside + /// job-level `variables:` mappings and `condition:` fields — **not** + /// inside step `env:` values, where the literal expression string is + /// passed verbatim (msazuresphere/4x4 build #612528, + /// githubnext/ado-aw#1076). To make that mistake structurally + /// impossible, the lowering pass automatically **hoists** every + /// `RuntimeExpression` in a step's `env:` to a compiler-generated + /// job-level `variables:` entry and rewrites the step `env:` value + /// to a `$(generated_name)` macro reference pointing at it. + RuntimeExpression(String), /// Pre-built YAML scalar emitted verbatim into the value position. /// /// Used by [`crate::compile::agentic_pipeline`] when a legacy YAML @@ -210,6 +234,18 @@ impl EnvValue { pub fn concat(values: Vec) -> Self { EnvValue::Concat(values) } + + /// Construct an [`EnvValue::RuntimeExpression`] from an ADO + /// runtime-expression **body** (without the `$[ ]` wrapper). + /// + /// Use this instead of [`EnvValue::literal`] whenever a step + /// `env:` value needs an ADO `$[ ... ]` runtime expression: the + /// lowering pass hoists it to a job-level `variables:` entry and + /// emits a `$(name)` macro reference, because ADO does not + /// evaluate `$[ ... ]` inside step `env:`. + pub fn runtime_expression(body: impl Into) -> Self { + EnvValue::RuntimeExpression(body.into()) + } } #[cfg(test)] @@ -269,4 +305,15 @@ mod tests { _ => panic!("expected Concat"), } } + + #[test] + fn runtime_expression_stores_body_verbatim() { + let v = EnvValue::runtime_expression("coalesce(dependencies.Agent.result, '')"); + match v { + EnvValue::RuntimeExpression(body) => { + assert_eq!(body, "coalesce(dependencies.Agent.result, '')"); + } + _ => panic!("expected RuntimeExpression"), + } + } } diff --git a/src/compile/ir/graph.rs b/src/compile/ir/graph.rs index 42669372..9e93e43c 100644 --- a/src/compile/ir/graph.rs +++ b/src/compile/ir/graph.rs @@ -334,6 +334,7 @@ fn collect_env_refs_into<'a>(v: &'a EnvValue, out: &mut Vec<&'a OutputRef>) { | EnvValue::AdoMacro(_) | EnvValue::PipelineVar(_) | EnvValue::Secret(_) + | EnvValue::RuntimeExpression(_) | EnvValue::RawYamlScalar(_) => {} EnvValue::StepOutput(r) => out.push(r), EnvValue::Coalesce(children) | EnvValue::Concat(children) => { diff --git a/src/compile/ir/lower.rs b/src/compile/ir/lower.rs index 166d10b3..5be91a28 100644 --- a/src/compile/ir/lower.rs +++ b/src/compile/ir/lower.rs @@ -592,11 +592,25 @@ fn lower_job(job: &Job, stage: Option<&StageId>, graph: &Graph) -> Result if let Some(t) = job.timeout { m.insert(s("timeoutInMinutes"), Value::from(minutes_ceil(t))); } - if !job.variables.is_empty() { + // Merge any explicit job-level variables with compiler-generated + // hoists for `EnvValue::RuntimeExpression` values found in step + // env blocks (ADO does not evaluate `$[ ... ]` inside step env, so + // each one is hoisted to a job-level variable here and read back + // via a `$(name)` macro in the step env). + let hoisted = collect_runtime_expr_hoists(&job.steps); + if !job.variables.is_empty() || !hoisted.is_empty() { let mut vars = Mapping::new(); for v in &job.variables { vars.insert(s(&v.name), s(&lower_env_value(&ctx, &v.value)?)); } + for v in &hoisted { + // An explicit job variable of the same name wins — never + // clobber an author-declared variable with a hoist. + let key = s(&v.name); + if !vars.contains_key(&key) { + vars.insert(key, s(&lower_env_value(&ctx, &v.value)?)); + } + } m.insert(s("variables"), Value::Mapping(vars)); } @@ -899,6 +913,7 @@ fn lower_bash(b: &BashStep, ctx: &LoweringContext<'_>) -> Result { if !b.env.is_empty() { let mut env_map = Mapping::new(); for (k, v) in &b.env { + reject_runtime_expr_literal_in_step_env(k, v)?; // RawYamlScalar bypasses string lowering — its inner value // is inserted into the env mapping directly so serde_yaml's // emitter sees the original scalar type (e.g. number vs @@ -943,6 +958,7 @@ fn lower_task(t: &TaskStep, ctx: &LoweringContext<'_>) -> Result { if !t.env.is_empty() { let mut env_map = Mapping::new(); for (k, v) in &t.env { + reject_runtime_expr_literal_in_step_env(k, v)?; let value = match v { EnvValue::RawYamlScalar(raw) => raw.clone(), other => s(&lower_env_value(ctx, other)?), @@ -1034,6 +1050,7 @@ fn lower_env_value(ctx: &LoweringContext<'_>, v: &EnvValue) -> Result { } Ok(out) } + EnvValue::RuntimeExpression(body) => Ok(format!("$({})", runtime_expr_var_name(body))), EnvValue::RawYamlScalar(raw) => { // String fallback for callers that still go through // `lower_env_value`; the env-mapping insertion path in @@ -1119,10 +1136,18 @@ fn lower_env_value_as_expr_atom(ctx: &LoweringContext<'_>, v: &EnvValue) -> Resu level of an env value only" ) } + EnvValue::RuntimeExpression(_) => { + // A `$[ ... ]` runtime expression cannot be nested inside + // another runtime expression (`coalesce(...)`). Authors + // should fold the inner logic into the surrounding + // expression body directly. + anyhow::bail!( + "ir::lower: EnvValue::RuntimeExpression is not valid inside a Coalesce \ + (or other expression-atom context); a `$[ ... ]` expression cannot be \ + nested inside another — inline the expression body instead" + ) + } EnvValue::RawYamlScalar(raw) => { - // Inside an ADO expression, render the raw scalar as a - // single-quoted literal (numbers / booleans → literal - // text without quotes). match raw { serde_yaml::Value::String(s) => Ok(format!("'{}'", s.replace('\'', "''"))), serde_yaml::Value::Number(n) => Ok(n.to_string()), @@ -1185,6 +1210,76 @@ fn minutes_ceil(d: Duration) -> u64 { secs.div_ceil(60) } +/// Deterministic job-level variable name for a hoisted +/// [`EnvValue::RuntimeExpression`] body. +/// +/// The name is derived from a hash of the expression body so the +/// variables-hoist pass in [`lower_job`] and the env-value lowering in +/// [`lower_env_value`] independently compute the **same** name without +/// threading a shared map — identical expressions collapse to a single +/// hoisted variable, distinct ones get distinct names. +fn runtime_expr_var_name(body: &str) -> String { + let digest = crate::hash::sha256_hex(body.as_bytes()); + format!("AwRtExpr_{}", &digest[..12]) +} + +/// Collect job-level `variables:` hoists for every distinct +/// [`EnvValue::RuntimeExpression`] appearing in the job's step `env:` +/// blocks, in first-appearance order. +/// +/// ADO does not evaluate `$[ ... ]` runtime expressions inside step +/// `env:`, so each one is hoisted to a compiler-generated job variable +/// (`AwRtExpr_`) whose value is the wrapped `$[ ]` +/// expression; the step `env:` then reads it via the `$(name)` macro +/// form (see [`lower_env_value`]). +fn collect_runtime_expr_hoists(steps: &[Step]) -> Vec { + use crate::compile::ir::job::JobVariable; + use std::collections::HashSet; + + let mut seen: HashSet = HashSet::new(); + let mut out: Vec = Vec::new(); + for step in steps { + let env = match step { + Step::Bash(b) => &b.env, + Step::Task(t) => &t.env, + _ => continue, + }; + for v in env.values() { + if let EnvValue::RuntimeExpression(body) = v { + let name = runtime_expr_var_name(body); + if seen.insert(name.clone()) { + out.push(JobVariable { + name, + value: EnvValue::literal(format!("$[ {body} ]")), + }); + } + } + } + } + out +} + +/// Reject an [`EnvValue::Literal`] that smuggles a `$[ ... ]` ADO +/// runtime expression into a step `env:` value. ADO evaluates these +/// only in job-level `variables:` and `condition:` fields, so a literal +/// here would be passed verbatim — use [`EnvValue::RuntimeExpression`] +/// (which hoists to a job variable) instead. +fn reject_runtime_expr_literal_in_step_env(key: &str, v: &EnvValue) -> Result<()> { + if let EnvValue::Literal(lit) = v + && lit.contains("$[") + { + anyhow::bail!( + "ir::lower: step env var {key:?} is an EnvValue::Literal carrying a \ + `$[ ... ]` ADO runtime expression ({lit:?}). ADO does NOT evaluate runtime \ + expressions inside step env (the literal string is passed verbatim — \ + msazuresphere/4x4 build #612528, githubnext/ado-aw#1076). Use \ + EnvValue::RuntimeExpression, which hoists the expression to a job-level \ + variable and emits a $(name) macro reference, instead." + ); + } + Ok(()) +} + fn s(v: impl Into) -> Value { Value::String(v.into()) } @@ -1206,6 +1301,127 @@ mod tests { } } + #[test] + fn lower_env_value_runtime_expression_emits_hoisted_macro() { + let g = Graph::default(); + let job = JobId::new("J").unwrap(); + let ctx = ctx_for(&g, &job); + let body = "coalesce(dependencies.Agent.result, '')"; + let expected = format!("$({})", runtime_expr_var_name(body)); + assert_eq!( + lower_env_value(&ctx, &EnvValue::runtime_expression(body)).unwrap(), + expected + ); + } + + #[test] + fn lower_job_hoists_runtime_expression_to_job_variable() { + let body = "dependencies.Agent.result"; + let step = Step::Bash( + BashStep::new("Conclusion", "echo hi") + .with_env("AGENT_RESULT", EnvValue::runtime_expression(body)), + ); + let mut job = Job::new( + JobId::new("Conclusion").unwrap(), + "Conclusion", + Pool::VmImage("u".into()), + ); + job.push_step(step); + let p = Pipeline { + name: "t".into(), + parameters: Vec::new(), + resources: Resources::default(), + triggers: Triggers::default(), + variables: Vec::new(), + body: PipelineBody::Jobs(vec![job]), + shape: PipelineShape::Standalone, + }; + let v = super::lower(&p).unwrap(); + let yaml = serde_yaml::to_string(&v).unwrap(); + + let name = runtime_expr_var_name(body); + // The job carries a hoisted variable whose value is the + // wrapped `$[ ... ]` runtime expression. + assert!( + yaml.contains(&format!("{name}: $[ {body} ]")), + "expected hoisted variable `{name}: $[ {body} ]` in:\n{yaml}" + ); + // The step env references the hoisted variable via the macro + // form — never the raw `$[ ... ]` expression. + assert!( + yaml.contains(&format!("AGENT_RESULT: $({name})")), + "expected step env `AGENT_RESULT: $({name})` in:\n{yaml}" + ); + // Structural guarantee: no `$[` survives inside any step env. + assert!( + !yaml.contains("AGENT_RESULT: $["), + "step env must not carry a raw `$[ ... ]` expression:\n{yaml}" + ); + } + + #[test] + fn lower_job_deduplicates_identical_runtime_expressions() { + let body = "dependencies.Agent.result"; + let job_steps = vec![ + Step::Bash( + BashStep::new("A", "echo a") + .with_env("R1", EnvValue::runtime_expression(body)), + ), + Step::Bash( + BashStep::new("B", "echo b") + .with_env("R2", EnvValue::runtime_expression(body)), + ), + ]; + let mut job = Job::new(JobId::new("J").unwrap(), "J", Pool::VmImage("u".into())); + for st in job_steps { + job.push_step(st); + } + let p = Pipeline { + name: "t".into(), + parameters: Vec::new(), + resources: Resources::default(), + triggers: Triggers::default(), + variables: Vec::new(), + body: PipelineBody::Jobs(vec![job]), + shape: PipelineShape::Standalone, + }; + let v = super::lower(&p).unwrap(); + let yaml = serde_yaml::to_string(&v).unwrap(); + let name = runtime_expr_var_name(body); + // Two distinct env vars share one identical expression → a + // single hoisted job variable. + assert_eq!( + yaml.matches(&format!("{name}: $[ {body} ]")).count(), + 1, + "identical runtime expressions must hoist to a single variable:\n{yaml}" + ); + } + + #[test] + fn lower_bash_rejects_dollar_bracket_literal_in_step_env() { + let step = Step::Bash( + BashStep::new("Bad", "echo hi") + .with_env("X", EnvValue::literal("$[ dependencies.Agent.result ]")), + ); + let mut job = Job::new(JobId::new("J").unwrap(), "J", Pool::VmImage("u".into())); + job.push_step(step); + let p = Pipeline { + name: "t".into(), + parameters: Vec::new(), + resources: Resources::default(), + triggers: Triggers::default(), + variables: Vec::new(), + body: PipelineBody::Jobs(vec![job]), + shape: PipelineShape::Standalone, + }; + let err = super::lower(&p).unwrap_err(); + let msg = format!("{err:#}"); + assert!( + msg.contains("EnvValue::RuntimeExpression"), + "error must point to the RuntimeExpression variant; got: {msg}" + ); + } + #[test] fn lower_condition_static_variants() { // Quick sanity that lower.rs threads the condition codegen From fc95110215d16c5306d9d7d9475ef2779641af51 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 18 Jun 2026 06:30:43 +0000 Subject: [PATCH 3/6] fix(ir): reject RuntimeExpression and $[ literals nested inside Concat Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> --- docs/ir.md | 2 +- src/compile/ir/lower.rs | 99 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 1 deletion(-) diff --git a/docs/ir.md b/docs/ir.md index 6a5ecbde..65d38a2c 100644 --- a/docs/ir.md +++ b/docs/ir.md @@ -22,7 +22,7 @@ Those wrappers are the only place per-target shape (top-level `PipelineShape`, t - `tasks.rs` — typed factory helpers for built-in ADO tasks that return preconfigured `TaskStep` values with required inputs set. Prefer extending these helpers for compiler-generated tasks rather than open-coding `TaskStep::new(...)` with raw string keys at each call site. - `job.rs` — `Job`, `Pool`, job variables, 1ES `templateContext` support, and target-job external `dependsOn` / `condition` wrapping. - `stage.rs` — `Stage` plus target-stage external `dependsOn` / `condition` wrapping. -- `env.rs` — typed environment values (`EnvValue`) including ADO macros, pipeline variables, secrets, `OutputRef`s, `Coalesce`, macro-form `Concat`, and `RuntimeExpression` (a `$[ ... ]` ADO runtime expression that the lowering pass auto-hoists to a job-level `variables:` entry — ADO does not evaluate `$[ ... ]` inside step `env:`). +- `env.rs` — typed environment values (`EnvValue`) including ADO macros, pipeline variables, secrets, `OutputRef`s, `Coalesce`, macro-form `Concat`, and `RuntimeExpression` (a `$[ ... ]` ADO runtime expression that the lowering pass auto-hoists to a job-level `variables:` entry — ADO does not evaluate `$[ ... ]` inside step `env:`). `RuntimeExpression` is only valid at the top level of a step `env:` value: nesting it inside `Concat` or `Coalesce` is rejected at lower time (the hoist pass walks only the top level, so a nested occurrence would emit a dangling `$(AwRtExpr_…)` macro). - `condition.rs` — the `Condition` / `Expr` AST and code generation to ADO condition syntax. - `output.rs` — `OutputDecl`, `OutputRef`, and the output-reference lowering rules. - `graph.rs` — graph construction, `dependsOn` derivation, output validation, `isOutput=true` promotion, and cycle detection. diff --git a/src/compile/ir/lower.rs b/src/compile/ir/lower.rs index 5be91a28..61150aad 100644 --- a/src/compile/ir/lower.rs +++ b/src/compile/ir/lower.rs @@ -1046,6 +1046,7 @@ fn lower_env_value(ctx: &LoweringContext<'_>, v: &EnvValue) -> Result { // for the bug history. let mut out = String::new(); for c in children { + reject_runtime_expr_in_concat(c)?; out.push_str(&lower_env_value(ctx, c)?); } Ok(out) @@ -1284,6 +1285,38 @@ fn s(v: impl Into) -> Value { Value::String(v.into()) } +/// Reject an [`EnvValue::RuntimeExpression`] (or a [`EnvValue::Literal`] +/// smuggling a `$[ ... ]` runtime expression) nested inside an +/// [`EnvValue::Concat`]. +/// +/// `Concat` is macro-form concatenation: its children lower verbatim +/// into the step `env:` scalar (see [`lower_env_value`]). A +/// `RuntimeExpression` child would emit a `$(AwRtExpr_)` macro +/// whose backing job variable is never hoisted (the hoist pass walks +/// only the top level of each step env), and a literal `$[ ... ]` would +/// be passed verbatim — both leaving a runtime expression that ADO does +/// not evaluate in step env. Mirrors the `RuntimeExpression`-in-`Coalesce` +/// rejection in [`lower_env_value_as_expr_atom`]. Nested `Concat` children +/// are re-checked via the recursion in [`lower_env_value`]. +fn reject_runtime_expr_in_concat(v: &EnvValue) -> Result<()> { + match v { + EnvValue::RuntimeExpression(body) => anyhow::bail!( + "ir::lower: EnvValue::RuntimeExpression ({body:?}) is not valid inside an \ + EnvValue::Concat. Concat lowers its children verbatim into the step env \ + scalar, so the hoist pass never creates the backing job variable, leaving \ + a dangling $(AwRtExpr_…) macro. Use RuntimeExpression at the top level of \ + a step env value only." + ), + EnvValue::Literal(lit) if lit.contains("$[") => anyhow::bail!( + "ir::lower: EnvValue::Literal ({lit:?}) inside an EnvValue::Concat carries a \ + `$[ ... ]` ADO runtime expression. ADO does NOT evaluate runtime expressions \ + inside step env (the literal string is passed verbatim). Use \ + EnvValue::RuntimeExpression at the top level of a step env value instead." + ), + _ => Ok(()), + } +} + #[cfg(test)] mod tests { use super::*; @@ -1314,6 +1347,72 @@ mod tests { ); } + /// A `RuntimeExpression` nested inside a `Concat` is rejected — the + /// hoist pass only walks the top level of each step env, so a nested + /// occurrence would emit a `$(AwRtExpr_…)` macro with no backing job + /// variable (a dangling reference). Mirrors the `RuntimeExpression` + /// -in-`Coalesce` rejection. + #[test] + fn lower_env_value_runtime_expression_inside_concat_errors() { + let g = Graph::default(); + let job = JobId::new("J").unwrap(); + let ctx = ctx_for(&g, &job); + let v = EnvValue::concat(vec![ + EnvValue::literal("prefix-"), + EnvValue::runtime_expression("dependencies.Agent.result"), + ]); + let err = lower_env_value(&ctx, &v).unwrap_err(); + let msg = format!("{err:#}"); + assert!( + msg.contains("EnvValue::RuntimeExpression") + && msg.contains("not valid inside an EnvValue::Concat"), + "expected RuntimeExpression-in-Concat error, got: {msg}" + ); + } + + /// A `Literal` smuggling a `$[ ... ]` runtime expression nested inside + /// a `Concat` is rejected — Concat lowers children verbatim, so the + /// literal would survive as an unevaluated runtime expression in step + /// env (the same footgun the top-level guard catches). + #[test] + fn lower_env_value_dollar_bracket_literal_inside_concat_errors() { + let g = Graph::default(); + let job = JobId::new("J").unwrap(); + let ctx = ctx_for(&g, &job); + let v = EnvValue::concat(vec![ + EnvValue::literal("prefix-"), + EnvValue::literal("$[ dependencies.Agent.result ]"), + ]); + let err = lower_env_value(&ctx, &v).unwrap_err(); + let msg = format!("{err:#}"); + assert!( + msg.contains("EnvValue::Literal") && msg.contains("EnvValue::Concat"), + "expected $[-literal-in-Concat error, got: {msg}" + ); + } + + /// A `RuntimeExpression` nested inside a `Concat` inside another + /// `Concat` is also rejected — the recursion in `lower_env_value` + /// re-checks each nested level. + #[test] + fn lower_env_value_runtime_expression_inside_nested_concat_errors() { + let g = Graph::default(); + let job = JobId::new("J").unwrap(); + let ctx = ctx_for(&g, &job); + let v = EnvValue::concat(vec![ + EnvValue::literal("a-"), + EnvValue::concat(vec![EnvValue::runtime_expression( + "dependencies.Agent.result", + )]), + ]); + let err = lower_env_value(&ctx, &v).unwrap_err(); + let msg = format!("{err:#}"); + assert!( + msg.contains("not valid inside an EnvValue::Concat"), + "expected nested RuntimeExpression-in-Concat error, got: {msg}" + ); + } + #[test] fn lower_job_hoists_runtime_expression_to_job_variable() { let body = "dependencies.Agent.result"; From bf69a90a0191e8a82781e2895005784b80c97045 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 18 Jun 2026 06:31:47 +0000 Subject: [PATCH 4/6] test(ir): cover Concat accepting valid macro-form children Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> --- src/compile/ir/lower.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/compile/ir/lower.rs b/src/compile/ir/lower.rs index 61150aad..d96dbb6a 100644 --- a/src/compile/ir/lower.rs +++ b/src/compile/ir/lower.rs @@ -1347,6 +1347,26 @@ mod tests { ); } + /// A `Concat` of ordinary (non-runtime-expression) children lowers + /// cleanly — the rejection guard must not reject valid macro-form + /// atoms like `AdoMacro` / `PipelineVar` / `Secret` / plain literals. + #[test] + fn lower_env_value_concat_accepts_valid_children() { + let g = Graph::default(); + let job = JobId::new("J").unwrap(); + let ctx = ctx_for(&g, &job); + let v = EnvValue::concat(vec![ + EnvValue::literal("prefix-"), + EnvValue::ado_macro("Build.BuildId").unwrap(), + EnvValue::pipeline_var("MyVar"), + EnvValue::secret("MY_SECRET"), + ]); + assert_eq!( + lower_env_value(&ctx, &v).unwrap(), + "prefix-$(Build.BuildId)$(MyVar)$(MY_SECRET)" + ); + } + /// A `RuntimeExpression` nested inside a `Concat` is rejected — the /// hoist pass only walks the top level of each step env, so a nested /// occurrence would emit a `$(AwRtExpr_…)` macro with no backing job From dc47ee77586315572854c9c25c7c0477b7755ff5 Mon Sep 17 00:00:00 2001 From: James Devine Date: Thu, 18 Jun 2026 10:27:34 +0100 Subject: [PATCH 5/6] fix(ir): cover RawYamlScalar in step-env runtime-expr guard, test explicit-var precedence Addresses rust-review feedback on PR #1082: - reject_runtime_expr_literal_in_step_env now also rejects a RawYamlScalar(String) carrying a \$[ ... ] runtime expression, which previously bypassed string lowering and slipped past the Literal-only check. - Add tests for the RawYamlScalar guard and for the explicit-job-variable -wins-over-hoist precedence path in collect_runtime_expr_hoists. - Document the RawYamlScalar coverage in docs/ir.md. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/ir.md | 2 +- src/compile/ir/lower.rs | 99 ++++++++++++++++++++++++++++++++++++++--- 2 files changed, 93 insertions(+), 8 deletions(-) diff --git a/docs/ir.md b/docs/ir.md index 65d38a2c..f368cdbe 100644 --- a/docs/ir.md +++ b/docs/ir.md @@ -22,7 +22,7 @@ Those wrappers are the only place per-target shape (top-level `PipelineShape`, t - `tasks.rs` — typed factory helpers for built-in ADO tasks that return preconfigured `TaskStep` values with required inputs set. Prefer extending these helpers for compiler-generated tasks rather than open-coding `TaskStep::new(...)` with raw string keys at each call site. - `job.rs` — `Job`, `Pool`, job variables, 1ES `templateContext` support, and target-job external `dependsOn` / `condition` wrapping. - `stage.rs` — `Stage` plus target-stage external `dependsOn` / `condition` wrapping. -- `env.rs` — typed environment values (`EnvValue`) including ADO macros, pipeline variables, secrets, `OutputRef`s, `Coalesce`, macro-form `Concat`, and `RuntimeExpression` (a `$[ ... ]` ADO runtime expression that the lowering pass auto-hoists to a job-level `variables:` entry — ADO does not evaluate `$[ ... ]` inside step `env:`). `RuntimeExpression` is only valid at the top level of a step `env:` value: nesting it inside `Concat` or `Coalesce` is rejected at lower time (the hoist pass walks only the top level, so a nested occurrence would emit a dangling `$(AwRtExpr_…)` macro). +- `env.rs` — typed environment values (`EnvValue`) including ADO macros, pipeline variables, secrets, `OutputRef`s, `Coalesce`, macro-form `Concat`, and `RuntimeExpression` (a `$[ ... ]` ADO runtime expression that the lowering pass auto-hoists to a job-level `variables:` entry — ADO does not evaluate `$[ ... ]` inside step `env:`). `RuntimeExpression` is only valid at the top level of a step `env:` value: nesting it inside `Concat` or `Coalesce` is rejected at lower time (the hoist pass walks only the top level, so a nested occurrence would emit a dangling `$(AwRtExpr_…)` macro). A `Literal` or `RawYamlScalar(String)` smuggling a raw `$[ ... ]` into a step `env:` value is likewise rejected at lower time — ADO passes such scalars verbatim, so the typed `RuntimeExpression` variant must be used instead. - `condition.rs` — the `Condition` / `Expr` AST and code generation to ADO condition syntax. - `output.rs` — `OutputDecl`, `OutputRef`, and the output-reference lowering rules. - `graph.rs` — graph construction, `dependsOn` derivation, output validation, `isOutput=true` promotion, and cycle detection. diff --git a/src/compile/ir/lower.rs b/src/compile/ir/lower.rs index d96dbb6a..04424424 100644 --- a/src/compile/ir/lower.rs +++ b/src/compile/ir/lower.rs @@ -1260,17 +1260,27 @@ fn collect_runtime_expr_hoists(steps: &[Step]) -> Vec Result<()> { - if let EnvValue::Literal(lit) = v + let raw_str = match v { + EnvValue::Literal(lit) => Some(lit.as_str()), + EnvValue::RawYamlScalar(Value::String(lit)) => Some(lit.as_str()), + _ => None, + }; + if let Some(lit) = raw_str && lit.contains("$[") { anyhow::bail!( - "ir::lower: step env var {key:?} is an EnvValue::Literal carrying a \ + "ir::lower: step env var {key:?} is a literal scalar carrying a \ `$[ ... ]` ADO runtime expression ({lit:?}). ADO does NOT evaluate runtime \ expressions inside step env (the literal string is passed verbatim — \ msazuresphere/4x4 build #612528, githubnext/ado-aw#1076). Use \ @@ -1541,6 +1551,81 @@ mod tests { ); } + #[test] + fn lower_bash_rejects_dollar_bracket_raw_yaml_scalar_in_step_env() { + // A RawYamlScalar wrapping a `$[ ... ]` string bypasses string + // lowering (its inner value is inserted verbatim), so it must be + // caught by the same guard as a plain Literal. + let step = Step::Bash(BashStep::new("Bad", "echo hi").with_env( + "X", + EnvValue::RawYamlScalar(serde_yaml::Value::String( + "$[ dependencies.Agent.result ]".into(), + )), + )); + let mut job = Job::new(JobId::new("J").unwrap(), "J", Pool::VmImage("u".into())); + job.push_step(step); + let p = Pipeline { + name: "t".into(), + parameters: Vec::new(), + resources: Resources::default(), + triggers: Triggers::default(), + variables: Vec::new(), + body: PipelineBody::Jobs(vec![job]), + shape: PipelineShape::Standalone, + }; + let err = super::lower(&p).unwrap_err(); + let msg = format!("{err:#}"); + assert!( + msg.contains("EnvValue::RuntimeExpression"), + "error must point to the RuntimeExpression variant; got: {msg}" + ); + } + + #[test] + fn lower_job_explicit_variable_wins_over_runtime_expr_hoist() { + use crate::compile::ir::job::JobVariable; + + // An author-declared job variable that happens to share the + // hash-derived hoist name must never be clobbered by the hoist. + let body = "dependencies.Agent.result"; + let name = runtime_expr_var_name(body); + let step = Step::Bash( + BashStep::new("Conclusion", "echo hi") + .with_env("AGENT_RESULT", EnvValue::runtime_expression(body)), + ); + let mut job = Job::new( + JobId::new("Conclusion").unwrap(), + "Conclusion", + Pool::VmImage("u".into()), + ); + job.variables.push(JobVariable { + name: name.clone(), + value: EnvValue::literal("explicit-wins"), + }); + job.push_step(step); + let p = Pipeline { + name: "t".into(), + parameters: Vec::new(), + resources: Resources::default(), + triggers: Triggers::default(), + variables: Vec::new(), + body: PipelineBody::Jobs(vec![job]), + shape: PipelineShape::Standalone, + }; + let v = super::lower(&p).unwrap(); + let yaml = serde_yaml::to_string(&v).unwrap(); + // The explicit value is preserved; the hoist's `$[ ... ]` form + // never overwrites it. + assert!( + yaml.contains(&format!("{name}: explicit-wins")), + "explicit job variable must win over the hoist:\n{yaml}" + ); + assert!( + !yaml.contains(&format!("{name}: $[ {body} ]")), + "hoist must not clobber the explicit job variable:\n{yaml}" + ); + } + #[test] fn lower_condition_static_variants() { // Quick sanity that lower.rs threads the condition codegen From 358a6e2a73f79af7a8c32477df5cae83d7fd5167 Mon Sep 17 00:00:00 2001 From: James Devine Date: Thu, 18 Jun 2026 11:10:24 +0100 Subject: [PATCH 6/6] fix(ir): reject RawYamlScalar \$[ ] in Concat, add Task hoist test + body-injection note Addresses rust-review feedback on PR #1082: - reject_runtime_expr_in_concat now also rejects a RawYamlScalar(String) carrying a \$[ ... ] runtime expression, mirroring the top-level guard; previously such a scalar concatenated verbatim into the step env (the exact footgun being patched). - Add a test for the Concat RawYamlScalar rejection and a test that a RuntimeExpression in a TaskStep env is hoisted (parity with BashStep). - Document on EnvValue::runtime_expression that the body is interpolated verbatim into \$[ {body} ] (compiler-internal use; reject ] if untrusted text is ever routed through it). - Update docs/ir.md for the nested-Concat RawYamlScalar rejection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/ir.md | 2 +- src/compile/ir/env.rs | 7 ++++ src/compile/ir/lower.rs | 81 ++++++++++++++++++++++++++++++++++++++--- 3 files changed, 83 insertions(+), 7 deletions(-) diff --git a/docs/ir.md b/docs/ir.md index f368cdbe..c0405981 100644 --- a/docs/ir.md +++ b/docs/ir.md @@ -22,7 +22,7 @@ Those wrappers are the only place per-target shape (top-level `PipelineShape`, t - `tasks.rs` — typed factory helpers for built-in ADO tasks that return preconfigured `TaskStep` values with required inputs set. Prefer extending these helpers for compiler-generated tasks rather than open-coding `TaskStep::new(...)` with raw string keys at each call site. - `job.rs` — `Job`, `Pool`, job variables, 1ES `templateContext` support, and target-job external `dependsOn` / `condition` wrapping. - `stage.rs` — `Stage` plus target-stage external `dependsOn` / `condition` wrapping. -- `env.rs` — typed environment values (`EnvValue`) including ADO macros, pipeline variables, secrets, `OutputRef`s, `Coalesce`, macro-form `Concat`, and `RuntimeExpression` (a `$[ ... ]` ADO runtime expression that the lowering pass auto-hoists to a job-level `variables:` entry — ADO does not evaluate `$[ ... ]` inside step `env:`). `RuntimeExpression` is only valid at the top level of a step `env:` value: nesting it inside `Concat` or `Coalesce` is rejected at lower time (the hoist pass walks only the top level, so a nested occurrence would emit a dangling `$(AwRtExpr_…)` macro). A `Literal` or `RawYamlScalar(String)` smuggling a raw `$[ ... ]` into a step `env:` value is likewise rejected at lower time — ADO passes such scalars verbatim, so the typed `RuntimeExpression` variant must be used instead. +- `env.rs` — typed environment values (`EnvValue`) including ADO macros, pipeline variables, secrets, `OutputRef`s, `Coalesce`, macro-form `Concat`, and `RuntimeExpression` (a `$[ ... ]` ADO runtime expression that the lowering pass auto-hoists to a job-level `variables:` entry — ADO does not evaluate `$[ ... ]` inside step `env:`). `RuntimeExpression` is only valid at the top level of a step `env:` value: nesting it inside `Concat` or `Coalesce` is rejected at lower time (the hoist pass walks only the top level, so a nested occurrence would emit a dangling `$(AwRtExpr_…)` macro). A `Literal` or `RawYamlScalar(String)` smuggling a raw `$[ ... ]` into a step `env:` value (whether at the top level or nested inside a `Concat`) is likewise rejected at lower time — ADO passes such scalars verbatim, so the typed `RuntimeExpression` variant must be used instead. - `condition.rs` — the `Condition` / `Expr` AST and code generation to ADO condition syntax. - `output.rs` — `OutputDecl`, `OutputRef`, and the output-reference lowering rules. - `graph.rs` — graph construction, `dependsOn` derivation, output validation, `isOutput=true` promotion, and cycle detection. diff --git a/src/compile/ir/env.rs b/src/compile/ir/env.rs index ccefaa69..aef0f080 100644 --- a/src/compile/ir/env.rs +++ b/src/compile/ir/env.rs @@ -243,6 +243,13 @@ impl EnvValue { /// lowering pass hoists it to a job-level `variables:` entry and /// emits a `$(name)` macro reference, because ADO does not /// evaluate `$[ ... ]` inside step `env:`. + /// + /// **Body is interpolated verbatim.** At hoist time the lowering + /// pass wraps `body` as `$[ {body} ]` without escaping, so this + /// constructor is intended for compiler-internal expressions only. + /// A `body` containing `]` would prematurely close the wrapper and + /// produce a malformed expression. If untrusted/user text is ever + /// routed here, validate it (reject `]`) before constructing. pub fn runtime_expression(body: impl Into) -> Self { EnvValue::RuntimeExpression(body.into()) } diff --git a/src/compile/ir/lower.rs b/src/compile/ir/lower.rs index 04424424..159e1196 100644 --- a/src/compile/ir/lower.rs +++ b/src/compile/ir/lower.rs @@ -1296,17 +1296,19 @@ fn s(v: impl Into) -> Value { } /// Reject an [`EnvValue::RuntimeExpression`] (or a [`EnvValue::Literal`] -/// smuggling a `$[ ... ]` runtime expression) nested inside an -/// [`EnvValue::Concat`]. +/// or [`EnvValue::RawYamlScalar`] smuggling a `$[ ... ]` runtime +/// expression) nested inside an [`EnvValue::Concat`]. /// /// `Concat` is macro-form concatenation: its children lower verbatim /// into the step `env:` scalar (see [`lower_env_value`]). A /// `RuntimeExpression` child would emit a `$(AwRtExpr_)` macro /// whose backing job variable is never hoisted (the hoist pass walks -/// only the top level of each step env), and a literal `$[ ... ]` would -/// be passed verbatim — both leaving a runtime expression that ADO does -/// not evaluate in step env. Mirrors the `RuntimeExpression`-in-`Coalesce` -/// rejection in [`lower_env_value_as_expr_atom`]. Nested `Concat` children +/// only the top level of each step env), and a literal `$[ ... ]` (in a +/// `Literal` or a `RawYamlScalar(String)`) would be passed verbatim — +/// both leaving a runtime expression that ADO does not evaluate in step +/// env. Mirrors the `RuntimeExpression`-in-`Coalesce` rejection in +/// [`lower_env_value_as_expr_atom`] and the top-level guard in +/// [`reject_runtime_expr_literal_in_step_env`]. Nested `Concat` children /// are re-checked via the recursion in [`lower_env_value`]. fn reject_runtime_expr_in_concat(v: &EnvValue) -> Result<()> { match v { @@ -1323,6 +1325,12 @@ fn reject_runtime_expr_in_concat(v: &EnvValue) -> Result<()> { inside step env (the literal string is passed verbatim). Use \ EnvValue::RuntimeExpression at the top level of a step env value instead." ), + EnvValue::RawYamlScalar(Value::String(lit)) if lit.contains("$[") => anyhow::bail!( + "ir::lower: EnvValue::RawYamlScalar ({lit:?}) inside an EnvValue::Concat carries \ + a `$[ ... ]` ADO runtime expression. ADO does NOT evaluate runtime expressions \ + inside step env (the scalar string is passed verbatim). Use \ + EnvValue::RuntimeExpression at the top level of a step env value instead." + ), _ => Ok(()), } } @@ -1421,6 +1429,29 @@ mod tests { ); } + /// A `RawYamlScalar(String)` carrying `$[ ... ]` nested inside a + /// `Concat` is rejected too — the scalar would otherwise concatenate + /// verbatim into the step env (the same footgun the top-level guard + /// catches). + #[test] + fn lower_env_value_dollar_bracket_raw_yaml_scalar_inside_concat_errors() { + let g = Graph::default(); + let job = JobId::new("J").unwrap(); + let ctx = ctx_for(&g, &job); + let v = EnvValue::concat(vec![ + EnvValue::literal("prefix-"), + EnvValue::RawYamlScalar(serde_yaml::Value::String( + "$[ dependencies.Agent.result ]".into(), + )), + ]); + let err = lower_env_value(&ctx, &v).unwrap_err(); + let msg = format!("{err:#}"); + assert!( + msg.contains("EnvValue::RawYamlScalar") && msg.contains("EnvValue::Concat"), + "expected $[-RawYamlScalar-in-Concat error, got: {msg}" + ); + } + /// A `RuntimeExpression` nested inside a `Concat` inside another /// `Concat` is also rejected — the recursion in `lower_env_value` /// re-checks each nested level. @@ -1488,6 +1519,44 @@ mod tests { ); } + #[test] + fn lower_job_hoists_runtime_expression_from_task_step_env() { + use crate::compile::ir::step::TaskStep; + + // `collect_runtime_expr_hoists` walks Task step env identically + // to Bash — a RuntimeExpression in a TaskStep env must hoist too. + let body = "dependencies.Agent.result"; + let mut task = TaskStep::new("Bash@3", "Run"); + task.env + .insert("AGENT_RESULT".into(), EnvValue::runtime_expression(body)); + let mut job = Job::new(JobId::new("J").unwrap(), "J", Pool::VmImage("u".into())); + job.push_step(Step::Task(task)); + let p = Pipeline { + name: "t".into(), + parameters: Vec::new(), + resources: Resources::default(), + triggers: Triggers::default(), + variables: Vec::new(), + body: PipelineBody::Jobs(vec![job]), + shape: PipelineShape::Standalone, + }; + let v = super::lower(&p).unwrap(); + let yaml = serde_yaml::to_string(&v).unwrap(); + let name = runtime_expr_var_name(body); + assert!( + yaml.contains(&format!("{name}: $[ {body} ]")), + "expected hoisted variable `{name}: $[ {body} ]` in:\n{yaml}" + ); + assert!( + yaml.contains(&format!("AGENT_RESULT: $({name})")), + "expected task step env `AGENT_RESULT: $({name})` in:\n{yaml}" + ); + assert!( + !yaml.contains("AGENT_RESULT: $["), + "task step env must not carry a raw `$[ ... ]` expression:\n{yaml}" + ); + } + #[test] fn lower_job_deduplicates_identical_runtime_expressions() { let body = "dependencies.Agent.result";