Improvements for local gem swapper utility

[justin808]

Local Gem Swapper Improvements

Based on code review, the following improvements should be implemented in future PRs:

High Priority

1. Additional Test Coverage

File: spec/demo_scripts/local_gems_cli_spec.rb (NEW)

Add tests for LocalGemsCLI class covering:

• Option parsing (--github, --branch, --demo, etc.)
• Context detection (running from demo directory)
• Config file application
• Error handling

2. Watch Mode Process Management

File: lib/demo_scripts/gem_swapper.rb:286

Issue: system('npm run watch &') backgrounds processes without tracking. Multiple invocations could leave orphaned processes.

Solution:

• Use spawn with explicit PID tracking
• Store PIDs in ~/.cache/local-gems/watch_pids.json
• Add bin/use-local-gems --kill-watch command to cleanup
• Add bin/use-local-gems --list-watch to show running processes
• Warn users about orphaned processes during --restore

Medium Priority

3. Improved Error Handling for npm install

File: lib/demo_scripts/gem_swapper.rb:248

Issue: system('npm install --silent 2>/dev/null') discards stderr, making troubleshooting difficult.

Solution:

• Use Open3.capture3 to capture stdout/stderr
• Display stderr only on failure
• Optionally add --strict mode that exits on install failures

Low Priority

4. Preserve JSON Formatting

File: lib/demo_scripts/gem_swapper.rb:178

Issue: JSON.pretty_generate may not match original package.json formatting.

Solution:

• Detect original indentation (2 spaces vs 4 spaces vs tabs)
• Preserve trailing newline

5. Enhanced Path Validation

File: lib/demo_scripts/gem_swapper.rb:79

Add explicit validation that expanded paths don't escape expected directories.

6. Git Hook Safety

File: lefthook.yml:46

Issue: Git diff commands could fail if remote branch doesn't exist.

Solution: Add existence checks for remote branches.

Implementation Priority

1. Watch mode process management - Prevents orphaned processes
2. npm install error handling - Improves debugging experience
3. Additional tests - Ensures reliability
4. JSON formatting - Nice-to-have for clean commits
5. Path validation - Defense in depth
6. Git hook safety - Edge case handling

[justin808]

Additional Improvements from Code Review

Security: Command Injection Risk (Medium Priority)

Location: lib/demo_scripts/gem_swapper.rb:150, 155-158

Issue: system() calls with string interpolation could be vulnerable if repository names or branches contain shell metacharacters.

# Current (vulnerable to shell injection):
system("git clone --depth 1 --branch #{info[:branch]} #{repo_url} #{cache_path} 2>&1")

# Better (array form avoids shell):
system('git', 'clone', '--depth', '1', '--branch', info[:branch], repo_url, cache_path, err: [:child, :out])

Solution: Use array form of system() for all git commands to avoid shell interpretation.

UX: Better Error Handling for Git Operations (Medium Priority)

Location: lib/demo_scripts/gem_swapper.rb:148-159

Issue: GitHub clone/update operations give generic error messages. Network issues, authentication problems, and invalid branches all show the same error.

Solution:

• Detect specific git failure modes (network, auth, branch not found)
• Provide helpful error messages with suggestions
• Example: "Branch 'fix-hmr' not found. Available branches: main, develop, v8-stable"

Enhancement: Configurable Cache Directory (Low Priority)

Location: lib/demo_scripts/gem_swapper.rb:20

Issue: Cache directory ~/.cache/local-gems is hardcoded.

Solution:

CACHE_DIR = File.expand_path(ENV.fetch('LOCAL_GEMS_CACHE_DIR', '~/.cache/local-gems'))

This allows users to override via LOCAL_GEMS_CACHE_DIR environment variable.

[justin808]

Summary of All Tracked Improvements

High Priority:

1. ✅ Watch mode process management (PID tracking, cleanup commands)
2. ✅ LocalGemsCLI test coverage
3. ✅ Command injection vulnerability (FIXED in latest commit)

Medium Priority:
4. ✅ Improved npm install error handling (capture stderr, --strict mode)
5. ✅ Better git clone/update error messages

Low Priority:
6. ✅ Preserve JSON formatting in package.json
7. ✅ Enhanced path validation
8. ✅ Git hook safety checks
9. ✅ Configurable cache directory (environment variable)

Test Coverage Gaps:
10. ✅ FilteredGemSwapper class tests
11. ✅ Watch mode behavior tests
12. ✅ GitHub cloning/updating integration tests

All items tracked for future PRs.

[justin808]

Updates after merge

Several items have been addressed:

Completed ✅

1. Watch Mode Process Management - Partially fixed in latest commits:

   • Changed from system('npm', 'run', 'watch', '&') to Process.spawn(...) for proper backgrounding
   • Still needs: PID tracking and cleanup mechanism (remains high priority)

2. npm install Error Handling - Remains as documented (medium priority)

3. LocalGemsCLI Test Coverage - Remains as documented (high priority)

New Issues Created

• Bug: swap-deps generates incorrect Bundler syntax for Git tags #38: Bug with Bundler syntax for Git tags (HIGH priority - functional bug)
• Enhancement: Add cache cleanup command for swap-deps #39: Cache cleanup command (medium priority)

Already Fixed

• Shell injection vulnerabilities fixed (using array form of system())
• Branch/tag name validation added
• GitHub repo format validation added