Green Tests, Broken Contracts: Surface-Reviewing Agent Pull Requests

Green unit tests do not mean the product contract is stable. Agent-generated pull requests often keep the algorithm intact while they rename exports, invent environment keys, add routes, or interpolate secrets into logs. Review those four surfaces first. Style review can wait.

A 400-line agent diff is a poor review object. People sample the “interesting” hunks and skip the boring ones. The boring hunks are where callers break.

The wrong unit of review

Default arguments, environment lookups, framework route decorators, and log interpolations rarely have dedicated tests. Agents treat them as implementation details. Downstream systems treat them as API.

This workflow inverts the usual order:

1. Build a surface inventory for main and for the PR head.
2. Diff the inventories, not the source tree.
3. Label each delta trust, revert, or test.
4. Refuse merge until every user-visible “trust” item has a pin: type stub, contract test, or changelog entry.

Ordinary logic review still happens. It happens after the surface is frozen. If the inventory is empty, you do not have a silent contract change in these four buckets. You can still have a logic bug.

Four surfaces unit tests almost never pin

Surface	Typical agent edit	Failure mode if merged
Public exports	Rename helper, add async, change keyword-only args	Import or type-check breakage downstream
Environment keys	New os.getenv("FOO_TIMEOUT", "30")	Silent default in prod; missing config in staging
HTTP / CLI routes	Extra @app.post("/v1/debug")	Accidental public API; authz gap
Log / error strings	logger.info(f"token={token}")	Secret leakage; alert-text drift

Treat invented numeric defaults as contract changes. A literal timeout=30 in a new client is not a local preference. It is a latency budget someone will page on.

Agents also invent plausible names. API_TIMEOUT_MS and CACHE_TTL look operational. They are often unwired. The agent’s sandbox stays green because the default fires. Production then runs a timeout you never reviewed.

Artifact: a heuristic surface inventory

The script below is a labeled heuristic, not a language server. It walks a tree, extracts conservative patterns from Python/JS/Go-ish files, and prints JSON. False negatives are expected. False positives should be cheap to dismiss.

#!/usr/bin/env python3
"""surface_inventory.py — heuristic contract surface for agent PR review."""
from __future__ import annotations

import json
import re
import sys
from pathlib import Path

PATTERNS = {
    "export_py": re.compile(
        r"^(async\s+)?def\s+([A-Za-z_]\w*)\s*\(([^)]*)\)", re.M
    ),
    "export_js": re.compile(
        r"export\s+(async\s+)?function\s+([A-Za-z_]\w*)\s*\(([^)]*)\)"
    ),
    "env": re.compile(
        r"""(?:os\.environ(?:\.get)?|os\.getenv|process\.env)\(?['\"]([A-Z][A-Z0-9_]{2,})['\"]"""
    ),
    "route": re.compile(
        r"""@(?:app|router)\.(get|post|put|patch|delete)\(\s*['\"]([^'\"]+)['\"]""",
        re.I,
    ),
    "log": re.compile(
        r"""(?:logger|log|console)\.(debug|info|warning|warn|error)\(([^)]{0,200})\)"""
    ),
    "default_num": re.compile(r"""(\w+)\s*=\s*(\d+)(?=\s*[,)])"""),
}

INTERESTING_DEFAULTS = {
    "timeout",
    "retries",
    "ttl",
    "port",
    "limit",
    "max_retries",
    "cache_ttl",
}
SKIP_PARTS = (".git/", "node_modules/", "vendor/", "dist/", "__pycache__/")


def iter_files(root: Path) -> list[Path]:
    files: list[Path] = []
    for p in root.rglob("*"):
        if not p.is_file():
            continue
        s = str(p)
        if any(part in s for part in SKIP_PARTS):
            continue
        if p.suffix in {".py", ".js", ".ts", ".go"}:
            files.append(p)
    return files


def inventory(root: Path) -> dict:
    out = {"exports": [], "env": [], "routes": [], "logs": [], "defaults": []}
    for path in iter_files(root):
        text = path.read_text(encoding="utf-8", errors="replace")
        rel = str(path.relative_to(root))
        for rx in (PATTERNS["export_py"], PATTERNS["export_js"]):
            for m in rx.finditer(text):
                out["exports"].append(
                    {"file": rel, "name": m.group(2), "params": m.group(3).strip()[:180]}
                )
        for m in PATTERNS["env"].finditer(text):
            out["env"].append({"file": rel, "key": m.group(1)})
        for m in PATTERNS["route"].finditer(text):
            out["routes"].append(
                {"file": rel, "method": m.group(1).upper(), "path": m.group(2)}
            )
        for m in PATTERNS["log"].finditer(text):
            snippet = re.sub(r"\s+", " ", m.group(2))[:160]
            out["logs"].append(
                {"file": rel, "level": m.group(1).lower(), "snippet": snippet}
            )
        for m in PATTERNS["default_num"].finditer(text):
            name, val = m.group(1), m.group(2)
            if name in INTERESTING_DEFAULTS:
                out["defaults"].append({"file": rel, "name": name, "value": val})
    for key in out:
        out[key] = sorted(out[key], key=lambda row: json.dumps(row, sort_keys=True))
    return out


def main() -> None:
    root = Path(sys.argv[1] if len(sys.argv) > 1 else ".")
    json.dump(inventory(root), sys.stdout, indent=2)
    sys.stdout.write("\n")


if __name__ == "__main__":
    main()

Run it twice. Once on origin/main, once on the PR branch. Then print added and removed rows.

git fetch origin main
python3 surface_inventory.py /path/to/main-worktree > /tmp/surface-main.json
python3 surface_inventory.py /path/to/pr-worktree > /tmp/surface-pr.json
python3 - <<'PY'
import json
from pathlib import Path

def load(p):
    return json.loads(Path(p).read_text())

a, b = load("/tmp/surface-main.json"), load("/tmp/surface-pr.json")
for key in a:
    sa = {json.dumps(x, sort_keys=True) for x in a[key]}
    sb = {json.dumps(x, sort_keys=True) for x in b[key]}
    added, removed = sorted(sb - sa), sorted(sa - sb)
    if not added and not removed:
        continue
    print(f"\n## {key}")
    for row in added:
        print("  +", row)
    for row in removed:
        print("  -", row)
PY

That printed delta is the review object. Keep the source diff for algorithms and data loss. Do not start there.

Classify every delta before arguing about style

Do not negotiate in the comment thread until each inventory row has a label.

Delta	Trust if	Revert if	Test if trusted
New export used only inside the PR	Name is stable and documented	Rename-only “cleanup” with no caller	Import / type-check in CI
New os.getenv / process.env	Key already exists in deploy docs and the secret store	Key is invented; default hides misconfig	Fail boot when unset in staging
New route	Authz matches sibling routes; schema file updated	Debug or admin path on the public router	Contract test for 401/403 and body schema
Log snippet includes a variable	Variable is a non-PII identifier	Token, cookie, Authorization, raw body	Grep test forbidding those names
Numeric default (timeout, retries, ttl)	Matches an existing SLO or config module	Magic number introduced “to make it work”	Pin the value in a config test

Revert is the default for invented environment keys. If the key is not in deploy docs, it does not belong in this PR. Named constants in a config module are cheaper to review than a new runtime knob.

Trust is allowed for pure moves when the inventory shows the same export name, parameter list, and role, and git log --follow agrees. If the inventory disagrees with git, revert the rename. Keep the logic patch.

What to test when you trust a surface change

Do not add more unit tests that call the new helper with mocks. Pin the surface. The following pytest module is a proposal, not a measured production suite.

# tests/test_surface_pins.py
import json
from pathlib import Path

FROZEN = {
    "routes": [
        {"method": "GET", "path": "/health"},
        {"method": "POST", "path": "/v1/jobs"},
    ],
    "env": ["DATABASE_URL", "JOB_TIMEOUT_MS"],
}


def test_routes_are_explicit():
    current = json.loads(Path("artifacts/surface-pr.json").read_text())
    got = {(r["method"], r["path"]) for r in current["routes"]}
    frozen = {(r["method"], r["path"]) for r in FROZEN["routes"]}
    unexpected = got - frozen
    missing = frozen - got
    assert not unexpected, f"unreviewed routes: {unexpected}"
    assert not missing, f"removed routes: {missing}"


