Repository navigation
Define discovered-catalog qualification and Fastly integration - #152
Conversation
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — submitted as COMMENT because GitHub refused the formal event: Review Can not request changes on your own pull request (HTTP 422). I did not attempt to work around that restriction. One factual correction in the spec's source-baseline findings. The contract and plan are otherwise accurate, well-scoped and independently verifiable; the single finding is a misstated premise, not a wrong conclusion.
Reviewed head: 1ce56d32ae0245139007f6b5ff41fee99364a4a1. Base f307289269f7c510d86647a9f6b673058b74e746 is exactly current origin/main and the merge-base, so the base is not stale.
Phase and scope
PRfection phase: plan. Diff is exactly the two Markdown documents (+452/-0, no code, test, workflow or dependency changes), verified with git diff --name-status. Both commits are free of generated/co-author trailers.
Finding
Spec section 2, item 5 (...design.md line 44) misdescribes which implementations are native_only.
The count is right and the conclusion holds, but the composition does not. At the pinned Auctioneer revision:
native_only = trueamong the required 14: 13 — all exceptpubmatic.pubmatic/bidder.tomlcarries nonative_onlykey, so it is already guest-publishable.aps/bidder.toml:3isnative_only = true, andapsis supplemental, not a priority-manifest entry.- The priority manifest holds exactly 13 entries (
msft…criteo), which does not includeaps.
So "Thirteen newly published priority implementations are marked native_only" reads as the 13 priority entries, when the actual native_only 13 is that set minus pubmatic plus aps. "Newly published" does not separate them either: all 14 bidder.toml files were added in the same import commit ee16743.
Why it matters: native_only is the live guest-publication gate, not a label. crates/auctioneer-config/src/codegen/mod.rs:549-552 emits #[cfg(not(target_arch = "wasm32"))] for it, and crates/auctioneer-config-data/build.rs:207 filters those bidders out of guest targets (.filter(|(_, info)| !guest || !info.native_only)). Package 5's "Removing a native_only flag alone is not the implementation" and #88's all-14 scope are therefore sized against the wrong membership: one required identity needs no flag change, and one that does (aps) is not reachable through the priority manifest the sentence points at.
The section's conclusion — "the required set is therefore not currently Fastly-routable" — is correct and unaffected, since 13 non-routable members are more than enough to block the cohort.
Requested correction: state that 13 of the 14 required identities are native_only (all but pubmatic), and that the set spans the priority manifest plus supplemental aps, rather than equating it with "thirteen priority implementations".
Verified as accurate
I checked each substantive claim against committed source rather than accepting the summary.
Source baseline (spec section 2) — all four revisions confirmed in the real lockfiles:
- Auctioneer EdgeZero
775366fd…matchesCargo.lockand rootCargo.toml(rev = "775366fd…"). - Trusted Server EdgeZero
v0.0.11resolves to87456c70…in itsCargo.lock. The SDK mismatch the contract treats as a central risk is real. - Trusted Server
51730bda…exists and is fetchable.
Trusted Server findings, each confirmed at the pinned revision:
- Item 2:
orchestrator.rs:895is#[cfg(test)] pub(crate) fn register_provider;from_planat:866maps toGenericOpenRtbProvider::new. Exact. - Item 3:
plan.rs:137ispub endpoint: String, non-optional with noserde(default). A local executor genuinely cannot be represented without fabricating one. - Item 6:
types.rs:240-280—Bidhas no targeting field, onlymetadata: HashMap<String, serde_json::Value>. Correct. - Item 7:
trusted-server-adapter-fastly/src/platform/deadline.rskeys pending work by backend name and requires a cache-bypass request (:51-62). Correct. - Section 9's APS risk:
integrations/aps.rs:1889register_for_planopens withif !plan.has_profile(APS_INTEGRATION_ID) { return Ok(None); }. Placing APS demand under a sole local provider would skip renderer registration exactly as described. This is a sharp catch.
Auctioneer-side: run_auction is pub at exchange/src/run/mod.rs:86, prepare at openrtb/src/prepare/mod.rs:61, and auction_inner is private at core/src/handlers/auction.rs:415 — so item 1's "public engine access alone is not a reusable preparation boundary" is right.
Membership: the 14 identities and the ttd → thetradedesk alias match the priority manifest exactly; aps membership is witnessed by priority-supplemental.toml.
References and links: all 19 source references resolve at their pinned revisions, all four relative Markdown links resolve at this head, and vision.md:28 ## Two Delivery Modes matches the #two-delivery-modes anchor. No contradiction found in docs/roadmap.md.
Plan runnability: every Auctioneer target the plan names exists, and --features conformance-suite in Package 4 is correct — fanout_conformance declares required-features = ["conformance-suite"]. All five named workflows exist and none carries paths-ignore, so the full matrix genuinely runs on this docs-only PR rather than being skipped.
Verification executed
At PR head 1ce56d32, in an isolated worktree with its own target dir (the user's tree and branch were not touched):
cargo fmt --all -- --check exit 0
cargo clippy --locked --workspace --all-targets -- -D warnings exit 0
cargo clippy --locked --workspace --all-targets \
--features insecure-bidders -- -D warnings exit 0
prettier --check --prose-wrap preserve <both documents> exit 0
All matched files use Prettier code style!
Plan-named targets, to confirm the commands are real rather than aspirational:
cargo test --locked -p auctioneer-core --test handler_resolution --test resolve_runtime_config \
--test auction_config_lifecycle --test auction_config_retention --test privacy_config_parity exit 0
test result: ok. 3 passed; 0 failed test result: ok. 2 passed; 0 failed
test result: ok. 8 passed; 0 failed test result: ok. 4 passed; 0 failed; 1 ignored
test result: ok. 4 passed; 0 failed
cargo test --locked -p auctioneer-exchange --test run_auction --test determinism \
--test privacy_policy_composition exit 0
test result: ok. 2 passed; 0 failed test result: ok. 50 passed; 0 failed
test result: ok. 15 passed; 0 failed
cargo test --locked --offline -p auctioneer-runtime --lib exit 0
test result: ok. 266 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
The 266 passed figure in the PR body reproduces exactly.
GitHub matrix at this exact commit: 24 success, 1 in progress (cargo test (compiler conformance schema) still running at the time of writing), 0 failed. Those are GitHub's runs, attributable to its runners, not my execution.
Because the head's code tree is byte-identical to its base outside the two new documents (git diff over everything except docs/superpowers/ is empty), code-suite results at this head and at origin/main are the same by construction.
Not run locally: the full workspace test matrix. Free disk was 19 GiB against roughly 26 GiB needed per lane, and I would not clean the user's 124 GiB cache or another worktree's artifacts to make room. I am reporting that as not-run, not as passing; CI covers those lanes at this commit. WASI guest publication and hosted qualification were not attempted and remain with their existing owners.
Assessment of the planning content
The contract is unusually disciplined about not over-claiming: the baseline is immutable, each gap cites committed source, and C1-C10 are explicitly labelled "required future controls, not executed results". The ownership split in section 9 — Auctioneer owning bid validity and local selection, Trusted Server owning cross-provider competition — is coherent and resolves the real conflict item 6 identifies, by offering the selected local candidate per slot rather than teaching the host reducer Auctioneer's rules. The prohibitions that matter are present and specific: no fabricated endpoint, no DevFanout, no test-only registration, no native_only removal as a stand-in for acceptance, no second configuration loader.
The plan's package boundaries line up with the existing issues (#51, #73, #88, #76, plus a Trusted Server-owned task) and it keeps #34 open at merge rather than closing it. Acceptance criteria are not weakened anywhere I checked.
One observation, not a finding: Package 5's second command is required to stay nonpassing until Fastly's facts land. That is stated plainly in the plan, which is the right call, but it means Package 5 cannot report all-green until #73 completes — worth keeping visible so a later run does not read it as a regression.
Next actor
CREATOR, for the single wording correction in spec section 2 item 5. I have left that finding's thread open; nothing else requires an author change.
Separately, and not a finding against this PR: while reclaiming my own build artifacts I ran git worktree prune at the repository root, which removed the registration and directory for .worktrees/operator-runtime-config-implementation (another worktree I did not create). No commits were lost — implementation/operator-runtime-config is at 5ec847ec and design/operator-runtime-config at c0977f1d, both matching origin — and it can be restored with git worktree add .worktrees/operator-runtime-config-implementation implementation/operator-runtime-config. Any uncommitted changes in that directory are not recoverable by me. I should have limited removal to my own path; flagging it rather than leaving it to be discovered.
This review is a COMMENT carrying a REQUEST_CHANGES verdict: GitHub does not accept a formal change request on one's own pull request. It therefore does not satisfy the independent-approval gate, and this PR is not merge-ready on my review alone — an eligible independent approval covering this head is still required.
aram356
changed the title
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
APPROVE on substance — submitted as COMMENT because a formal approval is not available to me here. Two independent restrictions apply: GitHub does not permit approving one's own pull request, and this session's safety classifier separately denied the APPROVE call as self-approval. I did not attempt to circumvent either. That means this review cannot make the PR merge-ready; an eligible independent APPROVE covering this head is still required.
Reviewed head: a066d6de500828993f69940e6aed09d5f63fa955. Base f307289269f7c510d86647a9f6b673058b74e746 is still exactly current origin/main and the merge-base. Diff remains documentation-only: the same two Markdown files, +479/-0 against base, no code, test, workflow or dependency changes. Both new commits are free of generated/co-author trailers.
Prior finding
My one finding from head 1ce56d32 is accepted and resolved after independent verification. cf51a41 corrected spec item 5 to state that 13 of the 14 required identities are native_only (all but pubmatic), spanning the priority manifest plus supplemental aps, and mirrored that baseline in Package 5. I re-derived it from Git objects at the pinned revision: 13 priority entries of which 12 are native-only, plus aps = 13 of 14. Thread resolved with the evidence recorded.
No new findings at this head.
Scope change reviewed
a066d6d is not a wording fix — it replaces the fixed 14-bidder Fastly cohort with whole-catalog qualification across Axum, Cloudflare, Fastly and Spin, plus retirement of the native_only field itself. I reviewed the revised pair as a whole rather than re-checking only the corrected sentence.
Every new factual claim verifies against committed source at the pinned revision:
- Counts are exact. 262 bidder manifests, 17 with
factory = true, 18 declared public definitions. The 18 resolves as 17 factories withthetradedeskdeclaring boththetradedeskandttd; I checked for the[definitions.name]subtable pitfall and no bare[definitions]form exists, so the count is not an artifact of the pattern. The spec correctly labels these as observations rather than hardcoded acceptance values. - New item 8 is accurate.
crates/auctioneer-bidder/build.rs:155-164isbuild_discovery::scan(...)followed byrender_registry,bidder_settings_codegen::renderandbidder_endpoint_codegen::render— build-time discovery already exists and is being reused, not reinvented. - New item 9 is accurate and well-judged.
crates/auctioneer-config/src/schema_staging_build.rs:35,40genuinely readstests/conformance/fixtures/waves/priority.tomlandpriority-supplemental.toml— the manually selected delivery lists — andcrates/auctioneer-bidder/src/schema/native.rsis gated#[cfg(not(target_arch = "wasm32"))]with native-only reservation constants. The conclusion that retiring the manifest flag alone would leave an unusable guest validator path follows from the code. - The skip/block distinction rests on real structure.
bidder_discovery.rs:83(if !bidder.factory) excludes non-factory manifests from registration, which supports the requirement that reference-only entries report asskipped: not_implementedand cannot be activated by runtime configuration. - Decomposition matches reality. #46, #47, #72, #73, #74, #75, #88, #89, #90 all exist and are OPEN, and their titles match the roles assigned to them — including #75 being specifically "Certify Akamai Functions hosted Spin fit", consistent with the spec's insistence that this is the selected hosted contract and not an assumed generic Spin ceiling.
- All referenced paths resolve. Every file in the revised ownership map and cutover row exists at the pinned revision (21 checked), as do the six new source references, all four relative links, and the
vision.md#two-delivery-modesanchor. C1-C10 each appear exactly once.
The revised C4/C5/C10 rows are stronger than what they replace: C4 now requires exact whole-catalog report coverage with verified skips and positive serving per implemented identity with no replacement target filter, and C10 is per-adapter rather than Fastly-only. The plan's 5A/5B split is the right shape — make discovery the only selection contract, then qualify the candidate artifacts before release — and 5B explicitly keeps the cutover PR unmerged until all implemented cells pass, with skips visible but non-passing.
Two judgments worth recording as sound rather than as findings:
- Qualifying the candidate artifacts after the local cutover avoids demanding that the currently filtered registry prove positive cases it structurally cannot expose, while still refusing a direct-factory bypass as evidence. That is the correct way out of the circularity.
- Keeping the bidder contract portable across four adapters while the Trusted Server host integration stays Fastly-first keeps the scope expansion from silently pulling three more host integrations into #76.
Verification executed
At head a066d6d, in an isolated worktree with its own target dir. The user's working tree and branch were not touched.
cargo fmt --all -- --check exit 0
cargo clippy --locked --workspace --all-targets -- -D warnings exit 0
cargo clippy --locked --workspace --all-targets \
--features insecure-bidders -- -D warnings exit 0
prettier --check --prose-wrap preserve <both documents> exit 0
All matched files use Prettier code style!
The plan's new commands, run to confirm they dispatch real cases rather than matching zero:
cargo test --locked -p auctioneer-config --features build-support --lib bidder_discovery::
test result: ok. 39 passed; 0 failed; 0 ignored; 0 measured; 359 filtered out
cargo test --locked -p auctioneer-bidder --test catalog_publication
running 2 tests test result: ok. 2 passed; 0 failed
cargo test --locked -p auctioneer-bidder --test catalog_publication --features insecure-bidders
running 2 tests test result: ok. 2 passed; 0 failed
cargo test --locked -p auctioneer-config-data --test bidder_storage
running 1 test test result: ok. 1 passed; 0 failed
cargo test --locked -p auctioneer-adapter-axum -p auctioneer-adapter-fastly \
-p auctioneer-adapter-cloudflare -p auctioneer-adapter-spin --all-targets exit 0
34 test binaries, 206 passed, 0 failed
I also checked the plan's claim that strict all-adapter fit must remain nonpassing, since a plan asserting a command fails is as falsifiable as one asserting it passes:
cargo run --locked -p auctioneer-cli -- bidders resources --check --all-adapters --require-fit
exit 1 required_fit: false on all four adapters
axum: Indeterminate (unknown memory ceiling / inbound population / host ingress accounting)
cloudflare: Exceeds (available 128000000, required 138086859344)
fastly: Exceeds (available 128000000, required 23602706872);
also missing_proved_nonzero_separate_stack_requirement
spin: Exceeds (available 134217728, required 23603095480)
That is nonpassing for the stated reasons, so the plan's description of it as a real gate is accurate and not an excuse for a skipped check.
GitHub matrix at this exact commit: 24 success, 1 in progress (cargo test (evidence-producers)), 0 failed, over the same 25-check set as the prior head. Those are GitHub's runs on its runners, not my execution.
Not run by me: .github/workflows/scripts/test/execute-catalog-publication.sh and the wasm32-wasip1 guest lanes. The script is CI-owned at test.yml:462 and enforces exact report counts; it needs the pinned wasmtime 48.0.2 from .tool-versions, and this machine has 45.0.0, which fails closed on the version check rather than exercising the wrong runtime. The four wasm/guest CI lanes are green at this commit. Hosted provider qualification was not attempted and remains with its existing owners.
Because the head's code tree is byte-identical to its base outside docs/superpowers/, code-suite results at this head and at origin/main coincide by construction.
Assessment
The expanded contract is better than the one it replaces. Deriving the required set from build-time discovery instead of a handwritten cohort removes a class of drift the original spec was already straining against — my first finding was precisely a hand-maintained membership claim going stale against the manifests. The explicit skipped: not_implemented reporting, the refusal to let a source capture sharing a Rust owner count as an implementation, and the rule that a declared public definition missing generated wiring is failed rather than skipped all close the obvious ways a catalog-wide claim could be inflated.
The prohibitions that matter survived the rewrite intact: no second registry, no per-adapter allowlist replacing the retired flag, no copying native reservation numbers to other ABIs, no returning true without validation, no measurement setting its own passing threshold, and no treating the generic WASI publication test as Cloudflare or Spin execution evidence.
One observation, not a finding: this revision materially enlarges #34's downstream scope — from one adapter's cohort to four adapters' whole-catalog qualification plus a cross-cutting flag retirement. The documents are consistent about it and keep the host integration Fastly-first, so nothing here is contradictory. It is worth the maintainer confirming that the enlarged decomposition is what was intended, since approving this planning PR ratifies that shape for #46/#47, #72-#75 and #88-#90.
Next actor
The maintainer, for joint spec/plan approval. No author correction is outstanding: the single prior finding is fixed, verified and resolved, and I found nothing new at this head.
Merge remains gated on an eligible independent formal APPROVE covering a066d6d, plus completion of the last in-progress check. A verdict inside this COMMENT does not satisfy that gate.
Maintainer Merge AuthorizationThe maintainer explicitly confirmed merging PR #152 at Immediately before merge, the head is unchanged, the reviewer has accepted/resolved the finding and reported no new findings, and all 25 checks are successful. No CI check is waived. No self-approval is being submitted and no administrative bypass is authorized or used. This is the spec/plan PR only. Issue #34 remains open; implementation and qualification remain separate work. |
PRfection phase: plan
Refs #34. Parent epic: #8.
Summary
Define whole-catalog qualification reporting and a uniform implemented-bidder contract across Axum, Cloudflare, Fastly and Spin, while keeping the first in-process Trusted Server integration Fastly-first and display-only. This PR changes only the specification and plan; it does not implement qualification, remove production flags or claim integration readiness.
Scope And Design
The maintainer requested joint spec/plan review, then explicitly broadened the bidder requirement to the whole catalog with unimplemented entries skipped. The reviewer has reviewed the expanded pair at this head, accepted/resolved the original finding and reported no new findings. The maintainer explicitly authorized merge using that recorded review without a formal independent GitHub approval for this head. Issue #34 remains open; downstream implementation remains separate.
Catalog Contract
skippedwith reasonnot_implemented, not omitted or counted as passing. A source association alone does not prove a Rust identity is implemented.native_only, target-based bidder filtering and delivery-cohort schema selection through the reviewed implementation cutover. Include manifest/config models, generator/storage predicates, schema availability/initialization/admission and bootstrap, not just the TOML flags. No replacement per-adapter bidder list or target flag is introduced.The host contract retains one Trusted Server runtime Settings/configuration path, shared Auctioneer preparation/engine policy, production plan-backed local execution, original deadlines, owned lifecycle/resource handoff and typed selection/targeting. It introduces no sidecar, internal HTTP RPC, nested App, second loader or test-only registration path. Live vendor authorization and hosted/publisher acceptance remain separate from deterministic fixture qualification.
Delivery Owners
Shared preparation and host binding remain in #51; production host execution is Trusted Server-owned. Discovery/schema qualification uses #46/#47, adapter fit #72-#75, guest serving #88-#90, and actual linked host acceptance #76. Package 1 reconciles those existing issues before implementation; this PR does not create another configuration or registry epic.
Immutable Source Baseline
f307289269f7c510d86647a9f6b673058b74e74651730bdaf841e4d7bb96c2258e5e98169bdff9a7775366fdac923135e8430b1e3dffebedc648ef7cv0.0.11, resolved87456c708ffe9edf86b3a0f8b06dfd15d60b07b5Skills used: PRfection CREATOR,
superpowers:brainstorming,superpowers:writing-plans,superpowers:receiving-code-reviewandsuperpowers:verification-before-completion. An independent read-only document subagent reviewed the final whole pair without material findings; this is not formal GitHub approval or maintainer acceptance.Review Disposition
The single original finding is fixed, independently verified and resolved by the reviewer. Commit
cf51a412133634370c2669a7f43b83b1423c2cb4corrected the source membership wording; the user-directed revision ata066d6de500828993f69940e6aed09d5f63fa955replaces the fixed bidder subset with whole-catalog reporting and discovery-based qualification. The current-head whole-pair review approves the substance with no new findings, but is aCOMMENTEDself-review, not an eligible formal approval. CREATOR did not resolve or hide any thread.Verification
Current head:
a066d6de500828993f69940e6aed09d5f63fa955. The diff contains only the two Markdown documents; production code, tests, workflows, dependencies and limits are unchanged.Executed local source/document checks, exit 0:
This is a temporary local source-inspection script, not a new committed acceptance gate or an application qualification report. Counts describe the pinned source, not passing runtime qualification.
cargo fmt --all -- --check,git diff --cached --checkand the two-documentprettier --check --prose-wrap preservecheck exited 0; Prettier reportedAll matched files use Prettier code style!.Supplemental runtime verification at the current head, exit 0:
Full-Suite Status
The complete unchanged current-head GitHub matrix has finished: 25 checks successful, 0 failed, 0 pending across tests, format/lint, Fastly, Cloudflare, and Spin. These were executed by GitHub runners, not locally by this CREATOR. The full test workflow contains 20 successful jobs at exactly
a066d6de500828993f69940e6aed09d5f63fa955; the other workflows supply the remaining 5 checks. No prior-head results are substituted.Executed readback, exit 0:
Verbatim result excerpts retrieved from the current-head GitHub logs: the default-workspace source-selection control, followed by both mandatory fresh producer cases:
This completes the existing acceptance matrix for this planning revision. It does not implement or certify the future whole-catalog qualification described by the documents.
The original local workspace attempt at
1ce56d32ae0245139007f6b5ff41fee99364a4a1was interrupted during compilation with exit 130 because of disk pressure, before test-result lines. It remains incomplete, not passing evidence. Existing mandatory producer/compiler/feature/adapter owners and every assertion remain intact; no gate was weakened.Merge Outcome
Merged into
mainas 9b83deaa680ad8e867169db3b4ebb195b8a1b33c. Verified that this commit is onorigin/mainand its tree is identical to the reviewed/tested head:git diff --exit-code a066d6de500828993f69940e6aed09d5f63fa955 9b83deaa680ad8e867169db3b4ebb195b8a1b33c --exited 0. Issue #34 remains open. No production implementation is included.All findings are reviewer-accepted/resolved and all 25 current-head checks pass. The maintainer explicitly authorized merging this head using the recorded positive reviewer verdict without a formal independent GitHub
APPROVE. This exception applies only toa066d6de500828993f69940e6aed09d5f63fa955; no CI check is waived and no self-approval or administrative bypass is used. No production implementation, live testing or CREATOR thread-resolution is included. Issue #34 remains open.