Add cache management commands to swap-deps (#39)

[justin808]

Summary

Adds cache management functionality to the bin/swap-deps utility to help manage disk space used by GitHub repositories cloned with the --github option.

Changes

• New CLI options:

  • --show-cache: Display cache location, total size, and list of cached repositories
  • --clean-cache: Remove all cached repositories
  • --clean-cache <gem>: Remove cache for a specific gem (e.g., shakapacker, react_on_rails)

• Implementation:

  • Added show_cache_info method to display cache statistics with human-readable sizes
  • Added clean_cache method with optional gem-specific filtering
  • Added helper methods for directory size calculation and formatting
  • Respects --dry-run flag for safe preview of cleanup operations
  • Excludes non-repo directories like watch_logs from cache operations

Usage Examples

# Show cache information
bin/swap-deps --show-cache

# Clean all cached repositories
bin/swap-deps --clean-cache

# Clean cache for specific gem
bin/swap-deps --clean-cache shakapacker

# Preview cleanup with dry-run
bin/swap-deps --clean-cache --dry-run

Test Plan

• Tested --show-cache with empty cache
• Tested --show-cache with cached repositories
• Tested --clean-cache to remove all repos
• Tested --clean-cache <gem> to remove specific gem repos
• Tested --dry-run flag with cleanup operations
• Verified help text includes new options
• Ran RuboCop - all checks pass
• Pre-commit hooks pass

Fixes #39

🤖 Generated with Claude Code

[coderabbitai]

Warning

Rate limit exceeded

@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 5 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 073878c and 7f7a1ea.

📒 Files selected for processing (3)

• lib/demo_scripts/gem_swapper.rb (3 hunks)
• lib/demo_scripts/swap_deps_cli.rb (6 hunks)
• spec/demo_scripts/gem_swapper_spec.rb (1 hunks)

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch add-cache-management

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[claude]

PR Review - Add Cache Management Commands to swap-deps (#44)

Summary

This PR adds cache management functionality to the bin/swap-deps utility, introducing three new commands: --show-cache, --clean-cache, and --clean-cache . The implementation is well-structured and follows the existing codebase patterns.

---

✅ Strengths

1. Good Feature Design

   • The cache management commands are intuitive and follow Unix-style patterns
   • --dry-run support is properly implemented for safety
   • Human-readable size formatting improves UX significantly

2. Code Quality

   • Follows existing code structure and conventions well
   • Proper use of RuboCop directives where complexity is justified
   • Good error handling with graceful degradation (returns 0 on StandardError in directory_size)

3. Documentation

   • Comprehensive PR description with usage examples
   • Help text properly updated with new commands
   • Clear test plan checklist

4. Security

   • Proper filtering to exclude watch_logs directory from cache operations
   • Pattern matching uses safe glob operations

---

🔍 Observations & Minor Suggestions

1. Potential Race Condition in show_cache_info (lib/demo_scripts/gem_swapper.rb:146-169)

The method iterates over directories twice using Dir.glob. If the cache is modified between these two iterations (e.g., by another process), the output could be inconsistent. Consider storing the results from the first iteration and reusing them.

2. Error Handling in directory_size (lib/demo_scripts/gem_swapper.rb:211-219)

Silently returning 0 on any error could mask permission issues or other filesystem problems. Consider logging a warning or being more specific about caught exceptions (e.g., Errno::EACCES, Errno::ENOENT).

3. Glob Pattern Matching for Gem-Specific Cleanup (lib/demo_scripts/gem_swapper.rb:233)

The pattern #{gem_name} could match unintended directories. For example, --clean-cache shake might match both shakapacker and shake-something-else. Consider more precise matching or adding validation.

4. Cyclomatic Complexity in run! Method (lib/demo_scripts/swap_deps_cli.rb:36-70)

The method now has 4 complexity metrics disabled. While the implementation is clear, consider extracting the command dispatch logic into a separate method for better maintainability.

5. Missing Test Coverage

The PR description mentions tests were run manually, but I do not see unit tests added for the new cache management methods (show_cache_info, clean_cache, clean_gem_cache, clean_all_cache). Consider adding RSpec tests similar to existing tests in spec/demo_scripts/gem_swapper_spec.rb.

---

🐛 Potential Bugs

Issue: Find.find Can Follow Symlinks

In directory_size (lib/demo_scripts/gem_swapper.rb:213), Find.find can follow symlinks which could:

• Calculate incorrect sizes (counting external files)
• Cause infinite loops (with circular symlinks)
• Raise exceptions

Recommendation: Add symlink detection with Find.prune if File.symlink?(file_path)

---

🔒 Security Considerations

1. Path Traversal (Low Risk) - The watch_logs exclusion is good, but ensure the pattern #{gem_name} in line 233 cannot be exploited with special characters. The existing validation in validate_github_repos should prevent this, but it only validates GitHub repo formats, not user-provided gem names for cleanup.

2. FileUtils.rm_rf Usage - Correctly used with validated paths. The dry-run check is properly implemented.

---

📊 Performance Considerations

• directory_size using Find.find is appropriate for this use case
• Multiple glob operations in show_cache_info could be optimized (see observation Reorganize into monorepo structure #1)
• For very large repositories, consider showing a progress indicator

---

✅ Test Coverage Assessment

Manual Testing: Comprehensive per PR description ✓
Unit Tests: Not added for new cache methods ⚠️
Recommendation: Add RSpec tests covering:

• show_cache_info with empty/populated cache
• clean_cache with/without dry-run
• clean_gem_cache with various patterns
• Edge cases (missing cache dir, permission errors)

---

🎯 Overall Assessment

Quality: 8.5/10

This is a solid, well-implemented feature that adds valuable functionality. The code follows existing patterns, includes proper safety measures (dry-run, backup exclusion), and provides good UX. The main areas for improvement are:

1. Add unit tests for new cache management functionality
2. Address the potential symlink issue in directory_size
3. Consider more specific error handling instead of catching all StandardError

Great work! The feature addresses issue #39 effectively and the implementation is production-ready with minor refinements.

---

Review completed by Claude Code

[claude]

PR Review: Add cache management commands to swap-deps (#39)

Summary

This PR adds comprehensive cache management functionality to the bin/swap-deps utility. The implementation includes display, cleanup, and maintenance features for GitHub repository caches. Overall, this is a well-implemented feature with good attention to security and robustness based on the second commit improvements.

---

Strengths

Code Quality

• Excellent security measures: Input validation for gem names prevents path traversal attacks (lib/demo_scripts/gem_swapper.rb:247-250)
• Good error handling: Specific exception handling for filesystem operations (EACCES, ENOENT) with informative warnings (lib/demo_scripts/gem_swapper.rb:223-231)
• Race condition fix: Caching Dir.glob results prevents TOCTTOU issues in show_cache_info (lib/demo_scripts/gem_swapper.rb:144)
• Symlink handling: Properly detects and skips symlinks to avoid circular references (lib/demo_scripts/gem_swapper.rb:215-217)
• Dry-run support: Respects --dry-run flag throughout cache operations
• Code reuse: Good extraction of cache_repo_dirs helper method to reduce duplication (lib/demo_scripts/gem_swapper.rb:203-209)

User Experience

• Clear output: Human-readable sizes and informative emoji-based messages
• Flexible cleanup: Supports both full cache cleanup and gem-specific cleanup
• Good documentation: Well-documented help text with examples (lib/demo_scripts/swap_deps_cli.rb:261-268)

Best Practices

• Consistent naming: Renamed to cache_repo_dirs following Ruby conventions
• Pattern matching: Uses both normalized (hyphens) and original (underscores) gem name patterns (lib/demo_scripts/gem_swapper.rb:255-260)
• Detailed comments: Explains security measures and design decisions

---

Issues and Suggestions

1. Performance Consideration (Minor)

Location: lib/demo_scripts/gem_swapper.rb:292, 297-299

The clean_all_cache method calculates directory sizes twice - once for total_size and again in the loop. Consider caching the sizes like in show_cache_info for better performance.

2. Potential Edge Case in Gem Name Matching (Minor)

Location: lib/demo_scripts/gem_swapper.rb:256-260

The gem name matching pattern could potentially match unintended directories. However, given that only SUPPORTED_GEMS can be cached, this is likely not a practical concern.

3. Missing Test Coverage for Edge Cases (Suggestion)

While the PR description mentions comprehensive testing, consider adding automated tests for:

• Behavior when cache directory exists but is empty
• Behavior when a gem name does not match any cached repos
• Security: Attempting to clean with invalid gem names
• Race conditions: Concurrent cache operations
• Symlinks in cache directories

4. Documentation (Very Minor)

Location: lib/demo_scripts/gem_swapper.rb:136, 166

Consider adding brief comments indicating that show_cache_info and clean_cache are CLI entry points.

---

Security Assessment

Excellent security practices:

• Input validation prevents path traversal attacks
• Uses Regexp.escape to prevent regex injection
• Validates gem names against strict pattern
• Only operates on directories within CACHE_DIR
• Excludes non-repo directories like watch_logs

No security concerns identified.

---

Performance Assessment

• Good: Uses Find.find for efficient directory traversal
• Good: Short-circuits on symlinks to avoid unnecessary work
• Minor issue: Double calculation of directory sizes in clean_all_cache
• Good: Human-readable size calculation is efficient

---

Test Coverage

The PR description indicates comprehensive testing. All manual tests pass, RuboCop passes, and pre-commit hooks pass.

Suggestion: Add automated tests for the security validations and edge cases.

---

Final Recommendation

Approve with minor suggestions. This PR is well-implemented with excellent security practices and good code quality. The suggestions above are minor optimizations and do not block merging.

Key takeaways:

1. The second commit addressed important security and robustness concerns
2. Code follows Ruby conventions and repository standards
3. User experience is well-designed with clear feedback
4. Minor performance optimization opportunity in clean_all_cache

Great work!

[claude]

Pull Request Review: Add cache management commands to swap-deps (#39)

Summary

This PR adds cache management functionality to the bin/swap-deps utility with three new commands: --show-cache, --clean-cache, and --clean-cache <gem>. Overall, this is a well-implemented feature with good attention to security, error handling, and user experience.

✅ Strengths

Code Quality

• Excellent security practices: Input validation with regex pattern /\A[\w.-]+\z/ prevents path traversal attacks in clean_gem_cache (lib/demo_scripts/gem_swapper.rb:250)
• Robust error handling: Multiple rescue blocks for Errno::EACCES, Errno::ENOENT, and general StandardError in directory_size (lib/demo_scripts/gem_swapper.rb:226-234)
• Good separation of concerns: Public CLI entry points delegate to well-focused private methods
• Consistent coding style: Follows RuboCop conventions and existing patterns in the codebase
• Safe file operations: Proper handling of symlinks to avoid circular references (lib/demo_scripts/gem_swapper.rb:218-220)

Performance Considerations

• Efficient caching: repo_dirs is retrieved once and reused to avoid redundant filesystem operations (lib/demo_scripts/gem_swapper.rb:145, 296)
• Single traversal: Directory sizes are calculated once per operation and stored in repo_info arrays
• Race condition prevention: Caches directories at operation start to avoid TOCTOU issues

User Experience

• Informative output: Clear emoji indicators and human-readable sizes improve CLI usability
• Dry-run support: Respects existing --dry-run flag for safe preview (lib/demo_scripts/gem_swapper.rb:275-276, 303-307)
• Helpful messages: Provides context when cache is empty or directories don't exist

🔍 Issues & Recommendations

1. Missing Test Coverage ⚠️ HIGH PRIORITY

The new cache management methods are not covered by tests. The existing test file (spec/demo_scripts/gem_swapper_spec.rb) has no tests for:

• show_cache_info
• clean_cache
• clean_gem_cache
• clean_all_cache
• cache_repo_dirs
• directory_size
• human_readable_size

Recommendation: Add comprehensive tests covering:

describe '#show_cache_info' do
  context 'when cache directory does not exist'
  context 'when cache is empty'
  context 'with cached repositories'
end

describe '#clean_cache' do
  context 'with gem_name parameter'
  context 'without gem_name parameter (clean all)'
  context 'with dry_run enabled'
end

describe '#directory_size' do
  context 'with permission errors'
  context 'with symlinks'
  context 'with missing paths'
end

describe '#human_readable_size' do
  it 'formats bytes correctly'
  it 'formats KB/MB/GB/TB correctly'
  it 'handles zero bytes'
end

2. Potential Security Issue: Gem Name Matching ⚠️ MEDIUM PRIORITY

In clean_gem_cache (lib/demo_scripts/gem_swapper.rb:258-263), the matching logic could have false positives:

normalized_gem = gem_name.tr('_', '-')
matching_dirs = cache_repo_dirs.select do |path|
  basename = File.basename(path)
  basename.match?(/[-_]#{Regexp.escape(normalized_gem)}[-_]/) ||
    basename.match?(/[-_]#{Regexp.escape(gem_name)}[-_]/)
end

Issue: This pattern matches gem names appearing anywhere in the directory name, not just as the repository component. For example:

• --clean-cache test might match username-test-branch OR test-user-repo-branch
• --clean-cache react might match shakacode-react_on_rails-main

Recommendation: Use more specific pattern matching that validates the full directory structure:

# Expected format: {org}-{repo}-{branch}
# Match the middle component only
basename.match?(/\A[^-]+-#{Regexp.escape(normalized_gem)}-/)

3. Minor: Incomplete Context Lines in CLI Options 📝 LOW PRIORITY

In swap_deps_cli.rb:189-199, the help option doesn't explicitly state that cleaning preserves watch_logs:

opts.on('--clean-cache [GEM]', 'Remove cached repositories (all or specific gem)') do |gem|

Recommendation: Update help text to be more explicit:

opts.on('--clean-cache [GEM]', 'Remove cached repositories (all or specific gem, excludes watch_logs)') do |gem|

4. Edge Case: Concurrent Access 📝 LOW PRIORITY

While show_cache_info caches directories at the start (line 145), there's still a potential race condition between calculating sizes and displaying them if external processes modify the cache.

Recommendation: This is likely acceptable for a developer tool, but consider wrapping critical sections with file locking if this becomes an issue. Document this behavior limitation in comments.

5. Code Duplication in Size Formatting 📝 LOW PRIORITY

The human_readable_size method (lib/demo_scripts/gem_swapper.rb:237-245) is well-implemented, but consider if Ruby's built-in ActiveSupport::NumberHelper.number_to_human_size could be used since this appears to be a Rails-adjacent project.

Current approach is fine, but worth considering for consistency with Rails conventions if ActiveSupport is already available.

6. Metrics Complexity Disabled 📝 INFORMATIONAL

The PR adds several rubocop:disable Metrics/* comments (lines 35, 247, 285). While the current complexity is reasonable for the functionality:

Observation: Consider extracting the pattern matching logic in clean_gem_cache to a separate method to reduce cognitive complexity:

def matches_gem_cache_pattern?(basename, gem_name, normalized_gem)
  basename.match?(/[-_]#{Regexp.escape(normalized_gem)}[-_]/) ||
    basename.match?(/[-_]#{Regexp.escape(gem_name)}[-_]/)
end

🔒 Security Assessment

✅ No security vulnerabilities identified, with excellent security practices:

1. ✅ Input validation prevents path traversal (line 250)
2. ✅ Uses FileUtils.rm_rf appropriately on validated paths only
3. ✅ Proper handling of symlinks to prevent security issues
4. ✅ Safe YAML loading already implemented in load_config
5. ⚠️ Minor: Gem name matching could be more restrictive (see issue Add Claude Code GitHub Workflow #2 above)

📊 Test Coverage

Current Status: ❌ No tests for new functionality

The PR description states tests were manually run, but automated tests are missing. This is the most critical gap that should be addressed before merging.

🎯 Overall Assessment

Recommendation: ✅ Approve with changes

This is well-crafted code that adds valuable functionality with good attention to security and UX. The main blocker is the lack of automated tests.

Required Before Merge:

1. ✅ Add comprehensive test coverage for all new methods
2. ⚠️ Consider improving gem name matching pattern (see issue Add Claude Code GitHub Workflow #2)

Nice to Have:

3. Update help text to clarify watch_logs exclusion
4. Consider extracting complex pattern matching to reduce method complexity

Great work on the implementation! The code quality is high, and the feature will be very useful for managing disk space when working with GitHub repositories.

---

🤖 Review generated by Claude Code

[claude]

Pull Request Review: Add Cache Management Commands to swap-deps (#39)

Summary

This PR adds cache management functionality to the swap-deps utility, allowing users to view and clean cached GitHub repositories. The implementation is well-structured, thoroughly tested, and production-ready.

✅ Strengths

Code Quality

• Excellent test coverage: 329 lines of comprehensive tests covering all edge cases (62 test cases, 100% passing)
• Progressive improvement: 4 commits show iterative refinement addressing security, performance, and robustness
• Clean separation of concerns: CLI layer (swap_deps_cli.rb) properly delegates to business logic (gem_swapper.rb)
• Consistent error handling: Proper exception handling for filesystem errors (EACCES, ENOENT)

Security

• Input validation: Gem names validated with regex /\A[\w.-]+\z/ to prevent path traversal attacks
• Safe pattern matching: matches_gem_cache_pattern? prevents false positives (e.g., matching gem name in org/branch positions)
• Symlink protection: Skips symlinks in directory_size to prevent circular references and incorrect calculations

Performance

• Optimized directory traversal: Cache sizes calculated once and reused (reduces operations from 2N+1 to N)
• Race condition prevention: Dir.glob results cached in show_cache_info to avoid TOCTOU issues
• Efficient helper methods: cache_repo_dirs centralizes directory filtering logic

User Experience

• Dry-run support: All destructive operations respect --dry-run flag
• Clear output: Human-readable sizes (B, KB, MB, GB, TB) with consistent formatting
• Helpful messages: Informative output for empty caches, missing directories, and no matches
• Good documentation: Comprehensive help text and usage examples in PR description

🔍 Minor Observations

Code Style

1. Long method in CLI (lib/demo_scripts/swap_deps_cli.rb:35):

   • The run! method now has complexity warnings disabled with Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
   • Consider extracting cache operation dispatch to a separate method:

   def run!
     # ...
     handle_cache_operations || handle_watch_operations || handle_swap_operations
   end
   
   def handle_cache_operations
     return false unless @show_cache || @clean_cache || @clean_cache_gem
     @show_cache ? show_cache_info : clean_cache_handler
     true
   end

2. watch_logs exclusion (lib/demo_scripts/gem_swapper.rb:208):

   • Hardcoded string 'watch_logs' appears in multiple places
   • Consider adding constant: WATCH_LOGS_DIR_NAME = 'watch_logs'

Documentation

3. Missing YARD param type:
   • The @param comment for clean_cache could specify type more explicitly:

   # @param gem_name [String, nil] Optional gem name to clean...

   Current implementation is clear, but formal YARD syntax would be: @param [String, nil] gem_name

Test Coverage

4. Edge case testing (all covered, but worth noting):
   • ✅ Zero bytes formatting
   • ✅ Symlink handling
   • ✅ Permission errors
   • ✅ Missing paths
   • ✅ Empty cache scenarios
   • ✅ Pattern matching false positives
   • ✅ Performance optimization verification

🎯 Recommendations (Optional Enhancements)

Future Improvements (Not Blockers)

1. Cache size limits: Consider adding a warning when cache exceeds a threshold (e.g., 1GB)
2. Last access time: Show when cached repos were last used to help identify stale entries
3. Interactive mode: For --clean-cache, prompt for confirmation unless --yes flag provided
4. Cache statistics: Track and display total number of cleanups performed

Testing

5. Integration test: While unit tests are comprehensive, consider adding an integration test that:
   • Clones a real repo to cache
   • Verifies --show-cache displays it
   • Cleans it with --clean-cache
   • Confirms removal

🔒 Security Assessment

✅ No security concerns identified

• Proper input validation prevents path traversal
• No unsafe file operations
• No command injection vectors
• Respects filesystem permissions

🚀 Performance Assessment

✅ Well optimized

• Minimized redundant directory traversals
• Cached results where appropriate
• No blocking operations without user control

🐛 Bug Assessment

✅ No bugs identified

• All edge cases handled
• Error conditions properly managed
• Race conditions addressed
• Test coverage validates correctness

📝 Documentation Assessment

✅ Good documentation

• Clear help text
• Usage examples provided
• Comments explain security measures
• Commit messages follow conventional format

Final Verdict

APPROVED ✅

This is high-quality code that demonstrates:

• Defensive programming with comprehensive error handling
• Security awareness with input validation
• Performance optimization with smart caching
• Excellent test coverage with 62 passing tests
• Clear user experience with helpful output

The iterative improvement across 4 commits shows thoughtful development addressing security, performance, and robustness concerns. The code is ready to merge.

Pre-merge Checklist

• Tests pass (62 tests, 100% passing)
• RuboCop passes
• Security review complete
• Performance optimized
• Documentation updated
• Pre-commit hooks pass

Great work on this feature! 🎉

---

Review performed by Claude Code