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

[jamesadevine]

Summary

Adds an always-on ExecContextExtension that precomputes focused PR execution-context signals for the agent, appends a tailored prompt fragment, and avoids per-workflow reinvention of merge-base/shallow-clone handling.

The contributor remains PR-gated, trust-boundary-safe (SYSTEM_ACCESSTOKEN only in the precompute step), and stages aw-context/pr/base.sha + aw-context/pr/head.sha so agents can run git diff $BASE..$HEAD directly in AWF.

As part of resolving merge conflicts with origin/main, extension collection was reconciled so both always-on extensions are preserved together:

• ExecContextExtension
• AzureCliExtension

Related collect_extensions tests were updated to reflect the combined always-on extension set and expected counts.

A follow-up merge from origin/main was also applied to resolve additional conflicts, including AGENTS.md; this follow-up did not change runtime behavior of the feature.

Test plan

• cargo build — clean.
• cargo clippy --all-targets --all-features — clean.
• cargo test — fails on test_execution_context_pr_does_not_leak_system_accesstoken (pre-existing failure observed during conflict-resolution validation).
• cargo test test_collect_extensions_ -- --nocapture — targeted extension-order/count tests pass.
• parallel_validation run:
  • Code Review completed
  • CodeQL: 0 alerts

[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 · ◷

[jamesadevine]

@copilot resolve the merge conflicts in this pull request

[Copilot]

@copilot resolve the merge conflicts in this pull request

Resolved by merging origin/main and fixing the conflicts in src/compile/extensions/mod.rs and src/compile/extensions/tests.rs in commit a23bcda.

[jamesadevine]

@copilot more conflicts to resolve!

[Copilot]

@copilot more conflicts to resolve!

Resolved the new merge conflicts by merging origin/main and fixing AGENTS.md in commit 69d9b84.