Add Spin framework adapter#58
Conversation
1464474 to
8e8edec
Compare
8e8edec to
6ed52e7
Compare
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
The PR adds a Spin framework adapter following the existing adapter pattern. The adapter code itself is clean and minimal, but there are fundamental workspace integration issues that prevent the crate from building or being validated by CI.
Findings
🔧 Needs Change
- Crate not in workspace members:
crates/mocktioneer-adapter-spinis not listed in[workspace].membersin rootCargo.toml. All CI gates (test, clippy, fmt) pass only because cargo doesn't know this crate exists — it is never compiled, tested, or linted. - Missing workspace dependencies:
edgezero-adapter-spinandspin-sdkare referenced asworkspace = truebut not declared in[workspace.dependencies]. Hard build failure once the crate is added to workspace members. - Duplicate dependency with conflicting features:
edgezero-adapter-spinappears in both[dependencies]and[target.wasm32.dependencies]with different feature specs. See inline comment for details. - Missing
license.workspace = true: Inconsistent with Fastly and Cloudflare adapters. - Empty crate on host target: All code is
#[cfg(target_arch = "wasm32")], leaving an empty lib for host builds.
❓ Questions
- Does
edgezero-adapter-spinexist upstream? It's not inCargo.lockor workspace deps. If the edgezero framework doesn't have Spin support yet, this PR is premature. - Spin 2 vs Spin 3?
spin_manifest_version = 2+wasm32-wasip1= Spin 2.x. Spin 3.x uses WASI Preview 2. Which is targeted?
🤔 Thoughts
spin.tomlbuild command missing-pflag: Will try to build entire workspace for WASM, which will fail (Axum can't compile to WASM).
⛏ Nitpicks
- Missing
echo_stdoutin spin adapter logging (edgezero.toml): Other adapters explicitly set this. Minor inconsistency.
🌱 Seeds
- CI feature check: Once merged,
cargo check --features "fastly cloudflare"should become--features "fastly cloudflare spin".
👍 Praise
- Clean route registration: All existing routes correctly updated with
"spin"in the adapters array. No routes missed. - Adapter stays thin: 12 lines delegating to
edgezero_adapter_spin::run_app. Exactly the right pattern.
CI Status
- fmt: PASS (but spin crate not checked — not in workspace)
- clippy: PASS (but spin crate not checked — not in workspace)
- tests: PASS (but spin crate not compiled — not in workspace)
- Add mocktioneer-adapter-spin to workspace members - Add edgezero-adapter-spin and spin-sdk to workspace dependencies - Add license.workspace = true to adapter Cargo.toml - Fix duplicate edgezero-adapter-spin dependency, set default = [] - Add module doc and cfg_attr for non-WASM host compilation - Update to Spin 3: manifest v3, wasm32-wasip2 target, spin-sdk 5.2 - Add -p flag to spin.toml and edgezero.toml build commands - Update CI feature check to include spin
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Clean adapter addition that follows the thin-adapter pattern well. All CI gates pass (fmt, clippy, tests, feature check). One blocking item on dependency placement.
Findings
🔧 Needs Change
edgezero-corein wrong dependency section: Should be in base[dependencies], not WASM-only target section — inconsistent with Cloudflare adapter and fragile if the adapter evolves (crates/mocktioneer-adapter-spin/Cargo.toml:23)
❓ Questions
- No
main.rsfallback: Fastly adapter has a#[cfg(not(target_arch = "wasm32"))] fn main()stub. Spin is acdyliblike Cloudflare (which also lacks one), so this is consistent — confirming it's intentional? - Release-only builds for local dev:
spin.tomlsource path and build command always use--release. Consistent with Fastly's pattern, but meansspin upalways does a release build.
🤔 Thoughts
anyhowdependency asymmetry: Neither Fastly nor Cloudflare adapters depend onanyhow. Justified byspin-sdkAPI contract — just noting the asymmetry.- Missing
echo_stdoutin spin logging section: Fastly adapter hasecho_stdout = false, Spin doesn't. Cloudflare also omits it, so consistent with at least one adapter.
🌱 Seeds
- Wildcard
allowed_outbound_hosts:["https://*:*"]is broad for a mock bidder making no outbound requests. Consider tightening to[]or documenting why (spin.toml:13). - Documentation updates needed: CLAUDE.md workspace layout, compilation targets table, and project overview don't mention Spin. Should be a follow-up.
📝 Notes
spin_manifest_version = 3is correct for Spin 3.x.gitignoreadditions for.spin/logs/are appropriateCargo.lockchanges bring in legitimate Spin ecosystem crates
CI Status
- fmt: PASS
- clippy: PASS
- tests: PASS (90 tests)
- feature check (
fastly cloudflare spin): PASS
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Clean, well-structured Spin adapter that follows established project patterns. One security-relevant finding around outbound permissions; the rest are questions and suggestions.
Findings
🔧 Needs Change
- Overly permissive
allowed_outbound_hosts:https://*:*grants full outbound HTTPS access but Mocktioneer never makes outbound calls (crates/mocktioneer-adapter-spin/spin.toml:13)
❓ Questions
- Does edgezero-adapter-spin require outbound host permissions?: If the framework makes outbound calls at init or during request processing, the field needs to stay but should be scoped to specific hosts rather than a wildcard
- Is
edgezero-coreneeded as a direct wasm32 dependency?:lib.rsnever imports from it directly — other WASM adapters have the same pattern, is it needed for Cargo feature unification? (crates/mocktioneer-adapter-spin/Cargo.toml:23)
🤔 Thoughts
- Missing
echo_stdoutin logging config: Other adapters explicitly set this field; Spin section omits it (edgezero.toml:218)
📌 Out of Scope
- Documentation needs updating: Adapters overview, architecture diagram, and CLAUDE.md compilation targets table don't include Spin yet — follow-up PR
- EdgeZero CLI
--adapter spinsupport: Confirm the CLI recognizes thespinadapter fromedgezero.tomlentries
CI Status
- fmt: PASS
- clippy: PASS
- tests: PASS (71 unit + 19 integration)
- Move edgezero-core to base [dependencies] with feature-unification comment - Tighten allowed_outbound_hosts to [] in spin.toml - Add echo_stdout = true to spin logging config in edgezero.toml - Update CI gate docs to include spin feature in cargo check command
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
New mocktioneer-adapter-spin crate wiring Mocktioneer onto Fermyon Spin via wasm32-wasip2. Diff is tight and follows the existing Fastly/Cloudflare adapter template. Build works, tests pass, and the wasm target compiles locally (9.3 MB output). Blocking items are in-scope CLAUDE.md updates — the file was partially refreshed (CI gate line) but several other stale sections were missed. A few Cargo.toml hygiene questions round out the feedback.
Findings
🔧 Needs Change
- CLAUDE.md — Project Overview sentence stale (CLAUDE.md:7-8): "Write once, deploy to Fastly Compute, Cloudflare Workers, or native Axum servers" — add Fermyon Spin.
- CLAUDE.md — Workspace crate count / layout stale (CLAUDE.md:9 + 15-24): line 9 still says "Cargo workspace with 4 crates" (now 5), and the layout tree omits
mocktioneer-adapter-spin/. - CLAUDE.md — Compilation Targets table missing Spin row (CLAUDE.md:67-71). Append:
| Spin | `wasm32-wasip2` | Requires `spin` CLI for local/deploy | - CLAUDE.md — Key Files Reference table missing Spin entry (CLAUDE.md:290-292). Add:
| Spin adapter | `crates/mocktioneer-adapter-spin/src/lib.rs` |
❓ Questions (inline)
- Misleading Cargo.toml comment about "cloudflare/fastly pattern" — Fastly is target-gated, not unconditional.
anyhowdeclared unconditionally but only used in wasm-gated code.spin-sdkwithdefault-features = false— intentional? Worth a one-line justification.allow(unused_imports)appears unnecessary since everyuseis already cfg-gated.
♻️ Refactor (inline)
[adapters.spin.commands]key ordering inconsistent with the other three adapters (build→deploy→serve).- Flag-order drift between the
buildcommand inedgezero.tomlvsspin.toml.
🌱 Seeds / 📌 Out of Scope
- No docs page for the Spin adapter.
docs/guide/adapters/hasfastly.md/cloudflare.md/axum.md, anddocs/guide/adapters/index.md:10-11plusdocs/.vitepress/config.mts:37-38sidebar only enumerate edge adapters. Reasonable as a follow-up PR to keep this diff runtime-focused — worth a tracking issue. - CI never actually builds the spin wasm.
test.ymlinstalls onlywasm32-wasip1and runscargo check --featureson host. Matches Fastly's pre-existing gap, but a breakage in the target-gatedsrc/lib.rsblocks wouldn't be caught. Worth a separate issue to tighten wasm build coverage across all three wasm adapters. spin.tomlwatchlist too narrow — won't retrigger onmocktioneer-core/src/**/*.rsoredgezero.tomlchanges duringspin watch.
👍 Praise
- Tight
allowed_outbound_hosts = []aligns with Mocktioneer's deterministic / self-contained design. - CI feature list (
test.yml) andCLAUDE.md:164updated in lockstep. - Full cfg-gating lets host tooling (fmt / clippy / test) run without
wasm32-wasip2installed. .gitignorescoped narrowly to.spin/logs/rather than a blanket.spin/.
CI Status (local verification)
cargo fmt --all -- --check: PASScargo clippy --workspace --all-targets --all-features -- -D warnings: PASScargo test --workspace --all-targets: PASS (19 tests)cargo check --workspace --all-targets --features "fastly cloudflare spin": PASScargo build --release --target wasm32-wasip2 -p mocktioneer-adapter-spin: PASS (9.3 MB wasm)
Summary
mocktioneer-adapter-spincrate so Mocktioneer can run on the Fermyon Spin platform via WASI.edgezero.toml..gitignore.Changes
crates/mocktioneer-adapter-spin/Cargo.tomlcdylibtarget,spinfeature, and WASM-only depscrates/mocktioneer-adapter-spin/src/lib.rsedgezero_adapter_spin::run_appcrates/mocktioneer-adapter-spin/spin.tomledgezero.toml"spin"to every route'sadapterslist; added[adapters.spin]section with build/serve/deploy commands.gitignore.spin/logs/directoriesTest plan
cargo test --workspace --all-targetscargo clippy --workspace --all-targets --all-features -- -D warningscargo check --workspace --all-targets --features "fastly cloudflare"Checklist
mocktioneer-core, not in adapter cratesroutes.rsandedgezero.toml