Walkthrough

Adds append_manifest to tarball and zip download flows, applies synthesized runtime package.json bytes during extraction when needed, and updates binary-resolution call sites, other download call sites, tests, and porting notes.

Changes

append_manifest feature

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant InstallPackageBySnapshot
  participant DownloadTarballToStore
  participant apply_append_manifest
  participant CAS

  InstallPackageBySnapshot->>InstallPackageBySnapshot: synthesize_runtime_manifest_bytes()
  InstallPackageBySnapshot->>DownloadTarballToStore: run(append_manifest: Some(bytes))
  DownloadTarballToStore->>DownloadTarballToStore: extract archive
  alt package.json missing
    DownloadTarballToStore->>apply_append_manifest: apply(cas_paths, pkg_files_idx, bytes)
    apply_append_manifest->>CAS: write synthesized package.json
  end
  DownloadTarballToStore-->>InstallPackageBySnapshot: extraction result

Suggested labels: product: pacquet

• X
• Mastodon
• Reddit
• LinkedIn

• Persist synthesized runtime package.json into the store-index row for warm installs.
• Thread append_manifest through tarball/zip fetchers so cold/warm materializations match.
• Add unit tests covering manifest folding and the no-op behavior for normal packages.

graph TD
  A["Install snapshot"] --> B["Synthesize manifest"] --> C["Download + extract"] --> D{"Archive has package.json?"}
  D -->|"no"| E["apply_append_manifest"] --> F[("CAFS blobs")] --> G[("Store index row")]
  D -->|"yes"| G

  subgraph Legend
    direction LR
    _fn["Function"] ~~~ _dec{"Decision"} ~~~ _db[("Persistent store")]
  end

Loading

The following are alternative approaches to this PR:

1. Post-write store-index patch-up

• ➕ Avoids adding a new parameter to all download call sites
• ➕ Could be implemented entirely in the store-index writer layer
• ➖ More complex lifecycle: you must coordinate with queued/in-flight writes
• ➖ Still needs to ensure CAS blob exists and is referenced consistently
• ➖ Harder to guarantee correctness under retries/concurrency

2. Force warm path to re-run binary fetch/synthesis

• ➕ Keeps store-index schema/rows unchanged
• ➕ Moves responsibility to materialization layer
• ➖ Defeats the purpose of warm materialization/prefetch
• ➖ Adds network/compute to warm installs and hurts performance/offline behavior

3. Add `dlx`/bin-linker fallback when manifest is missing

• ➕ Improves resilience for stores already containing pre-fix rows
• ➕ Does not require rewriting existing store-index data
• ➖ Paper-over fix: runtime slots can still be incomplete/missing package.json
• ➖ More conditional behavior and potential divergence from pnpm semantics

Recommendation: Keep the PR’s approach: thread a synthesized manifest via append_manifest and fold it into both CAFS output and the persisted PackageFilesIndex row before it’s written. This aligns with pnpm’s appendManifest semantics and fixes the warm-materialization failure at the source (store-index correctness). Consider the bin-linker fallback only as a follow-up hardening measure for already-corrupted stores.

Bug fix (7) +101 / -26

install_config_deps.rsPlumb new 'append_manifest' field for installer tarball downloads +1/-0

Plumb new 'append_manifest' field for installer tarball downloads

• Updates the tarball download invocation to set 'append_manifest: None' to match the expanded downloader API. This keeps env-installer builds compiling while reserving manifest appending for runtime archives only.

pacquet/crates/env-installer/src/install_config_deps.rs

install_package_by_snapshot.rsPass synthesized runtime manifest into binary downloads +15/-24

Pass synthesized runtime manifest into binary downloads

• Moves runtime 'package.json' synthesis earlier and passes it to both tarball and zip binary download paths via 'append_manifest'. Removes the previous post-download in-memory-only insertion so the persisted store-index row includes the manifest for warm installs.

pacquet/crates/package-manager/src/install_package_by_snapshot.rs

install_package_from_registry.rsUpdate registry tarball downloads for 'append_manifest' API +1/-0

Update registry tarball downloads for 'append_manifest' API

• Adds 'append_manifest: None' to registry package downloads, preserving existing behavior (registry packages already ship a 'package.json').

pacquet/crates/package-manager/src/install_package_from_registry.rs

patch.rsUpdate patch write path for new download options +1/-0

Update patch write path for new download options

• Extends the download configuration used by patching to include 'append_manifest: None'. No behavior change intended; keeps compilation consistent with the new tarball API.

pacquet/crates/package-manager/src/patch.rs

