--- id: review-vendor-extraction name: Review (the vendor extraction before dependent layers) status: completed depends_on: [implementation/vendor-differential-tests] scope: narrow risk: low impact: project level: review --- ## Description Review checkpoint before any layer builds on the vendor. Everything else in the crate (params, wire, producer, consumer, doors, backends) consumes the vendored typestate/codecs, so extraction errors (a behavior drift vs upstream, a missed cut, a wasm regression) would propagate into every downstream task. This review is the gate between the vendor group and the rest of the decomposition. Review against the architecture, in this order: 1. **Extraction completeness vs the cut table** (vendor.md): every "What is Extracted" row is present; every "What is NOT Extracted" row is absent. Diff `src/vendor/` against `/workspace/fast-socks5/src/` file-by-file, and the diff must contain exactly: the extraction cut (deletions per the cut table), the two carried changes (`new_reply` publication; the BIND surface being present-and-public), the extraction-time adjustments (anyhow/log removal, error-variant pruning, macro de-exporting, the no-unwrap translations), and the provenance/license notes. Any diff hunk outside those categories is a finding. 2. **Wasm-clean invariant** (ADR-009 §Amendment, ADR-011 §Amendment): `cargo check --target wasm32-unknown-unknown` and native `--no-default-features`-equivalent cleanliness; no `net`-shaped feature exists; `tokio` stays on the wasm-clean subset in `[dependencies]`. 3. **Convention compliance** (AGENTS.md): no `unwrap`/`expect`/panics outside tests; `thiserror` error surface; no comments in code except doc comments and the pinned non-obvious-correctness notes; naming; module structure (one module per file, `lib.rs` re-exports). 4. **RFC fidelity spot-check**: run the differential test suite; for two or three of the trickiest pinned expectations (e.g. the reply-form byte counts, the method-negotiation preference order), independently verify the pinned bytes against the reference checkout and RFC 1928. 5. **License/provenance** (ADR-011): the vendor point (v1.0.0) noted in-tree; the MIT notice present and referenced. 6. **Docs sync**: `vendor.md`'s extraction table and cut table match what landed (if the implementer discovered a boundary correction, 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`/ `Authentication`/`authenticate_callback`/auth example impls/`socks4`/ `resolve_dns`/`tcp_connect*`, with the only hit being 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`, with 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, with the refusal being the caller's and 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 - [x] The file-by-file diff vs upstream contains only cut-table/carried-change/adjustment/provenance changes, with no unexplained divergence - [x] Both wasm checks pass (`cargo check` and `cargo clippy --target wasm32-unknown-unknown -- -D warnings`) - [x] Full verification suite green: `cargo test`, `cargo clippy --all-targets -- -D warnings`, `cargo fmt --check` - [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) - [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), and it is 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) applied to `src/vendor/mod.rs`'s `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()`, a 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) - docs/architecture/decisions/011-inline-vendor.md - docs/architecture/decisions/009-feature-gates.md - AGENTS.md (conventions 1–17; the verification commands) - /workspace/fast-socks5 (the reference checkout for the diff) ## Notes - 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, and 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.