Adopt edgezero strict-clippy gate and PR #257 API#108
Conversation
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.
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.
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).
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.
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.
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.
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
Thoroughly executed strict-clippy adoption. ~700 findings resolved through real refactors (not #[allow] suppression), new edgezero API migration is complete, and the wire-format round-trip tests are exactly the right safety net for a rename-heavy change. All CI gates pass locally. No blocking issues — two questions about field naming intent and one pre-existing WASM compatibility concern worth tracking separately.
Findings
❓ Questions
Geo.kindwire key vs. OpenRTB spec — thekindfield now maps to"_type"(not the spec's"type"), whiletype2carries the spec-conformant key. This looks intentional and the round-trip test confirms it, but doc comments distinguishing the two fields would prevent future confusion. (openrtb.rs:405)Site.ref_— what spec field does this represent? —ref_serialises to"ref_"literally with no serde rename; no OpenRTB 2.5/3.0 spec field maps to that key. Worth a doc comment noting whether this is a compat shim or extension. (openrtb.rs:275)
🤔 Thoughts
- Empty
impl SizeDimensions {}— dead impl block, likely leftover from the reorder pass. (routes.rs:127) PixelQueryParams { .. }discardspidsilently — correct for a mock, but a comment explaining the intentional discard prevents future "is this a bug?" reads. (routes.rs:347)run_in_browserbut Node is the actual runner —wasm_bindgen_test_configure!(run_in_browser)is misleading; a comment noting the env-var fallback to Node would help. (contract.rs:13)- WASM compatibility:
std::sync::Mutex+std::time::Instantinverification.rs—LazyLock<Mutex<…>>andInstantare not available onwasm32-unknown-unknown. This pre-dates the PR and the new WASM contract tests don't exercise the verify path, so it's hidden. Not a regression here, but the new WASM CI matrix makes it the right moment to track this in a follow-up issue. (verification.rs:10-16)
📝 Notes
wasm-bindgen-test = "0.3"uses a semver range — safe becauseCargo.lockis committed, but inconsistent with the explicit CLI version-pin in CI. Consider= "0.3.71"to match. (mocktioneer-adapter-cloudflare/Cargo.toml:31)
👍 Praise
- Wire-format round-trip tests — the seven serde tests in
openrtb.rs(lines 582–703) are the right safety net for this rename-heavy PR. They pin JSON key names independently of struct field names, so a future accidental serde annotation removal becomes a test failure, not a silent wire-format change. #[expect(..., reason = "...")]usage — all five exception sites are correctly documented with accurate constraint descriptions, and each suppresses individual lints rather than using a blanketrestriction. This is exactly how these annotations should be used.- WASM contract test matrix — moving from a compile-check matrix to an actual execution matrix (Fastly via Viceroy, Cloudflare via wasm-bindgen-test-runner) is a meaningful CI improvement. This will surface adapter-dispatch regressions that host-only
cargo testcannot catch. - Alphabetical field ordering — applied systematically and completely across all changed files. Predictable field lookup without requiring a secondary index.
CI Status
- fmt: PASS
- clippy: PASS
- tests: PASS (90 tests)
| pub ext: Option<serde_json::Value>, | ||
| pub r#ref: Option<String>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub ref_: Option<String>, |
There was a problem hiding this comment.
❓ What spec field does ref_ map to?
The struct now has both r#ref (→ "ref", the OpenRTB 2.5 page referrer URL) and ref_ (→ "ref_" literally, no serde rename). The round-trip test confirms ref_ serialises to the key "ref_", which doesn't map to any field in the OpenRTB 2.5/3.0 spec.
Is ref_ a legacy field kept for backward compat with an older wire format, or a project-specific extension? A brief doc comment would clarify intent and indicate whether it's a candidate for future removal.
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.
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.
Integrates PR #80 (Trusted Server v1.1 signature verification) and the fixed-price auction rework into the edgezero strict-clippy branch. Conflict resolutions: - auction.rs: took main's pricing model (FIXED_BID_CPM, STANDARD_SIZES array, largest-area APS selection, ignored bid override); re-applied this branch's OpenRTB width/height field names and strict-clippy compliance. SIZE_MAP/get_cpm/phf are gone. - verification.rs: took main's Trusted Server v1.1 verification; re-applied the edgezero #257 Body/HTTP API shape and strict-clippy style (inline, ident renames, doc punctuation, from_mins). - routes.rs: handle_sizes keeps the Result<Response, EdgeError> return type; drops the cpm field and the get_cpm import (no longer exists). - render.rs / aps_endpoints.rs: kept this branch's #[cfg(test)] mod wrapping and test renames; folded in main's new tests and the largest-area rename. - Cargo.toml: phf dropped, js-sys added (both from main). All gates green: clippy (workspace, all-targets, all-features, -D warnings), 119 tests, fmt, "fastly cloudflare" feature check, and both wasm-target builds.
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.
|
Thanks for the review @prk-Jr. Addressed in 5ae2cd5:
All gates still green: clippy ( |
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Review of PR #108 found one blocking e2e test-registration issue plus two non-blocking comments. Rust/docs gates passed locally; Playwright passed but only registered 2 tests, which is the coverage issue below.
Folded into body
🔧 Playwright only registers 2 tests because AD_SIZES is populated too late — tests/playwright/creative-visibility.test.ts:40
AD_SIZES starts as [] and is populated in test.beforeAll, but Playwright registers tests synchronously while loading this module. The loop at line 40, and the second loop at line 103, therefore runs over an empty array, so the per-size tests are never registered. Local npm test only reported 2 tests instead of the intended per-size suite.
Suggestion: Use a static supported-size list for test registration and assert /_/sizes matches it, or move the dynamic iteration inside an async test with test.step after fetching sizes:
test('all supported SVG creatives render visibly', async ({ request, page }) => {
const response = await request.get('/_/sizes')
const data = (await response.json()) as { sizes: AdSize[] }
for (const size of data.sizes) {
await test.step(`${size.width}x${size.height}`, async () => {
await page.goto(`/static/img/${size.width}x${size.height}.svg`)
// existing assertions...
})
}
})📝 OpenRTB response example omits the signature-status query parameter — docs/api/openrtb-auction.md:107
iframe.html.hbs appends {{#if SIG}}&sig={{SIG}}{{/if}}, and iframe_html always sets SIG from metadata.signature.url_param(). For a minimal unsigned request, the creative URL includes &sig=not_present.
Suggestion: If this response example is intended to be exact, include &sig=not_present in the adm URL or add an ellipsis/note that the markup is abbreviated.
| use worker::wasm_bindgen::JsCast as _; | ||
| use worker::{Context, Env, Method as CfMethod, Request as CfRequest, RequestInit}; | ||
|
|
||
| // `run_in_browser` selects the browser harness, but CI runs these tests |
There was a problem hiding this comment.
⛏ Cloudflare contract-test comment says Node, but the test is configured for a browser harness: This file calls wasm_bindgen_test_configure!(run_in_browser), and GitHub logs for this PR show Running headless tests in Firefox. A local run also selected a browser harness. The comment saying CI runs under Node is inaccurate.
Suggestion: Either remove run_in_browser if Node is intended, or update the comment to say this uses the wasm-bindgen browser harness, currently headless Firefox in CI.
Closes #107.
Tracks stackpop/edgezero#257 (
chore/strict-clippy) and pulls mocktioneer onto the same strict clippy gate plus the defensive-coding pass it ships.Summary
edgezero-*workspace deps point atbranch = "chore/strict-clippy"pending PR #257 merge.Cargo.tomladopts[workspace.lints.clippy]from edgezero:pedantic = warn,restriction = deny, 13-entry allow-list.[workspace.lints.rust]deniesunsafe_code.clippy.tomlmirrors edgezero's test exemptions.[lints] workspace = trueadded to all four crate Cargo.tomls.Router::oneshotreturnsResult,BodyisOnce/StreamwithOption-returning bytes,run_appmoved todev_server::run_app,IntoResponse::into_response()fallible.Result<Response, EdgeError>for a uniform fallibility contract.Refactor approach
~700 clippy findings resolved through real refactors per the upstream principle:
1→1_i32,2.5→2.5_f64);#[inline]added to public fns/methods;test_prefix dropped from test fn names;ascasts replaced withf64::from(i32::try_from(...))/ saturating arithmetic;#[cfg(test)] mod teststo satisfytests_outside_test_module;w/hfields renamed towidth/heightwith#[serde(rename = "w"/"h")]to preserve JSON wire format;Site._ref→r#ref(raw identifier, default-serialises toref);Geo._type→kindwith#[serde(rename = "_type")].Remaining exceptions
Five call-site
#[expect(..., reason = "...")]annotations remain, each documenting a genuine constraint:auction.rs::get_cpmfloat_arithmeticf64by APIrender.rs::svg_layoutadapter-axum/main.rsprint_stderradapter-cloudflare/main.rsprint_stderradapter-fastly/main.rsprint_stderr(+ a documenteddead_codecfg_attr)Test plan
cargo check --workspace --all-targetscargo check --workspace --all-targets --features "fastly cloudflare"cargo clippy --workspace --all-targets --all-features -- -D warnings(0 findings)cargo test --workspace --all-targets(90 tests pass)cargo fmt --all -- --checkFollow-up
When stackpop/edgezero#257 merges to
main, flip thebranch = "chore/strict-clippy"entries in the rootCargo.tomlback tobranch = "main"and reruncargo update.