Skip to content

feat(testing): extend build_asgi_app with full serve-layer kwargs#626

Merged
bokelley merged 2 commits intomainfrom
claude/issue-618-build-asgi-app-kwargs
May 10, 2026
Merged

feat(testing): extend build_asgi_app with full serve-layer kwargs#626
bokelley merged 2 commits intomainfrom
claude/issue-618-build-asgi-app-kwargs

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

$(cat <<'EOF'

Summary

Closes #618.

adcp.testing.build_asgi_app previously returned a bare MCP ASGI app with no middleware. Adopters writing in-process integration tests had to import private helpers (_wrap_mcp_with_auth, _wrap_with_path_normalize, etc.) to replicate the production middleware stack — the exact friction the issue describes.

This PR:

  • Extends build_asgi_app with the full set of serve()-compatible kwargs: auth, allowed_hosts, allowed_origins, asgi_middleware, context_factory, middleware, streaming_responses, enable_dns_rebinding_protection, max_request_size, validation, discovery_base_url.
  • Applies the same wrapping order as _run_mcp_http in adcp.server.serve (auth → path_normalize → discovery → size_limit → asgi_middleware).
  • Extends build_test_client with all of the above except allowed_hosts (auto-derived from base_url).
  • Adds 13 new targeted tests covering every new parameter path.
  • Updates AGENTS.md import quick-reference with the new helpers.

Changed files

File What changed
src/adcp/testing/decisioning.py Rewrote build_asgi_app to apply the full middleware chain; extended build_test_client to forward all new params
tests/test_testing_decisioning.py 13 new tests for new kwargs
AGENTS.md Import quick-reference updated

Pre-PR review sign-offs

  • code-reviewer: ImportError blocker (SERVERDEFAULT_VALIDATION typo) found and fixed; allowed_origins missing from build_test_client added.
  • dx-expert: discovery_base_url docstring cross-reference added; allowed_origins param added to build_test_client.

Test plan

  • pytest tests/test_testing_decisioning.py -v — 31/31 pass
  • ruff check src/adcp/testing/decisioning.py tests/test_testing_decisioning.py — clean
  • mypy src/adcp/testing/decisioning.py — no new errors (pre-existing httpx/asgi-lifespan optional-dep errors only)
  • python -c "from adcp.testing import build_asgi_app, build_test_client, make_request_context" — imports cleanly

https://claude.ai/code/session_012Ujdm5DZ7CyECWqSq9KrHN
EOF
)


Generated by Claude Code

@bokelley
Copy link
Copy Markdown
Contributor Author

Adopter confirmation (filed #618)

Read the full diff against our `core/main.py:build_app()` boilerplate. This kills our private-API imports cleanly. Coverage check against everything our `_serve_kwargs` drives:

Our kwarg Covered by this PR
`name` ✅ `server_name`
`auto_emit_completion_webhooks` ✅ existing
`auth` (BearerTokenAuth) ✅ NEW
`asgi_middleware` ✅ NEW
`context_factory` ✅ NEW
`allowed_hosts` ✅ existing
`allowed_origins` ✅ NEW
`streaming_responses` ✅ NEW
`enable_dns_rebinding_protection` ✅ NEW
`port` / `host` (uvicorn-only) N/A — correctly excluded
`enable_debug_endpoints` Missing, but we don't drive it in tests anyway

After merge, our core/main.py:build_app shrinks from ~40 lines (including `from adcp.decisioning.serve import create_adcp_server_from_platform` and `from adcp.server.serve import _apply_asgi_middleware, _build_mcp_and_a2a_app` private imports) to roughly:

```python
def build_app():
from adcp.testing import build_asgi_app
kwargs = _serve_kwargs(include_scheduler=False, include_subdomain_routing=False)
return build_asgi_app(kwargs.pop("router"), **filtered_kwargs)
```

One concern: wrapping order

The PR docstring describes wrapping order matching `_run_mcp_http`. Worth verifying against the production path because we have downstream middleware (signing verification, dual-credential audit, transport detection) that depends on inner-vs-outer ordering. Specifically:

  • Auth runs innermost (good — body-peek before path normalize)
  • `asgi_middleware` runs outermost (good)
  • But `_run_mcp_http` also has a path-normalize wrapper between the size cap and discovery — verify that's preserved when `discovery_base_url` is None

If the answer is "yes, identical to production" then this is mergeable for our use case as-is.

Ready to adopt

Once this lands, we'll bump the floor and migrate `build_app` in the same PR. Happy to be the canary integration test — give us a heads-up when this merges.

@bokelley
Copy link
Copy Markdown
Contributor Author

@bokelley — verified against both src/adcp/testing/decisioning.py and src/adcp/server/serve.py.

Short answer: yes, path_normalize is preserved when discovery_base_url is None. The call is unconditional (no guard), so the stack when you omit a discovery URL is:

auth (innermost)
  └─ path_normalize          ← always applied
       └─ size_limit
            └─ asgi_middleware (outermost)

The if discovery_base_url is not None: guard only controls whether _wrap_with_discovery is inserted between path_normalize and size_limit — it doesn't affect path_normalize's presence or position.

One deliberate divergence from production to be aware of: _run_mcp_http always applies _wrap_with_discovery because resolve_base_url(host, port, None) synthesises a base URL from the bound socket. The test helper has no bound socket, so it skips discovery when you don't pass discovery_base_url. For your use case (no discovery endpoint in tests) this is the right behaviour, and all the ordering invariants your downstream middleware depends on (signing verification, dual-credential audit, transport detection) are preserved: auth innermost, asgi_middleware outermost, path_normalize in between.

The wrapping order is identical to production for your _serve_kwargs usage as-is.


Generated by Claude Code

@bokelley bokelley force-pushed the claude/issue-618-build-asgi-app-kwargs branch from 45b97a9 to 7c78108 Compare May 10, 2026 13:14
@bokelley bokelley marked this pull request as ready for review May 10, 2026 13:14
claude added 2 commits May 10, 2026 11:14
build_asgi_app previously returned only the bare mcp.streamable_http_app()
without auth, ASGI middleware, path normalization, or request size capping.
This caused silent gaps when tests needed transport-layer fidelity matching
production (auth middleware, CORS, custom context_factory, etc.).

Now applies the same wrapping chain as _run_mcp_http:
  auth (innermost) → path_normalize → discovery → size_limit → asgi_middleware

New params on build_asgi_app and build_test_client:
  auth, asgi_middleware, context_factory, middleware, streaming_responses,
  enable_dns_rebinding_protection, allowed_origins, max_request_size,
  validation (default DEFAULT_VALIDATION matching production), discovery_base_url

Also adds AGENTS.md import quick-reference entry for build_asgi_app,
build_test_client, make_request_context so coding agents discover these
helpers without falling back to network-layer mocks.

Closes #618

https://claude.ai/code/session_012Ujdm5DZ7CyECWqSq9KrHN
…wed_origins to build_test_client

- Fix SERVERDEFAULT_VALIDATION → SERVER_DEFAULT_VALIDATION (ImportError blocker found in pre-PR review)
- Add allowed_origins param to build_test_client and wire it through to build_asgi_app, matching issue spec

https://claude.ai/code/session_012Ujdm5DZ7CyECWqSq9KrHN
@bokelley bokelley force-pushed the claude/issue-618-build-asgi-app-kwargs branch from 7c78108 to 3b335d3 Compare May 10, 2026 15:15
@bokelley bokelley merged commit 8679a95 into main May 10, 2026
15 checks passed
@bokelley bokelley deleted the claude/issue-618-build-asgi-app-kwargs branch May 10, 2026 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(testing): public ASGI app builder for in-process test harnesses

2 participants