Add dual-path EdgeZero entry point with feature flag (PR 14)

[prk-Jr]

Summary

• Adds a feature-flagged dual-path entry point: requests are dispatched through the new EdgeZero TrustedServerApp or the preserved legacy_main based on edgezero_enabled in the trusted_server_config Fastly ConfigStore, with false as the safe default on any read failure
• Implements TrustedServerApp via edgezero_core::app::Hooks with all routes, plus FinalizeResponseMiddleware and AuthMiddleware — matching legacy routing semantics without the compat conversion layer
• Upgrades edgezero workspace dependencies from a pinned revision to branch=main and uses dispatch_with_config (non-deprecated) to avoid the double set_logger panic that run_app/run_app_with_logging would cause

Changes

File	Change
Cargo.toml	Pin all four edgezero deps to branch=main; bump toml to "1.1"
Cargo.lock	Updated for edgezero and toml version changes
crates/trusted-server-adapter-fastly/src/main.rs	Add is_edgezero_enabled(), feature-flag dispatch block, extract legacy_main(), use dispatch_with_config
crates/trusted-server-adapter-fastly/src/app.rs	TrustedServerApp implementing Hooks with all 14 routes; explicit GET / and POST / to cover matchit root path gap
crates/trusted-server-adapter-fastly/src/middleware.rs	New file: FinalizeResponseMiddleware and AuthMiddleware with golden header-precedence test
fastly.toml	Add trusted_server_config config store for local dev with edgezero_enabled = "true"
crates/trusted-server-core/src/auction/orchestrator.rs	rustfmt: remove blank line
crates/trusted-server-core/src/integrations/google_tag_manager.rs	rustfmt: reorder imports, reformat long use lists
crates/trusted-server-core/src/integrations/lockr.rs	rustfmt: inline trailing-brace onto condition
crates/trusted-server-core/src/integrations/permutive.rs	rustfmt: inline trailing-brace onto condition
crates/trusted-server-core/src/integrations/prebid.rs	rustfmt: reorder imports
crates/trusted-server-core/src/integrations/testlight.rs	rustfmt: reformat long use list
.claude/settings.json	Remove trailing comma in allowed-tools array

Closes

Closes #495

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
• Manual testing via fastly compute serve — EdgeZero path routes all named routes and GET / correctly; legacy path reachable by setting edgezero_enabled = "false"

Checklist

• Changes follow CLAUDE.md conventions
• No unwrap() in production code — use expect("should ...")
• Uses log macros (not println!)
• New code has tests
• No secrets or credentials committed

[aram356]

Summary

Reviewed the 13 files that PR 628 actually changes vs its base (feature/edgezero-pr13-...). The dual-path entry point is a sensible migration shape, and the explicit GET/POST "/" routes + the header-precedence middleware test are the kind of load-bearing details that prevent future outages. However, the EdgeZero path currently diverges from the legacy path in ways that are security- and reliability-relevant, and the branch = "main" pin on upstream deps makes builds non-deterministic. Blocking on those.

Blocking

🔧 wrench

• Forwarded-header sanitization missing on EdgeZero path — legacy strips Forwarded / X-Forwarded-* / Fastly-SSL before routing; EdgeZero hands the raw req to dispatch_with_config. With edgezero_enabled = "true" as the local-dev default, this is the default path. (main.rs:95)
• build_state() panics on misconfig — expect("should …") on settings / orchestrator / registry. Legacy returns a structured error response; EdgeZero now 5xx's with no detail. (app.rs:75)
• Docstring "built once at startup" is misleading — every request spins up a fresh Wasm instance, so build_state() runs per-request. Invites future false caching. (app.rs:61)
• Stale #[allow(dead_code)] on now-live middleware — five suppressions with "until Task 4 wires app.rs" comments. Task 4 is this PR. (middleware.rs:50,57,96,103,146)
• AuthMiddleware flattens Report<TrustedServerError> into EdgeError::internal(io::Error::other(...)) — loses per-variant status code and user message; generic 500 instead of the specific error. (middleware.rs:122)
• edgezero-* deps pinned to branch = "main" — non-deterministic builds; supply-chain path into prod via a moving upstream branch. Pin to a specific rev or fork tag. (Cargo.toml:59-62)

Non-blocking

🤔 thinking

• TLS metadata dropped on EdgeZero path — tls_protocol / tls_cipher hardcoded to None; legacy populates both. Low impact today (debug logging only), but a silent regression if any future signing/audit path reads them. (app.rs:123-124)

♻️ refactor

