feat(tools): guarded write CAS core with per-path FIFO chain (S4a, #1375)

knip.json

@@ -16,7 +16,12 @@
 	"playwright-ct": false,
 	"workspaces": {
 		"src": {
-			"entry": ["extension.ts!", "extension/api.ts!", "workers/countTokens.ts!"],
+			"entry": [
+				"extension.ts!",
+				"extension/api.ts!",
+				"workers/countTokens.ts!",
+				"services/file-safety/safeWriteText.ts!"
+			],
 			"project": [
 				"**/*.ts!",
 				"!**/__tests__/**!",
@@ -26,6 +31,7 @@
 				"!utils/WorkspacePathResolver.ts!",
 				"!test-utils/**!",
 				"!utils/versionToken.ts!",
+				"!core/tools/guardedWrite.ts!",
 				"!services/tools/Bm25Ranker.ts!",
 				"!services/tools/types.ts!",
 				"!core/tools/helpers/toolResultFormatting.ts!"

src/core/config/__tests__/importExport.spec.ts

@@ -24,6 +24,7 @@ import { safeWriteJson } from "../../../utils/safeWriteJson"
 
 import type { Mock } from "vitest"
 import { providerIdentifiers, retiredProviderIdentifiers } from "@roo-code/types/provider-identifiers"
+import { t } from "../../../i18n"
 
 function createDeferred<T>() {
 	let resolve!: (value: T) => void
@@ -1616,10 +1617,14 @@ describe("importExport", () => {
 			expect(mockContextProxy.export).toHaveBeenCalled()
 			expect(fs.mkdir).toHaveBeenCalledWith("/mock/path", { recursive: true })
 
-			expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/zoo-code-settings.json", {
-				providerProfiles: mockProviderProfiles,
-				globalSettings: mockGlobalSettings,
-			})
+			expect(safeWriteJson).toHaveBeenCalledWith(
+				"/mock/path/zoo-code-settings.json",
+				{
+					providerProfiles: mockProviderProfiles,
+					globalSettings: mockGlobalSettings,
+				},
+				{ refuseSymlinkTarget: true },
+			)
 		})
 
 		it("should include globalSettings when allowedMaxRequests is null", async () => {
@@ -1648,10 +1653,14 @@ describe("importExport", () => {
 				contextProxy: mockContextProxy,
 			})
 
-			expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/zoo-code-settings.json", {
-				providerProfiles: mockProviderProfiles,
-				globalSettings: mockGlobalSettings,
-			})
+			expect(safeWriteJson).toHaveBeenCalledWith(
+				"/mock/path/zoo-code-settings.json",
+				{
+					providerProfiles: mockProviderProfiles,
+					globalSettings: mockGlobalSettings,
+				},
+				{ refuseSymlinkTarget: true },
+			)
 		})
 
 		it("should handle errors during the export process", async () => {
@@ -1678,10 +1687,52 @@ describe("importExport", () => {
 			expect(mockProviderSettingsManager.export).toHaveBeenCalled()
 			expect(mockContextProxy.export).toHaveBeenCalled()
 			expect(fs.mkdir).toHaveBeenCalledWith("/mock/path", { recursive: true })
-			expect(safeWriteJson).toHaveBeenCalled() // safeWriteJson is called, but it will throw
-			// The error is caught and the function exits silently.
-			// Optionally, ensure no error message was shown if that's part of "silent"
-			// expect(vscode.window.showErrorMessage).not.toHaveBeenCalled();
+		})
+
+		it("notifies the user when the export write is refused, and still swallows the failure", async () => {
+			;(vscode.window.showSaveDialog as Mock).mockResolvedValue({
+				fsPath: "/mock/path/zoo-code-settings.json",
+			})
+
+			mockProviderSettingsManager.export.mockResolvedValue({
+				currentApiConfigName: "test",
+				apiConfigs: { test: { apiProvider: providerIdentifiers.openai, id: "test-id" } },
+				migrations: { rateLimitSecondsMigrated: false },
+			})
+			mockContextProxy.export.mockResolvedValue({ mode: "code" })
+
+			// The refusal this unit introduced: refuseSymlinkTarget is set at the call above, so a
+			// link at the path the user picked makes the writer reject instead of publishing.
+			const refusal = new Error(
+				"safeWriteJson: refusing to write through the symlink now at /mock/path/zoo-code-settings.json (before publication); the payload would land at /elsewhere/settings.json",
+			)
+			;(safeWriteJson as Mock).mockRejectedValueOnce(refusal)
+
+			const showErrorMessageSpy = vi.spyOn(vscode.window, "showErrorMessage").mockResolvedValue(undefined)
+			const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {})
+
+			// The contract the caller relies on: webviewMessageHandler awaits exportSettings without a
+			// try, so a refusal must not become an unhandled rejection.
+			await expect(
+				exportSettings({
+					providerSettingsManager: mockProviderSettingsManager,
+					contextProxy: mockContextProxy,
+				}),
+			).resolves.toBeUndefined()
+
+			// The user learns the file did not land, through the same channel and the same key shape the
+			// import route uses. Expected is computed with the same t() call the module makes, so the
+			// assertion holds whether this lane resolves t to the string or to the key.
+			expect(showErrorMessageSpy).toHaveBeenCalledTimes(1)
+			expect(showErrorMessageSpy).toHaveBeenCalledWith(
+				t("common:errors.settings_export_failed", { error: refusal.message }),
+			)
+			expect(showErrorMessageSpy).toHaveBeenCalledWith(expect.stringContaining("errors.settings_export_failed"))
+			expect(showErrorMessageSpy).not.toHaveBeenCalledWith(expect.stringContaining("settings_import_failed"))
+			expect(consoleErrorSpy).toHaveBeenCalledWith("Failed to export settings:", refusal)
+
+			showErrorMessageSpy.mockRestore()
+			consoleErrorSpy.mockRestore()
 		})
 
 		it("should handle errors during directory creation", async () => {
@@ -1771,10 +1822,14 @@ describe("importExport", () => {
 					contextProxy: mockContextProxy,
 				})
 
-				expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/zoo-code-settings.json", {
-					providerProfiles: mockProviderProfiles,
-					globalSettings: mockGlobalSettings,
-				})
+				expect(safeWriteJson).toHaveBeenCalledWith(
+					"/mock/path/zoo-code-settings.json",
+					{
+						providerProfiles: mockProviderProfiles,
+						globalSettings: mockGlobalSettings,
+					},
+					{ refuseSymlinkTarget: true },
+				)
 			})
 
 			it("should export model dimension for OpenAI Compatible provider", async () => {
@@ -1924,10 +1979,14 @@ describe("importExport", () => {
 				})
 
 				// Should not throw an error and should preserve original settings
-				expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/zoo-code-settings.json", {
-					providerProfiles: mockProviderProfiles,
-					globalSettings: mockGlobalSettings, // Should remain unchanged
-				})
+				expect(safeWriteJson).toHaveBeenCalledWith(
+					"/mock/path/zoo-code-settings.json",
+					{
+						providerProfiles: mockProviderProfiles,
+						globalSettings: mockGlobalSettings, // Should remain unchanged
+					},
+					{ refuseSymlinkTarget: true },
+				)
 			})
 
 			it("should maintain backward compatibility with existing exports", async () => {
@@ -1967,10 +2026,14 @@ describe("importExport", () => {
 				})
 
 				// Should not modify settings for non-openai-compatible providers
-				expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/zoo-code-settings.json", {
-					providerProfiles: mockProviderProfiles,
-					globalSettings: mockGlobalSettings, // Should remain unchanged
-				})
+				expect(safeWriteJson).toHaveBeenCalledWith(
+					"/mock/path/zoo-code-settings.json",
+					{
+						providerProfiles: mockProviderProfiles,
+						globalSettings: mockGlobalSettings, // Should remain unchanged
+					},
+					{ refuseSymlinkTarget: true },
+				)
 			})
 
 			it("should handle missing current provider gracefully", async () => {
@@ -2012,10 +2075,14 @@ describe("importExport", () => {
 				})
 
 				// Should not throw an error and should preserve original settings
-				expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/zoo-code-settings.json", {
-					providerProfiles: mockProviderProfiles,
-					globalSettings: mockGlobalSettings, // Should remain unchanged
-				})
+				expect(safeWriteJson).toHaveBeenCalledWith(
+					"/mock/path/zoo-code-settings.json",
+					{
+						providerProfiles: mockProviderProfiles,
+						globalSettings: mockGlobalSettings, // Should remain unchanged
+					},
+					{ refuseSymlinkTarget: true },
+				)
 			})
 		})
 
