fix: pin npm for lockfile generation to match publish audit#1659
fix: pin npm for lockfile generation to match publish audit#1659ChiragAgg5k wants to merge 1 commit into
Conversation
Greptile SummaryThis PR fixes a recurring publish failure for sdk-for-cli caused by lockfile drift: the local
Confidence Score: 4/5The change is safe to merge; it correctly roots out the two sources of lockfile drift without affecting any runtime behaviour of the generated SDKs. The script change is straightforward and the approach (pin via npx, drop the overrides injection) directly matches the audit workflow's behaviour. The one minor rough edge is that stderr from the npx/npm subprocess is still swallowed with 2>/dev/null, making download or registry failures harder to diagnose, but this is a pre-existing style issue and does not affect correctness. No files require special attention; the lockfile template regeneration is mechanical and the script logic is easy to follow. Important Files Changed
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
scripts/update-lockfiles.sh:118
**Stderr silently suppressed for npx download failures**
`2>/dev/null` already existed for the ambient-npm invocation, but with `npx -y` there is now an additional failure mode: if the network is unavailable or `npm@11.10.0` cannot be fetched from the registry, `npx` exits non-zero but the error message is discarded. The `set -e` in the script will still abort the run (the missing `package-lock.json` will cause the subsequent `cp` to fail with a generic "No such file" message), but the actual root cause — a network or registry error — is lost. Dropping `2>/dev/null`, or piping stderr to a log file, would surface that signal without breaking the existing quiet-on-success behaviour.
Reviews (1): Last reviewed commit: "(fix): pin npm for lockfile generation t..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| mkdir -p "$dir" | ||
| strip_twig "$template" > "$dir/package.json" | ||
| (cd "$dir" && npm_config_legacy_peer_deps=false npm install --package-lock-only --ignore-scripts --silent 2>/dev/null) | ||
| (cd "$dir" && npm_config_legacy_peer_deps=false npx -y "npm@${SDK_GEN_NPM_VERSION}" install --package-lock-only --ignore-scripts --silent 2>/dev/null) |
There was a problem hiding this comment.
2>/dev/null already existed for the ambient-npm invocation, but with npx -y there is now an additional failure mode: if the network is unavailable or npm@11.10.0 cannot be fetched from the registry, npx exits non-zero but the error message is discarded. The set -e in the script will still abort the run (the missing package-lock.json will cause the subsequent cp to fail with a generic "No such file" message), but the actual root cause — a network or registry error — is lost. Dropping 2>/dev/null, or piping stderr to a log file, would surface that signal without breaking the existing quiet-on-success behaviour.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/update-lockfiles.sh
Line: 118
Comment:
**Stderr silently suppressed for npx download failures**
`2>/dev/null` already existed for the ambient-npm invocation, but with `npx -y` there is now an additional failure mode: if the network is unavailable or `npm@11.10.0` cannot be fetched from the registry, `npx` exits non-zero but the error message is discarded. The `set -e` in the script will still abort the run (the missing `package-lock.json` will cause the subsequent `cp` to fail with a generic "No such file" message), but the actual root cause — a network or registry error — is lost. Dropping `2>/dev/null`, or piping stderr to a log file, would surface that signal without breaking the existing quiet-on-success behaviour.
How can I resolve this? If you propose a fix, please make it concise.
|
Closing — the twig lockfile templates are the single source of truth for the generated lock files, so the publish audit shouldn't regenerate the lock in CI at all. The drift check is being removed from sdk-for-cli's publish workflow instead; no npm pinning needed here. |
1 participant
The sdk-for-cli 22.6.1 publish failed at its
Audit npm dependenciesstep, which regeneratespackage-lock.jsonwith the workflow-pinned npm 11.10.0 and fails on any diff. The committed lock came fromupdate-lockfiles.sh, which used whatever npm is installed locally — npm 11.16.0 in this case, which emitslibcfields on optional native-binary entries that 11.10.0 doesn't. Thesync_npm_overridesstep made it worse: it injected anoverridesblock intopackages[""]that npm 11.10.0's own--package-lock-onlyoutput never contains, guaranteeing drift.Changes:
update-lockfiles.shnow runs lockfile generation throughnpx -y npm@${SDK_GEN_NPM_VERSION}(default 11.10.0, matching the sdk-for-cli publish workflow pin), instead of the ambient npm.sync_npm_overrides— npm resolvesoverridesfrompackage.jsonat install time and its canonical--package-lock-onlyoutput does not include the block inpackages[""]; injecting it breaks the audit'sgit diff --exit-code.templates/cli/package-lock.json.twigwith the pinned version. Verified the de-twigged output is a fixed point of the audit's rewrite: runningnpm@11.10.0 install --package-lock-onlyover it produces zero diff, and it byte-matches the normalized lock now on sdk-for-cli master (appwrite/sdk-for-cli#339).bun.lock.twigwas regenerated and is unchanged.npm ci && npm run buildverified working against the normalized lock. If the publish workflow's npm pin is ever bumped, regenerate withSDK_GEN_NPM_VERSION=<version> ./scripts/update-lockfiles.sh.