Works on remaining missing tests#306
Conversation
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Immutable cache refetch breaks
- Folder, tarball, and exec fetchers now reuse cached archives normally and only force refetch when checksum checking is enabled.
- ✅ Fixed: JSR loose resolution omits locator
- JSR semver and tag loose resolutions now return the concrete resolved locator when registry resolution succeeds.
Or push these changes by commenting:
@cursor push 227b15e7a4
Preview (227b15e7a4)
diff --git a/packages/zpm/src/descriptor_loose.rs b/packages/zpm/src/descriptor_loose.rs
--- a/packages/zpm/src/descriptor_loose.rs
+++ b/packages/zpm/src/descriptor_loose.rs
@@ -404,7 +404,7 @@
Ok(LooseResolution {
descriptor,
- locator: None,
+ locator: Some(resolution_result.resolution.locator),
})
}
@@ -445,7 +445,7 @@
if resolution_check_result.resolution.version == resolution_result.resolution.version {
Ok(LooseResolution {
descriptor,
- locator: None,
+ locator: Some(resolution_check_result.resolution.locator),
})
} else {
let fixed_range = resolution_result.resolution.version
@@ -456,7 +456,7 @@
Ok(LooseResolution {
descriptor,
- locator: None,
+ locator: Some(resolution_result.resolution.locator),
})
}
}
diff --git a/packages/zpm/src/fetchers/exec.rs b/packages/zpm/src/fetchers/exec.rs
--- a/packages/zpm/src/fetchers/exec.rs
+++ b/packages/zpm/src/fetchers/exec.rs
@@ -54,7 +54,7 @@
let locator_str
= locator.to_file_string();
- let pkg_blob = package_cache.refetch_blob_data(locator.clone(), ".zip", || async {
+ let fetch_archive = || async {
let temp_dir
= Path::temp_dir_pattern("exec-<>")?;
let build_dir
@@ -81,8 +81,14 @@
}).await??;
Ok(archive)
- }).await?;
+ };
+ let pkg_blob = if context.check_checksums {
+ package_cache.refetch_blob_data(locator.clone(), ".zip", fetch_archive).await?
+ } else {
+ package_cache.upsert_blob(locator.clone(), ".zip", fetch_archive).await?
+ };
+
let first_entry
= zpm_formats::zip::first_entry_from_zip(&pkg_blob.data)?;
diff --git a/packages/zpm/src/fetchers/folder.rs b/packages/zpm/src/fetchers/folder.rs
--- a/packages/zpm/src/fetchers/folder.rs
+++ b/packages/zpm/src/fetchers/folder.rs
@@ -50,7 +50,7 @@
let context_directory_for_entries
= context_directory.clone();
- let pkg_blob = package_cache.refetch_blob_data(locator.clone(), ".zip", || async {
+ let fetch_archive = || async {
let archive = tokio::task::spawn_blocking(move || -> Result<Vec<u8>, Error> {
let entries
= zpm_formats::entries_from_folder(&context_directory_for_entries)?
@@ -61,8 +61,14 @@
}).await??;
Ok(archive)
- }).await?;
+ };
+ let pkg_blob = if context.check_checksums {
+ package_cache.refetch_blob_data(locator.clone(), ".zip", fetch_archive).await?
+ } else {
+ package_cache.upsert_blob(locator.clone(), ".zip", fetch_archive).await?
+ };
+
let first_entry
= zpm_formats::zip::first_entry_from_zip(&pkg_blob.data)?;
diff --git a/packages/zpm/src/fetchers/tarball.rs b/packages/zpm/src/fetchers/tarball.rs
--- a/packages/zpm/src/fetchers/tarball.rs
+++ b/packages/zpm/src/fetchers/tarball.rs
@@ -48,7 +48,7 @@
let package_subdir_for_entries
= package_subdir.clone();
- let cached_blob = package_cache.refetch_blob_data(locator.clone(), ".zip", || async {
+ let fetch_archive = || async {
let tgz_data
= tarball_path.fs_read()?;
@@ -66,8 +66,14 @@
}).await??;
Ok(archive)
- }).await?;
+ };
+ let cached_blob = if context.check_checksums {
+ package_cache.refetch_blob_data(locator.clone(), ".zip", fetch_archive).await?
+ } else {
+ package_cache.upsert_blob(locator.clone(), ".zip", fetch_archive).await?
+ };
+
let first_entry
= zpm_formats::zip::first_entry_from_zip(&cached_blob.data)?;You can send follow-ups to the cloud agent here.
⏱️ Benchmark Resultsgatsby install-full-cold
📊 Raw benchmark data (gatsby install-full-cold)Base times: 4.338s, 4.373s, 4.440s, 4.358s, 4.404s, 4.352s, 4.330s, 4.426s, 4.338s, 4.319s, 4.369s, 4.363s, 4.336s, 4.353s, 4.360s, 4.386s, 4.385s, 4.419s, 4.407s, 4.375s, 4.444s, 4.339s, 4.385s, 4.330s, 4.306s, 4.308s, 4.353s, 4.369s, 4.292s, 4.405s Head times: 4.307s, 4.278s, 4.297s, 4.392s, 4.425s, 4.304s, 4.347s, 4.358s, 4.313s, 4.400s, 4.279s, 4.406s, 4.333s, 4.384s, 4.393s, 4.346s, 4.343s, 4.274s, 4.356s, 4.331s, 4.421s, 4.361s, 4.311s, 4.354s, 4.371s, 4.372s, 4.270s, 4.454s, 4.301s, 4.344s gatsby install-cache-only
📊 Raw benchmark data (gatsby install-cache-only)Base times: 1.263s, 1.246s, 1.245s, 1.238s, 1.255s, 1.261s, 1.280s, 1.227s, 1.250s, 1.254s, 1.255s, 1.245s, 1.239s, 1.259s, 1.240s, 1.248s, 1.264s, 1.249s, 1.242s, 1.255s, 1.230s, 1.254s, 1.239s, 1.248s, 1.242s, 1.247s, 1.227s, 1.250s, 1.243s, 1.250s Head times: 1.244s, 1.246s, 1.251s, 1.259s, 1.251s, 1.258s, 1.258s, 1.248s, 1.269s, 1.248s, 1.244s, 1.248s, 1.257s, 1.238s, 1.265s, 1.258s, 1.263s, 1.251s, 1.272s, 1.287s, 1.251s, 1.261s, 1.255s, 1.245s, 1.244s, 1.259s, 1.263s, 1.261s, 1.234s, 1.257s gatsby install-cache-and-lock (warm, with lockfile)
📊 Raw benchmark data (gatsby install-cache-and-lock (warm, with lockfile))Base times: 0.333s, 0.338s, 0.332s, 0.331s, 0.331s, 0.334s, 0.331s, 0.333s, 0.333s, 0.332s, 0.336s, 0.337s, 0.330s, 0.336s, 0.336s, 0.338s, 0.334s, 0.342s, 0.336s, 0.334s, 0.331s, 0.335s, 0.335s, 0.335s, 0.334s, 0.358s, 0.332s, 0.333s, 0.334s, 0.334s Head times: 0.335s, 0.343s, 0.336s, 0.335s, 0.334s, 0.337s, 0.355s, 0.350s, 0.339s, 0.345s, 0.337s, 0.338s, 0.340s, 0.341s, 0.341s, 0.335s, 0.340s, 0.341s, 0.340s, 0.343s, 0.340s, 0.340s, 0.339s, 0.340s, 0.342s, 0.340s, 0.341s, 0.338s, 0.340s, 0.342s |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Symlink folder sync check changed
- Restored folder checks to use symlink metadata so symlinked directories are replaced instead of treated as valid folders.
Or push these changes by commenting:
@cursor push 3b6d53d580
Preview (3b6d53d580)
diff --git a/packages/zpm-sync/src/lib.rs b/packages/zpm-sync/src/lib.rs
--- a/packages/zpm-sync/src/lib.rs
+++ b/packages/zpm-sync/src/lib.rs
@@ -280,7 +280,7 @@
SyncNode::Folder {template, ..} => {
let is_dir
- = path.fs_is_dir();
+ = metadata.is_dir();
Ok(SyncCheck {
must_remove: !is_dir,You can send follow-ups to the cloud agent here.
| must_remove: !metadata.is_dir(), | ||
| must_create: !metadata.is_dir() && template.is_none(), | ||
| must_remove: !is_dir, | ||
| must_create: !is_dir && template.is_none(), |
There was a problem hiding this comment.
Symlink folder sync check changed
Low Severity
Folder sync checks now use path.fs_is_dir() instead of metadata.is_dir() from fs_symlink_metadata(). For a symlink that points at a directory, those calls can disagree, so the sync step may treat the path as a directory when it previously required replacement, or the reverse.
Reviewed by Cursor Bugbot for commit 6556d7a. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Empty config field after onConflict
- Fields containing only onConflict metadata are now removed after recording the conflict mode so deserialization treats them as missing values.
- ✅ Fixed: Deduped virtuals skip peer links
- Peer link registration now resolves deduped virtual operations through their final master locator instead of skipping removed duplicate locators.
Or push these changes by commenting:
@cursor push b4dae504b1
Preview (b4dae504b1)
diff --git a/packages/zpm-config/src/lib.rs b/packages/zpm-config/src/lib.rs
--- a/packages/zpm-config/src/lib.rs
+++ b/packages/zpm-config/src/lib.rs
@@ -935,6 +935,8 @@
let mut field_modes
= BTreeMap::new();
+ let mut fields_to_remove
+ = Vec::new();
for (key, field_value) in mapping.iter_mut() {
let serde_yaml::Value::String(key) = key else {
@@ -956,9 +958,15 @@
if let Some(value) = field_mapping.remove(&value_key) {
*field_value = value;
+ } else if field_mapping.is_empty() {
+ fields_to_remove.push(key.clone());
}
}
+ for key in fields_to_remove {
+ mapping.remove(serde_yaml::Value::String(key));
+ }
+
(root_mode, field_modes)
}
diff --git a/packages/zpm/src/tree_resolver.rs b/packages/zpm/src/tree_resolver.rs
--- a/packages/zpm/src/tree_resolver.rs
+++ b/packages/zpm/src/tree_resolver.rs
@@ -464,27 +464,30 @@
.entry(final_resolution.clone()).or_default()
.insert(parent_locator.clone());
- if !self.resolution_tree.locator_resolutions.contains_key(&operation.virtualized_locator) {
+ let Some(virtualized_resolution) = self.resolution_tree.locator_resolutions.get(&final_resolution) else {
continue;
- }
+ };
- let peer_dependencies = &self.resolution_tree.locator_resolutions
- .get(&operation.virtualized_locator).unwrap()
- .peer_dependencies;
+ let peer_dependencies = virtualized_resolution.peer_dependencies.keys().cloned().sorted().collect_vec();
- for peer_ident in peer_dependencies.keys().sorted() {
- let root = operation.next_peer_slots.get(peer_ident)
+ for peer_ident in peer_dependencies {
+ let root = operation.next_peer_slots.get(&peer_ident)
.expect("Expected the peer dependency ident to be listed in the next peer slots");
+ let root = if root == &operation.virtualized_locator {
+ &final_resolution
+ } else {
+ root
+ };
self.peer_dependency_links
.entry(root.clone())
.or_default()
- .entry(peer_ident.clone())
+ .entry(peer_ident)
.or_default();
}
let virtualized_resolution = self.resolution_tree.locator_resolutions
- .get_mut(&operation.virtualized_locator).unwrap();
+ .get_mut(&final_resolution).unwrap();
for missing_peer_dependency in &virtualized_resolution.missing_peer_dependencies {
virtualized_resolution.dependencies.remove(missing_peer_dependency);You can send follow-ups to the cloud agent here.
|
@cursor review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Empty sync root deletes files
- Restored filtering for empty root folder nodes so an empty sync tree is skipped instead of deleting directory contents.
Or push these changes by commenting:
@cursor push 1ce49431ee
You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 4236bb8. Configure here.
| pub fn is_node_filtered_out(&self, node_idx: usize) -> bool { | ||
| if node_idx == 0 { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Empty sync root deletes files
Medium Severity
Exempting node index 0 from is_node_filtered_out means a SyncTree whose root folder has no registered children is no longer skipped. process_node then treats an existing root directory as an empty folder template and can enqueue deletes for every directory entry not in the (empty) child map.
Reviewed by Cursor Bugbot for commit 4236bb8. Configure here.
1 participant
Note
High Risk
Large cross-cutting changes to resolution, virtual peer deduplication, exec script execution, cache/refetch paths, and HTTP client selection affect core install/link behavior.
Overview
This PR closes gaps against upstream Yarn behavior and acceptance tests across resolution, caching, config, CLI output, and CI.
Package protocols and resolution: Adds
jsr:semver/tag ranges (mapped to@jsr/...npm-style ids),exec:ranges/references with workspace-only parents and script-gated builds, and optional content hashes onfile:/folder:/tarball:/exec:locators. Virtual peer instance deduplication is reworked to key on peer sets rather than per-descriptor consolidation.Install, cache, and network:
enableMirror,cacheFolder(replacinglocalCacheFolderName), selective global cache cleanup,refetch_blob_data, and offline-aware npm tag resolution that prefers cached tarballs. Per-host TLS overrides innetworkSettings,logFilters, install--jsonNDJSON reporting, and stricter peer-warning visibility when messages are filtered.PnP, binaries, and tooling: PnP fallback pool is populated from installed packages; visible binaries resolve via
node_moduleslayout.yarn run list, daemon--openquiet stdout, switch PM install export, git prepare via real yarn binaries, and CI pnpm@10 install.Config: Merges user/project rc with
onConflictextend/reset metadata before deserialization.Reviewed by Cursor Bugbot for commit 4236bb8. Bugbot is set up for automated code reviews on this repo. Configure here.