feat(tools): Centralize shell tool summarization (#4009)

6 files changed, 51 insertions, 58 deletions

diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts
index cd681f93..8f9ec1e2 100644
--- a/packages/core/src/core/coreToolScheduler.ts
+++ b/packages/core/src/core/coreToolScheduler.ts
@@ -656,39 +656,15 @@ export class CoreToolScheduler {
               return;
             }
 
-            let resultForDisplay: ToolResult = toolResult;
-            let summary: string | undefined;
-            if (scheduledCall.tool.summarizer) {
-              try {
-                const toolSignal = new AbortController();
-                summary = await scheduledCall.tool.summarizer(
-                  toolResult,
-                  this.config.getGeminiClient(),
-                  toolSignal.signal,
-                );
-                if (toolSignal.signal.aborted) {
-                  console.debug('aborted summarizing tool result');
-                  return;
-                }
-                if (scheduledCall.tool?.shouldSummarizeDisplay) {
-                  resultForDisplay = {
-                    ...toolResult,
-                    returnDisplay: summary,
-                  };
-                }
-              } catch (e) {
-                console.error('Error summarizing tool result:', e);
-              }
-            }
             const response = convertToFunctionResponse(
               toolName,
               callId,
-              summary ? [summary] : toolResult.llmContent,
+              toolResult.llmContent,
             );
             const successResponse: ToolCallResponseInfo = {
               callId,
               responseParts: response,
-              resultDisplay: resultForDisplay.returnDisplay,
+              resultDisplay: toolResult.returnDisplay,
               error: undefined,
             };
 
diff --git a/packages/core/src/core/nonInteractiveToolExecutor.ts b/packages/core/src/core/nonInteractiveToolExecutor.ts
index 03401514..ab001bd6 100644
--- a/packages/core/src/core/nonInteractiveToolExecutor.ts
+++ b/packages/core/src/core/nonInteractiveToolExecutor.ts
@@ -68,17 +68,9 @@ export async function executeToolCall(
       // No live output callback for non-interactive mode
     );
 
-    const tool_output = tool.summarizer
-      ? await tool.summarizer(
-          toolResult,
-          config.getGeminiClient(),
-          effectiveAbortSignal,
-        )
-      : toolResult.llmContent;
+    const tool_output = toolResult.llmContent;
 
-    const tool_display = tool.shouldSummarizeDisplay
-      ? (tool_output as string)
-      : toolResult.returnDisplay;
+    const tool_display = toolResult.returnDisplay;
 
     const durationMs = Date.now() - startTime;
     logToolCall(config, {
diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts
index acc8c01f..a4d56e22 100644
--- a/packages/core/src/tools/shell.test.ts
+++ b/packages/core/src/tools/shell.test.ts
@@ -4,9 +4,11 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import { expect, describe, it } from 'vitest';
+import { expect, describe, it, vi, beforeEach } from 'vitest';
 import { ShellTool } from './shell.js';
 import { Config } from '../config/config.js';
+import * as summarizer from '../utils/summarizer.js';
+import { GeminiClient } from '../core/client.js';
 
 describe('ShellTool', () => {
   it('should allow a command if no restrictions are provided', async () => {
@@ -396,3 +398,35 @@ describe('ShellTool', () => {
     );
   });
 });
+
+describe('ShellTool Bug Reproduction', () => {
+  let shellTool: ShellTool;
+  let config: Config;
+
+  beforeEach(() => {
+    config = {
+      getCoreTools: () => undefined,
+      getExcludeTools: () => undefined,
+      getDebugMode: () => false,
+      getGeminiClient: () => ({}) as GeminiClient,
+      getTargetDir: () => '.',
+    } as unknown as Config;
+    shellTool = new ShellTool(config);
+  });
+
+  it('should not let the summarizer override the return display', async () => {
+    const summarizeSpy = vi
+      .spyOn(summarizer, 'summarizeToolOutput')
+      .mockResolvedValue('summarized output');
+
+    const abortSignal = new AbortController().signal;
+    const result = await shellTool.execute(
+      { command: 'echo "hello"' },
+      abortSignal,
+    );
+
+    expect(result.returnDisplay).toBe('hello\n');
+    expect(result.llmContent).toBe('summarized output');
+    expect(summarizeSpy).toHaveBeenCalled();
+  });
+});
diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts
index e9f59f10..7e79c717 100644
--- a/packages/core/src/tools/shell.ts
+++ b/packages/core/src/tools/shell.ts
@@ -27,7 +27,7 @@ export interface ShellToolParams {
   directory?: string;
 }
 import { spawn } from 'child_process';
-import { llmSummarizer } from '../utils/summarizer.js';
+import { summarizeToolOutput } from '../utils/summarizer.js';
 
 const OUTPUT_UPDATE_INTERVAL_MS = 1000;
 
@@ -74,8 +74,6 @@ Process Group PGID: Process group started or \`(none)\``,
       },
       false, // output is not markdown
       true, // output can be updated
-      llmSummarizer,
-      true, // should summarize display output
     );
   }
 
@@ -490,6 +488,16 @@ Process Group PGID: Process group started or \`(none)\``,
         // returnDisplayMessage will remain empty, which is fine.
       }
     }
