A review checklist for AI-written code (what agents actually break)

Correctness failures: status and tests

1. Pipelines hide the exit code that matters

A generated CI script often pipes a test command through tee for readable logs. In Bash, the pipeline normally returns the status of its last command. If the tests fail but tee succeeds, the job reports success.

# Broken: npm test can fail while the script exits 0.
npm test 2>&1 | tee test.log

# Correct for Bash: make any failed pipeline stage fail the script.
set -Eeuo pipefail
npm test 2>&1 | tee test.log

# When you need the individual statuses:
set +e
npm test 2>&1 | tee test.log
test_rc=${PIPESTATUS[0]}
set -e
exit "$test_rc"

Review the shell named by the shebang; POSIX sh does not guarantee Bash's pipefail or PIPESTATUS. Also inspect subshells, command substitutions, and cleanup traps that may overwrite the original status. The concrete test is simple: replace the main command with false and require the script and CI step to fail.

2. Tests execute but cannot fail

Agents frequently add a test whose name describes the intended behavior while its assertion permits the bug. Here a missing authenticated resource returns 404, yet the test passes because it only excludes server errors.

it("rejects anonymous requests", async () => {
  const response = await request(app).get("/account");
  expect(response.status).toBeLessThan(500); // 200 and 404 both pass
});

it("rejects anonymous requests with the public error shape", async () => {
  const response = await request(app).get("/account");
  expect(response.status).toBe(401);
  expect(response.body).toEqual({ error: "authentication_required" });
});

Look for tests with no assertion, broad ranges, snapshots accepted without inspection, catches that log and continue, and mocks that reproduce the implementation rather than the public contract. Mutation-check the test manually: change the handler to return 200 and confirm this specific test fails for the right reason.

Scope and data failures: oversized diffs and unsafe locks

3. Scope creep hides inside helpful cleanup

Suppose the request is “reject an empty invoice ID in one API route.” The agent also renames a shared helper, reformats 24 files, upgrades a validation dependency, and changes unrelated error strings. Every extra surface increases regression risk and makes the required behavior harder to review.

git diff --stat
git diff --name-status
git diff -- src/routes/invoices.ts test/invoices.test.ts

# Find dependency and generated-file movement separately.
git diff -- package.json package-lock.json
git status --short

Write the allowed change set before reading implementation details: route, focused test, and perhaps one shared validator. Ask why every other file changed. A defensible answer must tie the file to an acceptance criterion. “The agent noticed it” is not evidence. Revert unrelated formatting and refactors into a separate pull request, even when they are individually sensible.

4. A correct migration takes the wrong production lock

Database code can be logically correct and operationally dangerous. A common PostgreSQL example is adding a normal index to a busy table. CREATE INDEX allows reads but blocks writes while it scans the table. The migration passed on an empty test database because there was no concurrent traffic.

-- Risky on a hot, large table:
CREATE INDEX idx_events_account_id ON events (account_id);

-- Lower write-blocking risk in PostgreSQL:
CREATE INDEX CONCURRENTLY idx_events_account_id
  ON events (account_id);

Concurrent index creation takes longer, performs more work, and cannot run inside a transaction block. A failed attempt may also leave an invalid index that needs explicit cleanup. Review the database engine and version, estimated row count, lock level, transaction wrapper, statement timeout, rollback behavior, and deployment order. Rehearse while another session holds or waits on representative writes; an empty-schema migration test does not measure lock impact.

Security and observability failures

5. Untrusted input crosses into an interpreter

Generated code often preserves the happy-path command while changing how arguments reach it. Template interpolation turns a commit supplied by an API caller into shell syntax:

// Broken: commit can contain shell metacharacters.
exec(`git show --stat ${commit}`, callback);

// Better: validate the expected identifier and avoid a shell.
if (!/^[0-9a-f]{40}$/i.test(commit)) {
  throw new TypeError("commit must be a full hexadecimal object ID");
}

execFile(
  "git",
  ["show", "--no-ext-diff", "--stat", commit],
  { timeout: 10_000, maxBuffer: 1_000_000 },
  callback
);

