Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions docs/ado-script.md
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,8 @@ It performs the work that used to live as ~190 lines of bash heredoc
inside `src/compile/extensions/exec_context/pr.rs`:

1. **Validate identifiers** — `PR_ID`, `SYSTEM_TEAMPROJECT`,
`BUILD_REPOSITORY_NAME`, and `SYSTEM_PULLREQUEST_TARGETBRANCH` are
`BUILD_REPOSITORY_NAME`, `SYSTEM_PULLREQUEST_TARGETBRANCH`, and
`SYSTEM_PULLREQUEST_SOURCEBRANCH` are
each matched against a strict allowlist regex (`validate.ts`)
before any of them are interpolated into a git refspec or the
agent prompt. On any failure the program writes
Expand All @@ -199,10 +200,12 @@ inside `src/compile/extensions/exec_context/pr.rs`:
2. **Resolve merge-base** — if the checkout is a synthetic
merge-commit (parent count ≥ 3 per ADO's PR-validation flow),
`merge-base.ts::resolveMergeBase` computes `git merge-base` over
the two parents. Otherwise it fetches the target branch with
progressive deepening (`--depth=200/500/2000/--unshallow`) and
then `git merge-base` against `HEAD`. Same `BASE_SHA` semantics
in both paths (git's true common ancestor).
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
`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
.../base.sha)..$(cat .../head.sha)` itself.
Expand Down
8 changes: 6 additions & 2 deletions docs/execution-context.md
Original file line number Diff line number Diff line change
Expand Up @@ -474,14 +474,18 @@ under `scripts/ado-script/src/exec-context-pr/`. The bundle:
discovery — ADO already populates these.
2. **Validates identifiers** with strict allowlist regexes
(`PR_ID` ⊆ digits, `PROJECT`/`REPO` ⊆ alphanumeric + `._-`,
`PROJECT` additionally allows space, `PR_TARGET_BRANCH` ⊆
`PROJECT` additionally allows space, `PR_TARGET_BRANCH` and
`PR_SOURCE_BRANCH` ⊆
alphanumeric + `._/-`). See `validate.ts`. Failure writes
`error.txt` and appends the failure prompt fragment.
3. **Detects merge-commit shape.** If `HEAD` has ≥ 3 tokens in
`git rev-list --parents HEAD` (the synthetic merge commit ADO
checks out for PR builds), uses `HEAD^2` as the PR head and
computes `git merge-base HEAD^1 HEAD^2` as the base — same
semantics as the deepening path, no target-branch fetch needed.
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
closed into `error.txt`.
Otherwise:
4. **Fetches the PR target branch with progressive deepening** —
`--depth=200`, then `500`, then `2000`, then finally `--unshallow`.
Expand Down
2 changes: 1 addition & 1 deletion scripts/ado-script/src/exec-context-pr/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ export function main(env: NodeJS.ProcessEnv = process.env): number {
const ids = idsOrErr;

const fetchEnv = bearerEnv(env.SYSTEM_ACCESSTOKEN);
const mb = resolveMergeBase(ids.targetShort, fetchEnv);
const mb = resolveMergeBase(ids.targetShort, fetchEnv, undefined, ids.sourceShort);
if (!mb.ok) {
writeFailure(prDir, promptPath, mb.reason, { prId: ids.prId, project: ids.project, repo: ids.repo });
return 0;
Expand Down
72 changes: 61 additions & 11 deletions scripts/ado-script/src/shared/__tests__/merge-base.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,26 +58,76 @@ describe("resolveMergeBase", () => {
}
});

it("falls back to HEAD^1 when synthetic-merge merge-base cannot resolve", () => {
const runGit = makeRunGit([
{
match: (a) => a.join(" ") === "rev-list --parents -n 1 HEAD",
result: { stdout: `${SHA_C} ${SHA_A} ${SHA_B}\n`, stderr: "", status: 0 },
},
it("deepens target and source refs when synthetic-merge merge-base cannot initially resolve", () => {
const depthArgsSeen: string[] = [];
const refsSeen: string[] = [];
const bearer = {
GIT_CONFIG_COUNT: "1",
GIT_CONFIG_KEY_0: "http.extraheader",
GIT_CONFIG_VALUE_0: "Authorization: bearer test-token",
};
const runGit: GitRunners["runGit"] = (args, env) => {
if (args.join(" ") === "rev-list --parents -n 1 HEAD") {
return { stdout: `${SHA_C} ${SHA_A} ${SHA_B}\n`, stderr: "", status: 0 };
}
if (args[0] === "fetch") {
expect(env).toEqual(bearer);
depthArgsSeen.push(args[2] ?? "");
refsSeen.push(args[4] ?? "");
return { stdout: "", stderr: "", status: 0 };
}
return { stdout: "", stderr: "no handler", status: 1 };
};
let mergeBaseCalls = 0;
const gitOk: GitRunners["gitOk"] = (args) => {
if (args.join(" ") === "rev-parse HEAD") return SHA_C;
if (args.join(" ") === "rev-parse HEAD^1") return SHA_A;
if (args.join(" ") === "rev-parse HEAD^2") return SHA_B;
if (args.join(" ") === `merge-base ${SHA_A} ${SHA_B}`) {
mergeBaseCalls++;
return mergeBaseCalls < 2 ? null : SHA_M;
}
return null;
};

const result = resolveMergeBase("main", bearer, { runGit, gitOk }, "feature/x");
expect(result.ok).toBe(true);
if (result.ok) {
expect(result.baseSha).toBe(SHA_M);
expect(result.headSha).toBe(SHA_B);
}
expect(depthArgsSeen).toEqual(["--depth=200", "--depth=200"]);
expect(refsSeen).toEqual([
"+refs/heads/main:refs/remotes/origin/main",
"+refs/heads/feature/x:refs/remotes/origin/feature/x",
]);
});

it("fails closed when synthetic-merge merge-base cannot resolve after deepening", () => {
let fetchCount = 0;
const runGit: GitRunners["runGit"] = (args) => {
if (args.join(" ") === "rev-list --parents -n 1 HEAD") {
return { stdout: `${SHA_C} ${SHA_A} ${SHA_B}\n`, stderr: "", status: 0 };
}
if (args[0] === "fetch") {
fetchCount++;
return { stdout: "", stderr: "", status: 0 };
}
return { stdout: "", stderr: "no handler", status: 1 };
};
const gitOk = makeGitOk([
{ match: (a) => a.join(" ") === "rev-parse HEAD", out: SHA_C },
{ match: (a) => a.join(" ") === "rev-parse HEAD^1", out: SHA_A },
{ match: (a) => a.join(" ") === "rev-parse HEAD^2", out: SHA_B },
{ match: (a) => a.join(" ") === `merge-base ${SHA_A} ${SHA_B}`, out: null },
]);

const result = resolveMergeBase("main", {}, { runGit, gitOk });
expect(result.ok).toBe(true);
if (result.ok) {
expect(result.baseSha).toBe(SHA_A);
expect(result.headSha).toBe(SHA_B);
const result = resolveMergeBase("main", {}, { runGit, gitOk }, "feature/x");
expect(result.ok).toBe(false);
if (!result.ok) {
expect(result.reason).toContain("Could not resolve base/head SHAs");
}
expect(fetchCount).toBe(8);
});

it("uses progressive deepening when HEAD has only 1 parent and stops on first resolution", () => {
Expand Down
2 changes: 2 additions & 0 deletions scripts/ado-script/src/shared/__tests__/prompt.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ const ids: Identifiers = {
project: "MyProject",
repo: "my-repo",
targetBranch: "refs/heads/main",
sourceBranch: "refs/heads/feature/x",
targetShort: "main",
sourceShort: "feature/x",
};

describe("successFragment", () => {
Expand Down
25 changes: 24 additions & 1 deletion scripts/ado-script/src/shared/__tests__/validate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ function env(overrides: Record<string, string> = {}): NodeJS.ProcessEnv {
return {
SYSTEM_PULLREQUEST_PULLREQUESTID: "4242",
SYSTEM_PULLREQUEST_TARGETBRANCH: "refs/heads/main",
SYSTEM_PULLREQUEST_SOURCEBRANCH: "refs/heads/feature/x",
SYSTEM_TEAMPROJECT: "MyProject",
BUILD_REPOSITORY_NAME: "my-repo",
...overrides,
Expand All @@ -21,7 +22,9 @@ describe("validateIdentifiers", () => {
expect(result.project).toBe("MyProject");
expect(result.repo).toBe("my-repo");
expect(result.targetBranch).toBe("refs/heads/main");
expect(result.sourceBranch).toBe("refs/heads/feature/x");
expect(result.targetShort).toBe("main");
expect(result.sourceShort).toBe("feature/x");
}
});

Expand Down Expand Up @@ -75,6 +78,22 @@ describe("validateIdentifiers", () => {
}
});

it("rejects empty source branch with a dedicated message", () => {
const result = validateIdentifiers(env({ SYSTEM_PULLREQUEST_SOURCEBRANCH: "" }));
expect(isIdentifierError(result)).toBe(true);
if (isIdentifierError(result)) {
expect(result.reason).toContain("SourceBranch is empty");
}
});

it("rejects source branch with disallowed characters", () => {
const result = validateIdentifiers(env({ SYSTEM_PULLREQUEST_SOURCEBRANCH: "refs/heads/feature; rm -rf /" }));
expect(isIdentifierError(result)).toBe(true);
if (isIdentifierError(result)) {
expect(result.reason).toContain("PR_SOURCE_BRANCH=");
}
});

it("accepts branch names with slashes, dots, dashes, underscores", () => {
const result = validateIdentifiers(env({ SYSTEM_PULLREQUEST_TARGETBRANCH: "refs/heads/release/v1.2.3-beta_rc" }));
expect(isIdentifierError(result)).toBe(false);
Expand All @@ -84,10 +103,14 @@ describe("validateIdentifiers", () => {
});

it("handles non-refs/heads/-prefixed branch as a bare name", () => {
const result = validateIdentifiers(env({ SYSTEM_PULLREQUEST_TARGETBRANCH: "main" }));
const result = validateIdentifiers(env({
SYSTEM_PULLREQUEST_TARGETBRANCH: "main",
SYSTEM_PULLREQUEST_SOURCEBRANCH: "feature/x",
}));
expect(isIdentifierError(result)).toBe(false);
if (!isIdentifierError(result)) {
expect(result.targetShort).toBe("main");
expect(result.sourceShort).toBe("feature/x");
}
});

Expand Down
54 changes: 48 additions & 6 deletions scripts/ado-script/src/shared/merge-base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,9 @@ function countParentTokens(runners: GitRunners): number {
* `depthArg` is one of `--depth=N` or `--unshallow` — passed
* verbatim so the caller can iterate the progressive-deepening loop.
*/
function fetchTargetAtDepth(
function fetchBranchAtDepth(
runners: GitRunners,
targetShort: string,
branchShort: string,
depthArg: string,
env: Record<string, string>,
): boolean {
Expand All @@ -67,7 +67,7 @@ function fetchTargetAtDepth(
"--no-tags",
depthArg,
"origin",
`+refs/heads/${targetShort}:refs/remotes/origin/${targetShort}`,
`+refs/heads/${branchShort}:refs/remotes/origin/${branchShort}`,
],
env,
);
Expand Down Expand Up @@ -105,7 +105,7 @@ export function ensureTargetRefFetched(
// against the deepened target ref.
const depths = ["--depth=200", "--depth=500", "--depth=2000", "--unshallow"];
for (const depthArg of depths) {
if (!fetchTargetAtDepth(runners, targetShort, depthArg, env)) {
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.
Expand All @@ -122,6 +122,32 @@ export function ensureTargetRefFetched(
};
}

function ensureSyntheticMergeParentsFetched(
targetShort: string,
sourceShort: string,
targetParentSha: string,
sourceParentSha: string,
env: Record<string, string>,
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 mb = runners.gitOk(["merge-base", targetParentSha, sourceParentSha]) ?? "";
if (mb.length > 0) {
return { ok: true, baseSha: mb };
}
}
return {
ok: false,
reason: `Could not resolve merge-base between synthetic PR parents after progressive deepening of 'origin/${targetShort}' and 'origin/${sourceShort}'.`,
};
}

/**
* Resolve `BASE_SHA` and `HEAD_SHA` for the PR.
*
Expand All @@ -132,7 +158,8 @@ export function ensureTargetRefFetched(
* default checkout mode for PR builds), `HEAD^1` is the target tip
* at PR preparation time and `HEAD^2` is the PR head. We compute
* `merge-base HEAD^1 HEAD^2` to match the deepening path's
* semantics. Falls back to `HEAD^1` if `merge-base` cannot resolve.
* 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`
Expand All @@ -145,6 +172,7 @@ export function resolveMergeBase(
targetShort: string,
env: Record<string, string>,
runners: GitRunners = defaultRunners,
sourceShort = "",
): MergeBaseResult {
const headSha = runners.gitOk(["rev-parse", "HEAD"]) ?? "";
const parentTokens = countParentTokens(runners);
Expand All @@ -159,7 +187,21 @@ export function resolveMergeBase(
headTipSha = p2;
if (p1.length > 0 && p2.length > 0) {
const mergeBase = runners.gitOk(["merge-base", p1, p2]) ?? "";
baseSha = mergeBase.length > 0 ? mergeBase : p1;
if (mergeBase.length > 0) {
baseSha = mergeBase;
} else if (sourceShort.length > 0) {
const fetched = ensureSyntheticMergeParentsFetched(
targetShort,
sourceShort,
p1,
p2,
env,
runners,
);
if (fetched.ok) {
baseSha = fetched.baseSha;
}
}
}
} else {
headTipSha = headSha;
Expand Down
22 changes: 18 additions & 4 deletions scripts/ado-script/src/shared/validate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,11 @@ export const PROJECT_RE = /^[A-Za-z0-9._ -]+$/;
// Repository names have no spaces.
export const REPO_RE = /^[A-Za-z0-9._-]+$/;

// PR target branch is interpolated into a git refspec
// PR target/source branches are interpolated into git refspecs
// ("+refs/heads/<short>:refs/remotes/origin/<short>"), so it must be a
// valid git branch name. The character set is what git itself accepts
// for `refs/heads/<name>`.
export const TARGET_BRANCH_RE = /^[A-Za-z0-9._/-]+$/;
export const PR_BRANCH_RE = /^[A-Za-z0-9._/-]+$/;

export type IdentifierError = {
/** A one-line reason, safe to embed in the agent prompt verbatim. */
Expand All @@ -46,8 +46,10 @@ export type Identifiers = {
project: string;
repo: string;
targetBranch: string;
sourceBranch: string;
/** The short branch name (`refs/heads/foo` -> `foo`). */
targetShort: string;
sourceShort: string;
};

/**
Expand Down Expand Up @@ -83,6 +85,7 @@ export function sanitizeForPrompt(value: string, maxLen = 80): string {
export function validateIdentifiers(env: NodeJS.ProcessEnv): Identifiers | IdentifierError {
const prId = env.SYSTEM_PULLREQUEST_PULLREQUESTID ?? "";
const targetBranch = env.SYSTEM_PULLREQUEST_TARGETBRANCH ?? "";
const sourceBranch = env.SYSTEM_PULLREQUEST_SOURCEBRANCH ?? "";
const project = env.SYSTEM_TEAMPROJECT ?? "";
const repo = env.BUILD_REPOSITORY_NAME ?? "";

Expand All @@ -98,17 +101,28 @@ export function validateIdentifiers(env: NodeJS.ProcessEnv): Identifiers | Ident
if (targetBranch.length === 0) {
return { reason: "System.PullRequest.TargetBranch is empty; cannot resolve merge-base." };
}
if (!TARGET_BRANCH_RE.test(targetBranch)) {
if (!PR_BRANCH_RE.test(targetBranch)) {
return {
reason: `PR identifier validation failed (PR_TARGET_BRANCH='${sanitizeForPrompt(targetBranch)}' contains disallowed characters).`,
};
}
if (sourceBranch.length === 0) {
return { reason: "System.PullRequest.SourceBranch is empty; cannot resolve merge-base." };
}
if (!PR_BRANCH_RE.test(sourceBranch)) {
return {
reason: `PR identifier validation failed (PR_SOURCE_BRANCH='${sanitizeForPrompt(sourceBranch)}' contains disallowed characters).`,
};
}

const targetShort = targetBranch.startsWith("refs/heads/")
? targetBranch.slice("refs/heads/".length)
: targetBranch;
const sourceShort = sourceBranch.startsWith("refs/heads/")
? sourceBranch.slice("refs/heads/".length)
: sourceBranch;

return { prId, project, repo, targetBranch, targetShort };
return { prId, project, repo, targetBranch, sourceBranch, targetShort, sourceShort };
}

/** Type guard distinguishing the validated identifiers from an error. */
Expand Down
1 change: 1 addition & 0 deletions scripts/ado-script/test/smoke.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,7 @@ describe("exec-context-pr.js smoke", () => {
AW_AGENT_PROMPT_FILE: agentPromptPath,
SYSTEM_PULLREQUEST_PULLREQUESTID: "4242",
SYSTEM_PULLREQUEST_TARGETBRANCH: "refs/heads/main",
SYSTEM_PULLREQUEST_SOURCEBRANCH: "refs/heads/feature",
SYSTEM_TEAMPROJECT: "SmokeProject",
BUILD_REPOSITORY_NAME: "smoke-repo",
},
Expand Down
4 changes: 3 additions & 1 deletion site/src/content/docs/reference/ado-script.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,9 @@ Two paths, both computing the same "merge-base of target tip and PR head":

1. **Synthetic merge commit** — ADO's default PR checkout produces a merge
commit (`HEAD` has ≥ 2 parents). `HEAD^1` is the target tip; `HEAD^2` is
the PR head. The bundle runs `git merge-base HEAD^1 HEAD^2`.
the PR head. The bundle runs `git merge-base HEAD^1 HEAD^2`; if that fails
in a shallow checkout, it fetches both target and source refs with
progressive deepening and retries before failing closed.
2. **Progressive deepening** — when `HEAD` is a normal commit (shallow clone
without a merge commit), the bundle fetches the target branch at increasing
depths (`--depth=200`, `500`, `2000`, `--unshallow`) until
Expand Down
Loading