CI debugging tools: Security and reliability improvements

[justin808]

Security Improvements

1. Remove eval from bin/ci-rerun-failures (Medium Priority)

Current code (line 242):

if eval "$cmd"; then

Issue: Using eval with commands from JOB_MAP. While currently safe (hardcoded), this could become a security risk if JOB_MAP is ever made configurable or loaded from external sources.

Recommendation: Use case statement instead:

run_command() {
  local cmd_type="$1"
  case "$cmd_type" in
    "lint-js-and-ruby")
      bundle exec rubocop && yarn run eslint && yarn start format.listDifferent
      ;;
    "rspec-package-tests")
      bundle exec rake run_rspec:gem
      ;;
    "package-js-tests")
      yarn test
      ;;
    "dummy-app-integration-tests")
      bundle exec rake run_rspec:all_dummy
      ;;
    "examples")
      bundle exec rake run_rspec:shakapacker_examples
      ;;
    *)
      echo "Unknown command type: $cmd_type"
      return 1
      ;;
  esac
}

2. Remove eval from bin/ci-run-failed-specs ✅ FIXED

Already fixed in commit 25f5be6 - now uses array execution.

Reliability Improvements

3. Add Bounds Check for Array Access (Low Priority)

Current code (bin/ci-run-failed-specs:118):

if [[ "${UNIQUE_SPECS[0]}" == *"spec/system"* ]] || [[ "${UNIQUE_SPECS[0]}" == *"spec/helpers"* ]]; then

Issue: Accesses array index without defensive bounds check.

Recommendation:

if [ ${#UNIQUE_SPECS[@]} -gt 0 ] && [[ "${UNIQUE_SPECS[0]}" == *"spec/system"* ]] || [[ "${UNIQUE_SPECS[0]}" == *"spec/helpers"* ]]; then

4. Document Ruby Version Requirement (Low Priority)

Current code (bin/ci-switch-config:134):

ruby script/convert

Issue: Executes ruby before version manager reloads. Might use wrong Ruby version.

Recommendation: Add comment documenting that current Ruby must be compatible, or detect and warn if wrong version.

5. Improve Git Restore Error Handling (Low Priority)

Current code (bin/ci-switch-config:214):

git restore Gemfile.development_dependencies package.json spec/dummy/package.json packages/react-on-rails-pro/package.json 2>/dev/null || true

Issue: Silently fails. User won't know if restoration failed.

Recommendation:

if ! git restore Gemfile.development_dependencies package.json spec/dummy/package.json packages/react-on-rails-pro/package.json 2>/dev/null; then
  print_warning "Some files could not be restored (may not exist in git)"
fi

Usability Improvements

6. Add Dependency Checks ✅ FIXED

Already added in commit 25f5be6:

• bin/ci-rerun-failures checks for gh and jq
• bin/ci-run-failed-specs checks for bundle

7. Add Dry-Run Mode (Nice to Have)

Add --dry-run flag to scripts (especially ci-switch-config) to show what would be executed without actually doing it.

Example:

bin/ci-switch-config minimum --dry-run

Would show:

[DRY RUN] Would create .tool-versions with Ruby 3.2.8, Node 20.18.1
[DRY RUN] Would run: ruby script/convert
[DRY RUN] Would run: yarn install
...

Implementation Priority

1. High: Item 1 (Remove eval from ci-rerun-failures)
2. Medium: Item 4 (Document Ruby version requirement)
3. Low: Items 3, 5, 7

All items are enhancements - the scripts are currently functional and safe for their intended use.

[justin808]

All items from this issue have now been completed! 🎉

Completed in PR #2056:

• ✅ Item 3: Added bounds check for array access in bin/ci-run-failed-specs
• ✅ Item 4: Documented Ruby version requirement in bin/ci-switch-config

Previously completed:

• ✅ Item 1: Removed eval from bin/ci-rerun-failures (replaced with case statement)
• ✅ Item 2: Removed eval from bin/ci-run-failed-specs (uses array execution)
• ✅ Item 5: Improved git restore error handling in bin/ci-switch-config
• ✅ Item 6: Added dependency checks to both scripts

Not implemented (as marked "nice to have"):

• Item 7: Dry-run mode (can be added in a future enhancement if needed)

All security and reliability improvements have been addressed. The scripts are now more robust and maintainable.