diff --git a/__tests__/bundle/bundle.test.ts b/__tests__/bundle/bundle.test.ts index c633ac8..3de97d3 100644 --- a/__tests__/bundle/bundle.test.ts +++ b/__tests__/bundle/bundle.test.ts @@ -563,6 +563,69 @@ describe('dist/index.js', () => { expect(gh.requestsMatching('PUT', /merge$/)).toEqual([]) }) + describe('with approve.github_review', () => { + const reviewMarker = '' + + function routeGithubReview() { + gh.route('GET', `${repo}/contents/.github%2Fprow.yaml`, { status: 200, body: yamlFile('approve:\n github_review: true\n') }) + // GITHUB_TOKEN is an installation token: GET /user is refused, its reviews are a Bot's + gh.route('GET', '/user', { status: 403, body: { message: 'Resource not accessible by integration' } }) + gh.route('PUT', /\/pulls\/1\/reviews\/\d+\/dismissals$/, { status: 200, body: {} }) + } + + it('/approve adds approved, then submits one APPROVE review on the head before the merge evaluation', async () => { + routeGithubReview() + routeApprove(['sdk/x.go'], { + comments: [{ id: 1, body: '/approve', user: { login: 'bob', type: 'User' }, created_at: '2024-01-01T00:00:01Z' }], + }) + + const result = await runApprove('/approve', 'bob') + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + expect(gh.requestsMatching('POST', /\/issues\/1\/labels$/).map(r => r.body)).toEqual([{ labels: ['approved'] }]) + const reviews = gh.requestsMatching('POST', /\/pulls\/1\/reviews$/) + expect(reviews).toHaveLength(1) + expect(reviews[0].body).toEqual({ + commit_id: pullBody.head.sha, + event: 'APPROVE', + body: expect.stringContaining(reviewMarker), + }) + expect((reviews[0].body as { body: string }).body).toContain('Approved via /approve by bob (OWNERS).') + const calls = gh.requests.map(r => `${r.method} ${r.path}`) + const at = (call: string) => calls.indexOf(call) + expect(at(`POST ${repo}/issues/1/labels`)).toBeLessThan(at(`POST ${repo}/pulls/1/reviews`)) + expect(at(`POST ${repo}/pulls/1/reviews`)).toBeLessThan(calls.lastIndexOf(`GET ${repo}/pulls/1`)) + expect(gh.requestsMatching('PUT', /dismissals$/)).toEqual([]) + }) + + it('/approve cancel removes approved and dismisses the mirrored review, leaving a review without the marker alone', async () => { + routeGithubReview() + routeApprove(['sdk/x.go'], { + labels: ['approved'], + comments: [ + { id: 900, body: `stale\n${marker}`, user: bot, created_at: '2024-01-01T00:00:00Z' }, + { id: 1, body: '/approve', user: { login: 'bob', type: 'User' }, created_at: '2024-01-01T00:00:01Z' }, + { id: 2, body: '/approve cancel', user: { login: 'bob', type: 'User' }, created_at: '2024-01-01T00:00:02Z' }, + ], + reviews: [ + { id: 70, state: 'APPROVED', user: bot, body: '', commit_id: pullBody.head.sha, submitted_at: '2024-01-01T00:00:00Z' }, + { id: 71, state: 'APPROVED', user: bot, body: `Approved via /approve by bob (OWNERS).\n${reviewMarker}`, commit_id: pullBody.head.sha, submitted_at: '2024-01-01T00:00:01Z' }, + ], + }) + + const result = await runApprove('/approve cancel', 'bob') + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + expect(gh.requestsMatching('DELETE', /\/issues\/1\/labels\/approved$/)).toHaveLength(1) + const dismissals = gh.requestsMatching('PUT', /dismissals$/) + expect(dismissals.map(r => r.path)).toEqual([`${repo}/pulls/1/reviews/71/dismissals`]) + expect(dismissals[0].body).toEqual({ message: 'approved removed: no approver covers sdk/x.go; withdrawn by bob (/approve cancel)' }) + expect(gh.requestsMatching('POST', /\/pulls\/1\/reviews$/)).toEqual([]) + }) + }) + it('refuses with a comment a commenter who approves none of the changed files', async () => { routeApprove(['sdk/x.go', 'olm/y.go']) diff --git a/__tests__/plugins/approve.test.ts b/__tests__/plugins/approve.test.ts index 906e751..d7e9651 100644 --- a/__tests__/plugins/approve.test.ts +++ b/__tests__/plugins/approve.test.ts @@ -7,7 +7,7 @@ import { approvalEvents, approveSettings, computeApproval, notifierMarker, rende import { mergeProwConfig } from '../../src/utils/config' import { effectiveOwners, ownersDir, parseOwners } from '../../src/utils/owners' -const defaults: ApproveSettings = { require_self_approval: false, ignore_review_state: false, lgtm_acts_as_approve: false } +const defaults: ApproveSettings = { require_self_approval: false, ignore_review_state: false, lgtm_acts_as_approve: false, github_review: false } // the OWNERS of a pull request as loadPullRequestOwners would resolve them, without the network function pullOwners(ownersFiles: Record, files: string[], author = 'author'): PullRequestOwners { @@ -18,6 +18,7 @@ function pullOwners(ownersFiles: Record, files: string[], author baseSha: 'basesha', author, draft: false, + open: true, requestedReviewers: [], assignees: [], labels: [], @@ -42,9 +43,9 @@ describe('approveSettings', () => { it('applies the Prow defaults and reads every flag', () => { expect(approveSettings({ ...mergeProwConfig({}, {}), sources: [] })).toEqual(defaults) expect(approveSettings({ - ...mergeProwConfig({}, { approve: { require_self_approval: true, ignore_review_state: true, lgtm_acts_as_approve: true } }), + ...mergeProwConfig({}, { approve: { require_self_approval: true, ignore_review_state: true, lgtm_acts_as_approve: true, github_review: true } }), sources: [], - })).toEqual({ require_self_approval: true, ignore_review_state: true, lgtm_acts_as_approve: true }) + })).toEqual({ require_self_approval: true, ignore_review_state: true, lgtm_acts_as_approve: true, github_review: true }) }) }) diff --git a/__tests__/plugins/approveErrorPaths.test.ts b/__tests__/plugins/approveErrorPaths.test.ts index a943869..4cfbb69 100644 --- a/__tests__/plugins/approveErrorPaths.test.ts +++ b/__tests__/plugins/approveErrorPaths.test.ts @@ -17,7 +17,7 @@ beforeAll(() => server.listen(utils.failOnUnhandledRequest)) afterEach(() => server.resetHandlers()) afterAll(() => server.close()) -const defaults: ApproveSettings = { require_self_approval: false, ignore_review_state: false, lgtm_acts_as_approve: false } +const defaults: ApproveSettings = { require_self_approval: false, ignore_review_state: false, lgtm_acts_as_approve: false, github_review: false } const sdkOwners = 'approvers:\n- bob\n' const link = (path: string) => `https://github.com/Codertocat/Hello-World/blob/basesha/${path}` @@ -29,6 +29,7 @@ function pullOwners(ownersFiles: Record, files: string[], author baseSha: 'basesha', author, draft: false, + open: true, requestedReviewers: [], assignees: [], labels: [], diff --git a/__tests__/plugins/approveEvents.test.ts b/__tests__/plugins/approveEvents.test.ts index c5cac42..b4518cb 100644 --- a/__tests__/plugins/approveEvents.test.ts +++ b/__tests__/plugins/approveEvents.test.ts @@ -224,7 +224,7 @@ describe('approveOnPullRequest', () => { expect(debug).toHaveBeenCalledWith('approve: labeled kind/bug does not concern approval') }) - it.each(['closed', 'ready_for_review', 'edited', 'assigned'])('%s is skipped', async (action) => { + it.each(['closed', 'edited', 'assigned'])('%s is skipped', async (action) => { const observeTree = new utils.ObserveRequest() server.use(utils.defaultBranchTree(['OWNERS'], observeTree)) diff --git a/__tests__/plugins/approveReview.test.ts b/__tests__/plugins/approveReview.test.ts new file mode 100644 index 0000000..ae43ae0 --- /dev/null +++ b/__tests__/plugins/approveReview.test.ts @@ -0,0 +1,600 @@ +import type { ApprovalEvent, ApprovalState, ApproveSettings } from '../../src/plugins/approve' +import type { PullRequestOwners } from '../../src/utils/pullRequestOwners' + +import { Buffer } from 'node:buffer' +import * as core from '@actions/core' +import { http } from 'msw' +import { setupServer } from 'msw/node' +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest' + +import { approveOnPullRequest, approveOnReview } from '../../src/plugins/approve' +import { forbiddenWarning, isOwnReview, notPermittedWarning, reviewBody, reviewMarker, syncApprovalReview, tokenIdentity, withdrawalReason } from '../../src/plugins/approveReview' +import { handlePullReq } from '../../src/pullReq/handlePullReq' +import { handlePullReqReview } from '../../src/pullReq/handlePullReqReview' +import labelFileContents from '../fixtures/labels/labelFileContentsResp.json' +import pullReqOpenedEvent from '../fixtures/pullReq/pullReqOpenedEvent.json' +import reviewSubmittedEvent from '../fixtures/pullReq/pullReqReviewSubmittedEvent.json' +import * as utils from '../testUtils' +import { prHandlers, pullBody, repo } from '../utils/ownersFixtures' + +const server = setupServer() +beforeAll(() => server.listen(utils.failOnUnhandledRequest)) +afterEach(() => server.resetHandlers()) +afterAll(() => server.close()) + +const twoDirs = { + 'OWNERS': 'approvers:\n- alice\n', + 'sdk/OWNERS': 'approvers:\n- bob\n', + 'olm/OWNERS': 'options:\n no_parent_owners: true\napprovers:\n- carol\n', +} +const twoFiles = ['sdk/x.go', 'olm/y.go'] +const on = 'approve:\n github_review: true\n' +const head = pullBody.head.sha + +interface Actor { login: string, type?: string } +interface CommentFixture { id?: number, body: string, user: Actor } +interface ReviewFixture { id?: number, state: string, user: Actor, body?: string, commit_id?: string } + +const bot: Actor = { login: 'github-actions[bot]', type: 'Bot' } +const approveBy = (login: string): CommentFixture => ({ body: '/approve', user: { login } }) +const mirrored = (overrides: Partial = {}): ReviewFixture => ({ state: 'APPROVED', user: bot, body: `Approved via /approve by bob, carol (OWNERS).\n${reviewMarker}`, commit_id: head, ...overrides }) + +interface Scenario { + owners?: Record + files?: string[] + author?: string + labels?: string[] + comments?: CommentFixture[] + reviews?: ReviewFixture[] + prowYaml?: string + /** `GET /user`: a login for a user token, a status for an installation token or a failure */ + user?: string | number + /** the answer to `POST /pulls/1/reviews` */ + createReview?: { status: number, body?: unknown } + dismissStatus?: number + pull?: Record +} + +function stamp(index: number): string { + return new Date(Date.UTC(2024, 0, 1, 0, 0, index)).toISOString() +} + +// one approval evaluation; every write is recorded in order +function serve(scenario: Scenario = {}) { + const { + owners = twoDirs, + files = twoFiles, + author = 'some-author', + labels = [], + comments = [], + reviews = [], + prowYaml = on, + user = 403, + createReview = { status: 200, body: { id: 999 } }, + dismissStatus = 200, + pull = {}, + } = scenario + + const calls: string[] = [] + const created: Record[] = [] + const dismissed: { id: string, body: Record }[] = [] + const userRead = new utils.ObserveRequest() + const listReviews = new utils.ObserveRequest() + + const configFile = structuredClone(labelFileContents) + configFile.content = Buffer.from(prowYaml).toString('base64') + + server.use( + ...utils.noOrgOrRepoConfigExcept(...(prowYaml === '' ? [] : ['.github/prow.yaml'])), + ...(prowYaml === '' ? [] : [http.get(utils.contentsUrl('.github/prow.yaml'), utils.mockResponse(200, configFile))]), + ...prHandlers(owners, files, { user: { login: author }, labels: labels.map(name => ({ name })), ...pull }), + utils.defaultBranchTree(Object.keys(owners)), + utils.repoHasLabels(['approved', 'lgtm']), + http.get(`${utils.api}/user`, typeof user === 'string' + ? utils.mockResponse(200, { login: user, type: 'User' }, userRead) + : utils.mockResponse(user, { message: 'Resource not accessible by integration' }, userRead)), + http.get(`${repo}/issues/1/comments`, utils.mockResponse(200, comments.map((c, i) => ({ id: c.id ?? 100 + i, created_at: stamp(i + 1), ...c })))), + http.get(`${repo}/pulls/1/reviews`, utils.mockResponse(200, reviews.map((r, i) => ({ id: r.id ?? 200 + i, submitted_at: stamp(i + 1), ...r })), listReviews)), + http.post(`${repo}/issues/1/labels`, () => { + calls.push('label') + return Response.json([]) + }), + http.delete(`${repo}/issues/1/labels/approved`, () => { + calls.push('unlabel') + return Response.json([]) + }), + http.post(`${repo}/issues/1/comments`, () => { + calls.push('notifier') + return Response.json({}, { status: 201 }) + }), + http.patch(`${repo}/issues/comments/:id`, () => { + calls.push('notifier') + return Response.json({}) + }), + http.post(`${repo}/pulls/1/reviews`, async ({ request }) => { + calls.push('review') + created.push(await request.json() as Record) + return Response.json(createReview.body ?? null, { status: createReview.status }) + }), + http.put(`${repo}/pulls/1/reviews/:id/dismissals`, async ({ request, params }) => { + calls.push('dismiss') + dismissed.push({ id: String(params.id), body: await request.json() as Record }) + return Response.json(dismissStatus === 200 ? {} : { message: 'Server Error' }, { status: dismissStatus }) + }), + ) + + return { calls, created, dismissed, userRead, listReviews } +} + +function prEvent(action: string) { + return new utils.MockContext({ ...pullReqOpenedEvent, action }) +} + +function reviewEvent(action: string, review: Record = {}) { + return new utils.MockContext({ ...reviewSubmittedEvent, action, review: { ...reviewSubmittedEvent.review, ...review } }) +} + +let setFailed: ReturnType +let warning: ReturnType +let debug: ReturnType + +beforeEach(() => { + utils.setupActionsEnv() + setFailed = vi.spyOn(core, 'setFailed').mockImplementation(() => {}) + warning = vi.spyOn(core, 'warning').mockImplementation(() => {}) + debug = vi.spyOn(core, 'debug') +}) + +describe('approve.github_review off (the default)', () => { + it('approved: no GET /user, no review written, the label and notifier as before', async () => { + const writes = serve({ prowYaml: '', comments: [approveBy('bob'), approveBy('carol')] }) + + await approveOnPullRequest(prEvent('opened')) + + expect(writes.calls).toEqual(['label', 'notifier']) + await expect(writes.userRead.notCalled()).resolves.toBe('not called') + expect(writes.created).toEqual([]) + }) + + it('not approved, with an earlier mirrored review on the pull request: nothing is dismissed', async () => { + const writes = serve({ prowYaml: 'approve:\n github_review: false\n', labels: ['approved'], reviews: [mirrored()] }) + + await approveOnPullRequest(prEvent('synchronize')) + + expect(writes.calls).toEqual(['unlabel', 'notifier']) + await expect(writes.userRead.notCalled()).resolves.toBe('not called') + }) + + it('with ignore_review_state not even the reviews are listed', async () => { + const writes = serve({ prowYaml: 'approve:\n ignore_review_state: true\n', comments: [approveBy('bob'), approveBy('carol')] }) + + await approveOnPullRequest(prEvent('opened')) + + expect(writes.calls).toEqual(['label', 'notifier']) + await expect(writes.listReviews.notCalled()).resolves.toBe('not called') + await expect(writes.userRead.notCalled()).resolves.toBe('not called') + }) + + it('a review carrying the marker never counts as an approval, even with the setting off', async () => { + const writes = serve({ prowYaml: '', author: 'carol', reviews: [mirrored({ user: { login: 'bob', type: 'User' } })] }) + + await approveOnPullRequest(prEvent('opened')) + + expect(writes.calls).toEqual(['notifier']) + }) +}) + +describe('approve.github_review: approved', () => { + it('submits one APPROVE review on the head commit, after the label and before nothing else', async () => { + const writes = serve({ comments: [approveBy('bob'), approveBy('carol')] }) + + await approveOnPullRequest(prEvent('opened')) + + expect(writes.calls).toEqual(['label', 'notifier', 'review']) + expect(writes.created).toEqual([{ + commit_id: head, + event: 'APPROVE', + body: reviewBody({ approvers: new Set(['bob', 'carol']) } as ApprovalState), + }]) + expect(writes.created[0].body).toContain('Approved via /approve by bob, carol (OWNERS).') + expect(writes.created[0].body).toContain(reviewMarker) + expect(writes.created[0].body).not.toContain('@') + expect(setFailed).not.toHaveBeenCalled() + }) + + it('is idempotent: the review on the head exists, so a re-run writes nothing', async () => { + const writes = serve({ labels: ['approved'], comments: [approveBy('bob'), approveBy('carol')], reviews: [mirrored()] }) + + await approveOnPullRequest(prEvent('synchronize')) + + expect(writes.created).toEqual([]) + expect(writes.dismissed).toEqual([]) + expect(debug).toHaveBeenCalledWith(`approve: #1 already carries the approval review on ${head}`) + }) + + it('synchronize: the review on the old head stays, a fresh one is submitted on the new head', async () => { + const writes = serve({ labels: ['approved'], comments: [approveBy('bob'), approveBy('carol')], reviews: [mirrored({ id: 7, commit_id: 'oldsha' })] }) + + await approveOnPullRequest(prEvent('synchronize')) + + expect(writes.created).toHaveLength(1) + expect(writes.created[0].commit_id).toBe(head) + expect(writes.dismissed).toEqual([]) + }) + + it('a mirrored review that GitHub dismissed as stale does not count as present', async () => { + const writes = serve({ labels: ['approved'], comments: [approveBy('bob'), approveBy('carol')], reviews: [mirrored({ state: 'DISMISSED' })] }) + + await approveOnPullRequest(prEvent('synchronize')) + + expect(writes.created).toHaveLength(1) + }) + + it('an approving review by someone else on the head, marker included, is not the bot\'s', async () => { + const writes = serve({ labels: ['approved'], comments: [approveBy('bob'), approveBy('carol')], reviews: [mirrored({ user: { login: 'mallory', type: 'User' } })] }) + + await approveOnPullRequest(prEvent('synchronize')) + + expect(writes.created).toHaveLength(1) + }) + + it('a pull request opened by the token\'s own user: warning, no review', async () => { + const writes = serve({ user: 'Prow-Bot', author: 'prow-bot', comments: [approveBy('bob'), approveBy('carol')] }) + + await approveOnPullRequest(prEvent('opened')) + + expect(writes.calls).toEqual(['label', 'notifier']) + expect(warning).toHaveBeenCalledWith('cannot submit the approval review: #1 was opened by the token\'s own identity (prow-bot), and GitHub does not let an author approve their own pull request (approve.github_review)') + expect(setFailed).not.toHaveBeenCalled() + }) + + it('github refusing a self-approval with 422: warning, not failure', async () => { + const writes = serve({ + author: 'github-actions[bot]', + comments: [approveBy('bob'), approveBy('carol')], + createReview: { status: 422, body: { message: 'Unprocessable Entity', errors: ['Can not approve your own pull request'] } }, + }) + + await expect(approveOnPullRequest(prEvent('opened'))).resolves.toBeUndefined() + + expect(writes.created).toHaveLength(1) + expect(warning).toHaveBeenCalledWith('cannot submit the approval review: #1 was opened by the token\'s own identity (github-actions[bot]), and GitHub does not let an author approve their own pull request (approve.github_review)') + }) + + it('a plain 403: warning naming the missing permission, not the repository setting, not failure', async () => { + serve({ + comments: [approveBy('bob'), approveBy('carol')], + createReview: { status: 403, body: { message: 'Resource not accessible by integration' } }, + }) + + await expect(approveOnPullRequest(prEvent('opened'))).resolves.toBeUndefined() + + expect(warning).toHaveBeenCalledWith(`${forbiddenWarning}: Resource not accessible by integration`) + expect(warning).not.toHaveBeenCalledWith(expect.stringContaining('Allow GitHub Actions')) + expect(forbiddenWarning).toBe('cannot submit the approval review: the token was refused; grant the workflow `pull-requests: write` (approve.github_review)') + }) + + it('"GitHub Actions is not permitted to approve pull requests." whatever the status: warning naming the repository setting', async () => { + for (const status of [422, 403]) { + warning.mockClear() + serve({ + comments: [approveBy('bob'), approveBy('carol')], + createReview: { status, body: { message: 'GitHub Actions is not permitted to approve pull requests.' } }, + }) + + await expect(approveOnPullRequest(prEvent('opened'))).resolves.toBeUndefined() + + expect(warning).toHaveBeenCalledWith(`${notPermittedWarning}: GitHub Actions is not permitted to approve pull requests.`) + expect(notPermittedWarning).toBe('cannot submit the approval review: enable "Allow GitHub Actions to create and approve pull requests" (Settings → Actions → General) or pass a token that can (approve.github_review)') + server.resetHandlers() + utils.setupActionsEnv() + } + }) + + it('any other API error fails the evaluation after the label and the notifier were written', async () => { + const writes = serve({ comments: [approveBy('bob'), approveBy('carol')], createReview: { status: 500, body: { message: 'Server Error' } } }) + + await expect(approveOnPullRequest(prEvent('opened'))).rejects.toThrow('could not submit the approval review: HttpError: Server Error') + + expect(writes.calls).toEqual(['label', 'notifier', 'review']) + }) + + it('a draft pull request gets no review until it is ready for review', async () => { + const draft = serve({ comments: [approveBy('bob'), approveBy('carol')], pull: { draft: true } }) + + await approveOnPullRequest(prEvent('opened')) + + expect(draft.calls).toEqual(['label', 'notifier']) + expect(draft.created).toEqual([]) + expect(debug).toHaveBeenCalledWith('approve: #1 is a draft; no approval review is submitted until it is ready for review') + + server.resetHandlers() + utils.setupActionsEnv() + const ready = serve({ labels: ['approved'], comments: [approveBy('bob'), approveBy('carol')] }) + + await approveOnPullRequest(prEvent('ready_for_review')) + + expect(ready.created).toHaveLength(1) + expect(ready.created[0].commit_id).toBe(head) + }) + + it('a draft that loses approved still has its mirrored review dismissed', async () => { + const writes = serve({ labels: ['approved'], reviews: [mirrored({ id: 9 })], pull: { draft: true } }) + + await approveOnPullRequest(prEvent('synchronize')) + + expect(writes.calls).toEqual(['unlabel', 'notifier', 'dismiss']) + expect(writes.dismissed.map(d => d.id)).toEqual(['9']) + }) + + it('a closed pull request gets no review', async () => { + const writes = serve({ comments: [approveBy('bob'), approveBy('carol')], pull: { state: 'closed' } }) + + await approveOnReview(reviewEvent('submitted')) + + expect(writes.calls).toEqual(['label', 'notifier']) + expect(debug).toHaveBeenCalledWith('approve: #1 is not open; its approval review is left alone') + }) + + it('ignore_review_state: the reviews are listed for the mirror but still do not count', async () => { + const writes = serve({ + prowYaml: 'approve:\n github_review: true\n ignore_review_state: true\n', + author: 'carol', + reviews: [{ state: 'APPROVED', user: { login: 'bob' } }], + }) + + await approveOnPullRequest(prEvent('opened')) + + await expect(writes.listReviews.called()).resolves.toBe('called') + expect(writes.calls).toEqual(['notifier']) + }) +}) + +describe('approve.github_review: not approved', () => { + it('/approve cancel dismisses every mirrored review, on any commit, with the reason', async () => { + const writes = serve({ + labels: ['approved'], + comments: [approveBy('bob'), approveBy('carol'), { body: '/approve cancel', user: { login: 'bob' } }], + reviews: [mirrored({ id: 7, commit_id: 'oldsha' }), mirrored({ id: 8 })], + }) + + await approveOnPullRequest(prEvent('synchronize')) + + expect(writes.calls).toEqual(['unlabel', 'notifier', 'dismiss', 'dismiss']) + const message = 'approved removed: no approver covers sdk/x.go; withdrawn by bob (/approve cancel)' + expect(writes.dismissed).toEqual([{ id: '7', body: { message } }, { id: '8', body: { message } }]) + expect(writes.created).toEqual([]) + }) + + it('reviews without the marker, by anyone, and reviews by others carrying it are never dismissed', async () => { + const writes = serve({ + labels: ['approved'], + author: 'carol', + reviews: [ + { state: 'APPROVED', user: bot, commit_id: head }, + { state: 'APPROVED', user: { login: 'dave', type: 'User' }, commit_id: head }, + mirrored({ user: { login: 'mallory', type: 'User' } }), + { state: 'CHANGES_REQUESTED', user: { login: 'bob' } }, + ], + }) + + await approveOnPullRequest(prEvent('synchronize')) + + expect(writes.calls).toEqual(['unlabel', 'notifier']) + expect(debug).toHaveBeenCalledWith('approve: #1 carries no approval review to dismiss') + }) + + it('a CHANGES_REQUESTED review names the reviewer', async () => { + const writes = serve({ + labels: ['approved'], + author: 'carol', + comments: [approveBy('bob')], + reviews: [mirrored({ id: 9 }), { state: 'CHANGES_REQUESTED', user: { login: 'bob' } }], + }) + + await approveOnReview(reviewEvent('submitted')) + + expect(writes.dismissed).toEqual([{ id: '9', body: { message: 'approved removed: no approver covers sdk/x.go; withdrawn by bob (changes requested)' } }]) + }) + + it('a refused dismissal fails the evaluation', async () => { + serve({ labels: ['approved'], reviews: [mirrored({ id: 9 })], dismissStatus: 500 }) + + await expect(approveOnPullRequest(prEvent('synchronize'))).rejects.toThrow('could not dismiss the approval review 9: HttpError: Server Error') + }) +}) + +describe('approve.github_review: the bot\'s own review never approves (user token)', () => { + // prow-bot is an OWNERS approver of every file: if its review counted, approved would hold itself up + const botApprover = { OWNERS: 'approvers:\n- prow-bot\n' } + + it('the mirrored review by the token\'s user is not an approval: approved goes, the review is dismissed', async () => { + const writes = serve({ + owners: botApprover, + files: ['a.go'], + user: 'prow-bot', + labels: ['approved'], + reviews: [mirrored({ id: 5, user: { login: 'Prow-Bot', type: 'User' } })], + }) + + await approveOnPullRequest(prEvent('synchronize')) + + expect(writes.calls).toEqual(['unlabel', 'notifier', 'dismiss']) + expect(writes.dismissed[0].body).toEqual({ message: 'approved removed: no approver covers a.go' }) + }) + + it('nor is any other review by the token\'s user, marker or not', async () => { + const writes = serve({ + owners: botApprover, + files: ['a.go'], + user: 'prow-bot', + reviews: [{ state: 'APPROVED', user: { login: 'prow-bot', type: 'User' }, commit_id: head }], + }) + + await approveOnPullRequest(prEvent('opened')) + + expect(writes.calls).toEqual(['notifier']) + }) + + it('a failing GET /user fails the evaluation before anything is written', async () => { + const writes = serve({ user: 500, comments: [approveBy('bob'), approveBy('carol')] }) + + await expect(approveOnPullRequest(prEvent('opened'))).rejects.toThrow('could not identify the token for approve.github_review') + expect(writes.calls).toEqual([]) + }) +}) + +describe('approve.github_review: no loops', () => { + it('pull_request_review for the mirrored review evaluates nothing (submitted and dismissed)', async () => { + const observeTree = new utils.ObserveRequest() + server.use(utils.defaultBranchTree(['OWNERS'], observeTree)) + + for (const action of ['submitted', 'dismissed']) { + await approveOnReview(reviewEvent(action, { id: 42, body: `Approved via /approve by bob (OWNERS).\n${reviewMarker}` })) + expect(debug).toHaveBeenCalledWith('approve: review 42 is the approval review this action mirrors; nothing to evaluate') + } + await expect(observeTree.notCalled()).resolves.toBe('not called') + }) + + it('handlePullReqReview on the mirrored review: approve writes nothing, tide still evaluates', async () => { + const writes = serve({ labels: ['approved'], comments: [approveBy('bob'), approveBy('carol')], reviews: [mirrored()] }) + + await handlePullReqReview(reviewEvent('submitted', { body: mirrored().body, user: bot })) + + expect(writes.calls).toEqual([]) + expect(setFailed).not.toHaveBeenCalled() + }) + + it('a second run on the same event submits no second review', async () => { + const first = serve({ comments: [approveBy('bob'), approveBy('carol')] }) + await approveOnPullRequest(prEvent('reopened')) + expect(first.created).toHaveLength(1) + + server.resetHandlers() + utils.setupActionsEnv() + const second = serve({ labels: ['approved'], comments: [approveBy('bob'), approveBy('carol')], reviews: [mirrored()] }) + await approveOnPullRequest(prEvent('reopened')) + expect(second.created).toEqual([]) + }) +}) + +describe('approve.github_review: order within one run', () => { + it('label, review, then the merge evaluation of the same run', async () => { + utils.setupJobsEnv('') + const writes = serve({ owners: { OWNERS: 'approvers:\n- alice\n' }, files: ['src/a.go'], author: 'alice', labels: ['lgtm'] }) + server.use( + utils.lgtmStatus(head), + http.put(`${repo}/pulls/1/merge`, () => { + writes.calls.push('merge') + return Response.json({ merged: true }) + }), + ) + let pulls = 0 + server.use(http.get(`${repo}/pulls/1`, () => { + pulls++ + return Response.json({ ...pullBody, user: { login: 'alice' }, labels: (pulls === 1 ? ['lgtm'] : ['lgtm', 'approved']).map(name => ({ name })) }) + })) + + await handlePullReq(prEvent('reopened')) + + expect(writes.calls).toEqual(['label', 'notifier', 'review', 'merge']) + expect(setFailed).not.toHaveBeenCalled() + }) + + it('a failed review is reported at the end of the run; tide still evaluates', async () => { + utils.setupJobsEnv('') + const writes = serve({ comments: [approveBy('bob'), approveBy('carol')], createReview: { status: 500, body: { message: 'Server Error' } } }) + let pulls = 0 + server.use(http.get(`${repo}/pulls/1`, () => { + pulls++ + return Response.json({ ...pullBody, user: { login: 'some-author' }, labels: [] }) + })) + + await handlePullReq(prEvent('reopened')) + + expect(writes.calls).toEqual(['label', 'notifier', 'review']) + // the owners read, then tide's own read + expect(pulls).toBe(2) + expect(core.info).toHaveBeenCalledWith('skipping pr #1: missing lgtm') + expect(setFailed).toHaveBeenCalledWith(expect.stringContaining('could not submit the approval review: HttpError: Server Error')) + }) +}) + +describe('tokenIdentity', () => { + it('a 404 on GET /user is an installation token too, and the answer is memoized per client', async () => { + const getAuthenticated = vi.fn().mockRejectedValue(Object.assign(new Error('Not Found'), { status: 404 })) + const octokit = { users: { getAuthenticated } } as unknown as Parameters[0] + + await expect(tokenIdentity(octokit)).resolves.toEqual({}) + await expect(tokenIdentity(octokit)).resolves.toEqual({}) + expect(getAuthenticated).toHaveBeenCalledTimes(1) + }) + + it('a rejection without a status fails', async () => { + const octokit = { users: { getAuthenticated: vi.fn().mockRejectedValue('boom') } } as unknown as Parameters[0] + + await expect(tokenIdentity(octokit)).rejects.toThrow('could not identify the token for approve.github_review: boom') + }) +}) + +describe('syncApprovalReview', () => { + it('a non-Error rejection of the review is an error', async () => { + const octokit = { pulls: { createReview: vi.fn().mockRejectedValue('boom') } } as unknown as Parameters[0] + const input = { + owners: { number: 1, open: true, headSha: head, author: 'someone' } as PullRequestOwners, + state: { approved: true, approvers: new Set(['bob']) } as ApprovalState, + reviews: [], + identity: {}, + reason: () => '', + } + + await expect(syncApprovalReview(octokit, prEvent('opened'), input)).rejects.toThrow('could not submit the approval review: boom') + }) +}) + +describe('withdrawalReason', () => { + const settings: ApproveSettings = { require_self_approval: false, ignore_review_state: false, lgtm_acts_as_approve: false, github_review: true } + const owners = (files: string[]) => ({ number: 1, files } as PullRequestOwners) + const state = (uncoveredFiles: string[]) => ({ uncoveredFiles } as ApprovalState) + const event = (user: string, kind: ApprovalEvent['kind'], at: number): ApprovalEvent => ({ user, kind, at: new Date(at) }) + + it('a pull request without changed files', () => { + expect(withdrawalReason(owners([]), state([]), [], settings)).toBe('the pull request changes no files') + }) + + it('lists five files at most', () => { + const files = ['a', 'b', 'c', 'd', 'e', 'f', 'g'] + expect(withdrawalReason(owners(files), state(files), [], settings)).toBe('no approver covers a, b, c, d, e and 2 more') + }) + + it('names who withdrew last, and /lgtm cancel only when lgtm acts as approve', () => { + const events = [ + event('Bob', 'approve', 1), + event('bob', 'cancel', 2), + event('carol', 'lgtm-cancel', 3), + event('dave', 'cancel', 4), + event('dave', 'approve', 5), + ] + expect(withdrawalReason(owners(['x']), state(['x']), events, settings)).toBe('no approver covers x; withdrawn by bob (/approve cancel)') + expect(withdrawalReason(owners(['x']), state(['x']), events, { ...settings, lgtm_acts_as_approve: true })) + .toBe('no approver covers x; withdrawn by bob (/approve cancel), carol (/lgtm cancel)') + }) +}) + +describe('isOwnReview', () => { + const body = `x\n${reviewMarker}` + + it('an installation token owns marked reviews by bots', () => { + expect(isOwnReview({ id: 1, state: 'APPROVED', body, user: bot }, {})).toBe(true) + expect(isOwnReview({ id: 1, state: 'APPROVED', body, user: { login: 'other-app[bot]', type: 'Bot' } }, {})).toBe(true) + expect(isOwnReview({ id: 1, state: 'APPROVED', body, user: { login: 'bob', type: 'User' } }, {})).toBe(false) + expect(isOwnReview({ id: 1, state: 'APPROVED', body: 'no marker', user: bot }, {})).toBe(false) + expect(isOwnReview({ id: 1, state: 'APPROVED', user: bot }, {})).toBe(false) + }) + + it('a user token owns marked reviews by its own login only', () => { + expect(isOwnReview({ id: 1, state: 'APPROVED', body, user: { login: 'Prow-Bot' } }, { login: 'prow-bot' })).toBe(true) + expect(isOwnReview({ id: 1, state: 'APPROVED', body, user: bot }, { login: 'prow-bot' })).toBe(false) + expect(isOwnReview({ id: 1, state: 'APPROVED', body, user: null }, { login: 'prow-bot' })).toBe(false) + }) +}) diff --git a/__tests__/utils/config.test.ts b/__tests__/utils/config.test.ts index 62a7c1a..4798a6d 100644 --- a/__tests__/utils/config.test.ts +++ b/__tests__/utils/config.test.ts @@ -306,8 +306,8 @@ describe('parseProwConfig', () => { describe('approve', () => { it('accepts every flag', () => { - expect(parseProwConfig('x', 'approve:\n require_self_approval: true\n ignore_review_state: true\n lgtm_acts_as_approve: false\n')).toEqual({ - approve: { require_self_approval: true, ignore_review_state: true, lgtm_acts_as_approve: false }, + expect(parseProwConfig('x', 'approve:\n require_self_approval: true\n ignore_review_state: true\n lgtm_acts_as_approve: false\n github_review: true\n')).toEqual({ + approve: { require_self_approval: true, ignore_review_state: true, lgtm_acts_as_approve: false, github_review: true }, }) }) @@ -320,6 +320,7 @@ describe('parseProwConfig', () => { ['a non-boolean require_self_approval', 'approve:\n require_self_approval: yes please\n', 'x: approve.require_self_approval must be a boolean'], ['a non-boolean ignore_review_state', 'approve:\n ignore_review_state: 1\n', 'x: approve.ignore_review_state must be a boolean'], ['a non-boolean lgtm_acts_as_approve', 'approve:\n lgtm_acts_as_approve: [true]\n', 'x: approve.lgtm_acts_as_approve must be a boolean'], + ['a non-boolean github_review', 'approve:\n github_review: \'true\'\n', 'x: approve.github_review must be a boolean'], ])('rejects %s', (_, text, error) => { expect(() => parseProwConfig('x', text)).toThrow(error) }) diff --git a/dist/index.js b/dist/index.js index 1ad636e..3287e28 100644 --- a/dist/index.js +++ b/dist/index.js @@ -41188,7 +41188,7 @@ function normalizeBlunderbuss(source, raw) { ignore_authors: raw.ignore_authors, }); } -const approveFlags = ['require_self_approval', 'ignore_review_state', 'lgtm_acts_as_approve']; +const approveFlags = ['require_self_approval', 'ignore_review_state', 'lgtm_acts_as_approve', 'github_review']; function normalizeApprove(source, raw) { if (!isMapping(raw)) { throw new Error(`${source}: approve must be a mapping`); @@ -42070,6 +42070,7 @@ async function pullRequestOwners_load(octokit, context, pullNumber) { headSha: pull.head.sha, author: (pull.user?.login ?? '').toLowerCase(), draft: pull.draft === true, + open: pull.state === 'open', requestedReviewers: (pull.requested_reviewers ?? []).map(user => user.login.toLowerCase()), assignees: (pull.assignees ?? []).map(user => user.login.toLowerCase()), labels: (pull.labels ?? []).map(label => label.name), @@ -44012,6 +44013,195 @@ async function tryMergePr(pr, octokit, context = github_context, policy, failure return successfulResults.has(verdict.result); } +;// CONCATENATED MODULE: ./lib/plugins/approveReview.js + + +/** identifies the APPROVE review that mirrors the `approved` label (`approve.github_review`) */ +const reviewMarker = ''; +// memoized per client: GET /user is read at most once per handler run +const identities = new WeakMap(); +/** + * tokenIdentity asks GitHub who the token is (`GET /user`), once per client. + * An installation token, `GITHUB_TOKEN` included, may not read `/user` and + * is answered with 403 (404 on some servers): its reviews are authored by a + * `Bot` user, which the approve plugin never counts anyway, so the login is + * left undefined. Any other failure fails the evaluation. + * + * @param octokit - a hydrated github client + */ +function tokenIdentity(octokit) { + let pending = identities.get(octokit); + if (pending === undefined) { + pending = octokit.users.getAuthenticated().then(({ data }) => ({ login: data.login.toLowerCase() }), (e) => { + const status = errorStatus(e); + if (status === 403 || status === 404) { + core_debug(`approve: GET /user answered ${status}; the token is an installation token`); + return {}; + } + throw new Error(`could not identify the token for approve.github_review: ${e}`); + }); + identities.set(octokit, pending); + } + return pending; +} +/** + * isOwnReview reports whether a review is the mirrored approval this action + * submitted: it carries the marker and was written by the token's identity + * (any `Bot` user when the token is an installation token). + * + * @param review - a review of the pull request + * @param identity - who the token is + */ +function isOwnReview(review, identity) { + if (!(review.body ?? '').includes(reviewMarker)) { + return false; + } + const login = review.user?.login?.toLowerCase(); + return identity.login === undefined ? isBotUser(review.user) : login === identity.login; +} +/** + * reviewBody is the text of the mirrored approval: who approved, without + * an `@`, since every push submits a fresh review and a mention would notify + * the approvers each time. + * + * @param state - the computed approval + */ +function reviewBody(state) { + return [ + `Approved via /approve by ${[...state.approvers].join(', ')} (OWNERS).`, + '', + 'This review mirrors the `approved` label: it is submitted while the label is set and dismissed when the label goes away. Use `/approve` and `/approve cancel` to change it.', + reviewMarker, + ].join('\n'); +} +const reasonFiles = 5; +const withdrawals = { + 'cancel': '/approve cancel', + 'review-changes': 'changes requested', + 'lgtm-cancel': '/lgtm cancel', +}; +/** + * withdrawalReason says why a pull request is not approved, for the + * dismissal message: the files nobody covers and, when someone withdrew + * their approval last, who and how. + * + * @param owners - the pull request and the OWNERS covering its files + * @param state - the computed approval + * @param events - what users did on the pull request, as counted + * @param settings - the resolved `approve` configuration + */ +function withdrawalReason(owners, state, events, settings) { + if (owners.files.length === 0) { + return 'the pull request changes no files'; + } + const shown = state.uncoveredFiles.slice(0, reasonFiles).join(', '); + const more = state.uncoveredFiles.length > reasonFiles ? ` and ${state.uncoveredFiles.length - reasonFiles} more` : ''; + const reason = `no approver covers ${shown}${more}`; + const latest = new Map(); + for (const event of [...events].sort((a, b) => a.at.getTime() - b.at.getTime())) { + if (event.kind.startsWith('lgtm') && !settings.lgtm_acts_as_approve) { + continue; + } + latest.set(event.user.toLowerCase(), event); + } + const withdrawn = [...latest.entries()] + .filter(([, event]) => withdrawals[event.kind] !== undefined) + .map(([user, event]) => `${user} (${withdrawals[event.kind]})`) + .sort(); + return withdrawn.length === 0 ? reason : `${reason}; withdrawn by ${withdrawn.join(', ')}`; +} +const notPermittedWarning = 'cannot submit the approval review: enable "Allow GitHub Actions to create and approve pull requests" (Settings → Actions → General) or pass a token that can (approve.github_review)'; +const forbiddenWarning = 'cannot submit the approval review: the token was refused; grant the workflow `pull-requests: write` (approve.github_review)'; +/** + * syncApprovalReview makes the action's own APPROVE review follow the + * `approved` label (`approve.github_review`). Approved: one review by the + * token on the current head commit, submitted unless it already exists. + * A draft gets none until it is ready for review. Not approved: every such + * review the action submitted earlier is dismissed, on any commit, drafts + * included. Reviews without the marker, or by anyone else, are never + * touched. GitHub refusing the approval itself (the repository does not let + * Actions approve, the token lacks `pull-requests: write`, or the token + * authored the pull request) is a warning; any other API error fails the + * evaluation. + * + * @param octokit - a hydrated github client + * @param context - the github context of the current action event + * @param input - see MirrorInput + */ +async function syncApprovalReview(octokit, context, input) { + const { owners, state, reviews, identity } = input; + const number = owners.number; + if (!owners.open) { + core_debug(`approve: #${number} is not open; its approval review is left alone`); + return; + } + const own = reviews.filter(review => review.state === 'APPROVED' && isOwnReview(review, identity)); + if (!state.approved) { + if (own.length === 0) { + core_debug(`approve: #${number} carries no approval review to dismiss`); + return; + } + const message = `approved removed: ${input.reason()}`; + for (const review of own) { + try { + await octokit.pulls.dismissReview({ ...context.repo, pull_number: number, review_id: review.id, message }); + } + catch (e) { + throw new Error(`could not dismiss the approval review ${review.id}: ${e}`); + } + info(`approve: dismissed the approval review ${review.id} on #${number}: ${message}`); + } + return; + } + if (own.some(review => review.commit_id === owners.headSha)) { + core_debug(`approve: #${number} already carries the approval review on ${owners.headSha}`); + return; + } + if (owners.draft) { + core_debug(`approve: #${number} is a draft; no approval review is submitted until it is ready for review`); + return; + } + if (identity.login !== undefined && identity.login === owners.author) { + warning(selfApprovalWarning(number, identity.login)); + return; + } + try { + await octokit.pulls.createReview({ + ...context.repo, + pull_number: number, + commit_id: owners.headSha, + event: 'APPROVE', + body: reviewBody(state), + }); + } + catch (e) { + const message = approveReview_errorMessage(e); + if (/approve your own pull request/i.test(message)) { + warning(selfApprovalWarning(number, owners.author)); + return; + } + if (/not permitted to approve pull requests/i.test(message)) { + warning(`${notPermittedWarning}: ${message}`); + return; + } + if (errorStatus(e) === 403) { + warning(`${forbiddenWarning}: ${message}`); + return; + } + throw new Error(`could not submit the approval review: ${e}`); + } + info(`approve: submitted the approval review on #${number} at ${owners.headSha}`); +} +function selfApprovalWarning(number, login) { + return `cannot submit the approval review: #${number} was opened by the token's own identity (${login}), and GitHub does not let an author approve their own pull request (approve.github_review)`; +} +function errorStatus(e) { + return typeof e === 'object' && e !== null && 'status' in e && typeof e.status === 'number' ? e.status : undefined; +} +function approveReview_errorMessage(e) { + return e instanceof Error ? e.message : String(e); +} + ;// CONCATENATED MODULE: ./lib/plugins/approve.js @@ -44022,10 +44212,11 @@ async function tryMergePr(pr, octokit, context = github_context, policy, failure + const approvedLabel = 'approved'; const notifierMarker = ''; const commandsDoc = 'https://github.com/cncf/prow-github-actions/blob/main/docs/commands.md'; -const approve_pullRequestActions = new Set(['opened', 'reopened', 'synchronize', 'labeled', 'unlabeled']); +const approve_pullRequestActions = new Set(['opened', 'reopened', 'synchronize', 'ready_for_review', 'labeled', 'unlabeled']); const approve_reviewActions = new Set(['submitted', 'dismissed']); /** * approveSettings resolves the `approve` configuration with Prow's defaults: @@ -44039,6 +44230,7 @@ function approveSettings(config) { require_self_approval: raw.require_self_approval ?? false, ignore_review_state: raw.ignore_review_state ?? false, lgtm_acts_as_approve: raw.lgtm_acts_as_approve ?? false, + github_review: raw.github_review ?? false, }; } /** @@ -44046,11 +44238,15 @@ function approveSettings(config) { * comments and the APPROVED / CHANGES_REQUESTED reviews of humans into events, * logins lowercased. Bots, other review states and comments without a command * yield nothing; a comment carrying both a command and its cancel is a cancel. + * The approval review this action mirrors (`approve.github_review`) never + * counts: a review carrying its marker is skipped, and so is every review by + * `tokenLogin`, the token's own user, so that `approved` cannot hold itself up. * * @param comments - the issue comments of the pull request * @param reviews - the reviews of the pull request + * @param tokenLogin - the login behind the workflow token, when it is a user token */ -function approvalEvents(comments, reviews) { +function approvalEvents(comments, reviews, tokenLogin) { const events = []; for (const comment of comments) { const login = humanLogin(comment.user); @@ -44070,7 +44266,7 @@ function approvalEvents(comments, reviews) { } for (const review of reviews) { const login = humanLogin(review.user); - if (login === undefined || review.submitted_at == null) { + if (login === undefined || review.submitted_at == null || login === tokenLogin || (review.body ?? '').includes(reviewMarker)) { continue; } const at = new Date(review.submitted_at); @@ -44277,8 +44473,10 @@ function ownersEntries(state, owners) { * evaluateApproval recomputes the approval of a pull request from its * comments and reviews, then makes the `approved` label and the notifier * comment match: the label is added or removed only when it changes, the - * notifier is posted once and edited in place afterwards. A pull request - * whose base branch has no OWNERS files is left alone. + * notifier is posted once and edited in place afterwards. With + * `approve.github_review` the token's own APPROVE review then follows the + * label, before any merge evaluation of the same run. A pull request whose + * base branch has no OWNERS files is left alone. * * @param octokit - a hydrated github client * @param context - the github context of the current action event @@ -44292,13 +44490,19 @@ async function evaluateApproval(octokit, context, pullNumber) { } const settings = approveSettings(await loadProwConfig(octokit, context)); const comments = await listComments(octokit, context, pullNumber); - const reviews = settings.ignore_review_state ? [] : await listReviews(octokit, context, pullNumber); - const state = computeApproval(owners, approvalEvents(comments, reviews), settings); + // the mirrored review needs the reviews and the token's identity; without it neither is read + const identity = settings.github_review ? await tokenIdentity(octokit) : undefined; + const reviews = settings.ignore_review_state && identity === undefined ? [] : await listReviews(octokit, context, pullNumber); + const events = approvalEvents(comments, settings.ignore_review_state ? [] : reviews, identity?.login); + const state = computeApproval(owners, events, settings); info(state.approved ? `approve: #${pullNumber} is approved by ${[...state.approvers].join(', ')}` : `approve: #${pullNumber} is not approved; nobody approves ${state.uncoveredFiles.join(', ') || 'anything'}`); await syncLabel(octokit, context, owners, state.approved); await upsertNotifier(octokit, context, pullNumber, comments, renderNotifier(state, owners, context.repo)); + if (identity !== undefined) { + await syncApprovalReview(octokit, context, { owners, state, reviews, identity, reason: () => withdrawalReason(owners, state, events, settings) }); + } } async function syncLabel(octokit, context, owners, approved) { const present = owners.labels.filter(label => label.toLowerCase() === approvedLabel); @@ -44350,8 +44554,9 @@ async function listReviews(octokit, context, pullNumber) { } /** * approveOnPullRequest is the `pull_request` handler: on `opened`, - * `reopened` and `synchronize`, and when a human adds or removes the - * `approved` label, it re-evaluates the approval. Approval is sticky across + * `reopened`, `synchronize` and `ready_for_review` (a draft gets no mirrored + * review until then), and when a human adds or removes the `approved` label, + * it re-evaluates the approval. Approval is sticky across * pushes; a push only matters because the changed files may differ. * * @param context - the github context of the current action event @@ -44371,6 +44576,8 @@ async function approveOnPullRequest(context = github_context) { /** * approveOnReview is the `pull_request_review` handler: a submitted or * dismissed review may add (APPROVED) or remove (CHANGES_REQUESTED) an approver. + * The approval review this action mirrors is an output, never an input: its + * own events (fired when the token is a user token) evaluate nothing. * * @param context - the github context of the current action event */ @@ -44380,6 +44587,10 @@ async function approveOnReview(context = github_context) { core_debug(`approve: skipping ${action} review action`); return; } + if (String(context.payload.review?.body ?? '').includes(reviewMarker)) { + core_debug(`approve: review ${context.payload.review?.id} is the approval review this action mirrors; nothing to evaluate`); + return; + } await evaluateOnOwnersRepo(context, context.payload.pull_request?.number); } async function evaluateOnOwnersRepo(context, pullNumber) { diff --git a/docs/automatic-merging.md b/docs/automatic-merging.md index ec9c480..3427b05 100644 --- a/docs/automatic-merging.md +++ b/docs/automatic-merging.md @@ -404,20 +404,106 @@ Steps: 1. Run the [`label-sync` job](./cron-jobs.md#label-sync), or create `approved` by hand. Until it exists an evaluation that wants to add it fails with `the label(s) approved cannot be applied because the repository doesn't have them`. -2. Subscribe the workflow to `pull_request` (`opened`, `reopened`, `synchronize`, `labeled`, - `unlabeled`) and `pull_request_review` (`submitted`, `dismissed`) so approvals from reviews and +2. Subscribe the workflow to `pull_request` (`opened`, `reopened`, `synchronize`, + `ready_for_review`, `labeled`, `unlabeled`) and `pull_request_review` (`submitted`, `dismissed`) so approvals from reviews and authorship are picked up ([events](./events.md)). `/approve` comments work with `issue_comment` alone. 3. Open PRs need an approver: an author who owns every changed file is approved on the next evaluation; anyone else needs `/approve` (or an approving review) from the OWNERS approvers. 4. If branch protection relied on the bot's approving review to satisfy "required approving reviews", that review is no longer submitted. Either let this action's merge gate be the approval signal (`approved` + `lgtm`, then it merges), or lower the required review count and - let a human's review, which the plugin also counts, satisfy the protection. + let a human's review, which the plugin also counts, satisfy the protection, or set + [`approve.github_review: true`](./commands.md#mirroring-approved-as-a-github-review) so that the + bot keeps an approving review on the head while `approved` is set + ([required reviews and OpenSSF Scorecard](#required-reviews-and-openssf-scorecard)). 5. To keep merging on `lgtm` alone, pin the gate: `tide: { labels: [lgtm] }`. The `approved` label and the notifier are still maintained for information. This repository has no OWNERS files, so its own workflows are unaffected. +## Required reviews and OpenSSF Scorecard + +A Prow-gated repository (OWNERS, `/lgtm`, `/approve`, the required `prow/lgtm` status) decides +approval with labels, not GitHub reviews, so its branch protection or ruleset typically requires +zero approving reviews. Requiring one would make every pull request need a GitHub review on top +of `/approve`. [`approve.github_review: true`](./commands.md#mirroring-approved-as-a-github-review) +closes that gap: while the PR carries `approved`, the token keeps an `APPROVE` review on the head +commit, and dismisses it when `approved` goes away. + +```yaml +approve: + github_review: true +``` + +What it is | What it is not +--- | --- +the approve plugin's verdict (every changed file covered by an OWNERS approver) mirrored as one review by the token | an extra human review: nobody looked at the code twice +an approving review for "Require a pull request before merging" with 1 required approval in branch protection; a ruleset's "Required approvals" is expected to count it the same way (not verified here) | a **code owner** review: "Require review from Code Owners" still needs an approval from a user or team that CODEOWNERS names, which `github-actions[bot]` is not +gone the moment `approved` is (cancel, `CHANGES_REQUESTED`, files no longer covered) | a way to approve: the bot's own review never counts toward `approved` + +**Repository setting.** With `GITHUB_TOKEN`, *Settings → Actions → General → Workflow +permissions → Allow GitHub Actions to create and approve pull requests* must be on (on an +organization's repositories the organization may force it off). Otherwise GitHub refuses the +review with `GitHub Actions is not permitted to approve pull requests.` and the run logs a warning +instead of failing. A GitHub App token or a machine user's PAT passed as the `token` secret +([installing](./installing.md#inputs-and-secrets)) does not need the setting; it must not be the +author of the pull requests it approves. The workflow permissions do not change: +`pull-requests: write` is already required. + +> [!WARNING] +> "Allow GitHub Actions to create and approve pull requests" is a **repository-wide** switch, not +> a grant to this action. Once it is on, every workflow in the repository can approve pull +> requests with `GITHUB_TOKEN`, including one that a collaborator with write access adds on their +> own branch. The safer setup leaves the setting off and passes a dedicated GitHub App token or a +> machine user's token as the `token` secret, so that only this action's identity can approve. + +**Stale-approval rules are neutralized.** `approved` is [sticky](./commands.md#approve): it stays +on a pull request across pushes, so the bot approves every new head again. That makes GitHub's +"Dismiss stale pull request approvals when new commits are pushed" and "Require approval of the +most recent reviewable push" ineffective as protections with this setting on: neither forces a +person to look at the new commits. The protection after a push is the `lgtm` side of the gate: +`lgtm` is [removed on every push](#lgtm-is-bound-to-a-commit) and the `prow/lgtm` status no longer +matches the new head, so **`prow/lgtm` must stay a required status check**. + +**Dismiss stale reviews.** With "Dismiss stale pull request approvals when new commits are pushed" +GitHub dismisses the bot's review on a push; the `synchronize` run submits a fresh one on the new +head while `approved` stays. Without it the old review stays and the new one joins it. Whether GitHub's own dismissal can land after the fresh +review, and dismiss that too, is not documented; the run's log and the PR's timeline show it if +it does, and the next approval evaluation (a comment, a review, a push, the +[`sweep` job](./cron-jobs.md#sweep)) puts it back. + +**Require approval of the most recent reviewable push.** GitHub wants the latest push approved by +someone other than its pusher. The fresh review on every new head is what satisfies it, as long +as the token's identity is not the pusher; as above, it does not mean anyone reviewed that push. + +**Refused dismissals.** If GitHub refuses to dismiss the bot's review (for example "Restrict who +can dismiss pull request reviews" does not include the token's identity), every approval +evaluation of that pull request fails with `could not dismiss the approval review ` until +someone allowed to dismisses the review by hand. + +**Drafts.** A draft pull request gets no review; it is submitted once the PR is marked ready for +review (the `ready_for_review` event). Dismissals still apply to drafts. + +**Mergeability right after the review.** The review is submitted before the merge evaluation of +the same run, so that evaluation reads the pull request after the review exists. GitHub computes +`mergeable_state` lazily; the [`unknown` retries](#unknown-github-computes-mergeability-lazily) +cover an `unknown` answer, but whether the first read can instead return the previous `blocked` +is not documented. If it does, the pull request is skipped as `not mergeable (blocked)` and the +next event or the cron merges it. With `GITHUB_TOKEN` the review fires no `pull_request_review` +event of its own; with a user token it does, and that run re-evaluates the merge. + +**Scorecard.** The [Branch-Protection](https://github.com/ossf/scorecard/blob/main/docs/checks.md#branch-protection) +check scores in tiers and credits a tier only once the previous one is complete. Tier 2 (6/10) +includes "Require at least 1 reviewer for approval before merging"; tier 3 (8/10) "Require +branch to pass at least 1 status check before merging". A repository that prevents force pushes +and deletion and requires the `prow/lgtm` status but no reviewer stops in tier 2 with partial +credit (4/10 observed); requiring one approving review completes tiers 2 and 3 (8/10). Tier 4 +asks for 2 reviewers and code owner review. The check reads the required review count; it does +not judge who approves. Scorecard's separate Code-Review check does not count bot reviews and +already recognizes Prow's `lgtm` and `approved` labels; this setting does not change it. The +required-review credit is nominal with this setup: the review mirrors the Prow decision, it is not +an independent control. + ## Upgrading from the `hold` label **BREAKING.** `/hold` used to apply `hold`; it now applies Prow's `do-not-merge/hold`. diff --git a/docs/commands.md b/docs/commands.md index cb19ac6..acad1ee 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -232,15 +232,63 @@ which the [`lgtm` PR job](./pr-jobs.md) removes on every push. vouches for); a changed file no OWNERS file covers can never be approved and is listed as such in the notifier. `/approve cancel` by someone who never approved is a no-op re-evaluation. -**No bot review.** On repositories with OWNERS files the bot submits no GitHub review any more: -a review by `github-actions[bot]` would satisfy branch protection's "required approving reviews" -on its own, which is the wrong signal once `approved` is what the [merge gate](./automatic-merging.md#the-merge-gate) -requires. `/approve cancel` therefore dismisses nothing; it just recomputes. - -Events that evaluate: `pull_request` `opened`, `reopened`, `synchronize`, `labeled`/`unlabeled` of -`approved`; `pull_request_review` `submitted`, `dismissed`; and the `/approve` family of comments. +**No bot review by default.** On repositories with OWNERS files the bot submits no GitHub review +of its own accord: a review by `github-actions[bot]` would satisfy branch protection's "required +approving reviews" on its own, which is the wrong signal once `approved` is what the +[merge gate](./automatic-merging.md#the-merge-gate) requires. `/approve cancel` therefore dismisses +nothing; it just recomputes. A repository that wants a required review to follow the Prow decision +opts in with [`approve.github_review`](#mirroring-approved-as-a-github-review). + +Events that evaluate: `pull_request` `opened`, `reopened`, `synchronize`, `ready_for_review`, +`labeled`/`unlabeled` of `approved`; `pull_request_review` `submitted`, `dismissed`; and the `/approve` family of comments. See [events](./events.md). +#### Mirroring `approved` as a GitHub review + +With [`approve.github_review: true`](./configuration.md#approve) (default `false`) the token keeps +an `APPROVE` review on the pull request's head commit for as long as the PR carries `approved`, +and dismisses it when `approved` goes away. Humans keep using `/approve` (and `/lgtm`); a branch +protection rule or ruleset that requires one approving review is then met by the Prow decision, +without anyone approving twice. It is the plugin's verdict mirrored, not an extra human review +([required reviews and OpenSSF Scorecard](./automatic-merging.md#required-reviews-and-openssf-scorecard)). +Only repositories with OWNERS files are affected; without them `/approve` submits a bot review +anyway ([below](#repositories-without-owners-files)). + +Situation | Action +--- | --- +the evaluation ends with `approved` | one `APPROVED` review by the token on the **current head** (`commit_id`); none is written when it already exists. Body: `Approved via /approve by (OWNERS).`, an explanation and the hidden marker ``. The approvers are named without `@`, so a fresh review after each push notifies nobody +the evaluation ends without `approved` (`/approve cancel`, a `CHANGES_REQUESTED` review, files no longer covered, ...) | every `APPROVED` review by the token that carries the marker is dismissed, on any commit, with `approved removed: `, ex: `approved removed: no approver covers sdk/x.go; withdrawn by bob (/approve cancel)` +`synchronize` (a new head) while `approved` stays | a fresh review on the new head; the one on the old commit is left as it is (GitHub may have dismissed it as stale). Reviews without the marker are never touched +the PR's author is the token's own identity (a PR the bot opened, or a `token` secret of the author) | GitHub does not let an author approve their own pull request: warning `cannot submit the approval review: # was opened by the token's own identity (), and GitHub does not let an author approve their own pull request (approve.github_review)`, no review +GitHub refuses with `GitHub Actions is not permitted to approve pull requests.` (reported as a 422 when "Allow GitHub Actions to create and approve pull requests" is off; GitHub does not document the status) | warning, not failure: `cannot submit the approval review: enable "Allow GitHub Actions to create and approve pull requests" (Settings → Actions → General) or pass a token that can (approve.github_review): ` +GitHub refuses with any other 403 (ex: the workflow lacks `pull-requests: write`) | warning, not failure: ``cannot submit the approval review: the token was refused; grant the workflow `pull-requests: write` (approve.github_review): `` +any other API error | `could not submit the approval review: ` or `could not dismiss the approval review : `, an error annotation; the run fails at the end, after the merge evaluation +the pull request is closed | nothing +the pull request is a draft | no review is submitted until it is marked ready for review (`ready_for_review` re-evaluates); dismissals still happen +GitHub refuses the dismissal (ex: "Restrict who can dismiss pull request reviews" excludes the token) | `could not dismiss the approval review : ` on every evaluation of the pull request until the review is dismissed by hand + +The order inside one run is: compute the approval → write the label → edit the notifier → +submit or dismiss the review → the merge evaluation, so the merge evaluation of the same run starts +after the review exists (whether GitHub's `mergeable_state` already reflects it is another matter, +see [required reviews](./automatic-merging.md#required-reviews-and-openssf-scorecard)). + +**The bot's review never approves.** It is an output, never an input: a review carrying the +marker never counts as an approval, and with the setting on neither does any review by the +token's own user (when `token` is a user's PAT; `GITHUB_TOKEN` and GitHub App reviews are by a +`Bot` and never count anyway). Otherwise `approved` would hold itself up. To find that user the +bot reads `GET /user` once per run; an installation token is answered with 403 and needs no login. +Use a dedicated machine user, not a maintainer's PAT, or that maintainer's own reviews stop +counting. A `pull_request_review` event for the bot's review (only a user token fires one) +evaluates nothing in the approve plugin; the merge gate still runs, which is harmless. + +Dismissing the bot's review by hand does not withdraw approval: the next evaluation submits it +again. Use `/approve cancel`. Turning the setting off leaves the reviews already submitted in +place; dismiss them by hand. Two runs evaluating the same pull request at the same moment (a push +and a review, say) may both submit a review; the duplicate is harmless. + +With the setting off nothing changes: no `GET /user`, no review read beyond what +`ignore_review_state` already reads, no review written. + ### Repositories without OWNERS files Zero behaviour change. `/approve` by an org member or collaborator makes the bot submit an diff --git a/docs/configuration.md b/docs/configuration.md index 5292dc7..fc5ec87 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -110,6 +110,7 @@ approve: require_self_approval: false ignore_review_state: false lgtm_acts_as_approve: false + github_review: false # /lgtm records the reviewed commit as a prow/lgtm commit status; this is the default lgtm: @@ -333,6 +334,7 @@ Field | Default | Meaning `require_self_approval` | `false` | `false`: the PR author implicitly approves every changed file their OWNERS entries cover (Prow's default). `true`: the author never counts, implicitly or through `/approve` `ignore_review_state` | `false` | `true`: GitHub reviews neither add (`APPROVED`) nor remove (`CHANGES_REQUESTED`) approvers `lgtm_acts_as_approve` | `false` | `true`: `/lgtm` counts as `/approve` and `/lgtm cancel` as `/approve cancel` when computing approval; the `lgtm` label is unaffected +`github_review` | `false` | `true`: while the PR carries `approved`, the token keeps an `APPROVE` review on the PR's head commit, and dismisses it when `approved` goes away, so that a required approving review is met by the Prow decision ([mirrored review](./commands.md#mirroring-approved-as-a-github-review)). Needs "Allow GitHub Actions to create and approve pull requests" with `GITHUB_TOKEN`; that setting is repo-wide and lets **any** workflow approve PRs with `GITHUB_TOKEN`, so a dedicated GitHub App or machine-user `token` is the safer setup ([repository setting](./automatic-merging.md#required-reviews-and-openssf-scorecard)). `false`: no review is written and no extra API call is made ### `lgtm` diff --git a/docs/events.md b/docs/events.md index 4e11288..a089c0f 100644 --- a/docs/events.md +++ b/docs/events.md @@ -10,7 +10,7 @@ Event | Input | Does --- | --- | --- `issue_comment` | `prow-commands` | Runs the [`/commands`](./commands.md) found in the comment; when one that writes labels ran, re-applies the [`require_matching_label`](./configuration.md#require_matching_label) rules and, on an open PR, runs [`tide`](./automatic-merging.md#event-driven-merging) ([why](#the-bots-writes-fire-no-events)). `issues` | — | `opened`, `reopened`, `labeled`, `unlabeled`: applies the [`require_matching_label`](./configuration.md#require_matching_label) rules. Other activity types are logged and skipped. -`pull_request` | `jobs` | Same `require_matching_label` handling on the PR's labels; [`owners-label`](./labeling.md#labels-from-owners-files) on `opened`, `reopened`, `synchronize`; [`blunderbuss`](./configuration.md#blunderbuss) on `opened` and `ready_for_review`; [`lgtm`](./automatic-merging.md#lgtm-is-bound-to-a-commit) binds a `labeled` `lgtm` by a human to the head; [`approve`](./commands.md#approve) on `opened`, `reopened`, `synchronize` and on `labeled`/`unlabeled` of `approved`; [`ok-to-test`](./commands.md#trigger) approves the pending runs of a labeled PR on `synchronize`, `reopened`; [`tide`](./automatic-merging.md#event-driven-merging) on `labeled`, `unlabeled`, `reopened`, `ready_for_review`, `edited`; then the [PR jobs](./pr-jobs.md); `lgtm` acts on `synchronize` only. `jobs` may be empty. +`pull_request` | `jobs` | Same `require_matching_label` handling on the PR's labels; [`owners-label`](./labeling.md#labels-from-owners-files) on `opened`, `reopened`, `synchronize`; [`blunderbuss`](./configuration.md#blunderbuss) on `opened` and `ready_for_review`; [`lgtm`](./automatic-merging.md#lgtm-is-bound-to-a-commit) binds a `labeled` `lgtm` by a human to the head; [`approve`](./commands.md#approve) on `opened`, `reopened`, `synchronize`, `ready_for_review` and on `labeled`/`unlabeled` of `approved`; [`ok-to-test`](./commands.md#trigger) approves the pending runs of a labeled PR on `synchronize`, `reopened`; [`tide`](./automatic-merging.md#event-driven-merging) on `labeled`, `unlabeled`, `reopened`, `ready_for_review`, `edited`; then the [PR jobs](./pr-jobs.md); `lgtm` acts on `synchronize` only. `jobs` may be empty. `pull_request_target` | `jobs` | Same as `pull_request` with a write token on fork PRs. Read the [safety rule](./pr-jobs.md#pull_request_target) first. The `enqueued` and `dequeued` activity types (merge queue) exist and are not handled yet. `pull_request_review` | — | `submitted`, `dismissed`: [`approve`](./commands.md#approve) re-evaluates the approval (an `APPROVED` review adds an approver, `CHANGES_REQUESTED` removes one) on repositories with OWNERS files, then [`tide`](./automatic-merging.md#event-driven-merging) evaluates the reviewed PR (a review can also satisfy branch protection; it is not `lgtm`). On a fork PR the token is read-only ([below](#fork-pull-requests-under-pull_request)). `check_suite`, `status` | — | `completed` / `success`: [`tide`](./automatic-merging.md#event-driven-merging) evaluates every open PR whose head is the commit. `status` is the legacy commit status API. @@ -167,7 +167,7 @@ Activity type | Handlers `opened` | `require_matching_label`, `owners-label`, `blunderbuss`, `approve` `reopened` | `require_matching_label`, `owners-label`, `approve`, `ok-to-test` (a PR carrying the label: pending runs approved), `tide` `synchronize` | `owners-label`, `approve` (the changed files may differ; approvals stay), `ok-to-test` (a PR carrying the label: pending runs approved), the `lgtm` job (`tide` waits for the next check suite: a push must not merge) -`ready_for_review` | `blunderbuss` (drafts wait for it by default), `tide` +`ready_for_review` | `blunderbuss` (drafts wait for it by default), `approve` (a draft gets its [mirrored review](./commands.md#mirroring-approved-as-a-github-review) now), `tide` `labeled`, `unlabeled` | `require_matching_label`, `lgtm` (`labeled` `lgtm` by a human: bound to the head), `approve` (only for the `approved` label: a human's change is re-evaluated), `tide` `edited` | `tide` (a base branch change alters mergeability) diff --git a/src/plugins/approve.ts b/src/plugins/approve.ts index ac795da..5d3427a 100644 --- a/src/plugins/approve.ts +++ b/src/plugins/approve.ts @@ -13,11 +13,13 @@ import { labelIssue, removeLabels } from '../utils/labeling' import { newOctokit } from '../utils/octokit' import { branchHasOwners, repoHasOwners } from '../utils/owners' import { loadPullRequestOwners } from '../utils/pullRequestOwners' +import { reviewMarker, syncApprovalReview, tokenIdentity, withdrawalReason } from './approveReview' export interface ApproveSettings { require_self_approval: boolean ignore_review_state: boolean lgtm_acts_as_approve: boolean + github_review: boolean } export type ApprovalEventKind = 'approve' | 'cancel' | 'review-approved' | 'review-changes' | 'lgtm' | 'lgtm-cancel' @@ -49,15 +51,17 @@ export interface IssueComment { export interface Review { id: number state: string + body?: string | null user?: { login?: string, type?: string } | null submitted_at?: string | null + commit_id?: string | null } export const approvedLabel = 'approved' export const notifierMarker = '' const commandsDoc = 'https://github.com/cncf/prow-github-actions/blob/main/docs/commands.md' -const pullRequestActions = new Set(['opened', 'reopened', 'synchronize', 'labeled', 'unlabeled']) +const pullRequestActions = new Set(['opened', 'reopened', 'synchronize', 'ready_for_review', 'labeled', 'unlabeled']) const reviewActions = new Set(['submitted', 'dismissed']) /** @@ -72,6 +76,7 @@ export function approveSettings(config: ProwConfig): ApproveSettings { require_self_approval: raw.require_self_approval ?? false, ignore_review_state: raw.ignore_review_state ?? false, lgtm_acts_as_approve: raw.lgtm_acts_as_approve ?? false, + github_review: raw.github_review ?? false, } } @@ -80,11 +85,15 @@ export function approveSettings(config: ProwConfig): ApproveSettings { * comments and the APPROVED / CHANGES_REQUESTED reviews of humans into events, * logins lowercased. Bots, other review states and comments without a command * yield nothing; a comment carrying both a command and its cancel is a cancel. + * The approval review this action mirrors (`approve.github_review`) never + * counts: a review carrying its marker is skipped, and so is every review by + * `tokenLogin`, the token's own user, so that `approved` cannot hold itself up. * * @param comments - the issue comments of the pull request * @param reviews - the reviews of the pull request + * @param tokenLogin - the login behind the workflow token, when it is a user token */ -export function approvalEvents(comments: IssueComment[], reviews: Review[]): ApprovalEvent[] { +export function approvalEvents(comments: IssueComment[], reviews: Review[], tokenLogin?: string): ApprovalEvent[] { const events: ApprovalEvent[] = [] for (const comment of comments) { @@ -106,7 +115,7 @@ export function approvalEvents(comments: IssueComment[], reviews: Review[]): App for (const review of reviews) { const login = humanLogin(review.user) - if (login === undefined || review.submitted_at == null) { + if (login === undefined || review.submitted_at == null || login === tokenLogin || (review.body ?? '').includes(reviewMarker)) { continue } const at = new Date(review.submitted_at) @@ -350,8 +359,10 @@ function ownersEntries(state: ApprovalState, owners: PullRequestOwners): OwnersE * evaluateApproval recomputes the approval of a pull request from its * comments and reviews, then makes the `approved` label and the notifier * comment match: the label is added or removed only when it changes, the - * notifier is posted once and edited in place afterwards. A pull request - * whose base branch has no OWNERS files is left alone. + * notifier is posted once and edited in place afterwards. With + * `approve.github_review` the token's own APPROVE review then follows the + * label, before any merge evaluation of the same run. A pull request whose + * base branch has no OWNERS files is left alone. * * @param octokit - a hydrated github client * @param context - the github context of the current action event @@ -366,8 +377,11 @@ export async function evaluateApproval(octokit: Octokit, context: Context, pullN const settings = approveSettings(await loadProwConfig(octokit, context)) const comments = await listComments(octokit, context, pullNumber) - const reviews = settings.ignore_review_state ? [] : await listReviews(octokit, context, pullNumber) - const state = computeApproval(owners, approvalEvents(comments, reviews), settings) + // the mirrored review needs the reviews and the token's identity; without it neither is read + const identity = settings.github_review ? await tokenIdentity(octokit) : undefined + const reviews = settings.ignore_review_state && identity === undefined ? [] : await listReviews(octokit, context, pullNumber) + const events = approvalEvents(comments, settings.ignore_review_state ? [] : reviews, identity?.login) + const state = computeApproval(owners, events, settings) core.info(state.approved ? `approve: #${pullNumber} is approved by ${[...state.approvers].join(', ')}` @@ -375,6 +389,9 @@ export async function evaluateApproval(octokit: Octokit, context: Context, pullN await syncLabel(octokit, context, owners, state.approved) await upsertNotifier(octokit, context, pullNumber, comments, renderNotifier(state, owners, context.repo)) + if (identity !== undefined) { + await syncApprovalReview(octokit, context, { owners, state, reviews, identity, reason: () => withdrawalReason(owners, state, events, settings) }) + } } async function syncLabel(octokit: Octokit, context: Context, owners: PullRequestOwners, approved: boolean): Promise { @@ -434,8 +451,9 @@ async function listReviews(octokit: Octokit, context: Context, pullNumber: numbe /** * approveOnPullRequest is the `pull_request` handler: on `opened`, - * `reopened` and `synchronize`, and when a human adds or removes the - * `approved` label, it re-evaluates the approval. Approval is sticky across + * `reopened`, `synchronize` and `ready_for_review` (a draft gets no mirrored + * review until then), and when a human adds or removes the `approved` label, + * it re-evaluates the approval. Approval is sticky across * pushes; a push only matters because the changed files may differ. * * @param context - the github context of the current action event @@ -457,6 +475,8 @@ export async function approveOnPullRequest(context: Context = github.context): P /** * approveOnReview is the `pull_request_review` handler: a submitted or * dismissed review may add (APPROVED) or remove (CHANGES_REQUESTED) an approver. + * The approval review this action mirrors is an output, never an input: its + * own events (fired when the token is a user token) evaluate nothing. * * @param context - the github context of the current action event */ @@ -466,6 +486,10 @@ export async function approveOnReview(context: Context = github.context): Promis core.debug(`approve: skipping ${action} review action`) return } + if (String(context.payload.review?.body ?? '').includes(reviewMarker)) { + core.debug(`approve: review ${context.payload.review?.id} is the approval review this action mirrors; nothing to evaluate`) + return + } await evaluateOnOwnersRepo(context, context.payload.pull_request?.number) } diff --git a/src/plugins/approveReview.ts b/src/plugins/approveReview.ts new file mode 100644 index 0000000..3a742be --- /dev/null +++ b/src/plugins/approveReview.ts @@ -0,0 +1,231 @@ +import type { Octokit } from '@octokit/rest' +import type { Context } from '../utils/context' +import type { PullRequestOwners } from '../utils/pullRequestOwners' +import type { ApprovalEvent, ApprovalState, ApproveSettings, Review } from './approve' + +import * as core from '@actions/core' + +import { isBotUser } from '../utils/comments' + +/** identifies the APPROVE review that mirrors the `approved` label (`approve.github_review`) */ +export const reviewMarker = '' + +/** who the workflow token acts as */ +export interface TokenIdentity { + /** the lowercased login behind a user token (PAT); undefined for an installation token (`GITHUB_TOKEN`, a GitHub App), whose reviews are authored by a `Bot` user */ + login?: string +} + +// memoized per client: GET /user is read at most once per handler run +const identities = new WeakMap>() + +/** + * tokenIdentity asks GitHub who the token is (`GET /user`), once per client. + * An installation token, `GITHUB_TOKEN` included, may not read `/user` and + * is answered with 403 (404 on some servers): its reviews are authored by a + * `Bot` user, which the approve plugin never counts anyway, so the login is + * left undefined. Any other failure fails the evaluation. + * + * @param octokit - a hydrated github client + */ +export function tokenIdentity(octokit: Octokit): Promise { + let pending = identities.get(octokit) + if (pending === undefined) { + pending = octokit.users.getAuthenticated().then( + ({ data }) => ({ login: data.login.toLowerCase() }), + (e: unknown) => { + const status = errorStatus(e) + if (status === 403 || status === 404) { + core.debug(`approve: GET /user answered ${status}; the token is an installation token`) + return {} + } + throw new Error(`could not identify the token for approve.github_review: ${e}`) + }, + ) + identities.set(octokit, pending) + } + return pending +} + +/** + * isOwnReview reports whether a review is the mirrored approval this action + * submitted: it carries the marker and was written by the token's identity + * (any `Bot` user when the token is an installation token). + * + * @param review - a review of the pull request + * @param identity - who the token is + */ +export function isOwnReview(review: Review, identity: TokenIdentity): boolean { + if (!(review.body ?? '').includes(reviewMarker)) { + return false + } + const login = review.user?.login?.toLowerCase() + return identity.login === undefined ? isBotUser(review.user) : login === identity.login +} + +/** + * reviewBody is the text of the mirrored approval: who approved, without + * an `@`, since every push submits a fresh review and a mention would notify + * the approvers each time. + * + * @param state - the computed approval + */ +export function reviewBody(state: ApprovalState): string { + return [ + `Approved via /approve by ${[...state.approvers].join(', ')} (OWNERS).`, + '', + 'This review mirrors the `approved` label: it is submitted while the label is set and dismissed when the label goes away. Use `/approve` and `/approve cancel` to change it.', + reviewMarker, + ].join('\n') +} + +const reasonFiles = 5 +const withdrawals: Partial> = { + 'cancel': '/approve cancel', + 'review-changes': 'changes requested', + 'lgtm-cancel': '/lgtm cancel', +} + +/** + * withdrawalReason says why a pull request is not approved, for the + * dismissal message: the files nobody covers and, when someone withdrew + * their approval last, who and how. + * + * @param owners - the pull request and the OWNERS covering its files + * @param state - the computed approval + * @param events - what users did on the pull request, as counted + * @param settings - the resolved `approve` configuration + */ +export function withdrawalReason(owners: PullRequestOwners, state: ApprovalState, events: ApprovalEvent[], settings: ApproveSettings): string { + if (owners.files.length === 0) { + return 'the pull request changes no files' + } + + const shown = state.uncoveredFiles.slice(0, reasonFiles).join(', ') + const more = state.uncoveredFiles.length > reasonFiles ? ` and ${state.uncoveredFiles.length - reasonFiles} more` : '' + const reason = `no approver covers ${shown}${more}` + + const latest = new Map() + for (const event of [...events].sort((a, b) => a.at.getTime() - b.at.getTime())) { + if (event.kind.startsWith('lgtm') && !settings.lgtm_acts_as_approve) { + continue + } + latest.set(event.user.toLowerCase(), event) + } + const withdrawn = [...latest.entries()] + .filter(([, event]) => withdrawals[event.kind] !== undefined) + .map(([user, event]) => `${user} (${withdrawals[event.kind]})`) + .sort() + + return withdrawn.length === 0 ? reason : `${reason}; withdrawn by ${withdrawn.join(', ')}` +} + +export interface MirrorInput { + owners: PullRequestOwners + state: ApprovalState + /** every review of the pull request, bots included */ + reviews: Review[] + identity: TokenIdentity + /** why the pull request is not approved; only read when it is not */ + reason: () => string +} + +export const notPermittedWarning = 'cannot submit the approval review: enable "Allow GitHub Actions to create and approve pull requests" (Settings → Actions → General) or pass a token that can (approve.github_review)' +export const forbiddenWarning = 'cannot submit the approval review: the token was refused; grant the workflow `pull-requests: write` (approve.github_review)' + +/** + * syncApprovalReview makes the action's own APPROVE review follow the + * `approved` label (`approve.github_review`). Approved: one review by the + * token on the current head commit, submitted unless it already exists. + * A draft gets none until it is ready for review. Not approved: every such + * review the action submitted earlier is dismissed, on any commit, drafts + * included. Reviews without the marker, or by anyone else, are never + * touched. GitHub refusing the approval itself (the repository does not let + * Actions approve, the token lacks `pull-requests: write`, or the token + * authored the pull request) is a warning; any other API error fails the + * evaluation. + * + * @param octokit - a hydrated github client + * @param context - the github context of the current action event + * @param input - see MirrorInput + */ +export async function syncApprovalReview(octokit: Octokit, context: Context, input: MirrorInput): Promise { + const { owners, state, reviews, identity } = input + const number = owners.number + if (!owners.open) { + core.debug(`approve: #${number} is not open; its approval review is left alone`) + return + } + + const own = reviews.filter(review => review.state === 'APPROVED' && isOwnReview(review, identity)) + + if (!state.approved) { + if (own.length === 0) { + core.debug(`approve: #${number} carries no approval review to dismiss`) + return + } + const message = `approved removed: ${input.reason()}` + for (const review of own) { + try { + await octokit.pulls.dismissReview({ ...context.repo, pull_number: number, review_id: review.id, message }) + } + catch (e) { + throw new Error(`could not dismiss the approval review ${review.id}: ${e}`) + } + core.info(`approve: dismissed the approval review ${review.id} on #${number}: ${message}`) + } + return + } + + if (own.some(review => review.commit_id === owners.headSha)) { + core.debug(`approve: #${number} already carries the approval review on ${owners.headSha}`) + return + } + if (owners.draft) { + core.debug(`approve: #${number} is a draft; no approval review is submitted until it is ready for review`) + return + } + if (identity.login !== undefined && identity.login === owners.author) { + core.warning(selfApprovalWarning(number, identity.login)) + return + } + + try { + await octokit.pulls.createReview({ + ...context.repo, + pull_number: number, + commit_id: owners.headSha, + event: 'APPROVE', + body: reviewBody(state), + }) + } + catch (e) { + const message = errorMessage(e) + if (/approve your own pull request/i.test(message)) { + core.warning(selfApprovalWarning(number, owners.author)) + return + } + if (/not permitted to approve pull requests/i.test(message)) { + core.warning(`${notPermittedWarning}: ${message}`) + return + } + if (errorStatus(e) === 403) { + core.warning(`${forbiddenWarning}: ${message}`) + return + } + throw new Error(`could not submit the approval review: ${e}`) + } + core.info(`approve: submitted the approval review on #${number} at ${owners.headSha}`) +} + +function selfApprovalWarning(number: number, login: string): string { + return `cannot submit the approval review: #${number} was opened by the token's own identity (${login}), and GitHub does not let an author approve their own pull request (approve.github_review)` +} + +function errorStatus(e: unknown): number | undefined { + return typeof e === 'object' && e !== null && 'status' in e && typeof e.status === 'number' ? e.status : undefined +} + +function errorMessage(e: unknown): string { + return e instanceof Error ? e.message : String(e) +} diff --git a/src/utils/config.ts b/src/utils/config.ts index bd1931a..16bf712 100644 --- a/src/utils/config.ts +++ b/src/utils/config.ts @@ -81,6 +81,8 @@ export interface ApproveConfig { ignore_review_state?: boolean /** `/lgtm` counts as `/approve`; default false */ lgtm_acts_as_approve?: boolean + /** keep an APPROVE review by the token on the head commit while `approved` is set; default false */ + github_review?: boolean } export interface LgtmConfig { @@ -558,7 +560,7 @@ function normalizeBlunderbuss(source: string, raw: unknown): BlunderbussConfig { }) } -const approveFlags = ['require_self_approval', 'ignore_review_state', 'lgtm_acts_as_approve'] as const +const approveFlags = ['require_self_approval', 'ignore_review_state', 'lgtm_acts_as_approve', 'github_review'] as const function normalizeApprove(source: string, raw: unknown): ApproveConfig { if (!isMapping(raw)) { diff --git a/src/utils/pullRequestOwners.ts b/src/utils/pullRequestOwners.ts index 7f9eb50..b4dbfc9 100644 --- a/src/utils/pullRequestOwners.ts +++ b/src/utils/pullRequestOwners.ts @@ -14,6 +14,8 @@ export interface PullRequestOwners { headSha: string author: string draft: boolean + /** the pull request is open (not closed or merged) */ + open: boolean requestedReviewers: string[] assignees: string[] labels: string[] @@ -89,6 +91,7 @@ async function load( headSha: pull.head.sha, author: (pull.user?.login ?? '').toLowerCase(), draft: pull.draft === true, + open: pull.state === 'open', requestedReviewers: (pull.requested_reviewers ?? []).map(user => user.login.toLowerCase()), assignees: (pull.assignees ?? []).map(user => user.login.toLowerCase()), labels: (pull.labels ?? []).map(label => label.name),