fix(compile): unify synthetic-PR variable namespace via ado-script

[jamesadevine]

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, so every
downstream consumer reads a single canonical AW_PR_* macro regardless
of build reason.

Root cause (msazuresphere/4x4 build 612528)

ADO $[ ... ] runtime expressions are evaluated only inside
variables: mappings and condition: fields — never inside step
env: values. The previous compiler emitted

SYSTEM_PULLREQUEST_PULLREQUESTID: "$[ coalesce(variables['System.PullRequest.PullRequestId'], variables['AW_SYNTHETIC_PR_ID']) ]"

directly inside step env in two places (exec_context/pr.rs +
filter_ir.rs). ADO passed the literal expression string verbatim to
bash. Build 612528:

• Stage PR execution context: [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).
• Evaluate PR filters: Fact 'pr_metadata' failed (Missing ADO env vars (ADO_PROJECT/ADO_REPO_ID/ADO_PR_ID) required for fact 'pr_metadata'); dependent checks skipped

Comments at exec_context/pr.rs:149 and compile/common.rs:1127 both
claimed step env supported $[ ... ] — empirically it does not.

Fix

Do the merge inside ado-script, not in step env:

1. exec-context-pr-synth/index.ts — runs unconditionally now. On
   real PR builds, copies SYSTEM_PULLREQUEST_* → AW_PR_*. On
   synth-promoted CI builds, discovers + emits AW_PR_* plus
   AW_SYNTHETIC_PR="true". On soft skips / GitHub repos, emits empty
   AW_PR_* + AW_SYNTHETIC_PR_SKIP="true". Every path emits the
   canonical variable set via both setOutput (cross-job) and
   setVar (same-job).
2. ado_script.rs::synthetic_pr_step — removed condition: ne(Build.Reason, 'PullRequest'); added SYSTEM_PULLREQUEST_* env
   passthrough so the bundle can detect real PR builds.
3. common.rs::generate_agent_job_variables — hoists renamed
   canonical AW_PR_ID / AW_PR_TARGETBRANCH / AW_PR_SOURCEBRANCH
   (+ existing AW_SYNTHETIC_PR flag) from
   dependencies.Setup.outputs['synthPr.*'] (the legitimate $[ ... ]
   location).
4. exec_context/pr.rs + filter_ir.rs — step env now uses
   plain $(AW_PR_*) macros; no $[ ... ] in step env. The bash
   gate collapses from if [ "$BUILD_REASON" != "PullRequest" ] && [ "$AW_SYNTHETIC_PR" != "true" ] to a single
   if [ -z "$AW_PR_ID" ] empty-check.

Regression guard

New assert_no_dollar_bracket_in_step_env test walks every step's
env: block in compiled YAML and asserts no $[ appears. Catches
this entire bug class going forward.

Test plan

• cargo test — 1821 unit tests + 132 compiler integration tests pass
  (5 previously-failing tests rewritten to assert the new contract:
  prepare_step_synth_active_uses_macros_for_hoisted_aw_pr_vars_and_bash_guard,
  setup_steps_emits_synth_step_when_synthetic_pr_active_without_gate,
  test_pr_filter_synth_mode_gate_step_uses_same_job_synth_ref,
  test_execution_context_pr_emits_prepare_step_and_prompt_supplement,
  test_synthetic_pr_default_emits_full_synth_wiring,
  test_generate_agent_job_variables_emits_hoisted_synth_outputs).
• cargo clippy --all-targets --all-features — clean.
• npm test (scripts/ado-script) — 282 tests pass including the
  rewritten synthPr suite covering real-PR / GitHub-repo / synth /
  soft-skip paths.
• npm run test:smoke — 6 smoke tests pass.
• Manual compile of pr-filter-tier1-agent.md confirms $[ ... ]
  appears only inside the Agent job's variables: block (4 hoists)
  and never in any step env:.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

[github-actions]

🔍 Rust PR Review

Summary: Looks good — the root cause diagnosis is correct and the fix is architecturally sound. Two minor issues worth addressing.

---

Findings

⚠️ Suggestions

• scripts/ado-script/src/exec-context-pr-synth/index.ts:338 — AW_PR_IS_DRAFT emitted but not hoisted to Agent job

  emitPrIdentifiers emits AW_PR_IS_DRAFT via both setOutput and setVar, and the TypeScript doc comment lists it as a first-class output variable. However generate_agent_job_variables (src/compile/common.rs:1168) only hoists AW_PR_ID, AW_PR_TARGETBRANCH, AW_PR_SOURCEBRANCH, and AW_SYNTHETIC_PR. Any Agent-job step that reads $(AW_PR_IS_DRAFT) will get empty. Nothing currently consumes it from the Agent job, so this isn't a live breakage — but it's an API promise (AW_PR_IS_DRAFT in the contract table) that silently doesn't work cross-job. Either add it to the hoist alongside the other three identifiers, or remove it from the doc-comment contract and limit its use to Setup-job steps.

• tests/compiler_tests.rs:1097 — assert_no_dollar_bracket_in_step_env only called from one test

  The helper is defined as a regression guard against the exact bug class this PR fixes, but it's only invoked in test_pr_filter_synth_mode_gate_step_uses_same_job_synth_ref. The two other synth-active integration tests that previously contained the broken $[ coalesce(...) ] forms — test_synthetic_pr_default_emits_full_synth_wiring and test_execution_context_pr_emits_prepare_step_and_prompt_supplement — don't call it. Adding the call at the end of both tests would mean the guard covers all three representative fixtures rather than just one.

---

✅ What Looks Good

• Root cause + fix are correct. $[ ... ] genuinely is not evaluated in step env: values; moving the real-vs-synth merge into the TypeScript bundle and having every downstream consumer use plain $(AW_PR_*) macros is the right architecture.
• SYSTEM_PULLREQUEST_PULLREQUESTID presence over BUILD_REASON for real-PR detection is more precise and directly checks the value that matters.
• emitPrIdentifiers("", "", "", "") before every emitSkip path ensures AW_PR_* variables are always defined as stable empty strings for same-job consumers, preventing the undefined-macro → literal-string hazard.
• Bash gate simplification from BUILD_REASON != PullRequest && AW_SYNTHETIC_PR != true to [ -z "$AW_PR_ID" ] is cleaner and correctly handles both real-PR and synth-promoted paths with a single check.
• Regression guards against the old AW_SYNTHETIC_PR_ID/TARGETBRANCH/SOURCEBRANCH names in test_generate_agent_job_variables_emits_hoisted_synth_outputs are a good catch.
• Doc strings throughout are thorough and directly cite the build evidence — this will save the next person from rediscovering the ADO constraint.

Generated by Rust PR Reviewer for issue #972 · sonnet46 1.7M · ◷