fix(ado-script): recover shallow Azure checkout merge bases

[jamesadevine]

Summary

Closes #1541.

• recover create-pull-request merge bases from Azure DevOps commonCommit / ahead / behind metadata while keeping shallow checkouts shallow
• use a bounded dual-ref fallback for non-ADO or unavailable REST paths, with no automatic full-history fetch
• split Agent patch-base preparation from SafeOutputs target-worktree preparation
• emit explicit fetchDepth: 0 when authors opt into full history
• add a path-filtered, scheduled Azure-native checkout regression pipeline and immutable Azure Repos fixture

Validation

• pre-fix Azure baseline: build 623611 reproduces the unresolved merge-base
• Azure regression suite: build 623630 passed all checkout scenarios
• cargo test --bin ado-aw compile
• cargo test --test bash_lint_tests
• cargo clippy --all-targets --all-features
• targeted ado-script tests, typecheck, full bundle build, and bundled smoke tests

Final Azure evidence

• Pre-fix baseline: build 623611 failed with the target-only shallow merge-base warning.
• Final PR-triggered run: build 623662 passed all five jobs.
• Final manual rerun: build 623668 passed from commit 252c41e6.
• GitHub PR checks, 1,314 Rust compile tests, bash lint, clippy, targeted TypeScript tests/typecheck, full bundle build, and post-hardening bundle smoke all pass.

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — well-structured refactor with solid input validation. A few items worth double-checking before merge.

Findings

🐛 Bugs / Logic Issues

• src/compile/types.rs — depth_for_emit() behavioral change is a breaking change for existing users. Old code: fetch_depth.filter(|d| *d != 0) → fetch-depth: 0 was silently suppressed (ADO pipeline setting remained in effect). New code: fetch-depth: 0 is now emitted as fetchDepth: 0 → full history. The old doc comment also said 0 meant "full history" so the old implementation was arguably buggy (it never actually gave full history). The fix is semantically correct, but if any existing consumer relied on fetch-depth: 0 as a no-op, this will now force a full git fetch. Worth a codemod or changelog note.

• scripts/ado-script/src/shared/merge-base.ts — ensureRefsForMergeBaseFetched, non-shallow path: sourceSpec is computed but never used. When isShallowRepository returns false, the function fetches only targetSpec. sourceSpec is constructed (including the validRef check and potential early return) but then ignored. A non-shallow repo already has the full source history locally, so omitting the source fetch is correct — but the dead variable is confusing and the early return on invalid sourceSpec fires even when the source fetch wouldn't happen. Consider if (!targetSpec) return { ok: false, ... } without the source check for the non-shallow branch, or add a comment explaining the omission.

• scripts/ado-script/src/shared/ado-client.ts:requireCount — dead value === undefined branch. !Number.isSafeInteger(undefined) is already true, so the explicit value === undefined check is unreachable. Not a runtime bug, but TypeScript's narrowing won't catch it and it looks like dead code.

🔒 Security Concerns

• merge-base.ts — SHA fetch via exact-commit refspec (+<sha>:ref) requires uploadpack.allowReachableSHA1InWant. When headSha is the sourceRef fallback and it's a bare SHA, the refspec +<sha>:refs/remotes/origin/ado-aw-prepare-source is generated. Azure Repos supports this, but if the guard logic in isCurrentAdoOrganization ever produces a false positive for a non-ADO remote, the fetch would fail with a cryptic server error. The existing sameOrgAdo gate makes this safe in practice — just worth a comment at the SHA branch of sourceRefspec to document the assumption.

• merge-base.ts — SOURCE_TRACKING_REF (refs/remotes/origin/ado-aw-prepare-source) persists across Agent runs. On a non-ephemeral agent pool with a workspace reuse policy, this ref will point to a previous run's source commit. The + force-push refspec always overwrites it, so it's not a correctness problem — but if a future caller reads the ref without a prior prepare step, it will see stale data silently. A comment noting the ref is ephemeral / always overwritten on use would help maintainers.

⚠️ Suggestions

• scripts/ado-script/src/prepare-pr-base/index.ts — preparePatchBase computes headSha before chdir, but chdir happens in main and runGit operates on CWD. So rev-parse HEAD after chdir is correct. However, headSha is computed at the top of preparePatchBase after chdir has already happened — the flow is fine, but worth an inline comment since the chdir is in the caller's loop.

