fix(history): cut snippets and indexed text without splitting a surrogate pair

changelog.d/history-snippets.fixed.md

@@ -0,0 +1 @@
+- The snippets the history tools cut, and the text they send the semantic index, no longer begin or end on half of a surrogate pair, half of an emoji for instance. `history--search` keeps up to 80 UTF-16 units each side of a match, `history--semantic_search` up to 400 of a message hit's text, and the semantic index up to `maxChars` (32,000 by default) of a message's or a summary's text, each one fewer at a cut that would split a pair. A summary hit's snippet is still the one the index service sends. The internal `safeSlice` now also moves a start that falls on a low surrogate forward past it.

src/modules/history/index.ts

@@ -215,9 +215,13 @@ const SEMANTIC_DEFAULT_LIMIT = 10;
 const SEMANTIC_MAX_LIMIT = 50;
 /** Largest k sent to the embed-service when over-fetching past dropped hits (its own cap is 200). */
 const SEMANTIC_MAX_FETCH = 200;
-/** Local snippet: the leading SEMANTIC_SNIPPET_CHARS of the current text. */
+/**
+ * Local snippet: the leading SEMANTIC_SNIPPET_CHARS of the current text, one
+ * fewer where the last would split a surrogate pair, since the snippet reaches
+ * the agent's context.
+ */
 function snippetOf(text: string): string {
-  return text.length <= SEMANTIC_SNIPPET_CHARS ? text : `${text.slice(0, SEMANTIC_SNIPPET_CHARS)}…`;
+  return text.length <= SEMANTIC_SNIPPET_CHARS ? text : `${safeSlice(text, 0, SEMANTIC_SNIPPET_CHARS)}…`;
 }
 const SEMANTIC_SNIPPET_CHARS = 400;
 
