Implement server-side ad slot templates with PBS and APS auction#680
Implement server-side ad slot templates with PBS and APS auction#680prk-Jr wants to merge 140 commits into
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Incorporate all review feedback (aram356 + jevansnyc): cache contract, consent/GDPR gating, async restructuring detail, CreativeOpportunityFormat schema, glob pattern fix, XSS escaping, win notifications, APS params, timeout config key, defineSlot fix, gpt.rs ownership, KV migration path, Phase 2 sketch - Fix Prettier formatting (format-docs CI) - Add implementation plan (12 tasks, TDD, ordered by dependency)
- Incorporate all review feedback (aram356 + jevansnyc): cache contract, consent/GDPR gating, async restructuring detail, CreativeOpportunityFormat schema, glob pattern fix, XSS escaping, win notifications, APS params, timeout config key, defineSlot fix, gpt.rs ownership, KV migration path, Phase 2 sketch - Fix Prettier formatting (format-docs CI) - Add implementation plan (12 tasks, TDD, ordered by dependency)
Replace the head-injected __ts_bids design with a server-cached bid delivery model fetched by the client via a new /ts-bids endpoint. The auction never blocks page rendering — </head> flushes immediately, body parses without waiting for bids, and the client fetches bids in parallel with content paint. Key changes: - §2 Goal: bid delivery decoupled from page rendering; FCP unchanged from no-TS baseline - §4.3 Auction Trigger: drop buffered/streaming dichotomy; single mode forces chunked encoding on all origins (WordPress, NextJS, etc.) - §4.4 Head Injection: only __ts_ad_slots and __ts_request_id injected at <head> open; bid results moved to /ts-bids endpoint - §4.6 Client Residual: __tsAdInit defines slots immediately, fetches bids via /ts-bids, applies targeting and fires refresh() after resolve - §4.7 (new) Caching Behavior: explicit cacheability table for HTML, JS, CSS, tsjs bundle, bid results; Fastly edge HTTP cache leveraged for origin HTML - §5 Request-Time Sequence: full mermaid diagram covering content + creative + burl flow with cache-hit and cache-miss branches; separate text sequences for cache-hit (~80ms FCP, ~900ms ad-visible) and cache-miss (~250ms FCP, ~1,050ms ad-visible) - §6 Performance Summary: cache-hit and cache-miss columns; FCP added as a tracked metric - §7 Implementation Scope: add bid_cache.rs, /ts-bids endpoint, force chunked encoding step - §8 Edge Cases: origin-agnostic entries; new entries for /ts-bids 404 and client-never-fetches-/ts-bids Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pivot from the /ts-bids fetch endpoint + in-process bid_cache design to
inline __ts_bids injection before </body>. The earlier design relied on
shared state that doesn't reliably survive Fastly Compute's per-request Wasm
isolate model — body injection achieves the same FCP property in a single
response with no shared-state requirement.
Key changes:
- §4.3: replace /ts-bids long-poll with bounded </body> hold tied to
A_deadline. Body content above </body> paints first; close-tag held
until auction completes or A_deadline fires (graceful __ts_bids = {}
fallback).
- §4.3: add auction-eligibility gating (consent, bot UA, prefetch hints,
HEAD method, slot match) so auctions fire on real first-page-load
impressions only.
- §4.4: replace __ts_request_id + /ts-bids machinery with two inline
<script> blocks — __ts_ad_slots at <head> open, __ts_bids before
</body> via lol_html el.on_end_tag().
- §4.5: move both nurl and burl to client-side firing from
slotRenderEnded after hb_adid match. Server-side firing rejected to
avoid billing inflation on bids that never render.
- §4.6: replace fetch+Promise pattern with synchronous __ts_bids read.
Add lazy slim-Prebid loader (post-window.load) for scroll/refresh
auctions and Phase B identity warm-up. Add ts_initial=1 slot-ownership
sentinel.
- §4.7: switch Cache-Control from private, no-store to private,
max-age=0 to preserve browser BFCache eligibility while still
preventing intermediate-cache leaks.
- §4.8 (new): document the EC/KV identity model as load-bearing auction
input — Phase A retrieval at request time, Phase B post-render
enrichment via slim-Prebid userID modules. Add bare-EC first-impression
caveat and auction_eid_count metric. Note federated-consortium
passphrase property and clickstream-compounding speed win.
- §5: update mermaid + cache-hit/miss timelines for bounded body hold;
ad-visible converges to ~870ms (hit) / ~1,020ms (miss).
- §6: drop /ts-bids RTT row; add DCL row; add clickstream-compounding,
TS-overhead, identity-coverage, and confidence-interval framing.
- §7: drop bid_cache.rs and /ts-bids endpoint from scope; add
auction-eligibility gating and slim-Prebid bundle build target. Add
explicit "Deleted" subsection.
- §8: drop /ts-bids edge cases; add SPA/pushState, bare-EC, bot/prefetch,
HEAD, BFCache restoration cases.
- §9.6: server-side GAM downgraded from "Phase 2 commitment" to
aspirational and contingent on Google agreement. §9.8 (slim-Prebid
bundle composition), §9.9 (Privacy Sandbox), §9.10 (per-bidder consent)
added as follow-ups.
Implementation plan at docs/superpowers/plans/2026-04-30-server-side-ad-templates.md
is now stale relative to this spec; needs regenerating before code lands.
…ities.toml Adds the creative_opportunities field to Settings struct to deserialize configuration for the server-side ad auction feature. Includes build.rs stubs for types required during build-time configuration validation. Creates creative-opportunities.toml with example slot configuration and updates trusted-server.toml with the [creative_opportunities] section defining GAM network ID, auction timeout, and price granularity settings. Tests pass with proper TOML parsing of the creative_opportunities section.
…ared auction state
- Add `ad_slots_script: Option<String>` and `ad_bids_state: Arc<RwLock<Option<String>>>` fields to `HtmlProcessorConfig`
- Update `from_settings` to initialize both new fields with safe defaults
- Prepend `ad_slots_script` inside the existing `<head>` handler before integration inserts
- Add `element!("body", ...)` handler that uses `end_tag_handlers()` to inject `__ts_bids` before `</body>`; falls back to empty `{}` when auction state is `None`
- Add `IntegrationRegistry::empty_for_tests()` test helper
- Add three new tests covering all injection paths
…gibility gates; max-age=0 - Make handle_publisher_request async; add orchestrator and slots_file params - Dispatch origin request with send_async before running auction in parallel - Gate auction on GET, no prefetch, no bot, matched slots, TCF purpose-1 consent - Run server-side auction and write bucketed bids to ad_bids_state Arc<RwLock> - Compute ad_slots_script after response headers; set Cache-Control: private, max-age=0 - Fix Stream arm to thread actual ad_slots_script and ad_bids_state through - Add build_auction_request, build_bid_map, build_bids_script, build_ad_slots_script helpers - Update route_tests.rs to pass empty slots_file to route_request
…m slotRenderEnded
- build_bid_map now returns serde_json::Map with full bid objects (hb_pb,
hb_bidder, hb_adid, nurl, burl) instead of a plain CPM string map
- build_bids_script / build_ad_slots_script now emit full <script> tags
using JSON.parse("…") for safe inline embedding; add html_escape_for_script helper
- build_ad_slots_script uses correct property names (gam_unit_path, div_id,
formats, targeting) matching the client-side TSJS bundle expectations
- Replace map_or(false, …) with is_some_and(…) on lines 546, 549, 567
- Add # Panics doc sections to handle_publisher_request and create_html_processor
…nities.toml at startup
… from slotRenderEnded; slim-Prebid lazy loader
- Enable APS and adserver_mock in auction config; set providers and mediator - Increase auction_timeout_ms from 500ms to 3000ms — 500ms was too tight for HTTPS round-trips to mocktioneer, leaving the mediator zero budget - Fix mediation request: send numeric price instead of opaque encoded_price; mocktioneer requires a decoded price field and does not support encoded_price - Expand creative-opportunities slot page_patterns to include /news/**
Define SlotRenderEndedEvent, SlotRenderEvent, and TestWindow types to eliminate all @typescript-eslint/no-explicit-any violations in gpt/index.ts and gpt/index.test.ts. Extend GptWindow with __tsjs_slim_prebid_url so installSlimPrebidLoader avoids the any cast.
prk-Jr
changed the title
Set gam_network_id to 88059007 (autoblog production network). Update atf_sidebar_ad slot to /88059007/autoblog/news with div_id ad-atf_sidebar-0-_r_2_ (desktop ATF sidebar, 300x250); restrict page_patterns to article paths only (/20**, /news/**) since that div does not exist on the homepage. Add homepage_header_ad slot targeting /88059007/autoblog/homepage with ad-header-0-_R_jpalubtak5lb_ for 970x90/728x90/970x250 leaderboard formats. Reduce auction_timeout_ms from 3000 to 500 to cap TTFB at the spec-recommended ceiling.
Gate /auction client EID resolution on the same identity-consent condition as the EC ID (`ec_id.is_some()`, already filtered by `ec_allowed()`). Previously client-provided EIDs from the request body or ts-eids cookie were resolved unconditionally, so a US/GPC or US-Privacy opt-out context — where EC identity use is denied but a non-personalized auction may still run — could forward persistent EIDs, since `gate_eids_by_consent` only strips on TCF/GDPR signals. This matches the publisher and /__ts/page-bids paths. Refresh TS-defined GPT slots when the publisher disabled initial load. With pubads().disableInitialLoad(), display() only registers a freshly defined slot and the ad request must come from refresh(); TS-owned first-impression slots were only display()ed, so they rendered blank. A wrapper around disableInitialLoad() records the state on window.tsjs, and adInit() refreshes its own slots when it is set (bundle and gpt_bootstrap.js). The detector only hooks an existing googletag stub so a plain import never touches window.googletag.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review: I reviewed PR #680 at bf76d41 against origin/main, including the Rust publisher/page-bids auction path, the GPT/Prebid browser code, config/build validation, CI status, and existing review threads. I did not find any new blocking correctness/security/data-loss issues. I left two non-blocking findings inline: one concrete build-time validation gap for creative-opportunity slot config, and one stale code-doc contract around SPA navigation ownership. CI is currently green across the reported PR checks.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary (third review pass)
Re-review at head bf76d411. The PR continues to improve markedly — 8 of the round-2 findings are resolved with tests (price-bucket truncation, sync-mediation parse divergence, mediator timeout cap, dispatched-auction warn, dead parse_ts_eids_cookie removed, case-insensitive cache-privacy matching, env-injected slot-id rejection test, valid glob doc example), and the new EID-consent gating and TS-slot display() work are correct and well-tested.
Two new blocking items are small build-validator gaps that reintroduce the "build-green, runtime-broken" failure mode the latest commit ("Reject build-time creative-opportunity configs the runtime can't load") set out to eliminate. The rest are parity/observability and doc items. All carry inline comments; cross-cutting context is below.
Blocking
🔧 wrench
- Build accepts empty
div_idoverride the runtime rejects —creative_slot_build_check.rs:107(inline). - Build validates only top-level keys; nested unknown/mistyped fields (
media_type,slot_id, …) pass build but fail runtimedeny_unknown_fields—creative_slot_build_check.rs:117(inline).
Both let a CI-green config fail at runtime settings load on the deployed service.
Non-blocking (highlights; details inline)
❓ question
- Initial-load detector ordering — if
adInit()drains before the publisher'sdisableInitialLoad(), a TS-owned first-impression slot can render blank (the bug this commit targeted); ordering is undocumented/untested.gpt/index.ts:394.
🤔 thinking
slim_prebid_url</script>breakout (config-derived; encoding gap) —gpt.rs:493.popstatere-runs fulladInit()+ page-bids fetch on same-path/hash navigation (re-requests impressions; test codifies the gap) —gpt/index.ts:711.- Collect path drops parse/transport failure attribution (providers missing from
provider_details; observability-only) —auction/orchestrator.rs:924. - Per-pattern glob silent-drop in a mixed-valid set, no
log::warn!—creative_opportunities.rs:220. zonetargeting silently not forwarded when explicitprebid.biddersare set —creative_opportunities.rs:281.- Unbounded buffering of content after the first
</body>until EOF —publisher.rs:784.
♻️ refactor
- APS still holds request-scoped
Mutexstate on a sharedArc(the only un-migrated provider; documented tech debt) —aps.rs:300. - Duplicated auction-request assembly between initial-page and page-bids paths —
publisher.rs:1190.
⛏ nitpick / 📝 note
normalize_page_bids_pathdoc still stranded onpage_bids_request_allowed—publisher.rs:1682.- Stale
# Panicsdoc onhandle_publisher_request—publisher.rs:1040. - Stale beacon-firing comment in
gpt_bootstrap.js:115.
CI Status
- cargo fmt: PASS
- cargo clippy: PASS
- cargo test: PASS
- vitest (JS): PASS
- CodeQL / integration / browser-integration: PASS
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Line-level findings accompanying the change-request review above (the inline comments failed to attach to that submission). 🔧×2 are the blocking build/runtime validation-parity gaps; the rest are non-blocking (❓ ordering, 🤔 parity/observability, ♻️ refactors, ⛏/📝 docs).
…review Address PR #680 review findings: Blocking build/runtime parity: - Remove the dead glob stub in build.rs so creative-slot page-pattern validation runs against the real glob crate. An invalid pattern such as `["["]` now fails the build instead of being embedded and dropped at runtime settings load. - Reject an empty/whitespace div_id override at build time, mirroring CreativeOpportunitySlot::validate_runtime. - Validate nested creative-slot fields (formats, providers, aps, prebid) against the runtime structs' deny_unknown_fields so env-injected typos like `mediatype` or `slotId` fail the build, not runtime. Observability and correctness: - Mirror the parallel auction path on the dispatch/collect path: attribute provider parse failures (error_type + message) and transport failures (via failed_backend_name) in provider_details. - Warn on each page pattern dropped during compile_patterns so a mixed valid/invalid set is visible to operators. - Escape the </script> terminator in the configured slim_prebid_url so it cannot break out of its inline script tag. - Guard SPA navigation: onNavigate no-ops when the path is unchanged, so popstate (hash-only or same-path back/forward) no longer re-requests impressions. Docs and comments: - Update the GPT scroll/refresh handoff comment to reflect installSpaAuctionHook + /__ts/page-bids ownership of SPA navigation. - Note that targeting.zone is not forwarded when explicit prebid.bidders are set. - Split the page-bids same-origin-gate and path-normalization docs onto their own functions; remove the stale # Panics section on handle_publisher_request. - Correct the stale slotRenderEnded/beacon comment in gpt_bootstrap.js. Tests added for div_id, nested-field, slim_prebid_url escaping, and SPA same-path guard behavior.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review:
Review Summary
Reviewed PR #680 at 535f520b67856336071c883755b471fbd61ca56e against main, focusing on the server-side ad-template auction paths, EdgeZero/legacy parity, GPT/Prebid browser flows, and current CI. The feature is much closer, but I found blocking privacy/correctness issues around disabled/consent-denied ad-stack behavior and EdgeZero cache finalization parity. CI also currently reports a failing CodeQL check.
Findings
P0 / Blockers
None.
P1 / High
-
EdgeZero finalization can re-enable shared caching for private/cookie responses —
crates/trusted-server-adapter-fastly/src/middleware.rs:219,crates/trusted-server-adapter-fastly/src/main.rs:351- Issue: The PR hardens the legacy finalizer so operator
response_headerscannot overwriteprivate/no-storeor reintroduce surrogate cache headers, and so lateSet-Cookieresponses are downgraded before send. The EdgeZero path still usesapply_finalize_headers(), which unconditionally applies operator headers, and then sends afterec_finalize_response()/ request-filter effects without the equivalentenforce_set_cookie_cache_privacy()guard. - Why it matters: With EdgeZero enabled, per-user ad HTML/page-bids responses or first-visit EC-cookie responses can regain shared
Cache-Control/Surrogate-Control, risking shared-cache replay of inline user bid data orSet-Cookie. This reopens the cache/privacy issue the legacy path now fixes. - Suggested fix: Share the protected finalizer between legacy and EdgeZero, or add the same private/no-store + surrogate-header skip logic to
apply_finalize_headers()and run anHttpResponse/Fastly equivalent ofenforce_set_cookie_cache_privacy()after EC finalization and response-filter effects. Add EdgeZero-path tests for private ad HTML/page-bids with operator surrogate headers and for lateSet-Cookieon an origin-public response.
- Issue: The PR hardens the legacy finalizer so operator
-
Empty
/__ts/page-bidsresponses still invokeadInit()and can enable GPT services — see inline comment oncrates/js/lib/src/integrations/gpt/index.ts.
P2 / Medium
-
Refresh auctions apply Prebid targeting globally instead of scoping to refreshed slots — see inline comment on
crates/js/lib/src/integrations/prebid/index.ts. -
Build-time slot validation still accepts nested values that runtime serde rejects —
crates/trusted-server-core/src/creative_slot_build_check.rs:167,crates/trusted-server-core/src/creative_slot_build_check.rs:233- Issue: The build validator checks allowed nested field names, dimensions, and IDs, but not all nested value shapes against the runtime schema. For example, a raw/env slot format with an invalid
media_typestring can pass field-name/dimension checks while runtime deserialization intoCreativeOpportunityFormat.media_type: MediaTypefails. Similar gaps exist for wrong nested value types such as non-string targeting or APS slot IDs. - Why it matters: This preserves a build-green/runtime-broken path for private/env slot config, which is exactly what the new build checker is meant to prevent.
- Suggested fix: Reuse the runtime slot deserializer in build validation if possible; otherwise explicitly validate
media_typeviaMediaTypeserde and enforce nested value types (page_patterns: string[],targeting: map<string,string>,providers.aps.slot_id: string, etc.). Add build-check tests for invalidmedia_typeand wrong nested value types.
- Issue: The build validator checks allowed nested field names, dimensions, and IDs, but not all nested value shapes against the runtime schema. For example, a raw/env slot format with an invalid
P3 / Low
None.
CI / Existing Reviews
Most checks pass, but the current gh pr checks 680 output shows CodeQL failing with two high-severity cleartext-logging alerts on crates/trusted-server-core/src/auction/orchestrator.rs:751 and :1111. If these are false positives for provider/mediator names, please add the appropriate suppression/explanation; otherwise remove sensitive values from those warnings. Existing review threads cover many prior auction/GPT issues; the findings above are not duplicates of the already-resolved comments I found.
Resolve conflicts from the EdgeZero PR #257 sync on main (#761): - Cargo.toml: adopt main's crate renames, fastly/log-fastly 0.12, and edgezero tracking the upstream main branch; keep the branch's glob dep. - publisher.rs: keep both sides' new tests; forward-port test body extraction to the Option-returning Body::into_bytes API and drop the now-unused response_body_string helper superseded by the branch tests. - auction/endpoints.rs: unwrap_or_default the Option-returning into_bytes. - Relocate the new GPT SPA tests into the renamed crates/trusted-server-js tree and refresh stale crates/js doc-comment paths. - Take main's CI-validated integration-tests lockfile (deps unchanged on the branch).
The EdgeZero sync (#761) renamed crates/js and crates/integration-tests to crates/trusted-server-*. The old directories still hold local-only build artifacts (node_modules, target, dist) whose gitignore rules moved with the rename, so git now sees them as untracked. Ignore the defunct paths until the directories are removed from disk.
P1 — EdgeZero finalize cache/Set-Cookie privacy parity: Share the protected finalizer between the legacy and EdgeZero paths. apply_finalize_headers now strips surrogate cache headers and downgrades cookie-bearing responses to private, and skips operator response_headers that would re-enable shared caching on uncacheable responses; finalize_response delegates to it. The EdgeZero entry point re-applies an HttpResponse enforce_set_cookie_cache_privacy after ec_finalize_response and request-filter effects so a late EC Set-Cookie cannot reach a shared cache. Adds middleware tests for both cases. P1 — empty page-bids must not enable GPT services: adInit() only enables GPT services when it has a slot to display or refresh, and the SPA hook skips adInit() for an empty page-bids response unless prior TS state needs sweeping. Prevents a consent-denied or kill-switched navigation from activating the publisher's GPT setup. P2 — scope Prebid refresh targeting to the refreshed slots: setTargetingForGPTAsync is called with the synthetic refresh ad-unit codes so a one-slot refresh no longer mutates unrelated GPT slots. P2 — validate nested slot value shapes at build time: The creative-slot build check now validates media_type against the runtime MediaType variants, targeting as a string map, page_patterns as strings, providers.aps.slot_id as a string, providers.prebid.bidders as a map, and floor_price as a number — closing build-green/runtime-broken gaps. A drift-guard test ties media_type to the runtime enum. CI — suppress CodeQL cleartext-logging false positives: Annotate the provider/mediator "not registered" warnings; they log static config identifiers, not secrets.
The merge took main's trusted-server-integration-tests Cargo.lock, but the branch's trusted-server-core now pulls in glob (the creative-slot build check uses glob::Pattern). The integration crate path-depends on core, so its locked graph was missing glob and the --locked CI build refused to update it. Add only glob v0.3.3; no other versions change, keeping the shared direct-dependency parity check green.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review:
Review Summary
Reviewed PR #680 at 5a835c13 against origin/main, focusing on the server-side ad-template auction paths, PBS/APS consent forwarding, streaming collection, JS GPT/Prebid integration, and EdgeZero compatibility. CI is green, but I found two high-confidence blocking issues that can leak/omit consent context in PBS calls or violate the configured auction hold deadline. I also left one non-blocking compatibility concern for the EdgeZero path.
Findings
P0 / Blockers
None.
P1 / High
-
cookies_onlyPrebid consent forwarding can drop KV-backed consent —crates/trusted-server-core/src/consent/mod.rs:124,crates/trusted-server-core/src/integrations/prebid.rs:877,crates/trusted-server-core/src/integrations/prebid.rs:1085- Issue:
build_consent_contextcan satisfy the local server-side auction gate from persisted KV consent when the request has no consent cookies/signals, butcookies_onlysetsconsent_ctx = None, souser.consent,user.ext.consent, andregsare omitted from the OpenRTB body. If the incoming request also lacks aCookieheader,copy_request_headersforwards no consent cookie either. - Why it matters: The edge can allow an auction based on KV-backed consent, then send PBS a request carrying user/device signals with no consent signal at all. That is a privacy/compliance regression in the server-side auction path.
- Suggested fix: In
cookies_onlymode, include body consent whenever no consent cookie is actually forwarded / the consent source is KV, or fail closed for Prebid in that case. Add a regression test with no incoming consent cookie, KV-backed consent allowing the auction, andconsent_forwarding = "cookies_only".
- Issue:
-
Dispatched auction collection can hold
</body>pastA_deadlinewhile materializing slow provider bodies — see inline comment.
P2 / Medium
- EdgeZero mode silently loses the server-side ad-template feature — see inline comment.
P3 / Low
None.
CI / Existing Reviews
gh pr checks 680 reports all checks passing (cargo fmt/test, clippy/analyze, vitest, browser/integration tests, CodeQL, JS/docs formatting). I reviewed existing comments and avoided re-reporting previously fixed GPT refresh, page-bids origin-gate, win-beacon, timeout-payload, and slot-validation findings. The KV-backed cookies_only issue is in the review body because the exact consent-forwarding lines are outside the final PR diff hunk and GitHub would not accept an inline comment there.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Refactor follow-up
Checked the existing review threads before submitting: the auction-request assembly refactor is already covered by an earlier comment, so I did not repeat it. These two are separate low-priority maintainability comments.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary (fourth review pass)
Re-review at head ae17b45a. Every round-3 finding is resolved — build/runtime validation parity for div_id and nested slot fields (with recursion + tests), collect-path observability (parse + transport attribution now mirror the parallel path), slim_prebid_url </script> escaping, the popstate same-path guard, initial-load resilience, per-pattern glob warnings, and the stale-doc/comment fixes. The new EdgeZero empty-config gate is correct and well-tested (defers to the legacy path when slots are configured; no panic or mis-route).
Two new blocking items are small one-line/small fixes; the rest are build/runtime parity residuals, diagnosability, and doc cleanups. Most carry inline comments; a few line-unanchorable items are below.
Blocking
🔧 wrench
tokiois a production dependency intrusted-server-core— used only by#[tokio::test]; links the tokio runtime into the wasm prod build. Move to[dev-dependencies].Cargo.toml:48(inline).- SPA
currentPathadvanced before fetch, never rolled back on failure — a transient/__ts/page-bidserror permanently strands that route (guard blocks retry).gpt/index.ts:681(inline).
Non-blocking (line-unanchorable; details for the rest are inline)
🌱 seedling
sanitizeAuctionUidreadsuid.atype(unknown) without anumbernarrow —prebid/index.ts:273.tsc --noEmitflags TS18046/TS2322, but the CI gates (esbuild + eslint + vitest) don't type-check, so it slips through. Fix:if (typeof uid.atype === 'number' && Number.isInteger(uid.atype) && uid.atype >= 0 && uid.atype <= 255).</body>bids/adInit()injection silently no-ops if lol_html'send_tag_handlers()returnsNone—html_processor.rs(the body end-tag handler). Happens for an implicitly-closed/EOF<body>;adSlotsis injected at<head>butbids/adInit()never fire and ads silently never render. Add alog::warn!on theNonebranch so the whole-feature failure is diagnosable.
📝 note
- Stale
PublisherResponse::Streamdoc —publisher.rs:337claims Stream covers HTML only when "no HTML post-processors are registered," butclassify_response_routeignores that and routes all processable HTML to Stream (post-processors run inside the streaming processor; theroute_streams_non_html_even_with_post_processors_registeredtest confirms). Drop the post-processor clause. (Related to the dead-param nitpick inline at:401.)
CI Status
- cargo fmt: PASS
- cargo clippy: PASS
- cargo test: PASS (1564 core tests)
- vitest (JS): PASS (172 tests)
- CodeQL / integration / browser-integration / EdgeZero integration: PASS
…tes-impl Reconcile the server-side ad-template/auction work with main's EdgeZero canary rollout, the Axum/Cloudflare/Spin adapters, and the edgezero into_bytes() repin. - main(): route to EdgeZero only when the canary rollout selects it and the settings carry no creative_opportunity slots (combine both gates). - Thread the new handle_publisher_request(kv, ec_context, auction) parameters through the Axum, Cloudflare, and Spin adapters with an empty AuctionDispatch — server-side auction stays deferred there, as on the Fastly EdgeZero path. - Keep HEAD's apply_floor_prices None-price drop and the corrected publisher OwnedProcessResponseParams / single stream_publisher_body; union the diverged publisher, html_processor, and orchestrator tests. - Drop the stale into_bytes().unwrap_or_default() test calls and add the new HtmlProcessorConfig / PlatformBackendSpec fields and the route_request slots argument to the bench and adapter tests.
Blocking: - Move tokio to [dev-dependencies] in trusted-server-core; it was only used by #[tokio::test] and was linking the runtime into the wasm prod build. Confirmed the release wasm adapter build no longer pulls tokio. - Roll back the SPA currentPath on a failed /__ts/page-bids fetch so a transient error no longer permanently strands that route (gpt/index.ts). Build/runtime parity and diagnostics: - Bound creative-opportunity format width/height to u32 range at build time so values the runtime u32 cannot hold are rejected early. - Add #[serde(deny_unknown_fields)] to the build.rs config stub to match the runtime type and reject mistyped table keys at build time. - Warn when the <body> end-tag handler is absent so a silently non-rendering server-side ad feature is diagnosable. - Log dropped slot bidders that are neither configured nor the aps provider. - Log build_bid_index collisions (multiple bids per seat/imp). JS correctness: - Narrow uid.atype to a number before the range check in sanitizeAuctionUid. - Resolve findInjectedSlotForRefresh by exact/container match before the prefix fallback, with a regression test for prefix-overlapping div_ids. - Guard the gpt_bootstrap prefix scan against an empty div_id. - Route injectAdmIntoSlot through findSlotElementByDivId for consistency. Cleanup and docs: - Remove the dead has_post_processors routing dependency from classify_response_route and (now unused) handle_publisher_request. - Extract the duplicated EID resolution/consent-gating/device tail shared by the initial-page and page-bids dispatch paths into one helper. - Anchor the surrogate cache-header list in a shared const so the legacy and EdgeZero Set-Cookie privacy paths stay aligned. - Refresh stale docs (PublisherResponse::Stream, the publisher module platform-coupling note, and UserInfo.eids consent-gate location).
Moving tokio to trusted-server-core dev-dependencies removed it from the crate's normal dependency list, so the integration-tests lockfile (which resolves core's non-dev deps) no longer pins tokio under core. Keeps `cargo --locked` green for the integration job.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review: I reviewed PR #680 at ae9d50a against main, including the current diff, CI, and existing review threads. CI is green and I did not find a P0/blocking security or data-loss issue, but I found one high-severity compatibility/correctness issue for the non-Fastly adapters plus a few medium-risk edge cases in the new streaming and browser paths.
Review Summary
This is a very large server-side ad-template change spanning Rust adapters/core and browser GPT/Prebid code. The current Fastly legacy path appears to have the expected route wiring, consent gates, cache protection, and auction lifecycle; the highest remaining risk is adapter parity: Axum/Cloudflare/Spin accept the new config and inject the browser hook, but do not actually wire the server-side slot templates or /__ts/page-bids endpoint.
Findings
P0 / Blockers
None found.
P1 / High
- Non-Fastly adapters silently disable configured slot templates and lack the SPA page-bids endpoint —
crates/trusted-server-adapter-axum/src/app.rs:174- Issue: Axum, Cloudflare, and Spin all build an
AuctionDispatchwithslots: &[](cloudflare/src/app.rs:303,spin/src/app.rs:605have the same pattern), and none of those adapters registersGET/OPTIONS /__ts/page-bids. At the same time the shared GPT integration can still installinstallSpaAuctionHook(), which fetches/__ts/page-bidson SPA navigation. Operators can configure[[creative_opportunities.slot]]successfully, but these adapters ignore it and route the SPA fetch to the publisher origin/fallback instead of the core handler. - Why it matters: This is a cross-adapter compatibility trap. A config that works on Fastly legacy silently loses initial server-side slots/auctions on Axum/Cloudflare/Spin, and SPA navigations can make internal endpoint requests to the origin, typically producing non-JSON failures and no refreshed server-side bids.
- Suggested fix: Either wire these adapters to pass
state.settings.creative_opportunity_slots()and register the same/__ts/page-bidsGET plus fail-closed OPTIONS behavior, or fail startup / suppress the GPT SPA hook when configured slots are present on adapters where server-side templates are intentionally unsupported.
- Issue: Axum, Cloudflare, and Spin all build an
P2 / Medium
-
Slim-loaded Prebid misses user-ID module initialization after
window.load—crates/trusted-server-js/lib/src/integrations/prebid/index.ts:928- Issue: The GPT slim loader appends the Prebid script from a
window.loadhandler. When that deferred script executes,window.loadhas already fired, but Prebid self-init only schedulesinstallUserIdModules()via anotherwindow.addEventListener('load', ...). - Why it matters: In the slim-loaded path, the user-sync/EID warm-up setup can be skipped, so later server-side auctions lose the identity enrichment this PR is trying to preserve.
- Suggested fix: During Prebid self-init, call
installUserIdModules()immediately whendocument.readyState === 'complete'; otherwise register the load listener with{ once: true }.
- Issue: The GPT slim loader appends the Prebid script from a
-
Raw
</bodybyte scanning can hold the stream on non-tag text —crates/trusted-server-core/src/publisher.rs:900- Issue:
BodyCloseHoldBufferstarts the auction hold at the first case-insensitive byte sequence</body, regardless of HTML parser state. That also matches strings/comments/raw-text content such as an inline script containing"</body>"before the real closing body tag. - Why it matters: A false positive near the top of the page would hold the rest of the document behind auction collection, defeating the intended “only delay the body tail / DCL” behavior and making performance dependent on page text.
- Suggested fix: Detect the actual body end through the HTML processor/parser state, or make the scanner HTML-aware enough to ignore comments, quoted attributes, and raw-text elements. Add a regression test with
</body>inside a script before visible body content.
- Issue:
-
Prefix-based slot lookup can misattribute overlapping dynamic div IDs —
crates/trusted-server-js/lib/src/integrations/gpt/index.ts:48- Issue: After exact lookup fails,
findSlotElementByDivId()returns the first DOM id that starts with the configured prefix. With overlapping configured prefixes such asad-andad-header-, DOM order can let the shorter prefix claim the longer slot’s element/iframe. - Why it matters: The render bridge then resolves the message source to the wrong slot, rejects the real bid/slot match, and can skip creative rendering or win/billing beacons for valid server-side bids.
- Suggested fix: Prefer exact/container matches across all configured slots first, then resolve prefix matches by longest prefix (or by a precomputed
divToSlotIdmap). Add an overlapping-prefix regression test.
- Issue: After exact lookup fails,
P3 / Low
None.
CI / Existing Reviews
gh pr checks 680 reports all current checks passing, including CodeQL, cargo fmt/tests, adapter checks, browser integration tests, vitest, and docs/TS formatting. I reviewed existing review comments and avoided re-reporting items that were already raised and fixed in prior passes.
| let mut ec_context = EcContext::default(); | ||
| let auction = AuctionDispatch { | ||
| orchestrator: &state.orchestrator, | ||
| slots: &[], |
There was a problem hiding this comment.
Automated review: P1 — non-Fastly adapters silently ignore configured server-side slots.
Axum passes slots: &[] into handle_publisher_request; Cloudflare and Spin do the same. Those adapters also do not register GET/OPTIONS /__ts/page-bids, while the shared GPT bundle can still fetch that endpoint on SPA navigation. A [[creative_opportunities.slot]] config that works on Fastly legacy therefore silently disables initial server-side templates on these adapters and sends SPA page-bids requests to the publisher origin/fallback.
Please either wire these adapters to pass state.settings.creative_opportunity_slots() and register the page-bids route with the same fail-closed OPTIONS behavior, or reject/suppress this feature on adapters where it is intentionally unsupported.
| if (typeof window !== 'undefined') { | ||
| installPrebidNpm(); | ||
| installRefreshHandler(); | ||
| window.addEventListener('load', () => { |
There was a problem hiding this comment.
Automated review: P2 — slim-loaded Prebid can miss user-ID initialization.
The GPT slim loader appends the Prebid script from a window.load handler. When this script executes, load has already fired, but this code only schedules installUserIdModules() on a future load event. That means the slim-loaded path can skip user-sync/EID warm-up setup.
Please run it immediately when document.readyState === 'complete', otherwise register the load listener with { once: true }.
| return Vec::new(); | ||
| } | ||
|
|
||
| if let Some(pos) = find_ascii_case_insensitive(&self.buffered, BODY_CLOSE_PREFIX) { |
There was a problem hiding this comment.
Automated review: P2 — raw </body scanning can hold too much of the page.
This detects the first byte sequence </body without regard to HTML parser state. If that text appears in an inline script/string/comment before the real closing body tag, the hold starts there and the rest of the document waits for auction collection, which defeats the intended tail-only DCL delay.
Please make the close-body detection parser-aware (or at least ignore raw-text/comment/quoted contexts), and add a regression test with "</body>" inside a script before visible body content.
| clearTargeting?(key?: string): GoogleTagSlot; | ||
| addService(service: GoogleTagPubAdsService): GoogleTagSlot; | ||
| getTargeting?(key: string): string[]; | ||
| } |
There was a problem hiding this comment.
Automated review: P2 — first prefix match can misattribute overlapping dynamic slots.
For dynamic div IDs, this returns the first DOM id starting with divId. With configured prefixes like ad- and ad-header-, the shorter prefix can claim the longer slot's element depending on DOM order, causing the render bridge to map iframe messages to the wrong slot and skip valid creative rendering/beacons.
Please prefer exact/container matches across all configured slots first, then choose the longest matching prefix (or resolve through a precomputed divToSlotId map).
5 participants
Summary
[creative_opportunities]intrusted-server.toml. Matching slots are selected from the incoming document URL at the edge, andwindow.tsjs.adSlotsis injected at<head>open so initial GPT setup does not need a separate slot-discovery request.window.tsjs.bidsbefore</body>.window.tsjs.adInitGPT runtime path. It readswindow.tsjs.adSlotsandwindow.tsjs.bidssynchronously, defines or reuses GPT slots, applies slot-level andhb_*targeting, setsts_initial=1, refreshes the initial slots, and firesnurl/burlonly afterslotRenderEndedconfirms the TS bid won viahb_adid.[integrations.prebid].suppress_nurl_bidders, while retaining the deployment-wide[integrations.prebid].suppress_nurlcompatibility switch.window.tsjs.adSlots/ GPT metadata instead of placeholder refresh sizes.ts-eidscookie written by TSJS, gates EIDs by consent, and forwards them as OpenRTBuser.ext.eidsto PBS.ClientInfointo the server-side PBS request so bidders see the browser client context rather than the Fastly edge context.GET /__ts/page-bidshook, which updateswindow.tsjs.adSlots/window.tsjs.bidsand re-runswindow.tsjs.adInit()afterpushState,replaceState, orpopstatenavigations.What the server-side auction sends to PBS
user.idts-eccookie or generated EC identityuser.consent/user.ext.consentuser.ext.eidsts-eidscookie plus EC identity resolutionuser.ext.ec_freshdevice.uaUser-Agentheaderdevice.ipClientInfo.client_ipdevice.geodevice.dnttrueif setDNT: 1headerdevice.languageAccept-Languageheadersite.domain/site.pagesite.refRefererheaderregs.gdpr/regs.us_privacy/regs.gppimp.*[creative_opportunities]slot templates and Prebid configtmax[creative_opportunities].auction_timeout_ms, falling back to[auction].timeout_msChanges
trusted-server.toml[creative_opportunities]slot templates and documents Prebid nurl suppression knobs..env.example/ docsSUPPRESS_NURL_BIDDERS.crates/trusted-server-core/src/creative_opportunities.rscrates/trusted-server-core/src/settings.rs/build.rstrusted-server.toml.crates/trusted-server-core/src/publisher.rswindow.tsjs.adSlotsandwindow.tsjs.bids, applies cache-control safeguards, handles page-bids responses, and forwards client context.crates/trusted-server-core/src/html_processor.rstsjs.bidsinjection.crates/trusted-server-core/src/auction/*crates/trusted-server-core/src/integrations/prebid.rsnurl/burlpropagation, and per-bidder suppression.crates/trusted-server-core/src/integrations/aps.rscrates/trusted-server-core/src/integrations/adserver_mock.rscrates/trusted-server-core/src/integrations/gpt.rs/gpt_bootstrap.jswindow.tsjs.adInitbootstrap path.crates/js/lib/src/integrations/gpt/index.tswindow.tsjs.adSlots,window.tsjs.bids, render-confirmed beacon firing, SPA updates, slim-Prebid loading, and Prebid creative rendering bridge.crates/js/lib/src/integrations/prebid/index.tscrates/trusted-server-adapter-fastly/src/main.rsCloses
Closes #677
Closes #697
Closes #698
Closes #699
Closes #700
Closes #702
Test plan
Automated
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkgit diff --checkcd crates/js/lib && node build-all.mjscd crates/js/lib && npm run lintcd crates/js/lib && npm run formatcd docs && npm run formatcd crates/js/lib && npx vitest runcurrently fails before test discovery in this workspace withERR_REQUIRE_ESMfromhtml-encoding-snifferrequiring@exodus/bytes/encoding-lite.js; no local JS test assertions execute under that failure mode.Manual end-to-end (browser DevTools console)
The steps below build on each other. Use a URL whose path matches one of the configured
[[creative_opportunities.slot]]page_patterns.Step 1 - Verify slot config is injected at
<head>openExpected: an array of slot objects. Each entry has
id,gam_unit_path,div_id,formats, andtargeting. Note thediv_idvalue from one matching slot for step 3.Step 2 - Verify server-side auction results are injected before
</body>Expected: an object keyed by slot ID. Winning slots include
hb_bidder,hb_pb, and, for Prebid cache-backed bids,hb_adid/ cache fields.Step 3 - Verify
window.tsjs.adInitwired GPT targetingReplace
SLOT_DIV_IDwith thediv_idfrom step 1.Expected: the slot has the configured GAM unit path and targeting includes
hb_pb,hb_bidder, any slot-level keys such aspos/zone, andts_initial: ["1"].Step 4 - Verify slot matching is page-pattern-aware
Navigate to a different configured path, for example
/when homepage slots are configured, and repeat step 2.Expected:
window.tsjs.bidsis keyed by the slots matching that page, not by slots from the previous page type.Step 5 - Confirm no duplicate bids injection
View page source and search for
.bids=JSON.parse.Expected: exactly one
window.tsjs.bidsassignment before</body>on pages where an auction ran.Pending (GAM line items required)
Creative delivery requires standard GAM line items targeting
hb_pb,hb_bidder, and related Prebid keys. That setup is outside this PR.Checklist
unwrap()in production codelogmacros instead ofprintln!