--- id: review-fix-round-002 name: "Review: the review-002 fix round before batch 2" status: completed depends_on: [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] scope: narrow risk: low impact: project level: 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, because 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), so doc-vs-code drift is 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, which is 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 - [x] Each review-002 finding verified closed (or explicitly re-dispositioned with a reason) via the per-finding checklist above - [x] `cargo llvm-cov` re-run and attributed (no new load-bearing gaps) - [x] The full verification sweep green, including `cargo publish --dry-run --allow-dirty` - [x] Convention compliance re-swept (no unwrap/expect/panics outside tests; comment posture; wasm-clean) - [x] ADR-014 accepted, the batch-2 gate condition - [x] 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()`, at 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), making the re-read 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), where the session's `await_inbound` is the one normative reply#2 reader; `BindReply1::await_inbound` is cut, and `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())` per 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), and 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, all 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, so 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 `split`s, 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, which 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, so 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, since 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) with a rewording 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 are 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/`, so 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.