diff --git a/docs/reviews/002-protocol-core-implementation-review.md b/docs/reviews/002-protocol-core-implementation-review.md index 0c6b356..840eb12 100644 --- a/docs/reviews/002-protocol-core-implementation-review.md +++ b/docs/reviews/002-protocol-core-implementation-review.md @@ -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 diff --git a/tasks/architecture/adr-014-idle-timeout-posture.md b/tasks/architecture/adr-014-idle-timeout-posture.md new file mode 100644 index 0000000..db1a264 --- /dev/null +++ b/tasks/architecture/adr-014-idle-timeout-posture.md @@ -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-.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. \ No newline at end of file diff --git a/tasks/implementation/fix-bind-reply2-reentry.md b/tasks/implementation/fix-bind-reply2-reentry.md new file mode 100644 index 0000000..3c1aa3b --- /dev/null +++ b/tasks/implementation/fix-bind-reply2-reentry.md @@ -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. \ No newline at end of file diff --git a/tasks/implementation/fix-coverage-residue.md b/tasks/implementation/fix-coverage-residue.md new file mode 100644 index 0000000..398c420 --- /dev/null +++ b/tasks/implementation/fix-coverage-residue.md @@ -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. \ No newline at end of file diff --git a/tasks/implementation/fix-hygiene-residue.md b/tasks/implementation/fix-hygiene-residue.md new file mode 100644 index 0000000..ea803db --- /dev/null +++ b/tasks/implementation/fix-hygiene-residue.md @@ -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. \ No newline at end of file diff --git a/tasks/implementation/fix-producer-gate-policy.md b/tasks/implementation/fix-producer-gate-policy.md new file mode 100644 index 0000000..a359f02 --- /dev/null +++ b/tasks/implementation/fix-producer-gate-policy.md @@ -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. \ No newline at end of file diff --git a/tasks/implementation/fix-rfc1928-posture.md b/tasks/implementation/fix-rfc1928-posture.md new file mode 100644 index 0000000..c9ad974 --- /dev/null +++ b/tasks/implementation/fix-rfc1928-posture.md @@ -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. \ No newline at end of file diff --git a/tasks/implementation/fix-test-helper-consolidation.md b/tasks/implementation/fix-test-helper-consolidation.md new file mode 100644 index 0000000..3f8948b --- /dev/null +++ b/tasks/implementation/fix-test-helper-consolidation.md @@ -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. \ No newline at end of file diff --git a/tasks/implementation/review-fix-round-002.md b/tasks/implementation/review-fix-round-002.md new file mode 100644 index 0000000..b6085c8 --- /dev/null +++ b/tasks/implementation/review-fix-round-002.md @@ -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. \ No newline at end of file