feat(compile): make on.pr a first-class trigger via synthetic CI-derived PR context

[jamesadevine]

Summary

Closes #916.

Makes on: pr a first-class trigger in Azure Repos by deriving PR context from the ADO REST API on CI-triggered builds. No Build Validation branch policy required.

Why

Azure DevOps Services ignores the YAML pr: block unless a per-branch Build Validation policy is registered server-side. Without that policy, a git push to a feature branch fires the compiled pipeline as Build.Reason = IndividualCI even when an open PR exists, so:

• scripts/ado-script/src/gate/bypass.ts takes the "Not a Pull Request build — gate passes automatically" path and never gets PR identifiers.
• scripts/ado-script/src/exec-context-pr/index.ts is conditioned on eq(Build.Reason, 'PullRequest') and is skipped entirely, so aw-context/pr/{base.sha,head.sha} and the agent-prompt PR-context fragment are never staged.
• PR-aware agents (e.g. PR reviewers) silently degrade.

This defeated the "one markdown file is the whole workflow" promise — registering an ado-aw agent shouldn't require a separate ADO-UI click for every target branch.

What

Adds a new front-matter knob on.pr.mode (default synthetic). The two modes give the author a single choice between "no policy required" and "policy installed":

on.pr.mode	Synthesis wiring	Top-level trigger:	Use when
synthetic (default)	emitted	ADO default (all branches)	No branch policy. The vast majority of agents.
policy	omitted	trigger: none	Operator installed a Build Validation branch policy. Real PR-typed builds only.

When mode: synthetic (the default):

1. Setup-job script exec-context-pr-synth.js queries the ADO REST API at build time:

   • BUILD_REASON == PullRequest → no-op (real PR build owns the path).
   • BUILD_REPOSITORY_PROVIDER == GitHub → no-op (GitHub repos already get correct pr: semantics).
   • Decode PR_SYNTH_SPEC (base64 JSON of pr.branches + pr.paths).
   • Fetch active PRs by sourceRefName == Build.SourceBranch, filter by targetRefName matching pr.branches.
   • Exactly one match → check pr.paths against the iteration-API change list. Otherwise → emit AW_SYNTHETIC_PR_SKIP=true with reason, agent self-skips cleanly.
   • On match: emit AW_SYNTHETIC_PR=true + AW_SYNTHETIC_PR_ID/SOURCEBRANCH/TARGETBRANCH/IS_DRAFT as Setup-job outputs.

2. Downstream env coalescing: gate.js and exec-context-pr.js env blocks switch ADO_PR_ID, ADO_SOURCE_BRANCH, ADO_TARGET_BRANCH, SYSTEM_PULLREQUEST_* to $[ coalesce(variables['System.PullRequest.*'], dependencies.Setup.outputs['synthPr.*']) ] so real PR builds remain bit-identical while CI-triggered builds pick up the synthesised identifiers.

3. Gate bypass update: gate/bypass.ts checks AW_SYNTHETIC_PR === 'true' and skips the "not a PR build" auto-pass when the synthPr step promoted the run.

4. Agent-job condition update: dependsOn condition gains an AW_SYNTHETIC_PR_SKIP guard and a broadened PR clause accepting real-PR / synthetic-promotion / gate-passed.

When mode: policy: none of the synth wiring above is emitted; the compiler additionally emits trigger: none so feature-branch pushes do not queue duplicate CI builds alongside the policy-driven PR build. Every PR update fires exactly one PR-typed build.

Why the CI trigger is intentionally not auto-narrowed in mode: synthetic

An earlier iteration auto-emitted a top-level trigger: block mirroring pr.branches.include to spare unrelated branches from queueing self-skipping builds. That was a logic bug: pr.branches.include lists PR target branches (e.g. main), but ADO trigger: fires on pushes to the listed branches. Narrowing to [main] suppressed CI on the feature branches synthPr actually needs to react to — pushing to feature/x with an open PR feature/x → main would never queue a build, defeating the entire synthetic-from-ci feature. The compiler therefore leaves the top-level trigger: at the ADO default ("trigger on every branch") in synth mode, and relies on the synthPr Setup step's existing fast-exit for cost control: a single listActivePullRequestsBySourceRef call returns [] on branches without a matching PR and the Agent job self-skips via AW_SYNTHETIC_PR_SKIP=true.

