alksocks/tasks/implementation/review-assembly-layer.md

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

id	name	status	depends_on	scope	risk	impact	level
review-assembly-layer	Review — the assembly layer (the batch-2 gate)	completed	implementation/assembly-integration-suite	narrow	low	project	review

Description

Review checkpoint closing batch 2 (the assembly layer: local providers, the front doors, the producer-side local backend, composed providers). After this gate the crate is feature-complete against the Phase 1 spec set; the remaining work is release polish (publish prep, the OQ-SK2-08 tuning note).

Review against the architecture, in this order:

1. The never-binds posture (the crate's defining requirement): the protocol layer still binds nothing — every kernel bind lives in local-gated code, is an explicit constructor decision, and the default crate's dependency graph carries no tokio/net (cargo tree spot-check). The doors' binds are the doors' own (assembly-layer, ADR-009).
2. Seam-contract compliance by the providers (ADR-004): the readiness contract honored by both egress providers (real async parks; no hot Ok(None)); ownership rides the handle structurally (the composed owner-wrapper inside the payloads; no _ bindings of owning values); the accept future owns its resources end-to-end (both accept variants); framing-once (the providers speak inner bytes; no double prefix).
3. Door fidelity (ADR-005/ADR-003): the NoAuth door is a literal raw pipe; the cred-bearing door terminates RFC 1929 locally and re-originates in-band (never pipes raw post-auth); the password never crosses the channel as a SOCKS password; the RFC UDP-header codec appears only at the door boundary; FRAG ≠ 0 refused.
4. The graduation's shape (ADR-007 variant 2): the composed accept advertises the real bound and the real peer — no expected-addr stand-in; the POC's superseded workaround is not resurrected.
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 (feature-gated backend modules never imported from the protocol/adapter/client modules — convention 15's last line); producer/consumer vocabulary.
6. Feature-gate hygiene (ADR-009): local brings only tokio/net (+ socket2 if justified); composed brings only alktunnels; the default graph unchanged; the wasm checks green for default + composed; local on wasm fails to build (the alktty posture — verify the failure is the expected one).
7. POC-fidelity spot-check: for two or three of the trickiest assembly behaviors (the composed egress's LRU eviction + closed- queue posture, the cred-bearing door's re-origination, the bind-first correlation), diff the crate's behavior against the POC crates' findings — the POCs are the empirical record; drift is a finding (except where an ADR supersedes the POC's shape — the bind-first graduation is the known one).
8. Docs sync: the specs match what landed (the integration task's reconciliation verified; any residue is a finding).

Summary

Review performed 2026-09-26 against the eight-criteria order; the gate opens (the crate is feature-complete against the Phase 1 spec set — release polish remains). One major correctness finding was corrected in the review change itself (R-1: the UDP door's in-band bridge lost the reader-state contract), plus a convention sweep residue (R-2: seven non-test expects → typed errors, the batch-1 R-2 class) and two doc residues (R-3, R-4). No seam-contract, never-binds, graduation-shape, feature-gate, or POC-fidelity drift was found beyond these.

1. The never-binds posture — clean. The protocol layer (producer.rs/consumer.rs/wire.rs/params.rs/egress.rs/ error.rs/rfc1928/) names no kernel-socket type (tokio::net appears only in doc prose; zero code references — swept). The default dependency graph carries no tokio/net/mio/socket2 (cargo tree -e normal -f "{p} [{f}]": tokio rides the wasm-clean subset bytes,io-util,macros,rt,sync,time; net appears only under --features local, which also brings mio+socket2; composed adds alktunnels 0.2.0 only and its tokio stays on the clean subset). Every kernel bind is an explicit constructor decision in local-gated code (local/mod.rs — the dial's connect, LocalEgress::bind's loopback 127.0.0.1:0 (a wildcard bind is the caller's variant, doc pinned), local_bind_accept's explicit bind_host param, the doors' TcpListener::bind/UdpSocket::bind behind builder .bind()) — the doors' binds are the doors' own (assembly-layer, ADR-009).

2. Seam-contract compliance (ADR-004) — clean. Readiness: the local egress parks on sock.readable().await then try_recv_from (WouldBlock → the spurious Ok(None), never hot); the composed egress parks on the shared queue's recv().await, and its closed-queue condition is a count-based design (live_relays + ever_opened) — a real improvement over the POC (the POC held its own inbound_tx clone, so its queue-None arm was unreachable; the landed egress actually detects every-relay-ended and returns Err, pinned by closed_queue_yields_err_not_a_hot_none). Ownership: rides the handle structurally everywhere — the local providers box their socket halves into the payloads; the composed accept wraps the Send + Unpin-only alktunnels halves in SyncHalves/OwnedWrite (Mutex<T: Send> is Sync, no unsafe) with the TunnelListener riding INSIDE the write payload (ForwardedOwner — the disjoint-capture trap is doc-pinned and the whole-struct rebinding is in place); the egress keeps the session in the table entry (the POC's insert-time-bug lesson, doc-pinned). No _ bindings of owning values (swept). The accept future owns its resources end-to-end in both variants (the listener lives inside the future; the composed future owns the listener + the oneshot receiver — the client-gone select! drop tears both down). Framing-once: the local egress returns addr + data inner bytes; the composed egress returns inner bytes with its own oversize guard mirroring the wire bound; the tunnel-side framing is the tunnel codec's (frame_datagram), never the RFC frame's.

3. Door fidelity (ADR-005/ADR-003) — clean, one finding corrected (R-1). The NoAuth door is a literal raw pipe (copy_bidirectional, zero translation — pinned by door-tcp.rs's raw-byte probes). The cred-bearing door terminates RFC 1929 locally through the forked accept_password_auth surface, maps through the static table (the check IS the lookup — one table, one truth), and re-originates in-band via the token sessions; the never-pipes-raw-post-auth property is pinned by the re-origination probe (the vanilla client's command bytes are consumed door-side; the in-band conversation is fresh). The token is the mapped identity's id riding the open-op auth_token payload (CF-005 precedence (1)) — the password never crosses the channel (pinned); the identity never reaches the vanilla client. The RFC UDP-header codec (parse_udp_request/new_udp_header) appears only at the door's boundary (the in-band wire stays {addr, data} frames — pinned end to end through a real echo target); FRAG ≠ 0 is refused by dropping the datagram, the association stays healthy (pinned). The TCP door's ASSOCIATE arm refuses RFC-faithfully before any session opens (no phantom data plane). R-1 is the UDP door's in-band decode state — see the Findings.

4. The graduation's shape (ADR-007 variant 2) — clean. The composed accept advertises the real OS-chosen bound (the open-op reply's bound field via the reply projection; a missing bound/listen FAILS the factory — Shortage with an explicit no-stand-in message, never an expected-addr stand-in) and the real forwarded-op peer (a Path peer maps to the RFC sentinel — the documented addressless case, not a stand-in). The POC's superseded workaround (reply#1-first + carried bind_addr + sentinel peer) is not resurrected — tests/composed-accept.rs pins the real addresses end to end (reply#1 advertises the real OS-chosen bound, reply#2 carries the forwarded op's real peer), and the establishment bound covers only the open-op round trip (the listener open is the factory's; the accept wait is the teardown-governed quiet-wait phase).

5. Convention compliance — one residue corrected (R-2). After R-2, no unwrap/expect/panic!/unreachable! in non-test library code (the seven batch-2 additions were the builder-required-field expects and two internal-invariant expects — same class as batch-1's R-2; all now typed). thiserror surfaces throughout (the new builder errors included); naming/module structure hold — the feature-gated backend modules (local/, composed/, door) are never imported from the protocol modules (swept: zero crate::local/crate::door/crate::composed references in the protocol layer); producer/consumer vocabulary holds (RFC-facing "server"/"client" only inside src/rfc1928/).

6. Feature-gate hygiene (ADR-009) — green, all four targets. cargo test (default) green; cargo test --all-features green; both wasm checks clean for default AND --features composed (the composed wasm gate verified live); local on wasm fails to build with exactly the expected shape — mio cannot compile on wasm32-unknown-unknown (the alktty posture's build-error guarantee, verified). local brings only tokio/net (+ mio/ socket2 transitively); composed brings only alktunnels; the default graph is unchanged from batch 1 (spot-checked per feature).

7. POC-fidelity spot-check — clean, with the R-1 exception and one documented improvement.

• The composed egress's LRU + closed-queue posture: the lazy destination→tunnel table, the LRU eviction-before-insert loop, the touch-on-access ordering, the session-inside-the-entry placement, and the relay task's shape are all line-faithful to POC #2's ComposedEgress (/workspace/alksocks-udp-poc/src/egress.rs). The one deliberate divergence is the closed-queue detection — the POC's recv().await-None arm was structurally unreachable (it held its own Sender); the crate replaces it with the live_relays/ever_opened count so the association actually ends when every tunnel ends (the POC's stated intent, realized; drift-in-the-right-direction, doc-pinned in the struct docs).
• The cred-bearing door's re-origination: matches POC #1's terminate-locally + re-speak shape (the POC's raw-pipe door is the NoAuth variant; the cred-bearing extension is the ADR-005 design, validated by the integration suite's vanilla-client round trips).
• The bind-first correlation: the token→oneshot registry shape with the forwarded handler owning the data-plane pump (R-02 held — the handler's inline pump_bidi await tracks the plane) matches the alktunnels 0.2.0 forwarded-op surface; the three-party half-close deadlock note (the bridge completes on EITHER direction's EOF) is the pinned non-obvious-correctness comment done right.

8. Docs sync — two residues fixed (R-3, R-4). The integration task's reconciliation was verified: overview.md (module map, feature gates, dependencies), consumer.md (the front doors, the TokenClient door-only contract, the BIND take-guard), producer.md §Wiring, and README.md Current State all match the tree. Residues fixed: producer.md's local-backend section described the landed shape as a channel pipe (copy_bidirectional onto a BiStream) — the landed ProducerDoor serves the RFC conversation on the accepted socket directly via serve_socks5_over (no channel in the path; rewritten, including the bind_policy gate note the docs omitted); associate_with_token's doc claimed "the TCP door uses this" — the landed TCP door refuses ASSOCIATE before any session (reworded); SessionOwner's docs claimed it is "shared with the composed accept" — the accept uses its own SyncHalves/OwnedWrite pair (reworded). No other spec drift.

Acceptance Criteria

• The never-binds posture verified (protocol layer + default graph)
• Seam-contract compliance verified for every provider
• Door fidelity verified (both variants + the UDP boundary)
• The graduation's shape verified (no stand-ins)
• Convention compliance swept
• Feature-gate hygiene verified (all four verification targets)
• POC-fidelity spot-check of the trickiest assembly behaviors
• Docs sync verified; findings recorded; the gate to release polish opens

Findings

• R-1 (major → fixed in the review change): the UDP front door's in-band decode (recv_inband, src/door.rs) created a fresh DatagramReader per call and returned only the first frame of each feed — a datagram split across channel chunks would have restarted the reassembly from scratch per read, and datagrams batched beyond the first in one chunk were silently dropped. This is exactly POC #2's first-cut bug (ADR-003: "reader state persists across calls"; AssociateSession::recv_datagram owns the pinned shape) — the door bridge did not carry it, and its own doc comment claimed persistence. Unexercised by the suite (every landed test drives one frame per chunk; the failure needs two frames coalescing into one read — a burst shape real echo targets produce under load). Fix: an owning InbandBridge (persistent DatagramReader + batched-datagram pending queue, the AssociateSession shape) threaded through the channel→socket pump, plus the fail-loud mid-frame EOF (never a silent partial datagram). Pinned by three unit tests on the bridge (batched frames queue-not-drop; partial-frame state persists across calls — the test fails against a per-call reader; EOF mid-frame errors loud) and an end-to-end door-udp test with a doubled-echo egress (batched_in_band_replies_both_reach_the_vanilla_client).
• R-2 (convention, fixed in the review change): seven non-test expects landed with batch 2 (convention 2's rule is the rule; batch-1's R-2 precedent applies): the FrontDoor/UdpFrontDoor builders' missing-.bind panics → DoorBuildError (MissingBindAddr, .build() is now Result); the ProducerDoor builder's missing-.producer panic and serve's missing-.bind panic → ProducerDoorBuildError/ProducerDoorError (the bind decision is serve's typed error, the io error is #[source]-wrapped); ComposedEgress::entry_for's inserted-above invariant → an EgressError::General structural diagnostic; register_forwarded_serving's registration panic → Result<CorrelationRegistry, String> (the underlying alkcall diagnostic surfaced, # Panics → # Errors); TokenClient::wrap's call-connection-taken panic → TokenClientError::CallConnectionTaken (a client is one calling surface — the door-only contract). Test call sites updated (.expect("the door builds") etc. — test-side, allowed); the new error types are re-exported (lib.rs).
• R-3 (doc residue, fixed): producer.md's producer-side local-backend section described the landed shape as a raw channel pipe (copy_bidirectional onto a BiStream, "the consumer door's code shape") — the landed ProducerDoor hands each accepted socket to serve_socks5_over (the RFC conversation runs on the socket; no channel pipe). Reconciled, including the bind_policy-is-this-path's-BIND-gate note and the cred-bearing follow-up posture (not landed).
• R-4 (doc residue, fixed): associate_with_token's doc claimed the TCP door calls it (it refuses ASSOCIATE before any session); SessionOwner's module + type docs claimed the composed accept shares it (the accept uses SyncHalves/OwnedWrite). Both reworded to the landed reality.
• R-5 (accepted, no fix demanded): the end-to-end batched_in_band_replies... test does not deterministically exercise the batching (the two echo frames may ride separate channel chunks — timing-dependent); the deterministic pins are the bridge unit tests (which demonstrably fail against the per-call reader). The end-to-end test still guards the regression class through the real stack; the unit tests carry the determinism.

References

• docs/architecture/overview.md, wire.md, producer.md, consumer.md (the specs under review)
• docs/architecture/decisions/003/004/005/006/007/008/009/010/012/014
• POC crates: /workspace/alksocks-{connect,udp,bind}-poc (the fidelity sources)
• AGENTS.md (conventions 1–17; the verification commands)
• The batch-1 review precedent: tasks/implementation/review-protocol-core.md