diff --git a/docs/ado-script.md b/docs/ado-script.md index 5175748c..40e6e6ec 100644 --- a/docs/ado-script.md +++ b/docs/ado-script.md @@ -273,26 +273,62 @@ non-zero: ### Env-var contract -The compiler injects these on the `node exec-context-pr-synth.js` -step; the predefined `SYSTEM_PULLREQUEST_*` variables are auto-mapped -into the process env by ADO at runtime: +The compiler injects only the non-auto-injected `SYSTEM_ACCESSTOKEN` bearer +(via the shared bundle-auth applier — see [Bundle env contract](#bundle-env-contract)) +plus the computed `PR_SYNTH_SPEC` on the `node exec-context-pr-synth.js` step. +Every predefined `System.*` / `Build.*` variable the bundle reads +(`SYSTEM_TEAMPROJECT`, `BUILD_REPOSITORY_ID`, `BUILD_REASON`, +`BUILD_REPOSITORY_PROVIDER`, `BUILD_SOURCEBRANCH`, `SYSTEM_PULLREQUEST_*`) is +auto-mapped into the process env by ADO at runtime, so it is **not** +re-projected on the step: | Env var | Source | Purpose | |---|---|---| | `PR_SYNTH_SPEC` | compiled inline (base64) | The branch/path filter spec | -| `SYSTEM_ACCESSTOKEN` | `$(System.AccessToken)` | ADO REST auth | +| `SYSTEM_ACCESSTOKEN` | `$(System.AccessToken)` (bundle-auth applier) | ADO REST auth | | `SYSTEM_COLLECTIONURI` | ADO auto-injected | ADO org base URL (read via `getWebApi()`; falls back to `SYSTEM_TEAMFOUNDATIONCOLLECTIONURI`) | -| `ADO_PROJECT` | `$(System.TeamProject)` | ADO project for the PR lookup | -| `ADO_REPO_ID` | `$(Build.Repository.ID)` | Repository id for the PR lookup | -| `BUILD_REASON` | `$(Build.Reason)` | Distinguishes CI from PR builds | -| `BUILD_REPOSITORY_PROVIDER` | `$(Build.Repository.Provider)` | Detects GitHub-typed repos | -| `BUILD_SOURCEBRANCH` | `$(Build.SourceBranch)` | Source ref matched against active PRs | -| `SYSTEM_PULLREQUEST_*` | ADO-injected | Real-PR identifiers propagated verbatim | +| `SYSTEM_TEAMPROJECT` | ADO auto-injected | ADO project for the PR lookup | +| `BUILD_REPOSITORY_ID` | ADO auto-injected | Repository id for the PR lookup | +| `BUILD_REASON` | ADO auto-injected | Distinguishes CI from PR builds | +| `BUILD_REPOSITORY_PROVIDER` | ADO auto-injected | Detects GitHub-typed repos | +| `BUILD_SOURCEBRANCH` | ADO auto-injected | Source ref matched against active PRs | +| `SYSTEM_PULLREQUEST_*` | ADO auto-injected | Real-PR identifiers read verbatim | The bundle lazy-imports `azure-devops-node-api` only when it needs to call the PR REST endpoints (steps 4 and 7); real PR builds and GitHub-typed repos return before any SDK load. +## Bundle env contract + +Every compiler-emitted step that runs an ado-script bundle has an implicit +environment contract — which `process.env` keys the bundle reads. That contract +is modelled in [`src/compile/ado_bundle.rs`](../src/compile/ado_bundle.rs): + +- **`Bundle`** enumerates every bundle with its on-disk `path()` and its + `auth()` requirement (`BundleAuth::Bearer` for bundles that read + `SYSTEM_ACCESSTOKEN` — for ADO REST via `getWebApi()` and/or git bearer auth + via `bearerEnv` — `BundleAuth::None` otherwise). +- **`apply_bundle_auth(step, bundle, token)`** is the single chokepoint that + projects `SYSTEM_ACCESSTOKEN` (from a `TokenSource`) into every + bearer-requiring bundle step. `SYSTEM_ACCESSTOKEN` is the one ADO predefined variable that is + **not** auto-injected — ADO maps it only when a step explicitly references + it — so it must be projected. This applier is why a step can no longer ship + without a bearer (the regression behind #1307). +- **`token_source_for(write_service_connection)`** unifies the + `System.AccessToken` vs `SC_WRITE_TOKEN` selection shared by the Conclusion + job and the Stage 3 executor. + +Every other ADO predefined variable (`System.*`, `Build.*`) is auto-injected +into every script step's env under its SCREAMING_SNAKE name, so bundle steps +**must not** re-project them (e.g. `SYSTEM_TEAMPROJECT: $(System.TeamProject)` +is a redundant mirror). `is_redundant_ado_mirror` identifies such entries and +the contract tests — plus the compiled-YAML churn guard in +`tests/compiler_tests.rs` — assert migrated steps do not emit them. The ADO +collection URI is likewise read from the auto-injected `SYSTEM_COLLECTIONURI` +(`scripts/ado-script/src/shared/auth.ts::getWebApi`, see #1307), so it +is never part of a step's env contract. + + ## End-to-end data flow ``` diff --git a/docs/extending.md b/docs/extending.md index 246c0587..789352bb 100644 --- a/docs/extending.md +++ b/docs/extending.md @@ -115,7 +115,25 @@ let step = Step::Bash( `BashStep::script` is the raw bash body. Do not include `- bash: |` or YAML indentation; the lowerer and serializer own YAML formatting. -### Task steps +### Steps that run an ado-script bundle + +If your step invokes an ado-script Node bundle (`/tmp/ado-aw-scripts/ado-script/*.js`), +model its env contract via [`src/compile/ado_bundle.rs`](../src/compile/ado_bundle.rs) +rather than hand-writing the auth env: + +- Add a `Bundle` variant with its `path()` and `auth()` (`BundleAuth::Bearer` + if the bundle reads `SYSTEM_ACCESSTOKEN` — for ADO REST via `getWebApi()` + and/or git bearer auth — else `BundleAuth::None`). +- Project the bearer with `apply_bundle_auth(step, Bundle::X, token)` — never + `.with_env("SYSTEM_ACCESSTOKEN", …)` by hand. Use `token_source_for(write_sc)` + to pick the token source. +- Do **not** re-project ADO predefined variables (`System.*` / `Build.*`): ADO + auto-injects them into every script step's env under their SCREAMING_SNAKE + names, so the bundle reads them directly. Only genuinely computed inputs + (base64 specs, `AW_*` config, `PARAM_*`) belong in `.with_env`. The contract + tests and the churn guard in `tests/compiler_tests.rs` enforce this. + + ```rust use crate::compile::ir::step::Step; 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 index f8ac6116..aae98ee9 100644 --- a/scripts/ado-script/src/exec-context-pr-synth/__tests__/harness.ts +++ b/scripts/ado-script/src/exec-context-pr-synth/__tests__/harness.ts @@ -41,8 +41,8 @@ export function makeEnv(overrides: Record): NodeJS.ProcessEnv { BUILD_REASON: "IndividualCI", BUILD_REPOSITORY_PROVIDER: "TfsGit", BUILD_SOURCEBRANCH: "refs/heads/feature/x", - ADO_PROJECT: "MyProject", - ADO_REPO_ID: "00000000-0000-0000-0000-000000000000", + SYSTEM_TEAMPROJECT: "MyProject", + BUILD_REPOSITORY_ID: "00000000-0000-0000-0000-000000000000", ...overrides, }; } 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 2ae3e95e..ba8806c0 100644 --- a/scripts/ado-script/src/exec-context-pr-synth/index.ts +++ b/scripts/ado-script/src/exec-context-pr-synth/index.ts @@ -180,11 +180,11 @@ export async function main(env: NodeJS.ProcessEnv = process.env): Promise.js`) has an implicit environment +//! contract: which `process.env` keys the bundle reads. Historically each +//! call site hand-wrote that step's `env:` block, which is how the Conclusion +//! step drifted and shipped without an ADO bearer token (issue #1307 — the +//! shared `getWebApi()` auth helper had no `SYSTEM_ACCESSTOKEN`). +//! +//! This module makes the **auth** half of that contract explicit and +//! enforceable: +//! +//! * [`Bundle`] enumerates every bundle, with its on-disk [`Bundle::path`] and +//! its [`Bundle::auth`] requirement. +//! * [`apply_bundle_auth`] is the single chokepoint that projects +//! `SYSTEM_ACCESSTOKEN` into a step for every bearer-requiring bundle, so no +//! call site can forget it again. +//! * [`token_source_for`] unifies the `System.AccessToken` vs `SC_WRITE_TOKEN` +//! selection that was previously duplicated between the Conclusion job and +//! the Stage 3 executor. +//! +//! ## What is *not* modelled here +//! +//! The ADO collection URI is read from the **auto-injected** +//! `SYSTEM_COLLECTIONURI` (see `scripts/ado-script/src/shared/auth.ts` and +//! #1307), so it is not part of any step's env contract. Likewise every ADO +//! predefined variable (`System.*`, `Build.*`) is auto-injected into every +//! script step's env under its SCREAMING_SNAKE name, so bundle steps must not +//! re-project them — [`is_redundant_ado_mirror`] identifies such redundant +//! mirrors and the contract test asserts migrated steps do not emit them. +//! `SYSTEM_ACCESSTOKEN` is the one documented exception: ADO maps it only when +//! a step explicitly references it, which is exactly what [`apply_bundle_auth`] +//! does. + +use crate::compile::extensions::ado_script as paths; +use crate::compile::ir::env::EnvValue; +use crate::compile::ir::step::BashStep; + +/// An ado-script Node bundle shipped in `ado-script.zip` and unpacked to +/// `/tmp/ado-aw-scripts/ado-script/.js` at pipeline runtime. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Bundle { + Gate, + Import, + ExecContextPr, + ExecContextPrSynth, + ExecContextPrChecks, + ExecContextPipeline, + ExecContextCiPush, + ExecContextWorkitem, + ExecContextSchedule, + ExecContextManual, + ExecContextRepo, + ApprovalSummary, + Conclusion, +} + +/// The auth contract a bundle requires from the step that invokes it. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum BundleAuth { + /// The bundle reads `SYSTEM_ACCESSTOKEN` from its env — for ADO REST auth + /// (via `getWebApi()` in `scripts/ado-script/src/shared/auth.ts`), for git + /// credential/bearer auth (via `shared/git.ts::bearerEnv`), or both — so + /// its step must project the token. `SYSTEM_ACCESSTOKEN` is the one ADO + /// predefined variable that is *not* auto-injected (ADO maps it only on + /// explicit reference), which is why it must be projected. The collection + /// URI, by contrast, comes from the auto-injected `SYSTEM_COLLECTIONURI` + /// and is therefore *not* part of this contract (see #1307). + Bearer, + /// The bundle needs no bearer (pure filesystem / git-without-auth / argv). + None, +} + +/// Which pipeline secret variable supplies the ADO bearer token for a step. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum TokenSource { + /// The pipeline's built-in OAuth token (`$(System.AccessToken)`), scoped + /// by the pipeline's job-authorization settings. + SystemAccessToken, + /// A write-capable ADO token minted from an ARM service connection into + /// the `SC_WRITE_TOKEN` pipeline variable (Stage 3 executor / Conclusion). + WriteServiceConnection, +} + +impl TokenSource { + /// The bare pipeline-variable name backing this token source. Lowering + /// wraps it as `$()`, so `SystemAccessToken` produces the same wire + /// form (`$(System.AccessToken)`) that the hand-written call sites emitted + /// before this module existed — no YAML churn on the token line. + pub fn variable(self) -> &'static str { + match self { + TokenSource::SystemAccessToken => "System.AccessToken", + TokenSource::WriteServiceConnection => "SC_WRITE_TOKEN", + } + } +} + +/// Select the token source for a step given the optional write service +/// connection. When a write SC is configured the compiler mints +/// `SC_WRITE_TOKEN` upstream; otherwise the built-in `$(System.AccessToken)` +/// is used. Shared by the Conclusion job and the Stage 3 executor so the two +/// paths cannot disagree. +pub fn token_source_for(write_service_connection: Option<&str>) -> TokenSource { + if write_service_connection.is_some() { + TokenSource::WriteServiceConnection + } else { + TokenSource::SystemAccessToken + } +} + +impl Bundle { + /// Every bundle, for registry-wide iteration. Consumed by the contract + /// tests in this module. `ALL` is compiled in all builds (not `cfg(test)`) + /// so that every `Bundle` variant is constructed here — that keeps the + /// `None`-auth variants, which production only ever *matches* on (never + /// constructs), from tripping `dead_code`. `ALL` itself is unused outside + /// tests, hence `#[allow(dead_code)]`. + #[allow(dead_code)] + pub const ALL: &'static [Bundle] = &[ + Bundle::Gate, + Bundle::Import, + Bundle::ExecContextPr, + Bundle::ExecContextPrSynth, + Bundle::ExecContextPrChecks, + Bundle::ExecContextPipeline, + Bundle::ExecContextCiPush, + Bundle::ExecContextWorkitem, + Bundle::ExecContextSchedule, + Bundle::ExecContextManual, + Bundle::ExecContextRepo, + Bundle::ApprovalSummary, + Bundle::Conclusion, + ]; + + /// The bundle's unpacked on-disk path inside the runtime VM. The Conclusion + /// bundle is referenced inline by `agentic_pipeline` (not via a shared + /// const), so its literal path is repeated here as the single registry + /// source of truth. Consumed by the contract tests in this module + /// (production references the path constants directly); `#[allow(dead_code)]` + /// rather than `#[cfg(test)]` so `paths` stays a normal import. + #[allow(dead_code)] + pub fn path(self) -> &'static str { + match self { + Bundle::Gate => paths::GATE_EVAL_PATH, + Bundle::Import => paths::IMPORT_EVAL_PATH, + Bundle::ExecContextPr => paths::EXEC_CONTEXT_PR_PATH, + Bundle::ExecContextPrSynth => paths::EXEC_CONTEXT_PR_SYNTH_PATH, + Bundle::ExecContextPrChecks => paths::EXEC_CONTEXT_PR_CHECKS_PATH, + Bundle::ExecContextPipeline => paths::EXEC_CONTEXT_PIPELINE_PATH, + Bundle::ExecContextCiPush => paths::EXEC_CONTEXT_CI_PUSH_PATH, + Bundle::ExecContextWorkitem => paths::EXEC_CONTEXT_WORKITEM_PATH, + Bundle::ExecContextSchedule => paths::EXEC_CONTEXT_SCHEDULE_PATH, + Bundle::ExecContextManual => paths::EXEC_CONTEXT_MANUAL_PATH, + Bundle::ExecContextRepo => paths::EXEC_CONTEXT_REPO_PATH, + Bundle::ApprovalSummary => paths::APPROVAL_SUMMARY_PATH, + Bundle::Conclusion => paths::CONCLUSION_PATH, + } + } + + /// The auth contract this bundle requires from its invoking step. + pub fn auth(self) -> BundleAuth { + match self { + // These bundles read SYSTEM_ACCESSTOKEN at runtime — for ADO REST + // (via getWebApi()) and/or git bearer auth (via bearerEnv). + Bundle::Gate + | Bundle::ExecContextPr + | Bundle::ExecContextPrSynth + | Bundle::ExecContextPrChecks + | Bundle::ExecContextPipeline + | Bundle::ExecContextCiPush + | Bundle::ExecContextWorkitem + | Bundle::ExecContextSchedule + | Bundle::Conclusion => BundleAuth::Bearer, + // Pure filesystem / git-without-auth / argv — no bearer. + Bundle::Import + | Bundle::ExecContextManual + | Bundle::ExecContextRepo + | Bundle::ApprovalSummary => BundleAuth::None, + } + } +} + +/// Project the bundle's auth env contract onto a step. +/// +/// For [`BundleAuth::Bearer`] bundles this maps `SYSTEM_ACCESSTOKEN` from the +/// chosen [`TokenSource`]. For [`BundleAuth::None`] it is a no-op — the `token` +/// argument is ignored (a `None`-auth bundle needs no bearer, and today no +/// caller routes such a bundle through this function). This is the single +/// guarantee that every bearer-requiring bundle step carries a token — the +/// structural fix for the class of bug behind #1307. +pub fn apply_bundle_auth(step: BashStep, bundle: Bundle, token: TokenSource) -> BashStep { + match bundle.auth() { + BundleAuth::Bearer => { + step.with_env("SYSTEM_ACCESSTOKEN", EnvValue::secret(token.variable())) + } + BundleAuth::None => step, + } +} + +/// True iff `(key, value)` is a redundant re-projection of an ADO predefined +/// variable that the runtime already auto-injects into every script step's env +/// under the SCREAMING_SNAKE form of its dotted name +/// (e.g. `SYSTEM_TEAMPROJECT: $(System.TeamProject)`). +/// +/// The auth token is deliberately projected as [`EnvValue::Secret`] (not +/// [`EnvValue::AdoMacro`]) by [`apply_bundle_auth`], so it is never flagged +/// here even though `System.AccessToken` shares the `SYSTEM_ACCESSTOKEN` name — +/// that projection is intentional (ADO maps the token only on explicit +/// reference). +/// +/// Note: some auto-injected vars are still projected deliberately even though +/// this function *would* flag them — e.g. the manual contributor's +/// `BUILD_REQUESTEDFOR` / `BUILD_REQUESTEDFOREMAIL` requestor-identity vars, +/// which are retained to keep the email-hygiene opt-in visible at the call +/// site. Such exemptions live in an explicit allowlist at the (test) call site +/// rather than being special-cased here, so this predicate stays a pure +/// "does the key mirror the macro" check. +/// +/// Consumed by the contract tests in this module; the compiled-YAML churn +/// guard in `tests/compiler_tests.rs` re-implements the same rule at the +/// integration level. Test-only, so compiled only under `cfg(test)`. +#[cfg(test)] +pub fn is_redundant_ado_mirror(key: &str, value: &EnvValue) -> bool { + match value { + EnvValue::AdoMacro(name) => key == name.replace('.', "_").to_uppercase(), + _ => false, + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn every_bundle_path_is_under_the_unpack_dir() { + for b in Bundle::ALL { + assert!( + b.path() + .starts_with("/tmp/ado-aw-scripts/ado-script/"), + "{b:?} path must live under the unzip destination" + ); + assert!(b.path().ends_with(".js"), "{b:?} path must be a .js bundle"); + } + } + + #[test] + fn apply_bundle_auth_projects_token_for_bearer_bundles_only() { + for b in Bundle::ALL { + let step = BashStep::new("t", "node x\n"); + let out = apply_bundle_auth(step, *b, TokenSource::SystemAccessToken); + let has_token = out.env.contains_key("SYSTEM_ACCESSTOKEN"); + match b.auth() { + BundleAuth::Bearer => assert!( + has_token, + "{b:?} is Bearer and must carry SYSTEM_ACCESSTOKEN" + ), + BundleAuth::None => assert!( + !has_token, + "{b:?} is None and must not carry SYSTEM_ACCESSTOKEN" + ), + } + } + } + + #[test] + fn token_source_selection_matches_write_service_connection() { + assert_eq!(token_source_for(None), TokenSource::SystemAccessToken); + assert_eq!(token_source_for(None).variable(), "System.AccessToken"); + assert_eq!( + token_source_for(Some("my-sc")), + TokenSource::WriteServiceConnection + ); + assert_eq!( + token_source_for(Some("my-sc")).variable(), + "SC_WRITE_TOKEN" + ); + } + + #[test] + fn projected_auth_token_is_not_flagged_as_a_mirror() { + // Secret-sourced token must never be mistaken for a redundant mirror, + // even though System.AccessToken -> SYSTEM_ACCESSTOKEN name-matches. + let token = EnvValue::secret(TokenSource::SystemAccessToken.variable()); + assert!(!is_redundant_ado_mirror("SYSTEM_ACCESSTOKEN", &token)); + } + + #[test] + fn detects_redundant_and_genuine_env() { + // Redundant mirror of an auto-injected predefined variable. + assert!(is_redundant_ado_mirror( + "SYSTEM_TEAMPROJECT", + &EnvValue::ado_macro("System.TeamProject").unwrap() + )); + assert!(is_redundant_ado_mirror( + "BUILD_SOURCESDIRECTORY", + &EnvValue::ado_macro("Build.SourcesDirectory").unwrap() + )); + // Genuine computed input (literal) is never a mirror. + assert!(!is_redundant_ado_mirror( + "GATE_SPEC", + &EnvValue::literal("base64") + )); + // A pipeline-var override (synth PR id) is never a mirror. + assert!(!is_redundant_ado_mirror( + "SYSTEM_PULLREQUEST_PULLREQUESTID", + &EnvValue::pipeline_var("AW_PR_ID") + )); + // A macro projected under a *different* key is not a self-mirror. + assert!(!is_redundant_ado_mirror( + "AW_PR_ID", + &EnvValue::ado_macro("System.PullRequest.PullRequestId").unwrap() + )); + } +} diff --git a/src/compile/agentic_pipeline.rs b/src/compile/agentic_pipeline.rs index d4d44317..c1c7d774 100644 --- a/src/compile/agentic_pipeline.rs +++ b/src/compile/agentic_pipeline.rs @@ -1508,6 +1508,7 @@ fn build_conclusion_job( cfg: &StandaloneCtx, prefix: &JobPrefix<'_>, ) -> Result> { + use crate::compile::ado_bundle::{Bundle, apply_bundle_auth, token_source_for}; // Conclusion job is always emitted when safe-outputs exist (gh-aw pattern). if front_matter.safe_outputs.is_empty() { return Ok(None); @@ -1536,12 +1537,15 @@ fn build_conclusion_job( download_artifact.continue_on_error = true; steps.push(Step::Task(download_artifact)); - let conclusion_script = "\ -if command -v node >/dev/null 2>&1 && [ -f /tmp/ado-aw-scripts/ado-script/conclusion.js ]; then\n \ - node /tmp/ado-aw-scripts/ado-script/conclusion.js\n\ + let conclusion_path = super::extensions::ado_script::CONCLUSION_PATH; + let conclusion_script = format!( + "\ +if command -v node >/dev/null 2>&1 && [ -f {conclusion_path} ]; then\n \ + node {conclusion_path}\n\ else\n \ echo \"##vso[task.logissue type=warning]conclusion.js unavailable; skipping conclusion reporting\"\n\ -fi\n"; +fi\n" + ); let mut conclusion_step = bash("Report pipeline conclusion", conclusion_script); conclusion_step = conclusion_step.with_condition(Condition::Always); // The Conclusion job's contract is "always runs, never fails": it exists to @@ -1577,21 +1581,19 @@ fi\n"; ); // Use SC_WRITE_TOKEN when a write service connection is configured; - // fall back to System.AccessToken otherwise. - let has_write_sc = front_matter + // fall back to System.AccessToken otherwise. The token source is selected + // by the shared `token_source_for` helper (same logic as the Stage 3 + // executor) and projected via the bundle-auth applier so the Conclusion + // step can never ship without a bearer (the regression that was #1307). + let write_sc = front_matter .permissions .as_ref() - .and_then(|p| p.write.as_ref()) - .is_some(); - if has_write_sc { - conclusion_step = conclusion_step.with_env( - "SYSTEM_ACCESSTOKEN", - EnvValue::secret("SC_WRITE_TOKEN"), - ); - } else { - conclusion_step = - conclusion_step.with_env("SYSTEM_ACCESSTOKEN", EnvValue::secret("System.AccessToken")); - } + .and_then(|p| p.write.as_deref()); + conclusion_step = apply_bundle_auth( + conclusion_step, + Bundle::Conclusion, + token_source_for(write_sc), + ); // Pass per-tool configs as individual flat env vars (gh-aw pattern). // Each field gets its own env var — avoids JSON-in-env-var corruption in ADO. diff --git a/src/compile/common.rs b/src/compile/common.rs index 5d611f94..c57fbdb6 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -1512,11 +1512,10 @@ pub fn generate_executor_ado_env( debug_create_issue_enabled: bool, ) -> String { let mut lines: Vec = Vec::new(); - if write_service_connection.is_some() { - lines.push("SYSTEM_ACCESSTOKEN: $(SC_WRITE_TOKEN)".to_string()); - } else { - lines.push("SYSTEM_ACCESSTOKEN: $(System.AccessToken)".to_string()); - } + // Select the ADO bearer via the shared `token_source_for` helper so the + // executor and the Conclusion job cannot disagree on the token source. + let token = crate::compile::ado_bundle::token_source_for(write_service_connection); + lines.push(format!("SYSTEM_ACCESSTOKEN: $({})", token.variable())); if debug_create_issue_enabled { lines.push("ADO_AW_DEBUG_GITHUB_TOKEN: $(ADO_AW_DEBUG_GITHUB_TOKEN)".to_string()); } diff --git a/src/compile/extensions/ado_script.rs b/src/compile/extensions/ado_script.rs index ee38252d..2acb433d 100644 --- a/src/compile/extensions/ado_script.rs +++ b/src/compile/extensions/ado_script.rs @@ -33,7 +33,7 @@ use crate::compile::ir::step::{BashStep, Step}; use crate::compile::ir::tasks::use_node::UseNode; use crate::compile::types::{PipelineFilters, PrFilters, SupplyChainConfig}; -const GATE_EVAL_PATH: &str = "/tmp/ado-aw-scripts/ado-script/gate.js"; +pub(crate) const GATE_EVAL_PATH: &str = "/tmp/ado-aw-scripts/ado-script/gate.js"; pub(crate) const IMPORT_EVAL_PATH: &str = "/tmp/ado-aw-scripts/ado-script/import.js"; /// Path to the exec-context-pr bundle inside the unpacked `ado-script.zip`. /// Consumed by `src/compile/extensions/exec_context/pr.rs` to invoke @@ -88,6 +88,12 @@ pub(crate) const EXEC_CONTEXT_PR_SYNTH_PATH: &str = /// safe outputs to a sanitized markdown summary tab. pub(crate) const APPROVAL_SUMMARY_PATH: &str = "/tmp/ado-aw-scripts/ado-script/approval-summary.js"; +/// Path to the conclusion bundle inside the unpacked `ado-script.zip`. Runs in +/// the always-on Conclusion job (see [`crate::compile::agentic_pipeline`]) to +/// file pipeline-failure work items and diagnostic signals. Referenced both by +/// that job's shell body and by `Bundle::Conclusion.path()` so the two copies +/// cannot diverge. +pub(crate) const CONCLUSION_PATH: &str = "/tmp/ado-aw-scripts/ado-script/conclusion.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. @@ -500,27 +506,17 @@ pub fn synthetic_pr_step_typed(spec_b64: &str) -> Result { for name in SYNTH_PR_OUTPUT_NAMES { step = step.with_output(OutputDecl::new(*name)); } - let envs: &[(&str, EnvValue)] = &[ - ( - "SYSTEM_ACCESSTOKEN", - EnvValue::ado_macro("System.AccessToken")?, - ), - ("ADO_PROJECT", EnvValue::ado_macro("System.TeamProject")?), - ("ADO_REPO_ID", EnvValue::ado_macro("Build.Repository.ID")?), - ("BUILD_REASON", EnvValue::ado_macro("Build.Reason")?), - ( - "BUILD_REPOSITORY_PROVIDER", - EnvValue::ado_macro("Build.Repository.Provider")?, - ), - ( - "BUILD_SOURCEBRANCH", - EnvValue::ado_macro("Build.SourceBranch")?, - ), - ("PR_SYNTH_SPEC", EnvValue::literal(spec_b64)), - ]; - for (k, v) in envs { - step = step.with_env(*k, v.clone()); - } + // ADO auto-injects the predefined context vars this bundle reads + // (SYSTEM_TEAMPROJECT, BUILD_REPOSITORY_ID, BUILD_REASON, + // BUILD_REPOSITORY_PROVIDER, BUILD_SOURCEBRANCH, SYSTEM_PULLREQUEST_*), + // so only the non-auto-injected SYSTEM_ACCESSTOKEN bearer and the + // compiler-computed PR_SYNTH_SPEC are projected here. + let step = crate::compile::ado_bundle::apply_bundle_auth( + step, + crate::compile::ado_bundle::Bundle::ExecContextPrSynth, + crate::compile::ado_bundle::TokenSource::SystemAccessToken, + ) + .with_env("PR_SYNTH_SPEC", EnvValue::literal(spec_b64)); Ok(step) } diff --git a/src/compile/extensions/exec_context/ci_push.rs b/src/compile/extensions/exec_context/ci_push.rs index 8868681f..092d56a9 100644 --- a/src/compile/extensions/exec_context/ci_push.rs +++ b/src/compile/extensions/exec_context/ci_push.rs @@ -46,9 +46,9 @@ //! - **REST failure** — Build API returned an error. Same. use crate::compile::extensions::CompileContext; +use crate::compile::ado_bundle::{Bundle, TokenSource, apply_bundle_auth}; use crate::compile::extensions::ado_script::EXEC_CONTEXT_CI_PUSH_PATH; use crate::compile::ir::condition::{Condition, Expr}; -use crate::compile::ir::env::EnvValue; use crate::compile::ir::step::{BashStep, Step}; use crate::compile::types::CiPushContextConfig; @@ -91,48 +91,26 @@ impl ContextContributor for CiPushContextContributor { return Ok(None); } let script = format!("set -euo pipefail\nnode '{EXEC_CONTEXT_CI_PUSH_PATH}'\n"); - let step = BashStep::new( - "Stage ci-push execution context (aw-context/ci-push/*)", - script, - ) - .with_condition(Condition::Or(vec![ - Condition::Eq( - Expr::Variable("Build.Reason".to_string()), - Expr::Literal("IndividualCI".to_string()), - ), - Condition::Eq( - Expr::Variable("Build.Reason".to_string()), - Expr::Literal("BatchedCI".to_string()), - ), - ])) - .with_env( - "SYSTEM_ACCESSTOKEN", - EnvValue::ado_macro("System.AccessToken")?, - ) - .with_env( - "SYSTEM_COLLECTIONURI", - EnvValue::ado_macro("System.CollectionUri")?, - ) - .with_env( - "SYSTEM_TEAMPROJECT", - EnvValue::ado_macro("System.TeamProject")?, - ) - .with_env( - "SYSTEM_DEFINITIONID", - EnvValue::ado_macro("System.DefinitionId")?, - ) - .with_env("BUILD_BUILDID", EnvValue::ado_macro("Build.BuildId")?) - .with_env( - "BUILD_SOURCESDIRECTORY", - EnvValue::ado_macro("Build.SourcesDirectory")?, - ) - .with_env( - "BUILD_SOURCEVERSION", - EnvValue::ado_macro("Build.SourceVersion")?, - ) - .with_env( - "BUILD_SOURCEBRANCH", - EnvValue::ado_macro("Build.SourceBranch")?, + // ADO auto-injects the predefined System.*/Build.* context variables + // into the step env, so the bundle reads them directly; only the + // non-auto-injected SYSTEM_ACCESSTOKEN bearer is projected here. + let step = apply_bundle_auth( + BashStep::new( + "Stage ci-push execution context (aw-context/ci-push/*)", + script, + ) + .with_condition(Condition::Or(vec![ + Condition::Eq( + Expr::Variable("Build.Reason".to_string()), + Expr::Literal("IndividualCI".to_string()), + ), + Condition::Eq( + Expr::Variable("Build.Reason".to_string()), + Expr::Literal("BatchedCI".to_string()), + ), + ])), + Bundle::ExecContextCiPush, + TokenSource::SystemAccessToken, ); Ok(Some(Step::Bash(step))) } @@ -156,6 +134,7 @@ impl ContextContributor for CiPushContextContributor { #[cfg(test)] mod tests { use super::*; + use crate::compile::ir::env::EnvValue; use crate::compile::extensions::CompileContext; use crate::compile::types::FrontMatter; @@ -216,24 +195,28 @@ mod tests { other => panic!("expected Or condition, got {other:?}"), } - // Trust boundary: bearer present. + // Trust boundary: bearer present, projected as a secret via the + // bundle-auth applier (not an AdoMacro). assert!(matches!( bash.env.get("SYSTEM_ACCESSTOKEN"), - Some(EnvValue::AdoMacro("System.AccessToken")) - )); - // Identifiers needed for the REST + git fetch. - assert!(matches!( - bash.env.get("SYSTEM_DEFINITIONID"), - Some(EnvValue::AdoMacro("System.DefinitionId")) - )); - assert!(matches!( - bash.env.get("BUILD_SOURCEVERSION"), - Some(EnvValue::AdoMacro("Build.SourceVersion")) - )); - assert!(matches!( - bash.env.get("BUILD_SOURCEBRANCH"), - Some(EnvValue::AdoMacro("Build.SourceBranch")) + Some(EnvValue::Secret(v)) if v == "System.AccessToken" )); + // All predefined System.*/Build.* context vars are auto-injected by + // ADO, so the step must not re-project them. + for stripped in [ + "SYSTEM_COLLECTIONURI", + "SYSTEM_TEAMPROJECT", + "SYSTEM_DEFINITIONID", + "BUILD_BUILDID", + "BUILD_SOURCESDIRECTORY", + "BUILD_SOURCEVERSION", + "BUILD_SOURCEBRANCH", + ] { + assert!( + bash.env.get(stripped).is_none(), + "{stripped} is auto-injected and must not be re-projected" + ); + } } #[test] diff --git a/src/compile/extensions/exec_context/manual.rs b/src/compile/extensions/exec_context/manual.rs index 3c5c24b8..4162dd94 100644 --- a/src/compile/extensions/exec_context/manual.rs +++ b/src/compile/extensions/exec_context/manual.rs @@ -143,6 +143,11 @@ impl ContextContributor for ManualContextContributor { } let script = format!("set -euo pipefail\nnode '{EXEC_CONTEXT_MANUAL_PATH}'\n"); + // BUILD_SOURCESDIRECTORY is auto-injected by ADO, so it is not + // re-projected. BUILD_REQUESTEDFOR / BUILD_REQUESTEDFOREMAIL are + // retained (identity vars behind the opt-in email hygiene gate — the + // projection makes the intent explicit at the call site). Manual has + // no bearer (BundleAuth::None). let mut step = BashStep::new( "Stage manual execution context (aw-context/manual/*)", script, @@ -154,10 +159,6 @@ impl ContextContributor for ManualContextContributor { .with_env( "BUILD_REQUESTEDFOR", EnvValue::ado_macro("Build.RequestedFor")?, - ) - .with_env( - "BUILD_SOURCESDIRECTORY", - EnvValue::ado_macro("Build.SourcesDirectory")?, ); // Email is opt-in for hygiene — see ManualContextConfig docs. diff --git a/src/compile/extensions/exec_context/pipeline.rs b/src/compile/extensions/exec_context/pipeline.rs index 23798edf..af827ce5 100644 --- a/src/compile/extensions/exec_context/pipeline.rs +++ b/src/compile/extensions/exec_context/pipeline.rs @@ -40,9 +40,9 @@ //! PR-author-controlled). use crate::compile::extensions::CompileContext; +use crate::compile::ado_bundle::{Bundle, TokenSource, apply_bundle_auth}; use crate::compile::extensions::ado_script::EXEC_CONTEXT_PIPELINE_PATH; use crate::compile::ir::condition::{Condition, Expr}; -use crate::compile::ir::env::EnvValue; use crate::compile::ir::step::{BashStep, Step}; use crate::compile::types::PipelineContextConfig; @@ -88,48 +88,22 @@ impl ContextContributor for PipelineContextContributor { return Ok(None); } let script = format!("set -euo pipefail\nnode '{EXEC_CONTEXT_PIPELINE_PATH}'\n"); - let step = BashStep::new( - "Stage pipeline execution context (aw-context/pipeline/*)", - script, - ) - .with_condition(Condition::Eq( - Expr::Variable("Build.Reason".to_string()), - Expr::Literal("ResourceTrigger".to_string()), - )) - .with_env( - "SYSTEM_ACCESSTOKEN", - EnvValue::ado_macro("System.AccessToken")?, - ) - .with_env( - "SYSTEM_COLLECTIONURI", - EnvValue::ado_macro("System.CollectionUri")?, - ) - .with_env( - "BUILD_SOURCESDIRECTORY", - EnvValue::ado_macro("Build.SourcesDirectory")?, - ) - // Upstream-build identifiers populated by ADO when the build - // was triggered via `resources.pipelines`. The bundle uses - // these to look up the upstream Build via the REST API. - // `Build.TriggeredBy.ProjectID` carries the project that owns - // the upstream pipeline (may differ from the consumer's - // project for cross-project triggers — ADO handles the - // routing natively). - .with_env( - "BUILD_TRIGGEREDBY_BUILDID", - EnvValue::ado_macro("Build.TriggeredBy.BuildId")?, - ) - .with_env( - "BUILD_TRIGGEREDBY_DEFINITIONID", - EnvValue::ado_macro("Build.TriggeredBy.DefinitionId")?, - ) - .with_env( - "BUILD_TRIGGEREDBY_DEFINITIONNAME", - EnvValue::ado_macro("Build.TriggeredBy.DefinitionName")?, - ) - .with_env( - "BUILD_TRIGGEREDBY_PROJECTID", - EnvValue::ado_macro("Build.TriggeredBy.ProjectID")?, + // ADO auto-injects every predefined System.*/Build.* variable into the + // step env (SCREAMING_SNAKE form), so the bundle reads + // SYSTEM_COLLECTIONURI / BUILD_SOURCESDIRECTORY / BUILD_TRIGGEREDBY_* + // directly without re-projection. Only the non-auto-injected + // SYSTEM_ACCESSTOKEN bearer is projected, via the bundle-auth applier. + let step = apply_bundle_auth( + BashStep::new( + "Stage pipeline execution context (aw-context/pipeline/*)", + script, + ) + .with_condition(Condition::Eq( + Expr::Variable("Build.Reason".to_string()), + Expr::Literal("ResourceTrigger".to_string()), + )), + Bundle::ExecContextPipeline, + TokenSource::SystemAccessToken, ); Ok(Some(Step::Bash(step))) } @@ -146,6 +120,7 @@ impl ContextContributor for PipelineContextContributor { #[cfg(test)] mod tests { use super::*; + use crate::compile::ir::env::EnvValue; use crate::compile::extensions::CompileContext; use crate::compile::types::FrontMatter; @@ -218,30 +193,22 @@ mod tests { // Bearer present. assert!(matches!( bash.env.get("SYSTEM_ACCESSTOKEN"), - Some(EnvValue::AdoMacro("System.AccessToken")) - )); - // Collection URI for REST endpoint construction. - assert!(matches!( - bash.env.get("SYSTEM_COLLECTIONURI"), - Some(EnvValue::AdoMacro("System.CollectionUri")) + Some(EnvValue::Secret(v)) if v == "System.AccessToken" )); - // TriggeredBy quartet. - for (env_key, ado_var) in [ - ("BUILD_TRIGGEREDBY_BUILDID", "Build.TriggeredBy.BuildId"), - ( - "BUILD_TRIGGEREDBY_DEFINITIONID", - "Build.TriggeredBy.DefinitionId", - ), - ( - "BUILD_TRIGGEREDBY_DEFINITIONNAME", - "Build.TriggeredBy.DefinitionName", - ), - ("BUILD_TRIGGEREDBY_PROJECTID", "Build.TriggeredBy.ProjectID"), + // Every predefined System.*/Build.* var this bundle reads is + // auto-injected by ADO, so the step must NOT re-project them. + for stripped in [ + "SYSTEM_COLLECTIONURI", + "BUILD_SOURCESDIRECTORY", + "BUILD_TRIGGEREDBY_BUILDID", + "BUILD_TRIGGEREDBY_DEFINITIONID", + "BUILD_TRIGGEREDBY_DEFINITIONNAME", + "BUILD_TRIGGEREDBY_PROJECTID", ] { - match bash.env.get(env_key) { - Some(EnvValue::AdoMacro(name)) => assert_eq!(*name, ado_var), - other => panic!("expected {env_key} -> AdoMacro({ado_var}), got {other:?}"), - } + assert!( + bash.env.get(stripped).is_none(), + "{stripped} is auto-injected and must not be re-projected" + ); } } diff --git a/src/compile/extensions/exec_context/pr.rs b/src/compile/extensions/exec_context/pr.rs index 9121f20b..40a5a66e 100644 --- a/src/compile/extensions/exec_context/pr.rs +++ b/src/compile/extensions/exec_context/pr.rs @@ -59,6 +59,7 @@ //! emitted YAML. use crate::compile::extensions::CompileContext; +use crate::compile::ado_bundle::{Bundle, TokenSource, apply_bundle_auth}; use crate::compile::extensions::ado_script::EXEC_CONTEXT_PR_PATH; use crate::compile::ir::condition::{Condition, Expr}; use crate::compile::ir::env::EnvValue; @@ -129,17 +130,13 @@ impl ContextContributor for PrContextContributor { // which is exactly when this step should skip. // // Coexists with `prepare_step` until production callers switch. - let (pr_id, target_branch, prelude, condition) = if self.synthetic_pr_active { + let (prelude, condition) = if self.synthetic_pr_active { ( - EnvValue::pipeline_var("AW_PR_ID"), - EnvValue::pipeline_var("AW_PR_TARGETBRANCH"), " if [ -z \"$AW_PR_ID\" ]; then\n echo \"[aw-context] No PR identifier resolved (not a PR build and not synth-promoted); skipping exec-context-pr.\"\n exit 0\n fi\n", Condition::Succeeded, ) } else { ( - EnvValue::ado_macro("System.PullRequest.PullRequestId")?, - EnvValue::ado_macro("System.PullRequest.TargetBranch")?, "", Condition::Eq( Expr::Variable("Build.Reason".to_string()), @@ -148,26 +145,28 @@ impl ContextContributor for PrContextContributor { ) }; let script = format!("set -euo pipefail\n{prelude}node '{EXEC_CONTEXT_PR_PATH}'\n"); - let step = BashStep::new("Stage PR execution context (aw-context/pr/*)", script) - .with_condition(condition) - .with_env( - "SYSTEM_ACCESSTOKEN", - EnvValue::ado_macro("System.AccessToken")?, - ) - .with_env("SYSTEM_PULLREQUEST_PULLREQUESTID", pr_id) - .with_env("SYSTEM_PULLREQUEST_TARGETBRANCH", target_branch) - .with_env( - "SYSTEM_TEAMPROJECT", - EnvValue::ado_macro("System.TeamProject")?, - ) - .with_env( - "BUILD_REPOSITORY_NAME", - EnvValue::ado_macro("Build.Repository.Name")?, - ) - .with_env( - "BUILD_SOURCESDIRECTORY", - EnvValue::ado_macro("Build.SourcesDirectory")?, - ); + // ADO auto-injects predefined System.*/Build.* context vars, so the + // bundle reads SYSTEM_TEAMPROJECT / BUILD_REPOSITORY_NAME / + // BUILD_SOURCESDIRECTORY (and, in real-PR mode, SYSTEM_PULLREQUEST_*) + // directly. Only the non-auto-injected SYSTEM_ACCESSTOKEN bearer and, + // in synth mode, the hoisted AW_PR_* overrides are projected. + let mut step = apply_bundle_auth( + BashStep::new("Stage PR execution context (aw-context/pr/*)", script) + .with_condition(condition), + Bundle::ExecContextPr, + TokenSource::SystemAccessToken, + ); + if self.synthetic_pr_active { + step = step + .with_env( + "SYSTEM_PULLREQUEST_PULLREQUESTID", + EnvValue::pipeline_var("AW_PR_ID"), + ) + .with_env( + "SYSTEM_PULLREQUEST_TARGETBRANCH", + EnvValue::pipeline_var("AW_PR_TARGETBRANCH"), + ); + } Ok(Some(Step::Bash(step))) } @@ -273,16 +272,17 @@ mod tests { ); // SYSTEM_ACCESSTOKEN must still be in the step's env (the - // trust boundary that the bundle relies on). + // trust boundary that the bundle relies on), projected as a secret + // via the bundle-auth applier. assert!(matches!( bash.env.get("SYSTEM_ACCESSTOKEN"), - Some(EnvValue::AdoMacro("System.AccessToken")) + Some(EnvValue::Secret(v)) if v == "System.AccessToken" )); } - /// Synth-inactive: PR id / target branch are plain - /// `EnvValue::AdoMacro` values, no Coalesce; condition is the - /// typed `Eq(Variable("Build.Reason"), Literal("PullRequest"))`. + /// Synth-inactive: the auto-injected `SYSTEM_PULLREQUEST_*` and + /// `SYSTEM_TEAMPROJECT` / `BUILD_*` context vars are NOT re-projected; + /// condition is the typed `Eq(Variable("Build.Reason"), Literal("PullRequest"))`. #[test] fn prepare_step_typed_synth_inactive_uses_plain_macros_and_narrow_condition() { let contributor = PrContextContributor::new(PrContextConfig::default(), false); @@ -298,14 +298,20 @@ mod tests { other => panic!("expected Step::Bash, got {other:?}"), }; - assert!(matches!( - bash.env.get("SYSTEM_PULLREQUEST_PULLREQUESTID"), - Some(EnvValue::AdoMacro("System.PullRequest.PullRequestId")) - )); - assert!(matches!( - bash.env.get("SYSTEM_PULLREQUEST_TARGETBRANCH"), - Some(EnvValue::AdoMacro("System.PullRequest.TargetBranch")) - )); + // In real-PR mode the bundle reads the auto-injected PR vars and + // context vars directly, so the step re-projects none of them. + for stripped in [ + "SYSTEM_PULLREQUEST_PULLREQUESTID", + "SYSTEM_PULLREQUEST_TARGETBRANCH", + "SYSTEM_TEAMPROJECT", + "BUILD_REPOSITORY_NAME", + "BUILD_SOURCESDIRECTORY", + ] { + assert!( + bash.env.get(stripped).is_none(), + "{stripped} is auto-injected and must not be re-projected" + ); + } // No BUILD_REASON / AW_SYNTHETIC_PR env entries (the bash // guard isn't emitted on the synth-inactive path). diff --git a/src/compile/extensions/exec_context/pr_checks.rs b/src/compile/extensions/exec_context/pr_checks.rs index ea8ac5b7..f70699fe 100644 --- a/src/compile/extensions/exec_context/pr_checks.rs +++ b/src/compile/extensions/exec_context/pr_checks.rs @@ -20,6 +20,7 @@ //! same `AW_PR_ID`-empty-check gate. use crate::compile::extensions::CompileContext; +use crate::compile::ado_bundle::{Bundle, TokenSource, apply_bundle_auth}; use crate::compile::extensions::ado_script::EXEC_CONTEXT_PR_CHECKS_PATH; use crate::compile::ir::condition::{Condition, Expr}; use crate::compile::ir::env::EnvValue; @@ -76,18 +77,17 @@ impl ContextContributor for PrChecksContextContributor { if !self.should_activate(ctx) { return Ok(None); } - // Mirror the PR contributor's synth-active env selection so - // the bundle reads the same PR id under both real and synth - // paths. - let (pr_id_env, condition, prelude) = if self.synthetic_pr_active { + // In synth mode the PR id is overridden from the hoisted AW_PR_ID job + // variable and the step always runs (guarded by a runtime prelude); + // in real-PR mode the auto-injected SYSTEM_PULLREQUEST_PULLREQUESTID is + // read directly and the step gates on Build.Reason == PullRequest. + let (condition, prelude) = if self.synthetic_pr_active { ( - EnvValue::pipeline_var("AW_PR_ID"), Condition::Succeeded, " if [ -z \"$SYSTEM_PULLREQUEST_PULLREQUESTID\" ]; then\n echo \"[aw-context] No PR identifier resolved; skipping exec-context-pr-checks.\"\n exit 0\n fi\n", ) } else { ( - EnvValue::ado_macro("System.PullRequest.PullRequestId")?, Condition::Eq( Expr::Variable("Build.Reason".to_string()), Expr::Literal("PullRequest".to_string()), @@ -97,29 +97,26 @@ impl ContextContributor for PrChecksContextContributor { }; let script = format!("set -euo pipefail\n{prelude}node '{EXEC_CONTEXT_PR_CHECKS_PATH}'\n"); - let step = BashStep::new( - "Stage PR-checks execution context (aw-context/pr/checks/*)", - script, - ) - .with_condition(condition) - .with_env( - "SYSTEM_ACCESSTOKEN", - EnvValue::ado_macro("System.AccessToken")?, - ) - .with_env( - "SYSTEM_COLLECTIONURI", - EnvValue::ado_macro("System.CollectionUri")?, - ) - .with_env( - "SYSTEM_TEAMPROJECT", - EnvValue::ado_macro("System.TeamProject")?, - ) - .with_env("BUILD_BUILDID", EnvValue::ado_macro("Build.BuildId")?) - .with_env( - "BUILD_SOURCESDIRECTORY", - EnvValue::ado_macro("Build.SourcesDirectory")?, - ) - .with_env("SYSTEM_PULLREQUEST_PULLREQUESTID", pr_id_env); + // ADO auto-injects predefined System.*/Build.* context vars, so only + // SYSTEM_ACCESSTOKEN (bearer, not auto-injected) and the mode-dependent + // PR id are projected. In synth mode the id comes from the hoisted + // AW_PR_ID job variable; in real-PR mode the auto-injected value is + // used directly. + let mut step = apply_bundle_auth( + BashStep::new( + "Stage PR-checks execution context (aw-context/pr/checks/*)", + script, + ) + .with_condition(condition), + Bundle::ExecContextPrChecks, + TokenSource::SystemAccessToken, + ); + if self.synthetic_pr_active { + step = step.with_env( + "SYSTEM_PULLREQUEST_PULLREQUESTID", + EnvValue::pipeline_var("AW_PR_ID"), + ); + } Ok(Some(Step::Bash(step))) } @@ -218,10 +215,10 @@ mod tests { Step::Bash(b) => b, _ => panic!(), }; - // Trust boundary: bearer must be present. + // Trust boundary: bearer must be present (secret via applier). assert!(matches!( bash.env.get("SYSTEM_ACCESSTOKEN"), - Some(EnvValue::AdoMacro("System.AccessToken")) + Some(EnvValue::Secret(v)) if v == "System.AccessToken" )); // Runtime gate: step must only fire on PR builds. match bash.condition.as_ref().expect("condition required") { @@ -233,14 +230,20 @@ mod tests { "expected Condition::Eq(Variable(Build.Reason), Literal(PullRequest)), got {other:?}" ), } - // PR ID env: plain ADO macro on non-synth path. - assert!( - matches!( - bash.env.get("SYSTEM_PULLREQUEST_PULLREQUESTID"), - Some(EnvValue::AdoMacro("System.PullRequest.PullRequestId")) - ), - "expected AdoMacro(System.PullRequest.PullRequestId), got {:?}", - bash.env.get("SYSTEM_PULLREQUEST_PULLREQUESTID") - ); + // Non-synth path: the bundle reads the auto-injected + // SYSTEM_PULLREQUEST_PULLREQUESTID (and other context vars) directly, + // so the step re-projects none of them. + for stripped in [ + "SYSTEM_PULLREQUEST_PULLREQUESTID", + "SYSTEM_COLLECTIONURI", + "SYSTEM_TEAMPROJECT", + "BUILD_BUILDID", + "BUILD_SOURCESDIRECTORY", + ] { + assert!( + bash.env.get(stripped).is_none(), + "{stripped} is auto-injected and must not be re-projected" + ); + } } } diff --git a/src/compile/extensions/exec_context/repo.rs b/src/compile/extensions/exec_context/repo.rs index 5c0d01e2..cbbcc975 100644 --- a/src/compile/extensions/exec_context/repo.rs +++ b/src/compile/extensions/exec_context/repo.rs @@ -53,22 +53,14 @@ impl ContextContributor for RepoContextContributor { return Ok(None); } let script = format!("set -euo pipefail\nnode '{EXEC_CONTEXT_REPO_PATH}'\n"); + // ADO auto-injects BUILD_SOURCESDIRECTORY / BUILD_SOURCEVERSION / + // BUILD_SOURCEBRANCH into the step env, so the git-only bundle reads + // them directly; only the compile-time AW_REPO_CONVENTIONS toggle is a + // genuine step input. Repo has no bearer (BundleAuth::None). let step = BashStep::new("Stage repo execution context (aw-context/repo/*)", script) // Always-on (no Build.Reason gate). The compile-time // activation flag is the only gate. .with_condition(Condition::Succeeded) - .with_env( - "BUILD_SOURCESDIRECTORY", - EnvValue::ado_macro("Build.SourcesDirectory")?, - ) - .with_env( - "BUILD_SOURCEVERSION", - EnvValue::ado_macro("Build.SourceVersion")?, - ) - .with_env( - "BUILD_SOURCEBRANCH", - EnvValue::ado_macro("Build.SourceBranch")?, - ) .with_env( "AW_REPO_CONVENTIONS", EnvValue::literal(self.config.conventions_enabled().to_string()), diff --git a/src/compile/extensions/exec_context/schedule.rs b/src/compile/extensions/exec_context/schedule.rs index 480ba4a4..98323585 100644 --- a/src/compile/extensions/exec_context/schedule.rs +++ b/src/compile/extensions/exec_context/schedule.rs @@ -15,9 +15,9 @@ //! `exec-context-ci-push`. use crate::compile::extensions::CompileContext; +use crate::compile::ado_bundle::{Bundle, TokenSource, apply_bundle_auth}; use crate::compile::extensions::ado_script::EXEC_CONTEXT_SCHEDULE_PATH; use crate::compile::ir::condition::{Condition, Expr}; -use crate::compile::ir::env::EnvValue; use crate::compile::ir::step::{BashStep, Step}; use crate::compile::types::ScheduleContextConfig; @@ -58,42 +58,20 @@ impl ContextContributor for ScheduleContextContributor { return Ok(None); } let script = format!("set -euo pipefail\nnode '{EXEC_CONTEXT_SCHEDULE_PATH}'\n"); - let step = BashStep::new( - "Stage schedule execution context (aw-context/schedule/*)", - script, - ) - .with_condition(Condition::Eq( - Expr::Variable("Build.Reason".to_string()), - Expr::Literal("Schedule".to_string()), - )) - .with_env( - "SYSTEM_ACCESSTOKEN", - EnvValue::ado_macro("System.AccessToken")?, - ) - .with_env( - "SYSTEM_COLLECTIONURI", - EnvValue::ado_macro("System.CollectionUri")?, - ) - .with_env( - "SYSTEM_TEAMPROJECT", - EnvValue::ado_macro("System.TeamProject")?, - ) - .with_env( - "SYSTEM_DEFINITIONID", - EnvValue::ado_macro("System.DefinitionId")?, - ) - .with_env("BUILD_BUILDID", EnvValue::ado_macro("Build.BuildId")?) - .with_env( - "BUILD_SOURCESDIRECTORY", - EnvValue::ado_macro("Build.SourcesDirectory")?, - ) - .with_env( - "BUILD_SOURCEVERSION", - EnvValue::ado_macro("Build.SourceVersion")?, - ) - .with_env( - "BUILD_SOURCEBRANCH", - EnvValue::ado_macro("Build.SourceBranch")?, + // ADO auto-injects the predefined System.*/Build.* context variables + // into the step env, so the bundle reads them directly; only the + // non-auto-injected SYSTEM_ACCESSTOKEN bearer is projected here. + let step = apply_bundle_auth( + BashStep::new( + "Stage schedule execution context (aw-context/schedule/*)", + script, + ) + .with_condition(Condition::Eq( + Expr::Variable("Build.Reason".to_string()), + Expr::Literal("Schedule".to_string()), + )), + Bundle::ExecContextSchedule, + TokenSource::SystemAccessToken, ); Ok(Some(Step::Bash(step))) } @@ -116,6 +94,7 @@ impl ContextContributor for ScheduleContextContributor { #[cfg(test)] mod tests { use super::*; + use crate::compile::ir::env::EnvValue; use crate::compile::extensions::CompileContext; use crate::compile::types::FrontMatter; @@ -183,8 +162,24 @@ mod tests { } assert!(matches!( bash.env.get("SYSTEM_ACCESSTOKEN"), - Some(EnvValue::AdoMacro("System.AccessToken")) + Some(EnvValue::Secret(v)) if v == "System.AccessToken" )); + // Predefined System.*/Build.* context vars are auto-injected by ADO; + // the step must not re-project them. + for stripped in [ + "SYSTEM_COLLECTIONURI", + "SYSTEM_TEAMPROJECT", + "SYSTEM_DEFINITIONID", + "BUILD_BUILDID", + "BUILD_SOURCESDIRECTORY", + "BUILD_SOURCEVERSION", + "BUILD_SOURCEBRANCH", + ] { + assert!( + bash.env.get(stripped).is_none(), + "{stripped} is auto-injected and must not be re-projected" + ); + } } #[test] diff --git a/src/compile/extensions/exec_context/workitem.rs b/src/compile/extensions/exec_context/workitem.rs index cae7a7a2..a392ae4c 100644 --- a/src/compile/extensions/exec_context/workitem.rs +++ b/src/compile/extensions/exec_context/workitem.rs @@ -55,6 +55,7 @@ //! block; same posture as the PR contributor. use crate::compile::extensions::CompileContext; +use crate::compile::ado_bundle::{Bundle, TokenSource, apply_bundle_auth}; use crate::compile::extensions::ado_script::EXEC_CONTEXT_WORKITEM_PATH; use crate::compile::ir::condition::{Condition, Expr}; use crate::compile::ir::env::EnvValue; @@ -127,18 +128,16 @@ impl ContextContributor for WorkitemContextContributor { if !self.should_activate(ctx) { return Ok(None); } - // Mirror the PR contributor's synth-vs-real PR identifier - // selection — when synth is active the PR id comes from the - // hoisted `AW_PR_ID` Agent-job variable. - let (pr_id_env, condition) = if self.synthetic_pr_active { - (EnvValue::pipeline_var("AW_PR_ID"), Condition::Succeeded) + // In synth mode the PR id comes from the hoisted AW_PR_ID Agent-job + // variable and the step always runs (guarded by a runtime prelude); + // in real-PR mode the auto-injected SYSTEM_PULLREQUEST_PULLREQUESTID is + // read directly and the step gates on Build.Reason == PullRequest. + let condition = if self.synthetic_pr_active { + Condition::Succeeded } else { - ( - EnvValue::ado_macro("System.PullRequest.PullRequestId")?, - Condition::Eq( - Expr::Variable("Build.Reason".to_string()), - Expr::Literal("PullRequest".to_string()), - ), + Condition::Eq( + Expr::Variable("Build.Reason".to_string()), + Expr::Literal("PullRequest".to_string()), ) }; @@ -152,40 +151,33 @@ impl ContextContributor for WorkitemContextContributor { let max_body_kb = self.config.max_body_kb_resolved(); let script = format!("set -euo pipefail\n{prelude}node '{EXEC_CONTEXT_WORKITEM_PATH}'\n"); - let step = BashStep::new( - "Stage workitem execution context (aw-context/workitem/*)", - script, - ) - .with_condition(condition) - .with_env( - "SYSTEM_ACCESSTOKEN", - EnvValue::ado_macro("System.AccessToken")?, - ) - .with_env( - "SYSTEM_COLLECTIONURI", - EnvValue::ado_macro("System.CollectionUri")?, - ) - .with_env( - "SYSTEM_TEAMPROJECT", - EnvValue::ado_macro("System.TeamProject")?, - ) - .with_env( - "BUILD_SOURCESDIRECTORY", - EnvValue::ado_macro("Build.SourcesDirectory")?, - ) - .with_env( - "BUILD_REPOSITORY_ID", - EnvValue::ado_macro("Build.Repository.ID")?, - ) - .with_env("SYSTEM_PULLREQUEST_PULLREQUESTID", pr_id_env) - .with_env( - "AW_WORKITEM_MAX_ITEMS", - EnvValue::literal(max_items.to_string()), - ) - .with_env( - "AW_WORKITEM_MAX_BODY_KB", - EnvValue::literal(max_body_kb.to_string()), + // ADO auto-injects predefined System.*/Build.* context vars, so only + // the SYSTEM_ACCESSTOKEN bearer (not auto-injected), the mode-dependent + // synth PR id, and the computed limits are projected. + let mut step = apply_bundle_auth( + BashStep::new( + "Stage workitem execution context (aw-context/workitem/*)", + script, + ) + .with_condition(condition), + Bundle::ExecContextWorkitem, + TokenSource::SystemAccessToken, ); + if self.synthetic_pr_active { + step = step.with_env( + "SYSTEM_PULLREQUEST_PULLREQUESTID", + EnvValue::pipeline_var("AW_PR_ID"), + ); + } + step = step + .with_env( + "AW_WORKITEM_MAX_ITEMS", + EnvValue::literal(max_items.to_string()), + ) + .with_env( + "AW_WORKITEM_MAX_BODY_KB", + EnvValue::literal(max_body_kb.to_string()), + ); Ok(Some(Step::Bash(step))) } @@ -302,13 +294,22 @@ mod tests { }; assert!(matches!( bash.env.get("SYSTEM_ACCESSTOKEN"), - Some(EnvValue::AdoMacro("System.AccessToken")) - )); - // PR id env from System macro (non-synth path). - assert!(matches!( - bash.env.get("SYSTEM_PULLREQUEST_PULLREQUESTID"), - Some(EnvValue::AdoMacro("System.PullRequest.PullRequestId")) + Some(EnvValue::Secret(v)) if v == "System.AccessToken" )); + // Non-synth path: the bundle reads the auto-injected PR + context + // vars directly, so the step re-projects none of them. + for stripped in [ + "SYSTEM_PULLREQUEST_PULLREQUESTID", + "SYSTEM_COLLECTIONURI", + "SYSTEM_TEAMPROJECT", + "BUILD_SOURCESDIRECTORY", + "BUILD_REPOSITORY_ID", + ] { + assert!( + bash.env.get(stripped).is_none(), + "{stripped} is auto-injected and must not be re-projected" + ); + } // Default caps surfaced as env literals so the bundle can read them. match bash.env.get("AW_WORKITEM_MAX_ITEMS") { Some(EnvValue::Literal(s)) => assert_eq!(s, "5"), diff --git a/src/compile/filter_ir.rs b/src/compile/filter_ir.rs index f31d9dc0..1af4ba7d 100644 --- a/src/compile/filter_ir.rs +++ b/src/compile/filter_ir.rs @@ -1204,6 +1204,7 @@ pub fn build_gate_step_typed( evaluator_path: &str, synthetic_pr_active: bool, ) -> anyhow::Result { + use crate::compile::ado_bundle::{Bundle, TokenSource, apply_bundle_auth}; use crate::compile::ir::condition::Condition; use crate::compile::ir::env::EnvValue; use crate::compile::ir::ids::StepId; @@ -1225,21 +1226,21 @@ pub fn build_gate_step_typed( let pr_synth_active = synthetic_pr_active && matches!(ctx, GateContext::PullRequest); let script = format!("node '{evaluator_path}'\n"); - let mut step = BashStep::new(ctx.display_name(), script) - .with_id(StepId::new(ctx.step_name())?) - .with_condition(Condition::Succeeded) - // The gate evaluator JS bundle emits `##vso[task.setvariable - // variable=SHOULD_RUN;isOutput=true]` at runtime — declare it - // here so cross-job consumers (e.g. the Agent-job condition's - // typed `Condition::Eq(Expr::StepOutput(..., "SHOULD_RUN"))`) - // pass graph validation. See `src/compile/ir/output.rs` for - // the `OutputDecl` contract. - .with_output(crate::compile::ir::output::OutputDecl::new("SHOULD_RUN")) - .with_env( - "SYSTEM_ACCESSTOKEN", - EnvValue::ado_macro("System.AccessToken")?, - ) - .with_env("GATE_SPEC", EnvValue::literal(spec_b64)); + let mut step = apply_bundle_auth( + BashStep::new(ctx.display_name(), script) + .with_id(StepId::new(ctx.step_name())?) + .with_condition(Condition::Succeeded) + // The gate evaluator JS bundle emits `##vso[task.setvariable + // variable=SHOULD_RUN;isOutput=true]` at runtime — declare it + // here so cross-job consumers (e.g. the Agent-job condition's + // typed `Condition::Eq(Expr::StepOutput(..., "SHOULD_RUN"))`) + // pass graph validation. See `src/compile/ir/output.rs` for + // the `OutputDecl` contract. + .with_output(crate::compile::ir::output::OutputDecl::new("SHOULD_RUN")), + Bundle::Gate, + TokenSource::SystemAccessToken, + ) + .with_env("GATE_SPEC", EnvValue::literal(spec_b64)); // AW_SYNTHETIC_PR (same-job consumer of the synthPr step) reads // the setVar-registered variable via plain `$(name)` macro. The diff --git a/src/compile/mod.rs b/src/compile/mod.rs index a06f3893..578611b5 100644 --- a/src/compile/mod.rs +++ b/src/compile/mod.rs @@ -8,6 +8,7 @@ mod common; pub(crate) use common::resolve_repos; +pub(crate) mod ado_bundle; pub(crate) mod agentic_pipeline; #[cfg(test)] mod codemod_integration_test; diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index cdd44d83..201298b4 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -5588,17 +5588,20 @@ fn test_execution_context_pr_emits_prepare_step_and_prompt_supplement() { compiled.contains("SYSTEM_PULLREQUEST_TARGETBRANCH: $(AW_PR_TARGETBRANCH)"), "Prepare step must pass the PR target branch via the hoisted AW_PR_TARGETBRANCH job-variable" ); + // ADO auto-injects the predefined System.*/Build.* context vars the + // bundle reads, so the prepare step must NOT re-project them (they were + // redundant mirrors — see src/compile/ado_bundle.rs). assert!( - compiled.contains("SYSTEM_TEAMPROJECT: $(System.TeamProject)"), - "Prepare step must pass the ADO project name through to the bundle" + !compiled.contains("SYSTEM_TEAMPROJECT: $(System.TeamProject)"), + "SYSTEM_TEAMPROJECT is auto-injected and must not be re-projected" ); assert!( - compiled.contains("BUILD_REPOSITORY_NAME: $(Build.Repository.Name)"), - "Prepare step must pass the repository name through to the bundle" + !compiled.contains("BUILD_REPOSITORY_NAME: $(Build.Repository.Name)"), + "BUILD_REPOSITORY_NAME is auto-injected and must not be re-projected" ); assert!( - compiled.contains("BUILD_SOURCESDIRECTORY: $(Build.SourcesDirectory)"), - "Prepare step must pass the workspace root through to the bundle" + !compiled.contains("BUILD_SOURCESDIRECTORY: $(Build.SourcesDirectory)"), + "BUILD_SOURCESDIRECTORY is auto-injected and must not be re-projected" ); // v7: the bundle install/download must be present in the Agent @@ -5761,6 +5764,108 @@ fn test_execution_context_pr_does_not_leak_system_accesstoken() { ); } +/// Churn guard for the bundle env-var contract refactor: no bundle step +/// re-projects an ADO predefined variable that the runtime already +/// auto-injects (e.g. `SYSTEM_TEAMPROJECT: $(System.TeamProject)`). Such +/// redundant mirrors were stripped when the auth contract was centralised in +/// `src/compile/ado_bundle.rs`; this test walks the compiled YAML and fails if +/// any reappear. +/// +/// The invariant: for a step `env:` entry `KEY: $(Dotted.Var)`, it is a +/// redundant mirror iff `KEY == Dotted.Var.replace('.', "_").to_uppercase()`. +/// `SYSTEM_ACCESSTOKEN: $(System.AccessToken)` is the sanctioned exception — +/// the token is NOT auto-injected and must be projected (that projection is +/// what fixed #1307). +/// +/// Runs against multiple fixture families so the exec-context prepare steps, +/// the synth-PR step, the conclusion step, AND the filter `gate.js` step are +/// all covered. +#[test] +fn test_bundle_steps_do_not_reproject_auto_injected_ado_vars() { + use serde_yaml::Value; + + // Keys retained deliberately even though they mirror an auto-injected var: + // - SYSTEM_ACCESSTOKEN: the bearer, which is NOT auto-injected (ADO maps + // it only on explicit reference), so it must be projected. + // - BUILD_REQUESTEDFOR / BUILD_REQUESTEDFOREMAIL: the manual contributor's + // requestor-identity vars. These ARE auto-injected, so stripping them + // would not change runtime behaviour — but they sit behind the + // `include_email_resolved()` email-hygiene opt-in, and projecting them + // keeps that privacy intent visible at the call site. See the comment in + // src/compile/extensions/exec_context/manual.rs::prepare_step_typed. + const RETAINED: &[&str] = &[ + "SYSTEM_ACCESSTOKEN", + "BUILD_REQUESTEDFOR", + "BUILD_REQUESTEDFOREMAIL", + ]; + + fn screaming_snake(dotted: &str) -> String { + dotted.replace('.', "_").to_uppercase() + } + + fn walk(v: &Value, offenders: &mut Vec) { + match v { + Value::Mapping(m) => { + if let Some(Value::Mapping(env_map)) = m.get(Value::String("env".to_string())) { + for (k, val) in env_map { + let (key, value) = match (k.as_str(), val.as_str()) { + (Some(k), Some(v)) => (k, v), + _ => continue, + }; + if RETAINED.contains(&key) { + continue; + } + // Match a bare `$(Dotted.Var)` macro value. + let inner = value + .strip_prefix("$(") + .and_then(|s| s.strip_suffix(')')); + if let Some(dotted) = inner + && dotted.contains('.') + && key == screaming_snake(dotted) + { + offenders.push(format!("{key}: {value}")); + } + } + } + for (_, vv) in m { + walk(vv, offenders); + } + } + Value::Sequence(seq) => { + for item in seq { + walk(item, offenders); + } + } + _ => {} + } + } + + // Fixture families that between them exercise every bundle step shape: + // - execution-context-agent.md: the exec-context prepare steps + synth-PR + // + conclusion (safe-outputs) steps. + // - pr-filter-tier1-agent.md / pipeline-filter-agent.md: the filter + // `gate.js` step (prGate / pipelineGate). + const FIXTURES: &[&str] = &[ + "execution-context-agent.md", + "pr-filter-tier1-agent.md", + "pipeline-filter-agent.md", + ]; + + for fixture in FIXTURES { + let compiled = compile_fixture(fixture); + let yaml: Value = serde_yaml::from_str(&compiled) + .unwrap_or_else(|e| panic!("{fixture} should parse as YAML: {e}")); + let mut offenders: Vec = Vec::new(); + walk(&yaml, &mut offenders); + assert!( + offenders.is_empty(), + "{fixture}: found redundant re-projections of auto-injected ADO variables in \ + compiled bundle steps (these must be dropped — the runtime auto-injects them): \ + {offenders:?}" + ); + } +} + /// Regression trap for an entire bug class: ADO step-level `condition:` /// fields MUST NOT reference `dependencies..outputs[...]`. That /// syntax is only legal in **job**-level conditions, in `variables:`