diff --git a/tasks/implementation/review-wave-2.md b/tasks/implementation/review-wave-2.md index 22cc42b..21915a1 100644 --- a/tasks/implementation/review-wave-2.md +++ b/tasks/implementation/review-wave-2.md @@ -1,7 +1,7 @@ --- id: review-wave-2 name: Wave 2 gate — the server-stack review -status: pending +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 @@ -52,13 +52,13 @@ booked (remediation before wave 3 decomposes). ## Acceptance Criteria -- [ ] Every ADR/mapping surface above read and confirmed (or +- [x] Every ADR/mapping surface above read and confirmed (or corrected with the correction landed) -- [ ] The spawn-and-return constraint verified as test-pinned; +- [x] The spawn-and-return constraint verified as test-pinned; decline-first-alloc-never verified structurally -- [ ] Full verification set green (`cargo test`, `--all-features`, +- [x] Full verification set green (`cargo test`, `--all-features`, clippy ×2, fmt, `cargo doc --no-deps`) -- [ ] Verdict recorded; wave 3 decomposition unblocked or findings +- [x] Verdict recorded; wave 3 decomposition unblocked or findings booked ## References @@ -69,8 +69,197 @@ booked (remediation before wave 3 decomposes). ## Notes -> To be filled by implementation agent +### 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 -> To be filled on completion \ No newline at end of file +**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`. \ No newline at end of file diff --git a/tasks/implementation/w2-bounded-resources.md b/tasks/implementation/w2-bounded-resources.md index cf558ff..c825510 100644 --- a/tasks/implementation/w2-bounded-resources.md +++ b/tasks/implementation/w2-bounded-resources.md @@ -61,8 +61,6 @@ not from this task. ## Notes -> To be filled by implementation agent - - **The count is handler-owned, not locked.** New private module `src/channel_cap.rs` (`ChannelCap`): the per-session active-channel count with `admit` (check+increment at accept sites) / `has_room` @@ -136,8 +134,6 @@ not from this task. ## Summary -> To be filled on completion - Landed the in-crate concurrent-channel cap at the handler surface: new private `channel_cap` module (count + admit/release semantics, unit-tested), wired into `SessionHandler`'s every open hook — the diff --git a/tasks/implementation/w2-dispatch-decline-core.md b/tasks/implementation/w2-dispatch-decline-core.md index cb1526e..df7b4df 100644 --- a/tasks/implementation/w2-dispatch-decline-core.md +++ b/tasks/implementation/w2-dispatch-decline-core.md @@ -61,21 +61,21 @@ declining stubs this task shapes the routes for. ## Acceptance Criteria -- [ ] Every `channel_open_*` hook gates per the feature graph; +- [x] Every `channel_open_*` hook gates per the feature graph; x11/auth-agent decline in every build; unfeatured types decline at open with clean wire reasons -- [ ] `exec_request` takes the parse path (git-parseable → git row, +- [x] `exec_request` takes the parse path (git-parseable → git row, else → session pipe row) before any allocation; request-less session channels decline as unfeatured per ADR-006 -- [ ] `subsystem_request` declines-with-reason for `sftp` (OQ-13 +- [x] `subsystem_request` declines-with-reason for `sftp` (OQ-13 placeholder) and git service names; `env_request` declines unconditionally (OQ-09) -- [ ] Decline-first-alloc-never holds structurally (declines precede +- [x] Decline-first-alloc-never holds structurally (declines precede any channel/resource allocation on every path) -- [ ] Feature-gated code paths exercised per build (`cargo test +- [x] Feature-gated code paths exercised per build (`cargo test --features git/session/tunnel` rows + `--all-features`); the skeleton decline-matrix rows green through the harness -- [ ] `cargo test`, `cargo clippy --all-targets -- -D warnings`, +- [x] `cargo test`, `cargo clippy --all-targets -- -D warnings`, `cargo clippy --all-features --all-targets -- -D warnings`, `cargo fmt --check` pass diff --git a/tasks/implementation/w2-loopback-harness.md b/tasks/implementation/w2-loopback-harness.md index 3874d71..87c5dd2 100644 --- a/tasks/implementation/w2-loopback-harness.md +++ b/tasks/implementation/w2-loopback-harness.md @@ -46,17 +46,17 @@ the wave-1 doubles work live through the wave-2 stack. ## Acceptance Criteria -- [ ] In-process server/client pair connects and authenticates end to +- [x] In-process server/client pair connects and authenticates end to end (fixture key → `MemIdentityProvider` → authenticated session) with no TCP port dependency (or the TCP choice recorded, if taken) -- [ ] Harness helpers cover: auth (success + failure observation), +- [x] Harness helpers cover: auth (success + failure observation), channel open (success + open-failure reason), session requests (success/failure), exit-status/close observation -- [ ] The transport choice and module placement are recorded -- [ ] The harness is documented as the contract-test vehicle; the +- [x] The transport choice and module placement are recorded +- [x] The harness is documented as the contract-test vehicle; the wave-1 doubles are exercised live through it -- [ ] `cargo test`, `cargo clippy --all-targets -- -D warnings`, +- [x] `cargo test`, `cargo clippy --all-targets -- -D warnings`, `cargo fmt --check` pass ## References