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
ChristianPavilonis
left a comment
There was a problem hiding this comment.
😃 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.
aram356
left a comment
There was a problem hiding this comment.
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
mainmerge 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()andget_original_header_count()contradictglobal_field_order: Unavailableon 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
- 🤔
RuntimeUriand the guides overstate how trustworthy the origin is (crates/edgezero-core/src/request.rs:251). - ♻️ Split
RequestIngress::newso the zero-override path can't fail (crates/edgezero-core/src/request.rs:306). - ♻️ Add a
Debugthat 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). - ⛏
InboundOriginlimit, 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.
- The Fastly signature still includes
- 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.mddoes this), condensing the review history into a short decision log, and defining or dropping the consumer labels.
- Stale content:
- ⛏ Runner nits:
cargo metadata --locked --no-deps(main.rs:259-265) exits 0 with a stale lock; drop--no-depsor the call.Dropswallows/bin/killfailures and thenwait()s, which can hang forever where/bin/killdoesn't exist (main.rs:196-197); callself.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; useStdio::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 fixtureCargo.lock).- All other checks pass.
dhruv8sh
left a comment
There was a problem hiding this comment.
PR Review
Summary
Re-review at d6e3f4cc. Every blocking item from the Oct 9 and Oct 16 reviews has been addressed: the fixture toolchain pins, Fastly's original-header APIs, the Cloudflare 500 docs, the runner's UTF-8 test and the contributor-local paths. So have most of the non-blocking ones: the shared authority comparison, a constructor that can't fail, a redacted Debug, duplicate-hook rejection, reading the Cloudflare URL once, Spin's ingress.rs, the baseline diff, Stdio::null(), cleanup order, the checksummed Spin download, and the cleaned-up spec and plan. The remaining findings are non-blocking.
😃 Praise
- The Axum raw-TCP ingress test runs real hyper parsing: duplicate and conflicting Content-Length, TE+CL, non-UTF-8 cookie bytes,
%2e/../paths, and the hook's stop, continue and error paths. It also checks that middleware runs only on the continued request. - The trust rules per adapter match what each runtime guarantees. No adapter trusts
Forwarded/X-Forwarded-*, and the tests send spoofed versions of them. - The 16 KiB target cap either stores the whole value or nothing, and the
compile_faildoctest proves callers can't bypass the bound. - Runtime limitations are recorded as pinned, explicitly reported outcomes in
observations.json(56 = 20 + 19 + 17 cases, consistent with the README and runner), not quietly skipped.
Findings
Non-blocking
- 🤔 Fastly global field order is
Unknown; the other adapters sayUnavailable(crates/edgezero-adapter-fastly/src/request.rs:611) — see inline. - 🤔 Ingress dispatch variants have no in-tree caller (
crates/edgezero-adapter-fastly/src/request.rs:393,:444) — see inline. - 🤔 Consider
#[non_exhaustive]on the new public enums (crates/edgezero-core/src/request.rs:289) — see inline. - ⛏ Cloudflare invalid header value → 400 path is untested (
crates/edgezero-adapter-cloudflare/src/request.rs:284) — see inline. - 📝 Cloudflare unknown-method behavior change needs a changelog line (
crates/edgezero-adapter-cloudflare/src/request.rs:257) — see inline. - ⛏ The runner's log read can hide the real error (
tests/fixtures/request-fidelity-runner/src/main.rs:1021) — see inline. - ⛏ The README's "existing" pins claim is stale (
tests/fixtures/request-fidelity/README.md:21) — see inline. - ⛏ Blank line (
crates/edgezero-core/src/request.rs:356) — see inline. - 🌱 Fixture pins are not drift-checked. fastly
=0.13.1, worker=0.8.5and wasm-bindgen=0.2.128match the workspace lock today, butcheck_shared_dep_pins.shonly compares spin-sdk's major version. worker-build0.8.5is also hard-coded intest.yml:340, the runner (main.rs:266) and the README. A workspacecargo updatecould leave the fixture testing an older SDK while the public matrix inoverview.mdnames the wrong versions. Suggest moving worker-build into[package.metadata.request-fidelity]and extending the pin check to the fixture's=pins. - 🌱 Fixture build output isn't cached in CI. Neither the
tests/fixtures/request-fidelity/target/nor therequest-fidelity-runner/target/directory is cached (test.yml:32,:181). Clippy and the runner use different target directories, so each wasm job compiles the fixture twice from scratch.
CI Status
- fmt: PASS
- clippy: PASS (on the pinned Rust 1.98.1)
- tests: PASS
- GitHub checks: all green, including the fastly, cloudflare and spin wasm test jobs
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Re-reviewed ca252139 against main (fc4ccba0). The fixes in d6e3f4cc and 52121d95 resolve every blocking item from the previous review, and nearly all the non-blocking ones, and each fix has a test. I verified them against the code.
Two blocking items remain:
- The newest commit,
ca252139, changes main's cache-seed CI. That is outside this PR's scope, and it couples every deploy smoke job to the cache probes. - The routing guide's hook workaround for
app!apps silently drops store registries.
The rest are smaller docs, tooling and test follow-ups.
Findings
Blocking
- ❓
ca252139changes main's cache-seed CI; please move it to its own PR. Every deploy smoke now depends on the cache probes (.github/workflows/deploy-action.yml:274). - 🔧 The
app!hook workaround silently drops stores, logging config and app name (docs/guide/routing.md:248).
Non-blocking
- ♻️ The fixture pin-guard negative tests mentioned in the reply aren't committed or run in CI (
scripts/check_shared_dep_pins.sh:111). - 🤔 The new top-level
CHANGELOG.mdstarts a repo-wide convention, and its entry leaves out the new public API and the runner-500 nuance (CHANGELOG.md:10). - ⛏
middleware.md:61contradicts the new line 233 (docs/guide/middleware.md:233). - ♻️ Committed docs record one-off run evidence that will go stale (
docs/guide/adapters/overview.md:202). - ⛏ The spec misstates Cloudflare's header-copy baseline (
docs/superpowers/specs/2026-10-05-pre-dispatch-request-fidelity-design.md:13). - ⛏ Local setup instructions don't match worker-build's tool lookup (
tests/fixtures/request-fidelity/README.md:14). - ⛏ Runner error messages leave out the program, the actual version, the tool name, or a non-UTF-8 log (
tests/fixtures/request-fidelity-runner/src/main.rs:258). - ⛏ Leftover "trusted" rustdoc wording (
crates/edgezero-core/src/request.rs:160). - ⛏ Spec lines 143 and 192 contradict "Implemented" and the overview (
docs/superpowers/specs/2026-10-05-pre-dispatch-request-fidelity-design.md:143). - 🏕 Older Fastly versions in a guide this PR edits.
docs/guide/adapters/fastly.md:438("SDK 0.12.1 exposes…") and:445("Viceroy 0.17.0 pinned for the fixtures") predate this PR, but they now contradict its own "Fastly SDK 0.13.1" text at:608and the Viceroy 0.21.0 pin in.tool-versions. Please update both while you're in this guide.
The prebuilt worker-build install that the author deferred (thread left open) is not repeated here.
CI Status
At ca252139, run locally on Rust 1.98.1:
- fmt: PASS
- clippy: PASS
- tests: PASS (1,546 passed, 0 failed, 1 ignored)
- feature check (
fastly cloudflare spin): PASS - spin wasm32-wasip2 check: PASS
check_shared_dep_pins.sh: PASS- fixture clippy (fastly, cloudflare, spin): PASS
- runner tests (11 passed, 1 ignored) and runner clippy: PASS
GitHub: all 28 checks pass.
Summary
Hooksintegration.Changes
edgezero-corePreDispatchHook, value/Arc builder registration with duplicate-registration rejection, and publicrequestmetadata types. Provide bounded target/origin capture, validated header overrides, redacted Debug and shared authority comparison.The hook follows adapter conversion and existing buffering/store bootstrap. It can terminate ordinary router processing; outer application/adapter finalizers may still run. Runtime authority describes the client's requested host and does not establish operator approval.
Axum, Cloudflare and Spin report unknown header octets/multiplicity/same-name order and unavailable global order. Fastly reports all four as unknown: its original-name/count APIs expose partial evidence without associating original fields with converted values. Aggregate count equality cannot prove per-header preservation. Fastly SDK 0.13.1 still has no safe original-target accessor. Workerd normalization/coalescing/replacement and pre-invocation runtime rejection remain capability limits.
Invalid Cloudflare runtime methods/headers produce bounded
EdgeError::bad_requestvalues for direct converter callers. Standard runners surface a Worker error, which the fetch wrapper returns as HTTP 500.The unrelated cache-seed hardening is removed from this diff and tracked in #435 (closes #433). Deploy release jobs remain independent of cache probes in both PRs. Optional hook registration directly through
app!is tracked in #434; this PR documents and tests the full manual workaround instead of expanding the macro API.Closes
Closes #402
Test plan
Verified locally for the review remediation with Rust 1.98.1 and Node 24.20.0:
cargo test --workspace --all-targets— zero failures.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.Tools used for raw probes: Viceroy 0.21.0, Spin 4.1.0, worker-build 0.8.5 and wasm-bindgen 0.2.128. Local tests do not certify production-wire preservation. Worker-build 0.8.5 has no official Linux prebuilt; CI caches its exact compiled executable, compiling only on a cold/mismatched cache. Eager ingress capture remains separate performance work.
Checklist
CLAUDE.mdconventions.{id}syntax.edgezero_core.