DEV Community: Jordan Li

Signal Handlers in the Wrong Thread: Reviewing an Agent's Graceful Shutdown PR

Jordan Li — Tue, 27 Oct 2026 12:47:33 +0000

A pull request lands at 09:40 titled "add graceful shutdown". It touches three files and adds 212 lines. CI is green, and the agent's summary promises "no more dropped jobs".

The summary is the least reliable artifact in the patch. The diff is the evidence. This review sorts one shutdown patch into three piles: what to trust, what to revert, what to test.

Why shutdown patches attract confident agents

Graceful shutdown looks like a small feature. It is actually a concurrency contract. That contract covers signal handling, in-flight work, and exit codes.

Agents generate plausible shutdown code quickly. Signal registration, a drain loop, and a sleep show up in the right order. The real failure modes sit in details the summary never mentions.

Step 1 — Classify every hunk before running anything

The reviewer reads the diff first and the description second. Each hunk gets a bucket, and each bucket gets a different action.

Bucket	Typical hunks	Action
Trust	New unit tests, type hints, log strings	Merge after a read
Test	Signal registration, drain loop, exit codes, timeouts	Prove with a harness
Revert	Bare `except`, handler inside a thread, fixed `sleep`	Remove before merge

Most agent patches contain all three buckets. Treating the PR as one unit is how a swallowed exception reaches production.

Step 2 — Build a worker with observable state

The patch needs a target. A 30-line worker is enough to reproduce the contract. It uses only the standard library.

# worker.py — long-running job runner with cooperative cancellation
import signal
import sys
import time

STOP = False


def _handle(signum, frame):
    global STOP
    STOP = True


signal.signal(signal.SIGTERM, _handle)
signal.signal(signal.SIGINT, _handle)


def job(i: int, seconds: float = 1.0) -> str:
    deadline = time.monotonic() + seconds
    while time.monotonic() < deadline:
        if STOP:
            return f"job-{i}:aborted"
        time.sleep(0.01)
    return f"job-{i}:done"


for i in range(200):
    print(job(i), flush=True)
    if STOP:
        print(f"worker: stopped after {i}", flush=True)
        sys.exit(0)
print("worker: drained", flush=True)

The handler runs in the main thread and only flips a flag. That is the safe pattern. The flag becomes the cancellation point inside the loop.

Step 3 — Write the harness that proves the claim

The test spawns the worker, waits for real work, then sends SIGTERM mid-job. The assertion is about the exit path, not the log text.

# test_shutdown.py — proves the in-flight job resolves before exit
import signal
import subprocess
import sys
import time

proc = subprocess.Popen(
    [sys.executable, "worker.py"],
    stdout=subprocess.PIPE,
    text=True,
    bufsize=1,
)

first = proc.stdout.readline().strip()
assert first.endswith(":done"), first

time.sleep(0.3)                 # land inside the next job
proc.send_signal(signal.SIGTERM)

rest, _ = proc.communicate(timeout=10)   # TimeoutExpired means the bug is real
lines = [line for line in rest.splitlines() if line]

assert any(":aborted" in line or ":done" in line for line in lines), lines
assert proc.returncode == 0, proc.returncode
print("PASS: in-flight job resolved, exit code 0")

A patched worker that ignores SIGTERM hangs here. communicate(timeout=10) raises, and the review has its first hard fact.

Step 4 — Revert three hunks on sight

The patch contains three suspicious hunks. All three appear in real agent output, and all three are revertable without discussion.

# Hunk 4 — outcome swallowed
try:
    drain()
except Exception:
    pass          # revert: hides partial writes and exits 0 anyway

# Hunk 7 — handler registered from a worker thread
threading.Thread(
    target=lambda: signal.signal(signal.SIGTERM, _handle)
).start()
# revert: ValueError, signal registration is main-thread only

# Hunk 9 — constant exit latency
time.sleep(30)    # revert: replaces a drain bound with a fixed wait

The second hunk fails loudly and is therefore harmless in review. The first and third fail quietly, which is why they need explicit attention.

Step 5 — Ask for a bounded, idempotent drain

The fixed version tracks in-flight work and caps the wait. A second signal escalates instead of being ignored.

INFLIGHT: list[str] = []
STOP = False


def drain(deadline_s: float = 5.0) -> list[str]:
    end = time.monotonic() + deadline_s
    finished: list[str] = []
    while INFLIGHT and time.monotonic() < end:
        finished.append(INFLIGHT.pop())
        time.sleep(0.01)
    return finished


def _handle(signum, frame):
    global STOP
    if STOP:                     # second signal: stop cooperating
        raise SystemExit(1)
    STOP = True

Both properties are testable. Repeat SIGTERM twice in the harness and assert exit code 1. That test is short and worth keeping permanently.

Step 6 — Verify the test fails on the old code

A passing test proves nothing until it fails on the unpatched branch. This is the mutation check, and it takes one command sequence.

python test_shutdown.py          # on main: expect TimeoutExpired
git apply agent-shutdown.patch
python test_shutdown.py          # expect PASS, exit code 0
git checkout -b review/shutdown

If the test passes on main, it is not testing shutdown. The agent likely wrote an assertion about log text instead of exit behavior.

Step 7 — Use a hosted model as a second reader

Runtime behavior needs local execution. Static opinion does not. A hosted model reads the diff well and the process tree badly.

MonkeyCode offers free model access plus a free server option, per the project's supplied availability claims. The useful move is one narrow prompt, not a general review request.

Diff below. List every code path that can exit the process
without running the drain loop. Cite line numbers only.

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

The free server option covers the harness run when the local machine cannot hold a long-lived process. Availability terms and allowances change, so confirm the current terms on the project's site before planning around them.

What this workflow does not cover

Child processes. The harness checks one PID and says nothing about grandchildren.
SIGKILL. No handler runs after SIGKILL, and no patch changes that.
Orchestrator timeouts. Kubernetes preStop and termination grace periods need cluster-level tests.
Partial writes. Draining a queue and committing a transaction are different problems.

Who should skip this approach

Skip the harness if the service holds no in-flight state. Stateless request handlers have nothing to drain, and the test adds maintenance for free.

Skip the hosted second reader if the diff contains regulated code. The classification step, revert rules, and mutation check still apply without any model.

Closing note

A shutdown patch is reviewed hunk by hunk, not summary by summary. The three piles are the whole method: trust the boring parts, revert the quiet failures, test the contract.

When a free server run reproduces a hang in under a minute, paste that failing output beside the diff. Evidence beats a confident PR description every time.

The 401 Retry Bug: Auditing an Agent-Written Backoff Loop With a Fake Server

Jordan Li — Mon, 26 Oct 2026 11:27:16 +0000

At 9:04 a.m. a reviewer opens a pull request titled "add exponential backoff". The diff is 62 lines long. CI is green. The code reads like a textbook.

It also retries a 401 six times. Along the way it sleeps about 63 seconds.

That pattern is common in agent-generated PRs. The happy path is correct. The failure path is never examined by anyone.

This review walks through one such PR end to end. The agent session that produced it used MonkeyCode's free model access, and the harness below ran on its free server option. Both are operator-supplied availability claims, not benchmarks.

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

Step 1 — Read the diff for three retry smells

The submitted hunk is short. It hides three defects.

-        return _post(url, payload, api_key)
+        for attempt in range(6):
+            try:
+                return _post(url, payload, api_key)
+            except Exception:
+                time.sleep(2 ** attempt + random.random())
+        return None

The first smell is except Exception. It catches KeyError from a bug in the caller. It catches programming errors and network errors with the same hand.

The second smell is the missing status filter. A 400 and a 503 produce identical behavior. Authentication failures get retried, which is pure latency with no upside.

The third smell is return None. A silent None corrupts every downstream caller. Every consumer now needs a new branch for a failure that used to raise.

Two smaller issues sit beside those. Retry-After is ignored entirely. And random.random() adds a fraction of a second, which is jitter in name only.

Step 2 — Stand up a fake server that records timestamps

You cannot review timing by reading code. Build a server that writes down every request.

The harness below uses only the standard library. It needs no network access and no API key.

# fake_limiter.py — stdlib only
import json, threading, time
from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer

class Recorder:
    def __init__(self):
        self.hits = []
        self.lock = threading.Lock()

    def add(self, path, headers):
        with self.lock:
            self.hits.append({"path": path, "t": time.monotonic(), "headers": dict(headers)})

def make_server(recorder, script):
    """script: list of (status, extra_headers, body); the last entry repeats."""
    state = {"i": 0}

    class Handler(BaseHTTPRequestHandler):
        protocol_version = "HTTP/1.1"

        def do_POST(self):
            length = int(self.headers.get("Content-Length") or 0)
            self.rfile.read(length)
            recorder.add(self.path, self.headers)
            i = min(state["i"], len(script) - 1)
            state["i"] += 1
            status, extra, body = script[i]
            payload = json.dumps(body).encode()
            self.send_response(status)
            for k, v in extra.items():
                self.send_header(k, v)
            self.send_header("Content-Type", "application/json")
            self.send_header("Content-Length", str(len(payload)))
            self.end_headers()
            self.wfile.write(payload)

        def log_message(self, *args):
            pass

    srv = ThreadingHTTPServer(("127.0.0.1", 0), Handler)
    threading.Thread(target=srv.serve_forever, daemon=True).start()
    return srv, srv.server_address[1]

ThreadingHTTPServer gets a random free port. The recorder stores time.monotonic() per hit. That timestamp is the whole artifact.

Step 3 — Write five assertions against the recorded timeline

Each test fixes one script and then checks the request log. Timing tolerance is deliberately loose.

# test_backoff.py — python -m unittest -v test_backoff.py
import unittest
from fake_limiter import Recorder, make_server
import client_fixed as client  # point at client_pr to watch it fail

class BackoffReview(unittest.TestCase):
    def setUp(self):
        self.rec = Recorder()

    def _serve(self, script):
        srv, port = make_server(self.rec, script)
        self.addCleanup(srv.shutdown)
        return f"http://127.0.0.1:{port}/v1/chat"

    def test_no_retry_on_401(self):
        url = self._serve([(401, {}, {"error": "unauthorized"})])
        with self.assertRaises(client.ModelError):
            client.call_model(url, {"prompt": "hi"}, "bad-key")
        self.assertEqual(len(self.rec.hits), 1)

    def test_honours_retry_after(self):
        url = self._serve([(429, {"Retry-After": "1"}, {"error": "slow down"}),
                           (200, {}, {"output": "ok"})])
        out = client.call_model(url, {"prompt": "hi"}, "k")
        self.assertEqual(out["output"], "ok")
        gap = self.rec.hits[1]["t"] - self.rec.hits[0]["t"]
        self.assertGreaterEqual(gap, 0.95)

    def test_raises_when_attempts_exhausted(self):
        url = self._serve([(429, {"Retry-After": "1"}, {"error": "slow down"})])
        with self.assertRaises(client.ModelError):
            client.call_model(url, {"prompt": "hi"}, "k", max_attempts=3)
        self.assertEqual(len(self.rec.hits), 3)

    def test_idempotency_key_is_stable(self):
        url = self._serve([(429, {"Retry-After": "0"}, {}), (200, {}, {"output": "ok"})])
        client.call_model(url, {"prompt": "hi"}, "k")
        keys = {h["headers"].get("Idempotency-Key") for h in self.rec.hits}
        self.assertEqual(len(keys), 1)
        self.assertNotIn(None, keys)

    def test_stops_before_budget(self):
        url = self._serve([(503, {}, {"error": "unavailable"})])
        with self.assertRaises(client.ModelError):
            client.call_model(url, {"prompt": "hi"}, "k", max_attempts=10, budget_s=1.0)
        elapsed = self.rec.hits[-1]["t"] - self.rec.hits[0]["t"]
        self.assertLess(elapsed, 2.0)

The test that passed for the wrong reason

A first draft asserted len(self.rec.hits) > 1. That proves a retry happened. It says nothing about whether the delay obeyed the server.

The draft also asserted total wall-clock time under five seconds. It passed, because the second response was a 200. The underlying defect only appears when every attempt fails.

Assert on the interval between recorded requests. Assert on the terminal state. Never assert on a happy path you already know works.

Step 4 — Patch the loop and keep what was right

The agent was right about one thing. Backoff belongs in the client, not in every caller. Keep the retry wrapper and replace its body.

# client_fixed.py
import json, random, time, urllib.error, urllib.request

RETRY_STATUS = {429, 500, 502, 503, 504}

class ModelError(RuntimeError):
    pass

def call_model(url, payload, api_key, *, max_attempts=4, budget_s=20.0, idem_key=None):
    idem_key = idem_key or f"{int(time.time() * 1000)}-{random.randrange(1 << 20):05x}"
    headers = {
        "Content-Type": "application/json",
        "Authorization": f"Bearer {api_key}",
        "Idempotency-Key": idem_key,
    }
    body = json.dumps(payload).encode()
    start, last = time.monotonic(), None

    for attempt in range(1, max_attempts + 1):
        try:
            req = urllib.request.Request(url, data=body, headers=headers, method="POST")
            with urllib.request.urlopen(req, timeout=30) as r:
                return json.loads(r.read())
        except urllib.error.HTTPError as e:
            last = f"HTTP {e.code}"
            if e.code not in RETRY_STATUS:
                raise ModelError(last) from e   # 400/401/403: fail fast
            wait = _retry_after(e.headers.get("Retry-After"))
        except (urllib.error.URLError, TimeoutError) as e:
            last = f"transport: {e}"
            wait = None

        if attempt == max_attempts:
            break
        if wait is not None:
            delay = wait
        else:
            delay = min(8.0, 0.5 * 2 ** (attempt - 1)) * random.uniform(0.5, 1.0)
        if time.monotonic() - start + delay > budget_s:
            break
        time.sleep(delay)

    raise ModelError(f"gave up after {attempt} attempts: {last}")

def _retry_after(raw):
    if not raw:
        return None
    try:
        return max(0.0, float(raw))
    except ValueError:
        return None  # HTTP-date form: fall back to computed backoff

