fix(migration): tighten 060 envelope match; stop re-activating escaped tags

060 un-wrapped legacy <tool_output> envelopes with a loose guard (open + close)
that could irreversibly mis-rewrite a bare tool row resembling the open, and
entity-decoded the wrapper tags back to live form — re-activating injection the
old escape had neutralised (and downgrade cannot undo it).

- Require the full legacy signature (the exact </tool_output>\n\n<system-
  reminder>\n join plus a trailing </system-reminder>), which wrap_tool_result
  only ever emitted with advisories. A bare row with a matching close but no
  advisory is left byte-for-byte untouched.
- Reverse only &amp; -> & ; leave the wrapper-tag entities escaped so a
  previously-defanged injection stays defanged.

Adds false-positive guard tests (open+close without advisory; missing tail).

tests/test_migration_060.py

@@ -3,11 +3,14 @@
 Drives ``command.upgrade`` from a programmatic Alembic config against an
 isolated SQLite database per test, then asserts:
 
-* a wrapped ``<tool_output>`` envelope row is rewritten to the bare
-  (entity-decoded) tool output, dropping the embedded ``<system-reminder>``
-  advisory blocks;
-* a row whose content merely *starts with* a literal ``<tool_output>`` line
-  but has no matching close is left untouched (structural-match guard);
+* a wrapped ``<tool_output>`` envelope row is rewritten to the bare tool
+  output, dropping the embedded ``<system-reminder>`` advisory blocks;
+* only ``&amp;`` → ``&`` is reversed — wrapper-tag entities stay escaped so a
+  previously-defanged injection is not re-activated (sec-2);
+* the tightened structural guard requires the *full* envelope signature, so a
+  bare row that merely starts with ``<tool_output>`` — or even one with a
+  matching ``</tool_output>`` close but no advisory — is left untouched (the
+  known-issue #1 false positive);
 * the ``_reminders`` side-channel column is nulled;
 * the migration is idempotent (a second run is a no-op);
 * a plain non-envelope row is untouched.
@@ -53,9 +56,11 @@ def _seed_row(conn: sa.Connection, **cols: object) -> None:
 
 
 # A wrapped envelope exactly as ``wrap_tool_result`` produced it: the
-# ``<tool_output>`` block followed by one ``<system-reminder>`` advisory.
+# ``<tool_output>`` block, then ``"\n".join`` with a part that itself begins
+# with ``\n<system-reminder>`` — yielding the ``</tool_output>\n\n<system-
+# reminder>`` double-newline join the tightened guard requires.
 _WRAPPED = (
-    "<tool_output>\nclean tool output\n</tool_output>\n"
+    "<tool_output>\nclean tool output\n</tool_output>\n\n"
     "<system-reminder>\nThe user sent a message. User message: check logs\n</system-reminder>"
 )
 
@@ -85,9 +90,10 @@ class TestMigration060:
         finally:
             engine.dispose()
 
-    def test_entity_decode_round_trips_literal_wrapper_text(self, tmp_path: Path) -> None:
-        """A tool output documenting the wrapper format escaped to entities
-        on the way in; the un-wrap decodes it back to the literal tags."""
+    def test_ampersand_decoded_but_wrapper_tags_left_escaped(self, tmp_path: Path) -> None:
+        """The un-wrap reverses only ``&amp;`` → ``&``.  Wrapper-tag entities are
+        left escaped on purpose: re-activating ``&lt;system-reminder&gt;`` into a
+        live tag would un-defang injection the old escape had neutralised."""
         db_path = tmp_path / "060-decode.db"
         cfg = _alembic_cfg(db_path)
         command.upgrade(cfg, "059")
@@ -95,7 +101,10 @@ class TestMigration060:
         # escape_wrapper_tags(``see <tool_output> & <system-reminder>``) →
         # ``&amp;`` first, then the tag escapes.
         inner_escaped = "see &lt;tool_output&gt; &amp; &lt;system-reminder&gt;"
-        wrapped = f"<tool_output>\n{inner_escaped}\n</tool_output>"
+        wrapped = (
+            f"<tool_output>\n{inner_escaped}\n</tool_output>\n\n"
+            "<system-reminder>\nThe user sent a message. User message: x\n</system-reminder>"
+        )
 
         engine = sa.create_engine(f"sqlite:///{db_path}")
         try:
@@ -108,7 +117,9 @@ class TestMigration060:
                 content = conn.execute(
                     sa.text("SELECT content FROM conversations WHERE tool_call_id = 'call_b'")
                 ).scalar_one()
-            assert content == "see <tool_output> & <system-reminder>"
+            # ``&amp;`` → ``&`` only; the wrapper-tag entities stay escaped.
+            assert content == "see &lt;tool_output&gt; & &lt;system-reminder&gt;"
+            assert "<system-reminder>" not in content
         finally:
             engine.dispose()
 
@@ -136,6 +147,59 @@ class TestMigration060:
         finally:
             engine.dispose()
 
+    def test_tool_output_open_close_without_advisory_untouched(self, tmp_path: Path) -> None:
+        """The known-issue #1 false positive: a bare tool output that genuinely
+        starts with ``<tool_output>`` AND has a matching ``</tool_output>`` close
+        but NO trailing ``<system-reminder>`` advisory is NOT a legacy envelope
+        (those were only emitted with advisories).  The tightened guard leaves
+        it byte-for-byte untouched — the loose open+close guard would have
+        irreversibly mis-rewritten it to its inner text."""
+        db_path = tmp_path / "060-noadvisory.db"
+        cfg = _alembic_cfg(db_path)
+        command.upgrade(cfg, "059")
+
+        # e.g. a tool that printed XML, or this project's own source/docs.
+        bare = "<tool_output>\nls -la output here\n</tool_output>\nplus a trailing line"
+
+        engine = sa.create_engine(f"sqlite:///{db_path}")
+        try:
+            with engine.begin() as conn:
+                _seed_row(conn, content=bare, tool_call_id="call_g")
+
+            command.upgrade(cfg, "060")
+
+            with engine.connect() as conn:
+                content = conn.execute(
+                    sa.text("SELECT content FROM conversations WHERE tool_call_id = 'call_g'")
+                ).scalar_one()
+            assert content == bare
+        finally:
+            engine.dispose()
+
+    def test_missing_trailing_system_reminder_close_untouched(self, tmp_path: Path) -> None:
+        """An open + join that lacks the trailing ``</system-reminder>`` close is
+        not a complete envelope — left untouched rather than half-rewritten."""
+        db_path = tmp_path / "060-notail.db"
+        cfg = _alembic_cfg(db_path)
+        command.upgrade(cfg, "059")
+
+        truncated = "<tool_output>\nx\n</tool_output>\n\n<system-reminder>\nno close here"
+
+        engine = sa.create_engine(f"sqlite:///{db_path}")
+        try:
+            with engine.begin() as conn:
+                _seed_row(conn, content=truncated, tool_call_id="call_h")
+
+            command.upgrade(cfg, "060")
+
+            with engine.connect() as conn:
+                content = conn.execute(
+                    sa.text("SELECT content FROM conversations WHERE tool_call_id = 'call_h'")
+                ).scalar_one()
+            assert content == truncated
+        finally:
+            engine.dispose()
+
     def test_nulls_reminders_column(self, tmp_path: Path) -> None:
         db_path = tmp_path / "060-reminders.db"
         cfg = _alembic_cfg(db_path)

turnstone/core/storage/migrations/versions/060_unwrap_tool_envelopes.py

@@ -10,14 +10,17 @@ turns, so the legacy carriers are now dead on the read path.
 This migration drains the carriers in place — UPDATE only, no row insertion:
 
 * **Envelopes** — every tool row whose ``content`` is a wrapped envelope is
-  rewritten to the bare (entity-decoded) tool output.  The embedded
-  ``<system-reminder>`` advisories are intentionally dropped (cosmetic loss of
-  historical nudge / interjection bubbles — the design accepts this rather than
-  resurrecting them as new rows).  The structural-match guard (open prefix AND a
-  matching close) is copied inline from the now-deleted
-  ``history_decoration.extract_advisories_from_tool_envelope`` so a tool output
-  that merely *starts with* a literal ``<tool_output>`` line — with no matching
-  close — is left untouched.
+  rewritten to the bare tool output.  The embedded ``<system-reminder>``
+  advisories are intentionally dropped (cosmetic loss of historical nudge /
+  interjection bubbles — the design accepts this rather than resurrecting them
+  as new rows).  The structural guard requires the *complete* legacy envelope
+  signature (``<tool_output>`` open, the exact ``</tool_output>\\n\\n<system-
+  reminder>\\n`` join, and a trailing ``</system-reminder>``) — not merely a
+  ``<tool_output>`` open + close — so a bare tool row that resembles the open
+  cannot be irreversibly mis-rewritten.  Only the ``&amp;`` → ``&`` half of the
+  original escape is reversed: re-activating the wrapper-tag escapes
+  (``&lt;system-reminder&gt;`` → ``<system-reminder>``) would un-defang
+  injection the old escape had neutralised, so those entities are left as-is.
 * **Reminders** — ``conversations._reminders`` is nulled wholesale; nothing
   writes the column anymore and the read path no longer projects it.
 
@@ -46,40 +49,63 @@ depends_on = None
 _BATCH = 500
 
 
-def _entity_decode_wrapper_tags(text: str) -> str:
-    """Reverse ``tool_advisory.escape_wrapper_tags`` (copied inline).
+def _decode_ampersand(text: str) -> str:
+    """Reverse only the ``&`` → ``&amp;`` half of ``escape_wrapper_tags``.
 
-    Decodes ``&amp;`` last so a body containing the literal string
-    ``&lt;tool_output&gt;`` round-trips identically to its source.
+    The original escape encoded ``&`` first, then the wrapper tags.  We
+    deliberately do NOT reverse the wrapper-tag half: a stored
+    ``&lt;system-reminder&gt;`` is content the old escape *neutralised* from
+    attacker tool output, and turning it back into a live ``<system-reminder>``
+    here would re-activate a previously-defanged injection in replayable
+    history (and ``downgrade`` cannot undo it).  Decoding ``&amp;`` → ``&`` is
+    safe and keeps genuine ampersands (and pre-existing ``&lt;…&gt;`` literals)
+    round-tripping; the cost is purely cosmetic — a tool output that truly
+    contained the literal text ``<tool_output>`` stays shown as the entity.
     """
-    if "&" not in text:
+    if "&amp;" not in text:
         return text
-    return (
-        text.replace("&lt;/tool_output&gt;", "</tool_output>")
-        .replace("&lt;tool_output&gt;", "<tool_output>")
-        .replace("&lt;system-reminder&gt;", "<system-reminder>")
-        .replace("&lt;/system-reminder&gt;", "</system-reminder>")
-        .replace("&amp;", "&")
-    )
+    return text.replace("&amp;", "&")
+
+
+# The legacy envelope (``tool_advisory.wrap_tool_result``) was emitted ONLY when
+# advisories were present, as ``"\n".join`` of a ``<tool_output>`` block and one
+# or more ``<system-reminder>`` blocks.  The exact bytes are therefore:
+#
+#     <tool_output>\n{escaped output}\n</tool_output>\n\n<system-reminder>\n …
+#       … \n</system-reminder>[\n\n<system-reminder>\n … \n</system-reminder>]*
+#
+# We match that full signature — open prefix AND the tool-output close followed
+# immediately by the ``\n\n<system-reminder>\n`` join AND a trailing
+# ``</system-reminder>`` — not just the ``<tool_output>`` open + close.  A bare
+# tool output that merely happens to start with ``<tool_output>`` (or even one
+# that contains a matching close) lacks the trailing advisory structure and is
+# left untouched, so a legit row can never be mis-rewritten.
+_TOOL_OPEN = "<tool_output>\n"
+_ENVELOPE_JOIN = "\n</tool_output>\n\n<system-reminder>\n"
+_ENVELOPE_TAIL = "\n</system-reminder>"
 
 
 def _unwrap_envelope(content: str) -> str | None:
-    """Return the bare tool output for a wrapped envelope, else ``None``.
+    """Return the bare tool output for a wrapped legacy envelope, else ``None``.
 
-    Structural-match guard (copied inline from the deleted
-    ``extract_advisories_from_tool_envelope``): the content must start with
-    the exact ``<tool_output>\\n`` open AND contain a matching
-    ``\\n</tool_output>`` close.  A tool output that merely starts with a
-    literal ``<tool_output>`` line but has no matching close is NOT an
-    envelope — return ``None`` so the caller leaves it untouched.
+    Requires the complete envelope signature (see the module-level constants),
+    not merely a ``<tool_output>`` open + close — the loose guard could
+    irreversibly mis-rewrite a bare tool row that resembled the open.  The
+    embedded ``<system-reminder>`` advisory blocks are intentionally dropped
+    (documented cosmetic loss); only the bare tool output is recovered.
+
+    The escape guarantees the body cannot contain a literal ``\\n</tool_output>``
+    (it was entity-encoded), so the first ``_ENVELOPE_JOIN`` is the real close.
     """
-    if not content.startswith("<tool_output>\n"):
+    if not content.startswith(_TOOL_OPEN):
         return None
-    close = content.find("\n</tool_output>")
-    if close == -1:
+    join = content.find(_ENVELOPE_JOIN)
+    if join == -1:
         return None
-    inner = content[len("<tool_output>\n") : close]
-    return _entity_decode_wrapper_tags(inner)
+    if not content.endswith(_ENVELOPE_TAIL):
+        return None
+    inner = content[len(_TOOL_OPEN) : join]
+    return _decode_ampersand(inner)
 
 
 def upgrade() -> None: