Exclude GAM paths from Prebid refresh auctions - #965
Conversation
f431f73 to
f3dc6ba
Compare
prk-Jr
left a comment
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
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
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).
| return Array.isArray(value) ? value.filter(isUsableRefreshAuctionExclusionSuffix) : []; | ||
| } | ||
|
|
||
| function isExcludedFromRefreshAuction( |
There was a problem hiding this comment.
🌱 seedling — No signal is emitted when a slot is excluded, so a too-broad suffix is invisible in the field. Because clearRefreshTargeting() already ran and no replacement auction follows, a display slot that matches by accident loses Trusted Server demand on every refresh after the first impression, and the only symptom is missing hb_* targeting in GAM.
A single debug line (or a counter surfaced through the GPT diagnostics overlay from #974) would make that misconfiguration diagnosable without a code change:
const excluded =
typeof adUnitPath === 'string' &&
excludedGamAdUnitPathSuffixes.some((suffix) => adUnitPath.endsWith(suffix));
if (excluded) {
log.debug(`[tsjs-prebid] refresh auction excluded ${adUnitPath}`);
}
return excluded;Not for this PR if you'd rather keep the predicate allocation-free — noting it as the follow-up that makes the opt-out observable.
| log.error('[tsjs-prebid] refresh targeting failed', error); | ||
| } | ||
| } | ||
| // Preserve the publisher's original refresh form. In particular, a bare |
There was a problem hiding this comment.
🤔 thinking — Correct for SRA, but worth naming the remaining cost: in a mixed global refresh the excluded slot's GAM refresh is still withheld until the auction completes (or the timeoutMs watchdog fires), because the whole original list goes out in one originalRefresh(slots, opts) call. So an excluded tracking slot avoids /auction but not the added latency — up to ~1.5s — unless every slot in that refresh is excluded, which is the only path that returns immediately.
That is the right tradeoff (splitting the refresh would break Single Request Architecture and change the publisher's request shape), and the design doc's §5.3 table does describe the ordering. The guide's caveat list is where an operator would look, though: it currently says matching slots "still refresh through GAM" without noting the mixed-refresh delay. One sentence there would set the expectation for anyone opting out a slot for timing reasons rather than for demand reasons.
| error | ||
| } | ||
|
|
||
| fn validate_excluded_gam_ad_unit_path_suffix(value: &str) -> Result<(), ValidationError> { |
There was a problem hiding this comment.
🌱 seedling — The validator rejects every input that cannot be a path suffix except one that can never match: a trailing slash. "/trackingonly/" passes all four checks, injects fine, and then silently matches nothing, since GAM ad-unit paths do not end in / — the PR's own '/123/trackingonly/' test case documents that direction of the asymmetry.
Given the design deliberately does no slash normalization (a good call — literal matching is auditable), rejecting the dead form at startup is more consistent than accepting it:
if value.ends_with('/') {
return Err(excluded_gam_ad_unit_path_suffix_validation_error(
"excluded_gam_ad_unit_path_suffixes entries must not end with '/'",
));
}This also subsumes the value == "/" case. Fine as a follow-up — the current behavior is fail-open, not wrong.
| config.excluded_gam_ad_unit_path_suffixes = canonical; | ||
| } | ||
|
|
||
| fn load_config( |
There was a problem hiding this comment.
📝 note — This is the right resolution of the earlier canonicalization concern: build() and validate_config_for_startup() can no longer disagree, and the test now asserts both paths plus the injected payload.
For the record on what remains: canonicalization is still a property of this helper rather than of deserialization, so a future caller that reaches for settings.integration_config::<PrebidIntegrationConfig>(PREBID_INTEGRATION_ID) directly would get the raw list. Today that is only reachable from tests, and because matching is some(endsWith), a duplicate suffix changes nothing observable beyond a slightly larger injected payload — so no action needed here.
3 participants
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