Exclude GAM paths from Prebid refresh auctions - #965
Conversation
f431f73 to
f3dc6ba
Compare
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
The refresh-path exclusion behavior, validation, injection, and test coverage look sound. One blocking CI failure must be resolved before merge.
Blocking
🔧 wrench
- TypeScript lint failure: The new refresh-exclusion tests introduce 12 explicit
anycasts rejected by@typescript-eslint/no-explicit-anyincrates/trusted-server-js/lib/test/integrations/prebid/index.test.ts(beginning at line 1534 and continuing through line 1695). Replace them with the existing typed test-window/PBJS helpers or narrow test interfaces so the blocking lint job passes.
CI Status
- TypeScript lint/format job: FAIL — 12
no-explicit-anyerrors - Rust tests and adapter checks: PASS
- Browser and integration tests: PASS
- Vitest: PASS
- Rust and docs formatting: PASS
- CodeQL and analysis: PASS
aram356
left a comment
There was a problem hiding this comment.
Summary
The feature itself is in good shape: strict fail-closed config validation on the server, a fail-open matcher in the browser, and a thorough JS test matrix (explicit, all-excluded, mixed, fail-open, literal matching). Requesting changes because the PR is currently unmergeable and stale against main (#967 landed after this branch's base and invalidates the rollout documentation), CI fails on TypeScript lint, and the committed spec contains real production identifiers.
Blocking
🔧 wrench
- CI failure:
format-typescript: 10@typescript-eslint/no-explicit-anyerrors in the new tests; use the existingTestWindowtype instead ofas any(crates/trusted-server-js/lib/test/integrations/prebid/index.test.ts:1534-1667, inline comment). - Merge conflict and stale base vs #967: GitHub reports the PR as CONFLICTING (
index.test.tsconflicts on a test merge ofmain). The base predates #967, which decoupled the prebid tsjs shim from the bundled Prebid.js and rewroteindex.ts. Please rebase onto currentmain, then re-run the full gate list; the unchecked test-plan items (WASM build,fastly compute serve) should also be completed since the shim architecture underneath this change moved. - Rollout documentation wrong after #967: the new guide section says the external bundle must be regenerated for the filter to apply; post-#967 the refresh wrapper ships in the server-served shim, so the filter and injected config deploy together with the server (docs/guide/integrations/prebid.md:359-364, inline comment; same claim in spec §8 and plan Task 5.3).
- Real production identifiers in the committed spec: real GAM network code and publisher name in the capture path; replace with fictional values per CLAUDE.md and the plan's own constraint (docs/superpowers/specs/2026-07-24-prebid-refresh-gam-path-opt-out-design.md:22, inline comment).
Non-blocking
🤔 thinking
- Redundant
targetSlotssubstitution on bare refresh:originalRefresh(undefined)already refreshes all slots, excluded ones included; the conditional only introduces inconsistent snapshot pinning and its comment misstates GPT behavior (crates/trusted-server-js/lib/src/integrations/prebid/index.ts:1224, inline comment).
♻️ refactor
- Set adds no value in the suffix matcher: keep the plain injected array instead of spreading a Set per slot (crates/trusted-server-js/lib/src/integrations/prebid/index.ts:1149, inline comment).
- Canonicalization drift risk: dedup is invoked separately in
build()andvalidate_config_for_startup(); fold it into one shared load/normalize path so a future consumer cannot read the non-canonical list (crates/trusted-server-core/src/integrations/prebid.rs:390, inline comment).
🌱 seedling
- Browser-side guard against a degenerate empty suffix:
adUnitPath.endsWith('')is always true, so an empty string in the injected list would exclude every slot. Server validation prevents it today; a one-line filter (suffix.startsWith('/') && suffix.length > 1) in the browser would preserve the fail-open guarantee if the injected config is ever produced by another path.
CI Status
- fmt: PASS
- clippy: PASS (all six configured targets)
- rust tests: PASS (fastly, axum, cloudflare, spin, CLI, parity)
- js tests: PASS (vitest)
- js lint/format: FAIL (
format-typescript, see blocking finding) - docs format: PASS
- browser/integration tests: PASS
f3dc6ba to
22c14de
Compare
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Approving. Every finding from the previous round is resolved in 22c14deb, and the re-review found no blocking issues. Reviewed at 22c14deb in a clean worktree: all seven changed files read in full, plus the surrounding installRefreshHandler() flow, Settings::get_typed(), and the shim's browser bootstrap.
The design holds up under the checks that matter here:
- Fail-closed server, fail-open browser. Invalid suffixes are rejected in
get_typed(), so the config-store deploy path is covered and not just TOML parsing. In the browser, a missing/non-string/throwinggetAdUnitPath()and a malformed injected list all keep the slot auction-eligible, which is the safe direction — a bad match suppresses demand, a missed match only costs the optimization. - The leading-slash requirement is doing real work. It turns
endsWith()into an implicit segment-boundary match:/123/nottrackingonlydoes not match/trackingonly. That is what makes literal, un-normalized matching safe to expose to operators. - Excluded slots keep their GAM refresh. They stay in
originalRefresh(slots, opts),setTargetingForGPTAsync()is scoped to the auction codes only, andclearRefreshTargeting()runs before filtering so a matching slot cannot serve on a stale header-bid winner. - The
adInitRefreshInProgressbypass is untouched, so thedisableInitialLoad()handoff still delivers server-applied targeting to GAM.
Resolved from the previous round
| Finding | Status |
|---|---|
12 @typescript-eslint/no-explicit-any errors failing format-typescript |
Fixed — tests use the typed test window; npm run lint is clean at --max-warnings=0 |
| Rollout doc invalidated by #967 | Fixed — guide, design §8, and plan all state the filter ships in the server-served tsjs-prebid shim, no bundle regeneration |
| Real GAM network code / publisher name in design doc | Fixed — now /123456/example-news/trackingonly |
Redundant targetSlots substitution on the no-auction path |
Fixed — both paths call originalRefresh(slots, opts), with bare-refresh assertions |
Set re-materialized per slot |
Fixed — plain readonly array, some() called directly |
| Canonicalization repeated at each call site | Fixed — one load_config() used by build and startup validation |
Non-blocking
🤔 thinking / 🌱 seedling / 📝 note
Left inline: mixed-refresh latency for excluded slots (index.ts:1336), no signal on exclusion (index.ts:704), trailing-slash suffix is dead-but-valid config (prebid.rs:354), and canonicalization scope (prebid.rs:400). None block merge.
⛏ nitpick
Design-record drift, docs only — the implementation is right and these are the checked-in artifacts describing it:
docs/superpowers/specs/2026-07-24-prebid-refresh-gam-path-opt-out-design.md:160still says "derive aSet" after the array refactor.- Same file at line 194, and the plan's acceptance criteria at line 196, say stale targeting is cleared from "every target slot". The implementation clears only
independentSlots— publisher delivery slots intentionally keep their already-applied targeting, and the pseudocode omits that split entirely.
👍 praise
- The validation split is the part worth copying: strict and field-specific on the server where a bad value is an operator error, permissive in the browser where a bad value would cost impressions. The four rejection cases each carry their own message, so a startup failure names the problem.
- The browser test matrix goes past the happy path —
it.eachover missing, non-string, and throwinggetAdUnitPath(), plus empty-string and non-array injected suffix lists, plus case-mismatch and trailing-slash cases that pin the literal-matching contract rather than just asserting it in prose. - Rust coverage asserts the negative too:
excludedGamAdUnitPathSuffixesis absent from the injected script when the list is empty, which is what keeps rollback and older-bundle pages behaviorally identical.
CI Status
All 19 checks pass at 22c14deb, including the three integration jobs that were still pending during the previous review.
cargo fmt: PASS- clippy (fastly, axum, cloudflare native + wasm, spin native + wasm): PASS
- rust tests (fastly, axum, cloudflare, spin, cross-adapter parity, ts CLI): PASS
- vitest: PASS
format-typescript/format-docs: PASS- integration tests, browser integration tests, Fastly EC lifecycle: PASS
- CodeQL / Analyze (rust, javascript-typescript, actions): PASS
Verified locally at 22c14deb: npx vitest run test/integrations/prebid/index.test.ts (114 passed), npm run lint (clean), cargo test -p trusted-server-core integrations::prebid (147 passed).
Conflict in the Prebid refresh handler: main (#965) documented that the delegated refresh preserves the publisher's original bare form, while this branch replaced that call with the diagnostics recording plus the scoped dispatch context. Both hold — `dispatchPrebidRefresh` passes `slots` and `opts` through unchanged — so the resolution keeps the diagnostics calls and main's comment, extended to say the wrapper only scopes the shared context. Also add `bid_id` to the auction-ID test provider's `Bid` literal, a field main added in #996 after this branch introduced the provider.
Brings in the PR #997 review fixes: the read-only diagnostics facade split from the internal recorder channel, lazily expiring request-intent evidence with one shared delivery-boundary timer, source-agnostic IDs no longer reported as reservations, the empty-render attempt eviction, and the scoped type-check gate for the export contract. The branch also carries a merge of main, whose #965 and #996 arrive here as squashes of work rc/july already implements more fully. Where the two sides describe the same feature, rc/july's implementation is kept: - APS, adserver_mock, auction/types.rs, auction/formats.rs — rc/july's OpenRTB provider, renderer-aware bid_id precedence, and typed renderer envelope supersede main's versions, which drop fields rc/july needs. - prebid.rs — rc/july canonicalizes the excluded-suffix list at both the startup and build paths already, so main's `load_config` helper adds nothing. Main's test is taken instead of rc/july's: it builds from raw settings rather than reusing the config `validate_config_for_startup` already canonicalized, so it actually exercises the build path. - prebid/index.ts — a bare refresh that filtered slots must deliver the resolved target list, not stay bare, so rc/july's `deliveredSlots` behavior and its test expectation both stand. Three fixes are ported into rc/july's shapes rather than resolved away: - The EC-derived auction ID reached page JavaScript here too, through different plumbing: both collect paths inlined `request.id.as_str()` into `write_bids_to_state`, and page-bids passed it to `build_bid_map_with_auction_id`. All three now mint a per-auction token via `diagnostics_auction_id()`, gated on the diagnostics integration being enabled. - A blank Prebid Cache UUID no longer ships cache coordinates. It loses the hb_adid precedence to `adid` or the bid id, so the Universal Creative would fetch `?uuid=<non-cache-id>` and miss instead of using the inline adm. The gate moves from `is_some()` to `non_empty()`, and main's regression test comes along with rc/july's `Bid` fields added. - The browser-side excluded-suffix list is validated before use. The server only de-duplicates it, so an empty suffix matched every ad unit path and pulled every slot out of the refresh auction, and a non-array value threw inside the publisher's own `refresh()`. Also collapses a duplicated `hb_auction_id` write in `build_bid_map` down to one guarded insert, and points the Prebid refresh recorder at `gptDiagnosticsRecorder` to match the new channel.
Summary
Changes
crates/trusted-server-core/src/integrations/prebid.rscrates/trusted-server-js/lib/src/integrations/prebid/index.tscrates/trusted-server-js/lib/test/integrations/prebid/index.test.tsdocs/guide/integrations/prebid.mdtrusted-server.example.tomldocs/superpowers/{specs,plans}/2026-07-24-prebid-refresh-gam-path-opt-out*Closes
Closes #964
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 serve/tmp/trusted-server-prebid-refresh-path-opt-out.Checklist
unwrap()in production code — useexpect("should ...")println!was added