From 8a31cf242cb1220f2064e67769a487356a27b4f8 Mon Sep 17 00:00:00 2001 From: Bruce Arctor <5032356+brucearctor@users.noreply.github.com> Date: Sun, 26 Jul 2026 16:25:08 -0700 Subject: [PATCH 1/2] feat: platform-specific code suggestion rendering (#42) --- .code-reviewer.example.yaml | 4 +++- internal/config/config_test.go | 20 ++++++++++++++++++++ internal/github/client.go | 6 +++++- internal/gitlab/client.go | 20 ++++++++++++++------ internal/reviewer/output.go | 10 ++++------ internal/reviewer/output_test.go | 16 +--------------- internal/vcs/types.go | 7 ++++--- 7 files changed, 51 insertions(+), 32 deletions(-) diff --git a/.code-reviewer.example.yaml b/.code-reviewer.example.yaml index 72e9ca2..d45ea3c 100644 --- a/.code-reviewer.example.yaml +++ b/.code-reviewer.example.yaml @@ -21,10 +21,12 @@ focus: [bugs, security] min_severity: low # How to post comments to GitLab MRs. -# notes: simple MR note (works with CI_JOB_TOKEN) +# comment_mode: notes # discussions: inline diff-anchored comments (needs PAT with api scope) comment_mode: notes +# cleanup_mode: delete # How to handle previous bot comments (delete or resolve) + # How to handle diffs that exceed the model's context window. # fail: error out with a helpful message (default, forces smaller MRs) # split: auto-split into chunks and merge results diff --git a/internal/config/config_test.go b/internal/config/config_test.go index d619d64..83559c4 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -704,6 +704,26 @@ func TestLoad_FlagAndEnvOptions(t *testing.T) { } }, }, + { + name: "cleanup_mode_flag", + args: []string{"code-reviewer", "--diff", "--cleanup-mode", "resolve"}, + env: map[string]string{"GOOGLE_CLOUD_PROJECT": "test-project"}, + assert: func(t *testing.T, cfg *Config) { + if cfg.CleanupMode != CleanupModeResolve { + t.Errorf("CleanupMode = %q, want %q", cfg.CleanupMode, CleanupModeResolve) + } + }, + }, + { + name: "cleanup_mode_env", + args: []string{"code-reviewer", "--diff"}, + env: map[string]string{"GOOGLE_CLOUD_PROJECT": "test-project", "CODE_REVIEWER_CLEANUP_MODE": "resolve"}, + assert: func(t *testing.T, cfg *Config) { + if cfg.CleanupMode != CleanupModeResolve { + t.Errorf("CleanupMode = %q, want %q", cfg.CleanupMode, CleanupModeResolve) + } + }, + }, } for _, tt := range tests { diff --git a/internal/github/client.go b/internal/github/client.go index 21cced4..35cd49a 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -240,10 +240,14 @@ func (c *Client) SubmitReview(ctx context.Context, projectID, prNumber string, r dropped++ continue } + commentBody := comment.Body + if comment.Suggestion != "" { + commentBody += fmt.Sprintf("\n\n```suggestion\n%s\n```", comment.Suggestion) + } validComments = append(validComments, ReviewCommentRequest{ Path: comment.Path, Line: comment.Line, - Body: comment.Body, + Body: commentBody, Side: "RIGHT", }) } diff --git a/internal/gitlab/client.go b/internal/gitlab/client.go index e302562..c575e90 100644 --- a/internal/gitlab/client.go +++ b/internal/gitlab/client.go @@ -327,8 +327,12 @@ func (c *Client) submitViaDraftNotes(ctx context.Context, projectID, mrIID strin } newLine := comment.Line + noteBody := comment.Body + if comment.Suggestion != "" { + noteBody += fmt.Sprintf("\n\n```suggestion:-0+0\n%s\n```", comment.Suggestion) + } draftReq := CreateDraftNoteRequest{ - Note: comment.Body, + Note: noteBody, Position: &DiscussionPosition{ PositionType: "text", BaseSHA: req.Version.BaseSHA, @@ -346,8 +350,8 @@ func (c *Client) submitViaDraftNotes(ctx context.Context, projectID, mrIID strin "line", comment.Line, "error", err, ) - noteBody := fmt.Sprintf("**%s:%d** — %s", comment.Path, comment.Line, comment.Body) - if _, noteErr := c.PostNote(ctx, projectID, mrIID, noteBody); noteErr != nil { + noteBodyStr := fmt.Sprintf("**%s:%d** — %s", comment.Path, comment.Line, noteBody) + if _, noteErr := c.PostNote(ctx, projectID, mrIID, noteBodyStr); noteErr != nil { slog.Error("note fallback also failed", "error", noteErr) } draftsFailed++ @@ -388,8 +392,12 @@ func (c *Client) submitViaIndividualComments(ctx context.Context, projectID, mrI break } newLine := comment.Line + noteBody := comment.Body + if comment.Suggestion != "" { + noteBody += fmt.Sprintf("\n\n```suggestion:-0+0\n%s\n```", comment.Suggestion) + } inlineReq := vcs.InlineCommentRequest{ - Body: comment.Body, + Body: noteBody, Position: &vcs.InlineCommentPosition{ BaseSHA: req.Version.BaseSHA, HeadSHA: req.Version.HeadSHA, @@ -407,8 +415,8 @@ func (c *Client) submitViaIndividualComments(ctx context.Context, projectID, mrI "error", err, ) // Fallback: post as a regular note. - noteBody := fmt.Sprintf("**%s:%d** — %s", comment.Path, comment.Line, comment.Body) - if _, err := c.PostNote(ctx, projectID, mrIID, noteBody); err != nil { + noteBodyStr := fmt.Sprintf("**%s:%d** — %s", comment.Path, comment.Line, noteBody) + if _, err := c.PostNote(ctx, projectID, mrIID, noteBodyStr); err != nil { slog.Error("failed to post fallback note", "error", err) } else { fallbackPosted++ diff --git a/internal/reviewer/output.go b/internal/reviewer/output.go index a21612b..81bb38d 100644 --- a/internal/reviewer/output.go +++ b/internal/reviewer/output.go @@ -68,9 +68,10 @@ func PostReview(ctx context.Context, cfg *config.Config, client VCSClient, resul if cfg.CommentMode == config.CommentModeDiscussions && version != nil { for _, f := range result.Findings { req.Comments = append(req.Comments, vcs.ReviewComment{ - Path: f.File, - Line: f.Line, - Body: formatInlineComment(f), + Path: f.File, + Line: f.Line, + Body: formatInlineComment(f), + Suggestion: f.Suggestion, }) } } @@ -117,9 +118,6 @@ func formatInlineComment(f model.Finding) string { var sb strings.Builder fmt.Fprintf(&sb, "%s **[%s]** %s\n\n", severityEmoji(f.Severity), f.Severity, f.Title) sb.WriteString(f.Body) - if f.Suggestion != "" { - fmt.Fprintf(&sb, "\n\n```suggestion\n%s\n```", f.Suggestion) - } return sb.String() } diff --git a/internal/reviewer/output_test.go b/internal/reviewer/output_test.go index b494dc9..753663b 100644 --- a/internal/reviewer/output_test.go +++ b/internal/reviewer/output_test.go @@ -202,21 +202,7 @@ func TestFormatInlineComment_WithoutSuggestion(t *testing.T) { } } -func TestFormatInlineComment_WithSuggestion(t *testing.T) { - f := model.Finding{ - Severity: "CRITICAL", - Title: "SQL injection", - Body: "Raw concat.", - Suggestion: "db.Query(\"SELECT * FROM t WHERE id = ?\", id)", - } - out := formatInlineComment(f) - if !strings.Contains(out, "```suggestion") { - t.Error("expected suggestion code block") - } - if !strings.Contains(out, "db.Query") { - t.Error("expected suggestion content") - } -} + func TestTokenUsageRendering(t *testing.T) { finding := model.Finding{File: "a.go", Line: 1, Severity: "LOW", Category: "style", Title: "test", Body: "body"} diff --git a/internal/vcs/types.go b/internal/vcs/types.go index b7d30c4..5dec43d 100644 --- a/internal/vcs/types.go +++ b/internal/vcs/types.go @@ -68,9 +68,10 @@ type InlineCommentRequest struct { // submission. Unlike InlineCommentRequest, it carries only the essential // positioning info — the platform client handles SHA context internally. type ReviewComment struct { - Path string // File path relative to repo root. - Line int // Line number in the new file. - Body string // Pre-formatted markdown body. + Path string // File path relative to repo root. + Line int // Line number in the new file. + Body string // Pre-formatted markdown body. + Suggestion string // Raw replacement code (empty if no suggestion). } // SubmitReviewRequest is the payload for submitting a complete code review From 6da013f3ad14accc130bb761a1c158260bac131f Mon Sep 17 00:00:00 2001 From: Bruce Arctor <5032356+brucearctor@users.noreply.github.com> Date: Sun, 26 Jul 2026 16:44:36 -0700 Subject: [PATCH 2/2] fix: address CodeRabbit review on PR #50 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Make rate-limit wait cancellable in ResolvePreviousReviews — use select on ctx.Done() instead of time.Sleep 2. Tag draft inline notes with botMarker — ensures resolve mode can identify bot discussions created via the draft notes path 3. Add submission-contract test verifying Suggestion and CleanupMode survive PostReview passthrough (table-driven) 4. Add config tests: invalid cleanup-mode validation, flag-over-env precedence --- internal/config/config_test.go | 26 +++++++++++++++++ internal/gitlab/client.go | 8 ++++-- internal/reviewer/output_test.go | 48 ++++++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 2 deletions(-) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 83559c4..307a1b7 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -724,6 +724,16 @@ func TestLoad_FlagAndEnvOptions(t *testing.T) { } }, }, + { + name: "cleanup_mode_flag_over_env", + args: []string{"code-reviewer", "--diff", "--cleanup-mode", "delete"}, + env: map[string]string{"GOOGLE_CLOUD_PROJECT": "test-project", "CODE_REVIEWER_CLEANUP_MODE": "resolve"}, + assert: func(t *testing.T, cfg *Config) { + if cfg.CleanupMode != CleanupModeDelete { + t.Errorf("CleanupMode = %q, want %q (flag should override env)", cfg.CleanupMode, CleanupModeDelete) + } + }, + }, } for _, tt := range tests { @@ -919,3 +929,19 @@ func TestIntentReview_MutuallyExclusiveWithSummarize(t *testing.T) { t.Errorf("error = %q, want mention of 'mutually exclusive'", err.Error()) } } + +func TestLoad_InvalidCleanupMode(t *testing.T) { + oldArgs := os.Args + defer func() { os.Args = oldArgs }() + os.Args = []string{"code-reviewer", "--diff", "--cleanup-mode", "archive"} + + t.Setenv("GOOGLE_CLOUD_PROJECT", "test-project") + + _, err := Load() + if err == nil { + t.Fatal("expected error for invalid cleanup-mode") + } + if !containsStr(err.Error(), "invalid cleanup-mode") { + t.Errorf("error = %q, want mention of 'invalid cleanup-mode'", err.Error()) + } +} diff --git a/internal/gitlab/client.go b/internal/gitlab/client.go index c575e90..a32ca38 100644 --- a/internal/gitlab/client.go +++ b/internal/gitlab/client.go @@ -251,7 +251,11 @@ func (c *Client) ResolvePreviousReviews(ctx context.Context, projectID, mrIID st continue } resolved++ - time.Sleep(apiRateDelay) + select { + case <-time.After(apiRateDelay): + case <-ctx.Done(): + return resolved, ctx.Err() + } } } return resolved, nil @@ -332,7 +336,7 @@ func (c *Client) submitViaDraftNotes(ctx context.Context, projectID, mrIID strin noteBody += fmt.Sprintf("\n\n```suggestion:-0+0\n%s\n```", comment.Suggestion) } draftReq := CreateDraftNoteRequest{ - Note: noteBody, + Note: noteBody + "\n" + botMarker, Position: &DiscussionPosition{ PositionType: "text", BaseSHA: req.Version.BaseSHA, diff --git a/internal/reviewer/output_test.go b/internal/reviewer/output_test.go index 753663b..ec79474 100644 --- a/internal/reviewer/output_test.go +++ b/internal/reviewer/output_test.go @@ -201,7 +201,55 @@ func TestFormatInlineComment_WithoutSuggestion(t *testing.T) { t.Error("should not have suggestion block when suggestion is empty") } } +func TestPostReview_PassesSuggestionAndCleanupMode(t *testing.T) { + tests := []struct { + name string + suggestion string + cleanupMode config.CleanupMode + }{ + {"with_suggestion_and_delete", "fixed := sanitize(input)", config.CleanupModeDelete}, + {"with_suggestion_and_resolve", "return fmt.Errorf(\"wrap: %w\", err)", config.CleanupModeResolve}, + {"no_suggestion", "", config.CleanupModeDelete}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockClient := &outputMockVCS{} + cfg := &config.Config{ + CIMode: true, + CIProjectID: "proj", + CIMergeRequestID: "1", + CommentMode: config.CommentModeDiscussions, + CleanupMode: tt.cleanupMode, + } + result := &model.ReviewResult{ + Summary: "Review", + Findings: []model.Finding{ + {File: "a.go", Line: 5, Severity: "HIGH", Category: "bug", Title: "issue", Body: "desc", Suggestion: tt.suggestion}, + }, + } + version := &vcs.DiffVersion{HeadSHA: "h", BaseSHA: "b", StartSHA: "s"} + + if err := PostReview(context.Background(), cfg, mockClient, result, version); err != nil { + t.Fatalf("unexpected error: %v", err) + } + req := mockClient.submitReviewReq + if req == nil { + t.Fatal("expected SubmitReview to be called") + } + if req.CleanupMode != string(tt.cleanupMode) { + t.Errorf("CleanupMode = %q, want %q", req.CleanupMode, tt.cleanupMode) + } + if len(req.Comments) != 1 { + t.Fatalf("expected 1 comment, got %d", len(req.Comments)) + } + if req.Comments[0].Suggestion != tt.suggestion { + t.Errorf("Suggestion = %q, want %q", req.Comments[0].Suggestion, tt.suggestion) + } + }) + } +} func TestTokenUsageRendering(t *testing.T) {