diff --git a/docs/safe-outputs.md b/docs/safe-outputs.md index c93a0793..f8b0dfca 100644 --- a/docs/safe-outputs.md +++ b/docs/safe-outputs.md @@ -30,6 +30,13 @@ safe-outputs: - agent-created work-items: - 12345 + update-pr: + allowed-operations: + - add-reviewers + allowed-reviewers: + - "user@example.com" + max-reviewers: 3 + max: 2 ``` Safe output configurations are passed to Stage 3 execution and used when processing safe outputs. @@ -1102,6 +1109,11 @@ This hybrid approach combines: Note: The source branch name is auto-generated from a sanitized version of the PR title plus a unique suffix (e.g., `agent/fix-bug-in-parser-a1b2c3`). This format is human-readable while preventing injection attacks. +The tool response includes a generated temporary PR ID such as `#aw_a1b2c3`. +The agent can pass that value as `pull_request_id` to later `update-pr` calls in +the same SafeOutputs job. The ID is generated by the MCP server and is not an +input to `create-pull-request`. + **Configuration options (front matter):** - `target-branch` - Target (base) branch the PR merges into (default: "main"). A plain literal branch name, applied to every repo unless overridden below. @@ -1146,7 +1158,7 @@ Note: The source branch name is auto-generated from a sanitized version of the P - `protected-files` - Controls whether manifest/CI files (e.g., `package-lock.json`, `.github/`, `*.lock`) can be modified: `"blocked"` (default, reject changes to these files) or `"allowed"` (permit all files) - `excluded-files` - Glob patterns for files to strip from the patch before applying (e.g., `["*.lock", "dist/**"]`) - `allowed-labels` - Allowlist of labels the agent is permitted to apply. If empty (default), any labels are accepted. -- `reviewers` - List of reviewer emails to add +- `reviewers` - List of reviewer emails or Azure DevOps user IDs to add - `labels` - List of labels to apply - `work-items` - List of work item IDs to link - `fallback-record-branch` - When PR creation fails, record the pushed branch name and target branch in the failure response so operators can manually create the PR (default: true) @@ -1277,7 +1289,7 @@ safe-outputs: Updates pull request metadata (reviewers, labels, auto-complete, vote, description). **Agent parameters:** -- `pull_request_id` - The PR ID to update (required) +- `pull_request_id` - A positive numeric PR ID, a quoted positive numeric ID, or a temporary ID (`#aw_...`) returned by an earlier `create-pull-request` call in the same SafeOutputs job (required) - `operation` - Update operation: `add-reviewers`, `add-labels`, `set-auto-complete`, `vote`, or `update-description` (required) - `reviewers` - Reviewer emails (required for `add-reviewers`) - `labels` - Label names (required for `add-labels`) @@ -1291,12 +1303,41 @@ safe-outputs: update-pr: allowed-operations: [] # Optional — restrict which operations are permitted (empty = all) allowed-repositories: [] # Optional — restrict which repos can be updated + allowed-reviewers: [] # Optional — non-empty list restricts reviewers; empty or ["*"] permits any valid reviewer + max-reviewers: 3 # Maximum reviewers in one add-reviewers call (default: 3) allowed-votes: [] # REQUIRED for vote operation — empty rejects all votes delete-source-branch: true # For set-auto-complete (default: true) merge-strategy: "squash" # For set-auto-complete: squash, noFastForward, rebase, rebaseMerge max: 1 # Maximum per run (default: 1) ``` +When `allowed-reviewers` is omitted or empty, any otherwise-valid reviewer is +permitted, matching gh-aw's reviewer policy. A non-empty list restricts +reviewers using case-insensitive exact matching; `["*"]` is an explicit +unrestricted form. Non-GUID reviewer values must also exactly match an Azure +DevOps identity email, account name, or display name; fuzzy Identity Picker +results are not selected. Reviewer identity or API failures return a warning +with structured `added` and `failed` arrays. Invalid configuration, disallowed +reviewers, and unresolved PR references fail before reviewer writes begin. + +Temporary PR references are resolved in safe-output proposal order, so +`create-pull-request` must appear before its `update-pr` entries. They are +in-memory references scoped to one SafeOutputs job: automatic and manually +reviewed safe outputs execute in separate jobs and cannot share a temporary ID. +When both tools are configured, the compiler therefore requires them to have +the same effective `require-approval` setting. +Each follow-up call counts against `update-pr.max`. + +Example agent call sequence: + +```json +{"title":"Update dependencies","description":"Refresh dependencies and related tests."} +{"pull_request_id":"#aw_a1b2c3","operation":"add-reviewers","reviewers":["user@example.com"]} +``` + +The first line represents the `create-pull-request` call; use the actual +temporary ID returned by that call in the later `update-pr` call. + ### link-work-items Links two Azure DevOps work items together. diff --git a/scripts/ado-script/src/executor-e2e/__tests__/ado-rest.test.ts b/scripts/ado-script/src/executor-e2e/__tests__/ado-rest.test.ts index 5c3cba3e..293e9d8a 100644 --- a/scripts/ado-script/src/executor-e2e/__tests__/ado-rest.test.ts +++ b/scripts/ado-script/src/executor-e2e/__tests__/ado-rest.test.ts @@ -19,6 +19,79 @@ describe("AdoRest.workItemTypeExists", () => { vi.unstubAllGlobals(); }); + describe("AdoRest.resolveIdentityId", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("passes GUID identities through without a request", async () => { + const fetchMock = vi.fn(); + vi.stubGlobal("fetch", fetchMock); + + await expect( + new AdoRest(options).resolveIdentityId( + "01234567-89ab-cdef-0123-456789abcdef", + ), + ).resolves.toBe("01234567-89ab-cdef-0123-456789abcdef"); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it("encodes the identity query and accepts one case-insensitive exact match", async () => { + const fetchMock = stubFetch( + () => + new Response( + JSON.stringify({ + value: [ + { + id: "reviewer-id", + displayName: "Near Match", + properties: { + Mail: { $value: "REQUESTER+E2E@example.com" }, + }, + }, + ], + }), + { + status: 200, + headers: { "content-type": "application/json" }, + }, + ), + ); + + await expect( + new AdoRest(options).resolveIdentityId("requester+e2e@example.com"), + ).resolves.toBe("reviewer-id"); + expect(fetchMock.mock.calls[0]?.[0]).toBe( + "https://vssps.dev.azure.com/org/_apis/identities?searchFilter=General&filterValue=requester%2Be2e%40example.com&api-version=7.1", + ); + }); + + it("rejects ambiguous exact matches", async () => { + stubFetch( + () => + new Response( + JSON.stringify({ + value: [ + { id: "one", providerDisplayName: "owner@example.com" }, + { + id: "two", + properties: { Account: { $value: "OWNER@example.com" } }, + }, + ], + }), + { + status: 200, + headers: { "content-type": "application/json" }, + }, + ), + ); + + await expect( + new AdoRest(options).resolveIdentityId("owner@example.com"), + ).resolves.toBeUndefined(); + }); + }); + describe("AdoRest authentication", () => { afterEach(() => { vi.unstubAllGlobals(); diff --git a/scripts/ado-script/src/executor-e2e/__tests__/create-pull-request-scenarios.test.ts b/scripts/ado-script/src/executor-e2e/__tests__/create-pull-request-scenarios.test.ts new file mode 100644 index 00000000..e9fa1d5a --- /dev/null +++ b/scripts/ado-script/src/executor-e2e/__tests__/create-pull-request-scenarios.test.ts @@ -0,0 +1,265 @@ +import { describe, expect, it } from "vitest"; + +import type { ExecutedRecord, ScenarioContext } from "../scenario.js"; +import { SkipError } from "../scenario.js"; +import { + createPullRequestAddReviewers, + createPullRequestScenarios, + resolveExecutorE2eReviewer, +} from "../scenarios/create-pull-request.js"; + +const ctx = { + orgUrl: "https://dev.azure.com/org/", + project: "P", + adoRepo: "agent-definitions", + buildId: "77", + token: "ado-token", + adoAwBin: "ado-aw", + workDir: "work", + rest: {}, + log: () => {}, + prefix: (tool: string) => `ado-aw-det-77-${tool}`, +} as unknown as ScenarioContext; + +type AddReviewersState = Parameters< + typeof createPullRequestAddReviewers.config +>[1]; + +const state = { + repo: "agent-definitions", + sourceBranch: "source", + targetBranch: "main", + baseCommit: "a".repeat(40), + patchRelPath: "create-pr-add-reviewers.patch", + patchSha256: "b".repeat(64), + patchContent: "patch", + sourcesDir: "sources", + checkoutDir: "checkout", + rest: {}, + executorToken: "token", + repositorySelector: "agent-definitions", + reviewer: "requester@example.com", + reviewerId: "reviewer-id", +} as unknown as AddReviewersState; + +describe("resolveExecutorE2eReviewer", () => { + it("trims the dedicated reviewer environment value", () => { + expect( + resolveExecutorE2eReviewer({ + EXECUTOR_E2E_REVIEWER: " requester@example.com ", + }), + ).toBe("requester@example.com"); + }); + + it.each([{}, { EXECUTOR_E2E_REVIEWER: " " }, { + EXECUTOR_E2E_REVIEWER: "$(Build.RequestedForEmail)", + }])("skips unavailable or unexpanded values", (env) => { + expect(() => resolveExecutorE2eReviewer(env)).toThrow(SkipError); + }); +}); + +describe("create-pull-request add-reviewers handoff", () => { + it("is registered with the constrained reviewer policy", () => { + const ids = createPullRequestScenarios.map( + (scenario) => scenario.id ?? scenario.tool, + ); + expect(ids).toContain("create-pull-request-add-reviewers"); + expect(createPullRequestAddReviewers.config(ctx, state)).toEqual({ + "allowed-operations": ["add-reviewers"], + "allowed-repositories": ["agent-definitions"], + "allowed-reviewers": ["requester@example.com"], + "max-reviewers": 1, + max: 1, + }); + }); + + it("stages create first and submits the reviewer against its temporary ID", async () => { + const prior = await createPullRequestAddReviewers.priorEntries!(ctx, state); + expect(prior).toEqual([ + expect.objectContaining({ + tool: "create-pull-request", + entry: expect.objectContaining({ + temporary_id: "#aw_prreviewers", + source_branch: "source", + }), + }), + ]); + await expect( + createPullRequestAddReviewers.ndjson(ctx, state), + ).resolves.toEqual({ + pull_request_id: "#aw_prreviewers", + operation: "add-reviewers", + reviewers: ["requester@example.com"], + }); + }); + + it("asserts temporary-ID resolution and live reviewer membership by identity ID", async () => { + const listReviewers = async () => [ + { id: "REVIEWER-ID", vote: 0, displayName: "Requester" }, + ]; + const assertionState = { + ...state, + rest: { listReviewers }, + } as unknown as AddReviewersState; + const created: ExecutedRecord = { + name: "create_pull_request", + status: "succeeded", + result: { + pull_request_id: 42, + temporary_id: "#aw_prreviewers", + }, + }; + const updated: ExecutedRecord = { + name: "update_pr", + status: "succeeded", + result: { + pull_request_id: 42, + operation: "add-reviewers", + added: ["REQUESTER@example.com"], + failed: [], + }, + }; + + await expect( + createPullRequestAddReviewers.assert( + ctx, + assertionState, + updated, + [created, updated], + ), + ).resolves.toBeUndefined(); + expect(assertionState.prId).toBe(42); + }); + + it.each([ + { + name: "the producer temporary ID differs", + created: { + pull_request_id: 42, + temporary_id: "#aw_wrong", + }, + updated: { + pull_request_id: 42, + operation: "add-reviewers", + added: ["requester@example.com"], + failed: [], + }, + }, + { + name: "the consumer resolves a different PR", + created: { + pull_request_id: 42, + temporary_id: "#aw_prreviewers", + }, + updated: { + pull_request_id: 43, + operation: "add-reviewers", + added: ["requester@example.com"], + failed: [], + }, + }, + { + name: "the operation differs", + created: { + pull_request_id: 42, + temporary_id: "#aw_prreviewers", + }, + updated: { + pull_request_id: 42, + operation: "update-description", + added: ["requester@example.com"], + failed: [], + }, + }, + { + name: "a reviewer fails", + created: { + pull_request_id: 42, + temporary_id: "#aw_prreviewers", + }, + updated: { + pull_request_id: 42, + operation: "add-reviewers", + added: [], + failed: ["requester@example.com (HTTP 403)"], + }, + }, + { + name: "the configured reviewer is absent from added", + created: { + pull_request_id: 42, + temporary_id: "#aw_prreviewers", + }, + updated: { + pull_request_id: 42, + operation: "add-reviewers", + added: ["someone@example.com"], + failed: [], + }, + }, + ])("rejects when $name", async ({ created, updated }) => { + const assertionState = { + ...state, + rest: { + listReviewers: async () => [ + { id: "reviewer-id", vote: 0, displayName: "Requester" }, + ], + }, + } as unknown as AddReviewersState; + const records: ExecutedRecord[] = [ + { + name: "create_pull_request", + status: "succeeded", + result: created, + }, + { + name: "update_pr", + status: "succeeded", + result: updated, + }, + ]; + + await expect( + createPullRequestAddReviewers.assert( + ctx, + assertionState, + records[1]!, + records, + ), + ).rejects.toThrow(); + }); + + it("rejects when the reviewer is absent from live ADO state", async () => { + const assertionState = { + ...state, + rest: { listReviewers: async () => [] }, + } as unknown as AddReviewersState; + const created: ExecutedRecord = { + name: "create_pull_request", + status: "succeeded", + result: { + pull_request_id: 42, + temporary_id: "#aw_prreviewers", + }, + }; + const updated: ExecutedRecord = { + name: "update_pr", + status: "succeeded", + result: { + pull_request_id: 42, + operation: "add-reviewers", + added: ["requester@example.com"], + failed: [], + }, + }; + + await expect( + createPullRequestAddReviewers.assert( + ctx, + assertionState, + updated, + [created, updated], + ), + ).rejects.toThrow("does not contain reviewer identity"); + }); +}); diff --git a/scripts/ado-script/src/executor-e2e/__tests__/index.test.ts b/scripts/ado-script/src/executor-e2e/__tests__/index.test.ts index 2c266930..8188c270 100644 --- a/scripts/ado-script/src/executor-e2e/__tests__/index.test.ts +++ b/scripts/ado-script/src/executor-e2e/__tests__/index.test.ts @@ -26,6 +26,8 @@ describe("scenario registry", () => { expect(ids).toContain("create-pull-request"); expect(ids).toContain("create-pull-request-self-multi-checkout"); expect(ids).toContain("create-pull-request-cross-org"); + expect(ids).toContain("create-pull-request-temporary-id-handoff"); + expect(ids).toContain("create-pull-request-add-reviewers"); expect(ids).toContain("create-branch-cross-org"); expect(ids).toContain("create-git-tag-cross-org"); }); diff --git a/scripts/ado-script/src/executor-e2e/__tests__/runner.test.ts b/scripts/ado-script/src/executor-e2e/__tests__/runner.test.ts index 665e0ea0..477e987d 100644 --- a/scripts/ado-script/src/executor-e2e/__tests__/runner.test.ts +++ b/scripts/ado-script/src/executor-e2e/__tests__/runner.test.ts @@ -173,6 +173,7 @@ fs.writeFileSync( function handoffScenario( onAssert: (records: ExecutedRecord[]) => void, + onCleanup: (records: ExecutedRecord[] | undefined) => void = () => {}, ): Scenario { return { id: "prior-entry-handoff", @@ -184,7 +185,7 @@ fs.writeFileSync( ], ndjson: async () => ({ issue_number: "#aw_x1" }), assert: async (_ctx, _state, _record, records) => onAssert(records), - cleanup: async () => {}, + cleanup: async (_ctx, _state, records) => onCleanup(records), }; } @@ -240,6 +241,39 @@ fs.writeFileSync( } }); + it("exposes prior records to cleanup when the primary entry fails", async () => { + const dir = await mkdtemp(join(tmpdir(), "ado-aw-runner-primary-fail-")); + try { + const bin = await writeEchoBin(dir, { "set-github-issue-type": "failed" }); + let asserted = false; + let cleanedRecords: ExecutedRecord[] | undefined; + const res = await runScenario( + { ...fakeCtx(), adoAwBin: bin, workDir: dir }, + handoffScenario( + () => { + asserted = true; + }, + (records) => { + cleanedRecords = records; + }, + ), + ); + + expect(res.ok).toBe(false); + expect(res.phase).toBe("execute"); + expect(res.message).toContain("executor reported status='failed'"); + expect(asserted).toBe(false); + expect(cleanedRecords?.map((record) => record.name)).toEqual([ + "create_github_issue", + "set_github_issue_type", + ]); + expect(cleanedRecords?.[0]?.status).toBe("succeeded"); + expect(cleanedRecords?.[1]?.status).toBe("failed"); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); + it("fails in the execute phase when a prior entry produced no record", async () => { const dir = await mkdtemp(join(tmpdir(), "ado-aw-runner-prior-missing-")); try { diff --git a/scripts/ado-script/src/executor-e2e/ado-rest.ts b/scripts/ado-script/src/executor-e2e/ado-rest.ts index c7da667e..d4f4e699 100644 --- a/scripts/ado-script/src/executor-e2e/ado-rest.ts +++ b/scripts/ado-script/src/executor-e2e/ado-rest.ts @@ -32,6 +32,20 @@ interface RequestOptions { headers?: Record; } +function asciiEqualsIgnoreCase(left: string, right: string): boolean { + if (left.length !== right.length) return false; + for (let i = 0; i < left.length; i += 1) { + const leftCode = left.charCodeAt(i); + const rightCode = right.charCodeAt(i); + const foldedLeft = + leftCode >= 65 && leftCode <= 90 ? leftCode + 32 : leftCode; + const foldedRight = + rightCode >= 65 && rightCode <= 90 ? rightCode + 32 : rightCode; + if (foldedLeft !== foldedRight) return false; + } + return true; +} + export class AdoRest { private readonly base: string; private readonly project: string; @@ -114,6 +128,72 @@ export class AdoRest { return this.base; } + /** + * Resolve an identity using the same exact-match fields as update-pr's + * production add-reviewers implementation. GUIDs are already canonical ADO + * identity IDs and do not require a network lookup. + */ + async resolveIdentityId(identity: string): Promise { + const value = identity.trim(); + if ( + /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i.test( + value, + ) + ) { + return value; + } + + const vsspsBase = this.base.replace( + "://dev.azure.com/", + "://vssps.dev.azure.com/", + ); + if (vsspsBase === this.base) { + throw new Error( + `cannot derive VSSPS identity endpoint from org URL '${this.base}'`, + ); + } + const query = new URLSearchParams({ + searchFilter: "General", + filterValue: value, + "api-version": "7.1", + }); + const res = await this.request<{ + value?: Array<{ + id?: string; + providerDisplayName?: string; + customDisplayName?: string; + displayName?: string; + properties?: Record; + }>; + }>(`${vsspsBase}/_apis/identities?${query.toString()}`); + + const matchingIds = new Set( + (res?.value ?? []) + .filter((candidate) => { + const directMatch = [ + candidate.providerDisplayName, + candidate.customDisplayName, + candidate.displayName, + ].some( + (field) => + typeof field === "string" && + asciiEqualsIgnoreCase(field, value), + ); + const propertyMatch = ["Account", "Mail"].some((field) => { + const propertyValue = candidate.properties?.[field]?.$value; + return ( + typeof propertyValue === "string" && + asciiEqualsIgnoreCase(propertyValue, value) + ); + }); + return directMatch || propertyMatch; + }) + .map((candidate) => candidate.id) + .filter((id): id is string => typeof id === "string" && id.length > 0), + ); + return matchingIds.size === 1 ? matchingIds.values().next().value : undefined; + } + // ---- Work items ------------------------------------------------------- async createWorkItem( diff --git a/scripts/ado-script/src/executor-e2e/runner.ts b/scripts/ado-script/src/executor-e2e/runner.ts index 725415ab..1721a6e9 100644 --- a/scripts/ado-script/src/executor-e2e/runner.ts +++ b/scripts/ado-script/src/executor-e2e/runner.ts @@ -12,7 +12,12 @@ import { join } from "node:path"; import { runExecute } from "./execute-cli.js"; import { SkipError } from "./scenario.js"; -import type { Scenario, ScenarioContext, ScenarioResult } from "./scenario.js"; +import type { + ExecutedRecord, + Scenario, + ScenarioContext, + ScenarioResult, +} from "./scenario.js"; function errMessage(err: unknown): string { return err instanceof Error ? err.message : String(err); @@ -29,6 +34,7 @@ export async function runScenario( let state: S | undefined; let setupDone = false; + let executedRecords: ExecutedRecord[] | undefined; const finish = (partial: Omit): ScenarioResult => ({ tool: scenarioId, @@ -97,6 +103,7 @@ export async function runScenario( extraEnv, log: ctx.log, }); + executedRecords = result.records; } catch (err) { // e.g. the ado-aw execute child timed out or failed to spawn. return finish({ ok: false, phase: "execute", message: errMessage(err) }); @@ -170,7 +177,7 @@ export async function runScenario( // a successful setup (SkipError or setup failure) leave setupDone false. if (setupDone) { try { - await scenario.cleanup(ctx, state as S); + await scenario.cleanup(ctx, state as S, executedRecords); ctx.log(`[${scenarioId}] cleanup done`); } catch (err) { ctx.log(`[${scenarioId}] cleanup WARNING: ${errMessage(err)}`); diff --git a/scripts/ado-script/src/executor-e2e/scenario.ts b/scripts/ado-script/src/executor-e2e/scenario.ts index be8b3315..1f3eb758 100644 --- a/scripts/ado-script/src/executor-e2e/scenario.ts +++ b/scripts/ado-script/src/executor-e2e/scenario.ts @@ -186,8 +186,17 @@ export interface Scenario { record: ExecutedRecord, records: ExecutedRecord[], ): Promise; - /** Best-effort teardown of everything setup/execute created. */ - cleanup(ctx: ScenarioContext, state: State): Promise; + /** + * Best-effort teardown of everything setup/execute created. + * + * `records` is present whenever `runExecute()` returned, including when the + * primary tool reported failure before `assert()` could populate `state`. + */ + cleanup( + ctx: ScenarioContext, + state: State, + records?: ExecutedRecord[], + ): Promise; } /** Outcome of running one scenario. */ diff --git a/scripts/ado-script/src/executor-e2e/scenarios/create-pull-request.ts b/scripts/ado-script/src/executor-e2e/scenarios/create-pull-request.ts index 16fafb4d..12302714 100644 --- a/scripts/ado-script/src/executor-e2e/scenarios/create-pull-request.ts +++ b/scripts/ado-script/src/executor-e2e/scenarios/create-pull-request.ts @@ -26,9 +26,15 @@ import { createHash } from "node:crypto"; import { mkdir, rm, writeFile } from "node:fs/promises"; import { join } from "node:path"; -import type { Scenario, ScenarioContext } from "../scenario.js"; +import type { + ExecutedRecord, + PriorEntry, + Scenario, + ScenarioContext, +} from "../scenario.js"; +import { SkipError } from "../scenario.js"; import { partialOutput } from "../execute-cli.js"; -import { detBody, numResult, Teardown } from "./common.js"; +import { detBody, numResult, strResult, Teardown } from "./common.js"; import { crossOrgSource, resolveCrossOrgEnv, @@ -62,6 +68,27 @@ interface CreatePrScenarioOptions { readonly changedFileSuffix?: string; } +const CREATE_PR_TEMPORARY_ID = "#aw_prcreate"; +const HANDOFF_TEMPORARY_ID = "#aw_prhandoff"; +const REVIEWERS_TEMPORARY_ID = "#aw_prreviewers"; + +interface AddReviewersState extends CreatePrState { + reviewer: string; + reviewerId: string; +} + +export function resolveExecutorE2eReviewer( + env: NodeJS.ProcessEnv = process.env, +): string { + const reviewer = env.EXECUTOR_E2E_REVIEWER?.trim(); + if (!reviewer || /^\$\([^)]+\)$/.test(reviewer)) { + throw new SkipError( + "create-pull-request-add-reviewers: EXECUTOR_E2E_REVIEWER is unavailable; run from Azure Pipelines with Build.RequestedForEmail", + ); + } + return reviewer; +} + function runGit( args: string[], cwd: string, @@ -270,6 +297,7 @@ function createPullRequestScenario( patch_file: state.patchRelPath, repository: state.repositorySelector, agent_labels: [], + temporary_id: CREATE_PR_TEMPORARY_ID, base_commit: state.baseCommit, patch_sha256: state.patchSha256, }), @@ -325,8 +353,265 @@ export const createPullRequestCrossOrg = createPullRequestScenario({ changedFileSuffix: "-cross-org", }); +function executedRecordForTool( + records: ExecutedRecord[], + tool: string, +): ExecutedRecord { + const recordName = tool.replaceAll("-", "_"); + const record = records.find((candidate) => candidate.name === recordName); + if (!record) { + throw new Error(`no executed record found for prior tool '${tool}'`); + } + return record; +} + +function stringArrayResult(record: ExecutedRecord, key: string): string[] { + const value = record.result?.[key]; + if ( + !Array.isArray(value) || + !value.every((entry): entry is string => typeof entry === "string") + ) { + throw new Error( + `executor result.${key} is not a string array (got ${JSON.stringify(value)})`, + ); + } + return value; +} + +async function cleanupTemporaryPrHandoff( + state: CreatePrState, + records?: ExecutedRecord[], +): Promise { + const teardown = new Teardown(); + if (state.prId !== undefined) { + const prId = state.prId; + teardown.add("abandon PR", () => + state.rest.abandonPullRequest(state.repo, prId), + ); + } else if (records !== undefined) { + const created = records.find( + (record) => + record.name === "create_pull_request" && record.status === "succeeded", + ); + if (created !== undefined) { + teardown.add("recover and abandon PR", async () => { + const prId = numResult(created, "pull_request_id"); + await state.rest.abandonPullRequest(state.repo, prId); + }); + } + } + await teardown + .add("delete source branch", () => + state.rest.deleteRef(state.repo, `refs/heads/${state.sourceBranch}`), + ) + .add("remove local checkout", () => + rm(state.sourcesDir, { recursive: true, force: true }), + ) + .run(); +} + +/** + * Runs create-pull-request and update-pr in one executor process. This is the + * production handoff shape: the create result registers the real PR under a + * temporary ID, then the following update resolves that ID without the model + * ever knowing Azure DevOps' numeric PR ID. + */ +export const createPullRequestTemporaryIdHandoff: Scenario = { + id: "create-pull-request-temporary-id-handoff", + tool: "update-pr", + targetsAdoRepo: true, + setup: (ctx) => + setupCreatePullRequest(ctx, { + id: "create-pull-request-temporary-id-handoff", + repositorySelector: "named", + patchRelPath: "create-pr-temporary-id-handoff.patch", + changedFileSuffix: "-temporary-id-handoff", + }), + config: (_ctx, state) => ({ + "allowed-operations": ["update-description"], + "allowed-repositories": [state.repo], + max: 1, + }), + priorEntries: async (ctx, state): Promise => [ + { + tool: "create-pull-request", + config: { + "target-branch": state.targetBranch, + "allowed-repositories": [state.repo], + "delete-source-branch": true, + "if-no-changes": "error", + "include-stats": false, + }, + entry: { + title: `${ctx.prefix("create-pull-request-temporary-id-handoff")} (do not merge)`, + description: detBody(ctx, "create-pull-request-temporary-id-handoff"), + source_branch: state.sourceBranch, + patch_file: state.patchRelPath, + repository: state.repositorySelector, + agent_labels: [], + temporary_id: HANDOFF_TEMPORARY_ID, + base_commit: state.baseCommit, + patch_sha256: state.patchSha256, + }, + }, + ], + files: async (_ctx, state) => ({ [state.patchRelPath]: state.patchContent }), + env: async (_ctx, state) => ({ + BUILD_SOURCESDIRECTORY: state.sourcesDir, + }), + ndjson: async (ctx) => ({ + pull_request_id: HANDOFF_TEMPORARY_ID, + operation: "update-description", + description: `${detBody(ctx, "create-pull-request-temporary-id-handoff")} Updated through temporary ID.`, + }), + assert: async (ctx, state, record, records) => { + const created = executedRecordForTool(records, "create-pull-request"); + const createdPrId = numResult(created, "pull_request_id"); + state.prId = createdPrId; + + if (strResult(created, "temporary_id") !== HANDOFF_TEMPORARY_ID) { + throw new Error( + `create-pull-request reported temporary_id '${strResult(created, "temporary_id")}', expected '${HANDOFF_TEMPORARY_ID}'`, + ); + } + const updatedPrId = numResult(record, "pull_request_id"); + if (updatedPrId !== createdPrId) { + throw new Error( + `temporary_id '${HANDOFF_TEMPORARY_ID}' resolved to PR #${updatedPrId}, but create-pull-request filed #${createdPrId}`, + ); + } + + const expectedDescription = + `${detBody(ctx, "create-pull-request-temporary-id-handoff")} Updated through temporary ID.`; + const pr = await state.rest.getPullRequest(state.repo, createdPrId); + if (pr.description !== expectedDescription) { + throw new Error( + `PR #${createdPrId} description was not updated through temporary ID`, + ); + } + }, + cleanup: async (_ctx, state, records) => + cleanupTemporaryPrHandoff(state, records), +}; + +/** + * Creates a PR and adds the pipeline requester as its reviewer in one + * executor invocation, proving temporary-ID handoff and live reviewer state. + */ +export const createPullRequestAddReviewers: Scenario = { + id: "create-pull-request-add-reviewers", + tool: "update-pr", + targetsAdoRepo: true, + setup: async (ctx) => { + const reviewer = resolveExecutorE2eReviewer(); + const reviewerId = await ctx.rest.resolveIdentityId(reviewer); + if (!reviewerId) { + throw new SkipError( + `create-pull-request-add-reviewers: reviewer '${reviewer}' did not resolve to exactly one ADO identity`, + ); + } + const state = await setupCreatePullRequest(ctx, { + id: "create-pull-request-add-reviewers", + repositorySelector: "named", + patchRelPath: "create-pr-add-reviewers.patch", + changedFileSuffix: "-add-reviewers", + }); + return { ...state, reviewer, reviewerId }; + }, + config: (_ctx, state) => ({ + "allowed-operations": ["add-reviewers"], + "allowed-repositories": [state.repo], + "allowed-reviewers": [state.reviewer], + "max-reviewers": 1, + max: 1, + }), + priorEntries: async (ctx, state): Promise => [ + { + tool: "create-pull-request", + config: { + "target-branch": state.targetBranch, + "allowed-repositories": [state.repo], + "delete-source-branch": true, + "if-no-changes": "error", + "include-stats": false, + }, + entry: { + title: `${ctx.prefix("create-pull-request-add-reviewers")} (do not merge)`, + description: detBody(ctx, "create-pull-request-add-reviewers"), + source_branch: state.sourceBranch, + patch_file: state.patchRelPath, + repository: state.repositorySelector, + agent_labels: [], + temporary_id: REVIEWERS_TEMPORARY_ID, + base_commit: state.baseCommit, + patch_sha256: state.patchSha256, + }, + }, + ], + files: async (_ctx, state) => ({ [state.patchRelPath]: state.patchContent }), + env: async (_ctx, state) => ({ + BUILD_SOURCESDIRECTORY: state.sourcesDir, + }), + ndjson: async (_ctx, state) => ({ + pull_request_id: REVIEWERS_TEMPORARY_ID, + operation: "add-reviewers", + reviewers: [state.reviewer], + }), + assert: async (_ctx, state, record, records) => { + const created = executedRecordForTool(records, "create-pull-request"); + const createdPrId = numResult(created, "pull_request_id"); + state.prId = createdPrId; + if (strResult(created, "temporary_id") !== REVIEWERS_TEMPORARY_ID) { + throw new Error( + `create-pull-request reported the wrong temporary_id for reviewer handoff`, + ); + } + + const updatedPrId = numResult(record, "pull_request_id"); + if (updatedPrId !== createdPrId) { + throw new Error( + `temporary_id '${REVIEWERS_TEMPORARY_ID}' resolved to PR #${updatedPrId}, but create-pull-request filed #${createdPrId}`, + ); + } + if (strResult(record, "operation") !== "add-reviewers") { + throw new Error("update-pr reported an unexpected operation"); + } + const failed = stringArrayResult(record, "failed"); + if (failed.length !== 0) { + throw new Error(`add-reviewers reported failures: ${failed.join(", ")}`); + } + const added = stringArrayResult(record, "added"); + if ( + !added.some( + (reviewer) => + reviewer.toLowerCase() === state.reviewer.toLowerCase(), + ) + ) { + throw new Error( + `add-reviewers result did not include configured identity '${state.reviewer}'`, + ); + } + + const reviewers = await state.rest.listReviewers(state.repo, createdPrId); + if ( + !reviewers.some( + (reviewer) => + reviewer.id.toLowerCase() === state.reviewerId.toLowerCase(), + ) + ) { + throw new Error( + `PR #${createdPrId} does not contain reviewer identity '${state.reviewerId}'`, + ); + } + }, + cleanup: async (_ctx, state, records) => + cleanupTemporaryPrHandoff(state, records), +}; + export const createPullRequestScenarios: Scenario[] = [ createPullRequest, createPullRequestSelfMultiCheckout, createPullRequestCrossOrg, + createPullRequestTemporaryIdHandoff, + createPullRequestAddReviewers, ]; diff --git a/src/compile/agentic_pipeline.rs b/src/compile/agentic_pipeline.rs index f5e9c941..5b148ca4 100644 --- a/src/compile/agentic_pipeline.rs +++ b/src/compile/agentic_pipeline.rs @@ -183,6 +183,7 @@ fn validate_pipeline_front_matter( common::validate_comment_target(front_matter)?; common::validate_update_work_item_target(front_matter)?; common::validate_submit_pr_review_events(front_matter)?; + common::validate_pull_request_outputs_config(front_matter)?; common::validate_update_pr_votes(front_matter)?; common::validate_resolve_pr_thread_statuses(front_matter)?; common::validate_ado_aw_debug_config(front_matter)?; diff --git a/src/compile/common.rs b/src/compile/common.rs index be9fec4b..30c3c2de 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -2352,6 +2352,11 @@ fn require_same_approval_lane( effective require-approval setting so temporary work-item IDs remain in one \ SafeOutputs job" ), + "create-pull-request" => anyhow::bail!( + "safe-outputs.create-pull-request and safe-outputs.{consumer} must have the same \ + effective require-approval setting so temporary pull-request IDs remain in one \ + SafeOutputs job" + ), _ => anyhow::bail!( "safe-outputs.{producer} and safe-outputs.{consumer} must have the same effective \ require-approval setting" @@ -2969,6 +2974,31 @@ pub fn validate_submit_pr_review_events(front_matter: &FrontMatter) -> Result<() Ok(()) } +/// Validate configuration shared by create-pull-request and update-pr. +pub fn validate_pull_request_outputs_config(front_matter: &FrontMatter) -> Result<()> { + if front_matter + .safe_outputs + .contains_key("create-pull-request") + && front_matter.safe_outputs.contains_key("update-pr") + { + require_same_approval_lane(front_matter, "create-pull-request", "update-pr")?; + } + + if let Some(config) = front_matter.safe_outputs.get("update-pr") + && let Some(max_reviewers) = config + .as_object() + .and_then(|object| object.get("max-reviewers")) + .and_then(serde_json::Value::as_u64) + { + anyhow::ensure!( + max_reviewers > 0, + "safe-outputs.update-pr.max-reviewers must be greater than zero" + ); + } + + Ok(()) +} + /// Validate that update-pr has a required `allowed-votes` field when the `vote` operation /// is enabled (i.e., `allowed-operations` is empty — meaning all ops — or explicitly contains /// "vote"). @@ -6118,6 +6148,108 @@ safe-outputs: assert!(error.contains("same effective require-approval")); } + #[test] + fn test_validate_rejects_mixed_approval_lanes_for_pull_request_tools() { + for (create_approval, update_approval) in [(true, false), (false, true)] { + let yaml = format!( + r#"--- +name: test +description: test +safe-outputs: + create-pull-request: + require-approval: {create_approval} + update-pr: + require-approval: {update_approval} + allowed-operations: + - update-description +--- +"# + ); + let (fm, _) = parse_markdown(&yaml).unwrap(); + let error = validate_pull_request_outputs_config(&fm) + .unwrap_err() + .to_string(); + assert!( + error.contains("temporary pull-request IDs") + && error.contains("same effective require-approval"), + "error: {error}" + ); + } + } + + #[test] + fn test_validate_accepts_matching_pull_request_approval_lanes() { + for approval in [true, false] { + let yaml = format!( + r#"--- +name: test +description: test +safe-outputs: + create-pull-request: + require-approval: {approval} + update-pr: + require-approval: {approval} + allowed-operations: + - update-description +--- +"# + ); + let (fm, _) = parse_markdown(&yaml).unwrap(); + assert!(validate_pull_request_outputs_config(&fm).is_ok()); + } + } + + #[test] + fn test_validate_pull_request_approval_lane_uses_effective_section_default() { + let yaml = r#"--- +name: test +description: test +safe-outputs: + require-approval: true + create-pull-request: {} + update-pr: + allowed-operations: + - update-description +--- +"#; + let (fm, _) = parse_markdown(yaml).unwrap(); + assert!(validate_pull_request_outputs_config(&fm).is_ok()); + } + + #[test] + fn test_validate_rejects_zero_max_reviewers() { + let yaml = r#"--- +name: test +description: test +safe-outputs: + update-pr: + allowed-operations: + - add-reviewers + max-reviewers: 0 +--- +"#; + let (fm, _) = parse_markdown(yaml).unwrap(); + let error = validate_pull_request_outputs_config(&fm) + .unwrap_err() + .to_string(); + assert!(error.contains("max-reviewers must be greater than zero")); + } + + #[test] + fn test_validate_allows_omitted_reviewer_allowlist() { + let yaml = r#"--- +name: test +description: test +safe-outputs: + update-pr: + allowed-operations: + - add-reviewers +--- +"#; + let (fm, _) = parse_markdown(yaml).unwrap(); + assert!(validate_pull_request_outputs_config(&fm).is_ok()); + } + #[test] fn test_validate_rejects_mixed_approval_lanes_for_create_and_comment_work_item() { let yaml = r#"--- diff --git a/src/execute.rs b/src/execute.rs index 7740caa5..7f34b850 100644 --- a/src/execute.rs +++ b/src/execute.rs @@ -1353,6 +1353,149 @@ mod tests { assert_eq!(manifest[1]["status"], "succeeded"); } + #[tokio::test] + async fn test_execute_safe_outputs_creates_then_updates_temporary_pr_reference() { + use std::process::Command; + use wiremock::matchers::{method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + let server = MockServer::start().await; + let api_base = "/Target%20Project/_apis/git/repositories/repo-id"; + Mock::given(method("GET")) + .and(path(format!("{api_base}/refs"))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "value": [] + }))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path(format!("{api_base}/pushes"))) + .respond_with(ResponseTemplate::new(201).set_body_json(serde_json::json!({ + "pushId": 1 + }))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path(format!("{api_base}/pullrequests"))) + .respond_with(ResponseTemplate::new(201).set_body_json(serde_json::json!({ + "pullRequestId": 42, + "url": "https://example.test/pr/42", + "createdBy": {"id": "creator-id"} + }))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("PATCH")) + .and(path(format!("{api_base}/pullRequests/42"))) + .respond_with(ResponseTemplate::new(200)) + .expect(1) + .mount(&server) + .await; + + let temp_dir = tempfile::tempdir().unwrap(); + let repo_dir = temp_dir.path().join("repo"); + let safe_outputs_dir = temp_dir.path().join("safe-outputs"); + std::fs::create_dir_all(&repo_dir).unwrap(); + std::fs::create_dir_all(&safe_outputs_dir).unwrap(); + let run_git = |args: &[&str]| { + let output = Command::new("git") + .args(args) + .current_dir(&repo_dir) + .output() + .expect("git command should run"); + assert!( + output.status.success(), + "git {:?} failed: {}", + args, + String::from_utf8_lossy(&output.stderr) + ); + output + }; + run_git(&["init", "-b", "main"]); + run_git(&["config", "user.email", "test@example.com"]); + run_git(&["config", "user.name", "Test User"]); + std::fs::write(repo_dir.join("file.txt"), "before\n").unwrap(); + run_git(&["add", "file.txt"]); + run_git(&["commit", "-m", "initial"]); + std::fs::write(repo_dir.join("file.txt"), "after\n").unwrap(); + run_git(&["add", "file.txt"]); + run_git(&["commit", "-m", "update file"]); + let patch = run_git(&["format-patch", "HEAD~1", "--stdout"]).stdout; + run_git(&["reset", "--hard", "HEAD~1"]); + run_git(&["update-ref", "refs/remotes/origin/main", "HEAD"]); + let base_commit = String::from_utf8(run_git(&["rev-parse", "HEAD"]).stdout) + .unwrap() + .trim() + .to_string(); + let patch_file = safe_outputs_dir.join("change.patch"); + std::fs::write(&patch_file, &patch).unwrap(); + let patch_sha256 = crate::hash::sha256_hex(&patch); + + let create = serde_json::json!({ + "name": "create-pull-request", + "title": "Update test file", + "description": "Update the test file before following up.", + "source_branch": "agent/update-test-file-abc123", + "patch_file": "change.patch", + "repository": "self", + "agent_labels": [], + "temporary_id": "#aw_pr123", + "base_commit": base_commit, + "patch_sha256": patch_sha256 + }); + let update = serde_json::json!({ + "name": "update-pr", + "pull_request_id": "#aw_pr123", + "operation": "update-description", + "description": "Updated through the temporary reference." + }); + let ndjson = format!( + "{}\n{}\n", + serde_json::to_string(&create).unwrap(), + serde_json::to_string(&update).unwrap() + ); + tokio::fs::write(safe_outputs_dir.join(SAFE_OUTPUT_FILENAME), ndjson) + .await + .unwrap(); + + let mut tool_configs = HashMap::new(); + tool_configs.insert( + "create-pull-request".to_string(), + serde_json::json!({"max": 1, "include-stats": false}), + ); + tool_configs.insert("update-pr".to_string(), serde_json::json!({"max": 1})); + let ctx = ExecutionContext { + ado_org_url: Some(server.uri()), + ado_organization: Some("target-org".to_string()), + ado_project: Some("Target Project".to_string()), + access_token: Some("test-token".to_string()), + working_directory: safe_outputs_dir.clone(), + source_directory: repo_dir.clone(), + self_repository_directory: repo_dir, + repository_id: Some("repo-id".to_string()), + repository_name: Some("target-repo".to_string()), + repository_provider: Some("TfsGit".to_string()), + tool_configs, + ..Default::default() + }; + + let results = execute_safe_outputs(&safe_outputs_dir, &ctx, &ToolFilter::default()) + .await + .unwrap(); + assert_eq!(results.len(), 2); + assert!(results[0].success, "{}", results[0].message); + assert_eq!( + results[0].data.as_ref().unwrap()["temporary_id"], + "#aw_pr123" + ); + assert_eq!(results[0].data.as_ref().unwrap()["pull_request_id"], 42); + assert!(results[1].success, "{}", results[1].message); + assert_eq!(results[1].data.as_ref().unwrap()["pull_request_id"], 42); + server.verify().await; + } + #[tokio::test] async fn test_execute_safe_outputs_empty_file_returns_empty() { let temp_dir = tempfile::tempdir().unwrap(); diff --git a/src/mcp.rs b/src/mcp.rs index 17af63e4..358f50f0 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -25,18 +25,18 @@ use crate::safe_outputs::{ MissingToolParams, MissingToolResult, NoopParams, NoopResult, PIPELINE_ARTIFACT_DEFAULT_MAX_FILE_SIZE, QueueBuildParams, QueueBuildResult, RemoveGithubIssueLabelsParams, RemoveGithubIssueLabelsResult, ReplyToPrCommentParams, - ReplyToPrCommentResult, ReportIncompleteParams, ReportIncompleteResult, - ResolvePrThreadParams, ResolvePrThreadResult, SetGithubIssueFieldParams, - SetGithubIssueFieldResult, SetGithubIssueTypeParams, SetGithubIssueTypeResult, - SubmitPrReviewParams, SubmitPrReviewResult, ToolResult, UnassignGithubIssueFromUserParams, - UnassignGithubIssueFromUserResult, UpdateGithubIssueParams, UpdateGithubIssueResult, - UpdatePrParams, UpdatePrResult, UpdateWikiPageParams, UpdateWikiPageResult, - UpdateWorkItemParams, UpdateWorkItemResult, UploadBuildAttachmentParams, - UploadBuildAttachmentResult, UploadPipelineArtifactParams, UploadPipelineArtifactResult, - UploadWorkitemAttachmentParams, UploadWorkitemAttachmentResult, Validate, anyhow_to_mcp_error, + ReplyToPrCommentResult, ReportIncompleteParams, ReportIncompleteResult, ResolvePrThreadParams, + ResolvePrThreadResult, SetGithubIssueFieldParams, SetGithubIssueFieldResult, + SetGithubIssueTypeParams, SetGithubIssueTypeResult, SubmitPrReviewParams, SubmitPrReviewResult, + ToolResult, UnassignGithubIssueFromUserParams, UnassignGithubIssueFromUserResult, + UpdateGithubIssueParams, UpdateGithubIssueResult, UpdatePrParams, UpdatePrResult, + UpdateWikiPageParams, UpdateWikiPageResult, UpdateWorkItemParams, UpdateWorkItemResult, + UploadBuildAttachmentParams, UploadBuildAttachmentResult, UploadPipelineArtifactParams, + UploadPipelineArtifactResult, UploadWorkitemAttachmentParams, UploadWorkitemAttachmentResult, + Validate, anyhow_to_mcp_error, }; use crate::sanitize::{SanitizeContent, sanitize as sanitize_text, sanitize_markdown}; -use crate::secure::WorkItemTemporaryId; +use crate::secure::{PullRequestTemporaryId, WorkItemTemporaryId}; /// Sanitize a title into a safe branch name slug. /// Only allows alphanumeric characters and dashes, collapses multiple dashes, @@ -220,6 +220,8 @@ pub struct SafeOutputs { custom_proposal_lock: Arc>, /// Serializes create-work-item temporary-ID allocation and proposal append. create_work_item_proposal_lock: Arc>, + /// Serializes create-pull-request temporary-ID allocation and proposal append. + create_pr_proposal_lock: Arc>, } /// Resolve which git directory to use for patch generation. @@ -537,8 +539,7 @@ impl SafeOutputs { WorkItemTemporaryId::parse(format!("#aw_{}", generate_short_id())).ok()?; let canonical = candidate.canonical(); let collision = existing.iter().any(|proposal| { - proposal.get("name").and_then(Value::as_str) - == Some(CreateWorkItemResult::NAME) + proposal.get("name").and_then(Value::as_str) == Some(CreateWorkItemResult::NAME) && proposal.get("temporary_id").and_then(Value::as_str) == Some(canonical.as_str()) }); @@ -636,6 +637,7 @@ impl SafeOutputs { tool_router, custom_proposal_lock: Arc::new(tokio::sync::Mutex::new(())), create_work_item_proposal_lock: Arc::new(tokio::sync::Mutex::new(())), + create_pr_proposal_lock: Arc::new(tokio::sync::Mutex::new(())), }) } @@ -1078,7 +1080,8 @@ and only the fields you want to update." name = "create-pull-request", description = "Create a new pull request to propose code changes. This tool captures all \ changes in the repository (both committed and uncommitted) and creates a PR from them. \ -Use 'self' for the pipeline's own repository, or a repository alias from the checkout list." +Use 'self' for the pipeline's own repository, or a repository alias from the checkout list. \ +Returns a generated temporary_id that can be passed as pull_request_id to later update-pr calls." )] async fn create_pr( &self, @@ -1131,7 +1134,31 @@ Use 'self' for the pipeline's own repository, or a repository alias from the che format!("agent/{}-{}", title_slug, short_id) }; - // Create the result with patch file reference and integrity hash + const MAX_ID_ATTEMPTS: usize = 16; + let _guard = self.create_pr_proposal_lock.lock().await; + let existing = self + .read_safe_output_file() + .await + .map_err(anyhow_to_mcp_error)?; + let temporary_id = (0..MAX_ID_ATTEMPTS) + .find_map(|_| { + let candidate = + PullRequestTemporaryId::parse(format!("#aw_{}", generate_short_id())).ok()?; + let canonical = candidate.canonical(); + let collision = existing.iter().any(|proposal| { + proposal.get("name").and_then(Value::as_str) == Some(CreatePrResult::NAME) + && proposal.get("temporary_id").and_then(Value::as_str) + == Some(canonical.as_str()) + }); + (!collision).then_some(candidate) + }) + .ok_or_else(|| { + anyhow_to_mcp_error(anyhow::anyhow!( + "Failed to allocate a unique create-pull-request temporary ID" + )) + })?; + + // Create the result with patch file reference, temporary ID, and integrity hash let result = CreatePrResult { name: CreatePrResult::NAME.to_string(), title: sanitized.title.clone(), @@ -1140,17 +1167,25 @@ Use 'self' for the pipeline's own repository, or a repository alias from the che patch_file: patch_filename, repository: repository.to_string(), agent_labels: sanitized.labels, + temporary_id: temporary_id.clone(), base_commit: Some(merge_base), patch_sha256, }; // Write to safe outputs - let _ = self.write_safe_output_file(&result).await; + self.write_safe_output_file(&result) + .await + .map_err(anyhow_to_mcp_error)?; - Ok(CallToolResult::success(vec![Content::text(format!( - "PR request saved for repository '{}'. Patch file: {}. Changes will be pushed and PR created during safe output processing.", - repository, result.patch_file - ))])) + let canonical = temporary_id.canonical(); + let mut response = CallToolResult::success(vec![Content::text(format!( + "PR request saved for repository '{}'. Patch file: {}. Use temporary ID {} as pull_request_id in later update-pr calls.", + repository, result.patch_file, canonical + ))]); + response.structured_content = Some(serde_json::json!({ + "temporary_id": canonical, + })); + Ok(response) } #[tool( @@ -1837,6 +1872,31 @@ mod tests { (safe_outputs, temp_dir) } + fn initialize_git_repo_with_change(path: &std::path::Path) { + use std::process::Command; + + let run = |args: &[&str]| { + let output = Command::new("git") + .args(args) + .current_dir(path) + .output() + .expect("git command should run"); + assert!( + output.status.success(), + "git {:?} failed: {}", + args, + String::from_utf8_lossy(&output.stderr) + ); + }; + run(&["init", "-b", "main"]); + run(&["config", "user.email", "test@example.com"]); + run(&["config", "user.name", "Test User"]); + std::fs::write(path.join("file.txt"), "before\n").unwrap(); + run(&["add", "file.txt"]); + run(&["commit", "-m", "initial"]); + std::fs::write(path.join("file.txt"), "after\n").unwrap(); + } + fn valid_create_work_item_params(suffix: &str) -> CreateWorkItemParams { CreateWorkItemParams { title: format!("Create work item {suffix}"), @@ -2046,6 +2106,30 @@ mod tests { assert_eq!(proposals[0]["temporary_id"], temporary_id); } + #[tokio::test] + async fn create_pr_returns_and_persists_generated_temporary_id() { + let (safe_outputs, temp_dir) = create_test_safe_outputs().await; + initialize_git_repo_with_change(temp_dir.path()); + + let response = safe_outputs + .create_pr(Parameters(CreatePrParams { + title: "Update test file".to_string(), + description: "Update the test file through a generated pull request.".to_string(), + repository: None, + labels: Vec::new(), + })) + .await + .unwrap(); + + let structured = response.structured_content.expect("structured response"); + let temporary_id = structured["temporary_id"].as_str().unwrap(); + assert!(PullRequestTemporaryId::parse(temporary_id).is_ok()); + let proposals = safe_outputs.read_safe_output_file().await.unwrap(); + assert_eq!(proposals.len(), 1); + assert_eq!(proposals[0]["name"], "create-pull-request"); + assert_eq!(proposals[0]["temporary_id"], temporary_id); + } + #[tokio::test] async fn create_work_item_preserves_html_description_in_proposal() { let (safe_outputs, _temp_dir) = create_test_safe_outputs().await; @@ -2592,6 +2676,24 @@ safe-outputs: assert!(!properties.contains_key("temporary_id")); } + #[tokio::test] + async fn test_create_pr_schema_excludes_internal_and_inline_reviewer_fields() { + let temp_dir = tempfile::tempdir().unwrap(); + let enabled = vec!["create-pull-request".to_string()]; + let so = SafeOutputs::new(temp_dir.path(), temp_dir.path(), Some(&enabled), None) + .await + .unwrap(); + let tools = so.tool_router.list_all(); + let tool = tools + .iter() + .find(|tool| tool.name.as_ref() == "create-pull-request") + .expect("create-pull-request should be enabled"); + let schema = serde_json::to_value(&tool.input_schema).unwrap(); + let properties = schema["properties"].as_object().unwrap(); + assert!(!properties.contains_key("temporary_id")); + assert!(!properties.contains_key("reviewers")); + } + #[tokio::test] async fn test_github_queue_propagates_ndjson_write_failures() { let temp_dir = tempfile::tempdir().unwrap(); diff --git a/src/safe_outputs/create_pull_request.rs b/src/safe_outputs/create_pull_request.rs index 7d2bb145..e1ccae1a 100644 --- a/src/safe_outputs/create_pull_request.rs +++ b/src/safe_outputs/create_pull_request.rs @@ -8,6 +8,7 @@ use tokio::process::Command; use crate::safe_outputs::{ExecutionContext, ExecutionResult, Executor, PATH_SEGMENT, Validate}; use crate::sanitize::{SanitizeContent, sanitize as sanitize_text, sanitize_config}; +use crate::secure::PullRequestTemporaryId; use crate::tool_result; use crate::validate::reject_pipeline_injection; use ado_aw_derive::SanitizeConfig; @@ -231,6 +232,7 @@ fn identity_picker_url(organization: &str) -> String { /// Parameters for creating a pull request #[derive(Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct CreatePrParams { /// Title for the pull request; should be concise and descriptive pub title: String, @@ -284,6 +286,7 @@ struct CreatePrResultFields { repository: String, #[serde(default)] agent_labels: Vec, + temporary_id: PullRequestTemporaryId, #[serde(skip_serializing_if = "Option::is_none")] base_commit: Option, /// SHA-256 hex digest of the patch file, recorded at staging time. @@ -311,6 +314,8 @@ tool_result! { /// Agent-provided labels (validated against allowed-labels at execution time) #[serde(default)] agent_labels: Vec, + /// Temporary identifier for later safe outputs in the same run + temporary_id: PullRequestTemporaryId, /// Base commit SHA recorded at patch generation time (merge-base of HEAD and /// the upstream branch). When present, Stage 3 uses this as the parent commit /// for the ADO Push API, ensuring the patch applies cleanly even if the target @@ -695,6 +700,12 @@ impl Executor for CreatePrResult { Err(failure) => return Ok(failure), }; debug!("Resolved repository ID: {}", target.repository_locator()); + if ctx.has_resolved_pull_request(&self.temporary_id)? { + return Ok(ExecutionResult::failure(format!( + "temporary_id '{}' was already used in this run", + self.temporary_id.canonical() + ))); + } let resolved_target_branch = config.resolve_target_branch(&repository_alias, &ctx.repo_refs); @@ -1282,7 +1293,7 @@ impl Executor for CreatePrResult { } let pr_data: serde_json::Value = pr_response.json().await?; - let pr_id = pr_data["pullRequestId"].as_i64().unwrap_or(0); + let pr_id = pr_data["pullRequestId"].as_u64().unwrap_or(0); let pr_web_url = pr_data["url"].as_str().unwrap_or(""); info!("Pull request created: #{} - {}", pr_id, pr_web_url); @@ -1295,7 +1306,36 @@ impl Executor for CreatePrResult { pr_id, token, connection_type: ctx.write_connection_type, + reviewers: &config.reviewers, }; + if pr_id == 0 { + return Ok(ExecutionResult::failure( + "Azure DevOps create-pull-request response contained no positive pull request ID", + )); + } + if let Err(error) = ctx.register_resolved_pull_request( + &self.temporary_id, + crate::safe_outputs::ResolvedPullRequest { + id: pr_id, + url: pr_web_url.to_string(), + target: target.clone(), + }, + ) { + return Ok(ExecutionResult::failure_with_data( + format!( + "Created pull request #{} but failed to register temporary_id '{}': {}", + pr_id, + self.temporary_id.canonical(), + crate::sanitize::neutralize_pipeline_commands(&error.to_string()) + ), + serde_json::json!({ + "pull_request_id": pr_id, + "url": pr_web_url, + "temporary_id": self.temporary_id.canonical(), + "repository": target.display_name(), + }), + )); + } set_pr_completion_options(&pr_ctx, pr_data["createdBy"]["id"].as_str()).await; add_reviewers_to_pr(&pr_ctx).await; @@ -1314,7 +1354,8 @@ impl Executor for CreatePrResult { "url": pr_web_url, "source_branch": source_branch, "target_branch": target_branch, - "draft": config.draft + "draft": config.draft, + "temporary_id": self.temporary_id.canonical(), }), )) } @@ -1711,9 +1752,10 @@ struct PrContext<'a> { client: &'a reqwest::Client, config: &'a CreatePrConfig, target: &'a crate::safe_outputs::result::AdoRepositoryTarget, - pr_id: i64, + pr_id: u64, token: &'a str, connection_type: Option, + reviewers: &'a [String], } /// Set PR completion options (delete-source-branch, squash-merge) and optionally @@ -1776,11 +1818,11 @@ async fn set_pr_completion_options(ctx: &PrContext<'_>, pr_created_by_id: Option /// issues a `PUT` for each one. Logs a warning if a reviewer cannot be resolved or /// if the API call fails; does not abort the overall PR creation. async fn add_reviewers_to_pr(ctx: &PrContext<'_>) { - if ctx.config.reviewers.is_empty() { + if ctx.reviewers.is_empty() { return; } - debug!("Adding {} reviewers", ctx.config.reviewers.len()); - for reviewer in &ctx.config.reviewers { + debug!("Adding {} reviewers", ctx.reviewers.len()); + for reviewer in ctx.reviewers { debug!("Adding reviewer: {}", reviewer); // Resolve reviewer identity (email/name -> ID) @@ -2492,6 +2534,26 @@ mod tests { assert!(params.validate().is_err()); } + #[test] + fn test_params_reject_internal_and_inline_reviewer_fields() { + assert!( + serde_json::from_value::(serde_json::json!({ + "title": "Valid title", + "description": "A sufficiently long description.", + "temporary_id": "#aw_pr123" + })) + .is_err() + ); + assert!( + serde_json::from_value::(serde_json::json!({ + "title": "Valid title", + "description": "A sufficiently long description.", + "reviewers": ["owner@example.com"] + })) + .is_err() + ); + } + #[test] fn test_validate_params_rejects_repository_pipeline_command() { let params = CreatePrParams { @@ -2513,6 +2575,7 @@ mod tests { patch_file: "/tmp/test.patch".to_string(), repository: "##vso[task.setvariable variable=x]y".to_string(), agent_labels: vec![], + temporary_id: PullRequestTemporaryId::parse("#aw_test1").unwrap(), base_commit: None, patch_sha256: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" .to_string(), @@ -2689,6 +2752,7 @@ mod tests { patch_file: "patch.diff".to_string(), repository: "cross-org-repo".to_string(), agent_labels: vec![], + temporary_id: PullRequestTemporaryId::parse("#aw_test1").unwrap(), base_commit: None, patch_sha256: "deadbeef".to_string(), }; @@ -2720,6 +2784,7 @@ mod tests { patch_file: "patch.diff".to_string(), repository: "cross-org-repo".to_string(), agent_labels: vec![], + temporary_id: PullRequestTemporaryId::parse("#aw_test2").unwrap(), base_commit: None, patch_sha256: "deadbeef".to_string(), }; @@ -2746,6 +2811,7 @@ mod tests { patch_file: "patch.diff".to_string(), repository: "cross-org-repo".to_string(), agent_labels: vec!["unapproved".to_string()], + temporary_id: PullRequestTemporaryId::parse("#aw_test3").unwrap(), base_commit: None, patch_sha256: "deadbeef".to_string(), }; @@ -2768,6 +2834,7 @@ mod tests { patch_file: "patch.diff".to_string(), repository: "not-checked-out".to_string(), agent_labels: vec![], + temporary_id: PullRequestTemporaryId::parse("#aw_test4").unwrap(), base_commit: None, patch_sha256: "deadbeef".to_string(), }; @@ -2816,6 +2883,7 @@ mod tests { patch_file: "patch.diff".to_string(), repository: "cross-org-repo".to_string(), agent_labels: vec![], + temporary_id: PullRequestTemporaryId::parse("#aw_test5").unwrap(), base_commit: None, patch_sha256: "deadbeef".to_string(), }; @@ -3404,6 +3472,7 @@ index 0000000..abcdefg patch_file: patch_file.to_string(), repository: "self".to_string(), agent_labels: vec![], + temporary_id: PullRequestTemporaryId::parse("#aw_hash1").unwrap(), base_commit: None, patch_sha256: wrong_hash, }; @@ -3446,6 +3515,9 @@ index 0000000..abcdefg resolved_work_items: std::sync::Arc::new(std::sync::Mutex::new( std::collections::HashMap::new(), )), + resolved_pull_requests: std::sync::Arc::new(std::sync::Mutex::new( + std::collections::HashMap::new(), + )), triggered_by_build_id: None, triggered_by_definition_name: None, triggered_by_build_number: None, diff --git a/src/safe_outputs/mod.rs b/src/safe_outputs/mod.rs index bd86a508..2dc4a49d 100644 --- a/src/safe_outputs/mod.rs +++ b/src/safe_outputs/mod.rs @@ -435,19 +435,21 @@ fn split_repository_target_name( pub(crate) fn resolve_repository_write_target( repository: Option<&str>, ctx: &ExecutionContext, -) -> Result { +) -> Result { let selector = repository.unwrap_or("self"); let Some(alias) = canonical_repository_alias(selector, ctx) else { return Err(ExecutionResult::failure(format!( "Repository '{selector}' is not in the allowed repository list" ))); }; - let current_org_url = ctx.ado_org_url.as_deref().ok_or_else(|| { - ExecutionResult::failure("Azure DevOps organization URL not configured") - })?; - let current_organization = ctx.ado_organization.as_deref().ok_or_else(|| { - ExecutionResult::failure("Azure DevOps organization name not configured") - })?; + let current_org_url = ctx + .ado_org_url + .as_deref() + .ok_or_else(|| ExecutionResult::failure("Azure DevOps organization URL not configured"))?; + let current_organization = ctx + .ado_organization + .as_deref() + .ok_or_else(|| ExecutionResult::failure("Azure DevOps organization name not configured"))?; let current_project = ctx .ado_project .as_deref() @@ -459,7 +461,7 @@ pub(crate) fn resolve_repository_write_target( .as_deref() .ok_or_else(|| ExecutionResult::failure("BUILD_REPOSITORY_NAME not set"))?; let (_, repository) = split_repository_target_name(name, current_project)?; - return Ok(crate::safe_outputs::result::AdoRepositoryTarget { + return Ok(AdoRepositoryTarget { alias, organization: current_organization.to_string(), organization_url: current_org_url.trim_end_matches('/').to_string(), @@ -507,8 +509,7 @@ pub(crate) fn resolve_repository_write_target( ))); } - let (project, repository_name) = - split_repository_target_name(&config.name, current_project)?; + let (project, repository_name) = split_repository_target_name(&config.name, current_project)?; let organization = config .organization .as_deref() @@ -534,7 +535,7 @@ pub(crate) fn resolve_repository_write_target( } } - Ok(crate::safe_outputs::result::AdoRepositoryTarget { + Ok(AdoRepositoryTarget { alias, organization: organization.to_string(), organization_url: if cross_organization { @@ -765,9 +766,9 @@ macro_rules! impl_temporary_reference_deserialize { mod add_build_tag; mod add_github_issue_labels; mod add_pr_comment; -mod assign_work_item; mod assign_github_issue_milestone; mod assign_github_issue_to_user; +mod assign_work_item; mod close_github_issue; mod comment_on_github_issue; mod comment_on_work_item; @@ -806,9 +807,9 @@ mod upload_workitem_attachment; pub use add_build_tag::*; pub use add_github_issue_labels::*; pub use add_pr_comment::*; -pub use assign_work_item::*; pub use assign_github_issue_milestone::*; pub use assign_github_issue_to_user::*; +pub use assign_work_item::*; pub use close_github_issue::*; pub use comment_on_github_issue::*; pub use comment_on_work_item::*; @@ -832,8 +833,8 @@ pub use reply_to_pr_comment::*; pub use report_incomplete::*; pub use resolve_pr_thread::*; pub use result::{ - ExecutionContext, ExecutionResult, Executor, ResolvedGithubIssue, ResolvedWorkItem, ToolResult, - Validate, anyhow_to_mcp_error, org_from_url, + AdoRepositoryTarget, ExecutionContext, ExecutionResult, Executor, ResolvedGithubIssue, + ResolvedPullRequest, ResolvedWorkItem, ToolResult, Validate, anyhow_to_mcp_error, org_from_url, }; pub use set_github_issue_field::*; pub use set_github_issue_type::*; @@ -1503,7 +1504,11 @@ mod tests { let error = resolve_repository_write_target(Some("target"), &ctx).unwrap_err(); - assert!(error.message.contains("declares the pipeline's current organization")); + assert!( + error + .message + .contains("declares the pipeline's current organization") + ); } #[test] diff --git a/src/safe_outputs/result.rs b/src/safe_outputs/result.rs index bd537595..f80c9d49 100644 --- a/src/safe_outputs/result.rs +++ b/src/safe_outputs/result.rs @@ -7,7 +7,7 @@ use std::collections::{HashMap, HashSet}; use std::sync::{Arc, Mutex}; use crate::sanitize::{SanitizeConfig, SanitizeContent}; -use crate::secure::{GithubTemporaryId, WorkItemTemporaryId}; +use crate::secure::{GithubTemporaryId, PullRequestTemporaryId, WorkItemTemporaryId}; /// Trait for tool results that include a name field pub trait ToolResult: Serialize { @@ -59,6 +59,14 @@ pub struct ResolvedWorkItem { pub url: String, } +/// An Azure DevOps pull request created earlier in the same Stage 3 execution. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ResolvedPullRequest { + pub id: u64, + pub url: String, + pub target: AdoRepositoryTarget, +} + /// Trusted compiler/source metadata for one checked-out repository alias. #[derive(Debug, Clone, PartialEq, Eq)] pub struct AdoRepositoryTargetConfig { @@ -274,6 +282,8 @@ pub struct ExecutionContext { pub resolved_github_issues: Arc>>, /// Temporary work-item IDs resolved by successful `create-work-item` calls. pub resolved_work_items: Arc>>, + /// Temporary pull-request IDs resolved by successful `create-pull-request` calls. + pub resolved_pull_requests: Arc>>, } impl ExecutionContext { @@ -382,6 +392,41 @@ impl ExecutionContext { .map_err(|_| anyhow::anyhow!("temporary work-item map lock poisoned"))?; Ok(work_items.get(&temporary_id.canonical()).cloned()) } + + pub fn has_resolved_pull_request( + &self, + temporary_id: &PullRequestTemporaryId, + ) -> anyhow::Result { + let pull_requests = self + .resolved_pull_requests + .lock() + .map_err(|_| anyhow::anyhow!("temporary pull-request map lock poisoned"))?; + Ok(pull_requests.contains_key(&temporary_id.canonical())) + } + + pub fn register_resolved_pull_request( + &self, + temporary_id: &PullRequestTemporaryId, + pull_request: ResolvedPullRequest, + ) -> anyhow::Result<()> { + register_resolved_reference( + &self.resolved_pull_requests, + temporary_id.canonical(), + pull_request, + "temporary pull-request map lock poisoned", + ) + } + + pub fn resolve_pull_request( + &self, + temporary_id: &PullRequestTemporaryId, + ) -> anyhow::Result> { + let pull_requests = self + .resolved_pull_requests + .lock() + .map_err(|_| anyhow::anyhow!("temporary pull-request map lock poisoned"))?; + Ok(pull_requests.get(&temporary_id.canonical()).cloned()) + } } /// Extract the organization name from an Azure DevOps org URL. @@ -496,6 +541,7 @@ impl ExecutionContext { uploaded_pipeline_artifact_keys: Arc::new(Mutex::new(HashSet::new())), resolved_github_issues: Arc::new(Mutex::new(HashMap::new())), resolved_work_items: Arc::new(Mutex::new(HashMap::new())), + resolved_pull_requests: Arc::new(Mutex::new(HashMap::new())), } } } @@ -579,6 +625,17 @@ impl ExecutionResult { } } + /// Create a warning result with additional data. + pub fn warning_with_data(message: impl Into, data: serde_json::Value) -> Self { + Self { + success: true, + warning: true, + budget_exhausted: false, + message: message.into(), + data: Some(data), + } + } + /// Create a failed execution result pub fn failure(message: impl Into) -> Self { Self { @@ -972,6 +1029,52 @@ mod tests { assert!(r.data.is_none()); } + #[test] + fn warning_with_data_preserves_structured_result() { + let r = ExecutionResult::warning_with_data( + "some reviewers failed", + serde_json::json!({"added": ["one"], "failed": ["two"]}), + ); + assert!(r.success); + assert!(r.is_warning()); + assert_eq!( + r.data.as_ref().unwrap()["failed"], + serde_json::json!(["two"]) + ); + } + + #[test] + fn pull_request_registry_resolves_and_rejects_duplicates() { + let ctx = ExecutionContext::default(); + let temporary_id = PullRequestTemporaryId::parse("aw_pr123").unwrap(); + let resolved = ResolvedPullRequest { + id: 42, + url: "https://example.test/pr/42".to_string(), + target: AdoRepositoryTarget { + alias: "self".to_string(), + organization: "org".to_string(), + organization_url: "https://dev.azure.com/org".to_string(), + project: "project".to_string(), + repository: "repo".to_string(), + repository_id: Some("repo-id".to_string()), + cross_organization: false, + }, + }; + + assert!(!ctx.has_resolved_pull_request(&temporary_id).unwrap()); + ctx.register_resolved_pull_request(&temporary_id, resolved.clone()) + .unwrap(); + assert!(ctx.has_resolved_pull_request(&temporary_id).unwrap()); + assert_eq!( + ctx.resolve_pull_request(&temporary_id).unwrap(), + Some(resolved.clone()) + ); + assert!( + ctx.register_resolved_pull_request(&temporary_id, resolved) + .is_err() + ); + } + #[test] fn test_execution_result_success_is_not_warning() { let r = ExecutionResult::success("all good"); diff --git a/src/safe_outputs/update_pr.rs b/src/safe_outputs/update_pr.rs index 67e44bde..823cd62d 100644 --- a/src/safe_outputs/update_pr.rs +++ b/src/safe_outputs/update_pr.rs @@ -5,10 +5,13 @@ use log::{debug, info, warn}; use percent_encoding::utf8_percent_encode; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; +use std::fmt; -use super::{PATH_SEGMENT, resolve_repo_name}; +use super::result::AdoRepositoryTarget; +use super::{PATH_SEGMENT, canonical_repository_alias, resolve_repository_write_target}; use crate::safe_outputs::{ExecutionContext, ExecutionResult, Executor, Validate}; use crate::sanitize::{SanitizeContent, sanitize as sanitize_text, sanitize_config}; +use crate::secure::PullRequestTemporaryId; use crate::tool_result; use crate::validate::reject_pipeline_injection; use anyhow::{Context, ensure}; @@ -33,6 +36,33 @@ const VALID_VOTES: &[&str] = &[ /// Valid merge strategy values accepted by ADO's completionOptions.mergeStrategy const VALID_MERGE_STRATEGIES: &[&str] = &["squash", "noFastForward", "rebase", "rebaseMerge"]; +const DEFAULT_MAX_REVIEWERS: usize = 3; +const MAX_REVIEWER_LEN: usize = 256; + +/// Positive Azure DevOps pull-request ID or a same-run temporary ID. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, JsonSchema)] +#[serde(untagged)] +pub enum PullRequestReference { + Number(u64), + Temporary(PullRequestTemporaryId), +} + +impl fmt::Display for PullRequestReference { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Number(id) => write!(formatter, "{id}"), + Self::Temporary(temporary_id) => formatter.write_str(&temporary_id.canonical()), + } + } +} + +impl_temporary_reference_deserialize!( + PullRequestReference, + PullRequestTemporaryId, + expecting = "a positive pull-request ID or #aw_ temporary ID", + negative = "pull_request_id must be positive", + quoted_out_of_range = "quoted pull_request_id is outside the u64 range", +); /// Map a vote string to its ADO numeric value fn vote_to_ado_value(vote: &str) -> Option { @@ -49,8 +79,8 @@ fn vote_to_ado_value(vote: &str) -> Option { /// Parameters for updating a pull request #[derive(Deserialize, JsonSchema)] pub struct UpdatePrParams { - /// Pull request ID (must be positive) - pub pull_request_id: i32, + /// Positive pull request ID or a temporary ID from create-pull-request. + pub pull_request_id: PullRequestReference, /// Repository alias: "self" for the pipeline repo, or an alias from the checkout list #[serde(default)] @@ -74,10 +104,9 @@ pub struct UpdatePrParams { impl Validate for UpdatePrParams { fn validate(&self) -> anyhow::Result<()> { - ensure!( - self.pull_request_id > 0, - "pull_request_id must be a positive integer" - ); + if let PullRequestReference::Number(id) = self.pull_request_id { + ensure!(id > 0, "pull_request_id must be positive"); + } if let Some(repository) = &self.repository { reject_pipeline_injection(repository, "repository")?; } @@ -97,6 +126,19 @@ impl Validate for UpdatePrParams { !reviewers.is_empty(), "reviewers list must not be empty for add-reviewers operation" ); + ensure!( + reviewers.len() <= 100, + "reviewers list must contain at most 100 entries" + ); + for reviewer in reviewers { + let reviewer = reviewer.trim(); + ensure!(!reviewer.is_empty(), "reviewer must not be empty"); + ensure!( + reviewer.len() <= MAX_REVIEWER_LEN, + "reviewer must be {MAX_REVIEWER_LEN} characters or fewer" + ); + reject_pipeline_injection(reviewer, "update-pr.reviewer")?; + } } "add-labels" => { let labels = self @@ -141,7 +183,7 @@ tool_result! { params = UpdatePrParams, /// Result of updating a pull request pub struct UpdatePrResult { - pull_request_id: i32, + pull_request_id: PullRequestReference, repository: Option, operation: String, reviewers: Option>, @@ -203,6 +245,16 @@ pub struct UpdatePrConfig { #[serde(default, rename = "allowed-votes")] pub allowed_votes: Vec, + /// Case-insensitive exact allowlist for model-selected reviewers. + /// Empty or a literal "*" allows any valid reviewer. + #[serde(default, rename = "allowed-reviewers")] + pub allowed_reviewers: Vec, + + /// Maximum reviewers accepted by one add-reviewers operation. + #[serde(default = "default_max_reviewers", rename = "max-reviewers")] + #[sanitize_config(skip)] + pub max_reviewers: usize, + /// Whether to delete the source branch after merge (for set-auto-complete, default: true) #[serde(default = "default_true", rename = "delete-source-branch")] pub delete_source_branch: bool, @@ -220,18 +272,121 @@ fn default_merge_strategy() -> String { "squash".to_string() } +fn default_max_reviewers() -> usize { + DEFAULT_MAX_REVIEWERS +} + impl Default for UpdatePrConfig { fn default() -> Self { Self { allowed_operations: Vec::new(), allowed_repositories: Vec::new(), allowed_votes: Vec::new(), + allowed_reviewers: Vec::new(), + max_reviewers: default_max_reviewers(), delete_source_branch: true, merge_strategy: "squash".to_string(), } } } +struct UpdatePrContext<'a> { + client: &'a reqwest::Client, + target: AdoRepositoryTarget, + pr_id: u64, + token: &'a str, + connection_type: Option, +} + +impl UpdatePrContext<'_> { + fn repository_api_base(&self) -> String { + format!( + "{}/{}/_apis/git/repositories/{}", + self.target.organization_url, + utf8_percent_encode(&self.target.project, PATH_SEGMENT), + utf8_percent_encode(self.target.repository_locator(), PATH_SEGMENT), + ) + } +} + +fn repository_is_allowed(config: &UpdatePrConfig, alias: &str) -> bool { + config.allowed_repositories.is_empty() + || config + .allowed_repositories + .iter() + .any(|allowed| allowed.eq_ignore_ascii_case(alias)) +} + +fn resolve_update_pr_target( + reference: &PullRequestReference, + requested_repository: Option<&str>, + config: &UpdatePrConfig, + ctx: &ExecutionContext, +) -> anyhow::Result> { + match reference { + PullRequestReference::Number(id) => { + if *id == 0 { + return Ok(Err(ExecutionResult::failure( + "pull_request_id must be positive", + ))); + } + let selector = requested_repository.unwrap_or("self"); + let Some(alias) = canonical_repository_alias(selector, ctx) else { + return Ok(Err(ExecutionResult::failure(format!( + "Repository '{}' is not in the allowed repository list", + crate::sanitize::neutralize_pipeline_commands(selector) + )))); + }; + if !repository_is_allowed(config, &alias) { + return Ok(Err(ExecutionResult::failure(format!( + "Repository '{}' is not in the allowed-repositories list: [{}]", + alias, + config.allowed_repositories.join(", ") + )))); + } + let target = match resolve_repository_write_target(Some(&alias), ctx) { + Ok(target) => target, + Err(error) => return Ok(Err(error)), + }; + Ok(Ok((*id, target))) + } + PullRequestReference::Temporary(temporary_id) => { + let Some(resolved) = ctx.resolve_pull_request(temporary_id)? else { + return Ok(Err(ExecutionResult::failure(format!( + "temporary pull-request ID '{}' has not been resolved; \ + create-pull-request must succeed earlier in the same SafeOutputs job", + temporary_id.canonical() + )))); + }; + if let Some(selector) = requested_repository { + let Some(alias) = canonical_repository_alias(selector, ctx) else { + return Ok(Err(ExecutionResult::failure(format!( + "Repository '{}' is not in the allowed repository list", + crate::sanitize::neutralize_pipeline_commands(selector) + )))); + }; + if !alias.eq_ignore_ascii_case(&resolved.target.alias) { + return Ok(Err(ExecutionResult::failure(format!( + "temporary pull-request ID '{}' resolved to repository '{}', which does \ + not match requested repository '{}'", + temporary_id.canonical(), + resolved.target.alias, + crate::sanitize::neutralize_pipeline_commands(selector) + )))); + } + } + if !repository_is_allowed(config, &resolved.target.alias) { + return Ok(Err(ExecutionResult::failure(format!( + "Repository '{}' is not in the allowed-repositories list: [{}]", + resolved.target.alias, + config.allowed_repositories.join(", ") + )))); + } + Ok(Ok((resolved.id, resolved.target))) + } + } +} + #[async_trait::async_trait] impl Executor for UpdatePrResult { fn dry_run_summary(&self) -> String { @@ -248,20 +403,10 @@ impl Executor for UpdatePrResult { self.pull_request_id, self.operation ); - let org_url = ctx - .ado_org_url - .as_ref() - .context("AZURE_DEVOPS_ORG_URL not set")?; - let project = ctx - .ado_project - .as_ref() - .context("SYSTEM_TEAMPROJECT not set")?; let token = ctx .access_token .as_ref() .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; - debug!("ADO org: {}, project: {}", org_url, project); - let config: UpdatePrConfig = ctx.get_tool_config("update-pr")?; debug!("Config: {:?}", config); @@ -276,58 +421,35 @@ impl Executor for UpdatePrResult { ))); } - // Validate repository against allowed-repositories - let repo_alias = self.repository.as_deref().unwrap_or("self"); - if !config.allowed_repositories.is_empty() - && !config - .allowed_repositories - .contains(&repo_alias.to_string()) - { - return Ok(ExecutionResult::failure(format!( - "Repository '{}' is not in the allowed-repositories list: [{}]", - repo_alias, - config.allowed_repositories.join(", ") - ))); - } - - // Resolve repo name - let repo_name = match resolve_repo_name(self.repository.as_deref(), ctx) { - Ok(name) => name, + let (pr_id, target) = match resolve_update_pr_target( + &self.pull_request_id, + self.repository.as_deref(), + &config, + ctx, + )? { + Ok(target) => target, Err(failure) => return Ok(failure), }; - debug!("Resolved repository: {}", repo_name); + debug!("Resolved PR target: {} #{}", target.display_name(), pr_id); let client = reqwest::Client::new(); - let encoded_project = utf8_percent_encode(project, PATH_SEGMENT).to_string(); - let base_url = format!( - "{}/{}/_apis/git/repositories", - org_url.trim_end_matches('/'), - encoded_project, - ); + let operation_ctx = UpdatePrContext { + client: &client, + target, + pr_id, + token, + connection_type: ctx.write_connection_type, + }; match self.operation.as_str() { "set-auto-complete" => { - self.execute_set_auto_complete( - &client, &base_url, &repo_name, token, org_url, &config, - ) - .await - } - "vote" => { - self.execute_vote(&client, &base_url, &repo_name, token, org_url, &config) - .await - } - "add-reviewers" => { - self.execute_add_reviewers(&client, &base_url, &repo_name, token, org_url) - .await - } - "add-labels" => { - self.execute_add_labels(&client, &base_url, &repo_name, token) - .await - } - "update-description" => { - self.execute_update_description(&client, &base_url, &repo_name, token) + self.execute_set_auto_complete(&operation_ctx, &config) .await } + "vote" => self.execute_vote(&operation_ctx, &config).await, + "add-reviewers" => self.execute_add_reviewers(&operation_ctx, &config).await, + "add-labels" => self.execute_add_labels(&operation_ctx).await, + "update-description" => self.execute_update_description(&operation_ctx).await, _ => Ok(ExecutionResult::failure(format!( "Unknown operation: {}", self.operation @@ -342,6 +464,79 @@ enum ReviewerAddResult { Failed(String), } +fn reviewer_execution_result( + pr_id: u64, + added: Vec, + failed: Vec, +) -> ExecutionResult { + let mut message = format!("Added {} reviewer(s) to PR #{}", added.len(), pr_id); + if !failed.is_empty() { + message.push_str(&format!( + " ({} failed: {})", + failed.len(), + failed.join(", ") + )); + } + let has_failures = !failed.is_empty(); + let data = serde_json::json!({ + "pull_request_id": pr_id, + "operation": "add-reviewers", + "added": added, + "failed": failed, + }); + if has_failures { + ExecutionResult::warning_with_data(message, data) + } else { + ExecutionResult::success_with_data(message, data) + } +} + +fn validate_and_normalize_reviewers( + reviewers: &[String], + config: &UpdatePrConfig, +) -> Result, ExecutionResult> { + if config.max_reviewers == 0 { + return Err(ExecutionResult::failure( + "update-pr.max-reviewers must be greater than zero", + )); + } + let allow_any = config.allowed_reviewers.is_empty() + || config + .allowed_reviewers + .iter() + .any(|allowed| allowed == "*"); + + let mut normalized = Vec::new(); + for reviewer in reviewers { + let reviewer = reviewer.trim(); + if !allow_any + && !config + .allowed_reviewers + .iter() + .any(|allowed| allowed.eq_ignore_ascii_case(reviewer)) + { + return Err(ExecutionResult::failure(format!( + "Reviewer '{}' is not in update-pr.allowed-reviewers", + crate::sanitize::neutralize_pipeline_commands(reviewer) + ))); + } + if !normalized + .iter() + .any(|existing: &String| existing.eq_ignore_ascii_case(reviewer)) + { + normalized.push(reviewer.to_string()); + } + } + if normalized.len() > config.max_reviewers { + return Err(ExecutionResult::failure(format!( + "add-reviewers requested {} unique reviewers, exceeding max-reviewers: {}", + normalized.len(), + config.max_reviewers + ))); + } + Ok(normalized) +} + impl UpdatePrResult { /// Set auto-complete on a pull request. /// @@ -350,11 +545,7 @@ impl UpdatePrResult { /// Uses the agent's own identity (not the PR creator) for proper audit trail. async fn execute_set_auto_complete( &self, - client: &reqwest::Client, - base_url: &str, - repo_name: &str, - token: &str, - org_url: &str, + operation_ctx: &UpdatePrContext<'_>, config: &UpdatePrConfig, ) -> anyhow::Result { // Validate merge_strategy before any network I/O @@ -366,16 +557,19 @@ impl UpdatePrResult { ))); } - let encoded_repo = utf8_percent_encode(repo_name, PATH_SEGMENT).to_string(); - // Resolve the agent's identity via connection data - let connection_url = format!("{}/_apis/connectiondata", org_url.trim_end_matches('/')); - let conn_response = client - .get(&connection_url) - .basic_auth("", Some(token)) - .send() - .await - .context("Failed to fetch connection data for auto-complete identity")?; + let connection_url = format!( + "{}/_apis/connectiondata", + operation_ctx.target.organization_url.trim_end_matches('/') + ); + let conn_response = crate::safe_outputs::authenticate_ado_request( + operation_ctx.client.get(&connection_url), + operation_ctx.token, + operation_ctx.connection_type, + ) + .send() + .await + .context("Failed to fetch connection data for auto-complete identity")?; if !conn_response.status().is_success() { let status = conn_response.status(); @@ -403,8 +597,9 @@ impl UpdatePrResult { // PATCH to set auto-complete using the agent's identity let patch_url = format!( - "{}/{}/pullRequests/{}?api-version=7.1", - base_url, encoded_repo, self.pull_request_id + "{}/pullRequests/{}?api-version=7.1", + operation_ctx.repository_api_base(), + operation_ctx.pr_id ); let patch_body = serde_json::json!({ "autoCompleteSetBy": { @@ -416,22 +611,24 @@ impl UpdatePrResult { } }); - info!("Setting auto-complete on PR #{}", self.pull_request_id); - let response = client - .patch(&patch_url) - .header("Content-Type", "application/json") - .basic_auth("", Some(token)) - .json(&patch_body) - .send() - .await - .context("Failed to set auto-complete on PR")?; + info!("Setting auto-complete on PR #{}", operation_ctx.pr_id); + let response = crate::safe_outputs::authenticate_ado_request( + operation_ctx.client.patch(&patch_url), + operation_ctx.token, + operation_ctx.connection_type, + ) + .header("Content-Type", "application/json") + .json(&patch_body) + .send() + .await + .context("Failed to set auto-complete on PR")?; if response.status().is_success() { - info!("Auto-complete set on PR #{}", self.pull_request_id); + info!("Auto-complete set on PR #{}", operation_ctx.pr_id); Ok(ExecutionResult::success_with_data( - format!("Auto-complete set on PR #{}", self.pull_request_id), + format!("Auto-complete set on PR #{}", operation_ctx.pr_id), serde_json::json!({ - "pull_request_id": self.pull_request_id, + "pull_request_id": operation_ctx.pr_id, "operation": "set-auto-complete", }), )) @@ -443,7 +640,7 @@ impl UpdatePrResult { .unwrap_or_else(|_| "Unknown error".to_string()); Ok(ExecutionResult::failure(format!( "Failed to set auto-complete on PR #{} (HTTP {}): {}", - self.pull_request_id, status, error_body + operation_ctx.pr_id, status, error_body ))) } } @@ -454,11 +651,7 @@ impl UpdatePrResult { /// PUTs the vote to the reviewers endpoint. async fn execute_vote( &self, - client: &reqwest::Client, - base_url: &str, - repo_name: &str, - token: &str, - org_url: &str, + operation_ctx: &UpdatePrContext<'_>, config: &UpdatePrConfig, ) -> anyhow::Result { let vote_str = self @@ -492,15 +685,20 @@ impl UpdatePrResult { // Resolve the current user identity. // Use the org URL for connection data — supports vanity domains and national clouds. - let connection_url = format!("{}/_apis/connectiondata", org_url.trim_end_matches('/')); + let connection_url = format!( + "{}/_apis/connectiondata", + operation_ctx.target.organization_url.trim_end_matches('/') + ); debug!("Connection data URL: {}", connection_url); - let conn_response = client - .get(&connection_url) - .basic_auth("", Some(token)) - .send() - .await - .context("Failed to fetch connection data")?; + let conn_response = crate::safe_outputs::authenticate_ado_request( + operation_ctx.client.get(&connection_url), + operation_ctx.token, + operation_ctx.connection_type, + ) + .send() + .await + .context("Failed to fetch connection data")?; if !conn_response.status().is_success() { let status = conn_response.status(); @@ -530,17 +728,19 @@ impl UpdatePrResult { // Positive votes (approve=10, approve-with-suggestions=5) are blocked when // the authenticated user is also the PR author. if vote_value > 0 { - let encoded_repo_check = utf8_percent_encode(repo_name, PATH_SEGMENT).to_string(); let pr_url = format!( - "{}/{}/pullRequests/{}?api-version=7.1", - base_url, encoded_repo_check, self.pull_request_id + "{}/pullRequests/{}?api-version=7.1", + operation_ctx.repository_api_base(), + operation_ctx.pr_id ); - let pr_response = client - .get(&pr_url) - .basic_auth("", Some(token)) - .send() - .await - .context("Failed to fetch PR for self-approval check")?; + let pr_response = crate::safe_outputs::authenticate_ado_request( + operation_ctx.client.get(&pr_url), + operation_ctx.token, + operation_ctx.connection_type, + ) + .send() + .await + .context("Failed to fetch PR for self-approval check")?; if pr_response.status().is_success() { let pr_body: serde_json::Value = pr_response @@ -557,7 +757,7 @@ impl UpdatePrResult { return Ok(ExecutionResult::failure(format!( "Self-approval blocked: the authenticated identity created PR #{} \ and cannot cast a positive vote ('{}') on it", - self.pull_request_id, vote_str + operation_ctx.pr_id, vote_str ))); } } else { @@ -568,17 +768,18 @@ impl UpdatePrResult { .unwrap_or_else(|_| "Unknown error".to_string()); return Ok(ExecutionResult::failure(format!( "Failed to fetch PR #{} for self-approval check (HTTP {}): {}", - self.pull_request_id, status, error_body + operation_ctx.pr_id, status, error_body ))); } } // PUT vote to reviewers endpoint - let encoded_repo = utf8_percent_encode(repo_name, PATH_SEGMENT).to_string(); let encoded_user_id = utf8_percent_encode(user_id, PATH_SEGMENT).to_string(); let vote_url = format!( - "{}/{}/pullRequests/{}/reviewers/{}?api-version=7.1", - base_url, encoded_repo, self.pull_request_id, encoded_user_id + "{}/pullRequests/{}/reviewers/{}?api-version=7.1", + operation_ctx.repository_api_base(), + operation_ctx.pr_id, + encoded_user_id ); let vote_body = serde_json::json!({ "vote": vote_value @@ -586,29 +787,31 @@ impl UpdatePrResult { info!( "Voting '{}' ({}) on PR #{}", - vote_str, vote_value, self.pull_request_id + vote_str, vote_value, operation_ctx.pr_id ); - let response = client - .put(&vote_url) - .header("Content-Type", "application/json") - .basic_auth("", Some(token)) - .json(&vote_body) - .send() - .await - .context("Failed to submit vote")?; + let response = crate::safe_outputs::authenticate_ado_request( + operation_ctx.client.put(&vote_url), + operation_ctx.token, + operation_ctx.connection_type, + ) + .header("Content-Type", "application/json") + .json(&vote_body) + .send() + .await + .context("Failed to submit vote")?; if response.status().is_success() { info!( "Vote '{}' submitted on PR #{}", - vote_str, self.pull_request_id + vote_str, operation_ctx.pr_id ); Ok(ExecutionResult::success_with_data( format!( "Vote '{}' submitted on PR #{}", - vote_str, self.pull_request_id + vote_str, operation_ctx.pr_id ), serde_json::json!({ - "pull_request_id": self.pull_request_id, + "pull_request_id": operation_ctx.pr_id, "operation": "vote", "vote": vote_str, "vote_value": vote_value, @@ -622,7 +825,7 @@ impl UpdatePrResult { .unwrap_or_else(|_| "Unknown error".to_string()); Ok(ExecutionResult::failure(format!( "Failed to submit vote on PR #{} (HTTP {}): {}", - self.pull_request_id, status, error_body + operation_ctx.pr_id, status, error_body ))) } } @@ -633,23 +836,23 @@ impl UpdatePrResult { /// the reviewers endpoint with vote 0. async fn execute_add_reviewers( &self, - client: &reqwest::Client, - base_url: &str, - repo_name: &str, - token: &str, - org_url: &str, + operation_ctx: &UpdatePrContext<'_>, + config: &UpdatePrConfig, ) -> anyhow::Result { - let reviewers = self + let requested_reviewers = self .reviewers .as_ref() .context("reviewers list is required for add-reviewers operation")?; + let reviewers = match validate_and_normalize_reviewers(requested_reviewers, config) { + Ok(reviewers) => reviewers, + Err(failure) => return Ok(failure), + }; - let encoded_repo = utf8_percent_encode(repo_name, PATH_SEGMENT).to_string(); let mut added = Vec::new(); let mut failed = Vec::new(); // Derive VSSPS base URL once, before the loop. - let trimmed_org = org_url.trim_end_matches('/'); + let trimmed_org = operation_ctx.target.organization_url.trim_end_matches('/'); let vssps_base = trimmed_org.replace("://dev.azure.com/", "://vssps.dev.azure.com/"); if vssps_base == trimmed_org { return Ok(ExecutionResult::failure(format!( @@ -661,15 +864,15 @@ impl UpdatePrResult { ))); } - for reviewer in reviewers { + for reviewer in &reviewers { match resolve_and_add_reviewer( - client, + operation_ctx.client, &vssps_base, - base_url, - &encoded_repo, - self.pull_request_id, + &operation_ctx.repository_api_base(), + operation_ctx.pr_id, reviewer, - token, + operation_ctx.token, + operation_ctx.connection_type, ) .await { @@ -680,35 +883,11 @@ impl UpdatePrResult { } } - if added.is_empty() && !failed.is_empty() { - Ok(ExecutionResult::failure(format!( - "Failed to add any reviewers to PR #{}: {}", - self.pull_request_id, - failed.join(", ") - ))) - } else { - let mut message = format!( - "Added {} reviewer(s) to PR #{}", - added.len(), - self.pull_request_id - ); - if !failed.is_empty() { - message.push_str(&format!( - " ({} failed: {})", - failed.len(), - failed.join(", ") - )); - } - Ok(ExecutionResult::success_with_data( - message, - serde_json::json!({ - "pull_request_id": self.pull_request_id, - "operation": "add-reviewers", - "added": added, - "failed": failed, - }), - )) - } + Ok(reviewer_execution_result( + operation_ctx.pr_id, + added, + failed, + )) } /// Add labels to a pull request. @@ -716,20 +895,17 @@ impl UpdatePrResult { /// For each label, POSTs to the labels endpoint. async fn execute_add_labels( &self, - client: &reqwest::Client, - base_url: &str, - repo_name: &str, - token: &str, + operation_ctx: &UpdatePrContext<'_>, ) -> anyhow::Result { let labels = self .labels .as_ref() .context("labels list is required for add-labels operation")?; - let encoded_repo = utf8_percent_encode(repo_name, PATH_SEGMENT).to_string(); let labels_url = format!( - "{}/{}/pullRequests/{}/labels?api-version=7.1", - base_url, encoded_repo, self.pull_request_id + "{}/pullRequests/{}/labels?api-version=7.1", + operation_ctx.repository_api_base(), + operation_ctx.pr_id ); let mut added = Vec::new(); @@ -740,18 +916,20 @@ impl UpdatePrResult { "name": label }); - debug!("Adding label '{}' to PR #{}", label, self.pull_request_id); - let response = client - .post(&labels_url) - .header("Content-Type", "application/json") - .basic_auth("", Some(token)) - .json(&label_body) - .send() - .await; + debug!("Adding label '{}' to PR #{}", label, operation_ctx.pr_id); + let response = crate::safe_outputs::authenticate_ado_request( + operation_ctx.client.post(&labels_url), + operation_ctx.token, + operation_ctx.connection_type, + ) + .header("Content-Type", "application/json") + .json(&label_body) + .send() + .await; match response { Ok(resp) if resp.status().is_success() => { - info!("Added label '{}' to PR #{}", label, self.pull_request_id); + info!("Added label '{}' to PR #{}", label, operation_ctx.pr_id); added.push(label.clone()); } Ok(resp) => { @@ -762,14 +940,14 @@ impl UpdatePrResult { .unwrap_or_else(|_| "Unknown error".to_string()); warn!( "Failed to add label '{}' to PR #{} (HTTP {}): {}", - label, self.pull_request_id, status, error_body + label, operation_ctx.pr_id, status, error_body ); failed.push(format!("{} (HTTP {})", label, status)); } Err(e) => { warn!( "Request failed for label '{}' on PR #{}: {}", - label, self.pull_request_id, e + label, operation_ctx.pr_id, e ); failed.push(format!("{} (request error)", label)); } @@ -779,14 +957,14 @@ impl UpdatePrResult { if added.is_empty() && !failed.is_empty() { Ok(ExecutionResult::failure(format!( "Failed to add any labels to PR #{}: {}", - self.pull_request_id, + operation_ctx.pr_id, failed.join(", ") ))) } else { let mut message = format!( "Added {} label(s) to PR #{}", added.len(), - self.pull_request_id + operation_ctx.pr_id ); if !failed.is_empty() { message.push_str(&format!( @@ -798,7 +976,7 @@ impl UpdatePrResult { Ok(ExecutionResult::success_with_data( message, serde_json::json!({ - "pull_request_id": self.pull_request_id, + "pull_request_id": operation_ctx.pr_id, "operation": "add-labels", "added": added, "failed": failed, @@ -810,20 +988,17 @@ impl UpdatePrResult { /// Update the description of a pull request. async fn execute_update_description( &self, - client: &reqwest::Client, - base_url: &str, - repo_name: &str, - token: &str, + operation_ctx: &UpdatePrContext<'_>, ) -> anyhow::Result { let description = self .description .as_ref() .context("description is required for update-description operation")?; - let encoded_repo = utf8_percent_encode(repo_name, PATH_SEGMENT).to_string(); let patch_url = format!( - "{}/{}/pullRequests/{}?api-version=7.1", - base_url, encoded_repo, self.pull_request_id + "{}/pullRequests/{}?api-version=7.1", + operation_ctx.repository_api_base(), + operation_ctx.pr_id ); let patch_body = serde_json::json!({ "description": description @@ -831,24 +1006,26 @@ impl UpdatePrResult { info!( "Updating description on PR #{} ({} chars)", - self.pull_request_id, + operation_ctx.pr_id, description.len() ); - let response = client - .patch(&patch_url) - .header("Content-Type", "application/json") - .basic_auth("", Some(token)) - .json(&patch_body) - .send() - .await - .context("Failed to update PR description")?; + let response = crate::safe_outputs::authenticate_ado_request( + operation_ctx.client.patch(&patch_url), + operation_ctx.token, + operation_ctx.connection_type, + ) + .header("Content-Type", "application/json") + .json(&patch_body) + .send() + .await + .context("Failed to update PR description")?; if response.status().is_success() { - info!("Description updated on PR #{}", self.pull_request_id); + info!("Description updated on PR #{}", operation_ctx.pr_id); Ok(ExecutionResult::success_with_data( - format!("Description updated on PR #{}", self.pull_request_id), + format!("Description updated on PR #{}", operation_ctx.pr_id), serde_json::json!({ - "pull_request_id": self.pull_request_id, + "pull_request_id": operation_ctx.pr_id, "operation": "update-description", }), )) @@ -860,7 +1037,7 @@ impl UpdatePrResult { .unwrap_or_else(|_| "Unknown error".to_string()); Ok(ExecutionResult::failure(format!( "Failed to update description on PR #{} (HTTP {}): {}", - self.pull_request_id, status, error_body + operation_ctx.pr_id, status, error_body ))) } } @@ -874,28 +1051,73 @@ async fn lookup_reviewer_id( vssps_base: &str, reviewer: &str, token: &str, + connection_type: Option, ) -> Option { - let identity_url = format!( - "{}/_apis/identities?searchFilter=General&filterValue={}&api-version=7.1", - vssps_base, - utf8_percent_encode(reviewer, PATH_SEGMENT), - ); + if reviewer.len() == 36 + && reviewer + .chars() + .filter(|character| *character == '-') + .count() + == 4 + && reviewer + .chars() + .all(|character| character.is_ascii_hexdigit() || character == '-') + { + return Some(reviewer.to_string()); + } + + let identity_url = format!("{}/_apis/identities", vssps_base); debug!("Resolving identity for '{}': {}", reviewer, identity_url); - match client - .get(&identity_url) - .basic_auth("", Some(token)) - .send() - .await + match crate::safe_outputs::authenticate_ado_request( + client.get(&identity_url).query(&[ + ("searchFilter", "General"), + ("filterValue", reviewer), + ("api-version", "7.1"), + ]), + token, + connection_type, + ) + .send() + .await { Ok(resp) if resp.status().is_success() => { let body: serde_json::Value = resp.json().await.unwrap_or_default(); - body.get("value") + let matching_ids = body + .get("value") .and_then(|v| v.as_array()) - .and_then(|arr| arr.first()) - .and_then(|entry| entry.get("id")) - .and_then(|id| id.as_str()) - .map(|s| s.to_string()) + .into_iter() + .flatten() + .filter(|identity| { + let direct_match = ["providerDisplayName", "customDisplayName", "displayName"] + .iter() + .filter_map(|field| identity.get(field).and_then(serde_json::Value::as_str)) + .any(|value| value.eq_ignore_ascii_case(reviewer)); + let property_match = ["Account", "Mail"] + .iter() + .filter_map(|field| { + identity + .get("properties") + .and_then(|properties| properties.get(field)) + .and_then(|property| property.get("$value")) + .and_then(serde_json::Value::as_str) + }) + .any(|value| value.eq_ignore_ascii_case(reviewer)); + direct_match || property_match + }) + .filter_map(|entry| entry.get("id").and_then(serde_json::Value::as_str)) + .collect::>(); + if matching_ids.len() == 1 { + matching_ids.into_iter().next().map(str::to_string) + } else { + if matching_ids.len() > 1 { + warn!( + "Identity lookup for '{}' returned multiple exact matches", + reviewer + ); + } + None + } } Ok(resp) => { warn!( @@ -917,27 +1139,29 @@ async fn lookup_reviewer_id( /// with a short reason string on any HTTP or transport error. async fn add_reviewer_to_pr( client: &reqwest::Client, - base_url: &str, - encoded_repo: &str, - pr_id: i32, + repository_api_base: &str, + pr_id: u64, reviewer_id: &str, reviewer: &str, token: &str, + connection_type: Option, ) -> ReviewerAddResult { let reviewer_url = format!( - "{}/{}/pullRequests/{}/reviewers/{}?api-version=7.1", - base_url, encoded_repo, pr_id, reviewer_id, + "{}/pullRequests/{}/reviewers/{}?api-version=7.1", + repository_api_base, pr_id, reviewer_id, ); let reviewer_body = serde_json::json!({ "vote": 0, "isRequired": false }); debug!("Adding reviewer '{}' to PR #{}", reviewer, pr_id); - let response = client - .put(&reviewer_url) - .header("Content-Type", "application/json") - .basic_auth("", Some(token)) - .json(&reviewer_body) - .send() - .await; + let response = crate::safe_outputs::authenticate_ado_request( + client.put(&reviewer_url), + token, + connection_type, + ) + .header("Content-Type", "application/json") + .json(&reviewer_body) + .send() + .await; match response { Ok(resp) if resp.status().is_success() => { @@ -972,24 +1196,26 @@ async fn add_reviewer_to_pr( async fn resolve_and_add_reviewer( client: &reqwest::Client, vssps_base: &str, - base_url: &str, - encoded_repo: &str, - pr_id: i32, + repository_api_base: &str, + pr_id: u64, reviewer: &str, token: &str, + connection_type: Option, ) -> ReviewerAddResult { - let Some(reviewer_id) = lookup_reviewer_id(client, vssps_base, reviewer, token).await else { + let Some(reviewer_id) = + lookup_reviewer_id(client, vssps_base, reviewer, token, connection_type).await + else { warn!("Could not resolve identity for '{}', skipping", reviewer); return ReviewerAddResult::Failed("identity not found".to_string()); }; add_reviewer_to_pr( client, - base_url, - encoded_repo, + repository_api_base, pr_id, &reviewer_id, reviewer, token, + connection_type, ) .await } @@ -1011,15 +1237,24 @@ mod tests { "operation": "set-auto-complete" }"#; let params: UpdatePrParams = serde_json::from_str(json).unwrap(); - assert_eq!(params.pull_request_id, 42); + assert_eq!(params.pull_request_id, PullRequestReference::Number(42)); assert_eq!(params.operation, "set-auto-complete"); assert!(params.repository.is_none()); } + #[test] + fn pull_request_reference_accepts_quoted_numbers_and_temporary_ids() { + let quoted: PullRequestReference = serde_json::from_str("\"42\"").unwrap(); + let temporary: PullRequestReference = serde_json::from_str("\"#aw_pr123\"").unwrap(); + assert_eq!(quoted, PullRequestReference::Number(42)); + assert!(matches!(temporary, PullRequestReference::Temporary(_))); + assert!(serde_json::from_str::("\"not-an-id\"").is_err()); + } + #[test] fn test_params_converts_to_result() { let params = UpdatePrParams { - pull_request_id: 42, + pull_request_id: PullRequestReference::Number(42), repository: Some("self".to_string()), operation: "set-auto-complete".to_string(), reviewers: None, @@ -1029,14 +1264,14 @@ mod tests { }; let result: UpdatePrResult = params.try_into().unwrap(); assert_eq!(result.name, "update-pr"); - assert_eq!(result.pull_request_id, 42); + assert_eq!(result.pull_request_id, PullRequestReference::Number(42)); assert_eq!(result.operation, "set-auto-complete"); } #[test] fn test_validation_rejects_zero_pr_id() { let params = UpdatePrParams { - pull_request_id: 0, + pull_request_id: PullRequestReference::Number(0), repository: None, operation: "set-auto-complete".to_string(), reviewers: None, @@ -1051,7 +1286,7 @@ mod tests { #[test] fn test_validation_rejects_invalid_operation() { let params = UpdatePrParams { - pull_request_id: 1, + pull_request_id: PullRequestReference::Number(1), repository: None, operation: "delete-pr".to_string(), reviewers: None, @@ -1067,7 +1302,7 @@ mod tests { #[test] fn test_validation_rejects_vote_without_value() { let params = UpdatePrParams { - pull_request_id: 1, + pull_request_id: PullRequestReference::Number(1), repository: None, operation: "vote".to_string(), reviewers: None, @@ -1082,7 +1317,7 @@ mod tests { #[test] fn test_validation_rejects_reviewers_without_list() { let params = UpdatePrParams { - pull_request_id: 1, + pull_request_id: PullRequestReference::Number(1), repository: None, operation: "add-reviewers".to_string(), reviewers: None, @@ -1097,7 +1332,7 @@ mod tests { #[test] fn test_validation_rejects_repository_pipeline_command() { let params = UpdatePrParams { - pull_request_id: 1, + pull_request_id: PullRequestReference::Number(1), repository: Some("##vso[task.setvariable variable=x]y".to_string()), operation: "set-auto-complete".to_string(), reviewers: None, @@ -1112,7 +1347,7 @@ mod tests { #[test] fn test_result_serializes_correctly() { let params = UpdatePrParams { - pull_request_id: 99, + pull_request_id: PullRequestReference::Number(99), repository: Some("self".to_string()), operation: "vote".to_string(), reviewers: None, @@ -1134,9 +1369,258 @@ mod tests { assert!(config.allowed_operations.is_empty()); assert!(config.allowed_repositories.is_empty()); assert!(config.allowed_votes.is_empty()); + assert!(config.allowed_reviewers.is_empty()); + assert_eq!(config.max_reviewers, DEFAULT_MAX_REVIEWERS); assert_eq!(config.merge_strategy, "squash"); } + #[test] + fn reviewer_policy_allows_omitted_allowlist_and_explicit_wildcard() { + let reviewers = vec!["owner@example.com".to_string()]; + assert_eq!( + validate_and_normalize_reviewers(&reviewers, &UpdatePrConfig::default()).unwrap(), + reviewers + ); + + let config = UpdatePrConfig { + allowed_reviewers: vec!["*".to_string()], + ..Default::default() + }; + assert_eq!( + validate_and_normalize_reviewers(&reviewers, &config).unwrap(), + reviewers + ); + } + + #[test] + fn reviewer_policy_restricts_non_empty_allowlist() { + let result = validate_and_normalize_reviewers( + &["other@example.com".to_string()], + &UpdatePrConfig { + allowed_reviewers: vec!["owner@example.com".to_string()], + ..Default::default() + }, + ); + assert!(result.unwrap_err().message.contains("allowed-reviewers")); + } + + #[test] + fn reviewer_policy_deduplicates_and_enforces_limit() { + let config = UpdatePrConfig { + allowed_reviewers: vec![ + "Owner@example.com".to_string(), + "other@example.com".to_string(), + ], + max_reviewers: 2, + ..Default::default() + }; + let reviewers = validate_and_normalize_reviewers( + &[ + "owner@example.com".to_string(), + "OWNER@example.com".to_string(), + ], + &config, + ) + .unwrap(); + assert_eq!(reviewers, ["owner@example.com"]); + + let too_many = validate_and_normalize_reviewers( + &[ + "owner@example.com".to_string(), + "other@example.com".to_string(), + "third@example.com".to_string(), + ], + &UpdatePrConfig { + allowed_reviewers: vec!["*".to_string()], + max_reviewers: 2, + ..Default::default() + }, + ); + assert!(too_many.unwrap_err().message.contains("max-reviewers")); + } + + #[test] + fn reviewer_results_warn_for_partial_and_total_failures() { + let partial = reviewer_execution_result( + 42, + vec!["added@example.com".to_string()], + vec!["failed@example.com (HTTP 403)".to_string()], + ); + assert!(partial.success); + assert!(partial.is_warning()); + assert_eq!( + partial.data.as_ref().unwrap()["added"][0], + "added@example.com" + ); + + let total = reviewer_execution_result( + 42, + Vec::new(), + vec!["failed@example.com (identity not found)".to_string()], + ); + assert!(total.success); + assert!(total.is_warning()); + assert_eq!( + total.data.as_ref().unwrap()["failed"] + .as_array() + .unwrap() + .len(), + 1 + ); + + let success = + reviewer_execution_result(42, vec!["added@example.com".to_string()], Vec::new()); + assert!(success.success); + assert!(!success.is_warning()); + } + + #[test] + fn temporary_reference_resolves_exact_registered_target() { + let temporary_id = PullRequestTemporaryId::parse("#aw_pr123").unwrap(); + let ctx = ExecutionContext::default(); + let target = AdoRepositoryTarget { + alias: "tools".to_string(), + organization: "other-org".to_string(), + organization_url: "https://dev.azure.com/other-org".to_string(), + project: "Other Project".to_string(), + repository: "tools".to_string(), + repository_id: Some("repo-id".to_string()), + cross_organization: true, + }; + ctx.register_resolved_pull_request( + &temporary_id, + crate::safe_outputs::ResolvedPullRequest { + id: 42, + url: "https://example.test/pr/42".to_string(), + target: target.clone(), + }, + ) + .unwrap(); + + let resolved = resolve_update_pr_target( + &PullRequestReference::Temporary(temporary_id), + None, + &UpdatePrConfig::default(), + &ctx, + ) + .unwrap() + .unwrap(); + assert_eq!(resolved, (42, target)); + } + + #[tokio::test] + async fn reviewer_identity_lookup_requires_exact_match() { + use wiremock::matchers::{method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/_apis/identities")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "value": [ + { + "id": "wrong-id", + "providerDisplayName": "Similar Person", + "properties": {"Mail": {"$value": "similar@example.com"}} + }, + { + "id": "exact-id", + "providerDisplayName": "Exact Person", + "properties": {"Mail": {"$value": "owner@example.com"}} + } + ] + }))) + .mount(&server) + .await; + + let id = lookup_reviewer_id( + &reqwest::Client::new(), + &server.uri(), + "owner@example.com", + "token", + None, + ) + .await; + assert_eq!(id.as_deref(), Some("exact-id")); + + let missing = lookup_reviewer_id( + &reqwest::Client::new(), + &server.uri(), + "missing@example.com", + "token", + None, + ) + .await; + assert!(missing.is_none()); + } + + #[tokio::test] + async fn reviewer_identity_lookup_rejects_ambiguous_exact_matches() { + use wiremock::matchers::{method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/_apis/identities")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "value": [ + { + "id": "first-id", + "properties": {"Mail": {"$value": "owner@example.com"}} + }, + { + "id": "second-id", + "properties": {"Mail": {"$value": "owner@example.com"}} + } + ] + }))) + .mount(&server) + .await; + + let id = lookup_reviewer_id( + &reqwest::Client::new(), + &server.uri(), + "owner@example.com", + "token", + None, + ) + .await; + assert!(id.is_none()); + } + + #[tokio::test] + async fn reviewer_identity_lookup_encodes_filter_as_one_query_parameter() { + use wiremock::matchers::{method, path, query_param}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + let server = MockServer::start().await; + let reviewer = "owner+alerts&team=core@example.com"; + Mock::given(method("GET")) + .and(path("/_apis/identities")) + .and(query_param("searchFilter", "General")) + .and(query_param("filterValue", reviewer)) + .and(query_param("api-version", "7.1")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "value": [{ + "id": "exact-id", + "properties": {"Mail": {"$value": reviewer}} + }] + }))) + .expect(1) + .mount(&server) + .await; + + let id = lookup_reviewer_id( + &reqwest::Client::new(), + &server.uri(), + reviewer, + "token", + None, + ) + .await; + assert_eq!(id.as_deref(), Some("exact-id")); + } + #[test] fn test_config_deserializes_from_yaml() { let yaml = r#" @@ -1148,6 +1632,9 @@ allowed-repositories: allowed-votes: - approve - reject +allowed-reviewers: + - owner@example.com +max-reviewers: 2 "#; let config: UpdatePrConfig = serde_yaml::from_str(yaml).unwrap(); assert_eq!(config.allowed_operations.len(), 2); @@ -1163,6 +1650,8 @@ allowed-votes: ); assert_eq!(config.allowed_repositories.len(), 1); assert_eq!(config.allowed_votes.len(), 2); + assert_eq!(config.allowed_reviewers, ["owner@example.com"]); + assert_eq!(config.max_reviewers, 2); } #[test] diff --git a/src/safe_outputs/upload_build_attachment.rs b/src/safe_outputs/upload_build_attachment.rs index 70dd486e..d0648f65 100644 --- a/src/safe_outputs/upload_build_attachment.rs +++ b/src/safe_outputs/upload_build_attachment.rs @@ -946,6 +946,9 @@ attachment-type: "agent-artifact" resolved_work_items: std::sync::Arc::new(std::sync::Mutex::new( std::collections::HashMap::new(), )), + resolved_pull_requests: std::sync::Arc::new(std::sync::Mutex::new( + std::collections::HashMap::new(), + )), triggered_by_build_id: None, triggered_by_definition_name: None, triggered_by_build_number: None, diff --git a/src/secure.rs b/src/secure.rs index 41e95c77..3c566ba0 100644 --- a/src/secure.rs +++ b/src/secure.rs @@ -304,6 +304,11 @@ validated_string! { WorkItemTemporaryId, "temporary_id", validate_temporary_id } +validated_string! { + /// A temporary Azure DevOps pull-request identifier used to link safe outputs in one run. + PullRequestTemporaryId, "temporary_id", validate_temporary_id +} + impl GithubTemporaryId { /// Canonical map/reference form with the leading `#`. pub fn canonical(&self) -> String { @@ -326,6 +331,17 @@ impl WorkItemTemporaryId { } } +impl PullRequestTemporaryId { + /// Canonical map/reference form with the leading `#`. + pub fn canonical(&self) -> String { + if self.as_str().starts_with('#') { + self.as_str().to_string() + } else { + format!("#{}", self.as_str()) + } + } +} + validated_string! { /// An Azure DevOps pipeline or variable-group variable name. /// diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index 991f20c4..fb3af4ce 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -1689,6 +1689,61 @@ Vote on pull requests. let _ = fs::remove_dir_all(&temp_dir); } +/// Test that temporary PR producers and consumers cannot be split across the +/// automatic and manually reviewed SafeOutputs jobs. +#[test] +fn test_pull_request_temporary_id_tools_require_matching_approval_lanes() { + let temp_dir = + std::env::temp_dir().join(format!("agentic-pipeline-prlane-{}", std::process::id())); + fs::create_dir_all(&temp_dir).expect("Failed to create temp directory"); + + let test_input = temp_dir.join("pr-lane-agent.md"); + let test_content = r#"--- +name: "PR Lane Agent" +description: "Agent that creates and then updates a pull request" +permissions: + write: my-write-sc +safe-outputs: + create-pull-request: + require-approval: true + update-pr: + require-approval: false + allowed-operations: + - update-description +--- + +## PR Lane Agent + +Create and update pull requests. +"#; + fs::write(&test_input, test_content).expect("Failed to write test input"); + + let output_path = temp_dir.join("pr-lane-agent.yml"); + let binary_path = PathBuf::from(env!("CARGO_BIN_EXE_ado-aw")); + let output = std::process::Command::new(&binary_path) + .args([ + "compile", + test_input.to_str().unwrap(), + "-o", + output_path.to_str().unwrap(), + ]) + .output() + .expect("Failed to run compiler"); + + assert!( + !output.status.success(), + "Compiler should reject temporary PR tools in different approval lanes" + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("temporary pull-request IDs") + && stderr.contains("same effective require-approval"), + "Unexpected compiler error: {stderr}" + ); + + let _ = fs::remove_dir_all(&temp_dir); +} + /// Test that update-pr compiles successfully whether the vote operation is made /// unreachable via `allowed-operations` (excluding "vote") or is reachable but /// backed by a non-empty `allowed-votes` list. Both configurations satisfy diff --git a/tests/executor-e2e/README.md b/tests/executor-e2e/README.md index bd1f52f4..5f920e80 100644 --- a/tests/executor-e2e/README.md +++ b/tests/executor-e2e/README.md @@ -128,6 +128,12 @@ The checked-in pipeline resolves `E2E_WORK_ITEM_ASSIGNEE` from a same-named definition/queue-time variable first, then falls back to `Build.RequestedForEmail`. +The live create-PR → add-reviewers scenario resolves +`EXECUTOR_E2E_REVIEWER` the same way: a definition/queue-time override takes +precedence, then `Build.RequestedForEmail` is used. The reviewer must resolve +to exactly one Azure DevOps identity; otherwise the scenario skips before +creating remote state. + > **Coverage note.** The signal scenarios (`noop`, `missing-tool`, > `missing-data`, `report-incomplete`) were previously exercised only by > now-deleted per-tool agentic smoke pipelines. Adding them here closes diff --git a/tests/executor-e2e/azure-pipelines.yml b/tests/executor-e2e/azure-pipelines.yml index 2b6a7bd7..abd1a94c 100644 --- a/tests/executor-e2e/azure-pipelines.yml +++ b/tests/executor-e2e/azure-pipelines.yml @@ -41,11 +41,13 @@ pool: variables: # Keep YAML defaults under private names so same-named pipeline/definition UI # variables (EXECUTOR_E2E_ADO_REPO, E2E_QUEUE_PIPELINE_ID, E2E_WIKI_NAME, - # E2E_WORK_ITEM_ASSIGNEE, CRATES_IO_FEED) are not shadowed by this block. + # E2E_WORK_ITEM_ASSIGNEE, EXECUTOR_E2E_REVIEWER, CRATES_IO_FEED) are not + # shadowed by this block. EFFECTIVE_EXECUTOR_E2E_ADO_REPO: $[ coalesce(variables['EXECUTOR_E2E_ADO_REPO'], 'agent-definitions') ] EFFECTIVE_E2E_QUEUE_PIPELINE_ID: $[ coalesce(variables['E2E_QUEUE_PIPELINE_ID'], '') ] EFFECTIVE_E2E_WIKI_NAME: $[ coalesce(variables['E2E_WIKI_NAME'], '') ] EFFECTIVE_E2E_WORK_ITEM_ASSIGNEE: $[ coalesce(variables['E2E_WORK_ITEM_ASSIGNEE'], variables['Build.RequestedForEmail'], '') ] + EFFECTIVE_EXECUTOR_E2E_REVIEWER: $[ coalesce(variables['EXECUTOR_E2E_REVIEWER'], variables['Build.RequestedForEmail'], '') ] # GitHub issue scenarios: optional scratch-repo override and forced issue-type # name. Both empty by default so the scenarios skip gracefully rather than # filing scratch issues into an unintended repository. @@ -153,6 +155,9 @@ steps: E2E_QUEUE_PIPELINE_ID: $(EFFECTIVE_E2E_QUEUE_PIPELINE_ID) E2E_WIKI_NAME: $(EFFECTIVE_E2E_WIKI_NAME) E2E_WORK_ITEM_ASSIGNEE: $(EFFECTIVE_E2E_WORK_ITEM_ASSIGNEE) + # The write-token identity creates/updates the PR, while the human run + # requester is the distinct reviewer selected by this E2E scenario. + EXECUTOR_E2E_REVIEWER: $(EFFECTIVE_EXECUTOR_E2E_REVIEWER) # GitHub issue scenarios. They reuse EXECUTOR_E2E_GITHUB_TOKEN above, # which must carry Issues:write on the scratch repo. When # EXECUTOR_E2E_SCENARIO_ISSUE_REPO is unset they fall back to