Review Agent PRs for Silent Interface Drift

A checkout squad opened an agent pull request on Monday. The title claimed a small null-safety cleanup. The diff looked tiny in GitHub.

The helper loadInvoice stopped throwing NotFoundError. It returned null instead. Three unit tests were rewritten to match.

No caller outside that file was updated. Staging then turned missing invoices into 500s. This scene is a reconstructed example, not a named postmortem.

The review problem

Agents often fix a function by changing its contract. JavaScript callers still parse. Tests stay green because the agent edited them too.

Reviewers who only skim highlighted hunks miss this. The dangerous change sits at the export boundary. Return shapes, thrown errors, and field names all drift.

This article uses a caller-inventory review. Each changed export is classified. Reviewers then trust, revert, or test that change.

What silent interface drift looks like

Start from the helper callers already depend on. They catch a typed error and map it to HTTP 404.

class NotFoundError extends Error {
  constructor(id) {
    super(`invoice ${id} not found`);
    this.name = "NotFoundError";
    this.status = 404;
  }
}

export async function loadInvoice(db, id) {
  const row = await db.invoices.findById(id);
  if (!row) {
    throw new NotFoundError(id);
  }
  return row;
}

An agent PR may rewrite the same file like this. The new body looks cleaner. It is not compatible with existing handlers.

export async function loadInvoice(db, id) {
  const row = await db.invoices.findById(id);
  if (!row) return null;
  return { ...row, amount: row.total };
}

Two contracts moved at once. Throw-to-null changed control flow. total to amount changed the row shape.

Route handlers that expected NotFoundError now receive null. Workers that read invoice.total now read undefined. Unit tests hide both shifts because the agent rewrote them.

Five-step review

Use this sequence on every agent PR. Do not start with style nits. Start with the frozen public surface.

1. Freeze the old export surface

Check out the merge base first. Save every touched export before reading new tests.

git fetch origin
BASE=$(git merge-base origin/main HEAD)
mkdir -p .review-freeze
git show "$BASE":src/invoices/loadInvoice.js \
  > .review-freeze/loadInvoice.base.js
git diff "$BASE" HEAD -- src > .review-freeze/pr.diff
git diff "$BASE" HEAD --stat

The freeze file is the contract. Reviewers compare the PR against that file. They do not compare against the agent's new assertions.

2. Inventory remaining call sites

Changed exports still have callers. JavaScript will not list them. A small script will.

// inventory-callers.mjs
import { readFileSync, readdirSync, statSync } from "node:fs";
import { join, relative } from "node:path";

const ROOT = process.argv[2] ?? "src";
const NAMES = new Set(process.argv.slice(3));

function walk(dir, out = []) {
  for (const name of readdirSync(dir)) {
    const p = join(dir, name);
    if (statSync(p).isDirectory()) {
      if (name === "node_modules" || name === ".git") continue;
      walk(p, out);
    } else if (/\.(js|mjs|cjs)$/.test(p)) {
      out.push(p);
    }
  }
  return out;
}

for (const file of walk(ROOT)) {
  const text = readFileSync(file, "utf8");
  for (const name of NAMES) {
    const re = new RegExp(String.raw`\b${name}\b`, "g");
    let m;
    while ((m = re.exec(text))) {
      const line = text.slice(0, m.index).split("\n").length;
      process.stdout.write(`${relative(".", file)}:${line}:${name}\n`);
    }
  }
}

Run it against the PR tree. Pass every changed export name.

node inventory-callers.mjs src loadInvoice NotFoundError

Every printed line is a review target. Unlisted files are not automatically safe. Dynamic imports can hide extra callers.

3. Classify each change

Build a three-column note for the PR. Use evidence from the freeze file only.

Change	Signal in the diff	Review action
Trust	Same arity, same throws, same keys	Keep, add one pin test
Revert	Throw removed, wrapper added, null introduced	Restore the old contract
Test	Behavior change with an explicit spec	Update callers and tests together

Trust only shape-preserving edits. Extra logging can be trusted. A rename that keeps the thrown type can be trusted.

Revert envelope changes. Revert null-for-throw swaps. Revert extra required arguments. Those edits rewrite callers by stealth.

Test documented behavior changes. A spec issue may request null. Then callers need updates in the same PR. Old error paths still need coverage.

4. Pin the old contract with a failing test

Do not accept the agent's rewritten unit tests as proof. Add a contract test from the freeze file. Run it on the PR branch.

// test/loadInvoice.contract.test.js
import test from "node:test";
import assert from "node:assert/strict";
import {
  loadInvoice,
  NotFoundError,
} from "../src/invoices/loadInvoice.js";

test("missing invoice still throws NotFoundError", async () => {
  const db = { invoices: { findById: async () => null } };
  await assert.rejects(
    () => loadInvoice(db, "inv_404"),
    (err) => {
      assert.equal(err.name, "NotFoundError");
      assert.equal(err.status, 404);
      return true;
    }
  );
});

test("found invoice returns the row object", async () => {
  const row = { id: "inv_1", total: 40 };
  const db = { invoices: { findById: async () => row } };
  const result = await loadInvoice(db, "inv_1");
  assert.equal(result, row);
  assert.equal(result.total, 40);
});

A red contract test blocks the merge. A green one documents the preserved shape. Reviewers then inspect remaining call sites.

5. Post mechanical review comments

Paste the inventory output into the review. Each call site gets one comment. The comment cites the freeze file, not taste.

src/http/invoiceRoute.js:41:loadInvoice
Contract drift: loadInvoice returned a row or threw NotFoundError.
This PR returns null. This handler still reads result.status.
Revert the throw-to-null change, or update this caller and its test.

src/workers/dunning.js:17:loadInvoice
Field drift: callers read invoice.total.
This PR emits amount and drops total.
Add a caller test before renaming the field.

src/reports/sumDaily.js:22:loadInvoice
This file only reads id.
Trust after the contract pin is green.

Keep comments mechanical. Do not debate naming here. Interface drift is a merge blocker.

Run the inventory on a clean tree

Long inventories need a clean checkout. Local node_modules often hides extra files. A throwaway machine keeps the freeze tree isolated.

Disclosure: This article was prepared as part of MonkeyCode's product outreach. MonkeyCode offers free model access and a free server option. Those availability claims come from the operator. This article does not name models, hardware, or quotas.

Clone the PR on that free server and run the same commands. A free hosted model may draft comments from the caller list. Reviewers still match every line to a file.

git clone "$REPO" /tmp/pr-review && cd /tmp/pr-review
git checkout "$PR_REF"
node inventory-callers.mjs src loadInvoice NotFoundError \
  | tee /tmp/callers.txt
node --test test/loadInvoice.contract.test.js

Keep the model prompt narrow. Feed only the freeze diff and caller list. Ask for comments in the template above. Discard extra refactors it suggests.

Teams with spare CI runners can skip that server. The inventory script does not depend on it.

Full walk-through of one review

The reconstructed PR also renamed a field. total became amount. Tests were updated in the same commit.

The inventory then prints HTTP and worker files.

src/http/invoiceRoute.js:41:loadInvoice
src/http/invoiceRoute.js:48:NotFoundError
src/workers/dunning.js:17:loadInvoice
src/reports/sumDaily.js:22:loadInvoice

invoiceRoute.js still catches NotFoundError. That hunk is a revert. dunning.js still reads invoice.total. That hunk needs a test and a field update.

sumDaily.js only uses id. That hunk can be trusted after a pin test. The posted review note then looks like this.

Revert: throw-to-null in loadInvoice
Revert: NotFoundError removed from the export map
Test: total -> amount in dunning.js
Trust: id-only reads in sumDaily.js after contract pin

Merge only after the contract test is green. Merge only after revert hunks land. Do not merge on the agent's test file alone.

A second pass checks error objects that leak across packages. Search for err.status and err.name next. Agents often delete those fields while cleaning helpers.

rg -n "NotFoundError|err\.status|invoice\.total" src test

Any remaining hit is another classify row. Repeat trust, revert, or test. Stop when the inventory and the search agree.

Limitations

The inventory uses word-boundary regex. It misses computed property access. It misses module.exports[name]() call sites.

It also misses other languages in a polyglot repo. GraphQL and RPC bindings stay invisible. Those edges need schema diffs, not grep.

Contract tests pin today's shape. They do not prove semantic equality. A new implementation can pass and still change rounding.

Hosted models draft comments only. They do not approve merges. They may invent callers that do not exist. Reviewers must match each comment to a path.

Who should not use this approach

Do not use this flow on greenfield prototypes. Those branches have no callers yet. A freeze file would only slow the spike.

Do not use it as a replacement for types. TypeScript would catch some envelope changes. The method still helps untyped JavaScript services.

Do not use it on generated protobuf stubs. Those contracts live in the schema. Review the .proto file instead.

Do not use it when the PR is a documented API break. Then the spec is the source of truth. The inventory only checks that every caller moved with it.

What to leave on the PR

Leave the freeze diff in the review thread. Attach the caller list as a comment. Record the three-column classification.

Future agent PRs then have a trail. The next reviewer can see rejected drifts. That trail matters more than a polished summary.

The agent will keep offering cleaner helpers. Cleaner is not the review standard. The standard is the old contract plus an explicit spec.