Node v24.15.0 nsolid v6.2.3 release#451
Merged
santigimeno merged 273 commits intonode-v24.x-nsolid-v6.xfrom Apr 29, 2026
Merged
Node v24.15.0 nsolid v6.2.3 release#451santigimeno merged 273 commits intonode-v24.x-nsolid-v6.xfrom
santigimeno merged 273 commits intonode-v24.x-nsolid-v6.xfrom
Conversation
PR-URL: nodejs/node#61372 Fixes: nodejs/node#61362 Refs: v8/v8@c5ff7c4 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: René <contact.9a5d6388@renegade334.me.uk>
PR-URL: nodejs/node#61503 Fixes: nodejs/node#61489 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: nodejs/node#61655 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>
PR-URL: nodejs/node#61422 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Ensure that the file handle is closed if header validation fails in respondWithFile. This prevents ERR_INVALID_STATE errors where a FileHandle object is closed during garbage collection. PR-URL: nodejs/node#61707 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Tim Perry <pimterry@gmail.com>
PR-URL: nodejs/node#61715 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
It needs to be added to the list to actually get registered. PR-URL: nodejs/node#61718 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs/node#61673 Reviewed-By: Richard Lau <richard.lau@ibm.com> Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
To help catch unregistered bindings. PR-URL: nodejs/node#61719 Refs: nodejs/node#61718 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs/node#61567 Reviewed-By: Tierney Cyren <hello@bnb.im> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Pietro Marchini <pietro.marchini94@gmail.com>
This aligns zstd streams with other compression libraries in this regard, and enables releasing memory early when the stream ends in JS instead of waiting for GC to clean up the wrapper object (which is a problem that is exacerbated in the zstd context because we do not track memory and report memory pressure to V8 yet). PR-URL: nodejs/node#61717 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
This makes it a bit easier to separate concerns, and results in reduced code duplication when compiling since this code does not depend on template parameters. PR-URL: nodejs/node#61717 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
This addresses a long-standing TODO comment, referencing the fact that these values are either known at compile time or can be inferred from the `this` value in the context class. PR-URL: nodejs/node#61717 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
This is both valuable as a diagnostic tool and as a way to inform the JS runtime about external allocations. Currently, this is a feature only enabled in the statically linked builds of zstd, so with `--shared-zstd`, we fall back to the non-tracking variant. PR-URL: nodejs/node#61717 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
PR-URL: nodejs/node#61757 Reviewed-By: Claudio Wunder <cwunder@gnome.org> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Aviv Keller <me@aviv.sh>
Build it with gdbjit support on supported platforms by default allows debugging JIT-compiled code in gdb when it's also enabled at run time (via --gdbjit). Simply building it in should not incur an overhead if it's not also enabled at run time. PR-URL: nodejs/node#61010 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
PR-URL: nodejs/node#61728 Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs/node#61729 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs/node#61731 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs/node#61756 Reviewed-By: René <contact.9a5d6388@renegade334.me.uk> Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs/node#61735 Fixes: nodejs/node#59282 Reviewed-By: Aviv Keller <me@aviv.sh> Reviewed-By: Claudio Wunder <cwunder@gnome.org> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs/node#42251 Reviewed-By: Stewart X Addison <sxa@redhat.com>
PR-URL: nodejs/node#61765 Reviewed-By: René <contact.9a5d6388@renegade334.me.uk> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs/node#61632 Refs: nodejs/node#58664 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Mattias Buelens <mattias@buelens.com>
Fixes: nodejs/node#61116 Signed-off-by: Juan José Arboleda <soyjuanarbol@gmail.com> PR-URL: nodejs/node#61178 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Signed-off-by: Juan José Arboleda <soyjuanarbol@gmail.com> PR-URL: nodejs/node#61754 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Richard Lau <richard.lau@ibm.com> Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Stewart X Addison <sxa@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs/node#60548 Fixes: nodejs/node#60507 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
PR-URL: nodejs/node#61652 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>
Bumps the eslint group in /tools/eslint with 6 updates: | Package | From | To | | --- | --- | --- | | [@babel/core](https://github.com/babel/babel/tree/HEAD/packages/babel-core) | `7.28.5` | `7.28.6` | | [@babel/eslint-parser](https://github.com/babel/babel/tree/HEAD/eslint/babel-eslint-parser) | `7.28.5` | `7.28.6` | | [@babel/plugin-syntax-import-source](https://github.com/babel/babel/tree/HEAD/packages/babel-plugin-syntax-import-source) | `7.27.1` | `7.28.6` | | [@stylistic/eslint-plugin](https://github.com/eslint-stylistic/eslint-stylistic/tree/HEAD/packages/eslint-plugin) | `5.6.1` | `5.7.1` | | [eslint-plugin-jsdoc](https://github.com/gajus/eslint-plugin-jsdoc) | `61.5.0` | `62.4.1` | | [globals](https://github.com/sindresorhus/globals) | `16.5.0` | `17.2.0` | Updates `@babel/core` from 7.28.5 to 7.28.6 - [Release notes](https://github.com/babel/babel/releases) - [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md) - [Commits](https://github.com/babel/babel/commits/v7.28.6/packages/babel-core) Updates `@babel/eslint-parser` from 7.28.5 to 7.28.6 - [Release notes](https://github.com/babel/babel/releases) - [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md) - [Commits](https://github.com/babel/babel/commits/v7.28.6/eslint/babel-eslint-parser) Updates `@babel/plugin-syntax-import-source` from 7.27.1 to 7.28.6 - [Release notes](https://github.com/babel/babel/releases) - [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md) - [Commits](https://github.com/babel/babel/commits/v7.28.6/packages/babel-plugin-syntax-import-source) Updates `@stylistic/eslint-plugin` from 5.6.1 to 5.7.1 - [Release notes](https://github.com/eslint-stylistic/eslint-stylistic/releases) - [Changelog](https://github.com/eslint-stylistic/eslint-stylistic/blob/main/CHANGELOG.md) - [Commits](https://github.com/eslint-stylistic/eslint-stylistic/commits/v5.7.1/packages/eslint-plugin) Updates `eslint-plugin-jsdoc` from 61.5.0 to 62.4.1 - [Release notes](https://github.com/gajus/eslint-plugin-jsdoc/releases) - [Commits](gajus/eslint-plugin-jsdoc@v61.5.0...v62.4.1) Updates `globals` from 16.5.0 to 17.2.0 - [Release notes](https://github.com/sindresorhus/globals/releases) - [Commits](sindresorhus/globals@v16.5.0...v17.2.0) --- updated-dependencies: - dependency-name: "@babel/core" dependency-version: 7.28.6 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: eslint - dependency-name: "@babel/eslint-parser" dependency-version: 7.28.6 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: eslint - dependency-name: "@babel/plugin-syntax-import-source" dependency-version: 7.28.6 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: eslint - dependency-name: "@stylistic/eslint-plugin" dependency-version: 5.7.1 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: eslint - dependency-name: eslint-plugin-jsdoc dependency-version: 62.4.1 dependency-type: direct:production update-type: version-update:semver-major dependency-group: eslint - dependency-name: globals dependency-version: 17.2.0 dependency-type: direct:production update-type: version-update:semver-major dependency-group: eslint ... Signed-off-by: dependabot[bot] <support@github.com> PR-URL: nodejs/node#61628 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Aviv Keller <me@aviv.sh>
Bumps the doc group in /tools/doc with 1 update: [unist-util-visit](https://github.com/syntax-tree/unist-util-visit). Updates `unist-util-visit` from 5.0.0 to 5.1.0 - [Release notes](https://github.com/syntax-tree/unist-util-visit/releases) - [Commits](syntax-tree/unist-util-visit@5.0.0...5.1.0) --- updated-dependencies: - dependency-name: unist-util-visit dependency-version: 5.1.0 dependency-type: direct:development update-type: version-update:semver-minor dependency-group: doc ... Signed-off-by: dependabot[bot] <support@github.com> PR-URL: nodejs/node#61646 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Original commit message: [wasm][exnref] Enable exnref R=mliedtke@chromium.org CC=ecmziegler@chromium.org Bug: 42204334 Change-Id: I0ddf1d29c936d73f7bb7909775a6bbd9a6ec5e2f Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6458423 Commit-Queue: Thibaud Michaud <thibaudm@chromium.org> Reviewed-by: Matthias Liedtke <mliedtke@chromium.org> Cr-Commit-Position: refs/heads/main@{#99795} PR-URL: nodejs/node#62567 Refs: v8/v8@33e7739 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs/node#61785 Backport-PR-URL: nodejs/node#62586 Fixes: nodejs/node#61690 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs/node#60623 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: nodejs/node#61820 Reviewed-By: René <contact.9a5d6388@renegade334.me.uk> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
This helps ensure that we already set the correct number of internal fields when creating objects, even if the number of internal fields of e.g. AsyncWrap changes over time. PR-URL: nodejs/node#62103 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
PR-URL: nodejs/node#62005 Fixes: nodejs/node#61724 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Using an internal field instead of a `v8::Global<>` removes an unnecessary memory leak footgun. This includes a test that demonstrates the issue, albeit using internal APIs. It's worth noting that if this PR is not accepted, we'd still be missing memory tracking for the `context_frame_` field, and we'd need to add it through our memory tracking API. PR-URL: nodejs/node#62103 Backport-PR-URL: nodejs/node#62357 Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: nodejs/node#62261 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs/node#62483 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Remove the suggestion to use child_process.spawn() with the shell option set for running .bat and .cmd files on Windows. Passing arguments through spawn with shell: true is deprecated (DEP0190) due to shell injection risks. Keep the exec() and direct cmd.exe spawn alternatives. Fixes: nodejs/node#58735 PR-URL: nodejs/node#62243 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>
Notable changes: cli: * (SEMVER-MINOR) add --max-heap-size option (tannal) nodejs/node#58708 * add --require-module/--no-require-module (Joyee Cheung) nodejs/node#60959 crypto: * (SEMVER-MINOR) add raw key formats support to the KeyObject APIs (Filip Skokan) nodejs/node#62240 fs: * (SEMVER-MINOR) add `throwIfNoEntry` option for fs.stat and fs.promises.stat (Juan José) nodejs/node#61178 http2: * (SEMVER-MINOR) add http1Options for HTTP/1 fallback configuration (Amol Yadav) nodejs/node#61713 module: * mark require(esm) as stable (Joyee Cheung) nodejs/node#60959 * mark module compile cache as stable (Joyee Cheung) nodejs/node#60971 net: * (SEMVER-MINOR) add `setTOS` and `getTOS` to `Socket` (Amol Yadav) nodejs/node#61503 sqlite: * (SEMVER-MINOR) add limits property to DatabaseSync (Mert Can Altin) nodejs/node#61298 * mark as release candidate (Matteo Collina) nodejs/node#61262 src: * (SEMVER-MINOR) add C++ support for diagnostics channels (RafaelGSS) nodejs/node#61869 stream: * (SEMVER-MINOR) rename `Duplex.toWeb()` type option to `readableType` (René) nodejs/node#61632 test_runner: * add exports option for module mocks (sangwook) nodejs/node#61727 * (SEMVER-MINOR) expose worker ID for concurrent test execution (Ali Hassan) nodejs/node#61394 * (SEMVER-MINOR) show interrupted test on SIGINT (Matteo Collina) nodejs/node#61676 PR-URL: nodejs/node#62681
WalkthroughThis pull request introduces a new Windows DSC configuration for Visual Studio Build Tools installation, updates multiple GitHub Actions workflows with action version bumps and logic refinements, updates several dependencies (acorn, ada, llhttp, minimatch, nbytes, ncrypto, merve), expands benchmark coverage with new tests and parameter configurations, and updates documentation including BUILDING.md with expanded platform/toolchain support guidance. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deps/ncrypto/ncrypto.h (1)
857-873:⚠️ Potential issue | 🟠 MajorWire the new key formats and point-form option through the serializer/parser before exposing them.
At Line 861 and Line 873, the public config starts advertising
RAW_PUBLIC,RAW_PRIVATE,RAW_SEED, andec_point_form, butdeps/ncrypto/ncrypto.ccstill only handles PEM/DER/JWK inTryParsePublicKey(),TryParsePrivateKey(),writePublicKey(), andwritePrivateKey(), and none of those paths readec_point_form. As written, callers that select the new raw formats still hit the existing failure paths, and non-default point-form requests are silently ignored.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/ncrypto/ncrypto.h` around lines 857 - 873, The public API added RAW_PUBLIC/RAW_PRIVATE/RAW_SEED and ec_point_form in AsymmetricKeyEncodingConfig but the serializer/parser functions still only support PEM/DER/JWK; update TryParsePublicKey, TryParsePrivateKey, writePublicKey, and writePrivateKey to honor PKFormatType::RAW_PUBLIC/RAW_PRIVATE/RAW_SEED and to respect AsymmetricKeyEncodingConfig::ec_point_form when serializing EC keys. Specifically, add parsing branches in TryParsePublicKey/TryParsePrivateKey to accept raw byte inputs (using the appropriate OpenSSL d2i/EVP_PKEY_from_raw/public/private APIs or seed-to-key derivation for RAW_SEED) and add write paths in writePublicKey/writePrivateKey that emit raw public/private/seed formats, and set EC_POINT conversion form via EC_KEY_set_conv_form or i2o_*/o2i_* helpers using ec_point_form when serializing EC keys so callers selecting RAW_* and non-default ec_point_form get the correct behavior.
🧹 Nitpick comments (1)
benchmark/util/strip-vt-control-characters.js (1)
15-28: Add adefaultcase in the input switch for fail-fast behavior.Line 15 currently relies fully on benchmark config correctness. A
defaultbranch makes unexpected inputs explicit and easier to debug.🧩 Suggested hardening
switch (input) { @@ case 'ansi-long': str = ('\u001B[31m' + 'colored text '.repeat(10) + '\u001B[39m').repeat(10); break; + default: + throw new Error(`Unknown input variant: ${input}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/util/strip-vt-control-characters.js` around lines 15 - 28, The switch over the input variable that assigns str lacks a default branch, so add a default case to the switch (the same switch that currently handles 'noAnsi-short', 'noAnsi-long', 'ansi-short', 'ansi-long') that fails fast by throwing an Error (or calling assert) with a clear message including the unexpected input value; this will make the behavior of the code (the assignment to str) explicit and easier to debug when the input is invalid.
🤖 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/coverage-windows.yml:
- Around line 6-28: The paths-ignore block currently contains unsupported
negation patterns '!.github/workflows/coverage-windows.yml' and a broad '.**'
entry that together cause this workflow file to be ignored; remove the negation
lines and ensure the workflow file itself is not excluded (either delete the
'.**' pattern or explicitly remove the workflow filename from the ignore list)
so changes to .github/workflows/coverage-windows.yml will trigger the job;
update the paths-ignore block (reference: paths-ignore, '.**', and
'!.github/workflows/coverage-windows.yml') accordingly.
In `@benchmark/buffers/buffer-bytelength-string.js`:
- Line 7: The benchmark adds 'hex' to the encoding list but getInput() only
prepares base64, so when encoding === 'hex' the test feeds plain text instead of
hex bytes; update getInput() to return a proper hex-encoded string for the hex
case (mirror how base64 is prepared in getInput()), e.g. convert the buffer to
hex when encoding === 'hex', so the buffer.byteLength path measures real hex
input; adjust any variable names in getInput() and references to encoding to
ensure the 'hex' branch is covered.
In `@benchmark/crypto/kem.js`:
- Around line 133-141: The decapsulation warmup uses crypto.encapsulate on a
private key KeyObject (keyObjects[0]) which is invalid; instead derive a public
key from that private KeyObject (use crypto.createPublicKey or the same helper
used elsewhere) and pass the public KeyObject to crypto.encapsulate to produce
warmupCiphertext, then call crypto.decapsulate(keyObject, warmupCiphertext) as
before; update the warmupCiphertext generation and reference crypto.encapsulate,
crypto.decapsulate, and keyObjects to locate the change.
In `@benchmark/util/strip-vt-control-characters.js`:
- Around line 30-34: The benchmark currently measures extra overhead because
assert.ok is executed inside the timed loop and bench.start/stop are misused;
move the assertion out of the measured section by declaring let result; before
the loop, call bench.start() correctly, run for (let i = 0; i < n; i++) { result
= stripVTControlCharacters(str); } then call bench.stop() and finally call
assert.ok(typeof result === 'string'); ensure you reference the existing symbols
stripVTControlCharacters, bench.start(), bench.stop(), assert.ok and the result
variable so the assertion no longer skews timing.
In `@benchmark/webstreams/readable-read-buffered.js`:
- Around line 18-23: The start(controller) implementation enqueues bufferSize
items without checking remaining quota, risking over-enqueueing when bufferSize
> n; update start to use the same boundary logic as pull by computing count =
Math.min(bufferSize, n - enqueued) (or equivalent) and only loop/enqueue that
many times, referencing the existing symbols start, controller, bufferSize,
enqueued, and n to locate and fix the logic.
In `@BUILDING.md`:
- Around line 682-685: The sentence in the WinGet instruction that currently
reads “install Node.js prerequisites” is inconsistent with the project name;
update the wording to refer to N|Solid instead (e.g., “install N|Solid
prerequisites”) in the block that begins “Use one of the above DSC files with
[winget configure]…” and the example line referencing “Visual Studio Community
Edition” so the guide consistently mentions N|Solid rather than Node.js.
In `@configure.py`:
- Around line 108-112: The --debug-symbols parser flag currently only adds a
Unix -g cflag, so update the GYP generation to also set MSVC-specific debug
settings when debug_symbols is true: instead of relying solely on cflags, add
msvs_settings entries for the compile and link steps (e.g., set
VCCLCompilerTool/ClCompile DebugInformationFormat to an appropriate value like
"Zi" or "Z7" and set the linker GenerateDebugInformation/LinkerSetting to true)
and ensure clang-cl builds map to these MSVC settings as well; locate where
debug_symbols is checked and -g is injected (the code that appends to cflags)
and augment it to emit the msvs_settings blocks for Windows/MSVC toolchains.
In `@deps/ncrypto/ncrypto.cc`:
- Around line 3558-3567: The uncompressed-point build currently ignores the
return values of x.encodePaddedInto(...) and y.encodePaddedInto(...), which can
leave the buffer zero-filled and allow invalid coordinates to be fed into
EC_POINT_oct2point(); modify the code around
DataPointer::Alloc/uncompressed_len/ptr and the two calls to encodePaddedInto so
you check each call's boolean/result and if either encodePaddedInto fails return
false (or otherwise abort) before calling EC_POINT_oct2point(); ensure you
reference the same symbols (ptr, field_len, encodePaddedInto,
EC_POINT_oct2point) so the function rejects bad/null/oversized coordinates the
same way as the affine path.
In `@deps/npm/lib/utils/verify-signatures.js`:
- Around line 63-67: Before emitting the JSON, ensure the attestations array is
sorted so output is deterministic: when building result with result.verified =
this.verified (guarded by this.npm.config.get('include-attestations')), sort
this.verified into a stable, well-defined order (e.g., by a unique key or string
compare) before calling output.buffer(result); apply the same sorting change to
the other occurrence that assigns result.verified around the later block (the
one at the other location referenced in the review) so both outputs are
deterministic.
---
Outside diff comments:
In `@deps/ncrypto/ncrypto.h`:
- Around line 857-873: The public API added RAW_PUBLIC/RAW_PRIVATE/RAW_SEED and
ec_point_form in AsymmetricKeyEncodingConfig but the serializer/parser functions
still only support PEM/DER/JWK; update TryParsePublicKey, TryParsePrivateKey,
writePublicKey, and writePrivateKey to honor
PKFormatType::RAW_PUBLIC/RAW_PRIVATE/RAW_SEED and to respect
AsymmetricKeyEncodingConfig::ec_point_form when serializing EC keys.
Specifically, add parsing branches in TryParsePublicKey/TryParsePrivateKey to
accept raw byte inputs (using the appropriate OpenSSL
d2i/EVP_PKEY_from_raw/public/private APIs or seed-to-key derivation for
RAW_SEED) and add write paths in writePublicKey/writePrivateKey that emit raw
public/private/seed formats, and set EC_POINT conversion form via
EC_KEY_set_conv_form or i2o_*/o2i_* helpers using ec_point_form when serializing
EC keys so callers selecting RAW_* and non-default ec_point_form get the correct
behavior.
---
Nitpick comments:
In `@benchmark/util/strip-vt-control-characters.js`:
- Around line 15-28: The switch over the input variable that assigns str lacks a
default branch, so add a default case to the switch (the same switch that
currently handles 'noAnsi-short', 'noAnsi-long', 'ansi-short', 'ansi-long') that
fails fast by throwing an Error (or calling assert) with a clear message
including the unexpected input value; this will make the behavior of the code
(the assignment to str) explicit and easier to debug when the input is invalid.
🪄 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: 5c901a09-f8aa-4bcd-8159-2795fd101fd7
⛔ Files ignored due to path filters (48)
deps/acorn/acorn-walk/dist/walk.d.mtsis excluded by!**/dist/**deps/acorn/acorn-walk/dist/walk.d.tsis excluded by!**/dist/**deps/acorn/acorn-walk/dist/walk.jsis excluded by!**/dist/**deps/acorn/acorn-walk/dist/walk.mjsis excluded by!**/dist/**deps/acorn/acorn/dist/acorn.d.mtsis excluded by!**/dist/**deps/acorn/acorn/dist/acorn.d.tsis excluded by!**/dist/**deps/acorn/acorn/dist/acorn.jsis excluded by!**/dist/**deps/acorn/acorn/dist/acorn.mjsis excluded by!**/dist/**deps/amaro/dist/index.jsis excluded by!**/dist/**deps/amaro/dist/package.jsonis excluded by!**/dist/**deps/icu-small/source/data/in/icudt78l.dat.bz2is excluded by!**/*.bz2deps/minimatch/dist/commonjs/assert-valid-pattern.d.tsis excluded by!**/dist/**deps/minimatch/dist/commonjs/assert-valid-pattern.d.ts.mapis excluded by!**/dist/**,!**/*.mapdeps/minimatch/dist/commonjs/assert-valid-pattern.js.mapis excluded by!**/dist/**,!**/*.mapdeps/minimatch/dist/commonjs/ast.d.tsis excluded by!**/dist/**deps/minimatch/dist/commonjs/ast.d.ts.mapis excluded by!**/dist/**,!**/*.mapdeps/minimatch/dist/commonjs/ast.jsis excluded by!**/dist/**deps/minimatch/dist/commonjs/ast.js.mapis excluded by!**/dist/**,!**/*.mapdeps/minimatch/dist/commonjs/brace-expressions.d.ts.mapis excluded by!**/dist/**,!**/*.mapdeps/minimatch/dist/commonjs/brace-expressions.jsis excluded by!**/dist/**deps/minimatch/dist/commonjs/brace-expressions.js.mapis excluded by!**/dist/**,!**/*.mapdeps/minimatch/dist/commonjs/escape.jsis excluded by!**/dist/**deps/minimatch/dist/commonjs/escape.js.mapis excluded by!**/dist/**,!**/*.mapdeps/minimatch/dist/commonjs/index.d.tsis excluded by!**/dist/**deps/minimatch/dist/commonjs/index.d.ts.mapis excluded by!**/dist/**,!**/*.mapdeps/minimatch/dist/commonjs/index.jsis excluded by!**/dist/**deps/minimatch/dist/commonjs/index.js.mapis excluded by!**/dist/**,!**/*.mapdeps/minimatch/dist/commonjs/unescape.jsis excluded by!**/dist/**deps/minimatch/dist/commonjs/unescape.js.mapis excluded by!**/dist/**,!**/*.mapdeps/minimatch/dist/esm/assert-valid-pattern.d.tsis excluded by!**/dist/**deps/minimatch/dist/esm/assert-valid-pattern.d.ts.mapis excluded by!**/dist/**,!**/*.mapdeps/minimatch/dist/esm/assert-valid-pattern.js.mapis excluded by!**/dist/**,!**/*.mapdeps/minimatch/dist/esm/ast.d.tsis excluded by!**/dist/**deps/minimatch/dist/esm/ast.d.ts.mapis excluded by!**/dist/**,!**/*.mapdeps/minimatch/dist/esm/ast.jsis excluded by!**/dist/**deps/minimatch/dist/esm/ast.js.mapis excluded by!**/dist/**,!**/*.mapdeps/minimatch/dist/esm/brace-expressions.d.ts.mapis excluded by!**/dist/**,!**/*.mapdeps/minimatch/dist/esm/brace-expressions.jsis excluded by!**/dist/**deps/minimatch/dist/esm/brace-expressions.js.mapis excluded by!**/dist/**,!**/*.mapdeps/minimatch/dist/esm/escape.jsis excluded by!**/dist/**deps/minimatch/dist/esm/escape.js.mapis excluded by!**/dist/**,!**/*.mapdeps/minimatch/dist/esm/index.d.tsis excluded by!**/dist/**deps/minimatch/dist/esm/index.d.ts.mapis excluded by!**/dist/**,!**/*.mapdeps/minimatch/dist/esm/index.jsis excluded by!**/dist/**deps/minimatch/dist/esm/index.js.mapis excluded by!**/dist/**,!**/*.mapdeps/minimatch/dist/esm/unescape.jsis excluded by!**/dist/**deps/minimatch/dist/esm/unescape.js.mapis excluded by!**/dist/**,!**/*.mapdeps/minimatch/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (252)
.configurations/configuration.vsBuildTools.dsc.yaml.github/workflows/auto-start-ci.yml.github/workflows/build-tarball.yml.github/workflows/close-stale-feature-requests.yml.github/workflows/close-stalled.yml.github/workflows/codeql.yml.github/workflows/coverage-windows.yml.github/workflows/daily-wpt-fyi.yml.github/workflows/doc.yml.github/workflows/lint-release-proposal.yml.github/workflows/notify-on-push.yml.github/workflows/scorecard.yml.github/workflows/timezone-update.yml.github/workflows/tools.ymlBUILDING.mdCHANGELOG.mdMakefilebenchmark/buffers/buffer-bytelength-string.jsbenchmark/buffers/buffer-copy-bytes-from.jsbenchmark/buffers/buffer-fill.jsbenchmark/buffers/buffer-indexof.jsbenchmark/buffers/buffer-tostring.jsbenchmark/buffers/buffer-transcode.jsbenchmark/crypto/create-keyobject.jsbenchmark/crypto/hash-stream-creation.jsbenchmark/crypto/hash-stream-throughput.jsbenchmark/crypto/kem.jsbenchmark/crypto/oneshot-sign.jsbenchmark/crypto/oneshot-verify.jsbenchmark/crypto/rsa-sign-verify-throughput.jsbenchmark/dgram/single-buffer.jsbenchmark/fixtures/empty.mjsbenchmark/fixtures/import-builtins.mjsbenchmark/misc/startup-core.jsbenchmark/util/strip-vt-control-characters.jsbenchmark/webstreams/readable-read-buffered.jscommon.gypiconfigure.pydeps/acorn/acorn-walk/CHANGELOG.mddeps/acorn/acorn-walk/README.mddeps/acorn/acorn-walk/package.jsondeps/acorn/acorn/CHANGELOG.mddeps/acorn/acorn/README.mddeps/acorn/acorn/package.jsondeps/ada/ada.cppdeps/ada/ada.gypdeps/ada/ada.hdeps/amaro/README.mddeps/amaro/package.jsondeps/googletest/include/gtest/gtest-test-part.hdeps/googletest/include/gtest/gtest.hdeps/googletest/include/gtest/internal/gtest-internal.hdeps/googletest/include/gtest/internal/gtest-port.hdeps/googletest/src/gtest-port.ccdeps/googletest/src/gtest-test-part.ccdeps/googletest/src/gtest.ccdeps/llhttp/.gitignoredeps/llhttp/CMakeLists.txtdeps/llhttp/README.mddeps/llhttp/include/llhttp.hdeps/llhttp/src/llhttp.cdeps/merve/BUILD.gndeps/merve/merve.cppdeps/merve/merve.hdeps/merve/merve_c.hdeps/merve/unofficial.gnideps/minimatch/README.mddeps/minimatch/index.jsdeps/minimatch/package.jsondeps/nbytes/.release-please-manifest.jsondeps/nbytes/CHANGELOG.mddeps/nbytes/CMakeLists.txtdeps/nbytes/include/nbytes.hdeps/nbytes/nbytes.pc.indeps/nbytes/release-please-config.jsondeps/nbytes/src/nbytes.cppdeps/ncrypto/ncrypto.ccdeps/ncrypto/ncrypto.hdeps/npm/docs/content/commands/npm-audit.mddeps/npm/docs/content/commands/npm-install-test.mddeps/npm/docs/content/commands/npm-install.mddeps/npm/docs/content/commands/npm-ls.mddeps/npm/docs/content/commands/npm-outdated.mddeps/npm/docs/content/commands/npm-publish.mddeps/npm/docs/content/commands/npm-trust.mddeps/npm/docs/content/commands/npm-update.mddeps/npm/docs/content/commands/npm.mddeps/npm/docs/content/using-npm/config.mddeps/npm/docs/content/using-npm/scripts.mddeps/npm/docs/content/using-npm/workspaces.mddeps/npm/docs/lib/index.jsdeps/npm/docs/output/commands/npm-access.htmldeps/npm/docs/output/commands/npm-adduser.htmldeps/npm/docs/output/commands/npm-audit.htmldeps/npm/docs/output/commands/npm-bugs.htmldeps/npm/docs/output/commands/npm-cache.htmldeps/npm/docs/output/commands/npm-ci.htmldeps/npm/docs/output/commands/npm-completion.htmldeps/npm/docs/output/commands/npm-config.htmldeps/npm/docs/output/commands/npm-dedupe.htmldeps/npm/docs/output/commands/npm-deprecate.htmldeps/npm/docs/output/commands/npm-diff.htmldeps/npm/docs/output/commands/npm-dist-tag.htmldeps/npm/docs/output/commands/npm-docs.htmldeps/npm/docs/output/commands/npm-doctor.htmldeps/npm/docs/output/commands/npm-edit.htmldeps/npm/docs/output/commands/npm-exec.htmldeps/npm/docs/output/commands/npm-explain.htmldeps/npm/docs/output/commands/npm-explore.htmldeps/npm/docs/output/commands/npm-find-dupes.htmldeps/npm/docs/output/commands/npm-fund.htmldeps/npm/docs/output/commands/npm-get.htmldeps/npm/docs/output/commands/npm-help-search.htmldeps/npm/docs/output/commands/npm-help.htmldeps/npm/docs/output/commands/npm-init.htmldeps/npm/docs/output/commands/npm-install-ci-test.htmldeps/npm/docs/output/commands/npm-install-test.htmldeps/npm/docs/output/commands/npm-install.htmldeps/npm/docs/output/commands/npm-link.htmldeps/npm/docs/output/commands/npm-ll.htmldeps/npm/docs/output/commands/npm-login.htmldeps/npm/docs/output/commands/npm-logout.htmldeps/npm/docs/output/commands/npm-ls.htmldeps/npm/docs/output/commands/npm-org.htmldeps/npm/docs/output/commands/npm-outdated.htmldeps/npm/docs/output/commands/npm-owner.htmldeps/npm/docs/output/commands/npm-pack.htmldeps/npm/docs/output/commands/npm-ping.htmldeps/npm/docs/output/commands/npm-pkg.htmldeps/npm/docs/output/commands/npm-prefix.htmldeps/npm/docs/output/commands/npm-profile.htmldeps/npm/docs/output/commands/npm-prune.htmldeps/npm/docs/output/commands/npm-publish.htmldeps/npm/docs/output/commands/npm-query.htmldeps/npm/docs/output/commands/npm-rebuild.htmldeps/npm/docs/output/commands/npm-repo.htmldeps/npm/docs/output/commands/npm-restart.htmldeps/npm/docs/output/commands/npm-root.htmldeps/npm/docs/output/commands/npm-run.htmldeps/npm/docs/output/commands/npm-sbom.htmldeps/npm/docs/output/commands/npm-search.htmldeps/npm/docs/output/commands/npm-set.htmldeps/npm/docs/output/commands/npm-shrinkwrap.htmldeps/npm/docs/output/commands/npm-star.htmldeps/npm/docs/output/commands/npm-stars.htmldeps/npm/docs/output/commands/npm-start.htmldeps/npm/docs/output/commands/npm-stop.htmldeps/npm/docs/output/commands/npm-team.htmldeps/npm/docs/output/commands/npm-test.htmldeps/npm/docs/output/commands/npm-token.htmldeps/npm/docs/output/commands/npm-trust.htmldeps/npm/docs/output/commands/npm-undeprecate.htmldeps/npm/docs/output/commands/npm-uninstall.htmldeps/npm/docs/output/commands/npm-unpublish.htmldeps/npm/docs/output/commands/npm-unstar.htmldeps/npm/docs/output/commands/npm-update.htmldeps/npm/docs/output/commands/npm-version.htmldeps/npm/docs/output/commands/npm-view.htmldeps/npm/docs/output/commands/npm-whoami.htmldeps/npm/docs/output/commands/npm.htmldeps/npm/docs/output/commands/npx.htmldeps/npm/docs/output/configuring-npm/folders.htmldeps/npm/docs/output/configuring-npm/install.htmldeps/npm/docs/output/configuring-npm/npm-global.htmldeps/npm/docs/output/configuring-npm/npm-json.htmldeps/npm/docs/output/configuring-npm/npm-shrinkwrap-json.htmldeps/npm/docs/output/configuring-npm/npmrc.htmldeps/npm/docs/output/configuring-npm/package-json.htmldeps/npm/docs/output/configuring-npm/package-lock-json.htmldeps/npm/docs/output/using-npm/config.htmldeps/npm/docs/output/using-npm/dependency-selectors.htmldeps/npm/docs/output/using-npm/developers.htmldeps/npm/docs/output/using-npm/logging.htmldeps/npm/docs/output/using-npm/orgs.htmldeps/npm/docs/output/using-npm/package-spec.htmldeps/npm/docs/output/using-npm/registry.htmldeps/npm/docs/output/using-npm/removal.htmldeps/npm/docs/output/using-npm/scope.htmldeps/npm/docs/output/using-npm/scripts.htmldeps/npm/docs/output/using-npm/workspaces.htmldeps/npm/lib/arborist-cmd.jsdeps/npm/lib/base-cmd.jsdeps/npm/lib/cli/entry.jsdeps/npm/lib/cli/exit-handler.jsdeps/npm/lib/cli/update-notifier.jsdeps/npm/lib/cli/validate-engines.jsdeps/npm/lib/commands/audit.jsdeps/npm/lib/commands/cache.jsdeps/npm/lib/commands/ci.jsdeps/npm/lib/commands/completion.jsdeps/npm/lib/commands/config.jsdeps/npm/lib/commands/dedupe.jsdeps/npm/lib/commands/diff.jsdeps/npm/lib/commands/dist-tag.jsdeps/npm/lib/commands/doctor.jsdeps/npm/lib/commands/exec.jsdeps/npm/lib/commands/explore.jsdeps/npm/lib/commands/find-dupes.jsdeps/npm/lib/commands/fund.jsdeps/npm/lib/commands/help-search.jsdeps/npm/lib/commands/help.jsdeps/npm/lib/commands/init.jsdeps/npm/lib/commands/install-ci-test.jsdeps/npm/lib/commands/install-test.jsdeps/npm/lib/commands/install.jsdeps/npm/lib/commands/link.jsdeps/npm/lib/commands/ls.jsdeps/npm/lib/commands/org.jsdeps/npm/lib/commands/outdated.jsdeps/npm/lib/commands/pack.jsdeps/npm/lib/commands/pkg.jsdeps/npm/lib/commands/profile.jsdeps/npm/lib/commands/prune.jsdeps/npm/lib/commands/publish.jsdeps/npm/lib/commands/run.jsdeps/npm/lib/commands/sbom.jsdeps/npm/lib/commands/shrinkwrap.jsdeps/npm/lib/commands/team.jsdeps/npm/lib/commands/trust/index.jsdeps/npm/lib/commands/unpublish.jsdeps/npm/lib/commands/update.jsdeps/npm/lib/commands/view.jsdeps/npm/lib/lifecycle-cmd.jsdeps/npm/lib/npm.jsdeps/npm/lib/package-url-cmd.jsdeps/npm/lib/utils/audit-error.jsdeps/npm/lib/utils/auth.jsdeps/npm/lib/utils/cmd-list.jsdeps/npm/lib/utils/error-message.jsdeps/npm/lib/utils/explain-dep.jsdeps/npm/lib/utils/explain-eresolve.jsdeps/npm/lib/utils/format-bytes.jsdeps/npm/lib/utils/format-search-stream.jsdeps/npm/lib/utils/get-identity.jsdeps/npm/lib/utils/log-file.jsdeps/npm/lib/utils/oidc.jsdeps/npm/lib/utils/open-url.jsdeps/npm/lib/utils/ping.jsdeps/npm/lib/utils/queryable.jsdeps/npm/lib/utils/reify-finish.jsdeps/npm/lib/utils/reify-output.jsdeps/npm/lib/utils/update-workspaces.jsdeps/npm/lib/utils/verify-signatures.jsdeps/npm/man/man1/npm-access.1deps/npm/man/man1/npm-adduser.1deps/npm/man/man1/npm-audit.1deps/npm/man/man1/npm-bugs.1deps/npm/man/man1/npm-cache.1deps/npm/man/man1/npm-ci.1deps/npm/man/man1/npm-completion.1deps/npm/man/man1/npm-config.1deps/npm/man/man1/npm-dedupe.1
💤 Files with no reviewable changes (6)
- deps/npm/docs/content/using-npm/scripts.md
- deps/npm/lib/commands/install-ci-test.js
- deps/npm/lib/commands/install-test.js
- deps/npm/docs/content/commands/npm-trust.md
- deps/npm/lib/commands/find-dupes.js
- deps/npm/lib/commands/prune.js
Comment on lines
+6
to
+28
| paths-ignore: | ||
| - '**.md' | ||
| - '**.nix' | ||
| - eslint.config.mjs | ||
| - '**/eslint.config_partial.mjs' | ||
| - android-configure | ||
| - android-configure.py | ||
| - android-patches/** | ||
| - benchmarks/** | ||
| - doc/** | ||
| - pyproject.yml | ||
| - tsconfig.json | ||
| - test/internet/** | ||
| - tools/actions/** | ||
| - tools/bootstrap/** | ||
| - tools/dep_updaters/** | ||
| - tools/doc/** | ||
| - tools/eslint-rules/** | ||
| - tools/eslint/** | ||
| - tools/lint-md/** | ||
| - typings/** | ||
| - .** | ||
| - '!.github/workflows/coverage-windows.yml' |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In GitHub Actions, do pull_request.paths-ignoreandpush.paths-ignoresupport negated patterns like!path to re-include files? What is the documented approach to include one workflow file while broadly ignoring dotfiles?
💡 Result:
Yes—GitHub Actions path filters support negated patterns (glob patterns prefixed with !) but they apply within the paths filter, not within paths-ignore. 1) paths-ignore does NOT re-include via ! - The documented semantics for paths-ignore are: “When all the path names match patterns in paths-ignore, the workflow will not run. If any path names do not match patterns in paths-ignore … the workflow will run.” This means paths-ignore is purely an exclusion list; you can’t reliably “subtract then add back” using ! inside paths-ignore. 2) Documented approach: use paths with positive pattern(s) plus ! exclusions - If you want both include and exclude logic for one event, the docs say to use the paths filter and prefix excluded entries with ! (negative patterns). - Also, if you define a ! negated path, you must define at least one non-! (positive) pattern. Practical example (broadly ignore dotfiles, but still include one specific workflow file) - Put a broad positive include for what you want to generally consider, then exclude dotfiles, and finally add back the specific file using another negated/excluded ordering rule via paths. - Example structure: on: pull_request: paths: - '' - '!/.*' # ignore dotfiles/hidden files and hidden directories - '!.github/workflows/ci.yml' # re-include one workflow file (note: must be expressed in paths, not paths-ignore) Notes: - Use paths (not paths-ignore) when you need “ignore dotfiles but include one file” behavior. - The docs also note ordering matters and that negated patterns are evaluated in order. Sources support: - GitHub Docs: paths/paths-ignore behavior and the rule that for include+exclude you must use paths with !, plus the requirement for at least one positive pattern when using !.
Citations:
- 1: https://docs.github.com/actions/using-workflows/workflow-syntax-for-github-actions
- 2: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions
- 3: https://stackoverflow.com/questions/71070064/github-workflow-excluding-paths-does-not-work
Remove negation patterns from paths-ignore—they don't re-include files.
paths-ignore in GitHub Actions does not support negation patterns (prefixed with !). The pattern !.github/workflows/coverage-windows.yml on lines 28 and 54 has no effect. Combined with the .** pattern, changes to this workflow file itself will be ignored, preventing the workflow from running on its own edits.
Suggested fix
pull_request:
types: [opened, synchronize, reopened, ready_for_review]
paths-ignore:
@@
- - .**
- - '!.github/workflows/coverage-windows.yml'
@@
push:
@@
paths-ignore:
@@
- - .**
- - '!.github/workflows/coverage-windows.yml'Also applies to: 32-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/coverage-windows.yml around lines 6 - 28, The paths-ignore
block currently contains unsupported negation patterns
'!.github/workflows/coverage-windows.yml' and a broad '.**' entry that together
cause this workflow file to be ignored; remove the negation lines and ensure the
workflow file itself is not excluded (either delete the '.**' pattern or
explicitly remove the workflow filename from the ignore list) so changes to
.github/workflows/coverage-windows.yml will trigger the job; update the
paths-ignore block (reference: paths-ignore, '.**', and
'!.github/workflows/coverage-windows.yml') accordingly.
| type: ['one_byte', 'two_bytes', 'three_bytes', | ||
| 'four_bytes', 'latin1'], | ||
| encoding: ['utf8', 'base64'], | ||
| encoding: ['utf8', 'base64', 'latin1', 'hex'], |
There was a problem hiding this comment.
Hex case is added at Line 7, but input is never hex-encoded
getInput() only prepares base64 (Lines 24-26). With encoding === 'hex', the benchmark feeds regular text, so this path mostly measures invalid-hex handling rather than normal hex byte-length behavior.
Proposed fix
function getInput(type, repeat, encoding) {
const original = (repeat === 1) ? chars[type] : chars[type].repeat(repeat);
if (encoding === 'base64') {
return Buffer.from(original, 'utf8').toString('base64');
}
+ if (encoding === 'hex') {
+ return Buffer.from(original, 'utf8').toString('hex');
+ }
return original;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmark/buffers/buffer-bytelength-string.js` at line 7, The benchmark adds
'hex' to the encoding list but getInput() only prepares base64, so when encoding
=== 'hex' the test feeds plain text instead of hex bytes; update getInput() to
return a proper hex-encoded string for the hex case (mirror how base64 is
prepared in getInput()), e.g. convert the buffer to hex when encoding === 'hex',
so the buffer.byteLength path measures real hex input; adjust any variable names
in getInput() and references to encoding to ensure the 'hex' branch is covered.
Comment on lines
+133
to
+141
| // Warm up OpenSSL's provider operation cache for each key object | ||
| if (isEncapsulate) { | ||
| for (const keyObject of keyObjects) { | ||
| crypto.encapsulate(keyObject); | ||
| } | ||
| } else { | ||
| const warmupCiphertext = crypto.encapsulate(keyObjects[0]).ciphertext; | ||
| for (const keyObject of keyObjects) { | ||
| crypto.decapsulate(keyObject, warmupCiphertext); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Benchmark warmup path in benchmark/crypto/kem.js:\n'
rg -n -C2 'createKeyFn = isEncapsulate \? crypto\.createPublicKey : crypto\.createPrivateKey|crypto\.encapsulate\(keyObjects\[0\]\)' benchmark/crypto/kem.js
printf '\nRelevant crypto API docs in the repository:\n'
rg -n -C2 'Key encapsulation using a KEM algorithm with a public key|Key decapsulation using a KEM algorithm with a private key|crypto\.encapsulate\(key\[, callback\]\)|crypto\.decapsulate\(key, ciphertext\[, callback\]\)'Repository: nodesource/nsolid
Length of output: 1800
🏁 Script executed:
cat benchmark/crypto/kem.jsRepository: nodesource/nsolid
Length of output: 1885
🌐 Web query:
Node.js crypto.encapsulate() crypto.decapsulate() public key private key requirements documentation
💡 Result:
Node.js provides crypto.encapsulate and crypto.decapsulate functions in the node:crypto module for Key Encapsulation Mechanisms (KEMs), added in recent versions (around v24.7.0+ with OpenSSL 3.0+ support, stability 1.2 - release candidate). These are used for post-quantum cryptography like ML-KEM and other algorithms. ### crypto.encapsulate(key[, callback]) - key: Public key (KeyObject, or input passed to crypto.createPublicKey). Supported types: * 'rsa' (OpenSSL >=3.0): RSA Secret Value Encapsulation * 'ec' (OpenSSL >=3.2): DHKEM(P-256/P-384/P-521, HKDF-SHA256) * 'x25519' (OpenSSL >=3.2): DHKEM(X25519, HKDF-SHA256) * 'x448' (OpenSSL >=3.2): DHKEM(X448, HKDF-SHA512) * 'ml-kem-512/768/1024' (OpenSSL >=3.5): ML-KEM - Returns synchronously: { sharedKey: Buffer, ciphertext: Buffer } - Async via callback: (err, { sharedKey, ciphertext }) ### crypto.decapsulate(key, ciphertext[, callback]) - key: Private key (KeyObject, or input passed to crypto.createPrivateKey). Same supported types as above. - ciphertext: Buffer from encapsulate. - Returns synchronously: sharedKey: Buffer - Async via callback: (err, sharedKey) Keys are generated with crypto.generateKeyPair('ml-kem-512') etc., returning { publicKey, privateKey } as KeyObjects. The Web Crypto API (globalThis.crypto.subtle) has similar methods: encapsulateBits, decapsulateBits, etc., primarily for ML-KEM, using CryptoKey objects with usages like 'encapsulateBits'/'decapsulateBits'. Example (node:crypto): const { generateKeyPairSync, encapsulate, decapsulate } = require('node:crypto'); const { publicKey, privateKey } = generateKeyPairSync('ml-kem-512'); const { sharedKey: encShared, ciphertext } = encapsulate(publicKey); const sharedKey = decapsulate(privateKey, ciphertext); console.log(encShared.equals(sharedKey)); // true Requires Node.js built with OpenSSL supporting KEM (3.0+), throws ERR_CRYPTO_KEM_NOT_SUPPORTED otherwise.
Citations:
- 1: https://nodejs.org/api/webcrypto.html
- 2: https://bun.com/reference/node/crypto/decapsulate
- 3: https://dev.to/zaheetdev/nodejs-v2470-released-post-quantum-cryptography-modern-webcrypto-and-more-1df9
- 4: nodejs/node@61c3bcdc56
- 5: crypto: support ML-KEM, DHKEM, and RSASVE key encapsulation mechanisms nodejs/node#59491
- 6: nodejs/node@14741539a7
- 7: https://nodejs.org/docs/latest-v25.x/api/crypto.html
- 8: https://nodejs.org/api/crypto.html
- 9: https://nodejs.org/docs/latest/api/webcrypto.html
- 10: nodejs/node@18a2ee5b6c
- 11: http://nodejs.org/docs/latest/api/webcrypto.html
Derive a public key before generating the decapsulation warmup ciphertext.
Line 139 calls crypto.encapsulate(keyObjects[0]) in the decapsulation branch, but keyObjects[0] is a private KeyObject created via crypto.createPrivateKey(). The Node.js crypto API requires encapsulate() to receive a public key, while decapsulate() is the private-key counterpart. This will fail at runtime. Derive a public key first, matching what the ciphertext pre-generation block already does.
💡 Proposed fix
} else {
- const warmupCiphertext = crypto.encapsulate(keyObjects[0]).ciphertext;
+ const warmupCiphertext =
+ crypto.encapsulate(crypto.createPublicKey(keyObjects[0])).ciphertext;
for (const keyObject of keyObjects) {
crypto.decapsulate(keyObject, warmupCiphertext);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmark/crypto/kem.js` around lines 133 - 141, The decapsulation warmup
uses crypto.encapsulate on a private key KeyObject (keyObjects[0]) which is
invalid; instead derive a public key from that private KeyObject (use
crypto.createPublicKey or the same helper used elsewhere) and pass the public
KeyObject to crypto.encapsulate to produce warmupCiphertext, then call
crypto.decapsulate(keyObject, warmupCiphertext) as before; update the
warmupCiphertext generation and reference crypto.encapsulate,
crypto.decapsulate, and keyObjects to locate the change.
Comment on lines
+30
to
+34
| bench.start(); | ||
| for (let i = 0; i < n; i++) { | ||
| const result = stripVTControlCharacters(str); | ||
| assert.ok(typeof result === 'string'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether assertions are inside the timed benchmark section in this file.
python - <<'PY'
from pathlib import Path
p = Path("benchmark/util/strip-vt-control-characters.js")
lines = p.read_text().splitlines()
start = end = None
for i, line in enumerate(lines, 1):
if "bench.start()" in line:
start = i
if "bench.end(" in line:
end = i
print(f"timed_section={start}-{end}")
for i in range(start, end + 1):
if "assert." in lines[i - 1]:
print(f"assert_in_timed_section: Line {i}: {lines[i - 1].strip()}")
PYRepository: nodesource/nsolid
Length of output: 1800
🏁 Script executed:
#!/bin/bash
# Verify the current state of the benchmark file
cat -n benchmark/util/strip-vt-control-characters.jsRepository: nodesource/nsolid
Length of output: 1885
🌐 Web query:
benchmark/util/strip-vt-control-characters.js nodesource/nsolid github
💡 Result:
The file benchmark/util/strip-vt-control-characters.js is a Node.js core benchmark script added in PR #61833 (merged Feb 24, 2026) to test performance improvements in util.stripVTControlCharacters, particularly a fast path for strings without ANSI escape codes using String.prototype.indexOf checks for ESC (\u001B) and CSI (\u009B). It benchmarks short/long inputs with/without ANSI codes using Node's benchmark runner, showing ~1.93x faster for short non-ANSI and ~6.91x for long non-ANSI strings. No evidence found of this exact file or path in nodesource/nsolid GitHub repository (https://github.com/nodesource/nsolid), which is a Node.js fork (latest branch node-v24.x-nsolid-v6.x, last push Apr 2, 2026). N|Solid does not appear to have a public 'benchmark/util' directory or specific references to this file. NodeSource maintains separate benchmark repos like nodejs-package-benchmark, but none match this path. The query likely refers to the Node.js core file, possibly in context of N|Solid performance testing.
Citations:
- 1: util: add fast path to stripVTControlCharacters nodejs/node#61833
- 2: nodejs/node@192c0382f4
- 3: https://github.com/nodesource/nsolid
- 4: https://github.com/nodesource/nsolid/pulls
- 5: https://github.com/nodesource/nodejs-package-benchmark
🌐 Web query:
site:github.com/nodejs/node benchmark/util/strip-vt-control-characters.js raw content
💡 Result:
'use strict'; const common = require('../common.js'); const { stripVTControlCharacters } = require('node:util'); const assert = require('node:assert'); const bench = common.createBenchmark(main, { input: ['noAnsi-short', 'noAnsi-long', 'ansi-short', 'ansi-long'], n: [1e6], }); function main({ input, n }) { let str; switch (input) { case 'noAnsi-short': str = 'This is a plain text string without any ANSI codes'; break; case 'noAnsi-long': str = 'Long plain text without ANSI. '.repeat(333); break; case 'ansi-short': str = '\u001B[31mHello\u001B[39m'; break; case 'ansi-long': str = ('\u001B[31m' + 'colored text '.repeat(10) + '\u001B[39m').repeat(10); break; } bench.start; for (let i = 0; i < n; i++) { const result = stripVTControlCharacters(str); assert.ok(typeof result === 'string'); } bench.end(n); }
Citations:
Move assertion out of the timed loop to avoid skewed benchmark results.
The assertion at line 33 adds overhead to every measured iteration, so the benchmark no longer isolates stripVTControlCharacters performance.
⚙️ Proposed fix
- bench.start;
+ bench.start();
+ let result;
for (let i = 0; i < n; i++) {
- const result = stripVTControlCharacters(str);
- assert.ok(typeof result === 'string');
+ result = stripVTControlCharacters(str);
}
bench.end(n);
+ assert.strictEqual(typeof result, 'string');(Note: Also fix missing parentheses in bench.start() call.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmark/util/strip-vt-control-characters.js` around lines 30 - 34, The
benchmark currently measures extra overhead because assert.ok is executed inside
the timed loop and bench.start/stop are misused; move the assertion out of the
measured section by declaring let result; before the loop, call bench.start()
correctly, run for (let i = 0; i < n; i++) { result =
stripVTControlCharacters(str); } then call bench.stop() and finally call
assert.ok(typeof result === 'string'); ensure you reference the existing symbols
stripVTControlCharacters, bench.start(), bench.stop(), assert.ok and the result
variable so the assertion no longer skews timing.
Comment on lines
+18
to
+23
| start(controller) { | ||
| // Pre-fill the buffer | ||
| for (let i = 0; i < bufferSize; i++) { | ||
| controller.enqueue('a'); | ||
| enqueued++; | ||
| } |
There was a problem hiding this comment.
Add boundary check to prevent over-enqueueing in edge cases.
The start method enqueues bufferSize items without checking whether bufferSize > n. While the current benchmark configuration ensures bufferSize is always much smaller than n, this could cause the stream to enqueue more items than intended if the parameters are changed. The pull method correctly uses Math.min(bufferSize, n - enqueued) at line 27.
🛡️ Proposed fix to add consistent boundary checking
start(controller) {
// Pre-fill the buffer
- for (let i = 0; i < bufferSize; i++) {
+ const toEnqueue = Math.min(bufferSize, n);
+ for (let i = 0; i < toEnqueue; i++) {
controller.enqueue('a');
enqueued++;
}
},📝 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.
Suggested change
| start(controller) { | |
| // Pre-fill the buffer | |
| for (let i = 0; i < bufferSize; i++) { | |
| controller.enqueue('a'); | |
| enqueued++; | |
| } | |
| start(controller) { | |
| // Pre-fill the buffer | |
| const toEnqueue = Math.min(bufferSize, n); | |
| for (let i = 0; i < toEnqueue; i++) { | |
| controller.enqueue('a'); | |
| enqueued++; | |
| } | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmark/webstreams/readable-read-buffered.js` around lines 18 - 23, The
start(controller) implementation enqueues bufferSize items without checking
remaining quota, risking over-enqueueing when bufferSize > n; update start to
use the same boundary logic as pull by computing count = Math.min(bufferSize, n
- enqueued) (or equivalent) and only loop/enqueue that many times, referencing
the existing symbols start, controller, bufferSize, enqueued, and n to locate
and fix the logic.
Comment on lines
+108
to
+112
| parser.add_argument('--debug-symbols', | ||
| action='store_true', | ||
| dest='debug_symbols', | ||
| default=None, | ||
| help='add debugging symbols to release builds (adds -g without enabling DCHECKs)') |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In GYP/MSVS builds, does adding -gtotarget_defaults.cflags enable debug symbols for Windows/MSVC or clang-cl, and what is the correct equivalent setting if not?
💡 Result:
No, adding -g to target_defaults.cflags does not enable debug symbols for Windows/MSVC or clang-cl builds in GYP/MSVS generators. The cflags are Unix-style flags primarily used by make/ninja generators. For MSVS (Visual Studio) generator, cflags are typically ignored or not translated to MSVC-specific flags like /Zi, /Z7, or /DEBUG for debug symbols. The correct equivalent is to use 'configurations' with 'msvs_settings': 'configurations': { 'Debug': { 'msvs_settings': { 'VCCLCompilerTool': { 'DebugInformationFormat': '3', # /Zi (Program Database) }, 'VCLinkerTool': { 'GenerateDebugInformation': 'true', } } } } For newer MSBuild (VS2010+), it maps to ClCompile/DebugInformationFormat = 'ProgramDatabase [/Zi]' and Link/GenerateDebugInformation = 'true [/DEBUG]'. This is evident from GYP's MSVS generator code which processes msvs_settings into VCProj/VCXProj tool settings for ClCompile/Link, including DebugInformationFormat (values like 1=Disabled, 3=Zi, 4=Z7), while cflags! are used to remove flags but generic cflags like -g are not specially handled for MSVS. For clang-cl, since it emulates MSVC driver, it recognizes MSVC flags (/Zi, /Z7) rather than -g, so same msvs_settings apply when using MSVS generator. Sources confirm MSVSSettings.py validates settings like DebugInformationFormat/BasicRuntimeChecks for VCCLCompilerTool/ClCompile, and generator/msvs.py applies msvs_settings to tools.
Citations:
- 1: https://chromium.googlesource.com/external/gyp/+/md-pages/docs/LanguageSpecification.md
- 2: https://gyp3.org/docs/UserDocumentation.html
- 3: https://github.com/nodejs/node-gyp/blob/main/gyp/pylib/gyp/generator/msvs.py
- 4: https://github.com/nodejs/node-gyp/blob/085b445d/gyp/pylib/gyp/generator/msvs.py
- 5: https://android.googlesource.com/platform/external/v8/+/30d0143/tools/gyp/pylib/gyp/generator/msvs.py
- 6: https://chromium.googlesource.com/external/gyp.git/+/master/pylib/gyp/generator/msvs.py
- 7: https://github.com/nodejs/node-gyp/blob/main/gyp/pylib/gyp/MSVSSettings.py
- 8: https://github.com/nodejs/node-gyp/blob/085b445d/gyp/pylib/gyp/MSVSSettings.py
- 9: https://github.com/nodejs/node-gyp/blob/main/gyp/pylib/gyp/MSVSSettings_test.py
- 10: https://chromium.googlesource.com/experimental/external/gyp/+/refs/heads/master/pylib/gyp/MSVSSettings_test.py
🏁 Script executed:
# First, let's examine the configure.py file around the lines mentioned in the review
cd /tmp && find . -name "configure.py" -type f 2>/dev/null | head -5Repository: nodesource/nsolid
Length of output: 1885
🏁 Script executed:
# List files to understand repo structure
git ls-files | grep -E "configure\.(py|js)" | head -10Repository: nodesource/nsolid
Length of output: 1885
🏁 Script executed:
# Check the actual configure.py implementation at the relevant lines
head -1580 configure.py | tail -15Repository: nodesource/nsolid
Length of output: 1885
🏁 Script executed:
# Search for how --debug-symbols is used after line 1578
sed -n '1570,1600p' configure.pyRepository: nodesource/nsolid
Length of output: 1885
🏁 Script executed:
# Search for Windows/MSVS specific debug symbol handling
rg -n "debug.*symbol|msvs.*debug|cflags.*debug" configure.py -A 2 -B 2Repository: nodesource/nsolid
Length of output: 1885
Add MSVC-specific debug symbol flags for Windows builds.
Line 1578 injects -g via cflags, which is a Unix/GNU-Clang flag. The GYP MSVS generator does not translate -g to MSVC debug info flags (/Zi, /Z7, /DEBUG). For Windows/MSVC and clang-cl builds to generate debug symbols, the code must use msvs_settings with VCCLCompilerTool/ClCompile configuration (DebugInformationFormat) and linker settings (GenerateDebugInformation). The current implementation will only enable debug symbols on Unix-like toolchains (make/ninja with GCC/Clang), not on Windows.
Affects lines: 1576–1578
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@configure.py` around lines 108 - 112, The --debug-symbols parser flag
currently only adds a Unix -g cflag, so update the GYP generation to also set
MSVC-specific debug settings when debug_symbols is true: instead of relying
solely on cflags, add msvs_settings entries for the compile and link steps
(e.g., set VCCLCompilerTool/ClCompile DebugInformationFormat to an appropriate
value like "Zi" or "Z7" and set the linker
GenerateDebugInformation/LinkerSetting to true) and ensure clang-cl builds map
to these MSVC settings as well; locate where debug_symbols is checked and -g is
injected (the code that appends to cflags) and augment it to emit the
msvs_settings blocks for Windows/MSVC toolchains.
Comment on lines
+3558
to
+3567
| // Field element byte length: ceil(degree_bits / 8). | ||
| size_t field_len = (EC_GROUP_get_degree(group) + 7) / 8; | ||
| // Build an uncompressed point: 0x04 || x || y, each padded to field_len. | ||
| size_t uncompressed_len = 1 + 2 * field_len; | ||
| auto buf = DataPointer::Alloc(uncompressed_len); | ||
| if (!buf) return false; | ||
| unsigned char* ptr = static_cast<unsigned char*>(buf.get()); | ||
| ptr[0] = POINT_CONVERSION_UNCOMPRESSED; | ||
| x.encodePaddedInto(ptr + 1, field_len); | ||
| y.encodePaddedInto(ptr + 1 + field_len, field_len); |
There was a problem hiding this comment.
Check both coordinate writes before parsing the uncompressed point.
Line 3566 and Line 3567 ignore the result of encodePaddedInto(). If either coordinate is null or wider than field_len, this buffer stays zero-filled and the new fast path can feed substituted coordinates into EC_POINT_oct2point() instead of failing. The old affine-coordinate path rejected that input; this branch should do the same.
Proposed fix
// Field element byte length: ceil(degree_bits / 8).
size_t field_len = (EC_GROUP_get_degree(group) + 7) / 8;
// Build an uncompressed point: 0x04 || x || y, each padded to field_len.
size_t uncompressed_len = 1 + 2 * field_len;
auto buf = DataPointer::Alloc(uncompressed_len);
if (!buf) return false;
unsigned char* ptr = static_cast<unsigned char*>(buf.get());
ptr[0] = POINT_CONVERSION_UNCOMPRESSED;
- x.encodePaddedInto(ptr + 1, field_len);
- y.encodePaddedInto(ptr + 1 + field_len, field_len);
+ if (x.encodePaddedInto(ptr + 1, field_len) != field_len ||
+ y.encodePaddedInto(ptr + 1 + field_len, field_len) != field_len) {
+ return false;
+ }📝 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.
Suggested change
| // Field element byte length: ceil(degree_bits / 8). | |
| size_t field_len = (EC_GROUP_get_degree(group) + 7) / 8; | |
| // Build an uncompressed point: 0x04 || x || y, each padded to field_len. | |
| size_t uncompressed_len = 1 + 2 * field_len; | |
| auto buf = DataPointer::Alloc(uncompressed_len); | |
| if (!buf) return false; | |
| unsigned char* ptr = static_cast<unsigned char*>(buf.get()); | |
| ptr[0] = POINT_CONVERSION_UNCOMPRESSED; | |
| x.encodePaddedInto(ptr + 1, field_len); | |
| y.encodePaddedInto(ptr + 1 + field_len, field_len); | |
| // Field element byte length: ceil(degree_bits / 8). | |
| size_t field_len = (EC_GROUP_get_degree(group) + 7) / 8; | |
| // Build an uncompressed point: 0x04 || x || y, each padded to field_len. | |
| size_t uncompressed_len = 1 + 2 * field_len; | |
| auto buf = DataPointer::Alloc(uncompressed_len); | |
| if (!buf) return false; | |
| unsigned char* ptr = static_cast<unsigned char*>(buf.get()); | |
| ptr[0] = POINT_CONVERSION_UNCOMPRESSED; | |
| if (x.encodePaddedInto(ptr + 1, field_len) != field_len || | |
| y.encodePaddedInto(ptr + 1 + field_len, field_len) != field_len) { | |
| return false; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deps/ncrypto/ncrypto.cc` around lines 3558 - 3567, The uncompressed-point
build currently ignores the return values of x.encodePaddedInto(...) and
y.encodePaddedInto(...), which can leave the buffer zero-filled and allow
invalid coordinates to be fed into EC_POINT_oct2point(); modify the code around
DataPointer::Alloc/uncompressed_len/ptr and the two calls to encodePaddedInto so
you check each call's boolean/result and if either encodePaddedInto fails return
false (or otherwise abort) before calling EC_POINT_oct2point(); ensure you
reference the same symbols (ptr, field_len, encodePaddedInto,
EC_POINT_oct2point) so the function rejects bad/null/oversized coordinates the
same way as the affine path.
Comment on lines
+63
to
+67
| const result = { invalid, missing } | ||
| if (this.npm.config.get('include-attestations')) { | ||
| result.verified = this.verified | ||
| } | ||
| output.buffer(result) |
There was a problem hiding this comment.
Sort verified before emitting JSON to keep output deterministic.
this.verified is appended during concurrent pMap execution, so result order can vary run-to-run. Stable ordering helps CI automation and snapshot consumers.
Suggested fix
if (this.npm.config.get('json')) {
const result = { invalid, missing }
if (this.npm.config.get('include-attestations')) {
- result.verified = this.verified
+ result.verified = this.verified.slice().sort(sortAlphabetically)
}
output.buffer(result)
return
}Also applies to: 359-366
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deps/npm/lib/utils/verify-signatures.js` around lines 63 - 67, Before
emitting the JSON, ensure the attestations array is sorted so output is
deterministic: when building result with result.verified = this.verified
(guarded by this.npm.config.get('include-attestations')), sort this.verified
into a stable, well-defined order (e.g., by a unique key or string compare)
before calling output.buffer(result); apply the same sorting change to the other
occurrence that assigns result.verified around the later block (the one at the
other location referenced in the review) so both outputs are deterministic.
santigimeno
force-pushed
the
node-v24.15.0-nsolid-v6.2.3-release
branch
4 times, most recently
from
c207cdb to
c86d1e3
Compare
2026-04-15 Node.js v24.15.0 Krypton (LTS) Release Git-EVTag-v0-SHA512: 4bfd4c83c5d14860c3819f68c27d10fea3c50f2a2ac42b588b85a458c82c066fd108cc1ca50aa313cb4e6cb3c3e77792109c551e20e4329a517a7cc914599f0e
PR-URL: nodejs/node#62898 Reviewed-By: Jordan Harband <ljharb@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> PR-URL: #456 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
santigimeno
force-pushed
the
node-v24.15.0-nsolid-v6.2.3-release
branch
from
c86d1e3 to
a1e0379
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
configure.py (1)
1576-1578:⚠️ Potential issue | 🟠 Major
--debug-symbolscurrently misses Windows/MSVS debug symbol generation.Line 1578 only appends
-gtocflags. For MSVS/clang-cl project generation, this does not provide the equivalent/Zi+/DEBUGsettings, so the flag is ineffective on Windows MSVS builds.💡 Proposed fix
o['variables']['debug_symbols'] = b(options.debug_symbols) if options.debug_symbols: - o['cflags'] += ['-g'] + if flavor == 'win': + msvs_settings = o.setdefault('msvs_settings', {}) + cxx_tool = msvs_settings.setdefault('VCCLCompilerTool', {}) + linker_tool = msvs_settings.setdefault('VCLinkerTool', {}) + # /Zi + cxx_tool['DebugInformationFormat'] = '3' + # /DEBUG + linker_tool['GenerateDebugInformation'] = 'true' + else: + o['cflags'] += ['-g']Use this read-only verification script to confirm the gap:
#!/bin/bash set -euo pipefail # Locate where --debug-symbols is parsed/applied. rg -n -C3 --type=py -- '--debug-symbols|debug_symbols|\\-g' configure.py # Check whether MSVC debug-info settings exist near this flow. rg -n -C3 --type=py 'DebugInformationFormat|GenerateDebugInformation|VCCLCompilerTool|VCLinkerTool' configure.py # Cross-check whether these settings are configured elsewhere in repo. rg -n -C2 'DebugInformationFormat|GenerateDebugInformation'Expected result:
configure.pyshows-ginjection fordebug_symbols, but no correspondingDebugInformationFormat/GenerateDebugInformationwiring in that code path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configure.py` around lines 1576 - 1578, The patch only appends "-g" to o['cflags'] when options.debug_symbols is set, which doesn't enable MSVC/clang-cl debug info; update the branch that handles options.debug_symbols (check symbols o['variables']['debug_symbols'], options.debug_symbols, and o['cflags']) to detect MSVC/clang-cl toolchains and add the MSVC equivalents: append "/Zi" to the MSVC/clang-cl compiler flags (e.g., o['cflags'] or the MSVC-specific flag list) and ensure the linker receives "/DEBUG" (e.g., add to o['ldflags'] or the MSVC linker flag list), leaving "-g" for GCC/Clang; use the existing compiler/host detection logic in the file to choose which flags to add so Windows MSVC project generation gets "/Zi" + "/DEBUG".
🧹 Nitpick comments (1)
Makefile (1)
342-361: checkmakemaxbodylengthwarnings: confirm CI policy; consider delegating to*-impltargets.Static analysis flags
testandtest-onlyfor exceeding checkmake’s allowed recipe line counts. If CI treats this as a failure, we should reduce thetest/test-onlyrecipe bodies (e.g., move the existing commands intotest-implandtest-only-impl, then maketest/test-onlyinvoke those with a single$(MAKE)line).Optional refactor sketch
+.PHONY: test-impl +test-impl: + $(MAKE) -s tooltest + $(MAKE) -s test-doc + $(MAKE) -s build-addons + $(MAKE) -s build-js-native-api-tests + $(MAKE) -s build-node-api-tests + $(MAKE) -s build-sqlite-tests + $(MAKE) -s cctest + $(MAKE) -s jstest + test: all ## Run default tests and build docs. - $(MAKE) -s tooltest - $(MAKE) -s test-doc - $(MAKE) -s build-addons - $(MAKE) -s build-js-native-api-tests - $(MAKE) -s build-node-api-tests - $(MAKE) -s build-sqlite-tests - $(MAKE) -s cctest - $(MAKE) -s jstest + $(MAKE) -s test-impl + +.PHONY: test-only-impl +test-only-impl: + $(MAKE) build-addons + $(MAKE) build-js-native-api-tests + $(MAKE) build-node-api-tests + $(MAKE) build-sqlite-tests + $(MAKE) cctest + $(MAKE) jstest + $(MAKE) tooltest test-only: all ## Run default tests without building the docs. - $(MAKE) build-addons - $(MAKE) build-js-native-api-tests - $(MAKE) build-node-api-tests - $(MAKE) build-sqlite-tests - $(MAKE) cctest - $(MAKE) jstest - $(MAKE) tooltest + $(MAKE) -s test-only-implPlease confirm whether your CI currently fails on checkmake
maxbodylengthwarnings for Makefile targets (vs only reporting them). If it fails, the delegation refactor above should be prioritized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 342 - 361, Reduce the recipe body for the Makefile targets flagged by checkmake (test and test-only) by moving their multiple command lines into new helper targets (e.g., test-impl and test-only-impl) and update test and test-only to delegate with a single invocation like "$(MAKE) -s test-impl" or "$(MAKE) test-only-impl"; ensure the helper targets contain the original sequence of sub-target calls (tooltest, test-doc, build-addons, build-js-native-api-tests, build-node-api-tests, build-sqlite-tests, cctest, jstest) and keep the PHONY declarations for the new impl targets to match existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@configure.py`:
- Line 1823: The list comprehension that appends libraries uses an ambiguous
loop variable `l`; update the comprehension that modifies output['libraries']
(the expression using default_libs.split(',')) to use a descriptive name such as
`lib` or `library` instead of `l` (e.g., replace `f'-l{l}' for l in
default_libs.split(',')` with `f'-l{lib}' for lib in default_libs.split(',')`)
so Ruff E741 is resolved while preserving the same behavior.
---
Duplicate comments:
In `@configure.py`:
- Around line 1576-1578: The patch only appends "-g" to o['cflags'] when
options.debug_symbols is set, which doesn't enable MSVC/clang-cl debug info;
update the branch that handles options.debug_symbols (check symbols
o['variables']['debug_symbols'], options.debug_symbols, and o['cflags']) to
detect MSVC/clang-cl toolchains and add the MSVC equivalents: append "/Zi" to
the MSVC/clang-cl compiler flags (e.g., o['cflags'] or the MSVC-specific flag
list) and ensure the linker receives "/DEBUG" (e.g., add to o['ldflags'] or the
MSVC linker flag list), leaving "-g" for GCC/Clang; use the existing
compiler/host detection logic in the file to choose which flags to add so
Windows MSVC project generation gets "/Zi" + "/DEBUG".
---
Nitpick comments:
In `@Makefile`:
- Around line 342-361: Reduce the recipe body for the Makefile targets flagged
by checkmake (test and test-only) by moving their multiple command lines into
new helper targets (e.g., test-impl and test-only-impl) and update test and
test-only to delegate with a single invocation like "$(MAKE) -s test-impl" or
"$(MAKE) test-only-impl"; ensure the helper targets contain the original
sequence of sub-target calls (tooltest, test-doc, build-addons,
build-js-native-api-tests, build-node-api-tests, build-sqlite-tests, cctest,
jstest) and keep the PHONY declarations for the new impl targets to match
existing behavior.
🪄 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: d293ac7c-c589-4fe9-a01b-77b0064ca57c
📒 Files selected for processing (7)
BUILDING.mdMakefileagents/statsd/src/binding.ccagents/zmq/src/binding.ccagents/zmq/src/http_client.hcommon.gypiconfigure.py
✅ Files skipped from review due to trivial changes (4)
- common.gypi
- agents/zmq/src/binding.cc
- agents/zmq/src/http_client.h
- agents/statsd/src/binding.cc
🚧 Files skipped from review as they are similar to previous changes (1)
- BUILDING.md
|
|
||
| if default_libs: | ||
| output['libraries'] += default_libs | ||
| output['libraries'] += [f'-l{l}' for l in default_libs.split(',')] |
There was a problem hiding this comment.
Rename the loop variable to resolve Ruff E741.
Line 1823 uses l, which is flagged as ambiguous (E741). Rename to a descriptive identifier.
✏️ Proposed fix
- output['libraries'] += [f'-l{l}' for l in default_libs.split(',')]
+ output['libraries'] += [f'-l{lib_name}' for lib_name in default_libs.split(',')]📝 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.
Suggested change
| output['libraries'] += [f'-l{l}' for l in default_libs.split(',')] | |
| output['libraries'] += [f'-l{lib_name}' for lib_name in default_libs.split(',')] |
🧰 Tools
🪛 Ruff (0.15.12)
[error] 1823-1823: Ambiguous variable name: l
(E741)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@configure.py` at line 1823, The list comprehension that appends libraries
uses an ambiguous loop variable `l`; update the comprehension that modifies
output['libraries'] (the expression using default_libs.split(',')) to use a
descriptive name such as `lib` or `library` instead of `l` (e.g., replace
`f'-l{l}' for l in default_libs.split(',')` with `f'-l{lib}' for lib in
default_libs.split(',')`) so Ruff E741 is resolved while preserving the same
behavior.
a1e0379 to
e8187ca
Compare
EHortua
approved these changes
Apr 29, 2026
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
20 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
New Features
--debug-symbolsconfigure optionBug Fixes
Documentation
Chores