-    return { llmContent, returnDisplay: returnDisplayMessage };
+
+    const summary = await summarizeToolOutput(
+      llmContent,
+      this.config.getGeminiClient(),
+      abortSignal,
+    );
+
+    return {
+      llmContent: summary,
+      returnDisplay: returnDisplayMessage,
+    };
   }
 }
diff --git a/packages/core/src/tools/tool-registry.ts b/packages/core/src/tools/tool-registry.ts
index 4040e4ce..1778c6d6 100644
--- a/packages/core/src/tools/tool-registry.ts
+++ b/packages/core/src/tools/tool-registry.ts
@@ -11,7 +11,6 @@ import { spawn } from 'node:child_process';
 import { StringDecoder } from 'node:string_decoder';
 import { discoverMcpTools } from './mcp-client.js';
 import { DiscoveredMCPTool } from './mcp-tool.js';
-import { defaultSummarizer } from '../utils/summarizer.js';
 import { parse } from 'shell-quote';
 
 type ToolParams = Record<string, unknown>;
@@ -48,7 +47,6 @@ Signal: Signal number or \`(none)\` if no signal was received.
       parameterSchema,
       false, // isOutputMarkdown
       false, // canUpdateOutput
-      defaultSummarizer,
     );
   }
 
diff --git a/packages/core/src/tools/tools.ts b/packages/core/src/tools/tools.ts
index e99eda92..68739d0e 100644
--- a/packages/core/src/tools/tools.ts
+++ b/packages/core/src/tools/tools.ts
@@ -5,7 +5,6 @@
  */
 
 import { FunctionDeclaration, PartListUnion, Schema } from '@google/genai';
-import { Summarizer, defaultSummarizer } from '../utils/summarizer.js';
 
 /**
  * Interface representing the base Tool functionality
@@ -45,16 +44,6 @@ export interface Tool<
   canUpdateOutput: boolean;
 
   /**
-   * A function that summarizes the result of the tool execution.
-   */
-  summarizer?: Summarizer;
-
-  /**
-   * Whether the tool's display output should be summarized
-   */
-  shouldSummarizeDisplay?: boolean;
-
-  /**
    * Validates the parameters for the tool
    * Should be called from both `shouldConfirmExecute` and `execute`
    * `shouldConfirmExecute` should return false immediately if invalid
@@ -109,8 +98,6 @@ export abstract class BaseTool<
    * @param isOutputMarkdown Whether the tool's output should be rendered as markdown
    * @param canUpdateOutput Whether the tool supports live (streaming) output
    * @param parameterSchema JSON Schema defining the parameters
-   * @param summarizer Function to summarize the tool's output
-   * @param shouldSummarizeDisplay Whether the tool's display output should be summarized
    */
   constructor(
     readonly name: string,
@@ -119,8 +106,6 @@ export abstract class BaseTool<
     readonly parameterSchema: Schema,
     readonly isOutputMarkdown: boolean = true,
     readonly canUpdateOutput: boolean = false,
-    readonly summarizer: Summarizer = defaultSummarizer,
-    readonly shouldSummarizeDisplay: boolean = false,
   ) {}
 
   /**