Guide · Node

How to review Node pull requests: the 9 defect classes with examples

Node services fail asynchronously: a rejected promise nobody awaited, a callback API called as if it returned a promise, or one bad item aborting a batch. This guide walks DiffDojo's nine defect classes as they show up in Node and Express code.

1. Security

Every route that loads a record by id needs an ownership check, not only authentication. Also look for user input in SQL strings, in child_process calls, in file paths built from user input without resolving them and checking they stay inside the intended directory, and in redirects.

Worked example:

• IDOR: any logged-in user can read every other user's order

2. Requirement fidelity

Pagination, sorting and the response shape are where Node APIs most often diverge from the spec: a total that is never returned, so the client cannot build the control the ticket asked for, or a filter applied after the page is cut. Compare each field the client needs with what the route returns before reading how it computes them.

Worked example:

• Requirement mismatch: the promised 'jump to last page' control can't be built

3. Edge cases & error handling

Ask what one failure does to the rest: in an import or a batch, a single bad row should be recorded and skipped, not abort the loop. Check timeouts on outbound calls and what happens on an empty input.

Worked example:

• Missing error path: one bad row aborts the whole import

4. Hallucinated APIs & dependencies

Invented options are common: an HTTP client configured with a retries option it does not have, or a callback-style function (fs.stat from fs, not fs/promises) awaited as if it returned a promise. Check each call against the installed version.

Worked examples:

• Hallucinated API: axios has no retries option, so nothing ever retries
• Wrong API flavor: awaiting callback-style fs.stat crashes every call

5. State & data flow

Module-level variables are shared by every request in the process. A cache keyed wrongly, or a variable reused across requests, mixes one user's data into another's response.

6. Performance

Look for one query per item (N+1), Promise.all over an unbounded list that opens thousands of connections at once, and synchronous file or crypto calls on the request path. All three work on a laptop with a small dataset, so ask what happens at the size and concurrency the service sees in production.

7. Data correctness

JavaScript numbers are floats: money needs integer minor units, and rounding must happen once, where the spec says. Dates without an explicit time zone are another source of off-by-a-day results.

8. Deceptive tests

A test that stubs the function under test, or forgets to await the assertion, passes regardless of the code. Check that each test can fail.

9. Over-engineering

A dependency for something the standard library already does, or an abstraction with one implementation, adds supply-chain and maintenance cost. Ask whether it is needed.

The checklist

1. Is every id-based lookup scoped to the caller, and no input reaching SQL, a shell or a path?
2. Does the response give the client everything the ticket needs?
3. Does one failing item stop the batch, and do outbound calls time out?
4. Does every option and function exist, in the flavour (callback or promise) used?
5. Is any module-level variable holding per-request data?
6. Any query per item, or unbounded concurrency?
7. Is money in integer minor units and rounding done once?
8. Does every test await what it checks, and can it fail?
9. Is each new dependency or layer necessary?

As of October 2026, the DiffDojo library has 60 live Node PRs to practise on, each graded on whether you catch the planted bug or approve the clean one.

All review guides · All code review examples