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

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:

1. Unknown tool name or operation fails before I/O.
2. Extra JSON fields are rejected, not silently stored.
3. Required identifiers fail closed.
4. Non-2xx responses map to typed errors, not raw strings dumped into the model context.
5. 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.