feat(compile): decouple create-pull-request diff base from checkout depth

[jamesadevine]

Summary

Closes #1413.

On an ADO agent pool whose default git fetch is shallow (fetchDepth: 1), a bare checkout leaves no refs/remotes/origin/<target> ref. The create-pull-request diff base is computed at agent time by the host-side SafeOutputs MCP server (src/mcp.rs::find_merge_base), which only inspects existing local refs and never fetches — so it cannot compute a merge base and the PR can't be opened:

Cannot determine diff base: no remote tracking branch found. This can happen
with shallow clones (fetchDepth: 1). Ensure the pipeline fetches full history ...

The only prior workaround was hand-editing the compiled .lock.yml to force fetchDepth: 0, which then failed the runtime "Verify pipeline integrity" step and forced ado-aw-debug.skip-integrity: true.

Approach (gh-aw style)

github/gh-aw keeps checkout shallow and does the heavy git deepening in a credentialed step, not the sandboxed agent. This PR adapts that model to ADO: whenever create-pull-request is configured, the compiler emits a credentialed Agent-job prepare step that runs on the host (before the AWF-wrapped agent, using $(System.AccessToken)) and, for each allowed create-PR repo dir, fetches + progressively deepens that repo's target branch into refs/remotes/origin/<target> and points refs/remotes/origin/HEAD at it. mcp.rs::find_merge_base then resolves a base with no in-sandbox network, no forced full-history checkout, and no lock hand-edit (integrity check stays on). It reuses the exact fetch/deepen logic already used by the PR execution-context precompute (shared/merge-base.ts::ensureTargetRefFetched). No new front-matter is required for the base case — automatic when create-pull-request is present.

Verified: the SafeOutputs MCP server runs as a host process (nohup ado-aw mcp-http … "{working_directory}"), not inside the AWF sandbox, and its bounding_directory == the working_directory checkout the prepare step deepens.

Multi-repo coverage

An agent can call create-pull-request with repository: <alias> for any checked-out repo. The prepare step deepens the target branch in every git dir mcp.rs::resolve_git_dir_for_patch would read — working_directory (self) + working_directory/<alias> for each checkout: alias — so a PR to any allowed repo works on shallow pools. The bundle consumes repeated --repo-dir <dir> --target-branch <branch> pairs with isolated per-dir failures.

Per-repo target branches (meta-checkout model)

A meta repo that checks out N repos can open a PR into each repo's own target branch. create-pull-request gains (collision-free — target-branch stays a plain literal):

• target-branch — literal fallback (default main), unchanged.
• target-branches — optional per-alias literal overrides (highest precedence).
• infer-target-from-checkout-ref — opt-in bool: a checkout repo with no explicit override targets its own repos: ref. self never infers.

Resolution per repo R: target-branches[R] → (if the bool) R's checkout ref → target-branch → main. The same resolver drives the compiler's per-dir deepening and Stage 3's PR creation, so the branch that is fetched/deepened always matches the branch the PR targets. target-branch is a plain literal (never a directive), so no branch name can be misread.

Branch semantics: the deepened branch is each repo's destination/base (target-branch), not the per-repo repos: checkout ref (the source side).

Key changes

• prepare-pr-base ado-script bundle — loops over (dir, target) pairs, deepening each via the shared ensureTargetRefFetched helper (extracted from resolveMergeBase); per-dir failures isolated.
• ado_bundle.rs — Bundle::PreparePrBase (BundleAuth::Bearer).
• ado_script.rs — PREPARE_PR_BASE_PATH, prepare_pr_base_active, prepare_pr_base_step_typed(&[(dir, target)]) (double-quoted ADO-macro dir + single-quoted literal branch, shellcheck-clean; SYSTEM_ACCESSTOKEN via apply_bundle_auth).
• create_pull_request.rs — CreatePrConfig gains target_branches + infer_target_from_checkout_ref + resolve_target_branch(); Stage 3 resolves per self.repository.
• result.rs / main.rs — ExecutionContext.repo_refs (alias → checkout ref), populated at Stage 3 from front_matter.repositories.
• types.rs / agentic_pipeline.rs — create_pr_config() (sanitized) + checkout_repo_refs(); the compiler builds the per-dir (dir, target) list via the shared resolver.
• mcp.rs — regression test proving find_merge_base resolves via the origin/HEAD symbolic ref.
• Docs — safe-outputs.md, ado-script.md, execution-context.md, AGENTS.md + site mdx mirrors.

