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

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.

1. Put MODEL_BASE_URL and MODEL_API_KEY in the shell or a secret store, never in the repo.
2. Cap MODEL_MAX_OUTPUT_TOKENS at a small integer, such as 128, so a review run stays bounded.
3. Keep MODEL_LOG_PROMPTS=0.
4. Send a synthetic prompt with no customer data, secrets, or internal hostnames.
5. 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.