mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-28 06:44:51 -06:00
fix(replay): apply review findings q-2 through q-7
Round-1 ``/review`` apply-pass. Drops stale ``UserInterjection`` references from comments and docstrings that no longer describe the post-PR drain shape, asserts the two-stream invariant in the new queued-message persistence test, and pins the ``content.trim()`` + ``renderAssistantToolBatch`` invariants on coord-side so a future refactor can't silently regress the Qwen3 phantom-card fix or the chronological-order render fix. Deferred: * **bug-1** (back-to-back ``user`` row when ``user_feedback`` from the approval-prompt UI callback coexists with a queued-message drain). Reachable on strict OpenAI-compatible local templates (Anthropic and Anthropic-via-merge-consecutive collapse fine; vLLM-hosted Mistral / Llama enforcing role alternation can reject). The pre-PR splice guarded against this case by riding queued items inside the tool result envelope; that guard is what motivated the original UserInterjection design, so the fix lane needs a deliberate decision rather than a quick patch. Sleeping on it. * **q-1** (delete dead ``UserInterjection`` class + tests). Held for the bug-1 decision — if the chosen fix is to resume the splice for the ``user_feedback``+queue coexistence case, the advisory shape stays load-bearing. Class now carries a docstring note marking it retained-pending-decision so a passing reader doesn't grep for producers and assume it's actually dead. Apply-pass content: * ``q-2``: drop "queued user interjections" from the persistent- advisory parenthetical in ``send``'s tool-result loop comment; rewrite to point at ``_flush_queued_messages`` for the queue path. * ``q-3``: ``__init__`` channel-routing comment loses "and ``UserInterjection``" — only ``GuardAdvisory`` remains. * ``q-4``: ``_queue_tool_advisory`` docstring + the tool-error nudge comment lose the user-interjection mentions; the docstring also now describes the side-channel + ``_apply_reminders_for_provider`` splice path (the actual mechanism). * ``q-5``: ``AttachmentsNotQueueableError`` docstring rewritten to describe the post-PR ``_flush_queued_messages`` flow — the single-combined-turn ``\n\n``-join shape can't carry image / file blocks, and per-item separate user turns would expand the strict- template role-ordering surface that the post-batch drain already balances. * ``q-6``: the new ``test_queued_message_persists_as_user_row_after_tool_batch`` in ``test_session.py`` now asserts ``stream_idx == 2`` so a future regression where the post-batch flush runs but the send-loop short- circuits before the next iteration surfaces in CI rather than manual repro. * ``q-7``: ``test_coordinator_page.py`` gets two new string-grep pins mirroring the existing ``test_app_js.py`` shape — ``content.trim()`` on coord's assistant-replay branch and ``renderAssistantToolBatch`` for the hoisted helper that orders content card before tool batch. ## Test plan - [x] ``ruff check`` clean - [x] ``mypy turnstone/`` clean (189 source files) - [x] Affected test surface (``test_session.py`` + ``test_tool_advisory.py`` + ``test_app_js.py`` + ``test_coordinator_page.py``) — 240 passed
This commit is contained in:
@@ -167,6 +167,28 @@ def test_coordinator_js_exposes_inline_approval_helpers():
|
||||
# direction.
|
||||
assert "function appendUserMessageWithAttachments" in body
|
||||
assert "msg-user-attach" in body
|
||||
# PR #487 — whitespace-only assistant content (Qwen3 with vLLM
|
||||
# ``--reasoning-parser`` strips ``<think>…</think>`` and emits only
|
||||
# ``"\n\n"`` as content before a tool call) must be skipped on
|
||||
# history replay or the empty ``.msg.assistant`` card surfaces as
|
||||
# a phantom row. The literal substring ``content.trim()`` is the
|
||||
# single-line guard the rendering branch uses; a refactor that
|
||||
# drops the trim() (e.g. simplifies to ``if (!content)``) silently
|
||||
# regresses the phantom-card fix on the multi-node coord path.
|
||||
# Mirrors ``test_app_js.py``'s same-shape pin on ``app.js``.
|
||||
assert "content.trim()" in body
|
||||
# PR #487 — coord history replay must render the assistant content
|
||||
# card BEFORE the tool batch, not after, so DOM order matches the
|
||||
# chronological order the model emitted (text → dispatch → results).
|
||||
# Pre-fix the tool_calls branch sat at the role-agnostic top of the
|
||||
# loop and rendered ahead of the assistant text that announced the
|
||||
# batch, putting parallel fan-outs visually above their narrating
|
||||
# message. The fix hoisted the synthesis into ``renderAssistantToolBatch``
|
||||
# called from inside the assistant branch AFTER the content card —
|
||||
# asserting the helper name lets a refactor that re-inlines or
|
||||
# renames it surface here instead of via manual reload testing.
|
||||
assert "function renderAssistantToolBatch" in body
|
||||
assert "renderAssistantToolBatch(m)" in body
|
||||
|
||||
|
||||
def test_coordinator_js_handle_child_state_no_longer_reads_sse_pending_approval_detail():
|
||||
|
||||
@@ -2231,6 +2231,13 @@ class TestMetacognitiveBuffers:
|
||||
)
|
||||
# Queue is empty after drain.
|
||||
assert session._queued_messages == {}
|
||||
# Two model turns: one for the tool-call iteration, one for the
|
||||
# follow-up after the post-batch flush appended the queued user
|
||||
# row. Without the second stream the model would never see /
|
||||
# respond to the queued message — pinning this guards against a
|
||||
# future regression where the post-batch flush runs but the
|
||||
# send-loop short-circuits before the next iteration.
|
||||
assert stream_idx == 2, f"expected 2 stream calls (tool-iter + follow-up), got {stream_idx}"
|
||||
|
||||
def test_start_nudge_fires_through_send(self, tmp_db):
|
||||
"""Pin the +1 count-shift invariant — `start` must still fire on the
|
||||
|
||||
+28
-25
@@ -152,14 +152,16 @@ class AttachmentsNotQueueableError(Exception):
|
||||
"""Raised by ``ChatSession.queue_message`` when called with non-empty
|
||||
``attachment_ids``.
|
||||
|
||||
Queued messages are injected at the next tool-result advisory seam
|
||||
where they ride inside the tool envelope as text-only
|
||||
``UserInterjection`` advisories. Attachments can't ride that path
|
||||
(advisories don't carry image / file blocks), so an attachment-
|
||||
bearing queued item would have to be appended as a separate
|
||||
``user`` turn — which would inject ``user`` between
|
||||
``assistant(tool_calls)`` and ``tool``, a role sequence strict
|
||||
providers (Mistral, Anthropic) reject.
|
||||
Queued messages drain via ``_flush_queued_messages`` after the tool
|
||||
batch completes, joining all queued items into a single text-only
|
||||
user turn (see the ``"\\n\\n".join(parts)`` shape). That join can't
|
||||
carry image / file blocks, so an attachment-bearing queued item
|
||||
would either be silently dropped or force a per-item separate user
|
||||
turn — the latter would inject extra ``user`` rows between the tool
|
||||
batch and the next assistant turn, expanding the strict-template
|
||||
role-ordering surface (Mistral / Anthropic) that the post-batch
|
||||
drain already balances. Rejecting attachments at queue time keeps
|
||||
the single-combined-turn invariant intact.
|
||||
|
||||
Callers surface this to the user as "wait for the current turn
|
||||
before attaching".
|
||||
@@ -838,8 +840,7 @@ class ChatSession:
|
||||
# ``_attach_pending_user_reminders`` and splice as
|
||||
# <system-reminder> blocks
|
||||
# - "tool" entries drain at the next tool-result batch via
|
||||
# ``_collect_advisories`` (alongside GuardAdvisory and
|
||||
# UserInterjection)
|
||||
# ``_collect_advisories`` (alongside ``GuardAdvisory``)
|
||||
# - "any" entries drain at whichever seam fires first; used for
|
||||
# wake-trigger-driven nudges that should not pin to a
|
||||
# specific seam
|
||||
@@ -3070,15 +3071,16 @@ class ChatSession:
|
||||
raw_output = output
|
||||
|
||||
# Advisory injection: persistent advisories (output
|
||||
# guard findings, queued user interjections) wrap
|
||||
# into the tool-result envelope and stay in
|
||||
# self.messages. Metacognitive tool-channel
|
||||
# reminders (tool_error / repeat) ride a side-channel
|
||||
# — never inside content — so the model sees the
|
||||
# splice only at the wire boundary via
|
||||
# _apply_reminders_for_provider, while UI/replay
|
||||
# guard findings) wrap into the tool-result envelope
|
||||
# and stay in self.messages. Metacognitive
|
||||
# tool-channel reminders (tool_error / repeat) ride
|
||||
# a side-channel — never inside content — so the
|
||||
# model sees the splice only at the wire boundary
|
||||
# via _apply_reminders_for_provider, while UI/replay
|
||||
# surfaces them as a themed bubble below the tool
|
||||
# result.
|
||||
# result. Queued user messages drain via
|
||||
# ``_flush_queued_messages`` AFTER this loop, so
|
||||
# they no longer ride the persistent-advisory path.
|
||||
persistent_advisories, metacog_reminders = self._collect_advisories(
|
||||
assessment, _tc_names.get(tc_id, ""), _ri == _last_idx
|
||||
)
|
||||
@@ -6135,10 +6137,11 @@ class ChatSession:
|
||||
def _queue_tool_advisory(self, nudge_type: str, text: str) -> None:
|
||||
"""Queue a metacognitive nudge for the next tool-result batch.
|
||||
|
||||
Drains in ``_collect_advisories`` alongside guard findings and
|
||||
user interjections; ``wrap_tool_result`` then renders it inside
|
||||
the tool-result envelope. Used for nudges that respond to model
|
||||
behaviour at a tool boundary: ``tool_error``, ``repeat``.
|
||||
Drains in ``_collect_advisories`` alongside guard findings, then
|
||||
rides the tool message dict's ``_reminders`` side-channel —
|
||||
spliced into wire content only at the provider boundary by
|
||||
``_apply_reminders_for_provider``. Used for nudges that respond
|
||||
to model behaviour at a tool boundary: ``tool_error``, ``repeat``.
|
||||
|
||||
No-ops while the session is inside a wake-driven turn (see
|
||||
``_queue_user_advisory`` for the rationale).
|
||||
@@ -6287,9 +6290,9 @@ class ChatSession:
|
||||
self._queue_tool_advisory("repeat", format_nudge("repeat"))
|
||||
|
||||
# Tool-error nudge — queued so the MetacognitiveAdvisory rides
|
||||
# the same _collect_advisories drain pass as guard findings and
|
||||
# user interjections. Cooldown gating in should_nudge keeps
|
||||
# this to one nudge per batch even with many failing tools.
|
||||
# the same _collect_advisories drain pass as guard findings.
|
||||
# Cooldown gating in should_nudge keeps this to one nudge per
|
||||
# batch even with many failing tools.
|
||||
if (
|
||||
self._mem_cfg.nudges
|
||||
and any(self._tool_error_flags.get(tc_id) for tc_id, _ in results)
|
||||
|
||||
@@ -66,7 +66,17 @@ class GuardAdvisory:
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class UserInterjection:
|
||||
"""Advisory for a message the user sent while the model was executing."""
|
||||
"""Advisory for a message the user sent while the model was executing.
|
||||
|
||||
Currently no production producer — pre-PR-#487 the queued-message
|
||||
drain in ``_collect_advisories`` instantiated this and rode it
|
||||
through ``wrap_tool_result``. PR #487 dropped the splice and now
|
||||
drains queued messages as a real user turn after the tool batch.
|
||||
Class + tests retained pending the back-to-back-user role-ordering
|
||||
decision (see review finding bug-1) — one viable fix path resumes
|
||||
the splice for the ``user_feedback`` + queue-drain coexistence
|
||||
case, in which case this advisory shape stays load-bearing.
|
||||
"""
|
||||
|
||||
message: str
|
||||
priority: str = PRIORITY_NOTICE
|
||||
|
||||
Reference in New Issue
Block a user