Repository navigation
Conversation
Combine the reusable application lifecycles (#379) with explicit request ingress fidelity. Fastly registry dispatch now converts through into_core_request_with_registries_and_ingress, so the request-only entry point from main also inserts the ingress snapshot after the extension callback, and dispatch_with_registries keeps capturing ingress before conversion. Keep both new CI steps and both new overview guide sections.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Reviewed base 683202c against head 965d482. Requesting changes for the confirmed compatibility regression documented inline as R1.
Verification included fresh locked Cloudflare WASM builds and raw loopback requests through CloudflareService::dispatch in pinned workerd 1.20260415.1 at both revisions. Native runner tests and additional assertion checks passed, as did formatting, shell syntax and whitespace checks. Exact-head CI is green; unchanged earlier checks were reused rather than independently repeated. This was a single-agent review. Spin runtime execution remains CI-only, and production ingress and Trusted Server integration remain unverified.
dhruv8sh
left a comment
There was a problem hiding this comment.
PR Review
Summary
Adds an optional PreDispatchHook on RouterBuilder and adapter-populated RequestIngress metadata (captured target, validated origin, per-axis header fidelity) across all four adapters, plus isolated raw-HTTP runtime fixtures and CI wiring. The core hook design is clean and well tested; findings are mostly about consistency, API ergonomics and scope.
😃 Praise
- The hook is a single
Optioncheck at the top ofRouterInner::dispatch— zero cost when absent, and existing routing is untouched. - Ordering tests are thorough: no state clone, no introspection, no middleware, extension methods, 404/405 paths, and both
Service::callandoneshoterror semantics. - Atomic bounded target capture (no truncated prefix, byte-counted) and the deliberate absence of
Debug/Display/serialization on confidential metadata. - Fastly snapshot-before-native-mutation design, with a test proving a scratch-extension collision cannot replace the adapter snapshot.
- The fixture runner's owned process-group cleanup and cancellation handling are careful and tested.
Findings
Blocking
- ❓ Spin runner flag change is unexplained (
.github/workflows/test.yml:165) — see inline. - 🔧 Contributor-local absolute paths in the spec (
docs/superpowers/specs/2026-10-05-pre-dispatch-request-fidelity-design.md:25) — see inline.
Non-blocking
- 🤔
InboundOriginauthority is not canonical and differs across adapters (crates/edgezero-adapter-fastly/src/request.rs:632,crates/edgezero-adapter-axum/src/request.rs:122) — see inline. - ♻️ Axum Content-Length override is a no-op (
crates/edgezero-adapter-axum/src/request.rs:96) — see inline. - ♻️
RequestIngress::newfallibility and duplicated fidelity defaults (crates/edgezero-core/src/request.rs:294) — see inline. - 🤔 Fastly public API growth: this adds
capture_request_ingress,into_core_request_with_ingress,dispatch_with_registries_and_ingress,into_core_request_with_registries_and_ingressandFastlyService::with_request_ingress. Five new public entry points for one concept is a lot of surface to maintain. Could the_with_registries_and_ingressvariants be the only ones (with the non-ingress versions delegating, as they already do), or could the snapshot be passed through the existingextendcallback / a single options struct? - ⛏ Doc placement (
docs/guide/adapters/axum.md:293) — see inline; applies to six guide pages. - ⛏ Spin helper placement:
capture_request_ingresslives incrates/edgezero-adapter-spin/src/context.rs, but per the adapter pattern conversion helpers belong inrequest.rs(context.rsholds the platform context type). The cfg-gated imports also sit directly against the struct doc comment, andmod ingress_testshas no blank line after the preceding}. - 📝 Cloudflare behavior changes worth a release note: unknown methods are no longer coerced to
GET(they now keep their token, so aGETroute no longer matches them); invalid header values are now a 400 at conversion rather than a 500 from the request builder; and conversion error messages no longer include parse detail. All reasonable, but user-visible. - 🤔 Scope and size: ~10k lines, including two
Cargo.locks, apackage-lock.json, a 1.4k-lineobservations.json, and ~680 lines of spec/plan that lean heavily on an external consumer (Trusted Server approvals, F0/F4 tasks, consumer acceptance gates). Consider trimming consumer-specific narrative from the in-repo spec, and possibly splitting the hook from the ingress-metadata work. Also, the fixture README says 56 probes while the PR body says 54. - 🌱 CI cost and supply chain: the Cloudflare job now installs Chrome, Node, and builds
worker-buildfrom source on every run (cargo install ... --force); the Spin CLI tarball is downloaded without checksum verification (consistent with the existing wasmtime step, but worth tightening for both). Cachingworker-buildand pinning SHA-256s would help.
📌 Out of Scope
#[app]-macro-generated apps have no way to register a pre-dispatch hook; only a hand-writtenHooks::routes()can. Worth a follow-up issue if the hook is meant to be broadly usable.
CI Status
- fmt: PASS
- clippy: PASS
- tests: PASS (workspace, including the new Axum raw-TCP ingress test)
- feature check (
fastly cloudflare spin): PASS - spin wasm32-wasip2 check: PASS
- GitHub checks on the PR: all green
| target: wasm32-wasip2 | ||
| runner_env: CARGO_TARGET_WASM32_WASIP2_RUNNER | ||
| runner_value: wasmtime run | ||
| runner_value: wasmtime run -W component-model-async=y -S p3=y,http=y |
There was a problem hiding this comment.
❓ Why does this PR change the Spin runner flags?
The latest test.yml runs on main succeed with plain wasmtime run for the Spin contract suite, and this change also rewrites the Spin and overview guide instructions. If the P3/async/HTTP flags are required by something introduced here, could you note what in the PR description? Otherwise, please revert this line and the matching doc edits to keep the PR scoped.
| - Section 9.2: bounded cookie inspection, duplicate detection and non-UTF-8 rejection. | ||
| - Sections 12 and 14: hardened local responses and actual adapter ordering/conversion tests. | ||
|
|
||
| Local source: `/Users/prk-jr/Desktop/opensource/rust/trusted-server/docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md`. Local consumer plan: `/Users/prk-jr/Desktop/opensource/rust/trusted-server/docs/superpowers/plans/2026-10-05-mobile-ad-render-trace-implementation-plan.md`, tasks F0/F4. |
There was a problem hiding this comment.
🔧 Contributor-local absolute paths
/Users/prk-jr/Desktop/opensource/rust/trusted-server/... exposes a local filesystem layout and isn't resolvable for anyone else. The GitHub links in the preceding paragraph already reference the same documents.
Fix: drop this line (or replace with pinned GitHub permalinks).
| let scheme = uri.scheme_str()?; | ||
| let _runtime_origin = | ||
| InboundOrigin::parse(scheme, uri.authority()?.as_str(), OriginSource::RuntimeUri).ok()?; | ||
| let origin = InboundOrigin::parse(scheme, host, OriginSource::RuntimeUri).ok()?; |
There was a problem hiding this comment.
🤔 Origin authority is the Host spelling, labelled RuntimeUri
The returned origin is parsed from the Host header, so EXAMPLE.com:80 is retained verbatim (the test asserts this), while Cloudflare and Spin return the URL-parsed authority (example.com). The same request therefore yields different authority() strings per adapter, and the RuntimeUri source label doesn't match where the bytes came from.
Suggestion: after the consistency check, return the origin built from the runtime URI (already computed as _runtime_origin):
let runtime_origin =
InboundOrigin::parse(scheme, uri.authority()?.as_str(), OriginSource::RuntimeUri).ok()?;
// ... host/port consistency check against the Host header ...
Some(runtime_origin)Separately, either canonicalize in InboundOrigin::parse (lowercase host, strip default port) or document that consumers must normalize before comparing. The same applies to Axum (edgezero-adapter-axum/src/request.rs:122).
| if hosts.next().is_some() { | ||
| return None; | ||
| } | ||
| let origin = InboundOrigin::parse("http", host, OriginSource::TransportBinding).ok()?; |
There was a problem hiding this comment.
🤔 Same non-canonical authority as Fastly
The origin authority is the Host header verbatim (case and explicit default port preserved), unlike Cloudflare/Spin. See the Fastly comment on request.rs:632 — consider normalizing in InboundOrigin::parse or documenting that authority() is not canonical.
| Preservation::Unknown, | ||
| Preservation::Unavailable, | ||
| ); | ||
| let content_length = HeaderFidelity::new( |
There was a problem hiding this comment.
♻️ Content-Length override is identical to common
Both are (Unknown, Unknown, Unknown, Unavailable), so the override changes nothing (the docs even say it "retains that same conservative value"). Removing it also removes the .expect and the #[expect(clippy::expect_used)]:
RequestIngress::new(target, inbound_origin(uri, headers, transport), common, Vec::new())| /// | ||
| /// Returns a bounded validation error for duplicate override names. | ||
| #[inline] | ||
| pub fn new( |
There was a problem hiding this comment.
♻️ Constructor fallibility and repeated defaults
new only fails on duplicate overrides, yet Fastly, Axum and Spin each wrap it in .expect(...) behind #[expect(clippy::expect_used)] with an empty/static list. Also, HeaderFidelity::new(Unknown, Unknown, Unknown, Unavailable) is duplicated in all four adapters, and four positional same-typed args are easy to swap silently.
Suggestion:
impl HeaderFidelity {
/// Copied from a runtime HeaderMap; original wire order is never exposed.
pub const RUNTIME_HEADER_MAP: Self = Self::new(
Preservation::Unknown, Preservation::Unknown,
Preservation::Unknown, Preservation::Unavailable,
);
}
impl RequestIngress {
pub fn new(target: CapturedTarget, origin: Option<InboundOrigin>, common: HeaderFidelity) -> Self { ... }
/// Replaces any existing override for `name`.
pub fn with_header_override(mut self, name: HeaderName, fidelity: HeaderFidelity) -> Self { ... }
}| - Deploy to [Cloudflare Workers](/guide/adapters/cloudflare) as an alternative | ||
| - Explore [Configuration](/guide/configuration) for manifest options | ||
|
|
||
| ## Ingress provenance |
There was a problem hiding this comment.
⛏ New section appended after ## Next Steps
The same happens in cloudflare.md, fastly.md, spin.md, routing.md and middleware.md. Please move the new sections above ## Next Steps so that stays the closing section. In this file, the Content-Length multiplicity point is also stated twice (second and third paragraphs).
Summary
Changes
edgezero-corePreDispatchHook/RouterBuilder::pre_dispatch_hookandRequestIngresstarget, origin, and per-header preservation APIs; test terminal responses, continuation mutation, errors, clone sharing, and non-Send stream inspection.edgezero-adapter-fastlyedgezero-adapter-cloudflareedgezero-adapter-axumedgezero-adapter-spintests/fixtures/request-fidelity*,scripts/test-request-fidelity.sh.github/workflows/test.ymldocs/guide/**,docs/superpowers/**The hook runs after successful adapter conversion and existing capability/store bootstrap. It does not bypass external finalizers or change inbound body buffering. Application response guarantees apply to requests the runtime delivers; parser rejection is a separate outcome.
The pinned local observations are capability evidence, not production-wire guarantees: Fastly 0.12.1 cannot expose the original target through its safe API; local workerd normalizes dot segments, combines repeated fields, replaces invalid UTF-8, and rejects extension methods before Worker invocation; Spin 4.0.0 rejects invalid UTF-8 before the component. Unknown preservation remains unknown, and original global header order is unavailable on all adapters. The PR does not recover information already lost or approve a consumer's weaker security contract.
The spec and plan record the user's separate approval on 2026-10-05 of Trusted Server's runtime-boundary amendment. That consumer classifies its application-visible pathname, retains literal-path authentication, rejects still-visible reserved aliases, and conservatively suppresses trace capture for ambiguous runtime Cookie representations. Its historical original-target/original-wire reconstruction requirements are superseded by that approval, not by an EdgeZero policy decision. A reviewed immutable EdgeZero revision, consumer repinning and resolved-graph tests, successful browser workflows on all four adapters, early native ordering, and terminal finalization remain consumer acceptance work; this toolkit PR does not complete consumer F0 or the trace feature.
The latest independent review clarified the Axum guide's common
Unknownmultiplicity and historical versus final probe counts. Origin validation establishes URI authority syntax, including registered-name punctuation and trailing dots, not DNS resolution or ownership. No parser/API change is needed; the follow-up modifies documentation only and passes pinned Node 24.12.0 docs format, lint and build.Closes
Closes #402
Test plan
cargo test --workspace --all-targets— 1,441 passed; 1 ignored.cargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcargo check --workspace --all-targets --features "fastly cloudflare spin"cargo check -p edgezero-adapter-spin --target wasm32-wasip2 --features spinwasm32-wasip1, Spinwasm32-wasip2, and Cloudflarewasm32-unknown-unknown.examples/app-demoworkspace formatting, strict clippy, and locked all-target tests.edgezero serve --adapter axum— not run; Axum real TCP ingress cases run in adapter tests../scripts/test-request-fidelity.sh allunder Node 24.12.0 — 54 cases matched the checked-in local-runtime observations, including unsupported outcomes.actionlintfindings after separating assignment from export. The command still exits 1 for two existing SC2086 findings reproduced against the HEAD workflow (lines 38 and 142 in HEAD; lines 38 and 149 in this diff).Checklist
{id}syntax.edgezero_core; provider-specific adapter types remain provider-owned.