Improve Prebid auction diagnostics

[ChristianPavilonis]

Summary

• Improve Prebid auction diagnostics so upstream HTTP failures surface status in /auction provider metadata, with capped body previews included only when Prebid debug mode is enabled.
• Treat successful empty Prebid responses (204 No Content or empty 200) as nobid instead of provider error.
• Remove Rubicon from the default client_side_bidders sample config so it is not enabled by default.

Changes

File	Change
crates/trusted-server-core/src/integrations/prebid.rs	Adds upstream HTTP status metadata for non-2xx responses, gates capped Prebid response body previews behind debug, treats empty successful responses as no-bid, uses PREBID_INTEGRATION_ID for provider response IDs, and adds tests for truncation/non-UTF-8 preview behavior.
crates/trusted-server-core/src/auction/orchestrator.rs	Preserves provider launch/parse/conversion errors as provider response metadata using client-safe current-context display text, and adds tests for safe output and truncation.
trusted-server.toml	Changes default client_side_bidders from ["rubicon"] to [].

Compatibility notes

• provider_details may include launch-failure diagnostics before awaited provider responses. Bid selection is order-insensitive, but consumers should not rely on provider_details being ordered by response completion.
• Successful empty Prebid responses now emit BidStatus::NoBid with reason = "empty_response" instead of provider error, which may shift error-rate/no-bid-rate dashboards after merge.

Closes

N/A — no issue filed.

Test plan

• cargo test --workspace
• cargo clippy --workspace --all-targets --all-features -- -D warnings
• cargo fmt --all -- --check
• JS tests: cd crates/js/lib && npx vitest run
• JS format: cd crates/js/lib && npm run format
• Docs format: cd docs && npm run format
• WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
• Manual testing via fastly compute serve
• Other: cargo test -p trusted-server-core prebid -- --nocapture; cargo test -p trusted-server-core auction -- --nocapture; targeted review-feedback tests for provider_error, prebid_body_preview, and parse_response_non_success

Checklist

• Changes follow CLAUDE.md conventions
• No unwrap() in production code — use expect("should ...")
• Uses log macros per CLAUDE.md (template says tracing, but this repo standard is log)
• New code has tests
• No secrets or credentials committed

[aram356]

Summary

Richer /auction diagnostics and correct no-bid classification for empty Prebid responses are well-motivated and well-tested. One blocker: error-stack Report Debug formatting bleeds internal source paths into the public /auction JSON response, which contradicts the explicit policy in error.rs separating client-facing messages from server-only details.

Blocking

🔧 wrench

• Information disclosure via format!("{error:?}"): provider_error_message ships error-stack Debug output (including at <file>:<line> entries) into the public /auction provider_details[].metadata.message. See inline. Switch to error.current_context().to_string() to keep the user-safe Display text but drop file/line/stack.

Non-blocking

🤔 thinking

• Upstream body_preview proxied to public clients: The first 1000 chars of upstream Prebid error bodies are returned in the /auction response. Consider gating on self.config.debug (mirrors the existing trace-log gate) or a request-time header. See inline.
• provider_details ordering changed: Launch failures now precede awaited responses in responses, whereas previously the list was ordered by completion. select_winning_bids is unaffected, but external consumers parsing provider_details see a different order. See inline.

♻️ refactor

• Use PREBID_INTEGRATION_ID constant: New AuctionResponse::error("prebid", …) / ::no_bid("prebid", …) call sites should use the existing constant (crates/trusted-server-core/src/integrations/prebid.rs:36) instead of the literal. See inline.

🌱 seedling

• Standardize diagnostic metadata across providers: Orchestrator-level error_type tagging is provider-agnostic, but only Prebid produces http_status / body_preview for non-success upstream responses. APS/GAM/etc. would be silent on the same failure mode. Worth a follow-up.

📌 out of scope

• client_side_bidders = [] is a silent client-side capability change: The JS bundle still ships the rubicon adapter at build time (crates/js/lib/build-all.mjs:32 → DEFAULT_PREBID_ADAPTERS = 'rubicon'), but the runtime default in trusted-server.toml no longer enables it. Publishers copying the example config will lose rubicon client-side without an adapter-time signal. Worth a CHANGELOG / release note.

⛏ nitpick

• Missing truncation tests: Neither prebid_body_preview (1000-char cap, String::from_utf8_lossy) nor provider_error_message (500-char cap) has a test pinning truncation/non-UTF-8 behavior. See inline.
• format!("{error:?}") allocates before truncation: Long error chains materialize a multi-KB string before chars().take(500) discards the tail. Becomes a non-issue if the wrench above is fixed (Display output is much shorter).

CI Status

• fmt: PASS
• clippy: PASS
• rust tests: PASS (incl. 4 new prebid + 1 orchestrator)
• js tests: PASS
• format-typescript / format-docs / browser integration tests / CodeQL: PASS

[aram356]

Summary

The diagnostic improvements are well-scoped and the prior round's findings are cleanly addressed (current-context truncation, debug-gated body_preview, PREBID_INTEGRATION_ID constant, regression tests for the 500/1000-char caps and attached-detail suppression). CI is green and the new tests pin the right boundaries.

The remaining concerns are about consistency of that boundary: a few error paths slip past the debug gate that the 4xx path now establishes. Inline comments below cover the three specific call sites; cross-cutting items are listed here.

Blocking

🔧 wrench

• Parse-error message bypasses debug gate: parse_response returns serde-error detail + body byte length unconditionally, while the 4xx branch hides body content behind config.debug (crates/trusted-server-core/src/integrations/prebid.rs:1141).
• warn! emits up to 1000 chars of upstream body without debug gate: previously trace-only; this is a logging behavior change worth gating in line with the metadata-path treatment (crates/trusted-server-core/src/integrations/prebid.rs:1112).
• Launch-failure metadata exposes internal message strings: e.g. signing-key kid via RequestSigner::from_services(). provider_error_message is sound; the issue is at the call site choosing to emit current_context() verbatim for error_type=launch_failed (crates/trusted-server-core/src/auction/orchestrator.rs:385).

Non-blocking

🌱 seedling

• Network-layer select() failures still drop providers from provider_responses (crates/trusted-server-core/src/auction/orchestrator.rs:481-486). The PR adds metadata for launch_failed, parse_response, and unsupported_response_body, but transport-level errors emerging from select() Err — and providers still pending after the deadline (backend_to_provider leftovers) — produce no entry. /auction clients can't distinguish "no bid" from "transport error" for those. Pre-existing, but the PR's stated intent is unified observability so it's a natural follow-up.

📌 out of scope

• JS bundle still defaults to bundling the Rubicon adapter (crates/js/lib/build-all.mjs:32). With client_side_bidders = [] now the default in trusted-server.toml, the bundled Rubicon Prebid.js adapter is dead weight unless a publisher overrides the list. Either default TSJS_PREBID_ADAPTERS='' to match, or have build-all.mjs derive the adapter list from the merged TOML. Worth a follow-up issue.

CI Status

• fmt: PASS
• clippy: PASS
• rust tests: PASS
• js tests: PASS
• integration / browser tests: PASS
• CodeQL / format-typescript / format-docs: PASS

[prk-Jr]

Summary

The remaining review concern is in the Prebid error-body preview path. The public preview is capped, but the helper still performs uncapped UTF-8/lossy processing before the cap is applied, and it is invoked even when debug mode is off.

Blocking

🔧 wrench

• Uncapped preview work before truncation: prebid_body_preview converts the entire upstream body with String::from_utf8_lossy(body) before taking the first 1000 chars, and the non-success response path calls it before checking config.debug. Large or invalid UTF-8 error bodies can still drive uncapped validation/allocation even when no preview will be exposed. See inline.

CI Status

• GitHub checks: Analyze (javascript-typescript) PASS
• Local fmt: PASS (cargo fmt --all -- --check)
• Local targeted Rust tests: PASS (prebid_body_preview; provider_error_response)

[aram356]

👍 Looks good

[prk-Jr]

👍 Looks good

[aram356]

Summary

Deep re-review against main at HEAD 50e3000d. Two prior approvals (aram356, prk-Jr) and all CI green. The PR has iterated through three rounds of reviewer feedback and addressed every prior finding (Debug→Display, debug-gated body previews, static launch-failure messages, PREBID_INTEGRATION_ID constant, byte-bounded from_utf8_lossy, dedicated truncation tests). The remaining findings below are forward-looking invariants and one cross-cutting concern; none block correctness.

Blocking

🔧 wrench

• SECURITY invariant comment on provider_error_message: safe today because all change_context sites use static or operator-controlled message strings, but a future provider interpolating upstream-controlled content into TrustedServerError::*.message would silently undo this PR's security work. See inline at crates/trusted-server-core/src/auction/orchestrator.rs:25.
• Lift error_type strings to constants: "http_status" / "parse_response" / "unsupported_response_body" / "launch_failed" / "empty_response" are now an implicit public contract — promote to module constants to prevent typos and centralize the taxonomy. See inline at crates/trusted-server-core/src/auction/orchestrator.rs:41.

Non-blocking

🌱 seedling

• Opt-in for provider diagnostics exposure: error_type / message / http_status / body_preview are now unconditionally surfaced on /auction. Some production operators may prefer server-logs-only diagnostics and a minimal public response. Worth a follow-up expose_provider_diagnostics: bool flag in AuctionConfig (default true for backward compat). Out of scope for this PR.

📝 note

• No-bid taxonomy shift: a 200 OK response with empty body previously failed serde_json::from_slice(&[]) and surfaced as a provider error; now it is BidStatus::NoBid with reason = "empty_response". This will shift the error-rate ↔ no-bid-rate ratio in any dashboards comparing pre/post merge. Worth a one-line release note.

⛏ nitpick

• UTF-8 boundary test gap: prebid_body_preview_ignores_bytes_after_bounded_slice exercises the byte cap with ASCII input only; the adversarial case is a multi-byte character straddling the byte boundary. See inline at crates/trusted-server-core/src/integrations/prebid.rs:3202.

CI Status

• fmt: PASS
• clippy: PASS
• cargo test: PASS
• vitest: PASS
• browser/integration: PASS

Locally verified cargo test -p trusted-server-core --lib against the touched test names (all 12 new tests pass) and cargo clippy -p trusted-server-core --all-targets --all-features -- -D warnings (clean).

[aram356]

Summary

Improves Prebid auction diagnostics: non-2xx upstream responses surface error_type/http_status (and a debug-gated body_preview) in /auction provider metadata, empty 204/200 responses become NoBid with reason=empty_response instead of provider error, and rubicon is dropped from the default client_side_bidders. Correctness is solid, the public-leak surface is handled deliberately, and coverage is thorough. Approving; the notes below are non-blocking.

I verified the full data path AuctionResponse.metadata → ProviderSummary → provider_details → public /auction response (no auth/debug gate), and confirmed all providers (prebid, aps, adserver_mock) keep upstream content out of error messages, so the diagnostic message field is safe.

Non-blocking

🌱 seedling

• Public-leak invariant is comment-enforced: exposing metadata["message"] is safe only while every provider keeps upstream content out of TrustedServerError messages — enforced by a comment, not types (crates/trusted-server-core/src/auction/orchestrator.rs:23).

🤔 thinking

• Partial launch-failure diagnostics: only the request_bids Err path emits a launch_failed entry; budget-exhausted / no-backend / disabled / unregistered skips vanish from provider_details (crates/trusted-server-core/src/auction/orchestrator.rs:402).

📝 note / ⛏ nitpick

• rubicon adapter still bundled by default despite empty client_side_bidders (trusted-server.toml:48, crates/js/lib/build-all.mjs:32).
• Over-engineered UTF-8 boundary test asserts std from_utf8_lossy behavior more than the helper (crates/trusted-server-core/src/integrations/prebid.rs:3207).

CI Status

CI on the PR ran only CodeQL (Analyze: PASS); Rust gates did not run, so verified locally on head 5f00e971:

• fmt: PASS
• clippy (-D warnings): PASS
• rust tests: PASS (966 passed, 0 failed)
• js tests: not run (no JS changes in this PR)

[aram356]

🌱 seedling — The safety of exposing metadata["message"] in the unauthenticated /auction response (ProviderSummary → provider_details, no debug/auth gate) rests entirely on every provider keeping upstream-controlled content out of TrustedServerError::*.message. That invariant is enforced by this comment, not the type system. Today all providers comply (verified prebid, aps, adserver_mock — all static messages, and current_context() strips error-stack attachments). But a future provider doing change_context(TrustedServerError::Auction { message: format!("bad body: {body}") }) would silently leak upstream bytes to all callers. Consider a dedicated safe-message newtype / redaction wrapper, or a test asserting the invariant across registered providers.

[aram356]

🤔 thinking — Launch-failure diagnostics are partial: only the request_bids Err path emits a launch_failed provider entry. Providers skipped earlier in Phase 1 — effective_timeout == 0 (budget exhausted), missing backend_name, disabled, or not-registered — still continue silently and never appear in provider_details. The budget-exhausted case is arguably the most useful diagnostic and is the one now silently dropped. Intentional scope, or worth emitting a skip entry there too?

[aram356]

📝 note — client_side_bidders = [] means nothing uses rubicon client-side by default, but crates/js/lib/build-all.mjs still sets DEFAULT_PREBID_ADAPTERS = 'rubicon', so the bundle continues to ship the rubicon Prebid.js adapter. Not a bug — just dead weight / a config inconsistency worth a follow-up.

[aram356]

⛏ nitpick — This test spends most of its body asserting String::from_utf8_lossy (std) behavior on a hand-sliced buffer, and its own comment notes the char cap dominates so the boundary case is "beyond the public character cap for ASCII input." The valuable assertions are on the helper output (capped char count, no "tail"); the bounded-slice pinning exercises std, not our code, and could be trimmed.