Attribute GPT renders to Trusted Server on observed evidence - #997
Attribute GPT renders to Trusted Server on observed evidence#997prk-Jr wants to merge 32 commits into
Conversation
Give publisher operators an opt-in, tab-local view of GPT-observed slot lifecycles and exact DOM bindings without changing auction or publisher behavior. Keep capture bounded, export allowlisted facts only, and cover activation, correlation, presentation, and non-interference across unit and browser tests.
Scope the diagnostics navigation assertion to the fixture navigation landmark so Playwright does not match the separate site-wide Home link in CI.
The diagnostics console reported that a slot filled, but not what filled it, so an operator could not tell whether the line item Trusted Server won had rendered or whether a house ad, a direct-sold line item, or an Ad Manager default had taken the slot. Retain the Ad Manager identifiers slotRenderEnded already carries for the delivered ad, and resolve delivery from two observed facts: the Trusted Server GPT integration reporting which GPT slot carries a bid, and the rendered creative requesting its markup from Trusted Server. Only that creative asks, so a claim settles the Ad Manager decision, and its absence settles the opposite once the attribution window closes. Nothing is inferred from targeting, price, or timing, and an unattributable claim is preserved as an issue rather than attached to a guess.
The overlay commits on this branch are the pre-squash copies of #974, which landed on main as a single reviewed commit that also carries the review changes. Adopt main's tree so the superseded copies drop out; the delivery attribution work is re-applied on top of main's implementation.
slotRenderEnded reports the Ad Manager identifiers for the ad it delivered, but the observer read only isEmpty, size, isBackfill, and slotContentChanged, so a filled slot could not be told apart from a slot filled by Trusted Server. Capture those identifiers, derive a response class from them, and settle the delivery question with the one piece of observed evidence available: only the creative of the line item carrying Trusted Server targeting asks Trusted Server for its markup. The GPT integration reports the candidate slot at adInit and the markup request from the render bridge; both calls are optional and no-op in a tab without diagnostics activated. Nothing is inferred from targeting, price, or timing.
Bring the GPT delivery-evidence and refresh-attribution diagnostics onto the July release branch. rc/july already carried a later evolution of the auction-ID plumbing (PR #922 render tracing, the delivered-winner-slot telemetry, and the APS renderer bridge), so the overlapping Rust and JS changes keep the rc/july implementation and layer the new diagnostics on top of it. Conflict resolutions: - publisher.rs: keep the rc/july `write_bids_to_state` -> delivered winner slots contract and prebuilt page-bids bid map; keep both new tests; update the incoming test provider to the current `ProviderRequestOutcome` trait. - gpt/index.ts: keep the render-trace and APS renderer paths and add the creative request/response/failure diagnostics around them. The attempt is recorded after the APS branch, which is served by the APS universal creative and has no Trusted Server creative response to resolve. - prebid/index.ts: keep the resolved bare-refresh slot list and dispatch it through the diagnostics-aware refresh wrapper. - store.ts: adopt the incoming response-based `slotOnload` correlation. - Drop the pre-squash `gpt_diagnostics_bootstrap.js` and its test, which the upstream #974 squash removed as superseded by server-recognized activation. Verified: cargo fmt, clippy (fastly/axum/cloudflare/cloudflare-wasm/spin-native/ spin-wasm), test-fastly, test-axum, test-cloudflare, test-spin, parity, vitest (812 tests), JS and docs format.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Reviewed PR #997 against main. The implementation has a confirmed blocker: the new browser-visible auction metadata exposes the EC-derived request identifier. I also found three medium-severity diagnostics correctness/API/resource-bounding issues. Details and concrete fixes are attached inline.
aram356
left a comment
There was a problem hiding this comment.
Summary
The diagnostics evidence model (per-slot request intents, bounded attempt lifecycle, evidence-ladder delivery) is well designed and thoroughly tested, and the presentation layer is XSS-clean (every DOM insertion goes through textContent/constant setAttribute, closed shadow root). However, the auction-correlation feature undermines the PR's own privacy contract: hb_auction_id is derived from the EC ID and ships to all users, and the type-level guards meant to protect the export surface are not enforced by any gate. Requesting changes on those grounds plus a few doc/test integrity issues; the rest is non-blocking.
Blocking
🔧 wrench
hb_auction_idexposes the HttpOnly EC ID to page JS and the diagnostics export (crates/trusted-server-core/src/publisher.rs:3297, inline comment)- Privacy type contract unenforced; one assertion already broken (crates/trusted-server-js/lib/test/integrations/gpt_diagnostics/types.test.ts:129, inline comment)
- Replacement-after-eviction test passes vacuously (crates/trusted-server-js/lib/test/integrations/gpt_diagnostics/store.test.ts:887, inline comment)
- Operator guide contradicts the implementation (docs/guide/integrations/gpt-diagnostics.md:399, inline comment)
Non-blocking
🤔 thinking
- Empty render leaves its creative attempt live (store.ts:621, inline comment)
- Pass-through publisher refreshes labeled
prebid_refresh(prebid/index.ts:1274, inline comment)
♻️ refactor
- Snapshot recomputed per subscriber:
scheduleNotificationcallsthis.snapshot()inside the listener loop (api.ts:233-235). Each subscriber gets a differentcapturedAt, and at the 5sTRUSTED_SERVER_ATTRIBUTION_WINDOW_MSboundary two subscribers in the same notification can disagree onpendingvscandidate_unconfirmed. It is also O(subscribers × slots × cycles). Hoist oneconst snapshot = this.snapshot();above the loop (in its own try so a snapshot failure still cannot escape). - Coverage gaps in new code paths (all verified uncovered via V8 coverage):
recordPublisherRefreshmalformed inputs (store.ts:341, 343) - the writer fed directly from publisher-suppliedrefresh()arguments is the only one of the three intent writers without a malformed-input test.MAX_TRUSTED_SERVER_ASSOCIATIONSLRU trim (store.ts:317-321) - the only bound in the file with no test; a regression silently drops the oldestauctionSlotIdassociation on long-lived SPAs.- WeakRef-collected expiry path (store.ts:880, 884) - the WeakRef-absent fallback is tested, but not a
WeakRefwhose target was collected, which is the case the WeakRef exists for. invalid_event_orderarms forslotRenderEnded/slotOnload/impressionViewable(store.ts:642-644, 661-663, 681-683) and the non-finite-percentage arm ofrecordSlotVisibilityChanged(store.ts:700-710).recordTrustedServerCreativeFailurewith an unknown attempt ID (store.ts:502-503);safelyRecordCreativeFailureearly return when the attempt was never created (gpt/index.ts:1173);badges.ts:123incomplete-sequence line; overlayyieldGroupIds/companyIdsfacts (overlay.ts:206-208).
- Test hygiene bugs:
- badges.test.ts:276:
gptDiagnosticsBadgeTextForTest.toString()returns onlybadgeText's own body, so thenot.toMatch(/GAM winner|bidder|provenance/i)guard cannot seedeliveryLabel, which is where the new delivery vocabulary lives. Assert over rendered output instead. - overlay.test.ts:327: the store is constructed without a
deferstub, sorecordPublisherRefreshschedules a real 5000mssetTimeoutthat outlives the test. Adddefer: () => undefinedmatching overlay.test.ts:119.
- badges.test.ts:276:
⛏ nitpick
- Tautological assertions: types.test.ts:124-128 and 135-137 assert hand-written literals against themselves (the identity
toBechecks, thetoHaveLength(8)on a literal array, and the blocklist regexes over objects the test itself wrote constrain nothing about production types); store.test.ts:622'snot.toContain('other_demand')cannot fail since that string exists nowhere in the codebase; badges.test.ts:131, 144, 220-232 addnot.toMatchguards on strings already pinned by exacttoBe. - Mega-tests: overlay.test.ts:113-322 (one
it, ~50 assertions across 9 scenarios), observer.test.ts:131-164 (5 behaviors with interleaved arrange steps), prebid/index.test.ts:1549-1614 (4 scenarios sharing mutable state) would be cleaner and more diagnosable asit.each, which these same files already use well elsewhere.
📝 note
- Stale PR description: the GitHub PR body still documents the deleted 5-state delivery ladder (
trusted_server/other_demand); the shipped code and guide use the 7-state ladder (trusted_server_response_sent,trusted_server_selected,candidate_unconfirmed,no_candidate,unknown,pending,not_applicable). Worth rewriting before merge. - Spec hygiene: the 2026-08-04 design spec declares a 4-value
GptDiagnosticsRequestPathunion with no forward pointer to the 2026-08-05 extension that addedpublisher_refresh; a one-line "Extended by ..." note would prevent a reader landing there first from getting a stale union. Both spec statuses (Proposed/Approved) should becomeImplementedwhen this merges, matching existing repo practice.
🌱 seedling
- Open Bidding classification: a yield-group render (
yieldGroupIdspresent, no line-item/creative ID) classifies asunclassified_non_emptydespite carrying positive Ad Manager evidence; a distinct response class may be worth adding later.
CI Status
- fmt: PASS
- clippy (all adapters): PASS
- rust tests (fastly/axum/cloudflare/spin/parity/CLI): PASS
- js tests (vitest): PASS
- browser + Fastly EC integration tests: PASS
- CodeQL: PASS
The winning-bid `hb_auction_id` carried `AuctionRequest.id`, which is
`ts-{ec_id}` whenever an Edge Cookie ID exists. That value reached
`window.tsjs.bids` and the page-bids JSON for every visitor, handing any
script on the page the identifier the `ts-ec` cookie keeps HttpOnly, and
it could not distinguish one auction from the next because it is stable
per visitor.
Mint an unrelated `ts-auc-{uuid}` per auction instead, and emit it only
when the GPT diagnostics integration is enabled, since nothing else
consumes it. `AuctionRequest.id` is unchanged for SSPs.
Keep the operator API read-only: `window.tsjs.gptDiagnostics` now exposes only snapshot, export, subscribe, show, and hide. The evidence writers Trusted Server's own modules use move to a separate internal channel, `window.tsjs.gptDiagnosticsRecorder`, so the documented contract matches what the object actually offers. Bound the store's deferred work by retained state rather than refresh rate. Request-intent evidence now expires lazily when the slot is next recorded or requested instead of owning a timer per source, which also removes the WeakRef-absent fallback that strongly retained every marked slot until expiry. Delivery-boundary notifications share one timer that re-arms from retained cycles. Stop reporting a source-agnostic GPT identifier as a reservation. Those IDs are populated for reservation and backfill alike, so they classify as `reservation` only alongside an explicit non-backfill fact. Evict a creative attempt whose cycle rendered empty, so a late markup response cannot claim a Trusted Server delivery against an empty render, and label badges from the derived delivery state instead of re-deriving the precedence rules from raw timestamps. Also: attribute `refresh(null)`, make the presentation switches exhaustive, hoist the snapshot out of the subscriber loop so every subscriber sees one capture, and make the export's attribution fields required. Type-check the export contract: the `expectTypeOf` assertions were never evaluated, and one of them was a genuine error. Scope `test.typecheck` to the type tests, since a package-wide `tsc --noEmit` still fails on pre-existing errors elsewhere. Fix the replacement-after-eviction test, which passed vacuously because ten open cycles made the final render ambiguous, and cover the paths the review identified: publisher-refresh malformed input, the association LRU trim, the out-of-order callback arms, a declined creative attempt, the Prebid dispatch-context restore arm, and wrapper install ordering. Correct the operator guide where it contradicted the implementation, and record the auction-token revision in the design spec.
Conflict in the Prebid refresh handler: main (#965) documented that the delegated refresh preserves the publisher's original bare form, while this branch replaced that call with the diagnostics recording plus the scoped dispatch context. Both hold — `dispatchPrebidRefresh` passes `slots` and `opts` through unchanged — so the resolution keeps the diagnostics calls and main's comment, extended to say the wrapper only scopes the shared context. Also add `bid_id` to the auction-ID test provider's `Bid` literal, a field main added in #996 after this branch introduced the provider.
Brings in the PR #997 review fixes: the read-only diagnostics facade split from the internal recorder channel, lazily expiring request-intent evidence with one shared delivery-boundary timer, source-agnostic IDs no longer reported as reservations, the empty-render attempt eviction, and the scoped type-check gate for the export contract. The branch also carries a merge of main, whose #965 and #996 arrive here as squashes of work rc/july already implements more fully. Where the two sides describe the same feature, rc/july's implementation is kept: - APS, adserver_mock, auction/types.rs, auction/formats.rs — rc/july's OpenRTB provider, renderer-aware bid_id precedence, and typed renderer envelope supersede main's versions, which drop fields rc/july needs. - prebid.rs — rc/july canonicalizes the excluded-suffix list at both the startup and build paths already, so main's `load_config` helper adds nothing. Main's test is taken instead of rc/july's: it builds from raw settings rather than reusing the config `validate_config_for_startup` already canonicalized, so it actually exercises the build path. - prebid/index.ts — a bare refresh that filtered slots must deliver the resolved target list, not stay bare, so rc/july's `deliveredSlots` behavior and its test expectation both stand. Three fixes are ported into rc/july's shapes rather than resolved away: - The EC-derived auction ID reached page JavaScript here too, through different plumbing: both collect paths inlined `request.id.as_str()` into `write_bids_to_state`, and page-bids passed it to `build_bid_map_with_auction_id`. All three now mint a per-auction token via `diagnostics_auction_id()`, gated on the diagnostics integration being enabled. - A blank Prebid Cache UUID no longer ships cache coordinates. It loses the hb_adid precedence to `adid` or the bid id, so the Universal Creative would fetch `?uuid=<non-cache-id>` and miss instead of using the inline adm. The gate moves from `is_some()` to `non_empty()`, and main's regression test comes along with rc/july's `Bid` fields added. - The browser-side excluded-suffix list is validated before use. The server only de-duplicates it, so an empty suffix matched every ad unit path and pulled every slot out of the refresh auction, and a non-array value threw inside the publisher's own `refresh()`. Also collapses a duplicated `hb_auction_id` write in `build_bid_map` down to one guarded insert, and points the Prebid refresh recorder at `gptDiagnosticsRecorder` to match the new channel.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed the current PR #997 head against main, including the final fixes for earlier feedback. The evidence model and safety hardening are strong, and CI is green. I found one source-compatibility regression and one subscriber-isolation issue; neither warrants blocking this otherwise solid change.
| }; | ||
| slots: GptDiagnosticsSlotExport[]; | ||
| callbackIssues: GptDiagnosticsCallbackIssue[]; | ||
| attributionIssues: GptDiagnosticsAttributionIssue[]; |
There was a problem hiding this comment.
P1 — Preserve the V1 export's source compatibility. attributionIssues did not exist on GptDiagnosticsExportV1 in main, so requiring it (and metadata.droppedAttributionIssues below) makes existing V1 object literals fail TypeScript compilation. The implementation plan explicitly calls for these additions to be optional while snapshots continue to emit them.
Suggested fix: Mark both fields optional and add a type test that constructs the legacy V1 shape without them.
| return; | ||
| } | ||
|
|
||
| for (const listener of this.listeners) { |
There was a problem hiding this comment.
P2 — Isolate subscribers from one another's mutations. Every listener receives this same mutable snapshot object. A listener registered first can mutate nested evidence before a later listener runs, so the later subscriber observes data that was not captured by diagnostics.
Suggested fix: Capture once for a consistent capturedAt, then deep-clone per listener (or deep-freeze the shared value), with a two-listener regression test.
| include: ['test/**/types.test.ts'], | ||
| // Errors reported from files outside `include` are pre-existing and | ||
| // unrelated; only the type assertions in the included files gate here. | ||
| ignoreSourceErrors: true, |
There was a problem hiding this comment.
🌱 seedling - With ignoreSourceErrors: true and only types.test.ts in scope, the new unhandledCase never-guards in overlay.ts/badges.ts surface in the IDE but not in CI: a new GptDiagnosticsDelivery/GptDiagnosticsRequestPath/GptDiagnosticsResponseClass member compiles, builds, and passes this gate. The union memberships pinned in types.test.ts today are only GptDiagnosticsAttributionIssueReason and the API/recorder keys. A follow-up that pins the remaining unions the same way (expectTypeOf<GptDiagnosticsDelivery>().toEqualTypeOf<...>()) would make a member addition fail CI and point the author at the switches.
|
|
||
| The derived `delivery` value uses these evidence-safe meanings: | ||
|
|
||
| | Delivery state | Panel wording | |
There was a problem hiding this comment.
📝 note - This table is the shipped 7-state ladder, but the GitHub PR description still documents the earlier 5-state ladder (trusted_server / other_demand). Worth rewriting the PR body before merge so the merge-commit reference material matches what shipped.
Summary
This PR extends the opt-in GPT diagnostics so an operator can answer two separate questions without inferring from targeting, price, or timing:
The implementation remains observational and requires zero publisher-code changes. Diagnostics must not suppress, delay, reorder, add, or remove GPT requests; change targeting; alter auctions or timeouts; or affect creative rendering.
Evidence reported
For every retained GPT request cycle, diagnostics can now report:
empty,backfill,reservation, orunclassified_non_emptytrusted_server_direct,prebid_refresh,publisher_refresh,competing, orunattributedloadObservedBeforeRender, without fabricating a negative duration or marking the cycle incompleteCalls that bypass every installed observation boundary remain honestly
unattributed. Overlapping request cycles remainambiguous; diagnostics does not guess to improve coverage counters.Refresh-source attribution
Request triggers are represented as short-lived, per-slot intents. Trusted Server, Prebid, and publisher evidence expire independently and the next matching
slotRequestedconsumes the complete intent.The diagnostics integration installs a standalone observer around the current
pubads.refreshbefore the deferred Prebid wrapper is installed. The observer:getSlots()adInitrefresh and during a Prebid-managed delegationPrebid exposes only a synchronous diagnostic context around the delegated refresh. It is restored after normal completion and throws, including hostile setter/proxy behavior, and cannot influence targeting, auction timing, callbacks, or whether refresh runs.
Pending intent expiry is coalesced to one callback per slot/source.
WeakRefavoids retaining slots where supported; older runtimes use a bounded five-second strong-reference fallback.Auction correlation
The server copies the existing
AuctionRequest.idinto optionalhb_auction_idwinning-bid metadata for both initial-document and page-bids responses. The value is used only as opaque diagnostic correlation data; it does not affect bid selection or GAM targeting, and no auction payload is retained.Creative and replacement attribution
The existing delivery ladder remains evidence-based:
trusted_serverother_demandno_candidatependingnot_applicableReplacement comparison uses GPT's primary creative ID with its source-agnostic creative ID as the fallback. Missing or one-sided evidence is omitted rather than described as a change.
Main changes
This branch deliberately does not import the
rc/julyGPT slot-handoff implementation or APS renderer behavior.Verification
cargo fmt --all -- --checkcargo test-fastly— Fastly adapter, core, OpenRTB, and doctestscargo test-axumcargo test-cloudflarecargo test-spincargo clippy-fastlycargo clippy-axumcargo clippy-cloudflare?ts_console=trueon representative GAM delivery pathsSafety and privacy
Relationship to earlier PRs
This remains the attribution follow-up to #974 and supersedes the closed pre-squash copies in #976 and #990.