Three properties matter here. Non-retryable codes raise immediately. Retry-After wins over computed backoff. The elapsed budget caps total sleep, so a 429 storm cannot hang a worker.

The idempotency key is generated once and reused across attempts. A retried POST then stays one logical request.

Step 5 — Record the review decision

The table below is the actual output of the review. It is short enough to paste into the PR description.

Area	Decision	Evidence
Retry wrapper placement	Trust	Callers keep one code path
Constant `2 ** attempt` sleep	Revert	Cap and budget are missing
`except Exception`	Revert	Hides `KeyError` from the caller
`return None`	Revert	Silent failures reach production
`Retry-After` handling	Test	`test_honours_retry_after`
4xx handling	Test	`test_no_retry_on_401`
Key stability	Test	`test_idempotency_key_is_stable`
Total wait ceiling	Test	`test_stops_before_budget`

Trust covers pure structure. Revert covers anything that changes failure semantics. Test covers behavior that only appears over time.

That split keeps review moving. You do not rewrite the PR. You revert three hunks and prove the rest.

Where this harness lies to you

The fake server is one process on loopback. It never models TLS handshakes, connection reuse, or proxy-generated 502s. A passing suite says nothing about those.

Timing assertions are the fragile part. A loaded CI runner can add tens of milliseconds. Keep thresholds loose, or mark timing tests as serial.

The harness also ignores fairness. It cannot tell you whether one tenant starves another under concurrency limits.

Finally, free-tier behavior can change without notice. Treat the environment as convenient, not contractual.

Who should skip this approach

Teams behind a paid tier with documented retry semantics should test against that contract instead. Their provider publishes limits and status codes worth encoding directly.

Teams that need per-tenant rate limiting need a shared limiter, not a per-client sleep loop. The harness here will not surface that class of bug.

Anyone without a way to run the client against a local endpoint should stop at Step 1. The three smells are visible by reading, and they cost nothing to flag.

What carries over

Agent-generated retry code fails in predictable places. It over-catches. It ignores server hints. It converts errors into None.

A recorded request timeline catches all three in under an hour. The server is 40 lines. The tests are five assertions.

The rest of the PR is usually fine, and saying so is part of the review. If you want to reproduce this workflow, MonkeyCode's free model access and free server option are enough to generate a candidate PR and run the harness against it. Point the import at your own client first.

A Model Fallback PR: What to Trust, What to Revert, What to Test

Jordan Li — Tue, 20 Oct 2026 09:31:35 +0000

A fallback PR lands at 09:12 on a Monday. CI is green and the description reads "adds model fallback for reliability." The diff spans 340 lines across four files.

The reviewer has one hour before the release branch closes. This is a scoping problem, not a coding problem.

The walkthrough below uses a constructed diff, written for teaching. It is not a real submission, and it carries no production metrics. The goal is a repeatable triage order: trust the boring parts, revert the dangerous parts, test whatever survives.

1. Slice the diff before reading it

Wide diffs hide risk in the parts nobody opens. Three commands shrink the review surface to something reviewable.

git fetch origin pull/412/head:pr-412
git diff --stat main...pr-412
git diff main...pr-412 -- 'services/*' 'tools/*' | grep -nE 'except|retry|sleep|charge|budget|fallback'

The first command isolates the branch. The second shows which files actually changed size. The third surfaces control-flow keywords, which is where fallback logic usually breaks.

A 340-line diff often reduces to 60 lines of real behavior. Review those 60 lines twice and skim the rest once.

2. The proposed fallback loop

The core hunk looks reasonable at a glance.

# tools/model_client.py (excerpt from the constructed diff)
import time

def complete(prompt, tools, models, budget):
    for model in models:
        for attempt in range(3):
            try:
                resp = model.call(prompt, tools=tools)
                budget.charge(resp.usage)      # charged after the call
                return resp
            except Exception:                  # too broad
                time.sleep(2 ** attempt)       # retry storm
    return None                                # silent failure

Four defects sit in ten lines. The bare except catches BudgetExceeded and keeps spending. The retry loop re-sends tool calls that may not be idempotent. The budget check runs after the spend. The None return erases which models failed.

3. What to trust

Some parts of an agent PR rarely need line-by-line scrutiny. Trust them provisionally, then move on.

Type signatures with no default values.
Logging fields that match existing schema names.
Config additions that mirror an existing block.
Test scaffolding that reuses the project's fixtures.
Renames with no behavior change, verified by git diff -M.

Trust here means "do not spend review minutes." It does not mean "approve unreviewed." Each item still needs one confirming check, usually a grep against the old call sites.

grep -rn "complete(" --include="*.py" services/ tools/ | grep -v tests/

If every call site already passes models= and budget=, the signature change is mechanical. If one call site still passes three arguments, the PR is incomplete.

4. What to revert before discussion

Revert first and debate later when a hunk violates a safety invariant. Five patterns qualify.

Pattern in diff	Why revert	Cheap replacement
`except Exception` around spend paths	Swallows budget and auth errors	Catch `TimeoutError` and `ConnectionError` only
Retry on non-idempotent tool calls	Duplicate side effects	Retry read-only tools, or pass an idempotency key
Post-call budget check	Overspend before refusal	Pre-flight estimate, then charge actual
`return None` on exhaustion	Hides failure cause	Raise `AllModelsFailed(errors=[...])`
`time.sleep(2 ** attempt)` unbounded	Retry storms under outage	Cap total wall time, not attempt count

Revert is a review action, not a rejection. The author keeps the structure and fixes three functions.

5. What to test with a fake client

The PR claims "improves reliability." That claim is testable without any network call. A fake model client makes the fallback path deterministic.

# tests/test_fallback.py
import pytest
from tools.model_client import complete, AllModelsFailed

class FakeModel:
    def __init__(self, name, failures=0, usage=10):
        self.name, self.failures, self.usage, self.calls = name, failures, usage, 0

    def call(self, prompt, tools=None):
        self.calls += 1
        if self.calls <= self.failures:
            raise TimeoutError(f"{self.name} timed out")
        return type("Resp", (), {"usage": self.usage, "model": self.name})()

def test_no_fallback_after_side_effecting_tool_call():
    primary, secondary = FakeModel("primary", failures=1), FakeModel("secondary")
    with pytest.raises(TimeoutError):
        complete("charge", tools=[charge_card], models=[primary, secondary], budget=None)
    assert secondary.calls == 0

Run the suite with pytest -q tests/test_fallback.py. Three more cases belong beside it. Budget exhaustion must not retry. Exhaustion must raise a typed error. The winning model name must appear in the response.

Write the failing test before requesting changes. A red test is a stronger review comment than a paragraph of prose.

6. Replaying the suite on free infrastructure

Cost usually blocks reviewers from rerunning a model-touching suite. MonkeyCode covers that gap with two operator-supplied options: free model access and a free server option. The operator states that access includes 10 million free tokens, and that the server option does not require a paid plan.

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

