Skip to content

Commit 327eef8

Browse files
committed
Fco edge case fixes and tests (final push until review)
- folders were getting diffed, added a check to prevent this. - .roo now excluded from checkpoint diffs - diffs for FCO now only check line changes vs full diff changes. This is a preventative measure due to search and replace calls rewriting the entire file. - tests for these edge cases added.
1 parent 5e6b158 commit 327eef8

File tree

6 files changed

+219
-58
lines changed

6 files changed

+219
-58
lines changed

src/core/checkpoints/index.ts

Lines changed: 72 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -160,44 +160,80 @@ export function getCheckpointService(cline: Task) {
160160
try {
161161
const checkpointFileChangeManager = provider?.getFileChangeManager()
162162
if (checkpointFileChangeManager) {
163-
// Get the initial baseline (preserve for cumulative diff tracking)
164-
let initialBaseline = checkpointFileChangeManager.getChanges().baseCheckpoint
163+
// Get the current baseline for cumulative tracking
164+
let currentBaseline = checkpointFileChangeManager.getChanges().baseCheckpoint
165165

166-
// Validate that the baseline exists in the shadow repository before attempting diff
167-
// If the baseline doesn't exist (e.g., from a previous task session), fall back to shadow repo base
166+
// For cumulative tracking, we want to calculate from baseline to new checkpoint
167+
// But if this is the first time or baseline is invalid, update it to fromHash
168168
try {
169-
await service.getDiff({ from: initialBaseline, to: initialBaseline })
169+
await service.getDiff({ from: currentBaseline, to: currentBaseline })
170+
log(
171+
`[Task#checkpointCreated] Using existing baseline ${currentBaseline} for cumulative tracking`,
172+
)
170173
} catch (baselineValidationError) {
171-
const shadowBase = service.baseHash || toHash
174+
// Baseline is invalid, use fromHash as the new baseline for cumulative tracking
172175
log(
173-
`[Task#checkpointCreated] Initial baseline ${initialBaseline} not found in shadow repo, falling back to shadow base ${shadowBase}`,
176+
`[Task#checkpointCreated] Baseline validation failed for ${currentBaseline}: ${baselineValidationError instanceof Error ? baselineValidationError.message : String(baselineValidationError)}`,
174177
)
175-
initialBaseline = shadowBase
176-
// Update FileChangeManager baseline to match shadow repository
177-
await checkpointFileChangeManager.updateBaseline(initialBaseline)
178+
log(`[Task#checkpointCreated] Updating baseline to fromHash: ${fromHash}`)
179+
currentBaseline = fromHash
180+
// Update FileChangeManager baseline to match
181+
try {
182+
await checkpointFileChangeManager.updateBaseline(currentBaseline)
183+
log(`[Task#checkpointCreated] Successfully updated baseline to ${currentBaseline}`)
184+
} catch (updateError) {
185+
log(
186+
`[Task#checkpointCreated] Failed to update baseline: ${updateError instanceof Error ? updateError.message : String(updateError)}`,
187+
)
188+
throw updateError
189+
}
178190
}
179191

180192
log(
181-
`[Task#checkpointCreated] Calculating cumulative changes from validated baseline ${initialBaseline} to ${toHash}`,
193+
`[Task#checkpointCreated] Calculating cumulative changes from baseline ${currentBaseline} to ${toHash}`,
182194
)
183195

184-
// Calculate cumulative diff from validated baseline to new checkpoint using checkpoint service
185-
const changes = await service.getDiff({ from: initialBaseline, to: toHash })
196+
// Calculate cumulative diff from baseline to new checkpoint using checkpoint service
197+
const changes = await service.getDiff({ from: currentBaseline, to: toHash })
186198

187199
if (changes && changes.length > 0) {
188200
// Convert to FileChange format with correct checkpoint references
189-
const fileChanges = changes.map((change: any) => ({
190-
uri: change.paths.relative,
191-
type: (change.paths.newFile
192-
? "create"
193-
: change.paths.deletedFile
194-
? "delete"
195-
: "edit") as FileChangeType,
196-
fromCheckpoint: initialBaseline, // Always reference initial baseline for cumulative view
197-
toCheckpoint: toHash, // Current checkpoint for comparison
198-
linesAdded: change.content.after ? change.content.after.split("\n").length : 0,
199-
linesRemoved: change.content.before ? change.content.before.split("\n").length : 0,
200-
}))
201+
const fileChanges = changes.map((change: any) => {
202+
const type = (
203+
change.paths.newFile ? "create" : change.paths.deletedFile ? "delete" : "edit"
204+
) as FileChangeType
205+
206+
// Calculate actual line differences for the change
207+
let linesAdded = 0
208+
let linesRemoved = 0
209+
210+
if (type === "create") {
211+
// New file: all lines are added
212+
linesAdded = change.content.after ? change.content.after.split("\n").length : 0
213+
linesRemoved = 0
214+
} else if (type === "delete") {
215+
// Deleted file: all lines are removed
216+
linesAdded = 0
217+
linesRemoved = change.content.before ? change.content.before.split("\n").length : 0
218+
} else {
219+
// Modified file: use FileChangeManager's improved calculation method
220+
const lineDifferences = FileChangeManager.calculateLineDifferences(
221+
change.content.before || "",
222+
change.content.after || "",
223+
)
224+
linesAdded = lineDifferences.linesAdded
225+
linesRemoved = lineDifferences.linesRemoved
226+
}
227+
228+
return {
229+
uri: change.paths.relative,
230+
type,
231+
fromCheckpoint: currentBaseline, // Reference current baseline for cumulative view
232+
toCheckpoint: toHash, // Current checkpoint for comparison
233+
linesAdded,
234+
linesRemoved,
235+
}
236+
})
201237

202238
log(`[Task#checkpointCreated] Found ${fileChanges.length} cumulative file changes`)
203239

@@ -228,13 +264,13 @@ export function getCheckpointService(cline: Task) {
228264
filesChanged: serializableChangeset,
229265
})
230266
} else {
231-
log(`[Task#checkpointCreated] No changes found between ${initialBaseline} and ${toHash}`)
267+
log(`[Task#checkpointCreated] No changes found between ${currentBaseline} and ${toHash}`)
232268
}
233269

234-
// DON'T update the baseline - keep it at initial baseline for cumulative tracking
270+
// DON'T update the baseline - keep it at current baseline for cumulative tracking
235271
// The baseline should only change when explicitly requested (e.g., checkpoint restore)
236272
log(
237-
`[Task#checkpointCreated] Keeping FileChangeManager baseline at ${initialBaseline} for cumulative tracking`,
273+
`[Task#checkpointCreated] Keeping FileChangeManager baseline at ${currentBaseline} for cumulative tracking`,
238274
)
239275
}
240276
} catch (error) {
@@ -419,12 +455,14 @@ export async function checkpointRestore(cline: Task, { ts, commitHash, mode }: C
419455
provider?.log(`[checkpointRestore] Cleared accept/reject state for fresh start`)
420456
}
421457

422-
// Calculate and send current changes (should be empty immediately after restore)
423-
const changes = fileChangeManager.getChanges()
424-
provider?.postMessageToWebview({
425-
type: "filesChanged",
426-
filesChanged: changes.files.length > 0 ? changes : undefined,
427-
})
458+
// Calculate and send current changes with LLM-only filtering (should be empty immediately after restore)
459+
if (cline.taskId && cline.fileContextTracker) {
460+
const changes = await fileChangeManager.getLLMOnlyChanges(cline.taskId, cline.fileContextTracker)
461+
provider?.postMessageToWebview({
462+
type: "filesChanged",
463+
filesChanged: changes.files.length > 0 ? changes : undefined,
464+
})
465+
}
428466
}
429467
} catch (error) {
430468
provider?.log(`[checkpointRestore] Failed to update FileChangeManager baseline: ${error}`)

src/services/checkpoints/ShadowCheckpointService.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -308,6 +308,17 @@ export abstract class ShadowCheckpointService extends EventEmitter {
308308
for (const file of files) {
309309
const relPath = file.file
310310
const absPath = path.join(cwdPath, relPath)
311+
312+
// Filter out directories - only include actual files
313+
try {
314+
const stat = await fs.stat(absPath)
315+
if (stat.isDirectory()) {
316+
continue // Skip directories
317+
}
318+
} catch {
319+
// If file doesn't exist (deleted files), continue processing
320+
}
321+
311322
const before = await this.git.show([`${from}:${relPath}`]).catch(() => "")
312323

313324
const after = await this.git.show([`${to ?? "HEAD"}:${relPath}`]).catch(() => "")

src/services/checkpoints/excludes.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,7 @@ const getLfsPatterns = async (workspacePath: string) => {
198198

199199
export const getExcludePatterns = async (workspacePath: string) => [
200200
".git/",
201+
".roo/",
201202
...getBuildArtifactPatterns(),
202203
...getMediaFilePatterns(),
203204
...getCacheFilePatterns(),

src/services/file-changes/FCOMessageHandler.ts

Lines changed: 36 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -44,10 +44,14 @@ export class FCOMessageHandler {
4444
if (!fileChangeManager) {
4545
fileChangeManager = await this.provider.ensureFileChangeManager()
4646
}
47-
if (fileChangeManager) {
47+
if (fileChangeManager && task?.taskId && task?.fileContextTracker) {
48+
const filteredChangeset = await fileChangeManager.getLLMOnlyChanges(
49+
task.taskId,
50+
task.fileContextTracker,
51+
)
4852
this.provider.postMessageToWebview({
4953
type: "filesChanged",
50-
filesChanged: fileChangeManager.getChanges(),
54+
filesChanged: filteredChangeset.files.length > 0 ? filteredChangeset : undefined,
5155
})
5256
}
5357
break
@@ -171,15 +175,19 @@ export class FCOMessageHandler {
171175
}
172176

173177
private async handleAcceptFileChange(message: WebviewMessage): Promise<void> {
178+
const task = this.provider.getCurrentCline()
174179
let acceptFileChangeManager = this.provider.getFileChangeManager()
175180
if (!acceptFileChangeManager) {
176181
acceptFileChangeManager = await this.provider.ensureFileChangeManager()
177182
}
178-
if (message.uri && acceptFileChangeManager) {
183+
if (message.uri && acceptFileChangeManager && task?.taskId && task?.fileContextTracker) {
179184
await acceptFileChangeManager.acceptChange(message.uri)
180185

181-
// Send updated state
182-
const updatedChangeset = acceptFileChangeManager.getChanges()
186+
// Send updated state with LLM-only filtering
187+
const updatedChangeset = await acceptFileChangeManager.getLLMOnlyChanges(
188+
task.taskId,
189+
task.fileContextTracker,
190+
)
183191
this.provider.postMessageToWebview({
184192
type: "filesChanged",
185193
filesChanged: updatedChangeset.files.length > 0 ? updatedChangeset : undefined,
@@ -225,13 +233,18 @@ export class FCOMessageHandler {
225233
// Remove from tracking since the file has been reverted
226234
await rejectFileChangeManager.rejectChange(message.uri)
227235

228-
// Send updated state
229-
const updatedChangeset = rejectFileChangeManager.getChanges()
230-
console.log(`[FCO] After rejection, sending ${updatedChangeset.files.length} files to webview`)
231-
this.provider.postMessageToWebview({
232-
type: "filesChanged",
233-
filesChanged: updatedChangeset.files.length > 0 ? updatedChangeset : undefined,
234-
})
236+
// Send updated state with LLM-only filtering
237+
if (currentTask?.taskId && currentTask?.fileContextTracker) {
238+
const updatedChangeset = await rejectFileChangeManager.getLLMOnlyChanges(
239+
currentTask.taskId,
240+
currentTask.fileContextTracker,
241+
)
242+
console.log(`[FCO] After rejection, sending ${updatedChangeset.files.length} LLM-only files to webview`)
243+
this.provider.postMessageToWebview({
244+
type: "filesChanged",
245+
filesChanged: updatedChangeset.files.length > 0 ? updatedChangeset : undefined,
246+
})
247+
}
235248
} catch (error) {
236249
console.error(`[FCO] Error reverting file ${message.uri}:`, error)
237250
// Fall back to old behavior (just remove from display) if reversion fails
@@ -341,12 +354,17 @@ export class FCOMessageHandler {
341354
fileChangeManager.setFiles(fileChanges)
342355
}
343356

344-
// Get filtered changeset and send to webview
345-
const filteredChangeset = fileChangeManager.getChanges()
346-
this.provider.postMessageToWebview({
347-
type: "filesChanged",
348-
filesChanged: filteredChangeset.files.length > 0 ? filteredChangeset : undefined,
349-
})
357+
// Get filtered changeset with LLM-only filtering and send to webview
358+
if (task?.taskId && task?.fileContextTracker) {
359+
const filteredChangeset = await fileChangeManager.getLLMOnlyChanges(
360+
task.taskId,
361+
task.fileContextTracker,
362+
)
363+
this.provider.postMessageToWebview({
364+
type: "filesChanged",
365+
filesChanged: filteredChangeset.files.length > 0 ? filteredChangeset : undefined,
366+
})
367+
}
350368
} else {
351369
this.provider.postMessageToWebview({
352370
type: "filesChanged",

src/services/file-changes/FileChangeManager.ts

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -145,17 +145,40 @@ export class FileChangeManager {
145145

146146
/**
147147
* Calculate line differences between two file contents
148+
* Uses a simple line-by-line comparison to count actual changes
148149
*/
149150
public static calculateLineDifferences(
150151
originalContent: string,
151152
newContent: string,
152153
): { linesAdded: number; linesRemoved: number } {
153-
const originalLines = originalContent.split("\n")
154-
const newLines = newContent.split("\n")
155-
156-
// Simple diff calculation
157-
const linesAdded = Math.max(0, newLines.length - originalLines.length)
158-
const linesRemoved = Math.max(0, originalLines.length - newLines.length)
154+
const originalLines = originalContent === "" ? [] : originalContent.split("\n")
155+
const newLines = newContent === "" ? [] : newContent.split("\n")
156+
157+
// For proper diff calculation, we need to compare line by line
158+
// This is a simplified approach that works well for most cases
159+
160+
const maxLines = Math.max(originalLines.length, newLines.length)
161+
let linesAdded = 0
162+
let linesRemoved = 0
163+
164+
// Compare each line position
165+
for (let i = 0; i < maxLines; i++) {
166+
const originalLine = i < originalLines.length ? originalLines[i] : undefined
167+
const newLine = i < newLines.length ? newLines[i] : undefined
168+
169+
if (originalLine === undefined && newLine !== undefined) {
170+
// Line was added
171+
linesAdded++
172+
} else if (originalLine !== undefined && newLine === undefined) {
173+
// Line was removed
174+
linesRemoved++
175+
} else if (originalLine !== newLine) {
176+
// Line was modified (count as both removed and added)
177+
linesRemoved++
178+
linesAdded++
179+
}
180+
// If lines are identical, no change
181+
}
159182

160183
return { linesAdded, linesRemoved }
161184
}

src/services/file-changes/__tests__/FileChangeManager.test.ts

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -324,6 +324,76 @@ describe("FileChangeManager (Simplified)", () => {
324324
expect(result.linesAdded).toBe(0)
325325
expect(result.linesRemoved).toBe(0)
326326
})
327+
328+
it("should handle line modifications (search and replace)", () => {
329+
const original = "function test() {\n return 'old';\n}"
330+
const modified = "function test() {\n return 'new';\n}"
331+
332+
const result = FileChangeManager.calculateLineDifferences(original, modified)
333+
334+
expect(result.linesAdded).toBe(1) // Modified line counts as added
335+
expect(result.linesRemoved).toBe(1) // Modified line counts as removed
336+
})
337+
338+
it("should handle mixed changes", () => {
339+
const original = "line1\nold_line\nline3"
340+
const modified = "line1\nnew_line\nline3\nextra_line"
341+
342+
const result = FileChangeManager.calculateLineDifferences(original, modified)
343+
344+
expect(result.linesAdded).toBe(2) // 1 modified + 1 added
345+
expect(result.linesRemoved).toBe(1) // 1 modified
346+
})
347+
348+
it("should handle empty original file", () => {
349+
const original = ""
350+
const modified = "line1\nline2\nline3"
351+
352+
const result = FileChangeManager.calculateLineDifferences(original, modified)
353+
354+
expect(result.linesAdded).toBe(3)
355+
expect(result.linesRemoved).toBe(0)
356+
})
357+
358+
it("should handle empty modified file", () => {
359+
const original = "line1\nline2\nline3"
360+
const modified = ""
361+
362+
const result = FileChangeManager.calculateLineDifferences(original, modified)
363+
364+
expect(result.linesAdded).toBe(0)
365+
expect(result.linesRemoved).toBe(3)
366+
})
367+
368+
it("should handle both files empty", () => {
369+
const original = ""
370+
const modified = ""
371+
372+
const result = FileChangeManager.calculateLineDifferences(original, modified)
373+
374+
expect(result.linesAdded).toBe(0)
375+
expect(result.linesRemoved).toBe(0)
376+
})
377+
378+
it("should handle single line files", () => {
379+
const original = "single line"
380+
const modified = "different line"
381+
382+
const result = FileChangeManager.calculateLineDifferences(original, modified)
383+
384+
expect(result.linesAdded).toBe(1)
385+
expect(result.linesRemoved).toBe(1)
386+
})
387+
388+
it("should handle whitespace-only changes", () => {
389+
const original = "line1\n indented\nline3"
390+
const modified = "line1\n indented\nline3"
391+
392+
const result = FileChangeManager.calculateLineDifferences(original, modified)
393+
394+
expect(result.linesAdded).toBe(1) // Whitespace change counts as modification
395+
expect(result.linesRemoved).toBe(1)
396+
})
327397
})
328398

329399
describe("getLLMOnlyChanges", () => {

0 commit comments

Comments
 (0)