Spotting the subtle flaws in AI-generated code
Recently, I’ve been seeing more people talk about how LLMs create code that looks plausible, but has subtle underlying issues that require an experienced engineer to spot. But what does this actually look like in the real world? I don’t think the answer here is cut and dried, because we’ve moved beyond identifying code that’s “syntactically correct” into something closer to “structurally correct”. This means that in order to spot these issues, you need to have a conceptual view of the codebase, rather than the ability to simply interpret the code syntax. Obviously each codebase is different, so I can’t really list every example of this here, but over the past couple of years I’ve identified some common patterns.
1. Massive pull requests. This isn’t exactly a code pattern, but it has become way more common since the rise of coding agents, so it’s worth mentioning. If you’re doing the right thing, you have a human that reviews each pull request before it gets merged. But there’s a limit to how much code a human can effectively review and validate in one sitting. If you give this person a 10,000 line pull request spanning 300 files to review, something is bound to slip through the cracks. Pull requests like this are usually a symptom of a couple problems. The first one is that people are working in chunks that are way too large, which is usually a result of poor planning or management. This has been an issue for years, but has obviously been exacerbated by engineers deciding to let their coding agents run free, whether it’s due to carelessness or management pressure. The second one is that your AI is majorly over-engineering solutions and getting carried away. In this case, you need to remember that you are the engineer here, and push for more concise and lean solutions.
2. Null checks. Coding agents love using null checks everywhere, especially in JavaScript and TypeScript. (If you want a fun rabbit hole to go down, research Tony Hoare’s billion dollar mistake). Not all null checks are bad, but you need to understand the upstream causes of that null value, and the downstream effects of how they’re handled. One example of this I frequently see is defaulting to today’s date if a function’s date param is null. The first thing to look at is your upstream cause. Why does this parameter have the potential to be null (or does it have the potential at all)? Does it make sense for it to be nullable? Generally, strongly typed parameters or a good API schema like Zod can eliminate the need to even check for a null value. The second thing to look at is the downstream effects. Let’s say our date parameter is used in a query that returns a list of scheduled customers for that day. It’s probably not the end of the world to default to today’s date for that, but you should consider doing that upstream (in whatever calls the function), for the sake of keeping things clean. However, for a query that’s timezone sensitive (a time tracking app comes to mind here), you could run into issues if your function decides to default to a date/time value that’s in a different time zone than the client calling it.
function getScheduledCustomers(date: Date | null) {
const day = date ?? new Date(); // "today" — but in whose timezone?
return scheduleRepo.forDay(day);
}The ?? new Date() looks harmless, but if the function’s caller doesn’t pass in a date, it simply resolves “today” in the server’s timezone.
3. Strange error handling, mainly catch and rethrow errors. Generally, if an error is thrown somewhere in the function chain of your API call, it will bubble back up the chain and be returned in the API response. I frequently see coding agents wrap functions inside of a try-catch block, handle only one specific error thrown by that function, then throw a generic error for other exceptions. One of the only times that it makes sense to do this is if your application relies on third party libraries or external APIs, in which you will need to build what’s called an anti-corruption layer (a topic for another time).
async function getInvoice(id: string): Promise<Invoice> {
try {
return await invoiceRepo.findById(id);
} catch (err) {
if (err instanceof NotFoundError) {
throw new Error(`Invoice ${id} not found`);
}
// a dropped connection, a timeout, or a bug in the repo
// all get flattened into one vague error
throw new Error("Failed to fetch invoice");
}
}Not only can this mask other errors that get thrown in the call stack, it’s simply unnecessary. However, this pattern really only applies if the error handling of the entire call stack is within your control.
4. Feature envy. Feature envy is what we call a “code smell”. It’s when a method inside a class seems more interested in the data or methods of another class than in its own. In some cases it’s fairly easy to spot, but in other cases it requires a deeper understanding of your application’s domain structure to determine whether or not it’s an issue. Quick example: let’s say you have an employee model, and a company model. The logic behind giving that employee a different role should live within the employee model, but if you’re not careful, the coding agent might try to slip it into the company’s model. It may not seem like a big deal, but if you let this go on long enough, your business logic will become a mess.
// leak: Company reaches into Employee to mutate its role
class Company {
reassign(employee: Employee, role: Role) {
// note: no check for employee.canHold(role)
employee.role = role;
}
}
// better: the rule lives with the model it belongs to
class Employee {
assignRole(role: Role) {
if (!this.canHold(role)) throw new InvalidRoleError(role);
this.role = role;
}
}In the first version, any validation around what roles an employee can hold now has to be remembered everywhere reassign is called. In the second, it’s impossible to set an invalid role because the model won’t let you.
5. Unnecessary or misplaced error defense. Coding agents have a tendency to do a lot of unusual and unnecessary error defense. Sometimes you’ll catch one doing some obscure check in an API call, and it’s funny because the comment it makes will describe it as something like “defense in depth”. The example below is a very slimmed down version of what I’m talking about, similar to cases I’ve seen in the past.
async function updateCustomer(id: string, patch: CustomerPatch) {
const customer = await customerRepo.findActiveById(id);
// misc customer mutation functions here
const customerBeingEdited = await customerRepo.findActiveById(id);
// "defense in depth": make sure the customer is still active before updating,
// in case they were suddenly deactivated
if (!customerBeingEdited.isActive()) {
throw new InactiveCustomerError(id);
}
return customerRepo.update(id, patch);
}You need to look at the surrounding business logic or UX flows and determine if it’s even possible for these types of errors to happen. If it has a 1 in 30,000 chance of ever happening, and your database supports transaction rollbacks, you’re just setting money and/or tokens on fire with these checks. Here, the extra read to ensure the customer is active only exists to cover a window that’s less than a second long. If this was a legit concern, the correct solution would be to let the repo throw an error on update, which would roll back the whole transaction and affect 0 rows.
6. Misleading function names. There’s a concept in software development called “side effects,” the idea that a function does something “on the side” in addition to just computing something from its arguments. Side effects can be hard to reason about, because we (as humans) make sense of a function based on its name, its arguments, and its return type. These functions should have intention revealing names, so that their purpose and their effects are predictable to other developers. Take a look at this example:
async function getUserById(id: string): Promise<User> {
const existing = await userRepo.findById(id);
if (existing) return existing;
// surprise: a "get" that writes
return userRepo.create({ id });
}getUserById typically implies that we’re going to get a user by their ID, or throw an error if they don’t exist. But the name here is misleading, as we’re creating a user with the passed in ID if they don’t exist: a hidden side effect that the name didn’t give us a clue about. I’ve caught this a few different times in code reviews. It’s more common than you might think, these types of functions can make debugging difficult.
7. Excessive code comments. This might be a controversial one. Code comments are not for narrating the obvious details of your code, but coding agents love doing this. This needlessly sets tokens on fire, and really obscures the readability of your code, so you need to keep this under control. Some people have very strong opinions on the use of comments, and consider them to be “code deodorant” that exists to mask code smells. I most often consider them to be acceptable if you’re documenting some sort of obscure bug that your code fixes, like adding a z-index to a button that would otherwise be covered by a modal. Otherwise, if you legitimately need comments everywhere for someone to understand what your code does, it’s a sign that your code needs to be refactored.
Coding agents may be a powerful tool, but if they’re not kept in line by an experienced software engineer, any productivity gains you make by using them will be squashed by the technical debt you incur. This list is far from exhaustive, but I hope that it will open your eyes and enable you to spot similar flaws in your own codebase.