fix(resolver): fetch publish times on the paths that resolve without the full-packument cache - #607
Merged
Conversation
…ache The update/add/dedupe/audit resolver runs without the full-packument disk cache (dist-tag freshness rule). When needs_time was on and that cache dir was absent, the initial fetch and all three pick-site heal arms fell back to the abbreviated corgi document, whose missing time map bypasses the age cutoff at the pick site: nub update crossed minimumReleaseAge onto a publish minutes old and rewrote package.json onto it, while nub install had just refused the same version. Add RegistryClient::fetch_packument_with_time (uncached full fetch, time included) and use it in all four needs_time fallbacks. The regression test mocks npmjs's Accept-dependent shape (corgi without time, full with time) and pins the gated pick.
The regression test ran in lenient mode, where the lowest-satisfying fallback lands on the mature version regardless of the age wall, so it would have kept passing with the gate disabled. Strict is Nub's shipped default and the mode that actually exercises the wall. The corgi fixture also cleared `modified`, which npmjs does not: the abbreviated document carries it, and for an actively published package it is recent — so it cannot serve as the maturity proof a time-less document is otherwise allowed. Modelling that keeps the fixture honest.
…indow Failing closed on an undateable version means a registry that serves no `time` data now blocks the whole range. The age-gate error reported that as an ordinary cutoff miss: it listed years-old versions as "blocked by age gate" and offered a wider `minimumReleaseAge` as the remedy, which would never have helped. Split the headline and help on whether any satisfying version carried a publish time at all. A populated map with one hole stays an ordinary age gate — there the rest of the document dates fine and the hole is the anomaly worth reporting as one.
Failing closed on an undateable version is user-visible: a registry that serves no publish dates now blocks instead of quietly installing. The resolution-level window had no page of its own — only the build-script floor that consumes it — so dlx and the runner both pointed at the floor for a posture it does not describe. Give it a section and repoint them. Also fixes the trustPolicyExclude example. The npmrc reader compares keys verbatim against the settings registry, which carries trustPolicyExclude and trust-policy-exclude; the documented trustPolicyExclude[] form matches neither and was silently ignored.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
The fail-closed rows hold under the default minimumReleaseAgeStrict=true; lenient mode keeps an undateable version eligible. Stating the mode makes the table right for both.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Important
The fix is right and the regression test is genuine, but the new fetch reuses deprecate's read-modify-write helper, so the default nub update / dedupe / audit path loses the sonic-rs single-pass parse plus its registry diagnostics, and the new missing-times classification contradicts its own doc comment and the docs table it ships with.
Reviewed changes — the second half of the minimumReleaseAge fix: making publish times actually get fetched on the resolve paths that run without the full-packument disk cache, plus the error wording and docs the fail-closed path from #602 needs.
- Uncached full-packument fetch —
RegistryClient::fetch_packument_with_timewraps the pre-existingfetch_packument_json_fresh, replacing the abbreviatedfetch_packumentin the fourneeds_timefallbacks (one inresolve/fetch.rs, three primer-heal arms inresolve/driver.rs). - Split age-gate error — a new
publish_times_missingflag onAgeGateDetailsselects a distinct headline viaheadline()and an early-return branch informat_age_gate_help, so an undateable range no longer reports as a missed cutoff with "widen the window" as the remedy. - Regression test in strict mode —
minimum_release_age_holds_without_full_packument_cachestands up a mock registry that mirrors npmjs's Accept-dependent shape (corgi withouttime,modifiedretained) and pins the mature pick; two unit tests cover the classification and its mixed-map boundary. - Docs — a new
### Cooling windowsection in the install page, relinked fromrunner/dlx.mdxandrunner/index.mdx, and thetrustPolicyExclude[]=example corrected to the bare key form.
ℹ️ The added metadata volume for update / dedupe / audit is unmeasured
Those three commands build their resolver with cache_full_packuments: false, and on that path fetch_one_packument skips the disk-cache lookup entirely — so every package in the graph now pulls the full document from the network on every run, where it previously pulled the abbreviated one. Full packuments run one to three orders of magnitude larger than corgi documents for popular packages.
- Correctness requires the
timemap, so fetching more is the right trade. - What is missing is a number: the PR reports test and lint results but no before/after timing for
nub updateon a real tree. - The design question underneath is whether these commands want a full-packument cache with forced revalidation instead of an uncached fetch — that keeps dist-tag freshness while paying the transfer once.
Technical details
# Unmeasured metadata volume on the no-full-cache commands
## Affected sites
- `vendor/aube/crates/aube-resolver/src/resolve/fetch.rs:181-190` — when
`needs_time && full_cache_dir.is_none()`, `cached` is `Default::default()`,
so there is no disk lookup and no disk write on this path.
- `vendor/aube/crates/aube-resolver/src/resolve/fetch.rs:269` (new) — the
uncached full fetch that now runs for every package.
- `vendor/aube/crates/aube/src/commands/settings_context.rs:452` — the
`cache_full_packuments: false` posture shared by `update`, `dedupe`, `audit`.
## Required outcome
- A measured before/after for `nub update` (and ideally `audit`) on a
realistic tree, so the cost of the fix is on the record.
- An explicit decision recorded on whether an always-revalidate full-packument
cache should replace the uncached fetch for these commands.
## Open questions for the human
- Is the transfer cost acceptable as-is for `update` / `dedupe` / `audit`, or
should the dist-tag freshness rule be satisfied by conditional revalidation
against the full cache rather than by skipping the cache?ℹ️ Nitpicks
- The new table's "Blocked" rows hold under Nub's
minimumReleaseAgeStrict=truedefault, but flip to eligible underminimumReleaseAgeStrict=false— which the same block offers as a remedy three lines later. Worth naming strict mode in the table intro (site/content/docs/install/index.mdx:415-422). - The PR body lists
addamong the commands that resolve without the full-packument cache. Its manifest pass uses the cached variants and its install phase setscache_full_packuments: true, so onlyupdate/dedupe/auditreach the changed branches.
Claude Opus ｜ 𝕏
Comment on lines
+361
to
+366
| pub async fn fetch_packument_with_time(&self, name: &str) -> Result<Packument, Error> { | ||
| let value = self.fetch_packument_json_fresh(name).await?; | ||
| let packument: Packument = serde_json::from_value(value) | ||
| .map_err(|e| Error::Io(std::io::Error::new(std::io::ErrorKind::InvalidData, e)))?; | ||
| Ok(packument) | ||
| } |
Contributor
There was a problem hiding this comment.
fetch_packument_json_fresh was written for deprecate's read-modify-write, so it parses with resp.json() and this wrapper then walks the value a second time via from_value — on the full document, where the fetch_packument it replaces did one sonic-rs pass straight into Packument. Since minimumReleaseAge defaults to 24h, this is the ordinary per-package fetch for every nub update / dedupe / audit, and it also drops the packument_fetch / packument_http_attempt / packument_first_byte diag events that pm-perf-tracing reads.
Technical details
# Uncached full fetch bypasses the client's parse + diagnostics path
## Affected sites
- `vendor/aube/crates/aube-registry/src/client/packument.rs:361-366` (new) —
delegates to `fetch_packument_json_fresh`, then `serde_json::from_value`.
- `vendor/aube/crates/aube-registry/src/client/endpoints.rs:97` — `resp.json()`
(serde_json), no `http_body_read` / `json_parse_sonic_rs` events, no
`Priority` header, and a body-decode failure propagates through `?` instead
of retrying the way `fetch_packument`'s loop did (`packument.rs:649-660`).
- `vendor/aube/crates/aube-registry/src/client/packument.rs:558-560` — the
`NetworkMode::Offline` guard every sibling fetch carries and this one does
not. Not reachable today (`settings_context.rs:434` builds the client with
no `.with_network_mode(...)`, so `update`/`dedupe`/`audit` are always
`Online`), but it is a landmine on a `pub` method the moment offline support
reaches those commands.
- Reference for the shape to match: `packument.rs:255` and `packument.rs:493`,
both of which route the network body through `parse_full_response`.
## Required outcome
- One parse pass through `parse_full_response` (sonic-rs) with its diag
events, not `resp.json()` + `from_value`.
- The `NetworkMode::Offline` guard present, matching the sibling fetches.
## Suggested approach
Give `fetch_packument_with_time` its own body instead of borrowing
`deprecate`'s: offline guard, `packument_url(name)` +
`authed_get_for_package`, `Accept: PACKUMENT_FULL_ACCEPT`, `check_body_cap`
with `packument_max_bytes`, then `parse_full_response::<Packument>(resp)` —
typed in a single pass, since nothing on this path needs the raw `Value`.
That also restores retry-on-body-decode if it reuses `fetch_packument`'s loop
shape rather than `send_metadata_with_retry`.
Comment on lines
+287
to
+290
| let publish_times_missing = !gated.is_empty() | ||
| && !gated | ||
| .iter() | ||
| .any(|(_, ver)| packument.time.contains_key(ver)); |
Contributor
There was a problem hiding this comment.
Dates for other versions, none for this one → Blocked) both describe a packument-wide test. A registry that dates 1.x and 2.x but not 3.0.0, requested as ^3, therefore reports "the registry served no publish times" and "no time data for this package" — neither of which is true. version_clears_cutoff reasons packument-wide for exactly this reason (semver_util.rs:120-123).
Technical details
# `publish_times_missing` scope disagrees with its own contract
## Affected sites
- `vendor/aube/crates/aube-resolver/src/error.rs:287-290` (new) — the flag is
computed over `gated`, i.e. only versions satisfying `task.range`.
- `vendor/aube/crates/aube-resolver/src/error.rs:86-92` (new) — the doc
comment says "a populated map with a hole in it is NOT this case".
- `site/content/docs/install/index.mdx:420` (new) — the table row that
documents the hole case as an ordinary block.
- `vendor/aube/crates/aube-resolver/src/error.rs:341-343` (new) — the help
line "the registry served no `time` data for this package", which is the
strongest of the overstated claims.
- `vendor/aube/crates/aube-resolver/src/semver_util.rs:120-123` — the
`has_version_times` test the gate itself uses.
- `vendor/aube/crates/aube-resolver/src/tests.rs:244-271` (new) — the mixed-map
test uses range `^4`, which matches the dated version, so it never exercises
the shape where every dated version sits outside the range.
## Required outcome
- The message must not claim the registry served no dates when it served dates
for versions outside the requested range.
- The flag's computation, its doc comment, and the docs table row must describe
the same rule.
- A test covering the shape where the packument's dated versions all fall
outside the requested range.
## Suggested approach
Two coherent options, and they differ in user-visible behavior:
- Keep the range scope and fix the wording — headline and help say "no publish
time for any version matching this range" — then correct the doc comment and
the docs table row to match. This keeps the remedy list the branch offers
(`minimumReleaseAgeExclude` / `Strict=false` / `minimumReleaseAge=0`), which
is the correct set for an undateable in-range version, since widening the
window cannot help there.
- Or key the flag on `packument.time.keys().any(|k| k != "created" && k !=
"modified")`, mirroring `version_clears_cutoff`. That matches the documented
intent exactly, but sends the hole case back to the "loosen
`minimumReleaseAge`" help, which does not resolve it either.
## Open questions for the human
- Which contract is the intended one — range-scoped (fix the comment and the
docs row) or packument-scoped (fix the computation)?|
|
||
| Which manifest field grants permission tracks the inferred incumbent: pnpm projects use `pnpm.onlyBuiltDependencies` / `pnpm.allowBuilds`, Bun projects use `trustedDependencies`, and the neutral `allowBuilds` field plus `nub approve-builds` apply in any project. An explicit denial (`allowBuilds: { pkg: false }`, `neverBuiltDependencies`) always wins. A package that wants to build but isn't allowed is skipped, with `WARN_NUB_IGNORED_BUILD_SCRIPTS` naming it and `nub approve-builds` as the remedy. | ||
|
|
||
| ### Cooling window |
Contributor
There was a problem hiding this comment.
ℹ️ This section is nested under ## Lifecycle scripts (line 396), whose subject is which dependencies may run build scripts. The cooling window decides which version is installed at all, so runner/dlx.mdx and runner/index.mdx now deep-link a resolution-level posture into the build-script section — a milder version of the mislabeling this PR set out to fix. Promoting it to a top-level ## (or grouping it with the other resolution-time gates) keeps the anchor honest.
Contributor
There was a problem hiding this comment.
ℹ️ The strict-mode scoping is right; one clause about what loosening actually does is not.
Reviewed changes — the single documentation commit pushed since the previous review at a686c69.
- Scope the cooling-window table to strict mode — names
minimumReleaseAgeStrict=trueas the default the table assumes and adds a sentence describing what loosening it changes.
Claude Opus ｜ 𝕏
| | No per-version dates, document older than the window | Allowed — the document's own timestamp bounds every version in it | | ||
| | No per-version dates, document newer than the window | Blocked | | ||
|
|
||
| Loosening strictness reverses both blocked rows: an undateable version becomes eligible again, and the lowest satisfying one is picked. |
Contributor
There was a problem hiding this comment.
ℹ️ The first clause is right, the second is not: under minimumReleaseAgeStrict=false an undateable version clears the cutoff outright, so it competes in the ordinary pick — dist-tags.latest first, then the highest satisfying version. The lowest-satisfying fallback only runs when every satisfying version is dated and too new, which is a different scenario from the two rows this sentence describes.
Technical details
# Lenient mode does not route undateable versions to the lowest-satisfying fallback
## Affected sites
- `site/content/docs/install/index.mdx:424` (new) — "and the lowest satisfying
one is picked".
- `site/content/docs/install/index.mdx:430` (new) — the same claim as the
inline comment on `minimumReleaseAgeStrict=false`, in a block whose framing
is the no-dates-at-all registry.
## Evidence
- `vendor/aube/crates/aube-resolver/src/semver_util.rs:119-127` — for a version
with no `time` entry, `modified_proves_maturity || !strict`, so lenient mode
returns `true`: the version clears the cutoff.
- `vendor/aube/crates/aube-resolver/src/semver_util.rs:252-260, 289-295` — a
version that clears the cutoff is eligible for the `dist-tags.latest`
preference and the highest-satisfying `best` scan.
- `vendor/aube/crates/aube-resolver/src/semver_util.rs:316-321` —
`fallback_lowest` is consulted only when `best` is `None`, i.e. when every
satisfying version failed the cutoff.
## Required outcome
- Both spots state that loosening strictness makes an undateable version
eligible for the normal pick (the gate becomes a no-op for that package),
and reserve the lowest-satisfying wording for the case where every
satisfying version is dated and too new.
Contributor
Author
|
Shipped in v0.7.0: https://github.com/nubjs/nub/releases/tag/v0.7.0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Completes the
minimumReleaseAgefix. #602 made an undeterminable publish age fail closed at the pick site; this lands the other half — making sure the publish times are actually fetched — plus the diagnostic and docs the new fail-closed path needs.Why both halves are required
update/add/dedupe/auditresolve without the full-packument disk cache (the dist-tag freshness rule,settings_context.rs:452), so theneeds_timefallback fetched npmjs's abbreviated document, which carries notimemap. Before #602 that silently disabled the gate. After #602 it hard-fails instead: no version can be dated, npmjs's corgimodifiedis recent for any actively published package, so every candidate is blocked and strict mode aborts.Verified on the merged tree — the regression test in strict mode, which is Nub's shipped default:
Both versions gated, including one published in 2024. Fetching the full document fixes it.
Contents
RegistryClient::fetch_packument_with_time, the uncached full-packument sibling, wired into all fourneeds_timefallbacks. Carried over from fix(resolver): enforce minimumReleaseAge without the full-packument cache #582 with authorship intact; that PR predates fix(resolver): fail closed when publish time can't prove a version's age #602 and can't merge as-is against it.modified, which npmjs does not (verified against registry.npmjs.org: abbreviated metadata carriesmodified, nevertime), and for an active package that timestamp is recent — so it must not stand in as the maturity proof a time-less document otherwise gets.trustPolicyExclude[]=example: the npmrc reader compares keys verbatim against the settings registry, which carriestrustPolicyExcludeandtrust-policy-exclude, so the bracket form was silently ignored.Verification
290 resolver tests pass. Root
cargo fmt --checkclean andcargo clippy --all-targets --all-features -- -D warningsclean. Vendored aube carries 36 pre-existing rustfmt diffs onmain; this branch has 35 — it fixes one and adds none.Refs #581. Supersedes #582.