Review the Invisible Contract: Env, Paths, and Retries in Agent PRs

Agent pull requests rarely fail because a function name is wrong. They fail because the typed surface stayed stable while the runtime contract moved. A new environment variable, a hardcoded path, a default timeout, or a retry loop is enough to change production behavior without turning a unit test red.

Review those four surfaces first. Trust public signatures. Revert undeclared wiring. Test I/O boundaries, not the agent's commit message.

This is a code-review rubric for agent-generated diffs, plus a small scanner you can run on a unified diff. The scanner is a teaching artifact. It does not approve a merge by itself.

Why the typed diff is the wrong zoom level

Agents optimize for a green local run. That bias shows up as defaults: os.getenv("X", "/tmp/x"), timeout=30, retries=3, except Exception: continue. Each default is a contract the ticket never named.

Cheap code makes this worse. When generating a patch is free, the patch grows sideways into config, filesystem, and network. Reviewers who only read the primary function will merge an implicit environment.

The useful question is not "does this look like the ticket?" It is "what must now be true of the machine for this to be correct?"

Four surfaces that carry the real contract

1. Environment and config reads

Flag every new getenv, environ, process.env, os.getenv, and config-file key. Ask three questions:

• Was this key documented in the ticket, the chart, or the runbook?
• What happens when it is missing? A crash is often better than a silent fallback.
• Does the fallback point at a shared location (/tmp, ., C:\\Temp) that another tenant or job can occupy?

Trust a typed config object that already existed. Revert a new optional env var whose only purpose is to make the agent's local path work.

2. Path and filesystem literals

Agents hardcode working directories, cache files, and "temporary" export paths. Those literals become production topology.

Look for:

• absolute paths
• relative paths that assume the process cwd
• open(..., "w") or write_text on a new file
• log or report directories created as a side effect of a GET handler

A path in application code is an interface. If the ticket did not ask to choose disk layout, revert the layout change and keep the export logic.

3. Time, sleep, and retries

Retries are the most common "reliability" gift in agent PRs. They convert a fast, loud failure into a slow, quiet one. Sleeps hide races. Backoff hides quota errors that the caller should see.

Treat a new retry as a product decision:

• How many attempts?
• Which exceptions are retryable?
• Is the operation idempotent?
• Who owns the timeout budget: this function, the HTTP client, or the orchestrator?

If the ticket said "export the report," revert the retry. If the ticket said "tolerate transient 503s from vendor X," keep a bounded retry with a metric and a non-retryable list.

4. Network and process boundaries

New requests, httpx, fetch, urllib, subprocess, Popen, or os.system lines are not helpers. They are new dependencies on the network and the OS.

Trust a call that already existed and only changed a field the ticket named. Revert a webhook, a curl, or a shell-out that the agent added to "notify" or "verify." Test the remaining call at the HTTP or process boundary with status codes the agent never generated.

Trust, revert, test

Surface	Trust	Revert	Test
Public function signature, types, return codes already in the API	Keep if the ticket named that change	Any extra optional argument that only exists to thread a local path	Contract tests on the old and new signature
Env / config keys	Keys that already ship in the chart or .env.example	New keys with silent defaults	Missing key, empty string, unexpected value
Filesystem	Existing storage abstraction	Hardcoded /tmp, cwd-relative writes, extra directories	Unwritable dir, collision, leftover files
Retry / sleep	Bounded retry on a named idempotent call	Blanket except Exception plus sleep	429, 500, 403, timeout, duplicate side effect
Network / subprocess	Existing client with the same host allowlist	New hosts, curl, shells, callbacks	DNS failure, TLS failure, non-zero exit
Comments and PR body	Nothing	Do not treat them as evidence	Ignore

The table is the review. The scanner below only points at rows you might have skipped.

Artifact: scan a unified diff for implied environment

Label: proposed local tool. It is pattern matching over a diff, not a program analysis. Run it on a branch before you argue about style.

