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.

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.