fix(reviewplan): stop re-posting a finding already raised on the PR

internal/pipeline/existing_threads_test.go

@@ -0,0 +1,82 @@
+package pipeline
+
+import (
+	"testing"
+
+	"github.com/open-cli-collective/codereview-cli/internal/gitprovider"
+	"github.com/open-cli-collective/codereview-cli/internal/threadcontext"
+)
+
+// 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"),
+		Anchor:   threadcontext.Anchor{Path: path},
+		Comments: comments,
+	}
+}
+
+// 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", 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) != 1 {
+		t.Fatalf("kept %d threads, want 1: %+v", len(got), got)
+	}
+	if got[0].Path != "a.go" {
+		t.Fatalf("kept the wrong thread: %+v", got[0])
+	}
+}
+
+// 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; matching against a reply would compare
+// against the wrong text.
+func TestExistingFindingThreadsCarriesTheOpeningComment(t *testing.T) {
+	got := existingFindingThreads([]threadcontext.Thread{
+		thread("a.go",
+			comment("the finding", true, true),
+			comment("fixed in abc123", false, false),
+		),
+	})
+
+	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")}); len(got) != 0 {
+		t.Fatalf("kept %d threads, want 0: %+v", len(got), got)
+	}
+}

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,39 @@ 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.
+//
+// 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 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: opening.Body,
+		})
+	}
+	return out
 }
 
 func (opts Options) buildRunSummary(req Request, inputs planRunInputs) (reviewplan.RunSummary, map[review.FindingID]string) {
@@ -2756,6 +2789,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{

internal/reviewplan/dedupe_raised_test.go

@@ -0,0 +1,205 @@
+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}}
+
+	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}}
+
+	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)
+	}
+}
+
+// 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}}
+
+	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 := "<!-- codereview:skip --> <!-- codereview:run-id=" +
+		"11111111-2222-3333-4444-555555555555:action=inline_comment-001 -->\n\n" +
+		postedBody(t, baseRequest())
+	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)
+	}
+}
+
+// Suppressing a near-match would lose real findings, so equality is exact once
+// formatting is folded.
+func TestBuildStillPostsADifferentFindingOnTheSameFile(t *testing.T) {
+	req := baseRequest()
+	other := "<!-- codereview:skip -->\n\nsomething else entirely\n\n" + inlineFooter
+	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)
+	}
+}
+
+// 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}}
+
+	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"},
+		{Path: "main.go", Body: "<!-- codereview:skip -->"},
+	}
+
+	if got := inlineCount(t, req); got != 1 {
+		t.Fatalf("posted %d inline comments, want 1: an empty thread suppressed a finding", got)
+	}
+}
+
+// Formatting alone must not make the same text look new.
+func TestNormalizeFindingTextFoldsFormatting(t *testing.T) {
+	a := normalizeFindingText("Finding   body\n\nhere")
+	b := normalizeFindingText("finding body here")
+	if a != b {
+		t.Fatalf("normalized %q and %q differently", a, b)
+	}
+}
+
+// On a host without native file-level comments every file-anchored finding
+// takes the fallback path, and such a finding re-anchors to the first hunk on
+// each run, so it is the class most in need of suppression. The rendered header
+// interpolates the file path, so stripping only the literal prefix would leave
+// the path in the text and nothing would ever match.
+func TestBuildSuppressesARepeatedFileLevelFinding(t *testing.T) {
+	fallback := func() Request {
+		req := baseRequest()
+		req.ProviderCaps.NativeFileLevelComments = false
+		req.Findings = []review.Finding{finding("f-1", "main.go",
+			review.Anchor{Kind: review.AnchorKindFile})}
+		return req
+	}
+
+	body := postedBody(t, fallback())
+	if !strings.Contains(body, fileLevelFallbackPrefix+"main.go") {
+		t.Fatalf("this test is not exercising the fallback path: %q", body)
+	}
+
+	req := fallback()
+	req.ExistingThreads = []ExistingThread{{Path: "main.go", Body: body}}
+	if got := inlineCount(t, req); got != 0 {
+		t.Fatalf("posted %d inline comments for a repeated file-level finding, want 0", got)
+	}
+}
+
+// A finding may quote an HTML, XML, or JSX comment out of the diff. Dropping it
+// 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.
+func TestBuildKeepsTextQuotedInsideANonMarkerComment(t *testing.T) {
+	quoting := func(quoted string) Request {
+		req := baseRequest()
+		f := finding("f-1", "main.go",
+			review.Anchor{Kind: review.AnchorKindLine, Side: review.DiffSideRight, Line: 12})
+		f.Body = "this line is wrong: <!-- " + quoted + " -->"
+		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 <!-- never closed"); !strings.Contains(got, "never closed") {
+		t.Fatalf("normalized = %q, want the quoted text kept", got)
+	}
+}