--- id: review-wave-2 name: "Wave 2 gate: the server-stack review" status: completed depends_on: [w2-server-mount, w2-auth-bridge, w2-loopback-harness, w2-dispatch-decline-core, w2-spawn-return, w2-bounded-resources] scope: narrow risk: low impact: phase level: 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 - [x] Every ADR/mapping surface above read and confirmed (or corrected with the correction landed) - [x] The spawn-and-return constraint verified as test-pinned; decline-first-alloc-never verified structurally - [x] Full verification set green (`cargo test`, `--all-features`, clippy ×2, fmt, `cargo doc --no-deps`) - [x] 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`.