alksocks/tasks/implementation/fix-rfc1928-posture.md

id, name, status, depends_on, scope, risk, impact, level

id	name	status	depends_on	scope	risk	impact	level
implementation/fix-rfc1928-posture	Fix — rfc1928 keep-or-cut pass + honest refusal vocabulary (review-002 N-1, N-4)	completed		narrow	medium	component	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

• N-4 shape chosen: the dedicated variant — SocksServerError::Refused(&'static str), display "Refused: {0}" — not the Bug display rename. The review's sketch favored the dedicated variant ("the honest shape"), and the vocabulary split is real: Bug stays upstream-verbatim (BUG: {0}, its "should be unreachable" contract intact) while policy refusals get their own honest label. The five sites now read Refused("bind command gate: not allowed"), Refused("dial refused in-stream"), Refused("egress refused in-stream"), Refused("bind factory refused in-stream"), Refused("bind accept refused in-stream") — same &'static str payload shape as Bug, zero wire impact (the RFC reply went out before each Err).
• The harness's Bug site was migrated too (the task said "leave unless it wants the new variant"): tests/harness.rs's minimal CONNECT producer returned Bug("dial refused in-stream") — the same working-as-designed refusal shape, so it takes the new variant for vocabulary consistency across the log surface. No test asserts that harness error's variant, so nothing else moved.
• The mapping-table test grew the new variant: SocksServerError::Refused rides the GeneralFailure fall-through loop in to_reply_error_mapping_table (alongside Bug), and a new test pins the display string and the fall-through mapping (refused_variant_display_is_honest_and_ wire_invisible) — the task's "one test pinning the new variant's display/shape".
• Provenance extended, not replaced (convention 17): the server.rs module doc gained a "Post-fork owned changes" paragraph (the cuts, the keep-notes, the Refused addition); docs/architecture/rfc1928.md gained the matching "owned-code post-fork changes" list under the deliberate modifications and a last_updated line. The bundled src/rfc1928/LICENSE notice already carries the "modifications after the fork point" clause generically — no change needed there.
• Keep-notes made current: stream.rs's ConnectError note records that the trigger fired and the disposition is keep (was: "cut it then"); transfer/wait_on_tcp's differential- suite note lives in the server.rs module doc; Bug's keep-note is the same paragraph (upstream-verbatim, its display untouched).
• The differential suite's :103 comment reworded to the RFC-ordering rationale (the server picks by its own preference, more-secure-first) — no cut-type reference remains anywhere.
• The AuthMethod trait doc was updated for the macro cut (upstream's "use enums for sets" line → a custom selection is a slice of method structs) and the module doc's extraction-time adjustments line dropped the #[macro_export] note (the macro itself is gone).

Summary

Both items landed. N-1 (the cut): auth_method_enums! (the macro, nothing else used it), StandardAuthentication + StandardAuthenticationStarted + allow_no_auth, skip_auth_this_is_not_rfc_compliant, and CheckResult's Option/Result impls are gone from src/rfc1928/server.rs — the producer-replaced machinery, per ADR-013's no-speculative-surface posture. Kept with notes: ConnectError (hand-port companion, stream.rs note updated to record the fired trigger + keep disposition), transfer/wait_on_tcp (differential-suite-consumed, semantics pinned there), Bug (upstream-verbatim, display intact). N-4 (the vocabulary): new SocksServerError::Refused(&'static str) variant, used at the five in-stream refusal sites (the BIND command gate + the four seam refusals) plus the harness's minimal producer; Bug's display is untouched (a real bug should still read BUG:); the wire bytes are byte-identical (every reply-code assertion stayed green unchanged). Tests: the three gate-site session-error asserts moved from AuthenticationRejected to Refused(...), the four Bug(...) asserts to Refused(...), the mapping-table loop grew the variant, and a new display/shape test pins both. Docs: the server.rs module doc's owned-changes paragraph, the rfc1928.md "owned-code post-fork changes" list + last_updated, the stream.rs keep-note, the AuthMethod doc, and the differential comment. The review-002 N-1 coverage blocks resolve (the cuts were the accepted-uncovered surface).

Verified: cargo test (356 green across all suites incl. --all-features), cargo clippy --all-targets -- -D warnings clean, cargo fmt --check clean, cargo doc --no-deps clean, cargo check --target wasm32-unknown-unknown + cargo clippy --target wasm32-unknown-unknown -- -D warnings clean.