Skip to content

Commit 8d6d783

Browse files
bokelleyclaude
andcommitted
fix(v3-reference-seller): expert-review fix-pack + serve(asgi_middleware=)
The 4-expert review of #357 surfaced multiple convergent blockers in the v3 reference seller MVP: * `app.py` `build_app()` imported `_build_mcp_and_a2a_app` from the wrong module → ImportError at adopter call time * `main()` never wired `SubdomainTenantMiddleware` → multi-tenant claim was false; `current_tenant()` would return None * `platform.create_media_buy` extracted `total_budget` / `currency` / `start_time` / `end_time` against a wrong-shape API surface (TotalBudget object, StartTiming union) — every persisted media buy stored corrupted data or NULL * `media_buy_id = f"mb_{uuid[:16]}"` collides at scale → IntegrityError Bundled fix: * Framework: add `asgi_middleware=Sequence[(cls, kwargs)]` kwarg to `adcp.server.serve()` (and forward through `adcp.decisioning.serve()`) so adopters layer Starlette-style ASGI middleware without dropping to `_build_mcp_and_a2a_app`. * Example: `main()` now passes `asgi_middleware=[(SubdomainTenant Middleware, {"router": router})]`. Dead `build_app()` helper removed. * `platform.py`: project `req.total_budget.amount/.currency` and unwrap `StartTiming.root` (`'asap'` → now()); `media_buy_id` is a full UUID (the (tenant_id, idempotency_key) unique constraint already guards replay). * `models.py` + `buyer_registry.py` + `seed.py`: `billing_capabilities`, `default_terms`, `allowed_brands` switched from JSON-encoded String columns to native JSON columns (matches the rest of the schema). * `audit.py`: docstring corrected — `record()` is best-effort but awaited inline (not fire-and-forget). * `tenant_router.py`: cache renamed from "LRU-ish" to bounded FIFO to match actual eviction behavior. * `README.md`: drop inaccurate claims (HTTP-Sig wired, project_account_for_response wired inline). Add dev-only callout for docker-compose trust auth + seed bearer token. Test: `tests/test_serve_asgi_middleware.py` covers the new kwarg's no-op + outermost-first composition contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 76ee0b5 commit 8d6d783

10 files changed

Lines changed: 245 additions & 139 deletions

File tree

examples/v3_reference_seller/README.md

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,14 @@ ships into one runnable binary:
1010
| Component | Module | Source |
1111
|---|---|---|
1212
| Tier 2 commercial-identity gate | `src/buyer_registry.py` | `adcp.decisioning.BuyerAgentRegistry` |
13-
| HTTP-Sig verifier → AuthInfo | (use `AuthInfo.from_verified_signer`) | `adcp.decisioning.AuthInfo` |
14-
| Subdomain tenant routing | `src/tenant_router.py` | `adcp.server.SubdomainTenantMiddleware` |
15-
| Account v3 projection (bank guard) | inline in platform | `adcp.types.project_account_for_response` |
13+
| Subdomain tenant routing | `src/tenant_router.py` + `src/app.py` | `adcp.server.SubdomainTenantMiddleware` |
14+
| Account v3 storage (bank-details column) | `src/models.py` | `Account.billing_entity` JSON column |
1615
| Audit trail | `src/audit.py` | `adcp.audit_sink.AuditSink` |
16+
| MCP + A2A on one binary | `src/app.py` | `serve(transport="both", asgi_middleware=...)` |
1717
| Durable HITL tasks (optional) | swap to `PgTaskRegistry` | `adcp.decisioning.pg.PgTaskRegistry` |
1818
| Durable webhook delivery (optional) | swap to `PgWebhookDeliverySupervisor` | `adcp.webhook_supervisor_pg` |
19-
| MCP + A2A on one binary | `src/app.py` | `serve(transport="both")` |
19+
| HTTP-Sig verifier → AuthInfo (TODO) | adopter middleware | `adcp.decisioning.AuthInfo.from_verified_signer` |
20+
| Account v3 projection on read (TODO) | adopter wires in `sync_accounts` | `adcp.types.project_account_for_response` |
2021

2122
## Run it
2223

@@ -36,6 +37,15 @@ DATABASE_URL=postgresql+asyncpg://postgres@localhost/adcp \
3637

3738
The server binds `0.0.0.0:3001` and serves both transports.
3839

40+
> ⚠️ **Local-dev only.** `docker-compose.yml` uses
41+
> `POSTGRES_HOST_AUTH_METHOD=trust` and exposes 5432 on
42+
> `0.0.0.0`. Do not run this compose file on a host reachable from
43+
> an untrusted network. `seed.py` plants a literal dev bearer
44+
> token (`dev-bearer-token-acme-1`) — do not run `seed.py` against
45+
> a production `DATABASE_URL`. Production deployments point
46+
> `DATABASE_URL` at managed Postgres with scram-sha-256 + a real
47+
> password, and seed via the admin API (not this script).
48+
3949
## What's wired
4050

4151
### Schema (`src/models.py`)
@@ -50,9 +60,13 @@ Four tables — the spine of a multi-tenant v3 seller:
5060
rejects suspended (transient) and blocked (terminal) agents with
5161
structured errors.
5262
- `accounts` — buyer-side accounts under recognized agents. Carries
53-
the spec 3.1-ready `billing_entity` (write-only bank details
54-
guarded via `project_account_for_response`) and `reporting_bucket`
55-
(offline reporting target).
63+
the spec 3.1-ready `billing_entity` (write-only bank details on
64+
responses) and `reporting_bucket` (offline reporting target). The
65+
reference seller does not implement `sync_accounts`, so the
66+
bank-details projection is a column-level architectural seam, not
67+
an enforced runtime guard — adopters who add `sync_accounts`
68+
MUST project through `adcp.types.project_account_for_response`
69+
before returning the row.
5670
- `media_buys` — terminal artifact of `create_media_buy`,
5771
idempotency-keyed for replay safety.
5872

@@ -83,10 +97,11 @@ alerting compose with `adcp.audit_sink.SlackAlertSink` via
8397
### Platform (`src/platform.py`)
8498

8599
`V3ReferenceSeller` implements `sales-non-guaranteed` — the five
86-
required Sales methods. Every method body reads
87-
`ctx.buyer_agent` (the resolved Tier 2 record) and `ctx.account`
88-
(the resolved account); both are populated by the framework's
89-
dispatch gate before the method runs.
100+
required Sales methods (`get_products`, `create_media_buy`,
101+
`update_media_buy`, `sync_creatives`, `get_media_buy_delivery`).
102+
Every method body reads `ctx.buyer_agent` (the resolved Tier 2
103+
record) and `ctx.account` (the resolved account); both are
104+
populated by the framework's dispatch gate before the method runs.
90105

91106
This file is the bulk of what an adopter customizes. Everything
92107
else is boilerplate the seller wires once.

examples/v3_reference_seller/seed.py

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
from __future__ import annotations
2020

2121
import asyncio
22-
import json
2322
import os
2423

2524
from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine
@@ -55,7 +54,7 @@ async def main() -> None:
5554
agent_url="https://signed-buyer.example/",
5655
display_name="Signed Buyer",
5756
status="active",
58-
billing_capabilities=json.dumps(["operator", "agent"]),
57+
billing_capabilities=["operator", "agent"],
5958
api_key_id=None,
6059
),
6160
BuyerAgent(
@@ -64,7 +63,7 @@ async def main() -> None:
6463
agent_url="https://bearer-buyer.example/",
6564
display_name="Bearer Buyer",
6665
status="active",
67-
billing_capabilities=json.dumps(["operator"]),
66+
billing_capabilities=["operator"],
6867
api_key_id="dev-bearer-token-acme-1",
6968
),
7069
BuyerAgent(
@@ -73,7 +72,7 @@ async def main() -> None:
7372
agent_url="https://suspended.example/",
7473
display_name="Suspended Buyer",
7574
status="suspended",
76-
billing_capabilities=json.dumps(["operator"]),
75+
billing_capabilities=["operator"],
7776
),
7877
]
7978
)

examples/v3_reference_seller/src/app.py

Lines changed: 17 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,9 @@
1212
* :class:`DbAuditSink` for compliance trail
1313
* :class:`V3ReferenceSeller` (the platform impl)
1414
15-
4. ``adcp.decisioning.serve(transport="both", ...)`` — single
16-
binary serving MCP at ``/mcp`` and A2A at ``/``.
15+
4. ``adcp.decisioning.serve(transport="both", asgi_middleware=[...])``
16+
— single binary serving MCP at ``/mcp`` and A2A at ``/`` with
17+
:class:`SubdomainTenantMiddleware` layered on the outer HTTP app.
1718
1819
Adopters fork this file and replace the platform impl, the seller-
1920
specific column populators, and the seed fixtures. Everything else
@@ -38,6 +39,7 @@
3839

3940
from adcp.decisioning import serve
4041
from adcp.server import (
42+
SubdomainTenantMiddleware,
4143
ToolContext,
4244
current_tenant,
4345
)
@@ -54,15 +56,14 @@
5456
logger = logging.getLogger(__name__)
5557

5658

57-
def _build_context_factory(router: SqlSubdomainTenantRouter):
59+
def _build_context_factory():
5860
"""``context_factory`` that pins :attr:`ToolContext.tenant_id`
5961
from the resolved tenant.
6062
6163
The middleware sets ``current_tenant()`` on the contextvar before
6264
dispatch; this factory reads it and writes ``tenant_id`` so the
6365
framework's idempotency middleware scopes correctly.
6466
"""
65-
del router # router is consumed via current_tenant(); kept for symmetry
6667

6768
def build(meta: RequestMetadata) -> ToolContext:
6869
tenant = current_tenant()
@@ -114,84 +115,28 @@ def main() -> None:
114115
)
115116
logger.info("Audit sink wired: %s. Tenant router cache: 256 hosts.", type(audit_sink).__name__)
116117

