diff --git a/docs/reviews/002-post-session-review.md b/docs/reviews/002-post-session-review.md index 73f0502..9da97da 100644 --- a/docs/reviews/002-post-session-review.md +++ b/docs/reviews/002-post-session-review.md @@ -1,5 +1,5 @@ --- -status: partially-resolved +status: resolved last_updated: 2026-09-05 reviewed_artifacts: - docs/architecture/decisions/009-channels-open-op-is-the-negotiation.md @@ -221,8 +221,37 @@ assert on `NegotiationSerialize(_)` today. Options: 2. Cheaper: reword the `NegotiationSerialize` doc to note the channels-path fail-fast reuse. No API change, still mislabeled. -Defer until the first channels-path consumer exists (per the L1 -version-skew note in ADR-009); option 1 at that point. +**Original disposition (superseded — see resolution)**: deferred until +the first channels-path consumer exists (per the L1 version-skew note +in ADR-009); option 1 at that point. + +**Rationale for superseding the deferral (2026-09-05)**: the deferral +trigger was circular — "the first channels-path consumer exists" only +fires *after* the change has become expensive (an exhaustive match in +that consumer turns the additive variant into a break), so deferring +converted a zero-risk change into a medium-risk one by waiting. The +ADR-009 citation was also misapplied: the L1 version-skew note governs +*wire* skew (the removed negotiation frame), not API-surface error +enums — ADR-009 doesn't decide this. And the "effectively +unreachable" framing belonged to R4's producer-side arm, not R5's: +the fail-fast path is live today (any `open_via_channels` caller with +bad params gets `NegotiationSerialize` now). The substantive pending +item was the `#[non_exhaustive]` policy decision, which is answerable +on principle without consumers: `TtyError` is already +`#[non_exhaustive]` (backend.rs:44, the two-way-door extension +pattern); alkcall's consumer-facing `AdapterError` is +`#[non_exhaustive]` for exactly this reason; `NegotiationError` and +`RawError` mirror fixed wire semantics and stay exhaustive +(deliberate — in-crate matchers keep exhaustiveness checking). + +**Resolution (2026-09-05)**: option 1 implemented pre-publish, when +adding a variant is additive (the crate is unpublished — there are no +consumers to break): `TtySessionError` is now `#[non_exhaustive]` +(with the policy rationale in its doc), the fail-fast path maps to +the new `InvalidParams(String)` variant, and the two fail-fast tests +assert `InvalidParams(_)`. No exhaustive `match` on `TtySessionError` +exists in-crate or in `tests/` (the tests use `matches!`, unaffected +by `#[non_exhaustive]`). --- @@ -274,13 +303,15 @@ version-skew note in ADR-009); option 1 at that point. | R2 | stale docs from the L1 redesign | align with ADR-009 | trivial | none | ✅ resolved (`37ae07a`) | | R3 | install-time identity snapshot | accepted design (hub-proxy rationale) | none | none | ✅ closed as intended | | R4 | silent death on parse-failure path | error frame or documented asymmetry | small | low | ✅ resolved (option 1) | -| R5 | `NegotiationSerialize` mislabel on fail-fast | additive variant (with `#[non_exhaustive]` decision) | small | medium (semver) | ⬜ open (deferred) | +| R5 | `NegotiationSerialize` mislabel on fail-fast | additive variant (with `#[non_exhaustive]` decision) | small | medium (semver) | ✅ resolved (option 1 + `#[non_exhaustive]`, pre-publish) | -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. +Both R4 and R5 were originally deferred as consumer-surface churn, but +the deferral rationale didn't hold (circular trigger, misapplied +ADR-009 citation — see R5's superseded-disposition note). Both were +resolved pre-publish, while the changes are additive by construction: +R4 via the error frame (no API change), R5 via `InvalidParams` + +`#[non_exhaustive]` on `TtySessionError` (additive while the crate is +unpublished; the enum policy is now written down in its doc). ## Notes diff --git a/src/session.rs b/src/session.rs index ff765a4..ffb812f 100644 --- a/src/session.rs +++ b/src/session.rs @@ -67,7 +67,16 @@ use crate::negotiation::{NegotiateRequest, NegotiationError, NegotiationWriter}; use crate::wire::{Chunk, ChunkReader, ChunkWriter, RawError, STREAM_CTRL_IN, STREAM_CTRL_OUT}; /// Errors from the typed consumer client. +/// +/// `#[non_exhaustive]` so new variants are additive (the same +/// two-way-door pattern as [`crate::backend::TtyError`], and the same +/// justification alkcall gives its consumer-facing `AdapterError`): +/// session drivers accrue failure modes (the channels-path fail-fast +/// variant was added pre-1.0), and an exhaustive match on this enum in +/// a downstream consumer would turn every addition into a breaking +/// change. #[derive(Debug, thiserror::Error)] +#[non_exhaustive] pub enum TtySessionError { /// The underlying transport I/O failed (not a clean EOF). #[error("io: {0}")] @@ -86,6 +95,14 @@ pub enum TtySessionError { /// `channel_id` missing from the response). #[error("channels open: {0}")] ChannelsOpen(String), + /// 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. + /// (R5: the parse previously surfaced as `NegotiationSerialize`, + /// a variant whose name and doc describe serializing the + /// negotiation frame, not parsing open-op params.) + #[error("invalid open params: {0}")] + InvalidParams(String), /// The server sent a negotiation error frame (the first frame on /// the stream is a length-prefixed JSON `{"error":"..."}` rather /// than a raw chunk). @@ -195,20 +212,21 @@ impl TtySession { /// and the producer parses the same value from the open op. /// /// Failures before the channel opens (ACL denial, unknown op, - /// channel cap, invalid params) surface as - /// [`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` + /// 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. pub async fn open_via_channels( client: &ChannelClient, params: serde_json::Value, ) -> Result { let _: NegotiateRequest = serde_json::from_value(params.clone()) - .map_err(TtySessionError::NegotiationSerialize)?; + .map_err(|e| TtySessionError::InvalidParams(e.to_string()))?; let (channel_id, send, recv) = client .open_channel( crate::channels::OP_TTY_OPEN, @@ -1083,7 +1101,7 @@ mod tests { .await .expect("open_via_channels timed out"); assert!( - matches!(result, Err(TtySessionError::NegotiationSerialize(_))), + matches!(result, Err(TtySessionError::InvalidParams(_))), "schema-invalid params fail at the local NegotiateRequest parse (fail-fast, pre-open)" ); } @@ -1106,7 +1124,7 @@ mod tests { .await .expect("open_via_channels timed out"); assert!( - matches!(result, Err(TtySessionError::NegotiationSerialize(_))), + matches!(result, Err(TtySessionError::InvalidParams(_))), "unparseable params must fail before the open op" ); }