alksocks/docs/reviews/001-rfc1928-full-review.md

status, last_updated, reviewed_artifacts, tool, reviewer

status	last_updated	reviewed_artifacts	tool	reviewer
resolved	2026-09-23	src/rfc1928/mod.rs src/rfc1928/server.rs src/rfc1928/client.rs src/rfc1928/util/mod.rs src/rfc1928/util/stream.rs src/rfc1928/util/target_addr.rs src/rfc1928/LICENSE src/lib.rs tests/rfc1928-differential.rs Cargo.toml	manual source read + cargo test/clippy/fmt/doc/wasm + cargo-llvm-cov + reference-checkout probes	post-fork full review (security + code smells + coverage)

Review 001 — src/rfc1928/ Post-Fork Full Review

Purpose

First full review of the targeted fork (src/rfc1928/) as owned code. The extraction review (tasks/implementation/ review-vendor-extraction.md) was deliberately narrow — extraction fidelity vs the reference checkout — and its six criteria are now superseded in part by ADR-013 (the fork posture dissolves the verbatim-preservation frame). This review asks the three questions the extraction review did not:

1. Security: where RFC 1928 wire data meets allocation, panic, error-mapping, or trust boundaries — reviewed independently of upstream (upstream's choices are not this crate's security posture anymore).
2. Code smells: convention-compliance (AGENTS.md) plus dead/duplicated/misleading surface left over from the extraction and the fork pass.
3. Coverage (cargo-llvm-cov): important gaps, not raw numbers — attributed to specific behaviors and their risk.

The scope is src/rfc1928/ only (the forked core). The producer/consumer/adapter layers do not exist yet (the tree is the core plus the differential suite); the doors and sessions named in the architecture docs are future work. That boundary is what makes several findings below "dead code today, load-bearing when the wrapper lands" — flagged as such rather than as current bugs.

Methodology

• Full read of all five forked source files + the differential suite
  • the module docs, cross-referenced against ADR-011/013 and docs/architecture/rfc1928.md.
