alktunnels/docs/reviews/001-implementation-review.md

Review 001 — alktunnels v1 Implementation (spec conformance, pre-publish gate)

Status

Implemented-conformant with findings (U-1 [major] implementation gap; C-1 [major] fixed in-review; minors logged, N-2 retracted). Findings filed from the tunnels/review-impl phase-gate review (2026-09-08), which subsumes the earlier tunnels/review-core-crates mid-phase pass (also filed from this same review — see the task Notes; no separate doc was warranted at the mid-phase point, and the two checklists are consolidated here per the review-impl deliverable).

U-1 is load-bearing for the UDP wire promise and is NOT yet fixed — see the remediation section (it owns the remaining remediation task). FIXED 2026-09-08 — see §Remediation resolution below. C-1 (AcceptQueue lost wakeup) is fixed with this review's commit, including its regression test. The trivial minors (N-4, N-5, N-7, N-8, N-9, N-10) are fixed with the same commit — the review's "trivial batch" remediation ordering, applied inline. The forward -L/TCP/unix paths and the wire surface itself (the one-way doors) are conformant as-is. All findings re-validated 2026-09-08 in a follow-up session (fresh executable probes; U-1's fix sketch corrected — see §Re-validation).

Findings use prefix U (units — remediation-work findings) and N (notes/trivia). This is the first review in this crate's series (numbering follows the alkcall house pattern — each review numbers independently).

Scope

The full v1 implementation at tree 2efac9b reviewed against the architecture spec set (docs/architecture/: wire.md, producer.md, consumer.md, overview.md, ADR-001..006, bast.md, open-questions.md) plus the AGENTS.md conventions, cross-checked against the alkcall 0.7.0 API surface the crate consumes (/workspace/@alkdev/alkcall, verified in source: operations.rs wrapper + establisher types, client.rs ChannelOpenError, manager.rs adopt/teardown, pump.rs pump_bidi).

Verified against: alktunnels 2efac9b (post end-to-end-suite); alkcall 0.7.0
Reading list: src/{lib,params,wire,producer,consumer,error}.rs,
  src/local/mod.rs, tests/{harness,producer_open_op,producer_listen,
  consumer_session,end_to_end,local_halves}.rs,
  docs/architecture/*.md (incl. decisions/001..006, bast.md),
  tasks/tunnels/*.md (the per-task notes)
Executed: cargo test (62 default / 69 --features local), 3x repeat-run
  clean both configs; clippy -D warnings (all-targets + wasm32); fmt;
  cargo doc --no-deps; cargo check/clippy --target wasm32-unknown-unknown;
  cargo publish --dry-run --allow-dirty (fails — see N-8)
Probes (temporary test file, deleted after the review run): real-socket
  UDP behavior of the `local` adapter — see U-1's evidence

Severity legend

Same scale as the alkcall reviews (004–007):

• [critical] — a decided spec invariant is violated in a way that makes a promised capability unreachable end-to-end; or corrupts data.
• [major] — a core protocol path cannot serve a decided behavior; works only via shapes the spec does not describe; or a wire/ABI promise is not actually delivered by the code.
• [minor] — drift, doc/spec inconsistency, or a missing convenience with no correctness impact.

---

Checklist verdicts (the review-impl five sections)

1. Wire conformance — PASS. Params shape exact (TunnelParams {resource, substrate}, deny_unknown_fields, schema enum ["tcp","udp","unix"], both required — src/params.rs:32-53, 59-92, tests params_round_trip, unknown_fields_rejected, unknown_substrate_rejected, and the wire-level schema_rejects_unknown_substrate_loudly → INVALID_INPUT). Op id channels/tunnel/sub, ALPN marker alk/tunnel, scope tunnel:open all pinned by constants and asserted in open_spec_matches_wire_md. Codec: [len: u16 BE], len=0 legal, Oversize at frame time, no sentinel in the codec — src/wire.rs conforms (and the codec-level split/batch/partial matrix is well tested). Typed errors: all five establishment reasons flow (three proven by test: unknown_resource, dial_failed, resource_shortage; handler_error maps through the same From arm; timeout proven by the hanging-establisher test), plus FORBIDDEN and channel:too_many_channels surfaces verified through the typed client error. The UDP wire framing itself is correct — the failure is at the local substrate boundary (U-1), not in the codec.
2. Producer conformance — PASS. Establisher = the awaited bounded phase (register_openable_with_establisher, timeout: None = the 10s default; per-registration override proven by the Timeout-mode harness registration). Plan flow (R-01): Establishment::new(plan) → wrapper → handler's plan parameter — no side-channel handoff anywhere (grep-verified: the only Mutex<HashMap> is the ResourceRegistry's own lookup table, a resource registry, not a plan handoff; no poll-loop take). Pump handler: pump_bidi awaited inline inside the spawned task (src/producer.rs:321-341), the returned JoinHandle tracks the data plane (R-02 pinned by the pump_handle_tracks_the_data_plane_r02 end-to-end test), no substrate types (grep tokio::net src/ outside src/local/ → empty). Registration post-hoc supported (W2, late_registration_is_visible). Listen variant: same op, no new wire surface, typed-error table proven (resource_shortage / dial_failed / unknown_resource).
3. Consumer conformance — PASS. The full TunnelSession surface per consumer.md: open / adopt (with the substrate parameter — documented task-note divergence from the task sketch) / stream_halves / take_halves / send_datagram / recv_datagram / pump_against / close / join / Drop. Teardown matrix sound: close-with-pump, join copy counts, Drop-with-pump (abort + reap), failed-adopt no-leak, pump-less join (0, 0, reaped) — all pinned in tests with channel_ids() leak asserts on both sides. No Clone (compile_fail doctest + type_assertions parity stub).
4. Conventions sweep — PASS with notes. No unwrap/expect/ panic! outside #[cfg(test)] blocks (grep-verified; the only non-test hits are inside mod tests). thiserror everywhere errors surface. Poisoned locks: all session/queue locks are tokio::sync::Mutex (unpoisoned by design) — the unwrap_or_else(into_inner) rule has no application point; N/A. Substrate types confined to src/local/ (grep-verified) + feature-gated; wasm-clean default crate re-verified (cargo check/clippy --target wasm32-unknown-unknown clean). pump_bidi consumed, never hand-rolled (grep for hand-rolled two-pump loops: none — the only tokio::io::copy calls live in tests). Inline // comments exist in a handful of places (consumer.rs:275, local/mod.rs:183-189,206-207,230-231,253, section banners) — all of them are exactly the safety/correctness-constraint class AGENTS.md convention 1 carves out (the F-2/OQ-TN-13 invariants, the poll-readiness waker subtleties); verdict: compliant, no action.
5. Docs ↔ implementation sync — PASS with drift (N-1, N-7). lib.rs re-exports = the public API (convention 16); the module map in overview.md matches the files; ADR statuses all Accepted (confirmed by the implementation — no Draft→Accepted flips needed). Drift items are minors: N-1 (the UdpHalf framing claim vs reality — the U-1 doc face), N-7 (lib doc link warning), N-9 (Cargo.toml readme points at a nonexistent README.md, which also breaks cargo publish --dry-run).

---

Part A — Findings

U-1 [major] — The local UDP adapter is not the FRAMED adapter: ADR-003's substrate-boundary codec is missing from UdpHalf, so UDP tunnels deliver codec-framed bytes to real targets and drop/mangle real-target traffic

Verified: YES, by code trace AND executable probes against real 127.0.0.1 sockets (temporary test file, run then deleted; probe results quoted below).

ADR-003 (Accepted, F-2 mandate) says the UDP framing is "applied at the substrate/pump boundary": "the establisher wraps the dialed UDP socket in the framed adapter BEFORE returning the plan (the plan payload is the adapter, not a raw socket) — the pump handler stays substrate-agnostic". consumer.md and wire.md repeat it ("the codec lives at the substrate/pump boundary"; producer.md §Pump Handler step 4: "the plan payload is the FRAMED adapter").

src/local/mod.rs's UdpHalf doc comment claims exactly this shape: "connect_udp: a connected UdpSocket wrapped in the FRAMED adapter (UdpHalf, ADR-003 — the codec composes at this boundary; the pump never sees UDP specifics)" and "The FRAMED adapter — the codec wraps at this boundary". But UdpHalf's implementation contains no framing at all: poll_write sends the caller's buffer as one raw datagram (try_send(buf)), poll_read yields one raw datagram (try_recv into scratch). It is the boundary-preserving adapter (one datagram per read/write — that part works), but the [len: u16 BE] codec never composes at it.

Meanwhile the session side (TunnelSession with Substrate::Udp) frames and unframes per ADR-003 (send_datagram/recv_datagram over DatagramReader). So the codec exists on exactly one end of the tunnel:

consumer session (framed codec) ⇄ channel stream ⇄ pump_bidi ⇄ UdpHalf (RAW)

Probes over the real local path (all results executable-pinned in the deleted probe run):

1. The target receives codec-framed bytes. send_datagram(b"query") → the real UDP echo server received the datagram 00 05 71 75 65 72 79 — i.e. [len=5]"query", the codec frame, not the payload. Every real UDP server behind a v1 tunnel sees tunnel-internal framing on every datagram. The F-2 invariant ("an empty datagram is two bytes — unambiguous with EOF") is only coherent if the framing is confined to the tunnel; leaking it to the target breaks the abstraction the ADR mandates.
2. Empty datagrams are EOF-shaped at the adapter. Reading an empty datagram from a raw UdpHalf returns Ok(()) with zero bytes filled — which tokio::io::copy (i.e. pump_bidi) treats as end-of-stream: the p2c pump finishes and shuts the channel down. Probe: a.send(&[]) to the dialed socket → the session's recv_datagram resolved Ok(None) (EOF) and the producer-side channel stayed alive — the tunnel is torn down (EOF) for what the codec explicitly defines as a legal datagram. This is F-2 violated end-to-end: the length prefix exists precisely so "empty datagram" and "EOF" are different wire shapes, but the raw adapter re-collapses them. The session-side round-trip tests pass only because the harness's framed_udp_echo_dial does the framing in the test (the codec lives at the test's boundary there — a correct-shape stand-in for what connect_udp should have done).
3. Datagrams > ~8 KiB split at the target. tokio::io::copy (the pump's engine) reads with an 8 KiB buffer (tokio DEFAULT_BUF_SIZE); poll_read on a raw socket adapter yields at most one datagram but copy hands the pump at most 8 KiB chunks, and the raw poll_write sends whatever it gets as one datagram. Probe: a 9000-byte datagram sent through the tunnel arrived at the target as two UDP datagrams (8192 + 810 bytes, the second starting mid-payload at 0x9e). On a real (non-loopback) network the split also collides with MTU. ADR-003's chunk-size independence property ("datagram boundaries survive chunk splitting, batching" — the codec's whole point) is broken at this boundary.
4. Target-initiated (unframed) traffic is silently swallowed. The session codec waits for [len] prefixes; a real server's raw datagram (e.g. b"HELLO") parses 0x48 0x45 as a declared length of 17253 and parks mid-frame — probe: recv_datagram never resolved (2s timeout). Asymmetric framing isn't just an interoperability wart — it silently drops peer traffic.

The framing probes also confirmed the parts that are correct: coalescing is safe (two back-to-back send_datagrams landed as two separate UDP datagrams — poll_write sends one buffer per call and the pump's copy preserves the chunk), the scratch-recv truncation check fires InvalidData exactly as documented (OQ-TN-13), and the 1400-byte round-trip test passes (too small to split).

Why [major], not [critical]

The wire format (the one-way door) is correct and unaffected — the codec is right, and the session side is right. What's broken is the v1 local UDP adapter, i.e. a feature-gated convenience module, and the damage is local to UDP-over-local: real UDP targets see corrupted/misframed traffic. It is exactly the "a core protocol path cannot serve a decided behavior — works only via shapes the spec does not describe" class: the spec-described shape (framed adapter at the establisher) does not exist, and the passing tests work only via the test-harness stand-in. Not [critical] because: the default (wasm) crate is unaffected, the wire surface is unaffected, and stream substrates (the SSH -L core case) are fully correct.

The fix (small)

Make UdpHalf the framed adapter ADR-003 already specifies: apply frame_datagram in poll_write (frame the caller's buffer, send one datagram per framed frame) and de-frame in poll_read (feed DatagramReader, emit the next payload). Then:

• len=0 arrives as the 2-byte frame — the AsyncRead zero-fill EOF ambiguity disappears (the F-2 fix works end-to-end);
• datagrams reassemble to their true size before crossing the pump (no 8 KiB split; the MTU discipline returns);
• real-target framing matches the session's (both ends codec-clean).

The local_halves.rs tests will keep passing (their assertions are session-level round-trips) — but note they passed before the fix for the wrong reason (both harness halves framed symmetrically), so the review-remediation test should assert the target-side bytes are payload-only (a recording echo server, like the probe) and that an empty datagram round-trips over the real socket path, not just the codec path.

Remediation task: tasks/tunnels/fix-udp-framed-adapter.md (created with this review).

RESOLUTION (2026-09-08, remediation commit): UdpHalf is the framed adapter — de-frames on poll_write (channel bytes → raw datagrams, via an incremental DatagramReader), frames on poll_read (raw datagrams → channel frames, via frame_datagram). All five verification gates landed as permanent tests in tests/local_halves.rs (the recording-target suite: payload-only at the target, empty round-trip, 9000-byte single datagram, unframed-target resolution, staging-not-truncation at 65507) plus a v6-dial gate for U-2. Two mechanics the gates forced beyond the sketch (pinned in ADR-003's amendment):

1. The write half ACCEPTS ALL offered bytes (reports them consumed) and queues completed datagrams for the socket — tokio copy re-offers a FULL window unsatisfied until drained, so accepting only complete frames deadlocks a >8 KiB frame (its tail sits in a full window forever). Backpressure is an accepted-byte high-water mark; the socket drain pumps on write/flush/shutdown (copy polls flush after every window).
2. A >caller-buffer frame is STAGED across reads (length prefix first) rather than the raw adapter's read-level InvalidData — gate 4's predicted assertion-surface change; the 65507-byte max-datagram round-trip through 16 KiB reads is the new fail-loud probe (silent truncation would still corrupt the framing).

U-2 [minor] — connect_udp binds 0.0.0.0 unconditionally — no v6 or dual-stack posture

src/local/mod.rs:116: UdpSocket::bind(("0.0.0.0", 0)). A UDP target that is only reachable over IPv6 (::1, v6-only host) fails at the connect step with no fallback; a v4-mapped dial works but the bind posture silently excludes v6. TCP/Unix dial have the same blind spot only insofar as TcpStream::connect resolves v6 fine (they do). Minor: a real deployment will notice immediately, the fix is bind on the resolved family of the target (or dual-stack), and no wire/API surface changes. Fold into the U-1 remediation task or a follow-up batch.

Amendment (review follow-up — the bind-posture case study). The follow-up discussion sharpened this finding from "v6 blind spot" into a general bind-posture rule, and it is a good case study in why initial-run reviews exist: hardcoding the bind was a reasonable in-scope choice for the implementing agent (nothing in the task or spec said otherwise), and the fix is now a posture, not a one-off. The rule the remediation implements:

1. No implicit wildcard binds. Neither 0.0.0.0 (v4 unspecified) nor :: (v6 unspecified — note ::1 is LOOPBACK, the v6 127.0.0.1, and is not a wildcard; the risky v6 address is ::). A wildcard bind on a host without a firewall exposes whatever is bound to the open internet.
2. connect_udp's bind is a client socket — bind-ephemeral → connect(target); the kernel delivers only datagrams from the connected peer, so its exposure profile is small. The floor fix is family-correct binding (resolve the target, bind the unspecified address of the matching family), and do NOT "fix" toward loopback defaults (127.0.0.1:0 would break every non-loopback target — a query to 8.8.8.8 cannot route from a loopback-bound socket). No bind-address parameter is added unless source-address selection becomes a real need.
3. bind_tcp (the listener half) already requires the caller to supply the address — that is the correct posture and stays: the assembly layer decides loopback vs a public interface. It gains a doc warning that a wildcard bind is the caller's explicit choice, exposed per the host's firewall.

Recorded here so the posture survives the implementation; the executable rider lives on tunnels/fix-udp-framed-adapter.md §Riders.

RESOLUTION (2026-09-08, remediation commit): connect_udp resolves the target first and binds the UNSPECIFIED address of the matching family (0.0.0.0:0 for v4 targets, [::]:0 for v6 — no implicit wildcard mismatch, no loopback default); no bind-address parameter added. bind_tcp gained the wildcard-exposure doc warning. Pinned by connect_udp_binds_the_matching_family_v6_targets_dial (a real ::1 echo round-trip through the full path).

U-3 [minor] — UdpHalf recv scratch allocates 65535 bytes per read half

src/local/mod.rs:164: recv_scratch: vec![0u8; 65535] — a 64 KiB allocation per dialed UDP tunnel (×2 — both split halves carry the field, though the write half's is empty). Fine for a handful of tunnels; worth a note when the assembly layer fans out (a DNS-proxy shape opens one channel per query). Consider sizing the scratch to the codec's MTU discipline (1400) with an oversized-datagram InvalidData rejection at the scratch level — that also tightens the fail-loud check (a >MTU datagram currently is received then error-matched against the caller buffer). Fold into the U-1 task.

RESOLUTION (2026-09-08, remediation commit): the scratch is allocated LAZILY on the first recv (the re-validation's mitigation — the MTU-shrink idea was unsafe as written; the full-size read is correctness-load-bearing on the de-frame path). The write half carries no scratch. Memory posture: one 64 KiB allocation per receiving UDP tunnel, on first recv.

C-1 [major] — AcceptQueue::pop can miss the final push (lost wakeup in the check→register window) — FIXED in-review

Status: FIXED during the review (commit with this doc).

src/producer.rs:292-305 (pop): the loop drops the state lock, then awaits notify.notified(). A push that lands in that window (a) enqueues under the lock, (b) calls notify_waiters() — which only wakes already-registered waiters — and (c) the pop then registers its notified() future after the notify fired: it sleeps with data in the queue until the next push/close. For a listen producer this is an open op hanging until the wrapper's 10s establishment bound maps it to timeout — a wrong reason code for a queue that had a handle ready. The accept_wait_resolves_when_push_ arrives_late test (200ms sleep before push) rides the race window blind: a fast push would have exposed it.

The fix (applied): the Notified future is created before the state check on every iteration — tokio's documented notify_waiters pattern. The future records the notify_waiters call counter at creation and compares it at its first poll; a push that fired notify_waiters in the thread-preemption window between the check and the first poll resolves the future immediately (captured), and a later push finds the waiter registered and wakes it. The naive shape (future created after the check — the original lock; check; await notified() sequence) loses that push. Verified against tokio 1.53's Notified/poll_notified source (the notify_waiters_calls counter comparison at State::Init). A regression test (pop_wakeup_survives_push_landing_in_the_registration_window, multi-thread flavor, 64-round hammer) rides with the fix — it cannot force the window deterministically (no test can; the window is a scheduling artifact), so the fix's correctness rests on the tokio contract, which the doc comment pins. The FIFO always-before-take property is unaffected (the ordering proof holds under the lock); only the wakeup completeness changes.

Why [major], not [minor]

The queue is public API (AcceptQueue is exported and documented as the listen-shape contract), and the failure mode is a hang → timeout → wrong typed reason on the security-relevant open path. It's rare (microseconds of window) but self-inflicted under normal operation (push racing pop is the queue's entire purpose).

N-1 [minor] — UdpHalf docs claim the codec composes at the boundary; it doesn't (the doc face of U-1)

src/local/mod.rs module doc: "wrap it in the FRAMED adapter (UdpHalf, ADR-003 — the codec composes at this boundary; the pump never sees UDP specifics)". Also connect_udp's doc and the module header ("UDP — connect_udp: ... wrapped in the FRAMED adapter"). Either the code gains the framing (U-1's fix — then the docs become true) or the docs must stop claiming it; both is the resolution. Filed separately from U-1 because even if U-1's fix is deferred, the docs currently describe behavior that does not exist — a conformance review must not leave that standing.

RESOLUTION (2026-09-08, remediation commit): U-1's fix landed — the docs are now true. The module header, connect_udp, and UdpHalf docs describe the composed codec (channel side framed, target side raw) with the ADR-003 amendment's mechanics.

N-2 — NON-FINDING (retracted during the review): DatagramReader buffering is bounded by construction

Filed as a suspect ("a lying len header buffers without bound"), then falsified by executable probe before filing (the probe test tripped on it): the u16 length field caps the decoder's own state by construction. acc never exceeds declared ≤ 65535; buf always drains fully into an in-flight frame (take = min(need, buf.len())) and holds < 2 bytes when no frame is in flight. Total buffered state ≤ 65535 + 1 bytes, by construction — the codec's u16 cap IS the buffering bound, and no BufferExceeded-style error is needed. Recorded here so the next reader doesn't re-derive the suspicion (or "fix" it); the attempted bound+error patch was reverted — the maximal-frame round-trip at 65535 proves the existing decoder is already at the correct bound.

N-3 [minor] — TunnelSession::stream_halves yields the raw halves of a UDP session's taken state confusingly vs take_halves on datagram sessions

take_halves on a datagram session hands out the framed raw halves (documented: "Datagram halves carry the codec framing (ADR-003) — raw bytes on the wire are [len: u16 BE][payload] frames"). That's correct per spec but is a footgun asymmetry: the same session exposes send_datagram/recv_datagram (codec applied) until take_halves, then the caller owns raw framed bytes. The doc covers it; a convenience take_framed_halves that keeps the codec (a DatagramReader-wrapped pair) would close the gap. No v1 blocker; the review notes it for the API-stability window (ADR-005's surface becomes ABI-stable at first external consumer — decide before then).

RESOLUTION (2026-09-08, decided before the first external consumer): option 1 — keep as-is, docs carry it — recorded as ADR-005's §Amendment (2026-09-08, N-3). No external consumer exists (window open, so the breaking reshape was available but undemonstrated); one take_halves semantic across substrates beats per-substrate divergence; the pump_against seam composes the framed ADAPTER (UdpHalf), not framed halves; and the convenience (option 2) is additive — it can land after the first consumer defines its shape without breaking anything. consumer.md's datagram section states the posture.

N-4 [minor] — open_reverse_channel builds the params JSON by hand instead of serializing TunnelParams — FIXED in-review

src/consumer.rs:395-409: the substrate is matched to its string and the payload assembled manually. serde_json::to_value(params) + wrapping in {"operationId", "input"} would remove the duplicated substrate mapping (it exists in three places now: params.rs serialize, open_reverse_channel, and the tests' call_open_params). Zero behavior difference today (deny_unknown_fields + the schema match); it's drift risk if TunnelParams ever grows a field — the hand-built payload silently won't carry it. Trivial cleanup.

Resolution (in-review): open_reverse_channel now serializes TunnelParams (serde_json::to_value) into the envelope's input; the duplicated substrate mapping is gone; a serialize failure maps to ReverseOpenError::Call(internal). All reverse-path tests pass unchanged.

N-5 [minor] — producer.rs module doc stale forward reference — FIXED in-review

src/producer.rs:22-23: "The listen establisher variant ... lands with tunnels/producer-listen." It landed (listen_establisher, AcceptQueue, register_tunnel_listen_openable are all in this file). Stale forward-reference; one-line doc cleanup.

Resolution (in-review): the module doc now points at listen_establisher + AcceptQueue in this file.

N-6 [minor] — TunnelEstablishError has no Timeout variant; the timeout reason exists only wrapper-side

The crate's typed establisher vocabulary (TunnelEstablishError) is UnknownResource / DialFailed / ResourceShortage / HandlerError — Timeout is correctly absent (the bound is the wrapper's, per ADR-049 §2 — an establisher shouldn't self-report it). This is correct as-is; filed so the next reader doesn't "fix" it by adding a variant. No action; recorded as a non-finding with a rationale.

N-7 [minor] — rustdoc: one broken intra-doc link in lib.rs — FIXED in-review

warning: unresolved link to `TunnelParams`
  --> src/lib.rs:12:40

TunnelParams is not re-exported at the crate root (it lives in params::). Either re-export it (it IS the wire type of ADR-001 — consumer-facing users will want it at the root; N-8) or link it qualified ([TunnelParams](crate::params::TunnelParams)). One-line fix; also clears cargo doc --no-deps's only warning (the review gate is a clean doc build).

Resolution (in-review): N-8's re-export landed, which resolves the link; cargo doc --no-deps is warning-free.

N-8 [minor] — Public API surface omits TunnelParams, Substrate, tunnel_open_spec, OP_TUNNEL_OPEN from the crate root — FIXED in-review

lib.rs re-exports session/error/wire/producer names but not the wire types a consumer assembly layer actually needs by name: TunnelParams/Substrate (every open/adopt call takes them), tunnel_open_spec (producer.md: "the open-op spec builder is public so assembly layers can compose variants"), OP_TUNNEL_OPEN. They're reachable at alktunnels::params::* (convention 16 says "public API surface is lib.rs re-exports" — reachable-only is arguably compliant, but the sibling alktty re-exports its params types). Trivial additive re-export; do it before the first external consumer pins the import style.

Resolution (in-review): TunnelParams, Substrate, tunnel_open_spec, and OP_TUNNEL_OPEN re-exported from the crate root (additive — no existing import breaks).

N-9 [minor] — cargo publish --dry-run fails: readme = "README.md" but no README exists — FIXED in-review

Cargo.toml declares readme = "README.md"; the file doesn't exist in the repo (the pre-release gate command in AGENTS.md fails on it — verified). Write the README (it's also what crates.io renders) or drop the manifest field before any publish consideration. This is exactly the class of thing the phase-gate review exists to catch.

Resolution (in-review): README.md written (purpose, the resource model, feature table, verification commands, status); cargo publish --dry-run --allow-dirty now completes through packaging + verification.

N-10 [minor] — Timeout-mode harness arm creates a second ChannelCore without explanation — FIXED in-review (comment added)

tests/harness.rs:222-250 (RegistrationMode::Timeout arm): a fresh ChannelCore is constructed for the hanging-establisher registration where the other arms reuse the outer core. Both wrap the same manager + default_policy(), so behavior is identical (the core is a cheap facade pair). A one-line comment would prevent a future reader from "fixing" the duplication and wondering why the outer core wasn't reused... or worse, from wiring a different policy there someday. Test-only; trivial.

Resolution (in-review): comment added on the arm.

N-11 [observation, pinned] — pump_against on a UDP session pumps raw halves; the framing responsibility is undocumented — FIXED in-review (doc note added)

pump_against takes the data plane (either variant) and pumps the raw BiStream against the accepted local stream — for a UDP session that means the caller must hold the codec responsibility (the end-to-end suite's reverse_udp_datagram_session_round_trips notes this: "No pump_against: the session's codec data plane is driven directly... the pump is the consumer's job here only when it holds local halves"). The API allows a UDP session + pump_against(raw_udp_socket) which would be the raw-pass-through shape ADR-003 bans. It's a caller-contract issue (the doc on pump_against doesn't warn), not a library bug — the session can't know whether the accepted halves are framed. Recommend a doc line on pump_against: "for UDP sessions, the accepted halves must present the same codec framing (ADR-003) — hand the framed adapter, not a raw socket." Fold into the U-1 remediation docs.

Resolution (in-review): the doc note is on pump_against now.

---

Part B — Non-findings (verified correct, recorded to bound the re-review)

• The open-op wire surface is exactly spec: Sub-typed, external, scope ["tunnel:open"] with required_scopes only (the required_scopes_any/resource fields correctly None-shaped), channel_open marker alk/tunnel, description set (round-trips through discovery per OQ-TN-08), input/output schemas matching wire.md byte-for-byte (asserted by open_spec_matches_wire_md).
• The establisher shape matches alkcall 0.7.0's contract exactly: (input, per_call_auth) -> BoxFuture<Result<Establishment, EstablishmentError>>; Establishment::new(plan) with the Send + Sync plan bound satisfied by TargetHandle's boxed + Sync halves (F-1); the establisher correctly does NOT touch the channel Connection (yield-once BiStream stays with the pump handler — ADR-049 amendment 2).
• The pump handler is the R-02 shape, structurally: accept_bi → plan downcast → Arc::try_unwrap → pump_bidi(...).await inline in the spawned task. The try_unwrap failure arm (establisher plan shared elsewhere — cannot happen with this establisher) logs and returns, which is teardown-at-birth telemetry-visible, not a panic. No spawn-and-forget, no hand-rolled two-pump loop anywhere.
• TunnelSession teardown is the ADR-005 matrix: close = abort + reap (returns bool), join = await + reap + counts (c2p, p2c, reaped) with the pump-panic arm logged and count-flattened to (0, 0) (no unwrap on the JoinError), Drop = abort + sync reap, idempotent (Option::take everywhere; an emptied session drops as a no-op; the doc-pinned UnknownChannel on a re-reaped entry is ignored). No Clone (compile-fail doctest).
• adopt's substrate parameter is a documented, reasoned divergence from the task sketch (consumer.rs doc: "the task sketch omitted it — a datagram session cannot exist without it") and is strictly more correct — the reverse-path caller knows the substrate it opened with; requiring it keeps the session constructor total. consumer.md's adopt(hub_manager, channel_id, TUNNEL_ALPN) sketch is superseded by the implementation's (manager, channel_id, substrate, alpn) — the doc's "adopt takes the same typed-error surface" sentence is the normative part and holds. Minor doc sync: update consumer.md's code sketch when the U-1 batch lands.
• open_reverse_channel's payload shape matches the wire: the auth_token rides top-level (alkcall's ADR-017 §7 payload-token posture), operationId/input per the call envelope; the missing- channel_id case is typed (NoChannelId).
• Error vocabulary is upstream-faithful: TunnelEstablishError → EstablishmentError 1:1 (From impl), and the wire reason codes arrive through ChannelOpenError::establishment_reason() — the crate adds no reason strings of its own (the ADR-049 vocabulary is owned upstream, correctly).
• Identity plumbing is end-to-end proven: CF-005 (a) override, (b) transport, token-over-transport (with a distinct provider id so precedence is actually proven), fail-closed FORBIDDEN, and the CF-006 establisher witness on both transport and token paths — the witness seam is a clean test-only hook (public IdentityWitness type is Option-gated, None in production wiring).
• The local feature gate is structurally clean: tokio/net only under local (Cargo.toml feature declaration confirmed), src/local/ is the only tokio::net consumer, the shared modules import nothing from it (local/ depends on producer.rs types, never vice versa), and the wasm target compiles clean with and without clippy.
• The BAST doc matches the codec: [len: u16 BE] struct, maxLength: 65535, "len == 0 is a legal empty datagram, NOT EOF" — and frame_datagram/DatagramReader implement precisely that (the runtime-codec/BAST drift is zero; the U-1 boundary issue lives outside the codec).

---

Verification gates for the U-1 remediation

(All five landed 2026-09-08 as permanent tests in tests/local_halves.rs — see §U-1's resolution note. Gates 1–3 are the gate_* recording-target suite; gate 4's truncation-surface update is truncation_fails_loud_oq_tn_13 (rewritten for the staged frame) + oversized_frame_stages_across_reads; gate 5 is the standing battery.)

1. A real-socket probe test (the review's probe shape, made permanent in tests/local_halves.rs): a recording UDP echo server asserts the datagrams it receives are payload bytes only (no [len] prefix) and an empty datagram round-trips (session recv_datagram yields Some(b""), channel stays alive).
2. A >8 KiB datagram round-trips as ONE datagram at the target (the 8 KiB copy-buffer split is gone).
3. An unframed datagram sent by the target resolves at the session (target-initiated traffic is decoded, not parked).
4. The existing local_halves suite stays green unchanged (its session-level round-trips are framing-symmetric today; they must not regress).
5. AcceptQueue's lost-wakeup regression test (C-1's) stays green; cargo test + --features local + wasm checks as usual.

Remediation ordering

1. tunnels/fix-udp-framed-adapter (U-1 + N-1 + N-11 doc notes + U-2 + U-3 + the gates above) — the only correctness-relevant remediation; blocks any UDP-over-local consumer, not the wire or the API.
2. Trivial batch (N-7, N-8, N-5, N-4, N-9, N-10) — one pass, no behavior changes; N-9 additionally unblocks the publish dry-run. (C-1's fix + regression test landed with this review's commit.)
3. Consider-before-first-consumer (N-3) — an API posture that becomes hard to change once an external consumer pins it; decide in the next phase or file an OQ. (N-2 was in this list and was retracted during the review — see its section.)

Re-validation (2026-09-08, follow-up session, tree f278cf7)

Every finding re-validated before remediation decomposition. Method: full re-read of the touched sources (src/local/mod.rs, src/producer.rs, src/consumer.rs, src/wire.rs, src/lib.rs, tests/harness.rs, tests/local_halves.rs, tests/producer_listen.rs) + fresh executable probes over real 127.0.0.1 UDP sockets (temporary test file, deleted after the run — independent of the original review's probe file) + the verification battery re-run.

• U-1 [major] — CONFIRMED by fresh probes. All four behaviors reproduced independently: (1) send_datagram(b"query") → the real recording target received 00 05 71 75 65 72 79 (the codec frame); (2) an empty datagram from the target → session recv_datagram resolved Ok(None) (EOF collapse — F-2 violated end-to-end); (3) a 9000-byte datagram arrived at the target as two datagrams (8192 + 810); (4) an unframed HELLO from the target never resolved at the session (2s timeout — parked mid-frame). The harness-stand-in explanation also confirmed: framed_udp_echo_dial (tests/harness.rs) frames at the test boundary, and local_halves.rs's byte-for-byte echo server passes the round-trip while the target still sees framed bytes.
• U-2, U-3, N-1 — CONFIRMED by inspection (bind at src/local/mod.rs:116; 65535 scratch at :164; the FRAMED-adapter doc claims in the module header, connect_udp, and UdpHalf).
• C-1 — CONFIRMED FIXED. The Notified-before-check shape is in src/producer.rs AcceptQueue::pop with the contract pinned in the doc comment; the 64-round multi-thread regression test rides in tests/producer_listen.rs (pop_wakeup_survives_push_landing_in_the_registration_window). Test totals are +1 over the review's counts in both configs (63 default / 70 with local) — that regression test.
• Fixed-in-review minors (N-4, N-5, N-7, N-8, N-9, N-10, N-11) — CONFIRMED in-tree (payload serialization in open_reverse_channel; the module-doc pointer; root re-exports incl. TunnelParams; README.md present; the harness comment; the pump_against framing note; cargo doc --no-deps warning-free; cargo publish --dry-run --allow-dirty completes).
• N-2 retraction and N-6 non-finding — CONFIRMED (decoder state bounded by construction: acc ≤ declared ≤ 65535, buf drains into in-flight frames, < 2 bytes idle; no Timeout variant is correct).
• Battery green: 63/70 tests, clippy -D warnings (all-targets + wasm32), fmt, doc, publish dry-run.

U-1 amendment: the fix sketch in §U-1 is direction-inverted

The §U-1 "The fix (small)" sketch reads "apply frame_datagram in poll_write … de-frame in poll_read" — written from the target's perspective. UdpHalf's pump face is the CHANNEL side, which is the framed side (the session's codec; ADR-003; probe 1 proves the framed bytes arrive at the adapter). The correct mechanics:

• poll_write is fed CHANNEL bytes (already [len]payload from the session) → it must de-frame (DatagramReader-style decode) and send each decoded payload as one raw datagram to the target.
• poll_read yields bytes destined for the CHANNEL (the session decodes there) → it must frame (frame_datagram) each raw target datagram and hand the frame bytes to the pump.

The sketch's version would double-frame the channel→target direction and strip framing the target needs in the target→channel direction — probes 1 and 4's defects would survive it. The five verification gates are unaffected (they assert end-to-end behavior); ADR-003's consequence paragraph already pins the right orientation ("len=0 yields a 2-byte wire frame" — on the channel side, where the pump's copy runs).

Two knock-on corrections for the remediation:

1. Gate 4's "stays green unchanged" is too strong for truncation_fails_loud_oq_tn_13 (tests/local_halves.rs): the raw adapter's InvalidData shape changes once framing composes — a

   caller-buffer frame becomes a partial-drain read (or an equivalent typed failure) rather than that exact error at that exact call. The fail-loud posture must hold; the assertion surface may legitimately need updating. Gate 4's intent (no regression in the other local_halves tests) stands.

2. U-3's "size the scratch to 1400" suggestion is unsafe as written: UdpSocket::try_recv truncates SILENTLY at the buffer size — a scratch smaller than the largest datagram would corrupt the framing (a truncated frame decodes as a shorter declared length, desynchronizing the stream) before any size check could reject it. The 65535 scratch is correctness-load-bearing on the de-frame path; mitigate memory (lazy allocation, shared scratch, or a pool) instead of shrinking it. The MTU-scratch idea survives only as an explicit oversize-rejection design with a full-size read underneath it.

References

• The spec set under review: docs/architecture/ (wire.md, producer.md, consumer.md, overview.md, decisions/001..006, bast.md, open-questions.md)
• alkcall 0.7.0 surfaces verified in source: channels/operations.rs (run_open_wrapper, OpenEstablisher, Establishment, EstablishmentError), channels/client.rs (ChannelOpenError), channels/manager.rs (adopt_channel, teardown_channel), channels/pump.rs (pump_bidi)
• ADR-049 review (the establishment surface): alkcall docs/reviews/006-channel-open-establishment-gap-review.md
• The task-level deliverables this review closes: tasks/tunnels/review-core-crates.md, tasks/tunnels/review-impl.md
• The house pattern: alkcall docs/reviews/ (numbering + severity legend)