Test plan

• cargo clippy --all-targets --all-features — clean.
• cargo test — 2513 unit + all integration suites green (0 failed), incl. the resolver precedence matrix, per-repo-target compile-integration (meta repo, diverse refs), multi-repo coverage, types.rs default/drift-guard, mcp symbolic-ref regression, and bash_lint_tests (shellcheck).
• ado-script npm test — prepare-pr-base + merge-base suites green (155 TS tests); bundle builds to the ado-script.zip root.
• Self-only agents and agents without create-pull-request compile with exactly the expected step (self-only ⇒ one --repo-dir/--target-branch pair).

[jamesadevine]

@copilot resolve conflicts

[Copilot]

@copilot resolve conflicts

Done. Merged origin/main into the branch. The only conflict was in AGENTS.md where both branches added an entry in the scripts/ado-script/src/ directory listing — resolved by keeping both the prepare-pr-base/ entry (from this PR) and the executor-e2e/ entry (from main). Merge commit: 06c435e.

[jamesadevine]

/rust-review

[github-actions]

✅ Rust PR Reviewer completed successfully!

[github-actions]

🔍 Rust PR Review

Summary: Solid implementation — the approach is correct and well-tested. Two minor findings worth addressing.

Findings

⚠️ Suggestions

• src/compile/types.rs:1246 — Implicit default branch is untested end-to-end.
  create_pr_target_branch() correctly falls back to "main" via .unwrap_or("main") when the target-branch key is absent from the config (e.g. create-pull-request: {} or create-pull-request:\n draft: true). However, both integration tests in compiler_tests.rs that touch this codepath explicitly set target-branch: main, so the .unwrap_or fallback path has no coverage. A test case like:

  "safe-outputs:\n  create-pull-request: {}\n"

  would make the contract explicit and catch any future regression in the default-branch logic.

• src/compile/types.rs:1246 vs src/safe_outputs/create_pull_request.rs:462 — Two independent "main" defaults.
  create_pr_target_branch() defaults to the hard-coded string "main", while CreatePrConfig has a separate default_target_branch() function. The PR description acknowledges these are manually kept in sync. Adding a compile-time assertion in the types.rs tests would turn a silent drift into an immediate failure:

  assert_eq!(
      crate::safe_outputs::create_pull_request::CreatePrConfig::default().target_branch,
      "main",
      "default must match create_pr_target_branch fallback"
  );

  Without this, a rename of the default branch in CreatePrConfig would produce a prepare step that fetches the wrong ref — the agent would succeed but find_merge_base would still fail.

✅ What Looks Good

• Shell injection is correctly handled. sh_single_quote uses POSIX '\'' escaping — branch names with metacharacters ($(...), backticks, ${}) are fully neutralised. The ADO bearer rides in masked env (SYSTEM_ACCESSTOKEN) not argv, mirroring bearerEnv in the TypeScript layer.
• prepare_pr_base_active / step-emit coupling is tight. Both flags are derived from the same create_pr_target_branch() call on the same immutable FrontMatter — no independent boolean can diverge.
• Graceful degradation. The TypeScript bundle exits 0 on benign fetch failures so the agent still runs; mcp.rs surfaces its own diff-base error if genuinely unresolvable. Correct posture.
• Regression test in mcp.rs is well-constructed. The test_find_merge_base_resolves_via_origin_head_symbolic_ref test uses a real git repo in a tempdir, sets up the exact ref layout the bundle would produce, and verifies the symbolic-ref probe path — exactly the scenario that was broken before this PR.
• Test coverage breadth. 4 unit tests in ado_script.rs + 3 integration tests in compiler_tests.rs (including the critical "absent create-pull-request" negative case) + TypeScript bundle tests cover all the main paths.

Generated by Rust PR Reviewer for #1425 · 97.1 AIC · ⌖ 6.65 AIC · ⊞ 6.3K · ◷
_{Comment /rust-review to run again}

[github-actions]

🔍 Rust PR Review

Summary: Looks good — solid implementation with thorough test coverage; one minor doc formatting bug and one edge case worth noting.

Findings

⚠️ Suggestions

