Skip to content

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

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

prk-Jr wants to merge 8 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 a checked-in baseline of 56 raw HTTP cases (Fastly 20, Cloudflare 19, Spin 17).

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 consumer-specific security amendment and acceptance gates remain documented in the historical design record; consumer repinning, resolved-graph tests and browser acceptance remain downstream work.

Review follow-up:

  • Use one conservative HeaderFidelity::RUNTIME_HEADER_MAP baseline across adapters and remove Axum's redundant Content-Length override. The constructor still rejects duplicate overrides as required by the design.
  • Return Fastly's validated runtime URI origin after Host consistency checks. Authority spelling is source-provided and consumers must normalize before comparison. Retain the snapshot/conversion/service/registry entry points for their distinct callers; shared conversion remains centralized, and passing the snapshot through mutable extensions would weaken the adapter-owned boundary.
  • Keep Spin's pure ingress helper under request/ingress.rs with host test coverage. Restore plain wasmtime run; the unchanged contract suite passes all 12 tests under pinned Wasmtime 44.0.1 without extra flags.
  • Remove contributor-local references, keep Next Steps last in six guides, and correct the checked-in baseline to 56 cases. Historical 54-case runs below remain historical; no fresh 56-probe run is claimed here.
  • Preserve unknown Cloudflare method tokens instead of coercing them to GET; invalid runtime header values now return 400, and method/URL/URI/header conversion errors use bounded messages without parse details. Existing SDK primary URL conversion remains intact.
  • Reuse cached worker-build only at the exact pinned version and verify the new Spin runtime archive against its official SHA-256 digest. Fixture lockfiles and observations remain for reproducibility.

Closes

Closes #402

Review follow-up validation — 2026-10-15

  • cargo test --workspace --all-targets — 1,540 passed, 1 ignored.
  • cargo fmt --all -- --check
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • cargo check --workspace --all-targets --features "fastly cloudflare spin"
  • cargo check -p edgezero-adapter-spin --target wasm32-wasip2 --features spin
  • Fastly WASM library tests under Viceroy — 97 passed. The updated origin regression failed before the fix and passed afterward.
  • Spin WASM contract tests under plain pinned Wasmtime 44.0.1 — 12 passed.
  • Docs lint, format and build under Node 24.12.0.
  • actionlint .github/workflows/test.yml, whitespace checks, baseline counts and six guide closing-section assertions.
  • Independent Rust and docs/workflow review found no remaining findings after the wording corrections.

The full 56-case runtime fixture matrix was not rerun during this review follow-up. Earlier execution evidence below is historical.

Validation before this review follow-up

  • 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 the historical 54 raw probes passed after those corrections. The checked-in baseline now includes two additional Cloudflare no-hook routing regressions (56 total).

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

Comment thread .github/workflows/test.yml Outdated
Comment thread docs/superpowers/specs/2026-10-05-pre-dispatch-request-fidelity-design.md Outdated
Comment thread crates/edgezero-adapter-fastly/src/request.rs
Comment thread crates/edgezero-adapter-axum/src/request.rs
Comment thread crates/edgezero-adapter-axum/src/request.rs Outdated
Comment thread crates/edgezero-core/src/request.rs
Comment thread docs/guide/adapters/axum.md

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

😃 Approved with non-blocking comments. The explicit ingress-fidelity limits clearly distinguish runtime-visible facts from information already lost.

♻️ The inline feedback focuses on sharing the conservative header-fidelity baseline, removing Axum’s redundant override, and tightening documentation: reflect the 56-case checked-in baseline without implying a fresh 56-probe run, replace contributor-local references, and keep Next Steps as the closing guide section.

Comment thread docs/superpowers/plans/2026-10-05-pre-dispatch-request-fidelity.md Outdated
Comment thread crates/edgezero-adapter-cloudflare/src/request.rs Outdated
Comment thread crates/edgezero-adapter-axum/src/request.rs Outdated
Comment thread docs/superpowers/specs/2026-10-05-pre-dispatch-request-fidelity-design.md Outdated
Comment thread docs/guide/adapters/axum.md
@prk-Jr
prk-Jr requested a review from dhruv8sh October 15, 2026 10:03

@aram356 aram356 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.

PR Review

Summary

Reviewed c0a90d62 against main (85d933b8). The hook and ingress code itself is unchanged by the merge and passes Rust 1.98.1's stricter lints. However, the merge left the isolated request-fidelity fixture on the previous toolchain, and all three wasm test jobs now fail. One design question about Fastly's original-header APIs bears on the fidelity contract, and a few docs and test-accuracy issues need fixing before merge.

Findings

Blocking

  • 🔧 The main merge left the fixture toolchain stale; all three wasm jobs fail. The fixture's exact pins (fastly 0.12.1, spin-sdk 6.0.0, worker 0.8.3, wasm-bindgen 0.2.122) no longer match the adapters. A verified pin bump, the CI tool versions and a Spin baseline re-record are needed (tests/fixtures/request-fidelity/Cargo.toml:32).
  • ❓ Fastly's original-header APIs were never evaluated. get_original_header_names() and get_original_header_count() contradict global_field_order: Unavailable on Fastly (crates/edgezero-adapter-fastly/src/request.rs:615).
  • 🔧 The Cloudflare "400" claim is wrong for the standard runners, which return 500 (docs/guide/adapters/cloudflare.md:375).
  • 🔧 The runner's UTF-8 regression test passes for the wrong reason (tests/fixtures/request-fidelity-runner/src/main.rs:1168).
  • 🔧 One contributor-local path remains (docs/superpowers/plans/2026-10-05-pre-dispatch-request-fidelity.md:342).

Non-blocking

  • 🤔 RuntimeUri and the guides overstate how trustworthy the origin is (crates/edgezero-core/src/request.rs:251).
  • ♻️ Split RequestIngress::new so the zero-override path can't fail (crates/edgezero-core/src/request.rs:306).
  • ♻️ Add a Debug that prints no values, following the store-handle pattern (crates/edgezero-core/src/request.rs:279).
  • ♻️ The authority comparison is duplicated across Fastly and Axum (crates/edgezero-adapter-fastly/src/request.rs:621).
  • 🤔 Hook registration: Arc-only, and a second registration silently replaces the first (crates/edgezero-core/src/router.rs:207).
  • 🤔 The overview implies Cloudflare-specific header fidelity (docs/guide/adapters/overview.md:189).
  • 📝 Document the hook's limits (docs/guide/routing.md:218).
  • ♻️ A baseline mismatch prints no diff (tests/fixtures/request-fidelity-runner/src/main.rs:431).
  • 🌱 CI cost and supply chain (.github/workflows/test.yml:339).
  • ⛏ Cloudflare reads the URL across the JS boundary twice (crates/edgezero-adapter-cloudflare/src/request.rs:258).
  • ⛏ Spin #[path] module placement (crates/edgezero-adapter-spin/src/lib.rs:11).
  • ⛏ Fastly capture-before-bootstrap doesn't change behavior (crates/edgezero-adapter-fastly/src/lib.rs:157).
  • ⛏ InboundOrigin limit, port and default-baseline details (crates/edgezero-core/src/request.rs:188).
  • ♻️ The spec and plan contradict the final code and read like a session log.
    • Stale content:
      • The Fastly signature still includes EnvConfig/env (spec:141, plan:145).
      • Cloudflare fidelity is called "transformed"/"unavailable" (spec:152, :154), but the code uses Unknown.
      • Spin runner flags are still listed (plan:195), though CI uses plain wasmtime run.
      • The Spin helper is placed in context.rs (plan:292).
      • Case counts appear as 47 (plan:294, :321, :352), 54 and 56.
    • Session-log content:
      • Approval logs ("the user authorized/approved/rejects/requested", spec:214, :224 and plan:20, :319, :346, :373).
      • Agent-tool instructions ("Use this instruction with EdgeZero's Codex", plan:259; "Independent Claude audit follow-up", plan:358).
      • F0/F4/F5 consumer labels that are never defined (spec:191-193, :240).
    • The consumer-negotiation sentence "EdgeZero does not approve a weaker consumer contract" also leaks into the public guide (overview.md:212).
    • Suggest updating the stale lines or adding a "superseded by §…" banner (main's 2026-09-15-reusable-app-lifecycle-design.md does this), condensing the review history into a short decision log, and defining or dropping the consumer labels.
  • ⛏ Runner nits:
    • cargo metadata --locked --no-deps (main.rs:259-265) exits 0 with a stale lock; drop --no-deps or the call.
    • Drop swallows /bin/kill failures and then wait()s, which can hang forever where /bin/kill doesn't exist (main.rs:196-197); call self.child.kill() first.
    • Child processes inherit the runner's stdin (main.rs:151-156), which can stop a TTY-reading child such as wrangler's hotkeys; use Stdio::null().
  • 🌱 Ingress capture always runs. Every adapter captures the target, parses the origin and allocates Arc<str> on every request, even when no hook or consumer is installed. It's cheap, but it adds per-request CPU on edge platforms. Worth making it lazy or opt-in later.

CI Status

At c0a90d62, run locally on Rust 1.98.1:

  • fmt: PASS
  • clippy: PASS
  • tests: PASS (1,544 passed, 0 failed, 1 ignored)
  • feature check (fastly cloudflare spin): PASS
  • spin wasm32-wasip2 check: PASS
  • check_shared_dep_pins.sh: PASS (it does not cover this fixture)

GitHub at c0a90d62:

  • spin wasm tests, cloudflare wasm tests, fastly wasm tests: FAIL at "Lint isolated ingress fixture" (stale fixture Cargo.lock).
  • All other checks pass.

edgezero-adapter-fastly = { path = "../../../crates/edgezero-adapter-fastly", features = ["fastly"], optional = true }
edgezero-adapter-cloudflare = { path = "../../../crates/edgezero-adapter-cloudflare", features = ["cloudflare"], optional = true }
edgezero-adapter-spin = { path = "../../../crates/edgezero-adapter-spin", features = ["spin"], optional = true }
fastly = { version = "=0.12.1", optional = true }

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.

🔧 The main merge left this fixture on the old toolchain, so all three wasm jobs fail

spin, cloudflare and fastly wasm tests fail at Lint isolated ingress fixture at c0a90d62:

error: cannot update the lock file .../tests/fixtures/request-fidelity/Cargo.lock because --locked was passed to prevent this

The path deps now require fastly 0.13, spin-sdk ~7.0 and worker 0.8.5. Refreshing only the lock is not enough. It resolves two copies of fastly (0.12.1 + 0.13.1) and two of spin-sdk (6.0.0 + 7.0.0), and I reproduced both builds failing:

expected `fastly::http::request::Request`, found `fastly::Request`
expected `IncomingBody<Request>`, found a different `IncomingBody<Request>`

Fix (verified): moving the pins to the root versions makes cargo clippy --all-targets -- -D warnings pass on all three targets with no source changes:

fastly = { version = "=0.13.1", optional = true }
worker = { version = "=0.8.5", default-features = false, features = ["http"], optional = true }
wasm-bindgen = { version = "=0.2.128", optional = true }
spin-sdk = { version = "=7.0.0", default-features = false, features = ["http"], optional = true }

Then regenerate Cargo.lock. The rest of the toolchain has to move with it:

  • .github/workflows/test.yml:344: worker-build 0.8.3 → 0.8.5. The runner checks that worker-build matches the fixture's worker.
  • Cargo.toml:8 and test.yml:357-358: Spin CLI 4.0.0 → 4.1.0. Main's .tool-versions notes that spin-sdk 7 needs Spin 4.1+ for wasi:http/types@0.3.0. GitHub's published digest for spin-v4.1.0-linux-amd64.tar.gz is sha256:aeeb892f94cf861f5302546faa0bdaa82cc4be30539621299746a6dc011f3b5a.
  • test.yml:376: once both locks agree, the exported WASM_BINDGEN_BIN matches again. Reading the version from the fixture's own lock would stop this from recurring.
  • scripts/check_shared_dep_pins.sh (new on main): please add this fixture. It covers the other spin-sdk copies but not this one, which is why it passes today.

Baseline under the new runtimes: I ran the probes locally with the bumped pins.

  • Fastly, Viceroy 0.21.0: all 20 cases match observations.json.
  • Spin 4.1.0: 2 of 17 cases change. The hook no longer sees Transfer-Encoding on chunked and te-cl (transfer_encoding_chunked: true → false, transfer_encoding_count: 1 → 0), so the Spin baseline needs re-recording and a note on why.
  • Cloudflare: not run locally.

Stale version text:

  • README :3 and :14 (Rust 1.95.0, Node 24.12.0, Viceroy 0.17.0, Spin 4.0.0, worker-build 0.8.3, wasm-bindgen 0.2.122).
  • main.rs:894 ("Spin 4.0 rejects…").
  • "SDK 0.12.1" in crates/edgezero-adapter-fastly/src/request.rs:586, docs/guide/adapters/fastly.md:608 and overview.md:197. I checked fastly 0.13.1: it still has no raw-target accessor, so NotExposed stands.
  • The PR body's Wasmtime 44.0.1 (main is on 48.0.1).

RequestIngress::new(
target,
origin,
HeaderFidelity::RUNTIME_HEADER_MAP,

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.

❓ Was Fastly's original-header API evaluated before reporting global order as Unavailable?

Preservation::Unavailable means "the runtime does not expose the required information". The Fastly SDK used here (0.12.1, and still in 0.13.1) has:

  • Request::get_original_header_names(): "the client request's header names exactly as they were originally received… the original order of the received headers"
  • Request::get_original_header_count(): "the number of headers in the client request as originally received"

Neither the spec, the plan, the guides nor the fixture mention them. I probed Viceroy 0.21.0 with a client request whose headers were host, X-Zeta, cookie, X-Alpha, COOKIE, Connection:

names=Some("host|x-zeta|cookie|x-alpha|connection") count=Some(6) from_client=true

Locally the names come back lowercased and de-duplicated, but in first-occurrence order. The count is exact and includes both cookie fields. Production Compute may match the SDK docs more closely, but that needs a check on the real platform.

Either way, for Fastly client requests global_field_order: Unavailable doesn't hold, and field multiplicity can be checked by comparing counts. Options:

  1. If this is deliberately out of scope, record why in the spec, and report Unknown instead of Unavailable for Fastly.
  2. Otherwise, compare get_original_header_count() with the converted HeaderMap length per request. That gives evidence of folded or dropped fields, which is the duplicate-control-field case the design cares about. It would also give the per-header override path its first production caller.

### Conversion compatibility

Unknown method tokens are retained rather than coerced to GET, so they no longer
match a GET route. Invalid runtime header values return 400 during conversion.

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.

🔧 Clients don't get a 400 for invalid header values through the normal entry points

into_core_request returns EdgeError::bad_request("invalid runtime header value") (crates/edgezero-adapter-cloudflare/src/request.rs:284). But dispatch_with_handles maps every conversion error through edge_error_to_worker into WorkerError::RustError. The worker 0.8.3 #[event(fetch)] wrapper then turns any Err into Response::error(.., 500) (worker-macros src/event.rs:80-84, 113-117). So run_app, dispatch_app and CloudflareService callers get 500. The PR description says the same "400" thing, and the into_core_request rustdoc # Errors section still lists only URL/URI failures.

Two options:

  • Correct the docs:

    Invalid runtime header values fail conversion with a bounded EdgeError::bad_request. Direct into_core_request callers receive that error; run_app and CloudflareService surface it as a worker::Error, which the fetch wrapper returns as 500.

  • Or deliver the intended status: in dispatch_with_handles, render conversion errors with EdgeError::into_response() and convert that response, instead of turning them into WorkerError.

Please also add the new method and header-value failures to the # Errors section.

}

#[test]
fn observation_verification_rejects_utf8_corruption_even_during_collection() {

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.

🔧 The UTF-8 half of this regression test passes for the wrong reason

The verdict has no origin_available or origin_matches_expected, so verify_observation fails at the origin check (around line 770) and never reaches the "utf8" check (C3 A9). A correct verdict (cookie_utf8: true, cookie_latin1: false) with the same fields fails with the same error. The test therefore wouldn't catch the C3 A9 → E9 defect coming back. (The unicode/500 half is fine: it reaches the "unexpected HTTP status" arm.)

let verdict = json!({
    "hook": true, "ordinary": false, "global_order_unavailable": true,
    "spoofed_origin_ignored": true, "origin_available": true,
    "origin_matches_expected": true, "target_available": true,
    "target_matches_runtime": true, "cookie_utf8": false, "cookie_latin1": true
});
let error = verify_observation("cloudflare", "utf8", 200, &verdict)
    .expect_err("should reject the former C3 A9 to E9 defect");
assert!(
    error.to_string().contains("C3 A9"),
    "should fail on the UTF-8 check, not an earlier one: {error}"
);


Preserving the approved consumer contract requires upstream capability work followed by real ingress verification. An alternative runtime-boundary contract must explicitly define normalized-path classification, transport-level rejection and unavailable-cookie reporting; it requires user approval because it changes the approved feature semantics. Disabling feature support on selected adapters also changes the approved all-four-adapter scope. This review does not silently choose either alternative.

Pinned primary source for the newly identified encoding assumption: [workerd UTF-8 header strings](https://github.com/cloudflare/workerd/blob/v1.20260415.1/src/workerd/api/headers.c%2B%2B#L276-L292). The reproduction report is `/tmp/edgezero-fresh-review-encoding.json`; only fixed statuses and boolean verdicts are recorded. Production conversion now preserves Worker UTF-8 strings; exact real Worker verification and the Python-to-Rust tooling replacement are complete.

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.

🔧 One contributor-local path is still here

/tmp/edgezero-fresh-review-encoding.json points at a file on one machine. It's the same kind of reference an earlier review asked to remove, and the cleanup in 02ab00d4 missed this one. Please drop it or replace it with the pinned upstream link already in this paragraph.

# Spin's metadata helper runs on the host; its full lib test artifact
# imports Spin-specific interfaces unavailable in plain Wasmtime.
# The real Spin HTTP fixture below executes the converter instead.
- name: Install lock-matched worker-build

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.

🌱 CI cost and supply chain

  • worker-build build time: the "skip if cached" check is correct, but it never takes effect. In every PR run, cargo install worker-build compiled from source (126–129s). The cache restore misses, and rust-cache cleans ~/.cargo/bin. Overall the PR adds about 4 minutes to the Cloudflare job, about 1 minute to Spin and about 1.3 minutes to Fastly. A pinned prebuilt install (taiki-e/install-action with an exact version, or cargo binstall), plus the fixture target dir in the cache paths, would recover most of that.
  • esbuild download: in module mode, worker-build downloads esbuild from the npm registry with no checksum unless ESBUILD_BIN is set. package-lock.json already pins esbuild with an integrity hash, so ESBUILD_BIN="$PWD/tests/fixtures/request-fidelity/node_modules/.bin/esbuild" would match the pinned-digest approach used for Spin.
  • Small items:
    • Use node-version-file: .tool-versions instead of hard-coding the Node version.
    • Add --retry to the Spin curl, and check the version before downloading.
    • The Spin lib-tests comment at :336-338 sits above the worker-build step.

let method = into_core_method(&req.method());
let method = CoreMethod::from_bytes(req.inner().method().as_bytes())
.map_err(|_error| EdgeError::bad_request("invalid runtime method"))?;
let runtime_url = req.inner().url();

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.

⛏ The runtime URL crosses the JS boundary twice

req.inner().url() copies the URL string, and then req.url() calls edge_request.url() again before parsing it (worker 0.8.5 Request::url). Parsing the copy already in hand keeps the same url::Url behavior as req.url(), which the earlier routing fix depends on:

let runtime_url = req.inner().url();
let target = CapturedTarget::capture(&runtime_url, TargetSource::RuntimeUrl, Preservation::Unknown);
let url: worker::Url = runtime_url
    .parse()
    .map_err(|_error| EdgeError::bad_request("invalid runtime URL"))?;

pub mod context;
mod decompress;
#[cfg(any(test, all(feature = "spin", target_arch = "wasm32")))]
#[path = "request/ingress.rs"]

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.

⛏ Module path and file location disagree

#[path = "request/ingress.rs"] mod ingress; declares crate::ingress at the crate root, but the file lives under src/request/, which suggests crate::request::ingress. That confuses readers and IDE navigation. Since request.rs is wasm-only and this helper must build on the host for tests, putting it at src/ingress.rs (plain mod ingress;) keeps the host coverage without #[path].

A: Hooks,
F: FnOnce(&fastly::Request, &mut Extensions),
{
let ingress = request::capture_request_ingress(&req);

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.

⛏ Capturing before bootstrap here doesn't change behavior

init_logger and A::build_app() never touch req, so capturing before them has the same effect as capturing inside dispatch_with_registries, which already captures before the extension callback. Meanwhile serve_app_with_request_extensions (:391) still goes through dispatch_with_registries, so the crate now has two capture patterns. Reverting this call site to request::dispatch_with_registries(&app, req, stores, extend) keeps one pattern and a smaller diff.

return Err(EdgeError::bad_request("invalid ingress origin"));
};
if authority.is_empty()
|| authority.len() > 1024

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.

⛏ Small InboundOrigin details

  • Limit: 1024 is a bare number, while the target limit has a public MAX_TARGET_BYTES. A pub const MAX_AUTHORITY_BYTES: usize = 1024; would make the bound visible to consumers.
  • Ports: a quick probe shows parse accepts example.com:0 and example.com:00080. Port 0 is never a valid destination. Leading zeros fit "source-provided spelling", but they're worth adding to the normalization note, since a consumer comparing against :80 has to handle them.
  • Defaults: HeaderFidelity::default() is all Unknown, while RUNTIME_HEADER_MAP has global order Unavailable. Two "conservative" baselines invite picking the wrong one; the Fastly test snapshot uses default(). Consider removing the Default derive or documenting the difference.

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

4 participants