tasks: decompose the review-002 fix round — eight dependency-ordered tasks
The general review (docs/reviews/002-protocol-core-implementation-review.md) decomposed into the fix-round group, grouped so no session carries both a code-fix vertical and the test sweep: - fix-bind-reply2-reentry: C-1 (the BIND reply#2 re-entry guard) + D-1 (the reader decision) + N-6 (the constructor prologue helper) + S-2 (the reap doc line) — one consumer session - fix-producer-gate-policy: C-3 (the explicit BIND gate input) + D-2 (the register/builder coverage) + S-1/N-2 (the stage doc lines) — one producer session - fix-rfc1928-posture: N-1 (the keep-or-cut pass) + N-4 (the honest refusal vocabulary) — one fork posture session - fix-hygiene-residue: D-3 (the ErrorDefinition declaration) + N-3 (the unreachable let-else) + N-9 (the is_mid_frame note) - fix-coverage-residue: N-8's attributed line list — tests only, after the code fixes (final shapes) - fix-test-helper-consolidation: N-5 — after the coverage round - architecture/adr-014-idle-timeout-posture: C-2 — the ADR-sized decision, independent, before the doors exist - review-fix-round-002: the closing checkpoint; the batch-2 gate The review doc's disposition table now names the landed tasks. N-7 is recorded-only (the review's own recommendation).
This commit is contained in:
glm-5.3-flash committed
1 parent
d612d7f7f2
commit
6d14d35a54
9 files changed
+828
-10
No files matched your search
@@ -838,17 +838,25 @@ without this note.
|
||||
|
||||
# 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 |
|
||||
Decomposed 2026-09-24 into the fix-round task group
|
||||
(`tasks/implementation/fix-*.md` + the ADR + the closing review) —
|
||||
the grouping follows the review's batches, adjusted so no session
|
||||
carries both a code-fix vertical and the test sweep:
|
||||
|
||||
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.
|
||||
| Task | Items | Shape |
|
||||
|---|---|---|
|
||||
| `implementation/fix-bind-reply2-reentry` | C-1, D-1, N-6, S-2 | the consumer correctness fix + the same-file residues |
|
||||
| `implementation/fix-producer-gate-policy` | C-3, D-2, S-1, N-2 | the gate-policy fix + the register test + the doc lines |
|
||||
| `implementation/fix-rfc1928-posture` | N-1, N-4 | the keep-or-cut pass + the refusal vocabulary |
|
||||
| `implementation/fix-hygiene-residue` | D-3, N-3, N-9 | the error-schema declaration + one-line postures |
|
||||
| `implementation/fix-coverage-residue` | N-8 (D-2's test rides the producer task) | tests only; after the code fixes |
|
||||
| `implementation/fix-test-helper-consolidation` | N-5 | after the coverage round (final population) |
|
||||
| `architecture/adr-014-idle-timeout-posture` | C-2 | the ADR-sized decision; independent, before the doors |
|
||||
| `implementation/review-fix-round-002` | — | the closing checkpoint; the batch-2 gate |
|
||||
|
||||
N-7 is recorded-only (no action — the review's own
|
||||
recommendation); its bound-re-litigation note lives in the review
|
||||
itself.
|
||||
|
||||
## References
|
||||
|
||||
|
||||
@@ -0,0 +1,106 @@
|
||||
---
|
||||
id: architecture/adr-014-idle-timeout-posture
|
||||
name: "ADR-014 — the idle-timeout posture for the pre-command handshake and the association (review-002 C-2)"
|
||||
status: pending
|
||||
depends_on: []
|
||||
scope: narrow
|
||||
risk: medium
|
||||
impact: project
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Review 002's 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). The alkcall 10s establishment bound governs the
|
||||
*open-op call*, not the in-stream conversation
|
||||
(`src/producer.rs:183-191` parks on `accept_bi` → `accept_no_auth`
|
||||
→ `read_command` with no timeout; the datagram stage's loop exits
|
||||
only on EOF/error/data). BIND's quiet wait got an explicit decision
|
||||
(ADR-007 — teardown-governed, "FTP-length waits stay legal"); the
|
||||
handshake and the association lifetime got "inherited from the
|
||||
channel" — implicit, never decided.
|
||||
|
||||
The mitigations are real (the 256-channel cap bounds the count,
|
||||
memory per idle channel is bounded, teardown fires on channel
|
||||
close — 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 batch 2's
|
||||
providers (`local`, `composed`) are about to route *vanilla
|
||||
clients* through these same phases via the front doors, where the
|
||||
peer is a kernel socket often outside alkcall's channel-cap regime
|
||||
entirely — slowloris-shaped holds become the assembly layer's
|
||||
problem, and the protocol layer is where a timeout seam would live.
|
||||
|
||||
**The deliverable is a decision, not code** — ADR-014
|
||||
(`docs/architecture/decisions/014-*.md`), landing *before* the
|
||||
`local` doors exist. Decide between the review's two options:
|
||||
|
||||
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). More work;
|
||||
touches no wire format.
|
||||
|
||||
The ADR must state, whichever option wins: the BIND quiet wait's
|
||||
existing decision (ADR-007) and how the new decision relates to it
|
||||
(same regime or different — and why the difference is principled);
|
||||
the DoS posture in the doors' terms (what a `local` door's accept
|
||||
path must do if the protocol layer does not); and whether the
|
||||
decision is revisitable post-batch-2 or is itself a one-way door
|
||||
(it should not be — a timeout seam is additive — but say so).
|
||||
|
||||
Also update `docs/architecture/open-questions.md` if C-2 was
|
||||
tracked there, and the affected spec lines (wire.md §UDP
|
||||
ASSOCIATE's association-lifetime bullet; producer.md's pump
|
||||
description) to carry the decided posture rather than the implicit
|
||||
one.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] ADR-014 exists at
|
||||
`docs/architecture/decisions/014-<slug>.md`, status accepted,
|
||||
deciding explicitly between the two options (or a reasoned
|
||||
third)
|
||||
- [ ] The BIND quiet wait's ADR-007 relationship is stated (same
|
||||
regime or a principled difference)
|
||||
- [ ] The doors' DoS posture is recorded (what the assembly layer
|
||||
must do if the protocol layer does not)
|
||||
- [ ] The affected spec lines (wire.md, producer.md) carry the
|
||||
decided posture — no implicit residue
|
||||
- [ ] The decision lands before any `local`/`composed` door work
|
||||
(the batch-2 tasks may depend on this one)
|
||||
- [ ] `cargo doc --no-deps` clean if any doc-comment carries the
|
||||
posture; the full verification sweep green
|
||||
|
||||
## References
|
||||
|
||||
- docs/reviews/002-protocol-core-implementation-review.md §C-2 (the
|
||||
finding — the task's source of truth)
|
||||
- docs/architecture/decisions/007-bind-in-base.md (the quiet wait's
|
||||
existing decision — the contrast case)
|
||||
- docs/architecture/decisions/008-provider-defaults.md,
|
||||
009-feature-gates.md (the batch-2 provider posture the decision
|
||||
precedes)
|
||||
- docs/architecture/wire.md §UDP ASSOCIATE (association lifetime =
|
||||
channel lifetime — the contract the decision qualifies)
|
||||
- The POC findings (the stage's measured shapes — the
|
||||
"sufficient for relay postures" lineage)
|
||||
|
||||
## Notes
|
||||
|
||||
> Agent fills this during implementation.
|
||||
|
||||
## Summary
|
||||
|
||||
> Agent fills this on completion.
|
||||
@@ -0,0 +1,111 @@
|
||||
---
|
||||
id: implementation/fix-bind-reply2-reentry
|
||||
name: "Fix — BIND reply#2 re-entry guard + consumer session hygiene (review-002 C-1, D-1, N-6, S-2)"
|
||||
status: pending
|
||||
depends_on: []
|
||||
scope: narrow
|
||||
risk: medium
|
||||
impact: component
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Close review 002's one reachable silent-corruption path plus the
|
||||
consumer-side residues that ride the same file
|
||||
(`docs/reviews/002-protocol-core-implementation-review.md`):
|
||||
|
||||
1. **C-1 (major) — the reply#2 phase is re-entrant with no
|
||||
spent-marker.** `BindSession::await_inbound`
|
||||
(`src/consumer.rs:757-763`) takes `&mut self`, leaves `inner`
|
||||
populated, and returns a `BindDataPlane<'_>` borrowing the same
|
||||
stream — nothing marks the phase consumed. A second call runs
|
||||
`read_reply2` against the *live data plane* (post-reply#2 bytes
|
||||
are the accepted peer's raw application data): a baffling
|
||||
`UnsupportedSocksVersion` at best, silent 10+-byte data
|
||||
corruption at worst (any peer protocol whose first byte is 0x05).
|
||||
The same shape replays through `reply1()` twice →
|
||||
`BindReply1::await_inbound`. The sibling sessions are all
|
||||
spent-state-guarded; this is the gap.
|
||||
|
||||
Fix per the review's sketch: make the reply#2 phase consumed in
|
||||
the session's established posture — `inner.take()` (a second call
|
||||
hits the `None` arm and returns the existing `Ok(None)` spent
|
||||
shape, which the method's doc *already* claims: "`None` after a
|
||||
taken plane") or an equivalent spent-marker riding the session.
|
||||
Note the take-shape's ownership consequence: `BindDataPlane`
|
||||
comes to own the stream (the `TakenInner` pairing posture
|
||||
`Socks5ChannelSession::into_inner` already pins) while the
|
||||
session stays the teardown owner for the channel entry. Either
|
||||
mechanism is acceptable; the spent shape must match the session
|
||||
family's posture (`Ok(None)` or the typed `SpentSessionError` —
|
||||
pick one, document it).
|
||||
|
||||
2. **D-1 (minor) — `BindReply1::await_inbound` is dead surface**
|
||||
(0% covered; every test drives the session's method). The choice
|
||||
interacts with C-1 — pick one normative reply#2 reader:
|
||||
- (a) the session's `await_inbound` (the review's lean: document
|
||||
`reply1()` as a view, demote `BindReply1::await_inbound` to a
|
||||
private helper or cut it), or
|
||||
- (b) `BindReply1::await_inbound` (the task spec's intent —
|
||||
`tasks/implementation/core-consumer-bind.md:74-75`; the
|
||||
session's method delegates to it and the handle is tested
|
||||
through).
|
||||
The alternate path must be impossible to double-read (private,
|
||||
cut, or spent-guarded).
|
||||
|
||||
3. **N-6 (observation) — the three session constructors share a
|
||||
near-identical prologue** (open → `BiStream::from_joined` →
|
||||
negotiated → teardown-on-err; `src/consumer.rs:121-149`,
|
||||
`290-324`, `702-733`). Collapse into one private
|
||||
`open_and_negotiate` helper — the teardown-at-birth invariant in
|
||||
one place instead of three (a batch-2 constructor then cannot
|
||||
hand-copy the reap path). Zero behavior change.
|
||||
|
||||
4. **S-2 (observation) — one doc line on the three sessions' `reap`**
|
||||
(`src/consumer.rs:197-202`, `490-495`, `787-792`): the dropped
|
||||
`teardown_channel` `JoinHandle` is deliberate — the producer-side
|
||||
pump task finishes via EOF propagation (the graceful shape);
|
||||
`clear_all` aborts instead (the substrate's abrupt path). No code
|
||||
change; prevents a future reader "fixing" the handle into an
|
||||
abort.
|
||||
|
||||
Tests: double `await_inbound` returns the spent shape AND the
|
||||
data-plane bytes survive untouched (the pinning test); the
|
||||
`reply1()`-twice path closed the same way; the existing BIND suite
|
||||
stays green unchanged (the round-trip, gate-refusal, factory/
|
||||
accept-failure mappings unaffected).
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] A second `BindSession::await_inbound` cannot re-read the data
|
||||
plane: it returns the spent shape, pinned by test, with the
|
||||
data-plane bytes asserted untouched
|
||||
- [ ] The `reply1()`-twice re-entry path is closed (the D-1
|
||||
decision: one normative reader; the alternate private/cut/
|
||||
spent-guarded) — the choice recorded in the task Notes
|
||||
- [ ] The session types' docs match the landed spent posture (the
|
||||
"`None` after a taken plane" claim is true of the code)
|
||||
- [ ] The three constructors share one `open_and_negotiate` helper;
|
||||
zero behavior change (the full consumer suite green unchanged)
|
||||
- [ ] The three `reap` sites carry the EOF-propagation doc line
|
||||
- [ ] `cargo test` green; clippy/fmt clean; both wasm checks clean
|
||||
|
||||
## References
|
||||
|
||||
- docs/reviews/002-protocol-core-implementation-review.md §C-1, §D-1,
|
||||
§N-6, §S-2
|
||||
- docs/architecture/consumer.md (the session postures; the
|
||||
spent-session shape)
|
||||
- tasks/implementation/core-consumer-bind.md (the landed BIND
|
||||
session; the `BindReply1` handle's spec'd intent)
|
||||
- tasks/implementation/review-protocol-core.md (R-2's
|
||||
`SpentSessionError` shape — the consistency target)
|
||||
|
||||
## Notes
|
||||
|
||||
> Agent fills this during implementation.
|
||||
|
||||
## Summary
|
||||
|
||||
> Agent fills this on completion.
|
||||
@@ -0,0 +1,110 @@
|
||||
---
|
||||
id: implementation/fix-coverage-residue
|
||||
name: "Tests — the review-002 coverage residue (N-8: stage exits, splits, spent arms)"
|
||||
status: pending
|
||||
depends_on: [implementation/fix-bind-reply2-reentry, implementation/fix-producer-gate-policy, implementation/fix-rfc1928-posture]
|
||||
scope: moderate
|
||||
risk: low
|
||||
impact: component
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
The review-002 coverage residue — the load-bearing gaps plus the
|
||||
single-arm cosmetics, attributed in
|
||||
`docs/reviews/002-protocol-core-implementation-review.md` §N-8 so
|
||||
the next `cargo llvm-cov` run needs no re-derivation. Tests only;
|
||||
rides *after* the three code-fix tasks so every test lands on the
|
||||
final shapes (the C-1 spent posture, the C-3 gate entry, the N-4
|
||||
vocabulary).
|
||||
|
||||
**The headline gaps** (the review's named priority):
|
||||
|
||||
1. **The datagram stage's three failure exits**
|
||||
(`src/producer.rs:374-386`, `405-414`) — the ASSOCIATE teardown
|
||||
surface, untested anywhere (the harness's egress never fails
|
||||
post-reply):
|
||||
- `ReplyFrameRejected` (an egress reply violating the joint
|
||||
bound)
|
||||
- `EgressEnded` via `next_reply`'s `Err` (the relay's end)
|
||||
- `EgressEnded` via a failing `egress.send` — at minimum this
|
||||
one gets the drop-witness treatment
|
||||
`egress_factory_called_once_and_dropped_with_the_stage` gives
|
||||
the happy path
|
||||
- the stage's `Io` exit (a failing write/read)
|
||||
2. **The two untested `split`s** (`src/consumer.rs:437`,
|
||||
`771-774`) — `AssociateSession::split` and `BindSession::split`
|
||||
on a *live* stream; the front-door-bridge surfaces
|
||||
(consumer.md), load-bearing for the batch-2 UDP/BIND doors. One
|
||||
test each (or the explicit "batch-2 door exercises it" note —
|
||||
prefer the test; the doors are not here yet).
|
||||
|
||||
**The single-arm residue** (batch freely; each is one small test):
|
||||
|
||||
3. `consumer.rs:356-359` — `send_datagram`'s spent-stream `Io` arm
|
||||
(mirror the `spent_session_*` shapes; the spent-`recv` arm is
|
||||
covered).
|
||||
4. `consumer.rs:419` — `recv_datagram`'s mid-batch later-frame `Err`
|
||||
arm (first frame `Ok`, a later one garbage → the pending `Ok`s
|
||||
dropped with the session — pin the fail-loud-drops-pending
|
||||
semantics).
|
||||
5. `consumer.rs:453-456` — `AssociateSession::into_inner`'s `Ok`
|
||||
arm (mirror `into_inner_pairs_the_pipe_with_the_teardown_owner`).
|
||||
6. `consumer.rs:636-642` — `BindDataPlane`'s `poll_flush`/
|
||||
`poll_shutdown` passthroughs (the BIND door's half-close rides
|
||||
these — one shutdown test).
|
||||
7. `consumer.rs:843` — `read_reply2`'s version-mismatch arm (a
|
||||
foreign producer's non-5 version reply#2 — one unit test on
|
||||
`read_reply2` directly).
|
||||
8. `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.
|
||||
9. `egress.rs:72-74, 226-230, 275-279` — the three `Debug` impl
|
||||
bodies (render once each); `error.rs:113-115` —
|
||||
`SocksEstablishmentReason`'s `Display`. Cosmetic; batch freely.
|
||||
|
||||
**Explicitly out of scope** (the review's accepted-uncovered
|
||||
posture): `rfc1928/client.rs:217-219` (`use_password_auth`'s misuse
|
||||
arm — review-001-era accepted-uncovered with a translated-error
|
||||
note) and the fork's accepted-uncovered blocks. Do not chase them.
|
||||
|
||||
If any arm became unreachable by the code-fix tasks (e.g. the C-1
|
||||
take-shape changes `BindSession::split`'s reachability), record the
|
||||
arm as resolved-by-change in the Notes rather than forcing a test
|
||||
through an artificial shape.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] The stage's failure exits tested (`ReplyFrameRejected`,
|
||||
`EgressEnded` both arms, `Io`) — the `send`-failure arm with
|
||||
the drop-witness assertion
|
||||
- [ ] `AssociateSession::split` + `BindSession::split` tested on a
|
||||
live stream (or the batch-2-door note recorded per arm)
|
||||
- [ ] The single-arm residue covered: spent-`send`, mid-batch
|
||||
later-frame `Err`, ASSOCIATE `into_inner`'s `Ok`,
|
||||
`BindDataPlane` shutdown, `read_reply2` version mismatch, the
|
||||
quiet-wait `Err(_)` arm, the Domain-peer fallback, the
|
||||
`Debug`/`Display` renders
|
||||
- [ ] `cargo llvm-cov` re-run: the named lines covered or recorded
|
||||
as accepted-uncovered with a reason in the task Notes
|
||||
- [ ] `cargo test` green; clippy/fmt clean; both wasm checks clean
|
||||
|
||||
## References
|
||||
|
||||
- docs/reviews/002-protocol-core-implementation-review.md §N-8 (the
|
||||
attributed line list — the task's source of truth)
|
||||
- tests/harness.rs (the topology + egress shapes to extend)
|
||||
- src/producer.rs `egress_factory_called_once_and_dropped_with_the_stage`
|
||||
(the drop-witness pattern)
|
||||
- tests/consumer-connect.rs `into_inner_pairs_the_pipe_with_the_teardown_owner`
|
||||
(the into_inner pattern)
|
||||
|
||||
## Notes
|
||||
|
||||
> Agent fills this during implementation.
|
||||
|
||||
## Summary
|
||||
|
||||
> Agent fills this on completion.
|
||||
@@ -0,0 +1,89 @@
|
||||
---
|
||||
id: implementation/fix-hygiene-residue
|
||||
name: "Fix — error-schema declaration + small hygiene residue (review-002 D-3, N-3, N-9)"
|
||||
status: pending
|
||||
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, 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`** —
|
||||
port 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 — 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) — one doc line naming the anticipated consumer
|
||||
("the doors") so the next dead-surface sweep does not flag it
|
||||
without this note.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] `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
|
||||
reworded doc matches the spec exactly
|
||||
- [ ] 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;
|
||||
`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
|
||||
|
||||
> Agent fills this during implementation.
|
||||
|
||||
## Summary
|
||||
|
||||
> Agent fills this on completion.
|
||||
@@ -0,0 +1,101 @@
|
||||
---
|
||||
id: implementation/fix-producer-gate-policy
|
||||
name: "Fix — producer BIND gate policy + register coverage + stage doc lines (review-002 C-3, D-2, S-1, N-2)"
|
||||
status: pending
|
||||
depends_on: []
|
||||
scope: narrow
|
||||
risk: medium
|
||||
impact: component
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Close review 002's producer-side API-shape hazard and the
|
||||
unexercised assembly surface, plus the two doc-line residues that
|
||||
ride the same files
|
||||
(`docs/reviews/002-protocol-core-implementation-review.md`):
|
||||
|
||||
1. **C-3 (minor) — `serve_socks5_over` derives `can_bind` from
|
||||
`accept.is_some()`** (`src/producer.rs:630-648`): factory-wired ⇒
|
||||
BIND allowed, with no way to say "factory wired but gate closed"
|
||||
on the substrate-free entry. 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*. For the batch-2 `local` backend — the documented
|
||||
consumer of this entry — the local door's BIND authorization has
|
||||
no seam to plug into except factory presence, which is exactly
|
||||
the shape that invites "listener configured" ⇒ "listener
|
||||
permitted" (the conflation ADR-006 exists to prevent).
|
||||
|
||||
Fix per the review's sketch: widen the substrate-free entry's
|
||||
gate to an explicit policy input — `serve_socks5_over_with_gate`
|
||||
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.
|
||||
|
||||
2. **D-2 (minor) — `Socks5Producer::register` and the builder's
|
||||
`egress`/`accept` setters are unexercised public surface** (0%
|
||||
covered; the harness registers via the free function
|
||||
`register_socks5_openable` instead, and the builder test drives
|
||||
only `serve_socks5_over`). The spec'd assembly shape
|
||||
(`producer.md` §Wiring) is the one assembly path with no test —
|
||||
a signature drift or delegation bug would pass the suite.
|
||||
|
||||
Fix: one integration test wiring a full
|
||||
`Socks5Producer::builder().dial(...).egress(...).accept(...)`
|
||||
through `.register(...)` and serving all three commands over the
|
||||
harness topology. This is the natural home for C-3's gate test.
|
||||
|
||||
3. **S-1 (minor) — the datagram stage's residual hot-`Ok(None)`
|
||||
posture, named as a decision.** The R-1 yield-guard makes the
|
||||
stage livelock-safe; the residual is a *misbehaving* egress
|
||||
returning `Ok(None)` at full speed = a yielding busy-wait
|
||||
(bounded by the egress's own correctness, unbounded by rate).
|
||||
Per the review: a posture note, not a backoff/budget (which
|
||||
would penalize the compliant path). One doc line on
|
||||
`run_datagram_stage` naming the residual.
|
||||
|
||||
4. **N-2 (observation) — one line in wire.md §UDP ASSOCIATE**: the
|
||||
per-datagram `egress.send` is awaited inline (the stage
|
||||
interleaves sends with reply drains); a slow egress applies
|
||||
backpressure to the association, not to other channels. The
|
||||
batch-2 composed-egress author inherits the bound knowingly.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] The substrate-free entry's BIND gate is an explicit policy
|
||||
input (with-gate entry or builder field); `accept.is_some()`
|
||||
remains only as the backward-compat default, never the sole
|
||||
gate
|
||||
- [ ] Test: accept factory wired + explicit `can_bind=false` ⇒ BIND
|
||||
refused 0x02; CONNECT/ASSOCIATE still serve
|
||||
- [ ] The pump handler's identity-derived gate is unchanged (the
|
||||
existing identity matrix stays green)
|
||||
- [ ] Integration test: full builder (`dial` + `egress` + `accept`)
|
||||
through `.register(...)` serving all three commands over the
|
||||
harness topology
|
||||
- [ ] `run_datagram_stage`'s doc names the hot-`Ok(None)` residual
|
||||
- [ ] wire.md §UDP ASSOCIATE carries the inline-send backpressure
|
||||
line
|
||||
- [ ] `cargo test` green; clippy/fmt clean; both wasm checks clean
|
||||
|
||||
## References
|
||||
|
||||
- docs/reviews/002-protocol-core-implementation-review.md §C-3, §D-2,
|
||||
§S-1, §N-2
|
||||
- docs/architecture/decisions/006-access-control.md (the gate
|
||||
posture — "listener configured" ≠ "listener permitted")
|
||||
- docs/architecture/producer.md §Wiring (the spec'd assembly shape)
|
||||
- docs/architecture/wire.md §UDP ASSOCIATE (the N-2 line's home)
|
||||
- tasks/implementation/core-producer-bind.md (the landed gate)
|
||||
|
||||
## Notes
|
||||
|
||||
> Agent fills this during implementation.
|
||||
|
||||
## Summary
|
||||
|
||||
> Agent fills this on completion.
|
||||
@@ -0,0 +1,123 @@
|
||||
---
|
||||
id: implementation/fix-rfc1928-posture
|
||||
name: "Fix — rfc1928 keep-or-cut pass + honest refusal vocabulary (review-002 N-1, N-4)"
|
||||
status: pending
|
||||
depends_on: []
|
||||
scope: narrow
|
||||
risk: medium
|
||||
impact: component
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
The review-001 keep-or-cut triggers have fired — the producer layer
|
||||
landed without consuming the anticipated surfaces — and the
|
||||
in-stream refusal vocabulary mislabels policy refusals as bugs.
|
||||
One posture pass over both
|
||||
(`docs/reviews/002-protocol-core-implementation-review.md`):
|
||||
|
||||
1. **N-1 (minor) — the fork's consumerless machinery.** The
|
||||
producer drives `negotiate_auth` with `&[NoAuthentication]` only;
|
||||
the seams carry their own error vocabularies; `pump_bidi`
|
||||
replaced `transfer`. Per the review's disposition:
|
||||
|
||||
**Cut** (the producer already replaced them; ADR-013's
|
||||
no-speculative-surface posture wins):
|
||||
- `StandardAuthentication` + `allow_no_auth` +
|
||||
`auth_method_enums!`'s generated enum
|
||||
(`src/rfc1928/server.rs:448-482`; the macro itself can stay or
|
||||
go with it — nothing else uses it)
|
||||
- `skip_auth_this_is_not_rfc_compliant`
|
||||
(`src/rfc1928/server.rs:185-187`; the consumer typestate's
|
||||
`skip_auth` config covers the in-tree need)
|
||||
- `CheckResult`'s `Option`/`Result` impls
|
||||
(`src/rfc1928/server.rs:162-172`; keep `bool` — re-add with a
|
||||
typed-rejection design if a door ever wants it)
|
||||
|
||||
**Keep with note** (the hand-port-map posture, ADR-013):
|
||||
- `ConnectError` (constructed nowhere, but the hand-port map's
|
||||
companion — its reply-code mapping is what a future port
|
||||
reuses; the module doc already says so)
|
||||
- `transfer`/`wait_on_tcp` (differential-suite-consumed; their
|
||||
semantics stay pinned)
|
||||
- `SocksServerError::Bug` (now consumed by the producer's
|
||||
refusal sites — see N-4 for its display)
|
||||
|
||||
The coverage blocks resolve automatically (the accepted-uncovered
|
||||
posture was exactly these blocks). Update the module docs where
|
||||
they name the cut machinery (e.g. `util/stream.rs`'s
|
||||
`ConnectError` note, the `auth_method_enums!` doc's "generate a
|
||||
similar enum" line) and the differential suite's comment that
|
||||
references `StandardAuthentication`'s method ordering
|
||||
(`tests/rfc1928-differential.rs:103` — reword to the RFC's
|
||||
ordering rationale, not the cut type).
|
||||
|
||||
2. **N-4 (observation) — the task-log refusal vocabulary mislabels
|
||||
policy refusals.** Five in-stream refusal sites return errors the
|
||||
wire never sees (the wire got its RFC-faithful reply already;
|
||||
these are log/observability signals only), but the vocabulary
|
||||
lies: the BIND command gate returns
|
||||
`SocksServerError::AuthenticationRejected` (an ACL refusal is not
|
||||
"authentication rejected" — `src/producer.rs:230-233`), and four
|
||||
seam refusals return `Bug("... refused in-stream")` (a policy
|
||||
refusal is not a bug; the display literally says `BUG: ...` —
|
||||
`src/producer.rs:262-264`, `302-304`, `483-485`, `541-543`).
|
||||
An operator grepping `BUG:` chases ghosts that are
|
||||
working-as-designed refusals.
|
||||
|
||||
Fix per the review's sketch: add one variant
|
||||
(`SocksServerError::Refused(&'static str)`-shaped) for the
|
||||
in-stream seam refusals and the command gate; use it at the five
|
||||
refusal sites. The wire bytes are untouched (these variants never
|
||||
reach the wire — the RFC reply went out first). Alternatively
|
||||
keep `Bug` and rename its display — but the dedicated variant is
|
||||
the honest shape; pick one and record it. Mind the fork's
|
||||
provenance posture (convention 17): this is an owned-code
|
||||
extension, so extend the module doc's carried-changes note if the
|
||||
variant list grows.
|
||||
|
||||
Tests: the five refusal sites' existing tests assert the mapped
|
||||
reply codes (unchanged — the wire is untouched); update any test
|
||||
asserting the *session-error* variant at those sites (the harness's
|
||||
`Bug` at `tests/harness.rs:387` is its own shape — leave unless it
|
||||
wants the new variant); one test pinning the new variant's
|
||||
display/shape if the vocabulary is table-tested elsewhere.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] `StandardAuthentication`, `allow_no_auth`, the generated
|
||||
enum, `skip_auth_this_is_not_rfc_compliant`, and
|
||||
`CheckResult`'s `Option`/`Result` impls are cut; `bool` stays
|
||||
- [ ] `ConnectError`, `transfer`/`wait_on_tcp`, `Bug` kept — each
|
||||
with its keep-note current (module docs name the posture)
|
||||
- [ ] The differential suite stays green (the cut machinery was not
|
||||
load-bearing for it; the `:103` comment reworded)
|
||||
- [ ] The five in-stream refusal sites carry the honest vocabulary
|
||||
(one variant or the renamed display — the choice recorded);
|
||||
the wire bytes are untouched (the reply-code assertions stay
|
||||
green)
|
||||
- [ ] The fork's provenance notice is extended, not replaced, if the
|
||||
carried-changes note names the new variant (convention 17)
|
||||
- [ ] `cargo test` green; clippy/fmt clean; both wasm checks clean;
|
||||
`cargo doc --no-deps` clean (module docs changed)
|
||||
|
||||
## References
|
||||
|
||||
- docs/reviews/002-protocol-core-implementation-review.md §N-1, §N-4
|
||||
- docs/reviews/001-rfc1928-full-review.md (the keep-or-cut triggers
|
||||
this closes out; the C-1 misleading-display note)
|
||||
- docs/architecture/decisions/013-targeted-fork.md (the hand-port
|
||||
map vs no-speculative-surface distinction)
|
||||
- docs/architecture/decisions/006-access-control.md (the gate —
|
||||
policy refusals are not authentication failures)
|
||||
- src/rfc1928/LICENSE (the provenance notice — extend, never
|
||||
replace)
|
||||
|
||||
## Notes
|
||||
|
||||
> Agent fills this during implementation.
|
||||
|
||||
## Summary
|
||||
|
||||
> Agent fills this on completion.
|
||||
@@ -0,0 +1,74 @@
|
||||
---
|
||||
id: implementation/fix-test-helper-consolidation
|
||||
name: "Tests — consolidate the duplicated harness shapes (review-002 N-5)"
|
||||
status: pending
|
||||
depends_on: [implementation/fix-coverage-residue]
|
||||
scope: narrow
|
||||
risk: low
|
||||
impact: component
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
The producer's test module re-implements the harness's shapes four
|
||||
times — ~250 structural-duplicate test lines
|
||||
(`docs/reviews/002-protocol-core-implementation-review.md` §N-5).
|
||||
The cause is legitimate (unit tests in `src/` cannot import
|
||||
`tests/harness.rs` — separate crates), so the fix is the standard
|
||||
shared-module pattern, not a harness import:
|
||||
|
||||
- `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` (the same trio)
|
||||
- `src/producer.rs:719-745` (echo-dial) vs `tests/harness.rs:131`
|
||||
|
||||
Fix per the review's sketch: a `tests/common/` path or a
|
||||
`#[cfg(test)]` `pub(crate)` module in `src/` compiled into both the
|
||||
lib test and the integration suite via a `path`-included module
|
||||
(the standard `mod common` pattern). Consolidates ~200 lines and
|
||||
keeps the two harness shapes from diverging further (they already
|
||||
have: the harness's `ChanEgress` and the producer's `QueueEgress`
|
||||
differ only in the mutex wrapper).
|
||||
|
||||
**Not collapsible** (deliberately different — leave them):
|
||||
`HotNoneEgress`/`PureHotNoneEgress` (R-1's two distinct violation
|
||||
shapes) and the `Witness` egress (carries the drop flag). The
|
||||
consolidation targets the *byte-near-identical* shapes only:
|
||||
`QueueEgress`/echo-dial and the `greet`/`reply` trio.
|
||||
|
||||
This task rides *after* the coverage-residue task so the
|
||||
consolidation sees the final test population (new tests from N-8
|
||||
may add more duplicates to fold). If the coverage task's new tests
|
||||
introduce fresh duplicates of harness shapes, fold them here too.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] The shared module exists and both the lib tests and the
|
||||
integration suite compile against it (the `mod common`
|
||||
pattern; no `tests/harness.rs` import from `src/`)
|
||||
- [ ] `QueueEgress`/echo-dial and the `greet`/`write_request`/
|
||||
`read_reply` trio are single-sourced (the byte-near-identical
|
||||
shapes)
|
||||
- [ ] `HotNoneEgress`/`PureHotNoneEgress`/`Witness` remain
|
||||
test-local (deliberately distinct shapes — not collapsed)
|
||||
- [ ] Zero behavior change: the full suite green with the same test
|
||||
count (minus any folded duplicates, recorded in the Notes)
|
||||
- [ ] `cargo test` green; clippy/fmt clean; both wasm checks clean
|
||||
|
||||
## References
|
||||
|
||||
- docs/reviews/002-protocol-core-implementation-review.md §N-5
|
||||
- tests/harness.rs (the shapes being single-sourced)
|
||||
- src/producer.rs (the four `Egress` impls + the trio)
|
||||
|
||||
## Notes
|
||||
|
||||
> Agent fills this during implementation.
|
||||
|
||||
## Summary
|
||||
|
||||
> Agent fills this on completion.
|
||||
@@ -0,0 +1,96 @@
|
||||
---
|
||||
id: review-fix-round-002
|
||||
name: "Review — the review-002 fix round before batch 2"
|
||||
status: pending
|
||||
depends_on: [implementation/fix-bind-reply2-reentry, implementation/fix-producer-gate-policy, implementation/fix-rfc1928-posture, implementation/fix-hygiene-residue, implementation/fix-coverage-residue, implementation/fix-test-helper-consolidation, architecture/adr-014-idle-timeout-posture]
|
||||
scope: narrow
|
||||
risk: low
|
||||
impact: project
|
||||
level: review
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
The closing checkpoint for the review-002 fix round — the gate
|
||||
between the fix tasks and batch 2 (the assembly layer: `local`
|
||||
providers, front doors, `composed` providers). Batch 2 consumes the
|
||||
session types, the gate entries, and the fork's posture, so a fix
|
||||
that silently changed a shape (the C-1 take-ownership, the C-3 gate
|
||||
entry, the N-4 vocabulary) would propagate into every door task.
|
||||
This is an inline review (it lives in `tasks/` and does not consume
|
||||
the `docs/reviews/` numbering — the next general review doc is
|
||||
`003-*`).
|
||||
|
||||
Verify the fix round against review 002's findings, in this order:
|
||||
|
||||
1. **C-1 closed correctly**: a second `await_inbound` cannot
|
||||
re-read the data plane (the spent shape pinned by test, the
|
||||
data-plane bytes asserted untouched); the D-1 reader decision is
|
||||
recorded and the alternate path is private/cut/spent-guarded;
|
||||
the session docs match the landed posture.
|
||||
2. **C-3 closed**: the substrate-free entry's BIND gate is an
|
||||
explicit policy input; "factory wired" no longer implies "BIND
|
||||
permitted"; the pump handler's identity-derived gate is
|
||||
untouched (the identity matrix green).
|
||||
3. **D-2 closed**: the spec'd assembly shape
|
||||
(`builder().dial().egress().accept()` → `.register(...)`) is
|
||||
exercised end to end.
|
||||
4. **N-1/N-4 landed coherently**: the cut list is gone (the
|
||||
coverage blocks resolved), the keep list carries current notes,
|
||||
the refusal vocabulary is honest at all five sites, the wire
|
||||
bytes are untouched, and the fork's provenance notice is
|
||||
extended-not-replaced (convention 17).
|
||||
5. **D-3 resolved**: the `ErrorDefinition` is declared (or the doc
|
||||
honestly reworded with the reason recorded) — doc-vs-code drift
|
||||
gone either way.
|
||||
6. **N-8 coverage**: `cargo llvm-cov` re-run; the named lines
|
||||
covered or recorded as accepted-uncovered with reasons; no new
|
||||
load-bearing gaps introduced by the fix round itself.
|
||||
7. **N-5/N-6 consolidation**: the shared helper module compiles
|
||||
into both test crates; the deliberately-distinct shapes were not
|
||||
collapsed; the constructor prologue is single-sourced.
|
||||
8. **C-2 decided**: ADR-014 exists, decides explicitly, and the
|
||||
spec lines carry the posture — the batch-2 gate condition.
|
||||
9. **Cross-cutting**: the full verification sweep (test,
|
||||
all-features, clippy, fmt, doc, both wasm checks,
|
||||
publish-dry-run); no convention regressions (no
|
||||
unwrap/expect/panics outside tests; no comments in code beyond
|
||||
doc comments + pinned correctness notes); the batch boundary
|
||||
still holds (no `local`/`composed` code exists).
|
||||
|
||||
Findings get the review-002 prefix treatment (`C`/`S`/`D`/`N` with
|
||||
an `F-` round prefix, e.g. `F-1`), fixed in the review change when
|
||||
small, filed back as tasks when not. The gate to batch 2 opens only
|
||||
when C-2's ADR is accepted and no major finding remains.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] Each review-002 finding verified closed (or explicitly
|
||||
re-dispositioned with a reason) — the per-finding checklist
|
||||
above
|
||||
- [ ] `cargo llvm-cov` re-run and attributed (no new load-bearing
|
||||
gaps)
|
||||
- [ ] The full verification sweep green, including
|
||||
`cargo publish --dry-run --allow-dirty`
|
||||
- [ ] Convention compliance re-swept (no unwrap/expect/panics
|
||||
outside tests; comment posture; wasm-clean)
|
||||
- [ ] ADR-014 accepted — the batch-2 gate condition holds
|
||||
- [ ] Findings recorded; the gate to batch 2 opens
|
||||
|
||||
## References
|
||||
|
||||
- docs/reviews/002-protocol-core-implementation-review.md (the
|
||||
findings this round closes)
|
||||
- docs/architecture/decisions/014-*.md (the C-2 decision — the
|
||||
gate condition)
|
||||
- tasks/implementation/review-protocol-core.md (the inline-review
|
||||
shape and findings format this follows)
|
||||
- AGENTS.md (the verification commands; conventions 1–17)
|
||||
|
||||
## Notes
|
||||
|
||||
> Agent fills this during implementation.
|
||||
|
||||
## Summary
|
||||
|
||||
> Agent fills this on completion.
|
||||
Reference in new issue
Block a user