From d612d7f7f2d8ee097081825dae329759eed8384d Mon Sep 17 00:00:00 2001 From: "glm-5.3-flash" Date: Thu, 24 Sep 2026 10:35:33 +0000 Subject: [PATCH] =?UTF-8?q?review=20003:=20the=20protocol-core=20implement?= =?UTF-8?q?ation=20reviewed=20=E2=80=94=20correctness/security/smells/cove?= =?UTF-8?q?rage?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit General review of the landed batch-1 protocol core (docs/reviews/ 002-protocol-core-implementation-review.md), complementing the narrow fidelity review: correctness hazards, security posture, dead surface, and cargo-llvm-cov attribution, cross-checked against alkcall 0.8.0 and the sibling crates. Two majors: - C-1: BindSession::await_inbound (and BindReply1::await_inbound) is re-entrant with no spent-marker — a second call re-reads the live data plane as reply#2, silently corrupting the pipe. Fix sketched: inner.take() (the session's own None-arm posture) + tests. - C-2: no timeout surface on the pre-command RFC handshake or the association (bounded memory, unbounded time; the alkcall establishment bound covers only the open op). A decision to make before batch 2's doors route vanilla clients through the same phases; sketched as an ADR-level choice, not a core defect. Minors: C-3 (the substrate-free can_bind gate conflates factory-wired with allowed), D-1 (BindReply1::await_inbound is spec'd, 0% covered, never called — the two-reader ambiguity), D-2 (Socks5Producer::register + the builder's egress/accept setters unexercised — the spec'd assembly shape has no test), D-3 (error_schemas declared vec![] while params.rs's doc claims the ADR-049/ADR-016 contract; alktty declares it), S-1 (the stage's Ok(None) defense is livelock-safe but rate-unbounded — posture note), N-1 (review 001's keep-or-cut triggers fired: StandardAuthentication/skip_auth_this_is_not_rfc_compliant/ CheckResult impls are cut candidates; ConnectError/transfer/ wait_on_tcp stay as hand-port-map companions; Bug is now consumed). Coverage (94.52% regions / 95.55% lines): the load-bearing gaps attributed in N-8 — the two untested split surfaces, ASSOCIATE into_inner's Ok arm, the datagram stage's three failure exits (ReplyFrameRejected/EgressEnded/Io), read_reply2's version arm, plus single-arm cosmetics. Verified: cargo test 348 green; --all-features green; clippy --all-targets -D warnings clean; fmt --check clean; cargo doc --no-deps clean; both wasm checks clean; cargo publish --dry-run completes; cargo llvm-cov 94.52/94.67/95.55 (regions/functions/ lines). No code changed in this review — findings documented for the fix decomposition. --- ...002-protocol-core-implementation-review.md | 868 ++++++++++++++++++ 1 file changed, 868 insertions(+) create mode 100644 docs/reviews/002-protocol-core-implementation-review.md diff --git a/docs/reviews/002-protocol-core-implementation-review.md b/docs/reviews/002-protocol-core-implementation-review.md new file mode 100644 index 0000000..0c6b356 --- /dev/null +++ b/docs/reviews/002-protocol-core-implementation-review.md @@ -0,0 +1,868 @@ +--- +status: open +last_updated: 2026-09-24 +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>`) 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 (review 002 R-2's shape). +- `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` 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, ®istry, auth, None)?` as *the* assembly +shape — the spec'd entry point — and the landed doc (producer.md +`Wiring`, reconciled by review 002 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(client, negotiate: impl FnOnce +(Socks5Stream) -> ...)` 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 of review 002: 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 + +| Batch | Items | Shape | +|---|---|---| +| 1 — correctness | C-1, C-3 (+ the D-2 test) | small code + tests; one session | +| 2 — hygiene | D-1, D-3, N-1 (cut list), N-3, N-4 | posture + small deletions/rewordings | +| 3 — coverage | D-2, N-8 (the split/stage-exit/read_reply2 items) | tests only; folds into batch 1's session | +| fold-in | N-2 (wire.md line), N-5, N-6 (when next touching those files), N-7/N-9 (notes, no action) | opportunistic | + +C-2 is its own ADR-sized decision (the idle-timeout surface) and +should ride the batch-2 planning, not this review's fix round — it +is a *decision to make before the doors exist*, not a defect in the +core. + +## 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) \ No newline at end of file