[josh-butcher-index]

Summary

• The Index Exchange SSP connector (src/ad_seller/clients/ssp_index_exchange.py) was implemented without access to IX API docs, so field names, endpoints, and request/response shapes were invented rather than matching the real deals-web-api /v3/deals contract.
• Rewrites the connector against the actual API: correct auth (Keycloak JWT), request/response field names, ETag-based PATCH updates, and removal of the nonexistent clone endpoint.
• Adds DSP resolution from buyer seat IDs (a seat ID can map to more than one DSP), routing ambiguous matches through the existing human-in-the-loop approval gate rather than guessing.
• Extends the MCP tool surface to support the full agentic flow end-to-end (resume_approval, approve_or_reject modifications, distribute_deal_via_ssp/create_curated_deal params) — this was verified end-to-end against real IX staging.
• No functionality beyond what the connector already attempted to provide, made correct rather than invented.

Test plan

• tests/unit/test_ssp_connectors.py and tests/unit/test_dsp_resolution_gate.py pass against the corrected fixtures
• tests/integration/test_deal_flow_e2e.py passes
• Manual smoke test against IX staging /v3/deals

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com

[aleksUIX]

If two seat IDs resolve to different DSPs, approval asks for one DSP for both seats. That cannot represent the request. Could this case fail clearly or split into separate deals?

[josh-butcher-index]

Good catch, thanks. Resolution was flattening matches across all seats, so seats on different DSPs ended up gated on picking one DSP for both. Fixed in 945dd96: it now resolves per seat and intersects the DSP sets. If a seat has no active match, or the seats share no DSP, it fails with a 400 that lists the seat→DSP mapping. If exactly one DSP owns every seat it's used directly, and the approval gate is only for a real collision, with candidates limited to DSPs shared by all seats.

I went with failing clearly rather than splitting, since splitting would turn one call into several deals (multiple deal IDs and external IDs) and change the tool's return shape. Happy to add splitting as a follow-up if you'd prefer it.

[Kishore-2572]

Checked 945dd96's fix for the multi-seat DSP intersection — correct, verified live with a seat-to-different-DSP scenario that previously would have produced a bogus "pick one for both" approval request; now correctly 400s with a clear per-seat mapping.

One open question while testing that same code path: _resolve_dsp_id_or_pending_approval (deal_service.py) matches Index's response back to the request using match.extended_seat_id, compared against the literal seat_id the caller passed in — treating the two as always identical.

Is extendedSeatID in the real API guaranteed to be character-for-character the same value as the seatID you queried with, or can Index return a different/normalized form of it?

Checked both iab-agentic-primitives and every other SSP connector in this repo — neither defines or uses this extended_seat_id concept anywhere, so there's no existing convention here to compare against; this connector is the only place it exists in our codebase. Since the PR description mentions this was built against Index's actual (non-public) API docs, you'd be the one with visibility into this.

• If they're always identical, this is a non-issue.
• If they can differ, _resolve_dsp_id_or_pending_approval will silently report "no active DSP found" for a seat that actually has a valid match — reproduced this directly by feeding in a mismatched extended_seat_id.

Worth a test either way — every existing test in test_dsp_resolution_gate.py happens to set them identical, so this case has no coverage currently, regardless of the answer.

[aleksUIX]

Failing clearly is the right scope for my multi-seat concern. Resolving each seat and limiting approval candidates to their common DSPs addresses it. No split-deal behavior is needed for that finding.

[josh-butcher-index]

Thanks for testing that path, @Kishore-2572. On the API question: seatID is our internal seat identifier, while extendedSeatID is the DSP-facing ID that callers pass in, and the two are not guaranteed to be equal. The resolver should not have treated them as interchangeable.

Fixed in 03f92ef. resolve_dsp_ids_for_seat_ids now makes one request per seat ID, run concurrently, and tags each match with the seat it was queried for (requested_seat_id). _resolve_dsp_id_or_pending_approval groups on that field, so resolution no longer depends on the extendedSeatID in the response. I added tests where the returned extendedSeatID differs from the requested one, and one asserting that each seat is queried separately.

[Kishore-2572]

