review 003: the protocol-core implementation reviewed — correctness/security/smells/coverage
General review of the landed batch-1 protocol core (docs/reviews/ 002-protocol-core-implementation-review.md), complementing the narrow fidelity review: correctness hazards, security posture, dead surface, and cargo-llvm-cov attribution, cross-checked against alkcall 0.8.0 and the sibling crates. Two majors: - C-1: BindSession::await_inbound (and BindReply1::await_inbound) is re-entrant with no spent-marker — a second call re-reads the live data plane as reply#2, silently corrupting the pipe. Fix sketched: inner.take() (the session's own None-arm posture) + tests. - C-2: no timeout surface on the pre-command RFC handshake or the association (bounded memory, unbounded time; the alkcall establishment bound covers only the open op). A decision to make before batch 2's doors route vanilla clients through the same phases; sketched as an ADR-level choice, not a core defect. Minors: C-3 (the substrate-free can_bind gate conflates factory-wired with allowed), D-1 (BindReply1::await_inbound is spec'd, 0% covered, never called — the two-reader ambiguity), D-2 (Socks5Producer::register + the builder's egress/accept setters unexercised — the spec'd assembly shape has no test), D-3 (error_schemas declared vec![] while params.rs's doc claims the ADR-049/ADR-016 contract; alktty declares it), S-1 (the stage's Ok(None) defense is livelock-safe but rate-unbounded — posture note), N-1 (review 001's keep-or-cut triggers fired: StandardAuthentication/skip_auth_this_is_not_rfc_compliant/ CheckResult impls are cut candidates; ConnectError/transfer/ wait_on_tcp stay as hand-port-map companions; Bug is now consumed). Coverage (94.52% regions / 95.55% lines): the load-bearing gaps attributed in N-8 — the two untested split surfaces, ASSOCIATE into_inner's Ok arm, the datagram stage's three failure exits (ReplyFrameRejected/EgressEnded/Io), read_reply2's version arm, plus single-arm cosmetics. Verified: cargo test 348 green; --all-features green; clippy --all-targets -D warnings clean; fmt --check clean; cargo doc --no-deps clean; both wasm checks clean; cargo publish --dry-run completes; cargo llvm-cov 94.52/94.67/95.55 (regions/functions/ lines). No code changed in this review — findings documented for the fix decomposition.
This commit is contained in:
glm-5.3-flash committed
1 parent
16b21e3447
commit
d612d7f7f2
1 file changed
+868
@@ -0,0 +1,868 @@
|
|||||||
|
---
|
||||||
|
status: open
|
||||||
|
last_updated: 2026-09-24
|
||||||
|
reviewed_artifacts:
|
||||||
|
- src/lib.rs
|
||||||
|
- src/wire.rs
|
||||||
|
- src/egress.rs
|
||||||
|
- src/producer.rs
|
||||||
|
- src/consumer.rs
|
||||||
|
- src/params.rs
|
||||||
|
- src/error.rs
|
||||||
|
- src/rfc1928/ (post-review-001 state, spot-checked)
|
||||||
|
- Cargo.toml
|
||||||
|
tool: manual source read + cargo test/clippy/fmt/doc/wasm/publish-dry-run + cargo-llvm-cov + alkcall/alktty/alktunnels substrate cross-checks
|
||||||
|
reviewer: general implementation review (correctness + security + smells + coverage)
|
||||||
|
scope_note: the protocol core (batch 1) only — src/ non-test regions plus
|
||||||
|
the seams they land; tests/ read for coverage attribution and
|
||||||
|
duplication assessment
|
||||||
|
---
|
||||||
|
|
||||||
|
# Review 002 — The Protocol-Core Implementation (Post-Landing Full Review)
|
||||||
|
|
||||||
|
## Purpose
|
||||||
|
|
||||||
|
General review of the batch-1 protocol core after landing — the
|
||||||
|
thirteen task group (`tasks/implementation/core-*.md`, commits
|
||||||
|
`ce8148d..16b21e3`). The narrow review
|
||||||
|
(`tasks/implementation/review-protocol-core.md`) checked spec/task
|
||||||
|
fidelity; this one asks the four questions that review did not:
|
||||||
|
|
||||||
|
1. **Correctness** — hazards in the landed producer/consumer behavior
|
||||||
|
itself (not spec conformance): re-entry paths, teardown races,
|
||||||
|
protocol-state corruption, error-shape correctness.
|
||||||
|
2. **Security** — resource exhaustion, gate fidelity, panic
|
||||||
|
surfaces, log/identity handling on the paths that now carry
|
||||||
|
real peer traffic.
|
||||||
|
3. **Code smells** — convention compliance plus the structural
|
||||||
|
smells a 13-task implementation round leaves behind (dead
|
||||||
|
surface, duplication, doc drift).
|
||||||
|
4. **Coverage** (`cargo-llvm-cov`) — important gaps, attributed to
|
||||||
|
specific behaviors and their risk.
|
||||||
|
|
||||||
|
Method note: the review is line-level over `src/` (all seven
|
||||||
|
modules), with substrate cross-checks against alkcall 0.8.0
|
||||||
|
(`pump_bidi`, `teardown_channel`, `register_openable_with_establisher`)
|
||||||
|
and the sibling crates (alktty's `ErrorDefinition` posture,
|
||||||
|
alktunnels' registration shape).
|
||||||
|
|
||||||
|
## Verification Baseline
|
||||||
|
|
||||||
|
All commands on the reviewed tree (commit `16b21e3`, clean):
|
||||||
|
|
||||||
|
- `cargo test`: **348 green** (121 lib + 38 associate + 39 bind +
|
||||||
|
40 connect + 40 integration + 30 harness + 37 differential + 3
|
||||||
|
doctests, 2 ignored).
|
||||||
|
- `cargo test --all-features`: **green** (same counts).
|
||||||
|
- `cargo clippy --all-targets -- -D warnings`: **clean**.
|
||||||
|
- `cargo fmt --check`: **clean**.
|
||||||
|
- `cargo doc --no-deps`: **clean**.
|
||||||
|
- `cargo check` + `clippy -D warnings` (wasm32): **clean**.
|
||||||
|
- `cargo publish --dry-run --allow-dirty`: **completes**.
|
||||||
|
- `cargo llvm-cov`: **94.52% regions / 94.67% functions / 95.55%
|
||||||
|
lines** (4427 covered / 197 missed).
|
||||||
|
|
||||||
|
### Coverage breakdown
|
||||||
|
|
||||||
|
| Module | Lines | Functions | Regions |
|
||||||
|
|---|---:|---:|---:|
|
||||||
|
| consumer.rs | 91.90% | 93.33% | 90.72% |
|
||||||
|
| egress.rs | 94.89% | 92.86% | 93.31% |
|
||||||
|
| error.rs | 96.15% | 90.00% | 92.52% |
|
||||||
|
| params.rs | 100.00% | 100.00% | 100.00% |
|
||||||
|
| producer.rs | 95.95% | 94.15% | 96.20% |
|
||||||
|
| rfc1928/client.rs | 96.53% | 100.00% | 94.46% |
|
||||||
|
| rfc1928/mod.rs | 97.30% | 95.65% | 93.36% |
|
||||||
|
| rfc1928/server.rs | 93.73% | 90.00% | 91.75% |
|
||||||
|
| rfc1928/util/stream.rs | 100.00% | 100.00% | 78.57% |
|
||||||
|
| rfc1928/util/target_addr.rs | 98.48% | 100.00% | 93.71% |
|
||||||
|
| wire.rs | 96.32% | 97.06% | 97.07% |
|
||||||
|
| **TOTAL** | **95.55%** | **94.67%** | **94.52%** |
|
||||||
|
|
||||||
|
Healthy for a protocol core. The missed lines decompose: ~60% are
|
||||||
|
the fork's accepted-uncovered blocks (the review-001 posture —
|
||||||
|
`StandardAuthentication`, `CheckResult`'s non-`bool` impls,
|
||||||
|
`skip_auth_this_is_not_rfc_compliant`) and test-side asserts; the
|
||||||
|
load-bearing gaps are attributed to the findings below (D-1, D-2,
|
||||||
|
N-8).
|
||||||
|
|
||||||
|
## Summary Statistics
|
||||||
|
|
||||||
|
| Severity | Count | IDs |
|
||||||
|
|----------|------:|-----|
|
||||||
|
| Critical | 0 | — |
|
||||||
|
| Major | 2 | C-1, C-2 |
|
||||||
|
| Minor | 6 | C-3, S-1, D-1, D-2, D-3, N-1 |
|
||||||
|
| Observation | 9 | S-2, N-2..N-9 |
|
||||||
|
|
||||||
|
No critical findings. The two majors: C-1 is a consumer-side
|
||||||
|
correctness hazard reachable through public API without any guard
|
||||||
|
(the BIND session's reply#2 reader re-enters on a live data plane);
|
||||||
|
C-2 is a production posture question (the pre-command handshake and
|
||||||
|
the association have no timeout surface — bounded memory, unbounded
|
||||||
|
time) that needs a decision before batch 2's doors route vanilla
|
||||||
|
clients through the same phases. One dead-surface finding (N-1)
|
||||||
|
closes out review 001's "cut if unconsumed" triggers, which have
|
||||||
|
now fired; one security note (S-1) records the datagram stage's
|
||||||
|
residual hot-loop posture; the rest are small.
|
||||||
|
|
||||||
|
Prefixes, aligned with review 001's legend extended to the whole
|
||||||
|
crate: `C` (correctness), `S` (security posture), `D` (dead
|
||||||
|
surface/API), `N` (observations/notes/smells).
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
# Part A — Findings
|
||||||
|
|
||||||
|
## C-1 [major] — `BindSession::await_inbound` (and `BindReply1::await_inbound`) are re-entrant with no spent-marker: a second call re-reads the *data plane* as reply#2, silently corrupting the pipe
|
||||||
|
|
||||||
|
**Files**: `src/consumer.rs:757-763` (`BindSession::await_inbound`),
|
||||||
|
`src/consumer.rs:561-567` (`BindReply1::await_inbound`), contrast:
|
||||||
|
`src/consumer.rs:176-184` (`Socks5ChannelSession::into_inner`'s
|
||||||
|
`SpentSessionError` shape)
|
||||||
|
|
||||||
|
**Problem**: `BindSession` is the crate's one two-phase session: the
|
||||||
|
constructor reads reply#1, and reply#2 is read later, on demand, by
|
||||||
|
`await_inbound`. The method takes `&mut self` and leaves `inner`
|
||||||
|
(`Option<Socks5Stream<BiStream>>`) populated after success — the
|
||||||
|
returned `BindDataPlane<'_>` *borrows* the same stream. Nothing
|
||||||
|
marks the reply#2 phase as consumed. A caller that invokes
|
||||||
|
`await_inbound` twice (a plausible misuse: an app-server wrapper
|
||||||
|
retry, a front-door bridge that polls, a future refactor) gets this:
|
||||||
|
|
||||||
|
1. The producer side has moved to the pass-through pump
|
||||||
|
(`pump_bidi` on the producer's quiet-wait exit) — post-reply#2
|
||||||
|
bytes are the accepted peer's raw application data.
|
||||||
|
2. The second `await_inbound` runs `read_reply2` against those
|
||||||
|
bytes: reads 4 bytes as `VER/REP/RSV/ATYP` and an address after
|
||||||
|
them, from the middle of the peer's data stream.
|
||||||
|
3. If the peer's first bytes do not start with `0x05`, the caller
|
||||||
|
gets a baffling `UnsupportedSocksVersion` (the pipe is already
|
||||||
|
desynchronized — the 4+ bytes were consumed either way). If they
|
||||||
|
*do* start with `0x05` (SOCKS-in-SOCKS, any binary protocol
|
||||||
|
whose first byte is 5), the read silently eats 10+ bytes of the
|
||||||
|
peer's data, fabricates a `BindDataPlane`, and returns `Ok` —
|
||||||
|
the corruption is invisible.
|
||||||
|
|
||||||
|
The same shape exists through the other door: `session.reply1()`
|
||||||
|
returns a fresh `BindReply1<'_>` handle each call (consumer.rs:745),
|
||||||
|
and two sequential `reply1()` → `await_inbound()` calls replay the
|
||||||
|
same double-read. The sibling sessions are all *safe* here — that is
|
||||||
|
what makes this one inconsistent rather than a design choice:
|
||||||
|
|
||||||
|
- `Socks5ChannelSession`: the full conversation is consumed by the
|
||||||
|
constructor; `into_inner` is the only taking method and carries
|
||||||
|
the `SpentSessionError` guard (review 002 R-2's shape).
|
||||||
|
- `AssociateSession`: `send_datagram`/`recv_datagram` are a proper
|
||||||
|
state machine (reader state, spent-stream `Io`/`None` shapes);
|
||||||
|
no re-entry ambiguity.
|
||||||
|
|
||||||
|
The fork's client typestate has the same latent shape
|
||||||
|
(`Socks5Stream::request` is callable repeatedly and would re-send a
|
||||||
|
command header onto the data plane), but it is pre-data-plane
|
||||||
|
machinery owned by the constructors — `BindSession` is where the
|
||||||
|
phase boundary crosses into caller reach.
|
||||||
|
|
||||||
|
**Fix (small)**: make the reply#2 phase consumed, in the session's
|
||||||
|
established posture:
|
||||||
|
|
||||||
|
- `BindSession::await_inbound` reads `inner.take()` — a second call
|
||||||
|
hits the `None` arm and returns the existing `Ok(None)` ("spent"
|
||||||
|
— consumer.md's posture for post-teardown reads). This is the
|
||||||
|
smallest change and matches the session's own `None`-arm doc
|
||||||
|
("`None` after a taken plane").
|
||||||
|
- `BindReply1::await_inbound` consumes the handle already; the
|
||||||
|
re-entry path is `reply1()` twice — either mark the session
|
||||||
|
consumed at `reply1()` (it is a borrow, so the flag rides the
|
||||||
|
session), or document that `reply1()` is a view and the session's
|
||||||
|
`await_inbound` is the normative reader (and demote
|
||||||
|
`BindReply1::await_inbound` to a private helper).
|
||||||
|
|
||||||
|
One test each: double `await_inbound` returns `Ok(None)` (or the
|
||||||
|
typed spent error, if the `SpentSessionError` posture is preferred
|
||||||
|
for consistency), and the data-plane bytes survive untouched.
|
||||||
|
|
||||||
|
**Lift**: closes the one reachable silent-corruption path in the
|
||||||
|
consumer surface. Everything else in the session types is
|
||||||
|
spent-state-guarded; this is the gap.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## C-2 [major] — The pre-command RFC handshake and the association have no timeout surface: a silent channel holds a spawned task and reader state indefinitely (bounded memory, unbounded time) — fine per contract, but the production posture needs a decision
|
||||||
|
|
||||||
|
**Files**: `src/producer.rs:183-191` (the pump task parks on
|
||||||
|
`accept_bi` → `accept_no_auth` → `read_command` with no timeout),
|
||||||
|
`src/producer.rs:350-420` (the datagram stage's `loop` — exits only
|
||||||
|
on EOF/error/data), contrast: the establishment bound
|
||||||
|
(`register_socks5_openable`, `timeout: None` → alkcall's 10s
|
||||||
|
default, covers only the open-op round trip)
|
||||||
|
|
||||||
|
**Problem**: the alkcall 10s establishment bound governs the
|
||||||
|
*open-op call*, not the in-stream conversation. After the channel
|
||||||
|
opens, the pump handler spawns its task and parks on the RFC
|
||||||
|
handshake; a consumer that opens a channel and never speaks holds —
|
||||||
|
per idle channel — one spawned task (parked on `read_exact`) and
|
||||||
|
one channel entry (bounded buffers, ADR-040). The same holds for an
|
||||||
|
established association (lifetime = channel lifetime is the
|
||||||
|
contract, wire.md §UDP ASSOCIATE) and a parked BIND quiet wait
|
||||||
|
(teardown-governed is the *decision* there — ADR-007, "FTP-length
|
||||||
|
waits stay legal").
|
||||||
|
|
||||||
|
The mitigations are real: the 256-channel per-connection cap
|
||||||
|
(ADR-041) bounds the count, memory per idle channel is bounded, and
|
||||||
|
the teardown paths all fire on channel close (the harness pins
|
||||||
|
client-gone teardowns). Nothing leaks; nothing grows. But the
|
||||||
|
aggregate is: any identity with `socks5:open` can hold 256 parked
|
||||||
|
tasks + 256 channel entries indefinitely *without sending a byte* —
|
||||||
|
and the RFC 1928 conversation itself, unlike BIND's quiet wait, was
|
||||||
|
never given a timeout *decision* (the quiet wait got one; the
|
||||||
|
handshake and the association lifetime got "inherited from the
|
||||||
|
channel").
|
||||||
|
|
||||||
|
**Why file it now**: batch 2's providers (`local`, `composed`) are
|
||||||
|
about to route *vanilla clients* through these same phases via the
|
||||||
|
front doors, where the peer is no longer a wrapper-aware consumer
|
||||||
|
but a kernel socket (often outside alkcall's channel-cap regime
|
||||||
|
entirely — a door's accepted TCP connection is not a channel).
|
||||||
|
Slowloris-shaped holds (open the door, send the greeting byte,
|
||||||
|
stop) become the assembly layer's problem, and the protocol layer
|
||||||
|
is where a timeout seam would live.
|
||||||
|
|
||||||
|
**Fix (decision, not code)**: an ADR-level note deciding one of:
|
||||||
|
|
||||||
|
1. **Idle timeout is the substrate's/assembly's job** — the
|
||||||
|
protocol layer never times out (consistent with "no in-band
|
||||||
|
control vocabulary" and association-lifetime-equals-channel);
|
||||||
|
record the DoS posture explicitly (the cap bounds it; doors
|
||||||
|
apply their own accept timeouts). Cheapest, defensible.
|
||||||
|
2. **A producer-side idle bound** — an optional per-session
|
||||||
|
inactivity timeout at the pump level (config on
|
||||||
|
`Socks5ProducerBuilder`), enforced by racing the handshake/stage
|
||||||
|
reads with a sleep timer; a timeout = clean EOF-shaped teardown,
|
||||||
|
no reply (the RFC has no timeout reply code; 0x01
|
||||||
|
`GeneralFailure` on a pre-reply timeout is the closest and is
|
||||||
|
what upstream's connect-timeout path surfaces).
|
||||||
|
|
||||||
|
Option 2 is more work and touches no wire format; option 1 needs
|
||||||
|
only the posture written down (it is currently *implicit* — the
|
||||||
|
review-002 gate checked teardown, not holds). Either way the
|
||||||
|
decision should land before the `local` doors exist, not after.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## C-3 [minor] — `Socks5Producer::serve_socks5_over` derives `can_bind` from `accept.is_some()`: factory-wired ⇒ BIND allowed, with no way to say "factory wired but gate closed" on the substrate-free entry
|
||||||
|
|
||||||
|
**Files**: `src/producer.rs:630-648` (`serve_socks5_over`'s
|
||||||
|
`self.accept.is_some()`), doc: "The command gate is resolved from
|
||||||
|
the builder's `accept` presence"
|
||||||
|
|
||||||
|
**Problem**: the pump handler (the channels path) resolves the gate
|
||||||
|
from the per-call opener identity (ADR-006 — correct). The
|
||||||
|
substrate-free entry derives it from seam *presence*: wiring an
|
||||||
|
accept factory flips the gate open for every caller of
|
||||||
|
`serve_socks5_over`, and there is no way to express "this producer
|
||||||
|
has a BIND factory but the local policy denies BIND" without
|
||||||
|
dropping the factory (losing BIND entirely). For the current
|
||||||
|
in-tree uses (tests, the harness) the conflation is harmless; for
|
||||||
|
the batch-2 `local` backend — the documented consumer of this entry
|
||||||
|
(producer.md: "the batch-2 local backend rides the same
|
||||||
|
registration") — it means the local door's BIND authorization has
|
||||||
|
no seam to plug into except factory presence.
|
||||||
|
|
||||||
|
This is a real-but-early API-shape hazard: it is exactly the shape
|
||||||
|
that invites a batch-2 shortcut where "listener configured" becomes
|
||||||
|
"listener permitted" (ADR-006's whole point was to split those).
|
||||||
|
|
||||||
|
**Fix (small, pre-batch-2)**: widen the substrate-free entry's gate
|
||||||
|
to an explicit policy input — e.g.
|
||||||
|
`Socks5Producer::serve_socks5_over_with_gate(stream, can_bind)` or
|
||||||
|
a builder field `.bind_policy(bool)` defaulting to
|
||||||
|
`accept.is_some()` for backward-compat with the landed tests. The
|
||||||
|
pump handler's identity-derived gate is unaffected. One test: a
|
||||||
|
producer with an accept factory + explicit `can_bind=false` refuses
|
||||||
|
0x02.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## S-1 [minor] — No idle/livelock bound on the datagram stage's `Ok(None)` tolerance is correct, but the stage's *reply* branch can spin hot against a chatty-but-empty egress — bounded by `yield_now`, unbounded by rate
|
||||||
|
|
||||||
|
**Files**: `src/producer.rs:383` (`Ok(None) => yield_now`),
|
||||||
|
`src/producer.rs:356` (the 16 KB read chunk)
|
||||||
|
|
||||||
|
**Problem**: the R-1 hardening makes the stage *livelock-safe*
|
||||||
|
against a zero-yield hot-`Ok(None)` egress, which is the right
|
||||||
|
structural guard. The residual posture: a *misbehaving* egress that
|
||||||
|
returns `Ok(None)` at full speed still costs one `yield_now` +
|
||||||
|
re-poll per cycle — the stage becomes a busy-wait consumer of CPU
|
||||||
|
while making no progress, indefinitely, on a current-thread
|
||||||
|
scheduler it now also *yields to* (so the runtime survives, but the
|
||||||
|
stage never parks). A compliant egress parks (the contract); a
|
||||||
|
buggy one degrades to a hot loop that yields. The contract
|
||||||
|
(`egress.rs:174-185`) says "implement the wait as a real async
|
||||||
|
park" — the stage defends itself, but nothing bounds the defense.
|
||||||
|
|
||||||
|
Also in this family: the 16 KB read chunk is fixed regardless of
|
||||||
|
the channel's chunking, and `DatagramReader`'s `BytesMut` retains
|
||||||
|
peak capacity (a 65535-byte frame leaves ~64 KiB of capacity
|
||||||
|
resident for the association's lifetime — per association, bounded,
|
||||||
|
but unshrunk).
|
||||||
|
|
||||||
|
**Recommendation**: accepted-uncovered posture note rather than a
|
||||||
|
fix — the stage's contract violation handling is defensive
|
||||||
|
correctness (a *compliant* egress never hot-`None`s), and a
|
||||||
|
backoff/budget in the stage would penalize the compliant path. One
|
||||||
|
line on `run_datagram_stage`'s doc naming the residual
|
||||||
|
(hot-`Ok(None)` egress = a yielding busy-wait, bounded by the
|
||||||
|
egress's own correctness), or a cheap consecutive-`Ok(None)`
|
||||||
|
budget that re-parks the select for a tick after N violations.
|
||||||
|
Low priority; filed so the residual is a named decision, not an
|
||||||
|
accident.
|
||||||
|
|
||||||
|
## S-2 [observation] — `teardown_channel`'s returned `JoinHandle` is dropped by every consumer-side `reap()`: teardown is EOF-propagated and detached, which is correct here, but the contract should say so
|
||||||
|
|
||||||
|
**Files**: `src/consumer.rs:197-202` (CONNECT `reap`),
|
||||||
|
`src/consumer.rs:490-495` (ASSOCIATE `reap`),
|
||||||
|
`src/consumer.rs:787-792` (BIND `reap`);
|
||||||
|
substrate: alkcall `manager.rs:521-536` (`teardown_channel` returns
|
||||||
|
`Option<JoinHandle>` for the caller to drain-then-abort)
|
||||||
|
|
||||||
|
**Problem**: alkcall's contract for `teardown_channel`'s returned
|
||||||
|
handle is "the caller can abort it after the drain completes"
|
||||||
|
(the channels-call path's shape). The consumer sessions discard it
|
||||||
|
(`.is_ok()` on the result), leaving the producer-side pump task
|
||||||
|
detached to finish via EOF propagation (the dropped demux sender
|
||||||
|
EOFs the producer's read half → `pump_bidi` completes). This is the
|
||||||
|
*graceful* teardown — aborting would cut mid-flight data — and it
|
||||||
|
is correct. But it silently depends on EOF propagation completing
|
||||||
|
without the producer-side task being reaped by anyone: on a
|
||||||
|
`clear_all` (transport EOF) alkcall aborts the tasks; on a
|
||||||
|
single-channel teardown the task runs to its natural end, holding
|
||||||
|
the accepted/dialed substrate resources until EOF lands. If a
|
||||||
|
provider's dial seam holds a resource whose drop is
|
||||||
|
expensive-but-async (a composed variant's channel session), the
|
||||||
|
teardown latency is unbounded by anything visible in this crate.
|
||||||
|
|
||||||
|
**Recommendation**: one doc line on the consumer sessions' `reap`
|
||||||
|
("the returned pump task is left to finish via EOF propagation —
|
||||||
|
the graceful shape; `clear_all` aborts instead, the substrate's
|
||||||
|
abrupt path") so the next reader does not "fix" the dropped handle
|
||||||
|
into an abort. No code change.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## D-1 [minor] — `BindReply1::await_inbound` is dead surface: spec'd, documented, and never exercised (0% covered); the session's `await_inbound` is the only reader any test drives
|
||||||
|
|
||||||
|
**Files**: `src/consumer.rs:561-567` (`BindReply1::await_inbound` —
|
||||||
|
uncovered), `src/consumer.rs:569-573` (`raw_mut` — covered), test
|
||||||
|
usage: `tests/consumer-bind.rs:97,442` (struct destructured for
|
||||||
|
`bind_addr` only), task spec: `tasks/implementation/
|
||||||
|
core-consumer-bind.md:74-75` (the handle named as the reply#1
|
||||||
|
surface)
|
||||||
|
|
||||||
|
**Problem**: every BIND test goes through
|
||||||
|
`BindSession::await_inbound` (consumer.rs:757). `BindReply1`'s own
|
||||||
|
reader is dead — the four `reply1()` call sites destructure
|
||||||
|
`bind_addr` or use `raw_mut`, never `await_inbound`. This is the
|
||||||
|
same shape review 001's C-2 flagged in the fork
|
||||||
|
(`StandardAuthentication` — spec'd machinery with zero callers),
|
||||||
|
and its stated trigger applies here too: "if it lands without
|
||||||
|
consuming it, cut it then." The consumer layer has landed; nothing
|
||||||
|
consumed it. Unlike C-1's double-read hazard (which lives in the
|
||||||
|
*same method body*), this is about surface hygiene: two ways to do
|
||||||
|
the same read, one untested, both public, in a crate whose session
|
||||||
|
types otherwise enforce single-phase discipline structurally.
|
||||||
|
|
||||||
|
**Fix**: pick one reader. Either (a) make `BindReply1::await_inbound`
|
||||||
|
the normative path (the session's method delegates to it) and test
|
||||||
|
it through the handle — the task spec's intent, and it gives the
|
||||||
|
reply#1-phase handle a real reader; or (b) cut
|
||||||
|
`BindReply1::await_inbound` + `BindReply1`'s status as more than a
|
||||||
|
data carrier (keep `bind_addr` + `raw_mut`, which *are* used).
|
||||||
|
C-1's fix interacts with this choice.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## D-2 [minor] — `Socks5Producer::register` and the builder's `egress`/`accept` setters are unexercised public surface (0% covered)
|
||||||
|
|
||||||
|
**Files**: `src/producer.rs:653-669` (`Socks5Producer::register`),
|
||||||
|
`src/producer.rs:680-697` (the builder's `egress`/`accept`
|
||||||
|
setters — only `.dial` rides `builder_shape_round_trips`), harness:
|
||||||
|
`tests/harness.rs:668` (registration goes through the free function
|
||||||
|
`register_socks5_openable`, not the builder)
|
||||||
|
|
||||||
|
**Problem**: `producer.md` §Wiring presents
|
||||||
|
`producer.register(&core, ®istry, auth, None)?` as *the* assembly
|
||||||
|
shape — the spec'd entry point — and the landed doc (producer.md
|
||||||
|
`Wiring`, reconciled by review 002 R-4) names exactly this
|
||||||
|
signature. No test exercises it: the harness re-implements the
|
||||||
|
registration via `register_socks5_openable` directly (same
|
||||||
|
delegation, different call path), and the builder test drives only
|
||||||
|
`serve_socks5_over`. So the one assembly shape the spec documents
|
||||||
|
is the one assembly path with no test — and the builder's
|
||||||
|
`egress`/`accept` setters are compile-checked only. A signature
|
||||||
|
drift or a delegation bug in `register` would pass the suite.
|
||||||
|
|
||||||
|
**Fix (small)**: one integration test wiring a full
|
||||||
|
`Socks5Producer::builder().dial(...).egress(...).accept(...)`
|
||||||
|
through `.register(...)` and serving all three commands over the
|
||||||
|
harness topology. Closes the gap and exercises the spec'd shape
|
||||||
|
end to end (it is also the natural home for C-3's gate test).
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## D-3 [minor] — `error_schemas` is declared `vec![]` while `params.rs`'s doc claims the spec follows ADR-049 §4 + ADR-016's typed error contract — alktty (the first protocol crate) declares the `channel:open_failed` `ErrorDefinition`, alksocks does not
|
||||||
|
|
||||||
|
**Files**: `src/params.rs:94-96` (the doc claim),
|
||||||
|
`src/params.rs:117` (`vec![]` — the actual spec), precedent:
|
||||||
|
`/workspace/@alkdev/alktty/src/channels.rs:198-208` (alktty declares
|
||||||
|
the `ErrorDefinition` with the `reason`/`message` schema and pins it
|
||||||
|
by test), sibling: alktunnels also passes `vec![]`
|
||||||
|
|
||||||
|
**Problem**: ADR-049 §4 says ALPN crates' open-op specs gain
|
||||||
|
matching `ErrorDefinition` entries per ADR-016 so `services/schema`
|
||||||
|
discloses the failure contract; alktty did exactly that (the
|
||||||
|
`channel:open_failed` definition + a schema + a test asserting the
|
||||||
|
declaration). alksocks's `socks5_open_spec` passes an empty vec —
|
||||||
|
but its module doc says "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)." The doc
|
||||||
|
describes a contract the spec does not carry. The typed vocabulary
|
||||||
|
works end to end *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`.
|
||||||
|
|
||||||
|
**Fix (small)**: either declare the `ErrorDefinition` (port
|
||||||
|
alktty's — the schema for `{ reason, message }` is already written
|
||||||
|
there; one test mirrors alktty's) or reword the doc to the honest
|
||||||
|
posture ("empty — the typed contract is behavioral via
|
||||||
|
`SocksOpenError::reason`; the `ErrorDefinition` pass is
|
||||||
|
crate-family-wide, matching alktunnels, pending"). The declare-side
|
||||||
|
is the better fix and is one ADR-cited block; the reword is the
|
||||||
|
honest minimum. Doc-vs-code drift either way gets fixed.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## N-1 [minor] — The review-001 keep-or-cut triggers have fired: `ConnectError`, `SocksServerError::Bug`, `StandardAuthentication`, `CheckResult`'s non-`bool` impls, `skip_auth_this_is_not_rfc_compliant`, `transfer`/`wait_on_tcp` — the producer layer landed without consuming any of them
|
||||||
|
|
||||||
|
**Files**: `src/rfc1928/util/stream.rs:56-83` (`ConnectError` —
|
||||||
|
constructed nowhere), `src/rfc1928/server.rs:72-73` (`Bug` — now
|
||||||
|
consumed by the producer's in-stream refusal sites; see below),
|
||||||
|
`src/rfc1928/server.rs:448-482`
|
||||||
|
(`StandardAuthentication` + `allow_no_auth`), `src/rfc1928/
|
||||||
|
server.rs:162-172` (`CheckResult` `Option`/`Result` impls),
|
||||||
|
`src/rfc1928/server.rs:185-187` (`skip_auth_this_is_not_rfc_
|
||||||
|
compliant`), `src/rfc1928/server.rs:661-667` (`transfer`) +
|
||||||
|
`647-657` (`wait_on_tcp` — differential-suite-consumed only)
|
||||||
|
|
||||||
|
**Problem**: review 001's resolution batch 3 kept four surfaces with
|
||||||
|
an explicit trigger: "if the producer layer lands without consuming
|
||||||
|
it, cut it then." The producer layer has landed (`make_socks5_pump_
|
||||||
|
handler` drives `negotiate_auth` with `&[NoAuthentication]` only;
|
||||||
|
the dial/egress/accept seams carry their own error vocabularies;
|
||||||
|
`pump_bidi` replaced `transfer`). None of the anticipated consumers
|
||||||
|
materialized. Two corrections to the trigger's assumptions, in the
|
||||||
|
fork's favor: `SocksServerError::Bug` now *does* have in-tree
|
||||||
|
constructors (the producer's four `Bug(...)` refusal sites plus the
|
||||||
|
gate site return `Bug("... refused in-stream")` — review 001's
|
||||||
|
dead-code posture partially resolved by use), and `transfer`/`wait_on_tcp`
|
||||||
|
are consumed by the differential suite (their semantics stay
|
||||||
|
pinned). But `ConnectError` (constructed nowhere), the
|
||||||
|
`StandardAuthentication` machinery (zero callers), `CheckResult`'s
|
||||||
|
`Option`/`Result` impls (every checker returns `bool`), and
|
||||||
|
`skip_auth_this_is_not_rfc_compliant` (no caller; upstream's own
|
||||||
|
legacy-layer companion) are still consumerless — now with the
|
||||||
|
trigger condition met.
|
||||||
|
|
||||||
|
This is the crate's own stated lean ("no speculative surface,"
|
||||||
|
ADR-013's sweep posture) versus the fork's hand-port-map posture
|
||||||
|
(ADR-013 also says keep the companions). The distinction that
|
||||||
|
matters: the hand-port map is for *code a future port would reuse*
|
||||||
|
(`ConnectError`'s reply-code mapping), while the rest is machinery
|
||||||
|
that the *producer already replaced* (`StandardAuthentication` —
|
||||||
|
the pump handler hard-codes `&[NoAuthentication]`; if username/
|
||||||
|
password ever arrives it arrives at the *door* per ADR-005, not in
|
||||||
|
the typestate).
|
||||||
|
|
||||||
|
**Fix (decision)**: one posture pass, small:
|
||||||
|
- **Cut**: `StandardAuthentication` + `allow_no_auth` +
|
||||||
|
`auth_method_enums!`'s generated enum (the macro's doc says
|
||||||
|
"generate a similar enum" — the macro itself can stay or go with
|
||||||
|
it; nothing else uses it), `skip_auth_this_is_not_rfc_compliant`
|
||||||
|
(the consumer typestate's `skip_auth` config covers the
|
||||||
|
in-tree need), `CheckResult`'s `Option`/`Result` impls (keep
|
||||||
|
`bool`; re-add with a typed-rejection design if a door ever wants
|
||||||
|
it).
|
||||||
|
- **Keep with note**: `ConnectError` (the hand-port map's
|
||||||
|
companion, already documented), `transfer`/`wait_on_tcp`
|
||||||
|
(differential-suite-consumed; upstream-faithful helpers),
|
||||||
|
`Bug` (now consumed by the producer).
|
||||||
|
The coverage blocks resolve automatically.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## N-2 [observation] — `run_datagram_stage`'s inline `egress.send` per frame is head-of-line blocking on the read side — documented as sufficient, and it is, but the bound deserves one line in wire.md
|
||||||
|
|
||||||
|
**Files**: `src/producer.rs:403-408` (send awaited inline before the
|
||||||
|
next select iteration), doc: `src/producer.rs:345-349` ("sufficient
|
||||||
|
for relay postures"), spec: wire.md §UDP ASSOCIATE
|
||||||
|
|
||||||
|
**Problem**: a slow `egress.send` blocks both the next channel read
|
||||||
|
*and* the next reply drain (the select is single-task). For the
|
||||||
|
relay postures the POC measured this is fine (the doc says so), and
|
||||||
|
the alternative (spawn-per-datagram or a send queue) contradicts
|
||||||
|
the one-relay-loop shape (convention 10). The residual — one slow
|
||||||
|
destination delays *all* datagram egress on that association — is
|
||||||
|
the same behavior a kernel-socket egress has
|
||||||
|
(`send_to` backpressure), so this is likely a non-issue in
|
||||||
|
practice. Filed as a spec line, not a fix: wire.md's datagram-stage
|
||||||
|
bullet should state "per-datagram `send` is awaited inline (the
|
||||||
|
stage interleaves sends with reply drains); a slow egress applies
|
||||||
|
backpressure to the association, not to other channels" so the
|
||||||
|
batch-2 composed-egress author inherits the bound knowingly.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## N-3 [observation] — `DatagramReader`'s `let-else` at wire.rs:229-231 is structurally unreachable (dead defensive branch)
|
||||||
|
|
||||||
|
**Files**: `src/wire.rs:229-231`
|
||||||
|
|
||||||
|
**Problem**: inside `feed`'s loop, the `let Some((declared, acc)) =
|
||||||
|
self.current.as_mut() else { break }` else-arm can never fire: if
|
||||||
|
`current` was `None` on entry and `buf.len() >= 2`, the block above
|
||||||
|
*sets* `current` before the let-else; if `buf.len() < 2`, the loop
|
||||||
|
broke at line 223. Coverage confirms it (0 executions). It is
|
||||||
|
harmless (compiler-required exhaustiveness for the borrow, arguably)
|
||||||
|
but it is the kind of branch that invites "what makes this fire?"
|
||||||
|
misreadings.
|
||||||
|
|
||||||
|
**Fix**: restructure to `let Some(&mut (declared, ref mut acc))`
|
||||||
|
matching on the already-known-`Some` state, or drop the else by
|
||||||
|
re-ordering (`if let Some(...) = ... { ...; continue; } break;`).
|
||||||
|
Zero behavior change; one-line simplification.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## N-4 [observation] — The gate's in-stream refusal returns `SocksServerError::AuthenticationRejected` (and factory refusals return `Bug("...")`): the *task-log* error vocabulary overloads semantics the wire never sees
|
||||||
|
|
||||||
|
**Files**: `src/producer.rs:230-233` (BIND gate →
|
||||||
|
`AuthenticationRejected`), `src/producer.rs:262-264` (dial refusal
|
||||||
|
→ `Bug("dial refused in-stream")`), `src/producer.rs:302-304`
|
||||||
|
(egress factory → `Bug(...)`), `src/producer.rs:483-485` +
|
||||||
|
`541-543` (bind factory/accept → `Bug(...)`)
|
||||||
|
|
||||||
|
**Problem**: these `Err` returns are log/observability signals only
|
||||||
|
(the wire got its RFC-faithful reply already; the caller surfaces
|
||||||
|
nothing). But the vocabulary mislabels: an ACL refusal is not
|
||||||
|
"authentication rejected," and a seam refusing per policy is not a
|
||||||
|
"BUG" (the variant's display literally says `BUG: ...` — review 001
|
||||||
|
C-1's misleading-display note). A future operator grepping `BUG:`
|
||||||
|
in producer logs chases ghosts that are *working as designed*
|
||||||
|
refusals. The forked `SocksServerError` vocabulary has no
|
||||||
|
"refused-by-policy" variant because upstream never needed one.
|
||||||
|
|
||||||
|
**Fix (small)**: add one variant
|
||||||
|
(`SocksServerError::Refused(&'static str)`-shaped, or reuse
|
||||||
|
`UnknownCommand`-style typed arms) for the in-stream seam refusals
|
||||||
|
and the command gate, and use it at the five refusal sites (four
|
||||||
|
`Bug`, one `AuthenticationRejected`). The wire bytes
|
||||||
|
are untouched; only the log vocabulary becomes honest. Alternatively
|
||||||
|
keep `Bug` and rename its display (it *is* consumed now, so the
|
||||||
|
review-001 "misleading in a no-panic crate" note is live).
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## N-5 [observation] — The producer's test module re-implements the harness's shape four times (structural, but collapsible)
|
||||||
|
|
||||||
|
**Files**: `src/producer.rs:954-987` (`QueueEgress`),
|
||||||
|
`1237-1274` (`HotNoneEgress`), `1321-1359` (`PureHotNoneEgress`),
|
||||||
|
`1504-1553` (`Witness` egress) — four `Egress` impls of the same
|
||||||
|
queue shape; `src/producer.rs:792-814` (`greet`/`write_request`/
|
||||||
|
`read_reply`) vs `tests/harness.rs:876-933` (same trio); echo-dial
|
||||||
|
at `src/producer.rs:719-745` vs `tests/harness.rs:131`
|
||||||
|
|
||||||
|
**Problem**: ~250 test lines are structural duplicates of the
|
||||||
|
harness's helpers. The cause is legitimate (unit tests in
|
||||||
|
`src/` cannot import `tests/harness.rs` — separate crates), and the
|
||||||
|
variants are not identical (the hot-none egress is deliberately
|
||||||
|
different per R-1's two shapes; `Witness` carries the drop flag).
|
||||||
|
But `QueueEgress`/`echo_egress_factory` and the `greet`/`reply`
|
||||||
|
trio are byte-near-identical to the harness's.
|
||||||
|
|
||||||
|
**Fix (small)**: a `tests/common/` path (or a `#[cfg(test)]`
|
||||||
|
`pub(crate)` module in `src/` compiled into both the lib test and
|
||||||
|
an integration test via `path`-included module — the standard
|
||||||
|
`mod common` pattern) shared by the producer unit tests and the
|
||||||
|
integration suite. Consolidates ~200 lines and keeps the two
|
||||||
|
harness shapes from diverging (they already have: the harness's
|
||||||
|
`ChanEgress` and producer's `QueueEgress` differ only in the mutex
|
||||||
|
wrapper). Cosmetic-to-moderate; do it when the next test round
|
||||||
|
touches either file, not as a standalone.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## N-6 [observation] — `consumer.rs`'s three session constructors share a near-identical prologue (open → BiStream → negotiated → teardown-on-err) — a small `open_negotiated` helper would collapse it
|
||||||
|
|
||||||
|
**Files**: `src/consumer.rs:121-149` (CONNECT),
|
||||||
|
`src/consumer.rs:290-324` (ASSOCIATE), `src/consumer.rs:702-733`
|
||||||
|
(BIND) — three copies of: `open_channel(OP_SOCKS5_OPEN,
|
||||||
|
Socks5Params::as_json(), SOCKS5_ALPN)` → `BiStream::from_joined` →
|
||||||
|
an `async` negotiation closure → `match` with
|
||||||
|
`teardown_channel(channel_id)` on `Err`
|
||||||
|
|
||||||
|
**Problem**: the three prologues differ only in the negotiation
|
||||||
|
body (the command request + what it returns). A shared
|
||||||
|
`async fn open_and_negotiate<T>(client, negotiate: impl FnOnce
|
||||||
|
(Socks5Stream<BiStream>) -> ...)` collapses ~30 lines × 3 to one
|
||||||
|
helper + three one-liners, and — more importantly — makes the
|
||||||
|
teardown-at-birth invariant (the `Err` arm reaps the adopted
|
||||||
|
channel) *one* place instead of three. A fourth constructor (any
|
||||||
|
batch-2 variant) today means a fourth hand-copied teardown path.
|
||||||
|
|
||||||
|
**Fix (small)**: one private helper; zero behavior change. Pair
|
||||||
|
with the C-1 fix's test round.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## N-7 [observation] — `AddrError::to_reply_error` maps *every* non-`IncorrectAddressType` failure to `ConnectionRefused` (0x05) — faithful to upstream, but semantically wrong for decode-side read failures
|
||||||
|
|
||||||
|
**Files**: `src/rfc1928/util/target_addr.rs:45-52`, consumers:
|
||||||
|
`try_notify!`'s `read_address` failures (a truncated command
|
||||||
|
address → 0x05), the consumer's `wire_error` mapping
|
||||||
|
(`src/consumer.rs:818-826` rides the same class)
|
||||||
|
|
||||||
|
**Problem**: an `AddrError::PortNumberUnreadable`/`DomainContent
|
||||||
|
Unreadable`/`IPv6Unreadable` (stream ended mid-address — an I/O
|
||||||
|
shape) surfaces as `ConnectionRefused` if it ever rides
|
||||||
|
`to_reply_error`. In practice the producer's `read_command` path
|
||||||
|
errors out before any reply on a mid-read EOF (the `?` short-circuits
|
||||||
|
to the session error, not a reply — `try_notify!` only fires for
|
||||||
|
`IncorrectAddressType`-class *typed* failures... actually it fires
|
||||||
|
for every `AddrError` — a mid-address EOF on a duplex produces a
|
||||||
|
0x05 reply). RFC-wise 0x05 is the catch-all upstream picked; a
|
||||||
|
mid-read EOF is arguably a 0x01 `GeneralFailure`. Upstream-identical
|
||||||
|
behavior (the mapping table is upstream's); the observable
|
||||||
|
difference is one reply code on a malformed/truncated peer.
|
||||||
|
|
||||||
|
**Recommendation**: none (upstream-faithful, wire-tested both ways
|
||||||
|
in the differential suite's domain-cap probes). Recorded to bound
|
||||||
|
re-litigation: if the reply-code mapping is ever revisited, this is
|
||||||
|
the one arm where "faithful" and "correct" diverge.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## N-8 [observation] — Coverage residue worth one line each (or accepted-uncovered with a note)
|
||||||
|
|
||||||
|
The full `--show-missing-lines` sweep attributed; these are the
|
||||||
|
residuals beyond D-1/D-2 (each is small and named here so the next
|
||||||
|
coverage run needs no re-derivation):
|
||||||
|
|
||||||
|
- `consumer.rs:356-359` — `send_datagram`'s spent-stream `Io` arm:
|
||||||
|
untested (the spent-`recv` arm at :396-398 *is* covered). One
|
||||||
|
test mirroring `spent_session_*` shapes.
|
||||||
|
- `consumer.rs:419` — `recv_datagram`'s mid-batch *later-frame*
|
||||||
|
`Err` arm (first frame `Ok`, a later one garbage → the pending
|
||||||
|
`Ok`s pushed before the error are discarded with the session):
|
||||||
|
one test would pin the fail-loud-drops-pending semantics.
|
||||||
|
- `consumer.rs:437, 771-774` — `AssociateSession::split` and
|
||||||
|
`BindSession::split`: never called on a *live* stream by any test
|
||||||
|
(ASSOCIATE's one call site is the spent-session probe's `None`
|
||||||
|
arm; BIND's is 0 outright; CONNECT's split *is*, heavily). The
|
||||||
|
two BIND/ASSOCIATE splits are the front-door-bridge surfaces
|
||||||
|
(consumer.md) — either one test each or a "batch-2 door
|
||||||
|
exercises it" note; they are load-bearing for the UDP/BIND
|
||||||
|
doors.
|
||||||
|
- `consumer.rs:453-456` — `AssociateSession::into_inner`'s `Ok`
|
||||||
|
arm (the `TakenAssociateInner` construction): CONNECT's
|
||||||
|
`into_inner` is tested (3 call sites); ASSOCIATE's is never
|
||||||
|
taken in any test. One test mirroring
|
||||||
|
`into_inner_pairs_the_pipe_with_the_teardown_owner`.
|
||||||
|
- `consumer.rs:636-642` — `BindDataPlane`'s `poll_flush`/
|
||||||
|
`poll_shutdown` passthroughs (the review-001 L-2 class, one layer
|
||||||
|
up): the BIND door's half-close rides these. One shutdown test.
|
||||||
|
- `consumer.rs:843` — `read_reply2`'s version-mismatch arm (a
|
||||||
|
producer sending a non-5 version reply#2): unreachable from the
|
||||||
|
in-tree producer (always 0x05) but reachable from a foreign one —
|
||||||
|
one unit test on `read_reply2` directly.
|
||||||
|
- `producer.rs:374-386, 405-414` — the datagram stage's
|
||||||
|
`ReplyFrameRejected`/`EgressEnded`/`Io` exits (an egress that
|
||||||
|
violates the joint bound, one whose `send` fails, one whose
|
||||||
|
writes fail): all three exits have no test anywhere (the
|
||||||
|
harness's egress never fails post-reply). The stage's failure
|
||||||
|
matrix is the ASSOCIATE teardown surface — at least
|
||||||
|
`EgressEnded` (the `send`-failure arm) deserves the
|
||||||
|
drop-witness test `egress_factory_called_once_and_dropped_with
|
||||||
|
_the_stage` gives the happy path.
|
||||||
|
- `producer.rs:560` — the quiet wait's `Err(_)` (I/O error, not
|
||||||
|
EOF) arm; `producer.rs:568` — the BIND reply#2 Domain-peer
|
||||||
|
fallback (`TargetAddr::Domain(_, port) → 0.0.0.0:port`) — one
|
||||||
|
factory returning a Domain peer pins it.
|
||||||
|
- `egress.rs:72-74, 226-230, 275-279` — the three `Debug` impls'
|
||||||
|
bodies (render once each); `error.rs:113-115` —
|
||||||
|
`SocksEstablishmentReason`'s `Display`. Cosmetic; batch freely.
|
||||||
|
- `rfc1928/client.rs:217-219` — `use_password_auth`'s
|
||||||
|
misuse arm (`Some(None)`-shaped methods vec): review-001-era
|
||||||
|
accepted-uncovered, still uncovered. Fine to leave; it has a
|
||||||
|
translated-error note.
|
||||||
|
|
||||||
|
The headline gap is the producer stage's three failure exits and
|
||||||
|
the two untested `split`s — everything else is single-arm
|
||||||
|
cosmetics.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## N-9 [observation] — `is_mid_frame` is public but consumed nowhere outside the codec's own tests (the teardown diagnostic lives in `recv_datagram`'s internal check)
|
||||||
|
|
||||||
|
**Files**: `src/wire.rs:253-260` (`is_mid_frame` — pub), consumers:
|
||||||
|
`src/consumer.rs:406` (internal use only), `tests/wire.rs` unit
|
||||||
|
tests
|
||||||
|
|
||||||
|
**Problem**: the doc sells it as "the teardown diagnostic" for
|
||||||
|
callers observing a truncated stream; the actual truncation
|
||||||
|
diagnosis is inlined in `recv_datagram` (consumer.rs:402-411). A
|
||||||
|
front-door bridge (batch 2) driving its own reader loop *would* use
|
||||||
|
it. Keep — one line of "anticipated consumer: the doors" posture on
|
||||||
|
the method, or leave as-is. Zero risk either way; filed only
|
||||||
|
because D-1-style dead-surface sweeps should not flag it later
|
||||||
|
without this note.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
# Part B — Non-findings (verified correct, recorded to bound the re-review)
|
||||||
|
|
||||||
|
- **The seam contracts hold under adversarial reading.** Re-derived
|
||||||
|
independently of review 002: the readiness contract's R-1
|
||||||
|
yield-guard is correct and necessary (the zero-yield unit test
|
||||||
|
reproduces the hazard without it); ownership-rides-the-handle has
|
||||||
|
no `_` bindings anywhere in non-test code (swept); the accept
|
||||||
|
future's drop-as-teardown is real for both exits (drop-flag
|
||||||
|
tests); framing-once holds (the egress sees no prefix —
|
||||||
|
`frame_inner` is the only prefixer on the reply path and
|
||||||
|
`frame` on the forward path).
|
||||||
|
- **The teardown matrix's real semantics are pinned** — the three
|
||||||
|
teardown rows (`core-integration.rs:659/717/770`) drive
|
||||||
|
half-close/EOF/client-gone mid-quiet-wait and then assert fresh
|
||||||
|
sessions of *all three* commands succeed (the unpoisoned
|
||||||
|
registration), with `no_leaked_channels` probes on both sides.
|
||||||
|
- **Gate composition is correct end to end.** The open gate
|
||||||
|
(`required_scopes` at the registry) runs before the establisher;
|
||||||
|
the command gate resolves `socks5:bind` from the per-call opener
|
||||||
|
identity at dispatch and precedes the accept seam (no phantom
|
||||||
|
listener — pinned). Scope-less fails closed at both gates; the
|
||||||
|
identity matrix pins all three rows over channels.
|
||||||
|
- **No panics in non-test code** (re-swept across all of `src/`,
|
||||||
|
not just the fork): every `unwrap`/`expect`/`panic!` hit is in a
|
||||||
|
`#[cfg(test)]` region. The convention-2 posture held through
|
||||||
|
three consumer sessions + the producer's in-stream refusal
|
||||||
|
paths.
|
||||||
|
- **Wire-driven allocation is bounded everywhere in the new code**:
|
||||||
|
the datagram stage's read chunk is fixed 16 KiB; the frame codec
|
||||||
|
bounds at frame time (the joint bound — tested at all three
|
||||||
|
per-form maxima + the 65507/domain corner); `read_address`'s
|
||||||
|
domain arm stays 255-bounded (S-1's guard, pinned);
|
||||||
|
`negotiate_auth`'s methods read is 1-byte-bounded; `new_reply` is
|
||||||
|
a fixed 10/22-byte form. No unbounded wire-driven allocation
|
||||||
|
exists.
|
||||||
|
- **The `select!` shapes are correct**: the datagram stage is
|
||||||
|
un-biased (random poll order — the POC's livelock concern applied
|
||||||
|
to the *branch's own readiness*, guarded by the R-1 yield); the
|
||||||
|
quiet wait is single-shot with exactly two exits, and its
|
||||||
|
one-byte read cannot loop.
|
||||||
|
- **`pump_bidi` is awaited inline everywhere** (CONNECT, BIND
|
||||||
|
post-reply#2); the handler's `JoinHandle` is returned to alkcall
|
||||||
|
(stored as `handler_task` in `ChannelState`), so the
|
||||||
|
data-plane-lifetime rule (R-02) holds — verified against
|
||||||
|
alkcall's `manager.rs` storage, not just the comments.
|
||||||
|
- **The in-band BIND variant-3 default is sound**: the instant
|
||||||
|
accept resolves reply#1→reply#2→`pump_bidi` against
|
||||||
|
`empty()`/`sink()` halves; EOF arrives immediately from the empty
|
||||||
|
half (the variant-3 test pins the full sequence including EOF).
|
||||||
|
The quiet-wait garbage test pins the other exit on the same
|
||||||
|
shape.
|
||||||
|
- **Consumer-side EOF semantics are consistent**: `recv_datagram`
|
||||||
|
returns `Ok(None)` on clean EOF (association end), fails loud on
|
||||||
|
EOF-mid-frame (`UnexpectedEof`), and `Ok(None)` on a *spent*
|
||||||
|
stream (the EOF-shape a torn-down association should present);
|
||||||
|
F-2 (empty datagram ≠ EOF) holds at every layer.
|
||||||
|
- **`egress_factory`'s sync-lift is correct** (the result captured
|
||||||
|
before the future — an `Err` lifts verbatim; tested both faces).
|
||||||
|
- **MSRV, wasm, publish**: `cargo publish --dry-run` completes;
|
||||||
|
both wasm checks clean; `rust-version = "1.88"` unchanged from
|
||||||
|
review 001's verified state (no MSRV-relevant surface added —
|
||||||
|
the new code is plain 2021-edition async/await).
|
||||||
|
- **License/provenance posture unchanged**: `src/rfc1928/LICENSE`
|
||||||
|
and the module docs still carry the full fork-point statement
|
||||||
|
(convention 17's "extend, never replace" — nothing touched it).
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
# Part C — What's Good
|
||||||
|
|
||||||
|
- **The seam architecture is doing its job.** The entire producer
|
||||||
|
test suite runs substrate-free over duplexes (the
|
||||||
|
differential-suite pattern generalized), while the integration
|
||||||
|
suite runs the same seams over real channels — and both pass
|
||||||
|
against the *same* handler code. That is the convention-7
|
||||||
|
inversion point paying off, and it is why the coverage is 94%+
|
||||||
|
without a kernel socket anywhere in the tests.
|
||||||
|
- **The teardown discipline is real, not just documented.** Every
|
||||||
|
session type carries the `Option`-inner + `Drop`-reap +
|
||||||
|
`close`-graceful triple, the compile-asserted no-`Clone`, the
|
||||||
|
spent-state guards, and the phantom-channel property is tested
|
||||||
|
on both sides (refused opens leave no entry; refused in-stream
|
||||||
|
targets reap before the error returns). The R-2 shape
|
||||||
|
(`SpentSessionError`) is carried consistently — except C-1, which
|
||||||
|
is precisely the one path it missed.
|
||||||
|
- **The error vocabularies are faithful and table-tested at every
|
||||||
|
layer**: dial/egress/accept errors → reply codes (table-tested),
|
||||||
|
reply codes → variants and back (both directions pinned, the
|
||||||
|
unassigned-code arm included), `WireError` → the RFC vocabulary
|
||||||
|
(one mapping, tested), open errors → the ADR-049 reason
|
||||||
|
vocabulary (parsed, one-way, with the unknown-code-does-not-alias
|
||||||
|
test). Nothing collapses into a generic error.
|
||||||
|
- **The datagram codec is tight**: the joint bound is enforced at
|
||||||
|
frame time with the per-form maxima pinned, the incremental
|
||||||
|
reader survives arbitrary chunk boundaries (byte-by-byte tested),
|
||||||
|
empty payloads round-trip as real datagrams end to end, and
|
||||||
|
garbage fails loud in both directions. F-2 is pinned at three
|
||||||
|
layers (codec, producer stage, consumer session).
|
||||||
|
- **The test suite's *semantics* are the right ones**: the
|
||||||
|
identity matrix, the teardown matrix with fresh-session
|
||||||
|
post-conditions, the concurrency probes, the witness assertions,
|
||||||
|
and the raw probes for the sentinels all test behavior over
|
||||||
|
channels, not implementation details. The 2.2k-line producer.rs
|
||||||
|
is 69% tests — the file-size smell is a testing artifact, not
|
||||||
|
bloat (the real smells are the targeted ones: N-5, N-6).
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
# Suggested Disposition
|
||||||
|
|
||||||
|
| Batch | Items | Shape |
|
||||||
|
|---|---|---|
|
||||||
|
| 1 — correctness | C-1, C-3 (+ the D-2 test) | small code + tests; one session |
|
||||||
|
| 2 — hygiene | D-1, D-3, N-1 (cut list), N-3, N-4 | posture + small deletions/rewordings |
|
||||||
|
| 3 — coverage | D-2, N-8 (the split/stage-exit/read_reply2 items) | tests only; folds into batch 1's session |
|
||||||
|
| fold-in | N-2 (wire.md line), N-5, N-6 (when next touching those files), N-7/N-9 (notes, no action) | opportunistic |
|
||||||
|
|
||||||
|
C-2 is its own ADR-sized decision (the idle-timeout surface) and
|
||||||
|
should ride the batch-2 planning, not this review's fix round — it
|
||||||
|
is a *decision to make before the doors exist*, not a defect in the
|
||||||
|
core.
|
||||||
|
|
||||||
|
## References
|
||||||
|
|
||||||
|
- `docs/reviews/001-rfc1928-full-review.md` (the fork review — the
|
||||||
|
severity prefixes, the coverage discipline, and the
|
||||||
|
keep-or-cut triggers this review closes out)
|
||||||
|
- `tasks/implementation/review-protocol-core.md` (the fidelity
|
||||||
|
review; its R-1/R-2 shapes are the posture C-1's fix follows)
|
||||||
|
- ADR-003/004/006/007/008/009 (the seams, gates, phase model,
|
||||||
|
providers, features); alkcall ADR-049/050 (establishment,
|
||||||
|
`pump_bidi`); ADR-016 (error schemas — D-3)
|
||||||
|
- alkcall 0.8.0 (`manager.rs` `teardown_channel`,
|
||||||
|
`pump.rs` `pump_bidi`, `spec.rs` `ErrorDefinition`);
|
||||||
|
alktty `src/channels.rs:198` (the declared-`ErrorDefinition`
|
||||||
|
precedent); alktunnels `src/params.rs` (the empty-vec sibling)
|
||||||
|
- The POC findings (the stage's measured shapes — C-2's
|
||||||
|
"sufficient for relay postures" lineage)
|
||||||
Reference in new issue
Block a user