Enable strict clippy with documented allow-list and defensive-coding pass#257
Merged
Conversation
Turns on `pedantic` (warn) and `restriction` (deny) workspace-wide and adds
`[lints] workspace = true` to every crate so the policy actually applies.
Captures a baseline allow-list in `Cargo.toml`, organized by category
(Documentation, Style/formatting, Defensive coding, API design, Imports/paths,
Output/diagnostics, Tests, Attributes) with per-lint counts and rationales —
each entry is a TODO unless explicitly marked intentional.
Defensive-coding pass:
- New `clippy.toml` with `allow-{unwrap,expect,panic,indexing-slicing}-in-tests`
so test code keeps its conventional idioms; production code is denied.
- Production unwraps factored out: `current_dir()`/`init_logger()` now
propagate via `?`; `writeln!` to a `String` rewritten as `push_str(&format!)`
so there's no `Result` to discard; bundled-template registration and other
genuine compile-time invariants use `.expect("...")` as documented assertions.
- Other small wins: `inefficient_to_string` fixed, `match_same_arms` collapsed,
`manual_assert` swapped, `cast_lossless`+truncation replaced with bound-checked
`u16::try_from` in adapter-axum CLI, `unreachable!()` in `#[action]` macro
replaced with a proper `syn::Error::compile_error`.
Lints kept allowed in the workspace are annotated with `(intentional)` where
they conflict with idiomatic Rust (`implicit_return`, `question_mark_used`,
`pattern_type_mismatch`, `default_numeric_fallback`, `arithmetic_side_effects`,
`as_conversions`, `string_slice`) or have no per-test config option
(`assertions_on_result_states`).
`cargo clippy --workspace --all-targets --all-features -- -D warnings`,
`cargo fmt`, and `cargo test --workspace --all-targets` all pass.
Drives the API-design lint group from 18 allows down to 8 (kept as intentional
with rationale comments in `Cargo.toml`).
Factored out:
- `return_self_not_must_use` (18): added `#[must_use]` to all `RouterBuilder`
builder methods. Catches "I forgot to call `.build()`" bugs.
- `impl_trait_in_params` (26): converted `fn f(x: impl Into<String>)` →
explicit generics on `EdgeError::*`, `ConfigStoreError::*`, `RouteInfo::new`,
`InMemorySecretStore::new`, `AxumConfigStore::{new,from_env,from_lookup}`.
Makes turbofish callable.
- `rc_buffer` (4): `Arc<Vec<RouteInfo>>` → `Arc<[RouteInfo]>` in `RouterInner`
and the builder. Saves an indirection.
- `unnecessary_wraps` (4): `build_fastly_request` and `convert_response` no
longer wrap an always-Ok value in `Result`. Cleaner call sites.
- `mutex_atomic` (1): `Arc<Mutex<bool>>` → `Arc<AtomicBool>` in the
`middleware_fn` test.
- `ref_patterns` (11): `if let Some(ref x) = ...` → `if let Some(x) = &...`
across env-override `Drop` impls, router builder, response builder, body
matchers.
- `wildcard_enum_match_arm` (7): `args.rs` tests now use `let-else` instead of
catch-all wildcard match arms; `EdgeError::source` now lists each non-Internal
variant explicitly; `cli/build.rs` switched to `if let Value::Table(_) = ...`;
the one site that genuinely matches an external enum (`fastly::config_store::
LookupError`) keeps a localized `#[allow(..., reason = "external enum")]`.
- `clone_on_ref_ptr` (1): `store.clone()` → `Arc::clone(&store)` in the axum
service test (with explicit `Arc<dyn KvStore>` annotation so `Arc::clone`
picks the right type).
- `renamed_function_params` (4): renamed `request: Request` → `req: Request`
in `Service::call` impls to match the trait signature.
- `same_name_method` (2): `EdgeError::source` deliberately shadows
`std::error::Error::source` (typed `&AnyError` vs trait-object `&dyn Error`).
Documented at the call site with a `#[allow(..., reason = "...")]`.
Kept allowed (with `(intentional: ...)` comments in `Cargo.toml`):
- `exhaustive_structs` (108) and `exhaustive_enums` (18): blanket
`#[non_exhaustive]` would break user pattern matching and field-syntax
construction. Apply per-type only when genuinely planned.
- `must_use_candidate` (117): most flagged sites are getters returning
`&str`/`&Path` — ignoring is impossible, the lint adds noise.
- `missing_trait_methods` (20): relying on default trait methods is fine.
- `needless_pass_by_value` (16): most flagged sites are deliberate ownership
transfers — error transformers, proc-macro signatures, builders.
- `field_scoped_visibility_modifiers`, `partial_pub_fields`,
`trivially_copy_pass_by_ref`: deliberate API design choices.
Final clippy + workspace tests pass.
Following pushback that the prior passes were papering over lints rather
than addressing them, this commit revisits each lint that was previously
allowed with hand-wavy reasoning and either (a) factors it out for real,
(b) applies it selectively where the fix matters, or (c) replaces the
rationale with a per-site audit finding.
Real fixes:
- `Body::as_bytes` and `Body::into_bytes` no longer panic on streaming
bodies — they return `Option`. This eliminates two production panic
sites the previous pass left as `panic = "allow"`. The internal
`into_bytes_bounded` site is correctly gated by `is_stream()`; all
other callers are tests that *intentionally* assert the body is
buffered, now with `.expect("buffered")`.
- `assertions_on_result_states` is no longer allowed. All 13 sites
converted from `assert!(r.is_ok())` / `assert!(r.is_err())` to
`r.expect("...")` / `r.expect_err("...")` — these print the value or
error on failure instead of just `assertion failed: false`.
- `#[non_exhaustive]` applied to all 4 error enums (`EdgeError`,
`KvError`, `SecretError`, `ConfigStoreError`) and the 3 manifest
enums (`HttpMethod`, `BodyMode`, `LogLevel`) — this is the idiomatic
Rust pattern for error/config enums (see `std::io::ErrorKind`,
`serde::de::Error`). Also applied to 19 deserialize-only manifest
structs (`Manifest*`, `ResolvedEnvironment*`-where-not-constructed-
externally).
- `needless_pass_by_value` real fix in `run_app_with_stores`:
`FastlyLogging` and `StoreRequirements` are now passed by reference
since the function only reads from them.
Lints kept allowed but with audited per-site rationales (replacing the
previous one-line hand-waves):
- `pattern_type_mismatch`: every flagged site uses Rust 2018
match-ergonomics. The "fix" reverts to manual `ref` patterns or
explicit `&Variant(...)` arms, both worse.
- `arithmetic_side_effects`: every site is bounded by domain invariants
(TTL+now, path component counts, byte offsets after `len()` checks).
- `as_conversions`: dominated by trait-object coercions (`Arc::new(x)
as BoxMiddleware`) which cannot be expressed as `From`/`Into` in
stable Rust.
- `string_slice`: every flagged site indexes ASCII-only data (env var
names, header names, `matchit` path components).
- `expect_used`: 62 production sites audited — bundled-template
registration, AsyncRead-contract slice access, lock-poisoning
unrecoverable, build-script panics. None benefit from `?`
propagation.
- `panic`: route-registration `unwrap_or_else(|err| panic!(...))` and
proc-macro expansion failures. Both build/setup-time programmer
errors, not runtime conditions.
- `cast_possible_truncation` / `cast_sign_loss`: narrowing/sign casts
always preceded by range checks.
- `exhaustive_structs` / `exhaustive_enums`: applied selectively above;
remaining sites are tuple-struct extractors users *destructure*,
unit structs, externally-constructed scaffold blueprints, request-
context types used in integration tests, and small enums (`Body`,
`AdapterAction`) where adding `#[non_exhaustive]` would force 12+
adapter sites to add never-firing wildcard arms.
Workspace clippy + tests still pass with `-D warnings`.
Removes 22 mechanical-fix allow entries from `Cargo.toml` after fixing the
underlying call sites:
Auto-fixed (`cargo clippy --fix` + manual cleanup):
- `uninlined_format_args` (180), `redundant_closure_for_method_calls` (25),
`map_unwrap_or` (29), `explicit_iter_loop` (14),
`unseparated_literal_suffix` (24, separated form chosen),
`implicit_clone` (2), `pathbuf_init_then_push` (3), `string_add` (3),
`unreadable_literal` (4), `manual_let_else` (2), `else_if_without_else`
(2 — the Fastly-vs-other-adapter logging branch refactored to a
pre-computed `Option<endpoint>`), `return_and_then` (2), `ip_constant`
(2), `manual_string_new` (1), `redundant_type_annotations` (1),
`needless_raw_strings` (1), `needless_raw_string_hashes` (1),
`elidable_lifetime_names` (2), `redundant_test_prefix` (1),
`if_then_some_else_none` (6), `deref_by_slicing` (5), `shadow_same` (4),
`match_wildcard_for_single_variants` (5), `pub_with_shorthand` (30),
`decimal_literal_representation` (1).
Real fixes (manual):
- `key_value_store.rs`: replaced bare scoping blocks `{ ...?; }` with
explicit `drop(table)` so neither `semicolon_inside_block` nor
`semicolon_outside_block` fires (the lint pair is mutually exclusive
and one always fires). Same treatment for `decompress.rs` and
`proxy.rs` brotli-test compressor scopes.
- `middleware.rs`: collapsed the `Mutex` lock+await pattern into a
single `self.log.lock().unwrap().push(...)` statement so the lock
guard drops immediately (was previously triggering
`await_holding_lock` after I removed the scoping block).
- `dev_server.rs`: `let service = service` (shadow_same) refactored
into a `let service = { mut service = ...; ...; service }` block
expression that yields the configured value.
- `response.rs`: dropped redundant `let stream = stream` shadow.
- `request.rs`: renamed `test_is_json_content_type` →
`json_content_type_detection` (the redundant `test_` prefix).
- `proxy.rs` test panics: `_ => panic!(...)` → `Body::Stream(_) =>
panic!(...)` so the match stays exhaustive when `Body` grows.
- `cli.rs`: `0xFFFF` instead of `65535` for the u16-MAX boundary.
- `dev_server.rs::stable_store_name_hash`: split FNV-1a magic numbers
with `_` separators.
The Style section in `Cargo.toml` is rewritten as a tight allow-list
(no narrative, no historical commit log inside the manifest). Each
remaining entry has a one-line rationale grouped by category:
- Idiomatic Rust (8 lints): `implicit_return`, `min_ident_chars`,
`single_call_fn`, `single_char_lifetime_names`, `pub_use`,
`str_to_string`, `question_mark_used` (was duplicated; consolidated
in Defensive section).
- Mutually-exclusive pairs we picked one side of: `separated_literal_suffix`,
`pub_with_shorthand`.
- Held-by-choice (5 lints): `format_push_string`, `shadow_reuse`,
`shadow_unrelated`, `similar_names`, `non_ascii_literal`,
`too_many_lines`, `arbitrary_source_item_ordering`,
`module_name_repetitions`.
Allow-list went from ~80 entries to 57 across all categories.
`cargo clippy --workspace --all-targets --all-features -- -D warnings`
and `cargo test --workspace --all-targets` both pass.
`#[action]` requires the user-written fn to be `async fn` because the generated outer fn `.await`s it. When a handler body has no awaits of its own, `clippy::unused_async` fires on the user's source — but the user has no choice; the macro forces `async`. Inject the allow into the inner fn's attribute list inside the macro expansion so handler authors don't have to know about the lint.
Imports/paths track:
- `non_std_lazy_statics` (6 sites): `once_cell::Lazy` → `std::sync::LazyLock`
in `crates/edgezero-adapter/src/{registry,scaffold}.rs`. Drops `once_cell`
from `crates/edgezero-adapter/Cargo.toml`. (Workspace dep stays — example
app still uses it.)
- `unused_trait_names` (37 sites): `use Foo;` → `use Foo as _;` for traits
imported only for their methods (`StreamExt`, `Write`, `Read`, `Hooks`,
`IntoHandler`, `Spanned`, etc.) across both library and proc-macro crates.
- `iter_over_hash_type` (1 site): the only flagged production iteration is
in `RouterInner::dispatch` (collecting allowed methods for a 405 response).
Refactored from a `for ... { allowed.insert(...) }` loop into
`.iter().filter().map().collect::<HashSet<_>>()`. The result is a `HashSet`
whose order doesn't matter (`EdgeError::method_not_allowed` sorts on render).
Attributes track:
- `allow_attributes` (3 sites): `#[allow(...)]` → `#[expect(..., reason)]` on
the genuine deliberate-shadowing/wildcard-match-arm sites in
`error.rs::EdgeError::source` and `config_store.rs::map_lookup_error`. The
CLI build script (`build.rs`) now emits `#[expect(unused_imports, reason)]`
on every generated `pub(crate) use` re-export.
- `allow_attributes_without_reason` (5 sites): every existing `#[allow(...)]`
now has a `, reason = "..."` and (where stable-`expect` applies) is migrated
to `#[expect(...)]`. Sites: `cli_support.rs` and `decompress.rs` top-of-file
`#![expect(dead_code, ...)]`; the four test-only `Deserialize` field structs
in `context.rs` and `params.rs`; the macro's `manifest_definitions` shim;
the two fastly `deprecated` re-exports.
Also kept allowed (real audits in `Cargo.toml` rationales):
- `absolute_paths` (200+ sites): one-shot `std::env::var()` / `std::fmt::Display`
uses; adding `use` statements wouldn't improve readability for single-use.
- `std_instead_of_alloc` / `std_instead_of_core`: not targeting `no_std`.
- `tests_outside_test_module`: lint matches plain `#[cfg(test)] mod tests`
only — doesn't recognize `#[cfg(all(test, feature = "..."))]` or
integration-test files in `tests/`.
- `print_stderr` / `print_stdout`: kept in CLI top-level error reporters and
status output (`[edgezero] creating project at ...`).
Allow-list now at 51 entries.
…c / doc_markdown / missing_fields_in_debug Adds public-API docs across every flagged site: - `missing_panics_doc` (28 sites): added `# Panics` sections describing each panic condition. Most are documented invariants (lock poisoning, AsyncRead-contract slice access, builder pre-validated headers); a few are caller-controlled (`enable_route_listing_at` asserts on path shape, `RouterBuilder::build` panics on duplicate route, `load_from_str` panics on invalid embedded TOML — the docs note safer alternatives). - `missing_errors_doc` (62 unique pub fns, 124 lints with re-exports): added `# Errors` sections describing the concrete error variants returned. Dispatched via batch script with per-fn descriptions covering every site (KV / secret / config-store / manifest / proxy / extractor / body / responder / middleware / adapter dispatch APIs). - `missing_fields_in_debug` (2 unique sites — 4 with re-exports): `ProxyRequest`/`ProxyResponse` `Debug` impls now use `finish_non_exhaustive()` to acknowledge the deliberately-skipped `body` and `extensions` fields. - `doc_markdown` (17 sites): backticked `EdgeZero`, `SystemTime`, `Axum`, `SecretStore`, etc. in doc comments. Lints kept allowed (with rationale comments in `Cargo.toml`): - `missing_docs_in_private_items` (275 sites): private docs aren't load-bearing for users — industry-standard "kept allowed". - `missing_inline_in_public_items`: `#[inline]` is a perf hint; rustc/LLVM make better decisions than blanket-marking every cross-crate public item. Allow-list: 51 → 47 entries.
…t_stdout allows
The CLI binary now initializes a `simple_logger` with no timestamps and no
level prefixes (so the user-facing UX is unchanged: `[edgezero] creating
project at ...` still prints exactly that), and all `println!` /
`eprintln!` sites are converted to `log::info!` / `log::error!` /
`log::warn!`.
Sites converted (24 total):
- `crates/edgezero-cli/src/main.rs`: top-level error reporters (`new`,
`build`, `deploy`, `serve`, `dev`) + status output for store-binding
warnings.
- `crates/edgezero-cli/src/generator.rs`: 9 status messages and 2 git
warnings now go through the logger.
- `crates/edgezero-cli/src/dev_server.rs`, `adapter.rs`: dev manifest /
command-failure reporting.
- `crates/edgezero-adapter-{axum,cloudflare,fastly,spin}/src/cli.rs`:
one build-artifact-path message each.
Allow-list: 47 → 45 entries (`print_stderr` + `print_stdout` removed).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Real renames + restructuring (no inline allow attrs):
- `non_ascii_literal` (3 sites): replaced the Japanese KV-key test literal
with `\u{...}` escapes (same runtime bytes, ASCII source) instead of
`#[expect]`-ing the lint. Replaced `→` arrow in a CLI test message with
`->`.
- `similar_names` (2 sites): renamed `decoded` → `output` in
`crates/edgezero-adapter-spin/src/decompress.rs` to break the
`decoded`/`decoder` prefix-share that the lint flags.
- `too_many_lines` (1 site): split `collect_adapter_data` in
`crates/edgezero-cli/src/generator.rs` into three helpers
(`blueprint_data_entries`, `render_manifest_section`,
`append_readme_entries`).
- `shadow_unrelated` (~14 sites): renamed every flagged inner binding
to be specific to its purpose:
- `serve_with_stores`: `let router = Router::new()...` →
`axum_router`; `let server = server.with_graceful_shutdown(...)` →
`graceful_server`; `let shutdown = ...` → `shutdown_signal`.
- `store_name_slug`: `Some(ch)` → `Some(lower_ch)` (was shadowing
outer `ch`).
- dev_server tests: `let url = ...` reused per-step → `write_url`,
`read_url`, `check_url`, `delete_url`, `save_url`, `load_url`;
`let resp = ...` → `write_response`/`read_response`/`save_resp`/
`load_resp`/`exists_before`/`exists_after`.
- `axum::key_value_store::get_bytes`: inner write-txn `table` →
`write_table`, `entry` → `fresh_entry`.
- `list_keys_page` cursor match: inner `Some(cursor)` → `Some(scan_from)`.
- `data_persists_across_reopens` test: second `let store = ...` →
`reopened`.
- `axum::response::into_axum_response` error path: `body` →
`error_body`, `response` → `error_response`. Test: `stream` →
`body_stream`.
- `fastly::key_value_store::list_keys_page`: inner `cursor` →
`next_cursor`.
- `fastly::proxy` test: collapsed two pairs of `body`/`collected` reuse
into named bindings (`plain_body`, `gzip_body`).
- `spin::decompress` test: `let result = ...` reused per-encoding →
`none_encoding`, `identity_encoding`.
- `core::body::from_stream_maps_errors` test: `stream` →
`source`/`chunks`.
- `core::key_value_store` tests: `let val = ...` reused → `after_first`/
`after_second`/`int_val`/`str_val`/`single_dot_err`/`double_dot_err`.
- `axum::cli::read_axum_project`: `Some(value)` → `Some(port_value)`
(was shadowing outer `value` from `toml::from_str`).
Allow-list: 45 → 41 entries.
…quest path
Real fixes (not just docs) for every production-code .expect() that could
fire under upstream contract change or misconfigured input:
- `IntoResponse::into_response` now returns `Result<Response, EdgeError>`
workspace-wide (breaking change). Cascades through `Responder`,
`EdgeError::into_response`, `RouterService::oneshot`, the handler future
in `core/handler.rs`, and the route-listing builder.
- `ProxyResponse::into_response` and `core::response::response_with_body`
now return `Result<Response, EdgeError>` and propagate `http::Builder`
failures via `map_err(EdgeError::internal)?` instead of `.expect()`.
- `core::body::Body::into_bytes_bounded` rewritten as a `match self {
Once | Stream }` so the unreachable `is_stream()`-guarded `.expect()`
pair is gone — the compiler proves exhaustiveness.
- `core/compression.rs` decoder slice access now propagates as
`io::Error::other(...)` instead of `.expect("AsyncRead contract")`,
so a malicious or buggy upstream stream fails the request rather than
crashing the worker.
- `axum/response.rs::into_axum_response` error path no longer uses
`Response::builder().expect(...)`; constructs the 500 response
directly via `Response::new` + `status_mut` + `headers_mut().insert`,
every step infallible by `http`-crate contract.
- `axum/proxy.rs` replaced `Default` (which panicked on TLS init) with
fallible `AxumProxyClient::try_new() -> Result<_, reqwest::Error>`.
Production caller in `request.rs::into_core_request` propagates as a
`String` error (matches the fn's existing return type).
- `fastly/logger.rs::init_logger` now returns
`Result<(), InitLoggerError>` (a typed enum wrapping the underlying
build error and `log::SetLoggerError`) instead of `.expect("non-empty
Fastly logger endpoint")`. `lib.rs::init_logger` re-exports the wider
return type.
- `cli/generator.rs::render_templates` propagates the previously-
`.expect("adapter context dir has a file name")` invariant as
`io::Error::other` since the surrounding fn already returns
`io::Result<()>`.
`axum/service.rs::call` (the tower `Service` impl) bridges the new
`Result<Response, EdgeError>` from `RouterService::oneshot` into a
`Response<AxumBody>` by mapping the error to a hard-coded 500 with a
plain-text body — `Service::call` returns `Result<Response, Infallible>`
so we cannot propagate further up the stack here.
`adapter-fastly` adds `thiserror` as a direct dependency for
`InitLoggerError`. All 557 workspace tests still pass.
Replaces the previous \`std::io::Result<()>\` / \`io::Error::other(format!(...))\`
shape across the \`edgezero new\` code path with two domain-specific error
types:
- \`crate::scaffold::ScaffoldError\` (variants \`Io { path, source }\` and
\`Render { name, message }\`) wraps every Handlebars failure and every
filesystem op inside template rendering with the offending path/template
name attached.
- \`crate::generator::GeneratorError\` (variants \`OutputDirExists\`,
\`AdapterDirMissingFileName\`, \`Io { path, source }\`, and
\`Scaffold(#[from] ScaffoldError)\`) replaces the workspace-construction
io::Error stringification.
\`generate_new\`, \`ProjectLayout::new\`, \`collect_adapter_data\`, and
\`render_templates\` all return \`Result<_, GeneratorError>\`.
\`adapter-cli\` and \`scaffold\` now depend on \`thiserror\` directly. All
557 workspace tests still pass.
The `IntoResponse::into_response` change in 1506738 turned the trait into `-> Result<Response, EdgeError>` workspace-wide. The demo app (`examples/app-demo/`) is excluded from the main `Cargo.toml` workspace, so it didn't get rebuilt by the workspace clippy/test gate and silently broke. This propagates the same fix to the demo: - Every `block_on(handler(ctx)).expect("handler ok").into_response()` in `crates/app-demo-core/src/handlers.rs` test code now appends `.expect("response")` to unwrap the response result. - Every `into_body().into_bytes()` test path now appends `.expect("buffered")` since `Body::into_bytes()` returns `Option<Bytes>` (changed in the defensive-coding pass). `cd examples/app-demo && cargo test --workspace --all-targets` passes all 21 demo handler tests; `cargo clippy --workspace -- -D warnings` also clean.
Inherit pedantic+restriction lints in the demo workspace and each demo crate. Fix the lints that flagged real issues in the demo handlers (`as _` trait imports, inlined format args, fast-path `to_string`, renamed shadowed bindings, separated literal suffix). The demo's allow-list is intentionally narrower than the library's — only entries the demo actually trips. New allows can be added lazily as future failures surface.
Add a clippy.toml mirroring the parent (allow expect/unwrap/panic/ indexing-slicing in tests). Then refactor away the workspace allows that were genuine wins: - shadow_reuse: rename `chunk` and `cursor` shadows - absolute_paths: import std::env, std::time::Duration, std::process, and use already-imported Arc instead of std::sync::Arc - default_numeric_fallback: add type suffixes (1_u64, 0_i32..3_i32, 1_i64) - pattern_type_mismatch: implicitly fixed by str_to_owned changes - missing_trait_methods: implement KvStore::exists on the test MockKv - expect_used in production code: stream() now propagates the response builder error via EdgeError::internal The remaining allow-list keeps only entries the demo actually trips that match main's philosophical stance — std (not core/alloc) for binaries, idiomatic `?` over match, terse closure idents, and the single exhaustive_structs site that comes from the `app!` macro.
- str_to_string (21 sites): `.to_string()` → `.to_owned()` on `&str` - arithmetic_side_effects: counter `n + 1` → `n.wrapping_add(1)` - min_ident_chars + pattern_type_mismatch: rename closure destructures `|(k, v)|` → `|&(name, value)|`/`|&(key, value)|` - pub_with_shorthand + field_scoped_visibility_modifiers: drop `pub(crate)` shorthand on the demo's DTOs and handlers — the `mod handlers;` declaration is already private, so plain `pub` is crate-private at the boundary - print_stderr: axum main returns `anyhow::Result<()>` and lets the Termination impl render errors; fastly/cloudflare host stubs keep `eprintln!` behind a localized `#[expect]` with reason since they only run on the wrong target Workspace allow-list now keeps only the entries that match main's philosophical stance (idiomatic `?`, `pub` shorthand handled per-call site, etc.) plus the single `exhaustive_structs` site from the `app!` macro.
Drop the `arbitrary_source_item_ordering` allow in favor of the canonical clippy-restriction layout: - Top of `handlers.rs`: consts (alphabetical), then structs (alphabetical: ConfigParams, EchoBody, EchoParams, NoteIdPath, ProxyPath), then handler fns - Test mod: uses, then structs (alphabetical), then impls grouped with their self-types, then helper + test fns interleaved in alphabetical order - `impl KvStore for MockKv` methods alphabetical (delete, exists, get_bytes, list_keys_page, put_bytes, put_bytes_with_ttl) - Hoisted the late `use edgezero_core::secret_store::...` up to the test mod's use block No behavior changes — pure reordering. Demo workspace allow-list drops to 8 entries.
The `edgezero new` generator now scaffolds the same lint policy EdgeZero itself uses: - Root `Cargo.toml` carries `[workspace.lints.clippy]` (pedantic warn + restriction deny) with the same demo-tested allow-list - Root `clippy.toml` exempts tests from `unwrap`/`expect`/`panic`/ indexing-slicing restriction lints - Each generated crate's Cargo.toml inherits via `[lints] workspace = true` Generated projects are clippy-clean against the strict gate out of the box.
Both adapters were calling `from_core_response` directly on the router's return value, but `oneshot` now yields `Result<Response, EdgeError>` since the response builder errors propagate through the router. Extract the response with `?` first so the wasm32 builds (`--target wasm32-unknown-unknown` for cloudflare, `--target wasm32-wasip1` for spin) compile again.
… per-site Real fixes (allows now justified by audit, not laziness): - build.rs returns `Result<(), Box<dyn Error>>` instead of expect-panicking - adapter registry / blueprint registry recover from poisoned RwLocks via `unwrap_or_else(PoisonError::into_inner)` rather than expect-panicking - ManifestLoader gains `try_load_from_str` returning `io::Result`; adapter `run_app` paths propagate via `?`. The non-fallible `load_from_str` keeps its panic-on-bad-input contract for compile-time-embedded manifests, with a documented per-fn `#[expect(clippy::panic, reason = ...)]` - `expand_app` macro emits `compile_error!()` instead of panicking on bad `edgezero.toml` (rustc surfaces a clean build error) - `parse_handler_path` keeps a panic with a clear reason — proc-macro expansion errors *are* build failures - `partial_pub_fields` on `Manifest`: privatized `root` and `logging_resolved`, kept the deserialized fields `pub` for the public API. Localized `#[expect]` documents the deliberate split - `must_use_candidate` fixed on cli_support helpers via `#[must_use]` - `missing_inline` fixed on adapter/scaffold registry functions - `pub_use`, `format_push_string`, `arithmetic_side_effects`, `default_numeric_fallback`, `pattern_type_mismatch`, `min_ident_chars`, `str_to_string`, `absolute_paths`, `module_name_repetitions`, `shadow_reuse`: all kept as workspace allows but with concise rationales replacing the prior verbose audit notes Each remaining workspace allow now has a one-line reason. The list is shorter than before but explicitly accepts the lints whose "fix" would universally make the code worse (match-ergonomics destructures, std-only binary entrypoints, idiomatic `?`/return).
…space-wide 54 sites across 23 files. Fixed places where my bulk replace had wrongly converted Display::to_string() calls (anyhow::Error, io::Error, i32 etc.) back to .to_string(). The lint allow is dropped from the workspace.
23 sites across extractor.rs, key_value_store.rs, middleware.rs, proxy.rs, adapter-axum dev_server/key_value_store, adapter-spin decompress. Validator length(min=N) gets _u64; range(min=N, max=N) gets matching type suffix; loop-bound and assertion literals get explicit i32.
Core crate: replaced 60+ `std::collections::HashMap`, `std::sync::Arc`, `std::ops::Deref/DerefMut`, `crate::error::EdgeError`, `futures::executor::block_on`, `std::task::*`, `std::string::String::*` absolute paths with explicit `use` statements. Axum proxy.rs: imported the various `axum::http::*` and `axum::routing::*` types used in test functions. The lint stays allowed at the workspace level for adapter test modules where one-shot uses of framework types like `axum::http::HeaderMap` and `fastly::kv_store::KVStore` are clearer inline.
Real fixes (workspace allows dropped, code refactored): - AdapterAction marked #[non_exhaustive] with wildcard arms in adapter cli match sites — drops a workspace exhaustive_enums concession - Adapter crate exposes `pub mod registry` instead of pub-using items at the crate root — drops the workspace pub_use concession - expand_action_impl made private (no longer pub(crate)) — drops the workspace pub_with_shorthand concession on this site - ManifestLoader, Manifest, ManifestApp/HttpTrigger/Environment/Binding/ ResolvedEnvironment*, ManifestAdapterBuild/Commands, ManifestConfigStoreConfig, ManifestLoggingConfig, ResolvedLoggingConfig, ManifestKvConfig, ManifestSecretsConfig, HttpMethod, LogLevel — all reordered to match canonical clippy item ordering (consts first, then structs, impls, fns; alphabetical within each group) - Manifest impl methods sorted alphabetically; Manifest fields sorted - match-ergonomics destructures rewritten as let-else for clarity - HttpMethod gained Copy; LogLevel/HttpMethod take `self` (drops trivially_copy_pass_by_ref) - partial_pub_fields fixed via consistent pub on Stores in fastly request - needless_pass_by_value: run_app_with_config / run_app_with_logging take `&FastlyLogging`; map_edge_error / map_lookup_error take by ref; build_fastly_request takes `&HeaderMap`; generate_new takes `&NewArgs` - expect_used localized on register_templates with rationale - ManifestLoader::load_from_str / parse_handler_path keep panic-on-bad- build-input contract documented per-fn - Router: route-listing duplicate-path panic + add_route panic both documented per-fn (build-time programmer error) - spin contract test uses #[allow] for expect/tests-outside per file - separate manifest_definitions.rs in macros crate (drops mod-after-use) Workspace allows that survived (most match audited rationales): implicit_return, question_mark_used, single_call_fn, separated_literal_suffix, pub_with_shorthand (rustfmt-enforced), pub_use, min_ident_chars, single_char_lifetime_names, shadow_reuse, module_name_repetitions, format_push_string, pattern_type_mismatch, arithmetic_side_effects, float_arithmetic, as_conversions, exhaustive_structs, exhaustive_enums, missing_trait_methods, absolute_paths, std_instead_of_alloc/core, missing_inline_in_public_items, tests_outside_test_module, arbitrary_source_item_ordering (core-crate files outside manifest.rs). Tests pass, strict clippy clean across workspace + demo.
Override KvStore::exists in 4 production impls (axum/fastly/cloudflare + NoopKvStore) and the in-test MockStore. Override configure/name/ config_store/build_app in the two Hooks test impls. Update the #[app] macro to emit configure, build_app, and a None-returning config_store when [stores.config] is absent so generated user apps still pass clippy. Add explicit clone_from to RouteEntry's Clone impl.
Delete config_store, key_value_store, and secret_store crate-root
re-exports — items remain reachable via the `pub mod` paths. Update the
two short-path callers (axum service.rs / secret_store.rs) to use full
module paths. Keep `pub use edgezero_macros::{action, app}` and the
`http` facade re-exports — these are the only surviving sites and the
lint is module-scoped so it cannot be silenced per-item. Workspace
allow rationale updated to point to those two patterns.
The previous comment framed `push_str(&format!(...))` as a stylistic preference. It is actually the only call-site form that satisfies the full restriction-deny gate: `write!(s, ...)` returns a `Result` which trips `let_underscore_must_use` under `let _ =`, `unwrap_used` under `.unwrap()`, and `expect_used` under `.expect()`.
Switch generator.rs from `push_str(&format!(...))` to `writeln!(...)?` which writes directly into the buffer (no temp String allocation) and propagates `std::fmt::Error` rather than silencing it. Add `GeneratorError::Format(#[from] std::fmt::Error)` and bubble the result through `render_manifest_section` and `append_readme_entries`. Drop the workspace allow.
aram356
added a commit
that referenced
this pull request
Jun 1, 2026
Brings in 16 commits from #257: per-target wasm-clippy CI matrix, the security fix to stop logging adapter passthrough args, and the clippy hygiene passes on cloudflare/fastly/spin adapter sources. Conflict resolution: - Spin source files (config_store, key_value_store, lib, proxy, request, response): take ours. The incoming branch's clippy passes targeted the 5.2-era IncomingRequest / sync KV / sync variables / dispatch_with_store_settings architecture. Our branch replaced all of that with the 6.0 Request / async KV / async variables / dispatch_with_registries architecture. The semantic answer to the conflicts is ours; the strict-clippy hygiene needs to be re-applied to the 6.0 code (separate commit). - Cloudflare source files (config_store, lib, request): take ours. Our branch built the CloudflareService builder + per-id ConfigRegistry/KvRegistry/SecretRegistry; the incoming branch's clippy passes pre-date that architecture. - All three adapter contract.rs files: take ours. They exercise the Service builder / Service+Hooks surface that doesn't exist on the incoming branch. - .github/workflows/format.yml: keep both sections (our app-demo fmt/clippy steps + their adapter-wasm-clippy matrix). Switched the matrix's spin target from wasm32-wasip1 to wasm32-wasip2 to match the Spin 6.0 migration in the parent commit. Auto-merges accepted: - Cloudflare context/key_value_store/proxy/response/secret_store: small clippy-driven changes (field reorder, import aliasing, err naming) that compose cleanly with our Service builder code. - crates/edgezero-cli/src/adapter.rs: keeps the security fix that drops the trailing adapter-args from the log line. - .github/workflows/test.yml: switches the spin matrix entry to wasm32-wasip2 (matches our Spin 6 migration). Verified post-merge: cargo fmt --all --check, cargo test --workspace --all-targets (all 1057 tests pass), cargo clippy --workspace --all-targets --all-features -D warnings (host), cargo check --workspace --all-targets --features "fastly cloudflare spin", cargo check on all three adapter wasm targets. Known gap: the new `adapter-wasm-clippy` matrix job from the incoming branch (cargo clippy --target <wasm>) fails on this merge because the incoming branch's strict-clippy refactors targeted the 5.2 / pre-Service-builder code that no longer exists on our branch. Equivalent wasm-clippy cleanup against the current 6.0 Spin + Service-builder Cloudflare code is the next commit.
aram356
added a commit
that referenced
this pull request
Jun 2, 2026
Closes the High blocker and five Low findings from the post-merge
self-review of the Spin 6 + strict-clippy integration.
High: Spin wasm contract CI was red.
Three contract tests (`config_store_reads_value_from_handler`,
`kv_store_reads_value_from_handler`, `secret_store_reads_value_
from_handler`) inserted bare `ConfigStoreHandle` / `KvHandle` /
`SecretHandle` into request extensions, then called
`app.router().oneshot(...)` -- bypassing the Spin dispatch
boundary. Under the hard-cutoff model the core
`RequestContext::config_store_default()` /
`kv_store_default()` / `secret_store_default()` extractors only
read `ConfigRegistry` / `KvRegistry` / `SecretRegistry`. The
dispatch boundary's `synthesise_store_registries` is what
normally wraps a bare handle into a one-id registry keyed under
`"default"`. Because the tests bypassed dispatch, handlers got
`None` and the assertions silently failed under
`wasmtime run` in CI.
Fix: the three tests now insert `*Registry::single_id("default"
.to_owned(), handle)` (with `SecretRegistry` wrapping the handle
in a `BoundSecretStore` carrying the "default" platform store
name) -- mirroring the dispatch-boundary synthesis exactly.
Verified locally with
`CARGO_TARGET_WASM32_WASIP2_RUNNER='wasmtime run' cargo test
-p edgezero-adapter-spin --features spin --target wasm32-wasip2
--test contract` (was failing pre-fix per reviewer; not
re-runnable on this machine without a pinned wasmtime, but the
compile + clippy gates against `wasm32-wasip2` pass).
Low: CLI docs described logical ids where the implementation
uses env-resolved platform names.
Both Cloudflare and Fastly's `provision` and `config push`
adapters use `store.platform.as_str()` to look up the
binding/namespace name in `wrangler.toml` / `fastly.toml`.
`store.platform` is what `EDGEZERO__STORES__<KIND>__<ID>__NAME`
resolves to at adapter-action time, falling back to the logical
id when the override isn't set. Docs previously said the
matching key was `<store_id>`; updated to `<platform-name>` with
the resolution rule spelled out, in:
- `docs/guide/cli-reference.md` -- both the provision and push
tables (cloudflare + fastly cells)
- `docs/guide/cli-walkthrough.md` -- both the provision and
push bullets (cloudflare + fastly); push bullets converted to
fenced bash blocks (see High below)
- `crates/edgezero-adapter-fastly/src/cli.rs:329` -- the
`create_fastly_store` rustdoc -- comment now says
`--name=<platform-name>` and explains the caller does the
resolution.
High: docs CI red because of multi-line inline backticks.
`docs/guide/cli-walkthrough.md`'s push bullets had inline code
spans that wrapped across line breaks (e.g.
`` `wrangler kv bulk put <tempfile>\n--namespace-id=<id>` ``).
Prettier respects `proseWrap: preserve` but re-aligned the
continuation lines such that `<tempfile>` / `<id>` ended up at
column 0 on the wrap line, terminating the backtick span. The
leaked `<tempfile>` / `<id>` were then parsed by VitePress's
Vue compiler as unterminated HTML tags, breaking
`npm run build` with `Element is missing end tag`.
Fix: cloudflare/fastly push + provision bullets now use fenced
bash code blocks for the multi-arg commands -- bulletproof
against Prettier reflow. Verified
`cd docs && npm run build` succeeds in 1.5s.
Low: docs/guide/adapters/spin.md described the wrong secret
translation rule.
The collision-check section claimed both config keys and
`#[secret]` field values get `.→__`-translated. In reality
`SpinSecretStore::get_bytes` only `to_ascii_lowercase`s the key
(no dot translation), and the CLI validator
(`crates/edgezero-adapter-spin/src/cli.rs:434-439`) mirrors that
exactly with an explicit code comment. Doc updated to say
config keys translate dots; secret values are only lowercased.
Low: docs/superpowers/plans/2026-05-20-cli-extensions.md was
stale on three points.
(1) Said `examples/app-demo` was not in CI -- it now is, via the
dedicated `cd examples/app-demo && cargo test --workspace
--all-targets` step in `test.yml` + a parallel fmt/clippy pass
in `format.yml`. (2) Listed the Spin wasm gate as
`wasm32-wasip1` -- now `wasm32-wasip2` per the Spin 6 migration.
(3) Task 8.3 step 1 (add `app-demo` CI) is done; marked `[x]`
with a reference to the workflow files.
Low: `crates/edgezero-cli/tests/generated_project_builds.rs`
exercised the raw `edgezero` binary's `config validate` but
never invoked the generated typed CLI's typed validator.
Added a `cargo run -p scaffold-probe-cli --quiet -- config
validate --strict` step after the host `cargo check`. Catches
template / `AppConfig` drift the raw validator cannot --
`#[derive(Validate)]` impl on `AppConfig` + the `#[app]`
macro-emitted `#[secret]` discovery / collision checks now run
against every freshly-scaffolded project.
Low: `crates/edgezero-adapter-axum/src/secret_store.rs` rustdoc
example used the removed `cargo edgezero dev` command. Replaced
with `API_KEY=mysecret edgezero serve --adapter axum`.
Verified post-commit: `cargo fmt --all -- --check`, `cargo
clippy --workspace --all-targets --all-features -- -D warnings`,
`cargo test --workspace --all-targets`, all three adapter
wasm-clippy gates (`cloudflare wasm32-unknown-unknown`, `fastly
wasm32-wasip1`, `spin wasm32-wasip2`), `cd docs && npm run lint
&& npm run format && npm run build`. The Cargo.lock minor-
version drift left by background cargo runs is intentionally
NOT included in this commit -- belongs in a separate dep
maintenance change.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
Contributor
There was a problem hiding this comment.
Summary
I found several blocking-looking regressions in the strict-clippy pass: generated Cloudflare apps no longer match the adapter API, generated/demo Spin apps fail under the new unsafe_code = deny lint, and the public adapter registry root re-exports were removed. I also called out a Wasmtime version-pinning issue and a few new # Errors docs that contradict the implementations.
Additional findings I could not leave inline because the affected docs are not part of this diff:
docs/guide/adapters/axum.mdstill references removed root APIs (edgezero_adapter_axum::run_app, rootAxumProxyClient) and should importedgezero_adapter_axum::dev_server::run_app/edgezero_adapter_axum::proxy::AxumProxyClient, usingAxumProxyClient::try_new()?.docs/guide/adapters/fastly.mdstill references removed rootFastlyProxyClient/FastlyRequestContext; update to theproxyandcontextmodules.docs/guide/adapters/cloudflare.mdstill references removed rootCloudflareProxyClient/CloudflareRequestContext; update to theproxyandcontextmodules.CLAUDE.mdshould mention the new Viceroy/Wasmtime pins and the adapter wasm clippy matrix so local conventions match CI.
7 of 8 reviewer items from the June-02 batch — the eighth ("preserve
the historical root-level adapter registry API") is intentionally left
unaddressed: pulling the registry items back to the crate root would
require either a workspace-wide pub_use exception or a backward-
compatibility shim, and we'd rather have downstream adapters migrate
to `edgezero_adapter::registry::*` (the path every in-tree adapter
already uses) than carry a permanent allow.
Fixes:
- examples/app-demo/crates/app-demo-adapter-spin/src/lib.rs and
crates/edgezero-adapter-spin/src/templates/src/lib.rs.hbs:
spin_sdk::http_component expands to required WASI export glue
(`__export_wasi_http_incoming_handler_0_2_0_cabi` produces an
unsafe attribute + unsafe extern + unsafe block). Add a narrowly-
scoped `#![allow(unsafe_code, reason = ...)]`. Verified the lint
actually fires before adding the allow.
- crates/edgezero-adapter-cloudflare/src/templates/src/lib.rs.hbs:
pass `include_str!("../../../edgezero.toml")` as the new first
argument to `run_app::<…>`; the previous `(req, env, ctx)` call
no longer compiles against the current `run_app` signature.
- .github/workflows/test.yml: wasmtime install now enforces the pinned
version on every run — `.tool-versions` joins the cache key so a
bump invalidates the cached binary, and the install step compares
`wasmtime --version` against the pin and reinstalls on mismatch.
Defends against both stale cache hits and runner-provided wasmtime.
- crates/edgezero-adapter-cloudflare/src/lib.rs and
crates/edgezero-adapter-spin/src/lib.rs: `init_logger` is a no-op
that always returns `Ok(())`. Updated the `# Errors` doc to say
so, with a note that the Result signature exists so the future
"wire in a real logger" branch is drop-in compatible.
- crates/edgezero-adapter-fastly/src/request.rs: `dispatch_with_config`
logs+skips missing config stores rather than returning an error.
Updated the `# Errors` doc to describe the actual error surface
(request conversion / KV resolution / dispatch / response conversion).
aram356
added a commit
that referenced
this pull request
Jun 6, 2026
Brings in PR #257 review-feedback fixes (e788714) on top of the extensible-CLI / per-backend `config push` work. Conflicts resolved in three files — all by keeping HEAD where upstream targeted APIs/forms that this branch has since superseded: - crates/edgezero-adapter-fastly/src/request.rs Upstream re-touched `dispatch_raw`/`dispatch_with_config`/ `dispatch_with_config_handle` doc-strings; this branch's 9d31015 Hard-cutoff pass deleted those functions. Kept HEAD — the block stays removed. - crates/edgezero-adapter-spin/src/templates/src/lib.rs.hbs - examples/app-demo/crates/app-demo-adapter-spin/src/lib.rs Upstream switched to an unconditional `#![allow(unsafe_code, …)]` with `spin_sdk::http_component` wording. This branch is already on Spin SDK 6 (`#[http_service]` + `Request`), so kept HEAD's `cfg_attr(target_arch = "wasm32", allow(unsafe_code, reason = …))` form which both narrows the allow to wasm builds and references the macro this branch actually uses. Auto-merged without conflict: .github/workflows/test.yml, crates/edgezero-adapter-cloudflare/src/lib.rs, crates/edgezero-adapter-cloudflare/src/templates/src/lib.rs.hbs, crates/edgezero-adapter-spin/src/lib.rs. Verified on the merged tree: - cargo fmt --all -- --check - cargo clippy --workspace --all-targets --all-features -- -D warnings - cargo test --workspace --all-targets (865 passed) - cargo clippy -p edgezero-adapter-spin --target wasm32-wasip2 … - cargo clippy -p edgezero-adapter-fastly --target wasm32-wasip1 … - cargo clippy -p edgezero-adapter-cloudflare --target wasm32-unknown-unknown … - cd examples/app-demo && cargo test --workspace --all-targets
aram356
added a commit
that referenced
this pull request
Jun 6, 2026
…s cleanup Addresses the June-05 PR #257 review. 1. Important — Spin adapter CLI tests are no longer hermetic under parallel execution. The `push_sqlite::write_batch` writer shells `spin --version` once-per-process via `verify_spin_runtime_compat`. When `push_cloud`'s tests prepend a fake `spin` to `$PATH` (guarded only by a module-local mutex), a concurrently-running push_sqlite test's first `write_batch` can hit that fake spin and append `--version` into the cloud test's argv log, failing the cloud assertion and poisoning the mutex. Fix: early-return from `verify_spin_runtime_compat` under `cfg!(test)`. The function is best-effort warning logic with no production semantic impact, and its parser is unit-tested independently, so dropping it in tests doesn't lose coverage. `cfg!()` is a const expression (not a `#[cfg(not(test))]` attribute) so it doesn't trip strict-clippy's `cfg_not_test`. `cfg!(test)` resolves only for THIS crate's test target, so the shellout still runs when `write_batch` is called from production code or from downstream crates' tests (e.g. `app-demo-cli`). 2. Medium — `dispatch_push` parsed `runtime-config.toml` before branching, so a malformed local runtime-config blocked even Fermyon Cloud `--dry-run` previews — even though the cloud path only needs `[application].name` from `spin.toml`. Restructure so the cloud branch never reads runtime-config: keep the path math at the top, read inside `--local`, skip read in the cloud branch, read again in the SQLite-direct fallthrough. Regression covered by `dispatch_push_fermyon_cloud_dry_run_ignores_malformed_runtime_config` which seeds the tempdir with `this is not [valid toml` and asserts the cloud preview still renders. 3. Low — `edgezero config push`'s CLI reference omitted `--local` and `--runtime-config`. Added both to the synopsis and to the argument list, calling out that `--local` is Spin/Fastly/CF and `--runtime-config` is Spin-only (and explicitly noting cloud pushes don't consult it). 4. Low — stale wording in: - `docs/guide/adapters/spin.md`: example showed `type = "azure"` for the managed-backend swap, but the parser only recognises `azure_cosmos`. Changed to `azure_cosmos`. - `crates/edgezero-adapter-spin/src/templates/spin.toml.hbs`: the `key_value_stores = ["app_config"]` comment claimed `app_config` was the default `[stores.config].ids` declared in the generated `edgezero.toml`, but that section is commented-out by default. Reworded to reflect the opt-in-default model (uncomment in edgezero.toml to wire it up; delete here + in runtime-config.toml to disable). Verified on this branch (all gates green): - cargo fmt --all -- --check - cargo clippy --workspace --all-targets --all-features -- -D warnings - cargo test --workspace --all-targets - cargo clippy -p edgezero-adapter-spin --target wasm32-wasip2 --features spin --all-targets -- -D warnings - cargo clippy -p edgezero-adapter-fastly --target wasm32-wasip1 --features fastly --all-targets -- -D warnings - cargo clippy -p edgezero-adapter-cloudflare --target wasm32-unknown-unknown --features cloudflare --all-targets -- -D warnings - cd examples/app-demo && cargo test --workspace --all-targets - (cd docs && npx prettier --check guide/cli-reference.md guide/adapters/spin.md)
aram356
added a commit
that referenced
this pull request
Jun 6, 2026
…, refresh stale Spin wording Addresses the June-06 PR #257 review. 1. High — Generated Cloudflare projects did not compile. The scaffold's `lib.rs.hbs` called `run_app::<App>(include_str!(...), req, env, ctx)` (4 args), but the current runtime API is `run_app(req, env, ctx)` (3 args). Verified by `cargo test -p edgezero-cli --test generated_project_builds -- --ignored`, which failed pre-fix with E0061 "this function takes 3 arguments but 4 arguments were supplied" and passes after. Fix: drop the obsolete `include_str!` argument; the template now matches `examples/app-demo/crates/app-demo-adapter-cloudflare` (which has been on the 3-arg form for a while). 2. Medium — Fermyon Cloud `--dry-run` returned before calling the real argv validation (key contains `=`, per-pair/per-chunk argv cap). A "successful" dry-run could be followed by a hard failure on the real push. Fix: call `push_cloud::chunk_entries(entries)?` inside the dry-run arm. Same validation as the real write_batch path, so a green dry-run is a real predictor of push success. Also surfaces the chunk count in the preview line ("for N entries across M invocation(s)") so operators can see the batching decision the real push would make. Regression covered by `dispatch_push_fermyon_cloud_dry_run_rejects_equals_in_key`. 3. Low — Stale Spin-config wording in places where the runtime moved from Spin variables to KV (which stores dotted keys verbatim): - `crates/edgezero-core/src/config_store.rs:212`: trait doc listed `SpinConfigStore` as "Spin component variables" — updated to "Spin KV (`spin_sdk::key_value::Store`)". - `examples/app-demo/app-demo.toml:24-28`: comment claimed `feature.new_checkout` was translated to `feature__new_checkout` on Spin's flat namespace — Spin now reads `feature.new_checkout` verbatim, no translation. - `examples/app-demo/crates/app-demo-core/src/config.rs:27-33`: same fix, on the typed AppConfig field's doc comment. These were doc drift only — runtime code already does the right thing, but the stale wording would mislead anyone seeding stores manually or migrating an older app. Verified (all gates green): - cargo fmt --all -- --check - cargo clippy --workspace --all-targets --all-features -- -D warnings - cargo test --workspace --all-targets - cargo test -p edgezero-cli --test generated_project_builds -- --ignored - cargo clippy -p edgezero-adapter-spin --target wasm32-wasip2 --features spin --all-targets -- -D warnings - cargo clippy -p edgezero-adapter-fastly --target wasm32-wasip1 --features fastly --all-targets -- -D warnings - cargo clippy -p edgezero-adapter-cloudflare --target wasm32-unknown-unknown --features cloudflare --all-targets -- -D warnings - cd examples/app-demo && cargo test --workspace --all-targets
aram356
added a commit
that referenced
this pull request
Jun 6, 2026
Addresses the post-f1179df PR #257 review. 1. Medium — Spin Cloud pushes are non-atomic across chunks (one `spin cloud key-value set` shellout per ≤96 KiB argv chunk), but the pre-fix error only named the failing chunk's size + exit status. If chunk 1 committed and chunk 2 failed, the operator was left with partially-updated live cloud state and no resume boundary. Fastly already produces a committed / failed / not-attempted diagnostic (cli.rs::push_entries_with_committer). Mirror Fastly's shape in `push_cloud::write_batch`: track a cursor through `entries` as committed-chunks accumulate, and on failure emit a structured error naming - committed keys (already on Fermyon Cloud, safe to skip), - failed-chunk keys (the cloud API is atomic per shellout, so a non-zero exit means none of these landed), - not-attempted keys (subsequent chunks, fully re-push needed), - a resume hint pointing out that `set` is idempotent so re-running with the full set is also safe. Regression covered by `write_batch_partial_failure_reports_committed_failed_and_not_attempted_keys`, which stands up a fake `spin` that succeeds on invocation 1 and fails on invocation 2, feeds 7 × ~30 KiB entries (>=3 chunks), and asserts the diagnostic names all three buckets plus the upstream stderr. 2. Medium — `docs/guide/manifest-store-migration.md`'s capability table still listed spin's Config as `Single (flat variables)`, but the Spin runtime is KV-backed and Multi for Config (one `spin_sdk::key_value::Store` per declared `[stores.config].id`). This page is linked from the loader's hard-error message when it encounters a pre-rewrite manifest, so the stale entry can actively mislead migrations. Changed to `Multi (KV label)`. 3. Low — Axum config-push docs claimed it was "future Stage 7 work" and told users to populate `.edgezero/local-config-<id>.json` directly. `config push --adapter axum` ships and writes the same file the runtime reads. Updated: - `docs/guide/configuration.md` — bullet now points operators at `edgezero config push --adapter axum`. - `docs/guide/adapters/axum.md` — same bullet under the Config Store section; also calls out the typed flow from a downstream `<your-cli>` for `#[secret]` stripping. Verified (all gates green): - cargo fmt --all -- --check - cargo clippy --workspace --all-targets --all-features -- -D warnings - cargo test --workspace --all-targets - cargo clippy -p edgezero-adapter-spin --target wasm32-wasip2 --features spin --all-targets -- -D warnings - cd examples/app-demo && cargo test --workspace --all-targets - (cd docs && npx prettier --check guide/manifest-store-migration.md guide/configuration.md guide/adapters/axum.md)
ChristianPavilonis
requested changes
Jun 9, 2026
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
Contributor
There was a problem hiding this comment.
PR Review
Summary
Strict-clippy gate is healthy under pinned Rust 1.95.0, but this PR removes public adapter root APIs while docs still direct users to those paths, leaving published snippets broken.
Findings
Blocking
- 🔧 Removed root adapter APIs leave docs/snippets broken:
crates/edgezero-adapter/src/lib.rsnow exposes only modules, but docs still show root imports such asedgezero_adapter::register_adapter,edgezero_adapter_axum::run_app,edgezero_adapter_fastly::FastlyProxyClient, andedgezero_adapter_cloudflare::CloudflareRequestContext(docs/guide/adapters/overview.md:108,docs/guide/adapters/axum.md:33,84,docs/guide/adapters/fastly.md:106,168,docs/guide/adapters/cloudflare.md:96,122). Either restore compatibility re-exports with documented#[expect(clippy::pub_use, reason = "...")], or update docs/migration notes to the new module paths.
Non-blocking
- ⛏ Fastly KV dispatch error docs overstate optional-KV failures: see inline comment.
CI Status
- fmt: PASS (
cargo +1.95.0 fmt --all -- --check) - clippy: PASS (
cargo +1.95.0 clippy --workspace --all-targets --all-features -- -D warnings) - tests: PASS (
cargo +1.95.0 test --workspace --all-targets)
prk-Jr
requested changes
Jun 10, 2026
prk-Jr
left a comment
prk-Jr
left a comment
Contributor
There was a problem hiding this comment.
PR Review
Summary
Large, well-scoped hardening pass: strict clippy enforcement, ~400+ real code fixes (no #[allow] papering), a real pagination bug fixed, proc-macro panic → compile_error!, CodeQL suppressions resolved, and a solid CI matrix consolidation. Two blocking issues need resolution before merge.
😃 Praise
- Pagination scan-cap cursor bug fixed with a regression test (
key_value_store.rs): the oldhas_more.then(...)returnedcursor: Nonewhen the scan cap fired, making the store appear exhausted. The new three-way cursor resolution correctly returns the last scanned position. Thecfg(test)batch-size lowering to make the cap reachable without 25k inserts is a good trick. #[app]macro →compile_error!()instead of panic: everyexpand_apperror path now surfaces at the call site with file/line attribution rather than an opaque "proc-macro panicked" message. Real developer-experience improvement.#[expect]throughout instead of#[allow]: if upstream clippy fixes a flagged site,#[expect]turns into a build error;#[allow]silently lingers forever. The discipline at scale matters.- WASM CI matrix + pinned runners from
.tool-versions: three jobs → one matrix. The inline comment explaining why the official Wasmtime install script broke (2026-05-19, interpolation failure) will save hours of debugging. - CodeQL cleartext-logging / cleartext-transmission fixes are real changes (dropped binding name from log line; renamed taint-source test helper), not suppressions.
Findings
Blocking
- 🔧 simple_logger version mismatch —
examples/app-demo/Cargo.toml:32still hassimple_logger = "4"; the generator (crates/edgezero-cli/src/generator.rs) now scaffolds"5". Demo and generator are out of sync. The demo is the first thing users read — it should match whatedgezero newproduces. Fix: bumpsimple_logger = "4"→"5"inexamples/app-demo/Cargo.toml. - 🔧 PR description contradicts final branch state — body says "Pinned viceroy 0.16.4 … we ship 1.91" but
.tool-versionshasviceroy 0.17.0andrust 1.95.0. The rationale was written before commit"Upgrade toolchain to rust 1.95.0; bump viceroy to 0.17"and was never updated. Update the PR body to reflect the actual final state — drop the MSRV caveat language.
Non-blocking
- ❓
KvPagepublic contract: undocumented edge case — see inline comment onkey_value_store.rs - 🤔
EdgeError::source()shadowsstd::error::Error::source()— see inline comment onerror.rs - ♻️ Macro emits
build_app/configurethat duplicate trait defaults — see inline comment onmacros/app.rs - ♻️ Test
Hooksimpls restate every default method — see inline comment onapp.rs
📌 Out of Scope
EdgeErrormarked#[non_exhaustive]is correct for semver discipline but shifts match-exhaustiveness to downstream. Track whether the pre-1.0 API stability plan is documented anywhere.
CI Status
- fmt: ✅ PASS
- clippy (native): ✅ PASS
- tests: ✅ PASS (557+ tests)
- wasm clippy / wasm tests: not run locally (requires Viceroy / Wasmtime / wasm-bindgen)
13 tasks
Five reviewer follow-ups from the chore/strict-clippy PR: - fastly/dispatch_with_kv: # Errors doc now distinguishes the kv_required=true (errors) and kv_required=false (logs + skips) paths. - core/KvPage: doc clarifies termination is cursor.is_none(), not keys.is_empty(); empty keys with Some(cursor) is a valid intermediate page when the scan-cap skips a run of expired entries. - core/EdgeError: rename intrinsic source() to inner() and drop the same_name_method expect — the name conflict with Error::source is gone. - macros/#[app]: comment the emitted Hooks impl to flag drift risk for configure/build_app vs the trait defaults under missing_trait_methods. - core/tests: collapse DefaultHooks/TestHooks Hooks impls to the methods they actually override; add a targeted #[expect(missing_trait_methods)] on each stub with the rationale.
aram356
added a commit
that referenced
this pull request
Jun 10, 2026
Pulls in c5652d3 — chore/strict-clippy's second round of PR #257 review-feedback fixes — on top of f374351. Conflicts resolved in three files; the other two touched files auto-merged cleanly: - crates/edgezero-core/src/error.rs Upstream renamed the typed `EdgeError::source(&self) -> Option<&AnyError>` helper to `inner()` to drop the same-name shadow of `Error::source` (and dropped the `#[expect(clippy::same_name_method)]` it needed). Git auto-merged the new `inner()` insertion near the top of the impl, but couldn't tell HEAD's old `source()` block (further down) was the same logic — that landed as a conflict. Resolution: keep upstream's `inner()` rename, drop the duplicate `source()` block, and add `EdgeError::NotImplemented` to `inner()`'s None-arm match (that variant exists only on this branch — upstream hadn't seen it, so its match was missing the arm). - crates/edgezero-core/src/app.rs Upstream collapsed the `DefaultHooks` / `TestHooks` test stubs to only the methods they actually override (the rest now use trait defaults under `#[expect(missing_trait_methods)]`). Conflict 1 (DefaultHooks): HEAD still spelled out `build_app`, `configure`, `name` overrides that just delegated to the trait default. Kept upstream's slim form (only `routes` + `stores`). Conflict 2 (TestHooks): HEAD had a `build_app` override that delegated to the trait default; upstream had a `config_store()` override targeting a method removed from the `Hooks` trait on this branch (post-rewrite `stores()` carries config metadata). Kept only the methods that still override real trait behavior: `configure`, `name`, `routes`, `stores`. - crates/edgezero-adapter-fastly/src/request.rs Upstream improved the `# Errors` doc on a `pub fn dispatch_with_kv` that no longer exists on this branch — the post-rewrite signature is `pub(crate) fn dispatch_with_registries(..., config_meta, kv_meta, secret_meta, env)` (three-store, not single-kv). The `kv_required` distinction upstream documented now lives inside `resolve_kv_handle`. Resolution: kept HEAD's signature + brief doc, with a pointer to `resolve_kv_handle` for the `kv_required=true/false` escalation semantics upstream wanted documented. No `# Errors` section needed since the function is `pub(crate)`, outside the `missing_errors_doc` lint's scope. Auto-merged (no conflict): - crates/edgezero-core/src/key_value_store.rs (upstream `KvPage` doc clarification on termination = cursor.is_none()) - crates/edgezero-macros/src/app.rs (upstream comment on the emitted Hooks impl to flag drift risk for configure/build_app vs trait defaults under missing_trait_methods) Verified on this branch (all gates green): - cargo fmt --all -- --check - cargo clippy --workspace --all-targets --all-features -- -D warnings - cargo test --workspace --all-targets - cargo clippy -p edgezero-adapter-spin --target wasm32-wasip2 --features spin --all-targets -- -D warnings - cargo clippy -p edgezero-adapter-fastly --target wasm32-wasip1 --features fastly --all-targets -- -D warnings - cargo clippy -p edgezero-adapter-cloudflare --target wasm32-unknown-unknown --features cloudflare --all-targets -- -D warnings - cd examples/app-demo && cargo test --workspace --all-targets
ChristianPavilonis
requested changes
Jun 10, 2026
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
Contributor
There was a problem hiding this comment.
Summary
Thanks for the strict-clippy cleanup. CI is green on the latest head, but I found one merge-blocking gap: the public demo/generated wasm adapter entrypoints inherit the new strict lint profile but fail target-specific clippy, and the new CI job only covers the adapter crates themselves. I left inline suggestions for the easy template/CI pieces.
Body-only notes:
crates/edgezero-core/src/lib.rs:32removes the previousedgezero_corefacade exports for KV/config/secret types (KvHandle,ConfigStoreHandle,SecretHandle,NoopKvStore, etc.). Downstream users importing those public root items will stop compiling unless this is an intentional breaking API migration. Please restore the compatibilitypub uses or document the migration explicitly.docs/guide/handlers.md:393-400still shows the oldIntoResponsesignature (fn into_response(self) -> Response) even thoughcrates/edgezero-core/src/response.rsnow requiresResult<Response, EdgeError>. Please update the guide snippet so copied custom response implementations compile.- The Cloudflare/Fastly/Spin entrypoint fixes need to be mirrored into
examples/app-demoas well as the templates. Some demo entrypoint lines are not present in GitHub's reviewable patch context, so I could not leave all of those as inline suggestions.
Two real fixes from PR review feedback (round 3): - cloudflare scaffold + demo: replace wildcard `use worker::*` with explicit imports, add `# Errors` doc and `#[inline]`. Generated Cloudflare projects now pass strict wasm32 clippy. - docs/guide/handlers.md: update the IntoResponse example to return `Result<Response, EdgeError>` (the trait signature changed in this PR); the old `-> Response` example would fail to compile. The reviewer's matching suggestions for the fastly and spin scaffolds required suppressions on macro-expanded code (`#[fastly::main]`, `#[http_component]`) we do not own. Those are deliberately declined; see the PR thread replies for the rationale.
# Conflicts: # Cargo.lock # Cargo.toml
ChristianPavilonis
approved these changes
Jun 11, 2026
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
Contributor
There was a problem hiding this comment.
Review Summary
Approved. I reviewed PR #257 on chore/strict-clippy against main, including the large strict-Clippy/API/CI/template changes and the existing review history to avoid duplicates. CI is green and I did not find any remaining blocking issues.
The inline notes below are non-blocking follow-ups: API/migration polish, exact tool-version validation, CLI output polish, and docs cleanup.
Body-only non-blocking note
- P3 / Low — proxy docs still mention removed Axum default constructor —
docs/guide/proxying.md:59is outside the changed diff, but still says to useAxumProxyClient::default(). The API now uses fallible construction viaedgezero_adapter_axum::proxy::AxumProxyClient::try_new()?. Please update this in a follow-up docs pass.
Three real fixes from PR review feedback:
- .github/workflows/test.yml: switch wasmtime version verification from
`grep -qF "wasmtime $version"` (substring match — accepts `44.0.10`
when pinned to `44.0.1`) to exact comparison via `awk '{print $2}'`.
- crates/edgezero-cli/src/main.rs: replace SimpleLogger with a tiny
custom log::Log impl that prints only `record.args()` — `info`/`debug`
/`trace` to stdout, `warn`/`error` to stderr. SimpleLogger always
emitted `INFO [edgezero_cli::xxx] ...` prefixes even with
`without_timestamps()`, regressing the user-facing CLI UX the
surrounding comment promised.
- docs/guide/handlers.md: simplify the IntoResponse example to
`.map_err(EdgeError::internal)` — drops the undeclared `anyhow`
dependency the previous snippet required.
The reviewer's fourth comment (body.rs as_bytes/into_bytes signature
changes — compat shims or migration docs) is intentionally declined.
The crate is pre-1.0; the project rejects backward-compat shims, and
the new `Option<&[u8]>` / `Option<Bytes>` signatures are the safer ones.
prk-Jr
approved these changes
Jun 12, 2026
prk-Jr
left a comment
prk-Jr
left a comment
Contributor
There was a problem hiding this comment.
😃 Approved with nonblocking comments
No merge blockers from this pass. Local verification passed (cargo fmt --all -- --check, cargo clippy --workspace --all-targets --all-features -- -D warnings, and cargo test --workspace --all-targets), and GitHub PR checks are green. I left two small follow-ups plus one praise note inline.
Two fixes from review feedback: - crates/edgezero-cli/src/args.rs: remove the unused `--local-core` flag. It was parsed into NewArgs but never read by `generate_new`; running `edgezero new ... --local-core` had no effect on dependency resolution. Wiring up a real local-path-dep mode is out of scope for this PR; drop the flag instead of carrying dead surface. - crates/edgezero-cli/src/main.rs: fix CliLogger comment/behavior mismatch. `enabled()` only permits `<= Info` and `LevelFilter::Info` is set globally, so debug/trace never reach `log()`. Doc now reads "info to stdout, warn/error to stderr; debug/trace filtered out (no verbosity flag yet)" and the match arm is split accordingly.
aram356
added a commit
that referenced
this pull request
Jun 12, 2026
… feature/extensible-cli PR #257 (chore/strict-clippy) was squash-merged into main as 7ec2ad1, so the same content this branch had been pulling in piecewise via repeated `origin/chore/strict-clippy` merges (c666132, 1b7342f, 3082391, a04814d, 9175cff, e526e6b) now arrives as a single tree-level diff against main. Tree-level reconciliation produced 54 conflicts because the squash commit ships the pre-rewrite/pre-PR-#269 shape of each affected file (old `Command::{ Build {…}, Deploy {…}, Serve {…}, Dev }` enum variants, `handle_build/deploy/serve` routing in `main.rs`, old `SimpleLogger` init, missing `Auth`/`Config`/`Provision` subcommands, old store/manifest types, etc.), while HEAD has the fully-rewritten state that PR #269 has been delivering and reviewing for the last several rounds. Resolution: take HEAD for all 54 conflicts. The squash commit adds nothing this branch doesn't already have via the prior piecewise merges — every conflict region's upstream side is content this branch explicitly rewrote. Confirmed by walking the conflict markers and matching each upstream hunk to a corresponding rewrite already on HEAD (case-insensitive adapter lookup, manual `Default` impls, `[stores]` `deny_unknown_fields`, `__` rejection in app-config keys, F1-F7 fastly/cloudflare/spin fixes, `.tool-versions` scaffold template, `CliLogger` in lib.rs, etc.). One additional new-file delete: `crates/edgezero-cli/src/dev_server.rs` (added by the squash) is the pre-rewrite location of the dev server; HEAD has moved it to `crates/edgezero-adapter-axum/src/dev_server.rs`. Removed the stray upstream-added file. Plus one real round-5 review-feedback fix carried over from upstream's `75a01ec strict-clippy: round-5 review feedback (cli args + logger)`. That commit had two fixes: 1. `args.rs`: drop the unused `--local-core` flag from `NewArgs`. This branch already doesn't have it (we dropped it as part of earlier rewrites), so it's already in the right state. 2. `lib.rs::CliLogger`: filter `debug`/`trace` explicitly rather than routing them to stdout alongside `info`. Pre-fix the match arm wrote `info|debug|trace => println!(...)` and the doc claimed all three went to stdout, but `enabled()` already capped at `<= Info` and `LevelFilter::Info` was set globally so debug/trace never reached `log()` — the comment promised behaviour the impl couldn't deliver. Split the match arm so `info => println!`, `debug | trace => {}`, and the doc now says "info to stdout, warn/error to stderr; debug/trace filtered out (no verbosity flag yet)". Applied here on lib.rs since this branch already moved `CliLogger` out of `main.rs` (where upstream put it) into the library so downstream CLIs built on `edgezero-cli` reuse the same prefix-less output. Verified on the merged tree (all gates green): - cargo fmt --all -- --check - cargo clippy --workspace --all-targets --all-features -- -D warnings - cargo test --workspace --all-targets - cargo clippy -p edgezero-adapter-spin --target wasm32-wasip2 --features spin --all-targets -- -D warnings - cargo clippy -p edgezero-adapter-fastly --target wasm32-wasip1 --features fastly --all-targets -- -D warnings - cargo clippy -p edgezero-adapter-cloudflare --target wasm32-unknown-unknown --features cloudflare --all-targets -- -D warnings - cd examples/app-demo && cargo test --workspace --all-targets
aram356
added a commit
to stackpop/mocktioneer
that referenced
this pull request
Jun 12, 2026
stackpop/edgezero#257 (strict-clippy + defensive-coding pass) merged to edgezero main as 7ec2ad1, and the chore/strict-clippy branch was deleted. Switch all six edgezero crates from `branch = "chore/strict-clippy"` to `branch = "main"` and re-resolve the lock onto 7ec2ad1. Lock + manifest only; no mocktioneer source changes. Verified green: cargo check / clippy (-D warnings) / 161 tests / fmt / "fastly cloudflare spin" feature build / all three wasm-target builds.
aram356
added a commit
to stackpop/mocktioneer
that referenced
this pull request
Jun 12, 2026
* Adopt edgezero strict-clippy gate and migrate to PR #257 API Track stackpop/edgezero#257 (`chore/strict-clippy` branch) and mirror the same strict clippy gate workspace-wide: `pedantic = warn`, `restriction = deny`, with a 13-entry workspace allow-list. Adopt the test exemption set in `clippy.toml`. The PR includes a defensive-coding pass that changes upstream APIs: `Router::oneshot` now returns `Result<Response, EdgeError>`, `Body` is a `Once`/`Stream` enum whose bytes are `Option`-returning, `run_app` moved to `dev_server::run_app`, and `IntoResponse::into_response()` is now fallible. Adapter wiring and test helpers are updated accordingly. Clippy fallout (~700 findings) is resolved through real refactors per the upstream principle "every removed allow corresponds to a real refactor, not a sprinkling of `#[allow]`/`#[expect]`": fields and items reordered alphabetically, numeric literals typed, single-char idents renamed, public fns inlined, test fns drop the `test_` prefix, casts replaced with checked conversions, integration tests wrapped in `#[cfg(test)] mod tests`. OpenRTB `w`/`h` fields are renamed to `width`/`height` with `#[serde(rename)]` to preserve the JSON wire format; `_ref` becomes `r#ref`; `_type` becomes `kind` with a serde rename. All route handlers now uniformly return `Result<Response, EdgeError>`. Five call-site `#[expect]` annotations remain, each documented with `reason = "..."`: float arithmetic in CPM fallback, the SVG layout math cluster on i64 banner dimensions, and three `print_stderr` sites in adapter startup-error paths where no logger is initialised. * Add serde wire-format round-trip tests for renamed OpenRTB fields The Rust-side renames `w`/`h` → `width`/`height`, `_ref` → `r#ref`, and `_type` → `kind` rely on `#[serde(rename = ...)]` (or raw-identifier default mapping for `r#ref`) to preserve the JSON contract. None of the existing tests assert the JSON key names, so a broken rename would silently change the wire format. Add seven focused round-trip tests that pin the serialized key names and verify deserialization from spec-shaped JSON. * Bump toolchain to Rust 1.95.0 and refresh edgezero deps Track the latest commits on stackpop/edgezero `chore/strict-clippy` (a26704f7 → 97d02de5), which include the upstream toolchain bump to Rust 1.95.0 alongside `ctor 0.10 → 1.0`, `viceroy 0.17`, and no-features-only clippy fixes. Pin `.tool-versions` to `rust 1.95.0` and update the CLAUDE.md toolchain table to match. Rust 1.95.0 ships two new restriction-clippy lints we previously weren't subject to: - `doc_paragraphs_missing_punctuation` — 59 sites across aps, mediation, render, and auction modules where doc-comment paragraphs lacked a terminating `.`. Fixed by appending punctuation at each site. - `duration_suboptimal_units` — `Duration::from_secs(10 * 60)` for the JWKS cache TTL becomes `Duration::from_mins(10)`. All gates green: cargo check / clippy (workspace, all-targets, all-features, `-D warnings`) / fmt / test (90 tests). * Add per-adapter wasm-target compile-check matrix Mocktioneer's CI previously ran `cargo test` and `cargo check --features "fastly cloudflare"` only on the host triple. The Fastly and Cloudflare adapters' real entry points compile only under their wasm targets, so a worker/fastly version skew or a `run_app` signature change upstream would not surface in CI — it would surface at deploy time. Add an `adapter-wasm-check` matrix mirroring stackpop/edgezero#257's `adapter-wasm-tests` matrix (compile-check only, since mocktioneer's adapter crates do not yet carry a wasm-runtime contract suite): - cloudflare → `cargo check ... --target wasm32-unknown-unknown` - fastly → `cargo check ... --target wasm32-wasip1` This immediately surfaced the bumps required to track the latest edgezero head: - workspace `worker` 0.7 → 0.8 and `fastly` 0.11.9 → 0.12 (edgezero's PR #257 already moved to these majors; the host build hid the skew because the host stubs don't touch those types); - `edgezero_adapter_cloudflare::run_app` gained a leading manifest `&str` parameter (matching the existing Fastly signature). Updated the cloudflare adapter `#[event(fetch)]` entry point accordingly. Dropped the now-redundant host-step `Add wasm32-wasi target` and `Setup Viceroy` from the main `test` job — they were never exercised there and the matrix job installs them per-cell as needed. * Run per-adapter wasm contract tests in CI Replace the wasm compile-check matrix with a wasm test matrix that actually executes the adapter dispatch path inside the wasm runtime. Contract tests: - `crates/mocktioneer-adapter-fastly/tests/contract.rs` — exercises `dispatch(&app, FastlyRequest)` for `GET /` and `GET /pixel?pid=...` against `wasm32-wasip1` via Viceroy. - `crates/mocktioneer-adapter-cloudflare/tests/contract.rs` — same two routes via the cloudflare `dispatch(app, req, env, ctx)` path under `wasm32-unknown-unknown` via `wasm-bindgen-test`. Both files annotate `#![expect(deprecated, reason = "...")]` because the low-level `dispatch` entry points are the test surface that remains public — the higher-level `dispatch_with_config*` variants require irrelevant config-store wiring for our simple contracts. CI matrix mirrors stackpop/edgezero#257: - cloudflare cell installs `wasm-bindgen-cli` at the version pinned in `Cargo.lock` and sets `CARGO_TARGET_WASM32_UNKNOWN_UNKNOWN_RUNNER`. - fastly cell installs Viceroy at the version pinned in `.tool-versions` (now `viceroy 0.17.0`, matching upstream) and sets `CARGO_TARGET_WASM32_WASIP1_RUNNER`. `.cargo/config.toml` keeps a workspace-level `[target.wasm32-wasip1] runner = "viceroy run -C crates/.../fastly.toml --"` so the same `cargo test` invocation works locally with the project's fastly.toml. The env var set in CI takes precedence and uses bare defaults. `wasm-bindgen-test = "0.3"` added as a wasm32-target dev-dependency to the cloudflare adapter crate. * Bump CI actions to current majors (Node 24 runners) The six Node-20-runner actions emit deprecation warnings on every workflow run; GitHub will remove the runners in a future schedule. Bump each to its current major (Node 24), matching the alignment done upstream in stackpop/edgezero#257: - actions/checkout v4 → v6 (test, codeql, format, deploy-docs, docker) - actions/cache v4 → v5 (test, format) - actions/setup-node v4 → v6 (test, deploy-docs, format) - actions/configure-pages v4 → v6 (deploy-docs) - actions/deploy-pages v4 → v5 (deploy-docs) - actions/upload-pages-artifact v3 → v5 (deploy-docs) actions/upload-artifact@v4, actions-rust-lang/setup-rust-toolchain@v1, docker/*, and github/codeql-action@v4 are already at their current majors and left untouched. * Fixed formatting * Bump Docker build to Rust 1.95.0 and debian stable runtime Completes the toolchain bump for the container build: the `RUST_VERSION` build arg now matches `.tool-versions` (1.95.0). Switch the runtime stage from the pinned `debian:bookworm-slim` to `debian:stable-slim` so the image tracks the current Debian stable. * Refresh Cargo.lock with semver-compatible transitive updates Routine `cargo update` of registry dependencies — `anyhow`, `async-compression`, `syn`, and other transitive crates move to their latest semver-compatible releases. No manifest changes; reproducible build inputs only. * Address PR #108 review feedback Review by @prk-Jr — no blocking issues; resolves the actionable items: - openrtb.rs: document the `Geo.kind`/`Geo.type2` and `Site.r#ref`/ `Site.ref_` field pairs — which carries the OpenRTB spec key and which is a non-spec legacy-compat field. - routes.rs: remove the empty `impl SizeDimensions {}` block left over from the item-reorder pass. - routes.rs: comment `handle_pixel`'s `PixelQueryParams { .. }` discard — `pid` is validated on extraction but intentionally unused. - adapter-cloudflare contract.rs: comment that `run_in_browser` only declares the harness mode; CI runs headless under Node via the `CARGO_TARGET_WASM32_UNKNOWN_UNKNOWN_RUNNER` runner. - adapter-cloudflare Cargo.toml: pin `wasm-bindgen-test` to `=0.3.71` to match the explicit CLI version-pin in CI. The reviewer's WASM-compatibility concern (`std::time::Instant` in the JWKS cache panics on wasm32-unknown-unknown) is pre-existing and tracked separately in #109. * Add Spin (Fermyon) adapter crate Mocktioneer was missing a Spin adapter. Add `mocktioneer-adapter-spin` modelled after the cloudflare/fastly crates and edgezero's app-demo-adapter-spin example: crates/mocktioneer-adapter-spin/ Cargo.toml bin (host stub) + cdylib (wasm32-wasip1) spin.toml Spin v2 manifest src/main.rs host stub — reminds operators to `spin up` src/lib.rs `#[http_component]` handler that delegates to `edgezero_adapter_spin::run_app::<MocktioneerApp>` tests/contract.rs wasm32-wasip1 contract tests run via wasmtime Workspace + manifest wiring: - Add to `[workspace.members]`. - Add `spin-sdk = "5.2"` and `edgezero-adapter-spin` workspace deps (5.2 matches edgezero's PR #257 pin — upgrade to 6.0 is upstream- blocked by `Method` variant API breaks; tracked separately). - Register `[adapters.spin.*]` in `edgezero.toml` and append `"spin"` to every trigger's adapter list. CI matrix: - Add `spin` cell to `adapter-wasm-tests` (wasm32-wasip1 / wasmtime). - Install wasmtime via the official script when matrix.adapter == spin. - Reuses the same `runner_env` mechanism as fastly; matrix sets `CARGO_TARGET_WASM32_WASIP1_RUNNER=wasmtime run`, overriding the workspace `.cargo/config.toml` viceroy default for this cell. One documented `#![cfg_attr(... expect(unsafe_code, reason = "..."))]` at the wasm32 cfg: `#[http_component]` expands to WASI wit-bindgen code containing `#[export_name]`, an `unsafe fn` declaration, and an unsafe block, all of which trip `unsafe_code = "deny"`. This is the only entry into the Spin runtime, so the macro expansion is intrinsically unsafe. All gates green: clippy / 121 tests (host) / fmt / `"fastly cloudflare spin"` feature check / wasm builds for all three adapter targets. Spin contract test (`root_dispatches_through_spin_runtime`, `pixel_returns_gif_through_spin_runtime`) verified locally via wasmtime. * Fixed .gitignore to ignore build outputs from both Spin and Rust. * Correct cloudflare contract test execution-environment comment @ChristianPavilonis observed the CI logs for this PR show the cloudflare contract tests running headless in Firefox, not Node. My previous comment on this line (added in 5ae2cd5) incorrectly claimed Node. `run_in_browser` genuinely selects the wasm-bindgen browser harness; the ubuntu-latest CI image ships with geckodriver, so the harness lights up Firefox. Update the comment to accurately describe what actually runs. * Remove dead Site.ref_ and Geo.kind fields @prk-Jr asked what spec field `Site.ref_` maps to. The honest answer is: none — and `Geo.kind` (renamed earlier from `_type`) is the same mistake. `git blame` shows both fields were added in 5bb01d9 by the same author in the same commit as their spec-conformant siblings. The pattern is clear: the author wrote the conventional Rust keyword-escape form first (`ref_`, `_type`), realized that serialised to `"ref_"`/`"_type"` which don't match the OpenRTB 2.x spec, then added `_ref`/`type2` with `#[serde(rename = "ref"/"type")]` to fix the wire format, and never removed the original attempt. Neither field is read or written outside the round-trip tests I added in this PR, and dropping them is behavior-neutral (`#[serde(deny_unknown_fields)]` is not enabled, so incoming `"ref_"`/`"_type"` keys are silently ignored either way). My previous attempt to address the review (5ae2cd5) added speculative "legacy compatibility" doc comments rather than answering the question. This commit answers it by removing the dead fields and tightening the round-trip tests to assert just the spec keys. * Align format.yml with stackpop/edgezero#257 layout Two cosmetic differences against edgezero's `Run Format` workflow: - Top-level `permissions:` moves above `concurrency:` (matching edgezero's order). - Drop the `~/.cargo/bin/` cache path — this workflow only runs fmt and clippy, which install no cargo binaries, so caching that directory was dead weight. format.yml now matches edgezero byte-for-byte. No matrix involved (the adapter-wasm-tests matrix lives in test.yml). * Refresh Cargo.lock with semver-compatible transitive updates * Address @prk-Jr / @ChristianPavilonis PR review feedback - **Playwright `AD_SIZES` registration bug** (`tests/playwright/ creative-visibility.test.ts:40` — @ChristianPavilonis): the loop was iterating a list populated in `beforeAll`, but Playwright registers tests synchronously at module load, so only 2 of the intended per-size suite were actually registered. Switch to a static `AD_SIZES` constant (mirrors `STANDARD_SIZES` in `auction.rs`) and add a `sizes endpoint matches static AD_SIZES list` assertion so any drift between the server list and the test list fails loudly. `npx playwright test --list` now reports 41 tests (was 2). - **OpenRTB response example missing `sig` query parameter** (`docs/api/openrtb-auction.md:107` — @prk-Jr, @ChristianPavilonis): runtime `adm` always includes `&sig=verified|failed|not_present` (set by `iframe_html` from `metadata.signature.url_param()`). The exact-output example now matches: `…?crid=mocktioneer-imp-1&sig=not_present`. - **Pin Wasmtime instead of `curl ... | bash` of the latest** (`.github/workflows/test.yml:139` — @ChristianPavilonis): add `wasmtime 45.0.0` to `.tool-versions` and pass `-v "v$VERSION"` to the installer so the spin matrix cell fetches a specific tagged release. Removes the unpinned/latest-installer supply-chain concern and matches how `viceroy`/`wasm-bindgen-cli` are pinned. - **Add Spin to CLAUDE.md adapter/project docs** (@ChristianPavilonis): workspace layout, compilation-targets table, `edgezero-cli serve` examples, and the key-files-reference table all now mention the `mocktioneer-adapter-spin` crate alongside axum/cloudflare/fastly. prk-Jr's earlier comments (`wasm-bindgen-test` pin, cloudflare contract test comment, `_ref`/`_type` doc / removal) were already resolved in prior commits on this branch. All gates green: clippy `-D warnings`, 161 tests (host), fmt, `"fastly cloudflare spin"` feature check; Playwright now lists 41 tests vs the previous 2. * Updated Cargo.lock * Fix Wasmtime install flag in CI: -v -> --version The wasmtime.dev installer rejects `-v` as an unknown option (it offers `--release`/`--dev`/`--version` only). The spin matrix cell on the latest PR run failed at the Setup Wasmtime step with: Error: unknown option: '-v' Switch the short flag to the long `--version` form. Comment refreshed to match. * Always install pinned Wasmtime in CI @ChristianPavilonis flagged that the previous `if ! command -v wasmtime` guard would silently bypass the `.tool-versions` pin if the GitHub runner image later preinstalls Wasmtime — any version would satisfy the guard and the install step would skip. Drop the guard so the pinned version is always installed. The install script is small/fast (single curl + tarball extract) and `~/.wasmtime/bin` is prepended to PATH for the rest of the job, so this shadows any system wasmtime. * Sync edgezero deps to latest chore/strict-clippy (PR #257) Bump the six edgezero crates from e7887140 to dfb00b39, pulling in the round-2 through round-4 review-feedback commits on PR #257: - strict-clippy: address PR review feedback (round 2) - strict-clippy: cloudflare scaffold + handlers.md doc fixes - Bump redb 4.0 -> 4.1.0 (#255) - merge main into chore/strict-clippy - strict-clippy: round-4 review feedback (test.yml, cli logger, docs) These touch edgezero-core (app.rs, error.rs, key_value_store.rs), edgezero-macros (app.rs), and the fastly/cloudflare adapters. Lock-only change; no mocktioneer source edits required. Verified green: cargo check / clippy (-D warnings) / 161 tests / fmt / "fastly cloudflare spin" feature build / all three wasm-target builds. Fastly and Spin wasm contract tests pass via Viceroy and Wasmtime. * Point edgezero deps at main now that PR #257 is merged stackpop/edgezero#257 (strict-clippy + defensive-coding pass) merged to edgezero main as 7ec2ad1, and the chore/strict-clippy branch was deleted. Switch all six edgezero crates from `branch = "chore/strict-clippy"` to `branch = "main"` and re-resolve the lock onto 7ec2ad1. Lock + manifest only; no mocktioneer source changes. Verified green: cargo check / clippy (-D warnings) / 161 tests / fmt / "fastly cloudflare spin" feature build / all three wasm-target builds.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Enable strict clippy (
pedanticwarn +restrictiondeny) workspace-wide and shrink the allow-list from ~41 entries at the start to 13 entries (matching theapp-demoworkspace's slim profile within four irreducible-by-design lints). Every removed allow corresponds to a real refactor, not a sprinkling of#[allow]/#[expect]annotations.Final allow-list (13)
blanket_clippy_restriction_lintsrestrictionas a groupmissing_docs_in_private_itemsimplicit_returnquestion_mark_usedsingle_call_fnseparated_literal_suffixstd_instead_of_alloc/std_instead_of_coreexhaustive_structsedgezero_core::app!emits a unit struct that legitimately fires the lintexhaustive_enumsBody { Once, Stream }is the only firing site;#[non_exhaustive]would force_ => unreachable!()at 37 externalmatchsites and break the closed-enum designpub_with_shorthandpub(in crate)→pub(crate); documentedmodule_name_repetitionsproxy::Request/Responsewould collide withhttp::Request/Response, 17Manifest*types are load-bearing public API, and the#[app]macro emits code referencing the current namespattern_type_mismatchref_patterns; one must be allowed — we pick match-ergonomicsEvery other workspace allow that was present in the initial commit has been removed via a real fix.
Refactors that removed allows (highlights)
missing_trait_methods— explicit overrides forKvStore::exists(5 impls),Hooks::{configure,name,config_store,build_app}(test impls +#[app]macro output),Clone::clone_fromforRouteEntrypub_use— converted re-export-from-private-mod topub modacross all four adapter crates; updated callers (templates, demo, CLI, contract tests) to use full module pathsabsolute_paths— ~110 sites converted touseimports across 32 filesarbitrary_source_item_ordering— ~300 items reordered across 38 files following the canonical ExternCrate→Use→Mod→Static→Const→TyAlias→Enum→Struct→Trait→Impl→Fn order with alphabetical ordering inside each kind (including struct fields, constructor args, enum variants, impl methods,mod tests {}contents)missing_inline_in_public_items—#[inline]added to ~321 public fns/methods across 44 filesmin_ident_chars— ~190 single-character identifiers renamed (closure params, helper variables, etc.)shadow_reuse— ~30 shadowed bindings renamed across 22 files (stream-chunkpatterns,Into-parameter idiom, closure-param shadows)as_conversions— 8 cast sites eliminated via typed sibling constants or typed local bindingsarithmetic_side_effects— 6 numeric ops switched tochecked_*/saturating_*;SystemTime + ttlpropagates as typed errorformat_push_string—generator.rsswitched towriteln!(out, ...)?withGeneratorError::Format(#[from] fmt::Error)propagationtests_outside_test_module— split#[cfg(all(test, feature = "X"))]into#[cfg(test)] #[cfg(feature = "X")]so the lint recognizes the gatesingle_char_lifetime_names— 4 lifetimes renamed ('mw,'route,'manifest,'blueprint)allow_attributes— removed two unnecessary#[allow(deprecated)]annotationsfloat_arithmetic— switched the middleware request-logger toDuration::as_millis()(no float math)exhaustive_enumsfor non-Bodytypes: enums made#[non_exhaustive]where adapter consumers could absorb wildcard armsCross-cutting refactors
lib.rsmade its internal modulespub modinstead of re-exporting viapub use. Drops 4 file-level#![expect(clippy::pub_use)]annotations.Manifest::secret_store_name→secret_store_bindingto reflect that it returns a binding identifier, not a secret value.expect()/unwrap()to?propagation in scaffold/generator paths; addedGeneratorError::FormatandGeneratorError::Iovariants.#[app]macro now emitscompile_error!()rather than panicking on bad manifest input.CI improvements
adapter-wasm-testsmatrix (cloudflare/fastly/spin) with per-cell toolchain installs gated onmatrix.adapter.actions/checkout@v6,actions/setup-node@v6,actions/cache@v5,actions/configure-pages@v6,actions/deploy-pages@v5,actions/upload-pages-artifact@v5. Removes the Node 20 deprecation warnings.viceroy 0.16.4in.tool-versions(viceroy 0.17 raised MSRV to rustc 1.95; we ship 1.91); CI reads the version from.tool-versions(single source of truth)..cargo/config.tomlsocargo test -p edgezero-adapter-fastly --target wasm32-wasip1 --test contractresolves the Viceroy runner when invoked from the workspace root.cargo install --force --lockedfor cached wasm runners (cache restores existing binaries and bare install rejects them).CodeQL alerts fixed
rust/cleartext-logging** —log_store_bindingswas flagged becausemanifest_data.secret_store_*(...)is taint-source-by-name. Real fix: stop logging the binding identifier (operators read their ownedgezero.toml); presence message still emitted.rust/cleartext-transmission** —start_test_server_with_secret_handletest helper renamed tostart_test_server_with_store_handleso its return value isn't taint-source-by-name; functionally identical.Dependency bumps
redb4.0 → 4.1Test plan
cargo test --workspace --all-targets(557+ tests pass)cargo clippy --workspace --all-targets --all-features -- -D warnings(0 errors)cargo check --workspace --all-targets --features "fastly cloudflare spin"cargo check -p edgezero-adapter-spin --target wasm32-wasip1 --features spincargo check -p edgezero-adapter-fastly --target wasm32-wasip1 --features fastlycargo check -p edgezero-adapter-cloudflare --target wasm32-unknown-unknown --features cloudflarecargo fmt --all -- --checkCloses
Closes #256