mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-25 21:34:47 -06:00
fix: pre-push review — operator-context retry walk, get_content offload, doc cleanups
- A1: add a shared `operator-context` marker to every operator-context row in
both UIs; the retry-skip walk keys on it, so a trailing watch-result /
guard-finding / idle-children card no longer makes retry regenerate the
wrong turn. Pinned by source-grep tests + a headless-DOM self-test.
- A2: wrap get_content's two sync DB gates (get_attachment + the unbounded
ws-scoped attachment_referenced_in_ws LIKE scan) in asyncio.to_thread so a
long scan can't stall the event loop — matching the module's convention.
- A3: add the HistoryEvent attachments docstring bullet; drop the dead
`interjection` class; document the interactive-only system-context label;
correct the SDK attachments-meta docs to {kind, filename, mime_type}
(size_bytes is not carried through the history projection).
This commit is contained in:
@@ -26,7 +26,7 @@ export interface HistoryEvent {
|
||||
* (e.g. `watch_triggered`'s `{watch_name, command, poll_count, max_polls,
|
||||
* is_final}`) so the renderer can rebuild per-kind UI (the watch-result
|
||||
* card); absent for kinds with no structured data
|
||||
* - `attachments`: per-attachment metadata `{kind, filename, mime_type, size_bytes}`
|
||||
* - `attachments`: per-attachment metadata `{kind, filename, mime_type}`
|
||||
* - `reasoning`: concatenated reasoning text for assistant turns that
|
||||
* round-tripped a thinking-block lane (Anthropic-with-thinking today;
|
||||
* OpenAI Responses + Gemini in later phases). Present only when the
|
||||
|
||||
@@ -279,6 +279,34 @@ def test_replay_renders_system_turn_via_add_system_context() -> None:
|
||||
)
|
||||
|
||||
|
||||
def test_retry_walk_skips_operator_context_cards() -> None:
|
||||
"""Interactive twin of the coord retry-skip guard.
|
||||
``_attachRetryToLastAssistant`` walks back past ``.operator-context`` rows
|
||||
before testing for ``.ts-approval`` — so a watch-result / guard-finding
|
||||
card (or a plain system bubble) trailing a tool-only turn doesn't make retry
|
||||
attach to a stale earlier assistant turn. Pin the walk predicate (scoped to
|
||||
the method) AND the shared marker on every operator row that can trail a
|
||||
tool batch, so adding a card kind without the marker fails loudly here."""
|
||||
body = _APP_JS.read_text(encoding="utf-8")
|
||||
start = _pane_method_offset(body, "_attachRetryToLastAssistant")
|
||||
end = _pane_method_offset(body, "announceToolBlock")
|
||||
fn = body[start:end]
|
||||
assert 'classList.contains("operator-context")' in fn, (
|
||||
"_attachRetryToLastAssistant must walk back past .operator-context "
|
||||
"rows so the tool-only retry skip fires even when a card trails."
|
||||
)
|
||||
# Every operator row that can trail a tool batch carries the shared marker.
|
||||
for cls in (
|
||||
'"msg system-context operator-context"',
|
||||
'"msg watch-result operator-context"',
|
||||
'"msg guard-finding operator-context"',
|
||||
):
|
||||
assert cls in body, (
|
||||
f"operator row className {cls} must carry the operator-context "
|
||||
"marker or the retry walk won't skip it."
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Phase 8 — Chunk D: MCP error embed + settings panel UX
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@@ -335,13 +335,46 @@ def test_coord_history_renders_system_turn_via_msg_variants():
|
||||
assert 'role === "system"' in body, (
|
||||
"coord history loop must have a system-role branch for first-class operator-context turns."
|
||||
)
|
||||
# The ``system`` _MSG_VARIANTS entry gives the bubble operator styling.
|
||||
assert 'system: "system-context"' in body, (
|
||||
"coordinator.js must map the system role to the .msg.system-context "
|
||||
"variant so operator-context turns get the operator styling."
|
||||
# The ``system`` _MSG_VARIANTS entry gives the bubble operator styling and
|
||||
# tags it with the shared ``operator-context`` marker (so the retry-skip
|
||||
# walk steps over it — see test_coord_retry_walk_skips_operator_context_cards).
|
||||
assert 'system: "system-context operator-context"' in body, (
|
||||
"coordinator.js must map the system role to the "
|
||||
"'system-context operator-context' variant so operator-context turns "
|
||||
"get the operator styling AND carry the retry-skip marker."
|
||||
)
|
||||
|
||||
|
||||
def test_coord_retry_walk_skips_operator_context_cards():
|
||||
"""Retry must NOT regenerate a stale assistant turn when the last DOM row is
|
||||
a tool batch trailed by an operator-context row. ``_refreshRetryButton``
|
||||
walks back past ``.operator-context`` rows before testing for
|
||||
``.coord-tool-batch`` — which only works if EVERY operator row carries the
|
||||
shared marker. Pin the walk predicate AND the marker on each structured
|
||||
card so a new card kind (or a walk keyed on a single class) can't silently
|
||||
re-introduce the wrong-turn retry regression."""
|
||||
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 'classList.contains("operator-context")' in body, (
|
||||
"_refreshRetryButton must walk back past .operator-context rows so the "
|
||||
"tool-only retry skip fires even when a card trails the tool batch."
|
||||
)
|
||||
for builder, cls in (
|
||||
("appendWatchResult", '"msg watch-result operator-context"'),
|
||||
("appendGuardFinding", '"msg guard-finding operator-context"'),
|
||||
("appendIdleChildren", '"msg idle-children operator-context"'),
|
||||
):
|
||||
assert cls in body, (
|
||||
f"{builder} must tag its card with the shared operator-context "
|
||||
f"marker ({cls}) or the retry walk won't skip it."
|
||||
)
|
||||
|
||||
|
||||
def test_coordinator_js_seeds_resume_cursor_only_on_initial_connect():
|
||||
"""coordinator.js must consume the /history resume cursor the same way
|
||||
ui/static/app.js does: the shared make_history_handler trims the
|
||||
|
||||
@@ -356,7 +356,10 @@
|
||||
// `.msg.system-context` rule (turnstone/shared_static/chat.css). The
|
||||
// generic history-replay branch already renders unknown roles via
|
||||
// appendText("system", …); this variant gives it the operator styling.
|
||||
system: "system-context",
|
||||
// The `operator-context` marker is shared by every operator row (this
|
||||
// bubble + the watch-result / guard-finding / idle-children cards) so the
|
||||
// retry-skip walk in _refreshRetryButton can skip them all uniformly.
|
||||
system: "system-context operator-context",
|
||||
};
|
||||
|
||||
function appendMsg(role, html, opts) {
|
||||
@@ -402,7 +405,7 @@
|
||||
// _buildWatchResultBubble.
|
||||
function appendWatchResult(meta, content) {
|
||||
const el = document.createElement("div");
|
||||
el.className = "msg watch-result";
|
||||
el.className = "msg watch-result operator-context";
|
||||
el.setAttribute("role", "article");
|
||||
el.setAttribute("data-ts-role", "watch");
|
||||
el.setAttribute("aria-label", "watch");
|
||||
@@ -452,7 +455,7 @@
|
||||
// _buildGuardFindingBubble.
|
||||
function appendGuardFinding(meta) {
|
||||
const el = document.createElement("div");
|
||||
el.className = "msg guard-finding";
|
||||
el.className = "msg guard-finding operator-context";
|
||||
el.setAttribute("role", "article");
|
||||
el.setAttribute("data-ts-role", "output_guard");
|
||||
el.setAttribute("aria-label", "output guard");
|
||||
@@ -494,7 +497,7 @@
|
||||
// producer); rendered via textContent so a hostile workstream name is inert.
|
||||
function appendIdleChildren(meta) {
|
||||
const el = document.createElement("div");
|
||||
el.className = "msg idle-children";
|
||||
el.className = "msg idle-children operator-context";
|
||||
el.setAttribute("role", "article");
|
||||
el.setAttribute("data-ts-role", "idle_children");
|
||||
el.setAttribute("aria-label", "idle children");
|
||||
@@ -535,7 +538,7 @@
|
||||
// "queued message" bubble for a ``user_interjection`` system turn — shows the
|
||||
// user's raw words (``meta.message``) rather than the model-directed framing
|
||||
// baked into ``content``, with brighter emphasis for ``!!!``-important
|
||||
// interjections. Reuses ``appendText`` (→ ``.msg.system-context``) + a class.
|
||||
// interjections. Reuses ``appendText`` and adds ``.important`` for ``!!!`` ones.
|
||||
function appendInterjection(meta, content) {
|
||||
const important = meta && meta.priority === "important";
|
||||
const text =
|
||||
@@ -543,7 +546,6 @@
|
||||
const el = appendText("system", text, {
|
||||
label: important ? "queued message · important" : "queued message",
|
||||
});
|
||||
el.classList.add("interjection");
|
||||
if (important) el.classList.add("important");
|
||||
return el;
|
||||
}
|
||||
@@ -4439,13 +4441,16 @@
|
||||
const old = messagesEl.querySelectorAll(".msg.assistant .msg-actions");
|
||||
for (let i = 0; i < old.length; i++) old[i].parentNode.removeChild(old[i]);
|
||||
// Skip retry when the most recent semantic turn is tool-only (last DOM
|
||||
// child is a .coord-tool-batch construct); walk back past .system-context
|
||||
// operator bubbles first so the guard still fires when the tool turn
|
||||
// carried a nudge / guard finding. (Coord's batch class is
|
||||
// child is a .coord-tool-batch construct); walk back past operator-context
|
||||
// rows first — the plain system bubble AND the structured watch-result /
|
||||
// guard-finding / idle-children cards all carry .operator-context — so the
|
||||
// guard still fires when the tool turn carried a nudge / guard finding.
|
||||
// Keying on the shared marker (not any single card class) keeps the skip
|
||||
// correct as new card kinds are added. (Coord's batch class is
|
||||
// .coord-tool-batch — the interactive pane uses .ts-approval, which does
|
||||
// not exist in coord's DOM.)
|
||||
let lastChild = messagesEl.lastElementChild;
|
||||
while (lastChild && lastChild.classList.contains("system-context")) {
|
||||
while (lastChild && lastChild.classList.contains("operator-context")) {
|
||||
lastChild = lastChild.previousElementSibling;
|
||||
}
|
||||
if (lastChild && lastChild.classList.contains("coord-tool-batch")) {
|
||||
|
||||
@@ -3731,6 +3731,8 @@ def make_attachment_handlers(cfg: SessionEndpointConfig) -> AttachmentHandlers:
|
||||
return JSONResponse({"attachments": rows})
|
||||
|
||||
async def get_content(request: Request) -> Response:
|
||||
import asyncio
|
||||
|
||||
from starlette.responses import Response as _Response
|
||||
|
||||
from turnstone.core.attachment_buffer import get_attachment_buffer
|
||||
@@ -3762,8 +3764,14 @@ def make_attachment_handlers(cfg: SessionEndpointConfig) -> AttachmentHandlers:
|
||||
stored_mime = staged.mime_type or "application/octet-stream"
|
||||
filename = staged.filename or "attachment"
|
||||
else:
|
||||
row = get_attachment(attachment_id)
|
||||
if not row or not attachment_referenced_in_ws(attachment_id, ws_id):
|
||||
# Both committed-blob gates are sync DB I/O — the ref check is an
|
||||
# unbounded ws-scoped LIKE scan (O(turns-in-ws)) run on every
|
||||
# committed-image request, so keep it off the event loop. Matches
|
||||
# the asyncio.to_thread convention used throughout this module.
|
||||
row = await asyncio.to_thread(get_attachment, attachment_id)
|
||||
if not row or not await asyncio.to_thread(
|
||||
attachment_referenced_in_ws, attachment_id, ws_id
|
||||
):
|
||||
return JSONResponse({"error": "Not found"}, status_code=404)
|
||||
body = row.get("content") or b""
|
||||
kind = row.get("kind") or ""
|
||||
|
||||
@@ -67,6 +67,8 @@ class HistoryEvent(ServerEvent):
|
||||
poll_count, max_polls, is_final}``) so the renderer can rebuild
|
||||
per-kind UI (the watch-result card); absent when the kind carries no
|
||||
structured data
|
||||
* ``attachments`` — per-attachment metadata ``{kind, filename,
|
||||
mime_type}`` (turns that carried image / document refs)
|
||||
* ``reasoning`` — concatenated reasoning text for assistant turns
|
||||
that round-tripped a thinking-block lane (Anthropic-with-thinking
|
||||
today; OpenAI Responses + Gemini in later phases). Present only
|
||||
|
||||
@@ -1005,7 +1005,9 @@
|
||||
trajectory) — the consolidation of the metacognition reminder /
|
||||
user-interjection / output-guard bubbles into one turn type that FOLLOWS
|
||||
the turn it advises. Shared so the interactive UI and the console coord
|
||||
viewer render it identically. Yellow accent reads as "operator metadata"
|
||||
viewer render the bubble identically (the visible kind label is
|
||||
interactive-only — see ``.msg-system-context-label`` below). Yellow accent
|
||||
reads as "operator metadata"
|
||||
against the amber user colour and the cyan tool cards; deliberately quieter
|
||||
so it doesn't compete for attention. The ``watch_triggered`` kind carries
|
||||
structured per-kind meta and branches into the richer ``.msg.watch-result``
|
||||
@@ -1017,6 +1019,12 @@
|
||||
padding: 6px 10px;
|
||||
white-space: pre-wrap;
|
||||
}
|
||||
/* The visible kind label is built ONLY by the interactive pane
|
||||
(app.js ``addSystemContext`` → a ``.msg-system-context-label`` span). The
|
||||
coord viewer (coordinator.js ``appendMsg``) renders the kind via
|
||||
border-colour + ``data-ts-role``/``aria-label`` with no visible label for
|
||||
ANY role, so this rule (and its ``.important`` variant below) is inert on the
|
||||
coord pane by design — not dead CSS. */
|
||||
.msg.system-context .msg-system-context-label {
|
||||
color: var(--yellow);
|
||||
font-weight: 600;
|
||||
|
||||
+13
-10
@@ -273,7 +273,8 @@ class Pane {
|
||||
source === "user_interjection" && meta && typeof meta === "object";
|
||||
const important = isInterjection && meta.priority === "important";
|
||||
const el = document.createElement("div");
|
||||
el.className = "msg system-context" + (important ? " important" : "");
|
||||
el.className =
|
||||
"msg system-context operator-context" + (important ? " important" : "");
|
||||
const body = document.createElement("div");
|
||||
body.className = "msg-body";
|
||||
const labelEl = document.createElement("span");
|
||||
@@ -2211,14 +2212,16 @@ class Pane {
|
||||
// .msg.assistant selector already excludes them — no extra guard needed.
|
||||
//
|
||||
// Skip retry attachment when the most recent semantic turn is
|
||||
// tool-only — last DOM child is a .ts-approval block. Walk back
|
||||
// past .system-context bubbles (operator-context system turns that
|
||||
// FOLLOW the tool batch they advise) so the guard fires correctly
|
||||
// even when the tool turn carried a nudge / guard finding. Without
|
||||
// this skip, retry lands on a stale prior assistant content bubble
|
||||
// belonging to an earlier turn.
|
||||
// tool-only — last DOM child is a .ts-approval block. Walk back past
|
||||
// operator-context rows (the plain system bubble AND the structured
|
||||
// watch-result / guard-finding cards — every operator row carries
|
||||
// .operator-context) which FOLLOW the tool batch they advise, so the
|
||||
// guard fires even when the tool turn carried a nudge / guard finding.
|
||||
// Keying on the shared marker (not any single card class) keeps the skip
|
||||
// correct as new card kinds are added. Without it, retry lands on a
|
||||
// stale prior assistant content bubble belonging to an earlier turn.
|
||||
let lastChild = this.messagesEl.lastElementChild;
|
||||
while (lastChild && lastChild.classList.contains("system-context")) {
|
||||
while (lastChild && lastChild.classList.contains("operator-context")) {
|
||||
lastChild = lastChild.previousElementSibling;
|
||||
}
|
||||
if (lastChild && lastChild.classList.contains("ts-approval")) {
|
||||
@@ -2758,7 +2761,7 @@ class Pane {
|
||||
// buildWatchResultBubble.
|
||||
function _buildWatchResultBubble(meta, content) {
|
||||
const el = document.createElement("div");
|
||||
el.className = "msg watch-result";
|
||||
el.className = "msg watch-result operator-context";
|
||||
el.setAttribute("role", "article");
|
||||
el.setAttribute("data-ts-role", "watch");
|
||||
el.setAttribute("aria-label", "watch");
|
||||
@@ -2805,7 +2808,7 @@ function _buildWatchResultBubble(meta, content) {
|
||||
// tool chip omits to stay terse. All text via textContent.
|
||||
function _buildGuardFindingBubble(meta) {
|
||||
const el = document.createElement("div");
|
||||
el.className = "msg guard-finding";
|
||||
el.className = "msg guard-finding operator-context";
|
||||
el.setAttribute("role", "article");
|
||||
el.setAttribute("data-ts-role", "output_guard");
|
||||
el.setAttribute("aria-label", "output guard");
|
||||
|
||||
Reference in New Issue
Block a user