From da387a98fe381febc24c335c1b8a8a82f39b2fca Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Claude Opus 5)" Date: Tue, 25 Aug 2026 11:12:08 +0200 Subject: [PATCH 1/2] fix(review): only the finding goes to the forge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A thread's replies were concatenated onto the finding under an author label, so an answer arrived on the pull request while the question that prompted it stayed local — the question being an aside, the answer not. Where the agent had also amended the finding, the answer arrived twice, once folded in and once verbatim. Only the finding travels now. Everything said after it was said to work out what the finding should say, so amending it is how an answer reaches the pull request, and the discussion stays where it happened. The dialog still counts local replies, and now says they are not sent. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018PkYQzbsnMihHesafWvXKs --- .../src/components/layout/github-dialog.tsx | 2 +- packages/ui/src/lib/review-submission.ts | 18 +++++-------- packages/ui/tests/review-submission.test.ts | 27 ++++++++++++++----- 3 files changed, 28 insertions(+), 19 deletions(-) diff --git a/packages/ui/src/components/layout/github-dialog.tsx b/packages/ui/src/components/layout/github-dialog.tsx index acddcec..3d0ce2a 100644 --- a/packages/ui/src/components/layout/github-dialog.tsx +++ b/packages/ui/src/components/layout/github-dialog.tsx @@ -263,7 +263,7 @@ export function GitHubDialog(props: GitHubDialogProps) { )} {thread.comments.length > 1 && ( - +{thread.comments.length - 1} repl{thread.comments.length === 2 ? 'y' : 'ies'} + +{thread.comments.length - 1} repl{thread.comments.length === 2 ? 'y' : 'ies'} · not sent )} diff --git a/packages/ui/src/lib/review-submission.ts b/packages/ui/src/lib/review-submission.ts index d8c8108..9887de3 100644 --- a/packages/ui/src/lib/review-submission.ts +++ b/packages/ui/src/lib/review-submission.ts @@ -2,21 +2,17 @@ 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 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] = thread.comments.filter(isReviewComment); return { threadId: thread.id, @@ -24,7 +20,7 @@ export function threadToPayload(thread: CommentThread): PrCommentPayload { side: thread.side === 'old' ? 'LEFT' : 'RIGHT', startLine: thread.startLine !== thread.endLine ? thread.startLine : null, endLine: thread.endLine, - body, + body: finding.body, }; } diff --git a/packages/ui/tests/review-submission.test.ts b/packages/ui/tests/review-submission.test.ts index e3f1e85..ce31f10 100644 --- a/packages/ui/tests/review-submission.test.ts +++ b/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'); }); }); @@ -151,14 +151,26 @@ 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).toContain('P2: the finding'); - expect(payload.body).toContain('and a reply the author should see'); + expect(payload.body).toBe('P2: the finding'); + }); + + // What went out on NCBackend3#14380: the agent answered a question by amending the finding and + // by replying, so the answer arrived twice, and the question that prompted it never arrived at + // all — an aside being local. The amendment is the whole of it. + 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).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 +200,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, the filter would drop it and there would be no finding to send. + expect(threadToPayload(thread).body).toBe('P1: old finding'); }); }); From 0cc22cdb24979f7a9759bdd27b9965f17915501f Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Claude Opus 5)" Date: Tue, 25 Aug 2026 11:26:43 +0200 Subject: [PATCH 2/2] fix(review): the summary was the other half of it The first pass fixed line comments and left the summary doing the same thing, without even the kind filter: every comment on a general thread was mapped into the posted body, so an aside on the summary would have gone to the forge. Both channels take the rule through one `findingOf` now. `find` rather than filter-and-destructure, returning `string | undefined` so the no-finding case is in the type instead of a footnote about which caller guards it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018PkYQzbsnMihHesafWvXKs --- packages/ui/src/lib/review-submission.ts | 11 +++++-- packages/ui/tests/review-submission.test.ts | 33 ++++++++++++++++++--- 2 files changed, 37 insertions(+), 7 deletions(-) diff --git a/packages/ui/src/lib/review-submission.ts b/packages/ui/src/lib/review-submission.ts index 9887de3..42e564d 100644 --- a/packages/ui/src/lib/review-submission.ts +++ b/packages/ui/src/lib/review-submission.ts @@ -11,8 +11,12 @@ export function isReviewComment(comment: { kind?: CommentKind }): boolean { * 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 { - const [finding] = thread.comments.filter(isReviewComment); + const finding = findingOf(thread); return { threadId: thread.id, @@ -20,7 +24,7 @@ export function threadToPayload(thread: CommentThread): PrCommentPayload { side: thread.side === 'old' ? 'LEFT' : 'RIGHT', startLine: thread.startLine !== thread.endLine ? thread.startLine : null, endLine: thread.endLine, - body: finding.body, + body: finding ?? '', }; } @@ -43,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'); } diff --git a/packages/ui/tests/review-submission.test.ts b/packages/ui/tests/review-submission.test.ts index ce31f10..173a77e 100644 --- a/packages/ui/tests/review-submission.test.ts +++ b/packages/ui/tests/review-submission.test.ts @@ -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); @@ -160,9 +188,6 @@ describe('what a thread sends to the forge', () => { expect(payload.body).toBe('P2: the finding'); }); - // What went out on NCBackend3#14380: the agent answered a question by amending the finding and - // by replying, so the answer arrived twice, and the question that prompted it never arrived at - // all — an aside being local. The amendment is the whole of it. 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' }, @@ -200,7 +225,7 @@ describe('what a thread sends to the forge', () => { delete (comment as { kind?: string }).kind; } - // Were the default an aside, the filter would drop it and there would be no finding to send. + // Were the default an aside, there would be no finding here to send. expect(threadToPayload(thread).body).toBe('P1: old finding'); }); });