Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

registry/crates/pnpm-registry/src/server.rs (3)

867-874: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Refresh time.modified on dist-tag mutations.

update_dist_tag() rewrites the packument without touching the time block. Tag-only changes will therefore look stale to clients that use time.modified as the freshness signal.

🐛 Minimal fix

     if let Err(err) = mutate(tags) {
         return error_response(&err);
     }
     let _ = tag; // tag name is used by the mutate closure
+    let time_entry = packument_obj
+        .entry("time".to_string())
+        .or_insert_with(|| Value::Object(serde_json::Map::new()));
+    let Some(time_obj) = time_entry.as_object_mut() else {
+        return error_response(&RegistryError::BadRequest {
+            reason: "stored time is not an object".to_string(),
+        });
+    };
+    time_obj.insert("modified".to_string(), Value::String(now_iso()));
     let new_bytes = match serde_json::to_vec_pretty(&packument) {
         Ok(b) => b,
         Err(err) => return error_response(&RegistryError::Json(err)),
     };

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@registry/crates/pnpm-registry/src/server.rs` around lines 867 - 874, When
mutating dist-tags in update_dist_tag (inside the mutate(tags) closure where
packument and tag are used), refresh the packument.time.modified field to the
current timestamp so tag-only changes appear fresh; if packument.time or
packument.time.modified is missing, create them and set modified to the current
UTC ISO-8601 string (use your existing datetime utility or
chrono::Utc::now().to_rfc3339()), then continue to serialize packument as before
with serde_json::to_vec_pretty.

---

554-563: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Seed publish merges from upstream when the local cache is cold.

Line 554 only reads the existing packument from disk. In proxy mode, publishing over a package that exists upstream but has never been cached locally will start the merge from None, and the newly written local packument will hide every upstream version/dist-tag on subsequent reads.

🐛 Minimal fix

-    let existing_bytes = match state.inner.cache.read_packument_any_age(&name).await {
-        Ok(b) => b,
-        Err(err) => return error_response(&err),
-    };
+    let existing_bytes = match state.inner.cache.read_packument_any_age(&name).await {
+        Ok(Some(bytes)) => Some(bytes),
+        Ok(None) => match load_packument_bytes(state, &name).await {
+            PackumentLoad::Ok(bytes) => Some(bytes),
+            PackumentLoad::NotFound => None,
+            PackumentLoad::Err(err) => return error_response(&err),
+        },
+        Err(err) => return error_response(&err),
+    };

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@registry/crates/pnpm-registry/src/server.rs` around lines 554 - 563, The
merge currently uses None when read_packument_any_age(&name) returns no local
packument, which causes local publishes to hide upstream versions; change the
flow so that if existing_bytes is None and the registry is running in proxy mode
you attempt to load the upstream packument and use that as existing for
merge_manifest. Concretely: after read_packument_any_age returns None, call the
code-path that fetches the packument from upstream (e.g., the existing
proxy/upstream fetch helper on state.inner or similar), deserialize it into a
Value, treat errors via error_response like the local read does, and pass that
Value (not None) into merge_manifest(name, incoming, now_iso()) so upstream
versions/dist-tags are preserved.

---

530-537: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reject publish bodies whose name disagrees with the URL.

Line 530 parses the incoming packument, but this path still never checks that incoming["name"] matches raw_name. A mismatched payload can be stored under the wrong package directory and leave the packument internally inconsistent.

🐛 Minimal fix

     let mut incoming: Value = match serde_json::from_slice(&body) {
         Ok(v) => v,
         Err(err) => return error_response(&RegistryError::Json(err)),
     };
