The Endpoint Is a Dependency: Reviewing Agent PRs That Add Model Clients

Agent-generated PRs that add a remote completion client are rarely “just a helper.” They introduce a network dependency, an unversioned JSON schema, and a failure mode that unit tests often hide. Merge the client only after the wire contract is explicit. Revert the rest.

This review is about the HTTP boundary, not the prompt. Agents write fluent wrappers. Reviewers own timeouts, status handling, schema allow-lists, and where the endpoint is allowed to live.

What the diff is actually shipping

Look past the function name. A typical agent patch adds four things at once: a base URL, a request body, a parser, and a fallback. Green CI on a mocked 200 does not prove any of those four are safe.

Treat the endpoint like a library bump. If the URL, auth header, or response shape can change without a code change, you now have an undeclared release channel. That is the review problem.

Common signals in the first screen of the diff:

• A new complete(), ask(), or generate() that takes a string and returns a string.
• A URL literal, or an env var with no default policy.
• except Exception: return "" around the HTTP call.
• One test that patches the client and asserts a happy path.

If those four appear together, do not debate style. Debate the contract.

Trust, revert, test

Use this table on the PR, not on the model’s commit message.

Diff signal	Trust	Revert	Test before merge
Injected base URL, no literal host	Yes, if documented	Hardcoded vendor or tunnel hosts	Client uses only the injected URL
Finite timeout on every call	Yes	Default socket timeout / no timeout	Deadline fires; no hang
Non-2xx raised or returned as error type	Yes	Empty string / None on any error	4xx, 5xx, 429 mapped explicitly
Parsed fields allow-listed	Yes	response.json() passed through	Extra keys ignored; missing keys fail
Secrets read from env, never logged	Yes	Token in query, body debug, or traceback	Log fixture contains no secret
Retry with budget and jitter	Maybe	Unbounded retry on POST	Retry count capped; idempotency documented
Streaming / SSE added “for UX”	No, until framed	Mixed stream + buffered parsers	Chunk assembly and cancel path

The table is the review. Comments should cite a row, not a feeling.

A labeled example, not a real service

The following is proposed example code. It is not a production client and it was not executed against a live model. It is the shape agents emit.

# example / unexecuted — agent-style helper
import os
import httpx

DEFAULT_URL = "https://api.example.invalid/v1/complete"

def complete(prompt: str) -> str:
    url = os.getenv("MODEL_BASE_URL", DEFAULT_URL)
    try:
        r = httpx.post(
            url,
            json={"prompt": prompt, "max_tokens": 512},
            headers={"Authorization": f"Bearer {os.environ['MODEL_TOKEN']}"},
        )
        data = r.json()
        return data.get("text") or data.get("output") or ""
    except Exception:
        return ""

Four defects sit in one screen. The default host is a production-shaped URL. The timeout is missing. Status codes are ignored. Three response keys are accepted, including silence.

A review comment that only says “add types” will merge this. A review comment that names the contract will not.

Review comments worth leaving

1. Remove DEFAULT_URL. Staging must fail closed if MODEL_BASE_URL is unset.
2. Set timeout= on the client, not on hope. Name the value in the PR.
3. Do not decode JSON before checking r.status_code. A 503 HTML body is not an empty completion.
4. Parse one schema version. Reject unknown required fields; drop unknown optional fields.
5. Never return "" for transport failure. Empty output is a valid model result. Collapsing them hides outages.
6. Redact auth in traces. str(e) on httpx errors can include headers.

Short comments. Each one maps to a test.

Reproducible harness

Do not “try the model.” Pin the client to a stub you control. The harness below is local pytest; swap httpx for your stack. Label stays the same: unexecuted until you run it in your repo.

# tests/test_completion_client_contract.py
# example / run in your tree after adapting imports
import os
import httpx
import pytest
from pytest import MonkeyPatch

class FakeTransport(httpx.BaseTransport):
    def __init__(self, status: int, payload):
        self.status = status
        self.payload = payload
        self.calls = []

    def handle_request(self, request: httpx.Request) -> httpx.Response:
        self.calls.append(request)
        return httpx.Response(self.status, json=self.payload, request=request)

