Skip to content

Prevent competing GPT first impressions and resize PUC shells - #1079

Open
ChristianPavilonis wants to merge 3 commits into
mainfrom
fix/gpt-first-impression-aps-shell
Open

Prevent competing GPT first impressions and resize PUC shells#1079
ChristianPavilonis wants to merge 3 commits into
mainfrom
fix/gpt-first-impression-aps-shell

Conversation

@ChristianPavilonis

@ChristianPavilonis ChristianPavilonis commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Stack

This child PR depends on #1070 and uses esi-edge-terminated-auth-main as 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_native keeps 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 -- --check
  • cargo clippy-fastly && cargo clippy-axum && cargo clippy-cloudflare && cargo clippy-cloudflare-wasm && cargo clippy-spin-native && cargo clippy-spin-wasm
  • cargo test-fastly && cargo test-axum && cargo test-cloudflare && cargo test-spin
  • cargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test parity
  • cd 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.mjs
  • cd docs && npm run format
  • cd 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 = false while reverting the child change. No stack PR is merged by this submission.

@ChristianPavilonis ChristianPavilonis changed the title Arbitrate GPT first impressions and resize PUC shells Prevent competing GPT first impressions and resize PUC shells Aug 26, 2026
Comment thread crates/trusted-server-core/src/integrations/gpt_bootstrap.js Fixed
ChristianPavilonis added a commit that referenced this pull request Aug 26, 2026
@aram356 aram356 assigned aram356 and ChristianPavilonis and unassigned aram356 Aug 27, 2026

@jevansnyc jevansnyc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/trusted-server-js/lib/src/integrations/prebid/index.ts
Comment thread crates/trusted-server-js/lib/src/integrations/prebid/index.ts
Comment thread crates/trusted-server-js/lib/src/integrations/prebid/index.ts
Comment thread crates/trusted-server-js/lib/src/integrations/prebid/index.ts
Comment thread crates/trusted-server-js/lib/src/core/first_impression.ts
Comment thread crates/trusted-server-js/lib/src/integrations/gpt/index.ts
Comment thread crates/trusted-server-js/lib/src/integrations/gpt/index.ts Outdated

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 disableInitialLoad assertions are now vacuous — see inline at crates/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 at crates/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 — clean
  • npx vitest run — 901/901 passed
  • node build-all.mjs — 13 modules built
  • cargo check-fastly — clean
  • cargo fmt --all -- --check — clean
  • cargo 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

Comment thread crates/trusted-server-js/lib/src/integrations/gpt/index.ts
Comment thread crates/trusted-server-js/lib/src/integrations/gpt/index.ts
Comment thread crates/trusted-server-js/lib/src/core/first_impression.ts
Comment thread crates/trusted-server-js/lib/src/integrations/prebid/index.ts Outdated
Comment thread crates/trusted-server-js/lib/test/integrations/gpt/ad_init.test.ts
Comment thread crates/trusted-server-core/src/integrations/gpt_bootstrap.js
Comment thread crates/trusted-server-js/lib/src/core/first_impression.ts Outdated
Comment thread crates/trusted-server-js/lib/src/core/first_impression.ts Outdated
Comment thread crates/trusted-server-js/lib/src/integrations/gpt/index.ts Outdated

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 try mislabels failures and drops win/billing beacons — see inline at crates/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 readFileSync of node_modules makes the whole browser spec unloadable without install — see inline at crates/trusted-server-integration-tests/browser/tests/shared/aps-renderer.spec.ts:13

🏕 camp site

  • Extraction to slot_element.ts dropped the rationale for the exact-id / prefix asymmetry — see inline at crates/trusted-server-js/lib/src/core/slot_element.ts:52

⛏ nitpick

  • Dead .sort() on pendingCodeCandidates — see inline at crates/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

