fix: detect changes inside file: dependencies on repeat install (pacquet + pnpm)#12317
Conversation
|
💖 Thanks for opening this pull request! 💖 |
Code Review by Qodo
1. file: refresh integration test missing
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRestores detection of changes inside local ChangesFile-Dependency Detection and Fast-Path Prevention
Sequence Diagram(s)sequenceDiagram
participant Client
participant installDeps
participant checkDepsStatus
participant findLocalFileDep
participant OptimisticRepeatInstall
Client->>installDeps: run pnpm install
installDeps->>checkDepsStatus: checkDepsStatus(..., treatLocalFileDepsAsOutdated: true)
checkDepsStatus->>findLocalFileDep: scan manifests for local file deps
findLocalFileDep-->>checkDepsStatus: found / not found
checkDepsStatus-->>installDeps: return upToDate result
installDeps->>OptimisticRepeatInstall: attempt optimistic fast-path
OptimisticRepeatInstall->>findLocalFileDep: has_local_file_dep across manifests
findLocalFileDep-->>OptimisticRepeatInstall: true / false
OptimisticRepeatInstall-->>installDeps: Decision::Skipped or Decision::UpToDate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoFix repeat install to invalidate optimistic fast path for file: dependencies WalkthroughsDescription• Ensure optimistic repeat-install never short-circuits when a project declares a file: dependency. • Add a caller-scoped flag so install avoids the fast path, while pnpm run behavior stays unchanged. • Port the same guard to pacquet and cover the behavior with new TS and Rust tests. Diagramgraph TD
A["installDeps (optimistic path)"] --> B["checkDepsStatus"] --> C{"treatLocalFileDepsAsOutdated?"}
C -->|"yes + has file:"| D["Report upToDate=false (force real install)"]
C -->|"no / no file:"| E["Mtime-based checks (manifest/lockfile)"]
F["pacquet: check_optimistic_repeat_install"] --> G{"has file: dep?"} --> H["Decision::Skipped (force full install)"]
High-Level AssessmentThe following are alternative approaches to this PR: 1. Fingerprint file: dependency contents
2. Persist file: dep snapshot metadata in lockfile/workspace state
3. Disable optimistic repeat install automatically when file: deps exist
Recommendation: The chosen approach (caller-scoped bail-out via treatLocalFileDepsAsOutdated on the install fast path, plus an unconditional bail in pacquet where the only caller is install) is the best trade-off: it restores v10-correctness for file: deps with minimal surface area and avoids regressing verify-deps-before-run. More precise solutions require durable fingerprinting metadata and substantially higher complexity/risk. File ChangesBug fix (3)
Tests (2)
Documentation (1)
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pacquet/crates/package-manager/src/optimistic_repeat_install.rs (1)
278-295: ⚡ Quick winPrefer the
PackageManifest::dependenciesAPI over raw JSON access.The implementation accesses the raw JSON via
manifest.value().get(*field), but thePackageManifest::dependenciesmethod (shown in context snippets) provides a type-safe iterator over dependency specs. Using it would simplify the code and make it more maintainable.♻️ Proposed refactor
-fn has_local_file_dep(project_manifests: &[(PathBuf, &PackageManifest)]) -> bool { - const FIELDS: [&str; 3] = ["dependencies", "devDependencies", "optionalDependencies"]; - project_manifests.iter().any(|(_, manifest)| { - FIELDS.iter().any(|field| { - manifest.value().get(*field).and_then(|value| value.as_object()).is_some_and(|deps| { - deps.values() - .any(|spec| spec.as_str().is_some_and(|spec| spec.starts_with("file:"))) - }) - }) - }) -} +fn has_local_file_dep(project_manifests: &[(PathBuf, &PackageManifest)]) -> bool { + use DependencyGroup::{Dev, Optional, Prod}; + project_manifests.iter().any(|(_, manifest)| { + manifest + .dependencies([Prod, Dev, Optional]) + .any(|(_, spec)| spec.starts_with("file:")) + }) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pacquet/crates/package-manager/src/optimistic_repeat_install.rs` around lines 278 - 295, The function has_local_file_dep currently inspects manifest JSON via manifest.value().get(...) to find "file:" dependency specs; instead, use the manifest's typed API by calling PackageManifest::dependencies (or the existing dependencies() method on PackageManifest) to iterate dependency entries for "dependencies", "devDependencies", and "optionalDependencies" and check each spec string for starts_with("file:"); replace the raw JSON access in has_local_file_dep with calls to PackageManifest::dependencies (or equivalent methods) for each of the three fields (formerly FIELDS) so you get a type-safe iterator over specs and then any(|spec| spec.starts_with("file:")).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pacquet/crates/package-manager/src/optimistic_repeat_install.rs`:
- Around line 278-295: The function has_local_file_dep currently inspects
manifest JSON via manifest.value().get(...) to find "file:" dependency specs;
instead, use the manifest's typed API by calling PackageManifest::dependencies
(or the existing dependencies() method on PackageManifest) to iterate dependency
entries for "dependencies", "devDependencies", and "optionalDependencies" and
check each spec string for starts_with("file:"); replace the raw JSON access in
has_local_file_dep with calls to PackageManifest::dependencies (or equivalent
methods) for each of the three fields (formerly FIELDS) so you get a type-safe
iterator over specs and then any(|spec| spec.starts_with("file:")).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3601fca4-9dd5-4a7d-8573-20337378eef2
📒 Files selected for processing (6)
.changeset/repeat-install-file-deps.mddeps/status/src/checkDepsStatus.tsdeps/status/test/checkDepsStatus.test.tsinstalling/commands/src/installDeps.tspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/package-manager/src/optimistic_repeat_install/tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Compile & Lint
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Dylint
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Doc
- GitHub Check: Analyze (javascript)
- GitHub Check: Code Coverage
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use Standard Style with trailing commas, prefer functions over classes, declare functions after they are used (relying on hoisting), limit functions to no more than two or three arguments, and use a single options object for functions needing more parameters
Follow import order: standard libraries first, then external dependencies (sorted alphabetically), then relative imports
Do not write comments that restate what the code already says; rename variables, split helpers, or move checks to more obvious places instead
Do not repeat documentation at call sites that already lives on the callee; update the JSDoc once and let every call site benefit
Use JSDoc for the function's contract (preconditions, postconditions, edge cases, why the function exists), not for re-narrating the function body
Do not record past implementation shape, refactor history, or removed code in comments; use git log and git blame for that information instead
Write comments only when the reason for code is non-obvious, a hidden invariant exists, a workaround for a known bug is needed, or an exception to surrounding pattern is deliberate
Files:
installing/commands/src/installDeps.tsdeps/status/test/checkDepsStatus.test.tsdeps/status/src/checkDepsStatus.ts
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: Log emissions are part of matching pnpm — when porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plainStringor&strin Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too viaTryFrom<String>and/orFromStrand do not provide an infallible public constructor
If upstream TypeScript never validates a branded string, just brand for type-safety in Rust by exposing an infallibleFrom<String>constructor
If upstream TypeScript occasionally constructs a branded string without validation, exposefrom_str_uncheckedin Rust as an escape hatch alongside the validating constructor
Match upstream serde behavior for branded strings crossing JSON, YAML, or INI boundaries by using#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization
Derive simple conversions for branded strings using#[derive(derive_more::From)]and#[derive(derive_more::Into)]instead of handwritingimplblocks; use manualimplonly when conversion needs custom logic
Model TypeScript string literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers, since the set of valid values is closed
Treat TypeScript string template literal types (like`${string}@${string}`) the same as branded string types in Rust, using a newtype wrapper with validation
Follow the code style guide inCODE_STYLE_GUIDE.md— imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Choose owned vs. borrowed parameters to minimize copies; widen to t...
Files:
pacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/package-manager/src/optimistic_repeat_install.rs
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
instanceof Errorfor checking if a caught error is an Error object in Jest tests; useutil.types.isNativeError()instead to work across realms
Files:
deps/status/test/checkDepsStatus.test.ts
🧠 Learnings (8)
📚 Learning: 2026-05-26T21:01:06.666Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11966
File: .changeset/require-tarball-integrity.md:6-6
Timestamp: 2026-05-26T21:01:06.666Z
Learning: In pnpm lockfile-related release notes/docs (especially changeset markdown), preserve URL hostnames exactly as they appear in pnpm-lock.yaml tarball resolution entries—keep hosts like `codeload.github.com`, `bitbucket.org`, and `gitlab.com` in lowercase. Do not “correct” them to title-case/preserve brand capitalization (e.g., LanguageTool rules like `GITHUB` capitalization) because these are literal URL fragments, not platform brand names.
Applied to files:
.changeset/repeat-install-file-deps.md
📚 Learning: 2026-05-14T09:04:00.133Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/test/publishedBy.test.ts:350-354
Timestamp: 2026-05-14T09:04:00.133Z
Learning: In the pnpm/pnpm repository, ESLint is the authoritative style linter. Do not raise review findings for missing trailing commas in multiline function calls (e.g., `fs.writeFileSync(...)`) when this repo’s ESLint configuration does not report them and lint passes. Prefer deferring to the ESLint results for this specific trailing-comma rule rather than enforcing it manually in code review.
Applied to files:
installing/commands/src/installDeps.tsdeps/status/test/checkDepsStatus.test.tsdeps/status/src/checkDepsStatus.ts
📚 Learning: 2026-06-05T13:47:26.046Z
Learnt from: vsumner
Repo: pnpm/pnpm PR: 12190
File: installing/deps-installer/src/install/index.ts:2337-2343
Timestamp: 2026-06-05T13:47:26.046Z
Learning: In the pnpm/pnpm codebase, `PnpmError` automatically prefixes `err.code` with `ERR_PNPM_` when you pass a code that does not already start with `ERR_PNPM_` (it normalizes `this.code` via `code.startsWith('ERR_PNPM_') ? code : `ERR_PNPM_${code}``). Therefore, during code review you should NOT flag `new PnpmError(...)` call sites for passing a bare error code (e.g., `new PnpmError('FROZEN_STORE_INCOMPATIBLE_WITH_PNPR', ...)`); the resulting `err.code` will still be `ERR_PNPM_FROZEN_STORE_INCOMPATIBLE_WITH_PNPR`.
Applied to files:
installing/commands/src/installDeps.tsdeps/status/test/checkDepsStatus.test.tsdeps/status/src/checkDepsStatus.ts
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.
Applied to files:
pacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/package-manager/src/optimistic_repeat_install.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/package-manager/src/optimistic_repeat_install.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.
Applied to files:
pacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/package-manager/src/optimistic_repeat_install.rs
📚 Learning: 2026-06-06T18:58:37.156Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12243
File: pacquet/crates/package-manager/src/install_package_by_snapshot.rs:319-322
Timestamp: 2026-06-06T18:58:37.156Z
Learning: When reviewing Rust code, do not assume `matches!(expr, Pattern(_))` will move out of `expr` if `Pattern(_)` contains no by-value bindings. `matches!` desugars to a `match` that auto-borrows the scrutinee for discrimination, so even if `expr` is a non-`Copy` value behind a shared reference (e.g., `&T`), the macro should not move-out of the borrowed data purely due to `matches!`. Treat `matches!(&expr, Pattern(_))` as a readability/clarity improvement, not a correctness requirement. Only flag potential move-out-of-borrow risks when the pattern includes by-value bindings that would require moving the matched value.
Applied to files:
pacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/package-manager/src/optimistic_repeat_install.rs
📚 Learning: 2026-06-05T13:47:05.929Z
Learnt from: vsumner
Repo: pnpm/pnpm PR: 12190
File: installing/deps-installer/test/install/frozenStore.ts:2-17
Timestamp: 2026-06-05T13:47:05.929Z
Learning: In the pnpm/pnpm repository, the shared Jest preset keeps `injectGlobals` at its default (`true`), so `test` and `expect` are available as Jest globals. Therefore, reviewers should not flag (or treat as TypeScript/compilation errors) missing `import { test, expect } from 'jest/globals'` when a test file uses `test`/`expect` without importing them. Importing from `jest/globals` may still be used for consistency with sibling files, but it is not required for execution in this repo unless a Jest preset is explicitly configured with `injectGlobals: false`.
Applied to files:
deps/status/test/checkDepsStatus.test.ts
🔇 Additional comments (12)
pacquet/crates/package-manager/src/optimistic_repeat_install.rs (2)
30-35: LGTM!
154-161: LGTM!pacquet/crates/package-manager/src/optimistic_repeat_install/tests.rs (3)
136-156: LGTM!
158-176: LGTM!
178-196: LGTM!.changeset/repeat-install-file-deps.md (1)
1-8: LGTM!deps/status/src/checkDepsStatus.ts (4)
30-36: LGTM!
72-80: LGTM!
163-174: LGTM!
648-665: LGTM!deps/status/test/checkDepsStatus.test.ts (1)
794-926: LGTM!installing/commands/src/installDeps.ts (1)
185-185: LGTM!
|
Code review by qodo was updated up to the latest commit 2e54668 |
Integrated-Benchmark Report (Linux)Each scenario reports direct installs and pnpr installs. Bencher consumes pacquet@HEAD and pnpr@HEAD. Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 4.24839802978,
"stddev": 0.17283668755338266,
"median": 4.159619127079999,
"user": 4.07218196,
"system": 3.41633188,
"min": 4.080380872579999,
"max": 4.55660938258,
"times": [
4.55660938258,
4.11262729758,
4.36463093058,
4.080380872579999,
4.4326905625799995,
4.39855000858,
4.1946697435799996,
4.124458758579999,
4.09479423058,
4.12456851058
]
},
{
"command": "pacquet@main",
"mean": 4.22907311208,
"stddev": 0.20480009132314458,
"median": 4.109486112079999,
"user": 4.02824436,
"system": 3.4509968800000004,
"min": 4.052138321579999,
"max": 4.56729668958,
"times": [
4.56729668958,
4.066483594579999,
4.064191171579999,
4.06710328558,
4.052138321579999,
4.51489778958,
4.115554630579999,
4.4009513365799995,
4.33869670758,
4.10341759358
]
},
{
"command": "pnpr@HEAD",
"mean": 2.11993962398,
"stddev": 0.10440143307674862,
"median": 2.1070098545800002,
"user": 2.7171987599999996,
"system": 2.9079628800000004,
"min": 1.9661425065800002,
"max": 2.30779488758,
"times": [
2.08901407758,
2.02692231458,
2.19340090558,
2.1923879625800002,
2.19989287158,
2.10216946558,
2.00982100458,
2.11185024358,
1.9661425065800002,
2.30779488758
]
},
{
"command": "pnpr@main",
"mean": 2.15084691578,
"stddev": 0.1665393742604394,
"median": 2.07072743358,
"user": 2.7202663599999997,
"system": 2.9254101799999996,
"min": 1.9571139555800001,
"max": 2.3618581985800002,
"times": [
1.9571139555800001,
2.29284035058,
2.0678828135800003,
1.9987481535800002,
2.3618581985800002,
2.04161839758,
2.07357205358,
2.35568495958,
2.34671178358,
2.01243849158
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.62985401988,
"stddev": 0.013477050904801752,
"median": 0.62291489108,
"user": 0.36068461999999996,
"system": 1.3371802,
"min": 0.61630897608,
"max": 0.65210547508,
"times": [
0.64277279808,
0.62378399808,
0.61707430808,
0.64198446608,
0.65210547508,
0.62204578408,
0.64294811508,
0.61630897608,
0.61808535808,
0.62143092008
]
},
{
"command": "pacquet@main",
"mean": 0.67219933748,
"stddev": 0.108428013517013,
"median": 0.63680261758,
"user": 0.36422292,
"system": 1.3387181,
"min": 0.61343862108,
"max": 0.97589058508,
"times": [
0.67613601708,
0.63945289108,
0.65855621508,
0.63262277708,
0.61343862108,
0.63415234408,
0.61789664308,
0.62272611708,
0.97589058508,
0.65112116408
]
},
{
"command": "pnpr@HEAD",
"mean": 0.68485909708,
"stddev": 0.024542969282961208,
"median": 0.68001474858,
"user": 0.37237802000000003,
"system": 1.3662254,
"min": 0.65769678208,
"max": 0.74591508008,
"times": [
0.69607657108,
0.67858593208,
0.65769678208,
0.69236555908,
0.67808415208,
0.66509614708,
0.66795762208,
0.74591508008,
0.68144356508,
0.68536956008
]
},
{
"command": "pnpr@main",
"mean": 0.6825293093800001,
"stddev": 0.019099616304447342,
"median": 0.67683604608,
"user": 0.37627872,
"system": 1.3576024,
"min": 0.66715236408,
"max": 0.73107059208,
"times": [
0.73107059208,
0.66969084908,
0.68252740408,
0.66976998808,
0.67029357508,
0.68852479708,
0.67448091408,
0.67919117808,
0.69259143208,
0.66715236408
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 4.31849694224,
"stddev": 0.03485979183470616,
"median": 4.32970531734,
"user": 3.8757404799999997,
"system": 3.3350613200000003,
"min": 4.2408191223400005,
"max": 4.35659604234,
"times": [
4.35659604234,
4.30944295234,
4.33489806534,
4.32451256934,
4.3392289143400005,
4.31727386934,
4.27909697034,
4.33857524934,
4.34452566734,
4.2408191223400005
]
},
{
"command": "pacquet@main",
"mean": 4.25591208134,
"stddev": 0.056221424047973895,
"median": 4.25059893034,
"user": 3.8288838800000002,
"system": 3.3262532199999995,
"min": 4.17663428034,
"max": 4.33896845934,
"times": [
4.29618748634,
4.338013413340001,
4.33896845934,
4.24796703134,
4.26560478934,
4.21698971334,
4.18538936434,
4.24013544634,
4.25323082934,
4.17663428034
]
},
{
"command": "pnpr@HEAD",
"mean": 2.24593640784,
"stddev": 0.12973064338309204,
"median": 2.2102198998400002,
"user": 2.54158008,
"system": 2.82065562,
"min": 2.09985389134,
"max": 2.43593852034,
"times": [
2.43593852034,
2.19302193734,
2.11321272134,
2.34103242734,
2.36942358434,
2.1685424863400002,
2.4017604173400002,
2.2274178623400003,
2.09985389134,
2.10916023034
]
},
{
"command": "pnpr@main",
"mean": 2.22844000794,
"stddev": 0.10566730065536296,
"median": 2.22059643334,
"user": 2.55908098,
"system": 2.84046402,
"min": 2.10745378734,
"max": 2.40296418634,
"times": [
2.10745378734,
2.11600897134,
2.40296418634,
2.22003013534,
2.32256483034,
2.10764795534,
2.22116273134,
2.1933704343400002,
2.23080268434,
2.36239436334
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.39254553118,
"stddev": 0.01806178951099464,
"median": 1.39567033848,
"user": 1.35857526,
"system": 1.7476858399999997,
"min": 1.35521763098,
"max": 1.41771149198,
"times": [
1.41031894798,
1.37954213498,
1.38336850598,
1.35521763098,
1.39546836098,
1.39587231598,
1.40657816998,
1.38304897198,
1.41771149198,
1.39832878098
]
},
{
"command": "pacquet@main",
"mean": 1.42287144298,
"stddev": 0.016991718442795383,
"median": 1.41972463348,
"user": 1.37859386,
"system": 1.76046784,
"min": 1.39502081298,
"max": 1.45103911598,
"times": [
1.40990191798,
1.42074313598,
1.41057834398,
1.39502081298,
1.41870613098,
1.41413658198,
1.43103505298,
1.44320882198,
1.43434451498,
1.45103911598
]
},
{
"command": "pnpr@HEAD",
"mean": 0.66548362068,
"stddev": 0.016522534386570226,
"median": 0.66444442598,
"user": 0.3430514599999999,
"system": 1.29786394,
"min": 0.64741428598,
"max": 0.69870516698,
"times": [
0.67033440498,
0.64902329998,
0.66990146298,
0.69870516698,
0.68301351298,
0.67066950898,
0.65898738898,
0.64870017198,
0.65808700298,
0.64741428598
]
},
{
"command": "pnpr@main",
"mean": 0.67470323978,
"stddev": 0.038724914899723545,
"median": 0.66274543898,
"user": 0.34507086,
"system": 1.30328014,
"min": 0.64794339298,
"max": 0.77952814798,
"times": [
0.64794339298,
0.65106016498,
0.68127084598,
0.66752058498,
0.6575497889799999,
0.67667242698,
0.77952814798,
0.65797029298,
0.65109323898,
0.67642351298
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 3.16326288004,
"stddev": 0.04825819735675574,
"median": 3.15244006164,
"user": 1.8857446999999996,
"system": 2.0366999600000004,
"min": 3.09528827414,
"max": 3.24244244014,
"times": [
3.21188451914,
3.1399012381399998,
3.18236404114,
3.15146848914,
3.21347943514,
3.24244244014,
3.10541984114,
3.09528827414,
3.1369688881399997,
3.15341163414
]
},
{
"command": "pacquet@main",
"mean": 3.09161499334,
"stddev": 0.03347412813495982,
"median": 3.09686769264,
"user": 1.7911249000000002,
"system": 2.0141028600000004,
"min": 3.04426258014,
"max": 3.15104483014,
"times": [
3.08458871814,
3.10084083914,
3.11503333314,
3.0595701641399997,
3.10948266214,
3.09289454614,
3.11061971114,
3.04781254914,
3.15104483014,
3.04426258014
]
},
{
"command": "pnpr@HEAD",
"mean": 0.65346179344,
"stddev": 0.011533503286689215,
"median": 0.6533737526400001,
"user": 0.31809659999999995,
"system": 1.2919521599999997,
"min": 0.6243287381400001,
"max": 0.6653434571400001,
"times": [
0.65182893214,
0.6521116371400001,
0.66127171914,
0.6243287381400001,
0.6504886321400001,
0.6653434571400001,
0.6516889171400001,
0.6609610361400001,
0.6619589971400001,
0.6546358681400001
]
},
{
"command": "pnpr@main",
"mean": 0.63974598944,
"stddev": 0.008476394837847187,
"median": 0.6423619016400002,
"user": 0.32275970000000004,
"system": 1.27750166,
"min": 0.6183932511400001,
"max": 0.6487278891400001,
"times": [
0.6415233561400001,
0.6183932511400001,
0.6447301471400001,
0.6392750731400001,
0.64551365514,
0.63719580614,
0.6487278891400001,
0.6432004471400001,
0.64331853914,
0.6355817301400001
]
}
]
} |
|
| Branch | pr/12317 |
| Testbed | pacquet |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 4,318.50 ms(+3.49%)Baseline: 4,172.95 ms | 5,007.54 ms (86.24%) |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot 🚷 view threshold | 3,163.26 ms(+5.69%)Baseline: 2,992.85 ms | 3,591.42 ms (88.08%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,392.55 ms(+5.85%)Baseline: 1,315.54 ms | 1,578.65 ms (88.21%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 4,248.40 ms(+6.97%)Baseline: 3,971.44 ms | 4,765.72 ms (89.14%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 629.85 ms(+1.37%)Baseline: 621.36 ms | 745.63 ms (84.47%) |
|
| Branch | pr/12317 |
| Testbed | pnpr |
⚠️ WARNING: No Threshold found!Without a Threshold, no Alerts will ever be generated.
Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the--ci-only-thresholdsflag.
Click to view all benchmark results
| Benchmark | Latency | milliseconds (ms) |
|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot | 2,245.94 ms |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot | 653.46 ms |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot | 665.48 ms |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot | 2,119.94 ms |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot | 684.86 ms |
|
Code review by qodo was updated up to the latest commit 2b3196c |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12317 +/- ##
==========================================
+ Coverage 88.03% 88.07% +0.03%
==========================================
Files 309 309
Lines 41595 41702 +107
==========================================
+ Hits 36618 36727 +109
+ Misses 4977 4975 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Code review by qodo was updated up to the latest commit fb9c546 |
|
Code review by qodo was updated up to the latest commit 7a3b64c |
|
Code review by qodo was updated up to the latest commit 8251a00 |
|
Code review by qodo was updated up to the latest commit 471f5b0 |
|
Code review by qodo was updated up to the latest commit 81652c9 |
|
Code review by qodo was updated up to the latest commit fda788d |
|
Code review by qodo was updated up to the latest commit d983e4b |
|
Code review by qodo was updated up to the latest commit f705dcf |
|
Code review by qodo was updated up to the latest commit 8fcfff7 |
|
Code review by qodo was updated up to the latest commit e3b880f |
e3b880f to
8c800b6
Compare
|
Code review by qodo was updated up to the latest commit 8c800b6 |
|
Code review by qodo was updated up to the latest commit 89b5d13 |
|
Code review by qodo was updated up to the latest commit c3dcdca |
…uet + pnpm) An override that maps a dependency to a file: specifier (or a bare local path/tarball) redirects every matching dependency in the graph to that directory, so its contents can change without any tracked mtime moving, the same blind spot as a direct local file dependency (pnpm#11795). The fast path now scans the resolved override values with the same local-file predicate on both the TypeScript and pacquet sides.
… pnpm) The local resolver's isFilespec (resolving/local-resolver/src/parseBareSpecifier.ts) treats any [a-z]: prefix as a local path, with no separator required after the colon, so a drive-relative spec like C:dir resolves locally. The repeat-install predicate required a separator, leaving such specs on the fast path. Relaxing is unambiguous: no registry or pnpm protocol is a single letter.
…le dep check (pacquet + pnpm)
The catalog-registry-range test reaches catalogs_cache_matches now that the workspace state records catalogs (pnpm#12382). Stamp the state with the test's catalogs so the cache comparison passes and the spec deref is what gets exercised, not the catalogs-outdated bail.
…Projects omits it When allProjects was provided, rootProjectManifest was ignored, so a root-only local file dependency (for example under includeWorkspaceRoot: false) could slip past the treatLocalFileDepsAsOutdated bail-out.
…nstall Installs a file: directory dependency, edits a file inside it, reruns install with optimisticRepeatInstall enabled, and asserts node_modules refreshes (pnpm#11795).
Avoid per-dependency allocations: catalog_resolves_to_local_file short-circuits before building an owned WantedDependency for non-catalog: specs, and is_local_file_spec checks tarball suffixes case-insensitively on bytes instead of allocating a lowercased copy. Use a distinct skip reason when pnpm.overrides cannot be parsed instead of misattributing the failure to a local file override.
2b1e94b to
4b4d436
Compare
… dep check findLocalFileDep only needs resolveFromCatalog for catalog: specs; guarding the call keeps the optimistic fast path allocation-free for the common case, matching the pacquet port.
|
Code review by qodo was updated up to the latest commit c0e4d9c |
… fast path (pacquet + pnpm) isLocalFileSpec / is_local_file_spec treated any colon-free spec ending in .tgz/.tar.gz/.tar as a local tarball, so a hosted-git shorthand like user/repo#release.tgz was misclassified and always forced a full install. Exclude specs carrying a # committish from the bare-tarball match.
|
Code review by qodo was updated up to the latest commit 4ea0486 |
…install (pacquet + pnpm) packageExtensions are merged into matching packages' manifests by the read-package hook during install, so a file:/local-path/tarball spec added there has the same content-change blind spot as a direct local file dependency without appearing in any project manifest. Scan dependencies and optionalDependencies (peers aren't fetched), respecting the install's include flags.
|
Code review by qodo was updated up to the latest commit a531a26 |
Already live on prod via the prior deploy; committing so main matches.
Summary
pnpm installreports "Already up to date" after edits inside afile:dependency's directory or after repacking afile:tarball. This is a v11 regression from theoptimisticRepeatInstalldefault flip in feat: set default minimumReleaseAge to 1 day (1440 minutes) #11158. Fixes pnpm install no longer detects changes in file: directory dependencies (regression from v10) #11795.checkDepsStatusgains atreatLocalFileDepsAsOutdatedoption: when set, any project manifest declaring a local file dependency makes the check report not up to date.installDepssets it on the optimistic fast path, so projects with local file dependencies always run a real install, which refetches those dependencies (the v10 behavior).file:specs, path-prefixed specs (./,../,~/, absolute POSIX paths, and Windows drive paths including drive-relative ones likeC:dir, matching the local resolver'sisFilespec), and bare tarball file names (vendor/pkg.tgz). It is deliberately narrower than the local resolver's bare-path matching: a bareuser/repois statically indistinguishable from a git shorthand at this layer, and matching it would kill the fast path for every project with git dependencies, so protocol-carrying and URL specs stay on the fast path.pnpm.overridesentries are scanned with the same predicate: an override mapping to a local file spec redirects every matching dependency in the graph to that directory, so it has the same blind spot as a direct local file dependency. Registry andlink:overrides keep the fast path.verifyDepsBeforeRunalso consumescheckDepsStatus, and treatingfile:deps as always stale there would force a reinstall before everypnpm run. Its behavior is unchanged, and a regression test pins that.check_optimistic_repeat_installbails unconditionally onfile:specifiers, because its only caller is the install command, the one consumer that sets the flag upstream.link:specifiers are excluded on both sides: they are symlinked, so changes inside them flow through without a reinstall.Why
Both branches of
checkDepsStatusare blind to content changes inside afile:dependency. The workspace branch exits early withupToDate: truewhen no project manifest's mtime moved, without ever reachinglinkedPackagesAreUpToDate. The non-workspace branch exits at the manifest-vs-lockfile mtime gate the same way. Editing a source file inside afile:dependency bumps neither, so the fast path can never see it; the fix has to bail before those gates rather than refine them. This is the fix shape (a) I proposed in my diagnosis on the issue thread (comment): the cost is a full resolution on repeat installs only for projects that declarefile:dependencies, which is exactly what v10 did.The manifest-only comparison in
@pnpm/lockfile.verification(allProjectsAreUpToDate) is intentional for the install-proper path and asserted by its tests, so this PR leaves it untouched.Checks
pnpm --filter @pnpm/deps.status test test/checkDepsStatus.test.ts(31 passed, 13 new)pnpm --filter @pnpm/deps.status run compileandpnpm --filter @pnpm/installing.commands run compile(tsgo + eslint clean)cargo test -p pacquet-package-manager optimistic_repeat_install(51 passed, 7 new; run in a rust:1.95.0 container)cargo fmt --check -p pacquet-package-managerRUSTDOCFLAGS="-D warnings" cargo doc -p pacquet-package-manager --no-depsWritten by an agent (Claude Code, claude-fable-5).
Summary by CodeRabbit
Bug Fixes
Tests
Chores