alksocks/tasks/implementation/review-fix-round-002.md

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

id	name	status	depends_on	scope	risk	impact	level
review-fix-round-002	Review — the review-002 fix round before batch 2	completed	implementation/fix-bind-reply2-reentry implementation/fix-producer-gate-policy implementation/fix-rfc1928-posture implementation/fix-hygiene-residue implementation/fix-coverage-residue implementation/fix-test-helper-consolidation architecture/adr-014-idle-timeout-posture	narrow	low	project	review

Description

The closing checkpoint for the review-002 fix round — the gate between the fix tasks and batch 2 (the assembly layer: local providers, front doors, composed providers). Batch 2 consumes the session types, the gate entries, and the fork's posture, so a fix that silently changed a shape (the C-1 take-ownership, the C-3 gate entry, the N-4 vocabulary) would propagate into every door task. This is an inline review (it lives in tasks/ and does not consume the docs/reviews/ numbering — the next general review doc is 003-*).

Verify the fix round against review 002's findings, in this order:

1. C-1 closed correctly: a second await_inbound cannot re-read the data plane (the spent shape pinned by test, the data-plane bytes asserted untouched); the D-1 reader decision is recorded and the alternate path is private/cut/spent-guarded; the session docs match the landed posture.
2. C-3 closed: the substrate-free entry's BIND gate is an explicit policy input; "factory wired" no longer implies "BIND permitted"; the pump handler's identity-derived gate is untouched (the identity matrix green).
3. D-2 closed: the spec'd assembly shape (builder().dial().egress().accept() → .register(...)) is exercised end to end.
4. N-1/N-4 landed coherently: the cut list is gone (the coverage blocks resolved), the keep list carries current notes, the refusal vocabulary is honest at all five sites, the wire bytes are untouched, and the fork's provenance notice is extended-not-replaced (convention 17).
5. D-3 resolved: the ErrorDefinition is declared (or the doc honestly reworded with the reason recorded) — doc-vs-code drift gone either way.
6. N-8 coverage: cargo llvm-cov re-run; the named lines covered or recorded as accepted-uncovered with reasons; no new load-bearing gaps introduced by the fix round itself.
7. N-5/N-6 consolidation: the shared helper module compiles into both test crates; the deliberately-distinct shapes were not collapsed; the constructor prologue is single-sourced.
8. C-2 decided: ADR-014 exists, decides explicitly, and the spec lines carry the posture — the batch-2 gate condition.
9. Cross-cutting: the full verification sweep (test, all-features, clippy, fmt, doc, both wasm checks, publish-dry-run); no convention regressions (no unwrap/expect/panics outside tests; no comments in code beyond doc comments + pinned correctness notes); the batch boundary still holds (no local/composed code exists).

Findings get the review-002 prefix treatment (C/S/D/N with an F- round prefix, e.g. F-1), fixed in the review change when small, filed back as tasks when not. The gate to batch 2 opens only when C-2's ADR is accepted and no major finding remains.

Acceptance Criteria

• Each review-002 finding verified closed (or explicitly re-dispositioned with a reason) — the per-finding checklist above
• cargo llvm-cov re-run and attributed (no new load-bearing gaps)
• The full verification sweep green, including cargo publish --dry-run --allow-dirty
• Convention compliance re-swept (no unwrap/expect/panics outside tests; comment posture; wasm-clean)
• ADR-014 accepted — the batch-2 gate condition holds
• Findings recorded; the gate to batch 2 opens

References

• docs/reviews/002-protocol-core-implementation-review.md (the findings this round closes)
• docs/architecture/decisions/014-*.md (the C-2 decision — the gate condition)
• tasks/implementation/review-protocol-core.md (the inline-review shape and findings format this follows)
• AGENTS.md (the verification commands; conventions 1–17)

Notes

Filled by the review agent (2026-09-25).

