diff --git a/internal/cli/postreview.go b/internal/cli/postreview.go index 13d0f58c01..a4adb9b5b3 100644 --- a/internal/cli/postreview.go +++ b/internal/cli/postreview.go @@ -312,11 +312,14 @@ 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) } var diffHunks map[string][][2]int @@ -618,6 +621,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.