feat: Implement Zod for Robust Configuration Validation #167
Conversation
|
|
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several changes across multiple files, primarily focusing on the restructuring of configuration handling within the project. A new dependency on Changes
Possibly related PRs
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 (
|
![standard-input[bot]](https://avatars.githubusercontent.com/in/1011034?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #167 +/- ##
==========================================
+ Coverage 75.63% 76.50% +0.87%
==========================================
Files 37 35 -2
Lines 1859 1873 +14
Branches 336 318 -18
==========================================
+ Hits 1406 1433 +27
+ Misses 453 440 -13 ☔ View full report in Codecov by Sentry. |
yamadashy
force-pushed
the
feature/zod
branch
from
e70a1d3
to
3af0bf3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (17)
src/shared/errorHandle.ts (1)
31-38: Consider enhancing the error handling implementation.While the implementation is functional, here are some suggestions to improve it:
-export const rethrowValidationErrorIfZodError = (error: unknown, message: string): void => { +export const convertZodError = (error: unknown, contextMessage: string): never => { if (error instanceof z.ZodError) { - const zodErrorText = error.errors.map((err) => `${err.path.join('.')}: ${err.message}`).join(', '); + const validationErrors = error.errors + .map((err) => ` • ${err.path.join('.')}: ${err.message}`) + .join('\n'); + throw new RepomixConfigValidationError( - `${message}\n ${zodErrorText}\n Please check the config file and try again.`, + `${contextMessage}\n\nValidation Errors:\n${validationErrors}\n\nPlease check the config file and try again.` ); } + return error as never; };Changes suggested:
- Rename function to better reflect its purpose (converts & throws)
- Improve error message formatting for better readability
- Add type narrowing for non-Zod errors
- Rename parameters to be more descriptive
src/core/file/fileProcess.ts (1)
Line range hint
6-38: Consider documenting configuration validation strategy.While the code correctly uses the
RepomixConfigMergedtype, it might be helpful to add a comment explaining that configuration validation is handled upstream by Zod schemas. This would help future maintainers understand the validation flow and prevent accidental duplicate validation.Add a comment above the functions:
+// Note: Configuration is validated using Zod schemas during load time in configLoad.ts export const processFiles = async (rawFiles: RawFile[], config: RepomixConfigMerged): Promise<ProcessedFile[]> => {package.json (1)
70-71: Consider upgrading Zod to the latest version.The current version specification
"zod": "^3.23.8"is using an older version. Consider upgrading to the latest stable version for potential bug fixes and improvements.- "zod": "^3.23.8" + "zod": "^3.22.4"tests/integration-tests/packager.test.ts (2)
Line range hint
16-24: Consider expanding test coverage for configuration validation.Since this PR introduces Zod validation, consider adding test cases that verify:
- Invalid configuration handling
- Edge cases in configuration validation
- Zod validation error messages
Example test case to add:
{ desc: 'invalid configuration', input: 'simple-project', output: 'invalid-config-output.txt', config: { output: { style: 'invalid_style' } }, // Should trigger Zod validation error expectedError: true }
Line range hint
56-63: Consider using vitest's built-in snapshot capabilities.The current manual snapshot update mechanism could be replaced with vitest's built-in snapshot testing features for more robust and maintainable tests.
Example refactor:
// Instead of manual file comparison expect(actualOutput).toMatchSnapshot({ // Use snapshot matchers with dynamic content replacement replacements: [ [/Generated by Repomix on:.*\n/gm, 'Generated by Repomix on: <DATE>\n'] ] });src/core/output/outputGenerate.ts (2)
Line range hint
77-86: Consider using exhaustive type checking for template styles.The switch statement could benefit from TypeScript's exhaustive type checking to ensure all possible style values are handled explicitly.
Consider refactoring like this:
- switch (config.output.style) { - case 'xml': - template = getXmlTemplate(); - break; - case 'markdown': - template = getMarkdownTemplate(); - break; - default: - template = getPlainTemplate(); - } + switch (config.output.style) { + case 'xml': + template = getXmlTemplate(); + break; + case 'markdown': + template = getMarkdownTemplate(); + break; + case 'plain': + template = getPlainTemplate(); + break; + default: + const exhaustiveCheck: never = config.output.style; + return exhaustiveCheck; + }
Line range hint
93-103: Enhance error handling with specific error information.The error handling for file reading could be more informative by including the specific error details.
Consider enhancing the error handling:
try { repositoryInstruction = await fs.readFile(instructionPath, 'utf-8'); - } catch { - throw new RepomixError(`Instruction file not found at ${instructionPath}`); + } catch (error) { + throw new RepomixError( + `Failed to read instruction file at ${instructionPath}: ${ + error instanceof Error ? error.message : 'Unknown error' + }` + ); }tests/config/configLoad.test.ts (1)
42-51: Consider splitting validation tests for better coverage.While the test correctly validates multiple invalid fields, consider splitting it into separate test cases for each validation scenario (invalid filePath, invalid style, invalid useDefaultPatterns) to:
- Make test failures more specific and easier to debug
- Improve test documentation
- Ensure complete coverage of validation rules
Example refactor:
- test('should throw RepomixConfigValidationError for invalid config', async () => { + test.each([ + ['invalid filePath type', { output: { filePath: 123 } }], + ['invalid style', { output: { style: 'invalid' } }], + ['invalid useDefaultPatterns', { ignore: { useDefaultPatterns: 'not a boolean' } }] + ])('should throw RepomixConfigValidationError for %s', async (_, invalidConfig) => { vi.mocked(fs.readFile).mockResolvedValue(JSON.stringify(invalidConfig)); vi.mocked(fs.stat).mockResolvedValue({ isFile: () => true } as Stats); await expect(loadFileConfig(process.cwd(), 'test-config.json')).rejects.toThrow(RepomixConfigValidationError); });tests/config/configSchema.test.ts (5)
12-22: Consider adding edge cases to output style tests.While the current tests cover basic valid and invalid cases, consider adding tests for edge cases:
it('should reject edge cases', () => { expect(() => repomixOutputStyleSchema.parse(undefined)).toThrow(z.ZodError); expect(() => repomixOutputStyleSchema.parse(null)).toThrow(z.ZodError); expect(() => repomixOutputStyleSchema.parse('')).toThrow(z.ZodError); });
24-58: Enhance base config schema test coverage.Consider splitting the invalid types test into separate test cases for better error isolation and debugging:
it('should reject invalid output.filePath type', () => { expect(() => repomixConfigBaseSchema.parse({ output: { filePath: 123 } })).toThrow(z.ZodError); }); it('should reject invalid output.style value', () => { expect(() => repomixConfigBaseSchema.parse({ output: { style: 'invalid' } })).toThrow(z.ZodError); }); it('should reject invalid include type', () => { expect(() => repomixConfigBaseSchema.parse({ include: 'not-an-array' })).toThrow(z.ZodError); });
60-93: Add boundary tests for numeric values and required fields.Consider adding tests for:
it('should validate topFilesLength boundaries', () => { const validConfig = { output: { // ... other required fields ... topFilesLength: 0 } }; expect(() => repomixConfigDefaultSchema.parse(validConfig)).toThrow(z.ZodError); }); it('should require essential output fields', () => { ['style', 'removeComments', 'removeEmptyLines', 'topFilesLength'].forEach(field => { const config = { output: { filePath: 'output.txt', style: 'plain', removeComments: false, removeEmptyLines: false, topFilesLength: 5, showLineNumbers: false, copyToClipboard: true } }; delete config.output[field]; expect(() => repomixConfigDefaultSchema.parse(config)).toThrow(z.ZodError); }); });
95-117: Add file path validation tests.Consider adding tests for various file path scenarios:
it('should validate file path patterns', () => { const configs = [ { output: { filePath: '../outside.txt' } }, { output: { filePath: '/absolute/path.txt' } }, { output: { filePath: 'relative/path.txt' } }, { output: { filePath: '' } } ]; configs.forEach(config => { expect(() => repomixConfigFileSchema.parse(config)) .not.toThrow(z.ZodError); }); });
119-139: LGTM! Consider adding CLI-specific option tests.The basic validation is well covered. Consider adding tests for CLI-specific behaviors if any exist.
src/config/configSchema.ts (2)
60-62: Redundant Schemas for File and CLI ConfigurationsBoth
repomixConfigFileSchemaandrepomixConfigCliSchemaare assignedrepomixConfigBaseSchema, resulting in redundancy since they are identical.Consider refactoring to eliminate redundancy or differentiate these schemas if they are intended to have unique validations in the future.
64-71: Simplify the Merged Configuration SchemaCombining
repomixConfigDefaultSchema,repomixConfigFileSchema, andrepomixConfigCliSchema, which are either extensions or duplicates of the base schema, may be unnecessarily complex.Simplify
repomixConfigMergedSchemaby mergingrepomixConfigDefaultSchemawithrepomixConfigBaseSchemadirectly, along with the requiredcwdproperty:-export const repomixConfigMergedSchema = repomixConfigDefaultSchema - .and(repomixConfigFileSchema) - .and(repomixConfigCliSchema) - .and( - z.object({ - cwd: z.string(), - }), - ); +export const repomixConfigMergedSchema = repomixConfigDefaultSchema + .and(repomixConfigBaseSchema) + .and( + z.object({ + cwd: z.string(), + }), + );src/config/configLoad.ts (2)
3-3: Consider simplifying the function name 'rethrowValidationErrorIfZodError'The function name
rethrowValidationErrorIfZodErroris quite long and repetitive. Simplifying it to something likehandleZodValidationErrororrethrowIfZodErrorcould enhance readability without sacrificing clarity.
Line range hint
85-109: Consider adding unit tests for configuration merging logicTo verify that the configuration merging behaves as expected across various scenarios, consider adding unit tests that test different combinations of
defaultConfig,fileConfig, andcliConfig. This will help ensure the merging logic remains robust against future changes.
🛑 Comments failed to post (1)
tests/config/configLoad.test.ts (1)
107-117: 🛠️ Refactor suggestion
Improve type safety and test coverage in merged config validation.
Several concerns with the current implementation:
- The
@ts-ignoresuppresses TypeScript's type checking without explanation. If this is intentional for testing invalid types, please add a comment explaining why.- Consider testing more merged configuration scenarios.
Suggested improvements:
- test('should throw RepomixConfigValidationError for invalid merged config', () => { - const fileConfig: RepomixConfigFile = { - output: { filePath: 'file-output.txt', style: 'plain' }, - }; - const cliConfig: RepomixConfigCli = { - // @ts-ignore - output: { style: 'invalid' }, // Invalid style - }; - - expect(() => mergeConfigs(process.cwd(), fileConfig, cliConfig)).toThrow(RepomixConfigValidationError); - }); + describe('invalid merged configs', () => { + test('should throw RepomixConfigValidationError for invalid style', () => { + const fileConfig: RepomixConfigFile = { + output: { filePath: 'file-output.txt', style: 'plain' }, + }; + // Using type assertion instead of @ts-ignore for explicit testing of runtime validation + const cliConfig = { + output: { style: 'invalid' }, + } as RepomixConfigCli; + + expect(() => mergeConfigs(process.cwd(), fileConfig, cliConfig)).toThrow(RepomixConfigValidationError); + }); + + test('should throw RepomixConfigValidationError when merging incompatible configs', () => { + const fileConfig: RepomixConfigFile = { + output: { filePath: 'test.txt' }, + ignore: { useDefaultPatterns: true } + }; + const cliConfig = { + ignore: { useDefaultPatterns: 'yes' } // invalid boolean + } as RepomixConfigCli; + + expect(() => mergeConfigs(process.cwd(), fileConfig, cliConfig)).toThrow(RepomixConfigValidationError); + }); + });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.describe('invalid merged configs', () => { test('should throw RepomixConfigValidationError for invalid style', () => { const fileConfig: RepomixConfigFile = { output: { filePath: 'file-output.txt', style: 'plain' }, }; // Using type assertion instead of @ts-ignore for explicit testing of runtime validation const cliConfig = { output: { style: 'invalid' }, } as RepomixConfigCli; expect(() => mergeConfigs(process.cwd(), fileConfig, cliConfig)).toThrow(RepomixConfigValidationError); }); test('should throw RepomixConfigValidationError when merging incompatible configs', () => { const fileConfig: RepomixConfigFile = { output: { filePath: 'test.txt' }, ignore: { useDefaultPatterns: true } }; const cliConfig = { ignore: { useDefaultPatterns: 'yes' } // invalid boolean } as RepomixConfigCli; expect(() => mergeConfigs(process.cwd(), fileConfig, cliConfig)).toThrow(RepomixConfigValidationError); }); });
yamadashy
force-pushed
the
feature/zod
branch
from
3af0bf3
to
d28f34d
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: 3
🧹 Outside diff range and nitpick comments (4)
src/shared/errorHandle.ts (1)
31-38: Consider enhancing error message formatting and documentationThe implementation is solid, but could benefit from some refinements:
+/** + * Checks if the error is a Zod validation error and rethrows it as a RepomixConfigValidationError + * with a formatted message. + * @param error - The error to check + * @param message - The context message to prepend to the error + * @throws {RepomixConfigValidationError} When the error is a Zod validation error + */ export const rethrowValidationErrorIfZodError = (error: unknown, message: string): void => { if (error instanceof z.ZodError) { - const zodErrorText = error.errors.map((err) => `${err.path.join('.')}: ${err.message}`).join(', '); + const zodErrorText = error.errors + .map((err) => `${err.path.join('.')}: ${err.message}`) + .join('\n '); throw new RepomixConfigValidationError( - `${message}\n ${zodErrorText}\n Please check the config file and try again.`, + `${message} + ${zodErrorText} + Please check the config file and try again.`, ); } };The suggested changes:
- Add JSDoc documentation for better API understanding
- Improve error message readability by using multi-line formatting
- Split the error mapping for better maintainability
src/config/configSchema.ts (2)
6-33: Consider adding validation constraints for string fieldsWhile the schema structure is well-organized, consider adding validation constraints for string fields to ensure data quality:
filePath: Validate file path formatheaderText: Set max lengthinstructionFilePath: Validate file path formatExample implementation:
- filePath: z.string().optional(), + filePath: z.string().min(1).regex(/^[^<>:"|?*]+$/).optional(), - headerText: z.string().optional(), + headerText: z.string().max(1000).optional(), - instructionFilePath: z.string().optional(), + instructionFilePath: z.string().min(1).regex(/^[^<>:"|?*]+$/).optional(),
1-81: Add JSDoc documentation for schemas and typesConsider adding comprehensive JSDoc documentation to explain:
- The purpose and usage of each schema
- The relationship between different schemas
- Examples of valid configurations
Example:
/** * Schema for output style configuration. * @example * const style: RepomixOutputStyle = 'markdown'; */ export const repomixOutputStyleSchema = z.enum(['plain', 'xml', 'markdown']);tests/config/configLoad.test.ts (1)
42-51: Consider splitting validation tests for better isolationWhile the test correctly validates invalid configuration handling, consider splitting it into separate test cases for each invalid field. This would:
- Make it clearer which validation failed
- Provide better test isolation
- Make maintenance easier
Example structure:
test('should throw RepomixConfigValidationError for invalid filePath type', async () => { const invalidConfig = { output: { filePath: 123 }, // Only testing filePath }; // ... rest of the test }); test('should throw RepomixConfigValidationError for invalid useDefaultPatterns type', async () => { const invalidConfig = { ignore: { useDefaultPatterns: 'not a boolean' }, // Only testing useDefaultPatterns }; // ... rest of the test });
🛑 Comments failed to post (3)
src/config/configSchema.ts (2)
64-71: 🛠️ Refactor suggestion
Add validation for cwd path
The
cwdfield should validate that the path exists and is a directory:- cwd: z.string(), + cwd: z.string() + .min(1) + .refine((path) => { + try { + return require('fs').statSync(path).isDirectory(); + } catch { + return false; + } + }, "Path must exist and be a directory"),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export const repomixConfigMergedSchema = repomixConfigDefaultSchema .and(repomixConfigFileSchema) .and(repomixConfigCliSchema) .and( z.object({ cwd: z.string() .min(1) .refine((path) => { try { return require('fs').statSync(path).isDirectory(); } catch { return false; } }, "Path must exist and be a directory"), }), );
35-58: 🛠️ Refactor suggestion
Add range validation for numeric fields
Consider adding range validation for numeric fields to prevent invalid values:
- topFilesLength: z.number(), + topFilesLength: z.number().int().positive().max(1000),This ensures
topFilesLengthis a positive integer with a reasonable upper limit.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export const repomixConfigDefaultSchema = repomixConfigBaseSchema.and( z.object({ output: z.object({ filePath: z.string(), style: repomixOutputStyleSchema, headerText: z.string().optional(), instructionFilePath: z.string().optional(), removeComments: z.boolean(), removeEmptyLines: z.boolean(), topFilesLength: z.number().int().positive().max(1000), showLineNumbers: z.boolean(), copyToClipboard: z.boolean(), }), include: z.array(z.string()), ignore: z.object({ useGitignore: z.boolean(), useDefaultPatterns: z.boolean(), customPatterns: z.array(z.string()).optional(), }), security: z.object({ enableSecurityCheck: z.boolean(), }), }), );tests/config/configLoad.test.ts (1)
107-117: 🛠️ Refactor suggestion
Avoid @ts-ignore and enhance merge validation tests
The test correctly validates merged configurations, but there are some improvements to consider:
- Instead of using
@ts-ignore, consider creating a proper type for testing invalid configurations:type InvalidCliConfig = Omit<RepomixConfigCli, 'output'> & { output: { style: unknown }; };
Add assertions for the specific validation error message to ensure the right validation failed.
Consider adding more test cases for other merge scenarios (e.g., nested merges, array merges).
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
src/config/configSchema.ts (4)
1-13: LGTM! Consider extracting file extensions as constants.The output style enum and default file path mapping are well-structured. The use of
z.enumensures type safety, and theconstassertion prevents accidental modifications.Consider extracting the file extensions as constants to make maintenance easier:
+const FILE_EXTENSIONS = { + PLAIN: 'txt', + MARKDOWN: 'md', + XML: 'xml', +} as const; export const defaultFilePathMap: Record<RepomixOutputStyle, string> = { - plain: 'repomix-output.txt', - markdown: 'repomix-output.md', - xml: 'repomix-output.xml', + plain: `repomix-output.${FILE_EXTENSIONS.PLAIN}`, + markdown: `repomix-output.${FILE_EXTENSIONS.MARKDOWN}`, + xml: `repomix-output.${FILE_EXTENSIONS.XML}`, } as const;
37-41: Consider enhancing security validation options.The current security schema only includes a basic enable/disable flag. Consider adding more granular security controls:
security: z .object({ enableSecurityCheck: z.boolean().optional(), + maxFileSize: z.number().positive().optional(), + allowedFileTypes: z.array(z.string()).optional(), + excludePatterns: z.array(z.string()).optional(), }) .optional(),
44-72: Document default configuration values.Consider adding JSDoc comments to explain the reasoning behind default values, especially for security and ignore settings.
Example:
/** * Default configuration schema with predefined values. * @property {boolean} ignore.useGitignore - Enabled by default to respect project's .gitignore * @property {boolean} ignore.useDefaultPatterns - Enabled to exclude common build/dependency directories * @property {boolean} security.enableSecurityCheck - Enabled by default for secure operation */ export const repomixConfigDefaultSchema = z.object({ // ... rest of the schema });
1-92: Add comprehensive module documentation.While the code is well-structured, adding a module-level JSDoc comment would help developers understand the overall purpose and relationships between different schemas.
Add at the top of the file:
/** * Configuration schema definitions using Zod. * * This module defines the validation schemas for Repomix configurations: * - Base Schema: Defines the structure without defaults * - Default Schema: Extends base with default values * - File Schema: For configuration files * - CLI Schema: For command-line options * - Merged Schema: Combines all schemas with runtime values * * @module configSchema */src/config/configLoad.ts (2)
Line range hint
67-82: Consider enhancing Zod validation error messagesWhile the error handling is comprehensive, we could make the validation errors more user-friendly by including the specific validation failure details from Zod.
Consider updating the error handling like this:
- rethrowValidationErrorIfZodError(error, 'Invalid config schema'); + rethrowValidationErrorIfZodError(error, 'Invalid config schema:', { + includeDetails: true, + prefix: 'Configuration validation failed:' + });
101-124: Consider simplifying config merging logicThe current nested spread operations could be simplified using a utility function for better maintainability.
Consider refactoring like this:
+ const mergeArrays = (...arrays: unknown[][]) => + arrays.reduce((acc, arr) => [...acc, ...(arr || [])], []); + + const mergeObjects = (...objects: Record<string, unknown>[]) => + objects.reduce((acc, obj) => ({ ...acc, ...(obj || {}) }), {}); const mergedConfig = { cwd, - output: { - ...baseConfig.output, - ...fileConfig.output, - ...cliConfig.output, - }, + output: mergeObjects(baseConfig.output, fileConfig.output, cliConfig.output), - include: [...(baseConfig.include || []), ...(fileConfig.include || []), ...(cliConfig.include || [])], + include: mergeArrays(baseConfig.include, fileConfig.include, cliConfig.include), ignore: { - ...baseConfig.ignore, - ...fileConfig.ignore, - ...cliConfig.ignore, + ...mergeObjects(baseConfig.ignore, fileConfig.ignore, cliConfig.ignore), - customPatterns: [ - ...(baseConfig.ignore.customPatterns || []), - ...(fileConfig.ignore?.customPatterns || []), - ...(cliConfig.ignore?.customPatterns || []), - ], + customPatterns: mergeArrays( + baseConfig.ignore.customPatterns, + fileConfig.ignore?.customPatterns, + cliConfig.ignore?.customPatterns + ), }, - security: { - ...baseConfig.security, - ...fileConfig.security, - ...cliConfig.security, - }, + security: mergeObjects(baseConfig.security, fileConfig.security, cliConfig.security), };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
package.json(1 hunks)src/cli/actions/defaultAction.ts(2 hunks)src/cli/actions/initAction.ts(1 hunks)src/cli/cliPrint.ts(1 hunks)src/cli/cliRun.ts(1 hunks)src/config/configLoad.ts(3 hunks)src/config/configSchema.ts(1 hunks)src/config/configTypes.ts(0 hunks)src/config/configValidate.ts(0 hunks)src/config/defaultConfig.ts(0 hunks)src/core/file/fileProcess.ts(1 hunks)src/core/file/fileSearch.ts(1 hunks)src/core/output/outputGenerate.ts(1 hunks)src/core/output/outputGeneratorTypes.ts(1 hunks)src/core/output/outputStyleDecorate.ts(1 hunks)src/core/packager.ts(1 hunks)src/index.ts(1 hunks)src/shared/errorHandle.ts(3 hunks)tests/config/configLoad.test.ts(3 hunks)tests/config/configSchema.test.ts(1 hunks)tests/config/configValidate.test.ts(0 hunks)tests/integration-tests/packager.test.ts(1 hunks)tests/testing/testUtils.ts(1 hunks)
💤 Files with no reviewable changes (4)
- src/config/configTypes.ts
- src/config/configValidate.ts
- src/config/defaultConfig.ts
- tests/config/configValidate.test.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- package.json
- src/cli/actions/initAction.ts
- src/cli/cliPrint.ts
- src/cli/cliRun.ts
- src/core/file/fileProcess.ts
- src/core/file/fileSearch.ts
- src/core/output/outputGenerate.ts
- src/core/output/outputGeneratorTypes.ts
- src/core/output/outputStyleDecorate.ts
- src/core/packager.ts
- src/index.ts
- src/shared/errorHandle.ts
- tests/config/configLoad.test.ts
- tests/config/configSchema.test.ts
- tests/integration-tests/packager.test.ts
- tests/testing/testUtils.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/cli/actions/defaultAction.ts
[warning] 119-121: src/cli/actions/defaultAction.ts#L119-L121
Added lines #L119 - L121 were not covered by tests
src/config/configLoad.ts
[warning] 95-96: src/config/configLoad.ts#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 98-98: src/config/configLoad.ts#L98
Added line #L98 was not covered by tests
🔇 Additional comments (7)
src/config/configSchema.ts (1)
78-85: Verify schema composition behavior.
The schema composition looks correct, but let's ensure it properly handles overlapping fields and maintains type safety.
✅ Verification successful
Schema composition is properly implemented and tested
The schema composition is working correctly with comprehensive test coverage:
- The merged schema properly combines default, file, and CLI schemas while enforcing required
cwdfield - Tests verify proper handling of overlapping fields and type safety
- Test cases cover validation of:
- Complete valid configurations
- Missing required fields
- Invalid field types
- Schema composition behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential schema composition issues in tests
# Look for test cases that verify schema composition behavior
# Test: Search for schema composition tests
rg -l "repomixConfigMergedSchema" "tests/"
# Test: Look for potential validation edge cases in test files
rg "\.and\(" "tests/" -A 5
Length of output: 101
Script:
#!/bin/bash
# Let's examine the test file content and schema validation tests
cat tests/config/configSchema.test.ts
# Also check the schema implementation for better context
cat src/config/configSchema.ts
# Look for any validation-related test cases
rg "test|describe|it.*schema" tests/config/configSchema.test.ts
Length of output: 9154
src/cli/actions/defaultAction.ts (3)
3-9: LGTM: Import changes align with Zod implementation
The migration from configTypes.js to configSchema.js and addition of repomixConfigCliSchema properly supports the new Zod-based configuration validation.
11-11: LGTM: Error handling utility import
The addition of rethrowValidationErrorIfZodError import supports proper Zod validation error handling.
116-121: Add test coverage for CLI config validation errors
The error handling paths in the buildCliConfig function lack test coverage. Please add test cases to verify:
- Validation failures for invalid CLI arguments
- Proper error messages in validation errors
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 119-121: src/cli/actions/defaultAction.ts#L119-L121
Added lines #L119 - L121 were not covered by tests
src/config/configLoad.ts (3)
3-14: LGTM: Clean migration to Zod-based type definitions
The import restructuring effectively transitions from manual type definitions to Zod schema-based types, improving type safety and validation.
Line range hint 26-65: LGTM: Enhanced error handling and user feedback
The function now provides better user guidance with informative messages about missing configurations and clear error handling.
126-131: LGTM: Proper validation of merged config
The final validation ensures the merged configuration maintains schema compliance.
yamadashy
force-pushed
the
feature/zod
branch
from
9054c15
to
5d05730
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/config/configSchema.ts (2)
20-21: Add validation constraints for text fields.Consider adding length constraints for text fields to prevent extremely long inputs:
- headerText: z.string().optional(), + headerText: z.string().max(1000).optional(), - instructionFilePath: z.string().optional(), + instructionFilePath: z.string().max(4096).optional(),
37-41: Consider expanding security validation options.The current security schema only has a single toggle. Consider adding more granular security controls such as:
- Maximum file size limits
- Allowed file extensions
- Sensitive data pattern exclusions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
src/cli/actions/defaultAction.ts(2 hunks)src/cli/actions/initAction.ts(1 hunks)src/config/configLoad.ts(3 hunks)src/config/configSchema.ts(1 hunks)src/config/defaultConfig.ts(0 hunks)src/shared/errorHandle.ts(3 hunks)tests/config/configSchema.test.ts(1 hunks)tests/testing/testUtils.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/config/defaultConfig.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/cli/actions/initAction.ts
- src/shared/errorHandle.ts
- tests/config/configSchema.test.ts
- tests/testing/testUtils.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/cli/actions/defaultAction.ts
[warning] 119-121: src/cli/actions/defaultAction.ts#L119-L121
Added lines #L119 - L121 were not covered by tests
src/config/configLoad.ts
[warning] 95-96: src/config/configLoad.ts#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 98-98: src/config/configLoad.ts#L98
Added line #L98 was not covered by tests
🔇 Additional comments (10)
src/config/configSchema.ts (3)
1-12: LGTM! Well-structured enum and type-safe mapping.
The implementation provides a robust type-safe mapping between output styles and their default file paths.
54-54: Skip comment: Past review about topFilesLength upper bound is still valid.
92-92: Consider adding schema validation test.
The default config parsing could throw if the schema is invalid. Consider wrapping it in a try-catch or moving it to an initialization function.
src/cli/actions/defaultAction.ts (3)
3-9: LGTM! Import changes align with Zod implementation
The migration from configTypes.js to configSchema.js and addition of repomixConfigCliSchema properly supports the new Zod-based validation system.
11-11: LGTM! Good error handling organization
The import of rethrowValidationErrorIfZodError follows good practices by centralizing error handling logic.
116-121:
Add test coverage for CLI config validation
The error handling logic for CLI config validation lacks test coverage. This is critical functionality that should be thoroughly tested.
Let's check the existing test coverage:
Please add test cases covering:
- Valid CLI configurations pass validation
- Invalid configurations throw appropriate errors
- Error messages are properly propagated
Would you like me to help create these test cases?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 119-121: src/cli/actions/defaultAction.ts#L119-L121
Added lines #L119 - L121 were not covered by tests
src/config/configLoad.ts (4)
3-14: LGTM: Clean import restructuring
The imports are well-organized and properly support the new Zod-based validation approach.
71-73: LGTM: Robust validation and error handling
The implementation properly validates configs using Zod schemas and provides clear error messages for different failure scenarios.
94-98: Add test coverage for default output path logic
The default output path handling logic lacks test coverage. This is a critical path that should be thoroughly tested.
#!/bin/bash
# Verify existing test coverage for default path logic
echo "Checking for existing test cases..."
rg -A 5 "test.*default.*output.*path" "tests/"
echo "Checking implementation details..."
rg -A 5 "defaultFilePathMap" "src/"Would you like me to help generate test cases for:
- When both CLI and file configs have null file paths
- Default file path selection based on style
- Default file path mapping for each output style
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 95-96: src/config/configLoad.ts#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 98-98: src/config/configLoad.ts#L98
Added line #L98 was not covered by tests
101-131: LGTM: Well-structured config merging
The config merging implementation is robust:
- Properly handles deep merging of nested properties
- Maintains precedence (CLI > file > default)
- Validates the final merged config
Implement Zod for Robust Configuration Validation