Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.92%. Comparing base (6c26182) to head (53af319).
⚠️ Report is 6 commits behind head on master.

@@           Coverage Diff           @@
##           master    #1714   +/-   ##
=======================================
  Coverage   79.92%   79.92%           
=======================================
  Files          90       90           
  Lines        4887     4887           
  Branches      920      920           
=======================================
  Hits         3906     3906           
  Misses        676      676           
  Partials      305      305           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Code Review

Thank you for this PR! This is a well-targeted fix for Node.js 22+ compatibility. Here's my detailed review:

✅ Strengths

1. Clear problem identification: The PR correctly identifies that Node.js 22+ changed SSL error code formats from ERR_SSL_SSLV3_ALERT_HANDSHAKE_FAILURE to ERR_SSL_SSL/TLS_ALERT_HANDSHAKE_FAILURE.

2. Good use of existing patterns: The use of assert.include() is consistent with existing test patterns in the codebase (see test/unit/connection-test.ts:40).

3. Helpful inline comment: The comment at line 566 clearly explains why both error codes are accepted.

4. Minimal, focused change: Only touches what's necessary to fix the compatibility issue.

5. Backwards compatible: Maintains support for older Node.js versions while adding support for newer ones.

🔍 Observations & Suggestions

1. CI Coverage Gap (Important)

Looking at .github/workflows/nodejs.yml, the CI matrix currently tests Node.js versions 18.x, 20.x, and 21.x (lines 37-38). However:

• This PR targets Node.js 22+ compatibility
• The issue won't be caught by CI until Node.js 22 is added to the test matrix

Recommendation: Consider adding Node.js 22.x to the CI matrix in a follow-up PR to ensure this fix is actually tested and prevent regressions.

2. Error Message Assertion Order

The assertion for error codes at lines 567-570 lists the old format first, then the new format. For consistency and future-proofing, consider listing them in chronological order (old to new) or in the order of likelihood. The current order is fine, but you could add a brief note about which is which:

assert.include(
  ['ERR_SSL_SSLV3_ALERT_HANDSHAKE_FAILURE', // Node.js < 22
   'ERR_SSL_SSL/TLS_ALERT_HANDSHAKE_FAILURE'], // Node.js >= 22
  (err as Error & { code?: string }).code
);

This is optional but improves readability.

3. Testing Framework Compatibility

The change uses Chai's assert.include() which checks if the second argument is included in the first argument (array). This is the correct usage for checking if a value exists in an array of expected values. ✅

🐛 Potential Issues

None identified - The implementation is sound.

⚡ Performance

No performance concerns - the change is in test code only.

🔒 Security

No security concerns - this is a test-only change.

🧪 Test Coverage

The change improves test coverage by making the test pass on both old and new Node.js versions. However, as noted above, Node.js 22 should be added to the CI matrix to validate this fix.

📝 Code Quality

• Formatting: ✅ Consistent with existing code
• TypeScript types: ✅ Properly typed with inline type assertions
• Comments: ✅ Clear explanatory comment included
• Naming: ✅ N/A

Final Recommendation

✅ Approve with suggestions

This is a clean, targeted fix that solves a real compatibility issue. The implementation is correct and follows existing patterns in the codebase.

Suggested next steps:

1. Merge this PR to fix the immediate issue
2. Follow up with a PR to add Node.js 22.x to the CI matrix

Great work!

Review generated by Claude Code

🎉 This PR is included in version 19.2.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