• 11 near-identical handler closures in routes() — a pair of file-local make_sync_handler / make_async_handler helpers would cut ~120 lines without harming auditability. (app.rs:175-301)
• FinalizeResponseMiddleware hardcodes FastlyPlatformGeo — take Arc<dyn PlatformGeo> instead so Middleware::handle can be unit-tested end-to-end. (middleware.rs:68)
• build_per_request_services duplicates platform::build_runtime_services — extract a shared helper that takes ClientInfo. (app.rs:111-127)

🌱 seedling

• fastly.toml flips local dev default to EdgeZero — combined with the blockers above, every fastly compute serve now exposes them. Consider defaulting to "false" until the blockers land. (fastly.toml:52)

⛏ nitpick

• AppState fields can be private (not pub(crate)). (app.rs:62-66)
• Root-route pairs clone closures four times — upstream RouterService::get_many would help. (app.rs:374-377)

📝 note

• The dispatch_with_config comment explaining the set_logger panic is excellent "why, not what" documentation. (main.rs:91-94)

👍 praise

• operator_response_headers_override_earlier_headers codifies a brittle precedence contract. (middleware.rs:217-233)
• Explicit GET "/" / POST "/" routes with the in-code explanation of matchit's wildcard gap prevent a future 404 outage. (app.rs:374-377)

CI Status

• browser integration tests: PASS
• integration tests: PASS
• prepare integration artifacts: PASS
• fmt / clippy / unit tests: not surfaced by gh pr checks — please confirm these ran.

[ChristianPavilonis]

Summary

Supplementary review — see aram356's review for the primary findings. This review covers additional items not raised there.

Non-blocking

🔧 wrench (cross-cutting, from earlier PRs in this stack)

• set_header drops multi-valued headers: edge_request_to_fastly in platform.rs:187 uses set_header instead of append_header, silently dropping duplicate headers. Pre-existing pattern (also in compat::to_fastly_request), but the EdgeZero path creates a new copy of the same bug.

🌱 seedling

• parse_edgezero_flag is case-sensitive: "TRUE" and "True" silently fall through to legacy path. Consider eq_ignore_ascii_case or logging unrecognized values.

📝 note (cross-cutting, from earlier PRs)

• Stale doc comment in platform/mod.rs:31: References fastly::Body in publisher.rs, but PR 11 already migrated to EdgeBody.

♻️ refactor (cross-cutting, from earlier PRs)

• Duplicated body_as_reader helper: Identical function in proxy.rs:24 and publisher.rs:23. Extract to shared utility.

⛏ nitpick (cross-cutting)

• Management API client re-created per write: Each put/delete in platform.rs constructs a new FastlyManagementApiClient. Fine for current usage, noted for future batch writes.

📌 out of scope

• compat.rs in core depends on fastly types: Already tracked as PR 15 removal target.

CI Status

• browser integration tests: PASS
• integration tests: PASS
• prepare integration artifacts: PASS

[ChristianPavilonis]

Summary

I found 5 issues to address before this dual-path entry-point rollout is ready. Three are functional regressions that can change request handling or response metadata, and two are documentation/configuration gaps that should be cleaned up before operators rely on the new path.

Findings folded into the review body

🤔 API/docs comments lag new proxy-rebuild and settings behavior: The latest reviewer findings also called out stale API/docs comments in crates/trusted-server-core/src/proxy.rs:989-997 and crates/trusted-server-core/src/settings.rs:476-480. Those files are not part of this PR's current diff, so I could not place them as inline comments on this review, but they should still be reconciled before the new proxy-rebuild and runtime-settings behavior is considered fully documented.

[aram356]

Summary

The previously blocking findings from the earlier review round are resolved: forwarded-header sanitization is now applied on the EdgeZero path, build_state() returns a Result with a startup_error_router fallback instead of panicking, AuthMiddleware preserves per-variant status codes via http_error, the "built once at startup" docstring is clarified, and edgezero deps are pinned to the full 40-char commit SHA 38198f9839b70aef03ab971ae5876982773fc2a1. parse_edgezero_flag is now case-insensitive with full test coverage.

Deep analysis surfaces two new blockers that share a single root cause: RouterService middleware in edgezero-core only runs on matched routes, and only GET/POST handlers are registered on this app. The combination produces two independent regressions on the EdgeZero path — non-GET/POST requests return 405 instead of being proxied to origin (legacy proxies every method), and router-level errors bypass FinalizeResponseMiddleware so responses miss X-Geo-*, X-TS-Version, X-TS-ENV, and operator-configured response_headers. Both can plausibly be fixed in one pass.

Inline comments cover the two blockers and the remaining non-blocking findings. CI locally: cargo fmt --check, cargo clippy --workspace --all-targets --all-features -- -D warnings, and cargo test --workspace all pass (821 tests).

Blocking

🔧 wrench

