DEV Community: Casey Chen

A 429 Loop Is Not Resilience: Reviewing Agent PRs That Retry Model Calls

Casey Chen — Tue, 13 Oct 2026 03:17:43 +0000

An agent pull request that wraps model calls in retries is a capacity change, not a reliability patch. Trust the error classification and the stop condition. Revert unbounded loops, retries on non-idempotent writes, and any comment that treats a shared server as spare capacity. Prove the budget with a local fault injector before the branch is allowed to touch a hosted endpoint.

Read the loop before the summary

Agent summaries promise resilience. The diff says otherwise when a new function calls the client inside a loop. Start with the loop bound, the status set, and whether the call writes state. Those three facts decide the review.

The initial call counts as attempt 1. A helper that retries three times after a failure sends four calls. Spell that in the test name so a later edit cannot widen the budget silently.

Look for four smells before reading the rest of the PR body.

A loop with no ceiling, or a ceiling copied from a comment rather than a named constant with a test.
A bare exception handler, or a retry on every status below 500, including 401, 403, and 422.
Sleep on the request thread, with no jitter and no respect for Retry-After.
The same wrapper around reads and writes, with no idempotency key.

Label every hunk

Use one label per hunk. Do not average the patch into a single pass.

Diff signal	Label	Why
Typed timeout, no retry added	Trust	Bounds wait time without multiplying calls
Retry only on connection reset, cap 2, read-only	Prove	The cap is necessary, not sufficient
Unbounded loop or a high cap with no test	Revert	Unjustified amplification
Retry on 400, 401, 403, 404, 409, 422	Revert	Deterministic client failures will not heal
Retry on 429 with jitter and a hard stop	Prove	Needs a clock-injection test
Retry around mail, deploy, or billing POST	Revert	Replay is a second effect
Idempotency key plus a replay test	Prove	The key is untrusted without the test
Hardcoded quota or a free-tier capacity claim	Revert	Unverified capacity is not an input
Test that only mocks the final 200	Revert the test	It hides the policy

A green unit test that stubs the second attempt as success does not move a row from Prove to Trust. The label column is the merge gate.

429 Too Many Requests is specified by RFC 6585. Retry-After belongs to HTTP semantics in RFC 9110. Treat other 4xx codes as non-retryable unless the spec for that code says the request can be repeated safely.

A classifier you can run locally

The module below is a review aid, not a client SDK and not a benchmark. It has no network calls. It targets Python 3.10 or newer.

The cap of 3 is a merge heuristic for interactive calls, not a vendor limit. Nothing here was executed for this article.

from dataclasses import dataclass

NON_RETRYABLE = frozenset({400, 401, 403, 404, 409, 422})
TRANSIENT = frozenset({408, 425, 429, 500, 502, 503, 504})

@dataclass(frozen=True)
class Attempt:
    status: int | None
    attempt: int
    max_attempts: int
    idempotent: bool
    writes_side_effect: bool
    honors_retry_after: bool

def label(a: Attempt) -> str:
    if a.max_attempts < 1 or a.max_attempts > 3:
        return 'revert'
    if a.writes_side_effect and not a.idempotent:
        return 'revert'
    if a.status in NON_RETRYABLE:
        return 'revert'
    if a.status == 429 and not a.honors_retry_after:
        return 'prove'
    if a.status in TRANSIENT or a.status is None:
        if a.attempt >= a.max_attempts:
            return 'trust_stop'
        return 'prove'
    if a.status is not None and 200 <= a.status < 300:
        return 'trust'
    return 'prove'

if __name__ == '__main__':
    samples = [
        Attempt(429, 1, 3, True, False, False),
        Attempt(401, 1, 2, True, False, True),
        Attempt(503, 2, 2, True, False, True),
        Attempt(503, 1, 8, False, True, True),
    ]
    for s in samples:
        print(s.status, label(s))

Expected lines are 429 prove, 401 revert, 503 trust_stop, and 503 revert. If those four labels are not obvious from the diff under review, the helper is not ready.

Pair the classifier with five local cases. Keep every case on a stub. None of them should open a socket.

Inject 401 and assert a single attempt.
Inject 429 with a two-second Retry-After and assert one bounded delay, not a tight loop.
Inject 503 until the cap and assert stop, counting the original call.
Mark a POST as a side effect without an idempotency key and assert refusal.
Advance a fake clock. Do not call time.sleep in CI.

class FakeClock:
    def __init__(self) -> None:
        self.delays: list[float] = []

    def sleep(self, seconds: float) -> None:
        if seconds < 0 or seconds > 30:
            raise AssertionError(seconds)
        self.delays.append(seconds)

That clock is an unexecuted proposal. Wire it into the helper under review. A production sleeper that ignores the injected function fails case 5 by construction.

Idempotency key, not a comment

A comment that says a call is safe to retry is not a key. Trust a stable key derived from the job id and the step name. Revert a key built with a fresh random value inside the loop. A new value per attempt defeats deduplication on the server.

def idempotency_key(job_id: str, step: str) -> str:
    # Unexecuted proposal: same key on every attempt of this step.
    return 'job-%s-%s' % (job_id, step)

Cancellation between attempts

A loop that checks an abort flag only before the first call will keep going after the worker is cancelled. Trust a check between attempts. Revert a helper that cannot observe shutdown. On a shared server, a cancelled job that continues is wasted capacity and a stale-write risk.

Commands for the pass

rg is convenient. git grep -n is the fallback when rg is absent. Replace the base ref with the PR base.

git diff --unified=5 origin/main -- '*.py' '*.ts' '*.go' | rg -n 'retry|backoff|max_retries|Retry-After|while True|tenacity'
rg -n 'time.sleep' tests

The second search is a smell detector, not a proof. A clean result still needs the five cases. A hit on time.sleep in tests means the clock is real, so the job can stall under a 429.

Suggested comment to paste and edit, not a record of a specific review. Trim it to the hunk you are looking at.

This wrapper multiplies calls. Cap reads at 2 attempts after you count the original call, refuse retry when the call writes state, and honor Retry-After on 429. Inject failures with a fake clock. A job that loops against a hosted server to see what happens should not be accepted.

Where a free model path changes the gate

Some teams let an agent draft this patch on a hosted assistant, then point CI at a free server instead of a stub. That choice does not relax the table.

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

MonkeyCode's free model access and free server option are the only availability claims used here. This article does not state a token quota, a model list, hardware, uptime, or a duration. Those figures were not verified, and they are the wrong kind of constant to paste into a retry helper. If a diff comment cites a specific free allowance as the reason retries are safe, revert the justification and ask for a primary doc link in the PR.

Keep the hosted path in one place. Use free model access, if it is already available, to draft the classifier and the five tests. Run the tests on a local stub.

After they pass, one canary call can confirm auth and URL shape. It cannot tell you whether backoff terminates.

Do not parallelize that canary. Do not loop it to find a ceiling. A shared free server has neighbors.

It is not a load-test target. A 429 from it is a stop sign for this review, not a defect to hammer through.

Limitations

Status codes are a partial taxonomy. Timeouts, reset streams, and truncated JSON need separate branches. This classifier does not cover them.

Retry-After may arrive as a delta or as an HTTP date. The sample does not parse either form.

The cap of 3 is a review heuristic for interactive calls, not a capacity study. Batch jobs need a ceiling taken from your own queue metrics. This protocol will not tell you whether a model answer is correct. It only decides whether a retry policy is safe to land.

Skip the approach when the call sends mail, charges a card, or mutates production without an idempotency key already under test. Skip it if the only oracle available is the hosted server. Also skip it when the change needs a formal error budget. A table is a gate, not an SLO.

If MonkeyCode is already how the draft gets written, keep the free server outside the retry loop and spend one capped canary only after the local labels match. Read the live terms before anyone writes a number into the patch.

A Schema Pass Is Not an Execution Grant: Reviewing Agent PRs That Add Tools

Casey Chen — Sun, 11 Oct 2026 22:20:26 +0000

Merge the parser. Hold the grant. An agent pull request that adds function calling is three changes in one diff: a schema, a policy, and a side effect. Schema checks can land like any other contract. Policy text and a live handler cannot ride along just because a model emitted valid JSON.

This piece uses a synthetic fixture, not a production incident report. No latency, cost, or pass-rate figures are claimed. The commands are local. The tests below are unexecuted examples, labeled so a reviewer can run them instead of trusting this draft.

What the diff usually hides

Agent PRs in this class often look small. A registry gains one tool. A test checks that one sample payload validates. The handler is short, usually added so the task can "finish."

That shape is the risk. Validation answers whether an object matched a schema. Execution answers whether this process may perform an action. Those questions fail independently.

Watch for these signals in the same commit:

A tool name that is a vague verb, such as run_task or apply_fix
A description that hands judgment to the model ("do what is needed")
A parameter typed as an unconstrained string
A handler that can reach a shell, an HTTP client, or the filesystem
A config default that enables the tool outside tests

Review each signal on its own. A green happy-path test does not clear the bundle.

Synthetic diff under review

The module below is a constructed review fixture. It is not a recommended implementation, and the handler does not start a process.

# synth/agent_tools.py — fixture, not production code
from dataclasses import dataclass
from typing import Any, Callable

@dataclass
class Tool:
    name: str
    description: str
    parameters: dict
    handler: Callable[[dict], Any]

def proposed_tools() -> list[Tool]:
    return [
        Tool(
            name="run_task",
            description="Run whatever is needed to finish the user task",
            parameters={
                "type": "object",
                "properties": {
                    "command": {"type": "string"},
                    "url": {"type": "string"},
                },
                "required": ["command"],
            },
            handler=lambda args: args["command"],
        )
    ]

Returning the string is deliberate. The review question is whether this shape may become an executor, not how to invoke one.

What can land with the parser

Trust only hunks a reviewer can re-run without a model and without a network.

The schema document, if a test pins the tool name, required keys, and types.
A pure function that accepts or rejects an argument object before any handler runs.
A dry-run result that records the intended action and performs none.
Renames, comments, or tighter types, once those tests exist.

A passing schema test is evidence about parsing. It is not evidence about least privilege.

What to split out of the PR

Split or drop any hunk that does the following:

Turns the new tool on from a default constructor, production config, or shared fixture
Forwards a model-supplied string into a shell, a query API, or a URL fetch
Uses the description string as the only constraint
Adds a catch-all tool next to a narrow one "for flexibility"
Copies raw arguments into logs, metric labels, or outbound errors
Asserts on live assistant prose instead of the gate's return value

This is not a rejection of tool calling. It is a rejection of merging the grant with the parser.

What the replacement must prove

The replacement PR needs three artifacts: an allowlist expressed as data, a gate that rejects unknown actions before the handler, and tests that call a fake runner only.

Gate

# synth/grants.py
ALLOWED = {
    "format_python": ("ruff", "format"),
    "show_version": ("ruff", "--version"),
}

class GrantRejected(Exception):
    pass

def gate(action: str, extra: list[str] | None = None) -> tuple[str, ...]:
    if action not in ALLOWED:
        raise GrantRejected(action)
    if extra:
        raise GrantRejected("extra arguments are out of policy")
    return ALLOWED[action]

def dry_run(action: str) -> dict:
    argv = gate(action)
    return {"action": action, "argv": argv, "executed": False}

ruff is a stand-in binary name. A real repository should list its own fixed argv pairs. Python 3.11+ is assumed for list[str] | None.

Tests

# tests/test_tool_grant.py
import pytest
from synth.grants import GrantRejected, dry_run, gate

def test_known_action_is_a_fixed_argv():
    assert gate("show_version") == ("ruff", "--version")

def test_dry_run_does_not_execute():
    report = dry_run("format_python")
    assert report["executed"] is False
    assert report["argv"][0] == "ruff"

@pytest.mark.parametrize("action", ["run_task", "bash", "curl", ""])
def test_unknown_action_is_rejected(action):
    with pytest.raises(GrantRejected):
        gate(action)

def test_extra_arguments_do_not_widen_the_grant():
    with pytest.raises(GrantRejected):
        gate("show_version", extra=["--unsafe"])

git diff --stat
git diff -U3 -- synth/agent_tools.py synth/grants.py
python -m pytest tests/test_tool_grant.py -q

Intended outcome of this unexecuted fixture: known actions return a fixed argv, dry-run sets executed to false, unknown actions raise, and extra arguments do not widen the grant. This draft does not report an executed pytest session. If a later edit appends extra onto argv, the last test is the signal. A model transcript is not.

Decision table

Diff signal	Bucket	Merge action
JSON Schema for a named tool, types pinned by tests	Parser	Land after the local test run
Description narrowed to one verb and one object	Policy text	Keep as documentation only
Unconstrained `command` or `url` string	Grant	Split out
Import of a shell, HTTP, or filesystem helper in the handler	Side effect	Require dry-run before any call
Config or constructor default enables the tool	Deploy surface	Remove the default
Test depends on a live model or a shared remote host	Oracle	Replace with the fake runner
Allowlist data plus rejection tests	Proof	Re-merge the grant alone

Apply the table per hunk, not per pull request. One PR can contain a trustworthy parser and three hunks that should not land.

Review order on a mid-sized agent diff

Use this order when the diff is past a couple of screens. It keeps the grant from hiding inside formatting noise.

List files touched. Separate schema, config, handler, and tests.
Read config and constructors before descriptions. A default-on flag is a deploy, even if the schema looks narrow.
Search the handler for process, network, and filesystem calls. If any exist, demand a dry-run path in the same replacement.
Confirm tests import the gate, not a hosted client.
Write the review comment against the table row, and name the hunk.

Comment template:

