Skip to content

Clarify GPT auction diagnostics evidence - #1154

Open
ChristianPavilonis wants to merge 7 commits into
feature/ts-console-improvementsfrom
feature/ts-console-clarity
Open

ChristianPavilonis wants to merge 7 commits into
feature/ts-console-improvementsfrom
feature/ts-console-clarity

Conversation

@ChristianPavilonis

Copy link
Copy Markdown
Collaborator

Summary

  • classify GPT requests only from observed auction evidence and keep server and browser timing clocks separate
  • distinguish auction winners, Prebid candidates, delivery evidence, and confirmed served creatives
  • add accessible Ad #N · Request #M navigation between page badges and request panels
  • correlate Prebid diagnostics with Prebid's auction IDs without changing behavior when diagnostics are inactive
  • reorganize panel facts and document every visible diagnostics label

This PR is stacked on #1121.

Validation

  • cargo test-fastly
  • cargo test-axum
  • cargo test-cloudflare
  • cargo test-spin
  • parity integration tests
  • all configured Clippy targets
  • cargo fmt --all -- --check
  • JavaScript Vitest suite, 904 tests
  • JavaScript ESLint, Prettier, and bundle build
  • documentation formatting, lint, and VitePress build
  • Playwright discovered all 3 GPT diagnostics browser tests

The Playwright browser tests could not execute locally because Docker access was denied at /var/run/docker.sock.

Closes #1081

@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

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
  • deliveryFact default arm returns undefined from a string function — see inline at overlay.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-label hides the status text from assistive tech — see inline at badges.ts:221
  • ♻️ prebidAuction deep-clone is written three times — see inline at api.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
  • 📝 recordPrebidAuction depends on being called after recordPrebidRefresh — see inline at store.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
  • 🌱 bidWon expiry and navigation-generation guards are untested — see inline at prebid/index.ts:534

Cross-cutting / body-level findings

  • 🏕 api.test.ts fake stores no longer satisfy ApiStore — the object literals passed to GptDiagnosticsApiController (around lines 82, 107, 150, 193, 262, 491) lack recordPrebidAuction and recordPrebidWin, so npx tsc --noEmit reports new TS2345 errors in that file. CI does not run tsc, 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.

Comment thread crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts Outdated
Comment thread docs/guide/integrations/gpt-diagnostics.md Outdated
Comment thread crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/badges.ts Outdated
Comment thread crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts Outdated
Comment thread crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts Outdated
Comment thread crates/trusted-server-js/lib/src/integrations/prebid/index.ts

@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

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

Comment thread crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/badges.ts Outdated
Comment thread crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts Outdated
@aram356 aram356 added this to the 202609 milestone Sep 14, 2026
@ChristianPavilonis
ChristianPavilonis force-pushed the feature/ts-console-improvements branch from a758e16 to 563670d Compare September 17, 2026 16:17
aram356 added a commit that referenced this pull request Sep 18, 2026
# 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 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

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

Comment thread crates/trusted-server-js/lib/src/integrations/prebid/index.ts Outdated
Co-authored-by: prk-Jr <49094961+prk-Jr@users.noreply.github.com>

@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

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

  • currency has no producer in the repo — see inline at crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/store.ts:258
  • prebidDiagnosticAttempts entries expire only on lookup — see inline at crates/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

Comment thread docs/guide/integrations/gpt-diagnostics-dictionary.md Outdated
Comment thread crates/trusted-server-js/lib/src/integrations/prebid/index.ts

@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

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 pinned docs/node_modules Prettier, 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

  • timingAnchor lost its trusted_server fallback — see inline at crates/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/else around one optional trailing argument — see inline at crates/trusted-server-js/lib/src/integrations/gpt/index.ts:1114

Cross-cutting / body-level findings

  • 🌱 GptDiagnosticsAuctionWinner.currency has no producer — the field is declared in core/types.ts:126, validated in store.ts:258-262 (bounded to 3 chars, uppercased, /^[A-Z]{3}$/), carried through clonePrebidAuctionEvidence, and rendered by priceBucket() in overlay.ts:251. But neither targetingCandidate() in prebid/index.ts nor the winner construction in gpt/index.ts:77-78 ever 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 PBS cur field, 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-highlight element appended by locateOnPage() survive a badge update, and it preserves keyboard focus on an activated badge across re-renders. The badge.style.transform = '' reset in the else branch 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.

Comment thread docs/guide/integrations/gpt-diagnostics-dictionary.md Outdated
Comment thread docs/guide/integrations/gpt-diagnostics-dictionary.md Outdated
Comment thread crates/trusted-server-js/lib/src/integrations/gpt/index.ts

@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

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

  • currency has no in-tree producer — see inline at crates/trusted-server-js/lib/src/core/types.ts:127
  • A later bare recordPrebidRefresh erases correlated Prebid evidence — see inline at crates/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 at crates/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 at crates/trusted-server-js/lib/src/integrations/prebid/index.ts:479

Cross-cutting / body-level findings

  • 👍 Correlation guards are well-defended — bidWon is rejected on a stale latestTargetedAuctionId, on window expiry, and on a navGeneration change, and each rejection path has a test. The complementary "diagnostics inactive ⇒ no onEvent registration, no getTargeting reads, no auctionId on the requestBids call" 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 dictionary anchor points at https://iabtechlab.github.io/trusted-server/guide/integrations/gpt-diagnostics-dictionary. VitePress has no cleanUrls setting in docs/.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), and base: '/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;

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 — 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:

  1. Read hb_cur in targetingCandidate() alongside hb_bidder/hb_pb and pass it through, or
  2. Document in the label dictionary that currency is reserved for external gptDiagnosticsRecorder callers 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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

Comment on lines +479 to +484
this.recordRequestIntentSource(slot, 'prebid_refresh', {
prebidAuction: Object.freeze({
auctionId: normalizedId,
...(candidate ? { targetingCandidate: candidate } : {}),
}),
});

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 — 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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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;

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 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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

Comment on lines +1620 to +1622
if (targetingApplied) {
recordCompletedPrebidAuction(completedAuctionId, auctionSlots, refreshAdUnitCodes);
}

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 — 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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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

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 — 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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

Comment on lines +1115 to +1132
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
);
}

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.

♻️ 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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

prk-Jr added a commit that referenced this pull request Sep 22, 2026
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 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

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 adjacent 1×1 placeholder hidden mismatch I had missed was corrected in the same pass.
  • The prebidDiagnosticAttempts expiry sweep is in place at the top of recordCompletedPrebidAuction.
  • currency was 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 at docs/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.

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.

(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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

aram356 added a commit that referenced this pull request Sep 24, 2026

This branch has not been deployed

No deployments
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.

Improvements to TS_CONSOLE for ad observability

3 participants