fix(resolver): skip optional deps whose version pick finds no match - #604
Conversation
An optional dependency whose range matched no version aborted the whole install instead of being skipped. The fetch-failure and platform-mismatch paths already skip optional deps; the version-pick path did not. This surfaces on packages shipping one optional binding per platform. rolldown declares 15 `@rolldown/binding-*` optionals, and the version pick runs before the os/cpu filter, so a binding the host will never install can fail the install when the registry does not carry it — a mirror holding only the bindings its own users have pulled, typically. The age-gate path stays fatal: a version withheld by minimumReleaseAge is a supply-chain signal the user opted into, not a package the registry lacks. Refs #580
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.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No blocking issues — two informational notes inline. The skip itself does byte-identical bookkeeping to the existing platform-mismatch path.
Reviewed changes — makes a NoMatch version pick non-fatal for optional dependencies in the vendored resolver, so a registry that carries a package but not the requested version can no longer abort an install over a dep the platform filter was about to drop anyway.
- Guarded skip arm in the version-pick loop — a new
PickResult::NoMatch if task.dep_type == DepType::Optionalarm indriver.rswarns, records the skip for root deps, and returnsOk(()); the generic fatalNoMatcharm below it is unchanged, so required deps still hard-fail. - New warning code —
WARN_AUBE_SKIPPED_OPTIONAL_NO_MATCHING_VERSIONregistered inwarnings.rsunder the resolver category with no exit code. - Two resolver tests — one asserting an optional dep with no matching version resolves cleanly, one asserting the same shape in
dependenciesstill errors withError::NoMatch.
I traced the early return against every structure the rest of process_task populates. It returns before the graph node, the parent dep wiring, the importer direct-dep list, visited, resolved_versions, and the transitive enqueue, so nothing downstream can reference the skipped name. A later task for the same package as a required dep still resolves normally, since resolved_versions is never populated and sibling dedupe can't reuse it. The age-gate arm staying fatal is a deliberate, documented divergence and I did not challenge it.
ℹ️ Nitpicks
vendor/aube/docs/error-codes.data.jsonis generated from theALLregistry and wasn't regenerated for the new warning, so the vendored tree's code table is now one entry behind. Nothing in nub's CI gates it.
Claude Opus ｜ 𝕏
| if task.is_root | ||
| && let Some(spec) = task.original_specifier.as_ref() | ||
| { | ||
| self.skipped_optional_dependencies | ||
| .entry(task.importer.clone()) | ||
| .or_default() | ||
| .insert(task.name.clone(), spec.clone()); | ||
| } |
There was a problem hiding this comment.
Recording this skip makes it sticky for root optional deps. check_drift exempts any name in skipped_optional_dependencies whose specifier still matches, so once the registry gap is recorded the dep is never retried on later installs even after the registry gains the version. That map is documented as capturing host-stable platform skips; registry availability is transient. Note the sibling fetch-failure skip records nothing — the comment above claims alignment with both paths, but only the platform one records.
Technical details
# Registry-availability skip is recorded in a map whose consumers assume host-stable skips
## Affected sites
- `vendor/aube/crates/aube-resolver/src/resolve/driver.rs:1025-1032` — new arm records into `skipped_optional_dependencies` for root deps.
- `vendor/aube/crates/aube-resolver/src/resolve/driver.rs:720-730` — the fetch-failure skip, the closest analogue (also a registry-availability failure), deliberately records nothing and only calls `note_root_done()`.
- `vendor/aube/crates/aube-lockfile/src/drift.rs:537-546` — the map is documented as "*runtime* platform skips"; the exemption is unconditional on cause.
- `vendor/aube/crates/aube-lockfile/src/drift.rs:1858-1878` — `fresh_when_optional_dep_was_recorded_as_skipped` confirms drift reports `Fresh`, so the install short-circuits.
- `vendor/aube/crates/aube-lockfile/src/pnpm/write.rs:362-379` — the entry is persisted to the lockfile.
## Symptom
1. `optionalDependencies: { "foo": "1.0.3" }`, mirror lacks `1.0.3`.
2. `nub install` skips, records, writes `skippedOptionalDependencies: { foo: 1.0.3 }`.
3. Mirror later syncs `1.0.3`.
4. `nub install` reports `Fresh` and never re-resolves; `foo` stays uninstalled until the specifier changes.
Only root optional deps are affected — the motivating `rolldown` bindings are transitive, so they record nothing and self-heal.
## Required outcome
- The reader can tell from the code why a transient registry gap is recorded in a map the drift check treats as permanent, or the recording is narrowed so the skip self-heals.
## Suggested approach
The tradeoff cuts both ways and the current choice is defensible — dropping the recording would make the next `nub install --frozen-lockfile` / `nub ci` fail with "manifest adds foo", which is exactly what the platform-skip recording exists to prevent. If recording stays, tightening the arm's comment to say it matches the platform path's bookkeeping (and why it diverges from the fetch-failure sibling that records nothing) would keep the next reader from concluding all three paths behave alike.
## Open questions for the human
- Is a permanently sticky skip acceptable for a cause that can resolve itself, or should a registry-gap skip be re-probed on the next non-frozen install?| .await | ||
| .expect("an optional dep with no matching version must not fail the resolve"); | ||
| assert!(graph_has_package(&graph, "p-map", "7.0.4")); | ||
| assert!(!graph_has_package(&graph, "native-binding", "1.0.3")); |
There was a problem hiding this comment.
Neither new test covers the recording half of the change. With an exact 1.0.3 range this assertion can't fail for any reason other than the resolve erroring, which the expect above already catches — asserting graph.skipped_optional_dependencies["."]["native-binding"] == "1.0.3" would cover the new lockfile-visible bookkeeping instead, matching how aube-lockfile/src/pnpm/tests.rs:3254 asserts the same map.
|
Shipped in v0.7.0: https://github.com/nubjs/nub/releases/tag/v0.7.0 |
rolldowndeclares 15@rolldown/binding-*packages asoptionalDependencies. The version pick runs before theos/cpufilter, so a binding the host will never install could fail the entire install when the registry did not carry it. An optional dependency should never be fatal — the fetch-failure and platform-mismatch paths already skip, but the version-pick path did not.Optional deps whose range matches no version are now skipped and recorded, with a warning. The age-gate path stays fatal: a version withheld by
minimumReleaseAgeis a supply-chain signal the user opted into, not a package the registry lacks.Verification
Differential against a registry with
1.0.3removed from one wrong-platform binding, fresh cache per leg:ERR_NUB_NO_MATCHING_VERSION, exit 20require('rolldown')loads after the install, so the tree is usable rather than silently incomplete. npm and pnpm both install cleanly in the same scenario.New tests cover both directions — an optional dep skips, a required dep still fails. The optional test was confirmed to fail without this change.
Closes #580