Skip to content

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

Merged
jamesadevine merged 1 commit into
mainfrom
jamesadevine/fix-synth-pr-resolve-unsubstituted-macros
Jun 11, 2026
Merged

fix(ado-script): treat unsubstituted ADO macros as empty in synthPr#975
jamesadevine merged 1 commit into
mainfrom
jamesadevine/fix-synth-pr-resolve-unsubstituted-macros

Conversation

@jamesadevine

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to #972. The first roll-out of the synthPr real-PR shortcut had a
runtime bug — ADO leaves undefined predefined-variable macros as the
literal string $(name) in step env (it does NOT substitute to empty).
The bundle read

SYSTEM_PULLREQUEST_PULLREQUESTID = "$(System.PullRequest.PullRequestId)"

on every non-PR build, saw a non-empty string, and went down the
real-PR shortcut path. Observed by @jamesadevine in the logs:

[synth-pr] real PR build #$(System.PullRequest.PullRequestId);
 propagating SYSTEM_PULLREQUEST_* to AW_PR_*

The bundle then emitted AW_PR_ID = $(System.PullRequest.PullRequestId) as
a literal, and downstream PR-identifier validation rejected it (same
class of failure as build 612528, just one level deeper).

Fix

Add a resolveAdoMacroEnv helper that treats literal $(name) strings
as empty:

function resolveAdoMacroEnv(value: string | undefined): string {
  if (!value) return "";
  if (/^\$\([^()]+\)$/.test(value)) return "";
  return value;
}

Match is on balanced $( ... ) with no nested parens — ADO macro
names never contain parens, so this is precise. Apply to every
SYSTEM_PULLREQUEST_* env read in exec-context-pr-synth/index.ts so
the bundle correctly falls through to the synth-discovery path on
non-PR builds.

Test plan

  • New regression test
    does NOT treat unsubstituted $(System.PullRequest.*) ADO macros as a real PR id
    feeds the exact malformed env from the production log and asserts:
    • "real PR build" log path is NOT taken
    • AW_PR_ID is not contaminated with a literal $(...) string
    • Bundle proceeds to synth-discovery and emits AW_SYNTHETIC_PR_SKIP
      on no match
  • npm test — 28/28 synthPr tests pass (1 new, 12 existing).
  • npm run build:exec-context-pr-synth — bundle rebuilt successfully.
  • No Rust-side changes required: the compiler-emitted YAML is already
    correct (it just passes the macros as env values); the fix is purely
    defensive handling of ADO's unsubstituted-macro behaviour inside the
    consumer bundle.

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>
@jamesadevine jamesadevine merged commit 1164c5f into main Jun 11, 2026
14 checks passed
jamesadevine added a commit that referenced this pull request Jun 12, 2026
…hPr stubs

Four follow-ups from the post-IR review of native-ado-compiler:

1. Clarify the OutputDecl / auto_is_output contract.
   The original doc-comment in src/compile/ir/output.rs promised the
   compiler auto-rewrites the bash body to add isOutput=true. It does
   not - the IR never introspects step bodies. The flag is an
   informational signal that extension authors must consult and act
   on themselves. Updated output.rs, graph.rs (outputs_needing_is_output
   field doc), and step.rs (BashStep::outputs field doc) to describe
   the real contract: graph pass detects which decls need the flag,
   producer is responsible for emitting it in the vso directive.

   Forgetting isOutput=true on a producer with cross-step consumers
   is now explicitly called out as a silent-failure mode (and the
   PR #956 / PR #975 / synthPr regression history is cited).

2. Implement Coalesce flattening in src/compile/ir/lower.rs.
   The EnvValue::Coalesce doc promised nested Coalesce values flatten
   into a single outer coalesce(...) call. The lowering pass produced
   nested coalesce(coalesce(...)) instead. ADO accepts both, but the
   IR contract now matches behaviour: added flatten_coalesce_into()
   helper used by both lower_env_value and lower_env_value_as_expr_atom
   in their Coalesce arms. New unit test covers the flatten case
   explicitly. No production producer uses nested Coalesce today,
   so lock files are unchanged.

3. Drop dead AW_SYNTHETIC_PR_* legacy declarations.
   src/compile/extensions/ado_script.rs::SYNTH_PR_OUTPUT_NAMES carried
   three entries (AW_SYNTHETIC_PR_ID, AW_SYNTHETIC_PR_SOURCEBRANCH,
   AW_SYNTHETIC_PR_TARGETBRANCH) with a comment claiming filter_ir.rs
   build_gate_step_typed referenced them via OutputRef. filter_ir.rs
   has zero OutputRef usages - it uses EnvValue::pipeline_var. The
   stubs were truly dead. Removed both the entries and the misleading
   comment. Updated the corresponding test in ado_script.rs.

4. Dedupe duplicated paragraph on SYNTH_PR_OUTPUT_NAMES doc-comment.

cargo build / cargo test (1814 unit + integration) / cargo clippy
--all-targets --all-features / cargo test --test bash_lint_tests all
clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jamesadevine added a commit that referenced this pull request Jun 12, 2026
…hPr stubs

Four follow-ups from the post-IR review of native-ado-compiler:

1. Clarify the OutputDecl / auto_is_output contract.
   The original doc-comment in src/compile/ir/output.rs promised the
   compiler auto-rewrites the bash body to add isOutput=true. It does
   not - the IR never introspects step bodies. The flag is an
   informational signal that extension authors must consult and act
   on themselves. Updated output.rs, graph.rs (outputs_needing_is_output
   field doc), and step.rs (BashStep::outputs field doc) to describe
   the real contract: graph pass detects which decls need the flag,
   producer is responsible for emitting it in the vso directive.

   Forgetting isOutput=true on a producer with cross-step consumers
   is now explicitly called out as a silent-failure mode (and the
   PR #956 / PR #975 / synthPr regression history is cited).

2. Implement Coalesce flattening in src/compile/ir/lower.rs.
   The EnvValue::Coalesce doc promised nested Coalesce values flatten
   into a single outer coalesce(...) call. The lowering pass produced
   nested coalesce(coalesce(...)) instead. ADO accepts both, but the
   IR contract now matches behaviour: added flatten_coalesce_into()
   helper used by both lower_env_value and lower_env_value_as_expr_atom
   in their Coalesce arms. New unit test covers the flatten case
   explicitly. No production producer uses nested Coalesce today,
   so lock files are unchanged.

3. Drop dead AW_SYNTHETIC_PR_* legacy declarations.
   src/compile/extensions/ado_script.rs::SYNTH_PR_OUTPUT_NAMES carried
   three entries (AW_SYNTHETIC_PR_ID, AW_SYNTHETIC_PR_SOURCEBRANCH,
   AW_SYNTHETIC_PR_TARGETBRANCH) with a comment claiming filter_ir.rs
   build_gate_step_typed referenced them via OutputRef. filter_ir.rs
   has zero OutputRef usages - it uses EnvValue::pipeline_var. The
   stubs were truly dead. Removed both the entries and the misleading
   comment. Updated the corresponding test in ado_script.rs.

4. Dedupe duplicated paragraph on SYNTH_PR_OUTPUT_NAMES doc-comment.

cargo build / cargo test (1814 unit + integration) / cargo clippy
--all-targets --all-features / cargo test --test bash_lint_tests all
clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant