feat(config): Add .revu.yml config file and its loader

[RealVidy]

#27

[socket-security]

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	@​types/​js-yaml@​4.0.9

View full report

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[revu-bot]

Pull Request Analysis

Overview

This PR adds a configuration system to the Revu application, allowing users to define custom coding guidelines through a .revu.yml file. The implementation includes a configuration handler module with functions to read, merge, and format configuration data, along with comprehensive tests. The PR also updates the README to document the new functionality.

The changes effectively solve the intended problem by providing a flexible way for users to customize coding guidelines that will be used during code reviews, with sensible defaults and proper error handling.

Code Quality Review

Strengths

• Well-structured module: The config-handler.ts is well-organized with clear separation of concerns.
• Comprehensive test coverage: The tests cover all main functions and edge cases, including file not found, parsing errors, and various configuration scenarios.
• Type safety: Proper TypeScript interfaces are used for configuration objects.
• Defensive programming: The code handles various error conditions gracefully, falling back to defaults when needed.
• Pure functions: Functions like formatCodingGuidelines and mergeConfigs are pure and focused on a single responsibility.
• Documentation: Good use of JSDoc comments for functions and interfaces.

Areas for Improvement

• Error logging: The error handling in readConfig logs to console directly. Consider using a more flexible logging approach that can be configured or mocked in tests.

  // Current approach
  console.error('Error reading .revu.yml configuration file:', error);
  
  // Consider a more flexible approach
  logger.error('Error reading .revu.yml configuration file:', error);

• Validation: There's no validation of the loaded configuration structure beyond checking if codingGuidelines is an array. Consider adding schema validation to ensure the loaded YAML has the expected structure.

• Path handling: The getCodingGuidelines function uses path.join which works differently on different operating systems. Consider using a more robust path normalization approach.

• Deep cloning: The getDefaultConfig function uses a shallow spread operator which might not be sufficient for deep objects:

  // Current implementation
  return { ...DEFAULT_CONFIG };
  
  // Consider a deep clone
  return JSON.parse(JSON.stringify(DEFAULT_CONFIG));
  // or use a library like lodash's cloneDeep

Security Assessment

• File access: The code reads files from the filesystem based on user-provided paths. While there's no direct security vulnerability, ensure that in the broader application context, the repository path is properly validated to prevent directory traversal attacks.

• YAML parsing: Using js-yaml.load without options could potentially lead to code execution if the YAML contains custom types. Consider using js-yaml.safeLoad or setting the schema: yaml.JSON_SCHEMA option to restrict to safe types only.

  // Current implementation
  const config = yaml.load(configContent) as CodingGuidelinesConfig;
  
  // Safer implementation
  const config = yaml.load(configContent, { schema: yaml.JSON_SCHEMA }) as CodingGuidelinesConfig;

Best Practices Evaluation

• Test organization: Tests are well-organized with clear descriptions and proper setup/teardown.

• Documentation: The README update is comprehensive and clearly explains the new feature.

• Error handling: Good error handling with fallbacks to default values.

• Type definitions: Proper use of TypeScript interfaces and types.

• Configuration extensibility: The [key: string]: unknown index signature in the interface allows for future extension.

• Default export: Consider using named exports consistently throughout the codebase instead of mixing default and named exports.

• Configuration validation: As mentioned earlier, adding schema validation would improve robustness.

Recommendations

1. Add configuration validation: Implement schema validation for the YAML configuration to ensure it matches the expected structure.

   function validateConfig(config: unknown): config is CodingGuidelinesConfig {
     if (!config || typeof config !== 'object') return false;
     
     const c = config as Record<string, unknown>;
     if (!Array.isArray(c.codingGuidelines)) return false;
     if (c.reviewSettings && typeof c.reviewSettings !== 'object') return false;
     
     return true;
   }

2. Use a logger abstraction instead of direct console calls to make the code more testable and configurable.

   // In a logger.ts file
   export interface Logger {
     log(message: string, ...args: any[]): void;
     error(message: string, ...args: any[]): void;
   }
   
   export const consoleLogger: Logger = {
     log: console.log,
     error: console.error
   };
   
   // Then inject the logger
   export async function readConfig(
     configPath = path.join(process.cwd(), '.revu.yml'),
     logger = consoleLogger
   ): Promise<CodingGuidelinesConfig> {
     // ...
     logger.error('Error reading .revu.yml configuration file:', error);
     // ...
   }

3. Implement deep cloning for configuration objects to prevent unintended mutations.

4. Add more robust path handling to ensure consistent behavior across operating systems.

Additional Notes

• The implementation is well-tested and should be reliable for the intended use case.
• The configuration system is designed to be extensible, which is good for future enhancements.
• Consider adding a configuration validation command or function that users can run to verify their .revu.yml files before using them in a review.
• In the future, you might want to add more configuration options beyond coding guidelines, and the current structure supports this well.