From 2bbeef80093d1c2dc860c1d9060f22da3170114e Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 22:53:23 +0000 Subject: [PATCH 1/4] fix: create API Review check on fork PRs Cation is not installed on contributor forks, so writing the API Review and Deprecation Review checks to the head repo meant they were skipped entirely for fork PRs. Write them to the base repo against the head SHA instead, matching how Semver Label Enforcement already works. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01U4dtTVsf8UKZDqhP8BCv1c --- spec/api-review-state.spec.ts | 77 ++++++++++++++++++- spec/deprecation-review-state.spec.ts | 26 ++++++- .../pull_request.api-skip-review_label.json | 1 + .../pull_request.approved_review_label.json | 1 + .../pull_request.declined_review_label.json | 1 + ...pull_request.labeled.exclusion_labels.json | 1 + .../pull_request.requested_review_label.json | 1 + src/api-review-state.ts | 51 ++++-------- src/deprecation-review-state.ts | 42 ++-------- 9 files changed, 124 insertions(+), 77 deletions(-) diff --git a/spec/api-review-state.spec.ts b/spec/api-review-state.spec.ts index db1ead3..f5409a4 100644 --- a/spec/api-review-state.spec.ts +++ b/spec/api-review-state.spec.ts @@ -194,10 +194,27 @@ describe('api review', () => { expect(moctokit.rest.checks.update).toHaveBeenCalled(); }); - it('should do nothing when the PR does not have an API review label on a fork PR', async () => { + it('should reset the check on the base repo when the PR does not have an API review label on a fork PR', async () => { let { pull_request } = loadFixture('api-review-state/pull_request.no_review_label.json'); - pull_request.fork = true; + pull_request.head.repo = { + ...pull_request.head.repo, + name: 'electron-fork', + full_name: 'contributor/electron-fork', + owner: { login: 'contributor' }, + fork: true, + }; + + moctokit.rest.checks.listForRef = vi.fn().mockReturnValue({ + data: { + check_runs: [ + { + name: API_REVIEW_CHECK_NAME, + id: '12345', + }, + ], + }, + }); const users = await addOrUpdateAPIReviewCheck(moctokit, pull_request); expect(users).toEqual(undefined); @@ -205,8 +222,12 @@ describe('api review', () => { expect(moctokit.rest.issues.addLabels).not.toHaveBeenCalled(); expect(moctokit.rest.issues.removeLabel).not.toHaveBeenCalled(); - expect(moctokit.rest.checks.listForRef).toHaveBeenCalled(); - expect(moctokit.rest.checks.update).not.toHaveBeenCalled(); + expect(moctokit.rest.checks.listForRef).toHaveBeenCalledWith( + expect.objectContaining({ owner: 'electron', repo: 'electron', ref: pull_request.head.sha }), + ); + expect(moctokit.rest.checks.update).toHaveBeenCalledWith( + expect.objectContaining({ owner: 'electron', repo: 'electron', check_run_id: '12345' }), + ); }); it(`should correctly parse user approval status for ${REVIEW_LABELS.REQUESTED} label`, async () => { @@ -875,6 +896,10 @@ describe('api review', () => { .get(`/orgs/electron/teams/${API_WORKING_GROUP}/members`) .reply(200, API_WORKING_GROUP_MEMBERS); + nock(GH_API) + .get(`/repos/electron/electron/commits/${pull_request.head.sha}/check-runs?per_page=100`) + .reply(200, { check_runs: [] }); + const r1 = loadFixture('api-review-state/pull_request_review/fork/review_lgtm.json'); nock(GH_API) .get(`/repos/electron/electron/pulls/${pull_request.number}/reviews`) @@ -887,8 +912,20 @@ describe('api review', () => { nock(GH_API) .get(`/repos/electron/electron/issues/${pull_request.number}/timeline`) + .times(2) .reply(200, []); + nock(GH_API) + .post('/repos/electron/electron/check-runs', (body) => { + expect(body).toMatchObject({ + name: API_REVIEW_CHECK_NAME, + head_sha: pull_request.head.sha, + status: 'in_progress', + }); + return true; + }) + .reply(200); + await robot.receive({ id: '123-456', name: 'pull_request_review', @@ -904,6 +941,10 @@ describe('api review', () => { .get(`/orgs/electron/teams/${API_WORKING_GROUP}/members`) .reply(200, API_WORKING_GROUP_MEMBERS); + nock(GH_API) + .get(`/repos/electron/electron/commits/${pull_request.head.sha}/check-runs?per_page=100`) + .reply(200, { check_runs: [] }); + const r1 = loadFixture('api-review-state/pull_request_review/fork/review_lgtm.json'); const r2 = loadFixture('api-review-state/pull_request_review/fork/review_lgtm_2.json'); nock(GH_API) @@ -917,8 +958,20 @@ describe('api review', () => { nock(GH_API) .get(`/repos/electron/electron/issues/${pull_request.number}/timeline`) + .times(2) .reply(200, []); + nock(GH_API) + .post('/repos/electron/electron/check-runs', (body) => { + expect(body).toMatchObject({ + name: API_REVIEW_CHECK_NAME, + head_sha: pull_request.head.sha, + status: 'in_progress', + }); + return true; + }) + .reply(200); + nock(GH_API) .get(`/repos/electron/electron/issues/${pull_request.number}/labels?per_page=100&page=1`) .reply(200, [ @@ -974,6 +1027,10 @@ describe('api review', () => { .get(`/orgs/electron/teams/${API_WORKING_GROUP}/members`) .reply(200, API_WORKING_GROUP_MEMBERS); + nock(GH_API) + .get(`/repos/electron/electron/commits/${pull_request.head.sha}/check-runs?per_page=100`) + .reply(200, { check_runs: [] }); + const r1 = loadFixture('api-review-state/pull_request_review/fork/review_declined.json'); nock(GH_API) .get(`/repos/electron/electron/pulls/${pull_request.number}/reviews`) @@ -985,8 +1042,20 @@ describe('api review', () => { nock(GH_API) .get(`/repos/electron/electron/issues/${pull_request.number}/timeline`) + .times(2) .reply(200, []); + nock(GH_API) + .post('/repos/electron/electron/check-runs', (body) => { + expect(body).toMatchObject({ + name: API_REVIEW_CHECK_NAME, + head_sha: pull_request.head.sha, + status: 'in_progress', + }); + return true; + }) + .reply(200); + nock(GH_API) .get(`/repos/electron/electron/issues/${pull_request.number}/labels?per_page=100&page=1`) .reply(200, [ diff --git a/spec/deprecation-review-state.spec.ts b/spec/deprecation-review-state.spec.ts index 6aa9a1f..caaf832 100644 --- a/spec/deprecation-review-state.spec.ts +++ b/spec/deprecation-review-state.spec.ts @@ -164,15 +164,35 @@ describe('deprecation review', () => { expect(moctokit.rest.checks.create).toHaveBeenCalledWith(expect.objectContaining(expected)); }); - it('should not use Checks API when the PR is from a fork', async () => { + it('should create the check on the base repo when the PR is from a fork', async () => { const { pull_request } = loadFixture( 'deprecation-review-state/pull_request.requested_review_label.json', ); - pull_request.head.repo.fork = true; + pull_request.head.repo = { + ...pull_request.head.repo, + name: 'deprecation-review-fork', + full_name: 'contributor/deprecation-review-fork', + owner: { login: 'contributor' }, + fork: true, + }; await addOrUpdateDeprecationReviewCheck(moctokit, pull_request); - expect(moctokit.rest.checks.create).not.toHaveBeenCalled(); + expect(moctokit.rest.checks.listForRef).toHaveBeenCalledWith( + expect.objectContaining({ + owner: 'dsanders11', + repo: 'deprecation-review', + ref: pull_request.head.sha, + }), + ); + expect(moctokit.rest.checks.create).toHaveBeenCalledWith( + expect.objectContaining({ + owner: 'dsanders11', + repo: 'deprecation-review', + name: DEPRECATION_REVIEW_CHECK_NAME, + head_sha: pull_request.head.sha, + }), + ); expect(moctokit.rest.checks.update).not.toHaveBeenCalled(); }); diff --git a/spec/fixtures/api-review-state/pull_request.api-skip-review_label.json b/spec/fixtures/api-review-state/pull_request.api-skip-review_label.json index c88d142..806a5df 100644 --- a/spec/fixtures/api-review-state/pull_request.api-skip-review_label.json +++ b/spec/fixtures/api-review-state/pull_request.api-skip-review_label.json @@ -36,6 +36,7 @@ }], "base": { "repo": { + "name": "electron", "owner": { "login": "electron" } diff --git a/spec/fixtures/api-review-state/pull_request.approved_review_label.json b/spec/fixtures/api-review-state/pull_request.approved_review_label.json index cda698a..a890169 100644 --- a/spec/fixtures/api-review-state/pull_request.approved_review_label.json +++ b/spec/fixtures/api-review-state/pull_request.approved_review_label.json @@ -45,6 +45,7 @@ ], "base": { "repo": { + "name": "electron", "owner": { "login": "electron" } diff --git a/spec/fixtures/api-review-state/pull_request.declined_review_label.json b/spec/fixtures/api-review-state/pull_request.declined_review_label.json index e4b1189..a16baa5 100644 --- a/spec/fixtures/api-review-state/pull_request.declined_review_label.json +++ b/spec/fixtures/api-review-state/pull_request.declined_review_label.json @@ -45,6 +45,7 @@ ], "base": { "repo": { + "name": "electron", "owner": { "login": "electron" } diff --git a/spec/fixtures/api-review-state/pull_request.labeled.exclusion_labels.json b/spec/fixtures/api-review-state/pull_request.labeled.exclusion_labels.json index 753d748..60ac8c4 100644 --- a/spec/fixtures/api-review-state/pull_request.labeled.exclusion_labels.json +++ b/spec/fixtures/api-review-state/pull_request.labeled.exclusion_labels.json @@ -35,6 +35,7 @@ ], "base": { "repo": { + "name": "electron", "owner": { "login": "electron" } diff --git a/spec/fixtures/api-review-state/pull_request.requested_review_label.json b/spec/fixtures/api-review-state/pull_request.requested_review_label.json index b5c2698..b03851f 100644 --- a/spec/fixtures/api-review-state/pull_request.requested_review_label.json +++ b/spec/fixtures/api-review-state/pull_request.requested_review_label.json @@ -45,6 +45,7 @@ ], "base": { "repo": { + "name": "electron", "full_name": "electron/electron", "owner": { "login": "electron" diff --git a/src/api-review-state.ts b/src/api-review-state.ts index c105d5c..9d8e0ed 100644 --- a/src/api-review-state.ts +++ b/src/api-review-state.ts @@ -104,38 +104,20 @@ export async function addOrUpdateAPIReviewCheck(octokit: Context['octokit'], pr: type CommentOrReview = ListReviewsItem & ListCommentsItem; - if (!pr.head.repo) { - log( - 'addOrUpdateAPIReviewCheck', - LogLevel.WARN, - `PR #${pr.number} does not have a head repo - cannot update check`, - ); - return; - } - - const fork = pr.head.repo.fork; + // Checks are always written to the base repo against the head SHA, so that + // they also work for PRs opened from forks (where Cation is not installed). const owner = pr.base.repo.owner.login; - const repo = pr.head.repo.name; - - if (fork) { - log( - 'addOrUpdateAPIReviewCheck', - LogLevel.INFO, - `${pr.number} is a fork - checks will not be created or updated`, - ); - } + const repo = pr.base.repo.name; // Fetch the latest API Review check for the PR. - const checkRun = fork - ? null - : ( - await octokit.rest.checks.listForRef({ - ref: pr.head.sha, - per_page: 100, - owner, - repo, - }) - ).data.check_runs.find((run) => run.name === API_REVIEW_CHECK_NAME); + const checkRun = ( + await octokit.rest.checks.listForRef({ + ref: pr.head.sha, + per_page: 100, + owner, + repo, + }) + ).data.check_runs.find((run) => run.name === API_REVIEW_CHECK_NAME); const resetToNeutral = async () => { if (!checkRun) return; @@ -284,9 +266,6 @@ export async function addOrUpdateAPIReviewCheck(octokit: Context['octokit'], pr: const users = { approved, declined, requestedChanges }; - // If the PR is a fork PR, return early as the Checks API doesn't work. - if (fork) return users; - // Update the GitHub Check with appropriate API review information. const updateCheck = async ( opts: Omit< @@ -299,16 +278,16 @@ export async function addOrUpdateAPIReviewCheck(octokit: Context['octokit'], pr: (checkRun.status === opts.status || !opts.status || opts.status === 'completed') ) { await octokit.rest.checks.update({ - owner: pr.head.repo!.owner.login, - repo: pr.head.repo!.name, + owner, + repo, name: API_REVIEW_CHECK_NAME, check_run_id: checkRun.id, ...opts, }); } else { await octokit.rest.checks.create({ - owner: pr.head.repo!.owner.login, - repo: pr.head.repo!.name, + owner, + repo, name: API_REVIEW_CHECK_NAME, head_sha: pr.head.sha, ...opts, diff --git a/src/deprecation-review-state.ts b/src/deprecation-review-state.ts index 6dfa372..48a3fc9 100644 --- a/src/deprecation-review-state.ts +++ b/src/deprecation-review-state.ts @@ -32,27 +32,10 @@ export async function addOrUpdateDeprecationReviewCheck(octokit: ProbotOctokit, `Validating ${pr.number} by ${pr.user.login}`, ); - if (!pr.head.repo) { - log( - 'addOrUpdateDeprecationReviewCheck', - LogLevel.WARN, - `PR #${pr.number} does not have a head repo - cannot update check`, - ); - return; - } - + // Checks are always written to the base repo against the head SHA, so that + // they also work for PRs opened from forks (where Cation is not installed). const owner = pr.base.repo.owner.login; - const repo = pr.head.repo.name; - - if (pr.head.repo.fork) { - log( - 'addOrUpdateDeprecationReviewCheck', - LogLevel.INFO, - `${pr.number} is a fork - checks will not be created or updated`, - ); - // If the PR is a fork PR, return early as the Checks API doesn't work. - return; - } + const repo = pr.base.repo.name; // Fetch the latest Deprecation Review check for the PR. const checkRun = ( @@ -99,16 +82,16 @@ export async function addOrUpdateDeprecationReviewCheck(octokit: ProbotOctokit, (checkRun.status === opts.status || !opts.status || opts.status === 'completed') ) { await octokit.rest.checks.update({ - owner: pr.head.repo!.owner.login, - repo: pr.head.repo!.name, + owner, + repo, name: DEPRECATION_REVIEW_CHECK_NAME, check_run_id: checkRun.id, ...opts, }); } else { await octokit.rest.checks.create({ - owner: pr.head.repo!.owner.login, - repo: pr.head.repo!.name, + owner, + repo, name: DEPRECATION_REVIEW_CHECK_NAME, head_sha: pr.head.sha, ...opts, @@ -147,17 +130,8 @@ export async function addOrUpdateDeprecationReviewCheck(octokit: ProbotOctokit, } export async function maybeAddChecklistComment(octokit: ProbotOctokit, pr: PullRequest) { - if (!pr.head.repo) { - log( - 'maybeAddChecklistComment', - LogLevel.WARN, - `PR #${pr.number} does not have a head repo - cannot add checklist comment`, - ); - return; - } - const owner = pr.base.repo.owner.login; - const repo = pr.head.repo.name; + const repo = pr.base.repo.name; // We do not care about PRs without the deprecation review requested label. const currentReviewLabel = pr.labels.find(({ name }) => isReviewLabel(name)); From 2b7aa4cc574e2bc94a3e854f0349ce9c5f57bd72 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 23:14:44 +0000 Subject: [PATCH 2/4] fix: harden review check lookup and isolate Checks API failures - Only look up Cation's own API Review / Deprecation Review check runs by passing `check_name` and `app_id` to `checks.listForRef`, so a same-named run from another app (e.g. a fork PR's Actions job) can't shadow them and the lookup isn't limited to the first page of check runs. - Log Checks API failures in `addOrUpdateAPIReviewCheck` instead of throwing, so API review approval state is still returned and label automation proceeds. - Isolate each PR in the API review cron so one failure doesn't skip the rest. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01U4dtTVsf8UKZDqhP8BCv1c --- spec/api-review-state.spec.ts | 190 +++++++++++++++++++++++--- spec/deprecation-review-state.spec.ts | 48 ++++++- src/api-review-state.ts | 126 +++++++++++------ src/deprecation-review-state.ts | 6 +- 4 files changed, 308 insertions(+), 62 deletions(-) diff --git a/spec/api-review-state.spec.ts b/spec/api-review-state.spec.ts index f5409a4..57d9d48 100644 --- a/spec/api-review-state.spec.ts +++ b/spec/api-review-state.spec.ts @@ -25,6 +25,14 @@ import { loadFixture } from './utils'; const GH_API = 'https://api.github.com'; +const APP_ID = 690857; + +const CHECK_RUNS_QUERY = { + check_name: API_REVIEW_CHECK_NAME, + app_id: APP_ID, + per_page: 100, +}; + const API_WORKING_GROUP_MEMBERS = [ { login: 'ckerr' }, { login: 'codebytere' }, @@ -48,11 +56,12 @@ describe('api review', () => { beforeEach(() => { nock.cleanAll(); nock.disableNetConnect(); + vi.stubEnv('APP_ID', String(APP_ID)); robot = new Probot({ githubToken: 'test', secret: 'secret', privateKey: 'private key', - appId: 690857, + appId: APP_ID, }); moctokit = { @@ -83,6 +92,7 @@ describe('api review', () => { }); afterEach(() => { + vi.unstubAllEnvs(); expect(nock.isDone()).toEqual(true); nock.cleanAll(); }); @@ -230,6 +240,90 @@ describe('api review', () => { ); }); + it('should only look up API Review check runs created by Cation', async () => { + const { pull_request } = loadFixture( + 'api-review-state/pull_request.requested_review_label.json', + ); + + // Simulate the API filtering by app: a same-named run from another app + // (e.g. a fork PR's Actions job) is only returned when not filtering. + moctokit.rest.checks.listForRef = vi.fn().mockImplementation(({ app_id }) => ({ + data: { + check_runs: + app_id === APP_ID + ? [] + : [{ name: API_REVIEW_CHECK_NAME, id: '67890', app: { id: 15368 } }], + }, + })); + + await addOrUpdateAPIReviewCheck(moctokit, pull_request); + + expect(moctokit.rest.checks.listForRef).toHaveBeenCalledWith( + expect.objectContaining({ check_name: API_REVIEW_CHECK_NAME, app_id: APP_ID }), + ); + expect(moctokit.rest.checks.update).not.toHaveBeenCalled(); + expect(moctokit.rest.checks.create).toHaveBeenCalledWith( + expect.objectContaining({ name: API_REVIEW_CHECK_NAME, head_sha: pull_request.head.sha }), + ); + }); + + it('should still return approval state on a fork PR when the Checks API throws', async () => { + const { pull_request } = loadFixture( + 'api-review-state/pull_request.requested_review_label.json', + ); + + pull_request.head.repo = { + ...pull_request.head.repo, + name: 'electron-fork', + full_name: 'contributor/electron-fork', + owner: { login: 'contributor' }, + fork: true, + }; + + moctokit.rest.checks.create = vi.fn().mockRejectedValue(new Error('Resource not accessible')); + moctokit.rest.pulls.listReviews.mockReturnValue({ + data: [ + { user: { id: 1, login: 'codebytere' }, body: 'API LGTM', state: 'COMMENTED' }, + { user: { id: 2, login: 'jkleinsc' }, body: 'API LGTM', state: 'COMMENTED' }, + ], + }); + moctokit.rest.teams.listMembersInOrg.mockReturnValue({ + data: [{ login: 'codebytere' }, { login: 'jkleinsc' }], + }); + + const users = await addOrUpdateAPIReviewCheck(moctokit, pull_request); + expect(moctokit.rest.checks.create).toHaveBeenCalled(); + expect(users).toEqual({ + approved: ['codebytere', 'jkleinsc'], + declined: [], + requestedChanges: [], + }); + + await checkPRReadyForMerge(moctokit, pull_request, users); + expect(moctokit.rest.issues.addLabels).toHaveBeenCalledWith( + expect.objectContaining({ labels: [REVIEW_LABELS.APPROVED] }), + ); + }); + + it('should still return approval state without writing the check when the check lookup throws', async () => { + const { pull_request } = loadFixture( + 'api-review-state/pull_request.requested_review_label.json', + ); + + moctokit.rest.checks.listForRef = vi.fn().mockRejectedValue(new Error('Server Error')); + moctokit.rest.pulls.listReviews.mockReturnValue({ + data: [{ user: { id: 1, login: 'codebytere' }, body: 'API DECLINED', state: 'COMMENTED' }], + }); + moctokit.rest.teams.listMembersInOrg.mockReturnValue({ + data: [{ login: 'codebytere' }], + }); + + const users = await addOrUpdateAPIReviewCheck(moctokit, pull_request); + expect(users).toEqual({ approved: [], declined: ['codebytere'], requestedChanges: [] }); + expect(moctokit.rest.checks.create).not.toHaveBeenCalled(); + expect(moctokit.rest.checks.update).not.toHaveBeenCalled(); + }); + it(`should correctly parse user approval status for ${REVIEW_LABELS.REQUESTED} label`, async () => { const { pull_request } = loadFixture( 'api-review-state/pull_request.requested_review_label.json', @@ -552,9 +646,8 @@ describe('api review', () => { const payload = loadFixture('api-review-state/pull_request.no_review_label.json'); nock(GH_API) - .get( - `/repos/electron/electron/commits/${payload.pull_request.head.sha}/check-runs?per_page=100`, - ) + .get(`/repos/electron/electron/commits/${payload.pull_request.head.sha}/check-runs`) + .query(CHECK_RUNS_QUERY) .reply(200, { check_runs: [ { @@ -592,9 +685,8 @@ describe('api review', () => { const payload = loadFixture('api-review-state/pull_request.api-skip-review_label.json'); nock(GH_API) - .get( - `/repos/electron/electron/commits/${payload.pull_request.head.sha}/check-runs?per_page=100`, - ) + .get(`/repos/electron/electron/commits/${payload.pull_request.head.sha}/check-runs`) + .query(CHECK_RUNS_QUERY) .reply(200, { check_runs: [ { @@ -639,7 +731,8 @@ describe('api review', () => { .reply(200, API_WORKING_GROUP_MEMBERS); nock(GH_API) - .get(`/repos/electron/electron/commits/${pull_request.head.sha}/check-runs?per_page=100`) + .get(`/repos/electron/electron/commits/${pull_request.head.sha}/check-runs`) + .query(CHECK_RUNS_QUERY) .reply(200, { check_runs: [ { @@ -697,7 +790,8 @@ describe('api review', () => { .reply(200, API_WORKING_GROUP_MEMBERS); nock(GH_API) - .get(`/repos/electron/electron/commits/${pull_request.head.sha}/check-runs?per_page=100`) + .get(`/repos/electron/electron/commits/${pull_request.head.sha}/check-runs`) + .query(CHECK_RUNS_QUERY) .reply(200, { check_runs: [ { @@ -797,7 +891,8 @@ describe('api review', () => { .reply(200, API_WORKING_GROUP_MEMBERS); nock(GH_API) - .get(`/repos/electron/electron/commits/${pull_request.head.sha}/check-runs?per_page=100`) + .get(`/repos/electron/electron/commits/${pull_request.head.sha}/check-runs`) + .query(CHECK_RUNS_QUERY) .reply(200, { check_runs: [ { @@ -897,7 +992,8 @@ describe('api review', () => { .reply(200, API_WORKING_GROUP_MEMBERS); nock(GH_API) - .get(`/repos/electron/electron/commits/${pull_request.head.sha}/check-runs?per_page=100`) + .get(`/repos/electron/electron/commits/${pull_request.head.sha}/check-runs`) + .query(CHECK_RUNS_QUERY) .reply(200, { check_runs: [] }); const r1 = loadFixture('api-review-state/pull_request_review/fork/review_lgtm.json'); @@ -942,7 +1038,8 @@ describe('api review', () => { .reply(200, API_WORKING_GROUP_MEMBERS); nock(GH_API) - .get(`/repos/electron/electron/commits/${pull_request.head.sha}/check-runs?per_page=100`) + .get(`/repos/electron/electron/commits/${pull_request.head.sha}/check-runs`) + .query(CHECK_RUNS_QUERY) .reply(200, { check_runs: [] }); const r1 = loadFixture('api-review-state/pull_request_review/fork/review_lgtm.json'); @@ -1028,7 +1125,8 @@ describe('api review', () => { .reply(200, API_WORKING_GROUP_MEMBERS); nock(GH_API) - .get(`/repos/electron/electron/commits/${pull_request.head.sha}/check-runs?per_page=100`) + .get(`/repos/electron/electron/commits/${pull_request.head.sha}/check-runs`) + .query(CHECK_RUNS_QUERY) .reply(200, { check_runs: [] }); const r1 = loadFixture('api-review-state/pull_request_review/fork/review_declined.json'); @@ -1120,9 +1218,8 @@ describe('api review', () => { .reply(200); nock(GH_API) - .get( - `/repos/electron/electron/commits/${payload.pull_request.head.sha}/check-runs?per_page=100`, - ) + .get(`/repos/electron/electron/commits/${payload.pull_request.head.sha}/check-runs`) + .query(CHECK_RUNS_QUERY) .reply(200, { check_runs: [] }); await robot.receive({ @@ -1233,4 +1330,65 @@ describe('api review', () => { payload, }); }); + + it('continues the cron pass past a PR that fails to update', async () => { + const { pull_request: failingPR } = loadFixture( + 'api-review-state/pull_request.requested_review_label.json', + ); + const passingPR = { + ...failingPR, + number: failingPR.number + 1, + head: { ...failingPR.head, sha: 'b7d7b8b1a1a0e6c0bdb56b2f6b0c4d2b1e0a9f31' }, + labels: [{ name: REVIEW_LABELS.REQUESTED }], + }; + + nock(GH_API) + .get('/app/installations') + .reply(200, [{ id: 1 }]); + + nock(GH_API) + .get('/installation/repositories') + .reply(200, { repositories: [{ name: 'electron', owner: { login: 'electron' } }] }); + + nock(GH_API) + .get('/repos/electron/electron/pulls') + .query({ per_page: 100, state: 'open' }) + .reply(200, [failingPR, passingPR]); + + // The first PR throws while fetching API WG members. + nock(GH_API) + .get(`/repos/electron/electron/commits/${failingPR.head.sha}/check-runs`) + .query(CHECK_RUNS_QUERY) + .reply(200, { check_runs: [] }); + + nock(GH_API).get(`/orgs/electron/teams/${API_WORKING_GROUP}/members`).reply(404); + + // The second PR is no longer semver-minor, so its check is reset. + nock(GH_API) + .get(`/repos/electron/electron/commits/${passingPR.head.sha}/check-runs`) + .query(CHECK_RUNS_QUERY) + .reply(200, { check_runs: [{ name: API_REVIEW_CHECK_NAME, id: '12345' }] }); + + nock(GH_API) + .patch('/repos/electron/electron/check-runs/12345', (body) => { + expect(body).toMatchObject({ conclusion: CheckRunStatus.NEUTRAL }); + return true; + }) + .reply(200); + + const setTimeoutSpy = vi.spyOn(global, 'setTimeout'); + try { + robot.load((app: Probot) => setupAPIReviewStateManagement(app)); + + // The pass is complete once the next interval is scheduled. + const nextRun = await vi.waitFor(() => { + const index = setTimeoutSpy.mock.calls.findIndex(([, ms]) => ms === 1000 * 60 * 5); + expect(index).not.toEqual(-1); + return setTimeoutSpy.mock.results[index].value; + }); + clearTimeout(nextRun); + } finally { + setTimeoutSpy.mockRestore(); + } + }); }); diff --git a/spec/deprecation-review-state.spec.ts b/spec/deprecation-review-state.spec.ts index caaf832..b8fad6a 100644 --- a/spec/deprecation-review-state.spec.ts +++ b/spec/deprecation-review-state.spec.ts @@ -23,6 +23,14 @@ import { loadFixture } from './utils'; const GH_API = 'https://api.github.com'; +const APP_ID = 690857; + +const CHECK_RUNS_QUERY = { + check_name: DEPRECATION_REVIEW_CHECK_NAME, + app_id: APP_ID, + per_page: 100, +}; + const handler = async (app: Probot) => { setupDeprecationReviewStateManagement(app); }; @@ -39,11 +47,12 @@ describe('deprecation review', () => { beforeEach(() => { nock.cleanAll(); nock.disableNetConnect(); + vi.stubEnv('APP_ID', String(APP_ID)); robot = new Probot({ githubToken: 'test', secret: 'secret', privateKey: 'private key', - appId: 690857, + appId: APP_ID, }); moctokit = { @@ -73,6 +82,7 @@ describe('deprecation review', () => { }); afterEach(() => { + vi.unstubAllEnvs(); console.log(nock.pendingMocks()); expect(nock.isDone()).toEqual(true); nock.cleanAll(); @@ -164,6 +174,36 @@ describe('deprecation review', () => { expect(moctokit.rest.checks.create).toHaveBeenCalledWith(expect.objectContaining(expected)); }); + it('should only look up Deprecation Review check runs created by Cation', async () => { + const { pull_request } = loadFixture( + 'deprecation-review-state/pull_request.requested_review_label.json', + ); + + // Simulate the API filtering by app: a same-named run from another app + // (e.g. a fork PR's Actions job) is only returned when not filtering. + moctokit.rest.checks.listForRef = vi.fn().mockImplementation(({ app_id }) => ({ + data: { + check_runs: + app_id === APP_ID + ? [] + : [{ name: DEPRECATION_REVIEW_CHECK_NAME, id: '67890', app: { id: 15368 } }], + }, + })); + + await addOrUpdateDeprecationReviewCheck(moctokit, pull_request); + + expect(moctokit.rest.checks.listForRef).toHaveBeenCalledWith( + expect.objectContaining({ check_name: DEPRECATION_REVIEW_CHECK_NAME, app_id: APP_ID }), + ); + expect(moctokit.rest.checks.update).not.toHaveBeenCalled(); + expect(moctokit.rest.checks.create).toHaveBeenCalledWith( + expect.objectContaining({ + name: DEPRECATION_REVIEW_CHECK_NAME, + head_sha: pull_request.head.sha, + }), + ); + }); + it('should create the check on the base repo when the PR is from a fork', async () => { const { pull_request } = loadFixture( 'deprecation-review-state/pull_request.requested_review_label.json', @@ -278,8 +318,9 @@ describe('deprecation review', () => { nock(GH_API) .get( - `/repos/dsanders11/deprecation-review/commits/${payload.pull_request.head.sha}/check-runs?per_page=100`, + `/repos/dsanders11/deprecation-review/commits/${payload.pull_request.head.sha}/check-runs`, ) + .query(CHECK_RUNS_QUERY) .reply(200, { check_runs: [], }); @@ -328,8 +369,9 @@ describe('deprecation review', () => { nock(GH_API) .get( - `/repos/dsanders11/deprecation-review/commits/${payload.pull_request.head.sha}/check-runs?per_page=100`, + `/repos/dsanders11/deprecation-review/commits/${payload.pull_request.head.sha}/check-runs`, ) + .query(CHECK_RUNS_QUERY) .reply(200, { check_runs: [ { diff --git a/src/api-review-state.ts b/src/api-review-state.ts index 9d8e0ed..830f08e 100644 --- a/src/api-review-state.ts +++ b/src/api-review-state.ts @@ -104,35 +104,63 @@ export async function addOrUpdateAPIReviewCheck(octokit: Context['octokit'], pr: type CommentOrReview = ListReviewsItem & ListCommentsItem; + type CheckRun = GetResponseDataTypeFromEndpointMethod< + typeof octokit.rest.checks.listForRef + >['check_runs'][0]; + // Checks are always written to the base repo against the head SHA, so that // they also work for PRs opened from forks (where Cation is not installed). const owner = pr.base.repo.owner.login; const repo = pr.base.repo.name; - // Fetch the latest API Review check for the PR. - const checkRun = ( - await octokit.rest.checks.listForRef({ - ref: pr.head.sha, - per_page: 100, - owner, - repo, - }) - ).data.check_runs.find((run) => run.name === API_REVIEW_CHECK_NAME); + // Checks API failures are logged rather than thrown, so that they can never + // block API review label automation. + const logCheckError = (err: unknown) => + log( + 'addOrUpdateAPIReviewCheck', + LogLevel.ERROR, + `Failed to update ${API_REVIEW_CHECK_NAME} check for #${pr.number}: ${err}`, + ); + + // Fetch the latest API Review check for the PR. Only runs created by Cation + // itself are considered, so that a same-named run from another app (e.g. an + // Actions job added by a fork PR) can't shadow it. + let checkRun: CheckRun | undefined; + let checkLookupFailed = false; + try { + checkRun = ( + await octokit.rest.checks.listForRef({ + ref: pr.head.sha, + check_name: API_REVIEW_CHECK_NAME, + app_id: Number(getEnvVar('APP_ID')), + per_page: 100, + owner, + repo, + }) + ).data.check_runs.find((run) => run.name === API_REVIEW_CHECK_NAME); + } catch (err) { + checkLookupFailed = true; + logCheckError(err); + } const resetToNeutral = async () => { if (!checkRun) return; - return await octokit.rest.checks.update({ - owner, - repo, - name: API_REVIEW_CHECK_NAME, - status: 'completed', - output: { - title: 'Outdated', - summary: 'PR no longer requires API Review', - }, - check_run_id: checkRun.id, - conclusion: CheckRunStatus.NEUTRAL, - }); + try { + await octokit.rest.checks.update({ + owner, + repo, + name: API_REVIEW_CHECK_NAME, + status: 'completed', + output: { + title: 'Outdated', + summary: 'PR no longer requires API Review', + }, + check_run_id: checkRun.id, + conclusion: CheckRunStatus.NEUTRAL, + }); + } catch (err) { + logCheckError(err); + } }; // We do not care about PRs without an API review label of any kind. @@ -273,25 +301,32 @@ export async function addOrUpdateAPIReviewCheck(octokit: Context['octokit'], pr: 'baseUrl' | 'headers' | 'mediaType' | 'owner' | 'repo' | 'name' | 'head_sha' >, ) => { - if ( - checkRun && - (checkRun.status === opts.status || !opts.status || opts.status === 'completed') - ) { - await octokit.rest.checks.update({ - owner, - repo, - name: API_REVIEW_CHECK_NAME, - check_run_id: checkRun.id, - ...opts, - }); - } else { - await octokit.rest.checks.create({ - owner, - repo, - name: API_REVIEW_CHECK_NAME, - head_sha: pr.head.sha, - ...opts, - }); + // If we couldn't look up the existing check, don't risk creating a duplicate. + if (checkLookupFailed) return users; + + try { + if ( + checkRun && + (checkRun.status === opts.status || !opts.status || opts.status === 'completed') + ) { + await octokit.rest.checks.update({ + owner, + repo, + name: API_REVIEW_CHECK_NAME, + check_run_id: checkRun.id, + ...opts, + }); + } else { + await octokit.rest.checks.create({ + owner, + repo, + name: API_REVIEW_CHECK_NAME, + head_sha: pr.head.sha, + ...opts, + }); + } + } catch (err) { + logCheckError(err); } return users; @@ -655,8 +690,15 @@ export function setupAPIReviewStateManagement(probot: Probot, disableCronForTest ); for (const pr of reviewPRs) { - const approvalState = await addOrUpdateAPIReviewCheck(octokit, pr); - await checkPRReadyForMerge(octokit, pr, approvalState); + // Isolate each PR so that one failure doesn't skip the remaining PRs. + try { + const approvalState = await addOrUpdateAPIReviewCheck(octokit, pr); + await checkPRReadyForMerge(octokit, pr, approvalState); + } catch (err) { + probot.log.error( + `Failed to update API review state for ${repo.owner.login}/${repo.name}#${pr.number}: ${err}`, + ); + } } } } diff --git a/src/deprecation-review-state.ts b/src/deprecation-review-state.ts index 48a3fc9..74a57b7 100644 --- a/src/deprecation-review-state.ts +++ b/src/deprecation-review-state.ts @@ -37,10 +37,14 @@ export async function addOrUpdateDeprecationReviewCheck(octokit: ProbotOctokit, const owner = pr.base.repo.owner.login; const repo = pr.base.repo.name; - // Fetch the latest Deprecation Review check for the PR. + // Fetch the latest Deprecation Review check for the PR. Only runs created by + // Cation itself are considered, so that a same-named run from another app + // (e.g. an Actions job added by a fork PR) can't shadow it. const checkRun = ( await octokit.rest.checks.listForRef({ ref: pr.head.sha, + check_name: DEPRECATION_REVIEW_CHECK_NAME, + app_id: Number(getEnvVar('APP_ID')), per_page: 100, owner, repo, From 52c81c8bd9632fd04561e1a2cef9462e0b195394 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 23:42:23 +0000 Subject: [PATCH 3/4] fix: pass the API Review check when review isn't needed PRs without an API review label, semver-patch PRs, and PRs with the skip-review label now get a completed/success "Not required" API Review check on their head SHA (created if missing, updated otherwise) instead of no check or a neutral "Outdated" one. This lets the check be made required without blocking PRs that don't need API review. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01U4dtTVsf8UKZDqhP8BCv1c --- README.md | 2 +- spec/api-review-state.spec.ts | 158 +++++++++++++++++++++++++++++----- src/api-review-state.ts | 43 +++++---- 3 files changed, 164 insertions(+), 39 deletions(-) diff --git a/README.md b/README.md index 980899c..6e388d7 100644 --- a/README.md +++ b/README.md @@ -26,7 +26,7 @@ This group's review is mandated on all API changes, and their goal is twofold: Even changes that seem trivial can often be made more consistent and future-proof with some modifications, and the folks on the API WG have the expertise to spot and suggest those changes. -In accordance with the above goals, this bot performs several bookkeeping duties. When a new PR is opened which is either `semver/minor` or `semver/major`, it will automatically add an `api-review/requested 🗳` label to the PR. Backport PRs (PRs to a release branch that is not `main`) and PRs labeled `backport`, `backport-check-skip`, or `fast-track 🚅` are exempt from API review. To add clarity to whether a review is occurring in a given Electron governance member's capacity as a member of the API WG, this bot then adds a GitHub Check on the PR which will update as members of the API WG indicate their approval statuses. +In accordance with the above goals, this bot performs several bookkeeping duties. When a new PR is opened which is either `semver/minor` or `semver/major`, it will automatically add an `api-review/requested 🗳` label to the PR. Backport PRs (PRs to a release branch that is not `main`) and PRs labeled `backport`, `backport-check-skip`, or `fast-track 🚅` are exempt from API review. To add clarity to whether a review is occurring in a given Electron governance member's capacity as a member of the API WG, this bot then adds a GitHub Check on the PR which will update as members of the API WG indicate their approval statuses. PRs that do not need API review (those without an API review label, those that are not `semver/minor` or `semver/major`, and those labeled `api-review/skip-review 🛂`) instead get a passing `API Review` check, so the check never blocks them. Members of the API Working Group must indicate their approval by leaving a comment via a PR Review containing `API LGTM`. This may not necessarily be a full approval with the GitHub API since approval by the API is primarily about the API shape and design. Both a PR review that comments and includes an LGTM indication or which is an approval with LGTM are sufficient. diff --git a/spec/api-review-state.spec.ts b/spec/api-review-state.spec.ts index 57d9d48..062ff1d 100644 --- a/spec/api-review-state.spec.ts +++ b/spec/api-review-state.spec.ts @@ -17,6 +17,7 @@ import { MINIMUM_PATCH_OPEN_TIME, API_REVIEW_CHECK_NAME, API_SKIP_DELAY_LABEL, + API_SKIP_REVIEW_LABEL, API_WORKING_GROUP, } from '../src/constants'; @@ -180,7 +181,13 @@ describe('api review', () => { expect(readyDate).toEqual(expectedDate); }); - it('should reset the check when PR does not have an API review label on a base PR', async () => { + const NOT_REQUIRED = { + name: API_REVIEW_CHECK_NAME, + status: 'completed', + conclusion: CheckRunStatus.SUCCESS, + }; + + it('should mark the existing check as not required when PR does not have an API review label', async () => { let { pull_request } = loadFixture('api-review-state/pull_request.no_review_label.json'); moctokit.rest.checks.listForRef = vi.fn().mockReturnValue({ @@ -189,6 +196,7 @@ describe('api review', () => { { name: API_REVIEW_CHECK_NAME, id: '12345', + status: 'in_progress', }, ], }, @@ -201,10 +209,20 @@ describe('api review', () => { expect(moctokit.rest.issues.removeLabel).not.toHaveBeenCalled(); expect(moctokit.rest.checks.listForRef).toHaveBeenCalled(); - expect(moctokit.rest.checks.update).toHaveBeenCalled(); + expect(moctokit.rest.checks.create).not.toHaveBeenCalled(); + expect(moctokit.rest.checks.update).toHaveBeenCalledWith( + expect.objectContaining({ + ...NOT_REQUIRED, + check_run_id: '12345', + output: { + title: 'Not required', + summary: 'This PR does not require API review: it has no API review label.', + }, + }), + ); }); - it('should reset the check on the base repo when the PR does not have an API review label on a fork PR', async () => { + it('should create a passing check on the base repo when a fork PR does not have an API review label', async () => { let { pull_request } = loadFixture('api-review-state/pull_request.no_review_label.json'); pull_request.head.repo = { @@ -215,28 +233,113 @@ describe('api review', () => { fork: true, }; + const users = await addOrUpdateAPIReviewCheck(moctokit, pull_request); + expect(users).toEqual(undefined); + + expect(moctokit.rest.issues.addLabels).not.toHaveBeenCalled(); + expect(moctokit.rest.issues.removeLabel).not.toHaveBeenCalled(); + + expect(moctokit.rest.checks.listForRef).toHaveBeenCalledWith( + expect.objectContaining({ owner: 'electron', repo: 'electron', ref: pull_request.head.sha }), + ); + expect(moctokit.rest.checks.update).not.toHaveBeenCalled(); + expect(moctokit.rest.checks.create).toHaveBeenCalledWith( + expect.objectContaining({ + ...NOT_REQUIRED, + owner: 'electron', + repo: 'electron', + head_sha: pull_request.head.sha, + }), + ); + }); + + it('should create a passing check when a semver-patch PR has no existing check', async () => { + const { pull_request } = loadFixture( + 'api-review-state/pull_request.requested_review_label.json', + ); + pull_request.labels = [{ name: REVIEW_LABELS.REQUESTED }, { name: SEMVER_LABELS.PATCH }]; + + const users = await addOrUpdateAPIReviewCheck(moctokit, pull_request); + expect(users).toEqual(undefined); + + expect(moctokit.rest.checks.update).not.toHaveBeenCalled(); + expect(moctokit.rest.checks.create).toHaveBeenCalledWith( + expect.objectContaining({ + ...NOT_REQUIRED, + head_sha: pull_request.head.sha, + output: { + title: 'Not required', + summary: 'This PR does not require API review: it is not semver-minor or semver-major.', + }, + }), + ); + }); + + it('should create a passing check when a PR has the skip-review label and no existing check', async () => { + const { pull_request } = loadFixture( + 'api-review-state/pull_request.requested_review_label.json', + ); + pull_request.labels = [ + { name: REVIEW_LABELS.REQUESTED }, + { name: SEMVER_LABELS.MINOR }, + { name: API_SKIP_REVIEW_LABEL }, + ]; + + const users = await addOrUpdateAPIReviewCheck(moctokit, pull_request); + expect(users).toEqual(undefined); + + expect(moctokit.rest.teams.listMembersInOrg).not.toHaveBeenCalled(); + expect(moctokit.rest.checks.update).not.toHaveBeenCalled(); + expect(moctokit.rest.checks.create).toHaveBeenCalledWith( + expect.objectContaining({ + ...NOT_REQUIRED, + head_sha: pull_request.head.sha, + output: { + title: 'Not required', + summary: `This PR does not require API review: it has the "${API_SKIP_REVIEW_LABEL}" label.`, + }, + }), + ); + }); + + it('should not write a passing check when the check lookup throws', async () => { + const { pull_request } = loadFixture('api-review-state/pull_request.no_review_label.json'); + + moctokit.rest.checks.listForRef = vi.fn().mockRejectedValue(new Error('Server Error')); + + const users = await addOrUpdateAPIReviewCheck(moctokit, pull_request); + expect(users).toEqual(undefined); + expect(moctokit.rest.checks.create).not.toHaveBeenCalled(); + expect(moctokit.rest.checks.update).not.toHaveBeenCalled(); + }); + + it('should supersede a not required check with a new in-progress run when API review is requested', async () => { + const { pull_request } = loadFixture( + 'api-review-state/pull_request.requested_review_label.json', + ); + moctokit.rest.checks.listForRef = vi.fn().mockReturnValue({ data: { check_runs: [ { name: API_REVIEW_CHECK_NAME, id: '12345', + status: 'completed', + conclusion: CheckRunStatus.SUCCESS, }, ], }, }); - const users = await addOrUpdateAPIReviewCheck(moctokit, pull_request); - expect(users).toEqual(undefined); - - expect(moctokit.rest.issues.addLabels).not.toHaveBeenCalled(); - expect(moctokit.rest.issues.removeLabel).not.toHaveBeenCalled(); + await addOrUpdateAPIReviewCheck(moctokit, pull_request); - expect(moctokit.rest.checks.listForRef).toHaveBeenCalledWith( - expect.objectContaining({ owner: 'electron', repo: 'electron', ref: pull_request.head.sha }), - ); - expect(moctokit.rest.checks.update).toHaveBeenCalledWith( - expect.objectContaining({ owner: 'electron', repo: 'electron', check_run_id: '12345' }), + expect(moctokit.rest.checks.update).not.toHaveBeenCalled(); + expect(moctokit.rest.checks.create).toHaveBeenCalledWith( + expect.objectContaining({ + name: API_REVIEW_CHECK_NAME, + head_sha: pull_request.head.sha, + status: 'in_progress', + }), ); }); @@ -661,10 +764,10 @@ describe('api review', () => { name: API_REVIEW_CHECK_NAME, status: 'completed', output: { - title: 'Outdated', - summary: 'PR no longer requires API Review', + title: 'Not required', + summary: 'This PR does not require API review: it has no API review label.', }, - conclusion: CheckRunStatus.NEUTRAL, + conclusion: CheckRunStatus.SUCCESS, }; nock(GH_API) @@ -700,10 +803,10 @@ describe('api review', () => { name: API_REVIEW_CHECK_NAME, status: 'completed', output: { - title: 'Outdated', - summary: 'PR no longer requires API Review', + title: 'Not required', + summary: 'This PR does not require API review: it has no API review label.', }, - conclusion: CheckRunStatus.NEUTRAL, + conclusion: CheckRunStatus.SUCCESS, }; nock(GH_API) @@ -1222,6 +1325,19 @@ describe('api review', () => { .query(CHECK_RUNS_QUERY) .reply(200, { check_runs: [] }); + // This event's payload predates the requested label, so the check is marked + // not required here and then superseded by the labeled event for that label. + nock(GH_API) + .post('/repos/electron/electron/check-runs', (body) => { + expect(body).toMatchObject({ + name: API_REVIEW_CHECK_NAME, + head_sha: payload.pull_request.head.sha, + conclusion: CheckRunStatus.SUCCESS, + }); + return true; + }) + .reply(200); + await robot.receive({ id: '123-456', name: 'pull_request', @@ -1363,7 +1479,7 @@ describe('api review', () => { nock(GH_API).get(`/orgs/electron/teams/${API_WORKING_GROUP}/members`).reply(404); - // The second PR is no longer semver-minor, so its check is reset. + // The second PR is no longer semver-minor, so its check is marked not required. nock(GH_API) .get(`/repos/electron/electron/commits/${passingPR.head.sha}/check-runs`) .query(CHECK_RUNS_QUERY) @@ -1371,7 +1487,7 @@ describe('api review', () => { nock(GH_API) .patch('/repos/electron/electron/check-runs/12345', (body) => { - expect(body).toMatchObject({ conclusion: CheckRunStatus.NEUTRAL }); + expect(body).toMatchObject({ conclusion: CheckRunStatus.SUCCESS }); return true; }) .reply(200); diff --git a/src/api-review-state.ts b/src/api-review-state.ts index 830f08e..823e045 100644 --- a/src/api-review-state.ts +++ b/src/api-review-state.ts @@ -143,21 +143,30 @@ export async function addOrUpdateAPIReviewCheck(octokit: Context['octokit'], pr: logCheckError(err); } - const resetToNeutral = async () => { - if (!checkRun) return; + // PRs that don't need API review get a passing check on every head SHA, so + // that the check can be required without blocking them. + const markNotRequired = async (reason: string) => { + // If we couldn't look up the existing check, don't risk creating a duplicate. + if (checkLookupFailed) return; + + const opts = { + owner, + repo, + name: API_REVIEW_CHECK_NAME, + status: 'completed' as const, + conclusion: CheckRunStatus.SUCCESS, + output: { + title: 'Not required', + summary: `This PR does not require API review: ${reason}.`, + }, + }; + try { - await octokit.rest.checks.update({ - owner, - repo, - name: API_REVIEW_CHECK_NAME, - status: 'completed', - output: { - title: 'Outdated', - summary: 'PR no longer requires API Review', - }, - check_run_id: checkRun.id, - conclusion: CheckRunStatus.NEUTRAL, - }); + if (checkRun) { + await octokit.rest.checks.update({ ...opts, check_run_id: checkRun.id }); + } else { + await octokit.rest.checks.create({ ...opts, head_sha: pr.head.sha }); + } } catch (err) { logCheckError(err); } @@ -166,7 +175,7 @@ export async function addOrUpdateAPIReviewCheck(octokit: Context['octokit'], pr: // We do not care about PRs without an API review label of any kind. const currentReviewLabel = pr.labels.find((l) => Object.values(REVIEW_LABELS).includes(l.name)); if (!currentReviewLabel) { - await resetToNeutral(); + await markNotRequired('it has no API review label'); return; } @@ -177,7 +186,7 @@ export async function addOrUpdateAPIReviewCheck(octokit: Context['octokit'], pr: LogLevel.INFO, 'Determined this PR is semver-patch and does not need review', ); - await resetToNeutral(); + await markNotRequired('it is not semver-minor or semver-major'); return; } @@ -188,7 +197,7 @@ export async function addOrUpdateAPIReviewCheck(octokit: Context['octokit'], pr: LogLevel.INFO, 'This PR has the skip-review label and does not need review', ); - await resetToNeutral(); + await markNotRequired(`it has the "${API_SKIP_REVIEW_LABEL}" label`); return; } From c0d7ac6f7174c7aaf1ed524974093db3d7617255 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 23:44:01 +0000 Subject: [PATCH 4/4] fix: keep API Review pending when Cation requests review The labeled(semver-minor) payload predates the api-review/requested label Cation adds in response, so the check was briefly marked as not required. Include the requested label when updating the check from that handler. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01U4dtTVsf8UKZDqhP8BCv1c --- spec/api-review-state.spec.ts | 16 +++++++++++++--- src/api-review-state.ts | 11 ++++++++++- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/spec/api-review-state.spec.ts b/spec/api-review-state.spec.ts index 062ff1d..f8c3fd1 100644 --- a/spec/api-review-state.spec.ts +++ b/spec/api-review-state.spec.ts @@ -1325,14 +1325,24 @@ describe('api review', () => { .query(CHECK_RUNS_QUERY) .reply(200, { check_runs: [] }); - // This event's payload predates the requested label, so the check is marked - // not required here and then superseded by the labeled event for that label. + nock(GH_API) + .get(`/orgs/electron/teams/${API_WORKING_GROUP}/members`) + .reply(200, API_WORKING_GROUP_MEMBERS); + + nock(GH_API).get(`/repos/electron/electron/pulls/${payload.number}/reviews`).reply(200, []); + + nock(GH_API).get(`/repos/electron/electron/issues/${payload.number}/comments`).reply(200, []); + + nock(GH_API).get(`/repos/electron/electron/issues/${payload.number}/timeline`).reply(200, []); + + // The check reflects the requested label even though this event's payload + // predates it, rather than being marked as not required. nock(GH_API) .post('/repos/electron/electron/check-runs', (body) => { expect(body).toMatchObject({ name: API_REVIEW_CHECK_NAME, head_sha: payload.pull_request.head.sha, - conclusion: CheckRunStatus.SUCCESS, + status: 'in_progress', }); return true; }) diff --git a/src/api-review-state.ts b/src/api-review-state.ts index 823e045..21e8f83 100644 --- a/src/api-review-state.ts +++ b/src/api-review-state.ts @@ -545,7 +545,7 @@ export function setupAPIReviewStateManagement(probot: Probot, disableCronForTest sender: { login: initiator }, } = context.payload; - const pr = context.payload.pull_request as PullRequest; + let pr = context.payload.pull_request as PullRequest; if (!label) { throw new Error('Something went wrong - label does not exist.'); @@ -580,6 +580,15 @@ export function setupAPIReviewStateManagement(probot: Probot, disableCronForTest labels: [REVIEW_LABELS.REQUESTED], }); + // This event's payload predates the label we just added, so include it + // when updating the check to avoid briefly marking it as not required. + if (!hasAPIReviewRequestedLabel(pr)) { + pr = { + ...pr, + labels: [...pr.labels, { name: REVIEW_LABELS.REQUESTED } as PullRequest['labels'][0]], + }; + } + // If the human-added label is an approve/decline API review label // remove it. } else if (isReviewLabel(label.name)) {