Fix Gemini Code's (GC) smarts.

- The tl;dr; is that GC couldn't see what the user was saying when tool call events happened in response. The rason why this was happening was because we were instantly invoking tools that the model told us to invoke and then instantly re-requesting. This resulted in the bug because the genai APIs can't update the chat history before a full response has been completed (doesn't know how to update if it's incomplete).
- To address the above issue I had to do quite the large refactor. The gist is that now turns truly drive everything on the server (vs. a server client split). This ensured that when we got tool invocations we could control when/how re-requesting would happen and then also ensure that history was updated. This change also meant that the server would act as an event publisher to enable the client to react to events rather than try and weave in complex logic between the events.
- A BIG change that this changeset incudes is the removal of all of the CLI tools in favor of the server tools.
- Removed some dead code as part of this
- **NOTE: Confirmations are still broken (they were broken prior to this); however, I've set them up to be able to work in the future, I'll dot hat in a follow up to be less breaking to others.**

Fixes https://b.corp.google.com/issues/412320087
This commit is contained in:
Taylor Mullen
2025-04-21 10:53:11 -04:00
committed by N. Taylor Mullen
parent e351baf10f
commit 81f0f618f7
27 changed files with 1283 additions and 2331 deletions

View File

@@ -7,7 +7,14 @@
import fs from 'fs';
import path from 'path';
import * as Diff from 'diff';
import { BaseTool, ToolResult, ToolResultDisplay } from './tools.js';
import {
BaseTool,
ToolCallConfirmationDetails,
ToolConfirmationOutcome,
ToolEditConfirmationDetails,
ToolResult,
ToolResultDisplay,
} from './tools.js';
import { SchemaValidator } from '../utils/schemaValidator.js';
import { makeRelative, shortenPath } from '../utils/paths.js';
import { isNodeError } from '../utils/errors.js';
@@ -48,8 +55,9 @@ interface CalculatedEdit {
/**
* Implementation of the Edit tool logic (moved from CLI)
*/
export class EditLogic extends BaseTool<EditToolParams, ToolResult> {
export class EditTool extends BaseTool<EditToolParams, ToolResult> {
static readonly Name = 'replace'; // Keep static name
private shouldAlwaysEdit = false;
private readonly rootDirectory: string;
@@ -61,9 +69,9 @@ export class EditLogic extends BaseTool<EditToolParams, ToolResult> {
// Note: The description here mentions other tools like ReadFileTool/WriteFileTool
// by name. This might need updating if those tool names change.
super(
EditLogic.Name,
'', // Display name handled by CLI wrapper
'', // Description handled by CLI wrapper
EditTool.Name,
'Edit',
'Replaces a SINGLE, UNIQUE occurrence of text within a file. Requires providing significant context around the change to ensure uniqueness. For moving/renaming files, use the Bash tool with `mv`. For replacing entire file contents or creating new files use the WriteFile tool. Always use the ReadFile tool to examine the file before using this tool.',
{
properties: {
file_path: {
@@ -225,7 +233,82 @@ export class EditLogic extends BaseTool<EditToolParams, ToolResult> {
};
}
// Removed shouldConfirmExecute - Confirmation is handled by the CLI wrapper
/**
* Handles the confirmation prompt for the Edit tool in the CLI.
* It needs to calculate the diff to show the user.
*/
async shouldConfirmExecute(
params: EditToolParams,
): Promise<ToolCallConfirmationDetails | false> {
if (this.shouldAlwaysEdit) {
return false;
}
const validationError = this.validateToolParams(params);
if (validationError) {
console.error(
`[EditTool Wrapper] Attempted confirmation with invalid parameters: ${validationError}`,
);
return false;
}
let currentContent: string | null = null;
let fileExists = false;
let newContent = '';
try {
currentContent = fs.readFileSync(params.file_path, 'utf8');
fileExists = true;
} catch (err: unknown) {
if (isNodeError(err) && err.code === 'ENOENT') {
fileExists = false;
} else {
console.error(`Error reading file for confirmation diff: ${err}`);
return false;
}
}
if (params.old_string === '' && !fileExists) {
newContent = params.new_string;
} else if (!fileExists) {
return false;
} else if (currentContent !== null) {
const occurrences = this.countOccurrences(
currentContent,
params.old_string,
);
const expectedReplacements =
params.expected_replacements === undefined
? 1
: params.expected_replacements;
if (occurrences === 0 || occurrences !== expectedReplacements) {
return false;
}
newContent = this.replaceAll(
currentContent,
params.old_string,
params.new_string,
);
} else {
return false;
}
const fileName = path.basename(params.file_path);
const fileDiff = Diff.createPatch(
fileName,
currentContent ?? '',
newContent,
'Current',
'Proposed',
{ context: 3 },
);
const confirmationDetails: ToolEditConfirmationDetails = {
title: `Confirm Edit: ${shortenPath(makeRelative(params.file_path, this.rootDirectory))}`,
fileName,
fileDiff,
onConfirm: async (outcome: ToolConfirmationOutcome) => {
if (outcome === ToolConfirmationOutcome.ProceedAlways) {
this.shouldAlwaysEdit = true;
}
},
};
return confirmationDetails;
}
getDescription(params: EditToolParams): string {
const relativePath = makeRelative(params.file_path, this.rootDirectory);

View File

@@ -29,7 +29,7 @@ export interface GlobToolParams {
/**
* Implementation of the Glob tool logic (moved from CLI)
*/
export class GlobLogic extends BaseTool<GlobToolParams, ToolResult> {
export class GlobTool extends BaseTool<GlobToolParams, ToolResult> {
static readonly Name = 'glob'; // Keep static name
/**
@@ -43,9 +43,9 @@ export class GlobLogic extends BaseTool<GlobToolParams, ToolResult> {
*/
constructor(rootDirectory: string) {
super(
GlobLogic.Name,
'', // Display name handled by CLI wrapper
'', // Description handled by CLI wrapper
GlobTool.Name,
'FindFiles', // Display name handled by CLI wrapper
'Efficiently finds files matching specific glob patterns (e.g., `src/**/*.ts`, `**/*.md`), returning absolute paths sorted by modification time (newest first). Ideal for quickly locating files based on their name or path structure, especially in large codebases.', // Description handled by CLI wrapper
{
properties: {
pattern: {

View File

@@ -51,7 +51,7 @@ interface GrepMatch {
/**
* Implementation of the Grep tool logic (moved from CLI)
*/
export class GrepLogic extends BaseTool<GrepToolParams, ToolResult> {
export class GrepTool extends BaseTool<GrepToolParams, ToolResult> {
static readonly Name = 'search_file_content'; // Keep static name
private rootDirectory: string;
@@ -62,9 +62,9 @@ export class GrepLogic extends BaseTool<GrepToolParams, ToolResult> {
*/
constructor(rootDirectory: string) {
super(
GrepLogic.Name,
'', // Display name handled by CLI wrapper
'', // Description handled by CLI wrapper
GrepTool.Name,
'SearchText',
'Searches for a regular expression pattern within the content of files in a specified directory (or current working directory). Can filter files by a glob pattern. Returns the lines containing matches, along with their file paths and line numbers.',
{
properties: {
pattern: {

View File

@@ -58,7 +58,7 @@ export interface FileEntry {
/**
* Implementation of the LS tool logic
*/
export class LSLogic extends BaseTool<LSToolParams, ToolResult> {
export class LSTool extends BaseTool<LSToolParams, ToolResult> {
static readonly Name = 'list_directory';
/**
@@ -73,9 +73,9 @@ export class LSLogic extends BaseTool<LSToolParams, ToolResult> {
*/
constructor(rootDirectory: string) {
super(
LSLogic.Name,
'', // Display name handled by CLI wrapper
'', // Description handled by CLI wrapper
LSTool.Name,
'ReadFolder',
'Lists the names of files and subdirectories directly within a specified directory path. Can optionally ignore entries matching provided glob patterns.',
{
properties: {
path: {

View File

@@ -33,7 +33,7 @@ export interface ReadFileToolParams {
/**
* Implementation of the ReadFile tool logic
*/
export class ReadFileLogic extends BaseTool<ReadFileToolParams, ToolResult> {
export class ReadFileTool extends BaseTool<ReadFileToolParams, ToolResult> {
static readonly Name: string = 'read_file';
private static readonly DEFAULT_MAX_LINES = 2000;
private static readonly MAX_LINE_LENGTH = 2000;
@@ -41,9 +41,9 @@ export class ReadFileLogic extends BaseTool<ReadFileToolParams, ToolResult> {
constructor(rootDirectory: string) {
super(
ReadFileLogic.Name,
'', // Display name handled by CLI wrapper
'', // Description handled by CLI wrapper
ReadFileTool.Name,
'ReadFile',
'Reads and returns the content of a specified file from the local filesystem. Handles large files by allowing reading specific line ranges.',
{
properties: {
path: {
@@ -236,16 +236,15 @@ export class ReadFileLogic extends BaseTool<ReadFileToolParams, ToolResult> {
const startLine = params.offset || 0;
const endLine = params.limit
? startLine + params.limit
: Math.min(startLine + ReadFileLogic.DEFAULT_MAX_LINES, lines.length);
: Math.min(startLine + ReadFileTool.DEFAULT_MAX_LINES, lines.length);
const selectedLines = lines.slice(startLine, endLine);
let truncated = false;
const formattedLines = selectedLines.map((line) => {
let processedLine = line;
if (line.length > ReadFileLogic.MAX_LINE_LENGTH) {
if (line.length > ReadFileTool.MAX_LINE_LENGTH) {
processedLine =
line.substring(0, ReadFileLogic.MAX_LINE_LENGTH) +
'... [truncated]';
line.substring(0, ReadFileTool.MAX_LINE_LENGTH) + '... [truncated]';
truncated = true;
}

File diff suppressed because it is too large Load Diff

View File

@@ -49,7 +49,14 @@ export interface Tool<
*/
getDescription(params: TParams): string;
// Removed shouldConfirmExecute as it's UI-specific
/**
* Determines if the tool should prompt for confirmation before execution
* @param params Parameters for the tool execution
* @returns Whether execute should be confirmed.
*/
shouldConfirmExecute(
params: TParams,
): Promise<ToolCallConfirmationDetails | false>;
/**
* Executes the tool with the given parameters
@@ -115,7 +122,17 @@ export abstract class BaseTool<
return JSON.stringify(params);
}
// Removed shouldConfirmExecute as it's UI-specific
/**
* Determines if the tool should prompt for confirmation before execution
* @param params Parameters for the tool execution
* @returns Whether or not execute should be confirmed by the user.
*/
shouldConfirmExecute(
// eslint-disable-next-line @typescript-eslint/no-unused-vars
params: TParams,
): Promise<ToolCallConfirmationDetails | false> {
return Promise.resolve(false);
}
/**
* Abstract method to execute the tool with the given parameters
@@ -148,3 +165,27 @@ export type ToolResultDisplay = string | FileDiff;
export interface FileDiff {
fileDiff: string;
}
export interface ToolCallConfirmationDetails {
title: string;
onConfirm: (outcome: ToolConfirmationOutcome) => Promise<void>;
}
export interface ToolEditConfirmationDetails
extends ToolCallConfirmationDetails {
fileName: string;
fileDiff: string;
}
export interface ToolExecuteConfirmationDetails
extends ToolCallConfirmationDetails {
command: string;
rootCommand: string;
description: string;
}
export enum ToolConfirmationOutcome {
ProceedOnce,
ProceedAlways,
Cancel,
}

View File

@@ -21,14 +21,14 @@ export interface WebFetchToolParams {
/**
* Implementation of the WebFetch tool logic (moved from CLI)
*/
export class WebFetchLogic extends BaseTool<WebFetchToolParams, ToolResult> {
export class WebFetchTool extends BaseTool<WebFetchToolParams, ToolResult> {
static readonly Name: string = 'web_fetch';
constructor() {
super(
WebFetchLogic.Name,
'', // Display name handled by CLI wrapper
'', // Description handled by CLI wrapper
WebFetchTool.Name,
'WebFetch',
'Fetches text content from a given URL. Handles potential network errors and non-success HTTP status codes.',
{
properties: {
url: {

View File

@@ -7,7 +7,14 @@
import fs from 'fs';
import path from 'path';
import * as Diff from 'diff'; // Keep for result generation
import { BaseTool, ToolResult, FileDiff } from './tools.js'; // Updated import (Removed ToolResultDisplay)
import {
BaseTool,
ToolResult,
FileDiff,
ToolEditConfirmationDetails,
ToolConfirmationOutcome,
ToolCallConfirmationDetails,
} from './tools.js'; // Updated import (Removed ToolResultDisplay)
import { SchemaValidator } from '../utils/schemaValidator.js'; // Updated import
import { makeRelative, shortenPath } from '../utils/paths.js'; // Updated import
import { isNodeError } from '../utils/errors.js'; // Import isNodeError
@@ -30,16 +37,17 @@ export interface WriteFileToolParams {
/**
* Implementation of the WriteFile tool logic (moved from CLI)
*/
export class WriteFileLogic extends BaseTool<WriteFileToolParams, ToolResult> {
export class WriteFileTool extends BaseTool<WriteFileToolParams, ToolResult> {
static readonly Name: string = 'write_file';
private shouldAlwaysWrite = false;
private readonly rootDirectory: string;
constructor(rootDirectory: string) {
super(
WriteFileLogic.Name,
'', // Display name handled by CLI wrapper
'', // Description handled by CLI wrapper
WriteFileTool.Name,
'WriteFile',
'Writes content to a specified file in the local filesystem.',
{
properties: {
file_path: {
@@ -98,6 +106,56 @@ export class WriteFileLogic extends BaseTool<WriteFileToolParams, ToolResult> {
return `Writing to ${shortenPath(relativePath)}`;
}
/**
* Handles the confirmation prompt for the WriteFile tool in the CLI.
*/
async shouldConfirmExecute(
params: WriteFileToolParams,
): Promise<ToolCallConfirmationDetails | false> {
if (this.shouldAlwaysWrite) {
return false;
}
const validationError = this.validateToolParams(params);
if (validationError) {
console.error(
`[WriteFile Wrapper] Attempted confirmation with invalid parameters: ${validationError}`,
);
return false;
}
const relativePath = makeRelative(params.file_path, this.rootDirectory);
const fileName = path.basename(params.file_path);
let currentContent = '';
try {
currentContent = fs.readFileSync(params.file_path, 'utf8');
} catch {
// File might not exist, that's okay for write/create
}
const fileDiff = Diff.createPatch(
fileName,
currentContent,
params.content,
'Current',
'Proposed',
{ context: 3 },
);
const confirmationDetails: ToolEditConfirmationDetails = {
title: `Confirm Write: ${shortenPath(relativePath)}`,
fileName,
fileDiff,
onConfirm: async (outcome: ToolConfirmationOutcome) => {
if (outcome === ToolConfirmationOutcome.ProceedAlways) {
this.shouldAlwaysWrite = true;
}
},
};
return confirmationDetails;
}
async execute(params: WriteFileToolParams): Promise<ToolResult> {
const validationError = this.validateParams(params);
if (validationError) {