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 46 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
… 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.
…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.
… deploy) The Fastly deployment guide only seeded Viceroy's local state; following the production flow deployed fail-loud OpenRTB/APS endpoints with no config blob. Adds the first-deploy sequence, the already-deployed resource-link caveat (Fastly consumes [setup] only when deploy creates a new service), and a re-push-after-config-change note.
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
Adapts Mocktioneer to edgezero v0.0.4 (extensible CLI, blob-model typed config, pluggable introspection routes). The change is well-scoped, thoroughly tested, and has clearly been through several review rounds. No blocking findings. Verdict is COMMENT — a handful of non-blocking questions, one build-cache cleanup, a design-level footgun to keep in view, and a stale doc line. Inline comments carry the file-specific items.
Findings
♻️ Refactor
- Dockerfile redundant manifest COPYs — six per-crate
Cargo.tomlcopies are superseded byCOPY crates ./cratesbeforecargo fetch, so they add no layer caching. Inline atDockerfile:16.
❓ Questions
anyhowin core with nosrc/usage — likely required by theAppConfigderive; please confirm, else drop. Inline atcrates/mocktioneer-core/Cargo.toml:9.bid_cpmaccepts+Inf—exclusive_min = 0.0passes infinity; upstream non-finite rejection isn't pinned by a test in this repo. Inline atcrates/mocktioneer-core/src/config.rs:15.
🌱 Seeds / 📌 Out of scope
- Unauthenticated introspection on prod adapters —
/_mocktioneer/{manifest,config,routes}publicly enumerable on cloudflare/fastly. Documented and safe today; footgun once anything sensitive enters the manifest. Inline atedgezero.toml:15. - Breaking runtime change —
/openrtb2/auctionand/e/dtb/bidnow return503 config_out_of_dateuntil an operator runsconfig push. This is the intended fail-loud behavior and is well documented, but existing deploys that upgrade will start erroring until they push. Please make sure this lands prominently in the release notes / CHANGELOG so it isn't a surprise on rollout.
⛏ Nitpicks
- Stale doc —
GET /_/sizesstill described "with pricing" though cpm is no longer echoed. Inline atREADME.md:51.
👍 Praise
- Fail-loud contract is genuinely well tested:
assert_config_out_of_date(503 +Retry-After+error.kind = config_out_of_date) exercised at the route level for both OpenRTB and APS, plus config-seeded auction contract tests through the real Fastly (Viceroy) and Cloudflare (wasm-bindgen) runtimes, and a Docker smoke test that gates image publication on an actual live bid. Strong end-to-end coverage of the new config-store path. - Security hygiene from the base is preserved throughout: constant-time token comparison, hostname / path-injection validation on
ts_domain, log sanitization, and#[serde(deny_unknown_fields)]on the typed config.
CI Status
Verified locally on the branch head:
- fmt: PASS
- clippy (
--workspace --all-targets --all-features -D warnings): PASS - tests (
--workspace --all-targets, host targets): PASS
Full remote CI (wasm adapter matrix, Playwright, Docker build + smoke, docs ESLint/Prettier) reported green.
…d_cpm guard, trim redundant Dockerfile COPYs, correct README - anyhow was added to mocktioneer-core by this PR but never referenced; the AppConfig derive does not need it. Full suite + clippy pass without it. - Add a test pinning that edgezero's loader rejects bid_cpm = inf/-inf/nan with InvalidValue. range(exclusive_min = 0.0) alone lets inf through, so this guarantee was relied on in a doc comment but untested here. - The six per-crate Cargo.toml COPYs were superseded by COPY crates ./crates before cargo fetch, buying zero cache reuse. Verified image still builds and the smoke test serves a bid. - /_/sizes no longer echoes cpm; drop "with pricing".
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Reviewed PR #110 against main. All reported CI checks pass, including Rust, adapter WASM, Docker smoke, Playwright, docs, and CodeQL. I found two non-blocking P2 issues; the CORS issue is inline.
The typed-config runtime wiring otherwise looks coherent, and I found no P0/P1 issues.
Additional P2 finding (not inline)
Trusted Server full-flow guide starts Axum without seeding typed config — docs/integrations/trusted-server.md:242-243
This standalone flow starts Axum directly, but it never copies mocktioneer.toml.example or runs config push. On a fresh checkout, the OpenRTB request in the later bidstream step now returns 503 config_out_of_date, so the documented flow cannot reach its verification step. Please add the config-copy and cargo run -p mocktioneer-cli -- config push --adapter axum --yes steps before starting the server. This was folded into the review body because the documentation file is unchanged in this PR and GitHub rejected an inline position for it.
| [[triggers.http]] | ||
| id = "introspect_manifest" | ||
| path = "/_mocktioneer/manifest" | ||
| methods = ["GET"] |
There was a problem hiding this comment.
P2 — Introspection routes omit CORS preflight
The three new introspection routes register only GET; there are no corresponding OPTIONS triggers. I reproduced OPTIONS /_mocktioneer/routes against the Axum adapter and it returns 405 Method Not Allowed (Allow: GET), while the documented CORS behavior says all routes have OPTIONS handlers. Browser/admin clients using custom headers therefore cannot reliably call these unauthenticated endpoints.
Please add matching OPTIONS triggers using the existing options handler and add route-level coverage for the preflight behavior.
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.