Node.js code review standards

# Node.js code review standards

Follow these standards when writing or changing code in this repository. Code review flags changes that break them.

## Prevent Event Loop Blocking in Node.js

Applies to `**/*.js`, `**/*.mjs`, `**/*.cjs`, `**/*.ts`, `**/*.mts`, `**/*.cts`.

Review Node.js server code, middleware, and queue consumers for work that blocks the event loop.

Flag changes that:
- Call synchronous fs APIs such as `readFileSync`, `writeFileSync`, `readdirSync`, or `existsSync` inside request handlers, middleware, or per-message consumer code.
- Hash or derive keys on the request path with `crypto.pbkdf2Sync`, `crypto.scryptSync`, `crypto.generateKeyPairSync`, `bcrypt.hashSync`, or `bcrypt.compareSync`.
- Compress or decompress request-sized data with `zlib` sync methods such as `gzipSync`, `gunzipSync`, or `brotliCompressSync`.
- Run `execSync`, `execFileSync`, or `spawnSync` from `child_process` in a server process.
- Add CPU-bound loops, sorts, or nested iteration whose cost grows with request or database input and has no size cap.
- Apply regular expressions with nested quantifiers, such as `(a+)+` or `([a-z]+)*$`, to user-controlled strings.
- Call `JSON.parse` or `JSON.stringify` on the request path for payloads that can reach many megabytes, such as full exports or unpaginated query results.

Do not flag:
- Synchronous calls at startup or module load, or in CLI scripts, build tooling, and tests.
- Work already moved to `worker_threads`, a worker pool such as Piscina, or a background job queue.
- Sync calls on small, fixed-size inputs, such as hashing a short token with `createHash`.

## Handle Async Errors and Unhandled Rejections in Node.js

Applies to `**/*.js`, `**/*.mjs`, `**/*.cjs`, `**/*.ts`, `**/*.mts`, `**/*.cts`.

Review Node.js async code for promise rejections and errors that escape every handler.

Flag changes that:
- Call a promise-returning function without `await`, `return`, or a `.catch()` handler, including inside `setTimeout` and `setInterval` callbacks.
- Pass an async callback to `forEach` or another method that ignores its return value, so failures go unhandled and the caller continues before the work finishes.
- Return a promise from inside a `try` block without `await`, so the `catch` block never sees its rejection.
- Register async listeners with `on()` or `once()` on EventEmitters, streams, or sockets without catching errors inside the listener.
- Add async route handlers or middleware to Express 4, or another framework that ignores returned promises, without a try/catch that calls `next(err)` or a wrapper that does.
- Swallow rejections with `.catch(() => {})` or an empty `catch` on writes or external calls whose failure the caller needs to know about.
- Add `process.on('uncaughtException')` or `process.on('unhandledRejection')` handlers that log and keep the process serving requests.

Do not flag:
- Async handlers in frameworks that forward rejections to their error handling, such as Express 5, Fastify, Koa, and NestJS.
- Deliberate fire-and-forget calls that attach a `.catch()` which logs or reports the error.
- Async listeners on emitters created with `captureRejections: true` that also have an `error` listener or a `Symbol.for('nodejs.rejection')` method to receive the rejection. Without one, the forwarded error still crashes the process.

## Require Timeouts and Bounded Retries on Node.js HTTP Calls

Applies to `**/*.js`, `**/*.mjs`, `**/*.cjs`, `**/*.ts`, `**/*.mts`, `**/*.cts`.

Review Node.js outbound HTTP calls for missing deadlines and unsafe retries.

Flag changes that:
- Call `fetch` in request handlers, jobs, or queue consumers without a `signal`, such as `AbortSignal.timeout(ms)`.
- Create axios instances or make axios requests without a `timeout` or `signal`; axios applies no timeout by default.
- Call `http.request` or `https.request` without a `timeout` or `signal`, or set a `timeout` or call `req.setTimeout()` without destroying the request in the `'timeout'` handler; the option only emits an event.
- Clear a manual abort timer as soon as `fetch` resolves, which leaves `res.json()`, `res.text()`, or body streaming with no overall deadline.
- Retry in a loop without a maximum attempt count, or without backoff and jitter between attempts.
- Retry non-idempotent requests, such as payments, order creation, or emails, without an idempotency key, or retry 4xx responses other than 408 and 429.

