diff --git a/internal/pipeline/existing_threads_test.go b/internal/pipeline/existing_threads_test.go new file mode 100644 index 0000000..bde3d0c --- /dev/null +++ b/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) + } +} diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 63b6628..102dc78 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,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{ diff --git a/internal/reviewplan/dedupe_raised_test.go b/internal/reviewplan/dedupe_raised_test.go new file mode 100644 index 0000000..a1def81 --- /dev/null +++ b/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 := " \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 := "\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: ""}, + } + + 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: " + 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:" ) @@ -97,6 +98,18 @@ type EventOptions struct { // ActionIDGenerator allocates a deterministic action id for an action kind. type ActionIDGenerator func(ActionKind) (string, error) +// ExistingThread is a review thread this identity already opened on the PR. +// +// Only threads this identity authored are carried: a human quoting the same +// code is not this review repeating itself, and must not suppress a finding. +type ExistingThread struct { + // Path is the file the thread is anchored to. + Path string + // Body is the thread's opening comment, as it was posted, including the + // decorations Build added to it. + Body string +} + // Request is the pure input to Build. type Request struct { PostMode PostMode @@ -110,6 +123,10 @@ type Request struct { // ThreadResponses are lifecycle-domain replies/resolutions for existing // inline discussion threads. ThreadResponses []review.ThreadResponseAction + // ExistingThreads are the review threads already on the PR that this + // identity authored. A finding repeating one of them is demoted to the + // rollup rather than posted again. + ExistingThreads []ExistingThread EventOptions EventOptions NoDiff bool @@ -455,6 +472,7 @@ func (b *builder) buildReview() (Plan, error) { } b.populateAnchoredFindings() + b.demoteFindingsAlreadyRaised() var actions []Action threadReplies, resolves, err := b.threadActions() @@ -569,6 +587,129 @@ func (b *builder) populateAnchoredFindings() { } } +// demoteFindingsAlreadyRaised keeps a finding out of a second inline thread +// when this identity has already opened one saying the same thing about the +// same file. +// +// Nothing else prevents this. A finding fixed by a later commit still sits in +// the cumulative diff, so it anchors again; posting is idempotent on run and +// action id, which a later run never matches. The result is the same finding +// posted repeatedly, most visibly against text that no longer exists at head, +// which teaches a reader to skim rather than read. +// +// Demotion, not deletion: the finding stays in the rollup, so a reviewer that +// still believes it is on record. A resolved thread is treated the same as an +// open one; both mean it has been said, and the open thread is still there to +// be read. +func (b *builder) demoteFindingsAlreadyRaised() { + if len(b.req.ExistingThreads) == 0 { + return + } + raised := make(map[string]struct{}, len(b.req.ExistingThreads)) + for _, thread := range b.req.ExistingThreads { + if key, ok := findingKey(thread.Path, thread.Body); ok { + raised[key] = struct{}{} + } + } + if len(raised) == 0 { + return + } + for id, anchored := range b.anchoredByID { + if anchored.Anchoring == review.AnchoringRollupOnly { + continue + } + key, ok := findingKey(anchored.FilePath, anchored.Body) + if !ok { + continue + } + if _, already := raised[key]; !already { + continue + } + anchored.Anchoring = review.AnchoringRollupOnly + anchored.Side = nil + anchored.Line = nil + anchored.DiffPosition = nil + b.anchoredByID[id] = anchored + } +} + +// findingKey identifies a finding by its file and its text, with everything +// Build adds to a posted comment stripped back off, so a body read from the +// host compares equal to the body about to be posted. +// +// The line is deliberately not part of the key. A fix shifts the lines around +// it, and the repeats worth suppressing are the ones that moved. +// +// Equality is exact after normalization rather than fuzzy. A near-match is a +// different claim often enough that suppressing it would lose real findings, +// and the repeats this exists for are identical. +func findingKey(path, body string) (string, bool) { + text := normalizeFindingText(body) + if text == "" { + return "", false + } + return strings.TrimSpace(path) + "\x00" + text, true +} + +// 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 := stripMarkerComments(body) + text = stripFileLevelPrefix(text) + text = strings.ReplaceAll(text, inlineFooter, " ") + return strings.ToLower(strings.Join(strings.Fields(text), " ")) +} + +// 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, markerPrefix) + if start < 0 { + out.WriteString(text) + return out.String() + } + out.WriteString(text[:start]) + out.WriteString(" ") + rest := text[start+len(markerPrefix):] + end := strings.Index(rest, htmlCommentClose) + if end < 0 { + // 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(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{