fix(install): try with HOME env dir if no npmrc found in XDG_CONFIG_HOME#26040
fix(install): try with HOME env dir if no npmrc found in XDG_CONFIG_HOME#26040shm0x wants to merge 17 commits intooven-sh:mainfrom
Conversation
WalkthroughThe npmrc configuration file path resolution now prioritizes environment variables XDG_CONFIG_HOME and HOME with existence checks, replacing the previous data_dir-based path construction with a conditional fallback to ".npmrc" when no config directory path is found. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
This PR solves the following issues:
Please consider checking the fix or address the underlying bug |
|
We've hit this bug on multiple CI pipelines and local dev environments using |
|
🤖 Claude is monitoring this PR I'll review code, respond to comments, and investigate CI failures.
|
|
Newest first ✅ 1d768 — Looks good! Reviewed 2 files across Previous reviews✅ 0d072 — Looks good! Reviewed 2 files across ✅ Reviewed 2 files across ✅ Reviewed 2 files across 🟡
|
|
No description provided. |
There was a problem hiding this comment.
LGTM — straightforward bug fix with good test coverage for the fallback behavior.
Extended reasoning...
Overview
This PR modifies two files: src/install/PackageManager.zig (+24/-21 lines) and test/cli/install/npmrc.test.ts (+74 lines). The Zig change fixes the .npmrc config file lookup logic so that when XDG_CONFIG_HOME is set but contains no .npmrc, the code falls back to HOME/.npmrc instead of silently failing. Previously, XDG_CONFIG_HOME.get() orelse HOME.get() would short-circuit if XDG was set, regardless of whether an .npmrc file existed there. The fix adds an existence check (bun.sys.existsZ) on the XDG path before using it, and falls back to HOME if no file is found. The install context creation was also de-duplicated.
Security risks
None. The change only affects which config file path is passed to loadNpmrcConfig. It does not introduce new file operations, network calls, or permission changes. The paths are derived from environment variables that were already being used.
Level of scrutiny
This is a small, self-contained bug fix in the package manager config loading path. The logic is straightforward (check file existence, fall back). The change is low risk and the tests adequately cover the two key scenarios: (1) XDG set without .npmrc falls back to HOME, and (2) XDG with .npmrc is preferred over HOME.
Other factors
The PR resolves multiple reported issues (#24124, #23128, #22971) and has community demand. Two tests are added covering the fallback and priority behaviors. No CODEOWNERS are defined for the install directory. The CLAUDE.md requirement for tests is satisfied. Previous reviews from the monitoring bot already marked the latest commits as good.
What does this PR do?
In github CI workflows, the XDG_CONFIG_HOME is set to
/home/runner/.config.When using .npmrc in HOME (the normal behavior), the current implementation will never look there if XDG_CONFIG_HOME is set and as there is no .npmrc in
/home/runner/.config, we cannot install or publish private packages.The workaround is to copying the $HOME/.npmrc locally or worst unsetting XDG_CONFIG_HOME but could break others programs.. I think this should work out of the box.
This PR will check if .npmrc exists in both places $XDG_CONFIG_HOME/.npmrc then $HOME/.npmrc.
Previously, the code used
XDG_CONFIG_HOME.get() orelse HOME.get()which meant if XDG_CONFIG_HOME was defined but no .npmrc existed there, it would never fall back to check HOME/.npmrc.How did you verify your code works?
Setup: XDG_CONFIG_HOME is set but .npmrc is in HOME