Description

pick_package_from_meta returns Ok(None) immediately when the chosen version’s manifest fails to
hydrate, instead of skipping it and retrying other candidates. This can cause npm resolution to
return None even when other satisfying versions exist.

Code

pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs[R229-240]

let Some(version) = picked_version else { return Ok(None) };
-    let Some(manifest) = meta_ref.versions.get(&version).cloned() else { return Ok(None) };
-    let mut manifest = manifest;
-    if !meta_ref.name.is_empty() {
+    let Some(manifest) = meta_ref.versions.get(&version) else { return Ok(None) };
+    if !meta_ref.name.is_empty() && manifest.name != meta_ref.name {
    // GitHub registry quirk: a scoped package can be published as
    // `@owner/foo` while the per-version `name` is just `foo`.
    // Match upstream's shim that pins the manifest name to the
    // packument-level name.
-        manifest.name.clone_from(&meta_ref.name);
+        let mut pinned = (*manifest).clone();
+        pinned.name.clone_from(&meta_ref.name);
+        return Ok(Some(Arc::new(pinned)));
}
Ok(Some(manifest))

Evidence

The module contract says undecodable versions should behave as absent, but pick_package_from_meta
treats a hydration failure as terminal Ok(None). npm_resolver then treats picked_package == None
as “no resolution” and returns Ok(None).

pacquet/crates/registry/src/package_versions.rs[13-16]
pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs[216-241]
pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs[393-401]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`pick_package_from_meta` picks a version *string* and then hydrates it via `meta_ref.versions.get(&version)`. Under lazy hydration, a malformed manifest yields `None`, and the function returns `Ok(None)` rather than treating that specific version as absent and attempting the next-best candidate.
### Issue Context
The lazy-hydration contract states undecodable fragments should behave as if absent. The resolver treats `picked_package: None` as “no resolution”, so an undecodable top candidate can incorrectly make the whole dependency unresolvable.
### Fix Focus Areas
- pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs[216-241]
- pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs[393-401]
- pacquet/crates/registry/src/package_versions.rs[13-16]
### Suggested fix
For range picks, loop: pick the best candidate string; attempt hydration; if hydration fails, record the version in an exclusion set and re-run the picker excluding those versions, until a hydratable manifest is found or no candidates remain. (Optional: `tracing::warn` once per excluded version to aid debugging.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

filter_pkg_metadata_by_publish_date may rewrite a dist-tag to a version key whose manifest cannot
decode, because it chooses candidates from keys/contains_key without requiring hydration success.
This can produce filtered metadata whose dist-tags later fail to resolve to a manifest.

Code

pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs[R410-419]

            Some((ref best, best_raw)) => {
                let best_deprecated = versions_within_date
                    .get(best_raw)
-                        .and_then(|manifest| manifest.deprecated.as_ref())
-                        .is_some();
+                        .is_some_and(|manifest| manifest.deprecated.is_some());
                let candidate_deprecated = versions_within_date
                    .get(candidate_raw)
-                        .and_then(|manifest| manifest.deprecated.as_ref())
-                        .is_some();
+                        .is_some_and(|manifest| manifest.deprecated.is_some());
                let candidate_wins = (candidate > *best
                    && best_deprecated == candidate_deprecated)
                    || (best_deprecated && !candidate_deprecated);

Evidence

Key scans can include versions whose manifests fail to decode (contains_key true while get is
None). The publish-date filter’s tag rewrite chooses candidates from keys and may select a version
that can’t be hydrated later.

pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs[378-429]
pacquet/crates/registry/src/package_versions/tests.rs[44-48]
pacquet/crates/registry/src/package_versions.rs[82-89]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`filter_pkg_metadata_by_publish_date` selects dist-tag replacements by iterating `versions_within_date.keys()` and can choose a candidate even if its manifest fragment is undecodable. Because `contains_key`/`keys` include undecodable entries, tags can end up pointing at a version that later yields `PackageVersions::get(...) == None`.
### Issue Context
Undecodable fragments are intended to behave as absent, so dist-tag rewrite should avoid choosing them as winners.
### Fix Focus Areas
- pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs[378-429]
- pacquet/crates/registry/src/package_versions.rs[82-89]
- pacquet/crates/registry/src/package_versions/tests.rs[44-48]
### Suggested fix
When evaluating candidates for `best_version`, require `versions_within_date.get(candidate_raw).is_some()` before (a) seeding `best_version = Some(...)` and (b) considering a candidate as a winner. Also consider changing the early `contains_key(version)` tag preservation check to verify `get(version).is_some()` if you want “absent on decode fail” semantics for tags too.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

Package::latest uses expect() on PackageVersions::get, but with lazy hydration invalid manifests no
longer fail whole-packument deserialization and instead return None on access. If dist-tags.latest
points at an undecodable manifest, latest() will panic.

Code

pacquet/crates/registry/src/package.rs[R164-168]

+    pub fn latest(&self) -> Arc<PackageVersion> {
    let version =
        self.dist_tags.get("latest").expect("latest tag is expected but not found for package");
-        self.versions.get(version).unwrap()
+        self.versions.get(version).expect("manifest of the latest version should decode")
}

Evidence

PackageVersions::get returns None on decode failure, and the module explicitly allows
undecodable fragments; latest() currently panics if that happens for the latest dist-tag target.

pacquet/crates/registry/src/package_versions.rs[13-16]
pacquet/crates/registry/src/package_versions.rs[82-89]
pacquet/crates/registry/src/package.rs[163-168]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Package::latest` calls `self.versions.get(version).expect(...)`. With lazy hydration, `get` can return `None` for undecodable fragments, so `latest()` can now panic at runtime.
### Issue Context
The `PackageVersions` contract allows undecodable fragments and treats them as absent; `latest()` should either propagate that absence or fail in a controlled, non-panicking way.
### Fix Focus Areas
- pacquet/crates/registry/src/package.rs[163-168]
- pacquet/crates/registry/src/package_versions.rs[82-89]
### Suggested fix
Change `latest()` to return `Option<Arc<PackageVersion>>` (or `Result<Arc<PackageVersion>, ...>`), and update callers/tests accordingly. If the API must remain infallible, consider selecting an alternative (e.g., the highest hydratable version) rather than panicking.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

detect_strongest_trust_evidence_before iterates via meta.versions.iter(), but
PackageVersions::iter() drops any version whose manifest fails to hydrate. This can miss prior
trust evidence and lower prior_rank, allowing a trust downgrade to pass undetected when some prior
version fragments are undecodable.

Code

pacquet/crates/resolving-npm-resolver/src/trust_checks.rs[224]

+    for (version, manifest) in meta.versions.iter() {

Evidence

The trust-downgrade logic depends on scanning all prior versions to compute the strongest prior
trust evidence; after this PR it uses meta.versions.iter(). But PackageVersions::iter() is
explicitly defined to skip undecodable fragments (via filter_map over hydrate(version)?), so any
prior version whose manifest fails to decode is silently omitted from the scan, potentially lowering
prior_rank.

pacquet/crates/resolving-npm-resolver/src/trust_checks.rs[164-189]
pacquet/crates/resolving-npm-resolver/src/trust_checks.rs[214-259]
pacquet/crates/registry/src/package_versions.rs[108-165]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The trust-downgrade scan walks prior versions using `meta.versions.iter()`, but the new lazy-hydration iterator skips undecodable manifests. That means earlier versions with trust evidence can be silently ignored, reducing the strongest prior trust rank and potentially allowing a downgrade to pass.

### Issue Context
- `PackageVersions::hydrate()` returns `None` on decode error.
- `PackageVersions::iter()` uses `filter_map(... hydrate(version)? ...)`, so decode failures are indistinguishable from absence and are silently dropped.
- Trust downgrade detection explicitly aims to avoid skipping data in ways that could mask earlier evidence.

### Fix Focus Areas
- pacquet/crates/resolving-npm-resolver/src/trust_checks.rs[164-258]
- pacquet/crates/registry/src/package_versions.rs[108-165]

### Suggested fix
In `detect_strongest_trust_evidence_before`, iterate version strings (e.g., `meta.versions.keys()`) and attempt hydration per version.
- If `meta.versions.get(version)` is `None` **and** `meta.versions.contains_key(version)` is `true`, treat that as a *decode failure* and return an error (e.g., `TrustViolation::TrustCheckFailed { reason: ... }`) rather than skipping it.
- This likely requires changing `detect_strongest_trust_evidence_before` to return `Result<Option<TrustEvidence>, TrustViolation>` and updating its caller in `fail_if_trust_downgraded` to propagate the error.

This makes the trust check fail closed when it cannot reliably determine prior trust evidence.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

read_mirror_headers() allocates a Vec sized from the on-disk headers_len field before
validating it against the actual file size, so a malformed/corrupted mirror can trigger an OOM
during load_meta_headers(_async) and crash warm resolves. This is reachable because
fetch_full_metadata_cached() calls load_meta_headers_async() on the hot path before issuing
requests.

Code

pacquet/crates/resolving-npm-resolver/src/mirror.rs[R284-310]

+fn read_mirror_headers(file: &mut File) -> Option<MetaHeaders> {
+    // Magic + two decimal lengths fit well inside this; the headers
+    // record is ~100 bytes of etag + timestamp.
+    let mut buf = [0u8; 1024];
+    let mut filled = 0usize;
+    while filled < buf.len() {
+        let n = file.read(&mut buf[filled..]).ok()?;
+        if n == 0 {
+            break;
+        }
+        filled += n;
+    }
+    let chunk = &buf[..filled];
+    let newline = chunk.iter().position(|&byte| byte == b'\n')?;
+    let line = std::str::from_utf8(&chunk[..newline]).ok()?;
+    let (headers_len, _) = parse_mirror_magic(line)?;
+    let headers_start = newline + 1;
+    let headers_end = headers_start.checked_add(headers_len)?;
+    let headers_json: std::borrow::Cow<'_, [u8]> = if headers_end <= chunk.len() {
+        std::borrow::Cow::Borrowed(&chunk[headers_start..headers_end])
+    } else {
+        // Headers record larger than the probe buffer — read the rest.
+        let mut rest = vec![0u8; headers_end - chunk.len()];
+        file.read_exact(&mut rest).ok()?;
+        let mut whole = chunk[headers_start..].to_vec();
+        whole.extend_from_slice(&rest);
+        std::borrow::Cow::Owned(whole)

Evidence

The mirror parser computes headers_end from the parsed headers_len and allocates rest of size
headers_end - chunk.len() without checking against file size or any cap; the header read is
invoked from the cached metadata fetcher before conditional GETs.

pacquet/crates/resolving-npm-resolver/src/mirror.rs[284-313]
pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached.rs[120-136]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`read_mirror_headers()` trusts the mirror file’s declared `headers_len` and may allocate an arbitrarily large buffer (`vec![0u8; ...]`) before confirming the file actually contains that many bytes. A corrupted/malicious cache entry can therefore cause excessive allocation/OOM and crash the resolver.
## Issue Context
This is on the warm path: `fetch_full_metadata_cached()` calls `load_meta_headers_async()` which delegates to `load_meta_headers()`/`read_mirror_headers()`.
## Fix Focus Areas
- pacquet/crates/resolving-npm-resolver/src/mirror.rs[274-313]
- pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached.rs[120-136]
### Suggested fix approach
- Before allocating for `headers_len`, validate it is within a reasonable maximum (e.g., a small constant like 8–64 KiB).
- Also validate `headers_len` against the file size (`file.metadata()?.len()`) and return `None` early if the declared record would exceed file length.
- Avoid allocating based solely on declared lengths; prefer bounded incremental reads or a `take()`-limited reader.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

Package::pinned_version picks the highest satisfying version string and then hydrates only that one;
if hydration fails, it returns None even when lower satisfying versions exist. This breaks the
intended “undecodable behaves as absent” semantics and can incorrectly report no match for a valid
range.

Code

pacquet/crates/registry/src/package.rs[R146-160]

+    pub fn pinned_version(&self, version_range: &str) -> Option<Arc<PackageVersion>> {
     let range: node_semver::Range = version_range.parse().unwrap(); // TODO: this step should have happened in PackageManifest
-        let mut satisfied_versions = self
+        // Match on the version *strings* so only the winning manifest
+        // hydrates from its raw fragment.
+        let highest = self
         .versions
-            .values()
-            .filter(|version| version.version.satisfies(&range))
-            .collect::<Vec<&PackageVersion>>();
-
-        satisfied_versions.sort_by(|a, b| a.version.partial_cmp(&b.version).unwrap());
-
-        // Optimization opportunity:
-        // We can store this in a cache to remove filter operation and make this a O(1) operation.
-        satisfied_versions.last().copied()
+            .keys()
+            .filter_map(|key| {
+                key.parse::<node_semver::Version>()
+                    .ok()
+                    .filter(|version| version.satisfies(&range))
+                    .map(|version| (version, key))
+            })
+            .max_by(|(left, _), (right, _)| left.partial_cmp(right).unwrap())?;
+        self.versions.get(highest.1)

Evidence

PackageVersions defines decode failure as “absent” and get returns None on decode failure; tests
also show a version can be present in key scans but absent on hydration. pinned_version currently
selects one highest candidate and does not retry when get returns None.

pacquet/crates/registry/src/package_versions.rs[13-16]
pacquet/crates/registry/src/package_versions.rs[82-89]
pacquet/crates/registry/src/package_versions/tests.rs[44-48]
pacquet/crates/registry/src/package.rs[145-161]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Package::pinned_version` selects a single “highest” satisfying version string and then calls `self.versions.get(...)`. With lazy hydration, `get` returns `None` when the manifest fragment is undecodable, causing `pinned_version` to incorrectly return `None` even if other satisfying versions are present.
### Issue Context
`PackageVersions` explicitly defines undecodable fragments as behaving “as if the version were absent”, so callers that implement picking should treat hydration failures as a signal to try the next candidate.
### Fix Focus Areas
- pacquet/crates/registry/src/package.rs[145-161]
- pacquet/crates/registry/src/package_versions.rs[82-89]
### Suggested fix
Compute satisfying candidates in descending semver order and attempt hydration in that order, returning the first `Some(Arc<PackageVersion>)`. If the best candidate fails to hydrate, continue to the next-best rather than returning `None` immediately.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

save_meta_indexed() builds fragment_bytes containing all version fragments and then copies it
again into a second bytes buffer (bytes.extend_from_slice(&fragment_bytes)), doubling peak
memory for large packuments during mirror writes. This can significantly increase allocator pressure
and risk OOM when caching very large packuments.

Code

pacquet/crates/resolving-npm-resolver/src/mirror.rs[R238-267]

+    let mut fragment_bytes = Vec::new();
+    let mut spans = Vec::with_capacity(meta.versions.len());
+    for (version, json) in meta.versions.fragments() {
+        let offset = fragment_bytes.len() as u64;
+        let len = u32::try_from(json.len()).unwrap_or(u32::MAX);
+        if len as usize != json.len() {
+            // A single >4 GiB version manifest is not a thing the npm
+            // registry produces; skip it rather than corrupt the index.
+            continue;
+        }
+        fragment_bytes.extend_from_slice(json.as_bytes());
+        spans.push((version.clone(), offset, len));
+    }
+
+    let index = serde_json::to_string(&MirrorIndex {
+        name: meta.name.clone(),
+        dist_tags: meta.dist_tags.clone(),
+        time: meta.time.clone(),
+        homepage: meta.homepage.clone(),
+        versions: spans,
+    })
+    .map_err(|error| SaveMetaError::Encode(EncodeMetaError(error)))?;
+
+    let mut contents = String::with_capacity(headers.len() + index.len() + 64);
+    let _ = writeln!(contents, "{MIRROR_MAGIC} {} {}", headers.len(), index.len());
+    contents.push_str(&headers);
+    contents.push_str(&index);
+    let mut bytes = contents.into_bytes();
+    bytes.extend_from_slice(&fragment_bytes);
+    save_meta(pkg_mirror, &bytes)

Evidence

The function first accumulates all fragment bytes into fragment_bytes, then constructs bytes
from the header/index string and appends fragment_bytes via copying; save_meta currently
requires a full &[u8], encouraging this extra buffering.

pacquet/crates/resolving-npm-resolver/src/mirror.rs[227-268]
pacquet/crates/resolving-npm-resolver/src/mirror.rs[417-437]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`save_meta_indexed()` materializes the fragment section twice in memory: once in `fragment_bytes`, then again when copying into `bytes` for `save_meta()`. For very large packuments, this amplifies peak memory usage unnecessarily.
## Issue Context
The new mirror format is meant to improve warm-load performance; mirror writes happen on 200 responses and on explicit persist paths.
## Fix Focus Areas
- pacquet/crates/resolving-npm-resolver/src/mirror.rs[227-268]
- pacquet/crates/resolving-npm-resolver/src/mirror.rs[417-443]
### Suggested fix approach
- Avoid the double buffer by either:
- Writing directly to the temp file in `save_meta` (change `save_meta` to accept a closure/writer and stream headers/index/fragments sequentially), or
- Building a single `Vec<u8>`: push the magic line + headers + index into one `Vec`, `reserve_exact` for fragment total size (if computed) and append fragments directly without an intermediate `fragment_bytes`.
- Keep the existing atomic temp+rename semantics.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

In pick_version_by_version_range, the deprecated-version fallback builds the non-deprecated
candidate list using opts.meta.versions.iter(), which hydrates every version fragment into a full
PackageVersion. When the initially selected max satisfying version is deprecated, this can trigger
full-packument hydration work and negate the lazy-hydration performance win for that pick.

Code

pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs[R305-310]

  if let Some(ref picked) = max_pick {
      let picked_meta = opts.meta.versions.get(picked);
-        let picked_is_deprecated =
-            picked_meta.and_then(|version| version.deprecated.as_ref()).is_some();
+        let picked_is_deprecated = picked_meta.is_some_and(|version| version.deprecated.is_some());
      if picked_is_deprecated && all_versions.len() > 1 {
          let non_deprecated: Vec<&str> = opts
              .meta

Evidence

The deprecated fallback computes non_deprecated by calling opts.meta.versions.iter(), and
PackageVersions::iter() is defined to hydrate every fragment; therefore, this fallback branch
forces full hydration across all versions when it triggers.

pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs[298-316]
pacquet/crates/registry/src/package_versions.rs[112-118]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`pick_version_by_version_range` uses `opts.meta.versions.iter()` inside the deprecated-fallback branch, which now hydrates *all* version manifests. This is avoidable because the fallback only needs to find the highest satisfying **non-deprecated** version.
### Issue Context
- `PackageVersions::iter()` hydrates every raw fragment (and is documented as suitable only for cold paths).
- The deprecated-fallback branch can run during normal picking when the best satisfying version is deprecated.
### Fix Focus Areas
- pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs[298-320]
- pacquet/crates/registry/src/package_versions.rs[112-118]
### Suggested fix approach
- Replace the `versions.iter().filter(|(_, manifest)| manifest.deprecated.is_none())...` full scan with a descending semver scan over candidate version **strings**.
- Hydrate only as needed:
- If `max_pick` is deprecated, iterate versions from highest to lowest (restricted to versions satisfying the range) and call `meta.versions.get(candidate)` until you find one with `deprecated.is_none()`.
- This should hydrate only a handful of candidates in typical cases rather than every version.
- Treat undecodable fragments (`get` returns `None`) as “absent”/skip when searching for a non-deprecated candidate, consistent with the PR’s semantics.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

project_abbreviated_meta uses meta.versions.iter() to build a version -> dist.tarball map,
which forces hydration of every version into a full PackageVersion even though only dist.tarball
is needed. For large packuments, this reintroduces substantial CPU/memory cost into the verifier
path and undermines the lazy-hydration benefit.

Code

pacquet/crates/registry/src/package_versions.rs[R112-118]

+    /// Iterate `(version, manifest)` pairs, hydrating every fragment.
+    /// Undecodable fragments are skipped. Full walks defeat the lazy
+    /// representation, so this belongs only on cold paths (the trust
+    /// verifier's history scan, tests).
+    pub fn iter(&self) -> impl Iterator<Item = (&String, Arc<PackageVersion>)> {
+        self.slots.iter().filter_map(|(version, slot)| Some((version, slot.hydrate(version)?)))
+    }

Evidence

PackageVersions::iter() is implemented t...

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 10b35404-de7d-47d2-9a13-de2b1a484b64

Reviewing files that changed from the base of the PR and between ae28135 and f6b75fc.

• pacquet/crates/resolving-npm-resolver/src/trust_checks.rs
• pacquet/crates/resolving-npm-resolver/src/trust_checks/tests.rs

• Create stacked PR
• Commit on current branch

• Create PR with unit tests
• Commit unit tests in branch manifest-lazy-hydration

• ➕ Avoids enabling serde_json raw_value feature
• ➕ Simpler custom Deserialize implementation
• ➖ Still eagerly builds full Value trees during packument parse (loses most CPU/allocation win)
• ➖ Does not preserve raw bytes for exact mirror round-trips without extra work

• ➕ Minimizes initial parse to just version strings
• ➕ Could avoid holding raw fragments for all versions in memory
• ➖ Requires additional network/filesystem fetches or a different packument wire format
• ➖ Breaks current assumption that one packument response contains all version manifests

• ➕ Can bound memory across many packages and evict cold manifests
• ➕ Potentially reduces per-packument retained state
• ➖ Adds concurrency complexity and tuning (capacity, eviction policy)
• ➖ Per-slot OnceLock already matches access patterns and keeps lookup O(1) with minimal contention