feat: ANSI colored terminal output, test coverage boost, CI cleanup - #4
Conversation
Local Mode UX: - New ColorTerminalOutput with ANSI colors: red=CRITICAL, orange=HIGH, yellow=MEDIUM, blue=LOW - Box-drawn summary header with severity counts - File-grouped findings with visual separators - Indented suggestion blocks with green highlighting - Auto-detect TTY: falls back to plain text when piped - --no-color flag and NO_COLOR env var (https://no-color.org/) Test Coverage (92 → 104 tests): - output_test.go: ColorTerminalOutput, formatSummaryNote, severityEmoji, severityColor, formatInlineComment (12 tests) - validator_test.go: isInHunkRange boundaries, negative line, partial path match (10 tests added) - Reviewer coverage: 44.7% → 65.7% CI Cleanup: - Split into 3 parallel jobs: build, test, lint - Added coverage profiling (go test -coverprofile) - Coverage summary printed in CI output - Coverage artifact uploaded for download - Deleted 3 stale merged branches from remote
|
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)
📝 WalkthroughWalkthroughAdds a ChangesColorized Terminal Output
CI Workflow Restructure
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 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: 2
🧹 Nitpick comments (4)
internal/reviewer/terminal.go (3)
32-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider splitting
ColorTerminalOutputfor readability.The function mixes header-box rendering, severity counting, and per-file/per-finding rendering in one ~100-line block. Extracting helpers (e.g.,
writeSummaryBox,writeFindingsByFile) would improve readability without changing 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/reviewer/terminal.go` around lines 32 - 134, `ColorTerminalOutput` is doing too much in one long block, making the rendering logic hard to read and maintain. Split the function into small helpers such as `writeSummaryBox` for the header/severity counts and `writeFindingsByFile` for grouping and rendering findings, then have `ColorTerminalOutput` orchestrate those helpers while preserving the existing output exactly.
59-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffFragile byte-length-based padding for emoji counts line.
Padding correction (
padLen := 53 - len(countsLine) + (len(parts) * 2)) hardcodes a per-emoji byte-width offset that only holds for the four specific single-codepoint emoji currently used. Any change to the emoji set (e.g., adding a variation-selector emoji or a wide-but-different-byte-length one) will silently misalign the box border. Consider computing visual width explicitly (e.g., via a rune/width-aware helper) rather than deriving it from byte length + a magic multiplier.🤖 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/terminal.go` around lines 59 - 80, The counts-line padding logic in the terminal box renderer is using a byte-length heuristic with a magic emoji multiplier, which is brittle for any future emoji changes. Update the padding calculation around countsLine/countsPadded in the terminal rendering path to use a visual-width-aware helper instead of len(countsLine) plus a fixed offset, so the border stays aligned regardless of which severity icons are appended.
151-156: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the local
maxhelper with the Go builtin.internal/reviewer/terminal.go:105,151-156— the module targets Go 1.26.1, so this helper is unnecessary; usemax(1, 50-len(file))directly and drop the local function.🤖 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/terminal.go` around lines 151 - 156, Remove the local max helper from terminal.go and switch the caller that computes the page size in the reviewer terminal flow to the Go builtin max instead. Update the logic that currently relies on the max(a, b int) function to use max(1, 50-len(file)) directly, and then delete the now-unused max helper definition..github/workflows/ci.yml (1)
32-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a composite action to dedupe checkout/setup-go across jobs.
Each of the three jobs (
build,test,lint) repeats the sameactions/checkout+actions/setup-gopair. A composite action or reusable workflow step would reduce duplication and keep the Go version pinned in a single place.🤖 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 @.github/workflows/ci.yml around lines 32 - 53, The build, test, and lint jobs repeat the same checkout and Go setup steps, so centralize that setup in a reusable composite action or shared workflow component. Update the test job alongside the other jobs to call the shared setup instead of duplicating actions/checkout and actions/setup-go, and keep the Go version pin in one place for easier maintenance.
🤖 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 @.github/workflows/ci.yml:
- Around line 32-53: The checkout steps in the test, build, and lint jobs are
persisting Git credentials by default even though these jobs never push changes.
Update each actions/checkout invocation to disable persisted credentials by
setting persist-credentials to false, so the GITHUB_TOKEN is not written into
.git/config after checkout.
In `@internal/reviewer/terminal.go`:
- Around line 110-114: The lint failure comes from building the formatted line
in terminal.go with sb.WriteString(fmt.Sprintf(...)); update the code in the
terminal rendering path to write the formatted output directly to the
strings.Builder using fmt.Fprintf instead. Keep the same formatting and
variables (including f.Line, sevColor, ansiBold, sevLabel, ansiReset, and
f.Title) so the output stays identical while satisfying staticcheck QF1012.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 32-53: The build, test, and lint jobs repeat the same checkout and
Go setup steps, so centralize that setup in a reusable composite action or
shared workflow component. Update the test job alongside the other jobs to call
the shared setup instead of duplicating actions/checkout and actions/setup-go,
and keep the Go version pin in one place for easier maintenance.
In `@internal/reviewer/terminal.go`:
- Around line 32-134: `ColorTerminalOutput` is doing too much in one long block,
making the rendering logic hard to read and maintain. Split the function into
small helpers such as `writeSummaryBox` for the header/severity counts and
`writeFindingsByFile` for grouping and rendering findings, then have
`ColorTerminalOutput` orchestrate those helpers while preserving the existing
output exactly.
- Around line 59-80: The counts-line padding logic in the terminal box renderer
is using a byte-length heuristic with a magic emoji multiplier, which is brittle
for any future emoji changes. Update the padding calculation around
countsLine/countsPadded in the terminal rendering path to use a
visual-width-aware helper instead of len(countsLine) plus a fixed offset, so the
border stays aligned regardless of which severity icons are appended.
- Around line 151-156: Remove the local max helper from terminal.go and switch
the caller that computes the page size in the reviewer terminal flow to the Go
builtin max instead. Update the logic that currently relies on the max(a, b int)
function to use max(1, 50-len(file)) directly, and then delete the now-unused
max helper definition.
🪄 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: 7ab26677-d3bb-4ca2-a3ef-255865325d1c
⛔ Files ignored due to path filters (2)
go.modis excluded by!go.modgo.sumis excluded by!**/*.sum,!go.sum
📒 Files selected for processing (6)
.github/workflows/ci.ymlinternal/config/config.gointernal/reviewer/output_test.gointernal/reviewer/reviewer.gointernal/reviewer/terminal.gointernal/reviewer/validator_test.go
| test: | ||
| name: Test | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: '1.26' | ||
|
|
||
| - name: Test with coverage | ||
| run: go test ./... -v -count=1 -race -coverprofile=coverage.out -covermode=atomic | ||
|
|
||
| - name: Coverage summary | ||
| run: go tool cover -func=coverage.out | tail -1 | ||
|
|
||
| - name: Upload coverage artifact | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: coverage-report | ||
| path: coverage.out | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Add persist-credentials: false to checkout steps that don't need to push.
The actions/checkout step here (and similarly in the build/lint jobs) leaves persist-credentials at its default of true, so the GITHUB_TOKEN is written to .git/config for the remainder of the job. This job also uploads a coverage.out artifact — if the artifact path ever broadens (e.g. workspace archiving) or a subsequent step is compromised, the persisted token could leak. Since none of these jobs push commits, credentials aren't needed post-checkout.
🔒 Proposed fix
- uses: actions/checkout@v4
+ with:
+ persist-credentials: falseApply the same change to the checkout steps in the build (line 20) and lint (line 58) jobs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test: | |
| name: Test | |
| runs-on: ubuntu-latest | |
| steps: | |
| - uses: actions/checkout@v4 | |
| - uses: actions/setup-go@v5 | |
| with: | |
| go-version: '1.26' | |
| - name: Test with coverage | |
| run: go test ./... -v -count=1 -race -coverprofile=coverage.out -covermode=atomic | |
| - name: Coverage summary | |
| run: go tool cover -func=coverage.out | tail -1 | |
| - name: Upload coverage artifact | |
| uses: actions/upload-artifact@v4 | |
| with: | |
| name: coverage-report | |
| path: coverage.out | |
| test: | |
| name: Test | |
| runs-on: ubuntu-latest | |
| steps: | |
| - uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false | |
| - uses: actions/setup-go@v5 | |
| with: | |
| go-version: '1.26' | |
| - name: Test with coverage | |
| run: go test ./... -v -count=1 -race -coverprofile=coverage.out -covermode=atomic | |
| - name: Coverage summary | |
| run: go tool cover -func=coverage.out | tail -1 | |
| - name: Upload coverage artifact | |
| uses: actions/upload-artifact@v4 | |
| with: | |
| name: coverage-report | |
| path: coverage.out |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 36-36: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/ci.yml around lines 32 - 53, The checkout steps in the
test, build, and lint jobs are persisting Git credentials by default even though
these jobs never push changes. Update each actions/checkout invocation to
disable persisted credentials by setting persist-credentials to false, so the
GITHUB_TOKEN is not written into .git/config after checkout.
Source: Linters/SAST tools
1 participant
Summary
Three improvements in one PR:
🎨 Local Mode UX — Rich Terminal Output
code-reviewer --diffnow produces ANSI-colored output:--no-colorflag +NO_COLORenv var (no-color.org)🧪 Test Coverage: 92 → 104 tests
New tests:
output_test.go: 12 tests — ColorTerminalOutput, formatSummaryNote, severityEmoji, severityColor, formatInlineCommentvalidator_test.go: +10 tests — isInHunkRange boundaries, negative lines, partial path matching🔧 CI Cleanup
build,test,lintgo test -coverprofile=coverage.out104 tests pass, -race clean.
Summary by CodeRabbit
--no-color/NO_COLOR) and automatic colorized terminal output when supported.