Clarify GPT auction diagnostics evidence - #1154
ChristianPavilonis wants to merge 7 commits into
Conversation
aram356
left a comment
There was a problem hiding this comment.
Summary
Solid direction: the page-bids handler now mirrors the SSAT dispatch path and shares the request-scoped RequestTimings T0, the store only classifies auctions from explicit evidence, and the new dictionary's bounds all match the store constants. The w/h removal in AuctionBidData also fixes two duplicate-identifier type errors that existed on the base. The blocking items are all on the overlay and docs: two new <details> sections lose their open state on every store update, the delivery switch introduces a strict-mode type error, and the delivery table still documents the pre-rename wording.
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 fix in prose because the change touches multiple files or lines outside the diff and can't be auto-applied.
Blocking
🔧 wrench
- Technical details and help sections collapse on every store update — see inline at
overlay.ts:872 deliveryFactdefault arm returnsundefinedfrom astringfunction — see inline atoverlay.ts:164- Delivery table still documents the pre-rename panel wording — see inline at
gpt-diagnostics.md:273
Non-blocking
♻️ refactor / 🤔 thinking / 📝 note / ⛏ nitpick / 🌱 seedling
- ♻️ Badge
aria-labelhides the status text from assistive tech — see inline atbadges.ts:221 - ♻️
prebidAuctiondeep-clone is written three times — see inline atapi.ts:83 - 🤔 "(currency not supplied)" renders on every price line — see inline at
overlay.ts:256 - 🤔 Selecting a previous request pins its history open with no way to clear — see inline at
overlay.ts:770 - 📝
recordPrebidAuctiondepends on being called afterrecordPrebidRefresh— see inline atstore.ts:485 - 📝 Badges now intercept clicks over the creative — see inline at
overlay.ts:104 - ⛏ Binding line duplicated between "Size and visibility" and Technical details — see inline at
overlay.ts:878 - 🌱
bidWonexpiry and navigation-generation guards are untested — see inline atprebid/index.ts:534
Cross-cutting / body-level findings
- 🏕
api.test.tsfake stores no longer satisfyApiStore— the object literals passed toGptDiagnosticsApiController(around lines 82, 107, 150, 193, 262, 491) lackrecordPrebidAuctionandrecordPrebidWin, sonpx tsc --noEmitreports newTS2345errors in that file. CI does not runtsc, so this passed, but extending the fakes keeps the test file honest against the interface it exercises.
CI Status
- browser integration tests: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- cargo test (ts CLI, native): PASS
- cargo test: PASS
- prepare integration artifacts: PASS
- format-typescript: PASS
- cargo test (axum native): PASS
- format-docs: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo fmt: PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- vitest: PASS
Branch protection reports no required checks for this PR.
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
The evidence model and request-relative timing changes are supported by passing tests. This pass confirms the existing details-state and TypeScript findings and identifies a separate keyboard-focus regression in the new interactive badges.
Blocking
- 🔧 [P2] Preserve help and technical-details expansion across live updates — see inline at
crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts:657. - 🔧 [P2] Keep keyboard focus when refreshing badge positions — see inline at
crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/badges.ts:220. - 🔧 [P2] Return a string from the delivery fallback — see inline at
crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts:164.
Validation and scope
At the original head, 445 focused GPT/Prebid/diagnostics tests passed. Scratch DOM probes reproduced both UI failures. The exact one-line suggestion was applied in isolation: all 904 JavaScript tests, full JavaScript formatting, and the 13-module bundle build passed. An isolated strict-type probe fails before and passes after that change; full-project TypeScript has other existing errors. Rust/browser gates were not repeated locally. Coverage includes the changed runtime paths and relevant surrounding code, not every unchanged line of the large publisher and test files.
One inline comment includes a verified one-click suggestion. The remaining fixes span multiple locations and need manual changes. The existing delivery-table wording and API-fake typing observations remain open; this review does not duplicate those inline threads.
CI Status
- browser integration tests: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- cargo test (ts CLI, native): PASS
- cargo test: PASS
- prepare integration artifacts: PASS
- format-typescript: PASS
- cargo test (axum native): PASS
- format-docs: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo fmt: PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- vitest: PASS
a758e16 to
563670d
Compare
9d5d7df to
9b5bb41
Compare
# Conflicts: # crates/trusted-server-core/src/publisher.rs # crates/trusted-server-integration-tests/browser/tests/nextjs/gpt-diagnostics.spec.ts # crates/trusted-server-js/lib/src/integrations/gpt/index.ts # crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/api.ts # crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/badges.ts # crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts # crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/store.ts # crates/trusted-server-js/lib/src/integrations/prebid/index.ts # crates/trusted-server-js/lib/test/integrations/gpt_diagnostics/badges.test.ts # crates/trusted-server-js/lib/test/integrations/gpt_diagnostics/overlay.test.ts # crates/trusted-server-js/lib/test/integrations/prebid/index.test.ts # docs/guide/integrations/gpt-diagnostics-dictionary.md # docs/guide/integrations/gpt-diagnostics.md
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Reviewed commit 54d33addf6350c40c8a679ee9535f222c8144d7c.
The previous help-disclosure, badge-focus, and TypeScript-return blockers are fixed with regression coverage. One new correlation issue remains: a cached Prebid bid reused on a later refresh can write its win onto the earlier GPT request. Reviewed against the actual stacked base 563670df95a3f96199700505434deb0bc2680c11 (#1121).
Blocking
- 🔧 Reject cached wins whose targeting auction differs from the original auction — see inline at
crates/trusted-server-js/lib/src/integrations/prebid/index.ts:521(one-click suggestion).
Non-blocking
📝 [P3] Synchronize the label dictionary with the final rendered labels
docs/guide/integrations/gpt-diagnostics-dictionary.md:93-95 documents Server request start → ..., but overlay.ts:300-306 renders Edge request T0 → ... or SPA page-bids T0 → .... Line 112 promises GPT-reported size: 1×1 placeholder hidden, while overlay.ts:364 now omits 1×1. The page introduces itself as the exact-label dictionary, so these entries should follow the final strings.
Proposed manual documentation text (multiple table rows/hunks; no one-click suggestion):
Edge request T0 → auction dispatched / SPA page-bids T0 → auction dispatched
Edge request T0 → auction collected / SPA page-bids T0 → auction collected
Edge request T0 → bids ready / SPA page-bids T0 → bids ready
GPT-reported size: placeholder hidden
Use the distinct edge-request and SPA-handler origins in the corresponding table descriptions. This is informational/non-blocking; no runtime issue is claimed here.
Validation
Original-head focused tests: 555 passed. The cached-bid reproduction fails at the original head; the exact suggestion with the reproduction passes all 907 package tests/type assertions. JavaScript formatting, source lint, and the 13-module bundle build pass; the verified patch remained byte-identical. The payload shape was checked against the installed, lock-matched Prebid implementation.
Full tsc --noEmit still reports existing package errors; this is not a claim of a clean whole-project typecheck. Rust and real-browser suites rely on passing remote CI; no live-browser cached-bid reproduction was run.
CI Status
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- browser integration tests: PASS
- cargo test (ts CLI, native): PASS
- vitest: PASS
- prepare integration artifacts: PASS
- format-typescript: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo fmt: PASS
- cargo test (axum native): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- format-docs: PASS
- cargo test: PASS
Co-authored-by: prk-Jr <49094961+prk-Jr@users.noreply.github.com>
aram356
left a comment
There was a problem hiding this comment.
Summary
Third review pass on this head. The prior round-1 and round-2 findings are genuinely fixed: I re-ran the original repros and confirmed help/technical disclosures now survive live renders, badge nodes are reconciled so keyboard focus survives store updates, deliveryFact returns string, and the latestTargetedAuctionId guard matches Prebid 10.26.0's real semantics (src/targeting.ts:667 sets it at targeting time). Verification in a reviewer worktree at 3e8cde2b7: 906 vitest tests pass, tsc reports no type errors, eslint and prettier clean.
One new blocking issue remains, plus a documentation mismatch on the page this PR now links operators to from the panel toolbar.
1 of the inline comments below carries a one-click GitHub
suggestion— use Commit suggestion to apply it as a commit on the PR branch. The remaining comments describe the fix in prose because applying them reflows content outside the proposed range.
Blocking
🔧 wrench
- Stale request selection pins a permanent warning banner — see inline at
crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts:763 - Dictionary documents three timing labels the panel never renders — see inline at
docs/guide/integrations/gpt-diagnostics-dictionary.md:93
Non-blocking
🌱 seedling / 📝 note
currencyhas no producer in the repo — see inline atcrates/trusted-server-js/lib/src/integrations/gpt_diagnostics/store.ts:258prebidDiagnosticAttemptsentries expire only on lookup — see inline atcrates/trusted-server-js/lib/src/integrations/prebid/index.ts:438
CI Status
No checks are marked required by branch protection on this branch; all reported checks pass.
- browser integration tests: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- cargo test (ts CLI, native): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test: PASS
- prepare integration artifacts: PASS
- cargo test (axum native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- format-typescript: PASS
- format-docs: PASS
- cargo fmt: PASS
- cargo test (cross-adapter parity): PASS
- vitest: PASS
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
A large, carefully-scoped diagnostics clarity pass: auction classification now requires positive observed evidence instead of inferring from route markers, Prebid candidate/win facts are correlated by exact auction ID + ad-unit code + slot object + navigation generation, badges become keyboard-accessible request-scoped controls, and a new label dictionary documents the operator-facing vocabulary. The runtime changes are well-bounded (128-attempt cap, 30s window, bounded string validation, listener installed only when a recorder is active) and well-tested.
The blocking findings are both in the new dictionary: it promises to define "the exact fixed labels", but four of the labels it quotes are strings the panel never emits. Since the dictionary is the PR's own deliverable for label accuracy, those should be corrected before merge.
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 applied in a scratch worktree and verified with the pinneddocs/node_modulesPrettier, individually and together. The remaining comments describe the fix in prose.
Blocking
🔧 wrench
- Dictionary's three server-timing labels never appear in the panel — see inline at
docs/guide/integrations/gpt-diagnostics-dictionary.md:89-103 - Dictionary misquotes the 1×1 placeholder label — see inline at
docs/guide/integrations/gpt-diagnostics-dictionary.md:112
Non-blocking
🤔 thinking
timingAnchorlost itstrusted_serverfallback — see inline atcrates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts:285- Badges became clickable and can overlay the creative — see inline at
crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts:104
♻️ refactor
- Redundant
if/elsearound one optional trailing argument — see inline atcrates/trusted-server-js/lib/src/integrations/gpt/index.ts:1114
Cross-cutting / body-level findings
-
🌱
GptDiagnosticsAuctionWinner.currencyhas no producer — the field is declared incore/types.ts:126, validated instore.ts:258-262(bounded to 3 chars, uppercased,/^[A-Z]{3}$/), carried throughclonePrebidAuctionEvidence, and rendered bypriceBucket()inoverlay.ts:251. But neithertargetingCandidate()inprebid/index.tsnor the winner construction ingpt/index.ts:77-78ever sets it, so the whole path is unreachable today. The dictionary states this deliberately ("Diagnostics do not infer a currency", "Diagnostics do not infer or read a Prebid currency targeting key"), so this reads as intentional forward-compatibility rather than a defect — flagging only so it doesn't quietly rot into dead code. If there's a follow-up that populates it from a PBScurfield, a tracking issue linked from the type would help. -
👍 Badge reconciliation instead of
replaceChildren—badges.ts:200-270. Switching to per-badge reconcile is what lets the.tsgd-highlightelement appended bylocateOnPage()survive a badge update, and it preserves keyboard focus on an activated badge across re-renders. Thebadge.style.transform = ''reset in theelsebranch is the easy thing to miss on a reuse path and it's handled correctly. Nice work.
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
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
Analyze (rust)/ CodeQL: not run
Locally in a scratch worktree at the PR head: npx vitest run — 45 files, 906 tests passed, no type errors.
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Evidence-only rewrite of the GPT auction diagnostics: classification now derives from observed auction facts rather than inference, server and browser clocks stay separated, and page badges become focusable buttons that deep-link to an exact Ad #N · Request #M panel row. Correlation of Prebid diagnostics is keyed on Prebid's own auction ID, ad-unit code, GPT slot, and navigation generation, and is fully inert when no diagnostics recorder is installed. The new label dictionary documents every operator-facing string.
All findings below are non-blocking observations; none change the merge decision. No inline comment carries a one-click suggestion — each proposed change either spans multiple ranges or needs a matching test change in another file, so they are described in prose for manual application.
Non-blocking
🤔 thinking
currencyhas no in-tree producer — see inline atcrates/trusted-server-js/lib/src/core/types.ts:127- A later bare
recordPrebidRefresherases correlated Prebid evidence — see inline atcrates/trusted-server-js/lib/src/integrations/gpt_diagnostics/store.ts:479-484 - Watchdog-timeout completions apply targeting but record no auction — see inline at
crates/trusted-server-js/lib/src/integrations/prebid/index.ts:1620-1622 - A dropped Prebid win is silent, unlike sibling store paths — see inline at
crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/store.ts:502
♻️ refactor
- The
if (auctionFacts)fork exists only to satisfy call arity — see inline atcrates/trusted-server-js/lib/src/integrations/gpt/index.ts:1115-1132
⛏ nitpick
- Per-call
new TextEncoder(), and attempt expiry sweeps only on new auctions — see inline atcrates/trusted-server-js/lib/src/integrations/prebid/index.ts:479
Cross-cutting / body-level findings
- 👍 Correlation guards are well-defended —
bidWonis rejected on a stalelatestTargetedAuctionId, on window expiry, and on anavGenerationchange, and each rejection path has a test. The complementary "diagnostics inactive ⇒ noonEventregistration, nogetTargetingreads, noauctionIdon therequestBidscall" test is exactly the right assertion for a zero-publisher-change integration, because it pins the inertness claim the docs make rather than just the happy path. - 📝 Docs link target verified — the panel's
Label dictionaryanchor points athttps://iabtechlab.github.io/trusted-server/guide/integrations/gpt-diagnostics-dictionary. VitePress has nocleanUrlssetting indocs/.vitepress/config.mts, so the build emits.html; GitHub Pages resolves the extensionless form anyway (the existing sibling page returns 200 both with and without.html), andbase: '/trusted-server'matches the URL. No action needed.
CI Status
- cargo test (cross-adapter parity): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo test (ts CLI, native): PASS
- cargo test (axum native): PASS
- cargo test: PASS
- cargo fmt: PASS
- format-typescript: PASS
- format-docs: PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- vitest: PASS
- prepare integration artifacts: PENDING
| bidder: string; | ||
| priceBucket: string; | ||
| /** ISO currency supplied by the evidence source; absent means not supplied. */ | ||
| currency?: string; |
There was a problem hiding this comment.
🤔 thinking — currency is added to the type, normalized in the store (store.ts:258-262), and rendered by priceBucket() (overlay.ts:251), but nothing in the repository ever supplies it.
Both producers build the winner without it:
// gpt/index.ts diagnosticsAuctionFacts
{ bidder: bid.hb_bidder, priceBucket: bid.hb_pb }
// prebid/index.ts targetingCandidate
return { bidder, priceBucket };prebid/index.test.ts even asserts expect(getTargeting).not.toHaveBeenCalledWith('hb_cur'), so the omission is deliberate — but the net effect is that the panel always prints a bare price bucket with no currency, which is the ambiguity the field was presumably added to remove. Two ways to close the loop:
- Read
hb_curintargetingCandidate()alongsidehb_bidder/hb_pband pass it through, or - Document in the label dictionary that
currencyis reserved for externalgptDiagnosticsRecordercallers and is never populated by the first-party integrations.
Apply manually — either option touches prebid/index.ts plus its test, or the docs page, so it can't be expressed as a single-file suggestion.
There was a problem hiding this comment.
Documented in 0fa7dfb using the documentation option. The dictionary and export allowlist now explicitly state that first-party GPT and Prebid integrations do not populate currency; it is accepted through the internal recorder channel, which remains unsupported for operator use. No hb_cur read was added. Docs format, lint, and build pass.
| this.recordRequestIntentSource(slot, 'prebid_refresh', { | ||
| prebidAuction: Object.freeze({ | ||
| auctionId: normalizedId, | ||
| ...(candidate ? { targetingCandidate: candidate } : {}), | ||
| }), | ||
| }); |
There was a problem hiding this comment.
🤔 thinking — recordRequestIntentSource replaces the source entry rather than merging it:
intent.sources.set(source, { observedAtMs, ...facts });So any later bare recordPrebidRefresh(slot) — which passes no facts — wipes the prebidAuction recorded here, as long as the GPT request hasn't consumed the intent yet.
Inside one completeRefresh the ordering is safe (recordPrebidRefreshForDiagnostics runs before recordCompletedPrebidAuction). The reachable case is a second pubads.refresh() that lands on the early return at prebid/index.ts:1541:
if (independentSlots.length === 0) {
recordPrebidRefreshForDiagnostics(targetSlots);
return dispatchPrebidRefresh(originalRefresh, slots, opts);
}If that fires for a slot whose previous auction's GAM request hasn't happened yet, the correlated auction ID is dropped and the cycle classifies as Auction not observed.
This fails in the safe direction (less evidence, never wrong evidence), so it isn't blocking. Worth either merging into the existing source evidence instead of overwriting:
const existing = intent.sources.get(source);
intent.sources.set(source, { ...existing, observedAtMs, ...facts });or a comment stating that last-write-wins is intended here. Apply manually — the fix sits in recordRequestIntentSource, outside this diff's hunks.
There was a problem hiding this comment.
Kept last-write-wins intentionally and pinned it in 0fa7dfb. The new store regression records auction A, observes a later bare refresh before slotRequested, then attempts to record A's win. It asserts that the later request keeps the Prebid route but gets neither A's auction classification nor its evidence. Merging would risk carrying an earlier auction into a later request. Added a source comment explaining this choice. The 102-test store suite and full 909-test JS suite pass.
| const matches = record?.requests.filter( | ||
| (cycle) => cycle.prebidAuction?.auctionId === normalizedId | ||
| ); | ||
| if (!matches || matches.length !== 1 || !matches[0]?.prebidAuction) return; |
There was a problem hiding this comment.
🤔 thinking — This early return is the one correlation failure in the store that leaves no trace.
A bidWon that reaches here can be dropped because the intent expired before the GPT request, because the GAM request never fired at all, or because the cycle was already evicted past MAX_REQUEST_CYCLES_PER_SLOT. Every comparable failure elsewhere in this file reports itself — recordTrustedServerCreativeRequest calls reportAttributionIssue('creative_request_without_slot', ...) and reportAttributionIssue('creative_request_without_cycle', ...) — and those surface in the panel's attribution issues count. A silently dropped Prebid win looks identical to an auction that simply never had a winner.
Consider reporting an attribution issue on the drop so the count stays honest:
if (!matches || matches.length !== 1 || !matches[0]?.prebidAuction) {
this.reportAttributionIssue('prebid_win_without_cycle', this.now());
return;
}Apply manually — this needs a new member in the attribution-issue union and a matching dictionary row, so it can't be a single-file suggestion.
There was a problem hiding this comment.
Deferring this non-blocking telemetry change. Adding a reason would change the exported attribution-issue contract, and we should first distinguish a win arriving before its GPT cycle from a cycle that expired, was evicted, or is ambiguous. This PR continues to discard uncorrelatable wins rather than attach them to a guessed request. No runtime change in this follow-up.
| if (targetingApplied) { | ||
| recordCompletedPrebidAuction(completedAuctionId, auctionSlots, refreshAdUnitCodes); | ||
| } |
There was a problem hiding this comment.
🤔 thinking — The watchdog path reaches this block with no auction ID.
fallbackTimer = setTimeout(() => completeRefresh(true), timeoutMs) calls completeRefresh with completedAuctionId undefined, so targetingApplied is true (targeting really was applied to the GPT slots) but recordCompletedPrebidAuction bails immediately at its typeof rawAuctionId !== 'string' guard. The resulting cycle carries genuine Prebid hb_* targeting yet reports Auction not observed.
That is consistent with the evidence-only rule — there is no auction ID to correlate on — but the dictionary's Auction not observed row currently reads "Explicit completed-auction evidence was absent or malformed", which doesn't describe the timeout case. Worth one sentence in gpt-diagnostics-dictionary.md covering "Prebid never invoked bidsBackHandler, so no auction ID was available even though targeting was applied".
Apply manually — the change belongs in the docs page, not in this file.
There was a problem hiding this comment.
Documented in 0fa7dfb. The dictionary now explains that the watchdog can apply targeting without bidsBackHandler ever supplying a completed-auction ID, so Auction not observed remains the expected result. No auction identity is inferred or synthesized. Docs format, lint, and build pass.
| if ( | ||
| !auctionId || | ||
| auctionId !== auctionId.trim() || | ||
| new TextEncoder().encode(auctionId).length > 256 |
There was a problem hiding this comment.
⛏ nitpick — Two small tidiness items in this new block; neither is a correctness problem.
1. Per-call TextEncoder. This line and boundedTargetingValue (line 455) each allocate a fresh encoder on every validated value. One module-level instance is enough:
const PREBID_TEXT_ENCODER = new TextEncoder();then PREBID_TEXT_ENCODER.encode(auctionId).length > 256 and PREBID_TEXT_ENCODER.encode(value).length > maxBytes.
2. Expiry sweep runs only on new auctions. The expiresAtMs sweep lives inside recordCompletedPrebidAuction, and individual entries are otherwise deleted only when their bidWon arrives. After the last auction on a page, up to MAX_PREBID_DIAGNOSTIC_ATTEMPTS (128) entries — each holding a strong reference to a GPT slot object — stay resident past the 30 s window until unload. Bounded and harmless, but sweeping inside the bidWon handler too would keep the map tight.
Apply manually — the encoder change needs a new module-level const plus two call-site edits across separate ranges, so it can't be one suggestion.
There was a problem hiding this comment.
Deferring both optional cleanups to keep this follow-up limited to the export documentation and the intent contract test. The existing 128-entry cap bounds retained references, and expiry checks still reject late wins. A sweep on bidWon would improve cleanup when another event arrives but would not release entries on an otherwise idle page. No timer or extra sweep is added here.
| if (auctionFacts) { | ||
| recorder?.recordTrustedServerOpportunity( | ||
| gptSlot, | ||
| slot.id, | ||
| opportunity, | ||
| bid.hb_auction_id, | ||
| requestedSlotSizes, | ||
| auctionFacts | ||
| ); | ||
| } else { | ||
| recorder?.recordTrustedServerOpportunity( | ||
| gptSlot, | ||
| slot.id, | ||
| opportunity, | ||
| bid.hb_auction_id, | ||
| requestedSlotSizes | ||
| ); | ||
| } |
There was a problem hiding this comment.
♻️ refactor — Both branches are runtime-identical.
auctionFacts is declared optional on the recorder (auctionFacts?: GptDiagnosticsAuctionFacts), so passing an explicit undefined behaves exactly like omitting the argument. The 18-line fork collapses to:
recorder?.recordTrustedServerOpportunity(
gptSlot,
slot.id,
opportunity,
bid.hb_auction_id,
requestedSlotSizes,
auctionFacts
);The only thing the fork buys is call arity, because test/integrations/gpt/ad_init.test.ts asserts with a five-argument toHaveBeenCalledWith(...). That's production shape driven by a test assertion rather than by behaviour — worth inverting: keep the single call and relax the assertions to include the trailing undefined.
Apply manually — collapsing this requires editing ad_init.test.ts in the same change, so it can't be a single-file suggestion.
There was a problem hiding this comment.
Keeping the existing call shape in this PR. Omitting the sixth argument and passing undefined are equivalent for the current store implementation, but observable through arguments.length by a recorder implementation. The existing tests preserve the five-argument form when facts are absent. Given that this is non-blocking, I am not changing that behavior solely to remove the branch. The full 909-test JS suite passes.
Main squashed the managed LiveRamp RampID work as #1054, which had taken three further commits after rc merged the feature branch, so the default merge base fell back to the pre-LiveRamp commit and reported the whole feature as conflicting. Resolve each conflicted file three-way against the LiveRamp tip rc already carries (6ff8e50), which takes main's newer state and keeps rc-only work on top: - Bundle module map (#1090): `[integrations.prebid.bundle.modules]` replaces the old `adapters` / `user_id_modules` keys in the example config and the configuration guide. - rc-only EID KV write reduction (#1157), GPT auction diagnostics (#1079, #1154) and the stored-request fix survive unchanged. - Prebid shim size bound stays at rc's 43 KB; main never moved it. - Drop the example config's duplicated managed User ID block, which the merge doubled outside the conflict markers. The stored-request smoke assertion moves onto main's shared runAuction helper: it now covers the bid-less slot only, because the helper's ad unit no longer carries a publisher-supplied trustedServer bid. Unit-level sanitization coverage is unchanged in the prebid index tests.
aram356
left a comment
There was a problem hiding this comment.
Summary
Second review pass, against head a7b19ca28. All four findings from my previous pass are fixed, and I re-ran each original repro to confirm rather than taking the commit message at its word.
Verification at this head in a reviewer worktree: 908 vitest tests pass, tsc reports no type errors, eslint clean, and npm run format passes in both crates/trusted-server-js/lib and docs.
Previous findings, re-checked:
- The stale-selection banner fix was applied verbatim, with a regression test that selects a request, evicts it past the retention cap, and asserts the notice is gone on the following render.
- The dictionary timing rows now render as
<T0 anchor> → …with a sentence naming both real prefixes — a better fix than the literal rows I proposed. An adjacent1×1 placeholder hiddenmismatch I had missed was corrected in the same pass. - The
prebidDiagnosticAttemptsexpiry sweep is in place at the top ofrecordCompletedPrebidAuction. currencywas deliberately left unpopulated; that remains an open thread from another reviewer with the same two options I raised.
Unrequested fix worth noting: serverAuctionTimingOrigin now derives from trustedServerEvidence.auctionType rather than from the presence of serverAuctionTimings. I probed the previous behaviour — an SPA auction carrying a winner but no server timings rendered Edge request T0 → auction dispatched Unavailable, mislabeling the clock. The new form yields SPA page-bids T0, and competing cycles still retain the correct origin because it keys off the source type rather than the aggregate classification.
One new issue remains, on the export contract.
Blocking
🔧 wrench
- V1 export allowlist omits
prebidAuction— see inline atdocs/guide/integrations/gpt-diagnostics.md:495
Surfaces checked with no finding
Adversarial input to the new store writers (hostile types, priceBucket bypass attempts such as 1e5, -1.0, and Arabic-Indic digits, and currency normalization) is rejected cleanly with no exceptions. The locate-highlight lifecycle clears correctly on overlay destroy and after expiry; repeated clicks stack highlights harmlessly and all of them clear.
The w / h removal in types.ts initially looked like it would orphan matchedBid.w at gpt/index.ts:1860, but the base declared those two fields twice inside AuctionBidData. This PR removes the duplicate pair and one declaration remains, so the consumer still resolves and the change is a genuine cleanup.
Note on concurrent review
Another reviewer posted a round against this same head with six open threads (one refactor, one nitpick, four thinking-aloud), all correctly classified non-blocking. Two of them overlap observations from my earlier pass — intent-source last-write-wins, and the watchdog path completing with no auction ID. I have not duplicated any of them here.
CI Status
No checks are marked required by branch protection on this branch; all reported checks pass.
- browser integration tests: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- cargo test (cross-adapter parity): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo test (ts CLI, native): PASS
- cargo test (axum native): PASS
- cargo test: PASS
- prepare integration artifacts: PASS
- format-typescript: PASS
- cargo fmt: PASS
- format-docs: PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- vitest: PASS
| @@ -431,14 +436,17 @@ content or altering the APS sandbox, so this field cannot prove the inner creati | |||
| pixels. | |||
There was a problem hiding this comment.
(Anchored here because the target text at line 495 is outside every diff hunk in this PR; the finding is about the V1 Export, Storage, and Privacy section further down this file.)
🔧 wrench — This section is an explicit enumerated allowlist, and its stated purpose is telling an operator exactly what leaves the browser in the downloaded JSON. The new prebidAuction field ships in that payload but is not listed anywhere in it.
I confirmed it empirically rather than by reading — api.snapshot() at version: 1 returns:
"prebidAuction": {
"auctionId": "pb-auction-1",
"targetingCandidate": { "bidder": "example-bidder", "priceBucket": "2.40" },
"win": { "bidder": "example-bidder", "priceBucket": "2.40" }
}The label dictionary documents prebidAuction.auctionId, .targetingCandidate, and .win as panel labels, but nothing in this export-and-privacy section mentions them. That matters here specifically because the section already itemizes the comparable trustedServerAuctionId token and spends a paragraph explaining why exporting it is safe — an opaque Prebid-supplied auction ID plus two bidder and bucketed-price pairs deserves the same treatment.
Worth noting the contract is version: 1 both before and after this PR, so a consumer parsing V1 now receives a field the documented contract does not list.
Proposed fix — insert after the server auction timing fields bullet (line 496):
- Completed client-side Prebid evidence (`prebidAuction`): Prebid's own opaque
`auctionId`, plus the bounded bidder and bucketed price for its targeting
candidate and its documented `bidWon` observation, when a completed Prebid
attempt was correlated to that exact request.(verified: applied in a scratch tree, npm run format in docs/ passes, +4 lines with no table reflow)
Apply manually — can't be auto-applied as a suggestion because line 495 sits outside every diff hunk in this PR (the last hunk in this file ends near line 453), so the inline-comment API would reject a suggestion anchored there.
There was a problem hiding this comment.
Addressed in 0fa7dfb. The V1 export allowlist now includes prebidAuction.auctionId, targetingCandidate, win, and optional currency. The privacy section documents the ID and field bounds, distinguishes the Prebid-supplied token from the server-minted token, and no longer implies that a targeting candidate must be the final served bidder. Docs format, lint, and build pass. Runtime behavior is unchanged.
Summary
Ad #N · Request #Mnavigation between page badges and request panelsThis PR is stacked on #1121.
Validation
cargo test-fastlycargo test-axumcargo test-cloudflarecargo test-spincargo fmt --all -- --checkThe Playwright browser tests could not execute locally because Docker access was denied at
/var/run/docker.sock.Closes #1081