Hunk: synth/agent_tools.py, tool run_task
Table row: unconstrained command string / grant
Decision: split out. Parser tests may land.
Replacement required: allowlist entry, gate rejection test, dry-run flag.
Not accepted as evidence: assistant prose, a green call to a shared host.

That comment is enough for a second reviewer to repeat the check. It does not depend on which model drafted the original patch.

Where optional hosted access fits

The gate does not need a hosted model. Review quality comes from the allowlist and the tests.

Disclosure: This article was prepared as part of MonkeyCode's product outreach. Operator-supplied context for this draft is limited to two availability claims: free model access and a free server option. Model names, quotas, hardware, duration, and benchmarks are omitted on purpose. They were not verified against a primary source here, and they change. Read current product documentation before use.

Optional sequence, after the local tests exist:

Keep tests/test_tool_grant.py as the merge bar.
Use free model access only to draft extra rejected-action names. A human deletes vague, duplicated, or out-of-scope cases before commit.
Use a free server only as a manual replay host for one recorded tool-call trace, on scratch input, outside CI.

Do not add that server to required fixtures. Do not fail the build when the free option is slow, capped, or withdrawn. A replay can suggest a missing rejection name. It cannot approve a handler.

Suggested note after a manual replay:

Replay host: free server option, manual, not a CI dependency.
Docs checked on: YYYY-MM-DD (reviewer fills the date).
Merge decision: tests/test_tool_grant.py, not the replay transcript.

If the docs are unavailable, skip the replay. Local tests still stand. If the access terms fit repository policy, those two free options are enough to draft rejection cases and replay one trace. Cite the docs date in the review comment. Do not cite a remembered allowance.

Limitations

The allowlist is only as careful as the binaries it names. This fixture does not cover tools loaded after startup, or a second agent that mints schemas at runtime. Description text can still mislead a model when the gate is correct. Enforcement belongs in gate, not in the prompt.

No count is offered for how often agent PRs mix these hunks. The table is a procedure, not a prevalence study. Teams bound by a data-processing addendum, a contractual uptime clause, or a ban on third-party inference should not send traces to a free server at all.

Who should skip this

Skip the hosted replay when the repository cannot accept an external processor, even for a scratch trace. Skip the grant split only when the change truly has no side effects, which is uncommon once a handler exists. Do not return this PR to the same agent to "fix the tests" unattended. The agent that widened the grant is a poor judge of whether the grant should exist.

Local tests remain the bar. Hosted drafting and replay stay optional, time-bounded, and disposable.

Model Snapshots Fail as Oracles: A Three-Bucket Review for Agent Diffs

Casey Chen — Sat, 10 Oct 2026 18:58:59 +0000

An agent pull request that deletes a local fixture and commits a model transcript has not added a test. It has stored one observation. Keep the intent to cover the parser. Revert the live call, the golden file, and any silent reroute. Prove the behavior with a fixture the repository owns.

That rule does not depend on which assistant drafted the patch. Review the diff. The generator's confidence is not evidence.

What landed in the branch

The shape is usually small. A file such as tests/golden/answer.json appears. A helper builds a client from a base URL. The assertion becomes equality against the new file. CI changes just enough for the job to reach an external inference host.

Sometimes a second hunk hides in the client. On timeout, quota, or HTTP 429, the code assigns a different base URL and sends the same payload again. That is not error handling. It is a second deployment, selected at runtime, with no review of which data leaves the process.

Read network edges before you read the PR prose. A required check that opens a socket will fail for reasons that are not regressions in your code. It also makes every fork run a client of an external host.

Trust, revert, prove

Split every hunk into three buckets before you write a comment. Short diffs still need the split. Length is a weak signal.

Trust lines that remain true when the network is off:

A pure parser for the completion payload.
The short list of fields the application actually reads.
A regression expressed as bytes in the repo, written by a reviewer or generated from a schema you control.

Revert lines that bind the merge gate, or production, to an environment you do not control:

HTTP clients in the default test path.
Checked-in transcripts used as equality oracles.
An except block that swaps in another base URL.
Logs of raw prompts, tool outputs, or Authorization values.
Hard-coded quotas, model identifiers, regions, or comments that say the host is permanently free.
Shell helpers that build a command string and pass shell=True.

Prove with two jobs. The contract job uses a local cassette and is required. The smoke job hits an external host only when someone dispatches it. Smoke may check a status code. It must not check exact prose.

Why equality against model text fails

Three properties of the design break the oracle. No benchmark is required to see them.

Sampling and server-side changes make byte-identical output unstable. A green run today does not pin the server you will get next week. The transcript can also echo the prompt. If the agent copied a credential or a customer string into the request, the golden file publishes it.

So the file is evidence of a past call, closer to a log than to a specification. Logs are not contracts. Merge the parser if it is sound. Delete the captured answer.

Latency fields fail the same way. A cassette that asserts elapsed_ms < 800 couples your build to someone else's queue. Drop timing, token-usage, and provider metadata from any file you intend to keep.

Cassette rules

A cassette is a contract only if a reviewer can explain every field. Keep three ideas in the file: a synthetic input label, the payload fragment the parser accepts, and the expected structure. Drop chain-of-thought text and raw tool traces. Those values change without your application code changing, and some of them are sensitive even when the prompt looked harmless.

Name the file after the behavior, not after the host or the date. parse_completion.json survives a server change. free_host_2026_10_10.json does not. If the agent used a timestamped name, rename it in the same review. The filename is part of the claim.

Worksheet

Fill the action from the patch, not from the description.

Question	Fail signal	Action
Does required CI open a socket?	`httpx`, `requests`, `curl`, or a client import under `tests/` without a skip mark	Move it out of the default job
Is model prose committed?	New JSON or text under `tests/` or `fixtures/` whose body is a completion	Revert; add a hand-written cassette
Can cache keys collide across tenants?	Key derived only from the prompt text	Revert, or include tenant id and contract version
Does an error change the host?	`except` assigns a second base URL	Revert silent failover; default the flag off
Did secrets enter the diff?	`.env`, `Bearer`, `sk-`, full prompt dumps	Revert; rotate if the commit is already public
Does the PR state a quota or model name it cannot cite?	Token counts, hardware, permanence	Delete the claim until a primary source exists

An external inference option is not an allowlist, a capacity plan, or an oracle. If the patch treats it as any of those, that hunk is a revert.

Commands

These commands are a proposed review sequence. They are not output captured from a named repository.

git diff --stat origin/main...HEAD
git diff origin/main...HEAD -- tests .github docker-compose.yml
rg -n -i "httpx|requests|curl|base_url|subprocess|shell=True|golden" tests .github
rg -n -i "authorization|api_key|bearer |sk-|prompt" -g "!**/node_modules/**"

Open new files under tests/ first. The risky call is often in a helper with a harmless name, such as client_for_tests or load_golden.

If the search hits shell=True, stop on that hunk. A shell command is concatenation. Unquoted expansion splits on spaces and interprets metacharacters. Prefer a fixed argument list and shell=False, or revert the helper. A note that the agent already ran the script is not a review outcome.

Proposed harness

The Python below is a sketch for reviewers to demand. It has not been executed for this article. It is not a benchmark, and the helper probe_status is a placeholder.

import json
import os
from pathlib import Path

import pytest

CASSETTE = Path(__file__).parent / "cassettes" / "parse_completion.json"

def parse_completion(payload: dict) -> dict:
    choice = payload["choices"][0]["message"]["content"]
    return {"text": choice, "truncated": False}

def test_parser_reads_contract_fields_only():
    payload = json.loads(CASSETTE.read_text(encoding="utf-8"))
    assert parse_completion(payload)["text"] == "cassette-text"

@pytest.mark.smoke
def test_optional_reachability_smoke():
    pytest.importorskip("httpx")
    base = os.environ.get("SMOKE_BASE_URL")
    if not base:
        pytest.skip("SMOKE_BASE_URL unset; smoke is opt-in")
    status = probe_status(base)  # proposal: GET {base}/models or /health
    assert status == 200

The cassette should contain boring synthetic text. Do not paste a live completion into it. Exact prose assertions belong only on that local file, because you can change the file in the same pull request as the parser.

Pair the tests with a workflow fragment. This fragment is also unexecuted.

jobs:
  contract:
    steps:
      - run: pytest -m "not smoke"
  smoke:
    if: github.event_name == 'workflow_dispatch'
    steps:
      - run: pytest -m smoke
        env:
          SMOKE_BASE_URL: ${{ vars.SMOKE_BASE_URL }}

Manual dispatch is the point. An external host on the automatic path becomes a flaky required check. It is also an unbounded client of shared capacity whenever the workflow retries.

What not to multiply

Retries, matrix builds, and parallel smoke jobs multiply calls. A retry policy that was reasonable for a paid primary can be a load spike against a shared server. Keep smoke to one probe and no retry loop.

If you need throughput numbers, that is a different test. It needs an owner, a concurrency cap, and a stop time. This worksheet does not define that test, and a green status code does not supply those numbers.

Cache refills have the same shape as a bad golden file. A job that warms a cache by replaying production prompts at an external host is a data export. Refuse it unless every prompt is synthetic and the cache key includes a tenant boundary plus a contract version.

Using a free model path without promoting it

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

MonkeyCode enters this review in two narrow places. Free model access can help a reviewer draft the comment or a synthetic cassette, after which a human still reads the diff. A free server option can be the value of SMOKE_BASE_URL on the manual job.

That is the full scope used here. This draft does not name models, token quotas, hardware, duration, or permanence, because those details were not supplied as citable sources. If a generated README claims a specific allowance, do not publish the figure. Ask for a primary document first. Availability is not a capacity plan.

Delete the product name and the worksheet still holds. Observations are not contracts. Required checks should not depend on an external inference host.

Who should skip this

Skip the split if you cannot keep fixtures free of customer data. Synthetic prompts only. If a real prompt is already in the branch, reverting that file is the first action. Rotate any credential that sits beside it.

Skip it if CI cannot exclude marks. A smoke test that still runs on every pull request is the original bug with a new name. Fix the workflow filter before discussing hosts.

Skip it when the main change is authorization, tenancy, or a tool that has side effects. Those patches need a threat model. A cassette will not catch a confused-deputy bug in a shell tool, and a status code will not show which files the tool touched.

Streaming responses also sit outside the sketch. Partial buffers, disconnects, and reassembled tokens need their own assertions. Do not freeze one stream mid-flight and call that file the spec.

Comment shape

Write the three buckets in the review. A usable comment looks like this:

Trust the parser and the field list in the application module.
Revert the golden transcript, the default base URL in tests, and the failover assignment.
Prove behavior with the cassette on the required job. Leave external reachability on manual dispatch.

If a free model path and a free server are already available to the team, attach them only to that manual job, after the required check is local. The merge decision stays on the cassette either way.

A Free Endpoint Is Not an Allowlist: Reviewing Agent PRs That Add Fetch

Casey Chen — Fri, 09 Oct 2026 15:34:09 +0000

An agent pull request that switches a client onto a free model server is three changes, not one. The client binding, the live smoke, and any new outbound tool each move data across a trust boundary. Review them separately. Trust a hunk only when a local test proves the bound. Revert the live path when that proof is missing.

The specimen in this note is constructed for review practice. It is not a report of a merged repository, a production incident, or a measured product limit.

What the specimen changes

The diff adds four surfaces that look unrelated in a file list:

agent/client.py defaults base_url to a hosted model endpoint and reads a key from the environment.
agent/tools/fetch.py registers a tool that issues GET for a URL chosen by the model.
.github/workflows/model-smoke.yml runs the client on push and pull_request.
tests/test_client.py expects a live completion instead of a stub.

Each file can be justified in isolation. The combination sends prompt text, tool traffic, and CI identity onto a network path the repository did not have. That combination is the review subject.

A short diff stat is not evidence of a small risk. Forty lines that open egress outweigh four hundred lines of renaming.

Gate table

Use the table as a merge gate. A row that fails its proof column blocks the PR even when the other rows look tidy.

Hunk	Trust only if	Revert if	Proof
Client base URL	Default is empty or loopback, and hosted URLs arrive from outside config	Library code defaults to a hosted endpoint	Construct the client with an empty environment and assert no socket use
Credential read	The key stays in the runner secret store	A key, `.env`, or Authorization value is in the diff	Search the diff for `API_KEY=`, `sk-`, and `Authorization`
Fetch tool	Allowlist is executable code, and redirects are off	Any URL, `file:`, or link-local target is accepted	Table tests for allow and deny, including redirects
CI smoke	Trigger is `workflow_dispatch` only, prompt is canned, timeout is set	Trigger is push, pull_request, or schedule	Read the `on:` block; run tests with the network stubbed
Tests	Default suite makes no network call	Passing requires a live server	Run the suite with sockets disabled

Review order

Read in this order so a client rename cannot hide the permission change.

Tool schema and HTTP helper.
Workflow triggers, then steps.
Tests, looking for live hosts and HTTP libraries.
Client defaults last.

A clean client does not redeem an open tool. A green live test does not redeem an open tool either. The live test is the policy you are trying to avoid making mandatory.

Commands

git diff --stat origin/main...HEAD
git diff origin/main...HEAD -- agent/tools .github/workflows tests
rg -n "httpx|requests\\.|base_url|workflow_dispatch|pull_request" agent .github tests

If the search hits both a fetch tool and a workflow trigger, treat the diff as one policy change. The model identifier string is the least useful line in the file.

Proposed allowlist

The module below is a proposed review gate for the specimen. It is not a benchmark, and it was not executed for this article.

from urllib.parse import urlparse

ALLOWED_HOSTS = frozenset({"docs.example.com", "static.example.com"})

