Skip to content

Node v18.20.8 nsolid v5.7.1 release#306

Merged
santigimeno merged 84 commits intonode-v18.x-nsolid-v5.xfrom
node-v18.20.8-nsolid-v5.7.1-release
May 12, 2025
Merged

Node v18.20.8 nsolid v5.7.1 release#306
santigimeno merged 84 commits intonode-v18.x-nsolid-v5.xfrom
node-v18.20.8-nsolid-v5.7.1-release

Conversation

@santigimeno
Copy link
Copy Markdown
Member

@santigimeno santigimeno commented May 7, 2025

Summary by CodeRabbit

  • New Features

    • Introduced a comprehensive gRPC agent for N|Solid, enabling advanced profiling, metrics, tracing, logging, and configuration management through a new asynchronous gRPC-based architecture.
    • Added support for OpenTelemetry integration, allowing export of traces, metrics, and logs over gRPC.
    • Implemented new profiling capabilities, including CPU, heap, heap sampling, and heap snapshot operations accessible via gRPC.
    • Added new Protocol Buffers and gRPC service definitions for structured communication between agents and the N|Solid Console.
    • Provided new ESLint configuration for agent code to enforce strict coding standards.
  • Documentation

    • Updated and added documentation files, including a new changelog and license for N|Solid, and revised build, contribution, governance, and security guides to reflect N|Solid-specific processes and branding.
  • Chores

    • Modernized and expanded CI workflows, added new workflows for dependency and timezone updates, and improved automation for labeling, notifications, and stale issue management.
    • Updated repository metadata, including mailmap, .gitignore, and .gitattributes for improved contributor mapping and file handling.
  • Style

    • Standardized and updated linting configurations for JavaScript, Python, and YAML files.
  • Refactor

    • Migrated build and test infrastructure to N|Solid-specific processes, including new Makefile targets and build scripts tailored for agent prerequisites and test automation.
  • Tests

    • Enhanced test prerequisites and automation for agent-related dependencies and profiling features.

This release brings major enhancements to N|Solid’s observability, automation, and development workflows, with a focus on robust gRPC-based agent communication and improved contributor experience.

