fix(config): gracefully handle configDependencies errors during config commands#11362
Merged
Conversation
2f244f9 to
ba3bcd7
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes pnpm CLI config-related commands so they don’t crash when configDependencies installation fails (e.g. missing auth token), allowing pnpm config set/get (and the pnpm set/get entrypoints) to proceed and update/read .npmrc.
Changes:
- Add an option to
installConfigDepsAndLoadHooksto catchconfigDependenciesinstall errors and log them at debug level. - Enable this error-tolerant mode for
pnpm config,pnpm set, andpnpm getin the CLI entrypoint. - Add unit + e2e regression coverage and a changeset entry.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pnpm/src/getConfig.ts |
Adds catchConfigDependenciesErrors option, debug logging on failure, and guards pnpmfile path prepending. |
pnpm/src/main.ts |
Enables catching configDependencies install errors for config/set/get command entrypoints. |
pnpm/test/getConfig.test.ts |
Unit tests for installConfigDepsAndLoadHooks success vs. caught vs. thrown failure behavior. |
pnpm/test/configurationalDependencies.test.ts |
E2E regression tests ensuring config writes/reads work even when configDependencies fails. |
.changeset/major-hats-press.md |
Records the patch-level change for release notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Running `pnpm config set` triggers `resolveAndInstallConfigDeps` before the new config (e.g. auth token) is saved to `.npmrc`, causing a 401 crash when the config dependency is hosted in a private registry. This patch adds a `catchConfigDependenciesErrors` flag to `installConfigDepsAndLoadHooks`. For `cmd === 'config'`, `'set'` and `'get'`, installation failures are now caught and logged at debug level so the command can proceed and actually write / read the auth token. When the install fails, plugin pnpmfile paths are not prepended either (they don't exist on disk yet), so `requireHooks` won't blow up with PNPMFILE_NOT_FOUND. `pnpm set` and `pnpm get` are separate top-level commands whose handlers delegate to the `config` command internally, so they are explicitly listed alongside `'config'` — users can hit pnpm#10684 via any of the three entry points. Covers the fix with: - Unit tests in pnpm/test/getConfig.test.ts exercising the `installConfigDepsAndLoadHooks` contract (success, caught-error, default-throw). - Three e2e tests in pnpm/test/configurationalDependencies.test.ts that spawn the real pnpm CLI against a bogus configDependency and assert each entry point (`pnpm config set`, `pnpm set`, `pnpm get`) still works. Fixes pnpm#10684
ba3bcd7 to
9ffa6f4
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesConfig Dependency Error Tolerance
Sequence DiagramsequenceDiagram
autonumber
participant Client
participant CLI
participant getConfig
participant Installer
participant Store
participant Logger
Client->>CLI: run command (config / set / get or other)
CLI->>getConfig: installConfigDepsAndLoadHooks(opts.tolerateConfigDependenciesErrors = cmd in {config,set,get})
getConfig->>Store: create store controller
getConfig->>Installer: resolveAndInstallConfigDeps(...)
alt install succeeds
Installer-->>getConfig: success
getConfig->>Store: ensure store closed
getConfig-->>CLI: return hooks/config
else install fails
Installer-->>getConfig: throws error
alt tolerateConfigDependenciesErrors = true
getConfig->>Logger: logger.debug("Failed to install configDependencies", error)
getConfig->>Store: ensure store closed
getConfig-->>CLI: continue without installed config deps
else
getConfig-->>CLI: rethrow error (command fails)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Address review feedback on PR pnpm#11362: - Replace the configDepsInstalled boolean with on-disk existence checks in calcPnpmfilePathsOfPluginDeps so already-installed config dep hooks from a previous run are still picked up when a subsequent install fails transiently. - Add e2e coverage for `pnpm config get` to match the existing `pnpm config set`, `pnpm set`, and `pnpm get` cases.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Per review feedback on PR pnpm#11362: the outer try/catch around createStoreController + resolveAndInstallConfigDeps + the close in finally was swallowing store creation and close errors when catchConfigDependenciesErrors was enabled, mislabeling them as install failures. Move createStoreController out of the catch and wrap only resolveAndInstallConfigDeps, while keeping the close in finally. Adds unit tests verifying store creation and close errors propagate even when catchConfigDependenciesErrors is true.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@pnpm/test/getConfig.test.ts`:
- Around line 123-135: Move the helper function buildBaseConfig so it is
declared after the tests that call it within the same describe block: locate all
usages of buildBaseConfig in the describe block and cut the current
buildBaseConfig declaration, then paste it below those call sites (after the
last test that uses it) so the file follows the hoisting-based ordering rule;
ensure the function name and signature (buildBaseConfig(): { config: Config,
context: ConfigContext }) remain unchanged and run tests to verify no
regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c7f378c7-e490-4205-8c00-217de5e73e84
📒 Files selected for processing (2)
pnpm/src/getConfig.tspnpm/test/getConfig.test.ts
Per repo style rule (functions are declared after they are used, relying on hoisting). Addresses CodeRabbit review feedback on PR pnpm#11362.
…figDependenciesErrors `tolerate` describes the behavior intent (proceed despite the failure) rather than the low-level mechanism (catch). No functional change.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…inally
The outer `try { inner-try-catch } finally { close }` and inner
`try { install } catch { log }` are equivalent to a flat
`try { install } catch { log } finally { close }`. createStoreController
stays outside the try so its errors still propagate; close stays in
finally so its errors aren't matched by catch.
The guard in main.ts triggers for any `pnpm config` subcommand (not just set/get), plus `pnpm set` and `pnpm get`. Update the changeset to match.
zkochan
approved these changes
May 5, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…is missing Address review feedback: previously calcPnpmfilePathsOfPluginDeps silently skipped any plugin without an on-disk pnpmfile, which masked misconfigured plugin packages (plugin dir present but no pnpmfile). Now the function only skips when the plugin directory is missing entirely (install didn't happen). When the directory exists but neither pnpmfile.mjs nor pnpmfile.cjs is present, the .cjs path is still yielded so requireHooks can surface PNPMFILE_NOT_FOUND for the misconfigured plugin.
zkochan
added a commit
that referenced
this pull request
May 5, 2026
…g commands (#11362) Closes #10684. Running `pnpm config set` triggers `resolveAndInstallConfigDeps` before the new config (e.g. auth token) is saved to `.npmrc`, causing a 401 crash when the config dependency is hosted in a private registry. This patch adds a `tolerateConfigDependenciesErrors` flag to `installConfigDepsAndLoadHooks`. For `cmd === 'config'`, `'set'` and `'get'`, installation failures are now caught and logged at debug level so the command can proceed and actually write / read the auth token. `pnpm set` and `pnpm get` are separate top-level commands whose handlers delegate to the `config` command internally, so they are explicitly listed alongside `'config'`; users can hit #10684 via any of the three entry points. Plugin pnpmfile paths from `configDependencies` are resolved by checking each file's existence on disk, so previously-installed hooks survive a transient install failure and `requireHooks` won't throw `PNPMFILE_NOT_FOUND` when nothing has been installed yet. Covers the fix with: - Unit tests in `pnpm/test/getConfig.test.ts` exercising the `installConfigDepsAndLoadHooks` contract (success, tolerated error, rethrown error, store creation/close errors propagating) and the on-disk pnpmfile resolution behavior. - Four e2e tests in `pnpm/test/configurationalDependencies.test.ts` that spawn the pnpm CLI against a bogus `configDependency` and assert each entrypoint (`pnpm config set`, `pnpm config get`, `pnpm set`, `pnpm get`) still works. --------- Co-authored-by: Zoltan Kochan <z@kochan.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #10684. Running
pnpm config settriggersresolveAndInstallConfigDepsbefore the new config (e.g. auth token) is saved to.npmrc, causing a 401 crash when the config dependency is hosted in a private registry.This patch adds a
tolerateConfigDependenciesErrorsflag toinstallConfigDepsAndLoadHooks. Forcmd === 'config','set'and'get', installation failures are now caught and logged at debug level so the command can proceed and actually write / read the auth token.pnpm setandpnpm getare separate top-level commands whose handlers delegate to theconfigcommand internally, so they are explicitly listed alongside'config'; users can hit #10684 via any of the three entry points.Plugin pnpmfile paths from
configDependenciesare resolved by checking each file's existence on disk, so previously-installed hooks survive a transient install failure andrequireHookswon't throwPNPMFILE_NOT_FOUNDwhen nothing has been installed yet.Covers the fix with:
pnpm/test/getConfig.test.tsexercising theinstallConfigDepsAndLoadHookscontract (success, tolerated error, rethrown error, store creation/close errors propagating) and the on-disk pnpmfile resolution behavior.pnpm/test/configurationalDependencies.test.tsthat spawn the pnpm CLI against a bogusconfigDependencyand assert each entrypoint (pnpm config set,pnpm config get,pnpm set,pnpm get) still works.