docs(tasks): close the wave-2 review gate — verdict green
Gate reading (commits b90e89b..196289f) against the architecture line by line: ADR-001 mount contract, ADR-002 Config construction, ADR-003 feature gates, ADR-004 dispositions, ADR-006 constraints, mapping.md skeleton decline matrix per build, bounded resources, and the harness — all confirmed. All four flagged interpretations confirmed as recorded. Fixed inline before committing (task-file hygiene only): - w2-loopback-harness, w2-dispatch-decline-core: acceptance boxes checked once verified against the landed rows/runs - w2-bounded-resources: stale placeholder lines removed - review-wave-2: verdict recorded Booked finding (next wave's decomposition): decline-path tracing/observability — the reason a given wire decline carried is currently docs-only (tracing unused in src/). Verification: cargo test 104 rows green; --all-features green; clippy ×2 -D warnings green; fmt clean; cargo doc 0 warnings; taskgraph validate 19 tasks.
This commit is contained in:
1 parent
196289f5ad
commit
5b2654f492
4 files changed
+207
-22
No files matched your search
@@ -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
|
||||
**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`.
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in new issue
Block a user