Address PR review feedback

- gitDiffFilter: actually accumulate context-line runs and collapse them

- toolResultCompressor: disable failing filters for the rest of a pass; warn once

- toolResultCompressor: preserve LanguageModelTextPart2 audience when rewriting

- toolResultCompressor: rename byte/bytes -> char/chars in telemetry + docs

- Add unit tests for ToolResultCompressorService.maybeCompress

Author: Megan Rogge

src/extension/tools/common/test/toolResultCompressor.spec.ts

@@ -0,0 +1,136 @@
+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *  Licensed under the MIT License. See License.txt in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/
+
+import { describe, expect, it } from 'vitest';
+import type { IConfigurationService } from '../../../../platform/configuration/common/configurationService';
+import type { ILogService } from '../../../../platform/log/common/logService';
+import type { ITelemetryService } from '../../../../platform/telemetry/common/telemetry';
+import {
+	LanguageModelDataPart,
+	LanguageModelPartAudience,
+	LanguageModelTextPart,
+	LanguageModelTextPart2,
+	LanguageModelToolResult,
+} from '../../../../vscodeTypes';
+import { IToolResultFilter, ToolResultCompressorService } from '../toolResultCompressor';
+
+const TOOL = 'run_in_terminal';
+
+function makeService(opts: {
+	enabled: boolean;
+	warnings?: string[];
+}): ToolResultCompressorService {
+	const config = {
+		getConfig: (_key: unknown) => opts.enabled,
+	} as unknown as IConfigurationService;
+	const telemetry = {
+		sendMSFTTelemetryEvent: () => { /* noop */ },
+	} as unknown as ITelemetryService;
+	const log = {
+		warn: (msg: string) => { opts.warnings?.push(msg); },
+	} as unknown as ILogService;
+	return new ToolResultCompressorService(config, telemetry, log);
+}
+
+function longText(prefix: string): string {
+	// Must exceed MIN_COMPRESSIBLE_LENGTH (80) so filters get a chance to run.
+	return prefix + ' ' + 'x'.repeat(200);
+}
+
+const replaceWithFooFilter: IToolResultFilter = {
+	id: 'test.replaceWithFoo',
+	toolNames: [TOOL],
+	matches: () => true,
+	apply: () => ({ text: 'foo', compressed: true }),
+};
+
+describe('ToolResultCompressorService', () => {
+	it('returns undefined when disabled', () => {
+		const svc = makeService({ enabled: false });
+		svc.registerFilter(replaceWithFooFilter);
+		const result = new LanguageModelToolResult([new LanguageModelTextPart(longText('hello'))]);
+		expect(svc.maybeCompress(TOOL, {}, result)).toBeUndefined();
+	});
+
+	it('returns undefined when no filters registered', () => {
+		const svc = makeService({ enabled: true });
+		const result = new LanguageModelToolResult([new LanguageModelTextPart(longText('hello'))]);
+		expect(svc.maybeCompress(TOOL, {}, result)).toBeUndefined();
+	});
+
+	it('returns undefined when no filters match', () => {
+		const svc = makeService({ enabled: true });
+		svc.registerFilter({
+			id: 'no-match',
+			toolNames: [TOOL],
+			matches: () => false,
+			apply: () => ({ text: 'foo', compressed: true }),
+		});
+		const result = new LanguageModelToolResult([new LanguageModelTextPart(longText('hello'))]);
+		expect(svc.maybeCompress(TOOL, {}, result)).toBeUndefined();
+	});
+
+	it('disables a throwing filter for the rest of the pass and warns once', () => {
+		const warnings: string[] = [];
+		const svc = makeService({ enabled: true, warnings });
+		let calls = 0;
+		svc.registerFilter({
+			id: 'thrower',
+			toolNames: [TOOL],
+			matches: () => true,
+			apply: () => { calls++; throw new Error('boom'); },
+		});
+		svc.registerFilter(replaceWithFooFilter);
+		const result = new LanguageModelToolResult([
+			new LanguageModelTextPart(longText('a')),
+			new LanguageModelTextPart(longText('b')),
+			new LanguageModelTextPart(longText('c')),
+		]);
+		const out = svc.maybeCompress(TOOL, {}, result)!;
+		expect(out).toBeDefined();
+		// Throwing filter is invoked exactly once on the first text part, then disabled.
+		expect(calls).toBe(1);
+		expect(warnings.length).toBe(1);
+		expect(warnings[0]).toContain('thrower');
+		// The other filter still rewrites every text part.
+		for (const part of out.content) {
+			expect(part).toBeInstanceOf(LanguageModelTextPart);
+			expect((part as LanguageModelTextPart).value).toBe('foo');
+		}
+	});
+
+	it('preserves non-text parts unchanged', () => {
+		const svc = makeService({ enabled: true });
+		svc.registerFilter(replaceWithFooFilter);
+		const dataPart = new LanguageModelDataPart(new Uint8Array([1, 2, 3]), 'application/octet-stream');
+		const textPart = new LanguageModelTextPart(longText('hello'));
+		const result = new LanguageModelToolResult([dataPart, textPart]);
+		const out = svc.maybeCompress(TOOL, {}, result)!;
+		expect(out.content[0]).toBe(dataPart);
+		expect(out.content[1]).toBeInstanceOf(LanguageModelTextPart);
+		expect((out.content[1] as LanguageModelTextPart).value).toBe('foo');
+	});
+
+	it('preserves LanguageModelTextPart2 audience metadata when rewriting', () => {
+		const svc = makeService({ enabled: true });
+		svc.registerFilter(replaceWithFooFilter);
+		const audience = [LanguageModelPartAudience.Assistant, LanguageModelPartAudience.User];
+		const part = new LanguageModelTextPart2(longText('hello'), audience);
+		const result = new LanguageModelToolResult([part]);
+		const out = svc.maybeCompress(TOOL, {}, result)!;
+		expect(out.content[0]).toBeInstanceOf(LanguageModelTextPart2);
+		const rewritten = out.content[0] as LanguageModelTextPart2;
+		expect(rewritten.value).toBe('foo');
+		expect(rewritten.audience).toEqual(audience);
+	});
+
+	it('skips text parts shorter than the minimum compressible length', () => {
+		const svc = makeService({ enabled: true });
+		svc.registerFilter(replaceWithFooFilter);
+		const result = new LanguageModelToolResult([new LanguageModelTextPart('tiny')]);
+		// Nothing was compressed because the part was below the threshold.
+		expect(svc.maybeCompress(TOOL, {}, result)).toBeUndefined();
+	});
+});

