Prevent 'connected' event listener accumulation on reconnect by using .once() instead of .on()#3211
Prevent 'connected' event listener accumulation on reconnect by using .once() instead of .on()#3211
Conversation
… .once() instead of .on()
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis pull request fixes a bug where event listeners accumulate on reconnection, causing 'connected' event handlers to fire multiple times. The fix introduces cleanup of stale listeners from previous reconnect attempts and switches from persistent listener registration to one-time listener emission per reconnect. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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 the current code and only fix it if needed.
Inline comments:
In `@packages/xrpl/src/client/index.ts`:
- Around line 265-267: The reconnect handler currently registers a new
once('connected') listener each time this.connection emits 'reconnect', which
can accumulate multiple pending listeners if reconnect() is called repeatedly
before a single 'connected' fires; change the logic in the reconnect path to
remove any previously-registered pending handler before adding a new one: keep a
named handler reference (e.g., this._reconnectConnectedHandler) and on
'reconnect' call this.connection.removeListener('connected',
this._reconnectConnectedHandler) (or off/removeListener depending on API), then
set this._reconnectConnectedHandler = () => this.emit('connected') and register
it with this.connection.once('connected', this._reconnectConnectedHandler); this
ensures stale listeners from repeated reconnect() attempts (see
Connection.reconnect(), connect(), disconnect()) are cleaned up.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
.github/workflows/nodejs.yml (3)
275-279:vuln-report.txtis not uploaded as a workflow artifact.The report is only
cat-ted to the job log. It is not downloadable after the job finishes. For a security-scanning job, being able to retrieve the report from the Artifacts panel (e.g., for audits or async review) is useful.✨ Proposed: upload the report as an artifact
- name: Print vulnerability report if: always() run: | echo "=== Vulnerability Scan Report ===" cat vuln-report.txt + + - name: Upload vulnerability report + if: always() + uses: actions/upload-artifact@v4 + with: + name: vulnerability-report + path: vuln-report.txt + retention-days: 30🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/nodejs.yml around lines 275 - 279, The workflow currently only prints vuln-report.txt in the "Print vulnerability report" step, so the report isn't downloadable; add a new job step after that which uses the actions/upload-artifact action to upload vuln-report.txt (e.g., step named "Upload vulnerability report" using actions/upload-artifact@v4) with artifact name like "vulnerability-report" and path "vuln-report.txt" so the report is stored in the workflow Artifacts panel for later retrieval.
237-240:vulnerability-scanhas no correspondingworkflow_callinput to allow it to be skipped.The
workflow_calltrigger (lines 18-38) exposesrun_unit_tests,run_integration_tests, andrun_browser_testsinputs so callers can gate each job.vulnerability-scanruns unconditionally with no equivalent input, making it impossible to skip when calling this workflow from another workflow (e.g., release pipelines that handle scanning separately).✨ Proposed: add a `run_vulnerability_scan` input
run_browser_tests: description: "Run browser tests job" required: false type: boolean default: true + run_vulnerability_scan: + description: "Run vulnerability scan job" + required: false + type: boolean + default: truevulnerability-scan: + if: ${{ github.event_name != 'workflow_dispatch' || inputs.run_vulnerability_scan != false }} runs-on: ubuntu-latest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/nodejs.yml around lines 237 - 240, Add a new workflow_call input named run_vulnerability_scan (type: boolean, default true) alongside the existing inputs like run_unit_tests/run_integration_tests/run_browser_tests, and then guard the vulnerability-scan job by adding an if condition that checks that input (use the same expression style as other jobs, e.g. if: ${{ inputs.run_vulnerability_scan }}). This ensures the vulnerability-scan job only runs when the caller enables it.
256-257: Missing node_modules cache — every run triggers a fullnpm ci.All other jobs (
build-and-lint,unit,integration,browser) share the sameCache node modulesstep. Thevulnerability-scanjob omits it, causing an unconditional full install on every push and PR, which adds avoidable latency.♻️ Add node_modules cache (consistent with other jobs)
- name: Install Dependencies + id: cache-nodemodules + uses: actions/cache@v4 + env: + cache-name: cache-node-modules + with: + path: | + node_modules + */*/node_modules + key: ${{ runner.os }}-deps-24.x-${{ hashFiles('**/package-lock.json') }} + restore-keys: | + ${{ runner.os }}-deps-24.x- + + - name: Install Dependencies + if: steps.cache-nodemodules.outputs.cache-hit != 'true' run: npm ci🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/nodejs.yml around lines 256 - 257, The vulnerability-scan job is missing the shared "Cache node modules" step so its "Install Dependencies" (run: npm ci) always does a full install; add the same cache step used by build-and-lint/unit/integration/browser jobs before the "Install Dependencies" step in the vulnerability-scan job (use the actions/cache setup keyed on node-modules plus package-lock.json/yarn.lock with appropriate restore-keys) so npm ci restores node_modules from cache when available and only installs when necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/nodejs.yml:
- Around line 265-273: The Trivy step "Scan SBOM for vulnerabilities using
Trivy" currently uses exit-code: 0 which prevents CI from failing on
HIGH/CRITICAL findings; change exit-code to 1 to make the step fail the job on
vulnerabilities and, if you need a grace period, add continue-on-error: true on
that step or create a tracked issue for the existing TODO so the change isn't
forgotten; update the step block around the Trivy action (the "Scan SBOM for
vulnerabilities using Trivy" step) accordingly and ensure the comment/TODO is
replaced with an issue reference if you choose the tracked-issue route.
- Around line 259-260: The workflow installs an outdated CycloneDX CLI; update
the GitHub Actions step named "Install cyclonedx-npm" to install the newer
package version by replacing `@cyclonedx/cyclonedx-npm`@4.0.2 with
`@cyclonedx/cyclonedx-npm`@4.1.2 so the run command uses the current stable
release and benefits from xmlbuilder2 v4 compatibility and modern standards.
- Around line 265-266: Update the Trivy Action used in the "Scan SBOM for
vulnerabilities using Trivy" workflow step by changing the uses value from
aquasecurity/trivy-action@0.28.0 to aquasecurity/trivy-action@0.34.0 (or a later
patch/minor release) to pick up the security fixes and newer vulnerability DB;
in the same step ensure any indirect actions (e.g., actions/cache) are pinned by
SHA if referenced by this action to match the hardening introduced in later
trivy-action versions.
---
Nitpick comments:
In @.github/workflows/nodejs.yml:
- Around line 275-279: The workflow currently only prints vuln-report.txt in the
"Print vulnerability report" step, so the report isn't downloadable; add a new
job step after that which uses the actions/upload-artifact action to upload
vuln-report.txt (e.g., step named "Upload vulnerability report" using
actions/upload-artifact@v4) with artifact name like "vulnerability-report" and
path "vuln-report.txt" so the report is stored in the workflow Artifacts panel
for later retrieval.
- Around line 237-240: Add a new workflow_call input named
run_vulnerability_scan (type: boolean, default true) alongside the existing
inputs like run_unit_tests/run_integration_tests/run_browser_tests, and then
guard the vulnerability-scan job by adding an if condition that checks that
input (use the same expression style as other jobs, e.g. if: ${{
inputs.run_vulnerability_scan }}). This ensures the vulnerability-scan job only
runs when the caller enables it.
- Around line 256-257: The vulnerability-scan job is missing the shared "Cache
node modules" step so its "Install Dependencies" (run: npm ci) always does a
full install; add the same cache step used by
build-and-lint/unit/integration/browser jobs before the "Install Dependencies"
step in the vulnerability-scan job (use the actions/cache setup keyed on
node-modules plus package-lock.json/yarn.lock with appropriate restore-keys) so
npm ci restores node_modules from cache when available and only installs when
necessary.
.github/workflows/nodejs.yml
Outdated
| - name: Install cyclonedx-npm | ||
| run: npm install -g @cyclonedx/cyclonedx-npm@4.0.2 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@cyclonedx/cyclonedx-npm changelog version 4.1.2 4.0.2 differences
💡 Result:
@cyclonedx/cyclonedx-npm — v4.0.2 vs v4.1.2 (changelog differences)
v4.0.2 (maintenance release)
- Runtime dependency bump:
@cyclonedx/cyclonedx-librarysupport raised to^9.0.0(from^8.4.0). [2] - Mostly CI/dev dependency maintenance (e.g.,
actions/setup-node4 → 5, eslint group bumps). [2]
v4.1.2
- Runtime dependency change: broadened
xmlbuilder2support to^3.0.2 || ^4.0.3(previously only^3.0.2). [1] - Style/internal standards: “Applied latest code standards”. [1]
- Mostly dependency maintenance (knip/eslint/actions/checkout/xmlbuilder2 bumps). [1]
In short: 4.0.2’s notable change is updating the CycloneDX library to v9; 4.1.2’s notable change is adding compatibility with xmlbuilder2 v4 plus style/standards updates. [1][2]
Sources
[1] GitHub Releases – v4.1.2 notes (CycloneDX/cyclonedx-node-npm) (github.com)
[2] GitHub Releases – v4.0.2 notes (CycloneDX/cyclonedx-node-npm) (github.com)
Citations:
- 1: https://github.com/CycloneDX/cyclonedx-node-npm/releases
- 2: https://github.com/CycloneDX/cyclonedx-node-npm/releases
🌐 Web query:
npm @cyclonedx/cyclonedx-npm latest version 2025 2026
💡 Result:
As of February 21, 2026, the latest released version of npm @cyclonedx/cyclonedx-npm is 4.1.2, released December 5, 2025. [1][2]
@cyclonedx/cyclonedx-npm@4.1.2 is the current stable release; consider upgrading from 4.0.2.
Version 4.1.2 (released December 2025) adds compatibility with xmlbuilder2 v4 and applies latest code standards—worth updating to reduce dependency drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/nodejs.yml around lines 259 - 260, The workflow installs
an outdated CycloneDX CLI; update the GitHub Actions step named "Install
cyclonedx-npm" to install the newer package version by replacing
`@cyclonedx/cyclonedx-npm`@4.0.2 with `@cyclonedx/cyclonedx-npm`@4.1.2 so the run
command uses the current stable release and benefits from xmlbuilder2 v4
compatibility and modern standards.
.github/workflows/nodejs.yml
Outdated
| - name: Scan SBOM for vulnerabilities using Trivy | ||
| uses: aquasecurity/trivy-action@0.28.0 | ||
| with: | ||
| scan-type: sbom | ||
| scan-ref: sbom.json | ||
| format: table | ||
| exit-code: 0 # TODO: Change to 1 to fail CI on vulnerabilities | ||
| output: vuln-report.txt | ||
| severity: CRITICAL,HIGH |
There was a problem hiding this comment.
exit-code: 0 makes the vulnerability scan purely informational — it cannot fail CI.
With exit-code: 0, any CRITICAL or HIGH vulnerabilities found by Trivy are silently recorded but never block a merge. The TODO comment acknowledges this but provides no tracking mechanism, and this configuration is as likely to stay indefinitely as it is to be changed. Consider either:
- Setting
exit-code: 1now (withcontinue-on-error: trueon the step if a hard failure is still too noisy), or - Opening a follow-up issue to track the TODO so it doesn't get forgotten.
🛡️ Proposed change to make blocking while tolerating a grace period
- name: Scan SBOM for vulnerabilities using Trivy
uses: aquasecurity/trivy-action@0.28.0
+ continue-on-error: true # Remove once the backlog is cleared
with:
scan-type: sbom
scan-ref: sbom.json
format: table
- exit-code: 0 # TODO: Change to 1 to fail CI on vulnerabilities
+ exit-code: 1
output: vuln-report.txt
severity: CRITICAL,HIGH🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/nodejs.yml around lines 265 - 273, The Trivy step "Scan
SBOM for vulnerabilities using Trivy" currently uses exit-code: 0 which prevents
CI from failing on HIGH/CRITICAL findings; change exit-code to 1 to make the
step fail the job on vulnerabilities and, if you need a grace period, add
continue-on-error: true on that step or create a tracked issue for the existing
TODO so the change isn't forgotten; update the step block around the Trivy
action (the "Scan SBOM for vulnerabilities using Trivy" step) accordingly and
ensure the comment/TODO is replaced with an issue reference if you choose the
tracked-issue route.
.github/workflows/nodejs.yml
Outdated
| - name: Scan SBOM for vulnerabilities using Trivy | ||
| uses: aquasecurity/trivy-action@0.28.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
aquasecurity/trivy-action release notes changelog 0.28.0 to 0.34.0
💡 Result:
aquasecurity/trivy-action — release notes (0.28.0 → 0.34.0)
0.28.0 (Oct 15)
- chore(deps): bump
setup-trivytov0.2.1(#411) [1]
0.29.0 (Nov 20)
- feat: allow skipping setup (
#414) - fix: oras command not found in “Update Trivy Cache” action (
#413) - docs: README updates (
#420,#429) - feat: add token for
setup-trivy(#421) - fix: bump
setup-trivy+contribdirectory path info (#424) - chore(deps): bump Trivy to
v0.57.1(#434) [2]
0.30.0 (Mar 14)
- fix/docs: README + action.yaml input text improvements (
#444,#452,#439) - chore(deps): bump Trivy to
v0.60.0(#453) [3]
0.31.0 (Jun 3)
- docs: note
unix:/prefix requirement fordocker-hostinput (#455) - fix: prevent action inputs leaking between invocations (
#454/#422) - security/maintenance: pin
aquasecurity/setup-trivyby hash (#456) - CI/docs: example updates + bump-workflow fixes (
#465,#466) - chore(deps): bump Trivy to
v0.63.0(#467) [3]
0.32.0 (Jul 4)
- chore(deps): bump Trivy to
v0.64.1(#474) [3]
0.33.0 (Aug 27)
- docs: README dependency updates + SBOM fs scan correction (
#378,#458) - security/maintenance: pin
actions/cacheby SHA (#480) - CI: add
orasto correctly set up sync jobs (#482) - chore(deps): bump Trivy to
v0.65.0(#481) [4]
0.33.1 (Sep 3)
- bump
setup-trivyaction tov0.2.4(#486) [4]
0.34.0 (Feb 12)
- CI: bump-trivy workflow improvements (setup-bats, install trivy, test updates) (
#494,#495) - docs: README update (
#493) - chore(deps): bump Trivy to
v0.68.1and thenv0.69.1(#496,#506) - CI: use checks bundle v2 in sync workflow (
#505) [4]
Sources: GitHub Releases for aquasecurity/trivy-action. [1][2][3][4]
Upgrade aquasecurity/trivy-action from 0.28.0 to 0.34.0 or later.
Between versions 0.28.0 and 0.34.0, the bundled Trivy scanner was upgraded from pre-0.57.1 to v0.69.1, bringing substantially more current vulnerability definitions. More importantly, v0.31.0 (released between your pinned version and the latest) includes a critical security fix: "prevent action inputs leaking between invocations" (#454). Additionally, v0.33.0 added security hardening by pinning the actions/cache action by SHA. Remaining on 0.28.0 exposes the workflow to both stale scan results and a potential security vulnerability in the action itself.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/nodejs.yml around lines 265 - 266, Update the Trivy Action
used in the "Scan SBOM for vulnerabilities using Trivy" workflow step by
changing the uses value from aquasecurity/trivy-action@0.28.0 to
aquasecurity/trivy-action@0.34.0 (or a later patch/minor release) to pick up the
security fixes and newer vulnerability DB; in the same step ensure any indirect
actions (e.g., actions/cache) are pinned by SHA if referenced by this action to
match the hardening introduced in later trivy-action versions.
kuan121
force-pushed
the
fix-client-connected-event-listener-accumulation-issue
branch
from
3d77685 to
0a984a0
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/nodejs.yml (2)
244-245:fetch-depth: 0fetches the full git history unnecessarily.SBOM generation only needs the working tree. A shallow clone (
fetch-depth: 1, or simply omitting the key) is sufficient and faster.♻️ Drop unnecessary full-history fetch
- uses: actions/checkout@v4 with: ref: ${{ env.GIT_REF }} - fetch-depth: 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/nodejs.yml around lines 244 - 245, The workflow currently sets fetch-depth: 0 which fetches full git history; change the checkout step to use a shallow clone by replacing fetch-depth: 0 with fetch-depth: 1 (or remove the fetch-depth key entirely) so SBOM generation only gets the working tree and the action runs faster—look for the checkout action block containing the literal fetch-depth: 0 and update that setting.
256-257: Missing node_modules cache — unconditionalnpm cion every run.All other jobs use
actions/cache@v4keyed on**/package-lock.jsonand gatenpm cibehind a cache-hit check. This job skips that pattern entirely, paying the full install cost on every workflow run.♻️ Align with the caching pattern used by other jobs
+ - name: Cache node modules + id: cache-nodemodules + uses: actions/cache@v4 + env: + cache-name: cache-node-modules + with: + path: | + node_modules + */*/node_modules + key: ${{ runner.os }}-deps-24.x-${{ hashFiles('**/package-lock.json') }} + restore-keys: | + ${{ runner.os }}-deps-24.x- + - name: Install Dependencies - run: npm ci + if: steps.cache-nodemodules.outputs.cache-hit != 'true' + run: npm ci🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/nodejs.yml around lines 256 - 257, The job currently always runs the "Install Dependencies" step which does an unconditional npm ci; add a preceding actions/cache@v4 step (e.g., id: cache-node-modules) that caches node_modules keyed on **/package-lock.json, then change the "Install Dependencies" step (e.g., id: install-deps) to run npm ci only when the cache miss occurs by gating it with if: steps.cache-node-modules.outputs.cache-hit != 'true' (or equivalent output name), ensuring npm ci runs only on cache misses and aligns this job with the other jobs' caching pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/nodejs.yml:
- Around line 265-273: The workflow step "Scan SBOM for vulnerabilities using
Trivy" currently sets exit-code: 0 so failures never fail CI; update the step to
set exit-code: 1 to make the Trivy SBOM scan fail the build on detected
vulnerabilities and replace the current TODO with a reference to a tracking
issue or add a new issue/PR link (or issue number) documenting the rollout plan
and exceptions; ensure you update the step using the same "Scan SBOM for
vulnerabilities using Trivy" step and the exit-code key to implement this
change.
- Around line 265-266: The workflow uses an outdated Trivy GitHub Action tag
"aquasecurity/trivy-action@0.28.0" which is vulnerable and behind current
stable; update that reference to "aquasecurity/trivy-action@0.34.0" in the
workflow step that currently reads aquasecurity/trivy-action@0.28.0 (the Scan
SBOM for vulnerabilities using Trivy step), then run or trigger the workflow to
verify the action runs with the newer Trivy scanner and no input-leakage
warnings.
- Around line 259-260: Update the GitHub Actions step that installs the
CycloneDX npm tool (step name "Install cyclonedx-npm") to use the current
package version by changing the installed package string from
"@cyclonedx/cyclonedx-npm@4.0.2" to "@cyclonedx/cyclonedx-npm@4.1.2"; ensure the
run command still uses npm install -g and verify the workflow runs successfully
after the version bump.
---
Nitpick comments:
In @.github/workflows/nodejs.yml:
- Around line 244-245: The workflow currently sets fetch-depth: 0 which fetches
full git history; change the checkout step to use a shallow clone by replacing
fetch-depth: 0 with fetch-depth: 1 (or remove the fetch-depth key entirely) so
SBOM generation only gets the working tree and the action runs faster—look for
the checkout action block containing the literal fetch-depth: 0 and update that
setting.
- Around line 256-257: The job currently always runs the "Install Dependencies"
step which does an unconditional npm ci; add a preceding actions/cache@v4 step
(e.g., id: cache-node-modules) that caches node_modules keyed on
**/package-lock.json, then change the "Install Dependencies" step (e.g., id:
install-deps) to run npm ci only when the cache miss occurs by gating it with
if: steps.cache-node-modules.outputs.cache-hit != 'true' (or equivalent output
name), ensuring npm ci runs only on cache misses and aligns this job with the
other jobs' caching pattern.
0a984a0 to
e2e9a4b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
.github/workflows/vulnerability-scan.yml (3)
17-19:fetch-depth: 0is unnecessary for a vulnerability scan.Full history isn't needed to generate an SBOM or run Trivy. Shallow fetch reduces checkout time.
♻️ Suggested fix
- uses: actions/checkout@v4 with: - fetch-depth: 0 + fetch-depth: 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/vulnerability-scan.yml around lines 17 - 19, Remove the unnecessary full-history checkout in the vulnerability scan workflow by deleting the fetch-depth: 0 setting (or set fetch-depth: 1) from the actions/checkout@v4 step so the job uses a shallow clone; update the checkout invocation that references actions/checkout@v4 to omit the fetch-depth key or change it to 1 to speed up the SBOM/Trivy run.
17-55: Consider pinning third-party actions to commit SHAs.All four third-party actions (
actions/checkout,actions/setup-node,actions/cache,aquasecurity/trivy-action) are pinned by version tag. Tags can be force-pushed, so a compromised tag could silently swap in malicious code. This risk is especially notable for a security-scanning workflow. SHA pinning is the standard mitigation.Example:
- - uses: actions/checkout@v4 + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/vulnerability-scan.yml around lines 17 - 55, The workflow pins third-party GitHub Actions by tags which can be force-updated; update each uses declaration (actions/checkout@v4, actions/setup-node@v4, actions/cache@v4, aquasecurity/trivy-action@0.34.0) to reference the corresponding immutable commit SHA for the exact tag you intend to use, replacing the tag form with the full SHA to prevent silent supply-chain changes; leave the npm package installs as-is but ensure any action references in the workflow use the commit SHAs instead of version tags.
12-15: Add an explicitpermissionsblock for least-privilege posture.Without a
permissionsdeclaration the job inherits the repository's default token permissions, which may include write scopes. A read-only scan job only needscontents: read.🛡️ Suggested fix
jobs: vulnerability-scan: runs-on: ubuntu-latest + permissions: + contents: read timeout-minutes: 10🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/vulnerability-scan.yml around lines 12 - 15, Add an explicit permissions block to the vulnerability-scan job to follow least-privilege practice: in the job definition for "vulnerability-scan" add a permissions stanza granting only contents: read so the job does not inherit broader repository token scopes; ensure the permissions block is at the same level as runs-on/timeout-minutes for the vulnerability-scan job.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/vulnerability-scan.yml:
- Around line 54-62: The Trivy step ("Scan SBOM for vulnerabilities using Trivy"
/ uses: aquasecurity/trivy-action@0.34.0) is configured with exit-code: 0 which
prevents CRITICAL/HIGH results from failing CI; change exit-code to 1 so the job
fails on findings and enforces the gate, and if there are known noisy findings
add a .trivyignore to suppress specific items rather than keeping the action
non-blocking.
- Around line 21-28: The workflow uses actions/setup-node@v4 with node-version:
24.x and installs npm@10, which diverges from release.yml; update the Node setup
to node-version: 20 (use actions/setup-node@v4 still) and change the npm install
to npm@11.6.0 to match release.yml, and update the vulnerability-scan cache key
(the cache key string used for npm dependencies) to reflect the Node/npm version
change so the SBOM and cached modules match the release build.
---
Nitpick comments:
In @.github/workflows/vulnerability-scan.yml:
- Around line 17-19: Remove the unnecessary full-history checkout in the
vulnerability scan workflow by deleting the fetch-depth: 0 setting (or set
fetch-depth: 1) from the actions/checkout@v4 step so the job uses a shallow
clone; update the checkout invocation that references actions/checkout@v4 to
omit the fetch-depth key or change it to 1 to speed up the SBOM/Trivy run.
- Around line 17-55: The workflow pins third-party GitHub Actions by tags which
can be force-updated; update each uses declaration (actions/checkout@v4,
actions/setup-node@v4, actions/cache@v4, aquasecurity/trivy-action@0.34.0) to
reference the corresponding immutable commit SHA for the exact tag you intend to
use, replacing the tag form with the full SHA to prevent silent supply-chain
changes; leave the npm package installs as-is but ensure any action references
in the workflow use the commit SHAs instead of version tags.
- Around line 12-15: Add an explicit permissions block to the vulnerability-scan
job to follow least-privilege practice: in the job definition for
"vulnerability-scan" add a permissions stanza granting only contents: read so
the job does not inherit broader repository token scopes; ensure the permissions
block is at the same level as runs-on/timeout-minutes for the vulnerability-scan
job.
| - name: Use Node.js 24.x | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 24.x | ||
|
|
||
| - name: Setup npm version 10 | ||
| run: | | ||
| npm i -g npm@10 --registry=https://registry.npmjs.org |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for release.yml to verify Node/npm versions used there
find . -name "release.yml" -type fRepository: XRPLF/xrpl.js
Length of output: 88
🏁 Script executed:
# Check the full content of vulnerability-scan.yml to see cache key and all details
find . -name "vulnerability-scan.yml" -type fRepository: XRPLF/xrpl.js
Length of output: 99
🌐 Web query:
Node.js 24 release date February 2026
💡 Result:
Node.js v24 was first released as Node.js 24.0.0 on May 6, 2025 (codename Krypton). It later moved into Active LTS in October 2025 and remains Active LTS as of February 2026. [1][2]
Sources
[1] (nodejs.org)
[2] (nodejs.org)
Citations:
- 1: https://nodejs.org/en/about/previous-releases?utm_source=openai
- 2: https://nodejs.org/pt/blog/release/v24.0.0?utm_source=openai
🏁 Script executed:
cat -n ./.github/workflows/release.yml | head -50Repository: XRPLF/xrpl.js
Length of output: 1829
🏁 Script executed:
cat -n ./.github/workflows/vulnerability-scan.ymlRepository: XRPLF/xrpl.js
Length of output: 2479
🏁 Script executed:
grep -n "setup-node\|node-version\|npm.*version" ./.github/workflows/release.ymlRepository: XRPLF/xrpl.js
Length of output: 336
🏁 Script executed:
grep -n "npm@" ./.github/workflows/release.ymlRepository: XRPLF/xrpl.js
Length of output: 187
Align Node.js and npm versions with release.yml to ensure SBOM consistency.
The vulnerability scan generates an SBOM that should reflect the same dependency tree as the actual release. This workflow uses Node 24.x with npm@10, but release.yml uses Node 20 with npm@11.6.0. Different Node/npm versions can resolve dependencies differently, causing SBOM divergence and potentially missing vulnerabilities that would be present in the release build.
🛠️ Suggested fix
- - name: Use Node.js 24.x
+ - name: Use Node.js 20
uses: actions/setup-node@v4
with:
- node-version: 24.x
+ node-version: 20
- - name: Setup npm version 10
+ - name: Setup npm version 11.6.0
run: |
- npm i -g npm@10 --registry=https://registry.npmjs.org
+ npm i -g npm@11.6.0 --registry=https://registry.npmjs.orgAlso update the cache key:
- key: ${{ runner.os }}-deps-24.x-${{ hashFiles('**/package-lock.json') }}
+ key: ${{ runner.os }}-deps-20.x-${{ hashFiles('**/package-lock.json') }}
restore-keys: |
- ${{ runner.os }}-deps-24.x-
+ ${{ runner.os }}-deps-20.x-🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/vulnerability-scan.yml around lines 21 - 28, The workflow
uses actions/setup-node@v4 with node-version: 24.x and installs npm@10, which
diverges from release.yml; update the Node setup to node-version: 20 (use
actions/setup-node@v4 still) and change the npm install to npm@11.6.0 to match
release.yml, and update the vulnerability-scan cache key (the cache key string
used for npm dependencies) to reflect the Node/npm version change so the SBOM
and cached modules match the release build.
| - name: Scan SBOM for vulnerabilities using Trivy | ||
| uses: aquasecurity/trivy-action@0.34.0 | ||
| with: | ||
| scan-type: sbom | ||
| scan-ref: sbom.json | ||
| format: table | ||
| exit-code: 0 # TODO: Change to 1 to fail CI on vulnerabilities | ||
| output: vuln-report.txt | ||
| severity: CRITICAL,HIGH |
There was a problem hiding this comment.
exit-code: 0 leaves the vulnerability scan toothless as a CI gate.
With exit-code: 0, CRITICAL/HIGH findings will never block a PR or push — the workflow always passes. The inline TODO acknowledges this. Until it's flipped to 1, this workflow provides no enforcement and only serves as an informational report.
🔒 Proposed fix
- exit-code: 0 # TODO: Change to 1 to fail CI on vulnerabilities
+ exit-code: 1If there are known baseline findings that would cause noise, consider adding a Trivy .trivyignore file to suppress them rather than suppressing all enforcement.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Scan SBOM for vulnerabilities using Trivy | |
| uses: aquasecurity/trivy-action@0.34.0 | |
| with: | |
| scan-type: sbom | |
| scan-ref: sbom.json | |
| format: table | |
| exit-code: 0 # TODO: Change to 1 to fail CI on vulnerabilities | |
| output: vuln-report.txt | |
| severity: CRITICAL,HIGH | |
| - name: Scan SBOM for vulnerabilities using Trivy | |
| uses: aquasecurity/trivy-action@0.34.0 | |
| with: | |
| scan-type: sbom | |
| scan-ref: sbom.json | |
| format: table | |
| exit-code: 1 | |
| output: vuln-report.txt | |
| severity: CRITICAL,HIGH |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/vulnerability-scan.yml around lines 54 - 62, The Trivy
step ("Scan SBOM for vulnerabilities using Trivy" / uses:
aquasecurity/trivy-action@0.34.0) is configured with exit-code: 0 which prevents
CRITICAL/HIGH results from failing CI; change exit-code to 1 so the job fails on
findings and enforces the gate, and if there are known noisy findings add a
.trivyignore to suppress specific items rather than keeping the action
non-blocking.
e2e9a4b to
53c0a0e
Compare
…sequential reconnect attempts
kuan121
force-pushed
the
fix-client-connected-event-listener-accumulation-issue
branch
from
53c0a0e to
4aeae98
Compare
1 participant
High Level Overview of Change
Fix event listener accumulation bug in Client's
'connected'event handler during reconnections. The fix cleans up stale listeners from previous reconnect attempts to prevent duplicate event emissions.Context of Change
Bug Report (#3210): Users reported that the
client.on('connected', ...)event handler was being called multiple times after each reconnection, following an N+1 pattern:Root Cause: The Client constructor's reconnect handler was using
.on()to add a new'connected'listener on every reconnection:Each reconnection would add a new persistent listener without removing the previous ones, causing all accumulated listeners to fire on subsequent connections.
Additional Edge Case: On flaky connections with multiple sequential reconnect attempts (via backoff retry logic), multiple
'reconnect'events can fire before a single'connected'event. Even with.once(), this would cause M duplicatethis.emit('connected')calls when connection finally succeeds.Solution: Clean up any stale listener from previous reconnect attempt before adding a new one:
Type of Change
Did you update HISTORY.md?
Test Plan
Unit Test
All existing unit tests pass
Manual Verification
Created test script that simulates 4 connections (1 initial + 3 reconnects):