Fix large lockfile fallback and align scope docs#2
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the tool’s JS local-fallback accuracy and robustness by (1) correctly fetching large lockfiles from GitHub when the Contents API returns encoding: none, and (2) treating local/workspace/file dependencies as workspace-local so they don’t get attributed as registry/transitive packages during fallback analysis. It also updates multiple docs to better reflect the shipped scope.
Changes:
- Add GitHub Contents API
encoding: nonehandling by falling back to fetching the corresponding blob by SHA. - Mark npm + Yarn local file/workspace references as
WorkspaceLocaland skip traversal/registry targeting for those packages. - Align README/release/smoke-test and other docs with the current supported scope.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/npm/lockfile.go | Marks parsed npm/Yarn packages as WorkspaceLocal when they use local/workspace-style references. |
| internal/npm/jsgraph.go | Skips dependency traversal into WorkspaceLocal packages when collecting target package views. |
| internal/npm/lockfile_test.go | Expands workspace and Yarn tests to ensure local/workspace deps are marked and not attributed as external transitive. |
| internal/github/client.go | Adds sha parsing from the Contents API response and blob-backed decoding fallback for encoding: none. |
| internal/github/client_test.go | Adds unit tests covering blob fallback and unsupported-encoding behavior. |
| internal/analysis/types_test.go | Adds coverage ensuring local/workspace packages are excluded from registry lookup targets. |
| docs/smoke-test.md | Updates smoke-test checklist to include large-lockfile blob fallback validation. |
| RELEASING.md | Updates release verification steps and encourages live PR validation. |
| README.md | Clarifies scope wording and notes blob-backed fetching for large lockfiles. |
| CONTRIBUTING.md | Updates contributor-facing scope wording and adds Yarn Classic fallback support note. |
| CHANGELOG.md | Documents the bugfixes and scope wording changes in Unreleased. |
| AGENTS.md | Updates agent guidance to include Yarn Classic fallback support. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| result.All[pkg.Path] = pkg | ||
| if pkg.WorkspaceLocal { | ||
| continue | ||
| } |
There was a problem hiding this comment.
The BFS now correctly avoids enqueueing WorkspaceLocal packages, but the later nested-package sweep (that adds any package whose path is under an already-included package) will still pull in .../node_modules/... dependencies under a workspace-local package path. That can re-introduce external packages as transitive even though traversal was intentionally skipped. Consider teaching that sweep to ignore workspace-local parents (e.g., only add nested packages when the parent result.All[existingPath] is not WorkspaceLocal).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 325d099779
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if pkg.WorkspaceLocal { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Skipping queue traversal for WorkspaceLocal packages here is incomplete because the package path is still inserted into result.All, and the later descendant sweep (strings.HasPrefix(pkg.Path, existingPath+"/node_modules/")) will re-include nested dependencies under that local package. In repositories where a workspace/file dependency has non-hoisted children (for example node_modules/local-lib/node_modules/*), those external packages are still counted as target transitives, so risk score and publish-age lookup behavior now depends on hoisting layout instead of consistently excluding local-package trees.
Useful? React with 👍 / 👎.
Summary
Verification
baseline-browser-mappingstorybookjs/storybook#33576