Conversation
Greptile SummaryThis PR fixes npm workspace handling by parsing workspace target path entries (e.g. Confidence Score: 5/5Safe to merge; logic is correct and well-tested with parse, write, and dedup round-trip regression tests. No P0 or P1 issues found. The one P2 comment is a minor performance concern in workspace_package_for_importer. Core logic — link-target importer parsing, redundant-top pruning, cross-importer reachability, and the non_link_roots filter — all check out against the existing parse infrastructure. No files require special attention. Important Files Changed
Reviews (4): Last reviewed commit: "fix(lockfile): skip workspace dups alrea..." | Re-trigger Greptile |
Benchmark changesVersions:
Public ratios: warm installs vs Bun 4x -> 10x; warm installs vs pnpm 5x -> 15x.
be154a5 vs 73a6617 | aube/bun/pnpm | 3 scenarios | 3 runs | 500mbit/50ms | generated by Codex. |
non_link_roots looked up each dep in the canonical map via canonical_key_from_dep_path, but for a workspace link dep the dep_path is `<name>@link:<path>`, which doesn't match the canonical map's `name@version` keys. The lookup always returned None, the filter passed every dep through, and the function was a no-op. Output stayed correct only because build_hoist_tree independently skips unknown canonical keys — a fragile coincidence. Filter on the dep_path tail's `link:` prefix instead, which matches the format directly. Also drop the stale write-doc bullet about workspace importers (this is the change that landed them). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 658b168. Configure here.
…efix
The previous fix checked `dep_path_tail(...).starts_with("link:")`, but
`LocalSource::dep_path()` produces `<name>@link+<hash>` (plus sign, not
colon). Real link dep_paths from the parser never matched. The
write-test masked this by hand-constructing `link:web` literals instead
of calling `LocalSource::Link::dep_path()`.
Look the dep up in `graph.packages` (keyed by the same dep_path the
DirectDep carries) and check `local_source` directly — robust to any
future change in the dep_path string format. Test now generates the
dep_path the same way the parser does.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per-workspace hoist trees were built in isolation and merged via or_insert, so when the root tree already placed `node_modules/<name>` the workspace still emitted `<importer>/node_modules/<name>` for the same canonical version. Node's upward `node_modules` walk from `<importer>/...` resolves the root copy, so the workspace-nested entry is dead weight — and real `npm install` omits it, so emitting it produces a diff on every round-trip. Compute the per-workspace tree, drop top-level segments whose canonical key already lives at `node_modules/<name>` in placed, and drop their entire subtree (Node never walks into a `node_modules/<name>` dir that doesn't exist, so descendants would orphan). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Test plan
Note: the patched aube install/ci path gets mise-versions past the original missing web workspace dependencies /
astrobinary failure. The subsequent web build exposes the existing Astro/Tailwind configuration mismatch, which is separate from the package-lock workspace importer bug.Note
Medium Risk
Touches npm
package-lock.jsonparsing and serialization logic for workspaces, which can affect dependency graph shape and emitted lockfile contents across installs. Changes are localized but may cause lockfile diffs or incorrect hoisting/link records if edge cases are missed.Overview
Preserves npm workspaces as first-class importers when parsing
package-lock.jsonby capturing each workspace target path (e.g.web/,packages/app/) and recording its prod/dev/optional direct deps (including their original specifiers) ingraph.importers.Extends the npm writer to emit workspace package entries and their root
node_modules/<workspace>link: truerecords, and to build per-workspacenode_modulestrees while skipping redundant workspace-nested packages already hoisted at the root. Updates reachability/flag computation to consider roots from all importers, and adds regression tests for workspace importer parse/write and duplicate-hoist avoidance.Reviewed by Cursor Bugbot for commit be154a5. Bugbot is set up for automated code reviews on this repo. Configure here.