From 6bc7f742d661a7789987c116f9483da04d361b1e Mon Sep 17 00:00:00 2001 From: fullsend-code <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Sun, 2 Aug 2026 02:55:59 +0000 Subject: [PATCH 1/2] feat(#788): minimize stale inline review comments on re-review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before this change, when the review agent re-reviewed a PR, inline comments from prior runs accumulated in the diff view. The sticky comment handled history correctly (collapsing old runs into
blocks), and formal review bodies were minimized, but inline review comments — what developers actually see on each code line — were never cleaned up between runs. Add ListPullRequestReviewComments to the forge.Client interface to retrieve existing inline comments on a PR. On re-review, the new minimizeStaleInlineComments function finds all inline comments posted by the bot and minimizes them as OUTDATED before posting the current run's findings. This mirrors the existing minimizeStaleReviews pattern for review bodies. Changes: - forge.go: add PullRequestReviewComment struct and ListPullRequestReviewComments to Client interface - github.go: implement using GET /pulls/{number}/comments with pagination - gitlab/mr.go: return ErrNotSupported (GitLab has no comment minimize feature) - fake.go: add PRReviewComments field and method implementation - postreview.go: add minimizeStaleInlineComments(), call it from submitFormalReview after minimizing stale review bodies - postreview_test.go: add tests for the new function including own-comments-only filtering, error tolerance, ErrNotSupported silent skip, and full integration with submitFormalReview Closes #788 --- internal/cli/postreview.go | 40 ++++++++++++++ internal/cli/postreview_test.go | 97 +++++++++++++++++++++++++++++++++ internal/forge/fake.go | 18 ++++++ internal/forge/forge.go | 19 +++++++ internal/forge/github/github.go | 43 +++++++++++++++ internal/forge/gitlab/mr.go | 7 +++ 6 files changed, 224 insertions(+) diff --git a/internal/cli/postreview.go b/internal/cli/postreview.go index 13d0f58c01..bbd1f11c42 100644 --- a/internal/cli/postreview.go +++ b/internal/cli/postreview.go @@ -317,6 +317,7 @@ func submitFormalReview(ctx context.Context, client forge.Client, owner, repo st } else { dismissStaleRequestChanges(ctx, client, owner, repo, pr, event, user, reviews, printer) minimizeStaleReviews(ctx, client, user, reviews, printer) + minimizeStaleInlineComments(ctx, client, owner, repo, pr, user, printer) } var diffHunks map[string][][2]int @@ -618,6 +619,45 @@ func minimizeStaleReviews(ctx context.Context, client forge.Client, user string, printer.StepDone("Stale reviews minimized") } +// minimizeStaleInlineComments finds all inline review comments posted +// by the given user on the PR and minimizes them. This prevents +// duplicate inline comments from accumulating across re-review runs. +// The sticky comment already preserves full review history, so +// minimizing prior inline comments loses no information. +// +// Errors are non-fatal — failing to minimize stale inline comments +// should not prevent the new review from being submitted. +func minimizeStaleInlineComments(ctx context.Context, client forge.Client, owner, repo string, pr int, user string, printer *ui.Printer) { + comments, err := client.ListPullRequestReviewComments(ctx, owner, repo, pr) + if err != nil { + // ErrNotSupported is expected for forges that don't support this + // operation (e.g., GitLab). Silently skip in that case. + if !forge.IsNotSupported(err) { + printer.StepInfo(fmt.Sprintf("Could not list inline comments (%v), skipping stale inline comment cleanup", err)) + } + return + } + + var stale []forge.PullRequestReviewComment + for _, c := range comments { + if c.User == user { + stale = append(stale, c) + } + } + + if len(stale) == 0 { + return + } + + printer.StepStart(fmt.Sprintf("Minimizing %d stale inline comment(s)", len(stale))) + for _, c := range stale { + if err := client.MinimizeComment(ctx, c.NodeID, "OUTDATED"); err != nil { + printer.StepInfo(fmt.Sprintf("Warning: could not minimize inline comment %s: %v", c.NodeID, err)) + } + } + printer.StepDone("Stale inline comments minimized") +} + // sanitizeReviewResult runs the security output pipeline over all // user-visible text fields in a ReviewResult. This catches leaked // secrets and zero-width–obfuscated tokens before they reach the diff --git a/internal/cli/postreview_test.go b/internal/cli/postreview_test.go index a92a5ad451..7372fc6af8 100644 --- a/internal/cli/postreview_test.go +++ b/internal/cli/postreview_test.go @@ -1511,3 +1511,100 @@ func TestBuildFallbackReviewBody(t *testing.T) { assert.Equal(t, "", body) }) } + +func TestMinimizeStaleInlineComments_MinimizesOwnComments(t *testing.T) { + fc := forge.NewFakeClient() + fc.PRReviewComments = map[string][]forge.PullRequestReviewComment{ + "acme/repo/1": { + {ID: 1, NodeID: "PRC_1", User: "fullsend-bot", Path: "a.go", Line: 10, Body: "old finding"}, + {ID: 2, NodeID: "PRC_2", User: "human-reviewer", Path: "a.go", Line: 12, Body: "human comment"}, + {ID: 3, NodeID: "PRC_3", User: "fullsend-bot", Path: "b.go", Line: 5, Body: "another old finding"}, + }, + } + + printer := ui.New(io.Discard) + minimizeStaleInlineComments(context.Background(), fc, "acme", "repo", 1, "fullsend-bot", printer) + + require.Len(t, fc.MinimizedComments, 2) + assert.Equal(t, "PRC_1", fc.MinimizedComments[0].NodeID) + assert.Equal(t, "OUTDATED", fc.MinimizedComments[0].Reason) + assert.Equal(t, "PRC_3", fc.MinimizedComments[1].NodeID) + assert.Equal(t, "OUTDATED", fc.MinimizedComments[1].Reason) +} + +func TestMinimizeStaleInlineComments_NoComments(t *testing.T) { + fc := forge.NewFakeClient() + printer := ui.New(io.Discard) + minimizeStaleInlineComments(context.Background(), fc, "acme", "repo", 1, "fullsend-bot", printer) + assert.Empty(t, fc.MinimizedComments) +} + +func TestMinimizeStaleInlineComments_ListErrorIsNonFatal(t *testing.T) { + fc := forge.NewFakeClient() + fc.Errors["ListPullRequestReviewComments"] = fmt.Errorf("API error") + + var out bytes.Buffer + printer := ui.New(&out) + minimizeStaleInlineComments(context.Background(), fc, "acme", "repo", 1, "fullsend-bot", printer) + + assert.Empty(t, fc.MinimizedComments) + assert.Contains(t, out.String(), "Could not list inline comments") +} + +func TestMinimizeStaleInlineComments_MinimizeErrorIsNonFatal(t *testing.T) { + fc := forge.NewFakeClient() + fc.Errors["MinimizeComment"] = fmt.Errorf("GraphQL error") + fc.PRReviewComments = map[string][]forge.PullRequestReviewComment{ + "acme/repo/1": { + {ID: 1, NodeID: "PRC_1", User: "fullsend-bot", Path: "a.go", Line: 10, Body: "old"}, + {ID: 2, NodeID: "PRC_2", User: "fullsend-bot", Path: "b.go", Line: 5, Body: "old2"}, + }, + } + + var out bytes.Buffer + printer := ui.New(&out) + minimizeStaleInlineComments(context.Background(), fc, "acme", "repo", 1, "fullsend-bot", printer) + + // MinimizeComment errors should be logged but not stop processing. + assert.Contains(t, out.String(), "could not minimize inline comment") +} + +func TestMinimizeStaleInlineComments_NotSupportedIsSilent(t *testing.T) { + fc := forge.NewFakeClient() + fc.Errors["ListPullRequestReviewComments"] = forge.ErrNotSupported + + var out bytes.Buffer + printer := ui.New(&out) + minimizeStaleInlineComments(context.Background(), fc, "acme", "repo", 1, "fullsend-bot", printer) + + assert.Empty(t, fc.MinimizedComments) + assert.NotContains(t, out.String(), "Could not list inline comments", + "ErrNotSupported should be silently skipped") +} + +func TestSubmitFormalReview_MinimizesStaleInlineComments(t *testing.T) { + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "fullsend-bot" + fc.PRReviews = map[string][]forge.PullRequestReview{ + "acme/repo/1": { + {ID: 100, NodeID: "PRR_100", User: "fullsend-bot", State: "COMMENTED", Body: "old review"}, + }, + } + fc.PRReviewComments = map[string][]forge.PullRequestReviewComment{ + "acme/repo/1": { + {ID: 1, NodeID: "PRC_1", User: "fullsend-bot", Path: "a.go", Line: 10, Body: "old inline"}, + {ID: 2, NodeID: "PRC_2", User: "someone-else", Path: "a.go", Line: 12, Body: "human inline"}, + {ID: 3, NodeID: "PRC_3", User: "fullsend-bot", Path: "b.go", Line: 5, Body: "old inline 2"}, + }, + } + + printer := ui.New(io.Discard) + err := submitFormalReview(context.Background(), fc, "acme", "repo", 1, "approve", "", "", nil, false, printer) + require.NoError(t, err) + + // Should minimize: 1 stale review body + 2 stale inline comments = 3. + require.Len(t, fc.MinimizedComments, 3) + assert.Equal(t, "PRR_100", fc.MinimizedComments[0].NodeID, "review body minimized first") + assert.Equal(t, "PRC_1", fc.MinimizedComments[1].NodeID, "first inline comment minimized") + assert.Equal(t, "PRC_3", fc.MinimizedComments[2].NodeID, "second inline comment minimized") +} diff --git a/internal/forge/fake.go b/internal/forge/fake.go index 83aee49939..a77013e102 100644 --- a/internal/forge/fake.go +++ b/internal/forge/fake.go @@ -236,6 +236,9 @@ type FakeClient struct { // Pull request reviews for ListPullRequestReviews. PRReviews map[string][]PullRequestReview // key: "owner/repo/number" + // Pull request review comments for ListPullRequestReviewComments. + PRReviewComments map[string][]PullRequestReviewComment // key: "owner/repo/number" + // Annotations for GetWorkflowRunAnnotations. Annotations []Annotation @@ -1493,6 +1496,21 @@ func (f *FakeClient) ListPullRequestReviews(_ context.Context, owner, repo strin return nil, nil } +func (f *FakeClient) ListPullRequestReviewComments(_ context.Context, owner, repo string, number int) ([]PullRequestReviewComment, error) { + f.mu.Lock() + defer f.mu.Unlock() + if e := f.err("ListPullRequestReviewComments"); e != nil { + return nil, e + } + if f.PRReviewComments != nil { + key := fmt.Sprintf("%s/%s/%d", owner, repo, number) + if comments, ok := f.PRReviewComments[key]; ok { + return comments, nil + } + } + return nil, nil +} + func (f *FakeClient) DismissPullRequestReview(_ context.Context, owner, repo string, number, reviewID int, message string) error { f.mu.Lock() defer f.mu.Unlock() diff --git a/internal/forge/forge.go b/internal/forge/forge.go index 6eaf0193c9..003851aafb 100644 --- a/internal/forge/forge.go +++ b/internal/forge/forge.go @@ -214,6 +214,20 @@ type ReviewComment struct { Body string // comment body (Markdown) } +// PullRequestReviewComment represents an existing inline comment on a +// pull request diff, as returned by the "List review comments" API. +// This is distinct from ReviewComment (which is used when *creating* +// inline comments). PullRequestReviewComment carries metadata needed +// for lifecycle management (e.g., minimizing stale comments). +type PullRequestReviewComment struct { + ID int + NodeID string + User string // author login + Path string + Line int + Body string +} + // PullRequestFileDiff represents a file changed in a pull request along // with its unified diff patch. The patch may be empty for binary files, // rename-only changes, or when GitHub truncates large diffs. @@ -524,6 +538,11 @@ type Client interface { // comments, when non-nil, attaches inline diff comments to the review. CreatePullRequestReview(ctx context.Context, owner, repo string, number int, event, body, commitSHA string, comments []ReviewComment) error ListPullRequestReviews(ctx context.Context, owner, repo string, number int) ([]PullRequestReview, error) + // ListPullRequestReviewComments returns all inline review comments + // on a pull request (comments attached to specific lines in the diff). + // Used to identify stale inline comments from prior review runs that + // should be minimized before posting new ones. + ListPullRequestReviewComments(ctx context.Context, owner, repo string, number int) ([]PullRequestReviewComment, error) DismissPullRequestReview(ctx context.Context, owner, repo string, number, reviewID int, message string) error // Change proposal merge diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index d59c97022e..8a8c6d91b0 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -2908,6 +2908,49 @@ func (c *LiveClient) ListPullRequestReviews(ctx context.Context, owner, repo str return result, nil } +// ListPullRequestReviewComments returns all inline review comments on a +// pull request, paginating automatically. These are the comments attached +// to specific lines in the diff (not issue comments or review bodies). +func (c *LiveClient) ListPullRequestReviewComments(ctx context.Context, owner, repo string, number int) ([]forge.PullRequestReviewComment, error) { + var result []forge.PullRequestReviewComment + + for page := 1; page <= 100; page++ { + resp, err := c.get(ctx, fmt.Sprintf("/repos/%s/%s/pulls/%d/comments?per_page=100&page=%d", owner, repo, number, page)) + if err != nil { + return nil, fmt.Errorf("list pull request review comments page %d: %w", page, err) + } + var raw []struct { + ID int `json:"id"` + NodeID string `json:"node_id"` + User struct { + Login string `json:"login"` + } `json:"user"` + Path string `json:"path"` + Line int `json:"line"` + Body string `json:"body"` + } + if err := decodeJSON(resp, &raw); err != nil { + return nil, fmt.Errorf("decoding pull request review comments page %d: %w", page, err) + } + + for _, r := range raw { + result = append(result, forge.PullRequestReviewComment{ + ID: r.ID, + NodeID: r.NodeID, + User: r.User.Login, + Path: r.Path, + Line: r.Line, + Body: r.Body, + }) + } + + if len(raw) < 100 { + break + } + } + return result, nil +} + // DismissPullRequestReview dismisses a review, changing its state to DISMISSED. func (c *LiveClient) DismissPullRequestReview(ctx context.Context, owner, repo string, number, reviewID int, message string) error { payload := map[string]string{ diff --git a/internal/forge/gitlab/mr.go b/internal/forge/gitlab/mr.go index 8842d301c3..e46f92602c 100644 --- a/internal/forge/gitlab/mr.go +++ b/internal/forge/gitlab/mr.go @@ -471,6 +471,13 @@ func (c *LiveClient) ListPullRequestReviews(ctx context.Context, owner, repo str return result, nil } +// ListPullRequestReviewComments is not supported on GitLab — there is no +// equivalent of GitHub's pull request review comment minimization. Inline +// comments on GitLab MRs are plain notes without a minimize/hide feature. +func (c *LiveClient) ListPullRequestReviewComments(_ context.Context, _, _ string, _ int) ([]forge.PullRequestReviewComment, error) { + return nil, forge.ErrNotSupported +} + // DismissPullRequestReview dismisses a review on a merge request. // // On GitLab, if the review was an approval, this unapproves the MR. From cad479949fd47aa73dfca6b6265f5a563ef28872 Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Sun, 2 Aug 2026 06:52:43 +0000 Subject: [PATCH 2/2] fix(review): decouple inline comment cleanup from review listing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move minimizeStaleInlineComments outside the ListPullRequestReviews- dependent else block so it runs independently. The function makes its own ListPullRequestReviewComments call and does not need the reviews variable — a transient ListPullRequestReviews failure should not prevent inline comment cleanup. Addresses review feedback on #820 --- internal/cli/postreview.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/internal/cli/postreview.go b/internal/cli/postreview.go index bbd1f11c42..a4adb9b5b3 100644 --- a/internal/cli/postreview.go +++ b/internal/cli/postreview.go @@ -312,11 +312,13 @@ func submitFormalReview(ctx context.Context, client forge.Client, owner, repo st user, err := client.GetAuthenticatedUser(ctx) if err != nil { printer.StepInfo("Could not determine authenticated user, skipping stale review cleanup") - } else if reviews, err := client.ListPullRequestReviews(ctx, owner, repo, pr); err != nil { - printer.StepInfo("Could not list reviews, skipping stale review cleanup") } else { - dismissStaleRequestChanges(ctx, client, owner, repo, pr, event, user, reviews, printer) - minimizeStaleReviews(ctx, client, user, reviews, printer) + if reviews, err := client.ListPullRequestReviews(ctx, owner, repo, pr); err != nil { + printer.StepInfo("Could not list reviews, skipping stale review cleanup") + } else { + dismissStaleRequestChanges(ctx, client, owner, repo, pr, event, user, reviews, printer) + minimizeStaleReviews(ctx, client, user, reviews, printer) + } minimizeStaleInlineComments(ctx, client, owner, repo, pr, user, printer) }