Merge remote-tracking branch 'origin/main' into garrytan/boston-v2

# Conflicts:
#	CHANGELOG.md
#	VERSION
#	package.json

browse/src/server.ts

@@ -59,6 +59,43 @@ import * as net from 'net';
 import * as path from 'path';
 import * as crypto from 'crypto';
 
+// ─── Unicode Sanitization ───────────────────────────────────────
+// Remove unpaired UTF-16 surrogate halves (\uD800–\uDFFF). Page DOM text,
+// OCR output, and other CDP-sourced strings can contain lone surrogates;
+// JSON consumers downstream (Anthropic API in particular) reject them with
+// "no low surrogate in string". Valid surrogate pairs (e.g. emoji) survive
+// unchanged. Lone halves become U+FFFD (�).
+//
+// INVARIANT: every server egress path that ships page-content strings MUST
+// route through this sanitizer. handleCommandInternal wraps the final
+// cr.result string (text/plain bodies carry lone surrogates verbatim;
+// JSON.stringify already escapes them). The two SSE producers below
+// stringify with `sanitizeReplacer` so payload string fields get cleaned
+// BEFORE escaping. Plain post-stringify regex is a no-op there because
+// JSON.stringify converts \uD800 → "\\ud800" — the regex can't see the
+// surrogate after that point.
+function sanitizeLoneSurrogates(str: string): string {
+  return str.replace(/[\uD800-\uDFFF]/g, (match, offset) => {
+    const code = match.charCodeAt(0);
+    if (code >= 0xD800 && code <= 0xDBFF) {
+      const next = str.charCodeAt(offset + 1);
+      if (next >= 0xDC00 && next <= 0xDFFF) return match;
+    }
+    if (code >= 0xDC00 && code <= 0xDFFF) {
+      const prev = str.charCodeAt(offset - 1);
+      if (prev >= 0xD800 && prev <= 0xDBFF) return match;
+    }
+    return '�';
+  });
+}
+
+// JSON.stringify replacer that sanitizes string values before they get
+// escape-encoded. Pair with stringify when the consumer will JSON.parse the
+// payload back into JS strings (SSE clients do this).
+function sanitizeReplacer(_key: string, value: unknown): unknown {
+  return typeof value === 'string' ? sanitizeLoneSurrogates(value) : value;
+}
+
 // ─── Config ─────────────────────────────────────────────────────
 const config = resolveConfig();
 ensureStateDir(config);
