alksocks/tasks/implementation/review-protocol-core.md

id, name, status, depends_on, scope, risk, impact, level

id	name	status	depends_on	scope	risk	impact	level
review-protocol-core	Review (the protocol core before any assembly-layer work)	completed	implementation/core-integration-suite	narrow	low	project	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, with the contract test in place pinning 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, since 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 clean); 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 and 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, which is 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, with 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, so 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, which is 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, which 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), matching ADR-003's re-read of the POC finding.

8. Docs sync: clean, with 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), while the landed signature takes (&ChannelCore, &OperationRegistry, AuthContext, Option<IdentityWitness>) 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

• Seam contracts hold (readiness / ownership / accept-future / framing-once), with R-1 corrected
• Wire fidelity: the frame, phase model, sentinels, lifetimes byte-checked against wire.md/bast.md
• Access control: both gates + never-implied grants pinned, the prior session's reconciliation verified
• Teardown/lifecycle: pump_bidi inline, the matrix pins the semantics, no teardown-at-birth paths
• Convention compliance: no unwrap/expect/panics outside tests after R-2; thiserror; naming; module structure; vocabulary
• Both wasm checks green (from clean), no net-shaped feature, tokio on the wasm-clean subset
• POC-fidelity spot-check of the three trickiest behaviors: the zero-yield drift (R-1) found and fixed
• Docs sync: specs match the tree; residues fixed in the same change
• 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, which 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<TakenInner, SpentSessionError> / Result<TakenAssociateInner, SpentSessionError> 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"), which is test-side and 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", which 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-
  1. carries the open gate unconditionally; a bind-only identity therefore fails closed for every command (FORBIDDEN at the registry, the establisher never runs, and no phantom channel is 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, being 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.