• ado-remote.ts — collectionUri casing inconsistency for DefaultCollection. The hasDefaultCollection case emits (org.visualstudio.com/redacted) with capital DandC, but the input check is .toLowerCase() === "defaultcollection". Since collectionUriis only used inisCurrentAdoOrganization(which extracts the org and lowercases it), this is harmless — but ifcollectionUri` is ever surfaced directly (e.g., in logs or future REST calls), it will have a different casing than the original URL. Consider normalizing to whatever casing the original URL had.

• Drop of --unshallow: the removal is intentional and documented, but repos with > 2000 commit divergence will now fall through to a warning rather than recovering. The fetch-depth: 0 opt-in is the right escape hatch — just make sure the warning message in warnRepo points users there explicitly. Currently it says "Set this checkout's fetch-depth to 0 only if the repository can afford full history" — that's good, but could be more explicit: "Add fetch-depth: 0 under the relevant repos: entry in your workflow front matter."

✅ What Looks Good

• The two-phase split (patch-base in Agent job / target-worktree in SafeOutputs) is a clean architectural separation that avoids over-fetching in Stage 3.
• validRef is thorough — covers path-traversal via .., control characters, and all ADO-forbidden ref characters in one place.
• requireSha / requireCount validate REST responses before they reach the git refspec layer — good parse-don't-validate discipline.
• isCurrentAdoOrganization correctly gates ADO REST usage so non-ADO remotes silently fall back to the bounded git strategy without any error surfaced to users.
• The oneLine() sanitizer on all user-visible error messages prevents newline injection into ADO log commands.

Generated by Rust PR Reviewer for #1563 · 48 AIC · ⌖ 6.08 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-structured fix with correct fallback layering. A few minor things worth noting.

Findings

⚠️ Suggestions

• scripts/ado-script/src/shared/merge-base.ts — SHA fetch may silently fail on Azure Repos
  sourceRefspec emits +<sha>:refs/remotes/origin/ado-aw-prepare-source when sourceRef is a 40-char hex string. Azure Repos requires the SHA to be reachable from an advertised ref (uploadpack.allowReachableSHA1InWant). If it isn’t, the fetch returns non-zero and falls through to bounded deepening — but the ok: false reason says "Exact source fetch failed at depth N: ..." rather than indicating it was a SHA-fetch limitation. The fallback chain is correct; the diagnostic message may just be confusing. Worth a clarifying comment.

• scripts/ado-script/src/shared/merge-base.ts:ensureExactMergeBaseFetched — non-shallow path only fetches target
  For a non-shallow repo the function skips the source fetch (full history means source commits are already present) and goes straight to localMergeBases. Correct in practice. It then checks bases.includes(commonCommit) against ADO’s metadata — if the repo is a partial clone with a missing object, the local merge-base could return an unexpected SHA. A git cat-file -e <commonCommit> guard before trusting the result would make this watertight, though the edge case is very unlikely on standard agent pools.

• scripts/ado-script/src/shared/ado-remote.ts — SSH / on-premises remotes silently skip REST path
  parseAdoRepoUrl returns null for non-HTTPS or unrecognized hosts, so isCurrentAdoOrganization returns false and the ‘restReason’ logged is "origin is not an eligible same-organization Azure Repos remote" even for SSH checkouts of the same org. Not a bug (the design note is clear), but someone debugging an SSH-checked-out repo will see a misleading message. A small log note distinguishing "SSH remote" from "different org" would help.

✅ What Looks Good

• depth_for_emit() fix is correct. Previously Some(0) was silently dropped (filter(|d| *d != 0)), so fetch-depth: 0 had no effect when the pipeline UI had a non-zero shallow default. Now it emits fetchDepth: 0 explicitly. The new lower_checkout_emits_zero_fetch_depth_explicitly test pins this behavior.

• checkedDepth bounds guard. Returning null for counts ≥ 10000 and falling through to bounded deepening is safe-by-default. The warnRepo callsite gives users a useful actionable hint.

• Ref validation in validRef covers all git ref constraints: control chars, git-special chars (~^:[?*[\), @{, .., trailing .lock, empty segments. Clean.

• ensureExactMergeBaseFetched cross-checks ADO’s commonCommit against local merge-base --all. Trusting ADO’s REST metadata only after local verification is the right security posture — a tampered response can’t inject a fake merge-base that bypasses the local check.

• Mode split (PatchBase / TargetWorktree) is clean. SafeOutputs only needs the target tip; fetching the full merge-base history there was unnecessary. The typed enum in Rust flows cleanly into the --mode CLI arg.

• Shell quoting is correct throughout. Repo dirs use double-quotes (ADO macros need $(...) expansion by bash), branch names use sh_single_quote (literal, no substitution risk). The bearer token is passed via GIT_CONFIG_* env, never in argv.

Generated by Rust PR Reviewer for #1563 · 68.2 AIC · ⌖ 5.89 AIC · ⊞ 6.2K · ◷