Skip to content

Fix GitHub ref swapping by cleaning stale Gemfile.lock entries #59

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Oct 18, 2025
+277 −40
Merged

Fix GitHub ref swapping by cleaning stale Gemfile.lock entries #59

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
827cf36
Fix GitHub ref swapping by cleaning stale Gemfile.lock entries
justin808 Oct 17, 2025
e45f1e5
Suggest --watch flag in next steps when not using watch mode
justin808 Oct 17, 2025
c014e36
Remove confusing <gem-path> placeholder from next steps
justin808 Oct 17, 2025
93cd90b
Add outdated check to --status command
justin808 Oct 17, 2025
dff7ad3
Fix bundler error when running --status from demo directory
justin808 Oct 17, 2025
6fb0937
Allow swap-deps to run from any directory within the project
justin808 Oct 17, 2025
a0d816f
Add --auto-update flag to automatically refresh outdated repos
justin808 Oct 17, 2025
5674136
Make auto-update the default for --status
justin808 Oct 17, 2025
0223a54
Add safety improvements and error handling
justin808 Oct 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 90 additions & 7 deletions lib/demo_scripts/gem_swapper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,22 @@ def kill_watch_processes
def show_status
puts '📊 Swapped dependencies status:'

outdated_repos = []

each_demo do |demo_path|
show_demo_status(demo_path)
outdated_repos.concat(check_outdated_repos(demo_path))
end

return unless outdated_repos.any?

puts "\n⚠️ Outdated GitHub repositories detected:"
outdated_repos.uniq.each do |repo_info|
commits = repo_info[:commits_behind]
puts " #{repo_info[:gem_name]} (#{repo_info[:repo]}@#{repo_info[:ref]}) - #{commits} commit(s) behind"
end
puts "\n💡 To update: Re-run your swap command to fetch latest changes"
puts " Example: bin/swap-deps --github '#{outdated_repos.first[:repo]}##{outdated_repos.first[:ref]}'"
end

# CLI entry point: Display cache information including location, size, and cached repositories
Expand Down Expand Up @@ -361,6 +374,50 @@ def detect_git_branch(path)
nil
end

# Check if any GitHub-based swapped gems are outdated
def check_outdated_repos(demo_path)
gemfile_path = File.join(demo_path, 'Gemfile')
return [] unless File.exist?(gemfile_path)

swapped_gems = detect_swapped_gems(gemfile_path)
outdated = []

swapped_gems.each do |gem|
next unless gem[:type] == 'github'

repo, ref = gem[:path].split('@', 2)
cache_path = github_cache_path(gem[:name], { repo: repo, branch: ref })

next unless File.directory?(cache_path)

commits_behind = check_commits_behind(cache_path, ref)
next unless commits_behind&.positive?

outdated << {
gem_name: gem[:name],
repo: repo,
ref: ref,
commits_behind: commits_behind
}
end

outdated
end

# 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
Comment on lines 415 to 437
Copy link

@coderabbitai coderabbitai bot Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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
     end

Wait, 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
     end

Actually, 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
     end

This 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.


def cache_repo_dirs
return [] unless File.directory?(CACHE_DIR)

Expand Down Expand Up @@ -1009,6 +1066,23 @@ def find_supported_gems_in_gemfile(demo_path)
end
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 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
Comment on lines 1121 to 1143
Copy link

@coderabbitai coderabbitai bot Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. Unused parameter: The gems_to_clean parameter is only checked for emptiness but never used for selective cleaning.
  2. 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.
  3. Missing error handling: File.delete could 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.

Suggested change
# 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


# rubocop:disable Metrics/MethodLength, Metrics/AbcSize
def run_bundle_install(demo_path, for_restore: false)
return if dry_run
Expand All @@ -1028,21 +1102,28 @@ def run_bundle_install(demo_path, for_restore: false)
system('bundle', 'install', '--quiet')
end
else
# Clean lock file to prevent "Could not find gem" errors
clean_gemfile_lock(demo_path, gems_to_update)

puts " Updating gems: #{gems_to_update.join(', ')}" if verbose
success = Dir.chdir(demo_path) do
# Update specific gems to pull from rubygems
result = system('bundle', 'update', *gems_to_update, '--quiet')
warn ' ⚠️ ERROR: Failed to update gems. Lock file may be inconsistent.' unless result
# 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
result
end
end
elsif gems_to_update.any?
# When swapping to local/GitHub dependencies, we need to update the gems
# to resolve potential lock file conflicts (e.g., version no longer available in new source)
puts ' Running bundle update (to resolve swapped gem sources)...'
puts " Updating gems: #{gems_to_update.join(', ')}" if verbose
# Clean lock file to prevent "Could not find gem" errors
clean_gemfile_lock(demo_path, gems_to_update)

