Repository navigation
Conversation
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Spec-only PR for #361, reviewed against main at 7aa5c993b. The design holds together and most of its current-behavior claims check out in the code. Several contract points are wrong when checked against PBS, quick-xml, and the source, and the caller contract does not yet carry the security and macro obligations the design relies on.
23 of the 30 inline comments carry a one-click GitHub
suggestion. Each was applied alone, and all together, to a scratch copy and passesprettier --checkwithdocs/.prettierrc. Use Commit suggestion (or Add suggestion to batch) to apply them. The other comments describe the change in prose because it spans several sections; where a suggestion also needs a matching edit elsewhere, the comment says "apply manually".
Blocking
🔧 wrench
- 🔧 The caller contract does not support allowing
sanitize_creatives = true(inline atL769-L772) - 🔧 PBS stored-impression clearing only works on PBS Go, and imp-level fields still leak (inline at
L527-L535) - 🔧 The macro contract covers only
${AUCTION_PRICE}, and the caller cannot resolve the rest (inline atL779-L782) - 🔧 Adjusted or converted PBS bids are cheap to detect, so enforce the restriction (inline at
L551-L554) - 🔧 VAST 1.0 protocols can never pass the
VASTroot check (inline atL271) - 🔧 11.4 is written against the pre-0.38 quick-xml API (inline at
L645-L649) - 🔧 The prolog rule admits malformed XML (inline at
L640-L641) - 🔧 The measurement gate contradicts 18.5, and the cost bound can be stated now (inline at
L665-L667) - 🔧 The routing diagnostics claims are false (inline at
L469-L472) - 🔧
AdSlot.videohas a second producer (inline atL411-L414) - 🔧 Config compatibility understates the blast radius, and the kill switch fails for video-only providers (inline at
L208-L210) - 🔧 The invalid-traffic text and the page-bids gate description are inaccurate (inline at
L814-L818) - 🔧 Mixed-slot classification is ambiguous (inline at
L588) - 🔧 The link to the browser-blocked domain rewriting design is broken (inline at
L1061-L1064)
❓ question
- ❓ Open question 1 (video when
ec_allowed()is false) needs an answer before approval (inline atL1074)
Non-blocking
- 🤔 Should banner and instream be one unit at all? (inline at
L132) - 🤔 The sound-off cross-field rule is stricter than AdCOM (inline at
L294-L296) - ♻️
api: []is stated three different ways (inline atL277) - ♻️
VideoPlacementserde does not round-trip (inline atL356) - ♻️ The 11.3 out-of-range rules conflict (inline at
L625-L626) - ♻️ The Fastly-only reason and open question 3 misdescribe deadlines (inline at
L1078-L1079) - ♻️ Caller lifecycle: expiry during wrapper load, notice transport, late responses (inline at
L783-L785) - ♻️ Consent synchronization names nothing the caller can check (inline at
L773-L776) - ♻️ Integrators need response examples and status codes (inline at
L700) - ♻️ 7.5 error mapping and paths are underspecified (inline at
L317) - ♻️ Video is invisible to operators of the experimental feature (inline at
L984) - ♻️ 18.5 validation cannot inspect what it claims (inline at
L954) - 🤔 Banner
nurl/burlfor every service (inline atL974) - ⛏ The first-use-case reason contradicts section 1 (inline at
L125) - ⛏ "Resolved:" renders inside item 4 (inline at
L1080-L1083)
Cross-cutting / body-level findings
- 📝 Docs deliverables in 19.3 are incomplete. Add
docs/guide/telemetry.md,docs/guide/error-reference.md,docs/guide/integrations/prebid.md:440("video and native are currently not emitted" becomes wrong), the Cloudflare and Spin guides (video not supported in production),api-reference.mdfor the new bannermtype/nurl/burl, andauction-orchestration.md's "Floor prices can be set per-slot in the auction request", which is already false (/auctionsetsfloor_price: None). 18.6 should point at the AGENTS.md CI gates, not CLAUDE.md. - 📝 Review process. The review log has one round and the status line says "draft v3"; CONTRIBUTING requires at least two agent rounds, ending with one that finds nothing blocking. Please record this review in the log.
- 📝 Unpublished source. The Related list and open question 1 cite an earlier draft by name that reviewers cannot read. Since this spec supersedes it, drop the reference and state open question 1's alternative directly.
CI Status
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
- Analyze (rust): PASS
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS (required)
- cargo test: PASS (required)
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- CLAUDE.md symlink guard: PASS
- CodeQL: PASS
- docs-proposal scripts: PASS
- format-docs: PASS (required)
- format-typescript: PASS (required)
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- vitest: PASS
| - Disable VPAID in the player or ad SDK before initialization (for IMA, | ||
| `VpaidMode.DISABLED`). Declared APIs cannot exclude VPAID, because a VAST can | ||
| carry an undeclared VPAID media file or a wrapper can resolve to one. | ||
| - Never render `adm` as HTML. |
There was a problem hiding this comment.
🔧 wrench: The caller contract does not support allowing sanitize_creatives = true
The decision (4, 12.4, 22) rests on VAST never being rendered as HTML on the publisher origin, but section 13 only says "Never render adm as HTML". IMA's documented companion path writes companionAd.getContent() into the page DOM, GPT companion slots auto-render HTMLResource and iFrameResource companions, OMID full access gives verification scripts page access, and a SIMID creative needs a cross-origin iframe. videojs-ima's vpaidMode defaults to enabled. Please also reword the reason in 4, 12.4, and 22: sanitization covers HTML that Trusted Server delivers, HTML inside VAST is safe only under the section 13 rules, and the sanitize_creatives guarantee does not extend to video. Turning this into a config error instead would push sanitizing operators to switch banner sanitization off, so caller rules plus documentation is the better fix.
| - Disable VPAID in the player or ad SDK before initialization (for IMA, | |
| `VpaidMode.DISABLED`). Declared APIs cannot exclude VPAID, because a VAST can | |
| carry an undeclared VPAID media file or a wrapper can resolve to one. | |
| - Never render `adm` as HTML. | |
| - Disable VPAID in the player or ad SDK before initialization (for IMA, | |
| `VpaidMode.DISABLED`; for videojs-ima, set `vpaidMode` to `DISABLED`). | |
| Declared APIs cannot exclude VPAID, because a VAST can carry an undeclared | |
| VPAID media file or a wrapper can resolve to one. | |
| - Dispatch on `mtype`. Never write the `adm` of an `mtype: 2` winner into an | |
| HTML sink (`innerHTML`, `document.write`, `srcdoc`, or a same-origin iframe). | |
| - Render no companion or nonlinear creative. With IMA, do not call | |
| `getCompanionAds`, and do not declare GPT companion slots on pages that play | |
| these ads. | |
| - Keep OMID verification at limited access, the IMA default. Never grant full | |
| access through `omidAccessModeRules` or the videojs-ima equivalent. | |
| - Load SIMID creatives only in a cross-origin iframe. |
| - Other impression-level fields (`imp.ext`, `tagid`, `bidfloor`) are left as | ||
| they are, so stored bidder configuration and floors still apply. | ||
|
|
||
| PBS treats `null` as removal and replaces arrays whole when it merges. The | ||
| generated types skip `None` and empty `Vec` fields on serialization, so the PBS | ||
| profile writes the authoritative form as a `serde_json::Value` step after | ||
| serialization. A unit test asserts the complete `imp.video` key set against the | ||
| generated type, so a codegen update that adds a field fails the test until the | ||
| field is handled. |
There was a problem hiding this comment.
🔧 wrench: PBS stored-impression clearing only works on PBS Go, and imp-level fields still leak
Checked in both servers' source. PBS Go merges jsonpatch.MergePatch(storedImps[id], impData.Imp) (endpoints/openrtb2/auction.go:1755), so null removes keys. PBS Java merges a typed Imp through JsonMerger.merge (StoredRequestProcessor.java:300), which calls valueToTree on a mapper configured with JsonInclude.Include.NON_NULL (ObjectMapperProvider.java:26). The nulls are dropped before the merge, so stored banner, placement, skip fields, audio, and native survive. Separately, stored instl, rwdd, ssai, exp, and pmp survive on both implementations: they relabel the break, and a deal's at overrides first price. Clearing stored battr also removes a block list.
| - Other impression-level fields (`imp.ext`, `tagid`, `bidfloor`) are left as | |
| they are, so stored bidder configuration and floors still apply. | |
| PBS treats `null` as removal and replaces arrays whole when it merges. The | |
| generated types skip `None` and empty `Vec` fields on serialization, so the PBS | |
| profile writes the authoritative form as a `serde_json::Value` step after | |
| serialization. A unit test asserts the complete `imp.video` key set against the | |
| generated type, so a codegen update that adds a field fails the test until the | |
| field is handled. | |
| - `imp.instl` is `0`, `imp.rwdd` is `0`, `imp.ssai` is `1`, and `imp.exp` is | |
| `video_max_exp_seconds`, so stored values cannot relabel the break as | |
| interstitial, rewarded, or server-stitched. `imp.pmp` is `null`, and stored | |
| impressions used by video units carry no deals, because a deal's `at` | |
| overrides the request's first-price declaration (12.3). | |
| - `imp.video.battr` is omitted rather than cleared, so a stored block list | |
| survives. | |
| - Other impression-level fields (`imp.ext`, `tagid`, `bidfloor`) are left as | |
| they are, so stored bidder configuration and floors still apply. | |
| PBS Go merges the raw incoming impression as an RFC 7386 merge patch, so `null` | |
| removes a key and arrays replace whole. PBS Java deserializes the request and | |
| serializes it back without nulls before merging, so there only explicit values | |
| and `[]` override storage and `null` clears nothing. Video units sent to a PBS | |
| Java provider must not use the stored-impression fallback unless the stored | |
| impression holds only `imp.ext`. Trusted Server cannot tell the two apart, so | |
| this is an operator rule, and 18.5 runs against both. | |
| The generated types skip `None` and empty `Vec` fields on serialization, so the | |
| PBS profile writes the authoritative form as a `serde_json::Value` step after | |
| serialization. The patch destructures the generated `Video` exhaustively (no | |
| `..`), so a codegen update that adds a field fails to compile until the field | |
| is handled. |
| - Treat `nurl` as a win notice, not a VAST tag URL. Fire `burl` at the VAST | ||
| Impression event. Resolve `${AUCTION_PRICE}` in both, and in VAST, at the | ||
| returned `price`, which is the clearing price under first-price requests | ||
| (12.3). |
There was a problem hiding this comment.
🔧 wrench: The macro contract covers only ${AUCTION_PRICE}, and the caller cannot resolve the rest
OpenRTB 2.6 section 4.4 says exchanges must support every macro for which data is available, and unavailable optional values become empty strings. Players such as IMA and videojs-ima resolve VAST [MACROS], not ${...}. In the code, every provider receives BidRequest.id = AuctionRequest.id (auction/openrtb.rs:407), the same value as the response id (auction/formats.rs:481), so ${AUCTION_ID} is resolvable. The upstream response bidid is never retained, and the response seat is Bid.bidder ("unknown" when absent, the bidder code for PBS), so ${AUCTION_BID_ID} and ${AUCTION_SEAT_ID} are not. Returning the upstream bidid and seat in bid ext would be the alternative to empty strings.
| - Treat `nurl` as a win notice, not a VAST tag URL. Fire `burl` at the VAST | |
| Impression event. Resolve `${AUCTION_PRICE}` in both, and in VAST, at the | |
| returned `price`, which is the clearing price under first-price requests | |
| (12.3). | |
| - Treat `nurl` as a win notice, not a VAST tag URL. Fire `burl` at the VAST | |
| Impression event. Players do not substitute OpenRTB `${...}` macros, so before | |
| firing either, and before passing `adm` to the player, replace | |
| `${AUCTION_PRICE}` with `price`, `${AUCTION_ID}` with the response `id` | |
| (Trusted Server sends the same ID to every provider), `${AUCTION_IMP_ID}` with | |
| `impid`, `${AUCTION_AD_ID}` with `adid`, and `${AUCTION_CURRENCY}` with `USD`. | |
| Replace every other OpenRTB macro, including `${AUCTION_BID_ID}`, | |
| `${AUCTION_SEAT_ID}`, and encoded `${NAME:X}` forms, with an empty string, as | |
| OpenRTB 2.6 section 4.4 directs for unavailable values. `price` is the | |
| clearing price under first-price requests (12.3). |
| PBS accounts that adjust or convert video bids are unsupported in this phase. | ||
| The configuration guide and the Prebid integration guide state this. Trusted | ||
| Server does not detect or reject these bids. A settlement price returned | ||
| alongside `price` is a future-state option (20.3). |
There was a problem hiding this comment.
🔧 wrench: Adjusted or converted PBS bids are cheap to detect, so enforce the restriction
PBS Go writes ext.origbidcpm whenever it is known (exchange/exchange.go:1385) and origbidcur whenever it is non-empty, and PBS Java sets both in BidAdjustmentsProcessor. "Documented, not enforced" means wrong billing figures ship silently. Please also add VideoPriceAdjusted / video_price_adjusted to the 11.5 table (apply manually; it is outside this range).
| PBS accounts that adjust or convert video bids are unsupported in this phase. | |
| The configuration guide and the Prebid integration guide state this. Trusted | |
| Server does not detect or reject these bids. A settlement price returned | |
| alongside `price` is a future-state option (20.3). | |
| PBS accounts that adjust or convert video bids are unsupported in this phase. | |
| Both PBS implementations write `ext.origbidcpm` and `ext.origbidcur` on every | |
| bid, so detection is one comparison: a PBS video bid whose `ext.origbidcpm` is | |
| present and differs from `price`, or whose `ext.origbidcur` is present and is | |
| not `USD`, rejects as `video_price_adjusted`. The configuration guide and the | |
| Prebid integration guide state the restriction. A settlement price returned | |
| alongside `price` is a future-state option (20.3). |
|
|
||
| ## 21. Open questions | ||
|
|
||
| 1. Should video be withheld when `ec_allowed()` is false? Aram's draft says yes. |
There was a problem hiding this comment.
❓ question: Open question 1 (video when ec_allowed() is false) needs an answer before approval
Consent is on the CONTRIBUTING spec checklist, so this needs a decision rather than an open question. I'd withhold video when ec_allowed() is false (US-state GPC, sale or share opt-out, no signals). Banner in that state sends no EC or EIDs, and with rewrite_creatives on its static assets go through /first-party/proxy, which drops the client's XFF and strips Set-Cookie. Video does the opposite: the browser hands its IP and cookies directly to every wrapper hop, tracker, media host, OMID vendor, and notice host, which is the disclosure an opt-out of sale or share is meant to prevent. The fill cost on US no-signal traffic is acceptable for an experimental feature, and the rule can be relaxed once first-party VAST delivery (20.3) exists. Proposed text: "When ec_allowed() is false, the video half of each slot is removed before routing (mixed to banner, video-only to no bid), counted as video_withheld_consent."
|
|
||
| ### 19.2 Deploy and rollback | ||
|
|
||
| 1. Deploy the binary with no config change. Video is off everywhere. Confirm |
There was a problem hiding this comment.
♻️ refactor: Video is invisible to operators of the experimental feature
Rejections appear only in the per-response ext; the debug logs are invisible at production Info level; an invalid-video 400 creates no telemetry row, because conversion fails before the observation exists; no row counts requested video slots; and auction_bid_stats_mv and seat_yield have no media_type dimension, so video CPMs blend into the banner guardrail. The 24-hour "unchanged" check here also cannot hold, because 11.1 and the at: 1 prerequisite move banner numbers by design. Proposed: ship at: 1 in an earlier release and let it soak; filter the guardrails to media_type = 'banner'; add one Info line per provider with rejection counts by reason; move the 20.2 event kinds ahead of production opt-in.
|
|
||
| - VAST check timing under Viceroy for a 10 KiB and a 1 MiB fixture, recorded in | ||
| the implementation PR. | ||
| - A repeatable run against a real Prebid Server with stored auction responses |
There was a problem hiding this comment.
♻️ refactor: 18.5 validation cannot inspect what it claims
A PBS storedauctionresponse returns stored seatbids without calling bidders, so there is no resolved bidder request to inspect; use storedbidresponse, or ext.debug.resolvedrequest on staging. The parity harness defines no providers (tests/parity.rs:33-52), so it never sees a video-enabled service. The reference-player run has no pass criterion (proposed: exactly one nurl, burl, and Impression request per played ad, and none when unplayed), and it loads IMA from Google's CDN, so mark it as manual evidence attached to the implementation PR.
|
|
||
| ### 19.1 Changes for every service | ||
|
|
||
| - Banner API winners gain `mtype: 1`, `nurl`, and `burl`. |
There was a problem hiding this comment.
🤔 thinking: Banner nurl/burl for every service
Neither tsjs consumer fires them, so for services that only use /auction this adds a billing trigger that any HTTP client can fire, and banner ends up with two macro contracts: server-expanded on SSAT, caller-expanded on the API. Consider limiting the new banner fields to video-enabled services, and in any case return only absolute https: notice URLs of at most 4096 bytes.
|
|
||
| | Decision | Choice | Reason | | ||
| | ------------------------------------------- | ---------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | First use case | Linear instream, `plcmt` 1 or 2, direct API only | Smallest contract with a real consumer; browser paths need their own render design. | |
There was a problem hiding this comment.
⛏ nitpick: The first-use-case reason contradicts section 1
Section 1 says no publisher is lined up to adopt it.
| | First use case | Linear instream, `plcmt` 1 or 2, direct API only | Smallest contract with a real consumer; browser paths need their own render design. | | |
| | First use case | Linear instream, `plcmt` 1 or 2, direct API only | Smallest contract a direct-API player can consume; browser paths need their own render design. | |
| 4. Should the experimental label come off on fixed adoption criteria (provider | ||
| acceptance of `imp.video`, delivered-to-impressed conversion, time to first | ||
| frame, eCPM against the publisher's previous path)? | ||
| Resolved: |
There was a problem hiding this comment.
⛏ nitpick: "Resolved:" renders inside item 4
Making it a heading separates the resolved list from the open questions.
| 4. Should the experimental label come off on fixed adoption criteria (provider | |
| acceptance of `imp.video`, delivered-to-impressed conversion, time to first | |
| frame, eCPM against the publisher's previous path)? | |
| Resolved: | |
| 4. Should the experimental label come off on fixed adoption criteria (provider | |
| acceptance of `imp.video`, delivered-to-impressed conversion, time to first | |
| frame, eCPM against the publisher's previous path)? | |
| ### Resolved |
Summary
POST /auction, auctioned throughstandardandprebid-serverproviders that opt in, and returned as inline VAST to the caller's player.mediaopt-in).Changes
docs/superpowers/specs/2026-10-20-361-instream-video-auction-api-design.mdKey decisions in the spec:
media = ["banner", "video"], validated against the media its profile declares. A service with no video provider behaves asmain.mainadmits, apart from bids that explicitly declare non-banner media.quick-xml, new dependency).at: 1) are a blocking prerequisite, because the caller substitutes${AUCTION_PRICE}with the returned price.Closes
Refs #361. Target design for #1325.
Spec
Spec:
docs/superpowers/specs/2026-10-20-361-instream-video-auction-api-design.mdTest plan
Checklist