def test_env_keys_are_known():
    current = json.loads(Path("artifacts/surface-pr.json").read_text())
    keys = {e["key"] for e in current["env"]}
    extra = keys - set(FROZEN["env"])
    assert not extra, f"unreviewed env keys: {extra}"

Update FROZEN in the same PR as the surface change. That forces the changelog discussion onto a file reviewers actually open.

For log leakage, a cheaper pin is a diff grep. Expect false positives. A noisy grep is cheaper than a leaked session cookie.

# heuristic: fail if new logs interpolate obvious secrets
if git diff origin/main...HEAD -U0 | grep -Ei \
  'logger\.(info|debug|error).*\{.*(token|secret|password|authorization|cookie)'; then
  echo "possible secret interpolation in logs" >&2
  exit 1
fi

Labeled example: a “small retry” PR

Suppose an agent is asked to handle intermittent HTTP 503s from a job API. A typical patch adds retries=3, timeout=30, reads JOB_TIMEOUT_MS, and logs the request URL with a bearer token “for debugging.” Tests mock requests.post and pass.

Hypothetical surface delta:

• defaults: retries=3, timeout=30 added in client.py
• env: JOB_TIMEOUT_MS added with default "30"
• logs: logger.info(f"fetch {url} token={token}")
• routes: unchanged
• exports: fetch_job gained a retries parameter

Classification:

1. Log line — revert. Token interpolation is not a retry fix.
2. JOB_TIMEOUT_MS — revert unless deploy docs already list it. If ops already uses the key, test that staging fails closed when unset.
3. retries=3 — test. Pin retry count next to the SLO. Three retries with no jitter can amplify an incident.
4. Signature change on fetch_job — trust only if every in-repo caller is updated in the same PR. Otherwise revert the extra kwargs and wrap internally.

None of that requires reading four hundred lines of retry helper. The inventory made the argument local.

Optional second pass on the inventory, not the tree

A second model is useful only if you starve it of the original prompt that produced the PR. Feed it the two JSON files and the classification table. Do not feed it the full diff. Do not ask it to rewrite the patch.

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

If you already extract the inventory, a constrained classifier is enough. MonkeyCode’s free model access and free server option can host that second pass; the job is labeling rows, not generating code. Require {surface, item, label, reason} with labels restricted to trust|revert|test. Discard any answer that proposes extra refactors.

Frozen reviewer prompt (proposal):

You are reviewing a surface inventory delta for a pull request.
You may only use the JSON added/removed rows and the classification table.
For each row output: surface, item, label (trust|revert|test), reason (<=20 words).
If an environment key is new and no deploy doc is in context, label revert.
If a log snippet interpolates token/secret/password/authorization/cookie, label revert.
If a route is new and authz is not in the inventory, label test.
Do not suggest features. Do not rewrite code.

Keep a human as the merger. The second pass exists to catch inventory rows a tired reviewer skipped. It is not an approval.

Limitations

• Regex inventories miss metaprogrammed routes, __getattr__ exports, and indirect environ access.
• Go and Java are under-covered. Extend patterns or switch to go/ast / tree-sitter before you trust silence.
• Default-number detection is name-based. n = 30 will not flag. timeout=30 will.
• This workflow does not replace SAST, license scanning, or threat modeling.
• A clean surface diff can still ship a logic regression. Keep ordinary review for algorithms and data loss.

Silence from the script is not a proof. It is a narrower question: did this PR move the tracked contract.

Who should not use this as the only gate

Skip this as a merge gate if:

• The repository has no stable public surface (throwaway prototypes).
• CI cannot fail on unexpected routes or env keys. The pin file is the product.
• The PR is a lockfile-only bump. Use the package manager’s audit tooling instead.
• You need a security sign-off. Log greps are not a disclosure review.

If the inventory is noisy on day one, freeze only routes and env. Add logs and defaults after a week of dismissing false positives. The value is the classification table, not the parser.

Surface drift is a review problem with a small mechanical core. Extract it. Label it. Pin what you trust. Revert what the agent invented so the tests would stay green.