From 63d9b098437220256efc15dc654238a2811a9df9 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Sun, 6 Sep 2026 07:46:13 +0800 Subject: [PATCH] feat(tools): per-write checkpoint + change card for edit & search_replace tools (B3a v2-8b, epic #1375) --- src/core/tools/ApplyPatchTool.ts | 69 ++++- src/core/tools/EditFileTool.ts | 24 +- src/core/tools/EditTool.ts | 31 ++ src/core/tools/SearchReplaceTool.ts | 32 ++ src/core/tools/WriteToFileTool.ts | 31 +- .../__tests__/applyPatchTool.execute.spec.ts | 225 ++++++++++++-- src/core/tools/__tests__/editFileTool.spec.ts | 52 +++- .../editSearchReplaceTool.changeCard.spec.ts | 274 ++++++++++++++++++ src/core/tools/__tests__/editTool.spec.ts | 8 + .../tools/__tests__/searchReplaceTool.spec.ts | 8 + .../tools/__tests__/writeToFileTool.spec.ts | 35 ++- 11 files changed, 755 insertions(+), 34 deletions(-) create mode 100644 src/core/tools/__tests__/editSearchReplaceTool.changeCard.spec.ts diff --git a/src/core/tools/ApplyPatchTool.ts b/src/core/tools/ApplyPatchTool.ts index 92bafdc38a..de7676778f 100644 --- a/src/core/tools/ApplyPatchTool.ts +++ b/src/core/tools/ApplyPatchTool.ts @@ -7,6 +7,7 @@ import { getReadablePath } from "../../utils/path" import { isPathOutsideWorkspace } from "../../utils/pathUtils" import { Task } from "../task/Task" import { checkpointSave } from "../checkpoints" +import { checkAutoApproval } from "../auto-approval" import { formatResponse } from "../prompts/responses" import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { fileExistsAtPath } from "../../utils/fs" @@ -159,6 +160,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { } else if (change.type === "delete") { // Delete file const deleteResult = await this.handleDeleteFile( + change, absolutePath, relPath, task, @@ -210,18 +212,31 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { // 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( + // when present, is the file's final location. B3a: the per-file + // approval diff/stats and auto-approval state, retained by the + // handlers, feed the per-step change card. + // Awaited: a later write must not interleave with this patch's + // staging/commit/journal/change-card work. checkpointSave never + // rejects (service call wrapped in try/catch upstream). + await checkpointSave( task, false, true, successfulChanges.map((change) => ({ path: change.movePath ?? change.path, operation: change.type === "add" ? "create" : change.type, + ...(change.diffStats + ? { + diffStats: { + additions: change.diffStats.added, + deletions: change.diffStats.removed, + }, + } + : {}), + ...(change.diff ? { diff: change.diff } : {}), + ...(change.autoApproved ? { autoApproved: true } : {}), })), - ).catch(() => {}) + ) } } } catch (error) { @@ -288,6 +303,21 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { diffStats, } satisfies ClineSayTool) + // B3a: retain the approval diff/stats and auto-approval state so the + // post-loop checkpoint hook can build the per-step change card. + change.diff = sanitizedDiff + change.diffStats = diffStats + change.autoApproved = + ( + await checkAutoApproval({ + state, + cwd: task.cwd, + ask: "tool", + text: completeMessage, + isProtected: isWriteProtected, + }) + ).decision === "approve" + // Show diff view if focus disruption prevention is disabled if (!isPreventFocusDisruptionEnabled) { await task.diffViewProvider.open(relPath) @@ -326,6 +356,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { } private async handleDeleteFile( + change: ApplyPatchFileChange, absolutePath: string, relPath: string, task: Task, @@ -361,6 +392,19 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { isProtected: isWriteProtected, } satisfies ClineSayTool) + // B3a: auto-approval state feeds the per-step change card (deletes have + // no diff to thread). + change.autoApproved = + ( + await checkAutoApproval({ + state: await task.providerRef.deref()?.getState(), + cwd: task.cwd, + ask: "tool", + text: completeMessage, + isProtected: isWriteProtected, + }) + ).decision === "approve" + const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) if (!didApprove) { @@ -460,6 +504,21 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { diffStats, } satisfies ClineSayTool) + // B3a: retain the approval diff/stats and auto-approval state so the + // post-loop checkpoint hook can build the per-step change card. + change.diff = sanitizedDiff + change.diffStats = diffStats + change.autoApproved = + ( + await checkAutoApproval({ + state, + cwd: task.cwd, + ask: "tool", + text: completeMessage, + isProtected: isWriteProtected, + }) + ).decision === "approve" + // Show diff view if focus disruption prevention is disabled if (!isPreventFocusDisruptionEnabled) { await task.diffViewProvider.open(relPath) diff --git a/src/core/tools/EditFileTool.ts b/src/core/tools/EditFileTool.ts index 87a010e90c..03c20ccee2 100644 --- a/src/core/tools/EditFileTool.ts +++ b/src/core/tools/EditFileTool.ts @@ -12,6 +12,7 @@ import { fileExistsAtPath } from "../../utils/fs" import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" import { sanitizeUnifiedDiff, computeDiffStats } from "../diff/stats" import { checkpointSave } from "../../core/checkpoints" +import { checkAutoApproval } from "../auto-approval" import type { ToolUse } from "../../shared/tools" import { BaseTool, ToolCallbacks } from "./BaseTool" @@ -468,12 +469,29 @@ export class EditFileTool extends BaseTool<"edit_file"> { if (perWriteCheckpoints) { // 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, { + // checkpoint commit that call produces. B3a threads the approval + // diff (for the change card) and whether the step was auto- + // approved (auto-approved steps always get the compact card). + const autoApproved = + ( + await checkAutoApproval({ + state, + cwd: task.cwd, + ask: "tool", + text: completeMessage, + isProtected: isWriteProtected, + }) + ).decision === "approve" + // Awaited: a later tool block must not interleave with this edit's + // staging/commit/journal/change-card work. checkpointSave never + // rejects (service call wrapped in try/catch upstream). + await checkpointSave(task, false, true, { path: relPath, operation: isNewFile ? "create" : "update", diffStats: diffStats ? { additions: diffStats.added, deletions: diffStats.removed } : undefined, - }).catch(() => {}) + ...(sanitizedDiff ? { diff: sanitizedDiff } : {}), + ...(autoApproved ? { autoApproved: true } : {}), + }) } await task.diffViewProvider.reset() diff --git a/src/core/tools/EditTool.ts b/src/core/tools/EditTool.ts index 2ae8bf4ed0..2c5e9a0999 100644 --- a/src/core/tools/EditTool.ts +++ b/src/core/tools/EditTool.ts @@ -11,6 +11,8 @@ import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { fileExistsAtPath } from "../../utils/fs" import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" import { sanitizeUnifiedDiff, computeDiffStats } from "../diff/stats" +import { checkpointSave } from "../../core/checkpoints" +import { checkAutoApproval } from "../auto-approval" import type { ToolUse } from "../../shared/tools" import { BaseTool, ToolCallbacks } from "./BaseTool" @@ -167,6 +169,7 @@ export class EditTool extends BaseTool<"edit"> { const state = await provider?.getState() const diagnosticsEnabled = state?.diagnosticsEnabled ?? true const writeDelayMs = state?.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS + const perWriteCheckpoints = state?.perWriteCheckpoints ?? true const isPreventFocusDisruptionEnabled = experiments.isEnabled( state?.experiments ?? {}, EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, @@ -229,6 +232,34 @@ export class EditTool extends BaseTool<"edit"> { const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, false) pushToolResult(message) + if (perWriteCheckpoints) { + // 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. B3a threads the approval + // diff (for the change card) and whether the step was auto- + // approved (auto-approved steps always get the compact card). + const autoApproved = + ( + await checkAutoApproval({ + state, + cwd: task.cwd, + ask: "tool", + text: completeMessage, + isProtected: isWriteProtected, + }) + ).decision === "approve" + // Awaited: a later tool block must not interleave with this edit's + // staging/commit/journal/change-card work. checkpointSave never + // rejects (service call wrapped in try/catch upstream). + await checkpointSave(task, false, true, { + path: relPath, + operation: "update", + diffStats: diffStats ? { additions: diffStats.added, deletions: diffStats.removed } : undefined, + ...(sanitizedDiff ? { diff: sanitizedDiff } : {}), + ...(autoApproved ? { autoApproved: true } : {}), + }) + } + await task.diffViewProvider.reset() this.resetPartialState() diff --git a/src/core/tools/SearchReplaceTool.ts b/src/core/tools/SearchReplaceTool.ts index e29b124010..253d87e634 100644 --- a/src/core/tools/SearchReplaceTool.ts +++ b/src/core/tools/SearchReplaceTool.ts @@ -11,6 +11,8 @@ import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { fileExistsAtPath } from "../../utils/fs" import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" import { sanitizeUnifiedDiff, computeDiffStats } from "../diff/stats" +import { checkpointSave } from "../../core/checkpoints" +import { checkAutoApproval } from "../auto-approval" import type { ToolUse } from "../../shared/tools" import { BaseTool, ToolCallbacks } from "./BaseTool" @@ -163,6 +165,7 @@ export class SearchReplaceTool extends BaseTool<"search_replace"> { const state = await provider?.getState() const diagnosticsEnabled = state?.diagnosticsEnabled ?? true const writeDelayMs = state?.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS + const perWriteCheckpoints = state?.perWriteCheckpoints ?? true const isPreventFocusDisruptionEnabled = experiments.isEnabled( state?.experiments ?? {}, EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, @@ -225,6 +228,35 @@ export class SearchReplaceTool extends BaseTool<"search_replace"> { const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, false) pushToolResult(message) + if (perWriteCheckpoints) { + // B2: the change-journal entry for this search-and-replace is appended + // inside checkpointSave (the hook stays a single call site), keyed + // by the checkpoint commit that call produces. B3a threads the + // approval diff (for the change card) and whether the step was auto- + // approved (auto-approved steps always get the compact card). + const autoApproved = + ( + await checkAutoApproval({ + state, + cwd: task.cwd, + ask: "tool", + text: completeMessage, + isProtected: isWriteProtected, + }) + ).decision === "approve" + // Awaited: a later tool block must not interleave with this + // search-and-replace's staging/commit/journal/change-card work. + // checkpointSave never rejects (service call wrapped in try/catch + // upstream). + await checkpointSave(task, false, true, { + path: relPath, + operation: "update", + diffStats: diffStats ? { additions: diffStats.added, deletions: diffStats.removed } : undefined, + ...(sanitizedDiff ? { diff: sanitizedDiff } : {}), + ...(autoApproved ? { autoApproved: true } : {}), + }) + } + await task.diffViewProvider.reset() this.resetPartialState() diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 5154af860d..47c2a21972 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -15,6 +15,7 @@ import { unescapeHtmlEntities } from "../../utils/text-normalization" import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" import { convertNewFileToUnifiedDiff, computeDiffStats, sanitizeUnifiedDiff, type DiffStats } from "../diff/stats" import { checkpointSave } from "../checkpoints" +import { checkAutoApproval } from "../auto-approval" import type { ToolUse } from "../../shared/tools" import { BaseTool, ToolCallbacks } from "./BaseTool" @@ -111,8 +112,14 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { ) // B2: the approval-diff stats for the write, shared by both the - // approval message and the change-journal entry below. + // approval message and the change-journal entry below. B3a also + // reuses the sanitized unified diff itself for the per-step change + // card (never recomputed). let approvalDiffStats: DiffStats | null = null + // Stryker disable next-line StringLiteral : pre-branch placeholder, both branches assign before first use (unobservable initializer) + let approvalDiff = "" + // Stryker disable next-line StringLiteral : pre-branch placeholder, both branches assign before first use (unobservable initializer) + let completeMessage = "" if (isPreventFocusDisruptionEnabled) { task.diffViewProvider.editType = fileExists ? "modify" : "create" @@ -128,7 +135,8 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { : convertNewFileToUnifiedDiff(newContent, relPath) unified = sanitizeUnifiedDiff(unified) approvalDiffStats = computeDiffStats(unified) - const completeMessage = JSON.stringify({ + approvalDiff = unified + completeMessage = JSON.stringify({ ...sharedMessageProps, content: unified, diffStats: approvalDiffStats || undefined, @@ -161,7 +169,8 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { : convertNewFileToUnifiedDiff(newContent, relPath) unified = sanitizeUnifiedDiff(unified) approvalDiffStats = computeDiffStats(unified) - const completeMessage = JSON.stringify({ + approvalDiff = unified + completeMessage = JSON.stringify({ ...sharedMessageProps, content: unified, diffStats: approvalDiffStats || undefined, @@ -192,13 +201,27 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { // checkpointSave (the hook stays a single call site), keyed by the // checkpoint commit that call produces. Await so the checkpoint // (staging + commit) finishes before the next queued write starts; - // otherwise two writes can collapse into one commit. + // otherwise two writes can collapse into one commit. B3a threads + // the approval diff (for the change card) and whether the step was + // auto-approved (auto-approved steps always get the compact card). + const autoApproved = + ( + await checkAutoApproval({ + state, + cwd: task.cwd, + ask: "tool", + text: completeMessage, + isProtected: isWriteProtected, + }) + ).decision === "approve" await checkpointSave(task, false, true, { path: relPath, operation: fileExists ? "update" : "create", diffStats: approvalDiffStats ? { additions: approvalDiffStats.added, deletions: approvalDiffStats.removed } : undefined, + ...(approvalDiff ? { diff: approvalDiff } : {}), + ...(autoApproved ? { autoApproved: true } : {}), }).catch(() => {}) } diff --git a/src/core/tools/__tests__/applyPatchTool.execute.spec.ts b/src/core/tools/__tests__/applyPatchTool.execute.spec.ts index f9fc895fdd..a643f78748 100644 --- a/src/core/tools/__tests__/applyPatchTool.execute.spec.ts +++ b/src/core/tools/__tests__/applyPatchTool.execute.spec.ts @@ -311,9 +311,15 @@ describe("ApplyPatchTool.execute - delete file success path", () => { expect(mockPushToolResult).toHaveBeenCalledWith("File saved successfully") expect(mockedCheckpointSave).toHaveBeenCalledOnce() - // The journal entry documents exactly the file the add wrote. + // The journal entry documents exactly the file the add wrote, with + // the approval diff/stats threaded for the change card (B3a). expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ - { path: "src/new.ts", operation: "create" }, + { + path: "src/new.ts", + operation: "create", + diffStats: { additions: 2, deletions: 0 }, + diff: expect.stringContaining("+hello"), + }, ]) // A fully successful add resets the consecutive-mistake counter. expect(mockTask.consecutiveMistakeCount).toBe(0) @@ -412,27 +418,34 @@ describe("ApplyPatchTool.execute - delete file success path", () => { ]) }) - it("does not record a checkpoint when the first hunk is access-denied", async () => { - // The denial happens before any hunk wrote, so the failed patch must - // leave no checkpoint behind: the partial flush has nothing to flush - // and the failure flag must not open the checkpoint gate. - const validateAccess = ( - mockTask["rooIgnoreController"] as unknown as { validateAccess: MockedFunction<() => boolean> } - ).validateAccess - validateAccess.mockReturnValueOnce(false) + it("does not record a checkpoint when the move destination is access denied (rooIgnore)", async () => { + // The rooIgnore guard runs before the write-protected/workspace + // checks, so an access-denied destination stops the move before any + // file is written and nothing is checkpointed. + // Structural cast for the test double (matches the mock style used for the controllers above). + const ref = (mockTask["providerRef"] as unknown as { deref: MockedFunction<() => unknown> }).deref + ref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + rooIgnoreController: { isPathIgnored: vi.fn().mockReturnValue(true) }, + }), + }) - const deniedFirstPatch = `*** Begin Patch -*** Add File: src/denied.ts -+hello -*** End Patch` + const movePatch = [ + "*** Begin Patch", + "*** Update File: src/old.ts", + "*** Move to: src/ignored.ts", + "@@", + "-original", + "+modified", + "*** End Patch", + ].join("\n") - await tool.execute({ patch: deniedFirstPatch }, mockTask as Task, { + await tool.execute({ patch: movePatch }, mockTask as Task, { askApproval: mockAskApproval, handleError: mockHandleError, pushToolResult: mockPushToolResult, }) - expect(mockTask.say).toHaveBeenCalledWith("rooignore_error", "src/denied.ts") expect(mockedCheckpointSave).not.toHaveBeenCalled() }) @@ -523,6 +536,31 @@ describe("ApplyPatchTool.execute - delete file success path", () => { expect(mockedCheckpointSave).toHaveBeenCalledOnce() }) + it("awaits the patch checkpoint before execute settles", async () => { + type SaveResult = Awaited> + let resolveSave: (value: SaveResult | PromiseLike) => void = () => {} + const saveDeferred = new Promise((resolve) => (resolveSave = resolve)) + mockedCheckpointSave.mockImplementationOnce(() => saveDeferred) + + const executePromise = tool.execute({ patch: movePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // execute must not settle while the checkpoint is still in flight: + // a later write would otherwise interleave with this patch's staged work. + let settled = false + void executePromise.finally(() => (settled = true)) + await new Promise((resolve) => setTimeout(resolve, 0)) + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(settled).toBe(false) + + resolveSave() + await executePromise + expect(settled).toBe(true) + }) + it("records a checkpoint when the in-place update succeeds", async () => { await tool.execute({ patch: updatePatch }, mockTask as Task, { askApproval: mockAskApproval, @@ -557,9 +595,146 @@ describe("ApplyPatchTool.execute - delete file success path", () => { }) expect(mockedCheckpointSave).toHaveBeenCalledOnce() + // B3a: each write threads the approval diff and stats computed by its + // handler so the per-step change card can reuse them verbatim. + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { + path: "src/a.ts", + operation: "create", + diffStats: { additions: 1, deletions: 0 }, + diff: expect.stringContaining("+alpha"), + }, + { + path: "src/b.ts", + operation: "update", + diffStats: { additions: 1, deletions: 1 }, + diff: expect.stringContaining("+second content"), + }, + ]) + }) + }) + + describe("change-card threading (B3a)", () => { + const mockedCheckpointSave = checkpointSave as MockedFunction + const deletePatch = `*** Begin Patch + *** Delete File: src/obsolete.ts + *** End Patch` + + it("threads autoApproved into the checkpoint writes for auto-approved steps", async () => { + // B3a: auto-approved steps carry autoApproved on every write so + // checkpointSave can force the compact (summary) change card. + const ref = (mockTask["providerRef"] as unknown as { deref: MockedFunction<() => unknown> }).deref + ref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ autoApprovalEnabled: true, alwaysAllowWrite: true }), + }) + + await tool.execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ - { path: "src/a.ts", operation: "create" }, - { path: "src/b.ts", operation: "update" }, + { path: "src/obsolete.ts", operation: "delete", autoApproved: true }, + ]) + }) + + it("threads autoApproved into the add writes for auto-approved steps", async () => { + // B3a: the add branch consults auto-approval with the approval message, + // so an auto-approved add threads the compact-card flag into the journal. + mockedFileExistsAtPath.mockResolvedValue(false) + const ref = (mockTask["providerRef"] as unknown as { deref: MockedFunction<() => unknown> }).deref + ref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ autoApprovalEnabled: true, alwaysAllowWrite: true }), + }) + const addPatch = `*** Begin Patch +*** Add File: src/new.ts ++hello ++world +*** End Patch` + + await tool.execute({ patch: addPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { + path: "src/new.ts", + operation: "create", + diffStats: { additions: 2, deletions: 0 }, + diff: expect.stringContaining("+hello"), + autoApproved: true, + }, + ]) + }) + + it("threads autoApproved into the update writes for auto-approved steps", async () => { + // B3a: the update handler (also used for moves) consults auto-approval + // with the approval message, so an auto-approved update threads the + // compact-card flag into the journal. + const ref = (mockTask["providerRef"] as unknown as { deref: MockedFunction<() => unknown> }).deref + ref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ autoApprovalEnabled: true, alwaysAllowWrite: true }), + }) + const updatePatch = `*** Begin Patch +*** Update File: src/test.ts +@@ +-original file content ++modified content +*** End Patch` + + await tool.execute({ patch: updatePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { + path: "src/test.ts", + operation: "update", + diffStats: { additions: 1, deletions: 1 }, + diff: expect.stringContaining("+modified content"), + autoApproved: true, + }, + ]) + }) + + it("records a checkpoint when the provider reference is unavailable", async () => { + // B3a: the delete branch re-fetches the provider state for the + // auto-approval probe; without a provider the probe yields no + // auto-approval, but the delete still records its checkpoint. + const ref = (mockTask["providerRef"] as unknown as { deref: MockedFunction<() => unknown> }).deref + ref.mockReturnValue(undefined) + + await tool.execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Successfully deleted")) + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { path: "src/obsolete.ts", operation: "delete" }, + ]) + }) + + it("omits autoApproved when the step is not auto-approved", async () => { + // The default provider state ({}) disables auto-approval, so no write + // carries the autoApproved flag and the card follows the user setting. + await tool.execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { path: "src/obsolete.ts", operation: "delete" }, ]) }) @@ -592,7 +767,12 @@ describe("ApplyPatchTool.execute - delete file success path", () => { // written, even though the whole patch succeeded. expect(mockedCheckpointSave).toHaveBeenCalledOnce() expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ - { path: "src/new.ts", operation: "create" }, + { + path: "src/new.ts", + operation: "create", + diffStats: { additions: 1, deletions: 0 }, + diff: expect.stringContaining("+fresh content"), + }, ]) }) @@ -623,7 +803,12 @@ describe("ApplyPatchTool.execute - delete file success path", () => { // ...and the successful subset is checkpointed and journaled. expect(mockedCheckpointSave).toHaveBeenCalledOnce() expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ - { path: "src/second.ts", operation: "create" }, + { + path: "src/second.ts", + operation: "create", + diffStats: { additions: 1, deletions: 0 }, + diff: expect.stringContaining("+fresh"), + }, ]) }) diff --git a/src/core/tools/__tests__/editFileTool.spec.ts b/src/core/tools/__tests__/editFileTool.spec.ts index 35501f021a..2f975966b0 100644 --- a/src/core/tools/__tests__/editFileTool.spec.ts +++ b/src/core/tools/__tests__/editFileTool.spec.ts @@ -795,11 +795,13 @@ describe("editFileTool", () => { expect(mockTask.consecutiveMistakeCount).toBe(0) expect(mockedCheckpointSave).toHaveBeenCalledOnce() // B2: the write info threads the path, operation, and the approval - // diff stats into the checkpoint hook. + // diff stats into the checkpoint hook. B3a: the approval diff itself is + // threaded verbatim for the per-step change card. expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask, false, true, { path: testFilePath, operation: "update", diffStats: { additions: 1, deletions: 1 }, + diff: "mock-diff", }) }) @@ -837,11 +839,13 @@ describe("editFileTool", () => { path: testFilePath, operation: "create", diffStats: { additions: 1, deletions: 1 }, + diff: "mock-diff", }) }) it("omits diff stats from the checkpoint write when the diff has no stats", async () => { // A null approval diff produces no diffStats on the journal write. + // The diff itself is still threaded for the change card (B3a). vi.mocked(computeDiffStats).mockReturnValueOnce(null) await executeEditFileTool({}) @@ -851,7 +855,53 @@ describe("editFileTool", () => { expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask, false, true, { path: testFilePath, operation: "update", + diff: "mock-diff", }) }) + + it("threads autoApproved into the checkpoint write for auto-approved steps", async () => { + // B3a: when the step is auto-approved the checkpoint write carries + // autoApproved so checkpointSave can force the compact change card. + mockTask.providerRef.deref = vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: {}, + autoApprovalEnabled: true, + alwaysAllowWrite: true, + }), + }) + + await executeEditFileTool({}) + + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask, false, true, { + path: testFilePath, + operation: "update", + diffStats: { additions: 1, deletions: 1 }, + diff: "mock-diff", + autoApproved: true, + }) + }) + + it("awaits the edit checkpoint before execute settles", async () => { + type SaveResult = Awaited> + let resolveSave: (value: SaveResult | PromiseLike) => void = () => {} + const saveDeferred = new Promise((resolve) => (resolveSave = resolve)) + mockedCheckpointSave.mockImplementationOnce(() => saveDeferred) + + const executePromise = executeEditFileTool({}) + + // execute must not settle while the checkpoint is still in flight: + // a later tool block would otherwise interleave with this edit's staged work. + let settled = false + void executePromise.finally(() => (settled = true)) + await new Promise((resolve) => setTimeout(resolve, 0)) + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(settled).toBe(false) + + resolveSave() + await executePromise + expect(settled).toBe(true) + }) }) }) diff --git a/src/core/tools/__tests__/editSearchReplaceTool.changeCard.spec.ts b/src/core/tools/__tests__/editSearchReplaceTool.changeCard.spec.ts new file mode 100644 index 0000000000..48e5008520 --- /dev/null +++ b/src/core/tools/__tests__/editSearchReplaceTool.changeCard.spec.ts @@ -0,0 +1,274 @@ +// npx vitest run core/tools/__tests__/editSearchReplaceTool.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 { EditTool } from "../EditTool" +import type { ToolCallbacks } from "../BaseTool" +import { SearchReplaceTool } from "../SearchReplaceTool" + +/** + * The shared surface of the two string-replacement edit tools: both execute a + * single `old_string` -> `new_string` replacement and report through the same + * ToolCallbacks, which is all the change-card tests exercise. + */ +interface EditLikeTool { + execute( + params: { file_path: string; old_string: string; new_string: string }, + task: Task, + callbacks: ToolCallbacks, + ): Promise +} + +vi.mock("fs/promises", () => ({ + default: { + // Contains exactly one occurrence of the old_string below. + readFile: vi.fn().mockResolvedValue("old line\n"), + }, +})) + +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" }), +})) + +interface Stubs { + mockTask: Pick< + Task, + | "cwd" + | "consecutiveMistakeCount" + | "recordToolError" + | "rooIgnoreController" + | "rooProtectedController" + | "processQueuedMessages" + | "didEditFile" + | "diffViewProvider" + | "providerRef" + | "fileContextTracker" + > + mockSaveDirectly: MockedFunction<(...args: unknown[]) => Promise> + mockGetState: MockedFunction<() => Promise>> +} + +/** + * 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 edit tools touch. + */ +function buildStubs(): Stubs { + const mockSaveDirectly = vi.fn().mockResolvedValue({ + newProblemsMessage: "", + userEdits: undefined, + finalContent: "new line\n", + }) + const diffViewProviderStub = { + editType: undefined as "create" | "modify" | undefined, + originalContent: undefined as string | undefined, + open: vi.fn().mockResolvedValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + scrollToFirstDiff: vi.fn(), + saveDirectly: mockSaveDirectly, + saveChanges: vi.fn().mockResolvedValue({ + newProblemsMessage: "", + userEdits: undefined, + finalContent: "new line\n", + }), + revertChanges: vi.fn().mockResolvedValue(undefined), + pushToolWriteResult: vi.fn().mockResolvedValue("Saved file"), + reset: vi.fn().mockResolvedValue(undefined), + } + const mockGetState = vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + // Exercise the focus-disruption (saveDirectly) save path. + experiments: { preventFocusDisruption: true }, + }) + const mockTask: Stubs["mockTask"] = { + cwd: "/workspace/project", + consecutiveMistakeCount: 0, + 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, + 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"], + } + return { mockTask, mockSaveDirectly, mockGetState } +} + +function cardTests(getTool: () => EditLikeTool) { + const mockedFileExistsAtPath = fileExistsAtPath as MockedFunction + const mockCheckpointSave = checkpointSave as MockedFunction + const mockCheckAutoApproval = checkAutoApproval as MockedFunction + + let tool: EditLikeTool + let stubs: Stubs + let mockAskApproval: MockedFunction<(...args: unknown[]) => Promise> + let mockHandleError: MockedFunction<(...args: unknown[]) => Promise> + let mockPushToolResult: MockedFunction<(...args: unknown[]) => void> + + beforeEach(() => { + vi.clearAllMocks() + mockedFileExistsAtPath.mockResolvedValue(true) + stubs = buildStubs() + tool = getTool() + mockAskApproval = vi.fn().mockResolvedValue(true) + mockHandleError = vi.fn().mockResolvedValue(undefined) + mockPushToolResult = vi.fn() + }) + + it("records a per-write checkpoint with the approval diff after a successful write", async () => { + await tool.execute( + { file_path: "src/thing.ts", old_string: "old line", new_string: "new line" }, + stubs.mockTask as Task, + { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }, + ) + + expect(mockCheckpointSave).toHaveBeenCalledTimes(1) + expect(mockCheckpointSave).toHaveBeenCalledWith(stubs.mockTask, false, true, { + path: "src/thing.ts", + operation: "update", + diffStats: { additions: 1, deletions: 1 }, + diff: "mock-diff", + }) + // B3a: the auto-approval probe runs on the approval message (the same + // payload the user approved) with the task's workspace root, so the + // compact-card decision is made from exactly what was approved. + expect(mockCheckAutoApproval).toHaveBeenCalledWith( + expect.objectContaining({ ask: "tool", cwd: "/workspace/project", isProtected: false }), + ) + }) + + it("marks auto-approved steps so the card renders compact", async () => { + mockCheckAutoApproval.mockResolvedValueOnce({ decision: "approve" }) + + await tool.execute( + { file_path: "src/thing.ts", old_string: "old line", new_string: "new line" }, + stubs.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("still checkpoints when the provider state is unavailable (default-on)", async () => { + // Without a provider the optional state read must keep the per-write + // checkpoint default (on) instead of hard-failing the write. + const ref = (stubs.mockTask["providerRef"] as unknown as { deref: MockedFunction<() => unknown> }).deref + ref.mockReturnValue(undefined) + + await tool.execute( + { file_path: "src/thing.ts", old_string: "old line", new_string: "new line" }, + stubs.mockTask as Task, + { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }, + ) + + expect(mockCheckpointSave).toHaveBeenCalledTimes(1) + expect(mockCheckpointSave).toHaveBeenCalledWith(stubs.mockTask, false, true, { + path: "src/thing.ts", + operation: "update", + diffStats: { additions: 1, deletions: 1 }, + diff: "mock-diff", + }) + }) + + it("skips the checkpoint when perWriteCheckpoints is explicitly disabled", async () => { + stubs.mockGetState.mockResolvedValueOnce({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: { preventFocusDisruption: true }, + perWriteCheckpoints: false, + }) + + await tool.execute( + { file_path: "src/thing.ts", old_string: "old line", new_string: "new line" }, + stubs.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(stubs.mockSaveDirectly).toHaveBeenCalled() + expect(mockCheckAutoApproval).not.toHaveBeenCalled() + }) + + it("records nothing when the approval is declined", async () => { + mockAskApproval.mockResolvedValue(false) + + await tool.execute( + { file_path: "src/thing.ts", old_string: "old line", new_string: "new line" }, + stubs.mockTask as Task, + { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }, + ) + + expect(mockCheckpointSave).not.toHaveBeenCalled() + expect(stubs.mockSaveDirectly).not.toHaveBeenCalled() + }) +} + +describe("EditTool.execute - per-write checkpoint and change card (B3a, epic #1375)", () => { + cardTests(() => new EditTool()) +}) + +describe("SearchReplaceTool.execute - per-write checkpoint and change card (B3a, epic #1375)", () => { + cardTests(() => new SearchReplaceTool()) +}) diff --git a/src/core/tools/__tests__/editTool.spec.ts b/src/core/tools/__tests__/editTool.spec.ts index a5f665b9e5..d4996b06e3 100644 --- a/src/core/tools/__tests__/editTool.spec.ts +++ b/src/core/tools/__tests__/editTool.spec.ts @@ -59,6 +59,14 @@ vi.mock("../../diff/stats", () => ({ computeDiffStats: vi.fn(() => ({ additions: 1, deletions: 1 })), })) +vi.mock("../../checkpoints", () => ({ + checkpointSave: vi.fn().mockResolvedValue(undefined), +})) + +vi.mock("../../auto-approval", () => ({ + checkAutoApproval: vi.fn().mockResolvedValue({ decision: "ask" }), +})) + vi.mock("vscode", () => ({ window: { showWarningMessage: vi.fn().mockResolvedValue(undefined), diff --git a/src/core/tools/__tests__/searchReplaceTool.spec.ts b/src/core/tools/__tests__/searchReplaceTool.spec.ts index 5cf10790d4..513b7e1ab6 100644 --- a/src/core/tools/__tests__/searchReplaceTool.spec.ts +++ b/src/core/tools/__tests__/searchReplaceTool.spec.ts @@ -59,6 +59,14 @@ vi.mock("../../diff/stats", () => ({ computeDiffStats: vi.fn(() => ({ additions: 1, deletions: 1 })), })) +vi.mock("../../checkpoints", () => ({ + checkpointSave: vi.fn().mockResolvedValue(undefined), +})) + +vi.mock("../../auto-approval", () => ({ + checkAutoApproval: vi.fn().mockResolvedValue({ decision: "ask" }), +})) + vi.mock("vscode", () => ({ window: { showWarningMessage: vi.fn().mockResolvedValue(undefined), diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 1edb9e8ad7..023f5c4707 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -11,6 +11,7 @@ import { ToolUse, ToolResponse, AskApproval, HandleError, PushToolResult } from import { checkpointSave } from "../../checkpoints" import { formatResponse } from "../../prompts/responses" import { writeToFileTool } from "../WriteToFileTool" +import { convertNewFileToUnifiedDiff, sanitizeUnifiedDiff } from "../../diff/stats" vi.mock("path", async () => { const originalPath = await vi.importActual("path") @@ -102,6 +103,10 @@ describe("writeToFileTool", () => { const testContent = "Line 1\nLine 2\nLine 3" const testContentWithMarkdown = "```javascript\nLine 1\nLine 2\n```" + // The exact approval diff the tool computes for a new file (B3a threads it + // into the checkpoint write for the per-step change card). + const newFileApprovalDiff = sanitizeUnifiedDiff(convertNewFileToUnifiedDiff(testContent, testFilePath)) + // Mocked functions with correct types const mockedFileExistsAtPath = fileExistsAtPath as MockedFunction const mockedCreateDirectoriesForFile = createDirectoriesForFile as MockedFunction @@ -489,10 +494,13 @@ describe("writeToFileTool", () => { expect(mockedCheckpointSave).toHaveBeenCalledOnce() // B2: the write info threads the path, operation, and the approval // diff stats (3 added lines, 0 removed) into the checkpoint hook. + // B3a: the approval diff itself is threaded verbatim for the + // per-step change card. expect(mockedCheckpointSave).toHaveBeenCalledWith(mockCline, false, true, { path: testFilePath, operation: "create", diffStats: { additions: 3, deletions: 0 }, + diff: newFileApprovalDiff, }) // The approval message carries the same stats object the journal // receives - not a coerced boolean or a dropped key. @@ -573,7 +581,8 @@ describe("writeToFileTool", () => { await executeWriteFileTool({}) // The experiment branch saves directly (no diff view) and still - // journals the write through the same single checkpoint hook. + // journals the write through the same single checkpoint hook, carrying + // the approval diff for the per-step change card (B3a). expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalledWith( testFilePath, testContent, @@ -585,6 +594,7 @@ describe("writeToFileTool", () => { path: testFilePath, operation: "create", diffStats: { additions: 3, deletions: 0 }, + diff: newFileApprovalDiff, }) // The focus-disruption branch threads the stats into the approval // message as well. @@ -605,5 +615,28 @@ describe("writeToFileTool", () => { operation: "update", }) }) + + it("threads autoApproved into the checkpoint write for auto-approved steps", async () => { + // B3a: when the step is auto-approved the checkpoint write carries + // autoApproved so checkpointSave can force the compact change card. + mockCline.providerRef.deref = vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + autoApprovalEnabled: true, + alwaysAllowWrite: true, + }), + }) + + await executeWriteFileTool({}) + + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockCline, false, true, { + path: testFilePath, + operation: "create", + diffStats: { additions: 3, deletions: 0 }, + diff: newFileApprovalDiff, + autoApproved: true, + }) + }) }) })