crates/patching/src/apply.rs (1)

10-10: ⚡ Quick win

Switch upstream references to blob/main links.

The changed porting docs use SHA-permalink URLs; use blob/main URLs so references track current upstream behavior.

As per coding guidelines: “open files from https://github.com/pnpm/pnpm/blob/main/... rather than from a permalinked SHA.”

Also applies to: 53-53

🤖 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 `@crates/patching/src/apply.rs` at line 10, The doc comment referencing the
upstream pnpm file uses a SHA-permalink; update the URL to the non-permalink
path so it tracks the main branch (use
https://github.com/pnpm/pnpm/blob/main/...), replacing the current SHA-based
link in the comment that references applyPatchToDir and any other similar
occurrences (e.g., the second occurrence around the symbol or comment near line
53) so both links point to blob/main instead of the SHA.

crates/package-manager/src/build_sequence/tests.rs (1)

64-64: ⚡ Quick win

Add one test that passes Some(patches) to cover the new branch.

Right now this file only exercises the None path; a patched-only node scenario would protect the new build trigger from regressions.

Also applies to: 78-78, 95-95, 114-114, 139-139, 168-168

🤖 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 `@crates/package-manager/src/build_sequence/tests.rs` at line 64, Add tests
that call build_sequence with Some(patches) instead of None to exercise the new
patched-only branch: construct a patches HashMap (keys matching package/node ids
used in the existing tests) and pass Some(patches) as the second argument to
build_sequence in the same test cases that currently call
build_sequence(&HashMap::new(), None, ...). Update the tests at the same
locations (currently calling build_sequence with None) to include at least one
scenario where patches contains entries for a node that has no other inputs so
the patched-only build trigger path in build_sequence is executed; reuse
existing test names and assertions but assert the expected behavior when patches
are present. Ensure you use the same function name build_sequence to locate the
code and mirror the existing test structure.

crates/package-manager/src/build_sequence.rs (1)

25-30: ⚡ Quick win

Use blob/main upstream links in the porting docs.

These changed comments still point to SHA-permalink URLs; switch them to https://github.com/pnpm/pnpm/blob/main/... so references stay aligned with upstream tip.

As per coding guidelines: “When porting features... open files from https://github.com/pnpm/pnpm/blob/main/... rather than from a permalinked SHA.”

Also applies to: 146-147

🤖 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 `@crates/package-manager/src/build_sequence.rs` around lines 25 - 30, The
comment in build_sequence.rs currently links upstream using a SHA-permalink;
update the URL(s) that reference getSubgraphToBuild (and the other link at lines
146-147) to use the moving tip form (https://github.com/pnpm/pnpm/blob/main/...)
instead of the SHA-based permalink so the porting docs follow the guideline;
locate the comment block referencing getSubgraphToBuild and any other upstream
links in this file and replace the permalinked paths with blob/main equivalents.

crates/package-manager/src/build_modules/tests.rs (1)

1258-1336: ⚡ Quick win

Add a cache-enabled variant of the patch-only snapshot test.

This test currently runs with cache disabled and no engine name, so it doesn’t exercise the patched-only cache-key path. Adding one variant with cache inputs enabled would close that regression gap.

🤖 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 `@crates/package-manager/src/build_modules/tests.rs` around lines 1258 - 1336,
Add a second test variant that exercises the patched-only cache-key path by
duplicating patch_only_snapshot_gets_patched_via_build_modules but enabling
cache inputs: set BuildModules.side_effects_cache = true and
side_effects_cache_write = true and provide an engine_name (e.g.,
Some("test-engine")), while keeping store_dir/store_index_writer/patches the
same; ensure the test uses the same setup (virtual_store_dir, pkg_dir, patch
file, patches map) and asserts the patched file was created, so the
cache-enabled code path is executed and covered.

Fix all unresolved CodeRabbit comments on this PR:

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

crates/patching/src/apply.rs:203

• Delete also uses fs::read_to_string, so it will fail with InvalidData on non-UTF8 files even if the patch would otherwise apply. If the intent is to mirror Node/patch-package behavior, decode file contents lossily (bytes + from_utf8_lossy) to keep patching robust and consistent with upstream semantics.

            // diffy::apply on a delete patch produces the empty
            // string when every hunk matches.
            let original = fs::read_to_string(&target)
                .map_err(|source| failed(format!("read {}: {source}", target.display())))?;

crates/patching/src/apply/tests.rs (1)

160-223: ⚡ Quick win

Assert that failed patch applications leave the filesystem untouched.

These tests prove the error path, but not the safety property. A regression could still create/modify an escaped ../... target or rewrite to-delete.txt before returning PatchFailed, and this suite would still pass. Please add postconditions that the escaped path is absent and that the stale-delete case preserves the original file contents.

Suggested assertions

 fn parent_dir_segment_in_create_errors() {
     let patched = tempdir().unwrap();
+    let escaped = patched.path().parent().unwrap().join("planted.txt");
     let patch_dir = tempdir().unwrap();
     let patch = write_patch(
         patch_dir.path(),
@@
     let PatchApplyError::PatchFailed { message, .. } = err else {
         panic!("expected PatchFailed, got: {err:?}");
     };
     assert!(message.contains("escapes target dir"), "got: {message}");
+    assert!(!escaped.exists(), "escape path must stay absent: {}", escaped.display());
 }
@@
 fn delete_with_mismatching_hunks_errors_without_unlinking() {
@@
     let err = apply_patch_to_dir(patched.path(), &patch).expect_err("must refuse mismatch");
     assert!(matches!(err, PatchApplyError::PatchFailed { .. }), "got: {err:?}");
     assert!(target.exists(), "file must NOT be unlinked when the patch doesn't match");
+    assert_eq!(fs::read_to_string(&target).unwrap(), "actually different content\n");
 }

Also applies to: 259-280

🤖 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 `@crates/patching/src/apply/tests.rs` around lines 160 - 223, Add postcondition
assertions to each test (parent_dir_segment_in_modify_errors,
parent_dir_segment_in_create_errors, parent_dir_segment_in_delete_errors) to
ensure failed patch applications leave the filesystem untouched: after calling
apply_patch_to_dir and asserting PatchFailed, verify that any escaped target
path (e.g., patched.path().join("../escape.txt"), "../planted.txt",
"../victim.txt" relative to patch_dir/patched) does not exist or was not
created, and for the delete test confirm the original file contents remain
unchanged (read the original file in patched before applying and assert its
contents are equal afterwards). Use the existing tempdir(), patched.path(),
patch_dir.path(), write_patch, and apply_patch_to_dir helpers to locate files
and perform the existence/content checks.

crates/package-manager/src/build_sequence.rs (1)

35-35: ⚡ Quick win

Use blob/main links in upstream references (not pinned SHAs).

Line 35 and Line 151 currently point to permalinked SHAs. Please switch these to https://github.com/pnpm/pnpm/blob/main/... to follow repo guidance for upstream-port references.

Suggested doc-link update

-/// [`getSubgraphToBuild`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/building/during-install/src/buildSequence.ts#L40-L67).
+/// [`getSubgraphToBuild`](https://github.com/pnpm/pnpm/blob/main/building/during-install/src/buildSequence.ts#L40-L67).
...
-/// `https://github.com/pnpm/pnpm/blob/b4f8f47ac2/building/during-install/src/buildSequence.ts`.
+/// `https://github.com/pnpm/pnpm/blob/main/building/during-install/src/buildSequence.ts`.

As per coding guidelines: “If reading on GitHub, open files from https://github.com/pnpm/pnpm/blob/main/... rather than from a permalinked SHA.”

Also applies to: 151-151

🤖 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 `@crates/package-manager/src/build_sequence.rs` at line 35, Update the upstream
GitHub links that currently use permalinked SHAs to use the blob/main form;
specifically, in the doc comments referencing getSubgraphToBuild and the other
upstream reference (the two occurrences in this file), replace URLs like
https://github.com/pnpm/pnpm/blob/<sha>/... with
https://github.com/pnpm/pnpm/blob/main/... so the links point to blob/main
instead of a pinned commit.