feat: Robustness, error handling, and test coverage (continuation of #47) - #48
Conversation
…wait for file operations
WalkthroughThe changes convert all file and environment management operations from synchronous to asynchronous patterns, update method names for clarity, and modularize secret handling and error reporting in the CLI application. New infrastructure is added for reading the package version, with corresponding asynchronous tests and workflow updates. Minor improvements are made to scripts, test setup, and repository metadata. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI (main)
participant PackageJsonFinder
participant Envilder
participant EnvFileManager
User->>CLI (main): Run CLI command
CLI (main)->>PackageJsonFinder: readPackageJsonVersion()
PackageJsonFinder-->>CLI (main): version string
CLI (main)->>Envilder: run(mapPath, envFilePath)
Envilder->>EnvFileManager: loadMapFile(mapPath)
EnvFileManager-->>Envilder: paramMap
Envilder->>EnvFileManager: loadEnvFile(envFilePath)
EnvFileManager-->>Envilder: existingEnvVariables
Envilder->>Envilder: envild(paramMap, existingEnvVariables)
Envilder->>Envilder: processSecret() for each secret
Envilder->>EnvFileManager: saveEnvFile(envFilePath, updatedEnvVariables)
EnvFileManager-->>Envilder: (done)
Envilder-->>CLI (main): (done)
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
macalbert
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @macalbert, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello team, Gemini here with a summary of this pull request. This PR, a continuation of work from #47, focuses on significantly improving the robustness, error handling, and test coverage of the Envilder CLI tool. The core changes involve refactoring the CLI entrypoint for clearer argument parsing, enhancing the main handler to provide better error messages and secret masking, improving the environment file manager to handle special characters and file operation errors more reliably, and expanding the test suite to cover these new error and edge cases. The overall goal is to make the CLI more reliable, user-friendly, maintainable, and secure.
Highlights
- Enhanced File Handling: The environment file manager has been updated to use asynchronous file operations and includes improved logic for escaping special characters (like backslashes, newlines, and quotes) when writing
.envfiles, ensuring cross-platform consistency. - Code Refactoring: Key components like the CLI entrypoint (
Cli.ts) and the main handler (EnvilderHandler.ts) have been refactored for better clarity, separation of concerns, and maintainability. Asynchronous operations are now properly handled usingasync/await. - Expanded Test Coverage: New unit and end-to-end tests have been added to specifically target argument validation, file handling edge cases (including special characters), and error scenarios, increasing confidence in the tool's reliability.
- Secret Masking in Logs: Logging of fetched secret values now includes masking, showing only the last few characters for values longer than 10 characters, improving security when running the tool.
Changelog
Click here to see the changelog
- src/Cli.ts
- Removed inline
findPackageJsonfunction and directpackage.jsonreading (lines 2-44). - Introduced
PackageJsonFinderfor reading the package version asynchronously (lines 6, 10, 38-44). - Updated
mainfunction to useasync/awaitfor getting the version (line 10).
- Removed inline
- src/cli/application/EnvilderHandler.ts
- Refactored
runmethod to useasync/awaitfor file operations and added atry...catchblock for overall error handling (lines 13-30). - Renamed internal method
fetchAndUpdateEnvVariablestoenvild(line 32). - Extracted logic for processing individual secrets into a new private
processSecretmethod (lines 55-74). - Improved error reporting for failed parameter fetches within the loop (line 73).
- Refactored
- src/cli/domain/ports/IEnvFileManager.ts
- Updated interface methods to be asynchronous (
Promise) (lines 2-7). - Renamed
loadParamMaptoloadMapFile(line 2). - Renamed
loadExistingEnvVariablestoloadEnvFile(line 3). - Renamed
writeEnvFiletosaveEnvFileand updated signature to returnPromise<void>(lines 4-7).
- Updated interface methods to be asynchronous (
- src/cli/infrastructure/EnvFileManager.ts
- Switched from synchronous
node:fsto asynchronousnode:fs/promises(line 1). - Implemented
loadMapFileusingawait fs.readFileand added JSON parsing error handling (lines 6-13). - Implemented
loadEnvFileusingawait fs.accessandawait fs.readFile, handling non-existent files gracefully (lines 16-27). - Implemented
saveEnvFileusingawait fs.writeFile(lines 29-37). - Moved string escaping logic to a private
escapeEnvValuehelper method (lines 39-44).
- Switched from synchronous
- src/cli/infrastructure/PackageJsonFinder.ts
- Added new file
PackageJsonFinder.ts. - Implemented
PackageJsonFinderclass with an asynchronousreadPackageJsonVersionmethod (lines 3-32). - Includes error handling for file not found (
ENOENT), invalid JSON, and missing version field (lines 15-23).
- Added new file
- tests/Cli.e2e.test.ts
- Added cleanup logic to remove
envilder-*.tgzfiles generated during the test run (lines 132-147).
- Added cleanup logic to remove
- tests/Cli.unit.test.ts
- Updated mock
EnvFileManagermethods to be asynchronous (vi.fn(async () => ...)orvi.fn(async () => {})) to match the interface changes (lines 29-31). - Added a new unit test
Should_ThrowError_When_ArgumentsAreInvalidsto verify Commander's argument validation (lines 75-101).
- Updated mock
- tests/cli/infrastructure/EnvFileManager.test.ts
- Updated tests to use
async/awaitfor file operations andafterEachcleanup (lines 12-20, 29, 49, 65, 80, 95, 113). - Renamed test descriptions to reflect the new method names (
loadMapFile,saveEnvFile) (lines 29, 44, 59, 74, 89, 104).
- Updated tests to use
- tests/cli/infrastructure/PackageJsonFinder.test.ts
- Added new file
PackageJsonFinder.test.ts. - Added unit tests for
PackageJsonFinder, covering successful version reading, file not found, missing version field, and invalid JSON (lines 6-78).
- Added new file
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Pull Request Overview
This PR continues improvements to the Envilder CLI tool by refactoring error handling, enhancing secret masking, and expanding test coverage.
- Refactors the CLI entrypoint and underlying handler for improved argument parsing and error messaging.
- Enhances file management in the environment file manager with asynchronous operations and robust string escaping.
- Expands unit and end-to-end tests to cover error scenarios and special character handling.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/cli/infrastructure/PackageJsonFinder.test.ts | Adds async tests for reading package.json version using the new PackageJsonFinder. |
| tests/cli/infrastructure/EnvFileManager.test.ts | Updates tests to use asynchronous file operations and new method names. |
| tests/Cli.unit.test.ts | Adjusts mocks to match updated async methods and adds a negative test for arguments. |
| tests/Cli.e2e.test.ts | Enhances cleanup logic by deleting generated tgz files cross-platform. |
| src/cli/infrastructure/PackageJsonFinder.ts | Implements async version of package.json version reader. |
| src/cli/infrastructure/EnvFileManager.ts | Refactors file operations to async, renames methods, and improves string escaping. |
| src/cli/domain/ports/IEnvFileManager.ts | Updates interface to reflect asynchronous methods. |
| src/cli/application/EnvilderHandler.ts | Improves error handling and secret processing with enhanced error messages. |
| src/Cli.ts | Refactors version detection and updates the CLI error messaging style. |
Comments suppressed due to low confidence (1)
src/Cli.ts:47
- [nitpick] Consider using a more conventional and professional error message to maintain consistency with the rest of the codebase.
console.error('🚨 Uh-oh! Looks like Mario fell into the wrong pipe! 🍄💥');
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly enhances the Envilder CLI's robustness, error handling, and test coverage. The refactoring of argument parsing, the core handler, and the environment file manager are well-executed. The move to asynchronous file operations and the addition of comprehensive tests are particularly commendable. The code is generally clear and well-structured. I've identified a few areas for potential improvement, mostly related to documentation and minor points of clarity, detailed in the comments below.
Summary of Findings
- Documentation: JSDoc comments were removed from
Cli.ts#mainandEnvilderHandler.ts#run. Restoring these would improve code maintainability. - Naming Convention: The method name
envildinEnvilderHandler.tsis unconventional and could be clearer. - Error Reporting: The error message for JSON parsing failures in
EnvFileManager.tscould include more details from the original error to aid debugging. - Module System Consistency: The e2e test file
tests/Cli.e2e.test.tsusesrequire('glob'), which is a CommonJS import in an otherwise ESM-style project. Using dynamicimport()or an ESM alternative would be more consistent. - Test Helper Discrepancy (Low Severity - Not Commented): The
escapeForEnvFilehelper intests/cli/infrastructure/EnvFileManager.test.tshandles only\nfor newlines, while the implementation'sescapeEnvValuecorrectly handles\r\n,\n, and\r. This is a minor point asdotenv.parseis robust, but aligning the test helper or adding specific test cases for CRLF/CR could be considered for completeness. (Not commented due to severity filter).
Merge Readiness
The pull request is in good shape with significant improvements. However, there are a few medium-severity suggestions related to documentation, naming, error reporting, and module consistency that I recommend addressing before merging. These changes will further enhance the quality and maintainability of the codebase. As an AI, I am not authorized to approve pull requests; please ensure further review and approval from team members.
![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
🧹 Nitpick comments (2)
src/cli/infrastructure/PackageJsonFinder.ts (1)
8-8: Consider more precise type annotation for better type safety.The current type
{ version?: unknown }is too loose. Consider using a more specific interface for better type safety and IDE support.- const pkg: { version?: unknown } = JSON.parse(content); + const pkg: { version?: string; [key: string]: unknown } = JSON.parse(content);tests/Cli.e2e.test.ts (1)
133-133: Use proper import statement instead of require().Consider importing
globat the top of the file for consistency with other imports.+import { glob } from 'glob';Then update the usage:
- const glob = require('glob'); - const tgzFiles = glob.sync(join(rootDir, 'envilder-*.tgz')); + const tgzFiles = glob.sync(join(rootDir, 'envilder-*.tgz'));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/Cli.ts(2 hunks)src/cli/application/EnvilderHandler.ts(1 hunks)src/cli/domain/ports/IEnvFileManager.ts(1 hunks)src/cli/infrastructure/EnvFileManager.ts(1 hunks)src/cli/infrastructure/PackageJsonFinder.ts(1 hunks)tests/Cli.e2e.test.ts(1 hunks)tests/Cli.unit.test.ts(2 hunks)tests/cli/infrastructure/EnvFileManager.test.ts(2 hunks)tests/cli/infrastructure/PackageJsonFinder.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
tests/cli/infrastructure/PackageJsonFinder.test.ts (1)
src/cli/infrastructure/PackageJsonFinder.ts (1)
PackageJsonFinder(3-32)
src/Cli.ts (1)
src/cli/infrastructure/PackageJsonFinder.ts (1)
PackageJsonFinder(3-32)
tests/Cli.unit.test.ts (3)
src/cli/application/EnvilderHandler.ts (1)
Envilder(4-76)src/cli/domain/EnvilderBuilder.ts (1)
EnvilderBuilder(10-56)src/Cli.ts (1)
main(8-36)
tests/cli/infrastructure/EnvFileManager.test.ts (1)
src/cli/infrastructure/EnvFileManager.ts (1)
EnvFileManager(5-45)
src/cli/infrastructure/EnvFileManager.ts (1)
src/cli/domain/ports/IEnvFileManager.ts (1)
IEnvFileManager(1-8)
🪛 GitHub Check: CodeQL
tests/Cli.e2e.test.ts
[warning] 138-138: Shell command built from environment values
This shell command depends on an uncontrolled file name.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: CodeQL
- GitHub Check: codeql-fortress (typescript)
- GitHub Check: powerup-test
🔇 Additional comments (20)
src/cli/infrastructure/PackageJsonFinder.ts (2)
4-24: Well-structured async implementation with good error handling.The method correctly uses async file operations, validates the version field type, and provides specific error messages for different failure scenarios. The ENOENT error detection is a nice touch for providing user-friendly messages.
26-31: Solid error detection utility method.The
isEnoentErrorhelper method properly handles unknown error types and safely checks for the ENOENT error code. Good defensive programming practice.tests/cli/infrastructure/PackageJsonFinder.test.ts (1)
1-78: Excellent test coverage and structure.The test suite comprehensively covers all scenarios:
- Valid package.json with version field
- Missing package.json file
- Missing version field
- Invalid JSON content
The setup/teardown logic ensures proper test isolation, and the async/await patterns are correctly implemented throughout.
tests/Cli.unit.test.ts (2)
29-31: Properly updated mocks for async file operations.The file manager mocks have been correctly updated to use the new async method names (
loadMapFile,loadEnvFile,saveEnvFile) and return promises as expected.
75-101: Good addition of error handling test case.The new test verifies that the CLI properly handles invalid arguments (missing values for required options) and ensures that the core business logic methods are not called in error scenarios. The regex pattern covers multiple potential error types, making the test robust.
src/Cli.ts (3)
4-6: Good addition of ES modules path handling imports.The new imports properly support ES modules path resolution and the PackageJsonFinder integration.
8-10: Well-executed async refactoring.Making the main function async and using
await getVersion()maintains consistency with the overall async pattern adopted throughout the codebase.
38-45: Clean separation of concerns for version retrieval.The
getVersionfunction properly encapsulates the version reading logic using the newPackageJsonFinderclass. The path resolution using ES modules APIs is correct and the relative path topackage.jsonis appropriate.tests/cli/infrastructure/EnvFileManager.test.ts (3)
3-4: LGTM: Proper async import patternThe import change to
node:fs/promisescorrectly aligns with the asynchronous file operations used throughout the tests.
12-20: Excellent async cleanup patternThe
afterEachhook properly handles asynchronous file cleanup with appropriate error handling. The try-catch blocks silently ignore errors for non-existent files, which is the correct behavior for cleanup operations.
104-117: LGTM: Proper async parameter map testThe test correctly uses async file writing with
fs.writeFileand properly awaits theloadMapFilemethod call. The test logic and assertions are sound.src/cli/domain/ports/IEnvFileManager.ts (1)
2-7: LGTM: Excellent interface modernizationThe interface changes represent a solid improvement:
- Async conversion: All methods now return Promises, enabling non-blocking I/O operations
- Improved naming:
loadParamMap→loadMapFile(more generic and clear)loadExistingEnvVariables→loadEnvFile(concise and descriptive)writeEnvFile→saveEnvFile(follows common save/load naming convention)- Consistent signatures: All methods follow consistent parameter and return type patterns
These changes align well with modern Node.js async patterns and improve code readability.
src/cli/infrastructure/EnvFileManager.ts (5)
1-1: LGTM: Proper async importThe import change to
node:fs/promisescorrectly supports the asynchronous file operations used throughout the class.
6-14: LGTM: Solid async JSON loading with proper error handlingThe method properly handles asynchronous file reading and includes appropriate error handling with typed error variables (
err: unknown). The error messages are informative and help with debugging.
16-27: LGTM: Excellent async file existence and loading patternThe implementation correctly uses
fs.access()for asynchronous file existence checking instead of the synchronousfs.existsSync(). The graceful handling of non-existent files by returning an empty object is appropriate for environment file loading.
29-37: LGTM: Clean async file saving with proper escapingThe method correctly implements asynchronous file writing and properly calls the extracted
escapeEnvValuemethod for value sanitization.
39-44: LGTM: Comprehensive escaping logic for .env filesThe extracted
escapeEnvValuemethod properly handles the key special characters that need escaping in.envfiles:
- Backslashes (
\\→\\\\)- Various newline formats (
\r\n,\n,\r→\\n)- Double quotes (
"→\\")The regex pattern
(\r\n|\n|\r)correctly handles different line ending formats across platforms.src/cli/application/EnvilderHandler.ts (3)
14-30: LGTM: Robust async orchestration with comprehensive error handlingThe
runmethod properly orchestrates the async file operations with excellent error handling:
- Correctly awaits all async
envFileManagermethod calls- Comprehensive try-catch block with detailed error logging
- Proper error re-throwing to maintain error propagation
- Clear success logging with file path information
The error message construction handles both
Errorinstances and other types gracefully.
32-53: LGTM: Well-structured secret processing with batch error handlingThe
envildmethod demonstrates excellent error handling patterns:
- Processes all secrets and collects errors rather than failing fast
- Provides comprehensive error reporting with all failed parameters
- Returns the mutated
existingEnvVariablesobject as expected- Clear separation of concerns with delegation to
processSecretThis approach ensures users get complete feedback about all missing parameters rather than stopping at the first failure.
55-76: LGTM: Excellent secret processing with smart value maskingThe
processSecretmethod implements several best practices:
- Security-conscious logging: Values are masked with asterisks, showing only the last 3 characters for values longer than 10 characters
- Proper error handling: Catches errors and returns descriptive error messages rather than throwing
- Appropriate warnings: Logs warnings for missing values without treating them as errors
- Mutation pattern: Directly updates the
existingEnvVariablesobject, which is efficient and clearThe masking logic
value.length > 10 ? '*'.repeat(value.length - 3) + value.slice(-3) : '*'.repeat(value.length)provides a good balance between security and debugging utility.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/Cli.e2e.test.ts (1)
122-154: Great security improvement, but consider simplifying the glob usage.The refactoring successfully addresses the shell injection vulnerability by using Node.js fs APIs instead of shell commands. However, the glob usage can be simplified.
Consider using the modern promise-based glob API instead of wrapping the callback:
- // Delete envilder-*.tgz files - const tgzFiles = await new Promise<string[]>((resolve, reject) => { - glob(join(rootDir, 'envilder-*.tgz'), (err, matches) => { - if (err) { - return reject(err); - } - return resolve(matches); - }); - }); + // Delete envilder-*.tgz files + const tgzFiles = await glob(join(rootDir, 'envilder-*.tgz'));Also, the comment on line 128 should be updated for clarity:
- // Ignore errors for individual file deletions + // Ignore errors if lib directory doesn't exist
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.json(2 hunks)src/Cli.ts(3 hunks)src/cli/application/EnvilderHandler.ts(1 hunks)tests/Cli.e2e.test.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Cli.ts
- src/cli/application/EnvilderHandler.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: codeql-fortress (typescript)
- GitHub Check: powerup-test
🔇 Additional comments (2)
tests/Cli.e2e.test.ts (2)
7-8: LGTM! Good imports for async file operations.The addition of async file system operations (
rm,unlink) and theglobpackage aligns well with the async refactoring goals and addresses the previous security concerns.
15-16: LGTM! Proper async/await implementation for test hooks.The test setup and teardown hooks are correctly converted to async functions that properly await the cleanup operations.
Also applies to: 28-29
…ipt and TypeScript
…ependency test: reduce timeout for E2E tests and simplify glob usage in cleanup function
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/Cli.test.ts (2)
75-101: Improve test robustness and specificity.The new test case provides good coverage for invalid argument scenarios, but consider these improvements:
- Error assertion specificity: The regex pattern
/required option|process\.exit called|CommanderError/iis overly broad and could mask different error types.- Global state manipulation: Direct manipulation of
process.argvcould cause issues in parallel test execution.Consider this refactor for more robust testing:
- it('Should_ThrowError_When_ArgumentsAreInvalids', async () => { - // Arrange - process.argv = [ - 'node', - 'cli.js', - '--map', - // missing map file argument - '--envfile', - // missing envfile argument - ]; + it('Should_ThrowError_When_ArgumentsAreInvalids', async () => { + // Arrange + const originalArgv = process.argv; + process.argv = [ + 'node', + 'cli.js', + '--map', + '--envfile', + ]; const envilderSpy = vi.spyOn(Envilder.prototype, 'run'); const withAwsProviderSpy = vi.spyOn( EnvilderBuilder.prototype, 'withAwsProvider', ); - // Act - const action = main(); - - // Assert - await expect(action).rejects.toThrow( - /required option|process\.exit called|CommanderError/i, - ); + try { + // Act + const action = main(); + + // Assert + await expect(action).rejects.toThrow(); + } finally { + process.argv = originalArgv; + } expect(envilderSpy).not.toHaveBeenCalled(); expect(withAwsProviderSpy).not.toHaveBeenCalled(); - withAwsProviderSpy.mockRestore(); });
72-72: Remove redundant mock restoration.The
withAwsProviderSpy.mockRestore()calls are redundant sincevi.restoreAllMocks()is already called inafterEach().expect(withAwsProviderSpy).toHaveBeenCalledWith('test-profile'); - withAwsProviderSpy.mockRestore(); });expect(withAwsProviderSpy).not.toHaveBeenCalled(); - withAwsProviderSpy.mockRestore(); });Also applies to: 100-100
tests/e2e.test.ts (1)
122-147: Excellent refactoring to async operations with robust error handling.The cleanUpSystem function has been well-refactored to use async fs operations instead of shell commands, which improves:
- Cross-platform compatibility by eliminating platform-dependent shell commands
- Error resilience with individual try-catch blocks for file operations
- Code clarity by using native Node.js APIs
The approach of ignoring individual file deletion errors is appropriate for cleanup operations, and keeping the npm uninstall as sync is correct since npm doesn't provide async APIs.
One minor suggestion for better debugging:
- } catch { - // Ignore errors for individual file deletions - } + } catch (error) { + // Ignore errors for individual file deletions + console.debug(`Failed to delete ${file}:`, error.message); + }.github/workflows/tests.yml (1)
3-7: Ensure least-privilege on workflow token permissions.The added
pull-requests: writescope may be broader than necessary if none of your steps update PR metadata. Consider removing any unused permissions to adhere to the principle of least privilege.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
.gitattributes(1 hunks).github/workflows/tests.yml(2 hunks)package.json(2 hunks)src/cli/domain/EnvilderBuilder.ts(1 hunks)tests/Cli.test.ts(2 hunks)tests/cli/application/EnvilderHandler.test.ts(1 hunks)tests/cli/infrastructure/AwsSsmSecretProvider.test.ts(1 hunks)tests/cli/infrastructure/EnvFileManager.test.ts(1 hunks)tests/e2e.test.ts(2 hunks)
✅ Files skipped from review due to trivial changes (4)
- tests/cli/application/EnvilderHandler.test.ts
- src/cli/domain/EnvilderBuilder.ts
- .gitattributes
- tests/cli/infrastructure/AwsSsmSecretProvider.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- tests/cli/infrastructure/EnvFileManager.test.ts
🔇 Additional comments (8)
tests/Cli.test.ts (2)
22-22: LGTM: Cleaner test suite naming.The simplified test suite name improves readability and follows testing best practices.
29-31: LGTM: Mock interface correctly updated for async patterns.The mock methods now align with the refactored asynchronous interface, using the updated method names and async/await patterns.
tests/e2e.test.ts (4)
7-8: LGTM! Good addition of async fs operations.The imports align well with the async refactoring across the codebase and improve cross-platform compatibility by replacing shell commands with native Node.js APIs.
14-14: LGTM! Cleaner test suite naming.The simplified test suite name is more concise while maintaining clarity.
28-29: LGTM! Consistent async pattern.The afterAll function correctly awaits the async cleanUpSystem function, maintaining consistency with the beforeAll changes.
15-16:Details
❓ Verification inconclusive
Verify the timeout reduction doesn't cause test failures.
The conversion to async is correct, but the timeout reduction from 120s to 30s is significant. Ensure this provides sufficient time for the build and install operations, especially in CI environments or slower systems.
Also applies to: 22-22
🏁 Script executed:
#!/bin/bash # Description: Check if there are any references to timing issues in CI or documentation # Expected: Find any mentions of build times or timeout configurations rg -i "timeout|build.*time|slow.*build" --type md rg -i "timeout.*test|test.*timeout" --type ts --type jsLength of output: 109
Confirm 30s timeout sufficiency for E2E tests
I wasn’t able to find any existing docs or CI configs that reference test timeouts—please manually verify that reducing thebeforeAlltimeout from 120 s to 30 s in tests/e2e.test.ts (around lines 15–16) won’t lead to failures on slower machines or in CI..github/workflows/tests.yml (2)
64-66: Use the new CI test script.Switching to
npm run test:cialigns with the updatedpackage.jsonand ensures tests run with JUnit reporting enabled.
67-73: Upload JUnit test results unconditionally.The
dorny/test-reporterstep is correctly configured to run always and publish./coverage/junit/test-results.xmlin thejest-junitformat.
Description
This PR continues the work started in PR #47, introducing further improvements and refactorings to the Envilder CLI tool.
What’s Introduced
src/Cli.ts) for clearer argument parsing and error handling.EnvilderHandler) to provide better error messages, warnings for missing secrets, and improved secret value masking in logs.EnvFileManager) to robustly handle special characters (backslashes, newlines, quotes) when reading and writing.envfiles, and improved error handling for file operations.IEnvFileManagerinterface for clarity and consistency.Why These Changes
These changes make the Envilder CLI more robust, user-friendly, maintainable, and secure for generating environment files from AWS SSM parameters, with clearer feedback and better test coverage.
Summary by CodeRabbit
New Features
Refactor
Tests
Chores