Integrate deal API with MCP#32
Conversation
7e12710 to
acc7d76
Compare
atc964
left a comment
There was a problem hiding this comment.
Thanks — direction is right and it merges cleanly against current main. Three things before this can land, one hard blocker:
1. Blocker — the IAB deal ID is dropped. SSPDeal.deal_id now stores the internal UUID (required by the MCP tools' z.string().uuid()), but externalDealId — the OpenRTB deal ID a DSP needs for activation — is unreachable through the API: distribute_deal_via_ssp returns model_dump(exclude={"raw"}) so it's stripped (deal_service.py ~1027-1035), and the UUID leaks into DEAL_SYNCED events (execution_activation_flow.py ~297-309). Please add external_deal_id to SSPDeal, populate it from externalDealId, and include it in the distribute response + events. Without it, synced deals can't be transacted downstream.
2. The persistent-session workaround treats the wrong bug. The 400-on-second-request root cause is in deals-api-mcp itself: one shared StreamableHTTPServerTransport per process (index.ts ~388-399) instead of transport-per-session keyed by Mcp-Session-Id. Fixing it there lets this client drop the process-lifetime session task — which today can't survive any disconnect (reconnect hits the same 400 until seller-agent restarts) and has no test coverage (all 26 tests mock the transport).
3. Architecture — this isn't an SSP. deals-api-mcp is a deal-sync hub (real SSPs would sit behind it as providers). Registering it under SSPClient muddies provenance (ssp_type: "custom" in responses/events) and inventory-routing semantics. Preference: a DealSyncClient connector family peer to SSPClient/AdServerClient — the shared DealChannelClient base makes that nearly mechanical. If it must ship under SSPClient short-term: add SSPType.DEAL_SYNC (not CUSTOM), and note the migration in docs.
Smaller items: don't fabricate dealFloor: cpm or 1.0 (error on missing cpm); send the configured seller identity rather than advertiser/"IAB Deals MCP" as the Deal Sync seller; PG deals should set guar=1; honor the list_deals status filter; add tests for session lifecycle / update_deal / factory registration; document that buyer statuses currently come from the mock provider (demo-only); and please drop the unrelated formatting hunks in gam_soap_client.py / quote_history.py.
Happy to pair on the DealSyncClient split if useful.
… add tests and docs deals-api-mcp's TypeScript MCP SDK sets _initialized=true on first session and never resets it after termination, so a second initialize permanently fails with 400. Fix: class-level persistent background task holds the session open for the entire process lifetime; __aexit__ is a no-op so subsequent requests reuse the session without re-initializing. Also fixes SSPDeal.deal_id to store the internal UUID (deal.id) instead of externalDealId — all MCP tools validate dealId as z.string().uuid() and reject the IAB string format. Fixes troubleshoot_deal to read buyer statuses from the correct response path (status.buyerStatuses, not top-level buyerSeats). Adds 26 unit tests covering parse, create, get, list, clone, troubleshoot, and status mapping. Adds docs/integration/deals-api-mcp.md with connection lifecycle, configuration, status mapping, and persistent session explanation.
…t /api/v1/ssps from docs deals-api-mcp has no dealType concept so the field was silently defaulting to PMP regardless of what was requested. Now echo the requested deal_type back on the returned SSPDeal so callers get an accurate response. Also removes the GET /api/v1/ssps example from docs — that route does not exist in the seller-agent REST API.
|
Confirmed — 59e7993 resolves the externalDealId blocker: Two remaining items:
One note on CI: the red check isn't your change — the runner can't clone the private iab-agentic-primitives dependency, so the suite never runs (known issue, fixed once that repo goes public — which is waiting on the secret rotation on your/Todd's side). Until then we're running the full suite locally against PR heads as the merge gate; we've done that for 59e7993 and will re-verify the final head with the DealSyncClient change before merge. |
Design skeleton:
|
59e7993 to
236ad7c
Compare
Splits deals-api-mcp out of the SSP family into a peer DealSyncClient family (alongside SSPClient and AdServerClient), per reviewer design spec. - Add deal_sync_base.py: DealSyncProvider enum, DealSyncClient ABC, DealSyncRegistry (register, get_client, get_default, list_providers) - Add deal_sync_factory.py: build_deal_sync_registry() reads DEAL_SYNC_CONNECTORS env var (replaces SSP_CONNECTORS=deals_api_mcp) - git mv ssp_deals_api_mcp_client.py → deals_api_mcp_client.py; DealsAPIMCPClient now extends DealSyncClient; drop ssp_type attrs, keep ssp_name; persistent-session machinery unchanged - ssp_factory.py: remove deals_api_mcp branch - settings.py: add deal_sync_connectors; regroup deals_api_mcp_* settings - deal_service.py: dual-registry resolution in distribute_deal_via_ssp and troubleshoot_deal_via_ssp; deal-sync response uses channel/provider/provider_name (no ssp_type leakage); ssp_type excluded from deal-sync model_dump; ValueError → 400 - execution_activation_flow.py: parallel SSP + deal-sync blocks; DEAL_SYNCED payload uses channel/provider/provider_name/deal_sync_deal_id - __init__.py: export DealSyncClient, DealSyncProvider, DealSyncRegistry, DealsAPIMCPClient, build_deal_sync_registry - docs: update env var, architecture, response example, file path - tests: update import path; add TestDealSyncFactory (3 tests) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
236ad7c to
ded4884
Compare
|
Thanks @Sirajmx — the latest push addresses nearly everything from our review. Confirmed on the current head:
Two remaining asks before we merge:
The failing CI is our private iab-agentic-primitives install issue, not your changes — being fixed separately. Once the two items above land we'll run our merged-tree verification and merge. |
|
…+ retry Live E2E testing showed that after a server restart, session_alive returns True (SDK SSE reconnect loop keeps background task alive) so the stale session ID is reused. The tool call then hangs indefinitely: fresh server returns 400 but no SSE response ever arrives. Fix: wrap all call_tool calls through _call_tool which applies a 30s timeout. On timeout or HTTP 400/404, forces _start_shared_session and retries once. First request after a drop now succeeds in ≤30s; all subsequent requests are instant on the new session. Update docs to accurately reflect the reconnect mechanism.
- Fix list_deals sending sellerStatus instead of status; deals-api-mcp schema expects status (integer), sellerStatus was silently dropped so every filtered query returned all deals regardless of filter value - Update TestListDeals assertions to match correct field name (status) - Add TestUpdateDeal: covers tool name, ID pass-through, updates spread, and response parsing via _parse_deal (cpm, external_deal_id, status)
|
Thanks for the feedback @atc964
All 7 MCP tool paths E2E verified against live deals-api-mcp. |
|
👍 |
Summary
fail with 400 Bad Request. Resolved with a class-level background task that holds the session open for the process lifetime
Test plan
succession — both return 200 with internal UUID as deal_id