review 001 remediation: the forked core's findings resolved

Batch 1 (F-1 + N-1 + S-2 + N-4): - reply_success/reply_error write replies with write_all (partial-write hazard closed; the err_when-chaining idiom suppresses clippy::unused_io_amount — reproduced fresh via a five-variant probe) - rfc1928.md's write_all bullet records both conversions and the real rule (replies are single-shot, all-or-nothing) + the lint blind spot - reject's doc names the 0xff vocabulary choice (RFC 1929 needs only non-0x00); Addr::Domain's deliberating comment pinned Batch 2 (coverage: L-1 + L-2 + L-3 + S-1): - client v6 request encoding pinned (22 bytes, ATYP=4, slice bounds) - Socks5Stream::shutdown() EOF/BrokenPipe propagation pinned - wait_on_tcp's io-error arm + SocksServerError::to_reply_error's full mapping table + read_command's command-phase version mismatch covered - S-1: read_address's 1-byte-field bound documented; decode-side 255/256 boundary pinned both faces Batch 3 (posture notes: C-1, C-2, N-2, N-3 — decided keep-with-note): - ConnectError: hand-port map's companion (cut if the producer layer lands without consuming it); StandardAuthentication: ADR-005 auth-split is the anticipated consumer; CheckResult same shape - tracing: the fork's declared logging seam, noted in Cargo.toml Coverage 92.63% → 93.09% regions / 95.40% → 96.07% lines; the named gaps closed, the remainder is accepted-uncovered with disposition. Review doc carries the re-verification + resolution record. Verification: cargo test (39 lib + 37 differential), clippy (all-targets + wasm32) -D warnings, fmt, doc, wasm check, MSRV 1.88, publish dry-run — all green.
2026-09-23 06:25:27 +00:00
1 parent 4bd25c093e
commit 7bd5d6a7e7
@@ -1,5 +1,5 @@
 ---
-status: open
+status: resolved
 last_updated: 2026-09-23
 reviewed_artifacts:
  - src/rfc1928/mod.rs
@@ -604,20 +604,20 @@ comment. Trivial.
 Volume is small — one trivial code fix plus doc/test riders — so
 recommended remediations are included per finding:

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