feat: edit a pull request review comment through the broker

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
 

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,

src/commands/pr.ts

@@ -53,6 +53,8 @@ Acting on one
   --side <left|right>  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 <id>      Reply inside the thread that comment id belongs to, with --body/--body-file
+  --edit <id>          Replace the body of that review comment, with --body/--body-file
+                       (an id --threads prints; no --number is needed)
   --resolve <thread>   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<number> {
   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<number> {
   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<number> {
         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<number> {
             ? '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;

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.

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;

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<Record<string, unknown>> = [];
+    const provider: Provider = {
+      name: 'with-edits',
+      getCredential: () => Promise.reject(new Error('not used here')),
+      pullRequest: (request) => {
+        seen.push(request as unknown as Record<string, unknown>);
+        // 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 }));

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');
+  });
 });