Do not flag:
- Calls through a shared client or wrapper that already sets a timeout.
- Long downloads or streaming requests with a deliberately large, explicit timeout.
- SDK clients configured with their own timeout option at construction.

## Stream Large Payloads Instead of Buffering in Node.js

Applies to `**/*.js`, `**/*.mjs`, `**/*.cjs`, `**/*.ts`, `**/*.mts`, `**/*.cts`.

Review Node.js code that handles files, uploads, downloads, and HTTP bodies for payloads loaded fully into memory.

Flag changes that:
- Read files of unbounded size, such as user uploads, exports, logs, or backups, with `fs.readFile` or `readFileSync` to send, upload, or transform them instead of using `fs.createReadStream`.
- Buffer upstream responses with `arrayBuffer()`, `text()`, `blob()`, or `Buffer.concat` over every chunk when proxying or saving files of unbounded size.
- Collect a request body with `req.on('data')` into a string or array without enforcing a maximum byte count.
- Raise body parser limits far above the defaults, such as `express.json({ limit: '500mb' })` or a large Fastify `bodyLimit`, to accept file-sized payloads.
- Accept multipart uploads with in-memory storage, such as multer `memoryStorage()`, without `limits.fileSize`.
- Connect streams with `.pipe()` and no error handling, where `pipeline()` from `node:stream/promises` would propagate errors and destroy every stream in the chain.
- Build a full export, CSV, or report as one string or array from an unbounded query instead of streaming rows.

Do not flag:
- Reading small, bounded files such as config, templates, or fixtures.
- Parsing JSON API responses with a known, small size.
- Body limits left at framework defaults or lowered.

## Prevent Command Injection and Path Traversal in Node.js

Applies to `**/*.js`, `**/*.mjs`, `**/*.cjs`, `**/*.ts`, `**/*.mts`, `**/*.cts`.

Review Node.js code that runs external commands or builds file paths for injection through untrusted input.

Flag changes that:
- Pass strings containing request data, filenames, branch names, or other external input to `child_process.exec` or `execSync`, which run the command through a shell.
- Call `spawn` or `execFile` with `shell: true` and arguments built from external input.
- Pass external values as command arguments that could start with `-` without a `--` end-of-options separator or validation, allowing option injection such as `--upload-pack` in `git`.
- Join or resolve user-supplied names, such as route params, query strings, upload filenames, or archive entry names, with `path.join` or `path.resolve` and use the result without checking that it stays inside the intended base directory.
- Check containment with `startsWith(baseDir)` without a trailing separator, so `/srv/uploads-old` passes a check for `/srv/uploads`.
- Sanitize paths with string replacement such as `.replace('../', '')`, which nested sequences like `....//` defeat.

Do not flag:
- `exec` calls whose command string is a constant with no external input.
- Paths built only from server-generated values, such as UUIDs or content hashes.
- Express `res.sendFile(name, { root })`, which rejects `..` segments when `root` is set.
- Build scripts, dev tooling, and tests that take arguments from the developer.

## Flag Security Vulnerabilities

Scan for security flaws: hardcoded secrets, SQL injection, XSS, weak auth, exposed data, poor crypto, unsafe deserialization, insecure HTTP calls in production, and more. Ensure all network requests use HTTPS/TLS. Identify attack vectors and security violations to maintain robust defenses.

## Require API Auth, Validation, and Tenant Scoping

Review API handlers and controller endpoints for missing authentication, authorization, tenant scoping, and request validation.

