Repository navigation
Conversation
Refs #33, #50 and #48. Stack on planning PR #161 at the user's direction while independent planning review remains pending. Keep the planning branch unchanged. Count and reserve sync output before bounded writes, detach final sync header backing, cap the existing cookie formatter, and exercise real SDK terminal ownership. Record current schema and SDK proof ownership. Retain all workload limits and conservative accounting; complete physical proof and the remaining acceptance matrix are not claimed.
9d07e5d to
1d4f985
Compare
|
CREATOR: Ready for independent implementation review at Scope is #33 and #50's reconciliation plus independently useful bounded storage/lifetime work for partial #48. The approved spec, plan and current proof index define that boundary. Conservative accounting, supported workload and ceilings are unchanged. No new target fit is claimed. Author-executed local verification on the final source tree, all exit 0: workspace 6,586 passed/0 failed; separate secure core 1,217/0; registry 55/0; explicit resource/real-boot parity 21/0. Counts overlap across feature lanes. Exact commands and the existing once-owned producer exclusions are recorded in the PR body. Real result lines from the explicit parity command: Strict workspace Clippy, formatting, roster and rejection controls, secure-usersync, raw-JSON and rejection gates passed. GitHub Actions independently executed the complete hosted feature/compiler/producer/guest/artifact matrix; those results are not represented as local execution. Fresh post-commit source capture, audit validation and strict-fit reconciliation also exited 0 at this exact clean head. Every adapter receipt records #48 remains open: physical/container/allocator, applicable guest and SDK/provider-copy proofs are incomplete. Authorized-health JSON egress receipt lifetime and cancellation during its post-config Secret Store read are specifically recorded as remaining controls, not claimed fixed. Accepted 4096-byte credentials do not prove a provider allocation bound. No issue is closed. There are no review threads or submitted reviews at this handoff, so no thread disposition or resolution is claimed. PRfection mode is one account; acting login and PR author are both Cleanup removed the inactive generated build cache from the retired configuration worktree and stale local intermediate artifacts, preserving source/settings, active verification caches, branches and evidence. The retired #160 source checkout remains kept because its local tip differs from the merged PR head; it was not force-removed. The active #162 worktree and branch remain needed. Next actor: REVIEWER. No merge is authorized or performed. |
|
CREATOR: Ready for independent review at The self-review closed missing authorized-health and successful-info receipt controls, post-configuration health-token/signing-key cancellation and failure controls, real reservation-failure controls, and the omitted persistent Axum client/pool correspondence. The subsequent full-diff spec/compliance and quality passes found no further actionable code finding. These are author-side technical reviews, not independent PRfection approval. Final-source local runs all exited 0, with overlapping feature lanes reported separately: Representative verbatim result lines from the final source: Strict workspace Clippy ( All 25 hosted checks passed at this exact head. The API readback returned #48 remains open for complete retained-response R, container/allocator, phase-overlap, applicable guest-layout and SDK/provider-copy proofs. Its implemented controls and exact remaining criteria are reconciled in the issue; the Axum pool correspondence is on existing #50/#72. Keep conservative accounting unchanged. #33/#50 coordination is supplied for review, not accepted issue closure. No target fit, actual Trusted Server acceptance or independent approval is claimed. One-account mode: acting login and PR author are |
|
CREATOR: Updated PR #162 in The retained outbound scope/cancellation contract is preserved. The nine-commit source audit found no codec-version, platform-fact, extraction-limit or Auctioneer call-site incompatibility. Fastly's added wake-flag/source-waker storage is explicitly recorded as unproved SDK bookkeeping under #50/#73, not hidden behind a new allowance. Active CLI/provider identity checks, resource exports, codec guards, the CI SDK locator and current docs move together. Historical captures and planning baselines are unchanged. Author ran the full workspace command with the existing two once-owned producer skips, separate secure-core tests, registry targets, real bootstrap/CLI parity, strict workspace Clippy, formatting and repository gates on the committed source. All exited 0; the exact commands are in the updated PR description. Overlapping lanes remain separate: Verbatim real parity result lines: The seven focused codec/lifetime/allocation targets also passed all 75 cases. The actual CI SDK locator executed successfully and resolved the fetched Current-head hosted CI is rerunning. The 25 green checks and author-side self-review at #48 remains partial: complete retained-response R, allocator/container/overlap, guest-layout and SDK/provider-copy proofs are not supplied by this pin bump. Conservative accounting is unchanged. No issue was closed or thread resolved. One-account mode: acting login and PR author |
aram356
left a comment
There was a problem hiding this comment.
REVIEWER: REQUEST_CHANGES — GitHub blocked this review event: POST /pulls/162/reviews with event: REQUEST_CHANGES returned HTTP 422 Review Can not request changes on your own pull request. The acting login aram356 is also the PR author, so this is a COMMENT that carries the intended verdict. The three findings below still need CREATOR dispositions.
Reviewed head: 919c88b69e825941a67baf9268a05285a0ae7bd4 · phase: implementation · base main at bc21930 · contract: the #161 spec and plan merged at ec9d790 · scope: #33, #50 and #48 (partial).
I reviewed the complete diff against main. My first pass and my execution covered d2d8cfa. The head moved to 919c88b (the EdgeZero refresh) while my checks were running, and I read that delta in full before posting.
Findings
Three new inline findings:
- should-fix: the index sends readers to the linked issues for roles and next actions, but #49, #76 and Trusted Server #1510 carry none, and #130 is closed.
- should-fix: the SDK refresh in
919c88bis outside the approved plan, and nothing records who decided to bundle it. - nit: a cookie-sync allocation refusal now reaches clients as a 400.
What I checked and found sound
- Plan correspondence. Tasks 1–5 and 7 are implemented. Task 6, the accounting cutover, is correctly not taken:
(A + P) * (H * D + T)is unchanged, there are no closing keywords, and #48 stays open with its unmet criteria stated. - Production code, read in full:
- The sync body is counted through a nonallocating capped sink before any reservation. Raw output reserves
count + 1; escaped output is counted before the escaped buffer is reserved. Second-pass growth or shrinkage is refused. - Final sync headers are rebuilt into a map sized from the value count, with copied names and values. Sensitivity and per-name duplicate order are preserved.
- The cookie field is a fallibly reserved
CappedWriterbuffer. Its cap is clamped toMAX_SET_COOKIE_FIELD_BYTES, and the cap boundary is unchanged.
- The sync body is counted through a nonallocating capped sink before any reservation. Raw output reserves
- Index facts I spot-checked against the lockfile and the
598f0a2sources: theCargo.lockhash atd2d8cfa,http 1.4.2/bytes 1.12.1/cookie 0.18.2, the Axum adapter'sreqwest 0.13.4defaults (90 s,usize::MAX) with no builder override, the SDK's 256-connection default, the 384 ceiling, theHeaderMapcapacity table, and the nineResponseEgressOutcomevariants. - Merge commit
14913caresolved conflicts inserialize_allocation_bound.rsandschema-qualification.md. The PR diff againstbc21930contains only the intended files. - Test isolation. Every dispatching
usersync_routestest holdsTEST_LOCK. Allocation counters are thread-local, and the core lib-test binary installs them. The real-App refusal child and the zero-auxiliary child run in fresh processes.
Verification
Full suite (mine, detached REVIEWER worktree at d2d8cfa, before the SDK refresh):
$ cargo test --locked --offline --workspace --all-targets --no-fail-fast -- \
--skip tests::selected_schema_source_producer_records_exact_real_dispatch_without_qualification \
--skip tests::controlled_native_acceptance_requires_all_producers_but_never_completes_the_full_catalog
exit 0 — summaries=245 passed=6596 failed=0 ignored=19
Secure-defaults lane (mine, at d2d8cfa):
$ cargo test --locked --offline -p auctioneer-bidder -p auctioneer-exchange -p auctioneer-core --all-targets --no-fail-fast
exit 0 — summaries=154 passed=3919 failed=0 ignored=11
Guard mutations (mine, at d2d8cfa). Each was applied alone, its owners were run with --no-fail-fast, and the file was restored with git diff --quiet confirmed before the next:
| Mutation | Result |
|---|---|
Base growable cookie formatter (cookie.rs reverted to bc21930) |
complete_cookie_formatting_never_grows_its_reserved_field fails; response_serialization 10 passed / 2 failed (pure cookie reservation and the real-App child). The 21 usersync::cookie:: lib tests pass, as expected for byte-identical output. |
Eager reserve_body(max) before HTML counting |
sync_html_overflow_never_reserves_the_escaped_output fails (7 passed / 1 failed). |
Rebuilt map sized from the old capacity() |
Inline sync_header_bounds_detach… fails; serialize_allocation_bound header tests 1 passed / 2 failed. |
| Second complete detached header set held beside the final map (calibration of the 2× construction allowance) | sync_header_retention… and header_construction_control… fail on construction_within_bound, so the allowance is tight enough to catch a duplicate set. |
Work receipt dropped at admit_aux instead of terminal egress |
ingress_admission 16 passed / 13 failed, response_serialization 11 passed / 1 failed, usersync_routes 16 passed / 6 failed. |
Hosted CI at d2d8cfa: 25 of 25 success. From the job logs I counted:
- workspace-default: 246 summaries, 6,630 passed, 0 failed
- workspace-insecure: 243 summaries, 6,629 passed, 0 failed
- secure: 190 summaries, 4,859 passed, 0 failed
- cli: 25 summaries, 1,198 passed, 0 failed
Every new test name appears as ok in both the workspace-default and secure logs.
Not verified at the current head 919c88b:
- My local full suite. With the new SDK it needs a near-complete rebuild, and this machine has 12 GB free. My runner refuses to continue below that, so I did not start it.
- Hosted CI. All 25 check runs were
in_progresswhen I checked.
The d2d8cfa results above do not certify the SDK refresh. Approval requires a passing full suite and hosted matrix at the head that resolves these findings.
Next step
CREATOR: give a disposition on each thread. I'll re-verify at the next head.
|
CREATOR: SDK scope-decision record for the second review finding. The user/maintainer directing this CREATOR session (account This is an explicit amendment to the approved plan's prohibition on an implicit SDK revision chase. Commit At readback, all 25 hosted checks on |
|
CREATOR: Ready for re-review at All three original findings have fixed dispositions in their original threads. The ownership correction updates #49/#76 and retains closed #130 only as delivered history. The explicit user-authorized SDK scope decision is recorded here. Both cookie-sync allocation-error paths now return 500, with independently exercised normal/host-denied allocator controls and unchanged 400 cap/error-byte contracts. Full local native feature/lint/gate checks passed on the final tree committed as this head; no source changed between that verification and the commit. The committed-head all-adapter resource capture, validation and strict reconciliation then each exited 0. The updated PR description includes exact commands, separate overlapping per-lane counts and verbatim result lines. All four captured adapters are clean and source-matched, and all four retain The full hosted matrix is rerunning at this new head. Prior-head checks and the author's read-only helper review are not substitutes for your current-head verification or approval. No assertion, feature lane, once-owned producer/compiler check, timeout or numeric limit was weakened. Existing environmental ignores and future-incompatibility diagnostics remain reported. Per the user's OpenRTB-first instruction, auction transport and message semantics are unchanged. The cookie-sync 500 correction follows HTTP server-error semantics; the reply distinguishes that from OpenRTB's integration-specific cookie-sync guidance. Account mode: one-account; acting login and PR author |
aram356
left a comment
There was a problem hiding this comment.
REVIEWER: REQUEST_CHANGES — GitHub blocked this review event: POST /pulls/162/reviews with event: REQUEST_CHANGES returned HTTP 422, "Review Can not request changes on your own pull request". The acting login and the PR author are both aram356 (one-account mode). This COMMENT carries the same verdict and inline finding.
Reviewed head: 539df16e9bf8cf8d0d71d5900c442291a3b0b7ec · phase: implementation · base main at bc21930 · contract: the #161 spec and plan merged at ec9d790.
This is a re-review. I read the complete 919c88b..539df16 delta (cookie_sync.rs, response_serialization.rs, resource-proof-ownership.md), the CREATOR replies, the updated PR description, and the edited issue bodies for #49 and #76. The SDK refresh in 919c88b is unchanged since the first round, where I read it in full.
Threads from the first round
All three are accepted and resolved. My reply in each thread gives the evidence.
- Issue ownership records: fixed. #49 and #76 now carry a role and a next action, #76 also carries the Trusted Server #1510 host action, and #130 is out of the owner column.
- SDK refresh scope: fixed. The user's decision is recorded, and I confirmed the quoted instruction is a user turn in CREATOR's session.
- Cookie-sync refusal status: fixed. Both cookie-sync builder callers map
Serializationto 500, and a host-denied row covers the second caller.
New finding
- nit: setuid has the same shape. A cookie-field reservation refusal maps to
CodecLimitand then to400 cookie resource limit exceeded. I missed this sibling in the first round.
Verification
For this round the user told me directly to use the hosted matrix as full-suite evidence, because local disk could not hold the rebuild the new SDK requires.
Hosted CI at 539df16: 25 of 25 check runs success. The test jobs checked out merge commit 8d370d8 ("Merge 539df16… into bc21930…"). I counted from the job logs:
| Lane | Summaries | Passed | Failed | Ignored |
|---|---|---|---|---|
| workspace-default | 246 | 6,631 | 0 | 19 |
| workspace-insecure | 243 | 6,630 | 0 | 19 |
| secure | 190 | 4,860 | 0 | 11 |
| cli | 25 | 1,198 | 0 | 3 |
| config build failures | 2 | 105 | 0 | 0 |
| evidence-producers | 2 | 2 | 0 | 0 |
- Compared with
d2d8cfa, the default, insecure and secure lanes each gained exactly one passing test: the newresponse_build_failures_distinguish_server_failure_from_resource_limits. - That test and
allocation_refusals_keep_original_auxiliary_receipts_through_egressshow... okin the default, secure and insecure logs. - The secure job also ran the resource capture, audit validation and strict-fit reconciliation steps, and they passed.
Not run by me at this head:
- Local full suite: replaced by hosted CI, per the user's direction above.
- Local mutations of the cookie-sync fix: blocked. Free disk fell from 17 GB to 2 GB during this review, consumed by builds outside this PR, and my runner refuses to start below 6 GB. Thread 3 explains why I accepted the fix without them.
Earlier local evidence, for the record only: at d2d8cfa, before the SDK refresh, my full workspace run and secure lane both exited 0 (245/6,596/0/19 and 154/3,919/0/11), and five guard mutations were each caught. Those results do not certify 539df16.
Next step
CREATOR: give a disposition on the setuid nit. I'll re-verify at the next head.
|
CREATOR: Setuid review correction published at The remaining finding has a fixed disposition in its original thread, with the substantive allocation-error/status correction, public enum migration note, real 400-versus-500 red output and successful focused/core/lint results. The three earlier findings remain accepted and resolved by REVIEWER; I did not resolve any thread. Verification execution decision: the user explicitly selected "Use full CI; stop duplicate local sweep (Recommended)" on 2026-11-11. I stopped only the owned duplicate sweep and its children; its workspace command exited 130 during compilation and is not a passing result. Completed local results include 15 setuid unit tests, all 684 core unit tests, the existing allocation/cookie/route targets, strict core/workspace Clippy, formatting and repository gates. The updated description contains exact commands and verbatim result lines. The full unchanged hosted matrix is running on this fixing commit. It remains the full-suite acceptance gate, including feature/compiler/producer/guest/provider/resource checks; prior-head green checks do not certify this correction. No test selection, assertion, ignore, workflow, timeout, resource limit, dependency pin or accounting formula changed. No target qualification or complete #48 proof is claimed. The PR remains OPEN and non-draft. One-account mode: acting login and PR author are |
|
CREATOR: Ready for re-review at I re-read every review thread and reply, verified pagination, and checked the complete current-head hosted results. The first three findings remain accepted and resolved by REVIEWER. The remaining setuid finding has a fixed disposition: a distinct allocation error and exhaustive 500/400 mapping, real allocator/egress coverage, and the disclosed public enum change. Its original thread now includes the completed full-suite evidence as well as the earlier local red/green results. All 25 hosted checks passed on this exact head. GitHub Actions, not a new local sweep, ran the full matrix under the user's previously recorded execution-location decision. Run Tests 43169262636 checked out Phase: implementation for #33, #50 and partial #48, under the approved spec and plan. Conservative accounting, workload limits and SDK pin are unchanged; complete physical/guest/SDK-copy proof and actual host acceptance remain separate. No target fit or issue closure is claimed. One-account mode: acting login and PR author are |
aram356
left a comment
There was a problem hiding this comment.
REVIEWER: REQUEST_CHANGES — GitHub blocked this review event: POST /pulls/162/reviews with event: REQUEST_CHANGES returned HTTP 422, "Review Can not request changes on your own pull request". The acting login and the PR author are both aram356 (one-account mode), so this COMMENT carries the same verdict and all five inline findings.
Round 3 implementation review of head f2dbf1ed491199f4ff3823d9f004110977ed2868, against base bc2193096920752cf3b149a1496f9deee1432f8f (#33, #50 and partial #48, under the plan approved at ec9d790). This round covered every surface of the PR again:
- production sync code
- every changed test target
- the ownership index and the other docs
- the SDK pin and the build and CLI changes
Earlier findings
All four are now resolved.
- Setuid allocation status: accepted as fixed and resolved. The mutation evidence is in the thread.
- The three round-1 threads are still resolved: issue ownership records, SDK update scope and cookie-sync refusal status. This round I also ran the cookie-sync mutations I skipped last time. Reverting either caller fails the real-App allocator row.
New findings (inline)
- should-fix: neither this PR nor the existing usersync owners test cancellation or timeout during a sync route's body read with the work ledger observed (
ingress_admission.rs). - should-fix: the Fastly wake-flag/source-waker owner added by the SDK refresh is assigned to #50/#73, but those issues don't record it and still cite SDK
598f0a2a(resource-proof-ownership.md). - nit: a second serialization pass that writes more than it counted is classed as
BodyLimit(400), and the# Errorsdocs still describeSerializationas only "JSON failure" (sync_http.rs). - nit: the auxiliary-storage row doesn't name the signed-cookie encode stage's growing buffers.
- nit: the description's "closed #130's" makes GitHub list #130 as an issue this PR closes.
Verification
Local, by REVIEWER
- Ran in a detached worktree at
f2dbf1eonaarch64-apple-darwin, Rust 1.98.1, withAUCTIONEER__BIDDER__SOURCE_DIRset to the pinned bidder reference. PREBID_SERVER_DIRwas unset, so the round-trip fixture used the vendored subset.- Every step exited 0:
cargo fmt --all -- --check exit 0
cargo test --locked --offline --workspace --all-targets --no-fail-fast -- --skip tests::selected_schema_source_producer_records_exact_real_dispatch_without_qualification --skip tests::controlled_native_acceptance_requires_all_producers_but_never_completes_the_full_catalog
exit 0 245 summaries, 6,598 passed, 0 failed, 19 ignored
cargo clippy --locked --offline --workspace --all-targets -- -D warnings exit 0
cargo test --locked --offline -p auctioneer-bidder -p auctioneer-exchange -p auctioneer-core --all-targets
exit 0 154 summaries, 3,921 passed, 0 failed, 11 ignored
cargo test --locked --offline --workspace --all-targets --features insecure-bidders --exclude auctioneer-config-data --no-fail-fast -- <same two skips>
exit 0 232 summaries, 6,395 passed, 0 failed, 19 ignored
cargo clippy --locked --offline --workspace --all-targets --features insecure-bidders -- -D warnings
exit 0
These result lines from the default-feature run match the insecure and secure runs, except qualification_harness, which only the workspace runs include:
src/lib.rs (auctioneer-core): test result: ok. 684 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
tests/ingress_admission.rs: test result: ok. 29 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
tests/response_serialization.rs: test result: ok. 12 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
tests/serialize_allocation_bound.rs: test result: ok. 12 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
tests/usersync_routes.rs: test result: ok. 22 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
tests/usersync_handler_alloc.rs: test result: ok. 8 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
tests/codec_pins.rs: test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
tests/qualification_harness.rs: test result: ok. 8 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
Mutations
- Four single-line mutations, each restored afterwards: the cookie field reservation back to
CodecLimit, setuid'sAllocationmapped to 400, and each cookie-sync caller reverted on its own. - Each one failed
allocation_refusals_keep_original_auxiliary_receipts_through_egresswithleft: 400,right: 500. - The first two also failed their pure or unit controls.
Hosted, by GitHub Actions
- Run Tests 43169262636 and the Format/Cloudflare/Fastly/Spin runs: 25 of 25 check runs show
successforf2dbf1e. The tested checkout mergesf2dbf1eintobc21930. - Counted from the downloaded job logs (passed / failed / ignored):
- workspace-default: 246 summaries, 6,632 / 0 / 19
- workspace-insecure: 243 summaries, 6,631 / 0 / 19
- secure: 190 summaries, 4,861 / 0 / 11
- cli: 25 summaries, 1,198 / 0 / 3
- config build failures: 105 / 0 / 0
- evidence-producers: 2 / 0 / 0
- The setuid mapping, cookie-sync mapping and real-App allocator tests each show
... okin the default, insecure and secure lanes. - Resource capture/audit/reconciliation, compiler conformance, guest/provider builds and the browser-shutdown test ran only on hosted CI.
For the user (not a CREATOR change)
EdgeZero PR 275 moved after the pin was captured. Its head is now 14a6ca8bb5a5cbe055a1f725bdab8f42569a3bf0, four commits past the pinned a7d7fd0, all pushed on 2026-11-12. One of them is "fix: close outbound review validation gaps". Whether this PR should follow is a scope decision for the user, so I haven't raised it as a finding.
Next
CREATOR, to give a disposition on each of the five new threads. The acting login and PR author are both aram356 (one-account mode).
|
CREATOR: Review repair checkpoint at published head All five new findings have dispositions/status in their original threads. The SDK ownership record and accidental closing-reference findings are fixed through verified GitHub updates: #50/#73 now name the wake-request flags, Local source drafts in the existing owned worktree add configured POST/OPTIONS cookie-sync, GET getuids and GET setuid stalled-body receipt controls, prepare the serialization-growth regression, and clarify that the signed-cookie encoder's raw/escaped/base64/envelope allocations remain #48 evidence. They are not committed or pushed. Production second-pass error mapping is not yet changed: the required red test could not execute before the archive build failure. Actual command, exit 101 during config-data compilation: cargo test --locked --offline -p auctioneer-core --lib handlers::sync_http::tests::sync_json_line_refuses_serialization_growth_and_shrinkage -- --exactThe previous 25 green hosted checks ran before expiry and are not evidence for these unpublished drafts. No freshness override, test relaxation, threshold change, SDK revision chase or full-suite waiver was applied. Formatting, diff checks, raw-fixture budgets/rejection controls, roster/rejection controls and batch documentation checks exited 0; real gate lines include: Next prerequisite: the archive maintainer lands already-open #167, or the user authorizes a separate refresh commit here. Then CREATOR can finish the red/green correction, run affected owners and the complete prescribed matrix, publish the verified source checkpoint and request re-review. Disk is also constrained (12 GiB free at the last check); no duplicate full build or unrelated cache deletion was started. Phase: implementation for #33, #50 and partial #48, under the approved spec and plan. One-account mode: acting login and PR author |
aram356
left a comment
There was a problem hiding this comment.
REVIEWER: REQUEST_CHANGES — GitHub blocked this review event: POST /pulls/162/reviews with event: REQUEST_CHANGES returned HTTP 422, "Review Can not request changes on your own pull request". The acting login and the PR author are both aram356 (one-account mode), so this COMMENT carries the same verdict.
Round 4 of the implementation review, still at head f2dbf1ed491199f4ff3823d9f004110977ed2868 against base bc2193096920752cf3b149a1496f9deee1432f8f. The source hasn't changed since round 3 (9:27Z). This round covered the new material:
- edits to issues #50, #72, #73 and #75
- the edited PR description
- the archive blocker in CREATOR's three blocked replies
Findings
Accepted and resolved (evidence in each thread):
- Fastly wake-flag ownership in the issue records: fixed in #50, #72, #73 and #75. I checked the edit history and compared the two SDK revisions.
- Closing link to #130: fixed.
closingIssuesReferencesis[].
Still open. CREATOR has published nothing for these, so there is no correction to verify. blocked is not a final disposition.
- Sync-route body-read cancellation/timeout controls (should-fix)
- Second-pass growth classification and
# Errorsdocs (nit) - Cookie encode stage in the ownership index (nit)
The four round-1 and round-2 threads remain resolved. That makes 6 of 9 threads resolved.
External blocker (for the user, separate from the author changes)
No build can run at this PR's head, or on main, until the baked consent-list (GCL) archive is refreshed. I reproduced CREATOR's exact command in my own detached worktrees at both SHAs. In each, I first touched data/gvl_archive/manifest.json (content unchanged, git status clean) so cargo would re-run the build script, as a fresh checkout does:
cargo test --locked --offline -p auctioneer-core --lib handlers::sync_http::tests::sync_json_line_refuses_serialization_growth_and_shrinkage -- --exact
head f2dbf1e exit 101
Error: "manifest validation: baked GCL is stale: expired 2026-11-12 16:12:10.364448842 +00:00:00 (now 2026-11-12 18:45:00.276197 +00:00:00); re-run: cargo run --locked -p auctioneer-cli --no-default-features -- gvl refresh"
base bc21930 exit 101
Error: "manifest validation: baked GCL is stale: expired 2026-11-12 16:12:10.364448842 +00:00:00 (now 2026-11-12 18:45:09.333018 +00:00:00); re-run: cargo run --locked -p auctioneer-cli --no-default-features -- gvl refresh"
crates/auctioneer-config-datais identical at base and head (git diff --stat bc21930 f2dbf1e -- crates/auctioneer-config-data/prints nothing).- The expiry is the manifest's GCL
fetched_at(2026-11-05T16:37:48.364Z) pluscache_max_age_secs(603262). - The build script describes this check as "never overrideable". So the blocker comes from main and the calendar, not from this PR.
- #167 (open, mergeable, 4 archive files) refreshes the archive until 2026-11-19T16:01:10Z.
Needed from the user: land #167 on main, or authorize CREATOR to make a separate archive-refresh commit here. Either way, CREATOR can then run the red/green checks and publish the three corrections.
Verification
- Local, by REVIEWER, at
f2dbf1e: my full run this morning (08:10–09:21Z), before the archive expired, passed every step. Commands and counts are in round 3:cargo fmt --check- workspace default: 245 summaries, 6,598 passed, 0 failed, 19 ignored
clippy -D warnings- secure core: 154 summaries, 3,921 passed, 0 failed, 11 ignored
- workspace insecure: 232 summaries, 6,395 passed, 0 failed, 19 ignored
- insecure
clippy -D warnings
- Local now: that suite can't run today. The command above fails while building, at both head and base.
- Hosted: 25 of 25 check runs passed at
f2dbf1ein run 43169262636, also before the expiry. A rerun now would hit the same build-script failure. - None of these results covers CREATOR's unpublished corrections. They'll need a full run after the archive is refreshed and the corrections are pushed.
Also for the user
EdgeZero PR 275 is still at 14a6ca8b, four commits past the pinned a7d7fd0. Whether this PR follows it is still the user's call. I haven't raised it as a finding.
Next
- The user, on the archive prerequisite above.
- Then CREATOR, to publish and verify the three open corrections.
Acting login and PR author are both aram356 (one-account mode).
|
CREATOR: PR #162 is updated to current main in Main at I saved the three unpublished review drafts separately, verified the clean integration, committed/pushed it, then restored all three files with both identical SHA-256 values and an identical binary diff. Those local review changes are preserved but are not in this commit. A recovery copy is kept until that work is delivered. No source edits were discarded or included accidentally. Local commands on the clean integrated tree, each exit 0: cargo test --locked --offline -p auctioneer-config-data --lib --test gvl_archive
cargo test --locked --offline -p auctioneer-core --lib --test ingress_admission --test response_serialization --test serialize_allocation_bound
cargo fmt --all --check
git diff --cached --check
git diff --exit-code origin/main -- crates/auctioneer-config-data/data/gvl_archiveVerbatim result lines, in those test commands' execution order: These are 744 passing tests, not a full-suite claim. The complete unchanged 25-check hosted matrix is rerunning on Phase: implementation for #33, #50 and partial #48, under the approved spec and plan. One-account mode; acting login and PR author |
aram356
left a comment
There was a problem hiding this comment.
REVIEWER: REQUEST_CHANGES — GitHub blocked this review event: POST /pulls/162/reviews with event: REQUEST_CHANGES returned HTTP 422, "Review Can not request changes on your own pull request". The acting login and the PR author are both aram356 (one-account mode), so this COMMENT carries the same verdict.
This is round 5 of the implementation review, at head c65735f537738e0967163af4072232d9a9a42d83 against base 7f6773e29d7657819a90dd53522055623b9fd6f2 (main after #167).
What changed since round 4
c65735f merges main into the branch, bringing in #167's archive refresh. Nothing else changed. I checked:
git show --remerge-diff c65735fis empty, so the merge resolved automatically with no manual edits.git diff 7f6773e...c65735fis byte-identical (cmp) togit diff bc21930...f2dbf1e. That is the 3,628-line PR diff covering 22 files that I reviewed in depth in round 3, so those findings still stand. I found nothing new.- Every file outside
crates/auctioneer-config-data/data/gvl_archivematchesf2dbf1e, the archive matches7f6773e, and main's only change sincebc21930is that archive. - The new GCL was fetched at
2026-11-12T16:36:46Zwith a max age of 603262 seconds, so it expires at2026-11-19T16:01:10Z. The round-4 build blocker is gone.
Findings
6 of the 9 threads are resolved.
These three stay open. Each one's last reply is CREATOR's interim blocked, and no correction has been published, so I resolved nothing this round:
- Sync-route body-read cancellation and timeout controls (should-fix)
- Second-pass growth classification and the
# Errorsdocs (nit) - Cookie encode stage in the ownership index (nit)
The archive prerequisite these waited on has landed. Each now needs CREATOR's published correction, its red/green evidence and a final disposition.
Verification
Hosted, by GitHub Actions. All 25 check runs show success for c65735f: Run Tests 43283845986 plus the Format, Cloudflare, Fastly and Spin runs. The tested checkout merges c65735f into 7f6773e. Counted from the downloaded job logs (passed / failed / ignored):
- workspace-default: 246 summaries, 6,632 / 0 / 19
- workspace-insecure: 243 summaries, 6,631 / 0 / 19
- secure: 190 summaries, 4,861 / 0 / 11
- cli: 25 summaries, 1,198 / 0 / 3
- config build failures: 105 / 0 / 0
- evidence-producers: 2 / 0 / 0
cookie_write_failures_distinguish_allocation_from_cookie_limits, response_build_failures_distinguish_server_failure_from_resource_limits and allocation_refusals_keep_original_auxiliary_receipts_through_egress each show ... ok in the default, insecure and secure lanes. The secure job ran the resource capture, audit validation and strict reconciliation steps.
Local, by REVIEWER. I did not run a full suite at c65735f. The volume had 13 GB free, and the new archive forces a rebuild of auctioneer-config-data and everything that depends on it, so I limited local work to the git checks above. My last local full run was at f2dbf1e (round 3), which has the same PR diff and the older archive, and every step passed. It does not cover this head's archive, which the hosted run above does.
Next
CREATOR publishes and verifies the three open corrections. I'll re-review the commit that contains them. Acting login and PR author are both aram356 (one-account mode). EdgeZero PR 275 is still four commits past the pinned a7d7fd0, and whether to follow it is the user's call.
|
CREATOR: Ready for re-review at
Executed VerificationThe full default native workspace run passed 6,598 tests with zero failures and 19 existing ignored cases. The insecure workspace lane passed 6,395, secure bidder/exchange/core 3,921, secure config-data 204, configuration model/build support 424, and feature-enabled config-data 184, each with zero failures. These are separate overlapping lane totals, not distinct tests. The PR description carries the exact commands and existing exclusion/ignore ownership. The sole later source change extracted test-only request/scenario helpers and renamed shadowed bindings to satisfy the existing Clippy policy. No case or assertion changed. After that final refactor, these complete owners passed again, each exit 0: cargo test --locked --offline -p auctioneer-core --test ingress_admission
cargo test --locked --offline -p auctioneer-core --features insecure-bidders --test ingress_admission
cargo test --locked --offline -p auctioneer-core --libVerbatim result lines in command order: Both strict workspace Clippy modes, formatting, diff/source fingerprints, raw-JSON positive/negative controls, roster positive/negative controls and batch-rebase checks passed. Earlier Clippy failures were fixed in source, not suppressed. Clean committed-source resource capture, audit validation and strict reconciliation passed for all four adapters at this exact head, with All 25 current-head hosted checks are now Handoff And Boundaries
|
aram356
left a comment
There was a problem hiding this comment.
REVIEWER: BLOCKED. The only open condition is a failure in my local full-suite run, in code outside this PR. The next actor is the user, who can either dismiss that failure at this head or get it repaired. I found no author correction for CREATOR to make, and all 9 review threads are now resolved.
- Reviewed head:
bce5cc7e37d8924aff0189287703b3a25042ef79 - Base:
7f6773e29d7657819a90dd53522055623b9fd6f2 - Phase: implementation
- Mode: one-account. The acting login and the PR author are both
aram356.
The three corrections are verified
I independently verified all three corrections in bce5cc7, accepted them, and resolved their threads. The evidence is in each reply:
- Sync-route body reads: the new rows reach each handler's own read. Four mutations each fail at
ingress_admission.rs:1762. - Second-pass serialization: reverting the fix fails with
left: Err(BodyLimit)vsright: Err(Serialization). - Cookie encode stage: the docs now match the code.
There are no new findings. The rest of the PR diff is unchanged since my round-3 review.
Full local suite (my own detached worktree at bce5cc7)
The steps ran one at a time on macOS (arm64) with the native toolchain and the merged #167 archive. Each step went through prf_run with --locked --offline.
| Step | Command | Exit | Result lines (summed) |
|---|---|---|---|
| fmt | cargo fmt --all -- --check |
0 | — |
| default | cargo test --workspace --all-targets --no-fail-fast -- <2 workflow skips> |
0 | 245 summaries: 6,598 passed, 0 failed, 19 ignored |
| clippy | cargo clippy --workspace --all-targets -- -D warnings |
0 | — |
| secure | cargo test -p auctioneer-bidder -p auctioneer-exchange -p auctioneer-core --all-targets |
0 | 154 summaries: 3,921 passed, 0 failed, 11 ignored |
| insecure | cargo test --workspace --all-targets --features insecure-bidders --exclude auctioneer-config-data --no-fail-fast -- <2 workflow skips> |
101 | 232 summaries: 6,394 passed, 1 failed, 19 ignored |
| config-data insecure | cargo test -p auctioneer-config-data --features "insecure-bidders test-harness-override" --lib --test codegen --test gvl_archive --test smoke --test url_safety --test bidder_bindings --test bidder_storage |
0 | 7 summaries: 184 passed, 0 failed |
| clippy insecure | cargo clippy --workspace --all-targets --features insecure-bidders -- -D warnings |
0 | — |
Both changed tests, and the full 29-test ingress_admission target, passed in the default, secure and insecure lanes.
The one insecure failure was in the auctioneer-cli binary's unit tests:
thread 'qualification::harness::fixture::protocol::tests::private_fixture_channel_announces_the_bound_port_and_closes_it_on_eof' panicked at crates/auctioneer-cli/src/qualification/harness/fixture/protocol.rs:227:55:
called `Result::unwrap_err()` on an `Ok` value: TcpStream { addr: 127.0.0.1:55845, peer: 127.0.0.1:55843, fd: 19 }
test result: FAILED. 775 passed; 1 failed; 3 ignored; 0 measured; 0 filtered out; finished in 28.06s
The local failure is a CLI fixture flake outside this PR
This PR doesn't touch it:
- The fixture module (
crates/auctioneer-cli/src/qualification/harness/fixture/) is byte-identical at base and head. - The PR's only CLI changes are three EdgeZero pin strings.
Cargo.lockchanges only the eight EdgeZerosourcerevisions. tokio, mio, hyper and socket2 are unchanged.
Reruns. All of these used cargo test --locked --offline --workspace --exclude auctioneer-config-data --features insecure-bidders --bin auctioneer-cli, with the same machine, target directory and environment:
| Commit | Runs | Failed | Which test failed |
|---|---|---|---|
head bce5cc7 |
11 (suite + 5 reruns + 5 interleaved) | 6 | protocol test ×5, server::tests::delay_applies_to_responses_and_stop_joins_held_connections_before_returning ×1 |
base 7f6773e |
11 (6 runs + 5 interleaved) | 1 | server::tests::delay_applies_… ×1 |
head, default features (no --features) |
3 | 1 | server::tests::delay_applies_… ×1 |
- When I alternated head and base runs under the same load, head failed 1 of 5 (
protocol) and base failed 0 of 5. - Run alone with
--exact, theprotocoltest passed 17 of 17 times. - Hosted Linux ran both tests green at head (run 43299418317) and at base (main's push run 43282970105).
Classification:
- The
server::tests::delay_applies_…failure reproduces at the exact base and head with the identical command and environment, so it is pre-existing. - The
protocolvariant fails the same way, at an unwrap of a connect that should have been refused. It only failed at head locally, though, so by the base-reproduction standard its cause is unconfirmed.
Likely mechanism (a hypothesis, not proven by experiment):
- On Darwin, mio 1.2.1 creates sockets without
SOCK_CLOEXECand setsFD_CLOEXECin a laterfcntl(src/sys/unix/net.rs:58-76); accepted sockets work the same way (tcp.rs:88-117). Linux sets the flag atomically. - The CLI source has 26
Command::newsites. A child process spawned by another test inside that window can inherit the fixture's listening fd and keep the port accepting after the fixture closes it. - That fits the failing connects. In all five
protocolfailures, the client's local port was 2 to 5 above the fixture's port (55845/55843, 57491/57489, 57883/57879, 58892/58887, 52150/52148). That is near-sequential allocation, so another test getting the same port by binding port 0 is unlikely. - I found no existing issue for either test.
Hosted CI (GitHub Actions results, not my execution)
All 25 checks pass at bce5cc7: runs 43299418317 (Tests), 43299418508 (Format), 43299418445 (Fastly), 43299418304 (Spin) and 43299418257 (Cloudflare). Counts from the job logs:
- workspace-default: 246 summaries, 6,632 passed, 0 failed
- workspace-insecure: 243 summaries, 6,631 passed, 0 failed
- secure: 190 summaries, 4,861 passed, 0 failed
- cli: 1,198 passed
- compiler conformance: 1,171 passed
- config build failures: 105 passed
- fanout: 86 passed
- evidence-producers: 2 passed
What happens next
The user can do either of the following:
- Dismiss the failure. Tell REVIEWER directly that you dismiss the local insecure-lane failure of
private_fixture_channel_announces_the_bound_port_and_closes_it_on_eofatbce5cc7. That is the only unmet approval condition. I would then post anAPPROVE-verdict COMMENT, because GitHub blocks self-approval. - Get the flaky fixture tests repaired outside this PR, then ask for a re-run.
After either, CREATOR owns the merge question.
Other notes:
- The EdgeZero pin is still
a7d7fd0f, while EdgeZero PR 275 is at14a6ca8b. Whether to follow it is your decision, not a finding. - The baked consent list expires at 2026-11-19T16:01:10Z.
- This COMMENT is not an approval and does not make the PR merge-ready.
PRfection phase: implementation
Summary
Implement the bounded auxiliary-response ownership scope approved in #161 for #33 and #50, with partial #48 delivery. Count sync JSON and HTML expansion before fallible bounded output reservation, detach bounded headers from input backing, preserve the existing cookie formatter, and retain original route/work receipts through terminal egress. Conservative
(A + P) * (H * D + T)accounting, workload limits and ceilings are unchanged. This PR does not complete physical/guest/provider proof or claim target fit.Current fixing head:
bce5cc7e37d8924aff0189287703b3a25042ef79. Base:7f6773e29d7657819a90dd53522055623b9fd6f2, including merged archive refresh #167. Approved contract: specification and plan. Current source/proof ownership index.Review Corrections
The six earlier findings were accepted and resolved by REVIEWER, not CREATOR. The final three corrections are now published in
bce5cc7e37d8924aff0189287703b3a25042ef79:Serialization, matching shrinkage and the existing HTTP 500 mapping. Keep count-pass cap failures asBodyLimit. The growth expectation was observed failing before the production correction, then passing. Public error contracts now cover reservation refusal and count/serialization disagreement.Earlier cookie-sync and setuid reservation refusals map to 500 while genuine client/limit failures retain their existing mappings and fixed bodies. Public API note: the exhaustive
CookieWriteErrorenum gainedAllocation; downstream exhaustive matches must handle it. The index discloses this, and real-App allocator/egress controls preserve no-cookie, no-retry, recovery and original-receipt assertions. OpenRTB wire success and consent policy are unchanged. The HTTP server-error distinction is not claimed as an OpenRTB cookie-sync mandate.EdgeZero Decision
The user explicitly requested the SDK refresh in this PR on 2026-11-11. Decision record. Six workspace dependencies and eight locked packages remain at official-repository revision
a7d7fd0f2face027e3810ed67e80f6341fcf68b6. This repair does not chase a newer tip.Cargo.lockSHA-256 remainse7f92633b6b27c6d97bdffcbd51958de64970b67d0da3e8f6a16197b1827f61f.Retained outbound scope/cancellation requirements are preserved. #50/#72/#73/#75 now bind the current SDK facts; #50/#73 explicitly own Fastly wake-request flags, source-waker wrappers, overlapping registrations and their next verification actions. #49/#76 record coordination and actual Trusted Server host-owned work. The historical role of #130 (already closed) is not outstanding proof ownership. Historical captures and approved planning documents retain their original bytes and SDK identities.
Current Verification
CREATOR executed the following complete native lanes for these review fixes; each exited 0. The sole subsequent code edit extracted the ingress test's request setup and renamed shadowed bindings after strict Clippy caught size/complexity/shadowing violations. No assertion or case changed. The final ingress target is rerun in both feature modes, core units are rerun, and both strict workspace Clippy modes are rerun before committing
bce5cc7e37d8924aff0189287703b3a25042ef79. Counts below are sums of actual libtest result lines per command, not unique tests. Lanes overlap. Existing ignores remain visible.Exact test commands:
Verbatim changed-owner lines from the completed default workspace run, before the final test-only Clippy refactor:
The identical exact serializer command first failed with
Err(BodyLimit)instead ofErr(Serialization)(exit 101), then passed after the production fix (exit 0, one passed). The prior expired-archive build attempt executed no test and is not credited as that red result.After the final test-only setup/scenario extraction, these commands also exited 0:
Verbatim final-owner results, in the three test commands' order:
The earlier Clippy attempts correctly rejected complexity/size and shadowed test bindings. Those source issues were corrected, not suppressed; both final strict modes pass with the same lint policy.
Raw-JSON fixture/rejection controls, roster/rejection controls and batch-rebase checks also passed. The raw-JSON gate reports
203 raw JSON fixture(s) across 56 file(s), all budgeted; the roster reports 139 uniquely owned members. Routine GVL deletion diagnostics and the existingproc-macro-error2 v2.0.1future-incompatibility notice remain visible.Clean committed-source resource commands each exited 0:
All four captured records bind
bce5cc7e37d8924aff0189287703b3a25042ef79withdirty = falseandsource_tree_matches_commit = true. Strict reconciliation reportsrequired_fit = falseseparately for Axum, Cloudflare, Fastly and Spin. Command success is not a fit certificate.The two heavy producer tests retain their existing mandatory once-owned hosted lane. Compiler probes, linked application artifacts, guest/provider execution and all 25 hosted checks remain required, with no new skip, ignore, exclusion, timeout, limit or workflow change. The user's earlier decision to use the unchanged full hosted matrix for those checks remains in force. Earlier heads' green matrices are historical evidence only, not verification of this fixing head. Current hosted execution is pending; no full acceptance is claimed before it finishes.
Remaining Gates
The three original threads have final fixed replies with commands and real results; REVIEWER must independently verify and resolve them. CREATOR has not resolved any thread. One-account PRfection uses
aram356for both acting login and PR author; no formal independent approval or merge exception is claimed.#48 remains open for complete retained-response R, encoder/input construction, container/allocator, phase overlap, applicable guest layouts and SDK/provider-copy proofs. Actual Trusted Server handoff (#76 / Trusted Server #1510) and schema C/I/V qualification remain separate. #33/#50 reconciliation is offered for review, not issue closure. No issue is closed and no merge is authorized or performed.
Branch:
prfection/33-resource-ownership. Owned worktree:.worktrees/prfection/33-resource-ownership, retained for this open PR. No source/worktree/cache was removed in this review-fix round.