---
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` — 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 `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` — 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
`expect`s and two internal-invariant `expect`s — 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

- [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 — 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
  `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. — 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`