fix(review): only the finding goes to the forge

packages/ui/src/components/layout/github-dialog.tsx

@@ -263,7 +263,7 @@ export function GitHubDialog(props: GitHubDialogProps) {
  )}
  {thread.comments.length > 1 && (
  <span className="text-[10px] text-text-muted shrink-0">
- +{thread.comments.length - 1} repl{thread.comments.length === 2 ? 'y' : 'ies'}
+ +{thread.comments.length - 1} repl{thread.comments.length === 2 ? 'y' : 'ies'} · not sent
  </span>
  )}
  </div>

packages/ui/src/lib/review-submission.ts

@@ -2,29 +2,29 @@ import type { CommentKind, CommentThread } from '../components/comments/types';
 import { GENERAL_THREAD_FILE_PATH, isThreadResolved } from '../components/comments/types';
 import type { PrCommentPayload, ReviewEvent } from './api';
 
-/**
- * A thread's replies are part of the same finding, so they are folded into the one comment
- * GitHub will hold — a review comment has no thread of its own until it exists.
- */
 export function isReviewComment(comment: { kind?: CommentKind }): boolean {
  return (comment.kind ?? 'review') === 'review';
 }
 
+/**
+ * Only the finding travels. Everything said after it was said to work out what the finding should
+ * say, so the way to get an answer onto the pull request is to amend the finding, not to ship the
+ * conversation that produced it.
+ */
+export function findingOf(thread: CommentThread): string | undefined {
+ return thread.comments.find(isReviewComment)?.body;
+}
+
 export function threadToPayload(thread: CommentThread): PrCommentPayload {
- // An aside is a conversation with the agent about the review, not part of it. Sending one would
- // put the whole exchange on the pull request.
- const [first, ...replies] = thread.comments.filter(isReviewComment);
- const body = replies.length
- ? [first.body, ...replies.map(reply => `**${reply.author.name}:** ${reply.body}`)].join('\n\n---\n\n')
- : first.body;
+ const finding = findingOf(thread);
 
  return {
  threadId: thread.id,
  filePath: thread.filePath,
  side: thread.side === 'old' ? 'LEFT' : 'RIGHT',
  startLine: thread.startLine !== thread.endLine ? thread.startLine : null,
  endLine: thread.endLine,
- body,
+ body: finding ?? '',
  };
 }
 
@@ -47,7 +47,8 @@ export function isGeneral(thread: CommentThread): boolean {
 export function summaryFromGeneralThreads(threads: CommentThread[]): string {
  return threads
  .filter(isGeneral)
- .flatMap(thread => thread.comments.map(comment => comment.body))
+ .map(findingOf)
+ .filter(body => !!body)
  .join('\n\n');
 }
 

packages/ui/tests/review-submission.test.ts

@@ -49,14 +49,14 @@ describe('threadToPayload', () => {
  expect(threadToPayload(thread({ side: 'old' })).side).toBe('LEFT');
  });
 
- it('folds replies into one body, attributed', () => {
+ it('sends the finding alone, however much was said after it', () => {
  const payload = threadToPayload(
  thread({
  comments: [comment('P2: name is unclear'), comment('agreed, renaming', 'Agent')],
  }),
  );
 
- expect(payload.body).toBe('P2: name is unclear\n\n---\n\n**Agent:** agreed, renaming');
+ expect(payload.body).toBe('P2: name is unclear');
  });
 });
 
@@ -102,6 +102,34 @@ describe('summaryFromGeneralThreads', () => {
  expect(summaryFromGeneralThreads([thread()])).toBe('');
  });
 
+ it('leaves the conversation about a summary out of it', () => {
+ const summary = summaryFromGeneralThreads([
+ thread({
+ filePath: GENERAL_THREAD_FILE_PATH,
+ comments: [
+ { ...comment('Verdict: two findings, both small'), kind: 'review' as const },
+ { ...comment('why only two?', 'You'), kind: 'aside' as const },
+ { ...comment('because the third turned out to be mine', 'Agent'), kind: 'aside' as const },
+ { ...comment('a reply nobody folded in', 'Agent'), kind: 'review' as const },
+ ],
+ }),
+ ]);
+
+ expect(summary).toBe('Verdict: two findings, both small');
+ });
+
+ it('contributes nothing from a thread that is only a question', () => {
+ const summary = summaryFromGeneralThreads([
+ thread({
+ filePath: GENERAL_THREAD_FILE_PATH,
+ comments: [{ ...comment('what did you not check?', 'You'), kind: 'aside' as const }],
+ }),
+ thread({ id: 'g2', filePath: GENERAL_THREAD_FILE_PATH, comments: [comment('Verdict: fine')] }),
+ ]);
+
+ expect(summary).toBe('Verdict: fine');
+ });
+
  it('recognises a general thread', () => {
  expect(isGeneral(thread({ filePath: GENERAL_THREAD_FILE_PATH }))).toBe(true);
  expect(isGeneral(thread())).toBe(false);
@@ -151,14 +179,23 @@ describe('what a thread sends to the forge', () => {
  } as unknown as CommentThread;
  }
 
- it('sends the finding and the discussion of it', () => {
+ it('sends the finding and nothing else', () => {
  const payload = threadToPayload(withComments([
  { body: 'P2: the finding' },
- { body: 'and a reply the author should see', name: 'You' },
+ { body: 'a reply that was never folded into the finding', name: 'You' },
+ ]));
+
+ expect(payload.body).toBe('P2: the finding');
+ });
+
+ it('sends an amended finding once, not the answer that produced it as well', () => {
+ const payload = threadToPayload(withComments([
+ { body: 'P2: the finding, amended to carry the answer' },
+ { body: 'is our money handling affected?', kind: 'aside', name: 'You' },
+ { body: 'No, and I was wrong to point at refunds. I have amended the finding.' },
  ]));
 
- expect(payload.body).toContain('P2: the finding');
- expect(payload.body).toContain('and a reply the author should see');
+ expect(payload.body).toBe('P2: the finding, amended to carry the answer');
  });
 
  // An aside is a conversation with the agent about the review. Posting it would put the whole
@@ -188,7 +225,8 @@ describe('what a thread sends to the forge', () => {
  delete (comment as { kind?: string }).kind;
  }
 
- expect(threadToPayload(thread).body).toContain('old reply');
+ // Were the default an aside, there would be no finding here to send.
+ expect(threadToPayload(thread).body).toBe('P1: old finding');
  });
 });