test(registry-mock): read uplinked packuments from pnpr's proxy cache dir#12205
Merged
Conversation
… dir #12195 separated pnpr's proxied upstream cache from hosted packages, moving proxied packuments to a `.pnpr-cache` subdirectory of the storage root. The `getIntegrity` test helper still only read the hosted `<storage>/<pkg>/package.json` path, so tests that resolve integrity for packages uplinked from the real npm registry (e.g. store add express, store prune is-negative) failed with ENOENT. Try the hosted location first, then `<storage>/.pnpr-cache/<pkg>/package.json`, keeping the existing retry for the lazy/in-flight cache write.
📝 WalkthroughWalkthroughRegistry mock integrity lookup now supports upstream-proxied packages stored separately in a ChangesRegistry Mock Multi-Candidate Integrity Lookup
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
Review Summary by QodoFix registry-mock getIntegrity to read from pnpr proxy cache directory
WalkthroughsDescription• Fix getIntegrity helper to read uplinked packages from pnpr's proxy cache • Support both hosted fixture packages and proxied upstream packages locations • Refactor error handling to distinguish transient vs permanent read failures • Resolve ENOENT failures in store add/prune tests with real npm packages Diagramflowchart LR
A["getIntegrity called"] --> B["Try hosted path<br/>storage/pkg/package.json"]
B --> C{File found?}
C -->|Yes| D["Parse packument"]
C -->|No| E["Try proxy cache path<br/>storage/.pnpr-cache/pkg/package.json"]
E --> F{File found?}
F -->|Yes| G["Parse packument"]
F -->|No| H{Max retries?}
H -->|No| I["Wait & retry"]
I --> B
H -->|Yes| J["Throw error"]
D --> K{Valid JSON?}
G --> K
K -->|Yes| L["Return integrity"]
K -->|Partial| M["Retry"]
M --> B
K -->|Error| N["Throw error"]
File Changes1. testing/registry-mock/src/index.ts
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
testing/registry-mock/src/index.ts (1)
92-95: 💤 Low valueConsider a clearer error when the requested version is missing.
If
pkgVersiondoesn't exist in the packument, line 94 throws a genericTypeError: Cannot read properties of undefined. A guard with a descriptive message would make test failures easier to debug.🛠️ Suggested improvement
const content = readPackument(candidatePaths) if (content) { + const versionData = content.versions[pkgVersion] + if (!versionData) { + throw new Error(`Version ${pkgVersion} not found in packument for ${pkgName}`) + } - return content.versions[pkgVersion].dist.integrity + return versionData.dist.integrity }🤖 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 `@testing/registry-mock/src/index.ts` around lines 92 - 95, The code calls readPackument(candidatePaths) and then directly accesses content.versions[pkgVersion].dist.integrity which throws a TypeError if the requested version is missing; add a guard after reading the packument to verify content is truthy and content.versions && content.versions[pkgVersion] exists, and if not throw a descriptive Error (e.g., "packument missing requested version <pkgVersion> for candidatePaths <candidatePaths>") so tests fail with a clear message; update the branch that currently returns content.versions[pkgVersion].dist.integrity to first validate presence and then return the integrity string.
🤖 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 `@testing/registry-mock/src/index.ts`:
- Around line 92-95: The code calls readPackument(candidatePaths) and then
directly accesses content.versions[pkgVersion].dist.integrity which throws a
TypeError if the requested version is missing; add a guard after reading the
packument to verify content is truthy and content.versions &&
content.versions[pkgVersion] exists, and if not throw a descriptive Error (e.g.,
"packument missing requested version <pkgVersion> for candidatePaths
<candidatePaths>") so tests fail with a clear message; update the branch that
currently returns content.versions[pkgVersion].dist.integrity to first validate
presence and then return the integrity string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a1f6ed7-e1ad-44a2-af6f-b24cad74d09b
📒 Files selected for processing (1)
testing/registry-mock/src/index.ts
📜 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). (1)
- GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Follow Standard Style with trailing commas, preferring functions over classes, and declaring functions after they are used (relying on hoisting)
Use a single options object instead of multiple parameters when a function needs more than two or three arguments
Follow Import Order: Standard libraries first, then external dependencies (alphabetically), then relative imports
Write self-documenting code where function names, parameters, and types explain what a function does without requiring prose comments
Do not write comments that restate what the code already says; refactor via renaming, splitting helpers, or restructuring instead
Do not repeat documentation at call sites that already exists in JSDoc on the callee; update JSDoc once for all call sites to benefit
Use JSDoc only for a function's contract (preconditions, postconditions, edge cases, why the function exists), not for re-narrating the body
Do not record past implementation shape, refactor history, or 'the previous code did X' framing in code; use git log and git blame instead
Write comments only when: the reason for code is non-obvious (hidden invariant, workaround for known bug, deliberate exception), or the right name doesn't fit (temporary technical constraint)
Files:
testing/registry-mock/src/index.ts
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.372Z
Learning: Applies to pacquet/**/tests/**/*.rs : Tests that need the mocked registry should start `pnpr` through `pacquet-testing-utils`; `cargo test` / `cargo nextest run` should not require a separate `just registry-mock launch` step
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11895
File: pnpm/test/deploy.ts:91-95
Timestamp: 2026-05-24T08:18:06.019Z
Learning: In the pnpm/pnpm repository, integration tests that hit the real `registry.npmjs.org` (e.g., for `pacquet` or `pnpm/pacquet`) do NOT use a runtime env-var gate (such as `PNPM_RUN_PUBLIC_REGISTRY_TESTS`). They simply pass `--config.registry=https://registry.npmjs.org/` directly to `execPnpm` and set a higher timeout. This is the established pattern, as seen in `pnpm/test/install/pacquet.ts` and `pnpm/test/deploy.ts`. Do not suggest adding env-var guards for these tests.
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-25T12:36:42.202Z
Learning: User-visible changes (CLI flags, defaults, environment variables, lockfile/manifest/state-file formats, error codes/messages, log emissions, store layout, hook semantics) in pnpm must be mirrored to pacquet in the same PR
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.372Z
Learning: Match how the same feature is implemented in the TypeScript pnpm CLI — any change in pacquet must match pnpm's behavior, logic, edge cases, config resolution, error messages, file/lockfile formats, and existing tests
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11878
File: resolving/npm-resolver/src/createNpmResolutionVerifier.ts:381-418
Timestamp: 2026-05-23T17:30:06.849Z
Learning: In `resolving/npm-resolver/src/pickPackage.ts` (pnpm/pnpm), the resolver's `PackageMetaCache` keys by `name` (abbreviated) and `name:full` (full metadata) only — no registry component is included. This is a pre-existing limitation meaning that if two different registries serve packages of the same name in one install, the cache will only hold the first fetched entry. The `createNpmResolutionVerifier.ts` shares this same cache and inherits the limitation; a `validateSharedMeta` name-check guards against cross-package contamination but cannot distinguish same-named packages from different registries. Tightening to a registry-qualified key would require a coordinated change to the resolver's cache key shape. The Pacquet/Rust side is already registry-qualified (`{registry}\x00{name}:full`).
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11778
File: pacquet/crates/resolving-local-resolver/tests/resolve.rs:365-372
Timestamp: 2026-05-20T21:18:56.391Z
Learning: In `pacquet/crates/resolving-local-resolver/tests/resolve.rs`, the test `fail_when_resolving_from_not_existing_directory_an_injected_dependency` intentionally uses `injected: false`. The test is a verbatim port of the upstream pnpm TypeScript test (resolving/local-resolver/test/index.ts at ef87f3ccff). The `injected` flag only affects the file/link protocol choice for plain directory paths; when the `file:` scheme is explicit in the bare specifier, the flag has no effect on the resolution code path. The misleading test name is inherited from upstream.
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:11:04.272Z
Learning: In the pacquet Rust port (pnpm/pnpm repo), the `ResolvedPackage.optional` AND-folding on revisit intentionally mirrors pnpm's `resolveDependencies.ts:1627-1648` behavior: only the directly-revisited package's `optional` flag is updated; transitive descendants are not re-walked. pnpm CLI corrects stale optional flags downstream via `copyDependencySubGraph` BFS in `lockfile/pruner/src/index.ts:160-205`, which tracks a `nonOptional` set and re-stamps any package reachable by an all-non-optional path. Pacquet does not yet have this pruner equivalent, so the stale flags flow directly through `dependencies_graph_to_lockfile.rs:409` → `create_virtual_store.rs:762` → `installability.rs:394`. A follow-up to port `copyDependencySubGraph` is planned; until then, do not flag the resolver-layer optional propagation gap as a bug in pacquet PRs — it is intentional parity with pnpm's resolver layer.
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12189
File: pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs:435-439
Timestamp: 2026-06-04T14:40:25.306Z
Learning: In `pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs` (pnpm/pnpm repo), the pnpr install accelerator always invokes `Install` with `lockfile_only: true` (hard-coded in `pnpr/crates/pnpr/src/install_accelerator/resolve.rs`). Under `lockfile_only: true`:
1. The `PrefetchingResolver` wrapper is skipped — the bare `inner_resolver` is used instead, so `PrefetchContext { config }` is never constructed.
2. The function returns before `CreateVirtualStore` is reached, so `install_package_by_snapshot` and its `config.auth_headers` fetch path are never hit.
pnpr's tarball fetch is handled separately in `resolve::fetch_uncached`, which independently receives the request-scoped `auth_headers`. Therefore, `auth_override` only needs to be threaded into the resolver-side components (NpmResolver, TarballResolver, NamedRegistryResolver) — not into PrefetchingResolver or CreateVirtualStore — on the pnpr path. For local installs (`lockfile_only: false`), `auth_override` is always `None` a...
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:08:06.093Z
Learning: Pacquet (pnpm's Rust port) has a cardinal rule: "match pnpm exactly — do not fix pnpm quirks unless the same fix has landed in pnpm first." Review comments should not suggest behavioral deviations from upstream pnpm, even when the upstream behavior appears buggy. If a real bug is identified, it must be fixed upstream first.
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.372Z
Learning: Reference the upstream pnpm commit/PR when porting code from pnpm in commit messages
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11755
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:370-380
Timestamp: 2026-05-20T01:42:44.958Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/pick_package.rs`, the fetch-error fallback that returns `Ok(PickPackageResult { meta: disk, picked_package: picked })` — even when `picked` is `None` — is intentional upstream parity with pnpm's `pickPackage.ts` catch block (https://github.com/pnpm/pnpm/blob/f657b5cb44/resolving/npm-resolver/src/pickPackage.ts#L420-L431). When a fetch fails and a disk mirror exists, the stale-mirror pick (including null/None) is returned verbatim; the transport error is logged via `tracing::debug!`. Do not flag this as a bug.
📚 Learning: 2026-05-23T17:30:06.849Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11878
File: resolving/npm-resolver/src/createNpmResolutionVerifier.ts:381-418
Timestamp: 2026-05-23T17:30:06.849Z
Learning: In `resolving/npm-resolver/src/pickPackage.ts` (pnpm/pnpm), the resolver's `PackageMetaCache` keys by `name` (abbreviated) and `name:full` (full metadata) only — no registry component is included. This is a pre-existing limitation meaning that if two different registries serve packages of the same name in one install, the cache will only hold the first fetched entry. The `createNpmResolutionVerifier.ts` shares this same cache and inherits the limitation; a `validateSharedMeta` name-check guards against cross-package contamination but cannot distinguish same-named packages from different registries. Tightening to a registry-qualified key would require a coordinated change to the resolver's cache key shape. The Pacquet/Rust side is already registry-qualified (`{registry}\x00{name}:full`).
Applied to files:
testing/registry-mock/src/index.ts
📚 Learning: 2026-05-14T07:57:23.823Z
Learnt from: mandarini
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/src/pickPackage.ts:183-221
Timestamp: 2026-05-14T07:57:23.823Z
Learning: In `resolving/npm-resolver/src/pickPackage.ts`, the error-fallback `catch` block (the `loadMeta(pkgMirror)` path that fires when the primary network fetch throws) intentionally does NOT call `maybeUpgradeAbbreviatedMetaForReleaseAge` or retry with `fullMetadata: true`. This is by design: the network is already known-iffy at that point, so an extra fetch would risk compounding the failure. The `ignoreMissingTimeField` warn-and-skip path is the accepted graceful degradation here.
Applied to files:
testing/registry-mock/src/index.ts
📚 Learning: 2026-05-29T18:03:15.372Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.372Z
Learning: Applies to pacquet/**/tests/**/*.rs : Tests that need the mocked registry should start `pnpr` through `pacquet-testing-utils`; `cargo test` / `cargo nextest run` should not require a separate `just registry-mock launch` step
Applied to files:
testing/registry-mock/src/index.ts
📚 Learning: 2026-05-24T08:18:06.019Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11895
File: pnpm/test/deploy.ts:91-95
Timestamp: 2026-05-24T08:18:06.019Z
Learning: In the pnpm/pnpm repository, integration tests that hit the real `registry.npmjs.org` (e.g., for `pacquet` or `pnpm/pacquet`) do NOT use a runtime env-var gate (such as `PNPM_RUN_PUBLIC_REGISTRY_TESTS`). They simply pass `--config.registry=https://registry.npmjs.org/` directly to `execPnpm` and set a higher timeout. This is the established pattern, as seen in `pnpm/test/install/pacquet.ts` and `pnpm/test/deploy.ts`. Do not suggest adding env-var guards for these tests.
Applied to files:
testing/registry-mock/src/index.ts
📚 Learning: 2026-05-29T18:03:15.372Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.372Z
Learning: Match how the same feature is implemented in the TypeScript pnpm CLI — any change in pacquet must match pnpm's behavior, logic, edge cases, config resolution, error messages, file/lockfile formats, and existing tests
Applied to files:
testing/registry-mock/src/index.ts
📚 Learning: 2026-05-25T12:36:42.202Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-25T12:36:42.202Z
Learning: User-visible changes (CLI flags, defaults, environment variables, lockfile/manifest/state-file formats, error codes/messages, log emissions, store layout, hook semantics) in pnpm must be mirrored to pacquet in the same PR
Applied to files:
testing/registry-mock/src/index.ts
📚 Learning: 2026-05-20T01:52:55.764Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11755
File: pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs:193-200
Timestamp: 2026-05-20T01:52:55.764Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs`, the package-level `modified` shortcut for the maturity/minimumReleaseAge filter uses inclusive `<=` (not strict `<`) when comparing `modified_date <= cutoff`. This mirrors the corrected behavior in pnpm (fixed in ab4c96ead5). The reasoning: `modified` is "last modification time," which is an upper bound on every version's `time[v]`. The per-version maturity filter uses `<=` (a version published exactly at the cutoff is mature). Since `modified == cutoff` means every version satisfies the per-version filter, the abbreviated-metadata fast path should accept this case rather than forcing a full-metadata re-fetch or raising `MissingTime`. The same fix was applied to pnpm TS: `pickPackage.ts` (×2) and `pickPackageFromMeta.ts`.
Applied to files:
testing/registry-mock/src/index.ts
📚 Learning: 2026-05-20T21:18:56.391Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11778
File: pacquet/crates/resolving-local-resolver/tests/resolve.rs:365-372
Timestamp: 2026-05-20T21:18:56.391Z
Learning: In `pacquet/crates/resolving-local-resolver/tests/resolve.rs`, the test `fail_when_resolving_from_not_existing_directory_an_injected_dependency` intentionally uses `injected: false`. The test is a verbatim port of the upstream pnpm TypeScript test (resolving/local-resolver/test/index.ts at ef87f3ccff). The `injected` flag only affects the file/link protocol choice for plain directory paths; when the `file:` scheme is explicit in the bare specifier, the flag has no effect on the resolution code path. The misleading test name is inherited from upstream.
Applied to files:
testing/registry-mock/src/index.ts
📚 Learning: 2026-05-25T14:58:11.105Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11931
File: pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs:560-589
Timestamp: 2026-05-25T14:58:11.105Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs`, all per-`(registry, name[, version])` caches in `NpmResolutionVerifier` (`published_at`, `full_meta`, `full_meta_for_trust`, `abbreviated_meta`, `local_meta`) intentionally use the same pattern: lock → miss-check → release lock → await fetch/load → re-acquire lock → insert. This uniform pattern is deliberate; do not flag individual caches for using it. The known follow-up improvement (replacing the pattern with `tokio::sync::OnceCell` per key inside a `Mutex<HashMap<…>>`) is tracked as a future structural change to cover all five caches simultaneously.
Applied to files:
testing/registry-mock/src/index.ts
📚 Learning: 2026-05-18T20:35:22.917Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11729
File: pacquet/crates/resolving-npm-resolver/src/fetch_attestation_published_at.rs:55-57
Timestamp: 2026-05-18T20:35:22.917Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/fetch_attestation_published_at.rs`, the npm attestation endpoint (`/-/npm/v1/attestations/{pkg_name}@{version}`) intentionally does NOT percent-encode the package name — the endpoint accepts literal `/` in scoped package names (e.g. `scope/pkg`). This matches upstream pnpm's `fetchAttestationPublishedAt.ts` behavior. Do not flag missing URL encoding here. By contrast, the full-metadata fetch paths (`fetch_full_metadata`, `fetch_full_metadata_cached`) DO percent-encode via the `registry_url::to_registry_url` helper, matching upstream's `toUri` behavior.
Applied to files:
testing/registry-mock/src/index.ts
📚 Learning: 2026-05-20T20:41:50.322Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11773
File: pacquet/crates/package-manager/src/install_package_from_registry.rs:111-114
Timestamp: 2026-05-20T20:41:50.322Z
Learning: In pacquet (pnpm/pnpm repo, Rust codebase), `install_package_from_registry` is the npm-only install path. The npm resolver always stamps `ResolveResult.id` (a `PkgResolutionId`) as `nameversion`. Parsing it back through `PkgNameVer` with `.expect()` is intentional — a parse failure means a mis-dispatch bug, not malformed external input. Per pacquet's CLAUDE.md: "Don't add error handling, fallbacks, or validation for scenarios that can't happen. Trust internal code and framework guarantees." Do not suggest replacing such `.expect()` calls with graceful error handling.
Applied to files:
testing/registry-mock/src/index.ts
📚 Learning: 2026-06-04T14:40:25.306Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12189
File: pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs:435-439
Timestamp: 2026-06-04T14:40:25.306Z
Learning: In `pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs` (pnpm/pnpm repo), the pnpr install accelerator always invokes `Install` with `lockfile_only: true` (hard-coded in `pnpr/crates/pnpr/src/install_accelerator/resolve.rs`). Under `lockfile_only: true`:
1. The `PrefetchingResolver` wrapper is skipped — the bare `inner_resolver` is used instead, so `PrefetchContext { config }` is never constructed.
2. The function returns before `CreateVirtualStore` is reached, so `install_package_by_snapshot` and its `config.auth_headers` fetch path are never hit.
pnpr's tarball fetch is handled separately in `resolve::fetch_uncached`, which independently receives the request-scoped `auth_headers`. Therefore, `auth_override` only needs to be threaded into the resolver-side components (NpmResolver, TarballResolver, NamedRegistryResolver) — not into PrefetchingResolver or CreateVirtualStore — on the pnpr path. For local installs (`lockfile_only: false`), `auth_override` is always `None` a...
Applied to files:
testing/registry-mock/src/index.ts
📚 Learning: 2026-06-04T20:24:28.672Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12198
File: pnpr/crates/pnpr/src/storage.rs:469-477
Timestamp: 2026-06-04T20:24:28.672Z
Learning: In `pnpr/crates/pnpr/src/storage.rs` (pnpm/pnpm repo, Rust), `Store::list_package_names` intentionally uses `fs::try_exists(...).await.unwrap_or(false)` and `if let Ok(mut inner) = fs::read_dir(...)` — NOT `?`-propagation — for per-entry checks. This is deliberate best-effort / verdaccio-style search behavior: (1) `try_exists(stray_file/package.json)` returns `ENOTDIR` (not `NotFound`) for a stray non-package file in the store root, so `?` would fail the entire search; (2) the `@`-scope `read_dir` would fail on a non-directory `@`-named entry; (3) switching to `DirEntry::file_type()` would stop following symlinked package dirs. Failures that DO propagate are preserved: opening the store root itself, and `next_entry()` during the walk. Do not suggest blanket `?`-propagation for these per-entry checks.
Applied to files:
testing/registry-mock/src/index.ts
📚 Learning: 2026-05-20T01:42:44.958Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11755
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:370-380
Timestamp: 2026-05-20T01:42:44.958Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/pick_package.rs`, the fetch-error fallback that returns `Ok(PickPackageResult { meta: disk, picked_package: picked })` — even when `picked` is `None` — is intentional upstream parity with pnpm's `pickPackage.ts` catch block (https://github.com/pnpm/pnpm/blob/f657b5cb44/resolving/npm-resolver/src/pickPackage.ts#L420-L431). When a fetch fails and a disk mirror exists, the stale-mirror pick (including null/None) is returned verbatim; the transport error is logged via `tracing::debug!`. Do not flag this as a bug.
Applied to files:
testing/registry-mock/src/index.ts
📚 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:
testing/registry-mock/src/index.ts
🔇 Additional comments (3)
testing/registry-mock/src/index.ts (3)
61-65: LGTM!
72-76: LGTM!
106-125: LGTM!
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
1 participant
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.
What
getIntegrity(in@pnpm/testing.registry-mock) now resolves a package version's tarball integrity from either the hosted location (<storage>/<pkg>/package.json) or pnpr's proxy cache (<storage>/.pnpr-cache/<pkg>/package.json), keeping the existing retry for the lazy/in-flight cache write.Why
#12195 (
feat(pnpr): separate the proxied upstream cache from published packages) moved proxied upstream packuments out of the storage root into a.pnpr-cachesubdirectory, but thegetIntegritytest helper still only read the hosted<storage>/<pkg>/package.jsonpath.As a result, every test that resolves integrity for a package uplinked from the real npm registry failed with
ENOENTandmainhas been red since:store/commands/test/store/storeAdd.ts→pnpm store add express@4.16.3store/commands/test/store/storePrune.ts→remove packages that are used by project that no longer exist(is-negative)Fixture/hosted packages (e.g.
@foo/no-deps) were unaffected because they live in the hosted store; only the upstream-proxied packages moved.Verification
express/is-negativepackuments (HTTP 200) and writes them to<storage>/.pnpr-cache/<pkg>/package.json, not<storage>/<pkg>/package.json.storeAdd.ts+storePrune.ts→ 18/18.@pnpm/testing.registry-mockisprivate: true, so no changeset is required.Written by an agent (Claude Code, claude-opus-4-8).
Summary by CodeRabbit
Bug Fixes
Chores