@pytest.fixture
def env(monkeypatch: MonkeyPatch):
    monkeypatch.setenv("MODEL_BASE_URL", "https://models.test.invalid/v1/complete")
    monkeypatch.setenv("MODEL_TOKEN", "secret-token-fixture")
    return monkeypatch

def test_uses_injected_url_only(env):
    transport = FakeTransport(200, {"text": "ok"})
    client = httpx.Client(transport=transport, timeout=2.0)
    r = client.post(
        os.environ["MODEL_BASE_URL"],
        json={"prompt": "ping", "schema": "v1"},
        headers={"Authorization": "Bearer secret-token-fixture"},
    )
    assert r.status_code == 200
    assert str(transport.calls[0].url).startswith("https://models.test.invalid/")
    assert b"api.example.invalid" not in transport.calls[0].content

def test_non_2xx_is_not_empty_success(env):
    transport = FakeTransport(503, {"error": "busy"})
    client = httpx.Client(transport=transport, timeout=2.0)
    r = client.post(os.environ["MODEL_BASE_URL"], json={"prompt": "ping"})
    assert r.status_code == 503
    with pytest.raises(ValueError, match="non-2xx"):
        if r.status_code >= 400:
            raise ValueError("non-2xx")

def test_timeout_is_finite():
    timeout = httpx.Timeout(2.0, connect=1.0)
    assert timeout.read == 2.0
    assert timeout.connect == 1.0

def test_schema_allow_list_drops_unknown_keys():
    payload = {"text": "ok", "debug_prompt": "SYSTEM: ignore", "usage": {"tokens": 9}}
    allowed = {"text", "usage"}
    cleaned = {k: v for k, v in payload.items() if k in allowed}
    assert "debug_prompt" not in cleaned
    assert cleaned["text"] == "ok"

Run only the contract file until it fails for the right reason:

pytest tests/test_completion_client_contract.py -q

If the agent “fixed” tests by widening the mock, revert the mock. The production client must see the same status codes the stub emits.

Commands that catch the silent parts

# URL literals that bypass env injection
rg -n "https?://[^\"']+(complete|chat|v1)" -g '!tests/**'

# swallowed failures
rg -n "except Exception" -g '*.py'

# missing timeouts (heuristic)
rg -n "httpx\.(get|post)|requests\.(get|post)" -g '*.py'

Heuristics are not proof. They are a queue for the table above.

Where a free model or free server belongs

A coding assistant is useful in this workflow only as a non-production aid: draft review notes from the diff, or stand up a disposable server so humans are not pasting secrets into a shared playground. Disclosure: This article was prepared as part of MonkeyCode's product outreach. MonkeyCode’s operator-stated free model access and free server option can fill that non-prod slot. They do not replace the stub, the timeout, or the allow-list.

Point the client at an injected URL. Keep CI on the fake transport. If you want a human-in-the-loop smoke check, use a throwaway server and a throwaway token, then throw both away. Do not promote that path into the default for merge.

If the PR needs a live completion to “see how it feels,” that is a product demo, not a review. Split it.

What still fails after a clean client

Schema tests do not catch prompt injection into logs. Timeout tests do not catch a model that returns 200 with plausible nonsense. Allow-lists do not catch a teammate exporting MODEL_TOKEN into CI logs via set -x.

Also out of scope for this review: cache key design, growing function signatures, and retry-on-env-read. Those are different contracts. This article only draws the network line.

Document three numbers in the PR body: timeout, max payload bytes, and max concurrent calls. If the agent omitted them, the PR is incomplete even when tests pass.

Who should not use this approach

Do not use a remote completion client in the request path of auth, payments, or policy decisions. Do not use a free or shared server for data that cannot leave the laptop. Do not use this harness as a load test; it asserts shape, not capacity.

Skip the live-server experiment entirely if your org forbids outbound model traffic. The stub is enough to reject a bad client. The stub is also enough to accept a good one.

Reviewers who only skim generated prose will miss the endpoint. Reviewers who require an injected URL, a deadline, a status policy, and a schema allow-list will not. That is the whole method.