• Non-GET/POST methods regress to 405 — legacy route_request falls through to handle_publisher_request for every method; EdgeZero registers only GET and POST. RouterInner::dispatch returns Err(EdgeError::method_not_allowed) for HEAD / OPTIONS / PUT / DELETE / PATCH, which oneshot turns into a bare 405. See inline on crates/trusted-server-adapter-fastly/src/app.rs:422.
• TS response headers dropped on router-level errors and handler EdgeError paths — RouterService::oneshot handles Err(EdgeError) via err.into_response() outside the middleware chain, so 405/404/handler-error responses never reach FinalizeResponseMiddleware. Separately, next.run(ctx).await? in the middleware short-circuits on any inner Err. Both need changing so finalize always runs. See inline on crates/trusted-server-adapter-fastly/src/middleware.rs:69.

Non-blocking

🤔 thinking

• Geo lookup runs on auth-rejected responses — legacy skips geo on 401 and sets X-Geo-Info-Available: false; EdgeZero unconditionally attaches full geo headers to the 401. Inline on middleware.rs:71.
• Per-request double open of trusted_server_config — is_edgezero_enabled() and dispatch_with_config each open the store. Use dispatch_with_config_handle and reuse a single handle. Inline on main.rs:98.
• TLS metadata hardcoded to None on EdgeZero path — tls_protocol / tls_cipher are populated by legacy; the EdgeZero FastlyRequestContext only carries client_ip. Silent regression for any future signing/audit path. Previously flagged; still present. Inline on app.rs:126.

♻️ refactor

• Eleven near-identical handler closures — two small helpers collapse all of them. Previously flagged. Inline on app.rs:214.
• build_per_request_services duplicates platform::build_runtime_services — extract a shared helper parameterized by ClientInfo. Previously flagged. Inline on app.rs:129.
• Module docstring claims middleware is always attached — but startup_error_router skips it. Inline on app.rs:5.

🌱 seedling

• fastly.toml defaults local dev to EdgeZero — combined with the blockers above, every fastly compute serve reproduces them. Inline on fastly.toml:54.

📝 note

• No tests for AuthMiddleware::handle, FinalizeResponseMiddleware::handle, or startup_error_router — apply_finalize_headers is tested standalone, but the middleware handle methods and the degraded startup-error path have no coverage. Inline on middleware.rs:238.

CI Status

• browser integration tests: PASS
• integration tests: PASS
• prepare integration artifacts: PASS
• fmt / clippy / unit tests (local): PASS — 821 tests passing

[ChristianPavilonis]

superseded

[ChristianPavilonis]

Superseding note: I re-posted the line-specific findings as inline comments after switching to the single-comment review-comments API with the PR head commit SHA.

One additional high-priority finding is still not inline because the relevant code is outside this PR diff:

P1 — ProxyRequestConfig.allowed_domains is ignored, so integration proxies inherit the first-party allowlist unintentionally (crates/trusted-server-core/src/proxy.rs:432-458,573-579,694-700)

ProxyRequestConfig documents two modes: integrations should pass &[] for open mode, while the first-party proxy should pass &settings.proxy.allowed_domains. But proxy_request() currently destructures allowed_domains into _, and proxy_with_redirects() always checks settings.proxy.allowed_domains instead. That means enabling proxy.allowed_domains for /first-party/proxy can unexpectedly break unrelated integration proxy traffic.

Suggested fix: thread config.allowed_domains through to proxy_with_redirects() and use that slice for both the initial target check and redirect-hop checks. I’d also add a regression test where settings.proxy.allowed_domains is non-empty but proxy_request(... allowed_domains: &[]) still succeeds.

[prk-Jr]

@ChristianPavilonis Fixed the non-inline ProxyRequestConfig.allowed_domains item in eb6a9e9f. ProxyRequestConfig::allowed_domains is now threaded into ProxyRedirectPolicy and used for both the initial target check and redirect-hop checks. Added regressions for open mode with a non-empty settings.proxy.allowed_domains and open-mode redirect hops; cargo test -p trusted-server-core proxy_request_ passes.

[aram356]

Summary

Round-3 review. The previously blocking findings from round 2 are all resolved: forwarded-header sanitization runs on the EdgeZero path, build_state returns a Result with a startup_error_router fallback, AuthMiddleware preserves per-variant status codes via http_error, FinalizeResponseMiddleware absorbs handler errors via IntoResponse, the AppState docstring is corrected, workspace edgezero deps are pinned to the full 40-char commit SHA 38198f9839b70aef03ab971ae5876982773fc2a1, parse_edgezero_flag is case-insensitive with full test coverage, HEAD/OPTIONS/PUT/PATCH/DELETE catch-all routes are registered, and FinalizeResponseMiddleware now takes Arc<dyn PlatformGeo> for testability.

This round surfaces two new blockers and four non-blocking findings. Both blockers are scope/behavior issues, not implementation bugs:

• Silent behavior change in proxy.rs — integration-proxy redirect chains are no longer bounded by settings.proxy.allowed_domains, and nothing in the PR body documents it.
• Non-standard HTTP methods bypass the middleware chain — legacy route_request falls through to publisher origin for every method; this path 405/404s them without running FinalizeResponseMiddleware, dropping all TS/geo/operator headers. Contradicts the FinalizeResponseMiddleware doc contract.

Both are addressable in this PR without a large rework. Inline comments describe concrete fix options.

Local CI is green: cargo fmt --check, cargo clippy --workspace --all-targets --all-features -D warnings, and cargo test --workspace (867 tests).

Blocking

🔧 wrench

• Scope creep: silent integration-proxy allowlist semantics change — proxy.rs now lets integration proxies opt out of settings.proxy.allowed_domains for initial target and redirect hops. Not in PR description. See inline on crates/trusted-server-core/src/proxy.rs:446.
• Non-standard HTTP methods bypass middleware — TRACE/CONNECT/WebDAV verbs skip FinalizeResponseMiddleware; TS/geo/operator headers dropped. Contradicts the doc contract. See inline on crates/trusted-server-adapter-fastly/src/app.rs:149.

Non-blocking

🤔 thinking

• Per-request INFO-level routing log — noisy at production traffic; consider log::debug! or once-per-cold-start. Inline on main.rs:96.

📝 note

• No tests for FinalizeResponseMiddleware::handle / AuthMiddleware::handle — the code changed this round has no direct unit coverage. Inline on middleware.rs:130.
• No end-to-end test for the EdgeZero dispatch path — would catch the method-bypass regression directly. Inline on app.rs:392.

CI Status

• browser integration tests: PASS (GitHub)
• integration tests: PASS (GitHub)
• prepare integration artifacts: PASS (GitHub)
• fmt: PASS (local)
• clippy: PASS (local)
• rust tests: PASS (local, 867 tests)

[ChristianPavilonis]

Summary

Reviewed the remaining PR #628 findings after excluding the consent_store replacement item and the orchestrator issue per discussion. I found two high-impact EdgeZero/legacy parity concerns plus a few documentation/comment accuracy issues. GitHub checks currently shown for the PR are passing.

[ChristianPavilonis]

EdgeZero path loses TLS metadata, making scheme detection fall back to http

build_per_request_services() sets tls_protocol and tls_cipher to None. After forwarded-header sanitization, RequestInfo::from_request() can no longer use X-Forwarded-Proto / Fastly-SSL, so it defaults to http.

This can change publisher URL rewriting and first-party URL generation from https://... to http://... on the EdgeZero path.

Suggestion: Carry TLS metadata through the EdgeZero Fastly request context, or temporarily assume HTTPS in the Fastly adapter until upstream exposes TLS fields.

[prk-Jr]

Confirmed — FastlyRequestContext only exposes client_ip; there are no TLS fields available through the EdgeZero context. Populating tls_protocol/tls_cipher requires an upstream change to FastlyRequestContext. Added a doc note to build_per_request_services acknowledging the gap; tracked for a follow-up PR.

[ChristianPavilonis]

EdgeZero flag docs omit case-insensitive true

parse_edgezero_flag() accepts TRUE / True via eq_ignore_ascii_case, but the comment only lists true and 1.

Suggestion: Document accepted values as "1" or "true" in any ASCII case.

[aram356]

Summary

Re-review against main-merge intent. The dual-path entry point and middleware shape are right, and prior round-2 blockers (build_state Result, forwarded-header sanitization, parse_edgezero_flag casing) are resolved. Three new functional regressions remain that change request handling vs legacy on identical input, plus two correctness/process gaps.

PR base is feature/edgezero-pr13-…; the diff vs main is 58 files because PRs 9–13 are unmerged. Findings below scope to the 13 files in this PR's actual delta.

CI: PASS (browser/integration tests, prepare artifacts).

Blocking

🔧 wrench

• EdgeZero path silently degrades when consent_store is misconfigured (compliance regression) — AppState::kv_store is hardcoded to UnavailableKvStore and the EdgeZero path never calls runtime_services_for_consent_route. Legacy returns 503 fail-closed when open_kv_store(name) fails (see route_tests.rs::configured_missing_consent_store_only_breaks_consent_routes); EdgeZero fail-opens because try_kv_fallback swallows KvError::Unavailable via ?. Inline on app.rs:92.
• Entry-point geo lookup overrides middleware's 401 geo-skip in production — Middleware correctly skips geo on 401 and emits X-Geo-Info-Available: false; the entry-point apply_finalize_headers then runs an unconditional geo lookup and overwrites it with full geo headers when a real client IP is present. The unit test passes only because it bypasses main() and lookup(None) → None. Inline on main.rs:115.
• Non-GET/POST methods on registered named-route paths regress to 405 — Named routes register a single method; matchit prefers the exact path over /{*rest}, so HEAD /first-party/proxy, OPTIONS /auction, PUT /admin/keys/rotate, etc. return 405 (legacy proxies them to publisher origin). The existing 405 test only covers TRACE on /, which doesn't exercise this case. Inline on app.rs:376.
• Silent settings drop on entry-point if let Ok(settings) = get_settings() — On Err, response goes out without TS headers; legacy fails fast at build_state(). Inline on main.rs:110.
• PR description doesn't match the diff — undisclosed tokio-test = "0.4" workspace dep (unused, not in Cargo.lock); description says edgezero deps pinned to branch=main but actual is a 40-char SHA; proxy.rs refactor + new "Behavior change from pre-PR-14" doc paragraph (factually wrong vs origin/main) not mentioned. Inlines on Cargo.toml:86 and proxy.rs:89.

❓ question

• Where is consent KV opened on the EdgeZero path? — If finding 1 is intentional (wired in PR 15/16), please link the issue/spec. Inline on app.rs:92.

Non-blocking

🤔 thinking

• TLS metadata still hardcoded to None — tls_protocol/tls_cipher not populated from FastlyRequestContext; silent regression for any future signing/audit path. Inline on app.rs:124. Previously flagged.
• Two geo lookups per request on the happy path — middleware + entry-point. Guard the second behind a "response missing TS headers" check. Inline on main.rs:111.
• Per-request double open of trusted_server_config config store — is_edgezero_enabled + dispatch_with_config both open it; SDK caches the handle, but the duplication is avoidable. Inline on main.rs:66. Previously flagged.

♻️ refactor

• Eleven near-identical handler closures — app.rs:273-359. Helper or route! macro collapses them. Inline on app.rs:351. Previously flagged.
• build_per_request_services duplicates platform::build_runtime_services — bodies identical except for client_info source. Inline on app.rs:127. Previously flagged.
• Module docstring overstates middleware coverage — top summary contradicts the "Startup error handling" section. Inline on app.rs:9.

🌱 seedling

• fastly.toml defaults local dev to EdgeZero — every fastly compute serve reproduces the regressions above. Recommend defaulting to "false" until parity is reached. Inline on fastly.toml:48.

📝 note

• No EdgeZero-side test for the consent-store regression — mirror configured_missing_consent_store_only_breaks_consent_routes driving TrustedServerApp::routes() after fix.
• No EdgeZero-side test for HEAD/OPTIONS on a named-route path — add dispatch_head_on_named_get_route_proxies_to_publisher.
• legacy_main cleanup TODO references issue #495 — confirm the issue covers compat.rs, route_request, legacy_main, the app::http_error duplication, and the entry-point finalize wrap.

CI Status

• browser integration tests: PASS
• integration tests: PASS
• prepare integration artifacts: PASS

[aram356]

🔧 wrench — EdgeZero path silently degrades when consent_store is misconfigured (compliance regression).

AppState::kv_store is hardcoded to UnavailableKvStore for the lifetime of the Wasm instance. Legacy route_request calls runtime_services_for_consent_route for /auction and the publisher fallback (main.rs:307-323); when settings.consent.consent_store is set but open_kv_store(name) fails, it returns Report<TrustedServerError::KvStore> and the route responds 503 fail-closed (test in route_tests.rs:222-260).

On this path, every consent KV read goes through UnavailableKvStore, which returns KvError::Unavailable from every method (platform/kv.rs:18-46). try_kv_fallback swallows the error via ? (consent/mod.rs:528-540), so the auction proceeds with no stored consent — fail-open.

Net effect: an operator typo on consent_store is fail-closed on legacy, fail-open on EdgeZero.

Fix: port runtime_services_for_consent_route into the EdgeZero path. Either resolve the consent KV store eagerly when building per-request services for consent-touching routes, or wrap auction_handler and fallback_handler with a helper that swaps services.kv_store() to the configured consent store and short-circuits with a 503 on open failure. Add a test mirroring configured_missing_consent_store_only_breaks_consent_routes driving TrustedServerApp::routes().

[aram356]

🔧 wrench — Entry-point geo lookup overrides the middleware's 401 geo-skip in production.

FinalizeResponseMiddleware::handle correctly skips geo for UNAUTHORIZED and inserts X-Geo-Info-Available: false (middleware.rs:78-87); the dispatch_auth_rejected_401_carries_finalize_headers test verifies it in isolation.

But the entry-point apply_finalize_headers here runs an unconditional FastlyPlatformGeo.lookup(client_ip). With a real client IP in production, geo_info is Some(...), and apply_finalize_headers overwrites X-Geo-Info-Available: false with full geo headers (X-Geo-Country, X-Geo-City, …). legacy_main checks the status before geo (main.rs:162-172); the EdgeZero entry-point doesn't.

The unit test passes only because it bypasses main() and lookup(None) returns None.

Fix:

let geo_info = if response.status() == StatusCode::UNAUTHORIZED {
    None
} else {
    FastlyPlatformGeo.lookup(client_ip).unwrap_or_else(|e| {
        log::warn!("entry-point geo lookup failed: {e}");
        None
    })
};

[aram356]

🔧 wrench — Non-GET/POST methods on registered named-route paths regress to 405.

Named routes register a single method (.get("/first-party/proxy", …), .post("/auction", …), etc.). The /{*rest} catch-all on the next block registers all 7 publisher_fallback methods, but matchit prefers exact path matches over wildcards — confirmed by your own dispatch_unregistered_method_returns_405_at_router_level test, which produces 405 even with the catch-all registered.

Therefore HEAD /first-party/proxy, OPTIONS /auction, PUT /admin/keys/rotate, etc. return 405 in EdgeZero. Legacy route_request falls through to _ => handle_publisher_request for every non-matching (method, path) pair (main.rs:256-275), proxying to the publisher origin.

This breaks cache-validation HEADs, CORS preflight OPTIONS, and any non-GET/POST traffic on a registered path. The existing 405 test only covers TRACE on /, which doesn't exercise this case (TRACE isn't in the catch-all method list either).

Fix options:

• (a) Register all 7 publisher_fallback methods on each named-route path, with non-matching methods routed to fallback_handler.
• (b) Drop the named routes and dispatch by method+path inside fallback_handler (closest to legacy semantics).
• (c) Use the catch-all only and prefix-check inside the handler (brittle).

Plus a regression test asserting HEAD /first-party/proxy returns a publisher-origin response rather than 405.

[aram356]

🔧 wrench — if let Ok(settings) = get_settings() silently drops TS headers on settings-load failure.

If get_settings() returns Err here, the response goes out without TS headers. legacy_main fails fast at build_state() (main.rs:138-143) and returns an error response. In practice settings load at Wasm startup so re-parsing is unlikely to fail mid-request, but the silent skip is fragile and inconsistent with legacy fail-fast semantics.

Fix: at minimum log a warning on the error branch. Better: cache the Arc<Settings> parsed during build_state() (the TrustedServerApp already has it) and reuse it here — threading it through dispatch_with_config request extensions avoids both the silent failure mode and the redundant parse.

[aram356]

🔧 wrench — Undisclosed tokio-test = "0.4" workspace dep is unused.

Not referenced anywhere in the workspace and isn't even resolved into Cargo.lock. Either remove or justify in the PR description.

While here: the PR description claims edgezero deps are pinned to branch=main, but the diff actually pins to a 40-char SHA 38198f9839b70aef03ab971ae5876982773fc2a1 (good — deterministic builds). Update the description to match the diff.

[aram356]

🌱 seedling — Local dev defaults to EdgeZero.

With the regressions in findings 1–3, every fastly compute serve reproduces them by default. Recommend edgezero_enabled = "false" in fastly.toml until the path reaches functional parity, then flip the default in a follow-up PR. Previously flagged.

[aram356]

👍 praise — Defensive default for the edgezero_enabled flag read.

unwrap_or_else(|e| { log::warn!(...); false }) is exactly the right safe-fallback shape: store-unavailable or key-missing routes through the legacy path with observable logging, so the new path can never be silently engaged on a flag-read error.

[aram356]

👍 praise — Case-insensitive + trim-tolerant flag parsing with full test coverage (tests mod at line 351-380). Small detail that prevents annoying operator errors.

[aram356]

👍 praise — apply_finalize_headers precedence test.

operator_response_headers_override_earlier_headers documents the precedence contract in code (operator headers can intentionally override managed X-Geo-* / X-TS-* headers). Solid invariant captured as an assertion, not just a comment.

[aram356]

👍 praise — End-to-end dispatch test that exercises the real router.

dispatch_auth_rejected_401_carries_finalize_headers is exactly the right shape: build the actual RouterService via TrustedServerApp::routes() and assert middleware behavior on a real request. Reusable pattern for the missing tests on the consent-store and HEAD-on-named-path regressions.

[aram356]

Summary

