Skip to content

feat(checkpoints): wire change journal into write/edit/apply-patch tools (B2 v2-4, epic #1375) #48

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
easonLiangWorldedtech wants to merge 1 commit into
base: feat/fws-v2-3-journal-core
Choose a base branch
from
Open

feat(checkpoints): wire change journal into write/edit/apply-patch tools (B2 v2-4, epic #1375) #48

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
185 changes: 141 additions & 44 deletions src/core/tools/ApplyPatchTool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,18 @@ interface ApplyPatchParams {
patch: string
}

/**
* B2: result of a single file operation within a patch. `succeeded` controls
* the whole-patch success state (and therefore the per-patch checkpoint),
* while `wrote` records whether the operation actually wrote a file — a no-op
* update must not produce a change-journal entry for a file that was never
* written.
*/
interface ApplyPatchFileOpResult {
succeeded: boolean
wrote: boolean
}

export class ApplyPatchTool extends BaseTool<"apply_patch"> {
readonly name = "apply_patch" as const

Expand Down Expand Up @@ -104,9 +116,12 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
}

// Process each file change. The handlers report whether their file
// operation succeeded, so a rejected approval or a failed local write
// does not get checkpointed as if the patch had succeeded.
// operation succeeded (which controls the whole-patch checkpoint) and
// whether it actually wrote a file (which controls the change journal
// — a no-op update must not be journaled). A rejected approval or a
// failed local write never gets checkpointed as a success.
let patchSucceeded = true
const successfulChanges: ApplyPatchFileChange[] = []
for (const change of changes) {
const relPath = change.path
const absolutePath = path.resolve(task.cwd, relPath)
Expand All @@ -116,45 +131,97 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
if (!accessAllowed) {
await task.say("rooignore_error", relPath)
pushToolResult(formatResponse.rooIgnoreError(relPath))
return
// B2 partial flush: break, not return - an earlier hunk may have
// already written a file, and those writes must still receive the
// checkpoint, journal entry, and change card. Failing the patch
// also keeps the consecutive-mistake counter from resetting.
patchSucceeded = false
break
}

// Check if file is write-protected
const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath) || false

if (change.type === "add") {
// Create new file
patchSucceeded =
(await this.handleAddFile(change, absolutePath, relPath, task, callbacks, isWriteProtected)) &&
patchSucceeded
const addResult = await this.handleAddFile(
change,
absolutePath,
relPath,
task,
callbacks,
isWriteProtected,
)
patchSucceeded = addResult.succeeded && patchSucceeded
if (addResult.wrote) {
successfulChanges.push(change)
}
} else if (change.type === "delete") {
// Delete file
patchSucceeded =
(await this.handleDeleteFile(absolutePath, relPath, task, callbacks, isWriteProtected)) &&
patchSucceeded
const deleteResult = await this.handleDeleteFile(
absolutePath,
relPath,
task,
callbacks,
isWriteProtected,
)
patchSucceeded = deleteResult.succeeded && patchSucceeded
if (deleteResult.wrote) {
successfulChanges.push(change)
}
} else if (change.type === "update") {
// Update file
patchSucceeded =
(await this.handleUpdateFile(
change,
absolutePath,
relPath,
task,
callbacks,
isWriteProtected,
)) && patchSucceeded
// Update file (a no-op update succeeds without writing)
const updateResult = await this.handleUpdateFile(
change,
absolutePath,
relPath,
task,
callbacks,
isWriteProtected,
)
patchSucceeded = updateResult.succeeded && patchSucceeded
if (updateResult.wrote) {
successfulChanges.push(change)
}
}
}

task.consecutiveMistakeCount = 0

// B1: one checkpoint for the whole patch (not per file), and only when
// every file operation succeeded. Live setting with default-on
// semantics: skip only when explicitly false.
// Reset the consecutive-mistake counter only after a fully successful
// patch: a failed operation (missing file, rejected move, ...) increments
// the counter, and the count must survive a partially written patch so
// the auto-approval safety net still engages across consecutive failed
// patches.
if (patchSucceeded) {
task.consecutiveMistakeCount = 0
}

// B1: one checkpoint for the whole patch (not per file). Live
// setting with default-on semantics: skip only when explicitly false.
// B3a partial flush: the checkpoint and journal are also taken when at
// least one file operation wrote, even if a later hunk of the same
// patch failed - the journal then documents exactly the subset that
// was written, and the failed operation was already reported through
// pushToolResult. A fully failed patch (nothing written) leaves no
// checkpoint behind.
if (patchSucceeded || successfulChanges.length > 0) {
const perWriteCheckpoints = (await task.providerRef?.deref()?.getState())?.perWriteCheckpoints
if (perWriteCheckpoints !== false) {
void checkpointSave(task, false, true).catch(() => {})
// B2: one journal entry per file that was actually written by
// the patch (the simplest correct design for multi-file patches),
// all referencing the single checkpoint above. A no-op update
// contributes no entry because nothing was written. `movePath`,
// when present, is the file's final location. diffStats is
// omitted: the per-file approval diffs are computed inside the
// handlers and are not retained after the patch completes.
void checkpointSave(
task,
false,
true,
successfulChanges.map((change) => ({
path: change.movePath ?? change.path,
operation: change.type === "add" ? "create" : change.type,
})),
).catch(() => {})
}
}
} catch (error) {
Expand All @@ -170,7 +237,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
task: Task,
callbacks: ToolCallbacks,
isWriteProtected: boolean,
): Promise<boolean> {
): Promise<ApplyPatchFileOpResult> {
const { askApproval, pushToolResult } = callbacks

// Check if file already exists
Expand All @@ -181,7 +248,8 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
const errorMessage = `File already exists: ${relPath}. Use Update File instead.`
await task.say("error", errorMessage)
pushToolResult(formatResponse.toolError(errorMessage))
return false
// Stryker disable next-line ObjectLiteral : failure sentinel, both fields are consumed only in falsy contexts (succeeded in a logical-and, wrote in an if) so the emptied object is behaviourally identical
return { succeeded: false, wrote: false }
}

const newContent = change.newContent || ""
Expand Down Expand Up @@ -235,7 +303,8 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
}
pushToolResult("Changes were rejected by the user.")
await task.diffViewProvider.reset()
return false
// Stryker disable next-line ObjectLiteral : failure sentinel, both fields are consumed only in falsy contexts (succeeded in a logical-and, wrote in an if) so the emptied object is behaviourally identical
return { succeeded: false, wrote: false }
}

// Save the changes
Expand All @@ -253,7 +322,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
pushToolResult(message)
await task.diffViewProvider.reset()
task.processQueuedMessages()
return true
return { succeeded: true, wrote: true }
}

private async handleDeleteFile(
Expand All @@ -262,7 +331,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
task: Task,
callbacks: ToolCallbacks,
isWriteProtected: boolean,
): Promise<boolean> {
): Promise<ApplyPatchFileOpResult> {
const { askApproval, pushToolResult } = callbacks

// Check if file exists
Expand All @@ -273,7 +342,8 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
const errorMessage = `File not found: ${relPath}. Cannot delete a non-existent file.`
await task.say("error", errorMessage)
pushToolResult(formatResponse.toolError(errorMessage))
return false
// Stryker disable next-line ObjectLiteral : failure sentinel, both fields are consumed only in falsy contexts (succeeded in a logical-and, wrote in an if) so the emptied object is behaviourally identical
return { succeeded: false, wrote: false }
}

const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)
Expand All @@ -295,7 +365,8 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {

if (!didApprove) {
pushToolResult("Delete operation was rejected by the user.")
return false
// Stryker disable next-line ObjectLiteral : failure sentinel, both fields are consumed only in falsy contexts (succeeded in a logical-and, wrote in an if) so the emptied object is behaviourally identical
return { succeeded: false, wrote: false }
}

// Delete the file
Expand All @@ -305,13 +376,14 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
const errorMessage = `Failed to delete file '${relPath}': ${error instanceof Error ? error.message : String(error)}`
await task.say("error", errorMessage)
pushToolResult(formatResponse.toolError(errorMessage))
return false
// Stryker disable next-line ObjectLiteral : failure sentinel, both fields are consumed only in falsy contexts (succeeded in a logical-and, wrote in an if) so the emptied object is behaviourally identical
return { succeeded: false, wrote: false }
}

task.didEditFile = true
pushToolResult(`Successfully deleted ${relPath}`)
task.processQueuedMessages()
return true
return { succeeded: true, wrote: true }
}

private async handleUpdateFile(
Expand All @@ -321,9 +393,13 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
task: Task,
callbacks: ToolCallbacks,
isWriteProtected: boolean,
): Promise<boolean> {
): Promise<ApplyPatchFileOpResult> {
const { askApproval, pushToolResult } = callbacks

// A move reports failure when the original file cannot be deleted
// after the copy (both paths would remain on disk).
let moveSucceeded = true

// Check if file exists
const fileExists = await fileExistsAtPath(absolutePath)
if (!fileExists) {
Expand All @@ -332,7 +408,8 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
const errorMessage = `File not found: ${relPath}. Cannot update a non-existent file.`
await task.say("error", errorMessage)
pushToolResult(formatResponse.toolError(errorMessage))
return false
// Stryker disable next-line ObjectLiteral : failure sentinel, both fields are consumed only in falsy contexts (succeeded in a logical-and, wrote in an if) so the emptied object is behaviourally identical
return { succeeded: false, wrote: false }
}

const originalContent = change.originalContent || ""
Expand All @@ -347,10 +424,12 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
const diff = formatResponse.createPrettyPatch(relPath, originalContent, newContent)
if (!diff) {
// A no-op change is not a failure: the patch processed cleanly and
// nothing was written, so the whole-patch success state is kept.
// nothing was written, so the whole-patch success state is kept —
// but `wrote` stays false so the change journal does not document a
// write that never happened.
pushToolResult(`No changes needed for '${relPath}'`)
await task.diffViewProvider.reset()
return true
return { succeeded: true, wrote: false }
}

// Check experiment settings
Expand Down Expand Up @@ -396,7 +475,8 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
}
pushToolResult("Changes were rejected by the user.")
await task.diffViewProvider.reset()
return false
// Stryker disable next-line ObjectLiteral : failure sentinel, both fields are consumed only in falsy contexts (succeeded in a logical-and, wrote in an if) so the emptied object is behaviourally identical
return { succeeded: false, wrote: false }
}

// Handle file move if specified
Expand All @@ -409,7 +489,8 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
await task.say("rooignore_error", change.movePath)
pushToolResult(formatResponse.rooIgnoreError(change.movePath))
await task.diffViewProvider.reset()
return false
// Stryker disable next-line ObjectLiteral : failure sentinel, both fields are consumed only in falsy contexts (succeeded in a logical-and, wrote in an if) so the emptied object is behaviourally identical
return { succeeded: false, wrote: false }
}

// Check if destination path is write-protected
Expand All @@ -421,7 +502,8 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
await task.say("error", errorMessage)
pushToolResult(formatResponse.toolError(errorMessage))
await task.diffViewProvider.reset()
return false
// Stryker disable next-line ObjectLiteral : failure sentinel, both fields are consumed only in falsy contexts (succeeded in a logical-and, wrote in an if) so the emptied object is behaviourally identical
return { succeeded: false, wrote: false }
}

// Check if destination path is outside workspace
Expand All @@ -433,7 +515,8 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
await task.say("error", errorMessage)
pushToolResult(formatResponse.toolError(errorMessage))
await task.diffViewProvider.reset()
return false
// Stryker disable next-line ObjectLiteral : failure sentinel, both fields are consumed only in falsy contexts (succeeded in a logical-and, wrote in an if) so the emptied object is behaviourally identical
return { succeeded: false, wrote: false }
}

// Save new content to the new path
Expand All @@ -452,11 +535,19 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
await fs.writeFile(moveAbsolutePath, newContent, "utf8")
}

// Delete the original file
// Delete the original file. A failed deletion leaves both paths on
// disk, so the move must be reported as a failure rather than
// checkpointed and journaled as a completed move.
try {
await fs.unlink(absolutePath)
} catch (error) {
moveSucceeded = false
console.error(`Failed to delete original file after move: ${error}`)
task.consecutiveMistakeCount++
task.recordToolError("apply_patch")
const errorMessage = `Move of '${relPath}' to '${change.movePath}' failed: could not delete the original file.`
await task.say("error", errorMessage)
pushToolResult(formatResponse.toolError(errorMessage))
}

await task.fileContextTracker.trackFileContext(change.movePath, "roo_edited" as RecordSource)
Expand All @@ -477,7 +568,13 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
pushToolResult(message)
await task.diffViewProvider.reset()
task.processQueuedMessages()
return true
if (!moveSucceeded) {
// The destination file was written on disk before the source
// deletion failed, so the write must still be checkpointed and
// journaled; the move itself is reported as failed.
return { succeeded: false, wrote: true }
}
return { succeeded: true, wrote: true }
}

override async handlePartial(task: Task, block: ToolUse<"apply_patch">): Promise<void> {
Expand Down
9 changes: 8 additions & 1 deletion src/core/tools/EditFileTool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,14 @@ export class EditFileTool extends BaseTool<"edit_file"> {
pushToolResult(message + replacementInfo)

if (perWriteCheckpoints) {
void checkpointSave(task, false, true).catch(() => {})
// B2: the change-journal entry for this edit is appended inside
// checkpointSave (the hook stays a single call site), keyed by the
// checkpoint commit that call produces.
void checkpointSave(task, false, true, {
path: relPath,
operation: isNewFile ? "create" : "update",
diffStats: diffStats ? { additions: diffStats.added, deletions: diffStats.removed } : undefined,
}).catch(() => {})
}

await task.diffViewProvider.reset()
Expand Down
Loading
Loading