Comment on lines +2220 to 2232
resizeCollapsedCreativeFrame(
e.source,
sourceSlotFrame,
cachedWidth,
cachedHeight,
generation,
stillOwnsCreative
);
} catch (err) {
safelyRecordCreativeFailure(attemptId, 'response_post_failed');
log.warn(`[tsjs-gpt] pbRender bridge: response post failed for '${slotId}'`, err);
return;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ refactorresizeCollapsedCreativeFrame() sits inside the try whose catch records safelyRecordCreativeFailure(attemptId, 'response_post_failed') and returns. By that point port.postMessage() has already succeeded, so the creative renders — but a throw from the resize is recorded as a post failure that did not happen, and it skips both safelyRecordCreativeResponse(attemptId) and fireWinBillingBeacons(slotId, matchedBid). The result is a delivered impression with no win/billing evidence, attributed to the wrong cause.

The inline-adm path already places the resize after the try (see the same file around the pbRender bridge served ... from inline adm branch); this makes the two consistent.

(Verified in a scratch worktree at this head: npx vitest run 938 passed / no type errors, prettier --check clean, eslint --max-warnings=0 clean.)

Suggested change
resizeCollapsedCreativeFrame(
e.source,
sourceSlotFrame,
cachedWidth,
cachedHeight,
generation,
stillOwnsCreative
);
} catch (err) {
safelyRecordCreativeFailure(attemptId, 'response_post_failed');
log.warn(`[tsjs-gpt] pbRender bridge: response post failed for '${slotId}'`, err);
return;
}
} catch (err) {
safelyRecordCreativeFailure(attemptId, 'response_post_failed');
log.warn(`[tsjs-gpt] pbRender bridge: response post failed for '${slotId}'`, err);
return;
}
resizeCollapsedCreativeFrame(
e.source,
sourceSlotFrame,
cachedWidth,
cachedHeight,
generation,
stillOwnsCreative
);

Comment on lines +1166 to +1167
).values(),
].sort((left, right) => left.registrationId - right.registrationId);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick — this .sort() is dead. pendingCodeCandidates is only consumed via pendingCodeCandidates.length === 1 ? pendingCodeCandidates[0] : undefined on the next line and .some(...) a few lines down, so element order is never observed. Leaving the comparator in reads as "lowest registrationId wins", which is the opposite of what this code deliberately does — it refuses to pick when more than one candidate matches.

(Verified in a scratch worktree at this head: npx vitest run 938 passed / no type errors, prettier --check clean, eslint --max-warnings=0 clean.)

Suggested change
).values(),
].sort((left, right) => left.registrationId - right.registrationId);
).values(),
];

Comment on lines +52 to +60
const exact = document.getElementById(divId);
if (exact) {
return { element: exact, prefixMatchCount: 1, activeMatchCount: 1 };
}

