Repository navigation
fix(pm): detect stale lockfile in ut install - #2797
Merged
Merged
Conversation
elrrrrrrr
marked this pull request as draft
April 16, 2026 12:18
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to detect and regenerate stale package-lock.json files during installation, ensuring they remain synchronized with package.json. It includes a new E2E test case to verify this logic. Feedback highlights potential performance overhead due to redundant file parsing and suggests refactoring to allow skipping the check when the lockfile is known to be fresh.
`fs::try_exists(&lock_path)` check in Self::install. As a result any `ut install` with an existing package-lock.json skipped the manifest resolution entirely — even when package.json had been edited — silently installing the stale tree from the lockfile. Re-introduce the freshness check: when the lockfile exists we now still run `is_pkg_lock_outdated`. Stale lockfiles (or freshness-check errors — corrupt lockfiles etc.) fall through to the pipeline cold path so we keep the concurrent resolve + download benefit #2576 was built for. Regression guard lives in `e2e/pm/stale-lockfile/` — a fixture that commits a package-lock.json whose root `dependencies` is empty while package.json declares abbrev and ini. Case 8.2 in e2e/utoo-pm.sh asserts: - `ut install` regenerates the lockfile - both packages end up in node_modules/ Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Drop duplicate `tracing::warn` in Self::install; `is_pkg_lock_outdated`
already logs the specific mismatch reason (root deps changed, engines
changed, …). Users now see one warn instead of two per stale detection.
- Collapse `lock_exists` + `lock_stale` into a single `use_fresh_lock`
expression — removes the double-negation.
- Drop the committed stale `package-lock.json` fixture and the
`git checkout --` dance in Case 8.2. The e2e now writes the stale
lockfile inline via heredoc, which:
* survives local edits / worktrees / stashed state
* avoids the `git add -f` fight with .gitignore
* keeps the fixture dir to just package.json (the declarative half)
- Assert root deps via `node` inspection of `packages[""].dependencies`
instead of `grep "abbrev"`, which would match transitive
`node_modules/abbrev` entries and hide a regression where only the
tree — not the root declaration — got rewritten.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The freshness check was reading the root package.json three times per `ut install` — once as DepsView, once inside find_workspaces (for the "workspaces" field), once as EnginesView. Each workspace was also read twice (once by find_workspaces, again as DepsView). Load the root as full `PackageJson` via `get_or_load_package_json` so subsequent reads on the same path hit the in-process cache (filled by `update_package_json`'s write-through). Within this function, drop the DepsView/EnginesView intermediates and read the fields directly off the same `PackageJson`; workspaces come back pre-loaded on the `find_workspaces` tuple so we don't re-load them either. Cache coherence: `update_package_json` already calls `set_package_json` write-through after every edit, and every `ut install` starts a fresh process with a cold cache — so external edits (IDE, `git checkout`) still see a disk read next run. Tests: the original `test_is_pkg_lock_outdated` simulated a "write file, re-check" flow that doesn't happen in real invocations (every mutation goes via `update_package_json` + write-through). Split it into five independent scenarios, each with its own TempDir so cache keys don't collide, which is also the idiomatic "one test per case". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
elrrrrrrr
force-pushed
the
fix/pm-install-stale-lockfile
branch
from
April 16, 2026 14:10
2488786 to
33ca6e6
Compare
elrrrrrrr
marked this pull request as ready for review
April 16, 2026 14:18
fireairforce
approved these changes
Apr 16, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
ut installwas silently installing stalenode_moduleswheneverpackage.jsondrifted frompackage-lock.json. Regression from refactor(pm): pipeline deps resolver #2576 — the oldensure_package_lockcall (which ranis_pkg_lock_outdated) got replaced with a bare file-exists check inSelf::install, so the freshness check never fired on the plain install path.resolve_with_pipeline(concurrent resolve + download), not the serialensure_package_lockpath.Repro the bug (before this fix)
ut installfinishes silently with an emptynode_modules. User thinks all is well.Test plan
cargo clippy -p utoo-pm --all-targets -- -D warnings --no-depscargo test -p utoo-pm(helper::lock suite green)package-lock.json is outdated, regenerating, installs both packages, and rewrites the lockfile.Case 8.2: stale lockfile detectionine2e/utoo-pm.sh; the trackede2e/pm/stale-lockfile/package-lock.jsonis intentionally stale so the test is self-contained🤖 Generated with Claude Code