alksocks/docs/reviews/002-protocol-core-implementation-review.md

---
status: resolved
last_updated: 2026-09-25 (the fix round verified closed — the
  review-fix-round-002 gate; every finding closed or
  re-dispositioned, F-1 fixed in the gate change)
reviewed_artifacts:
  - src/lib.rs
  - src/wire.rs
  - src/egress.rs
  - src/producer.rs
  - src/consumer.rs
  - src/params.rs
  - src/error.rs
  - src/rfc1928/ (post-review-001 state, spot-checked)
  - Cargo.toml
tool: manual source read + cargo test/clippy/fmt/doc/wasm/publish-dry-run + cargo-llvm-cov + alkcall/alktty/alktunnels substrate cross-checks
reviewer: general implementation review (correctness + security + smells + coverage)
scope_note: the protocol core (batch 1) only — src/ non-test regions plus
  the seams they land; tests/ read for coverage attribution and
  duplication assessment
---

# Review 002 — The Protocol-Core Implementation (Post-Landing Full Review)

## Purpose

General review of the batch-1 protocol core after landing — the
thirteen task group (`tasks/implementation/core-*.md`, commits
`ce8148d..16b21e3`). The narrow review
(`tasks/implementation/review-protocol-core.md`) checked spec/task
fidelity; this one asks the four questions that review did not:

1. **Correctness** — hazards in the landed producer/consumer behavior
   itself (not spec conformance): re-entry paths, teardown races,
   protocol-state corruption, error-shape correctness.
2. **Security** — resource exhaustion, gate fidelity, panic
   surfaces, log/identity handling on the paths that now carry
   real peer traffic.
3. **Code smells** — convention compliance plus the structural
   smells a 13-task implementation round leaves behind (dead
   surface, duplication, doc drift).
4. **Coverage** (`cargo-llvm-cov`) — important gaps, attributed to
   specific behaviors and their risk.

Method note: the review is line-level over `src/` (all seven
modules), with substrate cross-checks against alkcall 0.8.0
(`pump_bidi`, `teardown_channel`, `register_openable_with_establisher`)
and the sibling crates (alktty's `ErrorDefinition` posture,
alktunnels' registration shape).

## Verification Baseline

All commands on the reviewed tree (commit `16b21e3`, clean):

- `cargo test`: **348 green** (121 lib + 38 associate + 39 bind +
  40 connect + 40 integration + 30 harness + 37 differential + 3
  doctests, 2 ignored).
- `cargo test --all-features`: **green** (same counts).
- `cargo clippy --all-targets -- -D warnings`: **clean**.
- `cargo fmt --check`: **clean**.
- `cargo doc --no-deps`: **clean**.
- `cargo check` + `clippy -D warnings` (wasm32): **clean**.
- `cargo publish --dry-run --allow-dirty`: **completes**.
- `cargo llvm-cov`: **94.52% regions / 94.67% functions / 95.55%
  lines** (4427 covered / 197 missed).

### Coverage breakdown

| Module | Lines | Functions | Regions |
|---|---:|---:|---:|
| consumer.rs | 91.90% | 93.33% | 90.72% |
| egress.rs | 94.89% | 92.86% | 93.31% |
| error.rs | 96.15% | 90.00% | 92.52% |
| params.rs | 100.00% | 100.00% | 100.00% |
| producer.rs | 95.95% | 94.15% | 96.20% |
| rfc1928/client.rs | 96.53% | 100.00% | 94.46% |
| rfc1928/mod.rs | 97.30% | 95.65% | 93.36% |
| rfc1928/server.rs | 93.73% | 90.00% | 91.75% |
| rfc1928/util/stream.rs | 100.00% | 100.00% | 78.57% |
| rfc1928/util/target_addr.rs | 98.48% | 100.00% | 93.71% |
| wire.rs | 96.32% | 97.06% | 97.07% |
| **TOTAL** | **95.55%** | **94.67%** | **94.52%** |

Healthy for a protocol core. The missed lines decompose: ~60% are
the fork's accepted-uncovered blocks (the review-001 posture —
`StandardAuthentication`, `CheckResult`'s non-`bool` impls,
`skip_auth_this_is_not_rfc_compliant`) and test-side asserts; the
load-bearing gaps are attributed to the findings below (D-1, D-2,
N-8).

## Summary Statistics

| Severity | Count | IDs |
|----------|------:|-----|
| Critical | 0 | — |
| Major | 2 | C-1, C-2 |
| Minor | 6 | C-3, S-1, D-1, D-2, D-3, N-1 |
| Observation | 9 | S-2, N-2..N-9 |

