Add GitHub shorthand syntax and auto-detection to swap-deps #56
Conversation
WalkthroughConsolidates dependency status display in DependencySwapper, adds GitHub default-branch detection and local git-branch detection, and removes separate gem/package presenters. Enhances SwapDepsCLI to parse flexible inputs (local paths, full/shorthand GitHub specs), introducing a default repo map and new parsing helpers. Updates option handling and help text accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant DS as DependencySwapper
participant GH as Git Remote
participant FS as Filesystem
U->>DS: show_demo_status()
DS->>DS: swapped_gems.empty? check
alt No swapped gems
DS-->>U: return (no dependencies)
else Has swapped gems
DS->>DS: display_swapped_dependencies()
loop each gem
DS->>DS: display_dependency_info(gem)
alt Local path
DS->>FS: detect_git_branch(path)
FS-->>DS: branch name or nil
DS-->>U: print local path + branch
else GitHub spec
DS->>DS: validate_github_repos()
DS->>GH: detect_default_branch(repo)
GH-->>DS: default branch (or fallback)
DS-->>U: print repo + ref/branch
end
end
end
sequenceDiagram
autonumber
actor U as User
participant CLI as SwapDepsCLI
participant H as Helpers
U->>CLI: run with options
loop for each option value
CLI->>H: process_gem_value(gem, value)
alt Local path
H-->>CLI: add to @gem_paths
else GitHub spec (full)
H->>H: parse_github_spec_without_shorthand()
H-->>CLI: normalized [repo, ref, type]
CLI->>CLI: update @github_repos
else Shorthand (#branch/@tag)
H->>H: parse_github_spec(gem, shorthand) using GEM_REPOS
H-->>CLI: resolved [repo, ref, type]
CLI->>CLI: update @github_repos
end
end
CLI-->>U: proceed with swaps using @gem_paths/@github_repos
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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. Comment |
Code Review - PR #56: Add GitHub shorthand syntax and auto-detection to swap-depsOverviewThis PR adds convenient shorthand syntax for specifying GitHub dependencies and auto-detection of default branches. The implementation is well-structured and follows good practices overall. ✅ Strengths
🔍 Code Quality Issues1. Security: Command Injection Risk (lib/demo_scripts/gem_swapper.rb:651)Severity: Medium The detect_default_branch method uses backticks with string interpolation which is a risky pattern. The existing codebase uses array-form system() calls elsewhere (lines 712-713) for safety. Recommendation: Use Open3.capture2 (already required at line 8) instead of backticks to follow the existing pattern in the codebase. 2. Potential Bug: Nil Reference Handling (lib/demo_scripts/swap_deps_cli.rb:90-91)Severity: Low-Medium In process_gem_value, the ref can be nil (for auto-detection), but it's directly assigned to :branch. While this is handled later in validate_github_repos (line 638), it creates temporary state with nil values. Recommendation: Document this intentional behavior with a comment. 3. Code Duplication (lib/demo_scripts/swap_deps_cli.rb:99-137 & 188-204)Severity: Low The GitHub spec parsing logic appears twice: in parse_github_spec and in the --github option handler. Recommendation: Refactor the --github option to use the shared parsing method. 4. Network Call During Initialization (lib/demo_scripts/gem_swapper.rb:638)Severity: Low Auto-detection happens during validate_github_repos, which is called from the initializer. This means network calls occur during object construction, making initialization slower and harder to test. Recommendation: Consider lazy evaluation or moving detection to the clone_github_repos! phase where other network operations happen. 🧪 Test Coverage ConcernsThe PR mentions test plan items are checked off, but I don't see unit tests for the new parsing logic, edge case tests for malformed input, tests for auto-detection with mocked network calls, or tests for error conditions. Recommendation: Add test coverage in a follow-up PR or before merging, especially for parse_github_spec with various inputs and detect_default_branch with mocked git commands. 🔒 Security AssessmentGood: Input validation with regex, use of array-form system calls in most places Concerns: Backtick usage in detect_default_branch (see issue #1 above) Verdict: Generally secure with one recommended fix. 📋 Minor Issues
✨ Recommendations SummaryBefore merging:
Follow-up (nice to have): 🎉 ConclusionThis is a high-quality PR that adds valuable functionality. The shorthand syntax will significantly improve developer experience. With the security fix for command injection, this is ready to merge. Recommendation: Approve with minor changes Great work! The feature design is thoughtful and the implementation is clean. 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
demos/basic-v16-rspack/.gitignore(1 hunks)demos/basic-v16-rspack/babel.config.js(0 hunks)demos/basic-v16-rspack/bin/export-bundler-config(1 hunks)lib/demo_scripts/gem_swapper.rb(3 hunks)lib/demo_scripts/swap_deps_cli.rb(4 hunks)
💤 Files with no reviewable changes (1)
- demos/basic-v16-rspack/babel.config.js
🧰 Additional context used
🧬 Code graph analysis (1)
lib/demo_scripts/gem_swapper.rb (1)
lib/demo_scripts/github_spec_parser.rb (1)
validate_github_branch(53-74)
🪛 Brakeman (7.1.0)
lib/demo_scripts/gem_swapper.rb
[medium] 634-634: Possible command injection
Type: Command Injection
Confidence: Medium
More info: https://brakemanscanner.org/docs/warning_types/command_injection/
(Command Injection)
Pull Request Review - PR #56OverviewThis PR adds GitHub shorthand syntax and auto-detection to the ✅ StrengthsCode Quality
Features
Best Practices
🔍 Code Review ObservationsMinor Issues & Suggestions1. RuboCop Directive Duplication (swap_deps_cli.rb:86-102)The # Current implementation has duplicate branches
if File.exist?(expanded_path) || value.start_with?('./', '../')
@gem_paths[gem_name] = value
elsif value.start_with?('#', '@') || value.include?('/')
# GitHub processing
else
@gem_paths[gem_name] = value # Duplicate!
endSuggestion: Could simplify by handling GitHub specs first, defaulting to local path: if value.start_with?('#', '@') || (value.include?('/') && !File.exist?(File.expand_path(value)))
repo, ref, ref_type = parse_github_spec(gem_name, value)
@github_repos[gem_name] = { repo: repo, branch: ref, ref_type: ref_type }
else
@gem_paths[gem_name] = value
end2. Default Branch Detection Network Dependency (gem_swapper.rb:647-661)The
Suggestions:
output = `timeout 5 git ls-remote --symref https://github.com/#{repo}.git HEAD 2>/dev/null`3. Inconsistent
|
| Category | Rating | Notes |
|---|---|---|
| Code Quality | ⭐⭐⭐⭐⭐ | Excellent structure, well-documented |
| Security | ⭐⭐⭐⭐½ | Very good with minor network considerations |
| Testing | ⭐⭐⭐⭐ | Comprehensive, missing one method test |
| Performance | ⭐⭐⭐⭐ | Good approach, room for parallel optimization |
| Documentation | ⭐⭐⭐⭐½ | Great help text and examples |
| User Experience | ⭐⭐⭐⭐⭐ | Significant improvement with shorthand syntax |
🎬 Recommendations
Must Address
- None - the PR is in good shape!
Should Address
- Consider adding test for
detect_default_branchmethod - Add timeout to git ls-remote call to prevent hanging
- Consider refactoring
process_gem_valueto eliminate RuboCop directive
Nice to Have
- Add parallel branch detection for multiple repos
- Cache branch detection results for session duration
- Document the babel.config.js removal in demo README
- Add integration test for full shorthand workflow
🏁 Conclusion
Recommendation: ✅ APPROVE with minor suggestions
This is a high-quality PR that significantly improves developer experience. The code is well-tested, secure, and follows best practices. The shorthand syntax and auto-detection features are thoughtfully implemented. The suggestions above are mostly minor optimizations and enhancements rather than critical issues.
The PR demonstrates:
- Strong understanding of Ruby best practices
- Security awareness
- Excellent testing discipline
- User-centric design
Great work! 🎉
Review generated by Claude Code - PR #56
Pull Request Review - PR #56SummaryThis PR adds GitHub shorthand syntax and auto-detection features to the swap-deps CLI tool. The changes enhance developer experience by supporting shorthand notation for gem-specific flags and automatically detecting default branches. Code Quality & Best PracticesStrengths:
|
Areas for Improvement:1. Network Call During Object Construction (lib/demo_scripts/gem_swapper.rb:638)The detect_default_branch method makes a network call (git ls-remote) during the validation phase. While there's a comment acknowledging this design choice, it has implications: Concerns:
Recommendations:
|
2. Security: Command Injection Risk (lib/demo_scripts/gem_swapper.rb:641)While the comment states "Use array form to avoid shell injection (repo is already validated)", there's still a potential issue with the git ls-remote call. Analysis:
Recommendations:
|
3. Error Handling in process_gem_value (lib/demo_scripts/swap_deps_cli.rb:87-102)The RuboCop disable for Lint/DuplicateBranch suggests potential refactoring opportunities. Issues:
Recommendations:
|
4. Missing Test Coverage for New FeaturesLooking at the PR description, I see "Test plan" items checked off, but the diff doesn't show new test files. Missing tests for:
Recommendations:
|
Performance Considerations
Security Concerns
|
Additional ObservationsPositive:
Questions:
|
Recommendations SummaryHigh Priority:
Medium Priority:
Nice to Have:
ConclusionThis is a valuable feature enhancement that improves the developer experience. The code is generally well-structured and documented. The main concerns are around:
With these improvements, this will be a solid addition to the codebase! Great work on the comprehensive help text and shell-specific documentation. Review generated with assistance from Claude Code |
Pull Request ReviewGreat work on adding GitHub shorthand syntax and auto-detection! This significantly improves the developer experience. Strengths
Issues Found1. Network Error Handling (gem_swapper.rb:684-699)The detect_default_branch method makes network calls during init but lacks timeout handling. Could hang indefinitely on network issues. Suggestion: Add timeout parameter to Open3.capture2 2. Edge Case: Ambiguous Paths (swap_deps_cli.rb:88-92)File.exist? check could misidentify repo names as local paths if a matching directory exists. Example: Directory named shakacode would conflict with shakacode/shakapacker 3. Duplicate Branch Logic (swap_deps_cli.rb:86-103)The RuboCop disable for Lint/DuplicateBranch indicates if/else branches doing same thing. Could be refactored. 4. Security: Input ValidationVerify validate_github_repo properly sanitizes input before string interpolation in detect_default_branch Test Coverage Recommendations
Performance Notes
Overall Assessment: 8.5/10Solid PR with great UX improvements. Main concerns are network error handling and edge case resolution. Once hardened, this will be production-ready. Must Address
Great work! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
lib/demo_scripts/gem_swapper.rb (1)
630-652: Default-branch detection: add timeout; address Brakeman warning
- Add a short timeout to avoid hangs when git is slow/offline.
- Brakeman flags possible command injection (Line 642). Using array form and prior repo validation likely mitigates it; please confirm
validate_github_repoenforces a strict safe pattern.Apply this diff:
@@ -require 'open3' +require 'open3' +require 'timeout' @@ - output, status = Open3.capture2('git', 'ls-remote', '--symref', "https://github.com/#{repo}.git", 'HEAD') + output, status = nil, nil + begin + Timeout.timeout(5) do + output, status = Open3.capture2('git', 'ls-remote', '--symref', "https://github.com/#{repo}.git", 'HEAD') + end + rescue Timeout::Error + status = Struct.new(:success?).new(false) + output = '' + endPlease verify
validate_github_repolooks like:# expected: only [\w.\-]+/[\w.\-]+ allowed def validate_github_repo(repo) # ... strict allowlist for owner/repo ... endIf not, add such validation to fully silence the warning. (Based on static analysis hints)
lib/demo_scripts/swap_deps_cli.rb (2)
86-104: Local-path vs GitHub detection is solid; add guard for full URLsGreat fix prioritizing real paths. One edge case: users may pass full Git URLs (https://..., [email protected]:...). These will be treated as GitHub specs and misparsed. Recommend rejecting or normalizing these early for clearer UX.
def process_gem_value(gem_name, value) # Check if it's a local filesystem path first (before checking for GitHub specs) expanded_path = File.expand_path(value) - if File.exist?(expanded_path) || value.start_with?('./', '../') + if File.exist?(expanded_path) || value.start_with?('./', '../') # It's a local path @gem_paths[gem_name] = value - elsif value.start_with?('#', '@') || value.include?('/') + elsif value.match?(%r{\Ahttps?://}) || value.start_with?('git@') + raise Error, "Unsupported Git URL format: #{value}. Use 'user/repo', 'user/repo#branch', or 'user/repo@tag'." + elsif value.start_with?('#', '@') || value.include?('/') # It's a GitHub spec repo, ref, ref_type = parse_github_spec(gem_name, value) # NOTE: ref can be nil for auto-detection; validate_github_repos will detect default branch later @github_repos[gem_name] = { repo: repo, branch: ref, ref_type: ref_type } else # Default to local path (for simple names without paths or GitHub markers) @gem_paths[gem_name] = value end end
105-125: Parsing helpers look good; minor robustnessConsider trimming whitespace and early-rejecting empty shorthand (just '#' or '@') for clearer CLI errors (validation later will catch but message here can be friendlier).
def parse_github_spec_without_shorthand(spec) - if spec.include?('@') + spec = spec.strip + if spec.include?('@') # Full: user/repo@tag repo, ref = spec.split('@', 2) ref_type = :tag elsif spec.include?('#') # Full: user/repo#branch repo, ref = spec.split('#', 2) ref_type = :branch else # Just repo name: user/repo repo = spec ref = nil # Will auto-detect default branch ref_type = :branch end - + raise Error, 'Invalid GitHub spec: empty ref' if ref&.empty? [repo, ref, ref_type] endAlso applies to: 126-155
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/demo_scripts/gem_swapper.rb(3 hunks)lib/demo_scripts/swap_deps_cli.rb(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/demo_scripts/gem_swapper.rb (1)
lib/demo_scripts/github_spec_parser.rb (1)
validate_github_branch(53-74)
🪛 Brakeman (7.1.0)
lib/demo_scripts/gem_swapper.rb
[medium] 642-642: Possible command injection
Type: Command Injection
Confidence: Medium
More info: https://brakemanscanner.org/docs/warning_types/command_injection/
(Command Injection)
⏰ 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/swap_deps_cli.rb (1)
77-82: LGTM: default repo mapping, CLI flags, and help examples
- GEM_REPOS mapping aligns with project defaults.
- Gem-specific flags now accept shorthand/full specs; help clarifies zsh quoting.
- --github flow infers gem correctly.
Also applies to: 187-199, 205-211, 302-328
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/demo_scripts/gem_swapper.rb (1)
633-642: Tag support added but update/cache logic not yet adapted.The code now parses and stores
ref_type(:tagor:branch) in the validation phase (lines 634, 641). However, the concerns raised in the previous review aboutupdate_github_repoandgithub_cache_pathnot handling tags correctly remain unaddressed. Those methods (lines 738-748 and 722-728) still assume branch-style refs and will fail whenref_type: :tagis used.The previous review's suggested fixes for
update_github_repo(conditional fetch/checkout for tags) andgithub_cache_path(includeref_typein cache directory name) should still be applied to fully support tag refs.
🧹 Nitpick comments (2)
lib/demo_scripts/gem_swapper.rb (2)
305-331: LGTM with optional documentation suggestion.The dependency display logic correctly handles both GitHub and local path specs, resolves cache paths, checks directory existence, and detects git branches. The implementation is sound.
Consider adding a brief comment on line 314 explaining that the
/\.$pattern removes the npm package self-reference (e.g.,path/to/package/.→path/to/package):- # Clean up path (remove trailing /. from npm packages) + # Clean up path (remove trailing /. from npm packages - npm uses '.' to reference package root) path = gem[:path].sub(%r{/\.$}, '')
3-3: Remove unusedrequire 'English'. No English aliases are referenced; delete this line.
📜 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 (1)
lib/demo_scripts/github_spec_parser.rb (1)
validate_github_branch(53-74)
🪛 Brakeman (7.1.0)
lib/demo_scripts/gem_swapper.rb
[medium] 672-672: 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)
333-342: LGTM!The
detect_git_branchmethod safely detects the current branch for local git repositories usinggit rev-parse. The implementation properly handles non-git directories, command failures, and exceptions by returningnil.Note: When HEAD is detached (e.g., checked out to a specific commit), this will return 'HEAD' rather than a branch name, which is acceptable for the status display use case.
660-682: No command injection risk in detect_default_branch
The array-formOpen3.capture2call, combined withvalidate_github_repoenforcing%r{\A[\w.-]+/[\w.-]+\z}, guarantees the repo string is safe.
234-244: Confirm package.json swap displayThe
detect_swapped_packagesmethod is defined but never invoked; if package.json swaps should still be shown in--status, reintroduce its call and related display logic.
649-651: Empty or invalid refs already error before auto-detection.
•parse_github_spec_without_shorthandraises on empty ref after@or#.
• Any empty-string branch (e.g. from shorthand"#") is caught byvalidate_github_branchbefore or after auto-detect.
Auto-detection only runs whenresult[:branch]is genuinelynil.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/demo_scripts/gem_swapper.rb (1)
741-751: Critical: Tag handling still broken (duplicate of past review).The past review comment correctly identified that
update_github_repobreaks with tag refs because it usesorigin/#{info[:branch]}(line 748), which doesn't work for tags. Additionally,github_cache_path(lines 725-731) still doesn't includeref_type, causing cache collisions when a branch and tag share the same name.These issues remain unresolved and will cause failures when users specify tags (e.g.,
@v9.0.0).The fixes suggested in the past review comment are still applicable and should be implemented.
🧹 Nitpick comments (1)
lib/demo_scripts/gem_swapper.rb (1)
3-3: Remove unusedrequire 'English'. No special variables from the English module are used—delete line 3 inlib/demo_scripts/gem_swapper.rb.
📜 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 (1)
lib/demo_scripts/github_spec_parser.rb (1)
validate_github_branch(53-74)
🪛 Brakeman (7.1.0)
lib/demo_scripts/gem_swapper.rb
[medium] 675-675: Possible command injection
Type: Command Injection
Confidence: Medium
More info: https://brakemanscanner.org/docs/warning_types/command_injection/
(Command Injection)
🔇 Additional comments (3)
lib/demo_scripts/gem_swapper.rb (3)
298-303: LGTM! Cleaner dependency display.The refactored display logic provides a unified approach for presenting both GitHub and local dependencies, improving code maintainability.
305-345: LGTM! Well-structured dependency display logic.The implementation correctly:
- Distinguishes between GitHub and local dependencies
- Normalizes npm package paths by removing trailing
/.- Validates directory existence
- Detects git branches for local repos using safe command execution
- Provides comprehensive status information with appropriate indicators
The error handling in
detect_git_branchappropriately returnsnilon failure, preventing crashes while still providing useful information when available.
636-636: Approve branch auto-detection and safe git invocation
validate_github_repoenforces a strict[\w.-]+/[\w.-]+pattern (no shell metacharacters) andOpen3.capture2’s array form passes the URL as a single argument, eliminating injection risk.
Code Review - PR #56: Add GitHub shorthand syntax and auto-detection to swap-depsSummaryThis PR adds significant improvements to the swap-deps CLI tool, including shorthand syntax for GitHub specs, automatic default branch detection, and improved status display. Overall, the implementation is solid and well-thought-out. Strengths
Issues FoundMedium Priority Issues
Minor Issues
Security Assessment - All Good!
Test Coverage ConcernNo automated test additions found in this PR. Please consider adding:
RecommendationsHigh Priority:
Medium Priority: Overall AssessmentRating: 4/5 stars This is a well-implemented feature that significantly improves the developer experience. The code is secure, follows best practices, and includes excellent documentation. The main areas for improvement are adding automated tests and handling edge cases. The feature is production-ready but would benefit from the recommended improvements before merge. Great work! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
lib/demo_scripts/swap_deps_cli.rb (4)
86-103: Consider documenting the path-precedence behaviorThe current logic prioritizes existing filesystem paths over GitHub spec parsing, meaning
vendor/shakapackerwould be treated as a local path if that directory exists, even though it looks like a GitHub repo format. While this appears intentional (based on the past review and help text), it could surprise users.The help text mentions using
./ prefix to force local path if ambiguousbut doesn't clearly explain that existing directories are auto-detected as local paths regardless of their format.Consider adding a comment above Line 89 explaining this precedence:
def process_gem_value(gem_name, value) + # Precedence: existing filesystem paths > explicit ./ or ../ > GitHub specs > simple names # Check if it's a local filesystem path first (before checking for GitHub specs)Also consider adding a log message in verbose mode when an ambiguous path is treated as local:
if File.exist?(expanded_path) || value.start_with?('./', '../') # It's a local path @gem_paths[gem_name] = value + puts " Treating '#{value}' as local path (exists: #{File.exist?(expanded_path)})" if @verbose
105-124: Add validation for empty or malformed specsThe method doesn't validate that
specis non-empty or that the extractedrepois valid. Edge cases like empty strings or specs like@or#alone would result in empty repo names, causing confusing errors downstream.Add validation before returning:
repo = spec ref = nil # Will auto-detect default branch ref_type = :branch end + + raise Error, "Invalid GitHub spec: '#{spec}'" if repo.nil? || repo.empty? [repo, ref, ref_type]
126-154: Validate shorthand refs are non-emptyIf
specis just'#'or'@'without a ref,spec[1..]returns an empty string. This would pass through to@github_reposwith an empty branch/tag, potentially causing confusing errors in downstream validation or git operations.Add validation after extracting the ref in both shorthand branches:
if spec.start_with?('#') # Shorthand: #branch repo = GEM_REPOS[gem_name] raise Error, "No default repo for gem: #{gem_name}" unless repo ref = spec[1..] + raise Error, "Invalid shorthand spec: missing branch after '#'" if ref.empty? ref_type = :branch elsif spec.start_with?('@') # Shorthand: @tag repo = GEM_REPOS[gem_name] raise Error, "No default repo for gem: #{gem_name}" unless repo ref = spec[1..] + raise Error, "Invalid shorthand spec: missing tag after '@'" if ref.empty? ref_type = :tag
345-363: Remove duplicate exampleLines 348-349 and 360-362 contain the same example (mixing local paths and GitHub repos). The second occurrence appears to be redundant.
Remove the duplicate:
bin/swap-deps --shakapacker shakacode/shakapacker - MIXING SOURCES: - --------------- - # Mix local paths and GitHub repos - bin/swap-deps --shakapacker ~/dev/shakapacker \ - --react-on-rails '#feature-x' - # Apply to specific demo only
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/demo_scripts/swap_deps_cli.rb(3 hunks)
🔇 Additional comments (5)
lib/demo_scripts/swap_deps_cli.rb (5)
77-82: LGTM: Clean constant definitionThe default repository mapping is clear and properly frozen.
200-210: LGTM: Consistent option handlingThe gem-specific options correctly delegate to
process_gem_valuefor unified path/spec processing.
215-222: LGTM: Proper handling of --github optionCorrectly uses
parse_github_spec_without_shorthandsince gem-name context isn't available for shorthand resolution. The zsh quoting warning is a helpful addition.
186-198: Excellent documentation improvementsThe help text additions clearly explain the various input formats and provide practical examples. The zsh quoting warnings are particularly helpful for preventing common user errors.
Minor suggestion: Consider clarifying in Line 308 that the "/" check happens after filesystem path checks:
- 3. GitHub specs (contains #, @, or /) + 3. GitHub specs (starts with #, @, or contains /)This makes it clearer that all three are treated as GitHub specs if not already caught by earlier checks.
Also applies to: 305-313
139-139: No action needed: endless range syntaxspec[1..]is compatible with the project’s Ruby ≥ 3.0 requirement.
- Support shorthand syntax for gem-specific flags: - `--shakapacker '#main'` expands to shakacode/shakapacker#main - `--shakapacker '@v9.0.0'` expands to shakacode/[email protected] - `--shakapacker 'shakacode/shakapacker#branch'` for full spec - Auto-detect default branch when none specified: - `--shakapacker shakacode/shakapacker` detects main/master - Uses `git ls-remote --symref` for zero-cost detection - Add zsh escaping documentation: - Document that # and @ require quotes in zsh - Update help text with examples - Map gem names to default repos: - shakapacker -> shakacode/shakapacker - react_on_rails -> shakacode/react_on_rails - cypress-on-rails -> shakacode/cypress-on-rails Fixes issue where zsh interprets # as comment marker. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Check for existing filesystem paths before GitHub spec parsing - Detect paths starting with ./ or ../ as local paths - Prevents misclassifying paths with / as GitHub repos - Fixes issue where ~/path/to/gem was treated as GitHub spec Order of checks: 1. File.exist?(expanded_path) or starts with ./ or ../ 2. GitHub specs (starts with #, @ or contains /) 3. Default to local path for simple names 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Security Fixes: - Replace backticks with Open3.capture2 in detect_default_branch - Eliminates command injection risk while maintaining safety - Follows existing codebase pattern (already using Open3) Code Quality Improvements: - Document nil ref behavior in process_gem_value - Refactor --github option to use shared parse_github_spec_without_shorthand - Eliminates code duplication between --github and gem-specific flags - Add detailed comment explaining network call during validation Design Notes: - Auto-detection during validation is intentional for fail-fast behavior - git ls-remote is lightweight and happens once per unique repo - Repo validation occurs before any network calls 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Changes:
- Show git branch for local paths (e.g., "path/to/gem (main)")
- Warn when swapped directory no longer exists
- Consolidate gem/package display (no duplication)
- Support git worktrees (check for .git file or directory)
- Resolve GitHub cache paths for display
Output format:
Dependencies:
✓ gem_name → /path/to/gem (branch_name)
⚠️ gem_name → /missing/path - DIRECTORY NOT FOUND
Benefits:
- Single line per dependency instead of separate Gemfile/package.json sections
- Immediate visibility of missing directories
- Branch awareness helps identify which version is swapped
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
Changes: - Show GitHub repo indicator for dependencies from GitHub - Format: [📦 org/repo] after the path - Makes it clear which deps are from GitHub vs local paths Example outputs: GitHub: ✓ gem → /cache/path (main) [📦 shakacode/shakapacker] Local: ✓ gem → /local/path (feature-branch) Benefits: - Immediately distinguishes GitHub repos from local paths - Shows original GitHub repo even when using cache - Helps understand dependency sources at a glance 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Changes:
- Add clear "Path Detection Order" section explaining precedence
- Separate LOCAL PATHS and GITHUB REPOS examples
- Document ambiguous path handling (use ./ prefix if needed)
- Show format differences upfront in gem options section
Help now shows:
Path Detection Order:
1. Existing filesystem paths (checked with File.exist?)
2. Paths starting with ./ or ../
3. GitHub specs (contains #, @, or /)
4. Simple names default to local paths
Examples organized by type:
- LOCAL PATHS: absolute, ~/, ./ examples
- GITHUB REPOS: #branch, @tag, user/repo examples
- MIXING SOURCES: combined usage
Ambiguous case handling:
- ./shakacode/fork (local directory)
- shakacode/fork (GitHub repo)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
justin808
force-pushed
the
swap-deps-github-shorthand
branch
from
e1f3921 to
0c2dda9
Compare
Pull Request Review - PR 56Overall AssessmentThis is a well-crafted enhancement that adds significant developer experience improvements to the swap-deps tool. APPROVED with minor recommendations. Code QualityStrengths:
Suggestions:
SecurityStrong posture: Excellent use of array-form commands preventing injection, thorough input validation, path traversal protection. Minor concern: detect_default_branch makes network calls during initialization (line 687-702). Consider adding timeout handling. Performance
Potential Bugs
Test CoverageSubstantial existing tests (gem_swapper_spec.rb 41KB, github_spec_parser_spec.rb 9KB) Missing coverage for:
Recommendations Priority
Overall: Strong work! Production-ready with high-priority recommendations addressed. Review by Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/demo_scripts/gem_swapper.rb (1)
680-702: Command injection warning is a false positive; consider adding a comment for clarity.The static analysis tool flags line 692 as a potential command injection risk. However, this is a false positive because:
- The
repoparameter is validated byvalidate_github_repobefore this method is called (line 668)- The validation uses strict regex patterns (from GitHubSpecParser module) that only allow safe characters
- Open3.capture2 uses array form for command arguments, preventing shell injection
The code is secure, but the comment on lines 691-692 could be enhanced to document why this is safe.
Consider enhancing the comment to clarify security:
- # Use git ls-remote to find the default branch without cloning - # Use array form to avoid shell injection (repo is already validated by validate_github_repo) + # Use git ls-remote to find the default branch without cloning + # Security: repo is validated by validate_github_repo regex before calling this method, + # and Open3.capture2 uses array form to prevent shell injection output, status = Open3.capture2('git', 'ls-remote', '--symref', "https://github.com/#{repo}.git", 'HEAD')
📜 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(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/demo_scripts/gem_swapper.rb (1)
lib/demo_scripts/github_spec_parser.rb (1)
validate_github_branch(53-74)
🪛 Brakeman (7.1.0)
lib/demo_scripts/gem_swapper.rb
[medium] 692-692: Possible command injection
Type: Command Injection
Confidence: Medium
More info: https://brakemanscanner.org/docs/warning_types/command_injection/
(Command Injection)
🔇 Additional comments (13)
lib/demo_scripts/swap_deps_cli.rb (8)
77-82: LGTM! Clean default repository mapping.The
GEM_REPOSconstant provides a sensible default mapping for shorthand syntax. The mapping is clear and covers the three supported gems.
105-124: LGTM! Parsing logic correctly handles full GitHub specs.The method properly handles three cases:
- Full repo with tag (user/repo@tag)
- Full repo with branch (user/repo#branch)
- Just repo (user/repo, auto-detect branch)
The logic is clear and the nil ref with :branch type for auto-detection is appropriate.
126-154: LGTM! Shorthand expansion logic is correct.The method correctly handles:
- Shorthand branch (#branch) → expands using GEM_REPOS
- Shorthand tag (@tag) → expands using GEM_REPOS
- Full specs → delegates to parse_github_spec_without_shorthand
Error handling for missing default repos is appropriate.
186-198: Help text clearly explains path detection order.The documentation accurately describes the precedence:
- Existing filesystem paths
- Paths starting with ./ or ../
- GitHub specs
- Simple names default to local paths
The note about using ./ prefix to disambiguate is helpful.
200-210: LGTM! Option handlers correctly use process_gem_value.All three gem-specific options now route through
process_gem_value, which provides consistent handling of local paths and GitHub specs.
215-222: LGTM! --github flag properly handles full specs.The handler correctly:
- Parses using parse_github_spec_without_shorthand (no shorthand expansion)
- Infers gem name from repo
- Documents the need for quotes in zsh
306-364: LGTM! Comprehensive examples covering all use cases.The help text provides clear examples for:
- Local paths (absolute, home-relative, relative)
- GitHub shorthand (with zsh quoting notes)
- Full GitHub specs
- Auto-detection
- Mixing sources
The examples are practical and well-organized.
86-103: Nilbranchvalues are auto-detected and validated ingem_swapper.rb, so downstream handling is correct.lib/demo_scripts/gem_swapper.rb (5)
251-262: LGTM! Simplified status display logic.The refactoring correctly consolidates the display logic:
- Single call to
display_swapped_dependencieswhen gems are present- Simplified early return condition (only checks swapped_gems.empty?)
- Clearer messaging
315-320: LGTM! Clean unified display method.The new method provides a single entry point for displaying dependencies, delegating to
display_dependency_infofor each gem.
322-362: LGTM! Comprehensive dependency info display.The method handles both GitHub and local dependencies well:
- Resolves GitHub specs to cache paths
- Cleans up npm package paths
- Detects git branch for local paths
- Shows clear status indicators
The logic is thorough and the output will be user-friendly.
353-362: LGTM! Safe git branch detection.The method correctly:
- Checks for .git directory existence
- Uses git in read-only mode
- Returns nil on failure
- Handles exceptions gracefully
653-671: LGTM! Correctly handles nil branches and triggers auto-detection.The changes allow branch to remain nil initially (lines 653, 660), then populate it via
detect_default_branch(line 671). This enables the auto-detection feature mentioned in the PR objectives.
| @@ -1,5 +1,6 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| require 'English' | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
❓ Verification inconclusive
Verify that English is actually used in the codebase.
The require 'English' statement was added, but I don't see obvious usage of English module features (like $CHILD_STATUS instead of $?) in the visible changes.
Run the following script to check if English module features are used:
🏁 Script executed:
#!/bin/bash
# Description: Search for English module usage in gem_swapper.rb
# Look for common English module variables
rg -nP '\$CHILD_STATUS|\$INPUT_RECORD_SEPARATOR|\$OUTPUT_RECORD_SEPARATOR|\$PROCESS_ID|\$PROGRAM_NAME' lib/demo_scripts/gem_swapper.rb
# If not found, this require might be unnecessaryLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for common English module variables in lib/demo_scripts/gem_swapper.rb
rg -nP '\$(CHILD_STATUS|ERROR_INFO|ERROR_POSITION|LAST_MATCH_INFO|MATCH|PREMATCH|POSTMATCH|INPUT_RECORD_SEPARATOR|OUTPUT_RECORD_SEPARATOR|FIELD_SEPARATOR|FS|OFS|ORS|RS|PID|PROCESS_ID|PROGRAM_NAME)' lib/demo_scripts/gem_swapper.rbLength of output: 0
Remove unused require 'English'
No English module variables are referenced in lib/demo_scripts/gem_swapper.rb (line 3), so delete that require.
🤖 Prompt for AI Agents
In lib/demo_scripts/gem_swapper.rb around line 3, the file includes require
'English' but no English module variables are used; remove the require 'English'
line from the top of the file and run the test/lint suite to ensure nothing else
depends on it.
Code Review: PR #56 - Add GitHub shorthand syntax and auto-detection to swap-depsOverviewThis PR adds convenient shorthand syntax for GitHub specifications and automatic default branch detection to the swap-deps CLI tool. Overall, this is a well-implemented feature with good attention to security and usability. StrengthsCode Quality
Documentation
User Experience
|
Issues Found1. Missing Test Coverage for New Functionality (Priority: High)The new shorthand syntax and auto-detection features lack test coverage:
2. Disabled RuboCop Rule Without Justification (Priority: Medium)Line 85 in lib/demo_scripts/swap_deps_cli.rb has a disabled Lint/DuplicateBranch rule. The three branches in process_gem_value have identical code. Either refactor to eliminate duplication or add a comment explaining why the duplication is necessary. 3. Potential Edge Case: Empty String Handling (Priority: Low)In parse_github_spec, if a user provides --shakapacker '#' or --shakapacker '@', the code would set ref to an empty string. Add validation for empty refs. 4. Network Call During Initialization (Priority: Low)The detect_default_branch method makes network calls during object construction. Consider adding a timeout or retry mechanism for robustness. 5. Inconsistent Nil Handling (Priority: Low)In validate_github_repos, the branch is set to nil when not specified. Ensure all code paths handle nil branches correctly, especially in swap_gem_to_github and GitHub caching logic. |
Performance Considerations
Security AssessmentNo security concerns identified:
Suggestions for Follow-up
SummaryThis is a solid feature addition that improves developer experience significantly. The code quality is good, security practices are excellent, and the documentation is thorough. The main gap is test coverage for the new functionality. Once tests are added, this PR will be ready to merge. Recommendation: Approve with minor changes (add tests before merging) Generated with Claude Code |
Summary
--shakapacker '#main')Changes
Shorthand Syntax
Gem-specific flags now accept GitHub specs in addition to local paths:
--shakapacker '#main'→ shakacode/shakapacker#main--shakapacker '@v9.0.0'→ shakacode/[email protected]--shakapacker 'user/repo#branch'→ custom repo/branchAuto-Detection
When no branch/tag specified, automatically detects the default branch:
--shakapacker shakacode/shakapacker→ detects main/mastergit ls-remote --symreffor zero-cost detectionShell Escaping
#and@require quotes in zsh#as comment markerDefault Repo Mapping
Examples
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit