Error Handling and Test Coverage Improvements #171
Conversation
|
|
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several changes across various files, primarily focusing on updating script extensions from Changes
Possibly related PRs
Warning Rate limit exceeded@yamadashy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 29 seconds before requesting another review. ⌛ 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. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #171 +/- ##
===========================================
+ Coverage 76.84% 91.83% +14.99%
===========================================
Files 35 34 -1
Lines 1861 1861
Branches 317 428 +111
===========================================
+ Hits 1430 1709 +279
+ Misses 431 152 -279 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
yamadashy
force-pushed
the
hotfix/error-handling
branch
from
5b10320 to
005e470
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
bin/repomix.cjs (3)
4-15: Consider enhancing version check robustnessWhile the version check works, it could be more maintainable and robust.
Consider these improvements:
+'use strict'; + +const MIN_NODE_VERSION = 16; const nodeVersion = process.versions.node; -const [major] = nodeVersion.split('.').map(Number); +const major = parseInt(process.versions.node.split('.')[0], 10); const EXIT_CODES = { SUCCESS: 0, ERROR: 1, }; -if (major < 16) { +if (Number.isNaN(major) || major < MIN_NODE_VERSION) { console.error(`Repomix requires Node.js version 16 or higher. Current version: ${nodeVersion}\n`); process.exit(EXIT_CODES.ERROR); }
17-27: Consider adding error details to logsThe error handling for uncaught exceptions and unhandled rejections could provide more detailed information.
Consider this enhancement:
process.on('uncaughtException', (error) => { - console.error('Uncaught Exception:', error); + console.error('Uncaught Exception:', { + name: error.name, + message: error.message, + stack: error.stack, + }); process.exit(EXIT_CODES.ERROR); }); process.on('unhandledRejection', (reason) => { - console.error('Unhandled Promise Rejection:', reason); + const error = reason instanceof Error ? reason : new Error(String(reason)); + console.error('Unhandled Promise Rejection:', { + name: error.name, + message: error.message, + stack: error.stack, + }); process.exit(EXIT_CODES.ERROR); });
1-51: Overall implementation aligns well with PR objectivesThe enhanced error handling and process management implementation is solid and meets the requirements. The code introduces comprehensive error handlers, graceful shutdown handling, and Node.js version validation as specified in the PR objectives.
Consider documenting the error handling behavior and exit codes in the project's documentation to help users and developers understand how the CLI handles different error scenarios.
package.json (1)
Line range hint
47-71: Consider adding error handling specific dependenciesGiven the PR's focus on enhanced error handling and graceful shutdown, consider adding these helpful packages:
process-signal-handlerfor better SIGINT/SIGTERM handlingpretty-errorfor improved error formatting.github/workflows/ci.yml (1)
143-143: Consider adding error scenario tests.While the command change is correct, the workflow could benefit from additional test cases to validate the new error handling features introduced in this PR.
Consider adding test steps that:
- Trigger uncaught exceptions to verify the new error handlers
- Send SIGINT/SIGTERM signals to verify graceful shutdown
- Test with an incompatible Node.js version to verify version check
Example addition:
- run: node bin/repomix.cjs + # Test error handling + - name: Test error handling + run: | + # Test uncaught exception handling + node -e "process.emit('uncaughtException', new Error('Test error'))" bin/repomix.cjs || exit 0 + # Test graceful shutdown + (node bin/repomix.cjs & pid=$!; sleep 2; kill -SIGINT $pid) || exit 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
.github/workflows/ci.yml(1 hunks)bin/repomix.cjs(1 hunks)bin/repomix.js(0 hunks)package.json(1 hunks)src/cli/cliRun.ts(0 hunks)
💤 Files with no reviewable changes (2)
- bin/repomix.js
- src/cli/cliRun.ts
🔇 Additional comments (3)
package.json (2)
Line range hint 89-92: LGTM: Node.js version requirement is properly specified
The engines field correctly specifies Node.js >= 16.0.0, which aligns with the PR's objective of implementing version checking.
22-22: Verify consistent .cjs extension usage across the codebase
The change from repomix to repomix.cjs aligns with the PR objectives. Let's verify this change is consistent across the codebase.
✅ Verification successful
Extension change to .cjs is consistently implemented
The verification shows that:
- The
bin/repomix.cjsfile exists and has correct permissions - No remnants of old
.jsor.mjsfiles exist in the codebase - Package.json's bin field correctly points to
./bin/repomix.cjs - No other references to old file extensions were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of repomix.cjs and no remnants of old file
# Check for any remaining references to the old file
rg "repomix\.js" --type-not json
# Verify the new file exists
if [ ! -f "bin/repomix.cjs" ]; then
echo "Error: bin/repomix.cjs does not exist"
exit 1
fi
Length of output: 67
Script:
#!/bin/bash
# Let's do a more comprehensive check for file references and existence
# 1. Check for any references to repomix with various extensions
rg "repomix\.(js|cjs|mjs)" --type-not json
# 2. List all files in bin directory
ls -la bin/
# 3. Check package.json bin field
cat package.json | jq .bin
# 4. Find all JavaScript files in bin directory
fd -e js -e cjs -e mjs . bin/
Length of output: 327
.github/workflows/ci.yml (1)
143-143: LGTM! Command update aligns with enhanced error handling objectives.
The change from repomix to repomix.cjs reflects the transition to a more robust CLI implementation with improved error handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (24)
src/core/file/permissionCheck.ts (2)
Line range hint
27-90: Consider optimizing permission checks and enhancing error details.While the implementation is functionally correct, there are opportunities for improvement:
- The separate
fs.accesscalls could be combined for better performance- Error messages could be more specific about which permission failed
Consider this optimization:
- try { - await fs.access(dirPath, constants.R_OK); - details.read = true; - } catch {} - - try { - await fs.access(dirPath, constants.W_OK); - details.write = true; - } catch {} - - try { - await fs.access(dirPath, constants.X_OK); - details.execute = true; - } catch {} + const permissions = [ + { flag: constants.R_OK, key: 'read' }, + { flag: constants.W_OK, key: 'write' }, + { flag: constants.X_OK, key: 'execute' } + ]; + + await Promise.all(permissions.map(async ({ flag, key }) => { + try { + await fs.access(dirPath, flag); + details[key] = true; + } catch (e) { + logger.debug(`Failed to check ${key} permission:`, e); + } + }));
Line range hint
92-112: Consider expanding platform-specific permission guidance.The macOS instructions are detailed and helpful. Consider adding similar detailed guidance for other major platforms (Windows, Linux) to improve user experience across all environments.
Example enhancement:
const getMacOSPermissionMessage = (dirPath: string, errorCode?: string): string => { - if (platform() === 'darwin') { - return `Permission denied: Cannot access '${dirPath}', error code: ${errorCode}. + const platformSpecificGuide = { + darwin: `Permission denied: Cannot access '${dirPath}', error code: ${errorCode}. This error often occurs when macOS security restrictions prevent access to the directory. To fix this: 1. Open System Settings 2. Navigate to Privacy & Security > Files and Folders 3. Find your terminal app (Terminal.app, iTerm2, VS Code, etc.) 4. Grant necessary folder access permissions If your terminal app is not listed: - Try running repomix command again - When prompted by macOS, click "Allow" - Restart your terminal app if needed`, + win32: `Permission denied: Cannot access '${dirPath}', error code: ${errorCode}. + +To fix this: +1. Right-click the folder +2. Select Properties > Security +3. Click Edit to modify permissions +4. Ensure your user account has necessary permissions`, + linux: `Permission denied: Cannot access '${dirPath}', error code: ${errorCode}. + +To fix this: +1. Check folder permissions: ls -la "${dirPath}" +2. Modify permissions if needed: chmod u+rwx "${dirPath}" +3. Or add your user to the appropriate group` + }; + + return platformSpecificGuide[platform()] || `Permission denied: Cannot access '${dirPath}'`; - } - - return `Permission denied: Cannot access '${dirPath}'`; };src/cli/cliRun.ts (3)
Line range hint
29-53: LGTM! Consider additional error handling improvements.The try-catch block with
handleErroris a good improvement for error handling. However, to fully align with the PR objectives, consider adding handlers for unhandled promise rejections and process signals.Consider adding these handlers at the start of the
runfunction:export const run = async () => { + // Handle unhandled promise rejections + process.on('unhandledRejection', (reason) => { + handleError(reason instanceof Error ? reason : new Error(String(reason))); + }); + + // Handle process signals + process.on('SIGINT', () => { + logger.log('\nGracefully shutting down...'); + process.exit(0); + }); + try {
Line range hint
41-41: Add validation for numeric options.The
--top-files-lenoption usesNumber.parseIntbut lacks bounds checking.Consider adding validation:
-.option('--top-files-len <number>', 'specify the number of top files to display', Number.parseInt) +.option('--top-files-len <number>', 'specify the number of top files to display (min: 1)', (value) => { + const parsed = Number.parseInt(value); + if (isNaN(parsed) || parsed < 1) { + throw new Error('--top-files-len must be a positive number'); + } + return parsed; +})
Line range hint
55-76: LGTM! Consider optimizing version display.The function structure is clean with good separation of concerns. However, the version display could be optimized.
Consider moving the version display to the default action to avoid showing it during init/remote operations:
export const executeAction = async (directory: string, cwd: string, options: CliOptions) => { logger.setVerbose(options.verbose || false); if (options.version) { await runVersionAction(); return; } - const version = await getVersion(); - logger.log(pc.dim(`\n📦 Repomix v${version}\n`)); - if (options.init) { await runInitAction(cwd, options.global || false); return; } if (options.remote) { await runRemoteAction(options.remote, options); return; } + const version = await getVersion(); + logger.log(pc.dim(`\n📦 Repomix v${version}\n`)); await runDefaultAction(directory, cwd, options); };tests/cli/cliRun.test.ts (3)
25-33: Consider adding error scenarios to commander mockWhile the commander mock covers the happy path, consider adding test cases for common CLI error scenarios:
- Invalid command syntax
- Unknown options
- Parse errors
Example enhancement:
parseAsync: vi.fn().mockResolvedValue(undefined), +// Add error scenarios +parseAsyncWithError: vi.fn().mockRejectedValue(new Error('Invalid command')), +unknownOption: vi.fn().mockImplementation(() => { + throw new Error('Unknown option: --invalid'); +}),
35-38: Consider adding explicit return types to action mocksTo improve type safety, consider adding explicit return type annotations to the mocked action functions.
Example enhancement:
vi.mock('../../src/cli/actions/defaultAction', () => ({ runDefaultAction: vi.fn().mockResolvedValue<ActionResult>({ // typed mock response }) }));
40-78: Consider extracting mock data to fixturesThe mock data in the beforeEach hook is quite extensive. Consider extracting it to separate fixture files for better maintainability and reuse.
Example structure:
// __fixtures__/mockConfig.ts export const defaultConfig: RepomixConfigMerged = { cwd: process.cwd(), output: { // ... current config }, // ... rest of the config }; // __fixtures__/mockPackResult.ts export const defaultPackResult: PackResult = { totalFiles: 0, // ... rest of the pack result };tests/core/tokenCount/tokenCount.test.ts (5)
14-17: Consider improving type safety for mock encoder.The mock encoder type could be more strictly typed to match the actual
Tiktokeninterface.Consider this improvement:
- let mockEncoder: { - encode: Mock; - free: Mock; - }; + let mockEncoder: { + encode: Mock<(text: string) => number[]>; + free: Mock<() => void>; + };
44-44: Consider using more realistic mock token values.The mock token values (e.g.,
[123, 456, 789],[1, 2, 3]) could be more representative of actual tiktoken encoding. This would make the tests more valuable for catching potential issues with token counting.For example, for the text "Hello, world!", a more realistic mock might be:
- const mockTokens = [123, 456, 789]; // Example token IDs + const mockTokens = [15339, 11, 995, 0]; // Actual cl100k_base tokensAlso applies to: 75-76, 64-65
128-137: Consider adding edge cases for maximum input length.While testing long text is good, consider adding tests for:
- Text at or near the maximum allowed input length
- Text with varying ratios of tokens to characters
Example addition:
test('should handle text at maximum allowed length', () => { const maxLength = 100000; // Adjust based on actual limits const text = 'a'.repeat(maxLength); const mockTokens = Array(maxLength / 4).fill(1); // Approximate token ratio mockEncoder.encode.mockReturnValue(mockTokens); const count = tokenCounter.countTokens(text); expect(count).toBe(maxLength / 4); expect(mockEncoder.encode).toHaveBeenCalledWith(text); });
139-161: Consider testing additional error scenarios.The error handling tests could be expanded to cover:
- Different types of encoding errors
- Memory allocation errors
- State verification after errors
Example additions:
test('should handle memory allocation errors', () => { const error = new Error('Memory allocation failed'); mockEncoder.encode.mockImplementation(() => { throw error; }); const count = tokenCounter.countTokens('test'); expect(count).toBe(0); expect(mockEncoder.free).toHaveBeenCalled(); expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Memory allocation failed')); });
163-166: Consider testing additional cleanup scenarios.The cleanup test could be expanded to verify:
- Behavior when attempting to use the counter after freeing
- Multiple calls to free()
Example additions:
test('should handle calls after being freed', () => { tokenCounter.free(); expect(() => tokenCounter.countTokens('test')).toThrow(); }); test('should handle multiple free calls gracefully', () => { tokenCounter.free(); tokenCounter.free(); // Should not throw expect(mockEncoder.free).toHaveBeenCalledTimes(2); });src/cli/actions/initAction.ts (3)
Line range hint
41-124: Add error handling for file write operations.While the function handles file existence checks well, it lacks error handling for the file write operations. This could lead to unhandled exceptions if the write operation fails due to permissions or disk space issues.
Consider wrapping the file operations in try-catch:
- await fs.mkdir(path.dirname(configPath), { recursive: true }); - await fs.writeFile(configPath, JSON.stringify(config, null, 2)); + try { + await fs.mkdir(path.dirname(configPath), { recursive: true }); + await fs.writeFile(configPath, JSON.stringify(config, null, 2)); + } catch (error) { + prompts.log.error(`Failed to create config file: ${error.message}`); + return false; + }
Line range hint
126-176: Add error handling for file write operations and reduce code duplication.Similar to
createConfigFile, this function lacks error handling for file write operations. Additionally, there's significant duplication in file handling logic between this andcreateConfigFile.
- Add error handling for file operations:
- await fs.writeFile(ignorePath, defaultIgnoreContent); + try { + await fs.writeFile(ignorePath, defaultIgnoreContent); + } catch (error) { + prompts.log.error(`Failed to create ignore file: ${error.message}`); + return false; + }
- Consider extracting common file handling logic into a utility function:
async function writeFileWithConfirmation( filePath: string, content: string, options: { fileName: string, skipMessage?: string, overwriteMessage?: string, } ): Promise<boolean> { // Common file existence check and write logic }
Line range hint
14-17: Improve process exit handling and error types.The current implementation has two areas for improvement:
- Direct use of
process.exit()might not allow for cleanup operations.- Generic error handling could be more specific.
Consider these improvements:
- Replace direct process exit with a cleanup handler:
-const onCancelOperation = () => { - prompts.cancel('Initialization cancelled.'); - process.exit(0); -}; +const onCancelOperation = async () => { + prompts.cancel('Initialization cancelled.'); + await cleanup(); // Add cleanup function + process.exitCode = 0; +};
- Add specific error types for better error handling:
class ConfigurationError extends Error { constructor(message: string) { super(message); this.name = 'ConfigurationError'; } }tests/core/output/outputStyles/markdownStyle.test.ts (3)
6-86: Consider adding more test cases for edge cases and error scenarios.The test suite covers the happy path well but could be enhanced with additional test cases:
- Error cases (e.g., invalid template data)
- Empty/null values in the data object
- Special characters in the input text
- Very large content strings
- Unicode characters in the input
Example additional test:
test('should handle special characters and empty values', () => { const template = getMarkdownTemplate(); const compiledTemplate = Handlebars.compile(template); const data = { headerText: '### Special & < > " \' chars', summaryPurpose: '', summaryFileFormat: null, processedFiles: [] }; const result = compiledTemplate(data); expect(result).toContain('### Special & < > " ' chars'); expect(result).not.toContain('undefined'); expect(result).not.toContain('null'); });
89-200: Enhance file extension tests with additional edge cases.The test suite is comprehensive but could be strengthened with additional test cases:
- Case sensitivity tests (e.g.,
.JS,.Ts)- Multiple dots in filenames (e.g.,
file.test.js,file.spec.tsx)- Platform-specific paths (Windows backslashes, absolute paths)
- Special characters in filenames
- Non-ASCII characters in filenames
Example additional test cases:
test('should handle case-insensitive extensions', () => { expect(getExtension('file.JS')).toBe('javascript'); expect(getExtension('file.Ts')).toBe('typescript'); }); test('should handle complex filenames', () => { expect(getExtension('file.test.js')).toBe('javascript'); expect(getExtension('file.spec.tsx')).toBe('typescript'); expect(getExtension('C:\\path\\to\\file.js')).toBe('javascript'); expect(getExtension('file-name.with.dots.py')).toBe('python'); expect(getExtension('文件.js')).toBe('javascript'); });
91-94: Consider decoupling the file extension helper from Handlebars.The
getFileExtensionhelper is currently tightly coupled with Handlebars. Consider extracting it into a standalone utility function that can be:
- Used independently of Handlebars
- Easier to test without the Handlebars dependency
- More reusable across the codebase
Example refactor:
// fileUtils.ts export function getFileExtension(filePath: string): string { // Implementation here } // markdownStyle.ts import { getFileExtension } from './fileUtils'; Handlebars.registerHelper('getFileExtension', getFileExtension); // markdownStyle.test.ts import { getFileExtension } from './fileUtils';tests/core/file/permissionCheck.test.ts (5)
1-16: Consider adding type assertions for mocked functions.While the setup is well-structured, consider adding type assertions to ensure type safety for mocked functions:
- vi.mocked(platform).mockReturnValue('linux'); + vi.mocked(platform as () => string).mockReturnValue('linux');
18-66: Consider testing permission check order.The successful cases are well-tested, but consider adding a test to verify that permission checks are performed in the expected order (read → write → execute).
test('should perform permission checks in correct order', async () => { const checkOrder: string[] = []; vi.mocked(fs.readdir).mockResolvedValue([]); vi.mocked(fs.access).mockImplementation(async (path, mode) => { if (mode === constants.R_OK) checkOrder.push('read'); if (mode === constants.W_OK) checkOrder.push('write'); if (mode === constants.X_OK) checkOrder.push('execute'); return Promise.resolve(undefined); }); await checkDirectoryPermissions(testDirPath); expect(checkOrder).toEqual(['read', 'write', 'execute']); });
68-122: LGTM! Consider verifying error stack traces.The error cases are well-tested. For completeness, consider adding assertions to verify that error stack traces are preserved:
expect(result.error?.stack).toBeDefined();
124-155: Consider testing all major platforms.While macOS and Windows are tested, consider adding test cases for other major platforms (Linux variants) to ensure comprehensive platform-specific behavior coverage.
test.each([ ['linux', 'standard'], ['aix', 'standard'], ['freebsd', 'standard'], ['openbsd', 'standard'], ['sunos', 'standard'], ['android', 'standard'], ])('should handle %s platform with %s message', async (os, messageType) => { vi.mocked(platform).mockReturnValue(os); // ... test implementation });
186-241: Consider testing concurrent permission checks.The edge cases are well-tested. Consider adding a test for concurrent permission checks to ensure thread safety:
test('should handle concurrent permission checks', async () => { vi.mocked(fs.readdir).mockResolvedValue([]); vi.mocked(fs.access).mockImplementation(async () => { await new Promise(resolve => setTimeout(resolve, Math.random() * 10)); return Promise.resolve(undefined); }); const results = await Promise.all([ checkDirectoryPermissions(testDirPath), checkDirectoryPermissions(testDirPath), checkDirectoryPermissions(testDirPath) ]); results.forEach(result => { expect(result.hasPermission).toBe(true); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
bin/repomix.cjs(1 hunks)biome.json(1 hunks)src/cli/actions/initAction.ts(3 hunks)src/cli/cliRun.ts(2 hunks)src/core/file/permissionCheck.ts(3 hunks)tests/cli/cliRun.test.ts(1 hunks)tests/core/file/permissionCheck.test.ts(1 hunks)tests/core/output/outputStyles/markdownStyle.test.ts(1 hunks)tests/core/tokenCount/tokenCount.test.ts(1 hunks)vitest.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/repomix.cjs
🔇 Additional comments (9)
vitest.config.ts (1)
10-10: Verify the rationale for excluding index.ts from coverage.
While it's common to exclude entry points from coverage as they often contain bootstrapping code, given that this PR focuses on enhancing error handling, we should ensure that critical error handling paths are still tested elsewhere.
Let's verify the content of the excluded file and related test coverage:
✅ Verification successful
The initial results show that src/index.ts is just a thin export layer, and there are comprehensive error handling tests across the codebase. However, let's verify the specific CLI error handling coverage since that's the focus of this PR.
Excluding src/index.ts from coverage is appropriate
The exclusion is justified because:
- src/index.ts is a pure export file with no logic, only re-exporting from other modules
- Error handling is properly tested in the actual implementation files:
- CLI error handling is covered in tests/cli/cliRun.test.ts
- The main error handling logic is in handleError() which is imported from shared/errorHandle.js
- Multiple test files across the codebase verify error scenarios
- The critical paths are tested in their respective implementation files rather than the entry point
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze the excluded file and related test coverage
# Check the content of src/index.ts
echo "Content of src/index.ts:"
cat src/index.ts
# Look for related test files
echo -e "\nRelated test files:"
fd -e test.ts -e spec.ts index
# Check for error handling tests
echo -e "\nError handling related tests:"
rg -l "error|exception|reject" tests/
Length of output: 771
Script:
#!/bin/bash
# Let's check the error handling tests in cliRun.test.ts since it's directly related to CLI functionality
echo "Content of tests/cli/cliRun.test.ts:"
cat tests/cli/cliRun.test.ts
# Also check the implementation of CLI run to understand what's being tested
echo -e "\nContent of src/cli/cliRun.ts:"
cat src/cli/cliRun.ts
Length of output: 7096
biome.json (1)
5-5: LGTM! Addition of bin directory to Biome configuration.
The inclusion of "./bin/**" is appropriate as it ensures proper formatting and linting of the new CLI entry point files.
Let's verify the existence and content of files in the bin directory:
✅ Verification successful
✓ Verified: bin directory inclusion in Biome config is appropriate
The verification confirms that:
bin/repomix.cjsexists and contains the CLI entry point code- The file implements Node.js version check, error handling, and graceful shutdown as per PR objectives
- The
.cjsextension is used as mentioned in the PR summary
Including "./bin/**" in Biome's configuration is necessary to ensure proper formatting and linting of this CLI entry point file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and type of files in the bin directory
# Expected: Should find repomix.cjs and potentially other CLI-related files
# Check for files in bin directory
fd --type f . bin/
# Show file types to confirm they are text/script files
fd --type f . bin/ --exec file {}
Length of output: 110
Script:
#!/bin/bash
# Let's verify the content and presence of bin/repomix.cjs
cat bin/repomix.cjs
# Also check if there are any other references to bin directory
rg -l "bin/repomix" --type-not json
Length of output: 1303
src/core/file/permissionCheck.ts (1)
Line range hint 27-112: Overall implementation is robust and well-structured.
The permission checking logic is thorough and the error handling is comprehensive. The conversion to arrow functions maintains the same functionality while modernizing the syntax.
tests/cli/cliRun.test.ts (2)
11-23: LGTM! Comprehensive logger mock setup
The logger mock is well-implemented with all necessary logging methods and follows Vitest best practices.
1-10: Verify the use of .js extensions in import paths
While the imports are well-organized, the .js extensions in import paths (e.g., defaultAction.js) should be verified as this is a TypeScript file. Some TypeScript configurations might require .ts extensions or no extensions at all.
✅ Verification successful
Using .js extensions in imports is correct for this project
The project is configured with "moduleResolution": "NodeNext" in tsconfig.json, which requires using .js extensions in import paths even for TypeScript files. This is the correct approach for Node.js ESM packages. The ripgrep results show this pattern is consistently used throughout the entire codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check TypeScript configuration and other import patterns in the project
# Check tsconfig.json for module resolution settings
cat tsconfig.json | jq '.compilerOptions | select(.moduleResolution != null)'
# Look for other import patterns in TypeScript files
rg -t ts "from '[^']+\.(js|ts)'" --stats
Length of output: 14702
tests/core/tokenCount/tokenCount.test.ts (1)
1-166: Overall, the test implementation is robust and well-structured!
The test suite provides comprehensive coverage of the TokenCounter functionality, including error handling, which aligns well with the PR's objectives. The suggested improvements are enhancements that could be addressed in future iterations.
tests/core/output/outputStyles/markdownStyle.test.ts (1)
1-3: Verify the .js extension in the import path.
The import path ends with .js while we're in a TypeScript file. This might need verification to ensure it aligns with the project's module resolution configuration.
✅ Verification successful
Let me gather more information about the module resolution configuration to make a definitive decision.
The .js extension in imports is correct and follows the project's configuration
The project is properly configured as an ES module package with TypeScript:
- Package.json has
"type": "module" - TSConfig uses
"moduleResolution": "NodeNext"and"module": "NodeNext" - The
.jsextension in imports is consistently used throughout the codebase for TypeScript files
This is the correct setup for ES modules in TypeScript where import specifiers must include the .js extension even when importing .ts files, as they will be compiled to .js files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the module resolution configuration and verify the existence of the imported file
# Test 1: Check if the imported file exists with .js extension
fd -e js "markdownStyle.js$" src/
# Test 2: Check if there's a corresponding .ts file
fd -e ts "markdownStyle.ts$" src/
# Test 3: Check module resolution configuration in tsconfig
fd "tsconfig.*json$" . -x cat {} \; | jq 'select(.compilerOptions != null) | .compilerOptions | select(.moduleResolution != null) | .moduleResolution'
Length of output: 284
Script:
#!/bin/bash
# Check the module configuration in package.json and tsconfig
fd "package.json$" . -x cat {} \; | jq 'select(.type != null) | .type'
# Check tsconfig for module setting
fd "tsconfig.*json$" . -x cat {} \; | jq 'select(.compilerOptions != null) | .compilerOptions | select(.module != null) | .module'
# Look for any other imports with .js extension in TypeScript files
rg "from.*\.js[\"']" -g "*.ts" -A 1 -B 1
Length of output: 19927
tests/core/file/permissionCheck.test.ts (2)
157-184: LGTM! Comprehensive PermissionError class testing.
The PermissionError class tests thoroughly cover all scenarios including inheritance, property initialization, and optional parameters.
1-242: Excellent test coverage aligning with PR objectives.
The test implementation thoroughly validates the enhanced error handling capabilities introduced in the PR. The comprehensive coverage of various scenarios, platform-specific behaviors, and edge cases ensures robust error handling in the CLI entry point.
yamadashy
force-pushed
the
hotfix/error-handling
branch
2 times, most recently
from
2a52f62 to
864e081
Compare
yamadashy
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (19)
src/cli/cliRun.ts (4)
Line range hint
29-53: Add signal handlers for graceful shutdownBased on the PR objectives to implement graceful shutdown handling, consider adding handlers for SIGINT and SIGTERM signals to ensure clean process termination.
export const run = async () => { + // Setup signal handlers for graceful shutdown + process.on('SIGINT', () => { + logger.log('\nGracefully shutting down...'); + process.exit(0); + }); + process.on('SIGTERM', () => { + logger.log('\nGracefully shutting down...'); + process.exit(0); + }); try { program
Line range hint
29-53: Consider adding validation for numeric and path optionsThe CLI accepts numeric and path options that could benefit from validation to prevent runtime errors.
program .description('Repomix - Pack your repository into a single AI-friendly file') .arguments('[directory]') - .option('--top-files-len <number>', 'specify the number of top files to display', Number.parseInt) + .option('--top-files-len <number>', 'specify the number of top files to display', (value) => { + const parsed = Number.parseInt(value); + if (isNaN(parsed) || parsed <= 0) { + throw new Error('--top-files-len must be a positive number'); + } + return parsed; + }) - .option('-c, --config <path>', 'path to a custom config file') + .option('-c, --config <path>', 'path to a custom config file', (value) => { + if (!value.endsWith('.json')) { + throw new Error('Config file must be a JSON file'); + } + return value; + })
Line range hint
55-77: Enhance error handling for specific actionsEach action could benefit from specific error handling to provide more informative error messages.
export const executeAction = async (directory: string, cwd: string, options: CliOptions) => { logger.setVerbose(options.verbose || false); try { if (options.version) { await runVersionAction(); return; } const version = await getVersion(); logger.log(pc.dim(`\n📦 Repomix v${version}\n`)); if (options.init) { await runInitAction(cwd, options.global || false); return; } if (options.remote) { await runRemoteAction(options.remote, options); return; } await runDefaultAction(directory, cwd, options); + } catch (error) { + if (error instanceof Error) { + switch (true) { + case error.message.includes('config'): + throw new Error(`Configuration error: ${error.message}`); + case error.message.includes('remote'): + throw new Error(`Remote repository error: ${error.message}`); + default: + throw error; + } + } + throw error; } };
Line range hint
63-64: Enhance version display with additional informationConsider adding Node.js version and platform information to help with troubleshooting.
const version = await getVersion(); - logger.log(pc.dim(`\n📦 Repomix v${version}\n`)); + logger.log(pc.dim(` + 📦 Repomix v${version} + 🟢 Node ${process.version} + 💻 ${process.platform} ${process.arch} + `));tests/cli/actions/remoteAction.test.ts (3)
40-50: Consider adding error case tests.While the happy path is well tested, consider adding test cases for:
- When mkdtemp fails (e.g., permission issues)
- When the temp directory is not writable
Example test case:
test('should handle mkdtemp failure', async () => { vi.mocked(os.tmpdir).mockReturnValue('/mock/temp'); vi.mocked(fs.mkdtemp).mockRejectedValue(new Error('Permission denied')); await expect(createTempDirectory()).rejects.toThrow(); });
52-61: Add error handling test cases.Consider adding test cases for common cleanup failures:
- Directory doesn't exist
- Permission denied during removal
Example test case:
test('should handle cleanup failure gracefully', async () => { const mockDir = '/mock/temp/dir'; vi.mocked(fs.rm).mockRejectedValue(new Error('Permission denied')); await expect(cleanupTempDirectory(mockDir)).rejects.toThrow(); });
1-87: Consider organizing test cases using the AAA pattern.While the tests are well-structured, consider organizing each test case using the Arrange-Act-Assert pattern for better readability and maintainability. This would help separate the setup, execution, and verification phases of each test.
Example refactor:
test('should copy output file', async () => { // Arrange const sourceDir = '/source/dir'; const targetDir = '/target/dir'; const fileName = 'output.txt'; vi.mocked(fs.copyFile).mockResolvedValue(); // Act await copyOutputToCurrentDirectory(sourceDir, targetDir, fileName); // Assert expect(fs.copyFile).toHaveBeenCalledWith( path.join(sourceDir, fileName), path.join(targetDir, fileName) ); });src/cli/actions/remoteAction.ts (2)
Line range hint
61-70: Fix potential command injection vulnerability in git clone.The current implementation is vulnerable to command injection as it directly interpolates user input into the shell command.
export const cloneRepository = async (url: string, directory: string): Promise<void> => { logger.log(`Clone repository: ${url} to temporary directory. ${pc.dim(`path: ${directory}`)}`); logger.log(''); try { - await execAsync(`git clone --depth 1 ${url} ${directory}`); + await execAsync(['git', 'clone', '--depth', '1', url, directory].join(' ')); } catch (error) { throw new RepomixError(`Failed to clone repository: ${(error as Error).message}`); } };
Line range hint
87-99: Add permission checks before file operations.Consider adding permission checks before performing file operations to handle permission-related errors gracefully.
export const copyOutputToCurrentDirectory = async ( sourceDir: string, targetDir: string, outputFileName: string, ): Promise<void> => { const sourcePath = path.join(sourceDir, outputFileName); const targetPath = path.join(targetDir, outputFileName); try { + // Check read permission on source + await fs.access(sourcePath, fs.constants.R_OK); + // Check write permission on target directory + await fs.access(targetDir, fs.constants.W_OK); + logger.trace(`Copying output file from: ${sourcePath} to: ${targetPath}`); await fs.copyFile(sourcePath, targetPath); } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'EACCES') { + throw new RepomixError(`Permission denied: ${(error as Error).message}`); + } throw new RepomixError(`Failed to copy output file: ${(error as Error).message}`); } };tests/core/tokenCount/tokenCount.test.ts (1)
38-165: Consider adding a few more edge cases to strengthen test coverage.The test suite is well-structured with good coverage of various scenarios. Consider adding these additional test cases:
- Edge cases for null/undefined input
- Performance test with very large inputs (e.g., >100KB)
- Test for concurrent token counting to ensure thread safety
Example implementation:
test('should handle null/undefined input', () => { expect(tokenCounter.countTokens(null)).toBe(0); expect(tokenCounter.countTokens(undefined)).toBe(0); }); test('should handle very large inputs efficiently', () => { const start = performance.now(); const largeText = 'a'.repeat(100_000); const count = tokenCounter.countTokens(largeText); const duration = performance.now() - start; expect(duration).toBeLessThan(1000); // Should process within 1 second expect(count).toBeGreaterThan(0); }); test('should handle concurrent token counting', async () => { const texts = Array(10).fill('Hello, world!'); const counts = await Promise.all( texts.map(text => Promise.resolve(tokenCounter.countTokens(text))) ); expect(counts).toHaveLength(10); expect(new Set(counts).size).toBe(1); // All counts should be equal });tests/core/output/outputStyles/markdownStyle.test.ts (2)
6-86: Consider adding negative test cases for more robust testing.The current test suite for
getMarkdownTemplateis well-structured and covers the main functionality effectively. However, consider adding the following test cases for improved robustness:
- Test with malformed/invalid data
- Test with empty strings in optional fields
- Test with special characters in content
- Test with extremely large content
Example test case to add:
test('should handle malformed data gracefully', () => { const template = getMarkdownTemplate(); const compiledTemplate = Handlebars.compile(template); const data = { processedFiles: null, headerText: undefined, summaryPurpose: '', }; expect(() => compiledTemplate(data)).not.toThrow(); });
89-200: Consider additional edge cases for file extension handling.The test suite for
getFileExtensionis impressively comprehensive with excellent categorization. Consider adding these additional test cases:
- Case sensitivity tests (e.g.,
.JSvs.js)- Multiple extensions (e.g.,
file.test.ts,file.d.ts)- Mixed case extensions (e.g.,
.JsonC)Example test cases to add:
test('should handle case sensitivity and multiple extensions', () => { expect(getExtension('file.JS')).toBe('javascript'); expect(getExtension('file.test.ts')).toBe('typescript'); expect(getExtension('file.d.ts')).toBe('typescript'); expect(getExtension('file.JsonC')).toBe('json'); });tests/core/file/permissionCheck.test.ts (7)
1-16: LGTM! Consider extracting test constants.The test setup is well-structured with proper imports and mock configurations. Consider extracting commonly used test values like
testDirPathinto a separate constants object for better maintainability.+const TEST_CONSTANTS = { + DIR_PATH: '/test/directory', + MOCK_ERROR_MESSAGE: 'Permission denied', +} as const; + describe('permissionCheck', () => { - const testDirPath = '/test/directory'; + const testDirPath = TEST_CONSTANTS.DIR_PATH;
18-66: Add test case for empty directory path.The successful cases suite has good coverage but should include validation for edge cases like empty directory path.
test('should handle empty directory path', async () => { const result = await checkDirectoryPermissions(''); expect(result).toEqual({ hasPermission: false, error: expect.any(Error), }); expect(result.error?.message).toContain('Invalid directory path'); });
68-122: Add test case for network-related errors.The error cases suite should include handling of network-related errors (e.g., ENETDOWN, ETIMEDOUT) that could occur when checking permissions on network drives.
test('should handle network errors', async () => { const error = new Error('Network is down'); (error as NodeJS.ErrnoException).code = 'ENETDOWN'; vi.mocked(fs.readdir).mockRejectedValue(error); const result = await checkDirectoryPermissions(testDirPath); expect(result).toEqual({ hasPermission: false, error: expect.any(Error), }); expect(result.error?.message).toContain('Network error'); });
124-155: Add Linux-specific test case.While Windows and macOS cases are covered, consider adding a Linux-specific test case that verifies file permission bits messaging.
test('should return Linux-specific error message with permission bits', async () => { vi.mocked(platform).mockReturnValue('linux'); const error = new Error('Permission denied'); (error as NodeJS.ErrnoException).code = 'EACCES'; vi.mocked(fs.readdir).mockRejectedValue(error); const result = await checkDirectoryPermissions(testDirPath); expect(result.error).toBeInstanceOf(PermissionError); expect(result.error?.message).toContain('chmod'); expect(result.error?.message).toContain('file permissions'); });
157-184: Enhance PermissionError class test coverage.Consider adding tests for:
- Stack trace preservation
- JSON serialization of custom properties
test('should preserve stack trace', () => { const error = new PermissionError('Test message', '/test/path'); expect(error.stack).toBeDefined(); expect(error.stack).toContain('PermissionError: Test message'); }); test('should properly serialize custom properties', () => { const error = new PermissionError('Test message', '/test/path', 'EACCES'); const serialized = JSON.stringify(error); const parsed = JSON.parse(serialized); expect(parsed).toEqual({ name: 'PermissionError', message: 'Test message', path: '/test/path', code: 'EACCES' }); });
186-241: Add test case for symlink handling.The edge cases suite should include testing permission checks on symbolic links, as they may require different handling.
test('should handle symlink permissions', async () => { // Mock fs.lstat to identify path as symlink vi.spyOn(fs, 'lstat').mockResolvedValue({ isSymbolicLink: () => true } as fs.Stats); vi.mocked(fs.readdir).mockResolvedValue([]); vi.mocked(fs.access).mockResolvedValue(undefined); const result = await checkDirectoryPermissions(testDirPath); expect(result).toEqual({ hasPermission: true, details: { read: true, write: true, execute: true, }, isSymlink: true }); });
1-242: Consider reorganizing tests using test.each for similar test cases.The test file is well-structured and comprehensive. However, some test cases follow similar patterns and could be refactored using
test.eachto reduce code duplication and improve maintainability.describe('error cases', () => { test.each([ ['EPERM', 'Permission denied'], ['EACCES', 'Access denied'], ['EISDIR', 'Is a directory'] ])('should handle %s error with message "%s"', async (code, message) => { const error = new Error(message); (error as NodeJS.ErrnoException).code = code; vi.mocked(fs.readdir).mockRejectedValue(error); const result = await checkDirectoryPermissions(testDirPath); expect(result).toEqual({ hasPermission: false, error: expect.any(PermissionError) }); expect(result.error).toBeInstanceOf(PermissionError); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
biome.json(1 hunks)src/cli/actions/initAction.ts(3 hunks)src/cli/actions/remoteAction.ts(3 hunks)src/cli/cliRun.ts(2 hunks)src/core/file/permissionCheck.ts(3 hunks)tests/cli/actions/remoteAction.test.ts(1 hunks)tests/cli/cliRun.test.ts(1 hunks)tests/core/file/permissionCheck.test.ts(1 hunks)tests/core/output/outputStyles/markdownStyle.test.ts(1 hunks)tests/core/tokenCount/tokenCount.test.ts(1 hunks)vitest.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- biome.json
- src/cli/actions/initAction.ts
- src/core/file/permissionCheck.ts
- tests/cli/cliRun.test.ts
- vitest.config.ts
🔇 Additional comments (8)
tests/cli/actions/remoteAction.test.ts (3)
1-15: LGTM! Well-structured test setup.
The import statements and mock setup follow best practices:
- Using
node:prefix for Node.js built-ins - Proper separation of test framework and module imports
- Comprehensive mocking of external dependencies
23-38: LGTM! Comprehensive test coverage for URL formatting.
The test suite effectively covers all important URL formats:
- GitHub shorthand notation
- HTTPS URLs with and without .git suffix
- SSH URLs
63-87: LGTM! Excellent error handling coverage.
The test suite demonstrates good practices:
- Tests both successful and failed copy operations
- Verifies correct path joining
- Includes clear error messages
src/cli/actions/remoteAction.ts (1)
Line range hint 77-85: LGTM! Well-implemented error handling.
The function properly handles errors by catching exceptions and logging debug information, following the fail-safe principle by returning a boolean instead of throwing.
tests/core/tokenCount/tokenCount.test.ts (3)
1-11: LGTM! Clean mock setup and imports.
The mock setup for both tiktoken and logger is well-structured and follows testing best practices.
12-36: LGTM! Excellent test isolation and cleanup.
The test setup follows best practices by:
- Creating fresh instances for each test
- Properly cleaning up resources
- Resetting mocks between tests
1-165: Overall excellent test implementation!
The test suite demonstrates:
- Comprehensive coverage of various scenarios
- Proper test isolation and resource cleanup
- Clear and maintainable test structure
- Good error handling validation
tests/core/output/outputStyles/markdownStyle.test.ts (1)
1-3: LGTM! Clean and appropriate imports.
The imports are well-organized and include all necessary dependencies for testing.
yamadashy
force-pushed
the
hotfix/error-handling
branch
from
864e081 to
e34a8da
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (11)
bin/repomix.cjs (1)
3-14: Consider using semver for more robust version checkingWhile the current version check works, it could be more robust by using the
semverpackage for version comparison. Also, consider enhancing the error message with upgrade instructions.+const semver = require('semver'); const nodeVersion = process.versions.node; -const [major] = nodeVersion.split('.').map(Number); -if (major < 16) { +if (!semver.satisfies(nodeVersion, '>=16.0.0')) { console.error(`Repomix requires Node.js version 16 or higher. Current version: ${nodeVersion} +To upgrade Node.js: +- Using nvm: nvm install 16 +- Download from: https://nodejs.org/\n`); process.exit(EXIT_CODES.ERROR); }src/cli/cliRun.ts (2)
Line range hint
41-42: Consider adding validation for numeric and enum options.The
topFilesLenoption usesNumber.parseIntwithout validation, and thestyleoption accepts any string value. Consider adding validation to ensure these values are within acceptable ranges.Example improvement:
.option('--top-files-len <number>', 'specify the number of top files to display', - Number.parseInt) + (value) => { + const parsed = Number.parseInt(value); + if (isNaN(parsed) || parsed <= 0) { + throw new Error('top-files-len must be a positive number'); + } + return parsed; + }) .option('--style <type>', 'specify the output style (plain, xml, markdown)', + (value) => { + const validStyles = ['plain', 'xml', 'markdown']; + if (!validStyles.includes(value)) { + throw new Error(`style must be one of: ${validStyles.join(', ')}`); + } + return value; + })
Line range hint
55-75: Consider adding input validation and debug logging.The function structure and action handling look good. However, consider these improvements:
- Add input validation for the directory path.
- Add debug logging for verbose mode to help with troubleshooting.
Example improvement:
export const executeAction = async (directory: string, cwd: string, options: CliOptions) => { logger.setVerbose(options.verbose || false); + + // Validate directory path + if (directory && typeof directory !== 'string') { + throw new Error('Directory path must be a string'); + } + + if (options.verbose) { + logger.debug('Executing with options:', { + directory, + cwd, + ...options + }); + } if (options.version) { + logger.debug('Running version action'); await runVersionAction(); return; }tests/cli/actions/remoteAction.test.ts (1)
76-85: Consider enhancing error handling test with more specific error messages.The error message "Failed to copy output file" could be more informative by including the source and target paths, making it easier to diagnose issues.
Consider updating the error test like this:
test('should throw error when copy fails', async () => { const sourceDir = '/source/dir'; const targetDir = '/target/dir'; const fileName = 'output.txt'; vi.mocked(fs.copyFile).mockRejectedValue(new Error('Permission denied')); await expect(copyOutputToCurrentDirectory(sourceDir, targetDir, fileName)).rejects.toThrow( - 'Failed to copy output file', + `Failed to copy output file from ${path.join(sourceDir, fileName)} to ${path.join(targetDir, fileName)}`, ); });src/cli/actions/remoteAction.ts (2)
Line range hint
87-100: Enhance error handling and add input validation.While the error handling is good, consider these improvements:
- Add input validation for the file paths
- Make the error message more descriptive by including both source and target paths
export const copyOutputToCurrentDirectory = async ( sourceDir: string, targetDir: string, outputFileName: string, ): Promise<void> => { + if (!sourceDir || !targetDir || !outputFileName) { + throw new RepomixError('Invalid parameters: sourceDir, targetDir, and outputFileName are required'); + } + const sourcePath = path.join(sourceDir, outputFileName); const targetPath = path.join(targetDir, outputFileName); try { logger.trace(`Copying output file from: ${sourcePath} to: ${targetPath}`); await fs.copyFile(sourcePath, targetPath); } catch (error) { - throw new RepomixError(`Failed to copy output file: ${(error as Error).message}`); + throw new RepomixError( + `Failed to copy output file from '${sourcePath}' to '${targetPath}': ${(error as Error).message}` + ); } };
Line range hint
1-100: Standardize error handling patterns across the module.Consider implementing a consistent error handling strategy across all functions:
- All filesystem operations should have try-catch blocks
- Error messages should include relevant context (paths, parameters)
- Consider adding input validation for all public functions
- Use consistent error message formatting
This will improve debugging and error reporting throughout the application.
tests/core/tokenCount/tokenCount.test.ts (3)
14-36: Consider improving type safety in the test setup.While the setup is thorough, the type safety could be enhanced.
Consider this type-safe alternative:
- let mockEncoder: { - encode: Mock; - free: Mock; - }; + interface MockTiktoken { + encode: Mock<(text: string) => number[]>; + free: Mock<() => void>; + } + let mockEncoder: MockTiktoken; beforeEach(() => { mockEncoder = { encode: vi.fn(), free: vi.fn(), }; - vi.mocked(get_encoding).mockReturnValue(mockEncoder as unknown as Tiktoken); + vi.mocked(get_encoding).mockReturnValue(mockEncoder as MockTiktoken & Tiktoken);
128-137: Consider adding boundary condition tests for very long text.While the current test handles a 10,000 character input, it would be valuable to test:
- The maximum input length that tiktoken can handle
- Performance with varying input sizes
- Memory usage with extremely large inputs
Consider adding these test cases:
test('should handle maximum input length', () => { const maxLength = 1024 * 1024; // 1MB const text = 'a'.repeat(maxLength); const mockTokens = Array(1000).fill(1); mockEncoder.encode.mockReturnValue(mockTokens); const count = tokenCounter.countTokens(text); expect(count).toBe(1000); expect(mockEncoder.encode).toHaveBeenCalledWith(text); }); test('should handle input size close to token model context window', () => { const contextWindow = 4096; const text = 'a'.repeat(contextWindow * 4); // Assuming ~4 chars per token const mockTokens = Array(contextWindow).fill(1); mockEncoder.encode.mockReturnValue(mockTokens); const count = tokenCounter.countTokens(text); expect(count).toBe(contextWindow); expect(mockEncoder.encode).toHaveBeenCalledWith(text); });
139-161: Consider testing specific tiktoken error scenarios.While the current error handling tests are good, consider adding tests for specific error types that tiktoken might throw, such as encoding errors for invalid UTF-8 sequences or out-of-memory errors.
Consider adding these test cases:
test('should handle tiktoken UTF-8 encoding errors', () => { const invalidUtf8Error = new Error('invalid utf-8 sequence'); mockEncoder.encode.mockImplementation(() => { throw invalidUtf8Error; }); const count = tokenCounter.countTokens('\uD800'); // Invalid UTF-8 expect(count).toBe(0); expect(logger.warn).toHaveBeenCalledWith( expect.stringContaining('invalid utf-8 sequence') ); }); test('should handle tiktoken out-of-memory errors', () => { const outOfMemoryError = new Error('out of memory'); mockEncoder.encode.mockImplementation(() => { throw outOfMemoryError; }); const count = tokenCounter.countTokens('a'.repeat(1024 * 1024 * 1024)); // 1GB expect(count).toBe(0); expect(logger.warn).toHaveBeenCalledWith( expect.stringContaining('out of memory') ); });tests/core/output/outputStyles/markdownStyle.test.ts (2)
6-86: Consider adding more edge cases to strengthen the test suite.While the current test cases cover the basic functionality well, consider adding these scenarios to make the suite more robust:
- Error cases (e.g., invalid template data)
- Validation of markdown syntax in the output
- Empty or malformed input data
- Special characters in the input data
- Very large input data
Here's an example test case for handling special characters:
test('should properly escape special markdown characters in input', () => { const template = getMarkdownTemplate(); const compiledTemplate = Handlebars.compile(template); const data = { headerText: '# * _ [ ] ( ) ~ ` > Special chars', processedFiles: [] }; const result = compiledTemplate(data); expect(result).toContain('\\# \\* \\_ \\[ \\] \\( \\) \\~ \\` \\> Special chars'); });
124-125: Consider distinguishing between C and C++ file extensions.Currently, both
.cand.cppfiles are mapped to 'cpp'. This might be confusing for users as they are different languages. Consider using distinct syntax highlighting for each.-expect(getExtension('file.c')).toBe('cpp'); -expect(getExtension('file.cpp')).toBe('cpp'); +expect(getExtension('file.c')).toBe('c'); +expect(getExtension('file.cpp')).toBe('cpp');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
.github/workflows/ci.yml(1 hunks)bin/repomix.cjs(1 hunks)bin/repomix.js(0 hunks)biome.json(1 hunks)package.json(1 hunks)src/cli/actions/initAction.ts(3 hunks)src/cli/actions/remoteAction.ts(3 hunks)src/cli/cliRun.ts(2 hunks)src/core/file/permissionCheck.ts(3 hunks)tests/cli/actions/remoteAction.test.ts(1 hunks)tests/cli/cliRun.test.ts(1 hunks)tests/core/file/permissionCheck.test.ts(1 hunks)tests/core/output/outputStyles/markdownStyle.test.ts(1 hunks)tests/core/tokenCount/tokenCount.test.ts(1 hunks)vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- bin/repomix.js
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/ci.yml
- biome.json
- package.json
- src/cli/actions/initAction.ts
- src/core/file/permissionCheck.ts
- tests/cli/cliRun.test.ts
- tests/core/file/permissionCheck.test.ts
- vitest.config.ts
🔇 Additional comments (14)
bin/repomix.cjs (1)
27-36: Previous review comment about shutdown handling remains valid
The current shutdown implementation is still synchronous and doesn't allow for cleanup operations. Please refer to the previous review comment about enhancing shutdown handling with cleanup operations.
src/cli/cliRun.ts (2)
Line range hint 29-53: LGTM! Error handling improvements look good.
The conversion to an arrow function with proper async/await error handling aligns well with the PR objectives. The try-catch block ensures all errors are properly captured and handled.
Line range hint 29-75: Verify test coverage for CLI functionality.
The PR objectives mention adding tests for cliRun.ts. Let's verify the test coverage for both the run and executeAction functions.
✅ Verification successful
Test coverage for CLI functionality is comprehensive and well-structured
The test suite for cliRun.ts demonstrates thorough coverage:
- Tests for both
run()andexecuteAction()functions - Proper mocking of all dependencies (logger, commander, and all action modules)
- Coverage for all CLI options:
- Default execution path
- Verbose logging
- Version display
- Init functionality
- Remote repository processing
- Verification of correct action module calls with expected parameters
- Error handling through proper mock implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for cliRun.ts
# Look for test file
fd "cliRun.test" tests/
# Check test cases
rg -A 5 "describe|it\(" tests/cli/cliRun.test.ts
# Check mock implementations
rg "jest.mock" tests/cli/cliRun.test.ts
Length of output: 1003
Script:
#!/bin/bash
# Let's check for more test cases and mocks with better context
# Check all test cases with more context
rg -A 10 "test\(" tests/cli/cliRun.test.ts
# Check mock implementations with vitest
rg "vi.mock|vi.spyOn" tests/cli/cliRun.test.ts
# Check imports being mocked
rg "import.*from.*actions/" tests/cli/cliRun.test.ts
Length of output: 2531
tests/cli/actions/remoteAction.test.ts (4)
1-15: LGTM! Well-structured imports and mocks.
The import statements follow best practices with the 'node:' prefix for built-in modules, and all necessary dependencies are properly mocked.
17-21: LGTM! Good test structure and isolation.
The test suite is well-organized with proper mock resets between tests, ensuring good test isolation.
23-61: LGTM! Comprehensive test coverage for URL formatting and directory operations.
The test cases thoroughly cover different URL formats and directory operations with proper mocking and assertions.
1-85: Great alignment with PR objectives for improved test coverage!
These tests contribute significantly to the PR's goal of improving test coverage. The comprehensive test cases, proper mocking, and inclusion of error scenarios demonstrate a thorough testing approach.
src/cli/actions/remoteAction.ts (2)
55-59: Previous review comment is still applicable.
The suggestion to add explicit error handling for temporary directory creation remains valid, especially given the PR's focus on error handling improvements.
72-75: Previous review comment is still applicable.
The suggestion to add error handling for directory cleanup remains valid and aligns with the PR's error handling improvements.
tests/core/tokenCount/tokenCount.test.ts (2)
1-11: LGTM! Clean import statements and mock setup.
The imports are well-organized and the mocks are properly configured.
1-165: Overall, excellent test coverage with room for enhancements.
The test suite is well-structured with comprehensive coverage of basic scenarios, error handling, and resource cleanup. While there are opportunities for enhancement (as noted in previous comments), the current implementation provides a solid foundation for ensuring the reliability of the TokenCounter class.
tests/core/output/outputStyles/markdownStyle.test.ts (3)
1-3: LGTM! Imports are well-organized and appropriate.
The imports cover all necessary dependencies for testing: Handlebars for templating, vitest for testing utilities, and the function under test.
194-200: LGTM! Comprehensive edge case handling.
The edge cases are well thought out, covering:
- Files without extensions
- Dotfiles
- Unknown extensions
- Paths with directories
89-94: Verify the implementation of the getFileExtension helper.
The test suite references a Handlebars helper that's not visible in the provided code. Please ensure that the corresponding implementation is properly exported and documented.
✅ Verification successful
The getFileExtension helper is properly implemented and registered
The helper is correctly implemented in src/core/output/outputStyles/markdownStyle.ts as a Handlebars helper that extracts and maps file extensions to their corresponding syntax highlighting identifiers. The implementation aligns with the test suite's expectations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the getFileExtension helper implementation
rg -l "getFileExtension.*=.*function" --type ts
Length of output: 49
Script:
#!/bin/bash
# Let's try a broader search for getFileExtension helper using ast-grep
ast-grep --pattern 'getFileExtension = $_'
# Also search for any file containing getFileExtension string
rg "getFileExtension" --type ts -A 5
Length of output: 1774
yamadashy
force-pushed
the
hotfix/error-handling
branch
from
fdee489 to
3ee6356
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (14)
src/core/file/disposableTempDir.ts (3)
1-7: Add class-level documentation.The implementation looks good, but could benefit from JSDoc documentation describing the class's purpose and usage.
Add this documentation above the class declaration:
+/** + * Manages a temporary directory that is automatically cleaned up when the instance is disposed. + * Usage: + * ```ts + * async function example() { + * await using tempDir = await DisposableTempDir.create('prefix-'); + * const path = tempDir.getPath(); + * // Use temporary directory... + * } // Directory is automatically cleaned up when the block exits + * ``` + */ export class DisposableTempDir implements AsyncDisposable {
14-18: Add input validation for dirPrefix parameter.While the implementation is correct, it should validate the input parameter to ensure it's a valid directory prefix.
Consider adding this validation:
static async create(dirPrefix: string): Promise<DisposableTempDir> { + if (!dirPrefix || typeof dirPrefix !== 'string' || /[<>:"/\\|?*\x00-\x1F]/g.test(dirPrefix)) { + throw new Error('Invalid directory prefix. Must be a string containing valid filename characters.'); + } const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), dirPrefix)); logger.trace(`Created temporary directory. (path: ${pc.dim(tempDir)})`); return new DisposableTempDir(tempDir); }
24-32: Consider adding a directory existence check.While the implementation is robust, checking if the directory exists before attempting removal could optimize the cleanup process.
Consider this optimization:
async [Symbol.asyncDispose](): Promise<void> { try { logger.trace(`Cleaning up temporary directory: ${this.directory}`); + // Skip cleanup if directory doesn't exist + try { + await fs.access(this.directory); + } catch { + return; + } await fs.rm(this.directory, { recursive: true, force: true }); } catch (error) { // Log error but don't throw to ensure cleanup completes logger.warn(`Failed to cleanup temporary directory: ${this.directory}`, error); } }tests/cli/actions/remoteAction.test.ts (1)
57-79: Consider additional edge cases for copyOutputToCurrentDirectory.The current tests cover the basic success and failure scenarios well. Consider adding tests for:
- Empty or invalid source/target paths
- File existence verification
- Special characters in filenames
Example additional test:
test('should handle invalid source directory', async () => { const sourceDir = ''; const targetDir = '/target/dir'; const fileName = 'output.txt'; await expect(copyOutputToCurrentDirectory(sourceDir, targetDir, fileName)) .rejects.toThrow('Invalid source directory'); });src/cli/actions/remoteAction.ts (1)
Line range hint
57-66: Fix potential command injection vulnerability.The
urlparameter is directly interpolated into the shell command, which could allow command injection if the URL contains shell metacharacters.Apply this diff to sanitize the input:
export const cloneRepository = async (url: string, directory: string): Promise<void> => { logger.log(`Clone repository: ${url} to temporary directory. ${pc.dim(`path: ${directory}`)}`); logger.log(''); try { - await execAsync(`git clone --depth 1 ${url} ${directory}`); + // Validate URL format + if (!/^[a-zA-Z0-9_.\-/:@]+$/.test(url)) { + throw new RepomixError('Invalid repository URL format'); + } + // Use array syntax to avoid shell injection + await execAsync('git', ['clone', '--depth', '1', url, directory], { shell: false }); } catch (error) { throw new RepomixError(`Failed to clone repository: ${(error as Error).message}`); } };tests/config/globalDirectory.test.ts (3)
1-7: Consider using scoped mocking instead of global mockingWhile the current setup works, globally mocking the
node:osmodule could potentially affect other test files. Consider usingvi.mock()within the describe block or moving it to a separate test setup file if needed across multiple test files.-vi.mock('node:os'); +describe('getGlobalDirectory', () => { + beforeAll(() => { + vi.mock('node:os'); + }); + + afterAll(() => { + vi.unmock('node:os'); + });
8-21: Enhance type safety for environment variablesConsider using TypeScript's type system to better handle environment variables and prevent potential runtime errors.
+type ProcessEnv = { + LOCALAPPDATA?: string; + XDG_CONFIG_HOME?: string; + [key: string]: string | undefined; +}; -const originalEnv = process.env; +const originalEnv = process.env as ProcessEnv; beforeEach(() => { vi.resetAllMocks(); - process.env = { ...originalEnv }; + process.env = { ...originalEnv } as ProcessEnv; });
69-87: Add more edge cases for robust testingConsider adding the following edge cases:
- Non-ASCII characters in paths
- Maximum path length scenarios
- Network paths (UNC paths on Windows)
- Relative paths in environment variables
+ test('should handle non-ASCII characters in paths', () => { + Object.defineProperty(process, 'platform', { value: 'linux' }); + process.env.XDG_CONFIG_HOME = '/home/测试/配置'; + + const result = getGlobalDirectory(); + expect(result).toBe(path.join('/home/测试/配置', 'repomix')); + }); + + test('should handle UNC paths on Windows', () => { + Object.defineProperty(process, 'platform', { value: 'win32' }); + process.env.LOCALAPPDATA = '\\\\server\\share\\path'; + + const result = getGlobalDirectory(); + expect(result).toBe(path.join('\\\\server\\share\\path', 'Repomix')); + });tests/core/file/disposableTempDir.test.ts (2)
17-26: Add type assertions for mocked functionsConsider adding type assertions to the mocked functions for better type safety.
beforeEach(() => { vi.resetAllMocks(); - vi.mocked(os.tmpdir).mockReturnValue(mockTmpDir); - vi.mocked(fs.mkdtemp).mockResolvedValue(expectedTempPath); - vi.mocked(fs.rm).mockResolvedValue(undefined); + vi.mocked(os.tmpdir).mockReturnValue(mockTmpDir as string); + vi.mocked(fs.mkdtemp).mockResolvedValue(expectedTempPath as string); + vi.mocked(fs.rm).mockResolvedValue(undefined as void); });
91-118: Enhance integration tests with more edge casesWhile the current integration tests cover basic scenarios, consider adding:
- Test for nested directory structures
- Test for file permissions
- Test for race conditions
Example test case for nested structures:
test('should cleanup nested directory structure', async () => { await using tempDir = await DisposableTempDir.create(testPrefix); const nestedPath = path.join(tempDir.getPath(), 'nested', 'dir'); await fs.mkdir(nestedPath, { recursive: true }); await fs.writeFile(path.join(nestedPath, 'test.txt'), 'test'); // Disposal should remove everything });🧰 Tools
🪛 GitHub Check: Test (macos-latest, 16.x)
[failure] 115-115: tests/core/file/disposableTempDir.test.ts > DisposableTempDir > integration > should cleanup directory even if an error occurs
AssertionError: expected error to have message: Operation errorExpected: "Operation error"
Received: "Object not disposable"❯ tests/core/file/disposableTempDir.test.ts:115:7
🪛 GitHub Check: Test (ubuntu-latest, 16.x)
[failure] 115-115: tests/core/file/disposableTempDir.test.ts > DisposableTempDir > integration > should cleanup directory even if an error occurs
AssertionError: expected error to have message: Operation errorExpected: "Operation error"
Received: "Object not disposable"❯ tests/core/file/disposableTempDir.test.ts:115:7
tests/cli/cliPrint.test.ts (4)
9-18: Consider refactoring the picocolors mock for better maintainability.The picocolors mock could be simplified and made more maintainable.
Consider this refactoring:
-vi.mock('picocolors', () => ({ - default: { - white: (str: string) => `WHITE:${str}`, - dim: (str: string) => `DIM:${str}`, - green: (str: string) => `GREEN:${str}`, - yellow: (str: string) => `YELLOW:${str}`, - red: (str: string) => `RED:${str}`, - cyan: (str: string) => `CYAN:${str}`, - }, -})); +const createColorMock = (color: string) => (str: string) => `${color.toUpperCase()}:${str}`; +vi.mock('picocolors', () => ({ + default: { + white: createColorMock('white'), + dim: createColorMock('dim'), + green: createColorMock('green'), + yellow: createColorMock('yellow'), + red: createColorMock('red'), + cyan: createColorMock('cyan'), + }, +}));
25-48: Consider adding more test cases for better coverage.While the current tests cover the basic scenarios, consider adding:
- Edge case with zero total files
- Verification of file counts and sizes in the output
- Case with multiple suspicious files
Example additional test:
test('should print summary with zero files', () => { const config = createMockConfig({ security: { enableSecurityCheck: true }, }); printSummary(0, 0, 0, 'output.txt', [], config); expect(logger.log).toHaveBeenCalledWith(expect.stringContaining('0 file(s)')); });
50-93: LGTM with a minor suggestion.The security check tests are comprehensive. Consider adding a test case with multiple suspicious files to verify the ordering and numbering of the output.
Example additional test case:
test('should handle multiple suspicious files in sorted order', () => { const config = createMockConfig({ security: { enableSecurityCheck: true }, }); const suspiciousFiles: SuspiciousFileResult[] = [ { filePath: 'b.txt', messages: ['Issue 1'] }, { filePath: 'a.txt', messages: ['Issue 2'] }, ]; printSecurityCheck('/root', suspiciousFiles, config); const calls = (logger.log as vi.Mock).mock.calls.flat(); expect(calls).toEqual( expect.arrayContaining([ expect.stringContaining('1. a.txt'), expect.stringContaining('2. b.txt'), ]) ); });
95-121: Enhance test coverage for edge cases.The current tests verify basic functionality, but consider adding:
- Test case for files with equal character counts
- Verification of token count display in the output
- Test for exact ordering of files
Example additional test:
test('should handle files with equal character counts', () => { const fileCharCounts = { 'a.ts': 1000, 'b.ts': 1000, }; const fileTokenCounts = { 'a.ts': 200, 'b.ts': 100, }; printTopFiles(fileCharCounts, fileTokenCounts, 2); const calls = (logger.log as vi.Mock).mock.calls.flat(); expect(calls[1]).toContain('a.ts'); expect(calls[2]).toContain('b.ts'); expect(calls[1]).toContain('200 tokens'); expect(calls[2]).toContain('100 tokens'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
bin/repomix.cjs(1 hunks)src/cli/actions/remoteAction.ts(5 hunks)src/core/file/disposableTempDir.ts(1 hunks)tests/cli/actions/remoteAction.test.ts(1 hunks)tests/cli/cliPrint.test.ts(1 hunks)tests/config/globalDirectory.test.ts(1 hunks)tests/core/file/disposableTempDir.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/repomix.cjs
🧰 Additional context used
🪛 GitHub Check: Test (macos-latest, 16.x)
tests/core/file/disposableTempDir.test.ts
[failure] 12-12: tests/core/file/disposableTempDir.test.ts > DisposableTempDir > Symbol.asyncDispose > should clean up directory when disposed
TypeError: Object not disposable
❯ __typeError tests/core/file/disposableTempDir.test.ts:12:9
❯ __using tests/core/file/disposableTempDir.test.ts:20:40
❯ cleanup tests/core/file/disposableTempDir.test.ts:55:31
❯ tests/core/file/disposableTempDir.test.ts:60:7
[failure] 12-12: tests/core/file/disposableTempDir.test.ts > DisposableTempDir > Symbol.asyncDispose > should handle cleanup errors gracefully
TypeError: Object not disposable
❯ __typeError tests/core/file/disposableTempDir.test.ts:12:9
❯ __using tests/core/file/disposableTempDir.test.ts:20:40
❯ cleanup tests/core/file/disposableTempDir.test.ts:69:31
❯ tests/core/file/disposableTempDir.test.ts:72:7
[failure] 12-12: tests/core/file/disposableTempDir.test.ts > DisposableTempDir > Symbol.asyncDispose > should use correct options for directory removal
TypeError: Object not disposable
❯ __typeError tests/core/file/disposableTempDir.test.ts:12:9
❯ __using tests/core/file/disposableTempDir.test.ts:20:40
❯ cleanup tests/core/file/disposableTempDir.test.ts:83:31
❯ tests/core/file/disposableTempDir.test.ts:86:7
[failure] 12-12: tests/core/file/disposableTempDir.test.ts > DisposableTempDir > integration > should create and cleanup directory using await using
TypeError: Object not disposable
❯ __typeError tests/core/file/disposableTempDir.test.ts:12:9
❯ __using tests/core/file/disposableTempDir.test.ts:20:40
❯ cleanup tests/core/file/disposableTempDir.test.ts:96:31
❯ tests/core/file/disposableTempDir.test.ts:101:7
[failure] 115-115: tests/core/file/disposableTempDir.test.ts > DisposableTempDir > integration > should cleanup directory even if an error occurs
AssertionError: expected error to have message: Operation error
Expected: "Operation error"
Received: "Object not disposable"
❯ tests/core/file/disposableTempDir.test.ts:115:7
🪛 GitHub Check: Test (ubuntu-latest, 16.x)
tests/core/file/disposableTempDir.test.ts
[failure] 12-12: tests/core/file/disposableTempDir.test.ts > DisposableTempDir > Symbol.asyncDispose > should clean up directory when disposed
TypeError: Object not disposable
❯ __typeError tests/core/file/disposableTempDir.test.ts:12:9
❯ __using tests/core/file/disposableTempDir.test.ts:20:40
❯ cleanup tests/core/file/disposableTempDir.test.ts:55:31
❯ tests/core/file/disposableTempDir.test.ts:60:7
[failure] 12-12: tests/core/file/disposableTempDir.test.ts > DisposableTempDir > Symbol.asyncDispose > should handle cleanup errors gracefully
TypeError: Object not disposable
❯ __typeError tests/core/file/disposableTempDir.test.ts:12:9
❯ __using tests/core/file/disposableTempDir.test.ts:20:40
❯ cleanup tests/core/file/disposableTempDir.test.ts:69:31
❯ tests/core/file/disposableTempDir.test.ts:72:7
[failure] 12-12: tests/core/file/disposableTempDir.test.ts > DisposableTempDir > Symbol.asyncDispose > should use correct options for directory removal
TypeError: Object not disposable
❯ __typeError tests/core/file/disposableTempDir.test.ts:12:9
❯ __using tests/core/file/disposableTempDir.test.ts:20:40
❯ cleanup tests/core/file/disposableTempDir.test.ts:83:31
❯ tests/core/file/disposableTempDir.test.ts:86:7
[failure] 12-12: tests/core/file/disposableTempDir.test.ts > DisposableTempDir > integration > should create and cleanup directory using await using
TypeError: Object not disposable
❯ __typeError tests/core/file/disposableTempDir.test.ts:12:9
❯ __using tests/core/file/disposableTempDir.test.ts:20:40
❯ cleanup tests/core/file/disposableTempDir.test.ts:96:31
❯ tests/core/file/disposableTempDir.test.ts:101:7
[failure] 115-115: tests/core/file/disposableTempDir.test.ts > DisposableTempDir > integration > should cleanup directory even if an error occurs
AssertionError: expected error to have message: Operation error
Expected: "Operation error"
Received: "Object not disposable"
❯ tests/core/file/disposableTempDir.test.ts:115:7
🔇 Additional comments (10)
src/core/file/disposableTempDir.ts (3)
8-12: LGTM! Good encapsulation practices.
The private constructor and readonly member ensure proper initialization and immutability.
20-22: LGTM! Simple and effective.
The getter method correctly provides access to the directory path.
14-18: Verify concurrent usage safety.
Let's verify that the implementation handles concurrent operations safely, particularly around directory creation and cleanup.
Also applies to: 24-32
✅ Verification successful
Let me check the implementation details of the DisposableTempDir class and its usage to better understand the concurrent safety aspects.
No race condition concerns in the temporary directory implementation
The implementation is safe from race conditions because:
fs.mkdtemp()is designed to generate unique directory names atomically, preventing naming conflicts even with concurrent calls- Each instance manages its own unique directory path
- The cleanup operation uses
{ force: true }which safely handles cases where the directory might have been already removed - The class follows the singleton pattern per directory with private constructor, preventing external instantiation of duplicate handlers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential race conditions in concurrent directory handling
# Search for tests that verify concurrent usage
rg -l "concurrent|race condition|multiple instances" "tests/"
# Check if there are other temporary directory implementations that might conflict
ast-grep --pattern 'fs.mkdtemp($$$)'
Length of output: 328
Script:
#!/bin/bash
# Check the full implementation of DisposableTempDir
cat src/core/file/disposableTempDir.ts
# Check how it's being used in tests
cat tests/integration-tests/packager.test.ts
# Look for parallel/concurrent test execution configurations
fd -e json "jest.config" -X cat
Length of output: 3975
tests/cli/actions/remoteAction.test.ts (2)
1-18: LGTM! Well-structured mock setup.
The mock implementation preserves the original fs/promises functionality while isolating the copyFile function for testing. This approach maintains system integrity while allowing precise control over file operations in tests.
40-55: LGTM! Comprehensive URL format testing.
The test cases effectively cover all major URL formats and transformations, including GitHub shorthand, HTTPS URLs, and SSH URLs.
src/cli/actions/remoteAction.ts (1)
27-32: LGTM! Error handling improvements look great.
The error handling has been enhanced with:
- Specific error messages during cloning
- User-friendly feedback through the spinner
- Proper error propagation
tests/config/globalDirectory.test.ts (1)
1-87: Verify test coverage configuration
Ensure this test file is included in the coverage reports and contributing to the overall test coverage metrics.
✅ Verification successful
Test coverage configuration is properly set up
The test file is correctly included in coverage reports as:
- Test files under
tests/**/*.test.tsare included in test execution (vitest.config.ts) - Source files under
src/**/*are tracked for coverage (vitest.config.ts) - Coverage reporting is configured with text, JSON, and HTML reporters
- Coverage command is properly set up in package.json as
test-coverage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the test file is included in coverage configuration
# Look for coverage configuration in package.json or vitest.config.ts
echo "Checking coverage configuration..."
fd -e json -e ts -e js -x grep -l "coverage" {}
Length of output: 212
Script:
#!/bin/bash
# Check coverage configuration in vitest.config.ts
echo "Content of vitest.config.ts:"
cat vitest.config.ts
# Check coverage settings in package.json
echo -e "\nContent of package.json (coverage-related):"
cat package.json | grep -A 5 -B 5 "coverage"
Length of output: 1516
tests/core/file/disposableTempDir.test.ts (1)
1-10:
Verify the DisposableTempDir implementation for Symbol.asyncDispose
The test failures indicate that the DisposableTempDir class is not properly implementing the disposable interface. Let's verify the implementation.
The class should implement the disposable interface as follows:
class DisposableTempDir {
// ... other methods ...
async [Symbol.asyncDispose](): Promise<void> {
try {
await fs.rm(this.dirPath, { recursive: true, force: true });
logger.trace(`Cleaning up temporary directory: ${this.dirPath}`);
} catch (error) {
logger.warn(`Failed to cleanup temporary directory: ${this.dirPath}`, error);
}
}
}tests/cli/cliPrint.test.ts (2)
20-24: LGTM!
The test suite setup follows best practices with proper test isolation using beforeEach.
123-131: LGTM!
The completion message test is straightforward and effectively verifies the expected output.
yamadashy
force-pushed
the
hotfix/error-handling
branch
2 times, most recently
from
93f1601 to
1f2fac4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
bin/repomix.cjs (1)
3-4: Make version extraction more robustThe current version extraction could fail with malformed version strings.
Consider this more robust approach:
-const [major] = nodeVersion.split('.').map(Number); +const major = parseInt(nodeVersion.match(/^(\d+)/)?.[1] ?? '0', 10);src/cli/actions/remoteAction.ts (3)
28-29: Enhance the comment to be more descriptive.Consider adding more context to the comment, such as the depth limitation and its implications.
-// Clone the repository +// Clone the repository with depth=1 (shallow clone) to minimize download size
38-38: Consider improving the error message format.The current message combines two different states with "Error during repository cloning. cleanup...". Consider separating these for clarity.
-spinner.fail('Error during repository cloning. cleanup...'); +spinner.fail('Error during repository cloning'); +logger.trace('Cleaning up temporary directory...');
Line range hint
94-106: Consider adding a file existence check before copying.While the error handling is good, checking if the source file exists before attempting to copy would provide a more specific error message.
export const copyOutputToCurrentDirectory = async ( sourceDir: string, targetDir: string, outputFileName: string, ): Promise<void> => { const sourcePath = path.join(sourceDir, outputFileName); const targetPath = path.join(targetDir, outputFileName); try { + const exists = await fs.access(sourcePath).then(() => true).catch(() => false); + if (!exists) { + throw new RepomixError(`Output file not found: ${outputFileName}`); + } logger.trace(`Copying output file from: ${sourcePath} to: ${targetPath}`); await fs.copyFile(sourcePath, targetPath); } catch (error) { + if (error instanceof RepomixError) throw error; throw new RepomixError(`Failed to copy output file: ${(error as Error).message}`); } };tests/cli/cliPrint.test.ts (4)
8-18: Consider simplifying the picocolors mock implementation.The current mock implementation could be simplified using a factory function to reduce repetition.
-vi.mock('picocolors', () => ({ - default: { - white: (str: string) => `WHITE:${str}`, - dim: (str: string) => `DIM:${str}`, - green: (str: string) => `GREEN:${str}`, - yellow: (str: string) => `YELLOW:${str}`, - red: (str: string) => `RED:${str}`, - cyan: (str: string) => `CYAN:${str}`, - }, -})); +const createColorFn = (color: string) => (str: string) => `${color.toUpperCase()}:${str}`; +vi.mock('picocolors', () => ({ + default: { + white: createColorFn('white'), + dim: createColorFn('dim'), + green: createColorFn('green'), + yellow: createColorFn('yellow'), + red: createColorFn('red'), + cyan: createColorFn('cyan'), + }, +}));
95-121: Enhance test coverage for printTopFiles.Consider adding the following test cases to improve coverage:
- Verify the exact order of files (not just their presence)
- Test files with equal character counts to ensure consistent sorting
- Test with N larger than the number of files
Example additional test:
test('should handle files with equal character counts', () => { const fileCharCounts = { 'src/a.ts': 1000, 'src/b.ts': 1000, }; const fileTokenCounts = { 'src/a.ts': 200, 'src/b.ts': 200, }; printTopFiles(fileCharCounts, fileTokenCounts, 2); const calls = vi.mocked(logger.log).mock.calls; const fileLines = calls.filter(call => call[0].includes('src/')); expect(fileLines).toHaveLength(2); // Verify consistent ordering when char counts are equal });
25-48: Enhance test coverage for printSummary.The current tests focus mainly on the security check status. Consider adding tests for:
- Verification of all numeric values (files, characters, tokens)
- Edge cases with zero values
- Exact output format verification
Example additional test:
test('should print correct numeric values in summary', () => { const config = createMockConfig({ security: { enableSecurityCheck: true }, }); printSummary(0, 0, 0, 'output.txt', [], config); expect(logger.log).toHaveBeenCalledWith(expect.stringContaining('0 file(s)')); expect(logger.log).toHaveBeenCalledWith(expect.stringContaining('0 character(s)')); expect(logger.log).toHaveBeenCalledWith(expect.stringContaining('0 token(s)')); });
1-131: Consider architectural improvements for better test maintainability.The test file is well-structured but could benefit from:
- Extracting common test fixtures into shared constants
- Using consistent assertion patterns across all tests
- Adding type assertions for mock calls
Example improvements:
// At the top of the file const TEST_FIXTURES = { suspiciousFile: { filePath: path.normalize('config/secrets.txt'), messages: ['Contains API key', 'Contains password'], }, mockConfig: { security: { enableSecurityCheck: true }, }, } as const; // In tests const config = createMockConfig(TEST_FIXTURES.mockConfig);🧰 Tools
🪛 GitHub Check: Test (windows-latest, 16.x)
[failure] 86-86: tests/cli/cliPrint.test.ts > cliPrint > printSecurityCheck > should print details of suspicious files when found
AssertionError: expected "log" to be called with arguments: [ 'WHITE:1. WHITE:config/secrets.txt' ]Received:
1st log call:
Array [
- "WHITE:1. WHITE:config/secrets.txt",
- "WHITE:🔎 Security Check:",
]2nd log call:
Array [
- "WHITE:1. WHITE:config/secrets.txt",
- "DIM:──────────────────",
]3rd log call:
Array [
- "WHITE:1. WHITE:config/secrets.txt",
- "YELLOW:1 suspicious file(s) detected and excluded from the output:",
]4th log call:
Array [
- "WHITE:1. WHITE:config/secrets.txt",
- "WHITE:1. WHITE:config\secrets.txt",
]5th log call:
Array [
- "WHITE:1. WHITE:config/secrets.txt",
- "DIM: - Contains API key
- Contains password",
]6th log call:
Array [
- "WHITE:1. WHITE:config/secrets.txt",
"YELLOW:
These files have been excluded from the output for security reasons.",
]7th log call:
Array [
- "WHITE:1. WHITE:config/secrets.txt",
- "YELLOW:Please review these files for potential sensitive information.",
]Number of calls: 7
❯ tests/cli/cliPrint.test.ts:86:26
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
bin/repomix.cjs(1 hunks)src/cli/actions/remoteAction.ts(4 hunks)tests/cli/actions/remoteAction.test.ts(1 hunks)tests/cli/cliPrint.test.ts(1 hunks)tests/config/globalDirectory.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/cli/actions/remoteAction.test.ts
- tests/config/globalDirectory.test.ts
🧰 Additional context used
🪛 GitHub Check: Test (windows-latest, 16.x)
tests/cli/cliPrint.test.ts
[failure] 86-86: tests/cli/cliPrint.test.ts > cliPrint > printSecurityCheck > should print details of suspicious files when found
AssertionError: expected "log" to be called with arguments: [ 'WHITE:1. WHITE:config/secrets.txt' ]
Received:
1st log call:
Array [
- "WHITE:1. WHITE:config/secrets.txt",
- "WHITE:🔎 Security Check:",
]
2nd log call:
Array [
- "WHITE:1. WHITE:config/secrets.txt",
- "DIM:──────────────────",
]
3rd log call:
Array [
- "WHITE:1. WHITE:config/secrets.txt",
- "YELLOW:1 suspicious file(s) detected and excluded from the output:",
]
4th log call:
Array [
- "WHITE:1. WHITE:config/secrets.txt",
- "WHITE:1. WHITE:config\secrets.txt",
]
5th log call:
Array [
- "WHITE:1. WHITE:config/secrets.txt",
- "DIM: - Contains API key
-
- Contains password",
]
- Contains password",
6th log call:
Array [
- "WHITE:1. WHITE:config/secrets.txt",
-
"YELLOW:
-
These files have been excluded from the output for security reasons.",
]7th log call:
Array [
- "WHITE:1. WHITE:config/secrets.txt",
- "YELLOW:Please review these files for potential sensitive information.",
]
Number of calls: 7
❯ tests/cli/cliPrint.test.ts:86:26
🔇 Additional comments (3)
bin/repomix.cjs (2)
16-26: LGTM: Error handlers implementation is solid
The implementation properly handles both uncaught exceptions and unhandled promise rejections with appropriate error logging and process termination.
38-56: LGTM: Main execution flow is well-structured
The implementation:
- Uses IIFE pattern correctly for top-level await
- Provides detailed error information including stack traces
- Ensures proper process termination on errors
src/cli/actions/remoteAction.ts (1)
34-36: LGTM! Clear and well-structured code.
The code is well-organized with descriptive comments and proper error handling delegation.
yamadashy
force-pushed
the
hotfix/error-handling
branch
from
1f2fac4 to
2738963
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/cli/actions/remoteAction.ts (3)
23-24: LGTM! Consider enhancing error message clarity.The error handling improvements and cleanup flow look good. However, the error message could be more specific.
Consider this small improvement:
- spinner.fail('Error during repository cloning. cleanup...'); + spinner.fail(`Failed to process repository: ${(error as Error).message}`);Also applies to: 27-30, 34-37, 38-39, 41-42
Line range hint
68-77: Fix potential command injection vulnerability.The
urlparameter is directly interpolated into the shell command, which could lead to command injection if the URL contains shell metacharacters.Consider using an array of arguments with
execFileinstead:- await execAsync(`git clone --depth 1 ${url} ${directory}`); + const { execFile } = require('node:child_process'); + const execFileAsync = promisify(execFile); + await execFileAsync('git', ['clone', '--depth', '1', url, directory]);
Line range hint
94-107: Consider adding file existence check.The function handles copy errors well, but it might be helpful to check if the source file exists before attempting to copy.
Consider this improvement:
try { + const exists = await fs.access(sourcePath).then(() => true).catch(() => false); + if (!exists) { + throw new RepomixError(`Source file does not exist: ${sourcePath}`); + } logger.trace(`Copying output file from: ${sourcePath} to: ${targetPath}`); await fs.copyFile(sourcePath, targetPath); } catch (error) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
bin/repomix.cjs(1 hunks)src/cli/actions/remoteAction.ts(4 hunks)tests/cli/actions/remoteAction.test.ts(1 hunks)tests/cli/cliPrint.test.ts(1 hunks)tests/config/globalDirectory.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- bin/repomix.cjs
- tests/cli/actions/remoteAction.test.ts
- tests/cli/cliPrint.test.ts
- tests/config/globalDirectory.test.ts
🔇 Additional comments (3)
src/cli/actions/remoteAction.ts (3)
62-66: Skip comment - refer to previous review.
The previous review comment about adding explicit error handling for temporary directory creation is still valid and hasn't been addressed.
79-82: Skip comment - refer to previous review.
The previous review comment about adding error handling for directory cleanup is still valid and hasn't been addressed.
Line range hint 84-92: LGTM! Well-structured error handling.
The function properly handles Git installation checks with appropriate error logging and boolean return values.
yamadashy
force-pushed
the
hotfix/error-handling
branch
from
2738963 to
54b4256
Compare
yamadashy
force-pushed
the
hotfix/error-handling
branch
from
54b4256 to
47b179b
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
tests/cli/actions/remoteAction.test.ts (1)
43-58: Add edge cases to URL formatting tests.While the current tests cover common URL formats, consider adding edge cases:
test('should handle edge cases', () => { expect(() => formatGitUrl('')).toThrow('Invalid repository URL'); expect(() => formatGitUrl('invalid-format')).toThrow('Invalid repository URL'); expect(() => formatGitUrl('user/')).toThrow('Invalid repository URL'); expect(() => formatGitUrl('/repo')).toThrow('Invalid repository URL'); });
🛑 Comments failed to post (1)
tests/cli/actions/remoteAction.test.ts (1)
35-39: 🛠️ Refactor suggestion
Mock git installation check for reliable testing.
The current test relies on the actual git installation, which makes it environment-dependent. Consider mocking the git check for more reliable and predictable tests.
describe('checkGitInstallation Integration', () => { - test('should detect git installation in real environment', async () => { - const result = await checkGitInstallation(); - expect(result).toBe(true); - }); + test('should detect git installation', async () => { + vi.mock('child_process', () => ({ + execSync: vi.fn().mockReturnValue(Buffer.from('git version 2.30.1')) + })); + expect(await checkGitInstallation()).toBe(true); + }); + + test('should handle missing git installation', async () => { + vi.mock('child_process', () => ({ + execSync: vi.fn().mockImplementation(() => { + throw new Error('git not found'); + }) + })); + expect(await checkGitInstallation()).toBe(false); + }); });Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
tests/cli/actions/remoteAction.test.ts (1)
14-21: Enhance mock implementations for better test isolation.Consider expanding the fs mock to include other potentially needed operations like mkdir, rmdir, etc., to ensure complete test isolation. Also, configure the logger mock explicitly to verify logging behavior.
vi.mock('node:fs/promises', async (importOriginal) => { const actual = await importOriginal<typeof import('node:fs/promises')>(); return { ...actual, copyFile: vi.fn(), + mkdir: vi.fn(), + rmdir: vi.fn(), + access: vi.fn(), }; }); vi.mock('../../../src/shared/logger'); + +// Configure logger mock +const logger = await import('../../../src/shared/logger'); +vi.mocked(logger.default.info).mockImplementation(() => {}); +vi.mocked(logger.default.error).mockImplementation(() => {});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
bin/repomix.cjs(1 hunks)src/cli/actions/remoteAction.ts(4 hunks)tests/cli/actions/remoteAction.test.ts(1 hunks)tests/cli/cliPrint.test.ts(1 hunks)tests/config/globalDirectory.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- bin/repomix.cjs
- src/cli/actions/remoteAction.ts
- tests/cli/cliPrint.test.ts
- tests/config/globalDirectory.test.ts
🔇 Additional comments (1)
tests/cli/actions/remoteAction.test.ts (1)
60-82: LGTM: Comprehensive error handling in copyOutputToCurrentDirectory tests.
The test cases effectively cover both success and failure scenarios for file copying operations, aligning well with the PR's focus on error handling improvements.
This PR includes comprehensive improvements to error handling and test coverage across the codebase.
Error Handling Improvements
repomix.cjs)Test Coverage Improvements
cliRun.ts: Add command execution testspermissionCheck.ts: Cover all permission scenariostokenCount.ts: Improve token counting coveragemarkdownStyle.ts: Add template and extension handling testsindex.tsfrom coverage reporting