mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
fix(metacog): cleanup batch — share watch-key constant, sanitize metadata, drop tombstones
Closes round-1 review findings q-2 (minor), q-5 (minor), q-6 (nit), q-7
(nit), sec-1 (nit), perf-4 (nit).
* **q-5:** Export ``_WATCH_REMINDER_OPTIONAL_KEYS`` from
``turnstone/core/watch.py`` and import in the dispatch closure
(session.py) and the replay filter (server.py:_build_history). The
three-place duplication of the literal tuple
``("watch_name", "command", "poll_count", "max_polls", "is_final")``
is gone; future field adds touch one constant.
* **sec-1:** Run ``sanitize_payload`` over string-typed metadata fields
(``watch_name`` / ``command``) before they enter the queue. Today's
consumers all use ``textContent``, but the asymmetry — sanitised
``text`` alongside unsanitised metadata — would survive forever in
DB rows and resurface if a future consumer used a non-textContent
sink (aria-label, copy-to-clipboard, markdown render).
* **q-7:** Drop the per-iteration ``isinstance(reminder, dict)`` from
the dispatch closure's metadata comprehension. By the time the
block runs, ``text = reminder.get("text", "") if isinstance(...)``
+ the ``if not sanitized: return`` guard above already established
``reminder`` is a non-empty dict.
* **q-2:** Strip tombstone-style references — "post-#482", "post-#484",
"Step 7 of the watch-card UX plan", "Post-Step-7 dispatch surface",
and the brittle line-anchor "session.py:2685-2686" — across
``session.py``, ``test_session.py``, ``test_watch.py``,
``test_watch_dispatch.py``, ``test_watch_integration.py``. Comment
intent preserved; historical anchors gone.
* **q-6:** Drop the ``del source`` line in ``cli.py``'s
``on_user_reminder``; the parallel ``on_tool_reminder`` ignores
``tool_call_id`` without ``del`` and the comment alone is enough.
* **perf-4:** Document the SQLite ``render_as_batch=True`` recreate
cost in migration 050's docstring — first deployment after upgrade
copies the conversations table twice (one per ``add_column``).
PostgreSQL is unaffected.
5734 non-live tests pass; ruff + mypy clean.
(cherry picked from commit 7e35050b68)
This commit is contained in:
@@ -3191,11 +3191,10 @@ class TestDeliverWakeNudge:
|
||||
assert session._wake_source_tag == ""
|
||||
|
||||
def test_wake_row_persists_with_source_column(self, tmp_db):
|
||||
"""The wake's synthesised empty user turn now persists with
|
||||
``_source = "system_nudge"`` (post-#484 the skip at
|
||||
``session.py:2685-2686`` is dropped). Without persistence,
|
||||
a second tab connecting via /history would see the assistant
|
||||
turn with no preceding wake context.
|
||||
"""The wake's synthesised empty user turn persists with
|
||||
``_source = "system_nudge"``. Without persistence, a second
|
||||
tab connecting via /history would see the assistant turn with
|
||||
no preceding wake context.
|
||||
"""
|
||||
from turnstone.core.storage import get_storage
|
||||
|
||||
|
||||
+2
-2
@@ -507,8 +507,8 @@ class TestWatchRunner:
|
||||
runner.set_dispatch_fn("ws-1", fn1)
|
||||
runner.set_dispatch_fn("ws-2", fn2)
|
||||
|
||||
# Post-Step-7 dispatch surface: ``_dispatch_result`` takes a
|
||||
# structured reminder dict, not a bare string.
|
||||
# ``_dispatch_result`` takes a structured reminder dict, not a
|
||||
# bare string.
|
||||
reminder1 = {"type": "watch_triggered", "text": "msg1"}
|
||||
runner._dispatch_result("ws-1", reminder1, "watch-a")
|
||||
fn1.assert_called_once_with(reminder1, "watch-a")
|
||||
|
||||
@@ -1,8 +1,8 @@
|
||||
"""Tests for the watch dispatch closure built inside ``set_watch_runner``.
|
||||
|
||||
The closure routes watch results onto the per-session :class:`NudgeQueue`
|
||||
under the unified pull-model surface (post-#482). Each test focuses on
|
||||
one assertion: enqueue shape, sanitisation, soft-cap drop-oldest,
|
||||
under the unified pull-model surface. Each test focuses on one
|
||||
assertion: enqueue shape, sanitisation, soft-cap drop-oldest,
|
||||
``valid_until`` predicate, and concurrent-enqueue safety.
|
||||
|
||||
Tests in this file replace the pre-switchover suite that pinned the
|
||||
@@ -357,11 +357,11 @@ def test_dispatch_no_op_for_empty_payloads(tmp_db, payload: str):
|
||||
|
||||
|
||||
class TestMetadataPropagation:
|
||||
"""Step 7 of the watch-card UX plan: the dispatch closure pulls
|
||||
optional fields out of the structured ``reminder`` dict and
|
||||
attaches them to the queue entry's ``metadata``. Drain seams later
|
||||
merge ``metadata`` into the rendered reminder dict so the frontend
|
||||
can display a structured ``.msg.watch-result`` card.
|
||||
"""The dispatch closure pulls optional fields out of the structured
|
||||
``reminder`` dict and attaches them to the queue entry's
|
||||
``metadata``. Drain seams later merge ``metadata`` into the
|
||||
rendered reminder dict so the frontend can display a structured
|
||||
``.msg.watch-result`` card.
|
||||
"""
|
||||
|
||||
def test_dispatch_attaches_watch_metadata_on_enqueue(self, tmp_db):
|
||||
|
||||
@@ -248,8 +248,8 @@ def test_watch_dispatch_through_restore_fn_lands_on_rehydrated_session(tmp_db, m
|
||||
assert runner.get_dispatch_fn(original_ws_id) is None
|
||||
|
||||
# Stage 3 — fire a watch result. ``_dispatch_result`` should fall
|
||||
# through to the restore branch. Post-Step-7 dispatch surface uses
|
||||
# a structured reminder dict.
|
||||
# through to the restore branch. The dispatch surface takes a
|
||||
# structured reminder dict.
|
||||
runner._dispatch_result(
|
||||
original_ws_id,
|
||||
{"type": "watch_triggered", "text": "post-restore body"},
|
||||
|
||||
@@ -329,7 +329,6 @@ class TerminalUI(SessionUI):
|
||||
def on_user_reminder(self, reminders: list[dict[str, Any]], source: str | None = None) -> None:
|
||||
# ``source`` ignored — the CLI doesn't render a wake marker
|
||||
# (terminal output is anchored by sequence, not anchor element).
|
||||
del source
|
||||
self._print_reminder(reminders)
|
||||
|
||||
def on_tool_reminder(self, reminders: list[dict[str, Any]], tool_call_id: str) -> None:
|
||||
|
||||
@@ -1398,7 +1398,7 @@ class ChatSession:
|
||||
"""Inject the server-level WatchRunner and register a dispatch fn
|
||||
that routes watch results onto this session's NudgeQueue.
|
||||
|
||||
The dispatch closure is the post-#482 unified path: each watch
|
||||
The dispatch closure is the unified pull-model path: each watch
|
||||
fire enqueues a ``"watch_triggered"`` entry on ``"any"`` channel,
|
||||
and the existing drain seams (USER_DRAIN, TOOL_DRAIN,
|
||||
``IdleNudgeWatcher`` IDLE wake) splice it into a
|
||||
@@ -1457,10 +1457,15 @@ class ChatSession:
|
||||
except Exception:
|
||||
return False
|
||||
|
||||
from turnstone.core.watch import _WATCH_REMINDER_OPTIONAL_KEYS
|
||||
|
||||
def _maybe_sanitize(v: Any) -> Any:
|
||||
return sanitize_payload(v) if isinstance(v, str) else v
|
||||
|
||||
metadata = {
|
||||
k: reminder[k]
|
||||
for k in ("watch_name", "command", "poll_count", "max_polls", "is_final")
|
||||
if isinstance(reminder, dict) and k in reminder
|
||||
k: _maybe_sanitize(reminder[k])
|
||||
for k in _WATCH_REMINDER_OPTIONAL_KEYS
|
||||
if k in reminder
|
||||
}
|
||||
nudge_queue.enqueue(
|
||||
"watch_triggered",
|
||||
|
||||
@@ -14,6 +14,13 @@ at save time) and any preceding tab's reminder bubbles missing as well.
|
||||
``{type, text, ...optional}``. Empty / missing column means no
|
||||
reminders for that row.
|
||||
|
||||
**SQLite upgrade cost.** Alembic env.py runs migrations with
|
||||
``render_as_batch=True``, which on SQLite implements ``add_column`` by
|
||||
recreating the table. Two ``add_column`` calls = two full-table
|
||||
copies on first deployment after upgrade. For installs with months
|
||||
of chat history (millions of rows) the migration takes seconds to
|
||||
minutes. PostgreSQL is unaffected (metadata-only ALTER).
|
||||
|
||||
Revision ID: 050
|
||||
Revises: 049
|
||||
Create Date: 2026-05-06
|
||||
|
||||
@@ -195,6 +195,15 @@ def format_watch_message(
|
||||
return "\n".join(lines)
|
||||
|
||||
|
||||
_WATCH_REMINDER_OPTIONAL_KEYS = (
|
||||
"watch_name",
|
||||
"command",
|
||||
"poll_count",
|
||||
"max_polls",
|
||||
"is_final",
|
||||
)
|
||||
|
||||
|
||||
def build_watch_reminder(
|
||||
name: str,
|
||||
command: str,
|
||||
|
||||
+2
-8
@@ -92,6 +92,7 @@ from turnstone.core.session_ui_base import (
|
||||
fire_judge_verdict_metric,
|
||||
)
|
||||
from turnstone.core.tools import TOOLS # noqa: F401 — available for introspection
|
||||
from turnstone.core.watch import _WATCH_REMINDER_OPTIONAL_KEYS
|
||||
from turnstone.core.web_helpers import version_html as _version_html
|
||||
from turnstone.core.workstream import (
|
||||
Workstream,
|
||||
@@ -549,14 +550,7 @@ def _build_history(
|
||||
if not rtype and not rtext:
|
||||
continue
|
||||
clean: dict[str, Any] = {"type": rtype, "text": rtext}
|
||||
# Preserve watch-card optional fields verbatim.
|
||||
for opt_key in (
|
||||
"watch_name",
|
||||
"command",
|
||||
"poll_count",
|
||||
"max_polls",
|
||||
"is_final",
|
||||
):
|
||||
for opt_key in _WATCH_REMINDER_OPTIONAL_KEYS:
|
||||
if opt_key in r:
|
||||
clean[opt_key] = r[opt_key]
|
||||
clean_reminders.append(clean)
|
||||
|
||||
Reference in New Issue
Block a user