diff --git a/changelog.d/history-snippets.fixed.md b/changelog.d/history-snippets.fixed.md new file mode 100644 index 000000000..9ce0001dc --- /dev/null +++ b/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. diff --git a/src/modules/history/index.ts b/src/modules/history/index.ts index cabb5bb5f..f9db0bee7 100644 --- a/src/modules/history/index.ts +++ b/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; } diff --git a/src/modules/history/semantic.ts b/src/modules/history/semantic.ts index fe7d2dead..51c6c8f4e 100644 --- a/src/modules/history/semantic.ts +++ b/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, diff --git a/src/safe-slice.ts b/src/safe-slice.ts index 9339e0818..04b9090e4 100644 --- a/src/safe-slice.ts +++ b/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 diff --git a/test/history-module-ux.test.ts b/test/history-module-ux.test.ts index f06bef7a7..cc9bc29ce 100644 --- a/test/history-module-ux.test.ts +++ b/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) => + 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); diff --git a/test/history-semantic-review-findings.test.ts b/test/history-semantic-review-findings.test.ts index 0cf102e52..49b7c504e 100644 --- a/test/history-semantic-review-findings.test.ts +++ b/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); diff --git a/test/safe-slice.test.ts b/test/safe-slice.test.ts new file mode 100644 index 000000000..8eebd0b9e --- /dev/null +++ b/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}`); + }); +});