A Schema Pass Is Not an Execution Grant: Reviewing Agent PRs That Add Tools

Merge the parser. Hold the grant. An agent pull request that adds function calling is three changes in one diff: a schema, a policy, and a side effect. Schema checks can land like any other contract. Policy text and a live handler cannot ride along just because a model emitted valid JSON.

This piece uses a synthetic fixture, not a production incident report. No latency, cost, or pass-rate figures are claimed. The commands are local. The tests below are unexecuted examples, labeled so a reviewer can run them instead of trusting this draft.

What the diff usually hides

Agent PRs in this class often look small. A registry gains one tool. A test checks that one sample payload validates. The handler is short, usually added so the task can "finish."

That shape is the risk. Validation answers whether an object matched a schema. Execution answers whether this process may perform an action. Those questions fail independently.

Watch for these signals in the same commit:

• A tool name that is a vague verb, such as run_task or apply_fix
• A description that hands judgment to the model ("do what is needed")
• A parameter typed as an unconstrained string
• A handler that can reach a shell, an HTTP client, or the filesystem
• A config default that enables the tool outside tests

Review each signal on its own. A green happy-path test does not clear the bundle.

Synthetic diff under review

The module below is a constructed review fixture. It is not a recommended implementation, and the handler does not start a process.

# synth/agent_tools.py — fixture, not production code
from dataclasses import dataclass
from typing import Any, Callable

@dataclass
class Tool:
    name: str
    description: str
    parameters: dict
    handler: Callable[[dict], Any]

def proposed_tools() -> list[Tool]:
    return [
        Tool(
            name="run_task",
            description="Run whatever is needed to finish the user task",
            parameters={
                "type": "object",
                "properties": {
                    "command": {"type": "string"},
                    "url": {"type": "string"},
                },
                "required": ["command"],
            },
            handler=lambda args: args["command"],
        )
    ]

Returning the string is deliberate. The review question is whether this shape may become an executor, not how to invoke one.

What can land with the parser

Trust only hunks a reviewer can re-run without a model and without a network.

1. The schema document, if a test pins the tool name, required keys, and types.
2. A pure function that accepts or rejects an argument object before any handler runs.
3. A dry-run result that records the intended action and performs none.
4. Renames, comments, or tighter types, once those tests exist.

A passing schema test is evidence about parsing. It is not evidence about least privilege.

What to split out of the PR

Split or drop any hunk that does the following:

• Turns the new tool on from a default constructor, production config, or shared fixture
• Forwards a model-supplied string into a shell, a query API, or a URL fetch
• Uses the description string as the only constraint
• Adds a catch-all tool next to a narrow one "for flexibility"
• Copies raw arguments into logs, metric labels, or outbound errors
• Asserts on live assistant prose instead of the gate's return value

This is not a rejection of tool calling. It is a rejection of merging the grant with the parser.

What the replacement must prove

The replacement PR needs three artifacts: an allowlist expressed as data, a gate that rejects unknown actions before the handler, and tests that call a fake runner only.

Gate

# synth/grants.py
ALLOWED = {
    "format_python": ("ruff", "format"),
    "show_version": ("ruff", "--version"),
}

class GrantRejected(Exception):
    pass

def gate(action: str, extra: list[str] | None = None) -> tuple[str, ...]:
    if action not in ALLOWED:
        raise GrantRejected(action)
    if extra:
        raise GrantRejected("extra arguments are out of policy")
    return ALLOWED[action]

def dry_run(action: str) -> dict:
    argv = gate(action)
    return {"action": action, "argv": argv, "executed": False}

ruff is a stand-in binary name. A real repository should list its own fixed argv pairs. Python 3.11+ is assumed for list[str] | None.

Tests

# tests/test_tool_grant.py
import pytest
from synth.grants import GrantRejected, dry_run, gate

def test_known_action_is_a_fixed_argv():
    assert gate("show_version") == ("ruff", "--version")

