diff --git a/docs/reviews/002-graduation-implementation-review.md b/docs/reviews/002-graduation-implementation-review.md new file mode 100644 index 0000000..57ab933 --- /dev/null +++ b/docs/reviews/002-graduation-implementation-review.md @@ -0,0 +1,674 @@ +# Review 002 — The Graduation Implementation (direct op + bind-first + forwarded op) + +## Status + +**Conformant with findings.** Six sessions reviewed +(`52b6252..474844e` — the taskgraph fix + the six graduation feature +commits): the direct op end to end, the bind-first establisher + accept +dispatcher, the forwarded op's serving side, the local bind-first +halves, and `TunnelListener`. Wire surface, scope separation, typed +errors, teardown ownership, and the R-02/ADR-050 contracts all conform +(verified against the alkcall 0.8.0 source, not just its docs). No +[critical] findings. **One [minor] implementation gap (M-1) and a +cluster of [major]-labeled doc/spec-sync items (D-1..D-4) that are +doc-only in code impact** — the code is the wire-stable side and is +correct; the *docs* lag it. Nothing blocks the graduation consumers. + +Findings use prefix `M` (minor code), `D` (doc/spec sync), `N` (notes). +This is the second review in this crate's series (numbering follows the +alkcall house pattern — each review numbers independently). + +## Scope + +The six sessions' work at tree `474844e` reviewed against ADR-007, +ADR-008 (as amended), the updated wire.md/consumer.md/producer.md, the +AGENTS.md conventions, and — load-bearing — the alkcall 0.8.0 source +(`/workspace/@alkdev/alkcall/src/channels/{operations,pump,manager,mux,source,client}.rs`, +`src/registry/registration.rs`), cross-checking the six contracts this +implementation leans on. + +``` +Verified against: alktunnels 474844e; alkcall 0.8.0 (source-read) +Reading list: src/{lib,params,producer,consumer,error,wire}.rs, + src/local/mod.rs, tests/{harness,producer_bindfirst,producer_direct_op, + producer_forwarded,consumer_listener,local_bindfirst}.rs, + docs/architecture/*.md incl. decisions/007, 008 +Executed: cargo test (132 default / 157 --features local, doc-tests 2/2), + clippy --all-targets -D warnings (native + wasm32), fmt --check, + cargo doc --no-deps (0 warnings), cargo check + clippy on + wasm32-unknown-unknown — all green, matching the commit messages' claims; + cargo-llvm-cov 0.8.4 line/region coverage, default (95.63%) + + --features local (93.66%) — see the coverage appendix +Source-read (alkcall 0.8.0): pump_bidi shutdown-on-completion, + teardown_channel drop-sender EOF, mux implicit-EOF sentinel, + yield-once accept_bi + accepted-flag telemetry, + Establishment::with_reply_field + reserved-key guard, + open_channel_with_reply shape, registry input_schema gate + (compiled at registration, runs after ACL, INVALID_INPUT on violation) +``` + +## Severity legend + +Same scale as review 001 / the alkcall reviews: + +- **[critical]** — a decided spec invariant violated so a promised + capability is unreachable end-to-end; or data corruption. +- **[major]** — a core protocol path cannot serve a decided behavior; + or a wire/ABI promise is not delivered by the code. +- **[minor]** — drift, doc/spec inconsistency, or a missing + convenience with no correctness impact. + +--- + +# Checklist verdicts + +1. **Wire conformance (ADR-007/008 + wire.md) — PASS.** The direct op's + spec (`tunnel_direct_spec`) and the forwarded op's spec + (`tunnel_forwarded_spec`) match wire.md §The Direct Op / §The + Forwarded Op: op ids, `Sub` type, `alk/tunnel` ALPN marker, the + per-substrate `if`/`then` target/peer sub-schemas with + `additionalProperties: false`, and the three independent scopes + (`tunnel:open`/`tunnel:direct`/`tunnel:forwarded`) — each proven + never-implied by wire-level tests in both directions + (`producer_direct_op.rs` `tunnel_open_scope_does_not_imply_*`, + `producer_forwarded.rs::forwarded_scope_gate_is_independent_grant`). + The `listen` token's positivity is enforced at three layers + (schema `minimum: 1`, the `positive_u32` serde pin, the reply + projection's `> 0` filter) with the mint never yielding 0 + (`mint_token` seeded at 1, wrap-skip pinned by test). The reply + projection's asymmetry is deliberate and tested both ways: + `parse_listen_reply` is the loud typed viewer (malformed → + `MalformedReply`), `listen_reply_fields` the lenient session-surface + projection (malformed → both-`None` + loud log) — the doc comments + on both correctly state which to use when, and the additive posture + (unknown reply fields tolerated) is pinned by test. + +2. **Producer conformance — PASS.** The bind-first establisher is the + awaited bounded establishment phase (registry gate → mint → bind → + reply fields → `Establishment::new(plan)`), the plan is + typed-opaque (`BindFirstPlan` downcast test), and the accept + dispatcher is a faithful ADR-008 §1 implementation: `accept_bi` + FIRST (the R-02 accepted-flag telemetry — verified in upstream + `operations.rs:954-1000`, the wrapper's birth-teardown hint keys on + it), the idle-stream EOF watch races the accept loop in `select!`, + per-accept forwarded opens are isolated (drop the accept, keep + serving — pinned by two tests), loop-critical accept errors end the + loop, `drop(plan)` tears the listener down, and the handler's + `JoinHandle` awaits every per-accept pump (R-02 — no forwarded pump + outlives its listener). The forwarded-op serving handler parses + params (deny-unknown + positive-listen serde pin) and accepts the + `BiStream`, awaiting the injected handler's `JoinHandle` inline — + a malformed open tears down at birth with no phantom channel + (pinned: `forwarded_malformed_params_tear_down_loud`). + `register_openable` (no establisher) for the forwarded op matches + ADR-008 §3's no-establishment shape. + +3. **Consumer conformance — PASS.** `TunnelSession::open_direct` is the + separate constructor ADR-007 §5 pins (not a params enum — two + scopes, the caller states which it holds); the session type, data + planes, and teardown API are identical to `open`. `TunnelListener` + holds the write half IDLE (never exposed, never pumped), projects + the reply fields via the lenient path, and owns teardown + (`close` = graceful `shutdown` → EOF sentinel → reap; `Drop` = + implicit-EOF chunk → reap — the mux's implicit-EOF path is real, + verified in upstream `mux.rs:67-82`). No `Clone` compile-asserted + for both session types. `adopt` gains nothing — pinned by test at + both the standalone and through-the-real-forwarded-registration + levels. + +4. **Conventions sweep — PASS with notes.** No `unwrap`/`expect`/ + panics outside tests (the `unreachable!` arms in + `take_halves`/`pump_against` are unreachable by the documented + data-plane-XOR-pump invariant and are the pre-existing v1 shape). + No substrate types outside `src/local/` (grep-verified). No + comments beyond doc comments (the inline `//` density in + `producer.rs`/`local/mod.rs` is high but they are documenting + ADR-pinned contracts, which AGENTS.md convention 1's exception + covers — correctness constraints that would otherwise be missed). + The wasm-clean invariant holds (`cargo check + clippy` on + wasm32-unknown-unknown pass; `tokio` features unchanged; no new + non-local deps). Feature-gate verification: both `cargo test` + (default) and `cargo test --features local` pass. + +5. **Conformance to the amended ADR-008 — PASS.** Amendment 1's + producer-minted token is implemented exactly as pinned: minted at + establisher time (non-zero-seeded `AtomicU32`, per-open fresh, + per-producer unique, wrap-skip of the illegal 0), carried twice + (plan + `listen` reply field), stamped per-dispatcher from its own + plan (pinned by the two-open token-discipline test), and consumed + by the opener-side correlation (assembly-side bookkeeping — the + crate provides the op + params, not the wait state machine). + +--- + +# Findings + +## M-1 [minor] — The accept dispatcher's substrate fallback is silent and wrong-shaped + +`src/producer.rs:766-768` +(`make_bindfirst_accept_handler`): + +```rust +let substrate = serde_json::from_value::(input.clone()) + .map(|p| p.substrate) + .unwrap_or(Substrate::Tcp); +``` + +The listen open's input params have already passed the registry's +schema gate (enum-validated) **and** the establisher's +`parse_params` (which rejects unparseable input with +`handler_error`), so in the wired path the fallback is unreachable. +But the fallback shape itself is the problem: if the parse ever fails +(a registry whose schema gate is looser, an assembly variant, future +drift), the dispatcher silently stamps `Tcp` into every forwarded +open — for a UDP listen tunnel that is a raw-vs-framed data-plane +mismatch on the *opener* side, the exact invariant class review 001 +U-1 was about, and it degrades silently instead of loudly. + +The same silent-fallback shape exists in no other handler: the +forwarded-op handler (`make_forwarded_op_handler`, +`src/producer.rs:993-1002`) parses and **returns** on failure +(teardown-at-birth, the correct loud shape); the pump handler +downcasts the plan and returns on failure. + +**Suggested fix (small, mechanical):** on parse failure, log loud and +return (teardown-at-birth — the channel reaps, the far listener dies +with the plan, the same posture as every other early-return in this +file). Alternatively, since the establisher already parsed the params +successfully, stamp the substrate into `BindFirstPlan` at establisher +time and drop the second parse entirely — one parse, one source of +truth, no fallback branch at all. The second shape is strictly better +(the plan is already the per-open carrier; the substrate is +per-open-static) but changes a public-ish struct. + +**Why not fixed inline:** the choice between "log-and-return" and +"stamp the plan" is a small API-shape decision (`BindFirstPlan` +gains a field — a plan-payload change, cheap now, pre-consumer) that +the next session should make deliberately, and both need a +failure-path test. + +## D-1 [major-as-labeled, doc-only] — wire.md omits the `port` bounds the shipped schema enforces + +The shipped direct-op and forwarded-op schemas pin +`"port": {"minimum": 0, "maximum": 65535}` plus +`"additionalProperties": false` on the target/peer objects +(`src/params.rs:208-222`, `453-467`), and `listen` with +`"minimum": 1` (`src/params.rs:442`). wire.md (the normative WHAT) +says only `port: ` and "integer > 0" in prose — it does not +record the bounds as schema properties, nor the +`additionalProperties: false` gate that makes cross-shape targets +fail the registry's `INVALID_INPUT` class *before* the establisher +(the behavior `malformed_target_is_schema_class_and_leaves_no_channel` +pins on the wire). wire.md is the wire-stable contract doc: the +schema details are part of the one-way door (ADR-007 §"Wire-stable": +"the target sub-schema" is listed as wire-stable). The BAST doc +(`bast.md`) covers only the data-plane codec, not the params schemas, +so there is no second source recording these bounds. + +**Impact:** none at runtime (the code is the gate); the risk is a +future consumer implementing from wire.md alone and accepting a +shape the reference producer rejects (or vice versa). Sync the doc +before the first consumer (alksocks) ships. + +## D-2 [major-as-labeled, doc-only] — wire.md's `if`/`then` structure differs textually from ADR-007's `oneOf` + +ADR-007 §2 says "the target sub-schema is per-substrate (`oneOf` on +the sibling `substrate` value, validated at establisher time — see +Consequences)"; the shipped schema (and wire.md §The Forwarded Op's +structure, and the unit tests pinning it) use **`allOf` of two +`if`/`then` branches** instead. `oneOf` of full-target-schemas would +actually be *wrong* here (a `unix`-substrate open would need to fail +the `tcp` branch — plain `oneOf` on sibling-conditioned subschemas is +awkward; `if`/`then` composes correctly with the base +`properties`/`required`). The code is right; ADR-007's one-word +schema-structure reference is stale. One-line ADR erratum (or an +amendment note), plus ADR-008 §3's "validated at establisher time" +(ADR-007 §2's parenthetical) vs the shipped registry-side gate: the +establisher-time sentence in ADR-007 §2 line 82 ("validated at +establisher time") contradicts both the shipped design and the +wire.md text ("validated at establisher time" appears in +wire.md §The Direct Op but the test proves the registry gate fires +first — `malformed_target_is_schema_class_and_leaves_no_channel` +asserts the establisher's dial never runs). The ADR/wire.md sentence +should read "validated registry-side (the schema gate), before the +establisher; re-checked by serde at establisher time as the +backstop" — which is exactly what `src/params.rs`'s doc comments +already say. + +## D-3 [minor] — `jsonschema_valid` in `src/params.rs` tests is a stub, not a validator + +`src/params.rs:628-637`: `jsonschema_valid` parses the value with +serde and pattern-matches the substrate string — it validates the +serde layer, not the JSON Schema. Both call-sites' +names (`schema_rejects_unknown_substrate_value`, +`direct_target_schema_gates_shape_per_substrate` — the latter +correctly does *structural* schema assertions instead and is honest +about it) overpromise for the former: `schema_rejects_unknown_ +substrate_value` does not execute the spec's `input_schema` at all. +The registry-side gate IS exercised end-to-end by the integration +tests (`schema_rejects_unknown_substrate_loudly`, +`malformed_target_is_schema_class…`, the forwarded `listen: 0` gate), +so coverage is not lost — but the unit test's name promises more +than it delivers. Either rename it (e.g. +`serde_rejects_unknown_substrate_value`) or drop it in favor of the +integration probes. alkcall already compiles the real validators at +registration; a `jsonschema` dev-dependency purely for this unit +test is not warranted. + +## D-4 [minor] — CHANGELOG "Unreleased" preamble is stale + +`CHANGELOG.md`'s Unreleased section still opens with "implementation +is decomposed in `tasks/tunnels/` and lands in the next release… +the only behavior-affecting change so far (no wire or API change at +this level yet)" — written before the six implementation commits. +The section below it correctly lists all six features. The preamble +contradicts the section it introduces. Trim the preamble to a +one-liner (or drop it) before release. + +## N-1 [note] — `parse_listen_reply`'s malformed-`listen` message vs the serde pin's semantics + +`parse_listen_reply` requires `listen` to be "a positive u32" and +rejects 0 (`src/params.rs:329-339`) — matching the forwarded params' +wire contract (`minimum: 1`). Consistent and intentional, but note +the asymmetry with `bound`: a `bound: null` and a `listen: null` are +both treated as absent (`None | Some(Value::Null) => None`) — fine, +JSON-null-as-absent is the standard lenient reading, and the loud +path still fires for present-but-malformed shapes. No action; noting +the deliberate shapes for the record since `TunnelListenReply` +carries `deny_unknown_fields` while the *parse function* tolerates +unknown fields — the struct-level attribute is only load-bearing if +someone deserializes the struct directly from a full reply, which the +doc comment explicitly warns against ("Parse the full reply with +`parse_listen_reply`, never with `deny_unknown_fields` +deserialization of the whole reply"). The warning is doing real +work; keep both. + +## N-2 [note] — The idle-stream EOF watch treats ANY byte as teardown + +`make_bindfirst_accept_handler`'s `eof_watch` (`src/producer.rs:781-794`) +triggers teardown on `Ok(0)`, `Err(_)`, **and** `Ok(1..)` — a +misbehaving opener that writes to the idle listen channel also tears +the listener down. The doc comment owns this ("a peer that writes is +misbehaving; treat any non-empty read as an EOF trigger too"), and it +is the safe direction (fail closed — the channel is not a data +plane). Verified upstream that a `MpscSendStream` drop without +`shutdown` produces the implicit-EOF chunk (the `Ok(0)` path) and a +`shutdown` produces the sentinel — both arrive as `Ok(0)` at the far +reader, so both `TunnelListener::close` (explicit shutdown) and +`Drop` (implicit EOF) trigger the far teardown. Correct. No action; +recorded because a reviewer reading only ADR-008 §1 ("its EOF +cancels the loop") might implement the narrower `Ok(0)`-only check +and hang on a write-happy peer — the code is right, the ADR sentence +is thinner than the implementation. + +## N-3 [note] — `make_forwarded_op_handler` parses params BEFORE `accept_bi` + +`src/producer.rs:993-1006`: the malformed-params early return happens +before `conn.accept_bi()`, so the R-02 accepted-flag telemetry reads +`false` for a malformed-params open (the wrapper logs the +birth-teardown hint). This is the deliberate reading of R-02 — the +flag is telemetry for the "handler exited without accepting" *hint*, +and a malformed-params open IS a birth-teardown shape — but the +ordering means the hint fires for a handled-rejection case. Cosmetic +telemetry noise at worst (a `tracing::debug!`); the test suite pins +the teardown behavior, which is what matters. The alternative (accept +first, then parse) would hold a stream open for a params set the +producer already got wrong — the current order is the better one. +Documented here so a future alkcall reviewer doesn't flag the flag. + +## N-4 [note] — `SubstrateAddr` untagged leniency is schema-gated, and that's the right call (but it is a latent sharp edge) + +The untagged `SubstrateAddr` accepts extra fields (untagged variants +cannot `deny_unknown_fields`), so a cross-shape target like +`{"path": "/p", "host": "h"}` parses as the `Path` variant at serde +level. Both the direct-op and forwarded-op tests pin this asymmetry +with the same rationale: the SCHEMA is the shape-vs-substrate gate +(`additionalProperties: false` per branch). That holds for the +registry-dispatched path. Two things to keep in view: + +- The establisher-level backstop (the serde parse inside + `direct_establisher`) keys `render_target` off the sibling + `substrate` — a cross-shape target that sneaked past a looser + registry would render *something* (`/p` for the unix case) rather + than fail. Acceptable (garbage-in handled by the schema's absence), + but worth an ADR-line if a future consumer runs a schema-less + registry variant. +- If `SubstrateAddr` grows a variant whose shapes overlap + (e.g. a future `{host, port, flowlabel}`), untagged disambiguation + starts ordering — at that point switch to + `#[serde(untagged)]`-with-content or an internally-tagged form. + Pre-consumer, additive; just don't let it happen by accident. + +## N-5 [note] — The token-mint discipline is sound; the concurrent-mint shape is untestable at this layer and fine + +`mint_token` is a relaxed-ordering `fetch_add` loop skipping 0. The +"2^32 concurrent opens" wrap-collision math in the doc comment is +right (and the collision consequence — an opener correlating a +forwarded open to the wrong listener — requires the same producer to +have 2^32 *live* listeners simultaneously, impossible under the +256-channel per-connection cap; the token is per-producer-connection, +and each open rides one connection). Worth noting the cap makes the +wrap analysis even more favorable than the comment states. No action. + +## N-6 [note] — Test-only race window in `bindfirst_open_surfaces_reply_fields_and_teardown_cascades` + +`tests/consumer_listener.rs:276` (and several sibling tests) settle +the forwarded open with `tokio::time::sleep(300ms)` then poll +`channel_ids()`. Sleep-then-assert is the established house pattern +in this suite (the harness has no push-based settle surface), but +these are the tests most likely to flake on a loaded CI box (the +forwarded open traverses: producer accept → call plane → opener +dispatch → registry → wrapper → handler spawn). The suite has been +3x-repeat stable historically (review 001's methodology); if CI ever +shows a flake here, the fix is a bounded poll loop on +`channel_ids()` (the teardown assertions already use that shape — +extend it to the arrivals). No action now. + +## N-7 [note] — `test_bind()`'s listener is a bare `Arc` + +The test bind closures in `producer_bindfirst.rs` / +`consumer_listener.rs` put `Arc::new(4242u32)` in the plan's opaque +listener slot. Fine for tests that feed accepts via side channels — +and the mismatch probe (`accept_pair_loop_downcast_miss_fails_loud`) +proves the real dispatch path fails loud on exactly this shape — +but if a test ever wired `local_bindfirst_accept_loop()` against +`test_bind()`, the failure would surface as the generic +`handler_error`, not a test bug message. The tests consistently use +the queue loops instead. No action; the downcast-miss test covers +the failure mode. + +--- + +# What was checked and found conformant (highlights) + +- **The token path end-to-end** (Amendment 1): mint (seed-1, wrap-skip) + → reply field (reserved-key guard upstream keeps `channel_id` + wrapper-owned — verified in `operations.rs:927-946`) → lenient + projection → forwarded-param keying → opener-side correlation → + per-dispatcher own-plan stamping (two-open test) → hub-relay + pass-through (params untouched, reply fields per-hop-truthful — + upstream ADR-051 machinery, source-verified in the relay docs). +- **The teardown cascades**: listener drop → implicit-EOF chunk + (upstream `mux.rs` implicit-EOF path verified) → far dispatcher's + idle-read fires → loop breaks → `drop(plan)` (listener dies) → + pumps awaited → handler task exits → wrapper's handler-exit + teardown reaps the serving-side entry. Both directions (close's + explicit `shutdown` sentinel and Drop's implicit path) tested; the + leak-free guarantee asserted via `channel_ids()` on both managers. +- **Per-accept isolation**: forwarded-open failure (FORBIDDEN / + NOT_FOUND / correlation miss) drops only that accept; the loop + serves the next (two tests pin queue-drain + no-stray-channels). +- **Scope separation** proven structurally at the wire level in all + three pairwise directions, at both the raw-call and typed-session + layers. +- **CF-005/CF-006 identity surfaces**: the establisher witnesses the + END CALLER's identity on base/direct/bind-first paths (witness + wrapper for bind-first, inline for the others); the forwarded op's + opener-side gate sees the producer's per-call identity (the + upstream per-call overlay — tested via the scope gate). +- **Schema-gate ordering**: ACL → input_schema → establisher → + handler, verified in upstream `registration.rs` (all three dispatch + entry points) + pinned by the tests that assert the establisher's + dial never runs on a malformed target. +- **Doc-test parity**: both `compile_fail` asserts (no-Clone) live + and green. + +--- + +# Remediation + +Ordered smallest-first; nothing here blocks alksocks composition. + +1. **M-1** — the accept dispatcher's substrate fallback (log-and-return, + or stamp the substrate into `BindFirstPlan`). Small; needs a + failure-path test. +2. **D-3, D-4** — rename/strengthen the unit-test helper; trim the + CHANGELOG preamble. Trivial. +3. **D-1, D-2** — sync wire.md's schema bounds + ADR-007's + oneOf→if/then wording + the "establisher-time" → + "registry-side-gate, serde-backstop" sentence. Doc-only but + wire-contract-adjacent: do it before the first consumer ships. + +--- + +# Coverage review (appendix — `cargo-llvm-cov` 0.8.4, line/region mode) + +Measured with `cargo llvm-cov` (default) and `cargo llvm-cov +--features local`; branch-level instrumentation needs a nightly +toolchain (`-Z coverage-options=branch`), so these are line/region +numbers — branch coverage is analyzed manually below where it matters. + +## Headline numbers + +| Configuration | Line coverage | Note | +|---|---|---| +| default | **95.63%** (74 missed of 1692) | the wire/protocol surface | +| `--features local` | **93.66%** (133 missed of 2098) | adds `src/local/mod.rs` (84.0% lines) | + +By file (default / `local`): `error.rs` 100/100, `params.rs` +100/100, `wire.rs` 96.0/96.0, `producer.rs` 93.8/94.7, +`consumer.rs` 90.5/90.5, `local/mod.rs` —/84.0. + +The numbers are healthy for this suite's design (the integration tests +drive the real paths over the harness duplex and — with `local` — real +sockets); what remains uncovered is overwhelmingly **defensive +early-return arms that no wired path can reach** plus a handful of +genuine, cheap-to-close gaps. Classification follows. + +## Genuine gaps (worth closing) + +### G-1 — `TunnelSession::open`/`open_direct`/`TunnelListener::open` params-serialize error closures +`src/consumer.rs:94-97, 134-137, 487-490` (and +`open_reverse_channel`'s twin at `src/consumer.rs:593-594`). All four +are `serde_json::to_value(..).map_err(..)` arms over types with no +non-string keys or fallible serializers — unreachable in practice, and +the established house shape for this conversion. Closing these needs a +test-only serializer poison (`serde_json` can't fail on these types +without `try_collect` tricks). **Recommendation: leave uncovered; do +not chase.** If the suite ever grows a shared "unreachable error arm" +prose note, these four are its members. (Alternative: collapse the four +duplicated arms into one `fn params_to_input(..)` helper +in `consumer.rs` — then only ONE closure exists and the gap shrinks by +3. Cosmetic; fine to skip.) + +### G-2 — `TunnelListener::close`'s `shutdown` arm never observed at the far end +`src/consumer.rs:529-531`: `close()` runs +`write.shutdown().await` (the explicit EOF sentinel) then reaps — but +no test currently observes the FAR accept dispatcher's idle-read +firing from a `close()` (the graceful path). The Drop path (implicit +EOF → far teardown) IS observed end-to-end +(`bindfirst_open_surfaces_reply_fields_and_teardown_cascades`), and +the close path is pinned at the reap level +(`close_and_drop_reap_the_listen_channel`) — but the sentinel's +far-side effect for `close()` specifically is asserted nowhere: the +"the pushed accept was never consumed" assertion runs only in the +Drop-shaped tests. **Recommendation: one small test** — open against a +bind-first producer, push an accept, `listener.close().await`, then +poll that the far accept loop is gone (the same queue-len assertion +the Drop tests use). This is the only gap that touches a *promised* +behavior (ADR-008 §1's teardown contract, graceful half) with no +direct observation. Cheap: mirror +`bindfirst_full_flow…`'s harness with a `close()` instead of a drop. + +### G-3 — `TunnelSession::join`'s pump-`Err` arm (`src/consumer.rs:368-373`) +The `JoinHandle` error path (pump aborted → `JoinError` → warn + `(0, +0)`) never fires in tests: `close()`/`Drop` abort the pump and reap +without joining; `join()` is only ever exercised on live pumps. A +one-line test: open a session, `pump_against`, `close().await` (aborts +the pump), then… `join` consumes self — the shape needs a +`pump_against` session dropped into a spawn that is aborted before +`join`. Practical shape: `let s = session.pump_against(dup).await;` +take the pump out via `s.join()` racing a `s.abort()`-equivalent — +there is no public abort on the session, so the test should close the +channel out-of-band (`manager.teardown_channel`) while `join()` is +pending: the pump's copies error/EOF, `pump.await` still resolves Ok — +actually the JoinError arm fires only on abort/cancel, which the +session type never does before `join`. **Recommendation: leave the arm +as-is (defensive, log-only) but note it in the review as +unreachable-by-public-API** — the only way to hit it is a pump +panicking, which is out of contract. Alternatively add a +`#[cfg(test)]`-only unit test driving `join` on a session whose pump +handle was manually aborted via a test seam — not worth the surface +pollution on a warn-log arm. + +### G-4 — `local/mod.rs` `UnixListenerHalves` accept-first loop never driven (591-609) + `TcpListenerHalves::accept_pair` downcast path in the *queue*-shaped tests +`UnixListenerHalves::accept_loop` (src/local/mod.rs:591-607) has zero +coverage: no test constructs it (the unix dial test hand-rolls its own +echo listener; the bind-first unix test uses `accept_pair`). It is the +unix parity twin of the covered `TcpListenerHalves::accept_loop` +(hit at line 107-125) and its body is 15 lines of the same shape. +**Recommendation: one small unix listen-producer test** (mirror +`producer_listen.rs::accept_loop_stops_feeding_after_close_and_maps_the_drop` +with `bind_unix` + a temp path) — closes the entire 593-607 range and +exercises `UnixListenerHalves` beyond the bind-first path. Low value +(copy-shaped), but it is the largest single uncovered block in the +crate. + +### G-5 — The unnamed-peer error arms (unix) +`src/local/mod.rs:626-633` (the `accept_pair` unnamed-peer +`handler_error`) and `src/local/mod.rs:578-583` (the +`local_addr` unnamed-bind error) never fire: the unix bind-first test +binds a named client socket deliberately (the parity case), and no +test binds an abstract/autobind unix listener. Both are +fail-loud guards the ADR pins (ADR-008 §3 — "a producer that cannot +determine the peer does not serve a bind-first listener over that +substrate"). **Recommendation: one compact test** — bind an abstract +unix listener (Linux: a leading-`\0` path via socket2, or +`UnixListener::bind` on an unnamed autobind socket), call +`local_addr()` (expect the "no filesystem path" `handler_error`), and +drive one autobind-client accept through `accept_pair` expecting the +unnamed-peer error. Cheap, deterministic, and it pins the two ADR +sentences the doc comments promise. + +### G-6 — UDP bind-family resolve edge (`src/local/mod.rs:180-181`) +`connect_udp`'s `no addresses` arm (the resolver returning an empty +list). `connect_udp_unresolvable_target_maps_to_dial_failed` covers +the `to_socket_addrs` Err arm; the empty-success arm needs a +resolvable-but-zero-address name, which is not deterministically +producible with std resolution. **Recommendation: leave uncovered** +(documented as the std resolver's unreachable-in-practice arm) — or +refactor `connect_udp` to take the resolution behind a small +`ResolveFn` seam so the empty case is unit-testable. The seam is the +better long-term shape (it also removes the blocking +`to_socket_addrs` call from the async context — see N-8 below), so if +this gap is addressed, do it via the seam. + +### G-7 — `UdpHalf` `poll_read`/`poll_flush` readiness-error arms +(`src/local/mod.rs:369-377, 395-396, 422-423, 427, 520-521`): the +`poll_recv_ready`/`poll_send_ready` `Ready(Err)` arms and the +high-water `Pending`-after-drain arm. The veth sender-block test +(`udp_half_write_side_blocks_stashes_and_drains_on_resolve`) covers +the stash/queue/resolve spine (lines 505-528 mostly hit; 519-521's +`poll_send_ready` Pending arm never resolves Pending inside the test +window because the veth's EAGAIN pattern resolves readiness between +polls). These arms are one-poll-window states — deterministic capture +needs a mocked readiness source, not a socket. **Recommendation: +leave; the veth test already pins the observable behavior (stash → +retry → deliver), and the arms are unreachable differently per-kernel +(host-notes in the test document this).** + +## Deliberately uncovered (defensive; no action) + +- **Handler early-return arms** (`src/producer.rs:446-455, 759-764, + 1004-1005`): the `accept_bi`-failed / wrong-plan-payload / + `try_unwrap`-failed telemetry returns. A wired open always has a + plan of the right type; these fire only on a protocol bug or a + mismatched registration (assembly error, not wire state). Covered in + spirit by the unit-level downcast tests. To hit them, a test would + need a handler registered against a plan the establisher never + produced — an upstream-invariant violation, not a code path. +- **`warn!`/`error!` telemetry bodies** (`src/producer.rs:825, 833, + 838-849`; `src/consumer.rs:368-373`): per-accept forwarded-open + failure logging (the FORBIDDEN isolation test hits the *drop* + branch's effects but the tracing body's region shows missed because + the macro expands past the branch) and the loop-critical accept + error arm — wait: **the loop-critical arm IS behaviorally covered** + (`accept-loop-ended` via the queue_loop's closed-listener Err in + the teardown-cascade tests); the missed lines 838-849 are the + second `tracing::warn!` expansion inside that arm. Logging regions + in macro expansion show as missed lines while the branch executes. + Verified: `listen_channel_eof_cancels_loop_and_tears_listener_down` + drives a loop-critical accept error. No action. +- **`unreachable!` arms** (`src/wire.rs:113`, + `src/consumer.rs:248, 328, 333`): guarded by invariants + (`current` set before the `else` branch; data-plane XOR pump; the + spent-session double-take the type makes impossible). Standard + posture; the `debug_assert`-shaped gaps review 001 already passed. +- **`wire.rs:184`** — the `datagram_split_across_awkward_chunks_reassembles` + test's failure panic (the test always passes the reassembly). +- **`Default for DatagramReader`** (`src/wire.rs:82-84`): derive-shaped + boilerplate; all tests construct via `new()`. Trivial to close if + wanted (`let _: DatagramReader = Default::default();` in an existing + codec test) — otherwise delete the impl in favor of `#[derive(Default)]`. +- **`src/params.rs` 99.3%** — the two "missed regions" are the + `serde_json::from_value` error closures inside + `parse_listen_reply`'s bound/listen arms; the `listen_reply_rejects_malformed_shapes` + unit test exercises both paths (the macro-expansion region + accounting again). Line-level: 100%. No gap. +- **`Debug` impls** (`src/producer.rs:66, 109`): `TargetHandle`'s and + `BindFirstPlan`'s manual `fmt`. Never invoked in tests (no + `{:?}` on those types). One-line fix if desired: `assert_ne!` two + plans in the token-discipline unit test (forces `Debug`), or drop + the manual impls for `#[derive(Debug)]` with the opaque fields + skipped (`#[derive(Debug)]` on `Arc` prints the pointer — + the manual impl exists to keep the listener opaque). Keep the impls; + optionally add a `debug_fmt_smoke` unit test. +- **`witness_establisher`'s `None` arm** (`src/producer.rs:917`): the + registration wrapper's `None => establisher` branch — every test + registers with `Some(witness)`. One test registering + `register_tunnel_bindfirst_openable` with `identity_witness: None` + would cover it; trivial. + +## Branch-coverage notes (manual, no nightly needed here) + +The instrumented run is line/region only (stable; branch mode requires +`-Z coverage-options=branch`). The manually-reviewed decision points +and their test-pinned states: + +- `AcceptQueue::pop`'s three arms — all covered (`producer_listen.rs`: + empty-then-push, closed-and-empty, the lost-wakeup window test). +- `UdpHalf::poll_read`'s staged-frame / fresh-recv / WouldBlock / error + arms — all covered except the `poll_recv_ready` Err/Pending arms + (G-7). +- `UdpHalf::poll_write`'s high-water arm — covered by the veth test + (Pending observed); the inner `Poll::Ready(Err)` return (422-423) + uncovered (G-7's class). +- `pump_queue_to_socket`'s WouldBlock/ENOBUFS stash arm — covered + (veth test); the `poll_send_ready` triple (519-521) uncovered except + via the flush-driven path (G-7). +- `parse_listen_reply`'s malformed arms — covered (unit test); + line-level 100%. +- The idle-read `Ok(_)` arm (`src/producer.rs:792`) — same body as the + `Ok(0) | Err(_)` arm; llvm-cov counts the second match arm's + region separately. The write-happy-peer teardown shape is + documented (N-2 in the main review) but untested — a test would + write a byte to the idle channel and assert far teardown. Same + harness shape as the EOF tests; cheap to add with G-2 if desired + (it pins the "any non-empty read tears down" posture). + +## Verdict + +No coverage *gap* hides a correctness risk in the shipped paths: the +uncovered set is defensive arms + two doc-promised fail-loud behaviors +(G-5) + one ADR-contract half (G-2). The suite's integration-first +design (real sockets with `local`, real forwarded-openable +registrations, real EOF sentinels) is why the uncovered residue is +this small. Recommended additions, in order of value: **G-2, G-5, G-4, +G-7's `poll_send_ready` Err arm (skip — kernel-dependent), G-6 via the +ResolveFn seam (optional), G-3 (document-only)**. Combined with M-1's +failure-path test, that is roughly one session's worth of test work. + +## References + +- The six commits: `0789ab6` (direct op), `d586a5d` (forwarded params + + plan type), `7d23a89` (bind-first establisher + accept + dispatcher), `3fb26d9` (forwarded serving + reply projection), + `514e0c1` (local bind-first halves), `474844e` (`TunnelListener`) +- ADR-007, ADR-008 (as amended), wire.md, consumer.md, producer.md +- alkcall 0.8.0 surfaces this work rides: `open_channel_with_reply`, + `Establishment::with_reply_field` + the reserved-key guard, + flavor-form op ids (ADR-047 amendment 3), `pump_bidi` (ADR-050), + the registry `input_schema` gate, the R-02 accepted-flag telemetry + + birth-teardown hint +- Review 001 (the prior review; U-1's framed-adapter fix is + re-verified here by the direct-op UDP round-trip test) \ No newline at end of file