From 5fd4463269f9b4c159e59716c3e43c12083b5e50 Mon Sep 17 00:00:00 2001 From: James Devine Date: Thu, 11 Jun 2026 16:04:30 +0100 Subject: [PATCH] fix(compile): unify synthetic-PR variable namespace via ado-script Move the real-vs-synth PR-identifier merge from broken step-env coalesce expressions into the exec-context-pr-synth.js bundle. ADO does not evaluate `$[ ... ]` runtime expressions inside step env values (only inside variables: and condition: fields), so the previous `SYSTEM_PULLREQUEST_PULLREQUESTID: `$[ coalesce(...) ]` form passed the literal string to bash and broke both Stage PR execution context and Evaluate PR filters (msazuresphere/4x4 build 612528). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/template-markers.md | 12 +- .../__tests__/index.test.ts | 113 ++++++-- .../src/exec-context-pr-synth/index.ts | 202 +++++++++----- src/compile/common.rs | 73 +++-- src/compile/extensions/ado_script.rs | 60 +++- src/compile/extensions/exec_context/pr.rs | 176 ++++++------ src/compile/filter_ir.rs | 84 +++--- tests/compiler_tests.rs | 263 ++++++++++++------ 8 files changed, 623 insertions(+), 360 deletions(-) diff --git a/docs/template-markers.md b/docs/template-markers.md index 9ae2849a..dd04fa59 100644 --- a/docs/template-markers.md +++ b/docs/template-markers.md @@ -300,12 +300,14 @@ Generates the Agent job's `variables:` block. Currently emits content **only** w 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` +- `AW_PR_ID` — resolved PR id (real on PR builds, discovered on synth-promoted CI builds) +- `AW_PR_TARGETBRANCH` — resolved PR target branch (`refs/heads/`) +- `AW_PR_SOURCEBRANCH` — resolved PR source branch +- `AW_SYNTHETIC_PR` — `"true"` only when this build was synth-promoted from CI; empty on real PR builds -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. +The hoist exists because ADO `$[ ... ]` runtime expressions are ONLY evaluated inside `variables:` mappings and `condition:` fields — putting them in step `env:` values passes the literal expression string verbatim to bash (empirically observed in `msazuresphere/4x4` build #612528: the `Stage PR execution context` step received `PR_ID='$[ coalesce(...)...` as a literal and PR-identifier validation rejected it). Job-level `variables:` is the documented safe location for cross-job output references; subsequent step `env:` blocks then consume the hoisted values via the plain `$(name)` macro (no `$[ ... ]` in step env, ever). + +The real-vs-synth merge happens inside `exec-context-pr-synth.js` so consumers read a single canonical name regardless of whether the build is a real PR or a synth-promoted CI build. ## {{ working_directory }} 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 a03871b2..f65dba95 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 @@ -23,18 +23,66 @@ describe("exec-context-pr-synth main", () => { }); afterEach(() => vi.restoreAllMocks()); - it("no-ops on real PR builds (BUILD_REASON=PullRequest)", async () => { + // ── Real-PR path ───────────────────────────────────────────────── + // + // On a real PR build, ADO populates `SYSTEM_PULLREQUEST_*` env vars + // directly. The bundle propagates them into the canonical `AW_PR_*` + // namespace so downstream consumers can read a single name regardless + // of the build's reason. No API call is made; no `AW_SYNTHETIC_PR` + // flag is emitted (this is not a synth-promotion). + + it("propagates SYSTEM_PULLREQUEST_* to AW_PR_* on real PR builds", async () => { const { code, output } = await runMain( - makeEnv({ BUILD_REASON: "PullRequest", PR_SYNTH_SPEC: build_pr_synth_spec() }), + makeEnv({ + BUILD_REASON: "PullRequest", + SYSTEM_PULLREQUEST_PULLREQUESTID: "4242", + SYSTEM_PULLREQUEST_TARGETBRANCH: "refs/heads/main", + SYSTEM_PULLREQUEST_SOURCEBRANCH: "refs/heads/feature/x", + SYSTEM_PULLREQUEST_ISDRAFT: "false", + PR_SYNTH_SPEC: build_pr_synth_spec(), + }), ); expect(code).toBe(0); expect(mocked.listActivePullRequestsBySourceRef).not.toHaveBeenCalled(); + // AW_PR_* are emitted as BOTH output (cross-job) and var (same-job). + expect(output).toContain("AW_PR_ID;isOutput=true]4242"); + expect(output).toContain("##vso[task.setvariable variable=AW_PR_ID]4242"); + expect(output).toContain("AW_PR_TARGETBRANCH;isOutput=true]refs/heads/main"); + expect(output).toContain( + "##vso[task.setvariable variable=AW_PR_TARGETBRANCH]refs/heads/main", + ); + expect(output).toContain("AW_PR_SOURCEBRANCH;isOutput=true]refs/heads/feature/x"); + expect(output).toContain( + "##vso[task.setvariable variable=AW_PR_SOURCEBRANCH]refs/heads/feature/x", + ); + expect(output).toContain("AW_PR_IS_DRAFT;isOutput=true]false"); + // No synth-promotion flag on a real PR build. + expect(output).not.toContain("AW_SYNTHETIC_PR;isOutput=true]true"); expect(output).not.toContain("AW_SYNTHETIC_PR_SKIP"); - expect(output).not.toContain("AW_SYNTHETIC_PR=true"); expect(output).toContain("real PR build"); }); - it("no-ops on GitHub-typed repos (BUILD_REPOSITORY_PROVIDER=GitHub)", async () => { + it("detects real PR build by SYSTEM_PULLREQUEST_PULLREQUESTID, not BUILD_REASON", async () => { + // Defensive: even on builds where BUILD_REASON isn't "PullRequest" + // for some reason (e.g. a manual re-queue of a PR build), the + // presence of a SYSTEM_PULLREQUEST_PULLREQUESTID is the authoritative + // signal — that's the value we need to propagate. + const { code, output } = await runMain( + makeEnv({ + BUILD_REASON: "Manual", + SYSTEM_PULLREQUEST_PULLREQUESTID: "99", + SYSTEM_PULLREQUEST_TARGETBRANCH: "refs/heads/main", + PR_SYNTH_SPEC: build_pr_synth_spec(), + }), + ); + expect(code).toBe(0); + expect(mocked.listActivePullRequestsBySourceRef).not.toHaveBeenCalled(); + expect(output).toContain("AW_PR_ID;isOutput=true]99"); + }); + + // ── GitHub repo path ──────────────────────────────────────────── + + it("skips with empty AW_PR_* on GitHub-typed repos (CI builds)", async () => { const { code, output } = await runMain( makeEnv({ BUILD_REPOSITORY_PROVIDER: "GitHub", @@ -44,8 +92,15 @@ describe("exec-context-pr-synth main", () => { expect(code).toBe(0); expect(mocked.listActivePullRequestsBySourceRef).not.toHaveBeenCalled(); expect(output).toContain("GitHub-typed repo"); + // SKIP marker tells the Agent job's condition to opt out cleanly. + expect(output).toContain("AW_SYNTHETIC_PR_SKIP;isOutput=true]true"); + // Empty AW_PR_* so same-job consumers see stable defined variables. + expect(output).toContain("##vso[task.setvariable variable=AW_PR_ID]"); + expect(output).not.toContain("AW_PR_ID;isOutput=true]4242"); }); + // ── Hard failures ──────────────────────────────────────────────── + it("returns 1 (hard fail) when PR_SYNTH_SPEC is missing", async () => { const env = makeEnv({}); delete env.PR_SYNTH_SPEC; @@ -62,6 +117,13 @@ describe("exec-context-pr-synth main", () => { expect(output).toContain("PR_SYNTH_SPEC"); }); + // ── Soft skips (CI build, ADO repo, no matching PR) ───────────── + // + // Every soft-skip path emits empty AW_PR_* via setVar+setOutput so + // downstream consumers see stable defined variables (rather than the + // literal `$(AW_PR_ID)` string that ADO leaves when a macro is + // undefined). The SKIP marker gates the Agent job's `condition:`. + it("skips when source branch has no active PR (per ADO API)", async () => { mocked.listActivePullRequestsBySourceRef.mockResolvedValue([]); const { code, output } = await runMain( @@ -72,6 +134,8 @@ describe("exec-context-pr-synth main", () => { ); expect(code).toBe(0); expect(output).toContain("AW_SYNTHETIC_PR_SKIP;isOutput=true]true"); + // Empty defaults for AW_PR_*. + expect(output).toContain("##vso[task.setvariable variable=AW_PR_ID]"); expect(mocked.listActivePullRequestsBySourceRef).toHaveBeenCalledOnce(); }); @@ -87,6 +151,7 @@ describe("exec-context-pr-synth main", () => { const { code, output } = await runMain(makeEnv({ PR_SYNTH_SPEC: spec })); expect(code).toBe(0); expect(output).toContain("AW_SYNTHETIC_PR_SKIP;isOutput=true]true"); + expect(output).toContain("##vso[task.setvariable variable=AW_PR_ID]"); }); it("skips when >1 active PRs match (after target filter)", async () => { @@ -118,7 +183,9 @@ describe("exec-context-pr-synth main", () => { expect(output).toContain("no changed file"); }); - it("emits AW_SYNTHETIC_PR=true + identifiers on the happy path", async () => { + // ── Happy path: synth-promote a CI build with a matching PR ───── + + it("emits AW_PR_* + AW_SYNTHETIC_PR=true on the synth happy path", async () => { mocked.listActivePullRequestsBySourceRef.mockResolvedValue([ { pullRequestId: 1234, @@ -138,31 +205,27 @@ 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"); + // AW_PR_* emitted TWICE: once as output (cross-job, hoisted into the + // Agent job's `variables:` block) and once as a regular variable + // (same-job, consumed by the Setup-job gate step's `env:` via + // `$(AW_PR_*)` macros). See `setVar` in `shared/vso-logger.ts`. + expect(output).toContain("AW_PR_ID;isOutput=true]1234"); + expect(output).toContain("##vso[task.setvariable variable=AW_PR_ID]1234"); + expect(output).toContain("AW_PR_TARGETBRANCH;isOutput=true]refs/heads/main"); expect(output).toContain( - "##vso[task.setvariable variable=AW_SYNTHETIC_PR_TARGETBRANCH]refs/heads/main", + "##vso[task.setvariable variable=AW_PR_TARGETBRANCH]refs/heads/main", ); + expect(output).toContain("AW_PR_SOURCEBRANCH;isOutput=true]refs/heads/feature/x"); expect(output).toContain( - "##vso[task.setvariable variable=AW_SYNTHETIC_PR_SOURCEBRANCH]refs/heads/feature/x", + "##vso[task.setvariable variable=AW_PR_SOURCEBRANCH]refs/heads/feature/x", ); - expect(output).toContain("##vso[task.setvariable variable=AW_SYNTHETIC_PR_IS_DRAFT]false"); + expect(output).toContain("AW_PR_IS_DRAFT;isOutput=true]false"); + // Synth-promotion flag (only on this path, not real PR or skip). + expect(output).toContain("AW_SYNTHETIC_PR;isOutput=true]true"); + expect(output).toContain("##vso[task.setvariable variable=AW_SYNTHETIC_PR]true"); }); - it("emits AW_SYNTHETIC_PR_IS_DRAFT=true when the PR is a draft", async () => { + it("emits AW_PR_IS_DRAFT=true when the matched synth PR is a draft", async () => { mocked.listActivePullRequestsBySourceRef.mockResolvedValue([ { pullRequestId: 1, @@ -173,7 +236,7 @@ describe("exec-context-pr-synth main", () => { ]); const { code, output } = await runMain(makeEnv({ PR_SYNTH_SPEC: build_pr_synth_spec() })); expect(code).toBe(0); - expect(output).toContain("AW_SYNTHETIC_PR_IS_DRAFT;isOutput=true]true"); + expect(output).toContain("AW_PR_IS_DRAFT;isOutput=true]true"); }); it("skips path-filter API calls when paths.include and exclude are both empty", 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 d45afe43..fb853bfe 100644 --- a/scripts/ado-script/src/exec-context-pr-synth/index.ts +++ b/scripts/ado-script/src/exec-context-pr-synth/index.ts @@ -1,37 +1,63 @@ /** - * exec-context-pr-synth — Setup-job script that synthesises - * `Build.Reason == PullRequest` semantics on CI-triggered builds when - * an open PR matches the agent's `on.pr.branches` / `on.pr.paths` - * filters. + * exec-context-pr-synth — Setup-job script that normalises PR-identifier + * variables for downstream consumers. Runs unconditionally so that the + * single name `AW_PR_*` always carries the resolved PR identifiers, + * whether the build is a real PR or a CI build promoted to PR semantics. * - * Why this exists: Azure DevOps Services ignores the YAML `pr:` block - * unless a per-branch Build Validation policy is registered server- - * side. Without that policy, a push to a feature branch fires the - * pipeline as `Build.Reason = IndividualCI` even when an open PR - * exists, so the gate evaluator's "not a PR build" bypass triggers - * and `exec-context-pr.js` is skipped entirely. + * ## Why "always run" * - * This script runs in the Setup job before `prGate`, calls the ADO - * REST API to find the active PR for `Build.SourceBranch`, applies - * the front-matter filters, and emits `AW_SYNTHETIC_PR*` outputs - * that downstream gate + exec-context-pr steps coalesce with the - * real `System.PullRequest.*` variables. + * Earlier versions gated this script on `ne(Build.Reason, 'PullRequest')` + * and forced downstream steps to coalesce `$(System.PullRequest.X)` with + * `$(AW_SYNTHETIC_PR_X)` inside step `env:` via a `$[ ... ]` runtime + * expression. That fails: ADO only evaluates `$[ ... ]` inside the + * `variables:` block and `condition:` fields, NOT inside step `env:` + * values. Both `Stage PR execution context` and `Evaluate PR filters` + * received the literal expression string and short-circuited (see build + * ). * - * Runtime contract (all soft skips exit 0; only spec-decode errors - * and infra failures exit non-zero): + * The fix is to do the real-vs-synth merge HERE, in TypeScript, and have + * every downstream consumer read `$(AW_PR_*)` macros — no coalesce, no + * runtime expressions in step env. The path is identical on real PR + * builds and synth-promoted CI builds; only the `AW_SYNTHETIC_PR` + * boolean flag distinguishes them, for the Agent job's `condition:` + * which legitimately can use `dependencies.Setup.outputs[...]`. * - * 1. real PR build (BUILD_REASON=PullRequest) → no-op - * 2. GitHub-typed repo (BUILD_REPOSITORY_PROVIDER=GitHub) → no-op - * 3. Decode PR_SYNTH_SPEC (hard fail on corruption) + * ## Why synth in the first place + * + * Azure DevOps Services ignores the YAML `pr:` block unless a per- + * branch Build Validation policy is registered server-side. Without + * that policy, a push to a feature branch fires the pipeline as + * `Build.Reason = IndividualCI` even when an open PR exists. + * + * ## Variables emitted (each as BOTH `setOutput` and `setVar` — see + * `shared/vso-logger.ts::setVar` for the dual-emit rationale): + * + * - `AW_PR_ID` — resolved PR id (real or synth), empty if none + * - `AW_PR_TARGETBRANCH` — resolved target ref (`refs/heads/`) + * - `AW_PR_SOURCEBRANCH` — resolved source ref + * - `AW_PR_IS_DRAFT` — "true"/"false"/"" (only meaningful on synth path) + * - `AW_SYNTHETIC_PR` — "true" iff this build was synth-promoted + * (i.e. CI build + matched open PR). Empty + * on real PR builds and on non-promoted CI. + * - `AW_SYNTHETIC_PR_SKIP` — "true" iff synth was attempted but no + * match was found (gates Agent job to skip) + * + * ## Runtime contract (all soft skips exit 0; only spec-decode and + * infra errors exit non-zero): + * + * 1. real PR build (`SYSTEM_PULLREQUEST_PULLREQUESTID` non-empty) → + * copy `SYSTEM_PULLREQUEST_*` env into `AW_PR_*` and return + * 2. GitHub-typed repo (`BUILD_REPOSITORY_PROVIDER=GitHub`) → emit + * empty `AW_PR_*` + `AW_SYNTHETIC_PR_SKIP=true` (GitHub PR semantics + * are routed natively by ADO; CI builds on GitHub repos don't get + * synth-promoted) + * 3. Decode `PR_SYNTH_SPEC` (hard fail on corruption) * 4. fetch active PRs whose `sourceRefName == BUILD_SOURCEBRANCH` - * (no source-branch pre-filter against the spec — `on.pr.branches` - * lists *target* branches per ADO semantics, not the build's - * source branch) * 5. filter matched PRs by `targetRefName` against * `spec.branches.include` / `spec.branches.exclude` - * 6. count != 1 → skip - * 7. paths.include/exclude reject every changed file → skip - * 8. emit AW_SYNTHETIC_PR* outputs + * 6. count != 1 → emit empty `AW_PR_*` + skip + * 7. paths.include/exclude reject every changed file → empty + skip + * 8. on match: emit resolved `AW_PR_*` + `AW_SYNTHETIC_PR=true` */ import { getIterationChanges, @@ -45,22 +71,73 @@ import { decodeSpec, type PrSynthSpec } from "./spec.js"; const SKIP_OUTPUT = "AW_SYNTHETIC_PR_SKIP"; +/** + * Emit the canonical AW_PR_* identifier set as BOTH cross-job outputs + * (consumed by the Agent job's `variables:` hoist via + * `dependencies.Setup.outputs['synthPr.AW_PR_*']`) and same-job + * regular variables (consumed by the gate step's `env:` block via + * `$(AW_PR_*)` macros). Both forms are required: `isOutput=true` does + * NOT register the variable in the producing job's regular variable + * namespace, so a same-job consumer would see empty without the + * paired `setVar`. See `setVar` doc-comment in `shared/vso-logger.ts`. + * + * Empty strings are valid: downstream consumers treat empty `AW_PR_ID` + * as "not a PR build" (matching the legacy `Build.Reason` check). + */ +function emitPrIdentifiers( + prId: string, + targetBranch: string, + sourceBranch: string, + isDraft: string, +): void { + const emitBoth = (name: string, value: string): void => { + setOutput(name, value); + setVar(name, value); + }; + emitBoth("AW_PR_ID", prId); + emitBoth("AW_PR_TARGETBRANCH", targetBranch); + emitBoth("AW_PR_SOURCEBRANCH", sourceBranch); + emitBoth("AW_PR_IS_DRAFT", isDraft); +} + function emitSkip(reason: string): void { setOutput(SKIP_OUTPUT, "true"); logInfo(`[synth-pr] skip: ${reason}`); } export async function main(env: NodeJS.ProcessEnv = process.env): Promise { - // Step 1 — real PR build owns the path; the bundle has nothing to add. - if ((env.BUILD_REASON ?? "") === "PullRequest") { - logInfo("[synth-pr] BUILD_REASON=PullRequest; real PR build, synth skipped"); + // Step 1 — real PR build owns the path: propagate the predefined + // SYSTEM_PULLREQUEST_* env into the AW_PR_* namespace so downstream + // consumers can read a single name regardless of source. No API call + // needed; ADO has already populated the values. + // + // Detection: `SYSTEM_PULLREQUEST_PULLREQUESTID` is non-empty iff this + // is a real PR build. `BUILD_REASON=PullRequest` could also be used, + // but checking the actual id is more precise (it's the value we + // actually need to propagate, and it can't be set without the build + // really being a PR build). + const realPrId = env.SYSTEM_PULLREQUEST_PULLREQUESTID ?? ""; + if (realPrId.length > 0) { + emitPrIdentifiers( + realPrId, + env.SYSTEM_PULLREQUEST_TARGETBRANCH ?? "", + env.SYSTEM_PULLREQUEST_SOURCEBRANCH ?? "", + env.SYSTEM_PULLREQUEST_ISDRAFT ?? "", + ); + logInfo( + `[synth-pr] real PR build #${realPrId}; propagating SYSTEM_PULLREQUEST_* to AW_PR_*`, + ); return 0; } - // Step 2 — GitHub-typed repos already get correct pr: semantics from - // ADO. Synthesising would double-fire. + // Step 2 — GitHub-typed repos: ADO routes GitHub PR webhooks natively, + // so a CI build on a GitHub repo means there's no associated PR (it + // would have come in as a PR build instead). Emit empty AW_PR_* so + // same-job consumers have stable defined variables, plus the SKIP + // marker so the Agent job's `condition:` can opt out cleanly. if ((env.BUILD_REPOSITORY_PROVIDER ?? "").toLowerCase() === "github") { - logInfo("[synth-pr] GitHub-typed repo; ADO already provides PR semantics, synth skipped"); + emitPrIdentifiers("", "", "", ""); + emitSkip("GitHub-typed repo; ADO already provides PR semantics natively"); return 0; } @@ -75,6 +152,7 @@ export async function main(env: NodeJS.ProcessEnv = process.env): Promise 1) { + emitPrIdentifiers("", "", "", ""); emitSkip( `${matched.length} active PRs match source ${sourceBranch}; refusing to choose`, ); @@ -123,6 +203,7 @@ export async function main(env: NodeJS.ProcessEnv = process.env): Promise`, matching - // the `targetRefName` / `sourceRefName` shape returned by the ADO - // REST API (`refs/heads/main`, `refs/heads/feature/x`, etc.). The - // coalesce on consumer steps therefore yields a consistent - // refs-prefixed value whether the build was a real PR or synth- - // promoted. (The unprefixed short form is `TargetBranchName` — - // a separate predefined variable we deliberately do not use here.) - // See . - // 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). + // Emit the resolved PR identifiers under the canonical AW_PR_* + // names (same names downstream consumers use whether they run on a + // real PR build or a synth-promoted CI build). Plus the + // `AW_SYNTHETIC_PR` flag (only true on synth promotion) which the + // Agent job's `condition:` consults to require the PR-gate path. // - // 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"); + // Format note: `targetRefName` / `sourceRefName` from the ADO REST + // API are full refs (`refs/heads/main`, `refs/heads/feature/x`, + // etc.), matching the shape of `SYSTEM_PULLREQUEST_TARGETBRANCH` on + // real PR builds so downstream gets a consistent value either way. + // See . + emitPrIdentifiers( + String(prId), + pr.targetRefName ?? "", + pr.sourceRefName ?? sourceBranch, + pr.isDraft === true ? "true" : "false", + ); + setOutput("AW_SYNTHETIC_PR", "true"); + setVar("AW_SYNTHETIC_PR", "true"); logInfo( `[synth-pr] matched PR #${prId} (source=${pr.sourceRefName} target=${pr.targetRefName})`, diff --git a/src/compile/common.rs b/src/compile/common.rs index 6f19b98a..5d51d7ce 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -1119,20 +1119,33 @@ pub fn generate_job_timeout(front_matter: &FrontMatter) -> String { /// (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: +/// Hoist the synthPr Setup-job outputs into the Agent job's `variables:` +/// block so step-level `env:` mappings can consume them via the +/// `$(name)` macro form. Emitted only when the synth path is active. +/// +/// **Why job-level variables and not step-level env**: ADO `$[ ... ]` +/// runtime expressions only evaluate inside `variables:` blocks and +/// `condition:` fields — NOT inside step `env:` values. Putting +/// `$[ dependencies..outputs[...] ]` directly in step-level `env:` +/// fails: the literal expression string is passed verbatim to the step +/// (msazuresphere/4x4 build #612528 — `[aw-context] pr context +/// preparation failed: PR identifier validation failed (PR_ID='$[ +/// coalesce(variables['System.PullRequest.PullRequestId'], +/// variables['AW_SYNTHET…' is not a positive integer)`). The job-level +/// hoist is the only documented safe location: /// . /// +/// **Variable namespace**: +/// +/// - `AW_PR_ID` / `AW_PR_TARGETBRANCH` / `AW_PR_SOURCEBRANCH` — +/// resolved PR identifiers (real on PR builds, discovered on synth +/// builds). The merge happens inside `exec-context-pr-synth.js` so +/// every consumer can read a single name regardless of source. +/// - `AW_SYNTHETIC_PR` — boolean flag set to "true" only when the +/// build was synth-promoted from CI; empty on real PR builds. +/// Consumed by the Agent's bash exec-context-pr gate and by gate +/// bypass logic that needs to distinguish "real PR" from "synth". +/// /// 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 @@ -1155,7 +1168,7 @@ pub fn generate_agent_job_variables(synthetic_pr_active: bool) -> 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() + "variables:\n AW_PR_ID: $[ coalesce(dependencies.Setup.outputs['synthPr.AW_PR_ID'], '') ]\n AW_PR_TARGETBRANCH: $[ coalesce(dependencies.Setup.outputs['synthPr.AW_PR_TARGETBRANCH'], '') ]\n AW_PR_SOURCEBRANCH: $[ coalesce(dependencies.Setup.outputs['synthPr.AW_PR_SOURCEBRANCH'], '') ]\n AW_SYNTHETIC_PR: $[ coalesce(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], '') ]".to_string() } /// Format a single step's YAML string with proper indentation @@ -3840,16 +3853,21 @@ mod tests { 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 + // Each AW_PR_* identifier + the AW_SYNTHETIC_PR flag is 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). + // dependency is unresolved (e.g. Setup skipped). `synthPr` now + // always emits the canonical `AW_PR_*` names regardless of + // build reason (copying from `SYSTEM_PULLREQUEST_*` on real PR + // builds, discovered values on synth-promoted CI builds), so + // downstream consumers read a single uniform namespace via + // `$(AW_PR_*)` macros — no `$[ ... ]` in step `env:`. for name in &[ + "AW_PR_ID", + "AW_PR_TARGETBRANCH", + "AW_PR_SOURCEBRANCH", "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}'], '') ]" @@ -3859,6 +3877,21 @@ mod tests { "must hoist {name} from cross-job synth output: {out}" ); } + // Regression guard: the old AW_SYNTHETIC_PR_* names must not + // leak back — they were renamed to AW_PR_* when the bundle + // started normalising the real-vs-synth merge internally. + assert!( + !out.contains("AW_SYNTHETIC_PR_ID"), + "must not hoist legacy AW_SYNTHETIC_PR_ID — use AW_PR_ID: {out}" + ); + assert!( + !out.contains("AW_SYNTHETIC_PR_TARGETBRANCH"), + "must not hoist legacy AW_SYNTHETIC_PR_TARGETBRANCH — use AW_PR_TARGETBRANCH: {out}" + ); + assert!( + !out.contains("AW_SYNTHETIC_PR_SOURCEBRANCH"), + "must not hoist legacy AW_SYNTHETIC_PR_SOURCEBRANCH — use AW_PR_SOURCEBRANCH: {out}" + ); } // ─── normalize_yaml ─────────────────────────────────────────────────────── diff --git a/src/compile/extensions/ado_script.rs b/src/compile/extensions/ado_script.rs index 164cfef0..38b81250 100644 --- a/src/compile/extensions/ado_script.rs +++ b/src/compile/extensions/ado_script.rs @@ -173,14 +173,32 @@ fn resolver_step() -> String { } /// The synthetic-PR-context step that runs in the Setup job BEFORE -/// `prGate`. Looks up the open PR for `Build.SourceBranch` via the -/// ADO REST API and emits `AW_SYNTHETIC_PR*` outputs that downstream -/// gate + exec-context-pr steps coalesce with the real -/// `System.PullRequest.*` variables. +/// `prGate`. Normalises PR-identifier variables under the canonical +/// `AW_PR_*` names regardless of build reason: /// -/// `condition: ne(Build.Reason, 'PullRequest')` short-circuits the -/// bundle on real PR builds (the bundle would no-op anyway, but the -/// step-level condition skips the env-block evaluation too). +/// - **Real PR build** (`SYSTEM_PULLREQUEST_PULLREQUESTID` populated): +/// copies the existing `SYSTEM_PULLREQUEST_*` env values into the +/// `AW_PR_*` namespace. No API call. +/// - **CI build on ADO repo**: looks up the open PR for +/// `Build.SourceBranch` via the ADO REST API, applies the front- +/// matter filters, and emits `AW_PR_*` plus `AW_SYNTHETIC_PR=true` +/// on a match. +/// - **CI build on GitHub-typed repo**: emits empty `AW_PR_*` + +/// `AW_SYNTHETIC_PR_SKIP=true`. +/// +/// Always runs (`condition: succeeded()`). The previous form gated on +/// `ne(Build.Reason, 'PullRequest')`, which forced downstream consumers +/// to coalesce `$(System.PullRequest.X)` with `$(AW_SYNTHETIC_PR_X)` +/// inside step `env:` via `$[ ... ]` runtime expressions — but ADO +/// only evaluates `$[ ... ]` inside `variables:` and `condition:` +/// fields, NOT inside step `env:`. The literal expression string was +/// passed verbatim to bash and downstream PR steps short-circuited +/// (msazuresphere/4x4 build #612528). Doing the merge in the bundle +/// eliminates the bug class — every downstream consumer just reads +/// `$(AW_PR_*)` macros. +/// +/// `SYSTEM_PULLREQUEST_*` env vars are passed in so the bundle can +/// detect a real PR build and propagate the predefined values. fn synthetic_pr_step(spec_b64: &str) -> String { format!( r#"- bash: | @@ -188,7 +206,7 @@ fn synthetic_pr_step(spec_b64: &str) -> String { node '{EXEC_CONTEXT_PR_SYNTH_PATH}' name: synthPr displayName: "Resolve synthetic PR context" - condition: and(succeeded(), ne(variables['Build.Reason'], 'PullRequest')) + condition: succeeded() env: SYSTEM_ACCESSTOKEN: $(System.AccessToken) ADO_COLLECTION_URI: $(System.CollectionUri) @@ -197,6 +215,10 @@ fn synthetic_pr_step(spec_b64: &str) -> String { BUILD_REASON: $(Build.Reason) BUILD_REPOSITORY_PROVIDER: $(Build.Repository.Provider) BUILD_SOURCEBRANCH: $(Build.SourceBranch) + SYSTEM_PULLREQUEST_PULLREQUESTID: $(System.PullRequest.PullRequestId) + SYSTEM_PULLREQUEST_TARGETBRANCH: $(System.PullRequest.TargetBranch) + SYSTEM_PULLREQUEST_SOURCEBRANCH: $(System.PullRequest.SourceBranch) + SYSTEM_PULLREQUEST_ISDRAFT: $(System.PullRequest.IsDraft) PR_SYNTH_SPEC: "{spec_b64}""# ) } @@ -558,7 +580,27 @@ mod tests { ); assert!(steps[2].contains("exec-context-pr-synth.js")); assert!(steps[2].contains("PR_SYNTH_SPEC:")); - assert!(steps[2].contains("ne(variables['Build.Reason'], 'PullRequest')")); + // synthPr now runs unconditionally — it does the real-vs-synth + // merge internally, so downstream consumers always read + // `$(AW_PR_*)` macros regardless of build reason. + assert!( + steps[2].contains("condition: succeeded()"), + "synthPr must run unconditionally (not gated on Build.Reason): {}", + steps[2] + ); + assert!( + !steps[2].contains("ne(variables['Build.Reason'], 'PullRequest')"), + "synthPr must NOT gate on Build.Reason — it propagates SYSTEM_PULLREQUEST_* into AW_PR_* on real PR builds: {}", + steps[2] + ); + // Real-PR-detection requires the SYSTEM_PULLREQUEST_* env vars + // to be passed in so the bundle can short-circuit without the + // ADO REST API call. + assert!( + steps[2].contains("SYSTEM_PULLREQUEST_PULLREQUESTID: $(System.PullRequest.PullRequestId)"), + "synthPr must pass SYSTEM_PULLREQUEST_PULLREQUESTID so the bundle can detect a real PR build: {}", + steps[2] + ); } #[test] diff --git a/src/compile/extensions/exec_context/pr.rs b/src/compile/extensions/exec_context/pr.rs index 9713869c..3ba6b25d 100644 --- a/src/compile/extensions/exec_context/pr.rs +++ b/src/compile/extensions/exec_context/pr.rs @@ -122,58 +122,56 @@ 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, 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: . + // ## Synth-active vs synth-inactive env wiring // - // `System.PullRequest.TargetBranch` is `refs/heads/` (full - // ref form), matching the `targetRefName` shape stashed in - // `AW_SYNTHETIC_PR_TARGETBRANCH`, so the coalesce yields a - // consistent value either way. + // **Synth-active** (`mode: synthetic`, the default): the + // `synthPr` Setup-job step runs unconditionally and emits + // `AW_PR_ID` / `AW_PR_TARGETBRANCH` / `AW_PR_SOURCEBRANCH` + // under canonical names — on real PR builds they hold the + // copied `SYSTEM_PULLREQUEST_*` values; on synth-promoted CI + // builds they hold the discovered PR identifiers. The Agent + // job hoists those outputs to job-level variables (see + // `generate_agent_job_variables`). This step consumes them + // via plain `$(name)` macros — no `$[ ... ]` in step `env:` + // (which ADO doesn't evaluate; that bug bit + // msazuresphere/4x4 build #612528). + // + // **Synth-inactive** (`mode: policy`): no `synthPr` step + // emits the hoist; the step reads `$(System.PullRequest.*)` + // macros directly and gates on `eq(Build.Reason, + // 'PullRequest')` at step level. // // ## Synth-active gating — bash, not step `condition:` // // ADO step-level `condition:` fields CANNOT reference // `dependencies..outputs[...]`. That syntax is only legal - // in **job**-level `condition:`, in `variables:` mappings, - // and in step-level `env:` values (via `$[ ... ]`). Attempting - // to use it in a step condition produces a pipeline-validation - // error ("Unrecognized value: 'dependencies'") and the build - // fails before the Agent job starts. + // in **job**-level `condition:` and in `variables:` mappings. + // Attempting to use it in a step condition produces a pipeline- + // validation error ("Unrecognized value: 'dependencies'") and + // the build fails before the Agent job starts. // - // We therefore project the synth flag through the same - // step-level `env:` indirection used for the PR id / target - // 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. + // We therefore gate in bash: the resolved `AW_PR_ID` is empty + // iff this is neither a real PR build nor a synth-promoted CI + // build, which is exactly when the bundle should skip. Same + // gate logic, but in the only place ADO actually lets us put + // it. The step still emits as `succeeded` in the ADO UI on + // skips (with a single log line) rather than `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'], 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", + "$(AW_PR_ID)", + "$(AW_PR_TARGETBRANCH)", + // Bash gate. `$AW_PR_ID` reads the hoisted job-level + // variable via the step-env `$(...)` macro below. It + // is non-empty when the build is either a real PR or + // synth-promoted; empty otherwise. Quoted for + // shellcheck and `set -u` safety. + " if [ -z \"$AW_PR_ID\" ]; then\n echo \"[aw-context] No PR identifier resolved (not a PR build and not synth-promoted); skipping exec-context-pr.\"\n exit 0\n fi\n", "succeeded()", ) } else { @@ -184,18 +182,6 @@ impl ContextContributor for PrContextContributor { "eq(variables['Build.Reason'], 'PullRequest')", ) }; - // Synth-active path adds two env vars (BUILD_REASON + the - // 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: $(AW_SYNTHETIC_PR)" - } else { - "" - }; format!( r#"- bash: | set -euo pipefail @@ -206,7 +192,7 @@ impl ContextContributor for PrContextContributor { SYSTEM_PULLREQUEST_TARGETBRANCH: {target_branch_macro} SYSTEM_TEAMPROJECT: $(System.TeamProject) BUILD_REPOSITORY_NAME: $(Build.Repository.Name) - BUILD_SOURCESDIRECTORY: $(Build.SourcesDirectory){synth_env} + BUILD_SOURCESDIRECTORY: $(Build.SourcesDirectory) displayName: "Stage PR execution context (aw-context/pr/*)" condition: {condition}"# ) @@ -257,63 +243,57 @@ mod tests { } #[test] - fn prepare_step_synth_active_emits_coalesced_env_and_bash_synth_guard() { + fn prepare_step_synth_active_uses_macros_for_hoisted_aw_pr_vars_and_bash_guard() { let contributor = PrContextContributor::new(PrContextConfig::default(), true); let fm = pr_fm(); let ctx = CompileContext::for_test(&fm); let step = contributor.prepare_step(&ctx); - // Env: PR id + target branch are coalesced via cross-job runtime - // expressions wrapped in YAML double quotes (Agent job depends - // on Setup, so `dependencies.Setup.outputs[...]` is the correct - // form here — distinct from the gate step which is same-job). + // Env: PR id + target branch read the Agent-job-level hoisted + // AW_PR_* variables (which `generate_agent_job_variables` + // declares from `dependencies.Setup.outputs['synthPr.AW_PR_*']`). + // Use plain `$(name)` macros — NOT `$[ ... ]` runtime expressions + // (ADO doesn't evaluate `$[ ... ]` inside step `env:`; the + // literal expression string gets passed verbatim and downstream + // validation rejects it — see msazuresphere/4x4 build #612528). assert!( - step.contains( - "SYSTEM_PULLREQUEST_PULLREQUESTID: \"$[ coalesce(variables['System.PullRequest.PullRequestId'], variables['AW_SYNTHETIC_PR_ID']) ]\"" - ), - "synth-active prepare step must coalesce PR id with the hoisted job-level AW_SYNTHETIC_PR_ID variable: {step}" + step.contains("SYSTEM_PULLREQUEST_PULLREQUESTID: $(AW_PR_ID)"), + "synth-active prepare step must read the hoisted Agent-job-level AW_PR_ID via $() macro: {step}" ); assert!( - step.contains( - "SYSTEM_PULLREQUEST_TARGETBRANCH: \"$[ coalesce(variables['System.PullRequest.TargetBranch'], variables['AW_SYNTHETIC_PR_TARGETBRANCH']) ]\"" - ), - "synth-active prepare step must coalesce target branch with the hoisted job-level AW_SYNTHETIC_PR_TARGETBRANCH variable: {step}" + step.contains("SYSTEM_PULLREQUEST_TARGETBRANCH: $(AW_PR_TARGETBRANCH)"), + "synth-active prepare step must read the hoisted Agent-job-level AW_PR_TARGETBRANCH via $() macro: {step}" ); - // Env: BUILD_REASON + synth flag projected through env so the - // bash gate has plain `$BUILD_REASON` / `$AW_SYNTHETIC_PR` to - // 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}" - ); + // Defensive: NO `$[ ... ]` runtime expressions in this step's + // env block. They're only legal inside `variables:` mappings + // and `condition:` fields — putting them in step env is the + // exact bug class this refactor eliminates. + let env_block_start = step + .find("\n env:\n") + .expect("step must have an env block"); + let env_block_end = step[env_block_start..] + .find("\n displayName:") + .map(|i| env_block_start + i) + .unwrap_or(step.len()); + let env_block = &step[env_block_start..env_block_end]; assert!( - 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}" + !env_block.contains("$["), + "prepare step env block must not contain `$[ ` runtime expressions \ + (ADO doesn't evaluate them in step env — use job-level variables \ + hoist + $() macro instead): {env_block}" ); - // Bash guard: literal `if` chain that exits 0 when neither - // condition holds. Variables are double-quoted so shellcheck - // is clean and `set -u` is safe. + // Bash guard: empty `$AW_PR_ID` means "not a PR build and not + // synth-promoted". Single check replaces the previous + // BUILD_REASON + AW_SYNTHETIC_PR pair (the merge now happens + // inside `exec-context-pr-synth.js`). assert!( - step.contains( - "if [ \"$BUILD_REASON\" != \"PullRequest\" ] && [ \"$AW_SYNTHETIC_PR\" != \"true\" ]; then" - ), - "synth-active prepare step must include the bash gate (replaces the illegal step-level condition): {step}" + step.contains("if [ -z \"$AW_PR_ID\" ]; then"), + "synth-active prepare step must include the bash gate on empty AW_PR_ID: {step}" ); assert!( - step.contains( - "[aw-context] Not a PR build and not synth-promoted; skipping exec-context-pr." - ), + step.contains("[aw-context] No PR identifier resolved"), "synth-active prepare step must emit a single skip log line so the no-op is discoverable: {step}" ); @@ -333,7 +313,7 @@ mod tests { "condition: or(eq(variables['Build.Reason'], 'PullRequest'), eq(dependencies.Setup.outputs" ), "synth-active prepare step must NOT use the illegal cross-job dep ref in step `condition:` \ - (only legal in job-level conditions / `variables:` mappings / step `env:` values): {step}" + (only legal in job-level conditions / `variables:` mappings): {step}" ); } @@ -364,12 +344,12 @@ mod tests { // Defensive: the synth-mode signature MUST NOT appear when the // synth path is inactive. assert!( - !step.contains("synthPr.AW_SYNTHETIC_PR"), - "synth-inactive prepare step must not reference any synthPr Setup-job output: {step}" + !step.contains("AW_PR_ID"), + "synth-inactive prepare step must not reference the synth-only AW_PR_ID hoist: {step}" ); assert!( - !step.contains("coalesce("), - "synth-inactive prepare step must not emit a coalesce expression: {step}" + !step.contains("synthPr"), + "synth-inactive prepare step must not reference any synthPr Setup-job output: {step}" ); } } diff --git a/src/compile/filter_ir.rs b/src/compile/filter_ir.rs index f7d53bc4..f60d49ef 100644 --- a/src/compile/filter_ir.rs +++ b/src/compile/filter_ir.rs @@ -1121,12 +1121,12 @@ pub fn build_gate_spec(ctx: GateContext, checks: &[FilterCheck]) -> anyhow::Resu /// /// When `synthetic_pr_active` is true AND `ctx == GateContext::PullRequest`, /// 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 regular pipeline variables emitted by the `synthPr` -/// Setup-job step on a CI build promoted by `exec-context-pr-synth.js`. +/// `ADO_TARGET_BRANCH`) read the canonical `AW_PR_*` same-job variables +/// that the preceding `synthPr` step emits via `setVar` — whether the +/// build is a real PR (synthPr copied `SYSTEM_PULLREQUEST_*`) or a +/// synth-promoted CI build (synthPr discovered + emitted the values). /// Also exports `AW_SYNTHETIC_PR` so `gate/bypass.ts` knows to skip the -/// "not a PR build" bypass. +/// "not a PR build" bypass on synth-promoted builds. /// /// **Same-job synth references**: this gate step lives in the **Setup /// job** (`AdoScriptExtension::setup_steps` returns it), the same job @@ -1138,24 +1138,20 @@ pub fn build_gate_spec(ctx: GateContext, checks: &[FilterCheck]) -> anyhow::Resu /// 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 …`. +/// emits the same name as a regular (non-output) variable. The +/// `setVar` call in `exec-context-pr-synth/index.ts` is what makes +/// `$(AW_PR_*)` work here. +/// 3. The `$[ ... ]` runtime-expression form is NOT evaluated inside +/// step `env:` values — only inside `variables:` mappings and +/// `condition:` fields. Putting `$[ coalesce(...) ]` in step env +/// passes the literal expression string verbatim to the step +/// (msazuresphere/4x4 build #612528). /// -/// 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 -/// . +/// The fix is to do the real-vs-synth merge in `exec-context-pr-synth.js` +/// (which always runs and always emits the canonical `AW_PR_*` names), +/// and have this step consume them via plain `$(AW_PR_*)` macros — +/// reading the same-job regular variable that `setVar` registered. +/// See . pub fn compile_gate_step_external( ctx: GateContext, checks: &[FilterCheck], @@ -1188,42 +1184,28 @@ pub fn compile_gate_step_external( // Synthetic-from-ci flag: tells gate/bypass.ts that this CI build // 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 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). + // build" bypass must not auto-pass. Read via the same-job macro + // form — `synthPr` always runs and emits AW_SYNTHETIC_PR (=`"true"` + // on synth promotion, empty on real PR / non-promoted CI) via + // `setVar`, so `$(AW_SYNTHETIC_PR)` resolves cleanly without any + // runtime expression. let pr_synth_active = synthetic_pr_active && matches!(ctx, GateContext::PullRequest); if pr_synth_active { - step.push_str( - " AW_SYNTHETIC_PR: \"$[ coalesce(variables['AW_SYNTHETIC_PR'], '') ]\"\n", - ); + step.push_str(" AW_SYNTHETIC_PR: $(AW_SYNTHETIC_PR)\n"); } for (env_var, ado_macro) in &exports { let macro_str = if pr_synth_active { - // 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. + // Read the canonical `AW_PR_*` variables that `synthPr` + // always emits via `setVar` (same-job). On real PR builds + // `synthPr` copies `SYSTEM_PULLREQUEST_*` into them; on + // synth-promoted CI builds it discovers + emits them. The + // merge happens inside the bundle, so this step is blind + // to the source — single macro form, single code path. match *env_var { - "ADO_PR_ID" => { - "\"$[ coalesce(variables['System.PullRequest.PullRequestId'], variables['AW_SYNTHETIC_PR_ID']) ]\"" - } - "ADO_SOURCE_BRANCH" => { - "\"$[ coalesce(variables['System.PullRequest.SourceBranch'], variables['AW_SYNTHETIC_PR_SOURCEBRANCH']) ]\"" - } - "ADO_TARGET_BRANCH" => { - "\"$[ coalesce(variables['System.PullRequest.TargetBranch'], variables['AW_SYNTHETIC_PR_TARGETBRANCH']) ]\"" - } + "ADO_PR_ID" => "$(AW_PR_ID)", + "ADO_SOURCE_BRANCH" => "$(AW_PR_SOURCEBRANCH)", + "ADO_TARGET_BRANCH" => "$(AW_PR_TARGETBRANCH)", _ => ado_macro, } } else { diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index 1811158f..3b9ca279 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -3264,6 +3264,101 @@ fn assert_valid_yaml(compiled: &str, fixture_name: &str) { ); } +/// Assert that no step's `env:` block contains a `$[ ... ]` ADO runtime +/// expression. ADO ONLY evaluates `$[ ... ]` inside `variables:` mappings +/// and `condition:` fields — putting one in step `env:` passes the +/// literal expression string verbatim to the step. This caused +/// msazuresphere/4x4 build #612528 where downstream PR-identifier +/// validation rejected a `PR_ID='$[ coalesce(variables['System.Pull…'` +/// literal as "not a positive integer". +/// +/// Walks every step under every job (`extends.parameters.stages[*].jobs[*]`, +/// `jobs[*]`, `stages[*].jobs[*]`) and inspects the `env:` map at the +/// step level. Job-level `variables:` and `condition:` are correctly +/// skipped — those are the legitimate places for `$[ ... ]`. +fn assert_no_dollar_bracket_in_step_env(compiled: &str) { + let yaml_content: String = compiled + .lines() + .skip_while(|line| line.starts_with('#') || line.is_empty()) + .collect::>() + .join("\n"); + let doc: serde_yaml::Value = + serde_yaml::from_str(&yaml_content).expect("compiled YAML must parse"); + + let mut findings: Vec = Vec::new(); + walk_steps(&doc, "$", &mut |step, path| { + let env = step + .as_mapping() + .and_then(|m| m.get(serde_yaml::Value::String("env".into()))) + .and_then(|v| v.as_mapping()); + let Some(env) = env else { + return; + }; + for (k, v) in env { + let value_str = match v { + serde_yaml::Value::String(s) => s.clone(), + _ => continue, + }; + if value_str.contains("$[") { + let key_str = k.as_str().unwrap_or(""); + let display = step + .as_mapping() + .and_then(|m| m.get(serde_yaml::Value::String("displayName".into()))) + .and_then(|v| v.as_str()) + .unwrap_or(""); + findings.push(format!( + " at {path} (step '{display}'): env.{key_str} = {value_str:?}" + )); + } + } + }); + assert!( + findings.is_empty(), + "Found `$[ ... ]` ADO runtime expressions inside step env blocks. \ + ADO only evaluates these in `variables:` mappings and `condition:` \ + fields, NOT in step env values — the literal expression string is \ + passed verbatim to the step (msazuresphere/4x4 build #612528). \ + Use a job-level `variables:` hoist + `$(name)` macro instead.\n{}", + findings.join("\n") + ); +} + +/// Helper: walk every step in the YAML document, invoking `f` with each +/// step mapping and a slash-delimited path describing where it was found. +fn walk_steps( + doc: &serde_yaml::Value, + path: &str, + f: &mut F, +) { + use serde_yaml::Value; + match doc { + Value::Mapping(m) => { + // If this is a job with `steps:`, visit each step. + if let Some(Value::Sequence(steps)) = + m.get(Value::String("steps".into())) + { + for (i, step) in steps.iter().enumerate() { + let step_path = format!("{path}/steps[{i}]"); + f(step, &step_path); + } + } + // Recurse into common containers that might hold further jobs. + for (k, v) in m { + let key = k.as_str().unwrap_or("?"); + let child_path = format!("{path}/{key}"); + walk_steps(v, &child_path, f); + } + } + Value::Sequence(s) => { + for (i, v) in s.iter().enumerate() { + let child_path = format!("{path}[{i}]"); + walk_steps(v, &child_path, f); + } + } + _ => {} + } +} + // ─── ado-aw marker step (always-on extension) ─────────────────────────────── /// Assert that compiled YAML carries exactly one `# ado-aw-metadata: {…}` @@ -4466,41 +4561,44 @@ fn test_pr_filter_synth_mode_agent_condition_enforces_gate() { fn test_pr_filter_synth_mode_gate_step_uses_same_job_synth_ref() { let compiled = compile_fixture("pr-filter-tier1-agent.md"); + // Gate step reads `AW_SYNTHETIC_PR` via plain `$(...)` macro — the + // `synthPr` step (in the same Setup job) emits this name via + // `setVar`, registering it in the regular variable namespace. ADO + // `$[ ... ]` runtime expressions are NOT evaluated inside step + // `env:` values, so the previous `$[ coalesce(...) ]` form passed + // the literal expression string through to gate/facts.ts and + // caused `Missing ADO env vars` errors (msazuresphere/4x4 + // build #612528). assert!( - 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)." + compiled.contains("AW_SYNTHETIC_PR: $(AW_SYNTHETIC_PR)"), + "Gate step env must reference the same-job synthPr value via the plain \ + `$(AW_SYNTHETIC_PR)` macro — exec-context-pr-synth's `setVar` emits \ + the regular variable, and `$( )` macros work in step env (whereas \ + `$[ ... ]` runtime expressions don't)." ); // The fixture exercises source-branch and target-branch filters, - // so the synth treatment must appear on those env vars using the - // 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.) + // so the gate-step env vars must reference the canonical `AW_PR_*` + // names that synthPr always emits (resolved real-or-synth values). + // (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: $[ coalesce(variables['System.PullRequest.SourceBranch'], variables['AW_SYNTHETIC_PR_SOURCEBRANCH']) ]" - ), - "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." + compiled.contains("ADO_SOURCE_BRANCH: $(AW_PR_SOURCEBRANCH)"), + "ADO_SOURCE_BRANCH must reference the canonical AW_PR_SOURCEBRANCH variable \ + (emitted by exec-context-pr-synth via setVar — real on PR builds, \ + discovered on synth-promoted CI builds)." ); assert!( - compiled.contains( - "ADO_TARGET_BRANCH: $[ coalesce(variables['System.PullRequest.TargetBranch'], variables['AW_SYNTHETIC_PR_TARGETBRANCH']) ]" - ), - "ADO_TARGET_BRANCH must coalesce the real `System.PullRequest.TargetBranch` \ - variable with the same-job regular variable `AW_SYNTHETIC_PR_TARGETBRANCH`." + compiled.contains("ADO_TARGET_BRANCH: $(AW_PR_TARGETBRANCH)"), + "ADO_TARGET_BRANCH must reference the canonical AW_PR_TARGETBRANCH variable." ); + // Defensive: NO `$[ ... ]` runtime expressions in step env anywhere + // in this compiled output. ADO only evaluates them inside + // `variables:` mappings and `condition:` fields. (The Agent job's + // `variables:` hoist legitimately uses `$[ ... ]` — that's fine + // because it's inside `variables:`, not step env.) + assert_no_dollar_bracket_in_step_env(&compiled); + // The same-job gate step MUST NOT use the broken same-job runtime // expression form `variables['synthPr.X']` (resolves empty) nor the // cross-job `dependencies.Setup.outputs[...]` form (undefined in the @@ -5360,21 +5458,14 @@ fn test_execution_context_pr_emits_prepare_step_and_prompt_supplement() { // The synth-active prepare step uses `condition: succeeded()` and // gates in bash (cross-job `dependencies.Setup.outputs[...]` refs // are ILLEGAL in step-level `condition:` — ADO rejects them with - // "Unrecognized value: 'dependencies'"). The synth flag is - // projected through step `env:` (legal there) so a tiny bash - // prelude can do the gate. - assert!( - compiled.contains( - "if [ \"$BUILD_REASON\" != \"PullRequest\" ] && [ \"$AW_SYNTHETIC_PR\" != \"true\" ]; then" - ), - "Prepare step must include the bash gate that replaces the (illegal) cross-job step condition (mode: synthetic is the default)" - ); + // "Unrecognized value: 'dependencies'"). `synthPr` now always runs + // and emits a single resolved `AW_PR_ID` (real on PR builds, + // discovered on synth-promoted CI builds, empty otherwise), so the + // gate collapses to a single empty-check. assert!( - 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)" + compiled.contains("if [ -z \"$AW_PR_ID\" ]; then"), + "Prepare step must include the bash gate on empty AW_PR_ID (replaces the previous \ + BUILD_REASON + AW_SYNTHETIC_PR pair — the merge now happens inside synthPr)" ); // The Stage step's own env block must NOT contain a direct // `dependencies.Setup.outputs[...]` reference. (The same expression @@ -5397,17 +5488,22 @@ fn test_execution_context_pr_emits_prepare_step_and_prompt_supplement() { }) .unwrap_or(""); assert!( - !stage_step.contains("dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR']"), + !stage_step.contains("dependencies.Setup.outputs['synthPr."), "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 \ + `dependencies.Setup.outputs[...]` — that is cross-job syntax. 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}" + `$(AW_PR_*)` macros. Stage step body: {stage_step}" ); + // ADO does NOT evaluate `$[ ... ]` runtime expressions inside step + // `env:` values — only inside `variables:` mappings and + // `condition:` fields. Any `$[ ` in this step's env block would be + // passed through to bash as a literal string (the bug fixed here). assert!( - compiled.contains("BUILD_REASON: $(Build.Reason)"), - "Prepare step must project Build.Reason through env so the bash gate sees a plain $BUILD_REASON" + !stage_step.contains("$["), + "Stage step's env block must not contain `$[ ` runtime expressions \ + (ADO doesn't evaluate them at step-env scope). Use the Agent-job-level \ + `variables:` hoist + `$(name)` macros instead. Stage step body: {stage_step}" ); assert!( compiled.contains("SYSTEM_ACCESSTOKEN: $(System.AccessToken)"), @@ -5445,21 +5541,17 @@ 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 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:`. + // plain `$(AW_PR_*)` macros that read the Agent-job-level hoisted + // variables (populated from the `synthPr` Setup-job outputs which + // hold the resolved real-or-synth PR identifiers). No `$[ ... ]` + // in step env — see `generate_agent_job_variables` for the hoist. assert!( - 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" + compiled.contains("SYSTEM_PULLREQUEST_PULLREQUESTID: $(AW_PR_ID)"), + "Prepare step must pass the PR id via the hoisted AW_PR_ID job-variable" ); assert!( - 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" + compiled.contains("SYSTEM_PULLREQUEST_TARGETBRANCH: $(AW_PR_TARGETBRANCH)"), + "Prepare step must pass the PR target branch via the hoisted AW_PR_TARGETBRANCH job-variable" ); assert!( compiled.contains("SYSTEM_TEAMPROJECT: $(System.TeamProject)"), @@ -6007,30 +6099,33 @@ fn test_synthetic_pr_default_emits_full_synth_wiring() { // Broadened exec-context-pr gate — now a bash guard rather than a // step-level `condition:` (cross-job `dependencies.Setup.outputs[...]` // refs are ILLEGAL in step `condition:`; ADO rejects them with - // "Unrecognized value: 'dependencies'"). The synth flag is - // projected through step `env:` so the bash prelude can read it. + // "Unrecognized value: 'dependencies'"). `synthPr` now always runs + // and emits a single resolved `AW_PR_ID` (real on PR builds, + // discovered on synth-promoted CI builds), so the gate collapses + // to a single empty-check. assert!( - compiled.contains( - "if [ \"$BUILD_REASON\" != \"PullRequest\" ] && [ \"$AW_SYNTHETIC_PR\" != \"true\" ]; then" - ), - "Fixture A must include the bash gate on exec-context-pr.js (accepts real-PR OR synth-promoted builds)" + compiled.contains("if [ -z \"$AW_PR_ID\" ]; then"), + "Fixture A must include the bash gate on empty AW_PR_ID (replaces the previous \ + BUILD_REASON + AW_SYNTHETIC_PR pair — the merge now happens inside synthPr)" ); 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)" + compiled.contains("SYSTEM_PULLREQUEST_PULLREQUESTID: $(AW_PR_ID)"), + "Fixture A's exec-context-pr step must read the hoisted Agent-job-level \ + AW_PR_ID via the $() macro (NOT a $[ ... ] runtime expression in step env — \ + ADO doesn't evaluate those there, see build #612528)" ); // 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'], '') ]" - ), - "Fixture A must hoist `synthPr.AW_SYNTHETIC_PR` into Agent-job-level \ - `variables:` so step-env consumers can read `$(AW_SYNTHETIC_PR)` safely" - ); + // the cross-job synth outputs (legal scope for `dependencies.X.outputs[...]`). + for name in &["AW_PR_ID", "AW_PR_TARGETBRANCH", "AW_PR_SOURCEBRANCH", "AW_SYNTHETIC_PR"] { + let needle = format!( + "{name}: $[ coalesce(dependencies.Setup.outputs['synthPr.{name}'], '') ]" + ); + assert!( + compiled.contains(&needle), + "Fixture A must hoist `synthPr.{name}` into Agent-job-level `variables:` \ + so step-env consumers can read `$({name})` safely" + ); + } // Agent-job AW_SYNTHETIC_PR_SKIP guard. assert!( @@ -6040,13 +6135,11 @@ fn test_synthetic_pr_default_emits_full_synth_wiring() { // NOTE: this fixture does not declare `on.pr.filters`, so the // Agent-job condition has only the skip guard (no AND-NOT gate // clause). The `eq(synthPr.AW_SYNTHETIC_PR, 'true')` literal is - // therefore expected ONLY in the exec-context-pr.js step's - // env-projected `AW_SYNTHETIC_PR` value (asserted above as the - // `coalesce(dependencies.Setup.outputs[...]', '')` shape) — never - // as an Agent-job OR-arm in the dependsOn condition, which would - // silently bypass the gate for real-PR or synth-PR builds. A - // separate fixture covers the gate-enforced shape when - // `pr.filters` is present. + // therefore expected ONLY in the Agent-job-level `variables:` hoist + // and the cross-job condition arms — never as an Agent-job OR-arm + // in the dependsOn condition, which would silently bypass the gate + // for real-PR or synth-PR builds. A separate fixture covers the + // gate-enforced shape when `pr.filters` is present. // No auto-narrowed CI trigger — `pr.branches.include` lists PR TARGET // branches, and ADO `trigger:` fires on pushes TO listed branches, so