💖 Thanks for opening this pull request! 💖
Please be patient and we will get back to you as soon as we can.

My agent had a brain-fart on the commit comment in c99f6f9, hence why 475ef6b.

LOL, my agent reviewed my communication style on this comment

Never use casual slang like "brain-fart" on public project PRs. Use neutral technical language: "An error in an earlier commit message required a follow-up correction."

We had a conversation about owning self-made mistakes :)

Update on related issues #9073 and #9478:

After investigation, these are both the same root cause as this PR's fix — but in the v10 config reader, not v11:

• Run into problems while using packages like sqlite3 or better-sqlite3 that need bindings and build-tools #9073 (closed by zkochan): v10 --global installs ignore onlyBuiltDependencies. Workaround: pnpm approve-builds after install.
• PNPM package error, unable to find installed package #9478 (still open): Same v10 bug — sqlite3/better-sqlite3 postinstall scripts silently skipped during global install.

This PR does NOT close #9073 or #9478 — it fixes only the v11 GVS ordering bug (PR #11363).

The proper fix for #9073/#9478 is the v10 backport (adding else if (cliOptions['global']) to read workspace manifest from globalPkgDir), which is tracked separately.

GVS without an allow list kinda doesn't make sense at the moment unfortunately. We use the allow list to detect which packages should include the node.js version in the hash during installation. There's no other way to do it because the registry doesn't give us information about which dependencies should be built during resolution.

So, if you allow all builds, gvs will have all package duplicated for all node.js runtimes.

Production-ready bar for this PR

• Correctness: allowBuilds = {} default must NOT override workspace manifest or dangerouslyAllowAllBuilds from global config.yaml. The ordering guard allowBuilds == null && dangerouslyAllowAllBuilds !== true must be correct in all code paths (global workspace manifest present, absent, global config present, absent, and combinations).
• Architecture: The GVS default must run AFTER globalDepsBuildConfig re-application and AFTER workspace manifest reading. The fix preserves the existing phase ordering without introducing new coupling.
• No over-engineering: The guard is a 3-condition check — proportionate to the problem. No new abstractions are introduced.
• Comments: The comment on the GVS default block must accurately describe why the condition is ordered this way and what happens when each policy source is present.
• Tests: Both key scenarios must be tested: (1) workspace manifest allowBuilds + dangerouslyAllowAllBuilds coexist, (2) dangerouslyAllowAllBuilds alone without workspace manifest. Tests must clean up env and filesystem state.
• Performance: This is config-reading code (once per pnpm install), not a hot path. No performance concern.
• Operational safety: The change only reorders an existing default allowBuilds = {} assignment. It does not change the semantics of allowBuilds or dangerouslyAllowAllBuilds — the same values reach the same downstream functions. Safe to revert.
• Changeset accuracy: The changeset must not reference .npmrc (the bug is about global config.yaml / workspace manifest, not .npmrc).
• No scope creep: No unrelated refactors, no new abstractions, no changes outside the config reader.

Findings

1. Correctness & functional completeness

[NON-BLOCKING] Second test does not assert allowBuilds guard is NOT applied

• Type: Plausible risk
• Evidence: config/reader/test/index.ts:1793-1796
• Why it matters: The second test ("dangerouslyAllowAllBuilds is preserved when no workspace manifest exists") asserts config.allowBuilds is undefined and config.dangerouslyAllowAllBuilds is true. This verifies the guard's dangerouslyAllowAllBuilds !== true branch. However, it does NOT separately verify that when BOTH allowBuilds == null AND dangerouslyAllowAllBuilds !== true, the allowBuilds = {} default IS applied (the third branch of the guard). The first test does not test this branch either — it tests the case where allowBuilds is set by the workspace manifest, so the allowBuilds == null condition is false and the = {} default is not reached. A simple third test (GVS enabled, no workspace manifest, no dangerouslyAllowAllBuilds) would confirm the = {} default still fires when it should.
• Recommendation: Add a brief third test: global install with GVS enabled, no workspace manifest, no dangerouslyAllowAllBuilds → config.allowBuilds should be {} (engine-agnostic hashes for pure-JS). This is the normal case that was already working; a regression test confirms the reordering didn't break it.
• Confidence: Medium

[NIT] dangerouslyAllowAllBuilds !== true does not distinguish false from undefined

