Fix/sp 4350 skip fh2 winnowing hpsm binary files - #147
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughVersion 0.40.0 introduces binary file detection and handling to the WfpCalculator. Binary files are now identified via Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/sdk/scanner/WfpProvider/WfpCalculator/WfpCalculator.ts (2)
295-320: Binary short-circuit logic looks correct.Threading
isBinarythrough the three fingerprint entry points and bypassing bothcalc_wfp(snippet winnowing) andcalculate_opposite_line_ending_hash(fh2=) is the right call — both are line/code-derived and have no meaning on binary content. The HPSM path also correctly returns just thefile=line wrapped intruncate_string, matching the documented behavior inCHANGELOG.md(binary files emit onlyfile=MD5).One small observation: there are no unit tests in this PR exercising the binary path through
wfp_for_content/wfp_hpsm_for_content/wfp_only_md5. Given the regression this fix targets (RangeError: Invalid array lengthon large binaries) and the three new branches, a focused test that asserts (a) nofh2=line, (b) no winnowing lines, and (c) correctfile=md5,size,pathfor a binary buffer would be valuable to lock the behavior in.Want me to draft a test fixture (e.g. a small synthetic buffer with a NUL byte) and a spec covering all three winnowing modes?
Also applies to: 427-441
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sdk/scanner/WfpProvider/WfpCalculator/WfpCalculator.ts` around lines 295 - 320, Add unit tests covering the binary path through wfp_for_content, wfp_hpsm_for_content, and wfp_only_md5: create a small synthetic binary Buffer (e.g., includes a NUL byte) and call each function with isBinary=true and a representative maxSize; assert the output contains only the file=md5,size,path line (correct MD5 and size) and does NOT contain fh2= or any winnowing/snippet lines produced by calc_wfp or HPSM; include separate assertions for each function to lock the three branches and prevent regressions like the RangeError on large binaries.
595-595: Pass the in-memory content buffer to avoid redundant file I/O.Since
contentis already loaded from disk on line 594, passingpathtoisBinaryFileSynccauses the library to synchronously open and re-read the file. Theisbinaryfile@^4.0.8library accepts either a file path or a Buffer, and inspects the same initial ~1000 bytes in both cases, making the Buffer overload functionally equivalent. Passingcontenteliminates the redundant I/O on every scanned file without changing behavior.♻️ Proposed change
- const isBinary = content.length > 0 && isBinaryFileSync(path); + const isBinary = content.length > 0 && isBinaryFileSync(content);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sdk/scanner/WfpProvider/WfpCalculator/WfpCalculator.ts` at line 595, The isBinary determination currently calls isBinaryFileSync(path) which causes redundant sync file reads even though the file bytes are already loaded into the content Buffer; update the call in the WfpCalculator (symbol: isBinary, variables: content and path, function: isBinaryFileSync) to pass the in-memory content Buffer instead of path so the library inspects the Buffer and avoids re-opening the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/sdk/scanner/WfpProvider/WfpCalculator/WfpCalculator.ts`:
- Around line 295-320: Add unit tests covering the binary path through
wfp_for_content, wfp_hpsm_for_content, and wfp_only_md5: create a small
synthetic binary Buffer (e.g., includes a NUL byte) and call each function with
isBinary=true and a representative maxSize; assert the output contains only the
file=md5,size,path line (correct MD5 and size) and does NOT contain fh2= or any
winnowing/snippet lines produced by calc_wfp or HPSM; include separate
assertions for each function to lock the three branches and prevent regressions
like the RangeError on large binaries.
- Line 595: The isBinary determination currently calls isBinaryFileSync(path)
which causes redundant sync file reads even though the file bytes are already
loaded into the content Buffer; update the call in the WfpCalculator (symbol:
isBinary, variables: content and path, function: isBinaryFileSync) to pass the
in-memory content Buffer instead of path so the library inspects the Buffer and
avoids re-opening the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 982460c3-46c0-46c3-8b52-d0dcebebd76b
📒 Files selected for processing (4)
CHANGELOG.mdpackage.jsonsrc/sdk/scanner/Scannable/ScannableItem.tssrc/sdk/scanner/WfpProvider/WfpCalculator/WfpCalculator.ts
bdd44fd to
48e9674
Compare
3 participants
Summary by CodeRabbit
Bug Fixes
Improvements
Chores