refactor(compile): centralize ado-script bundle env contract

[jamesadevine]

Summary

Follow-up to #1311. Fixes the class of bug behind #1307: every compiler step that runs an ado-script bundle previously hand-wrote its env: block, so the Conclusion step drifted and shipped without an ADO bearer token.

Introduces src/compile/ado_bundle.rs as the single source of truth for the bundles' env/auth contract, and (per maintainer direction) strips the redundant auto-injected env projections.

The registry (src/compile/ado_bundle.rs)

• Bundle — every ado-script bundle, with path() and auth() (BundleAuth::Bearer when the bundle reads SYSTEM_ACCESSTOKEN (ADO REST via getWebApi() and/or git bearer auth), else None).
• apply_bundle_auth(step, bundle, token) — the single chokepoint that projects SYSTEM_ACCESSTOKEN into every bearer-requiring bundle step. SYSTEM_ACCESSTOKEN is the one ADO predefined var that is not auto-injected, so it must be projected — the structural guarantee that no step can ship without a bearer again.
• token_source_for(write_sc) — unifies the System.AccessToken vs SC_WRITE_TOKEN selection previously duplicated between the Conclusion job and the Stage 3 executor.
• is_redundant_ado_mirror(key, value) — flags redundant re-projections of auto-injected predefined vars.

Migration + strip

Every bundle call site (gate, synthetic-PR, the 8 exec-context contributors, conclusion, executor) now routes auth through the applier. The ~40 redundant SYSTEM_* / BUILD_* env projections that ADO already auto-injects into every script step are removed — verified against each bundle's actual process.env reads that the bundles consume the auto-injected SCREAMING_SNAKE names directly.

The synthetic-PR bundle read the renamed ADO_PROJECT/ADO_REPO_ID; migrated it onto auto-injected SYSTEM_TEAMPROJECT/BUILD_REPOSITORY_ID so those two projections could be dropped too.

Retained (documented): the SYSTEM_ACCESSTOKEN bearer, the mode-dependent synth PR-id overrides (pipeline_var(AW_PR_*)), the manual contributor's identity vars behind the email-hygiene gate, and all computed inputs (GATE_SPEC, PR_SYNTH_SPEC, AW_*, PARAM_*).

Tests

• Contract test over Bundle::ALL: apply_bundle_auth projects the token for Bearer bundles only; token_source_for selection; is_redundant_ado_mirror correctness.
• Per-contributor unit tests updated to assert the stripped keys are absent and the bearer is a Secret.
• New compiled-YAML churn guard in tests/compiler_tests.rs: walks the emitted pipeline and fails if any bundle step re-projects an auto-injected ADO variable.

No lock churn

The token wire form is byte-identical (secret("System.AccessToken") and the previous ado_macro(...)/secret(...) both lower to $(System.AccessToken)), and the committed tests/**/*.lock.yml fixtures don't exercise exec-context/gate/synth — confirmed zero drift after recompiling.

Validation

• vitest run — 441/441.
• cargo build (0 warnings), cargo test (all suites green), cargo clippy --all-targets --all-features — clean.

[github-actions]

🔍 Rust PR Review

Summary: Looks good — solid fix for a real bug class with clean abstraction. Two minor observations worth noting.

Findings

⚠️ Suggestions

• src/compile/ado_bundle.rs:124 — Bundle::Conclusion path not backed by a constant
  Bundle::Conclusion.path() returns a hardcoded string literal ("/tmp/ado-aw-scripts/ado-script/conclusion.js") rather than a pub(crate) const in ado_script.rs (the pattern every other bundle uses). The method is #[allow(dead_code)] and only consumed by tests; the production path lives inline in agentic_pipeline.rs's shell script body. The test every_bundle_path_is_under_the_unpack_dir only validates format (starts_with / ends_with), not that the two copies match — so they could diverge silently. Adding a CONCLUSION_PATH const to ado_script.rs and referencing it from both places would make the invariant compile-time-enforced, consistent with every other bundle.

