Review #003 Session 2 (P12 + P6 + P7 + P4 + P16):
- P12: primary types re-exported at the crate root (TtyAdapter,
TtySession, TtyBackend/TtyHandle/TtyParams, Chunk codec, wire
constants, ControlMessage, negotiation types, channels helpers;
LocalTtyBackend under `local`). signal_from_name re-export is
#[cfg(unix)]-gated to match its definition (wasm check caught it).
- P6: ChunkWriter validates before writing — stream_type > 4 or
payload > MAX_CHUNK_LEN fails locally (RawError) instead of
corrupting the peer's framing; length validated as u64 before the
u32 cast so oversized payloads cannot wrap past the check.
Empty-payload shape unified via a single write_validated helper.
- P7: ChunkReader tracks peeked state — read_chunk() after
peek_stream_type() completes the peeked chunk instead of consuming
a second header byte; second peek is idempotent;
read_chunk_after_peek debug-asserts a peek. Session read pump drops
its manual first_byte_peeked threading.
- P4: README.md (alkcall structure; quick-start examples per role
using the new root paths) + readme = "README.md" in [package].
- P16: CHANGELOG.md with the full [0.1.0] entry and tag link.
Verification: cargo test 104 lib (default) / 147 (--all-features);
clippy all-targets + wasm32 -D warnings; fmt; wasm check; doc 0
warnings; publish dry-run 44 files OK.
Session-1 resolution section records the concurrent-drainer fix shape,
the bug-validated regression test (fails with the old order
reinstated), the two P13 boundary/discard tests, and the deferred
accept-loop harness item. Verification counts updated (98 lib / 141
--all-features).
The stdout drainer started only after the pumps+exit join completed,
but the pumps share a bounded (64-slot) channel with it. Once the
channel filled, pump_stdout parked on send, the join never completed,
and the session hung with the client actively reading (review #003 P1;
reproduced empirically at N=62 ok / N=63 hang). Any real session (cat,
ls -R) exceeds 64 chunks instantly.
pump_session now spawns drain_chunks (owning writer_rx + client_write)
before the join; the exit chunk is still enqueued after the join and
writer_tx is dropped after it, so the single FIFO preserves the
exit-chunk-is-last invariant (ADR-005).
Tests (review #003 P13):
- backpressure regression: 80 chunks through a real drive_session,
in-order delivery + sentinel + exit asserted, 10s per-read timeout
so a regression fails instead of hanging (validated against the bug:
fails with the old order reinstated)
- wire: MAX_CHUNK_LEN exact-boundary round trip (limit is inclusive)
- session: stream_type 0/3 from the server ignored mid-stream without
desynchronizing framing or losing stdout/exit
Verification: cargo test 98 lib (default) / 141 --all-features,
clippy all-targets + wasm32 -D warnings, fmt --check, wasm check, doc
--no-deps all clean.
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.
Review #002 R5 — the open_via_channels fail-fast parse (a params value
that fails the local NegotiateRequest parse before a channel is
allocated) surfaced as NegotiationSerialize, whose name and doc
describe serializing the negotiation frame, not parsing open-op params.
- add TtySessionError::InvalidParams(String); the fail-fast path maps
to it (the serde_json::Error's From impl stays for
NegotiationSerialize's real users — the direct-path serialize)
- mark TtySessionError #[non_exhaustive] — the same two-way-door
pattern as TtyError (backend.rs) and alkcall's consumer-facing
AdapterError; the policy rationale is in the enum's doc
- NegotiationError / RawError stay exhaustive (deliberate — they
mirror fixed wire semantics; in-crate matchers keep exhaustiveness
checking)
- the two fail-fast tests assert InvalidParams(_) now
- review #002: R5 resolved; the superseded deferral rationale is
recorded (circular trigger — "first channels-path consumer exists"
fires after the change becomes expensive; misapplied citation —
ADR-009's version-skew note governs wire skew, not error enums;
the "unreachable" framing belonged to R4's arm, not R5's — the
fail-fast path is live today). The #[non_exhaustive] policy is
decided on principle, pre-publish, while the variant addition is
additive by construction
Verification: cargo test 95 lib (default) / 138 (--all-features);
clippy -D warnings native + wasm clean; fmt clean; doc 0 warnings.
Review #002 R4 — a NegotiateRequest parse failure of the open op's
schema-validated input died silently (log + return, channel teardown,
consumer observed NoExitChunk — indistinguishable from a crashed
producer), while the other post-open failure classes (unknown backend,
allocate_failed, ownership denial) wrote the 0x00-prefixed error frame.
- make_tty_open_handler now accepts the channel's BiStream and writes
a malformed_negotiation frame via the shared
crate::adapter::send_negotiation_error (now pub(crate)) before
returning; the consumer's M1 peek surfaces NegotiationRejected
unchanged
- the frame type and layout are unchanged (ADR-001 wire-stable
contract); no new frame type, no wire change
- tests: make_tty_open_handler seam test with a hand-built
schema-bypassing input (cwd: 42) + a real-registry end-to-end test
via ChannelClient::open_channel (bypasses open_via_channels's local
fail-fast parse — R5's path — so it exercises the producer handler)
- docs: ADR-009 amended (Parse-failure error frame section);
tty-adapter.md malformed_negotiation row covers both paths;
session.rs post-open failure lists updated; review #002 R4 resolved
Note: the review's "unreachable end-to-end" premise was refined —
open_via_channels parses params locally (fail-fast) so a TtySession
consumer never hits the producer-side parse failure, but direct
ChannelClient callers do; the schema is deliberately partial so a
schema-valid value (cwd typed as a number) reaches the handler.
Verification: cargo test 95 lib (default) / 138 (--all-features);
clippy -D warnings native + wasm clean; fmt clean; doc 0 warnings.
Documents the post-hoc review of the five resolution commits:
- R1 (ADR-009 missing) and R2 (stale L1-redesign docs) — resolved in
37ae07a
- R3 — install-time identity snapshot on the channels path, closed as
intended (hub-proxy design predating the alkcall split; constraint
recorded: registries must stay per-connection)
- R4 (silent death on the parse-failure path, no error frame) and R5
(NegotiationSerialize mislabel on the fail-fast path) — open,
deferred to the first post-1.0 error-surface decision
Also cross-links review #001's L1 resolution to ADR-009 and review #002.
Verification: cargo test 93 lib pass; fmt clean.
Review of the 2026-09-05 session commits:
- the ADR-009 decision cited by 96692d3 and docs/reviews/001 was never
written — added decisions/009-channels-open-op-is-the-negotiation.md
(context: the L1 two-gate disagreement, alkcall 0.4.0/0.4.1
prerequisites, producer/consumer design, consequences, door type)
- channels.rs module doc + register_openable doc still described the
old wire-frame negotiation read (drive_session) — aligned with
drive_session_pre_negotiated and the enforced input schema
- tty-adapter.md: session-driver section + ADR tables now reference
ADR-009 and the pre-negotiated driver; overview.md ADR index row
Verification: cargo test 93 lib (default) / 136 (--all-features);
clippy -D warnings native + wasm clean; fmt clean; doc 0 warnings.
- cargo +1.85 check passes (default + --all-features), plus wasm
target check and a full +1.85 test --all-features run (136 tests).
The declared MSRV is real, not aspirational.
- the lockfile pins the 1.85-compatible transitive set (jsonschema
0.46.9, idna_adapter 1.2.0, icu crates 2.0.x) — idna_adapter 1.2.2
requires rustc 1.86, icu 2.3 requires 1.88; stable still resolves
and all tests pass.
- review #001 is now fully resolved (status: fully-resolved). A CI
MSRV job can gate on 'cargo +1.85 check' once CI exists.
- signal tests use a marker-file readiness signal: the child's command
is 'echo ready > <marker>; exec sleep 60', the test polls
wait_for_file(marker, 5s) — marker exists = 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), and
the src/local unit tests; wait_for_file lives in tests/common.
- cancel-cleanup post-action sleeps became bounded polls for the
child's death (kill(pid,0) -> ESRCH, 5s deadline) — faster and
flake-proof in both directions.
- resize/cat-stdin tests need no readiness signal at all: the adapter's
input pump processes chunks in order — the sleeps there were pure
latency (integration suites now ~40ms, was 200-270ms).
- 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 — the reachable part of the
REQ-TTY-02 fallback chain.
- the remaining bridge error arms are documented-unreachable through
the public path (per the review's disposition), with per-arm
reasoning in the module doc: try_clone_reader (dup failure),
reader read error (EIO -> EOF mapped), take_writer (second-take
only), writer write/flush (externally-closed fd), waiter wait()
(already-reaped child). The ADR-055 §4 -1 sentinel is covered at
the adapter level (exit_error_sends_minus_one) — it also arises
when the oneshot drops on kill-on-cancel.
- local/pty.rs line coverage 80.85% -> 86.01%; total 94.48%.
The channels path no longer carries a second negotiation frame on the
channel's data stream (ADR-009). The open op's registry-validated
input IS the negotiation:
- producer: make_tty_open_handler parses the open op's input into a
NegotiateRequest and drives the new drive_session_pre_negotiated
(same three-pump driver as drive_session, minus the wire-frame
negotiation phase; validate/allocate factored into
validate_and_allocate, shared by both paths). Post-open failures
(unknown backend, allocate_failed, ownership denial) still go to the
client as a 0x00-prefixed negotiation error frame, so the consumer's
M1 disambiguation read applies unchanged. The tty:open scope gate is
enforced by the registry's AccessControl (not re-checked in the
handler).
- consumer: 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; the
0x00-error-frame peek retained).
- tty_open_spec's input schema is now the partial NegotiateRequest
shape (carriage/backend/cmd required; backend params stay free-form
— raw JSON Schema is permissive on unknown keys).
Prerequisites landed upstream: alkcall 0.4.0 enforces
OperationSpec.input_schema at dispatch (the registry check this design
leans on never existed before); alkcall 0.4.1 parks early-arrival
chunks for un-adopted channels instead of dropping them — the open
response / producer's-first-write race was silently losing the first
chunks (found by L3's test; the session never resolved).
L3: open_via_channels + from_bidi_stream_via now covered end-to-end
(5 session tests + pre-negotiated adapter test + shared-harness tests
in the new crate::testing module; the channels harness moved there so
session tests share it).
Verification: cargo test 93 lib (default), 116 lib + 19 integration
(--all-features); clippy -D warnings native + wasm clean; fmt clean;
doc 0 warnings; wasm check clean.
- alkcall 0.1.1 -> 0.3.1 (crates.io latest). No API breakage in the
surfaces alktty uses (core, channels, registry); all verification
gates pass unchanged.
- remove criterion + alktype dev-deps and the [[bench]] section: the
wire_vs_bast benchmark was extracted to the alktype project, leaving
this config dead.
glm-5.3-flash