Decisions frozen in design

Decision	Value
Knob name	on.pr.mode, default synthetic (alternative: policy)
Match rule	Exactly one open PR by sourceRefName whose targetRefName matches pr.branches.include/exclude
Path filtering	Enforced against iteration-API change list (ADO ignores pr.paths on CI)
Real PR builds	Bit-identical — synth step no-ops via step-level condition + runtime check
GitHub-typed repos	Runtime no-op (compiler emits identical YAML; bundle detects BUILD_REPOSITORY_PROVIDER=GitHub)
Skip path	Single logInfo line + AW_SYNTHETIC_PR_SKIP=true. Never red, never noisy.

Hard prerequisite

#915 (the addBuildTag("pr-gate:passed") colon bug) had to land first — every synthetic-PR gate path would otherwise die on the build-tag REST call. Already merged via #917 (4122a640).

Test plan

Automated (all green)

Surface	Before	After
Rust bin tests	1797	1811
Rust integration tests (compiler_tests)	124	129
Script unit tests (vitest)	251	281
Script smoke tests	6	6

Highlights:

• cargo test --bins — 1811 pass, 0 fail
• cargo test --test compiler_tests — 129 pass, 0 fail (includes 2 new snapshot fixtures: synthetic-pr-default.md asserts full synth wiring with no narrowed CI trigger; pr-mode-policy.md asserts the policy mode emits zero synth artefacts AND trigger: none)
• 26 new vitest cases in scripts/ado-script/src/exec-context-pr-synth/__tests__/ (match.ts glob semantics + index.ts runtime contract with mocked ADO client: real-PR no-op, GitHub no-op, spec decode errors, zero/multi-match skips, path-filter rejection, happy path, draft PR)
• 4 new gate bypass tests (AW_SYNTHETIC_PR=true suppresses bypass for PR specs only)
• 4 schema deserialisation tests for on.pr.mode (default→synthetic, explicit synthetic, explicit policy, invalid value rejected)
• 3 build_pr_synth_spec tests (round-trip, empty arrays, 8 KiB size cap)
• 2 generate_agentic_depends_on tests for the synth-skip + broader PR clause
• 3 generate_ci_trigger tests covering the two-mode contract (synth mode keeps ADO default, policy mode emits trigger: none, pipeline-completion trigger still wins)
• 2 new AdoScriptExtension::setup_steps tests for synth-step emission (without and with a gate)
• Bash-step lint passes (the new synth step is a one-line node … under set -euo pipefail)

Regression sweep

Recompiled 29 of 33 existing fixtures (the other 4 — runtime_imports_author_marker_* — expectedly need sibling files only the test harness copies). Confirmed:

• 6 fixtures with on.pr correctly carry the new synth wiring
• 23 fixtures without on.pr show zero synth artefacts
• Zero unintended diffs across non-on.pr agents

Manual end-to-end (deferred to a separate task)

Per the implementation plan, the end-to-end demo against msazuresphere/4x4/azure-devops-agentic-pipelines PR #38551 (reviewer pipeline runs without a Build Validation branch policy, producing review comments + ado-aw audit <buildId> showing a clean aw-context/pr/{base.sha,head.sha} stage and no missing-PR-context warnings) will be performed after merge. The 12-commit history is deliberately reviewable in isolation.

Files changed

• Schema: src/compile/types.rs — PrTriggerConfig.synthetic_from_ci
• Compiler: src/compile/{filter_ir,common,pr_filters,extensions/ado_script,extensions/exec_context/{mod,pr},extensions/mod}.rs
• Runtime bundle: scripts/ado-script/src/exec-context-pr-synth/{index,match,spec}.ts + tests + harness; scripts/ado-script/src/{gate/bypass,shared/ado-client}.ts
• Build: scripts/ado-script/package.json + .gitignore; .github/workflows/release.yml (pack the new bundle into ado-script.zip)
• Tests: tests/compiler_tests.rs + tests/fixtures/synthetic-pr-{default,opt-out}.md
• Docs: docs/front-matter.md (new "PR Triggering in Azure Repos" section); prompts/{create,update,debug}-ado-agentic-workflow.md cross-links

[github-actions]

🔍 Rust PR Review

Summary: The design and implementation are solid, but there is one logic bug in the CI trigger narrowing that would silently break the feature for the typical PR workflow (feature branch → PR → main), plus a minor injection surface to verify.

---

Findings

🐛 Bugs / Logic Issues

• src/compile/common.rs generate_ci_trigger — narrowed trigger: uses target branches, not source branches

  The PR's synthetic-from-ci mechanism works by:

  1. A developer pushes to feature/x → ADO fires CI for feature/x (Build.SourceBranch = refs/heads/feature/x)
  2. exec-context-pr-synth.js fetches active PRs whose sourceRefName = refs/heads/feature/x, filters by targetRefName matching pr.branches.include
  3. On a match, the build is promoted to PR semantics

  For step 1 to happen, the compiled pipeline's trigger: block must fire for feature/x. But generate_ci_trigger emits:

  trigger:
    branches:
      include:
        - 'main'    # pr.branches.include — these are PR TARGET branches

  ADO trigger: fires on pushes to the listed branches. With include: [main], only pushes to main trigger the pipeline. Pushes to feature/x are suppressed, so exec-context-pr-synth.js never runs on those builds.

  The test harness confirms the intended runtime contract: BUILD_SOURCEBRANCH = refs/heads/feature/x is the default in harness.ts::makeEnv, and the index tests simulate CI builds on feature branches. But those CI builds will never occur if the compiled trigger only includes target branches.

  Fix: Remove the trigger narrowing in branch 2 of generate_ci_trigger. Without an explicit trigger: block, ADO defaults to all-branches CI, which is what synthPr needs. The AW_SYNTHETIC_PR_SKIP fast-exit in the Setup job already handles wasted builds cheaply (one listActivePullRequestsBySourceRef call that returns [] for branches with no PRs). Alternatively, the narrowing should be documented as an opt-in trade-off where users accept that only direct-to-target-branch pushes get synthPr processing.

  The five generate_ci_trigger tests and the synthetic-pr-default.md snapshot fixture all pass because they test the emitted YAML, not the runtime effect of narrowing on feature branch builds.

🔒 Security Concerns

