Background

PR #766 fixed a crash in runAllBuildsCommand by adding validation for incompatible flag combinations. During code review, several additional improvements were identified that would enhance consistency, security, and testability across the config exporter codebase.

This issue tracks those optional enhancements as separate follow-up work.

Proposed Improvements

1. Path Validation for --save-dir in runAllBuildsCommand

Current State:

• run() function validates output paths using safeResolvePath (cli.ts:117-122)
• runAllBuildsCommand() doesn't validate saveDir before use
• This creates an inconsistency in security posture

Proposed:

• Add path validation to runAllBuildsCommand using safeResolvePath
• Ensure saveDir cannot escape the project directory
• Match the security pattern already used in run()

Files: package/configExporter/cli.ts

Priority: Medium (security consistency)

2. Format + Annotate Validation in runAllBuildsCommand

Current State:

• run() validates that --annotate requires YAML format (cli.ts:125)
• runAllBuildsCommand() applies defaults but doesn't validate the combination
• Users could theoretically get confusing behavior with --all-builds --annotate --format=json

Proposed:

• Add validation in runAllBuildsCommand to reject --annotate with non-YAML formats
• Match the validation pattern in run() for consistency

Files: package/configExporter/cli.ts

Priority: Low (UX consistency)

3. Improve Test Error Message Verification

Current State:

• Tests verify that process.exit is called on invalid flag combinations
• Tests don't verify the actual error messages shown to users
• This works because yargs displays errors, but we could have more confidence

Proposed:

• Consider using .exitProcess(false) + .fail() handler in yargs setup
• This would allow throwing errors instead of calling process.exit
• Tests could then assert on exact error messages
• Provides better test coverage of user-facing error text

Files:

• package/configExporter/cli.ts (yargs configuration)
• test/package/configExporter.test.js (test assertions)

Priority: Low (testability improvement)

Overall Priority

Low - These are consistency and quality-of-life improvements, not bugs. The current implementation works correctly.

Implementation Notes

• Each improvement could be addressed independently or together
• Consider whether the testability improvement (NO Merge - Relax ActiveSupport and Rails restrictions #3) provides enough value given the refactoring cost
• Path validation (Update webpack-dev-server #1) has the highest priority due to security consistency

Related

• Fixes Fix potential runtime crash and improve applyDefaults validation #738
• PR Fix potential crash: validate --all-builds with --output #766