Fix Graphify audit findings - #414
Conversation
|
Warning Review limit reached
More reviews will be available in 37 minutes. 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 selected for processing (6)
📝 WalkthroughWalkthroughCLI filtering arguments now propagate to all analyzers via a new ChangesAnalyzer Dispatch and Path Resolution
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ 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 |
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
0f13828 to
b2b73e7
Compare
|
✅ Created PR with unit tests: #415 |
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
✅ Created PR with unit tests: #416 |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
b2b73e7 to
ca449cb
Compare
Omen AnalysisDiff Risk
Risk Factors
Recommendations
Repository Health
Score Components
Tips for AI agentsUse these commands to investigate and improve low-scoring areas. Run full analysis: omen -f json score # health score with component breakdown
omen -f json diff # PR risk analysis
omen hotspot # high-churn + high-complexity filesCoupling (score: 53.110891089108904) -- needs attention omen graph && omen smellsBreak cyclic dependencies by introducing interfaces or extracting shared types. Reduce fan-out from hub modules by splitting responsibilities. Duplication (score: 41.962757623237316) -- needs attention omen clonesLook for Type-1 (exact) and Type-2 (renamed) clones. Extract shared logic into reusable functions or modules. Prioritize clones in high-churn files. General workflow for improving scores:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/semantic/sync.rs (1)
73-96:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPath format:
FileSet::files()is relative, so sync removal matching is consistent.
src/core/file_set.rsstoresFileSet.filesasowned.strip_prefix(root)andtest_file_set_stores_relative_pathsasserts everyfile_set.files()entry ispath.is_relative(), so thePathBuf::from(indexed_path)vscurrent_filescomparison atsrc/semantic/sync.rsis aligned on “relative vs absolute”.- Still ensure
sync.rsstripsindexed_pathusing the sameroot_paththatFileSetuses when generatingcurrent_files.🤖 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 `@src/semantic/sync.rs` around lines 73 - 96, The removal check compares indexed_path (possibly absolute) to current_files (relative), so normalize indexed_path the same way FileSet::files() does: derive a relative path by stripping root_path (use root_path.strip_prefix or PathBuf::strip_prefix) and compare that rel_indexed to current_files; still call self.cache.remove_file(indexed_path) with the original indexed identifier when removing and keep use of stats.removed and functions like check_file_changed and current_files unchanged.
🧹 Nitpick comments (2)
.gitignore (1)
61-63: ⚡ Quick winAnchor all patterns to root for consistency.
Only
/graph.jsonis root-anchored.graphify-out/andGRAPH_REPORT.mdwill match anywhere. Graphify generates at root; anchor all three.📌 Root-anchor all patterns
-graphify-out/ -GRAPH_REPORT.md +/graphify-out/ +/GRAPH_REPORT.md /graph.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 @.gitignore around lines 61 - 63, Update the .gitignore patterns so all entries are anchored to the repository root: change the unanchored patterns "graphify-out/" and "GRAPH_REPORT.md" to root-anchored equivalents (like "/graphify-out/" and "/GRAPH_REPORT.md") to match the already-root-anchored "/graph.json" and ensure these generated files are ignored only at the repo root.src/semantic/sync.rs (1)
451-457: ⚡ Quick winAdd a second
sync()to lock incremental behavior.One sync passes whether paths are relative or absolute. A re-sync would catch the
rel_pathmismatch described above (should reportindexed == 0).💚 Proposed addition
assert_eq!(stats.checked, 1); assert_eq!(stats.indexed, 1); assert_eq!(stats.errors, 0); assert_eq!(cache.symbol_count().unwrap(), 1); + + // Re-sync: nothing changed, so no file should be re-indexed. + let stats2 = sync.sync(&file_set, temp.path()).unwrap(); + assert_eq!(stats2.checked, 1); + assert_eq!(stats2.indexed, 0); + assert_eq!(stats2.removed, 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 `@src/semantic/sync.rs` around lines 451 - 457, Add a second call to sync.sync(...) after the first to exercise incremental re-sync behavior: call sync.sync(&file_set, temp.path()) a second time, capture the returned stats, and assert that the incremental run reports checked == 1, indexed == 0 (no re-indexing due to rel_path mismatch), and errors == 0 while cache.symbol_count() remains 1; this should follow the existing symbols (sync.sync, file_set, temp.path(), stats.indexed, cache.symbol_count) so the test fails if relative-vs-absolute rel_path handling is incorrect.
🤖 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.
Outside diff comments:
In `@src/semantic/sync.rs`:
- Around line 73-96: The removal check compares indexed_path (possibly absolute)
to current_files (relative), so normalize indexed_path the same way
FileSet::files() does: derive a relative path by stripping root_path (use
root_path.strip_prefix or PathBuf::strip_prefix) and compare that rel_indexed to
current_files; still call self.cache.remove_file(indexed_path) with the original
indexed identifier when removing and keep use of stats.removed and functions
like check_file_changed and current_files unchanged.
---
Nitpick comments:
In @.gitignore:
- Around line 61-63: Update the .gitignore patterns so all entries are anchored
to the repository root: change the unanchored patterns "graphify-out/" and
"GRAPH_REPORT.md" to root-anchored equivalents (like "/graphify-out/" and
"/GRAPH_REPORT.md") to match the already-root-anchored "/graph.json" and ensure
these generated files are ignored only at the repo root.
In `@src/semantic/sync.rs`:
- Around line 451-457: Add a second call to sync.sync(...) after the first to
exercise incremental re-sync behavior: call sync.sync(&file_set, temp.path()) a
second time, capture the returned stats, and assert that the incremental run
reports checked == 1, indexed == 0 (no re-indexing due to rel_path mismatch),
and errors == 0 while cache.symbol_count() remains 1; this should follow the
existing symbols (sync.sync, file_set, temp.path(), stats.indexed,
cache.symbol_count) so the test fails if relative-vs-absolute rel_path handling
is incorrect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3c8b48ee-1e3c-4dcc-8fb3-66f03be640bf
📒 Files selected for processing (6)
.gitignoresrc/analyzers/complexity.rssrc/main.rssrc/mcp/mod.rssrc/semantic/sync.rstests/integration_tests.rs
ca449cb to
1cc88b9
Compare
1 participant
Summary
Findings by lens
Verification
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Chores