alktty/docs/reviews/001-code-review.md

status, last_updated, reviewed_artifacts, tool, reviewer

status	last_updated	reviewed_artifacts	tool	reviewer
partially-resolved (M1, M2, L2, L4, L5, N1, N2, N3, N5)	2026-08-17	src/lib.rs src/wire.rs src/control.rs src/negotiation.rs src/backend.rs src/adapter.rs src/session.rs src/channels.rs src/local/mod.rs src/local/backend.rs src/local/pty.rs src/local/pipe.rs tests/common/mod.rs tests/negotiation.rs tests/pipe.rs tests/pty.rs Cargo.toml	manual source read + cargo test/clippy/fmt/doc + cargo-llvm-cov	post-Phase-5 code review (coverage + correctness + code smell)

Code Review #001 — Post-Phase-5 Review

Purpose

First full review of the alktty crate after Phase 5 (tests) landed. The publisher asked for three things: a coverage pass (cargo-llvm-cov) with an eye toward important gaps rather than raw numbers, a correctness pass, and a code-smell pass. This review covers all three.

The crate is a port of alknet-tty + alknet-tty-local from the alknet mono-repo into a single feature-gated crate, plus new channels integration (src/channels.rs) and a new consumer half (src/session.rs). The ported code (wire, control, negotiation, backend, adapter, local) is battle-tested; the new code (channels + session) is where the correctness findings concentrate, which is expected for first-pass code.

Methodology

