alktty/docs/reviews/002-post-session-review.md

status, last_updated, reviewed_artifacts, tool, reviewer, base_commit

status	last_updated	reviewed_artifacts	tool	reviewer	base_commit
resolved	2026-09-05	docs/architecture/decisions/009-channels-open-op-is-the-negotiation.md src/adapter.rs src/channels.rs src/session.rs src/testing.rs src/lib.rs src/wire.rs src/negotiation.rs src/local/pty.rs src/local/pipe.rs tests/common/mod.rs tests/pipe.rs tests/pty.rs Cargo.toml Cargo.lock docs/reviews/001-code-review.md	manual source read + alkcall 0.4.1 source read + cargo test/clippy/fmt/doc	post-session follow-up review (the 2026-09-05 resolution commits)	6ad1d84 (review

Code Review #002 — Follow-up on the 2026-09-05 Resolution Session

Purpose

A post-hoc review of the five commits that finished review #001 (9327a73, 96692d3, a734842, cdd6893, 6ad1d84). The session resolved L1 + L3 (the channels-path negotiation redesign), L6 (pty bridge error-path tests), N4 (readiness signals), and N6 (MSRV), plus a cargo cleanup (9327a73).

The session's verification claims were all reproduced clean; the findings below are documentation, process, and design-consistency items the session left behind — not correctness bugs in what shipped.

Verification Baseline

All verification re-run on the reviewed tree (6ad1d84, before this review's own commit):

• cargo test: 93 lib tests pass (default, wasm-clean build).
• cargo test --all-features: 136 tests pass (117 lib + 19 integration). Zero failures.
• cargo clippy --all-targets -- -D warnings: clean.
• cargo clippy --target wasm32-unknown-unknown -- -D warnings: clean.
• cargo fmt --check: clean.
• cargo doc --no-deps: 0 warnings.
• alkcall pinned at 0.4.1 (Cargo.lock); alkcall = "0.4.0" in Cargo.toml.
• CJK scan (rg "[\p{Han}]" excluding target/ + lockfile): zero matches in the repo — the session-summary language did not leak into any source, doc, or commit message.

---

Findings

R1. ADR-009 cited but never written

Files: src/adapter.rs:214, src/session.rs (5 sites), src/channels.rs:174, docs/reviews/001-code-review.md:578

Problem: commit 96692d3 and the review-#001 resolution section attribute the "open op's input is the negotiation" design to ADR-009 in docs/architecture/decisions/ — but the directory stopped at 008. A wire-adjacent design decision (the channels path carries no second negotiation frame) shipped with its decision record missing. Source doc comments referenced a file that did not exist, so a reader following the link found nothing.

Resolution (2026-09-05, 37ae07a): ADR-009 written (009-channels-open-op-is-the-negotiation.md) — context (the L1 two-gate disagreement, the alkcall 0.4.0/0.4.1 prerequisites, the version-skew caveat), producer/consumer design, consequences, and door type. overview.md ADR index and tty-adapter.md reference it.

---

R2. Stale docs left by the L1 redesign

Files: src/channels.rs (module doc + register_openable doc), docs/architecture/tty-adapter.md

Problem: the redesign removed the wire-frame negotiation read from the channels path, but two docs still described it:

• channels.rs module doc said the open handler "runs [drive_session] on it — the same code path the direct-ALPN TtyAdapter uses" — the exact behavior the commit removed.
• register_openable's doc described the input schema as "permissive … validated by the drive_session negotiation reader" — the reader no longer runs on this path, and alkcall 0.4 now enforces the schema at dispatch.
• tty-adapter.md said "the same drive_session runs in both direct and channels modes" and its ADR table had no ADR-009 row.

Resolution (2026-09-05, 37ae07a): all three aligned with drive_session_pre_negotiated and the enforced partial NegotiateRequest schema; tty-adapter.md gained the ADR-009 row and a paragraph contrasting the two paths' negotiation sources.

---

R3. Producer identity on the channels path is an install-time snapshot — accepted design

Files: src/channels.rs:109,203,209

Problem (as reviewed): register_openable captures auth.identity at install time and closes over it; the open handler then uses identity.clone().or_else(|| auth.identity.clone()) (channels.rs:209). The producer's scope/ownership checks therefore run against the identity the connection was authenticated with at install time, not a per-open resolution. If a registry were ever shared across connections (not the intended assembly — alkcall builds a per-connection registry in the install_channel_zero hook), every open would be attributed to the first connection's identity.

Disposition (publisher, 2026-09-05): closed as intended. The design predates the alkcall split (it comes from the alknet era): keep connections separate and build the overlay/proxy explicitly. A worker runs alktty alongside other call ops (file ops, sftp) and connects to a hub; the hub owns the connection and proxies the resource on top of it, with ACL at the hub. That keeps the auth model straightforward — the hub is the principal that owns the connection, and per-connection registries are the intended assembly shape. The or_else(auth.identity) fallback is belt-and-suspenders for an install path that did not carry an identity.

Guard: this holds only as long as the registry stays per-connection (the ADR-047 §4 amendment shape). If a future refactor ever shares one OperationRegistry across connections, the closed-over identity becomes wrong for every connection after the first — that refactor must pass identity per-open instead. Recorded here so the constraint survives the session that introduced it.

---

R4. Schema-invalid input that parses as NegotiateRequest dies silently (no error frame)

Files: src/channels.rs:212-218 (make_tty_open_handler)

Problem: the producer's post-open failure paths (unknown backend, allocate_failed, ownership denial) write the 0x00-prefixed negotiation error frame so the consumer's disambiguation peek surfaces NegotiationRejected. But a NegotiateRequest parse failure of the schema-validated input takes a third path: log + return, no error frame. The channel tears down (wrapper teardown), the consumer's peek observes a clean close, and the session reports the much less specific NoExitChunk — the same symptom as a producer that crashed without ever responding.

In practice this is currently unreachable: the schema requires carriage/backend/cmd, and a value satisfying those almost always parses (carriage: "raw" and non-empty string-array cmd are the only additional semantic checks, both owned by validate_and_allocate, which does write the error frame). The gap is the asymmetry itself — three failure classes, two visible to the consumer — and it is undocumented.

Fix options (deferred — either is small):

1. Write the error frame on parse failure (serialize a malformed_negotiation frame before returning). Symmetric with the other post-open failures; the M1 peek handles it unchanged.
2. Document the asymmetry in the handler's doc comment: parse failures are pre-validation, schema-checked, and effectively unreachable; only the semantic failures are client-visible.

Option 1 is preferred if a test can be written for it (feed a handler a schema-bypassing value directly — the unit-testable seam is make_tty_open_handler with a hand-built input).

Resolution (2026-09-05): option 1 implemented. make_tty_open_handler accepts the channel's BiStream and writes a malformed_negotiation error frame via the shared crate::adapter::send_negotiation_error (now pub(crate)) before returning — all three post-open failure classes are client-visible through the unchanged M1 peek. The review's reachability analysis is refined by the R5 fail-fast discovery: TtySession::open_via_channels parses params locally before opening, so the consumer never sends a value it can't parse itself — but the producer-side handler is still the reachable seam for direct ChannelClient callers (the schema is deliberately partial; a schema-valid cwd: 42 fails the typed parse). Tests: the make_tty_open_handler seam test (channels.rs) and a real-registry end-to-end test via ChannelClient::open_channel (testing.rs). ADR-009 amended (§"Parse-failure error frame"); tty-adapter.md error table updated.

---

R5. open_via_channels fail-fast surfaces as NegotiationSerialize

Files: src/session.rs:208-209

Problem: the local pre-open parse (serde_json::from_value::< NegotiateRequest>(params.clone())) fails fast before a channel is allocated — good — but the error surfaces as TtySessionError::NegotiationSerialize (#[from] serde_json::Error), whose name and doc say "the negotiation frame failed to serialize." A consumer debugging a bad params value sees a serialize-framing error that has nothing to do with writing a frame. The two fail-fast tests (open_via_channels_fails_fast_on_*) pin this mislabeled variant.

Fix (small, semver-visible): TtySessionError is not #[non_exhaustive] (unlike TtyError, backend.rs:44), so adding a variant is a breaking change — the same reason the session tests assert on NegotiationSerialize(_) today. Options:

1. Add InvalidParams(String) (or ChannelsParams) and map the fail-fast path to it. Additive variant; requires matching exhaustively anywhere the enum is matched (in-crate: the session tests). Do it alongside a #[non_exhaustive] decision for the enum before the first released consumer (the enum is part of the public surface — decide before, not after).
2. Cheaper: reword the NegotiationSerialize doc to note the channels-path fail-fast reuse. No API change, still mislabeled.

Original disposition (superseded — see resolution): deferred until the first channels-path consumer exists (per the L1 version-skew note in ADR-009); option 1 at that point.

Rationale for superseding the deferral (2026-09-05): the deferral trigger was circular — "the first channels-path consumer exists" only fires after the change has become expensive (an exhaustive match in that consumer turns the additive variant into a break), so deferring converted a zero-risk change into a medium-risk one by waiting. The ADR-009 citation was also misapplied: the L1 version-skew note governs wire skew (the removed negotiation frame), not API-surface error enums — ADR-009 doesn't decide this. And the "effectively unreachable" framing belonged to R4's producer-side arm, not R5's: the fail-fast path is live today (any open_via_channels caller with bad params gets NegotiationSerialize now). The substantive pending item was the #[non_exhaustive] policy decision, which is answerable on principle without consumers: TtyError is already #[non_exhaustive] (backend.rs:44, the two-way-door extension pattern); alkcall's consumer-facing AdapterError is #[non_exhaustive] for exactly this reason; NegotiationError and RawError mirror fixed wire semantics and stay exhaustive (deliberate — in-crate matchers keep exhaustiveness checking).

Resolution (2026-09-05): option 1 implemented pre-publish, when adding a variant is additive (the crate is unpublished — there are no consumers to break): TtySessionError is now #[non_exhaustive] (with the policy rationale in its doc), the fail-fast path maps to the new InvalidParams(String) variant, and the two fail-fast tests assert InvalidParams(_). No exhaustive match on TtySessionError exists in-crate or in tests/ (the tests use matches!, unaffected by #[non_exhaustive]).

---

What's Good

• The L1 refactor preserved semantics exactly. validate_and_allocate is a faithful extraction: same checks in the same order (carriage, cmd, backend lookup, scope, ownership, allocate), same error codes/fields, same error-frame-on-failure behavior. The enforce_scope flag is the only policy difference and it is documented at both call sites.
• The post-open error frame flows through the M1 peek on the channels path unchanged (from_halves_raw retains the peek) — the redesign did not quietly drop the disambiguation contract.
• The new end-to-end tests are genuine. They run the real register_openable producer through alkcall's channels stack (adapter → install hook → registry dispatch → open wrapper), not a mock of either side. The L2-emitting-backend test over the channels data plane asserts routing of real stdout/stderr bytes.
• The race bug found via L3 is real and the fix is the right shape. Verified in alkcall 0.4.1 source: route_payload parks unknown-channel_id payloads in a bounded FIFO (EARLY_ARRIVAL_CAP = 64), drained by adopt_channel — the open-op-response / first-write race is closed for push-first producers.
• The documented-unreachable arms in local/pty.rs hold. Each per-arm claim was checked against the code (try_clone_reader needs a dup failure; EIO→EOF is mapped in the PtyFd Read impl; take_writer fails only on second take; wait() failure needs an already-reaped child) — the reasoning is sound, and the reachable late-signal fallback chain got a real test.
• The N4 conversion is correct in both directions. Marker-file readiness (echo ready > marker; exec sleep 60) means the signal lands on the exec'd process, and the bounded death-polls (kill(pid,0) → ESRCH, 5 s deadline) replace both the flake-prone sleeps and the flake-prone single-probe asserts. The no-signal reasoning for the resize/cat-stdin tests is sound: the adapter's input pump processes chunks in order, so ordering, not timing, carries the test.
• The N6 lockfile pins are real. idna_adapter 1.2.0 and the icu 2.0.x/1.5.x set are pinned in Cargo.lock, matching the 1.85 claim.
• No CJK in the repo. The session summary's language choice did not leak into any file.

Remediation Plan

ID	Finding	Fix	Effort	Risk	Status
R1	ADR-009 never written	write the ADR	small	none	✅ resolved (37ae07a)
R2	stale docs from the L1 redesign	align with ADR-009	trivial	none	✅ resolved (37ae07a)
R3	install-time identity snapshot	accepted design (hub-proxy rationale)	none	none	✅ closed as intended
R4	silent death on parse-failure path	error frame or documented asymmetry	small	low	✅ resolved (option 1)
R5	NegotiationSerialize mislabel on fail-fast	additive variant (with #[non_exhaustive] decision)	small	medium (semver)	✅ resolved (option 1 + #[non_exhaustive], pre-publish)

Both R4 and R5 were originally deferred as consumer-surface churn, but the deferral rationale didn't hold (circular trigger, misapplied ADR-009 citation — see R5's superseded-disposition note). Both were resolved pre-publish, while the changes are additive by construction: R4 via the error frame (no API change), R5 via InvalidParams + #[non_exhaustive] on TtySessionError (additive while the crate is unpublished; the enum policy is now written down in its doc).

Notes

• Line numbers refer to the tree at 6ad1d84 (review base) with the 37ae07a doc fixes applied.
• This review deliberately does not re-litigate the alkcall 0.4.0/0.4.1 upstream changes (publisher-watched, accepted) or the alkhttp compatibility bumps.
• The R3 disposition records the publisher's rationale so the design constraint (per-connection registries) is written down where a future refactor will look.

Cross-references

• Review #001 (docs/reviews/001-code-review.md) — the findings this session resolved; its "Resolution (2026-09-05, L1 + L3)" section is the session's own account of 96692d3.
• ADR-009 (docs/architecture/decisions/009-channels-open-op-is-the-negotiation.md) — the decision record R1 added.
• alkcall 0.4.1 src/channels/manager.rs — the early-arrival parking (route_payload / park_early_arrival / drain_early_arrivals, EARLY_ARRIVAL_CAP = 64).