--- id: review-protocol-core name: Review — the protocol core before any assembly-layer work status: completed depends_on: [implementation/core-integration-suite] scope: narrow risk: low impact: project level: review --- ## Description Review checkpoint before batch 2 (the assembly layer: `local` providers, front doors, `composed` providers). Everything in batch 2 consumes the seams and the session types, so a core defect — a seam contract drift, a teardown hole, a wasm regression — would propagate into every downstream task. This review is the gate between the core group and the assembly layer, and the point where batch 2 gets decomposed (its task shapes may adjust to what the core actually landed). Review against the architecture, in this order: 1. **Seam contracts** (ADR-004, producer.md §The Three Seams): the readiness contract (`next_reply` `Pending` when nothing is ready — the contract test exists and pins it), ownership-rides-the-handle (no `_` bindings of owning values; handles own their payloads structurally), the accept future owning its resources end-to-end (the quiet-wait drop IS the client-gone teardown), framing-once (the egress speaks inner bytes; the wire layer frames once). 2. **Wire fidelity** (wire.md, ADR-003/ADR-007): the datagram frame (joint bound, F-2 at payload level), the phase model (no mode byte; post-reply garbage refused structurally in all three commands), the sentinel surfaces (ASSOCIATE's `127.0.0.1:0`, BIND's variant-3 zeros), association lifetime = channel lifetime. 3. **Access control** (ADR-006): the open gate (`socks5:open` at the registry), the command gate (`socks5:bind` at BIND dispatch), two grants never implied; refusals RFC-faithful everywhere (convention 2 — never collapsed into generic errors). 4. **Teardown/lifecycle** (wire.md §Sentinels, alkcall ADR-050): `pump_bidi` awaited inline (R-02 — no spawn-and-forget); the teardown matrix tests actually pin the semantics; no teardown-at-birth paths (the third-witness pattern held). 5. **Convention compliance** (AGENTS.md): no `unwrap`/`expect`/ panics outside tests; `thiserror` surfaces; no comments in code except doc comments + pinned non-obvious-correctness notes; naming; module structure (one module per file, `lib.rs` re-exports); producer/consumer vocabulary (no server/client framing outside RFC vocabulary). 6. **Wasm-clean invariant** (ADR-009): both wasm checks green; no `net`-shaped feature; tokio on the wasm-clean subset; the harness and tests stay out of the wasm build. 7. **POC-fidelity spot-check**: for two or three of the trickiest behaviors (the readiness contract, the quiet-wait two-exit semantics, the split-datagram reader persistence), diff the crate's behavior against the POC crates' findings — the POCs are the empirical record; drift is a finding. 8. **Docs sync**: the specs match what landed (the integration task's reconciliation verified; any residue is a finding). ## Notes > Filled by the review agent — see the Summary and the Findings. ## Summary Review performed 2026-09-24 against the eight-criteria order; the gate opens. Two findings were corrected in the review change itself (R-1 readiness-contract hardening; R-2 a non-test `expect`), plus three minor residues (R-3..R-5, fixed in the same change); no seam-contract, wire-fidelity, ACL, teardown, wasm, or POC-fidelity drift was found. One prior-session reconciliation (the bind-only identity row) was independently verified as correct — see the Notes. **1. Seam contracts (ADR-004, producer.md §The Three Seams) — clean, one finding corrected (R-1).** The readiness contract is documented on the trait (`egress.rs:174-185`), table-tested, and pinned behaviorally twice (the producer unit test's mutex-guarded hot egress + the integration suite's hot-`Ok(None)` under concurrent load). Ownership rides the handle structurally (`TargetHandle`/ `BindTargetHandle` payloads own their halves; no `_` bindings of owning values anywhere in the crate — swept); the accept future owns its resources end-to-end (`BindAccept::new` boxes it; the quiet wait's `select!`-pinned drop is the client-gone teardown, pinned by the drop-flag tests in `src/producer.rs` and `tests/core- integration.rs`); framing-once holds (`Egress::send` takes payload only, `next_reply` returns inner bytes, `frame_inner`/`frame` are the only prefixers; `frame == frame_inner` byte-identity is table-tested). R-1 is the readiness contract's own tolerance: see the Findings. **2. Wire fidelity — clean.** The datagram frame matches wire.md §The Data Plane byte-for-byte: `[len: u16 BE][addr + data]`, the joint bound (`frame`/`frame_inner` reject > 65535 with the exact per-form maxima pinned: 65528 v4 / 65516 v6 / 65276 domain), F-2 at payload level (empty datagram round-trips end to end, never EOF; `len == 0` inner is `EmptyFrame` — unreachable from a compliant peer), RSV/FRAG absent from the in-band codec. The phase model holds: no mode byte; post-reply garbage is refused structurally in all three commands (CONNECT = pure pipe, ASSOCIATE = garbage ATYP fails loud, BIND = the quiet wait's one-byte read ends the session with no reply#2) — each pinned by test. Sentinels verified: ASSOCIATE's `127.0.0.1:0` (asserted in the integration suite + the raw probes), BIND's variant-3 zeros (`0.0.0.0:0` reply#1 + reply#2, pinned); association lifetime = channel lifetime (EOF rows in the teardown matrix). The BAST document's joint-constraint note matches the codec. **3. Access control (ADR-006) — clean.** The open gate is `required_scopes = ["socks5:open"]` on the single op spec (`socks5_open_spec`); the command gate resolves `socks5:bind` from the per-call opener identity at BIND dispatch and precedes the accept seam (a scope-less BIND never mints a listener — pinned). Both refusals are RFC-faithful (0x02 at the command gate; the registry's `FORBIDDEN` at the open gate); the mapping tables (dial/egress/accept/factory errors → reply codes) are table-tested and route through the forked `ReplyError` — nothing collapsed into generic errors. The integration suite's identity matrix pins all three rows end to end with the CF-006 witness asserted. **4. Teardown/lifecycle — clean.** `pump_bidi` is awaited inline in all three data planes (CONNECT, BIND post-reply#2) inside the handler task; the `JoinHandle` tracks the data-plane lifetime (R-02 held — no spawn-and-forget). The teardown matrix tests pin the real semantics (half-close / EOF / client-gone-with-parked-accept, each followed by fresh sessions of every command). No teardown-at-birth paths: the egress-drop witness test flips the flag on stage exit, the accept-drop witness flips on the client-gone branch; the consumer sessions reap on `close`/`shutdown`/`Drop` with the leak probes asserting no phantom channels on either side. **5. Convention compliance — one finding corrected (R-2).** No `unwrap`/`expect`/`panic!`/`unreachable!` in non-test library code after R-2 (swept: `src/` non-test regions are clean; the forked core's panic translations were re-verified). `thiserror` surfaces throughout; naming/module structure (one module per file, `lib.rs` re-exports) hold; producer/consumer vocabulary holds (RFC-facing "server"/"client" only inside `src/rfc1928/`, the RFC vocabulary's home). Cargo.toml's two inline policy comments are the extraction-review F-3-accepted posture (carried, no fix demanded). **6. Wasm-clean invariant — green, with one accepted note (R-5).** Both wasm checks clean (`cargo check`/`clippy --target wasm32-unknown-unknown -- -D warnings`, from scratch); no `net`-shaped feature (the two declared features are empty markers); tokio on the wasm-clean subset with `default-features = false` (no `full` in `[dependencies]`); the harness/tests ride dev-deps only. R-5: see the Findings (a transitive `getrandom` carrier outside this crate's control). **7. POC-fidelity spot-check — clean, one finding corrected (R-1).** - *Readiness contract*: the crate's stage matches the POC's `select!`-driven shape (channel reads ∥ reply drains, `Ok(None)` tolerated as a spurious wakeup); the POC's LocalEgress/ ComposedEgress both park properly (`readable()` / queue `recv().await`) and the ComposedEgress's closed-queue arm returns `Err` rather than a hot `None` — exactly the posture the crate inherited. The drift the spot-check surfaced is the *zero-yield* bound of the contract (R-1) — a pure `async { Ok(None) }` egress livelocked the landed stage on a current-thread runtime (empirically reproduced; the POC's findings named the biased-select and hot-None hazards, the no-yield variant is the same class one step further). Fixed in the review change. - *Quiet-wait two-exit semantics*: the landed `select!` is single-shot with exactly the two exits (accept / EOF-garbage-error), not a loop — matches the POC's `never_loop` finding; the parked- accept probes make both exits deterministic; reply#1/reply#2 carry the factory's `bind_addr`/peer byte-exactly on the provider variant and the sentinels on variant 3. - *Split-datagram reader persistence*: `AssociateSession::recv_ datagram`'s reader + pending queue persist across calls (pinned by the 2-byte-prefix chunk test and the consumer-associate suite) — matches ADR-003's re-read of the POC finding. **8. Docs sync — clean, three residues fixed in the review change.** The integration task's reconciliation was verified against ADR-006 §Consequences, producer.md §Scope Gating, and the landed spec/tests (see the Notes). Residues found and fixed: `src/egress.rs`'s module doc still said the readiness tests "land with the producer-associate task" (that task is landed — reworded to point at where they live); `src/producer.rs`'s establisher doc said "in this task's scope" (same class — reworded); producer.md §Wiring's sketch showed `producer.register(&mut channel_core)` — the landed signature takes `(&ChannelCore, &OperationRegistry, AuthContext, Option)` and also carries the establisher (corrected). The rest of the specs (wire.md, consumer.md §Wiring, ADR-006, rfc1928.md, README Current State, lib.rs crate docs) match the tree. ## Acceptance Criteria - [x] Seam contracts hold (readiness / ownership / accept-future / framing-once) — with R-1 corrected - [x] Wire fidelity: the frame, phase model, sentinels, lifetimes byte-checked against wire.md/bast.md - [x] Access control: both gates + never-implied grants pinned, the prior session's reconciliation verified - [x] Teardown/lifecycle: pump_bidi inline, the matrix pins the semantics, no teardown-at-birth paths - [x] Convention compliance: no unwrap/expect/panics outside tests after R-2; thiserror; naming; module structure; vocabulary - [x] Both wasm checks green (from clean), no net-shaped feature, tokio on the wasm-clean subset - [x] POC-fidelity spot-check of the three trickiest behaviors — the zero-yield drift (R-1) found and fixed - [x] Docs sync: specs match the tree; residues fixed in the same change - [x] Findings recorded; the gate to batch 2 opens ### Findings - **R-1 (major → fixed in the review change)**: the datagram stage's `Ok(None)` tolerance (re-park) is safe against a hot egress only while its futures yield at least one poll boundary. A *zero-yield* egress — `next_reply` = a pure `async { Ok(None) }`, no await at all — wins every `select!` poll forever: the channel branch and every other task on a current-thread scheduler starve (empirically reproduced: the echo never arrives; the multi-thread runtime survives, so the landed mutex-guarded unit test masked the hazard at its lock's yield boundary). The POC's contract wording ("must be `Pending` when nothing is ready") is right; the landed stage's handling had one gap. Fix: the stage's `Ok(None)` handler now `yield_now()`s before re-parking (`src/producer.rs` `run_datagram_stage`), plus a pinning unit test with a pure zero-yield egress (the strictest violator — hangs without the yield) alongside the existing hot-None test. ADR-003's contract prose and `egress.rs`'s trait doc stay as written (the contract is on the egress; the stage now additionally defends itself). - **R-2 (convention, fixed in the review change)**: `Socks5Channel Session::into_inner` / `AssociateSession::into_inner` ended in `inner.expect("into_inner on a spent session")` — the only non-test `expect` in `src/`. Reachability is narrow (a spent session's inner is gone only after close/shutdown/Drop, and no public API constructs a spent session), but convention 2's rule is the rule: the shape is now `Result` / `Result` with a zero-carrier error type (`SpentSessionError` — a spent session's manager entry is always already reaped, so dropping it loses nothing; the large-Err/unit-Err lints shaped the choice), pinned by two spent-session unit tests. Test call sites updated (`.expect("the session is live")` — test-side, allowed). - **R-3 (doc residue, fixed)**: `src/egress.rs` module docs + the `src/producer.rs` establisher doc carried task-time phrasings ("lands with the producer-associate task" / "this task's scope") that are stale now the tasks are landed. Reworded to point at the landed reality. - **R-4 (doc residue, fixed)**: producer.md §Wiring's assembly sketch showed a one-argument `producer.register(...)`; the landed signature takes the core/registry/auth/witness quadruple. Reconciled (the same sync discipline the integration task applied to consumer.md). - **R-5 (doc residue, fixed)**: the `bind_quiet_wait_is_not_under_the_establishment_bound` test doc claimed the substrate-free probe exercises "the alkcall 10s default bound" — it cannot (the substrate-free shape has no alkcall bound; the over-channels wait-outlives-establishment behavior is the harness's pin by construction). Reworded to say what the test actually pins (the wait is teardown-governed, no in-stream timeout). - **R-6 (accepted, no fix demanded)**: the wasm-clean invariant rides a transitive feature unification outside this crate: `alkcall → jsonschema/referencing → ahash (runtime-rng) → getrandom 0.3.4`, and `wasm_js` (required to compile `getrandom` on `wasm32-unknown-unknown`) is enabled only because jsonschema/ referencing declare it for the wasm32 target. The default-crate check passes today (verified from a clean target dir), and the alkty sibling shows the identical posture; but a future alkcall/jsonschema feature change could silently break the wasm gate with no in-tree cause. Not actionable in this crate (the dep graph is alkcall's); noted for the wasm-adapter story's eventual OQ-SK2-06 work and for the batch-2 wasm checks. ## References - docs/architecture/wire.md, producer.md, consumer.md, rfc1928.md, bast.md, README.md (the specs under review) - docs/architecture/decisions/001/003/004/006/007/009/013 - docs/research/poc-udp-associate-findings.md, poc-bind-findings.md, poc-connect-wasm-findings.md (the POC fidelity sources) - /workspace/alksocks-udp-poc (the POC reference for the readiness contract's empirical record) - AGENTS.md (conventions 1–17; the verification commands) ## Notes - The prior integration session's Notes/Summary were read and cross-checked against the specs and the tree. The one reconciliation it flagged — the task file's identity-matrix row 2 expecting a bind-only identity to serve BIND, vs ADR-006's landed `required_scopes = ["socks5:open"]` unconditionally — is **correct as landed**: the op spec (the single op, all three commands, ADR- 001) carries the open gate unconditionally; a bind-only identity therefore fails closed for every command (FORBIDDEN at the registry, the establisher never runs, no phantom channel — pinned producer- and consumer-side in `identity_matrix_bind_only_fails_closed_at_the_open_gate`), and a BIND-capable deployment grants both scopes explicitly. The suite pins this; ADR-006 §Consequences states the asymmetry explicitly ("never implied, in either direction"); the task file's Notes record the reasoning. No residue: the drift was a Phase-0-era task phrasing, not a spec-vs-code contradiction. The user's flag was well-placed — this review treats the reconciliation as verified rather than open. - The self-reported verification claims reproduce (the landed suite was green at review start; this review's fixes re-ran the full sweep — see below). - Post-review verification (the review change included): `cargo test` 348 green (121 lib + 38 connect + 39 associate + 40 bind + 40 integration + 30 differential + 37 rfc1928 + 3 doctests — the two R-1/R-2 additions and the four R-2 call-site updates included), `cargo test --all-features` green, `cargo clippy --all-targets -- -D warnings` clean, `cargo fmt --check` clean, both wasm checks clean (re-run from a clean wasm target dir), `cargo doc --no-deps` clean. The batch boundary holds: no `local`/`composed` code exists.