From 6f12a00b9e41831160ec41def0fff9dfdc5aaaec Mon Sep 17 00:00:00 2001 From: James Devine Date: Thu, 11 Jun 2026 17:45:39 +0100 Subject: [PATCH] fix(ado-script): treat unsubstituted ADO macros as empty in synthPr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADO leaves undefined predefined-variable macros as the literal string `$(name)` in step env — it does NOT substitute to empty. The first roll-out of the synthPr real-PR shortcut read `SYSTEM_PULLREQUEST_PULLREQUESTID = "$(System.PullRequest.PullRequestId)"` on every non-PR build and treated it as a non-empty real PR id, so it emitted the literal macro as AW_PR_ID and logged: [synth-pr] real PR build #$(System.PullRequest.PullRequestId); propagating SYSTEM_PULLREQUEST_* to AW_PR_* Add `resolveAdoMacroEnv` helper that strips literal `$(name)` strings to empty (matching balanced `$(` ... `)` with no nested parens — ADO macro names never contain parens). Apply to every `SYSTEM_PULLREQUEST_*` env read so the bundle correctly falls through to the synth-discovery path on non-PR builds. New regression test covers the macro-literal env case, asserting the "real PR build" log path is NOT taken and AW_PR_ID is not contaminated with a literal `$(...)` string. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../__tests__/index.test.ts | 35 ++++++++++++++++ .../src/exec-context-pr-synth/index.ts | 41 +++++++++++++++---- 2 files changed, 67 insertions(+), 9 deletions(-) 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 f65dba95..ec618187 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 @@ -80,6 +80,41 @@ describe("exec-context-pr-synth main", () => { expect(output).toContain("AW_PR_ID;isOutput=true]99"); }); + it("does NOT treat unsubstituted $(System.PullRequest.*) ADO macros as a real PR id", async () => { + // Regression: ADO leaves undefined predefined-variable macros as + // the literal string `$(name)` in step env (it does NOT substitute + // to empty). Without resolveAdoMacroEnv, the bundle would treat + // `SYSTEM_PULLREQUEST_PULLREQUESTID = "$(System.PullRequest.PullRequestId)"` + // (the value seen on every non-PR build) as a real PR id and + // mis-emit `AW_PR_ID = $(System.PullRequest.PullRequestId)` as a + // literal — observed by @jamesadevine on the first roll-out: + // `[synth-pr] real PR build #$(System.PullRequest.PullRequestId); + // propagating SYSTEM_PULLREQUEST_* to AW_PR_*` + // The bundle must instead fall through to the synth-discovery path. + mocked.listActivePullRequestsBySourceRef.mockResolvedValue([]); + const { code, output } = await runMain( + makeEnv({ + BUILD_REASON: "IndividualCI", + 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: build_pr_synth_spec(), + }), + ); + expect(code).toBe(0); + // The "real PR build" log must NOT appear — the macro literals + // should have been resolved to empty and the bundle should have + // proceeded to the synth-discovery path (which then no-matched). + expect(output).not.toContain("real PR build"); + // AW_PR_ID must NOT carry the literal macro string. + expect(output).not.toContain("AW_PR_ID;isOutput=true]$("); + expect(output).not.toContain("##vso[task.setvariable variable=AW_PR_ID]$("); + // Should hit the synth-discovery path and emit SKIP on no match. + expect(mocked.listActivePullRequestsBySourceRef).toHaveBeenCalledOnce(); + expect(output).toContain("AW_SYNTHETIC_PR_SKIP;isOutput=true]true"); + }); + // ── GitHub repo path ──────────────────────────────────────────── it("skips with empty AW_PR_* on GitHub-typed repos (CI builds)", 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 fb853bfe..2ae3e95e 100644 --- a/scripts/ado-script/src/exec-context-pr-synth/index.ts +++ b/scripts/ado-script/src/exec-context-pr-synth/index.ts @@ -71,6 +71,28 @@ import { decodeSpec, type PrSynthSpec } from "./spec.js"; const SKIP_OUTPUT = "AW_SYNTHETIC_PR_SKIP"; +/** + * Resolve an ADO step-env value that may carry an unsubstituted + * `$(name)` macro. ADO leaves undefined predefined-variable macros as + * the literal string `$(Some.Variable.Name)` in step env — it does NOT + * substitute to empty. Without this guard, the bundle would read + * `SYSTEM_PULLREQUEST_PULLREQUESTID = "$(System.PullRequest.PullRequestId)"` + * on a non-PR build and treat it as a non-empty PR id (regression + * observed by @jamesadevine on the first roll-out: + * `[synth-pr] real PR build #$(System.PullRequest.PullRequestId); + * propagating SYSTEM_PULLREQUEST_* to AW_PR_*`). + * + * Returns the actual value when set, empty string when the value is + * absent, empty, or a literal `$()` macro. The macro pattern + * uses balanced `$(` ... `)` with no nested parens (ADO macro + * names never contain parens). + */ +function resolveAdoMacroEnv(value: string | undefined): string { + if (!value) return ""; + if (/^\$\([^()]+\)$/.test(value)) return ""; + return value; +} + /** * Emit the canonical AW_PR_* identifier set as BOTH cross-job outputs * (consumed by the Agent job's `variables:` hoist via @@ -111,18 +133,19 @@ export async function main(env: NodeJS.ProcessEnv = process.env): Promise 0) { emitPrIdentifiers( realPrId, - env.SYSTEM_PULLREQUEST_TARGETBRANCH ?? "", - env.SYSTEM_PULLREQUEST_SOURCEBRANCH ?? "", - env.SYSTEM_PULLREQUEST_ISDRAFT ?? "", + resolveAdoMacroEnv(env.SYSTEM_PULLREQUEST_TARGETBRANCH), + resolveAdoMacroEnv(env.SYSTEM_PULLREQUEST_SOURCEBRANCH), + resolveAdoMacroEnv(env.SYSTEM_PULLREQUEST_ISDRAFT), ); logInfo( `[synth-pr] real PR build #${realPrId}; propagating SYSTEM_PULLREQUEST_* to AW_PR_*`,