• Line-by-line behavior comparison against the reference checkout (/workspace/fast-socks5 v1.0.0) for every security-relevant path: length-driven allocation, reply-code mapping, error→reply-code mapping, auth state machine, the read_exact! macro's allocation expression, copy_bidirectional semantics.
• Independent verification of RFC 1928 edge behaviors (unassigned reply codes, domain caps, RSV/FRAG handling, method-preference order) against RFC text — not against upstream.
• cargo test (38 lib + 32 differential + 2 doc), cargo clippy --all-targets -- -D warnings (clean), cargo fmt --check (clean), cargo doc --no-deps (0 warnings), cargo check/clippy --target wasm32-unknown-unknown -- -D warnings (clean), cargo publish --dry-run --allow-dirty (packages cleanly, 35 files 279.5 KiB).
• MSRV: cargo +1.88 check native + wasm32 — passes (the manifest's rust-version = "1.88" is real; 1.85 was never claimed).
• cargo llvm-cov (summary + --show-missing-lines) — per-file attribution below.
• Executable probes for the clippy-suppression finding (F-1): a scratch crate (/tmp/opencode/io-probe) reproducing the exact generic/err_when shape; five variants driven to isolate the mechanism. All probe files deleted after the run.

Verification Baseline

All commands on the reviewed tree (commit f8c3444, clean):

• cargo test: 38 lib + 32 integration + 2 doc (ignored) — all pass.
• cargo clippy --all-targets -- -D warnings: clean.
• cargo clippy --target wasm32-unknown-unknown -- -D warnings: clean.
• cargo fmt --check: clean.
• cargo doc --no-deps: 0 warnings.
• cargo check --target wasm32-unknown-unknown: clean (the wasm-clean invariant holds — no net-shaped feature, tokio on the wasm-clean subset).
• cargo publish --dry-run --allow-dirty: completes (35 files, 279.5 KiB).
• cargo +1.88 check (native + wasm32): clean.
• cargo llvm-cov: 92.63% regions, 95.08% functions, 95.40% lines (1499 covered / 69 missed).

Coverage breakdown

Module	Lines	Functions	Regions
client.rs	95.44%	98.31%	93.81%
mod.rs	97.30%	95.65%	93.36%
server.rs	92.75%	89.86%	91.09%
util/stream.rs	100.00%	100.00%	78.57%
util/target_addr.rs	98.47%	100.00%	93.37%
TOTAL	95.40%	95.08%	92.63%

Overall healthy for a protocol codec. The uncovered lines (--show-missing-lines) decompose into: the client's v6 request-encoding arm (testable, untested), the AsyncWrite impl's poll_flush/poll_shutdown passthroughs, the StandardAuthentication macro arms (dead public surface today), SocksServerError::to_reply_error's non-Bug fall-through arms, accept_password_auth's CheckResult impls for Option/bool, and assorted error arms. Attribution lives with the findings below.

Summary Statistics

Severity	Count	IDs
Critical	0	—
Major	1	F-1
Minor	7	S-1, S-2, C-1, C-2, L-1, L-2, L-3
Observation	4	N-1, N-2, N-3, N-4

No critical findings. The one major is a latent correctness hazard masked by a clippy-lint gap (F-1 — the same class as the vendor review's F-1: a hazard invisible to the automated gates). The minors split: two security-posture notes that are dead-code today (S-1, S-2), four extraction leftovers that become real decisions when the wrapper lands (C-1, C-2, L-1, L-2), two small coverage gaps (L-2, L-3), and the smell findings (F-1's doc face, N-1..N-4).

Findings use the alkty/alktunnels prefixes adapted to a single-file scope: F (found-correctness/security), S (security posture), C (code smells/convention), L (coverage gaps), N (observations/notes).

---

Part A — Findings

F-1 [major] — reply_success/reply_error write replies with .write(), not .write_all(): a real partial-write hazard, invisible to clippy because the err_when chain suppresses unused_io_amount — and rfc1928.md's rationale claim is false for these sites

Files: src/rfc1928/server.rs:516 (reply_success), src/rfc1928/server.rs:529 (reply_error); docs/architecture/rfc1928.md:155-158 (the rationale claim)

Problem: both reply writers send the 10/22-byte reply with tokio::io::AsyncWriteExt::write, which may write fewer bytes than offered (a partial write). Upstream has the same calls verbatim (src/server.rs:932, src/server.rs:948) — this site rode the extraction unchanged. The client-side sibling (request_header, upstream src/client.rs:331) was correctly converted to write_all at extraction, and rfc1928.md records exactly that: "request_header writes with write_all where upstream wrote with write (a partial write of a request header would corrupt the conversation)". But the record frames it as a clippy::unused_io_amount consequence ("byte-identical on a healthy stream — clippy::unused_io_amount is deny-by-default"), and the server-side reply_* sites survived despite the same lint applying — so the claim as written is false for them, and the hazard they carry is live:

1. The hazard is real on the target substrates. A reply is written to a BiStream/duplex half in the channels world, where poll_write is a bounded-channel send and can accept a prefix under memory pressure (the channel's bounded per-channel buffer is exactly the partial-write producer on this substrate — the same class of behavior tokio documents for AsyncWrite generally: "a call to write represents an attempt to write some bytes"). A short write of a 10-byte reply desynchronizes the conversation: the client's read_request_reply parses the tail of the reply as the start of proxied data (or vice versa on a second reply — reply#2 of BIND), and the two sides' protocol streams separate. The failure is silent misalignment, not a typed error.
2. Why the gate misses it (executable probe, scratch crate, five variants): clippy::unused_io_amount is deny-by-default and fires on .write() in three control shapes (bare generic receiver, struct-field-of-generic receiver, concrete receiver — all confirmed erroring in the probe). It is suppressed by the err_when(...)? chain: ErrorContext's blanket impl is generic over T for Result<T, io::Error>, so .write(..).await .err_when(...)? type-infers T = usize without the lint's type-walk ever binding a concrete usize — the lint's expression-shape check misses it. The probe's minimal disambiguation: the same .write().await.err_when(...)? under a concrete receiver type fires when the chain resolves through io::Result<usize>, and stays silent through the generic blanket-impl path (exactly the typestate's shape); map(|_| ()) chains show the same suppression; write_all never trips the lint regardless. So the codebase's standard error-chaining idiom is a lint-suppressor for exactly this class of bug — worth knowing beyond these two sites.
3. The docs currently mis-teach. rfc1928.md's adjustment list presents the write_all conversion as a lint-driven, client-side fix and leaves the reader to conclude the reply_* sites were somehow out of the lint's reach. The truth is worse than the doc says and better than "unknown": the sites are identical hazards that the lint would flag in isolation but doesn't flag here. A review that leaves the doc claim standing certifies a false invariant.

Fix (small): .write(&reply).await → .write_all(&reply).await at both sites (identical semantics on a healthy stream, the RFC reply is a single bounded frame, and write_all is the project's established posture — the client site and every other write in the fork already use it). Update the rfc1928.md bullet to record the server-side sites as part of the same adjustment (one clause: "and reply_success/reply_error likewise — the unused_io_amount linter's blind spot under err_when-chaining is not a substitute for the conversion"). No wire change, no API change.

Lift: closes the one live correctness hazard in the forked code and makes the doc invariant true. Trivial effort (two call sites + one doc bullet); the differential suite already covers both reply forms byte-for-byte, so the fix is regression-guarded.

---

S-1 [minor] — read_address's domain arm allocates from an attacker-controlled byte with no cap before allocation — vec![0u8; len as usize] trusts a 255-max that only the encoder enforces

Files: src/rfc1928/util/target_addr.rs:222-229 (read_address)

Problem: the domain arm reads a 1-byte length, then immediately allocates vec![0u8; len as usize]. The cap is real — the wire length field is one byte, so len ≤ 255 by construction, and 255 bytes is bounded — but the code's safety rests on that implicit fact with no assertion, doc note, or test pinning it. The parallel sites in the same file and the client bound explicitly: the client's request_header checks domain.len() > u8::MAX before encoding; to_be_bytes checks DomainLenTooLong before encoding. The decode side has no corresponding visible bound — a reader (or a future refactor that widens the length field, e.g. a hypothetical FRAG-style extension or a misreading of the "1-byte length" rule) would turn this into an unbounded allocation from wire input.

Today the 1-byte field makes it safe: this is a posture finding, not a bug. But the crate's own security posture elsewhere treats wire-driven allocation as a named, guarded boundary (TargetAddr doc: "The domain name will be passed along to the proxy server"; the differential suite pins the 255 cap on the encode side). The decode side — the side where the attacker controls the bytes — deserves the same visible guard.

Recommendation: one doc line on read_address ("the 1-byte length field bounds the allocation at 255 — widening the field requires a cap here") plus a pinning test asserting the 255-byte domain decodes and a 256-byte claim is impossible by construction (the suite's command_domain_255_bytes_is_the_wire_maximum already pins 255 on the server path — the gap is only that nothing states the invariant). Alternatively add an explicit DomainLenTooLong-shaped check; zero behavior change. Low priority — filed so the invariant is written down before the producer half routes attacker traffic through this codec in production.

S-2 [minor] — The server's auth-reject path sends SOCKS5_AUTH_METHOD_NOT_ACCEPTABLE (0xff) as the password sub-protocol's status byte — faithful to upstream, but it is not the sub-protocol's defined rejection byte, and the ambiguity is undocumented

Files: src/rfc1928/server.rs:352-359 (reject), src/rfc1928/server.rs:344 (accept's success byte)

Problem: RFC 1929 (the username/password sub-protocol) defines the status byte as: 0x00 = success, any other value = failure. The fork (following upstream) writes [1, 0xff] on reject — 0xff being SOCKS5_AUTH_METHOD_NOT_ACCEPTABLE, the method-negotiation phase's constant, repurposed here. This is RFC-1929-compatible (any non-zero works) but conflates two vocabularies, and the module docs present the auth flow as RFC-facing. The observable consequence is benign today (interoperable servers/clients accept any non-zero), but: (a) a strict peer that treats the status byte as a boolean "0x00 vs anything" is fine, while one that echoes the value back for logging may mislabel it; (b) the in-tree client.rs check (is_success != SOCKS5_REPLY_SUCCEEDED) is written against 0x00 — the pairing works but only because both sides happen to use the same arbitrary constant.

Recommendation: a doc comment on reject naming the choice ("0xff is the convention upstream picked; RFC 1929 requires only non-0x00") — or switch the constant to a literal 0xff with a comment detaching it from the method-negotiation vocabulary. No wire change (the bytes are upstream-faithful and interoperate); the finding is the undocumented vocabulary conflation. Trivial.

---

C-1 [minor] — Dead error vocabulary: SocksServerError::Bug and ConnectError are constructed nowhere — extraction leftovers with no consumer in the forked region

Files: src/rfc1928/server.rs:72-73 (Bug), src/rfc1928/server.rs:66-67 (ConnectError(#[from] ...)), src/rfc1928/util/stream.rs:46-60 (ConnectError enum), src/rfc1928/util/stream.rs:62-74 (to_reply_error)

Problem: upstream constructs SocksServerError::Bug(...) three times — all in the cut kernel-net region (src/server.rs:1066, 1174, 1250: DnsResolveHelper/run_udp_proxy_custom's addr post-processing). Upstream constructs ConnectError exactly once — in the cut tcp_connect dialers (src/util/stream.rs:89-104). In the fork, both are unconstructable dead code: grep across src/ and tests/ finds zero construction sites. SocksError's ServerError(#[from] SocksServerError) transport keeps them reachable in the public API (they compile, they document, they map — but nothing can ever produce one). The to_reply_error mappings for ConnectError are tested (util/stream.rs tests) — testing a mapping for an error no code can raise.

The extraction review's F-4 accepted a string naming a cut function as a fidelity keep; the fork dissolved that rationale, and these two are the remaining dead vocabulary from the same category. rfc1928.md's not-forked inventory says the tcp_connect* dialers stayed in the reference checkout — but ConnectError rode along because the cut table's util/stream.rs row ("the read_exact!/ ready! macros + ConnectError stay") predates the fork's dead-code sweep and was never revisited.

Fix (small, behavioral-no-op): either (a) delete SocksServerError::Bug and the ConnectError enum + SocksServerError::ConnectError variant + its to_reply_error + its tests, or (b) keep them with a one-line module-doc note that they are the hand-port map's companions (the reply-code mapping is what a future kernel-net port would reuse). Option (a) is cleaner — SocksServerError::Bug's "BUG: ..." display is misleading in a crate that pins no-panics as a convention — but (b) is defensible if the kernel-net hand-port is still anticipated. Decide when the producer layer lands (it will either consume ConnectError's mappings — the dial-callback shape maps dial errors to reply codes — or not; that is the deciding consumer). Until then this is dead weight on the public error surface.

C-2 [minor] — StandardAuthentication (the macro-generated enum + allow_no_auth) has zero in-tree callers — the only forked surface nothing consumes

Files: src/rfc1928/server.rs:396-459 (auth_method_enums! expansion + StandardAuthentication + allow_no_auth), coverage: server.rs:417-427 (the macro's match arms) and 445-458 (allow_no_auth) are uncovered

Problem: upstream's StandardAuthentication is consumed by the cut Socks5Socket/Config legacy layer (src/server.rs:134-144, 762-768). In the fork, grep StandardAuthentication finds it only in its own definition and one differential-suite comment. Nothing constructs StandardAuthentication::* — the typestate's negotiate_auth is driven with &[NoAuthentication] / &[PasswordAuthentication] everywhere. Its uncovered lines are the largest contiguous uncovered block in server.rs (the macro match arms + allow_no_auth's two branches — 0% covered).

This is the ADR-013 posture's stated shape ("the #[allow(dead_code)] on AuthenticationMethod swept with actual usage") applied selectively: AuthenticationMethod got its sweep, the macro-generated enum did not — but the macro itself (auth_method_enums!) is documented as a user-facing extension point (its doc: "If you want to add your own custom methods, you can generate a similar enum"), and the producer layer's per-door auth selection (ADR-005's auth-split) plausibly wants it. So this is a judgment call, not a violation: keep the machinery (it is small, generic, and the producer's auth seams may drive it), or cut it until a consumer exists (the crate's "no speculative surface" lean, matching the client Config's minimal keep).

Recommendation: keep, but make the posture explicit — one module doc line on StandardAuthentication ("kept as the negotiation machinery's combinator — the producer layer's auth-split (ADR-005) is its anticipated consumer; currently uncalled in-tree") and note the coverage block as accepted-uncovered. If the producer layer lands without consuming it, cut it then. Zero code change now.

L-1 [minor] — The client's v6 request-encoding arm is untested (0% of request_header's V6 branch)

Files: src/rfc1928/client.rs:293-300 (uncovered), tests/rfc1928-differential.rs (no v6 request test)

Problem: coverage's single most important gap on the client side: request_header's TargetAddr::Ip(SocketAddr::V6) arm — the ATYP=4, 22-byte request form — is never exercised. The v6 reply form is tested twice (new_reply_v6_form, reply_v6_form_is_22_bytes, udp_codec_round_trip_ipv6), and read_address's v6 decode is tested (read_address_ipv6_round_trip) — but the request encoding of a v6 target has no test. A regression in the v6 arm (e.g. the packet[4..20]/packet[20..22] slice bounds, the ATYP byte, the port offset) would pass the entire suite. The v6 slice arithmetic is exactly the kind of boundary code that drifts silently.

Fix (small): one differential-suite test: mock server reads a 22-byte request for a v6 target and pins [05, 01, 00, 04, <16 octets>, <port>]. Small effort, closes the highest-value gap.

L-2 [minor] — Socks5Stream's AsyncRead/AsyncWrite passthrough impls are barely covered: only the happy poll_read/poll_write ride the tests

Files: src/rfc1928/client.rs:357-396 (the impls; 390-395 — poll_flush/poll_shutdown — uncovered; 383-388 covered only via one flush() call in io_impls_pipe_data_through_the_session)

Problem: the impls are 5-line passthroughs, so the risk is near-zero — but they are the consumer half's data plane (every pump_bidi-shaped consumer rides them), and poll_shutdown in particular is load-bearing for the EOF-sentinel convention (the channels data plane's half-close rides poll_shutdown reaching the underlying BiStream). A test that shuts down a Socks5Stream and asserts the underlying stream sees the shutdown would pin the half-close propagation the two-pump contract relies on. The DuplexStream-based harness can observe it (shutdown of one half → EOF for the peer).

Recommendation: one test: complete the handshake, call session.shutdown().await, assert the mock reads EOF. Small effort, pins the consumer half-close shape before the consumer layer's pumps depend on it.

L-3 [minor] — server.rs's uncovered block is the macro + auth-selection machinery (C-2's face) plus two error arms worth one test each

Files: src/rfc1928/server.rs:96-97 (to_reply_error's AddrError arm), 155-163 (CheckResult for Option/bool), 177-179 (skip_auth_this_is_not_rfc_compliant), 568 (read_command's version-mismatch return — covered via the differential suite's variant only at a different call shape), 632 (wait_on_tcp's io-error arm)

Problem: mostly dead-surface coverage debt (see C-2). Two arms deserve a note: wait_on_tcp's Err(err) arm (the io-error path of the UDP-associate control stream — reachable by dropping the duplex mid-read; one line of test) and to_reply_error's fall-through (GeneralFailure for the Io/FromUtf8/EOF arms — the mapping the producer's error surface will rely on when mapping session errors to reply codes; one test asserting the mapping table). Both are one-test-cheap; the rest is dead weight until C-2's decision.

---

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

• The F-1-class panic surface is clean. Post-extraction-review sweep re-verified: no unwrap/expect/panic!/unreachable!/ todo! outside #[cfg(test)] modules (grep-verified; every hit is inside a mod tests region). The three translated panics (from_u8's unassigned-code arm, use_password_auth's unreachable!, reply_error's runtime parse) are all present in their translated forms with pinning tests (reply_error_unassigned_codes_map_to_general_failure, the typed-error translation note, the compile-time V4_UNSPECIFIED_ZERO).
• Length-driven allocation is bounded everywhere else. The greeting/methods read (vec![0u8; methods_len as usize], 1-byte cap = 255), the password sub-protocol's username/password reads (1-byte caps), the domain length byte (S-1) — all bounded by a 1-byte wire field ≤ 255. No u16/u32-driven allocation exists in the forked region (the client's request_header uses a fixed MAX_ADDR_LEN + 3 stack buffer — 263 bytes, bounds-checked slices). The differential suite's command_domain_255_bytes_is _the_wire_maximum pins the boundary.
• The method-negotiation preference order is genuinely server-authoritative. The outer-loop-in-server_methods-order shape matches upstream (src/server.rs:901-902) and RFC 1928 §3's "the server selects one" reading; the differential suite pins both directions (client-offers-password-first gets no-auth when the server offers only none; accept_password_auth prefers password over none when both are offered). The 0xff refusal path is pinned.
• reply_success carries the caller's sock_addr verbatim — the typestate does not substitute its own address (upstream's 127.0.0.1 substitution lives in the cut run_tcp_proxy caller); the differential suite pins this distinction with the correct upstream citation. This matters for BIND's reply#1 (the bound address must be the listener's, supplied by the caller).
• copy_bidirectional's result is discarded intentionally — transfer maps neither direction's error to a reply because at the transfer interception point the reply has already been sent; the errors are observable only as stream termination. Upstream does the same (let _ =). The two-pump contract (AGENTS.md convention 8) is the caller's shape (pump_bidi), not transfer's; transfer remains the upstream-faithful interception-point helper. The differential suite pins the both-directions-to-completion behavior.
• The differential suite's 15 upstream file+line citations were spot-checked against the reference checkout — all accurate (server.rs:903-918 negotiation loop, server.rs:944-956 error reply, server.rs:1380 new_reply privacy, server.rs:1000-1003 request header read, server.rs:1087 the 127.0.0.1 substitution, server.rs:572-582/587-596 password reads, server.rs:620-627 reject write, client.rs:277-285 UDP-associate fallback + missing-target, client.rs:310-317/331 domain encode + write, client.rs:86-93 skip_auth branch, lib.rs:313-327/332-336/ 342 UDP codec, util/target_addr.rs:40-48 the reply-code mapping). The suite pins bytes, not upstream text — it survives upstream churn, and the citations are provenance, not assertions.
• The MSRV (rust-version = "1.88") is real: cargo +1.88 check passes native and wasm32. (alktty's N6-class finding — declared-but-unverified MSRV — does not reproduce here.)
• Dependency hygiene: the dependency set is exactly tokio (wasm-clean subset, default-features = false) / tracing / thiserror — tracing is currently referenced by no source file (see N-2) but is a declared, load-bearing posture for the fork ("Upstream log calls ... are dropped or re-pointed at the crate's tracing", rfc1928.md). default-features = false on tokio keeps the wasm graph minimal; verified by the wasm checks.
• The LICENSE notice is the extended form ADR-013 specifies: upstream MIT text + copyright preserved, fork-point statement (v1.0.0, extraction range 57b107b..5b18d4d, fork at 5b18d4d), and the modification note (post-fork changes are the project's under MIT OR Apache-2.0). The module docs carry the same provenance. Convention 17's "extend, never replace" holds.
• Wasm-clean invariant re-verified: no tokio::net anywhere in src/; the only tokio::io uses are the portable AsyncRead/ AsyncWrite/copy_bidirectional; cargo check + clippy -D warnings both clean on wasm32.
• cargo publish --dry-run completes — the manifest is publishable (no alkty-N9-class missing-README issue; there is no readme field and none needed).

---

Part C — Notes and observations

N-1 [observation] — rfc1928.md's "write_all adjustment" bullet is half-true (F-1's doc face)

The bullet (docs/architecture/rfc1928.md:153-158) records the client-side write_all conversion and attributes it to clippy::unused_io_amount being deny-by-default — which would make the server-side .write() sites impossible. The probe (F-1) resolves the contradiction: the lint fires on bare .write()? but is suppressed by the err_when/map(|_| ()) chaining idiom these sites use. The bullet should be updated to record both conversions (client request_header — landed; server reply_success/ reply_error — F-1's fix) and drop the linter attribution in favor of the real rule (replies are small, single-shot, and must be all-or-nothing). Recorded as the doc face of F-1; fixing F-1 without fixing this bullet leaves the invariant false in docs.

N-2 [observation] — tracing is a declared dependency referenced by no source file

grep -rn tracing src/ is empty; the dependency exists because the extraction adjustment list says log calls were "dropped or re-pointed at the crate's tracing" — in practice all were dropped (the typestate has no logging surface). Two postures are defensible: (a) keep the dep as the fork's declared logging seam for the producer layer's diagnostics (the try_notify! secondary-error site — upstream logged "extra error while reporting an error to the client" — is the natural first consumer); or (b) drop it from [dependencies] until the first real use. (a) is the likely outcome and costs nothing; filed so the dependency's purpose is stated rather than assumed. Cargo.toml's comment policy already carries the feature-note precedent.

N-3 [observation] — CheckResult's bool/Option impls and ErrorContext's FromUtf8Error impl are unused in-tree (the accept_password_auth machinery's loose ends)

accept_password_auth's closure contract (R: CheckResult) is designed for Result/Option/bool returns; in-tree every caller (tests only) returns bool. The Option/bool impls are uncovered (server.rs:155-163). ErrorContext's FromUtf8Error impl is used (read_username_password's two err_when("converting ...") sites) — covered. No action now; the same "keep or cut when the producer lands" decision as C-2 applies (the producer's username/password mapping may want Result-returning checkers with typed rejections).

N-4 [observation] — The Addr enum's Domain(String) variant comment (// Vec<[u8]> or Box<[u8]> or String ?) rode the extraction

src/rfc1928/util/target_addr.rs:207 carries upstream's deliberating comment. It is exactly the "non-obvious design-alternative" class AGENTS.md convention 1 neither requires nor forbids — but it reads as an unresolved question rather than a pinned decision (the String choice is upstream's and is fine: UTF-8 validation happens at parse). One-line resolution: either pin it ("String — validated UTF-8 at the codec boundary") or drop the comment. Trivial.

---

What's Good

• The wire vocabulary is sound and pinned. The reply form (10/22-byte), the zero-address error form, the method-negotiation preference order, the 255-byte domain cap, the UDP RSV/FRAG discipline, and the client's request encoding are all byte-pinned in a suite that names its RFC basis per test. The 92.63% region coverage with no uncovered wire-format path (the gaps are machinery, not codecs) is the right shape for a protocol core.
• The panic-surface discipline survived the fork. All three extraction-time panic translations are in place with pinning tests, and the whole-region sweep finds no new panic path — wire-reachable values map to typed errors everywhere (the F-1 of the extraction review stays fixed).
• The fork's two carried changes are real and guarded. new_reply is published and is the single reply-form encoder (no re-encoding anywhere); the BIND surface (TCPBind flowing through read_command, reply_success's addr parameter) is present exactly as ADR-007 describes, with the driving correctly deferred to producer-side code.
• The typestate genericity is intact — T: AsyncRead + AsyncWrite + Unpin throughout, no substrate type anywhere in the core (the wasm checks verify the structural half; the grep sweep verifies the naming half).
• The differential suite is the right artifact — bytes over text, upstream citations as provenance, six groups that map to the fork's actual risk surface, duplex-only (wasm-clean crate's native build). Its citations verified accurate against the v1.0.0 checkout.

---

Remediation

Volume is small — one trivial code fix plus doc/test riders — so recommended remediations are included per finding:

ID	Finding	Recommended fix	Effort	Risk	Status
F-1	.write() reply sites + false doc rationale	.write_all at 2 sites + rfc1928.md bullet update	trivial	none	✅ resolved
S-1	domain-length allocation invariant undocumented	doc line + pinning test	small	none	✅ resolved
S-2	0xff vocabulary conflation undocumented	doc line on reject	trivial	none	✅ resolved
C-1	Bug/ConnectError dead vocabulary	decide keep-with-note vs cut (decide at producer layer)	trivial	none	✅ resolved (keep-with-note)
C-2	StandardAuthentication uncalled	note posture now; cut if still uncalled when producer lands	trivial	none	✅ resolved (posture noted)
L-1	client v6 request form untested	one differential test	small	none	✅ resolved
L-2	poll_shutdown passthrough unpinned	one EOF-propagation test	small	none	✅ resolved
L-3	wait_on_tcp io-error arm uncovered	one test	trivial	none	✅ resolved (+ mapping-table + version-mismatch riders)
N-1	rfc1928.md bullet (F-1's face)	rides F-1	trivial	none	✅ resolved
N-2	tracing dependency purpose	note or first consumer	trivial	none	✅ resolved (posture noted)
N-3	CheckResult impls unused	decide at producer layer	none	none	✅ resolved (posture noted)
N-4	Addr::Domain comment	pin or drop	trivial	none	✅ resolved (pinned)

Suggested decomposition: (1) the F-1 batch (F-1 + N-1 + S-2 + N-4 — one commit, code + doc + no behavior change beyond the partial-write hardening); (2) the coverage batch (L-1 + L-2 + L-3 + S-1's pinning test — one commit, tests only); (3) the decide-at-producer-layer items (C-1, C-2, N-2, N-3) — record the posture, no code now.

Per the established workflow, every finding should be re-verified in the follow-up session before task decomposition (the F-1 mechanism claim in particular rests on an executable probe that should be reproduced fresh).

Remediation Resolution (2026-09-23)

All findings resolved in one session: findings re-verified fresh (source read of every named site), the F-1 mechanism probe reproduced, then the three batches applied with the review's decomposition. Batch 1 (code+doc, no behavior change beyond the partial-write hardening) and batch 2 (tests) and batch 3 (posture notes) land as one commit (the review's three batches are small and the verification battery runs once over the union; the batch boundaries are recorded below). The decide-at-producer-layer items were decided now — keep-with-note — so the posture is written down before the producer layer starts consuming this surface.

Re-verification

• F-1 confirmed at server.rs:516/529 (fresh read: both .write(&...) sites present, err_when-chained).
• The unused_io_amount suppression mechanism reproduced fresh (scratch crate /tmp/opencode/io-probe, five variants, deleted after the run): bare .write()? on a generic receiver fires the lint (deny-by-default confirmed); the same .write() through .err_when(...)?, .map(|_| ()), and a concrete-receiver err_when chain all stay silent; write_all never fires. Exactly the review's probe result.
• S-1, S-2, C-1, C-2, L-1, L-2, L-3, N-1..N-4 confirmed by inspection at the named sites (line numbers verified against the current tree; the dead-code claims grep-verified — zero construction sites for Bug/ConnectError, zero callers of StandardAuthentication/allow_no_auth, zero tracing references in src/).
• The rfc1928.md bullet (N-1) confirmed as written (the lint attribution stands; the server-side sites unrecorded).

Resolution — batch 1 (F-1 + N-1 + S-2 + N-4)

• F-1: reply_success/reply_error now .write_all the reply frame (server.rs, both sites). Wire-identical on a healthy stream; the partial-write hazard is closed; the differential suite's reply-form tests guard the fix byte-for-byte.
• N-1: the rfc1928.md adjustment bullet now records both conversions (client request_header + server reply_success/ reply_error), states the real rule (replies are small, single-shot, all-or-nothing), and documents the linter's err_when-chaining blind spot instead of the false lint-attribution claim.
• S-2: PasswordAuthenticationImpl::reject's doc names the vocabulary choice (0xff is the method-negotiation constant repurposed; RFC 1929 requires only non-0x00; the in-tree client/server pairing works because both sides write/check the same arbitrary constant).
• N-4: Addr::Domain(String)'s upstream deliberating comment replaced with the pinned decision (validated UTF-8 at the codec boundary).

Resolution — batch 2 (coverage: L-1 + L-2 + L-3 + S-1)

Four differential tests + one lib test (the review's small batch, plus one rider):

• L-1: client_v6_request_encoding_is_22_bytes_with_atyp_4 — the v6 request arm (packet[4..20]/packet[20..22] slice arithmetic) pinned byte-for-byte through the public surface.
• L-2: client_session_shutdown_propagates_to_the_backing_stream — session.shutdown() reaches the backing stream's poll_shutdown (the mock observes EOF; a post-shutdown write surfaces BrokenPipe), pinning the half-close propagation the consumer half's two-pump teardown rides.
• L-3: wait_on_tcp_wraps_io_errors_with_context — the Err(err) arm driven with a failing reader (a dropped-peer duplex read is a clean EOF, Ok(0) — the EOF arm — not the Err arm, so the test injects a failing reader); plus to_reply_error_mapping_table (server.rs in-module test) pinning the UnknownCommand/AddrError arms and the GeneralFailure fall-through across all non-mapped variants; plus (rider, the review's "worth one test" note on the version-mismatch arm) command_version_mismatch_refuses_before_address_parse covering read_command's command-phase version check.
• S-1: the read_address doc line states the 1-byte-field bound (widening the field requires a cap before the allocation), and decode_side_domain_allocation_is_bounded_by_the_1_byte_wire_field pins both faces (255 decodes through the attacker-side path; the encoder rejects 256, so the decode arm's len as usize can never ask for more).

Coverage after (llvm-cov): 93.09% regions / 95.65% functions / 96.07% lines (1528 covered / 60 missed; baseline 92.63/95.08/ 95.40 — 1499/69). The named gaps closed: client v6 request arm (293–300) covered, poll_flush/poll_shutdown passthroughs covered, wait_on_tcp's io-error arm covered, read_command's version- mismatch arm covered, to_reply_error's fall-through covered. Remaining misses are exactly the C-2/N-3 accepted-uncovered blocks (the StandardAuthentication macro machinery, allow_no_auth, CheckResult's Option/bool/bool impls, skip_auth_this_is_not_rfc_compliant) plus the client's use_password_auth misuse arm and error-test Ok(()) tails — all recorded as accepted-uncovered with their disposition below.

Resolution — batch 3 (posture notes: C-1, C-2, N-2, N-3)

All four decided now, keep-with-note, no code change (the review's option (b) for C-1; the recommendation for C-2/N-3/N-2):

• C-1: ConnectError + SocksServerError::Bug kept as the hand-port map's companions — the module doc on util/stream.rs states it (to_reply_error is what a future kernel-net port reuses; if the producer layer lands without consuming it, cut it then), and rfc1928.md's cut-table row records the posture. The deciding consumer remains the dial-callback shape (ADR-004) at producer-layer time.
• C-2: StandardAuthentication kept with the posture on its doc ("currently uncalled in-tree; the producer layer's per-door auth selection (ADR-005) is its anticipated consumer; cut if still uncalled when the producer lands"). Its coverage block is accepted-uncovered.
• N-2: tracing kept; Cargo.toml carries the feature-note- precedent comment (the fork's declared logging seam; first consumer expected at the producer layer's diagnostics — the try_notify! secondary-error site).
• N-3: CheckResult's doc records the same keep-or-cut decision (in-tree callers use bool only; the producer's username/password mapping with typed rejections is the anticipated Result consumer).

Verification (post-remediation)

• cargo test: 39 lib + 37 integration + 2 doc (ignored) — all pass (was 38/32/2; +1 lib test, +5 differential tests).
• cargo clippy --all-targets -- -D warnings: clean.
• cargo clippy --target wasm32-unknown-unknown -- -D warnings: clean.
• cargo fmt --check: clean.
• cargo doc --no-deps: 0 warnings.
• cargo check + clippy -D warnings (wasm32): clean — the wasm-clean invariant holds.
• cargo +1.88 check: clean (MSRV intact).
• cargo publish --dry-run --allow-dirty: completes.
• cargo llvm-cov: 93.09% regions / 95.65% functions / 96.07% lines (see the batch-2 section for the per-module delta).

---

References

• ADR-011 (the extraction mechanics, the cut table), ADR-013 (the targeted fork — the ownership posture this review assumes), ADR-007 (BIND), ADR-005 (auth-split), ADR-004 (the dial-callback shape — C-1's deciding consumer), ADR-009 (feature gates)
• docs/architecture/rfc1928.md (the fork spec — N-1's bullet)
• tasks/implementation/review-vendor-extraction.md (the prior, narrower review; its F-1 is re-verified clean here)
• /workspace/fast-socks5 (v1.0.0 — the behavioral reference; all citations in this review refer to it)
• The house pattern: alkty docs/reviews/001-code-review.md (the coverage/correctness/smell triad), alktunnels docs/reviews/001-implementation-review.md (the severity legend and re-validation discipline)
• RFC 1928 (§6 the reply form, §3 method negotiation, §7 the UDP associate frame), RFC 1929 (the password sub-protocol's status byte — S-2)