const prefixMatches = Array.from(document.querySelectorAll<HTMLElement>('[id]')).filter(
(element) => element.id.startsWith(divId) && !element.id.endsWith('-container')
);
if (prefixMatches.length === 1 && isElementVisible(prefixMatches[0]!)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏕 camp site — the move out of gpt/index.ts dropped both rationale comments that explained the exact-id / prefix asymmetry. They documented a genuinely non-obvious invariant: a hidden exact-id match still resolves, while a hidden prefix match does not. Now that this resolver is shared by the GPT and Prebid bundles rather than living next to its only caller, that reasoning is worth more, not less — otherwise the next reader is likely to "fix" the asymmetry.

Restoring them verbatim from the pre-move source:

(Verified in a scratch worktree at this head: npx vitest run 938 passed / no type errors, prettier --check clean, eslint --max-warnings=0 clean.)

Suggested change
const exact = document.getElementById(divId);
if (exact) {
return { element: exact, prefixMatchCount: 1, activeMatchCount: 1 };
}
const prefixMatches = Array.from(document.querySelectorAll<HTMLElement>('[id]')).filter(
(element) => element.id.startsWith(divId) && !element.id.endsWith('-container')
);
if (prefixMatches.length === 1 && isElementVisible(prefixMatches[0]!)) {
// Exact-id matches intentionally skip the visibility tiers below: a
// configured literal id is unambiguous, so a hidden match is still the
// right element (adInit defines the slot; GPT simply renders nothing while
// it is hidden). Prefix matches go through the tiers because a prefix can
// match several candidates and only visibility/layout disambiguates them —
// so a hidden exact-id match resolves while a hidden prefix match does not.
const exact = document.getElementById(divId);
if (exact) {
return { element: exact, prefixMatchCount: 1, activeMatchCount: 1 };
}
const prefixMatches = Array.from(document.querySelectorAll<HTMLElement>('[id]')).filter(
(element) => element.id.startsWith(divId) && !element.id.endsWith('-container')
);
// A unique prefix match may be a lazy slot that has not been sized yet, but
// it must still be visible through its ancestor containers.
if (prefixMatches.length === 1 && isElementVisible(prefixMatches[0]!)) {

const pendingCode = pendingCodeCandidates.length === 1 ? pendingCodeCandidates[0] : undefined;
const pending = pendingBid ?? pendingCode;
if (!pending) {
if (pendingCodeCandidates.some((candidate) => candidate.retainUntilContextChange)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 thinking — this branch suppresses the slot for the rest of the navigation, not for one delivery.

Two overlapping requestBids() calls for the same code while TS owns the slot produce two registrations with retainUntilContextChange === true. From then on pendingCodeCandidates.length === 1 never holds, so pending stays undefined, removeConsumedPublisherRegistration never runs, and prunePendingPublisherBids exempts retained records from expiresAt. Every later pubads.refresh() for that slot re-enters suppressedSlots here; a single-slot refresh then returns early at remainingSlots.length === 0.

The test 'fails closed without consuming TS-owned ambiguous code-only registrations' locks this in — it asserts originalRefresh is never called across three refreshes — and the design doc says tombstones are "never evicted or removed by timeout or auction failure". So this is deliberate, and I am not asking you to change it here.

The residual I want on the record: on a long-lived non-SPA page with scroll refresh, that physical slot is dead for the remainder of the page view. That reads differently from the PR description's "and then allows later publisher refresh auctions". If the intent is "suppress one losing delivery", bounding the ambiguous-tombstone case by FIRST_IMPRESSION_LEASE_MS would match it; if the intent really is navigation-scoped, the PR description is the thing to adjust.

Comment on lines +139 to +144
const exact = document.getElementById(divId);
const candidates = exact
? [exact]
: Array.from(document.querySelectorAll<HTMLElement>('[id]')).filter(
(element) => element.id.startsWith(divId) && !element.id.endsWith('-container')
);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 thinking — behaviour change worth calling out: when divId resolves to a real exact element, prefix candidates are now excluded entirely (exact ? [exact] : prefixMatches). The removed candidateSlotRootsForConfiguredDivId unioned the exact element and every startsWith(divId) element.

For a publisher whose configured div_id exists as a real element while the live creative lands in a ${div_id}-<suffix> sibling, the source no longer authenticates and the bridge fails closed (blank slot) where it previously resolved. That is the safe direction, and the prefix path exists precisely because some publishers have volatile div IDs — so it would be worth a line in the design doc's "authenticated frame resolution" section stating that an existing exact-id element wins outright and prefix siblings are not consulted.

frame.iframe.height = String(height);
frame.iframe.style.width = `${width}px`;
frame.iframe.style.height = `${height}px`;
for (const ancestor of collapsedAncestors) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 thinking — ancestor collapse is detected on either axis (hasCollapsedDimension(ancestor, 'width') || hasCollapsedDimension(ancestor, 'height')) but rewritten on both. An ancestor collapsed only in width (width: 1px; height: auto) picks up a hard pixel height it never had, which can pin a wrapper that was previously content-sized.

The iframe guard above is stricter — it requires both axes to be 1 and both computed dimensions collapsed. Either setting only the axis that was actually collapsed, or requiring both axes on ancestors too, would make the ancestor contract match the frame contract.

Comment on lines +13 to +19
const PUC_BANNER = readFileSync(
resolve(
__dirname,
"../../node_modules/prebid-universal-creative/dist/banner.js",
),
"utf8",
);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 thinkingPUC_BANNER is read at module scope, so a missing prebid-universal-creative install fails collection for the entire spec file, not just the new test — and it surfaces as a raw ENOENT on a node_modules path rather than "run npm ci in crates/trusted-server-integration-tests/browser". The four pre-existing APS renderer tests become undiagnosable for a reason unrelated to them.

Moving the readFileSync inside the test that needs it (or guarding it with a test.skip carrying that message) keeps the rest of the file runnable.

})();`;

test.describe("APS rendering", () => {
test("renders through real PUC and expands only its authenticated 1x1 shell", async ({

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 praise — driving the real pinned prebid-universal-creative@1.17.2 banner.js through ucTag.renderAd, against an actual nested collapsed-shell topology, is the right way to test this. A hand-rolled PUC stub would have proven nothing about the resize contract, since the whole guard is about what the real creative wrapper does to the DOM.

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 at crates/trusted-server-js/lib/src/integrations/prebid/index.ts:1167

Cross-cutting / body-level findings

  • 🌱 Make FIRST_IMPRESSION_LEASE_MS operator-configurable — the PR body already documents the residual risk: a publisher whose GPT load is consent-gated can complete requestBids() 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 (like excludedGamAdUnitPathSuffixes) would let slow-CMP publishers widen it without a code deploy. Follow-up, not this PR.
  • 📝 prebid-universal-creative pulls a deprecated core-js with 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 including core-js@<3 (deprecated, hasInstallScript: true) to the browser-test node_modules. Dev-only, but it runs a postinstall in CI; --ignore-scripts on 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

Comment on lines +287 to +290
for (const ancestor of collapsedAncestors) {
ancestor.style.width = `${width}px`;
ancestor.style.height = `${height}px`;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ refactorcollapsedAncestors collects ancestors collapsed in either dimension, but this loop forces both width and height to the creative size. An ancestor with height: 1px and fluid width (a common collapse placeholder inside a full-width band) gets its width forced to the creative width, narrowing a formerly full-width element. Setting only the still-collapsed dimension confines the mutation to what actually clips, and re-checking per dimension also correctly skips shrink-wrap ancestors that already expanded when the iframe grew.

Suggested change
for (const ancestor of collapsedAncestors) {
ancestor.style.width = `${width}px`;
ancestor.style.height = `${height}px`;
}
for (const ancestor of collapsedAncestors) {
if (hasCollapsedDimension(ancestor, 'width')) ancestor.style.width = `${width}px`;
if (hasCollapsedDimension(ancestor, 'height')) ancestor.style.height = `${height}px`;
}

(Scratch-verified: prettier, eslint, vitest 938/938, and node build-all.mjs pass with this applied — alone and together with the other suggestion.)

)
.map((pending) => [pending.registrationId, pending] as const)
).values(),
].sort((left, right) => left.registrationId - right.registrationId);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick — This .sort() has no observable effect: pendingCodeCandidates is only consumed via length === 1 ? pendingCodeCandidates[0] : undefined and an order-insensitive .some(...), so ordering by registrationId never influences the outcome.

Suggested change
].sort((left, right) => left.registrationId - right.registrationId);
];

(Scratch-verified: prettier, eslint, vitest 938/938, and node build-all.mjs pass with this applied — alone and together with the other suggestion.)

Comment on lines +278 to +289
export function markPublisherFirstImpressionDeliveryPending(
ts: TsjsApi,
token: string,
adIds: string[],
now = Date.now()
): void {
const found = findPublisherAuction(ts, token, now);
if (!found) return;
found.auction.phase = 'delivery_pending';
found.auction.adIds = [...new Set(adIds)];
if (found.claim.owner === 'publisher') found.claim.phase = 'delivery_pending';
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ refactorFirstImpressionPublisherAuction.adIds and .phase are written here but never read anywhere in production code, and a publisher claim's 'delivery_pending' phase is treated identically to 'auctioning' at every decision point (the prune conditions, claimFirstImpressionForTrustedServer, and publisherFirstImpressionRetryDelay all group them). Apart from its prune side effect, this function is behaviorally a no-op: delivery correlation happens through pendingPublisherBids / pendingPublisherCodes in prebid/index.ts, not through this state. Either wire the auction-level phase/ad IDs into a decision or diagnostics surface, or drop the two fields and simplify this call site.

Apply manually — can't be auto-applied as a suggestion because the removal spans types.ts, first_impression.ts, prebid/index.ts, and the tests that construct this state.

@prk-Jr
prk-Jr force-pushed the fix/gpt-first-impression-aps-shell branch from e538c8e to 1b2ac5c Compare August 28, 2026 11:20
@prk-Jr
prk-Jr force-pushed the fix/gpt-first-impression-aps-shell branch from 1b2ac5c to 20f780a Compare August 28, 2026 11:46
Base automatically changed from esi-edge-terminated-auth-main to main August 28, 2026 12:13
ChristianPavilonis and others added 3 commits August 28, 2026 17:43
* 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
prk-Jr force-pushed the fix/gpt-first-impression-aps-shell branch from 20f780a to 2a79e6a Compare August 28, 2026 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TS adInit overwrites publisher ads and APS replacements remain 1x1

4 participants