Round-5 re-review against feature/edgezero-pr13-… (PR's actual base). The round-4 blockers are resolved: EdgeZero now fails-closed on misconfigured consent_store (auction + fallback handlers call runtime_services_for_consent_route), the entry-point geo lookup skips on 401, every named path registers publisher_fallback_methods() so HEAD/OPTIONS/PUT/PATCH/DELETE no longer regress to 405, fastly.toml defaults edgezero_enabled = "false", and the previously undisclosed tokio-test dep is gone.

The feature is gated off in production (any flag-read failure falls back to legacy) and reversible by flipping one config-store value, so blast radius is bounded. Approving in spirit — but blocking on three specific items because the alternative is merging a PR with a stale description, no direct test for a security-relevant fix, and a regression-prone footgun in the route table.

Local CI: cargo fmt --check PASS, cargo clippy --workspace --all-targets --all-features -- -D warnings PASS, cargo test --workspace PASS (967 tests). GitHub CI on c17fa27e: browser/integration tests + prepare artifacts all PASS.

Blocking

🔧 wrench

• PR description does not match the diff. Says Pin all four edgezero deps to branch=main — Cargo.toml actually pins to rev = "38198f9839b70aef03ab971ae5876982773fc2a1" (Cargo.lock confirms). Missing from the description: the proxy.rs ProxyRequestConfig.allowed_domains semantics change, the /health short-circuit in main.rs, the match get_settings() finalize block in main.rs, and the named_paths_primary_methods bookkeeping in app.rs. Reviewers shouldn't have to reverse-engineer the diff. Round-3 and round-4 both flagged the description being inaccurate.
• Missing EdgeZero-side test for the consent-store regression. Inline on route_tests.rs:184 — see comment. The only existing test exercises the legacy route_request path; the actual fix lives in app.rs.
• named_paths_primary_methods has no compile-time guard. Inline on app.rs:444 — see comment. Adding a new named route requires editing two places to stay consistent; mismatch silently regresses HEAD/OPTIONS to 405. Either fix here or file a tracked follow-up issue with explicit ownership.

Non-blocking

🤔 thinking

• Two geo lookups per request on the happy path (main.rs:116)
• get_settings() runs 2× per EdgeZero request and is uncached (main.rs:110) — this also drives the INSECURE: … warning spam below
• INSECURE: … warning spam on EdgeZero path. Direct consequence of the previous item: each get_settings() call emits placeholder-secret warnings, so misconfigured deployments log them 2–3× per request. Should be emitted once at startup (cache Settings in a OnceLock).
• Per-request double open of trusted_server_config (main.rs:66)
• TLS metadata still hardcoded to None on EdgeZero path (app.rs:158)
• assert_ne!(status, METHOD_NOT_ALLOWED) is a brittle regression guard (app.rs:590)

♻️ refactor

• 11 near-identical handler closures in routes() (app.rs:308)
• build_per_request_services duplicates platform::build_runtime_services (app.rs:146)
• fallback_handler.clone() runs ~59 times per routes() call (app.rs:449)

📝 note

• /health shortcut has no test (main.rs:78)

🌱 seedling

• AppState.kv_store is dead state on most routes (app.rs:83)
• Entry-point apply_finalize_headers could be conditional — paired with the "two geo lookups" finding; sentinel-header approach cleans both up post-#495.

👍 praise

• finalize_handle_skips_geo_lookup_for_401 (middleware.rs:331) — PanicGeo is the right pattern for skip-condition tests.
• finalize_handle_absorbs_handler_error_and_injects_headers (middleware.rs:308) — locks down a real round-2 regression.
• dispatch_unregistered_method_returns_405_at_router_level (app.rs:598) — known limitation documented in test code, not a rotting comment.

CI Status

• browser integration tests: PASS (GitHub)
• integration tests: PASS (GitHub)
• prepare integration artifacts: PASS (GitHub)
• fmt: PASS (local)
• clippy -D warnings: PASS (local)
• rust tests: PASS (local, 967 tests)

[aram356]

🔧 wrench — Brittle method/path coupling has no compile-time guard.

Adding a new named route requires editing two places: the RouterService::builder() chain at lines 411–420, and the named_paths_primary_methods array at 427–437. There is no compile-time guarantee they stay in sync. If a contributor adds .put("/v2/foo", h) and forgets to extend the array, HEAD/OPTIONS/etc. on /v2/foo regress to 405 — exactly the bug this PR just fixed.

Suggested fix — build a single table and iterate it twice:

let named_routes: &[(&str, &[Method], _)] = &[
    ("/.well-known/trusted-server.json", &[Method::GET], discovery_handler),
    // …
];
for (path, primary, handler) in named_routes {
    for m in primary { router = router.route(path, m.clone(), handler.clone()); }
    for m in publisher_fallback_methods() {
        if !primary.contains(&m) { router = router.route(path, m, fallback_handler.clone()); }
    }
}

If you'd rather not refactor in this PR, file a follow-up issue with explicit ownership and link it from a comment here — leaving it implicit invites the exact regression that prior review rounds caught.

[aram356]

🔧 wrench — Missing EdgeZero-side test for the consent-store regression.

This call site is the round-4 fix: when consent_store is configured but unopenable, runtime_services_for_consent_route returns Err, and the auction handler propagates a 503 (matching legacy fail-closed). The fallback handler at line 398 does the same.

But the only existing test (configured_missing_consent_store_only_breaks_consent_routes in route_tests.rs) exercises the legacy route_request path only. There is no direct test coverage for the EdgeZero fix on this line.

Without it, a future change to the auction or fallback handler can silently fail-open on a misconfigured consent store and the test suite will say it's fine.

Mirror the legacy test driving TrustedServerApp::routes() — you already have the harness for it in this file's tests module:

#[test]
fn dispatch_auction_with_missing_consent_store_returns_503() {
    // build settings with consent_store = "missing-consent-store"
    // routes() → oneshot POST /auction → expect 503
}

[aram356]

🤔 thinking — Two geo lookups per request on the happy path.

FinalizeResponseMiddleware::handle already calls geo.lookup (skipping on 401) and writes geo headers. Then main.rs calls FastlyPlatformGeo.lookup again and apply_finalize_headers rewrites the same headers. For requests routed through registered routes (>99% of traffic), the second lookup is wasted ABI work — and the rewrite of identical headers is wasted CPU.

The duplication exists to cover router-level 405s for unregistered verbs (TRACE/CONNECT/WebDAV), which bypass the middleware chain. Two cleaner options:

1. Have FinalizeResponseMiddleware set a sentinel header like X-TS-Finalized: 1; the entry-point pass becomes a no-op when present (and strips the sentinel before returning).
2. Accept that exotic-verb 405s lack TS headers and remove the entry-point finalize entirely. The cost is one missing header set on responses to TRACE; the gain is one less geo ABI call per real request.

Not blocking — but worth fixing or filing as a perf follow-up before the EdgeZero path becomes the default.

[aram356]

🤔 thinking — get_settings() runs 2× per EdgeZero request and is uncached.

get_settings() does a full TOML parse + validate + emits INSECURE: … warnings on every call (see crates/trusted-server-core/src/settings_data.rs). On the EdgeZero path that work runs twice per request: once inside routes() → build_state() (line 93 of app.rs), and again here at the entry point.

Legacy runs it once (via build_state()). So this PR doubles the per-request settings cost on the new path.

Cheapest fix: cache the parsed Settings in a OnceLock inside get_settings(). Bigger fix: thread state.settings out of routes() and reuse it here.

This also drives the INSECURE: … warning spam (called out separately in the review body).

[aram356]

🤔 thinking — Per-request double open of trusted_server_config.

is_edgezero_enabled() opens the config store, then dispatch_with_config(…, "trusted_server_config") at line 103 opens it again. Fastly's SDK caches the handle so this is cheap, but it's avoidable — if edgezero-adapter-fastly exposes dispatch_with_config_handle (or similar), share one handle.

Previously flagged in rounds 2–3. Non-blocking, but the duplication is visible in the diff and easy to remove.

[aram356]

📝 note — /health shortcut has no test.

This bypasses logging init, settings, and dispatch. Trivial — but the only way to break it is to introduce an ordering bug, which is exactly the regression you'd want a test for. A 5-line test calling main() against Request::get("/health") would lock it down. (Also worth mentioning in the PR description, which currently doesn't.)

[aram356]

🌱 seedling — AppState.kv_store is dead state on most routes.

It's always UnavailableKvStore (line 99), and every consent-aware route replaces it via runtime_services_for_consent_route. Non-consent routes don't read KV. The field exists only so non-consent routes have something to pass through.

Dropping the field would simplify AppState and remove a misleading "we have a kv_store" signal. Out of scope for this PR — worth filing for the legacy-cleanup PR (#495).

[aram356]

👍 praise — PanicGeo whose lookup panics if called is a much sharper assertion than checking absence of headers. This is the right pattern for verifying skip-conditions — the test fails loudly the moment someone removes the 401 short-circuit.

[aram356]

👍 praise — finalize_handle_absorbs_handler_error_and_injects_headers codifies the IntoResponse contract: even when an inner handler errors, finalize headers must be injected. This locked down a real round-2 regression.

[aram356]

👍 praise — Documenting the known router-level-405 limitation in test code (rather than a comment that rots) is the right call. Future readers will understand the entry-point apply_finalize_headers call's purpose without spelunking.