Most AI-generated code is not obviously wrong. That is what makes reviewing it harder than reviewing human code.
A human engineer leaves fingerprints: a rushed function, an inconsistent name, a comment that openly admits the hack. A model leaves none. It produces uniform, confident output. Tidy naming, plausible structure, comments that explain intent with total assurance. The surface looks finished. The failure sits underneath, in the assumption nobody checked.
This is a review protocol for that situation, written for the engineer who signs the merge, not for the model that produced the patch.
The diffs below are hypothetical, written for illustration. They are not excerpts from client work, and nothing here claims a specific process or measured outcome at ELCODE.
1. The problem: fluent code, silent failure
Code that reads well invites a shallower review. Three failure modes show up repeatedly.
- Happy-path correctness. The implementation works for the input the model imagined, and only for that input. Empty lists, missing fields, and duplicate submissions were never in scope.
- Confident approximation. The function exists, the method name is real, but the semantics are wrong: an off-by-one in pagination, a timezone applied twice, a currency rounded at the wrong step.
- Locally reasonable, globally wrong. The change is fine in isolation, yet it duplicates an existing helper, bypasses a validation layer, or contradicts a convention the rest of the codebase depends on.
Review is not proofreading. The question is not whether the diff reads well, but whether the system is safe to operate after it merges.
2. Understand the change before reading the diff
Ask four questions before opening the file.
- What problem does this solve, and for whom?
- Which requirement or ticket does it map to?
- What is explicitly out of scope?
- What would break if this were reverted tomorrow?
If those answers cannot be stated in three sentences, the change is too large to review properly. Split it. A small diff with a clear intent is reviewable; a large diff with a plausible narrative is not.
Review intent first, implementation second. Most expensive review failures are not missed syntax. They are changes that were never needed, or that solved a different problem than the one stated.
3. Correctness
This is where looking right costs the most. Check the following against the actual requirement, not against the code's own comments.
- Empty and missing input:
null, empty string, empty array, zero, absent optional fields. - Boundary values: first page, last page, exactly the page size, maximum allowed length.
- State transitions: double submits, retries, out-of-order events, stale cache, concurrent updates to the same record.
- Failure paths: timeout, partial failure, upstream returning 200 with an error body.
- Who else is affected: which other callers use this function, endpoint, or table?
A hypothetical example. Assume invoices belong to an account, and the caller is authorized to access only invoices within that account. The endpoint must enforce that relationship on the server. The version below does not.
// Before: plausible, and wrong
export async function getInvoice(req, res) {
const invoice = await db.invoice.findUnique({
where: { id: req.params.id }
});
if (!invoice) return res.status(404).json({ error: "Not found" });
return res.json(invoice);
}
Nothing here is malformed. It is also an object-level authorization hole: any authenticated caller can read any invoice by guessing an id. Authentication answers "who are you". It never answers "may you see this row".
// After: scoped to the caller's account
export async function getInvoice(req, res) {
const invoice = await db.invoice.findFirst({
where: { id: req.params.id, accountId: req.user.accountId }
});
if (!invoice) return res.status(404).json({ error: "Not found" });
return res.json(invoice);
}
The 404 is deliberate. Returning 403 would confirm that a record exists in another account.
4. Security
Security is the category where a confident diff is least trustworthy, because the code can pass every functional test while remaining exploitable.
- Validation at the boundary. Every external input is validated once, at the edge, with an explicit schema: body, query, path, headers, uploaded files.
- Authorization on the server. UI-level hiding is not access control. Every read and write is checked against the acting identity, including nested resources.
- Secrets. No keys in client bundles, logs, error messages, or fixture files. Assume anything shipped to a browser is public.
- Dependencies. AI assistants add packages readily. Review every new entry in the lockfile: maintainer, provenance, install scripts, and whether the standard library already covers it.
- Injection. SQL, shell, template rendering, and log injection all start with string concatenation.
A second hypothetical example.
// Before: string-built query
const rows = await db.query(
SELECT FROM orders WHERE status = '${status}' ORDER BY created_at DESC LIMIT 50
);
// After: parameterized
const rows = await db.query(
"SELECT FROM orders WHERE status = $1 ORDER BY created_at DESC LIMIT 50",
[status]
);
Escaping quotes by hand is not a fix. Parameterization is the fix, because it separates code from data at the protocol level.
5. Testing
Tests are evidence, not decoration. A test that passes before and after the change proves nothing about the change.
- Does the test fail without the patch? Run it against the previous commit.
- Does it assert behavior a user or caller observes, rather than internal call order?
- Was the edge case found during review turned into a test? If not, it will regress.
- Do mocks hide the failure? A mocked repository that never returns empty will never catch the empty case.
- Is the regression scope covered: the endpoint, the caller, and the downstream consumer?
One meaningful integration test can reveal a failure that ten generated unit tests, all restating the implementation, would miss.
6. Maintainability
AI output tends toward plausible generality. That is a maintenance cost, not a feature.
- Readability: names describe domain meaning, not implementation steps.
- Complexity: nested conditionals and long functions are a signal to restructure, not to add comments.
- Duplication: the same helper now exists twice under different names.
- Consistency: the change follows the existing architecture, error format, and naming conventions, or it explains why not.
- Speculative code: abstractions, options, and fallbacks added "just in case" that nothing calls.
Reviewers should ask a blunt question: if this code breaks in six months, will the person debugging it understand why it exists?
7. Production readiness
The last gate is operational, and it is routinely skipped because the feature works locally.
- Logging: structured, with a correlation id that ties request, job, and error together.
- Observability: a metric or alert that would reveal this feature failing before a user reports it.
- Rollback: can the change be reverted without a data restore? Are migrations reversible or additive?
- Configuration: new environment variables documented, defaulted, and validated at boot.
- Failure isolation: timeouts, retries with backoff, and idempotency for anything that can be delivered twice.
- Blast radius: does this touch a shared table, a shared queue, or a shared cache key?
8. A practical checklist
Intent
- I can state the change in three sentences, including what is out of scope.
- The diff maps to a requirement, not to a plausible idea.
Correctness
- Empty, missing, boundary, and duplicate inputs are handled explicitly.
- The failure path returns a defined error, not a silent success.
- Callers of changed functions and endpoints were checked.
Security
- Authorization is enforced server-side, per object, not only per route.
- No secret, token, or internal URL is exposed to the client, logs, or errors.
- New dependencies were reviewed, or avoided.
- Queries and commands use parameterization for values; dynamic identifiers and sort options are restricted to an explicit allowlist.
Tests
- At least one test fails without the patch.
- The edge case found in review is now a test.
- Mocks do not erase the failure mode being tested.
Operations
- Logs and metrics would show failure before a user does.
- Alerts: define what failure looks like, who receives the alert, and what action follows.
- Rollback is possible without restoring data.
- New configuration is documented and validated.
Closing
AI compresses the distance between an idea and a working patch. That is genuinely useful, and it is not the bottleneck most teams struggle with. The bottleneck is judgment: deciding what should exist, what must be true before it ships, and who answers when it fails.
The model can write the patch. It cannot sign for it. That signature is still an engineering act, and it is the part of the work worth protecting.