params: the error-schema declaration + the wire hygiene residue (review-002 D-3, N-3, N-9)
- 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)
This commit is contained in:
1 parent
68d5be36a0
commit
ff3295422c
3 files changed
+166
-17
No files matched your search
+78
-5
@@ -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"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
+6
-4
@@ -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()
|
||||
}
|
||||
|
||||
@@ -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 {
|
||||
<read header; continue; }`) — every branch is live, the
|
||||
"current set above" invariant is structural (the else arm is the
|
||||
only place `current` is set), and the body is untouched. Zero
|
||||
behavior change: the full wire suite passes unmodified.
|
||||
- **N-9 — kept with the note.** One doc paragraph naming the
|
||||
anticipated batch-2 consumer (a front-door bridge driving its own
|
||||
reader loop — the doors) and citing the review finding.
|
||||
|
||||
## Summary
|
||||
|
||||
> Agent fills this on completion.
|
||||
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).
|
||||
Reference in new issue
Block a user