class EgressDenied(Exception):
    pass

def assert_fetch_allowed(url: str) -> str:
    parsed = urlparse(url)
    if parsed.scheme != "https":
        raise EgressDenied(f"scheme not allowed: {parsed.scheme!r}")
    host = (parsed.hostname or "").lower()
    if host not in ALLOWED_HOSTS:
        raise EgressDenied(f"host not allowed: {host!r}")
    if parsed.username or parsed.password:
        raise EgressDenied("credentials in URL")
    return parsed.geturl()

The matching tests are the artifact a reviewer can ask the agent to keep. They fail closed.

import pytest

@pytest.mark.parametrize("url", [
    "https://docs.example.com/guide",
    "https://static.example.com/a.png",
])
def test_allows_known_hosts(url):
    assert assert_fetch_allowed(url) == url

@pytest.mark.parametrize("url", [
    "http://docs.example.com/guide",
    "https://user:pw@docs.example.com/x",
    "https://169.254.169.254/latest/meta-data/",
    "https://evil.example/docs.example.com",
    "file:///etc/passwd",
    "https://docs.example.com.evil.example/x",
])
def test_denies_unlisted_targets(url):
    with pytest.raises(EgressDenied):
        assert_fetch_allowed(url)

Two gaps belong in the review comment, not in a follow-up ticket that never lands. The function does not follow redirects, so the HTTP client must disable redirect following or re-check every hop. The function does not pin DNS, so an allowed name can still resolve somewhere unexpected. If the team cannot accept both gaps, remove the tool.

Comments are not the gate

A docstring that says not to fetch internal hosts is not an allowlist. A PR description that calls the smoke safe because the server is free is not a trigger restriction. Ask for the test file. If the agent updates the comment and leaves the tool open, revert the tool.

Where a free server is allowed to appear

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

For this draft, MonkeyCode is relevant only as an operator-supplied option: a free model access path and a free server option are availability claims, not measured limits. This note does not state model names, token quotas, hardware, duration, uptime, or price. Those facts change, and none of them were verified here. Check the current primary terms before use. A free path is still an external processor of whatever you send.

Keep the hosted call inside all four constraints:

Default client target stays empty or loopback. Hosted configuration is injected, not committed.
The only workflow trigger is manual, with an explicit boolean input.
The prompt file is canned. It contains no source, no secrets, and no user data.
The job stores status, not raw prompt or completion text. Timeout is short. Retries stay at zero until a separate review.

# Proposed fragment for review. Not a deployed workflow.
on:
  workflow_dispatch:
    inputs:
      run_live_smoke:
        description: Run one canned call against the hosted endpoint
        required: true
        type: boolean
jobs:
  smoke:
    if: ${{ inputs.run_live_smoke == true }}
    timeout-minutes: 5
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - name: Canned prompt only
        run: python -m agent.smoke --prompt-file ci/canned_prompt.txt

Revert on: push and on: pull_request from the specimen. A free endpoint still receives the prompt, still shares capacity, and still fails for reasons that have nothing to do with the change under review. Making that call required on every push converts a convenience into a merge dependency.

If the free server is down, skip the smoke. Do not fall back to an unbounded fetch, and do not fall back to a second hosted vendor inside the same unreviewed hunk. Fallback is a new data flow. It needs its own row in the gate table.

Failures a green unit test can hide

The allowlist is strict, and the same PR appends a host for a demo. Review allowlist edits like auth edits. A one-line host addition is a permission grant.
The smoke is manual, and the step still prints the response body. Log retention is a copy. Print a status code and a byte length, not the text.
The application allowlist is correct, and HTTPS_PROXY is set in the runner. Proxy configuration can move bytes after your check. Fix the runner environment, or deny the run when a proxy is present.
A retry wrapper lands beside the smoke. Five retries are five sends. The smoke job should set the retry count to zero.
The test suite mocks assert_fetch_allowed itself. That test proves the mock, not the gate. Call the real function.

Who should skip this split

Do not use the manual-smoke pattern when any item below is true.

Prompts can contain customer data, credentials, or unreleased source. Keep that traffic off hosted endpoints, free or paid.
Nobody on the reviewing team can read the tool schema. An unread schema is an unread permission. Close the tool until a human can name every argument.
Redirects cannot be disabled, and the host list cannot be kept explicit. Delete the fetch tool. A warning comment is not a substitute.
The goal is a latency or quality number. This protocol does not produce one. It produces a pass or fail on egress and on trigger scope.

Approval check

Approve only when four facts are visible in the diff: a local or empty client default, deny-by-default fetch tests, no live call on push or pull request, and no secret material. The hosted free-server option can remain as a manual canned smoke after those four are true. If one is missing, land the allowlist and revert the live path.

When a similar agent PR is already in the queue, run the deny cases against a stub before enabling any hosted smoke. The endpoint that answers is not the review. The bytes you allow to leave are the review.

Merge the Logger, Revert the Payload: Reviewing Agent PRs That Record Prompts

Casey Chen — Thu, 08 Oct 2026 10:55:07 +0000

Merge the logger. Revert the payload. Prove redaction with an offline test before any model call.

Agent pull requests often add tracing in the same diff as a new tool. The logger name looks harmless. The attributes do not. A span that stores messages, tool_args, or Authorization is an export, not a debug aid. Treat that export as the change under review.

Public DEV discussion in early October 2026 clusters around agent experiments and model behavior. Those posts are topic signals, not evidence for this review. The rule below does not depend on any of them. It depends on what the diff writes to disk, to a collector, or to a third-party endpoint.

This note uses a synthetic pull request. Nothing here is a measured incident, a benchmark, or a claim about a named production system.

The diff under review

The proposed change does four things:

It wraps tool execution in an OpenTelemetry-style span helper.
It attaches the full chat messages array and raw tool arguments as span attributes.
It adds GET /debug/last-trace so a reviewer can “see what the model saw.”
It points a fixture at a shared model server so the new path “stays realistic.”

The first item can be a real improvement. Items 2 through 4 change the trust boundary. Review them separately, even when they arrive in one commit.

A shortened form of the risky helper, labeled as a proposal and not as shipped code:

# Synthetic bad pattern. Do not copy into a service.
def run_tool(span, tool_name, tool_args, messages, headers):
    span.set_attribute("tool.name", tool_name)
    span.set_attribute("tool.args", tool_args)          # may include tokens
    span.set_attribute("llm.messages", messages)        # user content export
    span.set_attribute("http.authorization", headers.get("Authorization"))
    return tool_registry[tool_name](**tool_args)

Short attributes such as tool name, status, latency, and token counts are reviewable. Unbounded payloads are not.

What to trust

Trust only what you can check on the branch without a network call.

The span helper is pure enough to unit test: given a dict, it returns attributes, and it does not perform I/O itself.
Attribute keys are a fixed allowlist, not **payload spread into the span.
String values have an explicit max length, and over-limit values are dropped or hashed, not truncated into a still-readable secret.
The debug route is absent, or it is gated by an admin auth check that already exists in this codebase and is covered by a test.
Tests use a fake transport. They do not require DNS, a vendor key, or a shared server.

A green CI run is not evidence here. If the new test calls a live endpoint, the green result only proves that the endpoint answered. It does not prove that logs are safe.

What to revert

Revert these even if the rest of the PR is useful:

Default-on capture of messages, prompt, tool_args, tool_result, request bodies, or response bodies.
Header attributes, especially Authorization, Cookie, Set-Cookie, and x-api-key.
A debug route that returns the last prompt, the last tool payload, or a trace ID mapped to raw content.
Fixture or CI configuration that calls a free or shared model server to assert behavior.
A “temporary” sample rate of 1.0 on a path that still records content. Sampling does not fix an export. It only reduces how often the export happens.

Keep the logger skeleton if the allowlist is sound. Revert the payload attachment in the same review. Do not leave a TODO that re-enables full capture behind an environment flag defaulting to true.

What to prove

Prove three claims before merge: sensitive keys never become attributes, nested encodings do not sneak through a shallow check, and the test fails closed when the helper changes.

Offline redaction check

The following is an unexecuted review artifact. It is a proposal for the PR branch, not a result from a run on 8 October 2026.

import json

DENY_KEYS = {
    "messages", "prompt", "tool_args", "tool_result",
    "authorization", "api_key", "cookie", "set-cookie",
}

def flatten_keys(value, prefix=""):
    keys = set()
    if isinstance(value, dict):
        for key, child in value.items():
            path = f"{prefix}.{key}" if prefix else str(key)
            keys.add(str(key).lower())
            keys |= flatten_keys(child, path)
    elif isinstance(value, list):
        for child in value:
            keys |= flatten_keys(child, prefix)
    elif isinstance(value, str):
        stripped = value.strip()
        if stripped.startswith("{") or stripped.startswith("["):
            try:
                keys |= flatten_keys(json.loads(stripped), prefix)
            except json.JSONDecodeError:
                pass
    return keys

def assert_span_safe(attributes: dict) -> None:
    found = flatten_keys(attributes) & DENY_KEYS
    if found:
        raise AssertionError(f"span exports sensitive keys: {sorted(found)}")

Add one case per deny key, plus one case where the payload is a JSON string inside a “safe” field such as note. A shallow key in attributes check misses that case. This helper still misses base64 and homoglyph keys. Say that in the test module docstring so the next reviewer does not treat the denylist as complete.

Commands for the review

Run these from the PR branch. They are inspection commands, not a deploy.

git diff --unified=3 origin/main...HEAD -- '*.py' '*.ts' '*.yml'
rg -n -i "messages|prompt|tool_args|authorization|api_key|last-trace" \
  --glob '!**/node_modules/**' --glob '!**/.venv/**'
rg -n "set_attribute\(|span\.setAttribute\(" -g '!**/vendor/**'

Then classify each hit. A key in a test fixture that expects rejection is fine. The same key in a production helper is a revert.

Decision table

Diff signal	Trust	Revert	Prove
Span around tool call, name + status + latency only	Yes, if helper is pure	No	Unit test the attribute dict
Full `messages` or tool args on the span	No	Yes	Denylist test must fail on current patch, pass after revert
Token-count integer, no text	Yes, after type check	No	Assert value is `int` and bounded
`/debug/last-trace` without auth	No	Yes	Route test expects 404 or 401, never 200 with a body
Fixture URL set to a shared model host	No	Yes	Test suite passes with network disabled
Env flag `LOG_PROMPTS=true` by default	No	Yes	Default config test expects false or absent

If a row is “Revert,” do not spend the review on naming, formatting, or model choice in that hunk. Fix the boundary first.

Where a free model endpoint fits

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

MonkeyCode is an open-source project. The outreach brief for this article states free model access, a free server option, and a free allowance on the order of 10 million tokens. This draft does not independently verify model names, hardware, duration, rate limits, or whether that allowance is still current on 8 October 2026. Confirm the live terms before you plan any review budget around them. Do not treat the number as a permanent quota.

Use that access only after the offline test is red. A disposable server is an optional manual repro for a scrubbed, synthetic prompt. It is not a test dependency, not a fixture host, and not a place to replay production traces. The earlier failure mode, calling a free server from a fixture and calling the result a contract, still stands. This workflow inverts it: the contract is the local assertion. The server, if used at all, comes later and stays out of CI.

A practical order:

Land the denylist test and watch it fail on the unsafe helper.
Remove payload attributes and the debug route. Watch the test pass with the network off.
If you still need to see a live tool round-trip, send one synthetic prompt from a local script, outside CI, to a free server whose current terms you just checked.
Record latency or error class in the PR comment if you want. Do not paste the prompt, the tool args, or the response body into the comment.

If the free server is down, slow, or quota-exhausted, the review does not block. The merge gate is the offline test.

Before you spend a live call on an agent diff, read the current MonkeyCode free-access terms and keep that server out of the test process.

Who should not use this workflow

Skip the live-repro step, and skip any third-party model endpoint, when any of these are true:

Policy forbids sending even synthetic prompts to an external service, or the data class is unknown.
The payload under review may contain customer content, credentials, or regulated data. Do not “sanitize and retry” on a hunch. Revert the export and review locally.
You need a quota, model list, or uptime guarantee. None of those are established in this article.
The PR’s real risk is authorization logic, schema compatibility, or retry amplification. Those need their own reviews. A redaction test will not catch them.

The denylist is also the wrong tool for a team that has not agreed which keys are sensitive. Agree the key set in the review, commit it next to the helper, and fail the build when the set and the helper diverge.

Review outcome

Approve the tracing shape when attributes are small, typed, and allowlisted. Request changes when the diff records prompts, tool payloads, or headers, or when a test needs a shared server to pass. The logger can ship the same day. The payload should not ship at all.

Limitations are part of the outcome. This check is key-oriented. It will not catch every encoding, every side channel, or every future attribute name. Re-run the rg pass on the next agent PR that touches telemetry. Agent diffs repeat this pattern under new file names.

A Constructor Default Is a Deploy: Reviewing Agent PRs That Bind Model Clients

Casey Chen — Thu, 08 Oct 2026 08:37:32 +0000

An agent pull request that adds a model client is a routing change, not a prompt tweak. Trust the pure request builder. Revert hardcoded hosts, missing deadlines, and any log line that can print a message body. Prove the remainder with a static scan and, only if policy allows, one isolated call against a non-production host.

The pressure on reviewers is volume, not a single headline. Agent-written integration diffs are landing in ordinary service repositories, often with a mocked test and a constructor that already picked a host. This workflow is for that diff. It does not rank models, and it does not freeze vendor names, quotas, regions, or hardware.

Read the constructor before the prompt

Open the client module first. Then config loading. Then the call site. Then tests.

A typical agent PR touches four places:

llm/client.py, where the constructor and complete() live
llm/config.py, where environment variables become a config object
app/handler.py, where the application calls the client
tests/test_client.py, which often mocks HTTP and never builds a real config

That order matters. A careful prompt in the handler can sit on top of a default host you do not operate. Reviewers who start at the prompt tend to comment on wording and miss the route.

Use a wide diff and a plain search before you write the review:

git diff --unified=30 origin/main -- llm/client.py llm/config.py app/handler.py
git grep -n "timeout=None" -- llm app
git grep -n "http://" -- llm app
git grep -n "https://" -- llm app

The search is a hint, not a proof. Concatenated URLs and keys passed through helpers will not match. You still read the constructor line by line.

Prefer a config object that fails closed when the route is unknown:

import os
from dataclasses import dataclass

@dataclass(frozen=True)
class ClientConfig:
    base_url: str
    api_key: str
    timeout_s: float
    max_output_tokens: int
    log_prompts: bool

def load_config() -> ClientConfig:
    return ClientConfig(
        base_url=os.environ["MODEL_BASE_URL"],
        api_key=os.environ["MODEL_API_KEY"],
        timeout_s=float(os.environ.get("MODEL_TIMEOUT_S", "20")),
        max_output_tokens=int(os.environ.get("MODEL_MAX_OUTPUT_TOKENS", "512")),
        log_prompts=os.environ.get("MODEL_LOG_PROMPTS", "0") == "1",
    )

Required variables raise KeyError when absent. Numeric defaults are visible in one place. Prompt logging defaults off. Those three properties are the bar for the rest of the review.

What to trust

Trust code that is pure and locally falsifiable. A function that accepts messages plus ClientConfig and returns a request dict, with no socket opened, is in that set. So is a unit test that asserts the timeout, the token cap, and that message text never appears in headers.

Trust error mapping when it preserves status and a request id and does not itself retry. Trust a diff that stays inside the client and its tests. If the same PR edits production compose files, auth middleware, or webhook handlers, split it. Those files are a different review.

A green mock is weaker evidence than it looks. respx, responses, or a hand-rolled fake prove that your code called the fake. They do not prove DNS, TLS, retention, or which host the default argument names.

What to revert

Some lines should not survive review, even when the agent left a plausible comment above them.

A literal http:// or https:// base URL in production source, including a free or shared host added so the demo runs.
A key, bearer token, or cookie in the diff, in .env, or in a fixture that is not clearly synthetic.
log_prompts=True, or any logger call that interpolates messages, tool results, or raw bodies.
timeout=None, or a client timeout longer than the caller's own deadline.
A loop over fallback hosts that continues until one returns success.
A network test that skips unless an env var is set, while remaining collected on the default CI job.

Fallback hosts deserve a hard look. The PR text will call the loop resilience. The second host is a different data processor. It may log differently, retain differently, and sit in a different abuse domain. Shipping that loop is a policy change, not a one-line helper.

Retries belong in the same revert pile when they wrap the new client. A retry multiplies load on a dependency that is already failing. If the PR needs backoff, that is a separate design with a budget, not a for loop added while fixing a timeout.

What to prove before merge

Three layers answer three different questions. Do not collapse them into one "tested with AI" checkbox.

Layer	Question	Pass condition
Static	Can this module import with no network and no key?	No production URL literal; missing required env raises
Unit	Does the builder honor config?	Timeout, token cap, and auth header match; prompt text stays out of headers and logs
Isolated call	Does one real request stay bounded?	Non-production host only; synthetic prompt; status recorded; message body not logged

Skip the isolated call when you have no approved non-production target. Merge the pure builder behind a feature flag that defaults off, rather than pointing the constructor at production to see whether it works.

Paste-in scan for the review

The script below is a review aid for this article. It is illustrative. It was not run against a live model service, and it is not a complete security scanner.

#!/usr/bin/env python3
"""Flag constructor defaults a reviewer should challenge."""

import re
import sys
from pathlib import Path

PATTERNS = {
    "url_literal": re.compile(r"https?://\S+"),
    "timeout_none": re.compile(r"timeout\s*=\s*None"),
    "log_prompt": re.compile(
        r"log_prompts\s*=\s*True|logger\.\w+\(.*messages", re.I
    ),
    "hardcoded_key": re.compile(
        r"(api_key|bearer)\s*=\s*['\"][^'\"]+['\"]", re.I
    ),
    "fallback_loop": re.compile(r"for\s+\w+\s+in\s+.+hosts", re.I),
}

def scan(path: Path) -> list[str]:
    text = path.read_text(encoding="utf-8")
    found = []
    for name, pattern in PATTERNS.items():
        for match in pattern.finditer(text):
            line = text[: match.start()].count("\n") + 1
            snippet = match.group(0)[:80]
            found.append(f"{path}:{line}: {name}: {snippet}")
    return found

def main() -> int:
    roots = [Path(p) for p in sys.argv[1:]] or [Path("llm")]
    found: list[str] = []
    for root in roots:
        files = [root] if root.is_file() else root.rglob("*.py")
        for path in files:
            found.extend(scan(path))
    if found:
        print("\n".join(found))
        return 1
    print("no constructor-default findings")
    return 0

if __name__ == "__main__":
    raise SystemExit(main())

Run it on the production modules, not on the whole repository:

python3 review_client_defaults.py llm/client.py llm/config.py

A non-zero exit is a review comment. It is not an automatic revert of unrelated files. Test fixtures may legitimately contain https://example.invalid. Keep those files out of the scan, or require an explicit allow marker that a second grep can list.

Add one unit test that would have caught the usual agent default:

def test_builder_keeps_prompt_out_of_headers():
    cfg = ClientConfig(
        base_url="https://example.invalid/v1",
        api_key="test-key",
        timeout_s=5.0,
        max_output_tokens=128,
        log_prompts=False,
    )
    req = build_request(
        cfg, messages=[{"role": "user", "content": "secret-phrase"}]
    )
    assert "secret-phrase" not in str(req["headers"])
    assert req["timeout_s"] == 5.0
    assert req["max_output_tokens"] == 128

example.invalid is a reserved name and should not be replaced with a routable host in this test. If a later edit does that, the test has stopped doing its job. The assertion on secret-phrase is there so a logging or header regression fails in CI without any network.

An isolated call, and where a free host fits

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

MonkeyCode provides free model access and a free server option. Those availability claims are the only product facts this workflow uses. Model names, token quotas, hardware, and how long either option lasts are intentionally absent here. They change, and a review article that freezes them becomes stale on purpose. Read the current product page before you rely on either option.

Use that access as the isolated-call target, not as a constructor default.

Put MODEL_BASE_URL and MODEL_API_KEY in the shell or a secret store, never in the repo.
Cap MODEL_MAX_OUTPUT_TOKENS at a small integer, such as 128, so a review run stays bounded.
Keep MODEL_LOG_PROMPTS=0.
Send a synthetic prompt with no customer data, secrets, or internal hostnames.
Record HTTP status, latency, and usage fields the response already includes. One sample is not a benchmark.

export MODEL_BASE_URL="https://your-review-host.example/v1"
export MODEL_API_KEY="from-secret-store"
export MODEL_TIMEOUT_S="20"
export MODEL_MAX_OUTPUT_TOKENS="128"
export MODEL_LOG_PROMPTS="0"
python3 -m llm.smoke --prompt "Reply with the single word pong."

Treat llm.smoke as a proposed command, not as a shipped CLI. Implement it so it exits before any network call when prompt logging is enabled, or when the prompt is longer than a limit you choose, such as 2000 characters. If the free server is shared, assume other tenants exist. Do not upload production traces to inspect what an agent did.

If you want a disposable host for that smoke check, read MonkeyCode's current free-access notes, confirm the live limits yourself, and leave that host out of default arguments.

Comment rubric

Use the same action words in every review so authors learn the bar.

Finding	Action	Reason
Required env vars, no URL literal, timeout set, prompts not logged	Approve the client surface	The route is explicit
URL literal points at a free or shared host	Revert the literal	A default host is a deploy
Tests mock HTTP and never build `ClientConfig`	Block on a builder test	The mock hides the default
No approved non-production key	Merge pure code behind a flag defaulting off	Missing access is not a reason to hit production
Second host on failure	Revert the loop	Fallback changes who receives the prompt
Logs lack status and token counts	Ask for metadata only	Cost review does not need message bodies

Limitations

The scanner matches text. It misses URLs built by concatenation, keys fetched at runtime and logged one function later, and prompts written by a dependency the diff did not vendor. It also false-positives on docstrings and comments. A person still reads the patch.

An isolated call does not establish quality, tail latency, or rate limits. Do not turn one smoke response into a performance claim or a quota claim. Free access is not a capacity plan. If the page you read today does not state a limit, do not invent one in the client, and do not paste a figure from an older post into the default arguments.

Who should skip this

Some teams should not run the isolated-call step at all. The static scan can still apply. The network step cannot.

Policies that forbid third-party model hosts, including free tiers. Point the review at your approved gateway and keep the static scan.
Codebases with regulated or customer content, unless you can guarantee the prompt is synthetic. If you cannot, stop after the unit test.
Reviewers who wanted a model comparison. This workflow does not rank outputs.
Unattended agents with merge rights. The script returns a finding. It does not approve a PR.

The merge you want is smaller than the diff the agent opened. Explicit config, a builder test, and no host that exists only because the constructor required a default.

A Fixture Is a Contract: Reviewing Agent PRs That Call a Free Server Instead

Casey Chen — Thu, 24 Sep 2026 22:16:03 +0000

Replacing a checked-in fixture with a live model call is a dependency change. It is not a better test merely because the endpoint is free. Trust the diff where the input, the response shape, and the failure path stay pinned in git. Revert it where the default job grows a network requirement, a moving model, or a credential.

Agent pull requests often call that swap realism. The summary is not the change. The file list is: a JSON fixture disappears, a client appears, and a URL becomes the oracle.

Classify the call before you comment on style

Treat the patch as four edits, even if the agent shipped them as one feature.

A client, including base URL, headers, timeout, and buffered versus streamed body.
A credential: environment read, default argument, or a literal in an example file.
A test that changed class, from a pure function over bytes to an integration test with a remote side effect.
A fallback policy: offline mock, opt-in live call, or live call on every run.

If a new call site does not fit one row, the pull request is incomplete. Copy this table into the review comment and fill it from the diff, not from the generated summary.

Call site	Default CI	Data allowed	Bucket
Fixture, no socket	On	Synthetic only	Keep, if the schema is asserted
Opt-in sandbox, marker-gated	Off	One redacted sample	Prove with a recorded response
Live call on the unit path	On	Whatever the test builds	Send back
Hosted URL as the application default	On	Real user data	Send back the default

A free server does not move a row from send-back to keep. Cost is not a contract.

Rows that look closed and are not

A call looks closed when the URL comes from an environment variable. It is not closed if the default value is a hosted origin. Missing configuration should fail the process, not dial out.

A marker is also not a control unless the default CI job excludes it. Check the workflow file, not the docstring. The next scan is a proposed command for the branch under review. It was not executed for this article.

git diff -U0 origin/main -- '*.py' '*.yml' '*.toml' '*.env*'

Search that diff for URL literals, timeout=None, and key assignments. The search is a pointer. The table is the decision.

A reduced diff, and the lines that decide it

The next snippet illustrates the patch shape. It is not a capture from a real repository. The host is reserved and intentionally unusable.

import os
import httpx

def complete(prompt: str) -> str:
    url = os.getenv('MODEL_URL', 'https://example.invalid/free')
    resp = httpx.post(url, json={'prompt': prompt}, timeout=None)
    resp.raise_for_status()
    return resp.json()['text']

Four facts fall out of those lines.

The default URL changes behavior for every caller, including tests that used to pass a dict.
timeout=None lets a stalled peer stall the worker. Set a bound from latency you have measured. A number copied from an article, including this one, is not a measurement.
The return type collapses to a string. Callers that used to branch on status now infer outcomes from prose.
The sent payload is not recorded. The diff cannot show whether logs or customer text are included.

A shape that survives review separates transport from policy. This version is a proposal, not a benchmarked client, and it was not executed here.

class Transport:
    def post(self, path: str, *, json: dict, timeout: float) -> dict:
        raise NotImplementedError

def complete(prompt: str, transport: Transport) -> dict:
    body = transport.post('/complete', json={'prompt': prompt}, timeout=5.0)
    required = {'id', 'text', 'finish'}
    if set(body) != required:
        raise ValueError('schema mismatch')
    return body

Tests supply a fake transport. Only a sandbox script binds a real URL. The keys are a local contract you impose on the saved response. They are not a description of any vendor payload. The 5.0 argument exists so the timeout cannot be omitted in this sketch. Replace it with a bound from your own run.

What the default job may keep

Keep evidence that can be re-run without a network.

The request is built from a fixture whose keys and size are known.
Non-success HTTP statuses fail the test instead of becoming empty stand-ins.
The oracle is a schema plus a cassette, not a substring of generated prose.
Any network test carries an explicit marker, and the default job excludes it.
The written summary matches the file list. A claim that the suite stayed hermetic is false if conftest.py session-scopes a live client.

A green run on the agent machine against a hosted sandbox is a sample. It is not a pin. Do not treat that sample as coverage for the default job.

What to send back

Send these back in the same review cycle. A follow-up issue is how a default path ships.

A hosted URL, free or paid, used when configuration is absent.
Any key, token, or cookie in the diff, including files named example.
Fixture deletion without a replacement artifact in the same commit.
Assertions on free-form model wording.
A health or readiness handler that performs generation. Process liveness and model availability are different questions.
A streaming parser with no test for a truncated body. Half an object is not a successful completion.

