feat: add NODE_NPM_INSTALL_FLAGS config key for scoped npm flags#6
Conversation
Add a new ~/.updatesrc key NODE_NPM_INSTALL_FLAGS that inserts extra flags into the node module's `npm install -g` invocation. This lets users scope flags like --legacy-peer-deps to the updates tool instead of setting them globally in ~/.npmrc. Default is empty (no behavior change). Implemented in both the Bash and native Windows PowerShell runtimes with corresponding dry-run output and tests.
Reviewer's GuideAdds a NODE_NPM_INSTALL_FLAGS configuration key to scope extra npm install flags for the node module, wires it through Bash and PowerShell implementations including dry-run and retry paths, hardens Windows test fixtures, and updates docs and tests accordingly. Sequence diagram for NODE_NPM_INSTALL_FLAGS in npm global install with ERESOLVE retrysequenceDiagram
actor User
participant InvokeModuleNode
participant InvokeNpmGlobalInstall
participant GetNpmInstallExtraFlags
participant GetNpmInstallRetryOptions
participant InvokeLoggedProcess
participant CompleteNpmGlobalInstallSuccess
participant npm
User->>InvokeModuleNode: invoke node module
InvokeModuleNode->>InvokeNpmGlobalInstall: Invoke-NpmGlobalInstall
InvokeNpmGlobalInstall->>GetNpmInstallExtraFlags: Get-NpmInstallExtraFlags
GetNpmInstallExtraFlags-->>InvokeNpmGlobalInstall: extraFlags
InvokeNpmGlobalInstall->>InvokeLoggedProcess: Invoke-LoggedProcess (npm install -g extraFlags -- packages)
InvokeLoggedProcess->>npm: run
npm-->>InvokeLoggedProcess: ExitCode, Output
InvokeLoggedProcess-->>InvokeNpmGlobalInstall: installResult
alt [installResult.ExitCode -eq 0]
InvokeNpmGlobalInstall->>CompleteNpmGlobalInstallSuccess: Complete-NpmGlobalInstallSuccess(Npm, Options extraFlags, Packages, Result)
CompleteNpmGlobalInstallSuccess-->>InvokeNpmGlobalInstall: status 0
InvokeNpmGlobalInstall-->>InvokeModuleNode: 0
else [installResult.Output -match ERESOLVE]
InvokeNpmGlobalInstall->>GetNpmInstallRetryOptions: Get-NpmInstallRetryOptions(Options extraFlags)
GetNpmInstallRetryOptions-->>InvokeNpmGlobalInstall: retryOptions (dedup --legacy-peer-deps)
InvokeNpmGlobalInstall->>InvokeLoggedProcess: Invoke-LoggedProcess (npm install -g retryOptions -- packages)
InvokeLoggedProcess->>npm: run
npm-->>InvokeLoggedProcess: retryResult
InvokeLoggedProcess-->>InvokeNpmGlobalInstall: retryResult
alt [retryResult.ExitCode -eq 0]
InvokeNpmGlobalInstall->>CompleteNpmGlobalInstallSuccess: Complete-NpmGlobalInstallSuccess(Npm, Options retryOptions, Packages, Result)
CompleteNpmGlobalInstallSuccess-->>InvokeNpmGlobalInstall: status 0
InvokeNpmGlobalInstall-->>InvokeModuleNode: 0
else [retryResult.ExitCode -ne 0]
InvokeNpmGlobalInstall-->>InvokeModuleNode: retryResult.ExitCode
end
else [no ERESOLVE]
InvokeNpmGlobalInstall-->>InvokeModuleNode: installResult.ExitCode
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughAdds a new ChangesNODE_NPM_INSTALL_FLAGS feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- When handling the ERESOLVE retry in
Invoke-NpmGlobalInstall, you always append--legacy-peer-depsin addition to any user-suppliedNodeNpmInstallFlags, which can lead to duplicate flags if the user also specifies--legacy-peer-deps; consider checking for that flag first or normalizing the combined flag list.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When handling the ERESOLVE retry in `Invoke-NpmGlobalInstall`, you always append `--legacy-peer-deps` in addition to any user-supplied `NodeNpmInstallFlags`, which can lead to duplicate flags if the user also specifies `--legacy-peer-deps`; consider checking for that flag first or normalizing the combined flag list.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR adds an opt-in ~/.updatesrc configuration key (NODE_NPM_INSTALL_FLAGS) to pass additional flags to the node module’s npm install -g invocation, implemented in both the Bash (updates) and native Windows PowerShell (updates-main.ps1) runtimes, and documented/tested accordingly.
Changes:
- Add
NODE_NPM_INSTALL_FLAGSconfig parsing and injection intonpm install -gargument construction (Bash + PowerShell). - Update dry-run output to display the effective
npm install -gcommand including the configured flags. - Document the new key in
SPEC.md,README.md, andCHANGELOG.md, and add regression tests for Bash and Windows-native runtimes.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
updates-main.ps1 |
Adds $script:NodeNpmInstallFlags, parses config, and injects split flags into npm global install + dry-run output. |
updates |
Adds NODE_NPM_INSTALL_FLAGS, parses config, and word-splits flags into npm global install + dry-run output. |
tests/test_windows_native.ps1 |
Adds a Windows-native test asserting node dry-run output includes configured npm flags. |
tests/test_cli.sh |
Adds Bash tests asserting node dry-run output includes/omits flags depending on config. |
SPEC.md |
Documents the new NODE_NPM_INSTALL_FLAGS config key in the config table. |
README.md |
Adds example usage and a short explanation of when to use NODE_NPM_INSTALL_FLAGS. |
CHANGELOG.md |
Notes the new config key under [Unreleased]. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
| Filename | Overview |
|---|---|
| updates | Adds NODE_NPM_INSTALL_FLAGS parsing and noglob-protected word-split into run_npm_global_install; introduces counter variables alongside arrays for nounset/Bash-3.2 compatibility; ERESOLVE retry correctly deduplicates --legacy-peer-deps; json_emit_summary refactored to avoid local array under nounset. One cosmetic gap: dry-run log shows raw config string rather than normalised flags. |
| updates-main.ps1 | Adds Get-NpmInstallExtraFlags and Get-NpmInstallRetryOptions helpers; NodeNpmInstallFlags wired into Invoke-NpmGlobalInstall and Invoke-ModuleNode; --legacy-peer-deps deduplication on ERESOLVE retry is correct; findstr calls hardened to full path. |
| tests/test_cli.sh | Adds dry-run with/without NODE_NPM_INSTALL_FLAGS tests, nounset compatibility test, and a glob-literal test that creates a file matching --flag=* to confirm glob protection; deduplication assertion on the ERESOLVE retry path. |
| tests/test_windows_native.ps1 | Removes CI env var before self-update fixture tests; adds NODE_NPM_INSTALL_FLAGS dry-run and retry-deduplication test cases; existing findstr calls updated to full %SystemRoot%\System32\findstr.exe path; regex patterns updated for optional \r? to handle CRLF line endings. |
| SPEC.md | NODE_NPM_INSTALL_FLAGS added to config table with correct type, default, and example; node module spec updated to document flag insertion, whitespace-split, ERESOLVE dedup, and allow-scripts flag retention. |
| README.md | NODE_NPM_INSTALL_FLAGS added to the .updatesrc example block and documented in prose; wording is clear and accurate. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[run_npm_global_install called] --> B{NODE_NPM_INSTALL_FLAGS set?}
B -- No --> C[npm_install_options = --]
B -- Yes --> D[set -f, word-split flags, set +f]
D --> E[npm_extra_flags = parsed flags
npm_install_options = flags --]
C --> F[set -- packages restore]
E --> F
F --> G{DRY_RUN?}
G -- Yes --> H[returns 0 via run wrapper]
G -- No --> I[mktemp npm_err_log]
I --> J[run_npm_global_install_logged
npm install -g options packages]
J --> K{exit 0?}
K -- Yes --> L[handle_npm_global_install_success
allow-scripts retry if needed]
K -- No --> M{ERESOLVE in err log?}
M -- No --> N[cat err, return status]
M -- Yes --> O[Build retry_flags:
remove --legacy-peer-deps
append --legacy-peer-deps once]
O --> P[run_npm_global_install_logged retry]
P --> Q{exit 0?}
Q -- Yes --> R[handle_npm_global_install_success
with retry_flags]
Q -- No --> S[cat err, return status]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[run_npm_global_install called] --> B{NODE_NPM_INSTALL_FLAGS set?}
B -- No --> C[npm_install_options = --]
B -- Yes --> D[set -f, word-split flags, set +f]
D --> E[npm_extra_flags = parsed flags
npm_install_options = flags --]
C --> F[set -- packages restore]
E --> F
F --> G{DRY_RUN?}
G -- Yes --> H[returns 0 via run wrapper]
G -- No --> I[mktemp npm_err_log]
I --> J[run_npm_global_install_logged
npm install -g options packages]
J --> K{exit 0?}
K -- Yes --> L[handle_npm_global_install_success
allow-scripts retry if needed]
K -- No --> M{ERESOLVE in err log?}
M -- No --> N[cat err, return status]
M -- Yes --> O[Build retry_flags:
remove --legacy-peer-deps
append --legacy-peer-deps once]
O --> P[run_npm_global_install_logged retry]
P --> Q{exit 0?}
Q -- Yes --> R[handle_npm_global_install_success
with retry_flags]
Q -- No --> S[cat err, return status]
Reviews (2): Last reviewed commit: "fix: guard empty arrays under nounset" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Implemented the three Greptile findings from the fresh review pass. Changed:
Validation:
Residual risk:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@updates`:
- Around line 707-711: The `npm_extra_flags` array expansions in the npm install
flow are not fully guarded for `set -u`, which can make an empty array behave as
unbound in Bash 3.2. Update the call sites around
`run_npm_global_install_logged` and `handle_npm_global_install_success`, and any
related loop using `npm_extra_flags`, to use the `${npm_extra_flags[@]+...}`
pattern so the default `NODE_NPM_INSTALL_FLAGS` path cannot abort before `npm
install`.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e1cef587-19b7-4e05-b75e-407fb3caa053
📒 Files selected for processing (8)
CHANGELOG.mdPLAN.mdREADME.mdSPEC.mdtests/test_cli.shtests/test_windows_native.ps1updatesupdates-main.ps1
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: windows-powershell-smoke
- GitHub Check: windows-powershell-smoke
🧰 Additional context used
📓 Path-based instructions (6)
**/CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Add a
CHANGELOG.mdentry for each release
Files:
CHANGELOG.md
**/PLAN.md
📄 CodeRabbit inference engine (AGENTS.md)
Maintain
PLAN.mdas living execution checklist; keep it updated as work lands
Files:
PLAN.md
**/SPEC.md
📄 CodeRabbit inference engine (AGENTS.md)
Maintain
SPEC.mdas living contract for flags/output/modules
Files:
SPEC.md
**/updates
📄 CodeRabbit inference engine (AGENTS.md)
**/updates: Don't print success messages for failed commands in the updates script
Add new functionality as a module function (module_<name>()) and register it inis_module_known(),module_description(),list_modules(), andrun_selected_modules()
Modules must be auto-detected and skip gracefully if the backing command is missing, unless--onlyis used (then missing deps are an error)
For--jsoncontract: stdout must be JSONL-only; route human output to stderr
KeepUPDATES_VERSIONinupdatesaligned with the latest release tag
Useconfig_set_boolfor adding boolean config keys in the updates script
Files:
updates
**/*.sh
📄 CodeRabbit inference engine (AGENTS.md)
**/*.sh: Validate scripts using./scripts/lint.shwhich runsbash -n,shellcheck, andshfmt -d
Assume Bash 3.2 compatibility (macOS system Bash); avoid Bash 4+ features
Prefer--dry-runsupport for anything that might mutate state
Start every logical section with a triple-hash banner:### SECTION: <name> — <description>
Files:
tests/test_cli.sh
**/tests/**/*.sh
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.sh: Run tests via./scripts/test.shwhich executes./tests/test_cli.sh
Keep output stable and easy to parse; prefer--no-emojiin tests
Files:
tests/test_cli.sh
🪛 ast-grep (0.44.0)
tests/test_cli.sh
[warning] 983-983: set +e (or set +o errexit) disables the shell's errexit option, so the script keeps running after a command fails. This masks failures of security-critical operations (downloads, signature/checksum verification, permission changes, cleanup of secrets), letting the script proceed with a bad or insecure state. Leave errexit enabled (set -e / set -euo pipefail), or handle failures explicitly with if/|| and an explicit exit instead of globally turning off failure detection.
Context: set +e
Note: [CWE-754] Improper Check for Unusual or Exceptional Conditions.
(set-plus-e-error-masking-bash)
🪛 PSScriptAnalyzer (1.25.0)
updates-main.ps1
[warning] 761-761: Function 'New-NpmInstallArguments' has verb that could change system state. Therefore, the function has to support 'ShouldProcess'.
(PSUseShouldProcessForStateChangingFunctions)
[warning] 761-761: The cmdlet 'New-NpmInstallArguments' uses a plural noun. A singular noun should be used instead.
Suggested fix: Singularized correction of 'New-NpmInstallArguments'
(PSUseSingularNouns)
[warning] 772-772: The cmdlet 'Get-NpmInstallExtraFlags' uses a plural noun. A singular noun should be used instead.
Suggested fix: Singularized correction of 'Get-NpmInstallExtraFlags'
(PSUseSingularNouns)
[warning] 780-780: The cmdlet 'Get-NpmInstallRetryOptions' uses a plural noun. A singular noun should be used instead.
Suggested fix: Singularized correction of 'Get-NpmInstallRetryOptions'
(PSUseSingularNouns)
🔇 Additional comments (13)
CHANGELOG.md (1)
10-13: LGTM!PLAN.md (1)
1-15: LGTM!README.md (1)
122-128: LGTM!SPEC.md (2)
266-266: LGTM!
466-468: 🎯 Functional CorrectnessAllow-scripts retry already preserves
NODE_NPM_INSTALL_FLAGSin both Bash and PowerShell.tests/test_cli.sh (1)
16-16: LGTM!Also applies to: 27-28, 945-948, 962-996, 1040-1128
tests/test_windows_native.ps1 (1)
17-18: LGTM!Also applies to: 436-466, 481-483, 505-559, 560-640, 931-989, 1002-1053, 1100-1152, 1195-1195, 1280-1298, 1338-1386, 1421-1469
updates (3)
680-693: LGTM!
725-739: LGTM!
749-806: LGTM!updates-main.ps1 (3)
761-787: LGTM!
840-869: LGTM!
950-951: LGTM!
|
@greptile review |
|
✅ Action performedReview finished.
|
Codex reviewFound 0 blocking issues in the current diff. Checked the npm flag plumbing across Bash and PowerShell, ERESOLVE retry ordering/deduplication, allow-scripts retry preservation, Bash 3.2 nounset handling, docs/spec/changelog coverage, and the new Bash/Windows regression tests. Validation already green locally and in GitHub checks. |
|
@sourcery-ai review |
Summary
~/.updatesrcconfig keyNODE_NPM_INSTALL_FLAGSthat passes extra flags tonpm install -gin thenodemodule, inserted before the--separator.@tarquinen/opencode-dcpvs@opencode-ai/plugin@opentui/coreversion mismatch) without requiring a global~/.npmrcoverride.updates) and native Windows PowerShell (updates-main.ps1) runtimes.Changes
updates(Bash): newNODE_NPM_INSTALL_FLAGSglobal, config parser case, word-split intorun_npm_global_install()args (including ERESOLVE retry path), and dry-run output.updates-main.ps1(PowerShell): new$script:NodeNpmInstallFlagsvariable, config parser case, whitespace-split intoInvoke-NpmGlobalInstallargs (including ERESOLVE retry path), and dry-run output inInvoke-ModuleNode.SPEC.md: added to the config key table.README.md: added to the config example and documented the key.CHANGELOG.md: added under[Unreleased].Test plan
NODE_NPM_INSTALL_FLAGS=--legacy-peer-depsin config,--dry-run --only nodeoutput includes the flagInvoke-Bootstrapwith stub environmentpwsh)python3not-found failure unrelated to this PRSummary by Sourcery
Add a configurable NODE_NPM_INSTALL_FLAGS option to scope extra npm install flags for the node module while keeping default behavior unchanged, with cross-platform support and tests.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
NODE_NPM_INSTALL_FLAGSto~/.updatesrcto forward extra, whitespace-separated flags (e.g.,--legacy-peer-deps) tonpm install -gfor the node module.Documentation
CHANGELOG,README, andSPECwith usage, defaults, and how flags appear in dry-run and retry scenarios.Bug Fixes
--legacy-peer-deps, and avoid duplicating it.Tests