Repository navigation
fix: harden npm global updates - #5
Conversation
Reviewer's GuideHarden npm-based global Node updates by sourcing NVM before resolving npm tooling, adding single-retry handling for npm ERESOLVE and npm 11 allow-scripts prompts, and preparing the v2.0.2 release metadata, docs, and tests across Bash and PowerShell harnesses. Sequence diagram for hardened npm global Node updatessequenceDiagram
actor User
participant UpdatesCli as updates
participant NvmEnv as NVM
participant NpmCheckUpdates as ncu_or_npx
participant NpmCli as npm
User->>UpdatesCli: run updates node
UpdatesCli->>NvmEnv: source nvm.sh
NvmEnv-->>UpdatesCli: NVM-managed npm/ncu available
UpdatesCli->>NpmCheckUpdates: ncu / npx npm-check-updates
NpmCheckUpdates-->>UpdatesCli: upgrade list
UpdatesCli->>NpmCli: npm install -g <packages>
alt [npm ERESOLVE]
UpdatesCli->>NpmCli: npm install -g <packages> --legacy-peer-deps
NpmCli-->>UpdatesCli: success (first error details suppressed)
else [no ERESOLVE]
NpmCli-->>UpdatesCli: success
end
opt [npm 11 skipped scripts]
UpdatesCli->>NpmCli: npm install -g <packages> --allow-scripts=<list>
NpmCli-->>UpdatesCli: success (one-shot allow-scripts)
end
UpdatesCli-->>User: node updates complete
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
More reviews will be available in 36 minutes and 30 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughVersion bumped to 2.0.2. The ChangesNVM Init and npm allow-scripts Retry
Sequence Diagram(s)sequenceDiagram
participant module_node
participant init_nvm_node_tools
participant run_npm_global_install
participant run_npm_global_install_logged
participant handle_npm_global_install_success
participant npm_allow_scripts_arg_from_log
module_node->>init_nvm_node_tools: source nvm.sh, nvm use
module_node->>run_npm_global_install: install package
run_npm_global_install->>init_nvm_node_tools: ensure NVM ready (idempotent)
run_npm_global_install->>run_npm_global_install_logged: npm install -g <pkg> → error log
alt exit 0
run_npm_global_install->>handle_npm_global_install_success: check log
handle_npm_global_install_success->>npm_allow_scripts_arg_from_log: extract --allow-scripts=...
npm_allow_scripts_arg_from_log-->>handle_npm_global_install_success: arg or empty
opt arg present
handle_npm_global_install_success->>run_npm_global_install_logged: retry with --allow-scripts=...
end
else ERESOLVE
run_npm_global_install->>run_npm_global_install_logged: retry --legacy-peer-deps
run_npm_global_install_logged-->>handle_npm_global_install_success: result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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)
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}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 243d7f150a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
243d7f1 to
152ff3b
Compare
|
@greptile review |
There was a problem hiding this comment.
Pull request overview
This PR hardens the node module’s global npm update flow in the updates Bash CLI (macOS/Linux) by preferring NVM-managed tooling in non-interactive runs and adding targeted retry behavior for common npm failure/warning cases. It also bumps release metadata to v2.0.2 and refreshes docs/tests accordingly.
Changes:
- Source NVM (when present) before resolving/running
npm/ncu/npxfor thenodemodule. - Add retry logic for npm
ERESOLVE(--legacy-peer-deps) and npm 11 “allow-scripts” one-shot guidance, suppressing first-pass details when a retry succeeds. - Bump versions to
2.0.2across the Bash/PowerShell entrypoints, docs, changelog, and tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| updates-main.ps1 | Bumps embedded Windows payload version to 2.0.2. |
| updates | Adds NVM sourcing + npm retry logic for global installs in the node module. |
| tests/test_windows_native.ps1 | Updates Windows native self-update tests for the 2.0.2 release. |
| tests/test_cli.sh | Adds/updates Unix CLI tests covering npm retry behavior and NVM sourcing. |
| tests/helpers/windows_test_lib.ps1 | Updates default Windows runtime version used by tests to 2.0.2. |
| SPEC.md | Documents NVM sourcing and npm retry behavior for the node module. |
| README.md | Updates Windows install snippet/version and node module description. |
| CHANGELOG.md | Adds 2.0.2 release notes describing the hardened npm flow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_windows_native.ps1 (1)
991-991:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winIncomplete variable migration causes test assertion mismatch.
This test installs
$previousReleaseVersion(2.0.1) on line 939, but asserts thatcurrent.txtstill contains the hardcoded'2.0.0'. The assertion should use$previousReleaseVersionto match the installed version.The same issue occurs at lines 1138, 1226, and 1309 in other self-update failure tests.
🔧 Proposed fix for all four test assertions
Line 991:
- Assert-Equal -Expected '2.0.0' -Actual ((Get-Content -LiteralPath (Join-Path $installRoot 'current.txt') -Raw).Trim()) -Message 'unsupported cached manifest digest should leave current.txt unchanged' + Assert-Equal -Expected $previousReleaseVersion -Actual ((Get-Content -LiteralPath (Join-Path $installRoot 'current.txt') -Raw).Trim()) -Message 'unsupported cached manifest digest should leave current.txt unchanged'Line 1138:
- Assert-Equal -Expected '2.0.0' -Actual ((Get-Content -LiteralPath (Join-Path $installRoot 'current.txt') -Raw).Trim()) -Message 'download failure should leave current.txt unchanged' + Assert-Equal -Expected $previousReleaseVersion -Actual ((Get-Content -LiteralPath (Join-Path $installRoot 'current.txt') -Raw).Trim()) -Message 'download failure should leave current.txt unchanged'Line 1226:
- Assert-Equal -Expected '2.0.0' -Actual ((Get-Content -LiteralPath (Join-Path $installRoot 'current.txt') -Raw).Trim()) -Message 'digest mismatch should leave current.txt unchanged' + Assert-Equal -Expected $previousReleaseVersion -Actual ((Get-Content -LiteralPath (Join-Path $installRoot 'current.txt') -Raw).Trim()) -Message 'digest mismatch should leave current.txt unchanged'Line 1309:
- Assert-Equal -Expected '2.0.0' -Actual ((Get-Content -LiteralPath (Join-Path $installRoot 'current.txt') -Raw).Trim()) -Message 'invalid extracted manifest should leave current.txt unchanged' + Assert-Equal -Expected $previousReleaseVersion -Actual ((Get-Content -LiteralPath (Join-Path $installRoot 'current.txt') -Raw).Trim()) -Message 'invalid extracted manifest should leave current.txt unchanged'🤖 Prompt for 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. In `@tests/test_windows_native.ps1` at line 991, In the test file, four Assert-Equal assertions check that current.txt remains unchanged after installation failures, but they use hardcoded '2.0.0' as the expected value instead of the actual installed version. Update the -Expected parameter in all four Assert-Equal calls (at the mentioned line locations) to use the $previousReleaseVersion variable instead of the hardcoded '2.0.0' string value to ensure the assertions match the version that was actually installed earlier in the test.
🤖 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 738-741: Update the sed pattern in the
npm_allow_scripts_arg_from_log() function to make backticks optional rather than
required, since npm 11+ does not consistently wrap the --allow-scripts
suggestion in backticks. Modify the pattern to match the --allow-scripts flag
whether it appears with backticks (e.g., `npm install -g --allow-scripts=...`)
or without (e.g., npm install -g --allow-scripts=...), and test the updated
pattern against actual npm 11.x stderr output to verify it reliably captures the
flag value in both formats and variations.
---
Outside diff comments:
In `@tests/test_windows_native.ps1`:
- Line 991: In the test file, four Assert-Equal assertions check that
current.txt remains unchanged after installation failures, but they use
hardcoded '2.0.0' as the expected value instead of the actual installed version.
Update the -Expected parameter in all four Assert-Equal calls (at the mentioned
line locations) to use the $previousReleaseVersion variable instead of the
hardcoded '2.0.0' string value to ensure the assertions match the version that
was actually installed earlier in the test.
🪄 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: 62fc95f7-beb4-4952-afc8-cf2f99b6c54b
📒 Files selected for processing (8)
CHANGELOG.mdREADME.mdSPEC.mdtests/helpers/windows_test_lib.ps1tests/test_cli.shtests/test_windows_native.ps1updatesupdates-main.ps1
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (5)
**/CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Add a
CHANGELOG.mdentry for each release
Files:
CHANGELOG.md
**/SPEC.md
📄 CodeRabbit inference engine (AGENTS.md)
Maintain
SPEC.mdas living contract for flags/output/modules
Files:
SPEC.md
**/*.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
**/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
🔇 Additional comments (8)
updates-main.ps1 (1)
22-22: LGTM!tests/helpers/windows_test_lib.ps1 (1)
612-612: LGTM!tests/test_windows_native.ps1 (1)
17-18: LGTM!SPEC.md (1)
462-466: LGTM!CHANGELOG.md (1)
10-16: LGTM!README.md (1)
32-35: LGTM!Also applies to: 91-92, 132-133, 157-160
updates (1)
9-9: LGTM!Also applies to: 121-121, 676-737, 743-907, 2717-2767
tests/test_cli.sh (1)
26-27: LGTM!Also applies to: 944-971, 997-1043
152ff3b to
9d817df
Compare
|
| Filename | Overview |
|---|---|
| updates | Core Bash script: adds NVM init, allow-scripts retry, and ERESOLVE retry hardening. When the ERESOLVE retry itself fails, the original error log is deleted before the retry runs, so only the retry's output is shown — original ERESOLVE details are lost. |
| updates-main.ps1 | PowerShell payload: mirrors Bash allow-scripts and ERESOLVE retry logic. Same diagnostic regression — $installResult (original ERESOLVE output) is never printed when the --legacy-peer-deps retry also fails. |
| tests/test_cli.sh | Adds Unix tests for NVM sourcing, allow-scripts retry (with and without parseable flag), and ERESOLVE suppression. Tests are well-structured and cover main success paths. |
| tests/test_windows_native.ps1 | Adds Windows tests for allow-scripts retry, unparseable warning surfacing, and parametrises existing self-update tests to use $currentReleaseVersion/$previousReleaseVersion variables. |
| tests/helpers/windows_test_lib.ps1 | Adds Assert-NotMatch helper and bumps default test version to 2.0.2. Straightforward. |
| install-windows.ps1 | Version default bumped from 2.0.1 to 2.0.2. Trivial change. |
| CHANGELOG.md | Adds v2.0.2 entry documenting NVM sourcing, allow-scripts retry, and ERESOLVE suppression fixes. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[run_npm_global_install pkg...] --> B[init_nvm_node_tools]
B --> C{mktemp ok?}
C -- no --> D[run npm install -g -- pkg]
C -- yes --> E[run_npm_global_install_logged err_log -- pkg]
E --> F{exit 0?}
F -- yes --> G[handle_npm_global_install_success err_log -- pkg]
G --> H{allow-scripts arg in log?}
H -- yes --> I[warn: retrying with allow-scripts]
I --> J[run_npm_global_install_logged allow_log allow-scripts-arg -- pkg]
J --> K[show allow_log, return status]
H -- no --> L{allow-scripts warning unparseable?}
L -- yes --> M[warn: could not parse list, show err_log, return 0]
L -- no --> N[show err_log, return 0]
F -- no --> O{ERESOLVE in log?}
O -- no --> P[show err_log, return status]
O -- yes --> Q[warn: retrying --legacy-peer-deps, rm err_log]
Q --> R[run_npm_global_install_logged new_log --legacy-peer-deps -- pkg]
R --> S{exit 0?}
S -- yes --> T[handle_npm_global_install_success new_log --legacy-peer-deps -- pkg]
S -- no --> U[show new_log only, original ERESOLVE lost, 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 pkg...] --> B[init_nvm_node_tools]
B --> C{mktemp ok?}
C -- no --> D[run npm install -g -- pkg]
C -- yes --> E[run_npm_global_install_logged err_log -- pkg]
E --> F{exit 0?}
F -- yes --> G[handle_npm_global_install_success err_log -- pkg]
G --> H{allow-scripts arg in log?}
H -- yes --> I[warn: retrying with allow-scripts]
I --> J[run_npm_global_install_logged allow_log allow-scripts-arg -- pkg]
J --> K[show allow_log, return status]
H -- no --> L{allow-scripts warning unparseable?}
L -- yes --> M[warn: could not parse list, show err_log, return 0]
L -- no --> N[show err_log, return 0]
F -- no --> O{ERESOLVE in log?}
O -- no --> P[show err_log, return status]
O -- yes --> Q[warn: retrying --legacy-peer-deps, rm err_log]
Q --> R[run_npm_global_install_logged new_log --legacy-peer-deps -- pkg]
R --> S{exit 0?}
S -- yes --> T[handle_npm_global_install_success new_log --legacy-peer-deps -- pkg]
S -- no --> U[show new_log only, original ERESOLVE lost, return status]
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
updates:699-716
**Original ERESOLVE details silently discarded when retry also fails**
The original `npm_err_log` is deleted (line 701) before the retry runs, so its ERESOLVE output is permanently gone. If the `--legacy-peer-deps` retry then also fails (non-zero `status`), only the retry's error is shown — the original diagnostic is lost. Before this PR, the original error was always printed before the retry ran, so a user would see the full failure chain.
The PR description says "suppress the expected first-pass error details when retry *succeeds*," but the implementation suppresses them even when the retry fails. Concretely: a user who upgrades a package, hits an ERESOLVE, and also hits a different network/auth error on the retry will see only the secondary error, with no indication of what the root-cause ERESOLVE was.
### Issue 2 of 2
updates-main.ps1:835-843
**PowerShell mirrors the same diagnostic loss on retry failure**
`$installResult` (containing the original ERESOLVE output) is never written when the `--legacy-peer-deps` retry fails. `Write-ProcessResultOutput -Result $retryResult` at line 842 shows only the retry's output; `$installResult` is silently dropped. This is the same regression as in the Bash path: users who need to diagnose a doubly-failed install see only the retry's error, not the original ERESOLVE details that triggered the retry.
Reviews (6): Last reviewed commit: "fix: harden npm global updates" | Re-trigger Greptile
|
@greptile review |
9d817df to
3c5197b
Compare
|
@greptile review |
|
✅ Action performedReview finished.
|
|
| Filename | Overview |
|---|---|
| updates | Core bash script: adds init_nvm_node_tools, refactors run_npm_global_install into logged/success helpers with ERESOLVE and allow-scripts retry chains. Logic is sound; one fragility in the sed-based allow-scripts extraction. |
| tests/test_cli.sh | Adds NVM sourcing test, allow-scripts retry test, and extended ERESOLVE+allow-scripts combined test; version constants bumped to 2.0.2/2.0.3. NVM_DIR is correctly unset at test start to prevent CI interference. |
| tests/test_windows_native.ps1 | All hardcoded version literals replaced with $currentReleaseVersion/$previousReleaseVersion variables; self-update fixture and assertion patterns updated to 2.0.2. Clean mechanical bump with no logic changes. |
| tests/helpers/windows_test_lib.ps1 | Single default parameter version bump from 2.0.1 to 2.0.2 in Install-RepoWindowsRuntime. |
| updates-main.ps1 | Version string bumped from 2.0.1 to 2.0.2; no other changes. |
| CHANGELOG.md | New 2.0.2 section added with three fixed entries matching the PR changes. |
| README.md | Version references updated to 2.0.2; node module description updated to document NVM sourcing and npm 11 allow-scripts retry behavior. |
| SPEC.md | Node module spec updated with NVM sourcing, ERESOLVE retry, and allow-scripts retry behavior. Accurately reflects the implementation. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[run_npm_global_install] --> B[init_nvm_node_tools]
B --> C{DRY_RUN?}
C -- yes --> D[run npm install -g]
C -- no --> E[mktemp npm_err_log]
E --> F[run_npm_global_install_logged\nfirst attempt]
F --> G{exit 0?}
G -- yes --> H[handle_npm_global_install_success\nnpm_err_log]
H --> I{allow-scripts\nwarning in log?}
I -- no --> J[cat log if non-empty, return 0]
I -- yes --> K[rm first-pass log\nmktemp allow_err_log]
K --> L[run_npm_global_install_logged\nwith allow-scripts arg]
L --> M[cat allow_err_log, return status]
G -- no --> N{ERESOLVE\nin log?}
N -- no --> O[cat log, return status]
N -- yes --> P[rm ERESOLVE log\nmktemp new log\nwarn: retrying with legacy-peer-deps]
P --> Q[run_npm_global_install_logged\n--legacy-peer-deps retry]
Q --> R{exit 0?}
R -- yes --> S[handle_npm_global_install_success\n--legacy-peer-deps path]
S --> T{allow-scripts\nwarning in log?}
T -- yes --> U[run_npm_global_install_logged\n--allow-scripts + --legacy-peer-deps]
T -- no --> V[cat log, return 0]
R -- no --> W[cat log, 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] --> B[init_nvm_node_tools]
B --> C{DRY_RUN?}
C -- yes --> D[run npm install -g]
C -- no --> E[mktemp npm_err_log]
E --> F[run_npm_global_install_logged\nfirst attempt]
F --> G{exit 0?}
G -- yes --> H[handle_npm_global_install_success\nnpm_err_log]
H --> I{allow-scripts\nwarning in log?}
I -- no --> J[cat log if non-empty, return 0]
I -- yes --> K[rm first-pass log\nmktemp allow_err_log]
K --> L[run_npm_global_install_logged\nwith allow-scripts arg]
L --> M[cat allow_err_log, return status]
G -- no --> N{ERESOLVE\nin log?}
N -- no --> O[cat log, return status]
N -- yes --> P[rm ERESOLVE log\nmktemp new log\nwarn: retrying with legacy-peer-deps]
P --> Q[run_npm_global_install_logged\n--legacy-peer-deps retry]
Q --> R{exit 0?}
R -- yes --> S[handle_npm_global_install_success\n--legacy-peer-deps path]
S --> T{allow-scripts\nwarning in log?}
T -- yes --> U[run_npm_global_install_logged\n--allow-scripts + --legacy-peer-deps]
T -- no --> V[cat log, return 0]
R -- no --> W[cat log, return status]
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
updates:738-741
**`sed` pattern coupled to npm's exact warning message format**
`npm_allow_scripts_arg_from_log` extracts the `--allow-scripts=...` argument by matching the literal string `` `npm install -g `` inside the warning line. If npm changes the warning text (e.g., drops the backtick-quoted command, renames the flag, or adjusts whitespace), the pattern returns an empty string and the retry silently never fires — the raw `npm warn allow-scripts` warning is then printed to the user without any retry attempt. There is no fallback or log message indicating that the retry was skipped.
Reviews (3): Last reviewed commit: "fix: harden npm global updates" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c5197b2de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
3c5197b to
a92d20a
Compare
|
@greptile review |
|
✅ Action performedReview finished.
|
a92d20a to
6156b7c
Compare
|
@greptile review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6156b7c2a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| npm_allow_scripts_arg_from_log() { | ||
| local npm_err_log="$1" | ||
| sed -n 's/.*\(--allow-scripts=[^`[:space:]]*\).*/\1/p' "$npm_err_log" | head -n 1 |
There was a problem hiding this comment.
Because this helper scans the entire npm error log before verifying that the text came from an npm warn allow-scripts advisory, a successful install that prints an unrelated warning/deprecation message containing --allow-scripts=foo is retried as npm install -g --allow-scripts=foo .... npm documents this flag as the global-install mechanism for allowing install scripts, so this can approve/run scripts based on arbitrary package-controlled output rather than npm's approval warning; scope extraction to the allow-scripts warning line and mirror the fix in updates-main.ps1.
Useful? React with 👍 / 👎.
Summary
ERESOLVEonce with--legacy-peer-depswhile suppressing the expected first-pass error details when retry succeeds--allow-scripts=...argument, without changing persistent npm configv2.0.2release metadata and testsValidation
bash -n updates tests/test_cli.sh scripts/*.sh./scripts/test.shgit diff --checkLocal validation gaps
./scripts/lint.shblocked locally:shellcheckis not installed;shfmtis also unavailable.bash ./scripts/release-build.sh 2.0.2blocked locally:zipis not installed.pwsh -NoLogo -NoProfile -File tests/test_windows_native.ps1blocked locally:pwshis not installed.CI installs these missing tools on hosted runners.
Summary by Sourcery
Prepare the 2.0.2 release with hardened global npm update behavior and refreshed tests and docs.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores:
Summary by CodeRabbit
Release Notes: v2.0.2
Bug Fixes
Documentation
Chores