diff --git a/docs/reviews/003-assembly-layer-implementation-review.md b/docs/reviews/003-assembly-layer-implementation-review.md index d44a70e..dcc36af 100644 --- a/docs/reviews/003-assembly-layer-implementation-review.md +++ b/docs/reviews/003-assembly-layer-implementation-review.md @@ -1,7 +1,8 @@ --- -status: open -last_updated: 2026-09-26 (the review written against the post-gate tree - `04c76e3` + the review's inline fixes; F-1/F-2 fixed in this change) +status: resolved +last_updated: 2026-09-27 (the residue round closed — S-1/N-1/N-2 doc + notes, D-1/D-2/D-3 + the N-8 probes landed as tests, the frontmatter + YAML fixed; every finding closed or re-dispositioned) reviewed_artifacts: - src/door.rs - src/local/mod.rs @@ -113,6 +114,14 @@ failure the docs call fatal). One coverage finding (D-2) is the batch-2 analogue of review 002's D-2 (the spec'd surface unexercised); the rest are small. +**Resolution (2026-09-27)**: the residue round closed the review — +S-1/N-1/N-2/N-3 as the recommended doc notes, D-1/D-2/D-3 as the +recommended tests, N-8's worthwhile probes landed (the remainder +accepted-uncovered with the disposition recorded per section); the +frontmatter's invalid YAML (plain-scalar colons in +`reviewed_artifacts`) fixed. Full verification green; see the +disposition table for commit pointers. + Prefixes follow the review-002 legend: `F` (correctness, fixed inline — continuing 002's `C` family with the batch's review-fixed disposition), `S` (security posture), `D` (dead surface/coverage), `N` @@ -208,7 +217,7 @@ monitoring `serve()` can now distinguish fatal from graceful. --- -## S-1 [minor] — The UDP door's reply path trusts the recorded client address for the association's whole lifetime: a spoofed first datagram hijacks every subsequent reply +## S-1 [minor, closed 2026-09-27] — The UDP door's reply path trusts the recorded client address for the association's whole lifetime: a spoofed first datagram hijacks every subsequent reply **Files**: `src/door.rs` (`UdpFrontDoor::serve_on`'s `client_addr` capture — the first inbound datagram's source wins permanently) @@ -240,7 +249,11 @@ accepts the off-path-first-datagram shape, the RFC's own posture") so the next reader does not mistake it for a bug. Filing now so the residual is a named decision, not an accident. -## D-1 [minor] — `associate_with_token` remains 0%-covered dead surface (the batch-2 completeness twin with no caller) +**Closed (the residue round)**: the recommendation landed — the +accepted-uncovered posture note is on `UdpFrontDoor::serve_on`'s doc +(commit `87afe70`). + +## D-1 [minor, fixed 2026-09-27] — `associate_with_token` remains 0%-covered dead surface (the batch-2 completeness twin with no caller) **Files**: `src/consumer.rs:450-475` (`associate_with_token` — uncovered; contrast `connect_with_token`/`bind_with_token`, both @@ -262,7 +275,14 @@ token-provider topology) — or an explicit accepted-uncovered note on the method. Either closes the review-002-D-1-class trigger before it fires a third time. -## D-2 [minor] — `ComposedEgress`'s failure/edge matrix is the batch-2 coverage gap: the LRU's structural `Err` arm, `relay_read`'s sender-closed exit, and the eviction-vs-live-relay accounting are untested +**Fixed (the residue round)**: `associate_session_with_token_round_trip` +(`tests/door-tcp.rs`, commit `59e8fb1`) — the probe drives the token +twin against the token-provider topology (the CF-005 open, the +sentinel BND.ADDR, a datagram round-trip through the echo egress, +shutdown reap). The review-002-D-1-class trigger is closed with the +test, not the note. + +## D-2 [minor, fixed 2026-09-27] — `ComposedEgress`'s failure/edge matrix is the batch-2 coverage gap: the LRU's structural `Err` arm, `relay_read`'s sender-closed exit, and the eviction-vs-live-relay accounting are untested **Files**: `src/composed/mod.rs` (module total 77.32% lines / 65.31% functions — the suite's lowest), `tests/composed-egress.rs` (6 tests, @@ -291,14 +311,29 @@ shapes that no test drives: upstream, so the guard should be unreachable, but nothing pins the guard fires (or that the tunnel codec's cap makes it unreachable). -**Recommendation**: one small test round before release polish: (a) -an egress with `max_tunnels: 1` whose single tunnel's far side EOFs → +**Recommendation**: one small test round before release polish: (a) an +egress with `max_tunnels: 1` whose single tunnel's far side EOFs → the association-end `Err` fires through the eviction path; (b) a unit probe on the invariant-`Err` arm's message shape (or fold it into (a)). The module's 17 uncovered functions are nearly all these two regions. -## D-3 [minor] — The `local` providers' refusal-mapping tables are partially uncovered: `dial_error_of`'s `TimedOut`/`PermissionDenied` arms and `bind_accept`'s `Shortage`-arm splits are compile-checked only +**Fixed (the residue round)**: commit `b43bf54` — (a) landed as +`eviction_to_a_failed_dial_ends_the_association_when_the_relay_ends` +(`tests/composed-egress.rs`; the genuine eviction→association-end +shape is the cap-1 eviction whose *replacement dial fails* — the far +side EOFs the evicted tunnel, the count reaches 0, the parked +`next_reply` returns the association-end `Err`; the originally +suggested far-side-EOF shape cannot reach 0 live relays because the +replacement tunnel's relay is always live — the test taught the +composition); the relay-exit pair pinned directly in +`src/composed/mod.rs`'s `egress_unit_tests` (the sender-closed silent +exit + the EOF→`Ended` path, with `Inbound` gaining `#[derive(Debug)]`); +the oversize guard pinned at the bound edge; the `Ended`-marker +saturating_sub discipline pinned (a live relay keeps the stage serving +beside an ended one). Module coverage: 77.32% → 85.49% lines. + +## D-3 [minor, fixed 2026-09-27] — The `local` providers' refusal-mapping tables are partially uncovered: `dial_error_of`'s `TimedOut`/`PermissionDenied` arms and `bind_accept`'s `Shortage`-arm splits are compile-checked only **Files**: `src/local/mod.rs:76-81` (`dial_error_of`'s `NetworkUnreachable`/`HostUnreachable`/`TimedOut`/`PermissionDenied` @@ -326,7 +361,17 @@ construct errors of each kind with `io::Error::new`); two tests, no new harness. The two structural arms (`no addresses tried`, the resolve-empty arm) stay accepted-uncovered with a note each. -## N-1 [observation] — `ProducerDoorGuard::close`'s `drop(self)`-then-await shape is correct but non-obvious: one doc line would prevent a "simplification" that deadlocks +**Fixed (the residue round)**: commit `59e8fb1` — four table tests in +`src/local/tests.rs`: the `dial_error_of` kind→arm table (all six +arms, incl. the `TimedOut`-shares-`NetworkUnreachable` and the +`PermissionDenied`→`General` shapes + the context rides the message), +the dial vocabulary's reply codes, the bind-map's kind→variant table +(`AddrInUse`/`PermissionDenied`→`Shortage`, `AddrNotAvailable`→ +`Refused`, the `_` catch-all→`General`), and the bind map's reply +codes. The two structural arms (`no addresses tried`, the +resolve-empty arm) stay accepted-uncovered as recommended. + +## N-1 [observation, closed 2026-09-27] — `ProducerDoorGuard::close`'s `drop(self)`-then-await shape is correct but non-obvious: one doc line would prevent a "simplification" that deadlocks **Files**: `src/local/producer_door.rs:194-202` @@ -344,7 +389,10 @@ before the await — the loop's `done_tx.closed()` fires only when every receiver is gone; holding it here would deadlock." Zero behavior change. -## N-2 [observation] — `bindfirst_accept`'s factory errors map to reply#1 codes whose vocabulary is wider than the producer's gate: `NotAllowed`/`Shortage` from a *listen resource* deserve a doc cross-check against the producer-side `bind_policy` gate +**Closed (the residue round)**: the doc line landed on `close` +(commit `87afe70`). + +## N-2 [observation, closed 2026-09-27] — `bindfirst_accept`'s factory errors map to reply#1 codes whose vocabulary is wider than the producer's gate: `NotAllowed`/`Shortage` from a *listen resource* deserve a doc cross-check against the producer-side `bind_policy` gate **Files**: `src/composed/mod.rs:718-820` (the factory's `Refused`/`NotAllowed`/`Shortage` mappings) @@ -367,7 +415,10 @@ policy sources. **Recommendation**: one doc line on `bindfirst_accept` naming the layering. No code change. -## N-3 [observation] — `FrontDoor::serve_on`'s per-accept task holds the session-cap permit for the connection's whole life, but the idle-first-byte timeout does not release early: a 256-cap door parks at "cap reached" for connections that are merely slow *readers* +**Closed (the residue round)**: the layering note landed on +`bindfirst_accept`'s doc (commit `87afe70`). + +## N-3 [observation, closed 2026-09-27] — `FrontDoor::serve_on`'s per-accept task holds the session-cap permit for the connection's whole life, but the idle-first-byte timeout does not release early: a 256-cap door parks at "cap reached" for connections that are merely slow *readers* **Files**: `src/door.rs:338-374` (the accept loop + `Semaphore`), `src/door.rs:393-401` (the idle-first-byte gate) @@ -388,6 +439,10 @@ first-byte ones). Same shape on `ProducerDoor`. both builders, or leave as-is — the knob's name is the docs' vocabulary, not a contract. Zero code. +**Closed (the residue round)**: both builders' `max_sessions` docs now +say "max-concurrent-*connections*" with the mechanism named (commit +`87afe70`). + ## N-4 [observation] — The cred-bearing door's `local_rfc1929_half` check closure writes the mapped identity through a `std::sync::Mutex` witness even when the check rejects: the `None` slot survives a failed check (harmless, but the witness's semantics are "last lookup", not "accepted identity") **Files**: `src/door.rs:530-557` @@ -461,7 +516,7 @@ docs. compliant, and documented). Recorded to bound re-litigation: the split-vs-eager-bind choice is per-type, not a pattern to propagate. -## N-8 [observation] — Coverage residue worth one line each (or accepted-uncovered with a note), beyond D-2/D-3 +## N-8 [observation, closed 2026-09-27] — Coverage residue worth one line each (or accepted-uncovered with a note), beyond D-2/D-3 The `--all-features --show-missing-lines` sweep attributed; these are the residuals beyond the findings (each small, named so the next @@ -521,6 +576,43 @@ The headline gaps: the BIND door's Domain-form reply wire (D-2's neighbor), the composed egress's failure matrix, and the `local/mod.rs` mapping-table arms. +**Closed (the residue round, commit `b43cbd2` + the D-2/D-3 commits)**: +the worthwhile probes landed; the timing-shaped/structural remainder +stays accepted-uncovered: + +- **Probed**: `CredentialTable::len`/`is_empty` + (`credential_table_len_and_is_empty_accessors`); `FrontDoor::serve` + (`front_door_serve_wrapper_round_trips`) and `UdpFrontDoor::serve` + (`udp_door_serve_wrapper_round_trips`) — both bind-then-serve + wrappers now round-trip; `write_refusal_reply`'s `_ => return` arm + (`write_refusal_reply_skips_non_reply_code_shapes`) AND the + re-encode's exact wire bytes pinned + (`write_refusal_reply_encodes_the_mapped_code`); the bridge's + garbage-in-a-later-batch probe + (`garbage_in_a_later_batch_frame_fails_loud`); the BIND door's + Domain-form reply wire — the batch-2 suite's most substantive + single-arm gap — closed by extracting the hand-written wire into + `bind_reply1_wire` (a pure function) and pinning both forms' bytes + (`bind_reply1_wire_forms`); D-2's matrix and D-3's tables (their + own sections above). +- **Accepted-uncovered (unchanged)**: the `DoorExit`/`UdpDoorExit`/ + `BackendExit` Display arms and `TokenClient`'s Debug (cosmetic); + the `(Some(_), None)` unreachable-by-construction arm (by design); + the timing-shaped UDP-door single arms (the `client_addr`-set skip, + the reply-before-first-inbound park, the spent-`split` arm, the + F-2 `Io`-exit arm — not producible without a poisoned socket); the + `local/mod.rs` structural terminals (D-3's accepted-uncovered pair); + the `producer_door.rs` accept-loop/error arms of the same + timing-shaped class; `egress.rs:518`'s test-internal panic arm; + `consumer.rs`'s two formatting arms; `raw_pipe`'s open-failure exit + and `pipe_connect_plane`'s Domain/`None`-reply arms (the NoAuth + open-failure shape needs a poisoned topology the harness cannot + produce; the CONNECT Domain reply rides the codec's own + `new_reply`-adjacent path, lower-value than the BIND wire). +- Total coverage moved 93.68% → **93.94% lines** (92.67% functions / + 94.74% regions); `composed/mod.rs` 77.32% → 85.49%, `door.rs` + 81.45% → 85.84%, `local/mod.rs` 84.91% → 89.19%. + --- # Part B — Non-findings (verified correct, recorded to bound the re-review) @@ -639,23 +731,31 @@ neighbor), the composed egress's failure matrix, and the # Suggested Disposition The batch-2 gate closed with the crate feature-complete; this review -found the residue a full-surface read always finds. Disposition: +found the residue a full-surface read always finds. Disposition +(the residue round executed 2026-09-27 — every row closed except the +per-item defers recorded in the sections above): | Item | Action | Shape | |---|---|---| | F-1, F-2 | **Fixed in this review change** | the refusal re-encoding + the UDP-door exit match + the pinning test | -| D-1 | one test or an accepted-uncovered note | the `associate_with_token` probe (or the note) in the release-polish round | -| D-2 | one small test round before release polish | the composed-egress failure matrix (2 tests) | -| D-3 | two table-driven unit tests | the `io::ErrorKind` → vocabulary tables | -| S-1 | accepted-uncovered posture note | one doc line on `UdpFrontDoor::serve_on` | -| N-1 | one doc line | the `close` drop-before-await note | -| N-2 | one doc line | the `bindfirst_accept` 0x02 layering note | -| N-3 | one doc word (or none) | "connections" vs "sessions" on both builders | +| S-1 | **Closed** — the accepted-uncovered posture note | `UdpFrontDoor::serve_on`'s doc (`87afe70`) | +| D-1 | **Fixed** — the test, not the note | `associate_session_with_token_round_trip` (`59e8fb1`) | +| D-2 | **Fixed** — the failure matrix | the eviction→association-end + the relay-exit/oversize unit probes (`b43bf54`) | +| D-3 | **Fixed** — the io-kind tables | four table tests in `src/local/tests.rs` (`59e8fb1`) | +| N-1 | **Closed** — the doc line | the `close` drop-before-await note (`87afe70`) | +| N-2 | **Closed** — the doc line | the `bindfirst_accept` 0x02 layering note (`87afe70`) | +| N-3 | **Closed** — the doc word | "connections" on both builders (`87afe70`) | | N-4 | none | recorded to bound re-review | | N-5 | defer | extract when a third accept-loop shape lands | | N-6 | none | covered by review 002 S-1's posture | | N-7 | none | recorded to bound re-review | -| N-8 | batch the cosmetics with D-2/D-3's tests | the release-polish round | +| N-8 | **Closed** — the worthwhile probes landed, the remainder dispositioned | the BIND Domain wire helper + table, the refusal-recode pins, the `serve()` wrappers, `len`/`is_empty`, the bridge garbage probe (`b43cbd2`) | + +Verification after the residue round: `cargo test` (default) and +`cargo test --all-features` green (15 suites); clippy (`--all-targets +--all-features` and both wasm targets) clean; fmt/doc clean; +`cargo publish --dry-run --allow-dirty` completes; coverage 93.94% +lines (93.68% at review time). The natural home for the residue is the release-polish task (publish prep, OQ-SK2-08) the batch-2 decomposition deferred — the review's