额外参数即发布：审查扩展签名的 Agent PR

An optional parameter is a release. Coding agents add them so a new branch compiles, then attach a default that every old caller inherits. Review that default as shipped behavior, not as leftover style. This article is a review method for agent-generated pull requests that grow public signatures, exports, and CLI flags. It states what to trust, what to revert, and what to test. It also includes a proposed AST diff you can run on two worktrees. The failure mode Agents optimize for the call site they can see. They do not inventory every downstream importer, wrapper script, or dashboard that shells out to your CLI. A defaulted parameter changes behavior for callers that never pass it. A newly public name is a support promise. A flag that defaults to on rewrites production argv you did not grep. This is not hidden I/O, swallowed exceptions, or tests that encode the bug. The diff lives in the signature. The regression shows up two services away. Define the contract before arguing about taste For a Python package, treat the following as public unless the repo already documents a narrower rule: Module-level names that do not start with _ Names listed in __all__ Symbols re-exported from __init__.py argparse / click / typer options and their defaults Dataclass fields and typed-dict keys that serialize to JSON or ORM rows Private helpers (_parse) can move. Public names cannot. If the agent promotes _parse to parse so a unit test can import it, that is a product decision. Revert the promotion or accept the support cost. Artifact: two-tree signature diff The script below is a proposed, unexecuted example. It walks two source trees (base vs PR) and prints added public callables, added parameters, and changed defaults. Run it on a checkout. Do not treat its output as a security audit. #!/usr/bin/env python3 """signature_diff.py — compare public function signatures between two trees.""" from __future__ import annotations import ast import sys from dataclasses import dataclass from pathlib import Path @dataclass(frozen=True) class Param: name: str kind: str default: str | None @dataclass(frozen=True) class FuncSig: qualname: str params: tuple[Param, ...] returns: str | None def _rel(path: Path, root: Path) -> str: return path.relative_to(root).as_posix() def _is_public(name: str) -> bool: return not name.startswith("_") def _dump_default(node: ast.expr | None) -> str | None: if node is None: return None return ast.dump(node, include_attributes=False) def _params(fn: ast.FunctionDef | ast.AsyncFunctionDef) -> tuple[Param, ...]: args = fn.args out: list[Param] = [] def take(items: list[ast.arg], kind: str, defaults: list[ast.expr | None]) -> None: padded: list[ast.expr | None] = [None] * (len(items) - len(defaults)) + list(defaults) for arg, default in zip(items, padded): out.append(Param(arg.arg, kind, _dump_default(default))) take(args.posonlyargs, "posonly", []) take(args.args, "pos_or_kw", list(args.defaults)) if args.vararg: out.append(Param(args.vararg.arg, "vararg", None)) take(args.kwonlyargs, "kwonly", list(args.kw_defaults)) if args.kwarg: out.append(Param(args.kwarg.arg, "kwarg", None)) return tuple(out) def _skip_path(path: Path) -> bool: parts = set(path.parts) if parts & {"venv", ".venv", "site-packages"}: return True if "tests" in parts or path.name.startswith("test_"): return True return False def walk(root: Path) -> dict[str, FuncSig]: found: dict[str, FuncSig] = {} for path in root.rglob("*.py"): if _skip_path(path): continue try: tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) except SyntaxError: continue mod = _rel(path, root).replace("/", ".").removesuffix(".py") for node in tree.body: if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): if not _is_public(node.name): continue q = f"{mod}:{node.name}" ret = ast.dump(node.returns, include_attributes=False) if node.returns else None found[q] = FuncSig(q, _params(node), ret) elif isinstance(node, ast.ClassDef) and _is_public(node.name): for item in node.body: if not isinstance(item, (ast.FunctionDef, ast.AsyncFunctionDef)): continue if not _is_public(item.name): continue q = f"{mod}:{node.name}.{item.name}" ret = ast.dump(item.returns, include_attributes=False) if item.returns else None found[q] = FuncSig(q, _params(item), ret) return found def diff(base: dict[str, FuncSig], pr: dict[str, FuncSig]) -> None: print("## added public callables") for k in sorted(set(pr) - set(base)): names = ", ".join(p.name for p in pr[k].params) print(f"+ {k}({names})") print("## removed public callables") for k in sorted(set(base) - set(pr)): print(f"- {k}") print("## signature changes") for k in sorted(set(base) & set(pr)): if base[k].params == pr[k].params and base[k].returns == pr[k].returns: continue b = {p.name: p for p in base[k].params} p = {p.name: p for p in pr[k].params} added = [n for n in p if n not in b] dropped = [n for n in b if n not in p] changed = [n for n in p if n in b and p[n].default != b[n].default] print(f"* {k}") if added: print(f" added params: {added}") for n in added: print(f" default={p[n].default}") if dropped: print(f" dropped params: {dropped}") if changed: print(f" changed defaults: {changed}") for n in changed: print(f" {b[n].default} -> {p[n].default}") if base[k].returns != pr[k].returns: print(f" return: {base[k].returns} -> {pr[k].returns}") def main(argv: list[str]) -> int: if len(argv) != 3: print("usage: signature_diff.py BASE_TREE PR_TREE", file=sys.stderr) return 2 diff(walk(Path(argv[1])), walk(Path(argv[2]))) return 0 if __name__ == "__main__": raise SystemExit(main(sys.argv)) Enter fullscreen mode Exit fullscreen mode Checkout both sides, then run: git fetch origin pull/123/head:pr-123 git worktree add /tmp/pr-base origin/main git worktree add /tmp/pr-head pr-123 python3 signature_diff.py /tmp/pr-base /tmp/pr-head Enter fullscreen mode Exit fullscreen mode Pair the AST walk with a grep for CLI and env-shaped contracts the parser will miss: git diff origin/main...HEAD -- '*.py' \ | rg -n 'add_argument\(|click\.(option|command)|typer\.Option|os\.environ|getenv\(' Enter fullscreen mode Exit fullscreen mode The script will not see C extensions, *args forwarding that later binds names, or module-level __getattr__. Use it as a net, not as a proof. Decision table: trust, revert, test Diff class Trust? Revert? Test New _-prefixed helper, no re-export Yes, if old public paths still have tests No Existing callers only New public function used only by tests No Yes, or keep it private Import linter: tests must not introduce new public names New param whose default is None and skips a prior required path No Yes, unless the skip is an intentional product change Omitted arg equals old behavior; explicit None is documented New param whose default changes a timeout, limit, batch size, or retry No Revert the default; require explicit opt-in Bound test with the old numeric value as the implicit default Default value changed (limit=100 → limit=1000) No Restore the old default; add a named constant if needed Snapshot callers that omit the arg New required param No, unless this is an intentional major Restore a default or split a new function Compile downstream examples New CLI flag defaulting to on No Default off, or drop the flag Run the documented command line with no new flags Return type widened (list[User] → `list[User] \ None`) No Keep the old type; raise or return empty Re-export from __init__.py Only if it was already public Drop the re-export Freeze from package import name "Trust" means: leave it in the merge. It does not mean the agent understood production. Review sequence that stays on the signature Generate the signature diff. If it is empty and the CLI grep is empty, stop this checklist and review other classes of bug. For each added parameter, write the implied call in the review comment: foo(a, b) before, foo(a, b, c=DEFAULT) after. If DEFAULT is not observationally equal to the old body, the PR is a behavior change. Reject DEFAULT=None as a stand-in for "do the new thing." None is a value. Callers pass it by accident. Reject promotions made for test convenience. If a test needs a hook, keep the hook private or add a supported testing entrypoint. Freeze one documented import list and one documented command line in CI. Agent PRs that change either must update the freeze file in the same commit. Proposed freeze test (unexecuted example): # tests/test_public_names_freeze.py import json from pathlib import Path import myservice as pkg FREEZE = Path(__file__).parent / "public_names.json" def test_public_names_match_freeze(): names = sorted(n for n in dir(pkg) if not n.startswith("_")) expected = json.loads(FREEZE.read_text()) assert names == expected, ( "Public names changed. If intentional, update public_names.json " "and the changelog in the same PR." ) Enter fullscreen mode Exit fullscreen mode That test is boring. Boring is the point. Agents will edit the freeze file if the instructions allow it. Treat a freeze-file edit as a product change, same as a schema migration. Where a scratch model and server help Reproducing two worktrees and running signature_diff.py needs a clean machine, not a chat transcript. If you do not want extra worktrees on a laptop, MonkeyCode's free model access and free server option can hold the checkout and run the commands above. Disclosure: This article was prepared as part of MonkeyCode's product outreach. Use the model only to draft the missing tests for each added default. Do not let it "fix" the freeze file. The reviewer still classifies each row in the decision table. Skip any hosted model if the tree is proprietary and policy forbids upload. The method does not depend on a vendor. What to test when you keep a new default If the team accepts a new optional parameter, the minimum suite is three cases, not one: Omission. Call the old arity. Assert the old observable result, including timing bounds if the default is a timeout or batch size. Explicit old value. Pass the previous implicit value. Assert equality with omission. If they diverge, the default is not the old behavior. Explicit new value. Pass the agent's new path. Assert the new result, and assert it cannot be reached by omission. For CLI flags, replace step 1 with the argv vector copied from a runbook. Do not invent a "typical" command line. Copy one from production logs or from the README operators already use. Label any load or latency claim as unmeasured unless a bench already lives in the repo. A default limit change is a performance change even when CPU tests stay green. Worked review comment (proposed text, not a recorded incident): NACK default. `fetch_page(limit=1000)` changes every caller that omitted `limit`. Old body used 100. Restore default=100. Keep `limit=` as opt-in. Add tests: omit == 100, limit=100 == omit, limit=1000 is the new path only. Enter fullscreen mode Exit fullscreen mode Limitations The AST walker ignores dynamically constructed signatures, functools.wraps mismatches, and protocols implemented in another language. __all__ lies. Prefer dir() on the installed package over reading __all__ by eye. Dataclass field defaults and Pydantic Field(default=...) are public contract. The sample script does not parse them. Extend it or review those files by hand. Internal-only apps with a single binary and no importers can skip the freeze file. They should not skip CLI default review. Generated protobuf or OpenAPI stubs are the schema, not the agent's Python wrapping. Review the schema file, then the wrapper. This method will not catch authorization gaps, oracle-contaminated tests, or hidden network calls. Those need other checklists. Skip this workflow when the PR is a typo, a comment, or a pinned dependency with no signature change. Apply it when the agent "just added a parameter so the new branch compiles." Teams with an existing API-freeze bot can keep the bot and still use the decision table. The table is the review standard. The script is only a finder. Closing An omitted argument is a caller. New defaults ship to that caller without a changelog line. Diff the signatures. Revert convenience exports. Keep new behavior behind explicit arguments. Freeze one import list and one command line. That is the whole method.