U9 re cut, rung 3 of 4: diff view publish, apply patch and the three edit tools (492 counted lines, 102 selector ranges, headroom 8)

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/__tests__/TaskHistoryStore.deleteSemantics.spec.ts

@@ -54,6 +54,16 @@ function historyFilePath(storagePath: string, taskId: string): string {
 	return path.join(storagePath, "tasks", taskId, GlobalFileNames.historyItem)
 }
 
+// The lock key is canonicalized through the parent directory, so the expected key is
+// the canonical directory plus the basename rather than the path built from
+// os.tmpdir(), which can be an 8.3 short path on the Windows runner. canonicalDirKey,
+// in src/services/file-safety/safeWriteText.ts, canonicalizes the nearest existing
+// ancestor with fs/promises realpath, and that call expands the short spelling, so the
+// expectation is canonicalized the way the delete takes the lock.
+async function canonicalKey(filePath: string): Promise<string> {
+	return path.join(await actualFs.realpath(path.dirname(filePath)), path.basename(filePath))
+}
+
 function storeInternals(store: TaskHistoryStore): {
 	cache: Map<string, HistoryItem>
 	taskFileMtimes: Map<string, number>
@@ -152,12 +162,17 @@ describe("TaskHistoryStore best-effort deletion semantics", () => {
 			const store = createStore()
 			await store.initialize()
 			await store.upsert(makeHistoryItem({ id: "locked-delete" }))
+			// The lock spy is created once for the whole file, so clear it to make the
+			// assertion below about this delete and not about every case before it.
+			vi.mocked(withFileLock).mockClear()
 			onWrite.mockClear()
 
 			await expect(store.delete("locked-delete")).resolves.toBeUndefined()
 
+			// The delete takes the lock on the canonical key, so the expectation is built the
+			// same way rather than from the spelling os.tmpdir handed to mkdtemp.
 			expect(vi.mocked(withFileLock)).toHaveBeenCalledWith(
-				historyFilePath(storagePath, "locked-delete"),
+				await canonicalKey(historyFilePath(storagePath, "locked-delete")),
 				expect.any(Function),
 			)
 			await expect(fs.access(historyFilePath(storagePath, "locked-delete"))).rejects.toMatchObject({

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/ApplyPatchTool.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 { versionTokenOfStat } from "../../utils/versionToken"
+import { canonicalizeForApproval, GuardRejectedError, errorCode } from "./guardedWrite"
 import { BaseTool, ToolCallbacks } from "./BaseTool"
 import type { ToolUse } from "../../shared/tools"
 import { parsePatch, ParseError, processAllHunks } from "./apply-patch"
@@ -85,10 +87,38 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
 				return
 			}
 
-			// Process each hunk
+			// Process each hunk. The read doubles as the S2 observation for the
+			// guarded publish (ReadFileTool contract: stat before and after the
+			// read, observe only when the on-disk version is unchanged between the
+			// two stats). Without it, the in-place modify publish is an unobserved
+			// write and the composed chat-diff default rejects it ("File already
+			// exists ... and was not read before this write") even though this tool
+			// just read the exact content the patch was applied to.
 			const readFile = async (filePath: string): Promise<string> => {
 				const absolutePath = path.resolve(task.cwd, filePath)
-				return await fs.readFile(absolutePath, "utf8")
+				const preReadStats = await fs.stat(absolutePath, { bigint: true }).catch(() => undefined)
+				const content: string = await fs.readFile(absolutePath, "utf8")
+				const postReadStats = await fs.stat(absolutePath, { bigint: true }).catch(() => undefined)
+				if (preReadStats && postReadStats) {
+					const preReadToken = versionTokenOfStat(preReadStats)
+					if (preReadToken === versionTokenOfStat(postReadStats)) {
+						// The tool's own hunk read, not a model read. With no prior observation the model has
+						// seen only the patch's hunks, so the read stays incomplete: it must not become authority
+						// for a later full-file replacement. A prior observation is carried - partial stays partial,
+						// complete stays complete - only while it still describes the version that was read. An
+						// OLDER observation is left exactly as the model earned it: refreshing it to the version this
+						// read published would hand the guarded save a compare-and-swap token for content the model
+						// never saw, so a patch built against v1 would overwrite an external v2 without the re-read
+						// remediation. Same stale policy as ApplyDiffTool's read.
+						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)
+						}
+					}
+				}
+				return content
 			}
 
 			let changes: ApplyPatchFileChange[]
@@ -160,6 +190,12 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
 
 		const newContent = change.newContent || ""
 		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.
+		const approvedCanonicalTarget = isOutsideWorkspace
+			? await canonicalizeForApproval(absolutePath, relPath)
+			: undefined
 
 		// Initialize diff view for new file
 		task.diffViewProvider.editType = "create"
@@ -214,9 +250,31 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
 
 		// Save the changes
 		if (isPreventFocusDisruptionEnabled) {
-			await task.diffViewProvider.saveDirectly(relPath, newContent, true, diagnosticsEnabled, writeDelayMs)
+			// Guarded publish: the patch supplies the complete new content, so create-guard
+			// semantics apply (an unobserved existing target is rejected, not overwritten).
+			await task.diffViewProvider.saveDirectly(
+				relPath,
+				newContent,
+				true,
+				diagnosticsEnabled,
+				writeDelayMs,
+				"create",
+				// 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 {
-			await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs)
+			// The add path publishes a whole new file, so create-guard semantics
+			// apply here as well.
+			await task.diffViewProvider.saveChanges(
+				diagnosticsEnabled,
+				writeDelayMs,
+				"create",
+				isOutsideWorkspace,
+				approvedCanonicalTarget,
+			)
 		}
 
 		// Track file edit operation
@@ -310,6 +368,12 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
 		const originalContent = change.originalContent || ""
 		const newContent = change.newContent || ""
 		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.
+		const approvedCanonicalTarget = isOutsideWorkspace
+			? await canonicalizeForApproval(absolutePath, relPath)
+			: undefined
 
 		// Initialize diff view
 		task.diffViewProvider.editType = "modify"
@@ -396,6 +460,12 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
 
 			// Check if destination path is outside workspace
 			const isMoveOutsideWorkspace = isPathOutsideWorkspace(moveAbsolutePath)
+			// A destination outside every workspace root is refused right here, so the publish at the
+			// end of this branch is always an inside-workspace one: it needs neither the approval flag
+			// nor a canonical identity. Capturing the identity here instead - as this did - ran
+			// canonicalizeForApproval for a destination that was about to be refused, so a destination
+			// whose name could not be resolved surfaced an exception where the user should get the tool
+			// error below.
 			if (isMoveOutsideWorkspace) {
 				task.consecutiveMistakeCount++
 				task.recordToolError("apply_patch")
@@ -408,12 +478,63 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
 
 			// Save new content to the new path
 			if (isPreventFocusDisruptionEnabled) {
+				// The destination content is the source file plus one targeted hunk, so it can
+				// only be as complete as the view the model had of the source. Carry that
+				// completeness to the destination BEFORE the guarded publish: the guard decides
+				// completeness at publish time, so a partial view must already be recorded when
+				// the per-path chain runs. Downgrading only after saveDirectly leaves a window
+				// in which a concurrent writer sees the destination as complete for content the
+				// model never fully read.
+				const sourceObs = task.observationRegistry.get(absolutePath)
+				const sourceComplete = sourceObs !== undefined && sourceObs.complete === true
+				if (!sourceComplete) {
+					// Only carry completeness the model already had for the destination. An
+					// unobserved destination stays unobserved so the guard's "read before a
+					// full-file write" rule still applies; recording a partial observation here
+					// would hand the model authority to edit a file it never read.
+					const destObs = task.observationRegistry.get(moveAbsolutePath)
+					if (destObs !== undefined) {
+						let destAbsent = false
+						try {
+							await fs.access(moveAbsolutePath)
+						} catch (error: unknown) {
+							// Only an access error that means "not there" is an absence verdict. An
+							// ELOOP or EACCES is a real I/O failure: reading it as absent would mark
+							// the destination partial and let the publish run against a path the tool
+							// never actually observed.
+							if (errorCode(error) !== "ENOENT") throw error
+							destAbsent = true
+						}
+						if (!destAbsent) {
+							// Replacing an existing destination with content built from a partial
+							// source view must be re-authorized by reading the source in full. Reject
+							// before any state changes and name the source: a remediation that names
+							// only the destination sends the model to re-read the wrong file, and a
+							// downgrade that survives a rejected publish loses a full destination read.
+							throw new GuardRejectedError(
+								`Cannot move a partially read file onto ${change.movePath}: re-read the whole source (${change.path}) first, then retry.`,
+								change.movePath,
+							)
+						}
+						// The destination was read and then deleted: the create guard permits the
+						// publish, and the content is still only as complete as the view the model
+						// had of the source, so carry the source's completeness instead of the
+						// destination's stale one.
+						task.observationRegistry.observe(moveAbsolutePath, destObs.version, false)
+					}
+				}
 				await task.diffViewProvider.saveDirectly(
 					change.movePath,
 					newContent,
 					false,
 					diagnosticsEnabled,
 					writeDelayMs,
+					"create",
+					sourceComplete,
+					// Guaranteed by the refusal above: this destination is inside a workspace root,
+					// so the guard's own containment check is what applies and no approval is claimed.
+					false,
+					undefined,
 				)
 			} else {
 				// Write to new path and delete old file
@@ -433,9 +554,33 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
 		} else {
 			// Save changes to the same file
 			if (isPreventFocusDisruptionEnabled) {
-				await task.diffViewProvider.saveDirectly(relPath, newContent, false, diagnosticsEnabled, writeDelayMs)
+				// Guarded publish: an update hunk is a targeted change, so it is guarded
+				// as an edit -- a partial observation authorizes it and stays partial. A
+				// stale observed version is still rejected with the re-read hint.
+				await task.diffViewProvider.saveDirectly(
+					relPath,
+					newContent,
+					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 {
-				await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs)
+				// The diff-view save is the same targeted hunk as the guarded save above:
+				// it must select the same edit guard, otherwise a partial read is
+				// rejected and the approved patch is thrown away.
+				await task.diffViewProvider.saveChanges(
+					diagnosticsEnabled,
+					writeDelayMs,
+					"edit",
+					isOutsideWorkspace,
+					approvedCanonicalTarget,
+				)
 			}
 
 			await task.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource)