Enable strict clippy with documented allow-list and defensive-coding pass#257
Open
aram356 wants to merge 74 commits into
Open
Enable strict clippy with documented allow-list and defensive-coding pass#257aram356 wants to merge 74 commits into
aram356 wants to merge 74 commits into
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.
Same heuristic as alert #7 — CodeQL taints any value returned by a function whose name contains "secret" and tracks it through to HTTP sinks. The test helper `start_test_server_with_secret_handle` was flagged because its return value's `base_url` flowed into `reqwest::Client::get(url)`. Rename the helper to `start_test_server_with_store_handle` and the return struct to `TestServerWithStore`. Functionally identical — the test just bootstraps a dev server with an optional handle. The remaining `with_secret_handle` builder method on `AxumDevServer` is unaffected because it returns `Self`, not a sink-bound value.
prk-Jr
requested changes
May 18, 2026
Contributor
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
Comprehensive strict-clippy enablement with a well-reasoned 13-entry allow-list and disciplined mechanical refactors across the workspace. Two items need resolution before merge: the wasmtime CI setup has security and reproducibility gaps, and the new .cargo/config.toml wasm runner creates a local-dev breakage for spin tests.
😃 Praise
- Allow-list documentation is exemplary — each workspace allow in
Cargo.tomlincludes a concrete rationale with file/site counts and tradeoffs. Theexhaustive_enums(37 external match sites) andmodule_name_repetitions(proxy collision, macro-generated names) entries set a high bar. - CI matrix consolidation — collapsing three duplicate wasm jobs into one matrix cuts ~80 lines and ensures identical treatment across adapters.
fail-fast: falseis correct. - CodeQL fixes go beyond suppression —
cleartext-loggingremoves the binding name from the log message semantically;cleartext-transmissionuses an accurate rename. Neither is a bare#[allow]. #[expect]over#[allow]throughout — remaining call-site suppressions use#[expect]withreason =, so future clippy versions will flag stale suppresses.
Findings
Blocking
- 🔧 wasmtime: curl-pipe, unpinned, uncached —
.github/workflows/test.yml:148. See inline. - ❓ wasm32-wasip1 runner conflict —
.cargo/config.toml:11. Viceroy hard-coded workspace-wide; spin tests broken locally. See inline.
Non-blocking
- ♻️
pub_with_shorthandcomment describes wrong lint direction —Cargo.toml:96. - 🤔 proc-macro still panics on invalid handler path —
crates/edgezero-macros/src/app.rs:214. Follow-up issue candidate. - 📝 Spin contract tests do not test Spin host ABI —
crates/edgezero-adapter-spin/tests/contract.rs:1. - 🏕 Pre-existing typo in
.tool-versions—fasltly→fastly.
📌 Out of Scope
secret_store_name→secret_store_bindingis a public API rename (semver-0.x allows it). A CHANGELOG entry would help external early adopters.
CI Status
- fmt: PASS
- clippy: PASS
- tests: PASS
This was referenced May 18, 2026
Three real coverage gaps from earlier commits were untested:
1. `KvStore::put_bytes_with_ttl` overflow error path
(axum/PersistentKvStore). Asserts `Duration::MAX` triggers
`SystemTime::checked_add` overflow and surfaces as
`KvError::Internal("ttl overflows system time")`.
2. `Manifest::try_load_from_str` Err path. Two cases: invalid TOML
bytes and a manifest that fails `validator` (empty config-store
name). Both should return `io::ErrorKind::InvalidData`.
3. `GeneratorError::Format` smoke test. The variant cannot fire in
practice (write-to-String is infallible), but it is part of the
public error surface and the `From<fmt::Error>` wiring must keep
working — assert construction + Display.
Existing coverage for the other behaviour-affecting changes was
already adequate: `KvStore::exists` is exercised by the
`contract_exists` macro across every impl plus 3 dedicated unit
tests, and `Hooks` default-method overrides are exercised by the
`TestHooks`/`DefaultHooks` tests already in app.rs.
ctor 1.0 requires explicit `#[ctor(unsafe)]` to acknowledge that
pre-main static-initialisation runs without the usual Rust safety
guarantees. The annotation is an attribute argument, not an
`unsafe { }` block, so the workspace `unsafe_code = "deny"` lint is
still satisfied. Updated the four adapter cli.rs files
(axum/cloudflare/fastly/spin).
spin-sdk 6.0 is NOT bumped: it raises the MSRV to rustc 1.93 but the
workspace ships rustc 1.91.1 (.tool-versions). Pin stays at 5.2 with
an explanatory comment until we bump the toolchain.
Bumps `.tool-versions`:
rust 1.91.1 → 1.95.0
viceroy 0.16.4 → 0.17.0
Both viceroy 0.17 and spin-sdk 6.0 raised their MSRV to rustc 1.93/1.95
respectively. We can now take viceroy 0.17 freely; spin-sdk 6.0 has
breaking API changes (Method variants → http::Method constants,
`IncomingRequest` removed, Builder::build() → .body()) and is left at
5.2 with a TODO until a focused migration PR.
New 1.95 clippy lints fixed in-place:
- `result_map_unwrap_or_default`: `.map(p).unwrap_or(false)` → `.is_ok_and(p)` (2 sites)
- `manual_map`: `.map(x).unwrap_or(default)` → `.map_or(default, x)` (1 site)
- `duration_suboptimal_units`: `Duration::from_secs(60)` → `from_mins(1)` in
non-const contexts. Two const items keep `from_secs(60 * 60 * 24 * 365)`
with a localized `#[expect(clippy::duration_suboptimal_units, reason =
"from_days/from_mins not stable in const context")]` because
`Duration::from_{mins,days}` const variants are still nightly-only.
- `to_string_in_format_args` / `inefficient_to_string`: replaced two
`ToString::to_string` / `str::to_string` with `str::to_owned`
- `missing_inline_in_public_items`: added `#[inline]` to two proc-macro
entrypoints in edgezero-macros, three EnvOverride methods + the
`env_guard` helper in axum/test_utils, and `From<Action>` for
AdapterAction in cli/adapter.rs
- `doc_paragraph_terminators`: added trailing punctuation to clap doc
comments on every variant/field of `Command`/`NewArgs` (cli/args.rs)
and the `KV_TABLE` doc in axum/key_value_store.rs
Docs:
- CLAUDE.md "Rust": 1.91.1 → 1.95.0
- CLAUDE.md "Fastly CLI": v13.0.0 → 15.1.0
- Fix typo `fasltly` → `fastly` in .tool-versions; remove dup line
- examples/app-demo/.../rust-toolchain.toml: 1.91.1 → 1.95.0
- test.yml: drop the now-stale "1.91 MSRV constraint" comment on the
viceroy install step
Both warnings sat behind `#[cfg]` gates that the `--all-features` build profile hid: 1. `fastly::init_logger` (no-features stub) needed `#[inline]` — `missing_inline_in_public_items` only fires when the stub branch is selected, i.e. when the `fastly` feature is off. 2. `cli::dev_server::EchoParams` (no-`dev-example` build) was defined after `default_router`/`build_dev_router`; the canonical item ordering wants structs before fns at module level. Moved `EchoParams` to the top of the module so the order is correct in either feature profile. Surfaces only via `cargo clippy --workspace --all-targets` (no `--all-features`); the existing CI runs `--all-features` so we did not catch this until now.
The `https://wasmtime.dev/install.sh` script broke as of 2026-05-19: its version-detection interpolation failed and it tried to download literal version `{`, causing the spin-wasm-tests CI job to fail ("Could not download Wasmtime version '{'"). Replace the install path with a direct GitHub-release tarball download, pinned to the version recorded in `.tool-versions` (same single-source-of-truth pattern already used for rust + viceroy). Adds `wasmtime 44.0.1` to `.tool-versions` and a `Resolve Wasmtime version` step in the workflow that greps it out.
aram356
added a commit
to stackpop/mocktioneer
that referenced
this pull request
May 19, 2026
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.
1. `pub_with_shorthand` comment direction was reversed in the workspace `Cargo.toml`. Confirmed by removing the allow: 6 sites fire `usage of \`pub\` without \`in\`` (i.e. clippy flags `pub(crate)` and wants `pub(in crate)`). Restore the allow with wording that matches the actual lint direction and reflects the audited 6-site count. 2. Workspace `.cargo/config.toml` was hard-coding the `wasm32-wasip1` runner to Viceroy, which silently broke `cargo test -p edgezero-adapter-spin --target wasm32-wasip1` from the workspace root (used viceroy host ABI instead of wasmtime). Fix: remove the workspace-level runner entirely and add a per-package config for spin (`crates/edgezero-adapter-spin/ .cargo/config.toml`) that selects `wasmtime run`. Fastly already had its own per-package config. CI continues to override via `CARGO_TARGET_WASM32_WASIP1_RUNNER` env var, so workspace-root invocations work in CI without the global default. 3. Add a module-level doc comment at the top of `crates/edgezero-adapter-spin/tests/contract.rs` explaining that the tests cover internal router/dispatch logic, NOT the Spin host ABI (no `spin_sdk`/WIT imports). A breaking change in the Spin runtime's WIT would not be caught here.
`parse_handler_path` previously panicked on a syntactically-invalid handler path in `edgezero.toml`, which rustc surfaced as a confusing "proc-macro panicked" message. Refactor to return `Result<ExprPath, String>`; `build_middleware_tokens` and `build_route_tokens` propagate the error; `expand_app` returns `compile_error!()` with the message, matching the existing error path for manifest read/parse/validation failures. Two new tests: parse_handler_path_accepts_absolute_crate_path (happy path) and parse_handler_path_rejects_invalid_syntax_with_message (asserts the error message names the failure and echoes the offending input). Addresses the PR review comment on `crates/edgezero-macros/src/app.rs`.
PR reviewer claimed the lint warns *against* longhand and recommends
shorthand (i.e. our `pub(crate)` use should never fire it). Verified
empirically — removing the allow on clippy 1.95 produces 6 errors:
error: usage of `pub` without `in`
| pub(crate) fn decompress_body(...)
| ^^^^^^^^^^ help: add it: `pub(in crate)`
= help: ...index.html#pub_with_shorthand
So `pub_with_shorthand` flags `pub(crate)` and suggests `pub(in crate)`;
the reviewer's reading is 180° off. Quote the diagnostic in the comment
itself so future maintainers don't fall into the same trap.
Contributor
Author
Re:
|
| Allow | Code uses | What fires |
|---|---|---|
pub_with_shorthand allowed |
pub(crate) |
nothing — we allow the form we use ✓ |
pub_with_shorthand denied |
pub(crate) |
"usage of pub without in" → 6 errors |
pub_without_shorthand allowed |
pub(crate) |
nothing — would only matter if we used pub(in crate) |
Six sites currently fire. Switching to pub(in crate) is blocked because rustfmt unconditionally rewrites it back to pub(crate) on save.
Pushed 5577695b (in commit Document pub_with_shorthand with verbatim clippy diagnostic) quoting the diagnostic inline in Cargo.toml so this doesn't come up again. CI is green.
aram356
added a commit
to stackpop/mocktioneer
that referenced
this pull request
May 19, 2026
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.
aram356
added a commit
to stackpop/mocktioneer
that referenced
this pull request
May 19, 2026
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.
Contributor
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
I reviewed the current PR and omitted the Fastly logger finding after checking the Fastly lifecycle: default #[fastly::main] requests run in fresh sandboxes, so that is not a blocker. Inline comments cover the findings that can be attached to pushed diff lines.
Additional findings folded into the body
Cloudflare scaffold calls the old run_app signature
- Severity: P1
- File:
crates/edgezero-adapter-cloudflare/src/templates/src/lib.rs.hbs:11 - Description:
run_appnow requiresmanifest_src, but the Cloudflare template still passes only(req, env, ctx), so newly generated Cloudflare projects will fail to compile. - Suggestion: Mirror the demo/template shape:
edgezero_adapter_cloudflare::run_app::<App>(include_str!("../../../edgezero.toml"), req, env, ctx).await.
Adapter docs contain broken entrypoint snippets
- Severity: P3
- Files:
docs/guide/adapters/axum.md:33,docs/guide/adapters/cloudflare.md:47 - Description: The Axum docs reference a root-level
edgezero_adapter_axum::run_appthat is not exported, and the Cloudflare docs use../../edgezero.toml, which is wrong for an entrypoint undersrc/lib.rs. - Suggestion: Match the generated/demo entrypoints (
dev_server::run_appor a root re-export for Axum, and../../../edgezero.tomlfor Cloudflare).
Spin is missing from architecture/overview docs
- Severity: P3
- Files:
docs/guide/architecture.md:13-16,docs/guide/adapters/overview.md:114-118 - Description: The project has a Spin adapter, but these overview docs still list only Fastly, Cloudflare, and Axum.
- Suggestion: Add Spin to the workspace layout, adapter sections, feature table, and adapter overview table.
This was referenced May 20, 2026
ChristianPavilonis review:
* Generator dependency seeds were stale relative to the adapters:
worker 0.7 → 0.8
fastly 0.11 → 0.12
simple_logger 4 → 5
Scaffolded projects pinned older provider SDKs than the adapters
expect, risking public-type mismatches in generated entrypoints.
* `PersistentKvStore::list_keys_page` lost keys when the scan cap was
hit. On a cap-hit the loop broke with `reached_end = false`, but the
cursor was only emitted when `live_keys.len() > limit`. An
under-filled page (cap reached while skipping a long expired run)
therefore returned `cursor: None` and callers stopped paginating,
silently missing live keys past the expired run. Now a cap-hit is
tracked and the last scanned key is returned as the resume cursor.
Added `list_keys_page_returns_resume_cursor_when_scan_cap_is_hit`.
`LIST_SCAN_BATCH_SIZE`/`MAX_SCAN_BATCHES` are lowered under
`cfg(test)` so the cap path is reachable with a 43-entry fixture
instead of 25k; pagination correctness is batch-size-independent.
Review comment #7: a freshly generated project failed its own restriction-deny clippy gate immediately. - Core handler template: mirror app-demo's passing structure — fallible `stream`/`IntoResponse` usage (no production `.expect`), alphabetically ordered structs, grouped test items, `IntoResponse` imported anonymously. - Adapter host stubs: add `#[expect(clippy::print_stderr, reason)]` and `allow(dead_code, reason)`; the axum entrypoint returns `anyhow::Result` instead of `eprintln!` + `process::exit`. - Inline the project-core `App` type in adapter entrypoints so import order stays stable regardless of project name. - key_value_store: replace `#[cfg(not(test))]` consts with `if cfg!(test)` and rename a `cursor` binding that shadowed the parameter (clippy `cfg_not_test` / `shadow_unrelated`). - Add scaffold lint-coverage assertions to the generator test.
# Conflicts: # crates/edgezero-adapter-axum/src/cli.rs # crates/edgezero-adapter-axum/src/dev_server.rs # crates/edgezero-cli/src/dev_server.rs # crates/edgezero-core/src/lib.rs
# Conflicts: # .github/workflows/test.yml # crates/edgezero-adapter-spin/src/lib.rs # crates/edgezero-adapter-spin/src/request.rs # crates/edgezero-adapter-spin/src/templates/src/lib.rs.hbs # crates/edgezero-core/src/config_store.rs # crates/edgezero-core/src/manifest.rs # crates/edgezero-core/src/secret_store.rs
The key_value_store and secret_store modules are already gated at their mod declaration in lib.rs (#[cfg(all(feature = "spin", target_arch = "wasm32"))]), so every per-item copy of that same cfg inside the files was a tautology. Removing the 14 no-op attributes makes both files consistent with their sibling request.rs/response.rs/proxy.rs, which already rely on the gated mod declaration.
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