fix(ado-script): fail closed for unresolved synthetic PR merge-base

[Copilot]

Summary

Synthetic PR checkouts could report a plausible but wrong base.sha when git merge-base HEAD^1 HEAD^2 failed in a shallow clone, silently falling back to the target parent.

• Merge-base resolution

  • Removes the HEAD^1 success fallback.
  • Retries synthetic parent merge-base resolution after progressively deepening both target and source refs.
  • Falls into the existing PR-context failure path if no true merge-base is found.

• Synthetic PR wiring

  • Validates System.PullRequest.SourceBranch.
  • Passes hoisted AW_PR_SOURCEBRANCH into the PR execution-context step for synthetic PR builds.

• Regression coverage

  • Covers successful deepening of both parents.
  • Covers fail-closed behavior when ancestry remains unavailable.

// Before: merge-base failure reported target parent as base
baseSha = mergeBase.length > 0 ? mergeBase : p1;

// After: deepen both refs, then only succeed with a real merge-base

Test plan

• cargo build
• cargo test
• cargo clippy --all-targets --all-features
• npm run typecheck
• npm run test
• npm run test:smoke

[jamesadevine]

/rust-review

[github-actions]

✅ Rust PR Reviewer completed successfully!

[github-actions]

🔍 Rust PR Review

Summary: Looks good — the fail-closed fix is correct and the security-sensitive refspec interpolation path is properly validated. One minor unit-test coverage gap noted.

Findings

⚠️ Suggestions

• src/compile/extensions/exec_context/mod.rs:999 — The unit test test_synth_pr_step_env_reads_hoisted_aw_vars only hoists ["AW_PR_ID", "AW_PR_TARGETBRANCH", "AW_SYNTHETIC_PR"] into the synthetic Agent-job variables and only asserts SYSTEM_PULLREQUEST_TARGETBRANCH. With the new pr.rs change, SYSTEM_PULLREQUEST_SOURCEBRANCH: $(AW_PR_SOURCEBRANCH) is now emitted, but this unit test won't exercise that path (the integration test in compiler_tests.rs does cover it). Suggest adding AW_PR_SOURCEBRANCH to the hoist list and an assertion alongside the SYSTEM_PULLREQUEST_TARGETBRANCH check for completeness.

• scripts/ado-script/src/shared/merge-base.ts:138 (ensureSyntheticMergeParentsFetched) — The !targetFetched && !sourceFetched → continue guard is safe in practice (if both --unshallow fetches fail the repo is already fully fetched, and prior successful depths would have found the merge-base if it existed), but the intent isn't obvious. A one-line comment explaining why skipping the merge-base check is correct here would help future readers avoid second-guessing it.

✅ What Looks Good

• Fail-closed correctness: The old baseSha = mergeBase.length > 0 ? mergeBase : p1 silent fallback is cleanly removed. When ensureSyntheticMergeParentsFetched returns ok: false, baseSha stays "" and resolveMergeBase returns { ok: false, reason: "Could not resolve base/head SHAs..." } — exactly what the PR claims.
• Security: PR_BRANCH_RE (/^[A-Za-z0-9._/-]+$/) correctly validates both target and source branches before they're interpolated into git refspecs. The rename from TARGET_BRANCH_RE is clean and the test cases (empty, disallowed chars) are thorough.
• AW_PR_SOURCEBRANCH wiring: The variable was already hoisted in ado_script.rs; pr.rs adding SYSTEM_PULLREQUEST_SOURCEBRANCH to the step env is a single-line, symmetric change that closes the gap.
• Tests: The two new vitest tests (deepens target and source refs and fails closed when...) are well-structured — the deepening test verifies both the correct fetch arguments and the return value; the fail-closed test counts exactly 8 fetches (4 depths × 2 branches), leaving no ambiguity about loop termination.
• Smoke test updated: Adding SYSTEM_PULLREQUEST_SOURCEBRANCH to the smoke env is correct; without it the bundle would now fail validation and the smoke would give a false positive.

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