• Create PR with unit tests
• Commit unit tests in branch multi-publish

Description

journal::roll_forward() treats any fs::try_exists() error on a staged tarball as “does not
exist”, so it can skip promotion, still write the packument, and then delete the journal
entry—leaving metadata visible without its tarball and without any remaining journal state to retry
from.

Code

pnpr/crates/pnpr/src/journal.rs[R211-236]

+async fn roll_forward(storage: &Storage, dir: &Path) -> Result<()> {
+    let manifest: Manifest = serde_json::from_slice(&fs::read(dir.join(MANIFEST_FILE)).await?)?;
+    for package in &manifest.packages {
+        let name = PackageName::parse(&package.name)?;
+        for tarball in &package.tarballs {
+            if fs::try_exists(&tarball.tmp_path).await.unwrap_or(false) {
+                let slot = TarballSlot::from_parts(
+                    tarball.tmp_path.clone(),
+                    name.clone(),
+                    tarball.filename.clone(),
+                );
+                storage.finalize_tarball_slot(slot).await?;
+            }
+        }
+        let journaled: serde_json::Value =
+            serde_json::from_slice(&fs::read(dir.join(&package.packument_file)).await?)?;
+        let existing: Option<serde_json::Value> = match storage.read_hosted_packument(&name).await?
+        {
+            Some(bytes) => Some(serde_json::from_slice(&bytes)?),
+            None => None,
+        };
+        let merged = merge_manifest(existing.as_ref(), &journaled, &now_iso());
+        storage.write_hosted_packument(&name, &serde_json::to_vec_pretty(&merged)?).await?;
+    }
+    fs::remove_dir_all(dir).await?;
+    Ok(())

Evidence

The recovery path explicitly swallows try_exists() errors (unwrap_or(false)), but still writes
the packument and removes the transaction directory, so a transient probe failure can lead to a
packument being made visible without its tarball and without any journal state left to replay.

pnpr/crates/pnpr/src/journal.rs[211-236]

Agent prompt

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

## Issue description
`roll_forward()` currently does:

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

Description

PublishJournal::recover() treats any error checking for the commit marker as "not committed" via
unwrap_or(false), so a sealed transaction can be rolled back and its staged tarballs deleted during
startup recovery. This can lose a publish that was already sealed as committed in the journal.

Code

pnpr/crates/pnpr/src/journal.rs[R161-168]

+        for dir in txn_dirs {
+            if fs::try_exists(dir.join(COMMIT_MARKER)).await.unwrap_or(false) {
+                roll_forward(storage, &dir).await?;
+                tracing::info!(txn = %dir.display(), "rolled publish journal entry forward");
+            } else {
+                roll_back(&dir).await;
+                tracing::info!(txn = %dir.display(), "rolled publish journal entry back");
+            }

Evidence

recover() currently interprets any try_exists error as “marker absent”, and roll_back()
deletes tarballs referenced by the manifest, so a committed txn can be destroyed if the marker check
fails transiently.

pnpr/crates/pnpr/src/journal.rs[146-169]
pnpr/crates/pnpr/src/journal.rs[225-240]

Agent prompt

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

## Issue description
`PublishJournal::recover()` uses `fs::try_exists(...).await.unwrap_or(false)` to decide whether a txn is sealed. Any I/O error (permissions, transient disk error) becomes `false`, which sends the txn to `roll_back()` and can delete staged tarballs for what may actually be a sealed/committed transaction.
### Issue Context
Recovery runs before the server accepts requests, so it is the last line of defense against partial publishes. On error, it should be conservative: never treat “can’t tell” as “unsealed”.
### Fix Focus Areas
- pnpr/crates/pnpr/src/journal.rs[146-169]
- pnpr/crates/pnpr/src/journal.rs[225-240]
### Suggested fix
Replace `unwrap_or(false)` with an explicit match:
- `Ok(true)` => roll forward
- `Ok(false)` => roll back
- `Err(e)` => return `Err(e.into())` (or at least log + abort recovery), so startup fails rather than risking rollback of a committed txn.

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

Description

commit_publishes() seals the transaction and then applies packages sequentially; any mid-loop error
returns immediately, potentially leaving earlier packages visible while returning an error for the
overall publish. This breaks the implied all-or-nothing semantics for a single request until a
restart runs journal recovery, and can trigger confusing client retries (e.g., “already published”
for the subset that succeeded).

Code

pnpr/crates/pnpr/src/server.rs[R1226-1234]

+    // Past the seal, failures must NOT clean up the staged files: the
+    // transaction is committed, and startup recovery finishes the
+    // apply from exactly this on-disk state.
+    for stage in staged {
+        for slot in stage.slots {
+            state.inner.storage.finalize_tarball_slot(slot).await?;
+        }
+        state.inner.storage.write_hosted_packument(&stage.name, &stage.merged_bytes).await?;
}

Evidence

After journal.seal() succeeds, the apply loop can make some packages visible and then abort on
error due to ?, propagating an error response while leaving partial state until recovery occurs at
a later startup.

pnpr/crates/pnpr/src/server.rs[1192-1234]
pnpr/crates/pnpr/src/journal.rs[17-21]

Agent prompt

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

## Issue description
After sealing the journal, `commit_publishes()` applies each package’s tarballs + packument in a loop using `?`. If applying package N fails after packages 0..N-1 were already written, the handler returns an error but the registry state is partially updated until a restart.
### Issue Context
The code comments claim I/O failure mid-apply “can never leave the batch partially published” because recovery will roll forward later, but the server may continue running without recovery being invoked, and clients will receive an error response even though some packages may already be visible.
### Fix Focus Areas
- pnpr/crates/pnpr/src/server.rs[1192-1236]
### Suggested fix
Pick one conservative strategy:
1) **Fail-fast + force recovery soon**: if any apply step fails after sealing, log the failure and shut down the process (or otherwise stop accepting requests) so the supervisor restarts and startup recovery completes the sealed txn.
2) **Immediate roll-forward retry**: on apply error after sealing, run a best-effort roll-forward from the just-written journal entry (or call `journal.recover(...)`) and only then decide what status to return.
3) **Change response semantics**: return a dedicated error/status that clearly indicates “committed but not yet applied; do not retry”, ideally with a txn id.
Also consider applying tarballs for **all** packages first and packuments second to reduce the window of partial *packument* visibility (though it doesn’t fully eliminate partial outcomes if packument writes fail mid-way).

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

Description

createPublishDocument() builds dist.tarball using a tarball filename that includes the full
scoped package name (e.g. @scope/name-1.0.0.tgz), which turns the tarball URL into a 5-segment
path that pnpr will not route. Consumers downloading via the published packument will hit 404 for
batch-published scoped packages.

Code

releasing/commands/src/publish/batchPublish.ts[R220-232]

+  const tarballName = `${name}-${version}.tgz`
+  const manifest: ExportedManifest = {
+    ...publishedManifest,
+    _id: `${name}@${version}`,
+    version,
+    _nodeVersion: process.versions.node,
+    dist: {
+      integrity: `sha512-${createHash('sha512').update(tarballData).digest('base64')}`,
+      shasum: createHash('sha1').update(tarballData).digest('hex'),
+      // libnpmpublish stores an http:// URL on purpose (clients fetch via HTTPS regardless when
+      // the registry is HTTPS); keep the wire format identical to a single-package publish.
+      tarball: new URL(`${name}/-/${tarballName}`, registry).href.replace(/^https:\/\//, 'http://'),
+    },

Evidence

The client constructs dist.tarball from a filename containing / for scoped packages, but pnpr’s
router only serves scoped tarballs at a 4-segment path with a single-segment filename, and pnpr’s
package-name logic documents/implements the canonical -.tgz tarball filename for serving.

releasing/commands/src/publish/batchPublish.ts[205-233]
pnpr/crates/pnpr/src/server.rs[371-401]
pnpr/crates/pnpr/src/server.rs[634-646]
pnpr/crates/pnpr/src/package_name.rs[43-86]

Agent prompt

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

## Issue description
For scoped packages, `createPublishDocument()` currently sets `dist.tarball` using `tarballName = `${name}-${version}.tgz`` (which contains `/`). This produces a tarball URL like `/<scope>/<name>/-/@scope/<name>-<ver>.tgz`, which does not match pnpr’s tarball routes and will 404 for clients.
### Issue Context
pnpr accepts scoped attachment keys like `@scope/name-1.0.0.tgz`, but it canonicalizes the on-disk/served tarball filename to `<basename>-<version>.tgz` and routes tarballs as `/{scope}/{name}/-/{filename}` where `{filename}` is a single segment.
### Fix Focus Areas
- releasing/commands/src/publish/batchPublish.ts[205-249]
- releasing/commands/test/publish/batchPublish.test.ts[99-151]
### Suggested fix
- Keep the `_attachments` key as the libnpmpublish-compatible form (`@scope/name-<ver>.tgz` for scoped).
- Compute a separate **canonical** tarball filename for `dist.tarball`, e.g.:
- `const basename = name.split('/').pop()!`
- `const canonicalFilename = `${basename}-${version}.tgz``
- `dist.tarball = new URL(`${name}/-/${canonicalFilename}`, registry).href.replace(...)`
- Update the batch publish tests’ `dist.tarball` expectation for scoped packages accordingly.

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

Description

batchPublishPackages() runs publish/postpublish scripts only after all per-registry batch requests
finish successfully, so if packages target multiple registries and a later registry publish fails,
earlier registries may already have published packages but none of their publish/postpublish scripts
will run. This differs from the non-batch recursive path, where each successfully published package
runs its scripts before moving on.

Code

releasing/commands/src/publish/batchPublish.ts[R81-98]

+  for (const [registry, group] of packedByRegistry.entries()) {
+    for (const { summary } of group) {
+      globalInfo(`📦 ${summary.id} → ${registry}`)
+    }
+    if (opts.dryRun) {
+      globalWarn(`Skip publishing ${group.length} package(s) to ${registry} (dry run)`)
+      continue
+    }
+    // eslint-disable-next-line no-await-in-loop
+    await multiPublishToRegistry(registry, group, opts)
+    globalInfo(`✅ Published ${group.length} package(s) to ${registry} in a single request`)
+  }
+  if (!opts.ignoreScripts) {
+    for (const { project } of packedPkgs) {
+      // eslint-disable-next-line no-await-in-loop
+      await runScriptsIfPresent(await lifecycleOpts(project.rootDir, opts), ['publish', 'postpublish'], project.manifest)
+    }
+  }

Evidence

Batch publish runs scripts only after the registry loop, so any thrown error aborts before scripts;
the single-package publish path runs scripts inside publish.ts per package, so earlier successes
get scripts even if a later publish fails.

releasing/commands/src/publish/batchPublish.ts[55-99]
releasing/commands/src/publish/publish.ts[267-300]

Agent prompt

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

## Issue description
`batchPublishPackages()` defers `publish`/`postpublish` until after looping over all registries. If `multiPublishToRegistry()` throws for a later registry, the function exits and skips scripts even for packages already published to earlier registries.
### Issue Context
This is observable when workspace packages have different `publishConfig.registry` values (or when `findRegistryInfo` picks different targets). The non-batch recursive path publishes package-by-package and runs scripts for each successful publish.
### Fix Focus Areas
- releasing/commands/src/publish/batchPublish.ts[55-99]
### Suggested fix
Change script execution to align with publish success:
- Option A: after each successful `multiPublishToRegistry(registry, group, ...)`, immediately run `publish`/`postpublish` scripts for packages in that `group`.
- Option B: maintain a list of successfully published packages/groups, and in a `finally` (or in a `catch` before rethrow) run scripts for the successes recorded so far.
Either way, ensure scripts are not accidentally run for packages whose publish request failed.

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

Description

publish_package parses the entire publish JSON body before calling enforce_access, so anonymous
callers can force expensive JSON parsing/allocation work (up to the 100MiB router limit) before
being rejected. This is a DoS-amplification regression compared to checking publish permissions
immediately after the package name is known from the URL.

Code

pnpr/crates/pnpr/src/server.rs[R930-933]

+    let incoming: Value = match serde_json::from_slice(&body) {
  Ok(v) => v,
  Err(err) => return error_response(&RegistryError::Json(err)),
};

Evidence

The router-wide publish body limit is 100MiB, and publish_package currently calls
serde_json::from_slice before validate_publish_doc, where enforce_access actually runs.
Therefore unauthorized requests still pay the JSON parse cost up front.

pnpr/crates/pnpr/src/server.rs[48-53]
pnpr/crates/pnpr/src/server.rs[916-953]
pnpr/crates/pnpr/src/server.rs[1074-1083]

Agent prompt

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

## Issue description
`PUT /:pkg` (`publish_package`) deserializes the request body into JSON before performing the publish permission check (`enforce_access`). This allows unauthenticated callers to spend server CPU/memory on large bodies before receiving a 401/403.
### Issue Context
- The package name is already available from the URL path, so publish permission checks can be performed before parsing the body.
- The router allows publish bodies up to 100MiB.
### Fix Focus Areas
- Move the publish permission check to run immediately after `PackageName::parse(raw_name)` and **before** `serde_json::from_slice(&body)`.
- Ensure the same early-rejection behavior is preserved for the single-package path; optionally consider a cheap early anonymous rejection for the batch endpoint too.
- pnpr/crates/pnpr/src/server.rs[916-969]
- pnpr/crates/pnpr/src/server.rs[48-53]

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

Description

If PublishJournal::seal() fails after creating the transaction directory, the partially-created
.pnpr-journal// directory is not cleaned up immediately; commit_publishes only removes staged
tmp tarballs on this path. These unsealed journal directories will be cleaned up by startup
recovery, but long-running processes can accumulate residue between restarts if seal failures
repeat.

Code

pnpr/crates/pnpr/src/journal.rs[R110-116]

+    pub async fn seal(&self, packages: &[JournaledPublish<'_>]) -> Result<SealedTxn> {
+        let dir = self.root.join(txn_id());
+        fs::create_dir_all(&dir).await?;
+        let mut manifest = Manifest { packages: Vec::with_capacity(packages.len()) };
+        for (index, package) in packages.iter().enumerate() {
+            let packument_file = format!("packument-{index}.json");
+            write_synced(&dir.join(&packument_file), package.packument).await?;

Evidence

seal() creates the txn directory and performs multiple writes without any local error cleanup; on
the caller side, the error path only cleans tmp tarball slots and returns, leaving any
partially-created journal entry behind until the next startup recovery.

pnpr/crates/pnpr/src/journal.rs[101-141]
pnpr/crates/pnpr/src/server.rs[1202-1224]

Agent prompt

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

## Issue description
`PublishJournal::seal()` creates the transaction directory early and then performs multiple writes. If any of those writes fail, the function returns an error without removing the partially-created txn directory, leaving journal residue until the next restart/recovery.
### Issue Context
- `commit_publishes()` handles `seal()` errors by cleaning tmp tarball slots, but it does not (and cannot) remove the partially created journal directory because the directory path is not returned on error.
- Startup recovery will delete unsealed entries, but only runs on startup.
### Fix Focus Areas
- Make `PublishJournal::seal()` best-effort cleanup its own txn directory on error (e.g., create `dir` first, then use a guard to `remove_dir_all(&dir)` unless the function reaches the success path).
- Keep cleanup best-effort (ignore cleanup errors) to avoid masking the original failure.
- pnpr/crates/pnpr/src/journal.rs[110-141]
- pnpr/crates/pnpr/src/server.rs[1206-1224]

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

Description

The new test writes cafile=${ENV_VAR_123} but asserts for the project-level request-destination
warning about registry, which is only emitted for registry/proxy URL settings. This makes the test
incorrect (likely failing) and does not validate the intended behavior.

Code

pnpm/test/getConfig.test.ts[R52-65]

test('console a warning when the .npmrc has an env variable in a project-level registry', async () => {
 prepare()

+  fs.writeFileSync('.npmrc', 'cafile=${ENV_VAR_123}', 'utf8')
+
+  await getConfig({
+    json: false,
+  }, {
+    workspaceDir: '.',
+    excludeReporter: false,
+  })
+
+  expect(console.warn).toHaveBeenCalledWith(expect.stringContaining('Ignored project-level request destination "registry"'))
+})

Evidence

The test currently uses cafile=${ENV_VAR_123} but expects a warning that loadNpmrcFiles only
emits for request-destination keys such as registry/proxy; cafile is not treated as a request
destination value key, so this specific warning will not be produced for that input.

pnpm/test/getConfig.test.ts[52-65]
config/reader/src/loadNpmrcFiles.ts[263-275]
config/reader/src/loadNpmrcFiles.ts[294-300]

Agent prompt

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

## Issue description
A new Jest test writes a project-level `.npmrc` containing `cafile=${ENV_VAR_123}` but expects a warning that only applies to request-destination keys (e.g. `registry`, `proxy`, `https-proxy`). This mismatch means the test is asserting the wrong behavior and is likely to fail.
### Issue Context
In `config/reader`, the `Ignored project-level request destination ...` warning is only emitted when an env placeholder appears in a *request destination* value (registry/proxy), not for `cafile`.
### Fix Focus Areas
- pnpm/test/getConfig.test.ts[52-65]
### Suggested fix
Update the test to either:
1) Write a request-destination key/value (e.g. `registry=${ENV_VAR_123}` or `https-proxy=${ENV_VAR_123}`) if the intent is to test the request-destination warning; **or**
2) Keep `cafile=${ENV_VAR_123}` but change the expectation to the warning that actually occurs for unresolved env placeholders (e.g. `Failed to replace env in config: ${ENV_VAR_123}`), and rename the test accordingly to avoid referring to `registry`.

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

Description

The new batch publish test calls toHaveLength() on the attachment object instead of on
attachment.length, so it will fail even when the tarball bytes and declared length are correct.
This can break CI for the PR despite a correct implementation.

Code

releasing/commands/test/publish/batchPublish.test.ts[R143-146]

+  const attachment = scopedPkg._attachments['@pnpmtest/batch-pkg-1-1.0.0.tgz']
+  expect(attachment).toBeDefined()
+  const tarballData = Buffer.from(attachment.data, 'base64')
+  expect(attachment).toHaveLength(tarballData.length)

Evidence

attachment is the value of _attachments[...] (an object with { content_type, data, length }),
but the test asserts the object itself has a length equal to the decoded tarball length, rather than
asserting attachment.length.

releasing/commands/test/publish/batchPublish.test.ts[141-150]

Agent prompt

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

## Issue description
A Jest assertion uses `expect(attachment).toHaveLength(...)` where `attachment` is an object that contains a `length` field; `toHaveLength` expects an array/string/Buffer-like value with a `.length` and will not validate the intended field.
### Issue Context
This makes the test fail regardless of correct publish document generation.
### Fix Focus Areas
- releasing/commands/test/publish/batchPublish.test.ts[143-146]
### Suggested fix
Replace the assertion with one that checks the numeric field, e.g.:
- `expect(attachment.length).toBe(tarballData.length)`
(or `expect(attachment.length).toEqual(tarballData.length)`).

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

Description

journal::roll_forward() treats any fs::try_exists() error on a staged tarball as “does not
exist”, so it can skip promotion, still write the packument, and then delete the journal
entry—leaving metadata visible without its tarball and without any remaining journal state to retry
from.

Code

pnpr/crates/pnpr/src/journal.rs[R211-236]

+async fn roll_forward(storage: &Storage, dir: &Path) -> Result<()> {
+    let manifest: Manifest = serde_json::from_slice(&fs::read(dir.join(MANIFEST_FILE)).await?)?;
+    for package in &manifest.packages {
+        let name = PackageName::parse(&package.name)?;
+        for tarball in &package.tarballs {
+            if fs::try_exists(&tarball.tmp_path).await.unwrap_or(false) {
+                let slot = TarballSlot::from_parts(
+                    tarball.tmp_path.clone(),
+                    name.clone(),
+                    tarball.filename.clone(),
+                );
+                storage.finalize_tarball_slot(slot).await?;
+            }
+        }
+        let journaled: serde_json::Value =
+            serde_json::from_slice(&fs::read(dir.join(&package.packument_file)).await?)?;
+        let existing: Option<serde_json::Value> = match storage.read_hosted_packument(&name).await?
+        {
+            Some(bytes) => Some(serde_json::from_slice(&bytes)?),
+            None => None,
+        };
+        let merged = merge_manifest(existing.as_ref(), &journaled, &now_iso());
+        storage.write_hosted_packument(&name, &serde_json::to_vec_pretty(&merged)?).await?;
+    }
+    fs::remove_dir_all(dir).await?;
+    Ok(())

Evidence

The recovery path explicitly swallows try_exists() errors (unwrap_or(false)), but still writes
the packument and removes the transaction directory, so a transient probe failure can lead to a
packument being made visible without its tarball and without any journal state left to replay.

pnpr/crates/pnpr/src/journal.rs[211-236]

Agent prompt

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

## Issue description
`roll_forward()` currently does:

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

• ➕ Works with any npm-compatible registry (no new server endpoint required)
• ➕ Avoids very large single JSON bodies (base64 tarballs) and proxy limits
• ➖ Still incurs N requests and N registry-side transactions
• ➖ Cannot provide cross-package atomicity semantics

• ➕ Allows streaming/multipart uploads instead of base64-in-JSON
• ➕ More control over partial failures (e.g., retry tarball upload without rebuilding docs)
• ➖ More endpoints and protocol complexity
• ➖ Harder to keep wire-compat with libnpmpublish semantics

• ➕ Client could send fewer fields and reuse existing tarball packing only
• ➕ Server could centralize policy enforcement and metadata normalization
• ➖ Server must replicate npm client metadata generation (dist tags, access, etc.)
• ➖ Higher risk of drift from npm/libnpmpublish behavior