alktunnels/tasks/tunnels/review-impl.md

id, name, status, depends_on, scope, risk, impact, level, tags

id	name	status	depends_on	scope	risk	impact	level	tags
tunnels/review-impl	Review alktunnels v1 implementation for spec conformance (pre-release gate)	completed	tunnels/end-to-end-suite tunnels/review-core-crates	moderate	low	project	review	review phase-gate

Description

Review the v1 implementation against the architecture spec before any publish consideration. This is the phase-gate review (the SDD process's review-injection point at the end of the critical path): the wire surface (ADR-001/002/003) is a one-way door, and the API surface (ADR-005/006) becomes ABI-stable once a consumer exists.

Review checklist

1. Wire conformance (wire.md + ADR-001/002/003):
   • params shape exact ({resource, substrate}, deny_unknown_fields, schema enum without unix until OQ-TN-14)
   • op id channels/tunnel/sub, ALPN marker alk/tunnel, scope tunnel:open
   • codec: [len: u16 BE], len=0 = legal empty datagram, Oversize at frame time, no sentinel in the codec
   • typed errors: the five establishment reasons + FORBIDDEN + channel:too_many_channels surfaces
2. Producer conformance (producer.md):
   • establisher = the awaited bounded phase; plan flow (R-01), meaning no side-channel handoff anywhere (grep for Mutex<HashMap> + poll-loop take patterns, the POC's dead shape)
   • pump handler: pump_bidi inline, no substrate types, JoinHandle tracks the data plane (R-02)
   • registration: post-hoc supported (W2); ChannelOperations:: register_on guidance correct
   • listen variant: same op, no new wire surface, typed error mapping
3. Consumer conformance (consumer.md + ADR-005):
   • TunnelSession: open/adopt/stream_halves/take_halves/ send_datagram/recv_datagram/pump_against/close/join/Drop
   • teardown matrix sound (no leaks; pump-less join (0, 0, reaped))
   • no Clone
4. Conventions sweep (AGENTS.md):
   • no comments in code (doc comments fine); no unwrap/expect outside tests; thiserror everywhere; poisoned-lock unwrap_or_else(into_inner)
   • substrate types confined to src/local/ (grep-verify: no tokio::net outside the feature gate + tests)
   • wasm-clean default crate (the structural guard passed at every step, but re-verify)
   • pump_bidi consumed, never hand-rolled (grep for hand-rolled two-pump loops)
5. Docs ↔ implementation sync: lib.rs doc comments match overview.md's module map; public API = lib.rs re-exports (convention 16); ADR statuses updated from Draft → Accepted where the implementation confirms them (or findings filed back to the OQ tracker).

Deliverables

Findings as a review doc (docs/reviews/001-implementation-review.md, the alkhttp/alkcall house pattern) with severity levels + concrete remediation tasks. Criticals block the phase; majors get remediation tasks; minors get a follow-up batch.

Acceptance Criteria

• Review doc filed with severity-graded findings
• Remediation tasks created for anything above trivial
• The 5 checklist sections each covered with a verdict

References

• docs/architecture/ (the full spec set)
• House pattern: /workspace/@alkdev/alkcall/docs/reviews/ (the review numbering + severity legend)

Notes

Agent fills during implementation.

Review executed 2026-09-08 (tree 2efac9b); deliverable: docs/reviews/001-implementation-review.md

Method: full source read (src/ + tests/) against the complete spec set; alkcall 0.7.0 API surfaces verified in source (wrapper, establisher types, client errors, manager adopt/teardown, pump_bidi); executable probes over real sockets for the local UDP path (temporary test file, deleted after the run, with the probe evidence quoted in the review); the full verification battery (see below). The mid-phase tunnels/ review-core-crates checklist is subsumed (consolidated in the review doc; that task's Notes carry its verdicts).

The five checklist verdicts

1. Wire conformance: PASS. Params shape exact (deny_unknown_ fields, required both, enum ["tcp","udp","unix"]; the OQ-TN-14 note in this checklist predates the OQ's resolution; unix SHIPS per OQ-TN-14's resolution, and the enum carries it correctly); op id/ALPN/scope pinned; codec correct including len=0 and frame-time Oversize; all five typed establishment reasons reachable and proven (3 by test: unknown_resource/dial_failed/ resource_shortage; handler_error through the same From arm; timeout by the hanging-establisher probe) + FORBIDDEN + channel:too_many_channels.
2. Producer conformance: PASS. Awaited bounded establisher (10s default + per-registration override proven); pure R-01 plan flow (grep-verified: no side-channel handoff, no poll-loop take); pump handler structurally R-02 (pump_bidi inline; JoinHandle tracks the data plane, pinned by test); post-hoc registration (W2) supported; listen variant rides the same op with the typed-error table proven.
3. Consumer conformance: PASS. Full session surface per ADR-005; teardown matrix sound (close/join/Drop/pump-less/failed-adopt, all leak-asserted on both managers); no Clone (compile-fail doctest). One documented divergence: adopt takes the substrate (the task sketch omitted it, since a datagram session cannot exist without it).
4. Conventions sweep: PASS. No unwrap/expect/panic outside #[cfg(test)] (grep-verified); thiserror everywhere; all locks are tokio::sync::Mutex (unpoisoned: the into_inner rule has no application point, N/A); substrate types confined to src/local/
   • feature gate; wasm clean (check + clippy --target wasm32-unknown-unknown, -D warnings both); pump_bidi consumed, never hand-rolled. Inline // comments are the safety-constraint class convention 1 allows.
5. Docs ↔ implementation sync: PASS with drift fixed in-review. Module map matches; ADRs all Accepted (implementation confirms); N-5 (stale doc forward-reference), N-7 (broken doc link), N-8 (missing root re-exports) fixed in-review.

Findings (full detail in the review doc)

• U-1 [major], the local UDP adapter is NOT the framed adapter ADR-003 mandates: codec-framed bytes reach real targets; empty datagrams are EOF-shaped at the adapter (tunnel teardown);

  8 KiB datagrams split; unframed target-initiated datagrams are dropped. Executable-pinned by probes. NOT fixed; remediation task tunnels/fix-udp-framed-adapter (blocks UDP-over-local consumers; wire surface unaffected).

• C-1 [major]: AcceptQueue::pop lost wakeup in the check→register window (open op hangs to the establishment bound on a lost push). Fixed in-review (create-Notified-before-check per iteration, the tokio notify_waiters contract; regression test added).
• U-2/U-3 [minor]: connect_udp v4-only bind posture; 65535-byte per-tunnel scratch. Riders on the U-1 task.
• N-4/N-5/N-7/N-8/N-9/N-10/N-11 [minor], fixed in-review (payload serialization, doc staleness, doc link, root re-exports, README + publish dry-run, harness comment, pump_against UDP framing doc note).
• N-2, retracted during the review by executable falsification (the codec's buffering is bounded by construction; the u16 cap IS the bound).
• N-6, a non-finding with rationale (TunnelEstablishError has no Timeout variant, correctly).

There are no criticals; the phase-gate passes.

Verification battery (all green)

• cargo test: 62 passed (default) / 69 passed (--features local), 3× repeat-run clean in both configs
• cargo clippy --all-targets -- -D warnings: clean
• cargo clippy --target wasm32-unknown-unknown -- -D warnings: clean
• cargo fmt --check: clean
• cargo doc --no-deps: clean after N-7's fix
• cargo check --target wasm32-unknown-unknown: clean
• cargo publish --dry-run --allow-dirty: passes after N-9's README

Summary

Agent fills this on completion.

Phase-gate review complete: the wire surface (the one-way door) is conformant; the API surface matches ADR-005/006; two majors found: U-1 (local UDP framing, remediation task created) and C-1 (queue lost wakeup, fixed with the review commit). The trivial batch + README + publish dry-run also landed with the review. Findings with severity grading and remediation ordering: docs/reviews/001-implementation-review.md.