fix(ado-script): treat unsubstituted ADO macros as empty in synthPr

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 () => {

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 `$(<anything>)` 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<number
   // 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 ?? "";
+  // Detection: `SYSTEM_PULLREQUEST_PULLREQUESTID` is non-empty (after
+  // `resolveAdoMacroEnv` strips unsubstituted `$(name)` literals) iff
+  // this is a real PR build. `BUILD_REASON=PullRequest` could also be
+  // used as a sanity check, but the resolved id is the actual value we
+  // need to propagate — and it can't be present without the build
+  // really being a PR build.
+  const realPrId = resolveAdoMacroEnv(env.SYSTEM_PULLREQUEST_PULLREQUESTID);
   if (realPrId.length > 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_*`,