src/extension/tools/common/toolResultCompressor.ts

@@ -8,7 +8,7 @@ import { ConfigKey, IConfigurationService } from '../../../platform/configuratio
 import { ILogService } from '../../../platform/log/common/logService';
 import { ITelemetryService } from '../../../platform/telemetry/common/telemetry';
 import { createServiceIdentifier } from '../../../util/common/services';
-import { LanguageModelTextPart } from '../../../vscodeTypes';
+import { LanguageModelTextPart, LanguageModelTextPart2 } from '../../../vscodeTypes';
 
 export const IToolResultCompressor = createServiceIdentifier<IToolResultCompressor>('IToolResultCompressor');
 
@@ -54,8 +54,9 @@ export interface IToolResultCompressor {
 }
 
 /**
- * Outputs at or below this size are not worth compressing.
- * Mirrors ztk's 80-byte minimum.
+ * Outputs at or below this many characters (UTF-16 code units, i.e.
+ * `string.length`) are not worth compressing. Mirrors ztk's 80-character
+ * minimum.
  */
 const MIN_COMPRESSIBLE_LENGTH = 80;
 
@@ -96,6 +97,11 @@ export class ToolResultCompressorService implements IToolResultCompressor {
 			return undefined;
 		}
 
+		// Mutable copy: filters that throw get spliced out so we don't repeatedly
+		// invoke a broken filter on every subsequent text part in this pass.
+		const activeFilters = matchingFilters.slice();
+		const disabledFilterIds = new Set<string>();
+
 		let totalBefore = 0;
 		let totalAfter = 0;
 		let anyCompressed = false;
@@ -111,23 +117,35 @@ export class ToolResultCompressorService implements IToolResultCompressor {
 			}
 
 			let current = original;
