feat: --fix mode — auto-apply suggestions - #29
Merged
Merged
Conversation
Run a review and automatically apply model-suggested fixes to local files. Usage: code-reviewer --diff --fix # review + apply fixes code-reviewer --diff --fix --dry-run # show what would be patched Architecture: - reviewer/fix.go: ApplyFixes engine + FormatFixSummary renderer - Groups fixes by file, applies bottom-up to preserve line numbers - Handles single-line and multi-line replacements - Graceful skip for missing files and out-of-range lines - config.go: --fix flag, requires --diff mode - Mutually exclusive with --explain and --summarize - reviewer.go: Step 8 wired after output — apply fixes if requested 9 new tests: - Single-line fix, multi-line expansion, no suggestion skip - File not found, line out of range - Multiple fixes same file (bottom-up ordering) - With repoRoot path resolution - FormatFixSummary empty + mixed applied/skipped
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
📝 WalkthroughWalkthroughThe PR adds a validated ChangesFix mode
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ReviewerRun
participant ApplyFixes
participant WorkingTree
participant FormatFixSummary
ReviewerRun->>ApplyFixes: validated findings and repository root
ApplyFixes->>WorkingTree: read and replace suggested lines
WorkingTree-->>ApplyFixes: applied or skipped result
ApplyFixes-->>ReviewerRun: fix results
ReviewerRun->>FormatFixSummary: fix results and color setting
FormatFixSummary-->>ReviewerRun: formatted summary
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: 5
🤖 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/fix.go`:
- Around line 100-104: Remove the model-generated "title" field from the
slog.Info call in the applied-fix logging block, keeping only stable metadata
such as fix.File and fix.Line; do not log any model-generated finding content.
- Around line 64-67: Constrain the file path handling in the fix flow around
filePath so model-derived Finding.File values cannot escape repoRoot. Clean the
joined path, compute its relative path from repoRoot, and skip the affected fix
when the relative path is outside the repository; retain normal processing for
paths within the root.
- Around line 47-62: The fix-processing logic around byFile and fixMap must
reject duplicate fixes targeting the same file and source line. Detect each
subsequent duplicate while building fixMap, mark it as skipped before sorting or
applying fixes, and ensure only the first fix remains eligible; add a regression
test covering same-file, same-line findings.
- Around line 107-118: Update the write-back flow around os.WriteFile in the
reviewer fix application function to write newContent to a temporary file in the
target’s directory, close and sync it, preserve the original file mode, and
atomically rename it over filePath only after success. Clean up failed temporary
files, mark all fixes for the file as failed through the existing
fixes[idx].Applied and Reason fields, and add a test covering write failure
without corrupting the original file.
In `@internal/reviewer/reviewer.go`:
- Around line 327-332: Update the Step 8 fix-summary output around ApplyFixes so
--fix --json never writes human-readable text to stdout; route FormatFixSummary
output to stderr when JSON mode is enabled while preserving normal stdout
behavior otherwise, and add coverage for the combined flags.
🪄 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: fbbc67e8-9122-4e5d-9f21-6517e65e2fb0
📒 Files selected for processing (4)
internal/config/config.gointernal/reviewer/fix.gointernal/reviewer/fix_test.gointernal/reviewer/reviewer.go
Comment on lines
+107
to
+118
| // Write back. | ||
| newContent := strings.Join(lines, "\n") | ||
| if err := os.WriteFile(filePath, []byte(newContent), 0o644); err != nil { | ||
| slog.Warn("failed to write file", "file", filePath, "error", err) | ||
| // Mark all fixes for this file as failed. | ||
| for _, idx := range idxs { | ||
| if fixes[idx].Applied { | ||
| fixes[idx].Applied = false | ||
| fixes[idx].Reason = fmt.Sprintf("write failed: %v", err) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Write fixes atomically.
os.WriteFile truncates the target before writing. A partial write failure can corrupt the working-tree file even though all fixes are reported as skipped. Write to a same-directory temporary file, close/sync it, then rename it atomically; preserve the original mode and add a write-failure test.
🤖 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/fix.go` around lines 107 - 118, Update the write-back flow
around os.WriteFile in the reviewer fix application function to write newContent
to a temporary file in the target’s directory, close and sync it, preserve the
original file mode, and atomically rename it over filePath only after success.
Clean up failed temporary files, mark all fixes for the file as failed through
the existing fixes[idx].Applied and Reason fields, and add a test covering write
failure without corrupting the original file.
1. Path traversal (SECURITY): Model-derived Finding.File could escape repoRoot via '../../'. Added filepath.Rel guard that skips fixes where the resolved path is outside the repo root. 2. Duplicate line collision (CORRECTNESS): Two fixes targeting the same file:line would silently corrupt each other. Added seen-map dedup that keeps the first fix and skips subsequent duplicates. 3. JSON stdout pollution: --fix --json appended human text after JSON. Fix summary now routes to stderr in JSON mode. 4. Log hygiene: Removed model-derived 'title' from slog to prevent sensitive diff content leaking to logs. 3 new tests: - PathTraversal: ../../etc/passwd → skipped - DuplicateSameLine: 2 fixes for same line → only first applied - AbsolutePathEscape: /etc/passwd → skipped
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/reviewer/fix_test.go (1)
236-325: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider consolidating into a table-driven test.
The file has roughly a dozen
TestApplyFixes_*functions that all exercise the sameApplyFixesentry point with different fixtures and a similar applied/skipped/reason assertion shape (including the three in this range). A table-driven test with per-case setup callbacks and expectations would reduce duplication and align with the stated preference for table-driven tests. As per coding guidelines, "Prefer table-driven tests and mock through interfaces... rather than concrete types" for**/*_test.go; as per path instructions, "Verify table-driven tests, proper cleanup, and race condition safety" for**/*_test.go.🤖 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/fix_test.go` around lines 236 - 325, Consolidate the related ApplyFixes tests, including TestApplyFixes_PathTraversal, TestApplyFixes_DuplicateSameLine, and TestApplyFixes_AbsolutePathEscape, into a table-driven test with per-case setup callbacks and expected applied/skipped/reason outcomes. Preserve each scenario’s fixtures and assertions, including traversal rejection, duplicate-line handling, and absolute-path rejection, while using t.Run and existing temporary-directory cleanup.Sources: Coding guidelines, 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/fix_test.go`:
- Around line 307-325: Strengthen TestApplyFixes_AbsolutePathEscape by asserting
the skipped fix’s Reason contains “path escapes repository root.” Add a separate
test case with repoRoot set to an empty string and an absolute temporary-file
path, verifying the fix is skipped and covers the unchecked-root branch.
---
Nitpick comments:
In `@internal/reviewer/fix_test.go`:
- Around line 236-325: Consolidate the related ApplyFixes tests, including
TestApplyFixes_PathTraversal, TestApplyFixes_DuplicateSameLine, and
TestApplyFixes_AbsolutePathEscape, into a table-driven test with per-case setup
callbacks and expected applied/skipped/reason outcomes. Preserve each scenario’s
fixtures and assertions, including traversal rejection, duplicate-line handling,
and absolute-path rejection, while using t.Run and existing temporary-directory
cleanup.
🪄 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: f56da742-e232-4e27-8989-617d7e4b4cea
📒 Files selected for processing (3)
internal/reviewer/fix.gointernal/reviewer/fix_test.gointernal/reviewer/reviewer.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/reviewer/reviewer.go
- internal/reviewer/fix.go
- AbsolutePathEscape: Now uses ../sibling/secret.txt to actually trigger the filepath.Rel traversal guard (not just file-not-found). Asserts 'escapes repository root' reason and verifies target file unmodified. - NoRepoRootAbsolutePath: New test covering repoRoot='' edge case with an absolute path, verifying graceful ReadFile failure.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/reviewer/fix_test.go (1)
353-372: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject rootless fix application instead of testing only a nonexistent path.
This only proves
ReadFilefails for this specific path. WithrepoRoot == "",ApplyFixesskips containment checks and writes any existing absolute path supplied in model output. Require a non-empty repository root before applying fixes, then use an existing temp file here and assert it remains unchanged.Proposed fix
func ApplyFixes(findings []model.Finding, repoRoot string) []ApplyFix { // Collect fixable findings. var fixes []ApplyFix // ... if len(fixes) == 0 { return nil } + + if repoRoot == "" { + for i := range fixes { + fixes[i].Reason = "repository root is required" + } + return fixes + }func TestApplyFixes_NoRepoRootAbsolutePath(t *testing.T) { - // When repoRoot is empty, the traversal guard is skipped. An absolute - // path to a non-existent file should fail gracefully on ReadFile. + target := filepath.Join(t.TempDir(), "outside.go") + if err := os.WriteFile(target, []byte("original\n"), 0o644); err != nil { + t.Fatal(err) + } findings := []model.Finding{{ - File: "/nonexistent/absolute/path.go", + File: target, // ... }} fixes := ApplyFixes(findings, "") - if !strings.Contains(fixes[0].Reason, "cannot read file") { - t.Errorf("expected read failure, got: %s", fixes[0].Reason) + if !strings.Contains(fixes[0].Reason, "repository root is required") { + t.Errorf("expected repository-root failure, got: %s", fixes[0].Reason) } }🤖 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/fix_test.go` around lines 353 - 372, Update ApplyFixes to reject an empty repoRoot before reading or writing any finding, ensuring rootless fix application produces an unapplied result without touching files. Revise TestApplyFixes_NoRepoRootAbsolutePath to use an existing temporary file, pass an empty root, and assert the fix is not applied and the file contents remain unchanged.
🤖 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/fix_test.go`:
- Line 307: Add doc comments for both exported test functions in
internal/reviewer/fix_test.go: lines 307-307, document that
TestApplyFixes_AbsolutePathEscape rejects sibling-path escapes; lines 353-353,
document the rootless absolute-path handling behavior. Use comments immediately
preceding each function and accurately describe the tested behavior.
---
Duplicate comments:
In `@internal/reviewer/fix_test.go`:
- Around line 353-372: Update ApplyFixes to reject an empty repoRoot before
reading or writing any finding, ensuring rootless fix application produces an
unapplied result without touching files. Revise
TestApplyFixes_NoRepoRootAbsolutePath to use an existing temporary file, pass an
empty root, and assert the fix is not applied and the file contents remain
unchanged.
🪄 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: e908b50a-de1a-4d31-82fb-741e311ca554
📒 Files selected for processing (1)
internal/reviewer/fix_test.go
| } | ||
| } | ||
|
|
||
| func TestApplyFixes_AbsolutePathEscape(t *testing.T) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add doc comments for the new exported test functions.
internal/reviewer/fix_test.go#L307-L307: add a comment describing the sibling-path escape rejection.internal/reviewer/fix_test.go#L353-L353: add a comment describing rootless absolute-path handling.
📍 Affects 1 file
internal/reviewer/fix_test.go#L307-L307(this comment)internal/reviewer/fix_test.go#L353-L353
🤖 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/fix_test.go` at line 307, Add doc comments for both
exported test functions in internal/reviewer/fix_test.go: lines 307-307,
document that TestApplyFixes_AbsolutePathEscape rejects sibling-path escapes;
lines 353-353, document the rootless absolute-path handling behavior. Use
comments immediately preceding each function and accurately describe the tested
behavior.
Source: Coding guidelines
Collapsed 13 individual TestApplyFixes_* functions + 2 TestFormatFixSummary_* into 2 table-driven tests with setup callbacks. Same coverage, less duplication. TestApplyFixes subtests (11): - single line replacement - multi-line suggestion expands one line - no suggestion is a no-op - file not found - line out of range - multiple fixes same file applied bottom-up - with repoRoot path resolution - path traversal via ../sibling blocked - ../../etc/passwd traversal blocked - duplicate same-line keeps first only - no repoRoot with absolute path fails gracefully TestFormatFixSummary subtests (2): - nil returns no suggestions message - mixed applied and skipped
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/reviewer/fix_test.go (1)
209-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winZero-valued count expectations are never asserted. The
> 0guards mean any case expecting0applied/skipped (e.g."no suggestion is a no-op") verifies nothing, and positive cases won't catch stray extra skips/applies (e.g."single line replacement"never checks thatskipped == 0). Assert both counts unconditionally so regressions surface.♻️ Assert exact counts
- if tt.wantApplied > 0 && applied != tt.wantApplied { - t.Errorf("applied = %d, want %d", applied, tt.wantApplied) - } - if tt.wantSkipped > 0 && skipped != tt.wantSkipped { - t.Errorf("skipped = %d, want %d", skipped, tt.wantSkipped) - } + if applied != tt.wantApplied { + t.Errorf("applied = %d, want %d", applied, tt.wantApplied) + } + if skipped != tt.wantSkipped { + t.Errorf("skipped = %d, want %d", skipped, tt.wantSkipped) + }🤖 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/fix_test.go` around lines 209 - 214, Update the count assertions in the test around the applied and skipped result checks to compare both values unconditionally against wantApplied and wantSkipped. Remove the > 0 guards so zero-valued expectations are validated and positive cases also detect unexpected extra applies or skips.
🤖 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/fix_test.go`:
- Around line 239-254: Update the checkPath file-content assertion block to fail
the test when os.ReadFile returns an error, rather than silently skipping the
wantIn and wantNotIn checks. Report the read failure with t.Errorf or the test’s
existing failure mechanism, and retain both content assertion loops for
successfully read files.
---
Nitpick comments:
In `@internal/reviewer/fix_test.go`:
- Around line 209-214: Update the count assertions in the test around the
applied and skipped result checks to compare both values unconditionally against
wantApplied and wantSkipped. Remove the > 0 guards so zero-valued expectations
are validated and positive cases also detect unexpected extra applies or skips.
🪄 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: a730f3db-c0a8-49d2-82ae-97c0f45c2850
📒 Files selected for processing (1)
internal/reviewer/fix_test.go
- Remove > 0 guards so wantApplied:0 / wantSkipped:0 cases actually verify, and positive cases catch stray extra applies/skips. - Fail test immediately on ReadFile error instead of silently skipping content checks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
New
--fixflag that runs a normal review, then automatically applies model-suggested fixes to local files in your working tree.The demo feature — review and fix in one command.
Usage
Example Output
Architecture
reviewer/fix.goApplyFixes()engine +FormatFixSummary()rendererreviewer/reviewer.goconfig.go--fixflag, requires--diffmodeFix Engine
repoRootValidation
--fixrequires--diffmode (no CI — can't modify remote files)--explainand--summarizeTests (9 new)
SingleLineMultiLineSuggestionNoSuggestionFileNotFoundLineOutOfRangeMultipleFixesSameFileWithRepoRootFormatFixSummary_EmptyFormatFixSummary_MixedSummary by CodeRabbit
New Features
--fixmode to automatically apply eligible review suggestions to the working tree.--fixis diff-only and cannot be combined with explain or summarize modes.Tests