Skip to content

Commit 1deddcd

Browse files
easonLiangWorldedtecheasonLiangWorldedtech
committed
fix(checkpoints): journal the write before bookkeeping can strand it
EditFileTool and WriteToFileTool both ran trackFileContext and pushToolWriteResult between the write landing and checkpointSave, so a throw from either post-write call left a completed write with no checkpoint and no journal entry. The checkpoint block moves whole to sit directly after the write branches in each file, with the perWriteCheckpoints guard and the approval diff statistics already in scope, and no line of the block changed. The no-op update test in the patch tool specification now asserts the journal argument the call actually carries, an empty entry list, instead of only the call count.
1 parent aa86dc7 commit 1deddcd

3 files changed

Lines changed: 25 additions & 24 deletions

File tree

‎src/core/tools/EditFileTool.ts‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -451,20 +451,6 @@ export class EditFileTool extends BaseTool<"edit_file"> {
451451
await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs)
452452
}
453453

454-
// Track file edit operation
455-
if (relPath) {
456-
await task.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource)
457-
}
458-
459-
task.didEditFile = true
460-
461-
// Get the formatted response message
462-
const replacementInfo =
463-
!isNewFile && expected_replacements > 1 ? ` (${expected_replacements} replacements)` : ""
464-
const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, isNewFile)
465-
466-
pushToolResult(message + replacementInfo)
467-
468454
if (perWriteCheckpoints) {
469455
// B2: the change-journal entry for this edit is appended inside
470456
// checkpointSave (the hook stays a single call site), keyed by the
@@ -480,6 +466,20 @@ export class EditFileTool extends BaseTool<"edit_file"> {
480466
}).catch(() => {})
481467
}
482468

469+
// Track file edit operation
470+
if (relPath) {
471+
await task.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource)
472+
}
473+
474+
task.didEditFile = true
475+
476+
// Get the formatted response message
477+
const replacementInfo =
478+
!isNewFile && expected_replacements > 1 ? ` (${expected_replacements} replacements)` : ""
479+
const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, isNewFile)
480+
481+
pushToolResult(message + replacementInfo)
482+
483483
await task.diffViewProvider.reset()
484484
this.resetPartialState()
485485

‎src/core/tools/WriteToFileTool.ts‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -175,16 +175,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
175175
await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs)
176176
}
177177

178-
if (relPath) {
179-
await task.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource)
180-
}
181-
182-
task.didEditFile = true
183-
184-
const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, !fileExists)
185-
186-
pushToolResult(message)
187-
188178
if (perWriteCheckpoints) {
189179
// B2: the change-journal entry for this write is appended inside
190180
// checkpointSave (the hook stays a single call site), keyed by the
@@ -200,6 +190,16 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
200190
}).catch(() => {})
201191
}
202192

193+
if (relPath) {
194+
await task.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource)
195+
}
196+
197+
task.didEditFile = true
198+
199+
const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, !fileExists)
200+
201+
pushToolResult(message)
202+
203203
await task.diffViewProvider.reset()
204204
this.resetPartialState()
205205

‎src/core/tools/__tests__/applyPatchTool.execute.spec.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -381,6 +381,7 @@ describe("ApplyPatchTool.execute - delete file success path", () => {
381381

382382
expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("No changes needed"))
383383
expect(mockedCheckpointSave).toHaveBeenCalledOnce()
384+
expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [])
384385
})
385386

386387
it("does not record a checkpoint when the user rejects the update", async () => {

0 commit comments

Comments
 (0)