JOB-148890 Fix dependency duplication#30
Conversation
timoteialbu
changed the title
timoteialbu
force-pushed
the
fix/workspace-filter-object-aliasing-bug
branch
from
8fee61a to
717dd0b
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug where the workspace package filtering introduced in PR #29 was ineffective due to Ruby object aliasing. The parse_libyear() method returns the same hash object it receives as all_libraries, making it identical to parsed_results. When add_dependabot_findings() injects cross-workspace packages into parsed_results, they simultaneously appear in the comparison hash, causing the filter to compare against itself and become a no-op.
Changes:
- Snapshot package names into an immutable Set before Dependabot mutations to break the object aliasing chain
- Update the filter method to compare against the pre-mutation Set instead of the aliased hash
- Add regression test that reproduces the exact production scenario with shared object mutation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
lib/library_version_analysis/pnpm.rb |
Captures parsed_results.keys.to_set snapshot after parse_libyear but before add_dependabot_findings in both workspace analysis methods; updates filter signature to accept Set instead of hash |
spec/pnpm_spec.rb |
Updates 5 existing filter tests to use Set literals; adds regression test simulating production aliasing scenario with shared hash mutation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| puts("\tPNPM [#{source}] parsing libyear") if LibraryVersionAnalysis.dev_output? | ||
| parsed_results, meta_data = parse_libyear(libyear_results, all_libraries) | ||
| # Snapshot before Dependabot: parse_libyear returns all_libraries as parsed_results (same object), |
There was a problem hiding this comment.
Missing require 'set' statement at the top of this file. In Ruby 3.0+, the Set class is not automatically loaded and must be explicitly required. Since this codebase uses Ruby 3.3.6 (as specified in .tool-versions) and the code now uses .to_set, you need to add require 'set' at the top of the file (around line 1-3) alongside the other require statements.
| "react" => LibraryVersionAnalysis::Versionline.new(owner: ":unknown", current_version: "18.2.0"), | ||
| "lodash" => LibraryVersionAnalysis::Versionline.new(owner: ":unknown", current_version: "4.17.21") | ||
| } | ||
| workspace_package_names = Set["react", "lodash"] |
There was a problem hiding this comment.
Missing require 'set' statement. In Ruby 3.0+, the Set class is not automatically loaded and must be explicitly required. Since this codebase uses Ruby 3.3.6 and the tests now use Set["react", "lodash"], you need to add require 'set' at the top of this spec file.
There was a problem hiding this comment.
Why do I need this if the tests pass? Presumably they would fail if this was really needed.
There was a problem hiding this comment.
I was thinking the same thing
timoteialbu
force-pushed
the
fix/workspace-filter-object-aliasing-bug
branch
from
717dd0b to
3ad46f9
Compare
…tive dep inflation Three bugs fixed: 1. parse_libyear() returns the same hash object it receives as all_libraries, so parsed_results and all_libraries are the same reference. When add_dependabot_findings injects cross-workspace packages into parsed_results, they simultaneously appear in all_libraries. The filter then compares the hash against itself and finds nothing to remove. Fix by snapshotting known package names into an immutable Set before Dependabot can mutate the hash. 2. pnpm list --json puts devDependencies under a separate "devDependencies" key, not under "dependencies". Workspace packages that only have devDependencies (like library packages with peerDependencies + devDependencies) produced an empty dependency graph, so transitive ownership propagation never ran and all transitive deps were uploaded as owner:unknown. Fix by processing both "dependencies" and "devDependencies" from the JSON output. 3. add_all_libraries used --depth=Infinity, pulling the entire transitive dependency tree into parsed_results as top-level library records. This caused hundreds of transitive deps to be uploaded as individual libraries (e.g. 588 for packages/visualizations which only has 23 direct deps). These transitive deps should only exist in the dependency graph for ownership propagation, not as standalone library records. Fix by using --depth=0 to only collect direct dependencies. Co-authored-by: Cursor <cursoragent@cursor.com>
timoteialbu
force-pushed
the
fix/workspace-filter-object-aliasing-bug
branch
from
3ad46f9 to
dd44e30
Compare
timoteialbu
changed the title
| "react" => LibraryVersionAnalysis::Versionline.new(owner: ":unknown", current_version: "18.2.0"), | ||
| "lodash" => LibraryVersionAnalysis::Versionline.new(owner: ":unknown", current_version: "4.17.21") | ||
| } | ||
| workspace_package_names = Set["react", "lodash"] |
There was a problem hiding this comment.
I was thinking the same thing
2 participants
Summary
Three bugs fixed:
Workspace filter was a no-op due to object aliasing:
parse_libyear()returns the same hash object it receives asall_libraries, soparsed_resultsandall_librariesare the same reference. Whenadd_dependabot_findingsinjects cross-workspace packages intoparsed_results, they simultaneously appear inall_libraries, making the filter compare the hash against itself. Fix: snapshot known package names into an immutableSetbefore Dependabot mutates the hash.Dependency graph empty for devDependency-only workspaces:
pnpm list --jsonputs devDependencies under a separate"devDependencies"key, not under"dependencies". Workspace packages that only have devDependencies (like library packages with peerDependencies + devDependencies, e.g.packages/visualizations) produced an empty dependency graph, so transitive ownership propagation never ran and all transitive deps were uploaded asowner: unknown. Fix: process both"dependencies"and"devDependencies"from the JSON output.Transitive dependency inflation:
add_all_librariesused--depth=Infinity, pulling the entire transitive dependency tree intoparsed_resultsas top-level library records. This caused hundreds of transitive deps to be uploaded as individual libraries (e.g. 588 forpackages/visualizationswhich only has 23 direct deps). Transitive deps should only exist in the dependency graph for ownership propagation, not as standalone library records. Fix: use--depth=0to only collect direct dependencies.Changes
lib/library_version_analysis/pnpm.rbparsed_results.keys.to_setbefore Dependabot; updatefilter_to_workspace_packagesto check against theSet; processpackage["devDependencies"]inadd_dependency_graph; changeadd_all_librariesfrom--depth=Infinityto--depth=0spec/pnpm_spec.rbSet; add 1 regression test for aliasing bug; add 2 tests for devDependencies graph buildingTest plan
packages/visualizationsuploads ~23 libraries (direct deps) instead of ~588