spec(slice-2): adversarial review patches

Patches the agent-tap design with seven findings from PM-side review
before the implementation plan gets written. Resolves invariant-breakers
and deferred-decision debt that would compound across steps 3-6.

- P1 (high): §8.5 #6 'byte-identical loop_driver.rs' was the wrong bar
  for the seam test. Reworded: the trait-method *call sites* are
  unchanged; impl bodies are free to differ. That's what 'the seam
  held' actually means — the contract is the test.
- P2 (high): tap: Option<TapHandle> and ChannelState were parallel
  state machines without an invariant tying them. Added an explicit
  state-pair table: tap is Some iff state == Connected. Lookups tolerate
  transient inconsistency; the source-of-truth is the pair, not either
  field alone.
- P3 (medium): v1 wire format was host-endian (silent big-endian
  hazard on paper). Nailed LE explicitly via i16::to_le_bytes /
  i16::from_le_bytes. v2 can negotiate endianness if a big-endian
  brain ever materializes; v1 contract is LE-only. Added a §3.4
  byte-order invariant alongside the sample-count invariant.
- P4 (medium): wss:// URLs were accepted by schema but returned
  501 at connect time (UX: 2 s into a call). Now rejected at
  POST /v1/sessions with 400 + clear message — fail fast at
  session-create. Updated §4.4, §7.1, §7.3, §9, and the decisions
  table for consistency; removed all stale '501 at connect' claims.
