From 9aad27898317f4fc8258bfd20b0cfcc53a5b44f4 Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Sat, 6 Jun 2026 15:11:55 -0700 Subject: [PATCH] =?UTF-8?q?refactor(ui):=20L-shell=20step=205e.1a=20?= =?UTF-8?q?=E2=80=94=20shared=20conversation.js=20for=20the=20dup'd=20help?= =?UTF-8?q?ers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stand up shared_static/conversation.js as the deduplicated conversational-pane substrate both panes import (interactive via ./, the coordinator via /shared/ — both ES modules since 5e.0). First tenants are the byte-identical duplicates the in-file comments flagged for the step-5e lift: stripAnsi, the watch-result card builder, and the system-nudge marker. No visible change — the builders return the same DOM; each caller still appends + scrolls. - conversation.js: stripAnsi (null-safe variant), buildWatchResultCard, buildSystemNudgeMarker. - interactive.js / coordinator.js: import the three, drop their local copies, delegate appendWatchResult + the nudge marker through the shared builders. - stripAnsi unified on the coordinator's null-safe form (interactive's threw on a non-string arg); identical output for string inputs. - tests: new test_conversation_js.py pins the module; the two retry-walk guards now check the watch-result marker in conversation.js (it moved there). - refreshes interactive.js's header comment, stale since 5e.0 made the coordinator an ES module too. --- tests/test_app_js.py | 13 ++- tests/test_conversation_js.py | 67 ++++++++++++++ tests/test_coordinator_page.py | 9 +- .../console/static/coordinator/coordinator.js | 66 ++------------ turnstone/shared_static/conversation.js | 85 +++++++++++++++++ turnstone/shared_static/interactive.js | 91 ++++--------------- 6 files changed, 196 insertions(+), 135 deletions(-) create mode 100644 tests/test_conversation_js.py create mode 100644 turnstone/shared_static/conversation.js diff --git a/tests/test_app_js.py b/tests/test_app_js.py index ef8b8a67..2b56bf41 100644 --- a/tests/test_app_js.py +++ b/tests/test_app_js.py @@ -16,10 +16,7 @@ from pathlib import Path import pytest _APP_JS = Path(__file__).resolve().parent.parent / "turnstone/ui/static/app.js" -_INTERACTIVE_JS = ( - Path(__file__).resolve().parent.parent - / "turnstone/shared_static/interactive.js" -) +_INTERACTIVE_JS = Path(__file__).resolve().parent.parent / "turnstone/shared_static/interactive.js" def _pane_method_offset(body: str, name: str) -> int: @@ -314,9 +311,14 @@ def test_retry_walk_skips_operator_context_cards() -> None: "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. + # The watch-result card moved to the shared conversation.js (step 5e.1); the + # plain system-context + guard-finding cards stay in the pane. + shared = (_INTERACTIVE_JS.parent / "conversation.js").read_text(encoding="utf-8") + assert '"msg watch-result operator-context"' in shared, ( + "buildWatchResultCard must carry the operator-context marker." + ) for cls in ( '"msg system-context operator-context"', - '"msg watch-result operator-context"', '"msg guard-finding operator-context"', ): assert cls in body, ( @@ -421,6 +423,7 @@ def test_phase8_mcp_error_helpers_defined() -> None: "STANDALONE_HOST must wire host.onConsentDetected -> _onConsentDetected" ) + def test_phase8_settings_panel_handlers_defined() -> None: """The settings modal exposes four entry points that the inline ``onclick`` attributes in index.html depend on. Renaming or diff --git a/tests/test_conversation_js.py b/tests/test_conversation_js.py new file mode 100644 index 00000000..13b73c46 --- /dev/null +++ b/tests/test_conversation_js.py @@ -0,0 +1,67 @@ +"""Guards for the shared conversational-pane module +(``turnstone/shared_static/conversation.js``). + +Born in step 5e.1: the deduplicated substrate BOTH the interactive pane +(shared_static/interactive.js) and the coordinator pane +(console/static/coordinator/coordinator.js) import. These pin the exports plus +the load-bearing invariants (operator-context marker, null-safe ANSI strip, no +innerHTML) so a regression in the shared module fails loudly here rather than +silently in one pane. +""" + +from __future__ import annotations + +from pathlib import Path + +_CONVERSATION_JS = ( + Path(__file__).resolve().parent.parent / "turnstone/shared_static/conversation.js" +) + + +def _body() -> str: + return _CONVERSATION_JS.read_text(encoding="utf-8") + + +def test_exports_the_shared_helpers() -> None: + """The three helpers both panes import must be exported — drop one and the + importing pane module fails to load entirely.""" + body = _body() + for name in ("stripAnsi", "buildWatchResultCard", "buildSystemNudgeMarker"): + assert f"export function {name}" in body, f"{name} must be exported" + + +def test_strip_ansi_is_null_safe() -> None: + """Unified on the coordinator's null-safe variant: a non-string argument + coerces to "" rather than throwing (interactive's old copy did not guard, + so this is a strict-superset behaviour for its call sites).""" + body = _body() + assert 'String(s == null ? "" : s).replace(' in body, ( + "stripAnsi must coerce its argument before .replace" + ) + + +def test_watch_card_carries_operator_context_marker() -> None: + """The watch-result card keeps the shared ``operator-context`` marker (the + retry-walk in both panes skips rows carrying it) and stays textContent-only.""" + body = _body() + assert '"msg watch-result operator-context"' in body + assert 'setAttribute("data-ts-role", "watch")' in body + for part in ( + "msg-watch-header", + "msg-watch-cmd", + "msg-watch-body", + "msg-watch-footer", + ): + assert part in body, f"watch card missing {part}" + + +def test_nudge_marker_shape() -> None: + body = _body() + assert '"msg user system-nudge"' in body + assert 'setAttribute("data-source", "system_nudge")' in body + + +def test_no_inner_html() -> None: + """House style: programmatic DOM only — no innerHTML *usage* in the shared + module (the header comment names it; guard the access pattern).""" + assert ".innerHTML" not in _body() diff --git a/tests/test_coordinator_page.py b/tests/test_coordinator_page.py index 63a8e7b3..f13e1a43 100644 --- a/tests/test_coordinator_page.py +++ b/tests/test_coordinator_page.py @@ -390,8 +390,15 @@ def test_coord_retry_walk_skips_operator_context_cards(): "_refreshRetryButton must walk back past .operator-context rows so the " "tool-only retry skip fires even when a card trails the tool batch." ) + # The watch-result card moved to the shared conversation.js (step 5e.1); the + # guard-finding + idle-children cards stay in the coordinator pane. + shared = ( + Path(__file__).resolve().parent.parent / "turnstone/shared_static/conversation.js" + ).read_text(encoding="utf-8") + assert '"msg watch-result operator-context"' in shared, ( + "buildWatchResultCard must tag its card with the operator-context marker." + ) for builder, cls in ( - ("appendWatchResult", '"msg watch-result operator-context"'), ("appendGuardFinding", '"msg guard-finding operator-context"'), ("appendIdleChildren", '"msg idle-children operator-context"'), ): diff --git a/turnstone/console/static/coordinator/coordinator.js b/turnstone/console/static/coordinator/coordinator.js index 15c3121d..c41a2daa 100644 --- a/turnstone/console/static/coordinator/coordinator.js +++ b/turnstone/console/static/coordinator/coordinator.js @@ -26,6 +26,12 @@ // bits a pane doesn't want: the "Console" back-link, the theme toggle, and the // shared #toast (the console shell already provides theme + toast). // --------------------------------------------------------------------------- +import { + stripAnsi, + buildWatchResultCard, + buildSystemNudgeMarker, +} from "/shared/conversation.js"; + function buildCoordChrome(root, opts) { opts = opts || {}; root.classList.add("coord-chrome-root"); @@ -484,18 +490,6 @@ function createCoordinatorPane(root, wsId, opts) { .replace(/'/g, "'"); } - // ANSI-escape stripper — mirrors ui/static/app.js so a tool that - // emits CSI sequences (rare on the coord tool surface, but bash - // through MCP / the underlying child node still can) lands as - // readable text inside the tool-batch result block. - function stripAnsi(s) { - return String(s == null ? "" : s).replace( - // eslint-disable-next-line no-control-regex - /\x1b(?:\[[0-9;?]*[@-~]|\][^\x07\x1b]*(?:\x07|\x1b\\)?|[()#][A-Za-z0-9]|.)/g, - "", - ); - } - // Post-process tool-output JSON: wrap known ws_id + node_id pairs in a // link pointing at /node/{node_id}/?ws_id={child_ws_id}. Only applies // when BOTH keys are present and look like valid hex ids. @@ -660,46 +654,10 @@ function createCoordinatorPane(root, wsId, opts) { // ``command`` / ``poll_count`` / ``max_polls`` / ``is_final``) delivered live // on the ``system_turn`` SSE event and on the ``/history`` projection. All // text goes through ``textContent`` so shell output containing angle brackets - // / scripts / steering bytes renders inertly. Mirrors the interactive pane's - // _buildWatchResultBubble. + // / scripts / steering bytes renders inertly. Delegates to the shared + // conversation.buildWatchResultCard; this wrapper appends + scrolls. function appendWatchResult(meta, content) { - const el = document.createElement("div"); - el.className = "msg watch-result operator-context"; - el.setAttribute("role", "article"); - el.setAttribute("data-ts-role", "watch"); - el.setAttribute("aria-label", "watch"); - const header = document.createElement("div"); - header.className = "msg-watch-header"; - header.textContent = - "watch" + (meta.watch_name ? " · " + String(meta.watch_name) : ""); - el.appendChild(header); - if (meta.command) { - const cmd = document.createElement("div"); - cmd.className = "msg-watch-cmd"; - cmd.textContent = "$ " + String(meta.command); - el.appendChild(cmd); - } - const body = document.createElement("pre"); - body.className = "msg-watch-body"; - // Prefer the structured ``output`` (raw shell output alone) so the body - // doesn't re-print the header / ``$ command`` lines the chrome already - // shows; fall back to the full turn ``content`` for legacy turns that - // predate the ``output`` meta field (migration 060 is additive). - body.textContent = - meta.output != null ? String(meta.output) : content || ""; - el.appendChild(body); - if (meta.poll_count != null && meta.max_polls != null) { - const footer = document.createElement("div"); - footer.className = "msg-watch-footer"; - const finalSuffix = meta.is_final ? " · final" : ""; - footer.textContent = - "poll " + - String(meta.poll_count) + - "/" + - String(meta.max_polls) + - finalSuffix; - el.appendChild(footer); - } + const el = buildWatchResultCard(meta, content); messagesEl.appendChild(el); _scheduleScroll(); return el; @@ -871,11 +829,7 @@ function createCoordinatorPane(root, wsId, opts) { // carried are now first-class operator-context ``system`` turns that // follow it and render via the ``system`` ``_MSG_VARIANTS`` styling. function appendSystemNudgeMarker() { - const el = document.createElement("div"); - el.className = "msg user system-nudge"; - el.setAttribute("data-source", "system_nudge"); - el.setAttribute("aria-label", "system nudge"); - el.textContent = "system nudge"; + const el = buildSystemNudgeMarker(); messagesEl.appendChild(el); return el; } diff --git a/turnstone/shared_static/conversation.js b/turnstone/shared_static/conversation.js new file mode 100644 index 00000000..922d8344 --- /dev/null +++ b/turnstone/shared_static/conversation.js @@ -0,0 +1,85 @@ +// conversation.js — shared conversational-pane substrate (step 5e). +// +// Deduplicated helpers used by BOTH the interactive pane +// (shared_static/interactive.js) and the coordinator pane +// (console/static/coordinator/coordinator.js). Both panes are ES modules +// (interactive since 5a, the coordinator since 5e.0), so this is a plain ESM +// module they import directly — interactive via `./conversation.js`, the +// coordinator via the absolute `/shared/conversation.js`. No window bridge: +// nothing classic consumes these (the standalone ui/static app.js drives the +// interactive Pane, which imports them on its behalf). +// +// House style holds: programmatic DOM (createElement / textContent / append), +// never innerHTML. Builders return a detached element and let the caller append +// + scroll, so they stay pane- and transport-agnostic. + +// ANSI / CSI escape stripper — a tool that emits control sequences (bash through +// MCP, or a child node) must land as readable text in the result block. +// Null-safe: a non-string argument coerces to "" rather than throwing. +export function stripAnsi(s) { + return String(s == null ? "" : s).replace( + // eslint-disable-next-line no-control-regex + /\x1b(?:\[[0-9;?]*[@-~]|\][^\x07\x1b]*(?:\x07|\x1b\\)?|[()#][A-Za-z0-9]|.)/g, + "", + ); +} + +// Structured `.msg.watch-result` card for a `watch_triggered` operator-context +// system turn — command-preview header + shell-output body + poll-counter footer. +// `content` is the formatted turn body; `meta` carries the structured fields +// (watch_name / command / output / poll_count / max_polls / is_final) delivered +// live on the `system_turn` SSE event and on the `/history` projection. All text +// goes through textContent so shell output containing angle brackets / scripts / +// steering bytes renders inertly. The caller appends + scrolls. +export function buildWatchResultCard(meta, content) { + const el = document.createElement("div"); + el.className = "msg watch-result operator-context"; + el.setAttribute("role", "article"); + el.setAttribute("data-ts-role", "watch"); + el.setAttribute("aria-label", "watch"); + const header = document.createElement("div"); + header.className = "msg-watch-header"; + header.textContent = + "watch" + (meta.watch_name ? " · " + String(meta.watch_name) : ""); + el.appendChild(header); + if (meta.command) { + const cmd = document.createElement("div"); + cmd.className = "msg-watch-cmd"; + cmd.textContent = "$ " + String(meta.command); + el.appendChild(cmd); + } + const body = document.createElement("pre"); + body.className = "msg-watch-body"; + // Prefer the structured `output` (raw shell output alone) so the body doesn't + // re-print the header / `$ command` lines the chrome already shows; fall back to + // the full turn `content` for legacy turns predating the `output` meta field + // (migration 060 is additive — no backfill). + body.textContent = meta.output != null ? String(meta.output) : content || ""; + el.appendChild(body); + if (meta.poll_count != null && meta.max_polls != null) { + const footer = document.createElement("div"); + footer.className = "msg-watch-footer"; + const finalSuffix = meta.is_final ? " · final" : ""; + footer.textContent = + "poll " + + String(meta.poll_count) + + "/" + + String(meta.max_polls) + + finalSuffix; + el.appendChild(footer); + } + return el; +} + +// Thin `.msg.user.system-nudge` marker — the visible-but-subtle anchor a +// wake-driven empty user turn renders, so the operator-context `system` turns +// that follow it land in the right place. The caller handles empty-state +// removal + append. +export function buildSystemNudgeMarker() { + const el = document.createElement("div"); + el.className = "msg user system-nudge"; + el.setAttribute("data-source", "system_nudge"); + el.setAttribute("aria-label", "system nudge"); + el.textContent = "system nudge"; + return el; +} diff --git a/turnstone/shared_static/interactive.js b/turnstone/shared_static/interactive.js index 110e8fdc..a66b65ad 100644 --- a/turnstone/shared_static/interactive.js +++ b/turnstone/shared_static/interactive.js @@ -10,10 +10,10 @@ // `opts.base` URL prefix: "" locally, "/node/{id}" when the console proxies a // session living on a cluster node (the LOCALITY invariant). // -// ES module — the first legacy pane lifted into a real module (coordinator.js -// stays classic; the shared substrate it leans on — composer/renderer/etc. — -// does too, as those still have classic consumers). The console shell.js -// imports the factory directly; the standalone loads it as +// ES module — the first legacy pane lifted into a real module (step 5a; the +// coordinator pane followed in 5e.0). The shared substrate it leans on — +// composer/renderer/etc. — is still classic, consumed as globals. The console +// shell.js imports the factory directly; the standalone loads it as // `