fix(lockfile): drop the redundant npm-alias target left by the pnpm reader - #609
Conversation
…eader Reading a pnpm-lock.yaml with an `npm:` alias synthesized an alias-keyed package AND kept the canonical real-name-keyed entry it was cloned from. A fresh resolve produces only the alias-keyed one, so the two graphs disagreed and every consumer that walks `packages` raw disagreed with them: dedupe reported a phantom removal on every run and `--check` exited 1 forever even though the lockfile was byte-identical; fetch over-counted and re-imported the tarball; `rebuild <real-name>` matched a package that is not an installed dependency. install and the linker were unaffected because they run filter_graph's reachability GC first. The reader now drops each alias-clone source that no importer and no surviving package edge reaches. It cannot drop unconditionally: an aliased package is frequently also a genuine dependency of something else, as in @isaacs/cliui, which reaches string-width@4.2.3 both through its string-width-cjs alias and through wrap-ansi's own edge. Dropping it is safe for serialization because the pnpm writer regenerates the canonical packages:/snapshots: key from the alias clone via alias_of. A `file:` target is excluded, since the writer keys locals by name rather than alias_of and could not regenerate one from the clone. Three related `pkg.name` vs `registry_name()` misses are fixed alongside, each of which made a command blind to npm-aliased packages: - ignored-builds/approve-builds looked the store index up by the alias, so an aliased package's unreviewed install script silently dropped out of the report and could never be approved. This is a prerequisite for the reader change: the phantom entry was what kept the pnpm-lock path reporting correctly, so removing it without this fix would have turned pnpm-lock mode into the same silent miss every other format had. approve-builds reports and writes allowBuilds under the real name but hands the follow-up rebuild its graph names, which stay the alias -- a second divergence class alongside the source-backed one already threaded there, and plural because one build can back several graph nodes. - audit queried advisories by the alias, which the endpoint has never heard of, so an aliased package came back clean no matter how many advisories it had. The response readback already keyed on registry_name(), so the alias spelling could never have matched. - outdated fetched the packument by the alias, warned that it could not be found, and then reported the dependency as up to date forever. Rows still display the alias the user wrote in package.json.
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No correctness issues found in the sweep or the companion fixes — two test-coverage rough edges worth a look.
Reviewed changes — the pnpm reader's npm:-alias handling, which left both the canonical real-name entry and the synthesized alias clone in the parsed graph while a fresh resolve emits only the clone, plus the four commands that were independently blind to aliased packages.
- Drop the unreferenced alias-clone source in the pnpm reader —
read.rsrecords the canonical key each clone came from (gated onlocal_source.is_none(), so afile:target survives) and sweeps the ones no importerDirectDepand no surviving package edge reaches, resolving edges throughresolve_dep_edgeexactly asfilter_graphdoes. - Re-key the audit advisory query on
registry_name()— required by the sweep, not optional: the canonical entry was accidentally supplying the real name to the bulk endpoint, andresolved_versions_by_namealready read the response back byregistry_name(). - Identify ignored-builds entries by
registry_name()—name,approval_key, and the store read move together sois_source_backed()'sapproval_key != nametest stays intact, andseenbecomes aBTreeMap<_, Option<usize>>so a collapsed second graph node still contributes to the newIgnoredEntry.graph_names. - Thread graph names through the approve-builds rebuild handoff —
run_projectnow returns the spellingrebuildmatches on (p.name) rather than the registry name it was approved under. - Key outdated's packument fetches on the registry name — deduped by registry name, row lookup switched from
removetogetsince several deps can share one, warnings deduped; rows still display the alias the user wrote. - Update the lockfile tests to assert the drop — three existing alias tests flip to asserting absence, and two new tests pin the counterparts that must survive (an alias target shared with
wrap-ansi, and afile:target the writer cannot regenerate).
The sweep holds up under the checks that matter. A single pass is sufficient because the clone is real_pkg.clone() and so carries the canonical's dep edges verbatim, keeping the dropped entry's children reachable. The pnpm writer regenerates the canonical packages: / snapshots: key and the importer version: field from alias_of, so the byte-identical round-trip survives — workspace_lockfile_round_trips_byte_identical_to_native_pnpm and the other 149 pnpm:: tests pass locally. Peer-context suffixes embedded in sibling keys are not a reference channel anywhere in the crate, and filter_graph already ignores them, so the sweep is no less conservative than the GC install has always run. Running before the git/tarball re-keying pass is safe because every local_source-bearing target is excluded from the recorded set.
ℹ️ The symptom the issue reports has no regression test
The parser invariant is well covered by the new and updated unit tests, but nothing pins the user-visible contract that broke. The reporter's failure was nub dedupe --check exiting 1 forever on a byte-identical lockfile, and vendor/aube/test/dedupe.bats still has no npm: alias case — its closest test uses a plain dependency. A future change to the sweep's retain predicate, to filter_graph, or to diff_graphs could reopen #578 with every unit test still green.
The alias-merge accumulation in collect_ignored is in the same position: the BTreeMap<_, Option<usize>> bookkeeping that folds two graph nodes into one entry and grows graph_names is reached only by an aliased or duplicated build, and no test constructs that graph.
Technical details
# The symptom the issue reports has no regression test
## Affected sites
- `vendor/aube/test/dedupe.bats` — has `aube dedupe --check exits 0 when lockfile is already deduped` and `aube dedupe removes orphan entries from the lockfile`, but no `npm:` alias fixture. The #578 repro (`"number-alias": "npm:is-number@7.0.0"` → install → `dedupe --check`) is not exercised at the command level anywhere.
- `vendor/aube/crates/aube/src/commands/ignored_builds.rs:211-287` — the `seen` merge path (`graph_names.push` for a second graph node behind the same build) has no test. The file has no `#[cfg(test)] mod tests` at all, and the new unit test in `approve_builds.rs` constructs an `IgnoredEntry` by hand rather than driving `collect_ignored`.
## Required outcome
- A test that fails if the phantom removal returns: install (or hand-write) a pnpm v9 lockfile with an `npm:` alias, then assert `dedupe --check` exits 0 and the lockfile hash is unchanged across a `dedupe` run.
- A test that exercises `collect_ignored`'s alias-merge on a graph where an alias and its real package (or two aliases of one package) both declare a lifecycle hook, asserting one entry with both graph names.
## Suggested approach
The `dedupe.bats` case is the cheaper and higher-value of the two — the existing `aube dedupe --check exits 0 when lockfile is already deduped` test is the template, and the alias fixture is a two-line `package.json` change. For `collect_ignored`, the blocker is that it reads a real lockfile and store; if factoring out a pure `(graph, policy) -> Vec<IgnoredEntry>` seam is more churn than it is worth, a bats case over `ignored-builds` on an aliased build-script package covers the same behavior end to end.Claude Opus ｜ 𝕏
The parser invariant was covered but the user-visible contract from #578 was not, so a change to the reader's sweep or to diff_graphs could reopen it with every unit test green. diff_graphs gains three cases: an aliased dep reads as no change across a re-resolve, a lingering canonical entry WOULD report a phantom removal (pinning the shape the reader must never hand back), and a shared alias target is absent from the diff. These are pure graph fixtures, so they run in the ordinary aube cargo test job rather than needing a registry. pm_verbs gains the reporter's end-to-end repro -- install an npm: alias under pnpm-lock.yaml, then assert dedupe leaves the lockfile byte-identical and --check exits 0. It resolves from the registry, so it follows the file's existing #[ignore = "network"] convention. The approve-builds handoff assertion re-implemented the expression it was meant to pin, so it could not have failed. The handoff is now a named rebuild_handoff_names called by both run_project and the test.
There was a problem hiding this comment.
ℹ️ The fix itself is unchanged and still holds up — two rough edges in the new test coverage.
Reviewed changes — e28508a, which is test-only: it answers both coverage gaps the previous review raised, adding a command-level npm:-alias dedupe test and a diff_graphs unit-test module, and extracting the approve-builds rebuild handoff so its test drives the production expression.
- Add a command-level
npm:-alias dedupe regression test —dedupe_ignores_npm_alias_targets_and_check_passesinstalls anumber-alias: npm:is-number@7.0.0fixture, then assertsdedupereports "already deduped", leaves the lockfile byte-identical, and--checkexits 0. Mirrors the adjacent workspace-link test exactly. - Extract
rebuild_handoff_names— the graph-name handoff is now a named function over&[&IgnoredEntry]called from bothrun_projectand its unit test, so the test no longer re-implements the expression it pins. - Add a
diff_graphsunit-test module indedupe.rs— three tests over hand-builtLockfileGraphs covering the deduped shape, the pre-fix lingering-canonical shape, and a shared alias target.
I traced the new integration test against the pre-fix reader and it genuinely reproduces the reported failure, so the guard is real where it runs. The fixture's packageManager: pnpm@11.17.0 selects pnpm-lock.yaml through detect.rs; the native_pnpm_aliases gate at write.rs:68-71 makes the writer emit only the canonical is-number@7.0.0; the pre-fix reader hands back both keys, so diff_graphs returns removed = ["is-number@7.0.0"] and all three of the test's assertions fail. The extraction also matches the previous review's suggested shape, and that thread is resolved.
Claude Opus ｜ 𝕏
Two of the three diff_graphs cases compared a graph against an identical copy of itself. diff_graphs never reads alias_of -- it filters on local_source and compares keys -- so both fixtures' alias fields were inert and the assertions reduced to reflexivity of a set difference. They passed whether or not the reader's sweep existed. Both now parse a real pnpm-lock.yaml fixture and diff the reader's own output against the shape a fresh resolve emits, which is the only way the sweep can be observed from this layer. Mutation-checked in both directions: disabling the sweep fails the orphan case, and making it unconditional fails the shared-target case. a_lingering_alias_target_would_report_a_phantom_removal stays a pure graph fixture on purpose -- it pins that #578 was fixed in the reader rather than papered over inside this filter, which is a constraint on a future change rather than an observation about the current one.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — bd04143, which is test-only: it rewires two of the three diff_graphs unit tests to drive the real pnpm reader instead of hand-built graphs, closing both coverage gaps the previous review raised.
- Parse the fixtures through the reader — a new
parse_pnpmhelper writes an inline pnpm v9 lockfile to a temp dir and callsaube_lockfile::pnpm::parse, soparsedis the reader's own output rather than a literal whosealias_offielddiff_graphsnever consults. - Rename both tests to the behavior they now pin —
an_orphan_alias_target_is_swept_so_dedupe_sees_no_changecovers the drop, andan_alias_target_a_second_consumer_needs_survives_the_sweepcovers the counterpart on the real@isaacs/cliuishape, wherewrap-ansi@7.0.0's own edge keepsstring-width@4.2.3live. - Print the parsed key set on failure — both assertions now carry the parsed keys in the message, so a CI failure names the offending entries without a local rerun.
- Keep the hand-built lingering-canonical test —
a_lingering_alias_target_would_report_a_phantom_removalstays a literal graph on purpose, pinning that the fix lives in the reader rather than insidediff_graphs' filter.
I confirmed each rewritten test fails on its own regression direction by mutating read.rs locally: short-circuiting the sweep makes the orphan test report removed = ["is-number@7.0.0"], and dropping the referenced.contains(key) term from the retain predicate makes the shared-target test report added = ["string-width@4.2.3"]. Both run in automation — vendor/aube is excluded from nub's cargo workspace so ci.yml's cargo test never sees them, but aube-parity.yml runs cargo test --workspace with working-directory: vendor/aube behind a vendor/aube/** path filter this PR matches. Green locally on bd04143: 3 dedupe tests, 150 pnpm:: tests, and cargo clippy -p aube --all-targets with no warnings.
Claude Opus ｜ 𝕏
Picks up #612, #609 and #613, which landed while this bump was in flight and which the PR body flagged as overlapping. One conflict, in commands/outdated.rs: #609 added a HashSet import and still called max_satisfying_version, which aube v1.35.0 renames to wanted_version. Unions the import and keeps the new name. The argument stays unreferenced — packument binds to &Packument out of the Some(Ok(p)) arm.
|
Shipped in v0.7.0: https://github.com/nubjs/nub/releases/tag/v0.7.0 |
Closes #578
The pnpm reader kept the canonical real-name entry alongside the alias-keyed clone it synthesizes for an
npm:alias. A fresh resolve emits only the clone, so consumers walkingpackagesraw disagreed with a re-resolved graph:dedupereported a phantom removal every run (--checkred forever on a byte-identical lockfile),fetchover-counted,rebuild <real-name>matched a non-installed dep. Usually hit transitively, viaglob@10reaching@isaacs/cliui.The reader now drops an alias-clone source nothing else reaches; shared targets survive,
file:targets are excluded.Also fixed, each blind to aliased packages:
ignored-builds/approve-builds(a prerequisite here),audit,outdated.Conformance green against real pnpm 10.15.1 and npm 11.17.0.