117-
# The framework's serve() drives uvicorn. We layer the tenant
118-
# middleware on the parent Starlette app via a build hook —
119-
# for the MVP we wire it inline by passing a custom ``app``
120-
# builder. For the Tier-2 wiring we use the ``buyer_agent_registry``
121-
# and ``context_factory`` kwargs the framework already supports.
122118
serve(
123119
platform=platform,
124120
name="v3-reference-seller",
125121
port=port,
126-
transport="both",
127-
buyer_agent_registry=buyer_registry,
128-
context_factory=_build_context_factory(router),
129-
# NOTE: SubdomainTenantMiddleware is added to the unified
130-
# Starlette app via ``add_middleware``. The current public
131-
# surface of ``serve()`` doesn't expose a hook for adopter
132-
# ASGI middleware on the parent — adopters who want it today
133-
# call ``_build_mcp_and_a2a_app`` directly and run uvicorn
134-
# themselves. Filed as a follow-up DX issue: the helper
135-
# below shows the pattern.
136122
host="0.0.0.0",
137-
)
138-
139-
140-
# Adopter-side helper for wiring SubdomainTenantMiddleware directly.
141-
# Until ``serve()`` accepts an ``asgi_middleware=`` kwarg, callers who
142-
# need tenant routing build the unified app themselves and run
143-
# uvicorn outside the framework's ``serve()`` wrapper.
144-
def build_app(
145-
platform: V3ReferenceSeller,
146-
*,
147-
router: SqlSubdomainTenantRouter,
148-
buyer_registry, # type: ignore[no-untyped-def]
149-
audit_sink, # type: ignore[no-untyped-def]
150-
):
151-
"""Construct the unified MCP+A2A app with the tenant middleware.
152-
153-
Returns an ASGI app callable; the caller runs uvicorn against it.
154-
"""
155-
from concurrent.futures import ThreadPoolExecutor
156-
157-
from adcp.decisioning.handler import PlatformHandler
158-
from adcp.decisioning.serve import _build_mcp_and_a2a_app
159-
from adcp.decisioning.task_registry import InMemoryTaskRegistry
160-
161-
executor = ThreadPoolExecutor(max_workers=8, thread_name_prefix="v3-ref-")
162-
handler = PlatformHandler(
163-
platform,
164-
executor=executor,
165-
registry=InMemoryTaskRegistry(),
123+
transport="both",
166124
buyer_agent_registry=buyer_registry,
125+
context_factory=_build_context_factory(),
126+
# SubdomainTenantMiddleware reads the request Host header,
127+
# resolves it via the SQL router, and sets the
128+
# ``current_tenant()`` contextvar before the handler runs.
129+
# The buyer_registry, AccountStore, and audit sink all read
130+
# that contextvar — this is the only multi-tenant wiring
131+
# point.
132+
asgi_middleware=[
133+
(SubdomainTenantMiddleware, {"router": router}),
134+
],
167135
)
168-
app = _build_mcp_and_a2a_app(
169-
handler,
170-
name="v3-reference-seller",
171-
port=3001,
172-
host="0.0.0.0",
173-
instructions=None,
174-
test_controller=None,
175-
context_factory=_build_context_factory(router),
176-
)
177-
# Wrap with the tenant middleware. ASGI middleware composes
178-
# outermost-first: requests pass through the tenant resolver
179-
# before reaching the dispatcher.
180-
from adcp.server.tenant_router import SubdomainTenantMiddleware as _SubdomainTenantMiddleware
181-
182-
class _Wrapped:
183-
def __init__(self, app):
184-
self._app = app
185-
self._mw = _SubdomainTenantMiddleware(app, router=router)
186-
187-
async def __call__(self, scope, receive, send):
188-
await self._mw(scope, receive, send)
189-
190-
return _Wrapped(app)
191136

192137

193138
if __name__ == "__main__":
194139
main()
195140

196141

197-
__all__ = ["build_app", "main"]
142+
__all__ = ["main"]

