Walkthrough

Adds mutual-exclusion validation to CLI argument parsing: disallow --all-builds with --output or --stdout, and disallow --stdout with --output. Adds tests exercising these validation paths (duplicate test insertion observed). No public API signatures changed.

Changes

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant User
  participant CLI as configExporter/cli
  participant Parser as parseArguments
  participant Runner as runEntry

  User->>CLI: invoke with flags
  CLI->>Parser: parseArguments(argv)
  alt conflicting flags
    Parser-->>CLI: throw Error("mutually exclusive")
    CLI->>Runner: catch Error
    Runner->>CLI: print error & return non-zero
  else valid args
    Parser-->>CLI: return ExportOptions
    CLI->>Runner: proceed with execution
  end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

• Review the new mutual-exclusion conditions and error messages for completeness and clarity.
• Verify tests are not duplicated unintentionally and correctly isolate process.exit.
• Confirm runtime path that previously used saveDir! is now protected by these validations.

Possibly related PRs

• Add CLI parser improvements and comprehensive test coverage (#694) #776 — Similar CLI mutual-exclusion validation and exported parseArguments changes.
• Add build validation to export-bundler-config CLI #717 — Related edits to package/configExporter/cli.ts touching flag validation.
• Issue Fix potential runtime crash and improve applyDefaults validation #738 — Reports the runtime crash scenario when --all-builds is used with --output; this PR implements the recommended validation.

Poem

A rabbit checks flags in evening light,
Hops past clashes, sets them right. 🐇
"Pick save-dir," it hums with cheer,
So builds stay tidy, errors steer clear. ✨

Pre-merge checks and finishing touches

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Code Review - PR #766

Summary

This PR successfully addresses issue #738 by preventing a potential runtime crash when using --all-builds with --output. The implementation is solid and follows existing patterns in the codebase.

Strengths

1. Correct Problem Identification

• The root cause analysis is accurate: applyDefaults only sets saveDir when !options.stdout && !options.output (cli.ts:413-432)
• Using --all-builds with --output would cause saveDir! assertion to fail at runtime (line 705)

2. Consistent Implementation

• The validation follows the exact same pattern as other mutually exclusive flag checks (lines 301-340)
• Error message format is consistent with existing validation messages
• Placement in the .check() block is appropriate

3. Error Handling Improvements

• Adding exitProcess(false) to yargs (line 157) is a good practice for testability
• The try-catch block in run() properly handles validation errors (lines 75-82)
• Error messages are clear and actionable ("Use --save-dir instead")

4. Test Coverage

• Tests verify the validation works correctly
• Tests confirm --all-builds with --save-dir is allowed
• Console mocking prevents test output pollution
• Tests appropriately verify error messages rather than implementation details

Observations & Suggestions

1. Test Assertion Strategy (Minor)
The second test ("allows --all-builds with --save-dir") relies on a side effect (config file error) to verify validation didn't trigger:

expect(consoleErrorSpy).toHaveBeenCalledWith(
  expect.stringContaining("Config file")
)

This is pragmatic given the test environment, but it couples the test to unrelated error behavior. The test comment acknowledges this limitation, which is good documentation.

Alternative approach (optional): Mock the file system or create a minimal config file to test the happy path more directly. However, the current approach is acceptable for validating that the specific validation error doesn't occur.

2. TypeScript Type Safety (Enhancement Opportunity - Not blocking)
The PR description and issue #738 mention that resolvedOptions.saveDir! uses a non-null assertion. While this PR prevents the crash through validation, the TypeScript type system still allows this unsafe pattern.

Consider (in a future PR): Create a ResolvedOptions type that makes the relationship between flags and saveDir explicit at compile-time. This would catch similar issues during development rather than requiring runtime validation.

3. Code Organization (Non-issue)
The error handling restructure in run() is well done - separating parsing errors from execution errors makes the control flow clearer.

Code Quality Checklist

• ✅ Best Practices: Follows existing patterns consistently
• ✅ Potential Bugs: No bugs identified; fix prevents the reported crash
• ✅ Performance: No performance impact (validation is O(1))
• ✅ Security: No security concerns
• ✅ Test Coverage: Adequate test coverage for the validation logic
• ✅ CLAUDE.md Compliance:
  • Follows existing code conventions
  • Changes are focused and minimal
  • Tests included for behavior changes

Verification Questions

1. Have you verified that the existing test suite passes with these changes?
2. Have you run yarn lint to ensure JavaScript linting passes? (per CLAUDE.md requirements)

Recommendation

LGTM with confidence ✅

This is a well-implemented fix that:

• Solves the reported issue correctly
• Maintains code consistency
• Includes appropriate tests
• Provides clear error messages to users
• Improves overall robustness with better error handling

The suggestions above are minor enhancements for future consideration and don't block merging this PR.

Review conducted by Claude Code following project guidelines from CLAUDE.md

@claude review this PR

Claude finished @justin808's task —— View job

PR Review for #766: Fix potential crash with --all-builds validation

Summary

This PR successfully addresses issue #738 by preventing a potential runtime crash when using --all-builds with incompatible output flags. The implementation is clean, well-tested, and follows existing code patterns.

Recommendation: ✅ APPROVE

All CI checks are passing, and the implementation is solid.

Detailed Analysis

Problem Statement

The runAllBuildsCommand function (line 739) uses a non-null assertion:

const targetDir = resolvedOptions.saveDir! // Set by applyDefaults

However, applyDefaults only sets saveDir when !options.stdout && !options.output (lines 447-466), causing a potential crash when --all-builds is combined with --output or --stdout.

Solution Quality ✅

1. Validation Logic (cli.ts:355-369)

Three new validations were added to the .check() block:

// Prevents crash from undefined saveDir
if (argv["all-builds"] && argv.output) {
  throw new Error("--all-builds and --output are mutually exclusive. Use --save-dir instead.")
}

// Also prevents undefined saveDir 
if (argv["all-builds"] && argv.stdout) {
  throw new Error("--all-builds and --stdout are mutually exclusive. Use --save-dir instead.")
}

// Prevents ambiguous output mode
if (argv.stdout && argv.output) {
  throw new Error("--stdout and --output are mutually exclusive. Use one or the other.")
}

Strengths:

• ✅ Consistent with existing validation patterns (lines 319-354)
• ✅ Clear, actionable error messages guide users to the correct alternative
• ✅ Placement in .check() ensures validation happens before execution
• ✅ Prevents the crash scenario described in Fix potential runtime crash and improve applyDefaults validation #738
• ✅ The third validation (--stdout + --output) is logically sound even though it wasn't in the original issue

2. Test Coverage (test/package/configExporter.test.js:286-331)

Four comprehensive tests added:

test("rejects --all-builds with --output")
test("rejects --all-builds with --stdout")  
test("rejects --stdout with --output")
test("allows --all-builds with --save-dir")  // Positive case

Strengths:

• ✅ Tests all three invalid combinations
• ✅ Includes positive test case for the intended usage pattern
• ✅ Proper mocking of process.exit prevents test suite termination
• ✅ Clean setup/teardown with beforeEach/afterEach

Minor observation: Tests only verify that process.exit is called, not the specific error messages. This is acceptable since yargs handles error display, but verifying the error messages would provide additional confidence. Not blocking.

CLAUDE.md Compliance ✅

• ✅ Trailing newlines: Both modified files end with trailing newlines
• ✅ Code style: Follows existing patterns in the codebase
• ✅ Test coverage: Comprehensive tests for new validation logic
• ✅ Focused changes: Minimal, targeted changes addressing the specific issue
• ✅ No unnecessary comments: Code is self-documenting

Changelog Assessment

According to CLAUDE.md, this is a user-visible bug fix (prevents a crash) and should have a changelog entry.

Suggested changelog entry (to be added under ## [Unreleased] → ### Fixed):

- **Fix potential crash when using `--all-builds` with incompatible output flags**. [PR #766](https://github.com/shakacode/shakapacker/pull/766) by [justin808](https://github.com/justin808). Fixes [#738](https://github.com/shakacode/shakapacker/issues/738).
  - Validates that `--all-builds` is not used with `--output` or `--stdout` (use `--save-dir` instead)
  - Validates that `--stdout` and `--output` are not used together
  - Prevents runtime crash from undefined `saveDir` in `runAllBuildsCommand`

Code Quality

Positive Observations

1. Excellent error messages: Each validation provides clear guidance on what to do instead
2. Consistent implementation: Follows exact same pattern as existing mutual exclusion checks
3. Comprehensive coverage: All three problematic combinations are addressed
4. Clean test structure: Well-organized test suite with proper isolation

Additional Context from Review Comments

CodeRabbit's reviews provide some suggestions but don't identify blocking issues:

1. Consider exitProcess(false) with .fail() handler (suggestion from 2025-10-29 review)

   • Currently yargs calls process.exit on validation errors
   • Could use .exitProcess(false) + .fail() to throw errors instead
   • Would allow tests to assert exact error messages
   • Status: Optional enhancement, not blocking

2. Annotate + format validation in runAllBuildsCommand (suggestion from 2025-10-29 review)

   • run() validates --annotate requires YAML (line 125)
   • runAllBuildsCommand doesn't have this check
   • Status: Minor inconsistency, but not related to this PR's fix

3. Path validation in runAllBuildsCommand (suggestion from 2025-10-29 review)

   • run() uses safeResolvePath for security (lines 117-122)
   • runAllBuildsCommand doesn't validate saveDir
   • Status: Valid security observation, but separate concern from crash fix

These are all good suggestions for future improvements but are out of scope for this focused bug fix PR.

CI Status ✅

All checks passing:

• ✅ Node based checks
• ✅ Test Both Bundlers
• ✅ ESLint Validation
• ✅ Dummy specs
• ✅ Ruby based checks
• ✅ Generator specs

Minor Observations (Non-blocking)

1. Test Error Message Verification

The tests verify that process.exit is called but don't check the actual error messages:

expect(() => {
  parseArguments(["--all-builds", "--output=config.yml"])
}).toThrow("process.exit called")

Potential enhancement: If adopting .exitProcess(false) + .fail(), tests could verify exact messages:

expect(() => parseArguments(["--all-builds", "--output=config.yml"]))
  .toThrow("--all-builds and --output are mutually exclusive")

This is not blocking since the implementation is clearly correct.

2. Type Safety Consideration

While this PR prevents the crash through runtime validation, TypeScript still allows the unsafe pattern via the non-null assertion saveDir!. A future enhancement could use conditional types to encode the relationship between flags and saveDir at compile-time, but that's beyond the scope of this fix.

Commit History

The PR has 4 well-structured commits:

1. a8a6fa9 - Initial fix for --all-builds + --output
2. d9c2566 - Added missing validations for other combinations
3. 79ef1d2 - Fixed CI by exporting parseArguments
4. ca89c62 - Added positive test case

The iterative approach shows good response to review feedback (particularly commit 4 addressing CodeRabbit's suggestion).

Final Recommendation

✅ APPROVE - This PR is ready to merge.

Strengths

• Correctly identifies and fixes the root cause
• Clean, maintainable implementation
• Comprehensive test coverage
• Follows project conventions
• All CI checks passing

Required Before Merge

• Add changelog entry as suggested above (user-visible bug fix per CLAUDE.md)

Optional Future Enhancements (Separate PRs)

• Consider .exitProcess(false) for better testability
• Add path validation to runAllBuildsCommand (security consistency)
• Add --annotate + format validation to runAllBuildsCommand (UX consistency)
• Consider compile-time type safety for flag relationships

Great work fixing this crash! The implementation is solid and the tests give good confidence that the fix works correctly.

| Branch: fix-output-all-builds-crash