diff --git a/src/core/tools/ApplyDiffTool.ts b/src/core/tools/ApplyDiffTool.ts index 3b664b3bd2..8577eb8d11 100644 --- a/src/core/tools/ApplyDiffTool.ts +++ b/src/core/tools/ApplyDiffTool.ts @@ -13,6 +13,8 @@ import { unescapeHtmlEntities } from "../../utils/text-normalization" import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" import { computeDiffStats, sanitizeUnifiedDiff } from "../diff/stats" import type { ToolUse } from "../../shared/tools" +import { checkAutoApproval } from "../auto-approval" +import { checkpointSave } from "../checkpoints" import { BaseTool, ToolCallbacks } from "./BaseTool" @@ -144,16 +146,19 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { diff: diffContent, } + // Both save branches present the same approval message, and the B3a + // per-write checkpoint (below) reuses it for auto-approval parity. + const completeMessage = JSON.stringify({ + ...sharedMessageProps, + diff: diffContent, + content: unifiedPatch, + originalContent, + diffStats, + isProtected: isWriteProtected, + } satisfies ClineSayTool) + if (isPreventFocusDisruptionEnabled) { // Direct file write without diff view - const completeMessage = JSON.stringify({ - ...sharedMessageProps, - diff: diffContent, - content: unifiedPatch, - originalContent, - diffStats, - isProtected: isWriteProtected, - } satisfies ClineSayTool) let toolProgressStatus @@ -191,15 +196,6 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { await task.diffViewProvider.update(diffResult.content, true) task.diffViewProvider.scrollToFirstDiff() - const completeMessage = JSON.stringify({ - ...sharedMessageProps, - diff: diffContent, - content: unifiedPatch, - originalContent, - diffStats, - isProtected: isWriteProtected, - } satisfies ClineSayTool) - let toolProgressStatus if (task.diffStrategy && task.diffStrategy.getProgressStatus) { @@ -224,6 +220,36 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) } + // B3a: per-write checkpoint + change card for the apply_diff write, with + // the same parity as write_to_file / edit_file / apply_patch (this tool + // previously wrote with no checkpoint and no card, so its edits had no + // per-file rollback surface in chat). The applied unified diff and its + // stats were already computed above for the tool message and are reused + // verbatim; auto-approved steps always get the compact card. Live setting + // with default-on semantics: skip only when explicitly false. apply_diff + // only ever modifies an existing file (a missing file errors out before + // the diff is applied), so the operation is always "update". + const perWriteCheckpoints = state?.perWriteCheckpoints + if (perWriteCheckpoints !== false) { + const autoApproved = + ( + await checkAutoApproval({ + state, + cwd: task.cwd, + ask: "tool", + text: completeMessage, + isProtected: isWriteProtected, + }) + ).decision === "approve" + await checkpointSave(task, false, true, { + path: relPath, + operation: "update", + ...(diffStats ? { diffStats: { additions: diffStats.added, deletions: diffStats.removed } } : {}), + ...(unifiedPatch ? { diff: unifiedPatch } : {}), + ...(autoApproved ? { autoApproved: true } : {}), + }).catch(() => {}) + } + // Track file edit operation if (relPath) { await task.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource) diff --git a/src/core/tools/__tests__/ApplyDiffTool.changeCard.spec.ts b/src/core/tools/__tests__/ApplyDiffTool.changeCard.spec.ts new file mode 100644 index 0000000000..dcf6c52f51 --- /dev/null +++ b/src/core/tools/__tests__/ApplyDiffTool.changeCard.spec.ts @@ -0,0 +1,242 @@ +// npx vitest run core/tools/__tests__/ApplyDiffTool.changeCard.spec.ts + +import type { MockedFunction } from "vitest" + +import { fileExistsAtPath } from "../../../utils/fs" +import { checkAutoApproval } from "../../auto-approval" +import { checkpointSave } from "../../checkpoints" +import type { Task } from "../../task/Task" +import { ApplyDiffTool } from "../ApplyDiffTool" + +vi.mock("fs/promises", () => ({ + default: { + readFile: vi.fn().mockResolvedValue("original file content\n"), + // The trial aggregate adds the S4b stat-pair self-observation around this + // read (ApplyDiffTool records its own read under the ReadFileTool contract); + // its `.catch(() => undefined)` fallback keeps the observation no-op when + // stat rejects, so this spec stays green on both the component branch and + // the trial aggregate. + stat: vi.fn().mockRejectedValue(new Error("stat unavailable in this spec")), + }, +})) + +vi.mock("../../../utils/fs", () => ({ + fileExistsAtPath: vi.fn().mockResolvedValue(true), +})) + +vi.mock("../../prompts/responses", () => ({ + formatResponse: { + toolError: vi.fn((msg: string) => `Error: ${msg}`), + rooIgnoreError: vi.fn((filePath: string) => `Access denied: ${filePath}`), + createPrettyPatch: vi.fn(() => "mock-diff"), + }, +})) + +vi.mock("../../diff/stats", () => ({ + // The real DiffStats shape is { added, removed } (the tool maps it to the + // change-card { additions, deletions } pair). + sanitizeUnifiedDiff: vi.fn((diff: string) => diff), + computeDiffStats: vi.fn(() => ({ added: 1, removed: 1 })), +})) + +vi.mock("../../checkpoints", () => ({ + checkpointSave: vi.fn().mockResolvedValue(undefined), +})) + +vi.mock("../../auto-approval", () => ({ + checkAutoApproval: vi.fn().mockResolvedValue({ decision: "ask" }), +})) + +describe("ApplyDiffTool.execute - per-write checkpoint and change card (B3a, epic #1375)", () => { + const mockedFileExistsAtPath = fileExistsAtPath as MockedFunction + const mockCheckpointSave = checkpointSave as MockedFunction + const mockCheckAutoApproval = checkAutoApproval as MockedFunction + + let tool: ApplyDiffTool + let mockTask: Pick< + Task, + | "cwd" + | "consecutiveMistakeCount" + | "consecutiveMistakeCountForApplyDiff" + | "recordToolError" + | "rooIgnoreController" + | "rooProtectedController" + | "processQueuedMessages" + | "didEditFile" + | "api" + | "diffStrategy" + | "diffViewProvider" + | "providerRef" + | "fileContextTracker" + > + let mockSaveDirectly: MockedFunction<(...args: unknown[]) => Promise> + let mockGetState: MockedFunction<() => Promise>> + let mockAskApproval: MockedFunction<(...args: unknown[]) => Promise> + let mockHandleError: MockedFunction<(...args: unknown[]) => Promise> + let mockPushToolResult: MockedFunction<(...args: unknown[]) => void> + + beforeEach(() => { + vi.clearAllMocks() + + mockedFileExistsAtPath.mockResolvedValue(true) + + // Structural stubs for the prevent-focus-disruption save path: the real + // DiffViewProvider is out of scope here, so vi.fn() doubles stand in for + // the members the tool touches. + mockSaveDirectly = vi.fn().mockResolvedValue({ + newProblemsMessage: "", + userEdits: undefined, + finalContent: "new content", + }) + const diffViewProviderStub = { + editType: undefined as "create" | "modify" | undefined, + originalContent: undefined as string | undefined, + saveDirectly: mockSaveDirectly, + open: vi.fn().mockResolvedValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + scrollToFirstDiff: vi.fn(), + saveChanges: vi.fn().mockResolvedValue(undefined), + pushToolWriteResult: vi.fn().mockResolvedValue("Saved file"), + reset: vi.fn().mockResolvedValue(undefined), + } + + mockGetState = vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + // Exercise the focus-disruption (saveDirectly) save path. + experiments: { preventFocusDisruption: true }, + }) + + mockTask = { + cwd: "/workspace/project", + consecutiveMistakeCount: 0, + consecutiveMistakeCountForApplyDiff: new Map(), + recordToolError: vi.fn(), + rooIgnoreController: { + validateAccess: vi.fn().mockReturnValue(true), + } as unknown as Task["rooIgnoreController"], + rooProtectedController: { + isWriteProtected: vi.fn().mockReturnValue(false), + } as unknown as Task["rooProtectedController"], + processQueuedMessages: vi.fn(), + didEditFile: false, + api: { + getModel: () => ({ id: "claude-sonnet-4-5" }), + } as unknown as Task["api"], + diffStrategy: { + applyDiff: vi.fn().mockResolvedValue({ success: true, content: "modified file content\n" }), + } as unknown as Task["diffStrategy"], + diffViewProvider: diffViewProviderStub as unknown as Task["diffViewProvider"], + providerRef: { + deref: vi.fn().mockReturnValue({ + getState: mockGetState, + }), + } as unknown as Task["providerRef"], + fileContextTracker: { + trackFileContext: vi.fn().mockResolvedValue(undefined), + } as unknown as Task["fileContextTracker"], + } + + mockAskApproval = vi.fn().mockResolvedValue(true) + mockHandleError = vi.fn().mockResolvedValue(undefined) + mockPushToolResult = vi.fn() + + tool = new ApplyDiffTool() + }) + + it("records a per-write checkpoint with the applied diff after a successful write", async () => { + await tool.execute({ path: "src/thing.ts", diff: "unified diff" }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockCheckpointSave).toHaveBeenCalledTimes(1) + expect(mockCheckpointSave).toHaveBeenCalledWith(mockTask, false, true, { + path: "src/thing.ts", + operation: "update", + diffStats: { additions: 1, deletions: 1 }, + diff: "mock-diff", + }) + // The card builder is fed the tool's own auto-approval decision, so the + // call must carry the live state, the tool's ask channel, the approval + // message text, and the protection flag (pins the call-argument shape). + expect(mockCheckAutoApproval).toHaveBeenCalledTimes(1) + expect(mockCheckAutoApproval).toHaveBeenCalledWith( + expect.objectContaining({ + state: expect.objectContaining({ diagnosticsEnabled: true }), + cwd: "/workspace/project", + ask: "tool", + text: expect.stringContaining("appliedDiff"), + isProtected: false, + }), + ) + }) + + it("marks auto-approved steps so the card renders compact", async () => { + mockCheckAutoApproval.mockResolvedValueOnce({ decision: "approve" }) + + await tool.execute({ path: "src/thing.ts", diff: "unified diff" }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockCheckpointSave).toHaveBeenCalledTimes(1) + expect(mockCheckpointSave.mock.calls[0]?.[3]).toEqual(expect.objectContaining({ autoApproved: true })) + }) + + it("skips the checkpoint when perWriteCheckpoints is explicitly disabled", async () => { + mockGetState.mockResolvedValueOnce({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: { preventFocusDisruption: true }, + perWriteCheckpoints: false, + }) + + await tool.execute({ path: "src/thing.ts", diff: "unified diff" }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockCheckpointSave).not.toHaveBeenCalled() + // The write itself still happens (the setting gates the checkpoint only), + // and auto-approval is never consulted when there is no card to build. + expect(mockSaveDirectly).toHaveBeenCalled() + expect(mockCheckAutoApproval).not.toHaveBeenCalled() + }) + + it("keeps the checkpoint path alive when the provider state is unavailable", async () => { + // providerRef.deref() -> undefined: state is undefined, so + // state?.perWriteCheckpoints must short-circuit (not throw) and the + // default-on semantics still record the checkpoint. + mockTask.providerRef = { + deref: vi.fn().mockReturnValue(undefined), + } as unknown as Task["providerRef"] + + await tool.execute({ path: "src/thing.ts", diff: "unified diff" }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockCheckpointSave).toHaveBeenCalledTimes(1) + expect(mockCheckAutoApproval).toHaveBeenCalledWith( + expect.objectContaining({ state: undefined, ask: "tool", cwd: "/workspace/project" }), + ) + }) + + it("records nothing when the approval is declined", async () => { + mockAskApproval.mockResolvedValue(false) + + await tool.execute({ path: "src/thing.ts", diff: "unified diff" }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockCheckpointSave).not.toHaveBeenCalled() + expect(mockSaveDirectly).not.toHaveBeenCalled() + }) +}) diff --git a/src/core/tools/apply-patch/apply.ts b/src/core/tools/apply-patch/apply.ts index 4ab377f732..64009d27bf 100644 --- a/src/core/tools/apply-patch/apply.ts +++ b/src/core/tools/apply-patch/apply.ts @@ -29,6 +29,15 @@ export interface ApplyPatchFileChange { originalContent?: string /** New content (for add/update) */ newContent?: string + /** + * B3a: the unified approval diff for this file (computed by the tool + * handler), reused by the per-step change card. + */ + diff?: string + /** B3a: { added, removed } stats of the approval diff, when computable. */ + diffStats?: { added: number; removed: number } + /** B3a: whether this file's approval was auto-approved. */ + autoApproved?: boolean } /**