🐛 fix(deps): Ruby: local path & recursive resolution of deps #255
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Previously, the Ruby Gemfile.lock resolver failed to identify dependencies sourced from local paths, resulting in "Unknown" licenses. The parser ignored PATH blocks and did not handle the ! suffix on dependency names. - Updates parseGemfileLock to capture local paths from PATH blocks. - Implements fetchLocalLicense to extract licenses from local .gemspec files. - Strips the ! suffix from dependency names in the lockfile. - Adds a regression test for local path dependencies.
- Update GemspecResolver to recursively resolve dependencies by traversing installed gemspecs. - This ensures that transitive dependencies are correctly identified and their licenses checked. - Fixes issue where transitive dependencies (e.g. citrus in toml-merge) were missed.
There was a problem hiding this comment.
Pull request overview
This PR fixes two bugs in Ruby dependency resolution: (1) support for local path dependencies in Gemfile.lock files, and (2) transitive dependency resolution for gemspec files. The implementation adds PATH block parsing to handle local gems, introduces license resolution from local gemspec files, and implements BFS-based recursive dependency traversal.
- Added PATH block parsing in Gemfile.lock parser to capture local gem paths
- Introduced GemspecResolver for direct gemspec dependency resolution with transitive dependency support
- Implemented local and installed gemspec license extraction as fallback options before RubyGems API
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/deps/ruby.go | Adds LocalPath to gemSpec struct, updates parseGemfileLock to handle PATH blocks and "!" suffix, implements GemspecResolver with BFS transitive resolution, adds helper functions for local and installed gemspec license/dependency parsing |
| pkg/deps/resolve.go | Registers the new GemspecResolver in the resolvers list |
| pkg/deps/ruby_test.go | Adds test cases for citrus library (0 runtime deps) and local_dep app (local path gem with MIT license) |
| pkg/deps/gemspec_test.go | New test file for GemspecResolver covering direct dependency detection and transitive dependency resolution via installed gems |
| pkg/deps/testdata/ruby/toml-merge/toml-merge.gemspec | Test fixture gemspec with runtime dependencies (toml-rb, tree_haver, ast-merge, version_gem) and development dependencies |
| pkg/deps/testdata/ruby/local_dep/Gemfile.lock | Test fixture Gemfile.lock with PATH block pointing to local citrus gem |
| pkg/deps/testdata/ruby/local_dep/citrus/citrus.gemspec | Minimal gemspec for citrus with MIT license and development dependencies |
| pkg/deps/testdata/ruby/citrus/Gemfile.lock | Test fixture Gemfile.lock for citrus library with PATH block using "." as remote |
| pkg/deps/testdata/ruby/citrus/citrus.gemspec | Full citrus gemspec with MIT license and development dependencies for testing library project resolution |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…GEM block in parseGemfileLock.
- Fix `Gemfile.lock` parsing: Reset `currentRemotePath` when entering `GEM` blocks to prevent incorrect `LocalPath` inheritance from preceding `PATH` blocks. - Improve Gemspec resolution: Implement recursive dependency resolution for gemspecs by traversing installed gemspecs to find transitive dependencies. - Improve License detection: - Validate gem name within gemspec content in `fetchLocalLicense` instead of relying solely on filename conventions. - Add `parseGemspecInfo` to extract both name and license from gemspecs. - Add regression tests for transitive dependencies (e.g., `citrus` in `toml-merge` case) and fix test data setup.
Contributor
Author
|
@kezhenxu94 |
- pkg/deps/gemspec_test.go: - Fixed goconst errors: Replaced repeated string literals "toml-rb", "citrus", and "MIT" with constants. - Fixed gocritic error: Updated the octal literal 0755 to the modern 0o755 syntax. - pkg/deps/ruby.go: - Fixed funlen error: Refactored parseGemfileLock by extracting the parsing logic into a lockParserState struct and helper methods (processLine, processSpecs, processDeps), significantly reducing the function length. - Fixed revive error: Renamed the name parameter to targetName in fetchLocalLicense to avoid potential shadowing or unused parameter warnings. - Fixed unparam error: Updated fetchInstalledLicense to return only string, as the error return value was always nil. - Fixed nestingReduce error: Inverted the if condition inside the loop in fetchInstalledLicense (and used continue) to reduce nesting. - Updated Call Sites: Adjusted all calls to fetchInstalledLicense to match the new function signature.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- When resolving local gemspecs by scanning the specifications directory, the previous logic relied solely on filename prefix matching. This could lead to false positives where a dependency name is a prefix of another installed gem (e.g., searching for foo matches foo-bar-1.0.gemspec). - This change adds a verification step that parses the candidate gemspec file to ensure the name attribute inside the gemspec exactly matches the requested dependency name. This applies to both findInstalledGemspec and fetchInstalledLicense.
- Replace the simple `!strings.ContainsAny` check with a robust regex pattern `rubyVersionRe` to validate Ruby gem versions. This ensures that only valid version strings (including pre-releases) are treated as specific versions, avoiding potential issues with constraints or invalid version strings.
- The `parseGemspecLicense` function was unused and redundant, as it simply wrapped `parseGemspecInfo`. Removing it reduces code duplication and cleans up the codebase.
- Update the gemspec license parsing logic to support multiple licenses defined in an array (e.g., `s.licenses = ['MIT', 'Apache-2.0']`). - Previously, only the first license was captured. Now, all licenses are extracted and joined with " OR ".
- Add documentation comments to `GemspecResolver` and its methods to explain that it handles `.gemspec` files by extracting runtime dependencies and recursively resolving their transitive dependencies from installed gems.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
kezhenxu94
previously approved these changes
Dec 19, 2025
Member
kezhenxu94
left a comment
kezhenxu94
left a comment
There was a problem hiding this comment.
Looks like some files are missing license header
Member
This is awesome! Less codes to maintain 😄 |
Contributor
Author
|
@kezhenxu94 Added missing license headers in 490aede |
kezhenxu94
approved these changes
Dec 19, 2025
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Check scanner.Err() after the loop in parseGemspecInfo to ensure any parsing errors are propagated instead of being silently ignored.
- When parsing a gemspec file, if multiple licenses are detected, we currently join them with "OR". This change adds a warning log to inform the user about this assumption, as some gems might intend "AND".
- Explicitly reset currentRemotePath when entering a PATH block in Gemfile.lock parser to prevent state leakage from previous blocks.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- This adds a safety check to the BFS implementation for transitive dependency resolution in pkg/deps/ruby.go. - It limits the queue size to 10,000 to prevent excessive resource consumption or infinite loops in pathological cases (e.g. circular dependencies).
…lock parser - The variable currentRemotePath was misleading as it captures local filesystem paths in PATH blocks, not remote URLs. Renaming it to currentLocalPath improves clarity and accuracy.
- Explicitly verify that the 'citrus' local dependency is resolved with the expected 'MIT' license and ensure it is not present in the skipped dependencies list. - This removes ambiguity in the test result and guarantees that local path resolution is functioning correctly.
- Explicitly document that version constraints in gemspec runtime dependencies are currently ignored. - This clarifies that the resolver defaults to the first found installed version of a gem, which may lead to incorrect resolution if multiple versions are installed and the first one does not satisfy the constraint.
- Update a misleading comment in the gemspec resolver to accurately reflect that it checks for locally installed gems first before falling back to the RubyGems API for license information.
- Update the regex used to extract licenses from gemspec files to be more specific. - The new pattern `\.licenses?\s*=\s*(\[[^\]]*\]|['"][^'"]*['"])` ensures that only array brackets or quoted strings are captured, preventing the accidental inclusion of comments or trailing characters that occurred with the previous overly permissive `([^#]*)` pattern.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Move `internal/logger` to `pkg/logger` to make it accessible to other packages. - Update all import paths from `github.com/apache/skywalking-eyes/internal/logger` to `github.com/apache/skywalking-eyes/pkg/logger`. - Replace direct usage of `github.com/sirupsen/logrus` in `pkg/deps/ruby.go` with the internal logger wrapper.
- Removes redundant checks for filename prefixes in `findInstalledGemspec` and `fetchInstalledLicense`. The prefix is already validated earlier in the loop, making the subsequent `strings.TrimPrefix` check always false (i.e., the prefix is always present and removed). - Remove dead code block in `findInstalledGemspec` - Remove dead code block in `fetchInstalledLicense`
- Enhance error message when dependency graph size limit is exceeded to include current size and potential causes. - Fix queue size check to prevent exceeding the limit of 10000 nodes. - Add debug logging for failures in finding installed gemspecs or parsing gemspec dependencies, instead of silently ignoring them. - switch to gem.coop as official gem server
- Implement `getAllGemspecs` to cache gemspec paths, reducing I/O overhead during dependency resolution. - Update `findInstalledGemspec` and `fetchInstalledLicense` to utilize the cached gemspec list instead of repeatedly scanning directories. - Introduce `sync.Mutex` to ensure thread-safe access to the gemspec cache. - Key the cache by `GEM_PATH` environment variable to support changes in the environment during runtime (e.g. in tests).
- Add unit tests for `fetchInstalledLicense` in `pkg/deps/ruby_test.go` covering: - Multiple versions of the same gem - Invalid version strings - Missing gemspec files - Gems with similar names - Add unit tests for path traversal protection in `pkg/deps/ruby_test.go` covering: - Path traversal attempts in `Gemfile.lock` - Fix `fetchInstalledLicense` in `pkg/deps/ruby.go` to return empty string for invalid version strings instead of falling back to any installed version.
Contributor
Author
|
@kezhenxu94 Aside from the question about logging above, this is done. |
- When multiple licenses are specified in a gemspec, we now assume all licenses apply (AND) rather than alternatives (OR). - This is a safer, more conservative approach. Users can override this in the configuration if the intention was OR. - Added tests for multiple licenses (both non-conflicting and potentially conflicting) to verify they are joined with "AND".
kezhenxu94
approved these changes
Dec 20, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bug 1: Ruby
Gemfile.lockresolver fails to detect licenses for local path dependenciesDescription:
The current implementation of the Ruby Gemfile.lock resolver fails to correctly identify and resolve licenses for dependencies defined with a local path source (e.g.,
gem 'citrus', path: '.').Symptoms:
"Unknown"license in the report.Gemfile.lock, these dependencies often appear with a!suffix (e.g.,citrus!) in theDEPENDENCIESsection, which was not being handled.PATHblock inGemfile.lock, which contains the mapping to the local directory, was being ignored.Reproduction:
Create a Ruby project with a
Gemfilethat references a local gem viapath:.Run
license-eyeheader check.The local gem is reported with an
"Unknown"license, even if its.gemspecdeclares a valid license.Solution:
parseGemfileLockto correctly parsePATHblocks inGemfile.lockand capture the local path (mapped fromremote:) for relevant gems.!suffix from dependency names in theDEPENDENCIESsection, which indicates a local source.fetchLocalLicenseto parse the license directly from the local.gemspecfile when a local path is detected, bypassing the RubyGems API lookup.citrusgem structure to verify the fix.Bug 2: GemspecResolver does not resolve transitive dependencies
Description:
When resolving dependencies from a
.gemspecfile,GemspecResolveronly considers direct dependencies declared in the gemspec. It does not recursively resolve dependencies of those dependencies. This leads to incomplete dependency graphs and missing license checks for transitive dependencies.Reproduction:
Athat depends onB.Bdepends onC.license-eyeonA.Cwill be missing from the report.Solution:
findInstalledGemspechelper to locate installed gemspecs inGEM_HOME.parseGemspecDependencieshelper to extract dependencies from a gemspec file.GemspecResolver.Resolveto traverse the dependency graph usingBFS, ensuring all transitive dependencies are discovered and checked.gemspec_test.gocovering thetoml-merge->toml-rb->citruscase.Issue 3: Improve dependency resolution and license detection
Gemfile.lockparsing: ResetcurrentRemotePathwhen enteringGEMblocks to prevent incorrectLocalPathinheritance from precedingPATHblocks.fetchLocalLicenseinstead of relying solely on filename conventions.parseGemspecInfoto extract both name and license from gemspecs.citrusintoml-mergecase) and fix test data setup.