Prevent competing GPT first impressions and resize PUC shells - #1079
Prevent competing GPT first impressions and resize PUC shells#1079ChristianPavilonis wants to merge 9 commits into
Conversation
jevansnyc
left a comment
There was a problem hiding this comment.
I traced the initial SSAT/page-bids path, publisher requestBids path, synthetic refresh path, and Universal Creative response path. The direct single-slot case works. The inline comments cover cases that can replace or suppress the wrong impression, or leave the creative clipped at 1x1.
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
First-impression arbitration across the bootstrap, GPT bundle, and Prebid refresh wrapper, plus authenticated collapsed-shell resizing in the Universal Creative bridge. The ownership model (navigation generation + exact DOM identity + auction token + expiry) is coherent and the resize guard set is thorough. Two blocking issues: the new ownership guard suppresses the stale-targeting sweep for publisher claims, and the render bridge silently lost frame-identity disambiguation for prefix-configured div IDs.
2 of the inline comments below carry a one-click GitHub
suggestion— use Commit suggestion (or Add suggestion to batch) to apply them as commits on the PR branch. The remaining comments describe the fix in prose because the change spans several call sites or needs a matching test change and cannot be auto-applied.
Blocking
🔧 wrench
- Stale TS targeting survives on publisher-claimed slots — see inline at
crates/trusted-server-js/lib/src/integrations/gpt/index.ts:1270 - Render bridge lost frame-identity disambiguation for prefix div IDs — see inline at
crates/trusted-server-js/lib/src/integrations/gpt/index.ts:262
Non-blocking
🤔 thinking / ♻️ refactor / ⛏ nitpick / 📝 note
- Two divergent prefix resolvers can put the two claimants on different elements — see inline at
crates/trusted-server-js/lib/src/core/first_impression.ts:100 - Overlap isolation is only half-realized — see inline at
crates/trusted-server-js/lib/src/integrations/prebid/index.ts:1040 - Flipped
disableInitialLoadassertions are now vacuous — see inline atcrates/trusted-server-js/lib/test/integrations/gpt/ad_init.test.ts:1964 - Three independent 5000 ms constants, no cross-reference — see inline at
crates/trusted-server-core/src/integrations/gpt_bootstrap.js:105 - Resolver forces style recalc in the
requestBids()hot path — see inline atcrates/trusted-server-js/lib/src/core/first_impression.ts:105 - Bootstrap and bundle prune rules differ — see inline at
crates/trusted-server-js/lib/src/core/first_impression.ts:59
👍 praise
- Collapsed-shell resize guard set and its tests — see inline at
crates/trusted-server-js/lib/src/integrations/gpt/index.ts:283
Verification performed for this review
Both suggestions were applied in an isolated reviewer worktree at this head and checked in isolation and as a batch:
prettier --check src/integrations/gpt/index.ts— cleannpx vitest run— 901/901 passednode build-all.mjs— 13 modules builtcargo check-fastly— cleancargo fmt --all -- --check— cleancargo test -p trusted-server-core --target aarch64-apple-darwin integrations::gpt— 47/47 passed- Post-verify patch snapshots byte-identical before and after every verification run
CI Status
- prepare integration artifacts: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- cargo test: PASS (required)
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS (required)
- vitest: PASS
- format-typescript: PASS (required)
- format-docs: PASS (required)
- CodeQL: PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- Analyze (actions): PASS
* docs: plan PR 1079 review remediation * fix(js): scope first impression delivery ownership * fix(js): reject stale creatives and expand nested shells * Prevent delayed publisher refresh overwrites
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
First-claimant first-impression arbitration across the GPT bootstrap, GPT bundle, and Prebid wrapper, plus a guarded collapsed-shell resize for Universal Creative delivery. The ownership model is coherent — bounded by navigation generation, exact DOM identity, auction token, and expiry — and the bootstrap/bundle pair keeps one shared contract rather than two drifting implementations. Nothing blocking survived verification; the findings below are one real (if low-probability) evidence-loss path, two mechanical cleanups, and four risk notes on deliberate design choices.
3 of the inline comments below carry a one-click GitHub
suggestion— use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. The remaining comments describe the concern in prose because they are questions about intent rather than a specific replacement.
Each suggestion was applied in isolation to a scratch worktree at e538c8e1 and verified with npx vitest run (45 files / 938 tests passed, no type errors), npx prettier --check, and npm run lint (eslint --max-warnings=0). None of them drifted the tree beyond the approved bytes.
Non-blocking
♻️ refactor
- Resize call inside the post's
trymislabels failures and drops win/billing beacons — see inline atcrates/trusted-server-js/lib/src/integrations/gpt/index.ts:2220
🤔 thinking
- Ambiguous TS-owned code-only registrations suppress the slot for the whole navigation — see inline at
crates/trusted-server-js/lib/src/integrations/prebid/index.ts:1171 - An existing exact-id element now excludes prefix candidates from source authentication — see inline at
crates/trusted-server-js/lib/src/integrations/gpt/index.ts:139 - Ancestor collapse detected on either axis, rewritten on both — see inline at
crates/trusted-server-js/lib/src/integrations/gpt/index.ts:287 - Top-level
readFileSyncofnode_modulesmakes the whole browser spec unloadable without install — see inline atcrates/trusted-server-integration-tests/browser/tests/shared/aps-renderer.spec.ts:13
🏕 camp site
- Extraction to
slot_element.tsdropped the rationale for the exact-id / prefix asymmetry — see inline atcrates/trusted-server-js/lib/src/core/slot_element.ts:52
⛏ nitpick
- Dead
.sort()onpendingCodeCandidates— see inline atcrates/trusted-server-js/lib/src/integrations/prebid/index.ts:1166
👍 praise
- Real Prebid Universal Creative in the browser test — see inline at
crates/trusted-server-integration-tests/browser/tests/shared/aps-renderer.spec.ts:207
CI Status
- cargo fmt: PASS
- cargo test: PASS
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- vitest: PASS
- format-typescript: PASS
- format-docs: PASS
- prepare integration artifacts: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- CodeQL: PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- Analyze (actions): PASS
aram356
left a comment
There was a problem hiding this comment.
Summary
First-claimant arbitration for GPT first impressions plus the PUC collapsed-shell resize. The ownership state machine (generation/element/token/lease bounds, fail-closed tombstones), the Prebid refresh-wrapper suppression paths, and the render-bridge revalidation all held up under adversarial reading, and the real-PUC Playwright test is strong evidence for the resize path. All findings below are refinements rather than defects.
2 of the inline comments below carry a one-click GitHub
suggestion— use Commit suggestion (or Add suggestion to batch) to apply them as commits on the PR branch. Both were verified in a scratch worktree (prettier, eslint, vitest 938/938,node build-all.mjs) individually and together. The remaining comment describes its fix in prose because the change spans multiple files.
Non-blocking
♻️ refactor
- Ancestor resize overrides both dimensions of a singly-collapsed ancestor — see inline at
crates/trusted-server-js/lib/src/integrations/gpt/index.ts:290 - Write-only publisher-auction state — see inline at
crates/trusted-server-js/lib/src/core/first_impression.ts:289
⛏ nitpick
- Dead
.sort()on a singleton-only candidate list — see inline atcrates/trusted-server-js/lib/src/integrations/prebid/index.ts:1167
Cross-cutting / body-level findings
- 🌱 Make
FIRST_IMPRESSION_LEASE_MSoperator-configurable — the PR body already documents the residual risk: a publisher whose GPT load is consent-gated can completerequestBids()yet lose the slot to the one-shot TS fallback when the CMP delays GPT past the 5-second lease. Sourcing the lease from the injected config (likeexcludedGamAdUnitPathSuffixes) would let slow-CMP publishers widen it without a code deploy. Follow-up, not this PR. - 📝
prebid-universal-creativepulls a deprecatedcore-jswith an npm install script — the new dev dependency (pinned 1.17.2, browser tests only, and a genuinely useful addition) transitively adds a gulp-cli chain includingcore-js@<3(deprecated,hasInstallScript: true) to the browser-testnode_modules. Dev-only, but it runs a postinstall in CI;--ignore-scriptson that install would neutralize it.
CI Status
- cargo fmt: PASS (required)
- cargo test: PASS (required)
- format-typescript: PASS (required)
- format-docs: PASS (required)
- cargo test (axum native): PASS
- cargo test (ts CLI, native): PASS
- cargo test (cross-adapter parity): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- vitest: PASS
- browser integration tests: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- CodeQL: PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- Analyze (actions): PASS
e538c8e to
1b2ac5c
Compare
1b2ac5c to
20f780a
Compare
* docs: plan PR 1079 review remediation * fix(js): scope first impression delivery ownership * fix(js): reject stale creatives and expand nested shells * Prevent delayed publisher refresh overwrites
20f780a to
2a79e6a
Compare
# Conflicts: # crates/trusted-server-core/src/integrations/gpt_bootstrap.js # crates/trusted-server-integration-tests/browser/tests/shared/aps-renderer.spec.ts # crates/trusted-server-js/lib/src/core/first_impression.ts # crates/trusted-server-js/lib/src/core/types.ts # crates/trusted-server-js/lib/src/integrations/gpt/index.ts # crates/trusted-server-js/lib/src/integrations/prebid/index.ts # crates/trusted-server-js/lib/test/integrations/gpt/ad_init.test.ts # crates/trusted-server-js/lib/test/integrations/prebid/index.test.ts # docs/guide/integrations/aps.md # docs/superpowers/specs/2026-07-24-prevent-duplicate-gpt-slot-requests-design.md
Stack
This child PR depends on #1070 and uses
esi-edge-terminated-auth-mainas its base. The parent PR's ESI and authentication changes are unchanged.Closes #1078.
What changed
The first valid claimant now owns each physical slot's first impression for the current navigation. A publisher
requestBids()call, GPT request, or GPT render prevents delayed page-bids data from retargeting and refreshing that slot. If Trusted Server claims first, the Prebid refresh wrapper filters one correlated losing publisher delivery, restores the TS targeting snapshot, and then allows later publisher refresh auctions. SPA navigation clears prior TS-managed targeting synchronously before the generation advances, so a publisher claim while page-bids is pending cannot preserve stale route keys.The ownership state is bounded by navigation generation, exact DOM identity, auction token, and expiry. GPT and Prebid share one exact/prefix slot resolver. Overlapping auctions keep separate registrations, and exact ad-ID delivery consumes only its matching registration. An abandoned publisher claim gets at most one per-slot TS fallback after the lease expires. Publisher tokens that become TS-owned remain bounded suppression tombstones until navigation or exact-element replacement, so arbitrarily late correlated callbacks still fail closed. The bootstrap and full GPT bundle use the same identity, pruning, and slot-cap contract, and ownership listeners do not depend on diagnostics.
The Universal Creative bridge now replaces the guarded resize that it suppresses in Prebid. After a TS response is posted successfully, it expands only the authenticated source iframe and collapsed ancestors through the authenticated slot root from 1x1 to the validated winning dimensions. When a configured prefix remains ambiguous by visibility and geometry, exactly one requesting-frame match can disambiguate delivery; zero or multiple matches fail closed. The resize guard rejects ambiguous or stale sources, invalid dimensions, anchors, interstitials, fixed or sticky shells, and already-expanded frames. It covers server APS, client-side APS capabilities, inline
adm, and PBS Cache responses.publisher_nativekeeps its separate replacement path.The design and APS guide now document first-claimant ownership, one-shot losing-delivery suppression, authenticated frame resolution, and collapsed ancestor-chain resizing. Strict TS-first delivery remains a separate design choice.
Validation
cargo fmt --all -- --checkcargo clippy-fastly && cargo clippy-axum && cargo clippy-cloudflare && cargo clippy-cloudflare-wasm && cargo clippy-spin-native && cargo clippy-spin-wasmcargo test-fastly && cargo test-axum && cargo test-cloudflare && cargo test-spincargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test paritycd crates/trusted-server-js/lib && npx vitest run(938 passed)cd crates/trusted-server-js/lib && npm run lint && npm run format && node build-all.mjscd docs && npm run formatcd crates/trusted-server-integration-tests/browser && npx playwright test tests/shared/aps-renderer.spec.ts --project=chromium(5 passed, including real Prebid Universal Creative geometry and terminal render evidence)Generated TSJS distribution files remain ignored and are not committed.
Residual risks and rollback
The five-second lease intentionally bounds ordinary publisher ownership and fallback timing. Publisher flows that do not request or render within that interval can yield the untouched slot to the one-shot TS fallback. Once Trusted Server owns the slot, already-registered losing tokens remain only until navigation or exact-element replacement so late correlated callbacks cannot escape suppression. The shell resize declines unusual or ambiguous GAM wrapper layouts rather than mutating them.
Rollback does not require a merge or deployment from this PR. Operators can disable the server-side opportunity path with
creative_opportunities.enabled = falsewhile reverting the child change. No stack PR is merged by this submission.