Skip to content

Add pre-dispatch hooks and explicit request ingress fidelity - #403

Open
prk-Jr wants to merge 6 commits into
mainfrom
spec/pre-dispatch-request-fidelity
Open

prk-Jr wants to merge 6 commits into
mainfrom
spec/pre-dispatch-request-fidelity

Conversation

@prk-Jr

@prk-Jr prk-Jr commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Let applications return a local response before method/path selection and ordinary middleware, including for unregistered methods delivered by a runtime.
  • Expose bounded, confidential ingress metadata with explicit target, origin, and header-fidelity limits so consumers can distinguish available facts from lost information.
  • Exercise the conversions and runtime boundary through native/browser/WASM contract tests and 54 fixed raw HTTP probes.

Changes

Crate / File Change
edgezero-core Add shared async PreDispatchHook/RouterBuilder::pre_dispatch_hook and RequestIngress target, origin, and per-header preservation APIs; test terminal responses, continuation mutation, errors, clone sharing, and non-Send stream inspection.
edgezero-adapter-fastly Capture ingress before bootstrap/native extension callbacks; support an explicit snapshot through service, registry dispatch, and request conversion; retain client provenance and validate the runtime origin against one consistent Host.
edgezero-adapter-cloudflare Preserve the Web Request method token and runtime URL, copy runtime header strings as UTF-8 bytes, attach ingress metadata with conservative per-request target preservation, and test canonical/normalized URL conversion in the browser.
edgezero-adapter-axum Attach ingress metadata and an adapter-owned plain-HTTP transport binding; add raw TCP parser-versus-hook regressions and avoid claiming Content-Length transformation without original field counts.
edgezero-adapter-spin Attach runtime URI ingress metadata without using client-supplied special headers as origin proof.
tests/fixtures/request-fidelity*, scripts/test-request-fidelity.sh Add locked, isolated fixture workspaces and a bounded native runner with process cleanup, explicit unsupported outcomes, and checked-in observations. Tokio stays confined to the native test runner.
.github/workflows/test.yml Run isolated harness/fixture gates, pinned browser conversion tests, and raw local-runtime probes.
docs/guide/**, docs/superpowers/** Document hook ordering, snapshot integration, adapter capabilities, verification, and consumer boundary decisions.

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 Unknown multiplicity 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 warnings
  • cargo fmt --all -- --check
  • cargo check --workspace --all-targets --features "fastly cloudflare spin"
  • cargo check -p edgezero-adapter-spin --target wasm32-wasip2 --features spin
  • WASM compilation checks for Fastly wasm32-wasip1, Spin wasm32-wasip2, and Cloudflare wasm32-unknown-unknown.
  • examples/app-demo workspace formatting, strict clippy, and locked all-target tests.
  • Docs lint, formatting, and build under Node 24.12.0.
  • Manual testing via edgezero serve --adapter axum — not run; Axum real TCP ingress cases run in adapter tests.
  • Other: isolated native runner tests (9), strict lint and formatting; isolated fixtures strict WASM lint and formatting.
  • Other: Fastly Viceroy contract tests (6) and WASM library tests (94); Cloudflare browser contract tests (11); Spin Wasmtime contract tests (12).
  • Other: freshly source-built ./scripts/test-request-fidelity.sh all under Node 24.12.0 — 54 cases matched the checked-in local-runtime observations, including unsupported outcomes.
  • Other: core documentation tests, Fastly CLI/default-mode checks, placeholder/legacy-read/nested-config checks.
  • Other: targeted workflow lint confirms the new probe step adds no actionlint findings 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).
  • Other: whitespace checks pass and the published worktree is clean.
  • Other: independent reviews approve the API/spec alignment and per-request preservation corrections; all four adapter contracts and 54 raw probes pass after those corrections.

Checklist

  • Changes follow CLAUDE.md conventions.
  • No Tokio dependencies added to core or adapter crates.
  • Route parameters use {id} syntax.
  • Portable types come from edgezero_core; provider-specific adapter types remain provider-owned.
  • Existing store registry wiring remains intact.
  • New code has tests.
  • No secrets or credentials included in the changed files or fixture configuration.

@prk-Jr prk-Jr self-assigned this Oct 5, 2026
@aram356
aram356 marked this pull request as draft October 5, 2026 15:56
prk-Jr added 3 commits October 6, 2026 12:44
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.
@prk-Jr
prk-Jr marked this pull request as ready for review October 8, 2026 10:36

@ChristianPavilonis ChristianPavilonis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/edgezero-adapter-cloudflare/src/request.rs Outdated

@dhruv8sh dhruv8sh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Option check at the top of RouterInner::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::call and oneshot error 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

  • 🤔 InboundOrigin authority 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::new fallibility 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_ingress and FastlyService::with_request_ingress. Five new public entry points for one concept is a lot of surface to maintain. Could the _with_registries_and_ingress variants be the only ones (with the non-ingress versions delegating, as they already do), or could the snapshot be passed through the existing extend callback / a single options struct?
  • ⛏ Doc placement (docs/guide/adapters/axum.md:293) — see inline; applies to six guide pages.
  • ⛏ Spin helper placement: capture_request_ingress lives in crates/edgezero-adapter-spin/src/context.rs, but per the adapter pattern conversion helpers belong in request.rs (context.rs holds the platform context type). The cfg-gated imports also sit directly against the struct doc comment, and mod ingress_tests has 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 a GET route 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, a package-lock.json, a 1.4k-line observations.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-build from 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). Caching worker-build and pinning SHA-256s would help.

📌 Out of Scope

  • #[app]-macro-generated apps have no way to register a pre-dispatch hook; only a hand-written Hooks::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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 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()?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 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()?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 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(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ 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(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛏ 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).

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add pre-dispatch hooks and explicit request ingress fidelity

3 participants