ENG-2954: Upgrade shield to Node v24 (+ pnpm 10.33.1 pin, release gh-CLI swap, SEA stdout-flush fix)#26
Open
ajag408 wants to merge 4 commits into
Open
ENG-2954: Upgrade shield to Node v24 (+ pnpm 10.33.1 pin, release gh-CLI swap, SEA stdout-flush fix)#26ajag408 wants to merge 4 commits into
ajag408 wants to merge 4 commits into
Conversation
ajag408
requested review from
Philippoes,
jdomingos,
petar-omni and
raiseerco
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughFixes CLI stdout truncation for large piped outputs by deferring ChangesCLI stdout flush fix and regression test
Node/pnpm toolchain upgrade to v1.3.0
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.github/workflows/release.yml:
- Around line 291-294: Make the release creation step idempotent by first
checking if the release already exists using gh release view command before
attempting to create it. If the release exists, use gh release upload with the
--clobber flag to re-upload the shield-* assets instead of creating a new
release. If the release does not exist, proceed with the current gh release
create command with the GITHUB_REF_NAME, title, and generate-notes options.
- Around line 43-47: The actions/setup-node steps in the release workflow do not
explicitly disable package-manager caching, creating a cache-poisoning attack
surface in a privileged workflow with security audits and production environment
access. Add the `package-manager-cache: false` input parameter to all four
setup-node action invocations in the release workflow to prevent automatic
caching and harden the security posture.
In `@src/cli.integration.test.ts`:
- Line 36: The file src/cli.integration.test.ts is missing a trailing newline at
the end, which is causing Prettier violations and CI failures. Add a newline
character at the very end of the file after the closing brace and semicolon to
satisfy Prettier formatting requirements.
🪄 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: CHILL
Plan: Pro
Run ID: af9fc4d3-7214-4816-9e23-88a43283acd9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
.github/workflows/ci.yml.github/workflows/release.yml.nvmrcpackage.jsonsrc/cli.integration.test.tssrc/cli.ts
ajag408
requested review from
aditya172926,
auralshin,
dnehl and
samuelmattjohnston
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
2 participants
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.
Summary by CodeRabbit
Bug Fixes
Chores
Tests
ENG-2954 — move
shieldto Node v24 (the monorepo already cut over; GitHub's forced-deprecation date has passed). The CI hardening PR has already merged to main, so this lands cleanly on top of it as the last change before the first hardened + Node-24 release tag.Three things are bundled (all touch the release/runtime path, all surfaced while validating this upgrade):
v1.2.6tag hit a Startup failure becausesoftprops/action-gh-releaseisn't on the org Actions allowlist. Swapped it for the GitHub-ownedghCLI (no allowlist dependency, same philosophy as the corepack/Socket choices).subprocessintegration path).getSupportedYieldIdswas returning truncated, unparseable JSON to every consumer. Pre-existing on the Node-22 binaries tooVersion bumped
1.2.5 → 1.3.0.Changes
Runtime / tooling
.nvmrc:20.17.0 → 24.16.0package.json:version 1.3.0;packageManager pnpm@10.12.2 → 10.33.1; addedengines(node >=20.19.0,pnpm >=10.33.1, mirroring the monorepo);@types/node ^20.0.0 → ^20.19.0pnpm-lock.yaml: regenerated on pnpm 10.33.1CI (
ci.yml)[20.17.0, 22.x] → [20.19.0, 22.x, 24.x](floor raised to the new engines baseline)24.xpnpm@10.33.1Release (
release.yml)node-version "22" → "24"across all three jobs (security-audit, publish, build-binaries incl. the Rosetta x64 path); corepackpnpm@10.33.1create-release: replacedsoftprops/action-gh-releasewithgh release create … --generate-notes(job already hascontents: write)Bug fix
src/cli.ts: exit inside theprocess.stdout.write(…, cb)callback so large responses fully flush before exit (waswrite()then immediateprocess.exit())src/cli.integration.test.ts: new regression test — spawns the bundle, capturesgetSupportedYieldIdsover a pipe, asserts the full >64 KB payload parsesQA Proof (local, Node 24.16.0, darwin-x64)
pnpm run lint→ 0 errors (55 pre-existing warnings, unchanged from main)pnpm run build→ greenpnpm run build:binary→shield-darwin-x64(121.9 MB), SEA inject + codesign OKgetSupportedYieldIdsover a pipe → 124758 bytes full output (was 65536 truncated pre-fix)isSupported→true(ethereum-eth-lido-staking),false(nope)diffclean)Truncation fix, pre vs post (piped
| wc -c):What needs to be verified in CI / at the release tag
24.xleg green — note: the Tron native-staking suite (native-staking.validator.test.ts) failed to run locally undertest:coverageon Node 24 macOS (jest-worker "circular structure to JSON"; all 5596 assertions otherwise pass). Verifying whether it reproduces on CI (ubuntu). If it does, it needs a follow-up fix before merge.v1.3.0(or an RC) tag — confirm thegh release createstep succeeds (this is what startup-failed onv1.2.6)build-binarieslegs green on Node 24:linux-x64,darwin-arm64,darwin-x64(Rosetta),win32-x64npm publishjob builds/tests clean on Node 24Risk
Low–moderate. Workflow/config + one targeted CLI fix. No validator logic changed.
enginesis advisory (.npmrchas noengine-strict). Main risk is the SEA build on Node 24 across the 3 non-local targets — covered by the release matrix.Notes for reviewers
esbuild --target=node20left as-is intentionally — it's a transpile floor, not a runtime cap; the bundle runs fine on the embedded Node 24, and it's SEA-only.gh-CLI release step needs no allowlist entry (it's arun:step, not an action). Whenrelease.ymllater flipsharden-runnertoegress-policy: block(Phase 2),api.github.com/uploads.github.comwill need allowlisting.