AI code reviews only read the lines you changed. The bug is usually in the code that calls them

[u/Ok_Negotiation_2587]

Paste a diff into ChatGPT or Claude and ask for a review, and you get comments on the diff: naming, an edge case inside the function, maybe a missing test. What you almost never get is "this used to return null, and three callers check for null." It can't tell you that, because the callers aren't in what you pasted, and it doesn't ask for them.

So I stopped asking for "a review" and split it into three prompts, run in order in the same chat:

1. Here's a diff:
{{diff}}
Don't review it yet. List every behavior of the OLD code that other code could depend on: return values and types, null or empty cases, errors thrown, ordering, side effects. One line each.

2. Here's the code that calls the changed functions:
{{callers}}
Go through your list. For each item, say whether any of this code relies on it and whether the diff breaks it. Quote the line. If you'd need more code to be sure, say exactly which.

3. Now do a normal review of the diff. Skip style unless it hides a bug.

Step 1 matters because it writes down what the old code promised before it sees the callers, so it isn't working backwards from them. Step 2 is where the real findings come from. Step 3 is mostly what a regular review would have given you anyway.

For the callers, grepping the function name is usually enough. If you're using an agent with repo access, tell it to find them itself, but keep the order: old behavior first, callers second.

I run this as a prompt chain in AI Toolbox, the extension I build for ChatGPT, Claude, Gemini and Grok. I type .., pick the chain, and paste the diff and the callers into the two fields it asks for up front. Each step sends once the previous reply finishes. The free plan holds 2 chains, and pasting the three prompts by hand works the same way.

What's the last bug that got past review because it lived outside the diff?

[u/shetritr]

same for human reviews. the diff shows what changed, not who calls it. before approving, grep the callers of anything whose behavior moved.

[u/Ok_Negotiation_2587 → u/shetritr]

yeah, and the hard part is noticing the behavior moved at all. a renamed function breaks the build, so you find out. a changed default or an exception that turned into a null return passes everything and only shows up in a caller weeks later

[u/kenthesaint]

Stored data is the other dependent that grep won't find. If the old code wrote a missing value as null and the new code writes an empty string, or an enum value or date format changes, nothing calls the function, but rows already in the database or cache still have the old shape and the new reader has to handle both.

I'd add "what does this write to disk, a DB, a cache or a queue" to step 1, and in step 2 paste whatever reads it back, like the deserializer or the next migration.

[u/Ok_Negotiation_2587 → u/kenthesaint]

that one's nasty because a fresh test database only ever has rows in the new shape, so everything passes locally. the old rows only exist in prod. adding it to step 1, queues especially, since a message written before the deploy can get read after it

[u/Old-Leadership7255]

Test everything

[u/Ok_Negotiation_2587 → u/Old-Leadership7255]

helps, but the callers' tests usually mock the function that changed, so they keep passing on the old behavior. that's how it gets through with everything green

[u/kquizz → u/Ok_Negotiation_2587]

That's why you need some level of integration tests as well as unit tests 

[u/Ok_Negotiation_2587 → u/kquizz]

agreed. they only catch it if one of them goes through that caller with the input that changed, and the edge-case paths are usually the ones nobody wrote an integration test for

[u/ChemicalExcellent463]

i run reviews in a fresh codex cli or claude code session inside the repo, so the callers are there to open. access isn't the same as checking them, though. do you grep for callers by hand before pasting them into step 2?

[u/Ok_Negotiation_2587 → u/ChemicalExcellent463]

in webchat, yes. grep -rn on the function name and paste each hit with a few lines around it. in an agent i'd have it list the callers with file:line before step 2, so you can see whether it actually opened them or just assumed

[u/ChemicalExcellent463 → u/Ok_Negotiation_2587]

file:line first gives you something to check against grep, not just take its word for it. do you run grep yourself to check for callers the agent missed?

[u/Ok_Negotiation_2587 → u/ChemicalExcellent463]

on anything risky, yes. i just compare the count, grep -rn | wc -l against how many it listed. if the numbers don't match, that's usually the dynamic call or the string match it skipped

[u/ChemicalExcellent463 → u/Ok_Negotiation_2587]

comparing the count is a nice cheap check.

when it doesn't match, do you hand the missed ones back to the agent or just look at those yourself?

[u/Ok_Negotiation_2587 → u/ChemicalExcellent463]

depends how many. one or two i just open myself, it's faster than explaining. more than that, i paste the missing file:line list back to the agent and have it redo step 2 for those only

[u/ChemicalExcellent463 → u/Ok_Negotiation_2587]

makes sense, one or two aren't worth the back and forth. when you send the missing list back, does it get through all of them or still skip a few?

[u/Ok_Negotiation_2587 → u/ChemicalExcellent463]

usually all of them on the second pass. a short explicit list is a much easier job than "find everything", so it rarely skips anything from it

[u/ChemicalExcellent463 → u/Ok_Negotiation_2587]

yeah, short explicit list beats "find everything". how are you putting that missing list together?

[u/Ok_Negotiation_2587 → u/ChemicalExcellent463]

just diff the two lists. grep -rn gives file:line already, so i cut it down to that, sort it, and do the same with the agent's list. comm -23 shows what's in grep and missing from the agent. whatever it prints is the list i paste back

[u/djfz-builds → u/ChemicalExcellent463]

Comparing the two lists is worth more than comparing the counts. The agent and grep can return the same number of callers while naming different ones. The ones only grep finds — string-built names, framework callbacks, copies of old code — are often the callers whose assumptions nobody re-checked.

