fix(core): Include binary files in directory structure output#883
fix(core): Include binary files in directory structure output#883
Conversation
According to the output message, the Repository Structure section should contain a complete list of file paths including binary files, while only excluding binary file contents from the Files section. Previously, generateOutput received safeFilePaths (which excluded binary files after security check), causing binary files to be missing from the directory structure entirely. This fix passes sortedFilePaths (all files found by searchFiles) to generateOutput instead, ensuring binary files appear in the directory structure while their contents remain excluded from output. Fixes #841
Summary of ChangesHello @yamadashy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a long-standing regression (introduced 14 months ago) that prevented binary files from appearing in the generated directory structure output. The fix ensures that the directory structure now correctly includes all files, while file contents continue to be filtered to only include safe, non-binary files, aligning with the intended behavior and documentation. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a regression where binary files were missing from the directory structure output, which contradicted the documented behavior. The issue was introduced when security features were added in v0.1.18, accidentally filtering binary files from the directory structure.
- Changes parameter passed to
generateOutputfromsafeFilePathstoallFilePaths - Ensures binary files appear in directory structure while keeping file contents properly filtered
- Resolves 14-month regression spanning v0.1.18 to v1.6.1
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe pack() function in src/core/packager.ts now calls generateOutput with allFilePaths instead of safeFilePaths. No other parts of the flow (logging, progress callbacks, writing, copying, metrics) were changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI
participant Packager as pack()
participant Generator as generateOutput()
CLI->>Packager: invoke pack()
Packager->>Packager: prepare allFilePaths, safeFilePaths
note over Packager: Changed: now uses allFilePaths
Packager->>Generator: generateOutput(allFilePaths, ...)
Generator-->>Packager: output
Packager->>Packager: write/copy/metrics unchanged
Packager-->>CLI: done
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a great fix that addresses a long-standing regression. The pull request description is excellent, clearly explaining the problem, root cause, and solution. I have one suggestion regarding test coverage to make this fix more robust against future changes.
| progressCallback('Generating output...'); | ||
| const output = await withMemoryLogging('Generate Output', () => | ||
| deps.generateOutput(rootDirs, config, processedFiles, safeFilePaths, gitDiffResult, gitLogResult), | ||
| deps.generateOutput(rootDirs, config, processedFiles, allFilePaths, gitDiffResult, gitLogResult), |
There was a problem hiding this comment.
This change correctly fixes the issue of binary files not appearing in the directory structure. However, the fix is not covered by the existing unit tests.
The current tests in tests/core/packager.test.ts use the same mock data for allFilePaths and safeFilePaths, so they would pass even with the old, incorrect code.
To ensure this regression doesn't happen again, please consider adding a new test case that specifically verifies this behavior. The test should simulate a scenario where a binary file is present, causing allFilePaths and safeFilePaths to differ, and then assert that generateOutput is called with the complete allFilePaths.
Here's a conceptual example of what the test could look like:
test('pack should pass all file paths to generateOutput for directory structure', async () => {
const allFilePaths = ['file1.txt', 'binary.bin'];
const safeFilePaths = ['file1.txt'];
const mockRawFiles = [
{ path: 'file1.txt', content: 'text' },
{ path: 'binary.bin', content: 'binary' },
];
const mockSafeRawFiles = [{ path: 'file1.txt', content: 'text' }];
const mockProcessedFiles = [{ path: 'file1.txt', content: 'processed text' }];
const mockDeps = {
// ... other necessary mocks
searchFiles: vi.fn().mockResolvedValue({ filePaths: allFilePaths }),
collectFiles: vi.fn().mockResolvedValue({ rawFiles: mockRawFiles, skippedFiles: [] }),
validateFileSafety: vi.fn().mockResolvedValue({
safeFilePaths,
safeRawFiles: mockSafeRawFiles,
suspiciousFilesResults: [],
}),
processFiles: vi.fn().mockResolvedValue(mockProcessedFiles),
generateOutput: vi.fn(),
};
await pack(['root'], createMockConfig(), vi.fn(), mockDeps);
expect(mockDeps.generateOutput).toHaveBeenCalledWith(
expect.any(Array),
expect.any(Object),
mockProcessedFiles,
allFilePaths, // Assert that all files are passed for output generation
expect.anything(),
expect.anything()
);
});
Deploying repomix with
|
| Latest commit: |
6776e02
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8d43afe3.repomix.pages.dev |
| Branch Preview URL: | https://fix-binary-skipped-structure.repomix.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #883 +/- ##
=======================================
Coverage 88.81% 88.81%
=======================================
Files 109 109
Lines 7608 7608
Branches 1436 1436
=======================================
Hits 6757 6757
Misses 851 851 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Code ReviewSummaryThis is an excellent fix for a subtle but important bug. The change correctly restores the intended behavior where binary files appear in the directory structure while their contents remain excluded from the output. Strengths
Observations
RecommendationsOptional Enhancement (not blocking): Consider adding an integration test case that explicitly verifies binary files appear in directory structure but not in content sections. However, this is not required for merging since the fix is correct and minimal, existing tests pass, and manual verification was performed. VerdictLGTM - Ready to merge! This fix correctly resolves issue #841 by ensuring the directory structure includes all files as documented, while maintaining the security filtering for file contents. Code Quality: 5/5 Great work investigating and fixing this long-standing issue! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Add comprehensive documentation about how binary files are handled in Repomix output: - Added new "Binary Files Handling" section in output.md explaining that binary file paths appear in directory structure while their contents are excluded - Updated security.md to clarify binary file handling approach - Included practical examples to help users understand the behavior This documentation update complements the fix in #883 and helps prevent user confusion about binary file handling.
Code ReviewSummaryThis PR fixes a critical regression where binary files were completely missing from the directory structure output. The fix is simple, correct, and well-documented. The change passes all existing tests and aligns with the documented behavior. ✅ Strengths
🔍 Code Qualitypackager.ts:121 deps.generateOutput(rootDirs, config, processedFiles, allFilePaths, gitDiffResult, gitLogResult)
📝 DocumentationThe new documentation in
🧪 Testing ObservationsExisting Test Coverage: expect(mockDeps.generateOutput).toHaveBeenCalledWith(
["root"],
mockConfig,
mockProcessedFiles,
mockFilePaths, // This is allFilePaths, not safeFilePaths
undefined,
undefined,
);This test would have caught the regression if it had been written to use separate Suggestion for Future Enhancement (not blocking): 🔒 Security✅ No security concerns. The fix actually maintains proper security boundaries:
⚡ Performance✅ No performance impact. 📦 Breaking Changes✅ None. This is a bug fix that restores the originally intended behavior. Recommendations
Compliance with CLAUDE.md
Overall Assessment: High quality fix. Approved! 🎉 |
Add comprehensive documentation about how binary files are handled in Repomix output: - Added new "Binary Files Handling" section in output.md explaining that binary file paths appear in directory structure while their contents are excluded - Updated security.md to clarify binary file handling approach - Included practical examples to help users understand the behavior This documentation update complements the fix in #883 and helps prevent user confusion about binary file handling.
yamadashy
force-pushed
the
fix/binary-skipped-structure
branch
from
49a32c0 to
bb88ba4
Compare
Code ReviewSummaryThis is an excellent bug fix that addresses a regression where binary files were not appearing in the directory structure section of the output. The fix is minimal, precise, and well-documented. ✅ Strengths1. Code Quality
2. Documentation
3. Root Cause Analysis
🔍 Observations1. Test Coverage
2. Code Comment // Use allFilePaths (not safeFilePaths) so directory structure includes binary files
const output = await withMemoryLogging('Generate Output', () =>
deps.generateOutput(rootDirs, config, processedFiles, allFilePaths, gitDiffResult, gitLogResult),
);This would make the intent explicit and help prevent similar regressions. 3. Documentation Example Accuracy The test fixtures use 🎯 Performance & Security
📝 Conventional CommitsThe commit message
🏆 VerdictThis is a high-quality bug fix that:
Recommendation: ✅ APPROVE with the optional suggestion to add an integration test with binary files to prevent future regressions. Great work! 🚀 |
…on.md Based on the comprehensive documentation in README.md, this update significantly expands the Include and Ignore Patterns sections: - Added new "Include Patterns" section with glob pattern examples and configuration examples - Expanded "Ignore Patterns" section with detailed explanations of all four ignore methods (.gitignore, default patterns, .repomixignore, custom patterns) - Added clear priority order for ignore patterns - Included cross-reference to Binary Files Handling section - Maintained consistency with README.md documentation This helps users find complete information on the website without needing to refer back to the README.
Code ReviewSummaryThis is an excellent fix for a 14-month regression where binary files were completely missing from the directory structure output. The fix is minimal, surgical, and well-documented. ✅ Strengths
🔍 Code Qualitypackager.ts (src/core/packager.ts:121)
Flow verification:
🧪 Test Coverage AnalysisThe existing test at expect(mockDeps.generateOutput).toHaveBeenCalledWith(
['root'],
mockConfig,
mockProcessedFiles, // filtered/safe content
mockFilePaths, // all file paths for structure
undefined,
undefined,
);Recommendation: Consider adding a specific integration test that:
Example location: Could you verify that test file covers this scenario adequately? 📝 Documentation QualityThe documentation updates are excellent:
🔒 Security Considerations✅ No security concerns - The fix actually improves the user experience while maintaining security:
⚡ Performance Considerations✅ No performance impact - The 🎯 Best Practices AdherenceFollowing CLAUDE.md guidelines:
📋 Suggestions
✨ ConclusionThis PR is ready to merge. It's a well-researched, minimal fix with excellent documentation. The change is correct, safe, and restores the expected behavior documented in the output messages. Recommendation: ✅ APPROVE Great work investigating and fixing this regression! 🎉 |
Applied the same documentation updates from English version to all other language versions (8 languages: ja, de, es, fr, ko, zh-cn, zh-tw, pt-br): **security.md changes:** - Updated binary file description from "Binary File Exclusion" to "Binary File Handling" with clarification that paths are listed in directory structure **configuration.md changes:** - Added new "Include Patterns" section with glob pattern examples - Significantly expanded "Ignore Patterns" section with detailed explanations of all 4 ignore methods - Added priority order for ignore patterns - Added "Binary Files Handling" section with comprehensive examples - Added cross-references between sections Note: Indonesian (id), Vietnamese (vi), and Hindi (hi) versions were not updated as their document structure differs significantly from the English version and requires separate review. All changes have been translated appropriately for each language while maintaining consistency with the English documentation. Website builds successfully with all changes.
Review SummaryThis PR successfully fixes a critical regression where binary files were not appearing in the directory structure output. The fix is straightforward, well-tested, and aligns with the documented behavior. Overall, this is a high-quality PR with excellent documentation and clear explanation of the root cause. ✅ Strengths1. Code Quality
2. Excellent Documentation
3. Test Coverage
🔍 Observations & Suggestions1. Test Coverage Enhancement (Minor Suggestion)While the existing unit test validates the correct parameter is passed, consider adding an integration test that explicitly verifies:
Suggested test location: Example test scenario: test('should include binary files in directory structure but not in file contents', async () => {
// Setup: Create a test directory with binary files
// Assert: Binary file paths appear in directory structure
// Assert: Binary file contents do not appear in output
});This would provide stronger regression protection and serve as living documentation of the expected behavior. 2. Documentation Completeness (Question)The documentation updates are comprehensive, but I noticed the Korean translation in 3. Variable Naming Clarity (Observation)The variable names are clear:
This naming convention makes the fix self-documenting. Well done! 🛡️ Security ReviewNo security concerns identified. The change actually improves security transparency by ensuring users can see all files in their repository structure, including binary files that are excluded from content output. This helps users verify nothing unexpected is being missed. ⚡ Performance ReviewNo performance impact. The change simply passes a different array reference to 🐛 Potential IssuesNone identified. The fix is sound and addresses the root cause correctly. 📋 Checklist Items
🎯 RecommendationAPPROVE with optional enhancement suggestion This PR is ready to merge. The optional integration test suggestion is a nice-to-have for future robustness but not blocking. Great work on the thorough root cause analysis and comprehensive documentation updates! 🚀 |
Summary
Fixes #841
This PR fixes a regression where binary files were not appearing in the directory structure section of the output, despite the documentation stating they should be included.
Background
According to the output message:
However, binary files were completely missing from the directory structure.
Root Cause
This regression was introduced in commit 3ff8392 (v0.1.18, August 2024) when implementing the security check feature. The implementation mistakenly passed
safeFilePaths(which excludes binary and suspicious files after security checks) togenerateOutput, when it should have passedallFilePathsto include all files in the directory structure.Timeline:
Changes
Changed
packager.tsto passallFilePathsinstead ofsafeFilePathstogenerateOutput:Testing
.jar,.png, etc.) now appear in directory structureChecklist
npm run testnpm run lint