Add GPT runtime diagnostics overlay - #974
Merged
ChristianPavilonis merged 3 commits intoJul 31, 2026
Merged
Conversation
Collaborator
Author
|
Fixed the browser integration failure in 79d0832. The test used an unscoped Local verification: |
ChristianPavilonis
marked this pull request as ready for review
July 29, 2026 15:19
ChristianPavilonis
requested review from
aram356 and
prk-Jr
and removed request for
prk-Jr
July 29, 2026 15:20
ChristianPavilonis
added a commit
that referenced
this pull request
Jul 29, 2026
# Conflicts: # crates/trusted-server-js/lib/src/core/types.ts
prk-Jr
reviewed
Jul 29, 2026
prk-Jr
left a comment
prk-Jr
left a comment
Collaborator
There was a problem hiding this comment.
Summary
A well-built, self-contained diagnostics module: bounded memory, exact-ID binding that refuses to guess, ambiguity kept as a first-class outcome, closed shadow DOM, and no mutation of publisher slot elements. No correctness or security defects found, so everything below is a suggestion rather than a blocker.
Suggestions
🤔 thinking
- Enabling diagnostics ships 30 KB of tester-only code to every visitor:
tsjs-gpt_diagnostics.jsis 30,343 bytes minified, the largest module in the bundle and 3× core, concatenated synchronously for 100% of traffic while every tab stays inactive until?ts_console=1. Immediate loading is the right call and is test-locked; the tension is thatsessionStorage-only activation gives the server no way to scope the payload to activated sessions. (crates/trusted-server-core/src/integrations/gpt_diagnostics.rs:56) - Unfiltered childList mutations drive full snapshots and forced layout on every DOM insertion, amplified by four independent per-frame snapshot consumers. (
badges.ts:183) slotOnload/impressionViewableare discarded when GPT omitsisEmpty, because both matchers requireisEmpty === falsewhile the overlay treatsundefinedas pending. (store.ts:245,store.ts:264)
♻️ refactor
- A missing
CSS.escapesurfaces asduplicate_dom_idin the exported binding reason, reporting a collision that does not exist. (binding.ts:180)
⛏ nitpick
- Badge clamp hardcodes the stylesheet width (
264vsmax-width: 260px). (badges.ts:134)
🌱 seedling
- Confirmed auction-to-render traceability is the natural next layer.
slotRenderEndedalready carrieslineItemId,campaignId,advertiserIdand thesourceAgnostic*pair on the same event object the observer reads, and the render bridge already knows when the rendered creative requests markup from Trusted Server. Together those answer whether the line item Trusted Server won is the one that rendered, or whether a house ad, direct-sold line item, or Ad Manager default took the slot — on observed evidence, never inferred from targeting or price. Implemented and opening stacked on this PR; it keeps the export clear of the privacy regex this PR's spec asserts.
👍 praise
- The
observed = matched + unmatched + ambiguousinvariant asserted directly in both the unit and browser tests. - Binding refuses prefix and container-ID guesses, and keeps ambiguity visible instead of picking a winner.
referencesUnchanged()pins the non-invasiveness claim to an assertion —cmd.push,display,defineSlot,refresh,fetch,XHR.open, and the history methods all verified unpatched — and the export is checked against a privacy regex.- Remount handling for hydration-driven removal, with an explicit host-collision guard.
CI Status
All 19 checks pass on 79d0832.
- fmt: PASS
- clippy / cargo check (fastly, axum, cloudflare, spin): PASS
- rust tests (incl. cross-adapter parity, ts CLI): PASS
- js tests (vitest) and format-typescript: PASS
- browser integration tests: PASS
- format-docs: PASS
9 tasks
This was referenced Jul 29, 2026
aram356
approved these changes
Jul 31, 2026
aram356
left a comment
aram356
left a comment
Collaborator
There was a problem hiding this comment.
Summary
Adds an opt-in, tab-local GPT runtime diagnostics console with a deployment-gated Rust integration, an early activation bootstrap, and a bounded client-side observer/store/binding/overlay stack. The design is disciplined about reporting only GPT-observed facts (conservative unmatched/ambiguous handling instead of guessing), storage is bounded and memory-only, panel rendering uses textContent throughout, nothing patches GPT/network/history (asserted by tests), and coverage across Rust, Vitest, and Playwright is thorough. No blocking findings; the inline comments are all non-blocking suggestions.
Non-blocking
♻️ refactor
- Full store snapshots on every DOM mutation / scroll frame:
badges.tsMutationObserver andBindingManager.refresh()deep-copy the entire store far more often than needed (crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/badges.ts:174, binding.ts:109) - Misleading binding reason when uniqueness is unverifiable: reports
duplicate_dom_idwith no observed duplicate (crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/binding.ts:180)
🤔 thinking
- Evicted slots are permanently untrackable: a fresh
slotRequestedafter eviction can never re-enter tracking; FIFO eviction can evict the most active slot (crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/store.ts:355) - Silent no-mount on host ID collision: no
log.warnand no troubleshooting-doc entry (crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts:277)
⛏ nitpick
- Full panel re-render resets details/scroll state (crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts:355)
primaryStatelabels a completed render “Pending render” (crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts:119)
📝 note
- Enabled deployments ship the diagnostics module to every visitor in the synchronous bundle; worth an operator-facing sentence in the docs (docs/guide/integrations/gpt-diagnostics.md:25)
CI Status
- fmt: PASS
- clippy (fastly/axum/cloudflare/cloudflare-wasm/spin-native/spin-wasm): PASS
- rust tests (fastly, axum, cloudflare, spin, parity, CLI): PASS
- js tests (vitest): PASS
- js/docs format: PASS
- integration + browser integration tests: PASS
- CodeQL: PASS
Give publisher operators an opt-in, tab-local view of GPT-observed slot lifecycles and exact DOM bindings without changing auction or publisher behavior. Keep capture bounded, export allowlisted facts only, and cover activation, correlation, presentation, and non-interference across unit and browser tests.
Scope the diagnostics navigation assertion to the fixture navigation landmark so Playwright does not match the separate site-wide Home link in CI.
ChristianPavilonis
force-pushed
the
feature/gpt-runtime-diagnostics-overlay
branch
from
July 31, 2026 22:01
acfad93 to
eb0a405
Compare
aram356
added a commit
that referenced
this pull request
Aug 1, 2026
The merge from main brought the new gpt_diagnostics test (#974), which predates this PR's test-wide no-explicit-any enforcement and failed the widened CI lint on the merge result. The stub only stores and relays event payloads without inspecting them, so unknown is the accurate parameter type; the src GptPubAdsService declares addEventListener as a method, whose bivariant parameter check accepts the narrowing.
aram356
added a commit
that referenced
this pull request
Aug 1, 2026
* Lint the full JS package in CI, not just src
The lint script only covered src/**, so eslint errors in test files and
the Node build scripts were invisible to the CI lint gate in format.yml.
Widen the script to eslint . and make the widened surface clean:
- Disable @typescript-eslint/no-explicit-any for test files — tests
routinely poke at private state and mock boundaries via any, and the
rule produced 249 errors there; drop the per-line disables it makes
redundant.
- Declare Node globals for the .mjs build scripts and Node-run test
harnesses via the globals package.
- Honor the underscore convention for intentionally unused parameters
in @typescript-eslint/no-unused-vars.
- Fix the remaining import/order and prefer-const violations.
* Address review: ESM-safe Node globals, global underscore convention, fail on warnings
- Use globals.nodeBuiltin instead of globals.node for .mjs files so
CommonJS-only names (__dirname, require, module) still fail no-undef
in ES modules
- Lift the no-unused-vars underscore ignore patterns to a top-level
rules block so the convention also covers the .mjs build scripts,
where tseslint recommended otherwise applies pattern-less defaults
- Add --max-warnings=0 to the lint scripts so rules landing at 'warn'
cannot pass CI silently
- Collapse leftover blank lines in two-line arrow bodies in
ad_init.test.ts from the eslint-disable directive removals
* Narrow the test no-explicit-any opt-out from blanket to per-file
Remove the test/** rule-off block so the rule stays live for new test
files, and handle the 255 existing occurrences by tier:
- File-level eslint-disable in the six files where `any` is structural
to the mocking approach (prebid/index, gpt/ad_init, gpt/index,
core/request, core/index, didomi/index — 246 occurrences)
- Real types where they were cheap: fetch stubs cast via
`as unknown as typeof fetch` (auction, click, proxy_sign), `as
AdUnit` with a type-only import in registry, and the config cast
dropped entirely ('info' is already in the logLevel union)
- A scoped eslint-disable-next-line for gpt_bootstrap's AnyRecord alias
ESLint 9 reports unused disable directives as warnings by default, so
with --max-warnings=0 any directive made redundant later fails CI.
* Type test mocks for real instead of exempting them from no-explicit-any
Replace the six file-level eslint-disable headers and the one scoped
directive with actual types, eliminating all 247 remaining any usages
in test/. The rule now applies to the whole test tree with no
exceptions.
- Window mocks: Omit<Window, 'tsjs'> intersections with Partial<TsjsApi>
from src/core/types, replacing the any-typed tsjs surfaces; this was
the real fix for the intersection problem an old ad_init comment used
to justify any
- Prebid: structural TestPbjs/TestAdUnit/TestBid types over the hoisted
mock; ~50 argument-site casts collapse into declaration-site
as TestPbjs casts (a plain annotation cannot compile against Prebid's
contravariant requestBids signature)
- core/index: drop the local Window.tsjs declare-global that conflicted
with the src declaration
- didomi: local TestDidomiConfig mirroring the non-exported src type
- Many casts turned out gratuitous (untyped vi.fn() already assigns to
typeof fetch; ad-unit literals already satisfy AdUnit) and were
deleted outright
One deliberate double-cast survives: prebid feeds a non-array bids
value through as unknown as TestAdUnit[] to prove the shim normalizes
malformed input.
Type-only change: no statement, assertion, or mock behavior touched.
Side effect: repo tsc --noEmit errors drop from 50 to 12, all remaining
ones pre-existing in files this PR does not touch.
* Tidy prebid test comments and refresh-mutation casts
Replace the four remaining em dashes in comments with plain wording,
and hoist the repeated NestedServerParams/NestedBrowserParams casts in
the nested-isolation test into refreshServerParams/refreshBrowserParams
locals, named apart from the publisher-side serverParams/browserParams
seeded earlier in the same test.
* Type the gpt_diagnostics stub listener as unknown instead of any
The merge from main brought the new gpt_diagnostics test (#974), which
predates this PR's test-wide no-explicit-any enforcement and failed the
widened CI lint on the merge result. The stub only stores and relays
event payloads without inspecting them, so unknown is the accurate
parameter type; the src GptPubAdsService declares addEventListener as a
method, whose bivariant parameter check accepts the narrowing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
crates/trusted-server-core/src/integrations/gpt_diagnostics*ts_consoleactivation bootstrap.crates/trusted-server-core/src/{config,html_processor,migration_guards}.rscrates/trusted-server-js/lib/src/integrations/gpt_diagnostics/crates/trusted-server-js/lib/src/core/crates/trusted-server-js/lib/test/integrations/gpt_diagnostics/crates/trusted-server-integration-tests/docs/guide/integrations/gpt-diagnostics.mddocs/superpowers/{specs,plans}/2026-07-28-gpt-runtime-diagnostics-overlay*trusted-server.example.tomlCloses
Closes #973
Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest runcd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servecargo test-cloudflare && cargo test-spincargo clippy-cloudflare && cargo clippy-cloudflare-wasm && cargo clippy-spin-native && cargo clippy-spin-wasmcargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test parity./scripts/test-cli.shCI=1 ./scripts/integration-tests.sh --nocapture./scripts/integration-tests-browser.shcd crates/trusted-server-js/lib && node build-all.mjscd docs && npm run buildLive-publisher acceptance remains pending because no source-controlled publisher URL or credentials are available. The deterministic browser fixture covers the same activation, lifecycle, binding, export, privacy, and non-interference boundaries locally.
Checklist
unwrap()in production code — useexpect("should ...")println!