feat: SARIF 2.1.0 output format - #12
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds incremental MR review support and SARIF 2.1.0 output. Config gains ChangesIncremental review and SARIF output
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReviewerRun
participant GitLabClient
participant FilterByFiles
participant WriteSARIF
ReviewerRun->>GitLabClient: fetch MR versions
GitLabClient-->>ReviewerRun: previous/current head SHA
ReviewerRun->>GitLabClient: CompareCommits(previous, current)
GitLabClient-->>ReviewerRun: changed file paths
ReviewerRun->>FilterByFiles: filterByFiles(diffs, changedFiles)
FilterByFiles-->>ReviewerRun: filtered diffs
ReviewerRun->>WriteSARIF: WriteSARIF(SARIFOutput, reviewResult)
WriteSARIF-->>ReviewerRun: write status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/reviewer/reviewer.go (1)
175-211: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSARIF output is skipped on GitLab post failure, and its error path loses the findings count.
Two issues in this block:
- If
PostToGitLabfails, the function returns early at line 200, so the SARIF block (204-211) never runs — the CI security-tab artifact is lost even though the review itself succeeded. Given SARIF output is meant to feed CI code-scanning independently of comment posting, consider writing SARIF before/independently of the GitLab-post step (or on a best-effort basis even if posting fails).- When
WriteSARIFfails, the function returns0for the finding count (line 207) instead oflen(allFindings), unlike the sibling error path at line 200 (return len(allFindings), fmt.Errorf(...)). Callers relying on the returned count (e.g., for exit-code gating) would incorrectly see zero findings despiteerrbeing non-nil.🐛 Proposed fix
// Write SARIF if requested. if r.cfg.SARIFOutput != "" { if err := WriteSARIF(r.cfg.SARIFOutput, result); err != nil { - return 0, fmt.Errorf("writing SARIF: %w", err) + return len(allFindings), fmt.Errorf("writing SARIF: %w", err) } slog.Info("SARIF output written", "path", r.cfg.SARIFOutput) }Consider moving the SARIF-write block ahead of the GitLab-posting step (or wrapping the post in a way that doesn't early-return before SARIF is written) so CI code-scanning output isn't dependent on comment-posting success.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/reviewer/reviewer.go` around lines 175 - 211, SARIF generation is currently blocked by an early return from the GitLab posting path, and the SARIF error path drops the findings count. Update the output flow in reviewer.go’s post-processing so WriteSARIF runs independently of PostToGitLab (or before it) and is still attempted even if posting fails, and change the WriteSARIF failure return to preserve len(allFindings) just like the PostToGitLab error path. Use the existing PostToGitLab and WriteSARIF branches in the Step 7 output section as the place to adjust the control flow.
🧹 Nitpick comments (2)
internal/reviewer/reviewer.go (1)
189-197: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant
GetMRVersionscall.
GetMRVersionsis already fetched at line 80 for incremental filtering; it's fetched again here for discussion-mode comment placement. Cache and reuse the first result (when available) to avoid a duplicate GitLab API round-trip on every CI run.♻️ Proposed fix: reuse versions fetched during incremental filtering
- var diffs []diff.FileDiff + var diffs []diff.FileDiff + var mrVersions []gitlab.DiffVersion ... if r.cfg.Incremental && r.cfg.CIMode && r.glClient != nil { versions, verr := r.glClient.GetMRVersions(ctx, r.cfg.CIProjectID, r.cfg.CIMergeRequestID) + mrVersions = versions ... } ... var version *gitlab.DiffVersion if r.cfg.CommentMode == config.CommentModeDiscussions { - versions, err := r.glClient.GetMRVersions(ctx, r.cfg.CIProjectID, r.cfg.CIMergeRequestID) - if err != nil { - slog.Warn("could not fetch MR versions, inline comments may fail", "error", err) - } else if len(versions) > 0 { - version = &versions[0] - } + if mrVersions == nil { + var err error + mrVersions, err = r.glClient.GetMRVersions(ctx, r.cfg.CIProjectID, r.cfg.CIMergeRequestID) + if err != nil { + slog.Warn("could not fetch MR versions, inline comments may fail", "error", err) + } + } + if len(mrVersions) > 0 { + version = &mrVersions[0] + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/reviewer/reviewer.go` around lines 189 - 197, The review placement logic in reviewer.go is making a redundant GetMRVersions call in the comment-mode discussions branch. Reuse the versions already fetched earlier for incremental filtering in reviewer logic instead of calling r.glClient.GetMRVersions again, and pass the cached result into the version selection used for inline/discussion comments. Keep the existing fallback behavior when no versions are available, but avoid the extra GitLab API round-trip.internal/config/config.go (1)
109-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove or reword the unused CI fields
internal/config/config.go:109-112
CIDiffBaseSHAandCICommitBeforeSHAare only populated from environment variables and never consumed elsewhere; the “reserved for future incremental review” note is stale now that incremental review uses MR versions plusCompareCommits. Either delete the fields or update the comment to match the current behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/config.go` around lines 109 - 112, The CI config struct includes stale unused fields, so either remove CIDiffBaseSHA and CICommitBeforeSHA from the config definition or update their documentation to reflect how incremental review actually works now. Check the config struct in internal/config/config.go and any references to these symbols to confirm they are not consumed elsewhere, then keep only the fields and comments that match the current CompareCommits-based behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/reviewer/reviewer.go`:
- Around line 78-102: The incremental review path in reviewer.go now calls
CompareCommits on VCSClient, but the interface and test doubles are missing that
method. Add CompareCommits to VCSClient in interfaces.go, implement it on the
GitLab client, and update the reviewer_test.go and output_test.go mocks so they
satisfy the expanded interface and the package builds cleanly.
---
Outside diff comments:
In `@internal/reviewer/reviewer.go`:
- Around line 175-211: SARIF generation is currently blocked by an early return
from the GitLab posting path, and the SARIF error path drops the findings count.
Update the output flow in reviewer.go’s post-processing so WriteSARIF runs
independently of PostToGitLab (or before it) and is still attempted even if
posting fails, and change the WriteSARIF failure return to preserve
len(allFindings) just like the PostToGitLab error path. Use the existing
PostToGitLab and WriteSARIF branches in the Step 7 output section as the place
to adjust the control flow.
---
Nitpick comments:
In `@internal/config/config.go`:
- Around line 109-112: The CI config struct includes stale unused fields, so
either remove CIDiffBaseSHA and CICommitBeforeSHA from the config definition or
update their documentation to reflect how incremental review actually works now.
Check the config struct in internal/config/config.go and any references to these
symbols to confirm they are not consumed elsewhere, then keep only the fields
and comments that match the current CompareCommits-based behavior.
In `@internal/reviewer/reviewer.go`:
- Around line 189-197: The review placement logic in reviewer.go is making a
redundant GetMRVersions call in the comment-mode discussions branch. Reuse the
versions already fetched earlier for incremental filtering in reviewer logic
instead of calling r.glClient.GetMRVersions again, and pass the cached result
into the version selection used for inline/discussion comments. Keep the
existing fallback behavior when no versions are available, but avoid the extra
GitLab API round-trip.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1db3e234-7702-44c8-924b-5052c0abf0e7
📒 Files selected for processing (4)
internal/config/config.gointernal/reviewer/reviewer.gointernal/reviewer/sarif.gointernal/reviewer/sarif_test.go
| // Step 2b: Incremental review — filter to only files changed in latest push. | ||
| if r.cfg.Incremental && r.cfg.CIMode && r.glClient != nil { | ||
| versions, verr := r.glClient.GetMRVersions(ctx, r.cfg.CIProjectID, r.cfg.CIMergeRequestID) | ||
| if verr != nil { | ||
| slog.Warn("failed to get MR versions for incremental review, falling back to full review", "error", verr) | ||
| } else if len(versions) > 1 { | ||
| // Compare previous version's head to current version's head. | ||
| prevHead := versions[1].HeadSHA | ||
| currHead := versions[0].HeadSHA | ||
| changedFiles, cerr := r.glClient.CompareCommits(ctx, r.cfg.CIProjectID, prevHead, currHead) | ||
| if cerr != nil { | ||
| slog.Warn("failed to compare commits for incremental review, falling back to full review", "error", cerr) | ||
| } else { | ||
| before := len(diffs) | ||
| diffs = filterByFiles(diffs, changedFiles) | ||
| slog.Info("incremental review", | ||
| "total_files", before, | ||
| "changed_files", len(changedFiles), | ||
| "reviewing", len(diffs), | ||
| ) | ||
| } | ||
| } else { | ||
| slog.Info("first push to MR, performing full review") | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the VCSClient interface and existing CompareCommits usages/mocks.
rg -n 'type VCSClient interface' -A 15 --type=go
rg -n 'CompareCommits' --type=goRepository: OpticDiff/code-reviewer
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the interface and the reviewer/output mocks, then inspect only the relevant slices.
fd -a 'reviewer.go|reviewer_test.go|output_test.go' internal
printf '\n--- VCSClient / CompareCommits search ---\n'
rg -n 'type VCSClient interface|CompareCommits|mockVCS|outputMockVCS' internal --type=go || true
printf '\n--- reviewer.go relevant lines ---\n'
sed -n '1,180p' internal/reviewer/reviewer.go
printf '\n--- reviewer_test.go relevant lines ---\n'
sed -n '1,260p' internal/reviewer/reviewer_test.go
printf '\n--- output_test.go relevant lines ---\n'
sed -n '1,260p' internal/output_test.goRepository: OpticDiff/code-reviewer
Length of output: 17527
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- internal/reviewer/interfaces.go ---\n'
sed -n '1,220p' internal/reviewer/interfaces.go
printf '\n--- CompareCommits implementation search ---\n'
rg -n 'func .*CompareCommits|CompareCommits\(' internal --type=go || true
printf '\n--- internal/reviewer/output_test.go relevant slice ---\n'
sed -n '280,380p' internal/reviewer/output_test.go
printf '\n--- compile-time interface checks in tests ---\n'
rg -n 'var _ VCSClient = ' internal/reviewer --type=go || trueRepository: OpticDiff/code-reviewer
Length of output: 4459
Add CompareCommits to VCSClient
internal/reviewer/reviewer.go calls r.glClient.CompareCommits(...), but internal/reviewer/interfaces.go does not declare it and the test doubles in internal/reviewer/reviewer_test.go and internal/reviewer/output_test.go don’t implement it. Add the method to VCSClient and implement/update the GitLab client and mocks, or this package will not build.
🧰 Tools
🪛 GitHub Actions: CI / 1_Test.txt
[error] 87-87: go test failed due to compile error: r.glClient.CompareCommits undefined (type VCSClient has no field or method CompareCommits)
🪛 GitHub Actions: CI / 2_Build.txt
[error] 87-87: go build failed: r.glClient.CompareCommits undefined (type VCSClient has no field or method CompareCommits)
🪛 GitHub Actions: CI / Build
[error] 87-87: go build failed: r.glClient.CompareCommits undefined (type VCSClient has no field or method CompareCommits)
🪛 GitHub Actions: CI / Test
[error] 87-87: go test failed: internal/reviewer/reviewer.go:87:37: r.glClient.CompareCommits undefined (type VCSClient has no field or method CompareCommits)
🪛 GitHub Check: Build
[failure] 87-87:
r.glClient.CompareCommits undefined (type VCSClient has no field or method CompareCommits)
🪛 GitHub Check: Lint
[failure] 87-87:
r.glClient.CompareCommits undefined (type VCSClient has no field or method CompareCommits)) (typecheck)
🪛 GitHub Check: Test
[failure] 87-87:
r.glClient.CompareCommits undefined (type VCSClient has no field or method CompareCommits)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/reviewer/reviewer.go` around lines 78 - 102, The incremental review
path in reviewer.go now calls CompareCommits on VCSClient, but the interface and
test doubles are missing that method. Add CompareCommits to VCSClient in
interfaces.go, implement it on the GitLab client, and update the
reviewer_test.go and output_test.go mocks so they satisfy the expanded interface
and the package builds cleanly.
Sources: Path instructions, Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/reviewer/reviewer_test.go (1)
1033-1241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a table-driven test for the incremental-review scenarios.
The four
TestRun_Incremental*functions share the samecfg/dsscaffolding and differ only inmockVCSsetup and assertions. Consolidating into a table-driven test would reduce duplication and make it easier to add new fallback scenarios later.As per path instructions for
**/*_test.go: "Verify table-driven tests, proper cleanup, and race condition safety."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/reviewer/reviewer_test.go` around lines 1033 - 1241, The incremental review tests are repetitive across the four TestRun_Incremental* cases, so consolidate them into a table-driven test to reduce duplicated cfg and mockDiffSource setup. Keep the shared Run/NewWithDiffSource scaffolding in one helper or loop, and vary only the mockVCS behavior, expected calls, and assertions for each scenario. Use the existing symbols TestRun_IncrementalReview, TestRun_IncrementalReview_FirstPush, TestRun_IncrementalReview_VersionErrorFallback, and TestRun_IncrementalReview_CompareErrorFallback as the cases to fold together.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/gitlab/client.go`:
- Around line 53-76: The CompareCommits method in Client currently only checks
compare_timeout, so it can return an incomplete file list when GitLab marks
diffs as collapsed or too_large. Update the response struct in CompareCommits to
capture those diff-level flags from Diffs, and fail closed with an error if any
diff is incomplete before building the returned files slice. Keep the existing
behavior in c.get, resp.CompareTimeout handling, and the final file collection
logic, but add the new completeness check inside CompareCommits.
---
Nitpick comments:
In `@internal/reviewer/reviewer_test.go`:
- Around line 1033-1241: The incremental review tests are repetitive across the
four TestRun_Incremental* cases, so consolidate them into a table-driven test to
reduce duplicated cfg and mockDiffSource setup. Keep the shared
Run/NewWithDiffSource scaffolding in one helper or loop, and vary only the
mockVCS behavior, expected calls, and assertions for each scenario. Use the
existing symbols TestRun_IncrementalReview, TestRun_IncrementalReview_FirstPush,
TestRun_IncrementalReview_VersionErrorFallback, and
TestRun_IncrementalReview_CompareErrorFallback as the cases to fold together.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 58ef3368-3645-498f-99a1-3b5c2a3ff032
📒 Files selected for processing (6)
internal/config/config.gointernal/gitlab/client.gointernal/reviewer/interfaces.gointernal/reviewer/output_test.gointernal/reviewer/reviewer.gointernal/reviewer/reviewer_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/reviewer/reviewer.go
- internal/config/config.go
1 participant
Summary
Adds SARIF (Static Analysis Results Interchange Format) 2.1.0 JSON output support, enabling CI systems (GitHub Code Scanning, GitLab Security Dashboard) to display code review findings in their native security/code scanning tabs.
Usage
Changes
internal/config/config.go: AddedSARIFOutputfield,SARIF_OUTPUTenv var, and--sarif <path>flaginternal/reviewer/sarif.go: SARIF 2.1.0 report builder with severity mapping (CRITICAL/HIGH→error, MEDIUM→warning, LOW→note), rule deduplication by category, and line clampinginternal/reviewer/sarif_test.go: Tests for severity mapping, empty findings, file I/O, and empty category fallbackinternal/reviewer/reviewer.go: Wired SARIF output into the review pipeline (runs after terminal/JSON output)SARIF output details
"general"github/codeql-action/upload-sarifSummary by CodeRabbit