The relevant use here is narrow. The fake-client tests above need no model at all. The replay pass, which sends recorded prompts through real models, is what consumes tokens. Running that replay on the free tier keeps a review harness affordable for one reviewer.

Treat every quota claim as unverified until read from the vendor's own terms. This article makes no claim about model names, throughput, hardware, or duration. Those details change, and stale numbers make reviews worse.

A practical split: fake-client tests in CI, replay pass by hand before approval. Keep replay output as an artifact attached to the PR, not as a comment thread.

7. Decide with a written rule

Reviews drift when the standard lives in someone's head. Write the rule down once.

Revert any hunk that retries a non-idempotent call.
Revert any spend path that catches broad exceptions.
Test any claim containing "reliability", "fallback", or "retry".
Trust mechanical changes only after a call-site grep.
Approve only when the failing tests from step 5 pass.

8. Limitations and who should skip this

This workflow fits PRs that touch model routing, retries, or budgets. It fits teams that can run a fake client in CI.

It does not fit three cases. Skip it when the PR is purely cosmetic. Skip it when the team has no budget abstraction to test against. Skip it when reviewers cannot attach artifacts to the PR.

Also skip the free-tier replay for anything sensitive. Recorded prompts may contain customer text. Scrub them first, or keep the entire replay local.

The triage order matters more than the tooling. Slice the diff, revert the unsafe hunks, then test the remaining claims.

Readers who want to rerun this harness can start from the project's free tier, attach their own replay artifacts, and compare notes on what their last fallback PR actually changed.

Reviewing an Agent PR That Adds a Token Budget Ledger

Jordan Li — Mon, 19 Oct 2026 08:15:15 +0000

A reviewer opens a pull request at 09:12. The title reads feat: add token budget ledger. The diff is 180 lines and the CI badge is green.

That is the moment to slow down. A ledger is accounting code. Everyone downstream trusts it, and almost nobody verifies it.

The PR in this review came from an agent session run on MonkeyCode. The project advertises free model access and a free server option.

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

The workflow below stays useful with every product mention removed. It is a review pattern, not a vendor tour.

1. Write down what the diff claims

Three claims usually hide inside a ledger PR. Each claim needs its own test.

The ledger counts real usage, not an estimate.
The ledger blocks calls that would exceed the budget.
The ledger survives retries, cancels, and dropped connections.

Claim one is a data question. Claim two is a policy question. Claim three is a state question.

Most agent PRs get claim two right. Claims one and three fail quietly in production.

2. Split the diff into trust, revert, and test

Use a fixed triage table before reading line by line. The table stops reviewers from grading style while missing arithmetic.

Diff area	Decision	Reason
`usage` parsed from the response body	Trust, then test	Server usage is the only authoritative number
Local tokenizer estimate used for enforcement	Revert	Two tokenizers, two answers, one budget
In-memory `spent` counter	Revert	Restarts and replicas reset it
Budget check before dispatch	Test	Off-by-one on `>` versus `>=`
Retry wrapper	Test	Retries are where double charging appears
Logging and metrics fields	Trust	Noise, not correctness

Revert first, argue later. A reverted estimate is cheaper than a wrong invoice.

3. Make the model server deterministic

Reconciliation needs fixed numbers. Point the client at a local server that always returns the same usage object.

# fake_model.py - no network egress, fixed usage, two modes
import json, sys
from http.server import BaseHTTPRequestHandler, HTTPServer

USAGE = {"prompt_tokens": 812, "completion_tokens": 96}
MODE = sys.argv[1] if len(sys.argv) > 1 else "normal"

class Handler(BaseHTTPRequestHandler):
    def do_POST(self):
        self.rfile.read(int(self.headers.get("content-length", 0)))
        payload = {"id": "resp_fixed_1", "choices": [{"message": {"content": "ok"}}]}
        if MODE != "no_usage":
            payload["usage"] = USAGE
        body = json.dumps(payload).encode()
        self.send_response(200)
        self.send_header("content-type", "application/json")
        self.send_header("content-length", str(len(body)))
        self.end_headers()
        self.wfile.write(body)

    def log_message(self, *args):
        pass

if __name__ == "__main__":
    HTTPServer(("127.0.0.1", 8099), Handler).serve_forever()

Start it with python fake_model.py. Start it again with python fake_model.py no_usage for the fail-closed test.

Two modes are enough. More modes mean the harness is testing itself.

4. Reconcile the ledger against server usage

This test catches most of the damage. The ledger must equal the sum of server usage, exactly.

# test_ledger_reconcile.py
import pytest
from ledger import Ledger, call_model, BudgetExceeded, TransientError, MissingUsage

URL = "http://127.0.0.1:8099"

def test_single_call_matches_server_usage():
    ledger = Ledger(budget=10_000)
    resp = call_model(URL, "hello", ledger=ledger)
    assert resp["usage"]["prompt_tokens"] == 812
    assert ledger.spent == 812 + 96
    assert ledger.remaining == 10_000 - 908

Run the same assertion against the reverted estimate. If the estimate agrees by luck, the test still passes. That luck is not a design.

Add a boundary test next. The call that lands exactly on the limit must be allowed once.

def test_exact_budget_boundary():
    ledger = Ledger(budget=908)
    call_model(URL, "hello", ledger=ledger)
    assert ledger.remaining == 0
    with pytest.raises(BudgetExceeded):
        call_model(URL, "hello", ledger=ledger)

5. Test retries as a charging problem

A retry wrapper turns one logical request into several HTTP calls. The ledger must charge once per logical request, not once per attempt.

def test_retry_charges_once_per_request_id():
    ledger = Ledger(budget=10_000)
    call_model(URL, "hello", ledger=ledger, request_id="r-1")
    with pytest.raises(TransientError):
        call_model(URL, "hello", ledger=ledger, request_id="r-1")  # injected drop
    assert ledger.spent == 908

If this fails, revert the counter and keep the request id. Idempotency keys belong in the ledger key space, not in a comment.

Cancelled streams need the same treatment. A cancelled stream still bills for tokens already emitted.

6. Fail closed when usage is missing

Restart the fake server in no_usage mode. The ledger must refuse to guess.

def test_missing_usage_fails_closed():
    ledger = Ledger(budget=10_000)
    with pytest.raises(MissingUsage):
        call_model(URL, "hello", ledger=ledger)
    assert ledger.spent == 0          # nothing charged, nothing invented
    assert ledger.state == "blocked"

Charging zero and continuing is the most common bug in this class. It turns a budget into a suggestion.

7. Read the failure table before approving

Symptom	Root cause	Review action
Spend drifts upward on long runs	Estimate used for enforcement	Revert to server usage
Budget resets after deploy	In-memory counter	Move state out, or document the reset
Double charge on timeout	Retry without request id	Revert retry, add idempotency key
Free calls after an upstream change	Missing `usage` treated as zero	Fail closed
One replica overspends	Per-process counters	Centralise or shard the budget

Two rows are state bugs. One is a data bug. All three reach production when CI only checks the happy path.

Limitations and who should skip this

The harness proves arithmetic and idempotency. It proves nothing about upstream pricing, provider quotas, or clock skew between replicas.

Skip this workflow for batch jobs that reserve a fixed budget up front. Skip it when nobody enforces a budget at all. A ledger nobody reads is dead code with tests attached.

Skip it too if the team cannot run a six-line local server. That constraint deserves fixing first.

Closing

A green badge on an agent PR means the agent's own tests passed. It does not mean the numbers are right.

Revert the estimate, keep the server usage, and pin the boundary test. Then the ledger earns its place in the diff.

Teams that want to rehearse this harness without wiring a paid provider can start on MonkeyCode's free model access and free server option. Verify the current terms before relying on them for anything long-lived.

What to Trust, Revert, and Test in an Agent-Generated Pull Request

Jordan Li — Sun, 18 Oct 2026 06:58:09 +0000

Suppose an agent opens a pull request that adds a summarization endpoint. CI is green. The diff also edits the shared config loader and one auth middleware file.

That is the normal case, not the dramatic one. Agent-written PRs rarely fail review on syntax. They fail on scope.

This article gives a three-bucket triage and a reproducible audit script. The goal is a review verdict a second engineer can re-run.

Green CI proves compilation, not intent

A passing pipeline answers one question. Does the code build and satisfy existing tests?

It does not answer whether the change belongs in this PR. It does not answer whether a shared default drifted. Treat the badge as a floor, never as evidence.

An agent optimizes for the stated task. Anything adjacent to the task looks like fair game to it. Config loaders, CI workflows, and middleware sit adjacent to almost every feature.

Step 1 — Sort the diff into three buckets

Before reading logic, classify every hunk. The bucket decides how much attention the hunk earns.

Bucket	Typical contents	Reviewer action
Trust	Renames, formatting, moved imports, generated lockfile churn	Skim, confirm tests still pass
Revert	Shared defaults, auth/middleware, CI workflows, migrations, dependency bumps, global exception handlers	Remove from the PR, open a separate one
Test	New endpoint logic, input validation, error mapping, outbound calls	Require a contract test and a failure-injection test

The asymmetry matters. A wrong trust hunk costs a revert. A wrong revert hunk costs an incident.

Step 2 — Measure blast radius with a script

The first artifact is a scope audit. It reads the diff, not the description.

#!/usr/bin/env python3
"""diff_scope.py - flag PR changes that fall outside the stated scope."""
import fnmatch
import subprocess
import sys

SENSITIVE = [
    "config/*", "**/settings.py", "**/defaults.py",
    "**/auth/*", "**/middleware/*",
    ".github/workflows/*",
    "migrations/*", "**/migrations/*",
    "requirements*.txt", "poetry.lock", "package-lock.json",
]

def changed_files(base):
    out = subprocess.run(
        ["git", "diff", "--name-only", f"{base}...HEAD"],
        capture_output=True, text=True, check=True,
    ).stdout
    return [line for line in out.splitlines() if line.strip()]

def classify(files):
    flagged = []
    for path in files:
        for pattern in SENSITIVE:
            if fnmatch.fnmatch(path, pattern):
                flagged.append((path, pattern))
                break
    return flagged

if __name__ == "__main__":
    base = sys.argv[1] if len(sys.argv) > 1 else "origin/main"
    files = changed_files(base)
    print(f"files changed: {len(files)}")
    for path, pattern in classify(files):
        print(f"SENSITIVE  {path}  (matched {pattern})")

Run it against the PR branch:

python diff_scope.py origin/main

One honest caveat: fnmatch treats * as greedy across slashes. The list over-matches on purpose. A review aid should produce false positives, not silent misses.

Step 3 — Grep for the three hunk shapes that hide bugs

Path filters miss logic edits inside in-scope files. Three shapes deserve a targeted grep.

git diff -U0 origin/main...HEAD -- '*.py' \
  | grep -nE '^\+.*(except Exception|except:|= None|os\.environ|default=)'

Read every hit. Swallowed exceptions turn a 500 into a 200 with an empty body. Changed defaults move behavior for every caller, not just the new one. A new os.environ read adds a deployment requirement the PR description probably omits.

Step 4 — Revert scope creep before reading logic

Revert first, review second. Reading logic inside a contaminated diff wastes attention.

Use file-level restore for files the PR never should have touched:

git checkout origin/main -- app/config/defaults.py .github/workflows/ci.yml
git commit -m "revert: out-of-scope changes, split into follow-up PRs"

Then re-run CI on the reduced diff. If the feature still passes, the reverted files were genuinely unrelated. If it fails, the agent had a hidden dependency you now know about.

That failure is the useful outcome. It converts an invisible coupling into a reviewable decision.

Step 5 — Test the contract, then the failure path

The test bucket needs two tests minimum. One for the happy contract, one for the failure the agent usually forgets.

# tests/test_summarize_contract.py - sketch; adapt imports to your app.
from fastapi.testclient import TestClient
from app.main import app

client = TestClient(app)

def test_empty_text_is_rejected():
    r = client.post("/summarize", json={"text": ""})
    assert r.status_code == 422

def test_upstream_timeout_is_not_a_200(monkeypatch):
    def boom(_text):
        raise TimeoutError("model timeout")
    monkeypatch.setattr("app.summarize.call_model", boom)
    r = client.post("/summarize", json={"text": "hello"})
    assert r.status_code >= 500
    assert "timeout" not in r.text.lower()  # no internal detail leaked

The second assertion catches a specific failure. An agent that adds a global error handler often leaks exception text to the client. That is a quiet information disclosure, and it ships green.

Run only the new tests first:

pytest tests/test_summarize_contract.py -q

If they pass against the unreverted diff, re-run after the revert. Order matters.

Step 6 — Record the verdict in the PR

A verdict that lives in a chat thread is not reviewable. Write it where the next reader looks.

## Review verdict
- Files outside stated scope: app/config/defaults.py, .github/workflows/ci.yml
- Reverted: both, moving to follow-up PRs
- Kept: app/summarize.py, tests/test_summarize_contract.py
- Tests added: empty-input 422, upstream-timeout 5xx, no detail leak
- Not verified: live model latency, per-request cost, load behavior

The final line is the most important. Review states what was not checked. Silence reads as coverage.

Where free model access fits

A first pass over a large diff is mechanical work. Ask a model to list suspicious hunks, then verify each one by hand.

MonkeyCode offers free model access and a free server option, per the operator. That combination suits this narrow job: a stateless prompt that returns a candidate list. The reviewer keeps every decision, and the script remains the source of truth.

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

Keep the prompt small. Paste the diff, the stated task, and the sensitive-path list. Ask for hunks that match the list, with file and line. Ignore any answer you cannot confirm in git diff.

Limitations and who should skip this

This workflow is file-path driven. It cannot see a subtle logic change inside an in-scope file, and it never replaces running the suite. A single-file agent PR will pass the scope audit and can still be wrong.

The audit also assumes a stable origin/main. Rebase-heavy branches make ...HEAD comparisons noisy.

Skip this approach in three cases. Teams that require pinned, audited model versions should not paste diffs into any hosted endpoint. Teams with strict data-residency rules should keep the diff local. Teams under a hard deadline should revert aggressively rather than attempt a full model-assisted pass.

Free-tier availability and quotas can change without notice. Confirm current limits before depending on them in a pipeline.

The takeaway

Trust the mechanical, revert the adjacent, test the failure. That ordering survives model upgrades and tool churn.

The script is thirty lines. The verdict template is five. Both are cheaper than one incident caused by a config default nobody read.

Readers who want to try the model-assisted first pass can start with the free model access and free server option, then verify every flag against the raw diff.

A Cache Key Bug in an Agent PR: Cross-Tenant Leakage, Tested

Jordan Li — Fri, 16 Oct 2026 04:59:45 +0000

The pull request arrived at 02:14 with a green checkmark and three files changed. Its claim was modest: skip duplicate model calls, cut latency on repeated prompts. The reviewer's first instinct was to trust the test suite.

The tests only covered the happy path. That is exactly where a caching change is least likely to break.

Note: the diff below is a minimal reconstruction for teaching, not a verbatim vendor pull request.

The Diff Under Review

import hashlib

CACHE = {}

def complete(prompt, tenant_id, model="default"):
    key = hashlib.sha256(prompt.encode()).hexdigest()
    if key in CACHE:
        return CACHE[key]
    resp = call_model(model, prompt)   # network call, can raise
    CACHE[key] = resp
    return resp

Three things stand out on the first read. The tenant_id argument is accepted and never used. Nothing checks whether the response is usable before storing it. The dictionary has no size bound, no TTL, and no eviction path.

The diff passes review because it is small and readable. Small and readable is not the same as correct.

Why the Happy Path Passes

The accompanying test calls complete twice with one prompt and one tenant. It asserts that the model client was invoked once.

That assertion is true. It also stays true after every defect below is fixed, which is why the suite gives reviewers no signal at all.

Three Defects the Existing Tests Never Touch

1. The Cache Key Ignores Tenant Identity

The key is a hash of the prompt text alone. Two tenants that send an identical string share one cache entry.

For a prompt like "Summarize the incident report." the collision is not theoretical. Support tooling, templates, and system prompts repeat across accounts all day long.

2. Blank and Failed Results Get Cached

If call_model returns None or an empty string on a transient upstream failure, that value becomes the stored answer. Every later caller reads the failure until the process restarts.

Negative caching converts a one-second blip into a multi-hour outage. The diff never distinguishes a good answer from a bad one.

3. The Cache Is Process-Local and Unbounded

Four workers mean four separate dictionaries and four independent hit rates. Memory grows with the number of unique prompts and never shrinks.

The diff looks like a shared cache. It behaves like a per-worker notebook with no eraser.

What to Trust, What to Revert, What to Test

Trust the pure helper. hashlib.sha256 is a reasonable key primitive.
Keep the tenant_id parameter and thread it into the key.
Revert the module-level mutable dictionary.
Revert any write of an empty, failed, or partial response.
Replace the single happy-path test with isolation, failure, and concurrency tests.

A Reproduction Plan You Can Run in Ten Minutes

Step 1: Write the isolation test first

import pytest

calls = []

def fake_call_model(model, prompt, tenant_id):
    calls.append(tenant_id)
    return f"{tenant_id}-answer"

def test_cache_does_not_leak_between_tenants():
    calls.clear()
    a = complete("Summarize the incident report.", tenant_id="acme")
    b = complete("Summarize the incident report.", tenant_id="globex")
    assert calls == ["acme", "globex"]
    assert a != b

Run pytest -k tenant -x. It fails against the diff. That failure is the evidence, not an opinion.

Step 2: Patch the key and rerun

def cache_key(tenant_id, model, prompt):
    parts = [tenant_id, model, prompt]
    return hashlib.sha256("\x00".join(parts).encode()).hexdigest()

The null-byte separator prevents ambiguous concatenation. The same test should now pass unchanged.

Step 3: Prove failures are not cached

def test_failures_are_not_cached(monkeypatch):
    calls.clear()
    def boom(*args, **kwargs):
        calls.append("attempt")
        raise TimeoutError("upstream")
    monkeypatch.setattr("app.model.call_model", boom)
    for _ in range(2):
        with pytest.raises(TimeoutError):
            complete("boom", tenant_id="acme")
    assert len(calls) == 2

Two attempts, two upstream calls. A cached exception would produce one.

Step 4: Measure the cross-worker hit rate

for i in $(seq 1 40); do
  curl -s -X POST localhost:8000/complete \
    -H 'content-type: application/json' \
    -d '{"prompt":"ping","tenant_id":"acme"}' > /dev/null
done
grep -c 'model_call' /var/log/app.log

Forty requests with one unique prompt should log far fewer than forty model calls. If the count stays near forty, the cache is not shared across workers.

Decision Table

Change	Verdict	Reason
`cache_key` helper	Keep	small, pure, easy to test
`tenant_id` in the key	Keep and require	blocks cross-tenant reads
module-level `CACHE` dict	Revert	process-local, unbounded, no TTL
caching of empty or failed results	Revert	turns a blip into a lasting outage
single happy-path test	Replace	never exercises isolation or failure

Reusing This Review Loop on a Free Server

The agent that produced the diff ran against MonkeyCode's free model access. The repro suite above ran on the project's free server option, which is enough for a few hundred test calls in one sitting.

The operator states the free tier also includes a promotional token allowance, currently advertised at 10 million tokens. Confirm the current terms in the repository before planning around any number. Disclosure: This article was prepared as part of MonkeyCode's product outreach.

The model is not the interesting part here. The loop is: generate, reproduce, revert, retest.

Limitations and Who Should Skip This

Teams with strict data residency rules should not send prompts to any hosted endpoint, free or paid.
Multi-tenant products need cache keys derived from tenant, model, and prompt version together.
A process-local dictionary is acceptable for a single-worker prototype and wrong in every other deployment.
If hit rate matters at scale, use a shared store with TTL, eviction, and a size cap.
This review does not attempt to validate model output quality. It only checks whether one tenant can read another tenant's answer.

Closing Note

An agent PR is a draft, not a decision. Trust the pure functions, revert the shared mutable state, and test the tenant boundary before anything else.

Review an Agent PR That Fetches Model-Named Docs URLs

Jordan Li — Tue, 13 Oct 2026 00:21:49 +0000

A maintainer found pull request 218 on a quiet Tuesday. An agent had added a docs fetch to a small coding tool. The summary promised richer answers from live documentation pages.

This write-up is a constructed review case, not a named outage. The file names are concrete so a team can replay the pass. This case claims no production metric and no customer incident.

The change on the branch

The diff touched three files in the agent runtime. One file pulled a URL out of the model text. Another file downloaded the page and built the next prompt.

A third file wrote that prompt beside the source tree. The agent described the feature as a context helper. The helper read a URL from the model reply.

It then inserted the page body into the following prompt. A narrow docs workflow can justify a bounded helper. This patch left the network path wide open.

What the reviewer can trust

Trust belongs to pure functions and to pinned fixtures. A delimiter wrapper can stay when it never opens a socket. A size constant can stay when a test locks the number.

A dry-run flag can stay when it only prints the plan. That flag must default to closed network access. A green summary written by the agent is not evidence.

function canTrust(change) {
  return change.pure === true
    && change.network === false
    && change.testsPinLimits === true;
}

The sketch above is a review aid, not a measured result. It does not approve any changed file by itself. A human reviewer still has to read the full diff.

What to revert before merge

Revert the open fetch of any model-named URL. A name inside a model reply is not an allowlist. A confident summary is not a host policy either.

Revert redirect following on that HTTP client immediately. A redirect can move the request after the first host check. The revert stands without a catalog of bypass tricks.

Revert every packed-prompt write inside the repository tree. Packed text may contain tool output or local secrets. A debug file in the tree can enter the next commit.

Revert paths that map a failed fetch to empty success. Empty success hides a broken context pack from the log. The agent then continues as if the page had been read.

Numbered review steps

1. Freeze the network first

Check out the pull request on a disposable branch first. Run the suite with external network access disabled. A helper that fails closed is easier to judge fairly.

git fetch origin pull/218/head:pr-218
git switch --detach pr-218
npm test

Those three commands above are only a review sketch. They are not a transcript from a finished execution. Replace the remote name if the fork uses another label.

2. Separate pure packing from I/O

Keep one function that only wraps text and enforces a cap. Move fetch calls and disk writes behind a narrow interface. Leave that interface unimplemented inside the unit tests.

export function packUntrusted(label, text, limit) {
  const body = String(text).slice(0, limit);
  return [
    `<untrusted source="${label}">`,
    body,
    "</untrusted>",
  ].join("\n");
}

The wrapper above is a proposal for the review harness. It does not prove the model will obey the fence. It only keeps untrusted text bounded and easy to spot.

3. Replace the open client with an allowlist

Accept exact hosts from a list a human already reviewed. Require HTTPS on every URL the helper approves. Refuse userinfo, blank hosts, and every non-HTTPS scheme.

const ALLOWED = new Set(["docs.example.com"]);

export function approveUrl(raw) {
  const url = new URL(raw);
  if (url.protocol !== "https:") {
    return { ok: false, reason: "scheme" };
  }
  if (url.username || url.password) {
    return { ok: false, reason: "userinfo" };
  }
  if (!ALLOWED.has(url.hostname)) {
    return { ok: false, reason: "host" };
  }
  return { ok: true, href: url.href };
}

Do not add private addresses just for local convenience. Do not match hosts with a loose substring check. Exact set membership is the rule this review enforces.

4. Pin limits with fixture tests

Cover one allowed host and one rejected host in tests. Cover a response body longer than the stated cap. Assert the packed body line stays within that cap.

import test from "node:test";
import assert from "node:assert/strict";
import { approveUrl } from "./approve-url.js";
import { packUntrusted } from "./pack.js";

test("rejects a host outside the set", () => {
  const verdict = approveUrl("https://evil.example/page");
  assert.equal(verdict.ok, false);
  assert.equal(verdict.reason, "host");
});

test("rejects a non-HTTPS scheme", () => {
  const verdict = approveUrl("http://docs.example.com/guide");
  assert.equal(verdict.ok, false);
});

test("caps packed untrusted text", () => {
  const packed = packUntrusted("docs", "x".repeat(80), 16);
  const lines = packed.split("\n");
  assert.equal(lines[1], "x".repeat(16));
  assert.equal(lines.length, 3);
});

The tests above are unexecuted examples for the reviewer. They do not replace a run against the actual branch. Apply the revert list before treating a green run as final.

5. Keep debug bundles outside the worktree

Send debug output to a temp directory the reviewer chooses. Refuse any path that resolves inside the project root. Delete that temp directory when the review session ends.

import path from "node:path";

export function assertOutsideRepo(repoRoot, requested) {
  if (!path.isAbsolute(requested)) {
    throw new Error("debug path must be absolute");
  }
  const root = path.resolve(repoRoot);
  const full = path.resolve(requested);
  const rel = path.relative(root, full);
  const inside = rel === ""
    || (!rel.startsWith("..") && !path.isAbsolute(rel));
  if (inside) {
    throw new Error("debug bundle must stay outside the repo");
  }
  return full;
}

The path helper above is proposed review code only. This draft does not include an execution log for it. Run it on a temp path and a repo path before trust.

6. Write the decision before any model rerun

Put a short decision table in the review note itself. Rerun a model only after the revert list is applied. A second model opinion must not reopen a reverted fetch.

Decision table

Change	Trust	Revert	Test next
Delimiter wrapper with no I/O	Yes, if pure	No	Fixture round trip
Size cap constant	Yes, if pinned	No	Over-cap fixture
Fetch of model-named URLs	No	Yes	Allowlist rejection
Redirect following	No	Yes	Client flag stays false
Debug file inside the repo	No	Yes	Path refusal
Failed fetch as empty success	No	Yes	Error stays visible

Failure modes worth a fixture

A missing allowlist fails open under a friendly model prompt. The model can name a host the project never reviewed. The fixture should reject that host before any socket opens.

A size cap that lives only in a comment will drift. The next edit can raise the slice without a failing test. Pin the number in an assertion, not in pull request prose.

A debug write beside the source tree looks harmless in a demo. The next commit can pick the file up with a broad add. The path test should fail when the target sits inside the root.

Empty success on a fetch failure looks like a small convenience. Callers then build the next prompt around a silent gap. The test should expect a thrown error or a visible failure flag.

Where a scratch server fits

After the local revert, move the branch to a disposable server. MonkeyCode is relevant as one place to rerun the patched agent. Disclosure: This article was prepared as part of MonkeyCode's product outreach.

Operator materials cite free model access, a free server, and open-source status. They also cite a free token allowance near ten million tokens. This article does not re-measure that allowance or name any model.

Confirm the live terms before a review depends on them. Treat the free server as a scratch runner, not a trust boundary. Point that runner at the patched branch and the fixture file.

Keep production credentials off that disposable machine entirely. A model reached through free access may draft the review note. It still cannot approve the host list or the merge.

git clone --branch pr-218-reverted /path/to/local/repo review-218
cd review-218
node --test test/approve-url.test.js

The scratch commands above are unverified in this draft. Stop if the suite asks for a secret or a production host. Do not widen the allowlist just to turn a red test green.

Record the red case in the review note instead of hiding it. A free server share is not a hardware spec or a duration promise. Remove the scratch workspace when the review session ends.

Limits of this pass

The allowlist rots when a documentation host changes its name. Exact checks miss the new domain until a human edits the set. That host edit belongs in the same pull request as the domain.

The wrapper does not prove a model will respect delimiters. It only makes untrusted text visible and length-capped for review. Broader injection study sits outside this particular review pass.

Quotas and model lists can change without notice in public docs. Do not copy an older token figure into a runbook as a guarantee. Read the current project page on the day of the review.

Who should skip this approach

Skip this approach for repositories that store customer data. Skip it when security review already locked the network policy. Skip it if no human will actually read the revert list.

Do not use the free server as a production runtime. Do not treat a free token pool as a load-test budget. Do not merge because a model agreed with the agent author.