Pulled 03f92ef and independently reproduced the extendedSeatID/seatID mismatch scenario live against the real connector + _resolve_dsp_id_or_pending_approval — resolution now correctly succeeds end-to-end where it previously would have wrongly reported "no active DSP found."

Negative-controlled the new regression test by reintroducing the old matching logic — it fails with exactly the old bug's error, confirming it genuinely catches this regression.

Also checked the case-insensitivity removed from deal_service.py (.casefold()) — not a regression, since matching no longer crosses between request/response strings at all. Verified live with a duplicate-seat-different-casing case.

Minor non-blocking note: seat resolution is now N concurrent per-seat calls instead of one batched call, with no cap on seat-list size. Tested a partial failure (one seat fails, others succeed) — fails cleanly, no silent partial state.

Full suite: 1660 passed, lint/format clean. Both review threads hold up under live testing. Approving.

[atc964]

Thanks Josh, and thanks to Kishore's team for the local validation. The connector rewrite itself is in good shape: the field mapping matches your fixture end to end, the ambiguous-seat path correctly books nothing while pending, and the error-path tests are genuinely strong. We need changes on the approval/resume flow and ID handling before this can merge, since they sit on the money path:

1. The DSP-resolution gate snapshots the create request including cpm into pending-approval state. list_pending_approvals (MCP) currently has no operator gate, and REST POST /approvals/{id}/decide and /resume accept any valid API key, while resume applies arbitrary modifications (including cpm) and then creates the IX deal. Please add the operator gate to list_pending_approvals and the REST decide/resume routes within this PR, or restrict modifications to dsp_id only, so a non-operator key can neither read the floor nor price the deal.
2. Resume is replayable: after approval, repeated resume calls re-run create_deal. Please add a consumed/terminal state (same pattern as the change-request idempotency fix in #64).
3. _parse_deal keeps only externalDealID and drops internalDealID, but get/update/clone need the internal ID per your own docstrings, so created deals can't be managed afterward. Suggest deal_id=str(internalDealID), external_deal_id=externalDealID.
4. Validate selected_dsp_id against the candidates recorded in the gate context (one-line membership check).
5. index_exchange_api_url now defaults to the production FQDN, which makes the factory's unconfigured-connector guard dead code and can send requests with no Authorization header to production IX on a misconfigured deploy. Please keep the default None.
6. SSPDealDistributeRequest.targeting narrowed dict to list without regenerating docs/api/openapi.json; please regenerate (note #83 adds a drift guard that will fail on this).

Worth fixing but not blocking: the ETag is fetched immediately before PATCH, which defeats lost-update protection (accept a caller-held ETag instead); CREATED/ARCHIVED list filters are silently dropped; unknown IX statuses default to CREATED rather than an error state; and a CHANGELOG entry is missing. The static-JWT limitation is documented and fine for now.

[josh-butcher-index]

Thanks for the thorough review, all six blocking items and the non-blocking ones are addressed in 00fae3d:

1. Operator gate. list_pending_approvals (MCP) is now operator-gated, and the REST /approvals routes (list, get, decide, resume) require an operator key. A buyer key gets 403 on each. I gated the GETs too since they return the same pricing snapshot.
2. Replay. A successful resume now marks the approval consumed before creating the deal, and a second resume returns 409. If creation fails, the approval goes back to approved so it can be retried. Note storage has no compare-and-set, so two truly simultaneous resumes could still race.
3. internalDealID. _parse_deal now sets deal_id=str(internalDealID) and external_deal_id=externalDealID. clone_deal derives its new external ID from external_deal_id accordingly.
4. selected_dsp_id. It must be one of the candidates recorded in the gate context, otherwise 400.
5. Default URL. index_exchange_api_url defaults to None again, so the factory's unconfigured-connector guard works.
6. OpenAPI. Updated for the targeting change and the approvals auth wording. The committed spec was already out of sync with the code before this PR, so I limited the edit to the parts this PR changes rather than regenerating the whole file. Happy to regenerate fully if you prefer.

Non-blocking: update_deal accepts a caller-held etag (get_deal exposes it as raw["_etag"]) and only fetches one when none is passed; list_deals raises for CREATED/ARCHIVED instead of silently dropping the filter; unknown IX statuses map to ERROR; CHANGELOG entry added. I left the seat-list size cap noted by @Kishore-2572 for a follow-up.

Tests added for each of the above.

[Kishore-2572]

Thanks @josh-butcher-index , I re-reviewed at 00fae3d.

Verified fixed

• Operator gate on all four /approvals routes and the list_pending_approvals MCP tool. Nothing else in seller-agent or buyer-agent calls these routes, so the stricter access breaks no caller.
• A second resume returns 409. I negative-controlled the test by removing the guard, and it fails as expected.
• index_exchange_api_url defaults to None again.
• The branch merges cleanly with current main; the full suite passes (1736), including the openapi drift guard.

Still needs changes

1. A lost IX response can lead to a duplicate deal (blocking)

_resume_ssp_distribution restores the approval to approved on any exception from create_deal. If IX accepted the POST but the response was lost (e.g. a read timeout), the next resume POSTs again.

I reproduced this with the real IndexExchangeSSPClient over an httpx.MockTransport that times out after receiving the POST: the approval goes back to approved, and IX receives 2 POSTs. Depending on whether IX de-duplicates externalDealID, that is either a duplicate deal or an approval that can never complete while a deal exists in IX that we never recorded. The existing tests mock create_deal, so they can't catch this.

Suggestion: only restore approved for errors known to happen before the request is sent (validation, connection errors). On a timeout or 5xx, look the deal up by externalDealID before allowing a retry.

2. modifications override the validated DSP choice

After the selected_dsp_id candidate check, every other key in modifications is setattr'd onto the create request:

modifications	Sent to IX
{"selected_dsp_id": 52, "dsp_id": 999}	dsp_id=999 (bypasses the new candidate check)
{"selected_dsp_id": 52, "cpm": "not-a-number"}	cpm="not-a-number"
{"selected_dsp_id": 52, "external_deal_id": "OTHER"}	external_deal_id="OTHER"

setattr on the Pydantic model doesn't validate. Suggestion: an allow-list of fields that may be supplied at decision time (never dsp_id or external_deal_id), and rebuilding the request through the model so values are validated.

3. targeting narrowing breaks non-IX SSPs

SSPDealCreateRequest.targeting and SSPDealDistributeRequest.targeting are shared by every SSP but now only accept IX's list shape. On main, a dict such as {"geo": ["US"]} is accepted; on this branch it is rejected for all SSPs, and the Magnite REST connector sends targeting straight through.

Suggestion: dict | list[dict] on the shared models, andnside the IX connector.

Minor

internalDealID is now returned as deal_id, but it isndeal record, so later get/update/clone still depends on the caller keeping it.

[josh-butcher-index]

Thanks for the re-review, and for reproducing the timeout with the real client. Addressing the remaining items in 5ec942a:

1. Lost IX response → duplicate deal. Agreed. IndexExchangeSSPClient.create_deal now treats a read timeout, dropped connection, or 5xx as "may have reached Index". It then looks the deal up with GET /v3/deals?search=<externalDealID>&accountIDs=<id> before deciding what to do. search is a substring match, so results are filtered to an exact externalDealID match and paged (up to 5 pages of 100).

• Deal found: returned as the created deal, so the approval completes.
• Deal not found: the original error is re-raised and the approval is restored to approved, so a retry is safe.
• Lookup fails, or no exact match within the page cap: a new SSPDealOutcomeUnknownError is raised. The approval stays consumed and the caller gets a 502 asking them to check Index before retrying.

4xx responses and connect errors never reached Index, so they skip the lookup. Tests drive the real client over httpx.MockTransport: timeout with the deal present, timeout with the deal absent, a decoy substring match, an exact match on page 2, 5xx with a failed lookup, and 4xx or connect error with no lookup.

2. modifications overriding the validated DSP. Agreed. Only an allow-list of fields can be supplied at decision time (name, cpm, account_id, advertiser, targeting, start_date, end_date, impressions_goal, buyer_seat_ids). dsp_id always comes from the validated selected_dsp_id, and external_deal_id can't be changed. The request is rebuilt through SSPDealCreateRequest.model_validate, so a bad value such as cpm="not-a-number" returns a 400 before anything is sent to Index.

3. targeting narrowing. Agreed. SSPDealCreateRequest.targeting is dict | list[dict] again, so dict-shaped targeting still works for other SSPs. The REST and MCP schemas are unchanged.

Minor (internalDealID as deal_id). Agreed that callers need to keep it for later get/update/clone. external_deal_id is returned alongside it.

[Kishore-2572]

Thanks Josh, I re-reviewed 5ec942a end to end: a real seller-agent server with real operator/buyer keys, the real IndexExchangeSSPClient, and a scriptable fake IX server over HTTP (dropped connections, 5xx/4xx, failing and lagging lookups). I also ran the full suite at the PR head (1647) and merged with current main (1750). Lint is clean.

Verified fixed

Item	Result
Operator gate	Buyer key → 403 on list, get, decide and distribute; no key → 401
Lost response after create	Lookup finds the deal, 1 POST to IX, approval completes, second resume → 409
Lost response before create	Approval restored to approved, retry creates exactly one deal
IX 4xx	No lookup; approval restored and retryable
Decoy substring match	Only the exact externalDealID is returned
modifications override	dsp_id: 999 and external_deal_id: "OTHER" ignored, the selected DSP is used; cpm: "not-a-number" → 400 with nothing sent to IX
internalDealID	deal_id is the internal ID, external_deal_id is the seller ID
Default URL / OpenAPI	Default is None; the drift guard passes on the merged result

Negative controls: disabling the post-failure lookup fails 3 tests, and letting modifications override dsp_id fails 1, so the new tests do protect the fixes.

Still needs changes

1. REST targeting is still list-only

The shared SSPDealCreateRequest accepts dicts again, but SSPDealDistributeRequest.targeting (schemas.py) and the MCP distribute_deal_via_ssp parameter are still list[dict]. POST /api/v1/deals/distribute with "targeting": {"geo": ["US"]} returns 422 on this branch; on main it is accepted. Dict-targeting callers (e.g. Magnite) are still broken at the API edge.

2. Seats can change after DSP resolution

buyer_seat_ids is in the decision-time allow-list, but dated against candidates resolved from the originalseats. With {"selected_dsp_id": 85, "buyer_seat_ids": ["seat-solo"]}, IX received dspID: 85 with a seat that only resolves to DSP 52.
Suggest removing buyer_seat_ids from the allow-list, ors against the selected DSP.

3. "Outcome unknown" has no recovery path

On 5xx + failed lookup, resume returns 502 and leaves themessage says to check Index before retrying, but a secondresume returns 409 and the approval can't be re-decided, so there's no way to retry or to record a deal ID that does exist in IX. Please add an operator path to either attach the found IX deal or reved, or at least document the manual step. ### Smaller (non-blocking) - Validation of selected_dsp_idandmodifications hapcide has already moved the approval out of pending. Abad selection or cpm gives 400 on resume, and the approval can't be corrected (re-decide → "already resolved"); the only way out is a new distribute.

• Keys outside the allow-list are dropped silently: a typo like cmp: 99 returns 200 with the original floor. Suggest rejecting unknown keys.
• On the direct (non-approval) distribute path, SSPDealOes as a bare **500**; only ValueError` is mapped there. A502 with the message would match the resume path.
• clone_deal generates a new random externalDealID peng a timed-out clone can't be de-duplicated by the lookup.

Pre-existing on main (not this PR, flagging for a follow-up issue)

distribute maps unknown deal_type values silently to PMP. In a cross-repo run, a deal sent with "deal_type": "PD" (buyer-agent's term) was created in IX as an auction deal and imported by buyer-agent as PA with a bid floor. With "PREFERRED" it round-trips correctly as PD with fixed_price_cpm. Negative cpm is also accepted, and IX 4xx responses surface as 500.

[josh-butcher-index]

Thanks for the thorough end-to-end re-review. Addressed in f1a001f:

1. REST/MCP targeting. Agreed, the shared request model was widened but the API edge wasn't. SSPDealDistributeRequest.targeting is now dict | list[dict], as is the MCP distribute_deal_via_ssp parameter. A dict body is accepted again, as on main.

2. Seats after DSP resolution. Agreed. buyer_seat_ids is removed from the decision-time allow-list, so the seats can't diverge from the ones the candidate DSPs were resolved from.

3. Outcome-unknown recovery. Agreed that 502 plus a permanently consumed approval was a dead end. An unconfirmed create now moves the approval to a new outcome_unknown status. Resuming an approval in that state re-runs only the externalDealID lookup and never POSTs. If the deal exists it is returned and the approval completes; if it's absent the approval is restored to approved (the call returns a 409 saying so) so the next resume can create it. If the lookup fails again the call returns the same 502 and the approval stays outcome_unknown.

Smaller items

• Validation moved to decision time: decide_approval now checks selected_dsp_id against the candidates and validates modifications while the approval is still pending, so a bad value is rejected and can be corrected. The resume-time checks remain as a backstop.
• Unknown modifications keys are rejected with a 400 instead of dropped, so cmp: 99 fails loudly.
• The direct distribute path now maps SSPDealOutcomeUnknownError to a 502 with the message, matching resume.
• clone_deal: within a single call the lookup does cover the clone, since create_deal uses the generated externalDealID. A caller retrying a whole clone gets a new ID, so for idempotent retries pass an explicit external_deal_id via overrides. Happy to document that if useful.

Pre-existing on main. Agreed these aren't from this PR (deal_type falling back to PMP, negative cpm, IX 4xx surfacing as 500). Keeping them out of this change; happy to open a follow-up issue.

[atc964]

Approve, with follow-ups.

Three solid fix rounds. Every blocking item from the October 22 review is verifiably fixed at f1a001f: the approval surfaces (REST and MCP) are operator-gated symmetrically, resume is consume-once with a lookup-only outcome_unknown recovery state, deal_id now carries Index's internalDealID with the seller ID in external_deal_id, selected_dsp_id is checked against the stored candidates at decide time and again at resume, modifications go through an allowlisted model_validate (and unknown keys now fail loudly), INDEX_EXCHANGE_API_URL has no production default, and the targeting union is consistent from ssp_base through the REST schema, the MCP parameter, and the regenerated openapi.json. The ETag handling is now real lost-update protection, and list_deals rejecting filters Index can't express beats silently returning everything. Credit to @Kishore-2572's end-to-end rig: the decoy-substring and lost-response scenarios reproduced against a real server are exactly the validation this needed, and the negative controls on the new tests are a nice touch.

Two conditions on the approval:

1. CI had not run at f1a001f (workflows awaiting fork approval; last green was 00fae3d). I've approved the workflow runs; needs a green run at head before merge.
2. Please open the follow-up issues you offered, covering: Keycloak JWT refresh (currently a documented manual step); the ungated MCP create_curated_deal, now slightly wider since it accepts buyer_seat_ids that distribute's stored-deal fallback will trust later; the non-CAS consumed-state race you've documented at approval_service.py:271 (including the wrinkle that a racing duplicate-create failure restores approved over consumed); and the pre-existing items from Kishore's list (deal_type to PMP fallback, negative cpm, IX 4xx as 500).

Small nits, fine to fix here or in the follow-up: MCP approve_or_reject validates the operator key but doesn't stamp decided_by_principal, so MCP-decided approvals audit as anonymous while REST records the verified principal; the decision string is still free-text ("aprove" silently records a rejection, worth an enum check in decide_approval); test_outcome_unknown_leaves_approval_consumed asserts outcome_unknown, so the name is stale. And one confirming sentence would help future readers: the recovery lookup assumes GET /v3/deals?search= matches externalDealID and that pageOffset is a page index. Can you confirm both against the real Deals API, since the fake-IX harness necessarily bakes in the same assumptions? All failure modes land safe-side regardless, so this is documentation, not a blocker.

Merge coordination: openapi.json overlaps #83 and #98, deal_service.py overlaps #92, and mcp_server.py/settings.py overlap #108/#107; whoever lands second rebases and (for openapi) regenerates.