alkdev/alktty

Fork 0

Files

T

glm-5.3-flash 66c6e693bb docs: record v0.1.0 publish in review #003 (review closed)

2026-09-05 17:02:12 +00:00

36 KiB

Raw Blame History

status, last_updated, resolved, published, reviewed_artifacts, tool, reviewer, base_commit, resolutions

status last_updated resolved published reviewed_artifacts tool reviewer base_commit resolutions

closed

2026-09-05

1-16 (P14 closed; P13's accept-loop harness deferred by disposition)

v0.1.0 (crates.io, 2026-09-05) — review closed before publish

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

AGENTS.md

docs/architecture/README.md

docs/plans/project-setup.md

two subagent reviews (code review + packaging/docs vs alkcall 0.4.1) + manual verification of every finding against source + empirical deadlock reproduction

prepublish review for v0.1.0 (subagent-assisted)

918af40 (review

P1 + P13 resolved 2026-09-05 (87c52e5) — see the Session-1 Resolution section

P12 + P6 + P7 + P4 + P16 resolved 2026-09-05 — see the Session-2 Resolution section

P8 + P9 + P10 + P5 + P11 + P2 + P3 + P15 resolved 2026-09-05 — see the Session-3 Resolution section

Code Review #003 — Prepublish Review (v0.1.0)

Purpose

Prepublish sanity check for the first crates.io release. Everything before this point was reviewed while the crate was unpublished, so every API-shape and packaging mistake was still free. This review is the last pass under that property: it looks for (a) correctness bugs, (b) semver-visible surface decisions that are cheap now and expensive per-consumer later, and (c) packaging/docs parity with alkcall 0.4.1 (the org's published-crate reference).

Two subagent reviews were run against this tree — a code review (conventions, wire format, concurrency, API surface, test gaps) and a packaging/docs review (Cargo.toml parity with alkcall, package-list inspection, docs consistency, MSRV). Every M and L finding below was then manually re-verified against source by the publisher before being recorded here; the P1 deadlock was additionally reproduced empirically (twice, independently).

Verification Baseline

All verification run on the reviewed tree (918af40, before this review's own commit):

cargo test: 95 lib tests pass (default, wasm-clean build).
cargo test --all-features: 138 tests pass (119 lib + 5 negotiation + 6 pipe + 8 pty). Zero failures.
cargo clippy --all-targets -- -D warnings: clean.
cargo clippy --target wasm32-unknown-unknown -- -D warnings: clean (also cargo check --target wasm32-unknown-unknown).
cargo fmt --check: clean.
cargo doc --no-deps: 0 warnings.
cargo publish --dry-run --allow-dirty: OK — packages 42 files, 671.0KiB (180.7KiB compressed), no warnings. (One packaged file is wrong — see P2.)
alkcall pinned at 0.4.1 (Cargo.lock); alkcall = "0.4.0" in Cargo.toml; MSRV 1.85 backed by lockfile pins (idna_adapter 1.2.0, icu 2.0.x/1.5.x) — no post-1.85 syntax found in src/ or tests/.
CJK scan (git ls-files | xargs grep -lP "[\p{Han}]"): zero matches in tracked files.

Findings

P1. [M — blocks publish] `pump_session` deadlocks when the backend produces ≥63 stdout chunks before exit

Files: src/adapter.rs:428-490 (pump_session)

Problem: the stdout/stderr pumps and the exit_code future are joined (tokio::join!, adapter.rs:465-475) before the client-facing drainer loop starts (adapter.rs:480-486). All three producers share one bounded channel (mpsc::channel::<Chunk>(64), adapter.rs:437). Once 64 chunks are queued, pump_stdout's writer_tx.send(...).await (adapter.rs:516) parks — and because the drainer hasn't started, the channel never drains. pump_stdout can't finish, so the sentinel send (adapter.rs:520) can't happen, exit_code may still resolve but join! never completes (it waits for the stdout pump handle), the exit chunk is never enqueued, and the drainer never starts. Circular wait; the session hangs with the client actively reading.

Reproduced empirically (mock backend streaming N chunks through the real drive_session over a duplex transport, client reading with a 10 s timeout):

N=62 → OK: 62 data chunks + stdout sentinel + exit chunk = 64, exactly fills the channel before the join completes.
N=63 → deadlock: client times out having received 0 chunks.

Any real session (cat a file, ls -R) exceeds 64 chunks instantly — the first published consumer would hit this on day one.

Fix direction (preserves the exit-chunk-is-last invariant, ADR-005): start the drainer before the join — spawn a task owning writer_rx and client_write that loops recv() → ChunkWriter::write_chunk (breaking on write error, as today); run the pumps/exit join exactly as now; send_exit_chunk after the join (same ordering — single mpsc FIFO, so the exit chunk is still last); then drop(writer_tx) so the drainer sees channel close and finishes. The W: AsyncWrite + Send + Unpin + 'static bound already permits moving the writer into a task. Add a >64-chunk regression test with the fix (see P13).

P2. [M] `AGENTS.md` ships in the published crate; exclude list has two dead entries

Files: Cargo.toml:11

Problem: alkcall's exclude contains "AGENTS.md" (alkcall/Cargo.toml:12); alktty's does not. Verified in the package list: alktty-0.1.0/AGENTS.md ships. Also:

"Cargo.lock" in exclude is a dead entry — modern cargo force-includes the lockfile; it ships in both crates regardless (alkcall doesn't exclude it). Remove the entry or keep as documentation.
"docs/research/" targets a directory that doesn't exist (harmless, but dead).
docs/plans/project-setup.md ships, including its /workspace/@alkdev/... absolute paths. alkcall ships only docs/architecture/*. Consider excluding docs/plans/.

Fix: add "AGENTS.md" (and decide on docs/plans/) to exclude; drop or keep the dead entries deliberately.

P3. [M] Stale agent- and reader-facing docs contradict the tree

Files: AGENTS.md:253-257,307-310, docs/architecture/README.md:21-22,28-37,138-142

Problem: three stale spots, one of them actively harmful:

AGENTS.md:310 says alkcall "is v0.1.x … Pin alkcall = "0.1.1"" while Cargo.toml pins alkcall = "0.4.0" (lock: 0.4.1). An agent following AGENTS.md would downgrade the dependency. Same doc's ADR mapping stops at 007→008 and omits ADR-009 (landed in 37ae07a).
AGENTS.md:253-254 and docs/architecture/README.md:140-142 say "Phase 5 … is not yet done" — Phase 5 landed 2026-08-17 per docs/plans/project-setup.md:5, and the integration tests exist and pass (19 tests).
docs/architecture/README.md:28-37 ADR table stops at 008 and the preamble says "renumbered into alktty's ADR range (001..008)" — the directory contains 009, which overview.md and tty-adapter.md already reference.

Fix: update AGENTS.md (alkcall guidance, phase status, ADR range 001..009) and the architecture README (ADR-009 row, phase status). Related N-level staleness in the plan doc is folded into P16.

P4. [M] No README (and no `readme` field)

Files: Cargo.toml ([package]), repo root

Problem: no README.md and no readme = "README.md" field; alkcall has both. cargo publish --dry-run does not warn, so nothing catches it at publish time — the crates.io page will be bare and the missing field is a (trivially additive, but avoidable) post-publish change.

Fix: write the README mirroring alkcall's structure — tagline + positioning paragraph, ## Quick start with per-role compilable examples (producer: backends map + TtyAdapter as ProtocolHandler, or channels::register_openable; consumer: TtySession::connect_direct / open_via_channels), ## Architecture (producer/consumer framing per AGENTS.md convention 7; wasm target + local feature note), ## Documentation (architecture docs, docs.rs), ## License. Add the readme field. Do this after P5/P6 so the examples show the final surface.

P5. [L] Backend lookup runs before the scope gate

Files: src/adapter.rs:300-312 (validate_and_allocate)

Problem: backends.get(&req.backend) (adapter.rs:300) is checked before has_scope(identity, TTY_OPEN_SCOPE) (adapter.rs:309). An unscoped caller gets unknown_backend for registered names and forbidden otherwise — a differential that enumerates registered backend names ("local", and every future alknet-docker/alknet-ssh registration) to any authenticated-but-unscoped identity.

Fix: move the scope check above the lookup (arguably above the carriage/cmd checks too). Trivial; check whether any test asserts unknown_backend with an unscoped identity and update accordingly.

P6. [L] Wire write paths accept invalid chunks

Files: src/wire.rs:255-306 (write_chunk, write_stdin, write_ctrl_in_json, write_ctrl_out_json)

Problem: the read path validates (stream_type > 4 → error, length > MAX_CHUNK_LEN → error, wire.rs:200-212), but the write paths validate nothing: they accept stream_type > 4, payloads larger than MAX_CHUNK_LEN, and compute chunk.bytes.len() as u32 — silently truncating a ≥2³² payload and corrupting the peer's framing. A bad Chunk from a caller (or a future backend bug) corrupts the peer's stream instead of failing locally.

Fix: return RawError (e.g. InvalidStreamType/ChunkTooLarge reused) when stream_type > 4 or len > MAX_CHUNK_LEN. Additive pre-consumers; aligns write-side with read-side invariants. Also decide the empty-payload inconsistency while touching this: write_chunk/ write_stdin skip the payload when empty but write_ctrl_* always writes it (wire.rs:261 vs 290/303) — harmless on the wire (length 0 ≡ skip) but pick one shape.

P7. [L] `ChunkReader`: calling `read_chunk()` after a manual `peek_stream_type()` silently corrupts framing

Files: src/wire.rs:166-185

Problem: read_chunk() internally calls peek_stream_type() then read_chunk_after_peek() (wire.rs:167-168). A caller who has already peeked and then calls read_chunk() (instead of read_chunk_after_peek()) consumes a second header byte — the peeked byte is gone — and the stream desynchronizes with no error. The docs steer callers correctly, but the misuse is one wrong call away and silent.

Fix: track peeked state in the reader (a bool; read_chunk asserts/uses it) or debug_assert!(!self.peeked) in read_chunk. Small, pre-consumers, and it makes the misuse loud instead of corrupt.

P8. [L] `StdinSink::poll_shutdown` returns `Pending` without registering a waker

Files: src/local/pty.rs:339-357

Problem: on a full stdin command channel, try_send(StdinCmd::Eof) → Full → Poll::Pending — but _cx is ignored, so no waker is registered. The input pump task may never be woken: a stdin blast that fills the channel immediately followed by EOF never delivers the EOF to the child, whose exit_code then never resolves. Same hang shape as P1, lower trigger probability.

Fix: mirror poll_write's inflight-future pattern in the same file (send an async send and poll it), so backpressure wakes correctly.

P9. [L] `pty.rs` uses `.lock().expect("… poisoned")` — violates the poisoned-lock convention

Files: src/local/pty.rs:134,175,184,433,474

Problem: five non-test sites use .expect() on lock acquisition. AGENTS.md convention 2 mandates unwrap_or_else(|e| e.into_inner()) so a panic in one operation does not cascade. (All other .expect() sites in src/local/ are under #[cfg(test)] — verified: local/backend.rs:83, pty.rs:546, pipe.rs:302 are the test-module boundaries.)

Fix: mechanical replacement at the five sites.

P10. [L] `pty.rs` panics on thread-spawn failure in library code

Files: src/local/pty.rs:465,504,523

Problem: thread::Builder::spawn(...).expect("spawn pty-reader/-writer/-waiter") panics on thread-spawn failure (fd/thread exhaustion) — a panic in library code, and the no-panics convention's spirit. allocate_pty already returns Result, so the error path exists.

Fix: map the three sites to TtyError (e.g. Backend / AllocFailed with the io error message).

P11. [L] `recv_stdout` doc says the stream ends at the sentinel; the implementation delivers it as an empty item

Files: src/session.rs:436-453 (doc vs unfold)

Problem: the doc states the stream ends when the server's stdout reaches EOF (the zero-length sentinel). The unfold ends only when the read pump terminates — the sentinel arrives as an empty Bytes item the consumer must filter. The crate's own tests do exactly that (session.rs:918, 1160), i.e. the implementation is the de-facto contract and the doc is wrong — or the doc is the intended contract and the unfold should terminate on the empty stdout item.

Fix: pick one before consumers codify the accident. Terminating the stream on the empty-stdout item matches the documented contract (and the sentinel's meaning: "drained") and is the better shape; the doc fix is the fallback.

P12. [L] No crate-root re-exports of the primary types

Files: src/lib.rs:49-58

Problem: the crate root exports modules only; every consumer writes alktty::adapter::TtyAdapter, alktty::session::TtySession, alktty::backend::{TtyBackend, TtyError, TtyHandle}. Ergonomics surface decided implicitly by omission. alkcall re-exports its core types (alkcall::core) — the org pattern favors a curated root.

Fix: add root re-exports (pub use of the primary types — and decide whether wire/control/negotiation constants like STREAM_STDOUT get root aliases). Purely additive; do it before the README (P4) so examples use the short paths.

P13. [L] Test gaps on the riskiest paths

Files: src/wire.rs (tests), tests/

Problem: no test drives >64 chunks through pump_session (the P1 regression — the single most important missing test in the crate); no MAX_CHUNK_LEN exact-boundary test (the suite covers over-limit only); no test for the server sending stream_type 0/3 toward the consumer's read pump (the pump's discard arm); and TtyAdapter::handle's accept_bi loop is untested (needs a transport harness).

Fix: the P1 fix lands with its backpressure regression test; add the boundary/pump-discard cases to wire.rs/session.rs unit tests. The accept-loop harness can wait for the first transport-level consumer.

P14. [N] `register_openable` returns `Result<(), String>`

Files: src/channels.rs:108

Problem: an untyped error locks String into the public shape. However, alkcall's own register_openable/register_on have the identical Result<(), String> shape (alkcall src/channels/operations.rs:65,405) — alktty is consistent with its upstream, and a typed error here ahead of alkcall's would be a divergence.

Disposition: closed as consistent-with-upstream. Revisit only if alkcall types its return first; match whatever alkcall does.

P15. [N] Minor polish batch

Verified, low-stakes items; batch into one commit when convenient:

src/backend.rs:253-254 — doc typo: "alktty's local the reference implementation" (missing "module is").
Cargo.toml:50 — tokio-stream is redundant as a dev-dependency (the main dep is genuinely used at adapter.rs:73; dev-deps inherit it).
src/adapter.rs:362 — NegotiationError::Io(_) arm returns silently while the catch-all arm logs debug!; add a log for parity.
src/session.rs:191-193 — connect_direct flattens alkcall's StreamError into io::Error::new(ConnectionReset, string), losing the type. Consider a TtySessionError variant pre-consumers (additive now, breaking later — same logic as R5 in review #002).
src/session.rs:228 — params.clone() exists only for the fail-fast parse; serde_json::from_value can consume via Deserialize::deserialize(&params)… or restructure to avoid the clone. Trivial.
src/adapter.rs:133 — set_identity result discarded with let _; log on failure or assert in debug.
src/adapter.rs:478 — drop(input_pump) detaches the client→backend pump; it lingers until the client disconnects after session end. Aborting at session end is tighter.
src/negotiation.rs:263-271 — an "error" key in fields would overwrite the error code in error_response_bytes; no caller passes one today. One-line guard or doc note.
src/adapter.rs:146-150, src/session.rs:451 — two trivial inline comments (split-idiom explanation; "Already taken") — the carve-out is for non-obvious correctness constraints; promote or remove.
docs/plans/project-setup.md:530-531 — test counts stale (says 80+19; actual 119 lib + 19 integration = 138 under --all-features); front-matter last_updated dates in overview.md/tty-adapter.md predate the ADR-009 edits.

P16. [N] No CHANGELOG.md

Files: repo root

Problem: alkcall ships a Keep-a-Changelog-format CHANGELOG.md (# Changelog preamble, ## [X.Y.Z] - YYYY-MM-DD sections with ### Added/Changed/Fixed, link definitions mapping versions to git tags). alktty has none, so v0.1.0's entry will be written retroactively from the log.

Fix: start CHANGELOG.md with the full [0.1.0] history (the crate's commits are the source), including the link definition for the v0.1.0 tag — and note alkcall's own 0.4.x headings currently lack theirs; don't replicate that nit.

Examined and closed

RawError/NegotiationError lack #[non_exhaustive] — matches the recorded policy from review #002's R5 resolution: those enums mirror fixed wire semantics and stay exhaustive deliberately (in-crate matchers keep exhaustiveness checking); only the extension-point enums (TtyError, TtySessionError) are #[non_exhaustive]. Both are. No change.
register_openable untyped error — see P14; consistent with alkcall's shape. Closed.
"Phase 7" references — all are the legitimate alknet-Phase-7 provenance note inside ADR-001 (control-channel split amendment), matching AGENTS.md's instruction. Not stale.
MSRV 1.85 — no let-chains, no edition-2024 syntax, no post-1.85 std APIs in src//tests/; lockfile pins back the claim. Clean.
unsafe — exactly the four documented libc::kill sites in src/local/ (pty.rs:157,162, pipe.rs:167,176). Convention 14 holds.
local-feature isolation — zero portable_pty/tokio::process/ std::thread/libc code outside src/local/ (doc mentions only); wasm check + clippy pass. Convention 9 holds.

What's Good

The verification baseline is genuinely clean — 138 tests, both clippy targets, fmt, doc, publish dry-run, wasm build all pass on the reviewed tree.
The reader side of the wire format is correct. Header bounds checks, bounded allocation (≤16 MiB before vec!), ConnectionClosed vs Io distinction, the 0x00 framing-disambiguation invariant, and the BAST-drift guard on enum ordering all match ADR-001/006.
The cancellation/kill contract holds. Both kill guards (LocalExitFuture, PipeExitFuture) disarm on resolve and kill on cancel (ADR-006), verified by the cancel-cleanup integration tests; TtySession::wait's watch pattern has no lost wakeup; Drop aborts the read pump.
The packaging is one file away from alkcall parity. Every other [package] field matches (edition, rust-version, license + both files, keywords at the 5 limit, categories, [lib] name), and the publish dry-run is otherwise warning-free.
The docs structure is sound. The ADR set is complete on disk (001–009, including the reversal pair 007/008), the "Phase 7" provenance is handled per convention, and the port-provenance alknet-tty mentions all correctly describe history.

Remediation Plan

ID	Finding	Fix	Effort	Risk	Status
P1	pump_session deadlock ≥63 chunks	concurrent drainer + regression test	small	medium (core pump)	✅ resolved (`87c52e5`)
P2	AGENTS.md ships; dead exclude entries	fix `exclude`	trivial	none	✅ resolved (Session 3)
P3	stale AGENTS.md + architecture README	refresh phase/ADR/alkcall text	trivial	none	✅ resolved (Session 3)
P4	no README + `readme` field	write README (after P5/P6/P12)	medium	none	✅ resolved (Session 2)
P5	scope gate after backend lookup	reorder checks	trivial	low (test updates)	✅ resolved (Session 3)
P6	write paths accept invalid chunks	validate on write; unify empty-payload shape	small	low	✅ resolved (Session 2)
P7	read-after-peek framing corruption	peeked-state guard	small	none	✅ resolved (Session 2)
P8	poll_shutdown missing waker	inflight-future pattern	small	low	✅ resolved (Session 3)
P9	poisoned-lock `.expect()` × 5	`into_inner()`	trivial	none	✅ resolved (Session 3)
P10	thread-spawn `.expect()` × 3	map to `TtyError`	trivial	none	✅ resolved (Session 3)
P11	recv_stdout sentinel contract	terminate stream on sentinel (or fix doc)	small	low (semver-visible)	✅ resolved (Session 3)
P12	no crate-root re-exports	`pub use` primary types	trivial	none	✅ resolved (Session 2)
P13	test gaps	backpressure (>64 chunks) + boundary tests	small	none	✅ resolved (`87c52e5`; accept-loop harness deferred)
P14	`Result<(), String>` on register_openable	closed — alkcall parity	none	none	✅ closed
P15	minor polish batch	one batched commit	small	none	✅ resolved (Session 3)
P16	no CHANGELOG.md	start changelog with [0.1.0]	small	none	✅ resolved (Session 2)

Suggested Session Breakdown

Natural break points, cheapest-dependency order first:

P1 + P13 — the deadlock and its regression test are one unit (the test proves the fix). Highest value; do first.
P12 + P6 + P7 — the API-shape batch (root re-exports, write validation, peek guard). Then P4 (README uses the final surface) and P16 (changelog) in the same or next session.
P8 + P9 + P10 — the local robustness batch, all in pty.rs.
P5 + P11 — producer-hygiene batch (scope reorder + sentinel contract decision).
P2 + P3 + P15 — packaging/docs closeout; then final cargo publish --dry-run and tag.

Session-1 Resolution (P1 + P13, 2026-09-05)

P1 and P13 were resolved together (the regression test is part of the fix). Commit: 87c52e5.

P1 fix: pump_session now spawns the drainer (drain_chunks, a new function owning writer_rx + client_write) before the pumps+exit join; after the join, send_exit_chunk enqueues as before, then drop(writer_tx) lets the drainer observe channel close and finish. The single FIFO preserves exit-chunk-is-last (ADR-005); W's existing 'static + Send + Unpin bounds permitted moving the writer into the task. The doc comment on pump_session was updated to describe the concurrent-drainer shape and why it is load-bearing.
P1 regression test (adapter::tests::backpressure_more_chunks_than_channel_capacity): drives 80 chunks (> the 64-slot channel) through a real drive_session via the TestBackend harness, asserts in-order delivery of all 80 + sentinel + exit, with a 10 s per-read timeout so a regression fails the test instead of hanging it (the test body was validated against the bug: with the old order reinstated, it fails with "chunk read must not stall (P1 regression: deadlock): Elapsed" — the producer is spawned so the test's own sends cannot park first).
P13 boundary test (wire::tests::chunk_at_exact_max_len_boundary): a payload of exactly MAX_CHUNK_LEN round-trips (the limit is inclusive); the existing chunk_too_large covers over-limit.
P13 discard test (session::tests::read_pump_ignores_client_to_server_stream_types_from_server): stream_type 0 and 3 chunks arriving from the server (mid-stream — a leading 0x00 is the negotiation-error marker, ADR-052 §5, so stream_type 0 cannot be first) are ignored without desynchronizing framing, losing stdout data, or blocking the exit. The remaining P13 item (the TtyAdapter::handle accept-loop harness) stays deferred to the first transport-level consumer, per the review.

Verification on the resolution tree: cargo test 98 lib (was 95) / cargo test --all-features 141 (was 138); clippy (all-targets + wasm32), fmt, wasm check, doc — all clean.

Session-2 Resolution (P12 + P6 + P7 + P4 + P16, 2026-09-05)

The API-shape batch (P12/P6/P7) plus the docs that use the final surface (P4/P16), per the review's suggested session breakdown item 2. Commit: c8e4610.

P12 — crate-root re-exports (src/lib.rs): pub use of the primary types — TtyAdapter, drive_session, drive_session_pre_negotiated, TTY_OPEN_SCOPE (adapter); TtyBackend, TtyHandle, TtyParams, TerminalParams, TtyControl, TtyControlHandle, TtyError, BoxFuture (backend); TtySession, TtySessionError (session); Chunk, ChunkReader, ChunkWriter, RawError and the five STREAM_* constants + CHUNK_HEADER_LEN + MAX_CHUNK_LEN (wire); ControlMessage and signal_from_name — the latter #[cfg(unix)]-gated to match its definition (caught by the wasm check); NegotiateRequest, TerminalParamsWire, NegotiationError, NegotiationReader, NegotiationWriter, error_response_bytes (negotiation); register_openable, tty_open_spec, OP_TTY_OPEN, TTY_ALPN (channels); LocalTtyBackend under the local feature. Module paths remain the full surface; the lib.rs doc comment gained a "Crate-root surface" section explaining the split.
P6 — write-path validation (src/wire.rs): all four ChunkWriter methods validate via a shared validate_header (stream_type ≤ 4, length ≤ MAX_CHUNK_LEN) before touching the transport, reusing RawError::InvalidStreamType/ChunkTooLarge. The length is validated as u64 before the u32 cast, so an oversized payload fails instead of wrapping past the check — validating the truncated value would have reintroduced the exact bug the validation exists to catch. Empty-payload shape unified: one write_validated helper emits length == 0 + no payload bytes for all four paths (the old write_ctrl_* always-write shape is gone; wire-equivalent). Tests: reject-invalid-stream-type (nothing written), reject-oversized (writer + all three helpers), and exact-boundary acceptance.
P7 — peek-state guard (src/wire.rs, src/session.rs): ChunkReader tracks peeked state. read_chunk() after peek_stream_type() completes the peeked chunk instead of consuming a second header byte; a second peek is idempotent; read_chunk_after_peek debug-asserts a peek happened. This let the session's read pump drop its manual first_byte_peeked threading — read_chunk() handles both orders. Tests: peek-then-read_chunk delivers two well-framed chunks (the old shape silently read the peeked byte's length field from the payload), and peek idempotence.
P4 — README + readme field: README.md mirrors alkcall's structure — tagline + positioning, ## Quick start with per-role examples (producer: backends map + TtyAdapter as ProtocolHandler; producer-via-channels: register_openable; consumer: TtySession::connect_direct with the full typed-method flow; consumer-via-channels: open_via_channels), ## Architecture (producer/consumer table per convention 7, wire-format summary, wasm + local note), ## Documentation, ## License. Examples use the P12 root paths. readme = "README.md" added to [package]; the dry-run package list confirms it ships.
P16 — CHANGELOG.md: Keep-a-Changelog format with a full [0.1.0] entry written from the commit log (wire format, backend trait, local backend, adapter, channels integration, session consumer, control messages, plus the Session-1/2 fixes folded into their features), with the [0.1.0] release-tag link definition (alkcall's missing-link nit not replicated). ADR references use the actual on-disk numbering (exit code = ADR-004, cancel-cleanup = ADR-005, framing = ADR-006, channels = ADR-008/009).

Verification on the resolution tree: cargo test 104 lib (was 98) / cargo test --all-features 128 lib + 19 integration = 147 (was 141); clippy (all-targets + wasm32) -D warnings, fmt, wasm check, doc (0 warnings), cargo publish --dry-run --allow-dirty (44 files, 702.8KiB) — all clean.

Session-3 Resolution (P8 + P9 + P10 + P5 + P11 + P2 + P3 + P15, 2026-09-05)

The local robustness batch, the producer-hygiene batch, and the packaging/docs closeout — the review's suggested session breakdown items 3, 4, and 5. This closes review #003.

P8 — poll_shutdown waker (src/local/pty.rs): StdinSink gained an inflight_close slot mirroring poll_write's inflight pattern — on a full stdin channel, poll_shutdown parks a reserve + send(StdinCmd::Eof) future and polls it on re-poll, so the poller's waker is registered and a stdin blast followed by EOF delivers the EOF instead of stranding it. Distinct from the byte-write slot so the two sends never share a future.
P9 — poisoned-lock .expect() ×5 (src/local/pty.rs): the five non-test lock sites (PtyControl::resize, both PtyControl::signal arms, the reader-thread and writer-thread master-lock acquisitions) use unwrap_or_else(|e| e.into_inner()) per convention 2.
P10 — thread-spawn .expect() ×3 (src/local/pty.rs): the reader/writer/waiter thread::Builder::spawn results map to TtyError::AllocFailed (the error path existed — allocate_pty returns Result); fd/thread exhaustion surfaces as an allocation error, not a panic.
P5 — scope gate first (src/adapter.rs): validate_and_allocate now runs the tty:open scope check before the carriage/cmd checks and the backend lookup. An authenticated-but-unscoped identity gets forbidden regardless of the request body — the unknown_backend-vs-forbidden name- enumeration differential is gone. The existing scope_gate_forbidden_without_tty_open test asserted the scoped behavior and still passes (it uses a registered backend name, so the reorder doesn't change its outcome); no test asserted unknown_backend with an unscoped identity.
P11 — sentinel contract (src/session.rs): recv_stdout now terminates the stream ON the zero-length stdout sentinel ("drained") — the sentinel is not yielded as an item. This matches the documented contract and the sentinel's meaning; consumers no longer filter. recv_stderr's doc now states stderr has no sentinel (the adapter's stderr pump emits none; that stream ends when the read pump terminates). The three session tests that filtered the sentinel manually now assert the stream ends on it (no filter). The README example's manual is_empty break was removed.
P2 — packaging (Cargo.toml): exclude is now [".opencode/", "AGENTS.md", "docs/reviews/", "docs/plans/", "docs/sdd_process.md"] — AGENTS.md no longer ships (alkcall parity), docs/plans/ (absolute /workspace/... paths) no longer ships, and both dead entries (Cargo.lock — force-included anyway; docs/research/ — nonexistent) are dropped. Package list verified: 42 files, 659.2KiB (was 44 files, 702.8KiB).
P3 — stale docs: AGENTS.md now says all five phases are landed, the ADR range is 001..009 with the alktty-native ADR-009 noted in the mapping list, and the alkcall guidance is corrected to "v0.4.x — Pin alkcall = "0.4.0" (lockfile resolves 0.4.1)" (was "v0.1.x — pin 0.1.1", which would have downgraded the dependency). docs/architecture/README.md gained the ADR-009 table row, the 001..009 preamble, and the landed-phase status.
P15 — polish batch (one commit, as planned):
- src/backend.rs doc typo fixed ("local feature module) is the reference implementation").
- tokio-stream dev-dependency removed (inherited from the main dep).
- src/adapter.rs — the NegotiationError::Io(_) arm now logs (debug!) for parity with the catch-all arm; set_identity failure is logged (AlreadySet is benign-by-construction, noted in a comment); drop(input_pump) → input_pump.abort() at session end (no lingering client→backend task holding the backend stdin half after the exit chunk).
- src/session.rs — connect_direct/from_bidi_stream_via map the accept_bi StreamError into a new TtySessionError::Open variant (#[from]) instead of flattening it into an anonymous io::Error(ConnectionReset) (additive pre-consumers, same logic as review #002 R5); the fail-fast params parse uses NegotiateRequest::deserialize(&params) (borrowing) instead of from_value(params.clone()) — no clone.
- src/negotiation.rs — error_response_bytes skips an "error" key in fields so it cannot overwrite the error code.
- Two trivial inline comments promoted or removed (the split-idiom comment in adapter.rs removed; the "Already taken" comment in session.rs now points at the documented behavior).
- docs/plans/project-setup.md test counts refreshed (119 lib + 19 integration = 138 under --all-features at the review baseline, with a note that the suites have grown); overview.md and tty-adapter.md front-matter last_updated dates and the ADR range updated for the ADR-009 edits.
- The input_pump.abort() change surfaced a latent test race: the send_stdin_round_trips_to_backend and resize_and_signal_dont_error tests used MockBackend (exit resolves on allocate) and were writing while the session data plane was already closing — previously masked by the lingering input pump the abort now reclaims. Both use a new GatedBackend (exit held until the test releases it), making the writes deterministic and asserting the exit code.

Verification on the resolution tree: cargo test 104 lib (unchanged) / cargo test --all-features 128 lib + 19 integration = 147 (unchanged; the two reworked tests kept their names); clippy (all-targets + wasm32) -D warnings, fmt, wasm check, doc (0 warnings), cargo publish --dry-run --allow-dirty (42 files, 659.2KiB) — all clean.

Review #003 is closed. Published: cargo publish uploaded alktty v0.1.0 to crates.io (2026-09-05, dry-run gate clean without --allow-dirty), and the v0.1.0 tag — the CHANGELOG link definition's target — is pushed. Post-publish iteration is expected at first-consumer time (the alkcall trajectory: unpublished mistakes were free, per-consumer ones are semver cost); the wire format (ADR-001) and the TtyBackend trait shape (ADR-002) are the one-way doors this review scrutinized hardest.

Notes

Line numbers refer to the tree at 918af40 (review base).
The P1 reproduction probe was a scratch crate at /tmp/opencode/alktty-deadlock-probe (mock TtyBackend emitting N chunks through real drive_session over tokio::io::duplex; N=62 passes, N=63 hangs) — ephemeral by design; the P13 regression test makes it permanent.
The two subagent reviews overlapped on findings P1/P5/P6/P7/P9/P10/ P12 and the #[non_exhaustive] question; the overlapping findings agreed, and every one was re-verified against source before being recorded. The packaging agent's file list came from cargo package --list --allow-dirty, not inference.
This review deliberately does not re-open decisions recorded in review #002 (R3's per-connection-registry constraint, R5's #[non_exhaustive] policy) — they are cited where relevant.

Cross-references

Review #001 (docs/reviews/001-code-review.md) — first full review; its M/L/N ID scheme is why this review uses P1…P16 (prepublish).
Review #002 (docs/reviews/002-post-session-review.md) — R5's resolution records the #[non_exhaustive] policy this review applied in "Examined and closed".
alkcall 0.4.1 (/workspace/@alkdev/alkcall) — packaging/README/ CHANGELOG reference for P2/P4/P16; src/channels/operations.rs for P14.
ADR-001 (wire format) and ADR-005 (exit-code chunk) — the invariants P1's fix must preserve.

36 KiB Raw Blame History Unescape Escape