• Full read of all 12 src/*.rs files (production + test modules) and all 4 integration test files.
• cargo test (default) and cargo test --all-features.
• cargo clippy --all-targets -- -D warnings and cargo clippy --target wasm32-unknown-unknown -- -D warnings.
• cargo check --target wasm32-unknown-unknown (the wasm-clean guard).
• cargo fmt --check and cargo doc --no-deps.
• cargo llvm-cov --all-features (summary + per-file + uncovered-lines) to attribute coverage gaps to specific code paths.
• cargo publish --dry-run --allow-dirty to confirm the package is publishable.
• Cross-reference every error path against its caller to confirm errors propagate (not swallowed) and carry useful attribution.

Verification Baseline

All verification run on the reviewed tree (commit 18c4924):

• cargo test: 81 lib tests pass (default, wasm-clean build).
• cargo test --all-features: 104 lib tests + 19 integration tests pass (123 total). Zero failures.
• cargo clippy --all-targets -- -D warnings: clean.
• cargo clippy --target wasm32-unknown-unknown -- -D warnings: clean.
• cargo check --target wasm32-unknown-unknown: clean (the default crate stays wasm-clean).
• cargo fmt --check: FAILS — 2 diffs in src/wire.rs (see L5).
• cargo doc --no-deps: 9 rustdoc warnings (see N1).
• cargo publish --dry-run --allow-dirty: packages cleanly (40 files, 608.2 KiB).
• cargo llvm-cov --all-features: 90.74% line coverage (3019/3327), 92.24% function coverage (392/425). Per-file breakdown below.

Coverage breakdown

Module	Lines	Functions
adapter.rs	92.35%	94.31%
backend.rs	89.94%	88.00%
channels.rs	93.43%	93.10%
control.rs	99.18%	100.00%
local/backend.rs	96.84%	94.12%
local/pipe.rs	90.84%	92.50%
local/pty.rs	80.85%	87.88%
negotiation.rs	99.26%	100.00%
session.rs	79.71%	79.25%
wire.rs	96.73%	96.23%
TOTAL	90.74%	92.24%

The two low modules are not test-helper noise — they are exactly where the correctness findings below live. session.rs (the consumer half) is the newest code and its lowest-covered paths are the channels constructor and the read-pump stdout/stderr routing (see L2, L3). local/pty.rs's uncovered lines are the blocking→async bridge's error paths (see L6).

Summary Statistics

Severity	Count
Critical	0
Medium	2 (M1, M2)
Low	6 (L1, L2, L3, L4, L5, L6)
Nit	6 (N1, N2, N3, N4, N5, N6)

No critical findings. The two Medium findings are in the consumer half (TtySession): the negotiation-rejection error frame is never handled, and wait() swallows the malformed-exit-chunk error. Both are correctness gaps in the newest code, not regressions in the ported code. The Low findings are a mix of coverage gaps, a public-API leak, and a process failure (cargo fmt). The Nits are hygiene.

---

Findings

M1. TtySession never handles the negotiation-rejection error frame

Files: src/session.rs:90 (NegotiationRejected), src/session.rs:225-254 (from_halves), src/session.rs:405-474 (read_pump)

Problem: TtySessionError::NegotiationRejected is dead — it is declared and documented but never constructed. from_halves writes the negotiation frame, then immediately starts read_pump with a ChunkReader. If the server rejects negotiation (unknown backend, forbidden, allocate_failed), it sends a 4-byte-length-prefixed JSON error frame whose first byte is 0x00. The ChunkReader will misparse that as a stream_type = 0 (stdin) chunk with a garbage length, and the session will either error with a confusing RawError or hang.

The entire ADR-052 §5 framing-disambiguation design exists precisely so the client can detect this case: an error frame's length prefix starts with 0x00, while a raw chunk's first byte is a stream_type in {1, 2, 4} (the server never sends 0 or 3). The producer half implements the disambiguation (it writes the error frame); the consumer half does not implement the read side. The error type documents the intent; the code does not do it.

Fix: after writing the negotiation frame, read the first byte of the response. If it is 0x00, read the remaining 3 length bytes, read the body, parse the {"error": "...", ...} JSON, and return TtySessionError::NegotiationRejected { error, fields }. Otherwise, the first byte is a stream_type and the session should hand the byte back to the ChunkReader (the reader needs a "push back one byte" path, or the session reads the full 5-byte header itself and constructs the first Chunk). This is the one place the consumer half is genuinely incomplete.

Lift: closes a real protocol gap — a rejected session currently surfaces as a confusing wire error instead of the intended NegotiationRejected. Medium effort (the disambiguation logic is ~30 lines, plus a small ChunkReader change to accept a pre-read first byte).

---

M2. wait() swallows MalformedExitChunk

Files: src/session.rs:359-381 (wait), src/session.rs:441 (read_pump)

Problem: the read pump does construct TtySessionError::MalformedExitChunk when a STREAM_CTRL_OUT chunk fails to parse as a ControlMessage (session.rs:441), but wait() collapses every Err(_) in the watch channel to TtySessionError::NoExitChunk:

if let Some(Err(_)) = borrow.as_ref() {
    return Err(TtySessionError::NoExitChunk);
}

A malformed exit chunk is therefore indistinguishable from a clean close. The MalformedExitChunk variant is effectively dead — the only place it is constructed is immediately flattened away by the only consumer of the watch channel. A client that receives a corrupt exit chunk gets "session ended without exit chunk" instead of "malformed exit chunk", which is misleading and hides a real protocol error.

Fix: wait() should match on the specific error variant and propagate it:

match borrow.as_ref() {
    Some(Ok(code)) => Ok(*code),
    Some(Err(e)) => Err(match e {
        TtySessionError::MalformedExitChunk(_) => /* clone/rebuild */,
        _ => TtySessionError::NoExitChunk,
    }),
    None => Err(TtySessionError::NoExitChunk),
}

TtySessionError is not Clone, so either derive Clone on the error type (it is all String/serde_json::Error/io::Error payloads — io::Error is Clone, serde_json::Error is Clone) or restructure the watch channel to carry a Result<i32, String> and map at the boundary. Small effort.

Lift: closes a swallowed-error path; makes the malformed-exit-chunk case observable. Small effort.

---

L1. Channels path ignores the registry-validated input

Files: src/channels.rs:180-205 (make_tty_open_handler)

Problem: make_tty_open_handler receives the open op's input (the NegotiateRequest params, validated by the registry's schema against carriage/backend/cmd required fields) and does let _ = input; — it reads the negotiation from the channel's wire frame instead. The registry validates input and enforces the scope gate, but the actual backend selection comes from the unvalidated wire frame. A client can pass schema-valid input and write a divergent frame.

