Suppress fabricated empty Prebid bidder params

[prk-Jr]

Summary

Stacked on #899 (feat/ssat-render-inline-creative) — base branch is that PR's head, not main.

A configured Prebid bidder with no inline params and no matching bid-param override was expanded into "bidder": {} in the OpenRTB request. PBS rejects an empty bidder object, surfacing as the intermittent auction error.

Fix

In to_openrtb (crates/trusted-server-core/src/integrations/prebid.rs):

• Track the bidders the publisher explicitly supplied (including an explicit empty {}).
• Reorder to: build bidder map → apply overrides → drop fabricated empties → decide stored-request.
• Drop any bidder still an empty object after overrides unless it was explicitly supplied.
• Recompute the PBS stored-request fallback after the drop, so a slot whose configured bidders all resolve to fabricated empties falls back to the stored-request/no-bid path instead of shipping invalid bidder objects.

Behavior contract:

• Absent bidder → dropped (never "bidder": {}).
• Publisher-supplied {} → preserved (misconfiguration stays visible).
• Bidder populated by a global/zone override → kept.
• Valid bidders still ship even when a sibling has no params.
• No eligible bidders → stored-request fallback.

The browser half of this defense (prevBidderParams retention on refresh) already shipped; this completes the server half.

Tests

New unit tests in prebid.rs:

• to_openrtb_drops_fabricated_empty_bidder_params
• to_openrtb_preserves_an_explicitly_empty_bidder
• to_openrtb_keeps_a_fabricated_bidder_that_an_override_populates
• to_openrtb_falls_back_to_stored_request_when_all_bidders_are_fabricated_empty

Verification

• cargo fmt --all -- --check ✓
• cargo clippy-fastly ✓
• cargo test-fastly ✓ (1628 core + 99 fastly, 0 failed)
• Other adapters (axum/cloudflare/spin) + clippy variants left to CI.

Test plan

• CI green across all adapters
• Manual: slot with a configured bidder lacking params no longer emits "bidder": {} to PBS

[ChristianPavilonis]

Review Summary

Approved with one non-blocking medium-severity finding documented inline. The primary fabricated-empty suppression, override ordering, and stored-request fallback behavior are covered by focused tests.

Please address the inline mixed-representation precedence case in follow-up work before relying on this path for clients that can send both direct bidder params and trustedServer.bidderParams.

[aram356]

Summary

The fabricated-empty suppression, override ordering, stored-request fallback, and focused tests are otherwise sound, but mixed bidder representations can still recreate the invalid empty bidder object nondeterministically.

Blocking

🔧 wrench

• Mixed bidder representations have nondeterministic precedence: direct bidder params can be overwritten by a fabricated empty object depending on HashMap iteration order (crates/trusted-server-core/src/integrations/prebid.rs:1440).

CI Status

• GitHub integration checks: PASS (4/4)
• fmt: PASS locally
• focused native core tests: PASS (44/44)
• Fastly test runtime: BLOCKED locally by unavailable macOS certificate keychain after successful compilation
• clippy-fastly: BLOCKED locally by a build-script tooling error (Unrecognized option: 'p')
• JS tests: not run; no JS/TS files changed

[aram356]

Summary

The or_insert fix in bfea103d9 does resolve the case flagged in the previous round — I confirmed the new 64-iteration test genuinely fails against the old extend and passes now. The reordering (build → override → drop → decide stored-request) is sound, and tracing the history shows this PR is finishing a migration #680 already intended rather than inventing a new policy: e06af4b0f abandoned "fill config.bidders with empty {}" in favour of the stored-request path a year ago, and that intent only leaked because expand_trusted_server_bidders fabricates the same empties one layer up. That evidence retired my initial concern that broadening the fallback was risky.

What holds the PR up is that the fix is one-directional, and that the central design choice re-creates the payload the PR exists to eliminate. PBS cannot distinguish a fabricated {} from an explicitly supplied one — they are identical bytes on the wire — so preserving explicit empties keeps emitting "bidder": {} through the primary shipped JS path, and parse_response turns the resulting PBS rejection into an auction-wide bid wipeout rather than a visible misconfiguration signal.

Blocking

🔧 wrench

• Preserving an explicit {} still ships the payload PBS rejects: provenance is invisible to PBS; reachable via the normal Prebid adapter path (params: {} → bidderParams: {kargo: {}}), and a PBS non-2xx zeroes every slot in the auction, not just the offending imp (prebid.rs:1480-1483, prebid.rs:2157-2166).
• The merge is asymmetric — an empty direct entry still wins over real params: the direct branch keeps an unconditional insert, so a direct {} deterministically discards trustedServer-supplied params and emits "bidder": {"kargo": {}} (prebid.rs:1453).

❓ question

• The premise underpinning the whole design is unverified in-repo: there is no test, fixture, or PBS mock anywhere backing either "PBS rejects "bidder": {}" or "an unknown stored-request ID yields a per-imp no-bid rather than a request-wide 400". Both exist only as prose in comments and commit messages. If a stored-request miss is in fact a 400, this trades one whole-auction failure mode for another instead of fixing it. Related: auction/endpoints.rs:60-63 already requires a stored request to exist for every slot code that omits inline params, and this PR widens that set to include all-fabricated-empty slots — operators with config.bidders set but no stored requests provisioned move from one broken state to another. Has the empty-bidder rejection been confirmed against the deployed PBS?

