review: vendor extraction — gate opens; F-1 panic arm fixed

The extraction review against vendor.md's cut table, ADR-011
§Amendment, and AGENTS.md conventions. All five file diffs vs the
fast-socks5 v1.0.0 reference checkout read hunk-by-hunk and
categorized (cut / carried changes / adjustments / provenance /
tests); wasm, clippy, fmt, doc all green; RFC 1928 spot-checks
(reply form, method order, UDP zero fallback, domain cap, FRAG
split) independently verified against the RFC text.

F-1 (critical, resolved here): ReplyError::from_u8 kept upstream's
unreachable!("ReplyError code unsupported.") arm while being
wire-reachable — Socks5Stream::read_request_reply feeds the raw
reply byte from the wire into it, and RFC 1928 §6 marks 0x09-0xFF
unassigned, so a misbehaving peer would panic the consumer task.
Unassigned codes now map to GeneralFailure (the RFC's catch-all),
pinned by a new test, and the panic-translation adjustment is
recorded in vendor.md's adjustment list (with the Config-Default
and write_all notes) in the same change.

Findings F-2..F-4 (dead commented code dropped with its region;
Cargo.toml feature-layout comments; the preserved resolve_dns error
string) reviewed and accepted. Review task file carries the full
record; the gate to the downstream task groups is open.

Verified: cargo test (38 + 32), cargo test --all-features, clippy
--all-targets and --target wasm32-unknown-unknown (-D warnings),
cargo check --target wasm32-unknown-unknown, cargo fmt --check,
cargo doc --no-deps.

docs/architecture/vendor.md

@@ -1,8 +1,11 @@
 ---
 status: draft
-last_updated: 2026-09-21 (the extraction amendment — ADR-011 §Amendment:
-extraction not wholesale copy, the net gate dissolves, two carried
-changes; Phase 1 initial draft 2026-09-20)
+last_updated: 2026-09-22 (extraction review F-1: `ReplyError::from_u8`'s
+panic arm mapped to `GeneralFailure`, panic-translation + Config/
+write_all adjustments recorded — see the adjustment list; the
+extraction amendment ADR-011 §Amendment: extraction not wholesale
+copy, the net gate dissolves, two carried changes; Phase 1 initial
+draft 2026-09-20)
 ---
 
 # alksocks — The Inline Vendor (fast-socks5 subset)
