From db37865ba280418d8e93df8556300c0e5f395f50 Mon Sep 17 00:00:00 2001 From: Bruce Arctor <5032356+brucearctor@users.noreply.github.com> Date: Sun, 26 Jul 2026 16:27:39 -0700 Subject: [PATCH] feat: opt-in resolve mode for previous review cleanup (#43) --- internal/config/config.go | 26 +++++++++++ internal/github/client.go | 3 ++ internal/gitlab/client.go | 89 ++++++++++++++++++++++++++++++++++--- internal/reviewer/output.go | 5 ++- internal/vcs/types.go | 7 +-- 5 files changed, 120 insertions(+), 10 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index b16aeff..2e10ed9 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -64,6 +64,14 @@ const ( CommentModeDiscussions CommentMode = "discussions" ) +// CleanupMode determines how previous bot comments are handled. +type CleanupMode string + +const ( + CleanupModeDelete CleanupMode = "delete" // Delete old comments (default). + CleanupModeResolve CleanupMode = "resolve" // Resolve old discussions (GitLab only; GitHub falls back to delete). +) + // ChunkStrategy determines how large diffs are handled. type ChunkStrategy string @@ -100,6 +108,7 @@ type Config struct { // Output settings. CommentMode CommentMode + CleanupMode CleanupMode DryRun bool OutputJSON bool NoColor bool // Disable ANSI color output. @@ -154,6 +163,7 @@ type repoConfig struct { Focus []string `yaml:"focus"` MinSeverity string `yaml:"min_severity"` CommentMode string `yaml:"comment_mode"` + CleanupMode string `yaml:"cleanup_mode"` ChunkStrategy string `yaml:"chunk_strategy"` ExcludedPatterns []string `yaml:"excluded_patterns"` ExtraRules string `yaml:"extra_rules"` @@ -186,6 +196,7 @@ func Load() (*Config, error) { Focus: []string{"all"}, MinSeverity: SeverityLow, CommentMode: CommentModeNotes, + CleanupMode: CleanupModeDelete, ChunkStrategy: ChunkStrategyFail, GitLabBaseURL: "https://gitlab.com", GitHubBaseURL: "https://api.github.com", @@ -308,6 +319,9 @@ func (c *Config) applyRepoConfig(data []byte) error { if rc.CommentMode != "" { c.CommentMode = CommentMode(rc.CommentMode) } + if rc.CleanupMode != "" { + c.CleanupMode = CleanupMode(rc.CleanupMode) + } if rc.ChunkStrategy != "" { c.ChunkStrategy = ChunkStrategy(rc.ChunkStrategy) } @@ -362,6 +376,9 @@ func (c *Config) loadEnv() { if v := os.Getenv("REVIEW_COMMENT_MODE"); v != "" { c.CommentMode = CommentMode(v) } + if v := os.Getenv("CODE_REVIEWER_CLEANUP_MODE"); v != "" { + c.CleanupMode = CleanupMode(v) + } if v := os.Getenv("REVIEW_CHUNK_STRATEGY"); v != "" { c.ChunkStrategy = ChunkStrategy(v) } @@ -444,6 +461,7 @@ func (c *Config) loadFlags() error { focus := fs.String("focus", "", "Review focus areas, comma-separated (bugs,security,performance,style,docs,all)") minSev := fs.String("min-severity", "", "Minimum severity to report (low, medium, high, critical)") commentMode := fs.String("comment-mode", "", "GitLab comment mode: notes (simple) or discussions (inline)") + cleanupMode := fs.String("cleanup-mode", "", "How to handle previous bot comments (delete or resolve)") chunkStrategy := fs.String("chunk-strategy", "", "How to handle large diffs: fail (default) or split") extraRules := fs.String("extra-rules", "", "Additional review rules appended to prompt") dryRun := fs.Bool("dry-run", false, "Run analysis but don't post to GitLab") @@ -501,6 +519,9 @@ func (c *Config) loadFlags() error { if *commentMode != "" { c.CommentMode = CommentMode(*commentMode) } + if *cleanupMode != "" { + c.CleanupMode = CleanupMode(*cleanupMode) + } if *chunkStrategy != "" { c.ChunkStrategy = ChunkStrategy(*chunkStrategy) } @@ -687,6 +708,11 @@ func (c *Config) validate() error { return fmt.Errorf("invalid comment-mode: %q (valid: notes, discussions)", c.CommentMode) } + // Validate cleanup mode. + if c.CleanupMode != CleanupModeDelete && c.CleanupMode != CleanupModeResolve { + return fmt.Errorf("invalid cleanup-mode: %q (valid: delete, resolve)", c.CleanupMode) + } + // Validate chunk strategy. if c.ChunkStrategy != ChunkStrategyFail && c.ChunkStrategy != ChunkStrategySplit { return fmt.Errorf("invalid chunk-strategy: %q (valid: fail, split)", c.ChunkStrategy) diff --git a/internal/github/client.go b/internal/github/client.go index 99c49da..21cced4 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -218,6 +218,9 @@ func (c *Client) CleanPreviousReviews(ctx context.Context, projectID, prNumber s // This ensures exactly one notification to the PR author in all cases. func (c *Client) SubmitReview(ctx context.Context, projectID, prNumber string, req vcs.SubmitReviewRequest) error { // 1. Clean previous bot comments. + if req.CleanupMode == "resolve" { + slog.Warn("GitHub does not support resolving previous reviews; falling back to delete mode") + } deleted, err := c.CleanPreviousReviews(ctx, projectID, prNumber) if err != nil { slog.Warn("failed to clean previous reviews", "error", err) diff --git a/internal/gitlab/client.go b/internal/gitlab/client.go index c9c31b4..e302562 100644 --- a/internal/gitlab/client.go +++ b/internal/gitlab/client.go @@ -173,6 +173,35 @@ func (c *Client) ListBotNotes(ctx context.Context, projectID, mrIID string) ([]v return botNotes, nil } +type Discussion struct { + ID string `json:"id"` + Notes []Note `json:"notes"` +} + +// ListDiscussions returns all discussions on a merge request. +func (c *Client) ListDiscussions(ctx context.Context, projectID, mrIID string) ([]Discussion, error) { + url := fmt.Sprintf("%s/projects/%s/merge_requests/%s/discussions", c.baseURL, url.PathEscape(projectID), mrIID) + var discussions []Discussion + err := c.getPaginated(ctx, url, func(page json.RawMessage) error { + var pageDiscussions []Discussion + if err := json.Unmarshal(page, &pageDiscussions); err != nil { + return err + } + discussions = append(discussions, pageDiscussions...) + return nil + }) + return discussions, err +} + +// ResolveDiscussion marks a discussion as resolved on a merge request. +func (c *Client) ResolveDiscussion(ctx context.Context, projectID, mrIID string, discussionID string) error { + apiURL := fmt.Sprintf("%s/projects/%s/merge_requests/%s/discussions/%s", c.baseURL, url.PathEscape(projectID), mrIID, discussionID) + type resolveReq struct { + Resolved bool `json:"resolved"` + } + return c.put(ctx, apiURL, resolveReq{Resolved: true}, nil) +} + // DeleteNote removes a note from a merge request. func (c *Client) DeleteNote(ctx context.Context, projectID, mrIID string, noteID int) error { url := fmt.Sprintf("%s/projects/%s/merge_requests/%s/notes/%d", c.baseURL, url.PathEscape(projectID), mrIID, noteID) @@ -201,16 +230,52 @@ func (c *Client) CleanPreviousReviews(ctx context.Context, projectID, mrIID stri return deleted, nil } +// ResolvePreviousReviews resolves all bot-tagged discussions on an MR. +func (c *Client) ResolvePreviousReviews(ctx context.Context, projectID, mrIID string) (int, error) { + discussions, err := c.ListDiscussions(ctx, projectID, mrIID) + if err != nil { + return 0, err + } + + resolved := 0 + for _, d := range discussions { + isBotDiscussion := false + for _, n := range d.Notes { + if strings.Contains(n.Body, botMarker) { + isBotDiscussion = true + break + } + } + if isBotDiscussion { + if err := c.ResolveDiscussion(ctx, projectID, mrIID, d.ID); err != nil { + continue + } + resolved++ + time.Sleep(apiRateDelay) + } + } + return resolved, nil +} + // SubmitReview posts a complete review to the merge request using draft notes // for a single-notification experience. Falls back to individual discussions // if the Draft Notes API is unavailable (GitLab < 15.11 or CE without the feature). func (c *Client) SubmitReview(ctx context.Context, projectID, mrIID string, req vcs.SubmitReviewRequest) error { // 1. Clean previous bot comments. - deleted, err := c.CleanPreviousReviews(ctx, projectID, mrIID) - if err != nil { - slog.Warn("failed to clean previous reviews", "error", err) - } else if deleted > 0 { - slog.Info(fmt.Sprintf("cleaned %d previous bot comment(s)", deleted)) + if req.CleanupMode == "resolve" { + resolved, err := c.ResolvePreviousReviews(ctx, projectID, mrIID) + if err != nil { + slog.Warn("failed to resolve previous reviews", "error", err) + } else if resolved > 0 { + slog.Info(fmt.Sprintf("resolved %d previous bot discussion(s)", resolved)) + } + } else { + deleted, err := c.CleanPreviousReviews(ctx, projectID, mrIID) + if err != nil { + slog.Warn("failed to clean previous reviews", "error", err) + } else if deleted > 0 { + slog.Info(fmt.Sprintf("cleaned %d previous bot comment(s)", deleted)) + } } // 2. Try draft notes path (single notification). @@ -607,6 +672,20 @@ func (c *Client) delete(ctx context.Context, url string) error { return c.do(req, nil) } +func (c *Client) put(ctx context.Context, url string, body interface{}, out interface{}) error { + data, err := json.Marshal(body) + if err != nil { + return fmt.Errorf("marshaling request: %w", err) + } + + req, err := http.NewRequestWithContext(ctx, http.MethodPut, url, bytes.NewReader(data)) + if err != nil { + return err + } + req.Header.Set("Content-Type", "application/json") + return c.do(req, out) +} + func (c *Client) do(req *http.Request, out interface{}) error { req.Header.Set("PRIVATE-TOKEN", c.token) diff --git a/internal/reviewer/output.go b/internal/reviewer/output.go index 1949706..a21612b 100644 --- a/internal/reviewer/output.go +++ b/internal/reviewer/output.go @@ -60,8 +60,9 @@ func TerminalOutput(result *model.ReviewResult) string { // PostReview posts review results to a GitLab merge request. func PostReview(ctx context.Context, cfg *config.Config, client VCSClient, result *model.ReviewResult, version *vcs.DiffVersion) error { req := vcs.SubmitReviewRequest{ - Summary: formatSummaryNote(result), - Version: version, + Summary: formatSummaryNote(result), + Version: version, + CleanupMode: string(cfg.CleanupMode), } if cfg.CommentMode == config.CommentModeDiscussions && version != nil { diff --git a/internal/vcs/types.go b/internal/vcs/types.go index 81adc13..b7d30c4 100644 --- a/internal/vcs/types.go +++ b/internal/vcs/types.go @@ -77,7 +77,8 @@ type ReviewComment struct { // as a single atomic operation. On GitHub this maps to a single API call // (POST /pulls/{pr}/reviews). On GitLab it maps to PostNote + N×CreateDiscussion. type SubmitReviewRequest struct { - Summary string // Top-level review body (markdown). - Comments []ReviewComment // Inline comments anchored to diff positions. - Version *DiffVersion // SHA context for inline comment positioning (may be nil). + Summary string // Top-level review body (markdown). + Comments []ReviewComment // Inline comments anchored to diff positions. + Version *DiffVersion // SHA context for inline comment positioning (may be nil). + CleanupMode string // "delete" or "resolve" — controls how old bot comments are handled. }