---

name: nodejs-review description: | Reviewing Node.js runtime code - what to flag beyond the language, and what the toolchain already flags

USE WHEN: you are reviewing, critiquing or auditing existing Node.js code - a "code review" of server code, a CLI, a script, a diff or a PR - and the concern is the RUNTIME: the event loop, streams, processes, file handles, EventEmitter, unhandled rejections

DO NOT USE FOR: language-level review - use review/typescript for typed code; Express/NestJS/Fastify routing - use the framework skills; writing or learning Node - use languages/nodejs; browser JavaScript, which has none of this allowed-tools: Read, Grep, Glob

Reviewing Node.js

This skill is about the runtime, not the language. Type and syntax findings belong in review/typescript; what follows are defects that exist because the code runs on a single-threaded event loop with an async I/O layer, and would not exist in the same code running anywhere else.

The organising fact: one process, one thread for your code. Almost every check below is a way of occupying that thread, or of losing track of work scheduled on it.

The snippets are fragments cut down to the defect, not runnable programs.

Already covered - do not spend review on it

Defect	Reported by
Unhandled promise rejection	the runtime — crashes the process since Node 15, so it is loud
A promise created and not awaited	@typescript-eslint/no-floating-promises — type-aware, so only if configured
require of a missing module	the runtime, at load
Callback called twice, callback never called	nothing
Blocking the event loop	nothing
A stream written faster than it drains	nothing
A listener added per request	nothing until the MaxListenersExceededWarning, which is a warning about a symptom

Rows 4 to 7 are the reason this skill exists: the runtime's most expensive mistakes have no diagnostic at all.

The checks that earn their place

Synchronous I/O on a request path

const cfg = fs.readFileSync(path);        // blocks every other request
const hash = crypto.pbkdf2Sync(pw, salt, 100000, 64, 'sha512');

When you see it: any *Sync call, execSync, or a CPU-bound loop inside a handler, a middleware, or a module that runs per request.

Ask: is this on the startup path or the request path? At startup, Sync is often the right choice and worth no comment. Per request, it stops the entire process — not just this request — and the latency lands on unrelated endpoints, which is why it is so hard to diagnose from the symptom.

A stream piped without backpressure

source.on('data', chunk => dest.write(chunk));   // ignores write() returning false

When you see it: a manual data/write loop instead of pipe or pipeline; res.write in a loop over a large result set.

Ask: what happens when the destination is slower than the source? write returning false means "buffer full"; ignoring it grows the buffer in memory until the process dies. pipeline() also propagates errors and destroys the streams, which a bare pipe does not.

An error listener that is not attached

const s = fs.createReadStream(p);
s.pipe(res);                                // no 's.on("error", ...)'

When you see it: a stream, a socket, or a child process without an error handler.

Ask: what happens on ENOENT? An error event with no listener is rethrown as an uncaught exception and takes the process down. pipeline(a, b, cb) is the form that routes it.

A listener added per request or per call

app.get('/x', (req, res) => {
  emitter.on('update', () => res.write('...'));   // never removed
});

When you see it: .on(...) inside a handler, a loop, or a reconnect path; process.on('unhandledRejection', ...) registered in a module loaded many times.

Ask: who removes this? Listeners accumulate on the emitter, hold the closure and everything it captured, and the only signal is a MaxListenersExceededWarning at 11 — which is a warning about the leak's size, not its existence. once or an explicit off in a finally.

process.exit() with output still buffered

console.log(report);
process.exit(0);          // stdout to a pipe is async: the write can be lost

When you see it: process.exit immediately after writing output, or inside a callback.

Ask: is stdout a TTY or a pipe here? To a terminal it is synchronous and the output appears; to a pipe or a file it is asynchronous, and exit truncates it. The bug shows up only when the CLI is used in a script — which is the case that matters. Setting process.exitCode and returning lets the loop drain.

spawn with a shell, or with unvalidated arguments

exec(`convert ${userFile} out.png`);        // a shell parses this
spawn('convert', [userFile, 'out.png']);    // no shell: argv is passed directly

When you see it: exec, execSync, or spawn(..., { shell: true }) with anything interpolated.

Ask: does a shell see this string? exec always spawns one, so ;, $() and backticks in the value are commands. spawn with an argv array and shell: false (the default) removes the whole category — and is also faster.

Unbounded concurrency in a fan-out

await Promise.all(urls.map(u => fetch(u)));   // 10,000 sockets at once

When you see it: Promise.all over an array whose length comes from data rather than from a constant.

Ask: how large can this get? Node will happily open every connection at once, exhausting file descriptors or the remote's rate limit. A concurrency limit makes the behaviour a decision instead of an accident. (Promise.all's partial-failure semantics are a separate concern — see review/typescript.)

A file handle or timer left open on the error path

const fh = await fs.promises.open(p);
const data = await parse(fh);        // throws: the handle is never closed
await fh.close();

When you see it: open, setInterval, a socket or a DB connection acquired without try/finally.

Ask: does every path release it? An open handle keeps the event loop alive, so the symptom is often a process that will not exit rather than an error. An un-unref'd setInterval does the same thing.

Module-scope state in a multi-instance deployment

let cache = {};                    // per process, not per app
export function get(k) { return cache[k]; }

When you see it: mutable module-level state used as a cache, a rate limiter, a session store, or a lock.

Ask: how many processes run this? Under cluster, PM2, or any horizontally scaled deployment, each has its own copy — so a rate limiter allows N times the limit and a lock locks nothing. It works perfectly on one dev machine.

Version- and deployment-dependent

What to read	Why it changes the review
engines in package.json, the CI Node version	Unhandled rejections terminate the process from Node 15; fetch is global from 18; --experimental-strip-types and stable test runner from 20/22. Do not suggest below the floor
type: module vs CommonJS	Decides whether top-level await exists and whether __dirname does
Is this clustered or replicated?	Decides whether the module-state finding is real
Is it a CLI or a server?	*Sync at startup is fine in both; process.exit truncation matters mainly for CLIs whose output is piped
Worker threads available?	The answer to CPU-bound work, and it changes the advice for the blocking check

What to say

Anchor the comment to the line, name the condition that triggers the defect, and say what breaks: "pbkdf2Sync here blocks the event loop for ~100 ms, so every concurrent request — including health checks — stalls behind it" beats "avoid sync APIs". If you cannot state the input that fails, it is a preference, not a defect.

Distinguish startup from request path before commenting on anything synchronous. That single distinction removes most false positives in Node review.