None of those lines become acceptable because the invoice is zero. Price can explain why someone tried the call. It cannot explain why the suite should depend on it.

What a closed socket must still prove

Prove the boundary with a check that a later generation pass cannot quietly delete. The harness is a proposal. It was not executed for this article. Point it at the repository you are reviewing.

import re
from pathlib import Path

LIVE_CALL = re.compile(r'(httpx\.|requests\.|aiohttp\.|websocket)')
MARKER = 'pytest.mark.network'

def test_default_suite_does_not_call_out(root: Path = Path('tests')):
    offenders = []
    for path in root.rglob('*.py'):
        text = path.read_text(encoding='utf-8')
        if LIVE_CALL.search(text) and MARKER not in text:
            offenders.append(path.as_posix())
    assert offenders == [], offenders

Run the focused file before you debate naming. This command is also unexecuted here.

pytest -q tests/test_offline_boundary.py

A failure means send the call site back. Do not ask for a weaker pattern until the call is gone or excluded from the default job.

Then mint one offline oracle, outside that job.

Write a single synthetic input to testdata/sandbox/case-01.json. No customer text, no internal hostnames, no keys.
Send that case once to a scratch server, with a timeout you selected. Save the raw response beside the input.
Commit the cassette. Do not commit a switch that re-enables the live call in CI.
Load the cassette in a local test. Assert status, the key set, and the absence of fields you did not request.

Cassette checks that stay stable

Prefer fields you control over fields the model phrases.

Status code from the transport, not from a sentence in the body.
Exact key set. Extra keys fail the test.
Declared types for the keys you required.
A maximum byte length, so a sudden verbose shift fails closed.

The live call exists to mint the file. The file exists so the next review can rerun without a network.

A scratch model and a scratch server, then disconnect

Drafting the client and minting one cassette still requires a model and a place to run a single prompt. A pure mock cannot show a real error envelope. That is the only gap this section fills.

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

MonkeyCode, an open-source project, fits here as the scratch environment and nowhere else in the merge. Free model access can draft the client and the failing boundary test. The free server option can replay the sanitized case so the cassette has a real envelope to freeze. After that file is saved, disconnect. The branch you ask others to review should pass with the network disabled.

Both availability claims are product facts supplied for this draft. They are not a token quota, a machine size, a latency target, an uptime figure, or a promise that the offer stays fixed. Read the current project documentation before you plan around them. If those docs and this article disagree, follow the docs.

A hosted scratch server is still another party. Do not upload production logs, customer content, or unpublished secrets in the name of realism. If the agent pointed the application default at that free server, split the change. Keep the cassette and the offline test. Remove the default URL.

Who should skip the recording step

This filter does not score model quality. It does not replace a contract test negotiated with a vendor.

Skip the live recording when:

The build must stay air-gapped, and an opt-in marker would be too easy to flip on the default branch.
The only useful payloads contain regulated or customer data.
You have no schema. A reasonable paragraph is not an oracle.
The branch is a demo with no production caller. Review cost will exceed the diff.
You need capacity or duration numbers. This article does not establish any.

Cassettes rot when a provider changes shape. Refresh them in a reviewed commit after you have read that notice. Do not let a scheduled job overwrite fixtures from a live model. That job is the same dependency again, only slower.

Close on the bucket, not on the price

The review comment should name the bucket. State that the default job dials out, that the URL default should be removed, and that the cassette can stay. Calling the endpoint acceptable because it is free is not a review.

Classify each new call, delete unmarked network use, record one redacted case, and merge only what still passes with the socket closed. For the draft and that single recording run, check MonkeyCode's current documentation for free model access and the free server option, then leave both out of the default configuration.

A Retry Is a Load Multiplier: Reviewing Agent PRs That Wrap Failures

Casey Chen — Wed, 23 Sep 2026 17:32:29 +0000

Retries look like robustness. In an agent-generated pull request they are usually a policy change: more traffic, a longer tail, and a different definition of failure.

Treat a new retry loop the way you would treat a new queue. Count the extra calls. Name the status codes. Decide what happens when the first attempt already committed work.

Agents add retries because the prompt said “make it reliable.” Reliability without a budget is just amplification.

What the diff is actually changing

A typical agent patch wraps an HTTP or RPC call in for attempt in range(3) and sleeps on any exception. The production change is not the sleep. It is the multiplier on every timeout, every 500, and every caller that already fans out.

Three attempts on a handler that calls three dependencies is nine upstream requests in the worst case. Add a 2s backoff and a 10s client timeout, and you have rewritten the SLA without touching an SLO document.

Review that math before you review naming.

Signals that the retry is a product change

New max_retries, attempts, or backoff constants with no owner comment.
A catch of Exception, Error, or a transport timeout around a POST, PATCH, or DELETE.
Sleeps that are linear (attempt * 0.5) with no jitter and no cap.
Retries on 400, 401, 403, or 404 as well as 429 and 503.
No idempotency key, no Retry-After handling, no metric for retry_count.

If two or more of those appear, do not merge on “looks defensive.” Merge only after the contract is explicit.

Trust, revert, prove

Use a three-bucket pass. Do not debate style until the buckets are filled.

Diff signal	Trust	Revert	Prove
Retry only on 429/503 with jitter and a hard cap	Keep the structure	Drop retries on 4xx and on POST without a key	Count upstream calls under injected 503s
Bare `except Exception` plus three attempts	Nothing	The catch-all and the loop	Re-introduce a typed retry allowlist
Timeout lowered and retries raised in the same PR	Neither change	One of the two; they fight	Show p99 vs attempt budget
Idempotency key added with the retry	The header plumbing	Key reused across users or requests	Duplicate POST does not double-apply
“Temporary” retry around a flaky test	Nothing	The retry in test helpers	Fix the race or mark the test

The table is the review. Comments about clean code come after the traffic story is settled.

Review fixture (labeled, unexecuted)

The following is a review fixture, not a production client and not a measured benchmark. It shows the question a reviewer must force the PR to answer: how many times does the upstream run?

# review_fixture_retry.py
from dataclasses import dataclass, field

RETRYABLE = {429, 503}


@dataclass
class FakeUpstream:
    status_sequence: list[int]
    calls: int = 0
    bodies: list[str] = field(default_factory=list)

    def handle(self, method: str, body: str) -> int:
        self.calls += 1
        self.bodies.append(body)
        idx = min(self.calls - 1, len(self.status_sequence) - 1)
        return self.status_sequence[idx]


def agent_style_call(up: FakeUpstream, method: str, body: str, attempts: int = 3) -> int:
    last = 599
    for _ in range(attempts):
        last = up.handle(method, body)
        if last < 500:  # agent often treats all 5xx + some 4xx as “transient”
            return last
    return last


def reviewed_call(
    up: FakeUpstream,
    method: str,
    body: str,
    attempts: int = 3,
    idempotency_key: str | None = None,
) -> int:
    if method in {"POST", "PATCH", "DELETE"} and not idempotency_key:
        return up.handle(method, body)  # no silent multiplier
    last = 599
    for _ in range(attempts):
        last = up.handle(method, body)
        if last not in RETRYABLE:
            return last
    return last

# test_retry_amplification.py
from review_fixture_retry import FakeUpstream, agent_style_call, reviewed_call


def test_agent_retry_multiplies_post_without_key():
    up = FakeUpstream(status_sequence=[503, 503, 200])
    status = agent_style_call(up, "POST", body='{"charge": 1}')
    assert status == 200
    assert up.calls == 3  # three charges if the first 503 already committed


def test_reviewed_post_does_not_retry_without_key():
    up = FakeUpstream(status_sequence=[503, 200])
    status = reviewed_call(up, "POST", body='{"charge": 1}')
    assert status == 503
    assert up.calls == 1


def test_reviewed_get_retries_only_allowlisted_codes():
    up = FakeUpstream(status_sequence=[503, 200])
    status = reviewed_call(up, "GET", body="")
    assert status == 200
    assert up.calls == 2


def test_reviewed_get_does_not_retry_404():
    up = FakeUpstream(status_sequence=[404, 200])
    status = reviewed_call(up, "GET", body="")
    assert status == 404
    assert up.calls == 1

Run the fixture locally:

python -m pytest -q test_retry_amplification.py

The interesting assertion is not status == 200. It is up.calls. Agent tests usually assert the happy path. Reviewer tests assert the multiplier.

A concrete review sequence

Work the PR in this order. Stop when a step fails; later nits are noise.

Name the unit of work. Is the call idempotent? If the handler creates a row, charges a card, or sends a webhook, a retry is a duplicate unless a key or a natural unique index exists.
Write the attempt budget as an inequality. attempts * per_try_timeout * fanout <= caller_timeout. If the PR cannot show this inequality, revert the retry values.
List retryable codes. Allow 429 and 503 by default. Require a comment for 408 or 425. Reject 400–404 and 401/403 unless the PR is talking to a known misbehaving dependency and the ticket says so.
Honor Retry-After. A fixed sleep that ignores the header will stampede when the dependency recovers.
Add jitter and a cap. Linear backoff without jitter synchronizes clients. Uncapped exponential backoff turns a 30s outage into a minutes-long retry storm.
Emit one metric and one log field. retry_count, retry_reason, idempotency_key present or absent. If you cannot graph retries, you cannot operate them.
Fail the first-attempt success that returns slowly. Timeout-then-retry is the double-apply path: the server may have finished after the client gave up.

Suggested review comment (copy, then edit)

Retry changes failure policy, not just resilience.

Please:
- Restrict retries to {429, 503} (and documented others).
- Do not retry POST/PATCH/DELETE without an idempotency key.
- Show attempts * timeout * fanout <= caller timeout.
- Add a test that asserts upstream call count, not only final status.
- Log retry_count and honor Retry-After.

I am -1 on merging the loop until the call-count test exists.

Short comments survive. Essays in GitHub do not.

Where a coding agent is useful — and where it is not

Reproducing the patch against a stub is the whole job. You do not need a large context window to count up.calls.

If you want an isolated checkout to replay the agent diff against the fixture above, MonkeyCode’s free model access and free server option can host that loop. Disclosure: This article was prepared as part of MonkeyCode's product outreach. Use them only as a sandbox for the tests; they do not decide whether a retry is safe.

Do not ask an agent to “add retries until CI is green.” That prompt produces the catch-all loop this review exists to reject. Ask it to generate the call-count test first. Then accept or revert the production loop against that test.

Limitations

This protocol assumes you can stub the dependency. It does not replace load tests for a retry storm at real QPS. It does not tell you the correct attempts for your SLO. Those numbers come from error budgets, not from a model.

Skip this approach when:

The call is a non-idempotent write and you cannot add a key or a uniqueness constraint.
The PR is already past incident response and you need a one-line freeze, not a redesign.
The “retry” lives in a test helper to hide flakes. Delete it. Fix the race.
You cannot observe retry count in production. Operating blind retries is worse than failing once.

A retry is a scheduler. Review it like one.

When an Agent PR Adds Function Calling, Review the Schema Before the Code

Casey Chen — Tue, 22 Sep 2026 15:45:41 +0000

A function-calling diff looks like glue. It is not. A new tool is a new runtime: new inputs, new side effects, a new timeout policy, and a new way to fail in production. Trust existing HTTP helpers only when they stay unchanged. Revert any schema that accepts arbitrary URLs, methods, or opaque blobs. Prove argument validation and error mapping before you merge the happy-path demo.

This is a first-pass review protocol for agent-generated pull requests that wire tools. It is not a vendor bake-off and not a claim about any particular model. The artifact is a decision table, two schemas, a small static check, and tests you can run locally.

What the PR is actually changing

Agents often present tool calling as a thin wrapper. The description says “connect the model to our API.” The diff is larger than that sentence.

Typical files in these PRs:

A JSON Schema or Pydantic model for tool arguments
A dispatcher that maps a tool name to a Python or TypeScript function
An HTTP client call, a database cursor, or a shell-adjacent helper
A README snippet with one successful transcript

The transcript is a demo. The schema is the product. If the schema is unbounded, the model did not get a helper. It got a general-purpose executor inside your process.

Treat the schema as a public interface even when the function is marked internal. Once a tool is registered, every future prompt can reach it.

Trust, revert, test

Use this table on the first review pass. Do not skip a column because the generated tests are green.

Signal in the diff	Trust?	Revert?	Must test
Reused, already-reviewed HTTP client	Yes, if headers, retries, and base URL are untouched	If the PR swaps the client “for simplicity”	Redirect handling and timeout inheritance
Tight enum for `operation` / `resource`	Yes, if it matches an existing service catalog	If the enum is a free-form string	Unknown enum values and aliases
`url: string`, `method: string`, `body: object`	No	Yes. That is a new runtime	Allowlist miss, non-JSON body, 4xx/5xx mapping
Tool description copied from a blog post	No	Comments and descriptions that over-claim	Whether the description matches actual side effects
Generated unit test that only mocks success	Not as proof	The test if it asserts nothing about validation	Invalid args, extra fields, timeout, empty tool name
Logging of full request/response	No	Payload logging that can hold tokens or PII	Log redaction on error paths
Default timeout invented in the wrapper	No	Magic numbers with no owner	Deadline shorter than the upstream SLO

The rule is mechanical. If you cannot name the side effect in one sentence, revert the tool. If you can name it, write a test that fails when the side effect expands.

Illustrative PR: an unbounded “API tool”

The following examples are labeled proposals. They are not from a production repo and they have not been executed in this article.

Proposed generated code (revert the schema):

# proposal: agent-generated tool wrapper — do not merge as-is
from typing import Any
import httpx

