feat(server): pre-validation request hook for spec-default injection (#614)

[bokelley]

Closes #614

What this adds

pre_validation_hooks: dict[str, Callable[[str, dict], dict]] | None on every public entry point — serve(), create_mcp_server(), create_a2a_server(), MCPToolSet, create_mcp_tools(), and create_tool_caller().

The per-tool hook runs on the raw wire dict before validate_request (JSON schema) and model_validate (Pydantic), so it can supply spec-mandated defaults for pre-v3 buyers that omit required fields. This eliminates the 273-line ASGI middleware workaround the issue author ships today.

Signature: (tool_name: str, raw_args: dict) -> dict

Quick example:

serve(
    router,
    pre_validation_hooks={
        "get_products": lambda n, a: {**a, "buying_mode": a.get("buying_mode", "brief")},
    },
)

Behaviour guarantees:

• None default → zero overhead on the hot path (no branch entered)
• Hook exceptions → ADCPTaskError(code="INVALID_REQUEST") — never INTERNAL_ERROR
• raw_params snapshotted before hook call → context echo always reflects original wire input, not server-injected defaults
• Per-tool scoping: hooks dict is fanned out to individual create_tool_caller() closures at registration time, not at call time

Files changed

File	Change
src/adcp/server/mcp_tools.py	Add pre_validation_hook param to create_tool_caller(), MCPToolSet, create_mcp_tools()
src/adcp/server/serve.py	Thread pre_validation_hooks through ServeConfig, serve(), _serve_mcp/a2a, create_mcp_server(), _register_handler_tools()
src/adcp/server/a2a_server.py	Thread through ADCPAgentExecutor.__init__() and create_a2a_server()
src/adcp/decisioning/serve.py	Add param + docstring example to decisioning serve()
tests/test_pre_validation_hooks.py	New — 7 tests covering all key invariants

Tested

• pytest tests/ -q → 4161 passed, 0 new failures (pre-existing test_real_tls_handshake_still_validates_hostname excluded — fails on main too)
• mypy src/adcp/ → Success: no issues found in 783 source files
• ruff check src/ → All checks passed

Pre-PR expert review

Two expert agents (code-reviewer + dx-expert) reviewed the diff independently. One blocker was found and fixed before this PR was opened:

BLOCKER (fixed): raw_params = params was originally assigned after the hook ran, meaning server-injected defaults (e.g. "buying_mode": "brief") would be echoed back to the buyer as if they sent them — a direct violation of the AdCP context-echo contract. Fixed by moving the snapshot to before the hook call. The fix is covered by test_hook_does_not_pollute_context_echo, which sends a wire payload with a context field, uses a stripping hook that returns a completely new dict (no context key), and asserts the response still echoes the original wire context.

NITs (not fixed, left for reviewer discretion):

• ServeConfig and a2a_server.py use dict[str, Callable[..., Any]]; mcp_tools.py/MCPToolSet use the precise dict[str, Callable[[str, dict[str, Any]], dict[str, Any]]] — the looser type at outer layers loses IDE completions
• No PreValidationHook type alias exported from adcp.server (compare: SkillMiddleware is exported); adopters writing typed code must spell out the full callable signature

---

This PR was opened by the triage automation for issue #614. A human reviewer should confirm the hook ordering and context-echo invariants before merging.

https://claude.ai/code/session_015hHQqXRbn2jX9WTu564gjZ

---

Generated by Claude Code

[bokelley]

Adopter confirmation (filed #614)

Read the diff. This kills our core/middleware/spec_defaults.py (273 LOC of JSON-RPC body bytes-rewriting). Migration is straightforward — every default we apply maps to a single hook callable.

Migration sketch on our side

```python
serve(
router,
pre_validation_hooks={
"get_products": _apply_get_products_defaults,
"create_media_buy": _apply_auth_filled_defaults,
"sync_creatives": _sync_creatives_defaults, # asset-type backfill + format_id wrap
"activate_signal": _apply_auth_filled_defaults,
"sync_accounts": _apply_auth_filled_defaults,
},
...
)
```

Where each callable is a small pure function — same logic as today's bytes-level patchers, just operating on a Python dict instead of decoded JSON bytes. Net: ~50 LOC of helpers replacing 273 LOC of ASGI middleware.

One design question

The hook signature is `(tool_name, raw_args) -> raw_args` — fine. But the PR docstring says "The hook must return a new dict; mutating the input in-place is a bug — the original is captured separately for context echo."

This is a footgun. `{**args, "buying_mode": "brief"}` is correct; `args["buying_mode"] = "brief"; return args` looks correct but breaks context echo silently.

Two options to mitigate:

1. Defensive copy at the call site before invoking the hook: `hook(name, dict(params))` — small allocation, eliminates the footgun entirely.
2. Document the failure mode harder with a runnable test/example showing what breaks.

Option 1 is the cleaner adopter contract — let people mutate freely, the framework guarantees context-echo safety. The cost is one dict shallow-copy per call which is negligible relative to schema validation right after.

Cross-link with #623

#623 ("Typed dispatcher rejects valid request when `account` is omitted") is the spec-correct fix for one of the specific cases this hook works around. They're complementary: this hook is the general-purpose adopter API for ALL spec defaults; #623 is the canonical fix for one specific buyer-omission case. Adopters running this hook for `account` placeholder injection can drop that hook entry once #623 lands. Worth a docstring note pointing readers there.

Ready to adopt

Once this lands we'll bump and migrate in one PR. Will report back on whether the migration matches the predicted ~50 LOC delta.

[bokelley]

Good catch on the footgun — Option 1 applied in 49feaef.

create_tool_caller now passes dict(params) to the hook instead of the live wire dict, so in-place mutation (args["buying_mode"] = "brief"; return args) is safe. The raw_params snapshot is taken before the copy is made, so context echo still reflects the original wire input regardless of what the hook does to its argument.

Docstrings in mcp_tools.py, a2a_server.py, and decisioning/serve.py updated to remove the "must return a new dict" restriction and describe the actual contract. Added test_in_place_mutation_is_safe_for_context_echo to lock the invariant — it exercises exactly the mutate-in-place-and-return pattern.

Also added the #623 cross-reference you suggested: adopters using a hook to inject an account placeholder can drop that entry once #623 lands.

Your migration sketch matches — {**args, "buying_mode": "brief"} and args["buying_mode"] = "brief"; return args are now equivalent.

---

Generated by Claude Code