DEV Community: Jordan Li

Review an Agent PR That Writes Tool JSON Into Runtime Config

Jordan Li — Fri, 09 Oct 2026 13:25:33 +0000

A composite review scenario starts with a failed Friday staging deploy. In that scenario, the planner route wrote tool JSON over runtime config. In the diff, retry ceilings climbed and TLS verification vanished.

The useful change was a fixture loader meant for local tests. The harmful change was an unbounded merge sitting beside that loader. This review splits those changes before any approval reaches the branch.

What landed in the diff

The agent added three files and edited the shared config module. One route parsed a request body and called a recursive merge helper. A sample payload sat in fixtures while the helper copied every key.

Reviewers can trust the loader after path checks pass in tests. They should revert any merge that writes into live config. They should test the allowlist that replaces that unsafe merge.

1. Read the helper that writes config

Start the review at the function that writes runtime config. Short helpers often hide wide effects across the whole process. This helper is the center of the review, not a side note.

function deepMerge(target, patch) {
  for (const key of Object.keys(patch)) {
    const next = patch[key];
    if (next && typeof next === "object" && !Array.isArray(next)) {
      target[key] = deepMerge(target[key] ?? {}, next);
    } else {
      target[key] = next;
    }
  }
  return target;
}

Every key in the patch can land inside the target object. Nested objects receive that same copy behavior at each level. Arrays and scalar values replace whatever value stood there before.

2. Mark the route trust boundary

The route treats model tool output as a trusted config patch. That output is not a source file a human already reviewed. A planner can emit keys the original author never typed.

function applyPlannerPatch(runtimeConfig, body) {
  const patch = body.patch ?? {};
  deepMerge(runtimeConfig, patch);
  return { ok: true, config: runtimeConfig };
}

Revert this route or gate it behind a local test flag only. Never bind the route on a shared or internet-facing server. The returned object also exposes the full config to callers.

3. Separate the loader you can trust

The fixture loader is the part worth keeping after a path review. It should accept only simple names and resolve under a fixed root. It should reject traversal segments before any file read begins.

import { readFile } from "node:fs/promises";
import path from "node:path";

export async function loadFixture(name) {
  if (!/^[a-z0-9-]+$/.test(name)) {
    throw new Error("fixture name rejected");
  }
  const root = path.resolve("fixtures");
  const file = path.resolve(root, `${name}.json`);
  if (!file.startsWith(root + path.sep)) {
    throw new Error("fixture path rejected");
  }
  return JSON.parse(await readFile(file, "utf8"));
}

Trust this loader only when tests cover bad names and odd paths. Do not trust it if it accepts absolute paths from the request. Do not let the loader feed its JSON into the live merge.

The path check above is a proposal, not a finished library. Reviewers should confirm it on each target platform before trust.

4. Build the deny table before style notes

Build a deny table before any comment about naming or style. That table is the main review artifact for this pull request. It tells the author which keys to delete from the patch path.

Key path	Risk if a patch lands	Review action
retries.max	Extra model calls can spend the token budget	Revert and set the cap in code
tls.rejectUnauthorized	Staging calls can skip certificate checks	Revert before any shared run
auth.mode	A string can weaken session checks	Revert and cover with tests
log.level	Debug logs may store raw prompts	Keep the code default only
db.url	A patch can redirect later writes	Deny the key and alert

The earlier float-billing review covers a different failure mode. This table covers control flags rather than currency math. Keep those two checklists in separate comments on the request.

5. Replace the merge with an allowlist

Allow only keys that a human already named in source control. Ignore every unknown key and record it for the review log. Return that ignored list beside the updated config snapshot.

const ALLOWED = new Set(["jobs.timeoutMs", "jobs.batchSize"]);

export function applyAllowed(config, patch, allowed = ALLOWED) {
  const ignored = [];
  for (const [key, value] of Object.entries(flatten(patch))) {
    if (!allowed.has(key)) {
      ignored.push(key);
      continue;
    }
    if (typeof value !== "number" || value < 1 || value > 30000) {
      ignored.push(key);
      continue;
    }
    setPath(config, key, value);
  }
  return { config, ignored };
}

Numeric values must stay inside a fixed inclusive range. String values never pass through this replacement helper. Nested control flags stay outside the allowed key set.

The flatten and setPath helpers belong in that same module. Reviewers should read both helpers before they approve the change. A careless flatten can still smuggle unexpected keys past a hasty check.

export function flatten(obj, prefix = "", out = {}) {
  for (const [key, value] of Object.entries(obj ?? {})) {
    const next = prefix ? `${prefix}.${key}` : key;
    if (value && typeof value === "object" && !Array.isArray(value)) {
      flatten(value, next, out);
    } else {
      out[next] = value;
    }
  }
  return out;
}

export function setPath(config, dotted, value) {
  const parts = dotted.split(".");
  let node = config;
  for (const part of parts.slice(0, -1)) {
    node[part] = node[part] ?? {};
    node = node[part];
  }
  node[parts.at(-1)] = value;
  return config;
}

6. Add tests the agent did not show

The tests below are proposed examples and were not executed here. They import the node test runner and the strict assert module. Run the file on a disposable runner before anyone merges the branch.

import test from "node:test";
import assert from "node:assert/strict";
import { applyAllowed } from "./apply-config.js";

test("rejects tls and retry overrides", () => {
  const config = {
    tls: { rejectUnauthorized: true },
    retries: { max: 2 },
    jobs: { timeoutMs: 1000 }
  };
  const patch = {
    tls: { rejectUnauthorized: false },
    retries: { max: 99 },
    jobs: { timeoutMs: 2500 }
  };
  const allowed = new Set(["jobs.timeoutMs"]);
  const result = applyAllowed(config, patch, allowed);
  assert.equal(config.tls.rejectUnauthorized, true);
  assert.equal(config.retries.max, 2);
  assert.equal(config.jobs.timeoutMs, 2500);
  assert.deepEqual(result.ignored.sort(), [
    "retries.max",
    "tls.rejectUnauthorized"
  ]);
});

test("drops unknown batch size outside the allowlist", () => {
  const config = { jobs: { timeoutMs: 1000 } };
  const patch = { jobs: { timeoutMs: 2500, batchSize: 4 } };
  const allowed = new Set(["jobs.timeoutMs"]);
  applyAllowed(config, patch, allowed);
  assert.equal(config.jobs.timeoutMs, 2500);
  assert.equal(config.jobs.batchSize, undefined);
});

A green run means the allowlist held for these two cases. It does not prove that every future key will stay safe. Add a new case whenever a fresh flag enters the config module.

Use these commands on the review branch, not on production hosts. The first command runs the file once with the default reporter. The second command prints a spec report for the pull request thread.

node --test test/apply-config.test.js
node --test --test-reporter=spec test/apply-config.test.js

7. Draft on a free runner, then decide locally

Disclosure: This article was prepared as part of MonkeyCode's product outreach. The outreach brief states free model access plus a free server option. It also states a free allowance of 10 million tokens.

Confirm the live terms before a review depends on that allowance. This draft assumes no model names, hardware specs, or fixed duration. Quotas and server access can change, so the live page wins.

A reviewer can clone the branch onto that free server option. The free model may draft a first comment from the deny table. A human still decides what to revert and what to keep.

A model draft remains a suggestion rather than a merge approval. Paste test output under the comment, not runtime config or tokens. Keep secrets and customer rows out of fixtures and prompts.

Fetch the pull request head into a local review branch first. Switch to that branch, then run the proposed test file. The branch name and pull number below are examples, not a live request.

git fetch origin pull/184/head:review-184
git switch review-184
node --test test/apply-config.test.js

8. Leave a four-line review comment

Write the review comment as four short ordered lines. Authors often skim long essays and miss the required revert. These lines should match the trust split from this review.

Trust the fixture loader after its path tests pass cleanly.
Revert the deep merge into runtime config and the public route.
Replace that merge with the allowlist and the deny table.
Require both proposed test cases before the next push.

Add a note if the agent retries model calls inside the route. That budget bug belongs in a separate ticket, not this one. Do not bury that budget issue under the merge discussion.

9. Watch three failure shapes

Watch for three repeated failure shapes in agent config diffs. A boolean buried under tls can disable certificate checks. A numeric retry field can spend a token allowance quickly.

A string auth mode can weaken session checks without a type error. Reproduction for this review stays local and non-destructive. Copy the bad patch into a fixture, then call the allowlist helper.

Assert that denied keys leave the original config values untouched. Store that fixture beside the tests so the next reviewer can rerun it.

{
  "tls": { "rejectUnauthorized": false },
  "retries": { "max": 99 },
  "jobs": { "timeoutMs": 2500 }
}

Limits and who should skip this

This allowlist fits flat numeric job settings and nothing broader. It is the wrong tool for secrets, URLs, or role names. Teams that already sign config bundles should verify those signatures instead.

A single-file prototype does not need this planner route at all. Do not use this pattern when patches come from tenant input. Do not use it to hot-edit flags on a production process.

Do not treat a model draft as the final human reviewer. Skip the free runner when the repository holds customer data. Skip it when legal review forbids sending code to a hosted model.

One optional next step

Readers who want a spare runner can check current MonkeyCode terms. Confirm the free server option and the stated token allowance first. Then run the proposed tests and keep the verdict in human hands.

Closing check

Approve the request only after the deep merge helper is gone. Keep the fixture loader when tests pin its file paths. Record ignored keys in the pull request thread for the next reviewer.

That record makes a repeated mistake easier to spot later. Ship the allowlist only after the proposed tests pass on the review branch.

Reviewing an Agent PR That Retries Model Calls Without a Budget

Jordan Li — Thu, 08 Oct 2026 09:26:03 +0000

Reviewing an Agent PR That Retries Model Calls Without a Budget

A review request lands on a Tuesday morning. An agent has opened a PR titled "add retry to model client."

The diff is 41 lines. It touches one file. CI is green.

Most reviewers approve it in ninety seconds. Two spend bugs are sitting inside those lines.

The diff looks reasonable at first glance

The agent wrapped the model call in a retry loop. It added exponential backoff. It caught broad exceptions.

None of that is obviously wrong on its own. The problem is what the loop never tracks: a total budget.

This scenario is illustrative, not a report of a specific incident. The code below is a composite of retry loops that agents commonly generate.

Step 1: Classify the diff before you read it

Agent PRs mix three kinds of change. Each kind deserves a different review depth.

Mechanical edits: renames, formatting, import sorting. Skim these.
Local logic: one function, one file. Read fully, then test.
Resource control: retries, caches, locks, limits. Read adversarially.

Retry logic always lands in the third bucket. A retry loop is a spend multiplier and a load multiplier.

Step 2: Find the missing budget

Here is the shape agents usually produce.

async def call_model(client, prompt):
    for attempt in range(5):
        try:
            return await client.complete(prompt)
        except Exception:
            await asyncio.sleep(0.5 * 2 ** attempt)
    raise RuntimeError("model call failed")

Five attempts means five billable requests per call. The loop has no token ceiling. It has no wall-clock ceiling either.

Multiply that by a fan-out batch of 200 items. One bad afternoon becomes 1,000 requests.

The table below is arithmetic, not a benchmark. It assumes 1,200 tokens per attempt.

Attempts per call	Items	Requests	Tokens
1	200	200	240,000
3	200	600	720,000
5	200	1,000	1,200,000

A free allowance disappears faster than most reviewers expect. Any operator-reported free tier, including MonkeyCode's stated allowance of 10 million free tokens, should be treated as a finite resource in review.

Step 3: Check what the loop sleeps on

await asyncio.sleep yields control back to the event loop. time.sleep blocks the entire loop.

Agents mix the two constantly, especially in "fixed" revisions. Search the diff for time.sleep in any async path and revert on sight.

Also check for jitter. Retries without jitter synchronize across workers and rebuild the spike you were avoiding. Look for random.uniform near the delay calculation.

Step 4: Reject retries on non-retryable errors

except Exception retries a 400 validation error. It also retries auth failures and schema mismatches. That is pure waste with a clean bill attached.

Require a predicate instead.

RETRYABLE_STATUS = {408, 425, 429, 500, 502, 503, 504}

def is_retryable(exc: Exception) -> bool:
    status = getattr(exc, "status_code", None)
    if status is not None:
        return status in RETRYABLE_STATUS
    return isinstance(exc, (TimeoutError, ConnectionError))

This predicate is the highest-value three lines in the review. It removes retries that cannot succeed.

Step 5: Replace the loop with a bounded helper

A budget object makes the bound explicit and testable.

The helper below requires Python 3.11 or newer, because it uses asyncio.timeout.

import asyncio
import random
from dataclasses import dataclass

@dataclass
class Budget:
    max_tokens: int
    max_attempts: int = 3
    base_delay: float = 0.5
    spent_tokens: int = 0

    def remaining(self) -> int:
        return self.max_tokens - self.spent_tokens

    def charge(self, usage: int) -> None:
        self.spent_tokens += max(usage, 0)

    def can_retry(self, attempt: int) -> bool:
        return attempt < self.max_attempts and self.remaining() > 0

    def backoff(self, attempt: int) -> float:
        delay = self.base_delay * 2 ** (attempt - 1)
        return delay + random.uniform(0, delay * 0.1)


class ModelCallFailed(RuntimeError):
    pass


async def call_with_budget(client, prompt, budget: Budget, deadline_s: float = 20.0):
    async with asyncio.timeout(deadline_s):
        for attempt in range(1, budget.max_attempts + 1):
            if budget.remaining() <= 0:
                raise ModelCallFailed("token budget exhausted")
            try:
                resp = await client.complete(prompt, max_tokens=budget.remaining())
            except Exception as exc:
                if not is_retryable(exc):
                    raise
                budget.charge(getattr(exc, "usage", 0))
                if not budget.can_retry(attempt):
                    raise
                await asyncio.sleep(budget.backoff(attempt))
                continue
            budget.charge(resp.usage)
            return resp
        raise ModelCallFailed("no attempt succeeded")