Non-blocking

🤔 thinking

• The new "cannot fire" comment is inaccurate: real inline params for a bidder outside config.bidders do fall back to the stored request, silently dropping those params (prebid.rs:1490-1493).
• Non-object params bypass the drop: null params ship as "bidder": {"kargo": null}; #[serde(default)] on BidConfig.params makes this reachable from a POST that omits params (prebid.rs:1481).

♻️ refactor

• Extract an is_empty_object helper: the emptiness predicate is inlined in the retain at prebid.rs:1481 and is needed again by both suggested fixes above. A shared fn is_empty_object(params: &Json) -> bool { params.as_object().is_some_and(serde_json::Map::is_empty) } keeps the rule in one place.

📌 out of scope

• zone leaks into every bidder's params: when a trustedServer entry has no bidderParams, expand_trusted_server_bidders (prebid.rs:942-957) hands each configured bidder the entire trustedServer object as its params, so a zone-only slot ships {"kargo": {"zone": "header"}, "triplelift": {"zone": "header"}} — a TS-internal orchestration key sent to PBS as a bidder param. These are non-empty, so the new drop never catches them; it's the third route by which fabricated params reach PBS. Pre-existing and not this PR's job, but it limits the "fabricated params never ship" contract and is worth a follow-up issue.

CI Status

• GitHub checks: PASS (4/4)
• fmt: PASS (verified locally)
• clippy (trusted-server-core, native host, -D warnings): PASS (verified locally)
• rust tests (trusted-server-core, native host): PASS (1631/1631, verified locally)
• js tests: not run — no JS/TS files changed

Note: #899 is still open, so this stays stacked and cannot merge first.

[aram356]

Summary

All four findings from the previous round are resolved, and I verified each against 6a75cc499 rather than taking the fix on description:

Finding	Before	After
🔧 Explicit {} ships the rejected payload	{"bidder":{"kargo":{}}}	{"storedrequest":{"id":"s"}}
🔧 Asymmetric merge drops real params	LOST PARAMS on iter 0	0/64 bad
🤔 null params bypass the drop	{"bidder":{"kargo":null}}	{"storedrequest":{"id":"s"}}
🤔 Inaccurate "cannot fire" comment	inaccurate	corrected, pinned by test

The merge rewrite is the right shape. Collecting expanded and direct separately and merging with an explicit clobbers_real guard is what makes this order-independent — no reordering of the previous single loop could have achieved it, since whichever branch ran first would win. I also checked the null-clobbers-real variant I hadn't explicitly raised (0/64 bad), and confirmed valid params still ship unchanged. is_unusable_bidder_params folds in the shared predicate, and #927 tracks the zone leak.

Verification on this head: cargo fmt --all -- --check PASS; clippy (trusted-server-core, --all-targets, -D warnings) clean; 1634/1634 core tests pass (+3 new); GitHub checks green 4/4.

Non-blocking

🤔 thinking

• The unconditional drop now warns on a designed path: creative-opportunity slots intentionally emit bidderParams: {}, so each one logs a warn per configured bidder calling its intended behavior a misconfiguration (prebid.rs:1498-1503, creative_opportunities.rs:294-305). Detail inline.

📌 out of scope

• The PBS premise remains unverified: your answer is fair — whether a stored-request miss is a per-imp no-bid or a request-wide 400 is deployed-PBS behavior and genuinely not knowable from this repo, and this PR does verifiably remove definitely-rejected payloads via the fallback #680 already established. It shouldn't hold the PR. But it stays an untested assumption underpinning the design, and there's still no PBS mock or fixture anywhere in the repo to pin it. Worth an issue alongside #927 so it's tracked rather than dropped — happy to file it if you'd prefer.

Approving. Note this stays stacked on #899, which is still open.

CI Status

• GitHub checks: PASS (4/4)
• fmt: PASS (verified locally)
• clippy (trusted-server-core, native host, -D warnings): PASS (verified locally)
• rust tests (trusted-server-core, native host): PASS (1634/1634, verified locally)
• js tests: not run — no JS/TS files changed

[aram356]

🤔 thinking — This now warns on a designed path, not just a misconfigured one.

Making the drop unconditional was the right call, but it widened what reaches this warn. creative_opportunities.rs:294-305 deliberately emits bidderParams: {} — its own comment says it gives "each bidder an empty params object that the override engine then fills with zone-aware rules". When no override matches that bidder/zone, every configured bidder is dropped here and each one logs a warning. I confirmed the shape lands in the fallback:

N2 creative-opp-empty-bidderParams => {"storedrequest":{"id":"s"}}

So a routine creative-opportunity slot emits N warnings (one per configured bidder) describing its intended behavior, and the message tells the operator it's a misconfiguration falling back to a stored request. Warn-level noise on an expected path erodes the signal this log exists to provide — the genuine publisher misconfiguration you added it for gets buried under the designed case.

Since the two are distinguishable at this point, they could log differently — e.g. keep warn when the slot supplied some inline params (a real publisher mistake), and drop to debug when the expansion fabricated everything (creative-opportunity / stored-request path). Even just collapsing to one line per slot rather than one per bidder would help.

Non-blocking — behavior is correct either way, this is purely about operability.