fix(compile): gate exec-context-pr synth path in bash, not step condition

[jamesadevine]

Summary

Fixes a hard pipeline-validation error that blocked every on.pr.mode: synthetic pipeline from starting the Agent job:

##[error]Unrecognized value: 'dependencies'. Located at position 53 within expression:
'or(eq(variables['Build.Reason'], 'PullRequest'), eq(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], 'true'))'.

Root cause: The synth-active branch of PrContextContributor::prepare_step emitted a cross-job dependencies.Setup.outputs[...] reference inside a step-level condition: field. ADO only allows that syntax in job-level condition:, in variables: mappings, and in step-level env: values (via $[ ... ]). The expression is rejected at pipeline-expansion time, so the build fails before the Agent job runs.

Fix: Project the synth flag through step env: (legal there) and gate in the bash body. The synth-inactive branch is unchanged — its condition only reads variables[...], which IS legal at step level.

Emitted YAML (synth-active, after fix)

- bash: |
    set -euo pipefail
    if [ "$BUILD_REASON" != "PullRequest" ] && [ "$AW_SYNTHETIC_PR" != "true" ]; then
      echo "[aw-context] Not a PR build and not synth-promoted; skipping exec-context-pr."
      exit 0
    fi
    node '/tmp/ado-aw-scripts/ado-script/exec-context-pr.js'
  env:
    SYSTEM_ACCESSTOKEN: $(System.AccessToken)
    SYSTEM_PULLREQUEST_PULLREQUESTID: "$[ coalesce(variables['System.PullRequest.PullRequestId'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_ID']) ]"
    SYSTEM_PULLREQUEST_TARGETBRANCH: "$[ coalesce(variables['System.PullRequest.TargetBranch'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_TARGETBRANCH']) ]"
    SYSTEM_TEAMPROJECT: $(System.TeamProject)
    BUILD_REPOSITORY_NAME: $(Build.Repository.Name)
    BUILD_SOURCESDIRECTORY: $(Build.SourcesDirectory)
    BUILD_REASON: $(Build.Reason)
    AW_SYNTHETIC_PR: "$[ coalesce(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], '') ]"
  displayName: "Stage PR execution context (aw-context/pr/*)"
  condition: succeeded()

coalesce(..., '') ensures the bash gate sees "" (not an unresolved $() literal) when synthPr skipped itself on a real-PR build. Variables are double-quoted for shellcheck cleanliness and set -u safety.

Regression-trap test (bug-class protection)

In addition to fixing the specific bug, this PR adds test_no_step_condition_references_cross_job_dependencies, which walks compiled YAML across four fixtures (synth-default, exec-context-agent, pr-mode-policy, minimal-agent), distinguishes step vs job/stage mappings, and fails with a pointer to the offending step if any future extension emits a step-level condition: containing dependencies.. This protects every extension that emits a step condition, not just exec_context/pr.

Why a guard at all

exec-context-pr.js runs in the Agent job and must be skipped when there is no PR (real or synthetic) — otherwise it writes a misleading "PR context preparation failed" prompt fragment + aw-context/pr/error.txt. The bash guard preserves that semantics; the only difference vs the old (broken) approach is that the step now shows as succeeded in the ADO UI with a single skip log line, rather than skipped.

Why bash-guard and not job-level variable promotion

A more ADO-native alternative (project the Setup-output through variables: on the Agent job, then reference variables['AW_SYNTHETIC_PR'] in the step condition) was considered. It requires a new CompilerExtension::agent_job_variables() trait method plus a new template marker on the Agent job in four base templates (base.yml, 1es-base.yml, job-base.yml, stage-base.yml). ~100 LOC of plumbing for one consumer is over-engineering today; the trait abstraction can be added later if a second extension needs Setup-output projection.

Test plan

• cargo build — clean
• cargo test --bin ado-aw exec_context::pr — 2/2 pass (updated synth-active test renamed _emits_coalesced_env_and_bash_synth_guard; synth-inactive test unchanged)
• cargo test --test compiler_tests test_execution_context_pr_emits_prepare_step_and_prompt_supplement test_synthetic_pr_default_emits_full_synth_wiring test_no_step_condition_references_cross_job_dependencies — 3/3 pass (including new regression trap)
• cargo test --test bash_lint_tests with ENFORCE_BASH_LINT=1 — shellcheck clean on the new bash guard
• cargo test — full suite green (1813 + 132 + … tests, 0 failures)
• Recompiled a consumer pipeline (pr-reviewer.yml) locally; grep confirms zero step-level condition: fields reference dependencies.
• ADO end-to-end verification pending push of the regenerated YAML to the consumer repo.

[github-actions]

🔍 Rust PR Review

Summary: Looks good — correct diagnosis, minimal surgical fix, thorough test coverage.

Findings

✅ What Looks Good

• Root cause and fix are both correct. ADO step-level condition: genuinely cannot reference dependencies.<Job>.outputs[...]; the error surfaces at pipeline-expansion time, not YAML-parse time, which makes it insidious. Projecting through env: ($[ ... ] is legal there) and gating in bash is exactly the right approach.

• Bash guard logic is sound. The && condition correctly exits only when neither BUILD_REASON == PullRequest nor AW_SYNTHETIC_PR == true, which preserves the original semantics for both real-PR builds and synth-promoted CI builds.

• coalesce(..., '') is the right default. If the Setup job's synthPr step skipped itself on a real PR build, dependencies.Setup.outputs[...] is null. coalesce(null, '') → "" → [ "" != "true" ] is true, but BUILD_REASON == "PullRequest" makes the outer && false, so the script continues. On a plain CI push both conditions are true and the script exits 0. No set -u hazard since the env var is always assigned.

• Generated YAML indentation is consistent. The prelude string uses 4-space leading indent (matching set -euo pipefail) with 6-space interior lines, and synth_env starts with \n (newline + 4 spaces) to stay aligned with the other env: entries. Spot-checked against the expected output in the PR description — matches exactly.

• The regression-trap test is well-designed. Walking parsed YAML and distinguishing step vs. job/stage mappings via step-exclusive keys (bash, script, task, powershell, pwsh, checkout) is a sound heuristic for current ADO YAML. The four fixtures cover synth-active, synth-inactive, filter-gated, and no-PR paths, giving good bug-class coverage across all condition-emitting extensions.

• Cosmetic trade-off is explicitly documented. The step now shows as succeeded (with a single skip log line) rather than skipped in the ADO UI — a minor UI difference that is called out both in code comments and the PR description. Good transparency.

⚠️ Suggestions

• walk heuristic won't catch future uses: steps (if ADO ever adds a resource-step key analogous to GitHub Actions' uses:). Not a problem today, but worth a one-line comment in the test to acknowledge the heuristic's scope, e.g. // step keys as of ADO YAML schema 2.x; extend if ADO adds new step-level keys. Entirely optional — the test is still a strong regression guard as-is.

Generated by Rust PR Reviewer for issue #937 · sonnet46 1M · ◷