docs: add review #002 (follow-up on the 2026-09-05 resolution session)
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.
This commit is contained in:
@@ -575,7 +575,8 @@ 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/`).**
|
||||
second negotiation frame (ADR-009 in `docs/architecture/decisions/`,
|
||||
added post-session in `37ae07a` — see review #002 R1).**
|
||||
|
||||
Two upstream alkcall changes were prerequisites (the review's premise
|
||||
that the registry "validates `input`" was wrong — alkcall never
|
||||
@@ -709,6 +710,9 @@ None — the review is fully resolved.
|
||||
commit on `main` at review time). The resolution section above
|
||||
reflects the tree at commit `9944153` and the 2026-09-05 L1+L3
|
||||
resolution.
|
||||
- A follow-up review of the 2026-09-05 resolution commits (including
|
||||
this ADR's addition and the L1 redesign's doc fallout) is at
|
||||
`docs/reviews/002-post-session-review.md`.
|
||||
- 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`.
|
||||
|
||||
@@ -0,0 +1,288 @@
|
||||
---
|
||||
status: partially-resolved
|
||||
last_updated: 2026-09-05
|
||||
reviewed_artifacts:
|
||||
- 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
|
||||
tool: manual source read + alkcall 0.4.1 source read + cargo test/clippy/fmt/doc
|
||||
reviewer: post-session follow-up review (the 2026-09-05 resolution commits)
|
||||
base_commit: 6ad1d84 (review #001 fully resolved)
|
||||
---
|
||||
|
||||
# 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`).
|
||||
|
||||
---
|
||||
|
||||
### 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.
|
||||
|
||||
Defer until the first channels-path consumer exists (per the L1
|
||||
version-skew note in ADR-009); option 1 at that point.
|
||||
|
||||
---
|
||||
|
||||
## 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 | ⬜ open (deferred) |
|
||||
| R5 | `NegotiationSerialize` mislabel on fail-fast | additive variant (with `#[non_exhaustive]` decision) | small | medium (semver) | ⬜ open (deferred) |
|
||||
|
||||
R4/R5 are deferred deliberately: both touch the consumer-facing error
|
||||
surface, both are cheap, and neither is reachable-by-design today.
|
||||
Batch them with the first post-1.0 API decision rather than churning
|
||||
the error enum before a consumer exists.
|
||||
|
||||
## 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`).
|
||||
Reference in New Issue
Block a user