diff --git a/CHANGELOG.md b/CHANGELOG.md index 6030280e..43802ab1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,8 @@ ### Fixed +- **Clearing a conversation can no longer be undone by crash recovery — even after you send a new message.** `Clear conversation` writes a one-time `.json.bak` before wiping the transcript; if the app then crashed or restarted, startup recovery saw the backup as "larger" than the cleared session and could restore the cleared history — including on top of a message you'd sent after clearing. Clear now stamps a unique per-clear marker on the session, and recovery treats a backup that predates that marker as stale, so a cleared conversation stays cleared. Genuine crash-loss recovery (unmarked or same-generation backups, unreadable/partial files) still restores as before. Hardens the clear-conversation data-loss fix from the prior releases. Thanks @rodboev. (#5584, #5570) +- **A message containing a broken emoji (lone Unicode surrogate) no longer breaks scroll position tracking.** If a message's text held a lone UTF-16 surrogate — e.g. a half-copied emoji from a truncated paste or interrupted stream — computing that message's scroll-anchor key threw an error, which broke scroll-position restore for the whole transcript. The anchor-key builder now handles lone surrogates safely (valid emoji are preserved; only the broken half is dropped), so scroll anchoring keeps working. Thanks @rumotoshino. (#5573, #5552) - **An in-progress CLI session now shows in the sidebar before its final output lands.** A CLI/agent session that had real activity (messages and user turns) but hadn't produced its final output yet was hidden from the sidebar until it completed, so an actively-running CLI session could be invisible while you waited on it. Such a session (real message count + user turns, not yet ended, no end reason) is now surfaced while it's still running. Empty/never-really-started rows stay hidden as before. Thanks @rodboev. (#5593, #5587) - **A completed answer is no longer followed by a misleading "No response from provider" card.** On the streaming settlement path, a stale terminal/partial-result flag could force the generic `no_response` error even when the current turn had already produced a full assistant answer — so you'd see the reply and then an error card under it. The generic silent-error path is now suppressed only when the merged current turn actually has a completed assistant answer; genuine failures (real silent failures, partial failures, auth/quota errors, cancellation, replayed rows, terminal tool/compression cases) all keep their existing error behavior. Thanks @rodboev. (#5592, #5575) - **The Mermaid diagram toolbar's "fit" and "fullscreen" buttons are now distinct icons.** The two buttons rendered a byte-identical glyph (the fullscreen icon was the fit icon's corner brackets drawn twice), so you couldn't tell them apart. Fullscreen now shows a distinct corner-frame-with-diagonal-arrows icon, visually separable from fit-to-screen's plain corner brackets. Thanks @rodboev. (#5565, #5525) diff --git a/api/models.py b/api/models.py index ad8308c4..7612b4fc 100644 --- a/api/models.py +++ b/api/models.py @@ -1057,6 +1057,7 @@ class Session: compression_recovery_action=None, truncation_watermark=None, truncation_boundary=None, + clear_generation=None, gateway_routing=None, gateway_routing_history=None, llm_title_generated: bool=False, manual_title: bool=False, @@ -1120,6 +1121,7 @@ class Session: ) self.truncation_watermark = truncation_watermark self.truncation_boundary = truncation_boundary + self.clear_generation = clear_generation self.gateway_routing = gateway_routing if isinstance(gateway_routing, dict) else None self.gateway_routing_history = gateway_routing_history if isinstance(gateway_routing_history, list) else [] self.llm_title_generated = bool(llm_title_generated) @@ -1192,6 +1194,7 @@ class Session: 'compression_recovery_source_session_id', 'compression_recovery_action', 'truncation_watermark', 'truncation_boundary', + 'clear_generation', 'gateway_routing', 'gateway_routing_history', 'llm_title_generated', 'manual_title', 'parent_session_id', 'worktree_path', 'worktree_branch', 'worktree_repo_root', 'worktree_created_at', diff --git a/api/routes.py b/api/routes.py index 1aeecb37..6acc78c7 100644 --- a/api/routes.py +++ b/api/routes.py @@ -13813,6 +13813,7 @@ def handle_post(handler, parsed) -> bool: s.pending_attachments = [] s.pending_started_at = None s.pending_user_source = None + s.clear_generation = uuid.uuid4().hex if had_sidecar_messages else None # Reset the title via the rename helper so clearing a manually-named # session also clears manual_title/llm_title_generated — otherwise the # reused session keeps its manual-title protection and never auto-names @@ -13833,6 +13834,7 @@ def handle_post(handler, parsed) -> bool: and persisted.get("pending_attachments") == [] and persisted.get("pending_started_at") is None and persisted.get("pending_user_source") is None + and persisted.get("clear_generation") == s.clear_generation ) except (OSError, json.JSONDecodeError, ValueError): logger.warning("session clear could not verify persisted empty state for %s", sid, exc_info=True) diff --git a/api/session_recovery.py b/api/session_recovery.py index 78dd1042..1081f365 100644 --- a/api/session_recovery.py +++ b/api/session_recovery.py @@ -211,6 +211,78 @@ def _backup_predates_intentional_shrink(session_path: Path, bak_path: Path) -> b return bak_ctx_len > live_ctx_len +def _session_records_clear_sentinel(session_path: Path, bak_path: Path) -> bool: + """Return True when the live sidecar records a provenanced clear sentinel. + + The live sidecar must carry the explicit /api/session/clear marker, and + the backup must not carry the same marker. Same-generation backups stay + recoverable; unreadable or partial matches fail open. + """ + try: + data = json.loads(session_path.read_text(encoding='utf-8')) + bak = json.loads(bak_path.read_text(encoding='utf-8')) + except (OSError, json.JSONDecodeError, ValueError): + return False + if not isinstance(data, dict) or not isinstance(bak, dict): + return False + clear_generation = data.get('clear_generation') + if not isinstance(clear_generation, str) or not clear_generation: + return False + if bak.get('clear_generation') == clear_generation: + return False + expected = { + 'messages': [], + 'context_messages': [], + 'truncation_watermark': 0.0, + 'truncation_boundary': 0.0, + 'active_stream_id': None, + 'pending_user_message': None, + 'pending_attachments': [], + 'pending_started_at': None, + 'pending_user_source': None, + } + for key, value in expected.items(): + if key not in data or data.get(key) != value: + return False + return True + + +def _live_supersedes_backup_by_clear_generation(session_path: Path, bak_path: Path) -> bool: + """Return True when the live sidecar provably post-dates the backup via a + clear sentinel even though the user has since sent NEW messages. + + Scope: this handles ONLY the post-clear-message case that the exact-empty + sentinel (_session_records_clear_sentinel) can't. After /api/session/clear + stamps a unique ``clear_generation`` and resets the truncation boundary to + 0.0, a pre-clear ``.json.bak`` is stale — restoring it would resurrect + cleared history on top of the post-clear message. We require: live carries a + ``clear_generation`` the backup lacks, live has a NON-EMPTY transcript, and + the live boundary still shows the clear reset (watermark == boundary == 0.0). + Empty clear-shaped sidecars stay governed by the exact-empty sentinel and its + existing recovery semantics. Same-generation backups stay recoverable; + unreadable/partial reads fail open (return False -> normal recovery), so a + genuine crash-loss is never suppressed. + """ + try: + data = json.loads(session_path.read_text(encoding='utf-8')) + bak = json.loads(bak_path.read_text(encoding='utf-8')) + except (OSError, json.JSONDecodeError, ValueError): + return False + if not isinstance(data, dict) or not isinstance(bak, dict): + return False + clear_generation = data.get('clear_generation') + if not isinstance(clear_generation, str) or not clear_generation: + return False + if bak.get('clear_generation') == clear_generation: + return False + live_messages = data.get('messages') + if not isinstance(live_messages, list) or len(live_messages) == 0: + return False + if data.get('truncation_watermark') != 0.0 or data.get('truncation_boundary') != 0.0: + return False + return True + + def inspect_session_recovery_status(session_path: Path) -> dict: """Return a status dict describing whether recovery is recommended. @@ -232,6 +304,17 @@ def inspect_session_recovery_status(session_path: Path) -> dict: } bak_count = _msg_count(bak_path) if bak_count > live_count: + if ( + _session_records_clear_sentinel(session_path, bak_path) + or _live_supersedes_backup_by_clear_generation(session_path, bak_path) + ): + return { + "session_id": session_path.stem, + "live_messages": live_count, + "bak_messages": bak_count, + "recommend": "no_action", + "intentional_clear_truncate": True, + } if ( _session_records_intentional_compress_shrink(session_path) and _backup_predates_intentional_shrink(session_path, bak_path) diff --git a/static/ui.js b/static/ui.js index bcecd34e..5cd1265a 100644 --- a/static/ui.js +++ b/static/ui.js @@ -845,11 +845,35 @@ function _messageVisibleIndexForRawIdx(rawIdx, visWithIdx){ } return -1; } +function _safeEncodeURIComponent(v){ + try{return encodeURIComponent(String(v));} + catch(e){ + // encodeURIComponent threw URIError -> one or more lone UTF-16 surrogates. + // Walk the string as UTF-16 code units: keep valid high(D800-DBFF) + + // low(DC00-DFFF) pairs intact (so emoji survive) and drop lone surrogates. + // No regex lookbehind/lookahead so this parses on every browser engine + // (some older WebViews / Safari <16.4 don't support lookbehind in regex + // literals, which would otherwise brick ui.js at parse time). + const s=String(v); + let cleaned=''; + for(let i=0;i=0xD800&&c<=0xDBFF){ + const n=(i+1=0xDC00&&n<=0xDFFF){cleaned+=s[i]+s[i+1];i++;} + }else if(c<0xDC00||c>0xDFFF){ + cleaned+=s[i]; + } + } + return encodeURIComponent(cleaned); + } +} + function _messageViewportAnchorKeyForMessage(m){ if(typeof _compressionMessageAnchorKey!=='function') return ''; const key=_compressionMessageAnchorKey(m); if(!key) return ''; - return [key.role||'',key.ts??'',key.attachments??0,key.text||''].map(v=>encodeURIComponent(String(v))).join('|'); + return [key.role||'',key.ts??'',key.attachments??0,key.text||''].map(v=>_safeEncodeURIComponent(v)).join('|'); } function _messageVisibleIndexForAnchorKey(anchorKey, visWithIdx){ const key=String(anchorKey||''); diff --git a/tests/test_5552_viewport_anchor_surrogate.py b/tests/test_5552_viewport_anchor_surrogate.py new file mode 100644 index 00000000..3ccf55c8 --- /dev/null +++ b/tests/test_5552_viewport_anchor_surrogate.py @@ -0,0 +1,289 @@ +"""Regression guard: viewport anchor keys must not crash on lone surrogates. + +The browser message renderer builds a per-message cache key for viewport +restoration via ``_messageViewportAnchorKeyForMessage``. It pipes +``role``, ``ts``, ``attachments``, and ``text`` through ``encodeURIComponent`` +and ``Array.prototype.map(...).join('|')``. + +``encodeURIComponent`` throws ``URIError: URI malformed`` when any of those +fields contains a lone UTF-16 surrogate (U+D800–U+DFFF, not part of a valid +pair). Because this happens synchronously inside ``renderMessages()``, a +single bad field on any message — e.g. one whose text was synthesized from a +truncated UTF-8 byte sequence in a tool output or attachment — used to +blank out the entire chat view. + +This test pulls the real ``_messageViewportAnchorKeyForMessage`` and +``_safeEncodeURIComponent`` out of ``static/ui.js`` and feeds them the exact +inputs that previously crashed. It also confirms valid emoji (surrogate +*pairs*) still encode normally, so we don't regress real content. + +Engine-compat note (added per gate review #5552): + +The fallback sanitizer is implemented as a ``charCodeAt`` code-unit walk +rather than a regex with lookbehind/lookahead. Some older WebViews and +Safari < 16.4 don't support regex lookbehind; a top-level regex literal +containing ``(?`` file is evaluated at +module PARSE time, so a parse failure there blanks the entire app for +those engines — strictly worse than the rare lone-surrogate crash this +fixes. The test ``test_no_lookbehind_in_static_ui_js`` enforces this by +refusing to merge a version of ``static/ui.js`` that contains a lookbehind +regex literal. +""" + +from __future__ import annotations + +import re +import shutil +import subprocess +from pathlib import Path + +import pytest + + +ROOT = Path(__file__).resolve().parents[1] +UI_JS_PATH = ROOT / "static" / "ui.js" + + +def _read_required_text(path: Path, label: str) -> str: + assert path.exists(), f"{label} not found at {path}" + return path.read_text(encoding="utf-8") + + +def _ui_js() -> str: + return _read_required_text(UI_JS_PATH, "static/ui.js") + + +def _run_node_script(script: str) -> str: + node = shutil.which("node") + if not node: + pytest.skip("node executable is required for JavaScript behavior checks") + try: + result = subprocess.run( + [node, "-e", script], + cwd=ROOT, + text=True, + capture_output=True, + timeout=10, + ) + except subprocess.TimeoutExpired as exc: + pytest.fail( + "node behavior check timed out" + f"\nstdout:\n{exc.stdout or ''}" + f"\nstderr:\n{exc.stderr or ''}" + ) + if result.returncode != 0: + pytest.fail( + "node behavior check failed" + f"\nstdout:\n{result.stdout or ''}" + f"\nstderr:\n{result.stderr or ''}" + ) + return result.stdout.strip() + + +def test_safe_encode_uri_component_defined(): + """The fix introduces a guarded wrapper. Make sure it's present and used.""" + src = _ui_js() + assert "function _safeEncodeURIComponent(" in src, ( + "_safeEncodeURIComponent helper is missing from static/ui.js" + ) + # The old vulnerable call site must be gone. + assert ( + ".map(v=>encodeURIComponent(String(v))).join('|')" + not in src + ), "raw encodeURIComponent is still being piped into the anchor key" + # And the new caller must be present. + assert ".map(v=>_safeEncodeURIComponent(v)).join('|')" in src, ( + "_messageViewportAnchorKeyForMessage is not routing through the safe wrapper" + ) + + +def test_safe_encode_uri_component_handles_lone_surrogates(): + """The exact inputs that previously crashed ``renderMessages()`` must now succeed.""" + script = r""" + const src = require('fs').readFileSync( + 'static/ui.js', 'utf8' + ); + // Extract just the two functions we care about. + const safeMatch = src.match( + /function _safeEncodeURIComponent\(v\)\{[\s\S]*?\n\}/ + ); + if (!safeMatch) { + console.log('MISSING_SAFE_FN'); + process.exit(0); + } + eval(safeMatch[0]); + + const cases = [ + // input, must_contain, must_not_contain + { name: 'plain_ascii', v: 'user', contains: ['user'], not: ['%'] }, + { name: 'plain_space', v: 'hello world', contains: ['hello%20world'], not: [] }, + { name: 'emoji_pair', v: '\u{1F43E}', contains: ['%F0%9F%90%BE'], not: ['\u{1F43E}'] }, + { name: 'lone_high', v: 'a\uD800b', contains: ['a', 'b'], not: ['\uD800'] }, + { name: 'lone_low', v: 'a\uDC00b', contains: ['a', 'b'], not: ['\uDC00'] }, + { name: 'mixed_surrogate', v: '\uD83Dhello\uD83Eworld', contains: ['hello', 'world'], not: ['\uD83D', '\uD83E'] }, + // Regression guards added per Greptile review: + // a) valid emoji pair must SURVIVE the fallback when a lone surrogate + // sits next to it (the broad regex used to strip both halves). + { name: 'mixed_pair_lone', v: '\u{1F43E}\uD800', contains: ['%F0%9F%90%BE'], not: ['\uD800'] }, + // b) literal pipe in text must be percent-encoded so the | used to + // join key segments can never appear inside a field value. + { name: 'pipe_with_lone', v: 'a|b\uD800c', contains: ['%7C'], not: ['|'] }, + // c) multiple lone surrogates in a row must all be stripped. + { name: 'multi_lone', v: '\uD800\uD801\uD802', contains: [], not: ['\uD800','\uD801','\uD802'] }, + // d) emoji-then-lone where the emoji comes first and the lone is mid-string. + { name: 'pair_middle_lone', v: 'foo\u{1F43E}\uD800bar', contains: ['foo','bar','%F0%9F%90%BE'], not: ['\uD800'] }, + // e) lone-then-emoji (lone must be skipped, emoji kept). + { name: 'lone_then_pair', v: '\uD800\u{1F43E}tail', contains: ['tail','%F0%9F%90%BE'], not: ['\uD800'] }, + ]; + + let ok = true; + for (const c of cases) { + let out; + try { + out = _safeEncodeURIComponent(c.v); + } catch (e) { + console.log(`THREW: ${c.name} -> ${e.message}`); + ok = false; + continue; + } + for (const needle of c.contains) { + if (!out.includes(needle)) { + console.log(`MISSING_CONTAIN: ${c.name} expected ${needle} in ${out}`); + ok = false; + } + } + for (const needle of c.not) { + if (out.includes(needle)) { + console.log(`FORBIDDEN_FOUND: ${c.name} contained ${needle} in ${out}`); + ok = false; + } + } + console.log(`OK ${c.name} -> ${out}`); + } + console.log(ok ? 'ALL_PASS' : 'FAIL'); + """ + stdout = _run_node_script(script) + assert "THREW" not in stdout, f"safe wrapper still throws: {stdout}" + assert "MISSING_CONTAIN" not in stdout, stdout + assert "FORBIDDEN_FOUND" not in stdout, stdout + assert stdout.rstrip().endswith("ALL_PASS"), stdout + + +def test_message_viewport_anchor_key_does_not_throw_on_surrogate_text(): + """End-to-end: a message whose ``text`` contains a lone surrogate must + produce a key without throwing — the failure mode that blanked the chat.""" + script = r""" + const src = require('fs').readFileSync( + 'static/ui.js', 'utf8' + ); + const safeMatch = src.match( + /function _safeEncodeURIComponent\(v\)\{[\s\S]*?\n\}/ + ); + const anchorMatch = src.match( + /function _messageViewportAnchorKeyForMessage\(m\)\{[\s\S]*?\n\}/ + ); + if (!safeMatch || !anchorMatch) { + console.log('MISSING_FN'); + process.exit(0); + } + eval(safeMatch[0]); + eval(anchorMatch[0]); + + // Build the minimal `_compressionMessageAnchorKey` shim the function + // requires (it just forwards the message fields). + _compressionMessageAnchorKey = (m) => ({ + role: m.role, + ts: m.ts, + attachments: m.attachments, + text: m.text, + }); + + const msgs = [ + { role: 'user', ts: 1783179992, attachments: 0, text: 'normal message' }, + { role: 'assistant', ts: 1783180000, attachments: 0, text: 'a\uD800b' }, + { role: 'user', ts: 1783180010, attachments: 1, text: 'reply with \uD83E🤖-bait' }, + // Regression guard: text with a literal '|' must produce a key whose + // segments still split cleanly (the pipe must be percent-encoded). + { role: 'user', ts: 1783180020, attachments: 0, text: 'a|b\uD800c' }, + // Emoji + lone, mid-string + { role: 'user', ts: 1783180030, attachments: 0, text: 'foo\u{1F43E}\uD800bar' }, + ]; + + let ok = true; + for (const m of msgs) { + let key; + try { + key = _messageViewportAnchorKeyForMessage(m); + } catch (e) { + console.log(`THREW: role=${m.role} -> ${e.message}`); + ok = false; + continue; + } + if (typeof key !== 'string' || key.length === 0) { + console.log(`EMPTY_KEY: role=${m.role}`); + ok = false; + continue; + } + // No surrogate code units should survive into the anchor key. + if (/[\uD800-\uDFFF]/.test(key)) { + console.log(`SURROGATE_IN_KEY: role=${m.role} -> ${key}`); + ok = false; + } + // The join separator is unescaped '|' and we always emit exactly 4 + // segments (role|ts|attachments|text). A literal '|' inside any + // field must have been percent-encoded to '%7C' by the safe wrapper. + const segs = key.split('|'); + if (segs.length !== 4) { + console.log(`BAD_SEGMENT_COUNT: role=${m.role} -> ${segs.length} segs in ${key}`); + ok = false; + } + console.log(`OK role=${m.role} key=${key}`); + } + console.log(ok ? 'ALL_PASS' : 'FAIL'); + """ + stdout = _run_node_script(script) + assert "THREW" not in stdout, stdout + assert "EMPTY_KEY" not in stdout, stdout + assert "SURROGATE_IN_KEY" not in stdout, stdout + assert "BAD_SEGMENT_COUNT" not in stdout, stdout + assert stdout.rstrip().endswith("ALL_PASS"), stdout + + +def test_no_lookbehind_in_static_ui_js(): + """No top-level lookbehind regex literal in ``static/ui.js``. + + A classic ``