docs: add review #003 (prepublish review for v0.1.0)
Two subagent reviews (code + packaging/docs vs alkcall 0.4.1) with every
finding manually re-verified against source at 918af40. 13 open findings:
P1 pump_session deadlock at >=63 stdout chunks (empirically reproduced,
N=62 ok / N=63 hangs), P2 AGENTS.md ships in the crate, P3 stale
AGENTS.md/architecture-README text, P4 no README, P5-P13 API-shape and
robustness items, P15-P16 polish/changelog. P14 closed as alkcall parity.
Includes suggested session breakdown for distributing the remediation.
Verification baseline on the reviewed tree: cargo test 95 lib (default) /
138 --all-features, clippy (all-targets + wasm32) clean, fmt clean, doc
clean, publish --dry-run OK, CJK scan clean.
This commit is contained in:
@@ -0,0 +1,525 @@
|
||||
---
|
||||
status: open
|
||||
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
|
||||
- AGENTS.md
|
||||
- docs/architecture/README.md
|
||||
- docs/plans/project-setup.md
|
||||
tool: two subagent reviews (code review + packaging/docs vs alkcall 0.4.1) + manual
|
||||
verification of every finding against source + empirical deadlock reproduction
|
||||
reviewer: prepublish review for v0.1.0 (subagent-assisted)
|
||||
base_commit: 918af40 (review #002 resolved)
|
||||
---
|
||||
|
||||
# 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(¶ms)`… 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) | open |
|
||||
| P2 | AGENTS.md ships; dead exclude entries | fix `exclude` | trivial | none | open |
|
||||
| P3 | stale AGENTS.md + architecture README | refresh phase/ADR/alkcall text | trivial | none | open |
|
||||
| P4 | no README + `readme` field | write README (after P5/P6/P12) | medium | none | open |
|
||||
| P5 | scope gate after backend lookup | reorder checks | trivial | low (test updates) | open |
|
||||
| P6 | write paths accept invalid chunks | validate on write; unify empty-payload shape | small | low | open |
|
||||
| P7 | read-after-peek framing corruption | peeked-state guard | small | none | open |
|
||||
| P8 | poll_shutdown missing waker | inflight-future pattern | small | low | open |
|
||||
| P9 | poisoned-lock `.expect()` × 5 | `into_inner()` | trivial | none | open |
|
||||
| P10 | thread-spawn `.expect()` × 3 | map to `TtyError` | trivial | none | open |
|
||||
| P11 | recv_stdout sentinel contract | terminate stream on sentinel (or fix doc) | small | low (semver-visible) | open |
|
||||
| P12 | no crate-root re-exports | `pub use` primary types | trivial | none | open |
|
||||
| P13 | test gaps | backpressure (>64 chunks) + boundary tests | small | none | open |
|
||||
| P14 | `Result<(), String>` on register_openable | closed — alkcall parity | none | none | ✅ closed |
|
||||
| P15 | minor polish batch | one batched commit | small | none | open |
|
||||
| P16 | no CHANGELOG.md | start changelog with [0.1.0] | small | none | open |
|
||||
|
||||
## Suggested Session Breakdown
|
||||
|
||||
Natural break points, cheapest-dependency order first:
|
||||
|
||||
1. **P1 + P13** — the deadlock and its regression test are one unit
|
||||
(the test proves the fix). Highest value; do first.
|
||||
2. **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.
|
||||
3. **P8 + P9 + P10** — the `local` robustness batch, all in `pty.rs`.
|
||||
4. **P5 + P11** — producer-hygiene batch (scope reorder + sentinel
|
||||
contract decision).
|
||||
5. **P2 + P3 + P15** — packaging/docs closeout; then final
|
||||
`cargo publish --dry-run` and tag.
|
||||
|
||||
## 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.
|
||||
Reference in New Issue
Block a user