From 431920ddefe970a5aa38c7e32f96048d1fe05116 Mon Sep 17 00:00:00 2001 From: "glm-5.3-flash" Date: Fri, 25 Sep 2026 04:02:16 +0000 Subject: [PATCH] =?UTF-8?q?review:=20the=20fix-round=20gate=20closed=20?= =?UTF-8?q?=E2=80=94=20every=20review-002=20finding=20verified,=20batch=20?= =?UTF-8?q?2=20opens?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The closing checkpoint (tasks/implementation/review-fix-round-002.md): each review-002 finding verified closed against the landed tree, the full verification sweep green, conventions re-swept, and the gate condition (ADR-014 accepted, no major finding) holds. Per-finding verification (all nine checklist points): - C-1: the take-guard verified — a second await_inbound returns the spent shape Ok(None), the re-read structurally impossible (the plane owns the stream); pinned by the double-read + reply1-twice tests. D-1's one-reader decision recorded (option (a), BindReply1 cut). - C-3 closed: the builder's explicit bind_policy is the substrate-free gate; the presence reading is only the backward-compat default; the pump handler's identity-derived gate byte-identical. - D-2 closed: the full builder chain through .register(...) serves all three commands end to end (RegisterMode::ProducerBuilder + the integration test, identity witness asserted). - N-1/N-4 coherent: the cut list gone (zero references), keep-notes current, the Refused vocabulary honest at all five sites + the harness's minimal producer, wire untouched, provenance extended. - D-3 resolved (declare side): the channel:open_failed ErrorDefinition declared, schema pinned drift-free against the typed parse table. - N-8 re-run: 96.30% regions / 97.28% lines; every named line covered; the residual attributed (accept_bi log arm, fork accepted-uncovered, test panic-guards) — no new load-bearing gaps. - N-5/N-6 verified: the shared module compiles into both crates via the crate-root mount + extern-crate-self alias; open_and_negotiate single-sources the teardown-at-birth prologue. - C-2 decided: ADR-014 accepted (option 1 — teardown-governed phases), the spec lines carry the posture. - Cross-cutting sweep green: test 375 green (+ all-features, local, composed), clippy -D warnings clean, fmt clean, doc clean, both wasm checks clean, publish dry-run completes; no unwrap/expect/panic in non-test code (mechanically attributed across src/); no local/ composed code exists (the batch boundary holds). One gate-round finding, F-1 (minor), fixed in this change: the review-numbering collision — "review 002 R-1/R-2/R-4" cited the batch-1 gate review's findings (tasks/implementation/ review-protocol-core.md) under a name that now means the general review; reworded to "the gate review's R-N" at all six sites (producer.rs, consumer.rs, egress.rs, the review doc). Review 002 marked resolved; the next general review doc is 003-*. Verification: cargo test (375 green), --all-features + --features local/composed green, clippy --all-targets -D warnings clean, fmt --check clean, doc clean, both wasm checks clean, publish dry-run completes, llvm-cov 96.30% regions / 97.28% lines attributed. --- ...002-protocol-core-implementation-review.md | 20 +- src/consumer.rs | 3 +- src/egress.rs | 7 +- src/producer.rs | 6 +- tasks/implementation/review-fix-round-002.md | 206 +++++++++++++++++- 5 files changed, 222 insertions(+), 20 deletions(-) diff --git a/docs/reviews/002-protocol-core-implementation-review.md b/docs/reviews/002-protocol-core-implementation-review.md index 840eb12..4edece2 100644 --- a/docs/reviews/002-protocol-core-implementation-review.md +++ b/docs/reviews/002-protocol-core-implementation-review.md @@ -1,6 +1,8 @@ --- -status: open -last_updated: 2026-09-24 +status: resolved +last_updated: 2026-09-25 (the fix round verified closed — the + review-fix-round-002 gate; every finding closed or + re-dispositioned, F-1 fixed in the gate change) reviewed_artifacts: - src/lib.rs - src/wire.rs @@ -152,7 +154,8 @@ what makes this one inconsistent rather than a design choice: - `Socks5ChannelSession`: the full conversation is consumed by the constructor; `into_inner` is the only taking method and carries - the `SpentSessionError` guard (review 002 R-2's shape). + the `SpentSessionError` guard (the gate review's R-2 shape — + `tasks/implementation/review-protocol-core.md`). - `AssociateSession`: `send_datagram`/`recv_datagram` are a proper state machine (reader state, spent-stream `Io`/`None` shapes); no re-entry ambiguity. @@ -393,7 +396,7 @@ setters — only `.dial` rides `builder_shape_round_trips`), harness: **Problem**: `producer.md` §Wiring presents `producer.register(&core, ®istry, auth, None)?` as *the* assembly shape — the spec'd entry point — and the landed doc (producer.md -`Wiring`, reconciled by review 002 R-4) names exactly this +`Wiring`, reconciled by the gate review's R-4) names exactly this signature. No test exercises it: the harness re-implements the registration via `register_socks5_openable` directly (same delegation, different call path), and the builder test drives only @@ -731,7 +734,7 @@ without this note. # Part B — Non-findings (verified correct, recorded to bound the re-review) - **The seam contracts hold under adversarial reading.** Re-derived - independently of review 002: the readiness contract's R-1 + independently: the readiness contract's R-1 yield-guard is correct and necessary (the zero-yield unit test reproduces the hazard without it); ownership-rides-the-handle has no `_` bindings anywhere in non-test code (swept); the accept @@ -858,6 +861,13 @@ N-7 is recorded-only (no action — the review's own recommendation); its bound-re-litigation note lives in the review itself. +**Resolved 2026-09-25** (`tasks/implementation/ +review-fix-round-002.md` — the closing gate): every finding closed +or re-dispositioned; the one gate-round finding (F-1, the +review-numbering collision on the R-citations — the R-findings +belong to the batch-1 gate review) fixed in the gate change. The +batch-2 gate is open (ADR-014 accepted; no major finding remains). + ## References - `docs/reviews/001-rfc1928-full-review.md` (the fork review — the diff --git a/src/consumer.rs b/src/consumer.rs index 6013f82..7a4cc2e 100644 --- a/src/consumer.rs +++ b/src/consumer.rs @@ -107,7 +107,8 @@ where } } -/// The spent-session marker (review 002 R-2): [`Socks5ChannelSession +/// The spent-session marker (the gate review's R-2, +/// tasks/implementation/review-protocol-core.md): [`Socks5ChannelSession /// ::into_inner`] / [`AssociateSession::into_inner`] return it when /// the pipe/stream was already taken — reachable only by calling /// twice on a session whose inner is gone. Carries nothing: a spent diff --git a/src/egress.rs b/src/egress.rs index b3dd3d7..c82a26e 100644 --- a/src/egress.rs +++ b/src/egress.rs @@ -41,9 +41,10 @@ //! //! The readiness-contract behavioral tests (an instant-`Ok(None)` //! egress must not starve the datagram stage — including the -//! zero-yield shape, review 002 R-1) live in the producer module's -//! test suite, next to the stage they pin. This module carries the -//! error→reply-code mapping tables, the sync-lift factory +//! zero-yield shape, the gate review's R-1, +//! tasks/implementation/review-protocol-core.md) live in the producer +//! module's test suite, next to the stage they pin. This module +//! carries the error→reply-code mapping tables, the sync-lift factory //! constructor, and the compile-shape assertions. use std::future::Future; diff --git a/src/producer.rs b/src/producer.rs index 3b0eff4..9f8241a 100644 --- a/src/producer.rs +++ b/src/producer.rs @@ -383,7 +383,8 @@ where // and the runtime's other tasks on a current-thread // scheduler (an instantly-ready branch that yields // nothing wins every poll — the readiness contract's POC - // #2 finding, hardened by review 002 R-1). + // #2 finding, hardened by the gate review's R-1 + // (tasks/implementation/review-protocol-core.md)). reply = egress.next_reply() => { match reply { Ok(Some(frame)) => { @@ -1242,7 +1243,8 @@ mod tests { .expect("clean"); } - /// The readiness contract's zero-yield bound (review 002 R-1): an + /// The readiness contract's zero-yield bound (the gate review's + /// R-1, tasks/implementation/review-protocol-core.md): an /// egress whose `next_reply` is a pure `async { Ok(None) }` — /// instantly ready AND yielding no poll boundary — must still not /// starve the stage's channel branch. The stage's `Ok(None)` diff --git a/tasks/implementation/review-fix-round-002.md b/tasks/implementation/review-fix-round-002.md index b6085c8..58386cf 100644 --- a/tasks/implementation/review-fix-round-002.md +++ b/tasks/implementation/review-fix-round-002.md @@ -1,7 +1,7 @@ --- id: review-fix-round-002 name: "Review — the review-002 fix round before batch 2" -status: pending +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 @@ -65,17 +65,17 @@ when C-2's ADR is accepted and no major finding remains. ## Acceptance Criteria -- [ ] Each review-002 finding verified closed (or explicitly +- [x] 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 +- [x] `cargo llvm-cov` re-run and attributed (no new load-bearing gaps) -- [ ] The full verification sweep green, including +- [x] The full verification sweep green, including `cargo publish --dry-run --allow-dirty` -- [ ] Convention compliance re-swept (no unwrap/expect/panics +- [x] 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 +- [x] ADR-014 accepted — the batch-2 gate condition holds +- [x] Findings recorded; the gate to batch 2 opens ## References @@ -89,8 +89,196 @@ when C-2's ADR is accepted and no major finding remains. ## Notes -> Agent fills this during implementation. +> 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 `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 — 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 -> Agent fills this on completion. \ No newline at end of file +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. \ No newline at end of file