feat(tools): route the remaining write tools through the guard (U7, #1375)

src/core/task-persistence/__tests__/TaskHistoryStore.deleteSemantics.spec.ts

@@ -36,6 +36,20 @@ vi.mock("fs/promises", async () => {
 const actualFs = await vi.importActual<typeof import("fs/promises")>("fs/promises")
 const actualFileLock = await vi.importActual<typeof import("../../../utils/fileLock")>("../../../utils/fileLock")
 
+// The delete path hands fs.unlink and the per-file lock a canonical spelling - it comes out of
+// resolveLockKey (services/file-safety/safeWriteText.ts:367), which follows links - while this file's
+// own expected paths come from the spelling os.tmpdir() answered. On a hosted Windows runner those
+// two spellings name the same file while differing as strings (the 8.3 form of the profile directory
+// against its long name), so comparing paths with === pins a spelling rather than a file: the
+// injected EACCES stops recognising its own target, the real unlink runs, and the read-back rows
+// see ENOENT. Reproduced on this machine by pointing TMP/TEMP at a junction alias of a real
+// directory, which produced the same four failures the hosted Windows lane reported.
+async function sameFileAs(candidate: unknown, expected: string): Promise<boolean> {
+	const resolved = await fs.realpath(String(candidate)).catch(() => String(candidate))
+	const expectedCanonical = await fs.realpath(expected).catch(() => expected)
+	return resolved === expectedCanonical
+}
+
 function makeHistoryItem(overrides: Partial<HistoryItem> = {}): HistoryItem {
 	return {
 		id: `task-${Date.now()}-${Math.random().toString(36).substring(2, 8)}`,
@@ -137,7 +151,8 @@ describe("TaskHistoryStore best-effort deletion semantics", () => {
 			const backup = path.join(path.dirname(filePath), `.${path.basename(filePath)}.bak_1_saved.tmp`)
 			await fs.writeFile(backup, JSON.stringify(history))
 			vi.mocked(fs.unlink).mockImplementation(async (target) => {
-				if (target === backup) throw Object.assign(new Error("Cleanup failed"), { code: "EACCES" })
+				if (await sameFileAs(target, backup))
+					throw Object.assign(new Error("Cleanup failed"), { code: "EACCES" })
 				return actualFs.unlink(target)
 			})
 			if (operation === "delete") await store.delete(history.id)
@@ -156,10 +171,28 @@ describe("TaskHistoryStore best-effort deletion semantics", () => {
 
 			await expect(store.delete("locked-delete")).resolves.toBeUndefined()
 
-			expect(vi.mocked(withFileLock)).toHaveBeenCalledWith(
-				historyFilePath(storagePath, "locked-delete"),
-				expect.any(Function),
-			)
+			// Production hands withFileLock the canonical spelling of the key (it comes out of
+			// resolveLockKey, which follows links), while historyFilePath builds this row's path from
+			// the spelling os.tmpdir() answered. On a hosted Windows runner those two name the same
+			// file and differ as strings - the 8.3 profile directory against its long name - so a string
+			// match pins a spelling. This row pins the file instead: the key the store locked must name
+			// this file inside the directory object this row's path resolves to. The parent directory is
+			// what gets canonicalised rather than the file, because the delete removes the file and after
+			// that neither realpath nor stat can answer for it.
+			const expectedFile = historyFilePath(storagePath, "locked-delete")
+			const expectedDir = await fs.realpath(path.dirname(expectedFile)).catch(() => path.dirname(expectedFile))
+			let matchingKeys = 0
+			let matchedHandlerType = ""
+			for (const call of vi.mocked(withFileLock).mock.calls) {
+				const acquired = String(call[0])
+				const acquiredDir = await fs.realpath(path.dirname(acquired)).catch(() => path.dirname(acquired))
+				if (acquiredDir === expectedDir && path.basename(acquired) === path.basename(expectedFile)) {
+					matchingKeys++
+					matchedHandlerType = typeof call[1]
+				}
+			}
+			expect(matchingKeys).toBeGreaterThan(0)
+			expect(matchedHandlerType).toBe("function")
 			await expect(fs.access(historyFilePath(storagePath, "locked-delete"))).rejects.toMatchObject({
 				code: "ENOENT",
 			})
@@ -242,7 +275,7 @@ describe("TaskHistoryStore best-effort deletion semantics", () => {
 			onWrite.mockClear()
 
 			vi.mocked(fs.unlink).mockImplementation(async (p) => {
-				if (p === historyFilePath(storagePath, "batch-b")) {
+				if (await sameFileAs(p, historyFilePath(storagePath, "batch-b"))) {
 					throw Object.assign(new Error("EACCES: permission denied, unlink"), { code: "EACCES" })
 				}
 				return actualFs.unlink(p)

src/core/task/Task.ts

@@ -106,6 +106,7 @@ import { buildNativeToolsArrayWithRestrictions } from "./build-tools"
 import { ToolRepetitionDetector } from "../tools/ToolRepetitionDetector"
 import { restoreTodoListForTask } from "../tools/UpdateTodoListTool"
 import { FileContextTracker } from "../context-tracking/FileContextTracker"
+import { ObservationRegistry } from "./observationRegistry"
 import { RooIgnoreController } from "../ignore/RooIgnoreController"
 import { RooProtectedController } from "../protect/RooProtectedController"
 import { type AssistantMessageContent, presentAssistantMessage } from "../assistant-message"
@@ -296,6 +297,10 @@ 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

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

@@ -0,0 +1,108 @@
+import { describe, it, expect, vi } from "vitest"
+
+import { ObservationRegistry } from "../observationRegistry"
+
+describe("ObservationRegistry", () => {
+	it("observe → get returns the recorded version and observedAt", () => {
+		const reg = new ObservationRegistry()
+		reg.observe("/a/b/c.ts", "1:2:300:4000000000:5000000000")
+
+		const obs = reg.get("/a/b/c.ts")
+		expect(obs).toBeDefined()
+		expect(obs!.version).toBe("1:2:300:4000000000:5000000000")
+		expect(typeof obs!.observedAt).toBe("number")
+	})
+
+	it("re-observe replaces the entry with a fresh observedAt", () => {
+		vi.useFakeTimers()
+		const reg = new ObservationRegistry()
+		reg.observe("/a/b/c.ts", "v1")
+		const first = reg.get("/a/b/c.ts")!
+		expect(first.version).toBe("v1")
+
+		vi.advanceTimersByTime(50)
+		reg.observe("/a/b/c.ts", "v2")
+		const second = reg.get("/a/b/c.ts")!
+		expect(second.version).toBe("v2")
+		expect(second.observedAt).toBeGreaterThan(first.observedAt)
+
+		vi.useRealTimers()
+	})
+
+	it("has returns true for observed paths, false otherwise", () => {
+		const reg = new ObservationRegistry()
+		reg.observe("/x.ts", "t1")
+		expect(reg.has("/x.ts")).toBe(true)
+		expect(reg.has("/y.ts")).toBe(false)
+	})
+
+	it("size reflects the number of observed entries", () => {
+		const reg = new ObservationRegistry()
+		expect(reg.size).toBe(0)
+		reg.observe("/a.ts", "t1")
+		reg.observe("/b.ts", "t2")
+		expect(reg.size).toBe(2)
+	})
+
+	it("clear removes all entries and resets size to 0", () => {
+		const reg = new ObservationRegistry()
+		reg.observe("/a.ts", "t1")
+		reg.observe("/b.ts", "t2")
+		reg.clear()
+		expect(reg.size).toBe(0)
+		expect(reg.get("/a.ts")).toBeUndefined()
+		expect(reg.has("/b.ts")).toBe(false)
+	})
+
+	it("get on empty registry returns undefined", () => {
+		const reg = new ObservationRegistry()
+		expect(reg.get("/any.ts")).toBeUndefined()
+	})
+
+	it("separate instances are independent — observing in one does not appear in the other", () => {
+		const regA = new ObservationRegistry()
+		const regB = new ObservationRegistry()
+		regA.observe("/shared.ts", "v1")
+		expect(regA.get("/shared.ts")).toBeDefined()
+		expect(regB.get("/shared.ts")).toBeUndefined()
+		regB.observe("/shared.ts", "v2")
+		expect(regA.get("/shared.ts")!.version).toBe("v1")
+		expect(regB.get("/shared.ts")!.version).toBe("v2")
+	})
+
+	describe("completeness scope (S4b follow-up #46)", () => {
+		it("defaults to a complete observation when the read scope is not given", () => {
+			const reg = new ObservationRegistry()
+			reg.observe("/a/b/c.ts", "v1")
+
+			expect(reg.get("/a/b/c.ts")!.complete).toBe(true)
+		})
+
+		it("records a partial observation when the read only returned a view of the file", () => {
+			const reg = new ObservationRegistry()
+			reg.observe("/a/b/c.ts", "v1", false)
+
+			expect(reg.get("/a/b/c.ts")!.complete).toBe(false)
+		})
+
+		it("re-observing replaces the entry's completeness with the new read's scope", () => {
+			const reg = new ObservationRegistry()
+			reg.observe("/a/b/c.ts", "v1", false)
+			reg.observe("/a/b/c.ts", "v2")
+
+			const obs = reg.get("/a/b/c.ts")!
+			expect(obs.version).toBe("v2")
+			expect(obs.complete).toBe(true)
+		})
+
+		it("re-observing with a partial scope downgrades a previously complete entry", () => {
+			const reg = new ObservationRegistry()
+			reg.observe("/a/b/c.ts", "v1")
+			reg.observe("/a/b/c.ts", "v2", false)
+
+			const obs = reg.get("/a/b/c.ts")!
+			expect(obs.version).toBe("v2")
+			expect(obs.complete).toBe(false)
+		})
+	})
+})

src/core/task/observationRegistry.ts

@@ -0,0 +1,69 @@
+/**
+ * Per-task file observation registry (upstream epic #1375, phase A2).
+ *
+ * Each Task owns its own instance so parent and subtask observations are
+ * independent. The S4 guarded-write will compare these versions against the
+ * token recomputed pre-write to detect stale reads or file replacement.
+ *
+ * Pure in-memory — zero I/O, no dependencies. The S4 guarded-write consults
+ * these observations for the version check and for the completeness check that
+ * gates a full-file replacement.
+ */
+
+export interface FileObservation {
+	/** Version token derived from on-disk fs.stat (bigint mode). */
+	version: string
+	/** Millisecond timestamp when the observation was recorded. */
+	observedAt: number
+	/**
+	 * Whether the read that produced this observation returned the complete
+	 * file. A slice, line-range, truncated, or indentation-block read returns
+	 * only a view of the file; such an observation authorizes targeted edits
+	 * on the view the model saw, but never a full-file replacement.
+	 */
+	complete: boolean
+}
+
+export class ObservationRegistry {
+	private readonly entries = new Map<string, FileObservation>()
+
+	/**
+	 * Record an observation for a file at its absolute path.
+	 *
+	 * Re-observing replaces the entry with a fresh observedAt timestamp, the
+	 * new version token, and the read's completeness. `complete` defaults to
+	 * true for callers that read the whole file themselves (spec doubles,
+	 * WriteToFileTool). A caller whose read is internal to a targeted edit must
+	 * carry the model's prior completeness instead, so the tool's own read cannot
+	 * upgrade a partial read into authority for a full-file replacement.
+	 */
+	observe(absolutePath: string, version: string, complete: boolean = true): void {
+		this.entries.set(absolutePath, { version, observedAt: Date.now(), complete })
+	}
+
+	get(absolutePath: string): FileObservation | undefined {
+		return this.entries.get(absolutePath)
+	}
+
+	has(absolutePath: string): boolean {
+		return this.entries.has(absolutePath)
+	}
+
+	/**
+	 * Drop the observation for one path. A caller that must revoke an
+	 * authorization it did not earn - a preview that observed a version the model
+	 * never read - needs this instead of clear(), which would also discard the
+	 * observations other reads of the same task still rely on.
+	 */
+	forget(absolutePath: string): boolean {
+		return this.entries.delete(absolutePath)
+	}
+
+	clear(): void {
+		this.entries.clear()
+	}
+
+	get size(): number {
+		return this.entries.size
+	}
+}

src/core/tools/ApplyDiffTool.ts

@@ -6,6 +6,7 @@ import { TelemetryService } from "@roo-code/telemetry"
 
 import { getReadablePath } from "../../utils/path"
 import { isPathOutsideWorkspace } from "../../utils/pathUtils"
+import { versionTokenOfStat } from "../../utils/versionToken"
 import { Task } from "../task/Task"
 import { formatResponse } from "../prompts/responses"
 import { fileExistsAtPath } from "../../utils/fs"
@@ -38,6 +39,20 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
 			diffContent = unescapeHtmlEntities(diffContent)
 		}
 
+		// Identity of a partial observation this call created (undefined when the model had
+		// already observed the file): only this exact observation may be removed on a failure
+		// exit, so a replaced or published observation is never discarded here. Declared outside
+		// the try so the catch exit can use it too.
+		let partialObservation: { path: string; version: string } | undefined
+		const forgetPartialObservationIfCurrent = () => {
+			if (
+				partialObservation &&
+				task.observationRegistry.get(partialObservation.path)?.version === partialObservation.version
+			) {
+				task.observationRegistry.forget(partialObservation.path)
+			}
+		}
+
 		try {
 			if (!relPath) {
 				task.consecutiveMistakeCount++
@@ -74,7 +89,33 @@ 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
+			// preview happens to stat afterwards. Same contract as ApplyPatchTool's hunk read:
+			// stat around the read and observe only when the file did not change underneath it.
+			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)) {
+					// A tool read is not a model read. With no prior observation this stays a
+					// partial observation of the version the diff was computed against - the only
+					// authorization the save can have, since apply_diff computes its hunks from
+					// this read. When the model already observed the file, keep the completeness it
+					// earned, but only on the version it was earned on: refreshing an OLDER
+					// observation to the current version would let content the model built from a
+					// stale read pass the compare-and-swap, so an out-of-date observation is left
+					// alone and the save fails with the re-read remediation.
+					const prior = task.observationRegistry.get(absolutePath)
+					if (prior === undefined) {
+						task.observationRegistry.observe(absolutePath, preReadToken, false)
+						partialObservation = { path: absolutePath, version: preReadToken }
+					} else if (prior.version === preReadToken) {
+						task.observationRegistry.observe(absolutePath, preReadToken, prior.complete === true)
+					}
+				}
+			}
 
 			// Apply the diff to the original content
 			const diffResult = (await task.diffStrategy?.applyDiff(
@@ -116,6 +157,7 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
 				task.recordToolError("apply_diff", formattedError)
 
 				pushToolResult(formattedError)
+				forgetPartialObservationIfCurrent()
 				return
 			}
 
@@ -174,10 +216,12 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
 				const didApprove = await askApproval("tool", completeMessage, toolProgressStatus, isWriteProtected)
 
 				if (!didApprove) {
+					forgetPartialObservationIfCurrent()
 					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.
 				task.diffViewProvider.editType = "modify"
 				task.diffViewProvider.originalContent = originalContent
 				await task.diffViewProvider.saveDirectly(
@@ -186,6 +230,7 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
 					false,
 					diagnosticsEnabled,
 					writeDelayMs,
+					"edit",
 				)
 			} else {
 				// Original behavior with diff view
@@ -220,12 +265,13 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
 
 				if (!didApprove) {
 					await task.diffViewProvider.revertChanges()
+					forgetPartialObservationIfCurrent()
 					task.processQueuedMessages()
 					return
 				}
 
 				// Call saveChanges to update the DiffViewProvider properties
-				await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs)
+				await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs, "edit")
 			}
 
 			// Track file edit operation
@@ -276,6 +322,7 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
 			return
 		} catch (error) {
 			await handleError("applying diff", error as Error)
+			forgetPartialObservationIfCurrent()
 			await task.diffViewProvider.reset()
 			this.resetPartialState()
 			task.processQueuedMessages()