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 index f1b3aab..76e9ad5 100644 --- a/docs/architecture/decisions/009-channels-open-op-is-the-negotiation.md +++ b/docs/architecture/decisions/009-channels-open-op-is-the-negotiation.md @@ -6,6 +6,11 @@ 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). +Amended 2026-09-05 (review #002 R4): a `NegotiateRequest` parse failure +of the schema-validated `input` is now a client-visible +`malformed_negotiation` error frame, not a silent teardown — see +§"Parse-failure error frame (R4 amendment, 2026-09-05)". + ## Context Before this ADR, the channels path carried the negotiation twice. The @@ -73,9 +78,10 @@ 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). +ADR-053 params pass through) was originally a handler-side failure +logged and returned without an error frame. The R4 amendment below +makes it a client-visible error frame like every other post-open +failure. ### Consumer side @@ -102,6 +108,37 @@ prefix starting `0x00`) from a raw chunk (`stream_type` in `{1, 2, 4}`). This ADR removes a frame from the channels data stream; it does not change any frame that remains. +### Parse-failure error frame (R4 amendment, 2026-09-05) + +All three post-open failure classes on the channels path are now +client-visible through the same `0x00` error-frame peek +(`from_halves_raw`): + +1. **Semantic validation failures** (`carriage != "raw"`, empty `cmd`, + unknown backend, `allocate_failed`, ownership denial) — error + frames from `validate_and_allocate` (unchanged). +2. **`NegotiateRequest` parse failure of schema-valid `input`** — the + open handler accepts the channel's `BiStream` and writes a + `malformed_negotiation` frame + (`{"error":"malformed_negotiation","message":"..."}`) via the shared + `crate::adapter::send_negotiation_error`, then returns. Reachable + despite the registry's schema check because the schema is + deliberately partial: e.g. `cwd` typed as a number passes the schema + (unknown-key/type fields pass through for the opaque ADR-053 + params) but fails the typed parse. +3. **Schema-invalid `input`** — rejected at dispatch by the registry + (alkcall 0.4) before any handler runs: a `CallError` on the open + op, no channel allocated, no error frame (unchanged). + +This replaces the original behavior (log + return, channel teardown, +consumer observes `NoExitChunk` — indistinguishable from a crashed +producer). The error-frame layout is unchanged (ADR-001's wire-stable +contract); no new frame type, no new stream type. Tests: the +`make_tty_open_handler` seam test (hand-built `input` bypassing the +schema, `channels.rs::tests`) and the real-registry end-to-end test +(`testing.rs`); both assert the consumer-side `0x00` peek observes +`malformed_negotiation`. + ## Consequences **Positive:** diff --git a/docs/architecture/tty-adapter.md b/docs/architecture/tty-adapter.md index e2fb746..3876e40 100644 --- a/docs/architecture/tty-adapter.md +++ b/docs/architecture/tty-adapter.md @@ -175,7 +175,7 @@ entering raw mode. The error response shape: | Error | When | Shape | |-------|------|------| | `unknown_backend` | the `backend` string is not in the adapter's backend map | `{"error":"unknown_backend","backend":"..."}` | -| `malformed_negotiation` | the negotiation frame failed to parse as JSON or failed `NegotiateRequest` validation | `{"error":"malformed_negotiation","message":"..."}` | +| `malformed_negotiation` | the negotiation frame failed to parse as JSON or failed `NegotiateRequest` validation — on the direct path the wire frame, on the channels path the open op's `input` (schema-valid values can still fail the typed parse, e.g. `cwd` typed as a number, because the schema is deliberately partial) | `{"error":"malformed_negotiation","message":"..."}` | | `allocate_failed` | `backend.allocate()` returned a `TtyError` | `{"error":"allocate_failed","message":"..."}` | After sending the error response, the adapter closes the write half of diff --git a/docs/reviews/002-post-session-review.md b/docs/reviews/002-post-session-review.md index 08ff7c3..73f0502 100644 --- a/docs/reviews/002-post-session-review.md +++ b/docs/reviews/002-post-session-review.md @@ -175,6 +175,22 @@ 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` @@ -257,13 +273,14 @@ version-skew note in ADR-009); option 1 at that point. | 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) | +| 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) | -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. +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 diff --git a/src/adapter.rs b/src/adapter.rs index a893a66..2c90276 100644 --- a/src/adapter.rs +++ b/src/adapter.rs @@ -168,7 +168,14 @@ fn has_scope(identity: &Option, scope: &str) -> bool { /// Send a negotiation error frame and close the write half. Consumes the /// writer so the underlying transport's shutdown runs after the frame is /// flushed. -async fn send_negotiation_error( +/// +/// Shared by the direct path's failure arms and the channels open +/// handler (`make_tty_open_handler` writes the same `malformed_negotiation` +/// frame on a `NegotiateRequest` parse failure of the open op's `input` — +/// the pre-negotiated driver has no writer to consume, so the handler +/// builds its own over the channel's write half). `pub(crate)` so the +/// channels handler reuses it; not public API. +pub(crate) async fn send_negotiation_error( mut writer: NegotiationWriter, error: &str, fields: &[(&str, &str)], diff --git a/src/channels.rs b/src/channels.rs index 8ec9149..990bbc2 100644 --- a/src/channels.rs +++ b/src/channels.rs @@ -43,7 +43,7 @@ use tracing::debug; use crate::adapter::{drive_session_pre_negotiated, TTY_OPEN_SCOPE}; use crate::backend::TtyBackend; -use crate::negotiation::NegotiateRequest; +use crate::negotiation::{NegotiateRequest, NegotiationWriter}; /// The per-ALPN open operation name (`channels//sub` convention, /// ADR-047). TTY is consumer-opens (the client requests a shell), so @@ -184,12 +184,27 @@ pub fn tty_open_spec() -> OperationSpec { /// three-pump session driver as the direct-ALPN path, minus the /// wire-frame negotiation phase. /// -/// Parse or validation failures (unknown backend, `allocate_failed`, -/// ADR-050 ownership denial) are reported to the client as a -/// negotiation error frame on the channel stream (the same -/// `0x00`-prefixed framing the direct path uses), so a consumer's -/// negotiation-error disambiguation read applies unchanged. The -/// `tty:open` scope gate is enforced by the registry's `AccessControl` +/// All failure classes write a negotiation error frame on the channel +/// stream (the same `0x00`-prefixed framing the direct path uses), so a +/// consumer's negotiation-error disambiguation read applies unchanged: +/// +/// - The registry rejects schema-invalid `input` at dispatch (alkcall +/// 0.4) — the open op fails, no channel is ever allocated, and the +/// failure is a `CallError` on the open op, not a channel-stream +/// error frame. But the schema is deliberately partial (the opaque +/// ADR-053 backend params pass through), so a schema-valid value can +/// still fail the full `NegotiateRequest` parse — e.g. `cwd` typed +/// as a number — and that failure is post-open: the handler writes a +/// `malformed_negotiation` error frame (the same +/// [`crate::adapter::send_negotiation_error`] the other failure +/// classes use) and the consumer's peek surfaces +/// `NegotiationRejected`. +/// - Semantic validation failures (unknown backend, `carriage != "raw"`, +/// empty `cmd`, `allocate_failed`, ADR-050 ownership denial) are +/// post-open error frames written by `validate_and_allocate` inside +/// [`crate::adapter::drive_session_pre_negotiated`]. +/// +/// The `tty:open` scope gate is enforced by the registry's `AccessControl` /// before this handler runs — the handler does not re-check it. /// /// The handler's `JoinHandle` is recorded by the channels wrapper for @@ -214,6 +229,20 @@ fn make_tty_open_handler( Ok(r) => r, Err(e) => { debug!("tty: channels open: invalid negotiate params: {e}"); + let stream = match channel_conn.accept_bi().await { + Ok(s) => s, + Err(e) => { + debug!("tty: channels open: accept_bi failed: {e}"); + return; + } + }; + let (_, mut client_write) = tokio::io::split(stream); + crate::adapter::send_negotiation_error( + NegotiationWriter::new(&mut client_write), + "malformed_negotiation", + &[("message", &e.to_string())], + ) + .await; return; } }; @@ -250,6 +279,91 @@ mod tests { use std::collections::HashMap as StdHashMap; use tokio::io::duplex; + /// The R4 unit-testable seam: feed `make_tty_open_handler` a + /// hand-built `input` the (partial) schema would accept but the full + /// `NegotiateRequest` parse rejects (`cwd` typed as a number). The + /// handler must write a `0x00`-prefixed `malformed_negotiation` error + /// frame on the channel stream — the same failure class as the + /// post-open semantic failures, not a silent teardown. The read side + /// is the consumer's exact post-open sequence (`from_halves_raw`): + /// peek the first byte, see `0x00`, read the length-prefixed error + /// frame. + #[tokio::test] + async fn open_handler_writes_error_frame_on_schema_bypassing_input() { + use alkcall::core::types::{BiStream, BidiStreamSource, StreamError}; + use tokio::io::AsyncReadExt; + + struct YieldOnce { + stream: tokio::sync::Mutex>, + } + #[async_trait::async_trait] + impl BidiStreamSource for YieldOnce { + async fn accept_bi(&self) -> Result { + self.stream + .lock() + .await + .take() + .ok_or(StreamError::ConnectionClosed) + } + async fn open_bi(&self) -> Result { + Err(StreamError::StreamClosed) + } + fn remote_addr(&self) -> Option { + None + } + fn close(&self, _code: u32, _reason: &str) { + // The take-on-drop of the wrapped stream is not needed + // for this test; the connection is never closed. + } + } + + let (client_end, server_end) = duplex(64 * 1024); + let (server_read, server_write) = tokio::io::split(server_end); + let bidi = BiStream::from_joined(server_read, server_write); + let channel_conn = Connection::from_source( + YieldOnce { + stream: tokio::sync::Mutex::new(Some(bidi)), + }, + TTY_ALPN.as_bytes().to_vec(), + ); + + let mut backends: HashMap> = HashMap::new(); + backends.insert("mock".to_string(), Arc::new(MockBackend::with_exit_code(0))); + let handler = make_tty_open_handler(Arc::new(backends), None, None); + + let task = handler( + serde_json::json!({ + "carriage": "raw", + "backend": "mock", + "cmd": ["true"], + "cwd": 42 + }), + channel_conn, + AuthContext::anonymous(b"test"), + ); + task.await.expect("handler task"); + + let mut read = client_end; + let mut first = [0u8; 1]; + tokio::time::timeout( + std::time::Duration::from_secs(5), + read.read_exact(&mut first), + ) + .await + .expect("no byte from handler") + .expect("read first byte"); + assert_eq!(first[0], 0x00, "error frame length prefix starts with 0x00"); + + let mut len_rest = [0u8; 3]; + read.read_exact(&mut len_rest).await.expect("read len rest"); + let len = u32::from_be_bytes([first[0], len_rest[0], len_rest[1], len_rest[2]]) as usize; + let mut body = vec![0u8; len]; + read.read_exact(&mut body).await.expect("read error body"); + let v: serde_json::Value = serde_json::from_slice(&body).expect("parse error frame"); + assert_eq!(v["error"], "malformed_negotiation"); + assert!(v["message"].as_str().is_some_and(|m| !m.is_empty())); + } + #[test] fn op_tty_open_is_channels_tty_sub() { assert_eq!(OP_TTY_OPEN, "channels/tty/sub"); diff --git a/src/session.rs b/src/session.rs index 9400e0b..ff765a4 100644 --- a/src/session.rs +++ b/src/session.rs @@ -196,11 +196,13 @@ impl TtySession { /// /// Failures before the channel opens (ACL denial, unknown op, /// channel cap, invalid params) surface as - /// [`TtySessionError::ChannelsOpen`]. Post-open failures (unknown - /// backend, allocate failure, ownership denial) arrive as a - /// negotiation error frame on the channel stream — the session - /// surfaces those as [`TtySessionError::NegotiationRejected`] via - /// the same `0x00` disambiguation read the direct path uses. + /// [`TtySessionError::ChannelsOpen`]. Post-open failures (a + /// `NegotiateRequest` parse failure of a schema-valid-but-unparseable + /// params value, unknown backend, allocate failure, ownership + /// denial) arrive as a negotiation error frame on the channel + /// stream — the session surfaces those as + /// [`TtySessionError::NegotiationRejected`] via the same `0x00` + /// disambiguation read the direct path uses. pub async fn open_via_channels( client: &ChannelClient, params: serde_json::Value, @@ -292,9 +294,10 @@ impl TtySession { /// Core inner for the channels path (ADR-009): the negotiation /// already happened in the open op — the stream is already in /// raw-chunk mode. The peek still applies: the producer sends a - /// `0x00`-prefixed error frame on post-open failures (unknown - /// backend, allocate failure, ownership denial), and a raw chunk - /// (`stream_type` in `{1, 2, 4}`) on success. + /// `0x00`-prefixed error frame on any post-open failure (a + /// `NegotiateRequest` parse failure of the open op's `input`, + /// unknown backend, allocate failure, ownership denial), and a raw + /// chunk (`stream_type` in `{1, 2, 4}`) on success. async fn from_halves_raw(read: R, write: W) -> Result where R: AsyncRead + Send + Unpin + 'static, diff --git a/src/testing.rs b/src/testing.rs index 18f10a3..e395596 100644 --- a/src/testing.rs +++ b/src/testing.rs @@ -206,4 +206,64 @@ mod tests { assert_eq!(first[0], crate::wire::STREAM_STDOUT); drop(send); } + + /// End-to-end (R4): `input` that passes the registry's (partial) + /// schema but fails the handler's full `NegotiateRequest` parse — + /// `cwd` typed as a number. The open op succeeds (the schema is + /// deliberately partial), the producer-side handler writes a + /// `0x00`-prefixed `malformed_negotiation` error frame on the + /// channel stream, and the consumer-side post-open read sequence + /// (`from_halves_raw`'s peek + error-frame parse) observes it — the + /// same failure class as the post-open semantic failures, not a + /// silent teardown. Uses `ChannelClient::open_channel` directly + /// (the consumer's local fail-fast parse in `open_via_channels` + /// would reject these params before the open op runs — R5's + /// fail-fast path). + #[tokio::test] + async fn schema_valid_but_unparseable_input_gets_malformed_negotiation_frame() { + use tokio::io::AsyncReadExt; + + let mut backends: HashMap> = HashMap::new(); + backends.insert("mock".to_string(), Arc::new(MockBackend::with_exit_code(0))); + let client = + wire_client_and_server(Arc::new(backends), None, Some(tty_identity("alice"))).await; + + let (_channel_id, _send, mut recv) = tokio::time::timeout( + std::time::Duration::from_secs(5), + client.open_channel( + OP_TTY_OPEN, + serde_json::json!({ + "carriage": "raw", + "backend": "mock", + "cmd": ["true"], + "cwd": 42 + }), + crate::channels::TTY_ALPN, + ), + ) + .await + .expect("open_channel timed out") + .expect("open_channel — the partial schema accepts cwd: 42"); + + // The consumer-side post-open sequence (`from_halves_raw`): + // peek the first byte; `0x00` = negotiation error frame. + let mut first = [0u8; 1]; + tokio::time::timeout( + std::time::Duration::from_secs(5), + recv.read_exact(&mut first), + ) + .await + .expect("no response byte from producer") + .expect("read first byte"); + assert_eq!(first[0], 0x00, "error frame length prefix starts with 0x00"); + + let mut len_rest = [0u8; 3]; + recv.read_exact(&mut len_rest).await.expect("read len rest"); + let len = u32::from_be_bytes([first[0], len_rest[0], len_rest[1], len_rest[2]]) as usize; + let mut body = vec![0u8; len]; + recv.read_exact(&mut body).await.expect("read error body"); + let v: serde_json::Value = serde_json::from_slice(&body).expect("parse error frame"); + assert_eq!(v["error"], "malformed_negotiation"); + assert!(v["message"].as_str().is_some_and(|m| !m.is_empty())); + } }