feat(editor): route the diff-view save through the guard (U8, #1375)

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__/Task.observationRegistry.spec.ts

@@ -0,0 +1,89 @@
+import { type ProviderSettings } from "@roo-code/types"
+
+import { Task } from "../Task"
+import { ObservationRegistry } from "../observationRegistry"
+import type { ClineProvider } from "../../webview/ClineProvider"
+import { providerIdentifiers } from "@roo-code/types/provider-identifiers"
+
+// Real-Task isolation proof for the observationRegistry field this PR adds to
+// Task. The unit spec covers the registry's own semantics; this one pins the
+// ownership contract the field exists for: every Task owns its registry, so a
+// parent's observations never authorize a subtask's writes (and vice versa).
+// The construction boilerplate mirrors Task.dispose.test.ts.
+vi.mock("../../webview/ClineProvider")
+vi.mock("../../../integrations/terminal/TerminalRegistry", () => ({
+	TerminalRegistry: {
+		releaseTerminalsForTask: vi.fn(),
+	},
+}))
+vi.mock("../../../utils/storage", () => ({
+	getTaskDirectoryPath: vi.fn().mockResolvedValue("/test/path/tasks/test-task"),
+}))
+vi.mock("../../../integrations/terminal/OutputInterceptor", () => ({
+	OutputInterceptor: {
+		cleanup: vi.fn().mockResolvedValue(undefined),
+	},
+}))
+vi.mock("../../ignore/RooIgnoreController")
+vi.mock("../../protect/RooProtectedController")
+vi.mock("../../context-tracking/FileContextTracker")
+vi.mock("../../../integrations/editor/DiffViewProvider")
+vi.mock("../../tools/ToolRepetitionDetector")
+vi.mock("../../../api", () => ({
+	buildApiHandler: vi.fn(() => ({
+		getModel: () => ({ info: {}, id: "test-model" }),
+	})),
+}))
+vi.mock("@roo-code/telemetry", () => ({
+	TelemetryService: {
+		instance: {
+			captureTaskCreated: vi.fn(),
+			captureTaskRestarted: vi.fn(),
+		},
+	},
+}))
+
+describe("Task observation registry ownership", () => {
+	const makeProvider = () => ({
+		context: { globalStorageUri: { fsPath: "/test/path" } },
+		getState: vi.fn().mockResolvedValue({ mode: "code" }),
+		log: vi.fn(),
+		flushPostStateToWebviewThrottled: vi.fn().mockResolvedValue(undefined),
+	})
+	const apiConfiguration = {
+		apiProvider: providerIdentifiers.anthropic,
+		apiKey: "test-key",
+	} as ProviderSettings
+
+	it("gives every real Task its own registry, isolated from every other", async () => {
+		const taskA = new Task({
+			provider: makeProvider() as unknown as ClineProvider,
+			apiConfiguration,
+			startTask: false,
+		})
+		const taskB = new Task({
+			provider: makeProvider() as unknown as ClineProvider,
+			apiConfiguration,
+			startTask: false,
+		})
+
+		try {
+			expect(taskA.observationRegistry).toBeInstanceOf(ObservationRegistry)
+			expect(taskB.observationRegistry).toBeInstanceOf(ObservationRegistry)
+			// Two real Tasks, two registries: a shared or static field fails here.
+			expect(taskA.observationRegistry).not.toBe(taskB.observationRegistry)
+
+			taskA.observationRegistry.observe("/tmp/observed-by-a.json", "version-a")
+
+			expect(taskA.observationRegistry.get("/tmp/observed-by-a.json")?.version).toBe("version-a")
+			// The other task's registry must not contain the observation: an
+			// observation earned by one task cannot authorize the other's writes.
+			expect(taskB.observationRegistry.get("/tmp/observed-by-a.json")).toBeUndefined()
+			expect(taskB.observationRegistry.has("/tmp/observed-by-a.json")).toBe(false)
+			expect(taskB.observationRegistry.size).toBe(0)
+		} finally {
+			await taskA.dispose().catch(() => {})
+			await taskB.dispose().catch(() => {})
+		}
+	})
+})

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

@@ -0,0 +1,135 @@
+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)
+		})
+	})
+
+	it("forget() removes exactly the given path and reports it", () => {
+		// DiffViewProvider revokes the observation for the preview path it published. Removing
+		// a neighbour's entry would make that neighbour's later write look untouched.
+		const reg = new ObservationRegistry()
+		reg.observe("/a/b/c.ts", "v1")
+		reg.observe("/a/b/d.ts", "v2")
+
+		expect(reg.forget("/a/b/c.ts")).toBe(true)
+
+		expect(reg.get("/a/b/c.ts")).toBeUndefined()
+		expect(reg.get("/a/b/d.ts")?.version).toBe("v2")
+		expect(reg.size).toBe(1)
+	})
+
+	it("forget() reports false and changes nothing for a path that was never observed", () => {
+		// A cancellation can forget a path whose observation was already dropped. The caller
+		// distinguishes the two cases through this return value, so it must not throw and must
+		// leave the other entries alone.
+		const reg = new ObservationRegistry()
+		reg.observe("/a/b/c.ts", "v1")
+
+		expect(reg.forget("/a/b/absent.ts")).toBe(false)
+
+		expect(reg.get("/a/b/c.ts")?.version).toBe("v1")
+		expect(reg.size).toBe(1)
+	})
+})

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

@@ -5,6 +5,7 @@ import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types"
 import { TelemetryService } from "@roo-code/telemetry"
 
 import { getReadablePath } from "../../utils/path"
+import { versionTokenOfStat } from "../../utils/versionToken"
 import { Task } from "../task/Task"
 import { formatResponse } from "../prompts/responses"
 import { fileExistsAtPath } from "../../utils/fs"
@@ -73,7 +74,32 @@ 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)
+					} 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(
@@ -174,7 +200,8 @@ 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.
 				task.diffViewProvider.editType = "modify"
 				task.diffViewProvider.originalContent = originalContent
 				await task.diffViewProvider.saveDirectly(
@@ -183,6 +210,11 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
 					false,
 					diagnosticsEnabled,
 					writeDelayMs,
+					"edit",
+					// ApplyDiffTool composes the patch from the bytes its own read produced, and the
+					// guard must judge that observation: a stale token stays stale, a partial read stays
+					// partial. This caller does not re-observe the basis.
+					false,
 				)
 			} else {
 				// Original behavior with diff view
@@ -222,7 +254,7 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
 				}
 
 				// Call saveChanges to update the DiffViewProvider properties
-				await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs)
+				await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs, "edit")
 			}
 
 			// Track file edit operation