Skip to content

Adopt edgezero strict-clippy gate and PR #257 API#108

Open
aram356 wants to merge 11 commits into
mainfrom
chore/edgezero-strict-clippy
Open

Adopt edgezero strict-clippy gate and PR #257 API#108
aram356 wants to merge 11 commits into
mainfrom
chore/edgezero-strict-clippy

Conversation

@aram356
Copy link
Copy Markdown
Contributor

@aram356 aram356 commented May 18, 2026

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

  • All edgezero-* workspace deps point at branch = "chore/strict-clippy" pending PR #257 merge.
  • Root Cargo.toml adopts [workspace.lints.clippy] from edgezero: pedantic = warn, restriction = deny, 13-entry allow-list. [workspace.lints.rust] denies unsafe_code.
  • New clippy.toml mirrors edgezero's test exemptions.
  • [lints] workspace = true added to all four crate Cargo.tomls.
  • Migrated to the new edgezero API: Router::oneshot returns Result, Body is Once/Stream with Option-returning bytes, run_app moved to dev_server::run_app, IntoResponse::into_response() fallible.
  • All eleven route handlers now return Result<Response, EdgeError> for a uniform fallibility contract.

Refactor approach

~700 clippy findings resolved through real refactors per the upstream principle:

  • struct fields, enum variants, impl methods reordered alphabetically (~300 sites);
  • numeric literals typed (11_i32, 2.52.5_f64);
  • single-char idents renamed where they don't carry spec meaning;
  • #[inline] added to public fns/methods;
  • test_ prefix dropped from test fn names;
  • as casts replaced with f64::from(i32::try_from(...)) / saturating arithmetic;
  • integration tests wrapped in #[cfg(test)] mod tests to satisfy tests_outside_test_module;
  • OpenRTB w/h fields renamed to width/height with #[serde(rename = "w"/"h")] to preserve JSON wire format;
  • Site._refr#ref (raw identifier, default-serialises to ref);
  • Geo._typekind with #[serde(rename = "_type")].

Remaining exceptions

Five call-site #[expect(..., reason = "...")] annotations remain, each documenting a genuine constraint:

Site Lint Constraint
auction.rs::get_cpm float_arithmetic CPM is f64 by API
render.rs::svg_layout 7-lint layout-math cluster i64 banner-dimension math on the SVG layout path
adapter-axum/main.rs print_stderr startup-error path before logger init
adapter-cloudflare/main.rs print_stderr host-side stub reminding operator to target wasm32
adapter-fastly/main.rs print_stderr (+ a documented dead_code cfg_attr) same wasm32 stub pattern

Test plan

  • cargo check --workspace --all-targets
  • cargo 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 -- --check

Follow-up

When stackpop/edgezero#257 merges to main, flip the branch = "chore/strict-clippy" entries in the root Cargo.toml back to branch = "main" and rerun cargo update.

aram356 added 7 commits May 18, 2026 15:04
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.
Copy link
Copy Markdown
Contributor

@prk-Jr prk-Jr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.kind wire key vs. OpenRTB spec — the kind field now maps to "_type" (not the spec's "type"), while type2 carries 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 { .. } discards pid silently — correct for a mock, but a comment explaining the intentional discard prevents future "is this a bug?" reads. (routes.rs:347)
  • run_in_browser but Node is the actual runnerwasm_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::Instant in verification.rsLazyLock<Mutex<…>> and Instant are not available on wasm32-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 because Cargo.lock is 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 blanket restriction. 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 test cannot 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)

Comment thread crates/mocktioneer-core/src/openrtb.rs
pub ext: Option<serde_json::Value>,
pub r#ref: Option<String>,
#[serde(skip_serializing_if = "Option::is_none")]
pub ref_: Option<String>,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/mocktioneer-core/src/routes.rs Outdated
Comment thread crates/mocktioneer-core/src/routes.rs
Comment thread crates/mocktioneer-adapter-cloudflare/tests/contract.rs
Comment thread crates/mocktioneer-adapter-cloudflare/Cargo.toml Outdated
aram356 added 3 commits May 20, 2026 20:14
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.
@aram356
Copy link
Copy Markdown
Contributor Author

aram356 commented May 21, 2026

Thanks for the review @prk-Jr. Addressed in 5ae2cd5:

Finding Resolution
Geo.kind wire key vs spec Doc comments added to kind (non-spec _type compat) and type2 (spec Geo.type).
Site.ref_ spec field Doc comments added to r#ref (spec Site.ref) and ref_ (non-spec legacy-compat field).
Empty impl SizeDimensions {} Removed — leftover from the reorder pass.
PixelQueryParams { .. } discards pid Comment added: pid is validated on extraction, intentionally unused.
run_in_browser misleading Comment added explaining the runner (CARGO_TARGET_WASM32_UNKNOWN_UNKNOWN_RUNNER) selects the headless-Node environment.
wasm-bindgen-test = "0.3" range Pinned to =0.3.71 to match the CI CLI version-pin.
WASM Instant/Mutex in verification.rs Pre-existing; tracked as a follow-up in #109 (Instant::now() panics on wasm32-unknown-unknown).

All gates still green: clippy (-D warnings), 119 tests, fmt.

@aram356 aram356 requested a review from prk-Jr May 21, 2026 05:39
Copy link
Copy Markdown
Contributor

@ChristianPavilonis ChristianPavilonis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adopt edgezero strict-clippy gate and PR #257 API

3 participants