Conversation
Rename crates/common → crates/trusted-server-core and crates/fastly → crates/trusted-server-adapter-fastly following the EdgeZero naming convention. Add EdgeZero workspace dependencies pinned to rev 170b74b. Update all references across docs, CI workflows, scripts, agent files, and configuration.
Introduces trusted-server-core::platform with PlatformConfigStore, PlatformSecretStore, PlatformKvStore, PlatformBackend, PlatformHttpClient, and PlatformGeo traits alongside ClientInfo, PlatformError, and RuntimeServices. Wires the Fastly adapter implementations and threads RuntimeServices into route_request. Moves GeoInfo to platform/types as platform-neutral data and adds geo_from_fastly for field mapping.
…o-pr2-platform-traits
- Defer KV store opening: replace early error return with a local UnavailableKvStore fallback so routes that do not need synthetic ID access succeed when the KV store is missing or temporarily unavailable - Use ConfigStore::try_open + try_get and SecretStore::try_get throughout FastlyPlatformConfigStore and FastlyPlatformSecretStore to honour the Result contract instead of panicking on open/lookup failure - Encapsulate RuntimeServices service fields as pub(crate) with public getter methods (config_store, secret_store, backend, http_client, geo) and a pub new() constructor; adapter updated to use new() - Reference #487 in FastlyPlatformHttpClient stub (PR 6 implements it) - Remove unused KvPage re-export from platform/mod.rs - Use super::KvHandle shorthand in RuntimeServices::kv_handle()
- Split fastly_storage.rs into storage/{config_store,secret_store,api_client,mod}.rs
- Add PlatformConfigStore read path via FastlyPlatformConfigStore::get using ConfigStore::try_open/try_get
- Add PlatformError::NotImplemented variant; stub write methods on FastlyPlatformConfigStore and FastlyPlatformSecretStore
- Add StoreName/StoreId newtypes with From<String>, From<&str>, AsRef<str>
- Add UnavailableKvStore to core platform module
- Add RuntimeServicesBuilder replacing 7-arg constructor
- Migrate get_active_jwks and handle_trusted_server_discovery to use &RuntimeServices
- Update call sites in signing.rs, rotation.rs, main.rs
- Add success-path test for handle_trusted_server_discovery using StubJwksConfigStore
- Fix test_parse_cookies_to_jar_empty typo (was emtpy)
- Make StoreName and StoreId inner fields private; From/AsRef provide all needed construction and access - Add #[deprecated] to GeoInfo::from_request with #[allow(deprecated)] at the three legacy call sites to track migration progress - Enumerate the six platform traits in the platform module doc comment - Extract backend_config_from_spec helper to remove duplicate BackendConfig construction in predict_name and ensure - Replace .into_iter().collect() with .to_vec() on secret plaintext bytes - Remove unused bytes dependency from trusted-server-adapter-fastly - Add comment on SecretStore::open clarifying it already returns Result (unlike ConfigStore::open which panics)
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Prior blocking findings (duplicate-header drops in to_fastly_request and copy_custom_headers) are both fixed in 2817761, with round-trip regression tests added. CI is green. Approving; non-blocking observations below.
Non-blocking
🤔 thinking
from_fastly_request_refsilently drops body (crates/trusted-server-core/src/compat.rs:54): docs note the body is empty, but the name doesn't signal it. Footgun if a future caller passes a POST expecting body access. Consider renaming tofrom_fastly_headers_refor adding adebug_assert!when the source request has a non-empty body. All current callers only read headers, so not a bug today.- Double conversion on the auction hot path:
auction/endpoints.rs:50buildshttp_reqonce, butauction/formats.rs:93(convert_tsjs_to_auction_request) re-converts the same underlyingfastly::Requestagain viafrom_fastly_request_ref. Each conversion iterates all headers — two copies per auction request. Acceptable for a temporary bridge (PR 15 removes it), but worth a comment referencing PR 15 so the duplication doesn't get frozen. forward_cookie_headerusesinsert, notappend(crates/trusted-server-core/src/cookies.rs:167,175,184): fine for HTTP/1 whereCookieis a single semicolon-joined value, but HTTP/2 (RFC 7540 §8.1.2.5) allows splitting across multiple headers and only the first would be forwarded. Matches pre-migration behavior and Fastly concatenates HTTP/2 cookie headers upstream, so likely not a live bug — worth documenting the assumption.
🌱 seedling
- Migration guard scope is "utility modules only" (
crates/trusted-server-core/src/migration_guards.rs:27-37): guards 6 files. Handler/integration files (publisher.rs,proxy.rs,registry.rs,integrations/*.rs,auction/*.rs) still usefastly::Requestby design. PR 12–14 should extend the banned-patterns list as files are migrated.
⛏ nitpick
- Dead case check in
copy_fastly_custom_headers(crates/trusted-server-core/src/compat.rs:144): checks both"x-"and"X-"thoughfastly::Requestnormalizes to lowercase. Not worth a commit in temporary code — trim on the next touch.
CI Status
- integration tests: PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
- Revert proxy.rs merge artifact: restore per-request allowed_domains at both redirect_is_permitted call sites; remove dead_code allow and stale comment — integration proxies defaulting to &[] get open mode again as documented - Drop unused trusted-server-js dep from adapter Cargo.toml - Fix check_response: gate body read behind error branch so 2xx paths do not buffer and discard the response body - Remove self-referential SECRET_UPSERT_METHOD test - Reorder write-cost doc so outbound HTTPS round-trip leads; handle-open caching noted as negligible - Refactor make_request to take fastly::http::Method; drop string match and unreachable arm; remove SECRET_UPSERT_METHOD const - Add SigningStoreIds named struct in endpoints.rs; update both call sites to destructure by name
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
PR introduces compat.rs as a temporary Fastly↔http bridge and migrates six utility modules (auth, cookies, synthetic, http_util, consent/extraction, consent/mod) off fastly::Request/fastly::Response. Prior approvals (2026-04-14) predate the latest commit ae402ff (2026-04-15), which renamed from_fastly_request_ref → from_fastly_headers_ref, dropped a redundant case check in copy_fastly_custom_headers, and switched forward_cookie_header to get_all + append. The round-2 fixes for duplicate-header preservation are clean, but a few concerns remain — notably a new runtime-panic surface at the edge and unused compat helpers that arrived before their callers.
Blocking
🔧 wrench
- New edge-wide panic surface on URL parsing:
compat::build_http_requestcalls.parse().expect(...)on every bridged request.http::Uriis stricter than Fastly's internalurl::Url, so a URL Fastly accepts buthttp::Urirejects panics the entire edge handler before auth can run. Previouslyenforce_basic_authusedreq.get_path()with no re-parse. (crates/trusted-server-core/src/compat.rs:14-31)
Non-blocking
🤔 thinking
- Redundant compat conversion on the prebid hot path:
request_bids(prebid.rs:1012) andto_openrtb(prebid.rs:713) both convert the samecontext.requestin the same auction flow — pull up once and thread through.
♻️ refactor
- Three compat functions ship without callers:
from_fastly_request,to_fastly_request, andfrom_fastly_responseare only referenced from their own tests.CLAUDE.mdsays "Don't design for hypothetical future requirements. No half-finished implementations either." Ship in the PR that uses them. (crates/trusted-server-core/src/compat.rs:40, 61, 90)
🌱 seedling / 📌 out of scope / ⛏ nitpick
- 📌 Redundant conversion at the auction boundary: acknowledged by TODO at
auction/formats.rs:93-95; accepted cost of incremental migration. - 🌱
sanitize_fastly_forwarded_headersget-then-remove:remove_headeris idempotent; theget_headerguard exists only for the debug log. (compat.rs:129-136) - ⛏
forward_cookie_headerpanics onHeaderValue::from_str: fires only on already-validated input, but atry_from+ skip keeps failure local to the function. (cookies.rs:156-186)
CI Status
- fmt: PASS
- clippy: PASS
- rust tests: PASS (841/841)
- browser integration / integration / artifacts (GitHub Actions): PASS
…into feature/edgezero-pr10-abstract-logging-initialization
…into feature/edgezero-pr11-utility-layer-migration-v2 Resolve conflicts by adopting PR10's ec_id naming throughout. cookies.rs set_ec_cookie/expire_ec_cookie retain Response<EdgeBody> types to satisfy migration_guards; Fastly-typed callers route through compat bridge. Remove synthetic.rs (deleted in PR10) and its migration_guards entry.
cookies.rs set_ec_cookie/expire_ec_cookie now take Response<EdgeBody> to satisfy the migration_guards invariant. registry.rs and publisher.rs call the Fastly-typed equivalents in compat instead. Remove synthetic.rs entry from migration_guards (file deleted in PR10).
…into feature/edgezero-pr11-utility-layer-migration-v2
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
PR 11 of the EdgeZero migration: lifts the utility layer (auth, cookies, http_util, consent) off fastly::{Request, Response} onto http::{Request, Response}<EdgeBody>, with a compat bridge module that lets non-migrated callers keep working. The shape of the migration is sound and the new migration_guards test is a strong regression backstop. The principal concerns are CI (clippy fails when run against main) and a migrated-but-untested code path that will silently change behavior in the next PR.
Blocking
🔧 wrench
- Clippy fails:
cookies::set_ec_cookiemissing# Panics(crates/trusted-server-core/src/cookies.rs:258) - Clippy fails:
cookies::expire_ec_cookiemissing# Panics(crates/trusted-server-core/src/cookies.rs:277) - Migrated
cookies::forward_cookie_headerhas no callers and no tests — semantics diverge fromcompat::forward_fastly_cookie_header(get_allvs first-only) (crates/trusted-server-core/src/cookies.rs:152)
CI didn't catch the clippy regressions because format.yml only triggers on PRs whose base is main. PR 623's base is the PR 10 feature branch, so the clippy + fmt gate was bypassed entirely.
Non-blocking
🤔 thinking
compatispub mod compat— exposed as public API despite being PR 15 removal scaffolding (crates/trusted-server-core/src/lib.rs:38)from_fastly_headers_ref# Panicsdoc is inaccurate — claims URL parse panics but usesunwrap_or_elsefallback (crates/trusted-server-core/src/compat.rs:46)to_fastly_responsesilently truncatesEdgeBody::Stream— latent risk as more callers migrate (crates/trusted-server-core/src/compat.rs:65)
♻️ refactor
compat::expire_fastly_synthetic_cookiehardcodes cookie attributes instead of reusingcookies::ec_cookie_attributes— DRY drift on security-sensitive code (crates/trusted-server-core/src/compat.rs:145)compatuses old "synthetic" naming instead of "ec" — inconsistent with the recent EC rename (crates/trusted-server-core/src/compat.rs:118)
⛏ nitpick
from_fastly_headers_refURI fallback to"/"swallows malformed URLs silently — at minimum log a warning (crates/trusted-server-core/src/compat.rs:17)
👍 praise
migration_guards.rs— concise regression test with well-justified limitationsPrebidAuctionProvider::to_openrtbrequest_info refactor — single source of truth, real readability win
CI Status
- fmt: PASS (
cargo fmt --all -- --check) - clippy: FAIL — 2 errors (
missing_panics_doconset_ec_cookie/expire_ec_cookie), bypassed by base branch routing - rust tests: PASS (
cargo test --workspace) - integration tests (GitHub Actions): PASS (3/3)
prk-Jr
changed the base branch from
feature/edgezero-pr10-abstract-logging-initialization
to
main
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
PR 11 of the EdgeZero migration: lifts six utility modules (auth, cookies, http_util, consent/extraction, consent/mod, plus the JSON content-type in request_signing/endpoints) off fastly::{Request, Response} onto http::{Request, Response}<EdgeBody> with a compat bridge for non-migrated callers. CI is green across all 13 jobs and clippy is clean against main locally. All blocking findings from the prior CHANGES_REQUESTED reviews are resolved — set_ec_cookie/expire_ec_cookie panics docs added, forward_cookie_header now has dedicated tests for the multi-Cookie and non-UTF-8 paths, and the *_synthetic_cookie → *_ec_cookie rename keeps the EC terminology consistent. Filing non-blocking observations only; a clean approve will follow.
Non-blocking
🤔 thinking
forward_cookie_headersource/target divergence: source-sideget_allvsget_headeris tested, but target-sideappendvssetis unflagged and would change behavior for any future caller that seedstowith a pre-existingCookieheader (crates/trusted-server-core/src/cookies.rs:169).- TSJS path allocates the response body twice through the
serve_static_with_etag→compat::to_fastly_responseround-trip; disappears in PR 13/14 (crates/trusted-server-core/src/publisher.rs:140).
🌱 seedling
- Double
http::Requestbuild per request inhandle_publisher_requestandhandle_auction— both buildhttp_reqexplicitly, then pass&req(fastly) toget_or_generate_ec_id, which rebuilds it insideget_ec_id(crates/trusted-server-core/src/edge_cookie.rs:120). compat::set_fastly_ec_cookieduplicates the validate-then-build control flow ofcookies::set_ec_cookie. Acookies::try_build_ec_cookie_valuehelper would dedupe both append-paths (crates/trusted-server-core/src/compat.rs:156).
📝 note
- PR description "Changes" table is out of sync with the diff: lists
auction/formats.rs,integrations/testlight.rs,proxy.rs,synthetic.rswhich aren't ingit diff main...HEAD(folded into PR 9/10 merges). Update before merge for accuracy.
👍 praise
- All 9 prior findings addressed correctly — panics docs, EC rename, attribute reuse,
#[doc(hidden)],debug_assert!on stream truncation, URI parse warning, plus comprehensive tests for the divergent code paths. migration_guardsban list extension tofastly::mime::APPLICATION_JSONkeeps the regression net tight (crates/trusted-server-core/src/migration_guards.rs:42).PrebidAuctionProvider::to_openrtbrequest_info refactor — liftsRequestInfo::from_requesttoprepare_request, eliminating a duplicate construction (crates/trusted-server-core/src/integrations/prebid.rs:1007).
CI Status
- fmt: PASS
- clippy: PASS (verified locally with
--workspace --all-targets --all-features -- -D warnings) - rust tests: PASS
- vitest: PASS
- browser integration tests: PASS
- format-typescript / format-docs: PASS
| let stripped = strip_cookies(s, CONSENT_COOKIE_NAMES); | ||
| if !stripped.is_empty() { | ||
| if let Ok(value) = http::HeaderValue::from_str(&stripped) { | ||
| to.headers_mut().append(header::COOKIE, value); |
There was a problem hiding this comment.
🤔 thinking — cookies::forward_cookie_header and compat::forward_fastly_cookie_header diverge on two axes, only one of which is tested.
The prior review flagged the source-side divergence (compat reads first Cookie header via get_header; this reads get_all) and the new test test_forward_cookie_header_multiple_cookie_headers_appended locks that in — good.
The target-side divergence is unflagged: this function uses headers_mut().append(header::COOKIE, …) (preserves any pre-existing Cookie on to); the compat shim uses to.set_header(header::COOKIE, …) (replaces). For a to that already carries a Cookie header, the two functions produce different output. No current caller does that, but PR 12+ will swap callers from compat to this; if a future caller seeds to from a request template that already has cookies, behavior changes silently.
Suggested fix: either
- Add a defensive test like
forward_cookie_header_appends_to_existing_target_cookiesthat documents the chosen contract, or - Add a one-line doc note on the function: "existing
Cookieheaders ontoare preserved; source values are appended after them."
Non-blocking — flagging now so PR 12/13 callers know to look.
| let body = trusted_server_js::concatenate_modules(&module_ids); | ||
| let mut resp = serve_static_with_etag(&body, req, "application/javascript; charset=utf-8"); | ||
| let http_resp = | ||
| serve_static_with_etag(&body, &http_req, "application/javascript; charset=utf-8"); |
There was a problem hiding this comment.
🤔 thinking — TSJS path now allocates the response body twice.
serve_static_with_etag returns Response<EdgeBody::Once(Bytes)>, then compat::to_fastly_response calls bytes.to_vec() (compat.rs:70) producing a fresh allocation. For the unified TSJS bundle this is a real per-request copy that didn't exist before the migration.
Not blocking — disappears when publisher migrates fully in PR 13/14 — but worth noting since /static/tsjs=* is one of the highest-RPS endpoints in the service.
| } | ||
|
|
||
| match handle_request_cookies(req)? { | ||
| let http_req = compat::from_fastly_headers_ref(req); |
There was a problem hiding this comment.
🌱 seedling — Double http::Request build per request.
Callers in publisher.rs:460 and auction/endpoints.rs:50 already build http_req = compat::from_fastly_headers_ref(&req) before invoking get_or_generate_ec_id, which then transitively calls into here and builds a second copy. Each rebuild clones every header.
Resolves naturally when get_ec_id migrates to take &Request<EdgeBody> in a later PR. Worth tracking as a follow-up so the duplication doesn't linger silently after compat is gone.
| header::SET_COOKIE, | ||
| crate::cookies::create_ec_cookie(settings, ec_id), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🌱 seedling — compat::set_fastly_ec_cookie and cookies::set_ec_cookie share validation (ec_cookie_value_is_safe) and assembly (create_ec_cookie), but the if !is_safe { warn!; return; } control flow is duplicated between them.
A cookies::try_build_ec_cookie_value(settings, ec_id) -> Option<String> helper would let both append-paths collapse to one if let Some(v) = … { append(SET_COOKIE, v) }. Acceptable scaffolding cost for now; flagging for the same PR 15 cleanup window.
| "fastly::Response", | ||
| "fastly::http::Method", | ||
| "fastly::http::StatusCode", | ||
| "fastly::mime::APPLICATION_JSON", |
There was a problem hiding this comment.
👍 praise — Extending the ban list to fastly::mime::APPLICATION_JSON is a nice touch. The migration_guards file now backstops both the request/response migration and the JSON content-type migration in request_signing/endpoints.rs with a single test, keeping the regression net tight without scattering assertions across modules.
| assert_eq!(all_cookies.len(), 2, "should append all Cookie headers"); | ||
| assert!(all_cookies.iter().any(|v| v.contains("session=abc123"))); | ||
| assert!(all_cookies.iter().any(|v| v.contains("theme=dark"))); | ||
| } |
There was a problem hiding this comment.
👍 praise — Test coverage on forward_cookie_header is now thorough: strip-on, strip-empty-result, no-strip, non-UTF-8 forwarding, and multi-Cookie append. Directly closes the prior reviewer's "behavior diverges silently" concern.
| log::info!("Prebid: requesting bids for {} slots", request.slots.len()); | ||
|
|
||
| let http_req = compat::from_fastly_headers_ref(context.request); | ||
| let request_info = RequestInfo::from_request(&http_req, context.client_info); |
There was a problem hiding this comment.
👍 praise — Lifting RequestInfo::from_request to prepare_request and threading it through to_openrtb eliminates the duplicate construction that previously sat inside to_openrtb. Net correctness/readability win, not just a mechanical type swap.
3 participants
Summary
fastly::Request/fastly::Responseusage so core helpers can operate onhttp::{Request, Response}andedgezero_core::Body.compatbridge at Fastly boundaries so handlers and integrations can keep working while later migration PRs move the remaining call stack.Changes
Cargo.tomlmimedependency used by migrated HTTP response helpers.Cargo.lockmimedependency.crates/trusted-server-adapter-fastly/src/main.rscrates/trusted-server-core/Cargo.tomlmimeworkspace dependency.crates/trusted-server-core/src/auction/endpoints.rscrates/trusted-server-core/src/auction/formats.rscrates/trusted-server-core/src/auth.rshttp::Request/Responseand update tests to HTTP builders.crates/trusted-server-core/src/compat.rscrates/trusted-server-core/src/consent/extraction.rshttp::Request<EdgeBody>.crates/trusted-server-core/src/consent/mod.rscrates/trusted-server-core/src/cookies.rscrates/trusted-server-core/src/http_util.rsClientInfo.crates/trusted-server-core/src/integrations/lockr.rscrates/trusted-server-core/src/integrations/permutive.rscrates/trusted-server-core/src/integrations/prebid.rscrates/trusted-server-core/src/integrations/registry.rscrates/trusted-server-core/src/integrations/testlight.rscrates/trusted-server-core/src/lib.rscrates/trusted-server-core/src/migration_guards.rscrates/trusted-server-core/src/proxy.rscrates/trusted-server-core/src/publisher.rscrates/trusted-server-core/src/request_signing/endpoints.rsmime::APPLICATION_JSON.crates/trusted-server-core/src/synthetic.rshttp::Request<EdgeBody>.Closes
Closes #492
Test plan
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest runcd crates/js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servecargo test --package trusted-server-core compat -- --nocapture,cargo test --package trusted-server-core http_util -- --nocapture,cargo test --package trusted-server-core request_signing -- --nocapture, andcargo test --package trusted-server-core migration_guards -- --nocapturecd crates/js/lib && npx vitest runcurrently fails before test execution withERR_REQUIRE_ESMinhtml-encoding-sniffer->@exodus/bytes/encoding-lite.js; leaving CI to capture the current JS environment issue.Hardening note
This PR does not add any new config-derived regex or pattern compilation paths. Basic auth still surfaces invalid enabled handler regex configuration as an error rather than panicking, covered by
auth::tests::returns_error_for_invalid_handler_regex_without_panickingalongside the existing settings startup validation tests.Checklist
unwrap()in production code — useexpect("should ...")println!)