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, with 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, per 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, so drift is a finding (except where an ADR supersedes the POC's shape, of which 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 with 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()), and 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), which is 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, with 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, i.e. 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), and 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 with Shortage and 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, the 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 on 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, as 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), but 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", but the landed TCP door refuses ASSOCIATE before any session (reworded); SessionOwner's docs claimed it is "shared with the composed accept", yet 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, so 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), yet 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, so 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., which is test-side and 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") since 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, i.e. 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