alkssh/tasks/implementation/review-wave-2.md

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

id	name	status	depends_on	scope	risk	impact	level
review-wave-2	Wave 2 gate: the server-stack review	completed	w2-server-mount w2-auth-bridge w2-loopback-harness w2-dispatch-decline-core w2-spawn-return w2-bounded-resources	narrow	low	phase	review

Description

The wave-2 review gate. The server stack is the substrate every mapping composes into (wave 3 hangs three mappings and the sftp row on it; wave 4 shares its foundations), so a shape error in the mount signature, the auth-bridge state, or the dispatch routing costs churn across all of them. The gate reads the landed stack against the architecture line by line:

• ADR-001, the mount contract: SshAdapter::handle takes a generic stream, mounts the full russh stack per stream, edge termination only; no ALPN/relay code exists; the per-stream limits posture holds.
• ADR-002, the Config construction (honest advertisement {publickey} exactly; the knob mapping; no des/dsa posture drift; the pin untouched).
• ADR-003, the feature gates as the dispatch's first cfg consumers: flat, independent, default = [] declines everything; x11/auth-agent never feature-gated; honest advertisement per build.
• ADR-004, the dispositions in code (explicit declines for every non-publickey method; offered-hook accept-and-delegate; ed25519-only explicit decline; fingerprint-only resolution; the identity rides the session).
• ADR-006, parse-only dispatch (no detect residue); decline-first- alloc-never structurally; the spawn-and-return constraint pinned by test, not convention.
• mapping.md, the skeleton decline-matrix rows green per build through the harness; the wire decline reasons vanilla-observable.
• Bounded resources, the cap enforced and recovering; the knobs behaviorally verified or recorded; no unbounded shape anywhere.
• The harness, with the contract-test vehicle convention established; the wave-1 doubles exercised live.
• Docs honesty: module docs state what is deliberately not here yet (the family seams, the client direction); the skeleton-stub behavior (declining routes awaiting wave 3) documented as such.
• The wave-1 gate's lesson absorbed, with cargo doc --no-deps running clean on this gate too.

Verdict: green (wave 3 decomposes, absorbing corrections) or findings booked (remediation before wave 3 decomposes).

Acceptance Criteria

• Every ADR/mapping surface above read and confirmed (or corrected with the correction landed)
• The spawn-and-return constraint verified as test-pinned; decline-first-alloc-never verified structurally
• Full verification set green (cargo test, --all-features, clippy ×2, fmt, cargo doc --no-deps)
• Verdict recorded; wave 3 decomposition unblocked or findings booked

References

• docs/plans/implementation.md (wave 2 section + this gate's role)
• docs/architecture/decisions/001…006, mapping.md, auth.md, overview.md, open-questions.md

Notes

Gate reading (line by line, 2026-10-28; commits b90e89b…196289f)

• ADR-001 ✓, as SshAdapter::handle is the mount: generic AsyncRead + AsyncWrite stream in, the full russh stack per stream via run_stream, returned run to completion (Result<(), AdapterError>, a decision recorded in the task's notes; no russh type rides the signature). No ALPN code, no relay leg, no alk/ssh string anywhere in src/. The per-stream limits posture holds: each mounted stream builds its own SessionHandler over the serve-config, so the cap/timeout/window budget is per-stream exactly as ADR-001's neutral consequence states.
• ADR-002 ✓, as the Config construction maps Limits one-for-one (window_size / maximum_packet_size / channel_buffer_size / max_auth_attempts verbatim; inactivity_timeout always Some with a test pinning the default postures and a second pinning the tunability); methods is built MethodSet::from(&[MethodKind::PublicKey][..]) with a test asserting inequality against russh's server_supported() (honest advertisement, not inherited); the pin is untouched (0.65, aws-lc-rs, no flate2/rsa/des/dsa); the recorded deliberations (server_id stays russh-default; keepalive stays client-direction) are in the task's notes.
• ADR-003 ✓, since the wave-2 dispatch core is the feature graph's first cfg consumer: flat, independent gates read via cfg! predicates (session ∨ git for session channels; tunnel for the three tunnel opens; session for pty/shell/pipe rows; git for the git row); default = [] declines everything (none-build wire rows); x11 (open and request) and agent forwarding decline unconditionally, never feature-gated; the sftp feature gates nothing and the subsystem row declines in every build (OQ-13 placeholder).
• ADR-004 ✓, with all dispositions in code as explicit hook implementations: auth_publickey (ed25519-only gate checked on both the signature algorithm and the key's algorithm before any resolution → fingerprint → resolve_from_fingerprint → accept-with-identity / reject fail-closed), the offered hook accept-and-delegates, and the four non-publickey methods each have an explicit reject body (never inherited). The bridge core (resolve_bridge) takes no username parameter, so OQ-03 holds structurally; wire rows prove username-independence. Auth declines are Auth::Reject dispositions end to end. The identity lands in per-connection handler state (SessionHandler::resolved_identity); its session-level landing is wave-3 assertion territory (flagged in the task's notes, as code is verifiable only through auth_succeeded consumption), confirmed as correctly deferred.
• ADR-006 ✓, showing parse-only dispatch with no detect residue (the SOCKS-detect is gone; a request-less session channel serves nothing by construction, since no serving path exists outside the request hooks); decline-first-alloc-never verified structurally: every open-level decline answers before any accept; the only pre-decline resource on any request path is none (exec parses via a pure fn first), and the spawned serving paths' only allocation is the post-decision handle clone; spawn-and-return is pinned by test, not convention; the serve.rs unit row (spawn returns while the path is pending) plus the two wire rows (tests/spawn_return.rs: handler-returns-fast under follow-up traffic with the spawn still gated; exit-status strictly before close, timestamps proving the report outlived the handler's return).
• mapping.md ✓, with the skeleton decline-matrix rows green per build through the harness (tests/dispatch_core.rs: shared rows in every build; per-build modules cfg-pinned to exactly one feature combination so no git-only row runs under --all-features by accident; each row carried a Contract stamp:). Wire decline reasons are vanilla-observable: open-level declines answer failure codes (AdministrativelyProhibited / the tunnel stub's ConnectFailed), request-level capability declines answer channel_failure with the channel provably still usable.
• Bounded resources ✓, with the in-crate cap enforced at the handler surface, recovering on close (wire row: cap reached → decline; close → open succeeds again, deterministic on the FIFO wire ordering; zero-cap row refuses everything without poisoning); the tunnel rows cap-gate before the stub so a declined stub cannot leak a count; max_auth_attempts verified behaviorally on the wire (ceiling → NoMoreAuthMethodsAvailable disconnect with the empty remaining-set shape); the size/window/clock knobs recorded as config-mapping-covered and the keepalive posture pinned by the harness row; max_exec_command_len correctly carried-not-enforced (wave-3 grammar's consumption, per the plan bullet). No unbounded shape anywhere: accepted-but-unserved session channels hold bounded buffers only (window/buffer knobs).
• The harness ✓, with the contract-test vehicle convention established exactly as the task pinned it: tokio-duplex transport
  • connect_stream (decision recorded in the module docs, over TCP loopback), sibling-of-the-doubles placement under tests/common/, the drive vocabulary the wave-3 mappings inherit, and the wave-1 doubles exercised live through it (MemIdentityProvider, MemHostKeySource, MemHostKeyPolicy both verdicts, fixture plumbing), as the acceptance row (in_process_pair_connects_and_authenticates_end_to_end) plus the auth-bridge and dispatch rows all ride it.
• Docs honesty ✓, as the handler module docs state precisely what is deliberately not here (the family seams, the client direction, the store-and-take handoff, the wave-3 tunnel accept-flow), each seam stub documents its wave-3 replacement point, and the inherited-posture note (the bare auth-agent / forwarded-streamlocal channel types decline via russh's own inline rejection at this pin, flagged as an upstream watch item since a pin bump that changed it would surface as accepted-then-dropped) is recorded rather than silently relied on.
• The wave-1 gate's lesson absorbed ✓, as cargo doc --no-deps ran clean on this gate (0 warnings), and taskgraph validate passes.

Interpretations (flagged for this gate), all confirmed

1. Git-parseability probe is skeleton-level, confirmed: classify_exec carries service-prefix + path-shape only, its module docs name the wave-3 replacement explicitly, and the probe rows pin exactly the POC-3 stage-A facts the skeleton needs (unknown service → pipe row; total over arbitrary input).
2. Routing stubs decline at the family-seam boundary, documented as skeleton; confirmed: the stub bodies are the named wave-3 replacement points (git_row_stub … tunnel_row_stub), the serving-path decline shape (failure then close) is wire-pinned, and the tunnel stub's ConnectFailed reason matches ADR-006 decision 1's fail shape.
3. Adapter error surface shaped in w2-server-mount, confirmed: AdapterError is the closed two-class taxonomy (Construction(#[from] KeySourceError) + Wire(#[from] russh::Error)) with the locked→no-server rule at construction and no catch-all; the wire decline reasons landed in the dispatch, not in the error taxonomy (the wave-1 reservation spent correctly).
4. Harness transport and placement pinned in w2-loopback-harness, confirmed and recorded in its module docs.

Minor issues found and fixed inline (this session)

Doc-hygiene only, in the task files (no src/ change warranted):

• w2-loopback-harness.md and w2-dispatch-decline-core.md: acceptance-criteria boxes still [ ] despite status: completed → checked (verified against the landed rows and runs).
• w2-bounded-resources.md: stale > To be filled… placeholder lines above already-filled Notes/Summary content → removed.
• review-wave-2.md (this file): placeholder lines removed; verdict recorded.

Booked findings (folded into the next waves, no remediation this gate)

• No observability instrumentation on the decline paths. The request-level decline class's "reason" is currently only the routing decision documented per build, so nothing server-side logs which reason a given wire decline carried (tracing is in the manifest per the ADR-002 toolkit but unused in src/ so far). Small follow-up task for the next wave's decomposition (alongside the real serving paths, where serve/decline path tracing matters more than in the stub wave): decline-path tracing events with the deciding context (row + feature build + channel), and a check of the family's tracing conventions before choosing the event shape.
• Carried forward, already documented (not new findings): the non-exhaustive-arg auth hooks are wire-testable only (resolves at wave 3's auth_succeeded consumption); forwarded-tcpip wire rows need the wave-3 acceptor vocabulary; the streamlocal/ direct-streamlocal runtime-Unix gate rides the wave-3 serving flow.

No shape errors found in the mount signature, the auth-bridge state, or the dispatch routing, the three churn-critical surfaces this gate exists to protect.

Verification (all green)

cargo test 104 rows across 7 binaries + doctests · cargo test --all-features (60 lib + integration rows) · cargo clippy --all-targets -- -D warnings ✓ · cargo clippy --all-features --all-targets -- -D warnings ✓ · cargo fmt --check ✓ · cargo doc --no-deps 0 warnings · taskgraph validate ✓ (19 tasks).

Summary

Verdict: green. The wave-2 server stack reads clean against the architecture line by line: ADR-001's mount contract (generic stream, full russh stack per stream, run-to-completion Result<(), AdapterError>, edge termination only), ADR-002's Config construction (exactly {publickey} advertisement, one-for-one knob mapping, always-set wall clock, pin untouched), ADR-003's feature graph as the dispatch's first cfg consumer (flat, default = [] declines everything, x11/auth-agent never gated), ADR-004's dispositions as explicit hook code (ed25519-only fingerprint bridge, four explicit declines, username structurally excluded), ADR-006's constraints parse-only dispatch, decline-first-alloc-never structural, and spawn-and-return pinned by test over the real wire, plus the bounded in-crate channel cap enforced and recovering at the handler surface, the skeleton decline-matrix rows green per feature build through the loopback harness, and docs that state what is deliberately deferred. All four flagged interpretations confirmed as recorded; minor task-file hygiene fixed inline; one small finding booked (decline-path tracing/observability, next wave). Wave 3 decomposition is unblocked and should absorb: the booked tracing task, the harness acceptor/vocabulary needs, and the deferred identity-landing assertion at wave 3's auth_succeeded.