diff --git a/README.md b/README.md index c22469c..95ab39e 100644 --- a/README.md +++ b/README.md @@ -238,13 +238,21 @@ git-credential-broker pr --host github.com --repo owner/repo \ --head feature --base main --title "a title" --body-file pr.md # say it about one line rather than about the pull request # --number 7 --comment --file Dockerfile --line 4 --side right --body "why this marker exists" + # read its review threads, answer one, or change a comment you wrote + # --number 7 --threads + # --number 7 --reply-to 123456 --body "good point" + # --edit 123456 --body "corrected: it is the other branch" # from your fork into an upstream: a remote name instead of a repository typed by hand git-credential-broker pr --repo code-vaults/repo --head origin:feature --base upstream:main --title "a title" \ --body-file pr.md ``` Both the app and the installation need `pull_requests: write`; without it the error names the -permission instead of failing at GitHub. `--draft` opens it as a draft. +permission instead of failing at GitHub. `--draft` opens it as a draft. `--threads` prints the ids of +a pull request's review threads and their comments; `--reply-to` adds a reply inside one, and +`--edit` replaces the body of one of those review comments. The comment id names the comment — and its +pull request — so `--edit` takes no `--number`. GitHub lets only an author change a comment, and the +comments this command posts are the app's, so `--edit` changes those rather than a person's. ### Opening one as yourself diff --git a/src/broker.ts b/src/broker.ts index 1ef50e4..d2b20dc 100644 --- a/src/broker.ts +++ b/src/broker.ts @@ -281,6 +281,7 @@ export function createRequestHandler( action !== 'comment' && action !== 'threads' && action !== 'reply' && + action !== 'edit' && action !== 'resolve' ) { return deny(context, CODES.BAD_REQUEST, `bad action ${JSON.stringify(action)}`, 'unknown pull request action'); @@ -341,16 +342,21 @@ export function createRequestHandler( if (action === 'update' && title === undefined && body === undefined && base === undefined) { return deny(context, CODES.BAD_REQUEST, 'empty update', 'an update has to change a title, a body or a base'); } - if ((action === 'comment' || action === 'reply') && (typeof body !== 'string' || body.trim() === '')) { + if ((action === 'comment' || action === 'reply' || action === 'edit') && (typeof body !== 'string' || body.trim() === '')) { return deny(context, CODES.BAD_REQUEST, 'empty comment', 'a comment has to say something'); } - if (action === 'reply' && (typeof commentId !== 'number' || !Number.isInteger(commentId) || commentId <= 0)) { - return deny(context, CODES.BAD_REQUEST, 'bad commentId', 'a reply needs the id of the comment it answers'); + if ((action === 'reply' || action === 'edit') && (typeof commentId !== 'number' || !Number.isInteger(commentId) || commentId <= 0)) { + return deny( + context, + CODES.BAD_REQUEST, + 'bad commentId', + action === 'reply' ? 'a reply needs the id of the comment it answers' : 'an edit needs the id of the review comment it changes', + ); } if (action === 'resolve' && (typeof threadId !== 'string' || threadId === '')) { return deny(context, CODES.BAD_REQUEST, 'bad threadId', 'resolving needs the id of the thread, which --threads prints'); } - if (action !== 'open' && action !== 'resolve' && (typeof number !== 'number' || !Number.isInteger(number) || number <= 0)) { + if (action !== 'open' && action !== 'resolve' && action !== 'edit' && (typeof number !== 'number' || !Number.isInteger(number) || number <= 0)) { return deny(context, CODES.BAD_REQUEST, `bad number ${JSON.stringify(number)}`, 'this action needs a pull request number'); } if (action === 'merge' && method !== 'merge' && method !== 'squash' && method !== 'rebase') { @@ -389,6 +395,9 @@ export function createRequestHandler( ...context, action, ...(typeof number === 'number' ? { number } : {}), + // The comment an edit changes is the one thing the entry would otherwise not say, and the + // number alone does not name it. Recorded for a reply too, which also carries an id. + ...(typeof commentId === 'number' ? { comment_id: commentId } : {}), ...(typeof head === 'string' ? { head } : {}), ...(typeof base === 'string' ? { base } : {}), pr_number: result.number, diff --git a/src/commands/pr.ts b/src/commands/pr.ts index cc10901..25ef934 100644 --- a/src/commands/pr.ts +++ b/src/commands/pr.ts @@ -53,6 +53,8 @@ Acting on one --side Which side of the diff --line counts on (default: right) --threads List its review threads: their ids, whether they are resolved, and the comments --reply-to Reply inside the thread that comment id belongs to, with --body/--body-file + --edit Replace the body of that review comment, with --body/--body-file + (an id --threads prints; no --number is needed) --resolve Mark that review thread resolved (the id --threads prints) --close Close it --merge Merge it (branch protection still applies) @@ -74,8 +76,14 @@ Common * @returns `close`, `merge`, `update` or `open`. */ function actionOf(args: { has(name: string): boolean; value(name: string): string | undefined }) { - if (args.value('resolve') !== undefined) return 'resolve' as const; - if (args.value('reply-to') !== undefined) return 'reply' as const; + // `--resolve`, `--reply-to` and `--edit` take a value, and a bare one is parsed as a boolean flag with + // no value at all. Selecting the action from the flag's *presence* is what turns that into the refusal + // below — reading only `value` let a bare `--edit` fall through to `update`, which patched the pull + // request itself with the body meant for the comment. + const present = (name: string): boolean => args.has(name) || args.value(name) !== undefined; + if (present('resolve')) return 'resolve' as const; + if (present('reply-to')) return 'reply' as const; + if (present('edit')) return 'edit' as const; if (args.has('threads')) return 'threads' as const; if (args.has('comment')) return 'comment' as const; if (args.has('status')) return 'status' as const; @@ -153,16 +161,19 @@ export async function runPr(argv: readonly string[]): Promise { if (action === 'open') { if (!head) fail('--head is required to open a pull request'); if (!title) fail('--title is required to open a pull request'); - } else if (action !== 'resolve' && number === undefined) { + } else if (action !== 'resolve' && action !== 'edit' && number === undefined) { const verb = action === 'status' ? 'inspect' : action; fail(`--number is required to ${verb} a pull request`); - } else if (action !== 'resolve' && (!Number.isInteger(Number(number)) || Number(number) <= 0)) { + } + // `--resolve` and `--edit` name their target by id and need no pull request number, but one given + // anyway still has to be a number. + if (number !== undefined && (!Number.isInteger(Number(number)) || Number(number) <= 0)) { fail(`--number must be a positive integer, got ${JSON.stringify(number)}`); } if (action === 'update' && title === undefined && body === undefined && base === undefined) { fail('an update has to change something: pass --title, --body/--body-file or --base'); } - if ((action === 'comment' || action === 'reply') && (body === undefined || body.trim() === '')) { + if ((action === 'comment' || action === 'reply' || action === 'edit') && (body === undefined || body.trim() === '')) { fail('a comment has to say something: pass --body or --body-file'); } @@ -185,6 +196,14 @@ export async function runPr(argv: readonly string[]): Promise { if (action === 'reply' && (replyTo === undefined || !Number.isInteger(Number(replyTo)) || Number(replyTo) <= 0)) { fail(`--reply-to needs the id of the comment being answered, got ${JSON.stringify(replyTo)}`); } + const editId = args.value('edit'); + if (action === 'edit' && (editId === undefined || !Number.isInteger(Number(editId)) || Number(editId) <= 0)) { + fail(`--edit needs the id of the review comment being changed, got ${JSON.stringify(editId)}`); + } + const resolveId = args.value('resolve'); + if (action === 'resolve' && (resolveId === undefined || resolveId.trim() === '')) { + fail(`--resolve needs the id of the thread being resolved, got ${JSON.stringify(resolveId)}`); + } if (viaHost) { if (head === undefined || title === undefined) fail('--via-host needs --head and --title'); @@ -243,8 +262,9 @@ export async function runPr(argv: readonly string[]): Promise { host, path: repo, action, - ...(number === undefined ? {} : { number: Number(number) }), + ...(number === undefined || action === 'edit' ? {} : { number: Number(number) }), ...(action === 'reply' ? { commentId: Number(replyTo) } : {}), + ...(action === 'edit' ? { commentId: Number(editId) } : {}), ...(action === 'resolve' ? { threadId: args.value('resolve') } : {}), ...(head === undefined ? {} : { head }), ...(base === undefined ? {} : { base }), @@ -284,7 +304,9 @@ export async function runPr(argv: readonly string[]): Promise { ? 'commented on' : action === 'reply' ? 'replied in' - : 'updated'; + : action === 'edit' + ? 'edited a review comment on' + : 'updated'; const state = response.prState === undefined ? '' : ` (${response.prState})`; say(`${done} #${response.prNumber ?? '?'}${state}: ${response.prUrl ?? '(no url)'}`); return 0; diff --git a/src/providers/github-app.ts b/src/providers/github-app.ts index 043d5c2..937610c 100644 --- a/src/providers/github-app.ts +++ b/src/providers/github-app.ts @@ -531,6 +531,24 @@ function describeCause(error: unknown): string { return typeof cause === 'string' && cause !== '' ? ` (${cause})` : ''; } +/** + * The pull request number in the URL GitHub returns for a pull request. + * + * A review comment carries the pull request it belongs to as `pull_request_url`, which is how a comment + * id — one for the whole repository, not one per pull request — is tied back to the pull request named + * on the command line. + * + * @param value - an `.../pulls/{number}` URL, as GitHub returns it. + * @returns the number, or undefined when the value names none. + */ +function pullRequestNumber(value: unknown): number | undefined { + if (typeof value !== 'string') return undefined; + const match = /\/pulls\/(\d+)\/?$/.exec(value); + if (match?.[1] === undefined) return undefined; + const number = Number(match[1]); + return Number.isInteger(number) && number > 0 ? number : undefined; +} + return { name: 'github-app', @@ -799,6 +817,26 @@ function describeCause(error: unknown): string { return { number: request.number ?? 0, url: '' }; } + if (request.action === 'edit') { + // A review comment is addressed by its own id, and that id is one for the whole repository, not + // one per pull request. The route therefore carries no pull request number and the CLI asks for + // none: the number to report is the one the comment itself names, read back from the answer + // rather than echoed from the request. The repository in the path is what the allowlist binds. + const endpoint = `${api}/repos/${request.owner}/${request.repo}/pulls/comments/${String(request.commentId ?? 0)}`; + const { text: edited } = await callWithToken('PATCH', endpoint, token, { body: request.body ?? '' }); + let changed: unknown = null; + try { + changed = JSON.parse(edited); + } catch { + changed = null; + } + const html = (changed ?? {}) as { html_url?: unknown; pull_request_url?: unknown }; + return { + number: pullRequestNumber(html.pull_request_url) ?? request.number ?? 0, + url: typeof html.html_url === 'string' ? html.html_url : '', + }; + } + if (request.action === 'threads') { // GraphQL, because review threads have no REST list. The repository and the number are // named here, so the allowlist still decides what can be reached. diff --git a/src/types.ts b/src/types.ts index 068562c..f8b69f4 100644 --- a/src/types.ts +++ b/src/types.ts @@ -72,6 +72,7 @@ export type PullRequestAction = | 'comment' | 'threads' | 'reply' + | 'edit' | 'resolve'; /** How a merge should be recorded. */ @@ -83,9 +84,9 @@ export interface PullRequestRequest { readonly owner: string; readonly repo: string; readonly action: PullRequestAction; - /** The pull request number. Not needed to open one, required for the rest. */ + /** The pull request number. Not needed to open one, to resolve a thread, or to edit a comment. */ readonly number?: number; - /** The review comment being answered, for `action: "reply"`. */ + /** The review comment being answered or changed, for `action: "reply"` or `"edit"`. */ readonly commentId?: number; /** The review thread being resolved, for `action: "resolve"`. */ readonly threadId?: string; diff --git a/test/unit/broker.test.ts b/test/unit/broker.test.ts index 3a037cb..aeec56b 100644 --- a/test/unit/broker.test.ts +++ b/test/unit/broker.test.ts @@ -409,6 +409,49 @@ describe('broker request handling', () => { assert.equal(wrongAction.code, 'bad-request', 'and an anchor only belongs on a comment'); }); + it('carries an edit by comment id alone, and audits which comment it changed', async () => { + const seen: Array> = []; + const provider: Provider = { + name: 'with-edits', + getCredential: () => Promise.reject(new Error('not used here')), + pullRequest: (request) => { + seen.push(request as unknown as Record); + // The provider reads the pull request back from the comment, so the broker need not be told it. + return Promise.resolve({ number: 8, url: 'https://github.com/acme/widget/pull/8#discussion_r99' }); + }, + }; + const { handle, events } = makeHarness(['acme/widget'], provider); + const good = await handle({ + ...REQUEST, + op: 'pull-request', + action: 'edit', + commentId: 99, + body: 'a corrected note', + }); + assert.equal(good.ok, true); + assert.equal(good.prNumber, 8); + assert.deepEqual( + [seen.at(-1)?.['action'], seen.at(-1)?.['commentId'], seen.at(-1)?.['body'], seen.at(-1)?.['number']], + ['edit', 99, 'a corrected note', undefined], + 'the action, the comment id and the new body reach the provider, with no number required', + ); + assert.equal(events.at(-1)?.['action'], 'edit', 'the audit records the action'); + assert.equal(events.at(-1)?.['comment_id'], 99, 'and the comment it changed, which a number would not name'); + assert.equal(events.at(-1)?.['pr_number'], 8, 'with the number the comment itself reports'); + + const empty = await handle({ + ...REQUEST, + op: 'pull-request', + action: 'edit', + commentId: 99, + body: ' ', + }); + assert.equal(empty.code, 'bad-request', 'an edit has to say something'); + + const noId = await handle({ ...REQUEST, op: 'pull-request', action: 'edit', body: 'a note' }); + assert.equal(noId.code, 'bad-request', 'and it has to name the comment it changes'); + }); + describe('prepareSocketPath', () => { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-sock-')); after(() => fs.rmSync(dir, { recursive: true, force: true })); diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index c749406..aaad021 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -714,12 +714,19 @@ describe('the anchor flags, at the CLI boundary', () => { * Every case here is refused by `fail`, which exits, so it cannot be a call into the module — and that is * the point of testing it here: the checks are the CLI's, and the unit tests under them never see them. */ + const home = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-pr-home-')); + after(() => fs.rmSync(home, { recursive: true, force: true })); + const cli = (args: readonly string[]): { status: number; stderr: string } => { try { execFileSync('node', ['src/cli/helper.ts', 'pr', ...args], { cwd: path.resolve('.'), encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'], + // A test-owned HOME and no socket in the environment: the fallback must not depend on the machine + // that runs the suite, and a case that gets past validation has to die at the socket rather than + // reach a broker someone happens to have running. + env: { ...process.env, HOME: home, GIT_BROKER_SOCKET: '', GIT_BROKER_SOCKET_FILE: '' }, }); return { status: 0, stderr: '' }; } catch (error) { @@ -762,4 +769,44 @@ describe('the anchor flags, at the CLI boundary', () => { assert.equal(updated.status, 1); assert.match(updated.stderr, /only goes with --merge/); }); + + it('refuses an edit with nothing to say, or with no comment to change', () => { + const noBody = cli([...base, '--edit', '99']); + assert.equal(noBody.status, 1, 'an edit with no body is refused'); + assert.match(noBody.stderr, /a comment has to say something/); + + for (const id of ['0', 'x', '-1']) { + const bad = cli([...base, '--edit', id, '--body', 'a corrected note']); + assert.equal(bad.status, 1, `--edit ${id} is refused`); + assert.match(bad.stderr, /--edit needs the id of the review comment/); + } + }); + + it('refuses a value flag given with no value, instead of updating the pull request', () => { + // A bare --edit / --reply-to / --resolve is parsed as a boolean flag with no value, so the action + // has to be selected from the flag's presence: reading only its value let the command fall through + // to `update`, which patched the body meant for the comment onto the pull request itself. + for (const [flag, message] of [ + ['--edit', /--edit needs the id of the review comment/], + ['--reply-to', /--reply-to needs the id of the comment/], + ['--resolve', /--resolve needs the id of the thread/], + ] as const) { + const refused = cli([...base, flag, '--body', 'a corrected note']); + assert.equal(refused.status, 1, `a bare ${flag} is refused`); + assert.match(refused.stderr, message); + } + }); + + it('needs no --number to edit, which names the comment instead', () => { + // The route is /pulls/comments/{id}: it carries no pull request number, so requiring one would be a + // flag the command could not honour. This case gets past validation and dies at the socket, which is + // exactly the proof that the number check let it through. + const noNumber = cli([ + '--host', 'github.com', '--repo', 'acme/widget', + '--edit', '99', '--body', 'a corrected note', + ]); + assert.equal(noNumber.status, 1); + assert.match(noNumber.stderr, /no broker socket/, 'it reached socket resolution'); + assert.doesNotMatch(noNumber.stderr, /--number is required/, 'without being asked for a number'); + }); }); diff --git a/test/unit/github-app.test.ts b/test/unit/github-app.test.ts index 3102415..8479936 100644 --- a/test/unit/github-app.test.ts +++ b/test/unit/github-app.test.ts @@ -138,6 +138,8 @@ function createStubFetch(options: { installations: unknown; /** What the GraphQL scope check is told the thread belongs to. */ threadRepo?: string; + /** Which pull request the stubbed review comment belongs to. */ + commentPr?: number; nowMs: number; token?: string; tokenStatus?: number; @@ -155,6 +157,7 @@ function createStubFetch(options: { installationPermissions = appPermissions, logText = 'stub log\n', jobMissing = false, + commentPr = 7, } = options; const calls: RecordedCall[] = []; const fetchImpl: FetchLike = (url, init) => { @@ -204,6 +207,16 @@ function createStubFetch(options: { ); } if (url.endsWith('/reviews')) return Promise.resolve(jsonResponse({})); + if (/\/pulls\/comments\/\d+$/.test(url)) { + return Promise.resolve( + jsonResponse({ + id: 99, + html_url: `https://github.com/acme/widget/pull/${commentPr}#discussion_r99`, + pull_request_url: `https://api.github.com/repos/acme/widget/pulls/${commentPr}`, + body: 'a corrected note', + }), + ); + } if (url.endsWith('/pulls')) { return Promise.resolve( jsonResponse({ number: 42, html_url: 'https://github.com/acme/widget/pull/42' }, 201), @@ -737,6 +750,55 @@ describe('createGithubAppProvider', () => { assert.deepEqual(review?.body, { body: 'a general remark', event: 'COMMENT' }, 'nothing new in the payload'); }); + it('changes a review comment by its id, and reports the comment it changed', async () => { + const stub = createStubFetch({ installations: [{ id: 42, account: { login: 'acme' } }], nowMs }); + const changed = await providerFor(stub).pullRequest!({ + action: 'edit', + host: 'github.com', + owner: 'acme', + repo: 'widget', + number: 7, + commentId: 99, + body: 'a corrected note', + }); + + const patch = stub.calls.find((call) => call.method === 'PATCH' && call.url.includes('/pulls/comments/')); + assert.ok(patch, 'a review comment is changed at the review comments endpoint'); + // The comment id names the thing and is one for the whole repository, so the route carries no pull + // request number and nothing is read first: the number to report comes back in the answer. + assert.equal(patch?.url, 'https://api.github.com/repos/acme/widget/pulls/comments/99'); + assert.deepEqual(patch?.body, { body: 'a corrected note' }); + assert.equal(patch?.authorization, 'Bearer ghs_stub_token_value', 'and it is the app that changes it'); + assert.equal( + stub.calls.some((call) => call.method === 'GET' && call.url.includes('/pulls/comments/')), + false, + 'no round trip to read the comment first', + ); + assert.equal(changed.number, 7); + assert.equal(changed.url, 'https://github.com/acme/widget/pull/7#discussion_r99'); + }); + + it("reports the comment's own pull request, not the number the caller passed", async () => { + // The route cannot honour a pull request number, so the provider must not echo one: the review + // comment object names the pull request it belongs to, and that is what the CLI reports. + const stub = createStubFetch({ + installations: [{ id: 42, account: { login: 'acme' } }], + nowMs, + commentPr: 8, + }); + const changed = await providerFor(stub).pullRequest!({ + action: 'edit', + host: 'github.com', + owner: 'acme', + repo: 'widget', + number: 7, + commentId: 99, + body: 'a corrected note', + }); + assert.equal(changed.number, 8, 'the comment names #8, so #8 is what is reported'); + assert.equal(changed.url, 'https://github.com/acme/widget/pull/8#discussion_r99'); + }); + it("resolves a thread with a token per owner, which is a person acting too", async () => { const dir = mkdtempSync(join(tmpdir(), 'resolve-owner-'));