puts ' Running bundle install (to resolve swapped gem sources)...'
puts " Installing gems: #{gems_to_update.join(', ')}" if verbose
success = Dir.chdir(demo_path) do
system('bundle', 'update', *gems_to_update, '--quiet')
# Install from scratch since we removed the lock file
system('bundle', 'install', '--quiet')
end
else
puts ' Running bundle install...'
Expand Down Expand Up @@ -1170,7 +1251,9 @@ def print_next_steps
puts ' - List: bin/swap-deps --list-watch'
puts ' - Stop: bin/swap-deps --kill-watch'
else
puts ' 4. Rebuild packages when needed: cd <gem-path> && npm run build'
puts ' 4. Rebuild packages when needed:'
puts ' - See paths: bin/swap-deps --status'
puts ' - Auto-rebuild: Re-run with --watch flag for automatic rebuilds on file changes'
end

puts "\n To restore: bin/swap-deps --restore"
Expand Down
10 changes: 8 additions & 2 deletions lib/demo_scripts/swap_deps_cli.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,9 @@ def run!
detect_context!
parse_options!

# Require bundler/setup only when actually running commands (not for --help)
require 'bundler/setup'
# Only require bundler/setup for commands that actually need gem dependencies
# Status, cache, and watch commands only use git/filesystem, not gems
require 'bundler/setup' unless read_only_command?

if @show_status
show_status_info
Expand Down Expand Up @@ -83,6 +84,11 @@ def run!

private

# Check if the command is read-only (doesn't need bundler/setup)
def read_only_command?
@show_status || @show_cache || @list_watch || @kill_watch || @clean_cache || @clean_cache_gem
end

# rubocop:disable Lint/DuplicateBranch
def process_gem_value(gem_name, value)
# Check if it's a local filesystem path first (before checking for GitHub specs)
Expand Down
16 changes: 9 additions & 7 deletions spec/demo_scripts/gem_swapper_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -731,17 +731,18 @@
end

context 'when for_restore is true' do
it 'runs bundle update for supported gems' do
it 'runs bundle install for supported gems after cleaning lock file' do
gemfile_content = <<~GEMFILE
gem 'rails'
gem 'shakapacker', '~> 9.0'
gem 'react_on_rails'
GEMFILE

allow(File).to receive(:read).with(gemfile_path).and_return(gemfile_content)
expect(swapper).to receive(:clean_gemfile_lock).with(demo_path, %w[shakapacker react_on_rails])
expect(Dir).to receive(:chdir).with(demo_path).and_yield
expect(swapper).to receive(:system)
.with('bundle', 'update', 'shakapacker', 'react_on_rails', '--quiet').and_return(true)
.with('bundle', 'install', '--quiet').and_return(true)

result = swapper.send(:run_bundle_install, demo_path, for_restore: true)
expect(result).to be true
Expand All @@ -761,9 +762,10 @@
gemfile_content = "gem 'shakapacker'"

allow(File).to receive(:read).with(gemfile_path).and_return(gemfile_content)
expect(swapper).to receive(:clean_gemfile_lock).with(demo_path, %w[shakapacker])
expect(Dir).to receive(:chdir).with(demo_path).and_yield
expect(swapper).to receive(:system).with('bundle', 'update', 'shakapacker', '--quiet').and_return(false)
expect(swapper).to receive(:warn).with(/ERROR: Failed to update gems/)
expect(swapper).to receive(:system).with('bundle', 'install', '--quiet').and_return(false)
expect(swapper).to receive(:warn).with(/ERROR: Failed to install gems/)
expect(swapper).to receive(:warn).with(/ERROR: bundle command failed/)

result = swapper.send(:run_bundle_install, demo_path, for_restore: true)
Expand All @@ -772,17 +774,17 @@
end

context 'when for_restore is false' do
it 'runs bundle update for swapped gems' do
it 'runs bundle install for swapped gems after cleaning lock file' do
gemfile_content = <<~GEMFILE
gem 'rails'
gem 'shakapacker', path: '/path/to/shakapacker'
gem 'react_on_rails', github: 'shakacode/react_on_rails'
GEMFILE

allow(File).to receive(:read).with(gemfile_path).and_return(gemfile_content)
expect(swapper).to receive(:clean_gemfile_lock).with(demo_path, %w[shakapacker react_on_rails])
expect(Dir).to receive(:chdir).with(demo_path).and_yield
expect(swapper).to receive(:system).with('bundle', 'update', 'shakapacker', 'react_on_rails',
'--quiet').and_return(true)
expect(swapper).to receive(:system).with('bundle', 'install', '--quiet').and_return(true)

swapper.send(:run_bundle_install, demo_path, for_restore: false)
end
Expand Down