• scripts/ado-script/src/exec-context-pr-synth/index.ts:163-167 — setOutput with API-derived branch names

  setOutput("AW_SYNTHETIC_PR_TARGETBRANCH", pr.targetRefName ?? "");
  setOutput("AW_SYNTHETIC_PR_SOURCEBRANCH", pr.sourceRefName ?? sourceBranch);

  pr.targetRefName and pr.sourceRefName come directly from the ADO REST API. If setOutput in vso-logger.ts doesn't sanitise values for ##vso[...] command injection, a repository owner who controls branch names could craft refs/heads/main##vso[task.setSecret]secretValue to inject logging commands. This is the same class of risk the project already guards against for addBuildTag's colon (fix(gate): use '.' separator in build tags so ADO doesn't reject ':' in REST path #917).

  ADO branch naming rules in practice make real-world exploitation unlikely, but it's worth confirming that vso-logger.ts::setOutput applies the same logInfo-style sanitisation used for bypass_label in bypass.ts, or add explicit value sanitisation here. (Low priority.)

---

✅ What Looks Good

• Spec validation is hardened in the right direction: decodeSpec in spec.ts hard-fails on any malformed PR_SYNTH_SPEC rather than soft-skipping — correct, since treating spec corruption as a skip would silently widen the synth attack surface.
• build_pr_synth_spec size cap matches the GATE_SPEC ceiling (8 KiB); the oversize test with 1000 branch globs is a solid regression guard.
• Coalesce expressions make real PR builds bit-identical: when synthPr step doesn't run (Build.Reason = PullRequest), coalesce(null, '') returns '', leaving real System.PullRequest.* variables in play.
• synthetic_pr_active is true but pr_trigger_for_synth is None invariant check in AdoScriptExtension::setup_steps — good defensive programming for the compile-time structural contract.
• Base64-encoded PR_SYNTH_SPEC is safe to embed in a YAML double-quoted scalar (base64 alphabet [A-Za-z0-9+/=] has no YAML double-quoted special characters).
• AW_SYNTHETIC_PR_SKIP guard placement in generate_agentic_depends_on is correct: it's evaluated first (ne(synthPr.AW_SYNTHETIC_PR_SKIP, 'true') before the or(Build.Reason, ...) clause) ensuring a synth-skip decision is always honoured over other activation reasons.
• Step-level condition on synthPr step (ne(Build.Reason, 'PullRequest')) is the right defence-in-depth: even if the downstream coalesce expressions were wrong, the step would never execute on a real PR build.
• Test coverage is comprehensive: round-trip spec tests, runtime contract tests with mocked ADO client (real-PR no-op, GitHub no-op, zero/multi-match skips, path-filter rejection, happy path, draft PR), snapshot fixtures with substring-negation back-compat guard.

Generated by Rust PR Reviewer for issue #922 · sonnet46 3.4M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Needs changes — good architecture overall, but one logic regression and one misleading doc fragment.

Findings

🐛 Bugs / Logic Issues

• src/compile/common.rs:2409-2417 — filter gate bypassed for real PR builds when synthetic-from-ci: true

  When synthetic_pr_active=true (the default for any on.pr: config) and has_pr_filters=true, the new agent-job dependsOn condition is:

  or(
    eq(variables['Build.Reason'], 'PullRequest'),    // arm a — unconditional for real PRs
    eq(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], 'true'),   // arm b
    eq(dependencies.Setup.outputs['prGate.SHOULD_RUN'], 'true')          // arm c
  )
  

  Arm (a) is unconditionally true on every real PR build. This is a regression from the original condition:

  or(
    ne(variables['Build.Reason'], 'PullRequest'),  // non-PR always passes
    eq(dependencies.Setup.outputs['prGate.SHOULD_RUN'], 'true')  // PR requires gate
  )
  

  The original required SHOULD_RUN=true for real PR builds; the new condition does not.

  When it bites: the gate soft-skips when SYSTEM_ACCESSTOKEN is unavailable — it sets SHOULD_RUN=false and exits 0 rather than cancelling the build. In that scenario a real PR with a mismatching title/label/draft filter will have succeeded()=true and arm(a)=true, so the agent runs despite the filter saying no. Since synthetic-from-ci defaults to true, this affects all existing agents with on.pr + filters:.

  Suggested fix — require the gate to pass in both PR paths:

  // synthetic_pr_active && has_pr_filters
  parts.push(
      r"and(
       or(
         eq(variables['Build.Reason'], 'PullRequest'),
         eq(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], 'true')
       ),
       eq(dependencies.Setup.outputs['prGate.SHOULD_RUN'], 'true')
     )"
      .to_string(),
  );

  This preserves "run if (real PR or synthetic) AND gate passed" rather than treating either status as an unconditional pass.

📄 Documentation Bug

• prompts/update-ado-agentic-workflow.md:51 — incorrect claim:

  auto-narrows the top-level trigger: block to those branches.

  The code does not do this — there are tests (test_generate_ci_trigger_synthetic_pr_does_not_narrow) and a multi-paragraph design note in docs/front-matter.md explaining exactly why CI-trigger narrowing was removed as a logic bug. This sentence is a leftover from an earlier iteration. The create-ado-agentic-workflow.md prompt got this right; update-ado-agentic-workflow.md didn't.

⚠️ Suggestions

• scripts/ado-script/src/shared/ado-client.ts:106 — comment says "first page (200 PRs)" but getPullRequests is called without a top argument; the azure-devops-node-api default is 100. Behaviour is correct (exactly-one contract catches pathological cases anyway), but the comment is misleading.

✅ What Looks Good

• bypass.ts: the synthetic guard requires both spec.context.build_reason === "PullRequest" and AW_SYNTHETIC_PR === "true", so a stray env var cannot suppress bypass on a pipeline-completion gate spec.
• spec.ts treats decode corruption as a hard error rather than a soft skip — correct, since silently widening the matched-branch set would be a security gap.
• synthetic_pr_step() has a step-level ne(Build.Reason, 'PullRequest') condition so real PR builds see zero overhead.
• PrTriggerConfig uses a hand-written Default impl delegating to pr_synthetic_from_ci_default() — avoids the #[derive(Default)] footgun where a non-false default becomes false.
• 8 KiB cap on PR_SYNTH_SPEC mirrors GATE_SPEC — good defence-in-depth against pathological front-matter.
• match.ts strips the refs/heads/ prefix before picomatch comparison, so main and refs/heads/main both work in on.pr.branches.include.

