From ff3295422c69dbb8cb7b3ef3bdd99a81252621f3 Mon Sep 17 00:00:00 2001 From: "glm-5.3-flash" Date: Thu, 24 Sep 2026 18:41:42 +0000 Subject: [PATCH] params: the error-schema declaration + the wire hygiene residue (review-002 D-3, N-3, N-9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - D-3: socks5_open_spec declares the channel:open_failed ErrorDefinition (alktty-ported schema, adapted) — reason enum is the full ADR-049 §3 five-code vocabulary matching SocksEstablishmentReason's parse table; description carries this crate's gate classes (FORBIDDEN / INVALID_INPUT / channel:too_many_channels) + the no-phantom-channel property. Pinned by open_spec_declares_the_open_failed_error_definition, which round-trips every enum value through the typed vocabulary — schema and typed view cannot drift. Module doc updated from claim to fact. - N-3: DatagramReader::feed's structurally-unreachable let-else restructured — the header read folds into the let-else's else arm (live-else shape); zero behavior change, wire suite green unchanged. - N-9: is_mid_frame kept; doc names the anticipated batch-2 consumer (the front-door bridges) so dead-surface sweeps don't flag it. Verification: - cargo test: 357 passed, 0 failed (all suites) - cargo clippy --all-targets -- -D warnings: clean - cargo fmt --check: clean - cargo doc --no-deps: clean - cargo check --target wasm32-unknown-unknown: clean - cargo clippy --target wasm32-unknown-unknown -- -D warnings: clean Task: tasks/implementation/fix-hygiene-residue.md (completed) --- src/params.rs | 83 +++++++++++++++++-- src/wire.rs | 10 ++- tasks/implementation/fix-hygiene-residue.md | 90 +++++++++++++++++++-- 3 files changed, 166 insertions(+), 17 deletions(-) diff --git a/src/params.rs b/src/params.rs index ce4c39c..4c3656a 100644 --- a/src/params.rs +++ b/src/params.rs @@ -24,7 +24,7 @@ use alkcall::channels::client::ChannelOpenError; use alkcall::registry::spec::{ - AccessControl, ChannelOpenSpec, OperationSpec, OperationType, Visibility, + AccessControl, ChannelOpenSpec, ErrorDefinition, OperationSpec, OperationType, Visibility, }; use serde::{Deserialize, Serialize}; use serde_json::json; @@ -91,14 +91,36 @@ impl Socks5Params { /// before the open handler (ADR-006; the open gate is the /// boundary). /// -/// `error_schemas` follow alkcall ADR-049 §4 + ADR-016 (the typed -/// `channel:open_failed` contract with the reason vocabulary, plus -/// the registry's own `FORBIDDEN` class). +/// `error_schemas` declare the `channel:open_failed` contract +/// (alkcall ADR-049 §3/§4 + ADR-016): the establisher's refusal +/// reasons (`dial_failed` / `unknown_resource` / `resource_shortage` +/// / `handler_error` / `timeout` — the ADR-049 §3 vocabulary, the +/// same five codes [`crate::error::SocksEstablishmentReason`] parses) +/// plus the registry's own `FORBIDDEN` class (the ACL gate runs +/// before the establisher; ADR-006). The wrapper's wire shape is +/// `details: { reason, message }` (ADR-049 §3). /// /// The typed open-error vocabulary lives in [`crate::error`] /// ([`crate::error::SocksOpenError`] + its /// [`crate::error::SocksOpenError::reason`] typed view). pub fn socks5_open_spec() -> OperationSpec { + let open_failed_schema = json!({ + "type": "object", + "properties": { + "reason": { + "type": "string", + "enum": [ + "dial_failed", + "unknown_resource", + "resource_shortage", + "handler_error", + "timeout" + ] + }, + "message": { "type": "string" } + }, + "required": ["reason", "message"] + }); OperationSpec::new( OP_SOCKS5_OPEN, OperationType::Sub, @@ -114,7 +136,19 @@ pub fn socks5_open_spec() -> OperationSpec { }, "required": ["channel_id"] }), - vec![], + vec![ErrorDefinition { + code: "channel:open_failed".to_string(), + description: "The channel open failed: the registry's ACL gate refused \ + (FORBIDDEN — no socks5:open scope), the params payload did \ + not match the schema (INVALID_INPUT), the per-connection \ + channel cap was hit (channel:too_many_channels), or the \ + establishment phase failed (details.reason ∈ the ADR-049 \ + §3 vocabulary). No channel_id is returned — no phantom \ + channel (ADR-049)." + .to_string(), + schema: open_failed_schema, + http_status: None, + }], AccessControl { required_scopes: vec![SOCKS5_OPEN_SCOPE.to_string()], required_scopes_any: None, @@ -217,4 +251,43 @@ mod tests { let local_only = ChannelOpenError::MissingChannelId; assert_eq!(establishment_reason(&local_only), None); } + + #[test] + fn open_spec_declares_the_open_failed_error_definition() { + let spec = socks5_open_spec(); + let def = spec + .error_schemas + .iter() + .find(|e| e.code == "channel:open_failed") + .expect("channel:open_failed ErrorDefinition declared"); + assert!( + !def.description.is_empty(), + "the failure contract is described" + ); + let props = &def.schema["properties"]; + assert!(props["message"]["type"] == "string"); + assert_eq!(def.schema["required"], json!(["reason", "message"])); + let enum_values = props["reason"]["enum"].as_array().expect("reason enum"); + let reasons: Vec<&str> = enum_values.iter().filter_map(|v| v.as_str()).collect(); + // The declared vocabulary is exactly the typed parse table + // (crate::error::SocksEstablishmentReason) — the schema and + // the typed view cannot drift. + assert_eq!( + reasons, + vec![ + "dial_failed", + "unknown_resource", + "resource_shortage", + "handler_error", + "timeout" + ] + ); + for reason in &reasons { + assert_eq!( + crate::error::SocksEstablishmentReason::parse(reason).map(|r| r.as_str()), + Some(*reason), + "declared reason {reason} is a typed ADR-049 §3 code" + ); + } + } } diff --git a/src/wire.rs b/src/wire.rs index 64bb5ec..580eb62 100644 --- a/src/wire.rs +++ b/src/wire.rs @@ -218,16 +218,14 @@ impl DatagramReader { self.buf.extend_from_slice(chunk); let mut out = Vec::new(); loop { - if self.current.is_none() { + let Some((declared, acc)) = self.current.as_mut() else { if self.buf.len() < FRAME_LEN_FIELD { break; } let declared = u16::from_be_bytes([self.buf[0], self.buf[1]]) as usize; self.buf.advance(FRAME_LEN_FIELD); self.current = Some((declared, BytesMut::new())); - } - let Some((declared, acc)) = self.current.as_mut() else { - break; + continue; }; if *declared == 0 { self.current = None; @@ -255,6 +253,10 @@ impl DatagramReader { /// stream end while this is set is the session's fail-loud /// truncated-datagram condition, never a silent partial /// datagram). + /// + /// Kept public for the anticipated batch-2 consumer — a front-door + /// bridge driving its own reader loop (the doors) — so a later + /// dead-surface sweep does not flag it (review-002 N-9). pub fn is_mid_frame(&self) -> bool { self.current.is_some() } diff --git a/tasks/implementation/fix-hygiene-residue.md b/tasks/implementation/fix-hygiene-residue.md index ea803db..30a2b18 100644 --- a/tasks/implementation/fix-hygiene-residue.md +++ b/tasks/implementation/fix-hygiene-residue.md @@ -1,7 +1,7 @@ --- id: implementation/fix-hygiene-residue name: "Fix — error-schema declaration + small hygiene residue (review-002 D-3, N-3, N-9)" -status: pending +status: completed depends_on: [] scope: narrow risk: low @@ -57,16 +57,16 @@ postures (`docs/reviews/002-protocol-core-implementation-review.md`): ## Acceptance Criteria -- [ ] `socks5_open_spec` declares the `channel:open_failed` +- [x] `socks5_open_spec` declares the `channel:open_failed` `ErrorDefinition` (the alktty-ported schema, adapted) — or the doc is reworded to the honest empty posture with the reason recorded -- [ ] A test pins the declaration (mirroring alktty's) — or the +- [x] A test pins the declaration (mirroring alktty's) — or the reworded doc matches the spec exactly -- [ ] The `feed` let-else is restructured; zero behavior change +- [x] The `feed` let-else is restructured; zero behavior change (the wire suite green unchanged) -- [ ] `is_mid_frame`'s doc names the anticipated consumer -- [ ] `cargo test` green; clippy/fmt clean; both wasm checks clean; +- [x] `is_mid_frame`'s doc names the anticipated consumer +- [x] `cargo test` green; clippy/fmt clean; both wasm checks clean; `cargo doc --no-deps` clean ## References @@ -82,8 +82,82 @@ postures (`docs/reviews/002-protocol-core-implementation-review.md`): ## Notes -> Agent fills this during implementation. +- **D-3 — declared, not reworded.** The review's lean (the declare + side) worked with no substrate constraint: the registry accepted + the definition on the first try. The schema is alktty's + `channel:open_failed` block + (`/workspace/@alkdev/alktty/src/channels.rs:165-208`), adapted: + the `reason` enum carries all five ADR-049 §3 codes + (`dial_failed` / `unknown_resource` / `resource_shortage` / + `handler_error` / `timeout`) — alktty's enum has four because its + establisher has no `resource_shortage` path, while this crate's + declared vocabulary is chosen to match + `SocksEstablishmentReason`'s parse table exactly (the typed view a + consumer branches on), not the current default establisher's + reachable subset (`socks5_session_establisher` is a session gate + returning `Establishment::default()`; a composed/registry + establisher could produce any of the five). The `description` + text is rewritten to this crate's establishment story: the + registry gate classes first (`FORBIDDEN` / `INVALID_INPUT` / + `channel:too_many_channels` — the ACL runs before the + establisher, ADR-006), then the establishment vocabulary, then + the no-phantom-channel property. One judgment call worth noting + for the review round: the declared enum is *wider* than the + default producer's behavioral surface (which produces none of the + five today — the dial is in-stream, post-open). That is the + correct direction to over-declare: `error_schemas` discloses the + op's contract shape (any registration of this op, per ADR-049 + §4), and the enum is the wire's parse table + (`SocksEstablishmentReason::parse`), so a stricter enum would + under-declare for custom-establisher registrations. +- **The pinning test cross-checks schema vs typed vocabulary.** + Beyond mirroring alktty's declaration pin + (`:780-807`), `open_spec_declares_the_open_failed_error_definition` + asserts each enum value round-trips through + `SocksEstablishmentReason::parse`/`as_str` — the declared schema + and the typed vocabulary cannot drift silently (the exact D-3 + drift class, pinned shut). +- **N-3 — restructured to a live-else shape, not the review's + sketch.** Both of the review's alternates were considered: the + `let Some(&mut (declared, ref mut acc))` match has the same + dead-arm problem (it must fire *after* the header read, which is + exactly the unreachable region), and the re-order-to-drop-else + sketch inverts the loop's exit order. The landed shape folds the + header read into the let-else's else arm + (`let Some((declared, acc)) = self.current.as_mut() else { + Agent fills this on completion. \ No newline at end of file +All three residue items closed in one pass (`src/params.rs`, +`src/wire.rs`): + +1. **D-3**: `socks5_open_spec` now declares the + `channel:open_failed` `ErrorDefinition` — alktty-ported schema + (`{ reason (enum of the five ADR-049 §3 codes), message }`, + both required), description adapted to this crate's gate classes + + establishment story. Module doc updated from claim to fact. + Pinned by `open_spec_declares_the_open_failed_error_definition` + (declaration + enum round-trips through + `SocksEstablishmentReason` — schema and typed vocabulary pinned + drift-free). +2. **N-3**: `DatagramReader::feed`'s structurally-unreachable + let-else restructured into a live-else shape (header read in the + else arm); zero behavior change — the wire suite green unchanged. +3. **N-9**: `is_mid_frame` kept; its doc names the anticipated + consumer (the batch-2 front-door bridges) so future dead-surface + sweeps don't flag it. + +Verification: `cargo test` 357 passed / 0 failed (all suites); +`cargo clippy --all-targets -- -D warnings` clean; `cargo fmt +--check` clean; `cargo doc --no-deps` clean; `cargo check +--target wasm32-unknown-unknown` clean; `cargo clippy --target +wasm32-unknown-unknown -- -D warnings` clean. Doc-vs-code drift is +gone either way (review-002 D-3 closed; the review-round gate item +"the `ErrorDefinition` is declared" holds). \ No newline at end of file