feat: implement file system reversion utilities for rewind - #15715
Conversation
Summary of ChangesHello @Adib234, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new set of utility functions designed to manage and revert file system changes made within a conversational AI interaction. It provides mechanisms to calculate the scope and impact of modifications over single or multiple turns and implements a robust file reversion system that can either directly restore files or intelligently apply patches to undo changes, enhancing the reliability and user control over automated file operations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces new functionality for rewinding file operations and includes corresponding tests. The core logic for calculating turn statistics, rewind impact, and reverting file changes appears sound. The tests cover various scenarios, including single-turn stats, cumulative impact, exact reverts, and new file deletions. One high-severity issue was identified related to error logging, where an error object was not properly stringified in a user-facing message, hindering debugging.
Adib234
changed the title
8231747 to
588f46c
Compare
e81c7ea to
3da5a29
Compare
37c0ec4 to
b16120c
Compare
|
Size Change: +1.03 kB (0%) Total Size: 22.3 MB
ℹ️ View Unchanged
|
gemini-cli
Bot
added
the
status/need-issue
|
Here are some observations from my review:
|
| if ( | ||
| result && | ||
| typeof result === 'object' && | ||
| 'diffStat' in result && |
There was a problem hiding this comment.
Thanks for the suggestion, @jacob314! I agree that centralizing the logic for safely extracting FileDiff objects from toolCall.resultDisplay will significantly improve maintainability and testability.
I've created a new utility function getFileDiffFromResultDisplay in packages/core/src/utils/fileDiffUtils.ts (which would need to be added in a separate commit or as part of this PR's broader changes) that performs the necessary runtime checks to ensure the object conforms to the FileDiff structure. This helper can then be used to simplify the logic in calculateTurnStats, calculateRewindImpact, and revertFileChanges.
Here's the proposed content for the new utility file:
// packages/core/src/utils/fileDiffUtils.ts
import type { FileDiff, ToolCallRecord } from '../types';
/**
* Safely extracts the FileDiff object from a tool call's resultDisplay.
* This helper performs runtime checks to ensure the object conforms to the FileDiff structure.
* @param resultDisplay The resultDisplay property of a ToolCallRecord.
* @returns The FileDiff object if found and valid, otherwise undefined.
*/
export function getFileDiffFromResultDisplay(
resultDisplay: ToolCallRecord['resultDisplay'],
): FileDiff | undefined {
if (
resultDisplay &&
typeof resultDisplay === 'object' &&
'fileName' in resultDisplay &&
typeof resultDisplay.fileName === 'string' &&
'filePath' in resultDisplay &&
typeof resultDisplay.filePath === 'string' &&
'originalContent' in resultDisplay &&
('newContent' in resultDisplay && typeof resultDisplay.newContent === 'string') &&
('isNewFile' in resultDisplay && typeof resultDisplay.isNewFile === 'boolean') &&
'diffStat' in resultDisplay &&
typeof resultDisplay.diffStat === 'object' &&
resultDisplay.diffStat !== null
) {
const diffStat = resultDisplay.diffStat as FileDiff['diffStat'];
if (
typeof diffStat.model_added_lines === 'number' &&
typeof diffStat.model_removed_lines === 'number' &&
typeof diffStat.user_added_lines === 'number' &&
typeof diffStat.user_removed_lines === 'number' &&
typeof diffStat.model_added_chars === 'number' &&
typeof diffStat.model_removed_chars === 'number' &&
typeof diffStat.user_added_chars === 'number' &&
typeof diffStat.user_removed_chars === 'number'
) {
return resultDisplay as FileDiff;
}
}
return undefined;
}And here's a suggestion for updating the calculateTurnStats function and adding the new import statement in packages/cli/src/ui/utils/rewindFileOps.ts, which falls within the lines highlighted by your comment:
| 'diffStat' in result && | |
| /** | |
| * @license | |
| * Copyright 2025 Google LLC | |
| * SPDX-License-Identifier: Apache-2.0 | |
| */ | |
| import type { | |
| FileDiff, | |
| ConversationRecord, | |
| MessageRecord | |
| } from '@google/gemini-cli-core'; | |
| import fs from 'node:fs/promises'; | |
| import * as Diff from 'diff'; | |
| import { coreEvents } from '@google/gemini-cli-core'; | |
| import { getFileDiffFromResultDisplay } from '../../../core/src/utils/fileDiffUtils.js'; | |
| export interface FileChangeDetail { | |
| fileName: string; | |
| diff: string; | |
| } | |
| export interface FileChangeStats { | |
| addedLines: number; | |
| removedLines: number; | |
| fileCount: number; | |
| firstFileName: string; | |
| details?: FileChangeDetail[]; | |
| } | |
| /** | |
| * Calculates file change statistics for a single turn. | |
| * A turn is defined as the sequence of messages starting after the given user message | |
| * and continuing until the next user message or the end of the conversation. | |
| * | |
| * @param conversation The full conversation record. | |
| * @param userMessage The starting user message for the turn. | |
| * @returns Statistics about lines added/removed and files touched, or null if no edits occurred. | |
| */ | |
| export function calculateTurnStats( | |
| conversation: ConversationRecord, | |
| userMessage: MessageRecord, | |
| ): FileChangeStats | null { | |
| const msgIndex = conversation.messages.indexOf(userMessage); | |
| if (msgIndex === -1) return null; | |
| let addedLines = 0; | |
| let removedLines = 0; | |
| const files = new Set<string>(); | |
| let hasEdits = false; | |
| // Look ahead until the next user message (single turn) | |
| for (let i = msgIndex + 1; i < conversation.messages.length; i++) { | |
| const msg = conversation.messages[i]; | |
| if (msg.type === 'user') break; // Stop at next user message | |
| if (msg.type === 'gemini' && msg.toolCalls) { | |
| for (const toolCall of msg.toolCalls) { | |
| const fileDiff = getFileDiffFromResultDisplay(toolCall.resultDisplay); | |
| if (fileDiff) { | |
| hasEdits = true; | |
| const stats = fileDiff.diffStat; | |
| addedLines += stats.model_added_lines + stats.user_added_lines; | |
| removedLines += stats.model_removed_lines + stats.user_removed_lines; | |
| files.add(fileDiff.fileName); | |
| } | |
| } | |
| } | |
| } | |
There was a problem hiding this comment.
Created helper in packages/core/src/utils/fileDiffUtils.ts and used it in both places
| if (msg.type === 'gemini' && msg.toolCalls) { | ||
| for (const toolCall of msg.toolCalls) { | ||
| const result = toolCall.resultDisplay; | ||
| if ( |
There was a problem hiding this comment.
duplicate logic here and above computing added and removed lines
There was a problem hiding this comment.
Added helper for computing added and removed lines in fileDiffUtils.ts
| addedLines, | ||
| removedLines, | ||
| fileCount: files.size, | ||
| firstFileName: files.values().next().value as string, |
There was a problem hiding this comment.
why include just the first file name? a more standard api would be to include all the names and then callers can filter if they want.
There was a problem hiding this comment.
Removed firstFileName, callers can filter on details.fileName
| result && | ||
| typeof result === 'object' && | ||
| 'diffStat' in result && | ||
| 'fileName' in result && |
There was a problem hiding this comment.
are all these checks really needed? shouldn't just the diffStat one be enough or do we sometimes have diffStat without the other fields?
There was a problem hiding this comment.
Looks like diffStat and isNewFile are optional, I think I'll update my helper to check for diffStat only
| // File might not exist | ||
| coreEvents.emitFeedback( | ||
| 'error', | ||
| `File does not exist : ${e instanceof Error ? e.message : String(e)}`, |
There was a problem hiding this comment.
This message is not quite clear enough. indicate the file that we attempted to revert changes on doesn't exist and perhaps that the failure occurred as part of rewinding file operations.
Also, what about the case where the whole file was deleted? seems like in that case it is expect that the file doesn't exist.
There was a problem hiding this comment.
Changed message to make it more clear and handled the case where the whole file was deleted
| // File deleted by user, but we expected content. | ||
| coreEvents.emitFeedback( | ||
| 'warning', | ||
| `File ${result.fileName} missing, cannot revert.`, |
There was a problem hiding this comment.
these other feedback messages also need context so they don't confuse the user.
| ); | ||
| }); | ||
|
|
||
| it('deletes new file on revert', async () => { |
There was a problem hiding this comment.
also add a test for deleting a revert and add tests capturing the failure modes when a revert fails verifying that the correct feedback messages are sent.
jacob314
left a comment
jacob314
left a comment
There was a problem hiding this comment.
Approved after these comments are addressed.
gemini-cli
Bot
added
priority/p1
| `An unexpected error occurred while reverting ${fileName}.`, | ||
| e, | ||
| ); | ||
| debugLogger.error(`Unexpected error during file reversion:`, e); |
There was a problem hiding this comment.
don't log the same error to both debug logger and emitFeedback.
3 participants
Summary
This PR introduces essential utility functions for calculating the impact of and performing file system reversions during a chat "rewind." These utilities enable the CLI to estimate how many lines and files will be affected when jumping back in history and provide a mechanism to safely undo those changes, even if the user has made interleaved modifications.
Details
File Operation Impact Calculation
calculateTurnStats: Computes the file change statistics (added/removed lines, file count) for a single conversational turn.calculateRewindImpact: Performs a cumulative calculation of file change statistics from a specific target message to the current state of the conversation. This is used to inform the user of the potential consequences of a rewind.Robust File Reversion
Related Issues
After this PR gets merged here are the other PRs that will get merged in order
#15716
#15717
#15718
#15720
How to Validate
Pre-Merge Checklist