Generated by Rust PR Reviewer for issue #922 · sonnet46 3.6M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Ambitious and well-designed feature — architecture is clean, test coverage is strong, but there are two runtime correctness concerns about how ADO evaluates the synthesised env-block expressions that should be verified before merge.

Findings

🐛 Bugs / Logic Issues

1. Gate step uses a cross-job output reference within the same job (src/compile/filter_ir.rs, compile_gate_step_external)

The gate step is emitted into the Setup job (returned from AdoScriptExtension::setup_steps). When synthetic_pr_active = true, the env block gains:

AW_SYNTHETIC_PR: $[ coalesce(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], '') ]
ADO_PR_ID: $[ coalesce(variables['System.PullRequest.PullRequestId'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_ID']) ]

dependencies.Setup.outputs['synthPr.X'] is cross-job syntax — it resolves correctly in downstream jobs (the Agent job), but within the Setup job itself the synthPr step's isOutput=true variables are accessible as $(synthPr.AW_SYNTHETIC_PR) (macro) or $[ steps.synthPr.outputs['AW_SYNTHETIC_PR'] ] (runtime expression). The dependencies.JobName.outputs[...] form is undefined inside the producing job.

Practical consequence: bypass.ts checks process.env.AW_SYNTHETIC_PR === "true". If the coalesce cannot resolve the synthPr output, AW_SYNTHETIC_PR arrives empty and bypass takes the "not a PR build → auto-pass" path for CI-promoted builds — the gate skips, defeating the PR filter entirely.

2. $[ coalesce(...) ] in step-level env: blocks (filter_ir.rs, exec_context/pr.rs)

ADO documents $[...] runtime expressions as valid in variables: blocks and condition: fields, but step-level env: blocks use $(...) macro expansion. If ADO treats $[ coalesce(...) ] as a literal string in an env: value, all downstream coalescing silently fails: SYSTEM_PULLREQUEST_PULLREQUESTID, ADO_PR_ID, etc. arrive at the bundle as the raw expression string rather than the actual PR identifier.

The standard pattern for accessing cross-job outputs in a step env is to map the output to a job-level variable first:

# Agent job
variables:
  synthPrId: $[ dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_ID'] ]
steps:
  - bash: ...
    env:
      ADO_PR_ID: $(synthPrId)   # macro, not $[]

This would require a compiler-generated variables: block per job. Worth an explicit ADO YAML reference or empirical test before merge, given the deferred E2E.

3. AW_SYNTHETIC_PR_TARGETBRANCH format inconsistency (exec-context-pr-synth/index.ts)

setOutput("AW_SYNTHETIC_PR_TARGETBRANCH", pr.targetRefName ?? "") stores refs/heads/main (ADO API format). But System.PullRequest.TargetBranch predefined variable is main (no prefix). After coalescing, exec-context-pr.js sees main for real PR builds and refs/heads/main for synth-promoted ones. If the bundle uses this for checkout or merge-base resolution, synth builds break. Fix: strip the prefix before setOutput:

setOutput("AW_SYNTHETIC_PR_TARGETBRANCH", (pr.targetRefName ?? "").replace(/^refs\/heads\//, ""));

⚠️ Suggestions

4. Gate enforcement silently bypassed for real PR builds in synthetic mode (src/compile/common.rs, generate_agentic_depends_on)

With synthetic_pr_active=true + has_pr_filters=true, the agent-job condition includes eq(Build.Reason, 'PullRequest') as a standalone OR arm. A real PR build that fails the gate (prGate.SHOULD_RUN=false, e.g., title doesn't match *[review]*) still passes this arm and runs the agent. Pre-synthetic mode enforced the gate for real PR builds. If intentional for synth mode, a comment explaining the design decision would prevent confusion for authors upgrading existing on.pr.filters: agents (default is now mode: synthetic).

5. Stale compile-coalesce-env todo comment (src/compile/extensions/exec_context/pr.rs ~line 133)

"The step's condition is also broadened in the compile-coalesce-env todo" — the condition broadening is already implemented in the same function. Should be removed or updated.

✅ What Looks Good

• Design: PrMode enum, PR_SYNTH_SPEC shape and 8 KiB size cap, exactly-one-match rule, and the comment explaining why the CI trigger must NOT be auto-narrowed to pr.branches.include (target vs source branch inversion) are all well-reasoned.
• Test coverage: 26 new vitest cases, 4 new generate_ci_trigger Rust unit tests, round-trip spec tests, and 2 new snapshot fixtures are thorough. The assertions specifically checking no auto-narrowed CI trigger are valuable regression guards.
• Bypass guard scoping: bypass.ts correctly limits the synthetic-PR suppression to PullRequest specs only; the four new bypass tests cover all edge cases cleanly.
• Security posture: Treating PR_SYNTH_SPEC decode failures as hard errors (exit 1, not soft skip) is the right call — a malformed spec should never widen the match surface silently.

Generated by Rust PR Reviewer for issue #922 · sonnet46 4.9M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — well-structured feature with strong test coverage and clear security reasoning. A few items worth fixing.

Findings

🐛 Bugs / Logic Issues

• exec-context-pr-synth/index.ts, runtime contract comment (line ~443): The comment says 4. branches.include/exclude miss on BUILD_SOURCEBRANCH → skip, but the code never filters on.pr.branches against BUILD_SOURCEBRANCH. It fetches PRs by sourceRefName == sourceBranch, then filters the matched PRs by their targetRefName against spec.branches. This is correct ADO semantics (pr.branches = target branches), but the contract comment is wrong and could mislead a future debugger into thinking the spec's branch list is being applied to the feature branch itself.

• ado-client.ts comment (line ~937): Says "first page (200 PRs)" — ADO's getPullRequests default page size without top is 100, not 200.

⚠️ Suggestions

• AdoScriptExtension.synthetic_pr_active + pr_trigger_for_synth coupling: These two fields must be set together (true ↔ Some(...)), enforced only by a runtime anyhow::anyhow! guard in setup_steps. The error message is clear and tests would catch a mismatch quickly, but wrapping them in a single Option<PrTriggerConfig> (using is_some() as the activation predicate) would make the invariant unrepresentable-wrong at the type level. Low urgency since the existing tests cover the pairing.

• $[...] runtime expressions as unquoted YAML scalars (filter_ir.rs, exec_context/pr.rs): Values like $[ coalesce(variables['synthPr.AW_SYNTHETIC_PR'], '') ] are embedded as unquoted block-mapping scalars containing single quotes. ADO's YAML parser handles these fine in practice and the existing gate-step YAML does the same, but the strict YAML spec reserves ' as a scalar indicator at the start of a value. Wrapping these in YAML double quotes (e.g. "$[ coalesce(...) ]") would be strictly correct and is the form shown in ADO documentation. Not a functional bug with ADO.

✅ What Looks Good

• Gate-bypass security fix is solid: The AND-NOT form and(ne(Build.Reason,'PullRequest'), ne(synthPr.AW_SYNTHETIC_PR,'true')) as the "unconditional pass" arm is exactly right. An earlier iteration with standalone eq(...) bypass arms would have let any PR build skip the gate — this version forces gate evaluation for all PR-flavour builds. The regression guards in the tests are excellent.
• Same-job vs cross-job variable reference distinction is correctly handled and explicitly documented in both filter_ir.rs and exec_context/pr.rs.
• Base64 spec injection safety: base64 alphabet contains no YAML-special characters; embedding in a double-quoted YAML scalar is safe.
• PrMode::Synthetic as #[default] is the right choice — zero migration friction for existing on.pr agents, and the opt-out path (mode: policy) is explicit.
• Test coverage is thorough: the regression guard for the gate-bypass bug (test_pr_filter_synth_mode_agent_condition_enforces_gate) and the flipped assertion in test_exec_context_pr_only_downloads_bundle_in_agent_job_not_setup both have good explanatory comments.

Generated by Rust PR Reviewer for issue #922 · sonnet46 2.5M · ◷