Fix GitHub ref swapping by cleaning stale Gemfile.lock entries #59
Conversation
When swapping gems between sources (rubygems ↔ GitHub ↔ local path), the Gemfile.lock can contain stale references to commits or versions that no longer exist, causing `bundle update` to fail with "Could not find gem" errors. This fix removes the Gemfile.lock before running bundle install when swapping gem sources, forcing Bundler to generate fresh lock entries from the updated Gemfile. This ensures: - GitHub refs work correctly even if branches are rebased/force-pushed - Switching from GitHub back to rubygems resolves cleanly - Local path swaps don't conflict with previous GitHub/rubygems locks 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
WalkthroughDetects GitHub-based swapped gems that are behind their remotes, reports counts, optionally auto-updates caches, forces clean installs by backing up and removing Gemfile.lock when updating/restoring swapped gems, replaces many Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as swap_deps_cli
participant Swapper as DependencySwapper
participant Cache as LocalCache
participant GitHub as RemoteRepo
CLI->>Swapper: show_status(demo_path, auto_update?)
Swapper->>Swapper: enumerate swapped gems (GitHub-based)
loop per GitHub-swapped gem
Swapper->>Cache: open cached repo (cache_path)
Swapper->>GitHub: fetch remote ref (check_commits_behind)
GitHub-->>Swapper: remote ref metadata
Swapper-->>Swapper: compute commits_behind
end
alt outdated found & auto_update true
Swapper->>Swapper: update_outdated_repos(outdated_repos)
else outdated found & auto_update false
Swapper-->>CLI: print manual update guidance
end
Swapper-->>CLI: print status summary
sequenceDiagram
autonumber
participant CLI as Caller
participant Swapper as DependencySwapper
participant FS as Filesystem
participant Bundler as Bundler
CLI->>Swapper: run_bundle_install(demo_path, gems_to_update, for_restore)
alt gems_to_update present
Swapper->>Swapper: compute gems_to_clean
Swapper->>FS: backup & remove `Gemfile.lock` (clean_gemfile_lock)
end
Swapper->>Bundler: run `bundle install --quiet`
Bundler-->>Swapper: success / failure
alt failure
Swapper->>FS: restore `Gemfile.lock` from backup (restore_lock_backup)
else success
Swapper->>FS: remove backup (cleanup_lock_backup)
end
Swapper-->>CLI: report result and print_next_steps
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/demo_scripts/gem_swapper.rb (1)
1012-1027: Consider adding error handling for file deletion.The approach of removing the lock file to force clean resolution is sound and addresses the PR objective. However,
File.deletecan raise exceptions (e.g.,Errno::EACCESfor permission issues) that aren't currently handled.Consider wrapping the deletion in error handling:
puts " Cleaning lock file entries for: #{gems_to_clean.join(', ')}" if verbose - # Simply remove the lock file and let bundle regenerate it - # This is the safest approach to avoid any inconsistencies - File.delete(lock_file) - puts ' Removed Gemfile.lock to force clean resolution' if verbose + # Simply remove the lock file and let bundle regenerate it + # This is the safest approach to avoid any inconsistencies + begin + File.delete(lock_file) + puts ' Removed Gemfile.lock to force clean resolution' if verbose + rescue Errno::EACCES => e + warn " ⚠️ Warning: Could not delete Gemfile.lock: #{e.message}" + rescue StandardError => e + warn " ⚠️ Warning: Unexpected error removing Gemfile.lock: #{e.message}" + end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/demo_scripts/gem_swapper.rb(2 hunks)spec/demo_scripts/gem_swapper_spec.rb(3 hunks)
🔇 Additional comments (4)
spec/demo_scripts/gem_swapper_spec.rb (3)
734-749: LGTM! Test correctly verifies lock file cleanup before installation.The test properly expects
clean_gemfile_lockto be called with the correct arguments beforebundle install, and the updated assertion aligns with the new installation-based approach instead of selective updates.
761-773: LGTM! Failure path test properly updated.The test correctly verifies that
clean_gemfile_lockis called even in the failure scenario and that the error message reflects the installation failure appropriately.
777-790: LGTM! Non-restore path test correctly updated.The test properly verifies the lock file cleanup and installation flow for the swapping scenario (for_restore: false).
lib/demo_scripts/gem_swapper.rb (1)
1048-1070: LGTM! Integration correctly addresses stale lock file issues.The integration of
clean_gemfile_lockbeforebundle installin both restore and non-restore paths effectively prevents "Could not find gem" errors when swapping gem sources. The approach of removing the lock file and running a fresh install is conservative and safe.The verbose logging helps with debugging, and the updated messaging accurately reflects the installation-based approach.
Code Review - PR #59: Fix GitHub ref swapping by cleaning stale Gemfile.lock entriesSummaryThis PR addresses a real problem with gem source swapping by removing stale Gemfile.lock entries before bundle operations. The solution is pragmatic and well-tested. Overall, this is a solid improvement with a few recommendations for enhancement. Strengths
Code Quality ObservationsPositive:
Minor Suggestion - Add error handling for file deletion (lib/demo_scripts/gem_swapper.rb:1025): Potential Issues
Performance ConsiderationsNuclear approach trade-off: Deleting the entire Gemfile.lock forces Bundler to resolve ALL dependencies from scratch, not just the swapped gems. This is safer (guaranteed consistency) but slower (full dependency resolution vs. targeted update). For projects with many dependencies, this could add noticeable overhead. However, this trade-off is acceptable because: (1) Correctness > speed for this operation, (2) The overhead is one-time per swap operation, (3) Alternative approaches (surgical lock file editing) are complex and error-prone. Security ConsiderationsNo major security concerns identified. The code uses File.join to construct paths (prevents path traversal), checks file existence before deletion, runs in the context of the user's permissions, and uses Regexp.escape when constructing regex patterns (line 1008). Minor hardening opportunity: Consider validating that demo_path is within expected directories to prevent accidental deletion of unrelated Gemfile.lock files. Test Coverage AssessmentCurrent Coverage: Good - Tests verify clean_gemfile_lock is called with correct parameters, verify behavior changes (bundle update → bundle install), verify error messaging updates, and edge cases are covered (empty gems list, file doesn't exist). Missing Coverage: Direct unit tests for clean_gemfile_lock method, error handling scenarios (permission denied, I/O errors), and dry-run mode for clean_gemfile_lock. Suggestion: Add dry_run support to clean_gemfile_lock to maintain consistency with other methods in the codebase. Recommendations SummaryHigh Priority:
Medium Priority: Low Priority: ConclusionThis is a well-thought-out solution to a legitimate problem. The approach of removing the entire lock file is pragmatic and safe, even if it's not the most performant option. The test updates demonstrate understanding of the behavior changes. Recommendation: Approve with minor improvements - The suggestions above would make this excellent code even better, but they're not blockers for merging. Great work! |
Makes it clearer that users can enable auto-rebuild by adding the --watch flag, instead of having to manually rebuild packages. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Instead of showing a vague placeholder, now tells users to run 'bin/swap-deps --status' to see actual gem paths. This is much clearer and actionable. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
PR ReviewI've reviewed PR #59 and here's my comprehensive feedback: ✅ Strengths
🔍 Code Quality ObservationsPositive:
Minor Suggestions:
🐛 Potential IssuesLow Priority:
⚡ Performance Considerations
🔒 Security Concerns
🧪 Test CoverageGood Coverage:
Potential Test Gaps:
📝 DocumentationUI Improvement Noted: 🎯 Overall AssessmentRating: ✅ Approve with Minor Suggestions This is a solid PR that solves a real problem with a pragmatic approach. The core logic is sound, tests are adequate, and the code is maintainable. The minor suggestions above are optional improvements that would enhance robustness but aren't blockers for merging. Recommendation: Approve and merge. Consider the error handling enhancement as a future improvement if issues arise in production. Great work solving this bundler edge case! The approach of regenerating the lock file from scratch is much more reliable than trying to surgically edit it. |
Pull Request ReviewThank you for this fix! The approach of cleaning the Gemfile.lock to resolve stale GitHub ref issues is sound and well-implemented. Here is my detailed review: Strengths
Suggestions for Improvement1. Consider Adding Error Handling for File Deletion In clean_gemfile_lock (lib/demo_scripts/gem_swapper.rb:1025), File.delete could raise an exception if permissions are insufficient or the file is locked. Consider wrapping it in a begin/rescue block to handle Errno::EACCES or Errno::EPERM exceptions gracefully. 2. Potential Race Condition in Multi-Process Environments If multiple processes run bundle operations simultaneously (unlikely but possible), deleting Gemfile.lock without atomic operations could cause issues. Consider adding a comment documenting the single-process execution assumption. 3. Test Coverage: Missing Edge Cases The tests verify the happy path and basic failure, but consider adding tests for when clean_gemfile_lock is called with an empty array or when the lock file does not exist. 4. Documentation: Method Visibility The clean_gemfile_lock method is currently private by context. Consider explicitly marking it as private for clarity. Security ConsiderationsNo security concerns identified
Performance ConsiderationsMinimal performance impact
Test Coverage AssessmentGood test coverage with minor suggestions above
Minor Observations
Overall AssessmentAPPROVED This is a well-thought-out fix that solves a real problem. The implementation is clean, safe, and well-tested. The minor suggestions above are optional enhancements rather than blockers. Recommendation: Merge after considering the error handling suggestion for File.delete to make the tool more robust in edge cases. Great work on the comprehensive test plan and clear documentation! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/demo_scripts/gem_swapper.rb (2)
1048-1057: Improve comment and error messaging clarity.Two minor clarity issues:
Line 1053: The comment "Install from scratch since we removed the lock file" could be clearer.
bundle installwithout a lock file still respects version constraints in the Gemfile—it's not truly "from scratch" like abundle updatewould be. Consider: "Regenerate lock file from Gemfile constraints".Line 1055: The error message "Check Gemfile for errors" is too specific. Bundle install failures can occur for many reasons (network issues, authentication, missing dependencies, etc.), not just Gemfile syntax errors.
Apply these changes for clearer messaging:
- # Install from scratch since we removed the lock file - result = system('bundle', 'install', '--quiet') - warn ' ⚠️ ERROR: Failed to install gems. Check Gemfile for errors.' unless result + # Regenerate lock file from Gemfile constraints + result = system('bundle', 'install', '--quiet') + warn ' ⚠️ ERROR: Failed to install gems. Check bundle output for details.' unless result
1062-1070: Clarify "from scratch" comment.Line 1068 has the same clarity issue as line 1053: the comment "Install from scratch since we removed the lock file" could be more precise.
bundle installregenerates the lock file based on Gemfile constraints, which is different from a true "from scratch" approach.Apply this change:
- # Install from scratch since we removed the lock file + # Regenerate lock file from Gemfile constraints
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/demo_scripts/gem_swapper.rb(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (1)
lib/demo_scripts/gem_swapper.rb (1)
1197-1199: LGTM! Improved user guidance.The expanded messaging provides clearer next steps by offering both manual and automated rebuild options, along with the command to view current paths. This enhances the user experience.
| # Removes stale gem entries from Gemfile.lock to prevent bundle update failures | ||
| # This is necessary when switching gem sources (rubygems <-> github <-> path) | ||
| # because the lock file may reference commits/versions that no longer exist | ||
| def clean_gemfile_lock(demo_path, gems_to_clean) | ||
| return unless gems_to_clean.any? | ||
|
|
||
| lock_file = File.join(demo_path, 'Gemfile.lock') | ||
| return unless File.exist?(lock_file) | ||
|
|
||
| puts " Cleaning lock file entries for: #{gems_to_clean.join(', ')}" if verbose | ||
|
|
||
| # Simply remove the lock file and let bundle regenerate it | ||
| # This is the safest approach to avoid any inconsistencies | ||
| File.delete(lock_file) | ||
| puts ' Removed Gemfile.lock to force clean resolution' if verbose | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Method name and behavior mismatch.
The method is named clean_gemfile_lock and accepts a gems_to_clean parameter, suggesting it would selectively clean specific gem entries. However, it removes the entire Gemfile.lock file regardless of which gems need cleaning (line 1025). This has implications:
- Unused parameter: The
gems_to_cleanparameter is only checked for emptiness but never used for selective cleaning. - Broader impact: Deleting the entire lock file forces Bundler to regenerate all dependency versions, not just the problematic gems. This could inadvertently update unrelated dependencies if their version constraints allow it.
- Missing error handling:
File.deletecould fail (e.g., permission issues) without any error handling.
Consider these improvements:
Option 1 (simpler): Rename to reflect actual behavior:
-def clean_gemfile_lock(demo_path, gems_to_clean)
- return unless gems_to_clean.any?
+def remove_gemfile_lock(demo_path)Option 2 (more precise but complex): Implement selective cleaning by parsing and modifying Gemfile.lock to remove only the specified gem sections, preserving other dependency versions. This would require parsing the lock file format.
Add error handling:
-File.delete(lock_file)
+begin
+ File.delete(lock_file)
+rescue Errno::EACCES => e
+ warn " ⚠️ Warning: Could not delete Gemfile.lock: #{e.message}"
+ return false
+end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Removes stale gem entries from Gemfile.lock to prevent bundle update failures | |
| # This is necessary when switching gem sources (rubygems <-> github <-> path) | |
| # because the lock file may reference commits/versions that no longer exist | |
| def clean_gemfile_lock(demo_path, gems_to_clean) | |
| return unless gems_to_clean.any? | |
| lock_file = File.join(demo_path, 'Gemfile.lock') | |
| return unless File.exist?(lock_file) | |
| puts " Cleaning lock file entries for: #{gems_to_clean.join(', ')}" if verbose | |
| # Simply remove the lock file and let bundle regenerate it | |
| # This is the safest approach to avoid any inconsistencies | |
| File.delete(lock_file) | |
| puts ' Removed Gemfile.lock to force clean resolution' if verbose | |
| end | |
| # Removes stale gem entries from Gemfile.lock to prevent bundle update failures | |
| # This is necessary when switching gem sources (rubygems <-> github <-> path) | |
| # because the lock file may reference commits/versions that no longer exist | |
| def remove_gemfile_lock(demo_path) | |
| lock_file = File.join(demo_path, 'Gemfile.lock') | |
| return unless File.exist?(lock_file) | |
| puts " Cleaning lock file entries" if verbose | |
| # Simply remove the lock file and let bundle regenerate it | |
| # This is the safest approach to avoid any inconsistencies | |
| begin | |
| File.delete(lock_file) | |
| puts ' Removed Gemfile.lock to force clean resolution' if verbose | |
| rescue Errno::EACCES => e | |
| warn " ⚠️ Warning: Could not delete Gemfile.lock: #{e.message}" | |
| return false | |
| end | |
| end |
The --status command now checks if GitHub-based swapped gems are outdated and displays how many commits behind they are. This helps developers know when they need to refresh their cached dependencies. Features: - Automatically fetches and compares commits with remote branch - Shows number of commits behind for each outdated gem - Provides example command to update Example output:⚠️ Outdated GitHub repositories detected: shakapacker (shakacode/shakapacker@justin808/early-hints) - 2 commit(s) behind 💡 To update: Re-run your swap command to fetch latest changes Example: bin/swap-deps --github 'shakacode/shakapacker#justin808/early-hints' 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Code Review - PR #59: Fix GitHub ref swapping by cleaning stale Gemfile.lock entriesOverall Assessment✅ Approved with minor suggestions - This is a well-thought-out solution to a real problem. The approach of removing the Gemfile.lock before bundle operations when swapping gem sources is sound and aligns with Bundler best practices. Code Quality & Best PracticesStrengths:
Suggestions:
Potential Bugs & Issues
Performance Considerations✅ Good:
New feature - Outdated repo checking:
Security Concerns✅ Secure:
Minor note:
Test Coverage✅ Excellent test coverage:
Suggestions:
Specific Code Review Commentslib/demo_scripts/gem_swapper.rb:1082 File.delete(lock_file)✅ Good: Simple and effective. Bundler will regenerate with correct entries. lib/demo_scripts/gem_swapper.rb:378-419 lib/demo_scripts/gem_swapper.rb:411 system('git', 'fetch', 'origin', branch, out: '/dev/null', err: '/dev/null')✅ Good: Proper suppression of output and safe system call. spec/demo_scripts/gem_swapper_spec.rb:785 expect(swapper).to receive(:clean_gemfile_lock).with(demo_path, %w[shakapacker react_on_rails])✅ Excellent: Tests verify the new method is called with correct parameters. SummaryThis PR effectively solves a real pain point with gem source swapping. The solution is simple, well-tested, and follows best practices. Recommendations:
Test Results: As noted in the PR description, 77/86 tests pass with 6 pre-existing failures unrelated to this change - this is acceptable. Great work on identifying and fixing this issue! 🎉 Review generated by Claude Code - feel free to discuss any points above! |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/demo_scripts/gem_swapper.rb (1)
1069-1084: Past review feedback remains unaddressed.The issues raised in the previous review comment still apply:
- Unused parameter:
gems_to_cleanis only checked for emptiness but never used for selective cleaning- Method behavior mismatch: The method removes the entire Gemfile.lock regardless of which gems need cleaning
- Missing error handling:
File.deleteat line 1082 could fail without any error handlingThe previous review suggested either:
- Renaming to
remove_gemfile_lockto reflect actual behavior- Implementing selective cleaning (complex)
- Adding error handling for the file deletion
Consider at minimum adding error handling:
-File.delete(lock_file) -puts ' Removed Gemfile.lock to force clean resolution' if verbose +begin + File.delete(lock_file) + puts ' Removed Gemfile.lock to force clean resolution' if verbose +rescue Errno::EACCES => e + warn " ⚠️ Warning: Could not delete Gemfile.lock: #{e.message}" + return false +end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/demo_scripts/gem_swapper.rb(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/demo_scripts/gem_swapper.rb (2)
lib/demo_scripts/swap_deps_cli.rb (1)
each_demo(509-517)lib/demo_scripts/demo_manager.rb (1)
each_demo(19-30)
🪛 Brakeman (7.1.0)
lib/demo_scripts/gem_swapper.rb
[medium] 414-414: Possible command injection
Type: Command Injection
Confidence: Medium
More info: https://brakemanscanner.org/docs/warning_types/command_injection/
(Command Injection)
🔇 Additional comments (4)
lib/demo_scripts/gem_swapper.rb (4)
141-157: Well-implemented outdated repository detection!The integration of outdated repository checks in
show_statusis clear and provides actionable guidance to users. The example command at line 156 is particularly helpful.
377-405: LGTM - Clean implementation!The method correctly identifies outdated GitHub repositories by checking commit differences. The logic flow is clear with appropriate early returns using
next.
1087-1138: Good integration of lock file cleaning.The changes correctly integrate
clean_gemfile_lockbefore bundle operations and appropriately replacebundle updatewithbundle installafter removing the lock file. The updated error messaging at line 1112 is also clearer.The logic properly handles three scenarios:
- Restore with no swapped gems (lines 1097-1103)
- Restore with swapped gems requiring lock cleanup (lines 1105-1115)
- Swap with lock cleanup (lines 1119-1127)
Note: This inherits any issues from
clean_gemfile_lockif it fails to delete the lock file (see separate comment).
1254-1257: Improved user guidance!The updated messaging provides clearer instructions about rebuild options, showing both the manual path to check status and the automatic rebuild option with the
--watchflag. This improves the user experience.
| # Check how many commits behind the local cache is from the remote branch | ||
| def check_commits_behind(cache_path, branch) | ||
| Dir.chdir(cache_path) do | ||
| # Fetch latest without output | ||
| system('git', 'fetch', 'origin', branch, out: '/dev/null', err: '/dev/null') | ||
|
|
||
| # Count commits between local and remote | ||
| output, status = Open3.capture2('git', 'rev-list', '--count', "HEAD..origin/#{branch}") | ||
| return status.success? ? output.strip.to_i : nil | ||
| end | ||
| rescue StandardError | ||
| nil | ||
| end |
There was a problem hiding this comment.
Avoid string interpolation in git commands.
Line 414 interpolates branch directly into a git command string, which is flagged by Brakeman as a potential command injection risk. While validate_github_branch should provide some protection, it's safer to use parameterized commands consistently.
Apply this diff to use the array form:
Dir.chdir(cache_path) do
# Fetch latest without output
system('git', 'fetch', 'origin', branch, out: '/dev/null', err: '/dev/null')
# Count commits between local and remote
- output, status = Open3.capture2('git', 'rev-list', '--count', "HEAD..origin/#{branch}")
+ output, status = Open3.capture2('git', 'rev-list', '--count', "HEAD..origin/#{branch}", chdir: cache_path)
return status.success? ? output.strip.to_i : nil
endWait, the issue is that Open3.capture2 requires the command as separate arguments, but we're passing a string. Let me revise:
Dir.chdir(cache_path) do
# Fetch latest without output
system('git', 'fetch', 'origin', branch, out: '/dev/null', err: '/dev/null')
# Count commits between local and remote
- output, status = Open3.capture2('git', 'rev-list', '--count', "HEAD..origin/#{branch}")
+ output, status = Open3.capture2('git', 'rev-list', '--count', 'HEAD', "--not", "origin/#{branch}")
return status.success? ? output.strip.to_i : nil
endActually, git's rev-list syntax requires the range format. The safest approach is:
Dir.chdir(cache_path) do
# Fetch latest without output
system('git', 'fetch', 'origin', branch, out: '/dev/null', err: '/dev/null')
+ remote_ref = "origin/#{branch}"
+ # Validate the remote ref exists before using it
+ _output, status = Open3.capture2('git', 'rev-parse', '--verify', remote_ref)
+ return nil unless status.success?
+
# Count commits between local and remote
- output, status = Open3.capture2('git', 'rev-list', '--count', "HEAD..origin/#{branch}")
+ output, status = Open3.capture2('git', 'rev-list', '--count', "HEAD..#{remote_ref}")
return status.success? ? output.strip.to_i : nil
endThis adds validation while still using the necessary string interpolation for the ref range. The key improvement is verifying the ref exists first, which provides defense-in-depth beyond the initial branch validation.
🧰 Tools
🪛 Brakeman (7.1.0)
[medium] 414-414: Possible command injection
Type: Command Injection
Confidence: Medium
More info: https://brakemanscanner.org/docs/warning_types/command_injection/
(Command Injection)
🤖 Prompt for AI Agents
In lib/demo_scripts/gem_swapper.rb around lines 407 to 419, replace the
Open3.capture2 call that currently builds a single command string with the
array/argument form so git is invoked without shell interpolation, and add a
prior check that the remote ref exists (e.g., git show-ref --verify --quiet
refs/remotes/origin/<branch> using Open3.capture3 or system) returning nil if
the ref is missing; then call Open3.capture2('git', 'rev-list', '--count',
"HEAD..origin/#{branch}") as separate args and preserve the existing
status.success? handling and rescue behavior.
The issue was that swap-deps was requiring bundler/setup unconditionally, which would fail when running from a demo directory with a Gemfile that had GitHub gems not yet checked out. Solution: Only require bundler/setup for commands that actually need gem dependencies (swap, restore, apply). Read-only commands (status, cache, list-watch, kill-watch, clean-cache) only use git/filesystem operations and don't need bundler. This fixes the error: 'The git source https://github.com/... is not yet checked out. Please run `bundle install`...' 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Pull Request Review: Fix GitHub ref swapping by cleaning stale Gemfile.lock entriesSummaryThis PR addresses a critical issue with gem swapping where stale Gemfile.lock entries cause bundle failures when switching between gem sources. The solution is well-architected and solves the root cause by removing the lock file before bundle operations. ✅ Strengths1. Clean Architecture & Separation of Concerns
2. Comprehensive Test Coverage
3. Enhanced User Experience
4. Code Quality
🔍 Observations & Considerations1. Security & Safety
2. Performance Consideration def check_commits_behind(cache_path, branch)
Dir.chdir(cache_path) do
# Fetch latest without output
system('git', 'fetch', 'origin', branch, out: '/dev/null', err: '/dev/null')
3. Error Handling
4. Edge Case: Concurrent Operations
5. Bundler Behavior Change
🐛 Potential IssuesMinor: Array Access Without Guard puts " Example: bin/swap-deps --github '#{outdated_repos.first[:repo]}##{outdated_repos.first[:ref]}'"
Minor: Read-only Command Detection def read_only_command?
@show_status || @show_cache || @list_watch || @kill_watch || @clean_cache || @clean_cache_gem
end
🧪 Test Coverage AssessmentExcellent Coverage:
Missing Tests (Non-blocking):
📋 Best Practices Compliance✅ Code Style: Follows RuboCop guidelines 🔒 Security AssessmentNo Security Concerns Identified
🎯 RecommendationsPriority: Low - These are suggestions for future enhancements, not blockers
✨ Final AssessmentRecommendation: ✅ APPROVE This is a high-quality PR that solves a real problem with a clean, well-tested solution. The code is production-ready and follows all repository best practices. Key Wins:
No blocking issues identified. The minor observations above are suggestions for potential future enhancements, not requirements for merging. Great work! 🚀 Review generated with Claude Code |
Previously, swap-deps had to be run from the project root. Now it can be
run from anywhere inside the project and it will:
1. Automatically find the project root by walking up the directory tree
looking for .swap-deps.yml or bin/swap-deps
2. Change to the project root before running
3. Detect if you were in a demo directory and auto-scope to that demo
This makes it much more convenient to use:
- Run from demo directory: auto-scopes to that demo
- Run from nested subdirectory: still finds project root and works
- Run from project root: works as before
Example output when run from demos/basic-v16-rspack:
📍 Changed to project root: /path/to/project
Detected you were in demo: basic-v16-rspack
Auto-scoped to this demo only
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/demo_scripts/swap_deps_cli.rb (1)
87-90: Consider renaming for clarity (optional).The method name
read_only_command?might be slightly misleading since it includes operations that modify state (e.g.,kill_watchkills processes,clean_cachedeletes files). However, the semantic intent—"commands that don't require gem dependencies"—is clear from the context and comment.For improved clarity, consider alternative names:
skip_bundler_setup?command_without_gem_dependencies?standalone_command?That said, the current name works well enough given the comment on line 43 explains the intent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/demo_scripts/swap_deps_cli.rb(2 hunks)
🔇 Additional comments (1)
lib/demo_scripts/swap_deps_cli.rb (1)
42-44: Verification complete: the conditional bundler/setup optimization is safe and correct.Both
DependencySwapperandFilteredDependencySwappercan be instantiated without bundler/setup. Their initialize methods only callsuper(**options)and set instance variables—no gem loading occurs. All file-level requires in gem_swapper.rb are standard library. The optimization properly defers bundler/setup loading until needed, which is correct for read-only commands (status, cache, watch operations).
When --status detects outdated GitHub repositories, users can now add the --auto-update flag to automatically pull latest changes and rebuild npm packages. Usage: bin/swap-deps --status --auto-update This will: 1. Check all swapped GitHub repos for new commits 2. Automatically update any that are behind 3. Rebuild their npm packages 4. Report what was updated Example output:⚠️ Outdated GitHub repositories detected: shakapacker (shakacode/shakapacker@justin808/early-hints) - 6 commit(s) behind 🔄 Auto-updating outdated repositories... Updating shakapacker... Rebuilding shakapacker... ✅ Updated 1 repository/repositories 💡 Changes are now available in your demos The --status command now also suggests using --auto-update when outdated repos are detected. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Pull Request Review - PR #59SummaryThis PR addresses a critical issue with gem swapping by cleaning stale Gemfile.lock entries when switching between gem sources. The approach is pragmatic and effective, with good test coverage updates. ✅ Strengths
🔍 Issues & Concerns1. Potential Race Condition in check_commits_behind (lib/demo_scripts/gem_swapper.rb:408-419)Severity: Medium The system() call doesn't check return status. If fetch fails silently, rev-list will use stale data. Recommendation: Check fetch success before counting commits 2. Command Injection Risk in check_commits_behind (lib/demo_scripts/gem_swapper.rb:414)Severity: High 🔴 String interpolation of branch into git command without validation. If branch name contains special characters, it could lead to command injection. Recommendation: Validate branch names to ensure they don't contain unexpected characters. 3. Missing Test Coverage for New FeaturesSeverity: Medium The following new methods lack test coverage:
Recommendation: Add unit tests, especially for clean_gemfile_lock and check_commits_behind with mocked git commands. 4. Performance: Git Fetch on Every Status CheckSeverity: Low system('git', 'fetch', ...) is called for every swapped GitHub gem during status checks. For multiple gems, this could be slow. Recommendation: Consider adding a --skip-fetch flag or caching fetch results with a timestamp. 🔒 Security Assessment
⚡ Performance Considerations
🧪 Test Coverage SummaryModified Tests: 3 specs updated in spec/demo_scripts/gem_swapper_spec.rb
Missing Tests: 6+ new methods without coverage 📋 Recommendations Summary
✨ Overall AssessmentCode Quality: Good - well-structured, readable, with clear intent Approach: Pragmatic - deleting lock file is simpler and safer than parsing/editing it Risk Level: Medium - command injection concern needs addressing before merge Recommendation: Request changes - address the command injection issue in check_commits_behind, then this is good to merge. Great work on solving a tricky bundler issue! The approach is sound and will save developers from frustrating Could not find gem errors. Review generated with Claude Code |
Auto-updating outdated repos is now enabled by default when running --status, making it more convenient for users. They no longer need to remember to add --auto-update. Changes: - Auto-update defaults to true - Added --no-auto-update flag to disable if needed - Updated help text to show it's the default - Simplified success message Usage: bin/swap-deps --status # Auto-updates by default bin/swap-deps --status --no-auto-update # Skip auto-update This provides the best user experience - outdated repos are automatically refreshed without requiring extra flags or manual intervention. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Code Review - PR #59: Fix GitHub ref swapping by cleaning stale Gemfile.lock entriesSummaryThis PR addresses a real pain point when swapping gem sources by removing stale Strengths1. Problem-Solution Fit
2. Code Quality
3. Enhanced Features
|
Issues & Recommendations🔴 High Priority1. Race Condition Risk in The method deletes Recommendation: Create a backup before deletion and restore it if the bundle install fails. 2. Silent Failure in Returns 🟡 Medium Priority3. Directory Traversal Without Validation (lib/demo_scripts/swap_deps_cli.rb:189-205) The 4. Dir.chdir Without Error Handling (lib/demo_scripts/swap_deps_cli.rb:172) Changing directory in 5. Inconsistent Error Messaging The second 6. Test Coverage Gap The tests mock file operations but do not test error cases or the new features (check_outdated_repos, update_outdated_repos, auto-update flag, context detection). |
Security Considerations✅ No major security issues found
Performance Considerations✅ Good optimizations:
Test Coverage✅ Strengths:
Recommendation: Add integration-style tests or at least unit tests for the new outdated repo detection feature. Overall AssessmentRating: 8/10 - Strong implementation with minor areas for improvement Recommendation: Approve with suggestions The core fix is sound and solves the stated problem effectively. The additional features (outdated detection, context detection) are valuable additions. Addressing the high-priority error handling issues would make this production-ready. Before merging:
Great work on a thorough solution to a real developer pain point! 🚀 Review generated by Claude Code |
Code Review: Fix GitHub ref swapping by cleaning stale Gemfile.lock entriesSummaryThis PR addresses a real pain point when swapping gem sources between rubygems, GitHub, and local paths. The solution of removing Gemfile.lock before installation is pragmatic and effective. Overall, this is a well-implemented fix with good test coverage. ✅ Strengths1. Core Solution is Sound
2. Enhanced User Experience
3. Context Detection Improvements
4. Performance Optimization
5. Test Coverage
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/demo_scripts/gem_swapper.rb (1)
1134-1156: Logging mismatch: message says “bundle update” but we run install.Fix user-visible text to match behavior.
Apply:
- puts ' Running bundle update (to restore gem sources)...' + puts ' Running bundle install (to restore gem sources)...'If you rename clean_gemfile_lock per prior comment, update call sites too:
- clean_gemfile_lock(demo_path, gems_to_update) + remove_gemfile_lock(demo_path)And in the swap branch:
- clean_gemfile_lock(demo_path, gems_to_update) + remove_gemfile_lock(demo_path)Also applies to: 1147-1169
♻️ Duplicate comments (2)
lib/demo_scripts/gem_swapper.rb (2)
415-427: Harden git ref usage and avoid false positives from missing remote refs.Add a remote ref existence check and avoid Dir.chdir for safety; also use File::NULL for portability. This was previously flagged; reinforcing here.
As per static analysis hints (Brakeman), apply:
- def check_commits_behind(cache_path, branch) - Dir.chdir(cache_path) do - # Fetch latest without output - system('git', 'fetch', 'origin', branch, out: '/dev/null', err: '/dev/null') - - # Count commits between local and remote - output, status = Open3.capture2('git', 'rev-list', '--count', "HEAD..origin/#{branch}") - return status.success? ? output.strip.to_i : nil - end - rescue StandardError - nil - end + def check_commits_behind(cache_path, branch) + # Fetch latest without output + system('git', '-C', cache_path, 'fetch', 'origin', branch, out: File::NULL, err: File::NULL) + + remote_ref = "origin/#{branch}" + # Verify remote ref exists + _out, status = Open3.capture2('git', '-C', cache_path, 'rev-parse', '--verify', remote_ref) + return nil unless status.success? + + # Count commits between local and remote + out, status = Open3.capture2('git', '-C', cache_path, 'rev-list', '--count', "HEAD..#{remote_ref}") + status.success? ? out.strip.to_i : nil + rescue StandardError + nil + endOptional: also guard branch validation earlier or normalize ref_type to branch-only before calling this method.
#!/bin/bash # Verify there are no remaining uses of '/dev/null' redirections and search for similar git rev-list patterns. rg -n --glob '!**/vendor/**' "/dev/null|HEAD\.\.origin/" -C2
1111-1126: Method name/param mismatch and missing error handling (already noted earlier).
- The method deletes the entire lock file irrespective of gems_to_clean.
- Unused gems_to_clean misleads callers.
- No error handling on File.delete.
Apply either rename or implement selective cleaning; minimally, add error handling:
-def clean_gemfile_lock(demo_path, gems_to_clean) - return unless gems_to_clean.any? +def remove_gemfile_lock(demo_path) lock_file = File.join(demo_path, 'Gemfile.lock') return unless File.exist?(lock_file) - puts " Cleaning lock file entries for: #{gems_to_clean.join(', ')}" if verbose + puts ' Removing Gemfile.lock to force clean resolution' if verbose - File.delete(lock_file) - puts ' Removed Gemfile.lock to force clean resolution' if verbose + begin + File.delete(lock_file) + puts ' ✓ Removed Gemfile.lock' if verbose + rescue SystemCallError => e + warn " ⚠️ Warning: Could not delete Gemfile.lock: #{e.message}" + return false + end endAnd update call sites (see below comment) accordingly.
🧹 Nitpick comments (2)
lib/demo_scripts/swap_deps_cli.rb (1)
163-186: Minor portability nits in path handling (optional).Using simple string prefix/sub may misbehave under symlinks or on Windows. Prefer Pathname.realpath + relative_path, and File::SEPARATOR-agnostic logic. Example sketch:
- Use Pathname.new(current_path).realpath.relative_path_from(Pathname.new(project_root).realpath)
- Use File::NULL instead of '/dev/null' when redirecting (cross‑platform)
No behavior change needed now; consider for robustness.
Also applies to: 188-206, 207-219
lib/demo_scripts/gem_swapper.rb (1)
385-413: Skip tag-based refs in “outdated” check; capture ref_type from Gemfile.Counting “commits behind” only makes sense for branches. Current detection doesn’t distinguish branch vs tag, so tags may be treated as branches. Recommend:
- Capture whether the Gemfile entry used branch or tag.
- Skip commits-behind for tags.
Apply these diffs outside this hunk to capture ref_type and use it here:
- GEM_GITHUB_PATTERN = lambda { |gem_name| - github_pattern = /^\s*gem\s+["']#{Regexp.escape(gem_name)}["'],\s*github:\s*["']([^"']+)["']/ - github_ref_pattern = /(?:,\s*(?:branch|tag):\s*["']([^"']+)["'])?/ - /#{github_pattern}#{github_ref_pattern}/ - } + GEM_GITHUB_PATTERN = lambda { |gem_name| + github_pattern = /^\s*gem\s+["']#{Regexp.escape(gem_name)}["'],\s*github:\s*["']([^"']+)["']/ + # capture 1: repo, 2: ref_type (branch|tag), 3: ref value + github_ref_pattern = /(?:,\s*(branch|tag):\s*["']([^"']+)["'])?/ + /#{github_pattern}#{github_ref_pattern}/ + }- github_match = gemfile_content.match(GEM_GITHUB_PATTERN.call(gem_name)) + github_match = gemfile_content.match(GEM_GITHUB_PATTERN.call(gem_name)) ... - elsif github_match - ref = github_match[2] || 'main' - swapped_gems << { name: gem_name, type: 'github', path: "#{github_match[1]}@#{ref}" } + elsif github_match + repo = github_match[1] + ref_type = (github_match[2] || 'branch').to_sym + ref = github_match[3] || 'main' + swapped_gems << { name: gem_name, type: 'github', path: "#{repo}@#{ref}", ref_type: ref_type }Then in this method:
- swapped_gems.each do |gem| + swapped_gems.each do |gem| next unless gem[:type] == 'github' + # Only branches can be "behind" + next if gem[:ref_type] == :tag
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/demo_scripts/gem_swapper.rb(5 hunks)lib/demo_scripts/swap_deps_cli.rb(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lib/demo_scripts/gem_swapper.rb (2)
lib/demo_scripts/swap_deps_cli.rb (1)
each_demo(555-563)lib/demo_scripts/demo_manager.rb (1)
each_demo(19-30)
lib/demo_scripts/swap_deps_cli.rb (1)
lib/demo_scripts/gem_swapper.rb (1)
show_status(139-165)
🪛 Brakeman (7.1.0)
lib/demo_scripts/gem_swapper.rb
[medium] 422-422: Possible command injection
Type: Command Injection
Confidence: Medium
More info: https://brakemanscanner.org/docs/warning_types/command_injection/
(Command Injection)
🔇 Additional comments (4)
lib/demo_scripts/swap_deps_cli.rb (3)
13-13: Auto-update plumbing looks good.attr_reader, default true, CLI flag wiring, and propagation to show_status are consistent.
Also applies to: 35-35, 322-325, 487-490
43-46: Safe gating of bundler/setup for read-only commands.Good separation; avoids unnecessary boot time for status/cache/watch-only flows.
Also applies to: 88-92
1296-1299: Updated next-steps messaging: LGTM.lib/demo_scripts/gem_swapper.rb (1)
138-165: Status + auto-update flow: LGTM.Clear output; dedupes repos; conditional auto-update reads CLI flag.
Addresses code review feedback with critical safety improvements: 1. Fixed race condition in clean_gemfile_lock: - Now creates backup before deleting lock file - Restores backup if bundle install fails - Cleans up backup on success - Prevents loss of working lock file state 2. Added verbose warnings to check_commits_behind: - Warns when git fetch fails - Warns when commit counting fails - Provides context for debugging - Helps distinguish errors from being up-to-date 3. Added error handling for Dir.chdir: - Wrapped in begin/rescue block - Raises clear error message on failure - Prevents process from being stuck in wrong directory 4. Validated directory traversal in find_project_root: - Uses File.realpath to resolve symlinks - Tracks visited directories to detect circular references - Catches and handles SystemCallError - Protects against symlink attacks These changes make the tool more robust and production-ready, with proper error handling and recovery mechanisms. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Code Review - PR #59Thank you for this comprehensive PR! The implementation addresses a real pain point with gem dependency swapping and includes several valuable features. SummaryThis PR successfully fixes the "Could not find gem" errors when swapping between different gem sources by removing stale Gemfile.lock entries. The solution is solid, with good error handling and backup mechanisms added in the latest commit. Strengths1. Excellent Safety ImprovementsThe backup/restore mechanism in clean_gemfile_lock is well-designed:
2. Good Error Handling
3. Security Considerations
4. Great UX Features
5. Test CoverageTests were properly updated to reflect the new flow Issues & RecommendationsCritical: Race Condition RiskLocation: lib/demo_scripts/gem_swapper.rb:1133-1142 Issue: Potential race condition between creating the backup and deleting the lock file. Recommendation: Add error handling to file operations to make the process more robust. Minor: Incomplete Error HandlingLocation: lib/demo_scripts/gem_swapper.rb:1135, 1150, 1158 Issue: File operations can fail (permission denied, disk full) but aren't wrapped in error handling. Recommendation: Wrap FileUtils.cp, File.delete, FileUtils.mv, and FileUtils.rm in begin/rescue blocks. Minor: Missing Test CoverageWhat's missing:
Performance: Redundant System CallsLocation: lib/demo_scripts/gem_swapper.rb:419-426 Issue: check_commits_behind fetches on every check. Recommendation: Consider caching fetch results for ~5 minutes. Security AssessmentGood practices:
Consider:
Test Coverage AssessmentCurrent:
Recommended:
VerdictRecommendation: Approve with minor suggestions This PR is well-implemented with good safety mechanisms. The issues noted are minor and can be addressed in follow-up PRs. Priority:
Great work! This will significantly improve the developer experience. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/demo_scripts/gem_swapper.rb (1)
1169-1196: Log message says “bundle update” but code runs install.Align the message with actual command to avoid confusion.
- puts ' Running bundle update (to restore gem sources)...' + puts ' Running bundle install (to restore gem sources)...'
♻️ Duplicate comments (3)
lib/demo_scripts/gem_swapper.rb (3)
416-433: Harden git rev-list call; verify remote ref first.Validate ref characters, ensure remote ref exists, then run rev-list. Addresses Brakeman warning and tag/invalid ref cases.
- def check_commits_behind(cache_path, branch) + def check_commits_behind(cache_path, branch) + # Basic sanity check to avoid unsafe ref usage + return nil unless branch.is_a?(String) && branch.match?(/\A[\w.\-\/]+\z/) Dir.chdir(cache_path) do # Fetch latest without output fetch_success = system('git', 'fetch', 'origin', branch, out: '/dev/null', err: '/dev/null') unless fetch_success warn " ⚠️ Warning: Failed to fetch branch #{branch} from #{cache_path}" if verbose return nil end - - # Count commits between local and remote - output, status = Open3.capture2('git', 'rev-list', '--count', "HEAD..origin/#{branch}") + remote_ref = "origin/#{branch}" + # Verify remote ref exists (skips tags or missing refs) + _out, status = Open3.capture2('git', 'rev-parse', '--verify', remote_ref) + return nil unless status.success? + # Count commits between local and remote + output, status = Open3.capture2('git', 'rev-list', '--count', "HEAD..#{remote_ref}") unless status.success? warn " ⚠️ Warning: Failed to count commits for #{branch} in #{cache_path}" if verbose return nil end
1121-1161: Method name vs behavior; add error handling.The method deletes the entire lockfile regardless of gems passed; param is unused beyond logging, and failures aren’t handled.
Option A — rename to reflect behavior and add error handling:
- # Removes stale gem entries from Gemfile.lock to prevent bundle update failures - # Returns the backup path if created, nil otherwise - def clean_gemfile_lock(demo_path, gems_to_clean) + # Removes Gemfile.lock to force a clean resolution. Returns backup path or nil. + def remove_gemfile_lock(demo_path, gems_to_clean = nil) return nil unless gems_to_clean.any? @@ - FileUtils.cp(lock_file, backup_lock) + begin + FileUtils.cp(lock_file, backup_lock) + rescue SystemCallError => e + warn " ⚠️ Warning: Could not back up Gemfile.lock: #{e.message}" + return nil + end @@ - File.delete(lock_file) + begin + File.delete(lock_file) + rescue SystemCallError => e + warn " ⚠️ Warning: Could not delete Gemfile.lock: #{e.message}" + return backup_lock + endAnd update call sites:
- lock_backup = clean_gemfile_lock(demo_path, gems_to_update) + lock_backup = remove_gemfile_lock(demo_path, gems_to_update) @@ - lock_backup = clean_gemfile_lock(demo_path, gems_to_update) + lock_backup = remove_gemfile_lock(demo_path, gems_to_update)Option B — keep name but document and still add error handling (if rename is undesirable).
385-413: Skip “behind” checks for tags and validate ref from Gemfile.Gemfile lines don’t preserve branch vs tag; treating a tag like a branch will fail fetch/rev-list. Add a remote-ref existence check and bail early if not a branch ref.
- commits_behind = check_commits_behind(cache_path, ref) + commits_behind = check_commits_behind(cache_path, ref) next unless commits_behind&.positive?And within check_commits_behind (below), verify
origin/<ref>exists; tags will naturally be skipped.
🧹 Nitpick comments (4)
lib/demo_scripts/swap_deps_cli.rb (3)
163-191: Use realpath for current_dir to match project_root resolution.Prevents false negatives when cwd is a symlink.
- current_dir = Dir.pwd + current_dir = File.realpath(Dir.pwd) rescue Dir.pwd
199-223: Depth limiter may miss project roots in deep trees.Limit of 10 can fail in monorepos. Prefer loop-to-root with visited set.
- max_depth = 10 # Prevent infinite loops - - max_depth.times do + loop do # Check if we've seen this directory before (circular reference) return nil if visited.include?(dir) @@ - break if parent == dir # Reached filesystem root + break if parent == dir # Reached filesystem root
224-236: Harden demo detection against symlinks.Compare realpaths to avoid misclassification.
- def detect_demo_from_path(current_path, project_root) - return nil unless current_path.start_with?(project_root) + def detect_demo_from_path(current_path, project_root) + current_real = File.realpath(current_path) rescue current_path + return nil unless current_real.start_with?(project_root) - relative_path = current_path.sub("#{project_root}/", '') + relative_path = current_real.sub("#{project_root}/", '')lib/demo_scripts/gem_swapper.rb (1)
139-165: Status with auto-update — looks good.Clear summary and guarded updates. Consider sorting by commits_behind desc for readability later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/demo_scripts/gem_swapper.rb(6 hunks)lib/demo_scripts/swap_deps_cli.rb(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lib/demo_scripts/gem_swapper.rb (2)
lib/demo_scripts/swap_deps_cli.rb (1)
each_demo(572-580)lib/demo_scripts/demo_manager.rb (1)
each_demo(19-30)
lib/demo_scripts/swap_deps_cli.rb (1)
lib/demo_scripts/gem_swapper.rb (1)
show_status(139-165)
🪛 Brakeman (7.1.0)
lib/demo_scripts/gem_swapper.rb
[medium] 426-426: Possible command injection
Type: Command Injection
Confidence: Medium
More info: https://brakemanscanner.org/docs/warning_types/command_injection/
(Command Injection)
🔇 Additional comments (9)
lib/demo_scripts/swap_deps_cli.rb (6)
13-13: Expose auto_update via attr_reader — OK.Public reader aligns with CLI flag usage and downstream propagation.
35-35: Default auto_update = true — confirm desired default behavior.This will mutate caches on --status by default. If that’s intended, fine; otherwise consider defaulting to false.
43-46: Lazy require for Bundler — good separation.Avoids unnecessary env mutation for read-only commands.
88-91: read_only_command? scope looks right.Covers status/cache/watch flows that don’t need gems.
339-342: --[no-]auto-update flag — OK.Option wiring and default messaging look good.
506-507: Propagate auto_update to swapper — OK.Signature matches downstream.
lib/demo_scripts/gem_swapper.rb (3)
439-471: Auto-update flow — OK.Good use of cache update, optional rebuilds, and robust JSON handling.
1203-1209: Install path after lock removal — OK.Installing from scratch post-delete is appropriate.
1343-1346: Next steps messaging — OK.Helpful guidance; integrates with status feature.
| # Find the project root by walking up the directory tree | ||
| def find_project_root(start_dir = Dir.pwd) | ||
| # Resolve symlinks to prevent attacks and circular references | ||
| dir = File.realpath(start_dir) | ||
| visited = Set.new | ||
| max_depth = 10 # Prevent infinite loops | ||
|
|
||
| max_depth.times do |
There was a problem hiding this comment.
NameError risk: Set is used but not required.
find_project_root uses Set without require 'set'. This will crash at runtime.
require 'optparse'
+require 'set'Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/demo_scripts/swap_deps_cli.rb around lines 194 to 201, the method
find_project_root uses Set but the file doesn’t require 'set', which will raise
a NameError at runtime; fix by adding require 'set' at the top of the file (or
immediately before find_project_root) so Set is defined before use, ensuring the
require is only added once and keeping require ordering consistent with existing
requires in this file.
Summary
Problem
When swapping gems between different sources (rubygems ↔ GitHub ↔ local path), the Gemfile.lock can contain stale references:
Solution
Added
clean_gemfile_lockmethod that removes the Gemfile.lock before running bundle commands when swapping gem sources. This forces Bundler to regenerate lock entries from scratch based on the updated Gemfile.Test plan
bin/swap-deps --github 'shakacode/shakapacker#justin808/early-hints'bundle installworks after swapbundle installworks after restore🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests