--- id: implementation/fix-hygiene-residue name: "Fix: error-schema declaration + small hygiene residue (review-002 D-3, N-3, N-9)" status: completed depends_on: [] scope: narrow risk: low impact: component level: implementation --- ## Description The small mechanical residue that does not ride either code-fix task: one doc-vs-code drift with a real fix, plus two one-line postures (`docs/reviews/002-protocol-core-implementation-review.md`): 1. **D-3 (minor): `error_schemas` is `vec![]` while the module doc claims the ADR-049 §4 + ADR-016 contract** (`src/params.rs:94-96` vs `:117`). The typed vocabulary works behaviorally (`SocksOpenError::reason` parses `details.reason` off the wire, tested), but a discovery consumer reading `error_schemas` from the registry sees no declared failure contract for `channels/socks5/sub`, and the doc describes a contract the spec does not carry. Fix per the review's lean: **declare the `ErrorDefinition`**, porting alktty's (the first protocol crate's precedent, `/workspace/@alkdev/alktty/src/channels.rs:165-208`: the `channel:open_failed` definition with the `{ reason, message }` schema, pinned by a test asserting the declaration at `:790`). One ADR-cited block; the schema is already written there. Adapt the description text to this crate's establishment story (the establisher's refusal reasons per ADR-049 §3 + the registry's `FORBIDDEN` class). One test mirrors alktty's declaration pin. If the declare-side turns out to hit a substrate constraint (the registry rejects the definition, say), fall back to the honest reword ("empty, since the typed contract is behavioral via `SocksOpenError::reason`; the `ErrorDefinition` pass is crate-family-wide, matching alktunnels, pending") and record why. 2. **N-3 (observation): `DatagramReader::feed`'s let-else at `src/wire.rs:229-231` is structurally unreachable** (0 executions; the block above always sets `current` before the let-else when `buf.len() >= 2`, and the loop broke at `:223` otherwise). Restructure per the review's sketch: match on the already-known-`Some` state, or re-order to drop the else. Zero behavior change; the full wire suite stays green unchanged. 3. **N-9 (observation): `is_mid_frame` is public but consumed nowhere outside the codec's own tests** (`src/wire.rs:253-260`). Keep (a batch-2 front-door bridge driving its own reader loop would use it), and add one doc line naming the anticipated consumer ("the doors") so the next dead-surface sweep does not flag it without this note. ## Acceptance Criteria - [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 - [x] A test pins the declaration (mirroring alktty's), or the reworded doc matches the spec exactly - [x] The `feed` let-else is restructured; zero behavior change (the wire suite green unchanged) - [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 - docs/reviews/002-protocol-core-implementation-review.md §D-3, §N-3, §N-9 - alkcall ADR-049 §4 + ADR-016 (the typed error contract: `/workspace/@alkdev/alkcall/docs/architecture/decisions/`) - alktty precedent: `/workspace/@alkdev/alktty/src/channels.rs:165-208` (the `ErrorDefinition` + schema) and `:790` (the pinning test) - alktunnels sibling: `/workspace/@alkdev/alktunnels/src/params.rs` (the empty-vec posture, the fallback's precedent) ## Notes - **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, because 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`, so 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 {