No critical findings. The two majors: C-1 is a consumer-side
correctness hazard reachable through public API without any guard
(the BIND session's reply#2 reader re-enters on a live data plane);
C-2 is a production posture question (the pre-command handshake and
the association have no timeout surface — bounded memory, unbounded
time) that needs a decision before batch 2's doors route vanilla
clients through the same phases. One dead-surface finding (N-1)
closes out review 001's "cut if unconsumed" triggers, which have
now fired; one security note (S-1) records the datagram stage's
residual hot-loop posture; the rest are small.

Prefixes, aligned with review 001's legend extended to the whole
crate: `C` (correctness), `S` (security posture), `D` (dead
surface/API), `N` (observations/notes/smells).

---

# Part A — Findings

## C-1 [major] — `BindSession::await_inbound` (and `BindReply1::await_inbound`) are re-entrant with no spent-marker: a second call re-reads the *data plane* as reply#2, silently corrupting the pipe

**Files**: `src/consumer.rs:757-763` (`BindSession::await_inbound`),
`src/consumer.rs:561-567` (`BindReply1::await_inbound`), contrast:
`src/consumer.rs:176-184` (`Socks5ChannelSession::into_inner`'s
`SpentSessionError` shape)

**Problem**: `BindSession` is the crate's one two-phase session: the
constructor reads reply#1, and reply#2 is read later, on demand, by
`await_inbound`. The method takes `&mut self` and leaves `inner`
(`Option<Socks5Stream<BiStream>>`) populated after success — the
returned `BindDataPlane<'_>` *borrows* the same stream. Nothing
marks the reply#2 phase as consumed. A caller that invokes
`await_inbound` twice (a plausible misuse: an app-server wrapper
retry, a front-door bridge that polls, a future refactor) gets this:

1. The producer side has moved to the pass-through pump
   (`pump_bidi` on the producer's quiet-wait exit) — post-reply#2
   bytes are the accepted peer's raw application data.
2. The second `await_inbound` runs `read_reply2` against those
   bytes: reads 4 bytes as `VER/REP/RSV/ATYP` and an address after
   them, from the middle of the peer's data stream.
3. If the peer's first bytes do not start with `0x05`, the caller
   gets a baffling `UnsupportedSocksVersion` (the pipe is already
   desynchronized — the 4+ bytes were consumed either way). If they
   *do* start with `0x05` (SOCKS-in-SOCKS, any binary protocol
   whose first byte is 5), the read silently eats 10+ bytes of the
   peer's data, fabricates a `BindDataPlane`, and returns `Ok` —
   the corruption is invisible.

The same shape exists through the other door: `session.reply1()`
returns a fresh `BindReply1<'_>` handle each call (consumer.rs:745),
and two sequential `reply1()` → `await_inbound()` calls replay the
same double-read. The sibling sessions are all *safe* here — that is
what makes this one inconsistent rather than a design choice:

- `Socks5ChannelSession`: the full conversation is consumed by the
  constructor; `into_inner` is the only taking method and carries
  the `SpentSessionError` guard (the gate review's R-2 shape —
  `tasks/implementation/review-protocol-core.md`).
- `AssociateSession`: `send_datagram`/`recv_datagram` are a proper
  state machine (reader state, spent-stream `Io`/`None` shapes);
  no re-entry ambiguity.

The fork's client typestate has the same latent shape
(`Socks5Stream::request` is callable repeatedly and would re-send a
command header onto the data plane), but it is pre-data-plane
machinery owned by the constructors — `BindSession` is where the
phase boundary crosses into caller reach.

**Fix (small)**: make the reply#2 phase consumed, in the session's
established posture:

- `BindSession::await_inbound` reads `inner.take()` — a second call
  hits the `None` arm and returns the existing `Ok(None)` ("spent"
  — consumer.md's posture for post-teardown reads). This is the
  smallest change and matches the session's own `None`-arm doc
  ("`None` after a taken plane").
- `BindReply1::await_inbound` consumes the handle already; the
  re-entry path is `reply1()` twice — either mark the session
  consumed at `reply1()` (it is a borrow, so the flag rides the
  session), or document that `reply1()` is a view and the session's
  `await_inbound` is the normative reader (and demote
  `BindReply1::await_inbound` to a private helper).

One test each: double `await_inbound` returns `Ok(None)` (or the
typed spent error, if the `SpentSessionError` posture is preferred
for consistency), and the data-plane bytes survive untouched.

**Lift**: closes the one reachable silent-corruption path in the
consumer surface. Everything else in the session types is
spent-state-guarded; this is the gap.

---

## C-2 [major] — The pre-command RFC handshake and the association have no timeout surface: a silent channel holds a spawned task and reader state indefinitely (bounded memory, unbounded time) — fine per contract, but the production posture needs a decision

**Files**: `src/producer.rs:183-191` (the pump task parks on
`accept_bi` → `accept_no_auth` → `read_command` with no timeout),
`src/producer.rs:350-420` (the datagram stage's `loop` — exits only
on EOF/error/data), contrast: the establishment bound
(`register_socks5_openable`, `timeout: None` → alkcall's 10s
default, covers only the open-op round trip)

**Problem**: the alkcall 10s establishment bound governs the
*open-op call*, not the in-stream conversation. After the channel
opens, the pump handler spawns its task and parks on the RFC
handshake; a consumer that opens a channel and never speaks holds —
per idle channel — one spawned task (parked on `read_exact`) and
one channel entry (bounded buffers, ADR-040). The same holds for an
established association (lifetime = channel lifetime is the
contract, wire.md §UDP ASSOCIATE) and a parked BIND quiet wait
(teardown-governed is the *decision* there — ADR-007, "FTP-length
waits stay legal").

The mitigations are real: the 256-channel per-connection cap
(ADR-041) bounds the count, memory per idle channel is bounded, and
the teardown paths all fire on channel close (the harness pins
client-gone teardowns). Nothing leaks; nothing grows. But the
aggregate is: any identity with `socks5:open` can hold 256 parked
tasks + 256 channel entries indefinitely *without sending a byte* —
and the RFC 1928 conversation itself, unlike BIND's quiet wait, was
never given a timeout *decision* (the quiet wait got one; the
handshake and the association lifetime got "inherited from the
channel").

**Why file it now**: batch 2's providers (`local`, `composed`) are
about to route *vanilla clients* through these same phases via the
front doors, where the peer is no longer a wrapper-aware consumer
but a kernel socket (often outside alkcall's channel-cap regime
entirely — a door's accepted TCP connection is not a channel).
Slowloris-shaped holds (open the door, send the greeting byte,
stop) become the assembly layer's problem, and the protocol layer
is where a timeout seam would live.

**Fix (decision, not code)**: an ADR-level note deciding one of:

1. **Idle timeout is the substrate's/assembly's job** — the
   protocol layer never times out (consistent with "no in-band
   control vocabulary" and association-lifetime-equals-channel);
   record the DoS posture explicitly (the cap bounds it; doors
   apply their own accept timeouts). Cheapest, defensible.
2. **A producer-side idle bound** — an optional per-session
   inactivity timeout at the pump level (config on
   `Socks5ProducerBuilder`), enforced by racing the handshake/stage
   reads with a sleep timer; a timeout = clean EOF-shaped teardown,
   no reply (the RFC has no timeout reply code; 0x01
   `GeneralFailure` on a pre-reply timeout is the closest and is
   what upstream's connect-timeout path surfaces).

Option 2 is more work and touches no wire format; option 1 needs
only the posture written down (it is currently *implicit* — the
review-002 gate checked teardown, not holds). Either way the
decision should land before the `local` doors exist, not after.

---

## C-3 [minor] — `Socks5Producer::serve_socks5_over` derives `can_bind` from `accept.is_some()`: factory-wired ⇒ BIND allowed, with no way to say "factory wired but gate closed" on the substrate-free entry

**Files**: `src/producer.rs:630-648` (`serve_socks5_over`'s
`self.accept.is_some()`), doc: "The command gate is resolved from
the builder's `accept` presence"

**Problem**: the pump handler (the channels path) resolves the gate
from the per-call opener identity (ADR-006 — correct). The
substrate-free entry derives it from seam *presence*: wiring an
accept factory flips the gate open for every caller of
`serve_socks5_over`, and there is no way to express "this producer
has a BIND factory but the local policy denies BIND" without
dropping the factory (losing BIND entirely). For the current
in-tree uses (tests, the harness) the conflation is harmless; for
the batch-2 `local` backend — the documented consumer of this entry
(producer.md: "the batch-2 local backend rides the same
registration") — it means the local door's BIND authorization has
no seam to plug into except factory presence.

This is a real-but-early API-shape hazard: it is exactly the shape
that invites a batch-2 shortcut where "listener configured" becomes
"listener permitted" (ADR-006's whole point was to split those).

**Fix (small, pre-batch-2)**: widen the substrate-free entry's gate
to an explicit policy input — e.g.
`Socks5Producer::serve_socks5_over_with_gate(stream, can_bind)` or
a builder field `.bind_policy(bool)` defaulting to
`accept.is_some()` for backward-compat with the landed tests. The
pump handler's identity-derived gate is unaffected. One test: a
producer with an accept factory + explicit `can_bind=false` refuses
0x02.

---

## S-1 [minor] — No idle/livelock bound on the datagram stage's `Ok(None)` tolerance is correct, but the stage's *reply* branch can spin hot against a chatty-but-empty egress — bounded by `yield_now`, unbounded by rate

**Files**: `src/producer.rs:383` (`Ok(None) => yield_now`),
`src/producer.rs:356` (the 16 KB read chunk)

**Problem**: the R-1 hardening makes the stage *livelock-safe*
against a zero-yield hot-`Ok(None)` egress, which is the right
structural guard. The residual posture: a *misbehaving* egress that
returns `Ok(None)` at full speed still costs one `yield_now` +
re-poll per cycle — the stage becomes a busy-wait consumer of CPU
while making no progress, indefinitely, on a current-thread
scheduler it now also *yields to* (so the runtime survives, but the
stage never parks). A compliant egress parks (the contract); a
buggy one degrades to a hot loop that yields. The contract
(`egress.rs:174-185`) says "implement the wait as a real async
park" — the stage defends itself, but nothing bounds the defense.

Also in this family: the 16 KB read chunk is fixed regardless of
the channel's chunking, and `DatagramReader`'s `BytesMut` retains
peak capacity (a 65535-byte frame leaves ~64 KiB of capacity
resident for the association's lifetime — per association, bounded,
but unshrunk).

**Recommendation**: accepted-uncovered posture note rather than a
fix — the stage's contract violation handling is defensive
correctness (a *compliant* egress never hot-`None`s), and a
backoff/budget in the stage would penalize the compliant path. One
line on `run_datagram_stage`'s doc naming the residual
(hot-`Ok(None)` egress = a yielding busy-wait, bounded by the
egress's own correctness), or a cheap consecutive-`Ok(None)`
budget that re-parks the select for a tick after N violations.
Low priority; filed so the residual is a named decision, not an
accident.

## S-2 [observation] — `teardown_channel`'s returned `JoinHandle` is dropped by every consumer-side `reap()`: teardown is EOF-propagated and detached, which is correct here, but the contract should say so

**Files**: `src/consumer.rs:197-202` (CONNECT `reap`),
`src/consumer.rs:490-495` (ASSOCIATE `reap`),
`src/consumer.rs:787-792` (BIND `reap`);
substrate: alkcall `manager.rs:521-536` (`teardown_channel` returns
`Option<JoinHandle>` for the caller to drain-then-abort)

**Problem**: alkcall's contract for `teardown_channel`'s returned
handle is "the caller can abort it after the drain completes"
(the channels-call path's shape). The consumer sessions discard it
(`.is_ok()` on the result), leaving the producer-side pump task
detached to finish via EOF propagation (the dropped demux sender
EOFs the producer's read half → `pump_bidi` completes). This is the
*graceful* teardown — aborting would cut mid-flight data — and it
is correct. But it silently depends on EOF propagation completing
without the producer-side task being reaped by anyone: on a
`clear_all` (transport EOF) alkcall aborts the tasks; on a
single-channel teardown the task runs to its natural end, holding
the accepted/dialed substrate resources until EOF lands. If a
provider's dial seam holds a resource whose drop is
expensive-but-async (a composed variant's channel session), the
teardown latency is unbounded by anything visible in this crate.

**Recommendation**: one doc line on the consumer sessions' `reap`
("the returned pump task is left to finish via EOF propagation —
the graceful shape; `clear_all` aborts instead, the substrate's
abrupt path") so the next reader does not "fix" the dropped handle
into an abort. No code change.

---

## D-1 [minor] — `BindReply1::await_inbound` is dead surface: spec'd, documented, and never exercised (0% covered); the session's `await_inbound` is the only reader any test drives

**Files**: `src/consumer.rs:561-567` (`BindReply1::await_inbound` —
uncovered), `src/consumer.rs:569-573` (`raw_mut` — covered), test
usage: `tests/consumer-bind.rs:97,442` (struct destructured for
`bind_addr` only), task spec: `tasks/implementation/
core-consumer-bind.md:74-75` (the handle named as the reply#1
surface)

**Problem**: every BIND test goes through
`BindSession::await_inbound` (consumer.rs:757). `BindReply1`'s own
reader is dead — the four `reply1()` call sites destructure
`bind_addr` or use `raw_mut`, never `await_inbound`. This is the
same shape review 001's C-2 flagged in the fork
(`StandardAuthentication` — spec'd machinery with zero callers),
and its stated trigger applies here too: "if it lands without
consuming it, cut it then." The consumer layer has landed; nothing
consumed it. Unlike C-1's double-read hazard (which lives in the
*same method body*), this is about surface hygiene: two ways to do
the same read, one untested, both public, in a crate whose session
types otherwise enforce single-phase discipline structurally.

**Fix**: pick one reader. Either (a) make `BindReply1::await_inbound`
the normative path (the session's method delegates to it) and test
it through the handle — the task spec's intent, and it gives the
reply#1-phase handle a real reader; or (b) cut
`BindReply1::await_inbound` + `BindReply1`'s status as more than a
data carrier (keep `bind_addr` + `raw_mut`, which *are* used).
C-1's fix interacts with this choice.

---

## D-2 [minor] — `Socks5Producer::register` and the builder's `egress`/`accept` setters are unexercised public surface (0% covered)

**Files**: `src/producer.rs:653-669` (`Socks5Producer::register`),
`src/producer.rs:680-697` (the builder's `egress`/`accept`
setters — only `.dial` rides `builder_shape_round_trips`), harness:
`tests/harness.rs:668` (registration goes through the free function
`register_socks5_openable`, not the builder)

**Problem**: `producer.md` §Wiring presents
`producer.register(&core, &registry, auth, None)?` as *the* assembly
shape — the spec'd entry point — and the landed doc (producer.md
`Wiring`, reconciled by the gate review's R-4) names exactly this
signature. No test exercises it: the harness re-implements the
registration via `register_socks5_openable` directly (same
delegation, different call path), and the builder test drives only
`serve_socks5_over`. So the one assembly shape the spec documents
is the one assembly path with no test — and the builder's
`egress`/`accept` setters are compile-checked only. A signature
drift or a delegation bug in `register` would pass the suite.

**Fix (small)**: one integration test wiring a full
`Socks5Producer::builder().dial(...).egress(...).accept(...)`
through `.register(...)` and serving all three commands over the
harness topology. Closes the gap and exercises the spec'd shape
end to end (it is also the natural home for C-3's gate test).

---

## D-3 [minor] — `error_schemas` is declared `vec![]` while `params.rs`'s doc claims the spec follows ADR-049 §4 + ADR-016's typed error contract — alktty (the first protocol crate) declares the `channel:open_failed` `ErrorDefinition`, alksocks does not

**Files**: `src/params.rs:94-96` (the doc claim),
`src/params.rs:117` (`vec![]` — the actual spec), precedent:
`/workspace/@alkdev/alktty/src/channels.rs:198-208` (alktty declares
the `ErrorDefinition` with the `reason`/`message` schema and pins it
by test), sibling: alktunnels also passes `vec![]`

**Problem**: ADR-049 §4 says ALPN crates' open-op specs gain
matching `ErrorDefinition` entries per ADR-016 so `services/schema`
discloses the failure contract; alktty did exactly that (the
`channel:open_failed` definition + a schema + a test asserting the
declaration). alksocks's `socks5_open_spec` passes an empty vec —
but its module doc says "error_schemas follow alkcall ADR-049 §4 +
ADR-016 (the typed `channel:open_failed` contract with the reason
vocabulary, plus the registry's own `FORBIDDEN` class)." The doc
describes a contract the spec does not carry. The typed vocabulary
works end to end *behaviorally* (`SocksOpenError::reason` parses
`details.reason` off the wire — tested), but a discovery consumer
reading `error_schemas` from the registry sees no declared failure
contract for `channels/socks5/sub`.

**Fix (small)**: either declare the `ErrorDefinition` (port
alktty's — the schema for `{ reason, message }` is already written
there; one test mirrors alktty's) or reword the doc to the honest
posture ("empty — the typed contract is behavioral via
`SocksOpenError::reason`; the `ErrorDefinition` pass is
crate-family-wide, matching alktunnels, pending"). The declare-side
is the better fix and is one ADR-cited block; the reword is the
honest minimum. Doc-vs-code drift either way gets fixed.

---

## N-1 [minor] — The review-001 keep-or-cut triggers have fired: `ConnectError`, `SocksServerError::Bug`, `StandardAuthentication`, `CheckResult`'s non-`bool` impls, `skip_auth_this_is_not_rfc_compliant`, `transfer`/`wait_on_tcp` — the producer layer landed without consuming any of them

**Files**: `src/rfc1928/util/stream.rs:56-83` (`ConnectError` —
constructed nowhere), `src/rfc1928/server.rs:72-73` (`Bug` — now
consumed by the producer's in-stream refusal sites; see below),
`src/rfc1928/server.rs:448-482`
(`StandardAuthentication` + `allow_no_auth`), `src/rfc1928/
server.rs:162-172` (`CheckResult` `Option`/`Result` impls),
`src/rfc1928/server.rs:185-187` (`skip_auth_this_is_not_rfc_
compliant`), `src/rfc1928/server.rs:661-667` (`transfer`) +
`647-657` (`wait_on_tcp` — differential-suite-consumed only)

**Problem**: review 001's resolution batch 3 kept four surfaces with
an explicit trigger: "if the producer layer lands without consuming
it, cut it then." The producer layer has landed (`make_socks5_pump_
handler` drives `negotiate_auth` with `&[NoAuthentication]` only;
the dial/egress/accept seams carry their own error vocabularies;
`pump_bidi` replaced `transfer`). None of the anticipated consumers
materialized. Two corrections to the trigger's assumptions, in the
fork's favor: `SocksServerError::Bug` now *does* have in-tree
constructors (the producer's four `Bug(...)` refusal sites plus the
gate site return `Bug("... refused in-stream")` — review 001's
dead-code posture partially resolved by use), and `transfer`/`wait_on_tcp`
are consumed by the differential suite (their semantics stay
pinned). But `ConnectError` (constructed nowhere), the
`StandardAuthentication` machinery (zero callers), `CheckResult`'s
`Option`/`Result` impls (every checker returns `bool`), and
`skip_auth_this_is_not_rfc_compliant` (no caller; upstream's own
legacy-layer companion) are still consumerless — now with the
trigger condition met.

This is the crate's own stated lean ("no speculative surface,"
ADR-013's sweep posture) versus the fork's hand-port-map posture
(ADR-013 also says keep the companions). The distinction that
matters: the hand-port map is for *code a future port would reuse*
(`ConnectError`'s reply-code mapping), while the rest is machinery
that the *producer already replaced* (`StandardAuthentication` —
the pump handler hard-codes `&[NoAuthentication]`; if username/
password ever arrives it arrives at the *door* per ADR-005, not in
the typestate).

**Fix (decision)**: one posture pass, small:
- **Cut**: `StandardAuthentication` + `allow_no_auth` +
  `auth_method_enums!`'s generated enum (the macro's doc says
  "generate a similar enum" — the macro itself can stay or go with
  it; nothing else uses it), `skip_auth_this_is_not_rfc_compliant`
  (the consumer typestate's `skip_auth` config covers the
  in-tree need), `CheckResult`'s `Option`/`Result` impls (keep
  `bool`; re-add with a typed-rejection design if a door ever wants
  it).
- **Keep with note**: `ConnectError` (the hand-port map's
  companion, already documented), `transfer`/`wait_on_tcp`
  (differential-suite-consumed; upstream-faithful helpers),
  `Bug` (now consumed by the producer).
The coverage blocks resolve automatically.

---

## N-2 [observation] — `run_datagram_stage`'s inline `egress.send` per frame is head-of-line blocking on the read side — documented as sufficient, and it is, but the bound deserves one line in wire.md

**Files**: `src/producer.rs:403-408` (send awaited inline before the
next select iteration), doc: `src/producer.rs:345-349` ("sufficient
for relay postures"), spec: wire.md §UDP ASSOCIATE

**Problem**: a slow `egress.send` blocks both the next channel read
*and* the next reply drain (the select is single-task). For the
relay postures the POC measured this is fine (the doc says so), and
the alternative (spawn-per-datagram or a send queue) contradicts
the one-relay-loop shape (convention 10). The residual — one slow
destination delays *all* datagram egress on that association — is
the same behavior a kernel-socket egress has
(`send_to` backpressure), so this is likely a non-issue in
practice. Filed as a spec line, not a fix: wire.md's datagram-stage
bullet should state "per-datagram `send` is awaited inline (the
stage interleaves sends with reply drains); a slow egress applies
backpressure to the association, not to other channels" so the
batch-2 composed-egress author inherits the bound knowingly.

---

## N-3 [observation] — `DatagramReader`'s `let-else` at wire.rs:229-231 is structurally unreachable (dead defensive branch)

**Files**: `src/wire.rs:229-231`

**Problem**: inside `feed`'s loop, the `let Some((declared, acc)) =
self.current.as_mut() else { break }` else-arm can never fire: if
`current` was `None` on entry and `buf.len() >= 2`, the block above
*sets* `current` before the let-else; if `buf.len() < 2`, the loop
broke at line 223. Coverage confirms it (0 executions). It is
harmless (compiler-required exhaustiveness for the borrow, arguably)
but it is the kind of branch that invites "what makes this fire?"
misreadings.

**Fix**: restructure to `let Some(&mut (declared, ref mut acc))`
matching on the already-known-`Some` state, or drop the else by
re-ordering (`if let Some(...) = ... { ...; continue; } break;`).
Zero behavior change; one-line simplification.

---

## N-4 [observation] — The gate's in-stream refusal returns `SocksServerError::AuthenticationRejected` (and factory refusals return `Bug("...")`): the *task-log* error vocabulary overloads semantics the wire never sees

**Files**: `src/producer.rs:230-233` (BIND gate →
`AuthenticationRejected`), `src/producer.rs:262-264` (dial refusal
→ `Bug("dial refused in-stream")`), `src/producer.rs:302-304`
(egress factory → `Bug(...)`), `src/producer.rs:483-485` +
`541-543` (bind factory/accept → `Bug(...)`)

**Problem**: these `Err` returns are log/observability signals only
(the wire got its RFC-faithful reply already; the caller surfaces
nothing). But the vocabulary mislabels: an ACL refusal is not
"authentication rejected," and a seam refusing per policy is not a
"BUG" (the variant's display literally says `BUG: ...` — review 001
C-1's misleading-display note). A future operator grepping `BUG:`
in producer logs chases ghosts that are *working as designed*
refusals. The forked `SocksServerError` vocabulary has no
"refused-by-policy" variant because upstream never needed one.

**Fix (small)**: add one variant
(`SocksServerError::Refused(&'static str)`-shaped, or reuse
`UnknownCommand`-style typed arms) for the in-stream seam refusals
and the command gate, and use it at the five refusal sites (four
`Bug`, one `AuthenticationRejected`). The wire bytes
are untouched; only the log vocabulary becomes honest. Alternatively
keep `Bug` and rename its display (it *is* consumed now, so the
review-001 "misleading in a no-panic crate" note is live).

---

## N-5 [observation] — The producer's test module re-implements the harness's shape four times (structural, but collapsible)

**Files**: `src/producer.rs:954-987` (`QueueEgress`),
`1237-1274` (`HotNoneEgress`), `1321-1359` (`PureHotNoneEgress`),
`1504-1553` (`Witness` egress) — four `Egress` impls of the same
queue shape; `src/producer.rs:792-814` (`greet`/`write_request`/
`read_reply`) vs `tests/harness.rs:876-933` (same trio); echo-dial
at `src/producer.rs:719-745` vs `tests/harness.rs:131`

**Problem**: ~250 test lines are structural duplicates of the
harness's helpers. The cause is legitimate (unit tests in
`src/` cannot import `tests/harness.rs` — separate crates), and the
variants are not identical (the hot-none egress is deliberately
different per R-1's two shapes; `Witness` carries the drop flag).
But `QueueEgress`/`echo_egress_factory` and the `greet`/`reply`
trio are byte-near-identical to the harness's.

**Fix (small)**: a `tests/common/` path (or a `#[cfg(test)]`
`pub(crate)` module in `src/` compiled into both the lib test and
an integration test via `path`-included module — the standard
`mod common` pattern) shared by the producer unit tests and the
integration suite. Consolidates ~200 lines and keeps the two
harness shapes from diverging (they already have: the harness's
`ChanEgress` and producer's `QueueEgress` differ only in the mutex
wrapper). Cosmetic-to-moderate; do it when the next test round
touches either file, not as a standalone.

---

## N-6 [observation] — `consumer.rs`'s three session constructors share a near-identical prologue (open → BiStream → negotiated → teardown-on-err) — a small `open_negotiated` helper would collapse it

**Files**: `src/consumer.rs:121-149` (CONNECT),
`src/consumer.rs:290-324` (ASSOCIATE), `src/consumer.rs:702-733`
(BIND) — three copies of: `open_channel(OP_SOCKS5_OPEN,
Socks5Params::as_json(), SOCKS5_ALPN)` → `BiStream::from_joined` →
an `async` negotiation closure → `match` with
`teardown_channel(channel_id)` on `Err`

**Problem**: the three prologues differ only in the negotiation
body (the command request + what it returns). A shared
`async fn open_and_negotiate<T>(client, negotiate: impl FnOnce
(Socks5Stream<BiStream>) -> ...)` collapses ~30 lines × 3 to one
helper + three one-liners, and — more importantly — makes the
teardown-at-birth invariant (the `Err` arm reaps the adopted
channel) *one* place instead of three. A fourth constructor (any
batch-2 variant) today means a fourth hand-copied teardown path.

**Fix (small)**: one private helper; zero behavior change. Pair
with the C-1 fix's test round.

---

## N-7 [observation] — `AddrError::to_reply_error` maps *every* non-`IncorrectAddressType` failure to `ConnectionRefused` (0x05) — faithful to upstream, but semantically wrong for decode-side read failures

**Files**: `src/rfc1928/util/target_addr.rs:45-52`, consumers:
`try_notify!`'s `read_address` failures (a truncated command
address → 0x05), the consumer's `wire_error` mapping
(`src/consumer.rs:818-826` rides the same class)

**Problem**: an `AddrError::PortNumberUnreadable`/`DomainContent
Unreadable`/`IPv6Unreadable` (stream ended mid-address — an I/O
shape) surfaces as `ConnectionRefused` if it ever rides
`to_reply_error`. In practice the producer's `read_command` path
errors out before any reply on a mid-read EOF (the `?` short-circuits
to the session error, not a reply — `try_notify!` only fires for
`IncorrectAddressType`-class *typed* failures... actually it fires
for every `AddrError` — a mid-address EOF on a duplex produces a
0x05 reply). RFC-wise 0x05 is the catch-all upstream picked; a
mid-read EOF is arguably a 0x01 `GeneralFailure`. Upstream-identical
behavior (the mapping table is upstream's); the observable
difference is one reply code on a malformed/truncated peer.

**Recommendation**: none (upstream-faithful, wire-tested both ways
in the differential suite's domain-cap probes). Recorded to bound
re-litigation: if the reply-code mapping is ever revisited, this is
the one arm where "faithful" and "correct" diverge.

---

## N-8 [observation] — Coverage residue worth one line each (or accepted-uncovered with a note)

The full `--show-missing-lines` sweep attributed; these are the
residuals beyond D-1/D-2 (each is small and named here so the next
coverage run needs no re-derivation):

- `consumer.rs:356-359` — `send_datagram`'s spent-stream `Io` arm:
  untested (the spent-`recv` arm at :396-398 *is* covered). One
  test mirroring `spent_session_*` shapes.
- `consumer.rs:419` — `recv_datagram`'s mid-batch *later-frame*
  `Err` arm (first frame `Ok`, a later one garbage → the pending
  `Ok`s pushed before the error are discarded with the session):
  one test would pin the fail-loud-drops-pending semantics.
- `consumer.rs:437, 771-774` — `AssociateSession::split` and
  `BindSession::split`: never called on a *live* stream by any test
  (ASSOCIATE's one call site is the spent-session probe's `None`
  arm; BIND's is 0 outright; CONNECT's split *is*, heavily). The
  two BIND/ASSOCIATE splits are the front-door-bridge surfaces
  (consumer.md) — either one test each or a "batch-2 door
  exercises it" note; they are load-bearing for the UDP/BIND
  doors.
- `consumer.rs:453-456` — `AssociateSession::into_inner`'s `Ok`
  arm (the `TakenAssociateInner` construction): CONNECT's
  `into_inner` is tested (3 call sites); ASSOCIATE's is never
  taken in any test. One test mirroring
  `into_inner_pairs_the_pipe_with_the_teardown_owner`.
- `consumer.rs:636-642` — `BindDataPlane`'s `poll_flush`/
  `poll_shutdown` passthroughs (the review-001 L-2 class, one layer
  up): the BIND door's half-close rides these. One shutdown test.
- `consumer.rs:843` — `read_reply2`'s version-mismatch arm (a
  producer sending a non-5 version reply#2): unreachable from the
  in-tree producer (always 0x05) but reachable from a foreign one —
  one unit test on `read_reply2` directly.
- `producer.rs:374-386, 405-414` — the datagram stage's
  `ReplyFrameRejected`/`EgressEnded`/`Io` exits (an egress that
  violates the joint bound, one whose `send` fails, one whose
  writes fail): all three exits have no test anywhere (the
  harness's egress never fails post-reply). The stage's failure
  matrix is the ASSOCIATE teardown surface — at least
  `EgressEnded` (the `send`-failure arm) deserves the
  drop-witness test `egress_factory_called_once_and_dropped_with
  _the_stage` gives the happy path.
- `producer.rs:560` — the quiet wait's `Err(_)` (I/O error, not
  EOF) arm; `producer.rs:568` — the BIND reply#2 Domain-peer
  fallback (`TargetAddr::Domain(_, port) → 0.0.0.0:port`) — one
  factory returning a Domain peer pins it.
- `egress.rs:72-74, 226-230, 275-279` — the three `Debug` impls'
  bodies (render once each); `error.rs:113-115` —
  `SocksEstablishmentReason`'s `Display`. Cosmetic; batch freely.
- `rfc1928/client.rs:217-219` — `use_password_auth`'s
  misuse arm (`Some(None)`-shaped methods vec): review-001-era
  accepted-uncovered, still uncovered. Fine to leave; it has a
  translated-error note.

The headline gap is the producer stage's three failure exits and
the two untested `split`s — everything else is single-arm
cosmetics.

---

## N-9 [observation] — `is_mid_frame` is public but consumed nowhere outside the codec's own tests (the teardown diagnostic lives in `recv_datagram`'s internal check)

**Files**: `src/wire.rs:253-260` (`is_mid_frame` — pub), consumers:
`src/consumer.rs:406` (internal use only), `tests/wire.rs` unit
tests

**Problem**: the doc sells it as "the teardown diagnostic" for
callers observing a truncated stream; the actual truncation
diagnosis is inlined in `recv_datagram` (consumer.rs:402-411). A
front-door bridge (batch 2) driving its own reader loop *would* use
it. Keep — one line of "anticipated consumer: the doors" posture on
the method, or leave as-is. Zero risk either way; filed only
because D-1-style dead-surface sweeps should not flag it later
without this note.

---

# Part B — Non-findings (verified correct, recorded to bound the re-review)

- **The seam contracts hold under adversarial reading.** Re-derived
  independently: the readiness contract's R-1
  yield-guard is correct and necessary (the zero-yield unit test
  reproduces the hazard without it); ownership-rides-the-handle has
  no `_` bindings anywhere in non-test code (swept); the accept
  future's drop-as-teardown is real for both exits (drop-flag
  tests); framing-once holds (the egress sees no prefix —
  `frame_inner` is the only prefixer on the reply path and
  `frame` on the forward path).
- **The teardown matrix's real semantics are pinned** — the three
  teardown rows (`core-integration.rs:659/717/770`) drive
  half-close/EOF/client-gone mid-quiet-wait and then assert fresh
  sessions of *all three* commands succeed (the unpoisoned
  registration), with `no_leaked_channels` probes on both sides.
- **Gate composition is correct end to end.** The open gate
  (`required_scopes` at the registry) runs before the establisher;
  the command gate resolves `socks5:bind` from the per-call opener
  identity at dispatch and precedes the accept seam (no phantom
  listener — pinned). Scope-less fails closed at both gates; the
  identity matrix pins all three rows over channels.
- **No panics in non-test code** (re-swept across all of `src/`,
  not just the fork): every `unwrap`/`expect`/`panic!` hit is in a
  `#[cfg(test)]` region. The convention-2 posture held through
   three consumer sessions + the producer's in-stream refusal
   paths.
- **Wire-driven allocation is bounded everywhere in the new code**:
  the datagram stage's read chunk is fixed 16 KiB; the frame codec
  bounds at frame time (the joint bound — tested at all three
  per-form maxima + the 65507/domain corner); `read_address`'s
  domain arm stays 255-bounded (S-1's guard, pinned);
  `negotiate_auth`'s methods read is 1-byte-bounded; `new_reply` is
  a fixed 10/22-byte form. No unbounded wire-driven allocation
  exists.
- **The `select!` shapes are correct**: the datagram stage is
  un-biased (random poll order — the POC's livelock concern applied
  to the *branch's own readiness*, guarded by the R-1 yield); the
  quiet wait is single-shot with exactly two exits, and its
  one-byte read cannot loop.
- **`pump_bidi` is awaited inline everywhere** (CONNECT, BIND
  post-reply#2); the handler's `JoinHandle` is returned to alkcall
  (stored as `handler_task` in `ChannelState`), so the
  data-plane-lifetime rule (R-02) holds — verified against
  alkcall's `manager.rs` storage, not just the comments.
- **The in-band BIND variant-3 default is sound**: the instant
  accept resolves reply#1→reply#2→`pump_bidi` against
  `empty()`/`sink()` halves; EOF arrives immediately from the empty
  half (the variant-3 test pins the full sequence including EOF).
  The quiet-wait garbage test pins the other exit on the same
  shape.
- **Consumer-side EOF semantics are consistent**: `recv_datagram`
  returns `Ok(None)` on clean EOF (association end), fails loud on
  EOF-mid-frame (`UnexpectedEof`), and `Ok(None)` on a *spent*
  stream (the EOF-shape a torn-down association should present);
  F-2 (empty datagram ≠ EOF) holds at every layer.
- **`egress_factory`'s sync-lift is correct** (the result captured
  before the future — an `Err` lifts verbatim; tested both faces).
- **MSRV, wasm, publish**: `cargo publish --dry-run` completes;
  both wasm checks clean; `rust-version = "1.88"` unchanged from
  review 001's verified state (no MSRV-relevant surface added —
  the new code is plain 2021-edition async/await).
- **License/provenance posture unchanged**: `src/rfc1928/LICENSE`
  and the module docs still carry the full fork-point statement
  (convention 17's "extend, never replace" — nothing touched it).

---

# Part C — What's Good

- **The seam architecture is doing its job.** The entire producer
  test suite runs substrate-free over duplexes (the
  differential-suite pattern generalized), while the integration
  suite runs the same seams over real channels — and both pass
  against the *same* handler code. That is the convention-7
  inversion point paying off, and it is why the coverage is 94%+
  without a kernel socket anywhere in the tests.
- **The teardown discipline is real, not just documented.** Every
  session type carries the `Option`-inner + `Drop`-reap +
  `close`-graceful triple, the compile-asserted no-`Clone`, the
  spent-state guards, and the phantom-channel property is tested
  on both sides (refused opens leave no entry; refused in-stream
  targets reap before the error returns). The R-2 shape
  (`SpentSessionError`) is carried consistently — except C-1, which
  is precisely the one path it missed.
- **The error vocabularies are faithful and table-tested at every
  layer**: dial/egress/accept errors → reply codes (table-tested),
  reply codes → variants and back (both directions pinned, the
  unassigned-code arm included), `WireError` → the RFC vocabulary
  (one mapping, tested), open errors → the ADR-049 reason
  vocabulary (parsed, one-way, with the unknown-code-does-not-alias
  test). Nothing collapses into a generic error.
- **The datagram codec is tight**: the joint bound is enforced at
  frame time with the per-form maxima pinned, the incremental
  reader survives arbitrary chunk boundaries (byte-by-byte tested),
  empty payloads round-trip as real datagrams end to end, and
  garbage fails loud in both directions. F-2 is pinned at three
  layers (codec, producer stage, consumer session).
- **The test suite's *semantics* are the right ones**: the
  identity matrix, the teardown matrix with fresh-session
  post-conditions, the concurrency probes, the witness assertions,
  and the raw probes for the sentinels all test behavior over
  channels, not implementation details. The 2.2k-line producer.rs
  is 69% tests — the file-size smell is a testing artifact, not
  bloat (the real smells are the targeted ones: N-5, N-6).

---

# Suggested Disposition

Decomposed 2026-09-24 into the fix-round task group
(`tasks/implementation/fix-*.md` + the ADR + the closing review) —
the grouping follows the review's batches, adjusted so no session
carries both a code-fix vertical and the test sweep:

| Task | Items | Shape |
|---|---|---|
| `implementation/fix-bind-reply2-reentry` | C-1, D-1, N-6, S-2 | the consumer correctness fix + the same-file residues |
| `implementation/fix-producer-gate-policy` | C-3, D-2, S-1, N-2 | the gate-policy fix + the register test + the doc lines |
| `implementation/fix-rfc1928-posture` | N-1, N-4 | the keep-or-cut pass + the refusal vocabulary |
| `implementation/fix-hygiene-residue` | D-3, N-3, N-9 | the error-schema declaration + one-line postures |
| `implementation/fix-coverage-residue` | N-8 (D-2's test rides the producer task) | tests only; after the code fixes |
| `implementation/fix-test-helper-consolidation` | N-5 | after the coverage round (final population) |
| `architecture/adr-014-idle-timeout-posture` | C-2 | the ADR-sized decision; independent, before the doors |
| `implementation/review-fix-round-002` | — | the closing checkpoint; the batch-2 gate |

N-7 is recorded-only (no action — the review's own
recommendation); its bound-re-litigation note lives in the review
itself.

**Resolved 2026-09-25** (`tasks/implementation/
review-fix-round-002.md` — the closing gate): every finding closed
or re-dispositioned; the one gate-round finding (F-1, the
review-numbering collision on the R-citations — the R-findings
belong to the batch-1 gate review) fixed in the gate change. The
batch-2 gate is open (ADR-014 accepted; no major finding remains).

## References

- `docs/reviews/001-rfc1928-full-review.md` (the fork review — the
  severity prefixes, the coverage discipline, and the
  keep-or-cut triggers this review closes out)
- `tasks/implementation/review-protocol-core.md` (the fidelity
  review; its R-1/R-2 shapes are the posture C-1's fix follows)
- ADR-003/004/006/007/008/009 (the seams, gates, phase model,
  providers, features); alkcall ADR-049/050 (establishment,
  `pump_bidi`); ADR-016 (error schemas — D-3)
- alkcall 0.8.0 (`manager.rs` `teardown_channel`,
  `pump.rs` `pump_bidi`, `spec.rs` `ErrorDefinition`);
  alktty `src/channels.rs:198` (the declared-`ErrorDefinition`
  precedent); alktunnels `src/params.rs` (the empty-vec sibling)
- The POC findings (the stage's measured shapes — C-2's
  "sufficient for relay postures" lineage)