Conversation
| let line_count = format_advice | ||
| .replacements | ||
| .iter() | ||
| .fold(0, |acc, r| acc + r.len()); | ||
| let note = format!( | ||
| "- {} ({line_count} line{})\n", | ||
| file.name.to_string_lossy().replace('\\', "/"), | ||
| if line_count > 1 { "s" } else { "" } | ||
| ); |
There was a problem hiding this comment.
Oh yea, I forgot to mention the thread comment is augmented to display the number of lines changed by clang-format. For example:
- src/demo/demo.cpp (4 lines)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #347 +/- ##
==========================================
- Coverage 92.89% 92.85% -0.05%
==========================================
Files 22 22
Lines 3364 3316 -48
==========================================
- Hits 3125 3079 -46
+ Misses 239 237 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
The missing lines in coverage are all about error handling. These weren't covered before either. It is rather difficult to instigate errors from external binaries or the file system... and doesn't seem worth it. |
|
Warning Review limit reached
More reviews will be available in 15 minutes and 3 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
WalkthroughThis PR replaces XML-based clang-format output parsing with file-based diff computation and project-local caching. The FormatAdvice data model shifts from offset-keyed Replacement vectors to Range collections backed by cached patch files, eliminating the quick-xml dependency. ChangesClang-format diff-based output processing and caching
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp-linter/src/clang_tools/clang_format.rs (1)
109-137:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't derive
FormatAdvicefrom a failedclang-formatinvocation.This path caches
output.stdoutand builds a diff even whenoutput.status.success()is false. On a non-zero exit with empty or partial stdout, that turns a tool failure into bogus formatting advice instead of surfacing the failure cleanly. Bail out before the cache write/diff unless the subprocess exited successfully.🤖 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 `@cpp-linter/src/clang_tools/clang_format.rs` around lines 109 - 137, Check output.status.success() immediately after running clang-format and bail out on non-zero exit before writing cache_format_fixes or computing diffs/FormatAdvice; specifically, if !output.status.success() return an Err variant of ClangCaptureError that surfaces the tool failure (include stderr and exit code) so you do not proceed to fs::write(&cache_format_fixes, &output.stdout) or build patched_contents/original_contents and derive FormatAdvice from potentially empty/partial stdout.
🤖 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 `@cpp-linter/src/clang_tools/clang_format.rs`:
- Around line 141-159: The replacements vector currently stores hunk.before
verbatim, but pure-insertion hunks have an empty hunk.before which causes
downstream code (RestClient::make_annotations() and make_format_comment()) to
count zero affected lines; detect when hunk.before.is_empty() inside the
diff.hunks() filter_map and replace it with a one-line anchored range
immediately before the insertion point (e.g., let anchor_start =
hunk.before.start.saturating_sub(1); use a Range from anchor_start to
anchor_start + 1) before pushing into FormatAdvice.replacements so
insertion-only fixes report as affecting one line.
In `@cpp-linter/src/run.rs`:
- Around line 163-172: ClangParams::from(&cli) currently computes
project_cache_dir relative to the original repo_root before the code calls
set_current_dir(), causing double-prefix paths; fix by ensuring
project_cache_dir is resolved to an absolute path before changing directories or
by rebuilding/normalizing it after set_current_dir(): either call
std::env::set_current_dir(...) first and then recreate clang_params with
ClangParams::from(&cli), or canonicalize the existing
clang_params.project_cache_dir (e.g., std::fs::canonicalize or join with
std::env::current_dir()) so project_cache_dir is absolute before using it in
create_dir_all/write for project_cache_dir and .gitignore; update uses of
ClangParams::project_cache_dir accordingly.
---
Outside diff comments:
In `@cpp-linter/src/clang_tools/clang_format.rs`:
- Around line 109-137: Check output.status.success() immediately after running
clang-format and bail out on non-zero exit before writing cache_format_fixes or
computing diffs/FormatAdvice; specifically, if !output.status.success() return
an Err variant of ClangCaptureError that surfaces the tool failure (include
stderr and exit code) so you do not proceed to fs::write(&cache_format_fixes,
&output.stdout) or build patched_contents/original_contents and derive
FormatAdvice from potentially empty/partial stdout.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 01bf8d26-b860-4155-af83-0e0ab4078338
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
cpp-linter/Cargo.tomlcpp-linter/src/clang_tools/clang_format.rscpp-linter/src/clang_tools/clang_tidy.rscpp-linter/src/cli/structs.rscpp-linter/src/common_fs/mod.rscpp-linter/src/error.rscpp-linter/src/rest_client/mod.rscpp-linter/src/run.rs
💤 Files with no reviewable changes (1)
- cpp-linter/Cargo.toml
This avoids discrepancies with clang-format's XML output.; see cpp-linter/cpp-linter#168. Instead, we now save the clang-format fixed output to a project-specific cache folder. Then we take the diff between the formatted output and the original file's content. The resulting diff is how we determine the lines changed by clang-format. This actually lays the ground work for other improvements, specifically how to assemble changes for an auto-fix commit. Additionally, this also removes the dependency used for XML parsing .
This is a culmination of - #354 - #347 It it the last step needed before providing a patch that consumers can push to auto-fix lints. # Intended flow of data 1. run clang-tidy before running clang-format 2. after running clang-tidy save the patched file to cache 3. after running clang-format: - check for clang-tidy patch in cache. - if cached patch is present, run clang-format on the clang-tidy patch. This step includes formatting lines that clang-tidy changed and lines that clang-format changed. - if cached patch is not present (clang-tidy was not run on the file), then cache the clang-format fixes instead. - `--lines-changed-only` is respected, but this may need tweaking in the future. 4. when creating a PR review, only add hunks that are not present in the review comments. I added a parameter to also calculate (and display) how many review comments were reused in previous PR reviews.
This is a culmination of - #354 - #347 It it the last step needed before providing a patch that consumers can push to auto-fix lints. # Intended flow of data 1. run clang-tidy before running clang-format 2. after running clang-tidy save the patched file to cache 3. after running clang-format: - check for clang-tidy patch in cache. - if cached patch is present, run clang-format on the clang-tidy patch. This step includes formatting lines that clang-tidy changed and lines that clang-format changed. - if cached patch is not present (clang-tidy was not run on the file), then cache the clang-format fixes instead. - `--lines-changed-only` is respected, but this may need tweaking in the future. 4. when creating a PR review, only add hunks that are not present in the review comments. I added a parameter to also calculate (and display) how many review comments were reused in previous PR reviews.
This is a culmination of - #354 - #347 It it the last step needed before providing a patch that consumers can push to auto-fix lints. # Intended flow of data 1. run clang-tidy before running clang-format 2. after running clang-tidy save the patched file to cache 3. after running clang-format: - check for clang-tidy patch in cache. - if cached patch is present, run clang-format on the clang-tidy patch. This step includes formatting lines that clang-tidy changed and lines that clang-format changed. - if cached patch is not present (clang-tidy was not run on the file), then cache the clang-format fixes instead. - `--lines-changed-only` is respected, but this may need tweaking in the future. 4. when creating a PR review, only add hunks that are not present in the review comments. I added a parameter to also calculate (and display) how many review comments were reused in previous PR reviews. Adjusted tests accordingly.
This is a culmination of - #354 - #347 It it the last step needed before providing a patch that consumers can push to auto-fix lints. # Intended flow of data 1. run clang-tidy before running clang-format 2. after running clang-tidy save the patched file to cache 3. after running clang-format: - check for clang-tidy patch in cache. - if cached patch is present, run clang-format on the clang-tidy patch. This step includes formatting lines that clang-tidy changed and lines that clang-format changed. - if cached patch is not present (clang-tidy was not run on the file), then cache the clang-format fixes instead. - `--lines-changed-only` is respected, but this may need tweaking in the future. 4. when creating a PR review, only add hunks that are not present in the review comments. I added a parameter to also calculate (and display) how many review comments were reused in previous PR reviews. Adjusted tests accordingly.
This is a culmination of - #354 - #347 It it the last step needed before providing a patch that consumers can push to auto-fix lints. # Intended flow of data 1. run clang-tidy before running clang-format 2. after running clang-tidy save the patched file to cache 3. after running clang-format: - check for clang-tidy patch in cache. - if cached patch is present, run clang-format on the clang-tidy patch. This step includes formatting lines that clang-tidy changed and lines that clang-format changed. - if cached patch is not present (clang-tidy was not run on the file), then cache the clang-format fixes instead. - `--lines-changed-only` is respected, but this may need tweaking in the future. 4. when creating a PR review, only add hunks that are not present in the review comments. I added a parameter to also calculate (and display) how many review comments were reused in previous PR reviews. Adjusted tests accordingly.
This is a culmination of - #354 - #347 It it the last step needed before providing a patch that consumers can push to auto-fix lints. # Intended flow of data 1. run clang-tidy before running clang-format 2. after running clang-tidy save the patched file to cache 3. after running clang-format: - check for clang-tidy patch in cache. - if cached patch is present, run clang-format on the clang-tidy patch. This step includes formatting lines that clang-tidy changed and lines that clang-format changed. - if cached patch is not present (clang-tidy was not run on the file), then cache the clang-format fixes instead. - `--lines-changed-only` is respected, but this may need tweaking in the future. 4. when creating a PR review, only add hunks that are not present in the review comments. I added a parameter to also calculate (and display) how many review comments were reused in previous PR reviews. If PR review is a summary only, then the patch will include all fixes that would have been posted in the diff. Adjusted tests accordingly.
This is a culmination of - [x] #354 - [x] #347 As this builds on the plan discussed in cpp-linter/cpp-linter#82, it it the last step needed before providing a patch that consumers can push to auto-fix lints. ## Intended flow of data 1. run clang-tidy before running clang-format 2. after running clang-tidy save the patched file to cache 3. after running clang-format: - check for clang-tidy patch in cache. - if cached patch is present, run clang-format on the clang-tidy patch. This step includes formatting lines that clang-tidy changed and lines that clang-format changed. - if cached patch is not present (clang-tidy was not run on the file), then cache the clang-format fixes instead. - `--lines-changed-only` is respected, but this may need tweaking in the future. 4. when creating a PR review, - only add hunks to the patch that are not present in the review comments. - I added a parameter to also calculate (and display) how many review comments were reused in previous PR reviews. - PR review summaries are worded better if the env var `CPP_LINTER_PR_REVIEW_SUMMARY_ONLY = 1`. If PR review is a summary only, then the patch will include all fixes that would have been posted in the diff. --- Adjusted tests accordingly. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved formatting/tidy patch handling to generate more accurate review diffs and line-range suggestions. * Refined how review summaries are produced (including “summary-only” behavior and reused-review messaging). * Improved review comment metadata when line ranges are identical. * **Tests** * Expanded diff and three-way diff coverage. * Updated review-body expectation logic to match the new summary rules. * **Chores** * Updated documentation dependency. * Refined caching and output processing for more reliable review generation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
resolves #376 This is the result of - #358 - #354 - #347 If there are any auto-fixes available, then a patch full of them is created and its path is set to a new output `fix-patch-path` output variable. If there were no auto-fixes, then the output variable is not set. Remember, an unset output variable is treated as an empty string (a false value) in `steps.linter.outputs.fix-patch-path` context. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Auto-fix patches are aggregated into a single unified patch file in the repository cache directory. * CI/CD now receives an optional `fix-patch-path` output (with normalized forward slashes) pointing to the generated patch when present. * Cached patch aggregation is recorded consistently when producing suggestions. * **Bug Fixes** * Clear any previously generated unified patch before starting a new run to prevent stale outputs. * **Tests** * Updated test inputs to ignore `build/**` and added validation for the generated patch file contents. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This avoids discrepancies with clang-format's XML output.; see cpp-linter/cpp-linter#168. Instead, we now save the clang-format fixed output to a project-specific cache folder. Then we take the diff between the formatted output and the original file's content. The resulting diff is how we determine the lines changed by clang-format.
This actually lays the ground work for other improvements, specifically how to assemble changes for an auto-fix commit.
Additionally, this also removes the dependency used for XML parsing.
Summary by CodeRabbit
New Features
.cpp-linter-cachedirectory to store formatting patches.Improvements