fix(cli): Add delimiter before printing tool response in non-interactive mode (#11351)

This commit is contained in:
Krishna Bajpai
2025-10-27 07:57:54 -07:00
committed by GitHub
parent 2fa13420ae
commit c7817aee30
7 changed files with 305 additions and 32 deletions

View File

@@ -0,0 +1,8 @@
// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html
exports[`runNonInteractive > should write a single newline between sequential text outputs from the model 1`] = `
"Use mock tool
Use mock tool again
Finished.
"
`;

View File

@@ -190,6 +190,9 @@ describe('runNonInteractive', () => {
}
}
const getWrittenOutput = () =>
processStdoutSpy.mock.calls.map((c) => c[0]).join('');
it('should process input and write text output', async () => {
const events: ServerGeminiStreamEvent[] = [
{ type: GeminiEventType.Content, value: 'Hello' },
@@ -215,9 +218,7 @@ describe('runNonInteractive', () => {
expect.any(AbortSignal),
'prompt-id-1',
);
expect(processStdoutSpy).toHaveBeenCalledWith('Hello');
expect(processStdoutSpy).toHaveBeenCalledWith(' World');
expect(processStdoutSpy).toHaveBeenCalledWith('\n');
expect(getWrittenOutput()).toBe('Hello World\n');
expect(mockShutdownTelemetry).toHaveBeenCalled();
});
@@ -285,8 +286,77 @@ describe('runNonInteractive', () => {
expect.any(AbortSignal),
'prompt-id-2',
);
expect(processStdoutSpy).toHaveBeenCalledWith('Final answer');
expect(processStdoutSpy).toHaveBeenCalledWith('\n');
expect(getWrittenOutput()).toBe('Final answer\n');
});
it('should write a single newline between sequential text outputs from the model', async () => {
// This test simulates a multi-turn conversation to ensure that a single newline
// is printed between each block of text output from the model.
// 1. Define the tool requests that the model will ask the CLI to run.
const toolCallEvent: ServerGeminiStreamEvent = {
type: GeminiEventType.ToolCallRequest,
value: {
callId: 'mock-tool',
name: 'mockTool',
args: {},
isClientInitiated: false,
prompt_id: 'prompt-id-multi',
},
};
// 2. Mock the execution of the tools. We just need them to succeed.
mockCoreExecuteToolCall.mockResolvedValue({
status: 'success',
request: toolCallEvent.value, // This is generic enough for both calls
tool: {} as AnyDeclarativeTool,
invocation: {} as AnyToolInvocation,
response: {
responseParts: [],
callId: 'mock-tool',
},
});
// 3. Define the sequence of events streamed from the mock model.
// Turn 1: Model outputs text, then requests a tool call.
const modelTurn1: ServerGeminiStreamEvent[] = [
{ type: GeminiEventType.Content, value: 'Use mock tool' },
toolCallEvent,
];
// Turn 2: Model outputs more text, then requests another tool call.
const modelTurn2: ServerGeminiStreamEvent[] = [
{ type: GeminiEventType.Content, value: 'Use mock tool again' },
toolCallEvent,
];
// Turn 3: Model outputs a final answer.
const modelTurn3: ServerGeminiStreamEvent[] = [
{ type: GeminiEventType.Content, value: 'Finished.' },
{
type: GeminiEventType.Finished,
value: { reason: undefined, usageMetadata: { totalTokenCount: 10 } },
},
];
mockGeminiClient.sendMessageStream
.mockReturnValueOnce(createStreamFromEvents(modelTurn1))
.mockReturnValueOnce(createStreamFromEvents(modelTurn2))
.mockReturnValueOnce(createStreamFromEvents(modelTurn3));
// 4. Run the command.
await runNonInteractive(
mockConfig,
mockSettings,
'Use mock tool multiple times',
'prompt-id-multi',
);
// 5. Verify the output.
// The rendered output should contain the text from each turn, separated by a
// single newline, with a final newline at the end.
expect(getWrittenOutput()).toMatchSnapshot();
// Also verify the tools were called as expected.
expect(mockCoreExecuteToolCall).toHaveBeenCalledTimes(2);
});
it('should handle error during tool execution and should send error back to the model', async () => {
@@ -369,7 +439,7 @@ describe('runNonInteractive', () => {
expect.any(AbortSignal),
'prompt-id-3',
);
expect(processStdoutSpy).toHaveBeenCalledWith('Sorry, let me try again.');
expect(getWrittenOutput()).toBe('Sorry, let me try again.\n');
});
it('should exit with error if sendMessageStream throws initially', async () => {
@@ -444,9 +514,7 @@ describe('runNonInteractive', () => {
'Error executing tool nonexistentTool: Tool "nonexistentTool" not found in registry.',
);
expect(mockGeminiClient.sendMessageStream).toHaveBeenCalledTimes(2);
expect(processStdoutSpy).toHaveBeenCalledWith(
"Sorry, I can't find that tool.",
);
expect(getWrittenOutput()).toBe("Sorry, I can't find that tool.\n");
});
it('should exit when max session turns are exceeded', async () => {
@@ -506,7 +574,7 @@ describe('runNonInteractive', () => {
);
// 6. Assert the final output is correct
expect(processStdoutSpy).toHaveBeenCalledWith('Summary complete.');
expect(getWrittenOutput()).toBe('Summary complete.\n');
});
it('should process input and write JSON output with stats', async () => {
@@ -850,7 +918,7 @@ describe('runNonInteractive', () => {
'prompt-id-slash',
);
expect(processStdoutSpy).toHaveBeenCalledWith('Response from command');
expect(getWrittenOutput()).toBe('Response from command\n');
});
it('should throw FatalInputError if a command requires confirmation', async () => {
@@ -905,7 +973,7 @@ describe('runNonInteractive', () => {
'prompt-id-unknown',
);
expect(processStdoutSpy).toHaveBeenCalledWith('Response to unknown');
expect(getWrittenOutput()).toBe('Response to unknown\n');
});
it('should throw for unhandled command result types', async () => {
@@ -962,7 +1030,7 @@ describe('runNonInteractive', () => {
expect(mockAction).toHaveBeenCalledWith(expect.any(Object), 'arg1 arg2');
expect(processStdoutSpy).toHaveBeenCalledWith('Acknowledged');
expect(getWrittenOutput()).toBe('Acknowledged\n');
});
it('should instantiate CommandService with correct loaders for slash commands', async () => {
@@ -1073,7 +1141,7 @@ describe('runNonInteractive', () => {
expect.objectContaining({ name: 'ShellTool' }),
expect.any(AbortSignal),
);
expect(processStdoutSpy).toHaveBeenCalledWith('file.txt');
expect(getWrittenOutput()).toBe('file.txt\n');
});
describe('CoreEvents Integration', () => {

View File

@@ -40,6 +40,7 @@ import {
handleCancellationError,
handleMaxTurnsExceededError,
} from './utils/errors.js';
import { TextOutput } from './ui/utils/textOutput.js';
export async function runNonInteractive(
config: Config,
@@ -52,6 +53,7 @@ export async function runNonInteractive(
stderr: true,
debugMode: config.getDebugMode(),
});
const textOutput = new TextOutput();
const handleUserFeedback = (payload: UserFeedbackPayload) => {
const prefix = payload.severity.toUpperCase();
@@ -183,7 +185,9 @@ export async function runNonInteractive(
} else if (config.getOutputFormat() === OutputFormat.JSON) {
responseText += event.value;
} else {
process.stdout.write(event.value);
if (event.value) {
textOutput.write(event.value);
}
}
} else if (event.type === GeminiEventType.ToolCallRequest) {
if (streamFormatter) {
@@ -220,6 +224,7 @@ export async function runNonInteractive(
}
if (toolCallRequests.length > 0) {
textOutput.ensureTrailingNewline();
const toolResponseParts: Part[] = [];
const completedToolCalls: CompletedToolCall[] = [];
@@ -297,9 +302,9 @@ export async function runNonInteractive(
} else if (config.getOutputFormat() === OutputFormat.JSON) {
const formatter = new JsonFormatter();
const stats = uiTelemetryService.getMetrics();
process.stdout.write(formatter.format(responseText, stats));
textOutput.write(formatter.format(responseText, stats));
} else {
process.stdout.write('\n'); // Ensure a final newline
textOutput.ensureTrailingNewline(); // Ensure a final newline
}
return;
}

View File

@@ -0,0 +1,23 @@
// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html
exports[`TextOutput > should correctly handle ANSI escape codes when determining line breaks 1`] = `
"hello
world
next"
`;
exports[`TextOutput > should handle ANSI codes that do not end with a newline 1`] = `
"hello
world"
`;
exports[`TextOutput > should handle a sequence of calls correctly 1`] = `
"first
second part
third"
`;
exports[`TextOutput > should handle empty strings with ANSI codes 1`] = `
"hello
world"
`;

View File

@@ -0,0 +1,99 @@
/**
* @license
* Copyright 2025 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
/// <reference types="vitest/globals" />
import type { MockInstance } from 'vitest';
import { vi } from 'vitest';
import { TextOutput } from './textOutput.js';
describe('TextOutput', () => {
let stdoutSpy: MockInstance<typeof process.stdout.write>;
let textOutput: TextOutput;
beforeEach(() => {
stdoutSpy = vi
.spyOn(process.stdout, 'write')
.mockImplementation(() => true);
textOutput = new TextOutput();
});
afterEach(() => {
stdoutSpy.mockRestore();
});
const getWrittenOutput = () => stdoutSpy.mock.calls.map((c) => c[0]).join('');
it('write() should call process.stdout.write', () => {
textOutput.write('hello');
expect(stdoutSpy).toHaveBeenCalledWith('hello');
});
it('write() should not call process.stdout.write for empty strings', () => {
textOutput.write('');
expect(stdoutSpy).not.toHaveBeenCalled();
});
it('writeOnNewLine() should not add a newline if the last char was a newline', () => {
// Default state starts at the beginning of a line
textOutput.writeOnNewLine('hello');
expect(getWrittenOutput()).toBe('hello');
});
it('writeOnNewLine() should add a newline if the last char was not a newline', () => {
textOutput.write('previous');
textOutput.writeOnNewLine('hello');
expect(getWrittenOutput()).toBe('previous\nhello');
});
it('ensureTrailingNewline() should add a newline if one is missing', () => {
textOutput.write('hello');
textOutput.ensureTrailingNewline();
expect(getWrittenOutput()).toBe('hello\n');
});
it('ensureTrailingNewline() should not add a newline if one already exists', () => {
textOutput.write('hello\n');
textOutput.ensureTrailingNewline();
expect(getWrittenOutput()).toBe('hello\n');
});
it('should handle a sequence of calls correctly', () => {
textOutput.write('first');
textOutput.writeOnNewLine('second');
textOutput.write(' part');
textOutput.ensureTrailingNewline();
textOutput.ensureTrailingNewline(); // second call should do nothing
textOutput.write('third');
expect(getWrittenOutput()).toMatchSnapshot();
});
it('should correctly handle ANSI escape codes when determining line breaks', () => {
const blue = (s: string) => `\u001b[34m${s}\u001b[39m`;
const bold = (s: string) => `\u001b[1m${s}\u001b[22m`;
textOutput.write(blue('hello'));
textOutput.writeOnNewLine(bold('world'));
textOutput.write(blue('\n'));
textOutput.writeOnNewLine('next');
expect(getWrittenOutput()).toMatchSnapshot();
});
it('should handle empty strings with ANSI codes', () => {
textOutput.write('hello');
textOutput.write('\u001b[34m\u001b[39m'); // Empty blue string
textOutput.writeOnNewLine('world');
expect(getWrittenOutput()).toMatchSnapshot();
});
it('should handle ANSI codes that do not end with a newline', () => {
textOutput.write('hello\u001b[34m');
textOutput.writeOnNewLine('world');
expect(getWrittenOutput()).toMatchSnapshot();
});
});

View File

@@ -0,0 +1,54 @@
/**
* @license
* Copyright 2025 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
/**
* A utility to manage writing text to stdout, ensuring that newlines
* are handled consistently and robustly across the application.
*/
import stripAnsi from 'strip-ansi';
export class TextOutput {
private atStartOfLine = true;
/**
* Writes a string to stdout.
* @param str The string to write.
*/
write(str: string): void {
if (str.length === 0) {
return;
}
process.stdout.write(str);
const strippedStr = stripAnsi(str);
if (strippedStr.length > 0) {
this.atStartOfLine = strippedStr.endsWith('\n');
}
}
/**
* Writes a string to stdout, ensuring it starts on a new line.
* If the previous output did not end with a newline, one will be added.
* This prevents adding extra blank lines if a newline already exists.
* @param str The string to write.
*/
writeOnNewLine(str: string): void {
if (!this.atStartOfLine) {
this.write('\n');
}
this.write(str);
}
/**
* Ensures that the output ends with a newline. If the last character
* written was not a newline, one will be added.
*/
ensureTrailingNewline(): void {
if (!this.atStartOfLine) {
this.write('\n');
}
}
}