prefetching_resolver.rsExtend prefetching downloads with 'append_manifest: None' +1/-0

Extend prefetching downloads with 'append_manifest: None'

• Updates resolve-time prefetch downloads to set 'append_manifest' explicitly to 'None', ensuring only runtime binary fetches can inject synthesized manifests.

pacquet/crates/package-manager/src/prefetching_resolver.rs

tarball_prefetch.rsAlign background tarball prefetch with new downloader API +1/-0

Align background tarball prefetch with new downloader API

• Adds 'append_manifest: None' to the spawned tarball downloads used for mem-cache prefetching. Keeps prefetch behavior unchanged while supporting the new manifest-folding capability.

pacquet/crates/package-manager/src/tarball_prefetch.rs

lib.rsImplement 'append_manifest' folding into CAFS + store-index rows +81/-2

Implement 'append_manifest' folding into CAFS + store-index rows

• Introduces 'apply_append_manifest' to write a synthesized 'package.json' into the CAS and record it in both 'cas_paths' and the persisted 'PackageFilesIndex' ('files' map and bundled 'manifest'). Adds an 'append_manifest' option to both tarball and zip download structs and applies it before queueing the store-index row, fixing warm materialization for runtime archives.

pacquet/crates/tarball/src/lib.rs

Refactor (1) +1 / -0

main.rsUpdate micro-benchmark tarball download configuration +1/-0

Update micro-benchmark tarball download configuration

• Adds 'append_manifest: None' to the benchmark harness’s tarball download invocation to match the updated API. No benchmark semantics change intended.

pacquet/tasks/micro-benchmark/src/main.rs

Tests (1) +74 / -4

tests.rsAdd unit tests for 'apply_append_manifest' and update call sites +74/-4

Add unit tests for 'apply_append_manifest' and update call sites

• Adds tests verifying that a synthesized runtime manifest is persisted into both the files map and bundled manifest, and that the function is a no-op when an archive already includes 'package.json'. Updates existing tests to populate the new 'append_manifest' field with 'None'.

pacquet/crates/tarball/src/tests.rs

Description

New test comments use historical phrasing like Pre-fix / pre-fix gap to describe prior behavior
rather than the current contract. This increases drift risk and violates the requirement that
comments describe present behavior and invariants.

Code

pacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs[R612-734]

