feat(agent): strip raw tool-scaffolding from streamed answers — deterministic filter with one bounded recovery

tests/unit/test_agent.py

@@ -32,7 +32,7 @@ import logging
 import uuid
 from collections.abc import AsyncGenerator, AsyncIterator
 from copy import deepcopy
-from typing import Any, cast
+from typing import TYPE_CHECKING, Any, cast
 
 import pytest
 from sqlalchemy.orm import Session
@@ -43,11 +43,15 @@ from app.rag import agent
 from app.rag.agent import (
     AGENT_TOOLS,
     AgentHolder,
+    MalformedReplyError,
     run_agent,
 )
 from app.rag.llm import LLMClient, LLMError, RetryPiece, StreamPiece, ToolCallPiece
 from app.rag.prompts import TOOLS_SECTION, _base, build_deflect_prompt, build_high_prompt
 
+if TYPE_CHECKING:
+    from app.rag.scaffolding import ScaffoldingFilter
+
 
 def _settings(**kwargs: Any) -> Settings:
     kwargs.setdefault("_env_file", None)
@@ -68,7 +72,13 @@ def _doc(source: str, path: str, title: str = "Title", content: str = "CONTENT")
 
 class ScriptedLLM:
     """Canned stream sequences; records every ``chat_stream`` request so
-    the tests can assert on the messages and the ``tools`` passthrough."""
+    the tests can assert on the messages and the ``tools`` passthrough.
+    Phase 71: when the caller passes a ``ScaffoldingFilter``, the canned
+    content pieces are fed through it exactly like
+    ``LLMClient.chat_stream`` (an empty clean result yields nothing; the
+    held tail is flushed on normal completion) — so a scaffolding-only
+    canned round streams no content pieces and leaves ``stripped_chars``
+    behind for the recovery policy to key on."""
 
     def __init__(self, *streams: list[StreamPiece | ToolCallPiece]) -> None:
         self.streams: list[list[StreamPiece | ToolCallPiece]] = list(streams)
@@ -78,12 +88,26 @@ class ScriptedLLM:
         self,
         messages: list[dict[str, str]],
         tools: list[dict[str, Any]] | None = None,
+        scaffolding: ScaffoldingFilter | None = None,
     ) -> AsyncIterator[StreamPiece | ToolCallPiece]:
         self.requests.append((deepcopy(messages), tools))
         if not self.streams:
             raise AssertionError("ScriptedLLM ran out of canned streams")
-        for piece in self.streams.pop(0):
-            yield piece
+        pieces = self.streams.pop(0)
+        if scaffolding is None:
+            for piece in pieces:
+                yield piece
+            return
+        for piece in pieces:
+            if isinstance(piece, StreamPiece) and piece.kind == "content":
+                cleaned = scaffolding.feed(piece.text)
+                if cleaned:
+                    yield StreamPiece("content", cleaned)
+            else:
+                yield piece
+        tail = scaffolding.flush()
+        if tail:
+            yield StreamPiece("content", tail)
 
 
 async def _run(
@@ -1102,6 +1126,7 @@ class FailingLLM:
         self,
         messages: list[dict[str, str]],
         tools: list[dict[str, Any]] | None = None,
+        scaffolding: ScaffoldingFilter | None = None,  # phase 71: fed like the real client
     ) -> AsyncIterator[StreamPiece | ToolCallPiece]:
         index = len(self.requests)
         pieces, error = (
@@ -1112,19 +1137,35 @@ class FailingLLM:
         self.requests.append(
             (deepcopy(messages), deepcopy(tools) if tools is not None else None)
         )
-        return self._attempt(index, pieces, error)
+        return self._attempt(index, pieces, error, scaffolding)
 
     async def _attempt(
         self,
         index: int,
         pieces: list[StreamPiece | ToolCallPiece],
         error: Exception | None,
+        scaffolding: ScaffoldingFilter | None,
     ) -> AsyncIterator[StreamPiece | ToolCallPiece]:
         try:
             for piece in pieces:
-                yield piece
+                if (
+                    scaffolding is not None
+                    and isinstance(piece, StreamPiece)
+                    and piece.kind == "content"
+                ):
+                    cleaned = scaffolding.feed(piece.text)
+                    if cleaned:
+                        yield StreamPiece("content", cleaned)
+                else:
+                    yield piece
             if error is not None:
+                # The tail is NOT flushed on a failed attempt — the real
+                # client only flushes a cleanly completed stream.
                 raise error
+            if scaffolding is not None:
+                tail = scaffolding.flush()
+                if tail:
+                    yield StreamPiece("content", tail)
         finally:
             self.closed.append(index)
 
@@ -1402,11 +1443,16 @@ def test_high_prompt_tools_section_with_notes_and_kb() -> None:
 
 
 def test_low_prompt_is_byte_identical_and_tool_free() -> None:
+    # Phase 71: the LOW prompt carries the owner-permitted plain-text
+    # line after the DEFLECT_MODE sentence (the marker-keying contract
+    # is unchanged; the line must not leak into the HIGH prompt —
+    # pinned in tests/unit/test_prompts.py).
     expected = (
         _base("LOW")
         + "\nDEFLECT_MODE: retrieval was weak — the titles below are the closest "
         "your notes come to the question. They are titles only; do not pretend "
         "they answer it. Use them to propose 2-3 alternative questions.\n"
+        "Reply in plain text only — you have no tools in this mode.\n"
         + "- T1\n- T2"
     )
     assert build_deflect_prompt(["T1", "T2"]) == expected
@@ -1418,3 +1464,253 @@ def test_low_prompt_is_byte_identical_and_tool_free() -> None:
     ):
         assert "<tools>" not in prompt
         assert TOOLS_SECTION not in prompt
+
+
+# ---------- phase 71: the scaffolding recovery policy (deterministic only) ----------
+
+#: The raw span from the 2026-09-03 incident (the E2E mock's trigger,
+#: task 05) — a complete span the filter strips in full.
+_INCIDENT_SPAN = (
+    "<|tool_call_start|>[read(path='/homelab/backup-notes.md')]<|tool_call_end|>"
+)
+
+
+def test_correction_instruction_is_the_harness_constant() -> None:
+    """Verbatim constant: the E2E mock (task 05) keys on a stable
+    substring of it, so it must not drift."""
+    assert agent.CORRECTION_INSTRUCTION == (
+        "Your previous reply contained raw tool-call markup, which is not "
+        "interpreted here. Answer the user's question directly in plain "
+        "text — no tool syntax."
+    )
+
+
+def test_malformed_reply_error_subclasses_llm_error() -> None:
+    assert issubclass(MalformedReplyError, LLMError)
+    assert not issubclass(LLMError, MalformedReplyError)
+
+
+def test_scaffolding_only_round_gets_exactly_one_recovery(
+    monkeypatch: pytest.MonkeyPatch,
+    caplog: pytest.LogCaptureFixture,
+) -> None:
+    """A round whose visible content is pure scaffolding → exactly TWO
+    model requests: the normal round, then the ONE recovery —
+    ``tools=None`` with :data:`CORRECTION_INSTRUCTION` folded into the
+    original single system message (the user message stays last). The
+    clean recovery answer ends the turn, the holder is untouched by the
+    recovery, and the strip was captured in the warning log."""
+    holder = AgentHolder()
+    llm = ScriptedLLM(
+        [StreamPiece("content", _INCIDENT_SPAN)],
+        [StreamPiece("content", "The clean recovery answer.")],
+    )
+    with caplog.at_level(logging.WARNING, logger="app.agent"):
+        pieces = asyncio.run(_run(llm, holder, _settings()))
+    # Nothing of the round's scaffolding was yielded — only the recovery.
+    assert pieces == [StreamPiece("content", "The clean recovery answer.")]
+    assert len(llm.requests) == 2  # the round + the one recovery
+    # The round: the original system prompt, the tools offered.
+    assert llm.requests[0][1] == AGENT_TOOLS
+    assert llm.requests[0][0] == [
+        {"role": "system", "content": "SYSTEM_PROMPT"},
+        {"role": "user", "content": "QUESTION"},
+    ]
+    # The recovery: no tools, a SINGLE system message carrying the folded
+    # correction, the user message last.
+    assert llm.requests[1][1] is None
+    assert llm.requests[1][0] == [
+        {
+            "role": "system",
+            "content": "SYSTEM_PROMPT\n" + agent.CORRECTION_INSTRUCTION,
+        },
+        {"role": "user", "content": "QUESTION"},
+    ]
+    # The recovery is a fixed policy, not a conversation: the holder is
+    # untouched by it.
+    assert holder.read_docs == [] and holder.tool_calls == 0
+    # The turn total feeds the API layer's ``scaffold_stripped=N`` field.
+    assert holder.scaffold_stripped == len(_INCIDENT_SPAN)
+    # One strip warning per stripped span, the span truncated to 200 chars.
+    strip_logs = [
+        r
+        for r in caplog.records
+        if r.levelno == logging.WARNING and r.getMessage().startswith("agent: stripped")
+    ]
+    assert len(strip_logs) == 1
+    message = strip_logs[0].getMessage()
+    assert message.startswith(
+        f"agent: stripped {len(_INCIDENT_SPAN)} chars of tool-scaffolding in round 1:"
+    )
+    assert _INCIDENT_SPAN[:200] in message
+
+
+def test_scaffolding_twice_settles_with_malformed_reply(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """The recovery answer is scaffolding again (a second empty reply) —
+    terminal: :class:`MalformedReplyError` (an :class:`LLMError` subclass)
+    after exactly two requests — no third request, no recovery of a
+    recovery."""
+    holder = AgentHolder()
+    llm = ScriptedLLM(
+        [StreamPiece("content", _INCIDENT_SPAN)],
+        [StreamPiece("content", _INCIDENT_SPAN)],
+    )
+    with pytest.raises(MalformedReplyError) as excinfo:
+        asyncio.run(_run(llm, holder, _settings()))
+    assert isinstance(excinfo.value, LLMError)
+    assert len(llm.requests) == 2  # exactly one recovery per turn
+    assert llm.requests[1][1] is None
+    assert agent.CORRECTION_INSTRUCTION in llm.requests[1][0][0]["content"]
+    assert holder.scaffold_stripped == 2 * len(_INCIDENT_SPAN)
+
+
+def test_scaffolding_with_real_content_needs_no_recovery(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """A round with real visible content PLUS scaffolding: the clean
+    content stands — one request only, the clean remainder yielded (no
+    raw tokens), no correction in any system prompt."""
+    holder = AgentHolder()
+    llm = ScriptedLLM(
+        [
+            StreamPiece("content", "Here it is: "),
+            StreamPiece("content", _INCIDENT_SPAN),
+            StreamPiece("content", " hope that helps."),
+        ],
+    )
+    pieces = asyncio.run(_run(llm, holder, _settings()))
+    assert [p for p in pieces if isinstance(p, StreamPiece)] == [
+        StreamPiece("content", "Here it is: "),
+        StreamPiece("content", " hope that helps."),
+    ]
+    assert len(llm.requests) == 1  # no recovery
+    for messages, _tools in llm.requests:
+        assert all(agent.CORRECTION_INSTRUCTION not in m["content"] for m in messages)
+    assert holder.scaffold_stripped == len(_INCIDENT_SPAN)
+
+
+def test_clean_turn_carries_no_correction_and_no_strip(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """A clean turn: one request, no correction in any system prompt,
+    zero stripped (the log field stays 0 — uniform)."""
+    holder = AgentHolder()
+    llm = ScriptedLLM(
+        [StreamPiece("thinking", "hmm "), StreamPiece("content", "a clean answer")]
+    )
+    pieces = asyncio.run(_run(llm, holder, _settings()))
+    assert pieces == [
+        StreamPiece("thinking", "hmm "),
+        StreamPiece("content", "a clean answer"),
+    ]
+    assert len(llm.requests) == 1
+    for messages, _tools in llm.requests:
+        assert all(agent.CORRECTION_INSTRUCTION not in m["content"] for m in messages)
+    assert holder.scaffold_stripped == 0
+
+
+def test_empty_round_without_a_strip_keeps_today_behavior(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """Round content 0 with NOTHING stripped (an empty/thinking-only
+    answer) → return as today — no recovery, no error."""
+    holder = AgentHolder()
+    llm = ScriptedLLM([StreamPiece("thinking", "nothing to say, honestly")])
+    pieces = asyncio.run(_run(llm, holder, _settings()))
+    assert pieces == [StreamPiece("thinking", "nothing to say, honestly")]
+    assert len(llm.requests) == 1
+    assert holder.scaffold_stripped == 0
+
+
+def test_scaffolding_round_with_tool_calls_needs_no_recovery(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """A scaffolding-only round that ALSO carried tool calls: the tool
+    ran, and the policy keys on the no-calls exit only — no recovery (the
+    next round is a normal tools-offered round carrying the tool
+    history)."""
+    monkeypatch.setattr(agent, "list_catalog", lambda db: [("S", "a.md", "A")])
+    holder = AgentHolder()
+    llm = ScriptedLLM(
+        [
+            StreamPiece("content", _INCIDENT_SPAN),
+            ToolCallPiece(id="call_1", name="ls", arguments={}),
+        ],
+        [StreamPiece("content", "the answer after the tool")],
+    )
+    pieces = asyncio.run(_run(llm, holder, _settings()))
+    # The round's scaffolding was stripped (no raw delta), the tool frame
+    # and the next round's answer flowed on.
+    assert pieces == [
+        ToolCallPiece(id="call_1", name="ls", arguments={}),
+        StreamPiece("content", "the answer after the tool"),
+    ]
+    assert len(llm.requests) == 2
+    assert llm.requests[1][1] == AGENT_TOOLS  # a normal round, not a recovery
+    for messages, _tools in llm.requests:
+        assert all(
+            m["content"] is None or agent.CORRECTION_INSTRUCTION not in m["content"]
+            for m in messages
+        )
+    assert holder.tool_calls == 1
+    assert holder.scaffold_stripped == len(_INCIDENT_SPAN)
+
+
+def test_recovery_after_tool_rounds_keeps_the_history(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """A scaffolding-only answer round after a tool round: the recovery
+    keeps the SINGLE (folded) system message at the front and the tool
+    history intact behind it — no second system message, no duplicated
+    correction."""
+    monkeypatch.setattr(agent, "list_catalog", lambda db: [("S", "a.md", "A")])
+    holder = AgentHolder()
+    llm = ScriptedLLM(
+        [ToolCallPiece(id="call_1", name="ls", arguments={})],
+        [StreamPiece("content", _INCIDENT_SPAN)],
+        [StreamPiece("content", "recovered after a tool round")],
+    )
+    pieces = asyncio.run(_run(llm, holder, _settings()))
+    assert pieces == [
+        ToolCallPiece(id="call_1", name="ls", arguments={}),
+        StreamPiece("content", "recovered after a tool round"),
+    ]
+    assert len(llm.requests) == 3  # tool round + stripped round + recovery
+    assert llm.requests[2][1] is None
+    recovered = llm.requests[2][0]
+    assert recovered[0] == {
+        "role": "system",
+        "content": "SYSTEM_PROMPT\n" + agent.CORRECTION_INSTRUCTION,
+    }
+    assert recovered[1] == {"role": "user", "content": "QUESTION"}
+    assert len(recovered) == 4
+    assert recovered[2]["role"] == "assistant"
+    assert recovered[3] == {
+        "role": "tool",
+        "tool_call_id": "call_1",
+        "content": "1 documents:\nsource: S | path: a.md | title: A",
+    }
+    assert sum(1 for m in recovered if m["role"] == "system") == 1
+    assert holder.scaffold_stripped == len(_INCIDENT_SPAN)
+
+
+def test_forced_final_scaffolding_only_settles_malformed(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """The round-cap forced final (``tools=None``) is filtered too: a
+    scaffolding-only forced answer never reaches the user raw — the turn
+    settles with :class:`MalformedReplyError` (the same terminal
+    semantics; this turn used no recovery, so nothing is doubled up)."""
+    monkeypatch.setattr(agent, "list_catalog", lambda db: [("S", "a.md", "A")])
+    holder = AgentHolder()
+    llm = ScriptedLLM(
+        [ToolCallPiece(id="call_1", name="ls", arguments={})],
+        [StreamPiece("content", _INCIDENT_SPAN)],
+    )
+    with pytest.raises(MalformedReplyError):
+        asyncio.run(_run(llm, holder, _settings(agent_max_rounds=1)))
+    assert len(llm.requests) == 2
+    assert llm.requests[1][1] is None  # the forced final — no recovery after it
+    assert holder.scaffold_stripped == len(_INCIDENT_SPAN)