feat(compile): add execution-context plugin with PR contributor (#860)

[jamesadevine]

Summary

Closes #860.

Adds an always-on ExecContextExtension that precomputes a
minimal, focused set of execution context signals for the agent
on PR-triggered builds, and appends a tailored fragment to the agent
prompt so reviewer / triage agents stop reinventing git fetch,
merge-base resolution, and shallow-clone handling in every workflow
body.

The design follows the issue author's broader-scope request: a
plugin surface (ContextContributor trait + Contributor enum)
rather than a one-off PR helper. Future contributors (work-item,
build-history, schedule, …) plug into the same step slot and share
the same trust-boundary.

Design (v6.2 — aligned with gh-aw)

After several iterations, the design converged on a much smaller
surface than v1, inspired by how
gh-aw handles PR context. The
compiler does ONLY what the agent cannot do itself inside the AWF
sandbox:

1. Fetch the PR target branch with progressive deepening until
   git merge-base resolves (requires the bearer; cannot happen
   inside the agent's network-isolated sandbox).
2. Stage aw-context/pr/base.sha and aw-context/pr/head.sha
   (40-char hex SHAs) so the agent reuses them across many
   git diff invocations.
3. Append a prompt fragment to /tmp/awf-tools/agent-prompt.md
   listing the right git commands and example ADO MCP tool calls
   (repo_get_pull_request_by_id,
   repo_list_pull_request_threads,
   repo_create_pull_request_thread) with PR id / project / repo
   pre-filled as literal values.
4. Auto-extend the agent's bash allow-list with 7 read-only git
   commands (git, git diff, git log, git show,
   git status, git rev-parse, git symbolic-ref) so the agent
   can inspect the diff locally.

The agent then runs git diff $BASE..$HEAD itself inside the AWF
sandbox — the objects are already fetched into the workspace and
mounted in. No changed-files.txt, no diff.patch, no
head-files/ / base-files/ snapshots, no status.txt protocol.

base.sha always has the SAME semantics in both code paths
(synthetic-merge-commit and progressive-deepening): the true common
ancestor via git merge-base. The agent therefore sees the same
"PR diff since branch-point" change set regardless of how ADO
checked out the workspace.

Trust boundary (non-negotiable)

• No persistCredentials: true and no checkout override —
  would write the bearer to .git/config inside the AWF-mounted
  workspace where the agent could exfiltrate it.
• SYSTEM_ACCESSTOKEN is mapped from $(System.AccessToken)
  only into the precompute step's env: block; never into the
  agent step.
• Bearer injected into git fetch via
  GIT_CONFIG_COUNT / GIT_CONFIG_KEY_0 / GIT_CONFIG_VALUE_0
  env vars (not git -c http.extraheader=… argv — no
  process-listing leak).
• Step gated by
  condition: eq(variables['Build.Reason'], 'PullRequest').
• All 4 identifiers interpolated into the agent prompt or git
  refspecs (PR_ID, PROJECT, REPO, PR_TARGET_BRANCH) are
  strictly regex-validated before use, even though they come from
  ADO infra rather than user-controlled input.

Front matter (additive, optional)

on:
  pr:
    branches:
      include: [main]

# Optional — defaults to "on when on.pr is configured".
execution-context:
  pr:
    enabled: true   # default true when on.pr is set; set false to opt out
                    # (also suppresses auto-adding the 7 git commands
                    # to the agent's bash allow-list).

on.pr is REQUIRED for the contributor to activate. Setting
pr.enabled: true on a non-PR-triggered agent does NOT activate
the contributor (the prepare step would be dead code, and silently
widening the agent's bash surface would be a footgun).

Iterating to v6.2 — what changed vs v1

The original implementation staged 6 files and a status.txt
protocol with 4 status values. After review feedback:

• Dropped status.txt, metadata.txt, changed-files*.txt,
  diff.patch, head-files/, base-files/. The agent runs its
  own git diff against the precomputed SHAs.
• Collapsed execution-context.pr from 5 knobs
  (scope/unified/max-diff-bytes/snapshots/enabled) to
  one (enabled).
• Short identifiers (PR id, project, repo) are interpolated
  directly into the prompt fragment via printf, not staged as
  files.
• Failure path: error.txt + failure prompt fragment that tells
  the agent to surface the error (and use ADO MCP as a fallback)
  rather than produce an empty review.
• Auto-extends the agent bash allow-list with the 7 git commands.

Robustness fixes from review

1. base.sha semantics are now consistent across the
   synthetic-merge-commit path (HEAD^1 + HEAD^2) and the
   progressive-deepening path. Both compute
   git merge-base so git diff $BASE..$HEAD produces the same
   change set regardless of ADO's checkout mode.
2. PR_TARGET_BRANCH is regex-validated before
   interpolation into the git refspec
   (^[A-Za-z0-9._/-]+$), matching the posture of the other
   identifiers.
3. pr.enabled: true without on.pr does not activate the
   contributor (no compile-time widening of the agent bash
   allow-list for a step that can never run at runtime).
4. Progressive deepening actually works — target-branch fetch
   loop only breaks when git merge-base resolves (not on the
   first successful fetch).
5. No argv leak — bearer in GIT_CONFIG_* env, not argv.
6. Strengthened trust-boundary test — walks the compiled YAML
   and asserts SYSTEM_ACCESSTOKEN appears only on the
   exec-context prepare step's env: block (matched by
   displayName).

Docs

• New docs/execution-context.md — full reference (motivation,
  contributors, field reference, agent-visible layout, prompt
  fragment shape, bash-allowlist auto-extension, trust boundary,
  precompute step walkthrough).
• docs/front-matter.md — added execution-context: block to
  the example.
• AGENTS.md — index entry + architecture tree updated.
• prompts/create-ado-agentic-workflow.md — directs PR-reviewer
  authors to use the auto-staged context and prompt fragment
  rather than rolling their own precompute.

Test plan

• cargo build — clean.
• cargo clippy --all-targets --all-features — clean.
• cargo test — 1739 unit tests + 127 compiler tests
  (9 exec-context tests) + bash_lint_tests with
  ENFORCE_BASH_LINT=1 (shellcheck v0.10.0) + every other suite:
  all pass.

Exec-context tests (9 total)

• test_execution_context_pr_compiled_output_is_valid_yaml
• test_execution_context_pr_emits_prepare_step_and_prompt_supplement
• test_execution_context_pr_does_not_leak_system_accesstoken
  (parses YAML, asserts token env appears only on the
  exec-context prepare step by displayName)
• test_execution_context_pr_not_emitted_when_no_pr_trigger
• test_execution_context_pr_auto_extends_bash_allowlist
• test_execution_context_pr_does_not_extend_bash_when_disabled
• test_execution_context_pr_enabled_true_without_on_pr_is_inactive
• test_execution_context_pr_synthetic_merge_uses_merge_base
  (regression guard against the inconsistent-base.sha bug)
• test_execution_context_pr_target_branch_validated

The execution-context-agent.md fixture is also in the
bash_lint_tests FIXTURES list so the generated bash is
shellchecked on every CI run.

E2E validation in a real ADO build (replacing existing
pr-reviewer steps with this contributor) is intentionally out of
scope for this PR — tracked separately because it requires a
real ADO run.

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — the trust boundary design is solid and the test coverage is thorough. One stale doc/comment inconsistency in the security-critical section warrants a fix before merge; two minor latent hazards noted below.

---

Findings

🔒 Security Concerns

• docs/execution-context.md trust boundary table + src/compile/extensions/exec_context/pr.rs module comment describe the old, less-secure bearer injection mechanism that was already superseded in the same PR.

  The PR description (and the inline bash comment at line ~774 of the generated step) correctly states the token is injected via GIT_CONFIG_COUNT/KEY_0/VALUE_0 env vars so it never appears in process argv. But two places still say git -c http.extraheader=...:

  • docs/execution-context.md, trust boundary table row for "In-step SYSTEM_ACCESSTOKEN + bash bearer wrapper":

    A git_fetch wrapper injects git -c http.extraheader="Authorization: bearer ${SYSTEM_ACCESSTOKEN}" fetch ...

  • src/compile/extensions/exec_context/pr.rs, module-level doc comment footgun add agentic workflows #3:

    wraps git fetches with git -c http.extraheader=Authorization: bearer ...

  These descriptions refer to the old approach that the PR itself says was replaced ("No argv leak — bearer moved from git -c http.extraheader=... to GIT_CONFIG_* env"). The trust boundary doc is exactly what future security reviewers will read. Having it describe the inferior approach is misleading and could cause a reviewer to incorrectly approve a future regression. Both locations should be updated to match the actual GIT_CONFIG_* mechanism.

⚠️ Suggestions

• src/compile/extensions/exec_context/mod.rs prompt_supplement(): when any_contributor_active is true but every contributor's prompt_fragment() returns an empty/whitespace string, the method returns Some("\n---\n\n## Execution context\n") — a section header with no body. Not a bug in v1 (the PR contributor always returns a non-empty fragment), but a latent hazard for future contributors that emit nothing. A guard like if body.trim_start_matches("\n---\n\n## Execution context\n").trim().is_empty() { return None; } before the final Some(body) would make the invariant self-enforcing.

• any_contributor_active pre-computation in ExecContextExtension::new() mirrors each contributor's should_activate() logic. The comment acknowledges this duplication as intentional. The risk is that a future contributor author wires up should_activate() but forgets to OR-in the aggregate check, silently suppressing the prompt supplement. A short doc comment like // MAINTENANCE: update this aggregate when adding a new contributor in new() would reduce that risk.

✅ What Looks Good

• Progressive deepening is correctly implemented — the loop only breaks when git merge-base actually resolves after each fetch depth, not on a successful-fetch-alone. This was the specific correctness issue called out in the PR description and the fix is right.
• Trust boundary is robust: SYSTEM_ACCESSTOKEN is scoped to the precompute step env; persistCredentials: true is never emitted (and there's a regression test that walks the parsed YAML to assert exactly this); .git/config is never written.
• Compile-time scope validation in ExecContextExtension::validate() rejects ADO expressions ($(, ${{, $[), pipeline commands (##vso[, ##[), and newlines — the right set of injection vectors for values that get interpolated by ADO before bash sees them.
• scope_for_bash() quoting uses POSIX single-quote escaping ('\') with the values already validated to be newline-free. Correct.
• CTX_OK tracking means a failing git diff flips the status to CONTEXT_GENERATION_FAILED instead of silently writing OK with an empty patch. The status file discipline is consistent throughout.
• Test coverage is thorough: YAML validity, prepare step spot-checks, trust-boundary YAML walk, no-PR-trigger suppression, compile-fail for ADO expressions in scope.

Generated by Rust PR Reviewer for issue #865 · sonnet46 1.6M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Well-engineered feature with a solid security design — a few correctness and documentation gaps worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/compile/extensions/exec_context/pr.rs:179–195 — base.sha has different semantics across the two code paths. In the synthetic merge-commit path, BASE_SHA = HEAD^1 (the target branch tip at merge-preparation time), while in the progressive-deepening path, BASE_SHA = git merge-base origin/$target HEAD (the true common ancestor). These are not the same SHA when the target branch has advanced since the PR branched. git diff $BASE..$HEAD therefore produces different change sets depending on ADO checkout mode — the agent sees a narrower "what's new vs current target" diff on one runner and a broader "full PR since branch-point" diff on another. The docs at docs/execution-context.md:89 call it "target merge-base SHA" but in the synthetic path it is the target tip. Either the synthetic path should compute git merge-base HEAD^1 HEAD^2 for consistency, or the docs/comments should clearly distinguish the two semantics.

• pr.rs:175 — || echo 0 is unreachable dead code. wc -w always exits 0 and outputs 0 on empty input, so the fallback || echo 0 never fires. Minor, but worth removing to avoid false impressions about the error-handling path.

🔒 Security Concerns

• pr.rs:147–151 — PR_TARGET_BRANCH has no format validation, unlike the other three identifiers. PR_ID, PROJECT, and REPO are all guarded by strict allowlist regexes before use. PR_TARGET_BRANCH is only checked for emptiness, then its stripped form is interpolated into a git refspec:

  git_fetch --no-tags "$_depth_arg" origin \
    "+refs/heads/${PR_TARGET_SHORT}:refs/remotes/origin/${PR_TARGET_SHORT}"

  The value comes from ADO infra (System.PullRequest.TargetBranch), so the practical risk is low. But the asymmetry means a defensive-in-depth validation line is missing. A regex like ^[A-Za-z0-9._/\-]+$ on PR_TARGET_BRANCH before use (or on PR_TARGET_SHORT after stripping) would close the gap consistently.

⚠️ Suggestions

• mod.rs:78–82 + pr.rs:76–83 — explicit pr.enabled: true on a non-PR-triggered agent silently widens the bash allow-list. The should_activate Some(true) => true arm is evaluated independently of whether on.pr is configured. The prepare step is ADO-condition-gated at runtime (safe), but required_bash_commands() is a compile-time artifact: the 7 git commands are added to the agent's bash allow-list even though the step will never run. Consider emitting a compile-time warning, or documenting this footgun explicitly in docs/execution-context.md.

• PR description is stale post-v6.2. The body describes scope[], unified, max-diff-bytes, and snapshots front-matter fields, and mentions test_execution_context_pr_scope_rejects_ado_expressions as a new test — all of which were removed in the v6.2 simplification commit. The PR description test plan also says "5 new exec-context tests" but the final code has 6. This creates reviewer confusion; worth updating the PR description or noting in a comment.

✅ What Looks Good

• Bearer token isolation via GIT_CONFIG_* env vars — the token never appears in process argv, never lands in .git/config, never reaches the agent step's env. The trust-boundary test that walks the YAML and asserts the token appears only under the expected displayName is a great regression guard.
• Progressive deepening loop logic — correctly breaks only when git merge-base actually resolves (not on the first successful fetch). The fetch_target_at_depth || continue pattern is clean.
• set -uo pipefail without -e + explicit fail() pattern — all failure paths write error.txt, append a failure prompt fragment, and exit 0 so the build proceeds. The agent surfaces the failure rather than the build failing silently.
• Compile-time identifier validation before prompt interpolation — strict allowlists on PR_ID/PROJECT/REPO before they're written into the agent prompt prevent ADO-expression and newline injection. Defence-in-depth even though these are infra-provided values.
• test_execution_context_pr_does_not_leak_system_accesstoken — walking the parsed YAML rather than just substring-searching for the token is the right approach for a trust-boundary regression test.

Generated by Rust PR Reviewer for issue #865 · sonnet46 2.7M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — the trust-boundary design is solid and the test suite is thorough. Two items worth addressing before merge.

---

Findings

⚠️ Suggestions

any_contributor_active duplication is a divergence trap (src/compile/extensions/exec_context/mod.rs, ExecContextExtension::new)

The any_contributor_active pre-computation in new() replicates each contributor's should_activate logic verbatim. The MAINTENANCE comment is good, but this pattern means a new contributor can be wired into contributors() and should_activate while its bash allow-list contribution is silently suppressed because any_contributor_active wasn't updated. That's exactly the footgun the comment warns about.

A lightweight guard: add a test that constructs ExecContextExtension::new() and a CompileContext for every contributor's "active" scenario, then asserts required_bash_commands() is non-empty — same input, both code paths exercised. That would catch a divergence immediately rather than at E2E time. As written, the nine tests don't cover this since they use compile-fixture paths that go through prepare_steps, not required_bash_commands independently.

---

Silent infra failure path in set -uo pipefail (no -e) (src/compile/extensions/exec_context/pr.rs, bash body)

Without -e, an early infrastructure failure (e.g. mkdir -p "$AW_PR_DIR" — unlikely but possible if BUILD_SOURCESDIRECTORY resolves to a read-only path) lets execution continue silently, fail() then can't write error.txt, and the step exits 0 with nothing staged and nothing in the agent prompt. The agent receives no context and no failure signal.

A simple guard at the top — mkdir -p "$AW_PR_DIR" || { echo "[aw-context] could not create aw-context dir"; exit 1; } — would surface this loudly. Unlike the merge-base failure (which is a normal operational case warranting the fail() soft path), a missing output directory is always a broken pipeline.

---

✅ What Looks Good

• Trust boundary is excellent. Bearer injected via GIT_CONFIG_* env (never argv), SYSTEM_ACCESSTOKEN scoped to the single prepare step's env: block, no persistCredentials: true. The YAML-walking regression test in test_execution_context_pr_does_not_leak_system_accesstoken is exactly the right kind of safety net.
• BASE_SHA consistency is correct. Both the synthetic-merge path (git merge-base MERGE_P1 MERGE_P2) and the progressive-deepening path (git merge-base origin/$target HEAD) use true merge-base semantics. The regression guard test_execution_context_pr_synthetic_merge_uses_merge_base is a good shield against future simplification.
• Progressive deepening loop is right. The break condition fires only when git merge-base actually resolves — not on the first successful fetch — so the agent always gets the true common ancestor or an explicit failure, never a stale shallow-clone "merge-base".
• printf '%s' throughout. The fail-path message uses printf '%s' for all four identifier substitutions, so even an ADO var that somehow contained %n or similar couldn't cause format-string trouble.
• Contributor/extension separation is clean. The private ContextContributor trait + Contributor enum mirrors the Extension enum pattern exactly; plugging in a future ScheduleContributor will be mechanical.

Generated by Rust PR Reviewer for issue #865 · sonnet46 3.8M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-designed, with excellent security posture and thorough test coverage. One defense-in-depth gap in a regex and a cosmetic EOF issue worth fixing before merge.

Findings

⚠️ Suggestions

• src/compile/extensions/exec_context/pr.rs — PR_TARGET_BRANCH regex allows .. sequences

  The validation regex is ^[A-Za-z0-9._/-]+$, which accepts values like ../evil or feature..name. Both git fetch and git merge-base reject ..-containing refspecs as "invalid refspec" / "Not a valid object name" respectively, so the attack fails gracefully into fail() — there is no security breach. But the PR's stated posture is "defence-in-depth with the same strictness as the other identifiers", and those identifiers don't allow ... Tightening the regex to explicitly exclude .. would be consistent with that posture:

  # Current
  if [[ ! "$PR_TARGET_BRANCH" =~ ^[A-Za-z0-9._/-]+$ ]]; then
  
  # Suggestion — add a secondary check that rejects ".." sequences
  if [[ ! "$PR_TARGET_BRANCH" =~ ^[A-Za-z0-9._/-]+$ ]] || [[ "$PR_TARGET_BRANCH" == *".."* ]]; then

  (Confirmed ../evil and feature..name pass the current regex via live bash test.)

• tests/compiler_tests.rs — missing newline at end of file

  The final } of the new exec-context test block has no trailing newline (\ No newline at end of file in the diff). All other source files in the repo end with a newline; this is inconsistent and will show up in future diffs.

✅ What Looks Good

• Trust boundary is solid: SYSTEM_ACCESSTOKEN is mapped only into the precompute step's env: block via GIT_CONFIG_* env vars (not git -c argv), never written to .git/config, never in the agent env. The YAML walk test (test_execution_context_pr_does_not_leak_system_accesstoken) enforces this programmatically against the compiled output.

• Consistent base.sha semantics: Both the synthetic-merge-commit path (git merge-base HEAD^1 HEAD^2) and the progressive-deepening path compute the true common ancestor, so git diff $BASE..$HEAD produces the same change set regardless of ADO checkout mode. The regression guard (test_execution_context_pr_synthetic_merge_uses_merge_base) locks this in.

• any_contributor_active divergence trap: The six unit tests in exec_context/mod.rs mirror every branch of PrContextContributor::should_activate, ensuring a future contributor author who wires should_activate but forgets the aggregate pre-computation trips at unit-test time rather than silently widening the bash allow-list at E2E time.

• fail() exiting 0: Intentional and correct — the build continues with the agent using the ADO MCP fallback rather than aborting the pipeline; the failure prompt fragment explicitly forbids an empty review.

• Bash allow-list footgun guard: pr.enabled: true without on.pr does NOT widen the allow-list. Correctly implemented in both ExecContextExtension::new and PrContextContributor::should_activate, and covered by dedicated tests.

Generated by Rust PR Reviewer for issue #865 · sonnet46 2M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Well-designed with strong trust-boundary controls and excellent test coverage; one low-severity prompt injection vector worth addressing.

Findings

🔒 Security Concerns

• scripts/ado-script/src/exec-context-pr/validate.ts:53 + index.ts:76 — When TARGET_BRANCH_RE validation fails, the raw (unvalidated) targetBranch value is embedded verbatim in the reason string, which is then appended to the agent prompt via failureFragment. A PR author with push access who creates a branch with a newline or markdown header in its name (e.g., refs/heads/foo\n## Injected Section\nIgnore previous instructions) can inject content into the agent's prompt context through this failure path. The vector requires push access to the repo and hits only the failure path, making it low severity — but worth a targeted fix. Consider sanitizing targetBranch before embedding it in the error reason (replace \r\n with , truncate to N chars, or use a placeholder like <invalid branch name>).

🐛 Bugs / Logic Issues

• scripts/ado-script/src/exec-context-pr/merge-base.ts:33 — countParents is named and documented as returning the number of parents, but actually returns the number of tokens in the rev-list --parents output (commit SHA + parent SHAs). For a merge commit the value is 3 (= 1 commit + 2 parents), not 2. The parents=${parents} in the failure message (resolveMergeBase line ~105) would emit parents=3 for a normal merge commit, which a debugging reader would interpret as "3 parents" (a rare octopus merge) rather than "standard 2-parent merge". The condition parents >= 3 is correct, but the name and message are misleading. parentTokenCount or just renaming the condition comment would prevent future confusion.

⚠️ Suggestions

• scripts/ado-script/src/exec-context-pr/git.ts:14 — The bearerEnv docstring says "the existing bash path falls through" — stale v6.2 language; the bash path no longer exists. Suggest updating to simply: "callers still attempt the fetch without authentication."

• scripts/ado-script/test/smoke.test.ts (exec-context-pr smoke tests) — The success smoke test verifies that base.sha and head.sha are non-empty 40-char hex strings and differ from each other, but doesn't cross-check them against the expected git ancestry. For makeSyntheticMergeRepo, the test could additionally verify head.sha == git rev-parse HEAD^2 and base.sha == git merge-base HEAD^1 HEAD^2 in the repo — a simple regression guard against SHA transposition or wrong-ref bugs.

• tests/fixtures/dedupe_gate_only.md — The fixture now disables exec-context to preserve the "download must land in Setup only" test's semantics. This means the inlined-imports: true + exec-context PR active combination (where the bundle download should appear in both Setup AND Agent jobs) has no test coverage. A new test verifying that download appears in the Agent job when inlined-imports: true and on.pr is configured without execution-context.pr.enabled: false would close this gap.

✅ What Looks Good

• Trust boundary is solid: SYSTEM_ACCESSTOKEN scoped to the single node exec-context-pr.js step, passed only to the spawned git child via GIT_CONFIG_* (not argv, not .git/config). The test_execution_context_pr_does_not_leak_system_accesstoken YAML-walk test is a strong regression guard.
• Progressive deepening logic is correct: the loop breaks only when git merge-base actually resolves (not just on a successful fetch), and base.sha semantics are identical between the synthetic-merge and deepening paths.
• Duplicate-activation safety: the divergence-trap unit tests in exec_context/mod.rs are a smart pattern for the dual pr_contributor_will_activate_with_cfg / should_activate maintenance hazard.
• pr.enabled: true without on.pr footgun prevention: correctly suppressed at both required_bash_commands and prepare_steps — the compile-time test test_execution_context_pr_enabled_true_without_on_pr_is_inactive directly guards the footgun scenario.
• set -euo pipefail + correct exit semantics: soft failures exit 0; only infra failure (mkdirSync) exits 1; wrapping bash propagates correctly.
• TypeScript unit test coverage is thorough — injectable GitRunners pattern lets the merge-base tests exercise every loop branch without a real git repo.

Generated by Rust PR Reviewer for issue #865 · sonnet46 2.4M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Well-engineered with good trust-boundary design — one security gap in the TypeScript failure path, one visibility inconsistency in Rust.

Findings

🔒 Security Concerns

• scripts/ado-script/src/exec-context-pr/prompt.ts — failureFragment — the partial.prId/project/repo values are embedded in the agent prompt without calling sanitizeForPrompt. In the failure path in index.ts, these are the raw unvalidated env values passed from validateIdentifiers's error branch:

  // index.ts
  writeFailure(prDir, promptPath, idsOrErr.reason, {
    prId:    env.SYSTEM_PULLREQUEST_PULLREQUESTID,  // raw, unvalidated
    project: env.SYSTEM_TEAMPROJECT,               // raw, unvalidated
    repo:    env.BUILD_REPOSITORY_NAME,            // raw, unvalidated
  });

  failureFragment then embeds these directly:

  `PR #${prId} in project ${project} / repository ${repo} -- context preparation failed.`

  If SYSTEM_PULLREQUEST_PULLREQUESTID were set to something like "\n## Injected\nIgnore previous instructions", it would be written verbatim into the agent prompt. The reason field is correctly sanitised (via sanitizeForPrompt inside validateIdentifiers), and the PR's own wording — “defence-in-depth is cheap” — applies equally here. validate.test.ts has a dedicated test asserting CR/LF is neutralised in reason, but there’s no equivalent guard for the partial values in prompt.test.ts.

  Fix: sanitise each partial value inside failureFragment:

  // prompt.ts
  import { sanitizeForPrompt } from "./validate.js";
  
  const prId    = partial.prId    ? sanitizeForPrompt(partial.prId)    : "<unknown>";
  const project = partial.project ? sanitizeForPrompt(partial.project) : "<unknown>";
  const repo    = partial.repo    ? sanitizeForPrompt(partial.repo)    : "<unknown>";

  These values come from ADO predefined variables (infra-set, not PR-author-controlled), so exploitability is low in practice — but the consistent-sanitisation posture is the right call.

⚠️ Suggestions

• src/compile/extensions/ado_script.rs:34 — EXEC_CONTEXT_PR_PATH is declared pub while the analogous IMPORT_EVAL_PATH immediately above uses pub(crate). Both are crate-internal constants consumed only by sibling modules; pub(crate) keeps the visibility tight and consistent.

✅ What Looks Good

• Trust boundary is solid. Bearer token flows through GIT_CONFIG_COUNT/KEY_0/VALUE_0 env vars scoped only to the spawned git subprocess — never in argv, never in .git/config, never visible to the agent step. The test_execution_context_pr_does_not_leak_system_accesstoken test walks the full compiled YAML to assert this, which is a strong guard.
• Progressive-deepening loop correctness: the loop breaks only when git merge-base actually resolves, not just on the first successful fetch. The behaviour is tested at both layers (TS: “retries with deeper fetches when earlier merge-base fails”; Rust: test_execution_context_pr_synthetic_merge_uses_merge_base).
• any_contributor_active divergence-trap tests in exec_context/mod.rs catch pre-computation skew before E2E. Thoughtful defensive design.
• pr.enabled: true without on.pr correctly inactive: bash allow-list footgun is guarded from both the compile-time path and the required_bash_commands path.
• set -euo pipefail on the slim wrapper with node invocation is correct: the bundle exits 0 on every soft failure and reserves non-zero for true infra failures (e.g. unwritable workspace), matching the || exit 1 posture of the previous bash implementation.

Generated by Rust PR Reviewer for issue #865 · sonnet46 2.7M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid, well-thought-out design — one logic bug introduced in ado_script.rs, otherwise looks good.

---

Findings

🐛 Bugs / Logic Issues

• src/compile/extensions/ado_script.rs — required_hosts() not updated for exec_context_pr_active

  prepare_steps() correctly emits install_and_download_steps() (which curls from github.com) whenever exec_context_pr_active is true, but required_hosts() was not updated to match:

  // current (incomplete after this PR):
  if self.has_gate() || self.runtime_imports_active() {
      vec!["github.com".to_string()]
  } else {
      vec![]  // ← wrong when exec_context_pr_active && !import_active && !has_gate
  }

  The exact scenario is the new dedupe_exec_context_pr_only.md fixture (inlined-imports: true + on.pr + no filters): prepare_steps downloads the bundle, but required_hosts claims no download happens. This is currently masked because github.com is in CORE_ALLOWED_HOSTS, so the AWF network policy doesn't block it — but the comment "Only request github.com when the bundle is actually downloaded" is now a lie, and the invariant will silently break if github.com is ever removed from the core list (e.g. for a locked-down deployment target).

  Fix:

  if self.has_gate() || self.runtime_imports_active() || self.exec_context_pr_active {
      vec!["github.com".to_string()]
  } else {
      vec![]
  }

  Missing test: The ext_with helper hardcodes exec_context_pr_active: false, so there's no unit test for required_hosts returning github.com in this new code path. A new test variant analogous to required_hosts_requests_github_when_runtime_imports_active would close the gap:

  #[test]
  fn required_hosts_requests_github_when_exec_context_pr_active() {
      let mut ext = ext_with(None, None, true); // inlined-imports: true, no gate
      ext.exec_context_pr_active = true;
      assert_eq!(ext.required_hosts(), vec!["github.com".to_string()]);
  }

---

⚠️ Suggestions

• src/compile/extensions/exec_context/pr.rs — module comment slightly overstates token isolation

  The comment says the bearer is "confined to the git subprocess's env exclusively." It's actually in the Node process's process.env too (the step's env: block sets it, and Node inherits it). The correct claim is "never in the agent step's env, never in argv, never written to .git/config." The key trust properties ARE correctly maintained — this is just a comment precision issue.

• scripts/ado-script/src/exec-context-pr/validate.ts — PROJECT_RE may reject legitimate ADO project names

  PROJECT_RE = /^[A-Za-z0-9._ -]+$/ excludes characters that ADO permits in project names (e.g. +, (, )). The consequence is a graceful degradation to the failure fragment rather than a security issue, but it may surprise users with projects like My-Project (test). Worth a comment noting this is intentional conservatism.

---

✅ What Looks Good

• Trust boundary is rigorous: SYSTEM_ACCESSTOKEN scoped to a single step's env:, no persistCredentials: true, no .git/config writes, bearer in GIT_CONFIG_* not argv. The YAML trust-boundary regression test (test_execution_context_pr_does_not_leak_system_accesstoken) walks the parsed YAML tree rather than string-matching — that's the right approach.
• Activation guard is correctly implemented: on.pr is required for the PR contributor; pr.enabled: true alone on a non-PR-triggered agent does nothing. The divergence-trap tests in mod.rs guard this across future contributors.
• sanitizeForPrompt usage is thorough: every user-controlled value going into the agent prompt (failure path) is sanitised before embedding. The adversarial-newline injection test in validate.test.ts is a good signal.
• Progressive deepening loop is correct: only breaks when merge-base actually resolves, not on the first successful fetch. The vitest stubs make the loop behaviour directly verifiable.
• Stale artifact cleanup (rmSync loop at startup) handles re-runs safely with force: true.
• appendFileSync creates the file if absent — the agent prompt path fallback is safe even if base.yml's "Prepare agent prompt" step hasn't run yet.
• Smoke test cross-checks SHAs against git's own output rather than just asserting 40 hex chars — catches SHA transposition bugs (e.g. HEAD^1/HEAD^2 swap).

Generated by Rust PR Reviewer for issue #865 · sonnet46 4.1M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid implementation — trust boundary is well-maintained, correctness guardrails are excellent, and the v7 migration to a TypeScript bundle is a clear improvement. A few minor issues worth addressing.

Findings

🐛 Bugs / Logic Issues

• merge-base.ts — resolveMergeBase writes baseSha/headTipSha to disk without validating they are 40-char hex SHAs. The smoke test asserts expect(baseSha).toMatch(/^[a-f0-9]{40}$/) after reading from disk, but the production function itself has no inline guard. If git merge-base or git rev-parse unexpectedly outputs multi-line or abbreviated content (e.g. misconfigured core.abbrev, unusual git config), an invalid SHA would be silently staged and the agent's git diff $BASE..$HEAD would error. Adding a /^[0-9a-f]{40}$/i assertion in resolveMergeBase before return { ok: true, ... } would close this gap before the smoke test catches it.

⚠️ Suggestions

• exec_context/mod.rs — pr_contributor_will_activate clones execution_context unnecessarily:

  let cfg = front_matter.execution_context.clone().unwrap_or_default();

  Since pr_contributor_will_activate_with_cfg takes &ExecutionContextConfig, a borrow avoids the allocation:

  let default = ExecutionContextConfig::default();
  let cfg = front_matter.execution_context.as_ref().unwrap_or(&default);

  Minor, but this is called on every collect_extensions invocation.

• exec_context/contributor.rs — Both name() and agent_env_vars() carry #[allow(dead_code)], meaning zero callers exist today. The trait forces every future contributor to implement two no-op methods. Consider providing default implementations (fn name(&self) -> &str { "unknown" } / fn agent_env_vars(&self) -> Vec<(String, String)> { vec![] }) so future contributors aren't required to write boilerplate for unused methods.

• exec-context-pr/index.ts — AW_AGENT_PROMPT_FILE is read from process.env as a test seam but is undocumented at the step boundary. The pr.rs prepare-step env: block does not restrict which env vars Node inherits, so an ADO pipeline variable named AW_AGENT_PROMPT_FILE would silently redirect where the prompt fragment is appended. Extremely low-risk (requires pipeline-variable write access), but a short comment in index.ts noting this is a test-only override would help future readers.

✅ What Looks Good

• Trust boundary: SYSTEM_ACCESSTOKEN is confined to the exec-context step's own env: block. The test_execution_context_pr_does_not_leak_system_accesstoken YAML-walker test is an excellent regression guard.
• Progressive deepening: the loop in resolveMergeBase correctly breaks only when git merge-base resolves — not on the first successful fetch. The v6.2 correctness fix is faithfully reproduced.
• Prompt-injection defence: sanitizeForPrompt is applied to every unvalidated env-var value on the failure path before it touches the agent prompt; validated identifiers flow through the type system.
• Divergence-trap unit tests (exec_context/mod.rs): independently testing required_bash_commands() against each activation scenario is a smart safeguard against future contributors updating should_activate without updating any_contributor_active.
• bearerEnv subprocess isolation: bearer in spawned git child's env via GIT_CONFIG_*, not in process.env and not in argv — strictly better than the v6.2 bash approach.
• Contributor enum / static dispatch: clean pattern matching the existing Extension enum; no Box<dyn> overhead.

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