def test_dry_run_does_not_execute():
    report = dry_run("format_python")
    assert report["executed"] is False
    assert report["argv"][0] == "ruff"

@pytest.mark.parametrize("action", ["run_task", "bash", "curl", ""])
def test_unknown_action_is_rejected(action):
    with pytest.raises(GrantRejected):
        gate(action)

def test_extra_arguments_do_not_widen_the_grant():
    with pytest.raises(GrantRejected):
        gate("show_version", extra=["--unsafe"])

git diff --stat
git diff -U3 -- synth/agent_tools.py synth/grants.py
python -m pytest tests/test_tool_grant.py -q

Intended outcome of this unexecuted fixture: known actions return a fixed argv, dry-run sets executed to false, unknown actions raise, and extra arguments do not widen the grant. This draft does not report an executed pytest session. If a later edit appends extra onto argv, the last test is the signal. A model transcript is not.

Decision table

Apply the table per hunk, not per pull request. One PR can contain a trustworthy parser and three hunks that should not land.

Review order on a mid-sized agent diff

Use this order when the diff is past a couple of screens. It keeps the grant from hiding inside formatting noise.

1. List files touched. Separate schema, config, handler, and tests.
2. Read config and constructors before descriptions. A default-on flag is a deploy, even if the schema looks narrow.
3. Search the handler for process, network, and filesystem calls. If any exist, demand a dry-run path in the same replacement.
4. Confirm tests import the gate, not a hosted client.
5. Write the review comment against the table row, and name the hunk.

Comment template:

Hunk: synth/agent_tools.py, tool run_task
Table row: unconstrained command string / grant
Decision: split out. Parser tests may land.
Replacement required: allowlist entry, gate rejection test, dry-run flag.
Not accepted as evidence: assistant prose, a green call to a shared host.

That comment is enough for a second reviewer to repeat the check. It does not depend on which model drafted the original patch.

Where optional hosted access fits

The gate does not need a hosted model. Review quality comes from the allowlist and the tests.

Disclosure: This article was prepared as part of MonkeyCode's product outreach. Operator-supplied context for this draft is limited to two availability claims: free model access and a free server option. Model names, quotas, hardware, duration, and benchmarks are omitted on purpose. They were not verified against a primary source here, and they change. Read current product documentation before use.

Optional sequence, after the local tests exist:

1. Keep tests/test_tool_grant.py as the merge bar.
2. Use free model access only to draft extra rejected-action names. A human deletes vague, duplicated, or out-of-scope cases before commit.
3. Use a free server only as a manual replay host for one recorded tool-call trace, on scratch input, outside CI.

Do not add that server to required fixtures. Do not fail the build when the free option is slow, capped, or withdrawn. A replay can suggest a missing rejection name. It cannot approve a handler.

Suggested note after a manual replay:

Replay host: free server option, manual, not a CI dependency.
Docs checked on: YYYY-MM-DD (reviewer fills the date).
Merge decision: tests/test_tool_grant.py, not the replay transcript.

If the docs are unavailable, skip the replay. Local tests still stand. If the access terms fit repository policy, those two free options are enough to draft rejection cases and replay one trace. Cite the docs date in the review comment. Do not cite a remembered allowance.

Limitations

The allowlist is only as careful as the binaries it names. This fixture does not cover tools loaded after startup, or a second agent that mints schemas at runtime. Description text can still mislead a model when the gate is correct. Enforcement belongs in gate, not in the prompt.

No count is offered for how often agent PRs mix these hunks. The table is a procedure, not a prevalence study. Teams bound by a data-processing addendum, a contractual uptime clause, or a ban on third-party inference should not send traces to a free server at all.

Who should skip this

Skip the hosted replay when the repository cannot accept an external processor, even for a scratch trace. Skip the grant split only when the change truly has no side effects, which is uncommon once a handler exists. Do not return this PR to the same agent to "fix the tests" unattended. The agent that widened the grant is a poor judge of whether the grant should exist.

Local tests remain the bar. Hosted drafting and replay stay optional, time-bounded, and disposable.