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

---
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) — 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** — 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** — `cargo doc --no-deps` runs
  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** ✓ — `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>` — the 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** ✓ — 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** ✓ — 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** ✓ — 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 — 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 — code
  verifiable only through `auth_succeeded` consumption), confirmed as
  correctly deferred.
- **ADR-006** ✓ — parse-only dispatch with no detect residue (the
  SOCKS-detect is gone; a request-less session channel serves
  nothing by construction — 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** ✓ — the skeleton decline-matrix rows are 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** ✓ — the in-crate cap is 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** ✓ — 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) — the acceptance row
  (`in_process_pair_connects_and_authenticates_end_to_end`) plus the
  auth-bridge and dispatch rows all ride it.
- **Docs honesty** ✓ — 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, 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** ✓ — `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, 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 — 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`.