Note the difference from the agent version. Failed attempts are charged too, when the provider reports usage on the error. Silent undercounting is a common agent bug.

Step 6: Prove the bound with a fake client

The tests below need no plugins. asyncio.run keeps the harness dependency-free.

import asyncio
import pytest
from app.budget import Budget, ModelCallFailed, call_with_budget


class ModelError(Exception):
    def __init__(self, status_code, usage=1200):
        super().__init__(f"status {status_code}")
        self.status_code = status_code
        self.usage = usage


class Response:
    def __init__(self, usage):
        self.usage = usage


class FakeClient:
    def __init__(self, statuses, usage=1200):
        self.statuses = list(statuses)
        self.usage = usage
        self.requests = 0

    async def complete(self, prompt, max_tokens):
        self.requests += 1
        status = self.statuses.pop(0) if self.statuses else 200
        if status != 200:
            raise ModelError(status, usage=self.usage)
        return Response(usage=self.usage)


def test_retry_stops_when_token_budget_runs_out():
    client = FakeClient([503, 503, 503, 503])
    budget = Budget(max_tokens=3000, max_attempts=5, base_delay=0.0)
    with pytest.raises(ModelError):
        asyncio.run(call_with_budget(client, "hi", budget))
    assert client.requests == 3
    assert budget.spent_tokens >= budget.max_tokens


def test_non_retryable_error_is_not_retried():
    client = FakeClient([400, 200])
    budget = Budget(max_tokens=10_000, max_attempts=5, base_delay=0.0)
    with pytest.raises(ModelError):
        asyncio.run(call_with_budget(client, "hi", budget))
    assert client.requests == 1


def test_deadline_caps_wall_clock_time():
    class SlowClient:
        async def complete(self, prompt, max_tokens):
            await asyncio.sleep(5)
            raise AssertionError("should never resolve")

    budget = Budget(max_tokens=10_000, max_attempts=5, base_delay=0.0)
    with pytest.raises(TimeoutError):
        asyncio.run(call_with_budget(SlowClient(), "hi", budget, deadline_s=0.05))

Run it with one command.

python -m pytest -q tests/test_budget.py

Three assertions carry the whole review. Request count is bounded. Non-retryable errors stop immediately. Wall-clock time is capped.

What to trust, what to revert, what to test

Signal in the diff	Verdict	Required evidence
`except Exception` around the model call	Revert	Replace with `is_retryable` predicate
`time.sleep` inside an async path	Revert	None; it blocks the event loop
Literal retry count, no budget object	Change requested	Test asserting a maximum request count
Failed attempts not charged	Change requested	Test with a usage-bearing error payload
`asyncio.timeout` present	Trust on 3.11+	Wall-clock assertion
Jittered backoff	Trust	Assertion that total delay stays under the deadline
Attempt number in logs	Trust	Check the log line exists on retry

Review the diff against this table before reading the prose description. Agent PR bodies often describe the intent, not the behavior.

Where this harness can run

Running the fake-client tests is free. Running them against hosted models is not.

The operator states that the project is open source, with free model access, a free server option, and an allowance of 10 million free tokens. Disclosure: This article was prepared as part of MonkeyCode's product outreach.

For this review workflow, the hosted workspace matters more than the token allowance. A reviewer can re-run the harness on a branch without provisioning a local GPU. Treat license terms and quota details as operator-stated, and verify the current terms yourself before depending on them.

Limitations and who should not use this

Budget accounting depends on provider-reported usage per response. If your provider omits usage on errors, count requests instead.
Streaming responses often report usage only at the end. Without a final usage event, the token bound becomes approximate.
asyncio.timeout needs Python 3.11 or newer. Adapt with asyncio.wait_for on older runtimes.
The budget is client-side. It cannot stop billing for a request already in flight.
Free tiers and quotas change. Do not encode a promotional allowance into production defaults.

Skip this approach if your provider already enforces hard spend caps, if your codebase is threaded and synchronous, or if you need billing-layer guarantees rather than client-layer ones.

Takeaway

A retry loop is a resource-control change, not a convenience change. Review it as a budget problem. Then assert the budget in a test before you approve.

Review Agent PRs That Bill Orders With Floats

Jordan Li — Thu, 08 Oct 2026 06:54:57 +0000

A Friday checkout pull request looked finished at first glance. The agent had wrapped the charge call in retries. The diff was small and the tests were green.

The reviewer opened the money helper before approving. Prices arrived as JavaScript binary floating point numbers. A later retry could post the same order twice.

That pair of choices is a common agent miss. The patch optimizes only the visible happy path. It skips ledger rules that production billing needs.

What the pull request changed

The branch added a charge helper to the Node service. It read the amount field from the request body. It converted that field with Number and called the provider.

async function chargeOrder(req) {
  const amount = Number(req.body.amount);
  const result = await provider.charge({
    orderId: req.body.orderId,
    amount,
  });
  return { ok: true, result };
}

A second commit retried any thrown error three times. The retry path had no idempotency key at all. There was no check that amount stayed integer cents.

async function chargeWithRetry(payload) {
  for (let attempt = 1; attempt <= 3; attempt += 1) {
    try {
      return await provider.charge(payload);
    } catch (error) {
      if (attempt === 3) throw error;
    }
  }
}

Green unit tests only mocked a successful charge. Those unit tests never replayed a provider timeout. They never asserted the stored integer minor units.

What to trust in the diff

Trust the narrow intent when the route contract is clear. A checkout handler should accept an order id. It should charge once and return a stable status.

Trust new tests that fail before the fix. Trust types that keep money in integer minor units. Trust a provider client that forwards an idempotency key.

Trust comments only when a test locks the same rule. An agent often explains a guard it did not add.

What to revert before merge

Revert float conversion of prices, tax, and totals. Binary floating point cannot represent every decimal cent. Adding one tenth and two tenths is not exact.

Revert blind retries that wrap payment provider posts. A timeout does not prove the provider skipped the charge. The next attempt can bill the customer again.

Revert catch blocks that ignore the error object. The sample swallows the first two provider failures. Operators then lose the provider code and request id.

Revert request-body amounts as the billing source of truth. The client can send a different total than the cart. The server should recompute minor units from stored line items.

Numbered review steps

Open the diff and list every money field. Mark each field as minor units, decimal string, or binary float. Treat every binary float field as a revert candidate.
Trace one order id from the route to the provider call. Confirm the same key is stored before the network call. Confirm a replay returns the first stored result.
Read the retry loop and name each caught error. Timeouts, conflicts, and server errors need different paths. A single loop for all errors is not enough.
Check the test files for risky boundary amounts. Include a zero total, a one-cent line, and a distorted float total. Include a timeout followed by a duplicate post.
Run the suite on a clean machine, not only in the agent log. A free server is enough for this small Node service. Record the command and the exit code in the review.
Write review comments that name the failing case. Ask for a patch, not a prose promise. Merge only after new tests fail on old code and then pass.

Commands that make the review reproducible

Check out the branch and limit the diff to billing files. The commands below are an example workflow, not a live pull request. Replace the branch name with the branch under review.

git diff --unified=3 main...HEAD -- src/charge.js test/charge-once.test.js
node --test test/charge-once.test.js

Save the exit code next to the review note. A green run on old tests does not approve the design. The new cents test must exist and must fail on the old helper.

A decision table for the reviewer

Diff signal	Trust for now	Revert or block	Test to add
Integer cents from stored lines	Yes, if a test locks the formula	No	One-cent and large quantity totals
Number(req.body.amount) sent onward	No	Yes	Reject non-integer input
Retry without an idempotency key	No	Yes	Timeout, then one duplicate post
Key stored before the provider call	Yes, if unique per order charge	No	Replay returns the prior status
Empty catch around provider errors	No	Yes	Assert the logged provider code
Mock that only returns success	No	Expand the tests	Failure and replay cases

Use this table as the reusable review artifact. Copy each row into the pull request comment. Check every row against the lines in the diff.

A minimal regression to demand

The replacement helper below is a review target, not a production billing engine. The helper stores order money as integer cents. It refuses a second charge when the order already has a provider reference.

function isDigits(text) {
  return text.length > 0 && [...text].every((ch) => ch >= '0' && ch <= '9');
}

function toCents(decimalText) {
  const parts = String(decimalText).split('.');
  const major = parts[0];
  const minor = parts[1];
  const valid = parts.length === 2 && isDigits(major) && minor
    && minor.length === 2 && isDigits(minor);
  if (!valid) throw new Error('amount must be dotted cents');
  return Number(major) * 100 + Number(minor);
}

async function chargeOnce(order, provider, store) {
  const existing = await store.getCharge(order.id);
  if (existing && existing.providerRef) return existing;

  const amountCents = order.lines.reduce((sum, line) => {
    return sum + toCents(line.unitPrice) * line.qty;
  }, 0);

  const row = existing || await store.saveCharge({
    orderId: order.id,
    amountCents,
    idempotencyKey: `order:${order.id}:charge`,
    providerRef: null,
  });

  const result = await provider.charge({
    amountCents: row.amountCents,
    idempotencyKey: row.idempotencyKey,
  });

  return store.attachRef(order.id, result.providerRef);
}

module.exports = { chargeOnce, toCents };

Label the snippet above as a proposed patch. It was not executed against a live payment provider here. Run it only after adapting it to a fake provider in CI.

A compact test plan fits in one proposed file. Import the helpers from the module under test first. The file should fail on the float helper and pass on the replay patch.

const test = require('node:test');
const assert = require('node:assert/strict');
const { chargeOnce } = require('./charge-once');

test('recomputes cents and does not charge twice', async () => {
  const calls = [];
  const provider = {
    async charge(payload) {
      calls.push(payload);
      return { providerRef: 'prv_1' };
    },
  };
  const memory = new Map();
  const store = {
    async getCharge(id) {
      return memory.get(id) || null;
    },
    async saveCharge(row) {
      memory.set(row.orderId, row);
      return row;
    },
    async attachRef(id, providerRef) {
      const row = memory.get(id);
      row.providerRef = providerRef;
      return row;
    },
  };
  const order = {
    id: 'ord_9',
    lines: [{ unitPrice: '0.10', qty: 2 }],
  };

  const first = await chargeOnce(order, provider, store);
  const second = await chargeOnce(order, provider, store);

  assert.equal(first.amountCents, 20);
  assert.equal(second.providerRef, 'prv_1');
  assert.equal(calls.length, 1);
});

Treat that test file as an unexecuted example. The assertion on call count is the replay lesson. A timeout injection test should still be added later.

A strict cents check also rejects distorted float totals. Repeated one-tenth prices are a typical failure case. Keep that case in the suite beside the replay test.

Failure cases the old tests hide

A timeout after the provider accepts the charge is the costly case. The client sees an error and the agent loop tries again. Without a stored key, both posts can capture funds.

A decimal string such as ten point one zero becomes a binary number. Multiplied by a quantity, the total can drift by a fraction. Providers that expect integer minor units may reject or mis-round.

A cart edit between page load and submit changes the true total. The request body still carries the stale browser amount. Recomputing from stored line items closes that gap.

A comment that asks for a real patch

Paste a comment that names the file, the rule, and the test. Vague praise does not block a bad merge. The example below is text for the reviewer, not model output.

src/charge.js: amount uses Number, and chargeWithRetry has no idempotency key.
Please store integer cents from line items, save a key before the provider call, and add a node:test that asserts one provider call after a replay.
I will re-review when that test fails on this commit and passes on the fix.

Where a free coding setup fits

MonkeyCode free model access can draft the first review pass. It can draft comments from the billing diff. Disclosure: This article was prepared as part of MonkeyCode's product outreach.

The final review decision stays with a human. A free server from that offering can run the test. Use a clean checkout rather than the agent workspace.

Confirm current free-tier limits on the product page before a review sprint. This article does not claim a token quota. It also omits hardware size and offer duration.

Use the model output as a checklist, not as an approval. Paste that decision table into the model prompt. Ask the model to map each row to file names and line ranges.

Then open those lines and confirm the mapping by hand. Reviewers can try this checklist on the next billing diff. That is a workflow trial, not a launch announcement.

Limitations

Integer cents do not solve currency rules by themselves. Some currencies use no minor-unit digits at all. Others use three digits after the major unit.

The dotted-cents parser above fits only two-digit minor units. Very large major units can exceed the safe integer range. A production helper should use a decimal library for those cases.

Idempotency keys must match the current provider contract. A guessed header name will not dedupe a charge. Read the provider's current API reference before copying the snippet.

The in-memory store will not survive two app instances. Production needs a unique constraint on the order id. This article does not provide that database migration.

The sample never handles partial refunds or tax by jurisdiction. Captured and authorized payment states are also out of scope. Teams with those flows need a ledger design, not this helper.

Free hosted runs can differ from a locked CI image. A passing free-server log is not a release certificate. Re-run the same command in the repository's required checks.

Who should skip this approach

Skip this pattern for card data, bank debits, or regulated payment flows. Those systems need a separately reviewed payment integration. They do not need an agent patch plus a blog snippet.

Skip it when the team cannot recompute totals from stored lines. Client-supplied amounts will keep drifting from the cart. Fix the cart model before reviewing the charge helper.

Skip free-tier execution for secrets, live keys, or customer payloads. Use a fake provider and synthetic orders only. A review machine is not a production vault.

Skip model-written approvals when no human reads the payment diff. The tool can list risks in the changed files. It cannot own the final merge on its own.

Closing the review

The Friday patch was incomplete rather than malicious. Float math and blind retries can double-charge a customer. That risk is enough to block the merge.

Trust the order contract and any test that fails on the old path. Revert binary money and retries that lack an idempotency key. Demand the cents test and the single-charge replay before merge.

Review Agent PRs That Mark User JSON Public

Jordan Li — Thu, 24 Sep 2026 19:38:44 +0000

The scene is a quiet Friday deploy on a small API. An agent pull request adds one invoice route. The diff touches twelve lines and two tests.

Staging passes those tests in under a minute. A shared cache can then mix two user payloads. One session can show the other user's invoice list.

The new route required a valid bearer token. The handler still set a public cache header. That pair is the defect this review targets.

This review targets HTTP cache headers, not in-process maps. An unbounded process Map is a different defect. Header policy is the only subject in this review.

The diff that looked finished

The generated change followed a familiar handler shape. The handler loaded rows for the current user. It returned JSON with a long max age.

// Agent diff excerpt. Treat as untrusted review input.
app.get('/v1/invoices', requireUser, async (req, res) => {
  const rows = await loadInvoices(req.user.id);
  res.set('Cache-Control', 'public, max-age=300');
  res.json({ rows });
});

The new test called the route only once. It asserted status 200 and a row count. It never sent a second identity through a shared cache.

Neat formatting hid an unclear response privacy contract. The handler looked small beside those new tests. The failure showed up only across two users.

Agents often copy cache headers from static asset examples. Those examples are valid only for public files. The same line is unsafe on a user route.

What a reviewer can trust

Trust the route path if the spec already named it. Trust the query when it binds the user id. Trust a test that fails before the header fix.

Do not trust a lone single-user status check. Do not trust public beside the auth middleware. Do not trust a comment that says the CDN is private.

The RFC 9111 text defines Cache-Control response directives. The public directive permits shared caches to store it. The private directive marks a response for one user.

The no-store directive forbids storing the response at all. A shared cache may ignore Authorization without Vary. That ignore path is the cross-user leak.

Header text alone is not proof of safety. must-revalidate does not split users inside a shared cache. It only constrains reuse after the response becomes stale.

RFC 9111 is the primary source for these directive names.

What to revert before merge

Reviewers revert every public directive on authenticated JSON. Reviewers should revert s-maxage on user payloads. Revert tests that never cross two user identities.

Put the conservative header back on the handler. Keep Vary when any proxy can still cache.

// Proposed replacement. Not executed in this draft.
app.get('/v1/invoices', requireUser, async (req, res) => {
  const rows = await loadInvoices(req.user.id);
  res.set('Cache-Control', 'private, no-store');
  res.set('Vary', 'Authorization, Cookie');
  res.json({ rows });
});

Also revert a helper that maps every 200 to one policy. Auth routes and anonymous routes need different directives. One shared helper caused this exact privacy miss.

Numbered review steps

List every route touched by the agent diff.
Mark each touched route as anonymous or authenticated.
Read every Cache-Control write and every Vary write.
Reject public and s-maxage on those authenticated routes.
Add a two-identity test in front of a cache.
Confirm the second body differs from the first body.
Confirm the second status is not a cached 200.
Keep production CDN settings out of this fixture.

Proposed two-identity harness

The harness below is only a written proposal. This article did not execute the harness below. Run it only on a local process or an isolated free server.

// Proposed ESM fixture. Unexecuted example.
import express from 'express';

const app = express();
const invoices = {
  'user-a': [{ id: 'inv-a' }],
  'user-b': [{ id: 'inv-b' }],
};

function requireUser(req, res, next) {
  const token = String(req.get('authorization') || '');
  const userId = token.replace('Bearer ', '');
  if (!invoices[userId]) return res.status(401).end();
  req.user = { id: userId };
  next();
}

app.get('/v1/invoices', requireUser, (req, res) => {
  res.set('Cache-Control', 'private, no-store');
  res.set('Vary', 'Authorization');
  res.json({ rows: invoices[req.user.id] });
});

app.listen(3000);

A tiny shared cache sits in front of the app. This proxy is also a proposed unexecuted example. It stores only responses that carry a public directive.

This proxy is a teaching sketch, not a production cache. It ignores status codes, hop headers, and request bodies. Use it only to show a public-header leak.

// Proposed cache proxy. Unexecuted sketch.
import http from 'node:http';

const store = new Map();

function isPublic(header) {
  return /(?:^|,)\s*public\s*(?:,|$)/i.test(header || '');
}

const proxy = http.createServer((req, res) => {
  const key = req.url;
  if (store.has(key)) {
    res.setHeader('X-Cache', 'HIT');
    return res.end(store.get(key));
  }
  const upstream = http.request(
    { hostname: '127.0.0.1', port: 3000, path: req.url, headers: req.headers },
    (up) => {
      const chunks = [];
      up.on('data', (chunk) => chunks.push(chunk));
      up.on('end', () => {
        const body = Buffer.concat(chunks);
        const cc = String(up.headers['cache-control'] || '');
        if (isPublic(cc)) store.set(key, body);
        res.setHeader('X-Cache', 'MISS');
        res.end(body);
      });
    }
  );
  upstream.end();
});

proxy.listen(3001);

Commands for the check stay short and local. They are proposed commands, not a recorded run.

# Proposed commands. Point them at the fixture only.
curl -sD - -o /tmp/a.json -H 'Authorization: Bearer user-a' \
  http://127.0.0.1:3001/v1/invoices
curl -sD - -o /tmp/b.json -H 'Authorization: Bearer user-b' \
  http://127.0.0.1:3001/v1/invoices
cmp /tmp/a.json /tmp/b.json

The cmp tool should report a file difference. A silent match means user B received user A data. That match is a revert signal, not a flake.

A header unit test still belongs in the suite. It catches a public directive before the proxy run. It does not replace the cross-user body check.

Start the fixture process before the header test. The test assumes the fixture process already listens.

// Proposed test. Unexecuted example.
import assert from 'node:assert/strict';
import test from 'node:test';

test('authenticated invoices are not stored as public', async () => {
  const res = await fetch('http://127.0.0.1:3000/v1/invoices', {
    headers: { authorization: 'Bearer user-a' },
  });
  const cc = res.headers.get('cache-control') || '';
  assert.equal(cc.includes('public'), false);
  assert.match(cc, /private|no-store/);
});

Decision table for the header

Route class	Header to keep	Header to reject	Test that must fail first
Authenticated JSON	`private, no-store`	`public`, `s-maxage`	Second identity sees the first body
Anonymous asset	`public, max-age=60`	blanket `no-store`	Missing cache hit on a public file
Mixed helper	Split by auth flag	One policy for every 200	Auth route inherits `public`

The table is a review aid for headers. It is not a CDN vendor compatibility matrix. Vendor defaults still need their own primary docs.

Where an isolated server helps

Disclosure: This article was prepared as part of MonkeyCode's product outreach.

MonkeyCode is the open-source project named for this workflow. The operator supplied that context for the draft. Free model access and a free server are the relevant options.

The operator states that free model access includes a ten-million-token budget. The operator also states that a free server option exists. This page adds no extra quota, duration, or hardware claim.

Those two options fit this review without touching production. The free server can host the fixture app and the proxy. Free model access can draft the failing two-identity test.

The reviewer still owns the final merge call. A model draft is not a passing test run. A token budget does not measure cache correctness.

A reviewer can place this fixture on that free server. A small share of the stated token budget can draft the failing test. The merge decision stays with the human reading the diff.

Limits of this review

This draft records no live benchmark and no vendor trace. Cache products differ on Vary and on Authorization. A green local proxy does not certify an edge network.

The private directive does not encrypt the body in transit. The no-store directive does not replace access control. TLS, auth, and query binding remain separate checks.

The ten-million-token figure is only an operator-stated budget. It is not a measurement from this article. Duration, model names, and hardware stay unspecified here.

Do not treat those blanks as product promises. Agent diffs can omit Vary while copying a private header. Read both header lines on every auth route.

A downstream override can still leak a private response. Compare the origin header with the edge response. Treat a mismatch as a failed review, not a pass.

Who should skip this approach

Skip the proxy proof when no shared cache exists. The header rule can still land in that case. The two-curl story would overclaim the real path.

Skip a blanket no-store policy on public docs. Those routes want a measured public cache policy. Copying the invoice header there wastes origin capacity.

Skip approval when two identities cannot be minted. Header inspection without a cross-user request is incomplete. Wait for the harness before merging the route.

Skip this method for streaming downloads and range responses. Those responses need a separate cache review path. This article covers small authenticated JSON bodies only.

Closing the review

Trust a bound user id and a failing cross-user test. Remove every public cache directive from authenticated JSON. Exercise a shared cache before approving the merge.

The agent can still write the route handler. The reviewer still owns the response privacy contract. A short harness makes that contract visible in review.

Review Agent PRs That Cache With Unbounded Maps

Jordan Li — Wed, 23 Sep 2026 16:25:48 +0000

A flash sale hit the catalog service at noon. Latency for product reads climbed past two seconds. An agent opened a pull request before the war room ended.

The title claimed a simple in-memory product cache. The diff touched one new module and two call sites. The reviewer almost approved it on size alone.

The change under review

The agent added a module-scoped Map named productCache. Each miss loaded a row and stored the whole record. No TTL, max size, or invalidation path existed.

Missing product ids were cached as null forever. Mutable row objects were returned to callers by reference. Price edits in admin never touched the Map.

// agent-generated PR fragment — review, do not merge
const productCache = new Map();

async function getProduct(db, id) {
  if (productCache.has(id)) {
    return productCache.get(id);
  }

  const rows = await db.query(
    'SELECT * FROM products WHERE id = $1',
    [id]
  );
  const product = rows[0] ?? null;
  productCache.set(id, product);
  return product;
}

async function updatePrice(db, id, price) {
  await db.query(
    'UPDATE products SET price = $1 WHERE id = $2',
    [price, id]
  );
  // no cache delete, no version bump, no TTL
}

This is a common agent performance patch pattern. The tests used three product fixtures and passed. Nothing in the suite spoke to key cardinality.

What to trust

Trust the diagnosis more than the chosen fix. Repeated catalog reads were a real hot path. Extracting a single lookup helper was a reasonable cleanup.

Trust a hit-path assertion against frozen expected JSON. Trust logs that print cache key count after load. Do not trust an unbounded Map as architecture.

What to revert

Revert these pieces before any merge discussion.

Revert the process global Map on multi-instance deploys.
Revert storage of live ORM objects without a clone.
Revert silent caching of null for missing identifiers.
Revert write paths that never delete or bump keys.
Revert cache keys built from unsanitized request parameters.

Each item is a production defect, not a nit. Stale prices during a sale are user-facing failures. A Map that grows with traffic is a memory leak.

Review workflow

Run this review in listed order without skips.

1. Name the key space

Write the cache key formula in the review comment. State the expected distinct key count at peak. Reject the PR if that number is unknown.

key = productId
peak distinct ids last week = REPLACE_WITH_METRICS
process replicas = REPLACE_WITH_DEPLOY

The numbers must come from production metrics. A local Map multiplies memory by replica count. It also splits hit ratio across those processes.

2. Demand an eviction policy in the diff

Accept only an explicit max size and TTL. True LRU eviction is a minimum acceptable bar. Document what happens when the cap is hit.

// proposal only — not production code
const MAX_ENTRIES = 10000;
const TTL_MS = 30000;

function evictIfNeeded(cache) {
  if (cache.size <= MAX_ENTRIES) {
    return;
  }
  const oldestKey = cache.keys().next().value;
  cache.delete(oldestKey);
}

Insertion-order eviction is not a true LRU policy. Call that limitation out in the review.

3. Require invalidation on every writer

List every function that mutates cached rows. Each writer must delete keys or bump a version. Missing one writer means silent stale catalog reads.

async function updatePrice(db, cache, id, price) {
  await db.query(
    'UPDATE products SET price = $1 WHERE id = $2',
    [price, id]
  );
  cache.delete(id);
}

This still fails across processes and nodes. Treat single-process delete as a local bandage.

4. Freeze returned values

Agents often cache the object they mutate later. A caller that sets product.price poisons the Map. Return a structured clone or a frozen copy.

function remember(cache, id, product) {
  const copy = structuredClone(product);
  Object.freeze(copy);
  cache.set(id, { value: copy, exp: Date.now() + TTL_MS });
  return structuredClone(copy);
}

Module scoped maps also break isolation between tests. Pass the cache into helpers during the review.

5. Test cardinality, not only correctness

Green unit tests with three rows prove almost nothing. Add a test that inserts far more keys than the cap. Add a test that updates a cached price and re-reads.

// proposal test — unexecuted until the suite runs
import assert from 'node:assert/strict';

async function testPriceUpdateBustsCache() {
  const cache = new Map();
  const db = makeFakeDb({ 7: { id: 7, price: 10 } });

  const first = await getProduct(db, cache, 7);
  await updatePrice(db, cache, 7, 12);
  const second = await getProduct(db, cache, 7);

  assert.equal(first.price, 10);
  assert.equal(second.price, 12);
}

Reproducible memory harness

The harness below is a local Node script. It is labeled unexecuted until a reviewer runs it. The script fills an unbounded Map and prints RSS.

// harness: cache-rss.mjs — run on a review box
// proposal / unexecuted until a reviewer runs it

const cache = new Map();
const PAYLOAD = JSON.stringify({
  id: 0,
  sku: 'sku-000000',
  title: 'x'.repeat(120),
  attrs: { color: 'red', size: 'm', extra: 'y'.repeat(80) },
});

function rssMb() {
  return (process.memoryUsage().rss / 1024 / 1024).toFixed(1);
}

const start = rssMb();
for (let i = 0; i < 200000; i++) {
  cache.set(i, JSON.parse(PAYLOAD.replace('0', String(i))));
  if (i % 50000 === 0) {
    console.log(JSON.stringify({ i: i, rssMb: rssMb(), size: cache.size }));
  }
}

console.log(JSON.stringify({
  startMb: start,
  endMb: rssMb(),
  keys: cache.size,
}));

Run the script with a hard memory ceiling.

node --max-old-space-size=256 cache-rss.mjs

A healthy review records the next three outcomes.

RSS should climb in step with distinct keys.
The unbounded script should die under 256 MB.
A capped Map should stay inside a stated budget.

Store the console JSON in the PR thread. Do not argue from intuition when RSS is cheap.

Decision table

Observation in the diff	Review action	Merge gate
Helper extraction only	Trust	Optional follow-up
Unbounded module Map	Revert	Block
Null cached forever	Revert	Block
No writer invalidation	Revert	Block
Mutable object returned	Revert	Block
TTL plus max size	Test	Load proof required
Replica-local cache	Test	Document split brain
Shared Redis or Memcached	Test	Auth, TTL, stampede

Use the table in the first review comment. Do not negotiate away the listed block rows.

Running the same review on a spare box

Some teams generate several agent patches overnight. Each patch needs the same RSS and invalidation checks. A spare box keeps that loop off a laptop.

Disclosure: This article was prepared as part of MonkeyCode's product outreach.

MonkeyCode offers free model access and a free server option. Reviewers can regenerate cache patches against the same failing test. The checklist above stays the source of truth.

Teams without a local review box may run the harness there. That is optional and does not replace replica-aware caching.

Limitations

This workflow catches process-local leaks and stale reads. It does not prove cache correctness under multi-node writes. RSS samples are noisy on shared or burstable hosts.

Insertion-order eviction is not LRU under random access. The structuredClone call fails on some driver objects. SELECT * still over-fetches even with a perfect cache.

Free hosted runners share noisy neighbors and CPU. Treat their RSS numbers as directional, not contractual. No duration, model name, or quota is assumed here.

Who should not use this approach

Do not ship in-process caches for prices or stock. Do not cache personal data in a process Map. Do not use this harness as a capacity plan.

Multi-region catalogs need a shared store with TTL. PCI and health data need encryption and strict eviction. Teams without a measured key cardinality should refuse the PR.

Close

Agent cache patches look small and feel kind. They hide latency by moving it into memory growth. Review the key space, writers, and RSS before merge.

The next agent PR will likely repeat this Map. Keep the decision table next to the review template. Merge only after eviction, invalidation, and a memory gate.

Review Agent PRs That Fan Out Unbounded Parallel Calls

Jordan Li — Tue, 22 Sep 2026 14:44:38 +0000

The pull request arrived during a quiet Tuesday standup. An agent had rewritten a slow enrichment loop. CI was green and staging felt twice as fast.

The change replaced a sequential map with Promise.all. Each identifier now hit a downstream billing API together. Nobody counted in-flight sockets before the merge.

Two hours later the payment vendor returned HTTP 429. The Node process also exhausted its HTTP agent pool. Latency for unrelated routes climbed without a clear owner.

This review treats that diff as a teaching case. It states what to keep, revert, and test. The sample code is a labeled, unexecuted example.

What the agent shipped

The agent framed the rewrite as a latency win. The old path awaited each enrich call in order. The new path fired every call in one burst.

// Proposal only: agent-generated handler (do not merge as-is)
async function enrichAccounts(accountIds, client) {
  const rows = await Promise.all(
    accountIds.map((id) => client.get(`/v1/accounts/${id}/balance`))
  );
  return rows.map((row) => row.data);
}

The unit tests stubbed client.get with instant fixtures. They never opened a real network socket. They never asserted a maximum in-flight count.

Downstream behavior stayed outside the automated test graph. Vendor rate limits never appeared in the CI log. Connection pool caps never appeared in those tests either.

What to trust

Trust the diagnosis that sequential awaits wasted idle time. Independent reads can overlap without changing the answers. The account id list can stay the input.

Trust tests that still pin the response mapping. Shape checks on a single record remain useful. Status handling for one isolated call can stay.

Do not trust green CI as capacity evidence. Stubs hide queueing, throttling, and DNS delay. Local duration is not proof of production concurrency.

What to revert

Revert unbounded Promise.all over user-controlled identifier lists. A single request may carry hundreds of ids. The fan-out then becomes an accidental load test.

Revert missing timeouts on each outbound call. A hung vendor socket pins a pool entry. Other tenants then wait on that same agent.

Revert silent omission of partial failure rules. Promise.all fails the whole batch on one reject. Operators then retry the full blast again later.

Revert new retries added beside the fan-out. Combined retries multiply traffic under HTTP 429. That pattern belongs in a later, separate review.

Review workflow

Reviewers can follow a fixed five-step pass. Each step produces a written note on the PR. Skip none of them for agent diffs.

Step 1 — Measure the cardinality

Read every list that feeds the parallel map. Record whether that length comes from the client. Record any server-side cap before the map.

A missing cap is a revert, not a nit. Note the largest production list from logs. If logs are absent, assume a hostile maximum.

Step 2 — Name the shared scarce resources

List sockets, vendor quotas, and shared database pools. List memory buffers held until all awaits finish. List any mutex the downstream service still uses.

If two resources can saturate together, revert the blast. Independent CPU work is not the same case. Network fan-out is the dangerous case here.

Step 3 — Demand an explicit concurrency limit

A hard limit belongs in the function signature. Default values should be conservative on first merge. Magic constants hidden inside helpers will drift later.

// Proposal only: bounded worker queue
async function mapLimit(items, limit, worker) {
  const out = new Array(items.length);
  let next = 0;

  async function run() {
    while (next < items.length) {
      const cur = next++;
      out[cur] = await worker(items[cur], cur);
    }
  }

  const n = Math.min(limit, items.length);
  await Promise.all(Array.from({ length: n }, run));
  return out;
}

The helper still needs timeouts inside the worker. A limit without deadlines only delays the hang. Both controls belong in the same review change.

Step 4 — Define partial failure before merge

Write the policy in the PR body as a table. All-or-nothing is valid only with a tiny batch. Mixed success needs a per-id error envelope.

Callers must not blindly retry identifiers that already succeeded. Idempotency keys belong on every follow-up write. This review does not reopen earlier write-retry guidance.

Step 5 — Prove the cap with a probe

Do not argue from intuition about event-loop overlap. Run a local server that counts in-flight calls. Fail the build when the cap is exceeded.

The next sections contain a concrete local probe. It is small enough for a free host. It does not require access to production traffic.

Reading the agent's justification

Agents often cite average latency from the stubbed suite. That number does not include vendor queue time. Reviewers should ignore latency claims without a live dependency.

Watch for comments that call the change a quick win. Quick wins that drop caps are not wins. Require a peak-in-flight number in the PR instead.

Watch the Node HTTP agent

Node reuses sockets through its default HTTP agent. The default agent does not cap parallel sockets per host. A Promise.all storm can open hundreds of connections.

Set maxSockets on a dedicated agent for that vendor. Keep that agent in module scope, not per request. Creating agents inside the handler leaks sockets over time.

// Proposal only: dedicated agent for one vendor host
import https from "node:https";

export const vendorAgent = new https.Agent({
  keepAlive: true,
  maxSockets: 8,
});

Pass { agent: vendorAgent } into the HTTP client. Fetch in Node 20 uses undici, not https.Agent. For fetch, cap concurrency inside mapLimit instead.

A reproducible concurrency probe

Save the two files under probes/fanout/. Run them with Node 20 or newer. Treat those numbers as local evidence only.

// probes/fanout/mock-vendor.mjs
import http from "node:http";

let inFlight = 0;
let peak = 0;

const server = http.createServer((req, res) => {
  inFlight += 1;
  peak = Math.max(peak, inFlight);
  setTimeout(() => {
    inFlight -= 1;
    res.writeHead(200, { "content-type": "application/json" });
    res.end(JSON.stringify({ ok: true, peak }));
  }, 80);
});

server.listen(4377, () => {
  console.log("mock vendor on 4377");
});

The mock holds every request for eighty milliseconds. Overlap then becomes visible as the peak. A sequential client should report a peak of one.

// probes/fanout/run.mjs
import assert from "node:assert/strict";

const LIMIT = Number(process.env.FANOUT_LIMIT || 4);
const N = Number(process.env.FANOUT_N || 40);
const MAX_PEAK = Number(process.env.FANOUT_MAX_PEAK || 4);

async function getBalance(id) {
  const ac = new AbortController();
  const t = setTimeout(() => ac.abort(), 2500);
  try {
    const res = await fetch(`http://127.0.0.1:4377/v1/accounts/${id}`, {
      signal: ac.signal,
    });
    if (!res.ok) throw new Error(`status ${res.status}`);
    return res.json();
  } finally {
    clearTimeout(t);
  }
}

async function mapLimit(items, limit, worker) {
  const out = new Array(items.length);
  let next = 0;
  async function run() {
    while (next < items.length) {
      const i = next++;
      out[i] = await worker(items[i], i);
    }
  }
  const n = Math.min(limit, items.length);
  await Promise.all(Array.from({ length: n }, run));
  return out;
}

const ids = Array.from({ length: N }, (_, i) => String(i + 1));
const rows = await mapLimit(ids, LIMIT, getBalance);
const peak = Math.max(...rows.map((r) => r.peak));

assert.equal(rows.length, N);
assert.ok(peak <= MAX_PEAK, `peak ${peak} exceeded ${MAX_PEAK}`);
console.log(JSON.stringify({ n: N, limit: LIMIT, peak }, null, 2));

Run the mock and the probe in two terminals.

node probes/fanout/mock-vendor.mjs
FANOUT_LIMIT=4 FANOUT_N=40 FANOUT_MAX_PEAK=4 node probes/fanout/run.mjs

Read the printed peak after a successful run. A peak of four means the cap held. A peak of forty means the helper never engaged.

Temporarily replace mapLimit with a raw Promise.all call. Keep FANOUT_MAX_PEAK at four for that run. The process should exit with an assertion error.

Record both command outputs in the PR thread. A screenshot is weaker than the JSON line. Reviewers should rerun the probe after later agent edits.

Decision table for the review note

Observation on the diff	Trust	Revert	Required test
Agent parallelizes independent GETs	Intent to overlap reads	Unbounded list	Peak in-flight cap
Vendor documents a tight route quota	Quota text as a clue	Client-side blast	429 fixture plus backoff
Handler maps a user-supplied id array	Input shape	Missing max length	Reject over cap before fetch
Tests mock fetch with instant data	Isolation	Using them as load proof	Local mock with delay
Agent adds `Promise.allSettled` only	Partial error objects	Still unbounded	Peak cap plus error envelope
Timeouts wrap each worker	Per-call deadline	Sharing one abort for all	Abort after 2500ms per call

The table belongs in the written review comment. It keeps later agents from reopening the same hole. It also stops reviewers from arguing from vibe.

Where a free review host fits

Agent diffs need a place that can run the probe. A laptop works, yet shared review queues still help. MonkeyCode offers free model access and a free server option for that loop.

Disclosure: This article was prepared as part of MonkeyCode's product outreach.

Free model access can apply the checklist to a new diff. It must not replace the probe or the revert list. Reviewers still paste command output before any approval.

The host does not prove vendor behavior under contract. It only proves that the client cap exists. Keep production soak tests on the real staging path.

Some teams already park review probes on a spare host. That free server option can run this fan-out check.

Limitations

The probe measures one process on one machine. Cluster-wide fan-out remains invisible to this probe. Multiple replicas can still multiply the vendor traffic.

The eighty millisecond delay is a stand-in only. Real p99 values will differ by region. Do not treat peak four as a universal default.

The helper is not a full job queue. It ignores fairness across incoming HTTP request traffic. A public API still needs admission control at the edge.

This workflow does not certify correctness of balances. It only constrains how hard the service pushes. Schema and auth reviews stay on their own checklists.

Model-written review text can miss hidden extra maps. Nested helpers may fan out after this pass. Search the diff for every Promise.all call site.

Who should skip this approach

Teams with a mesh retry budget already under test may skip it. Their gateway may already cap per-route outbound concurrency. Duplicating the probe then adds review noise.

Do not use this method as a vendor load test. Permission for that work lives with the vendor. A mock server is not their production fabric.

Do not merge because a model replied with approval. Generated approval text is not merge evidence. The probe JSON line is the evidence.

Skip the rewrite when the list is always length one. Parallelism then buys nothing and only adds code. Sequential await remains the clearer default path.

Close

Unbounded Promise.all remains a common agent shortcut. It looks like performance work in the diff. It is often a reliability defect in production.

Keep the independent-read intent when the data allows it. Revert missing caps, deadlines, and partial-failure rules together. Test peak in-flight count on a boring mock.

Review Agent PRs That Search by Loading Every Row

Jordan Li — Mon, 21 Sep 2026 12:55:59 +0000

A reviewer opened an agent pull request on Monday morning.
The description claimed a fast order search endpoint.

The diff looked small, typed, and fully tested.
Green CI sat on a single happy-path fixture.

The route accepted q, status, and from parameters.
The handler then loaded every order for the tenant.

The patch under review

The illustrative agent patch below is not production code.
It matches a pattern that shows up in generated list endpoints.

// illustrative agent patch — do not ship
app.get("/orders", auth, async (req, res) => {
  const { q, status, from } = req.query;
  const orders = await db.order.findMany({
    where: { tenantId: req.tenantId },
  });
  const filtered = orders.filter((o) => {
    const text = `${o.id} ${o.email} ${o.notes}`;
    const hit = !q || text.toLowerCase().includes(String(q).toLowerCase());
    const st = !status || o.status === status;
    const dt = !from || o.createdAt >= new Date(from);
    return hit && st && dt;
  });
  const result = [];
  for (const order of filtered) {
    const items = await db.item.findMany({ where: { orderId: order.id } });
    result.push({ ...order, items });
  }
  res.json({ orders: result, total: result.length });
});

The TypeScript types for the handler compile cleanly.
The handler uses the tenant id from the session.

That combination is the actual review trap.
Correct auth on the outer query hides a cost bomb.

What CI did not see

The fixture inserted only three tenant orders.
The assertion checked email match and item count.

// illustrative test from the same PR
it("finds an order by email fragment", async () => {
  await seedOrders(3);
  const res = await request(app)
    .get("/orders?q=buyer@")
    .set("Authorization", token);
  expect(res.status).toBe(200);
  expect(res.body.orders).toHaveLength(1);
});

Three rows never expose a full table scan.
Three extra item queries never trip a pool timeout.

The test proves a filter works on a toy set.
It does not prove the query plan stays bounded.

Agents often cite “tenant isolation already applied”.
Isolation without a cap still copies the whole tenant.

What to trust

Trust only artifacts that survive a hostile input size.

Keep tenant scoping on the first database predicate.
Keep auth middleware that rejects a missing token.
Keep stable response field names clients already consume.

Do not trust in-memory filters as access control.
Do not trust total when it counts an unbounded array.
Do not trust loops that fetch children per parent row.

A compile-clean select * equivalent is still a smell.
A mapped DTO does not shrink the rows first read.

What to revert

Revert the full table read before anything else.
Revert the JavaScript filter that copies every row.
Revert the per-order item query inside the loop.

Also revert silent casts of from through new Date.
Invalid dates become Invalid Date and fail closed or open.
Agents often pick the open path without a comment.

Keep the route path if the contract is already public.
Keep tenant isolation if a human already reviewed that helper.

Revert total: result.length on the same pass.
That number is a page size, not a table statistic.

Review workflow

Reviewers can run this sequence on any generated list PR.

Open the handler and list every findMany, select, or fetch.
Mark each call that lacks take, limit, or a cursor.
Mark each loop that performs another query or HTTP call.
Demand a failing test with a large fixture before merge.
Require the rewrite to push filters into the database.
Require a hard cap even after the database filter lands.

The next sections give a concrete test and a bounded rewrite.
Both are labeled proposals, not measured production results.

A failing test the agent will not write

Seed enough rows to make the naive path expensive.
Keep the seed local, disposable, and fully deterministic.

# proposed local seed — unexecuted in this article
npx prisma db push
node scripts/seed-orders.mjs --count 5000 --tenant t_demo
npx vitest run tests/orders-search.unbounded.test.js

// proposed test — unexecuted in this article
it("does not materialize the whole tenant order set", async () => {
  await seedOrders(5000);
  const start = Date.now();
  const res = await request(app)
    .get("/orders?q=needle-only-once&limit=20")
    .set("Authorization", token);
  const ms = Date.now() - start;
  expect(res.status).toBe(200);
  expect(res.body.orders.length).toBeLessThanOrEqual(20);
  expect(res.body.orders.length).toBeGreaterThan(0);
  expect(ms).toBeLessThan(500);
});

Five thousand rows is a floor, not a load test.
The assertion on duration is a tripwire, not a benchmark.

Tune the duration against the review machine, not a slogan.
Reject the PR if the handler has no limit argument at all.

Wall clocks flake, so count statements when logs exist.

// proposed — count statements instead of wall clock when possible
const queries = [];
db.$on("query", (e) => queries.push(e));
await request(app)
  .get("/orders?q=needle-only-once&limit=20")
  .set("Authorization", token);
expect(queries.filter((q) => /FROM\s+"Item"/i.test(q.query))).toHaveLength(1);
expect(queries.some((q) => /LIMIT/i.test(q.query))).toBe(true);

One child query for the page is the merge bar.
A child query per parent row still fails this gate.

Bounded rewrite to request

Push search predicates down into the query engine.
Cap the page size and load children in one query.

// proposed rewrite — review against your ORM
function parseStatus(value) {
  const allowed = new Set(["open", "paid", "canceled"]);
  if (value == null || value === "") return null;
  const status = String(value);
  if (!allowed.has(status)) {
    const err = new Error("invalid_status");
    err.statusCode = 400;
    throw err;
  }
  return status;
}

function parseDate(value) {
  if (value == null || value === "") return null;
  const ms = Date.parse(String(value));
  if (Number.isNaN(ms)) {
    const err = new Error("invalid_from");
    err.statusCode = 400;
    throw err;
  }
  return new Date(ms);
}

function decodeCursor(value) {
  if (!value) return null;
  if (!/^[a-z0-9_-]{8,64}$/i.test(String(value))) {
    const err = new Error("invalid_cursor");
    err.statusCode = 400;
    throw err;
  }
  return String(value);
}

app.get("/orders", auth, async (req, res) => {
  const q = String(req.query.q || "").slice(0, 64);
  const status = parseStatus(req.query.status);
  const from = parseDate(req.query.from);
  const limit = Math.min(Number(req.query.limit) || 20, 100);
  const cursor = decodeCursor(req.query.cursor);

  const orders = await db.order.findMany({
    where: {
      tenantId: req.tenantId,
      ...(status ? { status } : {}),
      ...(from ? { createdAt: { gte: from } } : {}),
      ...(q
        ? {
            OR: [
              { email: { contains: q, mode: "insensitive" } },
              { notes: { contains: q, mode: "insensitive" } },
            ],
          }
        : {}),
    },
    take: limit + 1,
    ...(cursor ? { cursor: { id: cursor }, skip: 1 } : {}),
    orderBy: [{ createdAt: "desc" }, { id: "desc" }],
    select: {
      id: true,
      email: true,
      status: true,
      notes: true,
      createdAt: true,
    },
  });

  const page = orders.slice(0, limit);
  const ids = page.map((o) => o.id);
  const items = ids.length
    ? await db.item.findMany({
        where: { orderId: { in: ids } },
        select: { id: true, orderId: true, sku: true, qty: true },
      })
    : [];
  const grouped = Map.groupBy(items, (row) => row.orderId);
  const next = orders.length > limit ? page[page.length - 1].id : null;

  res.json({
    orders: page.map((o) => ({ ...o, items: grouped.get(o.id) || [] })),
    nextCursor: next,
  });
});

The rewrite still needs an index on tenant, createdAt, and email.
Search on notes can still degrade without a dedicated strategy.

That is acceptable for a first merge bar.
It is not a replacement for a search product.

Reject a cursor that is not an identifier the tenant owns.
A leaked foreign id must not page another tenant’s table.

Decision table for the reviewer

Signal in the diff	Trust	Revert	Test
findMany with only tenantId	Isolation only	Unbounded read	Seed 5k rows
JS filter on query params	None	Whole filter	Invalid from
Loop plus findMany per id	None	N+1 loop	Statement count
total equals result.length	None	Fake totals	Page versus table
new Date(from) with no parse	None	Implicit cast	from=yesterday
limit missing from contract	None	Open page size	limit=100000
Cursor fields present	Shape only	Weak encoding	Tampered cursor

Read the table as a merge gate, not a style guide.
Any single red cell is enough to block.

Running the review loop on a free server

Human review still owns the final merge decision.
A hosted coding model can only draft the first pass.

Disclosure: This article was prepared as part of MonkeyCode's product outreach.
MonkeyCode offers free model access and a free server option.
Those two facts are the only product claims used here.

A practical review loop looks like the following.

Paste the handler and the test file into the model.
Ask for unbounded reads, N+1 loops, and missing caps.
Ignore any suggested rewrite until a failing test exists.
Run that test on the free server against a local database.
Merge only after the bounded query and the cap both land.

Do not ask the model to certify performance.
Do not paste production dumps, tokens, or customer emails.

The free server is useful for the 5k-row fixture.
It is not a substitute for staging traffic replay.

Reviewers who need a disposable host can use that free server after tests exist.

Limitations

This workflow catches materialization bugs on list reads.
It does not catch missing object-level authorization on GET /orders/:id.

It does not replace EXPLAIN on the real engine.
It does not prove index quality under production cardinality.

Duration assertions often flake on noisy shared hosts.
Prefer counting rows read when the ORM exposes query logs.

Cursor pagination still needs a stable orderBy clause.
Ties on createdAt will skip or repeat rows without id.

Full-text search needs remain outside this merge bar.
Notes search with contains can still scan a large index.

ILIKE plus leading wildcards will ignore many btree indexes.
Treat that as a later search ticket, not a silent merge.

Who should not use this approach

Do not use the 5k-row tripwire as a capacity plan.
Platform teams with existing query budgets already have better gates.

Do not run this seed against shared staging data.
The fixture belongs on a disposable database volume.

Do not accept a model summary as evidence of safety.
The value is the failing test and the bounded SQL.

Skip the rewrite if the endpoint is an internal dump.
Those paths need a job, a file, and an explicit ACL.

Treat every unbounded list handler as unmerged until a cap exists.
The agent can draft the route. The reviewer owns the bound.

Review Agent PRs That Ignore Client Abort Signals

Jordan Li — Sun, 20 Sep 2026 11:07:15 +0000

A billing enrichment pull request landed just after midnight. The coding agent wired a partner HTTP call into checkout. The recorded happy-path unit test stayed completely green.

The handler kept the outbound fetch alive anyway. Client disconnects did not cancel remaining database work. A small host then burned sockets on abandoned requests.

The failure mode in agent diffs

Coding agents optimize for a successful JSON body. They copy fetch calls from common public tutorials. They rarely thread the incoming request signal downward.

The diff still looks finished to a tired reviewer. Client cancellation, refresh, and proxy timeouts never appear. The merge then leaks work onto every dropped tab.

This cancellation bug is not exotic in production traffic. Modern browsers abort in-flight calls on navigation. Mobile clients abort calls during aggressive backgrounding.

Each abort should stop remaining outbound I/O. Agent-generated pull requests often keep going instead. That waste shows first on a free or small server.

What the agent usually commits

The following snippet is a labeled, unexecuted example. It shows a review pattern, not a recipe.

// unexecuted example — agent-style handler
export async function enrichOrder(req, res) {
  const orderId = req.params.id;
  const order = await db.orders.findById(orderId);
  const extra = await fetch(order.partnerUrl).then((r) => r.json());
  await db.orders.update(orderId, { extra });
  res.json({ ok: true, extra });
}

The partner call has no timeout budget. The client abort never reaches fetch. Updates still run after the requester leaves.

A second pattern shows up in helper modules. The agent starts work with void and moves on.

// unexecuted example — fire-and-forget after response
res.json({ ok: true });
void fetch(order.partnerUrl);

That form is worse under modest load. Errors vanish into unhandled rejections. Cancellation becomes impossible to test later.

Scan the PR with commands

Run these commands on the agent branch. They do not prove correctness. They only list suspects for the human review.

git fetch origin
git checkout pr-agent-enrich
git grep -nE "fetch\(|axios\(|got\(|undici" -- '*.js' '*.ts'
git grep -nE "void fetch|void axios" -- '*.js' '*.ts'
git grep -nE "AbortSignal|req\.signal|signal:" -- '*.js' '*.ts'
git grep -nE "res\.json\(|res\.send\(" -- '*.js' '*.ts'

A fetch hit without a nearby signal is a revert candidate. A res.json hit above a later await is another. Record both paths in the review notes.

How to read the diff

Follow this numbered pass before leaving comments.

Find every outbound HTTP helper in the changed files.
Check whether req.signal is an argument into that helper.
Check for a timeout signal beside the client signal.
Check that no await remains after the response is sent.
Check that retries reuse the original abort signal object.

Stop the pass when any step fails. One missing signal is enough to block merge. Style nits can wait until cancellation is real.

What to trust

Reviewers can trust naming and the intended checkout flow. The agent often picks the correct route file. It may reuse an existing order lookup helper.

Trust stubbed 200 tests for the partner shape. Those tests prove parsing on the happy path. They do not prove abort behavior at all.

Trust comments that mention timeouts only after a diff check. Agents leave timeout comments after removing the timeout. The comment is not evidence.

What to revert before merge

Use this numbered revert list during review.

Revert fetch calls that omit signal and a timeout.
Revert void or unawaited promises in request handlers.
Revert partner writes that continue after res.json.
Revert retries that recreate fetch without the same signal.
Revert debug logs that dump the entire partner payload.

Do not keep a temporary unbounded call. Temporary fetch helpers become permanent fixtures. Small servers feel that leak first and loudest.

Also revert partner URLs taken from the client body. Handlers should allowlist stored hosts instead. Cancellation cannot fix an unbounded destination list.

What to test instead

The original artifact is this abort test plan. Run it on the PR branch. Do not accept green CI that never aborts.

Step 1. Prove the handler accepts a signal

Pass req.signal or an equivalent AbortController. The partner fetch must receive that same signal. A unit stub should reject with AbortError when aborted.

Step 2. Abort before the partner responds

Start the request, then abort at ten milliseconds. The test must see no orders.update call. The process must not log an unhandled rejection.

Step 3. Abort during a slow write

If the write cannot cancel, document that driver limit. The handler should still stop follow-up fetches. It should not retry after the abort fires.

Step 4. Bound every outbound call

Set an explicit timeout besides the client abort. Client abort and server timeout differ in source. Both must close the outbound socket.

Step 5. Reject unknown partner hosts

Feed a URL whose host is not allowlisted. The handler must not open a socket. This check belongs beside cancellation, not after it.

Reproducible harness

Label this harness as a proposal. Operators should adapt names to the real app. The test uses Node.js node:test and built-in fetch.

// abort-review.mjs — proposal for PR CI
import test from 'node:test';
import assert from 'node:assert/strict';

let updateCalls = 0;

const db = {
  orders: {
    async findById(id) {
      return { id, partnerUrl: 'http://127.0.0.1:9/slow' };
    },
    async update() {
      updateCalls += 1;
    },
  },
};

async function enrichOrder(req) {
  const order = await db.orders.findById(req.params.id);
  const extra = await fetch(order.partnerUrl, {
    signal: AbortSignal.any([
      req.signal,
      AbortSignal.timeout(2000),
    ]),
  }).then((r) => r.json());
  await db.orders.update(order.id, { extra });
  return extra;
}

test('enrichment stops when the client aborts', async () => {
  updateCalls = 0;
  const controller = new AbortController();
  const req = { params: { id: 'ord_1' }, signal: controller.signal };

  const pending = enrichOrder(req);
  controller.abort();

  await assert.rejects(pending, { name: 'AbortError' });
  assert.equal(updateCalls, 0);
});

Run it with a single command.

node --test abort-review.mjs

A failing agent PR throws later, during connect. It also increments updateCalls. Reviewers should reject that branch on sight.

The corrected production shape looks like the next snippet. It remains a proposal for the real service. Wire it to the real logger and host allowlist.

// proposal — forward abort, timeout, and host allowlist
const PARTNER_HOSTS = new Set(['partner.example.internal']);

export async function enrichOrder(req, res) {
  const order = await db.orders.findById(req.params.id);
  const url = new URL(order.partnerUrl);
  if (!PARTNER_HOSTS.has(url.hostname)) {
    res.status(400).json({ error: 'partner_host_rejected' });
    return;
  }

  const signal = AbortSignal.any([
    req.signal,
    AbortSignal.timeout(2000),
  ]);

  try {
    const response = await fetch(url, { signal, redirect: 'error' });
    if (!response.ok) throw new Error('partner_http');
    const extra = await response.json();
    if (req.signal.aborted) return;
    await db.orders.update(order.id, { extra });
    res.json({ ok: true });
  } catch (err) {
    if (err.name === 'AbortError') {
      if (!res.headersSent) res.status(499).end();
      return;
    }
    if (!res.headersSent) {
      res.status(502).json({ error: 'partner_unavailable' });
    }
  }
}

The allowlist is not optional on a small host. Unbounded destinations turn into outbound floods. Cancellation without an allowlist still wastes DNS and sockets.

Decision table for reviewers

Diff signal	Trust	Action	Required test
`fetch(url)` with no options	Intent only	Add signal and timeout	Abort at 10ms
`void fetch(...)`	Nothing	Revert	Unhandled rejection check
Retry wrapper around fetch	Backoff math only	Pass the original signal	Abort mid-retry
`res.json` then `await`	Nothing	Revert the order	No write after abort
Partner host from request body	Nothing	Allowlist stored hosts	Unknown host opens zero sockets

Keep this table in the PR template. Agents will still omit signal. The table makes the revert boring and repeatable.

Reviewing on a constrained box

Abandoned fetches hurt most on small shared hosts. File descriptors run out first. Outbound NAT slots run out second.

Disclosure: This article was prepared as part of MonkeyCode's product outreach.

MonkeyCode is an open-source project with free model access. It also includes a 10 million free token grant and a free server option. Those resources fit a cancellation review harness. The model can draft abort tests from a diff. The server can run node --test on each agent branch.

Do not treat generated notes as the review. The failing test is the review. Tokens only help sketch stubs. The free server is one always-on worker for that command.

Reviewers who need a small box for the harness can try that free server option.

Limitations

AbortSignal does not cancel every database driver. Some pools finish the in-flight query. Some ORMs ignore signal today.

AbortSignal.any needs a current Node.js runtime. Older runtimes need a manual abort listener. The two-second timeout is an example, not a default.

This workflow does not replace load testing. It only catches leaked work after disconnect. It will not catch slow queries that finish inside the timeout.

Status 499 is a conventional local choice. Proxies may map it differently. Teams can log and close without a body instead.

Who should not use this approach

Do not use this harness as the only production gate. Teams without Node 20 plus must adapt APIs. Codebases without fetch need their own client abort hooks.

Do not abort paths that already committed payments. Those paths need idempotent completion. Cancellation belongs to HTTP side effects, not ledger writes.

Skip this pattern for batch jobs without a client. Jobs need a separate deadline signal. Pretending req.signal exists there hides bugs.

Frontend-only PRs do not need this server harness. Browser AbortController still matters there. The tests above target Node request handlers.

Close

Agent PRs often look done at status 200. Cancellation is the missing branch. Review the signal path, then merge.

Review Agent PRs That Hold Row Locks Across Fetch

Jordan Li — Sat, 19 Sep 2026 09:22:16 +0000

A checkout worker locked inventory rows during a Tuesday deploy. The agent pull request wrapped charge and email calls inside one transaction. Unit tests stayed green because payment mocks returned at once.

This review pattern appears in many agent-generated diffs. The patch reads like careful consistency for stock and payment. Production then waits on payment, mail, or a webhook.

The failure class

Agents reach for transactions when they notice races. They open BEGIN, then call fetch, then COMMIT. Row locks stay held for the whole network wait.

Other requests pile up on the same SKU row. Timeouts present as database load in dashboards. The real cause still sits in the pull request diff.

Agents also wrap SDK retries inside the same BEGIN block. Each retry extends the lock by another round trip. A flaky payment provider then serializes checkout for one SKU.

What the Tuesday PR contained

The next snippet is an unexecuted example. It mirrors a common agent-generated checkout patch.

// unexecuted example: agent-generated checkout handler
async function checkout(req, db) {
  const { sku, userId, paymentMethod } = req.body;
  await db.query("BEGIN");
  try {
    await db.query(
      "SELECT qty FROM inventory WHERE sku = $1 FOR UPDATE",
      [sku]
    );
    const charge = await fetch("https://payments.example/charge", {
      method: "POST",
      headers: { "content-type": "application/json" },
      body: JSON.stringify({ userId, sku, paymentMethod }),
    });
    if (!charge.ok) throw new Error("charge failed");
    await db.query(
      "UPDATE inventory SET qty = qty - 1 WHERE sku = $1",
      [sku]
    );
    await fetch("https://notify.example/email", {
      method: "POST",
      body: JSON.stringify({ userId, sku }),
    });
    await db.query("COMMIT");
    return { ok: true };
  } catch (err) {
    await db.query("ROLLBACK");
    throw err;
  }
}

The SELECT FOR UPDATE looks responsible in review. The two fetch calls do not belong inside that transaction. Charge latency now owns the inventory lock.

What to trust, revert, and test

Reviewers can split the diff with a hard rule. Database reads and writes may stay in the transaction. Network, disk, and queue I/O must leave it.

1. Trust the local invariant

Trust a short transaction that touches one aggregate. Trust SELECT FOR UPDATE followed only by local SQL. Trust COMMIT that happens before any fetch await.

2. Revert the outbound calls

Revert fetch, SDK, and mailer calls from the transaction body. Revert retries placed between BEGIN and COMMIT. Revert awaited remote logging inside the same block.

3. Test lock duration, not mock counts

Tests that stub fetch in one millisecond miss this class. A harness must keep the transaction open. A second client must probe the same row.

Review procedure

Follow these five steps on every agent PR that touches SQL.

Collect the transaction span from the diff hunks. Note BEGIN, COMMIT, and ORM transaction callbacks.
Mark every await that is not a database call. Include fetch, queues, object storage, and email SDKs.
Revert those awaits to after COMMIT or to a worker. Keep only local writes in the critical section.
Add a two-client lock test on the same key. Fail the build when payment I/O blocks the second session.
Require an outbox row or explicit compensation path. Do not return charged before the worker finishes.

Reviewers should cite file and line for each mark. Review comments without line numbers get ignored later.

Commands that surface the span

Start with the merge diff, not the whole tree. Network tokens beside BEGIN are the first signal.

git diff origin/main...HEAD -- '*.js' '*.ts' \
  | rg -n "BEGIN|startTransaction|\.transaction\(|fetch\(|COMMIT|commit\("

On a staging Postgres, inspect ungranted locks during a dry run. This command is an unexecuted example for a disposable database.

psql "$DATABASE_URL" -c "\
SELECT pid, mode, granted, relation::regclass \
FROM pg_locks \
WHERE NOT granted;"

A blocked pid during a mocked payment is enough evidence. Merge should wait until that wait disappears.

A reproducible scan artifact

The Node script below is a review aid. It scans a source file for BEGIN plus network calls. Treat it as a heuristic, not a full ORM parser.

// review-tx-fetch.js — run against a patch or source file
const fs = require("fs");

const NETWORK = /\b(fetch|axios|got|stripe|twilio|s3|sns|sqs)\b/i;
const BEGIN = /\b(BEGIN|startTransaction|transaction\()/i;
const COMMIT = /\b(COMMIT|commit\()/i;

function windows(src) {
  const lines = src.split(/\n/);
  const hits = [];
  let open = null;
  for (let i = 0; i < lines.length; i++) {
    if (BEGIN.test(lines[i])) open = { start: i + 1, net: [] };
    if (open && NETWORK.test(lines[i])) open.net.push(i + 1);
    if (open && COMMIT.test(lines[i])) {
      if (open.net.length) hits.push({ ...open, end: i + 1 });
      open = null;
    }
  }
  return hits;
}

const file = process.argv[2];
const src = fs.readFileSync(file, "utf8");
const hits = windows(src);
if (!hits.length) {
  console.log("no begin/fetch overlap found");
  process.exit(0);
}
for (const h of hits) {
  console.log(
    `transaction ${h.start}-${h.end} calls network at ${h.net.join(",")}`
  );
}
process.exit(1);

Run it on the checkout file before approval.

node review-tx-fetch.js checkout.js

A non-zero exit from the scanner should block merge. Humans still confirm wrappers the regex cannot see.

A lock-duration test plan

The next test is a labeled proposal for local Postgres. It uses two clients and a deferred stand-in for fetch.

// proposal: lock-duration.test.js
const { Pool } = require("pg");

const slowFetch = () =>
  new Promise((resolve) => setTimeout(() => resolve({ ok: true }), 1500));

test("second buyer is not blocked by payment I/O", async () => {
  const pool = new Pool({ connectionString: process.env.DATABASE_URL });
  const a = await pool.connect();
  const b = await pool.connect();
  await a.query("BEGIN");
  await a.query("SELECT qty FROM inventory WHERE sku = $1 FOR UPDATE", [
    "sku-1",
  ]);
  const blocked = b.query(
    "SELECT qty FROM inventory WHERE sku = $1 FOR UPDATE NOWAIT",
    ["sku-1"]
  );
  await slowFetch();
  await a.query("COMMIT");
  await expect(blocked).rejects.toMatchObject({ code: "55P03" });
  a.release();
  b.release();
  await pool.end();
});

The proposed test documents the failure first. After the fix, client B must not hit lock_not_available during payment I/O. Mock timers alone are not evidence of lock release.

The safer handler shape

Move side effects until after COMMIT. Persist an outbox row in the same transaction. A worker then sends payment and email without holding row locks.

// unexecuted example: local transaction, later side effects
async function checkout(req, db, queue) {
  const { sku, userId, paymentMethod } = req.body;
  const orderId = crypto.randomUUID();
  await db.query("BEGIN");
  try {
    const { rows } = await db.query(
      "SELECT qty FROM inventory WHERE sku = $1 FOR UPDATE",
      [sku]
    );
    if (!rows[0] || rows[0].qty < 1) throw new Error("sold out");
    await db.query("UPDATE inventory SET qty = qty - 1 WHERE sku = $1", [
      sku,
    ]);
    await db.query(
      "INSERT INTO outbox (id, kind, payload) VALUES ($1, $2, $3)",
      [
        orderId,
        "charge_and_email",
        JSON.stringify({ sku, userId, paymentMethod }),
      ]
    );
    await db.query("COMMIT");
  } catch (err) {
    await db.query("ROLLBACK");
    throw err;
  }
  await queue.publish("outbox", { orderId });
  return { accepted: true, orderId };
}

The handler now returns accepted, not charged. Payment failures retry without holding inventory locks during HTTP. Compensation still needs an explicit, tested design.

Decision table for the review thread

Paste this table into the agent PR. Each row needs a file and line citation.

Diff signal	Trust	Revert	Test
SELECT FOR UPDATE then SQL only	yes	no	qty invariant
fetch or SDK between BEGIN and COMMIT	no	yes	second session NOWAIT
retry wrapper around charge inside tx	no	yes	lock wait on pg_locks
outbox insert inside tx	yes	no	row visible after COMMIT
email SDK after COMMIT	yes	no	at-least-once worker

Reviewers should reject a green CI that never opened a second session. Mock call counts do not measure lock duration.

Running the scanner on a spare host

Teams can run the regex scanner on any laptop. A spare Linux box helps when the repo already expects remote CI.

Disclosure: This article was prepared as part of MonkeyCode's product outreach. MonkeyCode is an open-source project with operator-stated free model access and a free server option. The scanner and lock test can run on that free server when local Postgres is awkward. The method above stays useful without that host.

Limitations

The regex misses Knex, Prisma, and TypeORM callback transactions. It also misses SQL assembled at runtime. A green scan is not proof of safety.

The lock test needs real row locking behavior. SQLite and some pooled setups will not reproduce NOWAIT. Do not treat fake clocks as lock evidence.

Outbox designs create at-least-once side effects. Double charge is a different incident class. Idempotency keys still belong on the payment worker.

Who should not use this approach

Do not apply the revert rule to single-writer scripts. Do not force outbox infrastructure onto a one-user prototype. Do not enable the scanner as a linter without ORM review.

Skip any remote server path for production database dumps. Review data must stay synthetic and disposable.

After merge

Agent PRs often hide latency inside BEGIN. The durable fix is a shorter transaction and a later worker. Cite the network lines, run the two-client test, then merge.

Review Agent PRs That Retry Non-Idempotent Writes

Jordan Li — Fri, 18 Sep 2026 07:18:39 +0000

The checkout service froze after a double charge report. A Friday agent pull request had shipped a retry helper. The helper wrapped a POST that created paid orders.

Support later found two rows for one cart. This pattern shows up in agent diffs every week. The model sees a timeout and reaches for retries.

The suite stays green because tests never replay the write. Reviewers need a tighter loop than a green check. Treat the retry helper as untrusted until a replay test fails.

The scene inside the diff

The agent named the change make checkout resilient. The patch added a small withRetry utility around fetch. It caught network failures and ran the same POST again.

Comments claimed the change followed production best practice. The production handler was not idempotent at all. Each success inserted an order and captured a payment.

A retry after a slow 201 created a second capture. The UI showed one confirmation and two receipts. Finance only noticed during the next settlement window.

What to trust in the agent patch

Trust the diagnosis of a real timeout class. Network blips and slow gateways do exist. Trust a extracted helper only when the helper stays pure.

Trust tests that fail on a second insert. Trust a unique index more than a comment. Trust a stored idempotency key over a retry count.

Do not trust comments that cite resilience without a key. Do not trust a green suite with one happy path. Do not trust backoff copied from a generic blog snippet.

What to revert on sight

Revert retries around POST, PUT, PATCH, or DELETE writes. Revert helpers that recapture payment after a timeout. Revert sleeps that hide duplicate side effects in tests.

Revert changelog lines that call the handler idempotent. Keep a retry only when the operation is proven safe. Safe means the same key yields one business effect.

Safe also means tests prove the second call is a no-op. Anything else is a revert, not a nit. Write that revert reason in the review thread.

A concrete bad patch

The following TypeScript is a typical agent result. Treat it as an unexecuted example. It is a review fixture, not production code.

// unexecuted example — agent retry around a write
type CheckoutBody = {
  cartId: string;
  amountCents: number;
};

async function withRetry<T>(
  fn: () => Promise<T>,
  attempts = 3
): Promise<T> {
  let lastError: unknown;
  for (let i = 0; i < attempts; i += 1) {
    try {
      return await fn();
    } catch (err) {
      lastError = err;
      await new Promise((r) => setTimeout(r, 50 * (i + 1)));
    }
  }
  throw lastError;
}

export async function checkout(
  body: CheckoutBody
): Promise<{ orderId: string }> {
  return withRetry(async () => {
    const res = await fetch("http://orders.internal/orders", {
      method: "POST",
      headers: { "content-type": "application/json" },
      body: JSON.stringify(body),
    });
    if (!res.ok) {
      throw new Error(`checkout failed: ${res.status}`);
    }
    return res.json();
  });
}

The loop looks careful and the backoff looks adult. The POST still creates a new order on every success. A lost 201 still retries as a fresh write.

A safer shape to request

Ask the agent to send an idempotency key. Store the key with the order row. Reject a second insert for the same key.

Retry only the read of the stored result. Application checks alone lose a race. The database must enforce one row per key.

// unexecuted example — keyed write, then read on retry
import { randomUUID } from "node:crypto";

type CheckoutBody = {
  cartId: string;
  amountCents: number;
  idempotencyKey: string;
};

export async function checkout(
  body: CheckoutBody
): Promise<{ orderId: string }> {
  const key = body.idempotencyKey || randomUUID();
  const existing = await findOrderByKey(key);
  if (existing) {
    return { orderId: existing.id };
  }
  const created = await insertOrderOnce({
    cartId: body.cartId,
    amountCents: body.amountCents,
    idempotencyKey: key,
  });
  return { orderId: created.id };
}

Pair that handler with a unique constraint. Without the constraint the race still doubles charges. Reviewers should ask for the migration in the same PR.

-- unexecuted example — one business effect per key
ALTER TABLE orders
  ADD CONSTRAINT orders_idempotency_key_unique
  UNIQUE (idempotency_key);

Review workflow in numbered steps

Follow this sequence on every retry helper PR. Skip no step when money moves. Record the answers in the review comment.

List every HTTP method the helper wraps.
Mark each method as read or write.
Demand an idempotency key on every write.
Confirm a unique index exists for that key.
Search tests for a replay of the same body.
Fail the review if replay creates a second row.
Fail the review if payment capture runs twice.
Merge only after the replay test stays a no-op.

Use ripgrep before reading the story comments. Agents often hide retries one file away. The commands below keep that scan short.

git diff origin/main...HEAD --stat
rg -n "withRetry|retries|attempt" src test
rg -n "method: \"POST\"|method: 'POST'" src
rg -n "idempotency" src db

Reproducible test artifact

The next test is the actual review gate. Run it against a local fake insert path. The test should fail on the agent patch above.

// test/checkout.retry.test.js
const assert = require("node:assert/strict");
const { test } = require("node:test");

let inserts = 0;
const orders = new Map();

async function fakeInsert(body) {
  inserts += 1;
  const id = `ord_${inserts}`;
  const key = body.idempotencyKey || id;
  orders.set(key, { id, ...body });
  return { id };
}

async function checkoutWithRetry(body, attempts = 3) {
  let last;
  for (let i = 0; i < attempts; i += 1) {
    last = await fakeInsert(body);
  }
  return last;
}

test("replayed checkout must not insert twice", async () => {
  inserts = 0;
  orders.clear();
  const body = {
    cartId: "cart_1",
    amountCents: 1999,
    idempotencyKey: "idem_1",
  };
  await checkoutWithRetry(body, 3);
  assert.equal(inserts, 1);
  assert.equal(orders.size, 1);
});

Run the file with the built-in test runner. Keep the command in the review notes. A red log is the merge blocker, not a flake.

node --test test/checkout.retry.test.js

The assertion should fail until the insert path is unique. That red result is the review signal. A green suite without this case is not evidence.

Add a second case for missing keys. Agents often mint a new UUID on every retry. That habit defeats the unique index on paper.

test("retry must reuse the caller idempotency key", async () => {
  inserts = 0;
  orders.clear();
  const body = {
    cartId: "cart_2",
    amountCents: 5000,
    idempotencyKey: "idem_stable",
  };
  await checkoutWithRetry(body, 2);
  await checkoutWithRetry(body, 2);
  assert.equal(orders.size, 1);
  assert.equal([...orders.keys()][0], "idem_stable");
});

Decision table for the reviewer

Paste this table into the review comment. Agents respond better to a table than a lecture. Humans also merge faster with a binary action.

Signal in the diff	Trust	Action
Retry on GET of a stored order	Yes, timeout only	Keep with a cap
Retry on POST that inserts rows	No	Revert the helper
Comment says idempotent	No	Demand a unique index
Test hits the route once	No	Add a replay case
Unique key plus find-first	Conditional	Keep after a race test
Backoff without a key	No	Revert before merge
New UUID inside the retry loop	No	Require the client key

Free-server review loop

A small hosted loop keeps this check cheap. MonkeyCode is an open-source project with free model access and a free server option. Disclosure: This article was prepared as part of MonkeyCode's product outreach.

The operator also describes ten million free tokens for that model access. Use the server as a throwaway review box. Do not treat it as production hosting.

Do not paste live payment secrets into prompts. Do not load production dumps onto the free box. Keep fixtures synthetic and tiny.

Numbered setup for the box:

Clone the service onto the free server.
Add the replay test beside the agent patch.
Run node --test and save the failure log.
Send the diff plus the log to a free hosted model.
Ask only for revert lines and missing tests.
Re-run the suite after the model edits the patch.

A short prompt that stays in bounds looks like this. Keep the prompt boring and mechanical. The value is the failing test, not the chat.

Review this diff as a write-path change.
List HTTP methods that are not idempotent.
Name lines to revert.
Name one replay test that must fail today.
Do not add retries on POST.

git diff origin/main...HEAD > /tmp/pr.diff
node --test test/checkout.retry.test.js > /tmp/test.log 2>&1
wc -l /tmp/pr.diff /tmp/test.log

The model is a second pair of eyes on the table. Discard praise that is not backed by the replay test. Keep the unique constraint discussion in the same thread.

Limitations

This workflow does not prove end-to-end payment safety. It does not replace a ledger or a processor idempotency store. It does not model every timeout at the gateway.

The fake insert map is a local stand-in. It will miss database isolation bugs. It will miss webhook retries from the processor.

Teams still need a unique constraint and a dead-letter path. Free model output can still praise a bad retry. The test remains the source of truth.

Discard any suggestion that weakens the replay assertion. Discard any helper that retries capture itself. Discard any sleep added only to quiet CI.

Who should not use this approach

Do not use this loop for live card traffic. Do not use it as the only control in banking cores. Do not use it when the reviewer cannot read SQL constraints.

Skip the free server if the repo holds production secrets. Skip retries entirely when the write cannot be keyed. Skip agent refactors during an active incident.

Skip this article as a payment design guide. It is a review gate for agent diffs. Design still belongs with the ledger owner.

What good looks like after review

The merged handler accepts a client key. The table rejects a second row for that key. The retry, if any, only re-reads the stored order.

The suite replays the same body and still sees one insert. That is engineering on a noisy network. A retry without a key is only theater.

Treat the theater as a revert, not a polish pass. Reviewers can run the same gate on the next agent PR. The free server and free model access remain optional tools for that gate.

Review Agent PRs That Spread Request Bodies Into Writes

Jordan Li — Thu, 17 Sep 2026 05:28:23 +0000

The merge window opened at four on Friday.
A profile-update ticket sat on the sprint board.
An agent opened a compact TypeScript pull request.

The handler looked careful, typed, and complete.
Two unit tests asserted a successful 200 response.
A reviewer approved the patch within minutes.

Support saw the fallout after the weekend deploy.
One user profile now stored an admin role.
The public form never exposed that field.

This article reviews that class of agent patch.
It shows what to trust, revert, and test.
The working artifact is a Node review harness.

The pattern hiding in a clean diff

Agents often simplify update handlers under time pressure.
They spread the request body onto the stored record.
The TypeScript type still appears locally very strict.

Parsed JSON keeps extra enumerable keys at runtime.
Those extra keys reach the write path unchanged.
The editor type never sees them during review.

// Proposal: agent-generated handler. Do not copy into production.
export async function updateProfile(req, res, db) {
  const userId = req.session.userId;
  const current = await db.users.findById(userId);
  const next = { ...current, ...req.body };
  await db.users.save(next);
  return res.status(200).json({ ok: true, user: next });
}

The tests usually send only one displayName field.
They never send role, plan, or emailVerified.
Green CI does not prove a safe write.

What to trust in the pull request

Reviewers should trust small, named field copies only.
Trust a parser that strips unknown keys at the boundary.
Trust tests that assert rejected extra fields on disk.

Numbered signals that can remain in the patch:

An allowlist maps body keys to concrete columns.
A schema library strips unknown keys before writes.
The write uses explicit column parameters only.
Tests include at least one forbidden-field case.

Those four signals still need runtime proof in CI.
Do not trust comments that promise later hardening.
Do not trust a type that exists only in the editor.

What to revert on sight

Revert any spread of req.body into persistence.
Revert Object.assign of the payload onto stored records.
Revert queries built from a full client object.

Numbered revert list for the incoming diff:

Spread of req.body onto a loaded row.
Object.assign of req.body before the save.
ORM update calls that pass data: req.body.
Unfiltered loops over Object.keys during those writes.
Hidden fields such as role, tenantId, and plan.

Agents add these patterns while chasing fewer lines.
Fewer source lines are not a security property.
The human reviewer still owns the write surface.

What to test before merge

Tests must treat extra keys as hostile input.
Status 200 is not the assertion that matters.
The stored row is the assertion that matters.

Numbered test plan for the PR branch:

Send a valid displayName and one extra role.
Assert HTTP 200 or 400 per the public contract.
Reload the row from the database fixture.
Assert role remains the original stored value.
Repeat for tenantId, plan, and emailVerified.
Repeat with a nested object under a known key.
Repeat with a proto key on the payload.

The nested case catches shallow allowlists very quickly.
The prototype case catches unsafe object merges quickly.
Both show up in agent patches during refactors.

Review workflow in six steps

Follow this order on every agent update PR.

Search the diff for spread and Object.assign calls.
List every key the handler persists after merge.
Compare that list with the public API document.
Add a forbidden-field test before reading more code.
Run the test against the PR branch.
Keep the patch only if the row stays unchanged.

Command examples for a local checkout follow next.

git fetch origin pull/4821/head:pr-4821
git checkout pr-4821
rg -n "\.\.\.req\.body|Object\.assign\(|data: req\.body" src
grep -R -n -E "\.\.\.req\.body|Object\.assign\(|data: req\.body" src
node --test test/mass-assign.test.js

The ripgrep step is cheap and fully mechanical.
The grep fallback covers machines without ripgrep installed.
Neither command replaces the later database assertion.

Artifact: a reproducible harness

The harness below is a proposal for Node 20.
It does not require a networked database instance.
It uses an in-memory map as the store.

// lib/update-profile.js
const FORBIDDEN = new Set([
  "id",
  "role",
  "tenantId",
  "plan",
  "emailVerified",
  "priceCents",
]);

const ALLOWED = new Set(["displayName", "bio"]);

export function mergeTrusted(current, body) {
  const next = { ...current };
  for (const key of ALLOWED) {
    if (!Object.hasOwn(body, key)) continue;
    if (typeof body[key] !== "string") continue;
    next[key] = body[key];
  }
  return next;
}

export async function updateProfile(req, db) {
  const userId = req.session.userId;
  const current = await db.users.findById(userId);
  if (!current) return { status: 404, body: { ok: false } };

  for (const key of Object.keys(req.body || {})) {
    if (FORBIDDEN.has(key)) {
      return { status: 400, body: { ok: false, error: "unwritable_field" } };
    }
  }

  const next = mergeTrusted(current, req.body || {});
  await db.users.save(next);
  return { status: 200, body: { ok: true, user: publicUser(next) } };
}

export function publicUser(user) {
  return {
    id: user.id,
    displayName: user.displayName,
    bio: user.bio,
  };
}

// Teaching control only. Do not export from app entrypoints.
export async function updateProfileUnsafe(req, db) {
  const current = await db.users.findById(req.session.userId);
  const next = { ...current, ...req.body };
  await db.users.save(next);
  return { status: 200, body: { ok: true, user: next } };
}

// test/mass-assign.test.js
import test from "node:test";
import assert from "node:assert/strict";
import {
  updateProfile,
  updateProfileUnsafe,
} from "../lib/update-profile.js";

function memoryDb(seed) {
  const rows = new Map([[seed.id, { ...seed }]]);
  return {
    users: {
      async findById(id) {
        const row = rows.get(id);
        return row ? { ...row } : null;
      },
      async save(row) {
        rows.set(row.id, { ...row });
      },
    },
    async read(id) {
      return { ...rows.get(id) };
    },
  };
}

const seed = {
  id: "u1",
  displayName: "Ada",
  bio: "",
  role: "member",
  tenantId: "t-home",
  plan: "free",
  emailVerified: false,
};

test("unsafe spread promotes role from the client", async () => {
  const db = memoryDb(seed);
  const req = {
    session: { userId: "u1" },
    body: { displayName: "Ada Lovelace", role: "admin" },
  };
  await updateProfileUnsafe(req, db);
  const stored = await db.read("u1");
  assert.equal(stored.role, "admin");
});

test("safe handler rejects a role write", async () => {
  const db = memoryDb(seed);
  const req = {
    session: { userId: "u1" },
    body: { displayName: "Ada Lovelace", role: "admin" },
  };
  const result = await updateProfile(req, db);
  assert.equal(result.status, 400);
  const stored = await db.read("u1");
  assert.equal(stored.role, "member");
  assert.equal(stored.displayName, "Ada");
});

test("safe handler copies only allowlisted strings", async () => {
  const db = memoryDb(seed);
  const req = {
    session: { userId: "u1" },
    body: { displayName: "Ada Lovelace", bio: "math" },
  };
  const result = await updateProfile(req, db);
  assert.equal(result.status, 200);
  const stored = await db.read("u1");
  assert.equal(stored.displayName, "Ada Lovelace");
  assert.equal(stored.bio, "math");
  assert.equal(stored.plan, "free");
});

Run the drill with one local command only.

node --test test/mass-assign.test.js

The first test documents the failure mode clearly.
The second test is the actual merge gate.
The third test protects the intended feature path.

Label the unsafe export as a teaching control.
Do not ship updateProfileUnsafe into any production bundle.
Keep that export only inside the review drill.

Decision table for the reviewer

Diff signal	Trust	Revert	Required test
`{ ...row, ...req.body }`	No	Yes	Extra role must not persist
`Object.assign(row, body)`	No	Yes	Extra tenantId must not persist
`data: req.body` in an ORM update	No	Yes	Column dump before and after
Explicit allowlist plus unknown strip	Conditional	No	Allowed keys change; others 400
Comment says internal only	No	Yes	Call the route as a normal user
Type says UpdateProfileDto	No	Maybe	Runtime extra keys still required

Types lie when the boundary is HTTP JSON.
Comments also lie when agents generate them.
Database rows do not lie after the write.

Optional remote runner

Disclosure: This article was prepared as part of MonkeyCode's product outreach.
MonkeyCode provides free model access and a free server option.
Reviewers may run this harness there after a laptop check.
The steps stay valid if that product is ignored.

Limitations

This drill covers mass assignment on object writes.
It does not cover SQL injection or authz gaps.
It does not cover file uploads or webhook signatures.

The in-memory store hides real column-type coercion.
A real ORM may cast strings into booleans.
A real ORM may drop unknown keys by luck.

Luck is not a control for merge review.
Re-run the same tests against the staging schema.
Watch the actual UPDATE statement in query logs.

The forbidden list will rot without named owners.
New columns appear during unrelated refactors every quarter.
Agents then spread those new columns by default.

A 400 on unknown keys can break tolerant clients.
Some public patch APIs ignore extra fields instead.
That still requires a stored-row assertion after each write.

Shallow copies miss nested preference blobs.
Deep clones without allowlists copy too much.
Review nested objects as a second write surface.

Who should not use this approach

Do not use this checklist as a merge button.
Do not use it on services without a user model.
Do not use it as the only security review.

Skip the 400-on-forbidden policy for public patch APIs.
Some products ignore unknown keys on purpose by design.
That choice still needs the persist assertion in CI.

Skip any shared remote runner for regulated data.
The fixture above uses fake names only on purpose.
Never replay production dumps into a shared server.

The Friday patch failed a simple stored invariant.
Client JSON is not a trusted stored record.
Review the write, then review the stored row.

Review Agent PRs That Parse Untrusted JSON Without a Schema

Jordan Li — Wed, 16 Sep 2026 03:47:14 +0000

The on-call reviewer opened a quiet Monday pull request. An agent had added a payment webhook handler overnight. The diff looked tiny, and continuous integration was green.

A single new file parsed the request body as JSON. No schema file appeared anywhere in the patch. The catch block returned an empty object instead.

This pattern shows up constantly in agent-generated service code. The model wants a green typecheck and a short happy path. Untrusted bytes become trusted fields without a contract.

What this review is for

This article is a review playbook, not a framework tour. It covers agent pull requests that call JSON.parse on untrusted input. It also covers silent fallbacks that hide malformed payloads.

The reviewer learns what to trust, revert, and test. The method stays useful without any hosted coding product. A later section uses a free server only as a test runner.

The incident shape

The agent usually receives a ticket about inbound webhooks. It then invents a handler, a type, and a persistence call. The generated TypeScript often looks like the block below.

// proposed handler (agent PR) — example, not production code
export async function handleWebhook(req: Request, db: Db) {
  try {
    const payload = JSON.parse(String(req.body)) as WebhookEvent;
    await db.receipts.insert({
      id: payload.id,
      amount: payload.data.amount,
      email: payload.data.customer.email,
    });
    return { ok: true };
  } catch {
    return { ok: true, payload: {} };
  }
}

The cast tells the compiler the shape is already known. The empty catch tells operators that parse failure is success. Both lies will pass a shallow unit test without junk input.

Step 1. Isolate every parse hunk

Reviewers should not read the pull request as one story. They should list every JSON.parse and body-parser site. A unified diff is enough for this first pass.

git fetch origin pull/1842/head:pr-1842
git checkout pr-1842
git diff origin/main...HEAD -U5 -- '*.ts' '*.js'
git diff origin/main...HEAD -U0 | rg -n "JSON\.parse|req\.body|as [A-Z]"

Record each hit in a three-column review note. Column one is the file path and line. Column two is the trust boundary for those bytes.

Column three is the failure mode after a bad parse. Allowed labels are throw, empty object, null, or log.

Step 2. Classify trust before reading types

Agent comments often claim the payload is already validated. That sentence is not evidence of a runtime check. The source of the bytes is the only evidence.

Trust only these hunks during the first review pass.

Parse calls on fixture files written in the same patch.
Parse calls on constants defined inside the same patch.
Reuse of an existing, already reviewed schema helper.

Revert or block these hunks until poison tests exist.

HTTP bodies, query strings, and raw webhook bytes.
Queue messages and socket frames from other services.
Browser storage, cookies, and user-supplied CLI arguments.
Type assertions placed directly after JSON.parse calls.

The type assertion is not a runtime check at all. JSON.parse only proves the bytes were JSON text. It does not prove field names, types, or presence.

Step 3. Reject silent empty-object fallbacks

Empty objects are the agent's favorite repair trick. They keep CI green when a fixture is missing fields. They also drop payment events on the floor silently.

Use the decision table before arguing about style.

Failure behavior	Review action	Reason
Rethrow after logging parse error	Trust if logs omit secrets	Operators can see poison input
Return 400 with a stable error code	Trust for HTTP handlers	Callers can retry with a fix
Return `{}` or `[]` and continue	Revert	Downstream treats absence as valid
Empty `catch` with no log or metric	Revert	No signal and no test hook
Cast and proceed without a catch	Test, then likely revert	Invalid JSON still crashes later
Schema library with a 422 mapping	Trust after tests	Contract is explicit

The reviewer should restore a hard failure on malformed JSON. The agent may keep a typed error mapping for callers. It cannot keep a fake payload after a parse error.

// replacement contract — proposed example, not a production patch
import { WebhookEventSchema } from "./webhook-schema";

export async function handleWebhook(req: Request, db: Db) {
  const raw = String(req.body ?? "");
  let json: unknown;
  try {
    json = JSON.parse(raw);
  } catch (err) {
    throw new HttpError(400, "invalid_json", err);
  }
  const parsed = WebhookEventSchema.parse(json);
  await db.receipts.insert({
    id: parsed.id,
    amount: parsed.data.amount,
    email: parsed.data.customer.email,
  });
  return { ok: true };
}

Label this block as a proposed rewrite, not production fact. Teams should swap the schema import for their validator.

Step 4. Demand tests that send poison

Green CI on the happy fixture proves almost nothing here. The review should require four cases before any merge.

// tests/webhook-parse.test.ts — proposed review gate, not executed results
import { handleWebhook } from "../src/handle-webhook";

const db = { receipts: { insert: async () => undefined } };

test("rejects empty body", async () => {
  await expect(handleWebhook({ body: "" } as Request, db)).rejects.toMatchObject({
    status: 400,
    code: "invalid_json",
  });
});

test("rejects truncated object", async () => {
  await expect(handleWebhook({ body: '{"id":' } as Request, db)).rejects.toMatchObject({
    status: 400,
    code: "invalid_json",
  });
});

test("rejects missing amount", async () => {
  const body = JSON.stringify({
    id: "evt_1",
    data: { customer: { email: "a@b.c" } },
  });
  await expect(handleWebhook({ body } as Request, db)).rejects.toMatchObject({
    status: 400,
  });
});

test("inserts only after schema success", async () => {
  const insert = jest.fn(async () => undefined);
  const body = JSON.stringify({
    id: "evt_1",
    data: { amount: 1999, customer: { email: "a@b.c" } },
  });
  await handleWebhook({ body } as Request, { receipts: { insert } });
  expect(insert).toHaveBeenCalledWith({
    id: "evt_1",
    amount: 1999,
    email: "a@b.c",
  });
});

These tests are a proposed review gate, not executed results. They are unlabeled measurements and should be run locally.

Step 5. Scan the diff with a small harness

Human reviewers miss second parse sites in utility files. A short scanner keeps the classification list honest.

// tools/scan-json-parse.mjs — proposed local harness
import { execSync } from "node:child_process";

const diff = execSync("git diff origin/main...HEAD", { encoding: "utf8" });
const hits = [];
let file = "";
for (const line of diff.split("\n")) {
  if (line.startsWith("+++ b/")) file = line.slice(6);
  if (!line.startsWith("+") || line.startsWith("+++")) continue;
  const text = line.slice(1);
  if (/JSON\.parse\s*\(/.test(text)) {
    hits.push({ file, kind: "parse", text });
  }
  if (/catch\s*\([^)]*\)\s*\{\s*\}/.test(text)) {
    hits.push({ file, kind: "empty-catch", text });
  }
  if (/as any|as unknown as|as [A-Z][A-Za-z0-9_]+/.test(text) && /parse/.test(text)) {
    hits.push({ file, kind: "cast-after-parse", text });
  }
}
if (hits.length === 0) {
  console.log("no parse hunks in diff");
  process.exit(0);
}
for (const hit of hits) {
  console.log(`${hit.kind}\t${hit.file}\t${hit.text.trim()}`);
}
process.exit(2);

Run the scanner from the repository root after fetch.

node tools/scan-json-parse.mjs
echo $?

Exit code two means classification work remains unfinished. The script does not parse TypeScript grammar at all. It only flags added hunks that need a human label.

Suggested review comments

Paste short comments on the parse hunk, not the whole file. Three comments cover most agent webhook diffs of this shape.

Revert the empty catch. Malformed JSON must fail the handler.
JSON.parse plus a type assertion is not a schema.
Add tests for truncated JSON and a missing amount field.

Each comment is a command, not a discussion prompt. The authoring agent can act on commands without extra chat.

Where a free hosted runner fits

Some teams draft first review notes with a coding model. That step is optional for any team using this playbook. The scanner and poison tests already stand on their own.

MonkeyCode is an open-source coding assistant with free model access. Disclosure: This article was prepared as part of MonkeyCode's product outreach. It also offers a free server option for running checks.

The project documents a 10 million free-token allotment for model access. This article does not name models or remaining quota. It also does not report latency, hardware, or uptime claims.

A reviewer can paste classified hunks into a free model session. The prompt should ask for revert-or-test notes only. The model must not invent files missing from the diff.

Review only the added hunks below.
For each hunk, answer trust, revert, or test.
Forbid empty-object fallbacks on JSON.parse failures.
Do not invent files that are not in the diff.

The free server is a clean place to run the scanner. It can also run the four poison tests after clone. Keep the model away from production secrets and live keys.

The webhook fixture must use fake emails and fake ids. Curious readers can try that optional loop on the free server.

What to trust after the rewrite

Trust a schema parse that throws a mapped HTTP error. Trust tests that send truncated JSON and missing fields. Trust logs that record error class without secret headers.

Do not trust agent comments that simply say validated. Do not trust a WebhookEvent cast beside JSON.parse. Do not trust an ok flag returned from catch.

Limitations

The scanner is a regex pass over unified diffs. It will miss parse wrappers hidden behind local helpers. It will also flag JSON.parse inside generated snapshots.

The playbook assumes TypeScript or JavaScript HTTP services. It does not cover protobuf, Avro, or XML partner payloads. It does not measure model quality or token consumption.

The four tests do not prove idempotency or replay safety. Payment webhooks still need signature checks outside this article. Skipping signatures is a separate revert, not a style note.

Who should not use this approach

Do not apply the empty-object rule to closed generated fixtures. Do not run untrusted branches as root on a shared server. Do not paste live webhook payloads into a hosted model prompt.

Do not treat a model review comment as merge approval. Teams without JSON at the trust boundary can skip this. They still need a different review for SQL and files.

Close

Agent pull requests often hide contract work inside one parse. The contract belongs in a schema, a 400, and poison tests. The reviewer who classifies hunks first reverts less blindly.