Argument arrays remove shell interpretation, but they do not validate business meaning or stop every called program from parsing option-like values. Constrain the input to the smallest valid language, pass it through a non-shell API, limit runtime and output, and test quotes, whitespace, leading hyphens, separators, Unicode, oversized values, and empty input. Repeat the same review at SQL, HTML, regular-expression, template, path, and URL boundaries.

6. Error handling quietly turns failure into valid data

A catch-all fallback makes a dashboard look stable while the dependency is down. Callers cannot distinguish “there are no orders” from “we failed to load orders,” so alerts, retries, and user messages all disappear.

// Broken: outage becomes a valid empty result.
async function listOrders(customerId) {
  try {
    return await store.findOrders(customerId);
  } catch (error) {
    logger.warn("order lookup failed");
    return [];
  }
}

// Preserve the failure and add safe context.
async function listOrders(customerId) {
  try {
    return await store.findOrders(customerId);
  } catch (error) {
    throw new OrderLookupError("order lookup failed", { cause: error });
  }
}

Review every new catch, promise rejection handler, optional chain, default value, retry, and background callback. Ask whether the fallback is part of the product contract. If it is, require a metric and a visible degraded state. If it is not, preserve the typed error and map it once at the system boundary without logging secrets or customer data.

Maintenance failures: dead paths and stale instructions

7. The new path lands but the replaced code remains alive

An agent may introduce retryV2, switch the primary caller, and leave legacyRetry, its feature flag, tests, configuration keys, and exported types behind. The build stays green, but maintainers now have two apparent sources of truth and security fixes may land in only one.

rg -n "legacyRetry|ENABLE_LEGACY_RETRY|retry_timeout_ms" \
  src test config docs

# Language-specific checks may reveal more:
npx tsc --noEmit --noUnusedLocals --noUnusedParameters
npx eslint . --max-warnings 0

Trace both directions: who calls each new symbol, and what the old symbol still reaches. Remove obsolete code in the same change when compatibility is not required. If it is required, document the caller, removal condition, owner, and date. “Keep it just in case” creates code that no test proves and no one confidently deletes.

8. Documentation describes a command the code no longer accepts

Agents update the implementation and nearby comments but miss README examples, sample configuration, runbooks, copied snippets, and generated help. For example, the code renames timeout_ms to timeoutSeconds, while the deployment guide still supplies the old key. The service starts with a default, so CI never notices the ignored operator value.

rg -n "timeout_ms|timeoutSeconds" \
  README.md docs examples config src test

# Exercise the exact public examples in a clean environment.
node ./bin/service.js --config examples/service.json --check
node ./bin/service.js --help > /tmp/current-help.txt
diff -u docs/cli-help.txt /tmp/current-help.txt

Treat documentation commands and examples as executable interfaces. Run them from a fresh checkout with only documented prerequisites. Search old and new names across the entire repository, check environment-variable tables and defaults, and verify upgrade and rollback instructions. A prose review alone will not reveal a flag that the parser silently ignores.

A compact pull-request checklist

1. Exit: force the primary command to fail and confirm the script, job, and caller retain its nonzero status.
2. Tests: break the implementation deliberately and confirm the new test becomes red for the intended reason.
3. Scope: map every changed file to a requirement; separate unrelated cleanup.
4. Migration: identify production lock behavior, transaction limits, runtime, retry, rollback, and partial-failure residue.
5. Injection: trace every untrusted value into shells, queries, templates, paths, URLs, and parsers.
6. Errors: ensure failures remain distinguishable from valid empty, false, zero, or default results.
7. Dead code: search old names, flags, exports, tests, and configuration after the replacement lands.
8. Docs: run public commands and examples from a clean environment and compare generated help.

Finish with the repository's real formatter, static analysis, tests, migration checks, and a focused manual exercise of the changed behavior. Then inspect the final diff again. Generated code deserves the same authorship accountability as handwritten code: the reviewer accepts the behavior, not the explanation of how the patch was produced.