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

---
status: fully-resolved
last_updated: 2026-09-05
reviewed_artifacts:
  - 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
tool: manual source read + cargo test/clippy/fmt/doc + cargo-llvm-cov
reviewer: 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`:

```rust
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:

```rust
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

All findings are resolved — see the resolution sections below for
dates and commit references.

| 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 | ✅ resolved (2026-09-05) |
| L3 | `open_via_channels` 0% covered | end-to-end channels consumer test | medium | low | ✅ resolved (2026-09-05) |
| L6 | pty bridge error paths untested | targeted error-path tests | medium | low | ✅ resolved (2026-09-05) |
| N4 | sleep-based timing | readiness signals | small | low | ✅ resolved (2026-09-05) |
| N6 | MSRV unverified | CI MSRV job or bump | small | none | ✅ resolved (2026-09-05) |

### 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).

### Resolution (2026-09-05, L1 + L3 — the channels consumer path)

**L1 — resolved via the publisher decision: the channels path carries no
second negotiation frame (ADR-009 in `docs/architecture/decisions/`).**

Two upstream alkcall changes were prerequisites (the review's premise
that the registry "validates `input`" was wrong — alkcall never
enforced `input_schema` on any dispatch path):

- alkcall 0.4.0 — `OperationSpec.input_schema` is now enforced at call
  time by all three registry dispatch entry points (`invoke`,
  `invoke_streaming`, `invoke_sink`), compiled once at registration
  (fail-closed, same rule as `publish_schema`/CF-003). Violations
  return `INVALID_INPUT`. The `channels/tty/sub` spec's input schema
  now declares the shared `NegotiateRequest` fields (`carriage`,
  `backend`, `cmd` required); `tty`/`cwd`/`env`/backend-params stay
  free-form (raw JSON Schema is permissive on unknown keys, so the
  opaque ADR-053 params pass through).
- alkcall 0.4.1 — early-arrival chunks for a not-yet-adopted channel
  are parked (bounded per-channel buffer) and drained on
  `adopt_channel`, instead of dropped. The open-op-response /
  producer's-first-write race silently lost the first chunks of any
  push-first producer — found by L3's test (the session never resolved
  because the stdout sentinel + exit chunk of an
  immediately-resolving backend arrived before the adopt).

Design: the open op's `input` IS the negotiation (design 2 of the
publisher decision). `make_tty_open_handler` parses the
registry-validated `input` into a `NegotiateRequest` and drives
`drive_session_pre_negotiated` (new public API in `adapter.rs`) — the
same three-pump session driver as the direct path, minus the
wire-frame negotiation phase. Validation still runs
(`carriage`/`cmd`/backend lookup + ADR-050 ownership); failures go to
the client as a `0x00`-prefixed negotiation error frame on the channel
stream, so the M1 disambiguation read applies unchanged. The
`tty:open` scope gate is enforced by the registry's `AccessControl`
(not re-checked in the handler). `tty_open_spec()`'s schema is now the
partial `NegotiateRequest` shape. `TtySession::open_via_channels`
parses `params` locally (fail-fast before a channel is allocated),
opens the channel, and starts raw-chunk mode directly
(`from_halves_raw` — no negotiation write, peek retained for the
error-frame path).

Tests: `open_via_channels_end_to_end_negotiates_and_waits`,
`open_via_channels_surfaces_negotiation_rejected`,
`open_via_channels_fails_fast_on_schema_invalid_params`,
`open_via_channels_fails_fast_on_unparseable_params`,
`open_via_channels_routes_backend_stdout_and_stderr`,
`pre_negotiated_happy_path_over_plain_duplex` (adapter), plus the
shared harness tests in `src/testing.rs` (harness-level handler→client
data flow). The channels harness (`wire_client_and_server`) moved to
`crate::testing` so session tests share it with channels tests.

**L3 — resolved** by the same work: `open_via_channels` (and
`from_bidi_stream_via`) are covered end-to-end against the real
producer path (`register_openable` + `drive_session_pre_negotiated`
through alkcall's channels stack).

### Resolution (2026-09-05, L6 — pty bridge error paths)

Per the review's own disposition ("the `try_clone_reader`/`take_writer`
failure paths can be left as documented-unreachable if the publisher
agrees"), the bridge's error arms are now **documented-unreachable**
(module doc in `local/pty.rs` §"Bridge error paths"), with the
reasoning per arm: `try_clone_reader` needs a dup failure (exhausted
fd table — not deterministically forceable); a reader read error has
no trigger (EIO → EOF is mapped); `take_writer` only fails on a second
take (the bridge takes it once); a writer write/flush error requires
an externally-closed fd; the waiter `wait()` failure needs an
already-reaped child (the bridge never calls `try_wait`). The
ADR-055 §4 `-1` sentinel the review wanted tested is covered at the
adapter level (`exit_error_sends_minus_one` — the sentinel also arises
when the oneshot drops on kill-on-cancel, exercised by the
cancel-cleanup tests).

The reachable fallback chain *is* now tested:
`signal_after_child_exit_takes_both_kill_fallbacks` — a late signal
(after the child exited) takes `kill(-pgid)` fail → `kill(pid)` fail →
warn + return, with no panic. `local/pty.rs` line coverage
80.85% → 86.01%; the remaining uncovered lines are exactly the
documented-unreachable arms plus `StdinSink`'s in-flight-parking path
(a single write cannot fill the 64-slot channel).

### Resolution (2026-09-05, N4 — readiness signals replace fixed sleeps)

The signal tests now use a **marker-file readiness signal** (the
pattern the cancel-cleanup tests already used): the child's command is
`echo ready > <marker>; exec sleep 60`, and the test polls
`wait_for_file(marker, 5s)` — the marker existing means the shell
exec'd, so the signal lands on the real target regardless of machine
load. Applied in `tests/pty.rs` (both signal tests), `tests/pipe.rs`
(SIGTERM test), and the `src/local/` unit tests (`signal_int_kills_child`,
`signal_reaches_process_group_child`, `unknown_signal_*`,
`signal_term_kills_child`). `wait_for_file` lives in
`tests/common/mod.rs`; `src/local/` copies are private test helpers
(separate compilation units).

The fixed **post-action** sleeps in the cancel-cleanup tests (wait
after drop, then probe once) became **bounded polls** for the child's
death (`kill(pid, 0)` → ESRCH) with a 5 s deadline — faster and
flake-proof in both directions.

The resize/cat-stdin tests needed no readiness signal at all: the
adapter's input pump processes chunks in order and `resize` is safe
whenever the control handle exists — the sleeps there were pure
latency. Integration suites now finish in ~40 ms (was 200-270 ms
each).

### Resolution (2026-09-05, N6 — MSRV verified at 1.85)

`cargo +1.85 check` passes for the default crate and `--all-features`,
plus `cargo +1.85 check --target wasm32-unknown-unknown` and a full
`cargo +1.85 test --all-features` (all 136 tests). The declared MSRV
1.85 is real.

The only friction was transitive: `idna_adapter@1.2.2` (via
`idna` ← `jsonschema` ← alkcall) requires rustc 1.86, and the icu
v2.3 crates require 1.88. The Cargo.lock now pins the 1.85-compatible
set (`jsonschema 0.46.9`, `idna_adapter 1.2.0`, icu crates at 2.0.x)
— verified on stable too. If a future dependency bump silently raises
the real MSRV, `cargo +1.85 check` in CI will catch it (the toolchain
is installed via `rustup toolchain install 1.85 --profile minimal` +
`rustup target add wasm32-unknown-unknown --toolchain 1.85`; no CI
job exists yet, so the command here is the check until one does).

### Remaining (open)

None — the review is fully resolved.

---

## 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` and the 2026-09-05 L1+L3
  resolution.
- 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.