From f83bca98dbabc71392f175e0153c485f7cf5f677 Mon Sep 17 00:00:00 2001 From: opencode controller Date: Sun, 28 Jun 2026 13:41:54 -0400 Subject: [PATCH] spec(slice-2): adversarial review patches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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). --- .../2026-06-28-slice-2-agent-tap-design.md | 130 +++++++++++++----- 1 file changed, 92 insertions(+), 38 deletions(-) diff --git a/docs/superpowers/specs/2026-06-28-slice-2-agent-tap-design.md b/docs/superpowers/specs/2026-06-28-slice-2-agent-tap-design.md index 51a5bc6..b729adf 100644 --- a/docs/superpowers/specs/2026-06-28-slice-2-agent-tap-design.md +++ b/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, // inbound PCM (drained from peer via TapAudioPipe::on_pcm_frame) tx_audio_out: mpsc::Sender, // 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`. 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`) +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": "" }`. @@ -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 -- 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). |