+    let body_name = incoming.get("name").and_then(Value::as_str).unwrap_or("");
+    if body_name != name.as_str() {
+        return error_response(&RegistryError::BadRequest {
+            reason: format!(
+                "package in URL ({:?}) does not match body ({body_name:?})",
+                name.as_str()
+            ),
+        });
+    }
     let attachments = match extract_attachments(&mut incoming) {
         Ok(a) => a,
         Err(err) => return error_response(&err),
     };

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@registry/crates/pnpm-registry/src/server.rs` around lines 530 - 537, After
parsing incoming into serde_json::Value (variable incoming) but before calling
extract_attachments, validate that the packument name matches the URL by
checking incoming["name"] is a string and equals raw_name; if the field is
missing, not a string, or not equal to raw_name, return error_response with an
appropriate RegistryError (or a new variant) to reject the request. Implement
this check in the same scope where incoming is available (near the existing
serde_json::from_slice and before extract_attachments) and keep the
extract_attachments, error_response, and RegistryError::Json usage unchanged for
other error paths.

registry/crates/pnpm-registry/src/cache.rs (1)

163-166: 💤 Low value

Clarify comment wording.

The comment reads: "a benign 404 here would surface as a real error to the caller" — but the code specifically prevents that by returning Ok(false) on NotFound. Consider rewording to: "Without this handling, a benign 404 would surface as a real error to the caller."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@registry/crates/pnpm-registry/src/cache.rs` around lines 163 - 166, Update
the comment above the tarball-removal logic in cache.rs (the block that
documents "Remove a single tarball file. Returns `Ok(false)` when the file is
already gone" and the sentence mentioning a "benign 404") to clarify causality:
rephrase the sentence to something like "Without this handling, a benign 404
would surface as a real error to the caller." Keep the rest of the comment
intact and ensure it still references that the function returns `Ok(false)` on
`NotFound`.

registry/crates/pnpm-registry/tests/auth_publish.rs (1)

577-577: ⚡ Quick win

Consider a more direct assertion for clarity and efficiency.

The current approach collects all keys into a Vec just to compare them. A more idiomatic pattern would check the length and presence directly.

♻️ Proposed refactor

-    assert_eq!(served["versions"].as_object().unwrap().keys().collect::<Vec<_>>(), vec!["2.0.0"]);
+    let versions = served["versions"].as_object().unwrap();
+    assert_eq!(versions.len(), 1);
+    assert!(versions.contains_key("2.0.0"));

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@registry/crates/pnpm-registry/tests/auth_publish.rs` at line 577, The
assertion currently builds a Vec of keys to compare to ["2.0.0"]; instead,
directly assert the map size and presence: use
served["versions"].as_object().unwrap().len() == 1 (or assert_eq!) and
assert!(served["versions"].as_object().unwrap().contains_key("2.0.0")); update
the assertion around the served variable to these two checks to be clearer and
more efficient.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

registry/crates/pnpm-registry/src/server.rs (1)

710-718: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate partial-unpublish payload shape and package identity before persisting.

update_packument still writes arbitrary JSON and does not enforce body.name == URL package, so a malformed/mismatched payload can replace a valid packument with inconsistent content.

Proposed minimal fix

     let mut packument: Value = match serde_json::from_slice(body) {
         Ok(v) => v,
         Err(err) => return error_response(&RegistryError::Json(err)),
     };
-    if let Some(obj) = packument.as_object_mut() {
-        obj.remove("_attachments");
-        obj.remove("_rev");
-        obj.remove("_revisions");
-    }
+    let Some(obj) = packument.as_object_mut() else {
+        return error_response(&RegistryError::BadRequest {
+            reason: "packument body must be a JSON object".to_string(),
+        });
+    };
+    let body_name = obj.get("name").and_then(Value::as_str);
+    if body_name != Some(name.as_str()) {
+        return error_response(&RegistryError::BadRequest {
+            reason: format!(
+                "package in URL ({:?}) does not match body ({body_name:?})",
+                name.as_str()
+            ),
+        });
+    }
+    obj.remove("_attachments");
+    obj.remove("_rev");
+    obj.remove("_revisions");

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@registry/crates/pnpm-registry/src/server.rs` around lines 710 - 718, The
update_packument handler currently accepts arbitrary JSON in the packument
variable and persists it without validating shape or identity; change
update_packument so it deserializes the request body into a strong type (or at
minimum checks required fields) and verifies that packument["name"] (or the
deserialized name field) equals the package name derived from the request URL
before persisting; if deserialization fails or the name mismatches, return
error_response(&RegistryError::InvalidPayload) (or an appropriate error) instead
of writing the JSON, and keep the existing removal of internal fields
("_attachments", "_rev", "_revisions") only after the payload is validated.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

.github/workflows/test.yml (1)

87-92: ⚡ Quick win

Reminder: Remove temporary debug scope before merging.

This temporary branch-specific test scope restriction is clearly marked for reversion before merge, but it would significantly degrade CI coverage if accidentally landed on main.

Consider adding a GitHub issue to track removal, or use a feature flag / environment variable approach if you need this debug capability to persist across multiple pushes.

Would you like me to suggest a more robust approach for conditional test scoping, or open a tracking issue for this cleanup?

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/test.yml around lines 87 - 92, This PR contains a
temporary branch-specific CI guard that limits tests when REF_NAME ==
"pnpm-registry-auth" (writing script=ci:test-registry-access-only and
scope=registry-access only to GITHUB_OUTPUT); remove this block before merging
(or replace it with a safer mechanism) by either deleting the if-block that
checks REF_NAME or changing it to an opt-in environment variable/feature-flag
(e.g., CHECK_ONLY_REGISTRY_ACCESS) so accidental merges to main won’t narrow CI
coverage, and optionally open a tracking issue to remind the team to remove the
debug hook if you keep the flag approach.

registry/crates/pnpm-registry/src/search.rs (1)

103-103: 💤 Low value

Consider logging read errors for consistency.

Lines 107-108 and 121-122 log parse and build failures, but Line 103 silently skips files that can't be read. For debugging purposes, it might be helpful to log read errors as well, especially since you're already tracking statistics.

📊 Optional: Add read error logging

-        let Ok(bytes) = fs::read(&path).await else { continue };
+        let bytes = match fs::read(&path).await {
+            Ok(bytes) => bytes,
+            Err(err) => {
+                tracing::warn!(?err, ?path, "search: failed to read packument");
+                continue;
+            }
+        };

Note: Read errors might be more frequent than parse/build errors (e.g., permission issues), so monitor log volume if you add this.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@registry/crates/pnpm-registry/src/search.rs` at line 103, The file-read is
silently skipped on failure (let Ok(bytes) = fs::read(&path).await else {
continue };) — change this to capture and log the read error (include the path
and error details) instead of silently continuing; use the same
logger/statistics pattern used for parse/build failures (the surrounding
parse/build error logging) so read errors are recorded and, if appropriate,
increment the same failure counter or metric for consistency.