Node v20.19.6 nsolid v6.1.0 release#392
Conversation
PR-URL: nodejs/node#60419 Refs: nodejs/node#60050 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Richard Lau <richard.lau@ibm.com> Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Original commit message:
remove ppc and s390 from the host_arch list
We only support ppc64 and s390x.
Same changes is applied to depot_tools:
http://crrev.com/c/7100932
Change-Id: I1e4b660a1e46d1c609189301e8fd977fb7434748
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7102361
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Milad Farazmand <mfarazma@ibm.com>
Reviewed-by: Thibaud Michaud <thibaudm@chromium.org>
Cr-Commit-Position: refs/heads/main@{#103488}
Refs: v8/v8@2e4c5cf
Fixes: nodejs/build#4184
Co-authored-by: Michaël Zasso <targos@protonmail.com>
PR-URL: nodejs/node#60654
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <richard.lau@ibm.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Stewart X Addison <sxa@redhat.com>
Update `tools/make-v8.sh` so that it can work with either `gcc` or `clang`. Adds: - clang settings when CC/CXX environment variables set to clang/clang++. - Turns off warnings as errors. Removes: - goma settings. - Machine specific settings (moved to Jenkins job configuration). Refs: https://chromium-review.googlesource.com/c/v8/v8/+/5541431 PR-URL: nodejs/node#59893 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Origin commit message:
[loong64][codegen] Avoid memory access under stack pointer
According to LoongArch ABI doc chapter 8.2:
"Procedures must not assume the persistence on-stack data of which
the addresses lie below the stack pointer."
Change-Id: I92735e052227495ac9884c4290b57eaffbf905e1
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6786372
Auto-Submit: Zhao Jiazhong <zhaojiazhong-hf@loongson.cn>
Reviewed-by: Matthias Liedtke <mliedtke@chromium.org>
Commit-Queue: Matthias Liedtke <mliedtke@chromium.org>
Cr-Commit-Position: refs/heads/main@{#101634}
Backport-PR-URL: nodejs/node#59662
Refs: v8/v8@6b1b9bc
PR-URL: nodejs/node#59283
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Refs: actions/runner-images#13046 PR-URL: nodejs/node#60679 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
PR-URL: nodejs/node#58175 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs/node#58291 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Dario Piotrowicz <dario.piotrowicz@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com> Reviewed-By: Tierney Cyren <hello@bnb.im>
Signed-off-by: Matteo Collina <hello@matteocollina.com> Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: nodejs/node#58313 Backport-PR-URL: nodejs/node#58542 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Tim Perry <pimterry@gmail.com>
Since priority signaling covers more than `http2Stream.priority`, it makes more sense to rename the section. There were also a few missing mentions in `http2.md`. PR-URL: nodejs/node#58504 Backport-PR-URL: nodejs/node#58542 Refs: nodejs/node#58313 Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs/node#59113 Fixes: nodejs/node#58904 Reviewed-By: Richard Lau <richard.lau@ibm.com> Reviewed-By: Ruy Adorno <ruy@vlt.sh> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
PR-URL: nodejs/node#59133 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
PR-URL: nodejs/node#59134 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs/node#59134 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Windows recognizes the 8-bit [ISO/IEC 8859-1](https://en.wikipedia.org/wiki/ISO/IEC_8859-1) superscript digits ¹, ², and ³ as digits and treats them as valid parts of COM# and LPT# device names, making them reserved in every directory. Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com> PR-URL: nodejs/node#59261 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This is the certdata.txt[0] from NSS 3.114. This is the version of NSS that shipped in Firefox 142.0 on 2025-08-19. Certificates added: - TrustAsia TLS ECC Root CA - TrustAsia TLS RSA Root CA - SwissSign RSA TLS Root CA 2022 - 1 Certificates removed: - GlobalSign Root CA - Entrust.net Premium 2048 Secure Server CA - Baltimore CyberTrust Root - Comodo AAA Services root - XRamp Global CA Root - Go Daddy Class 2 CA - Starfield Class 2 CA [0] https://raw.githubusercontent.com/nss-dev/nss/refs/tags/NSS_3_114_RTM/lib/ckfw/builtins/certdata.txt PR-URL: nodejs/node#59571 Reviewed-By: Richard Lau <richard.lau@ibm.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
This is the certdata.txt[0] from NSS 3.116. This is the version of NSS that shipped in Firefox 144.0 on 2025-10-14. [0] https://raw.githubusercontent.com/nss-dev/nss/refs/tags/NSS_3_116_RTM/lib/ckfw/builtins/certdata.txt PR-URL: nodejs/node#59956 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Richard Lau <richard.lau@ibm.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: nodejs/node#60135 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs/node#60314 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: nodejs/node#60366 Refs: nodejs/nodejs.org#8231 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Claudio Wunder <cwunder@gnome.org> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Original commit message:
[py3.12] Optimize some py files
1. Remove the imp module and use its equivalent instead
2. Delete unused module imports
Bug: chromium:1487454
Change-Id: I06fd342ba8e17f96ee2e55926cca0efded845b2a
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4983809
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Ho Cheung <uioptt24@gmail.com>
Cr-Commit-Position: refs/heads/main@{#90641}
Refs: v8/v8@a0d0d4f
PR-URL: nodejs/node#60716
Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: nodejs/node#58840 Reviewed-By: Pietro Marchini <pietro.marchini94@gmail.com> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Links explaining the various target file-related `run([options])` pointed to the wrong section. PR-URL: nodejs/node#58876 Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
The `isSkipped` function in the JUnit reporter was incorrectly checking for `node?.attrs.failures` instead of `node?.attrs.skipped`. PR-URL: nodejs/node#59414 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Pietro Marchini <pietro.marchini94@gmail.com>
PR-URL: nodejs/node#59330 Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com> Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
PR-URL: nodejs/node#59434 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
PR-URL: nodejs/node#59049 Reviewed-By: theanarkh <theratliter@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
PR-URL: nodejs/node#59445 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Ruy Adorno <ruy@vlt.sh> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Fixes: nodejs/node#58331 PR-URL: nodejs/node#58646 Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
`ASYNC_THROW_IF_INSUFFICIENT_PERMISSIONS` may dereference `req_wrap_async`, so `CHECK_NOT_NULL(req_wrap_async)` should be used before and not after. PR-URL: nodejs/node#59487 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Richard Lau <richard.lau@ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
PR-URL: nodejs/node#59791 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: nodejs/node#60112 Reviewed-By: Richard Lau <richard.lau@ibm.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
PR-URL: nodejs/node#59607 Backport-PR-URL: nodejs/node#60066 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Pasting input should not trigger any completions and other calculations. This is now handled by just writing the string to the terminal in case the user is pasting. As soon as pasting is done, the completions will be re-enabled. Fixes: nodejs/node#40626 Fixes: nodejs/node#43343 PR-URL: nodejs/node#59857 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs/node#59806 Refs: openjs-foundation/cross-project-council#1588 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Richard Lau <richard.lau@ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs/node#59932 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Bumps [actions/setup-python](https://github.com/actions/setup-python) from 5.6.0 to 6.0.0. - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@a26af69...e797f83) --- updated-dependencies: - dependency-name: actions/setup-python dependency-version: 6.0.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> PR-URL: nodejs/node#60090 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Bumps [ossf/scorecard-action](https://github.com/ossf/scorecard-action) from 2.4.2 to 2.4.3. - [Release notes](https://github.com/ossf/scorecard-action/releases) - [Changelog](https://github.com/ossf/scorecard-action/blob/main/RELEASE.md) - [Commits](ossf/scorecard-action@05b42c6...4eaacf0) --- updated-dependencies: - dependency-name: ossf/scorecard-action dependency-version: 2.4.3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> PR-URL: nodejs/node#60096 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Bumps [step-security/harden-runner](https://github.com/step-security/harden-runner) from 2.12.2 to 2.13.1. - [Release notes](https://github.com/step-security/harden-runner/releases) - [Commits](step-security/harden-runner@6c439dc...f4a75cf) --- updated-dependencies: - dependency-name: step-security/harden-runner dependency-version: 2.13.1 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> PR-URL: nodejs/node#60094 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Bumps [actions/setup-node](https://github.com/actions/setup-node) from 4.4.0 to 5.0.0. - [Release notes](https://github.com/actions/setup-node/releases) - [Commits](actions/setup-node@49933ea...a0853c2) --- updated-dependencies: - dependency-name: actions/setup-node dependency-version: 5.0.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> PR-URL: nodejs/node#60093 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
I won't have enough time to contribute to Node.js to stay as a TSC member for a while so moving myself to emeritus. Signed-off-by: Michael Dawson <midawson@redhat.com> PR-URL: nodejs/node#60070 Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jordan Harband <ljharb@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <richard.lau@ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Abdirahim Musse <abdirahim.musse@ibm.com>
This note on `options.stdio` mentions that it's not recommended to pass `stdin` as a writable stream and `stdout/stderr` as readable, but then proceeds to say to always check that `stdin` is writable and `stdout/stderr` are readable. Going by the examples and the fact that `process.stdin` is readable and `process.stdout/process.stderr` are writable, I'm assuming that the types in the recommendation got swapped. PR-URL: nodejs/node#60114 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs/node#60068 Refs: nodejs/node#59553 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net>
"bechmark" -> "benchmark" in test-benchmark-readline.js. And fix test name in test-benchmark-validators.js. PR-URL: nodejs/node#59993 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
Fixes: nodejs/node#60034 PR-URL: nodejs/node#60103 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Notable changes: crypto: * update root certificates to NSS 3.114 (Node.js GitHub Bot) nodejs/node#59571 doc: * update the instruction on how to verify releases (Antoine du Hamel) nodejs/node#59113 * deprecate HTTP/2 priority signaling (Matteo Collina) nodejs/node#58313 PR-URL: nodejs/node#60784
Add comprehensive runtime control for asset collection (CPU/heap
profiling, snapshots) with multiple configuration methods.
Implements assetsEnabled flag with full control surface:
- NSOLID_ASSETS_ENABLED environment variable
- nsolid.start({ assetsEnabled: true/false }) configuration
- Runtime helpers: enableAssets() and disableAssets()
- Dynamic updates via gRPC reconfigure protocol
- Validation in both gRPC and ZMQ agents with proper errors
- Continuous profiler integration respects assetsEnabled state
This allows operators to disable expensive profiling operations
at runtime without process restarts, improving resource management
and production safety.
Also adds missing enableTraces()/disableTraces() helper methods
to complete the tracing API surface.
Comprehensive test coverage added for all asset types across gRPC,
ZMQ, and parallel test suites.
PR-URL: #377
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
2025-11-25 Node.js v20.19.6 Iron (LTS) Release Git-EVTag-v0-SHA512: fefc5febb567e32aea2cf55e73c615bf183ff679732654aef1a910d9a96237e326fd238754606dcbae1ddbe4a75bb89489b87bebfea8dd2bfa75bea22c7989f3
The previous logic used truthy checks which incorrectly returned null when a config value was explicitly set to false (e.g., assetsEnabled). Use nullish coalescing to only return null for undefined/null values.
WalkthroughComprehensive rebranding of Node.js to N|Solid across configuration, documentation, and build systems. Introduces new gRPC-based agent system for profiling, metrics, and tracing. Updates GitHub workflows, ESLint configuration, and adds DevContainer support. Includes Windows DSC provisioning configurations and extensive license/contributor metadata updates. Changes
Sequence DiagramsequenceDiagram
participant JS as JavaScript App
participant Binding as Native Binding<br/>(binding.cc)
participant Agent as GrpcAgent<br/>(Singleton)
participant Stream as Command/Asset<br/>Stream Reactors
participant Client as gRPC Client<br/>(NSolidService Stub)
participant Server as NSolid gRPC<br/>Server
JS->>Binding: start()
Binding->>Agent: Inst()->start()
Agent->>Client: MakeChannel() + MakeStub()
Agent->>Stream: Initialize CommandStream
Stream->>Client: Command() bidirectional stream
Client->>Server: Establish command stream
JS->>Binding: snapshot() / profile()
Binding->>Agent: start_heap_snapshot_from_js(CommandRequest)
Agent->>Stream: Write CommandRequest to queue
Stream->>Client: Async write response
Client->>Server: Send profiling command
Server-->>Client: CommandRequest (e.g., start profile)
Client-->>Stream: OnReadDone(request)
Stream-->>Agent: on_command_received(request)
Agent->>Agent: Process command, start profiling
Agent->>Stream: Write AssetStream data
Stream->>Client: ExportAsset/ExportContinuousProfile RPC
Client->>Server: Stream asset chunks
Server-->>Client: EventResponse (ack/error)
Client-->>Stream: OnDone(status)
Stream-->>Agent: on_asset_stream_done(status)
Agent->>JS: Profile callback/completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
.github/workflows/comment-labeled.yml (1)
26-26: The repository checks will prevent this workflow from executing in the nsolid repository.This workflow contains hardcoded repository checks (
github.repository == 'nodejs/node') on lines 26, 38, and 49 that will cause all three jobs to be skipped when this workflow runs in thenodesource/nsolidrepository. Either update the repository name in the conditionals tonodesource/nsolid(or use a variable), or remove this file if it's only intended for upstream Node.js..github/workflows/find-inactive-collaborators.yml (1)
18-18: Critical: Repository condition prevents workflow execution in nsolid.Line 18 hardcodes a check for
github.repository == 'nodejs/node', but this workflow resides in thenodesource/nsolidrepository. The workflow will never execute because the condition will always be false.Update the repository check to match the nsolid repository:
- if: github.repository == 'nodejs/node' + if: github.repository == 'nodesource/nsolid'LICENSE (1)
631-645: Restore the missing URL in the config.guess license noticeThe config.guess license block ends with "if not, see ." but is missing the standard GPLv3 URL. The complete sentence should read:
You should have received a copy of the GNU General Public License along with this program. If not, see https://www.gnu.org/licenses/.
Makefile (1)
155-155: Symlink target mismatch in Debug build rule.Line 155 links to
out/Debug/$(NODE_EXE)but should link toout/Debug/$(NODE_G_EXE)since the target being created is$(NODE_G_EXE).- if [ ! -r $@ ] || [ ! -L $@ ]; then ln -fs out/Debug/$(NODE_EXE) $@; fi + if [ ! -r $@ ] || [ ! -L $@ ]; then ln -fs out/Debug/$(NODE_G_EXE) $@; fi
🟡 Minor comments (7)
.github/workflows/update-wpt.yml-3-7 (1)
3-7: Clarify WPT update schedule and repository guardTwo small configuration issues are worth addressing:
- The comment says “Run once a week at 12:00 AM UTC on Sunday.” but
cron: 0 0 * * *runs daily at 00:00 UTC. Either the comment or the cron expression is currently wrong.- The job condition
if: github.repository == 'nodejs/node' || github.event_name == 'workflow_dispatch'means scheduled runs (event_name == 'schedule') will never execute in this repository; only manualworkflow_dispatchinvocations will. That might be intentional, but it’s easy to miss.I’d recommend explicitly aligning the comment and schedule, and double‑checking whether the repo check should be updated to this repo’s name if you expect the scheduled updates to run here.
Also applies to: 20-27, 52-69, 70-82
GOVERNANCE.md-243-253 (1)
243-253: Remove duplicate section: "How to review a collaborator nomination".Lines 243–253 and lines 294–310 contain the same heading and very similar content. This duplication appears unintentional and should be consolidated to a single section or the duplicate removed entirely. If the two sections are meant to convey different guidance, rename one and add clarifying context.
Also applies to: 294-310
BUILDING.md-35-40 (1)
35-40: Fix broken Windows “Option 1” anchor in the table of contentsThe TOC links to
#option-1-manual-install, but the section header is now just##### Manual install, so the generated fragment will be#manual-install. Please update the link target to keep navigation working..github/workflows/lint-release-proposal.yml-28-42 (1)
28-42: Fix logging to use the actual values instead of undefined$ACTUALBoth steps echo
Actual: $ACTUALbut never setACTUAL, so the logs are blank:
- Title step: should log
$COMMIT_SUBJECT.- Trailer step: should log
$PR_URL.Consider updating to:
echo "Actual: $COMMIT_SUBJECT" # ... echo "Actual: $PR_URL"so failures are easier to diagnose.
CONTRIBUTING.md-247-258 (1)
247-258: Fix typo in email address.Line 256 has an extra closing angle bracket in the email address. This could cause issues with email parsing or markdown rendering.
* [trevnorris](https://github.com/trevnorris) - - **Trevor Norris** <<trev.norris@gmail.com>>> - (he/him) + **Trevor Norris** <<trev.norris@gmail.com>> (he/him)agents/grpc/src/grpc_client.h-44-46 (1)
44-46: Uninitialized member variablestartcould cause undefined behavior.The
startmember is not initialized in the default constructor or member initializer list. When debug logging is disabled (lines 115-118 not executed),startretains an indeterminate value. The subsequent checkcall_data->start > 0at line 129 could evaluate unpredictably.EventType* event = nullptr; grpcagent::EventResponse* event_response = nullptr; - uint64_t start; + uint64_t start = 0;agents/grpc/src/grpc_agent.cc-71-71 (1)
71-71: Incorrect comment: value is 4MB, not 4GB.The comment states "4GB" but the actual value
4L * 1024 * 1024equals 4,194,304 bytes (4MB). For 4GB, it should be4L * 1024 * 1024 * 1024.-const size_t GRPC_MAX_SIZE = 4L * 1024 * 1024; // 4GB +const size_t GRPC_MAX_SIZE = 4L * 1024 * 1024; // 4MB
🧹 Nitpick comments (33)
.github/workflows/post-release.yml (2)
21-25: Add error handling and logging for cross-repository workflow triggering.The first step triggers a workflow in an external repository without checking for success or handling failures. If the workflow trigger fails, the job will still report success, potentially masking issues.
Add error handling and provide clearer output:
- name: Trigger update-links workflow on nodejs/release-cloudflare-worker run: | - gh workflow run update-links.yml --repo nodejs/release-cloudflare-worker + gh workflow run update-links.yml --repo nodejs/release-cloudflare-worker || exit 1 + echo "Successfully triggered update-links workflow" env: GITHUB_TOKEN: ${{ secrets.GH_USER_TOKEN }}
27-32: Add error handling and verify VERSION environment variable fallback.Similar to the first step, this cross-repository workflow trigger lacks error handling. Additionally, verify that
github.event.release.tag_nameis always available when the workflow is triggered by a release event.Improve error handling and add explicit variable validation:
- name: Trigger create-release-post workflow on nodejs/nodejs.org run: | - gh workflow run create-release-post.yml --repo nodejs/nodejs.org -f version=$VERSION + if [ -z "$VERSION" ]; then + echo "Error: VERSION is not set" + exit 1 + fi + gh workflow run create-release-post.yml --repo nodejs/nodejs.org -f version=$VERSION || exit 1 + echo "Successfully triggered create-release-post workflow with version $VERSION" env: GITHUB_TOKEN: ${{ secrets.GH_USER_TOKEN }} VERSION: ${{ inputs.version || github.event.release.tag_name }}Verify that
github.event.release.tag_nameis always populated when the workflow is triggered via the release event type..github/workflows/tools.yml (2)
199-204: Fragile path navigation in lint-md update step.Line 202 uses
cd ../..to navigate fromtools/lint-mdback to repo root. This works but is fragile and depends on script execution context. Consider using explicit paths or${{ github.workspace }}for robustness.if [ "$NEW_VERSION" != "" ]; then echo "NEW_VERSION=$NEW_VERSION" >> $GITHUB_ENV rm -rf package-lock.json node_modules npm install --ignore-scripts $NEW_VERSION npm install --ignore-scripts - cd ../.. - make lint-md-rollup + cd "${{ github.workspace }}" + make lint-md-rollup fi
58-289: Consider consolidating repetitive matrix entry patterns.The matrix strategy contains extensive duplication: nearly all dependency entries follow the identical pattern (run script → redirect to temp-output → grep NEW_VERSION → cleanup). This creates ~2000 lines of highly redundant YAML. While functionally correct, this reduces maintainability and makes future updates error-prone.
Consider extracting the common script pattern into a reusable composite action or a shared shell script template. This would reduce the workflow file to ~200 lines and simplify adding new dependencies. Example structure:
# Pseudo-code for composite action approach - name: Update dependency ${{ matrix.id }} uses: ./.github/actions/update-dep with: id: ${{ matrix.id }} script-path: ./tools/dep_updaters/update-${{ matrix.id }}.shHowever, if the current verbosity is intentional for CI/CD clarity and change traceability, this refactor can be deferred.
.gitpod.yml (1)
2-5: Global install of@node-core/utilsin Gitpod init is reasonableAdding a dedicated init step to install
@node-core/utilsglobally makes sense and aligns dev with CI tooling.If you prefer a single task/terminal or stricter ordering, consider chaining the pnpm install into the existing init, e.g.:
-tasks: - - init: ./configure && timeout 50m make -j16 || true - - init: pnpm i -g @node-core/utils +tasks: + - init: pnpm i -g @node-core/utils && ./configure && timeout 50m make -j16 || trueAlso verify that
pnpmis available in your Gitpod base image; if not, you may need a brief install step before using it..github/SUPPORT.md (1)
3-5: Support text and links look good; optional wording tweakThe content correctly points users to N|Solid and NodeSource resources and clearly distinguishes community vs. professional support. One minor polish you might consider is tightening the phrasing on Line 5:
- N|Solid, which follows the same schedule from [Node.js](https://github.com/nodejs/Release#release-schedule). + N|Solid, which follows the same release schedule as [Node.js](https://github.com/nodejs/Release#release-schedule).Also applies to: 9-12, 16-18, 20-28
.eslintrc.js (1)
16-23: ESLint overrides and new.substrrestriction are sensible; watch for edge casesThe new overrides for
lib/internal/modules/**/*.js(curly: 'error') andlib/internal/test_runner/**/*.js(node-core/set-proto-to-null-in-object) strengthen consistency in important internals, and the addedno-restricted-syntaxselector discouraging.substr()is a good modernization push.One thing to keep in mind: the selector
{ selector: "CallExpression[callee.property.name='substr']", message: 'Use String.prototype.slice() or String.prototype.substring() instead of String.prototype.substr()', }will also flag any custom APIs that happen to expose a
substrmethod. If such cases exist in this repo, you may need targetedeslint-disable-next-lineexceptions or a slightly narrower selector later, but as a default it’s a reasonable trade‑off.Also applies to: 48-49, 117-132, 201-219
GOVERNANCE.md (1)
237-237: Fix grammar: use hyphen for compound adjective.Line 237 should read "consensus-seeking processes" (hyphenated) when used as a compound adjective before a noun.
.github/workflows/commit-queue.yml (1)
1-5: Update secrets documentation to match actual secrets usedThe header comment still mentions
GH_USER_NAME, but the workflow now usessecrets.JENKINS_USER(viaUSERNAME) alongsideGH_USER_TOKENandJENKINS_TOKEN. It would be clearer to update the comment so consumers know which secrets must actually exist (JENKINS_USER,GH_USER_TOKEN,JENKINS_TOKEN).CHANGELOG.md (1)
5-12: Changelog entries for 20.x/19.x alignment look correctThe LTS/EoL flags and the v20 release index (through 20.19.6) are consistent and follow the existing structure. If you care about wording polish, you could hyphenate “Long Term Support” → “Long‑Term Support”, but that’s purely stylistic.
BUILDING.md (2)
217-247: Align Python version requirements across sectionsHere you say N|Solid “supports Python >= 3.6 for building and testing”, but the Unix/macOS prerequisite bullets still read
Python >=3.6 <=3.11 (see note above). That’s contradictory now that the note no longer carries the upper bound. It would be good to:
- Either reintroduce the explicit upper limit in the main note, or
- Drop the
<=3.11constraint from the platform-specific bullets,so contributors get a single, clear statement of the supported Python range.
148-148: Minor wording/spelling clean‑upsTwo tiny text nits you may want to adjust:
- Line 148: insert a space →
Xcode 11instead ofXcode11.- Line 845: change “a ABI incompatible” to “an ABI‑incompatible”.
Purely cosmetic, but improves readability.
Also applies to: 845-845
Makefile (1)
313-319: jstest target body length exceeds Makefile linter threshold.The jstest target now has 6 lines in its recipe (exceeding checkmake's default max of 5). While functionally correct, consider extracting the core test logic into a helper to keep recipes concise, or suppress the warning if intentional.
.github/workflows/test-asan.yml (1)
33-33: Tool version inconsistencies in disabled ASAN workflow.The ASAN workflow uses older tool versions than other CI workflows:
- setup-python: v5.3.0 (other workflows use v6.0.0)
- sccache: v0.8.1 (other workflows use v0.10.0)
Since this workflow is disabled (
if: false), these don't currently impact CI. However, when re-enabled, ensure version consistency across workflows or document any ASAN-specific requirements.Also applies to: 44-60
README.md (1)
1-51: Branding updates are consistent and complete.The transition from Node.js to N|Solid branding is thorough across documentation references, support URLs, and license information. The structural reorganization aligns with the project's rebrand.
Minor suggestion: Line 48 could be more concise; "a variety of licenses" → "various licenses" (per static analysis).
-N|Solid is available under the -[MIT license](https://opensource.org/licenses/MIT). N|Solid also includes -external libraries that are available under a variety of licenses. See +N|Solid is available under the +[MIT license](https://opensource.org/licenses/MIT). N|Solid also includes +external libraries available under various licenses. SeeCONTRIBUTING.md (2)
92-99: Minor style: add hyphens for compound adjectives.Per static analysis, compound adjectives should be hyphenated for clarity.
-For the initial N|Solid open source release, this branch was `nsolid-v5.x`. Prior to releases, relevant changes will be ported to the other +For the initial N|Solid open-source release, this branch was `nsolid-v5.x`. Before releases, relevant changes will be ported to the other
290-294: Minor style: add hyphen and consolidate footnote formatting.The footnote uses "Pre v5.0.0" which should be hyphenated as "Pre-v5.0.0" per static analysis.
-[^*]: Significant breaking changes or the introduction of substantial features - will drive the MAJOR version increments in N|Solid. Pre v5.0.0 versions of - N|Solid are proprietary, and there will be no further enhancements to these +[^*]: Significant breaking changes or the introduction of substantial features + will drive the MAJOR version increments in N|Solid. Pre-v5.0.0 versions of + N|Solid are proprietary, and there will be no further enhancements to theseagents/grpc/proto/packages.proto (1)
1-28: Packages schema is solid; fix stale comment and confirmcommon.protoimportThe
PackagesEvent/PackagesBody/Packagemessages look appropriate for packaging data (clear field names, unique ids, sensible types). Two minor follow‑ups:
- The header comment still refers to a
RuntimeResponseoneof design that isn’t implemented here, which can be confusing. Consider updating or removing that comment to match the current schema.- Buf reports
import "common.proto": file does not exist, similar to the other new protos. Please confirm thatcommon.protois present and reachable on the Buf/protoc include path.agents/grpc/proto/reconfigure.proto (1)
7-21: Consider using snake_case for field names and updating terminology.Two observations:
Naming convention: Proto3 style guide recommends
snake_casefor field names (e.g.,blocked_loop_thresholdinstead ofblockedLoopThreshold). Other protos in this PR likeasset.protouse snake_case (thread_id,start_ts). Consider aligning for consistency.Terminology:
tracingModulesBlacklistuses outdated terminology. Considertracing_modules_blocklistortracing_modules_deny_list.agents/grpc/proto/exit.proto (1)
7-10: Consider renamingErrorto avoid potential naming conflicts.The name
Erroris very generic and may conflict with other protobuf definitions or cause ambiguity in generated code (especially in languages like JavaScript whereErroris a built-in type). Consider a more specific name likeExitErrororErrorDetails.-message Error { +message ExitError { string message = 1; string stack = 2; }agents/grpc/proto/common.proto (1)
5-8: Consider usinggoogle.protobuf.Timestampor adjusting types.The custom
Timemessage duplicatesgoogle.protobuf.Timestamp. A few observations:
uint64for seconds prevents representing dates before Unix epoch (1970), unlikegoogle.protobuf.Timestampwhich usesint64.- Nanoseconds range is 0-999,999,999, which fits in
uint32(max ~4.2 billion).If the intent is to only support post-epoch timestamps and you prefer a custom type, the current design works. Otherwise, consider using the well-known type for broader ecosystem compatibility.
agents/grpc/proto/command.proto (2)
1-5: Remove or update design notes before release.These comments appear to be design notes/TODOs from the implementation phase. Consider removing them or converting to proper documentation if they describe intended future work.
-// I want to define here RuntimeResponse message which will be oneOf many other types of messages -// For example: -// - InfoResponse -// - PackagesResponse - syntax = "proto3";
22-27: Consider documenting the distinction betweenrequestIdandid.Both
requestIdandidare string identifiers. A brief comment clarifying their purpose would help maintainers (e.g., isidthe agent/process ID whilerequestIdis the correlation ID for the specific request?).agents/grpc/proto/asset.proto (1)
14-16: Clarify timestamp representation vs.Timemessage in common.proto.The
duration,start_ts, andend_tsfields usedoublefor milliseconds from epoch, whilecommon.protodefines aTimemessage with seconds/nanoseconds. Consider either:
- Adding a comment explaining why doubles are preferred here (e.g., JavaScript interop, precision requirements).
- Reusing the
Timemessage for consistency.This is a minor point if the design is intentional.
agents/grpc/src/grpc_utils.h (1)
38-45: Fixed buffer size may truncate formatted output.The 512-byte buffer could truncate longer format strings with many arguments. For debug logging, this may be acceptable, but consider using dynamic formatting or at minimum checking for truncation:
if constexpr (sizeof...(args) > 0) { char buffer[512]; - snprintf(buffer, sizeof(buffer), format, std::forward<Args>(args)...); - formatted_prefix = buffer; + int written = snprintf(buffer, sizeof(buffer), format, std::forward<Args>(args)...); + if (written >= static_cast<int>(sizeof(buffer))) { + // Truncated - could log a warning or use larger buffer + } + formatted_prefix = buffer; } else {agents/grpc/proto/profile.proto (1)
5-5: Optional: Consider proto file organization.Buf reports a
PACKAGE_DIRECTORY_MATCHlint warning—files with packagegrpcagentshould ideally be in agrpcagent/directory. This is a convention issue rather than a compilation error. If your project uses Buf for linting, you may want to either reorganize the proto files or configure Buf to ignore this rule for this directory.agents/grpc/proto/blocked_loop.proto (1)
1-5: Remove or update stale design comments.These lines appear to be leftover design notes about a
RuntimeResponsemessage withoneOfthat was never implemented in this file. Consider removing them to avoid confusion, or update them if they're still relevant for future work.agents/grpc/src/asset_stream.cc (1)
80-95: Stream statistics only updated when debug logging is enabled.The
stream_stats_.write_countandstream_stats_.total_bytesare only incremented inside the debug-enabled block. If these stats are needed for purposes other than debug logging (e.g., metrics, monitoring), they should be updated unconditionally.If stats are needed outside of debug logging:
+ // Update stream statistics + stream_stats_.write_count++; + stream_stats_.total_bytes += write_state_.asset.data().length(); + // Calculate and log latency for this write only when debug is enabled if (per_process::enabled_debug_list.enabled( DebugCategory::NSOLID_GRPC_AGENT) && write_state_.write_start > 0) { uint64_t latency = uv_hrtime() - write_state_.write_start; Debug("[out] [%" PRIu64 "] %s command=%s requestId=%s data.length=%zu\n", latency, ok ? "ok" : "not ok", write_state_.asset.common().command().c_str(), write_state_.asset.common().requestid().c_str(), write_state_.asset.data().length()); write_state_.write_start = 0; - - // Update stream statistics - stream_stats_.write_count++; - stream_stats_.total_bytes += write_state_.asset.data().length(); }agents/grpc/src/command_stream.cc (1)
30-41: Destructor may block indefinitely if stream never completes.The destructor waits on
on_done_cond_untilwrite_state_.doneis true. If the gRPC channel is in a problematic state whereOnDoneis never called, this could cause the destructor to hang indefinitely.Consider adding a timeout to the condition wait, or documenting that callers must ensure the stream is properly terminated before destruction.
CommandStream::~CommandStream() { nsuv::ns_mutex::scoped_lock lock(lock_); // try cancel and wait until OnDone is called if (!write_state_.done) { context_.TryCancel(); + // Consider adding a bounded wait to avoid indefinite blocking do { uv_cond_wait(&on_done_cond_, lock_.base()); } while (!write_state_.done); } uv_cond_destroy(&on_done_cond_); }agents/grpc/src/asset_stream.h (1)
71-75: Redundantprivate:access specifier.There are two consecutive
private:labels which is redundant.private: void NextWrite(); - private: std::weak_ptr<AssetStreamObserver> observer_;agents/grpc/src/binding.cc (1)
68-68: Minor formatting inconsistency.There's an extra space before the comma in the
ASSERT_EQcall.- ASSERT_EQ(2 , args.Length()); + ASSERT_EQ(2, args.Length());agents/grpc/src/grpc_agent.cc (2)
145-176: Consider defensive field access for robustness.While the JSON is parsed with exception handling disabled and
ASSERTchecks for valid parse, the subsequent field accesses (lines 155-174) assume all required fields exist. Ifblocked_for,loop_id,callback_cntr, or any stack frame fields are missing,nlohmann::json::get<T>()will throw an exception.The code at lines 162-170 correctly uses
find()to check optional string fields, but mandatory fields are accessed directly.Consider adding similar defensive checks for required fields, or document that the caller guarantees well-formed JSON:
blocked_body->set_thread_id(stor.thread_id); - blocked_body->set_blocked_for(body["blocked_for"].get<int32_t>()); - blocked_body->set_loop_id(body["loop_id"].get<int32_t>()); - blocked_body->set_callback_cntr(body["callback_cntr"].get<int32_t>()); + if (auto it = body.find("blocked_for"); it != body.end() && it->is_number()) { + blocked_body->set_blocked_for(it->get<int32_t>()); + } + if (auto it = body.find("loop_id"); it != body.end() && it->is_number()) { + blocked_body->set_loop_id(it->get<int32_t>()); + } + if (auto it = body.find("callback_cntr"); it != body.end() && it->is_number()) { + blocked_body->set_callback_cntr(it->get<int32_t>()); + }
413-418: Redundant custom deleter in singleton.The custom deleter
[](GrpcAgent* agent) { delete agent; }is the default behavior forshared_ptr. This can be simplified./*static*/ SharedGrpcAgent GrpcAgent::Inst() { - static SharedGrpcAgent agent(new GrpcAgent(), [](GrpcAgent* agent) { - delete agent; - }); + static SharedGrpcAgent agent(new GrpcAgent()); return agent; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (109)
.configurations/configuration.dsc.yaml(1 hunks).configurations/configuration.vsEnterprise.dsc.yaml(1 hunks).configurations/configuration.vsProfessional.dsc.yaml(1 hunks).devcontainer.json(1 hunks).eslintignore(1 hunks).eslintrc.js(8 hunks).flake8(0 hunks).github/CODEOWNERS(5 hunks).github/ISSUE_TEMPLATE/1-bug-report.yml(0 hunks).github/ISSUE_TEMPLATE/2-feature-request.yml(0 hunks).github/ISSUE_TEMPLATE/3-api-ref-docs-problem.yml(0 hunks).github/ISSUE_TEMPLATE/4-report-a-flaky-test.yml(0 hunks).github/ISSUE_TEMPLATE/config.yml(0 hunks).github/PULL_REQUEST_TEMPLATE.md(1 hunks).github/SUPPORT.md(1 hunks).github/dependabot.yml(1 hunks).github/label-pr-config.yml(6 hunks).github/workflows/auto-start-ci.yml(3 hunks).github/workflows/build-tarball.yml(4 hunks).github/workflows/build-windows.yml(0 hunks).github/workflows/close-stale-feature-requests.yml(3 hunks).github/workflows/close-stale-pull-requests.yml(1 hunks).github/workflows/close-stalled.yml(1 hunks).github/workflows/comment-labeled.yml(1 hunks).github/workflows/commit-lint.yml(1 hunks).github/workflows/commit-queue.yml(4 hunks).github/workflows/coverage-linux-without-intl.yml(1 hunks).github/workflows/coverage-linux.yml(1 hunks).github/workflows/coverage-windows.yml(3 hunks).github/workflows/create-release-proposal.yml(1 hunks).github/workflows/daily-wpt-fyi.yml(6 hunks).github/workflows/daily.yml(1 hunks).github/workflows/doc.yml(2 hunks).github/workflows/find-inactive-collaborators.yml(2 hunks).github/workflows/find-inactive-tsc.yml(2 hunks).github/workflows/label-flaky-test-issue.yml(1 hunks).github/workflows/label-pr.yml(0 hunks).github/workflows/license-builder.yml(1 hunks).github/workflows/lint-release-proposal.yml(1 hunks).github/workflows/linters.yml(4 hunks).github/workflows/major-release.yml(1 hunks).github/workflows/notify-on-push.yml(3 hunks).github/workflows/notify-on-review-wanted.yml(1 hunks).github/workflows/post-release.yml(1 hunks).github/workflows/scorecard.yml(2 hunks).github/workflows/test-asan.yml(1 hunks).github/workflows/test-internet.yml(1 hunks).github/workflows/test-linux.yml(3 hunks).github/workflows/test-macos.yml(3 hunks).github/workflows/test-ubsan.yml(1 hunks).github/workflows/timezone-update.yml(4 hunks).github/workflows/tools.yml(3 hunks).github/workflows/update-openssl.yml(2 hunks).github/workflows/update-v8.yml(1 hunks).github/workflows/update-wpt.yml(1 hunks).gitignore(4 hunks).gitpod.yml(1 hunks).mailmap(10 hunks)BUILD.gn(1 hunks)BUILDING.md(28 hunks)CHANGELOG.md(2 hunks)CHANGELOG_NSOLID.md(1 hunks)CODE_OF_CONDUCT.md(1 hunks)CONTRIBUTING.md(2 hunks)GOVERNANCE.md(6 hunks)LICENSE(8 hunks)LICENSE_NSOLID(1 hunks)Makefile(38 hunks)README.md(1 hunks)SECURITY.md(2 hunks)agents/.eslintrc.yaml(1 hunks)agents/grpc/lib/agent.js(1 hunks)agents/grpc/lib/nsolid.js(1 hunks)agents/grpc/proto/BUILD(1 hunks)agents/grpc/proto/asset.proto(1 hunks)agents/grpc/proto/blocked_loop.proto(1 hunks)agents/grpc/proto/command.proto(1 hunks)agents/grpc/proto/common.proto(1 hunks)agents/grpc/proto/exit.proto(1 hunks)agents/grpc/proto/info.proto(1 hunks)agents/grpc/proto/metrics.proto(1 hunks)agents/grpc/proto/nsolid_service.proto(1 hunks)agents/grpc/proto/packages.proto(1 hunks)agents/grpc/proto/profile.proto(1 hunks)agents/grpc/proto/reconfigure.proto(1 hunks)agents/grpc/proto/source_code.proto(1 hunks)agents/grpc/proto/startup_times.proto(1 hunks)agents/grpc/src/asset_stream.cc(1 hunks)agents/grpc/src/asset_stream.h(1 hunks)agents/grpc/src/binding.cc(1 hunks)agents/grpc/src/command_stream.cc(1 hunks)agents/grpc/src/command_stream.h(1 hunks)agents/grpc/src/grpc_agent.cc(1 hunks)agents/grpc/src/grpc_agent.h(1 hunks)agents/grpc/src/grpc_client.cc(1 hunks)agents/grpc/src/grpc_client.h(1 hunks)agents/grpc/src/grpc_errors.h(1 hunks)agents/grpc/src/grpc_utils.h(1 hunks)agents/grpc/src/proto/asset.grpc.pb.cc(1 hunks)agents/grpc/src/proto/asset.grpc.pb.h(1 hunks)agents/grpc/src/proto/asset.pb.cc(1 hunks)agents/grpc/src/proto/asset.pb.h(1 hunks)agents/grpc/src/proto/blocked_loop.grpc.pb.cc(1 hunks)agents/grpc/src/proto/blocked_loop.grpc.pb.h(1 hunks)agents/grpc/src/proto/blocked_loop.pb.cc(1 hunks)agents/grpc/src/proto/blocked_loop.pb.h(1 hunks)agents/grpc/src/proto/command.grpc.pb.cc(1 hunks)agents/grpc/src/proto/command.grpc.pb.h(1 hunks)agents/grpc/src/proto/command.pb.cc(1 hunks)
💤 Files with no reviewable changes (8)
- .github/ISSUE_TEMPLATE/config.yml
- .flake8
- .github/workflows/build-windows.yml
- .github/workflows/label-pr.yml
- .github/ISSUE_TEMPLATE/2-feature-request.yml
- .github/ISSUE_TEMPLATE/4-report-a-flaky-test.yml
- .github/ISSUE_TEMPLATE/1-bug-report.yml
- .github/ISSUE_TEMPLATE/3-api-ref-docs-problem.yml
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 0
File: :0-0
Timestamp: 2025-10-27T13:06:17.506Z
Learning: In the nodesource/nsolid repository, when reviewing PRs that add or modify V8 patches (e.g., in deps/v8/patches/), focus review comments on the patch management infrastructure (configure.py, apply-patches.sh, README.md, etc.) and not on the V8 code changes contained within the .patch files themselves, as the patch content is out of scope for the patch management PR.
📚 Learning: 2025-10-27T13:06:17.506Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 0
File: :0-0
Timestamp: 2025-10-27T13:06:17.506Z
Learning: In the nodesource/nsolid repository, when reviewing PRs that add or modify V8 patches (e.g., in deps/v8/patches/), focus review comments on the patch management infrastructure (configure.py, apply-patches.sh, README.md, etc.) and not on the V8 code changes contained within the .patch files themselves, as the patch content is out of scope for the patch management PR.
Applied to files:
.github/PULL_REQUEST_TEMPLATE.md.github/workflows/update-v8.yml.github/workflows/commit-queue.ymlREADME.md.github/CODEOWNERS.github/label-pr-config.ymlCONTRIBUTING.mdBUILDING.mdSECURITY.md
📚 Learning: 2025-10-14T10:18:23.123Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 377
File: test/parallel/test-nsolid-heap-profile-stream.js:81-88
Timestamp: 2025-10-14T10:18:23.123Z
Learning: In nsolid.heapProfileStream() and nsolid.heapSamplingStream(), synchronous throws occur for argument validation errors and immediate state validation checks (like assets being disabled), while async error events are emitted for runtime errors that manifest during execution (like invalid threadId).
Applied to files:
agents/grpc/lib/agent.js
📚 Learning: 2025-07-08T14:46:47.806Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 339
File: test/addons/nsolid-elf-utils/nsolid-elf-utils.js:13-14
Timestamp: 2025-07-08T14:46:47.806Z
Learning: In the nsolid test suite, native addon bindings are expected to be built beforehand; tests do not add fs.existsSync guards to skip when bindings are missing.
Applied to files:
.github/CODEOWNERSBUILDING.mdMakefile
📚 Learning: 2025-07-08T14:47:34.724Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 339
File: test/addons/nsolid-elf-utils/nsolid-elf-utils.js:16-18
Timestamp: 2025-07-08T14:47:34.724Z
Learning: In the nsolid test suite, external tool invocations (e.g., execSync to run `readelf`) are intentionally left uncaught so that any failure causes the test to fail rather than being skipped.
Applied to files:
BUILDING.md
📚 Learning: 2025-07-08T14:48:04.827Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 339
File: test/addons/nsolid-elf-utils/binding.cc:13-28
Timestamp: 2025-07-08T14:48:04.827Z
Learning: In nsolid test native addons (e.g., `test/addons/*/binding.cc`), additional JavaScript-facing argument validation is typically omitted because the tests supply well-formed inputs.
Applied to files:
BUILDING.mdMakefileagents/grpc/src/binding.cc
🧬 Code graph analysis (9)
agents/grpc/src/proto/blocked_loop.grpc.pb.h (4)
agents/grpc/src/proto/asset.pb.h (2)
grpcagent(57-62)grpcagent(68-427)agents/grpc/src/proto/info.pb.h (1)
grpcagent(60-73)agents/grpc/src/proto/blocked_loop.pb.h (1)
grpcagent(56-77)agents/grpc/src/proto/command.pb.h (1)
grpcagent(58-71)
agents/grpc/src/proto/asset.grpc.pb.h (1)
agents/grpc/src/proto/asset.pb.h (2)
grpcagent(57-62)grpcagent(68-427)
agents/grpc/src/grpc_utils.h (2)
agents/grpc/src/grpc_client.h (3)
node(25-152)nsolid(26-151)grpc(27-149)src/debug_utils.h (1)
DebugCategory(63-67)
agents/grpc/src/asset_stream.cc (2)
agents/grpc/src/proto/nsolid_service.grpc.pb.h (1)
NSolidService(31-44)agents/grpc/src/command_stream.cc (8)
OnDone(43-63)OnDone(43-43)OnWriteDone(87-100)OnWriteDone(87-87)NextWrite(102-107)NextWrite(102-102)Write(109-113)Write(109-109)
agents/grpc/src/grpc_client.cc (2)
agents/grpc/src/proto/nsolid_service.grpc.pb.h (1)
NSolidService(31-44)agents/grpc/src/proto/nsolid_service.grpc.pb.cc (2)
NewStub(40-44)NewStub(40-40)
agents/grpc/src/proto/command.grpc.pb.h (7)
agents/grpc/src/proto/command.pb.h (1)
grpcagent(58-71)agents/grpc/src/proto/asset.grpc.pb.h (1)
grpcagent(29-31)agents/grpc/src/proto/blocked_loop.grpc.pb.h (1)
grpcagent(35-37)agents/grpc/src/proto/common.grpc.pb.h (1)
grpcagent(29-31)agents/grpc/src/proto/exit.grpc.pb.h (1)
grpcagent(29-31)agents/grpc/src/proto/info.grpc.pb.h (1)
grpcagent(29-31)agents/grpc/src/proto/metrics.grpc.pb.h (1)
grpcagent(29-31)
agents/grpc/src/command_stream.cc (1)
agents/grpc/src/proto/nsolid_service.grpc.pb.h (1)
NSolidService(31-44)
agents/grpc/src/binding.cc (3)
agents/grpc/src/grpc_agent.cc (2)
Inst(413-418)Inst(413-413)agents/grpc/src/proto/profile.pb.h (1)
track_allocations(645-730)src/node_binding.cc (2)
RegisterExternalReferences(706-709)RegisterExternalReferences(706-706)
agents/grpc/src/asset_stream.h (4)
agents/grpc/src/grpc_client.h (3)
node(25-152)nsolid(26-151)grpc(27-149)agents/grpc/src/grpc_agent.h (1)
grpc(39-65)agents/grpc/src/asset_stream.cc (12)
AssetStream(16-39)AssetStream(41-42)OnDone(44-73)OnDone(44-44)OnWriteDone(75-107)OnWriteDone(75-75)Write(132-136)Write(132-132)WritesDone(138-143)WritesDone(138-138)NextWrite(109-130)NextWrite(109-109)agents/grpc/src/proto/nsolid_service.grpc.pb.h (1)
NSolidService(31-44)
🪛 actionlint (1.7.9)
.github/workflows/coverage-linux-without-intl.yml
51-51: condition "false" is always evaluated to false. remove the if: section
(if-cond)
.github/workflows/coverage-linux.yml
51-51: condition "false" is always evaluated to false. remove the if: section
(if-cond)
.github/workflows/test-asan.yml
41-41: condition "false" is always evaluated to false. remove the if: section
(if-cond)
42-42: label "ubuntu-20.04" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
.github/workflows/test-ubsan.yml
40-40: condition "false" is always evaluated to false. remove the if: section
(if-cond)
🪛 Buf (1.61.0)
agents/grpc/proto/packages.proto
8-8: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/source_code.proto
3-3: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/metrics.proto
3-3: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/asset.proto
3-3: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/info.proto
3-3: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/exit.proto
3-3: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/blocked_loop.proto
8-8: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/common.proto
3-3: Files with package "grpcagent" must be within a directory "grpcagent" relative to root but were in directory "agents/grpc/proto".
(PACKAGE_DIRECTORY_MATCH)
agents/grpc/proto/command.proto
8-8: import "profile.proto": file does not exist
(COMPILE)
agents/grpc/proto/profile.proto
5-5: Files with package "grpcagent" must be within a directory "grpcagent" relative to root but were in directory "agents/grpc/proto".
(PACKAGE_DIRECTORY_MATCH)
agents/grpc/proto/reconfigure.proto
3-3: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/nsolid_service.proto
3-3: import "asset.proto": file does not exist
(COMPILE)
agents/grpc/proto/startup_times.proto
3-3: import "common.proto": file does not exist
(COMPILE)
🪛 checkmake (0.2.2)
Makefile
[warning] 313-313: Target body for "jstest" exceeds allowed length of 5 (6).
(maxbodylength)
🪛 LanguageTool
CODE_OF_CONDUCT.md
[style] ~27-~27: Try using a synonym here to strengthen your wording.
Context: ...vances * Trolling, insulting/derogatory comments, and personal or political attacks * Pu...
(COMMENT_REMARK)
[style] ~62-~62: ‘with regard to’ might be wordy. Consider a shorter alternative.
Context: ...s obligated to maintain confidentiality with regard to the reporter of an incident. Further de...
(EN_WORDINESS_PREMIUM_WITH_REGARD_TO)
GOVERNANCE.md
[style] ~173-~173: The phrase ‘have the ability to’ might be wordy. Consider using “can”.
Context: ...their judgement be trusted, and do they have the ability to work well with others. #### The Authen...
(HAS_THE_ABILITY_TO)
[style] ~181-~181: Consider a more concise word here.
Context: ...n commit access to open-source projects in order to inject malicious code or for other nefa...
(IN_ORDER_TO_PREMIUM)
[style] ~199-~199: Try using a more formal synonym here to elevate your writing.
Context: ...in the private discussion stating you're planning on opening a public issue, e.g. "I see a number...
(PLAN_ON_INTEND)
[grammar] ~237-~237: Use a hyphen to join words.
Context: ...TSC may, following typical TSC consensus seeking processes, choose to advance a n...
(QB_NEW_EN_HYPHEN)
[style] ~304-~304: This phrase is redundant. Consider using “outside”.
Context: ...ned, and avoid sharing those concerns outside of the Collaborator discussion area. Ide...
(OUTSIDE_OF)
.github/SUPPORT.md
[style] ~4-~4: Consider using a different verb to strengthen your wording.
Context: ...dress general support questions. Please make sure you are using a currently supported ver...
(MAKE_SURE_ENSURE)
CHANGELOG_NSOLID.md
[uncategorized] ~7-~7: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...logs/NSOLID_CHANGELOG_V5_NODE_V20.md) Long Term Support
(EN_COMPOUND_ADJECTIVE_INTERNAL)
README.md
[uncategorized] ~11-~11: The official name of this software platform is spelled with a capital “H”.
Context: ...ut the instructions for getting support. ### Download Binaries, i...
(GITHUB)
[style] ~48-~48: Consider using a more concise synonym.
Context: ...rnal libraries that are available under a variety of licenses. See LICENSE and [L...
(A_VARIETY_OF)
CHANGELOG.md
[uncategorized] ~5-~5: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... 20](doc/changelogs/CHANGELOG_V20.md) Long Term Support * [Node.js 19](doc/changelogs...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~7-~7: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...js 18](doc/changelogs/CHANGELOG_V18.md) Long Term Support * [Node.js 17](doc/changelogs/C...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
CONTRIBUTING.md
[style] ~12-~12: Consider a more concise word here.
Context: .../github.com/nodesource/nsolid/issues>. In order to build this project, you will need a bui...
(IN_ORDER_TO_PREMIUM)
[grammar] ~97-~97: Use a hyphen to join words.
Context: ...TS version. For the initial N|Solid open source release, this branch was `nsolid-...
(QB_NEW_EN_HYPHEN)
[style] ~98-~98: ‘Prior to’ might be wordy. Consider a shorter alternative.
Context: ...release, this branch was nsolid-v5.x. Prior to releases, relevant changes will be port...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[style] ~216-~216: As an alternative to the over-used intensifier ‘quite’, consider replacing this phrase.
Context: ... of the types of Node.js API changes is quite wide, but it is also the least likely to lan...
(EN_WEAK_ADJECTIVE)
[style] ~222-~222: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ... zero-filling Buffers during allocation prior to the Buffer.alloc upstream Node.js API...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[grammar] ~293-~293: Use a hyphen to join words.
Context: ...MAJOR version increments in N|Solid. Pre v5.0.0 versions of N|Solid are propr...
(QB_NEW_EN_HYPHEN)
BUILDING.md
[grammar] ~148-~148: Ensure spelling is correct
Context: ...es are compiled with 10.15 as a target. Xcode11 is required to compile. <!--lint e...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~845-~845: Use a hyphen to join words.
Context: ...fficial N|Solid builds (e.g. using a ABI incompatible version of a dependency), p...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
GOVERNANCE.md
294-294: Multiple headings with the same content
(MD024, no-duplicate-heading)
CONTRIBUTING.md
294-294: Multiple headings with the same content
(MD024, no-duplicate-heading)
BUILDING.md
36-36: Link fragments should be valid
(MD051, link-fragments)
37-37: Link fragments should be valid
(MD051, link-fragments)
38-38: Link fragments should be valid
(MD051, link-fragments)
| /^src\/quic\/*/: c++, quic | ||
| /^src\/node_bob*/: c++, quic |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, read the full label-pr-config.yml to understand the context and tool
head -50 .github/label-pr-config.ymlRepository: nodesource/nsolid
Length of output: 2098
🏁 Script executed:
# Check lines around 33-34 for context
sed -n '25,40p' .github/label-pr-config.ymlRepository: nodesource/nsolid
Length of output: 597
🏁 Script executed:
# List actual directory structure in src/quic and src/node_bob areas
find src -type d -name "*quic*" -o -type d -name "*bob*" | head -20Repository: nodesource/nsolid
Length of output: 69
🏁 Script executed:
# Check what files exist in src/quic/ if it exists
ls -la src/quic/ 2>/dev/null || echo "src/quic/ does not exist"Repository: nodesource/nsolid
Length of output: 1511
🏁 Script executed:
# Look for patterns similar to lines 33-34 in the file to understand the syntax
grep -n "src/" .github/label-pr-config.yml | head -30Repository: nodesource/nsolid
Length of output: 43
🏁 Script executed:
# Check if any node_bob* directories or files exist
find src -name "*bob*" -o -name "*node_bob*" 2>/dev/nullRepository: nodesource/nsolid
Length of output: 94
🏁 Script executed:
# Look for any references to the labeling tool or tests
find . -name "*label*" -type f 2>/dev/null | grep -E "\.(yml|yaml|js|py|test)" | head -20Repository: nodesource/nsolid
Length of output: 1276
🏁 Script executed:
# Check git history for this file to understand the intent
git log --oneline .github/label-pr-config.yml 2>/dev/null | head -10Repository: nodesource/nsolid
Length of output: 119
🏁 Script executed:
# Verify what these patterns actually match by testing them
python3 << 'EOF'
import re
# Test pattern 1
pattern1 = r'^src\/quic\/*'
files_to_test = [
'src/quic/',
'src/quic//',
'src/quic/packet.h',
'src/quic/packet.cc',
]
print("Pattern 1: /^src\\/quic\\/*/ (regex: " + pattern1 + ")")
for f in files_to_test:
match = re.match(pattern1, f)
print(f" {f}: {'MATCH' if match else 'NO MATCH'}")
print()
# Test pattern 2
pattern2 = r'^src\/node_bob*'
files_to_test2 = [
'src/node_bob',
'src/node_bobb',
'src/node_bob.h',
'src/node_bob-inl.h',
'src/node_bobcc',
]
print("Pattern 2: /^src\\/node_bob*/ (regex: " + pattern2 + ")")
for f in files_to_test2:
match = re.match(pattern2, f)
print(f" {f}: {'MATCH' if match else 'NO MATCH'}")
print()
# Show corrected patterns
print("Corrected patterns:")
pattern1_fixed = r'^src\/quic/'
pattern2_fixed = r'^src\/node_bob[._-]'
print(f"Pattern 1 fixed: {pattern1_fixed}")
for f in ['src/quic/packet.h', 'src/quic/packet.cc']:
match = re.match(pattern1_fixed, f)
print(f" {f}: {'MATCH' if match else 'NO MATCH'}")
print(f"Pattern 2 fixed: {pattern2_fixed}")
for f in ['src/node_bob.h', 'src/node_bob-inl.h']:
match = re.match(pattern2_fixed, f)
print(f" {f}: {'MATCH' if match else 'NO MATCH'}")
EOFRepository: nodesource/nsolid
Length of output: 147
Fix regex patterns for QUIC and BOB subsystems—current patterns will not match any files.
The patterns at lines 33–34 are broken regex patterns that fail to match their intended files:
- Line 33:
/^src\/quic\/*/matches "zero or more forward slashes" afterquic/(e.g.,src/quic/,src/quic//). It does not match actual files likesrc/quic/packet.horsrc/quic/cid.cc. - Line 34:
/^src\/node_bob*/matchesnode_bobfollowed by zero or morebcharacters. It does not match the actual filessrc/node_bob.horsrc/node_bob-inl.hbecause the.and-characters are not accounted for.
Correct these patterns:
- /^src\/quic\/*/: c++, quic
- /^src\/node_bob*/: c++, quic
+ /^src\/quic\//: c++, quic
+ /^src\/node_bob[._-]/: c++, quicCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
.github/label-pr-config.yml around lines 33–34: the two regex patterns are
incorrect and don't match real files; replace them so the first matches any file
path under the src/quic directory (i.e., paths that start with src/quic/
followed by any characters) and the second matches files that begin with
src/node_bob followed by either a dot or a hyphen and the remainder of the
filename (to cover names like node_bob.h and node_bob-inl.h).
| # Disabled because "Report JS" step was crashing. | ||
| if: false | ||
| runs-on: ubuntu-24.04 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the full context of the workflow file around the disabled job
cat -n .github/workflows/coverage-linux-without-intl.yml | head -80Repository: nodesource/nsolid
Length of output: 3071
🏁 Script executed:
# Search for any TODO comments or issue references in the file
rg -i "todo|issue|#[0-9]|report.*js|js.*report" .github/workflows/coverage-linux-without-intl.ymlRepository: nodesource/nsolid
Length of output: 193
🏁 Script executed:
# Search for similar `if: false` patterns in other workflows
rg "if:\s*false" .github/workflows/Repository: nodesource/nsolid
Length of output: 514
🏁 Script executed:
# Check git history to see if this disable was recent
git log --oneline -20 -- .github/workflows/coverage-linux-without-intl.ymlRepository: nodesource/nsolid
Length of output: 119
🏁 Script executed:
# Search for any issues or PRs mentioning the JS report crash
rg -i "report.*js.*crash|js.*report.*crash" --type md .Repository: nodesource/nsolid
Length of output: 43
The coverage-linux-without-intl job is disabled without tracking; add an issue reference.
The if: false condition on line 51 disables this coverage workflow. While other similar disabled workflows include GitHub issue references (e.g., test-ubsan.yml, test-asan.yml), this one lacks any tracking reference. Add an issue reference or TODO comment with timeline for re-enabling, or remove the job if it's permanently superseded.
🧰 Tools
🪛 actionlint (1.7.9)
51-51: condition "false" is always evaluated to false. remove the if: section
(if-cond)
🤖 Prompt for AI Agents
.github/workflows/coverage-linux-without-intl.yml around lines 50 to 52: the job
is disabled via "if: false" but lacks a tracking reference; add a short TODO
comment or GitHub issue reference explaining why it was disabled and when to
re-enable (or remove the job if permanently obsolete). Update the file by adding
a one-line comment above or next to the "if: false" with an issue URL or "TODO:
re-enable by <date>" and the reason (e.g., "Report JS step crashing"), so the
disablement is tracked and actionable.
| coverage-linux: | ||
| if: github.event.pull_request.draft == false | ||
| runs-on: ubuntu-latest | ||
| # Disabled because "Report JS" step was crashing. | ||
| if: false |
There was a problem hiding this comment.
Disabled coverage job lacks tracking issue reference.
The coverage-linux job is permanently disabled due to a "Report JS" step crash, but there's no GitHub issue reference or FIXME comment for maintainers. This makes it unclear how or when to re-enable the job.
Add a link to a tracking issue or open one to document the failure and recovery steps:
coverage-linux:
- # Disabled because "Report JS" step was crashing.
+ # Disabled because "Report JS" step was crashing.
+ # TODO: Re-enable after fixing; see https://github.com/nodesource/nsolid/issues/XXX
if: falseDo you have an associated issue for the "Report JS" crash? If so, I can help update the comment with a reference.
📝 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.
| coverage-linux: | |
| if: github.event.pull_request.draft == false | |
| runs-on: ubuntu-latest | |
| # Disabled because "Report JS" step was crashing. | |
| if: false | |
| coverage-linux: | |
| # Disabled because "Report JS" step was crashing. | |
| # TODO: Re-enable after fixing; see https://github.com/nodesource/nsolid/issues/XXX | |
| if: false |
🧰 Tools
🪛 actionlint (1.7.9)
51-51: condition "false" is always evaluated to false. remove the if: section
(if-cond)
🤖 Prompt for AI Agents
.github/workflows/coverage-linux.yml lines 49-51: the coverage-linux job is
permanently disabled with `if: false` but lacks any tracking-issue reference or
FIXME note; add a clear comment above the job that includes a link to a GitHub
issue (or create one) describing the "Report JS" step crash, the current
workaround, expected owner and next steps, and include the issue number and date
(e.g. "FIXME: disabled due to Report JS crash — tracking:
github.com/ORG/REPO/issues/NN — reopened by @owner on YYYY-MM-DD"); optionally
change `if: false` to a conditional checking that the issue is closed (e.g.
reference a repo secret or workflow input) or add a TODO with the issue link so
maintainers know when/how to re-enable it.
| body: | | ||
| This PR was generated by the [`find-inactive-collaborators.yml` workflow](https://github.com/nodejs/node/blob/main/.github/workflows/find-inactive-collaborators.yml). | ||
|
|
||
| @nodejs/tsc Please follow up with the [offboarding tasks](https://github.com/nodejs/node/blob/main/doc/contributing/offboarding.md). |
There was a problem hiding this comment.
Update repository URLs in PR body template.
Lines 45 and 47 contain links to github.com/nodejs/node, but this workflow is in the nodesource/nsolid repository. Update these URLs to point to the correct repository for consistency.
Apply this diff to correct the repository references:
body: |
- This PR was generated by the [`find-inactive-collaborators.yml` workflow](https://github.com/nodejs/node/blob/main/.github/workflows/find-inactive-collaborators.yml).
+ This PR was generated by the [`find-inactive-collaborators.yml` workflow](https://github.com/nodesource/nsolid/blob/main/.github/workflows/find-inactive-collaborators.yml).
- @nodejs/tsc Please follow up with the [offboarding tasks](https://github.com/nodejs/node/blob/main/doc/contributing/offboarding.md).
+ @nodesource/leadership Please follow up with the [offboarding tasks](https://github.com/nodesource/nsolid/blob/main/doc/contributing/offboarding.md).📝 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.
| body: | | |
| This PR was generated by the [`find-inactive-collaborators.yml` workflow](https://github.com/nodejs/node/blob/main/.github/workflows/find-inactive-collaborators.yml). | |
| @nodejs/tsc Please follow up with the [offboarding tasks](https://github.com/nodejs/node/blob/main/doc/contributing/offboarding.md). | |
| body: | | |
| This PR was generated by the [`find-inactive-collaborators.yml` workflow](https://github.com/nodesource/nsolid/blob/main/.github/workflows/find-inactive-collaborators.yml). | |
| @nodesource/leadership Please follow up with the [offboarding tasks](https://github.com/nodesource/nsolid/blob/main/doc/contributing/offboarding.md). |
🤖 Prompt for AI Agents
.github/workflows/find-inactive-collaborators.yml around lines 44 to 47: the PR
body links point to github.com/nodejs/node but this workflow lives in
nodesource/nsolid, so update both repository URLs to reference
nodesource/nsolid. Replace the link to the workflow file and the offboarding
tasks link so they point to the corresponding paths in the nodesource/nsolid
repository (use
https://github.com/nodesource/nsolid/blob/main/.github/workflows/find-inactive-collaborators.yml
and
https://github.com/nodesource/nsolid/blob/main/doc/contributing/offboarding.md
respectively) ensuring the PR body text remains the same.
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@ac593985615ec2ede58e132d2e21d2b1cbd6127c # v3.3.0 | ||
| - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/license-builder.yml | sed -n '20,30p'Repository: nodesource/nsolid
Length of output: 707
🌐 Web query:
actions/checkout latest version December 2025 GitHub
💡 Result:
As of December 3, 2025 the latest actions/checkout release is v6. [1][2]
Sources:
- actions/checkout repository (release notes / README showing Checkout v6). [1]
- GitHub Marketplace listing for Checkout (notes mentioning v6). [2]
🌐 Web query:
gr2m/create-or-update-pull-request-action latest version December 2025
💡 Result:
Latest published major version: v1 — use gr2m/create-or-update-pull-request-action@v1. [1]
Source: GitHub Marketplace listing for "Create or Update Pull Request" action. [1]
Update actions/checkout to the latest version.
The actions/checkout@v4.2.2 is outdated. As of December 2025, the latest stable version is v6. The gr2m/create-or-update-pull-request-action@v1.9.4 appears to be current. Update actions/checkout to v6 (or at minimum v5) for the latest features, improvements, and security patches.
🤖 Prompt for AI Agents
.github/workflows/license-builder.yml around line 20: the workflow pins
actions/checkout to an old commit
(actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 which corresponds to
v4.2.2); update the action reference to a supported release tag such as
actions/checkout@v6 (or at minimum actions/checkout@v5) instead of the raw
commit SHA, ensure the rest of the workflow still functions with the newer
checkout version, run the workflow locally or in CI to verify no breakages, and
commit the updated workflow file.
| QueueCallback([](WeakGrpcAgent agent_wp) { | ||
| SharedGrpcAgent agent = agent_wp.lock(); | ||
| if (agent == nullptr) { | ||
| return; | ||
| } | ||
|
|
||
| agent->stop(); | ||
| delete agent.get(); | ||
| }, weak_from_this()); | ||
| return; |
There was a problem hiding this comment.
Critical: Double-delete vulnerability causes undefined behavior.
"When the shared_ptr goes out of scope and its reference count falls to zero it will attempt to delete p;, leading to double deletion and undefined behavior."
The delete agent.get() on line 516 manually deletes the object managed by SharedGrpcAgent (a shared_ptr). When the last shared_ptr reference goes out of scope, it will attempt to delete the already-freed memory, causing a double-free and undefined behavior.
The agent singleton from GrpcAgent::Inst() already has a custom deleter. If the intent is to shut down the agent on authentication failure, simply calling stop() and allowing the shared_ptr to manage lifetime is sufficient:
QueueCallback([](WeakGrpcAgent agent_wp) {
SharedGrpcAgent agent = agent_wp.lock();
if (agent == nullptr) {
return;
}
agent->stop();
- delete agent.get();
}, weak_from_this());If forced cleanup is required, consider resetting the singleton or using a different shutdown mechanism that respects shared_ptr ownership semantics.
📝 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.
| QueueCallback([](WeakGrpcAgent agent_wp) { | |
| SharedGrpcAgent agent = agent_wp.lock(); | |
| if (agent == nullptr) { | |
| return; | |
| } | |
| agent->stop(); | |
| delete agent.get(); | |
| }, weak_from_this()); | |
| return; | |
| QueueCallback([](WeakGrpcAgent agent_wp) { | |
| SharedGrpcAgent agent = agent_wp.lock(); | |
| if (agent == nullptr) { | |
| return; | |
| } | |
| agent->stop(); | |
| }, weak_from_this()); | |
| return; |
🤖 Prompt for AI Agents
In agents/grpc/src/grpc_agent.cc around lines 509 to 518, the code manually
deletes the object managed by a SharedGrpcAgent (shared_ptr) causing a
double-delete; remove the explicit delete agent.get() and only call
agent->stop() so shared_ptr manages lifetime, and if explicit teardown is
required, reset or release the singleton/shared_ptr via its proper reset/cleanup
API (do not use delete on agent.get()) to avoid breaking shared_ptr ownership
semantics.
| bool insecure = false; | ||
| std::string insecure_str; | ||
| bool insecure_set = per_process::system_environment-> | ||
| Get(kNSOLID_GRPC_INSECURE).To(&insecure_str); | ||
| // Only parse the insecure flag in non SaaS mode. | ||
| if (insecure_set && (!saas_ || saas_->testing)) { | ||
| insecure = std::stoi(insecure_str); | ||
| } |
There was a problem hiding this comment.
Missing exception handling for std::stoi.
std::stoi throws std::invalid_argument if no conversion can be performed, or std::out_of_range if the value is out of the representable range. If a user sets NSOLID_GRPC_INSECURE to a non-numeric value (e.g., "true", "yes"), this will cause an unhandled exception.
Consider using a safer parsing approach:
bool insecure = false;
std::string insecure_str;
bool insecure_set = per_process::system_environment->
Get(kNSOLID_GRPC_INSECURE).To(&insecure_str);
// Only parse the insecure flag in non SaaS mode.
if (insecure_set && (!saas_ || saas_->testing)) {
- insecure = std::stoi(insecure_str);
+ try {
+ insecure = std::stoi(insecure_str) != 0;
+ } catch (const std::exception&) {
+ // Treat invalid values as false, or log a warning
+ insecure = false;
+ }
}Alternatively, check for specific string values like "1", "true", "yes" without using std::stoi.
📝 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.
| bool insecure = false; | |
| std::string insecure_str; | |
| bool insecure_set = per_process::system_environment-> | |
| Get(kNSOLID_GRPC_INSECURE).To(&insecure_str); | |
| // Only parse the insecure flag in non SaaS mode. | |
| if (insecure_set && (!saas_ || saas_->testing)) { | |
| insecure = std::stoi(insecure_str); | |
| } | |
| bool insecure = false; | |
| std::string insecure_str; | |
| bool insecure_set = per_process::system_environment-> | |
| Get(kNSOLID_GRPC_INSECURE).To(&insecure_str); | |
| // Only parse the insecure flag in non SaaS mode. | |
| if (insecure_set && (!saas_ || saas_->testing)) { | |
| try { | |
| insecure = std::stoi(insecure_str) != 0; | |
| } catch (const std::exception&) { | |
| // Treat invalid values as false, or log a warning | |
| insecure = false; | |
| } | |
| } |
| #ifndef AGENTS_GRPC_SRC_GRPC_ERRORS_H_ | ||
| #define AGENTS_GRPC_SRC_GRPC_ERRORS_H_ | ||
|
|
There was a problem hiding this comment.
Missing required standard library includes.
The header uses uint32_t and std::string but does not include the necessary headers. This can cause compilation failures depending on include order.
#ifndef AGENTS_GRPC_SRC_GRPC_ERRORS_H_
#define AGENTS_GRPC_SRC_GRPC_ERRORS_H_
+#include <cstdint>
+#include <string>
+
#define GRPC_ERRORS(X) \📝 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.
| #ifndef AGENTS_GRPC_SRC_GRPC_ERRORS_H_ | |
| #define AGENTS_GRPC_SRC_GRPC_ERRORS_H_ | |
| #ifndef AGENTS_GRPC_SRC_GRPC_ERRORS_H_ | |
| #define AGENTS_GRPC_SRC_GRPC_ERRORS_H_ | |
| #include <cstdint> | |
| #include <string> | |
🤖 Prompt for AI Agents
In agents/grpc/src/grpc_errors.h around lines 1 to 3, the header uses uint32_t
and std::string but doesn't include their definitions; add the required standard
includes (e.g. #include <cstdint> and #include <string>) immediately after the
include guard so the types are available regardless of include order.
| using google::protobuf::Message; | ||
| using google::protobuf::util::MessageToJsonString; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid using declarations at file scope in headers.
These using declarations pollute the namespace of every file that includes this header. Move them inside the functions or use fully qualified names.
-using google::protobuf::Message;
-using google::protobuf::util::MessageToJsonString;
-
namespace node {
namespace nsolid {
namespace grpc {Then update the function to use qualified names:
template <typename... Args>
inline void DebugProtobufMsg(const char* format,
- const Message& msg,
+ const google::protobuf::Message& msg,
Args&&... args) {
if (per_process::enabled_debug_list.enabled(
DebugCategory::NSOLID_GRPC_AGENT)) {
std::string json_payload;
- const auto status = MessageToJsonString(msg, &json_payload);
+ const auto status = google::protobuf::util::MessageToJsonString(msg, &json_payload);Committable suggestion skipped: line range outside the PR's diff.
| agents/grpc/src/*.cc \ | ||
| agents/grpc/src/*.h \ | ||
| agents/otlp/src/*.cc \ | ||
| agents/otlp/src/*.h \ | ||
| agents/src/*.cc \ | ||
| agents/src/*.h \ | ||
| agents/statsd/src/*.cc \ | ||
| agents/statsd/src/*.h \ | ||
| agents/zmq/src/*.cc \ | ||
| agents/zmq/src/*.h \ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify agent and profiler directories exist
dirs=(
"agents/grpc/src"
"agents/otlp/src"
"agents/src"
"agents/statsd/src"
"agents/zmq/src"
"deps/nsolid_cpu_profiler"
"deps/nsolid_heap_profiler"
)
echo "=== Directory Verification ==="
for dir in "${dirs[@]}"; do
if [ -d "$dir" ]; then
echo "✓ $dir exists"
else
echo "✗ $dir MISSING"
fi
doneRepository: nodesource/nsolid
Length of output: 283
🏁 Script executed:
# Check the Makefile content around the specified lines
echo "=== Makefile lines 1441-1450 ==="
sed -n '1441,1450p' Makefile
echo ""
echo "=== Makefile lines 1474-1479 ==="
sed -n '1474,1479p' Makefile
# Also check context around these sections
echo ""
echo "=== Makefile lines 1435-1455 (broader context) ==="
sed -n '1435,1455p' MakefileRepository: nodesource/nsolid
Length of output: 1233
C++ linting coverage expanded for agent code only.
LINT_CPP_FILES now includes agent implementations in agents/grpc/src, agents/otlp/src, agents/src, agents/statsd/src, and agents/zmq/src.
Note: Lines 1474-1479 reference deps/nsolid_cpu_profiler and deps/nsolid_heap_profiler directories that do not exist in the repository. These paths will match no files via wildcard expansion and should be removed or the directories created if linting coverage for profilers is intended.
🤖 Prompt for AI Agents
In Makefile around lines 1441 to 1450 (and note references at 1474-1479), the
wildcard entries for deps/nsolid_cpu_profiler and deps/nsolid_heap_profiler
point to directories that do not exist and will expand to nothing; remove those
non-existent path patterns from LINT_CPP_FILES or create the missing directories
and add the intended files. Update the Makefile so LINT_CPP_FILES only contains
existing directories (or ensure the profiler dirs are created and populated),
and re-run a make lint target to verify the wildcard expansion no longer
silently matches no files.
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/notify-on-push.yml (1)
13-13: Repository checks hardcoded tonodejs/nodewill prevent workflows from triggering in nodesource/nsolid.Both conditions (line 13 and line 32) check
github.repository == 'nodejs/node', but this PR targets nodesource/nsolid. These workflows will never execute in this repository.Update the repository checks to match the target repository:
- if: github.repository == 'nodejs/node' && github.event.forced + if: github.repository == 'nodesource/nsolid' && github.event.forcedand
- if: github.repository == 'nodejs/node' + if: github.repository == 'nodesource/nsolid'Also applies to: 32-32
🟡 Minor comments (11)
.github/workflows/update-openssl.yml-30-30 (1)
30-30: Consider the maintenance and security posture of gr2m/create-or-update-pull-request-action.The pinned commit SHA (
77596e3166f328b24613f7082ab30bf2d93079d5) is good practice for reproducibility. However, third-party security scanners flag this action with a low security score due to dependency vulnerabilities and maintenance concerns. The action also has known issues with deprecated GitHub API usage. Consider auditing the action's dependency tree for CVEs using Snyk/Dependabot, or switch to a well-maintained alternative such aspeter-evans/create-pull-requestif active maintenance is a priority.Also applies to: 53-53
agents/grpc/proto/packages.proto-1-4 (1)
1-4: Remove or address the TODO comments before merging.The comments describe a future RuntimeResponse oneOf structure that isn't implemented in this file. If this design is still pending, it should be tracked in an issue rather than a TODO comment in production code. If the current structure is final, these comments should be removed.
Would you like me to help draft an issue to track the RuntimeResponse oneOf refactoring, or should these comments simply be removed?
agents/grpc/src/grpc_client.h-44-46 (1)
44-46: Initializestartmember to zero.The
startmember is not initialized in the default constructor. While the code guards usage withcall_data->start > 0(line 129), an uninitialized value could coincidentally be non-zero, leading to incorrect latency calculations when debug logging is disabled.EventType* event = nullptr; grpcagent::EventResponse* event_response = nullptr; - uint64_t start; + uint64_t start = 0;agents/grpc/src/grpc_agent.cc-71-71 (1)
71-71: Fix comment: value is 4MB, not 4GB.The constant is
4L * 1024 * 1024which equals 4,194,304 bytes (4MB), but the comment incorrectly states "4GB".-const size_t GRPC_MAX_SIZE = 4L * 1024 * 1024; // 4GB +const size_t GRPC_MAX_SIZE = 4L * 1024 * 1024; // 4MB.github/workflows/linters.yml-70-70 (1)
70-70: Permanently disabled condition should be clarified.The
&& falsedisables the "Get release version numbers" step, preventing NODE_RELEASED_VERSIONS from being set. While the markdown linter treats this variable as optional and will still run successfully, this condition appears to be either leftover debug code or intentional temporary disabling that should be documented or removed.Either remove
&& falseif this was unintentional, or replace the step with a comment explaining the temporary disable:- - name: Get release version numbers - if: ${{ github.event.pull_request && github.event.pull_request.base.ref == github.event.pull_request.base.repo.default_branch && false }} - id: get-released-versions - run: ./tools/lint-md/list-released-versions-from-changelogs.mjs >> $GITHUB_OUTPUT + # TODO: Re-enable version validation for markdown linting + # - name: Get release version numbers + # if: ${{ github.event.pull_request && github.event.pull_request.base.ref == github.event.pull_request.base.repo.default_branch }} + # id: get-released-versions + # run: ./tools/lint-md/list-released-versions-from-changelogs.mjs >> $GITHUB_OUTPUTBUILDING.md-36-38 (1)
36-38: Fix Windows ToC anchors to match current headingsThe ToC entries for “Windows Prerequisites” and the “Option 1/2” subsections link to
#windows-prerequisites,#option-1-manual-install, and#option-2-automated-install-with-boxstarter, but the Windows section now uses#### Prerequisitesand##### Manual installand no longer defines matching headings for those fragments. This is what markdownlint (MD051) is complaining about and it breaks intra-page navigation.Please either:
- Rename the Windows headings to match the existing fragments, or
- Update/remove the ToC entries so all linked fragments correspond to actual headings.
.github/workflows/daily-wpt-fyi.yml-22-31 (1)
22-31: Add error handling for external API fetch or document the accepted risk.The jq query logic is correct and validates against the actual schedule.json structure—the current date filtering works as intended, producing version numbers like [20, 22, 24, 25, "latest-nightly"] for supported releases.
However, the shell script on line 30 lacks error handling. If
curlfails (network issue, GitHub API rate limit, or repository unavailable) or the JSON is malformed, thematrixvariable could be empty or undefined. The workflow would not fail visibly; instead, thereportjob would receive an empty matrix and silently skip all test runs.Consider either:
- Adding error handling:
curl -s ... || echo 'matrix=["latest-nightly"]' >> "$GITHUB_OUTPUT"to provide a fallback- Adding
set -eand explicit status checks before writing toGITHUB_OUTPUT- Documenting that silent failure is acceptable given the low-frequency schedule
.github/workflows/lint-release-proposal.yml-28-35 (1)
28-35: Remove or populate the$ACTUALvariable—currently undefined.Line 33 echoes
$ACTUALwhich is never set, appearing as a debug artifact. Either define this variable or remove the echo statement for clarity.Apply this diff to remove the undefined variable reference:
echo "Expected commit title format: $EXPECTED_TITLE" COMMIT_SUBJECT="$(git --no-pager log -1 --format=%s)" - echo "Actual: $ACTUAL" echo "$COMMIT_SUBJECT" | grep -q -E "$EXPECTED_TITLE" echo "COMMIT_SUBJECT=$COMMIT_SUBJECT" >> "$GITHUB_ENV".github/workflows/lint-release-proposal.yml-36-42 (1)
36-42: Remove or populate the$ACTUALvariable—currently undefined (duplicate).Line 41 echoes
$ACTUALwhich is never set, appearing as a debug artifact. Either define this variable or remove the echo statement for clarity.Apply this diff to remove the undefined variable reference:
EXPECTED_TRAILER="^$GITHUB_SERVER_URL/$GITHUB_REPOSITORY/pull/[[:digit:]]+\$" echo "Expected trailer format: $EXPECTED_TRAILER" PR_URL="$(git --no-pager log -1 --format='%(trailers:key=PR-URL,valueonly)')" - echo "Actual: $ACTUAL" echo "$PR_URL" | grep -E -q "$EXPECTED_TRAILER".github/workflows/coverage-linux-without-intl.yml-50-51 (1)
50-51: Add GitHub issue reference to document the "Report JS" crash.This job is intentionally disabled (
if: false) due to the "Report JS" step crashing. Similar disabled workflows in this repository (test-ubsan.yml, test-asan.yml) include GitHub issue references to track their status. Add a link to the issue tracking this problem, like:if: false # Disabled because "Report JS" step was crashing. Reference: https://github.com/...This makes the temporary disable more transparent and easier to track for future resolution.
Makefile-1556-1573 (1)
1556-1573: Update ruff to current version; 0.5.2 is significantly outdated.The pinned version 0.5.2 (released July 2024) is 16 months old; the current stable version is 0.14.7 (November 2025). While
ruff checkcommand syntax is correct, the version gap represents missed bug fixes, security patches, and new linting rules. Consider updating to a current version, or document why 0.5.2 is intentionally pinned for compatibility reasons.
🧹 Nitpick comments (30)
.github/workflows/notify-on-push.yml (1)
21-21: Inconsistent use ofgithub.refvsgithub.ref_namein Slack message titles.Line 21 uses
github.ref(full reference path) while line 23 usesgithub.ref_name(short branch name). Line 63 also usesgithub.ref. This inconsistency can produce confusing notification messages (e.g., "force-pushed to refs/heads/main" vs "force-pushed to main").Standardize to
github.ref_namefor cleaner Slack messages:- SLACK_TITLE: ${{ github.actor }} force-pushed to ${{ github.ref }} + SLACK_TITLE: ${{ github.actor }} force-pushed to ${{ github.ref_name }}and
- SLACK_TITLE: Invalid commit was pushed to ${{ github.ref }} + SLACK_TITLE: Invalid commit was pushed to ${{ github.ref_name }}Also applies to: 23-23, 63-63
.github/workflows/update-wpt.yml (1)
65-65: Substring extraction for short commit hash may be fragile.The expression
${new_version:0:10}extracts the first 10 characters of the commit hash. While this typically works for git shas, consider whether a standard 7-character short hash would be sufficient or if the variable name should be more descriptive (e.g.,short_commit_sha)..github/workflows/tools.yml (1)
291-322: Consider extracting repeated conditional logic.The condition
github.event_name == 'schedule' || inputs.id == 'all' || inputs.id == matrix.idis repeated across five steps (lines 296, 300, 305, 309, 313). This duplication increases maintenance burden and risks inconsistency if the logic changes.Extract this to a job-level condition or a reusable helper variable. For example, add at the start of the matrix job steps:
- name: Determine if step should run id: should_run run: | if [ "${{ github.event_name }}" == "schedule" ] || [ "${{ inputs.id }}" == "all" ] || [ "${{ inputs.id }}" == "${{ matrix.id }}" ]; then echo "run=true" >> $GITHUB_OUTPUT fiThen replace all five conditionals with
if: steps.should_run.outputs.run == 'true'. This centralizes the logic and makes future maintenance easier.agents/grpc/src/asset_stream.h (3)
7-7: Consider using a more robust include path.The relative path
../../src/profile_collector.his fragile and could break if the file structure changes. Consider adding the include directory to the build configuration instead.
19-22: Consider usingenum classfor type safety.Using
enum classinstead of plainenumwould prevent implicit conversions and provide stronger type safety, consistent withErrorTypeingrpc_errors.h.-enum AssetStreamRpcType { - EXPORT_ASSET, - EXPORT_CONTINUOUS_PROFILE -}; +enum class AssetStreamRpcType { + ExportAsset, + ExportContinuousProfile +};
71-82: Minor: Duplicateprivate:access specifier.There are two consecutive
private:sections. Consider consolidating them.private: void NextWrite(); - private: std::weak_ptr<AssetStreamObserver> observer_;agents/grpc/proto/profile.proto (1)
1-35: Aligngrpcagentpackage with Buf PACKAGE_DIRECTORY_MATCH (or update Buf config)Buf reports that files with package
grpcagentshould live under agrpcagent/directory relative to the module root, but this file is inagents/grpc/proto. To satisfy that rule you either need to:
- Move the
grpcagentprotos under agrpcagent/subdirectory, or- Change the package name / Buf module configuration if this layout is intentional and you don’t want package-directory coupling.
The message definitions themselves look fine; this is just about keeping the schema layout consistent with your Buf setup.
agents/grpc/proto/common.proto (2)
16-16: Consider using snake_case for field names per proto3 style guide.The field
requestIduses camelCase, but the Protocol Buffers style guide recommends snake_case for field names (e.g.,request_id). This applies to other proto files in this PR as well (e.g.,cpuCores,cpuModel,execPath, etc.).While camelCase works functionally, following the proto3 convention improves consistency with the broader protobuf ecosystem.
3-3: Consider aligning package directory structure with package name.The Buf linter flags that files with package "grpcagent" should be within a "grpcagent" directory relative to the root. While the current structure works, following this convention improves proto file organization and tooling compatibility.
You could restructure to:
agents/grpc/proto/grpcagent/*.protoagents/grpc/proto/reconfigure.proto (1)
18-18: Consider more inclusive terminology.The field
tracingModulesBlacklistuses terminology that modern coding guidelines recommend avoiding. Consider alternatives liketracingModulesDenylistortracingModulesBlocklistfor better alignment with inclusive language practices.agents/grpc/proto/blocked_loop.proto (1)
1-5: Remove leftover planning comments.These comments appear to be from initial design planning and reference messages (RuntimeResponse, InfoResponse, PackagesResponse) that are not defined in this file. They should be removed to avoid confusion.
Apply this diff:
-// I want to define here RuntimeResponse message which will be oneOf many other types of messages -// For example: -// - InfoResponse -// - PackagesResponse - syntax = "proto3";agents/grpc/src/grpc_utils.h (1)
28-50: Consider handling potential format string truncation.The fixed 512-byte buffer at line 40 could silently truncate long format strings. While this is a debug utility where truncation is less critical, consider using
std::ostringstreamor checking thesnprintfreturn value to detect truncation.Optional improvement:
std::string formatted_prefix; if constexpr (sizeof...(args) > 0) { char buffer[512]; - snprintf(buffer, sizeof(buffer), format, std::forward<Args>(args)...); + int written = snprintf(buffer, sizeof(buffer), format, std::forward<Args>(args)...); + if (written >= static_cast<int>(sizeof(buffer))) { + Debug("DebugProtobufMsg: format string truncated\n"); + } formatted_prefix = buffer; } else {agents/grpc/proto/command.proto (1)
1-5: Remove leftover planning comments.These comments appear to be from initial design planning and reference messages that are not defined in this file. They should be removed to maintain clarity.
Apply this diff:
-// I want to define here RuntimeResponse message which will be oneOf many other types of messages -// For example: -// - InfoResponse -// - PackagesResponse - syntax = "proto3";agents/grpc/src/grpc_agent.h (1)
293-383: LGTM - consider future decomposition.The class manages many subsystems (tracing, metrics, logging, profiling, commands) through well-organized member variables. The use of
TSQueuefor message passing and atomics for flags is appropriate.As an architectural observation, this class has significant responsibilities. In future iterations, consider decomposing into smaller, focused components (e.g., separate handlers for metrics, profiling, commands) to improve maintainability.
agents/grpc/src/grpc_agent.cc (1)
1467-1467: Consider documenting the magic number 100.The value
100appears to be a safety margin (possibly for protobuf framing overhead), but it's unexplained. Consider either adding a comment or using a named constant.+ // Reserve 100 bytes for protobuf message overhead/framing size_t chunk_size = GRPC_MAX_SIZE - rest - 100;CHANGELOG_NSOLID.md (1)
7-7: Hyphenate the compound adjective modifying the noun.The phrase "Long Term Support" should use a hyphen when modifying the changelog version: "Long-Term Support".
Apply this diff:
-* [N|Solid 5 | Node.js 20](doc/changelogs/NSOLID_CHANGELOG_V5_NODE_V20.md) **Long Term Support** +* [N|Solid 5 | Node.js 20](doc/changelogs/NSOLID_CHANGELOG_V5_NODE_V20.md) **Long-Term Support**BUILDING.md (1)
148-148: Tighten up a couple of small grammar issuesTwo tiny wording fixes to make the docs read more cleanly (non-blocking):
- Line 148: change
Xcode11→Xcode 11.- Line 845: change
a ABI incompatible version→an ABI-incompatible version(or at leastan ABI incompatible version).You can apply something along these lines:
-[^5]: Our macOS x64 Binaries are compiled with 10.15 as a target. Xcode11 is - required to compile. +[^5]: Our macOS x64 binaries are compiled with 10.15 as a target. Xcode 11 is + required to compile. - incompatible with the official N|Solid builds (e.g. using a ABI incompatible + incompatible with the official N|Solid builds (e.g. using an ABI-incompatible version of a dependency), please reserve and use a custom `NODE_MODULE_VERSION`Also applies to: 845-845
.github/workflows/test-ubsan.yml (1)
40-40: Consider an alternative toif: falseto avoid actionlint noiseThe
test-ubsanjob is intentionally disabled viaif: false, which is fine functionally but triggers the actionlint warning about a condition that is always false.If you want to keep the job parked without linter noise, consider instead:
- Commenting out the entire job block, or
- Guarding on a feature flag (e.g.
if: ${{ vars.ENABLE_UBSAN == 'true' }}) so the condition isn’t a literal constant.Purely optional; behavior is correct as-is.
.eslintrc.js (1)
16-23: ESLint/@Stylistic migration and newsubstrrestriction look good; ensure lib config stays in syncThe additions here make sense:
- Adding
@stylistic/eslint-plugin-jsto thehackslist and@stylistic/jstoplugins, plus the big@stylistic/js/*rule block, is a clean way to move style rules off core ESLint.- Switching Babel parsing support to
@babel/plugin-syntax-import-attributesmatches the newer import-attributes syntax.- The targeted overrides for
lib/internal/modules/**/*.js(curly) andlib/internal/test_runner/**/*.js(node-core/set-proto-to-null-in-object) are narrowly scoped and low-risk.- The new
no-restricted-syntaxselector forCallExpression[callee.property.name='substr']is a reasonable enforcement against the legacysubstr()API.- Adding
EventSourceandWebSockettoglobalskeeps lint noise down where those browser-ish globals are exposed.One thing to double-check: the comment above
no-restricted-syntaxnotes that changes “should apply to ./lib as well” and be mirrored intolib/.eslintrc.yaml. If you wantsubstr()banned there too, please verify that file has the same restriction added.Also applies to: 43-49, 117-132, 201-229, 271-338, 345-373
agents/.eslintrc.yaml (1)
4-258: Agent ESLint policy mirrors core primordials rules appropriatelyThis config is consistent with Node core’s internal linting profile:
- It enforces
prefer-object-spread, bansBufferconstructor, and constrains mixed&&/||operators.- The
no-restricted-syntaxandno-restricted-globalsblocks push usage through primordials and internal modules instead of ambient globals, which is exactly what you want for security‑sensitive agent code.- Custom
node-core/*rules, especiallyprefer-primordials, are wired with the usual set of built-ins and look syntactically correct in YAML.- Explicitly declaring
require,process,exports,module,internalBinding, andprimordialsas readonly globals avoidsno-undefnoise without relaxing the primordials discipline.Only tiny nit (optional): the first
no-restricted-syntaxmessage still says “Please only use simple assertions in ./lib”, which might be confusing when applied underagents/. If you care about the wording, you could rephrase it to something directory-agnostic..github/CODEOWNERS (1)
172-178: Broad/depsrule overrides earlier per‑dependency CODEOWNERS entriesBecause GitHub processes CODEOWNERS from top to bottom and the last matching pattern wins, the late entry:
/deps @nodejs/security-wgwill override earlier, more-specific rules like
/deps/cares,/deps/llhttp/*,/deps/nghttp2/*,/deps/uvwasi/,/deps/v8/*, etc. For changes under those directories, only@nodejs/security-wgwill be treated as the owner; the protocol/domain teams listed above won’t be auto‑requested anymore.If that’s not the intent, consider one of these adjustments:
- Drop the broad
/depsrule and instead add@nodejs/security-wgto each per-dependency line where you want security involved, or- Move
/deps @nodejs/security-wgup earlier in the file (before the specific/deps/...patterns) so the more-specific rules win, or- Keep
/depsbut include all relevant teams on that single line.Worth double‑checking the desired ownership behavior before merging.
CONTRIBUTING.md (1)
144-146: Optional: Fix markdown linting for command examples.Lines 144–146 use
$prompt but don't show expected output, which violates markdown linting rules (MD014). If adopting strict markdown standards, update the code block format.Consider either removing the
$prompts or adding expected output to satisfy markdown linting.README.md (1)
10-11: Capitalize "GitHub" per standard conventions.Line 11 should capitalize "GitHub" to match standard branding guidelines.
Apply this diff:
-Looking for help? You can contact us at <https://nodesource.com/> or check out -the [instructions for getting support](.github/SUPPORT.md). +Looking for help? You can contact us at <https://nodesource.com/> or check out +the [instructions for getting support](.github/SUPPORT.md).Actually, the issue is in the text itself. Consider updating:
-the [instructions for getting support](.github/SUPPORT.md). +the [instructions for getting support](.github/SUPPORT.md).Reviewing the actual line 11, there is no "github" appearing in lowercase in the visible text. This may be a false positive from the linter since the link uses
.github/directory notation..configurations/configuration.dsc.yaml (1)
14-22: Clarify intent ofuseLatest: truefor Visual Studio package.Setting
useLatest: trueon the Visual Studio 2022 Community package may introduce inconsistency in developer environments, as it will always pull the latest available version rather than pinning to a tested/stable release. For a release-focused configuration, consider removing this flag or documenting the rationale..github/workflows/create-release-proposal.yml (3)
68-71: Update bot account name to align with N|Solid branding.The git author name is set to "Node.js GitHub Bot" but should be updated to "N|Solid GitHub Bot" to be consistent with the project rebranding throughout this PR.
Apply this diff:
- name: Setup git author run: | git config --local user.email "github-bot@iojs.org" - git config --local user.name "Node.js GitHub Bot" + git config --local user.name "N|Solid GitHub Bot"
61-66: Verify GHAUTH config directory works across all runner environments.Line 63 uses
XDG_CONFIG_HOMEfor changelog-maker config, which is Unix-specific. If this workflow could run on Windows runners in the future, ensure a cross-platform alternative exists or document platform limitations.
1-3: Document required repository secrets in comments.The workflow header comments document required secrets (GH_USER_TOKEN) but could be more explicit. Ensure repository documentation clearly lists all required secrets and their purpose.
.github/workflows/coverage-linux-without-intl.yml (1)
6-32: Path filtering now explicitly tracks build sources and coverage artifacts.The workflow now uses explicit path filtering rather than broad ignores, tracking C/C++ sources, JavaScript, tests, tools, and coverage configs. This improves precision but verify that "agents/**" and "codecov.yml" changes should actually trigger this workflow.
.github/workflows/coverage-linux.yml (1)
6-32: Explicit path filtering provides better PR trigger precision.The workflow now explicitly tracks covered source files, tests, tools, and configs rather than using broad ignores. Verify that tracking "agents/**" and "codecov.yml" as triggers aligns with code coverage goals.
Makefile (1)
313-319: Optional: Reduce jstest target body complexity.Static analysis flags the
jstesttarget body (6 lines) as exceeding the recommended length of 5. While this is a minor style issue, you could extract the long command into a helper variable for better readability:JSTEST_CMD = $(PYTHON) tools/test.py $(PARALLEL_ARGS) \ --mode=$(BUILDTYPE_LOWER) $(TEST_CI_ARGS) \ --skip-tests=$(CI_SKIP_TESTS) $(JS_SUITES) $(NATIVE_SUITES) .PHONY: jstest jstest: build-addons build-js-native-api-tests build-node-api-tests test-agents-prereqs ## Runs addon tests and JS tests NSOLID_DELAY_INIT="" $(JSTEST_CMD)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (109)
.configurations/configuration.dsc.yaml(1 hunks).configurations/configuration.vsEnterprise.dsc.yaml(1 hunks).configurations/configuration.vsProfessional.dsc.yaml(1 hunks).devcontainer.json(1 hunks).eslintignore(1 hunks).eslintrc.js(8 hunks).flake8(0 hunks).github/CODEOWNERS(5 hunks).github/ISSUE_TEMPLATE/1-bug-report.yml(0 hunks).github/ISSUE_TEMPLATE/2-feature-request.yml(0 hunks).github/ISSUE_TEMPLATE/3-api-ref-docs-problem.yml(0 hunks).github/ISSUE_TEMPLATE/4-report-a-flaky-test.yml(0 hunks).github/ISSUE_TEMPLATE/config.yml(0 hunks).github/PULL_REQUEST_TEMPLATE.md(1 hunks).github/SUPPORT.md(1 hunks).github/dependabot.yml(1 hunks).github/label-pr-config.yml(6 hunks).github/workflows/auto-start-ci.yml(3 hunks).github/workflows/build-tarball.yml(4 hunks).github/workflows/build-windows.yml(0 hunks).github/workflows/close-stale-feature-requests.yml(3 hunks).github/workflows/close-stale-pull-requests.yml(1 hunks).github/workflows/close-stalled.yml(1 hunks).github/workflows/comment-labeled.yml(1 hunks).github/workflows/commit-lint.yml(1 hunks).github/workflows/commit-queue.yml(4 hunks).github/workflows/coverage-linux-without-intl.yml(1 hunks).github/workflows/coverage-linux.yml(1 hunks).github/workflows/coverage-windows.yml(3 hunks).github/workflows/create-release-proposal.yml(1 hunks).github/workflows/daily-wpt-fyi.yml(6 hunks).github/workflows/daily.yml(1 hunks).github/workflows/doc.yml(2 hunks).github/workflows/find-inactive-collaborators.yml(2 hunks).github/workflows/find-inactive-tsc.yml(2 hunks).github/workflows/label-flaky-test-issue.yml(1 hunks).github/workflows/label-pr.yml(0 hunks).github/workflows/license-builder.yml(1 hunks).github/workflows/lint-release-proposal.yml(1 hunks).github/workflows/linters.yml(4 hunks).github/workflows/major-release.yml(1 hunks).github/workflows/notify-on-push.yml(3 hunks).github/workflows/notify-on-review-wanted.yml(1 hunks).github/workflows/post-release.yml(1 hunks).github/workflows/scorecard.yml(2 hunks).github/workflows/test-asan.yml(1 hunks).github/workflows/test-internet.yml(1 hunks).github/workflows/test-linux.yml(3 hunks).github/workflows/test-macos.yml(3 hunks).github/workflows/test-ubsan.yml(1 hunks).github/workflows/timezone-update.yml(4 hunks).github/workflows/tools.yml(3 hunks).github/workflows/update-openssl.yml(2 hunks).github/workflows/update-v8.yml(1 hunks).github/workflows/update-wpt.yml(1 hunks).gitignore(4 hunks).gitpod.yml(1 hunks).mailmap(10 hunks)BUILD.gn(1 hunks)BUILDING.md(28 hunks)CHANGELOG.md(2 hunks)CHANGELOG_NSOLID.md(1 hunks)CODE_OF_CONDUCT.md(1 hunks)CONTRIBUTING.md(2 hunks)GOVERNANCE.md(6 hunks)LICENSE(8 hunks)LICENSE_NSOLID(1 hunks)Makefile(38 hunks)README.md(1 hunks)SECURITY.md(2 hunks)agents/.eslintrc.yaml(1 hunks)agents/grpc/lib/agent.js(1 hunks)agents/grpc/lib/nsolid.js(1 hunks)agents/grpc/proto/BUILD(1 hunks)agents/grpc/proto/asset.proto(1 hunks)agents/grpc/proto/blocked_loop.proto(1 hunks)agents/grpc/proto/command.proto(1 hunks)agents/grpc/proto/common.proto(1 hunks)agents/grpc/proto/exit.proto(1 hunks)agents/grpc/proto/info.proto(1 hunks)agents/grpc/proto/metrics.proto(1 hunks)agents/grpc/proto/nsolid_service.proto(1 hunks)agents/grpc/proto/packages.proto(1 hunks)agents/grpc/proto/profile.proto(1 hunks)agents/grpc/proto/reconfigure.proto(1 hunks)agents/grpc/proto/source_code.proto(1 hunks)agents/grpc/proto/startup_times.proto(1 hunks)agents/grpc/src/asset_stream.cc(1 hunks)agents/grpc/src/asset_stream.h(1 hunks)agents/grpc/src/binding.cc(1 hunks)agents/grpc/src/command_stream.cc(1 hunks)agents/grpc/src/command_stream.h(1 hunks)agents/grpc/src/grpc_agent.cc(1 hunks)agents/grpc/src/grpc_agent.h(1 hunks)agents/grpc/src/grpc_client.cc(1 hunks)agents/grpc/src/grpc_client.h(1 hunks)agents/grpc/src/grpc_errors.h(1 hunks)agents/grpc/src/grpc_utils.h(1 hunks)agents/grpc/src/proto/asset.grpc.pb.cc(1 hunks)agents/grpc/src/proto/asset.grpc.pb.h(1 hunks)agents/grpc/src/proto/asset.pb.cc(1 hunks)agents/grpc/src/proto/asset.pb.h(1 hunks)agents/grpc/src/proto/blocked_loop.grpc.pb.cc(1 hunks)agents/grpc/src/proto/blocked_loop.grpc.pb.h(1 hunks)agents/grpc/src/proto/blocked_loop.pb.cc(1 hunks)agents/grpc/src/proto/blocked_loop.pb.h(1 hunks)agents/grpc/src/proto/command.grpc.pb.cc(1 hunks)agents/grpc/src/proto/command.grpc.pb.h(1 hunks)agents/grpc/src/proto/command.pb.cc(1 hunks)
💤 Files with no reviewable changes (8)
- .github/workflows/label-pr.yml
- .flake8
- .github/workflows/build-windows.yml
- .github/ISSUE_TEMPLATE/1-bug-report.yml
- .github/ISSUE_TEMPLATE/config.yml
- .github/ISSUE_TEMPLATE/4-report-a-flaky-test.yml
- .github/ISSUE_TEMPLATE/2-feature-request.yml
- .github/ISSUE_TEMPLATE/3-api-ref-docs-problem.yml
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 0
File: :0-0
Timestamp: 2025-10-27T13:06:17.506Z
Learning: In the nodesource/nsolid repository, when reviewing PRs that add or modify V8 patches (e.g., in deps/v8/patches/), focus review comments on the patch management infrastructure (configure.py, apply-patches.sh, README.md, etc.) and not on the V8 code changes contained within the .patch files themselves, as the patch content is out of scope for the patch management PR.
📚 Learning: 2025-10-27T13:06:17.506Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 0
File: :0-0
Timestamp: 2025-10-27T13:06:17.506Z
Learning: In the nodesource/nsolid repository, when reviewing PRs that add or modify V8 patches (e.g., in deps/v8/patches/), focus review comments on the patch management infrastructure (configure.py, apply-patches.sh, README.md, etc.) and not on the V8 code changes contained within the .patch files themselves, as the patch content is out of scope for the patch management PR.
Applied to files:
README.md.github/workflows/update-v8.ymlBUILDING.md.github/PULL_REQUEST_TEMPLATE.md.github/workflows/commit-queue.yml.github/SUPPORT.mdCONTRIBUTING.md.github/CODEOWNERS.github/label-pr-config.ymlMakefile
📚 Learning: 2025-07-08T14:46:47.806Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 339
File: test/addons/nsolid-elf-utils/nsolid-elf-utils.js:13-14
Timestamp: 2025-07-08T14:46:47.806Z
Learning: In the nsolid test suite, native addon bindings are expected to be built beforehand; tests do not add fs.existsSync guards to skip when bindings are missing.
Applied to files:
BUILDING.md.github/CODEOWNERSMakefile
📚 Learning: 2025-07-08T14:47:34.724Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 339
File: test/addons/nsolid-elf-utils/nsolid-elf-utils.js:16-18
Timestamp: 2025-07-08T14:47:34.724Z
Learning: In the nsolid test suite, external tool invocations (e.g., execSync to run `readelf`) are intentionally left uncaught so that any failure causes the test to fail rather than being skipped.
Applied to files:
BUILDING.md
📚 Learning: 2025-07-08T14:48:04.827Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 339
File: test/addons/nsolid-elf-utils/binding.cc:13-28
Timestamp: 2025-07-08T14:48:04.827Z
Learning: In nsolid test native addons (e.g., `test/addons/*/binding.cc`), additional JavaScript-facing argument validation is typically omitted because the tests supply well-formed inputs.
Applied to files:
BUILDING.mdagents/grpc/src/binding.ccMakefile
📚 Learning: 2025-10-14T10:18:23.123Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 377
File: test/parallel/test-nsolid-heap-profile-stream.js:81-88
Timestamp: 2025-10-14T10:18:23.123Z
Learning: In nsolid.heapProfileStream() and nsolid.heapSamplingStream(), synchronous throws occur for argument validation errors and immediate state validation checks (like assets being disabled), while async error events are emitted for runtime errors that manifest during execution (like invalid threadId).
Applied to files:
agents/grpc/lib/agent.js
🧬 Code graph analysis (13)
agents/grpc/src/proto/blocked_loop.grpc.pb.h (7)
agents/grpc/src/proto/nsolid_service.pb.h (2)
grpcagent(65-70)grpcagent(76-344)agents/grpc/src/proto/asset.grpc.pb.h (1)
grpcagent(29-31)agents/grpc/src/proto/command.grpc.pb.h (1)
grpcagent(35-37)agents/grpc/src/proto/common.grpc.pb.h (1)
grpcagent(29-31)agents/grpc/src/proto/exit.grpc.pb.h (1)
grpcagent(29-31)agents/grpc/src/proto/info.grpc.pb.h (1)
grpcagent(29-31)agents/grpc/src/proto/metrics.grpc.pb.h (1)
grpcagent(29-31)
agents/grpc/src/grpc_utils.h (1)
src/debug_utils.h (1)
DebugCategory(63-67)
agents/grpc/src/grpc_client.cc (2)
agents/grpc/src/proto/nsolid_service.grpc.pb.h (1)
NSolidService(31-44)agents/grpc/src/proto/nsolid_service.grpc.pb.cc (2)
NewStub(40-44)NewStub(40-40)
agents/grpc/src/asset_stream.h (4)
agents/grpc/src/grpc_utils.h (3)
node(11-54)nsolid(12-53)grpc(13-52)agents/grpc/src/grpc_agent.h (1)
grpc(39-65)agents/grpc/src/asset_stream.cc (2)
AssetStream(16-39)AssetStream(41-42)agents/grpc/src/grpc_agent.cc (2)
on_asset_stream_done(985-988)on_asset_stream_done(985-985)
agents/grpc/src/asset_stream.cc (2)
agents/grpc/src/proto/nsolid_service.grpc.pb.h (1)
NSolidService(31-44)agents/grpc/src/command_stream.cc (8)
OnDone(43-63)OnDone(43-43)OnWriteDone(87-100)OnWriteDone(87-87)NextWrite(102-107)NextWrite(102-102)Write(109-113)Write(109-109)
agents/grpc/src/binding.cc (3)
agents/grpc/src/grpc_agent.cc (2)
Inst(413-418)Inst(413-413)agents/grpc/src/proto/profile.pb.h (1)
track_allocations(645-730)src/node_binding.cc (2)
RegisterExternalReferences(706-709)RegisterExternalReferences(706-706)
agents/grpc/src/command_stream.h (1)
agents/grpc/src/proto/nsolid_service.grpc.pb.h (1)
NSolidService(31-44)
agents/grpc/src/command_stream.cc (2)
agents/grpc/src/proto/nsolid_service.grpc.pb.h (1)
NSolidService(31-44)agents/grpc/src/asset_stream.cc (8)
OnDone(44-73)OnDone(44-44)OnWriteDone(75-107)OnWriteDone(75-75)NextWrite(109-130)NextWrite(109-109)Write(132-136)Write(132-132)
agents/grpc/src/proto/asset.pb.cc (1)
agents/grpc/src/proto/asset.pb.h (25)
protobuf(42-47)protobuf(64-65)protobuf(108-115)protobuf(117-119)protobuf(120-122)protobuf(123-125)Asset(91-91)Asset(92-93)Asset(381-394)Asset(395-408)Asset(452-471)Asset(474-476)Asset(483-487)Asset(488-511)Asset(512-525)Asset(569-588)Asset(591-595)Asset(641-653)Asset(656-660)Asset(665-669)Asset(670-688)Asset(689-693)grpcagent(57-62)grpcagent(68-427)default_instance(126-129)
agents/grpc/src/grpc_client.h (2)
agents/grpc/src/grpc_utils.h (4)
node(11-54)nsolid(12-53)grpc(13-52)DebugProtobufMsg(29-50)agents/grpc/src/proto/nsolid_service.pb.h (7)
grpcagent(65-70)grpcagent(76-344)EventResponse(99-99)EventResponse(100-101)EventResponse(295-299)EventResponse(345-357)string(300-337)
agents/grpc/src/grpc_agent.cc (2)
agents/otlp/src/otlp_common.cc (4)
GetResource(143-166)GetResource(143-143)UpdateResource(168-182)UpdateResource(168-168)agents/grpc/src/grpc_client.cc (2)
MakeClientContext(55-65)MakeClientContext(56-57)
agents/grpc/src/grpc_agent.h (4)
agents/grpc/src/grpc_client.h (4)
namespace(14-19)node(25-152)nsolid(26-151)grpc(27-149)agents/grpc/src/asset_stream.h (4)
node(11-85)nsolid(12-83)grpc(13-45)AssetStreamObserver(30-35)agents/grpc/src/grpc_utils.h (3)
node(11-54)nsolid(12-53)grpc(13-52)agents/grpc/src/command_stream.h (1)
CommandStreamObserver(13-19)
agents/grpc/src/proto/command.pb.cc (1)
agents/grpc/src/proto/command.pb.h (21)
protobuf(43-48)protobuf(73-74)protobuf(117-124)protobuf(126-128)protobuf(129-131)protobuf(132-134)protobuf(325-332)protobuf(334-336)protobuf(337-339)protobuf(340-342)CommandResponse(100-100)CommandResponse(101-102)CommandResponse(1334-1338)CommandResponse(1343-1347)CommandResponse(1348-1367)CommandResponse(1369-1374)CommandResponse(1408-1420)grpcagent(58-71)default_instance(135-138)default_instance(343-346)default_instance(602-605)
🪛 actionlint (1.7.9)
.github/workflows/coverage-linux.yml
51-51: condition "false" is always evaluated to false. remove the if: section
(if-cond)
.github/workflows/test-ubsan.yml
40-40: condition "false" is always evaluated to false. remove the if: section
(if-cond)
.github/workflows/coverage-linux-without-intl.yml
51-51: condition "false" is always evaluated to false. remove the if: section
(if-cond)
.github/workflows/test-asan.yml
41-41: condition "false" is always evaluated to false. remove the if: section
(if-cond)
42-42: label "ubuntu-20.04" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🪛 Buf (1.61.0)
agents/grpc/proto/common.proto
3-3: Files with package "grpcagent" must be within a directory "grpcagent" relative to root but were in directory "agents/grpc/proto".
(PACKAGE_DIRECTORY_MATCH)
agents/grpc/proto/exit.proto
3-3: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/profile.proto
5-5: Files with package "grpcagent" must be within a directory "grpcagent" relative to root but were in directory "agents/grpc/proto".
(PACKAGE_DIRECTORY_MATCH)
agents/grpc/proto/source_code.proto
3-3: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/asset.proto
3-3: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/blocked_loop.proto
8-8: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/metrics.proto
3-3: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/startup_times.proto
3-3: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/info.proto
3-3: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/packages.proto
8-8: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/command.proto
8-8: import "profile.proto": file does not exist
(COMPILE)
agents/grpc/proto/reconfigure.proto
3-3: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/nsolid_service.proto
3-3: import "asset.proto": file does not exist
(COMPILE)
🪛 checkmake (0.2.2)
Makefile
[warning] 313-313: Target body for "jstest" exceeds allowed length of 5 (6).
(maxbodylength)
🪛 LanguageTool
CODE_OF_CONDUCT.md
[style] ~27-~27: Try using a synonym here to strengthen your wording.
Context: ...vances * Trolling, insulting/derogatory comments, and personal or political attacks * Pu...
(COMMENT_REMARK)
[style] ~62-~62: ‘with regard to’ might be wordy. Consider a shorter alternative.
Context: ...s obligated to maintain confidentiality with regard to the reporter of an incident. Further de...
(EN_WORDINESS_PREMIUM_WITH_REGARD_TO)
CHANGELOG_NSOLID.md
[uncategorized] ~7-~7: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...logs/NSOLID_CHANGELOG_V5_NODE_V20.md) Long Term Support
(EN_COMPOUND_ADJECTIVE_INTERNAL)
README.md
[uncategorized] ~11-~11: The official name of this software platform is spelled with a capital “H”.
Context: ...ut the instructions for getting support. ### Download Binaries, i...
(GITHUB)
[style] ~48-~48: Consider using a more concise synonym.
Context: ...rnal libraries that are available under a variety of licenses. See LICENSE and [L...
(A_VARIETY_OF)
BUILDING.md
[grammar] ~148-~148: Ensure spelling is correct
Context: ...es are compiled with 10.15 as a target. Xcode11 is required to compile. <!--lint e...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~845-~845: Use a hyphen to join words.
Context: ...fficial N|Solid builds (e.g. using a ABI incompatible version of a dependency), p...
(QB_NEW_EN_HYPHEN)
CHANGELOG.md
[uncategorized] ~5-~5: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... 20](doc/changelogs/CHANGELOG_V20.md) Long Term Support * [Node.js 19](doc/changelogs...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~7-~7: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...js 18](doc/changelogs/CHANGELOG_V18.md) Long Term Support * [Node.js 17](doc/changelogs/C...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
GOVERNANCE.md
[style] ~173-~173: The phrase ‘have the ability to’ might be wordy. Consider using “can”.
Context: ...their judgement be trusted, and do they have the ability to work well with others. #### The Authen...
(HAS_THE_ABILITY_TO)
[style] ~181-~181: Consider a more concise word here.
Context: ...n commit access to open-source projects in order to inject malicious code or for other nefa...
(IN_ORDER_TO_PREMIUM)
[style] ~199-~199: Try using a more formal synonym here to elevate your writing.
Context: ...in the private discussion stating you're planning on opening a public issue, e.g. "I see a number...
(PLAN_ON_INTEND)
[grammar] ~237-~237: Use a hyphen to join words.
Context: ...TSC may, following typical TSC consensus seeking processes, choose to advance a n...
(QB_NEW_EN_HYPHEN)
[style] ~304-~304: This phrase is redundant. Consider using “outside”.
Context: ...ned, and avoid sharing those concerns outside of the Collaborator discussion area. Ide...
(OUTSIDE_OF)
.github/SUPPORT.md
[style] ~4-~4: Consider using a different verb to strengthen your wording.
Context: ...dress general support questions. Please make sure you are using a currently supported ver...
(MAKE_SURE_ENSURE)
CONTRIBUTING.md
[style] ~12-~12: Consider a more concise word here.
Context: .../github.com/nodesource/nsolid/issues>. In order to build this project, you will need a bui...
(IN_ORDER_TO_PREMIUM)
[grammar] ~97-~97: Use a hyphen to join words.
Context: ...TS version. For the initial N|Solid open source release, this branch was `nsolid-...
(QB_NEW_EN_HYPHEN)
[style] ~98-~98: ‘Prior to’ might be wordy. Consider a shorter alternative.
Context: ...release, this branch was nsolid-v5.x. Prior to releases, relevant changes will be port...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[style] ~216-~216: As an alternative to the over-used intensifier ‘quite’, consider replacing this phrase.
Context: ... of the types of Node.js API changes is quite wide, but it is also the least likely to lan...
(EN_WEAK_ADJECTIVE)
[style] ~222-~222: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ... zero-filling Buffers during allocation prior to the Buffer.alloc upstream Node.js API...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[grammar] ~293-~293: Use a hyphen to join words.
Context: ...MAJOR version increments in N|Solid. Pre v5.0.0 versions of N|Solid are propr...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
BUILDING.md
36-36: Link fragments should be valid
(MD051, link-fragments)
37-37: Link fragments should be valid
(MD051, link-fragments)
38-38: Link fragments should be valid
(MD051, link-fragments)
CONTRIBUTING.md
144-144: Dollar signs used before commands without showing output
(MD014, commands-show-output)
145-145: Dollar signs used before commands without showing output
(MD014, commands-show-output)
146-146: Dollar signs used before commands without showing output
(MD014, commands-show-output)
| runs-on: ubuntu-latest | ||
| # not working on gcc-8 and gcc-9 see https://github.com/nodejs/node/issues/38570 | ||
| container: gcc:11 | ||
| runs-on: ubuntu-24.04 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ubuntu-24.04 default python command symlink python3
💡 Result:
Short answer: Ubuntu 24.04 does not ship an unversioned /usr/bin/python by default — the system uses python3. If you want python -> python3 you can either install the convenience package python-is-python3 (adds /usr/bin/python -> python3) or create your own alternative/symlink or shell alias.
Commands (pick one):
-
Install the distribution convenience package:
sudo apt update
sudo apt install python-is-python3
(this creates /usr/bin/python → /usr/bin/python3). [1][2] -
Use update-alternatives (manual control):
sudo update-alternatives --install /usr/bin/python python /usr/bin/python3 10
sudo update-alternatives --config python
(then verify with python --version). [4] -
Or set a per-user alias:
echo "alias python='python3'" >> ~/.bashrc && source ~/.bashrc
Cautions:
- Changing system /usr/bin/python (or /usr/bin/python3) can break system tools that expect a specific Python version; prefer python-is-python3 or a user-level alias, or add /usr/local/bin symlinks (which take precedence) rather than editing system-managed symlinks. [5][4]
Sources:
[1] Daniel Kossmann — Run python3 as python in Ubuntu 24.04.
[2] Launchpad package page — python-is-python3 for Ubuntu 24.04.
[4] Guides on using update-alternatives and risks (examples for Ubuntu).
[5] ServerFault/StackOverflow discussion about symlinks, update-alternatives and system breakage.
🌐 Web query:
ubuntu-24.04 gcc version default installed
💡 Result:
Ubuntu 24.04 LTS (Noble) uses GCC 13 as the distribution default (the gcc package points at GCC 13; GCC 14 is available in the archives). [1][2]
Sources:
- Ubuntu for Developers — Available GCC versions (shows gcc-defaults = 13 for 24.04). [1]
- Package info for gcc in 24.04 (gcc = 4:13.2.0-7ubuntu1). [2]
🌐 Web query:
Node.js configure script python requirement
💡 Result:
Short answer: The Node.js ./configure/build requires a "supported" Python — modern Node builds use Python 3 (not Python 2), but the exact acceptable minor versions depend on the Node release you are building. See Node's BUILDING.md which says "./configure" expects a supported Python on PATH, and node-gyp (used by native builds) documents how to point to a specific python executable. If you hit "Please use python3.x or python2.7" errors it's because that particular Node release's configure script only accepts a limited set of Python minors (older releases checked specific 3.x tuples). [1][2][3]
Recommendations:
- Use a recent Python 3 (match the Node release's BUILDING.md or configure script). [1]
- If needed, set npm_config_python or pass node-gyp/./configure the explicit python path. [2]
- For older Node versions, you may need to install a specific Python minor (e.g. 3.10/3.9 or 2.7) because their configure script only recognizes those. [3]
Sources:
- Node.js BUILDING.md (prerequisites / Python). [1]
- node-gyp docs (configuring which Python to use). [2]
- Node.js GitHub issue demonstrating older configure scripts restricting accepted Python minors. [3]
🏁 Script executed:
# Check Node.js build documentation for python and gcc requirements
# Look at the configure script expectations
cd . && find . -name "configure" -type f 2>/dev/null | head -5Repository: nodesource/nsolid
Length of output: 147
The removal of python-is-python3 will cause build failures on ubuntu-24.04.
Ubuntu 24.04 does not provide an unversioned python command by default—only python3 exists. Removing python-is-python3 means Node.js ./configure and other build scripts expecting the python command will fail.
Additionally:
- GCC version: Ubuntu 24.04 provides GCC 13 by default (newer than the removed GCC 11 container), which should be compatible with LTO builds but may have different behavior.
- Runner pinning: Pinning to
ubuntu-24.04prevents automatic updates; manual maintenance will be required when 24.04 reaches EOL.
Either reinstall python-is-python3 in the workflow, explicitly call python3 in build scripts, or create the symlink manually with update-alternatives.
🤖 Prompt for AI Agents
.github/workflows/daily.yml around line 16: removing the python-is-python3
package breaks builds on ubuntu-24.04 because the unversioned python binary is
absent; fix by either reinstalling python-is-python3 in the workflow before any
build steps, or update build scripts to call python3 instead of python, or
create a /usr/bin/python symlink via update-alternatives in a setup step; also
be aware you pinned ubuntu-24.04 so verify GCC 13 compatibility for LTO and plan
manual runner updates when 24.04 reaches EOL.
| jobs: | ||
| notifyOnReviewWanted: | ||
| name: Notify on Review Wanted | ||
| if: github.repository == 'nodejs/node' && github.event.label.name == 'review wanted' |
There was a problem hiding this comment.
Repository check hardcoded to nodejs/node will prevent this workflow from triggering.
Line 14 checks github.repository == 'nodejs/node', but this workflow is being added to nodesource/nsolid. The condition will never evaluate to true.
Update the repository check:
- if: github.repository == 'nodejs/node' && github.event.label.name == 'review wanted'
+ if: github.repository == 'nodesource/nsolid' && github.event.label.name == 'review wanted'📝 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.
| if: github.repository == 'nodejs/node' && github.event.label.name == 'review wanted' | |
| if: github.repository == 'nodesource/nsolid' && github.event.label.name == 'review wanted' |
🤖 Prompt for AI Agents
.github/workflows/notify-on-review-wanted.yml around line 14: the condition
currently checks for the hardcoded repository 'nodejs/node' which prevents the
workflow from running in nodesource/nsolid; update the repository check to match
this repository by replacing 'nodejs/node' with 'nodesource/nsolid' (or remove
the repository check entirely if the workflow should run across forks) so the
condition can evaluate to true for this repo.
| if: false # Temporarily disabled. References: https://github.com/nodejs/node/pull/52374, https://github.com/nodejs/node/pull/53651#issuecomment-2198510810 | ||
| runs-on: ubuntu-20.04 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the test-asan.yml file
cat -n .github/workflows/test-asan.yml | head -60Repository: nodesource/nsolid
Length of output: 2082
🏁 Script executed:
# Check what other workflow files exist and their runner specifications
find .github/workflows -name "*.yml" -o -name "*.yaml" | head -20Repository: nodesource/nsolid
Length of output: 855
🏁 Script executed:
# Search for ubuntu runner versions used across workflows
rg "runs-on:" .github/workflows/ -A 1Repository: nodesource/nsolid
Length of output: 5616
🌐 Web query:
GitHub Actions ubuntu-20.04 deprecated removed 2024
💡 Result:
Short answer: Ubuntu‑20.04 was not removed in 2024 — GitHub announced deprecation starting Feb 1, 2025 with brownouts in March 2025 and the hosted ubuntu-20.04 image fully retired in April 2025 (recommend switching to ubuntu-22.04 or ubuntu-24.04). [1][2]
Sources:
- GitHub Changelog: ubuntu‑20 image closing down (retire date April 15, 2025). [1]
- actions/runner-images issue announcing deprecation starting Feb 1, 2025 and brownouts. [2]
Update the runner label before re-enabling this workflow.
Ubuntu-20.04 has been retired from GitHub-hosted runners as of April 15, 2025. While the job is currently disabled with if: false, updating runs-on: ubuntu-20.04 to ubuntu-22.04 or ubuntu-24.04 is essential before this workflow can be re-enabled (the runner will not be available when the condition is removed). Other workflows in this repository use ubuntu-24.04, so that is the recommended target.
🧰 Tools
🪛 actionlint (1.7.9)
41-41: condition "false" is always evaluated to false. remove the if: section
(if-cond)
42-42: label "ubuntu-20.04" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🤖 Prompt for AI Agents
.github/workflows/test-asan.yml around lines 41 to 42: the workflow uses a
retired runner label `runs-on: ubuntu-20.04` and must be updated before
re-enabling; change the runner label to a supported image (preferably
`ubuntu-24.04` to match other workflows) so the job will run when `if: false` is
removed, and verify no job-specific actions require an older OS version.
| ./tools/dep_updaters/update-acorn.sh > temp-output | ||
| cat temp-output | ||
| tail -n1 temp-output | grep "NEW_VERSION=" >> "$GITHUB_ENV" || true | ||
| rm temp-output | ||
| - id: acorn-walk | ||
| subsystem: deps | ||
| label: dependencies | ||
| run: | | ||
| ./tools/dep_updaters/update-acorn-walk.sh > temp-output | ||
| cat temp-output | ||
| tail -n1 temp-output | grep "NEW_VERSION=" >> "$GITHUB_ENV" || true | ||
| rm temp-output | ||
| - id: ada | ||
| subsystem: deps | ||
| label: dependencies | ||
| run: | | ||
| ./tools/dep_updaters/update-ada.sh > temp-output | ||
| cat temp-output | ||
| tail -n1 temp-output | grep "NEW_VERSION=" >> "$GITHUB_ENV" || true | ||
| rm temp-output | ||
| - id: brotli | ||
| subsystem: deps | ||
| label: dependencies | ||
| run: | | ||
| ./tools/dep_updaters/update-brotli.sh > temp-output | ||
| cat temp-output | ||
| tail -n1 temp-output | grep "NEW_VERSION=" >> "$GITHUB_ENV" || true | ||
| rm temp-output |
There was a problem hiding this comment.
Improve NEW_VERSION extraction robustness.
The pattern tail -n1 temp-output | grep "NEW_VERSION=" >> "$GITHUB_ENV" || true silently masks failures. If the script doesn't output NEW_VERSION=<value> on the last line, the grep fails and nothing is appended to $GITHUB_ENV. The || true converts the error to success, allowing the workflow to continue with an undefined NEW_VERSION, resulting in PR titles like "deps: update acorn to " (empty version).
Improve robustness by:
- Checking whether NEW_VERSION was set after extraction
- Failing the step if the extraction was unsuccessful
Apply this pattern to all affected jobs (acorn, acorn-walk, ada, brotli, c-ares, cjs-module-lexer, eslint, github_reporter, googletest, gyp-next, histogram, icu, llhttp, minimatch, nghttp2, nghttp3, ngtcp2, simdutf, undici, uvwasi, zlib):
./tools/dep_updaters/update-acorn.sh > temp-output
cat temp-output
- tail -n1 temp-output | grep "NEW_VERSION=" >> "$GITHUB_ENV" || true
+ NEW_VERSION_LINE=$(tail -n1 temp-output | grep "NEW_VERSION=" || echo "")
+ if [ -z "$NEW_VERSION_LINE" ]; then
+ echo "ERROR: Failed to extract NEW_VERSION from update script output"
+ cat temp-output
+ exit 1
+ fi
+ echo "$NEW_VERSION_LINE" >> "$GITHUB_ENV"
rm temp-outputAlternatively, require that all update-*.sh scripts output NEW_VERSION=<version> on the last line consistently, then verify this in tests.
📝 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.
| ./tools/dep_updaters/update-acorn.sh > temp-output | |
| cat temp-output | |
| tail -n1 temp-output | grep "NEW_VERSION=" >> "$GITHUB_ENV" || true | |
| rm temp-output | |
| - id: acorn-walk | |
| subsystem: deps | |
| label: dependencies | |
| run: | | |
| ./tools/dep_updaters/update-acorn-walk.sh > temp-output | |
| cat temp-output | |
| tail -n1 temp-output | grep "NEW_VERSION=" >> "$GITHUB_ENV" || true | |
| rm temp-output | |
| - id: ada | |
| subsystem: deps | |
| label: dependencies | |
| run: | | |
| ./tools/dep_updaters/update-ada.sh > temp-output | |
| cat temp-output | |
| tail -n1 temp-output | grep "NEW_VERSION=" >> "$GITHUB_ENV" || true | |
| rm temp-output | |
| - id: brotli | |
| subsystem: deps | |
| label: dependencies | |
| run: | | |
| ./tools/dep_updaters/update-brotli.sh > temp-output | |
| cat temp-output | |
| tail -n1 temp-output | grep "NEW_VERSION=" >> "$GITHUB_ENV" || true | |
| rm temp-output | |
| ./tools/dep_updaters/update-acorn.sh > temp-output | |
| cat temp-output | |
| NEW_VERSION_LINE=$(tail -n1 temp-output | grep "NEW_VERSION=" || echo "") | |
| if [ -z "$NEW_VERSION_LINE" ]; then | |
| echo "ERROR: Failed to extract NEW_VERSION from update script output" | |
| cat temp-output | |
| exit 1 | |
| fi | |
| echo "$NEW_VERSION_LINE" >> "$GITHUB_ENV" | |
| rm temp-output |
🤖 Prompt for AI Agents
.github/workflows/tools.yml lines 62-89: the current NEW_VERSION extraction uses
`tail -n1 temp-output | grep "NEW_VERSION=" >> "$GITHUB_ENV" || true`, which
hides failures and can leave NEW_VERSION empty; update each job's run block
(acorn, acorn-walk, ada, brotli and the other listed jobs) to explicitly capture
the last line into a variable, validate it matches the `NEW_VERSION=` pattern,
write it to $GITHUB_ENV only if valid, and exit with a non-zero status if not;
specifically, replace the tail|grep...||true pipeline with something like: read
last_line < <(tail -n1 temp-output); if [[ $last_line =~ ^NEW_VERSION=.+$ ]];
then echo "$last_line" >> "$GITHUB_ENV"; else cat temp-output; echo "Failed to
extract NEW_VERSION from update-*.sh output" >&2; exit 1; fi — apply this change
to all affected jobs to ensure the workflow fails when extraction fails.
| - id: doc | ||
| subsystem: tools | ||
| label: tools | ||
| run: | | ||
| cd tools/lint-md | ||
| cd tools/doc | ||
| npm ci | ||
| NEW_VERSION=$(npm outdated --parseable | cut -d: -f4 | xargs) | ||
| if [ "$NEW_VERSION" != "" ]; then | ||
| echo "NEW_VERSION=$NEW_VERSION" >> $GITHUB_ENV | ||
| echo "NEW_VERSION=new version" >> $GITHUB_ENV | ||
| rm -rf package-lock.json node_modules | ||
| # Include $NEW_VERSION to explicitly update the package.json | ||
| # entry for the dependency and also so that semver-major updates | ||
| # are not skipped. | ||
| npm install --ignore-scripts $NEW_VERSION | ||
| npm install --ignore-scripts | ||
| cd ../.. | ||
| make lint-md-rollup | ||
| fi |
There was a problem hiding this comment.
Fix hardcoded "new version" string in doc update step.
Line 120 contains a hardcoded placeholder string instead of the actual package version. This will result in PR titles and bodies showing "new version" instead of the real version number, breaking the workflow's communicativeness.
The version extraction logic should capture the actual NEW_VERSION. Based on the pattern used for other tooling updates (lines 132-135), apply this diff:
- id: doc
subsystem: tools
label: tools
run: |
cd tools/doc
npm ci
NEW_VERSION=$(npm outdated --parseable | cut -d: -f4 | xargs)
if [ "$NEW_VERSION" != "" ]; then
- echo "NEW_VERSION=new version" >> $GITHUB_ENV
+ echo "NEW_VERSION=$NEW_VERSION" >> $GITHUB_ENV
rm -rf package-lock.json node_modules
# Include $NEW_VERSION to explicitly update the package.json
# entry for the dependency and also so that semver-major updates
# are not skipped.
npm install --ignore-scripts $NEW_VERSION
npm install --ignore-scripts
fi📝 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.
| - id: doc | |
| subsystem: tools | |
| label: tools | |
| run: | | |
| cd tools/lint-md | |
| cd tools/doc | |
| npm ci | |
| NEW_VERSION=$(npm outdated --parseable | cut -d: -f4 | xargs) | |
| if [ "$NEW_VERSION" != "" ]; then | |
| echo "NEW_VERSION=$NEW_VERSION" >> $GITHUB_ENV | |
| echo "NEW_VERSION=new version" >> $GITHUB_ENV | |
| rm -rf package-lock.json node_modules | |
| # Include $NEW_VERSION to explicitly update the package.json | |
| # entry for the dependency and also so that semver-major updates | |
| # are not skipped. | |
| npm install --ignore-scripts $NEW_VERSION | |
| npm install --ignore-scripts | |
| cd ../.. | |
| make lint-md-rollup | |
| fi | |
| - id: doc | |
| subsystem: tools | |
| label: tools | |
| run: | | |
| cd tools/doc | |
| npm ci | |
| NEW_VERSION=$(npm outdated --parseable | cut -d: -f4 | xargs) | |
| if [ "$NEW_VERSION" != "" ]; then | |
| echo "NEW_VERSION=$NEW_VERSION" >> $GITHUB_ENV | |
| rm -rf package-lock.json node_modules | |
| # Include $NEW_VERSION to explicitly update the package.json | |
| # entry for the dependency and also so that semver-major updates | |
| # are not skipped. | |
| npm install --ignore-scripts $NEW_VERSION | |
| npm install --ignore-scripts | |
| fi |
🤖 Prompt for AI Agents
.github/workflows/tools.yml around lines 112 to 127: the workflow currently
writes a hardcoded placeholder ("new version") to GITHUB_ENV instead of the
actual version string, so change the echo to export the real NEW_VERSION value
(e.g. echo "NEW_VERSION=$NEW_VERSION" >> $GITHUB_ENV), ensuring NEW_VERSION is
trimmed/quoted if necessary; keep the subsequent rm -rf and npm install steps
but pass $NEW_VERSION as the package spec (npm install --ignore-scripts
"$NEW_VERSION") so the package.json gets updated with the real version.
| QueueCallback([](WeakGrpcAgent agent_wp) { | ||
| SharedGrpcAgent agent = agent_wp.lock(); | ||
| if (agent == nullptr) { | ||
| return; | ||
| } | ||
|
|
||
| agent->stop(); | ||
| delete agent.get(); | ||
| }, weak_from_this()); |
There was a problem hiding this comment.
Critical: Undefined behavior - calling delete on shared_ptr's managed object.
Line 516 calls delete agent.get() on an object managed by SharedGrpcAgent (which is a std::shared_ptr). This causes undefined behavior:
- The raw pointer is deleted manually
- When the
shared_ptrdestructor runs, it will attempt to delete the same memory again (double-free)
If the intent is to force cleanup after auth failure, consider using a dedicated shutdown mechanism rather than manual deletion.
QueueCallback([](WeakGrpcAgent agent_wp) {
SharedGrpcAgent agent = agent_wp.lock();
if (agent == nullptr) {
return;
}
agent->stop();
- delete agent.get();
}, weak_from_this());📝 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.
| QueueCallback([](WeakGrpcAgent agent_wp) { | |
| SharedGrpcAgent agent = agent_wp.lock(); | |
| if (agent == nullptr) { | |
| return; | |
| } | |
| agent->stop(); | |
| delete agent.get(); | |
| }, weak_from_this()); | |
| QueueCallback([](WeakGrpcAgent agent_wp) { | |
| SharedGrpcAgent agent = agent_wp.lock(); | |
| if (agent == nullptr) { | |
| return; | |
| } | |
| agent->stop(); | |
| }, weak_from_this()); |
🤖 Prompt for AI Agents
In agents/grpc/src/grpc_agent.cc around lines 509 to 517, remove the manual
delete(agent.get()) call (which causes undefined behavior/double-free). Instead,
call agent->stop(); then release the local shared_ptr safely (e.g.,
agent.reset()) or signal the owning container to remove its shared_ptr so the
object can be destroyed normally; do not call delete on the managed raw pointer
— implement or invoke a proper shutdown/cleanup mechanism to ensure safe
destruction.
| bool insecure = false; | ||
| std::string insecure_str; | ||
| bool insecure_set = per_process::system_environment-> | ||
| Get(kNSOLID_GRPC_INSECURE).To(&insecure_str); | ||
| // Only parse the insecure flag in non SaaS mode. | ||
| if (insecure_set && (!saas_ || saas_->testing)) { | ||
| insecure = std::stoi(insecure_str); | ||
| } |
There was a problem hiding this comment.
Add exception handling for std::stoi parsing.
std::stoi can throw std::invalid_argument or std::out_of_range if the environment variable contains an invalid value. Consider wrapping in try-catch or using a safer alternative.
bool insecure_set = per_process::system_environment->
Get(kNSOLID_GRPC_INSECURE).To(&insecure_str);
// Only parse the insecure flag in non SaaS mode.
if (insecure_set && (!saas_ || saas_->testing)) {
- insecure = std::stoi(insecure_str);
+ try {
+ insecure = std::stoi(insecure_str) != 0;
+ } catch (const std::exception& e) {
+ Debug("Invalid NSOLID_GRPC_INSECURE value: %s\n", insecure_str.c_str());
+ }
}🤖 Prompt for AI Agents
In agents/grpc/src/grpc_agent.cc around lines 1020 to 1027, the call to
std::stoi on the environment string can throw std::invalid_argument or
std::out_of_range; wrap the stoi call in a try-catch that catches those
exceptions, set insecure to false on parse failure (or leave previous value),
and emit a warning/log entry including the invalid value so we don't crash on
bad env vars; alternatively, use a non-throwing parser (e.g., std::from_chars)
to parse the integer and handle parse failure the same way.
| #ifndef AGENTS_GRPC_SRC_GRPC_ERRORS_H_ | ||
| #define AGENTS_GRPC_SRC_GRPC_ERRORS_H_ | ||
|
|
There was a problem hiding this comment.
Missing standard library includes.
The header uses std::string and uint32_t but doesn't include the required headers. This will cause compilation errors when this header is included before other headers that happen to pull in these types.
#ifndef AGENTS_GRPC_SRC_GRPC_ERRORS_H_
#define AGENTS_GRPC_SRC_GRPC_ERRORS_H_
+#include <cstdint>
+#include <string>
+
#define GRPC_ERRORS(X) \📝 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.
| #ifndef AGENTS_GRPC_SRC_GRPC_ERRORS_H_ | |
| #define AGENTS_GRPC_SRC_GRPC_ERRORS_H_ | |
| #ifndef AGENTS_GRPC_SRC_GRPC_ERRORS_H_ | |
| #define AGENTS_GRPC_SRC_GRPC_ERRORS_H_ | |
| #include <cstdint> | |
| #include <string> | |
🤖 Prompt for AI Agents
In agents/grpc/src/grpc_errors.h around lines 1 to 3, the header declares types
that use std::string and uint32_t but does not include the standard headers; add
#include <string> and #include <cstdint> (placed after the include guard) so the
file compiles standalone regardless of include order.
| #### How to review a collaborator nomination | ||
|
|
||
| A collaborator nomination can be reviewed in the same way one would review a PR | ||
| adding a feature: | ||
|
|
||
| * If you see the nomination as something positive to the project, say so! | ||
| * If you are neutral, or feel you don't know enough to have an informed opinion, | ||
| it's certainly OK to not interact with the nomination. | ||
| * If you think the nomination was made too soon, or can be detrimental to the | ||
| project, share your concerns. See the section "How to oppose a collaborator | ||
| nomination" below. |
There was a problem hiding this comment.
Remove duplicate "How to review a collaborator nomination" section.
The section "How to review a collaborator nomination" appears twice (lines 243–253 and 294–309) with nearly identical content and slightly different wording. This appears to be unintentional duplication.
Remove the second occurrence (lines 294–310) and retain the first. If there are intentional differences, consolidate them into a single section.
-#### How to review a collaborator nomination
-
-A collaborator nomination can be reviewed in the same way one would review a PR
-adding a feature:
-
-* If you see the nomination as something positive to the project, say so!
-* If you are neutral, or feel you don't know enough to have an informed opinion,
- it's certainly OK to not interact with the nomination.
-* If you think the nomination was made too soon, or can be detrimental to the
- project, share your concerns, ideally before the public nomination is opened,
- and avoid sharing those concerns outside of the Collaborator discussion area.
- Ideally, list what step(s) the nominee could take that would make you
- approve their nomination.
- Given that there is no "Request for changes" feature in discussions and issues,
- try to be explicit when your comment is expressing a blocking concern.
- Similarly, once the blocking concern has been addressed, explicitly say so.
-
-Our goal is to keep gate-keeping at a minimal, but it cannot be zero since being
-a collaborator requires trust (collaborators can start CI jobs, use their veto,
-push commits, etc.), so what's the minimal amount is subjective, and there will
-be cases where collaborators disagree on whether a nomination should move
-forward.
-
-When concerns have been raised on the private discussion, refrain from opening
-the public issue. If no one has explicitly blocked the nomination and you'd like
-it to move forward, comment something like "If I don't hear any objections
-before (some time), I will open the public issue".Also applies to: 294-310
🤖 Prompt for AI Agents
In GOVERNANCE.md around lines 243-253 and 294-310, there is a duplicated "How to
review a collaborator nomination" section; remove the second occurrence (lines
294-310) and keep the first (243-253). If the later block contains any unique
wording or details not present in the first, merge those unique lines into the
retained section before deleting the duplicate, then update surrounding
numbering/spacing so the document remains consistent.
| .PHONY: test-agents-prereqs | ||
| test-agents-prereqs: | ||
| env npm_config_nodedir=$(PWD) $(NODE) ./deps/npm install zeromq@5 base85 --prefix test/common/nsolid-zmq-agent --no-save --no-package-lock | ||
| env npm_config_nodedir=$(PWD) $(NODE) ./deps/npm run build:libzmq --prefix test/common/nsolid-zmq-agent/node_modules/zeromq | ||
| env npm_config_nodedir=$(PWD) $(NODE) ./deps/npm install @opentelemetry/otlp-proto-exporter-base @grpc/grpc-js @grpc/proto-loader --prefix test/common/nsolid-otlp-agent --no-save --no-package-lock | ||
| env npm_config_nodedir=$(PWD) $(NODE) ./deps/npm install @grpc/grpc-js @grpc/proto-loader --prefix test/common/nsolid-grpc-agent --no-save --no-package-lock | ||
|
|
||
| .PHONY: test-agents-prereqs-clean | ||
| test-agents-prereqs-clean: | ||
| $(RM) -r test/common/nsolid-zmq-agent/node_modules | ||
| $(RM) -r test/common/nsolid-otlp-agent/node_modules | ||
|
|
There was a problem hiding this comment.
Missing cleanup for grpc agent in test-agents-prereqs-clean target.
Line 1648 installs @grpc/grpc-js and @grpc/proto-loader to test/common/nsolid-grpc-agent/node_modules, but the cleanup target (lines 1650–1653) does not remove it. This creates asymmetry and allows stale dependencies to accumulate.
Apply this diff to fix the cleanup:
.PHONY: test-agents-prereqs-clean
test-agents-prereqs-clean:
$(RM) -r test/common/nsolid-zmq-agent/node_modules
$(RM) -r test/common/nsolid-otlp-agent/node_modules
+ $(RM) -r test/common/nsolid-grpc-agent/node_modules🤖 Prompt for AI Agents
In Makefile around lines 1643 to 1654, the test-agents-prereqs target installs
dependencies into test/common/nsolid-grpc-agent/node_modules but the
test-agents-prereqs-clean target does not remove that directory; update the
cleanup target to also remove test/common/nsolid-grpc-agent/node_modules (add a
$(RM) -r test/common/nsolid-grpc-agent/node_modules entry) so all installed
agent node_modules are cleaned symmetrically.
santigimeno
changed the base branch from
node-v24.x-nsolid-v6.x
to
node-v20.x-nsolid-v6.x
20 participants
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.