Flag new or modified endpoints that:
- Accept request bodies without runtime validation, such as Zod schemas, DTOs, or framework-native validators.
- Trust user-supplied identifiers for tenant, organization, or account access instead of deriving access from the authenticated session.
- Return sensitive internal errors or use inconsistent HTTP status codes for 400, 401, 403, 404, and 500 cases.
- Mix reads and writes in a way that makes side effects hard to reason about.
- Add public or high-volume endpoints without rate limiting or abuse controls.

## Prevent Sensitive Data in Logs

Logging, error reporting, analytics events, and audit trails must not expose sensitive data. Flag code that logs PII, emails, passwords, API keys, auth tokens, session identifiers, payment data, or raw request/response bodies without redaction. Error serialization must preserve enough context to debug while sanitizing secrets and user data.

## Prevent Database Query Performance Regressions

Flag database access patterns that can degrade production performance, including N+1 queries, unbounded list endpoints, missing pagination, full-table scans, missing indexes on selective filters, per-row writes in loops, and queries that load more columns or rows than needed. Prefer bounded reads, batching, selective indexes, and query shapes that scale with table size.

## Enforce Pragmatic Test Coverage

Be reasonable when evaluating tests. Flag changes when important behavior is untested, but do not require tests for every export, type, config, metadata file, or mechanical change.

Flag PRs that:
- Change core business logic without covering the main success and failure paths.
- Add UI behavior without testing the user-visible interaction or state change.
- Add tests that only assert rendering, existence, snapshots, or that a function returns any value.
- Skip, mute, or weaken existing tests without a clear reason.
- Mock so much of the system that the test no longer validates the behavior the PR claims to change.

## Block Unsafe Database Migrations

Review database migrations for operations that can lock large tables, rewrite full tables, run long backfills, create blocking indexes, or require full table scans. Prefer phased rollouts, concurrent indexes, nullable columns followed by backfills, batched data changes, and explicit rollback or recovery plans for risky schema changes.

## Enforce Strict Maintainability Standards

Review changes for structural maintainability regressions. Behavior-correct code can still violate this rule if it makes the codebase harder to understand, extend, or decompose.

Be ambitious about structural simplification. Look for reframings that delete whole branches, helpers, modes, conditionals, or layers instead of adding more of them. Prefer solutions that remove complexity rather than rearrange it.

Flag PRs that:
- Push a file from under 1,000 lines to over 1,000 without a strong reason or clear organization. Prefer extracting helpers, subcomponents, modules, or local abstractions first.
- Add ad-hoc conditionals, scattered special cases, one-off booleans, nullable modes, or flags into already busy shared flows instead of a dedicated abstraction, helper, state machine, policy object, or separate module.
- Introduce thin wrappers, identity abstractions, or pass-through helpers that add indirection without reducing complexity.
- Use brittle, ad-hoc, or "magic" mechanisms that hide simple data-shape assumptions.
- Rely on unnecessary casts, any, unknown, or optional parameters when a clearer type boundary or explicit model would simplify control flow.
- Use silent fallbacks to paper over unclear invariants instead of making the boundary explicit.
- Duplicate existing helpers or put logic in the wrong layer when a canonical utility or module already owns the concept.
- Leak feature-specific logic into general-purpose or shared paths.
- Copy-paste logic instead of extracting a shared helper or pure function.
- Handle narrow edge cases in the middle of an already busy function.
- Refactor in ways that move complexity around without reducing the number of concepts a reader must hold in their head.
- Add "temporary" branching that is likely to become permanent debt.
- Serialize obviously independent work or leave related updates in a non-atomic shape when a cleaner structure is available.

Prioritize findings in this order:
1. Structural code-quality regressions
2. Missed opportunities for dramatic simplification
3. Spaghetti or branching complexity increases
4. Boundary, abstraction, or type-contract problems
5. File-size and decomposition concerns
6. Modularity and legibility concerns

Do not flag cosmetic nits when a larger structural issue is present. Prefer a smaller number of high-conviction findings over a long list of minor notes. Only report violations on changed lines with clear evidence from the diff.