- P5 (low): TapEngine spawn owner was unspecified. Added explicit
  ownership: session_map::drive_all_sessions (the binary's poll
  task) observes the Connected transition from run_poll_once and
  spawns the engine. Keeps loop_driver.rs behaviorally unchanged
  (the §8.5 #6 seam test still holds).
- P6 (low): 'two new deps' in §1.1 undercounted. Updated list:
  tokio-tungstenite, futures-util, url, serde_json (if not already
  pulled).
- P7 (low): ADR-0007 was 'post-implementation follow-up, not a
  slice-2 deliverable.' Tightened: strongly recommended to land
  alongside or immediately after slice-2 implementation — once
  step 3 ships against an unpinned protocol, the wire shape will
  silently drift. The spec lists the decisions ADR-0007 should
  capture (LE byte order added to the list).

docs/superpowers/specs/2026-06-28-slice-2-agent-tap-design.md

@@ -68,15 +68,17 @@ The seam slice 1 pre-paved (`AudioSource` / `AudioSink` traits in `rutster-media
 - **Rust echo brain crate** (`crates/rutster-tap-echo`) — a dual-purpose binary crate:
   a standalone dev-loop binary (`cargo run -p rutster-tap-echo`) and an in-process
   `EchoServer` used by integration tests.
-- Two new deps: `tokio-tungstenite` (WS client + server), `serde_json` if not already
-  pulled. New workspace member crate. Thorough learner-facing comments on the new
+- New workspace deps: `tokio-tungstenite` (WS client + server), `futures-util`
+  (`Sink`/`Stream` traits for `WebSocketStream`), `url` (URL parsing/validation per
+  §4.4), `serde_json` if not already pulled by slice-1's axum dep tree. New workspace
+  member crate. Thorough learner-facing comments on the new
   async/mpsc/ring-buffer patterns (slice-1 §7 standard carries over).
 
 ### 1.2 Out of scope (with scheduled return)
 
 | Deferred item | Returns in | Why deferred |
 |---|---|---|
-| `wss://` cert validation / mTLS / brain cert pinning | Step 6 (spend cap) | Same rationale as slice-1 HTTP TLS — TLS needs the cert story from ARCHITECTURE.md (Vault/KMS), which lands with the real trust boundary + authz. `ws://` 127.0.0.1 dev loop is sufficient to prove the tap interface. Slice-2 accepts `wss://` URLs at the schema level but the connect attempt returns a clear `501 NOT IMPLEMENTED` so the seam is reserved without ratifying a half-impl. |
+| `wss://` cert validation / mTLS / brain cert pinning | Step 6 (spend cap) | Same rationale as slice-1 HTTP TLS — TLS needs the cert story from ARCHITECTURE.md (Vault/KMS), which lands with the real trust boundary + authz. `ws://` 127.0.0.1 dev loop is sufficient to prove the tap interface. Slice-2 rejects `wss://` URLs at `POST /v1/sessions` time with `400 Bad Request" + clear message — fails fast at session-create so operators see the config error immediately, not mid-call. The seam is reserved for step 6 without ratifying a half-impl. |
 | Authn / authz on the `tap_url` override | Step 6 | Inherits slice-1's "no auth yet" posture; authz on who-may-set-`tap_url` lands with the spend gate. Spec §4.5 flags this as a known gap. |
 | Barge-in / VAD-driven playout kill | Step 4 | No reflex to enforce yet; the playout buffer in slice-2 just queues + drops on overflow, doesn't kill on caller speech. |
 | Real brain (STT / LLM / TTS, OpenAI Realtime, etc.) | Step 3 | Slice-2 proves the *interface* and the core-authoritative playout posture; step 3 swaps echo → real brain. |
@@ -191,9 +193,10 @@ brainstorming resolution of the "showcase Rust binaries + external Python" goal:
 ## 3. Tap wire protocol (`rutster-tap/src/protocol.rs`)
 
 A minimal **versioned JSON event protocol over WS text frames**. Every frame is one JSON
-object. PCM payloads are base64-encoded raw bytes of the `PcmFrame`'s `[i16; 480]`
-in-memory repr (host-endian — see §9 Open Decisions for the v1 simplification; on a
-little-endian host this is LE).
+object. PCM payloads are base64-encoded raw bytes of the `PcmFrame`'s `[i16; 480]` in
+**explicit little-endian byte order** (the v1 wire format nails LE; v2 may negotiate
+endianness if a big-endian brain ever materializes, but the v1 contract is LE-only —
+no silent host-endian hazard).
 
 ### 3.1 Envelope (common to all messages)
 
@@ -239,6 +242,10 @@ little-endian host this is LE).
   (20 ms @ 24 kHz mono). The receiver validates; mismatched frames are logged + counted
   + dropped (hot-path "drop + observe" policy from slice-1 §3.8 — *not* a
   connection-terminating error).
+- **Byte-order invariant:** PCM inside the base64 is **little-endian `i16`** bytes
+  (v1 wire contract). Encoders use `i16::to_le_bytes`; decoders use
+  `i16::from_le_bytes`. No host-endian silent hazard. See §9 Open Decisions for the
+  v2 endianness-negotiation hook if a big-endian brain ever materializes.
 - **Versioning:** `v: 1`. Unknown envelope fields are ignored (forwards-compat for
   additive changes). Unknown `type` values are logged + counted + dropped (not fatal).
   A future v2 negotiates via a `v` upgrade on `hello`.
@@ -359,7 +366,7 @@ on `Channel::Closing`.
 ```rust
 pub fn spawn_tap_engine(
     session_id: ChannelId,
-    tap_url: Url,                                   // validated ws:// 127.0.0.1 OR wss:// (which 501s in slice-2)
+    tap_url: Url,                                   // validated ws:// 127.0.0.1 (wss:// already rejected at POST /v1/sessions per §4.4)
     tx_pcm_in: mpsc::Receiver<PcmFrame>,             // inbound PCM (drained from peer via TapAudioPipe::on_pcm_frame)
     tx_audio_out: mpsc::Sender<PcmFrame>,            // outbound PCM (playout ring feed)
     close: oneshot::Receiver<()>,                    // aborted on Channel::Closing
@@ -391,9 +398,14 @@ I/O supervisor task in slice-2 doesn't widen slice-1's documented deviation.
 
 `ws://` schemes must resolve to `127.0.0.1` or `localhost` — enforced as a hard runtime
 check at session-create time (returns `400 Bad Request` from `POST /v1/sessions` if
-violated, with a clear error message). `wss://` URLs are accepted by the schema and
-rejected at connect time with a logged `501 NOT IMPLEMENTED` and a counter — the seam
-is reserved for step 6 without ratifying a half-impl.
+violated, with a clear error message). `wss://` URLs are **rejected at session-create
+time** with `400 Bad Request" + message "wss:// lands in step 6; use ws:// for the
+slice-2 dev loop." This is a better UX than accepting the URL at the schema layer and
+`501`-ing at connect time — failing fast at session-create means the operator sees the
+configuration error immediately, not 2 seconds into a call after ICE+DTLS completes.
+
+The `wss://` codepath reservation (schema-accept + immediate rejection) keeps step 6
+from needing a schema change while not ratifying a half-impl that misleads operators.
 
 ---
 
@@ -405,17 +417,25 @@ is reserved for step 6 without ratifying a half-impl.
    absent, falls back to `RUTSTER_TAP_URL` env (default `ws://127.0.0.1:8081/echo`).
    Core validates the scheme (§4.4).
 2. `POST /v1/sessions/:id/offer` — unchanged from slice-1; SDP answer returned.
-3. On ICE+DTLS `Connected` (the slice-1 transition): the binary spawns a **`TapEngine`
-   task** for this `ChannelId`. The task dials the brain URL per §4.3.
+3. On ICE+DTLS `Connected` (the slice-1 transition): **the binary's
+   `session_map::drive_all_sessions` poll task observes the `Connected` state
+   transition** (set inside `loop_driver::handle_event` on `Event::IceConnectionStateChange(Connected)`
+   — slice-1 code path, unchanged). When `run_poll_once` returns and the poll task sees
+   `channel.state == Connected && channel.tap.is_none()`, it spawns the `TapEngine`
+   task for this `ChannelId`. This keeps the spawn in the binary's session-map layer,
+   not in `rutster-media`'s loop driver (so the loop driver is still unaware of the
+   tap; slice-1's `loop_driver.rs` stays behaviorally unchanged per §8.5 #6).
    `Channel.tap = Some(TapHandle)` is set; the binary maps the `ChannelId` to the
    engine's mpsc handles in an internal `DashMap<ChannelId, TapConn>`.
 4. `Channel.state` transitions still drive the loopback peer (slice-1's machine
    unchanged). The `Connected→Closing→Closed` path is unchanged except for the additional
    tap teardown step in step 5 below.
-5. `DELETE /v1/sessions/:id` or peer-close → `Closing`: the binary sends `session_end`
+5. `DELETE /v1/sessions/:id` or peer-close → `Closing`: the poll task observes the
+   `Closing` transition (or the DELETE handler sets it directly). It sends `session_end`
    over the tap WS, awaits brain `bye` (bounded 500 ms), closes the WS, drops the engine
-   task (the `close` oneshot fires + `JoinHandle::abort`). `Channel.tap = None`. Then
-   slice-1's `Closing → Closed` path runs.
+   task (the `close` oneshot fires + `JoinHandle::abort`). `Channel.tap = None` is set
+   *before* the state advances to `Closed` (per §6's state invariant). Then slice-1's
+   `Closing → Closed` path runs.
 
 ### 5.2 Failure mode
 
@@ -485,6 +505,24 @@ similarly internal to the binary, not the `Channel`.
 attaches (New, Connecting states) and after it detaches (Closing, Closed). The None
 transition on Closing is the tap teardown signal the binary acts on.
 
+**State invariant (load-bearing):** `tap` and `ChannelState` are *not* parallel state
+machines — `tap` is tied to the `ChannelState` transitions:
+
+| `ChannelState` | `tap` field | Note |
+|---|---|---|
+| `New` | `None` | Not yet attached |
+| `Connecting` | `None` | Tap not spawned until ICE+DTLS reaches `Connected` |
+| `Connected` | `Some(TapHandle)` | Set as the `TapEngine` task is spawned |
+| `Closing` | transitively `None` | Teardown in flight — session_end sent, task aborted, field cleared to `None` *before* state advances to `Closed` |
+| `Closed` | `None` | Always |
+
+A `Channel` in `Connected` with `tap: None` is a **bug** — the engine task spawn failed
+and the state machine wasn't rolled back. The binary's per-state-transition logic MUST
+set the two fields together; lookups in the tap registry (`DashMap<ChannelId, TapConn>`)
+tolerate transient inconsistency (a `None` here means "asked too early" or "just tore
+down" — return `None` / silence), but the source-of-truth is the state pair, not the
+field alone.
+
 The `Channel`'s `id` field *is* the `session_id` carried in the tap's `hello` messages
 (§5.3) and the lookup key for the binary's tap registry — no separate `TapId` newtype.
 This means slice-2's design assumes a **1:1 mapping** between a `Channel` and its tap
@@ -514,9 +552,11 @@ Body now optionally carries a `tap_url`:
 - Body is optional; absent body → `tap_url = RUTSTER_TAP_URL` env default.
 - Body present, no `tap_url` field → same as absent body (env default).
 - Body present, `tap_url` field → env default overridden; scheme validated per §4.4.
-  Returns `400 Bad Request` on a non-loopback `ws://` URL or an unparseable URL.
-- `wss://` URLs pass schema validation and `501 NOT IMPLEMENTED` at connect time
-  (deferred to step 6).
+  Returns `400 Bad Request` on a non-loopback `ws://` URL, an unparseable URL, or a
+  `wss://` URL (deferred to step 6 — fail fast at session-create, don't `501` mid-call).
+- `wss://` URLs are rejected at `POST /v1/sessions` with `400 Bad Request" + message
+  "wss:// lands in step 6; use ws:// for now." Faster failure than accepting the URL
+  and `501`-ing at connect time.
 
 Response unchanged from slice-1: `{ "session_id": "<uuid>" }`.
 
@@ -532,9 +572,9 @@ debug area.
 Slice-2 inherits slice-1's "no authn/authz" posture. The `tap_url` override means any
 caller can point the core's tap at an arbitrary URL — a privilege that will require
 authn/authz in step 6. Slice-2's segment is local dev loop only (no production
-deployment); the gap is documented, not closed. The spec's `wss://` reservation (accept
-URL, 501 at connect) and `127.0.0.1`-only `ws://` enforcement bound the surface — a
-malicious local caller is on a trusted host.
+deployment); the gap is documented, not closed. The spec's `wss://` rejection at
+session-create (`400 Bad Request" with clear message — see §4.4) and `127.0.0.1`-only
+`ws://` enforcement bound the surface — a malicious local caller is on a trusted host.
 
 ---
 
@@ -633,12 +673,20 @@ The slice is complete when, on a clean checkout:
 5. **Both** `rutster-tap-echo` (Rust) **and** `examples/echo_brain/echo_brain.py`
    (Python) successfully interop against the core — proves the protocol is
    language-agnostic.
-6. `rutster-media`'s `loop_driver.rs` and `rtc_session.rs` are **byte-identical in
-   their media-loop paths** to slice-1 (the seam-test: the only diff in `rutster-media`
-   is that `EchoAudioPipe` is retained alongside `TapAudioPipe`, not replacing it). A
+6. **The seam test (load-bearing):** `rutster-media`'s `loop_driver.rs` and
+   `rtc_session.rs` keep their media-loop **trait-method call sites unchanged** —
+   `AudioSink::on_pcm_frame(...)` and `AudioSource::next_pcm_frame()` are still the
+   only calls the loop makes into the audio path. The *impl bodies* of those traits
+   differ (`EchoAudioPipe`'s pure-queue-pop becomes `TapAudioPipe`'s `try_recv` +
+   playout-ring drain), but that's the seam doing its job — the impl varies, the
+   call site doesn't. `RtcSession` is constructed with a different pipe (one-line
+   construction change at the binary boundary, not in `rutster-media`); the
+   `loop_driver`'s poll/inbound/outbound logic is behaviorally unchanged. A
    `git diff v<slice-1-tag> -- crates/rutster-media/src/loop_driver.rs
-   crates/rutster-media/src/rtc_session.rs` shows no hunks in the media-loop
-   functions (doc-comment edits are permitted; no behavior change).
+   crates/rutster-media/src/rtc_session.rs` shows no behavior-changing hunks
+   (doc-comment or import changes permitted). The trait *contract* is the
+   mistake-proofing — the loop calls the same methods whether the pipe echoes
+   in-process or ships PCM over WSS.
 7. `LEARNING.md` grows ≥3 new pointers: `mpsc`/`oneshot` patterns →
    `crates/rutster/src/tap_engine.rs`; `VecDeque` as a bounded ring →
    `crates/rutster-tap/src/tap_audio_pipe.rs`; async WS connect + `Sink`/`Stream` →
@@ -653,22 +701,28 @@ The slice is complete when, on a clean checkout:
   prefixed mode (raw LE i16 over WS binary frames) for a later rung. Re-evaluate when
   (a) a real brain (step 3) hits bandwidth ceilings, or (b) the fuzz harness (step 5)
   wants to fuzz a binary parser.
-- **Byte-endian negotiation.** v1 emits the host's native endian as raw bytes inside
-  the base64 payload. On a little-endian host (today's typical target) this is LE; on
-  a hypothetical big-endian host it would be BE without warning. The protocol should
-  nail explicit LE byte order in v2; for v1 it's documented as a simplification.
-  Today: every dev-loop brain (Python `websockets` lib, Rust on x86_64/aarch64) is
-  little-endian, so the risk is theoretical.
+- **Byte-endian negotiation.** v1 emits **explicit little-endian** bytes inside the
+  base64 payload (the encoder converts `i16` → `[u8; 2]` via `to_le_bytes()`; the
+  decoder reverses via `from_le_bytes()`). No host-endian silent hazard. If a
+  big-endian brain ever materializes (e.g. an exotic embedded target), v2 can
+  negotiate endianness on `hello`; v1's contract is LE-only. Today every dev-loop
+  brain (Python `websockets` lib, Rust on x86_64/aarch64) is little-endian, so the
+  explicit-LE choice costs nothing and removes a future ops gotcha.
 - **Tap protocol ADR.** PORT_PLAN §10 lists the agent-tap protocol as "presumptively
   WSS + core-as-client + clean PCM + core-authoritative playout," *not* a decided ADR
   yet. Slice-2 **hardens** the presumptive shape against a working implementation. The
   spec's "Implementation lands, then ADR ratifies the wire shape" is the deliberate
   sequence — an ADR-0007 (or similar) capturing slice-2's ratified decisions (JSON
-  envelope, base64 v1, core-authoritative playout, stateless reconnect contract) is
-  a post-implementation follow-up, not a slice-2 deliverable. The spec's existence is
-  the ratification for slice-2; the ADR is the durable form.
-- **`wss://` cert/mTLS posture.** Slice-2 reserves the `wss://` URL scheme (accepted at
-  schema validation, `501 NOT IMPLEMENTED` at connect) so step 6 doesn't need a schema
+  envelope, base64 v1, explicit-LE byte order, core-authoritative playout, stateless
+  reconnect contract, fail-fast wss:// rejection at session-create) is **strongly
+  recommended to land alongside or immediately after slice-2 implementation**, not
+  deferred past step 3. Once a real brain (OpenAI Realtime adapter in step 3) is
+  coded against this protocol, the wire shape will silently drift if not pinned in
+  an ADR. The spec's existence is the ratification for slice-2; the ADR is the durable
+  form — don't let "durable form" become "tomorrow's problem" while step 3 ships
+  against an unpinned protocol.
+- **`wss://` cert/mTLS posture.** Slice-2 rejects `wss://` URLs at session-create
+  time (`400 Bad Request" + clear message — see §4.4) so step 6 doesn't need a schema
   change. The actual cert-validation / mTLS / brain-cert-pinning impl is step-6 work.
 
 ---
@@ -707,7 +761,7 @@ slice-2 spec §1.2."
 | Tap architecture | **B. Decoupled TapEngine** — `TapAudioPipe` is a thin sync wrapper over mpsc + ring; `TapClient` (inside the engine task) owns the WSS connection; `RtcSession` only swaps `EchoAudioPipe` → `TapAudioPipe`. | A. In-pipe tap (WSS task owned by the AudioPipe); C. Explicit `tap` field on `RtcSession` bypassing the seam | Honors slice-1 §3.3's promise verbatim ("no code changes to `RtcSession` itself in step 2"); keeps the 20 ms loop pure (only mpsc + ring-touch); cold-path I/O task ≠ the step-4 forbidden "dedicated timing thread"; reconnect is localized to the engine. |
 | Wire protocol | **Own minimal versioned JSON event protocol**, base64 PCM in text WS frames. | Adopt OpenAI Realtime event schema verbatim; binary length-prefixed framing | ARCHITECTURE.md names WSS as presumptive transport because the consumer is a Python script / OpenAI-Realtime-style API; JSON event envelope is the natural mapping onto that ecosystem. Avoids vendor lock-in at our central interface; the step-3 OpenAI adapter translates. ~33% wire overhead is acceptable at 65 KB/s. |
 | Reference echo brain | **Both**: Python `examples/echo_brain/` (canonical foreign-language) + Rust `crates/rutster-tap-echo` (showcase + integration-test `EchoServer`). | Python-only; Rust-only | Python proves the protocol is language-agnostic and matches the "brain is a Python script" persona; Rust proves the wire types are reusable from outside the core and doubles as the in-process test server. The dual is the user's "showcase Rust binaries + external scripts" goal. |
-| TLS on tap | **`ws://` loopback only, `wss://` deferred to step 6.** Hard runtime check on `127.0.0.1`/`localhost`; `wss://` URL accepted by schema, `501 NOT IMPLEMENTED` at connect. | Always-`wss://` even for localhost (self-signed CA burden); plaintext-`ws://`-only with no `wss://` codepath | Matches slice-1's "TLS needs a cert story" stance; keeps the dev loop zero-cert. Reserves the `wss://` seam so step 6 doesn't need a schema change. |
+| TLS on tap | **`ws://` loopback only, `wss://` deferred to step 6.** Hard runtime check on `127.0.0.1`/`localhost`; `wss://` URL rejected at session-create with `400 Bad Request" + clear message ("wss:// lands in step 6; use ws:// for now"). | Always-`wss://` even for localhost (self-signed CA burden); plaintext-`ws://`-only with no `wss://` codepath; accepting wss:// at schema and `501`-ing mid-call | Matches slice-1's "TLS needs a cert story" stance; keeps the dev loop zero-cert. Fails fast at session-create so operators see config errors immediately, not 2 s into a call. Reserves the `wss://` seam so step 6 doesn't need a schema change. |
 | Tap URL config | **Env default + per-call override.** `RUTSTER_TAP_URL` env (default `ws://127.0.0.1:8081/echo`); `POST /v1/sessions` body optional `tap_url` overrides. | Env-only; per-call-only | Env default = simplest dev loop; per-call override demonstrates the routing seam (the precursor to multi-brain routing in step 6); authn on the override is a flagged step-6 gap (§7.3). |
 | Brain failure | **Silence + bounded-backoff reconnect (infinite retries).** 250 ms → 500 ms → 1 s → 2 s → cap 5 s; Channel stays Connected; playout flushed on disconnect; reconnect re-`hello`s with same `session_id`; stateless brain contract. | Silence + give-up; tear-down-the-call | Proves "tap is advisory; core disposes" hard — the call outlives the brain. Self-healing is the right posture for a real brain (which may also crash / restart). |
 | Playout buffer policy | **Drop-oldest on overflow, silence on underflow.** Capacity 5 frames (100 ms). | Drop-newest; larger / smaller capacity | Drop-oldest is lowest-latency-correct (sheds stale frames, keeps buffer at-or-behind real-time); 5 frames absorbs brain jitter without introducing perceptible delay. Capacity is a tunable *constant* in slice-2 (no runtime config). |