Teams without fixture discipline should not adopt the helper yet. A verbal allowlist written in chat will drift by the next sprint. Wait until the rejection tests live in the repository itself.

Closing note

The useful fragment was the packing idea, not the open client. Keep the pure wrapper and the pinned size cap. Revert the fetch, the redirects, and the in-repo debug write.

Run the fixture tests before any model sees the branch again. Readers who keep a scratch server can replay this same review. Check current free-access terms, then run the rejection tests.

Reject Shell Strings in an Agent Tool Pull Request

Jordan Li — Sun, 11 Oct 2026 20:58:18 +0000

A Friday incident stalled a small checkout service. The failing job came from an agent pull request. The patch added a helper named run_check for smoke runs.

The diff held forty lines and looked mostly tidy. One template string broke that tidy picture. The helper built a shell command from a branch name.

Read the code before the summary

The agent summary promised a faster review loop. The handler told a stricter and smaller story. A branch field from tool JSON entered a command string.

// Untrusted sketch from the PR. Do not merge it.
function runCheck(branch) {
  const cmd = `git fetch origin ${branch} && npm test`;
  return exec(cmd, { shell: true });
}

Tool JSON counts as caller-controlled input in this handler. A shell string adds a second parser to the path. A test that only passes main never sees that parser.

Keep the goal and drop the mechanism

Keep the tool name if existing jobs call it. Keep the sequence of fetch and then test. Keep any comment that asks for a timeout.

Drop the template string on the next commit. Drop the shell true flag on this call path. Drop a package whose only role is wrapping a shell.

Demand a fixed argument list

Call execFile with a separate argument array. Reject branch text that fails a tight pattern. Set a timeout so a stuck child ends.

import { execFile } from "node:child_process";
import { promisify } from "node:util";

const execFileAsync = promisify(execFile);
const BRANCH = /^[A-Za-z0-9._/-]{1,80}$/;

export async function runCheck(branch) {
  if (!BRANCH.test(branch)) {
    throw new Error("branch_rejected");
  }
  await execFileAsync("git", ["fetch", "origin", branch], {
    timeout: 20_000,
    shell: false,
  });
  return execFileAsync("npm", ["test"], {
    timeout: 120_000,
    shell: false,
  });
}

Treat this block as a proposed patch. It was not benchmarked on live hardware. It defines the shape the review should require.

Lock the rule into a test file

Store the harness next to the other tool tests. Read the handler source from disk during the test. Fail the job if shell mode comes back.

import assert from "node:assert/strict";
import { readFileSync } from "node:fs";
import test from "node:test";
import { runCheck } from "../tools/runCheck.js";

const src = readFileSync(
  new URL("../tools/runCheck.js", import.meta.url),
  "utf8"
);

test("handler does not build a shell string", () => {
  assert.equal(src.includes("shell: true"), false);
  assert.equal(src.includes("exec(`"), false);
  assert.match(src, /execFile/);
  assert.match(src, /timeout:/);
});

test("bad branch names fail before a process starts", async () => {
  await assert.rejects(() => runCheck("feature;echo"), /branch_rejected/);
  await assert.rejects(() => runCheck(""), /branch_rejected/);
});

Those two rejection cases must fail closed. They should throw before git or npm starts. Run them with node --test on a clean checkout tree.

A local pass is only a starting signal. The same file should run on a disposable runner. Record the exit code in the pull request.

Review the pull request in eight steps

Freeze the diff and hide the agent summary for the first pass.
List values that arrive from tool JSON, env files, or review text.
Flag every value that reaches exec, spawn, or a backtick command.
Replace shell interpolation with argument arrays and an explicit reject path.
Add the source assertions and both rejection cases from the harness.
Run that file on a free server that holds no production secrets.
Send only the frozen diff and the test log to a free model.
Let a human owner approve the merge after reading both outputs.

Step six needs a scratch workspace with no shared state. Clone the pull request branch into that workspace. Leave production tokens off that review machine.

Copy the command output into the review thread. Treat a nonzero exit as a revert, not a debate. Do not rerun the job with production credentials attached.

Let a free model draft notes, not approval

Disclosure: This article was prepared as part of MonkeyCode's product outreach. The operator describes MonkeyCode as an open-source project. Free model access can draft the first review note.

A free server option can run the harness off a laptop. This draft does not name a specific model. It does not state a token quota, disk size, or keep-alive time.

Those details change, so check the current docs before a long run. Older writeups are not evidence of today's limits. Paste the diff and the test log.

Leave out env files, customer rows, and cookies. Ask for three columns named trust, revert, and test. Drop any line that invents an advisory number.

Review this frozen diff for shell interpolation.
Return trust, revert, and test rows.
Do not suggest new network calls.

Stop if the note requests a live secret. Rerun the harness if the note rewrites the handler. Keep the model text as a comment.

It is not a merge approval by itself. The human owner still owns the merge decision. A green model note never replaces that owner.

Readers who already have access can rerun this harness on a scratch branch. Attach that exit code beside the review note.

Use a matrix when the diff grows

Signal in the diff	Review action	Reason
Tool field copied into a shell string	Revert	Untrusted text meets a second parser
`shell: true` beside a fixed command	Revert	`execFile` does not need that flag
Test covers only the name `main`	Extend	A bad branch never gets rejected
Missing timeout on `git` or `npm`	Add	A stuck child should not hold the job
Package added only to wrap `sh`	Remove	Supply chain grows without a real gain
Model note with no file citation	Ignore	Uncited advice is not review evidence

This matrix works only as a review aid. It is not a score and not a benchmark.

A row can be wrong for a sanctioned wrapper. Cite that wrapper in the review comment. Still ban raw command strings in the new tool.

State the limits in the same review

The branch pattern is a gate, not a full ref grammar. Valid Unicode names fail closed on purpose. Production CI should call git and npm by absolute path.

A free server image can change without a notice in this article. No uptime, region, or storage promise is made here. A source scan misses calls that sit in other files.

Run the scan across the whole pull request. The execFile call still starts local binaries. A writable PATH stays on the review list.

A comment that mentions shell true can trip the source scan. Confirm the hit against the real call before reverting a doc file.

The free model sees only the pasted context. It can miss a policy note in another doc. Human approval remains required for auth, billing, and migrations.

Do not use this path in three settings

Skip the free server when the runner holds production credentials. Shared capacity is the wrong place for those secrets. Use an internal locked runner for that case.

Skip the path when policy demands a signed private image. A shared option cannot satisfy that control. Record the exception in the team handbook.

Skip it when maintainers must run arbitrary scripts. That need wants a separate allowlist design. A thinner shell string is not that design.

Style-only reviews can safely ignore this harness. Use the harness when tool code can start processes. That boundary keeps the extra work small.

Close the review with one concrete comment

Name the reverted template string in the comment. Link the harness file and the exit code. Say the free-server run covered this file only.

Ask the author to confirm current free-tier limits first. A larger suite can wait for that check. The comment does not need a product pitch.

The checkout service needs a boring handler. A branch name must stay plain data. It must not become a shell script.

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.