diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts index 3fd74e08130e..66e6b5c5a0bb 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts @@ -556,15 +556,15 @@ const checkContextsPage = ( const pullRequestRefs = encodeJson({ base: { sha: "a1b2c3d" }, head: { sha: "b1c2d3e" } }); -const crossRepositoryDetail = (headRefOid = "abc123") => +const workflowApprovalDetail = (headRefOid = "abc123", isCrossRepository = true) => coreResponse({ headRefName: "feat/page", headRefOid, - isCrossRepository: true, - headRepositoryOwner: { __typename: "User", login: "octocat" }, + isCrossRepository, + headRepositoryOwner: { __typename: "User", login: isCrossRepository ? "octocat" : "acme" }, }); -const heads = (numbers: ReadonlyArray) => ({ +const heads = (numbers: ReadonlyArray, isCrossRepository = true) => ({ data: { repository: { pullRequests: { @@ -572,8 +572,11 @@ const heads = (numbers: ReadonlyArray) => ({ nodes: numbers.map((number) => ({ number, headRefOid: "abc123", - isCrossRepository: true, - headRepositoryOwner: { __typename: "User", login: "octocat" }, + isCrossRepository, + headRepositoryOwner: { + __typename: "User", + login: isCrossRepository ? "octocat" : "acme", + }, })), }, }, @@ -585,6 +588,7 @@ const workflowRuns = (ids: ReadonlyArray) => ({ id, name: id === 10 ? "build" : `run ${id}`, html_url: `https://example.com/${id}`, + pull_requests: [{ number: id === 12 ? 8 : 7 }], })), }); @@ -2241,47 +2245,84 @@ layer("GitHubPullRequestApi.layer", (it) => { }), ); - it.effect("does not approve action-required runs for a same-repository pull request", () => - Effect.gen(function* () { - mockedExecute.mockReturnValueOnce( - Effect.succeed( - output( - encodeJson( - coreResponse({ - number: 7, - title: "Pull request 7", - url: "https://github.com/acme/web/pull/7", - headRefName: "feat/page", - headRefOid: "abc123", - isCrossRepository: false, - headRepositoryOwner: { __typename: "Organization", login: "acme" }, - baseRefName: "main", - createdAt: "2026-07-01T00:00:00Z", - updatedAt: "2026-07-02T00:00:00Z", - }), - ), - ), - ), - ); - const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; + it.effect.each([true, false])( + "finds and approves waiting workflows (fork: %s)", + (isCrossRepository) => + Effect.gen(function* () { + workflowApprovalRoutes( + () => workflowApprovalDetail("abc123", isCrossRepository), + heads([7], isCrossRepository), + workflowRuns([10, 11, 12]), + ); + const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; - yield* cli.runPullRequestAction({ - cwd: "/w", - repository: "acme/web", - host: "github.com", - number: 7, - action: "approve-workflows", - }); + yield* cli.runPullRequestAction({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + action: "approve-workflows", + }); - expect(mockedExecute).toHaveBeenCalledTimes(1); - }), + expect(variablesOf("PullRequestHeads")[0]).toEqual({ + owner: "acme", + name: "web", + head: "feat/page", + }); + expect(restCallsTo("actions/runs?")[0]?.path).toBe( + "repos/acme/web/actions/runs?head_sha=abc123&branch=feat%2Fpage&event=pull_request&status=action_required&per_page=100&page=1", + ); + expect(restCallsTo("/approve").map((call) => [call.method, call.path])).toEqual([ + ["POST", "repos/acme/web/actions/runs/10/approve"], + ["POST", "repos/acme/web/actions/runs/11/approve"], + ]); + assert.strictEqual(restCallsTo("actions/runs?").length, 3); + }), ); - it.effect("finds and approves every workflow waiting on a maintainer", () => + it.effect.each([ + { label: "empty", pullRequests: [] }, + { label: "missing", pullRequests: undefined }, + ])("refuses approval when workflow PR association is $label", ({ pullRequests }) => Effect.gen(function* () { - workflowApprovalRoutes(() => crossRepositoryDetail(), heads([7]), workflowRuns([10, 11])); + workflowApprovalRoutes(() => workflowApprovalDetail(), heads([7]), { + workflow_runs: [{ ...workflowRuns([10]).workflow_runs[0], pull_requests: pullRequests }], + }); const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; + const result = yield* Effect.result( + cli.runPullRequestAction({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + action: "approve-workflows", + }), + ); + expect(result._tag).toBe("Failure"); + expect(restCallsTo("/approve")).toEqual([]); + }), + ); + it.effect("rechecks workflow PR association before approving", () => + Effect.gen(function* () { + let runReads = 0; + mockedExecute.mockImplementation((call) => { + const answer = + call.kind === "rest" + ? { + workflow_runs: [ + { + ...workflowRuns([10]).workflow_runs[0], + pull_requests: [{ number: ++runReads === 1 ? 7 : 8 }], + }, + ], + } + : call.operationName === "PullRequestHeads" + ? heads([7]) + : workflowApprovalDetail(); + return Effect.succeed(output(encodeJson(answer))); + }); + const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; yield* cli.runPullRequestAction({ cwd: "/w", repository: "acme/web", @@ -2289,26 +2330,14 @@ layer("GitHubPullRequestApi.layer", (it) => { number: 7, action: "approve-workflows", }); - - expect(variablesOf("PullRequestHeads")[0]).toEqual({ - owner: "acme", - name: "web", - head: "feat/page", - }); - expect(restCallsTo("actions/runs?")[0]?.path).toBe( - "repos/acme/web/actions/runs?head_sha=abc123&branch=feat%2Fpage&event=pull_request&status=action_required&per_page=100&page=1", - ); - expect(restCallsTo("/approve").map((call) => [call.method, call.path])).toEqual([ - ["POST", "repos/acme/web/actions/runs/10/approve"], - ["POST", "repos/acme/web/actions/runs/11/approve"], - ]); - assert.strictEqual(restCallsTo("actions/runs?").length, 3); + expect(runReads).toBe(2); + expect(restCallsTo("/approve")).toEqual([]); }), ); it.effect("counts revalidated runs that wait on a maintainer as GitHub reports them", () => Effect.gen(function* () { - workflowApprovalRoutes(() => crossRepositoryDetail(), heads([7]), workflowRuns([])); + workflowApprovalRoutes(() => workflowApprovalDetail(), heads([7]), workflowRuns([])); const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; const runs = yield* cli @@ -2331,6 +2360,14 @@ layer("GitHubPullRequestApi.layer", (it) => { status: "completed", conclusion: "action_required", head_branch: "feat/page", + pull_requests: [{ number: 7 }], + }, + { + id: 12, + status: "completed", + conclusion: "action_required", + head_branch: "feat/page", + pull_requests: [{ number: 8 }], }, { id: 11, status: "completed", conclusion: "success", head_branch: "feat/page" }, ], @@ -2342,33 +2379,36 @@ layer("GitHubPullRequestApi.layer", (it) => { }), ); - it.effect("refuses a stale workflow approval after the pull request head changes", () => - Effect.gen(function* () { - let detailReads = 0; - workflowApprovalRoutes( - () => crossRepositoryDetail(++detailReads === 1 ? "abc123" : "def456"), - heads([7]), - workflowRuns([10]), - ); - const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; + it.effect.each([true, false])( + "refuses a stale workflow approval after the head changes (fork: %s)", + (isCrossRepository) => + Effect.gen(function* () { + let detailReads = 0; + workflowApprovalRoutes( + () => + workflowApprovalDetail(++detailReads === 1 ? "abc123" : "def456", isCrossRepository), + heads([7], isCrossRepository), + workflowRuns([10]), + ); + const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; - const error = yield* Effect.flip( - cli.runPullRequestAction({ - cwd: "/w", - repository: "acme/web", - host: "github.com", - number: 7, - action: "approve-workflows", - }), - ); + const error = yield* Effect.flip( + cli.runPullRequestAction({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + action: "approve-workflows", + }), + ); - expect(error).toMatchObject({ - _tag: "GitHubWorkflowApprovalHeadChangedError", - number: 7, - }); - assert.strictEqual(detailReads, 2); - expect(restCallsTo("/approve")).toEqual([]); - }), + expect(error).toMatchObject({ + _tag: "GitHubWorkflowApprovalHeadChangedError", + number: 7, + }); + assert.strictEqual(detailReads, 2); + expect(restCallsTo("/approve")).toEqual([]); + }), ); it.effect("reads workflow runs and their pull request scope concurrently", () => @@ -2399,37 +2439,45 @@ layer("GitHubPullRequestApi.layer", (it) => { isCrossRepository: true, }); - expect(runs).toEqual([{ id: 10, name: "build", url: "https://example.com/10" }]); + expect(runs).toEqual([ + { pullRequestNumbers: [7], id: 10, name: "build", url: "https://example.com/10" }, + ]); }), ); - it.effect("refuses workflow approval when one head belongs to several pull requests", () => - Effect.gen(function* () { - workflowApprovalRoutes(() => crossRepositoryDetail(), heads([7, 8]), workflowRuns([])); - const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; + it.effect.each([true, false])( + "refuses ambiguous workflow approval (fork: %s)", + (isCrossRepository) => + Effect.gen(function* () { + workflowApprovalRoutes( + () => workflowApprovalDetail("abc123", isCrossRepository), + heads([7, 8], isCrossRepository), + workflowRuns([]), + ); + const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; - const error = yield* Effect.flip( - cli.listWorkflowRunsRequiringApproval({ - cwd: "/w", - repository: "acme/web", - host: "github.com", - number: 7, - headSha: "abc123", - headBranch: "feat/page", - headRepositoryOwner: "octocat", - isCrossRepository: true, - }), - ); + const error = yield* Effect.flip( + cli.listWorkflowRunsRequiringApproval({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + headSha: "abc123", + headBranch: "feat/page", + headRepositoryOwner: isCrossRepository ? "octocat" : "acme", + isCrossRepository, + }), + ); - expect(error).toMatchObject({ - _tag: "GitHubWorkflowApprovalRefusedError", - reason: "head-not-unique", - number: 7, - observedCount: 2, - limit: 1_000, - }); - expect(error.message).toContain("instead of uniquely matching #7"); - }), + expect(error).toMatchObject({ + _tag: "GitHubWorkflowApprovalRefusedError", + reason: "head-not-unique", + number: 7, + observedCount: 2, + limit: 1_000, + }); + expect(error.message).toContain("instead of uniquely matching #7"); + }), ); it.effect("refuses workflow approval when the head list stops before its end", () => @@ -2439,7 +2487,7 @@ layer("GitHubPullRequestApi.layer", (it) => { hasNextPage: true, endCursor: "same", } as never; - workflowApprovalRoutes(() => crossRepositoryDetail(), repeating, workflowRuns([])); + workflowApprovalRoutes(() => workflowApprovalDetail(), repeating, workflowRuns([])); const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; const error = yield* Effect.flip( diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.ts index 4b7014a89074..4b2b93e0803c 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.ts @@ -501,7 +501,7 @@ export class GitHubPullRequestApi extends Context.Service< readonly headSha: string; readonly headBranch: string; readonly headRepositoryOwner: string; - readonly isCrossRepository: true; + readonly isCrossRepository: boolean; }) => Effect.Effect, GitHubPullRequestApiError>; readonly getPullRequestStack: (input: { @@ -1491,6 +1491,7 @@ export const make = Effect.gen(function* () { run.head_branch === input.headBranch ? [ { + pullRequestNumbers: run.pull_requests?.map((pr) => pr.number) ?? [], id: run.id, name: run.name?.trim() || `Workflow run ${run.id}`, url: run.html_url?.trim() || null, @@ -1532,7 +1533,7 @@ export const make = Effect.gen(function* () { const exactHeads = heads.filter( (pullRequest) => pullRequest.headSha === input.headSha && - pullRequest.isCrossRepository === true && + pullRequest.isCrossRepository === input.isCrossRepository && pullRequest.headRepositoryOwner?.toLowerCase() === input.headRepositoryOwner.toLowerCase(), ); @@ -1577,7 +1578,18 @@ export const make = Effect.gen(function* () { ), ], { concurrency: 2 }, - ).pipe(Effect.map(([, runs]) => runs)); + ).pipe( + Effect.flatMap(([, runs]) => + runs.some((run) => run.pullRequestNumbers.length === 0) + ? Effect.fail( + workflowApprovalReadError( + input.cwd, + new Error("GitHub omitted workflow run pull-request associations."), + ), + ) + : Effect.succeed(runs.filter((run) => run.pullRequestNumbers.includes(input.number))), + ), + ); const simpleActionMutations = { "disable-auto-merge": (pullRequestId: string) => @@ -2421,7 +2433,6 @@ export const make = Effect.gen(function* () { const { owner, name } = parseRepositorySelector(input.repository); return getPullRequestDetail(input).pipe( Effect.flatMap((detail) => { - if (detail.isCrossRepository !== true) return Effect.void; if (detail.headSha == null || detail.headRepositoryOwner == null) { return Effect.fail( new GitHubWorkflowApprovalHeadUnavailableError({ @@ -2438,7 +2449,7 @@ export const make = Effect.gen(function* () { headSha: expectedHeadSha, headBranch: expectedHeadBranch, headRepositoryOwner: expectedHeadRepositoryOwner, - isCrossRepository: true, + isCrossRepository: detail.isCrossRepository === true, }).pipe( Effect.flatMap((runs) => Effect.forEach( @@ -2455,7 +2466,7 @@ export const make = Effect.gen(function* () { ); } if ( - current.isCrossRepository !== true || + current.isCrossRepository !== detail.isCrossRepository || current.headSha !== expectedHeadSha || current.headBranch !== expectedHeadBranch || current.headRepositoryOwner.toLowerCase() !== @@ -2473,7 +2484,7 @@ export const make = Effect.gen(function* () { headSha: current.headSha, headBranch: current.headBranch, headRepositoryOwner: current.headRepositoryOwner, - isCrossRepository: true, + isCrossRepository: detail.isCrossRepository === true, }); }), Effect.flatMap((currentRuns) => diff --git a/packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts b/packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts index 81623592066e..c495723f95ff 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts @@ -376,6 +376,7 @@ describe("gitHubViewerPermissions", () => { listWorkflowRunsRequiringApproval: () => Effect.succeed([ { + pullRequestNumbers: [7], id: 123, name: "contributor tests", url: "https://github.com/acme/web/actions/runs/123", @@ -428,7 +429,7 @@ const openDetail = { commits: [], }; -it.effect("does not classify same-repository gates as fork workflow approvals", () => +it.effect("discovers workflow approval for a same-repository bot pull request", () => Effect.gen(function* () { const provider = yield* make; const detail = yield* provider.getChangeRequest({ @@ -438,15 +439,29 @@ it.effect("does not classify same-repository gates as fork workflow approvals", number: 7, }); - expect(detail.workflowApprovalsRequired).toBe(0); - expect(detail.checks).toEqual([]); + expect(detail.workflowApprovalsRequired).toBe(1); + expect(detail.checks).toEqual([ + { + name: "bot tests", + status: "action-required", + description: "A maintainer must approve this workflow before it can run.", + url: "https://github.com/acme/web/actions/runs/123", + }, + ]); }).pipe( Effect.provide( Layer.mock(GitHubPullRequestApi.GitHubPullRequestApi)({ revalidateChecks: (_input, read) => read, getPullRequestDetail: () => Effect.succeed({ ...openDetail, isCrossRepository: false }), listWorkflowRunsRequiringApproval: () => - Effect.die("same-repository pull requests must not probe fork workflow approvals"), + Effect.succeed([ + { + pullRequestNumbers: [7], + id: 123, + name: "bot tests", + url: "https://github.com/acme/web/actions/runs/123", + }, + ]), getViewerAccess: () => Effect.succeed({ canWrite: true, diff --git a/packages/source-control-github/src/server/GitHubPullRequestProvider.ts b/packages/source-control-github/src/server/GitHubPullRequestProvider.ts index f90941bf882e..cef30a40b29b 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestProvider.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestProvider.ts @@ -283,7 +283,7 @@ export const make = Effect.gen(function* () { const readChecks = (input: ProviderRepositoryRef & { readonly number: number }) => cli.getPullRequestDetail(input).pipe( Effect.flatMap((pullRequest) => - (pullRequest.state !== "open" || pullRequest.isCrossRepository !== true + (pullRequest.state !== "open" ? Effect.succeed({ runs: [] as ReadonlyArray, unavailable: false, @@ -299,7 +299,7 @@ export const make = Effect.gen(function* () { headSha: pullRequest.headSha, headBranch: pullRequest.headBranch, headRepositoryOwner: pullRequest.headRepositoryOwner, - isCrossRepository: true, + isCrossRepository: pullRequest.isCrossRepository === true, }) .pipe( Effect.matchEffect({ diff --git a/packages/source-control-github/src/server/gitHubConditionalChecks.ts b/packages/source-control-github/src/server/gitHubConditionalChecks.ts index e41747b15a2f..513023bb190c 100644 --- a/packages/source-control-github/src/server/gitHubConditionalChecks.ts +++ b/packages/source-control-github/src/server/gitHubConditionalChecks.ts @@ -26,6 +26,7 @@ const decodeHead = Schema.decodeUnknownEffect(Schema.fromJsonString(HeadSchema)) class ChecksUnavailable extends Schema.TaggedError()("ChecksUnavailable", {}) {} const WorkflowRunSchema = Schema.Struct({ + pull_requests: Schema.optional(Schema.Array(Schema.Struct({ number: Schema.Int }))), id: Schema.Int, name: Schema.optional(Schema.NullOr(Schema.String)), html_url: Schema.optional(Schema.NullOr(Schema.String)), diff --git a/packages/source-control-github/src/server/gitHubPullRequestJson.ts b/packages/source-control-github/src/server/gitHubPullRequestJson.ts index 965855a4350a..5fd0441c81d0 100644 --- a/packages/source-control-github/src/server/gitHubPullRequestJson.ts +++ b/packages/source-control-github/src/server/gitHubPullRequestJson.ts @@ -205,6 +205,7 @@ const RawWorkflowRunsSchema = Schema.Struct({ workflow_runs: Schema.Array( Schema.Struct({ id: Schema.Int, + pull_requests: Schema.optional(Schema.Array(Schema.Struct({ number: Schema.Int }))), name: Schema.optional(Schema.NullOr(Schema.String)), html_url: Schema.optional(Schema.NullOr(Schema.String)), }), @@ -359,6 +360,7 @@ export interface GitHubPullRequestDetail extends GitHubPullRequestListItem { } export interface GitHubWorkflowRunApproval { + readonly pullRequestNumbers: ReadonlyArray; readonly id: number; readonly name: string; readonly url: string | null; @@ -1059,6 +1061,7 @@ export function decodeWorkflowRunsJson( if (!Result.isSuccess(decoded)) return Result.fail(decoded.failure); return Result.succeed({ runs: decoded.success.workflow_runs.map((run) => ({ + pullRequestNumbers: run.pull_requests?.map((pr) => pr.number) ?? [], id: run.id, name: trimmed(run.name) ?? `Workflow run ${run.id}`, url: trimmed(run.html_url),