fix(resolver): tolerate a non-integer dist.unpackedSize - #646
Conversation
`Dist::unpacked_size` was the one strict field left on the packument
version metadata. A registry serving `"unpackedSize": {...}` where a
byte count belongs failed `serde_json::from_value::<Packument>`, which
surfaced as
ERR_NUB_REGISTRY_ERROR
× failed to resolve dependencies
╰─▶ registry error for <pkg>: I/O error: invalid type: map, expected u64
and aborted the entire install. Packument fetches run concurrently, so
the package named in the message is whichever fetch lost the race, not
the one carrying the odd value — the error points at a different
package on each run.
The field is cosmetic: it is the best-effort install-size estimate
behind the progress bar's `4.2 MB / ~13.8 MB` segment. Every sibling
field on the same struct already degrades instead of failing
(`non_string_tolerant_map`, `string_or_seq`, `license_string`,
`npm_user_tolerant`, `bin_map`, `engines_tolerant`); this one did not,
so a wrong pixel cost a dead install. Deserialize it through a tolerant
visitor that keeps a non-negative integer and drops every other shape
to `None`.
Measured against pnpm on a local registry serving an object-valued
`unpackedSize`: pnpm resolves the packument and proceeds to fetch;
aube was alone in failing the resolve.
`Dist` is shared by `VersionMetadata` and `VersionMetadataRaw`, so the
mirror-the-attributes-byte-for-byte invariant on the latter needs no
second edit.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ One doc-comment correction inline, plus two scope notes. The deserializer itself checks out.
Reviewed changes — the single commit making dist.unpackedSize tolerant of non-integer JSON shapes.
- Tolerant
unpackedSizedeserializer —Dist::unpacked_sizegainsdeserialize_with = "unpacked_size_tolerant". The new visitor keepsvisit_u64, keepsvisit_i64only whereu64::try_fromsucceeds, and drops bool / f64 / null / seq / map / string toNonethrough the file's existingvisit_seq_to!/visit_map_to!/visit_strings_to!macros. - Parity property test —
unpacked_size_matches_referencepins the visitor against aValue::as_u64()oracle overarb_json(), with aparse_dist_withwrapper becausedistis the only tolerant field nested a level below the version entry.
Two things I checked that hold up. The visitor is exhaustive over every JSON value kind and agrees with the oracle on every shape the generator produces, and the test can genuinely fail with the bug present — a strict Option<u64> panics at parse_version_with's .expect("version parses") on the map case. The test also runs in CI despite vendor/aube being excluded from the root workspace, via aube-parity.yml's cargo test --workspace inside vendor/aube. Separately, production decodes packuments with sonic_rs::from_slice while the test drives serde_json::from_value; on the pinned versions (sonic-rs 0.5.8, serde_json 1.0.145, neither with arbitrary_precision) both route non-negative integers to visit_u64, negative to visit_i64, and floats plus anything over u64::MAX to visit_f64 under deserialize_any, and neither reaches visit_u128/visit_i128 from that path — so the omitted 128-bit handlers are dead code here, not a hole, and the serde_json-based parity assertion does transfer to production.
ℹ️ The same whole-install abort is still reachable through dist.integrity and dist.shasum
The motivating scenario is a mirror that rewrites dist sub-fields, and two of the other sub-fields on that same object have no tolerant handling at all: "integrity": {…} or "shasum": 123 aborts the packument exactly as unpackedSize did, with the same misattributed package name. Unlike unpackedSize these gate tarball verification, so silently degrading them to None is a real decision rather than an obvious extension — worth stating the position either way so the next reader knows the strictness is deliberate.
Technical details
# `dist` sub-fields other than `unpackedSize` remain strict
## Affected sites
- `vendor/aube/crates/aube-registry/src/lib.rs:441` — `pub integrity: Option<String>`, no serde attributes. `"integrity": {"algo": "sha512"}` → `invalid type: map, expected a string`, failing the whole packument.
- `vendor/aube/crates/aube-registry/src/lib.rs:442` — `pub shasum: Option<String>`, same shape. `"shasum": 123456` → `invalid type: integer, expected a string`.
- `vendor/aube/crates/aube-registry/src/lib.rs:458` — `#[serde(default)] pub attestations: Option<Attestations>`; the derived impl accepts only an object or null, so `"attestations": []` or `"attestations": "none"` aborts.
## Required outcome
- A reader of `unpacked_size_tolerant` can tell whether the remaining strict `dist` fields are strict on purpose. Either make the position explicit in the comment, or make them tolerant if that is the intent.
## Open questions for the human
- `integrity` / `shasum` feed tarball verification. Is dropping a malformed value to `None` acceptable there, or is a hard parse error the correct outcome for a field that gates integrity checking? That answer determines whether this is a comment change or a code change.ℹ️ The misattributed package name in the error is untouched
The PR description calls out that the failure "reads as a different random package each run" because packument fetches are concurrent. That comes from the decode error carrying no package context, and this diff doesn't change it — so the next off-spec field in this class will be just as hard to attribute to the package actually at fault. Reasonable as a follow-up rather than part of this fix, but worth noting it isn't addressed here.
Technical details
# Packument decode errors carry no package context
## Affected sites
- `vendor/aube/crates/aube-registry/src/client/parse.rs:25-26` — `sonic_rs::from_slice::<T>(&bytes).map_err(|e| Error::Io(std::io::Error::new(ErrorKind::InvalidData, e)))`. The wrapped error holds only serde's message (`invalid type: map, expected u64`) plus a byte offset; nothing identifies which package's packument was being decoded.
- `vendor/aube/crates/aube-registry/src/client/packument.rs:643` and `:855` — the two `parse_full_response::<Packument>` call sites. Both have the package name in scope.
## Required outcome
- A packument decode failure names the package whose packument failed to decode, independent of which concurrent fetch surfaces the error first.Claude Opus ｜ 𝕏
… stays strict
The previous comment claimed `unpacked_size` was the last strict field
on `Dist`. It is not: `integrity` and `shasum` are plain
`Option<String>`, and `attestations` rejects a string. Measured on a
registry serving each shape — `"integrity": {...}`, `"shasum": 123456`
and `"attestations": "none"` each still abort the packument parse
(`"attestations": []` is tolerated).
Those are strict on purpose and now say so at the field. Degrading a
malformed `integrity`/`shasum` to `None` means installing a tarball
with no verification, and a mirror able to rewrite the field is
precisely the one whose bytes must not be trusted unchecked — so the
loud failure is the intended outcome there, unlike the progress-bar
estimate `unpacked_size` feeds.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — 9791655, a doc-comment-only follow-up to the prior review; the deserializer and the property test are byte-identical to 8cd2cf6.
- Dropped the "last strict field" overclaim —
unpacked_size_tolerant's rationale no longer asserts that every otherDistfield already had a tolerant deserializer, and now points at the strictness note instead. - Recorded why
integrity/shasumstay strict — a new note onDist::integritystates that a malformed value aborting the parse is the intended outcome, becauseNonethere means importing the tarball unverified.
I checked the new note's claim rather than taking it on faith, and it holds. strict-store-integrity defaults to false for pnpm parity (install/settings.rs:375-383), so a None integrity takes the warn-and-import-unverified path at install/lifecycle.rs:874-880; driver.rs:1339-1343 derives the locked integrity from dist.integrity with a shasum_to_sri(dist.shasum) fallback, so the two fields jointly gate verification and dropping both really would mean unverified bytes. Both are still plain Option<String> with no serde attributes, so the "aborts the packument parse" half is accurate too. The [non_string_tolerant_map] and [Dist] intra-doc links resolve (lib.rs:114, same module) and unpacked_size_tolerant is private, so there's no private_intra_doc_links exposure.
One note carried forward rather than re-raised: the misattributed package name in the decode error is still untouched (client/parse.rs:25-26 wraps the error with no package context, while both parse_full_response::<Packument> call sites have the name in scope). That was flagged as an optional follow-up on the prior review and this diff doesn't claim to fix it.
Claude Opus ｜ 𝕏
|
Shipped in v0.7.0: https://github.com/nubjs/nub/releases/tag/v0.7.0 |
A registry serving
"unpackedSize": {...}aborted the whole install withERR_NUB_REGISTRY_ERROR: invalid type: map, expected u64. Packument fetches run concurrently, so the name in the message is whichever fetch lost the race — it reads as a different random package each run.unpacked_sizeonly feeds the progress bar's size estimate, so strictness cost a dead install for a wrong pixel. Now parsed through a tolerant visitor: integers kept, every other shape dropped toNone.integrity/shasumstay strict on purpose; a note at the field records why.Against a local registry serving that shape: v0.6.0 dies at the parse; this branch and pnpm 10 both resolve and reach the tarball fetch.