Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #349 +/- ##
==========================================
- Coverage 92.85% 92.43% -0.42%
==========================================
Files 22 21 -1
Lines 3316 3226 -90
==========================================
- Hits 3079 2982 -97
- Misses 237 244 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Collaborator
Author
|
This seemed easier than I thought it would be. Let's see what the AI review thinks... |
Contributor
WalkthroughThis PR threads a repo_root PathBuf through CLI params, RestClient, FileObj suggestion generation, and clang tool invocations; clang-format/tidy now run with CWD=repo_root and caches/IO use repo_root.join(CACHE_DIR) and repo_root.join(file.name) rather than relying on process CWD or a derived project cache dir. ChangesRepository Root Path Propagation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/rest_client/mod.rs`:
- Around line 76-81: RestClient::get_list_of_changed_files currently constructs
FileObj.name as a repo-root-relative path via
FileObj::from(...strip_prefix(repo_root)...), which causes
FileObj::make_suggestions_from_patch and clang_tidy::run_clang_tidy to fail
because they call fs::read/_write using that relative name while the process cwd
is not repo_root; fix by making file access use repo_root.join(&file.name) (or
store absolute paths in FileObj.name) — update FileObj::from (used by
RestClient::get_list_of_changed_files) or ensure all reads/writes in
FileObj::make_suggestions_from_patch (cpp-linter/src/common_fs/mod.rs) and
clang_tidy::run_clang_tidy (cpp-linter/src/clang_tools/clang_tidy.rs) prepend
clang_params.repo_root/appropriate repo_root PathBuf via join before calling
fs::read, fs::write, or any file APIs so behaviour matches clang_format which
already uses clang_params.repo_root.join(&file.name).
🪄 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: 291ffd5c-14ca-44fc-a032-30bdee4b61c4
📒 Files selected for processing (8)
cpp-linter/src/clang_tools/clang_format.rscpp-linter/src/clang_tools/clang_tidy.rscpp-linter/src/clang_tools/mod.rscpp-linter/src/cli/structs.rscpp-linter/src/git.rscpp-linter/src/rest_client/mod.rscpp-linter/src/run.rscpp-linter/tests/paginated_changed_files.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cpp-linter/tests/common/mod.rs (1)
51-54: 💤 Low valueLost diagnostic output when Meson commands fail.
The change from
.output()to.status()discards stdout/stderr. Ifmeson initormeson setupfails, developers will only see the generic panic message without the command's actual error output, making failures harder to debug.🔧 Proposed fix to preserve diagnostic output
- let status = cmd.status().expect("Failed to run 'meson init'"); - if !status.success() { - panic!("Failed to initialize meson project with 'meson init'"); - } + let output = cmd.output().expect("Failed to run 'meson init'"); + if !output.status.success() { + panic!( + "Failed to initialize meson project with 'meson init':\n{}", + String::from_utf8_lossy(&output.stderr) + ); + }Apply the same pattern to the
meson setupcommand:- let status = cmd - .status() - .expect("Failed to generate build assets with 'meson setup'"); - if !status.success() { - panic!("Failed to generate build assets with 'meson setup'"); - } + let output = cmd + .output() + .expect("Failed to generate build assets with 'meson setup'"); + if !output.status.success() { + panic!( + "Failed to generate build assets with 'meson setup':\n{}", + String::from_utf8_lossy(&output.stderr) + ); + }Also applies to: 63-68
🤖 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/tests/common/mod.rs` around lines 51 - 54, The tests currently call cmd.status() for the "meson init" (and similarly for "meson setup") commands which discards stdout/stderr; change these to use cmd.output(), check output.status.success(), and include output.stdout and output.stderr in the panic/error message so failing meson invocations show diagnostic output; update the blocks that create/run the command (the occurrences that set status from cmd and panic on failure) to capture and print the command's stdout/stderr for both "meson init" and "meson setup".cpp-linter/src/clang_tools/clang_tidy.rs (1)
300-300: 💤 Low valueConsider using
.join()for consistency with clang_format.rs.The equivalent path construction in
clang_format.rs(line 127) usesclang_params.repo_root.join(&file.name). Using the same idiom here improves consistency across the codebase.Suggested change
- let repo_file_path = PathBuf::from_iter([&clang_params.repo_root, &file.name]); + let repo_file_path = clang_params.repo_root.join(&file.name);🤖 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_tidy.rs` at line 300, Replace the PathBuf::from_iter construction used to build repo_file_path with the same join idiom used in clang_format.rs for consistency: use clang_params.repo_root.join(&file.name) when creating repo_file_path (symbols to update: repo_file_path, clang_params, file.name in clang_tidy.rs).
🤖 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/tests/paginated_changed_files.rs`:
- Line 39: The test get_paginated_changes has an incomplete refactor: remove the
unused internal TempDir and any env::set_current_dir calls, stop creating a new
TempDir inside the helper, and use the passed _tmp_dir (rename to tmp_dir if you
remove the underscore) throughout; specifically, replace uses of tmp.path() and
&PathBuf::from(".") with tmp_dir.path(), add the repo-root arg using
tmp_dir.path() (e.g. --repo-root=<tmp_dir.path()>), and update event_payload
construction to use tmp_dir.path() so the helper and get_paginated_changes
mirror the patterns in comments.rs and reviews.rs.
---
Nitpick comments:
In `@cpp-linter/src/clang_tools/clang_tidy.rs`:
- Line 300: Replace the PathBuf::from_iter construction used to build
repo_file_path with the same join idiom used in clang_format.rs for consistency:
use clang_params.repo_root.join(&file.name) when creating repo_file_path
(symbols to update: repo_file_path, clang_params, file.name in clang_tidy.rs).
In `@cpp-linter/tests/common/mod.rs`:
- Around line 51-54: The tests currently call cmd.status() for the "meson init"
(and similarly for "meson setup") commands which discards stdout/stderr; change
these to use cmd.output(), check output.status.success(), and include
output.stdout and output.stderr in the panic/error message so failing meson
invocations show diagnostic output; update the blocks that create/run the
command (the occurrences that set status from cmd and panic on failure) to
capture and print the command's stdout/stderr for both "meson init" and "meson
setup".
🪄 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: ec802b89-c7de-497c-baf3-f795a7061a57
📒 Files selected for processing (10)
cpp-linter/src/clang_tools/clang_tidy.rscpp-linter/src/cli/structs.rscpp-linter/src/common_fs/mod.rscpp-linter/src/git.rscpp-linter/src/rest_client/mod.rscpp-linter/src/run.rscpp-linter/tests/comments.rscpp-linter/tests/common/mod.rscpp-linter/tests/paginated_changed_files.rscpp-linter/tests/reviews.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- cpp-linter/src/git.rs
- cpp-linter/src/run.rs
ref: - cpp-linter/cpp-linter#144 - cpp-linter/cpp-linter#146 Let cpp-linter stay in working directory. Instead of changing into the given `--repo-root`, this patch uses the value as a scope of file system focus. - All clang tools are invoked in the given `--repo-root`. - All discovered files are stripped of `--repo-root` - Any local caching done is prefixed with `--repo-root` value. `--repo-root` value has always been expected sa a relative path, but this should also work if given an absolute path too.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/mod.rs`:
- Around line 143-151: The code builds the compile_commands.json path by joining
db_path twice when db_path.is_relative() (resulting in
<repo_root>/<db_path>/<db_path>/compile_commands.json); fix this in the fs::read
call in clang_tools/mod.rs by first computing a single base path (let base = if
db_path.is_relative() { clang_params.repo_root.join(db_path) } else {
db_path.to_path_buf() }) and then calling base.join("compile_commands.json")
(remove the extra .join(db_path)), ensuring the code that reads/opens the DB
uses that corrected base path.
🪄 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: 56ed8a21-4ff5-460f-b9be-e4d93a4f0ae3
📒 Files selected for processing (12)
cpp-linter/src/clang_tools/clang_format.rscpp-linter/src/clang_tools/clang_tidy.rscpp-linter/src/clang_tools/mod.rscpp-linter/src/cli/structs.rscpp-linter/src/common_fs/mod.rscpp-linter/src/git.rscpp-linter/src/rest_client/mod.rscpp-linter/src/run.rscpp-linter/tests/comments.rscpp-linter/tests/common/mod.rscpp-linter/tests/paginated_changed_files.rscpp-linter/tests/reviews.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- cpp-linter/tests/common/mod.rs
- cpp-linter/src/common_fs/mod.rs
- cpp-linter/src/clang_tools/clang_format.rs
- cpp-linter/tests/comments.rs
- cpp-linter/src/rest_client/mod.rs
- cpp-linter/tests/reviews.rs
- cpp-linter/src/clang_tools/clang_tidy.rs
- cpp-linter/src/run.rs
- cpp-linter/src/cli/structs.rs
Comment on lines
+143
to
+151
| && let Ok(db_str) = fs::read( | ||
| if db_path.is_relative() { | ||
| clang_params.repo_root.join(db_path) | ||
| } else { | ||
| db_path.to_path_buf() | ||
| } | ||
| .join(db_path) | ||
| .join("compile_commands.json"), | ||
| ) |
Contributor
There was a problem hiding this comment.
Relative compile database paths are joined twice.
For relative clang_params.database, the code builds <repo_root>/<db_path>/<db_path>/compile_commands.json, so DB parsing is skipped unexpectedly and tidy filename mapping can degrade.
Suggested fix
- if let Some(db_path) = &clang_params.database
- && let Ok(db_str) = fs::read(
- if db_path.is_relative() {
- clang_params.repo_root.join(db_path)
- } else {
- db_path.to_path_buf()
- }
- .join(db_path)
- .join("compile_commands.json"),
- )
+ if let Some(db_path) = &clang_params.database
+ && let Ok(db_str) = fs::read(
+ (if db_path.is_relative() {
+ clang_params.repo_root.join(db_path)
+ } else {
+ db_path.to_path_buf()
+ })
+ .join("compile_commands.json"),
+ )🤖 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/mod.rs` around lines 143 - 151, The code builds
the compile_commands.json path by joining db_path twice when
db_path.is_relative() (resulting in
<repo_root>/<db_path>/<db_path>/compile_commands.json); fix this in the fs::read
call in clang_tools/mod.rs by first computing a single base path (let base = if
db_path.is_relative() { clang_params.repo_root.join(db_path) } else {
db_path.to_path_buf() }) and then calling base.join("compile_commands.json")
(remove the extra .join(db_path)), ensuring the code that reads/opens the DB
uses that corrected base path.
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.
ref:
os.chdir()in production code cpp-linter#144--repo-rootis not the repo root path cpp-linter#146Let cpp-linter stay in working directory. Instead of changing into the given
--repo-root, this patch uses the value as a scope of file system focus.--repo-root.--repo-root--repo-rootvalue.--repo-rootvalue has always been expected as a relative path, but this should also work if given an absolute path too.Summary by CodeRabbit
Bug Fixes
Refactor