@@ -693,7 +730,7 @@ interface CommandResult {
  *   skipActivity: true when called from chain (chain emits 1 event for all subcommands)
  *   chainDepth: recursion guard — reject nested chains (depth > 0 means inside a chain)
  */
-async function handleCommandInternal(
+async function handleCommandInternalImpl(
   body: { command: string; args?: string[]; tabId?: number },
   tokenInfo?: TokenInfo | null,
   opts?: { skipRateCheck?: boolean; skipActivity?: boolean; chainDepth?: number },
@@ -1040,6 +1077,21 @@ async function handleCommandInternal(
   }
 }
 
+/**
+ * Sanitizing wrapper around handleCommandInternalImpl. ALL callers (single-command
+ * HTTP, batch loop, scoped-token dispatch) go through this so the lone-surrogate
+ * sanitization happens once at the architectural choke point, not per-leaf.
+ * Do not bypass this by calling handleCommandInternalImpl directly.
+ */
+async function handleCommandInternal(
+  body: { command: string; args?: string[]; tabId?: number },
+  tokenInfo?: TokenInfo | null,
+  opts?: { skipRateCheck?: boolean; skipActivity?: boolean; chainDepth?: number },
+): Promise<CommandResult> {
+  const cr = await handleCommandInternalImpl(body, tokenInfo, opts);
+  return { ...cr, result: sanitizeLoneSurrogates(cr.result) };
+}
+
 /** HTTP wrapper — converts CommandResult to Response */
 async function handleCommand(body: any, tokenInfo?: TokenInfo | null): Promise<Response> {
   const cr = await handleCommandInternal(body, tokenInfo);
@@ -1789,19 +1841,24 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle {
 
         const stream = new ReadableStream({
           start(controller) {
+            // SSE egress invariant: every JSON.stringify here ships page-content-derived
+            // fields (URLs, command args, errors) to the sidebar. Lone surrogates must
+            // be sanitized DURING stringify (via sanitizeReplacer) so they're cleaned
+            // before escape-encoding — post-stringify regex is ineffective because
+            // JSON.stringify has already converted \uD800 → "\\ud800".
             // 1. Gap detection + replay
             const { entries, gap, gapFrom, availableFrom } = getActivityAfter(afterId);
             if (gap) {
-              controller.enqueue(encoder.encode(`event: gap\ndata: ${JSON.stringify({ gapFrom, availableFrom })}\n\n`));
+              controller.enqueue(encoder.encode(`event: gap\ndata: ${JSON.stringify({ gapFrom, availableFrom }, sanitizeReplacer)}\n\n`));
             }
             for (const entry of entries) {
-              controller.enqueue(encoder.encode(`event: activity\ndata: ${JSON.stringify(entry)}\n\n`));
+              controller.enqueue(encoder.encode(`event: activity\ndata: ${JSON.stringify(entry, sanitizeReplacer)}\n\n`));
             }
 
             // 2. Subscribe for live events
             const unsubscribe = subscribe((entry) => {
               try {
-                controller.enqueue(encoder.encode(`event: activity\ndata: ${JSON.stringify(entry)}\n\n`));
+                controller.enqueue(encoder.encode(`event: activity\ndata: ${JSON.stringify(entry, sanitizeReplacer)}\n\n`));
               } catch (err: any) {
                 console.debug('[browse] Activity SSE stream error, unsubscribing:', err.message);
                 unsubscribe();
@@ -2150,10 +2207,15 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle {
         const encoder = new TextEncoder();
         const stream = new ReadableStream({
           start(controller) {
+            // SSE egress invariant: inspectorData and CDP event payloads carry
+            // page-DOM strings (selectors, attribute values, console messages).
+            // sanitizeReplacer cleans lone surrogates DURING JSON.stringify so
+            // they're neutralized before escape-encoding (post-stringify regex
+            // is a no-op once \uD800 has become "\\ud800").
             // Send current state immediately
             if (inspectorData) {
               controller.enqueue(encoder.encode(
-                `event: state\ndata: ${JSON.stringify({ data: inspectorData, timestamp: inspectorTimestamp })}\n\n`
+                `event: state\ndata: ${JSON.stringify({ data: inspectorData, timestamp: inspectorTimestamp }, sanitizeReplacer)}\n\n`
               ));
             }
 
@@ -2161,7 +2223,7 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle {
             const notify: InspectorSubscriber = (event) => {
               try {
                 controller.enqueue(encoder.encode(
-                  `event: inspector\ndata: ${JSON.stringify(event)}\n\n`
+                  `event: inspector\ndata: ${JSON.stringify(event, sanitizeReplacer)}\n\n`
                 ));
               } catch (err: any) {
                 console.debug('[browse] Inspector SSE stream error:', err.message);

browse/test/server-sanitize-surrogates.test.ts

@@ -0,0 +1,129 @@
+import { describe, test, expect } from 'bun:test';
+import * as fs from 'fs';
+import * as path from 'path';
+
+// The sanitizer is module-private in server.ts. Rather than refactor it to a
+// separate module just for testing, we extract its source via a regex slice and
+// eval it in a fresh function scope. Keeps the production layout untouched.
+const SERVER_PATH = path.resolve(import.meta.dir, '..', 'src', 'server.ts');
+const SERVER_SRC = fs.readFileSync(SERVER_PATH, 'utf-8');
+
+const fnMatch = SERVER_SRC.match(
+  /function sanitizeLoneSurrogates\(str: string\): string \{[\s\S]*?\n\}/
+);
+if (!fnMatch) throw new Error('Could not locate sanitizeLoneSurrogates in server.ts');
+
+// Strip TS annotations so eval works under plain JS.
+const jsSrc = fnMatch[0].replace('(str: string): string', '(str)');
+const sanitizeLoneSurrogates = new Function(`${jsSrc}\nreturn sanitizeLoneSurrogates;`)() as (
+  s: string,
+) => string;
+
+describe('sanitizeLoneSurrogates — unit cases', () => {
+  test('passthrough ASCII', () => {
+    expect(sanitizeLoneSurrogates('hello')).toBe('hello');
+  });
+
+  test('passthrough empty string', () => {
+    expect(sanitizeLoneSurrogates('')).toBe('');
+  });
+
+  test('preserves valid surrogate pair (U+1F389 🎉)', () => {
+    expect(sanitizeLoneSurrogates('hi 🎉')).toBe('hi 🎉');
+  });
+
+  test('replaces lone high surrogate mid-string', () => {
+    expect(sanitizeLoneSurrogates('a\uD800b')).toBe('a�b');
+  });
+
+  test('replaces lone low surrogate mid-string', () => {
+    expect(sanitizeLoneSurrogates('a\uDC00b')).toBe('a�b');
+  });
+
+  test('replaces trailing lone high at end of string', () => {
+    expect(sanitizeLoneSurrogates('a\uD800')).toBe('a�');
+  });
+
+  test('replaces leading lone low at start of string', () => {
+    expect(sanitizeLoneSurrogates('\uDC00b')).toBe('�b');
+  });
+
+  test('replaces two adjacent lone highs', () => {
+    expect(sanitizeLoneSurrogates('\uD800\uD800')).toBe('��');
+  });
+
+  test('replaces two adjacent lone lows', () => {
+    expect(sanitizeLoneSurrogates('\uDC00\uDC00')).toBe('��');
+  });
+
+  test('preserves valid pair followed by lone low', () => {
+    // 𐀀 = U+10000 = 𐀀, then a separate lone low.
+    const input = '𐀀\uDC00';
+    const output = sanitizeLoneSurrogates(input);
+    // Valid pair intact, trailing lone low replaced.
+    expect(output).toBe('𐀀�');
+  });
+
+  test('preserves valid pair preceded by lone low', () => {
+    const input = '\uDC00𐀀';
+    const output = sanitizeLoneSurrogates(input);
+    expect(output).toBe('�𐀀');
+  });
+});
+
+describe('sanitizeLoneSurrogates — bug-repro (D5)', () => {
+  // Pin the regression intent: a future refactor that drops sanitization
+  // must fail this test even if happy-path tests still pass.
+  test('unsanitized lone surrogate causes UTF-8 encode to substitute, sanitized version is stable', () => {
+    const badPayload = 'page content\uD800more content';
+
+    // Buffer.from(str, 'utf-8') silently substitutes invalid sequences with
+    // EF BF BD (U+FFFD). Round-trip is therefore lossy for lone surrogates.
+    const roundTrippedRaw = Buffer.from(badPayload, 'utf-8').toString('utf-8');
+    expect(roundTrippedRaw).not.toBe(badPayload); // proves the bug exists pre-sanitize
+
+    // After sanitization the round-trip is stable.
+    const sanitized = sanitizeLoneSurrogates(badPayload);
+    const roundTrippedSanitized = Buffer.from(sanitized, 'utf-8').toString('utf-8');
+    expect(roundTrippedSanitized).toBe(sanitized);
+  });
+
+  test('JSON.parse(JSON.stringify(...)) round-trip is stable after sanitization', () => {
+    // Anthropic's API path wraps the response body in a tool_result JSON
+    // object. JSON.stringify CAN encode a lone surrogate (escapes it), but
+    // some downstream consumers reject the resulting body.
+    const badPayload = 'before\uD800after';
+    const sanitized = sanitizeLoneSurrogates(badPayload);
+    const wrapped = JSON.stringify({ content: sanitized });
+    const reparsed = JSON.parse(wrapped) as { content: string };
+    // .toBe(sanitized) already proves the surrogate was replaced; the
+    // additional explicit check below documents the specific code points.
+    expect(reparsed.content).toBe(sanitized);
+    expect(reparsed.content.charCodeAt(6)).toBe(0xfffd); // � not \uD800
+  });
+});
+
+describe('sanitizeLoneSurrogates — wiring invariants', () => {
+  test('server.ts wraps every command result through handleCommandInternal', () => {
+    // The architectural choice is to wrap once at handleCommandInternal so
+    // both single-command HTTP and the batch loop inherit. If a future
+    // refactor moves sanitization back to handleCommand only, this test
+    // fails by detecting the missing wrapper.
+    expect(SERVER_SRC).toContain('async function handleCommandInternalImpl(');
+    expect(SERVER_SRC).toContain('result: sanitizeLoneSurrogates(cr.result)');
+  });
+
+  test('SSE activity feed sanitizes outbound frames via sanitizeReplacer', () => {
+    // Replacer must run DURING stringify; post-stringify regex is ineffective
+    // because JSON.stringify converts \uD800 → "\\ud800" before our regex sees it.
+    expect(SERVER_SRC).toContain('JSON.stringify(entry, sanitizeReplacer)');
+  });
+
+  test('SSE inspector stream sanitizes outbound frames via sanitizeReplacer', () => {
+    expect(SERVER_SRC).toContain('JSON.stringify(event, sanitizeReplacer)');
+  });
+
+  test('sanitizeReplacer is a function defined in server.ts', () => {
+    expect(SERVER_SRC).toContain('function sanitizeReplacer(');
+  });
+});