Add configuration-driven OpenRTB auction providers - #1016
Conversation
ChristianPavilonis
changed the title
8de9eab to
0c98095
Compare
|
@ChristianPavilonis to test in staging |
ChristianPavilonis
changed the title
51239d1 to
ba2eea6
Compare
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Large, coherent rework: provider identity, routing, transport, and response handling all move out of singleton integrations into one immutable, validated AuctionPlan shared by every adapter. The plan compiler, profile registry, and pure backend-naming policy are well factored, and the new validation (endpoint canonicalization, backend-name collision prediction, notification seat limits, static-extension bounds) is thorough.
Two blocking issues: the required cargo test check is failing because of this PR, and the config schema break ships with no operator-facing migration note.
1 of the inline comments below carries a one-click GitHub
suggestion— use Commit suggestion to apply it as a commit on the PR branch. The remaining comments describe the fix in prose because the change spans multiple files, touches lines outside the diff, or is a design question rather than a patch.
Blocking
wrench
- Required
cargo testjob fails: template-cache harness configures no auction provider — see Cross-cutting below - Breaking config cutover with no CHANGELOG entry and no safe deploy ordering — see Cross-cutting below
- Dead tautological assertion in the moved registration loop — see inline at
crates/trusted-server-core/src/integrations/registry.rs:827
Non-blocking
thinking / refactor
- One malformed envelope entry silently zeroes a slot's server-side demand — see inline at
crates/trusted-server-core/src/auction/routing.rs:434 - HTTPS-only endpoint canonicalization blocks loopback stub endpoints — see inline at
crates/trusted-server-core/src/auction/plan.rs:570 #[cfg(test)]orchestrator harness re-implements the production dispatch path — see inline atcrates/trusted-server-core/src/auction/orchestrator.rs:318run_auctioncarries the same body twice under oppositecfggates — see inline atcrates/trusted-server-core/src/auction/orchestrator.rs:849apply_prebidpairs imps to slots positionally — see inline atcrates/trusted-server-core/src/auction/openrtb.rs:280- Browser shim ownership inverted: unowned bidders now fail open to client-side — see Cross-cutting below
praise
- Response currency is finally checked — see inline at
crates/trusted-server-core/src/auction/openrtb.rs:552
Cross-cutting / body-level findings
-
wrench — Required
cargo testjob fails: the template-cache harness no longer configures any auction provider.scripts/template-cache-local-test.shis not touched by this PR, but thecargo testjob runs it, and it patches the example config by literal string replacement:s = s.replace('[integrations.prebid]\nenabled = false\nserver_url = "https://prebid.example.com/openrtb2/auction"', ...) s = s.replace('providers = []', 'providers = ["prebid"]', 1)
Both target strings were deleted from
trusted-server.example.tomlby this PR (grep -c 'providers = \[\]'andgrep -c server_urlboth return 0), so both replacements are silent no-ops. The stub then runs with[auction] enabled = trueand zero providers, so no bids are produced and 6 assertions fail in thecargo testjob:FAIL a bids script is present — got '0', want '1' FAIL the seam carries slot definitions, not just bids — got '0', want '1' FAIL the slot definitions reach the guarded scheduler — got '0', want '1' FAIL the winning bid's bucketed price reaches the reader — got '0', want '1' FAIL the served seam failed the real GPT module contract: Error: served document has no executable seam payload FAIL cache hit streams: the article is delivered before the auction resolves — got 'no', want 'yes' 15 passed, 6 failedRewriting the replacements to the new map shape is not sufficient on its own:
canonicalize_endpointrequiresscheme == "https"(crates/trusted-server-core/src/auction/plan.rs:570) and the harness stub endpoint ishttp://127.0.0.1:{port}/bid. The previous path accepted any scheme, sinceserver_urlcarried only#[validate(url)]. The harness needs either an HTTPS stub backend or an explicit loopback exemption in endpoint validation. -
wrench — Breaking config cutover ships with no CHANGELOG entry and no safe deploy ordering. Two schema breaks land together:
[auction].providerschanges from a list to a map (the list shape is explicitly rejected), andPrebidIntegrationConfigis rebuilt as a browser-only config with#[serde(deny_unknown_fields)], droppingserver_url,bidders, and the server-side override fields.IntegrationRegistry::with_plancallsprebid::register_for_plan, which doessettings.integration_config::<PrebidIntegrationConfig>(PREBID_INTEGRATION_ID)?. A live config blob still carryingserver_urltherefore fails to parse, the error propagates out ofbuild_state_from_settings, and the adapter comes up on the startup-error router. The reverse ordering fails too: a map-shaped blob does not parse on a binary that predates this PR. There is no deploy ordering that avoids an outage window — binary and config have to cut over together.CHANGELOG.mdis untouched by this PR. The repo documents exactly this class of change under[Unreleased] / Changedwith a Breaking marker and explicit upgrade/rollback ordering (see thesanitize_creativesand APS OpenRTB entries, both of which spell out "upgrade the binary first, then push the config" and the rollback constraint). This change needs the same treatment, including the fact thatts config pushnow rejects the old shape and that the previous log-and-strip tolerance for unknownbidders/client_side_biddersentries is now a hard startup error. -
thinking — Browser shim ownership inverted: unowned bidders now fail open to client-side. In
crates/trusted-server-js/lib/src/integrations/prebid/index.ts,installPrebidNpmpreviously folded every bidder not listed inclientSideBiddersinto thetrustedServerenvelope and stripped it fromunit.bids. It now folds only codes present inserverSideBidders(that is,[auction.bidders]) and leaves everything else in browser demand:unit.bids = unit.bids.filter( (bid) => bid?.bidder === ADAPTER_CODE || !serverSideBidders.has(bid?.bidder ?? '') );
An operator who upgrades the binary and pushes a config without populating
[auction.bidders]silently moves all demand from the server-side auction to direct browser SSP calls. There is no error and no warning;validate_browser_bidder_ownershiponly rejects codes claimed by both sides at once. Worth calling out explicitly in the migration note above, since it is a first-party-proxying regression that will not show up as a failure anywhere.
CI Status
- cargo test: FAIL (required)
- cargo fmt: PASS (required)
- format-typescript: PASS (required)
- format-docs: PASS (required)
- cargo test (axum native): PASS
- cargo test (cloudflare native): not reported separately;
cargo check (cloudflare native + wasm32-unknown-unknown): PASS - cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- vitest: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
- CodeQL: PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS (reported twice, from two workflow runs)
- Analyze (actions): PASS
The suggestion in this review was applied in an isolated worktree at ba2eea6 and verified against the full gate: cargo fmt --all -- --check, all six clippy aliases, cargo test-fastly / test-axum / test-cloudflare / test-spin, and the cross-adapter parity suite — all pass, with no drift between the approved bytes and the post-verification tree.
|
Addressed the requested changes in 35e1897:
I deliberately retained fail-closed malformed-envelope handling, HTTPS-only provider endpoints, and configured browser/server bidder ownership. The broader Validation passed across Fastly, Axum, Cloudflare, Spin, CLI, parity, clippy, JS, formatting, and both template-cache harness modes. All inline threads have replies and are resolved. |
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Large, disciplined rework: provider identity, routing, transport, and response handling move out of singleton integrations into one immutable, validated AuctionPlan shared by every adapter, with a common OpenRTB 2.6 driver and typed Standard / Prebid Server / APS profiles. The plan compiler and backend-naming consolidation are strong (deterministic ordering, strict validation that runs even when the auction is disabled, byte-stable Fastly backend names, unusually thorough tests). The blocking items are concentrated in three places: the all_eligible routing mode is only half-integrated for the prebid-server profile, the planned parsers add silent bid-admission rules under a parity claim, and the rewritten environment-override documentation teaches mechanisms the pinned EdgeZero overlay rejects.
16 of the inline comments below carry a one-click GitHub
suggestion; use Commit suggestion (or Add suggestion to batch) to apply them. Every suggestion was verified in a scratch worktree: applied in isolation (rustfmt, target-matched clippy, adapter checks, prettier) and then all together against the full CI gate (all six clippy aliases, all four adapter test suites, the cross-adapter parity suite, vitest, and the docs/JS format checks). The remaining comments describe fixes in prose because they span multiple files or lines outside the diff.
Blocking
wrench
all_eligibleis half-integrated for theprebid-serverprofile (cross-cutting, details below)- Array/table env-override docs do not work under the EdgeZero overlay - see inline at
docs/guide/configuration.md:174-191,docs/guide/configuration.md:1282,docs/guide/integrations/prebid.md:313,docs/guide/integrations-overview.md:346,docs/guide/error-reference.md:129-130 AuctionProvidertrait snippet documents a trait that does not exist - see inline atdocs/guide/auction-orchestration.md:330- Failed CI check: CodeQL (cross-cutting, details below)
question
- Planned PBS parser silently drops bids the legacy path delivered - see inline at
crates/trusted-server-core/src/integrations/prebid.rs:2119 - Unowned page bidders vanish from refresh auctions - see inline at
crates/trusted-server-js/lib/src/integrations/prebid/index.ts:1238 - APS enable/disable semantics inverted - see inline at
crates/trusted-server-core/src/integrations/aps.rs:1878 - All-providers-failed auctions now return HTTP success - see inline at
crates/trusted-server-core/src/auction/orchestrator.rs:1773
Non-blocking
thinking / refactor / nitpick / note / seedling
- Dead PII-retaining header snapshot in planned dispatch - see inline at
crates/trusted-server-core/src/auction/orchestrator.rs:1788(suggestion) - Harness mislabels select-failure drains as timeouts - see inline at
crates/trusted-server-core/src/auction/orchestrator.rs:529 - Standard-profile extraction requires optional
w/h- see inline atcrates/trusted-server-core/src/auction/openrtb.rs:619 - Disabled-auction path emits no telemetry - see inline at
crates/trusted-server-core/src/auction/endpoints.rs:182 dnt: Option<bool>can never beSome(false)- see inline atcrates/trusted-server-core/src/auction/routing.rs:296- Direct slot demand bypasses
MAX_BIDDER_ENTRIES- see inline atcrates/trusted-server-core/src/auction/routing.rs:438 - Injected-config escaping weaker than the bids-script escaper - see inline at
crates/trusted-server-core/src/integrations/prebid.rs:1340 - Commented-out tests dropped live coverage - see inline at
crates/trusted-server-core/src/integrations/prebid.rs:4223 - Explicit
nullrenderer carrier poisons APS bids - see inline atcrates/trusted-server-js/lib/src/integrations/prebid/index.ts:1022(suggestion) - Suggestions for smaller items inline at:
plan.rs:101,plan.rs:593,profile.rs:211,openrtb.rs:35,openrtb.rs:469,openrtb/test_executor.rs:44,openrtb/tests.rs:261,backend.rs:209(fastly),registry.rs:822,settings.rs:181,docs/guide/api-reference.md:88
Cross-cutting / body-level findings
- 🔧
all_eligibleis half-integrated for theprebid-serverprofile. Three symptoms, one root cause. (1) A PBS provider withrouting = "all_eligible"and browserbidderParamsfor a bidder with no[auction.bidders]route emitsimp.ext = {"prebid":{}}(openrtb.rs:299-308; the golden atopenrtb/tests.rs:740pins exactly this shape). Prebid Server rejects any imp lacking a bidder or stored request, so one such imp 400s the entire multi-slot request and slot-level demand elsewhere in the auction is lost, with only an internal unroutable-bidder counter as a trace. (2)plan.browser_bidder_codes()returns only explicit route keys (plan.rs:495-497), so an all-eligible PBS provider injects"serverSideBidders":[]and the browser cannot suppress its own client-side requests for those bidders. (3)validate_browser_bidder_ownershipdraws from the same source and is blind to double-ownership for such providers. Recommended fix: rejectrouting = "all_eligible"for theprebid-serverprofile at plan compile time unless every browser bidder is mapped, or wire all three surfaces to include all-eligible providers. Changing the imp-construction fallback alone would require a deliberate golden change. - 🔧 Failed CI check: CodeQL, "6 new alerts including 6 high severity" (not in the required set). All six are
rust/cleartext-logginginorchestrator.rs(lines 752, 1209, 1897, 1970, 2021, 2344) and all six are taint-analysis false positives: CodeQL taints the entireSettings/RuntimeServicesobject graph because construction touchesvalidate_tinybird_secret/validate_admin_handler_passwords/ secret-store readers, but the flagged statements log only provider IDs, backend names, andu32timeout/count values; five of the six are in#[cfg(test)]-only or production-unreachable code, and the sixth logs a mediator ID plus four budgets. No statement in the changed files logs secret material. The file already carrieslgtm[rust/cleartext-logging]suppressions inherited from main (new lines 1391, 1872) with the same reasoning; adding the identical two-line suppression at the six flagged statements keeps the check green without behavior change. - ♻️ Un-migrated runtime blobs fail startup with an unactionable error. Every pre-PR blob serializes
auction.providersas an array, so during the unavoidable binary/blob mismatch window of a rolling migration every service printsFailed to deserialize JSON configuration: invalid type: sequence, expected a mapwith no field path and no pointer to the migration (settings.rs:2688-2695; reproduced empirically). Wrap the deserialization inserde_path_to_error(yieldsauction.providers: ...) or pre-check for the old array shape and return a targeted "config uses the removed provider-list schema; re-push after migrating" error. The TOML path already produces precise errors. - 🤔
has_enforceable_total_request_deadlinelives in two unlinked places: the target descriptor (backend_naming.rs:227,242-244, consumed only by tests) and thePlatformHttpClienttrait default (http.rs:299, the runtime consumer atorchestrator.rs:250). Fastly and Axum have lockstep tests; Cloudflare and Spin assert only the descriptor half because their clients are cfg-gated out of native tests. This is precisely the plan-vs-runtime drift shape the PR exists to close for backend names. Derive one side from the other, or at minimum comment the descriptor field naming the trait method that must stay in lockstep. - 🤔 GPC is detected but never transmitted.
has_dataincludesconsent.gpc(openrtb.rs:418-422), butRegsExthas no gpc field, so a gpc-only context emitsregs: {"gdpr":0}and the signal is dropped; downstream bidders cannot honor an opt-out they never receive. If this is deliberate PBS-parity, document it at the parity comment; otherwise addgpctoregs.ext(IAB convention) in a follow-up. - 🤔 Literal seat
"unknown"diverges across paths. The planned PBS parser keepsreturned_seat = Some("unknown")for a literal seat (prebid.rs:2059-2064, test-pinned), while the mock-mediator restore treats"unknown"as absent (adserver_mock.rs:315-318).NotificationConfig.suppress_seatsmatches exact returned seats, so the same upstream seat suppresses notifications on the direct path but not after mediation. Align the mock restore with the direct-path semantics. - 🤔 TESTING.md debugging excerpts reference log lines that do not exist:
"Using auction orchestrator"and"Requesting bids from: prebid"match nothing in the tree, and"Registering auction provider: prebid"now emits configured provider IDs (pbs-main), not profile names (TESTING.md:48, 133-150). Operators grepping for these strings will conclude the orchestrator is not running. - 📝
crates/trusted-server-core/src/auction/README.mdtop half is stale: the request-flow boxes citemod.rs:149/mod.rs:274-322(mod.rs is 233 lines and contains only plan compilation; parsing lives inendpoints.rs), and the route table says routes are defined in the Fastlymain.rswith line numbers (the dispatch lives inapp.rs). The rewritten bottom half is accurate; the top half should reference symbols, not dead line anchors. - 📝
docs/guide/auction-orchestration.md:913still claims raw PBSdebug.httpcallsrequires[integrations.prebid].debug; after this PR that is the provider'sprofile_config.debug, and[integrations.prebid].debugis browser-only (error-reference.md:294 states this correctly). Direct contradiction of the PR's ownership split in the section operators will follow. - 📝
docs/guide/integrations/prebid.md:339-351zone-override walkthrough still illustrates withkargo/client_side_abc/_s2sHeaderPlacement, values that appear nowhere in the rewritten example above it (example-server/example-header-placement). - 📝 Stale rollback-compatibility rationale:
auction_config_types.rs:37-40andtrusted-server.example.toml:154still say omittingrewrite_creativeskeeps blobs rollback-safe, but the new always-serializedbiddersfield makes every re-pushed blob incompatible with the old binary regardless (the PR deleteddefault_auction_payload_is_accepted_by_legacy_schemaand documents wholesale rollback). Rewrite both comments to the post-migration truth. Similarly,auction_config_types.rs:1still explains the file split with abuild.rspath-inclusion rationale that no longer holds (no build.rs references the file, and the newpub usewould not compile under path inclusion). - ♻️ Test boilerplate: roughly 40 sites across
publisher.rs,html_processor.rs, and the integration test modules hand-roll the identical 8-lineIntegrationRegistry::with_plan(..., Arc::new(compile_auction_plan(...)))block, even though cfg(test)IntegrationRegistry::newexists for exactly this. A shared helper collapses the next plan-signature change from a 40-site edit to one. - 🏕 Legacy cfg(test) parity copies should be scheduled for deletion: the planned and legacy implementations of the APS debug headers, renderer, and both response parsers are hand-maintained near-duplicates (~1500 test-only lines across
aps.rsandprebid.rs), and nothing forces the frozen legacy copy to track future planned-path fixes, so parity tests can keep passing while asserting stale behavior. Fine as a transition; please leave a tracking issue. - 📝
head_inserts_for_plantakes&selfplus abrowser_configparameter that is alwaysself.config(prebid.rs:850-861, body never readsself); one of the two should go. The cfg-gated fallback body ofhead_inserts(lines 1361-1404) is unreachable in production and is a readability trap. - ⛏ Hygiene sweep (no inline comments to keep the set focused): missing "should ..." assertion messages in new tests (
auction/mod.rs:142-143,165, axumtests/routes.rs:100, fastlyplatform.rs:893-902, fastlybackend.rs:864-865,creative_opportunities.rs:2000-2002,publisher.rs:13000,prebid.rs:4830-4838);expect()messages with mid-sentence "should" (config.rs:453,461,config_payload.rs:207); test imports stranded below test fns (adapter-cloudflare/src/platform.rs:775-776,adapter-spin/src/platform.rs:800-802);std::iter::IntoIterator::into_iter([...])instead of[...].into_iter()(adapter-cloudflare/src/app.rs:701,745,adapter-spin/src/app.rs:877); APS planned parser uses the literal"http_status"where PBS usesERROR_TYPE_HTTP_STATUS(aps.rs:921-923); doc comments bisected by#[cfg(test)]attributes (orchestrator.rs:926-932,1096-1100);scripts/template-cache-local-test.shbuilds relative to the caller's cwd instead of$REPO_ROOTand its preflight omitspython3/lsof/curl; raw byte-string JSON bodies wherejson!is the convention (prebid.rs:8414,8460); two path spellings for the same re-exported type in publisher tests (publisher.rs:8795vs19555); backend-naming lost the length-arithmetic and ASCII-truncation comments the adapter versions carried (backend_naming.rs:15-17,407-418), andpredict_no_registration's injectivity silently depends on theProviderIdcharset, worth a comment. - 🌱 Coverage pins worth adding: a single-provider plan accepted by
validate_for_targeton Cloudflare/Spin (the>1boundary is tested only from above); invalid provider config still failing compile whenauction.enabled = false(holds today only by code ordering inmod.rs); thestarts_with("missing field ")tolerance atsettings.rs:191exercised with an integration that has genuinely required fields (testlight); both existing tests use all-default structs and never reach the branch, and the serde error-string coupling is otherwise unpinned. Also:AuctionTargetId::from_adapter_idhas no production callers (module doc promises CLI validation that does not reference it), and provider/bidder maps have no sanity cap outside the Fastly backend budget (a fat-fingered 10,000-provider config compiles on Axum and clones the common request per provider at request time). - 📌 Downstream note:
AuctionResponse.providernow carries operator-chosen provider IDs (pbs-main) instead of the literalsprebid/apson the planned path.Bid.biddersemantics are preserved (GAMhb_biddertargeting and renderer selection are safe), but anything matchingresponse.provider == "prebid"must be re-verified.
CI Status
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- CodeQL: FAIL (6 high
rust/cleartext-loggingalerts; all six verified as taint-analysis false positives, see the cross-cutting finding above) - Analyze (rust): PASS
- format-typescript: PASS (required)
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo test: PASS (required)
- Analyze (actions): PASS
- cargo test (axum native): PASS
- cargo fmt: PASS (required)
- format-docs: PASS (required)
- Analyze (javascript-typescript): PASS
- prepare integration artifacts: PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- vitest: PASS
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Follow-up pass on 1e737a8b, scoped to what the existing reviews on this head do not already cover: the new shared backend-naming codec, cross-adapter naming parity, the CI-executed shell harness, and the browser-exposed surface. The plan/OpenRTB/orchestrator/integration/doc findings from the standing CHANGES_REQUESTED review are not repeated here.
No inline comment below carries a one-click suggestion block; each proposed change is given as a plain fenced block to apply manually.
Coverage note, stated plainly: this pass verified platform/backend_naming.rs, the four adapter naming paths, scripts/template-cache-local-test.sh, .github/workflows/test.yml, CHANGELOG.md, and the plan-to-browser data path (html_processor.rs, creative_opportunities.rs, AuctionPlan::browser_bidder_codes). It did not independently re-review orchestrator.rs, openrtb.rs, integrations/prebid.rs, integrations/aps.rs, or the operator documentation.
Non-blocking
thinking
- Axum backend naming collapses
-,., and space, so provider sets valid on every other target fail startup on Axum — see inline atcrates/trusted-server-core/src/platform/backend_naming.rs:424
refactor
- The CI-gating template-cache harness hand-reimplements the Fastly backend-name codec in Python — see inline at
scripts/template-cache-local-test.sh:337
note
- The production dispatch loop lost the
lgtm[rust/cleartext-logging]suppression itscfg(test)twin kept — see inline atcrates/trusted-server-core/src/auction/orchestrator.rs:1154
praise
- The breaking-change entry is unusually complete — see inline at
CHANGELOG.md:12
Cross-cutting / body-level findings
- 📝 The six CodeQL
rust/cleartext-loggingalerts are false positives, independently confirmed. Reading all six flagged statements atorchestrator.rs:752,1209,1897,1970,2021, and2344: they log provider IDs, backend names, andu32timeout and count values only.provider_name()returns the operator-chosen provider ID from the compiled plan, and no interpolated argument carries secret material. This matches the conclusion already recorded on this PR; noting it here only as a second, independent verification so the failing check can be dispositioned rather than re-litigated. - 👍 The plan-to-browser data path is clean.
AuctionPlan::browser_bidder_codes(plan.rs:495-497) yields bidder route keys and nothing else, so no provider endpoint,profile_configvalue, credential, or predicted backend name reaches injected browser configuration through it. Thehtml_processor.rschanges in this PR are confined to#[cfg(test)]registry construction, andcreative_opportunities.rsfeedsAdSlotvalues into the server-side request rather than the page. Worth stating explicitly given that the PR moves provider identity into configuration that several browser-facing surfaces now read from.
CI Status
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- CodeQL: FAIL — 6 high-severity
rust/cleartext-loggingalerts; independently verified as false positives (see above). Not in the branch-protection required set. - Analyze (rust): PASS
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- format-typescript: PASS (required)
- format-docs: PASS (required)
- cargo fmt: PASS (required)
- cargo test: PASS (required)
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- prepare integration artifacts: PASS
- vitest: PASS
# Conflicts: # crates/trusted-server-adapter-spin/src/platform.rs # crates/trusted-server-core/src/auction/endpoints.rs # crates/trusted-server-core/src/auction_config_types.rs # crates/trusted-server-core/src/config.rs # crates/trusted-server-core/src/config_payload.rs # crates/trusted-server-core/src/creative_opportunities.rs # crates/trusted-server-core/src/integrations/prebid.rs # crates/trusted-server-core/src/integrations/registry.rs # crates/trusted-server-core/src/publisher.rs # crates/trusted-server-core/src/settings.rs # docs/guide/api-reference.md # docs/guide/auction-orchestration.md # docs/guide/configuration.md # docs/guide/ec-setup-guide.md # docs/guide/error-reference.md # docs/guide/fastly.md # docs/guide/first-party-proxy.md # docs/guide/getting-started.md # docs/guide/integrations/aps.md # docs/guide/proxy-signing.md # scripts/template-cache-local-test.sh # trusted-server.example.toml
# Conflicts: # crates/trusted-server-adapter-fastly/src/app.rs # crates/trusted-server-core/src/config.rs # scripts/template-cache-local-test.sh # trusted-server.example.toml
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Second pass, reviewing the five response commits (5146645, 87fac8a, 0306166, 80c1066, 95cb674) and the merge of origin/main (8c892e5). The response work is thorough: all 30 findings from the previous review are resolved or explicitly declined with sound rationale, each substantive fix carries real regression tests (verified under wasm32-wasip1/Viceroy and vitest, 901/901), and the merge is clean, audited by reproducing the auto-merge and inspecting every manual resolution; main's #1048/#1070/#1077/#870 features and all branch features arrived intact, with incoming main tests correctly ported onto the map-shaped auction schema. What remains: the CodeQL check still fails because the new suppression comments cannot dismiss the alerts, two documentation surfaces still contradict the PR's own corrected guidance, and one intent question on the restored error boundary.
5 of the inline comments below carry a one-click GitHub
suggestion. Each was verified in a scratch worktree, in isolation and together (rustfmt, target-matched clippy, targeted Viceroy test runs for the Rust ones, prettier for the docs ones), with byte-exact drift checks.
Fix verification (previous review)
- VERIFIED: all_eligible/PBS rejected at plan compile (plan.rs:307-311, golden regenerated through explicit routing); PBS and standard parsers infer missing dimensions on single-format slots with bounded
response_admissiondiagnostics; browser refresh keeps every non-server-owned bidder; all-launch-failed auctions return 502 withexecution_failedtelemetry; harness drains select failures as transport failures; disabled/auctionemitsauction_disabledtelemetry; injected config escapes every<as<; the three commented-out tests are live again; null renderer carrier falls through to meta; APS disabled semantics pinned by doc comment and test; planned dispatch no longer snapshots inbound headers across the origin wait; migration guidance, trait description, env-override docs (auction/prebid sections), TESTING.md log excerpts, README, rollback comments, and all eight small suggestions landed and check out against the code. - DECLINED with reasonable rationale (not re-raised): Accept-Language
*forwarding,dntthree-state collapse, direct-demand count cap, envelope all-or-nothing rejection. - NOT FIXED, carried below: CodeQL alert dismissal, the Key Components module table, the
"http_status"literal in aps.rs.
Blocking
wrench
- Failed CI check: CodeQL; the added
lgtmcomments are inert (cross-cutting, details below) - configuration.md sections outside the fix still teach env overrides that fail or silently no-op (cross-cutting, details below)
- TESTING.md credits the harness with mediation it never runs - see inline at
TESTING.md:148-149(suggestion)
question
- All-failed error boundary counts only launch failures - see inline at
crates/trusted-server-core/src/auction/orchestrator.rs:1792
Non-blocking
- Partial explicit dimension rejected as ambiguous on multi-format slots - see inline at
crates/trusted-server-core/src/auction/openrtb.rs:122 - Null and float dimensions forfeit inference - see inline at
crates/trusted-server-core/src/auction/openrtb.rs:93 - Local guard for the demandless-slot routing invariant - see inline at
crates/trusted-server-core/src/auction/openrtb.rs:418(suggestion) - Bare assertions in the launch-failure test - see inline at
crates/trusted-server-core/src/auction/orchestrator.rs:2869(suggestion) - Per-bid format-set rebuild in resolve_bid_dimensions - see inline at
crates/trusted-server-core/src/auction/openrtb.rs:109 - DispatchFailed arm drops per-provider diagnostics - see inline at
crates/trusted-server-core/src/auction/orchestrator.rs:861 - Operator docs silent on disabled-block rendering semantics - see inline at
docs/guide/integrations/aps.md:61(suggestion) - Env example overrides a leaf the example TOML never declares - see inline at
docs/guide/configuration.md:1612(suggestion) - Response-consumer note filed under Request Body - see inline at
docs/guide/api-reference.md:93
Cross-cutting / body-level findings
- 🔧 Failed CI check: CodeQL, and the suppression comments cannot close it. The six
rust/cleartext-loggingalerts remain open at the new head (orchestrator.rs argument lines 768, 1222, 1941, 2016, 2069, 2394) and remain false positives: they log only configured provider/mediator/backend identifiers, bounded counts, and u32 timeouts, with the taint inherited from Settings construction. The seven// lgtm[rust/cleartext-logging]comment pairs added in this delta sit two-plus lines above the flagged expressions, and code-scanning suppression only honors a marker on the alert's own line, so they are inert; future readers will assume the alerts are handled. Fix: dismiss the six alerts as false positives in the code-scanning UI (cleanest, and keeps the comments-above-code rule), or move the markers onto the flagged argument lines; either way drop the inert comment pairs. - 🔧 configuration.md still teaches broken env overrides outside the corrected sections. The scalar-leaf-only fix stopped at the auction/prebid sections; still present:
TRUSTED_SERVER__RESPONSE_HEADERS='{...}'JSON form (line 593) with a factually wrong tip at 597 (env segments preserve hyphens;env_segmentonly uppercases, so per-key overrides likeRESPONSE_HEADERS__X-CUSTOM-HEADERare what works, and the recommended JSON-object form is what cannot), indexedHANDLERS__0__*overrides (717-724, 819, 2149),REWRITE__EXCLUDE_DOMAINSJSON/indexed/comma forms (849-856),PROXY__ALLOWED_DOMAINS(940-947), andNEXTJS__REWRITE_ATTRIBUTEScomma form (1418). Against an existing array/table leaf the JSON and comma forms makets config validatefail; indexed segments are silent no-ops. TheHANDLERS__0__PASSWORD=$(cat /run/secrets/admin_password)pattern (819, 2149) is the dangerous one: operators believe the admin password was injected out of band while the placeholder in TOML is what ships, and this PR's own changes make placeholder passwords fail startup. These blocks sit outside this PR's diff hunks, so no inline comment; but they now directly contradict the corrected statement at lines 142-146 of the same file, and the fix is the same treatment those sections received. - 📝 Key Components table still stale (previous finding, unaddressed):
docs/guide/auction-orchestration.md:203-214lists orchestrator/provider/types/formats/endpoints/config.rs but notplan.rs,profile.rs,routing.rs,openrtb.rs, ortelemetry.rs, the modules the adjacent "Configuration-first plan" section describes. - ⛏
aps.rs:923still uses the literal"http_status"where prebid.rs importsERROR_TYPE_HTTP_STATUS(previous finding, unaddressed). Same value today, drift risk tomorrow. - ⛏ Bare assertions in the new admission tests:
prebid.rs:8537-8552(rejected-bid counts) and the inference-test assertions at 8571-8573 and 8602-8610 carry no "should ..." messages. - 🌱 Legacy
auction.providersmigration guard covers only the JSON path: the targeted "config uses the removed provider-list schema" error lives infrom_json_value(settings.rs:2886-2896, with test);from_tomlstill surfaces a generic serde type error for a legacy TOML validated locally. Runtime blob loading is covered, so low severity. - 🌱 Merge-audit note: main's #870 test
deploy_validation_skips_field_validation_for_integrations_with_omitted_enabled(config.rs:611) necessarily lost its[integrations.aps]variant in the merge because the branch rejects retired APS server fields even when disabled; the adserver_mock variant still proves the property. Optionally add a companion test asserting a retired-field[integrations.aps]section is rejected with the schema-migration error, so the replaced scenario has an explicit successor. - 📝 Merge quality: the origin/main merge was audited by reproducing the auto-merge (
git merge-tree) and inspecting all seven manually-decided files; conflict resolutions are unions, main's incoming tests were ported onto the map-shaped schema, the example TOML lost zero sections in either direction, and publisher.rs carries only branch changes relative to main. No losses found.
CI Status
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- CodeQL: FAIL (same six false-positive alerts; see the cross-cutting finding)
- Analyze (rust): PASS
- format-typescript: PASS (required)
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo test: PASS (required)
- Analyze (actions): PASS
- cargo test (axum native): PASS
- cargo fmt: PASS (required)
- format-docs: PASS (required)
- Analyze (javascript-typescript): PASS
- prepare integration artifacts: PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- vitest: PASS
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Third pass, reviewing 7f5dd39 against the previous review. Every finding from the second pass is resolved or reasonably dispositioned, each verified against the code: the six CodeQL alerts are dismissed as false positives in the code-scanning UI, the launch-failure boundary is kept deliberately with an inline explanation and a mixed-outcome regression test, dimension resolution now uses a precomputed per-response index with partial-coordinate matching and null/integral-float acceptance (four new tests), the configuration.md environment-override sweep is complete (all five remaining sections corrected, both HANDLERS__0__PASSWORD patterns removed, the incorrect JSON-preserves-hyphens tip replaced with the working per-key form), the legacy provider-list migration error now surfaces on every path via a custom deserializer on auction.providers (TOML, JSON blob, and CLI tests), and the module table, api-reference ordering, APS operator docs, assertion messages, ERROR_TYPE_HTTP_STATUS, debug_assert, and timeout_ms example all landed. The two inline comments below are non-blocking nitpicks.
1 of the inline comments carries a one-click
suggestion, verified against prettier in a scratch worktree.
Non-blocking
- Merged hard breaks in the secret Do-list - see inline at
docs/guide/configuration.md:2130(suggestion) - Inert
lgtmcomment pairs remain after the UI dismissals - see inline atcrates/trusted-server-core/src/auction/orchestrator.rs:765
CI Status
Remote CI had not run for this head at review time (only the Code Quality workflow completed):
- Analyze (javascript-typescript): PASS
- cargo fmt / clippy / adapter tests / parity / vitest / format checks: not run remotely at review time
Local gates run in the review worktree at 7f5dd39, all green:
cargo fmt --all -- --check: PASScargo clippy-fastly: PASScargo check-axum: PASScargo test-fastly: PASS (2785 passed, 0 failed)./scripts/test-cli.sh: PASS- docs prettier: PASS
7f5dd39 to
c31cf1e
Compare
Resolve 146 conflict hunks across 38 files. Most were the same work in two shapes: rc had already merged the impl branches of #1016, #1103 and #1036 before those branches were force-pushed and took further review fixes, which then reached main as squashes. Notable resolutions: - Upgrade edgezero from the pre-merge feature rev 055f7e94 to the released tag v0.0.8, which contains that branch as upstream #344 plus a Fastly CLI fix. Reword the now-stale "pinned feature branch" comments. - Take main's reviewed refinements: the precomputed bid dimension index, the separate wait-delay queue in the platform test double, trusted_client_ip secret resolution, whitespace-only secret key rejection, and the removal of the lgtm annotations. - Keep rc's later work: access telemetry, managed Prebid User ID modules, and request timing instrumentation. - Keep rc's secret-reference documentation, which stays consistent with the surrounding text, and its example.com placeholders. Fix three semantic conflicts that produced no markers: implement the new EcKvStore::key_exists on rc's TimedKvStore wrapper, ungate the HashSet import now used outside tests, and drop duplicate test-scope definitions that only the target-matched clippy runs surface.
3 participants
Summary
The old design tied provider identity, routing, transport, and response handling to singleton implementations. Adding another standards-compliant endpoint required more provider-specific code, and each adapter could derive backend behavior independently. This implementation moves those decisions into validated configuration while preserving existing Prebid Server and Amazon Publisher Services behavior.
Changes
Changed files
Root configuration and guidance
Cargo.lockREADME.mdTESTING.mdtrusted-server.example.tomlAdapters and CLI
crates/trusted-server-adapter-axum/src/app.rscrates/trusted-server-adapter-axum/src/platform.rscrates/trusted-server-adapter-axum/tests/routes.rscrates/trusted-server-adapter-cloudflare/src/app.rscrates/trusted-server-adapter-cloudflare/src/platform.rscrates/trusted-server-adapter-fastly/Cargo.tomlcrates/trusted-server-adapter-fastly/src/app.rscrates/trusted-server-adapter-fastly/src/backend.rscrates/trusted-server-adapter-fastly/src/platform.rscrates/trusted-server-adapter-fastly/src/tinybird.rscrates/trusted-server-adapter-spin/src/app.rscrates/trusted-server-adapter-spin/src/platform.rscrates/trusted-server-cli/src/prebid_bundle.rscrates/trusted-server-cli/tests/config_env_overlay.rsAuction core
crates/trusted-server-core/src/auction/README.mdcrates/trusted-server-core/src/auction/endpoints.rscrates/trusted-server-core/src/auction/formats.rscrates/trusted-server-core/src/auction/mod.rscrates/trusted-server-core/src/auction/openrtb.rscrates/trusted-server-core/src/auction/openrtb/test_executor.rscrates/trusted-server-core/src/auction/openrtb/tests.rscrates/trusted-server-core/src/auction/orchestrator.rscrates/trusted-server-core/src/auction/plan.rscrates/trusted-server-core/src/auction/profile.rscrates/trusted-server-core/src/auction/provider.rscrates/trusted-server-core/src/auction/routing.rscrates/trusted-server-core/src/auction/telemetry.rscrates/trusted-server-core/src/auction/test_support.rscrates/trusted-server-core/src/auction/types.rsConfiguration, platform, and request handling
crates/trusted-server-core/src/auction_config_types.rscrates/trusted-server-core/src/config.rscrates/trusted-server-core/src/config_payload.rscrates/trusted-server-core/src/creative_opportunities.rscrates/trusted-server-core/src/html_processor.rscrates/trusted-server-core/src/platform/backend_naming.rscrates/trusted-server-core/src/platform/http.rscrates/trusted-server-core/src/platform/mod.rscrates/trusted-server-core/src/platform/test_support.rscrates/trusted-server-core/src/platform/traits.rscrates/trusted-server-core/src/publisher.rscrates/trusted-server-core/src/settings.rscrates/trusted-server-core/src/test_support.rsIntegrations
crates/trusted-server-core/src/integrations/adserver_mock.rscrates/trusted-server-core/src/integrations/aps.rscrates/trusted-server-core/src/integrations/didomi.rscrates/trusted-server-core/src/integrations/google_tag_manager.rscrates/trusted-server-core/src/integrations/gpt_diagnostics.rscrates/trusted-server-core/src/integrations/mod.rscrates/trusted-server-core/src/integrations/nextjs/mod.rscrates/trusted-server-core/src/integrations/prebid.rscrates/trusted-server-core/src/integrations/registry.rscrates/trusted-server-core/src/integrations/sourcepoint.rsBrowser and integration tests
crates/trusted-server-integration-tests/fixtures/configs/trusted-server.integration.tomlcrates/trusted-server-js/lib/src/integrations/prebid/index.tscrates/trusted-server-js/lib/test/integrations/prebid/index.test.tscrates/trusted-server-js/lib/test/prebid-artifact-integration.test.mjsOperator and architecture documentation
docs/guide/api-reference.mddocs/guide/architecture.mddocs/guide/auction-orchestration.mddocs/guide/configuration.mddocs/guide/ec-setup-guide.mddocs/guide/error-reference.mddocs/guide/fastly.mddocs/guide/first-party-proxy.mddocs/guide/getting-started.mddocs/guide/integration-guide.mddocs/guide/integrations-overview.mddocs/guide/integrations/aps.mddocs/guide/integrations/prebid.mddocs/guide/proxy-signing.mddocs/superpowers/plans/2026-08-11-config-first-auction-provider-architecture-implementation-plan.mddocs/superpowers/specs/2026-08-10-config-first-auction-provider-architecture-design.mdScope
This is a large change because provider configuration is now the single source of truth for startup validation, adapter backend registration, runtime dispatch, browser bidder exposure, telemetry, and operator documentation. Shipping only part of that path would leave the old and new models active at the same time and allow them to disagree. The pull request keeps the work focused on auction-provider configuration and execution; it does not replace the existing static mock mediator or add runtime-loadable provider plugins.
Target-aware validation before
ts config pushremote I/O remains blocked on publishing and pinning the required EdgeZero callback dependency. The shared target-independent compiler and adapter startup validation are included here.Closes
Closes #1026
Test plan
Full verification still needs to run on the rebased implementation head. The current remote head has completed only the JavaScript and TypeScript CodeQL check.
cargo test-fastly && cargo test-axum && cargo test-cloudflare && cargo test-spin./scripts/test-cli.shcargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test paritycargo fmt --all -- --checkcargo clippy-fastly && cargo clippy-axum && cargo clippy-cloudflare && cargo clippy-cloudflare-wasm && cargo clippy-spin-native && cargo clippy-spin-wasmcd crates/trusted-server-js/lib && npx vitest runcd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatChecklist
unwrap()in production code; useexpect("should ...")logmacros, notprintln!