From 99bc08f2ed14fa18c307c1af87f30855d9302f53 Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Claude Opus 5)" Date: Fri, 11 Sep 2026 13:37:25 +0200 Subject: [PATCH 1/2] fix: a new finding on a line that already carries a comment is no longer dropped as a resend MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Whether a finding is already on the pull request was decided by its position: path, line and side, ignoring the wording. A line collects comments over rounds, so an earlier remark there — resolved, or another reviewer's — silently swallowed any new finding on that line. Two findings vanished that way. Identity decides it now. `comment_threads.submitted_at` already records what diffity sent, and the page has been reading it all along to mark a row *already on the pull request*; the submit path was the one still guessing from position. Where there is no local record — a finding imported from a bundle, or posted from another machine — the same wording in the same place from the same account is the evidence left, so the author's login now comes back with the existing comments. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Bwp5QefjsjMFeq6CK6cT6w --- README.md | 2 +- package-lock.json | 12 +- packages/api/package.json | 2 +- packages/cli/package.json | 2 +- packages/cli/src/inbox/runtime.ts | 6 +- packages/cli/src/server.ts | 14 +- packages/cli/src/threads.ts | 18 ++ .../cli/tests/create-review-dedup.test.ts | 220 ++++++++++++++++++ packages/git/package.json | 2 +- packages/github/package.json | 2 +- packages/github/src/comment-targets.ts | 40 +++- packages/github/src/index.ts | 1 + packages/github/src/pr.ts | 18 +- packages/github/tests/comment-targets.test.ts | 42 +++- packages/parser/package.json | 2 +- packages/ui/package.json | 2 +- 16 files changed, 349 insertions(+), 36 deletions(-) create mode 100644 packages/cli/tests/create-review-dedup.test.ts diff --git a/README.md b/README.md index 9315a60e..4695cea3 100644 --- a/README.md +++ b/README.md @@ -236,7 +236,7 @@ The forge dialog is a composer, not a push button: - **Comment**, **Approve** or **Request changes** — and an approval needs nothing attached, since a verdict stands on its own. Approve and Request changes are disabled on your own pull request, which the forge refuses anyway - everything goes as **one review**: one notification for the author, a summary that has somewhere to live, and no half-posted review if something fails -A comment on a line the pull request does not touch is caught before anything is sent, because the whole review is a single request and one unpostable line would reject all of it. Findings already sent are marked *already on the pull request* and left unselected. +A comment on a line the pull request does not touch is caught before anything is sent, because the whole review is a single request and one unpostable line would reject all of it. Findings already sent are marked *already on the pull request* and left unselected. A finding that has been sent once is not sent again, however it has been reworded since, because the forge cannot update the comment already there — but a new finding on a line that already carries someone's comment does go out, since the line says nothing about which finding is on it. Existing inline comments can be pulled into the viewer from the same dialog. diff --git a/package-lock.json b/package-lock.json index 824d4374..b957c353 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8457,7 +8457,7 @@ }, "packages/api": { "name": "@diffity/api", - "version": "0.10.35", + "version": "0.10.36", "dependencies": { "@diffity/parser": "*" }, @@ -8468,7 +8468,7 @@ }, "packages/cli": { "name": "@naturalcycles/diffity", - "version": "0.10.35", + "version": "0.10.36", "license": "MIT", "dependencies": { "commander": "^14.0.3", @@ -8492,7 +8492,7 @@ }, "packages/git": { "name": "@diffity/git", - "version": "0.10.35", + "version": "0.10.36", "devDependencies": { "@types/node": "^25.5.0", "typescript": "^5.9.3", @@ -8501,7 +8501,7 @@ }, "packages/github": { "name": "@diffity/github", - "version": "0.10.35", + "version": "0.10.36", "dependencies": { "@diffity/api": "*", "@diffity/parser": "*" @@ -8514,7 +8514,7 @@ }, "packages/parser": { "name": "@diffity/parser", - "version": "0.10.35", + "version": "0.10.36", "devDependencies": { "typescript": "^5.9.3", "vitest": "^4.1.0" @@ -8522,7 +8522,7 @@ }, "packages/ui": { "name": "@diffity/ui", - "version": "0.10.35", + "version": "0.10.36", "dependencies": { "@diffity/api": "*", "@diffity/parser": "*", diff --git a/packages/api/package.json b/packages/api/package.json index 1aa4b96f..5cf91d2d 100644 --- a/packages/api/package.json +++ b/packages/api/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/api", - "version": "0.10.35", + "version": "0.10.36", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/cli/package.json b/packages/cli/package.json index 696282e0..60743d29 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "@naturalcycles/diffity", - "version": "0.10.35", + "version": "0.10.36", "description": "Agent-agnostic, GitHub-style diff viewer and code review tool with a live agent loop", "type": "module", "bin": { diff --git a/packages/cli/src/inbox/runtime.ts b/packages/cli/src/inbox/runtime.ts index 8763f242..00a0776c 100644 --- a/packages/cli/src/inbox/runtime.ts +++ b/packages/cli/src/inbox/runtime.ts @@ -1,7 +1,7 @@ import { spawn, execFile } from 'node:child_process'; import { promisify } from 'node:util'; import type { LiveRequest } from '@diffity/api'; -import { createReview, fetchPrContext, type PrContext, type PrSnapshot } from '@diffity/github'; +import { createReview, fetchPrContext, getViewerLogin, type PrContext, type PrSnapshot } from '@diffity/github'; import { createWriteStream, mkdirSync, readFileSync, rmSync, writeFileSync, type WriteStream } from 'node:fs'; import { dirname, join } from 'node:path'; import { logsDir, type ExportOpts, type MarkPostedOpts, type PrepareDeps, type RunAgentOpts, type ServerHandle } from './prepare.js'; @@ -59,7 +59,9 @@ export function realPrepareDeps(nodePath: string, entry: string, dataDirFor: (wo prContext: (snapshot, worktree) => writePrContext(snapshot, dataDirFor(worktree), log), // In this process, with the reviewer's own credentials: posting the alert findings is the // daemon's own act, after the agent has finished, and never something the agent can reach. - postReview: opts => createReview(opts.owner, opts.repo, opts.prNumber, opts.headSha, opts.submission), + postReview: async opts => createReview(opts.owner, opts.repo, opts.prNumber, opts.headSha, opts.submission, { + viewerLogin: await getViewerLogin(), + }), markPosted: opts => markPosted(nodePath, entry, opts, dataDirFor(opts.worktree)), exportBundle: opts => exportBundle(nodePath, entry, opts, dataDirFor(opts.worktree)), log, diff --git a/packages/cli/src/server.ts b/packages/cli/src/server.ts index a387d906..4ee44feb 100644 --- a/packages/cli/src/server.ts +++ b/packages/cli/src/server.ts @@ -67,6 +67,7 @@ import { prCommits as githubPrCommits, prBaseRef as githubPrBaseRef, getCompareDiff as githubCompareDiff, + getViewerLogin, type CreateReviewOptions, type GitHubRemote, } from '@diffity/github'; @@ -83,7 +84,7 @@ import { computeDiffFingerprint } from './fingerprint.js'; import { parseDiffStatFiles } from './diff-stat.js'; import { parseDiffStatSummary } from './diff-stat.js'; import { anyReviewInProgress, getReviewRun } from './review-run.js'; -import { createThread, addReply, getThreadsForSession, markThreadsSubmitted, setThreadForgeComment, updateThreadStatus } from './threads.js'; +import { createThread, addReply, getThreadsForSession, markThreadsSubmitted, setThreadForgeComment, submittedThreadIds, updateThreadStatus } from './threads.js'; import { existingThreadFor } from './github-pull.js'; import { threadsResolvedRemotely } from './github-resolution.js'; import { noteViewerSeen, markViewerGone, viewerSnapshot, viewerIsPresent, viewerHasGone, awakeMs, VIEWER_POLL_MS } from './viewers.js'; @@ -831,7 +832,16 @@ export function startServer(options: ServerOptions): Promise { details.prNumber, details.headSha, submission, - target, + { + ...target, + // Which findings are already on the pull request is a question about these + // findings, not about the lines they sit on — a line collects comments over + // rounds, and the ones on it may be nothing to do with what is being sent now. + postedThreadIds: submittedThreadIds( + submission.comments.map(comment => comment.threadId).filter((id): id is string => !!id), + ), + viewerLogin: await getViewerLogin(), + }, ); // Only the reader's own submit counts as the pull request being handled; an agent // posting through this route is not the reviewer having reviewed it. The mark is diff --git a/packages/cli/src/threads.ts b/packages/cli/src/threads.ts index bca68b9b..847431ab 100644 --- a/packages/cli/src/threads.ts +++ b/packages/cli/src/threads.ts @@ -173,6 +173,24 @@ export function markThreadsSubmitted( } } +/** + * Of these findings, the ones already sent to the forge. What is on a line cannot say which + * finding put it there, so this record is what tells a resend from a new remark in the same place. + */ +export function submittedThreadIds(ids: string[]): Set { + if (ids.length === 0) { + return new Set(); + } + + const rows = queryAll<{ id: string }>( + `SELECT id FROM comment_threads + WHERE submitted_at IS NOT NULL AND id IN (${ids.map(() => '?').join(', ')})`, + ...ids, + ); + + return new Set(rows.map(row => row.id)); +} + /** Records which forge comment a thread exists as, once that is learned. */ export function setThreadForgeComment(threadId: string, githubCommentId: number): void { getDb() diff --git a/packages/cli/tests/create-review-dedup.test.ts b/packages/cli/tests/create-review-dedup.test.ts new file mode 100644 index 00000000..95c87b47 --- /dev/null +++ b/packages/cli/tests/create-review-dedup.test.ts @@ -0,0 +1,220 @@ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, mkdirSync, readFileSync, writeFileSync, rmSync, chmodSync } from 'node:fs'; +import { join, delimiter } from 'node:path'; +import { tmpdir } from 'node:os'; + +let root: string; +let repoDir: string; +let ghDir: string; +let origCwd: string; +let origPath: string | undefined; +let port: number; +let close: () => void; + +const PATCH = `diff --git a/a.ts b/a.ts +index 1111111..2222222 100644 +--- a/a.ts ++++ b/a.ts +@@ -1 +1,2 @@ +-const a = 1; ++const a = 2; ++const b = 3; +`; + +/** + * A gh that answers with the comments already on the pull request and keeps the review it is + * handed, so a test can see what actually went out rather than only what the route reported. + */ +function writeFakeGh(dir: string): void { + writeFileSync(join(dir, 'gh'), `#!/bin/sh +case "$1 $2" in + "--version ") echo "gh version 2.0.0"; exit 0 ;; + "auth status") exit 0 ;; + "pr view") + echo "{\\"number\\":1,\\"title\\":\\"A change\\",\\"url\\":\\"https://github.com/o/r/pull/1\\",\\"headRefOid\\":\\"$FAKE_GH_HEAD\\",\\"createdAt\\":\\"2026-01-01T00:00:00Z\\",\\"author\\":{\\"login\\":\\"alice\\"},\\"body\\":\\"\\"}" + exit 0 ;; + "pr diff") cat "$FAKE_GH_DIR/patch.diff"; exit 0 ;; + "api user") echo "me"; exit 0 ;; +esac +if [ "$2" = "repos/o/r/pulls/1/reviews" ] && [ "$3" = "--method" ]; then + cat > "$FAKE_GH_DIR/posted.json" + echo '{"html_url":"https://github.com/o/r/pull/1#pullrequestreview-9","id":9}' + exit 0 +fi +if [ "$2" = "repos/o/r/pulls/1/comments" ] && [ "$3" = "--jq" ]; then + echo 1 + exit 0 +fi +if [ "$2" = "repos/o/r/pulls/1/comments" ]; then + cat "$FAKE_GH_DIR/existing.json" + exit 0 +fi +echo "[]" +exit 0 +`); + chmodSync(join(dir, 'gh'), 0o755); +} + +/** What the pull request already carries on a.ts:1, as the REST API reports it. */ +function alreadyThere(body: string, login: string): void { + writeFileSync( + join(ghDir, 'existing.json'), + JSON.stringify([{ id: 1, path: 'a.ts', line: 1, side: 'RIGHT', body, user: { login, type: 'User' }, in_reply_to_id: null, start_line: null, created_at: '2026-01-01T00:00:00Z' }]), + ); +} + +/** A finding recorded locally, as the page records one, so it has an identity to be known by. */ +async function recordFinding(body: string): Promise { + const session = await (await fetch(`http://127.0.0.1:${port}/api/sessions/ensure`, { + method: 'POST', + headers: { 'Sec-Fetch-Site': 'same-origin' }, + })).json() as { id: string }; + const res = await fetch(`http://127.0.0.1:${port}/api/threads`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', 'Sec-Fetch-Site': 'same-origin' }, + body: JSON.stringify({ + sessionId: session.id, + filePath: 'a.ts', + side: 'new', + startLine: 1, + endLine: 1, + body, + author: { name: 'me', type: 'user' }, + }), + }); + const created = await res.json() as { id?: string }; + if (!created.id) { + throw new Error(`the thread was not recorded: ${JSON.stringify(created)}`); + } + return created.id; +} + +interface Posted { + status: number; + body: { submitted: number; skipped: number; errors: string[] }; + sent: { body: string }[]; +} + +async function post(comment: { body: string; threadId: string }): Promise { + rmSync(join(ghDir, 'posted.json'), { force: true }); + const res = await fetch(`http://127.0.0.1:${port}/api/github/create-review`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', 'Sec-Fetch-Site': 'same-origin' }, + body: JSON.stringify({ + event: 'COMMENT', + body: 'One finding.', + comments: [{ ...comment, filePath: 'a.ts', side: 'RIGHT', startLine: null, endLine: 1 }], + }), + }); + const reported = await res.json(); + let sent: { body: string }[] = []; + try { + sent = (JSON.parse(readFileSync(join(ghDir, 'posted.json'), 'utf-8')) as { comments: { body: string }[] }).comments; + } catch { + // no review reached the forge + } + return { status: res.status, body: reported, sent }; +} + +beforeAll(async () => { + origCwd = process.cwd(); + root = mkdtempSync(join(tmpdir(), 'diffity-dedup-route-')); + repoDir = join(root, 'repo'); + ghDir = join(root, 'gh-state'); + mkdirSync(repoDir); + mkdirSync(ghDir); + writeFileSync(join(ghDir, 'patch.diff'), PATCH); + + execFileSync('git', ['init', '-b', 'main', repoDir], { stdio: 'pipe' }); + const git = (args: string[]) => execFileSync('git', args, { cwd: repoDir, stdio: 'pipe' }); + git(['config', 'user.email', 't@t']); + git(['config', 'user.name', 'T']); + writeFileSync(join(repoDir, 'a.ts'), 'const a = 2;\nconst b = 3;\n'); + git(['add', '.']); + git(['commit', '-m', 'init']); + git(['remote', 'add', 'origin', 'https://github.com/o/r.git']); + process.env.FAKE_GH_HEAD = execFileSync('git', ['rev-parse', 'HEAD'], { cwd: repoDir, encoding: 'utf-8' }).trim(); + process.env.FAKE_GH_DIR = ghDir; + + const fakeBin = join(root, 'bin'); + mkdirSync(fakeBin); + writeFakeGh(fakeBin); + origPath = process.env.PATH; + process.env.PATH = `${fakeBin}${delimiter}${origPath ?? ''}`; + + process.env.DIFFITY_DATA_DIR = join(root, 'notes'); + process.chdir(repoDir); + + const { startServer } = await import('../src/server.js'); + const started = await startServer({ port: 0, diffArgs: [], effectiveRef: 'work' }); + port = started.port; + close = started.close; +}); + +afterAll(() => { + close?.(); + process.chdir(origCwd); + delete process.env.DIFFITY_DATA_DIR; + delete process.env.FAKE_GH_HEAD; + delete process.env.FAKE_GH_DIR; + if (origPath === undefined) { + delete process.env.PATH; + } else { + process.env.PATH = origPath; + } + try { + rmSync(root, { recursive: true, force: true, maxRetries: 20, retryDelay: 250 }); + } catch { + // still being written to + } +}); + +describe('a finding on a line that already carries a comment', () => { + it('reaches GitHub when it is not the comment already there', async () => { + alreadyThere('P2: an older remark, resolved since', 'me'); + + const posted = await post({ body: 'P1: a different problem entirely', threadId: 'fresh-finding' }); + + expect(posted.status).toBe(200); + expect(posted.body.skipped).toBe(0); + expect(posted.body.submitted).toBe(1); + expect(posted.sent.map(one => one.body)).toEqual(['P1: a different problem entirely']); + }, 15000); + + it('is held back when it is word for word the one already there', async () => { + alreadyThere('P2: the very same finding', 'me'); + + const posted = await post({ body: 'P2: the very same finding', threadId: 'same-again' }); + + expect(posted.status).toBe(200); + expect(posted.body.skipped).toBe(1); + expect(posted.body.submitted).toBe(0); + expect(posted.sent).toEqual([]); + }, 15000); + + it('goes out when the identical wording is another reviewer\'s, not ours', async () => { + alreadyThere('P2: the very same finding', 'someone-else'); + + const posted = await post({ body: 'P2: the very same finding', threadId: 'mine-too' }); + + expect(posted.status).toBe(200); + expect(posted.body.submitted).toBe(1); + expect(posted.sent.map(one => one.body)).toEqual(['P2: the very same finding']); + }, 15000); + + it('is held back once the record says it went out, however it reads now', async () => { + // The forge cannot edit the comment already there, so a reworded resend would land beside it. + writeFileSync(join(ghDir, 'existing.json'), '[]'); + const threadId = await recordFinding('P2: as first written'); + + const first = await post({ body: 'P2: as first written', threadId }); + expect(first.body.submitted).toBe(1); + + alreadyThere('P2: as first written', 'me'); + const again = await post({ body: 'P2: reworded since, same finding', threadId }); + + expect(again.body.skipped).toBe(1); + expect(again.body.submitted).toBe(0); + }, 15000); +}); diff --git a/packages/git/package.json b/packages/git/package.json index f772fe95..9ae8cea5 100644 --- a/packages/git/package.json +++ b/packages/git/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/git", - "version": "0.10.35", + "version": "0.10.36", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/github/package.json b/packages/github/package.json index 7418aa38..1645889b 100644 --- a/packages/github/package.json +++ b/packages/github/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/github", - "version": "0.10.35", + "version": "0.10.36", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/github/src/comment-targets.ts b/packages/github/src/comment-targets.ts index bdb4bcb8..c8970a45 100644 --- a/packages/github/src/comment-targets.ts +++ b/packages/github/src/comment-targets.ts @@ -11,6 +11,8 @@ export interface ExistingComment { line: number; side: string; body: string; + /** Who wrote it, so another reviewer's remark is not mistaken for one of ours. */ + login: string; } /** @@ -46,15 +48,45 @@ export function commentableLines(patch: string): Map { return byFile; } +/** What makes a finding one already sent, beyond the line it would sit on. */ +export interface PostedBefore { + /** The findings diffity's own record says have gone to this pull request. */ + threadIds?: ReadonlySet; + /** The account the review is posted as, when it is known. */ + viewerLogin?: string | null; +} + /** - * Matched on position rather than wording: editing a finding locally and submitting again is not - * a new remark about that line, and the forge has no way to update the one already there. + * Whether this finding is already on the pull request, as opposed to a new remark about a line + * that happens to carry one. A line collects comments over rounds — resolved ones, other + * reviewers' — so its position says nothing about which finding is there, and dropping on + * position alone silently swallows new findings. + * + * Identity settles it where there is a record: a finding diffity sent is not sent twice, however + * it has been reworded since, because the forge cannot update the comment already there. Where + * there is no record — a finding imported from a bundle, or posted from another machine — the + * same wording in the same place from the same account is the best evidence left. */ -export function isAlreadyCommented(existing: ExistingComment[], comment: PrComment): boolean { +export function isAlreadyCommented( + existing: ExistingComment[], + comment: PrComment, + posted: PostedBefore = {}, +): boolean { + if (comment.threadId && posted.threadIds?.has(comment.threadId)) { + return true; + } + return existing.some( one => one.path === comment.filePath && one.line === comment.endLine && - one.side === comment.side, + one.side === comment.side && + sameWording(one.body, comment.body) && + (!posted.viewerLogin || one.login === posted.viewerLogin), ); } + +/** Line endings and trailing space differ between what was sent and what comes back. */ +function sameWording(one: string, other: string): boolean { + return one.replace(/\r\n/g, '\n').trim() === other.replace(/\r\n/g, '\n').trim(); +} diff --git a/packages/github/src/index.ts b/packages/github/src/index.ts index e7f658cd..ab9f17ac 100644 --- a/packages/github/src/index.ts +++ b/packages/github/src/index.ts @@ -4,6 +4,7 @@ export { getComments, getCommentCount, pullComments, pullThreadState, createRevi export type { RemoteThreadState, CreateReviewOptions } from './pr.js'; export { getReviews, parseReviews } from './reviews.js'; export { commentableLines, isAlreadyCommented } from './comment-targets.js'; +export type { ExistingComment, PostedBefore } from './comment-targets.js'; export { matchCreatedComments } from './comment-ids.js'; export type { CreatedComment, SentComment } from './comment-ids.js'; export { isGitHubPrUrl, parseGitHubPrUrl, checkoutPr, getPrBase, parsePrBase } from './pr-url.js'; diff --git a/packages/github/src/pr.ts b/packages/github/src/pr.ts index 2edbef55..537d8fb8 100644 --- a/packages/github/src/pr.ts +++ b/packages/github/src/pr.ts @@ -1,16 +1,9 @@ import { ghAsync } from './exec.js'; import { matchCreatedComments, type CreatedComment, type SentComment } from './comment-ids.js'; import type { PrComment, PulledThread, ReviewResult, ReviewSubmission } from './types.js'; -import { commentableLines, isAlreadyCommented } from './comment-targets.js'; +import { commentableLines, isAlreadyCommented, type ExistingComment } from './comment-targets.js'; -interface ExistingComment { - path: string; - line: number; - side: string; - body: string; -} - export async function getComments(owner: string, repo: string, prNumber: number): Promise { try { const json = await ghAsync([ @@ -25,11 +18,12 @@ export async function getComments(owner: string, repo: string, prNumber: number) if (!Array.isArray(data)) { return []; } - return data.map((c: { path: string; line: number; side: string; body: string }) => ({ + return data.map((c: { path: string; line: number; side: string; body: string; user?: { login?: string } }) => ({ path: c.path, line: c.line, side: c.side, body: c.body, + login: c.user?.login ?? '', })); } catch { return []; @@ -218,6 +212,10 @@ export interface CreateReviewOptions { commitSha?: string; /** The diff the commentable lines come from; the pull request's own patch when none is given. */ patch?: string; + /** The findings diffity's record says already went to this pull request, by thread id. */ + postedThreadIds?: ReadonlySet; + /** The account the review posts as, so a resend is told apart from another reviewer's remark. */ + viewerLogin?: string | null; } /** @@ -262,7 +260,7 @@ export async function createReview( ); continue; } - if (isAlreadyCommented(existing, comment)) { + if (isAlreadyCommented(existing, comment, { threadIds: options.postedThreadIds, viewerLogin: options.viewerLogin })) { skipped++; continue; } diff --git a/packages/github/tests/comment-targets.test.ts b/packages/github/tests/comment-targets.test.ts index b24393b2..c26c11cd 100644 --- a/packages/github/tests/comment-targets.test.ts +++ b/packages/github/tests/comment-targets.test.ts @@ -26,6 +26,10 @@ function comment(filePath: string, endLine: number, body = 'P2: x'): PrComment { return { filePath, side: 'RIGHT', startLine: null, endLine, body }; } +function onThePr(body: string, login = 'fiddur', line = 11, path = 'src/a.ts') { + return { path, line, side: 'RIGHT', body, login }; +} + describe('commentableLines', () => { it('collects the lines each file actually shows', () => { const lines = commentableLines(patch); @@ -51,11 +55,39 @@ describe('commentableLines', () => { }); describe('isAlreadyCommented', () => { - const existing = [{ path: 'src/a.ts', line: 11, side: 'RIGHT', body: 'the original wording' }]; + const existing = [onThePr('P2: the original wording')]; + + it('drops a finding the record says was already sent, however it reads now', () => { + // The forge cannot update the comment already there, so a locally edited finding that + // has been posted once is not posted a second time beside it. + const sent: PrComment = { ...comment('src/a.ts', 11, 'reworded since'), threadId: 'abc' }; + + expect(isAlreadyCommented(existing, sent, { threadIds: new Set(['abc']) })).toBe(true); + }); + + it('sends a new finding on a line that already carries an older one', () => { + // The reported bug: earlier rounds leave comments on a line, resolved or not, and a fresh + // finding there was silently swallowed as a resend. + const fresh: PrComment = { ...comment('src/a.ts', 11, 'P1: this is a different problem'), threadId: 'new' }; + + expect(isAlreadyCommented(existing, fresh, { threadIds: new Set(['abc']), viewerLogin: 'fiddur' })).toBe(false); + }); + + it('takes the same wording in the same place as a resend when no record says otherwise', () => { + // A finding imported from a bundle, or posted from another machine, has no local mark. + expect(isAlreadyCommented(existing, comment('src/a.ts', 11, 'P2: the original wording'))).toBe(true); + }); + + it('ignores line endings and trailing space in that comparison', () => { + const withCrLf = [onThePr('P2: one\r\ntwo\r\n')]; + + expect(isAlreadyCommented(withCrLf, comment('src/a.ts', 11, 'P2: one\ntwo'))).toBe(true); + }); + + it('does not treat another reviewer\'s identical remark as ours', () => { + const theirs = [onThePr('P2: the original wording', 'someone-else')]; - it('matches on position, not on wording', () => { - // Editing a finding locally and resubmitting used to post a second comment beside the first. - expect(isAlreadyCommented(existing, comment('src/a.ts', 11, 'reworded since'))).toBe(true); + expect(isAlreadyCommented(theirs, comment('src/a.ts', 11, 'P2: the original wording'), { viewerLogin: 'fiddur' })).toBe(false); }); it('leaves a different line alone', () => { @@ -67,7 +99,7 @@ describe('isAlreadyCommented', () => { }); it('distinguishes the two sides', () => { - const onLeft: PrComment = { ...comment('src/a.ts', 11), side: 'LEFT' }; + const onLeft: PrComment = { ...comment('src/a.ts', 11, 'P2: the original wording'), side: 'LEFT' }; expect(isAlreadyCommented(existing, onLeft)).toBe(false); }); diff --git a/packages/parser/package.json b/packages/parser/package.json index a744cec4..261d7f40 100644 --- a/packages/parser/package.json +++ b/packages/parser/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/parser", - "version": "0.10.35", + "version": "0.10.36", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/ui/package.json b/packages/ui/package.json index 0e41a380..94234805 100644 --- a/packages/ui/package.json +++ b/packages/ui/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/ui", - "version": "0.10.35", + "version": "0.10.36", "type": "module", "private": true, "scripts": { From 39b8849cefefce3b10358e4cc44ed147748f6a63 Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Claude Opus 5)" Date: Fri, 11 Sep 2026 14:04:14 +0200 Subject: [PATCH 2/2] fix: a comment pulled from GitHub counts as being on the pull request already MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review found the record too narrow. Pulling comments records the forge comment a thread exists as, in `github_comment_id`, and never sets `submitted_at` — so a pulled comment, which the dialog offers like any other, would have gone back out as a second copy once edited locally or shifted by a new head. The old position rule hid that. `threadsOnTheForge` answers what the name says now: sent from here, or pulled from there. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Bwp5QefjsjMFeq6CK6cT6w --- packages/cli/src/server.ts | 4 +-- packages/cli/src/threads.ts | 10 +++--- .../cli/tests/create-review-dedup.test.ts | 35 +++++++++++++++++-- packages/github/src/comment-targets.ts | 11 +++--- packages/github/src/pr.ts | 2 +- 5 files changed, 47 insertions(+), 15 deletions(-) diff --git a/packages/cli/src/server.ts b/packages/cli/src/server.ts index 4ee44feb..ae6616eb 100644 --- a/packages/cli/src/server.ts +++ b/packages/cli/src/server.ts @@ -84,7 +84,7 @@ import { computeDiffFingerprint } from './fingerprint.js'; import { parseDiffStatFiles } from './diff-stat.js'; import { parseDiffStatSummary } from './diff-stat.js'; import { anyReviewInProgress, getReviewRun } from './review-run.js'; -import { createThread, addReply, getThreadsForSession, markThreadsSubmitted, setThreadForgeComment, submittedThreadIds, updateThreadStatus } from './threads.js'; +import { createThread, addReply, getThreadsForSession, markThreadsSubmitted, setThreadForgeComment, threadsOnTheForge, updateThreadStatus } from './threads.js'; import { existingThreadFor } from './github-pull.js'; import { threadsResolvedRemotely } from './github-resolution.js'; import { noteViewerSeen, markViewerGone, viewerSnapshot, viewerIsPresent, viewerHasGone, awakeMs, VIEWER_POLL_MS } from './viewers.js'; @@ -837,7 +837,7 @@ export function startServer(options: ServerOptions): Promise { // Which findings are already on the pull request is a question about these // findings, not about the lines they sit on — a line collects comments over // rounds, and the ones on it may be nothing to do with what is being sent now. - postedThreadIds: submittedThreadIds( + postedThreadIds: threadsOnTheForge( submission.comments.map(comment => comment.threadId).filter((id): id is string => !!id), ), viewerLogin: await getViewerLogin(), diff --git a/packages/cli/src/threads.ts b/packages/cli/src/threads.ts index 847431ab..89d513cd 100644 --- a/packages/cli/src/threads.ts +++ b/packages/cli/src/threads.ts @@ -174,17 +174,19 @@ export function markThreadsSubmitted( } /** - * Of these findings, the ones already sent to the forge. What is on a line cannot say which - * finding put it there, so this record is what tells a resend from a new remark in the same place. + * Of these findings, the ones that are on the forge already — sent from here, or pulled from + * there and so known by the comment they exist as. What sits on a line cannot say which finding + * put it there, so this record is what tells a resend from a new remark in the same place. */ -export function submittedThreadIds(ids: string[]): Set { +export function threadsOnTheForge(ids: string[]): Set { if (ids.length === 0) { return new Set(); } const rows = queryAll<{ id: string }>( `SELECT id FROM comment_threads - WHERE submitted_at IS NOT NULL AND id IN (${ids.map(() => '?').join(', ')})`, + WHERE (submitted_at IS NOT NULL OR github_comment_id IS NOT NULL) + AND id IN (${ids.map(() => '?').join(', ')})`, ...ids, ); diff --git a/packages/cli/tests/create-review-dedup.test.ts b/packages/cli/tests/create-review-dedup.test.ts index 95c87b47..80241def 100644 --- a/packages/cli/tests/create-review-dedup.test.ts +++ b/packages/cli/tests/create-review-dedup.test.ts @@ -64,17 +64,22 @@ function alreadyThere(body: string, login: string): void { ); } -/** A finding recorded locally, as the page records one, so it has an identity to be known by. */ -async function recordFinding(body: string): Promise { +async function ensureSession(): Promise { const session = await (await fetch(`http://127.0.0.1:${port}/api/sessions/ensure`, { method: 'POST', headers: { 'Sec-Fetch-Site': 'same-origin' }, })).json() as { id: string }; + return session.id; +} + +/** A finding recorded locally, as the page records one, so it has an identity to be known by. */ +async function recordFinding(body: string): Promise { + const sessionId = await ensureSession(); const res = await fetch(`http://127.0.0.1:${port}/api/threads`, { method: 'POST', headers: { 'Content-Type': 'application/json', 'Sec-Fetch-Site': 'same-origin' }, body: JSON.stringify({ - sessionId: session.id, + sessionId, filePath: 'a.ts', side: 'new', startLine: 1, @@ -217,4 +222,28 @@ describe('a finding on a line that already carries a comment', () => { expect(again.body.skipped).toBe(1); expect(again.body.submitted).toBe(0); }, 15000); + + it('is held back when it was pulled from GitHub rather than sent from here', async () => { + // A pulled comment knows the forge comment it exists as but was never submitted from here, + // and editing it locally moves it out of reach of the wording comparison. + alreadyThere('P2: someone else wrote this on the pull request', 'someone-else'); + const sessionId = await ensureSession(); + await fetch(`http://127.0.0.1:${port}/api/github/pull-comments`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', 'Sec-Fetch-Site': 'same-origin' }, + body: JSON.stringify({ sessionId }), + }); + const threads = await (await fetch(`http://127.0.0.1:${port}/api/threads?session=${sessionId}`)).json() as { + id: string; + githubCommentId: number | null; + comments: { body: string }[]; + }[]; + const pulled = threads.find(thread => thread.comments[0]?.body.includes('someone else wrote this')); + expect(pulled?.githubCommentId).toBe(1); + + const again = await post({ body: 'P2: reworded by me after pulling it', threadId: pulled!.id }); + + expect(again.body.skipped).toBe(1); + expect(again.body.submitted).toBe(0); + }, 15000); }); diff --git a/packages/github/src/comment-targets.ts b/packages/github/src/comment-targets.ts index c8970a45..5f590272 100644 --- a/packages/github/src/comment-targets.ts +++ b/packages/github/src/comment-targets.ts @@ -50,7 +50,7 @@ export function commentableLines(patch: string): Map { /** What makes a finding one already sent, beyond the line it would sit on. */ export interface PostedBefore { - /** The findings diffity's own record says have gone to this pull request. */ + /** The findings diffity's own record places on this pull request already. */ threadIds?: ReadonlySet; /** The account the review is posted as, when it is known. */ viewerLogin?: string | null; @@ -62,10 +62,11 @@ export interface PostedBefore { * reviewers' — so its position says nothing about which finding is there, and dropping on * position alone silently swallows new findings. * - * Identity settles it where there is a record: a finding diffity sent is not sent twice, however - * it has been reworded since, because the forge cannot update the comment already there. Where - * there is no record — a finding imported from a bundle, or posted from another machine — the - * same wording in the same place from the same account is the best evidence left. + * Identity settles it where there is a record: a finding already on the forge — sent from here, + * or pulled from there — is not sent again, however it has been reworded since, because the forge + * cannot update the comment already present. Where there is no record — a finding imported from a + * bundle, or posted from another machine — the same wording in the same place from the same + * account is the best evidence left. */ export function isAlreadyCommented( existing: ExistingComment[], diff --git a/packages/github/src/pr.ts b/packages/github/src/pr.ts index 537d8fb8..fe5d0a76 100644 --- a/packages/github/src/pr.ts +++ b/packages/github/src/pr.ts @@ -212,7 +212,7 @@ export interface CreateReviewOptions { commitSha?: string; /** The diff the commentable lines come from; the pull request's own patch when none is given. */ patch?: string; - /** The findings diffity's record says already went to this pull request, by thread id. */ + /** The findings diffity's record places on this pull request already, by thread id. */ postedThreadIds?: ReadonlySet; /** The account the review posts as, so a resend is told apart from another reviewer's remark. */ viewerLogin?: string | null;