docs: add post-Phase-5 code review (coverage + correctness + smell)
First full review of the alktty crate after Phase 5 landed. Covers coverage (cargo-llvm-cov), correctness, and code smell, with a remediation plan. - 2 Medium: TtySession never handles the negotiation-rejection error frame (M1); wait() swallows MalformedExitChunk (M2) - 6 Low: channels input ignored (L1), consumer stdout/stderr routing untested (L2), open_via_channels 0% covered (L3), mocks leak into public API (L4), cargo fmt fails (L5), pty bridge error paths untested (L6) - 6 Nit: 9 rustdoc warnings (N1), stale doc paths (N2), unsafe vs AGENTS.md §14 (N3), sleep-based timing (N4), duplicated seed helpers (N5), unverified MSRV (N6) Verification: cargo test (81), cargo test --all-features (123), clippy clean (native + wasm), wasm check clean, publish dry-run clean, llvm-cov 90.74% lines / 92.24% functions.
This commit is contained in:
@@ -0,0 +1,577 @@
|
||||
---
|
||||
status: open
|
||||
last_updated: 2026-08-17
|
||||
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
|
||||
|
||||
| ID | Finding | Fix | Effort | Risk |
|
||||
|----|---------|-----|--------|------|
|
||||
| L5 | `cargo fmt` fails | run `cargo fmt`, commit | trivial | none |
|
||||
| N1 | 9 rustdoc warnings | fix 9 link sites | trivial | none |
|
||||
| N2 | stale doc paths | fix 2 paths | trivial | none |
|
||||
| N3 | `unsafe` vs AGENTS.md §14 | amend convention wording | trivial | none |
|
||||
| L4 | mocks leak into public API | `pub(crate)` on `MockBackend`/`MockControl` | trivial | none |
|
||||
| N5 | `rand_seed`/`nanos_seed` ×4 | consolidate | trivial | none |
|
||||
| M2 | `wait()` swallows `MalformedExitChunk` | propagate the variant | small | low |
|
||||
| L2 | consumer stdout/stderr routing untested | add emitting test backend | small | low |
|
||||
| M1 | negotiation-rejection frame unhandled | implement disambiguation read | medium | medium (wire-facing) |
|
||||
| L1 | channels `input` ignored | decide drop-vs-pass-through | small | low |
|
||||
| L3 | `open_via_channels` 0% covered | end-to-end channels consumer test | medium | low |
|
||||
| L6 | pty bridge error paths untested | targeted error-path tests | medium | low |
|
||||
| N4 | sleep-based timing | readiness signals | small | low |
|
||||
| N6 | MSRV unverified | CI MSRV job or bump | small | none |
|
||||
|
||||
### Recommended Order
|
||||
|
||||
1. **L5 + N1 + N2 + N3 + L4 + N5** — the trivial hygiene batch. One
|
||||
commit, no behavior change, makes `cargo fmt`/`cargo doc` clean and
|
||||
the public API tidy. Do this first so the tree passes its own
|
||||
verification gate.
|
||||
2. **M2** — small, low-risk, closes a swallowed-error path in the
|
||||
consumer half.
|
||||
3. **L2** — small, high-value: the consumer half's happy path is
|
||||
currently untested.
|
||||
4. **M1** — the one real protocol gap. Medium effort and wire-facing,
|
||||
so it needs publisher sign-off on the disambiguation approach before
|
||||
implementation.
|
||||
5. **L1 + L3** — the channels consumer path; do together since L3's
|
||||
test will exercise L1's code.
|
||||
6. **L6** — pty bridge error paths; medium effort, lower priority than
|
||||
the consumer-half work.
|
||||
7. **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 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.
|
||||
Reference in New Issue
Block a user