TOOL_SCHEMA = {
    "name": "call_internal_api",
    "description": "Call any internal HTTP API the user might need.",
    "parameters": {
        "type": "object",
        "properties": {
            "method": {"type": "string"},
            "url": {"type": "string"},
            "body": {"type": "object"},
        },
        "required": ["method", "url"],
        "additionalProperties": True,
    },
}

async def call_internal_api(method: str, url: str, body: dict[str, Any] | None = None) -> dict[str, Any]:
    async with httpx.AsyncClient(timeout=30.0) as client:
        response = await client.request(method, url, json=body)
        return {"status": response.status_code, "json": response.json()}

Three product decisions hid in twelve lines. Arbitrary method. Arbitrary URL. A 30-second deadline with no owner. additionalProperties: True also means tomorrow’s prompt can smuggle fields the reviewer never saw.

Replacement schema (keep this shape):

ALLOWED_OPS = {
    "list_orders": {"method": "GET", "path": "/v1/orders"},
    "get_order": {"method": "GET", "path": "/v1/orders/{order_id}"},
}

TOOL_SCHEMA = {
    "name": "orders_api",
    "description": "Read orders from the orders service. No writes.",
    "parameters": {
        "type": "object",
        "additionalProperties": False,
        "properties": {
            "operation": {"type": "string", "enum": ["list_orders", "get_order"]},
            "order_id": {"type": "string", "minLength": 1, "maxLength": 64},
        },
        "required": ["operation"],
    },
}

The second schema is smaller on purpose. Tools should shrink capability, not wrap HTTP.

Proposed dispatcher:

from urllib.parse import urljoin

BASE_URL = "https://orders.internal.example"  # existing, already reviewed

async def orders_api(operation: str, order_id: str | None = None) -> dict:
    spec = ALLOWED_OPS.get(operation)
    if spec is None:
        raise ValueError(f"unknown operation: {operation}")
    if "{order_id}" in spec["path"] and not order_id:
        raise ValueError("order_id required")
    path = spec["path"].format(order_id=order_id or "")
    url = urljoin(BASE_URL, path)
    async with httpx.AsyncClient(timeout=5.0) as client:
        response = await client.request(spec["method"], url)
        response.raise_for_status()
        return response.json()

Path construction stays on a constant catalog. The timeout is now an explicit contract of the orders client, not a number the agent liked.

Static check before human review

Run a cheap grep pass on the PR. This is a review aid, not a security scanner and not an exploit kit. It only fails closed on patterns that almost always need a human.

# proposal: local review commands — run in the PR worktree
git diff --name-only origin/main...HEAD

python - <<'PY'
from pathlib import Path
import re, sys
needles = [
    (r'additionalProperties"?\s*:\s*True', "unbounded object"),
    (r'\burl\b.*type\"\s*:\s*\"string\"', "free-form url field"),
    (r'\bmethod\b.*type\"\s*:\s*\"string\"', "free-form method field"),
    (r'subprocess\.(run|Popen)|os\.system|shell=True', "shell side effect"),
    (r'eval\(|exec\(', "dynamic execution"),
]
failed = 0
for path in Path(".").rglob("*") :
    if path.suffix not in {".py", ".ts", ".js", ".json"} or not path.is_file():
        continue
    text = path.read_text(encoding="utf-8", errors="ignore")
    for pat, label in needles:
        if re.search(pat, text):
            print(f"REVIEW {label}: {path}")
            failed += 1
print(f"flags={failed}")
sys.exit(1 if failed else 0)
PY

A flag is not an automatic reject. It is a queue jump. The reviewer still has to decide trust, revert, or prove.

Tests that actually bound the tool

Generated tests often mock httpx and assert status == 200. That tests the mock. Bound the schema and the dispatcher instead.

# proposal: tests for the replacement tool — labeled, not executed here
import pytest

def test_schema_rejects_unknown_fields():
    from orders_tool import TOOL_SCHEMA
    assert TOOL_SCHEMA["parameters"]["additionalProperties"] is False
    assert "url" not in TOOL_SCHEMA["parameters"]["properties"]
    assert TOOL_SCHEMA["parameters"]["properties"]["operation"]["enum"] == [
        "list_orders",
        "get_order",
    ]

@pytest.mark.asyncio
async def test_unknown_operation_does_not_touch_network(monkeypatch):
    import orders_tool

    async def fail_request(*args, **kwargs):
        raise AssertionError("network should not run")

    monkeypatch.setattr(orders_tool.httpx.AsyncClient, "request", fail_request)
    with pytest.raises(ValueError, match="unknown operation"):
        await orders_tool.orders_api(operation="delete_everything")

@pytest.mark.asyncio
async def test_get_order_requires_id():
    with pytest.raises(ValueError, match="order_id required"):
        await orders_tool.orders_api(operation="get_order")

Add one integration test against a recorded fixture or a fake HTTP server if the client is new. Do not accept a screenshot of a chat session as coverage.

Minimum suite for any tool-adding PR:

Unknown tool name or operation fails before I/O.
Extra JSON fields are rejected, not silently stored.
Required identifiers fail closed.
Non-2xx responses map to typed errors, not raw strings dumped into the model context.
Timeouts are inherited from the existing client or documented as a new SLO.

If the agent also added retries, caching, or a catch-all except Exception, those are separate policy changes. Review them as their own diffs. Do not let a tool PR smuggle three policy changes under one “wire the model” title.

Comments are not contracts

Agent PRs in this cluster love comments. “Safe internal-only helper.” “The model will only call this with valid URLs.” “Timeout is fine for now.”

A comment is not an invariant. If the safety property matters, it belongs in the schema enum, the allowlist, or a test. If it cannot be expressed there, the tool is not ready.

Watch for these substitutions:

A docstring that lists allowed hosts while the code takes any str
# type: ignore on the dispatcher return value
README examples that use production URLs and real-looking tokens
“Temporary” print(response.text) left on the error path

Revert the comment if it is the only control. Keep the comment if it points at the allowlist that actually enforces the claim.

Where a local loop helps

The review above is a human protocol. You can execute the static check and the tests in any clone. Some teams want a disposable environment so the agent can regenerate the schema while the checklist stays fixed.

Disclosure: This article was prepared as part of MonkeyCode's product outreach. MonkeyCode is an open-source project with free model access and a free server option. That is relevant here only as a place to run the commands and tests against a throwaway branch. It does not replace the allowlist, and it does not make an unbounded url: string tool acceptable.

If you use such an environment, pin the review artifacts in the repo: the decision table, the schema file, and the tests. Do not pin a chat transcript.

Limitations

This protocol does not measure model quality. It does not estimate token cost. It does not certify that a tool is safe on a public network. It will miss semantic bugs where the schema is tight and the implementation still writes when the description says read.

It also assumes you have an existing service catalog. If the agent is inventing both the backend and the tool, you are not reviewing a wrapper. You are reviewing a new product, and this checklist is too small.

False confidence is the main failure mode. A green static check plus three unit tests can still hide a tool that returns secret-bearing error bodies to the model. Read the error mapper.

Who should not use this approach

Skip this workflow if you need a full threat model for tools that reach the public internet, local files, or shell. Skip it if your org has no owner for timeouts and retries. Skip it if the PR’s only evidence is a demo GIF.

Do not use the protocol as a reason to rubber-stamp generated tests. Do not widen a schema “so the model has more freedom” after the first incident. Freedom in a tool schema is production scope.

Merge bar

Merge the PR when all of the following hold:

The schema enums match a catalog you already operate
additionalProperties is false on tool arguments
There is no free-form URL, method, or shell string
Unknown operations fail before network or disk
Error paths are typed and redacted
Timeouts have an owner

Otherwise revert the tool surface and keep any unrelated refactors in a separate change. The code the agent wrote around the schema can be fine. The schema is the review.

If you need a scratch server to iterate the tests above, MonkeyCode’s free model access and free server option are enough to host that loop. The merge bar does not change if you run the same checks on your laptop.

A Catch-All Is a Policy Change: Reviewing Agent PRs That Swallow Exceptions

Casey Chen — Mon, 21 Sep 2026 14:05:30 +0000

Agent-generated pull requests often turn failures into successful returns. That is not a style nit. It is an API change.

A catch that logs and continues, or an except Exception: return None, rewrites every caller. Reviewers should treat the handler as a product decision: trust it only at an explicit boundary, revert it when the type is generic, and prove the rest with tests that fail closed.

The pattern in the diff

Agents are optimized to make the branch compile. A timeout, a missing key, or a type mismatch gets wrapped so the happy path type-checks. The suite stays green. Production then loses the failure mode.

# Typical agent-generated handler
def fetch_invoice(client, invoice_id: str) -> dict | None:
    try:
        payload = client.get(f"/invoices/{invoice_id}")
        return payload.json()
    except Exception as exc:
        print(f"failed to fetch invoice: {exc}")
        return None

Short. Compiles. Wrong.

Callers can no longer tell "no invoice" from "timeout" from "auth expired" from "JSON is not an object". An HTTP 500 becomes None. The next agent PR adds if data: and skips billing. Two diffs later, silence is the system.

A labeled alternative for the same function:

class InvoiceFetchError(Exception):
    def __init__(self, invoice_id: str, cause: BaseException):
        super().__init__(f"invoice {invoice_id}: {cause!r}")
        self.invoice_id = invoice_id
        self.cause = cause

def fetch_invoice(client, invoice_id: str) -> dict:
    try:
        response = client.get(f"/invoices/{invoice_id}", timeout=5.0)
        response.raise_for_status()
        payload = response.json()
    except (OSError, TimeoutError, ValueError, RequestException) as exc:
        raise InvoiceFetchError(invoice_id, exc) from exc

    if not isinstance(payload, dict) or "id" not in payload:
        raise InvoiceFetchError(invoice_id, ValueError("invalid invoice payload"))
    return payload

The second version still handles errors. It does not hide them. That distinction is the whole review.

Why the model emits this

The training mix rewards snippets that "keep going". Catch-all examples are common in tutorials. Typed failure is rarer in those snippets, so the model copies the tutorial.

Three pressures show up repeatedly in agent diffs:

Green path bias. A raised exception fails the demo. A swallowed one does not.
Type-narrowing by deletion. Returning None is cheaper for the model than introducing an error type and updating callers.
Log-as-proof. print or console.error is treated as handling. It is only I/O.

None of those pressures are malicious. They are still a contract change. Review the contract, not the model's intent.

Trust, revert, or prove

Run a three-bucket pass before reading the feature description. Do not negotiate with the rest of the diff until every new handler is labeled.

Trust

Trust a handler only when every item below holds:

The except / catch list is closed and named. No Exception, no error, no any.
Failure becomes a typed error or a result object callers already understand.
Side effects exist: a metric, a structured log without secrets, a span status.
The handler sits at a documented boundary: HTTP adapter, queue consumer, process edge, or FFI.
Tests show both the wrapped cause and the outward type.

If any item is missing, it is not trust. Move the line to another bucket.

Revert

Revert immediately when the PR:

Adds except Exception, catch (e), catch (...), or empty catch {}.
Returns a sentinel (None, {}, [], 0, false, HTTP 200 plus "ok") after a failure.
Logs a request body, Authorization header, cookie, or token.
Retries inside the same catch. That is a new call graph. Do not review it as "error handling".
Changes a function that used to raise into one that returns optional, with no caller and docs update.

Do not fix-forward a catch-all in the same review. The blast radius is the call graph, not the line.

Prove

Prove the middle cases. Mapping FileNotFoundError to 404, mapping a unique-constraint failure to 409, or isolating a third-party SDK that raises RuntimeError for business cases all belong here.

Require a test that failed on the old code and still fails if the handler is widened again. One mapping, one unexpected-error case. Not a narrative comment.

Decision table

Diff signal	Bucket	Reviewer action
`except Exception` / `catch (e)` / empty `catch`	Revert	Restore raise; demand a named type
`except SpecificError` → typed wrapper + `from exc`	Trust if tests exist	Check log fields and caller updates
`return None` / `return {}` on failure	Revert	A sentinel is a new API
Map DB unique violation → 409	Prove	Test 409 and test unexpected DB errors still 500
`except: log and continue` in a loop	Revert	Partial success must appear in the return type
Catch at HTTP edge, re-raise as Problem+JSON	Prove	Snapshot status and type URI; do not leak stacks
Catch in a helper used by many call sites	Revert	Policy does not belong in a helper

This table is the review artifact. Apply it before the rest of the PR.

A reproducible test plan

Do not accept "we logged it". Logs are not assertions. The suite below is a proposed gate, not an execution report.

# test_invoice_fetch_errors.py
import pytest
from unittest.mock import Mock
from requests import Timeout, HTTPError

from invoices import fetch_invoice, InvoiceFetchError

def test_timeout_does_not_become_none():
    client = Mock()
    client.get.side_effect = Timeout("read timed out")
    with pytest.raises(InvoiceFetchError) as ei:
        fetch_invoice(client, "inv_1")
    assert ei.value.invoice_id == "inv_1"
    assert isinstance(ei.value.cause, Timeout)

def test_http_500_does_not_become_empty_dict():
    response = Mock()
    response.raise_for_status.side_effect = HTTPError("500")
    client = Mock()
    client.get.return_value = response
    with pytest.raises(InvoiceFetchError):
        fetch_invoice(client, "inv_2")

def test_invalid_json_object_is_not_success():
    response = Mock()
    response.raise_for_status.return_value = None
    response.json.return_value = ["not", "an", "object"]
    client = Mock()
    client.get.return_value = response
    with pytest.raises(InvoiceFetchError):
        fetch_invoice(client, "inv_3")

Run them against the merge base, not as a local courtesy:

git fetch origin main
git diff --name-only origin/main...HEAD
pytest -q test_invoice_fetch_errors.py

If the agent also edited tests to expect None, revert those tests. A test that documents silence is not coverage. It is a permission slip.

TypeScript carries the same smell on full-stack agent PRs:

// Agent version
export async function fetchInvoice(id: string): Promise<Invoice | null> {
  try {
    const res = await fetch(`/invoices/${id}`);
    return await res.json();
  } catch (e) {
    console.error(e);
    return null;
  }
}

// Review target
export class InvoiceFetchError extends Error {
  constructor(readonly invoiceId: string, readonly cause: unknown) {
    super(`invoice ${invoiceId} fetch failed`);
  }
}

export async function fetchInvoice(id: string): Promise<Invoice> {
  let res: Response;
  try {
    res = await fetch(`/invoices/${id}`);
  } catch (cause) {
    throw new InvoiceFetchError(id, cause);
  }
  if (!res.ok) {
    throw new InvoiceFetchError(id, new Error(`http ${res.status}`));
  }
  const payload: unknown = await res.json();
  if (!isInvoice(payload)) {
    throw new InvoiceFetchError(id, new Error("invalid payload"));
  }
  return payload;
}

null is a value. An exception is control flow. Mixing them is how a billing job skips invoices without paging anyone.

Commands that rank the smell

These are heuristics. They rank files. The table still decides.

# Python catch-alls in this branch
git diff -U0 origin/main...HEAD | rg -n "except Exception|except:|except BaseException"

# TypeScript / JavaScript
git diff -U0 origin/main...HEAD | rg -n "catch \(e\)|catch \(err\)|catch \{"

# Sentinels returned near a handler (rough)
git diff -U0 origin/main...HEAD | rg -n -A2 "catch|except" | rg "return (None|null|\[\]|\{\}|false|0)"

# Added lines only
git diff origin/main...HEAD | rg '^\+' | rg "except Exception|catch \(e\)"

Second pass: call sites. If fetch_invoice used to raise, list every caller. Zero try around those callers means the PR just changed them all to implicit None handling.

rg -n "fetch_invoice\(" --type py
rg -n "fetchInvoice\(" --type ts

Record the count. If the handler widened and the caller count is greater than one, the PR is not local. Split it or revert it.

HTTP mapping is still a policy

Agents like this adapter:

def to_http(exc: BaseException) -> tuple[int, dict]:
    try:
        raise exc
    except InvoiceFetchError:
        return 404, {"error": "not_found"}
    except Exception:
        return 200, {"error": "ignored"}

The second branch is a product incident. Unexpected errors belong on 5xx, with a stable error code and no stack in the body. 404 is for a proven missing resource, not for a timeout.

Proposed mapping tests:

def test_unknown_error_is_500_not_200():
    status, body = to_http(RuntimeError("disk"))
    assert status == 500
    assert body["error"] != "ignored"

def test_missing_invoice_is_404():
    status, _ = to_http(InvoiceFetchError("inv_9", FileNotFoundError()))
    assert status == 404

If the PR adds the mapper and not these two tests, it is still prove, not trust.

Where a local review assistant fits

A rubric without a second pair of eyes drifts. Some teams run a constrained pass over the diff and allow only three labels: TRUST, REVERT, PROVE.

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

MonkeyCode is an open-source coding assistant. Operator-supplied availability includes free model access and a free server option, which is enough to run that labeled pass on the branch. That is the only reason it belongs in this workflow: it can apply the table. It cannot certify that a catch block is correct. If the assistant and the table disagree, keep the table.

Redact .env, tokens, and production payloads before any assistant sees the patch.

git diff origin/main...HEAD > /tmp/pr.diff
# Review /tmp/pr.diff against the decision table.
# Allowed labels: TRUST, REVERT, PROVE.
# Require a named exception type or a revert.

Limitations

This protocol does not ban try. Isolation at process edges is real work. Queue consumers, HTTP adapters, and batch jobs need a poison-message policy.

It does not replace on-call data. A typed error with no metric is still silent, just louder in the process dump.

It does not decide product questions such as "is a missing invoice a 404 or a 204". That is a spec. The agent PR is not a spec.

Retries, caches, fallback providers, and empty test assertions are separate reviews. Do not fold them into this pass.

Who should not use this approach

Do not use it to reject every error-mapping PR. Named mappings with tests belong in prove.
Do not apply it to generated protobuf or OpenAPI stubs you do not own. Review the wrapper, not the generated catch.
Do not use it as a substitute for a linter if CI already fails on except Exception. The table is for repos where those lines still land.
Do not run an assistant review on diffs that contain credentials, customer data, or proprietary weights.

If the repo already has an exception budget, a typed Result convention, and a forbid-catch-all lint, this article is redundant. Keep the lint.

Close the loop on the contract

The interesting part of an agent PR is rarely the feature. It is the control flow the model invented so the feature would compile.

A catch-all is a policy change. Revert the generic ones. Prove the specific ones. Trust nothing that returns success after a failure. The merge decision stays with the reviewer who can name the outward type.

A Cache Is a Second Database: Reviewing Agent PRs That Memoize Too Early

Casey Chen — Sun, 20 Sep 2026 11:59:44 +0000

Agent pull requests that add caching usually look like performance work. They are almost always consistency work. Until the key, lifetime, invalidation path, and stampede behavior are specified, treat an unsolicited cache as a second database that the rest of the system does not know about.

This review walks a constructed agent PR. The diff is typical: a hot read path, a green unit suite, and a memoization wrapper presented as a free speedup. The merge question is not “is Redis faster than Postgres?” The merge question is whether the PR created a new source of truth.

The PR under review

Label: constructed example, not a production incident. An agent was asked to “speed up invoice reads.” It added an in-process memo plus a Redis overlay and left the write path untouched.

# invoice_service.py  (agent diff, simplified)
import json
from functools import lru_cache

TTL_SECONDS = 3600

@lru_cache(maxsize=4096)
def _local(invoice_id: str) -> dict:
    return repo.fetch_invoice(invoice_id)

async def get_invoice(invoice_id: str) -> dict:
    cached = await redis.get(f"invoice:{invoice_id}")
    if cached:
        return json.loads(cached)

    invoice = _local(invoice_id)
    await redis.set(
        f"invoice:{invoice_id}",
        json.dumps(invoice, default=str),
        ex=TTL_SECONDS,
    )
    return invoice

async def void_invoice(invoice_id: str) -> None:
    await repo.mark_void(invoice_id)
    # agent comment: cache will expire in an hour

The unit tests still pass. They construct the service with a fake repo, call get_invoice once, and assert on field names. No write is issued. No second reader is issued. No clock is advanced.

# test_invoice_service.py  (also agent-authored)
def test_get_invoice_returns_number(service):
    invoice = asyncio.run(service.get_invoice("inv_1"))
    assert invoice["number"].startswith("INV-")

That test cannot see stale data. It never creates any.

What the diff is actually changing

Three stores now exist for one invoice:

The relational row in repo.
The process-local lru_cache on _local.
The Redis string at invoice:{id}.

Reads prefer Redis, then the process cache, then the database. Writes touch only the database. After void_invoice, a reader in the same process can still return the cached dict for up to an hour. A reader in another process can do the same from Redis. A deploy that restarts one replica clears only that replica’s LRU. The other replicas keep serving the voided invoice.

This is not an optimization sitting on top of an unchanged API. It is a new consistency model with no name, no metric, and no owner.

Trust, revert, test

Trust these parts, if they exist

A measured hot path. A profile, slow query log, or trace that names fetch_invoice is a real constraint. A comment that says “this might be slow” is not.
Tenant-aware key prefixes, if they are already the house style (tenant:{tid}:invoice:{id}).
Explicit serialization, if the payload is a documented DTO rather than json.dumps(obj, default=str).

None of those, by themselves, make the cache mergeable.

Revert these parts by default

Process-local lru_cache on anything that can be mutated by another request, worker, or replica.
Redis writes with a TTL and no delete on the mutating path.
default=str JSON. It turns datetimes and Decimals into strings that later reads will not round-trip.
Keys that omit tenant, environment, or schema version.
Comments that defer correctness to expiry. A TTL is a bound on staleness, not an invalidation protocol.

If those lines are the entire “performance” change, the honest review is a revert with a request for a cache design, not a nit about naming.

Test these behaviors before merge

Green field-name tests are not enough. The smallest honest suite is a state machine: read, mutate, read again, from more than one client.

# test_invoice_cache_contract.py
import asyncio
import json
import pytest

@pytest.mark.asyncio
async def test_void_is_visible_to_second_reader(service, redis, repo):
    first = await service.get_invoice("inv_1")
    assert first["status"] == "open"

    await service.void_invoice("inv_1")

    # Direct DB truth.
    assert repo.fetch_invoice("inv_1")["status"] == "void"

    # Cached read must not contradict DB.
    second = await service.get_invoice("inv_1")
    assert second["status"] == "void"

    raw = await redis.get("invoice:inv_1")
    if raw is not None:
        assert json.loads(raw)["status"] == "void"


@pytest.mark.asyncio
async def test_two_readers_do_not_stampede_writes(service, redis, monkeypatch):
    calls = {"n": 0}
    original = service.repo.fetch_invoice

    def counted(invoice_id):
        calls["n"] += 1
        return original(invoice_id)

    monkeypatch.setattr(service.repo, "fetch_invoice", counted)
    await redis.delete("invoice:inv_1")

    await asyncio.gather(
        service.get_invoice("inv_1"),
        service.get_invoice("inv_1"),
        service.get_invoice("inv_1"),
    )
    # A correct fill uses a lock, singleflight, or request coalescing.
    assert calls["n"] == 1

The second test will fail on the agent diff. That failure is the review artifact. Do not “fix” it by weakening the assertion to calls["n"] >= 1.

A compact review table

Signal in the PR	Meaning	Review action
Cache added, write path unchanged	Second database with no replication story	Revert cache, or add invalidation + tests
TTL used as the only invalidation	Staleness is unbounded by product rules, bounded by a constant	Require a documented stale budget
`lru_cache` on a request-level helper	Replica-local memory, not a shared cache	Revert or move to explicit request memo
Unit test hits a cold cache once	Suite cannot observe stale reads	Block until a mutate-then-read test exists
Key is `invoice:{id}`	Cross-tenant collision risk	Require the same prefix rules as the DB
`json.dumps(..., default=str)`	Irreversible type loss	Require a schema or codec
Comment promises a follow-up PR	The follow-up is not in this diff	Treat as absent

Commands that make the review cheap

Inspect whether the agent touched every mutating path, not just the getter.

git fetch origin pull/1842/head:pr-1842
git checkout pr-1842

git diff main...HEAD -- '*.py' | rg -n "lru_cache|redis\.(get|set|delete)|invalidate|void_|update_|mark_"

rg -n "get_invoice|void_invoice|pay_invoice|refund_invoice" -g '*.py'

If set appears and delete does not, the write path is unfinished. If void_invoice exists in the service and not in the diff, the agent optimized a read and ignored the business event that makes the read wrong.

Reproduce a stale read without a full cluster:

pytest -q test_invoice_cache_contract.py -k void_is_visible

If that test does not exist yet, the review is incomplete. Adding the test is cheaper than arguing about intended TTL behavior in comments.

Where a disposable model and server help

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

The review above does not need a model. It needs a mutating test and a second reader. Models are useful for one narrow job: enumerating invalidation paths the agent missed. That list is a candidate checklist, not evidence that the PR is safe.

A practical split:

Ask a model, on a throwaway branch, to list every function that can change invoice state.
Diff that list against the PR with rg, not against the model’s confidence.
Run the contract tests on a dedicated process so lru_cache and Redis are shared the way they will be in staging.

MonkeyCode’s free model access is enough for step 1 if you already have the repo context. The free server option is useful for step 3 when the cache only misbehaves under concurrent clients, which a single local pytest process can hide. Neither step replaces the table above.

Example prompt, treated as a draft checklist only:

Given these files, list mutating operations on Invoice.
For each, name the cache key that must be deleted or overwritten.
Do not propose a new cache. Do not claim the current cache is correct.

Keep the model output in the review notes. Do not paste it into the PR as documentation of runtime behavior.

Failure analysis: why this class of PR ships

Agents are rewarded for making a slow function return faster on the next call. Caches do that on the happy path. Reviewers are rewarded for not blocking a green suite. The combination produces optimization theater: latency charts that improve in a demo, and a voided invoice that remains payable for 59 minutes.

Two mechanical tells show up repeatedly.

Tell A: the cache is introduced in the same patch as the measurement. There is no before/after harness, only a comment. Without a harness, there is no way to know whether the slowness was the query, the N+1 caller, or a missing index.

Tell B: invalidation is described in prose. “We can add delete later” is a product fork. Later means another PR, another reviewer, and a window where production already has the second database.

If either tell is present, do not negotiate on TTL values. Revert the cache lines and keep any legitimate query fix.

A minimal mergeable shape

If the team actually wants a cache, the PR has to name the store and the invalidation. One acceptable skeleton:

CACHE_KEY = "v1:invoice:{invoice_id}"

async def get_invoice(invoice_id: str) -> dict:
    key = CACHE_KEY.format(invoice_id=invoice_id)
    raw = await redis.get(key)
    if raw is not None:
        return InvoiceDTO.model_validate_json(raw).model_dump()

    invoice = repo.fetch_invoice(invoice_id)
    payload = InvoiceDTO.model_validate(invoice).model_dump_json()
    await redis.set(key, payload, ex=60)  # stale budget, not a design
    return json.loads(payload)

