U9 re cut, rung 4 of 4: task history store, provider wiring and the read and write tools (463 counted lines, 93 selector ranges, headroom 37)

knip.json

@@ -28,7 +28,8 @@
 				"!utils/versionToken.ts!",
 				"!services/tools/Bm25Ranker.ts!",
 				"!services/tools/types.ts!",
-				"!core/tools/helpers/toolResultFormatting.ts!"
+				"!core/tools/helpers/toolResultFormatting.ts!",
+				"!core/tools/guardedWrite.ts!"
 			],
 			"ignoreDependencies": [
 				"@roo-code/config-typescript",

src/core/task-persistence/index.ts

@@ -13,7 +13,7 @@ export {
 } from "./taskMessages"
 export { taskMetadata } from "./taskMetadata"
 export { ensureMessageIdentifiers } from "./mergeMessageSnapshots"
-export { TaskHistoryStore } from "./TaskHistoryStore"
+export { TaskHistoryDeleteError, TaskHistoryStore } from "./TaskHistoryStore"
 export {
 	abandonDelegatedChild,
 	assertValidTransition,

src/core/task/__tests__/Task.dispose.test.ts

@@ -460,6 +460,33 @@ describe("Task.run() idempotency", () => {
 		mockApiConfiguration = { apiProvider: providerIdentifiers.anthropic, apiKey: "test-key" } as ProviderSettings
 	})
 
+	test("each Task constructs its own observation registry", async () => {
+		// The guard authorizes a write from the observations of the task that made it. One
+		// shared registry would let a parent read authorize a subtask write, and one task
+		// clearing its registry would revoke the other task authorization. The field is an
+		// instance initializer, so this asserts the ownership rather than the type.
+		const first = new Task({
+			provider: mockProvider as unknown as ClineProvider,
+			apiConfiguration: mockApiConfiguration,
+			task: "first",
+			startTask: false,
+		})
+		const second = new Task({
+			provider: mockProvider as unknown as ClineProvider,
+			apiConfiguration: mockApiConfiguration,
+			task: "second",
+			startTask: false,
+		})
+
+		expect(first.observationRegistry).not.toBe(second.observationRegistry)
+		first.observationRegistry.observe("/same/path.ts", "1:2:22:100:100", true)
+		expect(second.observationRegistry.get("/same/path.ts")).toBeUndefined()
+		expect(second.observationRegistry.size).toBe(0)
+
+		await first.dispose()
+		await second.dispose()
+	})
+
 	test("run() does not invoke startTask when task was already started by constructor", async () => {
 		// Spy on the prototype before construction so we capture the constructor's call too.
 		const startTaskSpy = vi.spyOn(Task.prototype as any, "startTask").mockResolvedValue(undefined)

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

@@ -70,6 +70,43 @@ describe("ObservationRegistry", () => {
 		expect(regB.get("/shared.ts")!.version).toBe("v2")
 	})
 
+	describe("forget", () => {
+		it("removes the entry and reports that it removed one", () => {
+			const registry = new ObservationRegistry()
+			registry.observe("/a.ts", "1:1:1:1:1", true)
+
+			expect(registry.forget("/a.ts")).toBe(true)
+			expect(registry.has("/a.ts")).toBe(false)
+			expect(registry.get("/a.ts")).toBeUndefined()
+			expect(registry.size).toBe(0)
+		})
+
+		it("reports false for a path it never observed, and changes nothing", () => {
+			const registry = new ObservationRegistry()
+			registry.observe("/a.ts", "1:1:1:1:1", true)
+
+			expect(registry.forget("/missing.ts")).toBe(false)
+			expect(registry.size).toBe(1)
+			expect(registry.get("/a.ts")?.version).toBe("1:1:1:1:1")
+		})
+
+		it("leaves the other entries alone - that is why a caller uses forget and not clear", () => {
+			// A caller revoking an authorization it did not earn must not discard the
+			// observations other reads of the same task still rely on.
+			const registry = new ObservationRegistry()
+			registry.observe("/a.ts", "1:1:1:1:1", true)
+			registry.observe("/b.ts", "2:2:2:2:2", false)
+
+			expect(registry.forget("/a.ts")).toBe(true)
+			expect(registry.get("/b.ts")).toEqual({
+				version: "2:2:2:2:2",
+				observedAt: expect.any(Number),
+				complete: false,
+			})
+			expect(registry.size).toBe(1)
+		})
+	})
+
 	describe("completeness scope (S4b follow-up #46)", () => {
 		it("defaults to a complete observation when the read scope is not given", () => {
 			const reg = new ObservationRegistry()

src/core/task/observationRegistry.ts

@@ -56,6 +56,16 @@ export class ObservationRegistry {
 		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()
 	}

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"
@@ -15,6 +16,7 @@ import { EXPERIMENT_IDS, experiments } from "../../shared/experiments"
 import { computeDiffStats, sanitizeUnifiedDiff } from "../diff/stats"
 import type { DiffResult, ToolUse } from "../../shared/tools"
 
+import { canonicalizeForApproval } from "./guardedWrite"
 import { BaseTool, ToolCallbacks } from "./BaseTool"
 
 interface ApplyDiffParams {
@@ -74,7 +76,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(
@@ -139,7 +166,19 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
 
 			// Check if file is write-protected
 			const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath) || false
+
+			// The guard in guardedWrite refuses a target outside every workspace root unless the tool
+			// layer says the user approved this one, and it binds the publish to the identity that was
+			// approved. apply_diff has both save paths behind an askApproval, so it has to carry both
+			// through; without them an approved write outside the workspace is rejected by its own guard.
 			const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)
+			// Captured before the approval is asked: guardedWrite binds the publish to this identity, so
+			// a name repointed between the approval and the publish is refused instead of publishing to
+			// whatever the name points at by then. Inside the try so an unresolvable target goes through
+			// the tool's normal error handling and the diff-view reset.
+			const approvedCanonicalTarget = isOutsideWorkspace
+				? await canonicalizeForApproval(absolutePath, relPath)
+				: undefined
 
 			const sharedMessageProps: ClineSayTool = {
 				tool: "appliedDiff",
@@ -177,7 +216,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(
@@ -186,6 +226,12 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
 					false,
 					diagnosticsEnabled,
 					writeDelayMs,
+					"edit",
+					// Seventh parameter is completeOverride (this call claims no completeness of its
+					// own); the eighth is the approval flag for a target outside every workspace root.
+					undefined,
+					isOutsideWorkspace,
+					approvedCanonicalTarget,
 				)
 			} else {
 				// Original behavior with diff view
@@ -225,7 +271,13 @@ 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",
+					isOutsideWorkspace,
+					approvedCanonicalTarget,
+				)
 			}
 
 			// Track file edit operation