fix: heid-code-review fixups — donut voiced-interview slices 2+3

Triaged the heid-code-review panel (3 arms; reconciled against 56dce00 — three findings already closed by the bug-hunt, and the two firewalled lenses converged independently on the same three defects). Fixed the real one + contract precision. Code: - kb_bridge: no-hit sentinel (F7, the sharpest solo). The consult prompt asks Mimir to emit NO_CORPUS_MATCH when nothing is relevant; pin_kb_context drops any extract containing it -> [], so a non-empty "no results found" answer is never pinned as the character's own memory. Live-proven: a grounding query pins (5.6s), a nonsense query -> [] (0.7s); the sentinel is artificial so `in` can't false-positive on a passage. Contract (the CODE is correct; the spec had drifted/undercounted — kept canonical): - DEC-8: the custom "donut" voice was registered EARLY (verified live), so mapping ratatoskr:donut -> "donut" is right; "preset now" was stale. A live gateway read INVERTED the 3-arm remedy (reverting to a preset would have been the regression). - FN tts_synthesize: declared the `url` swap-seam param (F3); voice membership is gateway-enforced not client-asserted (F2); the postcondition is a container-level RIFF/WAVE check, not a 16-bit-PCM fmt parse. - FN tts_endpoint: pad is BROWSER-SENT per DEC-7, not a server PAD lookup (F5); documented the 413 text cap. - FN pin_kb_context: documented the sentinel + the session-delete hygiene. Tests (real coverage gaps): - the read_note prompt test asserts the distinguishing "do NOT call read_note" phrase, not the bare token an inverted prompt would also carry (#8 mutation-blind). - extract-bound asserts the literal 2000, not the impl constant it slices by (#9). - route roster asserts /api/tts + /api/sessions/{id}/messages (#10 undercount). - new server test: a degraded KB consult ([]) still streams the turn to done (F9). Accepted (not fixed): caller-supplied agent_id (LAN/no-auth debug-tool trust model); no DEC-5 concurrency test (asyncio.Lock is trivially correct — a test would test asyncio, not our code). 545 green.
2026-08-01 19:48:06 -07:00
parent c437388682
commit eb0767e96d
4 changed files with 87 additions and 17 deletions
@@ -76,10 +76,14 @@ each independently shippable. Slice order is chosen for fastest visible result.
  `affect_update` SSE the console already consumes) → Zonos `emotion_valence`
  (pleasure) + `emotion_arousal` (arousal). This reframes the feature as voice
  OBSERVABILITY (hear the affect the persona pane shows), not chat-app TTS.
- **DEC-8 — voice: preset now, custom later.** Donut starts on a theatrical
-  Zonos preset (of the 8; pick expressive — Miranda/Penny/Emmie). A custom
-  "donut" voice registers server-side via infra-ops later; adoption is a single
-  `voice:` string swap — no code change.
+- **DEC-8 — voice: custom "donut" is REGISTERED (superseded the preset-first plan).**
+  The original plan was a theatrical Zonos preset first (Miranda/Penny/Emmie), custom
+  "donut" later. But infra-ops registered + verified the custom `voice:"donut"` before
+  slice-2 build (GET /v1/voices returns Donut; case-folded), so `_TTS_VOICE_MAP` maps
+  `ratatoskr:donut → "donut"` directly — no interim preset. Non-interview agents still
+  fall to the gateway default (Cora). (Amended 2026-08-02 per heid-code-review: 3 arms
+  flagged the code as DEC-8 drift; a live gateway read INVERTED the remedy — the code is
+  correct, DEC-8's "preset now" was stale.)

 ## Invariants

@@ -110,11 +114,17 @@ each independently shippable. Slice order is chosen for fastest visible result.

 ### FN tts_synthesize
 ```
-tts_synthesize(text: str, *, voice: str, dials: EmotionDials, client: httpx.AsyncClient) -> bytes
+tts_synthesize(text: str, *, voice: str, dials: EmotionDials, client: httpx.AsyncClient, url=ZONOS_TTS_URL) -> bytes
  # POST {input:text, voice, response_format:"wav", **dials} to the Zonos gateway; return wav bytes.
-  precondition: text non-empty; voice in the gateway's /v1/voices set.
-  postcondition: returns 16-bit RIFF/WAVE bytes.
-  error: gateway non-200 / transport failure -> TtsUnavailable (caller degrades per INV-TTS-4).
+  # `url` (added — heid-code-review F3) is the swap seam (DEC-1): the endpoint passes app.state.tts_url;
+  # tests pass a respx-mocked URL. Defaults to ZONOS_TTS_URL so the parameter is inert for the common call.
+  precondition: text non-empty. Voice membership in /v1/voices is GATEWAY-enforced, not client-asserted
+                (an unknown voice surfaces as a gateway non-200 -> TtsUnavailable) — the contract + bundle
+                carry no local voice catalog, so a client-side check would fork a source of truth.
+  postcondition: returns a RIFF/WAVE container (magic 0:4 == "RIFF" AND form 8:12 == "WAVE"). The DEC-3
+                 guard is operational ("is this wav, not HTML / mislabeled-PCM"), NOT a full 16-bit-PCM
+                 fmt-chunk parse (heid-code-review: "16-bit" is aspirational; the guard is container-level).
+  error: gateway non-200 / transport failure / non-WAVE body -> TtsUnavailable (caller degrades per INV-TTS-4).
  invariant: response_format is ALWAYS "wav" (DEC-3); never mp3/opus.
 ```

@@ -131,9 +141,13 @@ pad_to_dials(pad: PadState | None) -> EmotionDials

 ### FN tts_endpoint  (server.py, /api/tts)
 ```
-POST /api/tts  {text, agent_id?}  -> audio/wav
+POST /api/tts  {text, agent_id?, pad?}  -> audio/wav
  steps:
-    - resolve voice (per-character map -> preset; default Cora) + dials (pad_to_dials of the agent's current PAD if known).
+    - resolve voice (per-character map -> "donut"; default Cora; a non-str agent_id coerces to None).
+    - dials = pad_to_dials(PadState.from_obj(pad)) — the PAD is BROWSER-SENT in the request body (per DEC-7:
+      the console already holds live PAD from the affect_update SSE), NOT a server-side PAD lookup. (Clarified
+      per heid-code-review F5 — the original "the agent's current PAD if known" wording read as a server fetch.)
+    - reject text over the max-char budget with 413 (bug-hunt: bound before the lock); missing/non-str text -> 400.
    - tts_synthesize(...) behind the serialize guard (DEC-5); return wav with Content-Type audio/wav.
    - on TtsUnavailable -> 503 controlled envelope (client skips playback, INV-TTS-4).
 ```
@@ -149,6 +163,12 @@ pin_kb_context(question: str, agent_id: str | None, *, client) -> list[dict]   #
    - return [{"kind":"corpus_reference","text":<extract>}].
  error/empty/timeout: any failure or no hits -> [] (INV-KB-3; never raises to the turn path).
                       CancelledError (browser disconnect) is NOT caught — it propagates.
+  no-hit sentinel (heid-code-review F7): the consult prompt asks Mimir to emit exactly NO_CORPUS_MATCH
+                       when the search finds nothing relevant; pin_kb_context drops any extract containing
+                       it -> [], so a non-empty "no results found" answer is never pinned as the character's
+                       own memory. The token is artificial (no genuine passage contains it).
+  session hygiene (heid-bug-hunt): the throwaway Mimir consult session is deleted (SDK sessions.delete) on
+                       success/error/timeout via a caller-owned holder, so consults don't accumulate upstream.
  CONSULT PROMPT (foot-gun mitigation, tuned live 2026-08-02): force "search_library EXACTLY ONCE,
    no read_note" — converges Mimir in ~3-15s. The softer "do one search" phrasing let Mimir loop
    read_note<->search past a 25s ceiling on conversational (non-keyword) questions (live-observed).
@@ -45,6 +45,11 @@ _KB_BRIDGE_END_USER = "ratatoskr-kb-bridge"
 _CONSULT_TIMEOUT_S = 20.0
 # Bound the pinned extract; a whole-note dump would bloat the character's prompt.
 _MAX_EXTRACT_CHARS = 2000
+# A no-hit sentinel the consult prompt asks Mimir to emit when the search finds nothing
+# relevant, so a non-empty "no results found" answer isn't pinned as corpus evidence and
+# voiced as the character's own memory (heid-code-review F7). The token is artificial —
+# no real corpus passage contains it — so a substring check is safe, not a fragile heuristic.
+_NO_MATCH_SENTINEL = "NO_CORPUS_MATCH"


 def _consult_prompt(question: str, corpus: str) -> str:
@@ -56,7 +61,8 @@ def _consult_prompt(question: str, corpus: str) -> str:
        f"You may call search_library EXACTLY ONCE and call no other tool "
        f"(do NOT call read_note). Query: {question} (corpus: {corpus}). "
        f"After the single search returns, immediately output the most relevant "
-        f"result excerpts verbatim and stop."
+        f"result excerpts verbatim and stop. If the search returns nothing relevant, "
+        f"reply with exactly {_NO_MATCH_SENTINEL} and nothing else."
    )


@@ -126,6 +132,9 @@ async def pin_kb_context(
                await client.sessions.delete(sid)
            except Exception:
                pass
-    if not extract:
+    if not extract or _NO_MATCH_SENTINEL in extract:
+        # Empty, OR Mimir signalled no relevant hit — don't pin a non-answer as memory
+        # (heid-code-review F7). The sentinel is artificial, so `in` can't false-positive
+        # on a genuine passage.
        return []
    return [{"kind": "corpus_reference", "text": extract[:_MAX_EXTRACT_CHARS]}]
@@ -112,22 +112,34 @@ class TestConsult:
        create_body = fake.calls[0][1]
        assert create_body["agent_id"] == "mimir"
        stream_prompt = fake.calls[1][1][1]  # ("stream_turn", (sid, prompt), {})
-        # The foot-gun mitigation: EXACTLY one search_library call, no read_note.
+        # The foot-gun mitigation: EXACTLY one search_library call, and read_note FORBIDDEN.
+        # Assert the distinguishing phrase, not the bare token "read_note" — a mitigation-
+        # inverted prompt ("DO call read_note") would still contain the token (heid-code-review).
        assert "EXACTLY ONCE" in stream_prompt
-        assert "read_note" in stream_prompt                  # forbids the loop tool
+        assert "do NOT call read_note" in stream_prompt      # forbids the loop tool (negation)
        assert "Dungeon Crawler Carl" in stream_prompt      # corpus scoping
        assert "who is Princess Donut?" in stream_prompt     # the question
+        assert "NO_CORPUS_MATCH" in stream_prompt            # the no-hit sentinel instruction

    async def test_empty_consult_returns_empty(self) -> None:
        fake = _FakeSessions(events=[_text("   "), _done("   ")])
        out = await pin_kb_context("q", _DONUT, client=_FakeClient(fake))
        assert out == []

+    async def test_no_match_sentinel_pins_nothing(self) -> None:
+        # A non-empty "no results" answer carrying the sentinel must NOT be pinned as
+        # corpus memory (heid-code-review F7) — even wrapped in other prose.
+        fake = _FakeSessions(events=[_done("I searched but found NO_CORPUS_MATCH here.")])
+        out = await pin_kb_context("q", _DONUT, client=_FakeClient(fake))
+        assert out == []
+
    async def test_extract_bounded_to_max_chars(self) -> None:
        long = "x" * 5000
        fake = _FakeSessions(events=[_done(long)])
        out = await pin_kb_context("q", _DONUT, client=_FakeClient(fake))
-        assert len(out[0]["text"]) == kb_bridge._MAX_EXTRACT_CHARS
+        # Assert the concrete bound (2000), not the impl constant it's sliced by — a
+        # mutated constant must fail this (heid-code-review: non-circular assertion).
+        assert len(out[0]["text"]) == 2000


 class TestDegradation:
@@ -764,8 +764,10 @@ class TestCreateAppShape:
            "/api/memory/chunks",
            # v0.19.2 debug-surface parity (create_app POST-002)
            "/api/sessions/{session_id}/tools",
+            "/api/sessions/{session_id}/messages",
            "/api/sessions/{session_id}/bifrost",
            "/api/admin/events",
+            "/api/tts",
            "/api/turns/{session_id}", "/api/turns/{session_id}/stream",
            "/api/turns/{session_id}/cancel",
        ):
@@ -1550,8 +1552,7 @@ class TestTtsEndpoint:
        assert json.loads(route.calls.last.request.content)["voice"] == "Cora"

    def test_text_too_large_returns_413(self) -> None:
-        from ratatoskr.web.server import create_app
-        from ratatoskr.web.server import _TTS_MAX_TEXT_CHARS
+        from ratatoskr.web.server import _TTS_MAX_TEXT_CHARS, create_app

        app = create_app(_mock_client_factory(), tts_url=self._TTS)
        resp = TestClient(app).post(
@@ -1628,3 +1629,31 @@ class TestKbBridgeWiring:
            b"".join(resp.iter_bytes())
        body = json.loads(route.calls.last.request.content)
        assert "memory_context" not in body or body["memory_context"] is None
+
+    @respx.mock
+    def test_failed_consult_still_streams_the_turn(self, monkeypatch) -> None:
+        # A degraded/failed KB consult (returns []) must NOT block or fail the character
+        # turn — it streams to `done` with no pinned context (INV-KB-3, server-level;
+        # heid-code-review F9: the turn-path failure seam was untested end to end).
+        from ratatoskr.web import server
+        from ratatoskr.web.server import create_app
+
+        async def _degraded_pin(question, agent_id, *, client):
+            return []  # consult failed/timed-out internally → degraded to []
+
+        monkeypatch.setattr(server, "pin_kb_context", _degraded_pin)
+        respx.post("https://w.example/sessions/s-1/messages").mock(
+            return_value=_sse_resp(
+                _sse_chunk("42:1", {"type": "text", "content": "hi"})
+                + _sse_chunk("42:2", _DONE_BODY)
+            )
+        )
+        c = TestClient(create_app(_mock_client_factory()))
+        tid = c.post(
+            "/api/turns/s-1",
+            json={"content": "who is Carl?", "agent_id": "ratatoskr:donut"},
+        ).json()["turn_id"]
+        with c.stream("GET", f"/api/turns/s-1/stream?turn_id={tid}") as resp:
+            raw = b"".join(resp.iter_bytes())
+        events = [e["event"] for e in _parse_browser_sse(raw)]
+        assert "text" in events and "done" in events  # the turn completed normally