mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
dc35cbc7bf
Reverses the seam-2-only design from the prior commits on this branch.
Queued user messages arriving DURING a tool batch (Seam 1) splice into
the last tool result's envelope as ``UserInterjection`` advisories via
``wrap_tool_result``. Messages arriving BETWEEN turns (Seam 2) drain
as a single trailing user row via ``_flush_queued_messages`` with
``user_feedback`` (operator text alongside an approval, e.g. "y, use
full path") folded in as a prefix. Cancel/exception drains (Seam 3)
keep the existing ``_flush_queued_messages()`` call unchanged.
Why all three seams:
* Strict-template providers (Mistral, Llama via vLLM with stock chat
templates) reject role-alternation violations. A literal ``user``
row mid-tool-batch breaks ``assistant(tool_calls) → tool → ... →
assistant``; back-to-back ``user → user`` rows on the wire also fail.
* The seam-2-only design produced back-to-back ``user`` whenever
``user_feedback`` and queued items both fired — bug-1 from the round-1
review. Folding ``user_feedback`` as a prefix to the queue-drain
collapses the two into one row.
* During-batch arrivals couldn't ride seam 2 — the splice was the only
way to deliver same-turn without violating role alternation.
Storage symmetry:
Tool DB rows now store the wrapped ``output`` (envelope + advisories)
unconditionally — ``self.messages[i]['content']`` and
``conversations.content`` match exactly. List-typed output (image /
structured MCP results) uses ``wrap_tool_result(raw_joined_text,
advisories)`` at save time so the persisted string is anchored on
``<tool_output>\n`` for the replay parser. ``TOOL_RESULT_STORAGE_CAP``
is removed entirely; tools are responsible for bounding their own
output, storage faithfully represents in-memory. Removing the cap
also simplifies the parser — no truncated-envelope edge case.
Replay extraction:
``decorate_history_messages`` (REST ``/history``) and ``_build_history``
(SSE replay, resume, rewind, retry, post-load, rename re-replay) both
call the public ``extract_advisories_from_tool_envelope`` helper to
pull the envelope back into structured ``advisories`` for JS replay.
Both string content and list-typed content (image+queued-message
combo) covered. JS renders extracted advisories as normal user
bubbles after the tool block via the shared ``replayAdvisoriesAfterTool``
helper in ``shared_static/utils.js``.
Wrapper-tag escape and provider splice:
``escape_wrapper_tags`` now encodes pre-existing ``&`` first using an
``&`` sentinel so tool output containing literal entity strings
(documentation viewers, code analyzers, web scrapers returning entity-
encoded markup) round-trips correctly. Both encode and decode helpers
short-circuit on absence of ``<`` / ``&``.
``_apply_reminders_for_provider`` detects already-wrapped content
(string body and list text-part) by ``startswith("<tool_output>\n")``
and skips re-escape so existing envelopes survive intact when a tool
message also carries ``_reminders`` (the queued-message + tool-error
co-occurrence case is now common).
``decorate_history_messages`` runs in ``asyncio.to_thread`` to keep
MB-scale string work off the event loop.
Other cleanup:
* ``_collect_advisories`` delegates the queue drain to a named helper
``_drain_queued_messages_to_advisories`` so the swap-and-clear pattern
lives next to ``_flush_queued_messages``'s identical pattern and the
side-effect is documented at the call site.
* Preamble strings + body marker for ``UserInterjection`` round-trip
detection moved to module-level constants in ``tool_advisory.py``;
imported by ``history_decoration.py`` so a producer-side rephrase
can't silently desync the parser.
* ``_send_with_mocks`` ctxmgr extracted in ``test_session.py`` — the
six new send-driven tests share an 8-deep ``patch.object`` block.
* ``replayAdvisoriesAfterTool`` shared helper in
``shared_static/utils.js``; ``app.js`` and ``coordinator.js`` both
invoke it.
* Dead truncation-pill CSS removed (``.tool-output-truncated`` and
``.coord-tool-truncated``); the JS that added these elements went
away with ``TOOL_RESULT_STORAGE_CAP``.
* Tautological tests (``TestBuildHistoryAdvisoryPropagation``)
replaced with production-realistic round-trip tests built from
``wrap_tool_result(...)`` envelopes — REST and SSE-replay surfaces
pinned to the same wire shape; full DB round-trip pinned end-to-end.
Negative-tested:
* Reverting the prefix-merge in ``_flush_queued_messages`` produces
back-to-back ``user`` rows, breaking
``test_user_feedback_and_queued_coexistence_single_row_with_prefix``.
* Reverting the ``extract_advisories_from_tool_envelope`` call in
``_build_history``'s tool branch leaves the envelope verbatim in
wire content, breaking the round-trip tests.
* Reverting the wrapper-detection in ``_apply_reminders_for_provider``
entity-encodes the existing envelope's literal tags, breaking both
the string-content and list-content envelope-preservation tests.
* Reverting the ``wrap_tool_result(raw_text, advisories)`` projection
at the DB save site produces a string starting with the original
raw text, breaking
``test_tool_db_row_round_trips_list_output_with_advisories``.
Tests: 5918 passed, 3 deselected. Lint + format + mypy clean on
touched files.
(cherry picked from commit eca4bb79e4)
345 lines
17 KiB
Python
345 lines
17 KiB
Python
"""Tests for the /coordinator/{ws_id} HTML page handler.
|
|
|
|
The handler serves the shared template with the ws_id injected as a
|
|
``data-ws-id`` attribute. It does NOT enforce auth on the page itself —
|
|
auth gating happens on the API endpoints the page calls (an unauthenticated
|
|
visitor lands on the page but all API calls fail).
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import pytest
|
|
from starlette.applications import Starlette
|
|
from starlette.routing import Route
|
|
from starlette.testclient import TestClient
|
|
|
|
from turnstone.console.server import coordinator_page
|
|
|
|
|
|
@pytest.fixture
|
|
def client():
|
|
app = Starlette(routes=[Route("/coordinator/{ws_id}", coordinator_page, methods=["GET"])])
|
|
return TestClient(app)
|
|
|
|
|
|
def test_valid_ws_id_injects_data_attr(client):
|
|
ws_id = "a" * 32
|
|
resp = client.get(f"/coordinator/{ws_id}")
|
|
assert resp.status_code == 200
|
|
assert "text/html" in resp.headers["content-type"]
|
|
body = resp.text
|
|
# ws_id is injected into the html data-ws-id attribute.
|
|
assert f'data-ws-id="{ws_id}"' in body
|
|
# Template placeholder is fully substituted.
|
|
assert "{{WS_ID}}" not in body
|
|
# Sanity: the shared static imports are wired.
|
|
assert "/shared/base.css" in body
|
|
assert "/static/coordinator/coordinator.js" in body
|
|
|
|
|
|
def test_non_hex_ws_id_returns_400(client):
|
|
"""Only hex chars are allowed to avoid HTML injection."""
|
|
resp = client.get("/coordinator/not-hex-chars-here")
|
|
assert resp.status_code == 400
|
|
|
|
|
|
def test_ws_id_too_long_returns_400(client):
|
|
resp = client.get("/coordinator/" + "a" * 65)
|
|
assert resp.status_code == 400
|
|
|
|
|
|
def test_uppercase_hex_rejected(client):
|
|
# Our ws_ids are lowercase hex; reject mixed/upper to avoid surprises.
|
|
resp = client.get("/coordinator/" + "A" * 32)
|
|
assert resp.status_code == 400
|
|
|
|
|
|
def test_coordinator_js_exposes_inline_approval_helpers():
|
|
"""Smoke guard for two layers of the coord chat frontend: the
|
|
children-tree inline approve/deny block (the original Chunk 3
|
|
landing) and the PR #447 tool-batch construct that replaced the
|
|
pinned approval dock for the coord-self surface. Both layers'
|
|
helper symbols must remain reachable in the served JS so a
|
|
refactor that accidentally renames or removes them surfaces here
|
|
instead of in production where the affected gates silently stop
|
|
rendering. Asserts string presence only — no DOM parsing —
|
|
since coord.js has no JS test framework today (per the plan's
|
|
testing notes)."""
|
|
from pathlib import Path
|
|
|
|
coord_js = Path(__file__).resolve().parent.parent / (
|
|
"turnstone/console/static/coordinator/coordinator.js"
|
|
)
|
|
body = coord_js.read_text(encoding="utf-8")
|
|
# Approval-block rendering helpers
|
|
assert "function renderApprovalBlock" in body
|
|
assert "function _maxSeverityItem" in body
|
|
assert "function _renderSubItem" in body
|
|
# The submit + 409 race-handling path
|
|
assert "function submitChildApproval" in body or "submitChildApproval(" in body
|
|
# The shared approve POST helper (parameterized for child ws_ids)
|
|
assert "function approveWorkstream" in body or "approveWorkstream(" in body
|
|
# The 409 stale-call_id retry path uses invalidateLiveBadge +
|
|
# scheduleLiveFetch (Stage 3 cleanup removed the urgent flag —
|
|
# cache invalidation makes the TTL gate fall through naturally).
|
|
assert "invalidateLiveBadge(targetWsId)" in body
|
|
# Server-side payload field — drift here means the JS reads stale keys
|
|
assert "pending_approval_detail" in body
|
|
# Reconnect parity (chunk 4): the SSE re-open handler must drop
|
|
# non-permanent entries from the live-badge cache so a stale
|
|
# pending_approval_detail (left from before the disconnect)
|
|
# can't render zombie approve/deny buttons on a row whose
|
|
# approval was resolved during the gap. The implementation
|
|
# iterates the cache and deletes only !permanent entries —
|
|
# asserting the literal helper call keeps a refactor back to
|
|
# _liveBadgeCacheClear() (which would re-pay 403s on every
|
|
# reconnect for denied ids) from sneaking in.
|
|
assert "_liveBadgeCacheDelete" in body
|
|
# Edge-case matrix sentinel labels — POLICY-BLOCKED renders when
|
|
# an item has error set + needs_approval=False (server-side
|
|
# tool policy already blocked the call); "(judge unavailable)"
|
|
# renders when no verdict (judge or heuristic) and no
|
|
# judge_pending. Refactors that drop either branch silently
|
|
# regress to a buttoned approve UI on the wrong state.
|
|
assert "POLICY-BLOCKED" in body
|
|
assert "judge unavailable" in body
|
|
# Critical-risk handling — bug-1 was that risk_level='critical'
|
|
# rendered as low because RISK_SEVERITY only mapped 'crit'.
|
|
# Both aliases must remain in the table so a 'critical' verdict
|
|
# ranks at 3 and renders with the .risk.crit pill.
|
|
assert "critical: 3" in body
|
|
# Child approves must round-trip through the routing proxy at
|
|
# /v1/api/route/workstreams/{ws_id}/approve — the bare
|
|
# /v1/api/workstreams/.../approve path only works for the
|
|
# coord-self ws_id (the coord lives on the console process).
|
|
# Children live on cluster nodes and 404 without the prefix.
|
|
assert "/v1/api/route/workstreams/" in body
|
|
# Late-arriving LLM judge verdicts — Stage 3 Step 5 promoted
|
|
# ``intent_verdict`` and ``approval_resolved`` to first-class
|
|
# cluster-bus event types, so the coord adapter dispatches them
|
|
# as ``child_ws_intent_verdict`` / ``child_ws_approval_resolved``
|
|
# on the parent's SSE stream. The browser handlers write
|
|
# directly to liveBadgeCache (bypassing scheduleLiveFetch's
|
|
# visibility gate cleanly) so off-screen rows pick up verdicts
|
|
# without polling. Replaced the old ``_judgePollTick`` 90-second
|
|
# global poll loop and its visibility-gate-bypass workaround.
|
|
assert "handleChildIntentVerdict" in body
|
|
assert "handleChildApprovalResolved" in body
|
|
assert "child_ws_intent_verdict" in body
|
|
assert "child_ws_approval_resolved" in body
|
|
# Reload parity for the coord-self approval gate: init() must
|
|
# consume the authoritative GET /workstreams snapshot's
|
|
# pending_approval_detail so a freshly opened tab can render
|
|
# Approve/Deny before SSE replay arrives.
|
|
assert "wsSnapshot.pending_approval_detail" in body
|
|
assert "appendToolBatch(pendingDetail.items" in body
|
|
# Tool-batch construct (PR #447) — the inline replacement for the
|
|
# pinned approval-dock pattern. These helpers carry the
|
|
# state-machine that pairs each tool call with its result and
|
|
# embeds the approval flow. Refactors that rename or drop them
|
|
# silently regress the entire coord-self approval surface — the
|
|
# most novel and risky behavior in the PR.
|
|
assert "function appendToolBatch" in body
|
|
assert "function _morphBatchResolved" in body
|
|
assert "function _resolveBatchAction" in body
|
|
assert "function _refreshBatchTier" in body
|
|
assert "function _refreshRowStatus" in body
|
|
# State modifiers driven by the upgrade-in-place path
|
|
# (--running orphan promoted to --pending or --auto when SSE
|
|
# arrives with the authoritative shape). Both class names must
|
|
# remain reachable from JS — dropping either breaks the reload
|
|
# state machine that PR #447's review pass surfaced.
|
|
assert "coord-tool-batch--running" in body
|
|
assert "coord-tool-batch--pending" in body
|
|
# History replay's outcome classifier — denied / errored tool
|
|
# turns must render with the correct batch state on reload, not
|
|
# the contradictory "✓ approved" pill that pre-fix showed for
|
|
# any prior denial. bug-1 / bug-3 from the second /review pass.
|
|
assert "Denied by user" in body
|
|
assert "callOutcomes" in body
|
|
# User-message attachment pills — both live send (coordSend) and
|
|
# history replay route through appendUserMessageWithAttachments.
|
|
# Renaming or dropping the helper would silently regress the
|
|
# attachment affordance to the pre-fix plain-text bubble, which
|
|
# would only surface in manual testing of an attached-file flow.
|
|
# The CSS class is the visual anchor (coordinator.css) — keeping
|
|
# both literals in the smoke layer covers JS↔CSS drift in either
|
|
# 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():
|
|
"""Stage 3 cleanup — ``pending_approval_detail`` is no longer
|
|
piggybacked on child_ws_state events. Approval items now arrive
|
|
via bulk fetch on the activity_state="approval" transition;
|
|
verdicts via the explicit ``child_ws_intent_verdict`` event class;
|
|
resolution via ``child_ws_approval_resolved``. A refactor that
|
|
re-introduces the piggyback would silently re-open the
|
|
duplicate-path race the dedicated event classes were added to
|
|
eliminate.
|
|
|
|
Structural assertions (regex against multi-line source) — symbol-
|
|
presence alone wouldn't catch a guard that keeps the names but
|
|
inverts the comparison or drops the ``prev.live`` check. This
|
|
codebase has no JS test framework, so locking the guard's shape
|
|
here is the next-best thing to a behavioral test."""
|
|
import re
|
|
from pathlib import Path
|
|
|
|
coord_js = Path(__file__).resolve().parent.parent / (
|
|
"turnstone/console/static/coordinator/coordinator.js"
|
|
)
|
|
body = coord_js.read_text(encoding="utf-8")
|
|
|
|
# The piggyback read is gone from handleChildState. (The string
|
|
# may still appear elsewhere — e.g. handleChildIntentVerdict
|
|
# reading from cache, or comments — but never as ``ev.pending_approval_detail``.)
|
|
assert "ev.pending_approval_detail" not in body
|
|
# The pre-fix urgent-fetch on activity_state transitions is gone.
|
|
assert "enteredApproval" not in body
|
|
assert "leftApproval" not in body
|
|
# ``pendingApproval`` flag derivation must check BOTH state and
|
|
# activity_state. The worker thread can fire the state transition
|
|
# to "attention" before approve_tools updates activity_state, so
|
|
# checking only activity_state misses children that legitimately
|
|
# need approval. Pin the disjunction so the regression doesn't
|
|
# silently re-introduce.
|
|
assert re.search(
|
|
r'existing\.state\s*===\s*"attention"\s*\|\|\s*'
|
|
r'existing\.activity_state\s*===\s*"approval"',
|
|
body,
|
|
), (
|
|
"handleChildState must derive pendingApproval from "
|
|
"(state==='attention' || activity_state==='approval')"
|
|
)
|
|
|
|
# SSE-authoritative window constant is defined and used.
|
|
assert re.search(r"\bconst\s+SSE_AUTHORITATIVE_MS\s*=\s*\d+", body), (
|
|
"SSE_AUTHORITATIVE_MS constant must be defined as a numeric literal"
|
|
)
|
|
|
|
# SSE writers tag entries with sseUpdatedAt: Date.now() so the
|
|
# merge guard in flushLiveFetches preserves them against stale
|
|
# bulk-fetch responses. handleChildState only stamps when it
|
|
# AUTHORITATIVELY clears the detail (off-approval transition);
|
|
# writers that stamp unconditionally are intent_verdict (verdict
|
|
# stamp), approval_resolved (clear), and the optimistic-clear
|
|
# path in submitChildApproval. Pinning the literal Date.now()
|
|
# call keeps a refactor that drops the SSE-source tag entirely
|
|
# from sneaking in.
|
|
assert re.search(
|
|
r"sseUpdatedAt:\s*Date\.now\(\)",
|
|
body,
|
|
), "Critical SSE writers must stamp sseUpdatedAt: Date.now()"
|
|
|
|
# flushLiveFetches' merge guard structure: SSE-set pending_approval
|
|
# / _detail wins over a stale bulk-poll snapshot when (live) AND
|
|
# (prev exists) AND (prev.sseUpdatedAt set) AND (within window)
|
|
# AND (prev.live exists). Inverting the comparison or dropping
|
|
# any of these guards reopens the clobber bug.
|
|
merge_guard = re.search(
|
|
r"if\s*\(\s*live\s*&&\s*prev\s*&&\s*prev\.sseUpdatedAt\s*&&\s*"
|
|
r"now\s*-\s*prev\.sseUpdatedAt\s*<\s*SSE_AUTHORITATIVE_MS\s*&&\s*"
|
|
r"prev\.live\s*\)",
|
|
body,
|
|
)
|
|
assert merge_guard is not None, (
|
|
"flushLiveFetches merge guard must be the conjunction "
|
|
"(live && prev && prev.sseUpdatedAt && now - prev.sseUpdatedAt < "
|
|
"SSE_AUTHORITATIVE_MS && prev.live). An inverted comparison or "
|
|
"missing prev.live check would let a stale bulk-poll clobber a "
|
|
"fresh SSE-set approval."
|
|
)
|
|
|
|
# The merge body must preserve BOTH pending_approval and
|
|
# pending_approval_detail from prev — preserving only one would
|
|
# render a row with a phantom badge but no buttons (or vice versa).
|
|
merge_body = re.search(
|
|
r"mergedLive\s*=\s*Object\.assign\(\s*\{\}\s*,\s*live\s*,\s*\{"
|
|
r"[^}]*pending_approval:\s*prev\.live\.pending_approval[^}]*"
|
|
r"pending_approval_detail:\s*prev\.live\.pending_approval_detail",
|
|
body,
|
|
)
|
|
assert merge_body is not None, (
|
|
"Merge body must preserve both pending_approval AND "
|
|
"pending_approval_detail from prev.live — preserving only one "
|
|
"creates a half-rendered approval row."
|
|
)
|
|
|
|
# flushLiveFetches must forward sseUpdatedAt onto the new cache
|
|
# entry so the SSE-source tag survives the bulk-poll write back —
|
|
# without this, every bulk-poll resets the window and the next
|
|
# late-arriving poll silently clobbers.
|
|
assert re.search(
|
|
r"sseUpdatedAt:\s*prev\s*\?\s*prev\.sseUpdatedAt",
|
|
body,
|
|
), (
|
|
"flushLiveFetches must forward prev.sseUpdatedAt onto the new "
|
|
"cache entry (preserving the SSE-source window across bulk-poll "
|
|
"cycles) — without this, the second bulk-poll after an SSE "
|
|
"transition silently clobbers."
|
|
)
|
|
|
|
|
|
def test_coord_history_renders_user_interjection_advisory_after_tool_block():
|
|
"""Queued user messages spliced into the last tool-result envelope
|
|
of a batch (Seam 1) persist on the tool DB row as a wrapped
|
|
``<tool_output>`` envelope. ``decorate_history_messages`` extracts
|
|
the advisory back out and the wire layer projects it onto
|
|
``m.advisories``; the coord history loop must invoke the shared
|
|
``replayAdvisoriesAfterTool`` helper (defined in
|
|
``shared/utils.js``) so each ``user_interjection`` renders through
|
|
``appendUserMessageWithAttachments`` and the bubble looks identical
|
|
to a Seam 2/3 user row.
|
|
|
|
This test pins the call site so a refactor that drops the helper
|
|
invocation regresses the queued-during-batch replay shape silently.
|
|
Mirrors ``test_app_js.py``'s same-shape pin on interactive's
|
|
``replayHistory``."""
|
|
import re
|
|
from pathlib import Path
|
|
|
|
coord_js = Path(__file__).resolve().parent.parent / (
|
|
"turnstone/console/static/coordinator/coordinator.js"
|
|
)
|
|
body = coord_js.read_text(encoding="utf-8")
|
|
|
|
assert "replayAdvisoriesAfterTool(m.advisories" in body, (
|
|
"Coord history loop must invoke replayAdvisoriesAfterTool with "
|
|
"m.advisories so queued messages spliced into the tool envelope "
|
|
"render as user bubbles after the tool block."
|
|
)
|
|
# The renderer callback routes through appendUserMessageWithAttachments
|
|
# so the bubble matches a normal user-row replay.
|
|
assert re.search(
|
|
r"appendUserMessageWithAttachments\(\s*text",
|
|
body,
|
|
), (
|
|
"Coord history loop's renderer callback must route the extracted "
|
|
"advisory text through appendUserMessageWithAttachments so the "
|
|
"rendered bubble matches a normal user-row replay."
|
|
)
|