From 814ac54a23ac1f57cf7979fdff8b83694fa81007 Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 10 Jun 2026 15:16:26 +0100 Subject: [PATCH] fix(compile): repair synthetic-PR gate + Stage step env var plumbing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two interlocking ADO behaviours were silently collapsing the synthetic-PR path on CI-triggered builds: 1. **Same-job gate step (Setup/prGate)**: env values used macro concatenation `$(System.PullRequest.PullRequestId)$(synthPr.AW_SYNTHETIC_PR_ID)`. On non-PR builds ADO leaves undefined predefined macros as the literal string `"$(System.PullRequest.PullRequestId)"`, so the concatenated value parsed as NaN in gate.js's pr_metadata fact — the check failed with "Missing ADO env vars …", pr_is_draft was skipped, and SHOULD_RUN defaulted to true (gate bypassed real evaluation). Switched to runtime expressions `$[ coalesce(variables['System.PullRequest.X'], variables['AW_SYNTHETIC_PR_X']) ]`, which silently substitute empty for undefined vars. To make the right-hand side readable as a regular variable, exec-context-pr-synth now emits each AW_SYNTHETIC_PR* via both setOutput (cross-job) AND setVar (same-job, new helper in shared/vso-logger). 2. **Cross-job Stage step (Agent/"Stage PR execution context")**: env values referenced `$[ coalesce(dependencies.Setup.outputs['synthPr.X'], '') ]` at step-env scope. Empirically (msazuresphere/4x4 build #612290) this form resolves to the empty string even when the same expression works at job-condition scope, causing the bash guard to short-circuit on a synth-promoted build and the agent to emit noop. Switched to the documented safe pattern: hoist via a new `{{ agent_job_variables }}` marker (Agent-job-level `variables:` block) that pulls `dependencies.Setup.outputs[...]` at JOB scope, then consume from step-env via the `$(name)` macro / `variables['name']` runtime expression. Job-level variables are the documented safe location for cross-job output references. Validates against build #612290 (synthetic PR matched #38551 but agent emitted noop). All 281 bundle tests + 1815+ Rust tests + clippy pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/template-markers.md | 13 +++ .../__tests__/index.test.ts | 17 +++ .../src/exec-context-pr-synth/index.ts | 30 ++++- scripts/ado-script/src/shared/vso-logger.ts | 20 ++++ src/compile/common.rs | 87 +++++++++++++++ src/compile/extensions/exec_context/pr.rs | 101 +++++++++-------- src/compile/filter_ir.rs | 89 +++++++++------ src/data/1es-base.yml | 1 + src/data/base.yml | 1 + src/data/job-base.yml | 1 + src/data/stage-base.yml | 1 + tests/compiler_tests.rs | 105 +++++++++++++----- 12 files changed, 349 insertions(+), 117 deletions(-) diff --git a/docs/template-markers.md b/docs/template-markers.md index f4093946..9ae2849a 100644 --- a/docs/template-markers.md +++ b/docs/template-markers.md @@ -294,6 +294,19 @@ Generates a `timeoutInMinutes: ` job property for `Agent` when `engine.ti If `timeout-minutes` is not configured, this is replaced with an empty string. +## {{ agent_job_variables }} + +Generates the Agent job's `variables:` block. Currently emits content **only** when synthetic-PR-from-CI is active (`on.pr.mode == Synthetic`); replaced with an empty string otherwise. + +When active, this hoists the relevant `synthPr` Setup-job step outputs into Agent-job-level variables using `$[ coalesce(dependencies.Setup.outputs['synthPr.X'], '') ]` runtime expressions: + +- `AW_SYNTHETIC_PR` +- `AW_SYNTHETIC_PR_ID` +- `AW_SYNTHETIC_PR_TARGETBRANCH` +- `AW_SYNTHETIC_PR_SOURCEBRANCH` + +The hoist exists because `dependencies..outputs[...]` references at step-level `env:` scope proved unreliable in practice (empirically observed in `msazuresphere/4x4` build #612290: the same reference resolved correctly at job-condition scope but returned the empty string at step-env scope, causing the `Stage PR execution context` step's bash guard to misfire and the agent to emit `noop` on a synth-promoted build). Job-level `variables:` is the documented safe location for cross-job output references; subsequent step `env:` blocks then consume the hoisted values via the `$(name)` macro or a `$[ coalesce(variables['name'], ...) ]` runtime expression. + ## {{ working_directory }} Should be replaced with the appropriate working directory based on the effective workspace setting. diff --git a/scripts/ado-script/src/exec-context-pr-synth/__tests__/index.test.ts b/scripts/ado-script/src/exec-context-pr-synth/__tests__/index.test.ts index 3e09f323..a03871b2 100644 --- a/scripts/ado-script/src/exec-context-pr-synth/__tests__/index.test.ts +++ b/scripts/ado-script/src/exec-context-pr-synth/__tests__/index.test.ts @@ -138,11 +138,28 @@ describe("exec-context-pr-synth main", () => { const { code, output } = await runMain(makeEnv({ PR_SYNTH_SPEC: spec })); expect(code).toBe(0); expect(output).not.toContain("AW_SYNTHETIC_PR_SKIP"); + // Each AW_SYNTHETIC_PR* variable is emitted TWICE: once as an + // output (cross-job, consumed by the Agent job condition + the + // Agent-job-level `variables:` hoist) and once as a regular + // variable (same-job, consumed by the prGate step's env block + // via `$[ coalesce(variables['AW_SYNTHETIC_PR_X'], ...) ]`). + // See `setVar` in `shared/vso-logger.ts` for the rationale. expect(output).toContain("AW_SYNTHETIC_PR;isOutput=true]true"); expect(output).toContain("AW_SYNTHETIC_PR_ID;isOutput=true]1234"); expect(output).toContain("AW_SYNTHETIC_PR_TARGETBRANCH;isOutput=true]refs/heads/main"); expect(output).toContain("AW_SYNTHETIC_PR_SOURCEBRANCH;isOutput=true]refs/heads/feature/x"); expect(output).toContain("AW_SYNTHETIC_PR_IS_DRAFT;isOutput=true]false"); + // Regular-variable counterparts (no `isOutput`). Each line is a + // separate ##vso command terminated by `]value`. + expect(output).toContain("##vso[task.setvariable variable=AW_SYNTHETIC_PR]true"); + expect(output).toContain("##vso[task.setvariable variable=AW_SYNTHETIC_PR_ID]1234"); + expect(output).toContain( + "##vso[task.setvariable variable=AW_SYNTHETIC_PR_TARGETBRANCH]refs/heads/main", + ); + expect(output).toContain( + "##vso[task.setvariable variable=AW_SYNTHETIC_PR_SOURCEBRANCH]refs/heads/feature/x", + ); + expect(output).toContain("##vso[task.setvariable variable=AW_SYNTHETIC_PR_IS_DRAFT]false"); }); it("emits AW_SYNTHETIC_PR_IS_DRAFT=true when the PR is a draft", async () => { diff --git a/scripts/ado-script/src/exec-context-pr-synth/index.ts b/scripts/ado-script/src/exec-context-pr-synth/index.ts index 0a3b70f9..d45afe43 100644 --- a/scripts/ado-script/src/exec-context-pr-synth/index.ts +++ b/scripts/ado-script/src/exec-context-pr-synth/index.ts @@ -38,7 +38,7 @@ import { getPullRequestIterations, listActivePullRequestsBySourceRef, } from "../shared/ado-client.js"; -import { logError, logInfo, setOutput } from "../shared/vso-logger.js"; +import { logError, logInfo, setOutput, setVar } from "../shared/vso-logger.js"; import { matchesIncludeExclude, normalisePath, pathMatchesIncludeExclude } from "./match.js"; import { decodeSpec, type PrSynthSpec } from "./spec.js"; @@ -189,11 +189,29 @@ export async function main(env: NodeJS.ProcessEnv = process.env): Promise. - setOutput("AW_SYNTHETIC_PR", "true"); - setOutput("AW_SYNTHETIC_PR_ID", String(prId)); - setOutput("AW_SYNTHETIC_PR_TARGETBRANCH", pr.targetRefName ?? ""); - setOutput("AW_SYNTHETIC_PR_SOURCEBRANCH", pr.sourceRefName ?? sourceBranch); - setOutput("AW_SYNTHETIC_PR_IS_DRAFT", pr.isDraft === true ? "true" : "false"); + // Emit each AW_SYNTHETIC_PR* value TWICE: once as an output variable + // (visible cross-job via `dependencies.Setup.outputs['synthPr.X']` to + // the Agent job's condition + variables block) and once as a regular + // pipeline variable (visible same-job via `$(X)` macro and + // `$[ variables['X'] ]` runtime expression to the gate step that + // runs immediately after this one in the Setup job). + // + // The dual emission is required because `isOutput=true` alone does NOT + // register the variable in the producing job's regular variable + // namespace — same-job consumers (the `prGate` step in particular) + // would otherwise see empty values and the synth promotion would + // collapse silently. See `setVar` doc-comment in `shared/vso-logger.ts` + // for the underlying ADO contract. + const emitBoth = (name: string, value: string): void => { + setOutput(name, value); + setVar(name, value); + }; + + emitBoth("AW_SYNTHETIC_PR", "true"); + emitBoth("AW_SYNTHETIC_PR_ID", String(prId)); + emitBoth("AW_SYNTHETIC_PR_TARGETBRANCH", pr.targetRefName ?? ""); + emitBoth("AW_SYNTHETIC_PR_SOURCEBRANCH", pr.sourceRefName ?? sourceBranch); + emitBoth("AW_SYNTHETIC_PR_IS_DRAFT", pr.isDraft === true ? "true" : "false"); logInfo( `[synth-pr] matched PR #${prId} (source=${pr.sourceRefName} target=${pr.targetRefName})`, diff --git a/scripts/ado-script/src/shared/vso-logger.ts b/scripts/ado-script/src/shared/vso-logger.ts index 96c0196a..064b5702 100644 --- a/scripts/ado-script/src/shared/vso-logger.ts +++ b/scripts/ado-script/src/shared/vso-logger.ts @@ -51,6 +51,26 @@ export function setOutput(name: string, value: string): void { emit(`##vso[task.setvariable variable=${safeName};isOutput=true]${safeValue}`); } +/** + * Set a regular (non-output) pipeline variable, visible to subsequent + * steps in the **same job** via `$(name)` macro or + * `$[ variables['name'] ]` runtime expression. + * + * `isOutput=true` (used by `setOutput`) makes a variable available to + * downstream JOBS via `dependencies..outputs['.']`, + * but does NOT register it in the job's regular variable namespace — + * so `$(name)` and `$[ variables['name'] ]` resolve to empty in + * same-job consumers. Callers that need both same-job AND cross-job + * access must call BOTH `setVar` (same-job) and `setOutput` (cross-job). + * + * See . + */ +export function setVar(name: string, value: string): void { + const safeName = escapeProperty(name); + const safeValue = escapeMessage(value); + emit(`##vso[task.setvariable variable=${safeName}]${safeValue}`); +} + export function addBuildTag(tag: string): void { emit(`##vso[build.addbuildtag]${escapeMessage(tag)}`); } diff --git a/src/compile/common.rs b/src/compile/common.rs index 965c90df..6088a4ee 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -1049,6 +1049,53 @@ pub fn generate_job_timeout(front_matter: &FrontMatter) -> String { } } +/// Generate the Agent job's `variables:` block. +/// +/// Currently emits content **only** when synthetic-PR-from-CI is active +/// (`on.pr.mode == Synthetic`). In that mode we need to surface the +/// `synthPr` Setup-job step outputs to consumers in the Agent job +/// (today: the `Stage PR execution context` bash step in +/// `exec_context/pr.rs`). +/// +/// **Why job-level variables and not step-level env**: the canonical +/// ADO pattern for forwarding a cross-job step output is to declare a +/// job-level variable using a runtime expression `$[ ... ]`, then +/// consume that variable from step `env:` blocks via the `$(name)` +/// macro. Putting `$[ dependencies..outputs[...] ]` directly in +/// step-level `env:` is technically documented as supported but has +/// proven unreliable in practice — empirical evidence from +/// msazuresphere/4x4 build #612290 showed +/// `dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR']` resolving to +/// the empty string when referenced from a step's `env:` even though +/// the same expression worked in the Agent job's `condition:`. The +/// job-level form is the documented safe location: +/// . +/// +/// When this hoist is empty (the agent isn't using synthetic-PR-from-CI), +/// the marker collapses cleanly: the surrounding template indents the +/// marker on its own line and an empty replacement leaves no stray +/// keys at job scope. +pub fn generate_agent_job_variables(synthetic_pr_active: bool) -> String { + if !synthetic_pr_active { + return String::new(); + } + // The base indent on these continuation lines is just 2 spaces — + // `replace_with_indent` prepends the marker line's own indent to + // each subsequent line, so the keys here only need 2 extra spaces + // to land as proper children of `variables:` (which itself lands + // at the marker's column, the same column as `dependsOn:` / + // `pool:` on the Agent job). The same offset works for every + // base template (base.yml, 1es-base.yml, job-base.yml, stage-base.yml) + // because YAML child-indent is measured relative to the parent + // mapping key, not absolutely. + // + // `coalesce(..., '')` ensures the variable is the empty string + // rather than the unresolved literal `$[ ... ]` form if the + // dependency cannot be resolved (e.g. Setup was skipped or the + // synthPr step did not run). + "variables:\n AW_SYNTHETIC_PR: $[ coalesce(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], '') ]\n AW_SYNTHETIC_PR_ID: $[ coalesce(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_ID'], '') ]\n AW_SYNTHETIC_PR_TARGETBRANCH: $[ coalesce(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_TARGETBRANCH'], '') ]\n AW_SYNTHETIC_PR_SOURCEBRANCH: $[ coalesce(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_SOURCEBRANCH'], '') ]".to_string() +} + /// Format a single step's YAML string with proper indentation #[allow(dead_code)] pub fn format_step_yaml(step_yaml: &str) -> String { @@ -3386,6 +3433,7 @@ pub async fn compile_shared( synthetic_pr_active, ); let job_timeout = generate_job_timeout(front_matter); + let agent_job_variables = generate_agent_job_variables(synthetic_pr_active); // 9. Token acquisition and env vars let acquire_read_token = generate_acquire_ado_token( @@ -3560,6 +3608,7 @@ pub async fn compile_shared( ("{{ finalize_steps }}", &finalize_steps), ("{{ agentic_depends_on }}", &agentic_depends_on), ("{{ job_timeout }}", &job_timeout), + ("{{ agent_job_variables }}", &agent_job_variables), ("{{ repositories }}", &repositories), ("{{ schedule }}", &schedule), ("{{ pipeline_resources }}", &pipeline_resources), @@ -3707,6 +3756,44 @@ mod tests { fm } + // ─── generate_agent_job_variables ───────────────────────────────── + + #[test] + fn test_generate_agent_job_variables_empty_when_synth_inactive() { + assert_eq!(generate_agent_job_variables(false), ""); + } + + #[test] + fn test_generate_agent_job_variables_emits_hoisted_synth_outputs() { + let out = generate_agent_job_variables(true); + // The hoist must declare a `variables:` mapping at the Agent + // job level (the `{{ agent_job_variables }}` marker sits at the + // job-keys indent). + assert!( + out.starts_with("variables:"), + "must declare a `variables:` block: {out}" + ); + // Each AW_SYNTHETIC_PR* output that downstream consumers need + // must be hoisted via `$[ coalesce(dependencies.Setup.outputs[...], '') ]`. + // The `coalesce(..., '')` guarantees the variable is the empty + // string (rather than the literal `$[ ... ]` form) when the + // dependency is unresolved (e.g. Setup skipped). + for name in &[ + "AW_SYNTHETIC_PR", + "AW_SYNTHETIC_PR_ID", + "AW_SYNTHETIC_PR_TARGETBRANCH", + "AW_SYNTHETIC_PR_SOURCEBRANCH", + ] { + let needle = format!( + "{name}: $[ coalesce(dependencies.Setup.outputs['synthPr.{name}'], '') ]" + ); + assert!( + out.contains(&needle), + "must hoist {name} from cross-job synth output: {out}" + ); + } + } + // ─── atomic_write ───────────────────────────────────────────────────────── #[tokio::test] diff --git a/src/compile/extensions/exec_context/pr.rs b/src/compile/extensions/exec_context/pr.rs index e1b1cb61..9713869c 100644 --- a/src/compile/extensions/exec_context/pr.rs +++ b/src/compile/extensions/exec_context/pr.rs @@ -122,30 +122,24 @@ impl ContextContributor for PrContextContributor { // the spawned `git` subprocess via `GIT_CONFIG_*` env vars // (never argv). It is NEVER visible to the agent step. // - // When `mode: synthetic` is on, the PR-identifier env vars - // are emitted using `$[ coalesce(...) ]` so the bundle picks - // up either the real `System.PullRequest.*` (on a true PR - // build) OR the synthPr Setup-job output (on a CI build - // promoted via exec-context-pr-synth.js). + // When `mode: synthetic` is on, PR-identifier env vars are + // coalesced via `$[ ... ]` runtime expressions reading the + // **Agent-job-level** variables that `generate_agent_job_variables` + // hoists from `dependencies.Setup.outputs['synthPr.*']`. We + // deliberately do NOT put `dependencies.Setup.outputs[...]` + // directly in step-level `env:` here — that combination has + // proven unreliable in practice (msazuresphere/4x4 build + // #612290: the same reference resolved correctly at + // job-condition scope but returned empty at step-env scope, + // causing the bash gate to short-circuit on a synth-promoted + // build and the agent to emit `noop`). The job-variable hoist + // is the documented safe location for cross-job output + // references: . // - // Cross-job reference is correct here: this step runs in the - // **Agent** job (which depends on Setup), so - // `dependencies.Setup.outputs['synthPr.X']` resolves at runtime. - // (Same-job references would need `variables['synthPr.X']` - // instead — used by the gate step inside the Setup job itself.) - // Runtime expressions `$[ ... ]` are documented as valid in - // step-level `env:` blocks; see - // . // `System.PullRequest.TargetBranch` is `refs/heads/` (full - // ref form), matching the `targetRefName` shape returned by the - // ADO REST API and stored in `AW_SYNTHETIC_PR_TARGETBRANCH`, so - // the coalesce yields a consistent value either way. - // `$[ ... ]` runtime expressions are wrapped in YAML double - // quotes because their values contain single quotes (e.g. - // `variables['System.PullRequest.PullRequestId']`). ADO accepts - // them unquoted in practice, but double-quoting matches the - // form shown in ADO docs and is strictly conformant to the - // YAML spec (which reserves `'` as a scalar indicator). + // ref form), matching the `targetRefName` shape stashed in + // `AW_SYNTHETIC_PR_TARGETBRANCH`, so the coalesce yields a + // consistent value either way. // // ## Synth-active gating — bash, not step `condition:` // @@ -159,24 +153,25 @@ impl ContextContributor for PrContextContributor { // // We therefore project the synth flag through the same // step-level `env:` indirection used for the PR id / target - // branch and gate in the bash body. The step still emits as - // `succeeded` in the ADO UI on non-PR / non-synth builds - // (with a single skip log line) rather than as `skipped` — - // a minor cosmetic cost for avoiding a cross-cutting - // template / trait change. + // branch (reading the hoisted job-level variable, not the + // raw cross-job dependency) and gate in the bash body. The + // step still emits as `succeeded` in the ADO UI on + // non-PR / non-synth builds (with a single skip log line) + // rather than as `skipped` — a minor cosmetic cost for + // avoiding a cross-cutting template / trait change. // // The synth-INACTIVE branch is unchanged: its // `condition: eq(variables['Build.Reason'], 'PullRequest')` // only reads `variables[...]`, which IS legal at step level. let (pr_id_macro, target_branch_macro, prelude, condition) = if self.synthetic_pr_active { ( - "\"$[ coalesce(variables['System.PullRequest.PullRequestId'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_ID']) ]\"", - "\"$[ coalesce(variables['System.PullRequest.TargetBranch'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_TARGETBRANCH']) ]\"", - // Bash gate. `coalesce(..., '')` on the env-var side - // ensures `$AW_SYNTHETIC_PR` is `""` (not an unresolved - // `$()` literal) when Setup's `synthPr` step skipped - // itself on a real-PR build. Both var refs are quoted - // for shellcheck; both args to `[` are literal-string + "\"$[ coalesce(variables['System.PullRequest.PullRequestId'], variables['AW_SYNTHETIC_PR_ID']) ]\"", + "\"$[ coalesce(variables['System.PullRequest.TargetBranch'], variables['AW_SYNTHETIC_PR_TARGETBRANCH']) ]\"", + // Bash gate. `$AW_SYNTHETIC_PR` reads the hoisted + // job-level variable via the step-env `$(...)` macro + // below, which is "true" on a synth-promoted build, + // "" otherwise. Both var refs are quoted for + // shellcheck; both args to `[` are literal-string // comparisons so `set -u` is safe on either side. " if [ \"$BUILD_REASON\" != \"PullRequest\" ] && [ \"$AW_SYNTHETIC_PR\" != \"true\" ]; then\n echo \"[aw-context] Not a PR build and not synth-promoted; skipping exec-context-pr.\"\n exit 0\n fi\n", "succeeded()", @@ -190,12 +185,14 @@ impl ContextContributor for PrContextContributor { ) }; // Synth-active path adds two env vars (BUILD_REASON + the - // coalesced synth flag) the bash prelude reads. They're - // omitted on the synth-inactive path because that path's step - // condition (a plain `variables[...]` ref) is legal at step - // level and needs no in-bash gate. + // synth flag) the bash prelude reads. `AW_SYNTHETIC_PR` is + // pulled via the `$(name)` macro from the Agent-job-level + // `variables:` block (see `generate_agent_job_variables` in + // `compile/common.rs`) — NOT directly from + // `dependencies.Setup.outputs[...]`, see the doc-comment on + // this function for why. let synth_env = if self.synthetic_pr_active { - "\n BUILD_REASON: $(Build.Reason)\n AW_SYNTHETIC_PR: \"$[ coalesce(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], '') ]\"" + "\n BUILD_REASON: $(Build.Reason)\n AW_SYNTHETIC_PR: $(AW_SYNTHETIC_PR)" } else { "" }; @@ -272,30 +269,36 @@ mod tests { // form here — distinct from the gate step which is same-job). assert!( step.contains( - "SYSTEM_PULLREQUEST_PULLREQUESTID: \"$[ coalesce(variables['System.PullRequest.PullRequestId'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_ID']) ]\"" + "SYSTEM_PULLREQUEST_PULLREQUESTID: \"$[ coalesce(variables['System.PullRequest.PullRequestId'], variables['AW_SYNTHETIC_PR_ID']) ]\"" ), - "synth-active prepare step must coalesce PR id with synthPr fallback: {step}" + "synth-active prepare step must coalesce PR id with the hoisted job-level AW_SYNTHETIC_PR_ID variable: {step}" ); assert!( step.contains( - "SYSTEM_PULLREQUEST_TARGETBRANCH: \"$[ coalesce(variables['System.PullRequest.TargetBranch'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_TARGETBRANCH']) ]\"" + "SYSTEM_PULLREQUEST_TARGETBRANCH: \"$[ coalesce(variables['System.PullRequest.TargetBranch'], variables['AW_SYNTHETIC_PR_TARGETBRANCH']) ]\"" ), - "synth-active prepare step must coalesce target branch with synthPr fallback: {step}" + "synth-active prepare step must coalesce target branch with the hoisted job-level AW_SYNTHETIC_PR_TARGETBRANCH variable: {step}" ); // Env: BUILD_REASON + synth flag projected through env so the // bash gate has plain `$BUILD_REASON` / `$AW_SYNTHETIC_PR` to - // read (cross-job refs are illegal in step `condition:` but - // legal in step `env:` values). + // read. `AW_SYNTHETIC_PR` is read via the same-job macro from + // the Agent-job-level `variables:` block (hoisted from + // `dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR']` in + // `generate_agent_job_variables`), NOT the raw cross-job + // dependency reference at step-env scope (that combination + // proved unreliable in msazuresphere/4x4 build #612290). assert!( step.contains("BUILD_REASON: $(Build.Reason)"), "synth-active prepare step must project Build.Reason through env for the bash guard: {step}" ); assert!( - step.contains( - "AW_SYNTHETIC_PR: \"$[ coalesce(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], '') ]\"" - ), - "synth-active prepare step must project the synth flag through env (coalesced to '' on real-PR builds): {step}" + step.contains("AW_SYNTHETIC_PR: $(AW_SYNTHETIC_PR)"), + "synth-active prepare step must pull AW_SYNTHETIC_PR from the Agent-job-level hoisted variable via the $(...) macro: {step}" + ); + assert!( + !step.contains("dependencies.Setup.outputs['synthPr."), + "synth-active prepare step must NOT reference `dependencies.Setup.outputs[...]` directly at step-env scope — that combination is unreliable; use the hoisted Agent-job-level variables instead: {step}" ); // Bash guard: literal `if` chain that exits 0 when neither diff --git a/src/compile/filter_ir.rs b/src/compile/filter_ir.rs index a43f6caa..f7d53bc4 100644 --- a/src/compile/filter_ir.rs +++ b/src/compile/filter_ir.rs @@ -1123,26 +1123,38 @@ pub fn build_gate_spec(ctx: GateContext, checks: &[FilterCheck]) -> anyhow::Resu /// PR-identifier env vars (`ADO_PR_ID`, `ADO_SOURCE_BRANCH`, /// `ADO_TARGET_BRANCH`) are emitted using `$[ coalesce(...) ]` so they /// pick up either the real `System.PullRequest.*` variables (on a true -/// PR build) OR the `synthPr` Setup-job outputs (on a CI build promoted -/// by `exec-context-pr-synth.js`). Also exports `AW_SYNTHETIC_PR` so -/// `gate/bypass.ts` knows to skip the "not a PR build" bypass. +/// PR build) OR the regular pipeline variables emitted by the `synthPr` +/// Setup-job step on a CI build promoted by `exec-context-pr-synth.js`. +/// Also exports `AW_SYNTHETIC_PR` so `gate/bypass.ts` knows to skip the +/// "not a PR build" bypass. /// -/// **Same-job vs cross-job reference**: this gate step lives in the -/// **Setup job** (`AdoScriptExtension::setup_steps` returns it), the -/// same job as `synthPr`. Within the producing job, the cross-job form -/// `dependencies.Setup.outputs['synthPr.X']` is undefined (a job has no -/// entry for itself in `dependencies`), and — critically — the same-job -/// *runtime expression* form `$[ variables['synthPr.X'] ]` also resolves -/// to **empty**: step output variables are NOT exposed to runtime -/// expressions in the producing job. They are only reachable via the -/// **macro** form `$(synthPr.X)`, which expands them from the job's -/// output-variable namespace (empty when the producing step was skipped -/// or never set the output). We therefore reference synth outputs with -/// macros and rely on macro concatenation: real `System.PullRequest.*` -/// and synth `synthPr.*` are mutually exclusive (synthPr only runs on -/// non-PR builds; `System.PullRequest.*` is empty on non-PR builds), so -/// `$(System.PullRequest.X)$(synthPr.X)` yields exactly one non-empty -/// value. See +/// **Same-job synth references**: this gate step lives in the **Setup +/// job** (`AdoScriptExtension::setup_steps` returns it), the same job +/// as `synthPr`. Three ADO behaviours interact here: +/// +/// 1. The cross-job form `dependencies.Setup.outputs['synthPr.X']` is +/// undefined inside the producing job (a job has no entry for itself +/// in `dependencies`). +/// 2. Step output variables marked `isOutput=true` are NOT added to the +/// producing job's regular variable namespace, so `$(X)` and +/// `$[ variables['X'] ]` resolve to empty unless the producer ALSO +/// emits the same name as a regular (non-output) variable. +/// 3. The same-job macro `$(synthPr.X)` can sometimes expand for a bare +/// reference, but ADO leaves UNDEFINED predefined macros (e.g. +/// `$(System.PullRequest.PullRequestId)` on a non-PR build) as the +/// literal string `"$(System.PullRequest.PullRequestId)"`. Concatenating +/// `$(System.PullRequest.X)$(synthPr.AW_SYNTHETIC_PR_X)` therefore +/// yields `"$(System.PullRequest.X)"` on non-PR builds — which +/// `gate/facts.ts` then parses as `NaN` and fails with +/// `Missing ADO env vars …`. +/// +/// The fix is to emit the synth values as BOTH output variables (for +/// cross-job consumers like the Agent job condition) AND regular +/// variables (this happens in `exec-context-pr-synth/index.ts` via +/// `setVar`), and then have THIS step coalesce in a runtime expression +/// `$[ ... ]`, which silently substitutes empty for undefined predefined +/// variables and produces a clean numeric/string result on either path. +/// See /// . pub fn compile_gate_step_external( ctx: GateContext, @@ -1178,36 +1190,39 @@ pub fn compile_gate_step_external( // has been promoted to PR semantics, so the "not a PullRequest // build" bypass must not auto-pass. Always safe to emit (the gate // checks it strictly for the literal "true"), but only meaningful - // for PR gates. Same-job ref via the macro `$(synthPr.X)` — see - // function doc-comment for why this is NOT `variables['synthPr.X']` - // (resolves empty) nor `dependencies.Setup.outputs[...]` (undefined - // in the producing job). + // for PR gates. Same-job ref via the runtime expression + // `$[ coalesce(variables['AW_SYNTHETIC_PR'], '') ]` — this reads + // the **regular** (non-output) variable that `exec-context-pr-synth` + // emits via `setVar` in addition to its cross-job `setOutput`. The + // step output namespace `synthPr.X` is unreachable from runtime + // expressions in the producing job (see function doc-comment). let pr_synth_active = synthetic_pr_active && matches!(ctx, GateContext::PullRequest); if pr_synth_active { - // Macro form: `$(synthPr.AW_SYNTHETIC_PR)` expands to "true" when - // the same-job `synthPr` step matched a PR, and to empty when it - // was skipped (real PR build) or found no PR — never the literal - // "true", so `gate/bypass.ts`'s strict `=== "true"` check stays - // correct in every case. - step.push_str(" AW_SYNTHETIC_PR: \"$(synthPr.AW_SYNTHETIC_PR)\"\n"); + step.push_str( + " AW_SYNTHETIC_PR: \"$[ coalesce(variables['AW_SYNTHETIC_PR'], '') ]\"\n", + ); } for (env_var, ado_macro) in &exports { let macro_str = if pr_synth_active { - // Mutually-exclusive macro concatenation: on a real PR build - // `System.PullRequest.*` holds the value and `synthPr.*` is - // empty (step skipped); on a synth-promoted CI build - // `System.PullRequest.*` is empty and `synthPr.*` holds the - // value. Exactly one side is ever non-empty. + // Mutually-exclusive runtime-expression coalesce: on a real + // PR build `System.PullRequest.X` is populated and the synth + // step is skipped (so `variables['AW_SYNTHETIC_PR_X']` is + // empty); on a synth-promoted CI build `System.PullRequest.X` + // is empty/undefined and `variables['AW_SYNTHETIC_PR_X']` + // holds the value. `coalesce(...)` returns the first + // non-empty argument as a clean string — undefined predefined + // variables substitute to empty rather than the literal + // `$(...)` form that macro concatenation would produce. match *env_var { "ADO_PR_ID" => { - "\"$(System.PullRequest.PullRequestId)$(synthPr.AW_SYNTHETIC_PR_ID)\"" + "\"$[ coalesce(variables['System.PullRequest.PullRequestId'], variables['AW_SYNTHETIC_PR_ID']) ]\"" } "ADO_SOURCE_BRANCH" => { - "\"$(System.PullRequest.SourceBranch)$(synthPr.AW_SYNTHETIC_PR_SOURCEBRANCH)\"" + "\"$[ coalesce(variables['System.PullRequest.SourceBranch'], variables['AW_SYNTHETIC_PR_SOURCEBRANCH']) ]\"" } "ADO_TARGET_BRANCH" => { - "\"$(System.PullRequest.TargetBranch)$(synthPr.AW_SYNTHETIC_PR_TARGETBRANCH)\"" + "\"$[ coalesce(variables['System.PullRequest.TargetBranch'], variables['AW_SYNTHETIC_PR_TARGETBRANCH']) ]\"" } _ => ado_macro, } diff --git a/src/data/1es-base.yml b/src/data/1es-base.yml index ed8735e3..84f7febf 100644 --- a/src/data/1es-base.yml +++ b/src/data/1es-base.yml @@ -42,6 +42,7 @@ extends: displayName: "Agent" {{ agentic_depends_on }} {{ job_timeout }} + {{ agent_job_variables }} templateContext: type: buildJob outputs: diff --git a/src/data/base.yml b/src/data/base.yml index f248619f..7ce076e7 100644 --- a/src/data/base.yml +++ b/src/data/base.yml @@ -19,6 +19,7 @@ jobs: displayName: "Agent" {{ agentic_depends_on }} {{ job_timeout }} + {{ agent_job_variables }} pool: {{ pool }} steps: diff --git a/src/data/job-base.yml b/src/data/job-base.yml index 12b7001d..4228bbbb 100644 --- a/src/data/job-base.yml +++ b/src/data/job-base.yml @@ -6,6 +6,7 @@ jobs: displayName: "Agent" {{ agentic_depends_on }} {{ job_timeout }} + {{ agent_job_variables }} pool: {{ pool }} steps: diff --git a/src/data/stage-base.yml b/src/data/stage-base.yml index 54276aa7..6f3e34ce 100644 --- a/src/data/stage-base.yml +++ b/src/data/stage-base.yml @@ -20,6 +20,7 @@ stages: displayName: "Agent" {{ agentic_depends_on }} {{ job_timeout }} + {{ agent_job_variables }} pool: {{ pool }} steps: diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index 959d7316..e1d393fc 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -4464,28 +4464,38 @@ fn test_pr_filter_synth_mode_gate_step_uses_same_job_synth_ref() { let compiled = compile_fixture("pr-filter-tier1-agent.md"); assert!( - compiled.contains("AW_SYNTHETIC_PR: \"$(synthPr.AW_SYNTHETIC_PR)\""), - "Gate step env must reference the same-job `synthPr` output via the macro \ - `$(synthPr.AW_SYNTHETIC_PR)` — the `$[ variables['synthPr.X'] ]` runtime \ - expression resolves to empty inside the producing Setup job" + compiled.contains( + "AW_SYNTHETIC_PR: \"$[ coalesce(variables['AW_SYNTHETIC_PR'], '') ]\"" + ), + "Gate step env must reference the same-job synthPr value via the runtime \ + expression `$[ coalesce(variables['AW_SYNTHETIC_PR'], '') ]` — \ + exec-context-pr-synth's `setVar` emits a regular pipeline variable \ + alongside the `isOutput=true` form, and runtime expressions read the \ + regular-variable namespace reliably (whereas `$(synthPr.X)` macros and \ + `$[ variables['synthPr.X'] ]` runtime expressions over the step-output \ + namespace are both unreliable inside the producing job)." ); // The fixture exercises source-branch and target-branch filters, // so the synth treatment must appear on those env vars using the - // mutually-exclusive macro-concatenation form. (ADO_PR_ID is not - // exported by this fixture's filter set, so we don't assert it here.) + // runtime-expression `$[ coalesce(...) ]` form over the regular + // `AW_SYNTHETIC_PR_X` variables. (ADO_PR_ID is not exported by + // this fixture's filter set, so we don't assert it here.) assert!( compiled.contains( - "ADO_SOURCE_BRANCH: \"$(System.PullRequest.SourceBranch)$(synthPr.AW_SYNTHETIC_PR_SOURCEBRANCH)\"" + "ADO_SOURCE_BRANCH: \"$[ coalesce(variables['System.PullRequest.SourceBranch'], variables['AW_SYNTHETIC_PR_SOURCEBRANCH']) ]\"" ), - "ADO_SOURCE_BRANCH must concatenate the real `System.PullRequest.*` macro \ - with the same-job `synthPr.*` macro" + "ADO_SOURCE_BRANCH must coalesce the real `System.PullRequest.SourceBranch` \ + variable with the same-job regular variable `AW_SYNTHETIC_PR_SOURCEBRANCH` \ + (emitted by exec-context-pr-synth via setVar). Macro concatenation \ + `$(System.PullRequest.X)$(synthPr.X)` produced garbage on non-PR builds \ + because ADO leaves undefined predefined macros as literal strings." ); assert!( compiled.contains( - "ADO_TARGET_BRANCH: \"$(System.PullRequest.TargetBranch)$(synthPr.AW_SYNTHETIC_PR_TARGETBRANCH)\"" + "ADO_TARGET_BRANCH: \"$[ coalesce(variables['System.PullRequest.TargetBranch'], variables['AW_SYNTHETIC_PR_TARGETBRANCH']) ]\"" ), - "ADO_TARGET_BRANCH must concatenate the real `System.PullRequest.*` macro \ - with the same-job `synthPr.*` macro" + "ADO_TARGET_BRANCH must coalesce the real `System.PullRequest.TargetBranch` \ + variable with the same-job regular variable `AW_SYNTHETIC_PR_TARGETBRANCH`." ); // The same-job gate step MUST NOT use the broken same-job runtime @@ -5357,10 +5367,40 @@ fn test_execution_context_pr_emits_prepare_step_and_prompt_supplement() { "Prepare step must include the bash gate that replaces the (illegal) cross-job step condition (mode: synthetic is the default)" ); assert!( - compiled.contains( - "AW_SYNTHETIC_PR: \"$[ coalesce(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], '') ]\"" - ), - "Prepare step must project the synth flag through env (coalesced to '' on real-PR builds) for the bash gate to read" + compiled.contains("AW_SYNTHETIC_PR: $(AW_SYNTHETIC_PR)"), + "Prepare step must pull the synth flag from the Agent-job-level hoisted \ + variable via the $(AW_SYNTHETIC_PR) macro — NOT directly from \ + `dependencies.Setup.outputs[...]` at step-env scope (that combination \ + proved unreliable in msazuresphere/4x4 build #612290)" + ); + // The Stage step's own env block must NOT contain a direct + // `dependencies.Setup.outputs[...]` reference. (The same expression + // IS expected at Agent-job-level `variables:` scope, the documented + // safe location — that hoist is asserted separately.) Scope this + // check by isolating the Stage step's bash + env body. + let stage_step = compiled + .split("Stage PR execution context") + .nth(1) + .map(|tail| { + // Stop at the next step (`- bash:` / `- task:` / `- script:`) + // or end of the job (a less-indented key). + let stop_at = ["\n - bash:", "\n - task:", "\n - script:"]; + let end = stop_at + .iter() + .filter_map(|needle| tail.find(needle)) + .min() + .unwrap_or(tail.len()); + &tail[..end] + }) + .unwrap_or(""); + assert!( + !stage_step.contains("dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR']"), + "Stage step's own env block must NOT reference \ + `dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR']` — that combination \ + (cross-job dep ref at step-env scope) proved unreliable. The cross-job \ + output is hoisted into Agent-job-level `variables:` (see \ + `generate_agent_job_variables`) and the Stage step reads it via the \ + `$(AW_SYNTHETIC_PR)` macro. Stage step body: {stage_step}" ); assert!( compiled.contains("BUILD_REASON: $(Build.Reason)"), @@ -5402,16 +5442,21 @@ fn test_execution_context_pr_emits_prepare_step_and_prompt_supplement() { // v7 + mode: synthetic (the default): env passthrough — the bundle // reads ADO predefined vars from `process.env`. The compiler emits - // coalesced macros that prefer the real `System.PullRequest.*` vars - // (true PR builds) and fall back to the `synthPr` Setup-job outputs - // (CI builds promoted via exec-context-pr-synth.js). + // coalesced runtime expressions that prefer the real + // `System.PullRequest.*` vars (true PR builds) and fall back to the + // **hoisted Agent-job-level variables** populated from the synthPr + // Setup-job outputs (CI builds promoted via exec-context-pr-synth.js). + // The hoist (see `generate_agent_job_variables`) is required because + // `dependencies..outputs[...]` references in step-level `env:` + // proved unreliable in practice — they resolved to empty even when + // the same expression worked in the job's `condition:`. assert!( - compiled.contains("SYSTEM_PULLREQUEST_PULLREQUESTID: \"$[ coalesce(variables['System.PullRequest.PullRequestId'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_ID']) ]\""), - "Prepare step must pass the PR id (coalesced with synthPr fallback) through to the bundle" + compiled.contains("SYSTEM_PULLREQUEST_PULLREQUESTID: \"$[ coalesce(variables['System.PullRequest.PullRequestId'], variables['AW_SYNTHETIC_PR_ID']) ]\""), + "Prepare step must pass the PR id (coalesced with the hoisted AW_SYNTHETIC_PR_ID job-variable) through to the bundle" ); assert!( - compiled.contains("SYSTEM_PULLREQUEST_TARGETBRANCH: \"$[ coalesce(variables['System.PullRequest.TargetBranch'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_TARGETBRANCH']) ]\""), - "Prepare step must pass the PR target branch (coalesced with synthPr fallback) through to the bundle" + compiled.contains("SYSTEM_PULLREQUEST_TARGETBRANCH: \"$[ coalesce(variables['System.PullRequest.TargetBranch'], variables['AW_SYNTHETIC_PR_TARGETBRANCH']) ]\""), + "Prepare step must pass the PR target branch (coalesced with the hoisted AW_SYNTHETIC_PR_TARGETBRANCH job-variable) through to the bundle" ); assert!( compiled.contains("SYSTEM_TEAMPROJECT: $(System.TeamProject)"), @@ -5967,11 +6012,21 @@ fn test_synthetic_pr_default_emits_full_synth_wiring() { ), "Fixture A must include the bash gate on exec-context-pr.js (accepts real-PR OR synth-promoted builds)" ); + assert!( + compiled.contains("AW_SYNTHETIC_PR: $(AW_SYNTHETIC_PR)"), + "Fixture A must read the synth flag from the hoisted Agent-job-level \ + variable via the $(AW_SYNTHETIC_PR) macro (NOT directly from \ + `dependencies.Setup.outputs[...]` at step-env scope, which proved \ + unreliable in build #612290)" + ); + // The Agent-job-level hoist itself must be present and pull from + // the cross-job synth output (legal scope for `dependencies.X.outputs[...]`). assert!( compiled.contains( - "AW_SYNTHETIC_PR: \"$[ coalesce(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], '') ]\"" + "AW_SYNTHETIC_PR: $[ coalesce(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], '') ]" ), - "Fixture A must project the synth flag into the exec-context-pr.js step env for the bash gate" + "Fixture A must hoist `synthPr.AW_SYNTHETIC_PR` into Agent-job-level \ + `variables:` so step-env consumers can read `$(AW_SYNTHETIC_PR)` safely" ); // Agent-job AW_SYNTHETIC_PR_SKIP guard.