Adapt Mocktioneer to edgezero v0.0.4 (extensible CLI, typed config, introspection routes)#110
Adapt Mocktioneer to edgezero v0.0.4 (extensible CLI, typed config, introspection routes)#110aram356 wants to merge 44 commits into
Conversation
…tures, pricing docs, docker, prettier exclusion, CLI story)
…owers/ in docs prettier
…I seed, dockerfile cache-hygiene, docs build-exclusion, broader CLI docs)
…t(?Send) test impl, spin release build in verify)
…6, add clap/anyhow
…efault FIXED_BID_CPM
…er-cli, config gate
… (edgezero #269 removed request::dispatch)
…n/typed-config, add spin to agent builds, fix stale pricing/cli comments
…po-wide, update CLAUDE.md crate count/CPM
…ples; fix stale fixed-CPM comments
…r, fail-loud), config diff, CI/docs
…ush prereq to quickstarts; patch spin in local overlay
…onfig,routes}); pin edgezero to introspection-routes branch
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Reviewed PR #110 (feature/edgezero-extensible-cli) against main. The typed-config wiring is mostly coherent and CI is green, but I found two blocking deployment/runtime compatibility issues plus two medium follow-ups. Details are inline.
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
Adapts Mocktioneer to the breaking edgezero #269 (extensible CLI): typed MocktioneerConfig with runtime bid_cpm resolution via the fail-loud AppConfig extractor, a mocktioneer-cli crate, a new Spin adapter (wasm32-wasip2), introspection routes, [stores.config] plumbing, and a workspace-wide clippy-restriction gate. All local CI gates pass. Solid, well-tested work — the only merge-relevant question is the edgezero dependency pin target.
Note: the PR description still reads "docs-only — design + plan, no implementation code yet." The branch now carries the full implementation (~6.5k insertions). Worth updating the body so reviewers see the real scope.
Findings
❓ Questions
- edgezero dep pin (
Cargo.toml:32): pinned toworktree-feature+introspection-routesrather than thefeature/extensible-clithe docs describe. Reproducible today (Cargo.lock locks the SHA), fragile if that branch moves. See inline.
🌱 Seeds / 📌 Out of scope
- Env-var security on Workers (
routes.rs):MOCKTIONEER_TS_DOMAINSopen-redirect allowlist andMOCKTIONEER_PULL_TOKENresolve-auth arestd::env::var-gated, so both no-op on Cloudflare. Pre-existing; candidate to fold into the new[stores.config]. See inline. - Stale PR description: says docs-only; is a full implementation.
⛏ Nitpicks
- Dockerfile base drift (
Dockerfile:39): runtimedebian:stable-slim(rolling) vs builderbookworm(pinned). Pinbookworm-slimto match. Docker build not exercised by the local Rust CI.
🤔 Thoughts
- The clippy-
restrictionmechanical sweep (literal suffixes, struct-field reordering,as _imports,to_owned) is interleaved with the substantive logic changes, which makes the real diff harder to isolate. Future gate adoptions could land the reformat as its own commit/PR.
👍 Praise
- Mediation is now deterministic:
HashMap→BTreeMapplusunwrap()→let-else, with first-bidder-wins tie-break preserved (mediation.rs:160). - Constant-time bearer comparison over SHA-256 digests (avoids length leak); thorough
is_valid_hostnamerejecting IP literals / path / auth / port / fragment injection, with matching tests. - CI seeds a non-default
bid_cpmand asserts it round-trips throughconfig push— catches wiring regressions a default-value test would miss. - Spin adapter correctly gated to
wasm32with a#[expect(...)]-documentedunsafe_codeallowance, plus a wasip2 contract test in the CI matrix.
CI Status (run locally on this branch)
- fmt: PASS
- clippy (
--workspace --all-targets --all-features -D warnings): PASS - tests (
cargo test --workspace --all-targets): PASS
… gate uses example, APS route-level cpm tests, document Spin spin-up limitation
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
Solid adaptation to edgezero #269: typed MocktioneerConfig with fail-loud AppConfig wiring on the OpenRTB/APS paths, a thin mocktioneer-cli over the edgezero-cli library, the Spin wasip2 migration, Docker config seeding + smoke test, and CI config gates — with the docs updated everywhere the fail-loud model is user-visible. No blocking code issues; the remaining asks are PR metadata and small polish (4 inline comments below).
Findings
📝 Notes
- Stale PR title/body — please update before merge: the body still says "no implementation code yet… This PR is docs-only", but the branch now ships the full implementation (typed config + fail-loud
AppConfigwiring, themocktioneer-clicrate, Spin wasip2 migration, Docker config seeding + smoke test, CI config gates, introspection routes). Reviewers and the audit trail read the description first — please rewrite the title/body to describe what actually merges.
📌 Out of Scope
- Re-pin tracking: the
worktree-feature+introspection-routespin is acknowledged as intentional (introspection routes only exist there). Suggest opening a tracking issue to re-pin to edgezeromainonce #269 + the introspection branch land, so a throwaway-named branch doesn't rot underneath a merged main (Cargo.toml:26-31).
🌱 Seeds
- Introspection auth:
/_mocktioneer/{manifest,config,routes}are unauthenticated by design and the docs warn about it correctly. A future option: a shared-token middleware for public deployments, so the route block can stay bound inedgezero.tomlwithout a network-layer prerequisite.
👍 Praise
- Fail-loud test coverage: seeded-cpm + missing-config tests on both the OpenRTB and APS paths, a CI step that round-trips a non-default
0.35into the blob envelope, and a Docker smoke test that proves the0.0.0.0bind and an out-of-the-box auction. This is the right shape of evidence for a behavior-changing config model. - Docs discipline: the config-push prerequisite is surfaced on every quickstart, adapter, API, and integration page, and the
MOCKTIONEER__*env-overlay semantics (applied at push time, not runtime) are spelled out precisely.
CI Status
- fmt: PASS (verified locally)
- clippy: PASS (per CI)
- tests: PASS (per CI)
v0.0.4 (9e661ae5) is a superset release containing both the extensible CLI (edgezero #269) and the pluggable introspection routes (#300), replacing the transient worktree branch pin with a stable, durable tag.
…g, config_registry helper, drop dead unsafe_code allow
aram356
changed the title
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Reviewed PR #110 at 0bd3e152 against main. The core typed-config and pricing flow is coherent, but this review found one high-risk release-workflow problem and three medium test/documentation gaps. Current CI is green; the previously identified Spin runtime ABI incompatibility also remains a known operational limitation.
…exact 503 config_out_of_date contract, fresh-clone getting-started validate
…apters Adds a contract test to each adapter's existing wasm test binary that seeds a non-default bid_cpm (0.35) in an in-process ConfigStore, injects it via the service builder's with_config_handle, and dispatches POST /openrtb2/auction — covering request translation, the config-store binding, the AppConfig extractor, the auction handler, and response translation under the real wasm runtime (Viceroy / wasm-bindgen). Runs in the existing adapter-wasm-tests CI matrix; no new emulator infra. Spin equivalent waits on the upstream wasi:http ABI fix.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Approved. The typed-config pricing path, adapter bindings, Docker behavior, and CLI wiring are coherent, and all current CI checks pass. I left two non-blocking P2 notes inline: preserving the existing public builder signatures and completing the Fastly remote config deployment instructions.
The previously documented Spin runtime ABI limitation remains an acknowledged operational constraint.
| req: &OpenRTBRequest, | ||
| base_host: &str, | ||
| signature_status: SignatureStatus, | ||
| cpm: f64, |
There was a problem hiding this comment.
P2 — Preserve the existing public builder API
Adding the mandatory cpm argument here, and to build_aps_response, makes existing downstream calls fail with Rust E0061. The runtime wiring does not require breaking those public signatures. Please consider retaining the original functions as wrappers using FIXED_BID_CPM, and adding configured variants such as build_openrtb_response_with_cpm and build_aps_response_with_cpm for the handlers. The compatibility risk is limited because this crate is publish = false, so I am treating this as non-blocking.
|
|
||
| ```bash | ||
| cp mocktioneer.toml.example mocktioneer.toml | ||
| cargo run -p mocktioneer-cli -- config push --adapter fastly --local |
There was a problem hiding this comment.
P2 — Document the corresponding remote Fastly setup
This correctly seeds Viceroy's local state, but the production deployment section below only runs deploy; it never provisions or pushes the remote Fastly Config Store. A user following that flow deploys fail-loud OpenRTB/APS endpoints without their required blob. Please add the first-deploy sequence using mocktioneer-cli provision --adapter fastly, mocktioneer-cli config push --adapter fastly --yes, and then deploy, plus a note to re-push after config changes.
…dition 2024 The Spin description claimed wasm32-wasip2, but on main the adapter builds wasip1: spin-sdk is 5.2, edgezero.toml declares wasm32-wasip1, and the CI matrix runs the spin contract tests under wasip1 via wasmtime. wasip2 arrives with the spin-sdk 6 migration in #110, which adds its own wasip2 CI entry. Also update CLAUDE.md, which still declared Edition 2021.
3 participants
Closes #115
What this PR does
Adapts Mocktioneer to the released edgezero
v0.0.4, which lands the extensible CLI (stackpop/edgezero#269) and the pluggable introspection routes (#300). The design spec and implementation plan that opened this PR are included underdocs/superpowers/; the implementation now follows them end to end.Changes
edgezero-*git deps to the immutable tagv0.0.4; drop theinclude_str!(manifest)arg from every adapterrun_app; migrate the Spin adapter tospin-sdk ~6.0/wasm32-wasip2(Fastly stayswasm32-wasip1).MocktioneerConfig(bid_cpm,range(exclusive_min = 0.0)), a gitignoredmocktioneer.tomlwith a committedmocktioneer.toml.exampletemplate, and amocktioneer-clicrate mirroring edgezero's generated<name>-cli(config validate/diff/push,auth/build/deploy/provision/serve)./openrtb2/auction) and APS (/e/dtb/bid) handlers readbid_cpmfrom the bound config store via edgezero's typedAppConfigextractor. A fresh deploy mustconfig pushonce before those endpoints serve bids — until then they error.FIXED_BID_CPM = 0.20is the shipped default value, not a runtime fallback. Other endpoints (static/creative/pixel) are unaffected.config pushwrites the whole struct as a single canonical-JSON blob envelope (SHA-gated for drift detection) under the store's key, per adapter (axum local file / cloudflare KV / fastly config store / spin KV)./_mocktioneer/{manifest,config,routes}routes fromedgezero_core::introspection(unauthenticated — restrict at the network/middleware layer;[environment.secrets]are redacted).[stores.config]in the manifest + Spinruntime-config.toml; Dockerfile seeds the default config blob and binds0.0.0.0; docs (wasip2, fail-loud typed config, introspection, edgezero-cli-vs-mocktioneer-cli); aconfig validate --strict --app-config mocktioneer.toml.exampleCI gate + a Docker smoke test.Notes
main, which already carries the strict-clippy + edgezero PR #257 adaptation (via PR Adopt edgezero strict-clippy gate and PR #257 API #108).v0.0.4(immutable), so no post-merge re-pin is needed.spin upis blocked upstream: the adapter compiles towasm32-wasip2and passes thewasmtimerouter contract tests, butspin upon current Spin runtimes fails to link becausespin-sdk 6.0.0pullswasi:http@0.3.0-rc, which no released Spin provides. This is a spin-sdk/runtime ABI mismatch to resolve upstream; Fastly / Cloudflare / Axum are unaffected.CI
fmt·clippy -D warnings·test --workspace·check --features "fastly cloudflare"·config validate --strict· Docker build + smoke · Playwright · docs ESLint/Prettier — all green.