From 67010d5279d265e9164d51959cd1f7a31a3f8ee8 Mon Sep 17 00:00:00 2001 From: Stephen Belanger Date: Thu, 29 Oct 2026 00:34:32 +0800 Subject: [PATCH 1/3] fix(github): approve workflows on same-repository PRs --- .../src/server/GitHubPullRequestApi.test.ts | 220 ++++++++---------- .../src/server/GitHubPullRequestApi.ts | 11 +- .../server/GitHubPullRequestProvider.test.ts | 17 +- .../src/server/GitHubPullRequestProvider.ts | 4 +- 4 files changed, 121 insertions(+), 131 deletions(-) diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts index 3fd74e08130e..b98306cc538d 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", + }, })), }, }, @@ -2241,74 +2244,44 @@ 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; - - yield* cli.runPullRequestAction({ - cwd: "/w", - repository: "acme/web", - host: "github.com", - number: 7, - action: "approve-workflows", - }); - - expect(mockedExecute).toHaveBeenCalledTimes(1); - }), - ); - - it.effect("finds and approves every workflow waiting on a maintainer", () => - Effect.gen(function* () { - workflowApprovalRoutes(() => crossRepositoryDetail(), heads([7]), workflowRuns([10, 11])); - 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]), + ); + 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(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(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("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 @@ -2342,33 +2315,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", () => @@ -2403,33 +2379,39 @@ layer("GitHubPullRequestApi.layer", (it) => { }), ); - 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 +2421,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..8ed73732e965 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: { @@ -1532,7 +1532,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(), ); @@ -2421,7 +2421,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 +2437,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 +2454,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 +2472,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..04e156533d27 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts @@ -428,7 +428,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 +438,24 @@ 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([ + { 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({ From 961d1b031ba30d1942640d1998bc591734b00c6b Mon Sep 17 00:00:00 2001 From: Stephen Belanger Date: Thu, 29 Oct 2026 00:46:50 +0800 Subject: [PATCH 2/3] fix(github): scope workflow approvals to associated PRs --- .../src/server/GitHubPullRequestApi.test.ts | 70 ++++++++++++++++++- .../src/server/GitHubPullRequestApi.ts | 14 +++- .../server/GitHubPullRequestProvider.test.ts | 8 ++- .../src/server/gitHubConditionalChecks.ts | 1 + .../src/server/gitHubPullRequestJson.ts | 3 + 5 files changed, 92 insertions(+), 4 deletions(-) diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts index b98306cc538d..66e6b5c5a0bb 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts @@ -588,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 }], })), }); @@ -2251,7 +2252,7 @@ layer("GitHubPullRequestApi.layer", (it) => { workflowApprovalRoutes( () => workflowApprovalDetail("abc123", isCrossRepository), heads([7], isCrossRepository), - workflowRuns([10, 11]), + workflowRuns([10, 11, 12]), ); const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; @@ -2279,6 +2280,61 @@ layer("GitHubPullRequestApi.layer", (it) => { }), ); + it.effect.each([ + { label: "empty", pullRequests: [] }, + { label: "missing", pullRequests: undefined }, + ])("refuses approval when workflow PR association is $label", ({ pullRequests }) => + Effect.gen(function* () { + 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", + host: "github.com", + number: 7, + action: "approve-workflows", + }); + 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(() => workflowApprovalDetail(), heads([7]), workflowRuns([])); @@ -2304,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" }, ], @@ -2375,7 +2439,9 @@ 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" }, + ]); }), ); diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.ts index 8ed73732e965..4b2b93e0803c 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.ts @@ -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, @@ -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) => diff --git a/packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts b/packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts index 04e156533d27..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", @@ -454,7 +455,12 @@ it.effect("discovers workflow approval for a same-repository bot pull request", getPullRequestDetail: () => Effect.succeed({ ...openDetail, isCrossRepository: false }), listWorkflowRunsRequiringApproval: () => Effect.succeed([ - { id: 123, name: "bot tests", url: "https://github.com/acme/web/actions/runs/123" }, + { + pullRequestNumbers: [7], + id: 123, + name: "bot tests", + url: "https://github.com/acme/web/actions/runs/123", + }, ]), getViewerAccess: () => Effect.succeed({ 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), From 986f45c77899bd1ff20ea75cbc7ad2fc09e73cb6 Mon Sep 17 00:00:00 2001 From: maria-rcks Date: Sun, 1 Nov 2026 07:58:31 +0000 Subject: [PATCH 3/3] fix(github): preserve fork approvals and revalidate bot workflows --- .../src/server/GitHubPullRequestApi.test.ts | 72 ++++++++++++++++--- .../src/server/GitHubPullRequestApi.ts | 10 ++- .../src/server/gitHubConditionalChecks.ts | 24 +++---- 3 files changed, 81 insertions(+), 25 deletions(-) diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts index bf4d4df357d8..40ef949ede7a 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts @@ -21,7 +21,7 @@ import * as SourceControlRateLimit from "@t3tools/source-control-core/server/Sou import * as TestSourceControlHost from "@t3tools/source-control-testing/TestSourceControlHost"; import * as GitHubPullRequestApi from "./GitHubPullRequestApi.ts"; import * as GitHubPullRequestProvider from "./GitHubPullRequestProvider.ts"; -import { KnownWorkflowRuns } from "./gitHubConditionalChecks.ts"; +import { KnownWorkflowRuns, makeChecksRevalidator } from "./gitHubConditionalChecks.ts"; import * as GitHubAppAccess from "./GitHubAppAccess.ts"; import * as GitHubAppSignIn from "./GitHubAppSignIn.ts"; @@ -2416,13 +2416,19 @@ layer("GitHubPullRequestApi.layer", (it) => { ); it.effect.each([ - { label: "empty", pullRequests: [] }, - { label: "missing", pullRequests: undefined }, - ])("refuses approval when workflow PR association is $label", ({ pullRequests }) => + { label: "empty fork", pullRequests: [], isCrossRepository: true }, + { label: "empty same repository", pullRequests: [], isCrossRepository: false }, + { label: "missing fork", pullRequests: undefined, isCrossRepository: true }, + { label: "missing same repository", pullRequests: undefined, isCrossRepository: false }, + ])("handles $label workflow PR associations", ({ pullRequests, isCrossRepository }) => Effect.gen(function* () { - workflowApprovalRoutes(() => workflowApprovalDetail(), heads([7]), { - workflow_runs: [{ ...workflowRuns([10]).workflow_runs[0], pull_requests: pullRequests }], - }); + workflowApprovalRoutes( + () => workflowApprovalDetail("abc123", isCrossRepository), + heads([7], isCrossRepository), + { + workflow_runs: [{ ...workflowRuns([10]).workflow_runs[0], pull_requests: pullRequests }], + }, + ); const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; const result = yield* Effect.result( cli.runPullRequestAction({ @@ -2433,8 +2439,10 @@ layer("GitHubPullRequestApi.layer", (it) => { action: "approve-workflows", }), ); - expect(result._tag).toBe("Failure"); - expect(restCallsTo("/approve")).toEqual([]); + expect(result._tag).toBe(isCrossRepository ? "Success" : "Failure"); + expect(restCallsTo("/approve").map((call) => call.path)).toEqual( + isCrossRepository ? ["repos/acme/web/actions/runs/10/approve"] : [], + ); }), ); @@ -2470,6 +2478,52 @@ layer("GitHubPullRequestApi.layer", (it) => { }), ); + it.effect("revalidates same-repository workflow changes without a new head", () => + Effect.gen(function* () { + const sha = "a".repeat(40); + let runsChanged = false; + let reads = 0; + mockedExecute.mockImplementation((call) => { + if (call.kind !== "rest") return Effect.die("Expected REST"); + const isRuns = call.path.includes("actions/runs?"); + const changed = isRuns && runsChanged; + if (call.ifNoneMatch && !changed) return Effect.succeed(output("", false, false, {}, 304)); + const body = call.path.endsWith("/pulls/7") + ? { head: { sha, repo: { id: 1 } }, base: { repo: { id: 1 } } } + : isRuns + ? { workflow_runs: changed ? workflowRuns([10]).workflow_runs : [] } + : {}; + return Effect.succeed( + output(encodeJson(body), false, false, { etag: changed ? "new" : "old" }), + ); + }); + const revalidate = yield* makeChecksRevalidator; + const read = Effect.gen(function* () { + reads++; + const known = yield* KnownWorkflowRuns; + expect(known?.runs.length ?? 0).toBe(runsChanged ? 1 : 0); + return { + state: "open" as const, + checks: [], + headSha: sha, + workflowApprovalsRequired: known?.runs.length ?? 0, + }; + }); + yield* revalidate(pullRequestInput, read); + yield* revalidate(pullRequestInput, read); + expect(reads).toBe(1); + runsChanged = true; + yield* revalidate(pullRequestInput, read); + expect(reads).toBe(2); + }).pipe( + Effect.provideService(GitHubRequestPolicy.PinnedGitHubCredential, { + host: "github.com", + token: Redacted.make("token"), + credentialFingerprint: "same-repository-cache", + }), + ), + ); + it.effect("counts revalidated runs that wait on a maintainer as GitHub reports them", () => Effect.gen(function* () { workflowApprovalRoutes(() => workflowApprovalDetail(), heads([7]), workflowRuns([])); diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.ts index faf4e18b8873..09e4fbba4dd2 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.ts @@ -1611,14 +1611,20 @@ export const make = Effect.gen(function* () { { concurrency: 2 }, ).pipe( Effect.flatMap(([, runs]) => - runs.some((run) => run.pullRequestNumbers.length === 0) + !input.isCrossRepository && 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))), + : Effect.succeed( + runs.filter((run) => + run.pullRequestNumbers.length === 0 + ? input.isCrossRepository + : run.pullRequestNumbers.includes(input.number), + ), + ), ), ); diff --git a/packages/source-control-github/src/server/gitHubConditionalChecks.ts b/packages/source-control-github/src/server/gitHubConditionalChecks.ts index 513023bb190c..3708745ebdeb 100644 --- a/packages/source-control-github/src/server/gitHubConditionalChecks.ts +++ b/packages/source-control-github/src/server/gitHubConditionalChecks.ts @@ -142,14 +142,11 @@ export const makeChecksRevalidator = Effect.gen(function* () { if ((yield* get(`${root}/pulls/${input.number}`, true)) === null) return yield* read; const head = entry.head; if (head === null) return yield* fail(200); - const runsEndpoint = - head.head.repo?.id !== head.base.repo.id - ? `${root}/actions/runs?head_sha=${head.head.sha}&event=pull_request` - : null; + const runsEndpoint = `${root}/actions/runs?head_sha=${head.head.sha}&event=pull_request`; const endpoints = [ `${root}/commits/${head.head.sha}/check-runs?filter=all`, `${root}/commits/${head.head.sha}/status`, - ...(runsEndpoint === null ? [] : [runsEndpoint]), + runsEndpoint, ]; const runPages: Array = []; // Each endpoint has its own cursor; only pages within one endpoint depend on each other. @@ -177,15 +174,14 @@ export const makeChecksRevalidator = Effect.gen(function* () { const pages = runPages.map((endpoint) => decodeWorkflowRuns(entry.validators.get(endpoint)?.body ?? ""), ); - const known = - runsEndpoint !== null && pages.every(Option.isSome) - ? { - headSha: head.head.sha, - runs: pages.flatMap((page) => - Option.isSome(page) ? page.value.workflow_runs : [], - ), - } - : null; + const known = pages.every(Option.isSome) + ? { + headSha: head.head.sha, + runs: pages.flatMap((page) => + Option.isSome(page) ? page.value.workflow_runs : [], + ), + } + : null; const fresh = yield* read.pipe(Effect.provideService(KnownWorkflowRuns, known)); const value = { state: fresh.state,