diff --git a/docs/architecture/decisions/009-channels-open-op-is-the-negotiation.md b/docs/architecture/decisions/009-channels-open-op-is-the-negotiation.md new file mode 100644 index 0000000..f1b3aab --- /dev/null +++ b/docs/architecture/decisions/009-channels-open-op-is-the-negotiation.md @@ -0,0 +1,171 @@ +# ADR-009: The Channels Open Op's `input` Is the Negotiation + +## Status + +Accepted (2026-09-05). Resolves review #001 L1. Prerequisites: alkcall +0.4.0 (call-time `input_schema` enforcement) and alkcall 0.4.1 +(early-arrival parking for un-adopted channels). + +## Context + +Before this ADR, the channels path carried the negotiation twice. The +consumer opened a channel via the `channels/tty/sub` open op, passing +the `NegotiateRequest` params as the op's `input`; the producer-side +open handler then ignored that `input` (`let _ = input;`) and read a +second, full negotiation frame from the channel's data stream — the +same frame the direct-ALPN path reads. The registry validated `input` +against the op's schema, but the backend selection actually came from +the unvalidated wire frame. Two independent gates (the registry's +schema check on `input` and `drive_session`'s wire-frame parse) could +disagree: a client could pass schema-valid `input` and write a +divergent frame. + +Review #001 (L1) flagged this as a latent inconsistency and asked for +a publisher decision: either drop the `input` parameter or pass it +through as the authoritative negotiation. Investigation of alkcall +found the review's premise wrong in an important way — alkcall (≤0.3.1) +never enforced `input_schema` on any dispatch path at all: the schema +was advertise-only (metadata in discovery). The registry gate this +design leaned on did not exist. + +Two upstream changes landed to make the design sound: + +1. **alkcall 0.4.0** — `OperationSpec.input_schema` is enforced at + call time by all three registry dispatch entry points (`invoke`, + `invoke_streaming`, `invoke_sink`). The schema is compiled once at + registration (fail-closed, the same rule as `publish_schema` / + CF-003). Violations return `INVALID_INPUT` before the handler runs. +2. **alkcall 0.4.1** — the demux parks early-arrival chunks for a + not-yet-adopted channel (bounded FIFO, 64 payloads per channel) and + `adopt_channel` drains them into the new receiver. Before this, the + open-op-response / producer's-first-write race silently dropped the + first chunks of any push-first producer. This bug was found by the + L3 end-to-end test (an immediately-resolving backend's stdout + sentinel + exit chunk raced the consumer's `adopt_channel`; the + session never resolved). + +## Decision + +The open op's registry-validated `input` **is** the negotiation +(design 2 of the L1 decision). The channels path carries **no second +negotiation frame** on the channel's data stream — the stream starts +directly in raw-chunk mode (TTY's 5-byte format, ADR-001/ADR-008). + +### Producer side + +`make_tty_open_handler` parses the open op's `input` into a +`NegotiateRequest` and drives the new `drive_session_pre_negotiated` +(`src/adapter.rs`) — the same three-pump session driver as the direct +path, minus the wire-frame negotiation phase. Validation is factored +into `validate_and_allocate`, shared by both paths: `carriage == +"raw"`, non-empty `cmd`, backend lookup, the ADR-050 ownership check, +and `backend.allocate`. On the channels path the `tty:open` scope gate +is NOT re-checked in the handler — the registry's `AccessControl` +enforced it before the handler ran (the identity the registry checked, +resolved from the connection, is the authoritative one; the +handler-side identity is only the ownership-check subject). + +Post-open failures (unknown backend, `allocate_failed`, ownership +denial) still go to the client as a `0x00`-prefixed negotiation error +frame on the channel stream — the same framing the direct path uses — +so the consumer's ADR-001 §5 disambiguation read applies unchanged. + +A `NegotiateRequest` parse failure of the schema-validated `input` +(the schema is deliberately partial — `carriage`/`backend`/`cmd` +required, `tty`/`cwd`/`env`/backend-params free-form so the opaque +ADR-053 params pass through) is a handler-side failure; the handler +logs and returns without writing an error frame (the channel is torn +down by the wrapper's teardown task). + +### Consumer side + +`TtySession::open_via_channels` (`src/session.rs`) parses `params` as +a `NegotiateRequest` locally first (fail-fast before a channel is +allocated), opens the channel via `ChannelClient::open_channel`, +builds the channel `Connection`, and starts raw-chunk mode directly +(`from_halves_raw` — no negotiation frame is written). The +`0x00`-error-frame peek is retained: the first byte of the producer's +response disambiguates a post-open negotiation error frame (length +prefix starting `0x00`) from a raw chunk (`stream_type` in `{1, 2, 4}`). + +### Scope of the change + +- `tty_open_spec()`'s input schema becomes the partial + `NegotiateRequest` shape (required: `carriage`, `backend`, `cmd`). +- `drive_session_pre_negotiated` is public API (a new producer entry + point alongside `drive_session`); `validate_and_allocate` is + private, shared by both drivers. +- The direct-ALPN path is unchanged: it still reads the wire-frame + negotiation (ADR-001) and enforces the scope gate itself. +- The 5-byte chunk format, the error-frame layout, and the + `NegotiateRequest` JSON shape are unchanged (ADR-001, ADR-006). + This ADR removes a frame from the channels data stream; it does not + change any frame that remains. + +## Consequences + +**Positive:** + +- **One negotiation per session.** The registry's schema check and the + semantic validation run on the same value — the two-gate + disagreement is structurally impossible. +- **Fewer bytes and one less round trip** on the channels path: no + 4-byte length prefix + JSON frame on the data stream, and the + consumer needs no negotiation write before its first read. +- **The consumer's `open_via_channels` fails fast** on malformed + params before a channel is allocated. +- **`open_via_channels` and `from_bidi_stream_via` are covered + end-to-end** against the real producer path (review #001 L3): 5 + session tests + a pre-negotiated adapter test + the shared + `crate::testing` harness (moved out of `channels.rs::tests` so the + session tests share it). + +**Negative:** + +- **A second producer entry point.** `drive_session` (direct) and + `drive_session_pre_negotiated` (channels) must stay in sync on + validation semantics; the shared `validate_and_allocate` keeps the + validated logic in one place, but the scope-gate policy differs by + path (`enforce_scope` flag) and is documented at both call sites. +- **The schema is deliberately partial.** Raw JSON Schema is + permissive on unknown keys, so the opaque backend params pass + through unchecked by the registry; semantic validation stays in + `validate_and_allocate` and the backend's `allocate`. Duplicating + full `NegotiateRequest` schemas in the spec would create a second + definition to drift against `negotiation.rs`. +- **Consumer/producer version skew.** A consumer that writes the old + second negotiation frame to a producer built on this ADR will have + that frame parsed as a raw chunk (stream_type byte = first JSON + byte, garbage length) — the session fails confusingly. Acceptable + pre-1.0 (no released channels-path consumers); the direct path is + unaffected. + +## Door type + +**Two-way for now, one-way once a released consumer exists.** The +channels-path negotiation location (open-op `input` vs. a wire frame) +is reversible pre-1.0 — the `NegotiateRequest` shape, the 5-byte chunk +format, and the error-frame layout (the wire-stable contracts) are +untouched. Once a released consumer depends on "open op, then raw +chunks," removing or re-adding the wire-frame negotiation on this path +becomes a peer-breaking change. The `drive_session_pre_negotiated` +public API is additive; `drive_session` is unchanged. + +## References + +- [ADR-001](001-wire-format-and-two-carriage.md) — the two-carriage + model (JSON negotiation + raw chunks) and the §5 framing + disambiguation this ADR reuses for the post-open error frame +- [ADR-002](002-ttybackend-trait-and-ttyhandle.md) — the backend + selection and allocation this ADR's validation runs +- [ADR-006](006-negotiation-framing-self-contained.md) — the + negotiation frame layout (unchanged; the direct path still uses it) +- [ADR-008](008-channels-pure-channel-multiplexing.md) — the channels + layer strips its 8-byte header transparently; TTY owns its framing + (this ADR decides what TTY's framing on the channels path contains) +- `docs/reviews/001-code-review.md` §"Resolution (2026-09-05, L1 + L3)" +- `src/adapter.rs` — `drive_session`, `drive_session_pre_negotiated`, + `validate_and_allocate` +- `src/channels.rs` — `make_tty_open_handler`, `tty_open_spec` +- `src/session.rs` — `open_via_channels`, `from_halves_raw` +- `src/testing.rs` — the shared producer/consumer channels harness \ No newline at end of file diff --git a/docs/architecture/overview.md b/docs/architecture/overview.md index 47278b1..bb356da 100644 --- a/docs/architecture/overview.md +++ b/docs/architecture/overview.md @@ -285,6 +285,7 @@ PTY allocation code. See [ADR-003](decisions/003-local-backend-placement.md). | Self-contained negotiation framing | [ADR-006](decisions/006-negotiation-framing-self-contained.md) | alktty implements its own length-prefixed framing; format coincides with alkcall's by convention, not by code reuse | | TTY inside channels (reversed) | [ADR-007](decisions/007-tty-inside-channels.md) | Historical: the two-mode TTY design (direct vs inside-channels); reversed by ADR-008/093 | | Channels pure channel multiplexing | [ADR-008](decisions/008-channels-pure-channel-multiplexing.md) | TTY always uses its 5-byte format; the channels layer carries it transparently in the payload (reverses ADR-007) | +| The open op's `input` is the negotiation | [ADR-009](decisions/009-channels-open-op-is-the-negotiation.md) | The channels path carries no second negotiation frame; the open op's registry-validated `input` is the negotiation | ## Open Questions diff --git a/docs/architecture/tty-adapter.md b/docs/architecture/tty-adapter.md index a230ac4..e2fb746 100644 --- a/docs/architecture/tty-adapter.md +++ b/docs/architecture/tty-adapter.md @@ -62,9 +62,14 @@ The `drive_session` function is the per-stream session driver — the counterpart to the POC's `session::drive_session`, generalized from the local PTY backend to the `TtyBackend` trait. It is also the function the channels path reuses — per [ADR-008](decisions/008-channels-pure-channel-multiplexing.md), -TTY always uses its 5-byte format, so the same `drive_session` runs in +TTY always uses its 5-byte format, so the same driver runs in both direct `alk/tty` and channels `alk/channels` modes; only the -`BiStream` source differs. +`BiStream` source differs. The two paths differ in where the +`NegotiateRequest` comes from: the direct path reads the wire-frame +negotiation (ADR-001 §"Negotiation Frame"); the channels path parses +the open op's registry-validated `input` and runs +`drive_session_pre_negotiated` (ADR-009 — the channels path carries no +second negotiation frame on the channel's data stream). ## Why @@ -351,7 +356,8 @@ architectural commitment. | `TtyBackend` trait and `TtyHandle` | [ADR-002](decisions/002-ttybackend-trait-and-ttyhandle.md) | The backend the adapter dispatches to; the handles the adapter pumps | | Exit code on a control chunk | [ADR-004](decisions/004-exit-code-on-control-chunk.md) | The "exit chunk is last" invariant the adapter enforces | | Backend cleanup on session cancel | [ADR-005](decisions/005-backend-cleanup-on-session-cancel.md) | Dropping `exit_code` future kills the session target; the adapter triggers it by dropping the `TtyHandle` on cancel | -| Channels pure channel multiplexing | [ADR-008](decisions/008-channels-pure-channel-multiplexing.md) | `drive_session` runs unchanged in both direct and channels modes; only the `BiStream` source differs | +| Channels pure channel multiplexing | [ADR-008](decisions/008-channels-pure-channel-multiplexing.md) | The same session driver runs in both direct and channels modes; only the `BiStream` source differs | +| Negotiation carried in the open op | [ADR-009](decisions/009-channels-open-op-is-the-negotiation.md) | The channels path carries no second negotiation frame; the open op's registry-validated `input` is the negotiation (`drive_session_pre_negotiated`) | | Dynamic resource ownership | alknet ADR-050 | Terminal sessions as runtime-spawned resources; the adapter's access-control shape | ## Open Questions @@ -370,7 +376,10 @@ architectural commitment. cancel-cleanup contract the adapter triggers by dropping the `TtyHandle` on session cancel - [ADR-008](decisions/008-channels-pure-channel-multiplexing.md) — why - `drive_session` runs unchanged in channels mode + the same session driver runs in channels mode +- [ADR-009](decisions/009-channels-open-op-is-the-negotiation.md) — the + channels path carries no second negotiation frame (the open op's + `input` is the negotiation) - alknet ADR-050 — the ownership model the adapter's access control declares against - alknet ADR-007 — `Connection`, `accept_bi`, the handler-receives- diff --git a/src/channels.rs b/src/channels.rs index d8bb73e..8ec9149 100644 --- a/src/channels.rs +++ b/src/channels.rs @@ -13,10 +13,10 @@ //! respond with `{ channel_id }`. The `TtyOpenHandler` receives the //! channel's `Connection` (data-plane ALPN `alk/tty`), calls //! `accept_bi()` to get the channel's `BiStream`, and runs -//! [`crate::adapter::drive_session`] on it — the same code path the -//! direct-ALPN `TtyAdapter` uses (ADR-093: TTY always uses its 5-byte -//! format; the channels layer strips its 8-byte header and hands TTY -//! the payload transparently). +//! [`crate::adapter::drive_session_pre_negotiated`] on it — the same +//! three-pump session driver as the direct-ALPN path, minus the +//! wire-frame negotiation phase (the open op's `input` is the +//! negotiation, ADR-009). //! //! The access control (scope-gate + ownership) is wired into the //! `OperationSpec`'s `AccessControl` and enforced by the registry's @@ -65,9 +65,10 @@ pub const TTY_ALPN: &str = "alk/tty"; /// this helper. The helper builds the [`OperationSpec`] for /// `channels/tty/sub` with the `channel_open` marker set /// (`ChannelOpenSpec::new("alk/tty")`), an `AccessControl` carrying -/// the `tty:open` scope gate, and a permissive input schema (the -/// `NegotiateRequest` shape — JSON, validated by the -/// `drive_session` negotiation reader). It then wraps the +/// the `tty:open` scope gate, and the partial `NegotiateRequest` +/// input schema (`carriage`/`backend`/`cmd` required — alkcall 0.4 +/// enforces it at dispatch; the full parse and semantic validation +/// runs in the handler). It then wraps the /// `TtyOpenHandler` factory and calls /// [`ChannelCore::register_openable`]. ///