@@ -2249,10 +2316,14 @@ describe("importExport", () => {
 			})
 
 			// Step 4: Verify the exported data includes the model dimension
-			expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/test-settings.json", {
-				providerProfiles: mockProviderProfiles,
-				globalSettings: mockGlobalSettings,
-			})
+			expect(safeWriteJson).toHaveBeenCalledWith(
+				"/mock/path/test-settings.json",
+				{
+					providerProfiles: mockProviderProfiles,
+					globalSettings: mockGlobalSettings,
+				},
+				{ refuseSymlinkTarget: true },
+			)
 
 			// Step 5: Get the exported data for import test
 			const exportedData = (safeWriteJson as Mock).mock.calls[0][1]

src/core/config/importExport.ts

@@ -346,10 +346,18 @@ export const exportSettings = async ({ providerSettingsManager, contextProxy }:
 
 		const dirname = path.dirname(uri.fsPath)
 		await fs.mkdir(dirname, { recursive: true })
-		await safeWriteJson(uri.fsPath, { providerProfiles, globalSettings })
+		await safeWriteJson(uri.fsPath, { providerProfiles, globalSettings }, { refuseSymlinkTarget: true })
 	} catch (e) {
 		console.error("Failed to export settings:", e)
-		// Don't re-throw - the UI will handle showing error messages
+		// Don't re-throw - this route has no result object for the caller to report, so the
+		// notification happens here, the way the import route notifies through
+		// settings_import_failed below. A refusal to publish (a link at the chosen path the user
+		// never picked) is otherwise invisible: the dialog closes, the log line is not user-facing,
+		// and the file the user asked for does not exist. The writer's own message names the path it
+		// refused and the destination it would have written through, so the reason travels with the
+		// message.
+		const reason = e instanceof Error ? e.message : String(e)
+		await vscode.window.showErrorMessage(t("common:errors.settings_export_failed", { error: reason }))
 	}
 }
 

src/core/task/Task.ts

@@ -107,6 +107,7 @@ import { ToolRepetitionDetector } from "../tools/ToolRepetitionDetector"
 import { restoreTodoListForTask } from "../tools/UpdateTodoListTool"
 import { FileContextTracker } from "../context-tracking/FileContextTracker"
 import { RooIgnoreController } from "../ignore/RooIgnoreController"
+import { ObservationRegistry } from "./observationRegistry"
 import { RooProtectedController } from "../protect/RooProtectedController"
 import { type AssistantMessageContent, presentAssistantMessage } from "../assistant-message"
 import { DispatchState, type PendingWork } from "./dispatchState"
@@ -302,6 +303,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
 	readonly parentTask: Task | undefined = undefined
 	readonly taskNumber: number
 	readonly workspacePath: string
+	readonly observationRegistry = new ObservationRegistry()
 
 	/**
 	 * The mode associated with this task. Persisted across sessions
@@ -1464,6 +1466,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
 	/** Cancels the current persistence generation before creating the next assistant-turn boundary. */
 	private resetAssistantMessagePersistence(): void {
 		this.cancelAssistantMessagePersistence()
+
 		this.assistantMessagePersistencePromise = new Promise<AssistantMessagePersistenceResult>((resolve) => {
 			this.resolveAssistantMessagePersistence = resolve
 		})
@@ -3696,6 +3699,12 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
 		this.cancelAssistantMessagePersistence()
 		this.cancelPendingPartialMessageUpdate()
 
+		// Drop the per-task file observations. The registry holds an entry per absolute
+		// path the task read or wrote (version token + timestamp); nothing can consume them
+		// after disposal, and a long-lived extension host would otherwise keep every path a
+		// finished task touched alive.
+		this.observationRegistry.close()
+
 		// Release handler-owned host resources (e.g. VS Code configuration listeners).
 		// This is the fallback for leases that never settle; later lease releases
 		// find nothing retired and so cannot dispose twice.

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

@@ -1496,6 +1496,71 @@ describe("Cline", () => {
 		})
 	})
 
+	describe("observation registry lifecycle (S4a, epic #1375)", () => {
+		it("gives each Task its own observation registry", () => {
+			const firstTask = new Task({
+				provider: mockProvider,
+				apiConfiguration: mockApiConfig,
+				task: "first observation task",
+				startTask: false,
+			})
+			const secondTask = new Task({
+				provider: mockProvider,
+				apiConfiguration: mockApiConfig,
+				task: "second observation task",
+				startTask: false,
+			})
+
+			// The guarded-write contract assumes an observation in one task never validates
+			// a write issued by another task.
+			expect(firstTask.observationRegistry).not.toBe(secondTask.observationRegistry)
+			firstTask.observationRegistry.observe("/workspace/a.ts", "v-a")
+			expect(firstTask.observationRegistry.get("/workspace/a.ts")?.version).toBe("v-a")
+			expect(secondTask.observationRegistry.get("/workspace/a.ts")).toBeUndefined()
+		})
+
+		it("clears the observation registry when the task is disposed", async () => {
+			const task = new Task({
+				provider: mockProvider,
+				apiConfiguration: mockApiConfig,
+				task: "disposed observation task",
+				startTask: false,
+			})
+			task.observationRegistry.observe("/workspace/a.ts", "v-a")
+			task.observationRegistry.observe("/workspace/b.ts", "v-b")
+
+			await task.dispose()
+
+			// A disposed task cannot serve another guarded write, so its observed paths
+			// (version token + timestamp each) must not stay reachable for the host lifetime.
+			expect(task.observationRegistry.get("/workspace/a.ts")).toBeUndefined()
+			expect(task.observationRegistry.get("/workspace/b.ts")).toBeUndefined()
+			// Closing is what makes a later observation refuse, so it is asserted rather than
+			// left to the test name: clearing the map alone would look identical from here.
+			expect(task.observationRegistry.isClosed).toBe(true)
+		})
+
+		it("refuses an observation recorded after the task was disposed", async () => {
+			const task = new Task({
+				provider: mockProvider,
+				apiConfiguration: mockApiConfig,
+				task: "late observation task",
+				startTask: false,
+			})
+
+			await task.dispose()
+
+			// A read that was already in flight when the task was disposed can finish late and
+			// call observe(). Recording then would repopulate a registry no task owns and hand a
+			// version token to a guarded write that will never happen.
+			task.observationRegistry.observe("/workspace/late.ts", "v-late")
+
+			expect(task.observationRegistry.get("/workspace/late.ts")).toBeUndefined()
+			expect(task.observationRegistry.size).toBe(0)
+			expect(task.observationRegistry.isClosed).toBe(true)
+		})
+	})
+
 	describe("create", () => {
 		it("rejects missing task inputs before constructing the task", () => {
 			const onCreated = vi.fn()