• docs/ado-script.md ASCII tree — github-app-token/ still has └── in the tree, but prepare-pr-base/ is now added as a sibling after it. The connector should be changed to ├── (only the final sibling uses └──). Purely cosmetic, but the rendered tree will look broken.

  # Current (wrong):
  │   └── github-app-token/
  │       └── __tests__/
  │   └── prepare-pr-base/    ← second └── at same level
  
  # Should be:
  │   ├── github-app-token/
  │       └── __tests__/
  │   └── prepare-pr-base/
  

• src/compile/types.rs — create-pull-request: null activates the prepare step — create_pr_target_branch() uses .map(|v| ...) on the presence of the key, so safe-outputs: create-pull-request: null returns Some("main") and emits the prepare step even though the safe output is disabled. This is a minor edge case (authors wouldn't normally write null), but the create-pull-request: {} empty-map case has an explicit test while null doesn't. A guard like if v.is_null() { return None; } at the top of the closure would cover it.

✅ What Looks Good

• Bearer security: SYSTEM_ACCESSTOKEN is injected via apply_bundle_auth as a masked env (EnvValue::Secret), and the TypeScript passes it to git exclusively via GIT_CONFIG_* env vars (bearerEnv) — never in argv or written to .git/config. Correct trust boundary.
• Shell injection protection: sh_single_quote on target_branch is correct — the '\''-escaping technique properly handles embedded single quotes, including adversarial inputs like '; rm -rf /'.
• ensureTargetRefFetched extraction: functionally identical to the inlined loop in resolveMergeBase — same depth sequence, same early-exit logic. Clean refactoring with no behaviour change.
• Single source of truth: FrontMatter::create_pr_target_branch() keying the drift-guard off CreatePrConfig::default().target_branch is a good pattern; the explicit assertion in the test will catch any future rename.
• Benign-failure posture: fetch failures exit 0 so the agent still runs; the MCP server surfaces its own diff-base error only if truly unrecoverable. Correct graceful degradation.
• Test coverage: the TypeScript unit suite, Rust unit tests (prepare_pr_base_step_emits_bundle_invocation_with_target_and_bearer, declarations_agent_prepare_download_fires_when_only_prepare_pr_base_active), integration tests (explicit/default/implicit/absent cases), and the mcp.rs symbolic-ref regression test together cover the critical paths well.

Generated by Rust PR Reviewer for #1425 · 65.7 AIC · ⌖ 6.47 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Two findings from the prior review run remain unaddressed; everything else is solid.

Findings

🐛 Bugs / Logic Issues

• src/compile/types.rs — create-pull-request: null activates the prepare step (logic error)
  create_pr_target_branch() calls .map() on the presence of the key, so when safe-outputs: create-pull-request: null (or bare create-pull-request: with no value) appears in front matter, self.safe_outputs.get("create-pull-request") returns Some(Value::Null), the closure runs, Value::Null.get("target-branch") returns None, .unwrap_or_else(...) produces "main", and the function returns Some("main") — triggering the prepare step for a disabled safe output.
  Fix: switch to and_then and add an early return:

  pub fn create_pr_target_branch(&self) -> Option<String> {
      self.safe_outputs.get("create-pull-request").and_then(|v| {
          if v.is_null() {
              return None;
          }
          Some(
              v.get("target-branch")
                  .and_then(|t| t.as_str())
                  .map(str::to_string)
                  .unwrap_or_else(|| {
                      crate::safe_outputs::CreatePrConfig::default().target_branch
                  }),
          )
      })
  }

  (Flagged in run 28977362835, still present.)

⚠️ Suggestions

• docs/ado-script.md — ASCII tree connector still broken at the src/ directory level
  The root-level .js file list was correctly updated (├── github-app-token.js), but the src/ directory entry for github-app-token/ still uses └── even though prepare-pr-base/ is now a sibling after it:

  # Current (wrong):
  │   └── github-app-token/    ← should be ├── (not the last sibling)
  │       └── __tests__/
  │   └── prepare-pr-base/
  

  Cosmetic, but renders a broken tree in any Markdown viewer.
  (Flagged in run 28977362835, still present.)

✅ What Looks Good

• Single source of truth: CreatePrConfig::default().target_branch via .unwrap_or_else + the compile-time drift guard assertion in types.rs tests fully address the first review's ask.
• Shell injection: sh_single_quote POSIX-escaping on the user-supplied branch name; bearer rides exclusively in masked env (SYSTEM_ACCESSTOKEN), never in argv.
• Step ordering: prepare fires post-checkout, pre-Copilot — correct placement.
• prepare_pr_base_active gating: tight, correctly OR-composed into the download predicate in ado_script.rs.
• mcp.rs regression test: real git repo in a tempdir, sets up the exact ref layout the bundle produces, verifies the symbolic-ref resolution path — exactly the right kind of test for this fix.

Generated by Rust PR Reviewer for #1425 · 68.8 AIC · ⌖ 6.45 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid, well-designed implementation — the approach is sound and test coverage is thorough. Two minor issues worth addressing; everything else is done correctly.

Findings

🐛 Bugs / Logic Issues

• src/safe_outputs/create_pull_request.rs:484–488 — short_branch returns "" for "refs/heads/": The Rust implementation of short_branch doesn't guard against the degenerate case where stripping refs/heads/ yields an empty string:

  pub(crate) fn short_branch(git_ref: &str) -> &str {
      git_ref.strip_prefix("refs/heads/").unwrap_or(git_ref)  // returns "" for "refs/heads/"
  }

  The TypeScript counterpart in prepare-pr-base/index.ts explicitly guards: return s.length > 0 ? s : "main". If infer-target-from-checkout-ref: true and a repos: entry has ref: refs/heads/ (malformed but parseable), the Rust path in Stage 3 (resolve_target_branch) would return "", causing target_ref = "refs/heads/" and an ADO API error at PR creation time. Simple fix: git_ref.strip_prefix("refs/heads/").filter(|s| !s.is_empty()).unwrap_or(git_ref).

⚠️ Suggestions

• src/compile/agentic_pipeline.rs:961 — alias embedded in double-quoted bash arg without character validation: The per-checkout-alias dirs are built as format!("{}/{}", cfg.working_directory, alias) and then double-quoted verbatim in the emitted bash script. If a user-provided explicit alias= shorthand contains ", $, or a newline, the double-quoted shell string breaks. derive_alias (last segment of org/repo) is safe in practice, but the explicit-alias path through parse_shorthand has no character allowlist. This is the first place in the codebase that embeds an alias directly into a bash command-line argument; all previous uses go through YAML checkout: values. Suggest adding a compile-time validation in lower_repos (alongside the existing reserved-name and duplicate checks) that rejects aliases containing shell-unsafe characters (", $, `, \, newlines). Or apply sh_single_quote to the alias segment while keeping $(Build.SourcesDirectory) double-quoted separately.

• src/safe_outputs/create_pull_request.rs — target_branches keys are not sanitized: The #[derive(SanitizeConfig)] macro sanitizes HashMap values by default and requires #[sanitize_config(sanitize_keys)] to also sanitize keys. The new target_branches: HashMap<String, String> field has its values sanitized correctly but keys (repo aliases) are not. Keys are only used for lookup so there's no direct security exposure, but it's inconsistent with the project's sanitization discipline — an alias key from an attacker-influenced NDJSON replay could contain ##vso[ and survive unsanitized. Consider adding #[sanitize_config(sanitize_keys)] to target_branches.

✅ What Looks Good

• Shared resolver eliminates drift: CreatePrConfig::resolve_target_branch is called by both the compiler (to know which branch to deepen) and Stage 3 (to know which branch to target for the PR). The deepened branch and the PR base branch are guaranteed identical — the central design concern of this feature.
• Shell quoting discipline: sh_single_quote is correctly applied to branch literals (shadow-proof), and ADO macro paths are double-quoted following the existing import.js convention. SYSTEM_ACCESSTOKEN is projected via apply_bundle_auth (never in argv).
• TypeScript refactor: Extracting ensureTargetRefFetched from resolveMergeBase is clean — both callers now exercise the same fetch/deepen loop with the same GitRunners injection interface, making the test stubs valid for both.
• Per-dir failure isolation: Benign fetch failures are logged and skipped without hard-failing the step; only the agent run itself will surface the diff-base error if truly unrecoverable — correct posture.
• Test coverage: The resolver precedence matrix, multi-repo compile integration (5 fixture variants), mcp symbolic-ref regression, and bash_lint_tests coverage together give good confidence.

Generated by Rust PR Reviewer for #1425 · 95.9 AIC · ⌖ 6.54 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — the core security invariants are sound and the architecture is clean. Two findings worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/main.rs ~line 853 — ctx.repo_refs is populated with logic identical to FrontMatter::checkout_repo_refs() but written inline instead of calling the method. If the method's filtering logic is ever updated, the Stage 3 runtime path silently diverges from compile time. Should be ctx.repo_refs = front_matter.checkout_repo_refs();.

  // current (duplicated):
  ctx.repo_refs = front_matter
      .repositories
      .iter()
      .filter(|r| front_matter.checkout.iter().any(|a| a == &r.repository))
      .map(|r| (r.repository.clone(), r.repo_ref.clone()))
      .collect();
  
  // should be:
  ctx.repo_refs = front_matter.checkout_repo_refs();

• src/compile/types.rs create_pr_config() — serde_json::from_value(v.clone()).unwrap_or_default() silently falls back to CreatePrConfig::default() on any deserialization failure (e.g. user writes target-branches: "not-a-map"). At compile time the prepare step is still emitted with default values; Stage 3's get_tool_config presumably errors on the same malformed input, creating an asymmetry that makes these cases hard to debug. Consider bail!-ing (or at minimum a compile warning) when deserialization fails rather than swallowing it silently.

⚠️ Suggestions

• scripts/ado-script/src/prepare-pr-base/index.ts shortBranch — TS and Rust diverge for empty-string input: shortBranch("") → "main" (TS, via "" || "main"), short_branch("") → "" (Rust). The comment says they're "in lock-step" but they aren't for this edge case. Not reachable in practice (the compiler never emits --target-branch ""), but the comment misleads future readers. Either align the implementations or drop the lock-step claim.

✅ What Looks Good

• Alias validation (src/compile/common.rs): is_safe_path_segment check is the right chokepoint — fires at compile time in lower_repos, before any generated shell args are produced. The [A-Za-z0-9._-] + no-leading-dot allowlist correctly blocks every shell metacharacter that double-quoted bash embedding could expose.
• sh_single_quote quoting: branch names are correctly POSIX single-quoted (with '\\'' escaping) in prepare_pr_base_step_typed, so no branch name can inject into the generated script.
• Bearer token delivery: bearerEnv threads the ADO token via GIT_CONFIG_* env vars instead of argv — consistent with the existing exec-context bundles.
• Benign-failure design in prepareOneRepo: per-dir failures are isolated (logged, exit 0), so an unreachable checkout alias doesn't block the agent run.
• resolve_target_branch is shared between compile time and Stage 3 — the branch the compiler deepens and the branch Stage 3 targets are resolved by the same function, eliminating drift.
• Test coverage: precedence matrix, quoting assertions, default() drift-guard, and the mcp.rs symbolic-ref regression test all cover the scenarios that matter.

Generated by Rust PR Reviewer for #1425 · 78.4 AIC · ⌖ 6.59 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Well-designed, thoroughly-tested. Two findings worth addressing — one minor inconsistency and one edge-case UX issue.

Findings

⚠️ Suggestions

• src/safe_outputs/create_pull_request.rs — target_branches values bypass SanitizeConfig

  #[sanitize_config(sanitize_keys)] sanitizes HashMap keys (repo aliases) but not values (branch names). The scalar target_branch: String is sanitized by the struct-level SanitizeConfig derive, creating an asymmetry: the per-repo overrides in target_branches reach refs/heads/{value} and the ADO API without the same normalization pass that target_branch goes through. Given the integrity-check trust model this is low risk in practice, but the inconsistency is a latent footgun if the sanitization contract changes.

  Suggested fix: add #[sanitize_config(sanitize_keys, sanitize_values)] (or however the derive supports sanitizing values) so the two code paths are symmetric.

• src/safe_outputs/create_pull_request.rs::resolve_target_branch — tag refs under infer-target-from-checkout-ref

  When infer_target_from_checkout_ref: true and a repo's repos: ref is a tag (e.g., refs/tags/v1.2.3), short_branch("refs/tags/v1.2.3") returns the original (strips only refs/heads/). Stage 3 then constructs target_ref = "refs/heads/refs/tags/v1.2.3", which ADO rejects with a generic API error — giving the author no indication of the misconfiguration.

  A targeted bail! or a compiler-side warning when infer_target_from_checkout_ref is true and a checkout ref doesn't start with refs/heads/ would surface this much earlier.

✅ What Looks Good

• Alias shell-safety validation (src/compile/common.rs): the new is_safe_path_segment guard in lower_repos is correctly placed at the single chokepoint that converts raw user input into ADO/shell constructs. The error message is actionable, and the positive + negative test matrix covers the key metacharacters.

• sh_single_quote properly implements the POSIX '...'/'\\'' escape pattern; tests assert both the default target and non-trivial branch names like release/2.x.

• Stage 3 / compiler symmetry: create_pr_config() in types.rs mirrors get_tool_config's serde_json::from_value + sanitize_config_fields() path exactly (including the same unwrap_or_default fallback), so the branch fetched/deepened by the prepare step always matches the branch Stage 3 uses as the PR target.

• ensureTargetRefFetched extraction from resolveMergeBase is clean. Progressive-deepening semantics are preserved verbatim, the shared helper keeps both call sites in lock-step, and the new tests verify per-depth short-circuit behavior.

• prepare-pr-base bundle regression test (src/mcp.rs): creating a real local git repo with a manually-set refs/remotes/origin/HEAD symbolic ref, then asserting find_merge_base resolves via it, directly exercises the shallow-pool path the PR fixes. Right kind of regression anchor.

• Non-fatal posture: per-dir fetch failures log + skip so one unreachable repo never blocks the agent run. main() always returns 0; only a genuinely unusable BUILD_SOURCESDIRECTORY would hard-fail.

Generated by Rust PR Reviewer for #1425 · 126.1 AIC · ⌖ 6.38 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — the approach is sound, security is solid, and the tests are comprehensive. One minor UX issue worth noting.

Findings

⚠️ Suggestions

• src/compile/types.rs + src/compile/extensions/mod.rs: create_pr_config() called twice per compilation

  collect_extensions() calls create_pr_config().is_some() to set prepare_pr_base_active, then build_agent_job() calls it again to get the actual config. Each call does a serde_json::from_value deserialization. If the config is malformed, the eprintln! warning fires twice — once during extension collection, once during job building.

  The duplication is a consequence of mirroring the github_app_token_active pattern (flag lives in extension, step lives in job builder). One lightweight fix would be to compute the flag from whichever parse already happened first:

  // in build_agent_job, compute once and reuse:
  if let Some(pr_cfg) = front_matter.create_pr_config() {
      // use pr_cfg here; also set prepare_pr_base_active from this value
  }

  Or accept the double call but note it in a comment so future maintainers don't wonder why warnings appear twice. The PR description already explains the symmetry intent, so this is low priority.

• src/compile/agentic_pipeline.rs:45: eprintln! for the non-branch ref warning

  The compile-time advisory for infer-target-from-checkout-ref on a non-branch checkout ref uses eprintln!(). Double-check this matches whatever diagnostic channel other compiler warnings use (some projects funnel these through a structured warning list rather than raw stderr so they appear consistently). If eprintln! is the established convention here, ignore this.

✅ What Looks Good

• Security: Alias validation added to lower_repos() via is_safe_path_segment() is the right chokepoint — it protects both the ADO checkout step and the double-quoted bash --repo-dir argument from shell metacharacters before either consumer can use the alias. sh_single_quote() on the target branch is correct. The #[sanitize_config(sanitize_keys)] on target_branches sanitizing both keys and values closes a plausible VSO-injection vector on per-repo overrides.

• Symmetry between Rust and TypeScript: short_branch() / shortBranch() handle the degenerate "refs/heads/" case identically (return original string, never ""), including in the tests. resolve_target_branch() and the Stage 3 executor use the same function — the deepened branch and the PR target branch cannot drift.

• Failure posture: prepareOneRepo returning false and the bundle exiting 0 is correct — a fetch failure on a weird pool shouldn't abort the agent run; it just means mcp.rs::find_merge_base will hit the existing error path. The isolation (per-dir failures don't block other dirs) is well-designed.

• Precedence matrix tests (test_resolve_target_branch_precedence, compiler integration tests for per-repo targets) cover all three levels and all edge cases (self never infers, unknown alias falls back to default, explicit wins over inference). The regression test in mcp.rs for find_merge_base via origin/HEAD symbolic ref is a clean, deterministic verification of the exact path the bundle enables.

• Token projection: Bearer goes through apply_bundle_auth → masked env var, never argv. The BundleAuth::Bearer classification for PreparePrBase is consistent with other network-calling bundles.

• Step ordering: Placed after checkout: self (git dir exists) and before the AWF-wrapped Copilot invocation (refs available when agent proposes PR). ✓

Generated by Rust PR Reviewer for #1425 · 92.3 AIC · ⌖ 9.38 AIC · ⊞ 6.2K · ◷