diff --git a/tests/test_migration_060.py b/tests/test_migration_060.py index 4f429565..97ec88ae 100644 --- a/tests/test_migration_060.py +++ b/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 ```` envelope row is rewritten to the bare - (entity-decoded) tool output, dropping the embedded ```` - advisory blocks; -* a row whose content merely *starts with* a literal ```` line - but has no matching close is left untouched (structural-match guard); +* a wrapped ```` envelope row is rewritten to the bare tool + output, dropping the embedded ```` advisory blocks; +* only ``&`` → ``&`` 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 ```` — or even one with a + matching ```` 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 -# ```` block followed by one ```` advisory. +# ```` block, then ``"\n".join`` with a part that itself begins +# with ``\n`` — yielding the ``\n\n`` double-newline join the tightened guard requires. _WRAPPED = ( - "\nclean tool output\n\n" + "\nclean tool output\n\n\n" "\nThe user sent a message. User message: check logs\n" ) @@ -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 ``&`` → ``&``. Wrapper-tag entities are + left escaped on purpose: re-activating ``<system-reminder>`` 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 & ``) → # ``&`` first, then the tag escapes. inner_escaped = "see <tool_output> & <system-reminder>" - wrapped = f"\n{inner_escaped}\n" + wrapped = ( + f"\n{inner_escaped}\n\n\n" + "\nThe user sent a message. User message: x\n" + ) 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 & " + # ``&`` → ``&`` only; the wrapper-tag entities stay escaped. + assert content == "see <tool_output> & <system-reminder>" + assert "" 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 ```` AND has a matching ```` close + but NO trailing ```` 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 = "\nls -la output here\n\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 ```` 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 = "\nx\n\n\n\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) diff --git a/turnstone/core/storage/migrations/versions/060_unwrap_tool_envelopes.py b/turnstone/core/storage/migrations/versions/060_unwrap_tool_envelopes.py index b92c467e..e8e0e33e 100644 --- a/turnstone/core/storage/migrations/versions/060_unwrap_tool_envelopes.py +++ b/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 - ```` 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 ```` line — with no matching - close — is left untouched. + rewritten to the bare tool output. The embedded ```` + 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 (```` open, the exact ``\\n\\n\\n`` join, and a trailing ````) — not merely a + ```` open + close — so a bare tool row that resembles the open + cannot be irreversibly mis-rewritten. Only the ``&`` → ``&`` half of the + original escape is reversed: re-activating the wrapper-tag escapes + (``<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 ``&`` → ``&`` half of ``escape_wrapper_tags``. - Decodes ``&`` last so a body containing the literal string - ``<tool_output>`` 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 + ``<system-reminder>`` is content the old escape *neutralised* from + attacker tool output, and turning it back into a live ```` + here would re-activate a previously-defanged injection in replayable + history (and ``downgrade`` cannot undo it). Decoding ``&`` → ``&`` is + safe and keeps genuine ampersands (and pre-existing ``<…>`` literals) + round-tripping; the cost is purely cosmetic — a tool output that truly + contained the literal text ```` stays shown as the entity. """ - if "&" not in text: + if "&" not in text: return text - return ( - text.replace("</tool_output>", "") - .replace("<tool_output>", "") - .replace("<system-reminder>", "") - .replace("</system-reminder>", "") - .replace("&", "&") - ) + return text.replace("&", "&") + + +# The legacy envelope (``tool_advisory.wrap_tool_result``) was emitted ONLY when +# advisories were present, as ``"\n".join`` of a ```` block and one +# or more ```` blocks. The exact bytes are therefore: +# +# \n{escaped output}\n\n\n\n … +# … \n[\n\n\n … \n]* +# +# We match that full signature — open prefix AND the tool-output close followed +# immediately by the ``\n\n\n`` join AND a trailing +# ```` — not just the ```` open + close. A bare +# tool output that merely happens to start with ```` (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 = "\n" +_ENVELOPE_JOIN = "\n\n\n\n" +_ENVELOPE_TAIL = "\n" 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 ``\\n`` open AND contain a matching - ``\\n`` close. A tool output that merely starts with a - literal ```` 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 ```` open + close — the loose guard could + irreversibly mis-rewrite a bare tool row that resembled the open. The + embedded ```` 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`` + (it was entity-encoded), so the first ``_ENVELOPE_JOIN`` is the real close. """ - if not content.startswith("\n"): + if not content.startswith(_TOOL_OPEN): return None - close = content.find("\n") - if close == -1: + join = content.find(_ENVELOPE_JOIN) + if join == -1: return None - inner = content[len("\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: