crates/package-manager/src/build_modules.rs (2)

405-429: 💤 Low value

The #[allow(clippy::too_many_arguments)] is justified but could be documented.

The 19 parameters are the natural result of extracting the loop body for parallel dispatch. Refactoring into a parameter struct would add complexity for this internal helper without meaningful benefit. Consider adding a brief inline comment explaining the rationale:

+// Extracted loop body for parallel dispatch; parameters mirror the outer scope bindings.
 #[allow(clippy::too_many_arguments)]
 fn build_one_snapshot<R: Reporter>(

🤖 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.rs` around lines 405 - 429, The
#[allow(clippy::too_many_arguments)] on build_one_snapshot is justified; add a
concise inline comment above the attribute or function explaining that the 19
parameters are intentional because this helper was extracted from a loop for
parallel dispatch and that wrapping them into a struct would add indirection and
complexity for an internal helper (mentioning build_one_snapshot, the parallel
dispatch context, and why refactoring is undesirable) so future maintainers
understand the rationale.

---

355-358: 💤 Low value

Thread pool creation panic is acceptable but could be more resilient.

The .expect() on ThreadPoolBuilder::build() will panic if the OS refuses to spawn threads (resource exhaustion, ulimit, etc.). While rare, this is a runtime failure outside of developer control.

Consider propagating the error or at least logging before panicking. However, given that upstream pnpm's runGroups also doesn't gracefully handle worker pool failures, this matches upstream behavior and the panic message is descriptive enough.

🤖 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.rs` around lines 355 - 358, The
current code calls ThreadPoolBuilder::new().num_threads(child_concurrency.max(1)
as usize).build().expect(...), which will panic if the OS refuses to create
threads; update this to handle the Result from ThreadPoolBuilder::build()
instead of unwrapping: either propagate the error (return a Result from the
surrounding function) or call build().map_err(|e| {
process_logger.error(format!("failed to build rayon thread pool: {}", e)); e })?
and then propagate or abort; at minimum replace expect(...) with proper error
handling around build() referencing ThreadPoolBuilder::build, the pool variable,
and child_concurrency so the failure can be logged or returned upstream rather
than unconditionally panicking.

Fix all unresolved CodeRabbit comments on this PR:

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

Fix all unresolved CodeRabbit comments on this PR:

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

crates/config/src/defaults.rs (2)

109-120: ⚡ Quick win

Use blob/main for upstream pnpm references instead of pinned SHAs.

These inline links currently point to a specific commit hash, which makes long-term porting/cross-checking drift-prone in this repo. Please switch them to https://github.com/pnpm/pnpm/blob/main/....

As per coding guidelines: "If reading on GitHub, open files from https://github.com/pnpm/pnpm/blob/main/... (which always resolves to the tip of main) rather than from a permalinked SHA."

Also applies to: 126-127, 134-134, 148-148

🤖 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/config/src/defaults.rs` around lines 109 - 120, Update the inline
GitHub permalinks in the doc comments to point to the moving tip of main instead
of a SHA: replace occurrences of "https://github.com/pnpm/pnpm/blob/<sha>/..."
with "https://github.com/pnpm/pnpm/blob/main/..." in the comments surrounding
default_child_concurrency, default_child_concurrency_with_parallelism and the
referenced getDefaultWorkspaceConcurrency links (also update the other similar
comment links nearby). Ensure all doc comment links in this module use blob/main
so they track upstream main.

---

117-123: ⚡ Quick win

Narrow visibility for test-only helpers.

default_child_concurrency_with_parallelism and resolve_child_concurrency_with_parallelism are described as internal test helpers but are declared pub. Consider reducing to pub(crate) (or private) to avoid unnecessary public API exposure.

Suggested minimal change

-pub fn default_child_concurrency_with_parallelism(parallelism: u32) -> u32 {
+pub(crate) fn default_child_concurrency_with_parallelism(parallelism: u32) -> u32 {
     parallelism.min(4)
 }

-pub fn resolve_child_concurrency_with_parallelism(option: Option<i32>, parallelism: u32) -> u32 {
+pub(crate) fn resolve_child_concurrency_with_parallelism(option: Option<i32>, parallelism: u32) -> u32 {
     match option {
         None => default_child_concurrency_with_parallelism(parallelism),
         Some(n) if n > 0 => n as u32,
         Some(n) => parallelism.saturating_sub(n.unsigned_abs()).max(1),
     }
 }

Also applies to: 146-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/config/src/defaults.rs` around lines 117 - 123, The two test-only
helper functions default_child_concurrency_with_parallelism and
resolve_child_concurrency_with_parallelism are declared public but should have
narrower visibility; change their declarations from pub to pub(crate) (or remove
pub entirely if only used within the crate) so they are not exposed in the
public API, updating the function signatures for
default_child_concurrency_with_parallelism and
resolve_child_concurrency_with_parallelism accordingly.

crates/config/src/defaults/tests.rs (1)

53-53: ⚡ Quick win

Normalize upstream test-reference links to blob/main.

The new upstream references in test docs are pinned to a commit SHA. Please switch them to blob/main for consistency with the repo’s pnpm-porting rule.

As per coding guidelines: "If reading on GitHub, open files from https://github.com/pnpm/pnpm/blob/main/... (which always resolves to the tip of main) rather than from a permalinked SHA."

Also applies to: 61-61, 69-69, 77-77, 89-89, 99-99, 118-118

🤖 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/config/src/defaults/tests.rs` at line 53, The test doc links are
pinned to a commit SHA; update each GitHub URL that points to
"github.com/pnpm/pnpm/blob/<commit-sha>/..." and replace the "<commit-sha>"
segment with "main" so they become "github.com/pnpm/pnpm/blob/main/...". Locate
the referenced link near the comment text '`getDefaultWorkspaceConcurrency: cpu
num < 4`' and apply the same change to the other occurrences in this file (the
equivalent link anchors referenced in the comments around the other test
descriptions) so all upstream pnpm links use blob/main.

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

176-177: ⚡ Quick win

Use pnpm/blob/main for upstream test reference links

Please switch the doc link to https://github.com/pnpm/pnpm/blob/main/... so the reference always points to current upstream behavior.

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

🤖 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` around lines 176 - 177,
Update the upstream reference URL in the doc comment inside
crates/package-manager/src/build_sequence/tests.rs so it uses the branch-based
path (/blob/main/...) instead of the permalinked SHA; locate the comment block
containing the link to https://github.com/pnpm/pnpm/blob/b4f8f47ac2/... and
replace that SHA segment with main so the doc link always points to current
upstream behavior.

Fix all unresolved CodeRabbit comments on this PR:

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