Repository navigation
Plan shared resource ownership and bounded auxiliary responses - #161
Conversation
aram356
left a comment
There was a problem hiding this comment.
REVIEWER: REQUEST_CHANGES — GitHub blocked this review event: HTTP 422 {"errors":["Review Can not request changes on your own pull request"]}. The acting login is the PR author (aram356), so this is posted as COMMENT. It carries the same verdict and both inline findings; it is not a change-request state GitHub will enforce.
Planning PR (PRfection phase: plan). Reviewed head b7910d05245103b352e2cbec943aa9e7974b3eaf, base 499f0e540e95e1c86b730c1030aabef28de01d0a (current main). Acting login and PR author are both aram356 (one-account mode). The diff adds exactly two documents and no code. There were no earlier reviews or threads, so this round has nothing to resolve.
The design is sound, and the plan is unusually precise: almost every concrete claim I checked against the code was exact. I'm requesting changes for two should-fix findings that need a CREATOR disposition before approval. Neither one is a design defect.
Findings
| # | Severity | Anchor | Summary |
|---|---|---|---|
| 1 | should-fix | plan :212 |
--lib handlers::health_gvl:: selects no tests at 499f0e5; the six health integration owners go unnamed, and #160 rewrites that route |
| 2 | should-fix | spec :124 |
Per-row responsible role and next action make the index a local ledger, against AGENTS.md's backlog-of-record rule (#140, 2026-11-05) |
Planning gates
- The spec records scope approval (
:21, "AG approved retaining the current permits…"), and the plan records its status (:11). Both state that neither implementation nor merge is authorized. - The body names
superpowers:brainstormingandsuperpowers:writing-plans. The plan carries the writing-plans header and checkbox steps. - Every acceptance criterion on #33, #50 and #48 maps to a spec section, including #50's retired-ledger items: host bookkeeping and Fastly recorder storage in §4, and ownership of the Fastly stack by #73 at
:193. The plan keeps the #33 → #50/#48 order and usesRefs, not closing keywords.
What I checked against the code at 499f0e5
Paths and identifiers. All 61 file paths the documents name exist. All 22 named functions, types, constants and exact test names exist, including validate_snapshot, validate_components, component_values, check_capture, compare, growth_between, bounded_json_line, serialized_json_bytes, AuxWorkCharges.output, PROBE_CONFIG_MIN_PERMITS, and all three records::tests::… names used with --exact.
Commands. All 50 --test selections exist with their required-features satisfied. My first pass flagged five, all false positives: application is the CLI's default feature, and build-support enables config-model. All 10 named --features exist on their packages. Plan lines 76 and 318 are byte-for-byte the commands CI runs at test.yml:124, and the flag constraints at bidders/mod.rs:56-63 permit both forms. reconcile-resource-fit.sh accepts exit 0 or 1 only when the JSON required_fit verdicts agree, exactly as the plan says. Of the module filters, handlers::sync_http:: (27 tests), serialize:: (14), handlers::tests:: (13), usersync::cookie:: (21), info_bidders:: (29), gates:: (14), resource_profile:: (2) and bidders::resources_platform:: (9, matching plan :31) all select tests; health_gvl:: selects none (finding 1). bidders is declared in main.rs, not lib.rs, so plan :29's --bin rule is correct.
Behavioral claims the design depends on, all exact:
- The current auxiliary charge is
(A + P) × (D × H + T)(budget.rs:1720-1738,aux_term), withP = max(A, PROBE_CONFIG_MIN_PERMITS), as plan:264states. Probe output is charged separately (probe_output_term). Given its premises (R ≤ D,H ≥ 1), the proposedA·(D + (H−1)·R + T) + P·Dcan never exceed the current term, so a mistaken premise fails safe. validate_snapshotcallsvalidate_componentsatrecords/mod.rs:664, beforematch modeat:666. Frozen records therefore pass through current component arithmetic, which is precisely the cutover hazard plan:289-292names.growth_betweenuseschecked_subon every catalog term, andcomparerequires matchinglock_sha256andbaked_config_sha256. So the documents correctly treat a handoff saving as a refused comparison rather than as growth.bounded_json_linerefusesmax < 1and caps the raw output atmax − 1before pushing the newline (sync_http.rs:672-702). Task 3's arithmetic therefore preserves today's refusal boundary to the byte. The same code confirms spec §2's gap: growableBoundedBodys, raw and escaped buffers retained together, and a finalpush.allowed_originbuilds itsHeaderValuefrom the matched configured origin (sync_http.rs:203-206).usersync_wire_reference.rsmakes no production calls ("handler response comparisons live in their unit owner"), whilehandlers/mod.rstests drive production handlers against the samewire.json. That confirms plan:171-173.
SDK facts (spec §4) against the pinned checkout at 598f0a2a490eb83dd31c06c024a0cefa8e049628, found through locked offline metadata:
| Adapter | Spec | SDK source |
|---|---|---|
| Fastly | 128,000,000 B; 1,000,000 B stack; one inbound | 128_000_000, PerExecution, Some(1_000_000), NonZeroU32::MIN |
| Cloudflare | 128,000,000 B per instance; inbound unknown | 128_000_000, PerInstance, unknown(ProviderUnpublished) |
| Hosted Spin | 134,217,728 B per execution; inbound one | 128 * 1024 * 1024, PerExecution, HostedDefault, NonZeroU32::MIN |
Axum's dev_server.rs and ingress_config.rs exist.
Issue routing. Every referenced issue exists, is open, and has a title matching its assigned role: #9, #29, #46, #47, #49, #72, #73, #74, #75, #76, #88, #147, #148, #150, and Trusted Server #1510.
Not independently verified: the "17/18 native / 3/4 guest serving bindings" and "15/16 schemas" counts (spec :72-75). No existing test pins them, and confirming them needs a build. Plan Task 1 re-derives membership from source, so I'm leaving them to that step rather than certifying them here.
Verification
CI at b7910d0. 25/25 jobs succeeded across the five workflows. The workflows have no paths filters, and the Run Tests jobs ran for 191 s to 1,569 s, so none was skipped. From the log archive (run 42697947033), there are zero test result: FAILED lines, and workspace-default shows summaries=246 passed=6556 failed=0 ignored=20, identical to the base, as a docs-only change should be.
The author's local run was at ed17282. I diffed ed17282..b7910d0: the only change is one table cell (catalog_continuity → records/catalog_continuity), so that run covers code identical to the head. It is the author's evidence, attributed to them, not mine.
My own runs, in a detached REVIEWER worktree at b7910d0:
cargo test --locked --offline -p auctioneer-core --lib handlers::health_gvl:: -- --list
→ 0 tests, 0 benchmarks (exit 0) finding 1
cargo test --locked --offline -p auctioneer-core --lib handlers::sync_http:: -- --list
→ 27 tests listed positive control
cargo test --locked --offline --workspace --all-targets --no-fail-fast -- <CI's two producer skips>
→ IN PROGRESS at submission. Not a pass; I will post the aggregate as a follow-up.
Merge readiness
Not approved. Both findings need a CREATOR disposition, and reviewDecision is empty. mergeStateStatus reads CLEAN only because nothing mechanical blocks this PR, so it is not a merge signal.
Next actor: CREATOR, for both findings. When they are accepted and the head is re-verified, the next step is CREATOR's merge question to the user, since this account cannot formally approve its own PR.
|
REVIEWER: My full local suite at the reviewed head The run was in a detached REVIEWER worktree, with Verification is now complete for this docs-only head. The verdict is unchanged: REQUEST_CHANGES. The two should-fix threads still need CREATOR dispositions before I can accept this phase. |
|
CREATOR: Re-review requested at
I ran the full default-workspace CI lane: exit 0, Readback confirms OPEN, non-draft, base Account mode: one account; acting login and PR author Next actors: CI and REVIEWER. Please verify these dispositions at this head and resolve the original threads only if accepted. |
aram356
left a comment
There was a problem hiding this comment.
REVIEWER: APPROVE — formal approval not submitted. The acting login aram356 is also the PR author, and GitHub does not accept this account's review events on its own PR. It already rejected REQUEST_CHANGES on this PR with HTTP 422 (review 5852409669), so this is a COMMENT. It is not a formal approval and does not make the PR merge-ready.
Reviewed head: c66bbbbf4c0a2c9ae87d2b13e2225f4bc7e86ed1 · phase: plan. git diff --stat b7910d0 c66bbbb shows the two planning documents only: 2 files, +50/−13.
Findings
Both findings are verified fixed, and I have resolved both threads:
- Health selector and #160 overlap: resolved. The six integration targets replace the zero-dispatch filter, and #160's merge-order reconciliation is named under Baseline And Ownership.
- Proof index versus GitHub backlog: resolved. Rows carry source-bound proof state and the owning issue. Responsible roles, next actions, status and blockers live on the issues, and spec §8 keeps #33's and #50's obligations there.
I found no new issues in the revised text.
Verification
Basis. At the user's direct instruction, the full-suite gate at this head rests on hosted CI. My local full run was stopped before it completed (see below). It is not reported as passing.
Hosted CI at c66bbbb: 25 of 25 check runs success. I downloaded every job log and tallied its test result: lines; the counts are mine, from those logs:
| Job | Summaries | Passed | Failed |
|---|---|---|---|
| cargo test (workspace-default) | 246 | 6556 | 0 |
| cargo test (workspace-insecure) | 243 | 6555 | 0 |
| cargo test (secure) | 190 | 4760 | 0 |
| cargo test (cli) | 25 | 1196 | 0 |
| cargo test (compiler conformance schema) | 14 | 1045 | 0 |
| cargo test (config build failures) | 2 | 104 | 0 |
| cargo test (fanout conformance suite) | 7 | 86 | 0 |
| cargo test (compiler conformance settings) | 2 | 56 | 0 |
| cargo test (compiler conformance exports) | 3 | 36 | 0 |
| cargo test (compiler conformance discovery) | 2 | 34 | 0 |
| fastly compute build + package size | 2 | 4 | 0 |
| cargo test (evidence-producers) | 2 | 2 | 0 |
| schema source selected-schema / private-layout; application artifact axum / cloudflare / fastly / spin; spin build + component size | 1 each | 1 each | 0 |
The remaining six jobs are compile, lint or size checks and print no test summaries: cargo fmt + clippy, cargo clippy (fanout conformance suite), the three wasm target checks, and wrangler dry-run + upload size.
Focused (my run, detached REVIEWER worktree at c66bbbb):
$ cargo test --locked --offline -p auctioneer-core --test health_gvl --test health_gvl_auth --test health_gvl_failclosed --test health_gvl_observer --test health_gvl_stale_observer --test health_gvl_docs
exit 0 — 9 + 3 + 1 + 1 + 1 + 1 = 16 passed, 0 failed
Local full suite (my run, stopped at the user's instruction, not complete):
$ 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
started 03:50:09Z, last output 04:56:19Z, then stopped; no exit code; partial: summaries=170 passed=4804 failed=1 ignored=19
The host was heavily oversubscribed by unrelated concurrent builds: load average about 40 on 14 cores. The one failure was:
crates/auctioneer-adapter-axum/tests/wave_transport.rs
tests::the_wave_drive_preserves_sibling_deadlines_and_original_slots_on_the_wire ... FAILED
panicked at crates/auctioneer-adapter-axum/tests/wave_transport.rs:783:28:
the recovery call succeeds: Transport(TransportError("outbound send failed"))
test result: FAILED. 13 passed; 1 failed
It did not reproduce. Five isolated reruns of cargo test --locked --offline -p auctioneer-adapter-axum --test wave_transport at this head each gave test result: ok. 14 passed; 0 failed. The same target gives 14 passed; 0 failed in both CI jobs that run it at this head, and in my full local run at b7910d0, whose non-document files are byte-identical. This PR changes no code. I read it as a load-sensitive loopback flake. I have not reproduced it at the base, so I make no pre-existing claim.
Outside this diff
Draft #162's index (docs/testing/resource-proof-ownership.md:39 at 9d07e5d) still has a Responsible Role And Next Action column. It was written against the superseded row contract and needs to follow the corrected one when #162 is refreshed.
Next step
One-account mode: CREATOR takes the merge question to the user. Merging without an eligible independent formal approval is a one-time exception only the user can grant for this PR and head, and CREATOR records it on the PR. This plan acceptance does not authorize merging any implementation.
|
CREATOR: One-time merge exception, one-account mode.
|
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.
PRfection phase: plan
Refs #33, #50 and #48.
Summary
This PR proposes the shared resource-proof inventory, SDK/adapter correspondence and bounded auxiliary response plan. It contains only the specification and implementation plan, closes no issue, and authorizes neither implementation nor merging.
Reuse the existing permits, completion ownership, adapter profiles and additive budget. Workload limits, ceilings, headroom, configuration flow, privacy, cookie integrity, dependencies, source pins and historical evidence remain unchanged. An accounting reduction still requires complete storage, lifetime, population and applicable host-copy proofs.
Review Corrections
Both submitted findings are corrected in
c66bbbbf4c0a2c9ae87d2b13e2225f4bc7e86ed1:The revision changes two documents only. It does not publish or modify implementation PR #162. The local document review found no additional issues; that is not independent GitHub approval. Both review threads remain open for REVIEWER.
Executed Verification
I executed these checks on the working-tree bytes committed as
c66bbbbf4c0a2c9ae87d2b13e2225f4bc7e86ed1, using Rust 1.98.1, SDK598f0a2a490eb83dd31c06c024a0cefa8e049628, the existing pinned source repository andCARGO_PROFILE_DEV_DEBUG=0as in CI. The new commit differs from reviewed headb7910d05245103b352e2cbec943aa9e7974b3eafonly in the two documents; no runtime, test, manifest or lockfile changed.The original selector was reproduced, not treated as passing verification:
The replacement owners all executed, exit 0:
cargo test --locked --offline -p auctioneer-core --test health_gvl --test health_gvl_auth --test health_gvl_failclosed --test health_gvl_observer --test health_gvl_stale_observer --test health_gvl_docsThe full default-workspace CI lane exited 0, with aggregate
summaries=245 passed=6522 failed=0 ignored=20: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_catalogThose are the workflow's existing separately owned producer exclusions, not new skips. They, the other feature lanes, compiler/guest/provider jobs and
test = falseowners remain mandatory in current-head CI. This native lane is not the entire acceptance matrix.Representative actual result lines:
These commands also exited 0:
Actual policy output included
batch-API rebase clean,roster arithmetic consistent; 139 members enumerated, uniquely owned, every counted label discharged, andok: 203 raw JSON fixture(s) across 56 file(s), all budgeted. No assertion, ignore, fixture allowance, workflow or warning policy was weakened.Handoff And Remaining Gates
Head:
c66bbbbf4c0a2c9ae87d2b13e2225f4bc7e86ed1. Base:main. Branch:prfection/33-resource-ownership-plan. Worktree:.worktrees/prfection/33-resource-ownership-plan. Account mode: one account; acting login and PR author arearam356. Skills: PRfection creator, receiving code review, using Git worktrees, requesting code review and verification before completion; the original planning phase used brainstorming and writing plans.Current-head hosted checks and independent acceptance remain required; previous-head green checks are not attributed to this commit. No resource/guest/provider qualification or target fit is claimed. Keep #33, #50 and #48 open until their independent acceptance criteria are delivered. The planning PR requires reviewer acceptance and the user's merge decision; do not merge implementation code into this planning branch.
No thread was resolved, no issue was closed and nothing was merged. Existing planning/implementation worktrees and their caches remain needed by the open stack; none is eligible for removal. Next actors: CI and REVIEWER for this head.