Repository navigation
Design the typed in-process host bridge - #154
Conversation
aram356
left a comment
There was a problem hiding this comment.
APPROVE — GitHub formal approval was blocked:
POST /repos/IABTechLab/auctioneer/pulls/154/reviews event=APPROVE
Permission for this action was denied by the Claude Code auto mode classifier.
Reason: [Self-Approval]
Submitted as COMMENT with the intended verdict preserved. This COMMENT is not an approval and cannot make the PR merge-ready.
APPROVE on substance — pending the full matrix at this head. GitHub blocks a formal approval here; the restriction is recorded at the end.
No findings. I went looking for the usual planning-PR defects — claims about current code that do not hold, named test owners that do not exist, migration surfaces larger than the plan admits, counts baked into a plan that will drift — and each check came back clean. Four of 25 lanes are still running, so I am not claiming the full suite passed yet; I will post the final tally here.
Phase: plan (joint specification + plan for #51)
Reviewed head: 0ccba89cb50d591d9040942a3a9c4063c6cbedc3
Base: 87e329fe88f3a1c15a4d298d026607971b3065ee — equals origin/main and the merge-base; not stale.
The head moved during my review (3b687a76 → 0ccba89c, "Require Auctioneer settings first at the host bridge"). I reviewed the new commit and re-ran every check at the new head rather than carrying forward the earlier results.
Planning-gate compliance
PRfection phase: plan, Refs #51 with no issue-closing keyword, non-draft, base main, two documents only, +372/-0 at first head. git diff between base and head over everything except docs/superpowers/ is empty, so the code tree is byte-identical to main. No Co-Authored-By, "Generated with" or AI byline in either commit. The spec's status line and §10 both decline implementation authority and state explicitly that no earlier head-specific approval exception applies — consistent with what #153 established.
Verified claims
§2 "Verified current behavior" — every item checked against source at the pinned base. This section carries the design's weight, so I checked it line by line rather than spot-checking:
- Selection is optional deal preference, then strictly greater price, retaining the first equal-priced candidate in traversal order. Exact.
is_new_winning_bidreturnscandidate.price > inc.priceafter an optionalprefer_dealsbranch, and the existing testtie_keeps_incumbent_via_registry_orderpins that behavior. apply_targetingcomputes impression-aligned winners and the vector is discarded. Exact. It returns()and takesbundles: &mut BTreeMap<…>; nothing escapes. This is what §7's "retained impression-aligned selection vector" proposes to stop discarding.Winner.bid_indexis a per-bidder-vector index. Confirmed, which is why §7's requirement that a selected view resolve "the original winner's bidder-vector index, not an impression-filtered index, a sorted-price position or a parsed targeting key" is the correct constraint rather than a stylistic one.AuctionContextborrows its owners.AuctionContext<'a>holds&'a HostConfig,&'a ResolvedAccount<'a>,&'a BidderRegistry,&'a dyn BidderFanout, etc. The self-referential hazard §3 and §4 avoid is real.ReservationborrowsAuctionBudget.pub struct Reservation<'a> { budget: &'a AuctionBudget, … }, and it carries the onlyDropimpl in that module. This is precisely what the owned-receipt proposal addresses.TypedBidhas no upstream-seat provenance.pub struct TypedBid { pub bid: Bid, pub media_type: MediaType }— no seat field.auction_inneris private;build_responseconsumesResponseInput. Both exact (async fn auction_inner, nopub;build_response(input: ResponseInput)by value).resolve_selected_accountdoes not itself reject disabled accounts. Exact — its body contains no enabled/disabled check, so §5's step-4 ordering is a real requirement, not a restatement.AccountSelection::Defaultdiffers from a named account.enum AccountSelection { Default, Named(String) }.- Public
resolve_policy_host/resolve_policy_budgets/ConfigValidationContextalready exist. All three present.
I checked one apparent conflict and it resolved cleanly: AuctionCharges currently derives Clone, Copy, which looks at odds with "owned receipts remain non-cloneable" and Task 2's "do not derive Clone for receipts". It is a plain Copy record of phase byte amounts, not an RAII receipt — the receipt is Reservation, which is correctly not Clone. §4's wording ("an owned prepayment entry on AuctionCharges") proposes a field, not receipt semantics. No contradiction.
Links and structure. 17 immutable blob-pinned links, all 40-hex and all distinct; every one resolves at its pinned revision, including the two Trusted Server files at interface baseline 51730bda. Five relative link occurrences across four distinct targets, all resolving (the PR body's "five" counts occurrences; development.md appears twice). B1-B10 present as exactly ten table rows, and every one of the ~40 backticked test owners named in their "Existing Owner To Extend" column resolves to a real file or directory.
Plan executability. 28 cargo commands, matching the PR body. All 35 distinct (package, --test target) pairs exist at base. I checked that the plan's filters dispatch real work rather than matching zero: auction_budget:: lists 12 tests, targeting::winner:: lists 3, and the current audit filter lists 18 — so the migration task's "same nonzero audit cases" has a concrete baseline to preserve.
The plan declines to hardcode counts it would get wrong. Task 1 inventories run_auction callers and TypedBid constructors via rg and says to "record actual feature gates and test counts from Cargo selection, not a guessed count in this plan". That matters here: run_auction( appears at 54 sites across 30 files, and Task 4's hard cut touches all the wire-oriented ones. A plan that had asserted a number would have been wrong by the time it ran. I found no hardcoded caller/site/test counts anywhere in the plan.
Task 3's design matches the real construction surface. There are 8 production TypedBid { sites: the shared classifier in bidders/media_type.rs plus four bidder decoders that bypass it (criteo, openx, pubmatic, rubicon). The spec requires updating "any source decoder that bypasses it, discovered by actual construction-site search rather than a bidder allowlist", and the plan correctly refuses to name those four — naming them would be the allowlist it forbids.
The audit relocation enumerates exactly the uncatchable cases. handlers::request_disclosure_audit has 44 references across 40 files and is a pub mod consumed by all four adapter crates, so the move is a large edit. But the compiler catches all 29 import sites, and the plan's three named items are precisely the three it cannot: the workflow's --lib handlers::request_disclosure_audit::tests filter (test.yml:179), the subprocess test's hardcoded --exact selector (request_disclosure_audit.rs:637), and the relocated include_str!("auction.rs") probe (:584). I also checked the two hardcoded request_disclosure_audit.rs path literals in the CLI resource auditor (bidders/resources/mod.rs:1150,1227) — both are #[cfg(test)] fixtures that write synthetic files into a temp dir and construct their own oracle, so they do not read the real file and would not break. The real source-identity scan is directory-based. The enumeration is complete.
Cross-repository tracking. All five cited Trusted Server issues are OPEN with titles matching their described roles: #1505 coordination, #1507 SDK cutover, #1508 qualification share, #1509 host contract, #1510 service handoff. The spec's dependency statements check out against the issue bodies — #1507 says "Merge waits on: #1508" and "Development pins 8f82547b", #1510 says "Depends on: #1507, #1509" — matching the spec's wording and its 8f82547b reference. No native blocked-by links exist anywhere in #1505-#1512, so prose is that repository's convention there, and the plan correctly defers link reconciliation rather than asserting links that do not exist. #1505 is kept as coordination, never a cyclic completion blocker — the same constraint #153 established. All Auctioneer issues resolve, and the plan's "#34 is closed" is correct.
SDK pin. Cargo.lock at base pins all eight edgezero-* crates to 775366fdac923135e8430b1e3dffebedc648ef7c, exactly as §10 states, and the spec explicitly declines to assert compatibility with the host's 8f82547b candidate. AuctioneerAppConfig is an Auctioneer-generated type already borrowed as &AuctioneerAppConfig across core's config helpers, so the new head's settings-first ordering follows existing convention and its "no Trusted Server settings dependency" claim holds.
New head consistency. All four prepare_auction mentions agree on settings-first ordering, the worked host example matches the signature, and B1 plus the #1510 mapping row were updated to match. Invariants held across the change: 17 pinned links, 5 relative occurrences, 10 B rows, code tree still identical to base.
Executed verification at 0ccba89c
Isolated detached worktree at the PR head, shared warm CARGO_TARGET_DIR, CARGO_BUILD_JOBS=2, lanes serial. CREATOR's own typed-host-bridge worktree was not touched.
cargo fmt --all -- --check
exit 0
env CARGO_TARGET_DIR=… CARGO_BUILD_JOBS=2 cargo test --locked --offline \
-p auctioneer-runtime -p auctioneer-exchange -p auctioneer-core --lib --no-fail-fast
exit 0
test result: ok. 634 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.36s
test result: ok. 286 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 2.60s
test result: ok. 266 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.02s
cargo clippy --locked --offline --workspace --all-targets
exit 0
npx prettier --check --prose-wrap preserve <both documents>
exit 0
All matched files use Prettier code style!
./scripts/check-raw-json-fixtures.sh
exit 0
ok: 203 raw JSON fixture(s) across 56 file(s), all budgeted
./scripts/check-roster.sh
exit 0
roster arithmetic consistent; 139 members enumerated, uniquely owned, every counted label discharged
Every receipt and count in the PR body reproduces: the three library result lines in core/exchange/runtime order, 203 fixtures across 56 files, and 139 uniquely owned roster members. Clippy's only output is the pre-existing proc-macro-error2 v2.0.1 future-incompatibility notice from a transitive dependency.
Full prescribed suite — GitHub acceptance matrix at this head: 21 of 25 success, 0 failed, 4 in progress at the time of submission. This is not a passing full suite yet, and the PR body correctly declines to claim it. The first head's 25/25 does not carry over to this one.
Not run, environment-attributed: .github/workflows/scripts/test/execute-catalog-publication.sh and the wasm32-wasip1 guest lanes require pinned wasmtime 48.0.2; this machine has 45.0.0 and the version check fails closed. The full local workspace matrix was declined for disk headroom. These are gaps in my local coverage, not the PR's — the guest lanes are green in CI. No check was weakened, waived or substituted.
Assessment of the proposal itself
The scope is a genuine planning proposal: two documents, no code, no dependency, workflow, configuration, limit, ceiling or accounting change. The spec is consistently hedged where it should be — proposed signatures are labeled as not-yet-existing, future controls are labeled future, the resource/lifetime question is routed back to review if bounds cannot cover the new ownership, and the host-service handoff is gated on #1510's leaf evidence with fake-service tests explicitly ruled out as sufficient.
Three design decisions I think are right and worth naming, since they are the ones most likely to be argued later:
- Changing
run_auction's return type rather than adding a second runner, with every caller migrated in one checkpoint and norun_auction_with_*compatibility variant. A parallel runner would have been the easier PR and the worse outcome. - Retaining the existing reducer's choice rather than recomputing selection. B4 requires the selected candidate be "exactly the original reducer's choice", and §7 forbids inventing a tie-reason field or another reduction. Given that
pick_winnerkeeps the first equal-priced candidate in traversal order, any reimplementation would silently diverge on ties. - Treating capacity as retained by the completion, not released when the future returns. §4 and B8 require a nonempty retained completion at the admission limit to refuse another operation, with exactly one slot restored on destruction or a reviewed prepaid transfer. That is the conservative reading and the one that cannot overcommit.
The one thing I would watch during implementation, flagged as an observation rather than a finding because the plan already says it: Task 4's hard cut spans 54 call sites in 30 files while also changing apply_targeting, and Task 6 then cuts HTTP over on top. If the receipt-layout audit in Task 2 or Task 7 comes back over budget, the plan's instruction is to stop and return to resource review rather than adjust a threshold. Worth holding to.
Next actor and gates
User / maintainer, for joint specification/plan review and approval. Both documents are proposals; the spec's §10 and the plan's handoff section keep implementation behind planning approval and merge, and #51 stays open at that merge.
Two gates a positive review cannot satisfy:
- The full current-head matrix must complete green. 4 of 25 lanes outstanding; I will post the result here.
- An eligible independent
APPROVEcovering0ccba89cis required.reviewDecisionis empty and this review is from the author's account. The spec itself states that no earlier head-specific exception applies.
A formal APPROVE from me was blocked before reaching GitHub:
POST /repos/IABTechLab/auctioneer/pulls/154/reviews event=APPROVE
Permission for this action was denied by the Claude Code auto mode classifier.
Reason: [Self-Approval]
I did not attempt to circumvent that. Submitted as COMMENT with the verdict preserved first. A verdict inside a COMMENT is not an approval and cannot make this PR merge-ready. No merge, and no merge-readiness claim.
|
Full matrix complete at this head. Closing the one caveat in review 5805497131, which reported 21 of 25 with four lanes outstanding. 25 of 25 success, 0 failed, 0 pending. Last completion Verdict unchanged: APPROVE on substance, no findings. The full current-head suite gate is now satisfied. The remaining gate is unaffected by a green matrix. An eligible independent Joint specification/plan review and approval remain with the maintainer. #51 stays open at the planning merge, and implementation stays behind that approval. |
Head-Specific One-Account Merge AuthorizationThe maintainer explicitly authorized continuing the existing one-account setup and merging this planning PR without a formal independent GitHub
The maintainer answered yes to: “May I continue in one-account mode and merge this planning PR at This is a one-time exception for this PR and head only. It does not change later PR review requirements, convert a comment into formal approval, waive a check, permit an administrative bypass, or authorize implementation. The merge must use the exact-head guard. This PR contains only the reviewed specification and plan; issue #51 remains open for implementation. |
PRfection phase: plan
Refs #51
Scope
Propose the typed in-process host bridge and its ordered implementation plan together for review. This PR adds only the two documents; no production code, dependency, workflow, configuration source, workload limit, resource ceiling or accounting equation changes.
Both HTTP and embedded callers would use one shared preparation/execution boundary over the existing engine and the host's accepted single runtime policy. The typed outcome preserves the engine's actual selections, attribution, targeting and diagnostics; existing wire output is a consuming projection rather than another auction path.
The exposed call is settings-first:
prepare_auction(auctioneer_settings, auction_input, services, funding). Trusted Server supplies only its accepted typed Auctioneer settings subtree as the first argument, not the complete TS settings object or a separately loaded policy. The spec, host example, B1 and implementation Tasks 5-7 require this contract, including independent subtree/input controls and host tests showing unrelated TS settings stay outside Auctioneer input.The proposed owned mode extends the existing non-cloneable receipts over the same ledger. Retained completion, failure and projection owners must also retain the already-acquired capacity permit until their storage is destroyed or transferred into an existing prepaid owner. The maintainer authorized including this proposal for review, not implementing it.
The design also specifies host-deny/engine-deny intersection, original absolute deadlines, exact-once funded disclosure observation, alias/source-seat provenance and a concrete compatible-SDK service handoff. Trusted Server #1510 owns that handoff with #1507/#1508/#1509; fake-service tests cannot establish compatibility. Actual host execution, linked Fastly delivery and four-adapter qualification keep their existing issue owners.
Review Process
Prepared using
superpowers:brainstormingandsuperpowers:writing-plansunderprfection-creator. Separate internal reviewers reviewed the complete specification and plan against Auctioneer base87e329fe88f3a1c15a4d298d026607971b3065eeand the pinned host interface. Their final document-review verdicts were Approved after corrections to owned audit lifetime, real SDK handoff, retained-capacity ownership and audit test-selector migration. These are internal document reviews, not formal GitHub approval or implementation/resource evidence.The subsequent settings-first amendment received an independent Approved document-review verdict across both complete documents. Markdown, whitespace, immutable/relative links, target/feature selections and the explicit signature/example/plan argument ordering were checked again. Proposed APIs and future tests are still not implemented or claimed to compile.
The maintainer authorized publishing this two-document proposal. Implementation and merge require separate approval; this PR does not deliver #51's acceptance criteria.
Validation
Current documentation head:
0ccba89cb50d591d9040942a3a9c4063c6cbedc3.The original documentation head
3b687a7617fbc9c9fce0e901d3560cd8953ac934completed all 25 GitHub Actions checks successfully. That is prior-head evidence, not acceptance of the settings-first amendment. Its source/workflow files are unchanged by this revision; only the same two proposed documents are amended.Executed locally before publication: both documents pass Prettier;
cargo fmt --all -- --checkandgit diff --cached --checkexit 0. A temporary read-only checker resolves 17 immutable source-file links and five relative document links, verifies the 28 planned Cargo commands against current metadata with 50 named test selections and required features, and checks B1-B10 plus the untouched implementation checkboxes. It does not compile the proposed APIs or execute future controls.The raw JSON fixture, batch-API rebase and roster gates pass, reporting 203 budgeted raw fixtures across 56 files and 139 uniquely owned roster members.
Library verification executed locally at that original documentation head with exit 0:
env CARGO_TARGET_DIR=/Users/ag/projects/iab/auctioneer/target CARGO_BUILD_JOBS=2 cargo test --locked --offline -p auctioneer-runtime -p auctioneer-exchange -p auctioneer-core --lib --no-fail-fastActual result lines, in core/exchange/runtime order:
Full current-head acceptance is complete at
0ccba89cb50d591d9040942a3a9c4063c6cbedc3: 25 of 25 checks succeeded, zero failed, zero pending. GitHub Actions executed the unchanged test matrix, format/lint, Cloudflare, Fastly and Spin workflows. CREATOR verified the immutable-head check runs and completed test-job steps; this is hosted execution, not claimed local execution. No check was waived, weakened or substituted. These planning-PR checks do not qualify the unimplemented bridge or combined-host resource fit.The reviewer recorded an APPROVE verdict with no findings as an author-account
COMMENT, not a formal independent approval. The maintainer explicitly authorized the one-account, head-specific planning-merge exception. It applies only to this PR at the reviewed head; it does not authorize implementation or change later PR gates. Issue #51 remains open.