• src/compile/extensions/exec_context/manual.rs:159 — BUILD_REQUESTEDFOR / BUILD_REQUESTEDFOREMAIL are semantically redundant mirrors
  These two projections are in the RETAINED allowlist in the churn guard and are documented as intentional ("makes the intent explicit at the call site"), but they are standard ADO predefined variables that the runtime auto-injects. The stated goal of the PR is to strip all redundant mirrors; retaining these two for documentation is a reasonable pragmatic choice, but the code comment and RETAINED list are the only hints that they differ from the rest of the stripped keys. A brief note in the churn guard test (or in is_redundant_ado_mirror's doc comment) explaining why these are exempted would make the asymmetry less surprising for future contributors.

✅ What Looks Good

• Core fix is correct. apply_bundle_auth is a structural guarantee — the BundleAuth::AdoRest arm makes it impossible to forget the bearer on any new REST-calling bundle step without explicitly acknowledging the auth contract. The Conclusion step regression was a clean class-of-bug fix, not just a point fix.

• is_redundant_ado_mirror correctly exempts the auth token. EnvValue::Secret never matches the AdoMacro arm, so the deliberately-projected SYSTEM_ACCESSTOKEN bearer is never flagged as redundant, even though its key matches the screaming-snake form of System.AccessToken. This is exactly the right invariant.

• token_source_for unification. The old code had the System.AccessToken vs SC_WRITE_TOKEN selector duplicated between agentic_pipeline.rs and common.rs::generate_executor_ado_env. Both now call the shared helper, eliminating the drift vector.

• Wire-form byte-identity preserved. The EnvValue::Secret("System.AccessToken") → $(System.AccessToken) lowering matches the previous EnvValue::AdoMacro("System.AccessToken") form. Zero YAML churn on the token line is important for lock-file stability.

• Churn guard integration test. Walking the compiled YAML for redundant mirrors as a test-time invariant (rather than relying purely on code review) is a good defense-in-depth pattern.

• Test coverage is comprehensive. Unit tests cover apply_bundle_auth for every variant in Bundle::ALL, token_source_for selection, and is_redundant_ado_mirror edge cases (Secret-typed token, pipeline-var override, cross-key macro). Per-contributor tests explicitly assert that stripped keys are absent.

Generated by Rust PR Reviewer for #1315 · 113.9 AIC · ⌖ 7.28 AIC · ⊞ 6.1K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — clean structural fix, well-tested, and the TypeScript bundle migration is correctly coordinated. One minor doc inaccuracy in the new BundleAuth enum; everything else is solid.

---

Findings

⚠️ Suggestions

• src/compile/ado_bundle.rs:57 — BundleAuth::AdoRest variant doc says "authenticates to the ADO REST API via getWebApi()" but ExecContextPr doesn't call getWebApi() at all — it passes SYSTEM_ACCESSTOKEN to bearerEnv() for git credential auth (exec-context-pr/index.ts:124). The actual invariant the variant captures is "needs SYSTEM_ACCESSTOKEN to be projected", not specifically REST. The mismatch could mislead a maintainer adding a new bundle that uses the token only for git. Suggest rewording: "The bundle reads SYSTEM_ACCESSTOKEN from env (ADO REST auth, git bearer, or both)".

• src/compile/ado_bundle.rs:~210 — apply_bundle_auth silently discards token for BundleAuth::None bundles. No callers today pass None-auth bundles through this function (they simply don't call it), so it's harmless, but the signature's acceptance of a TokenSource without any doc note on None paths could be confusing. A /// No-op for BundleAuth::None; token is ignored note in the doc comment would make the contract explicit.

✅ What Looks Good

• Core fix is structurally sound. The apply_bundle_auth chokepoint means a new bundle contributor can't accidentally ship without a bearer — the class-of-bug behind conclusion.js fails to file pipeline_failure work item — ADO_COLLECTION_URI env var never injected by compiler (v0.39.0) #1307 cannot recur through normal extension of the Bundle enum.
• TypeScript bundle migration is correctly coordinated. ADO_PROJECT/ADO_REPO_ID → SYSTEM_TEAMPROJECT/BUILD_REPOSITORY_ID is changed atomically in both index.ts and the test harness alongside the compiler, and the vitest suite validates the bundle reads the right names.
• BundleAuth::None classification is verified correct. ExecContextManual, ExecContextRepo, Import, and ApprovalSummary are confirmed to not import ado-client or call getWebApi() / bearerEnv, and the manual bundle source (index.ts:25) explicitly documents "No bearer; SYSTEM_ACCESSTOKEN is NOT projected".
• Churn guard test is well-designed. The walk + screaming_snake integration test walks real compiled YAML and the RETAINED allowlist (SYSTEM_ACCESSTOKEN, BUILD_REQUESTEDFOR, BUILD_REQUESTEDFOREMAIL) is correctly scoped.
• Zero YAML churn on the hot path. EnvValue::secret("System.AccessToken") and the old EnvValue::ado_macro(...) lower to the same $(System.AccessToken) wire form, so compiled lock fixtures are unaffected.
• token_source_for deduplication. The Conclusion job and Stage 3 executor previously had independent branches that could diverge; routing both through the shared helper eliminates that class of drift.

Generated by Rust PR Reviewer for #1315 · 120.4 AIC · ⌖ 6.27 AIC · ⊞ 6.1K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — the core fix is correct, the abstraction is well-designed, and test coverage is solid. A few minor items worth addressing before merge.

---

Findings

⚠️ Suggestions

• src/compile/ado_bundle.rs:105-117, 146, 213 — Bundle::ALL, Bundle::path(), and is_redundant_ado_mirror carry #[allow(dead_code)] rather than #[cfg(test)]. All three are only called from the #[cfg(test)] block in this file; none are referenced by tests/compiler_tests.rs (the churn guard re-implements screaming_snake inline). Prefer #[cfg(test)] for the items that are genuinely test-only so they're excluded from release builds and the allow suppression isn't hiding accidental orphans:

  #[cfg(test)]
  pub const ALL: &'static [Bundle] = &[...];
  
  #[cfg(test)]
  pub fn path(self) -> &'static str { ... }
  
  #[cfg(test)]
  pub fn is_redundant_ado_mirror(key: &str, value: &EnvValue) -> bool { ... }

• tests/compiler_tests.rs:5764 — The churn guard (test_bundle_steps_do_not_reproject_auto_injected_ado_vars) only exercises execution-context-agent.md. Bundle::Gate (used in PR-filter pipelines) and any future bundle steps in other fixture families are not covered. Even running against one additional fixture that exercises the gate step (e.g. a PR-trigger fixture) would close this gap.

• src/compile/agentic_pipeline.rs:1580, src/compile/filter_ir.rs:1225 — Both call sites use fully-qualified crate::compile::ado_bundle::apply_bundle_auth(..., crate::compile::ado_bundle::Bundle::..., crate::compile::ado_bundle::TokenSource::...). The other call sites (all the exec_context/* files) correctly import via use crate::compile::ado_bundle::{Bundle, TokenSource, apply_bundle_auth};. The two outliers are verbose but harmless; aligning them with the pattern used elsewhere would improve readability.

---

✅ What Looks Good

• Core fix is structurally sound. The apply_bundle_auth chokepoint guarantees no bearer-requiring bundle step can ship without SYSTEM_ACCESSTOKEN. The previous hand-written env blocks that drifted (the root cause of conclusion.js fails to file pipeline_failure work item — ADO_COLLECTION_URI env var never injected by compiler (v0.39.0) #1307) are eliminated.
• Wire-identical lowering. The change from EnvValue::AdoMacro("System.AccessToken") to EnvValue::secret("System.AccessToken") produces the same $(System.AccessToken) in generated YAML — confirmed by the lock-file non-drift note in the PR description. No customer-visible change.
• token_source_for unification. The common.rs executor and agentic_pipeline.rs conclusion job now share a single token-selection function. The byte-identical output is verified in token_source_selection_matches_write_service_connection.
• is_redundant_ado_mirror stays a pure predicate. The allowlist for intentional exceptions (SYSTEM_ACCESSTOKEN, BUILD_REQUESTEDFOR, BUILD_REQUESTEDFOREMAIL) lives at the call site in the churn guard, not special-cased inside the predicate. Good separation.
• Synth-mode PR id overrides are preserved. pr.rs, pr_checks.rs, and workitem.rs all correctly continue to project SYSTEM_PULLREQUEST_PULLREQUESTID from AW_PR_ID in synth mode; only the real-PR-mode redundant projections are dropped.
• Tests are updated in lockstep. Every modified contributor has corresponding unit-test updates asserting the bearer is a Secret (not AdoMacro) and that stripped keys are absent. The existing synth-active assertions (e.g. SYSTEM_PULLREQUEST_PULLREQUESTID: $(AW_PR_TARGETBRANCH) in compiler_tests.rs) are preserved unchanged, confirming no behavioral regression.
• ado_bundle.rs unit tests are comprehensive. The four unit tests cover auth projection per variant, token-source selection, the Secret-is-not-a-mirror invariant, and the redundant-mirror detector — good coverage for the new module.

Generated by Rust PR Reviewer for #1315 · 85.5 AIC · ⌖ 6.42 AIC · ⊞ 6.1K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — clean structural fix for the auth-contract drift class. No bugs found.

Findings

✅ What Looks Good

• Wire-form equivalence confirmed. EnvValue::Secret("System.AccessToken") and the old EnvValue::AdoMacro("System.AccessToken") both lower to $(System.AccessToken) in lower_env_value (same $({name}) branch). The YAML output is byte-identical; no lock-file drift. Bonus: Secret is the semantically correct variant here since it triggers the audit-path tagging the old AdoMacro form would miss.

• apply_bundle_auth truly is a single chokepoint. Every BundleAuth::Bearer site now routes through it, and the apply_bundle_auth_projects_token_for_bearer_bundles_only contract test covers all 13 variants in Bundle::ALL. The Conclusion entry being Bearer is exactly what fixes the conclusion.js fails to file pipeline_failure work item — ADO_COLLECTION_URI env var never injected by compiler (v0.39.0) #1307 regression class.

• is_redundant_ado_mirror predicate design is correct. Matching only on EnvValue::AdoMacro means the intentional Secret projection for SYSTEM_ACCESSTOKEN is never flagged — the variant difference is load-bearing, not incidental.

• Churn guard in tests/compiler_tests.rs is a solid integration-level complement to the unit contract tests; walking compiled YAML from real fixture families catches emitter-path regressions that IR-level tests would miss.

• GATE_EVAL_PATH visibility promotion (const → pub(crate)) is the only path constant that needed changing; all others were already pub(crate). Complete.

• token_source_for deduplication eliminates the has_write_sc / write_service_connection.is_some() branch that was separately implemented in agentic_pipeline.rs and common.rs. Both now produce the same string.

⚠️ Observations (no action required, but worth being aware of)

• BundleAuth::None bundles never pass through apply_bundle_auth in production (repo.rs, manual.rs build their steps directly). This is correct — calling it would be a no-op — but a new contributor adding a None-auth bundle might not realise they can skip the call. The docs/extending.md guidance is clear; just noting the asymmetry is deliberate.

• BUILD_REQUESTEDFOR / BUILD_REQUESTEDFOREMAIL in manual.rs are kept as explicit ado_macro projections even though ADO auto-injects them. The comment explains the reason (email-hygiene opt-in visibility), and the churn guard's RETAINED list correctly exempts them. No issue, but they are functionally redundant at runtime.

• pr.rs synth prelude checks $AW_PR_ID; pr_checks.rs / workitem.rs synth preludes check $SYSTEM_PULLREQUEST_PULLREQUESTID (which itself expands from $(AW_PR_ID)). Both guard correctly — ADO expands the macro before the script runs — but the inconsistency is worth noting if these preludes ever need to be aligned.

Generated by Rust PR Reviewer for #1315 · 79.3 AIC · ⌖ 6.96 AIC · ⊞ 6.1K · ◷