jamie-bitflight/code-review-nodejs

Node.js Code Review Patterns

Stack-specific rules loaded by dh:code-reviewer when package.json and *.js/*.mjs files are detected (without TypeScript).

Synchronous I/O in Request Path

• fs.readFileSync, fs.writeFileSync, execSync, spawnSync in any function called during request handling are blocking findings
• Synchronous I/O blocks the event loop and degrades all concurrent requests
• All file system operations in server code must use the async variants or fs/promises

// WRONG: blocks event loop
app.get("/config", (req, res) => {
  const config = fs.readFileSync("./config.json", "utf8");
  res.json(JSON.parse(config));
});

// RIGHT: non-blocking
app.get("/config", async (req, res) => {
  const config = await fs.promises.readFile("./config.json", "utf8");
  res.json(JSON.parse(config));
});

Stream Backpressure

• Piping streams without handling backpressure is a blocking finding for high-throughput paths
• readable.pipe(writable) handles backpressure automatically — prefer it over manual data event listeners
• Manual data event listeners must check writable.write() return value and pause the readable when it returns false

Process Exit

• process.exit() is only acceptable in CLI entrypoints — it is a blocking finding in library code, route handlers, or middleware
• Unhandled process.on("uncaughtException") that calls process.exit() without logging the error is a blocking finding

Security

• eval() is a blocking finding everywhere — no exceptions
• new Function(code) with user-controlled code is a blocking finding
• Shell arguments constructed by string concatenation with user input before passing to exec or spawn are a blocking finding
• execFile is required over exec when calling external programs — exec invokes a shell and is vulnerable to injection
• User-controlled values used as file paths must be validated against an allowed base directory (path traversal)

// WRONG: shell injection vector
exec(`convert ${userInput} output.png`);

// RIGHT: no shell, explicit args
execFile("convert", [userInput, "output.png"]);

Dependency Hygiene

• * version ranges in package.json are a blocking finding — they produce non-reproducible installs
• ^ ranges are acceptable; ~ is preferred for stricter patch-level pinning
• package-lock.json or yarn.lock must be committed — without a lockfile, versions are not reproducible in CI
• Dev-only dependencies must be in devDependencies, not dependencies — they inflate production bundle size

Event Emitter Cleanup

• EventEmitter.on() listeners added in component/connection lifecycle must be removed when that lifecycle ends
• Missing removeListener or off() calls are a blocking finding when the emitter outlives the listener
• Use EventEmitter.once() for one-shot listeners to avoid manual cleanup

Environment Variables

• All required environment variables must be validated at startup, before the server begins accepting requests
• process.env.SOME_VAR! without validation is a blocking finding — the app will fail with a confusing error at runtime rather than a clear startup message
• Provide a .env.example file listing all required variables — checked in, never containing real values

execFile Over exec

// WRONG: shell injection risk
exec(`git log --oneline ${branch}`);

// RIGHT: explicit argument array, no shell
execFile("git", ["log", "--oneline", branch], (err, stdout) => { ... });

Anti-Patterns

// WRONG: missing error handling on EventEmitter
server.on("connection", (socket) => {
  socket.on("data", handleData);
  // missing: socket.on("end", cleanup) and removeListener
});

// WRONG: unvalidated env at use site
const apiKey = process.env.API_KEY;
fetch(url, { headers: { Authorization: apiKey } }); // null if unset

// RIGHT: validate at startup
if (!process.env.API_KEY) {
  console.error("FATAL: API_KEY environment variable is required");
  process.exit(1);
}
const apiKey = process.env.API_KEY;