The per-finding verification (the task's numbered order)

1. C-1 closed correctly. The take-shape landed as the review's first sketch: BindSession::await_inbound takes the inner stream before reading reply#2 (inner.take() — consumer.rs:792-803); a second call hits the None arm → the spent shape Ok(None) (consumer.rs:793-795). The returned BindDataPlane owns the stream (the TakenInner pairing posture; the session stays the teardown owner) — the re-read is structurally impossible, not just guarded. A failed read restores the inner (the phase is consumed on success only — the post-failure re-call re-reads against the already-ended session, EOFs, never a live data-plane re-read). Pinned by double_await_inbound_returns_the_spent_shape_and_bytes_survive (tests/consumer-bind.rs:497 — the second call Ok(None) + the first plane's echo intact + a fresh session round-trips) and reply1_twice_is_a_view_and_the_reader_yields_one_plane (tests/consumer-bind.rs:583). D-1's reader decision is recorded: option (a) — the session's await_inbound is the one normative reply#2 reader; BindReply1::await_inbound is cut, reply1() is a documented view (bind_addr + raw_mut) — recorded in the fix task's Notes and the module docs (consumer.rs:574-579, 762-765).
2. C-3 closed. The substrate-free entry's BIND gate is the builder's explicit bind_policy (.bind_policy(bool) — producer.rs:736-739, resolved once at build(), self.bind_policy.unwrap_or(self.accept.is_some()) — producer.rs:741-748); "factory wired" no longer implies "BIND permitted" (the presence reading is only the backward-compat default). Pinned by builder_bind_policy_false_refuses_bind_while_connect_associate_ serve (explicit-closed ⇒ 0x02, factory un-called, CONNECT + ASSOCIATE still serve) and builder_default_bind_policy_follows_accept_presence (the default both ways). The pump handler's identity-derived gate is byte-identical (producer.rs:181-185) — the identity matrix (pump_handler_resolves_the_bind_gate_from_the_opener_identity) stayed green untouched.
3. D-2 closed. The spec'd assembly shape is exercised end to end: the harness's RegisterMode::ProducerBuilder arm (tests/harness.rs:581-610) wires the full .dial().egress().accept() chain through Socks5Producer::register(...), and producer_builder_registers_and_serves_all_three_commands (tests/core-integration.rs:996) serves CONNECT + ASSOCIATE + BIND over the topology with the identity witness asserted (the builder-delegated establisher ran).
4. N-1/N-4 landed coherently. The cut list is gone (auth_method_enums!, StandardAuthentication + allow_no_auth, skip_auth_this_is_not_rfc_compliant, CheckResult's Option/Result impls — swept: zero references anywhere); the keep list carries current notes (ConnectError's stream.rs note records the fired trigger + the keep disposition; transfer/wait_on_tcp differential-suite-consumed; Bug upstream-verbatim). The refusal vocabulary is honest at all five sites — SocksServerError::Refused(&'static str) ("Refused: {0}") at the command gate + the four seam refusals (producer.rs 237/269/309/502/560) plus the harness's minimal producer (tests/harness.rs:294); Bug's upstream-verbatim display intact. The wire bytes are untouched (every reply-code assertion stayed green unchanged). Provenance extended-not-replaced: the server.rs module doc's owned-changes paragraph, rfc1928.md's "owned-code post-fork changes" list + the last_updated line; src/rfc1928/LICENSE's generic post-fork clause needed no change.
5. D-3 resolved (the declare side). socks5_open_spec declares the channel:open_failed ErrorDefinition (params.rs:139-151 — the alktty-ported {reason, message} schema, the reason enum exactly SocksEstablishmentReason's parse table), doc-vs-code drift gone. Pinned by open_spec_declares_the_open_failed_error_definition (params.rs:256-292), which cross-checks every declared enum value through the typed parse table — the schema and the typed vocabulary cannot drift.
6. N-8 coverage re-run and attributed. cargo llvm-cov on the final tree: 96.30% regions / 96.43% functions / 97.28% lines (5142 covered / 140 missed). Every N-8-named line is covered (verified against --show-missing-lines: the stage's failure exits, both splits, the spent arms, the mid-batch Err arm, into_inner's Ok, the plane passthroughs, read_reply2's version pin, the quiet-wait Err(_) arm, the Domain-peer fallback, the Debug/Display renders). The residual uncovered lines are exactly the task's recorded set: producer.rs 188-189 (the accept_bi substrate-failure log arm — untestable outside alkcall's test surface), rfc1928/client.rs 217-219 + internals (the review-001/002 accepted-uncovered posture), rfc1928/mod.rs 136-141 / target_addr.rs 132/151/180 (the fork's pre-existing residue), plus test-module panic-guard branches (coverage-of-tests noise) and consumer.rs 937 (the spent-shape diagnostic closure — evaluates only on Err, the spent shape is Ok(None) by design; cosmetic, not chased). No new load-bearing gaps introduced.
7. N-5/N-6 consolidation verified. The shared module (tests/common/mod.rs, 314 lines) compiles into both crates via the crate-root #[cfg(test)] #[path] mount + the extern crate self alias (lib.rs:73-82) and harness's name-preserving re-exports — the lib tests and the integration suite build against the same copy; no tests/harness.rs import from src/. The deliberately-distinct shapes stayed test-local (HotNoneEgress/PureHotNoneEgress/the drop-witness egresses); the constructor prologue is single-sourced (open_and_negotiate, consumer.rs:85-108 — the teardown-at-birth invariant in one place; N-6).
8. C-2 decided. ADR-014 exists (docs/architecture/decisions/014-idle-timeout-posture.md, status Accepted, 2026-09-24), deciding option 1 explicitly (the protocol layer never times out; every phase teardown-governed), with the ADR-007 same-regime relationship stated, the doors' accept-path obligations recorded pre-batch-2, and the revisit-trigger written. The spec lines carry the posture (wire.md §UDP ASSOCIATE's association-lifetime bullet + the no-in-band-vocabulary bullet; producer.md §The Open Handler + the local-backend section + the decision table).
9. Cross-cutting sweep green (below); the batch boundary holds (no local/composed code exists — the features are declared empty in Cargo.toml per ADR-009, no cfg(feature) in src/).

Findings (this review round)

No correctness, security, or coverage findings. One minor doc residue, fixed in this review change (the F-1 treatment):

• F-1 [minor] — review-numbering collision: three code comments/docs and two sites in the review doc cited "review 002 R-1/R-2/R-4", but those R-findings belong to the batch-1 gate review (tasks/implementation/review-protocol-core.md — the unnumbered inline review); "review 002" now unambiguously means the C/S/D/N general review this task gates. An operator (or a later sweep) following "review 002 R-1" would land in the wrong document. Fixed at all six sites (producer.rs ×2, consumer.rs, egress.rs, the review doc ×2) — reworded to "the gate review's R-N (tasks/implementation/review-protocol-core.md)". Zero code impact; the review doc's remaining in-body R-1/R-2 mentions ride the references section, which already names the task file. Also removed an empty stray src/producer/ directory (a leftover, tracked by nothing).

Verification (the full sweep, this tree)

• cargo test: 375 green (145 lib + 38 associate + 41 bind + 40 connect + 41 integration + 30 harness + 37 differential + 3 doctests, 2 ignored).
• cargo test --all-features: green (same counts); also green with --features local and --features composed (both empty).
• cargo clippy --all-targets -- -D warnings: clean.
• cargo fmt --check: clean.
• cargo doc --no-deps: clean.
• cargo check + clippy -D warnings (wasm32): clean.
• cargo publish --dry-run --allow-dirty: completes.
• cargo llvm-cov: 96.30% regions / 96.43% functions / 97.28% lines (5142 covered / 140 missed) — the per-module table: consumer 98.60% lines, egress 99.67%, error 100%, params 100%, producer 95.70%, wire 96.58%, rfc1928 96.53–99.39% (the accepted-uncovered fork blocks ride client.rs's 96.53%).
• Convention compliance re-swept: every unwrap/expect/ panic!/todo!/unimplemented! hit across all of src/ (eleven files) mechanically attributed — all inside #[cfg(test)] regions; no panics in non-test code; comment posture held (doc comments + pinned correctness notes only); no cfg(feature) outside Cargo.toml declarations.

The gate to batch 2

Open. ADR-014 is accepted (the C-2 gate condition holds); no major finding remains (zero findings beyond F-1, minor + fixed); the full sweep is green; the fix-round's shapes (the take-guard, the bind_policy gate, the Refused vocabulary, the declared ErrorDefinition) are the shapes batch 2's door tasks consume. The next general review doc is 003-* (this checkpoint was an inline review in tasks/ — it consumed none of the docs/reviews/ numbering).

Summary

The review-002 fix round is verified closed, finding by finding (the task's nine-point checklist above): C-1's take-guard with the spent shape pinned and the data-plane bytes asserted; C-3's explicit bind_policy gate; D-2's full builder-through-register pin serving all three commands; N-1's cut list gone with the keep-notes current; N-4's Refused vocabulary at all five refusal sites (wire untouched); D-3's ErrorDefinition declared and drift-free-pinned; N-8's coverage re-run green (96.30%/97.28%, every named line covered, the residual attributed to the recorded accepted- uncovered set); N-5/N-6 consolidated on the shared module + open_and_negotiate; C-2 decided by accepted ADR-014 with the spec lines carrying the posture. One minor finding (F-1, the review- numbering collision on the R-citations) fixed in this change. The full verification sweep is green (test 375, all-features, clippy, fmt, doc, both wasm checks, publish dry-run, llvm-cov), conventions re-swept clean, the batch boundary holds, and the gate to batch 2 opens.