diff --git a/AGENTS.md b/AGENTS.md index 41667fac..b91fd93d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -268,7 +268,7 @@ fail-closed and only pauses when the agent actually proposed a reviewed output. │ ├── approval-summary/ # Safe-outputs summary renderer (bundled to approval-summary.js; end-of-Agent-job summary tab) │ ├── github-app-token/ # GitHub App token minter (bundled to github-app-token.js; mints installation token in Agent + Detection when engine.github-app-token is set) │ ├── executor-e2e/ # Stage 3 safe-output E2E test harness (not a bundle; runs deterministic scenarios against a real ADO project and files a GitHub issue on failure) -│ ├── prepare-pr-base/ # create-pull-request base-ref preparer (bundled to prepare-pr-base.js; fetches/deepens target branch so mcp.rs finds a diff base on shallow-default pools — issue #1413; emitted in BOTH the Agent job and the SafeOutputs job before the executor's worktree add — issue #1453) +│ ├── prepare-pr-base/ # create-pull-request preparer (bundled to prepare-pr-base.js): Agent mode uses ADO diff metadata + bounded dual-ref fallback to make the merge-base reachable; SafeOutputs mode fetches only the target worktree tip │ └── shared/ # Shared modules across bundles (auth, ado-client, env-facts, types.gen.ts) ├── tests/ # Integration tests and fixtures ├── docs/ # Per-concept reference documentation (see index below) diff --git a/docs/ado-script.md b/docs/ado-script.md index 8ab1707b..45881b24 100644 --- a/docs/ado-script.md +++ b/docs/ado-script.md @@ -74,21 +74,16 @@ pipeline** as runtime helpers. Today it produces thirteen bundles: base — issue #1413) **and** the SafeOutputs job (before `ado-aw execute`, so the Stage 3 executor's `git worktree add` resolves `origin/`; each ADO job has an isolated checkout, so the ref must be re-fetched in the job that - builds the worktree — issue #1453). For each allowed create-PR repo dir - (`self` + every `checkout:` alias, passed as repeated `--repo-dir - --target-branch ` pairs in the same dir form - `mcp.rs::resolve_git_dir_for_patch` resolves), it fetches and progressively - deepens THAT repo's resolved target branch into `refs/remotes/origin/` - and points `refs/remotes/origin/HEAD` at it, so a PR can be computed/opened on - shallow-default agent pools without a full-history `checkout: self`. In a - multi-checkout ("meta repo") setup each dir may carry a - different target (see `create-pull-request`'s `target-branches` / - `infer-target-from-checkout-ref`). Reuses - `shared/merge-base.ts::ensureTargetRefFetched` (the same fetch/deepen logic as - the PR execution-context precompute). Each `--repo-dir` is a double-quoted - ADO-macro path; each `--target-branch` is a single-quoted literal; the ADO - bearer (`SYSTEM_ACCESSTOKEN`) rides in masked env for the authenticated git - fetch. Per-dir failures are isolated (logged + skipped). Runs outside AWF. See + builds the worktree — issue #1453). Agent `patch-base` mode uses ADO + `getCommitDiffs` metadata (`commonCommit`, ahead, behind) to fetch the exact + shallow source/target ranges and verifies the base locally. Ineligible or + unavailable REST falls back to bounded dual-ref depths 200/500/2000, never an + automatic full-history fetch. SafeOutputs `target-worktree` mode fetches only + the target tip at depth 1. Each allowed repo is passed as a typed + `--repo-dir` / `--source-ref` / `--target-branch` tuple; per-dir failures are + isolated and surfaced as ADO warnings. The bearer + (`SYSTEM_ACCESSTOKEN`) remains in masked env and spawned-git + `GIT_CONFIG_*`, never argv or `.git/config`. Runs outside AWF. See [`safe-outputs.md`](safe-outputs.md#create-pull-request). > **Internal-only.** `ado-script` is not a user-facing front-matter @@ -216,10 +211,10 @@ inside `src/compile/extensions/exec_context/pr.rs`: merge-commit (parent count ≥ 3 per ADO's PR-validation flow), `merge-base.ts::resolveMergeBase` computes `git merge-base` over the two parents. If that cannot resolve in a shallow checkout, it - fetches both target and source refs with progressive deepening - (`--depth=200/500/2000/--unshallow`) and retries the parent - merge-base. Otherwise it fetches the target branch with progressive - deepening and then runs `git merge-base` against `HEAD`. Same + fetches both target and source refs with bounded deepening + (`--depth=200/500/2000`) and retries the parent merge-base. Otherwise + it fetches both source and target at the same bounded depths and then + runs `git merge-base` against `HEAD`. Same `BASE_SHA` semantics in both paths (git's true common ancestor). 3. **Stage artefacts** — writes `aw-context/pr/base.sha` and `aw-context/pr/head.sha` so the agent can `git diff $(cat diff --git a/docs/execution-context.md b/docs/execution-context.md index 5ab1b729..25105c14 100644 --- a/docs/execution-context.md +++ b/docs/execution-context.md @@ -29,7 +29,7 @@ historically rebuilt the same ~120 lines of bash to work around this. The execution-context plugin owns that step centrally — but does *only* the part the agent cannot do for itself: -- Fetches the PR target branch with progressive deepening until +- Fetches the PR source and target refs with bounded deepening until `git merge-base` resolves (requires the bearer; cannot happen inside the agent's sandbox). - Writes the resolved `base.sha` and `head.sha` so the agent can @@ -484,11 +484,11 @@ under `scripts/ado-script/src/exec-context-pr/`. The bundle: computes `git merge-base HEAD^1 HEAD^2` as the base — same semantics as the deepening path. If a shallow checkout lacks sufficient ancestry, it fetches both the target and source refs - with progressive deepening and retries; unresolved ancestry fails + with bounded deepening and retries; unresolved ancestry fails closed into `error.txt`. Otherwise: -4. **Fetches the PR target branch with progressive deepening** — - `--depth=200`, then `500`, then `2000`, then finally `--unshallow`. +4. **Fetches source and target with bounded deepening** — + `--depth=200`, then `500`, then `2000`. After each successful fetch, attempts `git merge-base origin/ HEAD` and continues to the next depth if it cannot resolve yet. See `merge-base.ts`. @@ -540,7 +540,7 @@ preserves the Stage 1 read-only invariant with these design choices: | Mechanism | Decision | |-----------------------------------------------------------|----------| | Override `checkout: self` with `persistCredentials: true` | **Rejected.** It would write the build identity's bearer into `.git/config` inside the workspace, which is then mounted into the AWF sandbox where the agent could read and exfiltrate it. | -| Override `checkout: self` with `fetchDepth: 0` | **Rejected.** Unnecessary — the precompute fetches exactly the refs it needs. | +| Override `checkout: self` with `fetchDepth: 0` | **Not automatic.** Authors may opt into explicit full history, but it can be very expensive; the precompute normally uses bounded source/target fetches. | | In-step `SYSTEM_ACCESSTOKEN` + `GIT_CONFIG_*` bearer env | **Adopted.** `SYSTEM_ACCESSTOKEN` is mapped from `$(System.AccessToken)` only into the `node exec-context-pr.js` step's process env. The bundle's `git.ts::bearerEnv` then injects `GIT_CONFIG_COUNT` / `GIT_CONFIG_KEY_0` / `GIT_CONFIG_VALUE_0` into the *spawned `git` child process's* env only — not into the Node process's own env, and never via `git -c` on argv. The token never appears in process listings and is never written to disk. After the Node process exits, the bearer is gone from the runtime environment the agent inherits. | After the precompute step exits, the bearer is gone from the runtime diff --git a/docs/front-matter.md b/docs/front-matter.md index b6ca4cff..c8865592 100644 --- a/docs/front-matter.md +++ b/docs/front-matter.md @@ -408,10 +408,11 @@ Object fields: ### Tuning checkout fetch behavior (`fetch-depth` / `fetch-tags`) -On large monorepos the checkout step can dominate the run: ADO's default -`checkout` performs a full-history clone **and** `git fetch --tags`, which may -take tens of minutes. `fetch-depth` and `fetch-tags` let you tune this -per-repository: +On large monorepos the checkout step can dominate the run. Azure DevOps can +apply a pipeline-level shallow-fetch setting (newer pipelines commonly use +depth 1), while tag syncing can also add substantial transfer. `fetch-depth` +and `fetch-tags` let source-controlled YAML override those settings per +repository: ```yaml repos: @@ -420,7 +421,9 @@ repos: fetch-tags: false # skip the (often huge) tag fetch ``` -- `fetch-depth: 0` means **full history** (no `fetchDepth` is emitted). +- `fetch-depth: 0` explicitly emits `fetchDepth: 0`, disabling shallow fetch + even when the pipeline UI is configured for depth 1. Full history can be + very expensive in a large or old repository. - When a field is omitted the ADO default applies, so agents that don't set these compile **unchanged**. - Setting `fetch-depth`/`fetch-tags` on an entry with `checkout: false` has no diff --git a/docs/safe-outputs.md b/docs/safe-outputs.md index 7d338e54..13edcc29 100644 --- a/docs/safe-outputs.md +++ b/docs/safe-outputs.md @@ -272,31 +272,32 @@ Creates a pull request with code changes made by the agent. When invoked: During Stage 3 execution, the repository is validated against the allowed list (from `checkout:` + "self"), then the patch is applied and a PR is created in Azure DevOps. -**Shallow-clone agent pools (automatic):** The diff base for the patch is -computed at agent time from the checked-out repository. On agent pools whose -default git fetch is shallow (`fetchDepth: 1`), a bare `checkout` leaves no -`origin/` ref, which would otherwise prevent the diff base from -being computed. To handle this transparently, whenever `create-pull-request` is -configured the compiler emits a credentialed **prepare step** that fetches and -progressively deepens the configured `target-branch` and points `origin/HEAD` at -it — in the `self` checkout **and in each additional `checkout:` repo dir**, so a -PR to *any* allowed repository works. The prepare step runs in **both** the Agent -job (before the agent runs, so the host-side SafeOutputs MCP server can compute -the diff base) **and** the SafeOutputs job (before `ado-aw execute`, so the -Stage 3 executor's `git worktree add` resolves `origin/` — each ADO job -has an isolated checkout, so the ref must be re-fetched in the job that builds -the worktree; issue #1453). This means create-pull-request works on -shallow-default pools **without** forcing a full-history checkout and **without** -hand-editing the compiled lock (so the runtime integrity check keeps passing). No -configuration is required. See [`docs/ado-script.md`](ado-script.md) +**Shallow-clone agent pools (automatic):** The diff base is computed at agent +time from the checked-out repository. For same-organization Azure Repos, +`prepare-pr-base.js` asks the ADO Diffs API for the exact `commonCommit`, +`aheadCount`, and `behindCount`, then fetches only the source and target ranges +needed to make that base locally reachable. It verifies the server result with +`git merge-base --all` before the host-side SafeOutputs MCP server can generate +a patch. Non-Azure/cross-organization/unavailable-REST cases use bounded +dual-ref depths 200/500/2000 and fail clearly rather than silently fetching full +history. + +The compiler emits separate modes in the two isolated ADO jobs: + +- **Agent — `patch-base`:** prepares and verifies both sides of the merge-base. +- **SafeOutputs — `target-worktree`:** fetches only `origin/` at depth 1 + for the executor's `git worktree add` (issue #1453). + +No full-history checkout or `--unshallow` fallback is forced. Authors can +explicitly set `repos: [{ name: self, fetch-depth: 0 }]` when they accept that +potentially large cost. The generated lock remains source-controlled and the +runtime integrity check stays enabled. See [`docs/ado-script.md`](ado-script.md) (`prepare-pr-base.js`). -> **Branch semantics.** The step deepens each repo's resolved `target-branch` -> (the PR's **destination/base**) — not the per-repo `repos:` checkout `ref` (the -> source side). By default every repo targets the single `target-branch`; enable -> `infer-target-from-checkout-ref` (and/or `target-branches`) to give each repo -> its own base branch in a multi-checkout setup. The deepened branch always -> matches the branch the PR targets (shared resolution). +> **Branch semantics.** Each repo carries its checkout source ref and its +> resolved PR destination. By default every repo targets the single +> `target-branch`; enable `infer-target-from-checkout-ref` (and/or +> `target-branches`) to give each repo its own base branch. **Stage 3 Execution Architecture (Hybrid Git + ADO API):** diff --git a/prompts/create-ado-agentic-workflow.md b/prompts/create-ado-agentic-workflow.md index 687aef03..c5dc891a 100644 --- a/prompts/create-ado-agentic-workflow.md +++ b/prompts/create-ado-agentic-workflow.md @@ -496,7 +496,7 @@ When `on.pr` is set: the native ADO `pr:` trigger block is generated from `branc **`on.pr` triggering works without a Build Validation branch policy.** By default (`mode: synthetic`), the compiler emits a Setup-job script that, on CI-triggered builds, looks up the open PR for `Build.SourceBranch` via the ADO REST API and promotes the build to PR semantics if exactly one matches `pr.branches` (and `pr.paths` if configured). Zero or multiple matches → the Agent job self-skips cleanly. Set `on.pr.mode: policy` when an operator-installed Build Validation branch policy is in place — that mode omits all synth wiring AND emits `trigger: none` so feature-branch pushes do not queue duplicate CI builds alongside the policy-driven PR build. Note that in `mode: synthetic` the top-level CI `trigger:` is **not** auto-narrowed to `pr.branches.include`: those are PR target branches, and ADO `trigger:` fires on pushes *to* listed branches, so narrowing would suppress CI on the feature branches synthPr must react to. Full reference: ["PR Triggering in Azure Repos" in `docs/front-matter.md`](../docs/front-matter.md#pr-triggering-in-azure-repos). -**PR-reviewer agents — DO NOT write your own precompute step.** When `on.pr` is set, the compiler automatically (1) fetches the PR target branch with progressive deepening, (2) resolves and stages `aw-context/pr/base.sha` + `aw-context/pr/head.sha`, (3) appends a prompt fragment listing common `git diff`/`git show`/`git log` commands and example Azure DevOps MCP tool calls (`repo_get_pull_request_by_id`, `repo_list_pull_request_threads`, `repo_create_pull_request_thread`) with the PR id / project / repo pre-filled, and (4) adds `git`, `git diff`, `git log`, `git show`, `git status`, `git rev-parse`, `git symbolic-ref` to the agent's bash allow-list. The agent runs `git diff $BASE..$HEAD` itself inside the AWF sandbox (objects are already fetched into the workspace). On failure (e.g. merge-base could not be resolved), the failure fragment tells the agent to surface the error rather than produce an empty review. Opt out via `execution-context.pr.enabled: false`. Full reference: [`docs/execution-context.md`](../docs/execution-context.md). +**PR-reviewer agents — DO NOT write your own precompute step.** When `on.pr` is set, the compiler automatically (1) fetches the PR source and target refs with bounded deepening, (2) resolves and stages `aw-context/pr/base.sha` + `aw-context/pr/head.sha`, (3) appends a prompt fragment listing common `git diff`/`git show`/`git log` commands and example Azure DevOps MCP tool calls (`repo_get_pull_request_by_id`, `repo_list_pull_request_threads`, `repo_create_pull_request_thread`) with the PR id / project / repo pre-filled, and (4) adds `git`, `git diff`, `git log`, `git show`, `git status`, `git rev-parse`, `git symbolic-ref` to the agent's bash allow-list. The agent runs `git diff $BASE..$HEAD` itself inside the AWF sandbox (objects are already fetched into the workspace). On failure (e.g. merge-base could not be resolved), the failure fragment tells the agent to surface the error rather than produce an empty review. Opt out via `execution-context.pr.enabled: false`. Full reference: [`docs/execution-context.md`](../docs/execution-context.md). #### Pipeline Triggers (`on.pipeline`) diff --git a/scripts/ado-script/src/prepare-pr-base/__tests__/index.test.ts b/scripts/ado-script/src/prepare-pr-base/__tests__/index.test.ts index 0615d73a..3b65f905 100644 --- a/scripts/ado-script/src/prepare-pr-base/__tests__/index.test.ts +++ b/scripts/ado-script/src/prepare-pr-base/__tests__/index.test.ts @@ -1,224 +1,275 @@ -import { describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import type { CommitDiffMetadata } from "../../shared/ado-client.js"; import type { GitResult } from "../../shared/git.js"; import type { GitRunners } from "../../shared/merge-base.js"; -import { main, parseArgs } from "../index.js"; +import { + main, + parseArgs, + type PrepareArgs, + type PrepareDependencies, +} from "../index.js"; -const SHA_M = "d".repeat(40); +const HEAD = "a".repeat(40); +const TARGET = "b".repeat(40); +const BASE = "c".repeat(40); -/** - * Build a `GitRunners` pair that records every `runGit` invocation and lets a - * matcher decide the result. `gitOk` answers `merge-base` queries. - */ -function makeRunners(opts: { - fetchStatus: number; - symbolicStatus?: number; +function metadata(overrides: Partial = {}): CommitDiffMetadata { + return { + commonCommit: BASE, + aheadCount: 7, + behindCount: 5, + sourceCommit: HEAD, + targetCommit: TARGET, + ...overrides, + }; +} + +function dependencies(opts: { + remote?: string; + fetchStatus?: number; mergeBase?: string | null; -}): { runners: GitRunners; calls: string[][] } { - const calls: string[][] = []; - const runGit: GitRunners["runGit"] = (args) => { - calls.push(args); - let result: GitResult = { stdout: "", stderr: "", status: 1 }; - if (args[0] === "fetch") { - result = { stdout: "", stderr: "", status: opts.fetchStatus }; - } else if (args[0] === "symbolic-ref") { - result = { stdout: "", stderr: "", status: opts.symbolicStatus ?? 0 }; - } - return result; + targetSha?: string | null; + metadata?: CommitDiffMetadata; + metadataError?: unknown; + chdir?: (dir: string) => void; +} = {}): { + deps: PrepareDependencies; + calls: Array<{ args: string[]; env?: Record }>; + dirs: string[]; +} { + const calls: Array<{ args: string[]; env?: Record }> = []; + const dirs: string[] = []; + const runners: GitRunners = { + runGit: (args, env) => { + calls.push({ args, env }); + let result: GitResult = { stdout: "", stderr: "", status: 1 }; + if (args[0] === "fetch") { + result = { + stdout: "", + stderr: opts.fetchStatus === 0 || opts.fetchStatus === undefined ? "" : "fetch failed", + status: opts.fetchStatus ?? 0, + }; + } else if (args[0] === "symbolic-ref") { + result = { stdout: "", stderr: "", status: 0 }; + } + return result; + }, + gitOk: (args) => { + const command = args.join(" "); + if (command === "rev-parse HEAD") return HEAD; + if (command === "remote get-url origin") { + return opts.remote ?? "https://dev.azure.com/org/project/_git/repo"; + } + if (command.startsWith("merge-base --all ")) { + return opts.mergeBase === undefined ? BASE : opts.mergeBase; + } + if (command.startsWith("rev-parse origin/")) { + return opts.targetSha === undefined ? TARGET : opts.targetSha; + } + return null; + }, }; - const gitOk: GitRunners["gitOk"] = (args) => { - if (args[0] === "merge-base") { - return opts.mergeBase === undefined ? SHA_M : opts.mergeBase; - } - return null; + const getCommitDiffMetadata = vi.fn(async () => { + if (opts.metadataError !== undefined) throw opts.metadataError; + return opts.metadata ?? metadata(); + }); + const chdir = opts.chdir ?? ((dir: string) => void dirs.push(dir)); + return { + deps: { runners, chdir, getCommitDiffMetadata }, + calls, + dirs, }; - return { runners: { runGit, gitOk }, calls }; } -/** A `chdir` stub that records the dirs it was asked to enter. */ -function recordingChdir(): { chdir: (dir: string) => void; dirs: string[] } { - const dirs: string[] = []; - return { chdir: (dir: string) => void dirs.push(dir), dirs }; +function patchArgs(): PrepareArgs { + return { + mode: "patch-base", + repos: [ + { + dir: "/src", + sourceRef: "refs/heads/feature", + target: "main", + }, + ], + fallbackTarget: "main", + }; } -describe("parseArgs", () => { - it("pairs each --repo-dir with the following --target-branch", () => { - const args = parseArgs([ - "--repo-dir", - "/src", - "--target-branch", - "main", - "--repo-dir", - "/src/tools", - "--target-branch", - "release", - ]); - expect(args.repos).toEqual([ - { dir: "/src", target: "main" }, - { dir: "/src/tools", target: "release" }, - ]); - }); +afterEach(() => vi.restoreAllMocks()); - it("strips a leading refs/heads/ from targets", () => { - const args = parseArgs([ - "--repo-dir", - "/src", - "--target-branch", - "refs/heads/release/2.x", - ]); - expect(args.repos).toEqual([{ dir: "/src", target: "release/2.x" }]); +describe("parseArgs", () => { + it("parses mode and per-repository source/target triples", () => { + expect( + parseArgs([ + "--mode", + "patch-base", + "--repo-dir", + "/src", + "--source-ref", + "refs/heads/feature", + "--target-branch", + "refs/heads/main", + ]), + ).toEqual(patchArgs()); }); - it("uses the fallback target for a --repo-dir with no following --target-branch", () => { - // A leading global --target-branch sets the fallback; a bare trailing - // --repo-dir inherits it. - const args = parseArgs(["--target-branch", "develop", "--repo-dir", "/src"]); - expect(args.fallbackTarget).toBe("develop"); - expect(args.repos).toEqual([{ dir: "/src", target: "develop" }]); + it("allows target-worktree entries without a source ref", () => { + expect( + parseArgs([ + "--mode", + "target-worktree", + "--repo-dir", + "/src", + "--target-branch", + "develop", + ]), + ).toEqual({ + mode: "target-worktree", + repos: [{ dir: "/src", target: "develop", sourceRef: undefined }], + fallbackTarget: "main", + fallbackSourceRef: undefined, + }); }); - it("defaults the fallback target to main", () => { - expect(parseArgs([]).fallbackTarget).toBe("main"); - expect(parseArgs([]).repos).toEqual([]); + it("rejects unknown modes", () => { + expect(() => parseArgs(["--mode", "everything"])).toThrow(/Unsupported/); }); }); describe("prepare-pr-base main", () => { - it("fetches origin/ and sets origin/HEAD on success", () => { - const { runners, calls } = makeRunners({ fetchStatus: 0, mergeBase: SHA_M }); - const { chdir, dirs } = recordingChdir(); - const rc = main( - { repos: [{ dir: "/src", target: "main" }], fallbackTarget: "main" }, - {}, - runners, - chdir, + it("uses ADO metadata to fetch exact shallow source and target ranges", async () => { + vi.spyOn(process.stdout, "write").mockImplementation(() => true); + const { deps, calls, dirs } = dependencies(); + const rc = await main( + patchArgs(), + { + SYSTEM_COLLECTIONURI: "https://dev.azure.com/org/", + SYSTEM_ACCESSTOKEN: "token", + }, + deps, ); expect(rc).toBe(0); expect(dirs).toEqual(["/src"]); + const fetches = calls.filter((call) => call.args[0] === "fetch"); + expect(fetches).toHaveLength(2); + expect(fetches[0]!.args).toContain("--depth=8"); + expect(fetches[0]!.args).toContain("--no-recurse-submodules"); + expect(fetches[0]!.args).toContain( + `+${HEAD}:refs/remotes/origin/ado-aw-prepare-source`, + ); + expect(fetches[1]!.args).toContain("--depth=6"); + expect(fetches[1]!.args).toContain( + `+${TARGET}:refs/remotes/origin/main`, + ); + expect(fetches[0]!.env).toMatchObject({ + GIT_CONFIG_VALUE_0: "Authorization: bearer token", + }); + expect(calls.some((call) => call.args[0] === "symbolic-ref")).toBe(true); + }); - const fetch = calls.find((c) => c[0] === "fetch"); - expect(fetch).toBeDefined(); - expect(fetch).toContain("+refs/heads/main:refs/remotes/origin/main"); - - const sym = calls.find((c) => c[0] === "symbolic-ref"); - expect(sym).toEqual([ - "symbolic-ref", - "refs/remotes/origin/HEAD", - "refs/remotes/origin/main", - ]); + it("falls back to bounded dual-ref fetch when ADO REST is unavailable", async () => { + vi.spyOn(process.stdout, "write").mockImplementation(() => true); + const error = Object.assign(new Error("forbidden"), { statusCode: 403 }); + const { deps, calls } = dependencies({ metadataError: error }); + await main( + patchArgs(), + { SYSTEM_COLLECTIONURI: "https://dev.azure.com/org/" }, + deps, + ); + const fetches = calls.filter((call) => call.args[0] === "fetch"); + expect(fetches).toHaveLength(1); + expect(fetches[0]!.args).toContain("--depth=200"); + expect(fetches[0]!.args).toContain( + "+refs/heads/feature:refs/remotes/origin/ado-aw-prepare-source", + ); + expect(fetches[0]!.args).toContain( + "+refs/heads/main:refs/remotes/origin/main", + ); }); - it("deepens each repo dir with its OWN target branch (meta-repo)", () => { - const { runners, calls } = makeRunners({ fetchStatus: 0, mergeBase: SHA_M }); - const { chdir, dirs } = recordingChdir(); - const rc = main( + it("target-worktree fetches only the target tip at depth one", async () => { + vi.spyOn(process.stdout, "write").mockImplementation(() => true); + const { deps, calls } = dependencies(); + await main( { - repos: [ - { dir: "/src", target: "main" }, - { dir: "/src/tools", target: "release" }, - { dir: "/src/docs", target: "gh-pages" }, - ], + mode: "target-worktree", + repos: [{ dir: "/src", target: "develop" }], fallbackTarget: "main", }, {}, - runners, - chdir, + deps, ); - expect(rc).toBe(0); - expect(dirs).toEqual(["/src", "/src/tools", "/src/docs"]); - - // Each dir fetched + set origin/HEAD to ITS target ref. - const fetchRefspecs = calls - .filter((c) => c[0] === "fetch") - .map((c) => c.find((a) => a.startsWith("+refs/heads/"))); - expect(fetchRefspecs).toEqual([ - "+refs/heads/main:refs/remotes/origin/main", - "+refs/heads/release:refs/remotes/origin/release", - "+refs/heads/gh-pages:refs/remotes/origin/gh-pages", - ]); - const symTargets = calls.filter((c) => c[0] === "symbolic-ref").map((c) => c[2]); - expect(symTargets).toEqual([ - "refs/remotes/origin/main", - "refs/remotes/origin/release", - "refs/remotes/origin/gh-pages", + const fetches = calls.filter((call) => call.args[0] === "fetch"); + expect(fetches).toHaveLength(1); + expect(fetches[0]!.args).toEqual([ + "fetch", + "--no-tags", + "--no-recurse-submodules", + "--depth=1", + "origin", + "+refs/heads/develop:refs/remotes/origin/develop", ]); }); - it("isolates a per-dir chdir failure — other dirs still processed", () => { - const { runners, calls } = makeRunners({ fetchStatus: 0, mergeBase: SHA_M }); - const dirs: string[] = []; - const chdir = (dir: string) => { - dirs.push(dir); - if (dir === "/src/broken") { - throw new Error("no such directory"); - } - }; - const rc = main( + it("does not send the ADO bearer to a non-Azure origin", async () => { + vi.spyOn(process.stdout, "write").mockImplementation(() => true); + const { deps, calls } = dependencies({ + remote: "https://github.com/example/repo.git", + }); + await main( { - repos: [ - { dir: "/src", target: "main" }, - { dir: "/src/broken", target: "release" }, - { dir: "/src/lib", target: "main" }, - ], + mode: "target-worktree", + repos: [{ dir: "/src", target: "main" }], fallbackTarget: "main", }, - {}, - runners, - chdir, + { SYSTEM_ACCESSTOKEN: "secret-token" }, + deps, ); - expect(rc).toBe(0); - expect(dirs).toEqual(["/src", "/src/broken", "/src/lib"]); - // Only the two good dirs fetched + set origin/HEAD (broken skipped). - expect(calls.filter((c) => c[0] === "fetch").length).toBe(2); - expect(calls.filter((c) => c[0] === "symbolic-ref").length).toBe(2); + const fetch = calls.find((call) => call.args[0] === "fetch"); + expect(fetch?.env).toEqual({}); }); - it("exits 0 without setting origin/HEAD when every fetch fails (benign)", () => { - const { runners, calls } = makeRunners({ fetchStatus: 1, mergeBase: null }); - const { chdir } = recordingChdir(); - const rc = main( - { repos: [{ dir: "/src", target: "main" }], fallbackTarget: "main" }, + it("isolates a checkout-directory failure and processes later repos", async () => { + vi.spyOn(process.stdout, "write").mockImplementation(() => true); + const visited: string[] = []; + const { deps, calls } = dependencies({ + chdir: (dir) => { + visited.push(dir); + if (dir === "/broken") throw new Error("missing"); + }, + }); + await main( + { + mode: "target-worktree", + repos: [ + { dir: "/broken", target: "main" }, + { dir: "/good", target: "main" }, + ], + fallbackTarget: "main", + }, {}, - runners, - chdir, + deps, ); - // Non-fatal: the agent still runs; mcp.rs surfaces its own error if needed. - expect(rc).toBe(0); - expect(calls.some((c) => c[0] === "symbolic-ref")).toBe(false); + expect(visited).toEqual(["/broken", "/good"]); + expect(calls.filter((call) => call.args[0] === "fetch")).toHaveLength(1); }); - it("falls back to BUILD_SOURCESDIRECTORY + fallback target when no repos given", () => { - const { runners, calls } = makeRunners({ fetchStatus: 0, mergeBase: SHA_M }); - const { chdir, dirs } = recordingChdir(); - main( - { repos: [], fallbackTarget: "develop" }, - { BUILD_SOURCESDIRECTORY: "/agent/src" }, - runners, - chdir, - ); - expect(dirs).toEqual(["/agent/src"]); - const fetch = calls.find((c) => c[0] === "fetch"); - expect(fetch).toContain("+refs/heads/develop:refs/remotes/origin/develop"); - }); - - it("passes the SYSTEM_ACCESSTOKEN bearer into the git fetch env", () => { - const seenEnvs: Array | undefined> = []; - const runners: GitRunners = { - runGit: (args, env) => { - if (args[0] === "fetch") seenEnvs.push(env); - return { stdout: "", stderr: "", status: 0 }; + it("uses BUILD_SOURCESDIRECTORY and BUILD_SOURCEBRANCH for legacy no-arg calls", async () => { + vi.spyOn(process.stdout, "write").mockImplementation(() => true); + const { deps, dirs } = dependencies({ remote: "https://github.com/org/repo" }); + await main( + { + mode: "patch-base", + repos: [], + fallbackTarget: "develop", }, - gitOk: (args) => (args[0] === "merge-base" ? SHA_M : null), - }; - const { chdir } = recordingChdir(); - main( - { repos: [{ dir: "/src", target: "main" }], fallbackTarget: "main" }, - { SYSTEM_ACCESSTOKEN: "tok" }, - runners, - chdir, + { + BUILD_SOURCESDIRECTORY: "/agent/src", + BUILD_SOURCEBRANCH: "refs/heads/feature", + }, + deps, ); - expect(seenEnvs[0]).toMatchObject({ - GIT_CONFIG_VALUE_0: "Authorization: bearer tok", - }); + expect(dirs).toEqual(["/agent/src"]); }); }); diff --git a/scripts/ado-script/src/prepare-pr-base/index.ts b/scripts/ado-script/src/prepare-pr-base/index.ts index ddc47aff..08cf6556 100644 --- a/scripts/ado-script/src/prepare-pr-base/index.ts +++ b/scripts/ado-script/src/prepare-pr-base/index.ts @@ -1,216 +1,293 @@ /** - * prepare-pr-base — make the create-pull-request diff base available on - * shallow-default Azure DevOps agent pools (issue #1413). + * Prepare local refs required by create-pull-request on shallow Azure + * Pipelines checkouts. * - * ## Why this exists + * `patch-base` runs in the Agent job. For same-organization Azure Repos it + * asks ADO for the exact common commit and divergence counts, fetches only the + * source/target ranges needed to reach that base, and verifies the result + * locally. Other remotes use bounded dual-ref deepening (200/500/2000). * - * The `create-pull-request` safe-output patch is generated 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. On an agent pool whose - * default git fetch is shallow (`fetchDepth: 1`), `checkout: self` leaves no - * `refs/remotes/origin/` ref and too little history, so the server - * cannot compute a merge base and the PR can't be opened. - * - * This bundle runs as a **credentialed Agent-job prepare step on the host** - * (before the AWF-wrapped Copilot run, using `$(System.AccessToken)`). For each - * allowed create-pull-request repo dir (`self` + every `checkout:` alias, passed - * as repeated `--repo-dir --target-branch ` pairs in the same form - * the MCP server resolves the dirs), it fetches that repo's target branch into - * `refs/remotes/origin/` and progressively deepens local history to the - * merge base — reusing the exact `shared/merge-base.ts::ensureTargetRefFetched` - * logic that the PR execution-context precompute already uses. It also points - * each dir's `refs/remotes/origin/HEAD` at its target so `mcp.rs`'s symbolic-ref - * default-branch detection resolves the right base. Because the MCP server - * operates on those same dirs, the base then resolves with no in-sandbox network. - * - * Each `--target-branch` is the create-pull-request **destination/base** branch - * for its `--repo-dir` (which in a multi-checkout "meta repo" setup may differ - * per repo) — NOT the per-repo `checkout:` ref (the source/HEAD side). - * - * ## Trust boundary - * - * Mirrors the exec-context bundles: the bearer (`SYSTEM_ACCESSTOKEN`) is passed - * to the spawned `git` child via `GIT_CONFIG_*` env vars (see - * `shared/git.ts::bearerEnv`) — never in argv, never written to `.git/config`. - * The compiler-owned, non-secret `--repo-dir` / `--target-branch` are argv flags - * (immune to ADO pipeline-variable shadowing). - * - * ## Posture - * - * Benign fetch failures (e.g. a pool that refuses shallow deepening) are logged - * and the step still exits 0 so the agent runs; the agent then simply hits the - * existing `mcp.rs` diff-base error if truly unrecoverable. Only genuine infra - * errors (an unusable `BUILD_SOURCESDIRECTORY`) hard-fail. - * - * Invocation: node prepare-pr-base.js \ - * --repo-dir --target-branch \ - * [--repo-dir --target-branch ...] - * env: SYSTEM_ACCESSTOKEN (bearer for the git fetch) + * `target-worktree` runs in SafeOutputs. It only fetches the target tip needed + * by the executor's `git worktree add`. */ +import { + getCommitDiffMetadata as defaultGetCommitDiffMetadata, + httpStatusCode, + type CommitDiffMetadata, +} from "../shared/ado-client.js"; +import { + isCurrentAdoOrganization, + parseAdoRepoUrl, +} from "../shared/ado-remote.js"; import { bearerEnv, gitOk as defaultGitOk, runGit as defaultRunGit } from "../shared/git.js"; -import { ensureTargetRefFetched, type GitRunners } from "../shared/merge-base.js"; +import { + ensureExactMergeBaseFetched, + ensureRefsForMergeBaseFetched, + ensureTargetTipFetched, + type GitRunners, +} from "../shared/merge-base.js"; +import { logWarning } from "../shared/vso-logger.js"; const defaultRunners: GitRunners = { runGit: defaultRunGit, gitOk: defaultGitOk, }; -/** A single checkout dir to deepen, with the target branch to deepen there. */ +export type PrepareMode = "patch-base" | "target-worktree"; + export interface RepoTarget { - /** Checkout dir (as `mcp.rs::resolve_git_dir_for_patch` resolves it). */ dir: string; - /** Short target branch to fetch/deepen in that dir (no `refs/heads/`). */ target: string; + sourceRef?: string; } export interface PrepareArgs { - /** - * The checkout dirs to deepen with their per-repo target branch — one per - * allowed create-pull-request repo, in the SAME form the host-side SafeOutputs - * MCP server resolves them (`resolve_git_dir_for_patch`): `working_directory` - * for `self`, and `working_directory/` for each `checkout:` alias. In a - * multi-checkout ("meta repo") setup each dir may carry a different target. - * Empty when none were passed (falls back to `[{ BUILD_SOURCESDIRECTORY, - * fallbackTarget }]`, then the process cwd). - */ + mode: PrepareMode; repos: RepoTarget[]; - /** Target for the fallback dir when no `--repo-dir` was passed. */ fallbackTarget: string; + fallbackSourceRef?: string; } -function shortBranch(name: string): string { - const s = name.replace(/^refs\/heads\//, ""); - // In lock-step with the Rust `short_branch` resolver for every input: a - // normal ref → its short form; a degenerate `refs/heads/` (empty after - // stripping) → the original string (so a malformed ref fails loudly the same - // way on both sides, never silently diverging); and `""` → `""`. `parseArgs` - // supplies its own "main" default for a genuinely absent `--target-branch`. - return s.length > 0 ? s : name; +export interface PrepareDependencies { + runners: GitRunners; + chdir: (dir: string) => void; + getCommitDiffMetadata: ( + project: string, + repository: string, + targetBranch: string, + sourceCommit: string, + ) => Promise; +} + +const defaultDependencies: PrepareDependencies = { + runners: defaultRunners, + chdir: process.chdir.bind(process), + getCommitDiffMetadata: defaultGetCommitDiffMetadata, +}; + +function shortTargetBranch(name: string): string { + const short = name.replace(/^refs\/heads\//, ""); + return short.length > 0 ? short : name; +} + +function oneLine(value: unknown, maxLength = 500): string { + const text = String(value instanceof Error ? value.message : value) + .replace(/[\r\n]+/g, " ") + .trim(); + return text.length <= maxLength ? text : `${text.slice(0, maxLength)}...`; +} + +function flushPending( + repos: RepoTarget[], + pending: Partial | null, + fallbackTarget: string, + fallbackSourceRef?: string, +): void { + if (!pending?.dir) return; + repos.push({ + dir: pending.dir, + target: pending.target ?? fallbackTarget, + sourceRef: pending.sourceRef ?? fallbackSourceRef, + }); } -/** - * Parse repeated `--repo-dir ` / `--target-branch ` flags into - * ordered `{ dir, target }` pairs. Each `--repo-dir` takes the target from the - * `--target-branch` that immediately follows it (compiler emits them adjacent); - * a `--target-branch` with no preceding un-paired `--repo-dir` sets the fallback - * target. Branch names are normalized to short form (`refs/heads/x` → `x`). - */ export function parseArgs(argv: string[]): PrepareArgs { const repos: RepoTarget[] = []; + let mode: PrepareMode = "patch-base"; let fallbackTarget = "main"; - let pendingDir: string | null = null; + let fallbackSourceRef: string | undefined; + let pending: Partial | null = null; + for (let i = 0; i < argv.length; i++) { - if (argv[i] === "--repo-dir") { - // A previous dir without its own target inherits the fallback. - if (pendingDir !== null && pendingDir.length > 0) { - repos.push({ dir: pendingDir, target: fallbackTarget }); + const flag = argv[i]; + const value = argv[i + 1] ?? ""; + if (flag === "--mode") { + if (value !== "patch-base" && value !== "target-worktree") { + throw new Error(`Unsupported prepare-pr-base mode '${value}'.`); } - pendingDir = argv[i + 1] ?? ""; + mode = value; i++; - } else if (argv[i] === "--target-branch") { - const target = shortBranch(argv[i + 1] ?? "main"); - if (pendingDir !== null && pendingDir.length > 0) { - repos.push({ dir: pendingDir, target }); - pendingDir = null; - } else { - fallbackTarget = target; - } + } else if (flag === "--repo-dir") { + flushPending(repos, pending, fallbackTarget, fallbackSourceRef); + pending = { dir: value }; + i++; + } else if (flag === "--source-ref") { + if (pending?.dir) pending.sourceRef = value; + else fallbackSourceRef = value; + i++; + } else if (flag === "--target-branch") { + const target = shortTargetBranch(value || "main"); + if (pending?.dir) pending.target = target; + else fallbackTarget = target; i++; } } - // A trailing dir with no following --target-branch inherits the fallback. - if (pendingDir !== null && pendingDir.length > 0) { - repos.push({ dir: pendingDir, target: fallbackTarget }); - } - return { repos, fallbackTarget }; + flushPending(repos, pending, fallbackTarget, fallbackSourceRef); + return { mode, repos, fallbackTarget, fallbackSourceRef }; } -/** - * Deepen `origin/` in a single checkout dir and point that dir's - * `origin/HEAD` at it. Returns `true` when the base resolved. Never throws — a - * dir that isn't a git repo (a quirky-workspace path the MCP would also fail on) - * or a fetch that can't reach the base is a benign, isolated skip. - */ -function prepareOneRepo( +function pointOriginHead( repoDir: string, - targetShort: string, - fetchEnv: Record, + target: string, runners: GitRunners, - chdir: (dir: string) => void, -): boolean { - try { - chdir(repoDir); - } catch (err) { - // Non-fatal: the MCP server would fail on this same path too. Skip and let - // other repos proceed. - process.stdout.write( - `[prepare-pr-base] note: skipping '${repoDir}' (could not chdir: ${(err as Error).message}).\n`, - ); - return false; - } - - const fetched = ensureTargetRefFetched(targetShort, fetchEnv, runners); - if (!fetched.ok) { - process.stdout.write( - `[prepare-pr-base] warning: ${fetched.reason} create-pull-request may fail to compute a diff base for '${repoDir}' on this pool.\n`, - ); - return false; - } - - // Point origin/HEAD at the fetched target so mcp.rs's - // `git symbolic-ref refs/remotes/origin/HEAD` default-branch probe resolves - // to origin/ even when the target is not main/master. +): void { const sym = runners.runGit([ "symbolic-ref", "refs/remotes/origin/HEAD", - `refs/remotes/origin/${targetShort}`, + `refs/remotes/origin/${target}`, ]); if (sym.status !== 0) { process.stdout.write( - `[prepare-pr-base] note: could not set refs/remotes/origin/HEAD -> origin/${targetShort} in '${repoDir}' (${sym.stderr.trim()}); relying on origin/${targetShort} directly.\n`, + `[prepare-pr-base] note: could not set origin/HEAD for '${repoDir}' (${oneLine(sym.stderr)}).\n`, ); } +} + +function warnRepo(repoDir: string, target: string, reason: string): void { + logWarning( + `[prepare-pr-base] '${repoDir}' target '${target}': ${oneLine(reason)} ` + + "create-pull-request may fail. Set this checkout's fetch-depth to 0 only if the repository can afford full history.", + ); +} + +async function preparePatchBase( + repo: RepoTarget, + env: NodeJS.ProcessEnv, + fetchEnv: Record, + deps: PrepareDependencies, +): Promise { + const { runners } = deps; + const headSha = runners.gitOk(["rev-parse", "HEAD"]) ?? ""; + const sourceRef = repo.sourceRef ?? env.BUILD_SOURCEBRANCH ?? headSha; + let restReason = "origin is not an eligible same-organization Azure Repos remote"; + + const remote = runners.gitOk(["remote", "get-url", "origin"]) ?? ""; + const identity = parseAdoRepoUrl(remote); + const sameOrgAdo = identity !== null && isCurrentAdoOrganization(identity, env); + const repoFetchEnv = sameOrgAdo ? fetchEnv : {}; + const restDisabled = env.ADO_AW_PREPARE_PR_BASE_DISABLE_REST === "1"; + if (restDisabled) { + restReason = "ADO REST disabled for deterministic fallback testing"; + } else if (identity && sameOrgAdo) { + try { + const metadata = await deps.getCommitDiffMetadata( + identity.project, + identity.repository, + repo.target, + headSha, + ); + const exact = ensureExactMergeBaseFetched( + repo.target, + metadata, + repoFetchEnv, + runners, + ); + if (exact.ok) { + pointOriginHead(repo.dir, repo.target, runners); + process.stdout.write( + `[prepare-pr-base] base ready in '${repo.dir}' via ado-rest ` + + `(merge-base=${exact.baseSha}, ahead=${metadata.aheadCount}, behind=${metadata.behindCount}).\n`, + ); + return true; + } + restReason = exact.reason; + } catch (err) { + const status = httpStatusCode(err); + restReason = `${status ? `ADO REST ${status}: ` : "ADO REST unavailable: "}${oneLine(err)}`; + } + } + const bounded = ensureRefsForMergeBaseFetched( + sourceRef, + repo.target, + repoFetchEnv, + runners, + ); + if (!bounded.ok) { + warnRepo(repo.dir, repo.target, `${restReason}; ${bounded.reason}`); + return false; + } + pointOriginHead(repo.dir, repo.target, runners); process.stdout.write( - `[prepare-pr-base] base ref ready in '${repoDir}': origin/${targetShort} fetched/deepened (merge-base=${fetched.baseSha}).\n`, + `[prepare-pr-base] base ready in '${repo.dir}' via bounded git fetch ` + + `(merge-base=${bounded.baseSha}; REST=${oneLine(restReason)}).\n`, ); return true; } -export function main( +function prepareTargetWorktree( + repo: RepoTarget, + env: NodeJS.ProcessEnv, + fetchEnv: Record, + deps: PrepareDependencies, +): boolean { + const remote = deps.runners.gitOk(["remote", "get-url", "origin"]) ?? ""; + const identity = parseAdoRepoUrl(remote); + const repoFetchEnv = + identity && isCurrentAdoOrganization(identity, env) ? fetchEnv : {}; + const fetched = ensureTargetTipFetched(repo.target, repoFetchEnv, deps.runners); + if (!fetched.ok) { + warnRepo(repo.dir, repo.target, fetched.reason); + return false; + } + pointOriginHead(repo.dir, repo.target, deps.runners); + process.stdout.write( + `[prepare-pr-base] target tip ready in '${repo.dir}' ` + + `(origin/${repo.target}=${fetched.baseSha}).\n`, + ); + return true; +} + +export async function main( args: PrepareArgs, env: NodeJS.ProcessEnv = process.env, - runners: GitRunners = defaultRunners, - chdir: (dir: string) => void = process.chdir.bind(process), -): number { - // Deepen every checkout dir the MCP server might generate a patch from — one - // per allowed create-pull-request repo (`self` + each `checkout:` alias), each - // with its own target branch. When the compiler passed none, fall back to - // BUILD_SOURCESDIRECTORY then cwd (with the fallback target). + deps: PrepareDependencies = defaultDependencies, +): Promise { let repos = args.repos; if (repos.length === 0) { - const fallbackDir = env.BUILD_SOURCESDIRECTORY; repos = [ - { dir: fallbackDir && fallbackDir.length > 0 ? fallbackDir : ".", target: args.fallbackTarget }, + { + dir: env.BUILD_SOURCESDIRECTORY || ".", + target: args.fallbackTarget, + sourceRef: args.fallbackSourceRef ?? env.BUILD_SOURCEBRANCH, + }, ]; } - const fetchEnv = bearerEnv(env.SYSTEM_ACCESSTOKEN); - // Per-dir failures are isolated (logged + skipped) so one unreachable repo - // never blocks the others or the agent run. - for (const { dir, target } of repos) { - prepareOneRepo(dir, target, fetchEnv, runners, chdir); + for (const repo of repos) { + try { + deps.chdir(repo.dir); + } catch (err) { + warnRepo(repo.dir, repo.target, `could not enter checkout: ${oneLine(err)}`); + continue; + } + if (args.mode === "target-worktree") { + prepareTargetWorktree(repo, env, fetchEnv, deps); + } else { + await preparePatchBase(repo, env, fetchEnv, deps); + } } return 0; } -// CLI entry guard: only run when invoked directly (not when imported by tests). if ( typeof process !== "undefined" && process.argv[1] && - /prepare-pr-base(\/index)?\.js$/.test(process.argv[1]) + /prepare-pr-base(\/index)?\.js$/.test(process.argv[1].replace(/\\/g, "/")) ) { - const args = parseArgs(process.argv.slice(2)); - process.exit(main(args)); + let args: PrepareArgs; + try { + args = parseArgs(process.argv.slice(2)); + } catch (err) { + process.stderr.write(`[prepare-pr-base] ${oneLine(err)}\n`); + process.exit(1); + } + void main(args).then( + (code) => process.exit(code), + (err) => { + process.stderr.write(`[prepare-pr-base] fatal: ${oneLine(err)}\n`); + process.exit(1); + }, + ); } diff --git a/scripts/ado-script/src/shared/__tests__/ado-client.test.ts b/scripts/ado-script/src/shared/__tests__/ado-client.test.ts index a75fc324..8bf56faa 100644 --- a/scripts/ado-script/src/shared/__tests__/ado-client.test.ts +++ b/scripts/ado-script/src/shared/__tests__/ado-client.test.ts @@ -4,6 +4,8 @@ import { BuildStatus } from "azure-devops-node-api/interfaces/BuildInterfaces.js // Mock the auth module before importing ado-client const { mockGitApi, mockBuildApi, mockWebApi, mockGetWebApi } = vi.hoisted(() => { const mockGitApi = { + getBranch: vi.fn(), + getCommitDiffs: vi.fn(), getPullRequestById: vi.fn(), getPullRequestIterations: vi.fn(), getPullRequestIterationChanges: vi.fn(), @@ -25,6 +27,7 @@ vi.mock("../auth.js", () => ({ })); import { + getCommitDiffMetadata, getPullRequestById, getPullRequestIterations, getIterationChanges, @@ -34,6 +37,8 @@ import { describe("ado-client", () => { beforeEach(() => { + mockGitApi.getBranch.mockReset(); + mockGitApi.getCommitDiffs.mockReset(); mockGitApi.getPullRequestById.mockReset(); mockGitApi.getPullRequestIterations.mockReset(); mockGitApi.getPullRequestIterationChanges.mockReset(); @@ -52,6 +57,49 @@ describe("ado-client", () => { expect(result).toEqual({ pullRequestId: 42 }); }); + it("getCommitDiffMetadata pins the target branch tip and orients the diff correctly", async () => { + const source = "a".repeat(40); + const target = "b".repeat(40); + const common = "c".repeat(40); + mockGitApi.getBranch.mockResolvedValue({ commit: { commitId: target } }); + mockGitApi.getCommitDiffs.mockResolvedValue({ + commonCommit: common, + aheadCount: 7, + behindCount: 5, + }); + + const result = await getCommitDiffMetadata("project", "repo", "release/2.x", source); + expect(mockGitApi.getBranch).toHaveBeenCalledWith("repo", "release/2.x", "project"); + expect(mockGitApi.getCommitDiffs).toHaveBeenCalledWith( + "repo", + "project", + true, + 1, + 0, + expect.objectContaining({ version: target }), + expect.objectContaining({ version: source }), + ); + expect(result).toEqual({ + commonCommit: common, + aheadCount: 7, + behindCount: 5, + sourceCommit: source, + targetCommit: target, + }); + }); + + it("getCommitDiffMetadata rejects incomplete or invalid metadata", async () => { + mockGitApi.getBranch.mockResolvedValue({ commit: { commitId: "b".repeat(40) } }); + mockGitApi.getCommitDiffs.mockResolvedValue({ + commonCommit: "not-a-sha", + aheadCount: -1, + behindCount: 0, + }); + await expect( + getCommitDiffMetadata("project", "repo", "main", "a".repeat(40)), + ).rejects.toThrow(/commonCommit/); + }); + it("getPullRequestIterations calls SDK with (repoId, prId, project)", async () => { mockGitApi.getPullRequestIterations.mockResolvedValue([{ id: 1 }]); const result = await getPullRequestIterations("p", "r", 42); diff --git a/scripts/ado-script/src/shared/__tests__/ado-remote.test.ts b/scripts/ado-script/src/shared/__tests__/ado-remote.test.ts new file mode 100644 index 00000000..4fc3f1af --- /dev/null +++ b/scripts/ado-script/src/shared/__tests__/ado-remote.test.ts @@ -0,0 +1,79 @@ +import { describe, expect, it } from "vitest"; + +import { + adoOrganizationFromCollectionUri, + isCurrentAdoOrganization, + parseAdoRepoUrl, +} from "../ado-remote.js"; + +describe("parseAdoRepoUrl", () => { + it("parses dev.azure.com remotes with userinfo and encoded names", () => { + expect( + parseAdoRepoUrl( + "https://build@dev.azure.com/MyOrg/My%20Project/_git/repo%20name", + ), + ).toEqual({ + collectionUri: "https://dev.azure.com/MyOrg/", + organization: "myorg", + project: "My Project", + repository: "repo name", + }); + }); + + it("parses visualstudio.com remotes", () => { + expect( + parseAdoRepoUrl("https://myorg.visualstudio.com/Project/_git/repo/"), + ).toEqual({ + collectionUri: "https://myorg.visualstudio.com/", + organization: "myorg", + project: "Project", + repository: "repo", + }); + }); + + it("parses legacy DefaultCollection visualstudio.com remotes", () => { + expect( + parseAdoRepoUrl( + "https://myorg.visualstudio.com/DefaultCollection/Project/_git/repo", + ), + ).toEqual({ + collectionUri: "https://myorg.visualstudio.com/DefaultCollection/", + organization: "myorg", + project: "Project", + repository: "repo", + }); + }); + + it("rejects non-ADO and malformed remotes", () => { + expect(parseAdoRepoUrl("https://github.com/org/repo.git")).toBeNull(); + expect(parseAdoRepoUrl("not a url")).toBeNull(); + expect(parseAdoRepoUrl("https://dev.azure.com/org/project/repo")).toBeNull(); + }); +}); + +describe("ADO collection matching", () => { + it("extracts organizations from both service URL forms", () => { + expect(adoOrganizationFromCollectionUri("https://dev.azure.com/MyOrg/")).toBe( + "myorg", + ); + expect( + adoOrganizationFromCollectionUri("https://myorg.visualstudio.com/"), + ).toBe("myorg"); + }); + + it("recognizes same-org identities and rejects cross-org identities", () => { + const identity = parseAdoRepoUrl( + "https://dev.azure.com/myorg/Project/_git/repo", + )!; + expect( + isCurrentAdoOrganization(identity, { + SYSTEM_COLLECTIONURI: "https://dev.azure.com/myorg/", + }), + ).toBe(true); + expect( + isCurrentAdoOrganization(identity, { + SYSTEM_COLLECTIONURI: "https://dev.azure.com/other/", + }), + ).toBe(false); + }); +}); diff --git a/scripts/ado-script/src/shared/__tests__/merge-base.test.ts b/scripts/ado-script/src/shared/__tests__/merge-base.test.ts index 4242188a..2b28cc6a 100644 --- a/scripts/ado-script/src/shared/__tests__/merge-base.test.ts +++ b/scripts/ado-script/src/shared/__tests__/merge-base.test.ts @@ -1,7 +1,14 @@ import { describe, expect, it } from "vitest"; import type { GitResult } from "../git.js"; -import { ensureTargetRefFetched, resolveMergeBase, type GitRunners } from "../merge-base.js"; +import { + ensureExactMergeBaseFetched, + ensureRefsForMergeBaseFetched, + ensureTargetRefFetched, + ensureTargetTipFetched, + resolveMergeBase, + type GitRunners, +} from "../merge-base.js"; // Real-shape SHAs (40-char lowercase hex) so the production // SHA40 guard in resolveMergeBase accepts them. We keep the @@ -72,8 +79,8 @@ describe("resolveMergeBase", () => { } if (args[0] === "fetch") { expect(env).toEqual(bearer); - depthArgsSeen.push(args[2] ?? ""); - refsSeen.push(args[4] ?? ""); + depthArgsSeen.push(args.find((arg) => arg.startsWith("--depth=")) ?? ""); + refsSeen.push(...args.filter((arg) => arg.startsWith("+"))); return { stdout: "", stderr: "", status: 0 }; } return { stdout: "", stderr: "no handler", status: 1 }; @@ -96,10 +103,10 @@ describe("resolveMergeBase", () => { expect(result.baseSha).toBe(SHA_M); expect(result.headSha).toBe(SHA_B); } - expect(depthArgsSeen).toEqual(["--depth=200", "--depth=200"]); + expect(depthArgsSeen).toEqual(["--depth=200"]); expect(refsSeen).toEqual([ + "+refs/heads/feature/x:refs/remotes/origin/ado-aw-prepare-source", "+refs/heads/main:refs/remotes/origin/main", - "+refs/heads/feature/x:refs/remotes/origin/feature/x", ]); }); @@ -127,7 +134,7 @@ describe("resolveMergeBase", () => { if (!result.ok) { expect(result.reason).toContain("Could not resolve base/head SHAs"); } - expect(fetchCount).toBe(8); + expect(fetchCount).toBe(3); }); it("uses progressive deepening when HEAD has only 1 parent and stops on first resolution", () => { @@ -150,7 +157,7 @@ describe("resolveMergeBase", () => { }; const gitOk = makeGitOk([ { match: (a) => a.join(" ") === "rev-parse HEAD", out: SHA_C }, - { match: (a) => a.join(" ") === "merge-base origin/main HEAD", out: SHA_M }, + { match: (a) => a.join(" ") === "merge-base --all origin/main HEAD", out: SHA_M }, ]); const result = resolveMergeBase("main", {}, { runGit: runGitTracking, gitOk }); @@ -176,7 +183,7 @@ describe("resolveMergeBase", () => { ]); const gitOk: GitRunners["gitOk"] = (args) => { if (args.join(" ") === "rev-parse HEAD") return SHA_C; - if (args.join(" ") === "merge-base origin/main HEAD") { + if (args.join(" ") === "merge-base --all origin/main HEAD") { mergeBaseCalls++; // First two attempts fail; third succeeds return mergeBaseCalls < 3 ? null : SHA_M; @@ -217,7 +224,7 @@ describe("resolveMergeBase", () => { } }); - it("skips depths where fetch fails (e.g. --unshallow on already-unshallow repo)", () => { + it("skips bounded depths where fetch fails", () => { let fetchAttempts = 0; let mergeBaseAttempts = 0; const runGit: GitRunners["runGit"] = (args) => { @@ -233,7 +240,7 @@ describe("resolveMergeBase", () => { }; const gitOk: GitRunners["gitOk"] = (args) => { if (args.join(" ") === "rev-parse HEAD") return SHA_C; - if (args.join(" ") === "merge-base origin/main HEAD") { + if (args.join(" ") === "merge-base --all origin/main HEAD") { mergeBaseAttempts++; return SHA_M; } @@ -260,7 +267,7 @@ describe("resolveMergeBase", () => { }; const gitOk = makeGitOk([ { match: (a) => a.join(" ") === "rev-parse HEAD", out: SHA_C }, - { match: (a) => a.join(" ") === "merge-base origin/main HEAD", out: SHA_M }, + { match: (a) => a.join(" ") === "merge-base --all origin/main HEAD", out: SHA_M }, ]); const bearer = { @@ -331,11 +338,13 @@ describe("ensureTargetRefFetched", () => { it("resolves at the first depth and reports the merge base", () => { const depthArgsSeen: string[] = []; const runGit: GitRunners["runGit"] = (args) => { - if (args[0] === "fetch") depthArgsSeen.push(args[2] ?? ""); + if (args[0] === "fetch") { + depthArgsSeen.push(args.find((arg) => arg.startsWith("--depth=")) ?? ""); + } return { stdout: "", stderr: "", status: 0 }; }; const gitOk = makeGitOk([ - { match: (a) => a.join(" ") === "merge-base origin/main HEAD", out: SHA_M }, + { match: (a) => a.join(" ") === "merge-base --all origin/main HEAD", out: SHA_M }, ]); const result = ensureTargetRefFetched("main", {}, { runGit, gitOk }); @@ -348,12 +357,14 @@ describe("ensureTargetRefFetched", () => { it("keeps deepening until merge-base resolves, then stops", () => { const depthArgsSeen: string[] = []; const runGit: GitRunners["runGit"] = (args) => { - if (args[0] === "fetch") depthArgsSeen.push(args[2] ?? ""); + if (args[0] === "fetch") { + depthArgsSeen.push(args.find((arg) => arg.startsWith("--depth=")) ?? ""); + } return { stdout: "", stderr: "", status: 0 }; }; let calls = 0; const gitOk: GitRunners["gitOk"] = (a) => { - if (a.join(" ") === "merge-base origin/main HEAD") { + if (a.join(" ") === "merge-base --all origin/main HEAD") { calls++; // Resolves only on the 3rd depth (--depth=2000). return calls >= 3 ? SHA_M : null; @@ -375,3 +386,208 @@ describe("ensureTargetRefFetched", () => { if (!result.ok) expect(result.reason).toContain("origin/main"); }); }); + +describe("targeted shallow history helpers", () => { + it("fetches exact source/target SHAs at count-derived depths and verifies the base", () => { + const calls: string[][] = []; + const runners: GitRunners = { + runGit: (args) => { + calls.push(args); + return { stdout: "", stderr: "", status: 0 }; + }, + gitOk: (args) => + args.join(" ") === "merge-base --all origin/main HEAD" ? SHA_M : null, + }; + const result = ensureExactMergeBaseFetched( + "main", + { + commonCommit: SHA_M, + aheadCount: 7, + behindCount: 5, + sourceCommit: SHA_A, + targetCommit: SHA_B, + }, + {}, + runners, + ); + expect(result).toEqual({ ok: true, baseSha: SHA_M }); + expect(calls[0]).toContain("--depth=8"); + expect(calls[0]).toContain( + `+${SHA_A}:refs/remotes/origin/ado-aw-prepare-source`, + ); + expect(calls[1]).toContain("--depth=6"); + expect(calls[1]).toContain(`+${SHA_B}:refs/remotes/origin/main`); + }); + + it("rejects an API-directed fetch beyond the automatic safety limit", () => { + let fetches = 0; + const runners: GitRunners = { + runGit: () => { + fetches++; + return { stdout: "", stderr: "", status: 0 }; + }, + gitOk: () => null, + }; + const result = ensureExactMergeBaseFetched( + "main", + { + commonCommit: SHA_M, + aheadCount: 10_000, + behindCount: 0, + sourceCommit: SHA_A, + targetCommit: SHA_B, + }, + {}, + runners, + ); + expect(result.ok).toBe(false); + expect(fetches).toBe(0); + }); + + it("fetches both refs in one bounded command and never unshallows", () => { + const calls: string[][] = []; + const runners: GitRunners = { + runGit: (args) => { + calls.push(args); + return { stdout: "", stderr: "", status: 0 }; + }, + gitOk: (args) => + args.join(" ") === "merge-base --all origin/main HEAD" ? SHA_M : null, + }; + const result = ensureRefsForMergeBaseFetched( + "refs/heads/feature", + "main", + {}, + runners, + ); + expect(result.ok).toBe(true); + expect(calls).toHaveLength(1); + expect(calls[0]).toContain("--depth=200"); + expect(calls[0]).toContain( + "+refs/heads/feature:refs/remotes/origin/ado-aw-prepare-source", + ); + expect(calls[0]).toContain("+refs/heads/main:refs/remotes/origin/main"); + expect(calls.flat()).not.toContain("--unshallow"); + }); + + it("fetches only the target tip for SafeOutputs", () => { + const calls: string[][] = []; + const runners: GitRunners = { + runGit: (args) => { + calls.push(args); + return { stdout: "", stderr: "", status: 0 }; + }, + gitOk: (args) => + args.join(" ") === "rev-parse origin/main" ? SHA_B : null, + }; + expect(ensureTargetTipFetched("main", {}, runners)).toEqual({ + ok: true, + baseSha: SHA_B, + }); + expect(calls).toEqual([ + [ + "fetch", + "--no-tags", + "--no-recurse-submodules", + "--depth=1", + "origin", + "+refs/heads/main:refs/remotes/origin/main", + ], + ]); + }); + + it("preserves a full checkout by omitting depth-limited fetch arguments", () => { + const calls: string[][] = []; + const runners: GitRunners = { + runGit: (args) => { + calls.push(args); + return { stdout: "", stderr: "", status: 0 }; + }, + gitOk: (args) => { + if (args.join(" ") === "rev-parse --is-shallow-repository") return "false"; + if (args.join(" ") === "merge-base --all origin/main HEAD") return SHA_M; + if (args.join(" ") === "rev-parse origin/main") return SHA_B; + return null; + }, + }; + expect( + ensureExactMergeBaseFetched( + "main", + { + commonCommit: SHA_M, + aheadCount: 7, + behindCount: 5, + sourceCommit: SHA_A, + targetCommit: SHA_B, + }, + {}, + runners, + ), + ).toEqual({ ok: true, baseSha: SHA_M }); + expect(calls).toEqual([ + [ + "fetch", + "--no-tags", + "--no-recurse-submodules", + "origin", + `+${SHA_B}:refs/remotes/origin/main`, + ], + ]); + + calls.length = 0; + expect(ensureTargetTipFetched("main", {}, runners).ok).toBe(true); + expect(calls[0]).toEqual([ + "fetch", + "--no-tags", + "--no-recurse-submodules", + "origin", + "+refs/heads/main:refs/remotes/origin/main", + ]); + }); + + it("accepts valid Git ref punctuation and Unicode without shell parsing", () => { + const calls: string[][] = []; + const runners: GitRunners = { + runGit: (args) => { + calls.push(args); + return { stdout: "", stderr: "", status: 0 }; + }, + gitOk: (args) => + args.join(" ") === "merge-base --all origin/release+次 HEAD" + ? SHA_M + : null, + }; + const result = ensureRefsForMergeBaseFetched( + "refs/heads/team's+候補@x", + "release+次", + {}, + runners, + ); + expect(result.ok).toBe(true); + expect(calls[0]).toContain( + "+refs/heads/team's+候補@x:refs/remotes/origin/ado-aw-prepare-source", + ); + expect(calls[0]).toContain( + "+refs/heads/release+次:refs/remotes/origin/release+次", + ); + }); + + it("never shortens an existing shallow history depth", () => { + const calls: string[][] = []; + const runners: GitRunners = { + runGit: (args) => { + calls.push(args); + return { stdout: "", stderr: "", status: 0 }; + }, + gitOk: (args) => { + if (args.join(" ") === "rev-list --count HEAD") return "500"; + if (args.join(" ") === "merge-base --all origin/main HEAD") return SHA_M; + if (args.join(" ") === "rev-parse origin/main") return SHA_B; + return null; + }, + }; + expect(ensureTargetTipFetched("main", {}, runners).ok).toBe(true); + expect(calls[0]).toContain("--depth=500"); + expect(calls[0]).toContain("--no-recurse-submodules"); + }); +}); diff --git a/scripts/ado-script/src/shared/ado-client.ts b/scripts/ado-script/src/shared/ado-client.ts index 07ac0c3d..8798d067 100644 --- a/scripts/ado-script/src/shared/ado-client.ts +++ b/scripts/ado-script/src/shared/ado-client.ts @@ -10,7 +10,10 @@ import type { GitPullRequestIteration, GitPullRequestIterationChanges, } from "azure-devops-node-api/interfaces/GitInterfaces.js"; -import { PullRequestStatus } from "azure-devops-node-api/interfaces/GitInterfaces.js"; +import { + GitVersionType, + PullRequestStatus, +} from "azure-devops-node-api/interfaces/GitInterfaces.js"; import { BuildStatus, type Build } from "azure-devops-node-api/interfaces/BuildInterfaces.js"; const SLEEP_MS = 1000; @@ -23,6 +26,8 @@ const ITERATION_CHANGES_PAGE_SIZE = 100; // to advance the skip cursor. 100 pages × 100 entries = 10 000 changed // files, which is well beyond any realistic PR. const MAX_ITERATION_CHANGE_PAGES = 100; +const SHA40_RE = /^[0-9a-f]{40}$/i; +const MAX_COMMIT_DIFF_COUNT = 1_000_000; const sleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)); @@ -60,19 +65,21 @@ function withTimeout(label: string, ms: number, fn: () => Promise): Promis function isTransient(err: unknown): boolean { if (err instanceof TimeoutError) return true; - if (err && typeof err === "object") { - const e = err as Record; - const sc = - typeof e.statusCode === "number" - ? e.statusCode - : typeof e.response?.status === "number" - ? e.response.status - : undefined; - if (typeof sc === "number" && sc >= 500 && sc < 600) return true; - } + const sc = httpStatusCode(err); + if (typeof sc === "number" && sc >= 500 && sc < 600) return true; return false; } +export function httpStatusCode(err: unknown): number | undefined { + if (!err || typeof err !== "object") return undefined; + const e = err as Record; + return typeof e.statusCode === "number" + ? e.statusCode + : typeof e.response?.status === "number" + ? e.response.status + : undefined; +} + export async function withRetry(label: string, fn: () => Promise): Promise { const ms = timeoutMs(); try { @@ -85,6 +92,75 @@ export async function withRetry(label: string, fn: () => Promise): Promise } } +export interface CommitDiffMetadata { + commonCommit: string; + aheadCount: number; + behindCount: number; + sourceCommit: string; + targetCommit: string; +} + +function requireSha(label: string, value: string | undefined): string { + if (!value || !SHA40_RE.test(value)) { + throw new Error(`${label} is not a 40-character commit SHA`); + } + return value.toLowerCase(); +} + +function requireCount(label: string, value: number | undefined): number { + if ( + !Number.isSafeInteger(value) || + value === undefined || + value < 0 || + value > MAX_COMMIT_DIFF_COUNT + ) { + throw new Error(`${label} is not a valid commit count`); + } + return value; +} + +/** + * Ask Azure Repos for the exact common commit and divergence counts between a + * target branch tip and the checked-out source commit. The target tip is first + * resolved to a SHA so a branch update cannot change the comparison midway + * through the two REST calls. + */ +export async function getCommitDiffMetadata( + project: string, + repositoryId: string, + targetBranch: string, + sourceCommit: string, +): Promise { + const sourceSha = requireSha("sourceCommit", sourceCommit); + return withRetry("getCommitDiffMetadata", async () => { + const git = await (await getWebApi()).getGitApi(); + const branch = await git.getBranch(repositoryId, targetBranch, project); + const targetSha = requireSha("target branch commit", branch.commit?.commitId); + const result = await git.getCommitDiffs( + repositoryId, + project, + true, + 1, + 0, + { + version: targetSha, + versionType: GitVersionType.Commit, + }, + { + version: sourceSha, + versionType: GitVersionType.Commit, + }, + ); + return { + commonCommit: requireSha("commonCommit", result.commonCommit), + aheadCount: requireCount("aheadCount", result.aheadCount), + behindCount: requireCount("behindCount", result.behindCount), + sourceCommit: sourceSha, + targetCommit: targetSha, + }; + }); +} + export async function getPullRequestById( project: string, _repoId: string, diff --git a/scripts/ado-script/src/shared/ado-remote.ts b/scripts/ado-script/src/shared/ado-remote.ts new file mode 100644 index 00000000..9dea0bf8 --- /dev/null +++ b/scripts/ado-script/src/shared/ado-remote.ts @@ -0,0 +1,99 @@ +export interface AdoRepoIdentity { + collectionUri: string; + organization: string; + project: string; + repository: string; +} + +function decodeSegment(value: string): string | null { + try { + const decoded = decodeURIComponent(value); + return decoded.length > 0 ? decoded : null; + } catch { + return null; + } +} + +/** + * Parse Azure DevOps Services Git HTTPS remotes. Unknown/on-premises shapes + * deliberately return null so callers can use their git-only fallback rather + * than guessing repository identity. + */ +export function parseAdoRepoUrl(raw: string): AdoRepoIdentity | null { + let url: URL; + try { + url = new URL(raw); + } catch { + return null; + } + if (url.protocol !== "https:") return null; + + const host = url.hostname.toLowerCase(); + const parts = url.pathname.split("/").filter((part) => part.length > 0); + let organization: string; + let projectPart: string; + let repoPart: string; + let collectionUri: string; + + if (host === "dev.azure.com") { + if (parts.length !== 4 || parts[2]?.toLowerCase() !== "_git") return null; + const orgPart = decodeSegment(parts[0] ?? ""); + if (!orgPart) return null; + organization = orgPart.toLowerCase(); + projectPart = parts[1] ?? ""; + repoPart = parts[3] ?? ""; + collectionUri = `https://dev.azure.com/${orgPart}/`; + } else if (host.endsWith(".visualstudio.com")) { + const hasDefaultCollection = + parts.length === 4 && + parts[0]?.toLowerCase() === "defaultcollection" && + parts[2]?.toLowerCase() === "_git"; + const directProject = + parts.length === 3 && parts[1]?.toLowerCase() === "_git"; + if (!hasDefaultCollection && !directProject) return null; + organization = host.slice(0, -".visualstudio.com".length); + if (organization.length === 0) return null; + projectPart = parts[hasDefaultCollection ? 1 : 0] ?? ""; + repoPart = parts[hasDefaultCollection ? 3 : 2] ?? ""; + collectionUri = hasDefaultCollection + ? `https://${organization}.visualstudio.com/DefaultCollection/` + : `https://${organization}.visualstudio.com/`; + } else { + return null; + } + + const project = decodeSegment(projectPart); + const repository = decodeSegment(repoPart); + if (!project || !repository) return null; + return { collectionUri, organization, project, repository }; +} + +export function adoOrganizationFromCollectionUri(raw: string | undefined): string | null { + if (!raw) return null; + let url: URL; + try { + url = new URL(raw); + } catch { + return null; + } + const host = url.hostname.toLowerCase(); + if (host === "dev.azure.com") { + const org = url.pathname.split("/").find((part) => part.length > 0); + return org ? decodeSegment(org)?.toLowerCase() ?? null : null; + } + if (host.endsWith(".visualstudio.com")) { + const org = host.slice(0, -".visualstudio.com".length); + return org.length > 0 ? org : null; + } + return null; +} + +export function isCurrentAdoOrganization( + identity: AdoRepoIdentity, + env: NodeJS.ProcessEnv, +): boolean { + const current = adoOrganizationFromCollectionUri( + env.SYSTEM_COLLECTIONURI ?? env.SYSTEM_TEAMFOUNDATIONCOLLECTIONURI, + ); + return current !== null && current === identity.organization; +} diff --git a/scripts/ado-script/src/shared/index.ts b/scripts/ado-script/src/shared/index.ts index ade05a4c..0263e3c2 100644 --- a/scripts/ado-script/src/shared/index.ts +++ b/scripts/ado-script/src/shared/index.ts @@ -3,6 +3,7 @@ export * as vso from "./vso-logger.js"; export * as envFacts from "./env-facts.js"; export * as policy from "./policy.js"; export * as adoClient from "./ado-client.js"; +export * as adoRemote from "./ado-remote.js"; // Promoted from exec-context-pr/ during Stage 0 of the contributor // build-out so upcoming contributors (`pipeline`, `ci-push`, // `workitem`, ...) can reuse them without fragmenting the workspace diff --git a/scripts/ado-script/src/shared/merge-base.ts b/scripts/ado-script/src/shared/merge-base.ts index 21b28b00..791d6d18 100644 --- a/scripts/ado-script/src/shared/merge-base.ts +++ b/scripts/ado-script/src/shared/merge-base.ts @@ -1,6 +1,9 @@ import { gitOk as defaultGitOk, runGit as defaultRunGit, type GitResult } from "./git.js"; const SHA40_RE = /^[0-9a-f]{40}$/i; +const TARGETED_DEPTH_LIMIT = 10_000; +const BOUNDED_DEPTHS = [200, 500, 2000] as const; +const SOURCE_TRACKING_REF = "refs/remotes/origin/ado-aw-prepare-source"; export type MergeBaseSuccess = { ok: true; @@ -52,20 +55,20 @@ function countParentTokens(runners: GitRunners): number { * Fetch the PR target branch from origin into * `refs/remotes/origin/` at the given depth. * - * `depthArg` is one of `--depth=N` or `--unshallow` — passed - * verbatim so the caller can iterate the progressive-deepening loop. + * The numeric depth is emitted as `--depth=N` for bounded recovery. */ function fetchBranchAtDepth( runners: GitRunners, branchShort: string, - depthArg: string, + depth: number, env: Record, ): boolean { const result = runners.runGit( [ "fetch", "--no-tags", - depthArg, + "--no-recurse-submodules", + `--depth=${depth}`, "origin", `+refs/heads/${branchShort}:refs/remotes/origin/${branchShort}`, ], @@ -79,9 +82,274 @@ export type FetchDeepenResult = | { ok: true; baseSha: string } | { ok: false; reason: string }; +export type ExactMergeBaseMetadata = { + commonCommit: string; + aheadCount: number; + behindCount: number; + sourceCommit: string; + targetCommit: string; +}; + +function validRef(value: string): boolean { + if ( + !value.startsWith("refs/") || + value.endsWith("/") || + value.endsWith(".") || + value.includes("//") || + value.includes("..") || + value.includes("@{") + ) { + return false; + } + for (const segment of value.split("/")) { + if (!segment || segment.startsWith(".") || segment.endsWith(".lock")) { + return false; + } + } + for (const char of value) { + const code = char.codePointAt(0) ?? 0; + if ( + code <= 0x20 || + code === 0x7f || + ["~", "^", ":", "?", "*", "[", "\\"].includes(char) + ) { + return false; + } + } + return true; +} + +function sourceRefspec(sourceRef: string): string | null { + if (SHA40_RE.test(sourceRef)) { + return `+${sourceRef}:${SOURCE_TRACKING_REF}`; + } + const full = sourceRef.startsWith("refs/") + ? sourceRef + : `refs/heads/${sourceRef}`; + if (!validRef(full)) return null; + return `+${full}:${SOURCE_TRACKING_REF}`; +} + +function targetRefspec(targetShort: string, source = `refs/heads/${targetShort}`): string | null { + if ( + !validRef(`refs/heads/${targetShort}`) || + (!SHA40_RE.test(source) && !validRef(source)) + ) { + return null; + } + return `+${source}:refs/remotes/origin/${targetShort}`; +} + +function localMergeBases(targetShort: string, runners: GitRunners): string[] { + const raw = runners.gitOk(["merge-base", "--all", `origin/${targetShort}`, "HEAD"]); + if (!raw) return []; + return raw + .split(/\s+/) + .filter((sha) => SHA40_RE.test(sha)) + .map((sha) => sha.toLowerCase()); +} + +function checkedDepth(count: number): number | null { + if (!Number.isSafeInteger(count) || count < 0 || count >= TARGETED_DEPTH_LIMIT) { + return null; + } + return count + 1; +} + +function isShallowRepository(runners: GitRunners): boolean { + return runners.gitOk(["rev-parse", "--is-shallow-repository"]) !== "false"; +} + +function currentHistoryFloor(runners: GitRunners): number { + const raw = runners.gitOk(["rev-list", "--count", "HEAD"]) ?? ""; + const count = Number(raw); + return Number.isSafeInteger(count) && count > 0 ? count : 1; +} + +/** + * Fetch exactly enough source and target history to make an Azure + * server-computed common commit locally verifiable. + */ +export function ensureExactMergeBaseFetched( + targetShort: string, + metadata: ExactMergeBaseMetadata, + env: Record, + runners: GitRunners = defaultRunners, +): FetchDeepenResult { + const commonCommit = metadata.commonCommit.toLowerCase(); + const sourceCommit = metadata.sourceCommit.toLowerCase(); + const targetCommit = metadata.targetCommit.toLowerCase(); + if ( + !SHA40_RE.test(commonCommit) || + !SHA40_RE.test(sourceCommit) || + !SHA40_RE.test(targetCommit) + ) { + return { ok: false, reason: "Azure diff metadata contained an invalid commit SHA." }; + } + + const sourceDepth = checkedDepth(metadata.aheadCount); + const targetDepth = checkedDepth(metadata.behindCount); + if (sourceDepth === null || targetDepth === null) { + return { + ok: false, + reason: `Azure diff depth exceeds the ${TARGETED_DEPTH_LIMIT}-commit automatic safety limit.`, + }; + } + + const sourceSpec = sourceRefspec(sourceCommit); + const targetSpec = targetRefspec(targetShort, targetCommit); + if (!sourceSpec || !targetSpec) { + return { ok: false, reason: "Could not construct safe exact-commit fetch refspecs." }; + } + if (isShallowRepository(runners)) { + const historyFloor = currentHistoryFloor(runners); + const effectiveSourceDepth = Math.max(sourceDepth, historyFloor); + const effectiveTargetDepth = Math.max(targetDepth, historyFloor); + const sourceFetch = runners.runGit( + [ + "fetch", + "--no-tags", + "--no-recurse-submodules", + `--depth=${effectiveSourceDepth}`, + "origin", + sourceSpec, + ], + env, + ); + if (sourceFetch.status !== 0) { + return { + ok: false, + reason: `Exact source fetch failed at depth ${effectiveSourceDepth}: ${sourceFetch.stderr.trim()}`, + }; + } + const targetFetch = runners.runGit( + [ + "fetch", + "--no-tags", + "--no-recurse-submodules", + `--depth=${effectiveTargetDepth}`, + "origin", + targetSpec, + ], + env, + ); + if (targetFetch.status !== 0) { + return { + ok: false, + reason: `Exact target fetch failed at depth ${effectiveTargetDepth}: ${targetFetch.stderr.trim()}`, + }; + } + } else { + const targetFetch = runners.runGit( + ["fetch", "--no-tags", "--no-recurse-submodules", "origin", targetSpec], + env, + ); + if (targetFetch.status !== 0) { + return { + ok: false, + reason: `Exact target fetch failed: ${targetFetch.stderr.trim()}`, + }; + } + } + + const bases = localMergeBases(targetShort, runners); + if (!bases.includes(commonCommit)) { + return { + ok: false, + reason: `Azure common commit '${commonCommit}' was not a local best merge-base after targeted fetch.`, + }; + } + return { ok: true, baseSha: commonCommit }; +} + +/** + * Bounded git-only recovery for non-Azure remotes or unavailable REST + * metadata. Source and target are fetched together at every depth so neither + * side of a divergent shallow graph remains truncated. + */ +export function ensureRefsForMergeBaseFetched( + sourceRef: string, + targetShort: string, + env: Record, + runners: GitRunners = defaultRunners, +): FetchDeepenResult { + const sourceSpec = sourceRefspec(sourceRef); + const targetSpec = targetRefspec(targetShort); + if (!sourceSpec || !targetSpec) { + return { ok: false, reason: "Source or target ref is not safe to fetch." }; + } + if (!isShallowRepository(runners)) { + const fetched = runners.runGit( + ["fetch", "--no-tags", "--no-recurse-submodules", "origin", targetSpec], + env, + ); + if (fetched.status === 0) { + const bases = localMergeBases(targetShort, runners); + if (bases.length > 0) return { ok: true, baseSha: bases[0]! }; + } + return { + ok: false, + reason: `Could not resolve merge-base after fetching full-checkout target 'origin/${targetShort}'.`, + }; + } + const historyFloor = currentHistoryFloor(runners); + const depths = [...new Set(BOUNDED_DEPTHS.map((depth) => Math.max(depth, historyFloor)))]; + for (const depth of depths) { + const specs = sourceSpec === targetSpec ? [sourceSpec] : [sourceSpec, targetSpec]; + const fetched = runners.runGit( + [ + "fetch", + "--no-tags", + "--no-recurse-submodules", + `--depth=${depth}`, + "origin", + ...specs, + ], + env, + ); + if (fetched.status !== 0) continue; + const bases = localMergeBases(targetShort, runners); + if (bases.length > 0) { + return { ok: true, baseSha: bases[0]! }; + } + } + return { + ok: false, + reason: `Could not resolve merge-base for source '${sourceRef}' and target 'origin/${targetShort}' after bounded depths 200/500/2000.`, + }; +} + +/** Fetch only the target tip needed by Stage 3's `git worktree add`. */ +export function ensureTargetTipFetched( + targetShort: string, + env: Record, + runners: GitRunners = defaultRunners, +): FetchDeepenResult { + const spec = targetRefspec(targetShort); + if (!spec) return { ok: false, reason: "Target branch is not safe to fetch." }; + const depthArgs = isShallowRepository(runners) + ? [`--depth=${currentHistoryFloor(runners)}`] + : []; + const fetched = runners.runGit( + ["fetch", "--no-tags", "--no-recurse-submodules", ...depthArgs, "origin", spec], + env, + ); + if (fetched.status !== 0) { + return { + ok: false, + reason: `Target-tip fetch failed: ${fetched.stderr.trim()}`, + }; + } + const targetSha = runners.gitOk(["rev-parse", `origin/${targetShort}`]) ?? ""; + if (!SHA40_RE.test(targetSha)) { + return { ok: false, reason: `Fetched target 'origin/${targetShort}' did not resolve to a commit.` }; + } + return { ok: true, baseSha: targetSha.toLowerCase() }; +} + /** * Fetch `origin/` into the local clone and progressively - * deepen it (`--depth=200`, `500`, `2000`, then `--unshallow`) until + * deepen it (`--depth=200`, `500`, `2000`) until * `git merge-base origin/ HEAD` resolves — i.e. until the * clone carries enough history to reach the merge base of HEAD and the * target branch. @@ -101,24 +369,16 @@ export function ensureTargetRefFetched( env: Record, runners: GitRunners = defaultRunners, ): FetchDeepenResult { - // Progressive deepening: stop ONLY when merge-base actually resolves - // against the deepened target ref. - const depths = ["--depth=200", "--depth=500", "--depth=2000", "--unshallow"]; - for (const depthArg of depths) { - if (!fetchBranchAtDepth(runners, targetShort, depthArg, env)) { - // Fetch failed at this depth (e.g. --unshallow on an already- - // unshallowed repo). Continue to the next depth or bail out after - // the loop. - continue; - } - const mb = runners.gitOk(["merge-base", `origin/${targetShort}`, "HEAD"]) ?? ""; - if (mb.length > 0) { - return { ok: true, baseSha: mb }; + for (const depth of BOUNDED_DEPTHS) { + if (!fetchBranchAtDepth(runners, targetShort, depth, env)) continue; + const bases = localMergeBases(targetShort, runners); + if (bases.length > 0) { + return { ok: true, baseSha: bases[0]! }; } } return { ok: false, - reason: `Could not resolve merge-base against 'origin/${targetShort}' after progressive deepening.`, + reason: `Could not resolve merge-base against 'origin/${targetShort}' after bounded depths 200/500/2000.`, }; } @@ -130,13 +390,25 @@ function ensureSyntheticMergeParentsFetched( env: Record, runners: GitRunners, ): FetchDeepenResult { - const depths = ["--depth=200", "--depth=500", "--depth=2000", "--unshallow"]; - for (const depthArg of depths) { - const targetFetched = fetchBranchAtDepth(runners, targetShort, depthArg, env); - const sourceFetched = fetchBranchAtDepth(runners, sourceShort, depthArg, env); - if (!targetFetched && !sourceFetched) { - continue; - } + const sourceSpec = sourceRefspec(sourceShort); + const targetSpec = targetRefspec(targetShort); + if (!sourceSpec || !targetSpec) { + return { ok: false, reason: "Synthetic PR source or target ref is not safe to fetch." }; + } + for (const depth of BOUNDED_DEPTHS) { + const fetched = runners.runGit( + [ + "fetch", + "--no-tags", + "--no-recurse-submodules", + `--depth=${Math.max(depth, currentHistoryFloor(runners))}`, + "origin", + sourceSpec, + targetSpec, + ], + env, + ); + if (fetched.status !== 0) continue; const mb = runners.gitOk(["merge-base", targetParentSha, sourceParentSha]) ?? ""; if (mb.length > 0) { return { ok: true, baseSha: mb }; @@ -144,7 +416,7 @@ function ensureSyntheticMergeParentsFetched( } return { ok: false, - reason: `Could not resolve merge-base between synthetic PR parents after progressive deepening of 'origin/${targetShort}' and 'origin/${sourceShort}'.`, + reason: `Could not resolve merge-base between synthetic PR parents after bounded deepening of 'origin/${targetShort}' and '${sourceShort}'.`, }; } @@ -161,9 +433,9 @@ function ensureSyntheticMergeParentsFetched( * semantics. If the shallow checkout lacks enough ancestry, fetch * the target and source refs with progressive deepening and retry. * - * 2. **Progressive deepening**: when HEAD is a normal commit, fetch - * the target branch with `--depth=200`, `500`, `2000`, `--unshallow` - * until `git merge-base origin/ HEAD` resolves. + * 2. **Bounded deepening**: when HEAD is a normal commit, fetch source and + * target together with `--depth=200`, `500`, `2000` until + * `git merge-base origin/ HEAD` resolves. * * `env` is the result of `bearerEnv(token)` — passed to git's fetch * subprocess so the bearer never leaks into argv or to other tools. @@ -205,10 +477,10 @@ export function resolveMergeBase( } } else { headTipSha = headSha; - // Progressive deepening (extracted into `ensureTargetRefFetched` so - // the `prepare-pr-base` bundle can reuse the identical fetch/deepen - // sequence for its side effect). - const fetched = ensureTargetRefFetched(targetShort, env, runners); + const fetched = + sourceShort.length > 0 + ? ensureRefsForMergeBaseFetched(sourceShort, targetShort, env, runners) + : ensureTargetRefFetched(targetShort, env, runners); if (fetched.ok) { baseSha = fetched.baseSha; } diff --git a/scripts/ado-script/test/smoke.test.ts b/scripts/ado-script/test/smoke.test.ts index 200c9de8..b231ffc1 100644 --- a/scripts/ado-script/test/smoke.test.ts +++ b/scripts/ado-script/test/smoke.test.ts @@ -329,8 +329,7 @@ describe("prepare-pr-base.js smoke", () => { // exactly the shallow-default ADO SafeOutputs-job checkout in #1453. const target = "develop"; - // 1. A "remote" repo: default branch `main`, plus `develop` one commit - // ahead of it. HEAD is left on main (the default branch). + // 1. A "remote" repo with genuinely divergent source/target histories. const originDir = resolve(dir, "origin"); mkdirSync(originDir, { recursive: true }); runGitInRepo(originDir, ["init", "-q", "-b", "main"]); @@ -342,6 +341,14 @@ describe("prepare-pr-base.js smoke", () => { runGitInRepo(originDir, ["add", "-A"]); runGitInRepo(originDir, ["commit", "-q", "-m", "develop commit"]); runGitInRepo(originDir, ["checkout", "-q", "main"]); + writeFileSync(resolve(originDir, "main.txt"), "main\n", "utf8"); + runGitInRepo(originDir, ["add", "-A"]); + runGitInRepo(originDir, ["commit", "-q", "-m", "main commit"]); + const expectedBase = spawnSync( + "git", + ["merge-base", "main", target], + { cwd: originDir, encoding: "utf8" }, + ).stdout.trim(); // 2. SHALLOW single-branch clone of `main` only. A `file://` URL (not a // bare path) forces the transport that honours `--depth`, so the clone @@ -379,7 +386,17 @@ describe("prepare-pr-base.js smoke", () => { // local `file://` remote needs no auth, so SYSTEM_ACCESSTOKEN is empty. const prep = spawnSync( process.execPath, - [preparePrBaseBundlePath, "--repo-dir", checkout, "--target-branch", target], + [ + preparePrBaseBundlePath, + "--mode", + "patch-base", + "--repo-dir", + checkout, + "--source-ref", + "refs/heads/main", + "--target-branch", + target, + ], { env: { ...process.env, SYSTEM_ACCESSTOKEN: "" }, encoding: "utf8" }, ); expect(prep.status).toBe(0); @@ -396,6 +413,12 @@ describe("prepare-pr-base.js smoke", () => { { cwd: checkout, encoding: "utf8" }, ).stdout.trim(); expect(originHead).toBe(`refs/remotes/origin/${target}`); + const actualBase = spawnSync( + "git", + ["merge-base", "HEAD", `origin/${target}`], + { cwd: checkout, encoding: "utf8" }, + ).stdout.trim(); + expect(actualBase).toBe(expectedBase); // 6. The executor's exact worktree operation now SUCCEEDS, checked out at // the target branch's tip. diff --git a/site/src/content/docs/reference/ado-script.mdx b/site/src/content/docs/reference/ado-script.mdx index 70164f66..d6761401 100644 --- a/site/src/content/docs/reference/ado-script.mdx +++ b/site/src/content/docs/reference/ado-script.mdx @@ -24,7 +24,7 @@ pipelines** as runtime helpers. Today it produces fifteen bundles: - **`conclusion.js`** — Conclusion job reporter that files/comments ADO work items for pipeline failures and diagnostic signals (Conclusion job) - **`approval-summary.js`** — renders a sanitized per-tool summary of proposed safe outputs into the build's `ado-aw-safe-outputs` summary tab (end of Agent job) - **`github-app-token.js`** — mints (and revokes) a GitHub App installation token for the Copilot engine when `engine.github-app-token` is configured (Agent + Detection jobs) -- **`prepare-pr-base.js`** — fetches/deepens each allowed create-pull-request repo's target branch (self + every `checkout:` alias, each with its own resolved target) so a PR can be computed and opened on shallow-default agent pools; runs in **both** the Agent job (for the SafeOutputs MCP diff base) and the SafeOutputs job (for the Stage 3 executor's worktree — issue #1453) +- **`prepare-pr-base.js`** — Agent `patch-base` mode uses ADO diff metadata to fetch and verify only the source/target history needed for the merge-base (bounded 200/500/2000 dual-ref fallback); SafeOutputs `target-worktree` mode fetches only the target tip at depth 1 (issue #1453)