Two AI reviewers, one Fastify PR, and a 404 that quietly became a 414

#codereview #ai #node #webdev

I ran two AI code reviewers from different vendors over the same merged pull request. One of them found nothing. The other found a behavior change that is still in Fastify today, and it was wrong about one important detail.

This is what happened, how I checked it, and what I changed in my own tool because of it.

The PR

fastify#6716, "feat: add support of onMaxParamLength", merged in May and first released in v5.9.0. It adds a hook for when a route parameter is longer than maxParamLength (default 100 characters).

What the reviewers said

Both reviewers got the same diff and the same instructions, and ran independently.

gpt-5.5 returned 0 findings.
claude-sonnet-5 returned 3. The important one: over-long route params used to get a 404 and now get a 414, even in apps that never opted in. It also noted that apps with their own setNotFoundHandler stop seeing those requests, and that the maxParamLength docs still say "the not found route will be invoked."

It cited the evidence: the new handler is wired in by default, and the PR's own tests change their expected status from 404 to 414 without configuring anything new.

Checking it

A reviewer that sounds confident isn't the same as a reviewer that's right, so I reproduced it:

const fastify = require('fastify')
const app = fastify()
app.get('/test/:id', async (req) => ({ id: req.params.id }))

const longId = 'x'.repeat(150)
const res = await app.inject({
  method: 'GET',
  url: `/test/${longId}`,
})
console.log(res.statusCode)

	Before the PR	v5.9.0 and later
Default app	404, route not found	414, `FST_ERR_MAX_PARAM_LENGTH`
App with `setNotFoundHandler`	404 from the app's handler	414, the app's handler never runs

Same result on 5.12.5 and on main. The docs line is unchanged on both.

Where the reviewer was wrong

It labeled the change a regression. It isn't. The 414 was requested in issue #6697, and returning 414 for an over-long URI is arguably more correct than 404.

What actually slipped through is smaller: the release note says only "feat: add support of onMaxParamLength", the docs still describe the old behavior, and the custom not-found handler side effect isn't mentioned anywhere. If your app relied on that handler for long params, it changed in a minor release without a word.

That's worth a human's minute. It isn't worth blocking a merge.

The rule I changed

Until this week, my tool blocked a PR whenever either reviewer rated something high or critical. On an earlier run of this same PR, that rule showed it as blocked because of one reviewer's findings: a test typo rated critical (real, but in a callback that only runs when the test is already failing) and a "broken" docs anchor that wasn't broken. The other reviewer raised neither.

The new rule: a PR is only blocked when both reviewers independently find the same defect and both rate it at or above the team's threshold. Anything one reviewer raises alone is marked "needs review."

Under that rule, this PR came back as "needs review." That seems right to me. A missed block still puts the finding in front of a person. A wrong block gets in everyone's way.

The cost is real, though: blocking now happens rarely, so a strict gate will miss things one reviewer saw.

What I take from it

Two reviewers disagreeing is information. Here, one saw nothing and the other saw a real compatibility change.
Accurate is not the same as important. Most of what both reviewers said was correct and minor.
Verify before you repeat a finding. The reviewer was right about the facts and wrong about the story. If I had posted "Fastify shipped a regression," I'd have been wrong too.

One PR is an anecdote, not a benchmark. I'd like to hear how your team handles this with human reviewers: does one "request changes" block, or do you need a second opinion?

Disclosure: I build Veridu, the tool that ran these reviews. The stored review and the reproduction are on that page.

DEV Community