This is not a vulnerability today (the only ACL is identity-scoped, and drive_session re-validates the frame), but it is a latent inconsistency: the channels wrapper's AccessControl and the drive_session negotiation reader are two independent gates that can disagree. The doc comment on make_tty_open_handler acknowledges the frame is read from the wire "same as the direct-ALPN path", but the input parameter is then dead weight.

Fix: decide with the publisher. Either (a) drop the input parameter from the OpenHandler signature if the wire frame is the source of truth, or (b) pass input through to drive_session so the registry-validated params are authoritative and the wire frame is ignored. Option (b) is the more consistent design but requires a drive_session variant that takes pre-parsed params. Defer until the channels path gets a real consumer.

Lift: removes a latent inconsistency. Small effort either way.

---

L2. Consumer read-pump stdout/stderr routing is untested

Files: src/session.rs:419-429 (read_pump), src/session.rs:588-608 (recv_stdout_yields_backend_stdout)

Problem: the consumer's core data path — actually receiving stdout and stderr chunks and routing them to the right channel — is never tested with real data. The one test that touches it (recv_stdout_yields_backend_stdout) explicitly admits the gap:

MockBackend doesn't pump stdout (it resolves exit immediately), so the stdout stream should be empty.

MockBackend drops its stdout/stderr senders on allocation, so the read pump's STREAM_STDOUT/STREAM_STDERR arms (session.rs:419-429) are never exercised with a non-empty payload. The producer half's stdout/stderr pumping is well-tested (adapter.rs), but the consumer half's receiving of those chunks is not. A regression in the routing (e.g. stdout chunks landing on the stderr channel) would not be caught.

Fix: add a test backend (or extend MockBackend) that emits a stdout chunk and a stderr chunk before resolving exit, then assert recv_stdout() yields the stdout bytes and recv_stderr() yields the stderr bytes. Small effort, high value — this is the consumer half's happy path.

---

L3. open_via_channels and from_bidi_stream_via are 0% covered

Files: src/session.rs:175-197 (open_via_channels), src/session.rs:213-221 (from_bidi_stream_via)

Problem: the entire consumer channels path is untested. Only connect_direct is exercised (via the duplex harness in session.rs::tests). open_via_channels — the constructor that opens a channel via ChannelClient, adopts the channel, builds a Connection from channel_source, and negotiates — has zero coverage. The producer side of channels (register_openable, TtyOpenHandler) is tested end-to-end in channels.rs::tests, but the consumer side is not.