• Type: Plausible risk
• Evidence: config/reader/src/index.ts:504
• Why it matters: The guard dangerouslyAllowAllBuilds !== true leaves the allowBuilds = {} default active when dangerouslyAllowAllBuilds is false (explicitly set in config). If dangerouslyAllowAllBuilds: false is set alongside no workspace manifest, the result is allowBuilds: {}, which means "no packages can build" — the same semantic as dangerouslyAllowAllBuilds: false. This is correct behavior (allowBuilds: {} means "only packages listed may build" and the list is empty). But it's worth noting because the subtle distinction between false and undefined could matter if the semantics of allowBuilds: {} ever change.
• Recommendation: No code change needed. The behavior is correct. Consider adding a brief inline comment if this distinction comes up in review.
• Confidence: High (the behavior is correct; the concern is about future-proofing)

2. Architecture & boundary integrity

No issues found in this area based on the diff and reviewed context.

The fix moves 5 lines down ~120 lines past globalDepsBuildConfig re-application and workspace manifest reading. This is the correct architectural fix — the GVS default must come after all policy sources have been resolved. No new cross-layer coupling is introduced.

3. Code clarity, clean code & maintainability

[NIT] Guard condition is a 3-clause conjunction that requires careful reading

• Type: Plausible risk
• Evidence: config/reader/src/index.ts:502-505
• Why it matters: enableGlobalVirtualStore && allowBuilds == null && dangerouslyAllowAllBuilds !== true requires the reader to mentally trace 3 policy sources (GVS toggle, workspace manifest, global config) and understand that allowBuilds == null means "no workspace manifest policy" and dangerouslyAllowAllBuilds !== true means "no explicit permissive policy". The comment above helps, but the guard itself is dense.
• Recommendation: The comment is sufficient documentation. No code change needed — extracting this into a named function would be over-engineering for a 3-clause guard that is used exactly once.
• Confidence: High

4. Comments & code documentation

No issues found. The comment at config/reader/src/index.ts:495-500 accurately explains the why (engine-agnostic vs engine-specific hashes) and the when (only when no policy is configured). The test comments at lines 1755-1758 and 1792-1795 explain the regression context clearly.

5. Tests & validation

[NON-BLOCKING] First test does not verify that workspace manifest allowBuilds produces correct createAllowBuildFunction behavior

• Type: Plausible risk
• Evidence: config/reader/test/index.ts:1750-1751
• Why it matters: The test asserts config.allowBuilds equals { '@some/pkg': true, esbuild: true } after getConfig(). This verifies the config object, but not the downstream behavior. createAllowBuildFunction iterates Object.entries(opts.allowBuilds) — the test uses the Record<string, boolean> form which is correct. But the test doesn't call createAllowBuildFunction to verify that @some/pkg is allowed and other packages return undefined. This is a config-reader unit test, so testing the config object is appropriate scope; the build policy has its own tests. No change needed, but worth noting.
• Recommendation: No change needed — appropriate scope for a config-reader test.
• Confidence: High

[NIT] Test relies on import.meta.dirname for PNPM_HOME without documenting the path contract

• Type: Verified issue
• Evidence: config/reader/test/index.ts:36
• Why it matters: PNPM_HOME: import.meta.dirname means globalPkgDir resolves to <import.meta.dirname>/global/v11/. The first test creates a workspace manifest at that exact path. If the test file is ever moved, the path contract changes silently. This is a pre-existing convention used by all global tests in the file, not introduced by this PR.
• Recommendation: No change needed — existing convention.
• Confidence: High

6. Performance

No performance concerns. This is config-reading code executed once per pnpm install. The moved block is a simple conditional assignment.

7. Operational risk

No operational concerns. The change is a reordering of an existing default assignment. The same values flow to the same downstream functions (createAllowBuildFunction, graph-hasher). Safe to revert — the old behavior is restored by moving the block back up.

8. Adversarial review

Attempting to falsify: "What if globalDepsBuildConfig is empty?"

If globalDepsBuildConfig is an empty object (no dangerouslyAllowAllBuilds, no allowBuilds in config), then:

• extractAndRemoveDependencyBuildOptions strips both keys → globalDepsBuildConfig = {}
• hasDependencyBuildOptions(pnpmConfig) returns false (both keys are now null)
• Object.assign(pnpmConfig, globalDepsBuildConfig) → no-op (empty object)
• Guard: allowBuilds == null && dangerouslyAllowAllBuilds !== true → true
• Result: allowBuilds = {} ✅ Engine-agnostic hashes for pure-JS. Correct.

Attempting to falsify: "What if workspace manifest has allowBuilds AND dangerouslyAllowAllBuilds is set?"

This is exactly test 1. The workspace manifest sets allowBuilds via addSettingsFromWorkspaceManifestToConfig. dangerouslyAllowAllBuilds was already re-applied by Object.assign(pnpmConfig, globalDepsBuildConfig). Guard: allowBuilds == null → false. Result: allowBuilds keeps workspace manifest value, dangerouslyAllowAllBuilds stays true. ✅ Both coexist. Correct.

Attempting to falsify: "What if dangerouslyAllowAllBuilds: false?"

• extractAndRemoveDependencyBuildOptions strips dangerouslyAllowAllBuilds
• globalDepsBuildConfig = { dangerouslyAllowAllBuilds: false }
• hasDependencyBuildOptions(pnpmConfig) returns false (both keys null after stripping)
• Object.assign(pnpmConfig, globalDepsBuildConfig) → dangerouslyAllowAllBuilds = false
• Guard: allowBuilds == null && dangerouslyAllowAllBuilds !== true → true (false !== true)
• Result: allowBuilds = {} — means "no packages may build". Same behavior as dangerouslyAllowAllBuilds: false. ✅ Correct.

Attempting to falsify: "What about the !hasDependencyBuildOptions check above the guard?"

Line 492: if (!hasDependencyBuildOptions(pnpmConfig)) — this re-applies globalDepsBuildConfig. After this line, pnpmConfig has both dangerouslyAllowAllBuilds and allowBuilds re-set from globalDepsBuildConfig. THEN the GVS guard checks these values. The ordering is: re-apply → guard. ✅ Correct.

What assumption is most likely to be wrong?

The assumption that addSettingsFromWorkspaceManifestToConfig always sets allowBuilds when the workspace manifest contains it. Looking at the code (line 760-799): it iterates Object.entries(newSettings) and sets each key on pnpmConfig. This is a general-purpose "copy all settings" function — not specific to allowBuilds. The risk: if getOptionsFromPnpmSettings transforms or omits allowBuilds. But since the first test confirms config.allowBuilds has the expected value, this is verified. ✅

What test gives false confidence?

The first test uses { '@some/pkg': true, esbuild: true } — valid Record form. If a future contributor copies this test but uses array form ['@some/pkg', 'esbuild'], the test would pass (config object has the array) but createAllowBuildFunction would iterate Object.entries(['@some/pkg', 'esbuild']) → { '0': '@some/pkg', '1': 'esbuild' } → package names '0' and '1'. This is a pre-existing API concern, not introduced by this PR.

What I could not fully verify

• Full test suite execution: The pnpm-v11 monorepo requires node>=22.13 with node_modules installed. Prior sessions ran eslint (0 errors) and the @pnpm/config.reader tests (2 GVS tests passed, 20 global tests passed, 2 pre-existing failures in unrelated tests). CI is the authoritative verification.
• createAllowBuildFunction integration: The config-reader tests verify the config object. I did not verify that createAllowBuildFunction produces the expected AllowBuild function for each scenario, but this is out of scope for a config-reader unit test.
• E2E global install with GVS: No end-to-end test was run that actually performs pnpm add -g <pkg> with GVS enabled and verifies build scripts run. The unit tests verify the config layer; E2E verification would require a full pnpm build.

Final verdict

✅ Ready to merge — no blocking issues.

The fix is correct, proportionate, and well-tested for the two key scenarios. The guard ordering is verified by code tracing. The changeset is accurate. Two non-blocking suggestions (add a third test for the allowBuilds = {} default path, consider the false vs undefined edge case) are improvements, not merge gates.

@zkochan are you able to approve / restart the remaining hung(?) CI workflows? Thanks!

No actionable comments were generated in the recent review. 🎉

Learnt from: zkochan
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/test/publishedBy.test.ts:350-354
Timestamp: 2026-05-14T09:04:00.133Z
Learning: In the pnpm/pnpm repository, ESLint is the authoritative style linter. Do not raise review findings for missing trailing commas in multiline function calls (e.g., `fs.writeFileSync(...)`) when this repo’s ESLint configuration does not report them and lint passes. Prefer deferring to the ESLint results for this specific trailing-comma rule rather than enforcing it manually in code review.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

@coderabbitai review

Congrats on merging your first pull request! 🎉🎉🎉