async def void_invoice(invoice_id: str) -> None:
    await repo.mark_void(invoice_id)
    await redis.delete(CACHE_KEY.format(invoice_id=invoice_id))

Still missing from that skeleton: singleflight, stampede tests, tenant prefixes, and what happens when delete fails. It is mergeable only relative to the original agent diff, and only with the contract tests above. It is not a general cache framework.

Limitations

This review pattern assumes a single object cache keyed by ID. It does not cover HTTP Cache-Control, CDN layers, ORM identity maps, or materialized views. Those have different invalidation graphs.

It also assumes you can run a second reader against the same Redis and the same process memory. If the agent cached inside a serverless isolate that dies after each request, lru_cache may be accidentally harmless and Redis may still be wrong. Harmless in one runtime is not a reason to keep the decorator.

Do not use the model-enumeration step on repositories that contain secrets in fixtures. Cache keys and TTL constants are enough context.

Who should not use this approach

Teams with a standing cache standard and a shared helper. Review against that helper, not against this article’s skeleton.
Changes that are actually query plans or indexes. If EXPLAIN already shows the win, adding Redis is a new product.
Reviewers looking for a reason to auto-approve “performance” PRs. The point of the table is to block incomplete stores, not to rubber-stamp them when a TTL is present.
Anyone treating a model-generated invalidation list as complete. Models omit event handlers, admin scripts, and out-of-band SQL.

A cache is a second database. Agent PRs that add one without a write protocol are not unfinished optimizations. They are unfinished data models. Revert the store, keep the measurement, and do not merge a TTL as if it were a spec.

If you need a throwaway box to run the two-reader contract tests away from your laptop, MonkeyCode’s free server option is one place to park that harness while you decide whether the cache should exist at all.

A Probability Is Not a Gate: Reviewing Agent PRs That Put LLMs in CI

Casey Chen — Sat, 19 Sep 2026 10:06:03 +0000

An LLM step in CI is not extra review coverage. It is a new production dependency that can veto a merge with a different answer on every run. Agent pull requests that add that step should be triaged like any other hidden scheduler: keep a testable client, revert the gate, and prove the contract against a replayable endpoint.

This walkthrough uses an illustrative agent PR. It does not measure model quality. It measures something narrower. Non-deterministic jobs do not belong on the critical path of main.

The PR under review

The agent titled the change "add AI review to pull requests." The diff is small. The blast radius is not.

Typical files:

.github/workflows/ai-review.yml
scripts/ai_review.py
a prompt file, often prompts/pr_review.md
a new secret name in the README, and sometimes in the workflow env block

The workflow usually looks like this.

# illustrative — do not copy into a required check
name: ai-review
on: [pull_request]
jobs:
  review:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - uses: actions/setup-python@v5
        with:
          python-version: "3.12"
      - run: pip install httpx
      - env:
          MODEL_API_KEY: ${{ secrets.MODEL_API_KEY }}
          MODEL_BASE_URL: ${{ vars.MODEL_BASE_URL }}
        run: python scripts/ai_review.py

Three properties turn a comment helper into a merge gate:

The job is required, or it will be made required "after we see it work."
A non-zero exit fails the pull request.
Model output is parsed for words such as BLOCK, FAIL, or security issue.

If any of those three is present, the PR did not add coverage. It added a lottery.

Agents add this pattern because the task prompt asked for quality, not because CI gained a deterministic oracle. A linter fails on the same AST every time. A model does not. Treating the second like the first is the defect.

What the script usually does

The companion script is also small. That is why it survives a skim.

# illustrative agent output — scripts/ai_review.py
# Example only. Not a recommended implementation.
import os, re, subprocess, sys
from pathlib import Path
import httpx

diff = subprocess.check_output(
    ["git", "diff", "origin/main...HEAD"], text=True
)
prompt = Path("prompts/pr_review.md").read_text() + "\n\n" + diff
url = os.environ.get("MODEL_BASE_URL", "https://api.example.invalid/v1/chat")
r = httpx.post(
    url,
    headers={"Authorization": f"Bearer {os.environ['MODEL_API_KEY']}"},
    json={"messages": [{"role": "user", "content": prompt}]},
)
text = r.json()["choices"][0]["message"]["content"]
if re.search(r"BLOCK|FAIL|CRITICAL", text, re.I):
    print(text)
    sys.exit(1)
print(text)

Concrete problems in that block:

No timeout, size cap, or retry budget. A hung vendor becomes a hung required check.
The full diff is uploaded. .env, lockfile churn, and private keys ride along unless something else strips them.
Free-form prose is mapped to sys.exit(1). That mapping is the gate.
A hardcoded host appears as a default. Environment injection becomes optional, then forgotten.
There is no denylist test. Green CI means "the model answered," not "secrets stayed local."

Trust stops at the first branch that interprets natural language as a boolean.

What to trust

Trust only what you can replay without the model.

YAML shape: checkout, language setup, which files are sent
Secret names (not values) and whether those names are required
HTTP client construction: timeouts, headers, maximum body size
Redaction: whether the script drops .env, key files, and Authorization lines before upload
Exit-code policy: whether the process always exits 0 after posting a comment

A timeout value is evidence. A prompt file is not. One is a contract. The other is input to a function you do not control.

What to revert

Revert the merge gate. Keep the client if it is actually a client.

Revert immediately when the script:

Fails the job on phrases in model output
Uploads the full diff, including paths matching **/.env*, **/*secret*, or **/id_rsa
Hard-codes a vendor URL, model identifier, or token budget
Posts unreviewed comments as GITHUB_TOKEN with pull-requests: write
Leaves continue-on-error at the default false on a model call
Reuses the production inference secret in CI

A comment a human may ignore is a tool. A red X a human must override is an owner. Agents rarely add the owner. They add the X.

Hard-coded hosts deserve the same revert as hard-coded credentials. Both prevent the next reviewer from pointing the client at a fixture. Both also couple main to one vendor's incident calendar.

What to test

Test the client as a library. Do not test "the model is smart."

Minimum assertions:

Diff collection respects a path denylist.
Request bodies are capped (for example 64 KiB). A timeout is set. Retries cannot amplify into a storm.
The parser never maps unknown text to fail.
The CI job sets continue-on-error: true, or the job is not a required status check.
Replay against a fixture endpoint succeeds or fails deterministically for canned payloads.

Item 5 is where a free model endpoint is useful. It is a network fixture. It is not a judge. A 200 proves the client can speak HTTP. It does not prove the prompt should block a merge.

Artifact: a triage harness

The following is a proposed local tool. It is not production CI. Run it on the PR branch before arguing about prompt wording.

Decision table

Signal in the diff	Trust	Revert	Test
HTTP client with timeout and size cap	Client code	—	Unit test the cap
Required GitHub check on model output	—	The check	Assert the job is optional
Prompt file with no input bound	—	Unbounded prompt	Length / cap test
Secret used in a required job	Name only	Coupling to prod key	Secret scanning
`continue-on-error: true` plus comment only	Comment path	—	Mock GitHub API
Hard-coded vendor host	—	Host and model id	Inject `MODEL_BASE_URL`
Diff sent without a denylist	—	Upload path	Fixture repo that contains `.env`

The table is the review. The script below only automates the first pass so the comment can be short.

Commands

# illustrative review commands — run on the PR branch
git fetch origin pull/N/head:pr-n
git switch pr-n

# 1. Find CI jobs that invoke the new script
rg -n "ai_review|MODEL_API_KEY|openai|anthropic|complete\(" \
  .github/workflows scripts

# 2. Fail the review if the workflow can veto merge on model text
python3 tools/review_llm_ci.py --workflow .github/workflows/ai-review.yml

`tools/review_llm_ci.py`

#!/usr/bin/env python3
"""Classify an agent-added 'AI review' workflow. Proposed review aid."""
from __future__ import annotations

import argparse
import os
import re
import sys
from pathlib import Path

try:
    import yaml
except ImportError:
    print("pip install pyyaml", file=sys.stderr)
    sys.exit(2)

FAIL_WORDS = re.compile(r"\b(block|fail|error|vulnerable|critical)\b", re.I)


def load_workflow(path: Path) -> dict:
    data = yaml.safe_load(path.read_text())
    if not isinstance(data, dict):
        raise ValueError(f"not a mapping: {path}")
    return data


def jobs(doc: dict) -> dict:
    raw = doc.get("jobs") or {}
    return raw if isinstance(raw, dict) else {}


def step_runs(job: dict) -> str:
    chunks = []
    for step in job.get("steps") or []:
        if isinstance(step, dict) and isinstance(step.get("run"), str):
            chunks.append(step["run"])
    return "\n".join(chunks)


def is_required_model_gate(job: dict) -> bool:
    if job.get("continue-on-error") is True:
        return False
    blob = yaml.safe_dump(job)
    return bool(re.search(r"MODEL_|API_KEY|base_url|openai|anthropic", blob, re.I))


def classify(doc: dict) -> list[str]:
    findings = []
    for name, job in jobs(doc).items():
        if not isinstance(job, dict):
            continue
        dumped = yaml.safe_dump(job)
        if is_required_model_gate(job):
            findings.append(
                f"REVERT gate: job {name!r} calls a model without continue-on-error"
            )
        if "GITHUB_TOKEN" in dumped and "pull-request" in dumped.lower():
            findings.append(f"TEST comment auth: job {name!r} can post on the PR")
        if step_runs(job) and "MODEL_BASE_URL" not in dumped:
            findings.append(f"REVERT missing MODEL_BASE_URL injection in job {name!r}")
    return findings


def scan_script(path: Path) -> list[str]:
    if not path.exists():
        return [f"TEST missing script: {path}"]
    text = path.read_text(errors="replace")
    findings = []
    if "timeout" not in text.lower():
        findings.append(f"REVERT missing timeout in {path}")
    if FAIL_WORDS.search(text) and "sys.exit(1)" in text:
        findings.append(f"REVERT text-to-exit mapping in {path}")
    if not any(s in text for s in (".env", "denylist", "deny_list", "skip_files")):
        findings.append(f"REVERT no path denylist in {path}")
    return findings


def replay_healthcheck() -> list[str]:
    """Optional: prove client config can reach a fixture. Not a quality test."""
    base = os.environ.get("REVIEW_REPLAY_URL")
    if not base:
        return ["TRUST skip replay: set REVIEW_REPLAY_URL to exercise the client"]
    try:
        import urllib.request

        req = urllib.request.Request(base, method="GET")
        with urllib.request.urlopen(req, timeout=5) as resp:
            if 200 <= resp.status < 500:
                return [f"TEST replay reached {base} status={resp.status}"]
            return [f"REVERT replay unexpected status {resp.status} from {base}"]
    except Exception as exc:
        return [f"TEST replay error (fixture, not product): {exc!r}"]


def main() -> int:
    parser = argparse.ArgumentParser()
    parser.add_argument("--workflow", type=Path, required=True)
    parser.add_argument("--script", type=Path, default=Path("scripts/ai_review.py"))
    args = parser.parse_args()
    findings = []
    findings.extend(classify(load_workflow(args.workflow)))
    findings.extend(scan_script(args.script))
    findings.extend(replay_healthcheck())
    for line in findings:
        print(line)
    revert = sum(1 for item in findings if item.startswith("REVERT"))
    print(f"summary: {revert} revert-class finding(s), {len(findings)} total")
    return 1 if revert else 0


if __name__ == "__main__":
    sys.exit(main())

Exit code 1 means "do not merge as-is." Exit code 0 means "no revert-class signal." It does not mean the model is correct. Phrase lists go stale. Agents rename BLOCK to needs_changes. The human still reads the parser.

Replay fixture, not a second reviewer

Reviewers still need somewhere to point MODEL_BASE_URL when the client is worth keeping. A dead vendor URL blocks that proof. The harness treats replay as optional on purpose: classification of the gate should work on an airplane. Construction of the client should not require a paid account.

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

MonkeyCode offers free model access and a free server option. For this protocol, that pair is only a replay fixture. Export REVIEW_REPLAY_URL and MODEL_BASE_URL so the client can be constructed, timed out, and failed on purpose. Do not attach that fixture as a required GitHub check. Doing so would recreate the defect under review. If the client cannot accept a base URL from the environment, that absence is a revert by itself.

Limitations

The harness does not evaluate prompt quality, jailbreak resistance, or suggestion accuracy.
A 200 from a fixture is not a latency, quota, or availability SLO.
Regex over workflow YAML misses composite actions and reusable workflows in other repositories.
Exit-word lists (BLOCK, FAIL) rot. Re-read the parser on every PR.
Free endpoints change. Pin nothing in CI that you cannot replace with one environment variable.
Product features that are inference need contract tests and capacity planning. They are out of scope here. A merge veto on prose is in scope.

Who should not use this approach

Teams whose CI has no egress. Skip replay. Keep the YAML classification.
Regulated repositories that cannot send diffs or prompts to any third-party model, free or not. Revert the upload path regardless of fixture.
Changes that only add a local, deterministic linter. Those are not this PR.
Anyone hoping an LLM job will replace human review of auth, migrations, or lockfiles.

Review comment to paste

When the agent PR matches the table, a short review comment is enough:

This job makes merge depend on non-deterministic model text.
Please:
1. Keep the HTTP client if it has timeout, size cap, and MODEL_BASE_URL.
2. Revert required status / non-zero exit on model output.
3. Add a denylist test with a fixture .env in the diff.
4. Set continue-on-error: true if a comment bot is still desired.

That is the protocol. Trust the wiring you can replay. Revert the gate. Test the denylist and the exit policy. The model can wait until it is a product, not a coin flip on main.