@@ -120,6 +123,24 @@ mechanical edits to the copied code, ADR-011 §Amendment):
   anyhow::Error)`) is dropped — the crate's error surface is
   `thiserror`-typed; the vendored `SocksError`/`SocksServerError`
   stay the wire-level vocabulary.
+- The panic paths in the copied region are translated (AGENTS.md
+  convention 2 — no panics in library code; the extraction
+  post-dates the client task's summary, corrected by the extraction
+  review's F-1): `ReplyError::from_u8`'s upstream `unreachable!()`
+  arm for codes outside the RFC table maps unassigned codes
+  (`0x09`–`0xFF`, RFC 1928 §6) to `GeneralFailure` instead — the
+  value comes off the wire (`Socks5Stream::read_request_reply`
+  feeds the raw reply byte there), so a misbehaving peer surfaces
+  as a typed error, never a panic. (`client.rs`'s
+  `use_password_auth` `unreachable!()` and `server.rs`'s
+  `reply_error` `unwrap` were already translated by their tasks.)
+- The client `Config` derives `Default` instead of upstream's manual
+  impl (upstream's manual impl only spelled the cut
+  `connect_timeout` field), and `request_header` writes with
+  `write_all` where upstream wrote with `write` (a partial write of
+  a request header would corrupt the conversation; byte-identical on
+  a healthy stream — `clippy::unused_io_amount` is deny-by-default).
+  Both are recorded here so a future re-vendor re-applies them.
 
 ## Maintenance Posture
 

src/vendor/mod.rs

@@ -224,6 +224,13 @@ impl ReplyError {
         }
     }
 
+    /// Map a wire reply code to its variant. Codes outside the RFC
+    /// table (`0x09`–`0xFF`, unassigned per RFC 1928 §6) map to
+    /// `GeneralFailure` — the RFC's catch-all — instead of panicking:
+    /// the value comes off the wire (the client session feeds the raw
+    /// reply byte here), so a misbehaving peer must surface as a typed
+    /// error, never a panic. Upstream's `unreachable!()` arm was the
+    /// review's F-1 correction.
     #[inline]
     #[rustfmt::skip]
     pub fn from_u8(code: u8) -> ReplyError {
@@ -237,7 +244,7 @@ impl ReplyError {
             consts::SOCKS5_REPLY_TTL_EXPIRED                => ReplyError::TtlExpired,
             consts::SOCKS5_REPLY_COMMAND_NOT_SUPPORTED      => ReplyError::CommandNotSupported,
             consts::SOCKS5_REPLY_ADDRESS_TYPE_NOT_SUPPORTED => ReplyError::AddressTypeNotSupported,
-            _                                               => unreachable!("ReplyError code unsupported."),
+            _                                               => ReplyError::GeneralFailure,
         }
     }
 }
@@ -383,6 +390,18 @@ mod tests {
         }
     }
 
+    /// F-1 (review): unassigned reply codes (`0x09`–`0xFF`, RFC 1928
+    /// §6) surface as `GeneralFailure` — the RFC's catch-all — never
+    /// a panic. Upstream's `unreachable!()` arm was removed: the code
+    /// comes off the wire (`read_request_reply` → `from_u8`), so a
+    /// misbehaving peer must not crash the consumer task.
+    #[test]
+    fn reply_error_unassigned_codes_map_to_general_failure() {
+        for code in [0x09u8, 0x0a, 0x7f, 0xff] {
+            assert_eq!(ReplyError::from_u8(code).as_u8(), 0x01);
+        }
+    }
+
     #[tokio::test]
     async fn udp_header_round_trip_ip() {
         let target = TargetAddr::Ip("192.168.0.1:80".parse().unwrap());

tasks/implementation/review-vendor-extraction.md

@@ -1,7 +1,7 @@
 ---
 id: review-vendor-extraction
 name: Review — the vendor extraction before dependent layers
-status: pending
+status: completed
 depends_on: [implementation/vendor-differential-tests]
 scope: narrow
 risk: low
@@ -52,22 +52,124 @@ Review against the architecture, in this order:
    e.g. a transitive dependency the cut table missed — the doc was
    updated in the same change, not silently diverged).
 
+## Notes
+
+> To be filled by implementation agent
+
+## Summary
+
+Review performed 2026-09-22 against the six-criteria order; the gate
+opens. One critical finding (F-1) was corrected in the review change
+itself (vendor `mod.rs` + `vendor.md`, same commit); no other
+divergence, cut, or drift was found in the full file-by-file diff vs
+`/workspace/fast-socks5` v1.0.0.
+
+**1. Extraction completeness vs the cut table** — every "What is
+Extracted" row is present; every "What is NOT Extracted" row is
+absent (swept by name for `Socks5Datagram`/`connect*`/`run_*`/
+`udp_bind_random_port`/`handle_udp*`/`transfer_udp`/`Socks5Server`/
+`Socks5Socket`/`Incoming`/`DnsResolveHelper`/`Config<A>`/
+`Authentication`/`authenticate_callback`/auth example impls/`socks4`/
+`resolve_dns`/`tcp_connect*` — the only hit is the preserved verbatim
+error string in `target_addr.rs` referencing `resolve_dns`, a
+documented fidelity keep). Every hunk of all five file diffs
+(`lib.rs`→`mod.rs`, `server.rs`, `client.rs`, `util/stream.rs`,
+`util/target_addr.rs`) was read and categorized: extraction cut,
+carried changes (`new_reply` pub; the BIND surface present), the
+adjustments (anyhow/log removal, error pruning incl. `MissingTargetAddr`,
+macro de-exporting, unwrap/unreachable translations, `write`→
+`write_all`), provenance headers, and first-party tests. The
+commented-out upstream dead code (`OtherReply` etc.) dropping with its
+region is mechanical. No unexplained divergence.
+
+**2. Wasm-clean invariant**: `cargo check --target
+wasm32-unknown-unknown` and `cargo clippy --target
+wasm32-unknown-unknown -- -D warnings` both clean; no `net`-shaped
+feature; tokio on the wasm-clean subset (`rt`, `sync`, `io-util`,
+`macros`, `time` — no `full`) with `default-features = false`; deps
+are exactly tokio/tracing/thiserror (dev: full tokio only).
+
+**3. Convention compliance**: no `unwrap`/`expect`/`unreachable!`/
+`panic!` outside `#[cfg(test)]` modules after the F-1 correction
+(see below); `thiserror` surface intact; naming and module structure
+(one module per file, upstream names preserved, `lib.rs` re-exports
+`vendor`); `#![forbid(unsafe_code)]` present. `Cargo.toml`'s two
+inline comments are the sole code-adjacent comment in non-doc
+position — carried from the scaffolding task's declared posture
+(ADR-009 feature-layout note); no fix demanded.
+
+**4. RFC fidelity spot-check** (independent): fetched RFC 1928 and
+verified against the pinned tests — the reply form (§6: VER/REP/RSV/
+ATYP/BND.ADDR/BND.PORT → 10 bytes v4, 22 bytes v6, RSV=X'00', codes
+`0x00`–`0x08`, `0x09`–`0xFF` unassigned), the method-selection
+preference order (§3 + upstream's outer-loop contract), the UDP
+ASSOCIATE zero fallback (§7: "MUST use a port number and address of
+all zeros"), the 255-byte domain cap (§5: 1-byte length, no NUL), and
+the FRAG parser/caller split (§7: MUST drop FRAG≠0 — refusal is the
+caller's, passthrough the parser's). All pinned bytes correct. The
+differential suite's 32 tests pass; its upstream file+line citations
+were spot-checked against the reference checkout and are accurate.
+
+**5. License/provenance**: `src/vendor/LICENSE` present with the
+upstream MIT text (Copyright (c) 2025 Jonathan Dizdarevic) + v1.0.0
+attribution + ADR-011/vendor.md pointers; referenced from the vendor
+`mod.rs` doc. Vendor point noted in-tree (module docs + LICENSE).
+
+**6. Docs sync**: `vendor.md`'s tables match the tree. The boundary
+corrections discovered by implementers were documented in-task (the
+`mod.rs`-not-`lib.rs` vocabulary placement; the `SocksServerError`
+early landing). The F-1 correction updated `vendor.md`'s adjustment
+list in the same change (panic-translation paragraph + the
+Config-Default/`write_all` notes; `last_updated` bumped).
+
 ## Acceptance Criteria
 
-- [ ] The file-by-file diff vs upstream contains only
+- [x] The file-by-file diff vs upstream contains only
       cut-table/carried-change/adjustment/provenance changes — no
       unexplained divergence
-- [ ] Both wasm checks pass (`cargo check` and
+- [x] Both wasm checks pass (`cargo check` and
       `cargo clippy --target wasm32-unknown-unknown -- -D warnings`)
-- [ ] Full verification suite green: `cargo test`,
+- [x] Full verification suite green: `cargo test`,
       `cargo clippy --all-targets -- -D warnings`, `cargo fmt --check`
-- [ ] No convention violations (unwrap/panics/error surface/naming)
-- [ ] License + provenance notes verified present
-- [ ] `vendor.md` matches the implemented tree (or the mismatch is
+- [x] No convention violations (unwrap/panics/error surface/naming)
+- [x] License + provenance notes verified present
+- [x] `vendor.md` matches the implemented tree (or the mismatch is
       filed as a blocker before proceeding)
-- [ ] Findings recorded; critical findings block the downstream task
+- [x] Findings recorded; critical findings block the downstream task
       groups until resolved
 
+### Findings
+
+- **F-1 (critical → resolved in the review change)**:
+  `ReplyError::from_u8` kept upstream's `unreachable!("ReplyError
+  code unsupported.")` arm (`src/vendor/mod.rs:240` pre-fix) —
+  wire-reachable: `Socks5Stream::read_request_reply` feeds the raw
+  reply byte from the wire into it (`src/vendor/client.rs:340`), and
+  RFC 1928 §6 marks `0x09`–`0xFF` unassigned, so a misbehaving peer
+  panics the consumer task. It escaped the client task's file-scoped
+  no-panic criterion (the panic lives in `mod.rs`, the vocabulary
+  task's file, whose criterion enumerated variants but not panic
+  arms). Fix: unassigned codes map to `GeneralFailure` (the RFC's
+  catch-all) — `src/vendor/mod.rs` `from_u8` + a pinning test
+  (`reply_error_unassigned_codes_map_to_general_failure`); recorded
+  in `vendor.md`'s adjustment list (same change, check-6 discipline).
+  Post-fix suite: 38 + 32 tests, clippy/fmt/wasm/doc green.
+- **F-2 (minor, accepted):** upstream's commented-out dead code
+  (`// OtherReply(u8)` and friends) was dropped rather than copied.
+  Classification: falls under "the extraction cut" + mechanical
+  adjustment (dead text has no behavior; re-vendor diffs are
+  line-based against live code). Noted so a future re-vendor doesn't
+  re-derive the decision.
+- **F-3 (minor, accepted):** `Cargo.toml`'s `[features]` section
+  carries inline comments (AGENTS.md convention 1 says no comments in
+  code; `Cargo.toml` policy comments mirroring a pinned ADR boundary
+  are the established sibling posture — alktunnels/alktty carry none,
+  but the note is ADR-009's empty-features contract, load-bearing
+  until the features populate). Accepted; no fix demanded.
+- **F-4 (minor, accepted):** `ToSocketAddrs`' error text still names
+  the cut `resolve_dns()` — verbatim upstream string, documented in
+  the wire-vocabulary task's Notes and the module doc. Fidelity keep.
+
 ## References
 
 - docs/architecture/vendor.md (the spec under review)
@@ -78,8 +180,17 @@ Review against the architecture, in this order:
 
 ## Notes
 
-> To be filled by implementation agent
-
-## Summary
-
-> To be filled on completion
+- The five implementation tasks' Notes/Summary sections were read and
+  cross-checked: their self-reported verification claims reproduce
+  (`cargo test` 37+32 pre-review → 38+32 post-F-1; wasm/clippy/fmt
+  green), and their boundary corrections were honest and documented
+  (`mod.rs` vocabulary placement; `SocksServerError` early landing).
+  One inaccuracy found: the wire-vocabulary task's acceptance list
+  claimed "No unwrap outside tests" scope as file-local — the F-1
+  panic arm lived in `mod.rs` and was missed by the per-task framing
+  (each task verified its own files); the review's whole-vendor sweep
+  is what caught it. Task summaries left as-is (historical record);
+  the correction is recorded here and in vendor.md.
+- Differential-suite doc-comment citations spot-checked
+  (`server.rs:903-918`, `client.rs:277-285`, `server.rs:1380`,
+  `server.rs:1000-1003`, `server.rs:455-469`): all accurate.