REVIEW_GUIDE.md
How changes to pnpm/pnpm are reviewed, and how the decision to accept, reject, narrow, or
redesign a change is made — a framework a human or AI reviewer can apply to reach the same bar.
The central question for any PR:
Does this change solve a real pnpm problem, in the right layer, with a contained user-visible contract, and without unacceptable security, performance, compatibility, or maintenance cost — and is it the smallest correct version of itself?
This is the canonical review guide for the repository, applied by human reviewers and by two
automated reviewers — CodeRabbit (.coderabbit.yaml) and Qodo
(.pr_agent.toml). To cut duplicate comments the bots divide review depth —
CodeRabbit leads correctness and conventions, Qodo leads security and performance — but every
reviewer applies the same priority order below. AGENTS.md points here.
install, add, update, remove, resolution, fetching,
linking, lockfile handling, store access, and pnpr request paths are hot paths.Security is the first priority — review with a security-first lens. Treat manifests, lockfiles, registry metadata, tarballs, paths, environment variables, lifecycle scripts, workspace config, and git metadata as attacker-controlled. Surface plausible issues even when they're edge cases, but always explain the exploit path and impact on the changed code; never give generic security advice untethered from the diff. Security fixes themselves need precise threat modeling:
Treat as attacker-controlled: package metadata, tarball contents, lockfiles, workspace
manifests, .npmrc/environment config, registry responses, git URLs, filesystem paths, and
script names.
Look especially for:
configDependencies, patches, and workspace links;Advisory regression themes — recurring classes from past pnpm advisories:
.npmrc and pnpm-workspace.yaml must not expand victim environment secrets
into registry URLs, auth headers, proxy settings, token helpers, or other outbound requests;allowBuilds, ignored-build reporting,
explicit denials, and pacquet parity;resolution.commit,
refs, URLs); prevent command/argument injection and never pass attacker-controlled values as
executable flags;directories.bin,
transitive aliases, patch files, tar/zip entries, symlink/hardlink targets, file/git deps,
Windows path separators, executable shims, permission changes, and delete/write destinations;Recurring judgement calls:
wx/exclusive-create is not atomic — a crash mid-write
leaves an invalid file in the store. Use temp-file-plus-rename, or write package.json last
as a completion marker..npmrc/workspace config must not expand victim secrets into outbound
registry/auth/proxy values. Token helpers are executable code — only from trusted/user
config. Suggested copy-paste commands must not embed shell-expandable attacker-controlled keys.Don't overreact to audit output. An advisory on dev-only/non-runtime code, or on functions pnpm doesn't call, may be safe to ignore; don't force a semver-breaking update to silence a non-exploitable warning. A misconfigured consumer is not a pnpm bug.
Security defaults still need a performance design. A gate that adds a registry request per lockfile package needs a cache/fast path (e.g. a per-lockfile cache keyed on the lockfile hash) before it ships as a default.
pnpm is performance-sensitive; be skeptical of extra work in common flows.
Reject or redesign changes that add thousands of filesystem ops per install, a network round trip per dependency, full metadata fetches where smaller/cached data would do, repeated parsing/scans in resolver/linker loops, noisy logs or expensive checks on the warm path, or broad fast-path invalidation for rare cases. Prefer cheap markers and existing invariants.
Benchmarks should use multiple runs (with variance when the gap is small); hot/warm/cold store and cold-install rows where relevant; realistic dependency counts (2000–3000 packages can be a small project); and evidence the win is on a common path. A negligible gain that adds complexity isn't worth it — and the same evidence justifies refusing an optimization (cost is sub-0.1% or off the hot path).
Many PRs are closed not because they're buggy but because the change doesn't make sense for pnpm. Before line-by-line review, ask:
The single most useful question is "What is the benefit of this?" When you reject, say why in a sentence or two and link the superseding PR if one exists.
Push back on changes that are mostly churn:
Judge a change on the change itself; how it was authored is not a review criterion.
One PR does one thing.
For every touched file, ask "why was this needed for this PR?" If it isn't obvious from the goal, it comes out.
When matching npm functionality, use npm's recognized command name and behavior unless there's
a deliberate reason to differ (npm view → pnpm view, not a new name or alias). Support the
spec forms users expect (foo@2, dist-tags) without duplicating resolver logic. Don't copy
npm's command sprawl.
Prior art from npm, Yarn, vlt, etc. is useful but not automatic justification. Check that syntax and semantics genuinely match pnpm. Steer users to the right existing mechanism instead.
Changing a default usually needs a major version, a strong user signal (a poll or widespread demand), or an opt-in setting first.
Don't add large or noisy logs unless the user can act on them.
minimumReleaseAge and trustPolicy.Avoid fixes that make unrelated packages learn resolver-specific details; prefer a precise gate where the ambiguous value is consumed.
Duplication is a frequent problem in this monorepo. Search packages/, fs/, crypto/,
text/, default-reporter, and the manifest/lockfile utilities first, and prefer a maintained
package over a custom reimplementation of parsing, serialization, or shell escaping. But reuse
is the goal, not abstraction for its own sake — don't extract an indirection that doesn't
reduce coupling.
Tests must prove the changed behavior, not just execute nearby code.
getIntegrity()); don't depend on unreleased Node.js behavior."pnpm" explicitly with the right bump: patch (bug fix / internal),
minor (feature / setting), major (breaking).pnpm is the source of truth; pacquet is the Rust port that matches it exactly. Every command should have a pacquet equivalent, and every user-visible change must land in both products — flags, defaults, env handling, error codes/messages, lockfile shape, store layout, build policy, lifecycle behavior, config handling, output. A change without its pacquet counterpart is incomplete.
Before adding a dependency or writing custom logic: check for an existing repo utility; compare maintained packages that already solve it; add it at the narrowest package that needs it; avoid heavy dependencies for small conveniences; and don't hand-roll serialization, parsing, or shell escaping when a proven library or local helper exists.
The voice is short, direct, and specific.
suggestion blocks for exact wording.For each PR, in order:
"pnpm" included; one per change; accurate. (§8)PnpmError, no swallowed errors, good names, reused libraries, correct
dependency placement, config through options. (AGENTS.md → Conventions)A change is mergeable when it is the smallest correct, secure, in-scope version of a thing pnpm should do, in the right layer, proven by a meaningful test, documented if user-visible, and mirrored in pacquet.