Conversation
- Fix cargo fmt formatting issues - Return EdgeError for unsupported HTTP methods instead of silently defaulting to GET - Add init_logger() matching Cloudflare's no-op pattern, call in run_app - Expose SpinRequestContext unconditionally (pure Rust, no WASM deps) - Add AppExt trait on App for consistent adapter API - Add contract test scaffold and context unit tests - Replace bare .unwrap() with .expect() in proxy header insert - Simplify anyhow wrapping in proxy error path to format!()
705d6fe to
58aa63c
Compare
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Solid adapter implementation that follows the established Fastly/Cloudflare pattern well. However, the adapter does not compile for its actual wasm32-wasip1 target, and the scaffolding pipeline produces broken projects due to missing workspace deps and unresolved template variables. These must be fixed before merge.
😃 Praise
- CLI scaffolding is thorough and well-tested — build/deploy/serve, blueprint registration,
ctorauto-registration, manifest discovery with distance-based sorting, and 4 solid unit tests - Context module is correctly ungated from
target_archso it works in native tests - Method conversion (
into_core_method) handlesMethod::Otherwith proper error reporting — more robust than the Fastly adapter
Findings
Blocking
- 🔧 wasm32 compile error:
EdgeError::internal(format!(...))—Stringdoesn't implInto<anyhow::Error>(proxy.rs:48) - 🔧 Scaffold
{crate_name}placeholder not replaced: generator only handles{crate}/{crate_dir}(cli.rs:178) - 🔧
proj_spin_underscoredtemplate var never populated: renders as empty → brokenspin.tomlsource path (spin.toml.hbs:12) - 🔧 Generated projects fail:
spin-sdkmissing fromseed_workspace_dependencies()ingenerator.rs:109 - 🔧
AppExtduplicatesdispatch()logic: should delegate like Cloudflare/Fastly (lib.rs:42-56) - 🔧 Zero contract test cases:
build_test_app()exists but no#[test]functions (tests/contract.rs) - 🔧 CI feature check missing
spin:.github/workflows/test.yml:59only checksfastly cloudflare
Non-blocking
- 🤔 Silent header drops in proxy.rs and response.rs: non-UTF-8/invalid headers discarded with no logging
- 🤔
client_addrstored as rawString: Fastly uses typedOption<IpAddr>— cross-adapter inconsistency - ♻️
init_logger()identical cfg branches: both returnOk(()), gate adds no value - ♻️ Body stream collection duplicated: identical
Body::Stream → Vec<u8>in proxy.rs and response.rs - ⛏ Template duplicate dependency:
edgezero-adapter-spinin both[dependencies]and[target.wasm32.dependencies]
📌 Out of Scope
- 🌱 No compression/decompression in proxy: Fastly transparently decompresses gzip/brotli upstream responses. Spin passes compressed bytes through raw. Worth tracking for parity.
- 📌
CLAUDE.mdnot updated: workspace layout, compilation targets, CI features all omit Spin.
CI Status
- fmt: PASS
- clippy: PASS
- tests: PASS (native)
- feature check (
fastly cloudflare): PASS - wasm32-wasip1 compile: FAIL (
cargo check -p edgezero-adapter-spin --target wasm32-wasip1 --features spin)
prk-Jr
left a comment
There was a problem hiding this comment.
Can we update the documentation and readme.
- Fix wasm32 compile error: format!() -> anyhow::anyhow!() in proxy.rs
- Fix scaffold {crate_name} -> {crate} placeholder in Spin build command
- Add proj_{id}_underscored template var for spin.toml wasm filename
- Add spin-sdk to seed_workspace_dependencies for generated projects
- Delegate AppExt::dispatch to request::dispatch (matches CF pattern)
- Change dispatch() return to concrete spin_sdk::http::Response type
- Parse client_addr as Option<IpAddr> instead of Option<String>
- Extract shared collect_body_bytes helper to deduplicate proxy/response
- Add log::warn for silently dropped non-UTF-8 headers
- Simplify init_logger by removing redundant cfg gates
- Add contract tests (2 native, 3 wasm32-gated) with proper cfg split
- Add spin to CI feature check and wasm32-wasip1 compilation step
- Update CLAUDE.md and README.md with Spin adapter documentation
Review Fixes — SummaryAll blocking and non-blocking findings from @aram356's review have been addressed in commit 7c2a2f9. Blocking Fixes
Non-blocking Fixes
DocumentationUpdated Verification
@prk-Jr — documentation and README have been updated with Spin adapter info as requested. |
aram356
left a comment
There was a problem hiding this comment.
Follow-up review — critical pathing bugs
The previous review addressed compile errors and code-quality issues. This review focuses on two runtime-breaking bugs that cause edgezero-cli build --adapter spin to fail on any hyphenated crate name or workspace member. Both are reproducible in the examples/app-demo demo.
Summary of findings
| Sev | Issue | Impact |
|---|---|---|
| High | Artifact lookup uses raw package name (hyphens) but Cargo emits underscores | build --adapter spin exits with "compiled artifact not found" for any hyphenated crate |
| High | spin.toml source path points to crate-local target/, but workspace builds emit to workspace target/ |
spin up fails to find the wasm binary for any workspace member |
| Medium | find_workspace_root climbs to the topmost Cargo.toml, overshooting nested workspaces like examples/app-demo/ |
Build/deploy commands may search the wrong tree |
| Medium | Non-UTF-8 headers silently dropped across request/response/proxy boundaries | Breaks binary headers (signatures, auth tokens) in proxy edge cases |
| Medium | Test suite only exercises simple (non-hyphenated, flat workspace) artifact lookup — misses both regressions above | False green on CI |
Fix artifact lookup hyphen-to-underscore bug in all three adapter CLIs, stop find_workspace_root at [workspace] tables for nested workspaces, compute correct target dir in spin.toml template for workspace members, and use HeaderValue::from_bytes for inbound request/proxy headers. Adds tests for hyphenated crate names and nested workspace resolution.
Review Fixes — Follow-up Review (Mar 11)All findings from @aram356's follow-up review addressed in commit e9f4548.
Verification
|
Summary
Adds a new
edgezero-adapter-spincrate that bridges Spin HTTP components into the EdgeZero router, following the established adapter pattern (Fastly, Cloudflare).crates/edgezero-adapter-spin/with context, request, response, proxy, and CLI modulescargo edgezero new --adapter spinscaffoldingexamples/app-demo/crates/app-demo-adapter-spin/reference implementationinit_logger(),AppExttrait,run_app()convenience entry point — matching Fastly/Cloudflare adaptersWhat's Included
context.rsSpinRequestContext— extractsspin-client-addrandspin-full-urlheadersrequest.rsspin_sdk::http::IncomingRequest→ coreRequestwith proper WASI resource handle managementresponse.rsResponse→spin_sdk::http::Response(buffered + streaming bodies)proxy.rsSpinProxyClientusingspin_sdk::http::sendfor outbound HTTPcli.rscargo build --target wasm32-wasip1), deploy (spin deploy), serve (spin up) commandslib.rsinit_logger(),AppExttrait,run_app()entry pointWhat's Not Included (Future Work)
Testing
cargo fmt --all -- --check— cleancargo clippy --workspace --all-targets --all-features -- -D warnings— cleancargo test --workspace --all-targets— all tests pass (6 Spin adapter tests including context unit tests)cargo check --workspace --all-targets --features "fastly cloudflare"— cleantests/contract.rs(gated behindspin + wasm32, follows Fastly/Cloudflare pattern)