diff --git a/AGENTS.md b/AGENTS.md index eb9496b..b45210f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -194,10 +194,13 @@ implementation agents. for the scope gate and optionally consults `OwnershipProvider` for the resource-ownership check (`provider.owns(id_ref, kind, &id, "tty")` — the 4-arg shape alkcall adopted). The channels path gets - both for free via `ChannelCore::register_openable`, which wires - `AccessControl` into the operation spec — the registry runs the ACL - before the wrapper, so the `OpenHandler` only needs to validate - params and spawn the protocol. Terminal sessions are resources per + the scope gate for free via `ChannelCore::register_openable` + (which wires `AccessControl` into the operation spec — the + registry runs the ACL before the wrapper) and runs the ownership + check in the establisher (ADR-010 — it is part of the + establishment phase since alkcall 0.5.0 ADR-049, so a denial is + `channel:open_failed`, not an in-band frame; the pump handler + only allocates and pumps). Terminal sessions are resources per ADR-050 (ported as alktty ADR-003); `OwnershipStore::record(&self, identity, resource_type, resource_id)` is the 3-arg shape (alkcall dropped the old `action` arg). @@ -311,5 +314,6 @@ wasm-clean" invariant — run it whenever a non-`local` module changes. `StreamError`) come from `alkcall::core`. alkcall is v0.4.x — breaking changes are expected at this major-zero stage; this is the first real consumer, so we find and fix issues upstream rather than - working around them. Pin `alkcall = "0.4.0"` (lockfile resolves - 0.4.1) and bump deliberately. \ No newline at end of file + working around them. Pin `alkcall = "0.5.0"` (the + channel-open establishment phase — ADR-049, review 006 E-01/N-1; + adopted by alktty ADR-010) and bump deliberately. \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index acdfcf7..4d93dbb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,70 @@ All notable changes to this crate are documented here. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this crate adheres to [Semantic Versioning](https://semver.org/). +## [0.2.0] - 2026-09-06 + +The alkcall 0.5.0 adoption (ADR-049 — review 006 E-01/N-1): the +channels-path establishment phase. Breaking on the channels path's +error surface; the wire format, the direct-ALPN path, and the +`TtyBackend` trait are unchanged. + +### Added + +- **Channels establishment phase (ADR-010; alkcall 0.5.0 ADR-049 / + review 006 E-01).** `register_openable` now registers + `channels/tty/sub` with an establisher + (`ChannelCore::register_openable_with_establisher`): the semantic + validation that used to be post-open in-band error frames — the + full `NegotiateRequest` parse of schema-valid `input`, `carriage == + "raw"`, non-empty `cmd`, backend lookup, and the ADR-050 ownership + check — runs before the open reply. Rejections resolve the open op + with `channel:open_failed` carrying `details: { reason, message }` + (`unknown_resource` for an unknown backend, `handler_error` for + malformed-negotiation/ownership-denial, `timeout` on the + establishment deadline) and no channel ever exists opener-side — + the SSH contract. The phantom-channel workaround + (allocate → succeed → in-band-fail) is retired. +- **`channel:open_failed` ErrorDefinition on `tty_open_spec()` + (ADR-016).** The establishment-failure contract (the reachable + `details.reason` enum) is disclosed via `services/schema`; the spec + also gains a `description` (review 006 E-02) disclosed via + `services/list`. +- New tests: the establisher unit gates (parse/carriage/cmd/backend/ + ownership mappings), the end-to-end `channel:open_failed` surfaces + (the review-006 verification gate), and a pinned test guarding the + one failure class that stays in-band (below). + +### Changed + +- **BREAKING — `TtySessionError::ChannelsOpen` carries alkcall's + typed `ChannelOpenError` (`#[from]`) instead of a flattened + `String`** (alkcall 0.5.0 ADR-049 §4 / review 006 N-1, applied at + alktty's layer). Consumers branch on + `ChannelOpenError::establishment_reason()` instead of parsing a + debug string. +- **BREAKING — channels-path semantic failures change shape.** An + unknown backend (and schema-valid-but-unparseable params, + ownership denial) previously surfaced as + `TtySessionError::NegotiationRejected` (an in-band `0x00`-prefixed + error frame on a channel the open op had already reported as + succeeding); they now fail the open op itself as + `TtySessionError::ChannelsOpen(CallFailed{ channel:open_failed })` + (ADR-010). The one failure class that still arrives in-band is + `allocate_failed` (still `NegotiationRejected`): the establisher + cannot carry the allocated `TtyHandle` across to the pump handler + (`Establishment` is payloadless), and re-allocating would violate + ADR-005's kill-on-`Drop` contract — revisit when alkcall gives + `Establishment` a payload. The direct-ALPN path's failure surface + is unchanged (two transports, two contracts). +- `alkcall = "0.5.0"`. + +### Fixed + +- **No more phantom channels on the channels path.** Ledger, policy + count, and manager state balance on every establishment rejection + (the wrapper's teardown runs before the reply), instead of + allocating a channel that immediately EOFs with an error frame. + ## [0.1.0] - 2026-09-05 Initial crates.io release: the `alk/tty` terminal-session protocol — diff --git a/Cargo.lock b/Cargo.lock index 51f2412..ace893b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -27,9 +27,9 @@ dependencies = [ [[package]] name = "alkcall" -version = "0.4.1" +version = "0.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9badefe048a194c93eed09bc5326ebf092fc86817e561d88de2cd12b6302753a" +checksum = "0bbaeb718c370b74f9136d1d59b4608249bd7d1dd2332c0259f98351d709737f" dependencies = [ "async-trait", "bytes", @@ -48,7 +48,7 @@ dependencies = [ [[package]] name = "alktty" -version = "0.1.0" +version = "0.2.0" dependencies = [ "alkcall", "async-trait", diff --git a/Cargo.toml b/Cargo.toml index c8d6a2b..3449df6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "alktty" -version = "0.1.0" +version = "0.2.0" edition = "2021" rust-version = "1.85" license = "MIT OR Apache-2.0" @@ -25,7 +25,7 @@ default = [] local = ["dep:portable-pty", "dep:tokio-util", "tokio/process", "tokio/rt-multi-thread"] [dependencies] -alkcall = "0.4.0" +alkcall = "0.5.0" # Minimal, wasm-clean tokio features. `local` adds `process` + # `rt-multi-thread` (non-wasm). Do NOT use `features = ["full"]` — it # pulls in `signal`/`fs`/`net` which break `wasm32-unknown-unknown`. diff --git a/docs/architecture/README.md b/docs/architecture/README.md index 32acea7..269d255 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -19,7 +19,8 @@ Syntax Tree) document for the wire format, and the ADRs. ## Applicable ADRs Ported from the alknet mono-repo and renumbered into alktty's ADR -range (001..009; ADR-009 is alktty-native). The alknet originals at +range (001..009; ADR-009 and ADR-010 are alktty-native). The alknet +originals at `/workspace/@alkdev/alknet/docs/architecture/decisions/` remain the authoritative source for any ADR not yet ported, and for the alknet ADRs referenced by alknet number in the docs below (which are not @@ -35,7 +36,8 @@ tty-specific and therefore not ported into alktty's ADR range). | [006](decisions/006-negotiation-framing-self-contained.md) | Self-Contained Negotiation Framing (No alkcall-Internal-Wire-Types Dependency) | alknet ADR-057 | Accepted | | [007](decisions/007-tty-inside-channels.md) | TTY Inside Channels — Sub-Streams, Not Wire Format | alknet ADR-077 | Accepted (**reversed by ADR-008** — kept for historical context) | | [008](decisions/008-channels-pure-channel-multiplexing.md) | Channels Pure Channel Multiplexing (8-Byte Header, No `stream_type`) | alknet ADR-093 | Accepted (amends alknet ADR-071/074; reverses ADR-007) | -| [009](decisions/009-channels-open-op-is-the-negotiation.md) | The Channels Open Op's `input` Is the Negotiation | alktty-native | Accepted (resolves review #001 L1; amended by review #002 R4 — parse failure is a client-visible error frame) | +| [009](decisions/009-channels-open-op-is-the-negotiation.md) | The Channels Open Op's `input` Is the Negotiation | alktty-native | Accepted (resolves review #001 L1; amended by review #002 R4 — parse failure is a client-visible error frame; amended by ADR-010 — semantic failures move into the establisher) | +| [010](decisions/010-channels-establisher-migration.md) | Channels-Path Establishment Failures Move into an Establisher | alktty-native | Accepted (adopts alkcall 0.5.0 ADR-049; amends ADR-009's R4 amendment — in-band frames shrink to `allocate_failed`) | ## Key Design Principles 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 76e9ad5..c2c9418 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 @@ -11,6 +11,16 @@ 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)". +Amended 2026-09-06 (ADR-010): with alkcall 0.5.0 (ADR-049 — review 006 +E-01), the semantic-failure classes the R4 amendment reported in-band +(malformed negotiation, unknown backend, ownership denial) move into +the producer's establisher and resolve as `channel:open_failed` call +errors — no phantom channel. The in-band error-frame path on this +channel shrinks to one class: `allocate_failed` (the establisher +cannot carry the allocated handle across — see ADR-010 §2). The R4 +tests that asserted the in-band frames for those classes now assert +the call-error shape. + ## Context Before this ADR, the channels path carried the negotiation twice. The diff --git a/docs/architecture/decisions/010-channels-establisher-migration.md b/docs/architecture/decisions/010-channels-establisher-migration.md new file mode 100644 index 0000000..0fc8721 --- /dev/null +++ b/docs/architecture/decisions/010-channels-establisher-migration.md @@ -0,0 +1,216 @@ +# ADR-010: Channels-Path Establishment Failures Move into an Establisher + +## Status + +Accepted (2026-09-06). Migrates the channels-path semantic-failure +surface per alkcall 0.5.0 / ADR-049 (review 006 E-01 + N-1) — the +sequencing ADR-049 §5 names ("alktty migration — channels-path +semantic failures move into an establisher; the direct-ALPN in-band +error frame is retained — two transports, two contracts"). +Prerequisites: alkcall 0.5.0 (`ChannelCore::register_openable_with_establisher`, +`channel:open_failed`, typed `ChannelOpenError`). + +Amends ADR-009's R4 amendment: the in-band error-frame path on the +channels path shrinks to one failure class (`allocate_failed`). + +## Context + +ADR-009 made the open op's `input` the negotiation and moved semantic +validation into the producer-side handler. Because alkcall 0.4.x's +open wrapper could not fail after allocation (review 006 E-01), the +semantic-failure classes had nowhere to go but the channel stream: +the handler wrote a `0x00`-prefixed negotiation error frame +(`malformed_negotiation`, `unknown_backend`, `allocate_failed`, +ownership-denial `forbidden`) on a channel the open op had already +reported as succeeding. The consumer observed success-then-frame — +a per-crate workaround ADR-049 explicitly retires: + +> alktty's per-crate in-band error vocabulary is retired on the +> channels path; every future ALPN crate gets the establishment reply +> for free. + +alkcall 0.5.0 implements the resolution: the wrapper awaits a bounded +establisher (`OpenEstablisher` — `Fn(Value, AuthContext) -> BoxFuture< +Result>`) between channel +allocation and the open reply; on rejection it tears the channel down +and replies `channel:open_failed` with `details: { reason, message }` +(reason ∈ `dial_failed` / `unknown_resource` / `resource_shortage` / +`handler_error` / `timeout`). The SSH contract holds +consumer-visibly: a failed open never returns a `channel_id`. +`ChannelClient::open_channel` carries the `CallError` verbatim in a +typed `ChannelOpenError` (review 006 N-1), so the reason is +branchable end-to-end. + +## Decision + +### 1. The channels path registers an establisher + +`register_openable` (alktty's helper, signature unchanged) now calls +`ChannelCore::register_openable_with_establisher` with +`make_tty_establisher` — the establishment phase runs the semantic +validation that used to be post-open error frames: + +- Full `NegotiateRequest` parse of the registry-schema-validated + `input` (the schema stays deliberately partial — the opaque ADR-053 + backend params pass through; the establisher is the typed gate). +- `carriage == "raw"`, non-empty `cmd`. +- Backend lookup against the adapter's backend map. +- The ADR-050 ownership check (moved from the pump handler's + `validate_and_allocate`; the scope gate stays the registry's — + pre-allocation, unchanged). + +Rejections map into ADR-049's fixed reason vocabulary (no new codes — +the vocabulary is alkcall's one-way wire surface): + +| TTY failure class | `EstablishmentError` variant | `details.reason` | +|---|---|---| +| `NegotiateRequest` parse failure | `HandlerError` | `handler_error` | +| `carriage != "raw"`, empty `cmd` | `HandlerError` | `handler_error` | +| unknown backend | `UnknownResource` | `unknown_resource` | +| ownership denial | `HandlerError` | `handler_error` | +| establishment deadline | (wrapper) | `timeout` | + +`dial_failed` and `resource_shortage` have no TTY producer path (TTY +dials nothing at open time; `allocate` capacity failures are the +in-band class below) — the spec's `ErrorDefinition` for +`channel:open_failed` declares only the three reachable reasons. +`HandlerError` covers both the malformed-negotiation semantics (the +request shape is wrong) and the ACL-outcome ownership denial (an +authorization failure, not a dial failure). + +### 2. Backend allocation stays in the pump handler — `allocate_failed` stays in-band + +The one failure class that cannot migrate: `backend.allocate`. Two +reasons, both structural: + +1. **`Establishment` is payloadless** ("reserved for a channel plan"). + There is no sound way to hand the allocated `TtyHandle` from the + establisher to the pump handler: the establisher and handler are + separate closures registered once per connection (no per-open key + is shared between them), and an `Arc>>` + stash would need exactly that key to be race-free. +2. **Double-`allocate` is unsound under ADR-005's kill-on-`Drop`** + contract: re-allocating in the handler after an establisher-side + allocate would spawn the session target twice (the first handle's + kill-guard would fire on its drop — a session killed before it + started), and leaking the establisher-side handle would leak the + live target. + +So `backend.allocate` runs where it always did — inside +`drive_session_pre_negotiated`'s `validate_and_allocate` — and +`allocate_failed` remains a post-open `0x00`-prefixed negotiation +error frame, surfaced consumer-side as +[`TtySessionError::NegotiationRejected`] via the same peek the direct +path uses. A pinned test (`allocate_failure_still_arrives_in_band_on_ +channels_path`) guards this boundary. Revisit when alkcall gives +`Establishment` a payload (a channel plan carrying the handle would +let `allocate_failed` join the call-error surface). + +The pump handler's parse arm (`malformed_negotiation` frame) and +`validate_and_allocate`'s other error frames are now defense-in-depth: +the establisher rejects those classes first, so on the registered +path only `allocate_failed` is reachable. The arms stay (a +no-establisher registration still compiles against alkcall's API and +must not silently EOF), and the R4 seam test pins the handler-side +frame. + +### 3. The direct-ALPN path is unchanged + +`TtyAdapter::handle` / `drive_session` keep the wire-frame +negotiation (ADR-001) and the full in-band error-frame vocabulary +(`unknown_backend`, `malformed_negotiation`, `allocate_failed`, +`forbidden`). Two transports, two contracts — the direct path has no +open op to fail, and its `0x00`-peek disambiguation is the ADR-001 +wire-stable contract. The `enforce_scope` split from ADR-009 is +unchanged (`true` direct, `false` channels — the registry's +`AccessControl` is the channels scope gate). + +### 4. Spec enrichment (review 006 E-02 + ADR-016) + +`tty_open_spec()` gains: + +- `description` — disclosed by `services/list` (the open op describes + itself; the produced resource set stays OQ-40's deferred + `channel/resources/subscribe` shape). +- An `ErrorDefinition` for `channel:open_failed` with the + `details.reason` enum — so `services/schema` discloses the + establishment-failure contract per ADR-016 (ADR-049 §3: "ALPN + crates' open-op specs gain matching `ErrorDefinition` entries so + `services/schema` discloses the failure contract"). + +### 5. Consumer surface + +`TtySession::open_via_channels` failures after this ADR: + +| Failure | Where | Consumer-visible as | +|---|---|---| +| schema-invalid params | registry gate | `ChannelsOpen(CallFailed{ INVALID_INPUT })` | +| unparseable params (local fail-fast) | pre-open local parse | `InvalidParams` | +| malformed negotiation (schema-valid) | establisher | `ChannelsOpen(CallFailed{ channel:open_failed, reason: handler_error })` | +| unknown backend | establisher | `ChannelsOpen(CallFailed{ channel:open_failed, reason: unknown_resource })` | +| ownership denial | establisher | `ChannelsOpen(CallFailed{ channel:open_failed, reason: handler_error })` | +| establishment timeout | wrapper bound | `ChannelsOpen(CallFailed{ channel:open_failed, reason: timeout })` | +| ACL denial (scope) | registry gate | `ChannelsOpen(CallFailed{ FORBIDDEN })` | +| channel cap | wrapper `check_open` | `ChannelsOpen(CallFailed{ channel:too_many_channels })` | +| **allocate failure** | pump handler, post-open | **`NegotiationRejected{ "allocate_failed" }`** | + +`TtySessionError::ChannelsOpen` now carries alkcall's typed +`ChannelOpenError` verbatim (`#[from]`) instead of a flattened +`String` — the N-1 fix applied at alktty's layer, so consumers branch +on `establishment_reason()` without unwrapping strings. + +## Consequences + +**Positive:** + +- The phantom-channel workaround is retired: a semantically invalid + open no longer allocates → succeeds → in-band-fails. Ledger, policy + count, and manager state balance on every rejection. +- Retry policy / UX can branch: `unknown_resource` (bad config — don't + retry), `timeout` (maybe retry), `allocate_failed` (capacity — + still distinguishable in-band). +- The consumer's `open_via_channels` failure is a typed call error — + no peeking at the data stream for anything but `allocate_failed`. + +**Negative:** + +- **Breaking (0.2.0):** channels-path semantic failures change shape + (`NegotiationRejected{unknown_backend}` → `ChannelsOpen(CallFailed{ + channel:open_failed})`), and `ChannelsOpen`'s payload changed. The + direct path's failure surface is unchanged. +- The `channel:open_failed` reason codes the establisher emits join + the wire-stable error set (ADR-049's one-way door). +- `allocate_failed` remains a second, in-band failure shape on the + channels path — one residual asymmetry, pinned and documented until + `Establishment` gains a payload. + +## Door type + +**One-way (wire-visible).** The establisher mapping fixes +`details.reason` values for TTY's channels path — consumers branch on +`unknown_resource` / `handler_error` / `timeout`, so re-mapping a +failure class between reasons is a peer-breaking change. The +establisher's internal validation order is a two-way door +(implementation detail behind the fixed surface). ADR-009's +"two-way for now" negotiation-location decision is unaffected — this +ADR moves failure *reporting*, not the negotiation *location*. + +## References + +- alkcall ADR-049 (the establisher, `channel:open_failed`, the typed + client error — the upstream capability this ADR adopts) +- alkcall review 006 (E-01 the establishment gap; N-1 the typed + client error; E-02 the discovery enrichment this ADR's spec + enrichment adopts) +- ADR-009 (the open op's `input` is the negotiation; the R4 + amendment's in-band error frames — shrunk to `allocate_failed` by + this ADR) +- ADR-005 (kill-on-`Drop` — why establisher-side allocation is + unsound) +- ADR-001 (the §5 `0x00` disambiguation — retained for the direct + path and the channels `allocate_failed` frame) +- ADR-002 (backend selection/allocation ownership) +- `src/channels.rs` — `make_tty_establisher`, `make_tty_open_handler`, + `tty_open_spec` +- `src/session.rs` — `TtySessionError::ChannelsOpen`, `open_via_channels` +- `src/testing.rs` — the pinned `allocate_failed`-stays-in-band test \ No newline at end of file diff --git a/docs/architecture/tty-adapter.md b/docs/architecture/tty-adapter.md index 5e177e7..a270b6a 100644 --- a/docs/architecture/tty-adapter.md +++ b/docs/architecture/tty-adapter.md @@ -65,11 +65,13 @@ channels path reuses — per [ADR-008](decisions/008-channels-pure-channel-multi 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. 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). +`NegotiateRequest` comes from and where semantic failures go: the +direct path reads the wire-frame negotiation (ADR-001 §"Negotiation +Frame") and answers failures with in-band error frames; the channels +path parses the open op's registry-validated `input` and runs +`drive_session_pre_negotiated` (ADR-009 — no second negotiation +frame), with the establisher rejecting the semantic-failure classes +as `channel:open_failed` before the handler spawns (ADR-010). ## Why @@ -172,11 +174,20 @@ entering raw mode. The error response shape: { "error": "unknown_backend", "backend": "kubernetes" } ``` +On the **direct-ALPN path** this is the failure surface for every +negotiation/allocation failure class (below). On the **channels path** +the semantic classes (unknown backend, malformed negotiation, +ownership denial) are rejected by the establisher before the open +reply and surface as `channel:open_failed` call errors (alkcall 0.5.0 +ADR-049 / alktty ADR-010) — the in-band frames on that path are +defense-in-depth arms plus the one non-migratable class +(`allocate_failed`, ADR-010 §2). + | 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 — 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":"..."}` | +| `unknown_backend` | the `backend` string is not in the adapter's backend map — direct path, or the channels path's defense-in-depth arm (the establisher rejects it as `channel:open_failed` / `unknown_resource` first) | `{"error":"unknown_backend","backend":"..."}` | +| `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 — the establisher rejects it as `channel:open_failed` / `handler_error` first; the handler-side frame is defense-in-depth, ADR-010 §2) | `{"error":"malformed_negotiation","message":"..."}` | +| `allocate_failed` | `backend.allocate()` returned a `TtyError` — on both paths: the establisher cannot carry the allocated handle across to the pump handler (ADR-010 §2), so allocation failure is the one in-band failure class on the channels path | `{"error":"allocate_failed","message":"..."}` | After sending the error response, the adapter closes the write half of the bidi stream. The client reads the error frame and treats stream close @@ -358,6 +369,7 @@ architectural commitment. | 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) | 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`) | +| Channels establishment failures are call errors | [ADR-010](decisions/010-channels-establisher-migration.md) | The channels path's semantic failures are rejected by the establisher (`channel:open_failed`, alkcall ADR-049); `allocate_failed` stays in-band | | Dynamic resource ownership | alknet ADR-050 | Terminal sessions as runtime-spawned resources; the adapter's access-control shape | ## Open Questions @@ -379,6 +391,11 @@ architectural commitment. 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 + registry-validated `input` is the negotiation) +- [ADR-010](decisions/010-channels-establisher-migration.md) — the + channels path's semantic failures are establisher call errors + (`channel:open_failed`); the in-band frames shrink to + `allocate_failed` `input` is the negotiation) - alknet ADR-050 — the ownership model the adapter's access control declares against diff --git a/src/channels.rs b/src/channels.rs index 990bbc2..744452b 100644 --- a/src/channels.rs +++ b/src/channels.rs @@ -1,5 +1,6 @@ //! Producer half of the channels integration — the `register_openable` -//! helper and the `TtyOpenHandler` factory (ADR-047 §3). +//! helper and the `TtyOpenHandler` factory (ADR-047 §3, amended by +//! alkcall ADR-049 / alktty ADR-010 with an establishment phase). //! //! This is the path where TTY rides inside `alk/channels` rather than //! directly on a `alk/tty` ALPN connection. The assembly layer (an @@ -9,20 +10,27 @@ //! on the per-connection overlay registry. When a consumer (a //! [`crate::session::TtySession`] or any `ChannelClient` caller) //! invokes `channels/tty/sub`, the alkcall channels wrapper does -//! `check_open` → `open_channel` → spawn the `TtyOpenHandler` → -//! 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_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). +//! `check_open` → `open_channel` → **await the establisher bounded** +//! (alkcall ADR-049 §1) → spawn the `TtyOpenHandler` → respond with +//! `{ channel_id }`. The establisher performs the semantic validation +//! (full `NegotiateRequest` parse, `carriage == "raw"`, backend +//! lookup, ADR-050 ownership check) and fails the open op with +//! `channel:open_failed` on rejection — no phantom channel. 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_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). Backend allocation stays in the pump +//! handler (`allocate_failed` still arrives in-band as a negotiation +//! error frame — the establisher cannot carry the allocated handle +//! across to the handler; ADR-010). //! //! The access control (scope-gate + ownership) is wired into the //! `OperationSpec`'s `AccessControl` and enforced by the registry's -//! `invoke`/`invoke_streaming` before the `TtyOpenHandler` runs — the -//! handler itself only validates that the negotiated backend exists -//! and spawns the session driver. This is the difference from the +//! `invoke`/`invoke_streaming` before the establisher runs — the +//! establisher itself only runs the ADR-050 ownership check (the +//! scope gate is the registry's); this is the difference from the //! direct-ALPN `TtyAdapter`, which does its own ad-hoc scope check; //! the channels path gets ACL for free from alkcall's registry. //! @@ -31,12 +39,14 @@ use std::collections::HashMap; use std::sync::Arc; -use alkcall::channels::operations::{ChannelCore, OpenHandler}; +use alkcall::channels::operations::{ + ChannelCore, Establishment, EstablishmentError, OpenEstablisher, OpenHandler, +}; use alkcall::core::auth::AuthContext; use alkcall::core::ownership::OwnershipProvider; use alkcall::core::Connection; use alkcall::registry::spec::{ - AccessControl, ChannelOpenSpec, OperationSpec, OperationType, Visibility, + AccessControl, ChannelOpenSpec, ErrorDefinition, OperationSpec, OperationType, Visibility, }; use serde_json::{json, Value}; use tracing::debug; @@ -107,8 +117,16 @@ pub fn register_openable( auth: AuthContext, ) -> Result<(), String> { let spec = tty_open_spec(); - let open_handler = make_tty_open_handler(backends, ownership, auth.identity.clone()); - core.register_openable(spec, open_handler, registry, auth) + let establisher = make_tty_establisher(Arc::clone(&backends), ownership, auth.identity.clone()); + let open_handler = make_tty_open_handler(backends, auth.identity.clone()); + core.register_openable_with_establisher( + spec, + Some(establisher), + open_handler, + registry, + auth, + None, + ) } /// Build the [`OperationSpec`] for `channels/tty/sub`. @@ -130,9 +148,28 @@ pub fn register_openable( /// here would create a second definition to drift. The registry check /// catches structurally-broken opens (missing command, wrong types); /// the full parse and semantic validation (`carriage == "raw"`, -/// backend lookup) runs in the handler via -/// [`crate::adapter::drive_session_pre_negotiated`]. +/// backend lookup, ownership) runs in the establisher (ADR-010 — +/// failures are `channel:open_failed`); `backend.allocate` runs in the +/// pump handler. +/// +/// The op declares an [`ErrorDefinition`] for `channel:open_failed` +/// (ADR-016 — establishment failures are disclosed so `services/schema` +/// shows the failure contract; reason ∈ `unknown_resource` / +/// `handler_error` / `timeout`, per the ADR-049 vocabulary — see +/// `make_tty_establisher` for the mapping) and carries a +/// `description` (review 006 E-02 — discovery enrichment). pub fn tty_open_spec() -> OperationSpec { + let open_failed_schema = json!({ + "type": "object", + "properties": { + "reason": { + "type": "string", + "enum": ["unknown_resource", "handler_error", "timeout"] + }, + "message": { "type": "string" } + }, + "required": ["reason", "message"] + }); OperationSpec::new( OP_TTY_OPEN, OperationType::Sub, @@ -155,7 +192,16 @@ pub fn tty_open_spec() -> OperationSpec { "channel_id": { "type": "integer", "minimum": 0 } } }), - vec![], + vec![ErrorDefinition { + code: "channel:open_failed".to_string(), + description: "Establishment failed after channel allocation: the establisher \ + rejected the negotiation (malformed negotiation, unknown backend, \ + ownership denial) or the establishment deadline expired. Details: \ + { reason, message }. No channel_id is returned." + .to_string(), + schema: open_failed_schema, + http_status: None, + }], AccessControl { required_scopes: vec![TTY_OPEN_SCOPE.to_string()], required_scopes_any: None, @@ -164,45 +210,129 @@ pub fn tty_open_spec() -> OperationSpec { }, None, ) + .with_description( + "Open a terminal session (alk/tty). The open op's input is the \ + NegotiateRequest (ADR-009); the data plane is TTY's 5-byte chunk \ + format on the allocated channel.", + ) .with_channel_open(ChannelOpenSpec::new(TTY_ALPN)) } +/// Build the [`OpenEstablisher`] for `channels/tty/sub` — the awaited +/// establishment phase (alkcall ADR-049 §1, alktty ADR-010). +/// +/// The establisher runs the semantic validation that used to be +/// post-open error frames inside the pump handler (ADR-009 R4): the +/// full `NegotiateRequest` parse of the registry-schema-validated +/// `input` (the schema is deliberately partial so the opaque ADR-053 +/// backend params pass through), `carriage == "raw"`, non-empty +/// `cmd`, the backend lookup, and the ADR-050 ownership check. Any +/// rejection resolves `Err(EstablishmentError)` and the wrapper +/// replies `channel:open_failed` with `details.reason` — the SSH +/// contract: the channel never exists opener-side, no `channel_id` is +/// ever returned. +/// +/// The `tty:open` scope gate is NOT checked here — the registry's +/// `AccessControl` enforced it before the establisher ran (the +/// identity view the registry checked, resolved from the connection, +/// is the authoritative one; the establisher-side identity is only +/// the ownership-check subject — the same split as the old +/// handler-side check). +/// +/// Mapping into the ADR-049 reason vocabulary (no new codes — the +/// vocabulary is alkcall's one-way wire surface): +/// +/// - `NegotiateRequest` parse failure, `carriage != "raw"`, empty +/// `cmd` → [`EstablishmentError::HandlerError`] (`malformed_ +/// negotiation` semantics — the request shape is wrong). +/// - Unknown backend → [`EstablishmentError::UnknownResource`]. +/// - Ownership denial → [`EstablishmentError::HandlerError`] (an ACL +/// outcome, not a dial outcome; the registry's scope gate stays the +/// pre-allocation `FORBIDDEN` path). +/// +/// Backend allocation deliberately does NOT happen here: the +/// establisher has no way to hand the allocated `TtyHandle` to the +/// pump handler (`Establishment` is payloadless — reserved for a +/// channel plan), and a second `allocate` for the same open would +/// violate the kill-on-`Drop` contract (ADR-005). `allocate_failed` +/// therefore stays a post-open in-band negotiation error frame +/// (ADR-010). +fn make_tty_establisher( + backends: Arc>>, + ownership: Option>, + identity: Option, +) -> OpenEstablisher { + Arc::new(move |input: Value, auth: AuthContext| { + let backends = Arc::clone(&backends); + let ownership = ownership.clone(); + let identity = identity.clone().or_else(|| auth.identity.clone()); + Box::pin(async move { + let req: NegotiateRequest = match serde_json::from_value(input) { + Ok(r) => r, + Err(e) => { + debug!("tty: channels establisher: invalid negotiate params: {e}"); + return Err(EstablishmentError::HandlerError { + message: format!("malformed negotiation: {e}"), + }); + } + }; + if req.carriage != "raw" { + return Err(EstablishmentError::HandlerError { + message: "malformed negotiation: carriage must be 'raw'".to_string(), + }); + } + if req.cmd.is_empty() { + return Err(EstablishmentError::HandlerError { + message: "malformed negotiation: cmd must be non-empty".to_string(), + }); + } + let backend = match backends.get(&req.backend) { + Some(b) => Arc::clone(b), + None => { + return Err(EstablishmentError::UnknownResource { + message: format!("unknown backend: {}", req.backend), + }) + } + }; + let params = crate::backend::TtyParams::from(req); + if let Some(provider) = ownership { + if let Some((kind, id)) = backend.resource_id(¶ms) { + let owns = identity + .as_ref() + .map(|id_ref| provider.owns(id_ref, kind, &id, "tty")) + .unwrap_or(false); + if !owns { + debug!("tty: channels establisher: ownership denied"); + return Err(EstablishmentError::HandlerError { + message: "forbidden: caller does not own the requested tty \ + resource" + .to_string(), + }); + } + } + } + Ok(Establishment {}) + }) + }) +} + /// Build the [`OpenHandler`] for `channels/tty/sub`. /// /// The handler receives the open op's `input` — the `NegotiateRequest` -/// params, validated against the spec's input schema by the registry -/// (alkcall 0.4: `input_schema` is enforced at dispatch) and passed -/// through as the authoritative negotiation (ADR-009: the channels -/// path does not carry a second negotiation frame on the channel; the -/// open op's `input` *is* the negotiation). The channel's -/// [`Connection`] (data-plane ALPN `alk/tty`) and the peer's -/// [`AuthContext`] come with it. The handler parses `input`, calls -/// `accept_bi()` to get the channel's `BiStream`, splits it into +/// params, validated by the establisher before the handler was spawned +/// (alkcall ADR-049 §1: the establisher is awaited bounded, before the +/// reply and before the pump handler) — re-parses it, calls +/// `accept_bi()` to get the channel's [`BiStream`], splits it into /// read/write halves (the stdlib `tokio::io::split` idiom — the same /// split the direct-ALPN `TtyAdapter::handle` does), and runs /// [`crate::adapter::drive_session_pre_negotiated`] on them — the same -/// three-pump session driver as the direct-ALPN path, minus the -/// wire-frame negotiation phase. -/// -/// 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`]. +/// three-pump session driver as the direct-ALPN path. The establisher +/// already rejected the semantic-failure classes (malformed params, +/// `carriage != "raw"`, unknown backend, ownership denial) as +/// `channel:open_failed`, so the driver's `validate_and_allocate` pass +/// on this path only re-runs the allocation — its negotiation error +/// frames remain reachable only for `allocate_failed` (the establisher +/// cannot carry the handle across — ADR-010). /// /// The `tty:open` scope gate is enforced by the registry's `AccessControl` /// before this handler runs — the handler does not re-check it. @@ -215,13 +345,11 @@ pub fn tty_open_spec() -> OperationSpec { /// `manager.teardown_channel` and `policy.on_close` (ADR-047 §7). fn make_tty_open_handler( backends: Arc>>, - ownership: Option>, identity: Option, ) -> OpenHandler { Arc::new( move |input: Value, channel_conn: Connection, auth: AuthContext| { let backends = Arc::clone(&backends); - let ownership = ownership.clone(); let identity = identity.clone().or_else(|| auth.identity.clone()); tokio::spawn(async move { @@ -259,7 +387,7 @@ fn make_tty_open_handler( client_read, req, backends, - ownership, + None, identity, ) .await; @@ -279,15 +407,17 @@ 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. + /// The R4 unit-testable seam, retained as a defense-in-depth gate: + /// 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). Post-ADR-010 the establisher rejects + /// this class before the handler ever spawns — this test pins the + /// handler's own parse arm, which must still write a + /// `0x00`-prefixed `malformed_negotiation` error frame rather than + /// silently EOFing if that input ever reaches it (e.g. a + /// no-establisher registration). 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}; @@ -329,7 +459,7 @@ mod tests { 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 handler = make_tty_open_handler(Arc::new(backends), None); let task = handler( serde_json::json!({ @@ -369,6 +499,187 @@ mod tests { assert_eq!(OP_TTY_OPEN, "channels/tty/sub"); } + // --- ADR-010: the establisher (semantic validation → channel:open_failed) + + fn mock_backends_one() -> Arc>> { + let mut backends: HashMap> = HashMap::new(); + backends.insert("mock".to_string(), Arc::new(MockBackend::with_exit_code(0))); + Arc::new(backends) + } + + async fn establisher_result( + backends: Arc>>, + ownership: Option>, + input: Value, + ) -> Result { + let establisher = make_tty_establisher(backends, ownership, None); + establisher(input, AuthContext::anonymous(b"test")).await + } + + #[tokio::test] + async fn establisher_ok_on_valid_params() { + let result = establisher_result( + mock_backends_one(), + None, + json!({ "carriage": "raw", "backend": "mock", "cmd": ["true"] }), + ) + .await; + assert!(result.is_ok(), "valid params establish, got {result:?}"); + } + + #[tokio::test] + async fn establisher_rejects_schema_bypassing_parse() { + let result = establisher_result( + mock_backends_one(), + None, + json!({ "carriage": "raw", "backend": "mock", "cmd": ["true"], "cwd": 42 }), + ) + .await; + match result { + Err(e) => { + assert_eq!(e.reason(), "handler_error"); + assert!(e.message().contains("malformed negotiation")); + } + Ok(_) => panic!("schema-bypassing input must be rejected by the establisher"), + } + } + + #[tokio::test] + async fn establisher_rejects_non_raw_carriage() { + let result = establisher_result( + mock_backends_one(), + None, + json!({ "carriage": "line", "backend": "mock", "cmd": ["true"] }), + ) + .await; + match result { + Err(e) => { + assert_eq!(e.reason(), "handler_error"); + assert!(e.message().contains("carriage must be 'raw'")); + } + Ok(_) => panic!("non-raw carriage must be rejected"), + } + } + + #[tokio::test] + async fn establisher_rejects_empty_cmd() { + let result = establisher_result( + mock_backends_one(), + None, + json!({ "carriage": "raw", "backend": "mock", "cmd": [] }), + ) + .await; + match result { + Err(e) => { + assert_eq!(e.reason(), "handler_error"); + assert!(e.message().contains("cmd must be non-empty")); + } + Ok(_) => panic!("empty cmd must be rejected"), + } + } + + #[tokio::test] + async fn establisher_rejects_unknown_backend_as_unknown_resource() { + let result = establisher_result( + mock_backends_one(), + None, + json!({ "carriage": "raw", "backend": "nope", "cmd": ["true"] }), + ) + .await; + match result { + Err(e) => { + assert_eq!(e.reason(), "unknown_resource"); + assert_eq!(e.message(), "unknown backend: nope"); + } + Ok(_) => panic!("unknown backend must be rejected"), + } + } + + /// The ADR-050 ownership check runs in the establisher (moved from + /// the pump handler's `validate_and_allocate`); a denial is + /// `handler_error` (an ACL outcome, not a dial outcome). + #[tokio::test] + async fn establisher_rejects_ownership_denial() { + use alkcall::core::OwnershipStore; + + let store = alkcall::core::ownership::InMemoryOwnershipStore::new(); + // The requested resource is owned by someone else (or nobody). + store + .record( + &Identity { + id: "mallory".to_string(), + scopes: vec![], + resources: HashMap::new(), + }, + "session", + "s-42", + ) + .await + .expect("record"); + let provider: Arc = Arc::new(store); + + struct OwnedBackend; + #[async_trait::async_trait] + impl TtyBackend for OwnedBackend { + async fn allocate( + &self, + _params: &crate::backend::TtyParams, + ) -> Result { + panic!("allocate must not run during establishment"); + } + fn resource_id( + &self, + _params: &crate::backend::TtyParams, + ) -> Option<(&'static str, String)> { + Some(("session", "s-42".to_string())) + } + } + let mut backends: HashMap> = HashMap::new(); + backends.insert("mock".to_string(), Arc::new(OwnedBackend)); + + let result = establisher_result( + Arc::new(backends), + Some(provider), + json!({ "carriage": "raw", "backend": "mock", "cmd": ["true"] }), + ) + .await; + match result { + Err(e) => { + assert_eq!(e.reason(), "handler_error"); + assert!(e.message().contains("forbidden")); + } + Ok(_) => panic!("ownership denial must be rejected by the establisher"), + } + } + + /// E-02: the spec carries a description (disclosed by + /// `services/list`) and declares the `channel:open_failed` error + /// schema (ADR-016 — the establishment-failure contract is + /// discoverable via `services/schema`). + #[test] + fn tty_open_spec_declares_description_and_open_failed_error() { + let spec = tty_open_spec(); + assert!( + spec.description.is_some(), + "description set for services/list disclosure (review 006 E-02)" + ); + let def = spec + .error_schemas + .iter() + .find(|e| e.code == "channel:open_failed") + .expect("channel:open_failed ErrorDefinition declared"); + let enum_values = def.schema["properties"]["reason"]["enum"] + .as_array() + .expect("reason enum"); + let reasons: Vec<&str> = enum_values.iter().filter_map(|v| v.as_str()).collect(); + // The establisher produces only these three (dial_failed and + // resource_shortage have no TTY producer path — ADR-010). + assert_eq!( + reasons, + vec!["unknown_resource", "handler_error", "timeout"] + ); + } + #[test] fn tty_alpn_is_alk_tty() { assert_eq!(TTY_ALPN, "alk/tty"); diff --git a/src/session.rs b/src/session.rs index 3d57c1b..de581ad 100644 --- a/src/session.rs +++ b/src/session.rs @@ -20,7 +20,11 @@ //! half, and runs the typed-methods flow directly in raw chunk mode //! (ADR-009: the open op's `params` — validated by the registry's //! input schema — *are* the negotiation; no second negotiation -//! frame is written on the channel's data stream). +//! frame is written on the channel's data stream). Semantic +//! establishment failures are rejected by the producer's establisher +//! and surface as `channel:open_failed` call errors (alkcall 0.5.0 +//! ADR-049 / alktty ADR-010); allocation failure stays in-band +//! (see `open_via_channels`' docs). //! //! The session handle exposes: //! - [`TtySession::send_stdin`] / [`TtySession::close_stdin`] — write @@ -91,10 +95,19 @@ pub enum TtySessionError { /// The negotiation frame failed to write (framing I/O error). #[error("negotiation write: {0}")] NegotiationWrite(#[from] NegotiationError), - /// The channels open op failed (returned a `CallError` or - /// `channel_id` missing from the response). + /// The channels open op failed. Carries alkcall's typed + /// `ChannelOpenError` verbatim (alkcall 0.5.0, ADR-049 §4): a + /// `CallFailed` variant wraps the wire `CallError`, so a + /// semantic-establishment failure (unknown backend, malformed + /// negotiation, ownership denial — rejected by the producer's + /// establisher) surfaces as `channel:open_failed` with + /// `details.reason` branchable via + /// `ChannelOpenError::establishment_reason()`; ACL denial and + /// unknown-op failures carry their own codes (`FORBIDDEN`, + /// `UNKNOWN_OPERATION`). `MissingChannelId` / `AdoptFailed` are + /// the local-only failure shapes. #[error("channels open: {0}")] - ChannelsOpen(String), + ChannelsOpen(#[from] alkcall::channels::client::ChannelOpenError), /// The channels open op's `params` failed the consumer's local /// `NegotiateRequest` parse — the fail-fast check in /// [`TtySession::open_via_channels`] before a channel is allocated. @@ -218,13 +231,20 @@ impl TtySession { /// Failures before the channel opens (ACL denial, unknown op, /// channel cap) surface as [`TtySessionError::ChannelsOpen`]; a /// params value that fails the local `NegotiateRequest` parse - /// surfaces as [`TtySessionError::InvalidParams`]. 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. + /// surfaces as [`TtySessionError::InvalidParams`]. Semantic + /// establishment failures (a `NegotiateRequest` parse failure of a + /// schema-valid-but-unparseable params value, `carriage != "raw"`, + /// empty `cmd`, unknown backend, ownership denial) are rejected by + /// the producer's establisher before the reply — they surface as + /// [`TtySessionError::ChannelsOpen`] wrapping a `CallFailed` whose + /// `CallError` is `channel:open_failed` with + /// `details.reason` (alkcall 0.5.0 ADR-049; the SSH contract: no + /// `channel_id` is ever returned). The one post-open failure class + /// that still arrives in-band is allocation failure + /// (`allocate_failed`): it surfaces as + /// [`TtySessionError::NegotiationRejected`] via the `0x00` + /// disambiguation read (the establisher cannot carry the allocated + /// backend handle across to the pump handler — ADR-010). pub async fn open_via_channels( client: &ChannelClient, params: serde_json::Value, @@ -243,7 +263,7 @@ impl TtySession { crate::channels::TTY_ALPN, ) .await - .map_err(TtySessionError::ChannelsOpen)?; + .map_err(TtySessionError::from)?; debug!("tty: opened channel {channel_id} via channels"); let remote_addr = client.manager().remote_addr(); @@ -1203,10 +1223,15 @@ mod tests { assert_eq!(code, 0); } - /// The same end-to-end path with an unknown backend: the producer - /// sends a `0x00`-prefixed error frame on the channel stream, the - /// consumer's post-open peek disambiguates it, and the session - /// surfaces `NegotiationRejected` (M1 on the channels path). + /// The ADR-049 §5 migration gate: an unknown backend is rejected by + /// the producer's establisher before the reply — the open op fails + /// with `channel:open_failed` + `details.reason == "unknown_resource"` + /// (the SSH "channel never exists opener-side" contract: no + /// `channel_id`, no session), instead of the pre-0.2.0 post-open + /// in-band `unknown_backend` negotiation error frame + /// (`NegotiationRejected`). The direct-ALPN path keeps the in-band + /// frame (two transports, two contracts — see + /// `connect_direct_returns_negotiation_rejected`). #[tokio::test] async fn open_via_channels_surfaces_negotiation_rejected() { let client = @@ -1219,13 +1244,23 @@ mod tests { .await .expect("open_via_channels timed out"); match result { - Err(TtySessionError::NegotiationRejected { error, fields }) => { - assert_eq!(error, "unknown_backend"); - assert_eq!(fields.get("backend").map(String::as_str), Some("nope")); + Err(TtySessionError::ChannelsOpen( + alkcall::channels::client::ChannelOpenError::CallFailed { error }, + )) => { + assert_eq!(error.code, "channel:open_failed"); + let details = error.details.expect("details carry the reason"); + assert_eq!(details["reason"], "unknown_resource"); + assert_eq!(details["message"], "unknown backend: nope"); } - Ok(_) => panic!("expected NegotiationRejected, got Ok(session)"), - Err(other) => panic!("expected NegotiationRejected, got {other:?}"), + Ok(_) => panic!("expected channel:open_failed, got Ok(session)"), + Err(other) => panic!("expected ChannelsOpen(channel:open_failed), got {other:?}"), } + // The SSH contract, consumer-visible: no channel was adopted + // locally (a failed open never returns a channel_id). + assert!( + client.manager().channel_ids().iter().all(|&id| id == 0), + "no data channel survives a failed establishment (channel 0 is the call channel)" + ); } /// The open op's `input` is schema-validated by the registry diff --git a/src/testing.rs b/src/testing.rs index e395596..abc933e 100644 --- a/src/testing.rs +++ b/src/testing.rs @@ -207,28 +207,26 @@ mod tests { 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). + /// End-to-end (R4, post-ADR-010): `input` that passes the registry's + /// (partial) schema but fails the establisher's full + /// `NegotiateRequest` parse — `cwd` typed as a number. The + /// establisher rejects it before the reply: the open op fails with + /// `channel:open_failed` + `details.reason == "handler_error"` (the + /// malformed-negotiation mapping), the channel is torn down, and no + /// `channel_id` is ever returned. 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; + async fn schema_valid_but_unparseable_input_fails_open_with_open_failed() { + use alkcall::channels::client::ChannelOpenError; 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( + let result = tokio::time::timeout( std::time::Duration::from_secs(5), client.open_channel( OP_TTY_OPEN, @@ -242,8 +240,72 @@ mod tests { ), ) .await + .expect("open_channel timed out"); + match result { + Err(ChannelOpenError::CallFailed { error }) => { + assert_eq!(error.code, "channel:open_failed"); + let details = error.details.expect("details carry the reason"); + assert_eq!(details["reason"], "handler_error"); + assert!( + details["message"] + .as_str() + .is_some_and(|m| m.contains("malformed negotiation")), + "message carries the malformed-negotiation detail, got {details}" + ); + } + Ok(_) => panic!("expected channel:open_failed, got Ok(channel)"), + Err(other) => panic!("expected CallFailed(channel:open_failed), got {other:?}"), + } + // The SSH contract: no data channel survives a failed + // establishment (channel 0 is the call channel). + assert!( + client.manager().channel_ids().iter().all(|&id| id == 0), + "no channel survives a failed establishment" + ); + } + + /// Post-ADR-010, the one failure class that still arrives in-band on + /// the channels path is allocation failure (`allocate_failed`): the + /// establisher cannot carry the allocated `TtyHandle` across to the + /// pump handler (ADR-010), so `backend.allocate` runs in the handler + /// and its failure surfaces as a `0x00`-prefixed + /// `allocate_failed` negotiation error frame via the consumer's + /// `from_halves_raw` peek. Pins the boundary of the establisher + /// migration. + #[tokio::test] + async fn allocate_failure_still_arrives_in_band_on_channels_path() { + use tokio::io::AsyncReadExt; + + struct AllocFailBackend; + + #[async_trait::async_trait] + impl TtyBackend for AllocFailBackend { + async fn allocate( + &self, + _params: &crate::backend::TtyParams, + ) -> Result { + Err(crate::backend::TtyError::AllocFailed { + message: "out of ptys".to_string(), + }) + } + } + + let mut backends: HashMap> = HashMap::new(); + backends.insert("mock".to_string(), Arc::new(AllocFailBackend)); + 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"] }), + crate::channels::TTY_ALPN, + ), + ) + .await .expect("open_channel timed out") - .expect("open_channel — the partial schema accepts cwd: 42"); + .expect("open_channel — establishment succeeds, allocation is post-open"); // The consumer-side post-open sequence (`from_halves_raw`): // peek the first byte; `0x00` = negotiation error frame. @@ -263,7 +325,7 @@ mod tests { 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_eq!(v["error"], "allocate_failed"); assert!(v["message"].as_str().is_some_and(|m| !m.is_empty())); } }