From 841f938e0b1638b84fea403af5fc39209472595b Mon Sep 17 00:00:00 2001 From: James Devine Date: Mon, 8 Jun 2026 22:19:08 +0100 Subject: [PATCH 01/19] feat(compile): add synthetic-from-ci front-matter knob to on.pr Default true. Plumbs the schema through PrTriggerConfig with a serde-renamed field; existing struct-literal call sites use ..Default::default() for forward-compat. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/common.rs | 6 +++ src/compile/pr_filters.rs | 3 ++ src/compile/types.rs | 77 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 85 insertions(+), 1 deletion(-) diff --git a/src/compile/common.rs b/src/compile/common.rs index d1c442cd..ac3c2c9c 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -6420,6 +6420,7 @@ safe-outputs: }), paths: None, filters: None, + ..Default::default() }), schedule: None, }); @@ -6445,6 +6446,7 @@ safe-outputs: }), paths: None, filters: None, + ..Default::default() }), schedule: None, }); @@ -6470,6 +6472,7 @@ safe-outputs: exclude: vec![], }), filters: None, + ..Default::default() }), schedule: None, }); @@ -6495,6 +6498,7 @@ safe-outputs: exclude: vec!["tests/\ninjected: true".to_string()], }), filters: None, + ..Default::default() }), schedule: None, }); @@ -6520,6 +6524,7 @@ safe-outputs: }), paths: None, filters: None, + ..Default::default() }), schedule: None, }); @@ -6543,6 +6548,7 @@ safe-outputs: exclude: vec!["tests/**".to_string()], }), filters: None, + ..Default::default() }), schedule: None, }); diff --git a/src/compile/pr_filters.rs b/src/compile/pr_filters.rs index c12bd621..f2c5e052 100644 --- a/src/compile/pr_filters.rs +++ b/src/compile/pr_filters.rs @@ -140,6 +140,7 @@ mod tests { }), paths: None, filters: None, + ..Default::default() }), schedule: None, }); @@ -163,6 +164,7 @@ mod tests { exclude: vec!["docs/*".into()], }), filters: None, + ..Default::default() }), schedule: None, }); @@ -184,6 +186,7 @@ mod tests { title: Some(PatternFilter { pattern: "*[agent]*".into() }), ..Default::default() }), + ..Default::default() }), schedule: None, }); diff --git a/src/compile/types.rs b/src/compile/types.rs index 100fb80f..86c150df 100644 --- a/src/compile/types.rs +++ b/src/compile/types.rs @@ -1245,7 +1245,7 @@ impl SanitizeConfigTrait for PrContextConfig { // ─── PR Trigger Types ─────────────────────────────────────────────────────── /// PR trigger configuration with native ADO filters and runtime gate filters. -#[derive(Debug, Deserialize, Clone, Default)] +#[derive(Debug, Deserialize, Clone)] pub struct PrTriggerConfig { /// Native ADO branch filter for PR triggers #[serde(default)] @@ -1256,6 +1256,34 @@ pub struct PrTriggerConfig { /// Runtime filters evaluated via gate steps in the Setup job #[serde(default)] pub filters: Option, + /// Whether to synthesise PullRequest semantics on CI builds when an + /// open PR matches the configured branches/paths. Defaults to `true`. + /// + /// Azure DevOps Services ignores the YAML `pr:` block unless a Build + /// Validation branch policy is registered server-side. When this knob + /// is on, an Setup-job script (`exec-context-pr-synth.js`) looks up + /// the open PR via the ADO REST API on CI-triggered builds and + /// exposes PR identifiers as `dependencies.Setup.outputs['synthPr.*']` + /// so downstream gate/exec-context-pr steps behave as if + /// `Build.Reason == PullRequest`. Set to `false` to opt out and + /// require an operator-installed branch policy. + #[serde(default = "pr_synthetic_from_ci_default", rename = "synthetic-from-ci")] + pub synthetic_from_ci: bool, +} + +impl Default for PrTriggerConfig { + fn default() -> Self { + Self { + branches: None, + paths: None, + filters: None, + synthetic_from_ci: pr_synthetic_from_ci_default(), + } + } +} + +fn pr_synthetic_from_ci_default() -> bool { + true } impl SanitizeConfigTrait for PrTriggerConfig { @@ -2219,6 +2247,53 @@ triggers: assert_eq!(changed.exclude, vec!["docs/**"]); } + #[test] + fn test_pr_trigger_config_synthetic_from_ci_default_true() { + let yaml = r#" +triggers: + pr: + branches: + include: [main] +"#; + let val: serde_yaml::Value = serde_yaml::from_str(yaml).unwrap(); + let tc: OnConfig = serde_yaml::from_value(val["triggers"].clone()).unwrap(); + assert!( + tc.pr.unwrap().synthetic_from_ci, + "synthetic-from-ci must default to true when omitted" + ); + } + + #[test] + fn test_pr_trigger_config_synthetic_from_ci_explicit_false() { + let yaml = r#" +triggers: + pr: + branches: + include: [main] + synthetic-from-ci: false +"#; + let val: serde_yaml::Value = serde_yaml::from_str(yaml).unwrap(); + let tc: OnConfig = serde_yaml::from_value(val["triggers"].clone()).unwrap(); + assert!( + !tc.pr.unwrap().synthetic_from_ci, + "synthetic-from-ci: false must opt out of synthesis" + ); + } + + #[test] + fn test_pr_trigger_config_synthetic_from_ci_explicit_true() { + let yaml = r#" +triggers: + pr: + branches: + include: [main] + synthetic-from-ci: true +"#; + let val: serde_yaml::Value = serde_yaml::from_str(yaml).unwrap(); + let tc: OnConfig = serde_yaml::from_value(val["triggers"].clone()).unwrap(); + assert!(tc.pr.unwrap().synthetic_from_ci); + } + #[test] fn test_pr_trigger_config_paths_only() { let yaml = r#" From 76356d3b1040bf1aaf060c269d8e1b7312a599f8 Mon Sep 17 00:00:00 2001 From: James Devine Date: Mon, 8 Jun 2026 22:20:59 +0100 Subject: [PATCH 02/19] feat(compile): add build_pr_synth_spec for PR_SYNTH_SPEC env var Serializes on.pr branches/paths to base64-encoded JSON consumed by the upcoming exec-context-pr-synth.js bundle. Mirrors GATE_SPEC encoding and adds an 8 KiB defence-in-depth cap. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/filter_ir.rs | 142 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 142 insertions(+) diff --git a/src/compile/filter_ir.rs b/src/compile/filter_ir.rs index 235e2506..d02824b5 100644 --- a/src/compile/filter_ir.rs +++ b/src/compile/filter_ir.rs @@ -1154,6 +1154,89 @@ pub fn compile_gate_step_external( Ok(step) } +// ─── PR synthetic-from-ci spec ────────────────────────────────────────────── + +/// Base64-encoded JSON spec consumed by the `exec-context-pr-synth.js` +/// bundle at runtime. Carries the PR branch/path filters the agent +/// declared in front-matter so the bundle can match an active PR by +/// `sourceRefName` and filter by `targetRefName` + changed-file paths. +/// +/// Shape: +/// ```json +/// { +/// "branches": { "include": [...], "exclude": [...] }, +/// "paths": { "include": [...], "exclude": [...] } +/// } +/// ``` +/// +/// All four arrays are always present (possibly empty) for shape stability — +/// the bundle can rely on the fields existing. +#[derive(Debug, serde::Serialize)] +struct PrSynthSpec { + branches: PrSynthGlobs, + paths: PrSynthGlobs, +} + +#[derive(Debug, serde::Serialize)] +struct PrSynthGlobs { + include: Vec, + exclude: Vec, +} + +/// Maximum decoded size of `PR_SYNTH_SPEC`. Matches the spirit of the +/// `GATE_SPEC` 8 KiB ceiling — synth specs are smaller (no checks, no +/// facts), but the same defence-in-depth bound prevents pathological +/// front-matter from blowing up the bundle's parser. +const PR_SYNTH_SPEC_MAX_BYTES: usize = 8 * 1024; + +/// Build the base64-encoded `PR_SYNTH_SPEC` value for the given PR +/// trigger configuration. +/// +/// The returned string is safe to embed inside a YAML double-quoted +/// scalar (the base64 alphabet contains no characters that require +/// YAML escaping). +pub fn build_pr_synth_spec( + pr: &crate::compile::types::PrTriggerConfig, +) -> anyhow::Result { + use base64::{Engine as _, engine::general_purpose::STANDARD}; + + let spec = PrSynthSpec { + branches: PrSynthGlobs { + include: pr + .branches + .as_ref() + .map(|b| b.include.clone()) + .unwrap_or_default(), + exclude: pr + .branches + .as_ref() + .map(|b| b.exclude.clone()) + .unwrap_or_default(), + }, + paths: PrSynthGlobs { + include: pr + .paths + .as_ref() + .map(|p| p.include.clone()) + .unwrap_or_default(), + exclude: pr + .paths + .as_ref() + .map(|p| p.exclude.clone()) + .unwrap_or_default(), + }, + }; + + let json = serde_json::to_string(&spec)?; + anyhow::ensure!( + json.len() <= PR_SYNTH_SPEC_MAX_BYTES, + "PR_SYNTH_SPEC serialised size {} exceeds {}-byte cap; reduce the number/length of on.pr branches/paths globs", + json.len(), + PR_SYNTH_SPEC_MAX_BYTES + ); + Ok(STANDARD.encode(json.as_bytes())) +} + /// Collect ADO macro exports needed by the given checks. fn collect_ado_exports( checks: &[FilterCheck], @@ -1246,6 +1329,65 @@ mod tests { use super::*; use crate::compile::types::*; + // ─── PR_SYNTH_SPEC tests ──────────────────────────────────────────── + + #[test] + fn test_build_pr_synth_spec_roundtrip() { + use base64::{Engine as _, engine::general_purpose::STANDARD}; + use serde_json::Value; + + let pr = PrTriggerConfig { + branches: Some(BranchFilter { + include: vec!["main".into(), "release/*".into()], + exclude: vec!["test/*".into()], + }), + paths: Some(PathFilter { + include: vec!["src/*".into()], + exclude: vec!["docs/*".into()], + }), + filters: None, + ..Default::default() + }; + let b64 = build_pr_synth_spec(&pr).expect("synth spec must build"); + let decoded = STANDARD.decode(b64.as_bytes()).expect("must decode base64"); + let parsed: Value = serde_json::from_slice(&decoded).expect("must be valid JSON"); + assert_eq!(parsed["branches"]["include"], serde_json::json!(["main", "release/*"])); + assert_eq!(parsed["branches"]["exclude"], serde_json::json!(["test/*"])); + assert_eq!(parsed["paths"]["include"], serde_json::json!(["src/*"])); + assert_eq!(parsed["paths"]["exclude"], serde_json::json!(["docs/*"])); + } + + #[test] + fn test_build_pr_synth_spec_omitted_arrays_become_empty() { + use base64::{Engine as _, engine::general_purpose::STANDARD}; + use serde_json::Value; + + let pr = PrTriggerConfig::default(); + let b64 = build_pr_synth_spec(&pr).expect("synth spec must build"); + let decoded = STANDARD.decode(b64.as_bytes()).unwrap(); + let parsed: Value = serde_json::from_slice(&decoded).unwrap(); + assert_eq!(parsed["branches"]["include"], serde_json::json!([])); + assert_eq!(parsed["branches"]["exclude"], serde_json::json!([])); + assert_eq!(parsed["paths"]["include"], serde_json::json!([])); + assert_eq!(parsed["paths"]["exclude"], serde_json::json!([])); + } + + #[test] + fn test_build_pr_synth_spec_rejects_oversize() { + // Generate enough branch globs to blow past the 8 KiB cap. + let pr = PrTriggerConfig { + branches: Some(BranchFilter { + include: (0..1000).map(|i| format!("very/long/branch/glob/pattern/{i}")).collect(), + exclude: vec![], + }), + paths: None, + filters: None, + ..Default::default() + }; + let err = build_pr_synth_spec(&pr).expect_err("oversize spec must fail"); + assert!(err.to_string().contains("PR_SYNTH_SPEC"), "error must mention spec: {err}"); + } + // ─── Fact tests ───────────────────────────────────────────────────── #[test] From dca03bae47a0e2e9c6efb280b638e80d187498a4 Mon Sep 17 00:00:00 2001 From: James Devine Date: Mon, 8 Jun 2026 22:25:50 +0100 Subject: [PATCH 03/19] feat(scripts): scaffold exec-context-pr-synth ado-script bundle Adds skeleton index.ts (no-op main) and match.ts (picomatch-based branch/path glob helpers with refs/heads/ and leading-slash normalisation). Wires the bundle into the npm build/clean/test:smoke chain and the release.yml ado-script.zip packager. Adds picomatch as a direct dependency so ncc bundles it deterministically. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/release.yml | 2 +- scripts/ado-script/.gitignore | 1 + scripts/ado-script/package-lock.json | 12 ++- scripts/ado-script/package.json | 11 ++- .../src/exec-context-pr-synth/index.ts | 32 ++++++++ .../src/exec-context-pr-synth/match.ts | 78 +++++++++++++++++++ 6 files changed, 129 insertions(+), 7 deletions(-) create mode 100644 scripts/ado-script/src/exec-context-pr-synth/index.ts create mode 100644 scripts/ado-script/src/exec-context-pr-synth/match.ts diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index aadb0159..c534ab71 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -73,7 +73,7 @@ jobs: run: | set -euo pipefail cd scripts - zip -r ../ado-script.zip ado-script/gate.js ado-script/import.js ado-script/exec-context-pr.js + zip -r ../ado-script.zip ado-script/gate.js ado-script/import.js ado-script/exec-context-pr.js ado-script/exec-context-pr-synth.js - name: Upload release assets env: diff --git a/scripts/ado-script/.gitignore b/scripts/ado-script/.gitignore index 59cbba45..6aea872c 100644 --- a/scripts/ado-script/.gitignore +++ b/scripts/ado-script/.gitignore @@ -3,5 +3,6 @@ node_modules gate.js import.js exec-context-pr.js +exec-context-pr-synth.js schema *.tsbuildinfo diff --git a/scripts/ado-script/package-lock.json b/scripts/ado-script/package-lock.json index be43eacc..af284041 100644 --- a/scripts/ado-script/package-lock.json +++ b/scripts/ado-script/package-lock.json @@ -8,10 +8,12 @@ "name": "@ado-aw/scripts", "version": "0.0.0", "dependencies": { - "azure-devops-node-api": "^14.1.0" + "azure-devops-node-api": "^14.1.0", + "picomatch": "^4.0.4" }, "devDependencies": { "@types/node": "^20.19.39", + "@types/picomatch": "^4.0.3", "@vercel/ncc": "^0.38.4", "json-schema-to-typescript": "^15.0.4", "typescript": "^5.9.3", @@ -447,6 +449,13 @@ "undici-types": "~6.21.0" } }, + "node_modules/@types/picomatch": { + "version": "4.0.3", + "resolved": "https://registry.npmjs.org/@types/picomatch/-/picomatch-4.0.3.tgz", + "integrity": "sha512-iG0T6+nYJ9FAPmx9SsUlnwcq1ZVRuCXcVEvWnntoPlrOpwtSTKNDC9uVAxTsC3PUvJ+99n4RpAcNgBbHX3JSnQ==", + "dev": true, + "license": "MIT" + }, "node_modules/@vercel/ncc": { "version": "0.38.4", "resolved": "https://registry.npmjs.org/@vercel/ncc/-/ncc-0.38.4.tgz", @@ -1287,7 +1296,6 @@ "version": "4.0.4", "resolved": "https://registry.npmjs.org/picomatch/-/picomatch-4.0.4.tgz", "integrity": "sha512-QP88BAKvMam/3NxH6vj2o21R6MjxZUAd6nlwAS/pnGvN9IVLocLHxGYIzFhg6fUQ+5th6P4dv4eW9jX3DSIj7A==", - "dev": true, "license": "MIT", "engines": { "node": ">=12" diff --git a/scripts/ado-script/package.json b/scripts/ado-script/package.json index 0aa0cf0c..de386bdd 100644 --- a/scripts/ado-script/package.json +++ b/scripts/ado-script/package.json @@ -7,23 +7,26 @@ "node": ">=20.0.0" }, "scripts": { - "build": "npm run codegen && npm run clean && npm run build:gate && npm run build:import && npm run build:exec-context-pr", - "clean": "node -e \"const fs=require('node:fs'); fs.rmSync('.ado-build',{recursive:true,force:true}); fs.rmSync('gate.js',{force:true}); fs.rmSync('import.js',{force:true}); fs.rmSync('exec-context-pr.js',{force:true});\"", + "build": "npm run codegen && npm run clean && npm run build:gate && npm run build:import && npm run build:exec-context-pr && npm run build:exec-context-pr-synth", + "clean": "node -e \"const fs=require('node:fs'); fs.rmSync('.ado-build',{recursive:true,force:true}); fs.rmSync('gate.js',{force:true}); fs.rmSync('import.js',{force:true}); fs.rmSync('exec-context-pr.js',{force:true}); fs.rmSync('exec-context-pr-synth.js',{force:true});\"", "build:gate": "ncc build src/gate/index.ts -o .ado-build/gate -m -t && node -e \"const fs=require('node:fs'); fs.copyFileSync('.ado-build/gate/index.js','gate.js'); fs.rmSync('.ado-build/gate',{recursive:true,force:true});\"", "build:import": "ncc build src/import/index.ts -o .ado-build/import -m -t && node -e \"const fs=require('node:fs'); fs.copyFileSync('.ado-build/import/index.js','import.js'); fs.rmSync('.ado-build/import',{recursive:true,force:true});\"", "build:exec-context-pr": "ncc build src/exec-context-pr/index.ts -o .ado-build/exec-context-pr -m -t && node -e \"const fs=require('node:fs'); fs.copyFileSync('.ado-build/exec-context-pr/index.js','exec-context-pr.js'); fs.rmSync('.ado-build/exec-context-pr',{recursive:true,force:true});\"", + "build:exec-context-pr-synth": "ncc build src/exec-context-pr-synth/index.ts -o .ado-build/exec-context-pr-synth -m -t && node -e \"const fs=require('node:fs'); fs.copyFileSync('.ado-build/exec-context-pr-synth/index.js','exec-context-pr-synth.js'); fs.rmSync('.ado-build/exec-context-pr-synth',{recursive:true,force:true});\"", "build:check": "ls -lh gate.js && wc -c gate.js", "codegen": "node -e \"require('node:fs').mkdirSync('schema', { recursive: true })\" && cargo run --quiet --manifest-path ../../Cargo.toml -- export-gate-schema --output schema/gate-spec.schema.json && npx json2ts schema/gate-spec.schema.json -o src/shared/types.gen.ts --bannerComment \"// AUTO-GENERATED from Rust IR via cargo run -- export-gate-schema. Do not edit; run npm run codegen.\"", "test": "vitest run", - "test:smoke": "npm run build:gate && npm run build:import && npm run build:exec-context-pr && vitest run -c vitest.config.smoke.ts", + "test:smoke": "npm run build:gate && npm run build:import && npm run build:exec-context-pr && npm run build:exec-context-pr-synth && vitest run -c vitest.config.smoke.ts", "lint": "echo TODO", "typecheck": "tsc --noEmit" }, "dependencies": { - "azure-devops-node-api": "^14.1.0" + "azure-devops-node-api": "^14.1.0", + "picomatch": "^4.0.4" }, "devDependencies": { "@types/node": "^20.19.39", + "@types/picomatch": "^4.0.3", "@vercel/ncc": "^0.38.4", "json-schema-to-typescript": "^15.0.4", "typescript": "^5.9.3", diff --git a/scripts/ado-script/src/exec-context-pr-synth/index.ts b/scripts/ado-script/src/exec-context-pr-synth/index.ts new file mode 100644 index 00000000..f27165a5 --- /dev/null +++ b/scripts/ado-script/src/exec-context-pr-synth/index.ts @@ -0,0 +1,32 @@ +/** + * exec-context-pr-synth — Setup-job script that synthesises + * `Build.Reason == PullRequest` semantics on CI-triggered builds when + * an open PR matches the agent's `on.pr.branches` / `on.pr.paths` + * filters. + * + * Why this exists: Azure DevOps Services ignores the YAML `pr:` block + * unless a per-branch Build Validation policy is registered server- + * side. Without that policy, a push to a feature branch fires the + * pipeline as `Build.Reason = IndividualCI` even when an open PR + * exists, so the gate evaluator's "not a PR build" bypass triggers + * and `exec-context-pr.js` is skipped entirely. + * + * This script runs in the Setup job before `prGate`, calls the ADO + * REST API to find the active PR for `Build.SourceBranch`, applies + * the front-matter filters, and emits `AW_SYNTHETIC_PR*` outputs + * that downstream gate + exec-context-pr steps coalesce with the + * real `System.PullRequest.*` variables. + * + * Skeleton-only in this commit — full runtime contract is implemented + * in the synth-bundle-logic todo. + */ + +export function main(_env: NodeJS.ProcessEnv = process.env): number { + return 0; +} + +// Top-level invocation. `process.exit` is called here (not in `main`) +// so tests can call `main(env)` and inspect the return value without +// terminating the test process. +const exitCode = main(); +process.exit(exitCode); diff --git a/scripts/ado-script/src/exec-context-pr-synth/match.ts b/scripts/ado-script/src/exec-context-pr-synth/match.ts new file mode 100644 index 00000000..bae6c518 --- /dev/null +++ b/scripts/ado-script/src/exec-context-pr-synth/match.ts @@ -0,0 +1,78 @@ +/** + * Glob matching helpers for branch refs and changed-file paths. + * + * Both branches and paths in front-matter use the same ADO/git-style + * glob syntax: `*` matches one path segment, `**` matches any number, + * `?` matches a single char. We normalise refs by stripping the + * `refs/heads/` prefix so authors can write either `main` or + * `refs/heads/main` and both match. + * + * Built on picomatch for correctness; picomatch is added as a direct + * dependency so ncc bundles it deterministically. + */ +import picomatch from "picomatch"; + +const REFS_HEADS_PREFIX = "refs/heads/"; + +/** Strip a leading `refs/heads/` prefix if present. */ +export function normaliseBranchRef(ref: string): string { + return ref.startsWith(REFS_HEADS_PREFIX) ? ref.slice(REFS_HEADS_PREFIX.length) : ref; +} + +/** Strip a leading `/` from a path if present (iteration-API paths start with `/`). */ +export function normalisePath(p: string): string { + return p.startsWith("/") ? p.slice(1) : p; +} + +/** + * Apply include/exclude semantics: include must match (or be empty), + * and exclude must not match. + * + * - Empty `includes` → include-all (treat as "no positive filter"). + * - Non-empty `includes` → at least one must match. + * - Non-empty `excludes` → none must match. + * + * Mirrors ADO's `branches:` / `paths:` semantics in PR triggers. + */ +export function matchesIncludeExclude( + value: string, + includes: string[], + excludes: string[], +): boolean { + const normalised = normaliseBranchRef(value); + const normIncludes = includes.map(normaliseBranchRef); + const normExcludes = excludes.map(normaliseBranchRef); + const includeMatches = + normIncludes.length === 0 || + normIncludes.some((g) => picomatch(g, { dot: true })(normalised)); + if (!includeMatches) return false; + if ( + normExcludes.length > 0 && + normExcludes.some((g) => picomatch(g, { dot: true })(normalised)) + ) { + return false; + } + return true; +} + +/** Path variant: normalises leading `/` instead of `refs/heads/`. */ +export function pathMatchesIncludeExclude( + pathValue: string, + includes: string[], + excludes: string[], +): boolean { + const normalised = normalisePath(pathValue); + const normIncludes = includes.map(normalisePath); + const normExcludes = excludes.map(normalisePath); + const includeMatches = + normIncludes.length === 0 || + normIncludes.some((g) => picomatch(g, { dot: true })(normalised)); + if (!includeMatches) return false; + if ( + normExcludes.length > 0 && + normExcludes.some((g) => picomatch(g, { dot: true })(normalised)) + ) { + return false; + } + return true; +} From 104de5f77d75153d48d522863863233f39d6c551 Mon Sep 17 00:00:00 2001 From: James Devine Date: Mon, 8 Jun 2026 22:30:08 +0100 Subject: [PATCH 04/19] feat(scripts): implement exec-context-pr-synth runtime contract Decodes PR_SYNTH_SPEC, no-ops on real PR builds and GitHub-typed repos, queries ADO REST for active PRs by sourceRefName, enforces exactly-one-match + target-branch filter + path filter, emits AW_SYNTHETIC_PR* outputs consumed by gate.js and exec-context-pr.js. Adds listActivePullRequestsBySourceRef to shared/ado-client.ts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/exec-context-pr-synth/index.ts | 180 +++++++++++++++++- .../src/exec-context-pr-synth/spec.ts | 72 +++++++ scripts/ado-script/src/shared/ado-client.ts | 28 +++ 3 files changed, 275 insertions(+), 5 deletions(-) create mode 100644 scripts/ado-script/src/exec-context-pr-synth/spec.ts diff --git a/scripts/ado-script/src/exec-context-pr-synth/index.ts b/scripts/ado-script/src/exec-context-pr-synth/index.ts index f27165a5..4bb102be 100644 --- a/scripts/ado-script/src/exec-context-pr-synth/index.ts +++ b/scripts/ado-script/src/exec-context-pr-synth/index.ts @@ -17,16 +17,186 @@ * that downstream gate + exec-context-pr steps coalesce with the * real `System.PullRequest.*` variables. * - * Skeleton-only in this commit — full runtime contract is implemented - * in the synth-bundle-logic todo. + * Runtime contract (all soft skips exit 0; only spec-decode errors + * and infra failures exit non-zero): + * + * 1. real PR build (BUILD_REASON=PullRequest) → no-op + * 2. GitHub-typed repo (BUILD_REPOSITORY_PROVIDER=GitHub) → no-op + * 3. Decode PR_SYNTH_SPEC (hard fail on corruption) + * 4. branches.include/exclude miss on BUILD_SOURCEBRANCH → skip + * 5. fetch open PRs by sourceRefName + filter by targetRefName + * 6. count != 1 → skip + * 7. paths.include/exclude reject everything → skip + * 8. emit AW_SYNTHETIC_PR* outputs */ +import { + getIterationChanges, + getPullRequestIterations, + listActivePullRequestsBySourceRef, +} from "../shared/ado-client.js"; +import { logError, logInfo, setOutput } from "../shared/vso-logger.js"; + +import { matchesIncludeExclude, normalisePath, pathMatchesIncludeExclude } from "./match.js"; +import { decodeSpec, type PrSynthSpec } from "./spec.js"; + +const SKIP_OUTPUT = "AW_SYNTHETIC_PR_SKIP"; + +function emitSkip(reason: string): void { + setOutput(SKIP_OUTPUT, "true"); + logInfo(`[synth-pr] skip: ${reason}`); +} + +export async function main(env: NodeJS.ProcessEnv = process.env): Promise { + // Step 1 — real PR build owns the path; the bundle has nothing to add. + if ((env.BUILD_REASON ?? "") === "PullRequest") { + logInfo("[synth-pr] BUILD_REASON=PullRequest; real PR build, synth skipped"); + return 0; + } + + // Step 2 — GitHub-typed repos already get correct pr: semantics from + // ADO. Synthesising would double-fire. + if ((env.BUILD_REPOSITORY_PROVIDER ?? "").toLowerCase() === "github") { + logInfo("[synth-pr] GitHub-typed repo; ADO already provides PR semantics, synth skipped"); + return 0; + } + + // Step 3 — decode the spec. Corruption is a hard failure. + let spec: PrSynthSpec; + try { + spec = decodeSpec(env.PR_SYNTH_SPEC); + } catch (e) { + logError(`[synth-pr] ${(e as Error).message}`); + return 1; + } + + const sourceBranch = env.BUILD_SOURCEBRANCH ?? ""; + if (sourceBranch.length === 0) { + emitSkip("BUILD_SOURCEBRANCH is empty"); + return 0; + } + + // Step 4 — quick branch-filter rejection without hitting the API. + if (!matchesIncludeExclude(sourceBranch, spec.branches.include, spec.branches.exclude)) { + emitSkip(`source branch ${sourceBranch} excluded by on.pr.branches filters`); + return 0; + } + + const project = env.ADO_PROJECT ?? ""; + const repoId = env.ADO_REPO_ID ?? ""; + if (project.length === 0 || repoId.length === 0) { + logError( + "[synth-pr] required env vars ADO_PROJECT and ADO_REPO_ID must be set by the compiler", + ); + return 1; + } + + // Step 5 — fetch active PRs whose source branch is exactly ours, then + // also enforce the agent's branches filter against the TARGET ref. + let prs; + try { + prs = await listActivePullRequestsBySourceRef(project, repoId, sourceBranch); + } catch (e) { + logError(`[synth-pr] listActivePullRequestsBySourceRef failed: ${(e as Error).message}`); + return 1; + } + + const matched = prs.filter((pr) => + matchesIncludeExclude( + pr.targetRefName ?? "", + spec.branches.include, + spec.branches.exclude, + ), + ); + + // Step 6 — exactly-one rule. + if (matched.length === 0) { + emitSkip(`no active PR for source ${sourceBranch} matches on.pr.branches`); + return 0; + } + if (matched.length > 1) { + emitSkip( + `${matched.length} active PRs match source ${sourceBranch}; refusing to choose`, + ); + return 0; + } + + const pr = matched[0]!; + const prId = pr.pullRequestId; + if (typeof prId !== "number") { + emitSkip("matched PR has no pullRequestId"); + return 0; + } + + // Step 7 — path filter. Only fetch iteration changes if the agent + // declared a path filter; otherwise short-circuit (the + // pathMatchesIncludeExclude call would accept everything anyway, but + // the API call is wasted). + const hasPathFilter = spec.paths.include.length > 0 || spec.paths.exclude.length > 0; + if (hasPathFilter) { + let iterations; + try { + iterations = await getPullRequestIterations(project, repoId, prId); + } catch (e) { + logError(`[synth-pr] getPullRequestIterations failed: ${(e as Error).message}`); + return 1; + } + if (iterations.length === 0) { + emitSkip(`PR ${prId} has no iterations`); + return 0; + } + const latest = iterations[iterations.length - 1]!; + const iterationId = latest.id; + if (typeof iterationId !== "number") { + emitSkip(`PR ${prId} latest iteration has no id`); + return 0; + } + let changes; + try { + changes = await getIterationChanges(project, repoId, prId, iterationId); + } catch (e) { + logError(`[synth-pr] getIterationChanges failed: ${(e as Error).message}`); + return 1; + } + const changedPaths: string[] = []; + for (const entry of changes.changeEntries ?? []) { + const itemPath = entry.item?.path; + if (typeof itemPath === "string" && itemPath.length > 0) { + changedPaths.push(normalisePath(itemPath)); + } + } + const anyAccepted = changedPaths.some((p) => + pathMatchesIncludeExclude(p, spec.paths.include, spec.paths.exclude), + ); + if (!anyAccepted) { + emitSkip( + `no changed file in PR ${prId} matches on.pr.paths (${changedPaths.length} files inspected)`, + ); + return 0; + } + } + + // Step 8 — emit synthetic PR identifiers. Downstream gate + + // exec-context-pr steps coalesce these with real + // System.PullRequest.* via $[ coalesce(...) ] env wiring (added in + // the compile-coalesce-env todo). + setOutput("AW_SYNTHETIC_PR", "true"); + setOutput("AW_SYNTHETIC_PR_ID", String(prId)); + setOutput("AW_SYNTHETIC_PR_TARGETBRANCH", pr.targetRefName ?? ""); + setOutput("AW_SYNTHETIC_PR_SOURCEBRANCH", pr.sourceRefName ?? sourceBranch); + setOutput("AW_SYNTHETIC_PR_IS_DRAFT", pr.isDraft === true ? "true" : "false"); -export function main(_env: NodeJS.ProcessEnv = process.env): number { + logInfo( + `[synth-pr] matched PR #${prId} (source=${pr.sourceRefName} target=${pr.targetRefName})`, + ); return 0; } // Top-level invocation. `process.exit` is called here (not in `main`) // so tests can call `main(env)` and inspect the return value without // terminating the test process. -const exitCode = main(); -process.exit(exitCode); +main() + .then((code) => process.exit(code)) + .catch((e: unknown) => { + logError(`[synth-pr] unhandled error: ${(e as Error).message}`); + process.exit(1); + }); diff --git a/scripts/ado-script/src/exec-context-pr-synth/spec.ts b/scripts/ado-script/src/exec-context-pr-synth/spec.ts new file mode 100644 index 00000000..bb99ddf9 --- /dev/null +++ b/scripts/ado-script/src/exec-context-pr-synth/spec.ts @@ -0,0 +1,72 @@ +/** + * Decodes the compiler-emitted `PR_SYNTH_SPEC` env var into a typed + * filter spec. The compiler builds this via + * `crate::compile::filter_ir::build_pr_synth_spec` (Rust side); both + * shapes must stay in lock-step. + * + * Decode failures are HARD errors — a malformed spec indicates either + * compiler corruption or a manual env tamper. Treating it as a soft + * skip would silently widen the permitted attack surface (any branch + * could fool the synth path into matching). + */ + +export interface PrSynthSpec { + branches: { include: string[]; exclude: string[] }; + paths: { include: string[]; exclude: string[] }; +} + +export function decodeSpec(b64: string | undefined): PrSynthSpec { + if (!b64 || b64.length === 0) { + throw new Error("PR_SYNTH_SPEC env var is missing or empty"); + } + let json: string; + try { + json = Buffer.from(b64, "base64").toString("utf8"); + } catch (e) { + throw new Error(`PR_SYNTH_SPEC: base64 decode failed: ${(e as Error).message}`); + } + let parsed: unknown; + try { + parsed = JSON.parse(json); + } catch (e) { + throw new Error(`PR_SYNTH_SPEC: JSON parse failed: ${(e as Error).message}`); + } + return validate(parsed); +} + +function validate(value: unknown): PrSynthSpec { + if (!value || typeof value !== "object") { + throw new Error("PR_SYNTH_SPEC: expected object at root"); + } + const obj = value as Record; + return { + branches: validateGlobs(obj.branches, "branches"), + paths: validateGlobs(obj.paths, "paths"), + }; +} + +function validateGlobs( + value: unknown, + fieldName: string, +): { include: string[]; exclude: string[] } { + if (!value || typeof value !== "object") { + throw new Error(`PR_SYNTH_SPEC: expected ${fieldName} to be an object`); + } + const obj = value as Record; + return { + include: validateStringArray(obj.include, `${fieldName}.include`), + exclude: validateStringArray(obj.exclude, `${fieldName}.exclude`), + }; +} + +function validateStringArray(value: unknown, fieldName: string): string[] { + if (!Array.isArray(value)) { + throw new Error(`PR_SYNTH_SPEC: expected ${fieldName} to be an array of strings`); + } + for (const item of value) { + if (typeof item !== "string") { + throw new Error(`PR_SYNTH_SPEC: ${fieldName} contains non-string entry`); + } + } + return value as string[]; +} diff --git a/scripts/ado-script/src/shared/ado-client.ts b/scripts/ado-script/src/shared/ado-client.ts index 2686582a..4e62d448 100644 --- a/scripts/ado-script/src/shared/ado-client.ts +++ b/scripts/ado-script/src/shared/ado-client.ts @@ -10,6 +10,7 @@ import type { GitPullRequestIteration, GitPullRequestIterationChanges, } from "azure-devops-node-api/interfaces/GitInterfaces.js"; +import { PullRequestStatus } from "azure-devops-node-api/interfaces/GitInterfaces.js"; import { BuildStatus, type Build } from "azure-devops-node-api/interfaces/BuildInterfaces.js"; const SLEEP_MS = 1000; @@ -95,6 +96,33 @@ export async function getPullRequestById( }); } +/** + * Lists active pull requests whose `sourceRefName` matches the given + * value. Used by `exec-context-pr-synth` to discover the open PR for + * `Build.SourceBranch` on CI-triggered builds (no Build Validation + * branch policy required). + * + * Returns an empty array if no PRs match. The ADO REST API caps page + * size; for the synth path we deliberately fetch only the first page + * (200 PRs) since the synth contract requires *exactly one* match — + * a source branch with >200 simultaneous active PRs against it is + * pathological and the bundle will skip via the "multi-match" path. + */ +export async function listActivePullRequestsBySourceRef( + project: string, + repoId: string, + sourceRefName: string, +): Promise { + return withRetry("listActivePullRequestsBySourceRef", async () => { + const git = await (await getWebApi()).getGitApi(); + return git.getPullRequests( + repoId, + { sourceRefName, status: PullRequestStatus.Active }, + project, + ); + }); +} + /** * Fetches all pull-request iterations. * From d5bb0b01a773965c802b5967f0b8efd38ab94352 Mon Sep 17 00:00:00 2001 From: James Devine Date: Mon, 8 Jun 2026 22:34:45 +0100 Subject: [PATCH 05/19] test(scripts): vitest coverage for exec-context-pr-synth bundle 26 new tests across match.ts (glob normalisation + include/exclude semantics) and index.ts (real-PR no-op, GitHub no-op, spec decode errors, zero/multi-match skips, path filter rejection, happy path with all five AW_SYNTHETIC_PR* outputs). Adds an ESM entry-point guard to index.ts so importing main() does not trigger top-level process.exit. Removes the source-branch pre-filter; pr.branches filters the TARGET ref per the issue spec, not the build source. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../__tests__/harness.ts | 61 ++++++ .../__tests__/index.test.ts | 176 ++++++++++++++++++ .../__tests__/match.test.ts | 81 ++++++++ .../src/exec-context-pr-synth/index.ts | 37 ++-- 4 files changed, 341 insertions(+), 14 deletions(-) create mode 100644 scripts/ado-script/src/exec-context-pr-synth/__tests__/harness.ts create mode 100644 scripts/ado-script/src/exec-context-pr-synth/__tests__/index.test.ts create mode 100644 scripts/ado-script/src/exec-context-pr-synth/__tests__/match.test.ts diff --git a/scripts/ado-script/src/exec-context-pr-synth/__tests__/harness.ts b/scripts/ado-script/src/exec-context-pr-synth/__tests__/harness.ts new file mode 100644 index 00000000..f8ac6116 --- /dev/null +++ b/scripts/ado-script/src/exec-context-pr-synth/__tests__/harness.ts @@ -0,0 +1,61 @@ +/** + * Test harness for the exec-context-pr-synth bundle. + * + * Provides: + * - `runMain(env)` — invokes the bundle's main() with a captured + * stdout buffer. + * - `makeEnv(overrides)` — returns a minimal env block populated + * with the required vars, easy to override per case. + * - `build_pr_synth_spec(spec)` — base64-encodes a PrSynthSpec JSON + * for the PR_SYNTH_SPEC env var. + */ +import { vi } from "vitest"; + +import { main } from "../index.js"; +import { _resetCompletedForTesting } from "../../shared/vso-logger.js"; + +export interface RunResult { + code: number; + output: string; +} + +export async function runMain(env: NodeJS.ProcessEnv): Promise { + _resetCompletedForTesting(); + const chunks: string[] = []; + const writeSpy = vi + .spyOn(process.stdout, "write") + .mockImplementation((c: any) => { + chunks.push(typeof c === "string" ? c : c.toString()); + return true; + }); + try { + const code = await main(env); + return { code, output: chunks.join("") }; + } finally { + writeSpy.mockRestore(); + } +} + +export function makeEnv(overrides: Record): NodeJS.ProcessEnv { + return { + BUILD_REASON: "IndividualCI", + BUILD_REPOSITORY_PROVIDER: "TfsGit", + BUILD_SOURCEBRANCH: "refs/heads/feature/x", + ADO_PROJECT: "MyProject", + ADO_REPO_ID: "00000000-0000-0000-0000-000000000000", + ...overrides, + }; +} + +export function build_pr_synth_spec( + spec: { + branches?: { include: string[]; exclude: string[] }; + paths?: { include: string[]; exclude: string[] }; + } = {}, +): string { + const full = { + branches: spec.branches ?? { include: ["main"], exclude: [] }, + paths: spec.paths ?? { include: [], exclude: [] }, + }; + return Buffer.from(JSON.stringify(full), "utf8").toString("base64"); +} diff --git a/scripts/ado-script/src/exec-context-pr-synth/__tests__/index.test.ts b/scripts/ado-script/src/exec-context-pr-synth/__tests__/index.test.ts new file mode 100644 index 00000000..3e09f323 --- /dev/null +++ b/scripts/ado-script/src/exec-context-pr-synth/__tests__/index.test.ts @@ -0,0 +1,176 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("../../shared/ado-client.js", () => ({ + listActivePullRequestsBySourceRef: vi.fn(), + getPullRequestIterations: vi.fn(), + getIterationChanges: vi.fn(), +})); + +import * as adoClient from "../../shared/ado-client.js"; +import { build_pr_synth_spec, makeEnv, runMain } from "./harness.js"; + +const mocked = adoClient as unknown as { + listActivePullRequestsBySourceRef: ReturnType; + getPullRequestIterations: ReturnType; + getIterationChanges: ReturnType; +}; + +describe("exec-context-pr-synth main", () => { + beforeEach(() => { + mocked.listActivePullRequestsBySourceRef.mockReset(); + mocked.getPullRequestIterations.mockReset(); + mocked.getIterationChanges.mockReset(); + }); + afterEach(() => vi.restoreAllMocks()); + + it("no-ops on real PR builds (BUILD_REASON=PullRequest)", async () => { + const { code, output } = await runMain( + makeEnv({ BUILD_REASON: "PullRequest", PR_SYNTH_SPEC: build_pr_synth_spec() }), + ); + expect(code).toBe(0); + expect(mocked.listActivePullRequestsBySourceRef).not.toHaveBeenCalled(); + expect(output).not.toContain("AW_SYNTHETIC_PR_SKIP"); + expect(output).not.toContain("AW_SYNTHETIC_PR=true"); + expect(output).toContain("real PR build"); + }); + + it("no-ops on GitHub-typed repos (BUILD_REPOSITORY_PROVIDER=GitHub)", async () => { + const { code, output } = await runMain( + makeEnv({ + BUILD_REPOSITORY_PROVIDER: "GitHub", + PR_SYNTH_SPEC: build_pr_synth_spec(), + }), + ); + expect(code).toBe(0); + expect(mocked.listActivePullRequestsBySourceRef).not.toHaveBeenCalled(); + expect(output).toContain("GitHub-typed repo"); + }); + + it("returns 1 (hard fail) when PR_SYNTH_SPEC is missing", async () => { + const env = makeEnv({}); + delete env.PR_SYNTH_SPEC; + const { code, output } = await runMain(env); + expect(code).toBe(1); + expect(output).toContain("PR_SYNTH_SPEC env var is missing"); + }); + + it("returns 1 when PR_SYNTH_SPEC is malformed base64", async () => { + const { code, output } = await runMain( + makeEnv({ PR_SYNTH_SPEC: "not!valid!base64!!!" }), + ); + expect(code).toBe(1); + expect(output).toContain("PR_SYNTH_SPEC"); + }); + + it("skips when source branch has no active PR (per ADO API)", async () => { + mocked.listActivePullRequestsBySourceRef.mockResolvedValue([]); + const { code, output } = await runMain( + makeEnv({ + BUILD_SOURCEBRANCH: "refs/heads/feature/unrelated", + PR_SYNTH_SPEC: build_pr_synth_spec({ branches: { include: ["main"], exclude: [] } }), + }), + ); + expect(code).toBe(0); + expect(output).toContain("AW_SYNTHETIC_PR_SKIP;isOutput=true]true"); + expect(mocked.listActivePullRequestsBySourceRef).toHaveBeenCalledOnce(); + }); + + it("skips when a PR is active but its target branch is excluded by on.pr.branches", async () => { + mocked.listActivePullRequestsBySourceRef.mockResolvedValue([ + { + pullRequestId: 1, + sourceRefName: "refs/heads/feature/x", + targetRefName: "refs/heads/release/old", + }, + ]); + const spec = build_pr_synth_spec({ branches: { include: ["main"], exclude: [] } }); + const { code, output } = await runMain(makeEnv({ PR_SYNTH_SPEC: spec })); + expect(code).toBe(0); + expect(output).toContain("AW_SYNTHETIC_PR_SKIP;isOutput=true]true"); + }); + + it("skips when >1 active PRs match (after target filter)", async () => { + mocked.listActivePullRequestsBySourceRef.mockResolvedValue([ + { pullRequestId: 1, sourceRefName: "refs/heads/feature/x", targetRefName: "refs/heads/main" }, + { pullRequestId: 2, sourceRefName: "refs/heads/feature/x", targetRefName: "refs/heads/main" }, + ]); + const { code, output } = await runMain(makeEnv({ PR_SYNTH_SPEC: build_pr_synth_spec() })); + expect(code).toBe(0); + expect(output).toContain("AW_SYNTHETIC_PR_SKIP;isOutput=true]true"); + expect(output).toContain("2 active PRs"); + }); + + it("skips when path filter rejects all changed files", async () => { + mocked.listActivePullRequestsBySourceRef.mockResolvedValue([ + { pullRequestId: 42, sourceRefName: "refs/heads/feature/x", targetRefName: "refs/heads/main" }, + ]); + mocked.getPullRequestIterations.mockResolvedValue([{ id: 1 }]); + mocked.getIterationChanges.mockResolvedValue({ + changeEntries: [{ item: { path: "/docs/readme.md" } }], + }); + const spec = build_pr_synth_spec({ + branches: { include: ["main"], exclude: [] }, + paths: { include: ["src/**"], exclude: [] }, + }); + const { code, output } = await runMain(makeEnv({ PR_SYNTH_SPEC: spec })); + expect(code).toBe(0); + expect(output).toContain("AW_SYNTHETIC_PR_SKIP;isOutput=true]true"); + expect(output).toContain("no changed file"); + }); + + it("emits AW_SYNTHETIC_PR=true + identifiers on the happy path", async () => { + mocked.listActivePullRequestsBySourceRef.mockResolvedValue([ + { + pullRequestId: 1234, + sourceRefName: "refs/heads/feature/x", + targetRefName: "refs/heads/main", + isDraft: false, + }, + ]); + mocked.getPullRequestIterations.mockResolvedValue([{ id: 7 }]); + mocked.getIterationChanges.mockResolvedValue({ + changeEntries: [{ item: { path: "/src/foo.rs" } }], + }); + const spec = build_pr_synth_spec({ + branches: { include: ["main"], exclude: [] }, + paths: { include: ["src/**"], exclude: [] }, + }); + const { code, output } = await runMain(makeEnv({ PR_SYNTH_SPEC: spec })); + expect(code).toBe(0); + expect(output).not.toContain("AW_SYNTHETIC_PR_SKIP"); + expect(output).toContain("AW_SYNTHETIC_PR;isOutput=true]true"); + expect(output).toContain("AW_SYNTHETIC_PR_ID;isOutput=true]1234"); + expect(output).toContain("AW_SYNTHETIC_PR_TARGETBRANCH;isOutput=true]refs/heads/main"); + expect(output).toContain("AW_SYNTHETIC_PR_SOURCEBRANCH;isOutput=true]refs/heads/feature/x"); + expect(output).toContain("AW_SYNTHETIC_PR_IS_DRAFT;isOutput=true]false"); + }); + + it("emits AW_SYNTHETIC_PR_IS_DRAFT=true when the PR is a draft", async () => { + mocked.listActivePullRequestsBySourceRef.mockResolvedValue([ + { + pullRequestId: 1, + sourceRefName: "refs/heads/feature/x", + targetRefName: "refs/heads/main", + isDraft: true, + }, + ]); + const { code, output } = await runMain(makeEnv({ PR_SYNTH_SPEC: build_pr_synth_spec() })); + expect(code).toBe(0); + expect(output).toContain("AW_SYNTHETIC_PR_IS_DRAFT;isOutput=true]true"); + }); + + it("skips path-filter API calls when paths.include and exclude are both empty", async () => { + mocked.listActivePullRequestsBySourceRef.mockResolvedValue([ + { + pullRequestId: 1, + sourceRefName: "refs/heads/feature/x", + targetRefName: "refs/heads/main", + }, + ]); + const { code, output } = await runMain(makeEnv({ PR_SYNTH_SPEC: build_pr_synth_spec() })); + expect(code).toBe(0); + expect(mocked.getPullRequestIterations).not.toHaveBeenCalled(); + expect(mocked.getIterationChanges).not.toHaveBeenCalled(); + expect(output).toContain("AW_SYNTHETIC_PR;isOutput=true]true"); + }); +}); diff --git a/scripts/ado-script/src/exec-context-pr-synth/__tests__/match.test.ts b/scripts/ado-script/src/exec-context-pr-synth/__tests__/match.test.ts new file mode 100644 index 00000000..837b8ed1 --- /dev/null +++ b/scripts/ado-script/src/exec-context-pr-synth/__tests__/match.test.ts @@ -0,0 +1,81 @@ +import { describe, expect, it } from "vitest"; + +import { + matchesIncludeExclude, + normaliseBranchRef, + normalisePath, + pathMatchesIncludeExclude, +} from "../match.js"; + +describe("normaliseBranchRef", () => { + it("strips refs/heads/ prefix", () => { + expect(normaliseBranchRef("refs/heads/main")).toBe("main"); + expect(normaliseBranchRef("refs/heads/feature/x")).toBe("feature/x"); + }); + it("leaves other refs alone", () => { + expect(normaliseBranchRef("main")).toBe("main"); + expect(normaliseBranchRef("refs/tags/v1")).toBe("refs/tags/v1"); + }); +}); + +describe("normalisePath", () => { + it("strips a leading slash", () => { + expect(normalisePath("/src/foo.rs")).toBe("src/foo.rs"); + }); + it("leaves paths without a leading slash alone", () => { + expect(normalisePath("src/foo.rs")).toBe("src/foo.rs"); + }); +}); + +describe("matchesIncludeExclude (branches)", () => { + it("matches when include list is empty (include-all)", () => { + expect(matchesIncludeExclude("main", [], [])).toBe(true); + expect(matchesIncludeExclude("refs/heads/main", [], [])).toBe(true); + }); + + it("matches exact branch names with refs/heads/ normalisation on BOTH sides", () => { + expect(matchesIncludeExclude("refs/heads/main", ["main"], [])).toBe(true); + expect(matchesIncludeExclude("main", ["refs/heads/main"], [])).toBe(true); + }); + + it("rejects branches that are not in the include list", () => { + expect(matchesIncludeExclude("refs/heads/feature/x", ["main"], [])).toBe(false); + }); + + it("supports * glob inside one segment", () => { + expect(matchesIncludeExclude("refs/heads/release/1.0", ["release/*"], [])).toBe(true); + }); + + it("supports ** glob across segments", () => { + expect(matchesIncludeExclude("refs/heads/feature/x/y", ["feature/**"], [])).toBe(true); + }); + + it("exclude wins over include", () => { + expect(matchesIncludeExclude("refs/heads/feature/x", ["**"], ["feature/*"])).toBe(false); + }); + + it("returns true when include matches and exclude does not", () => { + expect(matchesIncludeExclude("refs/heads/main", ["main", "release/*"], ["dev/*"])).toBe( + true, + ); + }); +}); + +describe("pathMatchesIncludeExclude (paths)", () => { + it("matches when include list is empty (include-all)", () => { + expect(pathMatchesIncludeExclude("/src/foo.rs", [], [])).toBe(true); + }); + + it("handles leading-slash normalisation on both sides", () => { + expect(pathMatchesIncludeExclude("/src/foo.rs", ["/src/**"], [])).toBe(true); + expect(pathMatchesIncludeExclude("src/foo.rs", ["src/**"], [])).toBe(true); + }); + + it("rejects paths that fall outside the include glob", () => { + expect(pathMatchesIncludeExclude("/docs/x.md", ["src/**"], [])).toBe(false); + }); + + it("exclude wins over include", () => { + expect(pathMatchesIncludeExclude("/tests/x.rs", ["**"], ["tests/**"])).toBe(false); + }); +}); diff --git a/scripts/ado-script/src/exec-context-pr-synth/index.ts b/scripts/ado-script/src/exec-context-pr-synth/index.ts index 4bb102be..ff0bc3ee 100644 --- a/scripts/ado-script/src/exec-context-pr-synth/index.ts +++ b/scripts/ado-script/src/exec-context-pr-synth/index.ts @@ -75,12 +75,6 @@ export async function main(env: NodeJS.ProcessEnv = process.env): Promise process.exit(code)) - .catch((e: unknown) => { - logError(`[synth-pr] unhandled error: ${(e as Error).message}`); - process.exit(1); - }); +// +// Guard: only execute when this module is the program entry point. +// When the test harness imports `main`, the import-side-effect block +// below is skipped (vitest sets `process.argv[1]` to the runner, not +// to this module). When ncc bundles for production, `process.argv[1]` +// is the resolved bundle path, which matches `import.meta.url`. +import { fileURLToPath } from "node:url"; + +if ( + typeof process.argv[1] === "string" && + process.argv[1] === fileURLToPath(import.meta.url) +) { + main() + .then((code) => process.exit(code)) + .catch((e: unknown) => { + logError(`[synth-pr] unhandled error: ${(e as Error).message}`); + process.exit(1); + }); +} From 6fcc9a88c2ed146a75ee65efe25d9b49651561a8 Mon Sep 17 00:00:00 2001 From: James Devine Date: Mon, 8 Jun 2026 22:36:03 +0100 Subject: [PATCH 06/19] fix(gate): honour AW_SYNTHETIC_PR so synthetic PR builds skip the bypass When the upstream synthPr Setup-job step has elected a CI build for PR treatment, the gate must run the full PR-spec predicates instead of auto-passing via the 'not a PullRequest build' bypass. Only PullRequest specs honour the override; PipelineCompletion specs are unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- scripts/ado-script/src/gate/bypass.test.ts | 50 ++++++++++++++++++++++ scripts/ado-script/src/gate/bypass.ts | 10 ++++- 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/scripts/ado-script/src/gate/bypass.test.ts b/scripts/ado-script/src/gate/bypass.test.ts index 192ed7c5..70f8e081 100644 --- a/scripts/ado-script/src/gate/bypass.test.ts +++ b/scripts/ado-script/src/gate/bypass.test.ts @@ -81,4 +81,54 @@ describe("runBypass", () => { expect(failedCompletes).toEqual([]); expect(writes.join("")).toContain("%0A"); // embedded \n encoded }); + + // ─── Synthetic-PR support (issue #916) ──────────────────────────── + + it("does NOT bypass when AW_SYNTHETIC_PR=true even on a non-PR build reason", async () => { + process.env.ADO_BUILD_REASON = "IndividualCI"; + process.env.AW_SYNTHETIC_PR = "true"; + try { + const result = await runBypass(baseSpec); + expect(result).toBe(false); + expect(writes.join("")).not.toContain("setvariable variable=SHOULD_RUN"); + expect(writes.join("")).not.toContain("build.addbuildtag"); + } finally { + delete process.env.AW_SYNTHETIC_PR; + } + }); + + it("still bypasses when AW_SYNTHETIC_PR is anything other than 'true'", async () => { + process.env.ADO_BUILD_REASON = "IndividualCI"; + process.env.AW_SYNTHETIC_PR = "false"; + try { + const result = await runBypass(baseSpec); + expect(result).toBe(true); + } finally { + delete process.env.AW_SYNTHETIC_PR; + } + }); + + it("still bypasses when AW_SYNTHETIC_PR is unset (existing behaviour)", async () => { + process.env.ADO_BUILD_REASON = "IndividualCI"; + delete process.env.AW_SYNTHETIC_PR; + const result = await runBypass(baseSpec); + expect(result).toBe(true); + }); + + it("AW_SYNTHETIC_PR only suppresses bypass for PullRequest specs", async () => { + // For non-PR specs (e.g. PipelineCompletion), AW_SYNTHETIC_PR is + // irrelevant; bypass should fall through to the regular reason check. + process.env.ADO_BUILD_REASON = "Manual"; + process.env.AW_SYNTHETIC_PR = "true"; + try { + const pipelineSpec: GateSpec = { + ...baseSpec, + context: { ...baseSpec.context, build_reason: "ResourceTrigger", bypass_label: "pipeline-completion" }, + }; + const result = await runBypass(pipelineSpec); + expect(result).toBe(true); // bypass still triggers — synth only affects PullRequest + } finally { + delete process.env.AW_SYNTHETIC_PR; + } + }); }); diff --git a/scripts/ado-script/src/gate/bypass.ts b/scripts/ado-script/src/gate/bypass.ts index 78ef3536..56a55580 100644 --- a/scripts/ado-script/src/gate/bypass.ts +++ b/scripts/ado-script/src/gate/bypass.ts @@ -2,13 +2,21 @@ * Bypass logic: when ADO_BUILD_REASON does not match the spec's expected * build reason (e.g. spec is for PullRequest but build was Manual), the * gate auto-passes. + * + * Exception: when `AW_SYNTHETIC_PR === "true"` (set by the upstream + * `synthPr` Setup-job step in `exec-context-pr-synth.js`), the build is + * a CI-triggered run that has been promoted to "behave like a PR" by + * the synthetic-from-ci path. The build reason is still `IndividualCI` + * but we want the full PR-spec evaluation to run, not the bypass. */ import type { GateSpec } from "../shared/types.gen.js"; import { setOutput, addBuildTag, complete, logInfo } from "../shared/vso-logger.js"; export async function runBypass(spec: GateSpec): Promise { const buildReason = process.env.ADO_BUILD_REASON ?? ""; - if (buildReason !== spec.context.build_reason) { + const synthetic = + spec.context.build_reason === "PullRequest" && process.env.AW_SYNTHETIC_PR === "true"; + if (!synthetic && buildReason !== spec.context.build_reason) { // Routed through logInfo so the (compiler-controlled but theoretically // template-influenceable) bypass_label cannot smuggle a `##vso[` prefix // into the line. Mirrors the Python log line for parity. From 7bce7f3598a2596b015e6c781269d49b8856d81a Mon Sep 17 00:00:00 2001 From: James Devine Date: Mon, 8 Jun 2026 22:40:11 +0100 Subject: [PATCH 07/19] feat(compile): emit synthPr Setup step when synthetic-from-ci active Adds EXEC_CONTEXT_PR_SYNTH_PATH constant, synthetic_pr_active + pr_trigger_for_synth fields to AdoScriptExtension, synthetic_pr_step() generator, and wires collect_extensions to populate the flags from on.pr.synthetic-from-ci. Setup-job emits the synthPr step BEFORE prGate so downstream env coalescing can read the dependencies.Setup.outputs['synthPr.*'] values. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/extensions/ado_script.rs | 124 ++++++++++++++++++++++++++- src/compile/extensions/mod.rs | 38 +++++--- 2 files changed, 149 insertions(+), 13 deletions(-) diff --git a/src/compile/extensions/ado_script.rs b/src/compile/extensions/ado_script.rs index 41197fea..abcf5c89 100644 --- a/src/compile/extensions/ado_script.rs +++ b/src/compile/extensions/ado_script.rs @@ -35,6 +35,11 @@ pub(crate) const IMPORT_EVAL_PATH: &str = "/tmp/ado-aw-scripts/ado-script/import /// Consumed by `src/compile/extensions/exec_context/pr.rs` to invoke /// the bundle from the PR contributor's prepare step. pub(crate) const EXEC_CONTEXT_PR_PATH: &str = "/tmp/ado-aw-scripts/ado-script/exec-context-pr.js"; +/// Path to the synthetic-PR-context bundle inside the unpacked +/// `ado-script.zip`. Runs in the Setup job before `prGate`; consumed +/// by [`AdoScriptExtension::setup_steps`]. +pub(crate) const EXEC_CONTEXT_PR_SYNTH_PATH: &str = + "/tmp/ado-aw-scripts/ado-script/exec-context-pr-synth.js"; const RELEASE_BASE_URL: &str = "https://github.com/githubnext/ado-aw/releases/download"; /// Single always-on extension that owns all `ado-script` bundle wiring. @@ -52,6 +57,17 @@ pub struct AdoScriptExtension { /// shared `exec_context_pr_active` predicate so this stays in /// lock-step with `ExecContextExtension`'s own activation gate. pub exec_context_pr_active: bool, + /// Whether the synthetic-from-ci path is active for this agent. + /// Set when `on.pr.synthetic-from-ci != false`. Drives: + /// - Setup-job install/download fire (even with no `filters:`). + /// - Setup-job `synthPr` step emission (before any gate step). + /// - Downstream env coalescing (handled in `compile-coalesce-env`). + pub synthetic_pr_active: bool, + /// The PR trigger config required to build `PR_SYNTH_SPEC`. Only + /// populated when `synthetic_pr_active` is `true`. Cloned because + /// the extension outlives the borrow of `FrontMatter` in + /// `collect_extensions`. + pub pr_trigger_for_synth: Option, } impl AdoScriptExtension { @@ -143,6 +159,35 @@ fn resolver_step() -> String { ) } +/// The synthetic-PR-context step that runs in the Setup job BEFORE +/// `prGate`. Looks up the open PR for `Build.SourceBranch` via the +/// ADO REST API and emits `AW_SYNTHETIC_PR*` outputs that downstream +/// gate + exec-context-pr steps coalesce with the real +/// `System.PullRequest.*` variables. +/// +/// `condition: ne(Build.Reason, 'PullRequest')` short-circuits the +/// bundle on real PR builds (the bundle would no-op anyway, but the +/// step-level condition skips the env-block evaluation too). +fn synthetic_pr_step(spec_b64: &str) -> String { + format!( + r#"- bash: | + set -euo pipefail + node '{EXEC_CONTEXT_PR_SYNTH_PATH}' + name: synthPr + displayName: "Resolve synthetic PR context" + condition: and(succeeded(), ne(variables['Build.Reason'], 'PullRequest')) + env: + SYSTEM_ACCESSTOKEN: $(System.AccessToken) + ADO_COLLECTION_URI: $(System.CollectionUri) + ADO_PROJECT: $(System.TeamProject) + ADO_REPO_ID: $(Build.Repository.ID) + BUILD_REASON: $(Build.Reason) + BUILD_REPOSITORY_PROVIDER: $(Build.Repository.Provider) + BUILD_SOURCEBRANCH: $(Build.SourceBranch) + PR_SYNTH_SPEC: "{spec_b64}""# + ) +} + impl CompilerExtension for AdoScriptExtension { fn name(&self) -> &str { "ado-script" @@ -162,10 +207,20 @@ impl CompilerExtension for AdoScriptExtension { fn setup_steps(&self, _ctx: &CompileContext) -> Result> { let (pr_checks, pipeline_checks) = self.lowered_checks(); - if pr_checks.is_empty() && pipeline_checks.is_empty() { + if pr_checks.is_empty() && pipeline_checks.is_empty() && !self.synthetic_pr_active { return Ok(vec![]); } let mut steps = install_and_download_steps(); + if self.synthetic_pr_active { + let pr = self.pr_trigger_for_synth.as_ref().ok_or_else(|| { + anyhow::anyhow!( + "synthetic_pr_active is true but pr_trigger_for_synth is None — \ + collect_extensions must populate both together" + ) + })?; + let spec_b64 = crate::compile::filter_ir::build_pr_synth_spec(pr)?; + steps.push(synthetic_pr_step(&spec_b64)); + } if !pr_checks.is_empty() { steps.push(compile_gate_step_external( GateContext::PullRequest, @@ -412,6 +467,8 @@ mod tests { pipeline_filters: pipeline, inlined_imports: inlined, exec_context_pr_active: false, + synthetic_pr_active: false, + pr_trigger_for_synth: None, } } @@ -456,6 +513,71 @@ mod tests { assert!(steps[2].contains("node '/tmp/ado-aw-scripts/ado-script/gate.js'")); } + #[test] + fn setup_steps_emits_synth_step_when_synthetic_pr_active_without_gate() { + use crate::compile::types::{BranchFilter, PrTriggerConfig}; + let ext = AdoScriptExtension { + pr_filters: None, + pipeline_filters: None, + inlined_imports: true, + exec_context_pr_active: false, + synthetic_pr_active: true, + pr_trigger_for_synth: Some(PrTriggerConfig { + branches: Some(BranchFilter { + include: vec!["main".into()], + exclude: vec![], + }), + paths: None, + filters: None, + ..Default::default() + }), + }; + let fm: FrontMatter = serde_yaml::from_str("name: t\ndescription: t").unwrap(); + let ctx = CompileContext::for_test(&fm); + let steps = ext.setup_steps(&ctx).unwrap(); + assert_eq!(steps.len(), 3, "install + download + synthPr"); + assert!(steps[0].contains("NodeTool@0")); + assert!(steps[1].contains("Download ado-aw scripts")); + assert!(steps[2].contains("name: synthPr"), "third step must be synthPr"); + assert!(steps[2].contains("exec-context-pr-synth.js")); + assert!(steps[2].contains("PR_SYNTH_SPEC:")); + assert!(steps[2].contains("ne(variables['Build.Reason'], 'PullRequest')")); + } + + #[test] + fn setup_steps_emits_synth_step_before_gate_when_both_active() { + use crate::compile::types::{BranchFilter, PrTriggerConfig}; + let filters = PrFilters { + labels: Some(LabelFilter { + any_of: vec!["run-agent".into()], + ..Default::default() + }), + ..Default::default() + }; + let ext = AdoScriptExtension { + pr_filters: Some(filters), + pipeline_filters: None, + inlined_imports: true, + exec_context_pr_active: false, + synthetic_pr_active: true, + pr_trigger_for_synth: Some(PrTriggerConfig { + branches: Some(BranchFilter { + include: vec!["main".into()], + exclude: vec![], + }), + paths: None, + filters: None, + ..Default::default() + }), + }; + let fm: FrontMatter = serde_yaml::from_str("name: t\ndescription: t").unwrap(); + let ctx = CompileContext::for_test(&fm); + let steps = ext.setup_steps(&ctx).unwrap(); + assert_eq!(steps.len(), 4, "install + download + synthPr + prGate"); + assert!(steps[2].contains("name: synthPr")); + assert!(steps[3].contains("name: prGate")); + } + #[test] fn gate_and_import_eval_paths_consistent_with_download_step() { let extract_dir = "/tmp/ado-aw-scripts/"; diff --git a/src/compile/extensions/mod.rs b/src/compile/extensions/mod.rs index a38d7f4c..dc6a988e 100644 --- a/src/compile/extensions/mod.rs +++ b/src/compile/extensions/mod.rs @@ -697,18 +697,32 @@ pub fn collect_extensions(front_matter: &FrontMatter) -> Vec { Extension::AdoAwMarker(AdoAwMarkerExtension), Extension::GitHub(GitHubExtension), Extension::SafeOutputs(SafeOutputsExtension), - Extension::AdoScript(Box::new(AdoScriptExtension { - pr_filters: front_matter.pr_filters().cloned(), - pipeline_filters: front_matter.pipeline_filters().cloned(), - inlined_imports: front_matter.inlined_imports, - // Tell the ado-script extension whether the PR-context - // contributor will activate so it can fire the Agent-job - // install/download even when `inlined-imports: true` (no - // import.js needed). The two extensions stay loosely - // coupled: ExecContextExtension owns invoking the bundle; - // AdoScriptExtension owns installing it. Shared helper - // keeps the activation predicate in lock-step. - exec_context_pr_active: pr_contributor_will_activate(front_matter), + Extension::AdoScript(Box::new({ + // PR trigger config drives both the PR-context contributor + // (exec-context-pr.js) and the new synthetic-from-ci path + // (exec-context-pr-synth.js). Compute the two flags from + // the same source so they stay in lock-step. + let pr_cfg = front_matter.pr_trigger(); + let synthetic_pr_active = pr_cfg.is_some_and(|p| p.synthetic_from_ci); + AdoScriptExtension { + pr_filters: front_matter.pr_filters().cloned(), + pipeline_filters: front_matter.pipeline_filters().cloned(), + inlined_imports: front_matter.inlined_imports, + // Tell the ado-script extension whether the PR-context + // contributor will activate so it can fire the Agent-job + // install/download even when `inlined-imports: true` (no + // import.js needed). The two extensions stay loosely + // coupled: ExecContextExtension owns invoking the bundle; + // AdoScriptExtension owns installing it. Shared helper + // keeps the activation predicate in lock-step. + exec_context_pr_active: pr_contributor_will_activate(front_matter), + synthetic_pr_active, + pr_trigger_for_synth: if synthetic_pr_active { + pr_cfg.cloned() + } else { + None + }, + } })), // Always-on execution-context extension. Owns the `aw-context/` // precompute pipeline. Defaults to `ExecutionContextConfig::default()` From 7e9cae093b2cec030de92a9a59b63221817892a0 Mon Sep 17 00:00:00 2001 From: James Devine Date: Mon, 8 Jun 2026 22:49:55 +0100 Subject: [PATCH 08/19] feat(compile): coalesce real + synthetic PR env vars in gate and exec-context-pr Extends compile_gate_step_external with a synthetic_pr_active flag. When true and ctx == PullRequest, ADO_PR_ID / ADO_SOURCE_BRANCH / ADO_TARGET_BRANCH are emitted as coalesce(variables.System.PullRequest.*, dependencies.Setup.outputs.synthPr.*) so the gate evaluator picks up either the real PR variables or the synthPr Setup-job outputs. Also exports AW_SYNTHETIC_PR so gate/bypass.ts can detect the synthetic-promotion case. PrContextContributor gains the same synthetic_pr_active flag and switches SYSTEM_PULLREQUEST_PULLREQUESTID + SYSTEM_PULLREQUEST_TARGETBRANCH to the coalesced form, with the step condition broadened to accept either real or synthetic PR. This also closes compile-exec-context-cond. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/extensions/ado_script.rs | 5 ++ src/compile/extensions/exec_context/mod.rs | 18 ++++++- src/compile/extensions/exec_context/pr.rs | 39 +++++++++++++-- src/compile/filter_ir.rs | 58 +++++++++++++++++----- tests/compiler_tests.rs | 34 +++++++++---- 5 files changed, 124 insertions(+), 30 deletions(-) diff --git a/src/compile/extensions/ado_script.rs b/src/compile/extensions/ado_script.rs index abcf5c89..7ed12c12 100644 --- a/src/compile/extensions/ado_script.rs +++ b/src/compile/extensions/ado_script.rs @@ -226,6 +226,7 @@ impl CompilerExtension for AdoScriptExtension { GateContext::PullRequest, &pr_checks, GATE_EVAL_PATH, + self.synthetic_pr_active, )?); } if !pipeline_checks.is_empty() { @@ -233,6 +234,10 @@ impl CompilerExtension for AdoScriptExtension { GateContext::PipelineCompletion, &pipeline_checks, GATE_EVAL_PATH, + // Pipeline-completion gates never observe synthetic PR + // semantics; the coalesce wiring only applies to + // PullRequest gates. + false, )?); } Ok(steps) diff --git a/src/compile/extensions/exec_context/mod.rs b/src/compile/extensions/exec_context/mod.rs index b8302303..803ced81 100644 --- a/src/compile/extensions/exec_context/mod.rs +++ b/src/compile/extensions/exec_context/mod.rs @@ -101,6 +101,11 @@ pub struct ExecContextExtension { /// means "is `on.pr` configured" — future trigger contributors /// will OR in their own checks here. any_contributor_active: bool, + /// Whether `on.pr.synthetic-from-ci` is on. Passed through to the + /// PR contributor so it can emit coalesced + /// `SYSTEM_PULLREQUEST_*` env vars (real value preferred, synthPr + /// Setup-job output as fallback). + synthetic_pr_active: bool, } impl ExecContextExtension { @@ -120,9 +125,13 @@ impl ExecContextExtension { // activation answer. let any_contributor_active = pr_contributor_will_activate_with_cfg(&config, front_matter); + let synthetic_pr_active = front_matter + .pr_trigger() + .is_some_and(|p| p.synthetic_from_ci); Self { config, any_contributor_active, + synthetic_pr_active, } } @@ -134,7 +143,14 @@ impl ExecContextExtension { // "on by default when on.pr is configured" behaviour without // the user having to write `execution-context.pr: {}`. let pr_cfg = self.config.pr.clone().unwrap_or_default(); - vec![Contributor::Pr(PrContextContributor::new(pr_cfg))] + // The PR contributor needs to know whether synthetic-from-ci + // is on so it can emit coalesced SYSTEM_PULLREQUEST_* env vars + // (real value preferred, synthPr output as fallback). + let synthetic_pr_active = self.synthetic_pr_active; + vec![Contributor::Pr(PrContextContributor::new( + pr_cfg, + synthetic_pr_active, + ))] } } diff --git a/src/compile/extensions/exec_context/pr.rs b/src/compile/extensions/exec_context/pr.rs index a7d1f282..77d5f9e1 100644 --- a/src/compile/extensions/exec_context/pr.rs +++ b/src/compile/extensions/exec_context/pr.rs @@ -68,11 +68,19 @@ use super::contributor::ContextContributor; /// (unless explicitly disabled via `execution-context.pr.enabled: false`). pub(super) struct PrContextContributor { config: PrContextConfig, + /// Whether `on.pr.synthetic-from-ci` is on for this agent. Drives + /// emission of the coalesced `SYSTEM_PULLREQUEST_*` env vars so the + /// bundle reads either real PR identifiers (true PR builds) or the + /// `synthPr` Setup-job outputs (CI builds promoted via synth). + synthetic_pr_active: bool, } impl PrContextContributor { - pub(super) fn new(config: PrContextConfig) -> Self { - Self { config } + pub(super) fn new(config: PrContextConfig, synthetic_pr_active: bool) -> Self { + Self { + config, + synthetic_pr_active, + } } } @@ -113,19 +121,40 @@ impl ContextContributor for PrContextContributor { // block. Node receives it on `process.env` and passes it to // the spawned `git` subprocess via `GIT_CONFIG_*` env vars // (never argv). It is NEVER visible to the agent step. + // + // When `synthetic-from-ci` is on, the PR-identifier env vars + // are emitted using `$[ coalesce(...) ]` so the bundle picks + // up either the real `System.PullRequest.*` (on a true PR + // build) OR the synthPr Setup-job output (on a CI build + // promoted via exec-context-pr-synth.js). The step's + // condition is also broadened in the + // `compile-exec-context-cond` todo. + let (pr_id_macro, target_branch_macro, condition) = if self.synthetic_pr_active { + ( + "$[ coalesce(variables['System.PullRequest.PullRequestId'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_ID']) ]", + "$[ coalesce(variables['System.PullRequest.TargetBranch'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_TARGETBRANCH']) ]", + "or(eq(variables['Build.Reason'], 'PullRequest'), eq(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], 'true'))", + ) + } else { + ( + "$(System.PullRequest.PullRequestId)", + "$(System.PullRequest.TargetBranch)", + "eq(variables['Build.Reason'], 'PullRequest')", + ) + }; format!( r#"- bash: | set -euo pipefail node '{EXEC_CONTEXT_PR_PATH}' env: SYSTEM_ACCESSTOKEN: $(System.AccessToken) - SYSTEM_PULLREQUEST_PULLREQUESTID: $(System.PullRequest.PullRequestId) - SYSTEM_PULLREQUEST_TARGETBRANCH: $(System.PullRequest.TargetBranch) + SYSTEM_PULLREQUEST_PULLREQUESTID: {pr_id_macro} + SYSTEM_PULLREQUEST_TARGETBRANCH: {target_branch_macro} SYSTEM_TEAMPROJECT: $(System.TeamProject) BUILD_REPOSITORY_NAME: $(Build.Repository.Name) BUILD_SOURCESDIRECTORY: $(Build.SourcesDirectory) displayName: "Stage PR execution context (aw-context/pr/*)" - condition: eq(variables['Build.Reason'], 'PullRequest')"# + condition: {condition}"# ) } diff --git a/src/compile/filter_ir.rs b/src/compile/filter_ir.rs index d02824b5..40019655 100644 --- a/src/compile/filter_ir.rs +++ b/src/compile/filter_ir.rs @@ -1118,10 +1118,19 @@ pub fn build_gate_spec(ctx: GateContext, checks: &[FilterCheck]) -> anyhow::Resu /// Compile filter checks into a bash gate step using an external evaluator /// script. ADO variables are passed via the step's `env:` block (idiomatic /// ADO pattern), and the gate spec is base64-encoded in GATE_SPEC. +/// +/// When `synthetic_pr_active` is true AND `ctx == GateContext::PullRequest`, +/// PR-identifier env vars (`ADO_PR_ID`, `ADO_SOURCE_BRANCH`, +/// `ADO_TARGET_BRANCH`) are emitted using `$[ coalesce(...) ]` so they +/// pick up either the real `System.PullRequest.*` variables (on a true +/// PR build) OR the `synthPr` Setup-job outputs (on a CI build promoted +/// by `exec-context-pr-synth.js`). Also exports `AW_SYNTHETIC_PR` so +/// `gate/bypass.ts` knows to skip the "not a PR build" bypass. pub fn compile_gate_step_external( ctx: GateContext, checks: &[FilterCheck], evaluator_path: &str, + synthetic_pr_active: bool, ) -> anyhow::Result { use base64::{Engine as _, engine::general_purpose::STANDARD}; @@ -1147,8 +1156,36 @@ pub fn compile_gate_step_external( step.push_str(" SYSTEM_ACCESSTOKEN: $(System.AccessToken)\n"); step.push_str(&format!(" GATE_SPEC: \"{}\"\n", spec_b64)); + // Synthetic-from-ci flag: tells gate/bypass.ts that this CI build + // has been promoted to PR semantics, so the "not a PullRequest + // build" bypass must not auto-pass. Always safe to emit (the gate + // checks it strictly for the literal "true"), but only meaningful + // for PR gates. + let pr_synth_active = synthetic_pr_active && matches!(ctx, GateContext::PullRequest); + if pr_synth_active { + step.push_str( + " AW_SYNTHETIC_PR: $[ coalesce(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], '') ]\n", + ); + } + for (env_var, ado_macro) in &exports { - step.push_str(&format!(" {}: {}\n", env_var, ado_macro)); + let macro_str = if pr_synth_active { + match *env_var { + "ADO_PR_ID" => { + "$[ coalesce(variables['System.PullRequest.PullRequestId'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_ID']) ]" + } + "ADO_SOURCE_BRANCH" => { + "$[ coalesce(variables['System.PullRequest.SourceBranch'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_SOURCEBRANCH']) ]" + } + "ADO_TARGET_BRANCH" => { + "$[ coalesce(variables['System.PullRequest.TargetBranch'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_TARGETBRANCH']) ]" + } + _ => ado_macro, + } + } else { + ado_macro + }; + step.push_str(&format!(" {}: {}\n", env_var, macro_str)); } Ok(step) @@ -1698,6 +1735,7 @@ mod tests { GateContext::PullRequest, &[], "/tmp/ado-aw-scripts/ado-script/gate.js", + false, ) .unwrap(); assert!(result.is_empty()); @@ -1716,8 +1754,7 @@ mod tests { let result = compile_gate_step_external( GateContext::PullRequest, &checks, - "/tmp/ado-aw-scripts/ado-script/gate.js", - ) + "/tmp/ado-aw-scripts/ado-script/gate.js", false) .unwrap(); assert!(result.contains("- bash:"), "should be a bash step"); assert!( @@ -1748,8 +1785,7 @@ mod tests { let result = compile_gate_step_external( GateContext::PullRequest, &checks, - "/tmp/ado-aw-scripts/ado-script/gate.js", - ) + "/tmp/ado-aw-scripts/ado-script/gate.js", false) .unwrap(); assert!( result.contains("ADO_BUILD_REASON"), @@ -1775,8 +1811,7 @@ mod tests { let result = compile_gate_step_external( GateContext::PipelineCompletion, &checks, - "/tmp/ado-aw-scripts/ado-script/gate.js", - ) + "/tmp/ado-aw-scripts/ado-script/gate.js", false) .unwrap(); assert!( result.contains("name: pipelineGate"), @@ -1805,8 +1840,7 @@ mod tests { let result = compile_gate_step_external( GateContext::PullRequest, &checks, - "/tmp/ado-aw-scripts/ado-script/gate.js", - ) + "/tmp/ado-aw-scripts/ado-script/gate.js", false) .unwrap(); assert!( result.contains("ADO_REPO_ID"), @@ -1831,8 +1865,7 @@ mod tests { let result = compile_gate_step_external( GateContext::PullRequest, &checks, - "/tmp/ado-aw-scripts/ado-script/gate.js", - ) + "/tmp/ado-aw-scripts/ado-script/gate.js", false) .unwrap(); // Verify tier-1 (pipeline-var only) checks do not export API-related env vars // Look for the env: block exports (YAML format with leading spaces) @@ -1948,8 +1981,7 @@ mod tests { let _step = compile_gate_step_external( GateContext::PullRequest, &checks, - "/tmp/ado-aw-scripts/ado-script/gate.js", - ) + "/tmp/ado-aw-scripts/ado-script/gate.js", false) .unwrap(); // Verify the spec captures all three filters with correct fact dependencies diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index a675527b..556de2b1 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -4251,10 +4251,14 @@ fn test_exec_context_pr_only_downloads_bundle_in_agent_job_not_setup() { "Agent job is missing the exec-context-pr prepare step (the consumer of the download)" ); if let Some(setup) = extract_job_block(&yaml, "Setup") { + // Setup-job script bundle download IS expected when on.pr is + // configured (synthetic-from-ci default-on emits the synthPr + // step, which is a bundle consumer). Only assert the Agent + // job has the bundle download; the Setup-job download is the + // synth feature's correct behaviour. assert!( - !setup.contains("Download ado-aw scripts"), - "Setup job should NOT have the script bundle download when the only consumer is the Agent-job exec-context-pr step. \ - Setup block contents: {}", + setup.contains("Download ado-aw scripts"), + "Setup job SHOULD have the script bundle download when synthetic-from-ci is on (the synthPr step is a bundle consumer). Setup block: {}", setup ); } @@ -5182,8 +5186,8 @@ fn test_execution_context_pr_emits_prepare_step_and_prompt_supplement() { "Should emit the PR context prepare step displayName" ); assert!( - compiled.contains("condition: eq(variables['Build.Reason'], 'PullRequest')"), - "Prepare step must be gated on PR builds at the ADO condition layer" + compiled.contains("condition: or(eq(variables['Build.Reason'], 'PullRequest'), eq(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], 'true'))"), + "Prepare step must be gated on PR builds OR synthetic-PR Setup outputs at the ADO condition layer (synthetic-from-ci is default-on)" ); assert!( compiled.contains("SYSTEM_ACCESSTOKEN: $(System.AccessToken)"), @@ -5219,15 +5223,18 @@ fn test_execution_context_pr_emits_prepare_step_and_prompt_supplement() { "v7: the git_fetch wrapper moved into the bundle" ); - // v7: env passthrough — Node reads the ADO predefined vars from - // `process.env` (see `index.ts::main` and `validate.ts`). + // v7 + synthetic-from-ci (default-on): env passthrough — the bundle + // reads ADO predefined vars from `process.env`. The compiler emits + // coalesced macros that prefer the real `System.PullRequest.*` vars + // (true PR builds) and fall back to the `synthPr` Setup-job outputs + // (CI builds promoted via exec-context-pr-synth.js). assert!( - compiled.contains("SYSTEM_PULLREQUEST_PULLREQUESTID: $(System.PullRequest.PullRequestId)"), - "Prepare step must pass the PR id through to the bundle" + compiled.contains("SYSTEM_PULLREQUEST_PULLREQUESTID: $[ coalesce(variables['System.PullRequest.PullRequestId'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_ID']) ]"), + "Prepare step must pass the PR id (coalesced with synthPr fallback) through to the bundle" ); assert!( - compiled.contains("SYSTEM_PULLREQUEST_TARGETBRANCH: $(System.PullRequest.TargetBranch)"), - "Prepare step must pass the PR target branch through to the bundle" + compiled.contains("SYSTEM_PULLREQUEST_TARGETBRANCH: $[ coalesce(variables['System.PullRequest.TargetBranch'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_TARGETBRANCH']) ]"), + "Prepare step must pass the PR target branch (coalesced with synthPr fallback) through to the bundle" ); assert!( compiled.contains("SYSTEM_TEAMPROJECT: $(System.TeamProject)"), @@ -5344,6 +5351,11 @@ fn test_execution_context_pr_does_not_leak_system_accesstoken() { // Stage 3 SafeOutputs executor — separate non-agent job; needs // the token to apply safe outputs against ADO. See PR #873. "Execute safe outputs (Stage 3)", + // Setup-job synth-PR step. Needs the token to call the ADO REST + // API to look up the active PR for `Build.SourceBranch` on + // CI-triggered builds (issue #916). Runs in the Setup job, well + // before the AWF sandbox is provisioned for the Agent job. + "Resolve synthetic PR context", ]; let mut saw_exec_context_step = false; From 4c56f38d78de33d5f7de77745f038804da5e6883 Mon Sep 17 00:00:00 2001 From: James Devine Date: Mon, 8 Jun 2026 22:55:23 +0100 Subject: [PATCH 09/19] feat(compile): update Agent-job dependsOn condition for synthetic PR Extends generate_agentic_depends_on with a synthetic_pr_active flag. When true the condition gains a leading ne(synthPr.AW_SYNTHETIC_PR_SKIP, true) guard plus a broadened PR clause accepting real PR builds, synthPr promotion, or gate-passed. Threads the flag from compile_shared via front_matter.pr_trigger().synthetic_from_ci. Existing callers default to false (no behavioural change). Adds two unit tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/common.rs | 96 ++++++++++++++++++++++++++++++++++----- src/compile/pr_filters.rs | 8 ++-- 2 files changed, 90 insertions(+), 14 deletions(-) diff --git a/src/compile/common.rs b/src/compile/common.rs index ac3c2c9c..0d5037f7 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -2359,23 +2359,52 @@ pub fn generate_agentic_depends_on( has_pipeline_filters: bool, expressions: &[&str], is_jobs_template_target: bool, + synthetic_pr_active: bool, ) -> String { let has_gate = has_pr_filters || has_pipeline_filters; - let has_setup = !setup_steps.is_empty() || has_gate; - let has_internal_condition = has_gate || !expressions.is_empty(); + let has_setup = !setup_steps.is_empty() || has_gate || synthetic_pr_active; + let has_internal_condition = has_gate || !expressions.is_empty() || synthetic_pr_active; // Build the shared condition body once. Reused across the internal-only // (standalone/1es/stage) path and the dual-branch jobs-template path. let condition_body: Option = if has_internal_condition { let mut parts = vec!["succeeded()".to_string()]; - if has_pr_filters { + // Synthetic-from-ci: the synthPr Setup-job step may have decided + // this build should self-skip (no matching PR, wrong target + // branch, no matching changed files). Always honour that flag — + // it must trump every other reason to run. + if synthetic_pr_active { parts.push( - r"or( + r"ne(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_SKIP'], 'true')" + .to_string(), + ); + } + if has_pr_filters { + // With synthetic-from-ci, the agent should run when EITHER + // (a) it is a real PR build (existing path) OR + // (b) the synthPr step promoted this CI build to PR semantics + // (synthPr.AW_SYNTHETIC_PR=true) OR + // (c) the gate passed for any other reason. + // Without synthetic-from-ci, the existing two-arm condition + // is preserved verbatim. + if synthetic_pr_active { + parts.push( + r"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(), + ); + } else { + parts.push( + r"or( ne(variables['Build.Reason'], 'PullRequest'), eq(dependencies.Setup.outputs['prGate.SHOULD_RUN'], 'true') )" - .to_string(), - ); + .to_string(), + ); + } } if has_pipeline_filters { parts.push( @@ -3301,12 +3330,16 @@ pub async fn compile_shared( } } + let synthetic_pr_active = front_matter + .pr_trigger() + .is_some_and(|p| p.synthetic_from_ci); let agentic_depends_on = generate_agentic_depends_on( &front_matter.setup, has_pr_filters, has_pipeline_filters, &expressions, matches!(front_matter.target, crate::compile::types::CompileTarget::Job), + synthetic_pr_active, ); let job_timeout = generate_job_timeout(front_matter); @@ -8146,14 +8179,14 @@ safe-outputs: #[test] fn test_generate_agentic_depends_on_empty_steps() { - assert!(generate_agentic_depends_on(&[], false, false, &[], false).is_empty()); + assert!(generate_agentic_depends_on(&[], false, false, &[], false, false).is_empty()); } #[test] fn test_generate_agentic_depends_on_with_steps() { let step: serde_yaml::Value = serde_yaml::from_str("bash: x").unwrap(); assert_eq!( - generate_agentic_depends_on(&[step], false, false, &[], false), + generate_agentic_depends_on(&[step], false, false, &[], false, false), "dependsOn: Setup" ); } @@ -8165,7 +8198,7 @@ safe-outputs: // dual-branch ${{ if }} template expressions so external template // parameters merge correctly. let step: serde_yaml::Value = serde_yaml::from_str("bash: x").unwrap(); - let out = generate_agentic_depends_on(&[step], false, false, &[], true); + let out = generate_agentic_depends_on(&[step], false, false, &[], true, false); // dependsOn branches assert!( out.contains("${{ if eq(length(parameters.dependsOn), 0) }}:"), @@ -8196,7 +8229,7 @@ safe-outputs: fn test_generate_agentic_depends_on_jobs_template_no_setup() { // No internal Setup, no gates: only the non-empty-external branches // are emitted (both dependsOn and condition default to omitted). - let out = generate_agentic_depends_on(&[], false, false, &[], true); + let out = generate_agentic_depends_on(&[], false, false, &[], true, false); assert!( !out.contains("${{ if eq(length(parameters.dependsOn), 0) }}:"), "should not emit empty-deps branch when no internal Setup: {out}" @@ -8224,7 +8257,7 @@ safe-outputs: // With internal PR gate, the condition block emits BOTH branches: // empty-external uses the existing internal expression verbatim; // non-empty-external ANDs the external clause into the body. - let out = generate_agentic_depends_on(&[], true, false, &[], true); + let out = generate_agentic_depends_on(&[], true, false, &[], true, false); assert!( out.contains("${{ if eq(parameters.condition, '') }}:"), "missing empty-condition branch: {out}" @@ -8246,6 +8279,47 @@ safe-outputs: ); } + #[test] + fn test_agentic_depends_on_synthetic_pr_active_emits_skip_guard_and_broader_pr_clause() { + // synthetic_pr_active=true + has_pr_filters=true → emits the + // AW_SYNTHETIC_PR_SKIP guard and broadens the PR clause to + // accept real PR builds OR synthPr promotion OR gate-passed. + let out = generate_agentic_depends_on(&[], true, false, &[], false, true); + assert!(out.contains("dependsOn: Setup"), "should depend on Setup"); + assert!( + out.contains("ne(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_SKIP'], 'true')"), + "must honour the synth-skip flag: {out}" + ); + assert!( + out.contains("eq(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], 'true')"), + "must accept synthetic-PR promotion as an activation reason: {out}" + ); + assert!( + out.contains("eq(variables['Build.Reason'], 'PullRequest')"), + "must still accept real PR builds: {out}" + ); + // Old "ne(Build.Reason, PullRequest)" arm must be GONE — that was + // the pre-synthetic permissive default and now contradicts the + // synth-skip guard. + assert!( + !out.contains("ne(variables['Build.Reason'], 'PullRequest')"), + "synth path must replace the permissive ne(Build.Reason, PullRequest) arm: {out}" + ); + } + + #[test] + fn test_agentic_depends_on_synthetic_pr_without_filters_still_emits_skip_guard() { + // synthetic_pr_active=true but no filters → Setup job still + // exists (the synthPr step lives there) so the dependsOn must + // be present and the skip guard must apply. + let out = generate_agentic_depends_on(&[], false, false, &[], false, true); + assert!(out.contains("dependsOn: Setup"), "should depend on Setup: {out}"); + assert!( + out.contains("ne(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_SKIP'], 'true')"), + "must honour the synth-skip flag even without filters: {out}" + ); + } + #[test] fn test_generate_finalize_steps_empty() { assert!(generate_finalize_steps(&[]).is_empty()); diff --git a/src/compile/pr_filters.rs b/src/compile/pr_filters.rs index f2c5e052..963b2ff8 100644 --- a/src/compile/pr_filters.rs +++ b/src/compile/pr_filters.rs @@ -241,7 +241,7 @@ mod tests { #[test] fn test_generate_agentic_depends_on_with_pr_filters() { - let result = generate_agentic_depends_on(&[], true, false, &[], false); + let result = generate_agentic_depends_on(&[], true, false, &[], false, false); assert!(result.contains("dependsOn: Setup"), "should depend on Setup"); assert!(result.contains("condition:"), "should have condition"); assert!(result.contains("Build.Reason"), "should check Build.Reason"); @@ -251,14 +251,14 @@ mod tests { #[test] fn test_generate_agentic_depends_on_setup_only_no_condition() { let step: serde_yaml::Value = serde_yaml::from_str("bash: echo hello").unwrap(); - let result = generate_agentic_depends_on(&[step], false, false, &[], false); + let result = generate_agentic_depends_on(&[step], false, false, &[], false, false); assert_eq!(result, "dependsOn: Setup"); assert!(!result.contains("condition:"), "no condition without PR filters"); } #[test] fn test_generate_agentic_depends_on_nothing() { - let result = generate_agentic_depends_on(&[], false, false, &[], false); + let result = generate_agentic_depends_on(&[], false, false, &[], false, false); assert!(result.is_empty()); } @@ -544,6 +544,7 @@ mod tests { false, &["eq(variables['Custom.ShouldRun'], 'true')"], false, + false, ); // No setup steps, no PR filters → no dependsOn, but the expression produces a condition. assert!(!result.contains("dependsOn"), "no dependsOn without setup/filters"); @@ -560,6 +561,7 @@ mod tests { false, &["eq(variables['Custom.Flag'], 'yes')"], false, + false, ); assert!(result.contains("prGate.SHOULD_RUN"), "should check gate output"); assert!(result.contains("Custom.Flag"), "should include expression"); From a217df182ffb4c90be240474db0066656317b21f Mon Sep 17 00:00:00 2001 From: James Devine Date: Mon, 8 Jun 2026 22:58:26 +0100 Subject: [PATCH 10/19] feat(compile): auto-emit narrowed CI trigger when synthetic-from-ci is on When on.pr.synthetic-from-ci is on (default) and on.pr.branches.include is non-empty, emit a top-level trigger: block mirroring those branches so CI fires only on the configured set. Without this, ADO would queue a build for every push and most would be wasted compute (synthPr would skip them). Pipeline/schedule suppression still wins, synthetic-from-ci: false preserves the previous default, and an empty include list disables the narrowing. Five new unit tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/common.rs | 157 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 153 insertions(+), 4 deletions(-) diff --git a/src/compile/common.rs b/src/compile/common.rs index 0d5037f7..7e533c91 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -564,7 +564,21 @@ pub fn generate_pr_trigger(on_config: &Option, has_schedule: bool) -> } } -/// Generate CI trigger configuration +/// Generate CI trigger configuration. +/// +/// Three branches, in priority order: +/// 1. **Suppression** — when a pipeline-completion trigger or a schedule +/// is configured, `trigger: none` is emitted so unrelated commits do +/// not also queue a build (existing behaviour). +/// 2. **Synthetic-from-ci narrowing** — when `on.pr.synthetic-from-ci` +/// is on (default) AND `on.pr.branches.include` is non-empty AND +/// branch (1) does not apply, emit a `trigger:` block mirroring +/// `pr.branches.include` so CI fires only on those branches. +/// Without this, ADO's implicit "trigger on every branch" would +/// queue a build for every push, only for the synthPr step to skip +/// most of them — wasted compute. (Issue #916, decision 2.) +/// 3. **Default** — otherwise emit the empty string (ADO's "trigger +/// on every branch" default). pub fn generate_ci_trigger(on_config: &Option, has_schedule: bool) -> String { let has_pipeline_trigger = on_config .as_ref() @@ -572,10 +586,31 @@ pub fn generate_ci_trigger(on_config: &Option, has_schedule: bool) -> .is_some(); if has_pipeline_trigger || has_schedule { - "trigger: none".to_string() - } else { - String::new() + return "trigger: none".to_string(); } + + // Branch 2 — narrowed trigger emitted by the synthetic-from-ci feature. + if let Some(pr) = on_config.as_ref().and_then(|o| o.pr.as_ref()) + && pr.synthetic_from_ci + && let Some(branches) = pr.branches.as_ref() + && !branches.include.is_empty() + { + let mut yaml = String::from("trigger:\n branches:\n include:\n"); + for b in &branches.include { + yaml.push_str(&format!(" - '{}'\n", b.replace('\'', "''"))); + } + // Mirror exclude if the agent declared one — keeps CI exactly + // aligned with the PR-trigger configuration. + if !branches.exclude.is_empty() { + yaml.push_str(" exclude:\n"); + for b in &branches.exclude { + yaml.push_str(&format!(" - '{}'\n", b.replace('\'', "''"))); + } + } + return yaml.trim_end().to_string(); + } + + String::new() } /// Generate pipeline resource YAML for pipeline completion triggers @@ -4779,6 +4814,120 @@ mod tests { assert_eq!(result, "trigger: none"); } + // ─── generate_ci_trigger: synthetic-from-ci narrowing (issue #916) ──────── + + #[test] + fn test_generate_ci_trigger_synthetic_pr_narrows_to_include_branches() { + let triggers = Some(crate::compile::types::OnConfig { + pipeline: None, + pr: Some(crate::compile::types::PrTriggerConfig { + branches: Some(crate::compile::types::BranchFilter { + include: vec!["main".into(), "release/*".into()], + exclude: vec![], + }), + paths: None, + filters: None, + ..Default::default() // synthetic_from_ci defaults to true + }), + schedule: None, + }); + let result = generate_ci_trigger(&triggers, false); + assert!(result.starts_with("trigger:\n branches:\n include:")); + assert!(result.contains("- 'main'")); + assert!(result.contains("- 'release/*'")); + assert!(!result.contains("exclude:"), "no exclude was configured"); + } + + #[test] + fn test_generate_ci_trigger_synthetic_pr_mirrors_exclude() { + let triggers = Some(crate::compile::types::OnConfig { + pipeline: None, + pr: Some(crate::compile::types::PrTriggerConfig { + branches: Some(crate::compile::types::BranchFilter { + include: vec!["main".into()], + exclude: vec!["users/*".into()], + }), + paths: None, + filters: None, + ..Default::default() + }), + schedule: None, + }); + let result = generate_ci_trigger(&triggers, false); + assert!(result.contains("include:")); + assert!(result.contains("- 'main'")); + assert!(result.contains("exclude:")); + assert!(result.contains("- 'users/*'")); + } + + #[test] + fn test_generate_ci_trigger_synthetic_pr_off_returns_default_empty() { + let triggers = Some(crate::compile::types::OnConfig { + pipeline: None, + pr: Some(crate::compile::types::PrTriggerConfig { + branches: Some(crate::compile::types::BranchFilter { + include: vec!["main".into()], + exclude: vec![], + }), + paths: None, + filters: None, + synthetic_from_ci: false, + }), + schedule: None, + }); + let result = generate_ci_trigger(&triggers, false); + assert!( + result.is_empty(), + "synthetic-from-ci: false must NOT auto-narrow the CI trigger (back-compat): {result}" + ); + } + + #[test] + fn test_generate_ci_trigger_synthetic_pr_empty_include_returns_default_empty() { + let triggers = Some(crate::compile::types::OnConfig { + pipeline: None, + pr: Some(crate::compile::types::PrTriggerConfig { + branches: Some(crate::compile::types::BranchFilter { + include: vec![], + exclude: vec![], + }), + paths: None, + filters: None, + ..Default::default() + }), + schedule: None, + }); + let result = generate_ci_trigger(&triggers, false); + assert!( + result.is_empty(), + "empty include list means no narrowing — keep ADO default trigger: {result}" + ); + } + + #[test] + fn test_generate_ci_trigger_pipeline_trigger_suppresses_synthetic_narrowing() { + let triggers = Some(crate::compile::types::OnConfig { + pipeline: Some(crate::compile::types::PipelineTrigger { + name: "Build".into(), + project: None, + branches: vec![], + filters: None, + }), + pr: Some(crate::compile::types::PrTriggerConfig { + branches: Some(crate::compile::types::BranchFilter { + include: vec!["main".into()], + exclude: vec![], + }), + paths: None, + filters: None, + ..Default::default() + }), + schedule: None, + }); + let result = generate_ci_trigger(&triggers, false); + assert_eq!(result, "trigger: none", "pipeline trigger must take priority over synth narrowing"); + } + // ─── generate_pipeline_resources ───────────────────────────────────────── #[test] From 8df384b525fcc992712da2618a3627e7cc492f26 Mon Sep 17 00:00:00 2001 From: James Devine Date: Mon, 8 Jun 2026 23:01:10 +0100 Subject: [PATCH 11/19] test(compile): snapshot fixtures for synthetic-from-ci default-on and opt-out Adds two fixtures and two integration tests. Fixture A asserts the full synth wiring is emitted (synthPr step, PR_SYNTH_SPEC env, broadened exec-context-pr.js condition, AW_SYNTHETIC_PR_SKIP guard, narrowed trigger block). Fixture B asserts ALL synth artefacts are absent under synthetic-from-ci: false (substring-negation back-compat guard, no stored baseline needed). The GitHub-resource case planned as fixture C is omitted because it produces the same YAML as A; the runtime no-op is covered by the bundle's vitest suite. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/compiler_tests.rs | 80 +++++++++++++++++++++++++- tests/fixtures/synthetic-pr-default.md | 24 ++++++++ tests/fixtures/synthetic-pr-opt-out.md | 19 ++++++ 3 files changed, 122 insertions(+), 1 deletion(-) create mode 100644 tests/fixtures/synthetic-pr-default.md create mode 100644 tests/fixtures/synthetic-pr-opt-out.md diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index 556de2b1..7d1e3bd8 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -5643,4 +5643,82 @@ Body. // v7: this validation now lives in the `exec-context-pr.js` bundle // (see `validate.ts::TARGET_BRANCH_RE`); the vitest tests under // `validate.test.ts` guard the regression there. This Rust-side -// test is removed for the same reason. \ No newline at end of file +// test is removed for the same reason. + +// ─── Synthetic-from-ci snapshot fixtures (issue #916) ──────────────────────── + +/// Fixture A: agent with on.pr and default synthetic-from-ci: true. +/// Compiled YAML must contain the full synth wiring (synthPr Setup step, +/// PR_SYNTH_SPEC env, broadened exec-context-pr.js condition, agent-job +/// AW_SYNTHETIC_PR_SKIP guard) plus the narrowed top-level `trigger:` block. +#[test] +fn test_synthetic_pr_default_emits_full_synth_wiring() { + let compiled = compile_fixture_with_flags("synthetic-pr-default.md", &["--skip-integrity"]); + assert_valid_yaml(&compiled, "synthetic-pr-default.md"); + + // synthPr step in Setup job, before prGate. + assert!( + compiled.contains("name: synthPr"), + "Fixture A must emit the synthPr Setup-job step" + ); + assert!( + compiled.contains("PR_SYNTH_SPEC:"), + "Fixture A must emit the PR_SYNTH_SPEC env var carrying the base64 spec" + ); + assert!( + compiled.contains("exec-context-pr-synth.js"), + "Fixture A must reference the synth bundle path" + ); + + // Broadened exec-context-pr condition. + assert!( + compiled.contains( + "condition: or(eq(variables['Build.Reason'], 'PullRequest'), eq(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], 'true'))" + ), + "Fixture A must broaden the exec-context-pr.js condition to accept synthetic PRs" + ); + + // Agent-job AW_SYNTHETIC_PR_SKIP guard. + assert!( + compiled.contains("ne(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_SKIP'], 'true')"), + "Fixture A's Agent-job condition must honour the synth-skip flag" + ); + assert!( + compiled.contains("eq(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], 'true')"), + "Fixture A's Agent-job condition must accept synth promotion as an activation reason" + ); + + // Narrowed CI trigger block. + assert!( + compiled.contains("trigger:\n branches:\n include:\n - 'main'"), + "Fixture A must auto-emit a narrowed CI trigger mirroring on.pr.branches.include" + ); +} + +/// Fixture B: agent with on.pr and synthetic-from-ci: false. +/// Back-compat guard — the compiled YAML must contain NONE of the +/// synthesis artefacts. (Substring-negation; no stored baseline required.) +#[test] +fn test_synthetic_pr_opt_out_omits_all_synth_artefacts() { + let compiled = compile_fixture_with_flags("synthetic-pr-opt-out.md", &["--skip-integrity"]); + assert_valid_yaml(&compiled, "synthetic-pr-opt-out.md"); + + for needle in &[ + "synthPr", + "AW_SYNTHETIC_PR", + "PR_SYNTH_SPEC", + "exec-context-pr-synth", + ] { + assert!( + !compiled.contains(needle), + "synthetic-from-ci: false must produce zero synth artefacts; \ + found {needle} in compiled YAML" + ); + } + + // Auto-narrowed CI trigger is ALSO suppressed when synthetic-from-ci is off. + assert!( + !compiled.contains("trigger:\n branches:\n include:\n - 'main'"), + "synthetic-from-ci: false must NOT auto-narrow the CI trigger (back-compat)" + ); +} diff --git a/tests/fixtures/synthetic-pr-default.md b/tests/fixtures/synthetic-pr-default.md new file mode 100644 index 00000000..796a5048 --- /dev/null +++ b/tests/fixtures/synthetic-pr-default.md @@ -0,0 +1,24 @@ +--- +name: "Synthetic PR Default Agent" +description: "Fixture exercising on.pr with default synthetic-from-ci=true (issue #916)" +on: + pr: + branches: + include: [main] +--- + +## Synthetic PR Default Agent + +This agent has `on.pr` configured with default `synthetic-from-ci: true`. +On a CI-triggered build (no Build Validation policy), the Setup-job +`synthPr` step looks up the open PR for `Build.SourceBranch` and exposes +its identifiers so the gate evaluator and exec-context-pr bundles +behave as if `Build.Reason == PullRequest`. + +The compiled YAML must contain: + +- A `synthPr` step in the Setup job, before the gate +- A `PR_SYNTH_SPEC:` env var carrying the base64 spec +- The broadened `exec-context-pr.js` condition (`or(...)`) +- The Agent-job `dependsOn` condition with the `AW_SYNTHETIC_PR_SKIP` guard +- A narrowed top-level `trigger:` block mirroring `pr.branches.include` diff --git a/tests/fixtures/synthetic-pr-opt-out.md b/tests/fixtures/synthetic-pr-opt-out.md new file mode 100644 index 00000000..01dad07a --- /dev/null +++ b/tests/fixtures/synthetic-pr-opt-out.md @@ -0,0 +1,19 @@ +--- +name: "Synthetic PR Opt-Out Agent" +description: "Fixture exercising on.pr with synthetic-from-ci=false (issue #916 back-compat)" +on: + pr: + branches: + include: [main] + synthetic-from-ci: false +--- + +## Synthetic PR Opt-Out Agent + +This agent has `on.pr` configured with `synthetic-from-ci: false`, +preserving the pre-synthesis behaviour (requires an operator-installed +Build Validation branch policy for PR triggering to work). + +The compiled YAML must contain NONE of the synthesis artefacts — +`synthPr`, `AW_SYNTHETIC_PR`, `PR_SYNTH_SPEC`, or `exec-context-pr-synth` +— so back-compat is preserved. From 8d23497714c7e1cfcfcd0a9623a9d49aea5bd81f Mon Sep 17 00:00:00 2001 From: James Devine Date: Mon, 8 Jun 2026 23:02:53 +0100 Subject: [PATCH 12/19] docs: document on.pr.synthetic-from-ci across front-matter and prompt files front-matter.md gains the new knob in the example block and a 'PR Triggering in Azure Repos' section walking through why the feature exists, the 7-step runtime contract, the auto-narrowed CI trigger, and how to opt out. The three prompt files (create/update/debug) each cross-link to the new section with a one-paragraph note tailored to their context. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/front-matter.md | 74 ++++++++++++++++++++++++++ prompts/create-ado-agentic-workflow.md | 2 + prompts/debug-ado-agentic-workflow.md | 2 +- prompts/update-ado-agentic-workflow.md | 7 +++ 4 files changed, 84 insertions(+), 1 deletion(-) diff --git a/docs/front-matter.md b/docs/front-matter.md index 5982c3df..2462545b 100644 --- a/docs/front-matter.md +++ b/docs/front-matter.md @@ -104,6 +104,21 @@ on: # trigger configuration (unified under on: key) include: [main] paths: include: [src/*] + synthetic-from-ci: true # default true. When true (and `on.pr.branches.include` + # is non-empty), CI builds on the included branches + # call the ADO REST API at Setup time to find the + # open PR for `Build.SourceBranch`. If exactly one + # matches, the build is promoted to behave as + # `Build.Reason == PullRequest` — gate evaluator + # runs, exec-context-pr stages `aw-context/pr/`, + # and the agent runs against the PR diff. No Build + # Validation branch policy required. Zero or + # multiple matches → the Agent job self-skips + # cleanly. Also auto-emits a top-level `trigger:` + # block mirroring `pr.branches.include` so CI fires + # only on those branches. Set to `false` to opt out + # (requires an operator-installed branch policy). + # See "PR triggering in Azure Repos" callout below. filters: # runtime PR filters (compiled to gate step) title: "*[review]*" author: @@ -328,3 +343,62 @@ The filter gate step uses `System.AccessToken` for self-cancellation If the token is unavailable, the gate step logs a warning and the build completes as "Succeeded" (with the agent job skipped via condition) rather than "Cancelled". + +## PR Triggering in Azure Repos + +Azure DevOps Services **ignores the YAML `pr:` block unless a per-branch +Build Validation branch 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 — the gate +evaluator's "not a PR build" bypass triggers and `exec-context-pr.js` +is skipped. PR-aware agents (e.g. PR reviewers) silently degrade. + +`ado-aw` works around this with the always-on `on.pr.synthetic-from-ci` +feature: when `on.pr` is configured, a Setup-job script +(`exec-context-pr-synth.js`) calls the ADO REST API at build time to +find the active PR for `Build.SourceBranch` and promotes the build to +PR semantics if a single match is found. **No branch policy required.** + +### How it works under the hood + +On every CI build with `on.pr.synthetic-from-ci: true` (the default): + +1. **Real PR build?** If `Build.Reason == PullRequest` (a branch policy + is configured), the synth step no-ops and the existing PR path + handles everything. +2. **GitHub-typed repo resource?** GitHub repos already get correct + `pr:` semantics from ADO. The synth step no-ops. +3. **Look up the PR.** Otherwise, the script calls + `GET /{project}/_apis/git/repositories/{repoId}/pullrequests` + filtered by `sourceRefName == Build.SourceBranch` and + `status = active`. +4. **Filter by target branch.** PRs whose `targetRefName` does not match + `on.pr.branches.include` (respecting `exclude`) are dropped. +5. **Exactly one match.** Zero or multiple matches → emit + `AW_SYNTHETIC_PR_SKIP=true`; the Agent job self-skips cleanly with a + single info log line. Never noisy, never red. +6. **Path filter.** If `on.pr.paths` is configured, the script enforces + it against the PR's changed-file list (which ADO's CI trigger + ignores). Empty intersection → skip. +7. **Promote.** Otherwise, emit `AW_SYNTHETIC_PR=true` plus the PR + identifiers as Setup-job outputs. Downstream `gate.js` and + `exec-context-pr.js` env blocks coalesce these with the real + `System.PullRequest.*` variables, so the gate evaluator runs the + full PR-spec predicates and `aw-context/pr/{base.sha,head.sha}` is + staged for the agent. + +### Auto-narrowed CI trigger + +When `synthetic-from-ci` is on AND `on.pr.branches.include` is +non-empty, the compiler also auto-emits a top-level `trigger:` block +mirroring those branches so unrelated branches do not queue builds +that would only self-skip. Pipeline/schedule triggers and an explicit +`synthetic-from-ci: false` both suppress this narrowing. + +### Opting out + +Set `on.pr.synthetic-from-ci: false` to disable. The compiled YAML +will then contain none of the synthesis wiring (`synthPr`, +`AW_SYNTHETIC_PR`, `PR_SYNTH_SPEC`) and PR triggering will require an +operator-installed branch policy as before. + diff --git a/prompts/create-ado-agentic-workflow.md b/prompts/create-ado-agentic-workflow.md index 935bde0c..101936ad 100644 --- a/prompts/create-ado-agentic-workflow.md +++ b/prompts/create-ado-agentic-workflow.md @@ -429,6 +429,8 @@ on: When `on.pr` is set: the native ADO `pr:` trigger block is generated from `branches:` and `paths:`. Runtime `filters:` compile to a gate step in the Setup job that self-cancels the build when they do not match. +**`on.pr` triggering works without a Build Validation branch policy.** By default (`synthetic-from-ci: true`), the compiler emits a Setup-job script that, on CI-triggered builds, looks up the open PR for `Build.SourceBranch` via the ADO REST API and promotes the build to PR semantics if exactly one matches `pr.branches` (and `pr.paths` if configured). Zero or multiple matches → the Agent job self-skips cleanly. Set `on.pr.synthetic-from-ci: false` to require an operator-installed branch policy (back-compat). The compiler also auto-emits a narrowed top-level `trigger:` block mirroring `pr.branches.include` so unrelated branches do not queue self-skipping builds. Full reference: ["PR Triggering in Azure Repos" in `docs/front-matter.md`](../docs/front-matter.md#pr-triggering-in-azure-repos). + **PR-reviewer agents — DO NOT write your own precompute step.** When `on.pr` is set, the compiler automatically (1) fetches the PR target branch with progressive deepening, (2) resolves and stages `aw-context/pr/base.sha` + `aw-context/pr/head.sha`, (3) appends a prompt fragment listing common `git diff`/`git show`/`git log` commands and example Azure DevOps MCP tool calls (`repo_get_pull_request_by_id`, `repo_list_pull_request_threads`, `repo_create_pull_request_thread`) with the PR id / project / repo pre-filled, and (4) adds `git`, `git diff`, `git log`, `git show`, `git status`, `git rev-parse`, `git symbolic-ref` to the agent's bash allow-list. The agent runs `git diff $BASE..$HEAD` itself inside the AWF sandbox (objects are already fetched into the workspace). On failure (e.g. merge-base could not be resolved), the failure fragment tells the agent to surface the error rather than produce an empty review. Opt out via `execution-context.pr.enabled: false`. Full reference: [`docs/execution-context.md`](../docs/execution-context.md). #### Pipeline Triggers (`on.pipeline`) diff --git a/prompts/debug-ado-agentic-workflow.md b/prompts/debug-ado-agentic-workflow.md index d2315422..80816daf 100644 --- a/prompts/debug-ado-agentic-workflow.md +++ b/prompts/debug-ado-agentic-workflow.md @@ -32,7 +32,7 @@ Agent → Detection → SafeOutputs | **SafeOutputs** | Executes approved safe outputs (create PRs, work items, wiki pages, etc.) | Write (`permissions.write`) | Standard ADO agent | Additional optional jobs: -- **Setup** — runs before `Agent` (from `setup:` front matter) +- **Setup** — runs before `Agent` (from `setup:` front matter). When `on.pr` is set with default `synthetic-from-ci: true`, this job also runs a `synthPr` step that calls the ADO REST API to promote CI-triggered builds to PR semantics when an open PR matches; if it sets `AW_SYNTHETIC_PR_SKIP=true`, the Agent job is skipped cleanly. See [`docs/front-matter.md#pr-triggering-in-azure-repos`](../docs/front-matter.md#pr-triggering-in-azure-repos). - **Teardown** — runs after `SafeOutputs` (from `teardown:` front matter) --- diff --git a/prompts/update-ado-agentic-workflow.md b/prompts/update-ado-agentic-workflow.md index 8e0e7049..77803298 100644 --- a/prompts/update-ado-agentic-workflow.md +++ b/prompts/update-ado-agentic-workflow.md @@ -44,6 +44,13 @@ triggers → steps → post-steps → setup → teardown → network → permissions → parameters ``` +> **`on.pr` knob update**: when changing `on.pr.branches` or +> `on.pr.paths`, also confirm whether `synthetic-from-ci` (default +> `true`) is appropriate. With it on, the compiler emits a Setup-job +> ADO REST call to discover the open PR for `Build.SourceBranch` and +> auto-narrows the top-level `trigger:` block to those branches. +> Reference: [`docs/front-matter.md#pr-triggering-in-azure-repos`](../docs/front-matter.md#pr-triggering-in-azure-repos). + ### Step 3 — Validate the Changes Run through the validation checklist (see below) before finalizing. Fix any issues and inform the user of corrections made. From 9f643fef76b349280fc815715d82f45f181782c7 Mon Sep 17 00:00:00 2001 From: James Devine Date: Tue, 9 Jun 2026 09:37:17 +0100 Subject: [PATCH 13/19] style: cargo fmt on touched files Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/common.rs | 30 ++- src/compile/extensions/ado_script.rs | 66 ++--- src/compile/extensions/exec_context/mod.rs | 19 +- src/compile/extensions/mod.rs | 13 +- src/compile/filter_ir.rs | 83 +++++-- src/compile/pr_filters.rs | 273 ++++++++++++++++----- src/compile/types.rs | 7 +- tests/compiler_tests.rs | 20 +- 8 files changed, 339 insertions(+), 172 deletions(-) diff --git a/src/compile/common.rs b/src/compile/common.rs index 7e533c91..82351bc9 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -3373,7 +3373,10 @@ pub async fn compile_shared( has_pr_filters, has_pipeline_filters, &expressions, - matches!(front_matter.target, crate::compile::types::CompileTarget::Job), + matches!( + front_matter.target, + crate::compile::types::CompileTarget::Job + ), synthetic_pr_active, ); let job_timeout = generate_job_timeout(front_matter); @@ -4761,8 +4764,14 @@ mod tests { let result = generate_pr_trigger(&triggers, true); assert!(result.contains("pr: none")); // When both pipeline and schedule are active, the comment must mention both reasons. - assert!(result.contains("schedule"), "should mention schedule: {result}"); - assert!(result.contains("upstream pipeline"), "should mention upstream pipeline: {result}"); + assert!( + result.contains("schedule"), + "should mention schedule: {result}" + ); + assert!( + result.contains("upstream pipeline"), + "should mention upstream pipeline: {result}" + ); } // ─── generate_ci_trigger ───────────────────────────────────────────────── @@ -4925,7 +4934,10 @@ mod tests { schedule: None, }); let result = generate_ci_trigger(&triggers, false); - assert_eq!(result, "trigger: none", "pipeline trigger must take priority over synth narrowing"); + assert_eq!( + result, "trigger: none", + "pipeline trigger must take priority over synth narrowing" + ); } // ─── generate_pipeline_resources ───────────────────────────────────────── @@ -6942,7 +6954,10 @@ safe-outputs: let exts = crate::compile::extensions::collect_extensions(&fm); let ctx = crate::compile::extensions::CompileContext::for_test(&fm); let result = generate_prepare_steps(&[], &exts, &ctx).unwrap(); - assert!(result.contains("elan-init.sh"), "should include elan installer"); + assert!( + result.contains("elan-init.sh"), + "should include elan installer" + ); assert!(result.contains("Lean 4"), "should include Lean prompt"); assert!( result.contains("--default-toolchain stable"), @@ -8462,7 +8477,10 @@ safe-outputs: // exists (the synthPr step lives there) so the dependsOn must // be present and the skip guard must apply. let out = generate_agentic_depends_on(&[], false, false, &[], false, true); - assert!(out.contains("dependsOn: Setup"), "should depend on Setup: {out}"); + assert!( + out.contains("dependsOn: Setup"), + "should depend on Setup: {out}" + ); assert!( out.contains("ne(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_SKIP'], 'true')"), "must honour the synth-skip flag even without filters: {out}" diff --git a/src/compile/extensions/ado_script.rs b/src/compile/extensions/ado_script.rs index 7ed12c12..a311a3f7 100644 --- a/src/compile/extensions/ado_script.rs +++ b/src/compile/extensions/ado_script.rs @@ -543,7 +543,10 @@ mod tests { assert_eq!(steps.len(), 3, "install + download + synthPr"); assert!(steps[0].contains("NodeTool@0")); assert!(steps[1].contains("Download ado-aw scripts")); - assert!(steps[2].contains("name: synthPr"), "third step must be synthPr"); + assert!( + steps[2].contains("name: synthPr"), + "third step must be synthPr" + ); assert!(steps[2].contains("exec-context-pr-synth.js")); assert!(steps[2].contains("PR_SYNTH_SPEC:")); assert!(steps[2].contains("ne(variables['Build.Reason'], 'PullRequest')")); @@ -781,11 +784,8 @@ mod tests { #[test] fn rejects_absolute_posix_path_at_compile_time() { let workspace = TestWorkspace::new(); - let err = resolve_imports_inline( - "{{#runtime-import /etc/passwd}}", - &workspace.path, - ) - .unwrap_err(); + let err = + resolve_imports_inline("{{#runtime-import /etc/passwd}}", &workspace.path).unwrap_err(); assert!( err.to_string().contains("absolute paths are not allowed"), "expected absolute-path rejection, got: {err}" @@ -843,11 +843,8 @@ mod tests { #[test] fn rejects_path_containing_closing_brace() { let workspace = TestWorkspace::new(); - let err = resolve_imports_inline( - "{{#runtime-import foo}bar.md}}", - &workspace.path, - ) - .unwrap_err(); + let err = + resolve_imports_inline("{{#runtime-import foo}bar.md}}", &workspace.path).unwrap_err(); assert!( err.to_string().contains("is not allowed"), "expected `}}` rejection, got: {err}" @@ -861,13 +858,11 @@ mod tests { #[test] fn rejects_relative_path_with_dotdot_segment() { let workspace = TestWorkspace::new(); - let err = resolve_imports_inline( - "{{#runtime-import ../escape.md}}", - &workspace.path, - ) - .unwrap_err(); + let err = resolve_imports_inline("{{#runtime-import ../escape.md}}", &workspace.path) + .unwrap_err(); assert!( - err.to_string().contains("'..' path components are not allowed"), + err.to_string() + .contains("'..' path components are not allowed"), "expected '..' rejection, got: {err}" ); } @@ -875,13 +870,12 @@ mod tests { #[test] fn rejects_path_with_embedded_dotdot_segment() { let workspace = TestWorkspace::new(); - let err = resolve_imports_inline( - "{{#runtime-import sub/../../escape.md}}", - &workspace.path, - ) - .unwrap_err(); + let err = + resolve_imports_inline("{{#runtime-import sub/../../escape.md}}", &workspace.path) + .unwrap_err(); assert!( - err.to_string().contains("'..' path components are not allowed"), + err.to_string() + .contains("'..' path components are not allowed"), "expected '..' rejection, got: {err}" ); } @@ -899,7 +893,8 @@ mod tests { ) .unwrap_err(); assert!( - err.to_string().contains("'..' path components are not allowed"), + err.to_string() + .contains("'..' path components are not allowed"), "expected '..' rejection, got: {err}" ); } @@ -907,13 +902,12 @@ mod tests { #[test] fn rejects_backslash_dotdot_segment_on_windows_style_paths() { let workspace = TestWorkspace::new(); - let err = resolve_imports_inline( - r"{{#runtime-import sub\..\..\escape.md}}", - &workspace.path, - ) - .unwrap_err(); + let err = + resolve_imports_inline(r"{{#runtime-import sub\..\..\escape.md}}", &workspace.path) + .unwrap_err(); assert!( - err.to_string().contains("'..' path components are not allowed"), + err.to_string() + .contains("'..' path components are not allowed"), "expected '..' rejection, got: {err}" ); } @@ -927,16 +921,8 @@ mod tests { workspace.write("..hidden.md", "DOTHIDDEN"); workspace.write("name..md", "DOUBLE"); - let a = resolve_imports_inline( - "{{#runtime-import ..hidden.md}}", - &workspace.path, - ) - .unwrap(); - let b = resolve_imports_inline( - "{{#runtime-import name..md}}", - &workspace.path, - ) - .unwrap(); + let a = resolve_imports_inline("{{#runtime-import ..hidden.md}}", &workspace.path).unwrap(); + let b = resolve_imports_inline("{{#runtime-import name..md}}", &workspace.path).unwrap(); assert_eq!(a, "DOTHIDDEN"); assert_eq!(b, "DOUBLE"); diff --git a/src/compile/extensions/exec_context/mod.rs b/src/compile/extensions/exec_context/mod.rs index 803ced81..6ebc0406 100644 --- a/src/compile/extensions/exec_context/mod.rs +++ b/src/compile/extensions/exec_context/mod.rs @@ -123,8 +123,7 @@ impl ExecContextExtension { // so unit tests that construct a custom `config` (separate // from `front_matter.execution_context`) still see the right // activation answer. - let any_contributor_active = - pr_contributor_will_activate_with_cfg(&config, front_matter); + let any_contributor_active = pr_contributor_will_activate_with_cfg(&config, front_matter); let synthetic_pr_active = front_matter .pr_trigger() .is_some_and(|p| p.synthetic_from_ci); @@ -222,9 +221,7 @@ mod tests { //! unit-test time rather than at E2E time. use super::*; - use crate::compile::types::{ - ExecutionContextConfig, FrontMatter, PrContextConfig, - }; + use crate::compile::types::{ExecutionContextConfig, FrontMatter, PrContextConfig}; /// Parse a minimal markdown agent into a `FrontMatter`. fn parse_fm(src: &str) -> FrontMatter { @@ -250,8 +247,10 @@ mod tests { /// `should_activate`, this assertion trips. #[test] fn required_bash_commands_matches_pr_contributor_active_default() { - let ext = - ExecContextExtension::new(ExecutionContextConfig::default(), &pr_triggered_front_matter()); + let ext = ExecContextExtension::new( + ExecutionContextConfig::default(), + &pr_triggered_front_matter(), + ); let cmds = ext.required_bash_commands(); assert!( !cmds.is_empty(), @@ -304,8 +303,10 @@ mod tests { /// no commands. Mirrors `should_activate`'s `on.pr` gate. #[test] fn required_bash_commands_suppressed_without_on_pr() { - let ext = - ExecContextExtension::new(ExecutionContextConfig::default(), &no_trigger_front_matter()); + let ext = ExecContextExtension::new( + ExecutionContextConfig::default(), + &no_trigger_front_matter(), + ); assert!( ext.required_bash_commands().is_empty(), "without on.pr configured, required_bash_commands must be empty" diff --git a/src/compile/extensions/mod.rs b/src/compile/extensions/mod.rs index dc6a988e..19d5d6f3 100644 --- a/src/compile/extensions/mod.rs +++ b/src/compile/extensions/mod.rs @@ -625,22 +625,22 @@ macro_rules! extension_enum { mod ado_aw_marker; pub mod ado_script; -mod exec_context; mod azure_cli; +mod exec_context; mod github; mod safe_outputs; // Re-export tool/runtime extensions from their colocated homes -pub use ado_aw_marker::AdoAwMarkerExtension; -pub use azure_cli::AzureCliExtension; pub use crate::runtimes::dotnet::DotnetExtension; pub use crate::runtimes::lean::LeanExtension; pub use crate::runtimes::node::NodeExtension; pub use crate::runtimes::python::PythonExtension; pub use crate::tools::azure_devops::AzureDevOpsExtension; pub use crate::tools::cache_memory::CacheMemoryExtension; +pub use ado_aw_marker::AdoAwMarkerExtension; pub use ado_script::AdoScriptExtension; -pub use exec_context::{pr_contributor_will_activate, ExecContextExtension}; +pub use azure_cli::AzureCliExtension; +pub use exec_context::{ExecContextExtension, pr_contributor_will_activate}; pub use github::GitHubExtension; pub use safe_outputs::SafeOutputsExtension; @@ -731,10 +731,7 @@ pub fn collect_extensions(front_matter: &FrontMatter) -> Vec { // the block + having no `on.pr` produces zero output. See // `extensions/exec_context/`. Extension::ExecContext(ExecContextExtension::new( - front_matter - .execution_context - .clone() - .unwrap_or_default(), + front_matter.execution_context.clone().unwrap_or_default(), front_matter, )), // Always-on Azure CLI. Tool phase — mounts host /opt/az and diff --git a/src/compile/filter_ir.rs b/src/compile/filter_ir.rs index 40019655..73f38930 100644 --- a/src/compile/filter_ir.rs +++ b/src/compile/filter_ir.rs @@ -1232,9 +1232,7 @@ const PR_SYNTH_SPEC_MAX_BYTES: usize = 8 * 1024; /// The returned string is safe to embed inside a YAML double-quoted /// scalar (the base64 alphabet contains no characters that require /// YAML escaping). -pub fn build_pr_synth_spec( - pr: &crate::compile::types::PrTriggerConfig, -) -> anyhow::Result { +pub fn build_pr_synth_spec(pr: &crate::compile::types::PrTriggerConfig) -> anyhow::Result { use base64::{Engine as _, engine::general_purpose::STANDARD}; let spec = PrSynthSpec { @@ -1388,7 +1386,10 @@ mod tests { let b64 = build_pr_synth_spec(&pr).expect("synth spec must build"); let decoded = STANDARD.decode(b64.as_bytes()).expect("must decode base64"); let parsed: Value = serde_json::from_slice(&decoded).expect("must be valid JSON"); - assert_eq!(parsed["branches"]["include"], serde_json::json!(["main", "release/*"])); + assert_eq!( + parsed["branches"]["include"], + serde_json::json!(["main", "release/*"]) + ); assert_eq!(parsed["branches"]["exclude"], serde_json::json!(["test/*"])); assert_eq!(parsed["paths"]["include"], serde_json::json!(["src/*"])); assert_eq!(parsed["paths"]["exclude"], serde_json::json!(["docs/*"])); @@ -1414,7 +1415,9 @@ mod tests { // Generate enough branch globs to blow past the 8 KiB cap. let pr = PrTriggerConfig { branches: Some(BranchFilter { - include: (0..1000).map(|i| format!("very/long/branch/glob/pattern/{i}")).collect(), + include: (0..1000) + .map(|i| format!("very/long/branch/glob/pattern/{i}")) + .collect(), exclude: vec![], }), paths: None, @@ -1422,7 +1425,10 @@ mod tests { ..Default::default() }; let err = build_pr_synth_spec(&pr).expect_err("oversize spec must fail"); - assert!(err.to_string().contains("PR_SYNTH_SPEC"), "error must mention spec: {err}"); + assert!( + err.to_string().contains("PR_SYNTH_SPEC"), + "error must mention spec: {err}" + ); } // ─── Fact tests ───────────────────────────────────────────────────── @@ -1754,7 +1760,9 @@ mod tests { let result = compile_gate_step_external( GateContext::PullRequest, &checks, - "/tmp/ado-aw-scripts/ado-script/gate.js", false) + "/tmp/ado-aw-scripts/ado-script/gate.js", + false, + ) .unwrap(); assert!(result.contains("- bash:"), "should be a bash step"); assert!( @@ -1785,7 +1793,9 @@ mod tests { let result = compile_gate_step_external( GateContext::PullRequest, &checks, - "/tmp/ado-aw-scripts/ado-script/gate.js", false) + "/tmp/ado-aw-scripts/ado-script/gate.js", + false, + ) .unwrap(); assert!( result.contains("ADO_BUILD_REASON"), @@ -1811,7 +1821,9 @@ mod tests { let result = compile_gate_step_external( GateContext::PipelineCompletion, &checks, - "/tmp/ado-aw-scripts/ado-script/gate.js", false) + "/tmp/ado-aw-scripts/ado-script/gate.js", + false, + ) .unwrap(); assert!( result.contains("name: pipelineGate"), @@ -1840,7 +1852,9 @@ mod tests { let result = compile_gate_step_external( GateContext::PullRequest, &checks, - "/tmp/ado-aw-scripts/ado-script/gate.js", false) + "/tmp/ado-aw-scripts/ado-script/gate.js", + false, + ) .unwrap(); assert!( result.contains("ADO_REPO_ID"), @@ -1865,17 +1879,19 @@ mod tests { let result = compile_gate_step_external( GateContext::PullRequest, &checks, - "/tmp/ado-aw-scripts/ado-script/gate.js", false) + "/tmp/ado-aw-scripts/ado-script/gate.js", + false, + ) .unwrap(); // Verify tier-1 (pipeline-var only) checks do not export API-related env vars // Look for the env: block exports (YAML format with leading spaces) let lines: Vec<&str> = result.lines().collect(); - let has_repo_id_export = lines.iter().any(|line| { - line.trim_start().starts_with("ADO_REPO_ID:") - }); - let has_pr_id_export = lines.iter().any(|line| { - line.trim_start().starts_with("ADO_PR_ID:") - }); + let has_repo_id_export = lines + .iter() + .any(|line| line.trim_start().starts_with("ADO_REPO_ID:")); + let has_pr_id_export = lines + .iter() + .any(|line| line.trim_start().starts_with("ADO_PR_ID:")); assert!( !has_repo_id_export, "should not export ADO_REPO_ID for title-only (tier-1) check" @@ -1981,16 +1997,34 @@ mod tests { let _step = compile_gate_step_external( GateContext::PullRequest, &checks, - "/tmp/ado-aw-scripts/ado-script/gate.js", false) + "/tmp/ado-aw-scripts/ado-script/gate.js", + false, + ) .unwrap(); // Verify the spec captures all three filters with correct fact dependencies let spec = build_gate_spec(GateContext::PullRequest, &checks).unwrap(); - assert_eq!(spec.checks.len(), 3, "should produce 3 checks from 3 filters"); - assert!(spec.facts.iter().any(|f| f.kind == "pr_title"), "title filter requires pr_title fact"); - assert!(spec.facts.iter().any(|f| f.kind == "pr_is_draft"), "draft filter requires pr_is_draft fact"); - assert!(spec.facts.iter().any(|f| f.kind == "pr_labels"), "labels filter requires pr_labels fact"); - assert!(spec.facts.iter().any(|f| f.kind == "pr_metadata"), "API-derived facts should pull in pr_metadata dependency"); + assert_eq!( + spec.checks.len(), + 3, + "should produce 3 checks from 3 filters" + ); + assert!( + spec.facts.iter().any(|f| f.kind == "pr_title"), + "title filter requires pr_title fact" + ); + assert!( + spec.facts.iter().any(|f| f.kind == "pr_is_draft"), + "draft filter requires pr_is_draft fact" + ); + assert!( + spec.facts.iter().any(|f| f.kind == "pr_labels"), + "labels filter requires pr_labels fact" + ); + assert!( + spec.facts.iter().any(|f| f.kind == "pr_metadata"), + "API-derived facts should pull in pr_metadata dependency" + ); } // ─── Schema tests ────────────────────────────────────────────────── @@ -2042,8 +2076,7 @@ mod tests { .join("ado-script") .join("schema") .join("gate-spec.schema.json"); - std::fs::create_dir_all(schema_path.parent().unwrap()) - .expect("should create schema dir"); + std::fs::create_dir_all(schema_path.parent().unwrap()).expect("should create schema dir"); std::fs::write(&schema_path, &schema).expect("should write schema file"); // Verify it's readable and valid diff --git a/src/compile/pr_filters.rs b/src/compile/pr_filters.rs index 963b2ff8..97a517ac 100644 --- a/src/compile/pr_filters.rs +++ b/src/compile/pr_filters.rs @@ -96,12 +96,13 @@ pub(super) fn add_condition_to_steps( // ─── Helpers ──────────────────────────────────────────────────────────────── - // ─── Tests ────────────────────────────────────────────────────────────────── #[cfg(test)] mod tests { - use crate::compile::common::{generate_agentic_depends_on, generate_pr_trigger, generate_setup_job}; + use crate::compile::common::{ + generate_agentic_depends_on, generate_pr_trigger, generate_setup_job, + }; use crate::compile::extensions::CompileContext; use crate::compile::types::*; @@ -118,15 +119,21 @@ mod tests { let triggers = Some(OnConfig { pipeline: None, pr: Some(PrTriggerConfig::default()), - schedule: None, + schedule: None, }); let result = generate_pr_trigger(&triggers, true); // PrTriggerConfig::default() has no branches/paths, so the native block is empty // (meaning "trigger on all PRs" in ADO). The schedule/pipeline suppression ("pr: none") // must NOT be emitted because the explicit pr: key overrides it — regardless of whether // has_schedule or has_pipeline_trigger is set. - assert!(result.is_empty(), "default PrTriggerConfig should produce empty string (trigger on all PRs)"); - assert!(!result.contains("pr: none"), "triggers.pr should override schedule/pipeline suppression"); + assert!( + result.is_empty(), + "default PrTriggerConfig should produce empty string (trigger on all PRs)" + ); + assert!( + !result.contains("pr: none"), + "triggers.pr should override schedule/pipeline suppression" + ); } #[test] @@ -142,13 +149,16 @@ mod tests { filters: None, ..Default::default() }), - schedule: None, + schedule: None, }); let result = generate_pr_trigger(&triggers, false); assert!(result.contains("pr:"), "should emit pr: block"); assert!(result.contains("branches:"), "should include branches"); assert!(result.contains("main"), "should include main branch"); - assert!(result.contains("release/*"), "should include release/* branch"); + assert!( + result.contains("release/*"), + "should include release/* branch" + ); assert!(result.contains("exclude:"), "should include exclude"); assert!(result.contains("test/*"), "should include test/* exclusion"); } @@ -166,7 +176,7 @@ mod tests { filters: None, ..Default::default() }), - schedule: None, + schedule: None, }); let result = generate_pr_trigger(&triggers, false); assert!(result.contains("pr:"), "should emit pr: block"); @@ -183,19 +193,24 @@ mod tests { branches: None, paths: None, filters: Some(PrFilters { - title: Some(PatternFilter { pattern: "*[agent]*".into() }), + title: Some(PatternFilter { + pattern: "*[agent]*".into(), + }), ..Default::default() }), ..Default::default() }), - schedule: None, + schedule: None, }); let result = generate_pr_trigger(&triggers, false); // When only runtime filters are configured (no branches/paths), no native // pr: block is emitted. ADO interprets this as "trigger on all PRs" — the // runtime gate step handles the actual filtering. Do NOT change this to // emit "pr: none" or the gate will never run. - assert!(result.is_empty(), "filters-only should not emit a pr: block (use default trigger)"); + assert!( + result.is_empty(), + "filters-only should not emit a pr: block (use default trigger)" + ); } // Gate step tests now use the spec/extension directly since generate_setup_job @@ -208,27 +223,39 @@ mod tests { let fm = test_fm(); let ctx = make_ctx(&fm); let filters = PrFilters { - title: Some(PatternFilter { pattern: "*[review]*".into() }), + title: Some(PatternFilter { + pattern: "*[review]*".into(), + }), ..Default::default() }; let result = generate_setup_job(&[], "MyPool", Some(&filters), None, &[], &ctx).unwrap(); // No extension → no gate step → setup job has no steps → empty - assert!(result.is_empty(), "filters without extension should produce empty setup job"); + assert!( + result.is_empty(), + "filters without extension should produce empty setup job" + ); } #[test] fn test_generate_setup_job_with_user_steps_and_filters() { let fm = test_fm(); let ctx = make_ctx(&fm); - let step: serde_yaml::Value = serde_yaml::from_str("bash: echo hello\ndisplayName: User step").unwrap(); + let step: serde_yaml::Value = + serde_yaml::from_str("bash: echo hello\ndisplayName: User step").unwrap(); let filters = PrFilters { - title: Some(PatternFilter { pattern: "test".into() }), + title: Some(PatternFilter { + pattern: "test".into(), + }), ..Default::default() }; - let result = generate_setup_job(&[step], "MyPool", Some(&filters), None, &[], &ctx).unwrap(); + let result = + generate_setup_job(&[step], "MyPool", Some(&filters), None, &[], &ctx).unwrap(); // User steps are conditioned on gate output even without extension assert!(result.contains("User step"), "should include user step"); - assert!(result.contains("prGate.SHOULD_RUN"), "user steps should reference gate output"); + assert!( + result.contains("prGate.SHOULD_RUN"), + "user steps should reference gate output" + ); } #[test] @@ -236,16 +263,25 @@ mod tests { let fm = test_fm(); let ctx = make_ctx(&fm); let result = generate_setup_job(&[], "MyPool", None, None, &[], &ctx).unwrap(); - assert!(result.is_empty(), "no setup steps and no filters should produce empty string"); + assert!( + result.is_empty(), + "no setup steps and no filters should produce empty string" + ); } #[test] fn test_generate_agentic_depends_on_with_pr_filters() { let result = generate_agentic_depends_on(&[], true, false, &[], false, false); - assert!(result.contains("dependsOn: Setup"), "should depend on Setup"); + assert!( + result.contains("dependsOn: Setup"), + "should depend on Setup" + ); assert!(result.contains("condition:"), "should have condition"); assert!(result.contains("Build.Reason"), "should check Build.Reason"); - assert!(result.contains("prGate.SHOULD_RUN"), "should check gate output"); + assert!( + result.contains("prGate.SHOULD_RUN"), + "should check gate output" + ); } #[test] @@ -253,7 +289,10 @@ mod tests { let step: serde_yaml::Value = serde_yaml::from_str("bash: echo hello").unwrap(); let result = generate_agentic_depends_on(&[step], false, false, &[], false, false); assert_eq!(result, "dependsOn: Setup"); - assert!(!result.contains("condition:"), "no condition without PR filters"); + assert!( + !result.contains("condition:"), + "no condition without PR filters" + ); } #[test] @@ -265,14 +304,18 @@ mod tests { #[test] fn test_generate_setup_job_gate_spec_via_extension() { // Filter content is now tested via build_gate_spec, not generate_setup_job - use crate::compile::filter_ir::{build_gate_spec, lower_pr_filters, GateContext}; + use crate::compile::filter_ir::{GateContext, build_gate_spec, lower_pr_filters}; let filters = PrFilters { author: Some(IncludeExcludeFilter { include: vec!["alice@corp.com".into()], exclude: vec!["bot@noreply.com".into()], }), - source_branch: Some(PatternFilter { pattern: "feature/*".into() }), - target_branch: Some(PatternFilter { pattern: "main".into() }), + source_branch: Some(PatternFilter { + pattern: "feature/*".into(), + }), + target_branch: Some(PatternFilter { + pattern: "main".into(), + }), ..Default::default() }; let checks = lower_pr_filters(&filters); @@ -286,9 +329,11 @@ mod tests { #[test] fn test_generate_setup_job_gate_non_pr_bypass_in_spec() { - use crate::compile::filter_ir::{build_gate_spec, lower_pr_filters, GateContext}; + use crate::compile::filter_ir::{GateContext, build_gate_spec, lower_pr_filters}; let filters = PrFilters { - title: Some(PatternFilter { pattern: "test".into() }), + title: Some(PatternFilter { + pattern: "test".into(), + }), ..Default::default() }; let checks = lower_pr_filters(&filters); @@ -300,21 +345,22 @@ mod tests { #[test] fn test_generate_setup_job_gate_build_tags() { let filters = PrFilters { - title: Some(PatternFilter { pattern: "test".into() }), + title: Some(PatternFilter { + pattern: "test".into(), + }), ..Default::default() }; // Build tags are now in the evaluator, driven by spec. Verify spec content. - use crate::compile::filter_ir::{build_gate_spec, lower_pr_filters, GateContext}; + use crate::compile::filter_ir::{GateContext, build_gate_spec, lower_pr_filters}; let checks = lower_pr_filters(&filters); let spec = build_gate_spec(GateContext::PullRequest, &checks).unwrap(); assert_eq!(spec.context.tag_prefix, "pr-gate"); assert_eq!(spec.checks[0].tag_suffix, "title-mismatch"); } - #[test] fn test_gate_step_includes_api_facts_for_tier2() { - use crate::compile::filter_ir::{build_gate_spec, lower_pr_filters, GateContext}; + use crate::compile::filter_ir::{GateContext, build_gate_spec, lower_pr_filters}; let filters = PrFilters { labels: Some(LabelFilter { any_of: vec!["run-agent".into()], @@ -324,26 +370,42 @@ mod tests { }; let checks = lower_pr_filters(&filters); let spec = build_gate_spec(GateContext::PullRequest, &checks).unwrap(); - assert!(spec.facts.iter().any(|f| f.kind == "pr_metadata"), "should require pr_metadata fact"); - assert!(spec.facts.iter().any(|f| f.kind == "pr_labels"), "should require pr_labels fact"); + assert!( + spec.facts.iter().any(|f| f.kind == "pr_metadata"), + "should require pr_metadata fact" + ); + assert!( + spec.facts.iter().any(|f| f.kind == "pr_labels"), + "should require pr_labels fact" + ); } #[test] fn test_gate_step_no_api_facts_for_tier1_only() { - use crate::compile::filter_ir::{build_gate_spec, lower_pr_filters, GateContext}; + use crate::compile::filter_ir::{GateContext, build_gate_spec, lower_pr_filters}; let filters = PrFilters { - title: Some(PatternFilter { pattern: "test".into() }), + title: Some(PatternFilter { + pattern: "test".into(), + }), ..Default::default() }; let checks = lower_pr_filters(&filters); let spec = build_gate_spec(GateContext::PullRequest, &checks).unwrap(); - assert!(spec.facts.iter().any(|f| f.kind == "pr_title"), "should require pr_title fact for title filter"); - assert!(!spec.facts.iter().any(|f| f.kind == "pr_metadata"), "should not require pr_metadata for title-only"); + assert!( + spec.facts.iter().any(|f| f.kind == "pr_title"), + "should require pr_title fact for title filter" + ); + assert!( + !spec.facts.iter().any(|f| f.kind == "pr_metadata"), + "should not require pr_metadata for title-only" + ); } #[test] fn test_gate_step_labels_any_of() { - use crate::compile::filter_ir::{build_gate_spec, lower_pr_filters, GateContext, PredicateSpec}; + use crate::compile::filter_ir::{ + GateContext, PredicateSpec, build_gate_spec, lower_pr_filters, + }; let filters = PrFilters { labels: Some(LabelFilter { any_of: vec!["run-agent".into(), "needs-review".into()], @@ -366,7 +428,9 @@ mod tests { #[test] fn test_gate_step_labels_none_of() { - use crate::compile::filter_ir::{build_gate_spec, lower_pr_filters, GateContext, PredicateSpec}; + use crate::compile::filter_ir::{ + GateContext, PredicateSpec, build_gate_spec, lower_pr_filters, + }; let filters = PrFilters { labels: Some(LabelFilter { none_of: vec!["do-not-run".into()], @@ -386,7 +450,9 @@ mod tests { #[test] fn test_gate_step_draft_false() { - use crate::compile::filter_ir::{build_gate_spec, lower_pr_filters, GateContext, PredicateSpec}; + use crate::compile::filter_ir::{ + GateContext, PredicateSpec, build_gate_spec, lower_pr_filters, + }; let filters = PrFilters { draft: Some(false), ..Default::default() @@ -400,13 +466,21 @@ mod tests { } other => panic!("expected Equals, got {:?}", other), } - assert!(spec.facts.iter().any(|f| f.kind == "pr_is_draft"), "should include pr_is_draft fact"); - assert!(spec.facts.iter().any(|f| f.kind == "pr_metadata"), "should include pr_metadata dependency"); + assert!( + spec.facts.iter().any(|f| f.kind == "pr_is_draft"), + "should include pr_is_draft fact" + ); + assert!( + spec.facts.iter().any(|f| f.kind == "pr_metadata"), + "should include pr_metadata dependency" + ); } #[test] fn test_gate_step_changed_files() { - use crate::compile::filter_ir::{build_gate_spec, lower_pr_filters, GateContext, PredicateSpec}; + use crate::compile::filter_ir::{ + GateContext, PredicateSpec, build_gate_spec, lower_pr_filters, + }; let filters = PrFilters { changed_files: Some(IncludeExcludeFilter { include: vec!["src/**/*.rs".into()], @@ -417,20 +491,27 @@ mod tests { let checks = lower_pr_filters(&filters); let spec = build_gate_spec(GateContext::PullRequest, &checks).unwrap(); match &spec.checks[0].predicate { - PredicateSpec::FileGlobMatch { include, exclude, .. } => { + PredicateSpec::FileGlobMatch { + include, exclude, .. + } => { assert!(include.contains(&"src/**/*.rs".to_string())); assert!(exclude.contains(&"docs/**".to_string())); } other => panic!("expected FileGlobMatch, got {:?}", other), } - assert!(spec.facts.iter().any(|f| f.kind == "changed_files"), "should include changed_files fact"); + assert!( + spec.facts.iter().any(|f| f.kind == "changed_files"), + "should include changed_files fact" + ); } #[test] fn test_gate_step_combined_tier1_and_tier2() { - use crate::compile::filter_ir::{build_gate_spec, lower_pr_filters, GateContext}; + use crate::compile::filter_ir::{GateContext, build_gate_spec, lower_pr_filters}; let filters = PrFilters { - title: Some(PatternFilter { pattern: "\\[review\\]".into() }), + title: Some(PatternFilter { + pattern: "\\[review\\]".into(), + }), draft: Some(false), labels: Some(LabelFilter { any_of: vec!["run-agent".into()], @@ -441,20 +522,38 @@ mod tests { let checks = lower_pr_filters(&filters); let spec = build_gate_spec(GateContext::PullRequest, &checks).unwrap(); // Tier 1 fact - assert!(spec.facts.iter().any(|f| f.kind == "pr_title"), "should include pr_title"); + assert!( + spec.facts.iter().any(|f| f.kind == "pr_title"), + "should include pr_title" + ); // Tier 2 facts - assert!(spec.facts.iter().any(|f| f.kind == "pr_metadata"), "should include pr_metadata"); - assert!(spec.facts.iter().any(|f| f.kind == "pr_is_draft"), "should include pr_is_draft"); - assert!(spec.facts.iter().any(|f| f.kind == "pr_labels"), "should include pr_labels"); + assert!( + spec.facts.iter().any(|f| f.kind == "pr_metadata"), + "should include pr_metadata" + ); + assert!( + spec.facts.iter().any(|f| f.kind == "pr_is_draft"), + "should include pr_is_draft" + ); + assert!( + spec.facts.iter().any(|f| f.kind == "pr_labels"), + "should include pr_labels" + ); // Checks - assert_eq!(spec.checks.len(), 3, "should have 3 checks (title, draft, labels)"); + assert_eq!( + spec.checks.len(), + 3, + "should have 3 checks (title, draft, labels)" + ); } // ─── Tier 3 filter tests ──────────────────────────────────────────────── #[test] fn test_gate_step_time_window() { - use crate::compile::filter_ir::{build_gate_spec, lower_pr_filters, GateContext, PredicateSpec}; + use crate::compile::filter_ir::{ + GateContext, PredicateSpec, build_gate_spec, lower_pr_filters, + }; let filters = PrFilters { time_window: Some(super::super::types::TimeWindowFilter { start: "09:00".into(), @@ -476,7 +575,9 @@ mod tests { #[test] fn test_gate_step_min_and_max_changes() { - use crate::compile::filter_ir::{build_gate_spec, lower_pr_filters, GateContext, PredicateSpec}; + use crate::compile::filter_ir::{ + GateContext, PredicateSpec, build_gate_spec, lower_pr_filters, + }; let filters = PrFilters { min_changes: Some(2), max_changes: Some(100), @@ -495,7 +596,9 @@ mod tests { #[test] fn test_gate_step_build_reason_include() { - use crate::compile::filter_ir::{build_gate_spec, lower_pr_filters, GateContext, PredicateSpec}; + use crate::compile::filter_ir::{ + GateContext, PredicateSpec, build_gate_spec, lower_pr_filters, + }; let filters = PrFilters { build_reason: Some(IncludeExcludeFilter { include: vec!["PullRequest".into(), "Manual".into()], @@ -517,7 +620,9 @@ mod tests { #[test] fn test_gate_step_build_reason_exclude() { - use crate::compile::filter_ir::{build_gate_spec, lower_pr_filters, GateContext, PredicateSpec}; + use crate::compile::filter_ir::{ + GateContext, PredicateSpec, build_gate_spec, lower_pr_filters, + }; let filters = PrFilters { build_reason: Some(IncludeExcludeFilter { include: vec![], @@ -547,10 +652,19 @@ mod tests { false, ); // No setup steps, no PR filters → no dependsOn, but the expression produces a condition. - assert!(!result.contains("dependsOn"), "no dependsOn without setup/filters"); + assert!( + !result.contains("dependsOn"), + "no dependsOn without setup/filters" + ); assert!(result.contains("condition:"), "should have condition"); - assert!(result.contains("Custom.ShouldRun"), "should include expression"); - assert!(result.contains("succeeded()"), "should still require succeeded"); + assert!( + result.contains("Custom.ShouldRun"), + "should include expression" + ); + assert!( + result.contains("succeeded()"), + "should still require succeeded" + ); } #[test] @@ -563,14 +677,17 @@ mod tests { false, false, ); - assert!(result.contains("prGate.SHOULD_RUN"), "should check gate output"); + assert!( + result.contains("prGate.SHOULD_RUN"), + "should check gate output" + ); assert!(result.contains("Custom.Flag"), "should include expression"); assert!(result.contains("Build.Reason"), "should check build reason"); } #[test] fn test_gate_step_change_count_includes_changed_files_fact() { - use crate::compile::filter_ir::{build_gate_spec, lower_pr_filters, GateContext}; + use crate::compile::filter_ir::{GateContext, build_gate_spec, lower_pr_filters}; let filters = PrFilters { changed_files: Some(IncludeExcludeFilter { include: vec!["src/**".into()], @@ -608,20 +725,33 @@ triggers: assert_eq!(filters.time_window.as_ref().unwrap().end, "17:00"); assert_eq!(filters.min_changes, Some(5)); assert_eq!(filters.max_changes, Some(100)); - assert_eq!(filters.build_reason.as_ref().unwrap().include, vec!["PullRequest", "Manual"]); - assert_eq!(filters.expression.as_ref().unwrap(), "eq(variables['Custom.Flag'], 'true')"); + assert_eq!( + filters.build_reason.as_ref().unwrap().include, + vec!["PullRequest", "Manual"] + ); + assert_eq!( + filters.expression.as_ref().unwrap(), + "eq(variables['Custom.Flag'], 'true')" + ); } #[test] fn test_gate_step_commit_message() { - use crate::compile::filter_ir::{build_gate_spec, lower_pr_filters, GateContext, PredicateSpec}; + use crate::compile::filter_ir::{ + GateContext, PredicateSpec, build_gate_spec, lower_pr_filters, + }; let filters = PrFilters { - commit_message: Some(PatternFilter { pattern: "*[skip-agent]*".into() }), + commit_message: Some(PatternFilter { + pattern: "*[skip-agent]*".into(), + }), ..Default::default() }; let checks = lower_pr_filters(&filters); let spec = build_gate_spec(GateContext::PullRequest, &checks).unwrap(); - assert!(spec.facts.iter().any(|f| f.kind == "commit_message"), "should include commit_message fact"); + assert!( + spec.facts.iter().any(|f| f.kind == "commit_message"), + "should include commit_message fact" + ); match &spec.checks[0].predicate { PredicateSpec::GlobMatch { fact, pattern } => { assert_eq!(fact, "commit_message"); @@ -644,10 +774,18 @@ on: let val: serde_yaml::Value = serde_yaml::from_str(yaml).unwrap(); let oc: OnConfig = serde_yaml::from_value(val["on"].clone()).unwrap(); assert!(oc.schedule.is_some(), "should have schedule"); - assert_eq!(oc.schedule.unwrap().expression(), "daily around 14:00", "schedule expression should round-trip"); + assert_eq!( + oc.schedule.unwrap().expression(), + "daily around 14:00", + "schedule expression should round-trip" + ); let pr = oc.pr.expect("should have pr"); let filters = pr.filters.expect("pr should have filters"); - assert_eq!(filters.title.unwrap().pattern, "*[review]*", "title pattern should round-trip"); + assert_eq!( + filters.title.unwrap().pattern, + "*[review]*", + "title pattern should round-trip" + ); assert!(oc.pipeline.is_none(), "should not have pipeline"); } @@ -681,4 +819,3 @@ on: assert_eq!(filters.commit_message.unwrap().pattern, "*[skip-agent]*"); } } - diff --git a/src/compile/types.rs b/src/compile/types.rs index 86c150df..39294baf 100644 --- a/src/compile/types.rs +++ b/src/compile/types.rs @@ -1628,8 +1628,6 @@ timeout-minutes: 60 assert_eq!(env.get("AWS_REGION").unwrap(), "us-west-2"); } - - // ─── PermissionsConfig deserialization ─────────────────────────────── #[test] @@ -2321,10 +2319,7 @@ triggers: "#; let val: serde_yaml::Value = serde_yaml::from_str(yaml).unwrap(); let tc: OnConfig = serde_yaml::from_value(val["triggers"].clone()).unwrap(); - assert_eq!( - tc.pipeline.as_ref().unwrap().name, - "Build Pipeline" - ); + assert_eq!(tc.pipeline.as_ref().unwrap().name, "Build Pipeline"); assert_eq!( tc.pr.unwrap().filters.unwrap().title.unwrap().pattern, "*[review]*" diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index 7d1e3bd8..c622e4e4 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -1,7 +1,6 @@ use std::fs; use std::path::PathBuf; - /// Asserts that all required `{{ marker }}` placeholders are present in the template. fn assert_required_markers(content: &str) { let required = [ @@ -158,7 +157,6 @@ fn test_example_file_structure() { ); } - /// Test that validates the presence of required dependencies #[test] fn test_project_dependencies() { @@ -633,8 +631,7 @@ Do something. String::from_utf8_lossy(&output.stderr) ); - let compiled = - fs::read_to_string(&output_path).expect("Compiled YAML should exist on success"); + let compiled = fs::read_to_string(&output_path).expect("Compiled YAML should exist on success"); assert!( compiled.contains("SYSTEM_ACCESSTOKEN: $(System.AccessToken)"), "Executor must map SYSTEM_ACCESSTOKEN from $(System.AccessToken) by default. \ @@ -4661,7 +4658,12 @@ fn test_default_pipeline_mounts_az_and_allows_azure_hosts() { chars after the displayName). Window:\n{window}" ); // Anchor strings: lock the load-bearing parts of the advisory. - for anchor in ["/usr/bin/az", "az devops", "AZURE_DEVOPS_EXT_PAT", "missing-tool"] { + for anchor in [ + "/usr/bin/az", + "az devops", + "AZURE_DEVOPS_EXT_PAT", + "missing-tool", + ] { assert!( compiled.contains(anchor), "compiled YAML must contain advisory anchor `{anchor}`. \ @@ -5154,8 +5156,6 @@ fn test_job_target_with_setup_emits_dual_branch_dependson_with_each() { let _ = fs::remove_dir_all(&temp_dir); } - - // ============================================================================ // Execution-context extension (issue #860) // ============================================================================ @@ -5309,9 +5309,9 @@ fn test_execution_context_pr_does_not_leak_system_accesstoken() { // that contains SYSTEM_ACCESSTOKEN, capture the // sibling `displayName` (if any). if let Some(Value::Mapping(env_map)) = m.get(Value::String("env".to_string())) { - let has_token = env_map.iter().any(|(k, _v)| { - matches!(k, Value::String(s) if s == "SYSTEM_ACCESSTOKEN") - }); + let has_token = env_map + .iter() + .any(|(k, _v)| matches!(k, Value::String(s) if s == "SYSTEM_ACCESSTOKEN")); if has_token { let display = m .get(Value::String("displayName".to_string())) From 127c786f1e6ac5130e37d206e8bc3bd3aa43512b Mon Sep 17 00:00:00 2001 From: James Devine Date: Tue, 9 Jun 2026 10:39:48 +0100 Subject: [PATCH 14/19] fix(compile): drop synthetic-from-ci CI trigger narrowing The narrowed trigger emitted by branch 2 of generate_ci_trigger used pr.branches.include as the trigger include list. Those are PR target branches (e.g. 'main'), but ADO trigger: fires on pushes TO listed branches, so narrowing to [main] suppressed CI on the feature branches synthPr actually needs to react to: a push to feature/x with an open PR feature/x -> main would never queue a build, defeating the entire synthetic-from-ci feature. Remove branch 2 of generate_ci_trigger, the four narrowing-shape unit tests, and the narrowed-trigger assertion in test_synthetic_pr_default_emits_full_synth_wiring. Add a positive 'does not narrow' unit test, a negative integration assertion, and keep the pipeline-trigger priority test. Update docs/front-matter.md and prompts/create-ado-agentic-workflow.md to explain why narrowing is intentionally absent. Cost concern from the original commit (a217df18) is addressed by the synthPr Setup step's existing fast-exit: a single listActivePullRequestsBySourceRef call returns [] for branches without a matching PR and the Agent job self-skips via AW_SYNTHETIC_PR_SKIP. Addresses Rust PR Reviewer feedback on #922. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/front-matter.md | 28 +++--- prompts/create-ado-agentic-workflow.md | 2 +- src/compile/common.rs | 128 ++++++++----------------- tests/compiler_tests.rs | 14 ++- 4 files changed, 70 insertions(+), 102 deletions(-) diff --git a/docs/front-matter.md b/docs/front-matter.md index 2462545b..6046218f 100644 --- a/docs/front-matter.md +++ b/docs/front-matter.md @@ -104,11 +104,11 @@ on: # trigger configuration (unified under on: key) include: [main] paths: include: [src/*] - synthetic-from-ci: true # default true. When true (and `on.pr.branches.include` - # is non-empty), CI builds on the included branches - # call the ADO REST API at Setup time to find the + synthetic-from-ci: true # default true. When true, every CI build + # calls the ADO REST API at Setup time to find the # open PR for `Build.SourceBranch`. If exactly one - # matches, the build is promoted to behave as + # matches `on.pr.branches`/`on.pr.paths`, the build + # is promoted to behave as # `Build.Reason == PullRequest` — gate evaluator # runs, exec-context-pr stages `aw-context/pr/`, # and the agent runs against the PR diff. No Build @@ -387,13 +387,19 @@ On every CI build with `on.pr.synthetic-from-ci: true` (the default): full PR-spec predicates and `aw-context/pr/{base.sha,head.sha}` is staged for the agent. -### Auto-narrowed CI trigger - -When `synthetic-from-ci` is on AND `on.pr.branches.include` is -non-empty, the compiler also auto-emits a top-level `trigger:` block -mirroring those branches so unrelated branches do not queue builds -that would only self-skip. Pipeline/schedule triggers and an explicit -`synthetic-from-ci: false` both suppress this narrowing. +### Why the CI trigger is not auto-narrowed + +`pr.branches.include` lists PR **target** branches (e.g. `main`), but +ADO `trigger:` fires on pushes **to** the listed branches. Narrowing +`trigger:` to `pr.branches.include` would suppress 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). The +compiler therefore leaves the top-level `trigger:` at the ADO default +("trigger on every branch") when synth is on, and relies on the +synthPr Setup step's fast-exit for cost control: a single +`listActivePullRequestsBySourceRef` call returns `[]` on branches +without a matching PR and the Agent job self-skips cleanly via +`AW_SYNTHETIC_PR_SKIP=true`. ### Opting out diff --git a/prompts/create-ado-agentic-workflow.md b/prompts/create-ado-agentic-workflow.md index 101936ad..3a0b0181 100644 --- a/prompts/create-ado-agentic-workflow.md +++ b/prompts/create-ado-agentic-workflow.md @@ -429,7 +429,7 @@ on: When `on.pr` is set: the native ADO `pr:` trigger block is generated from `branches:` and `paths:`. Runtime `filters:` compile to a gate step in the Setup job that self-cancels the build when they do not match. -**`on.pr` triggering works without a Build Validation branch policy.** By default (`synthetic-from-ci: true`), the compiler emits a Setup-job script that, on CI-triggered builds, looks up the open PR for `Build.SourceBranch` via the ADO REST API and promotes the build to PR semantics if exactly one matches `pr.branches` (and `pr.paths` if configured). Zero or multiple matches → the Agent job self-skips cleanly. Set `on.pr.synthetic-from-ci: false` to require an operator-installed branch policy (back-compat). The compiler also auto-emits a narrowed top-level `trigger:` block mirroring `pr.branches.include` so unrelated branches do not queue self-skipping builds. Full reference: ["PR Triggering in Azure Repos" in `docs/front-matter.md`](../docs/front-matter.md#pr-triggering-in-azure-repos). +**`on.pr` triggering works without a Build Validation branch policy.** By default (`synthetic-from-ci: true`), the compiler emits a Setup-job script that, on CI-triggered builds, looks up the open PR for `Build.SourceBranch` via the ADO REST API and promotes the build to PR semantics if exactly one matches `pr.branches` (and `pr.paths` if configured). Zero or multiple matches → the Agent job self-skips cleanly. Set `on.pr.synthetic-from-ci: false` to require an operator-installed branch policy (back-compat). Note that the top-level CI `trigger:` is **not** auto-narrowed to `pr.branches.include`: those are PR target branches, and ADO `trigger:` fires on pushes *to* listed branches, so narrowing would suppress CI on the feature branches synthPr must react to. Full reference: ["PR Triggering in Azure Repos" in `docs/front-matter.md`](../docs/front-matter.md#pr-triggering-in-azure-repos). **PR-reviewer agents — DO NOT write your own precompute step.** When `on.pr` is set, the compiler automatically (1) fetches the PR target branch with progressive deepening, (2) resolves and stages `aw-context/pr/base.sha` + `aw-context/pr/head.sha`, (3) appends a prompt fragment listing common `git diff`/`git show`/`git log` commands and example Azure DevOps MCP tool calls (`repo_get_pull_request_by_id`, `repo_list_pull_request_threads`, `repo_create_pull_request_thread`) with the PR id / project / repo pre-filled, and (4) adds `git`, `git diff`, `git log`, `git show`, `git status`, `git rev-parse`, `git symbolic-ref` to the agent's bash allow-list. The agent runs `git diff $BASE..$HEAD` itself inside the AWF sandbox (objects are already fetched into the workspace). On failure (e.g. merge-base could not be resolved), the failure fragment tells the agent to surface the error rather than produce an empty review. Opt out via `execution-context.pr.enabled: false`. Full reference: [`docs/execution-context.md`](../docs/execution-context.md). diff --git a/src/compile/common.rs b/src/compile/common.rs index 82351bc9..0ee8f3c7 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -566,19 +566,24 @@ pub fn generate_pr_trigger(on_config: &Option, has_schedule: bool) -> /// Generate CI trigger configuration. /// -/// Three branches, in priority order: +/// Two branches, in priority order: /// 1. **Suppression** — when a pipeline-completion trigger or a schedule /// is configured, `trigger: none` is emitted so unrelated commits do /// not also queue a build (existing behaviour). -/// 2. **Synthetic-from-ci narrowing** — when `on.pr.synthetic-from-ci` -/// is on (default) AND `on.pr.branches.include` is non-empty AND -/// branch (1) does not apply, emit a `trigger:` block mirroring -/// `pr.branches.include` so CI fires only on those branches. -/// Without this, ADO's implicit "trigger on every branch" would -/// queue a build for every push, only for the synthPr step to skip -/// most of them — wasted compute. (Issue #916, decision 2.) -/// 3. **Default** — otherwise emit the empty string (ADO's "trigger +/// 2. **Default** — otherwise emit the empty string (ADO's "trigger /// on every branch" default). +/// +/// Note: `on.pr.synthetic-from-ci` (issue #916) must NOT narrow the CI +/// trigger to `pr.branches.include`. Those branches are PR **target** +/// branches (e.g. `main`); ADO `trigger:` fires on pushes **to** the +/// listed branches. Narrowing to target branches suppresses CI on the +/// feature branches the synth flow actually needs to react to — pushing +/// to `feature/x` with an open PR `feature/x → main` would never queue +/// a build. The cost of "wasted" CI builds on branches without a +/// matching PR is bounded: the synthPr Setup step issues one +/// `listActivePullRequestsBySourceRef` call that returns `[]` for those +/// branches and emits `AW_SYNTHETIC_PR_SKIP=true`, which the Agent-job +/// `dependsOn` condition honours to skip cleanly. pub fn generate_ci_trigger(on_config: &Option, has_schedule: bool) -> String { let has_pipeline_trigger = on_config .as_ref() @@ -589,27 +594,6 @@ pub fn generate_ci_trigger(on_config: &Option, has_schedule: bool) -> return "trigger: none".to_string(); } - // Branch 2 — narrowed trigger emitted by the synthetic-from-ci feature. - if let Some(pr) = on_config.as_ref().and_then(|o| o.pr.as_ref()) - && pr.synthetic_from_ci - && let Some(branches) = pr.branches.as_ref() - && !branches.include.is_empty() - { - let mut yaml = String::from("trigger:\n branches:\n include:\n"); - for b in &branches.include { - yaml.push_str(&format!(" - '{}'\n", b.replace('\'', "''"))); - } - // Mirror exclude if the agent declared one — keeps CI exactly - // aligned with the PR-trigger configuration. - if !branches.exclude.is_empty() { - yaml.push_str(" exclude:\n"); - for b in &branches.exclude { - yaml.push_str(&format!(" - '{}'\n", b.replace('\'', "''"))); - } - } - return yaml.trim_end().to_string(); - } - String::new() } @@ -4823,98 +4807,70 @@ mod tests { assert_eq!(result, "trigger: none"); } - // ─── generate_ci_trigger: synthetic-from-ci narrowing (issue #916) ──────── - - #[test] - fn test_generate_ci_trigger_synthetic_pr_narrows_to_include_branches() { + // ─── generate_ci_trigger: synthetic-from-ci must NOT narrow (issue #916) ── + // + // Earlier iterations of the synth feature auto-narrowed the top-level + // `trigger:` block to `pr.branches.include`. That broke the feature: + // ADO `trigger:` fires on pushes *to* the listed branches, but + // `pr.branches.include` lists PR **target** branches. Narrowing to + // `[main]` suppressed CI on feature branches — which are exactly the + // pushes the synth flow needs to react to. The tests below pin the + // current behaviour: synth-active configs leave the CI trigger at the + // ADO default (empty string → "trigger on every branch") and rely on + // the synthPr Setup step's fast-exit for cost control. + + #[test] + fn test_generate_ci_trigger_synthetic_pr_does_not_narrow() { let triggers = Some(crate::compile::types::OnConfig { pipeline: None, pr: Some(crate::compile::types::PrTriggerConfig { branches: Some(crate::compile::types::BranchFilter { include: vec!["main".into(), "release/*".into()], - exclude: vec![], - }), - paths: None, - filters: None, - ..Default::default() // synthetic_from_ci defaults to true - }), - schedule: None, - }); - let result = generate_ci_trigger(&triggers, false); - assert!(result.starts_with("trigger:\n branches:\n include:")); - assert!(result.contains("- 'main'")); - assert!(result.contains("- 'release/*'")); - assert!(!result.contains("exclude:"), "no exclude was configured"); - } - - #[test] - fn test_generate_ci_trigger_synthetic_pr_mirrors_exclude() { - let triggers = Some(crate::compile::types::OnConfig { - pipeline: None, - pr: Some(crate::compile::types::PrTriggerConfig { - branches: Some(crate::compile::types::BranchFilter { - include: vec!["main".into()], exclude: vec!["users/*".into()], }), paths: None, filters: None, - ..Default::default() - }), - schedule: None, - }); - let result = generate_ci_trigger(&triggers, false); - assert!(result.contains("include:")); - assert!(result.contains("- 'main'")); - assert!(result.contains("exclude:")); - assert!(result.contains("- 'users/*'")); - } - - #[test] - fn test_generate_ci_trigger_synthetic_pr_off_returns_default_empty() { - let triggers = Some(crate::compile::types::OnConfig { - pipeline: None, - pr: Some(crate::compile::types::PrTriggerConfig { - branches: Some(crate::compile::types::BranchFilter { - include: vec!["main".into()], - exclude: vec![], - }), - paths: None, - filters: None, - synthetic_from_ci: false, + ..Default::default() // synthetic_from_ci defaults to true }), schedule: None, }); let result = generate_ci_trigger(&triggers, false); assert!( result.is_empty(), - "synthetic-from-ci: false must NOT auto-narrow the CI trigger (back-compat): {result}" + "synthetic-from-ci must NOT narrow the CI trigger — pr.branches.include lists \ + PR TARGET branches, but ADO trigger: fires on pushes TO listed branches, so \ + narrowing would suppress CI on the feature branches synthPr needs. Got: {result}" ); } #[test] - fn test_generate_ci_trigger_synthetic_pr_empty_include_returns_default_empty() { + fn test_generate_ci_trigger_synthetic_pr_off_returns_default_empty() { let triggers = Some(crate::compile::types::OnConfig { pipeline: None, pr: Some(crate::compile::types::PrTriggerConfig { branches: Some(crate::compile::types::BranchFilter { - include: vec![], + include: vec!["main".into()], exclude: vec![], }), paths: None, filters: None, - ..Default::default() + synthetic_from_ci: false, }), schedule: None, }); let result = generate_ci_trigger(&triggers, false); assert!( result.is_empty(), - "empty include list means no narrowing — keep ADO default trigger: {result}" + "synthetic-from-ci: false must leave the CI trigger at ADO default: {result}" ); } #[test] - fn test_generate_ci_trigger_pipeline_trigger_suppresses_synthetic_narrowing() { + fn test_generate_ci_trigger_pipeline_trigger_still_suppresses() { + // Pipeline-completion trigger continues to emit `trigger: none` + // regardless of any `on.pr` configuration; this branch was never + // about synth narrowing — it's the long-standing rule that + // upstream-pipeline triggers exclude commit-driven CI. let triggers = Some(crate::compile::types::OnConfig { pipeline: Some(crate::compile::types::PipelineTrigger { name: "Build".into(), @@ -4936,7 +4892,7 @@ mod tests { let result = generate_ci_trigger(&triggers, false); assert_eq!( result, "trigger: none", - "pipeline trigger must take priority over synth narrowing" + "pipeline-completion trigger must continue to emit `trigger: none`" ); } diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index c622e4e4..0704528b 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -5650,7 +5650,9 @@ Body. /// Fixture A: agent with on.pr and default synthetic-from-ci: true. /// Compiled YAML must contain the full synth wiring (synthPr Setup step, /// PR_SYNTH_SPEC env, broadened exec-context-pr.js condition, agent-job -/// AW_SYNTHETIC_PR_SKIP guard) plus the narrowed top-level `trigger:` block. +/// AW_SYNTHETIC_PR_SKIP guard). The CI trigger must NOT be auto-narrowed +/// to `pr.branches.include` — those are PR target branches, and narrowing +/// would suppress CI on the feature branches synthPr actually needs. #[test] fn test_synthetic_pr_default_emits_full_synth_wiring() { let compiled = compile_fixture_with_flags("synthetic-pr-default.md", &["--skip-integrity"]); @@ -5688,10 +5690,14 @@ fn test_synthetic_pr_default_emits_full_synth_wiring() { "Fixture A's Agent-job condition must accept synth promotion as an activation reason" ); - // Narrowed CI trigger block. + // No auto-narrowed CI trigger — `pr.branches.include` lists PR TARGET + // branches, and ADO `trigger:` fires on pushes TO listed branches, so + // narrowing would suppress CI on the feature branches synthPr needs. assert!( - compiled.contains("trigger:\n branches:\n include:\n - 'main'"), - "Fixture A must auto-emit a narrowed CI trigger mirroring on.pr.branches.include" + !compiled.contains("trigger:\n branches:\n include:\n - 'main'"), + "Fixture A must NOT auto-narrow the top-level CI trigger to pr.branches.include — \ + narrowing to PR target branches would defeat synthPr by suppressing CI on the \ + feature branches it must react to" ); } From ae6e29a34f0f1cdbb6de59594389831bc4995f0d Mon Sep 17 00:00:00 2001 From: James Devine Date: Tue, 9 Jun 2026 11:08:50 +0100 Subject: [PATCH 15/19] feat(compile)!: replace synthetic-from-ci bool with on.pr.mode enum MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the unshipped on.pr.synthetic-from-ci: true|false boolean with a two-value enum on.pr.mode: synthetic|policy (default synthetic). The two modes give the agent author a single coherent choice between the no-policy-required path and the operator-installed-branch-policy path. Mode semantics: * synthetic (default) — emit synthPr Setup-job step + downstream env coalescing + broadened conditions. CI trigger left at ADO default (all branches). Synth promotes CI builds with a matching open PR; non-matching CI builds self-skip cleanly via AW_SYNTHETIC_PR_SKIP. No Build Validation branch policy required. * policy — omit all synth wiring AND emit rigger: none. Branch-policy-driven PR builds are the sole source of pipeline runs; feature-branch pushes no longer queue duplicate CI builds. Choose this when an operator has explicitly installed a Build Validation branch policy. Previously, synthetic-from-ci: false omitted the synth wiring but did NOT suppress the CI trigger, so feature-branch pushes still queued CI builds that immediately bypassed the gate as 'not a PR build'. The new mode: policy closes that gap by emitting rigger: none, so every PR update fires exactly one PR-typed build. Implementation: * New PrMode { Synthetic, Policy } enum (Default = Synthetic) in src/compile/types.rs, replacing PrTriggerConfig.synthetic_from_ci: bool. PrTriggerConfig now derives Default. * generate_ci_trigger gains a mode: policy → trigger: none branch after the existing pipeline/schedule suppression branch. * All internal call sites (extensions/mod.rs, extensions/exec_context/mod.rs, common.rs::generate_agentic_depends_on derivation) replace p.synthetic_from_ci with matches!(p.mode, PrMode::Synthetic). The internal synthetic_pr_active: bool flag is preserved — it remains the right semantic abstraction. * Fixtures: synthetic-pr-opt-out.md renamed to pr-mode-policy.md with mode: policy; synthetic-pr-default.md description cleaned (no longer references the removed narrowing). The policy fixture's integration test now asserts rigger: none is emitted. * Unit tests rewritten around the two-mode contract: synth mode keeps the ADO default, policy mode emits trigger:none, pipeline-completion trigger still wins on priority. Schema tests cover all four cases (omitted, synthetic, policy, invalid value). * Docs (front-matter.md), and prompts (create/update/debug) rewritten to present the two modes as a table and explain the policy mode's rigger: none emission. No back-compat alias for synthetic-from-ci since the knob never shipped (still on the feature branch). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/front-matter.md | 73 +++++++------ prompts/create-ado-agentic-workflow.md | 2 +- prompts/debug-ado-agentic-workflow.md | 2 +- prompts/update-ado-agentic-workflow.md | 13 ++- src/compile/common.rs | 107 ++++++++++--------- src/compile/extensions/ado_script.rs | 2 +- src/compile/extensions/exec_context/mod.rs | 8 +- src/compile/extensions/exec_context/pr.rs | 4 +- src/compile/extensions/mod.rs | 5 +- src/compile/filter_ir.rs | 2 +- src/compile/types.rs | 113 ++++++++++++++------- tests/compiler_tests.rs | 38 ++++--- tests/fixtures/pr-mode-policy.md | 21 ++++ tests/fixtures/synthetic-pr-default.md | 6 +- tests/fixtures/synthetic-pr-opt-out.md | 19 ---- 15 files changed, 245 insertions(+), 170 deletions(-) create mode 100644 tests/fixtures/pr-mode-policy.md delete mode 100644 tests/fixtures/synthetic-pr-opt-out.md diff --git a/docs/front-matter.md b/docs/front-matter.md index 6046218f..8b9e9ae7 100644 --- a/docs/front-matter.md +++ b/docs/front-matter.md @@ -104,21 +104,24 @@ on: # trigger configuration (unified under on: key) include: [main] paths: include: [src/*] - synthetic-from-ci: true # default true. When true, every CI build - # calls the ADO REST API at Setup time to find the - # open PR for `Build.SourceBranch`. If exactly one - # matches `on.pr.branches`/`on.pr.paths`, the build - # is promoted to behave as - # `Build.Reason == PullRequest` — gate evaluator - # runs, exec-context-pr stages `aw-context/pr/`, - # and the agent runs against the PR diff. No Build - # Validation branch policy required. Zero or - # multiple matches → the Agent job self-skips - # cleanly. Also auto-emits a top-level `trigger:` - # block mirroring `pr.branches.include` so CI fires - # only on those branches. Set to `false` to opt out - # (requires an operator-installed branch policy). - # See "PR triggering in Azure Repos" callout below. + mode: synthetic # synthetic (default) | policy. Controls how + # `on.pr` builds reach the pipeline. + # - synthetic: a Setup-job script calls the + # ADO REST API on every CI build, finds the + # open PR for `Build.SourceBranch`, and + # promotes the build to PR semantics if it + # matches `branches`/`paths`. No Build + # Validation branch policy required. Zero + # or multiple matches → Agent job + # self-skips cleanly. CI trigger stays at + # the ADO default (all branches). + # - policy: the operator has installed a + # Build Validation branch policy. Compiler + # omits all synth wiring AND emits + # `trigger: none` so feature-branch pushes + # do not queue duplicate CI builds. Real + # PR-typed builds drive everything. + # See "PR Triggering in Azure Repos" below. filters: # runtime PR filters (compiled to gate step) title: "*[review]*" author: @@ -353,15 +356,17 @@ policy, a `git push` to a feature branch fires the compiled pipeline as evaluator's "not a PR build" bypass triggers and `exec-context-pr.js` is skipped. PR-aware agents (e.g. PR reviewers) silently degrade. -`ado-aw` works around this with the always-on `on.pr.synthetic-from-ci` -feature: when `on.pr` is configured, a Setup-job script -(`exec-context-pr-synth.js`) calls the ADO REST API at build time to -find the active PR for `Build.SourceBranch` and promotes the build to -PR semantics if a single match is found. **No branch policy required.** +`ado-aw` lets the agent author pick one of two coherent strategies via +`on.pr.mode`: -### How it works under the hood +| `on.pr.mode` | Synthesis wiring | Top-level `trigger:` | Use when | +|---|---|---|---| +| `synthetic` (default) | emitted (synthPr Setup step, coalesced env, broadened conditions) | ADO default (all branches) | No branch policy. **The vast majority of agents.** | +| `policy` | omitted | `trigger: none` | Operator has installed a Build Validation branch policy and wants real PR-typed builds only, no duplicate CI builds. | -On every CI build with `on.pr.synthetic-from-ci: true` (the default): +### `mode: synthetic` — how it works under the hood + +On every CI build: 1. **Real PR build?** If `Build.Reason == PullRequest` (a branch policy is configured), the synth step no-ops and the existing PR path @@ -387,7 +392,7 @@ On every CI build with `on.pr.synthetic-from-ci: true` (the default): full PR-spec predicates and `aw-context/pr/{base.sha,head.sha}` is staged for the agent. -### Why the CI trigger is not auto-narrowed +### Why the CI trigger is not auto-narrowed in `mode: synthetic` `pr.branches.include` lists PR **target** branches (e.g. `main`), but ADO `trigger:` fires on pushes **to** the listed branches. Narrowing @@ -395,16 +400,24 @@ ADO `trigger:` fires on pushes **to** the listed branches. Narrowing branches synthPr actually needs to react to (pushing to `feature/x` with an open PR `feature/x → main` would never queue a build). The compiler therefore leaves the top-level `trigger:` at the ADO default -("trigger on every branch") when synth is on, and relies on the -synthPr Setup step's fast-exit for cost control: a single +("trigger on every branch") in synth mode, and relies on the synthPr +Setup step's fast-exit for cost control: a single `listActivePullRequestsBySourceRef` call returns `[]` on branches without a matching PR and the Agent job self-skips cleanly via `AW_SYNTHETIC_PR_SKIP=true`. -### Opting out +### `mode: policy` — when to choose it + +Choose `mode: policy` when the operator has explicitly installed an +Azure DevOps Build Validation branch policy targeting the compiled +pipeline. In this mode the compiler: + +- Omits all synth wiring (`synthPr` step, `PR_SYNTH_SPEC` env, + `AW_SYNTHETIC_PR_SKIP` guard, coalesced env macros, broadened + `exec-context-pr.js` condition). +- Emits `trigger: none` so feature-branch pushes do not queue + duplicate CI builds alongside the policy-driven PR build. -Set `on.pr.synthetic-from-ci: false` to disable. The compiled YAML -will then contain none of the synthesis wiring (`synthPr`, -`AW_SYNTHETIC_PR`, `PR_SYNTH_SPEC`) and PR triggering will require an -operator-installed branch policy as before. +Result: every PR update fires exactly one PR-typed build (`Build.Reason +== PullRequest`); commit-driven CI is fully silenced. diff --git a/prompts/create-ado-agentic-workflow.md b/prompts/create-ado-agentic-workflow.md index 3a0b0181..ffe780c2 100644 --- a/prompts/create-ado-agentic-workflow.md +++ b/prompts/create-ado-agentic-workflow.md @@ -429,7 +429,7 @@ on: When `on.pr` is set: the native ADO `pr:` trigger block is generated from `branches:` and `paths:`. Runtime `filters:` compile to a gate step in the Setup job that self-cancels the build when they do not match. -**`on.pr` triggering works without a Build Validation branch policy.** By default (`synthetic-from-ci: true`), the compiler emits a Setup-job script that, on CI-triggered builds, looks up the open PR for `Build.SourceBranch` via the ADO REST API and promotes the build to PR semantics if exactly one matches `pr.branches` (and `pr.paths` if configured). Zero or multiple matches → the Agent job self-skips cleanly. Set `on.pr.synthetic-from-ci: false` to require an operator-installed branch policy (back-compat). Note that the top-level CI `trigger:` is **not** auto-narrowed to `pr.branches.include`: those are PR target branches, and ADO `trigger:` fires on pushes *to* listed branches, so narrowing would suppress CI on the feature branches synthPr must react to. Full reference: ["PR Triggering in Azure Repos" in `docs/front-matter.md`](../docs/front-matter.md#pr-triggering-in-azure-repos). +**`on.pr` triggering works without a Build Validation branch policy.** By default (`mode: synthetic`), the compiler emits a Setup-job script that, on CI-triggered builds, looks up the open PR for `Build.SourceBranch` via the ADO REST API and promotes the build to PR semantics if exactly one matches `pr.branches` (and `pr.paths` if configured). Zero or multiple matches → the Agent job self-skips cleanly. Set `on.pr.mode: policy` when an operator-installed Build Validation branch policy is in place — that mode omits all synth wiring AND emits `trigger: none` so feature-branch pushes do not queue duplicate CI builds alongside the policy-driven PR build. Note that in `mode: synthetic` the top-level CI `trigger:` is **not** auto-narrowed to `pr.branches.include`: those are PR target branches, and ADO `trigger:` fires on pushes *to* listed branches, so narrowing would suppress CI on the feature branches synthPr must react to. Full reference: ["PR Triggering in Azure Repos" in `docs/front-matter.md`](../docs/front-matter.md#pr-triggering-in-azure-repos). **PR-reviewer agents — DO NOT write your own precompute step.** When `on.pr` is set, the compiler automatically (1) fetches the PR target branch with progressive deepening, (2) resolves and stages `aw-context/pr/base.sha` + `aw-context/pr/head.sha`, (3) appends a prompt fragment listing common `git diff`/`git show`/`git log` commands and example Azure DevOps MCP tool calls (`repo_get_pull_request_by_id`, `repo_list_pull_request_threads`, `repo_create_pull_request_thread`) with the PR id / project / repo pre-filled, and (4) adds `git`, `git diff`, `git log`, `git show`, `git status`, `git rev-parse`, `git symbolic-ref` to the agent's bash allow-list. The agent runs `git diff $BASE..$HEAD` itself inside the AWF sandbox (objects are already fetched into the workspace). On failure (e.g. merge-base could not be resolved), the failure fragment tells the agent to surface the error rather than produce an empty review. Opt out via `execution-context.pr.enabled: false`. Full reference: [`docs/execution-context.md`](../docs/execution-context.md). diff --git a/prompts/debug-ado-agentic-workflow.md b/prompts/debug-ado-agentic-workflow.md index 80816daf..b538a6ea 100644 --- a/prompts/debug-ado-agentic-workflow.md +++ b/prompts/debug-ado-agentic-workflow.md @@ -32,7 +32,7 @@ Agent → Detection → SafeOutputs | **SafeOutputs** | Executes approved safe outputs (create PRs, work items, wiki pages, etc.) | Write (`permissions.write`) | Standard ADO agent | Additional optional jobs: -- **Setup** — runs before `Agent` (from `setup:` front matter). When `on.pr` is set with default `synthetic-from-ci: true`, this job also runs a `synthPr` step that calls the ADO REST API to promote CI-triggered builds to PR semantics when an open PR matches; if it sets `AW_SYNTHETIC_PR_SKIP=true`, the Agent job is skipped cleanly. See [`docs/front-matter.md#pr-triggering-in-azure-repos`](../docs/front-matter.md#pr-triggering-in-azure-repos). +- **Setup** — runs before `Agent` (from `setup:` front matter). When `on.pr` is set with the default `mode: synthetic`, this job also runs a `synthPr` step that calls the ADO REST API to promote CI-triggered builds to PR semantics when an open PR matches; if it sets `AW_SYNTHETIC_PR_SKIP=true`, the Agent job is skipped cleanly. With `mode: policy` there is no synthPr step (and `trigger: none` is emitted so the operator's Build Validation branch policy is the sole source of PR builds). See [`docs/front-matter.md#pr-triggering-in-azure-repos`](../docs/front-matter.md#pr-triggering-in-azure-repos). - **Teardown** — runs after `SafeOutputs` (from `teardown:` front matter) --- diff --git a/prompts/update-ado-agentic-workflow.md b/prompts/update-ado-agentic-workflow.md index 77803298..93fb0f03 100644 --- a/prompts/update-ado-agentic-workflow.md +++ b/prompts/update-ado-agentic-workflow.md @@ -45,11 +45,14 @@ permissions → parameters ``` > **`on.pr` knob update**: when changing `on.pr.branches` or -> `on.pr.paths`, also confirm whether `synthetic-from-ci` (default -> `true`) is appropriate. With it on, the compiler emits a Setup-job -> ADO REST call to discover the open PR for `Build.SourceBranch` and -> auto-narrows the top-level `trigger:` block to those branches. -> Reference: [`docs/front-matter.md#pr-triggering-in-azure-repos`](../docs/front-matter.md#pr-triggering-in-azure-repos). +> `on.pr.paths`, also confirm whether `mode` (default `synthetic`) is +> appropriate. In `synthetic` mode the compiler emits a Setup-job ADO +> REST call to discover the open PR for `Build.SourceBranch` and +> leaves the top-level `trigger:` at the ADO default. Switch to +> `mode: policy` only if the operator has explicitly installed a +> Build Validation branch policy — that mode emits `trigger: none` +> and drops the synth wiring. Reference: +> [`docs/front-matter.md#pr-triggering-in-azure-repos`](../docs/front-matter.md#pr-triggering-in-azure-repos). ### Step 3 — Validate the Changes diff --git a/src/compile/common.rs b/src/compile/common.rs index 0ee8f3c7..7df08396 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -566,24 +566,27 @@ pub fn generate_pr_trigger(on_config: &Option, has_schedule: bool) -> /// Generate CI trigger configuration. /// -/// Two branches, in priority order: -/// 1. **Suppression** — when a pipeline-completion trigger or a schedule -/// is configured, `trigger: none` is emitted so unrelated commits do -/// not also queue a build (existing behaviour). -/// 2. **Default** — otherwise emit the empty string (ADO's "trigger -/// on every branch" default). +/// Three branches, in priority order: +/// 1. **Suppression by pipeline / schedule** — when a +/// pipeline-completion trigger or a schedule is configured, +/// `trigger: none` is emitted so unrelated commits do not also +/// queue a build (existing behaviour). +/// 2. **`on.pr.mode: policy`** — the operator has installed a Build +/// Validation branch policy and the agent author has declared +/// intent to rely on it. Emit `trigger: none` so feature-branch +/// pushes do not queue duplicate CI builds alongside the real +/// PR-typed build the policy fires. +/// 3. **Default** — otherwise emit the empty string (ADO's "trigger +/// on every branch" default). This is the `on.pr.mode: synthetic` +/// path: the synthPr Setup step will promote CI builds to PR +/// semantics, and the synthPr step's fast-exit (`AW_SYNTHETIC_PR_SKIP`) +/// handles wasted CI builds on branches without a matching PR. /// -/// Note: `on.pr.synthetic-from-ci` (issue #916) must NOT narrow the CI -/// trigger to `pr.branches.include`. Those branches are PR **target** -/// branches (e.g. `main`); ADO `trigger:` fires on pushes **to** the -/// listed branches. Narrowing to target branches suppresses CI on the -/// feature branches the synth flow actually needs to react to — pushing -/// to `feature/x` with an open PR `feature/x → main` would never queue -/// a build. The cost of "wasted" CI builds on branches without a -/// matching PR is bounded: the synthPr Setup step issues one -/// `listActivePullRequestsBySourceRef` call that returns `[]` for those -/// branches and emits `AW_SYNTHETIC_PR_SKIP=true`, which the Agent-job -/// `dependsOn` condition honours to skip cleanly. +/// Note: synth mode must NOT narrow the CI trigger to +/// `pr.branches.include` — those are PR **target** branches, but ADO +/// `trigger:` fires on pushes **to** the listed branches. Narrowing +/// would suppress CI on the feature branches synthPr needs to react +/// to. pub fn generate_ci_trigger(on_config: &Option, has_schedule: bool) -> String { let has_pipeline_trigger = on_config .as_ref() @@ -594,6 +597,16 @@ pub fn generate_ci_trigger(on_config: &Option, has_schedule: bool) -> return "trigger: none".to_string(); } + // Branch 2 — `on.pr.mode: policy`. The operator owns trigger semantics + // via a Build Validation branch policy, so the YAML CI trigger must be + // silenced to avoid duplicate builds. We still emit the `pr:` block + // (the policy uses the YAML `paths:` filter to refine queueing). + if let Some(pr) = on_config.as_ref().and_then(|o| o.pr.as_ref()) + && matches!(pr.mode, crate::compile::types::PrMode::Policy) + { + return "trigger: none".to_string(); + } + String::new() } @@ -2388,8 +2401,8 @@ pub fn generate_agentic_depends_on( // (standalone/1es/stage) path and the dual-branch jobs-template path. let condition_body: Option = if has_internal_condition { let mut parts = vec!["succeeded()".to_string()]; - // Synthetic-from-ci: the synthPr Setup-job step may have decided - // this build should self-skip (no matching PR, wrong target + // `mode: synthetic` (default): the synthPr Setup-job step may have + // decided this build should self-skip (no matching PR, wrong target // branch, no matching changed files). Always honour that flag — // it must trump every other reason to run. if synthetic_pr_active { @@ -2399,13 +2412,13 @@ pub fn generate_agentic_depends_on( ); } if has_pr_filters { - // With synthetic-from-ci, the agent should run when EITHER + // With `mode: synthetic`, the agent should run when EITHER // (a) it is a real PR build (existing path) OR // (b) the synthPr step promoted this CI build to PR semantics // (synthPr.AW_SYNTHETIC_PR=true) OR // (c) the gate passed for any other reason. - // Without synthetic-from-ci, the existing two-arm condition - // is preserved verbatim. + // With `mode: policy`, the existing two-arm condition + // is preserved verbatim (no synth path is active). if synthetic_pr_active { parts.push( r"or( @@ -3351,7 +3364,7 @@ pub async fn compile_shared( let synthetic_pr_active = front_matter .pr_trigger() - .is_some_and(|p| p.synthetic_from_ci); + .is_some_and(|p| matches!(p.mode, crate::compile::types::PrMode::Synthetic)); let agentic_depends_on = generate_agentic_depends_on( &front_matter.setup, has_pr_filters, @@ -4807,20 +4820,16 @@ mod tests { assert_eq!(result, "trigger: none"); } - // ─── generate_ci_trigger: synthetic-from-ci must NOT narrow (issue #916) ── + // ─── generate_ci_trigger: on.pr.mode behaviour (issue #916) ────────────── // - // Earlier iterations of the synth feature auto-narrowed the top-level - // `trigger:` block to `pr.branches.include`. That broke the feature: - // ADO `trigger:` fires on pushes *to* the listed branches, but - // `pr.branches.include` lists PR **target** branches. Narrowing to - // `[main]` suppressed CI on feature branches — which are exactly the - // pushes the synth flow needs to react to. The tests below pin the - // current behaviour: synth-active configs leave the CI trigger at the - // ADO default (empty string → "trigger on every branch") and rely on - // the synthPr Setup step's fast-exit for cost control. - - #[test] - fn test_generate_ci_trigger_synthetic_pr_does_not_narrow() { + // The synth path (default, `mode: synthetic`) leaves the CI trigger at + // ADO default ("trigger on every branch") and relies on the synthPr + // Setup step to promote / skip per build. Policy path (`mode: policy`) + // emits `trigger: none` so the operator-installed Build Validation + // policy is the sole source of pipeline runs — no duplicate builds. + + #[test] + fn test_generate_ci_trigger_pr_mode_synthetic_keeps_default() { let triggers = Some(crate::compile::types::OnConfig { pipeline: None, pr: Some(crate::compile::types::PrTriggerConfig { @@ -4830,21 +4839,22 @@ mod tests { }), paths: None, filters: None, - ..Default::default() // synthetic_from_ci defaults to true + ..Default::default() // mode defaults to Synthetic }), schedule: None, }); let result = generate_ci_trigger(&triggers, false); assert!( result.is_empty(), - "synthetic-from-ci must NOT narrow the CI trigger — pr.branches.include lists \ - PR TARGET branches, but ADO trigger: fires on pushes TO listed branches, so \ - narrowing would suppress CI on the feature branches synthPr needs. Got: {result}" + "mode: synthetic must leave the CI trigger at ADO default — \ + pr.branches.include lists PR TARGET branches, but ADO trigger: \ + fires on pushes TO listed branches, so narrowing would suppress \ + CI on the feature branches synthPr needs. Got: {result}" ); } #[test] - fn test_generate_ci_trigger_synthetic_pr_off_returns_default_empty() { + fn test_generate_ci_trigger_pr_mode_policy_emits_trigger_none() { let triggers = Some(crate::compile::types::OnConfig { pipeline: None, pr: Some(crate::compile::types::PrTriggerConfig { @@ -4854,23 +4864,24 @@ mod tests { }), paths: None, filters: None, - synthetic_from_ci: false, + mode: crate::compile::types::PrMode::Policy, }), schedule: None, }); let result = generate_ci_trigger(&triggers, false); - assert!( - result.is_empty(), - "synthetic-from-ci: false must leave the CI trigger at ADO default: {result}" + assert_eq!( + result, "trigger: none", + "mode: policy must suppress the CI trigger so the operator-installed \ + branch policy is the sole source of pipeline runs (no duplicate builds)" ); } #[test] fn test_generate_ci_trigger_pipeline_trigger_still_suppresses() { // Pipeline-completion trigger continues to emit `trigger: none` - // regardless of any `on.pr` configuration; this branch was never - // about synth narrowing — it's the long-standing rule that - // upstream-pipeline triggers exclude commit-driven CI. + // regardless of any `on.pr` configuration; this branch is the + // long-standing rule that upstream-pipeline triggers exclude + // commit-driven CI. let triggers = Some(crate::compile::types::OnConfig { pipeline: Some(crate::compile::types::PipelineTrigger { name: "Build".into(), diff --git a/src/compile/extensions/ado_script.rs b/src/compile/extensions/ado_script.rs index a311a3f7..5d79d3f6 100644 --- a/src/compile/extensions/ado_script.rs +++ b/src/compile/extensions/ado_script.rs @@ -58,7 +58,7 @@ pub struct AdoScriptExtension { /// lock-step with `ExecContextExtension`'s own activation gate. pub exec_context_pr_active: bool, /// Whether the synthetic-from-ci path is active for this agent. - /// Set when `on.pr.synthetic-from-ci != false`. Drives: + /// Set when `on.pr.mode == Synthetic` (the default). Drives: /// - Setup-job install/download fire (even with no `filters:`). /// - Setup-job `synthPr` step emission (before any gate step). /// - Downstream env coalescing (handled in `compile-coalesce-env`). diff --git a/src/compile/extensions/exec_context/mod.rs b/src/compile/extensions/exec_context/mod.rs index 6ebc0406..e41a118a 100644 --- a/src/compile/extensions/exec_context/mod.rs +++ b/src/compile/extensions/exec_context/mod.rs @@ -101,8 +101,8 @@ pub struct ExecContextExtension { /// means "is `on.pr` configured" — future trigger contributors /// will OR in their own checks here. any_contributor_active: bool, - /// Whether `on.pr.synthetic-from-ci` is on. Passed through to the - /// PR contributor so it can emit coalesced + /// Whether `on.pr.mode == Synthetic` for this agent. Passed through + /// to the PR contributor so it can emit coalesced /// `SYSTEM_PULLREQUEST_*` env vars (real value preferred, synthPr /// Setup-job output as fallback). synthetic_pr_active: bool, @@ -126,7 +126,7 @@ impl ExecContextExtension { let any_contributor_active = pr_contributor_will_activate_with_cfg(&config, front_matter); let synthetic_pr_active = front_matter .pr_trigger() - .is_some_and(|p| p.synthetic_from_ci); + .is_some_and(|p| matches!(p.mode, crate::compile::types::PrMode::Synthetic)); Self { config, any_contributor_active, @@ -142,7 +142,7 @@ impl ExecContextExtension { // "on by default when on.pr is configured" behaviour without // the user having to write `execution-context.pr: {}`. let pr_cfg = self.config.pr.clone().unwrap_or_default(); - // The PR contributor needs to know whether synthetic-from-ci + // The PR contributor needs to know whether `mode: synthetic` // is on so it can emit coalesced SYSTEM_PULLREQUEST_* env vars // (real value preferred, synthPr output as fallback). let synthetic_pr_active = self.synthetic_pr_active; diff --git a/src/compile/extensions/exec_context/pr.rs b/src/compile/extensions/exec_context/pr.rs index 77d5f9e1..a6ca14c5 100644 --- a/src/compile/extensions/exec_context/pr.rs +++ b/src/compile/extensions/exec_context/pr.rs @@ -68,7 +68,7 @@ use super::contributor::ContextContributor; /// (unless explicitly disabled via `execution-context.pr.enabled: false`). pub(super) struct PrContextContributor { config: PrContextConfig, - /// Whether `on.pr.synthetic-from-ci` is on for this agent. Drives + /// Whether `on.pr.mode == Synthetic` for this agent. Drives /// emission of the coalesced `SYSTEM_PULLREQUEST_*` env vars so the /// bundle reads either real PR identifiers (true PR builds) or the /// `synthPr` Setup-job outputs (CI builds promoted via synth). @@ -122,7 +122,7 @@ impl ContextContributor for PrContextContributor { // the spawned `git` subprocess via `GIT_CONFIG_*` env vars // (never argv). It is NEVER visible to the agent step. // - // When `synthetic-from-ci` is on, the PR-identifier env vars + // When `mode: synthetic` is on, the PR-identifier env vars // are emitted using `$[ coalesce(...) ]` so the bundle picks // up either the real `System.PullRequest.*` (on a true PR // build) OR the synthPr Setup-job output (on a CI build diff --git a/src/compile/extensions/mod.rs b/src/compile/extensions/mod.rs index 19d5d6f3..ed38212d 100644 --- a/src/compile/extensions/mod.rs +++ b/src/compile/extensions/mod.rs @@ -699,11 +699,12 @@ pub fn collect_extensions(front_matter: &FrontMatter) -> Vec { Extension::SafeOutputs(SafeOutputsExtension), Extension::AdoScript(Box::new({ // PR trigger config drives both the PR-context contributor - // (exec-context-pr.js) and the new synthetic-from-ci path + // (exec-context-pr.js) and the synthetic-from-ci path // (exec-context-pr-synth.js). Compute the two flags from // the same source so they stay in lock-step. let pr_cfg = front_matter.pr_trigger(); - let synthetic_pr_active = pr_cfg.is_some_and(|p| p.synthetic_from_ci); + let synthetic_pr_active = + pr_cfg.is_some_and(|p| matches!(p.mode, crate::compile::types::PrMode::Synthetic)); AdoScriptExtension { pr_filters: front_matter.pr_filters().cloned(), pipeline_filters: front_matter.pipeline_filters().cloned(), diff --git a/src/compile/filter_ir.rs b/src/compile/filter_ir.rs index 73f38930..bd06ce74 100644 --- a/src/compile/filter_ir.rs +++ b/src/compile/filter_ir.rs @@ -1191,7 +1191,7 @@ pub fn compile_gate_step_external( Ok(step) } -// ─── PR synthetic-from-ci spec ────────────────────────────────────────────── +// ─── PR synthetic-from-ci spec (mode: synthetic) ──────────────────────────── /// Base64-encoded JSON spec consumed by the `exec-context-pr-synth.js` /// bundle at runtime. Carries the PR branch/path filters the agent diff --git a/src/compile/types.rs b/src/compile/types.rs index 39294baf..bd71818c 100644 --- a/src/compile/types.rs +++ b/src/compile/types.rs @@ -1245,7 +1245,7 @@ impl SanitizeConfigTrait for PrContextConfig { // ─── PR Trigger Types ─────────────────────────────────────────────────────── /// PR trigger configuration with native ADO filters and runtime gate filters. -#[derive(Debug, Deserialize, Clone)] +#[derive(Debug, Deserialize, Clone, Default)] pub struct PrTriggerConfig { /// Native ADO branch filter for PR triggers #[serde(default)] @@ -1257,33 +1257,47 @@ pub struct PrTriggerConfig { #[serde(default)] pub filters: Option, /// Whether to synthesise PullRequest semantics on CI builds when an - /// open PR matches the configured branches/paths. Defaults to `true`. - /// - /// Azure DevOps Services ignores the YAML `pr:` block unless a Build - /// Validation branch policy is registered server-side. When this knob - /// is on, an Setup-job script (`exec-context-pr-synth.js`) looks up - /// the open PR via the ADO REST API on CI-triggered builds and - /// exposes PR identifiers as `dependencies.Setup.outputs['synthPr.*']` - /// so downstream gate/exec-context-pr steps behave as if - /// `Build.Reason == PullRequest`. Set to `false` to opt out and - /// require an operator-installed branch policy. - #[serde(default = "pr_synthetic_from_ci_default", rename = "synthetic-from-ci")] - pub synthetic_from_ci: bool, -} - -impl Default for PrTriggerConfig { - fn default() -> Self { - Self { - branches: None, - paths: None, - filters: None, - synthetic_from_ci: pr_synthetic_from_ci_default(), - } - } + /// PR-trigger mode. Drives whether the compiler emits the + /// synthetic-from-ci Setup-job step (`mode: synthetic`, default) or + /// steps back and expects an operator-installed Build Validation + /// branch policy to drive PR builds (`mode: policy`). See + /// [`PrMode`] for the two values. + #[serde(default, rename = "mode")] + pub mode: PrMode, } -fn pr_synthetic_from_ci_default() -> bool { - true +/// How `on.pr` builds reach the pipeline. +/// +/// Azure DevOps Services ignores the YAML `pr:` block unless a +/// per-branch Build Validation policy is registered server-side. This +/// enum lets the agent author pick one of two coherent strategies: +/// +/// * [`PrMode::Synthetic`] (default) — the compiler emits a Setup-job +/// script (`exec-context-pr-synth.js`) that calls the ADO REST API +/// on CI-triggered builds, finds the open PR for `Build.SourceBranch`, +/// and exposes PR identifiers as `dependencies.Setup.outputs['synthPr.*']` +/// so the gate and `exec-context-pr.js` behave as if +/// `Build.Reason == PullRequest`. The top-level `trigger:` stays at +/// ADO's "trigger on every branch" default. **No branch policy +/// required.** This is the right choice for the vast majority of +/// agents. +/// +/// * [`PrMode::Policy`] — the compiler omits all synth wiring AND +/// emits `trigger: none` at the top level, so the pipeline only +/// queues when ADO's Build Validation branch policy fires a real +/// `Build.Reason == PullRequest` build. Choose this when the +/// operator has explicitly installed a branch policy and wants to +/// avoid duplicate CI builds firing on every push. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Deserialize, Serialize)] +#[serde(rename_all = "kebab-case")] +pub enum PrMode { + /// Synthesise PR context from the ADO REST API on CI-triggered + /// builds. Top-level `trigger:` left at ADO default. + #[default] + Synthetic, + /// Operator-installed Build Validation branch policy drives PR + /// builds; CI trigger is suppressed via `trigger: none`. + Policy, } impl SanitizeConfigTrait for PrTriggerConfig { @@ -2246,7 +2260,7 @@ triggers: } #[test] - fn test_pr_trigger_config_synthetic_from_ci_default_true() { + fn test_pr_trigger_config_mode_default_synthetic() { let yaml = r#" triggers: pr: @@ -2255,41 +2269,62 @@ triggers: "#; let val: serde_yaml::Value = serde_yaml::from_str(yaml).unwrap(); let tc: OnConfig = serde_yaml::from_value(val["triggers"].clone()).unwrap(); - assert!( - tc.pr.unwrap().synthetic_from_ci, - "synthetic-from-ci must default to true when omitted" + assert_eq!( + tc.pr.unwrap().mode, + PrMode::Synthetic, + "mode must default to synthetic when omitted" ); } #[test] - fn test_pr_trigger_config_synthetic_from_ci_explicit_false() { + fn test_pr_trigger_config_mode_explicit_synthetic() { let yaml = r#" triggers: pr: branches: include: [main] - synthetic-from-ci: false + mode: synthetic "#; let val: serde_yaml::Value = serde_yaml::from_str(yaml).unwrap(); let tc: OnConfig = serde_yaml::from_value(val["triggers"].clone()).unwrap(); - assert!( - !tc.pr.unwrap().synthetic_from_ci, - "synthetic-from-ci: false must opt out of synthesis" - ); + assert_eq!(tc.pr.unwrap().mode, PrMode::Synthetic); } #[test] - fn test_pr_trigger_config_synthetic_from_ci_explicit_true() { + fn test_pr_trigger_config_mode_explicit_policy() { let yaml = r#" triggers: pr: branches: include: [main] - synthetic-from-ci: true + mode: policy "#; let val: serde_yaml::Value = serde_yaml::from_str(yaml).unwrap(); let tc: OnConfig = serde_yaml::from_value(val["triggers"].clone()).unwrap(); - assert!(tc.pr.unwrap().synthetic_from_ci); + assert_eq!( + tc.pr.unwrap().mode, + PrMode::Policy, + "mode: policy must deserialise correctly" + ); + } + + #[test] + fn test_pr_trigger_config_mode_invalid_value_errors() { + let yaml = r#" +triggers: + pr: + branches: + include: [main] + mode: bananas +"#; + let val: serde_yaml::Value = serde_yaml::from_str(yaml).unwrap(); + let err = serde_yaml::from_value::(val["triggers"].clone()) + .expect_err("mode: bananas must be rejected at deserialisation"); + let msg = err.to_string(); + assert!( + msg.contains("bananas") || msg.contains("variant") || msg.contains("unknown"), + "error must mention the bad variant; got: {msg}" + ); } #[test] diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index 0704528b..bc3d4816 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -4249,13 +4249,13 @@ fn test_exec_context_pr_only_downloads_bundle_in_agent_job_not_setup() { ); if let Some(setup) = extract_job_block(&yaml, "Setup") { // Setup-job script bundle download IS expected when on.pr is - // configured (synthetic-from-ci default-on emits the synthPr + // configured (default `mode: synthetic` emits the synthPr // step, which is a bundle consumer). Only assert the Agent // job has the bundle download; the Setup-job download is the // synth feature's correct behaviour. assert!( setup.contains("Download ado-aw scripts"), - "Setup job SHOULD have the script bundle download when synthetic-from-ci is on (the synthPr step is a bundle consumer). Setup block: {}", + "Setup job SHOULD have the script bundle download when mode: synthetic is on (the synthPr step is a bundle consumer). Setup block: {}", setup ); } @@ -5187,7 +5187,7 @@ fn test_execution_context_pr_emits_prepare_step_and_prompt_supplement() { ); assert!( compiled.contains("condition: or(eq(variables['Build.Reason'], 'PullRequest'), eq(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], 'true'))"), - "Prepare step must be gated on PR builds OR synthetic-PR Setup outputs at the ADO condition layer (synthetic-from-ci is default-on)" + "Prepare step must be gated on PR builds OR synthetic-PR Setup outputs at the ADO condition layer (mode: synthetic is the default)" ); assert!( compiled.contains("SYSTEM_ACCESSTOKEN: $(System.AccessToken)"), @@ -5223,7 +5223,7 @@ fn test_execution_context_pr_emits_prepare_step_and_prompt_supplement() { "v7: the git_fetch wrapper moved into the bundle" ); - // v7 + synthetic-from-ci (default-on): env passthrough — the bundle + // v7 + mode: synthetic (the default): env passthrough — the bundle // reads ADO predefined vars from `process.env`. The compiler emits // coalesced macros that prefer the real `System.PullRequest.*` vars // (true PR builds) and fall back to the `synthPr` Setup-job outputs @@ -5647,7 +5647,7 @@ Body. // ─── Synthetic-from-ci snapshot fixtures (issue #916) ──────────────────────── -/// Fixture A: agent with on.pr and default synthetic-from-ci: true. +/// Fixture A: agent with on.pr and default `mode: synthetic`. /// Compiled YAML must contain the full synth wiring (synthPr Setup step, /// PR_SYNTH_SPEC env, broadened exec-context-pr.js condition, agent-job /// AW_SYNTHETIC_PR_SKIP guard). The CI trigger must NOT be auto-narrowed @@ -5701,13 +5701,15 @@ fn test_synthetic_pr_default_emits_full_synth_wiring() { ); } -/// Fixture B: agent with on.pr and synthetic-from-ci: false. -/// Back-compat guard — the compiled YAML must contain NONE of the -/// synthesis artefacts. (Substring-negation; no stored baseline required.) +/// Fixture B: agent with on.pr and `mode: policy`. +/// The compiled YAML must contain NONE of the synthesis artefacts AND +/// must emit `trigger: none` so feature-branch pushes do not queue +/// duplicate CI builds alongside the operator's branch-policy-driven PR +/// builds. #[test] -fn test_synthetic_pr_opt_out_omits_all_synth_artefacts() { - let compiled = compile_fixture_with_flags("synthetic-pr-opt-out.md", &["--skip-integrity"]); - assert_valid_yaml(&compiled, "synthetic-pr-opt-out.md"); +fn test_pr_mode_policy_omits_synth_and_emits_trigger_none() { + let compiled = compile_fixture_with_flags("pr-mode-policy.md", &["--skip-integrity"]); + assert_valid_yaml(&compiled, "pr-mode-policy.md"); for needle in &[ "synthPr", @@ -5717,14 +5719,22 @@ fn test_synthetic_pr_opt_out_omits_all_synth_artefacts() { ] { assert!( !compiled.contains(needle), - "synthetic-from-ci: false must produce zero synth artefacts; \ + "mode: policy must produce zero synth artefacts; \ found {needle} in compiled YAML" ); } - // Auto-narrowed CI trigger is ALSO suppressed when synthetic-from-ci is off. + // CI trigger must be suppressed so we don't double-queue with the + // policy-driven PR build. + assert!( + compiled.contains("trigger: none"), + "mode: policy must emit `trigger: none` so feature-branch pushes do not \ + queue duplicate CI builds alongside the branch-policy-driven PR build" + ); + + // And of course must NOT auto-narrow either (defensive). assert!( !compiled.contains("trigger:\n branches:\n include:\n - 'main'"), - "synthetic-from-ci: false must NOT auto-narrow the CI trigger (back-compat)" + "mode: policy must not emit a narrowed CI trigger" ); } diff --git a/tests/fixtures/pr-mode-policy.md b/tests/fixtures/pr-mode-policy.md new file mode 100644 index 00000000..058d3a08 --- /dev/null +++ b/tests/fixtures/pr-mode-policy.md @@ -0,0 +1,21 @@ +--- +name: "PR Mode Policy Agent" +description: "Fixture exercising on.pr with mode: policy (issue #916)" +on: + pr: + branches: + include: [main] + mode: policy +--- + +## PR Mode Policy Agent + +This agent has `on.pr` configured with `mode: policy`, meaning the +operator has installed an Azure DevOps Build Validation branch policy +that fires real `Build.Reason == PullRequest` builds and the compiler +must: + +- emit none of the synth artefacts (`synthPr`, `AW_SYNTHETIC_PR`, + `PR_SYNTH_SPEC`, `exec-context-pr-synth`), and +- emit `trigger: none` at the top level so feature-branch pushes do not + queue duplicate CI builds alongside the policy-driven PR build. diff --git a/tests/fixtures/synthetic-pr-default.md b/tests/fixtures/synthetic-pr-default.md index 796a5048..7dab497d 100644 --- a/tests/fixtures/synthetic-pr-default.md +++ b/tests/fixtures/synthetic-pr-default.md @@ -1,6 +1,6 @@ --- name: "Synthetic PR Default Agent" -description: "Fixture exercising on.pr with default synthetic-from-ci=true (issue #916)" +description: "Fixture exercising on.pr with default mode: synthetic (issue #916)" on: pr: branches: @@ -9,7 +9,7 @@ on: ## Synthetic PR Default Agent -This agent has `on.pr` configured with default `synthetic-from-ci: true`. +This agent has `on.pr` configured with the default `mode: synthetic`. On a CI-triggered build (no Build Validation policy), the Setup-job `synthPr` step looks up the open PR for `Build.SourceBranch` and exposes its identifiers so the gate evaluator and exec-context-pr bundles @@ -21,4 +21,4 @@ The compiled YAML must contain: - A `PR_SYNTH_SPEC:` env var carrying the base64 spec - The broadened `exec-context-pr.js` condition (`or(...)`) - The Agent-job `dependsOn` condition with the `AW_SYNTHETIC_PR_SKIP` guard -- A narrowed top-level `trigger:` block mirroring `pr.branches.include` +- No top-level `trigger:` block (ADO default = "trigger on every branch") diff --git a/tests/fixtures/synthetic-pr-opt-out.md b/tests/fixtures/synthetic-pr-opt-out.md deleted file mode 100644 index 01dad07a..00000000 --- a/tests/fixtures/synthetic-pr-opt-out.md +++ /dev/null @@ -1,19 +0,0 @@ ---- -name: "Synthetic PR Opt-Out Agent" -description: "Fixture exercising on.pr with synthetic-from-ci=false (issue #916 back-compat)" -on: - pr: - branches: - include: [main] - synthetic-from-ci: false ---- - -## Synthetic PR Opt-Out Agent - -This agent has `on.pr` configured with `synthetic-from-ci: false`, -preserving the pre-synthesis behaviour (requires an operator-installed -Build Validation branch policy for PR triggering to work). - -The compiled YAML must contain NONE of the synthesis artefacts — -`synthPr`, `AW_SYNTHETIC_PR`, `PR_SYNTH_SPEC`, or `exec-context-pr-synth` -— so back-compat is preserved. From 41a9436936e1c2b09ec8fed03fda1e8c6c62e67b Mon Sep 17 00:00:00 2001 From: James Devine Date: Tue, 9 Jun 2026 12:18:09 +0100 Subject: [PATCH 16/19] fix(compile): gate-step same-job synthPr ref and agent-condition gate enforcement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses two real correctness bugs and one stale comment from the Rust PR Reviewer feedback on #922. (1) Gate-step same-job synthPr reference (filter_ir.rs::compile_gate_step_external): The gate step is emitted into the Setup job (same job as `synthPr`), so the env-block references `dependencies.Setup.outputs['synthPr.X']` were silently wrong — that is cross-job syntax and resolves to null inside the producing job, making `coalesce(...)` always return the empty string. On synth-promoted CI builds this left `AW_SYNTHETIC_PR=''`, so `gate/bypass.ts` took the "not a PR build" auto-pass and the agent ran without `pr.filters` being evaluated at all. Fixed by switching the gate-step env coalesce to the same-job runtime expression `variables['synthPr.X']`, which resolves step output variables added to the producing job's variable scope. The Agent-job env (in `exec_context/pr.rs`) keeps `dependencies.Setup.outputs[...]` — that step runs cross-job where the dependencies form is the correct one. (2) Agent-job condition gate enforcement (common.rs::generate_agentic_depends_on): With `mode: synthetic` + `pr.filters`, the synth branch emitted `or(eq(Build.Reason, 'PullRequest'), eq(synthPr.AW_SYNTHETIC_PR, 'true'), eq(prGate.SHOULD_RUN, 'true'))`. The first two arms make any PR build (real or synth) run the agent UNCONDITIONALLY — silently bypassing the gate that `pr.filters` exists to enforce. Replaced with `or(and(ne(Build.Reason, 'PullRequest'), ne(synthPr.AW_SYNTHETIC_PR, 'true')), eq(prGate.SHOULD_RUN, 'true'))` — non-PR / non-synth builds run unconditionally; real-PR and synth-PR builds must pass the gate. (3) Removed stale `compile-coalesce-env todo` comment in `exec_context/pr.rs`; the work referenced is now implemented in the same function. Findings 2 and 3 from the same review were false alarms: `$[ coalesce(...) ]` IS documented as valid in step-level `env:` blocks, and `System.PullRequest.TargetBranch` is documented as the full `refs/heads/` form, matching `pr.targetRefName`. Both clarified inline with MS-docs cross-references. Test changes: * `test_agentic_depends_on_synthetic_pr_active_emits_skip_guard_and_gate_enforced_pr_clause` (renamed from `_and_broader_pr_clause`) — pins the new AND-NOT shape and asserts the old permissive bypass arms are gone. * `test_pr_filter_synth_mode_agent_condition_enforces_gate` — integration test that exercises the `pr-filter-tier1-agent.md` fixture (mode: synthetic + pr.filters) and asserts the Agent-job dependsOn condition contains both AND-NOT arms and none of the buggy bypass arms. * `test_pr_filter_synth_mode_gate_step_uses_same_job_synth_ref` — integration test asserting the gate-step env uses `variables['synthPr.X']` (same-job) and contains no `dependencies.Setup.outputs['synthPr.X']` cross-job references inside the producing job. * `test_synthetic_pr_default_emits_full_synth_wiring` — dropped the misleading `eq(synthPr.AW_SYNTHETIC_PR, 'true')` assertion (the fixture has no filters, so that string came from the exec-context-pr step, not the Agent-job condition). All `cargo test` (1811 lib + 131 compiler-integration + others) and `cargo clippy --all-targets --all-features` pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/exec-context-pr-synth/index.ts | 16 +- src/compile/common.rs | 62 ++++--- src/compile/extensions/exec_context/pr.rs | 16 +- src/compile/filter_ir.rs | 23 ++- tests/compiler_tests.rs | 158 +++++++++++++++++- 5 files changed, 241 insertions(+), 34 deletions(-) diff --git a/scripts/ado-script/src/exec-context-pr-synth/index.ts b/scripts/ado-script/src/exec-context-pr-synth/index.ts index ff0bc3ee..46f2f231 100644 --- a/scripts/ado-script/src/exec-context-pr-synth/index.ts +++ b/scripts/ado-script/src/exec-context-pr-synth/index.ts @@ -172,9 +172,19 @@ export async function main(env: NodeJS.ProcessEnv = process.env): Promise`, matching + // the `targetRefName` / `sourceRefName` shape returned by the ADO + // REST API (`refs/heads/main`, `refs/heads/feature/x`, etc.). The + // coalesce on consumer steps therefore yields a consistent + // refs-prefixed value whether the build was a real PR or synth- + // promoted. (The unprefixed short form is `TargetBranchName` — + // a separate predefined variable we deliberately do not use here.) + // See . setOutput("AW_SYNTHETIC_PR", "true"); setOutput("AW_SYNTHETIC_PR_ID", String(prId)); setOutput("AW_SYNTHETIC_PR_TARGETBRANCH", pr.targetRefName ?? ""); diff --git a/src/compile/common.rs b/src/compile/common.rs index 7df08396..2dc06cfd 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -2413,17 +2413,28 @@ pub fn generate_agentic_depends_on( } if has_pr_filters { // With `mode: synthetic`, the agent should run when EITHER - // (a) it is a real PR build (existing path) OR - // (b) the synthPr step promoted this CI build to PR semantics - // (synthPr.AW_SYNTHETIC_PR=true) OR - // (c) the gate passed for any other reason. - // With `mode: policy`, the existing two-arm condition - // is preserved verbatim (no synth path is active). + // (a) the build is neither a real PR build NOR a synth-promoted + // CI build — in that case the gate doesn't apply (bypass.ts + // auto-passes) and the agent runs unconditionally, OR + // (b) the gate evaluator passed (`prGate.SHOULD_RUN=true`), + // covering both the real-PR-with-filter-match path and the + // synth-PR-with-filter-match path. + // + // CRITICAL: do NOT emit `eq(Build.Reason, 'PullRequest')` or + // `eq(synthPr.AW_SYNTHETIC_PR, 'true')` as standalone OR + // arms — that would let a real PR or synth-promoted build run + // the agent EVEN WHEN `pr.filters` failed (i.e. silently + // bypass the gate for the very builds it's meant to filter). + // + // With `mode: policy` (synth not active), the original + // two-arm condition is preserved verbatim. if synthetic_pr_active { parts.push( r"or( - eq(variables['Build.Reason'], 'PullRequest'), - eq(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], 'true'), + and( + ne(variables['Build.Reason'], 'PullRequest'), + ne(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], 'true') + ), eq(dependencies.Setup.outputs['prGate.SHOULD_RUN'], 'true') )" .to_string(), @@ -8411,30 +8422,41 @@ safe-outputs: } #[test] - fn test_agentic_depends_on_synthetic_pr_active_emits_skip_guard_and_broader_pr_clause() { + fn test_agentic_depends_on_synthetic_pr_active_emits_skip_guard_and_gate_enforced_pr_clause() { // synthetic_pr_active=true + has_pr_filters=true → emits the - // AW_SYNTHETIC_PR_SKIP guard and broadens the PR clause to - // accept real PR builds OR synthPr promotion OR gate-passed. + // AW_SYNTHETIC_PR_SKIP guard and a gate-enforced PR clause: real + // and synth PR builds must pass the gate (no permissive + // bypass arms). let out = generate_agentic_depends_on(&[], true, false, &[], false, true); assert!(out.contains("dependsOn: Setup"), "should depend on Setup"); assert!( out.contains("ne(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_SKIP'], 'true')"), "must honour the synth-skip flag: {out}" ); + // The PR clause must REQUIRE the gate for real-PR AND synth-PR + // builds — i.e. allow unconditional run only when neither + // applies. Both AND-NOT arms must be present. assert!( - out.contains("eq(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], 'true')"), - "must accept synthetic-PR promotion as an activation reason: {out}" + out.contains("ne(variables['Build.Reason'], 'PullRequest')"), + "must contain the `ne(Build.Reason, 'PullRequest')` AND-NOT arm: {out}" ); assert!( - out.contains("eq(variables['Build.Reason'], 'PullRequest')"), - "must still accept real PR builds: {out}" + out.contains("ne(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], 'true')"), + "must contain the `ne(synthPr.AW_SYNTHETIC_PR, 'true')` AND-NOT arm: {out}" ); - // Old "ne(Build.Reason, PullRequest)" arm must be GONE — that was - // the pre-synthetic permissive default and now contradicts the - // synth-skip guard. assert!( - !out.contains("ne(variables['Build.Reason'], 'PullRequest')"), - "synth path must replace the permissive ne(Build.Reason, PullRequest) arm: {out}" + out.contains("eq(dependencies.Setup.outputs['prGate.SHOULD_RUN'], 'true')"), + "must still accept gate-passed as an activation reason: {out}" + ); + // Defensive regression guards: the old permissive arms that + // bypassed the gate for any PR build MUST be gone. + assert!( + !out.contains("eq(variables['Build.Reason'], 'PullRequest')"), + "the buggy `eq(Build.Reason, PullRequest)` bypass arm must be gone: {out}" + ); + assert!( + !out.contains("eq(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], 'true')"), + "the buggy `eq(synthPr.AW_SYNTHETIC_PR, true)` bypass arm must be gone: {out}" ); } diff --git a/src/compile/extensions/exec_context/pr.rs b/src/compile/extensions/exec_context/pr.rs index a6ca14c5..1ffbbbca 100644 --- a/src/compile/extensions/exec_context/pr.rs +++ b/src/compile/extensions/exec_context/pr.rs @@ -127,8 +127,20 @@ impl ContextContributor for PrContextContributor { // up either the real `System.PullRequest.*` (on a true PR // build) OR the synthPr Setup-job output (on a CI build // promoted via exec-context-pr-synth.js). The step's - // condition is also broadened in the - // `compile-exec-context-cond` todo. + // condition is also broadened to accept synth-promoted builds. + // + // Cross-job reference is correct here: this step runs in the + // **Agent** job (which depends on Setup), so + // `dependencies.Setup.outputs['synthPr.X']` resolves at runtime. + // (Same-job references would need `variables['synthPr.X']` + // instead — used by the gate step inside the Setup job itself.) + // Runtime expressions `$[ ... ]` are documented as valid in + // step-level `env:` blocks; see + // . + // `System.PullRequest.TargetBranch` is `refs/heads/` (full + // ref form), matching the `targetRefName` shape returned by the + // ADO REST API and stored in `AW_SYNTHETIC_PR_TARGETBRANCH`, so + // the coalesce yields a consistent value either way. let (pr_id_macro, target_branch_macro, condition) = if self.synthetic_pr_active { ( "$[ coalesce(variables['System.PullRequest.PullRequestId'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_ID']) ]", diff --git a/src/compile/filter_ir.rs b/src/compile/filter_ir.rs index bd06ce74..c6e7fed2 100644 --- a/src/compile/filter_ir.rs +++ b/src/compile/filter_ir.rs @@ -1126,6 +1126,18 @@ pub fn build_gate_spec(ctx: GateContext, checks: &[FilterCheck]) -> anyhow::Resu /// PR build) OR the `synthPr` Setup-job outputs (on a CI build promoted /// by `exec-context-pr-synth.js`). Also exports `AW_SYNTHETIC_PR` so /// `gate/bypass.ts` knows to skip the "not a PR build" bypass. +/// +/// **Same-job vs cross-job reference**: this gate step lives in the +/// **Setup job** (`AdoScriptExtension::setup_steps` returns it), the +/// same job as `synthPr`. Within the producing job, the cross-job form +/// `dependencies.Setup.outputs['synthPr.X']` is undefined (a job has +/// no entry for itself in `dependencies`), so we use the same-job +/// runtime expression `variables['synthPr.X']` instead, which resolves +/// step output variables added to the job's variable scope by prior +/// `isOutput=true` setvariable commands. See +/// . +/// Runtime expressions (`$[ ... ]`) are valid in step-level `env:` +/// blocks per the same docs. pub fn compile_gate_step_external( ctx: GateContext, checks: &[FilterCheck], @@ -1160,11 +1172,12 @@ pub fn compile_gate_step_external( // has been promoted to PR semantics, so the "not a PullRequest // build" bypass must not auto-pass. Always safe to emit (the gate // checks it strictly for the literal "true"), but only meaningful - // for PR gates. + // for PR gates. Same-job ref via `variables['synthPr.X']` — see + // function doc-comment for why this is NOT `dependencies.Setup...`. let pr_synth_active = synthetic_pr_active && matches!(ctx, GateContext::PullRequest); if pr_synth_active { step.push_str( - " AW_SYNTHETIC_PR: $[ coalesce(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], '') ]\n", + " AW_SYNTHETIC_PR: $[ coalesce(variables['synthPr.AW_SYNTHETIC_PR'], '') ]\n", ); } @@ -1172,13 +1185,13 @@ pub fn compile_gate_step_external( let macro_str = if pr_synth_active { match *env_var { "ADO_PR_ID" => { - "$[ coalesce(variables['System.PullRequest.PullRequestId'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_ID']) ]" + "$[ coalesce(variables['System.PullRequest.PullRequestId'], variables['synthPr.AW_SYNTHETIC_PR_ID']) ]" } "ADO_SOURCE_BRANCH" => { - "$[ coalesce(variables['System.PullRequest.SourceBranch'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_SOURCEBRANCH']) ]" + "$[ coalesce(variables['System.PullRequest.SourceBranch'], variables['synthPr.AW_SYNTHETIC_PR_SOURCEBRANCH']) ]" } "ADO_TARGET_BRANCH" => { - "$[ coalesce(variables['System.PullRequest.TargetBranch'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_TARGETBRANCH']) ]" + "$[ coalesce(variables['System.PullRequest.TargetBranch'], variables['synthPr.AW_SYNTHETIC_PR_TARGETBRANCH']) ]" } _ => ado_macro, } diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index bc3d4816..2de91493 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -4371,6 +4371,152 @@ fn test_pr_filter_agent_depends_on_setup() { ); } +/// Regression guard for the synth-mode gate-bypass bug: with `mode: +/// synthetic` (the default) AND `on.pr.filters` present, the Agent-job +/// condition must REQUIRE the gate to pass for real-PR and synth-PR +/// builds. Earlier iterations emitted `or(eq(Build.Reason, 'PullRequest'), +/// eq(synthPr.AW_SYNTHETIC_PR, 'true'), ...)` which silently bypassed the +/// gate for any PR build — defeating the purpose of `pr.filters`. +#[test] +fn test_pr_filter_synth_mode_agent_condition_enforces_gate() { + let compiled = compile_fixture("pr-filter-tier1-agent.md"); + + // Extract the Agent-job dependsOn condition body so the assertions + // target only that section (the same strings can appear elsewhere — + // e.g. the exec-context-pr.js step's condition — and would create + // false positives if we matched the whole compiled output). + let agent_block = extract_job_block(&compiled, "Agent").expect("Agent job present"); + let condition_section = agent_block + .split("condition: |") + .nth(1) + .map(|tail| { + // Stop at the next top-level Agent-job field. `steps:` always + // exists; `pool:` / `variables:` / `workspace:` may exist + // before it. The first one we hit terminates the condition + // body. Using exact field names avoids matching inner + // condition lines that start with 4+ spaces. + let stop_at = [ + "\n pool:", + "\n steps:", + "\n variables:", + "\n workspace:", + ]; + let end = stop_at + .iter() + .filter_map(|needle| tail.find(needle)) + .min() + .unwrap_or(tail.len()); + &tail[..end] + }) + .unwrap_or(""); + + // Correct shape: the AND-NOT clause requiring (not real PR) AND + // (not synth PR) before the unconditional-run branch is taken. + // Whitespace-agnostic substring matches. + assert!( + condition_section.contains("ne(variables['Build.Reason'], 'PullRequest')") + && condition_section + .contains("ne(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], 'true')"), + "Agent-job dependsOn condition must contain the AND-NOT arms \ + `ne(Build.Reason, 'PullRequest')` and `ne(synthPr.AW_SYNTHETIC_PR, 'true')` \ + so the gate is enforced for PR builds (real or synth). \ + Condition section: {condition_section}" + ); + assert!( + condition_section.contains("eq(dependencies.Setup.outputs['prGate.SHOULD_RUN'], 'true')"), + "Agent-job dependsOn condition must keep the gate-passed activation arm: \ + {condition_section}" + ); + + // Defensive: the old permissive bypass arms that bypassed the gate + // for any PR build MUST NOT appear inside the Agent-job dependsOn + // condition. + assert!( + !condition_section.contains("eq(variables['Build.Reason'], 'PullRequest')"), + "Agent-job dependsOn condition must NOT contain the buggy \ + `eq(Build.Reason, 'PullRequest')` bypass arm (would auto-run on \ + every real PR build regardless of gate): {condition_section}" + ); + assert!( + !condition_section + .contains("eq(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], 'true')"), + "Agent-job dependsOn condition must NOT contain the buggy \ + `eq(synthPr.AW_SYNTHETIC_PR, 'true')` bypass arm (would auto-run on \ + every synth-promoted build regardless of gate): {condition_section}" + ); +} + +/// Regression guard for the synth-mode gate-step same-job ref bug: the +/// gate step lives in the **Setup** job (same job as `synthPr`), so its +/// env block must use `variables['synthPr.X']` (same-job runtime +/// expression) — `dependencies.Setup.outputs[...]` is undefined inside +/// the producing job and silently coalesces to empty, leaving +/// `AW_SYNTHETIC_PR` empty and causing the bypass to misfire. +#[test] +fn test_pr_filter_synth_mode_gate_step_uses_same_job_synth_ref() { + let compiled = compile_fixture("pr-filter-tier1-agent.md"); + + assert!( + compiled + .contains("AW_SYNTHETIC_PR: $[ coalesce(variables['synthPr.AW_SYNTHETIC_PR'], '') ]"), + "Gate step env must use same-job `variables['synthPr.X']` runtime expression — \ + `dependencies.Setup.outputs[...]` is undefined inside the producing Setup job" + ); + // The fixture exercises source-branch and target-branch filters, + // so the synth-coalesce treatment must appear on those env vars + // using the same-job `variables[...]` form. (ADO_PR_ID is not + // exported by this fixture's filter set, so we don't assert it here.) + assert!( + compiled.contains( + "ADO_SOURCE_BRANCH: $[ coalesce(variables['System.PullRequest.SourceBranch'], variables['synthPr.AW_SYNTHETIC_PR_SOURCEBRANCH']) ]" + ), + "ADO_SOURCE_BRANCH coalesce must use same-job `variables[...]` form" + ); + assert!( + compiled.contains( + "ADO_TARGET_BRANCH: $[ coalesce(variables['System.PullRequest.TargetBranch'], variables['synthPr.AW_SYNTHETIC_PR_TARGETBRANCH']) ]" + ), + "ADO_TARGET_BRANCH coalesce must use same-job `variables[...]` form" + ); + + // The same-job gate step MUST NOT use the cross-job + // `dependencies.Setup.outputs[...]` form for synthPr references. + // (It's fine elsewhere — e.g. the Agent-job dependsOn condition — but + // not inside the Setup job's own steps.) + // The same-job gate step MUST NOT use the cross-job + // `dependencies.Setup.outputs[...]` form for synthPr references. + // (It's fine elsewhere — e.g. the Agent-job dependsOn condition — but + // not inside the Setup job's own steps.) Bound the gate-step + // section by the start of the next top-level job (`\n - job: `), + // since extract_job_block's `\n- job: ` boundary doesn't match the + // 2-space-indented job list items produced for this target. + let setup_block = extract_job_block(&compiled, "Setup").expect("Setup job present"); + let gate_section = setup_block + .split("name: prGate") + .nth(1) + .map(|tail| { + let stop_at = [ + "\n - bash:", + "\n - task:", + "\n - script:", + "\n - job: ", + ]; + let end = stop_at + .iter() + .filter_map(|needle| tail.find(needle)) + .min() + .unwrap_or(tail.len()); + &tail[..end] + }) + .unwrap_or(""); + assert!( + !gate_section.contains("dependencies.Setup.outputs['synthPr."), + "Gate step (inside Setup job) must NOT reference `dependencies.Setup.outputs['synthPr.X']` — \ + that is cross-job syntax and is undefined within the producing job. \ + Gate section: {gate_section}" + ); +} + /// Native ADO PR trigger block is emitted for branch/path filters. #[test] fn test_pr_filter_tier1_has_native_pr_trigger() { @@ -5685,10 +5831,14 @@ fn test_synthetic_pr_default_emits_full_synth_wiring() { compiled.contains("ne(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_SKIP'], 'true')"), "Fixture A's Agent-job condition must honour the synth-skip flag" ); - assert!( - compiled.contains("eq(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], 'true')"), - "Fixture A's Agent-job condition must accept synth promotion as an activation reason" - ); + // NOTE: this fixture does not declare `on.pr.filters`, so the + // Agent-job condition has only the skip guard (no AND-NOT gate + // clause). The `eq(synthPr.AW_SYNTHETIC_PR, 'true')` literal is + // therefore expected ONLY in the exec-context-pr.js step's + // broadened condition (asserted above) — never as an Agent-job + // OR-arm, which would silently bypass the gate for real-PR or + // synth-PR builds. A separate fixture covers the gate-enforced + // shape when `pr.filters` is present. // No auto-narrowed CI trigger — `pr.branches.include` lists PR TARGET // branches, and ADO `trigger:` fires on pushes TO listed branches, so From 6ccb13231710c83f5357214d0d7c109cb4ee667c Mon Sep 17 00:00:00 2001 From: James Devine Date: Tue, 9 Jun 2026 13:28:01 +0100 Subject: [PATCH 17/19] fix: address Rust PR Reviewer feedback round 4 (#922) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings from the latest review on this PR: (1) `exec-context-pr-synth/index.ts` runtime-contract comment was misleading: step 4 said `branches.include/exclude miss on BUILD_SOURCEBRANCH → skip`, but the bundle never filters `on.pr.branches` against the source branch (that would be wrong — `on.pr.branches` lists PR *target* branches per ADO semantics). The actual flow fetches PRs by `sourceRefName == BUILD_SOURCEBRANCH`, then filters the matched PRs by their `targetRefName` against `spec.branches`. Updated the contract comment to reflect this. (2) `ado-client.ts` comment claimed "first page (200 PRs)" — ADO's `getPullRequests` default page size without an explicit `$top` is 100, not 200. Updated to match the SDK default. (3) `AdoScriptExtension` previously had two coupled fields (`synthetic_pr_active: bool` + `pr_trigger_for_synth: Option`) whose pairing was enforced only by a runtime `anyhow!` guard in `setup_steps`. Refactored to a single field `pr_trigger_for_synth: Option<...>` whose `is_some()` IS the activation predicate, exposed as `synthetic_pr_active()` for callers that read it. The invariant is now unrepresentable-wrong at the type level; the runtime guard is gone. Tests and `collect_extensions` updated accordingly. (4) Wrapped `$[ ... ]` runtime expressions in YAML double quotes in the step `env:` blocks emitted by `filter_ir.rs::compile_gate_step_external` and `exec_context/pr.rs::prepare_step`. The values contain single quotes (`variables['System.PullRequest.X']`) and although ADO's YAML parser accepts them unquoted in practice, double-quoting is the form shown in ADO docs and is strictly conformant to the YAML spec (which reserves `'` as a scalar indicator). Adjusted the corresponding integration-test assertions to match. Validation: - `cargo build` clean - `cargo test` (1811 lib + 131 compiler-integration + others) 0 failures - `cargo clippy --all-targets --all-features` clean - `npm test` in scripts/ado-script/ — 281/281 vitest passes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/exec-context-pr-synth/index.ts | 10 ++-- scripts/ado-script/src/shared/ado-client.ts | 7 +-- src/compile/extensions/ado_script.rs | 46 +++++++++++-------- src/compile/extensions/exec_context/pr.rs | 10 +++- src/compile/extensions/mod.rs | 21 +++++---- src/compile/filter_ir.rs | 13 ++++-- tests/compiler_tests.rs | 13 +++--- 7 files changed, 73 insertions(+), 47 deletions(-) diff --git a/scripts/ado-script/src/exec-context-pr-synth/index.ts b/scripts/ado-script/src/exec-context-pr-synth/index.ts index 46f2f231..0a3b70f9 100644 --- a/scripts/ado-script/src/exec-context-pr-synth/index.ts +++ b/scripts/ado-script/src/exec-context-pr-synth/index.ts @@ -23,10 +23,14 @@ * 1. real PR build (BUILD_REASON=PullRequest) → no-op * 2. GitHub-typed repo (BUILD_REPOSITORY_PROVIDER=GitHub) → no-op * 3. Decode PR_SYNTH_SPEC (hard fail on corruption) - * 4. branches.include/exclude miss on BUILD_SOURCEBRANCH → skip - * 5. fetch open PRs by sourceRefName + filter by targetRefName + * 4. fetch active PRs whose `sourceRefName == BUILD_SOURCEBRANCH` + * (no source-branch pre-filter against the spec — `on.pr.branches` + * lists *target* branches per ADO semantics, not the build's + * source branch) + * 5. filter matched PRs by `targetRefName` against + * `spec.branches.include` / `spec.branches.exclude` * 6. count != 1 → skip - * 7. paths.include/exclude reject everything → skip + * 7. paths.include/exclude reject every changed file → skip * 8. emit AW_SYNTHETIC_PR* outputs */ import { diff --git a/scripts/ado-script/src/shared/ado-client.ts b/scripts/ado-script/src/shared/ado-client.ts index 4e62d448..d4356bb3 100644 --- a/scripts/ado-script/src/shared/ado-client.ts +++ b/scripts/ado-script/src/shared/ado-client.ts @@ -104,9 +104,10 @@ export async function getPullRequestById( * * Returns an empty array if no PRs match. The ADO REST API caps page * size; for the synth path we deliberately fetch only the first page - * (200 PRs) since the synth contract requires *exactly one* match — - * a source branch with >200 simultaneous active PRs against it is - * pathological and the bundle will skip via the "multi-match" path. + * (the SDK default is 100 PRs without an explicit `$top`) since the + * synth contract requires *exactly one* match — a source branch with + * >100 simultaneous active PRs against it is pathological and the + * bundle will skip via the "multi-match" path anyway. */ export async function listActivePullRequestsBySourceRef( project: string, diff --git a/src/compile/extensions/ado_script.rs b/src/compile/extensions/ado_script.rs index 5d79d3f6..164cfef0 100644 --- a/src/compile/extensions/ado_script.rs +++ b/src/compile/extensions/ado_script.rs @@ -57,19 +57,32 @@ pub struct AdoScriptExtension { /// shared `exec_context_pr_active` predicate so this stays in /// lock-step with `ExecContextExtension`'s own activation gate. pub exec_context_pr_active: bool, - /// Whether the synthetic-from-ci path is active for this agent. - /// Set when `on.pr.mode == Synthetic` (the default). Drives: + /// PR trigger config required to build `PR_SYNTH_SPEC`. `Some(_)` + /// is the single source of truth for "synthetic-from-ci path is + /// active for this agent" — `is_some()` replaces what used to be a + /// separate `synthetic_pr_active: bool` field, eliminating the + /// invariant that the two had to be set together. Drives: + /// /// - Setup-job install/download fire (even with no `filters:`). /// - Setup-job `synthPr` step emission (before any gate step). /// - Downstream env coalescing (handled in `compile-coalesce-env`). - pub synthetic_pr_active: bool, - /// The PR trigger config required to build `PR_SYNTH_SPEC`. Only - /// populated when `synthetic_pr_active` is `true`. Cloned because - /// the extension outlives the borrow of `FrontMatter` in - /// `collect_extensions`. + /// + /// Cloned from the front-matter because the extension outlives the + /// borrow of `FrontMatter` in `collect_extensions`. pub pr_trigger_for_synth: Option, } +impl AdoScriptExtension { + /// Whether the synthetic-from-ci path is active for this agent. + /// Set when `on.pr.mode == Synthetic` (the default), in which case + /// `pr_trigger_for_synth` is populated. The compile-time + /// invariant "if active, the spec must be available" is encoded in + /// the field type, so this is just a thin accessor. + pub fn synthetic_pr_active(&self) -> bool { + self.pr_trigger_for_synth.is_some() + } +} + impl AdoScriptExtension { /// Compute the lowered PR and pipeline checks once. Returns /// `(pr_checks, pipeline_checks)`; either may be empty, in which @@ -207,17 +220,15 @@ impl CompilerExtension for AdoScriptExtension { fn setup_steps(&self, _ctx: &CompileContext) -> Result> { let (pr_checks, pipeline_checks) = self.lowered_checks(); - if pr_checks.is_empty() && pipeline_checks.is_empty() && !self.synthetic_pr_active { + if pr_checks.is_empty() && pipeline_checks.is_empty() && !self.synthetic_pr_active() { return Ok(vec![]); } let mut steps = install_and_download_steps(); - if self.synthetic_pr_active { - let pr = self.pr_trigger_for_synth.as_ref().ok_or_else(|| { - anyhow::anyhow!( - "synthetic_pr_active is true but pr_trigger_for_synth is None — \ - collect_extensions must populate both together" - ) - })?; + // `pr_trigger_for_synth.is_some()` is the type-level encoding + // of "synth path is active for this agent" — no separate flag + // to keep in lock-step. If `Some(_)`, the spec is guaranteed + // available. + if let Some(pr) = self.pr_trigger_for_synth.as_ref() { let spec_b64 = crate::compile::filter_ir::build_pr_synth_spec(pr)?; steps.push(synthetic_pr_step(&spec_b64)); } @@ -226,7 +237,7 @@ impl CompilerExtension for AdoScriptExtension { GateContext::PullRequest, &pr_checks, GATE_EVAL_PATH, - self.synthetic_pr_active, + self.synthetic_pr_active(), )?); } if !pipeline_checks.is_empty() { @@ -472,7 +483,6 @@ mod tests { pipeline_filters: pipeline, inlined_imports: inlined, exec_context_pr_active: false, - synthetic_pr_active: false, pr_trigger_for_synth: None, } } @@ -526,7 +536,6 @@ mod tests { pipeline_filters: None, inlined_imports: true, exec_context_pr_active: false, - synthetic_pr_active: true, pr_trigger_for_synth: Some(PrTriggerConfig { branches: Some(BranchFilter { include: vec!["main".into()], @@ -567,7 +576,6 @@ mod tests { pipeline_filters: None, inlined_imports: true, exec_context_pr_active: false, - synthetic_pr_active: true, pr_trigger_for_synth: Some(PrTriggerConfig { branches: Some(BranchFilter { include: vec!["main".into()], diff --git a/src/compile/extensions/exec_context/pr.rs b/src/compile/extensions/exec_context/pr.rs index 1ffbbbca..594a5ccb 100644 --- a/src/compile/extensions/exec_context/pr.rs +++ b/src/compile/extensions/exec_context/pr.rs @@ -141,10 +141,16 @@ impl ContextContributor for PrContextContributor { // ref form), matching the `targetRefName` shape returned by the // ADO REST API and stored in `AW_SYNTHETIC_PR_TARGETBRANCH`, so // the coalesce yields a consistent value either way. + // `$[ ... ]` runtime expressions are wrapped in YAML double + // quotes because their values contain single quotes (e.g. + // `variables['System.PullRequest.PullRequestId']`). ADO accepts + // them unquoted in practice, but double-quoting matches the + // form shown in ADO docs and is strictly conformant to the + // YAML spec (which reserves `'` as a scalar indicator). let (pr_id_macro, target_branch_macro, condition) = if self.synthetic_pr_active { ( - "$[ coalesce(variables['System.PullRequest.PullRequestId'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_ID']) ]", - "$[ coalesce(variables['System.PullRequest.TargetBranch'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_TARGETBRANCH']) ]", + "\"$[ coalesce(variables['System.PullRequest.PullRequestId'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_ID']) ]\"", + "\"$[ coalesce(variables['System.PullRequest.TargetBranch'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_TARGETBRANCH']) ]\"", "or(eq(variables['Build.Reason'], 'PullRequest'), eq(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], 'true'))", ) } else { diff --git a/src/compile/extensions/mod.rs b/src/compile/extensions/mod.rs index ed38212d..0aca3964 100644 --- a/src/compile/extensions/mod.rs +++ b/src/compile/extensions/mod.rs @@ -700,11 +700,17 @@ pub fn collect_extensions(front_matter: &FrontMatter) -> Vec { Extension::AdoScript(Box::new({ // PR trigger config drives both the PR-context contributor // (exec-context-pr.js) and the synthetic-from-ci path - // (exec-context-pr-synth.js). Compute the two flags from - // the same source so they stay in lock-step. + // (exec-context-pr-synth.js). + // + // `pr_trigger_for_synth` is the SINGLE source of truth for + // synth-path activation: when `Some(_)` the extension emits + // the synthPr Setup-job step and downstream wiring; when + // `None` it doesn't. The previous separate `bool` flag is + // now derived via `AdoScriptExtension::synthetic_pr_active()`. let pr_cfg = front_matter.pr_trigger(); - let synthetic_pr_active = - pr_cfg.is_some_and(|p| matches!(p.mode, crate::compile::types::PrMode::Synthetic)); + let pr_trigger_for_synth = pr_cfg + .filter(|p| matches!(p.mode, crate::compile::types::PrMode::Synthetic)) + .cloned(); AdoScriptExtension { pr_filters: front_matter.pr_filters().cloned(), pipeline_filters: front_matter.pipeline_filters().cloned(), @@ -717,12 +723,7 @@ pub fn collect_extensions(front_matter: &FrontMatter) -> Vec { // AdoScriptExtension owns installing it. Shared helper // keeps the activation predicate in lock-step. exec_context_pr_active: pr_contributor_will_activate(front_matter), - synthetic_pr_active, - pr_trigger_for_synth: if synthetic_pr_active { - pr_cfg.cloned() - } else { - None - }, + pr_trigger_for_synth, } })), // Always-on execution-context extension. Owns the `aw-context/` diff --git a/src/compile/filter_ir.rs b/src/compile/filter_ir.rs index c6e7fed2..7e6a6cb2 100644 --- a/src/compile/filter_ir.rs +++ b/src/compile/filter_ir.rs @@ -1176,8 +1176,13 @@ pub fn compile_gate_step_external( // function doc-comment for why this is NOT `dependencies.Setup...`. let pr_synth_active = synthetic_pr_active && matches!(ctx, GateContext::PullRequest); if pr_synth_active { + // YAML-quote runtime expressions whose value contains single quotes. + // Per ADO docs, `$[ ... ]` runtime expressions are valid in step + // `env:` blocks; wrapping in double quotes keeps the value + // strictly conformant to the YAML spec (which reserves `'` as a + // scalar indicator) and matches the form shown in ADO docs. step.push_str( - " AW_SYNTHETIC_PR: $[ coalesce(variables['synthPr.AW_SYNTHETIC_PR'], '') ]\n", + " AW_SYNTHETIC_PR: \"$[ coalesce(variables['synthPr.AW_SYNTHETIC_PR'], '') ]\"\n", ); } @@ -1185,13 +1190,13 @@ pub fn compile_gate_step_external( let macro_str = if pr_synth_active { match *env_var { "ADO_PR_ID" => { - "$[ coalesce(variables['System.PullRequest.PullRequestId'], variables['synthPr.AW_SYNTHETIC_PR_ID']) ]" + "\"$[ coalesce(variables['System.PullRequest.PullRequestId'], variables['synthPr.AW_SYNTHETIC_PR_ID']) ]\"" } "ADO_SOURCE_BRANCH" => { - "$[ coalesce(variables['System.PullRequest.SourceBranch'], variables['synthPr.AW_SYNTHETIC_PR_SOURCEBRANCH']) ]" + "\"$[ coalesce(variables['System.PullRequest.SourceBranch'], variables['synthPr.AW_SYNTHETIC_PR_SOURCEBRANCH']) ]\"" } "ADO_TARGET_BRANCH" => { - "$[ coalesce(variables['System.PullRequest.TargetBranch'], variables['synthPr.AW_SYNTHETIC_PR_TARGETBRANCH']) ]" + "\"$[ coalesce(variables['System.PullRequest.TargetBranch'], variables['synthPr.AW_SYNTHETIC_PR_TARGETBRANCH']) ]\"" } _ => ado_macro, } diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index 2de91493..9f03a25d 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -4457,8 +4457,9 @@ fn test_pr_filter_synth_mode_gate_step_uses_same_job_synth_ref() { let compiled = compile_fixture("pr-filter-tier1-agent.md"); assert!( - compiled - .contains("AW_SYNTHETIC_PR: $[ coalesce(variables['synthPr.AW_SYNTHETIC_PR'], '') ]"), + compiled.contains( + "AW_SYNTHETIC_PR: \"$[ coalesce(variables['synthPr.AW_SYNTHETIC_PR'], '') ]\"" + ), "Gate step env must use same-job `variables['synthPr.X']` runtime expression — \ `dependencies.Setup.outputs[...]` is undefined inside the producing Setup job" ); @@ -4468,13 +4469,13 @@ fn test_pr_filter_synth_mode_gate_step_uses_same_job_synth_ref() { // exported by this fixture's filter set, so we don't assert it here.) assert!( compiled.contains( - "ADO_SOURCE_BRANCH: $[ coalesce(variables['System.PullRequest.SourceBranch'], variables['synthPr.AW_SYNTHETIC_PR_SOURCEBRANCH']) ]" + "ADO_SOURCE_BRANCH: \"$[ coalesce(variables['System.PullRequest.SourceBranch'], variables['synthPr.AW_SYNTHETIC_PR_SOURCEBRANCH']) ]\"" ), "ADO_SOURCE_BRANCH coalesce must use same-job `variables[...]` form" ); assert!( compiled.contains( - "ADO_TARGET_BRANCH: $[ coalesce(variables['System.PullRequest.TargetBranch'], variables['synthPr.AW_SYNTHETIC_PR_TARGETBRANCH']) ]" + "ADO_TARGET_BRANCH: \"$[ coalesce(variables['System.PullRequest.TargetBranch'], variables['synthPr.AW_SYNTHETIC_PR_TARGETBRANCH']) ]\"" ), "ADO_TARGET_BRANCH coalesce must use same-job `variables[...]` form" ); @@ -5375,11 +5376,11 @@ fn test_execution_context_pr_emits_prepare_step_and_prompt_supplement() { // (true PR builds) and fall back to the `synthPr` Setup-job outputs // (CI builds promoted via exec-context-pr-synth.js). assert!( - compiled.contains("SYSTEM_PULLREQUEST_PULLREQUESTID: $[ coalesce(variables['System.PullRequest.PullRequestId'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_ID']) ]"), + compiled.contains("SYSTEM_PULLREQUEST_PULLREQUESTID: \"$[ coalesce(variables['System.PullRequest.PullRequestId'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_ID']) ]\""), "Prepare step must pass the PR id (coalesced with synthPr fallback) through to the bundle" ); assert!( - compiled.contains("SYSTEM_PULLREQUEST_TARGETBRANCH: $[ coalesce(variables['System.PullRequest.TargetBranch'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_TARGETBRANCH']) ]"), + compiled.contains("SYSTEM_PULLREQUEST_TARGETBRANCH: \"$[ coalesce(variables['System.PullRequest.TargetBranch'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_TARGETBRANCH']) ]\""), "Prepare step must pass the PR target branch (coalesced with synthPr fallback) through to the bundle" ); assert!( From c0ad1e100f3201dec50c4d5a35f74cbea53c553d Mon Sep 17 00:00:00 2001 From: James Devine Date: Tue, 9 Jun 2026 14:15:33 +0100 Subject: [PATCH 18/19] fix: address Rust PR Reviewer round-5 suggestions (#922) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three minor suggestions from the latest review. (1) `ado-client.ts::listActivePullRequestsBySourceRef` — added `?? []` guard on the SDK call. azure-devops-node-api's `getPullRequests` can return `null` instead of `[]` on empty REST bodies; the bundle's `.filter(...)` on the result would have thrown at runtime. Matches the established pattern (`getIterationChanges` uses `result.changeEntries ?? []`). (2) `exec_context/pr.rs` — added a `#[cfg(test)] mod tests` block with two targeted unit tests pinning the emitted YAML for `PrContextContributor::prepare_step`: * `prepare_step_synth_active_emits_coalesced_env_and_broadened_condition` — asserts the synth-mode env coalesces (PR id + target branch, YAML double-quoted runtime expressions) and the broadened `or(eq(Build.Reason,'PullRequest'), eq(synthPr.AW_SYNTHETIC_PR, 'true'))` condition. * `prepare_step_synth_inactive_emits_plain_macros_and_narrow_condition` — asserts plain `$(System.PullRequest.*)` macros + narrow `eq(Build.Reason,'PullRequest')` condition + defensive "no synthPr references in synth-inactive output". Previously these were only covered indirectly via the `synthetic-pr-default.md` snapshot fixture. (3) `types.rs::SanitizeConfigTrait for PrTriggerConfig` — added a one-line comment explaining why `mode` is intentionally absent from `sanitize_config_fields` (it's a `Copy` enum with no string content and any malformed input is rejected at deserialisation). Validation: - `cargo build` clean - `cargo test` 1813 lib + 131 compiler-integration + others, 0 failures - `cargo clippy --all-targets --all-features` clean - `npm test` in scripts/ado-script/ — 281/281 vitest passes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- scripts/ado-script/src/shared/ado-client.ts | 14 ++- src/compile/extensions/exec_context/pr.rs | 94 +++++++++++++++++++++ src/compile/types.rs | 4 + 3 files changed, 108 insertions(+), 4 deletions(-) diff --git a/scripts/ado-script/src/shared/ado-client.ts b/scripts/ado-script/src/shared/ado-client.ts index d4356bb3..07ac0c3d 100644 --- a/scripts/ado-script/src/shared/ado-client.ts +++ b/scripts/ado-script/src/shared/ado-client.ts @@ -108,6 +108,10 @@ export async function getPullRequestById( * synth contract requires *exactly one* match — a source branch with * >100 simultaneous active PRs against it is pathological and the * bundle will skip via the "multi-match" path anyway. + * + * The `?? []` guard handles the SDK's habit of returning `null` for + * empty result bodies on some REST responses; callers iterate / filter + * the result, so an empty array is the only safe contract. */ export async function listActivePullRequestsBySourceRef( project: string, @@ -116,10 +120,12 @@ export async function listActivePullRequestsBySourceRef( ): Promise { return withRetry("listActivePullRequestsBySourceRef", async () => { const git = await (await getWebApi()).getGitApi(); - return git.getPullRequests( - repoId, - { sourceRefName, status: PullRequestStatus.Active }, - project, + return ( + (await git.getPullRequests( + repoId, + { sourceRefName, status: PullRequestStatus.Active }, + project, + )) ?? [] ); }); } diff --git a/src/compile/extensions/exec_context/pr.rs b/src/compile/extensions/exec_context/pr.rs index 594a5ccb..ddff42fb 100644 --- a/src/compile/extensions/exec_context/pr.rs +++ b/src/compile/extensions/exec_context/pr.rs @@ -196,3 +196,97 @@ impl ContextContributor for PrContextContributor { ] } } + +#[cfg(test)] +mod tests { + //! Direct unit tests for `PrContextContributor::prepare_step` — + //! pins both the `mode: synthetic` (default) and `mode: policy` + //! emitted YAML shapes for the env-coalesce macros and the + //! step-level `condition:`. Catches accidental regressions of the + //! coalesce wiring without round-tripping through a full snapshot + //! fixture. + use super::*; + use crate::compile::extensions::CompileContext; + use crate::compile::types::{FrontMatter, PrContextConfig}; + + fn parse_fm(src: &str) -> FrontMatter { + let (fm, _) = crate::compile::common::parse_markdown(src).unwrap(); + fm + } + + fn pr_fm() -> FrontMatter { + parse_fm( + "---\nname: test\ndescription: test\non:\n pr:\n branches:\n include: [main]\n---\n", + ) + } + + #[test] + fn prepare_step_synth_active_emits_coalesced_env_and_broadened_condition() { + let contributor = PrContextContributor::new(PrContextConfig::default(), true); + let fm = pr_fm(); + let ctx = CompileContext::for_test(&fm); + let step = contributor.prepare_step(&ctx); + + // Env: PR id + target branch are coalesced via cross-job runtime + // expressions wrapped in YAML double quotes (Agent job depends + // on Setup, so `dependencies.Setup.outputs[...]` is the correct + // form here — distinct from the gate step which is same-job). + assert!( + step.contains( + "SYSTEM_PULLREQUEST_PULLREQUESTID: \"$[ coalesce(variables['System.PullRequest.PullRequestId'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_ID']) ]\"" + ), + "synth-active prepare step must coalesce PR id with synthPr fallback: {step}" + ); + assert!( + step.contains( + "SYSTEM_PULLREQUEST_TARGETBRANCH: \"$[ coalesce(variables['System.PullRequest.TargetBranch'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_TARGETBRANCH']) ]\"" + ), + "synth-active prepare step must coalesce target branch with synthPr fallback: {step}" + ); + + // Condition: broadened to accept real PR builds OR synth-promoted + // CI builds. + assert!( + step.contains( + "condition: or(eq(variables['Build.Reason'], 'PullRequest'), eq(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], 'true'))" + ), + "synth-active prepare step must broaden the condition to accept synth-promoted builds: {step}" + ); + } + + #[test] + fn prepare_step_synth_inactive_emits_plain_macros_and_narrow_condition() { + let contributor = PrContextContributor::new(PrContextConfig::default(), false); + let fm = pr_fm(); + let ctx = CompileContext::for_test(&fm); + let step = contributor.prepare_step(&ctx); + + // Env: plain `$(...)` macros for the real System.PullRequest.* + // predefined variables — no coalesce, no quoting. + assert!( + step.contains("SYSTEM_PULLREQUEST_PULLREQUESTID: $(System.PullRequest.PullRequestId)"), + "synth-inactive prepare step must use the plain ADO macro form: {step}" + ); + assert!( + step.contains("SYSTEM_PULLREQUEST_TARGETBRANCH: $(System.PullRequest.TargetBranch)"), + "synth-inactive prepare step must use the plain ADO macro form: {step}" + ); + + // Condition: narrow to real PR builds only. + assert!( + step.contains("condition: eq(variables['Build.Reason'], 'PullRequest')"), + "synth-inactive prepare step must keep the narrow PR-build condition: {step}" + ); + + // Defensive: the synth-mode signature MUST NOT appear when the + // synth path is inactive. + assert!( + !step.contains("synthPr.AW_SYNTHETIC_PR"), + "synth-inactive prepare step must not reference any synthPr Setup-job output: {step}" + ); + assert!( + !step.contains("coalesce("), + "synth-inactive prepare step must not emit a coalesce expression: {step}" + ); + } +} diff --git a/src/compile/types.rs b/src/compile/types.rs index bd71818c..b789b087 100644 --- a/src/compile/types.rs +++ b/src/compile/types.rs @@ -1302,6 +1302,10 @@ pub enum PrMode { impl SanitizeConfigTrait for PrTriggerConfig { fn sanitize_config_fields(&mut self) { + // `mode` (PrMode enum, `Copy`) has no string content to + // sanitize — it's a closed kebab-case-deserialised enum, so + // any malformed input is already rejected at deserialisation + // time. Intentionally absent here. if let Some(ref mut b) = self.branches { b.sanitize_config_fields(); } From 04a033615b7850f0dec3b3e39ebdf73f4d28070b Mon Sep 17 00:00:00 2001 From: James Devine Date: Tue, 9 Jun 2026 14:34:14 +0100 Subject: [PATCH 19/19] fix: address Rust PR Reviewer round-6 suggestions (#922) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three items from the latest review. (1) Stale test name (`tests/compiler_tests.rs` ~L4239) — `test_exec_context_pr_only_downloads_bundle_in_agent_job_not_setup` was the original intent, but the body was later updated to assert the Setup job DOES carry the bundle download (the synthPr step is a Setup-job bundle consumer). Renamed to `test_exec_context_pr_downloads_bundle_in_both_jobs_with_synth_mode` and updated the doc-comment to describe the two consumers. (2) `is_synthetic_pr()` helper on `FrontMatter` — `front_matter.pr_trigger().is_some_and(|p| matches!(p.mode, PrMode::Synthetic))` was duplicated verbatim in three places (`compile_shared`, `ExecContextExtension::new`, `collect_extensions`). Extracted to `FrontMatter::is_synthetic_pr()` in `src/compile/types.rs` so a future `PrMode` variant can't drift across the three sites. All three call sites now use the helper. (3) Truncated doc-comment on `PrTriggerConfig.mode` — the previous text ("Whether to synthesise PullRequest semantics on CI builds when an / PR-trigger mode. Drives whether ...") read as a cut-off sentence. Rewrote to "Determines how `on.pr` builds reach the pipeline; see [`PrMode`] for the two supported strategies (`synthetic`, `policy`). Defaults to [`PrMode::Synthetic`]." Validation: - `cargo build` clean - `cargo test` 1813 lib + 131 compiler-integration + others, 0 failures - `cargo clippy --all-targets --all-features` clean Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/common.rs | 4 +--- src/compile/extensions/exec_context/mod.rs | 4 +--- src/compile/extensions/mod.rs | 13 +++++++++---- src/compile/types.rs | 20 ++++++++++++++------ tests/compiler_tests.rs | 12 ++++++++---- 5 files changed, 33 insertions(+), 20 deletions(-) diff --git a/src/compile/common.rs b/src/compile/common.rs index 2dc06cfd..965c90df 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -3373,9 +3373,7 @@ pub async fn compile_shared( } } - let synthetic_pr_active = front_matter - .pr_trigger() - .is_some_and(|p| matches!(p.mode, crate::compile::types::PrMode::Synthetic)); + let synthetic_pr_active = front_matter.is_synthetic_pr(); let agentic_depends_on = generate_agentic_depends_on( &front_matter.setup, has_pr_filters, diff --git a/src/compile/extensions/exec_context/mod.rs b/src/compile/extensions/exec_context/mod.rs index e41a118a..b5c19c46 100644 --- a/src/compile/extensions/exec_context/mod.rs +++ b/src/compile/extensions/exec_context/mod.rs @@ -124,9 +124,7 @@ impl ExecContextExtension { // from `front_matter.execution_context`) still see the right // activation answer. let any_contributor_active = pr_contributor_will_activate_with_cfg(&config, front_matter); - let synthetic_pr_active = front_matter - .pr_trigger() - .is_some_and(|p| matches!(p.mode, crate::compile::types::PrMode::Synthetic)); + let synthetic_pr_active = front_matter.is_synthetic_pr(); Self { config, any_contributor_active, diff --git a/src/compile/extensions/mod.rs b/src/compile/extensions/mod.rs index 0aca3964..c56e784e 100644 --- a/src/compile/extensions/mod.rs +++ b/src/compile/extensions/mod.rs @@ -707,10 +707,15 @@ pub fn collect_extensions(front_matter: &FrontMatter) -> Vec { // the synthPr Setup-job step and downstream wiring; when // `None` it doesn't. The previous separate `bool` flag is // now derived via `AdoScriptExtension::synthetic_pr_active()`. - let pr_cfg = front_matter.pr_trigger(); - let pr_trigger_for_synth = pr_cfg - .filter(|p| matches!(p.mode, crate::compile::types::PrMode::Synthetic)) - .cloned(); + // The activation predicate (`mode == Synthetic`) lives in + // `FrontMatter::is_synthetic_pr()` so it stays in lock-step + // with the other two call sites (`compile_shared` and + // `ExecContextExtension::new`). + let pr_trigger_for_synth = if front_matter.is_synthetic_pr() { + front_matter.pr_trigger().cloned() + } else { + None + }; AdoScriptExtension { pr_filters: front_matter.pr_filters().cloned(), pipeline_filters: front_matter.pipeline_filters().cloned(), diff --git a/src/compile/types.rs b/src/compile/types.rs index b789b087..6b79ae9d 100644 --- a/src/compile/types.rs +++ b/src/compile/types.rs @@ -749,6 +749,17 @@ impl FrontMatter { self.on_config.as_ref().and_then(|o| o.pr.as_ref()) } + /// Whether the synthetic-from-ci path is active for this agent — + /// i.e. `on.pr` is configured AND `on.pr.mode == PrMode::Synthetic` + /// (the default). Centralised here so the three compile-time call + /// sites (`collect_extensions`, `ExecContextExtension::new`, + /// `compile_shared`) cannot drift on the predicate if a future + /// `PrMode` variant is added. + pub fn is_synthetic_pr(&self) -> bool { + self.pr_trigger() + .is_some_and(|p| matches!(p.mode, PrMode::Synthetic)) + } + /// Get the PR runtime filters (if any). pub fn pr_filters(&self) -> Option<&PrFilters> { self.pr_trigger().and_then(|pr| pr.filters.as_ref()) @@ -1256,12 +1267,9 @@ pub struct PrTriggerConfig { /// Runtime filters evaluated via gate steps in the Setup job #[serde(default)] pub filters: Option, - /// Whether to synthesise PullRequest semantics on CI builds when an - /// PR-trigger mode. Drives whether the compiler emits the - /// synthetic-from-ci Setup-job step (`mode: synthetic`, default) or - /// steps back and expects an operator-installed Build Validation - /// branch policy to drive PR builds (`mode: policy`). See - /// [`PrMode`] for the two values. + /// Determines how `on.pr` builds reach the pipeline; see + /// [`PrMode`] for the two supported strategies (`synthetic`, + /// `policy`). Defaults to [`PrMode::Synthetic`]. #[serde(default, rename = "mode")] pub mode: PrMode, } diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index 9f03a25d..06d51930 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -4229,14 +4229,18 @@ fn test_neither_feature_active_emits_no_node_or_download_anywhere() { } /// Per-job download placement: when the gate is inactive AND runtime imports -/// are inlined, but `on.pr` is configured and execution-context PR is not -/// disabled, the `exec-context-pr.js` bundle is the only consumer — the -/// download must land in the Agent job only. +/// are inlined, but `on.pr` is configured (default `mode: synthetic`) and +/// execution-context PR is not disabled, two bundle consumers are active: +/// +/// * `synthPr` (Setup job) — emitted by the synthetic-from-ci path +/// * `exec-context-pr.js` (Agent job) — staged by the PR contributor +/// +/// so the script bundle download MUST land in BOTH jobs. /// /// Closes a coverage gap that `dedupe_gate_only.md` previously left by /// pinning `execution-context.pr.enabled: false`. #[test] -fn test_exec_context_pr_only_downloads_bundle_in_agent_job_not_setup() { +fn test_exec_context_pr_downloads_bundle_in_both_jobs_with_synth_mode() { let yaml = compile_fixture("dedupe_exec_context_pr_only.md"); let agent = extract_job_block(&yaml, "Agent").expect("Agent job should exist"); assert!(