Fix: add an end-to-end test that wires a ChannelClient to a server-side ChannelCore (the wire_client_and_server harness in channels.rs::tests already does the hard part) and calls TtySession::open_via_channels(client, params), asserting the session negotiates and wait() resolves. Medium effort (the harness exists but is in a different module's test scope; it may need to be shared or duplicated).

---

L4. Test mocks leak into the public API

Files: src/backend.rs:286 (MockControl), src/backend.rs:308 (MockBackend)

Problem: MockBackend and MockControl are pub in backend.rs, and backend is pub mod (lib.rs:50). They are only used by #[cfg(test)] modules (adapter.rs, channels.rs, session.rs tests), so they should not be part of the public API surface. Downstream consumers see alktty::backend::MockBackend and alktty::backend::MockControl, which is noise and a maintenance liability (they are now part of the semver surface).

Fix: change pub struct MockBackend / pub struct MockControl to pub(crate) struct, or gate them behind a test-util feature. The pub(crate) route is simplest and sufficient — all uses are in-crate test modules. Small effort.

---

L5. cargo fmt --check fails — unformatted code was committed

Files: src/wire.rs:447-460 (bast_stream_type_enum_matches_wire_constants)

Problem: cargo fmt --check reports 2 diffs, both in the BAST drift-detection test added in commit 18c4924. The agent's commit message claimed "My change introduced no new warnings," but it introduced a cargo fmt failure — the cargo fmt --check step in AGENTS.md's verification commands was skipped. This is the most concrete, actionable item in the review: the tree does not pass its own verification gate.

Fix: run cargo fmt and commit the result. Trivial effort.

---

L6. local/pty.rs blocking→async bridge error paths are untested

Files: src/local/pty.rs:409-411 (try_clone_reader failure), src/local/pty.rs:428-431 (reader read error), src/local/pty.rs:450-452 (take_writer failure), src/local/pty.rs:460-466 (writer write/flush failure), src/local/pty.rs:486-489 (waiter wait() failure)

Problem: local/pty.rs is the lowest-covered module (80.85% lines), and the uncovered lines are exactly the error paths of the three-thread bridge. These are the paths that matter most for a blocking→async bridge: what happens when the master reader can't be cloned, when the writer thread's write_all fails, when the waiter's wait() fails. The happy path (echo, cat, resize, signal) is well-tested; the failure paths are not.

Some of these are structurally hard to reach (e.g. try_clone_reader failing requires the master to be in a bad state), but the writer write/flush failure and the waiter wait() failure are reachable by killing the child mid-write or racing the waiter. The wait() failure path in particular is the one that produces the -1 exit code sentinel (ADR-055 §4), which is a documented wire-format behavior with no test.

Fix: add targeted tests for the reachable paths (waiter wait() failure → -1 exit code; writer write failure → clean thread exit). The try_clone_reader/take_writer failure paths can be left as documented-unreachable if the publisher agrees. Medium effort.

---

N1. Nine rustdoc warnings (unresolved intra-doc links)

Files: src/lib.rs:23,30, src/backend.rs:96, src/negotiation.rs:75, src/channels.rs:12,53,70, src/session.rs:16,259

Problem: cargo doc --no-deps emits 9 unresolved-link warnings. These are the "gross" warnings the Phase-5 agent noted as pre-existing. Each is a distinct fix:

• lib.rs:23 [local] and lib.rs:30 [local::LocalTtyBackend] — the local module is feature-gated, so the link target doesn't exist in the default (no-features) doc build. Fix: use a plain-code link (`local`) or gate the doc line with #[cfg(feature = "local")].
• backend.rs:96 argv[0] and negotiation.rs:75 argv[0] — parsed as intra-doc links. Fix: escape as `argv[0]` (backticks) or argv\[0\].
• channels.rs:12,70 [TtyOpenHandler] — TtyOpenHandler is a private fn, not linkable. Fix: plain-code `TtyOpenHandler`.
• channels.rs:53 [crate::adapter::TtyAdapter::alpn] — alpn is a method, not a field/associated item. Fix: link the type [crate::adapter::TtyAdapter] and mention alpn() in prose.
• session.rs:16 [alkcall::channels::ChannelClient] — wrong path; the type is alkcall::channels::client::ChannelClient. Fix the path.
• session.rs:259 [close_stdin] — needs [Self::close_stdin].

Fix: one pass over the 9 sites. Trivial effort, but it makes cargo doc clean and unblocks treating doc warnings as errors in CI.

---

N2. Stale doc paths that don't exist in this repo

Files: src/wire.rs:32, src/adapter.rs:51

Problem: two doc comments reference paths that don't exist in the alktty tree:

• wire.rs:32 → docs/architecture/crates/tty/tty-wire.md — the crates/tty/ subpath is an alknet-mono-repo path; in alktty the file is docs/architecture/tty-wire.md.
• adapter.rs:51 → docs/research/alknet-crate-extraction/findings.md — there is no docs/research/ directory in alktty.

Fix: update both to the correct alktty paths. Trivial effort.

---

N3. unsafe blocks contradict AGENTS.md §14

Files: src/local/pty.rs:130,135, src/local/pipe.rs:167,176,420

Problem: AGENTS.md §14 states "the crate has zero unsafe blocks." The code has five unsafe { libc::kill(...) } blocks (all in the local feature module, all the safe-libc-call pattern the convention describes). The convention's own wording is violated — either the code or the convention needs reconciling. The libc::kill calls are the documented signal-forwarding path and are not a safety concern, but the convention should be accurate.

Fix: amend AGENTS.md §14 to say "zero unsafe blocks outside the local feature module's libc::kill signal-forwarding calls (safe libc crate APIs, not unsafe blocks in the crate's own logic)." Trivial effort.

---

N4. Sleep-based timing in tests

Files: src/local/pty.rs (150-300ms sleeps), src/local/pipe.rs, tests/pipe.rs, tests/pty.rs

Problem: the signal and cancel-cleanup tests use fixed tokio::time::sleep delays (150-300ms) to let the child process reach a state before signaling or dropping. This is a flakiness risk on slow CI — a loaded machine can exceed the grace period and the test fails spuriously. The cancel-cleanup tests are better (they poll for a pid file), but the signal tests (signal_int_kills_child, signal_reaches_process_group_child, pipe_signal_sigterm_kills_child) sleep a fixed duration before signaling.

Fix: replace the fixed sleeps with a readiness signal where possible (e.g. have the child write a marker to a temp file before exec, as the cancel-cleanup tests already do), or widen the grace period and add a retry. Low priority — the tests pass reliably on the current machine — but worth hardening before CI.

---

N5. rand_seed/nanos_seed duplicated four times

Files: src/local/pipe.rs:462, src/local/pty.rs:705, tests/pipe.rs:218, tests/pty.rs:262

Problem: four identical nanos-timestamp helpers (used to uniquify temp-file names in the cancel-cleanup tests) are copy-pasted across the crate and the integration tests. The integration-test copies can't share with the crate (separate compilation units), but the two src/local/ copies could be a single pub(crate) helper, and the two tests/ copies could live in tests/common/mod.rs.

Fix: consolidate. Trivial effort.

---

N6. rust-version = "1.85" is unverified

Files: Cargo.toml:5

Problem: the crate declares rust-version = "1.85" but there is no MSRV CI job, and the review toolchain is 1.94. The declared MSRV is aspirational until it is actually checked. A dependency bump or a new language feature could silently raise the real MSRV above 1.85.

Fix: add an MSRV check to CI (e.g. cargo +1.85 check), or bump the declared rust-version to a value that is actually verified. Low priority.

---

What's Good

The crate is in strong shape for a first-pass port + new-code review. Highlights:

• The ported code is clean. wire, control, negotiation, backend, and adapter are well-tested (92-99% line coverage) and the error paths propagate correctly. The ChunkReader/ChunkWriter and NegotiationReader/NegotiationWriter are defensive (bounds-checked lengths, ConnectionClosed vs Io distinction, no oversized allocations).
• The exit-chunk-is-last invariant (ADR-055) is genuinely enforced and tested. pump_session joins both stdout/stderr pumps and the exit future before enqueueing the exit chunk, and the integration tests assert no chunk follows the exit chunk.
• The kill-on-Drop contract (ADR-056) is real and tested. Both LocalExitFuture (PTY) and PipeExitFuture (pipe) implement the disarm-on-resolve / kill-on-cancel pattern correctly, and the cancel-cleanup tests probe the child's pid after drop to confirm no orphan.
• The BAST drift-detection test is a nice touch. The bast_stream_type_enum_matches_wire_constants test (wire.rs:445) parses the BAST document and asserts the StreamType enum matches the STREAM_* constants — exactly the cheap drift guard the plan's "Risk: BAST schema drift" mitigation called for.
• The control-channel split (Phase 7) is well-tested. The adapter tests cover Exit-on-STREAM_CTRL_IN and STREAM_CTRL_OUT-from- client as protocol violations, and the exit-chunk-arrives-on-ctrl-out test pins the direction.
• No unwrap/expect/panic! in production code. All are confined to #[cfg(test)] modules and the test harness. The poisoned-mutex pattern (unwrap_or_else(|e| e.into_inner())) is used where it matters.
• The wasm-clean invariant holds. cargo check --target wasm32-unknown-unknown and the wasm clippy pass are both clean, and the local feature is properly isolated.

---

Remediation Plan

ID	Finding	Fix	Effort	Risk	Status
L5	cargo fmt fails	run cargo fmt, commit	trivial	none	✅ resolved
N1	9 rustdoc warnings	fix 9 link sites	trivial	none	✅ resolved
N2	stale doc paths	fix 2 paths	trivial	none	✅ resolved
N3	unsafe vs AGENTS.md §14	amend convention wording	trivial	none	✅ resolved
L4	mocks leak into public API	pub(crate) on MockBackend/MockControl	trivial	none	✅ resolved
N5	rand_seed/nanos_seed ×4	consolidate	trivial	none	✅ resolved
M2	wait() swallows MalformedExitChunk	propagate the variant	small	low	✅ resolved
L2	consumer stdout/stderr routing untested	add emitting test backend	small	low	✅ resolved
M1	negotiation-rejection frame unhandled	implement disambiguation read	medium	medium (wire-facing)	✅ resolved
L1	channels input ignored	decide drop-vs-pass-through	small	low	open
L3	open_via_channels 0% covered	end-to-end channels consumer test	medium	low	open
L6	pty bridge error paths untested	targeted error-path tests	medium	low	open
N4	sleep-based timing	readiness signals	small	low	open
N6	MSRV unverified	CI MSRV job or bump	small	none	open

Resolution (2026-08-17, commit 9944153)

Nine findings were resolved in a single commit:

• M1 — TtySession::from_halves now peeks the first response byte and returns NegotiationRejected on a 0x00-prefixed error frame (ADR-052 §5 disambiguation). ChunkReader gained peek_stream_type/read_chunk_after_peek to support the peek. Test: connect_direct_returns_negotiation_rejected.
• M2 — wait() now surfaces MalformedExitChunk instead of collapsing it to NoExitChunk. The exit watch channel carries a cloneable ExitOutcome enum (the TtySessionError payloads are not Clone); MalformedExitChunk now carries a String. Test: wait_returns_malformed_exit_chunk.
• L2 — added EmittingBackend and recv_stdout_and_stderr_route_backend_data, covering the consumer read-pump stdout/stderr routing with real data.
• L4 — MockBackend/MockControl/MockStdinSink are now #[cfg(test)] pub(crate), removed from the public API.
• L5 — cargo fmt (the BAST drift test was unformatted).
• N1 — all 9 rustdoc intra-doc links fixed; cargo doc is clean.
• N2 — stale doc paths fixed (crates/tty/ and docs/research/).
• N3 — AGENTS.md §14 amended to accurately describe the local module's libc::kill unsafe blocks.
• N5 — nanos_seed consolidated into tests/common/mod.rs.

Post-fix coverage: session.rs 79.71% → 87.43% lines, total 90.74% → 91.47% lines. All verification gates pass (cargo test, cargo test --all-features, clippy native + wasm, fmt, doc, wasm check).

Remaining (open)

• L1 — channels input ignored; needs a publisher decision (drop the parameter vs pass-through to drive_session).
• L3 — open_via_channels still 0% covered; needs the channels harness shared across modules.
• L6 — pty bridge error paths untested.
• N4 — sleep-based timing in signal/cancel tests.
• N6 — MSRV unverified.

Recommended Order (remaining)

1. L1 + L3 — the channels consumer path; do together since L3's test will exercise L1's code.
2. L6 — pty bridge error paths; medium effort, lower priority than the consumer-half work.
3. N4 + N6 — test hardening and MSRV; defer until CI exists.

---

Notes

• All line numbers refer to the tree at commit 18c4924 (the last commit on main at review time). The resolution section above reflects the tree at commit 9944153.
• The coverage numbers are from cargo llvm-cov --all-features on the same tree. The --show-missing-lines output was used to attribute gaps; the full report is at target/llvm-cov/html.
• This review does not cover documentation quality (README, inline docs, docs.rs rendering) beyond the stale-path nit (N2) and the rustdoc-link nit (N1). Per the publisher's workflow, that is a separate sweep.
• Findings M1 and M2 are in the consumer half (TtySession), which is the newest code in the crate. The ported producer half (adapter, wire, negotiation, backend, local) is in good shape; the findings there are coverage gaps (L6) and hygiene (N2, N3), not correctness bugs.