glm-5.3-flash
8ee9216a07
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.
305 lines
14 KiB
Markdown
305 lines
14 KiB
Markdown
---
|
|
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`).
|
|
|
|
**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.
|
|
|
|
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 | ✅ resolved (option 1) |
|
|
| R5 | `NegotiationSerialize` mislabel on fail-fast | additive variant (with `#[non_exhaustive]` decision) | small | medium (semver) | ⬜ open (deferred) |
|
|
|
|
R5 is deferred deliberately: it touches the consumer-facing error
|
|
surface, it is cheap, and the producer-side parse-failure arm (R4's
|
|
concern) is now client-visible regardless. Batch it 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`). |