alksocks/tasks/implementation/review-vendor-extraction.md

id, name, status, depends_on, scope, risk, impact, level

id	name	status	depends_on	scope	risk	impact	level
review-vendor-extraction	Review — the vendor extraction before dependent layers	completed	implementation/vendor-differential-tests	narrow	low	project	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 — 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<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 cut-table/carried-change/adjustment/provenance changes — no unexplained divergence
• Both wasm checks pass (cargo check and cargo clippy --target wasm32-unknown-unknown -- -D warnings)
• 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 filed as a blocker before proceeding)
• 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)
• 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 — 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.