From 8b15d594002ac75a5b66fc86284146b722f9c62f Mon Sep 17 00:00:00 2001 From: piekstra Date: Mon, 31 Aug 2026 16:51:33 -0400 Subject: [PATCH 1/2] fix(reviewplan): stop re-posting a finding already raised on the PR A finding fixed by a later commit is still present in the cumulative base..head diff, so it anchors again on the next review. Posting is idempotent on run id and action id, which a later run never matches. Nothing anywhere compared a new finding to the threads already on the PR, so the same finding could be posted again on every run, most visibly against source text that no longer exists at head. A reader who sees that learns to skim review comments rather than read them. The planner now takes the threads this identity already opened and keeps a finding out of a second inline thread when one of them says the same thing about the same file. Matching is by file and text, never by line: a fix shifts the lines around it, and the repeats worth suppressing are the ones that moved. Everything the planner adds to a posted comment is stripped back off first, along with any HTML comments, because markers carry a run id that differs on every run and would otherwise make every comparison fail. Equality is exact once formatting is folded, since a near-match is a different claim often enough that suppressing it would lose real findings. Only threads this identity authored to report a finding are considered. A human quoting the same code is not the review repeating itself. Demotion, not deletion: the finding stays in the review body, so a reviewer that still believes it remains on record, and an unresolved thread is still there to be read. Resolved and open threads are treated alike; both mean it has been said. --- internal/pipeline/existing_threads_test.go | 68 +++++++++ internal/pipeline/pipeline.go | 28 +++- internal/reviewplan/dedupe_raised_test.go | 162 +++++++++++++++++++++ internal/reviewplan/reviewplan.go | 117 +++++++++++++++ 4 files changed, 374 insertions(+), 1 deletion(-) create mode 100644 internal/pipeline/existing_threads_test.go create mode 100644 internal/reviewplan/dedupe_raised_test.go diff --git a/internal/pipeline/existing_threads_test.go b/internal/pipeline/existing_threads_test.go new file mode 100644 index 0000000..08c0d54 --- /dev/null +++ b/internal/pipeline/existing_threads_test.go @@ -0,0 +1,68 @@ +package pipeline + +import ( + "testing" + + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + "github.com/open-cli-collective/codereview-cli/internal/threadcontext" +) + +func thread(path string, crAuthored, resolved bool, bodies ...string) threadcontext.Thread { + comments := make([]threadcontext.Comment, 0, len(bodies)) + for _, body := range bodies { + comments = append(comments, threadcontext.Comment{Body: body}) + } + return threadcontext.Thread{ + ID: gitprovider.ThreadID("t"), + Resolved: resolved, + Anchor: threadcontext.Anchor{Path: path}, + Comments: comments, + Status: threadcontext.Status{CRAuthoredFinding: crAuthored}, + } +} + +// A human quoting the same code is not this review repeating itself, so only +// threads this identity opened to report a finding can suppress one. +func TestExistingFindingThreadsKeepsOnlyOurFindingThreads(t *testing.T) { + got := existingFindingThreads([]threadcontext.Thread{ + thread("a.go", true, true, "ours, resolved"), + thread("b.go", true, false, "ours, open"), + thread("c.go", false, true, "someone else's"), + }) + + if len(got) != 2 { + t.Fatalf("kept %d threads, want 2: %+v", len(got), got) + } + for _, e := range got { + if e.Path == "c.go" { + t.Fatalf("a thread this identity did not author was kept: %+v", e) + } + } + // Resolution is carried through rather than filtered on: an open thread + // means the finding has been said too. + if !got[0].Resolved || got[1].Resolved { + t.Fatalf("resolution was not carried through: %+v", got) + } +} + +// The opening comment is the finding; the rest is the conversation about it, +// and matching against a reply would compare against the wrong text. +func TestExistingFindingThreadsCarriesTheOpeningComment(t *testing.T) { + got := existingFindingThreads([]threadcontext.Thread{ + thread("a.go", true, true, "the finding", "fixed in abc123", "thanks"), + }) + + if len(got) != 1 { + t.Fatalf("kept %d threads, want 1", len(got)) + } + if got[0].Body != "the finding" { + t.Fatalf("body = %q, want the opening comment", got[0].Body) + } +} + +// A thread with no comments carries no finding to compare against. +func TestExistingFindingThreadsSkipsAThreadWithNoComments(t *testing.T) { + if got := existingFindingThreads([]threadcontext.Thread{thread("a.go", true, true)}); len(got) != 0 { + t.Fatalf("kept %d threads, want 0: %+v", len(got), got) + } +} diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 63b6628..9bdc158 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -928,6 +928,7 @@ func executeLLMPhases(ctx context.Context, opts Options, req Request, mode execu findingSessions: findingSessions, reviewerFailures: reviewerFailures, reviewerCoverage: reviewerCoverage, + threadContext: prepared.threadContext, startedAt: now, }) if err != nil { @@ -2341,7 +2342,31 @@ type planRunInputs struct { findingSessions map[review.FindingID]string reviewerFailures []ReviewerFailure reviewerCoverage []reviewplan.ReviewerCoverageSummary - startedAt time.Time + // threadContext is every normalized thread already on the PR. The planner + // reads it to avoid opening a second thread for a finding it has already + // raised. + threadContext []threadcontext.Thread + startedAt time.Time +} + +// existingFindingThreads reduces the PR's threads to the ones this identity +// opened to report a finding, which are the only ones a repeat could duplicate. +// A human quoting the same code is not this review repeating itself. +func existingFindingThreads(threads []threadcontext.Thread) []reviewplan.ExistingThread { + var out []reviewplan.ExistingThread + for _, thread := range threads { + if !thread.Status.CRAuthoredFinding || len(thread.Comments) == 0 { + continue + } + // The opening comment is the finding; later comments are the + // conversation about it. + out = append(out, reviewplan.ExistingThread{ + Path: thread.Anchor.Path, + Body: thread.Comments[0].Body, + Resolved: thread.Resolved, + }) + } + return out } func (opts Options) buildRunSummary(req Request, inputs planRunInputs) (reviewplan.RunSummary, map[review.FindingID]string) { @@ -2756,6 +2781,7 @@ func (opts Options) buildPlan(req Request, pr gitprovider.PR, postMode reviewpla Rollup: rollup, ThreadActions: threadActions, ThreadResponses: append([]review.ThreadResponseAction(nil), runInputs.threadResponses...), + ExistingThreads: existingFindingThreads(runInputs.threadContext), RepoGuidanceUnavailable: dossier.RepoGuidanceUnavailableReason(runInputs.repoSources) != "", RepoGuidanceUnavailableReason: dossier.RepoGuidanceUnavailableReason(runInputs.repoSources), EventOptions: reviewplan.EventOptions{ diff --git a/internal/reviewplan/dedupe_raised_test.go b/internal/reviewplan/dedupe_raised_test.go new file mode 100644 index 0000000..950f39b --- /dev/null +++ b/internal/reviewplan/dedupe_raised_test.go @@ -0,0 +1,162 @@ +package reviewplan + +import ( + "strings" + "testing" + + "github.com/open-cli-collective/codereview-cli/internal/review" +) + +// postedBody is what Build renders into a comment, which is what a later run +// reads back from the host when it lists threads. +func postedBody(t *testing.T, req Request) string { + t.Helper() + plan, err := Build(req) + if err != nil { + t.Fatal(err) + } + inline := actionsOfKind(plan.Actions, ActionKindInlineComment) + if len(inline) != 1 { + t.Fatalf("want one inline comment to read a body from, got %d", len(inline)) + } + return inline[0].InlineComment.Body +} + +func inlineCount(t *testing.T, req Request) int { + t.Helper() + plan, err := Build(req) + if err != nil { + t.Fatal(err) + } + return len(actionsOfKind(plan.Actions, ActionKindInlineComment)) +} + +// A finding fixed by a later commit is still in the cumulative diff, so it +// anchors again and posts again. Repeating it against text that no longer +// exists teaches a reader to skim. +func TestBuildDoesNotRepeatAFindingAlreadyRaised(t *testing.T) { + first := baseRequest() + body := postedBody(t, first) + + second := baseRequest() + second.ExistingThreads = []ExistingThread{{Path: "main.go", Body: body, Resolved: true}} + + if got := inlineCount(t, second); got != 0 { + t.Fatalf("posted %d inline comments for a finding already raised, want 0", got) + } +} + +// Suppressed from the thread, not from the review: a reviewer that still +// believes the finding stays on record. +func TestBuildKeepsARepeatedFindingInTheRollup(t *testing.T) { + req := baseRequest() + body := postedBody(t, baseRequest()) + req.ExistingThreads = []ExistingThread{{Path: "main.go", Body: body, Resolved: true}} + + plan, err := Build(req) + if err != nil { + t.Fatal(err) + } + submits := actionsOfKind(plan.Actions, ActionKindSubmitReview) + if len(submits) == 0 { + t.Fatal("no submit-review action was planned") + } + if !strings.Contains(submits[0].SubmitReview.Body, "finding body") { + t.Fatalf("the review body does not carry the suppressed finding:\n%s", submits[0].SubmitReview.Body) + } +} + +// An open thread means the finding has been said too, and it is still there to +// be read. +func TestBuildDoesNotRepeatAFindingOnAnUnresolvedThread(t *testing.T) { + req := baseRequest() + body := postedBody(t, baseRequest()) + req.ExistingThreads = []ExistingThread{{Path: "main.go", Body: body, Resolved: false}} + + if got := inlineCount(t, req); got != 0 { + t.Fatalf("posted %d inline comments duplicating an open thread, want 0", got) + } +} + +// The repeats worth suppressing are the ones a fix moved, so the line cannot be +// part of what identifies a finding. +func TestBuildSuppressesARepeatWhoseLineMoved(t *testing.T) { + req := baseRequest() + body := postedBody(t, baseRequest()) + req.Findings = []review.Finding{finding("f-1", "main.go", + review.Anchor{Kind: review.AnchorKindLine, Side: review.DiffSideRight, Line: 14})} + req.ExistingThreads = []ExistingThread{{Path: "main.go", Body: body, Resolved: true}} + + if got := inlineCount(t, req); got != 0 { + t.Fatalf("posted %d inline comments for a finding that only moved, want 0", got) + } +} + +// Markers carry a run id that differs every run, so leaving them in the +// comparison would make every one fail and suppress nothing. +func TestBuildSuppressesARepeatDespiteADifferentRunMarker(t *testing.T) { + req := baseRequest() + // What the host actually stores: the planned body with the run's marker + // prepended at post time. The run id differs on every run. + stale := " \n\n" + + postedBody(t, baseRequest()) + req.ExistingThreads = []ExistingThread{{Path: "main.go", Body: stale, Resolved: true}} + + if got := inlineCount(t, req); got != 0 { + t.Fatalf("posted %d inline comments; a differing run marker defeated the match", got) + } +} + +// Suppressing a near-match would lose real findings, so equality is exact once +// formatting is folded. +func TestBuildStillPostsADifferentFindingOnTheSameFile(t *testing.T) { + req := baseRequest() + other := "\n\nsomething else entirely\n\n" + inlineFooter + req.ExistingThreads = []ExistingThread{{Path: "main.go", Body: other, Resolved: true}} + + if got := inlineCount(t, req); got != 1 { + t.Fatalf("posted %d inline comments, want 1: a different finding must still be raised", got) + } +} + +// The same words about a different file are a different claim. +func TestBuildStillPostsTheSameTextAboutAnotherFile(t *testing.T) { + req := baseRequest() + body := postedBody(t, baseRequest()) + req.ExistingThreads = []ExistingThread{{Path: "other.go", Body: body, Resolved: true}} + + if got := inlineCount(t, req); got != 1 { + t.Fatalf("posted %d inline comments, want 1: another file is another claim", got) + } +} + +// A thread carrying no text of its own cannot establish that anything was said. +func TestBuildIgnoresAnEmptyExistingThread(t *testing.T) { + req := baseRequest() + req.ExistingThreads = []ExistingThread{ + {Path: "main.go", Body: " \n\n", Resolved: true}, + {Path: "main.go", Body: "", Resolved: true}, + } + + if got := inlineCount(t, req); got != 1 { + t.Fatalf("posted %d inline comments, want 1: an empty thread suppressed a finding", got) + } +} + +// An unterminated comment must not leak marker text into the comparison, and +// must not panic. +func TestNormalizeFindingTextHandlesAnUnterminatedComment(t *testing.T) { + if got := normalizeFindingText("before ` spans. Markers are written that way, +// and they carry a run id that differs on every run, so leaving them in would +// make every comparison fail. +func stripHTMLComments(text string) string { + var out strings.Builder + for { + start := strings.Index(text, "") + if end < 0 { + // An unterminated comment: nothing after it is markup worth + // keeping, and dropping it beats emitting the marker text. + return out.String() + } + text = rest[end+len("-->"):] + } +} + func (b *builder) anchorFinding(finding review.Finding) AnchoredFinding { body := sanitize(finding.Body) anchored := AnchoredFinding{ From d762a66842a8a5eb9b4da2ad7b9e3c98245e9975 Mon Sep 17 00:00:00 2001 From: piekstra Date: Mon, 31 Aug 2026 17:09:09 -0400 Subject: [PATCH 2/2] fix(reviewplan): match the finding text a host actually stores Three corrections, two of which meant the suppression could not fire on the case it exists for. The file-level fallback header interpolates the file path after its prefix. Stripping only the literal prefix left the path in the text, so a stored file-level comment normalized to "main.go finding body" while the finding it came from normalized to "finding body" and never matched. On a host without native file-level comments that is every file-anchored finding, and such a finding re-anchors to the first hunk on every run, so it is the class most in need of this. Stripping every HTML comment took text out of the comparison that stays in the posted body. A finding may quote an HTML, XML, or JSX comment out of the diff, and two findings differing only inside such a quote collapsed to one key. Only this tool's own markers are stripped now. The authorship filter read Status.CRAuthoredFinding, which is also set when this identity merely replied to a thread someone else opened. The opening comment's own author and marker are tested instead, so a human's thread cannot carry their text in as a finding of ours and suppress a real one. ExistingThread.Resolved is dropped. Resolution is deliberately not part of the decision, so a field nothing branches on only invited a caller to think it was. --- internal/pipeline/existing_threads_test.go | 66 +++++++++------ internal/pipeline/pipeline.go | 16 +++- internal/reviewplan/dedupe_raised_test.go | 99 ++++++++++++++++------ internal/reviewplan/reviewplan.go | 58 +++++++++---- 4 files changed, 164 insertions(+), 75 deletions(-) diff --git a/internal/pipeline/existing_threads_test.go b/internal/pipeline/existing_threads_test.go index 08c0d54..bde3d0c 100644 --- a/internal/pipeline/existing_threads_test.go +++ b/internal/pipeline/existing_threads_test.go @@ -7,49 +7,63 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/threadcontext" ) -func thread(path string, crAuthored, resolved bool, bodies ...string) threadcontext.Thread { - comments := make([]threadcontext.Comment, 0, len(bodies)) - for _, body := range bodies { - comments = append(comments, threadcontext.Comment{Body: body}) +// comment builds one normalized thread comment with the two flags the filter +// reads. +func comment(body string, ours, findingMarker bool) threadcontext.Comment { + return threadcontext.Comment{ + Body: body, + AuthoredByPostingIdentity: ours, + HasFindingMarker: findingMarker, } +} + +func thread(path string, comments ...threadcontext.Comment) threadcontext.Thread { return threadcontext.Thread{ ID: gitprovider.ThreadID("t"), - Resolved: resolved, Anchor: threadcontext.Anchor{Path: path}, Comments: comments, - Status: threadcontext.Status{CRAuthoredFinding: crAuthored}, } } -// A human quoting the same code is not this review repeating itself, so only -// threads this identity opened to report a finding can suppress one. -func TestExistingFindingThreadsKeepsOnlyOurFindingThreads(t *testing.T) { +// Only a thread this identity opened to report a finding can be one it repeats. +func TestExistingFindingThreadsKeepsOnlyOurOwnFindingThreads(t *testing.T) { got := existingFindingThreads([]threadcontext.Thread{ - thread("a.go", true, true, "ours, resolved"), - thread("b.go", true, false, "ours, open"), - thread("c.go", false, true, "someone else's"), + thread("a.go", comment("ours", true, true)), + thread("b.go", comment("someone else's", false, true)), + thread("c.go", comment("ours, but not a finding", true, false)), }) - if len(got) != 2 { - t.Fatalf("kept %d threads, want 2: %+v", len(got), got) + if len(got) != 1 { + t.Fatalf("kept %d threads, want 1: %+v", len(got), got) } - for _, e := range got { - if e.Path == "c.go" { - t.Fatalf("a thread this identity did not author was kept: %+v", e) - } + if got[0].Path != "a.go" { + t.Fatalf("kept the wrong thread: %+v", got[0]) } - // Resolution is carried through rather than filtered on: an open thread - // means the finding has been said too. - if !got[0].Resolved || got[1].Resolved { - t.Fatalf("resolution was not carried through: %+v", got) +} + +// A thread a human opened and this identity only replied to carries the human's +// text. Treating it as ours would let it suppress a real finding. +func TestExistingFindingThreadsSkipsAThreadWeOnlyRepliedTo(t *testing.T) { + got := existingFindingThreads([]threadcontext.Thread{ + thread("a.go", + comment("a human's finding", false, false), + comment("our reply", true, true), + ), + }) + + if len(got) != 0 { + t.Fatalf("kept %d threads, want 0: a thread we only replied to was treated as ours: %+v", len(got), got) } } -// The opening comment is the finding; the rest is the conversation about it, -// and matching against a reply would compare against the wrong text. +// The opening comment is the finding; matching against a reply would compare +// against the wrong text. func TestExistingFindingThreadsCarriesTheOpeningComment(t *testing.T) { got := existingFindingThreads([]threadcontext.Thread{ - thread("a.go", true, true, "the finding", "fixed in abc123", "thanks"), + thread("a.go", + comment("the finding", true, true), + comment("fixed in abc123", false, false), + ), }) if len(got) != 1 { @@ -62,7 +76,7 @@ func TestExistingFindingThreadsCarriesTheOpeningComment(t *testing.T) { // A thread with no comments carries no finding to compare against. func TestExistingFindingThreadsSkipsAThreadWithNoComments(t *testing.T) { - if got := existingFindingThreads([]threadcontext.Thread{thread("a.go", true, true)}); len(got) != 0 { + if got := existingFindingThreads([]threadcontext.Thread{thread("a.go")}); len(got) != 0 { t.Fatalf("kept %d threads, want 0: %+v", len(got), got) } } diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 9bdc158..102dc78 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -2352,18 +2352,26 @@ type planRunInputs struct { // existingFindingThreads reduces the PR's threads to the ones this identity // opened to report a finding, which are the only ones a repeat could duplicate. // A human quoting the same code is not this review repeating itself. +// +// The opening comment is tested directly rather than through +// Status.CRAuthoredFinding, which is also set when this identity merely replied +// to a thread someone else opened. That would carry the other author's text in +// as a finding of ours and let it suppress a real one. func existingFindingThreads(threads []threadcontext.Thread) []reviewplan.ExistingThread { var out []reviewplan.ExistingThread for _, thread := range threads { - if !thread.Status.CRAuthoredFinding || len(thread.Comments) == 0 { + if len(thread.Comments) == 0 { continue } // The opening comment is the finding; later comments are the // conversation about it. + opening := thread.Comments[0] + if !opening.AuthoredByPostingIdentity || !opening.HasFindingMarker { + continue + } out = append(out, reviewplan.ExistingThread{ - Path: thread.Anchor.Path, - Body: thread.Comments[0].Body, - Resolved: thread.Resolved, + Path: thread.Anchor.Path, + Body: opening.Body, }) } return out diff --git a/internal/reviewplan/dedupe_raised_test.go b/internal/reviewplan/dedupe_raised_test.go index 950f39b..a1def81 100644 --- a/internal/reviewplan/dedupe_raised_test.go +++ b/internal/reviewplan/dedupe_raised_test.go @@ -39,7 +39,7 @@ func TestBuildDoesNotRepeatAFindingAlreadyRaised(t *testing.T) { body := postedBody(t, first) second := baseRequest() - second.ExistingThreads = []ExistingThread{{Path: "main.go", Body: body, Resolved: true}} + second.ExistingThreads = []ExistingThread{{Path: "main.go", Body: body}} if got := inlineCount(t, second); got != 0 { t.Fatalf("posted %d inline comments for a finding already raised, want 0", got) @@ -51,7 +51,7 @@ func TestBuildDoesNotRepeatAFindingAlreadyRaised(t *testing.T) { func TestBuildKeepsARepeatedFindingInTheRollup(t *testing.T) { req := baseRequest() body := postedBody(t, baseRequest()) - req.ExistingThreads = []ExistingThread{{Path: "main.go", Body: body, Resolved: true}} + req.ExistingThreads = []ExistingThread{{Path: "main.go", Body: body}} plan, err := Build(req) if err != nil { @@ -66,18 +66,6 @@ func TestBuildKeepsARepeatedFindingInTheRollup(t *testing.T) { } } -// An open thread means the finding has been said too, and it is still there to -// be read. -func TestBuildDoesNotRepeatAFindingOnAnUnresolvedThread(t *testing.T) { - req := baseRequest() - body := postedBody(t, baseRequest()) - req.ExistingThreads = []ExistingThread{{Path: "main.go", Body: body, Resolved: false}} - - if got := inlineCount(t, req); got != 0 { - t.Fatalf("posted %d inline comments duplicating an open thread, want 0", got) - } -} - // The repeats worth suppressing are the ones a fix moved, so the line cannot be // part of what identifies a finding. func TestBuildSuppressesARepeatWhoseLineMoved(t *testing.T) { @@ -85,7 +73,7 @@ func TestBuildSuppressesARepeatWhoseLineMoved(t *testing.T) { body := postedBody(t, baseRequest()) req.Findings = []review.Finding{finding("f-1", "main.go", review.Anchor{Kind: review.AnchorKindLine, Side: review.DiffSideRight, Line: 14})} - req.ExistingThreads = []ExistingThread{{Path: "main.go", Body: body, Resolved: true}} + req.ExistingThreads = []ExistingThread{{Path: "main.go", Body: body}} if got := inlineCount(t, req); got != 0 { t.Fatalf("posted %d inline comments for a finding that only moved, want 0", got) @@ -101,7 +89,7 @@ func TestBuildSuppressesARepeatDespiteADifferentRunMarker(t *testing.T) { stale := " \n\n" + postedBody(t, baseRequest()) - req.ExistingThreads = []ExistingThread{{Path: "main.go", Body: stale, Resolved: true}} + req.ExistingThreads = []ExistingThread{{Path: "main.go", Body: stale}} if got := inlineCount(t, req); got != 0 { t.Fatalf("posted %d inline comments; a differing run marker defeated the match", got) @@ -113,7 +101,7 @@ func TestBuildSuppressesARepeatDespiteADifferentRunMarker(t *testing.T) { func TestBuildStillPostsADifferentFindingOnTheSameFile(t *testing.T) { req := baseRequest() other := "\n\nsomething else entirely\n\n" + inlineFooter - req.ExistingThreads = []ExistingThread{{Path: "main.go", Body: other, Resolved: true}} + req.ExistingThreads = []ExistingThread{{Path: "main.go", Body: other}} if got := inlineCount(t, req); got != 1 { t.Fatalf("posted %d inline comments, want 1: a different finding must still be raised", got) @@ -124,7 +112,7 @@ func TestBuildStillPostsADifferentFindingOnTheSameFile(t *testing.T) { func TestBuildStillPostsTheSameTextAboutAnotherFile(t *testing.T) { req := baseRequest() body := postedBody(t, baseRequest()) - req.ExistingThreads = []ExistingThread{{Path: "other.go", Body: body, Resolved: true}} + req.ExistingThreads = []ExistingThread{{Path: "other.go", Body: body}} if got := inlineCount(t, req); got != 1 { t.Fatalf("posted %d inline comments, want 1: another file is another claim", got) @@ -135,8 +123,8 @@ func TestBuildStillPostsTheSameTextAboutAnotherFile(t *testing.T) { func TestBuildIgnoresAnEmptyExistingThread(t *testing.T) { req := baseRequest() req.ExistingThreads = []ExistingThread{ - {Path: "main.go", Body: " \n\n", Resolved: true}, - {Path: "main.go", Body: "", Resolved: true}, + {Path: "main.go", Body: " \n\n"}, + {Path: "main.go", Body: ""}, } if got := inlineCount(t, req); got != 1 { @@ -144,14 +132,6 @@ func TestBuildIgnoresAnEmptyExistingThread(t *testing.T) { } } -// An unterminated comment must not leak marker text into the comparison, and -// must not panic. -func TestNormalizeFindingTextHandlesAnUnterminatedComment(t *testing.T) { - if got := normalizeFindingText("before " + req.Findings = []review.Finding{f} + return req + } + + req := quoting("beta") + req.ExistingThreads = []ExistingThread{{Path: "main.go", Body: postedBody(t, quoting("alpha"))}} + + if got := inlineCount(t, req); got != 1 { + t.Fatalf("posted %d inline comments, want 1: two findings differing only "+ + "inside a quoted comment were treated as the same", got) + } +} + +// The marker's run id differs on every run, so it must not reach the key even +// when the span is unterminated. +func TestNormalizeFindingTextDropsAnUnterminatedMarker(t *testing.T) { + if got := normalizeFindingText("before " + markerPrefix + "run-id=abc"); got != "before" { + t.Fatalf("normalized = %q, want %q", got, "before") + } +} + +// An unterminated comment that is not a marker is ordinary quoted text. +func TestNormalizeFindingTextKeepsAnUnterminatedNonMarkerComment(t *testing.T) { + if got := normalizeFindingText("before " escapedMarkerPrefix = "<!-- codereview:" ) @@ -107,8 +108,6 @@ type ExistingThread struct { // Body is the thread's opening comment, as it was posted, including the // decorations Build added to it. Body string - // Resolved reports whether the thread has been marked resolved. - Resolved bool } // Request is the pure input to Build. @@ -652,40 +651,65 @@ func findingKey(path, body string) (string, bool) { return strings.TrimSpace(path) + "\x00" + text, true } -// normalizeFindingText strips the decorations this package puts on a posted -// comment, along with any HTML comments the host carries, and folds whitespace -// so that formatting alone cannot make the same text look new. +// normalizeFindingText strips everything a posted comment carries that is not +// the finding, and folds whitespace so that formatting alone cannot make the +// same text look new. func normalizeFindingText(body string) string { - text := stripHTMLComments(body) + text := stripMarkerComments(body) + text = stripFileLevelPrefix(text) text = strings.ReplaceAll(text, inlineFooter, " ") - text = strings.ReplaceAll(text, fileLevelFallbackPrefix, " ") return strings.ToLower(strings.Join(strings.Fields(text), " ")) } -// stripHTMLComments removes `` spans. Markers are written that way, -// and they carry a run id that differs on every run, so leaving them in would -// make every comparison fail. -func stripHTMLComments(text string) string { +// stripMarkerComments removes this tool's own `` spans, +// which carry a run id that differs on every run and would otherwise make every +// comparison fail. +// +// Only those spans. A finding may legitimately quote an HTML, XML, or JSX +// comment out of the diff, and dropping that would take real text out of the +// comparison while leaving it in the posted body, so two findings differing +// only inside a quoted comment would collapse to one key. +func stripMarkerComments(text string) string { var out strings.Builder for { - start := strings.Index(text, "") + rest := text[start+len(markerPrefix):] + end := strings.Index(rest, htmlCommentClose) if end < 0 { - // An unterminated comment: nothing after it is markup worth - // keeping, and dropping it beats emitting the marker text. + // An unterminated marker. Emitting its text would put a run id into + // the key, so everything from here is dropped. return out.String() } - text = rest[end+len("-->"):] + text = rest[end+len(htmlCommentClose):] } } +// stripFileLevelPrefix removes the file-level fallback header, which is the +// prefix followed by the interpolated file path on the same line. +// +// Dropping only the literal prefix would leave the path in the text, so a +// stored file-level comment would never match the finding it came from. On a +// host without native file-level comments that is every file-anchored finding, +// which is the class most in need of this: such a finding re-anchors to the +// first hunk on every run. +func stripFileLevelPrefix(text string) string { + start := strings.Index(text, fileLevelFallbackPrefix) + if start < 0 { + return text + } + rest := text[start+len(fileLevelFallbackPrefix):] + if end := strings.IndexByte(rest, '\n'); end >= 0 { + return text[:start] + " " + rest[end+1:] + } + return text[:start] + " " +} + func (b *builder) anchorFinding(finding review.Finding) AnchoredFinding { body := sanitize(finding.Body) anchored := AnchoredFinding{