From d9f5b2e288f5f74a308ea8d6d6ce258ac905db2b Mon Sep 17 00:00:00 2001 From: chinesepowered Date: Fri, 31 Jul 2026 01:46:11 -0700 Subject: [PATCH] fix(core): keep compactString within its limit when the marker does not fit (#7872) * fix(core): keep compactString within its limit when the marker does not fit The compaction marker embeds the original length, so it runs 60-80 characters on its own. `contentBudget` was clamped to 0 when the limit was smaller than that, but the marker was then appended regardless: compactStringForRecording('x'.repeat(70), 60) -> 79 chars compactStringForHistory('x'.repeat(64), 63) -> 64 chars compactStringForHistory('x'.repeat(100), 50) -> 65 chars The first returns more characters than the input it was asked to compact, which is the opposite of what a compaction function is for. Both entry points are exported and take a caller-supplied limit. When the marker cannot fit alongside any content there is nothing useful to announce, so hard-truncate the head instead of explaining the truncation at greater length than the truncated text. The marker is unchanged wherever the limit leaves room for it. * test(core): fix the purpose ternary and cover surrogates on the guard path Two review findings on the compaction tests. The `it.each` over both purposes called `compactStringForRecording` in both branches, so the two `history` rows silently re-ran the recording case. The history marker is 65 characters against recording's 79, so the two purposes take different content budgets and the history budget was never exercised. The `marker.length >= limit` guard also had no surrogate coverage: it slices without a marker, so it has a boundary of its own, and both existing surrogate-aware tests run at the default limit and take the head+marker+tail path instead. Added four rows over both purposes at limits 9 and 8. Replacing `safeHeadEnd(value, limit)` with a bare `limit` fails the two odd-limit rows and leaves the even-limit controls green, so the coverage is load-bearing. --------- Co-authored-by: Shaojin Wen --- .../utils/toolResultDisplayCompaction.test.ts | 79 +++++++++++++++++++ .../src/utils/toolResultDisplayCompaction.ts | 12 +++ 2 files changed, 91 insertions(+) diff --git a/packages/core/src/utils/toolResultDisplayCompaction.test.ts b/packages/core/src/utils/toolResultDisplayCompaction.test.ts index c4dd7be679..5703f7c01a 100644 --- a/packages/core/src/utils/toolResultDisplayCompaction.test.ts +++ b/packages/core/src/utils/toolResultDisplayCompaction.test.ts @@ -381,3 +381,82 @@ describe('toolResultDisplayCompaction', () => { expect(compactedTeam.teamName).toContain('truncated from'); }); }); + +describe('compactString limit', () => { + // The compaction marker embeds the original length, so it is 60-80 + // characters on its own. It used to be appended whatever the limit was, + // which meant a small caller-supplied limit got back more than it asked + // for -- and sometimes more than the string it was given. + it.each([ + ['recording' as const, 70, 60], + ['history' as const, 64, 63], + ['history' as const, 100, 50], + ['recording' as const, 200, 10], + ['history' as const, 40, 0], + ])( + 'keeps %s output within bounds for input %d at limit %d', + (purpose, inputLength, limit) => { + const value = 'x'.repeat(inputLength); + const compact = + purpose === 'recording' + ? compactStringForRecording(value, limit) + : compactStringForHistory(value, limit); + + expect(compact.length).toBeLessThanOrEqual(limit); + // Compacting must never hand back more characters than it was given. + expect(compact.length).toBeLessThanOrEqual(value.length); + }, + ); + + // Guards against over-correcting: when the limit does leave room for the + // marker, the marker must still be there. These pass before and after. + it.each([ + ['recording' as const, 5000, 500], + ['history' as const, 5000, 200], + ['history' as const, 5000, 120], + ])( + 'still explains the truncation for %s at input %d, limit %d', + (purpose, inputLength, limit) => { + const value = 'x'.repeat(inputLength); + const compact = + purpose === 'recording' + ? compactStringForRecording(value, limit) + : compactStringForHistory(value, limit); + + expect(compact.length).toBeLessThanOrEqual(limit); + expect(compact).toContain('truncated'); + }, + ); + + // The `marker.length >= limit` path slices without a marker, so it has a + // boundary of its own to get right. The two surrogate-aware tests above both + // run at the default limit and take the head+marker+tail path, so neither + // reaches this one. + it.each([ + ['history' as const, 9], + ['history' as const, 8], + ['recording' as const, 9], + ['recording' as const, 8], + ])( + 'does not split a surrogate pair when the marker does not fit, for %s at limit %d', + (purpose, limit) => { + const value = '😀'.repeat(40); + const compact = + purpose === 'recording' + ? compactStringForRecording(value, limit) + : compactStringForHistory(value, limit); + + expect(compact.length).toBeLessThanOrEqual(limit); + expect(hasUnpairedSurrogate(compact)).toBe(false); + // A whole number of pairs survived, so the cut backed off to a boundary + // rather than landing between a high and low surrogate. + expect(compact.length % 2).toBe(0); + // Confirms this really is the marker-does-not-fit path. + expect(compact).not.toContain('truncated'); + }, + ); + + it('returns a short string untouched regardless of the marker length', () => { + expect(compactStringForHistory('short', 1000)).toBe('short'); + }); +}); diff --git a/packages/core/src/utils/toolResultDisplayCompaction.ts b/packages/core/src/utils/toolResultDisplayCompaction.ts index fdd1fe2ce2..e118c09b6f 100644 --- a/packages/core/src/utils/toolResultDisplayCompaction.ts +++ b/packages/core/src/utils/toolResultDisplayCompaction.ts @@ -84,6 +84,18 @@ function compactString( } const marker = buildStringCompactionMarker(value, purpose); + + // The marker is 60-80 characters and was appended whatever the limit was, + // so a caller-supplied limit below that got back more than it asked for -- + // and for a limit under the input length, more characters than it passed + // in. `compactStringForRecording('x'.repeat(70), 60)` returned 79. When the + // marker cannot fit alongside any content there is nothing to announce, so + // hard-truncate instead of explaining the truncation at greater length than + // the truncated text. + if (marker.length >= limit) { + return copyString(value.slice(0, safeHeadEnd(value, Math.max(0, limit)))); + } + const contentBudget = Math.max(0, limit - marker.length); const headLength = Math.ceil(contentBudget * 0.6); const tailLength = contentBudget - headLength;