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

---
id: review-assembly-layer
name: Review (the assembly layer, the batch-2 gate)
status: completed
depends_on: [implementation/assembly-integration-suite]
scope: narrow
risk: low
impact: project
level: 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 `expect`s → 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
`expect`s and two internal-invariant `expect`s, 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

- [x] The never-binds posture verified (protocol layer + default
      graph)
- [x] Seam-contract compliance verified for every provider
- [x] Door fidelity verified (both variants + the UDP boundary)
- [x] The graduation's shape verified (no stand-ins)
- [x] Convention compliance swept
- [x] Feature-gate hygiene verified (all four verification targets)
- [x] POC-fidelity spot-check of the trickiest assembly behaviors
- [x] 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
  `expect`s 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`