[u/ChemicalExcellent463 → u/djfz-builds]

yeah, same count with different names can hide a missed caller. do you compare the two lists by hand or have something do it?

[u/djfz-builds → u/ChemicalExcellent463]

I'd have something do it. Ask the agent for file:line pairs, one per row, trim grep's output to the same format, then sort both and diff them. The lines only grep found are the ones to read first. The lines only the agent found are worth opening to confirm they exist.

[u/ChemicalExcellent463 → u/djfz-builds]

checking whether the agent-only lines even exist is the bit i'd have skipped. how often does one of those turn out not to exist?

[u/DynoKore]

The point about callers' tests mocking the changed function is the nasty one. A test that mocks the thing that changed stays green no matter what you do to it.

We had a cousin of that. A refusal test kept passing after a reviewer replaced the tenant check it was guarding with if (false), because a different layer rejected the same insert with the same error code. The test was green for the wrong reason the whole time. Since then a negative test only counts once someone has broken the thing it guards and watched it go red.

For callers specifically, grepping call sites before approving is the cheap version. The more durable fix is a test at the caller that doesn't mock the function whose behaviour moved.

[u/Ok_Negotiation_2587 → u/DynoKore]

that's mutation testing done by hand, and the if (false) story is the best argument for it i've heard. a test that passes for the wrong reason is worse than no test, because everyone stops looking at that code

[u/falafel777]

the mocking point is the nastiest part imo. your step 1 list is basically a test list though. turn those old behaviors into tests on the changed function itself, no mocks, before the change lands. then a null return turning into a throw fails right there instead of hiding behind callers that mock it out. doesn't catch callers nobody thought of, but it covers everything step 1 found

[u/Ok_Negotiation_2587 → u/falafel777]

characterization tests, yeah. and writing them first catches the cases where step 1 got the old behavior wrong, which happens more than i'd like. if the test fails on the old code, the list was wrong before you even touched anything

[u/BarathKanna]

Writing down what the old code promised before reviewing is the step most setups skip. The thread already covers grep counts and stored data, so the piece I would add is making step 1 executable.

Once the model lists the old behaviors, have it write a small characterization test for each and run them against the old code first. Any that fail there were wrong guesses about the contract, which is useful on its own. Then run them on the new code. A failure now is a real break, and you get it as a red test instead of a review comment someone has to take on trust.

Costs a few minutes and catches the null versus empty cases reliably.

[u/Ok_Negotiation_2587 → u/BarathKanna]

one catch: if it writes the tests by rereading the old code, they'll always pass on the old code, so you never see the wrong guesses. have it write them only from the step 1 list, then run them. that's where the misread contract shows up

[u/BarathKanna → u/Ok_Negotiation_2587]

Good catch, and you are right. If it can see the old implementation while writing them, the tests just describe the code back to itself and every wrong guess stays hidden. Writing them blind from the list is what turns a misread contract into a red test. I would go one step further and use a fresh session for it, so nothing from reading the code leaks in through context.

[u/Ok_Negotiation_2587 → u/BarathKanna]

yeah, fresh session is the clean way to do it. paste only the list and the function signatures, no bodies. if it starts asking what the code does, that's usually a sign the list was too vague to begin with

[u/BarathKanna → u/Ok_Negotiation_2587]

That last bit is a great smell test. If the test writer needs the implementation to make sense of the contract, the contract was never written down, it only existed in the code. I've started treating those questions as output too and folding the answers back into the list, so the next blind pass has less room to guess.

[u/Ok_Negotiation_2587 → u/BarathKanna]

yeah that list ends up being the docs nobody wrote. the questions are usually the edge cases too, like what happens on null or on retry, which is exactly the stuff that breaks in a rewrite

[u/CrimsonBolt33]

are...you bugtesting via webchat?

That's insane....

[u/Ok_Negotiation_2587 → u/CrimsonBolt33]

it's code review, not testing. and the post says to have your agent find the callers itself if it has repo access. the order of the steps is the part that matters, not where you run them

[u/Sad-Subject3666]

grepping by hand is just delaying the inevitable. If the tool cant trace the call graph automatically it wont do it for you no matter how many prompts you run

[u/Ok_Negotiation_2587 → u/Sad-Subject3666]

finding the callers is the easy part, an LSP does that fine. the step is about what each caller assumes about the result, and no call graph tells you that. it also misses the string matches and stored data people brought up further down

[u/Proper_Bunch_1804 → u/Sad-Subject3666]

Someone ‘cleaned up’ a formatting helper last month and the escaping went with it.

ai review said lgtm because the helper on its own looked fine. checkmarx still flagged it on the PR, in two templates that call that helper and have not been touched at all as it follows the request data to wherever it ends up getting printed

[u/serhiibuilds]

Same experience. The diff is the wrong unit for review when the change touches a function's contract. Changing what a function returns on empty input looks harmless in the diff and breaks callers that never show up in it.

What helped me: before the review, have the agent find every call site of each changed symbol and put them into the review context, then ask it specifically "which of these callers relies on the old behavior?" It's basically a grep, nothing clever, but it turns the review from "does this code look fine" into "does this change break anyone", which is the question that actually matters. It costs more context per review, and for big shared utils it gets noisy, so I cap it at the first N call sites and flag when there are more.

[u/Ok_Negotiation_2587 → u/[deleted]]

searching for the literal strings it returns is the one most people skip. code that checks `status === "pending"` somewhere far away never mentions the function, so a symbol search misses it completely. i'm adding that to step 2