alksocks/tasks/implementation/review-protocol-core.md

---
id: review-protocol-core
name: Review (the protocol core before any assembly-layer work)
status: completed
depends_on: [implementation/core-integration-suite]
scope: narrow
risk: low
impact: project
level: review
---

## Description

Review checkpoint before batch 2 (the assembly layer: `local`
providers, front doors, `composed` providers). Everything in batch 2
consumes the seams and the session types, so a core defect (a seam
contract drift, a teardown hole, a wasm regression) would propagate
into every downstream task. This review is the gate between the core
group and the assembly layer, and the point where batch 2 gets
decomposed (its task shapes may adjust to what the core actually
landed).

Review against the architecture, in this order:

1. **Seam contracts** (ADR-004, producer.md §The Three Seams): the
   readiness contract (`next_reply` `Pending` when nothing is ready,
   with the contract test in place pinning it), ownership-rides-the-handle
   (no `_` bindings of owning values; handles own their payloads
   structurally), the accept future owning its resources end-to-end
   (the quiet-wait drop IS the client-gone teardown), framing-once
   (the egress speaks inner bytes; the wire layer frames once).
2. **Wire fidelity** (wire.md, ADR-003/ADR-007): the datagram frame
   (joint bound, F-2 at payload level), the phase model (no mode
   byte; post-reply garbage refused structurally in all three
   commands), the sentinel surfaces (ASSOCIATE's `127.0.0.1:0`,
   BIND's variant-3 zeros), association lifetime = channel lifetime.
3. **Access control** (ADR-006): the open gate (`socks5:open` at the
   registry), the command gate (`socks5:bind` at BIND dispatch),
   two grants never implied; refusals RFC-faithful everywhere
   (convention 2, never collapsed into generic errors).
4. **Teardown/lifecycle** (wire.md §Sentinels, alkcall ADR-050):
   `pump_bidi` awaited inline (R-02, no spawn-and-forget); the
   teardown matrix tests actually pin the semantics; no
   teardown-at-birth paths (the third-witness pattern held).
5. **Convention compliance** (AGENTS.md): no `unwrap`/`expect`/
    panics outside tests; `thiserror` surfaces; no comments in code
    except doc comments + pinned non-obvious-correctness notes;
    naming; module structure (one module per file, `lib.rs`
    re-exports); producer/consumer vocabulary (no server/client
    framing outside RFC vocabulary).
6. **Wasm-clean invariant** (ADR-009): both wasm checks green; no
    `net`-shaped feature; tokio on the wasm-clean subset; the harness
    and tests stay out of the wasm build.
7. **POC-fidelity spot-check**: for two or three of the trickiest
    behaviors (the readiness contract, the quiet-wait two-exit
    semantics, the split-datagram reader persistence), diff the
    crate's behavior against the POC crates' findings, since the POCs are
    the empirical record; drift is a finding.
8. **Docs sync**: the specs match what landed (the integration
    task's reconciliation verified; any residue is a finding).

## Notes

> Filled by the review agent: see the Summary and the Findings.

## Summary

Review performed 2026-09-24 against the eight-criteria order; the
gate opens. Two findings were corrected in the review change itself
(R-1 readiness-contract hardening; R-2 a non-test `expect`), plus
three minor residues (R-3..R-5, fixed in the same change); no
seam-contract, wire-fidelity, ACL, teardown, wasm, or POC-fidelity
drift was found. One prior-session reconciliation (the bind-only
identity row) was independently verified as correct (see the Notes).

**1. Seam contracts (ADR-004, producer.md §The Three Seams): clean,
one finding corrected (R-1).** The readiness contract is documented
on the trait (`egress.rs:174-185`), table-tested, and pinned
behaviorally twice (the producer unit test's mutex-guarded hot
egress + the integration suite's hot-`Ok(None)` under concurrent
load). Ownership rides the handle structurally (`TargetHandle`/
`BindTargetHandle` payloads own their halves; no `_` bindings of
owning values anywhere in the crate, swept clean); the accept future owns
its resources end-to-end (`BindAccept::new` boxes it; the quiet
wait's `select!`-pinned drop is the client-gone teardown, pinned by
the drop-flag tests in `src/producer.rs` and `tests/core-
integration.rs`); framing-once holds (`Egress::send` takes payload
only, `next_reply` returns inner bytes, `frame_inner`/`frame` are
the only prefixers; `frame == frame_inner` byte-identity is
table-tested). R-1 is the readiness contract's own tolerance: see
the Findings.

**2. Wire fidelity: clean.** The datagram frame matches wire.md §The
Data Plane byte-for-byte: `[len: u16 BE][addr + data]`, the joint
bound (`frame`/`frame_inner` reject > 65535 with the exact per-form
maxima pinned: 65528 v4 / 65516 v6 / 65276 domain), F-2 at payload
level (empty datagram round-trips end to end, never EOF; `len == 0`
inner is `EmptyFrame` and unreachable from a compliant peer), RSV/FRAG
absent from the in-band codec. The phase model holds: no mode byte;
post-reply garbage is refused structurally in all three commands
(CONNECT = pure pipe, ASSOCIATE = garbage ATYP fails loud, BIND =
the quiet wait's one-byte read ends the session with no reply#2),
each pinned by test. Sentinels verified: ASSOCIATE's `127.0.0.1:0`
(asserted in the integration suite + the raw probes), BIND's
variant-3 zeros (`0.0.0.0:0` reply#1 + reply#2, pinned);
association lifetime = channel lifetime (EOF rows in the teardown
matrix). The BAST document's joint-constraint note matches the
codec.

**3. Access control (ADR-006): clean.** The open gate is
`required_scopes = ["socks5:open"]` on the single op spec
(`socks5_open_spec`); the command gate resolves `socks5:bind` from
the per-call opener identity at BIND dispatch and precedes the
accept seam (a scope-less BIND never mints a listener, which is pinned).
Both refusals are RFC-faithful (0x02 at the command gate; the
registry's `FORBIDDEN` at the open gate); the mapping tables
(dial/egress/accept/factory errors → reply codes) are table-tested
and route through the forked `ReplyError`, with nothing collapsed into
generic errors. The integration suite's identity matrix pins all
three rows end to end with the CF-006 witness asserted.

**4. Teardown/lifecycle: clean.** `pump_bidi` is awaited inline in
all three data planes (CONNECT, BIND post-reply#2) inside the
handler task; the `JoinHandle` tracks the data-plane lifetime (R-02
held, so no spawn-and-forget). The teardown matrix tests pin the real
semantics (half-close / EOF / client-gone-with-parked-accept, each
followed by fresh sessions of every command). No teardown-at-birth
paths: the egress-drop witness test flips the flag on stage exit,
the accept-drop witness flips on the client-gone branch; the
consumer sessions reap on `close`/`shutdown`/`Drop` with the leak
probes asserting no phantom channels on either side.

**5. Convention compliance: one finding corrected (R-2).** No
`unwrap`/`expect`/`panic!`/`unreachable!` in non-test library code
after R-2 (swept: `src/` non-test regions are clean; the forked
core's panic translations were re-verified). `thiserror` surfaces
throughout; naming/module structure (one module per file, `lib.rs`
re-exports) hold; producer/consumer vocabulary holds (RFC-facing
"server"/"client" only inside `src/rfc1928/`, the RFC vocabulary's
home). Cargo.toml's two inline policy comments are the
extraction-review F-3-accepted posture (carried, no fix demanded).

**6. Wasm-clean invariant: green, with one accepted note (R-5).**
Both wasm checks clean (`cargo check`/`clippy --target
wasm32-unknown-unknown -- -D warnings`, from scratch); no
`net`-shaped feature (the two declared features are empty markers);
tokio on the wasm-clean subset with `default-features = false` (no
`full` in `[dependencies]`); the harness/tests ride dev-deps only.
R-5: see the Findings (a transitive `getrandom` carrier outside
this crate's control).

**7. POC-fidelity spot-check: clean, one finding corrected (R-1).**
- *Readiness contract*: the crate's stage matches the POC's
  `select!`-driven shape (channel reads ∥ reply drains, `Ok(None)`
  tolerated as a spurious wakeup); the POC's LocalEgress/
  ComposedEgress both park properly (`readable()` / queue
  `recv().await`) and the ComposedEgress's closed-queue arm returns
  `Err` rather than a hot `None`, which is exactly the posture the crate
  inherited. The drift the spot-check surfaced is the *zero-yield*
  bound of the contract (R-1): a pure `async { Ok(None) }` egress
  livelocked the landed stage on a current-thread runtime
  (empirically reproduced; the POC's findings named the biased-select
  and hot-None hazards; the no-yield variant is the same class one
  step further). Fixed in the review change.
- *Quiet-wait two-exit semantics*: the landed `select!` is
  single-shot with exactly the two exits (accept / EOF-garbage-error),
  not a loop, which matches the POC's `never_loop` finding; the parked-
  accept probes make both exits deterministic; reply#1/reply#2 carry
  the factory's `bind_addr`/peer byte-exactly on the provider
  variant and the sentinels on variant 3.
- *Split-datagram reader persistence*: `AssociateSession::recv_
  datagram`'s reader + pending queue persist across calls (pinned by
  the 2-byte-prefix chunk test and the consumer-associate suite),
  matching ADR-003's re-read of the POC finding.

**8. Docs sync: clean, with three residues fixed in the review change.**
The integration task's reconciliation was verified against ADR-006
§Consequences, producer.md §Scope Gating, and the landed spec/tests
(see the Notes). Residues found and fixed: `src/egress.rs`'s module
doc still said the readiness tests "land with the producer-associate
task" (that task is landed; reworded to point at where they live);
`src/producer.rs`'s establisher doc said "in this task's scope"
(same class, reworded); producer.md §Wiring's sketch showed
`producer.register(&mut channel_core)`, while the landed signature takes
`(&ChannelCore, &OperationRegistry, AuthContext, Option<IdentityWitness>)`
and also carries the establisher (corrected). The rest of the specs
(wire.md, consumer.md §Wiring, ADR-006, rfc1928.md, README Current
State, lib.rs crate docs) match the tree.

## Acceptance Criteria

- [x] Seam contracts hold (readiness / ownership / accept-future /
      framing-once), with R-1 corrected
- [x] Wire fidelity: the frame, phase model, sentinels, lifetimes
      byte-checked against wire.md/bast.md
- [x] Access control: both gates + never-implied grants pinned, the
      prior session's reconciliation verified
- [x] Teardown/lifecycle: pump_bidi inline, the matrix pins the
      semantics, no teardown-at-birth paths
- [x] Convention compliance: no unwrap/expect/panics outside tests
      after R-2; thiserror; naming; module structure; vocabulary
- [x] Both wasm checks green (from clean), no net-shaped feature,
      tokio on the wasm-clean subset
- [x] POC-fidelity spot-check of the three trickiest behaviors:
      the zero-yield drift (R-1) found and fixed
- [x] Docs sync: specs match the tree; residues fixed in the same
      change
- [x] Findings recorded; the gate to batch 2 opens

### Findings

- **R-1 (major, fixed in the review change)**: the datagram stage's
  `Ok(None)` tolerance (re-park) is safe against a hot egress only
  while its futures yield at least one poll boundary. A *zero-yield*
  egress (`next_reply` = a pure `async { Ok(None) }`, no await at
  all) wins every `select!` poll forever: the channel branch and
  every other task on a current-thread scheduler starve (empirically
  reproduced: the echo never arrives; the multi-thread runtime
  survives, so the landed mutex-guarded unit test masked the hazard
  at its lock's yield boundary). The POC's contract wording ("must
  be `Pending` when nothing is ready") is right; the landed stage's
  handling had one gap. Fix: the stage's `Ok(None)` handler now
  `yield_now()`s before re-parking (`src/producer.rs`
  `run_datagram_stage`), plus a pinning unit test with a pure
  zero-yield egress (the strictest violator, which hangs without the
  yield) alongside the existing hot-None test. ADR-003's contract
  prose and `egress.rs`'s trait doc stay as written (the contract
  is on the egress; the stage now additionally defends itself).
- **R-2 (convention, fixed in the review change)**: `Socks5Channel
  Session::into_inner` / `AssociateSession::into_inner` ended in
  `inner.expect("into_inner on a spent session")`, the only
  non-test `expect` in `src/`. Reachability is narrow (a spent
  session's inner is gone only after close/shutdown/Drop, and no
  public API constructs a spent session), but convention 2's rule is
  the rule: the shape is now `Result<TakenInner, SpentSessionError>`
  / `Result<TakenAssociateInner, SpentSessionError>` with a
  zero-carrier error type (`SpentSessionError`; a spent session's
  manager entry is always already reaped, so dropping it loses
  nothing; the large-Err/unit-Err lints shaped the choice), pinned
  by two spent-session unit tests. Test call sites updated
  (`.expect("the session is live")`, which is test-side and allowed).
- **R-3 (doc residue, fixed)**: `src/egress.rs` module docs + the
  `src/producer.rs` establisher doc carried task-time phrasings
  ("lands with the producer-associate task" / "this task's scope")
  that are stale now the tasks are landed. Reworded to point at the
  landed reality.
- **R-4 (doc residue, fixed)**: producer.md §Wiring's assembly sketch
  showed a one-argument `producer.register(...)`; the landed
  signature takes the core/registry/auth/witness quadruple. Reconciled
  (the same sync discipline the integration task applied to
  consumer.md).
- **R-5 (doc residue, fixed)**: the
  `bind_quiet_wait_is_not_under_the_establishment_bound` test doc
  claimed the substrate-free probe exercises "the alkcall 10s
  default bound", which it cannot (the substrate-free shape has no
  alkcall bound; the over-channels wait-outlives-establishment
  behavior is the harness's pin by construction). Reworded to say
  what the test actually pins (the wait is teardown-governed, no
  in-stream timeout).
- **R-6 (accepted, no fix demanded)**: the wasm-clean invariant rides
  a transitive feature unification outside this crate: `alkcall →
  jsonschema/referencing → ahash (runtime-rng) → getrandom 0.3.4`,
  and `wasm_js` (required to compile `getrandom` on
  `wasm32-unknown-unknown`) is enabled only because jsonschema/
  referencing declare it for the wasm32 target. The default-crate
  check passes today (verified from a clean target dir), and the
  alkty sibling shows the identical posture; but a future
  alkcall/jsonschema feature change could silently break the wasm
  gate with no in-tree cause. Not actionable in this crate (the dep
  graph is alkcall's); noted for the wasm-adapter story's
  eventual OQ-SK2-06 work and for the batch-2 wasm checks.

## References

- docs/architecture/wire.md, producer.md, consumer.md, rfc1928.md,
  bast.md, README.md (the specs under review)
- docs/architecture/decisions/001/003/004/006/007/009/013
- docs/research/poc-udp-associate-findings.md,
  poc-bind-findings.md, poc-connect-wasm-findings.md (the POC
  fidelity sources)
- /workspace/alksocks-udp-poc (the POC reference for the readiness
  contract's empirical record)
- AGENTS.md (conventions 1–17; the verification commands)

## Notes

- The prior integration session's Notes/Summary were read and
  cross-checked against the specs and the tree. The one
  reconciliation it flagged (the task file's identity-matrix row 2
  expecting a bind-only identity to serve BIND, vs ADR-006's landed
  `required_scopes = ["socks5:open"]` unconditionally) is **correct
  as landed**: the op spec (the single op, all three commands, ADR-
  001) carries the open gate unconditionally; a bind-only identity
  therefore fails closed for every command (FORBIDDEN at the
  registry, the establisher never runs, and no phantom channel is pinned
  producer- and consumer-side in
  `identity_matrix_bind_only_fails_closed_at_the_open_gate`), and a
  BIND-capable deployment grants both scopes explicitly. The suite
  pins this; ADR-006 §Consequences states the asymmetry explicitly
  ("never implied, in either direction"); the task file's Notes
  record the reasoning. No residue: the drift was a Phase-0-era task
  phrasing, not a spec-vs-code contradiction. The user's flag was
  well-placed; this review treats the reconciliation as verified
  rather than open.
- The self-reported verification claims reproduce (the landed suite
  was green at review start; this review's fixes re-ran the full
  sweep (see below).
- Post-review verification (the review change included): `cargo
  test` 348 green (121 lib + 38 connect + 39 associate + 40 bind +
  40 integration + 30 differential + 37 rfc1928 + 3 doctests, being the
  two R-1/R-2 additions and the four R-2 call-site updates
  included), `cargo test --all-features` green, `cargo clippy
  --all-targets -- -D warnings` clean, `cargo fmt --check` clean,
  both wasm checks clean (re-run from a clean wasm target dir),
  `cargo doc --no-deps` clean. The batch boundary holds: no
  `local`/`composed` code exists.