@@ -1682,7 +1686,7 @@ export class HistoryModule implements Module {
       if (msgId !== undefined) {
         const m = cm.getMessage(msgId);
         if (!m) return 'offBranch';
-        const text = messageIndexText(m, includePrivate).slice(0, this.semanticCfg!.maxChars ?? 32_000);
+        const text = safeSlice(messageIndexText(m, includePrivate), 0, this.semanticCfg!.maxChars ?? 32_000);
         if (!text) return 'stale';
         currentText.set(h.id, text);
         return 'ok';
@@ -2588,12 +2592,12 @@ function buildAuthorFilter(include: AuthorSpec | undefined, exclude: AuthorSpec
 /** ~SNIPPET_CONTEXT_CHARS of surrounding context on each side of a match, or
  *  the first ~SNIPPET_FALLBACK_CHARS of content when no meaningful match
  *  position is given (kept simple on purpose — this is a readability aid,
- *  not a highlighting engine). */
+ *  not a highlighting engine). Neither end splits a surrogate pair. */
 function snippetAround(text: string, index: number, length: number): string {
-  if (index < 0) return text.slice(0, SNIPPET_FALLBACK_CHARS);
+  if (index < 0) return safeSlice(text, 0, SNIPPET_FALLBACK_CHARS);
   const start = Math.max(0, index - SNIPPET_CONTEXT_CHARS);
   const end = Math.min(text.length, index + length + SNIPPET_CONTEXT_CHARS);
   const prefix = start > 0 ? '…' : '';
   const suffix = end < text.length ? '…' : '';
-  return prefix + text.slice(start, end) + suffix;
+  return prefix + safeSlice(text, start, end) + suffix;
 }

src/modules/history/semantic.ts

@@ -293,7 +293,7 @@ export function messageToItem(msg: StoredMessage, cfg: { includePrivateTools?: b
   const tsMs = msg.timestamp.getTime();
   return {
     id: `msg:${String(msg.id)}`,
-    text: text.slice(0, cfg.maxChars ?? 32_000),
+    text: safeSlice(text, 0, cfg.maxChars ?? 32_000),
     ts: tsMs / 1000,
     channel: channelOf(msg),
     kind: 'message',
@@ -320,7 +320,7 @@ export function summaryToItem(s: SummaryLike, cfg: { maxChars?: number }): Index
   if (!text) return null;
   return {
     id: `sum:${s.id}`,
-    text: text.slice(0, cfg.maxChars ?? 32_000),
+    text: safeSlice(text, 0, cfg.maxChars ?? 32_000),
     ts: s.startMs / 1000,
     kind: 'summary',
     level: s.level,

src/safe-slice.ts

@@ -2,9 +2,17 @@
  * Slice a string without splitting surrogate pairs.
  *
  * Regular String.prototype.slice can cut in the middle of a surrogate pair,
- * producing invalid UTF-16. This function adjusts the end index to avoid that.
+ * producing invalid UTF-16. Here no half of a pair faces a cut: a start on a
+ * low surrogate moves forward past it, and an end just after a high surrogate
+ * moves back before it, whether or not the other half is there. A start of 0
+ * and an end at or past the length cut nothing, so the string's own first and
+ * last units stay as they are.
  */
 export function safeSlice(str: string, start: number, end: number): string {
+  // A low surrogate where the start cuts is a pair's second half, or a lone half: either way, skip it
+  const first = str.charCodeAt(start);
+  if (start > 0 && first >= 0xdc00 && first <= 0xdfff) start += 1;
+
   if (end >= str.length) return str.slice(start);
 
   // If we're about to cut between a high and low surrogate, back up one char

test/history-module-ux.test.ts

@@ -210,6 +210,25 @@ describe('HistoryModule UX: wholeWord', () => {
   });
 });
 
+describe('HistoryModule UX: snippets', () => {
+  it('keep up to 80 units each side of a match, one fewer where the 80th would split a surrogate pair', async () => {
+    // One code point, two UTF-16 units. With the "x" first, 80 units before the match start on the second half of
+    // a pair, and with the "z" after it, 80 units after the match end on the first half of one: each end moves in.
+    const grin = '\u{1F600}';
+    const before = `x${grin.repeat(100)}y`;
+    const { mod } = stub([
+      mk({ id: 's1', ms: min(1), text: `${before}needlez${grin.repeat(100)}` }),
+      // Here the message ends within 80 units of the match, so the snippet runs to its end and only its start moves.
+      mk({ id: 's2', ms: min(2), text: `${before}needlez` }),
+    ]);
+    const snippets = async (input: Record<string, unknown>) =>
+      data(await call(mod, 'search', input)).matches.map((m: any) => m.snippet);
+    const expected = [`…${grin.repeat(39)}yneedlez${grin.repeat(39)}…`, `…${grin.repeat(39)}yneedlez`];
+    assert.deepEqual(await snippets({ query: 'needle' }), expected);
+    assert.deepEqual(await snippets({ query: 'needle', regex: true }), expected);
+  });
+});
+
 describe('HistoryModule UX: order + scannedThrough', () => {
   it('order:newest scans the most recent maxScan and returns newest-first, with a to: resume point', async () => {
     const { mod } = stub(fixture);

test/history-semantic-review-findings.test.ts

@@ -366,6 +366,45 @@ describe('semantic sync: Greptile #173 findings', () => {
     await m.stop();
   });
 
+  it("a hit's snippet keeps up to 400 units of its text, one fewer where the 400th would split a surrogate pair", async () => {
+    // One code point, two UTF-16 units, so after the "x" the 400th unit is the first half of the 200th of them.
+    const grin = '\u{1F600}';
+    const cm = new StubCm([msg('e', T0, `x${grin.repeat(250)}`)]);
+    const m = new HistoryModule({ semantic: { url: svc.url, namespace: 'n', syncIntervalMs: 0, maxSyncBeforeSearch: 0 } });
+    m.bind(cm as unknown as ContextManager);
+    await m.syncSemanticIndex();
+    const res = await m.handleToolCall({ id: 'c', name: 'semantic_search', input: { query: 'x' } });
+    const hit = (res.data as { hits: Array<{ id: string; snippet: string }> }).hits.find((h) => h.id === 'msg:e');
+    assert.ok(hit, 'the message is a hit');
+    assert.equal(hit.snippet, `x${grin.repeat(199)}…`);
+    await m.stop();
+  });
+
+  it("with maxChars at 400, a hit's snippet is its text cut there at a whole character", async () => {
+    // maxChars cuts the current text the snippet is built from. After the "x", the 400th unit is the first half of a pair.
+    const grin = '\u{1F600}';
+    const cm = new StubCm([msg('e', T0, `x${grin.repeat(250)}`)]);
+    const m = new HistoryModule({ semantic: { url: svc.url, namespace: 'n', syncIntervalMs: 0, maxSyncBeforeSearch: 0, maxChars: 400 } });
+    m.bind(cm as unknown as ContextManager);
+    await m.syncSemanticIndex();
+    const res = await m.handleToolCall({ id: 'c', name: 'semantic_search', input: { query: 'x' } });
+    const hit = (res.data as { hits: Array<{ id: string; snippet: string }> }).hits.find((h) => h.id === 'msg:e');
+    assert.ok(hit, 'the message is a hit');
+    assert.equal(hit.snippet, `x${grin.repeat(199)}`);
+    await m.stop();
+  });
+
+  it("the text indexed is cut at maxChars at a whole character, a message's and a summary's", async () => {
+    const grin = '\u{1F600}';
+    const summary = { id: 's1', level: 1, content: `x${grin.repeat(250)}`, tokens: 3, startMs: T0, endMs: T0 + 1, firstSequence: 0, lastSequence: 1, createdMs: T0 + 5 };
+    const m = new HistoryModule({ semantic: { url: svc.url, namespace: 'n', syncIntervalMs: 0, maxChars: 400 } });
+    m.bind(new StubCm([msg('e', T0, `x${grin.repeat(250)}`)], [summary]) as unknown as ContextManager);
+    await m.syncSemanticIndex();
+    assert.equal(svc.items.get('msg:e')?.text, `x${grin.repeat(199)}`);
+    assert.equal(svc.items.get('sum:s1')?.text, `x${grin.repeat(199)}`);
+    await m.stop();
+  });
+
   it('G3#2 queued edits for messages that left the branch do not spend the sync budget', async () => {
     const cm = new StubCm(Array.from({ length: 300 }, (_, i) => msg(`m${i}`, T0 + i * 3_600_000, `message ${i}`)));
     const m = mod(cm);

test/safe-slice.test.ts

@@ -0,0 +1,35 @@
+import { describe, it } from 'node:test';
+import assert from 'node:assert/strict';
+import { safeSlice } from '../src/safe-slice.js';
+
+// One code point, two UTF-16 units: a high surrogate, then a low one.
+const grin = '\u{1F600}';
+const [high, low] = [grin[0]!, grin[1]!];
+
+describe('safeSlice: no half of a surrogate pair faces a cut', () => {
+  it('a start on the second half of a pair moves forward past it', () => {
+    assert.equal(safeSlice(`a${grin}b`, 2, 4), 'b');
+  });
+
+  it('a start on any other unit stays where it is', () => {
+    assert.equal(safeSlice(`ab${grin}c`, 1, 5), `b${grin}c`);
+    assert.equal(safeSlice(`a${grin}b`, 1, 4), `${grin}b`);
+  });
+
+  it("a start of 0 cuts nothing, so the string's own first unit stays, a lone low surrogate included", () => {
+    assert.equal(safeSlice(`${low}ab`, 0, 3), `${low}ab`);
+  });
+
+  it('an end inside a pair moves back before it', () => {
+    assert.equal(safeSlice(`a${grin}b`, 0, 2), 'a');
+  });
+
+  it('an end just after a whole pair stays where it is', () => {
+    assert.equal(safeSlice(`a${grin}b`, 0, 3), `a${grin}`);
+  });
+
+  it("an end at or past the length cuts nothing, so the string's own last unit stays, a lone high surrogate included", () => {
+    assert.equal(safeSlice(`a${grin}`, 0, 3), `a${grin}`);
+    assert.equal(safeSlice(`a${high}`, 0, 10), `a${high}`);
+  });
+});