-			for (const filter of matchingFilters) {
+			for (let i = 0; i < activeFilters.length; /* manual increment */) {
+				const filter = activeFilters[i];
 				try {
 					const out = filter.apply(current, input);
 					if (out.compressed && out.text.length < current.length) {
 						current = out.text;
 						usedFilterIds.add(filter.id);
 					}
+					i++;
 				} catch (err) {
-					// "Never make it worse." Drop the filter on error and keep going.
-					this._logService.warn(`[ToolResultCompressor] filter ${filter.id} threw on tool ${toolName}: ${err}`);
+					// "Never make it worse." Disable the filter for the rest of this
+					// compression pass so it can't repeatedly throw on later text parts,
+					// and warn at most once per filter.
+					activeFilters.splice(i, 1);
+					if (!disabledFilterIds.has(filter.id)) {
+						disabledFilterIds.add(filter.id);
+						this._logService.warn(`[ToolResultCompressor] filter ${filter.id} threw on tool ${toolName}; disabled for this pass: ${err}`);
+					}
 				}
 			}
 
 			totalBefore += original.length;
 			totalAfter += current.length;
 			if (current !== original) {
 				anyCompressed = true;
+				// Preserve LanguageModelTextPart2 audience metadata if present.
+				if (part instanceof LanguageModelTextPart2) {
+					return new LanguageModelTextPart2(current, part.audience);
+				}
 				return new LanguageModelTextPart(current);
 			}
 			return part;
@@ -145,21 +163,21 @@ export class ToolResultCompressorService implements IToolResultCompressor {
 		return compressed as vscode.LanguageModelToolResult;
 	}
 
-	private _sendTelemetry(toolName: string, filterIds: string[], beforeBytes: number, afterBytes: number) {
+	private _sendTelemetry(toolName: string, filterIds: string[], beforeChars: number, afterChars: number) {
 		/* __GDPR__
 			"toolResultCompressed" : {
 				"owner": "meganrogge",
 				"comment": "Reports tool output compression savings.",
 				"toolName": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "comment": "The tool whose output was compressed." },
 				"filters": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "comment": "Comma-separated filter ids that fired." },
-				"beforeBytes": { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "isMeasurement": true, "comment": "Total text part bytes before compression." },
-				"afterBytes": { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "isMeasurement": true, "comment": "Total text part bytes after compression." }
+				"beforeChars": { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "isMeasurement": true, "comment": "Total text part length in UTF-16 code units before compression." },
+				"afterChars": { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "isMeasurement": true, "comment": "Total text part length in UTF-16 code units after compression." }
 			}
 		*/
 		this._telemetryService.sendMSFTTelemetryEvent(
 			'toolResultCompressed',
 			{ toolName, filters: filterIds.join(',') },
-			{ beforeBytes, afterBytes },
+			{ beforeChars, afterChars },
 		);
 	}
 }

src/extension/tools/node/compressors/terminalOutputCompressor.ts

@@ -56,21 +56,23 @@ export const gitDiffFilter: IToolResultFilter = {
 	apply(text): IToolResultFilterOutput {
 		const lines = text.split('\n');
 		const out: string[] = [];
-		let suppressed = 0;
+		// Number of context lines to keep at the start of each unchanged run before
+		// collapsing the rest into a single "... N omitted ..." marker.
+		const KEEP_CONTEXT = 1;
+		let contextRun = 0;
 		let inBinaryOrLock = false;
-		let currentFileHeader: string | null = null;
 
-		const flushSuppression = () => {
-			if (suppressed > 0) {
-				out.push(`... ${suppressed} unchanged context line${suppressed === 1 ? '' : 's'} omitted ...`);
-				suppressed = 0;
+		const flushContextRun = () => {
+			const omitted = contextRun - KEEP_CONTEXT;
+			if (omitted > 0) {
+				out.push(`... ${omitted} unchanged context line${omitted === 1 ? '' : 's'} omitted ...`);
 			}
+			contextRun = 0;
 		};
 
 		for (const line of lines) {
 			if (line.startsWith('diff --git')) {
-				flushSuppression();
-				currentFileHeader = line;
+				flushContextRun();
 				inBinaryOrLock = /package-lock\.json|yarn\.lock|pnpm-lock\.yaml|\.lockb?$|\.snap$/.test(line);
 				if (inBinaryOrLock) {
 					out.push(line);
@@ -92,20 +94,19 @@ export const gitDiffFilter: IToolResultFilter = {
 			// Keep file-mode markers, hunk markers, +/- lines verbatim.
 			if (line.startsWith('+++ ') || line.startsWith('--- ') || line.startsWith('@@') ||
 				line.startsWith('+') || line.startsWith('-') || line.startsWith('Binary files ')) {
-				flushSuppression();
+				flushContextRun();
 				out.push(line);
 				continue;
 			}
-			// Unchanged context line — suppress past 1 line per side.
-			// Cheap heuristic: just count consecutive context runs and collapse runs >2.
-			suppressed++;
-			if (suppressed <= 1) {
+			// Unchanged context line: keep the first KEEP_CONTEXT lines of each run,
+			// then count the rest so the next non-context line can flush a single
+			// summary marker.
+			contextRun++;
+			if (contextRun <= KEEP_CONTEXT) {
 				out.push(line);
-				suppressed = 0; // reset since we kept it
 			}
 		}
-		flushSuppression();
-		void currentFileHeader;
+		flushContextRun();
 
 		const result = out.join('\n');
 		return {

src/extension/tools/node/test/terminalOutputCompressor.spec.ts

@@ -51,6 +51,27 @@ describe('gitDiffFilter', () => {
 		expect(out.text).toContain('@@ -1,3 +1,3 @@');
 		expect(out.text).not.toContain('index abc..def');
 	});
+	it('collapses long unchanged-context runs into a single marker', () => {
+		const ctxLines = Array.from({ length: 20 }, (_, i) => ` this is context line number ${i}`);
+		const text = [
+			'diff --git a/foo.ts b/foo.ts',
+			'--- a/foo.ts',
+			'+++ b/foo.ts',
+			'@@ -1,22 +1,22 @@',
+			...ctxLines,
+			'-old',
+			'+new',
+		].join('\n');
+		const out = gitDiffFilter.apply(text, input);
+		// First context line is kept verbatim, the next 19 are collapsed.
+		expect(out.text).toContain(' this is context line number 0');
+		expect(out.text).not.toContain(' this is context line number 5');
+		expect(out.text).not.toContain(' this is context line number 19');
+		expect(out.text).toContain('19 unchanged context lines omitted');
+		expect(out.text).toContain('-old');
+		expect(out.text).toContain('+new');
+		expect(out.compressed).toBe(true);
+	});
 	it('omits lockfile diffs', () => {
 		const text = [
 			'diff --git a/package-lock.json b/package-lock.json',