#!/usr/bin/env python3
"""scan_agent_diff.py — flag implicit runtime assumptions in a unified diff.

Usage:
  git diff origin/main...HEAD > /tmp/pr.diff
  python3 scan_agent_diff.py /tmp/pr.diff
"""
from __future__ import annotations

import re
import sys
from collections import defaultdict
from pathlib import Path

RULES = [
    ("env", re.compile(r"(os\.environ|os\.getenv|getenv\(|process\.env|ENV\[)")),
    ("path_literal", re.compile(r"[\"'](/var/|/tmp/|/home/|C:\\\\|\\.\\./)" )),
    ("sleep_retry", re.compile(r"(time\.sleep|asyncio\.sleep|setTimeout\(|backoff|retry|tenacity|retries\s*=)", re.I)),
    ("network", re.compile(r"(requests\.|httpx\.|urllib\.|fetch\(|aiohttp\.)")),
    ("subprocess", re.compile(r"(subprocess\.|os\.system|Popen\(|shell=True)")),
    ("write", re.compile(r"(open\([^\n]*['\"]w|write_text\(|to_csv\(|dump\())")),
    ("bare_except", re.compile(r"except(\s+Exception)?\s*:\s*($|pass|continue|return)")),
    ("new_default", re.compile(r"^\+\s*def\s+\w+\([^)]*=")),
]

HUNK_FILE = re.compile(r"^\+\+\+ b/(.+)$")
ADDED = re.compile(r"^\+(?!\+)")


def scan(diff_text: str) -> dict[str, list[tuple[str, int, str]]]:
    hits: dict[str, list[tuple[str, int, str]]] = defaultdict(list)
    current = "(unknown)"
    line_no = 0
    for raw in diff_text.splitlines():
        line_no += 1
        m = HUNK_FILE.match(raw)
        if m:
            current = m.group(1).strip()
            continue
        if not ADDED.match(raw):
            continue
        body = raw[1:]
        for name, pattern in RULES:
            if pattern.search(body):
                hits[name].append((current, line_no, body.rstrip()))
    return hits


def main(argv: list[str]) -> int:
    if len(argv) != 2:
        print("usage: scan_agent_diff.py <unified.diff>", file=sys.stderr)
        return 2
    text = Path(argv[1]).read_text(encoding="utf-8", errors="replace")
    hits = scan(text)
    total = 0
    for name, _pattern in RULES:
        rows = hits.get(name, [])
        print(f"## {name}: {len(rows)}")
        for path, n, body in rows:
            print(f"  {path}:{n}: {body}")
            total += 1
        print()
    print(f"total_flags={total}")
    # Non-zero when anything fired so CI can mark 'needs human review'.
    return 1 if total else 0


if __name__ == "__main__":
    raise SystemExit(main(sys.argv))

The exit code is deliberate. Zero means the scanner found nothing, not that the PR is safe. Non-zero means a human should open the listed lines before discussing naming or test style.

Commands that keep the review on the branch

git fetch origin
git checkout -f review-agent-pr   # or the PR ref your forge provides
git diff --stat origin/main...HEAD
git diff origin/main...HEAD > /tmp/pr.diff
python3 scan_agent_diff.py /tmp/pr.diff; echo exit=$?

# Optional: restrict to runtime languages before arguing about docs.
git diff origin/main...HEAD -- '*.py' '*.ts' '*.js' '*.go' > /tmp/pr-code.diff
python3 scan_agent_diff.py /tmp/pr-code.diff

Read the --stat line first. If the agent touched lockfiles, CI, Docker, or Helm in a ticket about a report export, stop. That is a different review, and it is usually a revert.

Constructed walkthrough (not a production incident)

The ticket: "Write the daily report to object storage and return the object key."

The agent PR typically adds more than that. A constructed patch:

import os, time, json, subprocess, urllib.request