+/// End-to-end regression for the CI failure on pnpm/pnpm#12811, mirroring
+/// the core of `installing/deps-installer/test/install/nodeRuntime.ts:209`
+/// (`installing Node.js runtime`): a cold install, then `rimraf
+/// node_modules` + an offline reinstall. A runtime archive ships no
+/// `package.json`, so pacquet synthesizes one — and it must be baked into
+/// the persisted store-index row, not just this install's `cas_paths`.
+/// Pre-fix the row recorded neither the `package.json` file nor a bundled
+/// `manifest`, so a later *warm* install (which materializes straight from
+/// the row and never re-runs `fetch_binary_resolution_to_cas`) landed a
+/// manifest-less slot and `pnpm dlx node@runtime:<v>` died in `getBinName`
+/// with `dlx_read_manifest`.
+#[tokio::test]
+async fn installing_a_runtime_persists_the_synthesized_manifest_into_the_store_index_row() {
+    use pacquet_store_dir::{
+        SharedVerifiedFilesCache, StoreIndex, StoreIndexWriter, store_index_key,
+    };
+    use std::sync::atomic::AtomicU8;
+
+    let archive_tmp = tempfile::tempdir().expect("tempdir");
+    let tarball_path = archive_tmp.path().join("node-fixture.tar.gz");
+    let tarball_bytes = build_runtime_tarball_fixture();
+    std::fs::write(&tarball_path, &tarball_bytes).expect("write the fixture tarball");
+    let integrity = ssri::IntegrityOpts::new()
+        .algorithm(ssri::Algorithm::Sha512)
+        .chain(&tarball_bytes)
+        .result();
+
+    let store_tmp = tempfile::tempdir().expect("tempdir");
+    // `offline: true` is safe — a `file:` URL bypasses the offline gate,
+    // and it guarantees the install never reaches the network.
+    let config = leaked_offline_config("https://registry.test", store_tmp.path());
+    let (writer, writer_task) = StoreIndexWriter::spawn(&config.store_dir);
+
+    let package_key: PackageKey = "node@runtime:22.0.0".parse().expect("parse runtime key");
+    let metadata = pacquet_lockfile::PackageMetadata {
+        resolution: LockfileResolution::Binary(BinaryResolution {
+            url: format!("file:{}", tarball_path.display()),
+            integrity: integrity.clone(),
+            bin: BinarySpec::Map(std::collections::BTreeMap::from([(
+                "node".to_string(),
+                "bin/node".to_string(),
+            )])),
+            archive: BinaryArchive::Tarball,
+            prefix: None,
+        }),
+        version: Some("22.0.0".to_string()),
+        has_bin: Some(true),
+        engines: None,
+        cpu: None,
+        os: None,
+        libc: None,
+        deprecated: None,
+        prepare: None,
+        bundled_dependencies: None,
+        peer_dependencies: None,
+        peer_dependencies_meta: None,
+    };
+    let snapshot = pacquet_lockfile::SnapshotEntry::default();
+    let layout = crate::VirtualStoreLayout::legacy(store_tmp.path().join("vstore"), 120);
+    let allow_build_policy = crate::AllowBuildPolicy::new(
+        std::collections::HashSet::default(),
+        std::collections::HashSet::default(),
+        false,
+    );
+    let skipped = crate::SkippedSnapshots::new();
+    let logged_methods = AtomicU8::new(0);
+    let verified_files_cache = SharedVerifiedFilesCache::default();
+
+    // Cold install: fetch the fixture, synthesize the manifest, queue the row.
+    let cold_cas_paths = super::InstallPackageBySnapshot {
+        http_client: &pacquet_network::ThrottledClient::default(),
+        config,
+        layout: &layout,
+        store_index: None,
+        store_index_writer: Some(&writer),
+        prefetched_cas_paths: None,
+        progress_reported: None,
+        tarball_mem_cache: None,
+        verified_files_cache: &verified_files_cache,
+        logged_methods: &logged_methods,
+        requester: "/project",
+        package_key: &package_key,
+        metadata: &metadata,
+        snapshot: &snapshot,
+        allow_build_policy: &allow_build_policy,
+        skipped: &skipped,
+        workspace_root: store_tmp.path(),
+        node_linker: pacquet_config::NodeLinker::Hoisted,
+        defer_link: false,
+        link_concurrency_probe: None,
+    }
+    .run::<pacquet_reporter::SilentReporter>()
+    .await
+    .expect("cold runtime install");
+    assert!(cold_cas_paths.contains_key("package.json"), "the cold slot gets the manifest");
+
+    // Flush the store-index writer so the row is durable before read-back.
+    drop(writer);
+    writer_task.await.expect("join the writer task").expect("flush the store index");
+
+    // The persisted row must carry the synthesized `package.json` in both
+    // its `files` map and its bundled `manifest` — the pre-fix gap that a
+    // warm materialization would inherit.
+    let index_key =
+        store_index_key(&integrity.to_string(), &package_key.without_peer().to_string());
+    let row = StoreIndex::open_in(&config.store_dir)
+        .expect("open the store index")
+        .get(&index_key)
+        .expect("read the runtime row")
+        .expect("the runtime row was persisted");
+    assert!(row.files.contains_key("package.json"), "the row records the synthesized package.json");
+    let manifest = row.manifest.expect("the row records a bundled manifest");
+    assert_eq!(
+        manifest.get("bin").and_then(|bin| bin.get("node")).and_then(serde_json::Value::as_str),
+        Some("bin/node"),
+        "the bundled manifest carries the runtime bin",
+    );
+
+    // Warm reinstall — nodeRuntime.ts's `rimraf node_modules` + offline
+    // reinstall. Delete the archive so the store is the only possible
+    // source, then materialize straight from the persisted row. The slot
+    // must still get the `package.json`; pre-fix the row lacked it.
+    std::fs::remove_file(&tarball_path).expect("remove the fixture so only the store can serve");

Evidence

The checklist forbids historical/temporal phrasing in added/modified comments. The referenced
comment blocks explicitly use Pre-fix / pre-fix gap / pre-fix the row lacked it, which are
historical descriptions rather than current-contract wording.

Rule 772199: Comments must describe current behavior, not historical implementations
pacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs[612-622]
pacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs[712-734]

Agent prompt

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

## Issue description
New comments in the runtime install regression test describe *past* behavior using phrases like `Pre-fix` and `pre-fix gap`. Per the checklist, comments should describe the current behavior/contract (or the invariant being protected) without historical narration.
## Issue Context
This is within a regression test and inline comments explaining why the persisted store-index row must contain the synthesized `package.json`/bundled `manifest`. The same intent can be expressed as a current invariant (e.g., "If the row omits `package.json`, a warm install will fail...") without referencing pre-fix history.
## Fix Focus Areas
- pacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs[612-622]
- pacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs[712-734]

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

Description

build_runtime_tarball_fixture() writes a tar entry header without calling header.set_cksum(), which
can produce an invalid tar stream that archive readers reject, breaking the new regression test.
This repo’s other tar fixtures consistently recompute the checksum before appending entries.

Code

pacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs[R599-604]

+    let mut header = tar::Header::new_gnu();
+    header.set_size(script.len() as u64);
+    header.set_mode(0o755);
+    tar_builder
+        .append_data(&mut header, "node-v22.0.0-fixture/bin/node", &script[..])
+        .expect("append the fixture bin entry");

Evidence

The new fixture builds a tar header and appends it without recomputing the checksum. Elsewhere in
the repo, tar fixtures explicitly call set_cksum() before append, indicating this is the
expected/required pattern for producing valid tar bytes.

pacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs[597-605]
pacquet/crates/package-manager/src/prefetching_resolver/tests.rs[141-152]

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 runtime tarball fixture builds a `tar::Header` but never calls `set_cksum()`. Many tar writers/readers expect the header checksum to be correct; without it, extraction may fail and the regression test won’t reliably validate the intended behavior.
## Issue Context
Other fixtures in this repo call `header.set_cksum()` after setting size/mode/path and before appending data.
## Fix Focus Areas
- pacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs[597-605]
## Suggested change
Add `header.set_cksum();` after `set_mode(...)` (and after setting path if you add that later) and before `append_data(...)`.

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

Description

apply_append_manifest only checks for an exact "package.json" key, so an archive containing a
case-variant like "Package.json" will end up with two distinct entries in cas_paths/files. On
case-insensitive filesystems, materialization may keep the wrong bytes because imports are no-op on
AlreadyExists and the marker placement also treats an existing destination as success.

Code

pacquet/crates/tarball/src/lib.rs[R618-636]

+) -> Result<(), TarballError> {
+    if pkg_files_idx.files.contains_key("package.json") {
+        return Ok(());
+    }
+    let (cas_path, file_hash) =
+        store_dir.write_cas_file(manifest_bytes, false).map_err(TarballError::WriteCasFile)?;
+    let checked_at =
+        UNIX_EPOCH.elapsed().ok().and_then(|elapsed| u64::try_from(elapsed.as_millis()).ok());
+    let info = CafsFileInfo {
+        digest: format!("{file_hash:x}"),
+        // A synthesized manifest is a plain, non-executable data file;
+        // `0o644` is the same canonical mode `add_files_from_dir` reports
+        // for a non-executable entry (and pnpm's Windows-host default).
+        mode: 0o644,
+        size: manifest_bytes.len() as u64,
+        checked_at,
+    };
+    cas_paths.insert("package.json".to_string(), cas_path);
+    pkg_files_idx.files.insert("package.json".to_string(), info);

Evidence

The new helper inserts a canonical package.json purely based on an exact-case check, which allows
both Package.json and package.json to coexist in the index/maps. The materialization layer
treats existing targets as success (no overwrite), so a colliding preexisting casing can prevent the
synthesized bytes from being written as the completion marker on case-insensitive filesystems.

pacquet/crates/tarball/src/lib.rs[613-646]
pacquet/crates/package-manager/src/import_indexed_dir.rs[218-246]
pacquet/crates/package-manager/src/import_indexed_dir.rs[250-304]
pacquet/crates/package-manager/src/link_file.rs[155-190]

Agent prompt

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

## Issue description
`apply_append_manifest` treats presence of `package.json` as a case-sensitive lookup (`files.contains_key("package.json")`). If an archive contains a case-variant filename (e.g. `Package.json`), `apply_append_manifest` will still add a synthesized lowercase `package.json` entry, creating two paths that collide on case-insensitive filesystems.
During import/materialization, file imports are no-ops when the destination already exists, and marker placement (`import_marker_atomic`) also treats an existing destination as success. This means the synthesized manifest can fail to land on disk (or the on-disk bytes can be inconsistent with the store-index row), undermining the goal of reliably materializing the runtime’s `package.json`.
## Issue Context
- `apply_append_manifest` inserts `"package.json"` without guarding against case-variants.
- Import/materialization treats `AlreadyExists` as success and does not overwrite.
## Fix Focus Areas
- pacquet/crates/tarball/src/lib.rs[613-646]
- pacquet/crates/tarball/src/tests.rs[3212-3261]
## Suggested fix approach
1. In `apply_append_manifest`, before deciding to early-return or insert, scan for any existing key in `pkg_files_idx.files` that matches `package.json` in a case-insensitive way (e.g. `eq_ignore_ascii_case`).
2. If an exact `package.json` exists: keep current behavior (`Ok(())`).
3. If only case-variants exist:
 - Remove those entries from both `pkg_files_idx.files` and `cas_paths` (and log a warning that the archive shipped an unexpected manifest casing), then proceed to write/insert the synthesized canonical `package.json`.
 - This avoids creating two colliding entries and guarantees the canonical marker file will be materialized.
4. Add a unit test covering the case-variant scenario (e.g. pre-populate `idx.files` with `"Package.json"` and ensure `apply_append_manifest` results in only canonical `"package.json"` being present).

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

Description

append_manifest mutates the persisted PackageFilesIndex (adds package.json and possibly
manifest) before the row is queued, but the store-index key remains {integrity}\t{pkg_id}. If
two installs generate different synthesized manifests for the same (integrity, pkg_id), a warm
install can reuse a row with the wrong persisted manifest/bin mapping, making installs dependent on
prior store state.

Code

pacquet/crates/tarball/src/lib.rs[R2394-2398]

+        // Fold the synthesized runtime `package.json` into the row before
+        // it is persisted, so warm reinstalls (which read the row) get it.
+        if let Some(manifest_bytes) = append_manifest {
+            apply_append_manifest(store_dir, manifest_bytes, &mut cas_paths, &mut pkg_files_idx)?;
+        }

Evidence

The store-index row key is only {integrity}\t{pkg_id}, but this PR now mutates the queued row
based on append_manifest bytes (written into files["package.json"] and possibly manifest).
Those bytes are synthesized from BinaryResolution.bin, so if two installs ever synthesize
different bytes for the same key, the persisted row can be reused incorrectly.

pacquet/crates/tarball/src/lib.rs[2289-2419]
pacquet/crates/store-dir/src/store_index.rs[868-900]
pacquet/crates/package-manager/src/install_package_by_snapshot.rs[827-846]

Agent prompt

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

## Issue description
This PR persists a synthesized runtime `package.json`/bundled `manifest` into the store-index row, but the row key stays `"{integrity}\t{pkg_id}"`. That means the cached row’s *value* can vary with `append_manifest` bytes (derived from lockfile `BinaryResolution.bin`), while the *key* does not—so store reuse can return a row whose synthesized manifest differs from what the current install would synthesize.
### Issue Context
- The tarball/zip download path now calls `apply_append_manifest(...)` before queueing the `PackageFilesIndex` row.
- The store-index key is still computed from integrity + pkg_id only.
- There is precedent in `git_hosted_store_index_key` that when cached content depends on an extra dimension (`built`), the key must account for it.
### How to fix
Pick one of these mitigation strategies (ordered by least invasive to pnpm interop):
1) **On store-hit / prefetched-row reuse when `append_manifest` is `Some`:**
- Compute the digest (sha512 hex) of `append_manifest` bytes.
- Verify the cached row’s `files["package.json"].digest` (and/or bundled `manifest`) matches what would be synthesized.
- If it’s missing/mismatched, treat as a cache miss and rebuild/overwrite the row (or at minimum, override the returned `cas_paths` with the synthesized `package.json` CAS entry so the current install is deterministic).
2) **Make synthesis deterministic per `(integrity, pkg_id)`** by deriving the runtime `bin` mapping from the extracted archive (or from a fixed runtime contract) rather than trusting arbitrary lockfile `bin` maps.
### Fix Focus Areas
- pacquet/crates/tarball/src/lib.rs[2289-2421]
- pacquet/crates/store-dir/src/store_index.rs[868-900]
- pacquet/crates/package-manager/src/install_package_by_snapshot.rs[827-852]

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

Description

The regression test constructs a file URL via format!("file:{}", tarball_path.display()), which does
not guarantee a valid/encoded file URL (notably on Windows and for paths with reserved characters
like spaces/#). This can cause the test to fail to parse or resolve the archive path, masking the
real regression signal.

Code

pacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs[R648-649]

+            url: format!("file:{}", tarball_path.display()),
+            integrity: integrity.clone(),

Evidence

The test currently constructs a file: URL by string concatenation, while other parts of the repo
use Url::from_file_path() specifically to produce valid file URLs across platforms.

pacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs[630-656]
pacquet/crates/cli/src/cli_args/deploy.rs[1206-1211]

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 test uses string formatting to create a `file:` URL from a filesystem path. This is not cross-platform and may produce an invalid URL (missing required slashes, wrong separators on Windows, and no percent-encoding).
## Issue Context
This repo already uses `url::Url::from_file_path()` when it needs a correct file URL.
## Fix Focus Areas
- pacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs[645-656]
## Suggested change
Replace:

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

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.