examples/v3_reference_seller/src/audit.py

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,17 @@
55
queries ("who suspended buyer X yesterday?").
66
77
Adopters with Slack / PagerDuty alerting compose this sink with
8-
:class:`adcp.audit_sink.SlackAlertSink` via a
9-
``CompositeAuditSink`` — both fire on every event; Slack is
10-
fire-and-forget, the DB sink is the durable record.
11-
12-
The sink is **fire-and-forget** by contract: failures inside
13-
:meth:`record` are bounded and swallowed by the framework's audit
14-
middleware. Adopters needing transactional audit (refusing the
15-
request if audit fails) implement their own pre-dispatch middleware.
8+
:class:`adcp.audit_sink.SlackAlertSink` via a ``CompositeAuditSink``
9+
so each sink fires on every event — Slack for the alert, this DB
10+
sink for the durable record.
11+
12+
Failure semantics: ``record()`` is **best-effort** — it ``await`` s
13+
the DB write inline, then swallows any exception so the audit step
14+
never fails the request. The audit step is therefore in the latency
15+
path of every skill call; adopters with high-volume audit traffic
16+
either decouple via an ``asyncio.Queue`` worker or batch writes.
17+
Adopters needing transactional audit (refusing the request if audit
18+
fails) implement their own pre-dispatch middleware.
1619
"""
1720

1821
from __future__ import annotations

examples/v3_reference_seller/src/buyer_registry.py

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616

1717
from __future__ import annotations
1818

19-
import json
2019
import logging
2120
from typing import TYPE_CHECKING
2221

@@ -106,10 +105,10 @@ async def resolve_by_credential(
106105

107106
def _row_to_agent(row: BuyerAgentRow) -> BuyerAgent:
108107
"""Project ORM row → framework typed :class:`BuyerAgent`."""
109-
capabilities = frozenset(json.loads(row.billing_capabilities))
108+
capabilities = frozenset(row.billing_capabilities or ())
110109
terms: BuyerAgentDefaultTerms | None = None
111-
if row.default_terms_json:
112-
d = json.loads(row.default_terms_json)
110+
if row.default_terms:
111+
d = row.default_terms
113112
terms = BuyerAgentDefaultTerms(
114113
rate_card=d.get("rate_card"),
115114
payment_terms=d.get("payment_terms"),
@@ -118,7 +117,7 @@ def _row_to_agent(row: BuyerAgentRow) -> BuyerAgent:
118117
)
119118
brands: frozenset[str] | None = None
120119
if row.allowed_brands:
121-
brands = frozenset(json.loads(row.allowed_brands))
120+
brands = frozenset(row.allowed_brands)
122121
return BuyerAgent(
123122
agent_url=row.agent_url,
124123
display_name=row.display_name,

examples/v3_reference_seller/src/models.py

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@
2727

2828
from __future__ import annotations
2929

30-
import json
3130
import uuid
3231
from datetime import datetime, timezone
3332
from typing import Any
@@ -131,8 +130,9 @@ class BuyerAgent(Base):
131130
:class:`buyer_registry.TenantScopedBuyerAgentRegistry` on top.
132131
133132
``billing_capabilities`` is the framework-enforced
134-
``frozenset[BillingMode]`` from the spec — adopters store as a
135-
JSON array and project at the seam.
133+
``frozenset[BillingMode]`` from the spec — stored as a native
134+
JSON array column and projected to ``frozenset`` at the
135+
registry seam.
136136
"""
137137

138138
__tablename__ = "buyer_agents"
@@ -156,24 +156,24 @@ class BuyerAgent(Base):
156156
#: (terminal) before the platform method runs.
157157
status: Mapped[str] = mapped_column(String(32), nullable=False, default="active")
158158

159-
#: JSON-encoded list of permitted ``BillingMode`` values. Default
160-
#: ``["operator"]`` is the pre-trust passthrough posture — agents
161-
#: under this row get operator-billed accounts only.
162-
billing_capabilities: Mapped[str] = mapped_column(
163-
String(255), nullable=False, default=lambda: json.dumps(["operator"])
159+
#: List of permitted ``BillingMode`` values. Default ``["operator"]``
160+
#: is the pre-trust passthrough posture — agents under this row
161+
#: get operator-billed accounts only.
162+
billing_capabilities: Mapped[list[str]] = mapped_column(
163+
JSON, nullable=False, default=lambda: ["operator"]
164164
)
165165

166166
#: Pre-trust beta API key. Adopters running signing-only auth
167167
#: leave NULL.
168168
api_key_id: Mapped[str | None] = mapped_column(String(128), nullable=True)
169169

170170
#: Default account terms (rate card, payment terms, credit limit,
171-
#: billing entity FK). JSON for portability.
172-
default_terms_json: Mapped[str | None] = mapped_column(String, nullable=True)
171+
#: billing entity FK).
172+
default_terms: Mapped[dict[str, Any] | None] = mapped_column(JSON, nullable=True)
173173

174174
#: Pre-RFC allowlist of brand domains. Layered with the (Tier 3)
175175
#: ``BrandAuthorizationResolver`` once spec #3690 lands.
176-
allowed_brands: Mapped[str | None] = mapped_column(String, nullable=True)
176+
allowed_brands: Mapped[list[str] | None] = mapped_column(JSON, nullable=True)
177177

178178
#: Adopter passthrough — internal ids, contract refs, etc.
179179
ext: Mapped[dict[str, Any] | None] = mapped_column(JSON, nullable=True)

0 commit comments

Comments
 (0)