def export_report(rows):
    root = os.getenv("REPORT_DIR", "/tmp/reports")
    os.makedirs(root, exist_ok=True)
    path = os.path.join(root, "latest.json")
    for attempt in range(5):
        try:
            with open(path, "w", encoding="utf-8") as fh:
                json.dump(rows, fh)
            urllib.request.urlopen("https://example.invalid/hook", timeout=30)
            subprocess.check_call(["aws", "s3", "cp", path, "s3://company-reports/latest.json"])
            return path
        except Exception:
            time.sleep(2)
    return path

Apply the table.

Trust: nothing in this function until the storage interface matches the existing object-store client. The return type even lies: it returns a local path after a failed retry.

Revert:

1. REPORT_DIR defaulting to /tmp/reports — undeclared multi-tenant filesystem.
2. latest.json — concurrent jobs clobber each other.
3. The webhook — not in the ticket, new network identity.
4. aws s3 cp via subprocess — new process dependency, worse error handling than the SDK already in the repo.
5. Five attempts with except Exception — 403, schema errors, and disk-full all become a two-second pause.

Keep, after rewrite: a single call to the existing storage client, a caller-supplied object key, no retry unless the storage client already retries idempotent PUTs.

Test plan (execute these; do not accept the agent's tests as covering them):

1. Storage client raises 403 — function must fail, must not sleep five times.
2. Storage client raises a timeout — fail or retry only if the PUT is documented idempotent.
3. rows is empty — still writes a valid object or returns a defined error, not a leftover file.
4. Two concurrent exports — distinct keys, no latest.json race.
5. Env REPORT_DIR is unset — if you reverted the env var, this test is unnecessary; if you kept it, missing must error.
6. No outbound HTTP — packet-level or monkeypatched socket proves the webhook is gone.

That sixth case is the one reviewers skip. Agents add "just a ping" more often than they add a new type.

What a second-pass model review is for

After the scanner prints lines, a second model can classify each hit as trust, revert, or test. It should see the scanner output and the ticket text, not a request to "improve the PR." The prompt is a constraint: no new files, no new env keys, no retries unless the ticket used those words.

Disclosure: This article was prepared as part of MonkeyCode's product outreach. MonkeyCode is an open-source coding assistant with free model access and a free server option. Those two properties matter only after you already have a diff and a checklist. They let you run the classification step on a server you control instead of pasting a proprietary patch into a public chat. They do not replace the revert decisions above, and they do not make the scanner sound.

If you already run the script locally, a hosted pass is optional. Use it to group flags, not to generate more code.

Limitations

• Regular expressions over added lines miss indirection: cfg[key], generated clients, and macros.
• Comments and tests that mention retry create false positives. That is acceptable. False negatives are the risk.
• YAML, Helm, Terraform, and GitHub Actions are where many env contracts actually land. Extend the rules or review those files by hand.
• The scanner cannot tell idempotent PUTs from duplicate charge calls. Only the domain model can.
• Agent-written tests will often assert the new default path. Green tests that encode /tmp/reports are part of the contract change. Delete them with the code.
• No timing, token, or hardware numbers are claimed for any model or host. If a vendor page disagrees with this article, trust the vendor page for product facts.

Who should not use this approach

Do not use the scanner as a merge gate for authentication, cryptography, payments, or safety-critical control loops. Pattern flags are not a threat model.

Do not use it if the PR is a lockstep schema migration. Invisible contracts there live in data, not in getenv.

Do not use a model second pass on diffs that contain secrets, customer payloads, or proprietary weights. Redact first. If redaction is harder than reading the diff, skip the model.

Do not keep a retry the scanner did not flag just because the function name contains Reliable. Names are not contracts.

Close

Agent PRs are easy to read at the wrong layer. The signature is stable, the story is fluent, and the new behavior is a default argument. Start at env, paths, retries, and process boundaries. Trust what the API already promised. Revert the wiring the ticket did not buy. Test the failure the agent never generated.

The script is sixty lines so you can disagree with it. If a rule is noisy, delete the rule. If a PR still merges a /tmp default, the problem was not the linter.