RafaelGSS and others added 30 commits January 21, 2025 09:22
PR-URL: nodejs/node#53190
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Refs: https://github.com/actions/upload-artifact/releases/tag/v4.0.0
Refs: https://github.com/actions/download-artifact/releases/tag/v4.1.0
PR-URL: nodejs/node#51219
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 4.0.0 to 4.3.0.
- [Release notes](https://github.com/actions/upload-artifact/releases)
- [Commits](actions/upload-artifact@c7d193f...26f96df)

---
updated-dependencies:
- dependency-name: actions/upload-artifact
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
PR-URL: nodejs/node#51643
Refs: actions/upload-artifact@26f96df
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Bumps [actions/download-artifact](https://github.com/actions/download-artifact) from 4.1.0 to 4.1.1.
- [Release notes](https://github.com/actions/download-artifact/releases)
- [Commits](actions/download-artifact@f44cd7b...6b208ae)

---
updated-dependencies:
- dependency-name: actions/download-artifact
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
PR-URL: nodejs/node#51644
Refs: actions/download-artifact@6b208ae
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Bumps [actions/download-artifact](https://github.com/actions/download-artifact) from 4.1.1 to 4.1.3.
- [Release notes](https://github.com/actions/download-artifact/releases)
- [Commits](actions/download-artifact@6b208ae...87c5514)

---
updated-dependencies:
- dependency-name: actions/download-artifact
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
PR-URL: nodejs/node#51938
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 4.3.0 to 4.3.1.
- [Release notes](https://github.com/actions/upload-artifact/releases)
- [Commits](actions/upload-artifact@26f96df...5d5d22a)

---
updated-dependencies:
- dependency-name: actions/upload-artifact
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
PR-URL: nodejs/node#51941
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Bumps [actions/download-artifact](https://github.com/actions/download-artifact) from 4.1.3 to 4.1.4.
- [Release notes](https://github.com/actions/download-artifact/releases)
- [Commits](actions/download-artifact@87c5514...c850b93)

---
updated-dependencies:
- dependency-name: actions/download-artifact
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
PR-URL: nodejs/node#52314
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Bumps [actions/download-artifact](https://github.com/actions/download-artifact) from 4.1.4 to 4.1.7.
- [Release notes](https://github.com/actions/download-artifact/releases)
- [Commits](actions/download-artifact@c850b93...65a9edc)

---
updated-dependencies:
- dependency-name: actions/download-artifact
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
PR-URL: nodejs/node#52784
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 4.3.1 to 4.3.3.
- [Release notes](https://github.com/actions/upload-artifact/releases)
- [Commits](actions/upload-artifact@5d5d22a...6546280)

---
updated-dependencies:
- dependency-name: actions/upload-artifact
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
PR-URL: nodejs/node#52785
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Bumps [actions/download-artifact](https://github.com/actions/download-artifact) from 4.1.7 to 4.1.8.
- [Release notes](https://github.com/actions/download-artifact/releases)
- [Commits](actions/download-artifact@65a9edc...fa0a91b)

---
updated-dependencies:
- dependency-name: actions/download-artifact
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
PR-URL: nodejs/node#54167
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 4.3.3 to 4.3.4.
- [Release notes](https://github.com/actions/upload-artifact/releases)
- [Commits](actions/upload-artifact@6546280...0b2256b)

---
updated-dependencies:
- dependency-name: actions/upload-artifact
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
PR-URL: nodejs/node#54166
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 4.3.4 to 4.4.0.
- [Release notes](https://github.com/actions/upload-artifact/releases)
- [Commits](actions/upload-artifact@0b2256b...5076954)

---
updated-dependencies:
- dependency-name: actions/upload-artifact
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
PR-URL: nodejs/node#54703
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 4.4.0 to 4.4.3.
- [Release notes](https://github.com/actions/upload-artifact/releases)
- [Commits](actions/upload-artifact@5076954...b4b15b8)

---
updated-dependencies:
- dependency-name: actions/upload-artifact
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
PR-URL: nodejs/node#55685
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Signed-off-by: dependabot[bot] <support@github.com>
PR-URL: nodejs/node#56861
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs/node#55855
Backport-PR-URL: nodejs/node#55962
Refs: nodejs/node#55333
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
PR-URL: nodejs/node#55977
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs/node#56795
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
PR-URL: nodejs/node#55699
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs/node#55850
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs/node#55889
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
PR-URL: nodejs/node#55973
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
PR-URL: nodejs/node#56255
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs/node#54432
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
PR-URL: nodejs/node#55980
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
PR-URL: nodejs/node#56995
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Previously when checking the initial timing we did a lot of checks
after accessing and copying timing.duration and before we check
that timing.duration is roughly the same as performance.now(),
which can lead to flakes if the overhead of the checking is
big enough. Update the test to check timing.duration against
performance.now() as soon as possible when it's copied instead
of computed.
:#

PR-URL: nodejs/node#49892
Refs: nodejs/reliability#676
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Richard Lau <rlau@redhat.com>
PR-URL: nodejs/node#53993
Fixes: nodejs/node#53989
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
The `out/Makefile` target in `Makefile` has an incomplete list of
`.gyp` files for Node.js dependencies in `deps`, but also the ones
that are listed are unconditional. If using any of the `--shared-*`
configure options, it should be possible to still build Node.js if
the corresponding directory under `deps` is removed.

Convert the explicit list of dependency `*.gyp` files for the
`out/Makefile` target to a glob. This will pick up any toplevel
`.gyp` files for dependencies present in `deps`.

PR-URL: nodejs/node#55789
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
OpenSSL 3.4 has a breaking change where the outputLength is now
mandatory for shake* hash algorithms.

openssl/openssl@b911fef
PR-URL: nodejs/node#56160
Refs: nodejs/node#56159
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
santigimeno added 19 commits May 7, 2025 11:36
No need to subscribe to the `TracingChannels` directly, but calling
`bindStore()` it's enough as it sets the channel as `ActiveChannel`.

PR-URL: #276
Fixes: nodesource/nsolid-roadmap#26
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Fixes: nodesource/nsolid-roadmap#26
PR-URL: #276
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
As reported by abseil-cpp. By the looks of it, it's fixed if we make
sure `CommandStream` instnace is created before the `OtlpGrpcClient`
instance.

Fixes: nodesource/nsolid-roadmap#28
PR-URL: #277
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Helper class that wraps a TSQueue and a uv_async_t handle which it's a
common pattern used throughout the code to send data into a thread loop.

PR-URL: #279
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
As a first example on how to use AsyncTSQueue.

PR-URL: #279
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Stop using synchronous exporting as it may block for a substantial
period of time on network disconnections.

PR-URL: #278
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
PR-URL: #286
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
PR-URL: #180
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
PR-URL: #286
PR-URL: #288
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
PR-URL: #280
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
After having enabled `ENABLE_ASYNC_EXPORT` in `opentelemetry-cpp`.

PR-URL: #281
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Replace manual message handling with AsyncTSQueue for profile data.
Remove initialize() method as initialization is now handled in
constructor.
Simplify profile callback implementation.
Remove process_profiles() as AsyncTSQueue handles this automatically.
Update ZMQ and gRPC agents to remove initialize() calls.

PR-URL: #282
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Which was overlooked in the initial implementation.
Add tests covering the case.

Fixes: nodesource/nsolid-roadmap#29
PR-URL: #293
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
It needs the abseil dependency to successfully build it. It was working
locally for me because I had that dependency already installed globally
in my dev box.

PR-URL: #291
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Specifically the ones retrieving `metrics`, `info` and `config` which
can contain non-ascii-only string fields.

PR-URL: #295
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
This commit refactors the GrpcClient::DelegateAsyncExport method to use
templates instead of having multiple overloaded implementations for each
event type.

This change significantly reduces code duplication while maintaining the
same functionality, making the codebase more maintainable and less prone
to errors when adding new event types.

PR-URL: #294
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
PR-URL: #298
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
2025-03-27 Node.js v18.20.8 Hydrogen (LTS) Release
Git-EVTag-v0-SHA512: d57a53ecd5d691e17939f560839e7e6fc2d6cca40fc5a0c909c78a2c8bbbcd5f44fa24a5a7830826ed4d8d5a98d6084a3800301a3a22e8c6cf51ba2a514d222b
@santigimeno santigimeno requested a review from juanarbol May 7, 2025 10:16
@santigimeno santigimeno self-assigned this May 7, 2025
@windsurf-bot
Copy link
Copy Markdown

windsurf-bot Bot commented May 7, 2025

I ran into an unexpected issue while reviewing this PR. Please try again later.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 7, 2025

Walkthrough

This change introduces a comprehensive gRPC agent system for the N|Solid runtime, adding new native modules, C++ source and header files, Protocol Buffer definitions, and generated gRPC/protobuf code. The agent implements bidirectional and streaming gRPC communication for profiling, metrics, logging, tracing, configuration, and asset export. It integrates with OpenTelemetry, supports SaaS authentication, and provides a robust asynchronous event-driven architecture for remote monitoring and control of N|Solid processes.

Changes

File(s) Change Summary
agents/grpc/lib/agent.js, agents/grpc/lib/nsolid.js New JavaScript modules exporting agent interfaces and native bindings for gRPC agent integration.
agents/grpc/proto/*.proto New Protocol Buffer definitions for gRPC agent communication, covering assets, commands, events, profiling, metrics, info, packages, source code, startup times, and reconfiguration.
agents/grpc/proto/BUILD New build script to generate C++ code from .proto files using protoc and gRPC plugins.
agents/grpc/src/asset_stream.cc, agents/grpc/src/asset_stream.h New C++ implementation and header for AssetStream class, managing client-side gRPC streaming for asset export with async handling and observer notification.
agents/grpc/src/binding.cc New Node.js native binding exposing gRPC agent profiling and snapshotting methods to JavaScript.
agents/grpc/src/command_stream.cc, agents/grpc/src/command_stream.h New C++ implementation and header for CommandStream class, managing bidirectional gRPC streams for command exchange with async read/write and observer callbacks.
agents/grpc/src/grpc_agent.cc, agents/grpc/src/grpc_agent.h New C++ implementation and header for GrpcAgent class, orchestrating the gRPC agent's lifecycle, event handling, profiling, metrics, OpenTelemetry integration, SaaS support, and async communication.
agents/grpc/src/grpc_client.cc, agents/grpc/src/grpc_client.h New C++ implementation and header for gRPC client utilities, including channel/context/stub creation and a template for async event export via gRPC.
agents/grpc/src/grpc_errors.h New header defining gRPC-specific error codes, messages, and an error struct for use throughout the agent.
agents/grpc/src/proto/*.pb.h, agents/grpc/src/proto/*.pb.cc Generated C++ protobuf headers and sources for all defined .proto messages, implementing full serialization, deserialization, and message APIs.
agents/grpc/src/proto/*.grpc.pb.h, agents/grpc/src/proto/*.grpc.pb.cc Generated C++ gRPC headers and sources for service scaffolding, supporting async and sync RPCs (currently stubs for future service implementation).
agents/.eslintrc.yaml New ESLint config for agents, enforcing strict coding standards, import restrictions, and custom rules for internal consistency.

Changes to Exported/Public Entities

  • New exported JavaScript agent interface and binding in agents/grpc/lib/agent.js and agents/grpc/lib/nsolid.js.
  • New C++ classes: AssetStream, CommandStream, GrpcAgent, GrpcClient, and associated observer interfaces and data structures.
  • New Protocol Buffer message classes (C++): Asset, CommandArgs, CommandRequest, CommandResponse, Time, ErrorInfo, CommonResponse, Stack, BlockedLoopBody, BlockedLoopEvent, UnblockedLoopBody, UnblockedLoopEvent, InfoBody, InfoEvent, MetricsBody, MetricsEvent, PackagesBody, PackagesEvent, ProfileArgs, CPUProfileArgs, HeapProfileArgs, HeapSamplingArgs, HeapSnapshotArgs, ReconfigureBody, ReconfigureEvent, SourceCodeArgs, SourceCodeEvent, StartupTimesEvent, and more.
  • New gRPC service definition: NSolidService with multiple RPC methods for commands and event export.

Sequence Diagram(s)

sequenceDiagram
    participant JS as JavaScript (N|Solid)
    participant Binding as Native Binding (C++)
    participant GrpcAgent as GrpcAgent (C++)
    participant gRPC as gRPC Server (Console/Remote)
    participant Observer as Observer/Callback

    JS->>Binding: Call start_cpu_profile(args)
    Binding->>GrpcAgent: start_cpu_profile(args)
    GrpcAgent->>gRPC: Send CommandRequest (start CPU profile)
    gRPC-->>GrpcAgent: CommandResponse (ack/complete)
    GrpcAgent-->>Binding: Return result
    Binding-->>JS: Return result

    Note over GrpcAgent,gRPC: For streaming events (e.g., metrics/assets)
    GrpcAgent->>gRPC: Stream Asset/Metrics/Info events
    gRPC-->>GrpcAgent: Stream responses (if any)
    GrpcAgent->>Observer: Notify on stream completion or error
Loading

Poem

In the warren of code, a new path appears,
With gRPC streams and protobuf cheers!
Agents now chatter in C++ and JS,
Exporting their metrics—no need to guess.
Async and streaming, with OpenTelemetry’s might,
The N|Solid agent hops into sight!
🐇✨
✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 23

🔭 Outside diff range comments (1)
.github/workflows/comment-labeled.yml (1)

46-56: ⚠️ Potential issue

Add notable-change job triggered by label
The new job correctly uses pull_request_target to post comments, which allows it to run with the latest workflow code. Be cautious: pull_request_target runs in the context of the base branch and can execute with elevated privileges. Ensure you don’t add any untrusted steps or code here.

♻️ Duplicate comments (1)
agents/grpc/src/binding.cc (1)

129-132: Same namespace problem for StopSamplingSync

Mirrors the previous issue – adjust to the correct fully-qualified symbol once
the shadowing fix is applied.

🧹 Nitpick comments (74)
CHANGELOG.md (1)

5-11: Update version list for Node.js lifecycle.
The top-level list now highlights 18 as LTS and marks older versions appropriately. Consider hyphenating compound adjectives (e.g., “Long-Term Support”) for consistency with style guides.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~5-~5: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... 18](doc/changelogs/CHANGELOG_V18.md) Long Term Support * [Node.js 17](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 16](doc/changelogs/CHANGELOG_V16.md) Long Term Support * [Node.js 15](doc/changelogs/C...

(EN_COMPOUND_ADJECTIVE_INTERNAL)

.devcontainer/Dockerfile (1)

1-1: Pin the base image for reproducibility

Using the nightly tag can lead to unexpected breakages as it changes frequently. To ensure a stable and reproducible development environment, consider pinning to the specific Node.js version released (e.g., 18.20.8) or to a known LTS devcontainer image.

Example diff:

- FROM nodejs/devcontainer:nightly
+ FROM nodejs/devcontainer:18.20.8
CHANGELOG_NSOLID.md (2)

5-5: Grammar: use “an” before vowel sounds

“Select a N|Solid version…” should be updated to:

- Select a N|Solid version below to view the changelog history:
+ Select an N|Solid version below to view the changelog history:
🧰 Tools
🪛 LanguageTool

[misspelling] ~5-~5: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...eral-urls prohibited-strings--> Select a N|Solid version below to view the chang...

(EN_A_VS_AN)


7-7: Compound adjective should be hyphenated

Use “Long-Term Support” for clarity:

- * [N|Solid 5 | Node.js 18](doc/changelogs/NSOLID_CHANGELOG_V5_NODE_V18.md) **Long Term Support**
+ * [N|Solid 5 | Node.js 18](doc/changelogs/NSOLID_CHANGELOG_V5_NODE_V18.md) **Long-Term Support**
🧰 Tools
🪛 LanguageTool

[uncategorized] ~7-~7: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...logs/NSOLID_CHANGELOG_V5_NODE_V18.md) Long Term Support

(EN_COMPOUND_ADJECTIVE_INTERNAL)

GOVERNANCE.md (1)

1-3: Remove unnecessary escape in blockquote

To render the admonition marker correctly, drop the backslash before the brackets:

- > \[!IMPORTANT]
+ > [!IMPORTANT]
agents/grpc/lib/nsolid.js (1)

1-1: Remove redundant "use strict" directive
Node.js ES modules and CommonJS modules run in strict mode by default, making the explicit directive unnecessary.

🧰 Tools
🪛 Biome (1.9.4)

[error] 1-1: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)

agents/grpc/proto/BUILD (1)

4-4: Parameterize gRPC plugin location
Hardcoding ./out/Release/grpc_cpp_plugin couples the build to a specific directory layout. Consider using an env var or discovering it via which grpc_cpp_plugin.

agents/grpc/proto/startup_times.proto (1)

7-10: Consider reserving field numbers for future compatibility
Adding reserved statements can prevent accidental reuse of field IDs and ease future schema evolution.

For example:

message StartupTimesEvent {
  reserved 3 to max;
  CommonResponse common = 1;
  map<string, uint64> body = 2;
}
agents/grpc/proto/asset.proto (1)

8-15: Consider using bytes for binary payloads
The data field is defined as string, which assumes UTF-8 text. If you intend to send binary blobs, switch to:

bytes data = 4;
agents/grpc/lib/agent.js (1)

15-23: Prefer relative/internal resolution over bare identifiers
Using 'internal/nsolid_assets' relies on non-standard module resolution. Consider using a relative path or require.resolve to guard against refactoring breakage.

.gitignore (1)

170-171: Ignore NSolid-specific build directories
Entries for /nsolid and /nsolid_g correctly omit NSolid outputs; consider appending a trailing slash (e.g., /nsolid/) to explicitly target directories.

.devcontainer/.devcontainer.json (1)

10-10: Prune Docker resources on initialization
Running docker system prune -f -a is aggressive and may remove needed images or volumes. Consider narrowing the scope (e.g., --filter "label!=keep") or documenting its effect.

.github/SUPPORT.md (1)

3-6: Refine wording for clarity
Consider replacing “make sure” with “ensure” to strengthen the instruction:

- Please make sure you are using a currently supported version
+ Please ensure you are using a currently supported version
🧰 Tools
🪛 LanguageTool

[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)

agents/grpc/proto/reconfigure.proto (1)

7-18: Use consistent Protobuf field naming conventions.

Current fields use lowerCamelCase (e.g., blockedLoopThreshold, tracingEnabled). The Protobuf style guide recommends snake_case for field names. Consider renaming fields to:

blocked_loop_threshold, tracing_enabled, promise_tracking, etc.
CODE_OF_CONDUCT.md (2)

27-27: Refine wording for clarity and tone.

The phrase "and unwelcome sexual attention or advances" could be tightened for readability. For example:

* The use of sexualized language or imagery and unwelcome sexual attention
🧰 Tools
🪛 LanguageTool

[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)


62-62: Simplify wording to reduce wordiness.

Change:

maintain confidentiality with regard to the reporter

to:

maintain confidentiality regarding the reporter

or

maintain confidentiality about the reporter
🧰 Tools
🪛 LanguageTool

[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)

agents/grpc/proto/packages.proto (1)

1-5: Remove or convert commented-out TODO into a concrete task.

The introductory comments about RuntimeResponse outline future work but aren't actionable. Either implement the oneof or replace with a // TODO: tag and track it in the issue tracker.

README.md (1)

48-48: Refine phrasing for license variety
Consider replacing "a variety of licenses" with "various licenses" for conciseness.

🧰 Tools
🪛 LanguageTool

[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)

.github/workflows/auto-start-ci.yml (1)

13-15: Clarify and track the temporary Node.js version pin
The inline todo for Node v18 compatibility is useful, but consider standardizing the TODO format (e.g., TODO: uppercase) and adding a reference to an issue or timeline for when this workaround can be removed.

agents/.eslintrc.yaml (3)

1-3: Specify Node.js environment for ESLint
Currently only es6: true is set. If these rules apply to Node.js code, consider adding:

env:
  node: true
  es6: true

to enable Node-specific globals and parser options.


4-10: Enhance operator grouping in no-mixed-operators
You’ve restricted &&/|| mixing; consider adding arithmetic or bitwise operator groups if similar precedence risks exist in agents code.


30-164: no-restricted-globals list is exhaustive but long
Consider grouping related globals or comments to improve maintainability. Otherwise, the explicit restrictions prevent unsafe global usage effectively.

SECURITY.md (5)

7-7: Add missing comma after introductory adverb

- Normally your report will be acknowledged within 5 days...
+ Normally, your report will be acknowledged within 5 days...

18-18: Hyphenate compound adjective

- ## Reporting a bug in a third party module
+ ## Reporting a bug in a third-party module
🧰 Tools
🪛 LanguageTool

[uncategorized] ~18-~18: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...eported issue. ## Reporting a bug in a third party module Security bugs in third party mo...

(EN_COMPOUND_ADJECTIVE_INTERNAL)


20-20: Hyphenate compound adjective

- Security bugs in third party modules should be reported...
+ Security bugs in third-party modules should be reported...
🧰 Tools
🪛 LanguageTool

[uncategorized] ~20-~20: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... a third party module Security bugs in third party modules should be reported to their res...

(EN_COMPOUND_ADJECTIVE_INTERNAL)


31-32: Insert comma for clause clarity

- held locally pending the announcement.
+ held locally, pending the announcement.
🧰 Tools
🪛 LanguageTool

[uncategorized] ~32-~32: Possible missing comma found.
Context: ...e not committed to the public repository but rather held locally pending the annou...

(AI_HYDRA_LEO_MISSING_COMMA)


41-41: Correct preposition for eligibility

- Experimental features are eligible to reports...
+ Experimental features are eligible for reports...
🧰 Tools
🪛 LanguageTool

[uncategorized] ~41-~41: The preposition “for” seems more likely in this position than the preposition “to”.
Context: ...rts Experimental features are eligible to reports as any other stable feature of ...

(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_TO_FOR)

.github/workflows/coverage-linux.yml (2)

40-44: Pin actions/checkout and setup-python to specific commits
Locking these actions at v4.1.0 and v4.7.0 improves reproducibility. Ensure to update these references periodically to incorporate security and functionality patches.


63-63: Pin codecov-action to a specific commit
Locking codecov-action at v3.1.4 (commit) improves CI stability. Plan for periodic updates to benefit from new features and security fixes.

.github/workflows/comment-labeled.yml (2)

13-17: New NOTABLE_CHANGE_MESSAGE for release note guidance
Introducing a guidance message is helpful. Consider checking line breaks, as YAML will include leading spaces—ensure the rendered comment doesn’t include unintended indentation.


37-38: Scope permissions for fast-track job
Restricting to pull-requests: write is appropriate for commenting. Consider if contents: read is needed for any future steps in this job.

.github/workflows/scorecard.yml (2)

19-19: Use contents: read instead of read-all
While read-all grants global read, switching to explicit contents: read adheres more closely to least privilege. If additional read scopes are needed, specify them explicitly.


45-64: Review Scorecard action configuration
The Scorecard action is correctly pinned. You’ve enabled publish_results for public repos; ensure private repos supply repo_token when needed. Consider splitting public/private config via conditional YAML or inputs.

.github/workflows/close-stale-pull-requests.yml (2)

12-18: Close message clarity
The CLOSE_MESSAGE is informative. For readability, consider adding a blank line before "If you feel closing..." to separate the call to action.


19-26: Stale warning message
The warning text matches the deadlines. You may want to add a comma after "...not the right thing to do," for improved grammar.

agents/grpc/proto/command.proto (1)

1-5: Remove development placeholder comments.
The leading comments (lines 1–5) appear to be scaffolding notes. Consider removing or replacing them with formal documentation or examples.

.github/workflows/label-flaky-test-issue.yml (1)

22-47: Improve platform parsing for reliability.
The current approach replaces newlines with literal \n and uses brittle sed patterns. Consider using awk to extract the lines between the Platform header and the next blank line, preserving true newlines:

PLATFORMS=$(printf "%s\n" "$BODY" | awk '/^Platform$/ {flag=1; next} flag && /^$/ {exit} flag')

This is more robust against formatting variations.

CONTRIBUTING.md (7)

13-15: Refine phrasing for conciseness.
Consider changing “In order to build this project…” to “To build this project, you need…” for a more direct tone.


17-19: Ensure proper list formatting.
Add a blank line before the dependency links (* [Dependencies]… and * [Setting up…]) so the bullet list renders correctly in Markdown.


59-62: Clarify branch strategy phrasing.
“The reason is to simplify the change rebase.” is missing “The” and could read “This simplifies change rebasing.” for clarity and grammatical correctness.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~60-~60: A determiner appears to be missing. Consider inserting it.
Context: ...e changes are landed on the latest tag. Reason is to simplify the change rebase. There...

(AI_EN_LECTOR_MISSING_DETERMINER)


[uncategorized] ~62-~62: A comma might be missing here.
Context: ...nflicting commits that would need to be resolved which could take a lot of time with lit...

(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)


139-139: Fix subject–verb agreement.
“N|Solid don't use…” should be “N|Solid doesn’t use…”.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~139-~139: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...roved Pull Requests Currently, N|Solid don't use the commit-queue label for lan...

(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)


144-146: Include command outputs or remove dollar signs.
Markdownlint flags dollar signs without example output. Either add sample output under each command or remove the leading $ prompts.

🧰 Tools
🪛 markdownlint-cli2 (0.17.2)

144-144: Dollar signs used before commands without showing output
null

(MD014, commands-show-output)


145-145: Dollar signs used before commands without showing output
null

(MD014, commands-show-output)


146-146: Dollar signs used before commands without showing output
null

(MD014, commands-show-output)


216-216: Strengthen word choice.
Replace “quite wide” with “broad” to avoid weak intensifiers.

🧰 Tools
🪛 LanguageTool

[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)


222-222: Replace wordy phrasing.
Simplify “prior to” to “before” in the context “allocation prior to the Buffer.alloc…”.

🧰 Tools
🪛 LanguageTool

[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)

.github/workflows/update-v8.yml (1)

39-44: Check for NEW_VERSION presence.
If update-v8-patch.sh does not output NEW_VERSION=, the workflow will proceed silently. Add a check to fail when NEW_VERSION is missing:

if ! grep -q 'NEW_VERSION=' temp-output; then
  echo 'ERROR: NEW_VERSION not found' >&2
  exit 1
fi
agents/grpc/proto/blocked_loop.proto (1)

1-5: TODO comment for RuntimeResponse message.

The file includes a comment indicating future plans to define a RuntimeResponse message with a oneOf field for various response types. Consider implementing this design or removing the comment if this approach is no longer planned.

agents/grpc/proto/nsolid_service.proto (1)

30-32: Simple EventResponse message.

The EventResponse message provides a straightforward way to return error information from the various export RPCs. Consider adding a success status field to provide explicit success indication rather than relying on an empty error message.

message EventResponse {
  string error_message = 1;
+ bool success = 2;
}
agents/grpc/proto/profile.proto (3)

19-20: Consider adding implementation details to CPUProfileArgs.

The CPUProfileArgs message is currently empty. If this is intentional as a placeholder for future implementation, consider adding a comment indicating this. Otherwise, add the necessary fields for CPU profiling configuration.

message CPUProfileArgs {
+  // TODO: Add CPU profiling specific configuration parameters
}

7-17: Add documentation comments to describe message fields.

The ProfileArgs message contains several fields without documentation. Consider adding comments to explain:

  • What thread_id represents (is 0 for main thread or all threads?)
  • The unit of duration (milliseconds, seconds?)
  • The expected structure and usage of metadata
message ProfileArgs {
+  // Thread ID to profile, 0 for main thread or all threads
  uint64 thread_id = 1;
+  // Duration of profiling in milliseconds
  uint64 duration = 2;
+  // Additional metadata for the profile request
  google.protobuf.Struct metadata = 3;
  oneof args {
    CPUProfileArgs cpu_profile = 4;
    HeapProfileArgs heap_profile = 5;
    HeapSamplingArgs heap_sampling = 6;
    HeapSnapshotArgs heap_snapshot = 7;
  }
}

27-31: Document HeapSamplingArgs parameters and flags.

The HeapSamplingArgs message contains parameters that would benefit from documentation, especially the flags field which likely corresponds to specific bit flags.

message HeapSamplingArgs {
+  // Sampling interval in bytes (0 for default)
  uint64 sample_interval = 1;
+  // Maximum stack depth to capture (0 for default)
  uint32 stack_depth = 2;
+  // Sampling flags (see heap_profiler.h for valid options)
  uint32 flags = 3;
}
.github/workflows/coverage-linux-without-intl.yml (1)

56-57: Test failure is silently ignored.

The test command has || exit 0 which means it will always succeed even if tests fail. Make sure this is intentional and consider adding a comment explaining why failures are being ignored.

-      - name: Test
-        run: NODE_V8_COVERAGE=coverage/tmp make test-cov -j2 V=1 TEST_CI_ARGS="-p dots --measure-flakiness 9" || exit 0
+      - name: Test
+        # Tests are allowed to fail without failing the workflow as we're primarily interested in coverage data
+        run: NODE_V8_COVERAGE=coverage/tmp make test-cov -j2 V=1 TEST_CI_ARGS="-p dots --measure-flakiness 9" || exit 0
agents/grpc/src/grpc_client.cc (2)

36-40: Consider making keepalive parameters configurable.

The keepalive parameters are hardcoded. Consider making these configurable through the OtlpGrpcClientOptions to allow for different scenarios.

-  grpc_arguments.SetInt(GRPC_ARG_KEEPALIVE_TIME_MS, 30 * 1000 /* 30 sec*/);
-  grpc_arguments.SetInt(GRPC_ARG_KEEPALIVE_TIMEOUT_MS, 15 * 1000 /* 15 sec*/);
+  grpc_arguments.SetInt(GRPC_ARG_KEEPALIVE_TIME_MS, 
+                        options.keepalive_time_ms.value_or(30 * 1000) /* default: 30 sec*/);
+  grpc_arguments.SetInt(GRPC_ARG_KEEPALIVE_TIMEOUT_MS, 
+                        options.keepalive_timeout_ms.value_or(15 * 1000) /* default: 15 sec*/);

47-48: Remove unnecessary empty line.

There's an extra empty line that can be removed.

  }

-
  SslCredentialsOptions ssl_opts;
BUILDING.md (4)

334-336: Consider adding executable example for Windows

You've included a helpful note about using python3 on Windows, but it might be beneficial to include a complete example command with the expected output.

 > Note: On Windows you should use `python3` executable.
 > Example: `python3 tools/test.py test/message`
+> 
+> ```console
+> C:\path\to\nsolid> python3 tools/test.py test/message
+> ```

123-123: Minor grammatical issue

There's a minor grammatical issue in the footnote text.

-[^1]: Older kernel versions may work. However official N|Solid release
+[^1]: Older kernel versions may work. However, official N|Solid release
🧰 Tools
🪛 LanguageTool

[uncategorized] ~123-~123: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ... [^1]: Older kernel versions may work. However official N|Solid release binaries a...

(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)


781-781: Minor punctuation issue

Add a comma after "By default" for improved readability.

-By default N|Solid is built so that all dependencies
+By default, N|Solid is built so that all dependencies
🧰 Tools
🪛 LanguageTool

[uncategorized] ~781-~781: Did you mean: “By default,”?
Context: ... to use shared dependencies at runtime By default N|Solid is built so that all dependenci...

(BY_DEFAULT_COMMA)


821-821: Minor punctuation issue

Add a comma after "To maintain ABI compatibility" for improved readability.

-To maintain ABI compatibility it is required that distributed builds
+To maintain ABI compatibility, it is required that distributed builds
🧰 Tools
🪛 LanguageTool

[uncategorized] ~821-~821: A comma might be missing here.
Context: ...within a major release. To maintain ABI compatibility it is required that distributed builds ...

(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)

.github/workflows/daily-wpt-fyi.yml (1)

45-46: jq is used without an explicit install – runner images change over time

Ubuntu images usually include jq, but this is not guaranteed. A sudden image update could break the step that discovers the latest nightly.

Consider installing it explicitly (adds only a few seconds):

- name: Install jq
  run: sudo apt-get update && sudo apt-get install -y jq

(or switch to the built-in jq action).

agents/grpc/src/proto/asset.pb.cc (1)

1-15: Generated protobuf sources add >10 K LoC – consider excluding from the repo

Keeping generated .pb.cc / .pb.h files in Git bloats diffs and obscures real code reviews (as here).
Typical practice is to:

  1. Commit only the .proto definitions.
  2. Regenerate artefacts during the build (CMake/configure.py/npm script).
  3. Cache them in CI to avoid rebuild cost.

Unless you have strong reasons (e.g., vendoring binaries for language-agnostic consumers), consider moving generated files to build outputs and add them to .gitignore.

agents/grpc/src/binding.cc (3)

68-76: Argument validation causes hard aborts in production

ASSERT aborts the process when the API is mis-used from JS.
Prefer throwing a TypeError back to JavaScript so the application can handle
it gracefully.

-ASSERT_EQ(2 , args.Length());
-ASSERT(args[0]->IsNumber());
-ASSERT(args[1]->IsBoolean());
+CHECK_GE(args.Length(), 2);   // dev assertion
+if (!args[0]->IsNumber() || !args[1]->IsBoolean()) {
+  isolate->ThrowException(
+    v8::Exception::TypeError(
+      OneByteString(isolate, "heapProfile(duration, trackAllocations)")));
+  return;
+}

98-110: Numeric truncation: double ➜ uint64

v8::Number::Value() returns a double; direct assignment to uint64_t
silently truncates fractional values and can mis-represent integers larger than
2^53. Validate and cast explicitly:

double d = args[0].As<Number>()->Value();
if (d < 0 || d > std::numeric_limits<uint64_t>::max())
  // throw RangeError
uint64_t sample_interval = static_cast<uint64_t>(d);

138-147: Missing export of RegisterExternalReferences in non-main threads

RegisterExternalReferences always registers Start, but Start is not
exported when the current Env is not the main thread (see lines 160-164).
This leaves an external reference that can never be invoked and wastes a slot.
Consider only registering Start when the main thread check passes.

agents/grpc/src/command_stream.h (3)

44-49: Minor: mark trivial helpers noexcept & inline

is_done() only acquires a mutex and returns a boolean; declaring it noexcept + inline documents intent and allows better optimisation:

-  bool is_done() const {
+  inline bool is_done() const noexcept {

44-45: Avoid an extra copy when queueing responses

Write(grpcagent::CommandResponse&& resp) should forward the r-value into the queue to avoid an unnecessary copy. Assuming TSQueue::Push (or similar) supports move-semantics:

-  void Write(grpcagent::CommandResponse&& resp);
+  template <typename T>
+  void Write(T&& resp) {
+    nsuv::ns_mutex::scoped_lock lock(lock_);
+    response_q_.push(std::forward<T>(resp));
+    NextWrite();
+  }

If TSQueue does not expose push(T&&), consider emplacing or explicitly std::move(resp) when enqueuing inside the .cc file.


55-62: Thread-safety: guard write_state_ with std::atomic for read-only checks

write_state_.done is read under a mutex in is_done() but also accessed in the destructor (inside the same mutex) and presumably in OnDone() (gRPC thread). Converting the two booleans to std::atomic<bool> removes the need for holding lock_ just to check flags, shortens critical sections and clarifies memory ordering.

 struct WriteState {
-  bool write_done = true;
-  bool done = false;
+  std::atomic<bool> write_done{true};
+  std::atomic<bool> done{false};
   grpcagent::CommandResponse resp;
 };
agents/grpc/src/proto/common.pb.cc (1)

1-6: Consider excluding generated .pb.cc from VCS

This file is 940+ lines of fully generated protobuf code. Keeping generated artefacts under version control:

  • Bloats the repository size.
  • Causes noisy diffs on every regeneration (e.g. protoc version bumps).
  • Hides the real, hand-written changes in reviews.

Unless you have a documented reason (e.g. hermetic builds without protoc in CI), it is usually cleaner to add agents/grpc/src/proto/*.pb.[ch]cc to .gitignore and regenerate in the build pipeline.

agents/grpc/src/proto/blocked_loop.pb.h (1)

1-6: Large generated header checked in

Same remark as for common.pb.cc: this 1 800-line header is generated. Storing it in the repo adds maintenance overhead; please consider generating at build time instead.

agents/grpc/src/grpc_client.h (1)

83-96: Missing null-check for result_callback

result_callback is dereferenced unconditionally.
Either ASSERT_NOT_NULL(result_callback) before swapping, or early-return with error code -2 if the caller forgot to supply one.

agents/grpc/src/proto/asset.pb.h (1)

585-587: Comment typo in include-guard footer

The #endif comment expands to
GOOGLE_PROTOBUF_INCLUDED_GOOGLE_PROTOBUF_INCLUDED_asset_2eproto, which duplicates the prefix and does not match the #define at the top (GOOGLE_PROTOBUF_INCLUDED_asset_2eproto).
Although this is harmless to the pre-processor, it is confusing for readers and automated linters.

-#endif  // GOOGLE_PROTOBUF_INCLUDED_GOOGLE_PROTOBUF_INCLUDED_asset_2eproto
+#endif  // GOOGLE_PROTOBUF_INCLUDED_asset_2eproto
Makefile (2)

139-150: ninja dependency isn’t auto-detected

NINJA ?= ninja is introduced, but the subsequent build step ($(NINJA) -C …) fails with a cryptic error when ninja isn’t installed.
Add a simple check to surface a friendly message:

NINJA ?= ninja
ifeq (, $(shell command -v $(NINJA)))
$(error "ninja not found – install ninja-build or set NINJA=/path/to/ninja")
endif

308-316: test-agents-prereqs performs live npm install – harms offline / hermetic builds

The new prerequisite installs several packages from the public npm registry every time the test target runs.
This introduces non-determinism (un-pinned transitive deps) and breaks CI environments without external network access.

Suggestions:

  1. Check the dependencies into a local cache or use npm ci --offline with a committed package-lock.json.
  2. Gate the install behind an environment flag so package fetches happen only on demand.
agents/grpc/src/grpc_agent.h (2)

8-12: Avoid ./ prefix in include path

#include "./proto/nsolid_service.grpc.pb.h" uses a relative ./ which is unnecessary and occasionally upsets include-path tooling.
Prefer:

#include "proto/nsolid_service.grpc.pb.h"

54-58: JSThreadMetrics lacks rule-of-five safeguards

The struct stores a SharedThreadMetrics smart-pointer but does not explicitly default/copy/move its special members.
If SharedThreadMetrics has custom semantics (e.g. ref-counting), accidental copies could be costly or unsafe.
Consider:

struct JSThreadMetrics {
  explicit JSThreadMetrics(SharedEnvInst envinst);
  SharedThreadMetrics metrics_;
  JSThreadMetrics(const JSThreadMetrics&) = default;
  JSThreadMetrics(JSThreadMetrics&&) noexcept = default;
  JSThreadMetrics& operator=(const JSThreadMetrics&) = default;
  JSThreadMetrics& operator=(JSThreadMetrics&&) noexcept = default;
};
agents/grpc/src/grpc_agent.cc (1)

71-72: GRPC_MAX_SIZE comment is misleading (4 MB ≠ 4 GB)

4L * 1024 * 1024 is 4 MiB, not 4 GiB. This mismatch can confuse future maintainers and already surfaces later in the chunk-splitting logic.

-const size_t GRPC_MAX_SIZE = 4L * 1024 * 1024;  // 4GB
+// Maximum gRPC message size we support: 4 MiB.
+const size_t GRPC_MAX_SIZE = 4L * 1024 * 1024;  // 4 MB
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9265d2f and b9e7e7e.

📒 Files selected for processing (105)
  • .devcontainer/.devcontainer.json (1 hunks)
  • .devcontainer/Dockerfile (1 hunks)
  • .eslintignore (1 hunks)
  • .eslintrc.js (11 hunks)
  • .flake8 (0 hunks)
  • .gitattributes (1 hunks)
  • .github/CODEOWNERS (3 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 (0 hunks)
  • .github/workflows/authors.yml (0 hunks)
  • .github/workflows/auto-start-ci.yml (3 hunks)
  • .github/workflows/build-tarball.yml (3 hunks)
  • .github/workflows/build-windows.yml (2 hunks)
  • .github/workflows/close-stale-feature-requests.yml (1 hunks)
  • .github/workflows/close-stale-pull-requests.yml (1 hunks)
  • .github/workflows/close-stalled.yml (1 hunks)
  • .github/workflows/comment-labeled.yml (2 hunks)
  • .github/workflows/commit-lint.yml (1 hunks)
  • .github/workflows/commit-queue.yml (2 hunks)
  • .github/workflows/coverage-linux-without-intl.yml (1 hunks)
  • .github/workflows/coverage-linux.yml (2 hunks)
  • .github/workflows/coverage-windows.yml (3 hunks)
  • .github/workflows/daily-wpt-fyi.yml (1 hunks)
  • .github/workflows/daily.yml (1 hunks)
  • .github/workflows/doc.yml (0 hunks)
  • .github/workflows/find-inactive-collaborators.yml (1 hunks)
  • .github/workflows/find-inactive-tsc.yml (3 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/linters.yml (3 hunks)
  • .github/workflows/notify-force-push.yml (0 hunks)
  • .github/workflows/notify-on-push.yml (1 hunks)
  • .github/workflows/scorecard.yml (1 hunks)
  • .github/workflows/test-asan.yml (1 hunks)
  • .github/workflows/test-internet.yml (1 hunks)
  • .github/workflows/test-linux.yml (1 hunks)
  • .github/workflows/test-macos.yml (1 hunks)
  • .github/workflows/timezone-update.yml (1 hunks)
  • .github/workflows/tools.yml (3 hunks)
  • .github/workflows/update-openssl.yml (1 hunks)
  • .github/workflows/update-v8.yml (1 hunks)
  • .gitignore (4 hunks)
  • .gitpod.yml (1 hunks)
  • .mailmap (22 hunks)
  • BUILDING.md (23 hunks)
  • CHANGELOG.md (2 hunks)
  • CHANGELOG_NSOLID.md (1 hunks)
  • CODE_OF_CONDUCT.md (1 hunks)
  • CONTRIBUTING.md (2 hunks)
  • GOVERNANCE.md (3 hunks)
  • LICENSE (9 hunks)
  • LICENSE_NSOLID (1 hunks)
  • Makefile (31 hunks)
  • README.md (1 hunks)
  • SECURITY.md (1 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/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)
  • agents/grpc/src/proto/command.pb.h (1 hunks)
  • agents/grpc/src/proto/common.grpc.pb.cc (1 hunks)
  • agents/grpc/src/proto/common.grpc.pb.h (1 hunks)
  • agents/grpc/src/proto/common.pb.cc (1 hunks)
💤 Files with no reviewable changes (11)
  • .github/ISSUE_TEMPLATE/3-api-ref-docs-problem.yml
  • .flake8
  • .github/ISSUE_TEMPLATE/2-feature-request.yml
  • .github/ISSUE_TEMPLATE/1-bug-report.yml
  • .github/ISSUE_TEMPLATE/config.yml
  • .github/ISSUE_TEMPLATE/4-report-a-flaky-test.yml
  • .github/workflows/label-pr.yml
  • .github/workflows/authors.yml
  • .github/workflows/notify-force-push.yml
  • .github/workflows/doc.yml
  • .github/label-pr-config.yml
🧰 Additional context used
🧬 Code Graph Analysis (3)
agents/grpc/src/asset_stream.h (2)
agents/grpc/src/grpc_client.h (1)
  • agent_id (64-64)
agents/grpc/src/asset_stream.cc (2)
  • AssetStream (18-33)
  • AssetStream (35-36)
agents/grpc/src/binding.cc (2)
agents/grpc/src/grpc_agent.cc (2)
  • Inst (420-425)
  • Inst (420-420)
src/node_binding.cc (2)
  • RegisterExternalReferences (730-733)
  • RegisterExternalReferences (730-730)
agents/grpc/src/command_stream.h (2)
agents/grpc/src/grpc_client.h (1)
  • agent_id (64-64)
agents/grpc/src/command_stream.cc (2)
  • CommandStream (18-33)
  • CommandStream (35-46)
🪛 Biome (1.9.4)
agents/grpc/lib/nsolid.js

[error] 1-1: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)

agents/grpc/lib/agent.js

[error] 1-1: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)

🪛 LanguageTool
CHANGELOG_NSOLID.md

[misspelling] ~5-~5: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...eral-urls prohibited-strings--> Select a N|Solid version below to view the chang...

(EN_A_VS_AN)


[uncategorized] ~7-~7: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...logs/NSOLID_CHANGELOG_V5_NODE_V18.md) Long Term Support

(EN_COMPOUND_ADJECTIVE_INTERNAL)

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)

.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)

SECURITY.md

[typographical] ~6-~6: Consider adding a comma after ‘Normally’ for more clarity.
Context: ... Runtime via security@nodesource.com Normally your report will be acknowledged within...

(RB_LY_COMMA)


[uncategorized] ~18-~18: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...eported issue. ## Reporting a bug in a third party module Security bugs in third party mo...

(EN_COMPOUND_ADJECTIVE_INTERNAL)


[uncategorized] ~20-~20: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... a third party module Security bugs in third party modules should be reported to their res...

(EN_COMPOUND_ADJECTIVE_INTERNAL)


[uncategorized] ~32-~32: Possible missing comma found.
Context: ...e not committed to the public repository but rather held locally pending the annou...

(AI_HYDRA_LEO_MISSING_COMMA)


[uncategorized] ~41-~41: The preposition “for” seems more likely in this position than the preposition “to”.
Context: ...rts Experimental features are eligible to reports as any other stable feature of ...

(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_TO_FOR)

README.md

[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)

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)


[uncategorized] ~60-~60: A determiner appears to be missing. Consider inserting it.
Context: ...e changes are landed on the latest tag. Reason is to simplify the change rebase. There...

(AI_EN_LECTOR_MISSING_DETERMINER)


[uncategorized] ~62-~62: A comma might be missing here.
Context: ...nflicting commits that would need to be resolved which could take a lot of time with lit...

(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)


[grammar] ~91-~91: The verb ‘bisect’ does not usually follow articles like ‘a’. Check that ‘bisect’ is spelled correctly; using ‘bisect’ as a noun may be non-standard.
Context: ... branch. The reason for this is so that a bisect can always have passing tests, and all ...

(A_INFINITIVE)


[uncategorized] ~97-~97: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...e) LTS version. For the initial N|Solid open source release, this branch was nsolid-v5.x....

(EN_COMPOUND_ADJECTIVE_INTERNAL)


[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)


[uncategorized] ~139-~139: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...roved Pull Requests Currently, N|Solid don't use the commit-queue label for lan...

(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)


[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)

CHANGELOG.md

[uncategorized] ~5-~5: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... 18](doc/changelogs/CHANGELOG_V18.md) Long Term Support * [Node.js 17](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 16](doc/changelogs/CHANGELOG_V16.md) Long Term Support * [Node.js 15](doc/changelogs/C...

(EN_COMPOUND_ADJECTIVE_INTERNAL)

BUILDING.md

[grammar] ~37-~37: The word ‘install’ is not a noun.
Context: ...rerequisites) * Option 1: Manual install * [Opt...

(A_INSTALL)


[grammar] ~38-~38: The word ‘install’ is not a noun.
Context: ...l-install) * [Option 2: Automated install with Boxstarter](#option-2-automated-in...

(A_INSTALL)


[uncategorized] ~123-~123: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ... [^1]: Older kernel versions may work. However official N|Solid release binaries a...

(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)


[grammar] ~142-~142: Make sure that ‘run’ is a verb and that it has the correct inflection to agree with the noun ‘infrastructure’.
Context: ...ies are provided. However, tests in our infrastructure only run on WoW64. Furthermore, compiling on...

(PRPS_NN_ONLY_NN)


[uncategorized] ~470-~470: A comma might be missing here.
Context: ...use the debug build with all the normal dependencies overwrite the release version in the in...

(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)


[grammar] ~471-~471: The word ‘install’ is not a noun.
Context: ...es overwrite the release version in the install directory: ```console $ make install P...

(A_INSTALL)


[uncategorized] ~781-~781: Did you mean: “By default,”?
Context: ... to use shared dependencies at runtime By default N|Solid is built so that all dependenci...

(BY_DEFAULT_COMMA)


[uncategorized] ~821-~821: A comma might be missing here.
Context: ...within a major release. To maintain ABI compatibility it is required that distributed builds ...

(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)

🪛 Buf (1.47.2)
agents/grpc/proto/asset.proto

3-3: import "common.proto": file does not exist

(COMPILE)

agents/grpc/proto/reconfigure.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/metrics.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/packages.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/blocked_loop.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/command.proto

8-8: import "profile.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/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)

🪛 markdownlint-cli2 (0.17.2)
CONTRIBUTING.md

144-144: Dollar signs used before commands without showing output
null

(MD014, commands-show-output)


145-145: Dollar signs used before commands without showing output
null

(MD014, commands-show-output)


146-146: Dollar signs used before commands without showing output
null

(MD014, commands-show-output)

🪛 actionlint (1.7.4)
.github/workflows/notify-on-push.yml

63-63: receiver of object dereference "default_branch" must be type of object but got "string"

(expression)

🔇 Additional comments (243)
.gitattributes (1)

3-5: EOL normalization for executable files is correctly applied.
The additions enforce LF line endings on deps/npm/bin/npm, deps/npm/bin/npx, and deps/corepack/shims/corepack, which aligns with POSIX expectations and prevents cross-platform inconsistencies.

.github/dependabot.yml (1)

1-11: Monthly Dependabot updates are configured correctly.
The new configuration sets a monthly cadence, prefixes PRs with “meta,” and caps open updates at 10, which balances staying up-to-date with manageable PR volume.

.github/workflows/find-inactive-collaborators.yml (6)

13-14: Grant read-only permission for repository contents.
Adding permissions: contents: read tightens the default scope for this workflow.


22-22: Pin actions/checkout to a specific commit for reproducibility.
Locking to 8ade135a41bc03ea155e62e844d188df1ea18608 ensures workflow stability even if future v4.x releases introduce breaking changes.


28-28: Pin actions/setup-node to a specific commit.
Using 5e21ff4d9bc1a8cf6de233a3057d20ec6b3fb69d (v3.8.1) aligns with the reproducibility practice across workflows.


36-38: Pin create-or-update PR action and clarify behavior.
Locking gr2m/create-or-update-pull-request-action to a commit hash and documenting its no-op behavior if up-to-date improves security and maintainability.


39-41: Verify use of GH_USER_TOKEN secret.
This workflow references ${{ secrets.GH_USER_TOKEN }}; please confirm that this secret exists with the correct permissions.


44-48: Clear and informative PR body.
The multi-line body adds context and directs the @nodejs/tsc team to offboarding tasks—great for handoff.

CHANGELOG.md (2)

30-31: Adjust table headers to current LTS versions.
Adding Node.js 18 and retaining Node.js 16 LTS headers is correct—no further action needed.


35-49: Inject latest patch entries for Node.js 18.
The new 18.20.8 entry and subsequent links are accurate and in descending order, matching release conventions.

.github/workflows/license-builder.yml (4)

9-11: Set default read-only permissions.
Adding permissions: contents: read at the workflow level tightens security by minimizing privileges for all jobs.


14-17: Override job permissions for PR creation.
Granting contents: write and pull-requests: write only to the update_license job follows the principle of least privilege.


20-20: Pin actions/checkout to a commit.
Using commit 8ade135a41bc03ea155e62e844d188df1ea18608 (v4.1.0) ensures consistent checkout behavior.


24-26: Pin PR creation action and document its behavior.
Locking gr2m/create-or-update-pull-request-action to a SHA and clarifying no-op semantics is aligned with reproducibility best practices.

.eslintignore (1)

9-12: Ignore patterns look appropriate

Excluding benchmark/fixtures and older changelog files while explicitly including CHANGELOG_v18.md aligns with the updated linting scope. Ensure these paths match exactly the directory structure in the repo.

.github/PULL_REQUEST_TEMPLATE.md (1)

2-6: Template updates are clear

The expanded instructions (bulleted links to CONTRIBUTING and commit guidelines) and the new notable-change label note enhance contributor guidance. No further changes needed here.

Also applies to: 13-14

GOVERNANCE.md (2)

125-126: Approval criteria update looks good

The refined language specifying “TSC voting member” approvals and opposition is clear and aligns with governance requirements.


184-184: Collaborators link is correct

The updated discussions URL for collaborator nominations points to the intended category. No changes needed.

.github/workflows/close-stalled.yml (3)

12-14: Explicit global permission scoping
Granting contents: read at the workflow level limits privileges to only what's necessary for metadata access. Good practice.


17-19: Job-level write permissions for stale action
Scoping issues: write and pull-requests: write at the stale job correctly confines elevated access to only that job.


23-23: Pinning actions/stale to a commit hash
Using an exact commit (1160a22…) rather than a floating tag (v4) ensures reproducible runs and guards against upstream breaking changes.

agents/grpc/lib/nsolid.js (1)

4-6: Correct initialization and export of native bindings
The entry point cleanly acquires the nsolid_grpc_agent binding and passes it into the agent wrapper. This aligns with existing patterns for internalBinding-based modules.

agents/grpc/src/proto/blocked_loop.grpc.pb.cc (1)

1-26: Skip review of generated code
This file is auto-generated by the gRPC C++ plugin; manual edits will be overwritten.

agents/grpc/src/proto/asset.grpc.pb.h (1)

1-33: Skip review of generated code
Header generated by the gRPC C++ plugin. Changes should originate from the .proto definition.

agents/grpc/src/proto/asset.grpc.pb.cc (1)

1-25: Skip review of generated code
Source generated by the gRPC C++ plugin; manual edits will be lost on regeneration.

.github/workflows/daily.yml (3)

11-13: Scope permissions to least privilege
Adding permissions: contents: read reduces token scope, which hardens the workflow against unintended side effects.


20-20: Pin actions/checkout to a specific commit
Locking to 8ade135a41bc03ea155e62e844d188df1ea18608 ensures reproducible and secure checkouts.


24-24: Pin actions/setup-node to a specific commit
Using 5e21ff4d9bc1a8cf6de233a3057d20ec6b3fb69d avoids breaking changes from newer releases.

.github/workflows/commit-lint.yml (4)

8-10: Limit permissions for this workflow
Setting permissions: contents: read aligns with least-privilege principles for commit-lint.


18-19: Migrate deprecated ::set-output syntax
Replacing ::set-output with appending to $GITHUB_OUTPUT is the recommended approach for newer GitHub Actions versions.


20-20: Pin actions/checkout to a specific commit
Locking to 8ade135a41bc03ea155e62e844d188df1ea18608 enhances workflow stability and security.


26-26: Pin actions/setup-node to a specific commit
Using 5e21ff4d9bc1a8cf6de233a3057d20ec6b3fb69d ensures consistent Node.js setup.

.github/workflows/close-stale-feature-requests.yml (3)

31-33: Add global contents: read permission
This change scopes the workflow’s permissions appropriately, reducing risk.


36-38: Grant job-level write permissions for stale operations
issues: write and pull-requests: write are required by actions/stale to manage issue/PR states.


42-42: Pin actions/stale to a specific commit
Locking to 1160a2240286f5da8ec72b1c0816ce2481aabf84 (v8.0.0) improves reproducibility and security.

agents/grpc/src/proto/common.grpc.pb.cc (1)

1-25: Skip review for generated code
This file is auto-generated by the gRPC C++ plugin from common.proto; manual edits will be overwritten.

.gitpod.yml (2)

1-5: Verify ignoring build errors in init task
The || true after ./configure && timeout 50m make -j16 will mask failures. Confirm that ignoring errors here is intentional to avoid workspace aborts.


6-12: Review GitHub prebuild settings
By setting pullRequests: false and addCheck: false, prebuilds and checks are disabled for PRs. Ensure this aligns with your desired developer experience.

agents/grpc/src/proto/command.grpc.pb.cc (1)

1-26: Generated Code – Skip Manual Review
This file is auto-generated by the gRPC C++ plugin from command.proto. Manual edits will be overwritten on regeneration; any custom logic should be placed in separate implementation files.

agents/grpc/proto/BUILD (1)

5-8:

Details

✅ Verification successful

Verify proto import paths
Ensure that common.proto and other dependencies exist in the specified --proto_path roots; otherwise protoc will error.


🏁 Script executed:

#!/bin/bash
# Check for common.proto in any of the proto paths
find agents/grpc/proto deps/protobuf/src deps/opentelemetry-cpp/third_party/opentelemetry-proto -type f -name common.proto

Length of output: 253


Proto import paths verified
All required common.proto files are present in the specified --proto_path roots:

  • agents/grpc/proto/common.proto
  • deps/opentelemetry-cpp/third_party/opentelemetry-proto/opentelemetry/proto/common/v1/common.proto

No missing proto dependencies detected.

agents/grpc/proto/startup_times.proto (1)

3-3:

Details

✅ Verification successful

Validate import of common.proto
Buf reported that common.proto was not found. Confirm it’s present in agents/grpc/proto/ or adjust your --proto_path.


🏁 Script executed:

#!/bin/bash
# Confirm common.proto is in the proto dir
find agents/grpc/proto -maxdepth 1 -type f -name common.proto

Length of output: 94


Import of common.proto is valid
The file agents/grpc/proto/common.proto exists at the expected path. The import in agents/grpc/proto/startup_times.proto is correct. Ensure your Buf configuration or CLI invocation includes agents/grpc/proto in its --proto_path (or buf.yaml roots) so that common.proto can be resolved.

🧰 Tools
🪛 Buf (1.47.2)

3-3: import "common.proto": file does not exist

(COMPILE)

agents/grpc/proto/asset.proto (1)

3-3:

Details

✅ Verification successful

Ensure common.proto is accessible
Buf flagged common.proto as missing. Verify it exists in agents/grpc/proto/ or update your proto paths.


🏁 Script executed:

#!/bin/bash
find agents/grpc/proto -maxdepth 1 -type f -name common.proto

Length of output: 94


common.proto import is valid
The file agents/grpc/proto/common.proto exists, so the import "common.proto"; in agents/grpc/proto/asset.proto is correct. No changes required.

🧰 Tools
🪛 Buf (1.47.2)

3-3: import "common.proto": file does not exist

(COMPILE)

agents/grpc/lib/agent.js (3)

1-1: Review 'use strict' necessity
ES Modules are strict by default, but CommonJS modules require the directive. Confirm your module type in package.json or lint config before removing it.

# Check if the project is using ESM
grep -R '"type": "module"' -n package.json
🧰 Tools
🪛 Biome (1.9.4)

[error] 1-1: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)


3-12: Verify destructured function names
Ensure that internal/nsolid_assets and the native binding provide methods matching these names (heapProfile, heapProfileEnd, etc.). Mismatches here will lead to runtime undefined errors.

grep -R "module\\.exports" -n agents/grpc/src

24-27: API export shape is clear
The returned object cleanly exposes start, status, and stop along with internal asset methods—nice and concise.

.github/workflows/build-windows.yml (4)

26-27: Update Python version and flaky tests behavior
Bumping PYTHON_VERSION to 3.11 aligns with other workflows, and switching FLAKY_TESTS to keep_retrying should reduce intermittent CI failures.


29-30: Add minimal permissions scope
Granting only contents: read restricts the workflow’s privileges appropriately.


41-42: Pin actions/checkout to a specific SHA
Locking actions/checkout at 8ade135... improves stability and security by preventing unintended upgrades.


45-47: Pin actions/setup-python to a specific SHA
Using 61a6322... for actions/setup-python follows best practices for reproducible CI.

.gitignore (4)

10-11: Track devcontainer configuration
The !.devcontainer/ and !.devcontainer/.devcontainer.json rules correctly ensure your new container files aren’t ignored.


23-23: Include Gitpod configuration
Un-ignoring .gitpod.yml is necessary to support Gitpod workflows.


121-121: Ignore V8 tap JSON files
Adding /v8*-tap.json avoids tracking auto-generated V8 test output.


144-144: Ignore nested .github directories in deps
The /deps/**/.github/ pattern prevents vendored GitHub metadata from polluting the repo.

.devcontainer/.devcontainer.json (2)

3-8: Configure recommended VS Code extensions
The selected extensions support PR reviews, live collaboration, icon theming, and IntelliCode—perfect for a core development environment.


11-18: Customize integrated terminal profile
Defining Zsh as a login shell in Linux containers is a solid choice for consistency.

agents/grpc/src/proto/command.grpc.pb.h (1)

1-39: Skip generated gRPC code review
This header is auto-generated by the gRPC C++ plugin; manual edits will be lost.

agents/grpc/src/proto/blocked_loop.grpc.pb.h (1)

1-39: Skip generated gRPC code review
This header is auto-generated by the gRPC C++ plugin; manual edits will be lost.

agents/grpc/src/proto/common.grpc.pb.h (1)

1-34: Skip review for generated code
This file is auto-generated by the gRPC C++ plugin from common.proto. Manual edits will be overwritten.

.mailmap (10)

7-7: Approve contributor mapping
The identity mapping for Adam Langley is correctly formatted.


31-31: Approve contributor mapping
The identity mapping for Andreas Schwab is correctly formatted.


48-50: Approve contributor mappings
The three entries for Ash Cripps consolidate aliases correctly.


55-55: Approve contributor mapping
The identity mapping for Austin Kelleher is correctly formatted.


72-73: Approve contributor mappings
The two entries for Beth Griggs consolidate aliases correctly.


83-83: Approve contributor mapping
The identity mapping for Brian Muenzenmeyer is correctly formatted.


90-90: Approve contributor mapping
The identity mapping for Camillo Bruni is correctly formatted.


94-96: Approve contributor mappings
The three entries for Chemi Atlow consolidate aliases correctly.


99-100: Approve contributor mappings
The two entries for Chengzhong Wu consolidate aliases correctly.


110-111: Approve contributor mappings
The two entries for Claudio Wunder and Clemens Backes consolidate aliases correctly.

.github/workflows/coverage-windows.yml (7)

15-16: Restrict workflow to main branch
Limiting push triggers to main aligns with other CI workflows.


31-32: Upgrade Python and retry policy
Bumping to Python 3.11 and updating FLAKY_TESTS to keep_retrying matches upstream workflows.


34-36: Add read-only permissions
Granting contents: read follows least-privilege best practices.


40-40: Update runner environment
Switching to windows-2022 ensures consistency with other Windows workflows.


42-42: Pin actions/checkout to commit
Using a SHA instead of a tag improves reproducibility and security.


46-46: Pin actions/setup-python to commit
Locking to a specific commit SHA aligns with repository policy on pinned actions.


68-68: Pin codecov-action to commit
Pinning codecov/codecov-action to a SHA prevents unintended upgrades.

.github/SUPPORT.md (4)

9-12: Approve updated support links
Links now correctly point to N|Solid resources in place of Node.js communities.


16-16: Approve license statement update
The disclaimer correctly references N|Solid’s open-source license.


20-20: Approve new section heading
The "Professional Support Offering" section heading is clear and well placed.


22-29: Approve professional support details
The section accurately describes NodeSource’s N|Solid support services with clear scope and link.

.github/workflows/commit-queue.yml (5)

21-23: Add repository read permission
Granting contents: read at the global level aligns with other workflows’ permission settings.


26-28: Grant pull-request read permission
Adding pull-requests: read for the get_mergeable_prs job follows least-privilege.


35-44: Revise PR listing logic
Fetching both commit-queue and fast-track PRs, then deduplicating with jq, is a clean approach. The date filter ensures only recent PRs are queued.


61-61: Pin actions/checkout to commit
Locking to SHA 8ade135a41bc03ea155e62e844d188df1ea18608 improves workflow stability.


74-74: Pin actions/setup-node to commit
Using SHA 5e21ff4d9bc1a8cf6de233a3057d20ec6b3fb69d ensures consistent Node.js setup.

agents/grpc/proto/info.proto (3)

7-23: Well-structured InfoBody message with comprehensive system metadata.

The InfoBody message contains a complete set of fields needed to identify and describe a Node.js process, including hardware details, runtime information, and application metadata.


25-28: InfoEvent follows the standard event pattern with CommonResponse.

The InfoEvent message correctly follows what appears to be a standard pattern by including a common response object and a specific body, enabling consistent handling across different event types.


3-3:

Details

❓ Verification inconclusive

Verify the import of "common.proto" exists in your build environment.

Static analysis indicates that the import "common.proto" cannot be found. Ensure that this file exists in a location that can be resolved by your protobuf compiler, either in the same directory or in a directory included in the protoc include path.


🏁 Script executed:

#!/bin/bash
# Check if common.proto exists in the expected locations
echo "Checking for common.proto in the proto directory..."
find agents/grpc/proto -name "common.proto" -type f

echo "Checking for proto include paths in build files..."
find agents/grpc -name "*.gyp" -o -name "*.gypi" -o -name "Makefile" -o -name "*.mk" | xargs grep -l "proto"

Length of output: 413


Confirm that agents/grpc/proto is on your protoc include path

We found common.proto alongside info.proto in agents/grpc/proto, but none of your Makefile, .gyp/.gypi, or .mk files reference this directory. Please verify that your build (e.g., your protoc invocation or CMake/Bazel rules) includes agents/grpc/proto so that

import "common.proto";

resolves correctly.

• agents/grpc/proto/common.proto exists
• No build scripts reference the proto directory
• Ensure protoc is called with -I agents/grpc/proto (or equivalent)

🧰 Tools
🪛 Buf (1.47.2)

3-3: import "common.proto": file does not exist

(COMPILE)

.github/workflows/test-linux.yml (5)

26-27: Good updates to Python version and flaky test handling.

Upgrading to Python 3.11 and changing FLAKY_TESTS to keep_retrying should improve CI reliability.


29-30: Security improvement with explicit permissions.

Adding explicit permissions following the principle of least privilege is a security best practice.


37-37: Security enhancement with pinned action version.

Pinning to a specific commit hash improves security and reproducibility of workflows.


41-41: Security enhancement with pinned action version.

Pinning to a specific commit hash improves security and reproducibility of workflows.


49-49: Test enhancement with flakiness measurement.

Adding the --measure-flakiness 9 parameter will help track and address intermittent test failures.

.github/workflows/test-internet.yml (7)

10-14: Improved trigger paths for internet-related tests.

Expanding the trigger paths to include DNS and networking files is appropriate for this workflow.


21-25: Consistent path triggers for push events.

Good consistency by applying the same path triggers to both pull_request and push events.


32-33: Good updates to Python version and flaky test handling.

Upgrading to Python 3.11 and changing FLAKY_TESTS to keep_retrying should improve CI reliability.


35-36: Security improvement with explicit permissions.

Adding explicit permissions following the principle of least privilege is a security best practice.


40-40: Smart conditional execution for the workflow.

The condition ensures the workflow runs only in appropriate contexts, avoiding unnecessary runs.


43-43: Security enhancement with pinned action version.

Pinning to a specific commit hash improves security and reproducibility of workflows.


47-47: Security enhancement with pinned action version.

Pinning to a specific commit hash improves security and reproducibility of workflows.

agents/grpc/src/grpc_errors.h (3)

1-2: Proper header guards in place.

The header guards follow best practices for preventing multiple inclusion.


4-12: Well-organized error definition macro.

The GRPC_ERRORS macro provides a clear and maintainable way to define error types with consistent metadata.


14-33: Clean namespace organization and error type definitions.

The code follows good C++ practices with proper namespace nesting and a type-safe enum class for error types.

.github/workflows/find-inactive-tsc.yml (4)

13-15: Add minimal required permissions.

Granting contents: read follows the principle of least privilege for this workflow. Verify that no additional permissions (e.g., pull-requests, actions) are needed for any of the steps.


23-31: Pin actions/checkout to an immutable SHA.

Using a specific commit (8ade135a41bc03ea155e62e844d188df1ea18608) ensures reproducible CI runs. Confirm that this SHA corresponds to v4.1.0 of actions/checkout and is updated consistently across workflows.


36-40: Pin actions/setup-node to a stable SHA.

Locking actions/setup-node@5e21ff4d9bc1a8cf6de233a3057d20ec6b3fb69d (v3.8.1) similarly improves security and stability. Ensure the version meets your Node.js setup requirements.


45-52: Verify PR creation action and token scope.

The gr2m/create-or-update-pull-request-action is pinned to 77596e3166f328b24613f7082ab30bf2d93079d5. Ensure the GH_USER_TOKEN secret is defined with proper scopes (e.g., contents: write, pull_requests: write) and is correctly referenced.

agents/grpc/proto/reconfigure.proto (2)

3-3: Verify existence and import path for common.proto.

Static analysis flags that common.proto isn't found. Ensure that common.proto is present in the proto include path or adjust the import to point to the correct relative path.

🧰 Tools
🪛 Buf (1.47.2)

3-3: import "common.proto": file does not exist

(COMPILE)


17-18: Clarify tracingModulesBlacklist type and semantics.

The field uint32 tracingModulesBlacklist = 11; suggests a bitmask but the name implies a list. Confirm whether this should be:

  • A repeated string of module names to blacklist, or
  • An enum/bitmask representing categories.
    If it's a list of names, update the type accordingly.
agents/grpc/proto/packages.proto (1)

8-8: Verify common.proto import path.

Similar to reconfigure.proto, ensure common.proto is available in the proto include directories or adjust the path accordingly.

🧰 Tools
🪛 Buf (1.47.2)

8-8: import "common.proto": file does not exist

(COMPILE)

.github/workflows/test-asan.yml (5)

32-35: Upgrade Python version and adjust ASAN settings.

Bumping to Python 3.11 and adding ASAN_OPTIONS is appropriate. Confirm that all test dependencies and CI scripts support Python 3.11 and that FLAKY_TESTS=keep_retrying behaves as expected.


36-38: Least-privilege permissions for the workflow.

Granting only contents: read is sufficient for ASan testing. Approving this minimal permission set.


42-43: Explicitly exclude draft PRs from ASan testing.

The if: github.event.pull_request.draft == false guard prevents unnecessary runs on drafts, saving CI resources.


50-54: Pin checkout and Python setup actions.

Locking actions/checkout@8ade135a4... (v4.1.0) and actions/setup-python@61a6322... (v4.7.0) SHAs ensures consistent runner behavior. Verify these SHAs correspond to the intended versions.


62-62: Adjust test command for flakiness measurement.

Appending --measure-flakiness 9 to TEST_CI_ARGS can help triage intermittent failures. Confirm that your test harness supports this flag and that 9 is the desired threshold.

README.md (8)

1-6: Update project title and introduction
The new title and introductory paragraphs correctly reflect the N|Solid branding and succinctly describe the project. The Code of Conduct link is clear and appropriately emphasized.


10-11: Verify support documentation link
Please confirm that .github/SUPPORT.md exists and is the intended support instructions file. If the support guide has been relocated or renamed, update this link accordingly.


15-19: Approve download section updates
The download URLs have been updated to NodeSource distribution channels and correctly point to the binaries and installers for N|Solid.


21-24: Approve API documentation link
Linking to https://docs.nodesource.com/ for N|Solid-specific documentation is appropriate and clearly distinguished from generic Node.js docs.


30-33: Approve build instructions reference
Referencing BUILDING.md for build-from-source instructions is consistent and correctly scoped to N|Solid.


35-39: Approve security reporting section
The link to SECURITY.md is accurate and directs users to the proper vulnerability reporting process for N|Solid.


46-50: Approve license section update
Updating to show both LICENSE and LICENSE_NSOLID ensures that users see the core MIT license as well as the bundled third-party licenses.

🧰 Tools
🪛 LanguageTool

[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)


52-53: Approve contributing link update
The reference to CONTRIBUTING.md is correctly updated for N|Solid-specific contribution guidelines.

agents/grpc/proto/metrics.proto (3)

3-4: Verify import path for common.proto
Buf indicates that common.proto may not be found at this path. Please ensure that common.proto resides in the same directory (agents/grpc/proto) or adjust the import to the correct relative path.

🧰 Tools
🪛 Buf (1.47.2)

3-3: import "common.proto": file does not exist

(COMPILE)


5-6: Confirm OpenTelemetry import path
Importing opentelemetry/proto/metrics/v1/metrics.proto aligns with the external proto structure. Ensure your build tool (Bazel/Make/CMake) has this path configured in its include paths.


9-16: Approve MetricsBody and MetricsEvent definitions
The message definitions correctly encapsulate resource metrics and pair them with common response metadata. The field numbering is logical and in sequence.

.github/workflows/test-macos.yml (5)

32-34: Verify Python version compatibility
Upgrading to Python 3.11 is a good modernization step. Please confirm that all scripts and dependencies in the macOS workflow are fully compatible with Python 3.11.


35-37: Approve minimal permissions block
Granting read-only access to repository contents (contents: read) follows the principle of least privilege and is sufficient for this workflow.


43-48: Approve pinning actions to specific commits
Locking actions/checkout and actions/setup-python to immutable SHAs improves reproducibility and security.


60-73: Approve cleanup step for Android SDK
Removing the Android SDK and grouping disk usage logs is a practical enhancement to free up space on macOS runners.


75-78: Approve dynamic parallelism and flakiness measurement
Using getconf _NPROCESSORS_ONLN for -j and adding --measure-flakiness 9 increases efficiency and reliability of CI runs.

LICENSE_NSOLID (1)

1-546: Comprehensive license file appears correct
The LICENSE_NSOLID file comprehensively lists the MIT license and bundled third-party licenses. The formatting and attribution notices are clear and complete.

agents/grpc/proto/common.proto (2)

3-4: Package directory mismatch – verify grpcagent path
Buf warns that package "grpcagent" should reside in a grpcagent directory. Please confirm that your directory structure or build configuration allows this package declaration under agents/grpc/proto.

🧰 Tools
🪛 Buf (1.47.2)

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)


5-20: Approve core CommonResponse messages
The Time, ErrorInfo, and CommonResponse messages are well-defined and serve as solid foundations for the gRPC agent system. Field ordering and types are appropriate.

.github/workflows/auto-start-ci.yml (6)

17-18: Explicit minimal permissions look good
Granting only contents: read is aligned with the principle of least privilege.


22-23: Job-level PR permissions are correctly scoped
Limiting the get-prs-for-ci job to pull-requests: read is appropriate for its functionality.


32-38: GH CLI invocation and output handling
The gh pr list command and $GITHUB_OUTPUT usage follow recommended patterns. Consider adding --json url or other fields if more context is needed in downstream steps.


42-44: Start-CI job permissions are correct
contents: read and pull-requests: write accurately reflect the needs of triggering CI on external PRs.


49-54: Pinned actions ensure workflow stability
Locking actions/checkout@… and actions/setup-node@… to specific SHAs is a best practice for reproducible CI.


63-65: Double-quoted GH_USER_TOKEN for safety
Quoting the GH_USER_TOKEN secret prevents word-splitting issues if the token contains special characters.

.github/workflows/build-tarball.yml (9)

31-32: Bump to Python 3.11 and retry flakiness mode
Upgrading to 3.11 aligns with other CI updates. Please verify that all test dependencies support 3.11.


34-35: Added explicit contents: read permission
Granting read-only access to repository contents reduces the blast radius of the workflow.


42-42: Pinned actions/checkout for reproducibility
Locking to SHA 8ade135a41bc… follows the repository’s CI hardening strategy.


46-48: Pinned Python setup action
Using actions/setup-python@61a6322… ensures consistent environment configuration.


60-63: Pinned upload-artifact action
Locking to v4.6.0 SHA maintains artifact upload stability.


68-71: Pinned checkout in test job
Consistent pinning across jobs ensures that the workflow does not break due to action updates.


72-74: Pinned setup-python in test job
Aligns with the build job’s environment configuration.


78-81: Pinned download-artifact action
Ensures the test job always uses a known-good version of the download step.


97-97: Flakiness measurement flag added
The --measure-flakiness 9 argument is a welcome addition. Confirm that the Makefile target supports retrying up to 9 times as intended.

agents/.eslintrc.yaml (3)

10-29: Comprehensive no-restricted-syntax rules look solid
The selectors cover common pitfalls (assert usage, error throwing). Ensure these rules align with the new gRPC agent code patterns.


165-174: Custom node-core/* rules enabled
The additional rules (prototype-pollution, lowercase-name, no-array-destructuring) enforce internal consistency. Confirm all rule implementations exist in tools/eslint-rules.


237-245: Globals block correctly locks module internals
Marking require, process, etc., as read-only aligns with primordials usage. No changes needed here.

.github/workflows/timezone-update.yml (1)

43-46: Verify use of GH_USER_TOKEN secret
You’re mapping GH_USER_TOKEN into GITHUB_TOKEN. Confirm that the GH_USER_TOKEN secret exists and has the correct scopes (repo write). Consider using the default GITHUB_TOKEN if it suffices.

.github/workflows/coverage-linux.yml (6)

14-15: Restrict push trigger to main branch
The workflow now only runs on pushes to main, dropping master. Confirm that the repository fully transitioned to main; if other branches (e.g., legacy master) are still active, you may miss coverage reports.


29-31: Upgrade Python version and flaky tests strategy
Pinning to Python 3.11 and changing FLAKY_TESTS to keep_retrying aligns with modern runtimes. Ensure that all Linux runners have Python 3.11 available and that downstream scripts respect the new FLAKY_TESTS value.


32-33: Limit permissions for security
Adding contents: read is a good move to follow the principle of least privilege. No write permissions are granted at this level, which is appropriate for coverage reporting.


50-50: Update gcovr version
Upgrading to gcovr==7.2 brings new features and fixes. Confirm compatibility with the existing C++ codebase and coverage tooling.


56-56: Measure test flakiness and allow failures
Adding --measure-flakiness 9 helps detect unreliable tests; exiting with 0 ensures coverage continues even if flaky tests fail. Confirm that this strategy won't mask genuine test regressions.


58-58: Revise C++ coverage report command
Switching to gcovr --object-directory=out ... --gcov-executable="llvm-cov-18 gcov" is more explicit. Verify that all coverage data is correctly captured under out and that filtering by src matches your source layout.

.github/workflows/comment-labeled.yml (1)

23-25: Scope permissions for stale-comment job
Granting issues: write and pull-requests: write only to this job minimizes risk. This aligns with least privilege.

agents/grpc/proto/exit.proto (1)

3-4: Verify common.proto import path
Static analysis flags that common.proto may not be located alongside this file. Confirm that your Bazel proto_path or include directives include the directory containing common.proto, or adjust the import to the correct relative path.

🧰 Tools
🪛 Buf (1.47.2)

3-3: import "common.proto": file does not exist

(COMPILE)

.github/workflows/scorecard.yml (3)

27-29: Scoped write permissions for security checks
Limiting security-events: write and id-token: write to the analysis job is appropriate. Ensure you uncomment contents: read if running in private repos.


36-38: Pin step-security/harden-runner action
Locking to commit v2.5.1 is good. The TODO to switch from audit to block egress after evaluation is useful—remember to update once safe.


68-73: Set retention policy for SARIF artifact
You’ve chosen a 5-day retention which aligns with team needs. Confirm this window captures all triage activity post-scan before it expires.

.github/workflows/close-stale-pull-requests.yml (4)

1-9: Define optional endDate input for workflow dispatch
Allowing an endDate override is flexible. Ensure the date format documented in the description matches the one parsed by the date command (RFC-2822).


28-30: Scope global permissions
Granting only contents: read at the top level is minimal. The job correctly upgrades to pull-requests: write for closing PRs.


38-44: Compute and override END_DATE
Using date --date='525600 minutes ago' --rfc-2822 sets the default to one year ago. The conditional override works, but ensure the environment variable is exported correctly (GITHUB_ENV append).


45-60: Validate actions/stale configuration
Setting days-before-issue-stale: -1 and days-before-issue-close: -1 disables stale handling for issues, while PRs are managed at 150/30 days. Confirm this behavior matches repository policy.

agents/grpc/proto/source_code.proto (4)

1-2: Syntax version is correctly set.
Using syntax = "proto3"; ensures compatibility with protobuf v3.


7-11: SourceCodeArgs message is well-defined.
Fields thread_id, script_id, and path use appropriate types and numbering.


13-19: SourceCodeEvent message structure looks good.
Including CommonResponse along with thread_id, path, code, and is_esm covers all required fields.


3-3:

Details

✅ Verification successful

Verify common.proto import path.
Buf static analysis reports import "common.proto"; file not found. Ensure common.proto is present in agents/grpc/proto/ and that the build includes this directory in the --proto_path.


🏁 Script executed:

#!/bin/bash
# Verify existence and content of common.proto
if [[ -f agents/grpc/proto/common.proto ]]; then
  grep -R "message CommonResponse" agents/grpc/proto/common.proto || \
    echo "CommonResponse not defined in common.proto" >&2
else
  echo "common.proto not found in agents/grpc/proto/" >&2
fi

Length of output: 132


Import path for common.proto verified
The file agents/grpc/proto/common.proto exists and defines message CommonResponse as expected. No changes are needed to the import "common.proto"; line.
If Buf still reports a missing import, please ensure that agents/grpc/proto/ is included in your build’s --proto_path (or in your buf.yaml/buf.mod configuration).

🧰 Tools
🪛 Buf (1.47.2)

3-3: import "common.proto": file does not exist

(COMPILE)

agents/grpc/proto/command.proto (5)

6-6: Correct proto3 declaration.
The file correctly specifies syntax = "proto3";.


14-20: CommandArgs oneof grouping is correct.
Using a oneof for reconfigure, profile, and source_code enforces mutual exclusivity and is idiomatic.


22-27: CommandRequest message is clear.
Fields requestId, id, command, and nested CommandArgs cover the request payload comprehensively.


29-32: CommandResponse message is appropriate.
A simple status code and message suffice for response handling.


8-10:

Details

✅ Verification successful

Verify import paths for proto dependencies.
Static analysis warns that profile.proto may not be found. Ensure that profile.proto, reconfigure.proto, and source_code.proto exist alongside this file and are included in your proto include paths.


🏁 Script executed:

#!/bin/bash
# Check for imported proto files
for proto in profile.proto reconfigure.proto source_code.proto; do
  if [[ ! -f agents/grpc/proto/$proto ]]; then
    echo "$proto is missing in agents/grpc/proto/" >&2
  fi
done

Length of output: 337


Proto imports verified
The files profile.proto, reconfigure.proto, and source_code.proto all exist in agents/grpc/proto/. No missing imports detected. Ensure that agents/grpc/proto is included in your protoc include paths so these imports resolve correctly.

🧰 Tools
🪛 Buf (1.47.2)

8-8: import "profile.proto": file does not exist

(COMPILE)

.github/workflows/label-flaky-test-issue.yml (2)

1-6: Correct triggers and permissions.
The workflow triggers on issue opened and labeled events and scopes permissions to contents: read and issues: write, following the principle of least privilege.


49-54: Verify GH CLI authentication environment variable.
gh issue edit requires authentication. Confirm that the gh CLI recognizes the GITHUB_TOKEN environment variable in this context, or switch to GH_TOKEN if needed.

.github/workflows/update-v8.yml (1)

19-23: Pinned actions usage is good practice.
Pinning actions/checkout, actions/cache, and actions/setup-node to specific commit SHAs improves stability and reproducibility.

Also applies to: 33-37

LICENSE (10)

58-58: Copyright year update for Acorn.

The copyright year for Acorn has been updated to include 2022, keeping the license information current.


81-104: License text expanded for c-ares.

The license for c-ares has been expanded to include the full MIT license text with proper attribution to Massachusetts Institute of Technology and extended copyright years to 2023. This provides more complete licensing information.


121-131: Added license for ittapi.

New dependency ittapi from Intel Corporation has been properly added with its BSD-style license, which is important for license compliance.


150-150: Updated Unicode copyright year.

The copyright year for Unicode has been updated to 2023, reflecting the latest version being used.


772-1010: Added detailed Postject license.

The license for Postject has been comprehensively added, including both the MIT license for Postject itself and the Apache 2.0 license for its LIEF dependency. This thorough documentation ensures proper attribution and compliance.


1295-1297: Updated zlib version and copyright years.

The zlib version reference has been updated to 1.2.13.1 (2022) from the previous 1.2.11 (2017), reflecting the newer version being used.


1319-1339: Added license for simdutf.

New MIT-licensed dependency simdutf has been properly documented with its license terms.


1341-1361: Added license for ada.

New MIT-licensed dependency ada (2023) has been properly documented with its license terms.


1363-1380: Added license for minimatch.

New ISC-licensed dependency minimatch has been properly documented with its license terms.


2153-2183: Added license for base64.

New BSD-style licensed dependency base64 has been properly documented with its license terms, including the copyright notices for multiple contributors from 2005-2019.

.github/workflows/notify-on-push.yml (1)

6-29: Notification system for force pushes to main branch.

The workflow properly implements Slack notifications for force pushes to the main branch, which is a good security practice to monitor potentially disruptive repository changes.

agents/grpc/proto/blocked_loop.proto (4)

12-19: Stack message definition for call stacks.

The Stack message is well-structured to contain all relevant information about stack frames including evaluation status, script information, function name, and position data.


21-32: BlockedLoop event messages.

The BlockedLoopBody and BlockedLoopEvent messages properly capture all necessary details for blocked event monitoring, including thread ID, duration, and stack information.


34-44: UnblockedLoop event messages.

The UnblockedLoopBody and UnblockedLoopEvent messages appropriately track the unblock events with the necessary identifiers to match with corresponding block events.


6-10:

Details

❓ Verification inconclusive

Protocol Buffer definition with import.

The file imports common.proto which may cause build issues if not found in the proto compiler's import path. Ensure that the build system correctly includes the directory containing common.proto.


🏁 Script executed:

#!/bin/bash
# Verify that common.proto exists and is accessible
find agents/grpc/proto -name "common.proto"

Length of output: 74


Verify Proto Import Path Configuration

File: agents/grpc/proto/blocked_loop.proto
Lines: 6–10

syntax = "proto3";

import "common.proto";

package grpcagent;
  • Confirmed common.proto exists at agents/grpc/proto/common.proto.
  • Ensure your build system (e.g., your protoc include paths in Makefile, Bazel rules, Gradle config, etc.) includes agents/grpc/proto so that the import "common.proto" resolves without errors.
🧰 Tools
🪛 Buf (1.47.2)

8-8: import "common.proto": file does not exist

(COMPILE)

agents/grpc/proto/nsolid_service.proto (2)

16-28: NSolidService gRPC interface design.

The service defines a comprehensive set of RPCs for various event types. The bidirectional streaming design for Command and the unary pattern for event exports are appropriate for their respective use cases.

Note that the Command RPC has request and response types reversed from typical convention - it takes CommandResponse stream and returns CommandRequest stream. This is unusual but may be intentional for your use case. Confirm this design is as intended.


1-14:

Details

✅ Verification successful

Service definition with multiple imports.

The service definition imports multiple proto files that define message types. Ensure all imported proto files are available in the protobuf compiler's path.


🏁 Script executed:

#!/bin/bash
# Verify all imported proto files exist in the proto directory
cd agents/grpc/proto
for IMPORT in asset.proto blocked_loop.proto command.proto exit.proto info.proto metrics.proto packages.proto reconfigure.proto source_code.proto startup_times.proto; do
  if [ ! -f "$IMPORT" ]; then
    echo "Missing import: $IMPORT"
  else
    echo "Found: $IMPORT"
  fi
done

Length of output: 2558


All imported proto files verified
All ten imported .proto files under agents/grpc/proto were found. No missing imports—no further action required.

🧰 Tools
🪛 Buf (1.47.2)

3-3: import "asset.proto": file does not exist

(COMPILE)

.eslintrc.js (12)

21-21: Updated Babel plugin for import attributes.

The Babel plugin has been updated from @babel/plugin-syntax-import-assertions to @babel/plugin-syntax-import-attributes, reflecting the change in the ECMAScript specification from import assertions to import attributes.


47-47: Consistent Babel plugin update in parser options.

The Babel plugin in parserOptions.babelOptions.plugins has been similarly updated to @babel/plugin-syntax-import-attributes for consistency with the earlier change.


58-59: Additional ESM test files configuration.

Configuration for ESM type flag test files has been added to the module source type override, properly ensuring these files are treated as ES modules by the linter.


114-129: New rule enforcement for specific directories.

Two new override blocks have been added:

  1. Enforcing the curly rule in module implementation files
  2. Enforcing the custom node-core/set-proto-to-null-in-object rule in the test runner

These targeted rules improve code consistency and security in specific critical areas of the codebase.


136-137: Simplified arrow function rules.

The arrow function formatting rules have been simplified to use string shorthand instead of objects, which is more concise while maintaining the same enforcement.


145-145: Simplified ignore pattern for capitalized comments.

The ignore pattern for the capitalized-comments rule has been simplified while maintaining the same functionality, improving rule readability.


153-153: Simplified comma-dangle rule.

The comma-dangle rule has been simplified to consistently require trailing commas in multiline expressions.


174-176: Simplified spacing rules.

The key-spacing and linebreak-style rules have been simplified to use string shorthand instead of objects, making the configuration more concise.


190-190: Simplified no-else-return rule.

The no-else-return rule has been simplified to use string shorthand instead of an object, maintaining the same enforcement with cleaner configuration.


297-297: Simplified space-in-parens rule.

The space-in-parens rule has been simplified to use string shorthand instead of an object, maintaining the same enforcement with cleaner configuration.


319-319: Added JSDoc rule configuration.

The jsdoc/no-defaults rule has been explicitly disabled, aligning with the project's approach to JSDoc enforcement.


333-350: Added new globals.

Two new globals have been added as readable:

  1. CustomEvent - A standard DOM API for custom events
  2. ShadowRealm - A JavaScript proposal for isolated evaluation contexts

These additions help prevent false positives when these APIs are used in the codebase.

agents/grpc/proto/profile.proto (1)

1-6:

Details

❌ Incorrect review comment

Package directory mismatch should be addressed.

The static analysis tool indicates that files with package "grpcagent" should be within a directory "grpcagent" relative to root, but this file is in "agents/grpc/proto". Consider either changing the package name to match your directory structure or moving the file to comply with the convention.


🏁 Script executed:

#!/bin/bash
# Check if other proto files in the same directory use the same package name
find agents/grpc/proto -name "*.proto" -exec grep -l "^package grpcagent;" {} \;

# Check if there's any package directory structure guideline in build files
find agents/grpc -name "BUILD*" -o -name "*.bzl" | xargs grep -l "proto"

Length of output: 616


🏁 Script executed:

#!/bin/bash
# Show the contents of the Bazel BUILD file to check proto_library import settings
sed -n '1,200p' agents/grpc/proto/BUILD

Length of output: 393


Ignore Proto Package Directory Mismatch

The grpcagent package is intentionally applied to all files under agents/grpc/proto/ and is correctly resolved at build time via the --proto_path=agents/grpc/proto/ flag. The package name does not need to mirror the on-disk folder layout—proto imports and generated namespaces work as intended. Instead of moving files or renaming the package, adjust your lint/static-analysis configuration to treat agents/grpc/proto as the root of the grpcagent package.

Likely an incorrect or invalid review comment.

🧰 Tools
🪛 Buf (1.47.2)

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)

.github/workflows/linters.yml (5)

16-18: Python version update looks good.

The Python version has been updated from 3.10 to 3.11, which ensures the workflow uses a more recent Python version.


20-22: Good security practice with explicit permissions.

Adding explicit permissions that limit the workflow to read-only access to the repository contents is a good security practice, following the principle of least privilege.


93-96: Removing the -i flag might change error handling behavior.

The -i flag (which likely stood for "ignore errors") has been removed from the make lint-py-build command on line 94. This change means the workflow will now fail if this step encounters errors, which is likely the desired behavior for better CI enforcement.


105-107: Updated shell linter script extension.

The shell script linter now uses tools/lint-sh.mjs instead of tools/lint-sh.js, suggesting a migration to ES modules. This is a positive modernization of the codebase.


69-72:

Details

❓ Verification inconclusive

Verify the conditional change to release versions step.

The condition for retrieving release version numbers has been modified to always evaluate to false by adding && false. This effectively disables the step. Ensure this is intentional and not a debugging artifact that was accidentally committed.


🏁 Script executed:

#!/bin/bash
# Check if other workflows have similar changes or if this is an isolated case
grep -r "&& false" .github/workflows/ --include="*.yml" | grep -v "coverage-linux-without-intl.yml"

Length of output: 275


Confirm intentional disabling of release version retrieval

The if condition in .github/workflows/linters.yml has been changed to include && false, which effectively disables the “Get release version numbers” step. This is the only workflow file with that pattern—please verify that this guard was added intentionally (and not as a leftover debug tweak) before merging.

• File: .github/workflows/linters.yml
• Lines: 69–72 (Get release version numbers step)

.github/workflows/coverage-linux-without-intl.yml (3)

1-23: Well-structured workflow configuration.

The new workflow is properly configured with appropriate triggers, path filters, and branch specifications. The paths-ignore section with the exception for this workflow file (lines 12 and 22) ensures the workflow can be updated without being ignored by its own filter.


32-34: Good practice using explicit permissions.

Explicitly specifying read-only permissions follows security best practices and the principle of least privilege.


52-53: Build configuration correctly disables internationalization.

The build step correctly uses the --without-intl flag alongside coverage instrumentation, which aligns with the workflow's purpose of testing without internationalization support.

agents/grpc/src/grpc_client.cc (1)

19-23: Well-implemented debug utility function.

The debug utility function is cleanly implemented using perfect forwarding which efficiently handles various argument types without unnecessary copies.

agents/grpc/src/asset_stream.h (1)

1-73: Well-designed gRPC stream implementation

The implementation follows good practices for asynchronous gRPC client streaming. The class structure with AssetStream, AssetStor, and AssetStreamObserver provides a clean separation of concerns.

A few notable strong points:

  • Proper use of thread safety with mutex and thread-safe queue
  • Good memory management using weak pointers for the observer pattern
  • Clear state management through the WriteState struct
.github/CODEOWNERS (3)

3-7: Improved documentation of CODEOWNERS purpose

This clarification helps contributors better understand that the file is for automation purposes rather than defining requirements for landing PRs.


23-26: Good addition of website team ownership

Adding specific ownership for website-related assets ensures that the right team is notified for relevant changes.


145-173: Comprehensive team ownership for new components

The additions for Test runner, Single Executable Apps, Permission Model, and Dependency Update Tools ensure proper team notifications for important subsystems.

.github/workflows/update-openssl.yml (1)

1-73: Well-structured OpenSSL update automation workflow

This workflow provides automated updating of OpenSSL dependencies with proper safety checks and a two-stage commit process (source update followed by regeneration of platform files).

Notable strengths:

  • Checks for existing update branches to prevent duplicate work
  • Properly parses version information to detect actual updates
  • Includes proper authentication and credentials handling
  • Separates source updates and regenerated files into distinct commits for clearer review
BUILDING.md (7)

1-9: Title and issue tracker references updated for N|Solid

The changes correctly update the document's title and GitHub issue tracker links to reflect N|Solid.


103-122: Comprehensive platform support table updates

The platform list has been thoroughly updated with N|Solid-specific support tiers and compatibility notes. This information is critical for users to understand which platforms are officially supported.


217-217: Updated Python version range

The Python version range has been extended to include 3.11 for building and testing.


559-559: Updated Python version link for Windows

The link to Python 3.11 from the Microsoft Store is provided for Windows users.


621-627: Android build instructions clarification

The Android build instructions have been improved to specify required parameters for SDK version and target architecture.


734-755: Clear OpenSSL configuration and FIPS compliance sections

The new sections on OpenSSL appname configuration and FIPS-compliant OpenSSL provide valuable information for users requiring these features.


780-817: Added guidance for shared dependencies

This new section properly explains the options and responsibilities for distributors who want to use shared dependencies at runtime rather than bundling them.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~781-~781: Did you mean: “By default,”?
Context: ... to use shared dependencies at runtime By default N|Solid is built so that all dependenci...

(BY_DEFAULT_COMMA)

.github/workflows/daily-wpt-fyi.yml (1)

57-59: Edge-case: extracting the nightly commit SHA can return an empty string

process.version.split(/-nightly\d{8}/)[1] assumes the version string always embeds a commit suffix.
If that format ever changes (e.g., nightly tags without SHA), $SHORT_SHA becomes an empty string and the subsequent gh api call fails.

Add a guard:

if [[ -z "$SHORT_SHA" ]]; then
  echo "::error::Unable to extract commit SHA from ${{ steps.setup-node.outputs.node-version }}"
  exit 1
fi
agents/grpc/src/command_stream.cc (2)

35-46: Destructor waits unbounded – add timeout to avoid hangs on force-push / CI cancel

If the underlying gRPC stream never calls OnDone (e.g., network partition), the conditional wait blocks forever, stalling thread shutdown and CI jobs.

Use uv_cond_timedwait with a finite timeout and log a warning:

const uint64_t kWaitMs = 10'000;
int r = uv_cond_timedwait(&on_done_cond_, lock_.base(),
                          uv_hrtime() / 1e6 + kWaitMs);
if (r == UV_ETIMEDOUT) {
  Debug("CommandStream::~CommandStream timed out waiting for OnDone\n");
  break;
}

70-84: ⚠️ Potential issue

OnReadDone early-exit path might leak the outstanding hold

When ok is false, you invoke StartWritesDone() and RemoveHold().
However, if ok is true but the observer expired (lines 72-78), neither branch releases the hold, producing an imbalance between AddHold() (constructor) and RemoveHold(). This prevents the stream from finishing and keeps the object alive.

Add RemoveHold() in that code path too:

-    if (obs) {
+    if (obs) {
       obs->on_command_received(std::move(server_request_));
       StartRead(&server_request_);
       return;
     }
+    // observer gone → drop hold
+    RemoveHold();

Likely an incorrect or invalid review comment.

agents/grpc/src/asset_stream.cc (1)

69-79: Race between WritesDone() and late enqueues

NextWrite() only finalises the stream when the queue is empty and
write_done_called is set. If a writer enqueues after this check but before
StartWritesDone() actually runs (possible without external back-pressure) the
asset will be discarded. Consider:

  1. Mark stream done (WritesDone()).
  2. Thread A enters NextWrite(), sees empty queue, proceeds to StartWritesDone().
  3. Thread B enqueues just after the emptiness check – asset lost.

Protect the enqueue/write_done_called order with the same mutex or add a
separate “closed” flag tested inside the queue push.

agents/grpc/src/proto/blocked_loop.pb.cc (1)

1-1642: Generated protobuf code – review skipped

This file is 100 % protoc-generated; manual changes will be overwritten.

agents/grpc/src/binding.cc (1)

92-95: Broken call ‑ Snapshot::StopTrackingHeapObjectsSync

After fixing the shadowing issue you still need to call the right helper.
StopTrackingHeapObjectsSync is defined on Snapshot objects, not in a
namespace. For synchronous stop you probably want:

-  Snapshot::StopTrackingHeapObjectsSync(GetLocalEnvInst(args.GetIsolate()))
+  node::profiler::Snapshot::StopTrackingHeapObjectsSync(
+      GetLocalEnvInst(args.GetIsolate()))

Please verify the intended API.

agents/grpc/src/proto/command.pb.cc (1)

1-1134: Generated file – manual review unnecessary

command.pb.cc is entirely generated code (// Generated by the protocol buffer compiler. DO NOT EDIT!).
Any modification would be overwritten the next time protoc is run, so there is no action required here.

agents/grpc/src/proto/command.pb.h (1)

1-1206: Generated file – manual review unnecessary

command.pb.h is also produced by protoc. Please avoid editing it by hand; any changes should be made in the corresponding .proto specification and regenerated instead.

.github/workflows/tools.yml (2)

291-298: Potential false negative in Generate commit message step

The condition relies on env.COMMIT_MSG == '', but COMMIT_MSG is only set via $GITHUB_ENV in a previous step.
GitHub evaluates env.* before processing the $GITHUB_ENV writes of earlier steps, so the variable will always look unset here, even when a message was produced upstream.
If multiple matrix jobs update the same PR, later jobs may overwrite a custom message.

Consider persisting the message in the PR branch or using the PR body to decide whether to regenerate.


8-40: ⚠️ Potential issue

workflow_dispatch.inputs.id list is out-of-sync with the matrix

minimatch, zlib, histogram, zlib, etc. appear in the matrix below (lines 209-260) but are missing from the choice list exposed to users.
Triggering the workflow with id=minimatch will therefore fail validation.

Add the missing ids or – better – derive the options automatically from the matrix to avoid future drift.

         options:
           - all
+          - minimatch
+          - zlib
+          - histogram
   ...

Likely an incorrect or invalid review comment.

Makefile (2)

36-56: convert_to_junit macro is conditionally re-defined — risk of silent override

convert_to_junit is defined once under ENABLE_V8_TAP (lines 36-39) and again under ENABLE_CONVERT_V8_JSON_TO_XML (lines 53-56).
If both env-flags are enabled simultaneously, the second definition silently overwrites the first, making it non-obvious which implementation will run.

Consider guarding the second definition or merging the logic:

ifndef convert_to_junit_defined
define convert_to_junitendef
convert_to_junit_defined := 1
endif

or make the two feature flags mutually exclusive and fail fast when both are set.


79-82: Hard-wiring NODE to the nsolid binary might break tooling

Changing the default executable from ./node to ./nsolid affects many developer scripts that assume a standard node name (e.g. npx, corepack, third-party tooling).
Please double-check that every script in the repo references $(NODE) (never the bare word node) and that downstream consumers/build-systems are aware of the rename.
A safer pattern is to keep NODE ?= ./node$(EXEEXT) and introduce a separate NSOLID variable, switching only where required.

agents/grpc/src/grpc_agent.cc (1)

1483-1508: parse_saas_token silently mutates the public key – verify intent

Replacing every ',' with '!' in the public-key portion (std::replace(pubKey.begin(), pubKey.end(), ',', '!');) alters the token before it is forwarded.
If the SaaS backend expects the original base-62/64 representation this will break authentication; if the mutation is intentional (e.g., URL-safe encoding) add an explanatory comment and possibly unit-test it.

Comment thread agents/grpc/proto/BUILD
@@ -0,0 +1,8 @@
~/software/protobuf/src/protoc \
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid hardcoded tilde path for protoc
Using ~/software/protobuf/src/protoc is not portable and may fail in CI or other developer environments. Reference protoc in $PATH or via an environment variable.

Apply for example:

-~/software/protobuf/src/protoc \
+${PROTOC:-protoc} \
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
~/software/protobuf/src/protoc \
${PROTOC:-protoc} \

Comment on lines +37 to +38
- run: ./tools/update-timezone.mjs

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Raise critical: Missing Node.js setup before running script
The update-timezone.mjs script requires Node.js, but there’s no actions/setup-node or similar step. Add:

+      - name: Set up Node.js
+        uses: actions/setup-node@v3
+        with:
+          node-version: '16'
       - run: ./tools/update-timezone.mjs
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- run: ./tools/update-timezone.mjs
- name: Set up Node.js
uses: actions/setup-node@v3
with:
node-version: '16'
- run: ./tools/update-timezone.mjs

Comment on lines +7 to +10
message Error {
string message = 1;
string stack = 2;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid generic message name Error
Using a top-level Error message can collide with generated or language-specific error types (std::error, Error in JS). Consider renaming to ExitError or namespacing it via a nested message to reduce collision risk.

Comment on lines +12 to +16
message ExitBody {
int32 code = 1;
Error error = 2;
string profile = 3;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Use bytes for binary profile data
If profile contains serialized binary payloads (e.g., pprof data), prefer bytes profile = 3; to avoid UTF-8 validation and size limits of string.


jobs:
v8-update:
if: github.repository == 'nodejs/node'
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Update repository condition.
The conditional if: github.repository == 'nodejs/node' prevents this workflow from running in nodesource/nsolid. Adjust or remove this filter to target the correct repo.

Comment on lines +112 to +123
// Call the correct async export method on the stub
(stub->async()->*exportFunc)(call_data->grpc_context.get(),
call_data->event,
call_data->event_response,
[call_data](::grpc::Status status) {
call_data->grpc_status = status;
call_data->result_callback(call_data->grpc_status,
std::move(call_data->arena),
*call_data->event,
call_data->event_response);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Argument-evaluation order can cause use-after-free

The lambda passes std::move(call_data->arena) before *call_data->event:

call_data->result_callback(call_data->grpc_status,
                           std::move(call_data->arena),   // 1️⃣ move-out
                           *call_data->event,              // 2️⃣ deref
                           call_data->event_response);

Because C++17 still leaves argument evaluation order unspecified, the move could occur first.
If the user’s callback immediately resets the unique_ptr, the subsequent dereference of event becomes undefined.

Mitigation: pass the arena last (or pass a raw pointer):

- call_data->result_callback(status, std::move(call_data->arena),
-                            *call_data->event, call_data->event_response);
+ call_data->result_callback(status,
+                            *call_data->event,
+                            call_data->event_response,
+                            std::move(call_data->arena));

Don’t forget to adjust the callback signature accordingly.

Comment thread Makefile
Comment on lines +49 to +52
TAP_V8 := --json-test-results $(TAP_V8_JSON) --slow-tests-cutoff 1000000
TAP_V8_INTL := --json-test-results $(TAP_V8_INTL_JSON) --slow-tests-cutoff 1000000
TAP_V8_BENCHMARKS := --json-test-results $(TAP_V8_BENCHMARKS_JSON) --slow-tests-cutoff 1000000

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Variable reuse may clobber CLI flags

TAP_V8, TAP_V8_INTL, TAP_V8_BENCHMARKS are first used to hold file paths (JSON outputs) and then immediately reassigned to CLI flags (--json-test-results …).
Overloading one variable for two unrelated meanings hurts readability and can break other targets that rely on the original value.

Prefer distinct names, e.g.:

TAP_V8_JSON_PATH := $(PWD)/v8-tap.json
TAP_V8_FLAGS     := --json-test-results $(TAP_V8_JSON_PATH)

Comment on lines +84 to +86
void set_asset_cb(SharedEnvInst envinst,
const v8::Local<v8::Function>& cb);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Thread-safety of asset_cb_map_

set_asset_cb() inserts into asset_cb_map_, and callbacks may be invoked from multiple libuv threads.
std::map is not thread-safe for concurrent reads/writes.
If these mutations/read occur on different threads (e.g. agent thread vs JS main loop), protect the map with a mutex or switch to a concurrency-friendly structure.

Comment on lines +367 to +370
it = config.find("statsdBucket");
if (it != config.end()) {
body->set_statsdbucket(*it);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Wrong field setter: statsdtags is stored into blockedLoopThreshold

PopulateReconfigureEvent accidentally calls set_blockedloopthreshold() when the JSON field is statsdtags.

-  it = config.find("statsdtags");
-  if (it != config.end()) {
-    body->set_blockedloopthreshold(*it);
-  }
+  it = config.find("statsdtags");
+  if (it != config.end()) {
+    body->set_statsdtags(*it);  // proto has `statsdtags` field
+  }

Without this fix the console receives an empty statsdtags array and an unexpected value in blockedLoopThreshold.

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +1420 to +1428
if (asset_size > GRPC_MAX_SIZE) {
// Split the data into chunks
Debug("Asset size larger than supported (%ld > %ld): "
"splitting profile into chunks\n", asset_size, GRPC_MAX_SIZE);
size_t prof_size = stor.profile.size();
size_t rest = asset_size - prof_size;
size_t offset = 0;
size_t chunk_size = GRPC_MAX_SIZE - rest - 100;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Potential zero / negative chunk_size in profile chunking logic

chunk_size is computed as GRPC_MAX_SIZE - rest - 100.
If restGRPC_MAX_SIZE - 100, the result underflows (wraps to a very large size_t) causing an infinite loop or std::out_of_range on substr.
Consider clamping the result:

-      size_t chunk_size = GRPC_MAX_SIZE - rest - 100;
+      size_t chunk_size = GRPC_MAX_SIZE > (rest + 100)
+                            ? GRPC_MAX_SIZE - rest - 100
+                            : 1024 * 256;               // fall back to 256 kB
+      if (chunk_size == 0) chunk_size = 1024 * 256;     // safety net

Also log a warning when the fallback path is taken so oversized metadata is visible to operators.

@santigimeno santigimeno changed the base branch from node-v22.x-nsolid-v5.x to node-v18.x-nsolid-v5.x May 8, 2025 21:09
@santigimeno santigimeno merged commit b9e7e7e into node-v18.x-nsolid-v5.x May 12, 2025
2 checks passed
@santigimeno santigimeno deleted the node-v18.20.8-nsolid-v5.7.1-release branch May 12, 2025 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.