feat(tools): wire guarded writes into the diff-view save paths (S4b, #1375)

src/core/task/Task.ts

@@ -297,16 +297,15 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
 	readonly instanceId: string
 	readonly metadata: TaskMetadata
 
-	// The observed on-disk version of each file this task has read. Declared here so the
-	// read tools can record it; a write guard later compares a token against this registry.
-	readonly observationRegistry = new ObservationRegistry()
-
 	todoList?: TodoItem[]
 
 	readonly rootTask: Task | undefined = undefined
 	readonly parentTask: Task | undefined = undefined
 	readonly taskNumber: number
 	readonly workspacePath: string
+	// The observed on-disk version of each file this task has read. Declared here so the
+	// read tools can record it; a write guard later compares a token against this registry.
+	readonly observationRegistry = new ObservationRegistry()
 
 	/**
 	 * The mode associated with this task. Persisted across sessions

src/core/task/__tests__/taskObservationRegistry.spec.ts

@@ -0,0 +1,259 @@
+import { WebviewFocusTracker } from "../../webview/WebviewFocusTracker"
+// npx vitest run core/task/__tests__/flushPendingToolResultsToHistory.spec.ts
+
+import * as os from "os"
+import * as path from "path"
+import * as vscode from "vscode"
+
+import type { GlobalState, ProviderSettings } from "@roo-code/types"
+import { TelemetryService } from "@roo-code/telemetry"
+
+import { Task } from "../Task"
+import { ClineProvider } from "../../webview/ClineProvider"
+import { ContextProxy } from "../../config/ContextProxy"
+import { providerIdentifiers } from "@roo-code/types/provider-identifiers"
+
+// Mock delay before any imports that might use it
+vi.mock("delay", () => ({
+	__esModule: true,
+	default: vi.fn().mockResolvedValue(undefined),
+}))
+
+vi.mock("execa", () => ({
+	execa: vi.fn(),
+}))
+
+vi.mock("../../../utils/safeWriteJson", () => ({
+	safeWriteJson: vi.fn().mockResolvedValue(undefined),
+}))
+
+vi.mock("fs/promises", async (importOriginal) => {
+	const actual = (await importOriginal()) as Record<string, unknown>
+	const mockFunctions = {
+		mkdir: vi.fn().mockResolvedValue(undefined),
+		writeFile: vi.fn().mockResolvedValue(undefined),
+		readFile: vi.fn().mockResolvedValue("[]"),
+		unlink: vi.fn().mockResolvedValue(undefined),
+		rmdir: vi.fn().mockResolvedValue(undefined),
+	}
+
+	return {
+		...actual,
+		...mockFunctions,
+		default: mockFunctions,
+	}
+})
+
+const { mockPWaitFor } = vi.hoisted(() => {
+	return { mockPWaitFor: vi.fn().mockImplementation(async () => Promise.resolve()) }
+})
+
+vi.mock("p-wait-for", () => ({
+	default: mockPWaitFor,
+}))
+
+vi.mock("vscode", () => {
+	const mockDisposable = { dispose: vi.fn() }
+	const mockEventEmitter = { event: vi.fn(), fire: vi.fn() }
+	const mockTextDocument = { uri: { fsPath: "/mock/workspace/path/file.ts" } }
+	const mockTextEditor = { document: mockTextDocument }
+	const mockTab = { input: { uri: { fsPath: "/mock/workspace/path/file.ts" } } }
+	const mockTabGroup = { tabs: [mockTab] }
+
+	return {
+		TabInputTextDiff: vi.fn(),
+		CodeActionKind: {
+			QuickFix: { value: "quickfix" },
+			RefactorRewrite: { value: "refactor.rewrite" },
+		},
+		window: {
+			createTextEditorDecorationType: vi.fn().mockReturnValue({
+				dispose: vi.fn(),
+			}),
+			visibleTextEditors: [mockTextEditor],
+			tabGroups: {
+				all: [mockTabGroup],
+				close: vi.fn(),
+				onDidChangeTabs: vi.fn(() => ({ dispose: vi.fn() })),
+			},
+			showErrorMessage: vi.fn(),
+		},
+		workspace: {
+			workspaceFolders: [
+				{
+					uri: { fsPath: "/mock/workspace/path" },
+					name: "mock-workspace",
+					index: 0,
+				},
+			],
+			createFileSystemWatcher: vi.fn(() => ({
+				onDidCreate: vi.fn(() => mockDisposable),
+				onDidDelete: vi.fn(() => mockDisposable),
+				onDidChange: vi.fn(() => mockDisposable),
+				dispose: vi.fn(),
+			})),
+			fs: {
+				stat: vi.fn().mockResolvedValue({ type: 1 }),
+			},
+			onDidSaveTextDocument: vi.fn(() => mockDisposable),
+			getConfiguration: vi.fn(() => ({ get: (_key: string, defaultValue: unknown) => defaultValue })),
+		},
+		env: {
+			uriScheme: "vscode",
+			language: "en",
+		},
+		EventEmitter: vi.fn().mockImplementation(function () {
+			return mockEventEmitter
+		}),
+		Disposable: {
+			from: vi.fn(),
+		},
+		TabInputText: vi.fn(),
+	}
+})
+
+vi.mock("../../mentions", () => ({
+	parseMentions: vi.fn().mockImplementation((text) => {
+		return Promise.resolve({ text: `processed: ${text}`, mode: undefined, contentBlocks: [] })
+	}),
+	openMention: vi.fn(),
+	getLatestTerminalOutput: vi.fn(),
+}))
+
+vi.mock("../../../integrations/misc/extract-text", () => ({
+	extractTextFromFile: vi.fn().mockResolvedValue("Mock file content"),
+}))
+
+vi.mock("../../environment/getEnvironmentDetails", () => ({
+	getEnvironmentDetails: vi.fn().mockResolvedValue(""),
+}))
+
+vi.mock("../../ignore/RooIgnoreController")
+
+vi.mock("../../condense", async (importOriginal) => {
+	const actual = (await importOriginal()) as Record<string, unknown>
+	return {
+		...actual,
+		summarizeConversation: vi.fn().mockResolvedValue({
+			messages: [{ role: "user", content: [{ type: "text", text: "continued" }], ts: Date.now() }],
+			summary: "summary",
+			cost: 0,
+			newContextTokens: 1,
+		}),
+	}
+})
+
+vi.mock("../../../utils/storage", () => ({
+	getTaskDirectoryPath: vi
+		.fn()
+		.mockImplementation((globalStoragePath, taskId) => Promise.resolve(`${globalStoragePath}/tasks/${taskId}`)),
+	getSettingsDirectoryPath: vi
+		.fn()
+		.mockImplementation((globalStoragePath) => Promise.resolve(`${globalStoragePath}/settings`)),
+}))
+
+vi.mock("../../../utils/fs", () => ({
+	fileExistsAtPath: vi.fn().mockReturnValue(false),
+}))
+
+describe("Task observation registry isolation", () => {
+	let mockProvider: ClineProvider
+	let mockApiConfig: ProviderSettings
+	let mockOutputChannel: vscode.OutputChannel
+	let mockExtensionContext: vscode.ExtensionContext
+
+	beforeEach(() => {
+		if (!TelemetryService.hasInstance()) {
+			TelemetryService.createInstance([])
+		}
+
+		const storageUri = {
+			fsPath: path.join(os.tmpdir(), "test-storage"),
+		}
+
+		mockExtensionContext = {
+			globalState: {
+				get: vi.fn().mockImplementation((_key: keyof GlobalState) => undefined),
+				update: vi.fn().mockImplementation((_key, _value) => Promise.resolve()),
+				keys: vi.fn().mockReturnValue([]),
+			},
+			globalStorageUri: storageUri,
+			workspaceState: {
+				get: vi.fn().mockImplementation((_key) => undefined),
+				update: vi.fn().mockImplementation((_key, _value) => Promise.resolve()),
+				keys: vi.fn().mockReturnValue([]),
+			},
+			secrets: {
+				get: vi.fn().mockImplementation((_key) => Promise.resolve(undefined)),
+				store: vi.fn().mockImplementation((_key, _value) => Promise.resolve()),
+				delete: vi.fn().mockImplementation((_key) => Promise.resolve()),
+			},
+			extensionUri: {
+				fsPath: "/mock/extension/path",
+			},
+			extension: {
+				packageJSON: {
+					version: "1.0.0",
+				},
+			},
+		} as unknown as vscode.ExtensionContext
+
+		mockOutputChannel = {
+			name: "test-output",
+			replace: vi.fn(),
+			appendLine: vi.fn(),
+			append: vi.fn(),
+			clear: vi.fn(),
+			show: vi.fn(),
+			hide: vi.fn(),
+			dispose: vi.fn(),
+		}
+
+		mockProvider = new ClineProvider(
+			mockExtensionContext,
+			mockOutputChannel,
+			"sidebar",
+			new ContextProxy(mockExtensionContext),
+			new WebviewFocusTracker(),
+		)
+
+		mockApiConfig = {
+			apiProvider: providerIdentifiers.anthropic,
+			apiModelId: "claude-3-5-sonnet-20241022",
+			apiKey: "test-api-key",
+		}
+
+		mockProvider.postMessageToWebview = vi.fn().mockResolvedValue(undefined)
+		mockProvider.postStateToWebview = vi.fn().mockResolvedValue(undefined)
+		mockProvider.postStateToWebviewWithoutTaskHistory = vi.fn().mockResolvedValue(undefined)
+		mockProvider.updateTaskHistory = vi.fn().mockResolvedValue(undefined)
+	})
+
+	it("keeps two real Task instances' observation registries independent", async () => {
+		const first = new Task({
+			provider: mockProvider,
+			apiConfiguration: mockApiConfig,
+			task: "first task",
+			startTask: false,
+		})
+		const second = new Task({
+			provider: mockProvider,
+			apiConfiguration: mockApiConfig,
+			task: "second task",
+			startTask: false,
+		})
+
+		// Both start empty, and observing through one task must not reach the other.
+		expect(first.observationRegistry.size).toBe(0)
+		expect(second.observationRegistry.size).toBe(0)
+
+		first.observationRegistry.observe("/shared/file.ts", "1:2:3:4:5")
+
+		expect(first.observationRegistry.has("/shared/file.ts")).toBe(true)
+		expect(second.observationRegistry.has("/shared/file.ts")).toBe(false)
+		expect(second.observationRegistry.size).toBe(0)
+
+		await first.abortTask(true).catch(() => undefined)
+		await second.abortTask(true).catch(() => undefined)
+	})
+})

src/core/tools/ApplyDiffTool.ts

@@ -9,6 +9,7 @@ import { isPathOutsideWorkspace } from "../../utils/pathUtils"
 import { Task } from "../task/Task"
 import { formatResponse } from "../prompts/responses"
 import { fileExistsAtPath } from "../../utils/fs"
+import { versionTokenOfStat } from "../../utils/versionToken"
 import { RecordSource } from "../context-tracking/FileContextTrackerTypes"
 import { unescapeHtmlEntities } from "../../utils/text-normalization"
 import { EXPERIMENT_IDS, experiments } from "../../shared/experiments"
@@ -74,7 +75,25 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
 				return
 			}
 
+			// The diff below is built from this exact read, so the save that follows must be
+			// authorized against the version captured here - not against whatever version the
+			// diff view happens to stat afterwards (there is no diff view on the
+			// focus-disruption path). Same contract as ApplyPatchTool's hunk read: stat
+			// around the read and keep the token only when the file did not change underneath
+			// it. The token stays local until the guarded publish consumes it: observing at
+			// read time would leave it in the task-wide registry through a failed hunk, a
+			// rejection, and the diff-view branch, where a later write_to_file could spend it
+			// on an unrelated overwrite.
+			let readVersionToken: string | undefined
+			const preReadStats = await fs.stat(absolutePath, { bigint: true }).catch(() => undefined)
 			const originalContent: string = await fs.readFile(absolutePath, "utf-8")
+			const postReadStats = await fs.stat(absolutePath, { bigint: true }).catch(() => undefined)
+			if (preReadStats && postReadStats) {
+				const preReadToken = versionTokenOfStat(preReadStats)
+				if (preReadToken === versionTokenOfStat(postReadStats)) {
+					readVersionToken = preReadToken
+				}
+			}
 
 			// Apply the diff to the original content
 			const diffResult = (await task.diffStrategy?.applyDiff(
@@ -177,21 +196,46 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
 					return
 				}
 
-				// Save directly without showing diff view or opening the file
+				// Save directly without showing diff view or opening the file. The diff is
+				// applied to an existing file, so edit-guard semantics require a prior read.
+				// The observation is recorded here, at the guarded publish that consumes it,
+				// so a failed hunk, a rejection, and the diff-view branch leave no token in
+				// the task-wide registry.
 				task.diffViewProvider.editType = "modify"
 				task.diffViewProvider.originalContent = originalContent
+				if (readVersionToken !== undefined) {
+					task.observationRegistry.observe(absolutePath, readVersionToken)
+				}
 				await task.diffViewProvider.saveDirectly(
 					relPath,
 					diffResult.content,
 					false,
 					diagnosticsEnabled,
 					writeDelayMs,
+					"edit",
 				)
 			} else {
 				// Original behavior with diff view
 				// Show diff view before asking for approval
 				task.diffViewProvider.editType = "modify"
 				await task.diffViewProvider.open(relPath)
+				// The diff above was computed against this tool's own read, but open() re-reads
+				// the file and records that newer text as the save baseline. If the file changed
+				// between the two reads, saveChanges() would guard the newer baseline while
+				// publishing content derived from the older one, silently overwriting the write
+				// that landed in between. Fail closed instead: nothing is saved until the model
+				// re-reads and reapplies against the current content.
+				if (task.diffViewProvider.originalContent !== originalContent) {
+					task.consecutiveMistakeCount++
+					task.recordToolError("apply_diff")
+					const staleError = `The file ${relPath} changed after it was read for this diff, so the diff was not applied. Read the file again and reapply the diff to its current content.`
+					await task.say("error", staleError)
+					task.didToolFailInCurrentTurn = true
+					await task.diffViewProvider.revertChanges()
+					task.processQueuedMessages()
+					pushToolResult(staleError)
+					return
+				}
 				await task.diffViewProvider.update(diffResult.content, true)
 				task.diffViewProvider.scrollToFirstDiff()