feat(file-safety): guarded text publish module + specs (A4, #1375)

src/integrations/editor/DiffViewProvider.ts

@@ -1,6 +1,7 @@
 import * as vscode from "vscode"
 import * as path from "path"
 import * as fs from "fs/promises"
+import { constants as fsConstants } from "fs"
 import * as diff from "diff"
 import stripBom from "strip-bom"
 import delay from "delay"
@@ -18,6 +19,11 @@ import { arePathsEqual, getReadablePath } from "../../utils/path"
 import { formatResponse } from "../../core/prompts/responses"
 import { diagnosticsToProblemsString, getNewDiagnostics } from "../diagnostics"
 import { Task } from "../../core/task/Task"
+import {
+	assertPublishWithinAuthorizedRoot,
+	isPublishCommittedError,
+	safeWriteText,
+} from "../../services/file-safety/safeWriteText"
 
 import { DecorationController } from "./DecorationController"
 
@@ -1155,9 +1161,44 @@ export class DiffViewProvider {
 		// Get diagnostics before editing the file
 		this.preDiagnostics = vscode.languages.getDiagnostics()
 
+		// Reject an escape through an intermediate symlink before anything is
+		// created on disk: createDirectoriesForFile follows symlinks, so an
+		// approved inside path whose intermediate link points outside the
+		// workspace would otherwise create the outside directory before
+		// safeWriteText's authorizedRoot check rejected the publish. The same
+		// canonical check runs inside safeWriteText; running it here first keeps
+		// the allowlist decision free of filesystem side effects.
+		await assertPublishWithinAuthorizedRoot(this.cwd, absolutePath, absolutePath)
+
 		// Write the content directly to the file
 		await createDirectoriesForFile(absolutePath)
-		await fs.writeFile(absolutePath, content, "utf-8")
+		// safeWriteText publishes with rename, which only needs write permission on the
+		// DIRECTORY. The previous fs.writeFile required it on the file itself, so a target
+		// the user marked read-only used to fail with EACCES; keep that contract instead of
+		// silently renaming over it. A not-yet-existing target stays creatable.
+		try {
+			await fs.access(absolutePath, fsConstants.W_OK)
+		} catch (error: unknown) {
+			const code =
+				typeof error === "object" && error !== null && "code" in error
+					? (error as { code?: string }).code
+					: undefined
+			if (code !== "ENOENT") throw error
+		}
+		try {
+			await safeWriteText(absolutePath, content, { authorizedRoot: this.cwd })
+		} catch (error) {
+			// Both errors are raised after the atomic rename committed the content: they
+			// describe a durability or access-rights condition the tool layer cannot fix by
+			// writing again. Treat the save as completed so the tool's bookkeeping matches
+			// the disk (and content already on disk is not retried), and surface the
+			// condition as a warning instead.
+			if (isPublishCommittedError(error)) {
+				console.warn(`saveDirectly: content committed with a warning: ${error.message}`)
+			} else {
+				throw error
+			}
+		}
 
 		// Open the document to ensure diagnostics are loaded
 		// When openFile is false (PREVENT_FOCUS_DISRUPTION enabled), we only open in memory

src/integrations/editor/__tests__/DiffViewProvider.spec.ts

@@ -18,8 +18,23 @@ vi.mock("fs/promises", () => ({
 	readFile: vi.fn().mockResolvedValue("file content"),
 	writeFile: vi.fn().mockResolvedValue(undefined),
 	access: vi.fn().mockResolvedValue(undefined),
+	mkdir: vi.fn().mockResolvedValue(undefined),
+	rename: vi.fn().mockResolvedValue(undefined),
+	unlink: vi.fn().mockResolvedValue(undefined),
 }))
 
+// Mock safeWriteText (used by saveDirectly). The real module's error classes stay
+// available through importOriginal, so a test can hand the caller an actual
+// DaclRestoreError instead of a look-alike.
+vi.mock("../../../services/file-safety/safeWriteText", async (importOriginal) => {
+	const actual = await importOriginal<typeof import("../../../services/file-safety/safeWriteText")>()
+	return {
+		...actual,
+		safeWriteText: vi.fn().mockResolvedValue(undefined),
+		assertPublishWithinAuthorizedRoot: vi.fn().mockResolvedValue(undefined),
+	}
+})
+
 // Mock utils
 vi.mock("../../../utils/fs", () => ({
 	createDirectoriesForFile: vi.fn().mockResolvedValue([]),
@@ -29,6 +44,8 @@ vi.mock("../../../utils/fs", () => ({
 vi.mock("path", () => ({
 	resolve: vi.fn((cwd, relPath) => `${cwd}/${relPath}`),
 	basename: vi.fn((path) => path.split("/").pop()),
+	dirname: vi.fn((path) => path.split("/").slice(0, -1).join("/") || "/"),
+	join: (...args: string[]) => args.join("/"),
 }))
 
 // Mock vscode
@@ -770,6 +787,153 @@ describe("DiffViewProvider", () => {
 	})
 
 	describe("saveDirectly method", () => {
+		it("rejects an escaped target before creating directories for it", async () => {
+			const { assertPublishWithinAuthorizedRoot, safeWriteText } =
+				await import("../../../services/file-safety/safeWriteText")
+			const { createDirectoriesForFile } = await import("../../../utils/fs")
+			vi.mocked(createDirectoriesForFile).mockClear()
+			vi.mocked(safeWriteText).mockClear()
+			vi.mocked(assertPublishWithinAuthorizedRoot).mockClear()
+			const escape = new Error("safeWriteText: refusing to publish outside the authorized root")
+			vi.mocked(assertPublishWithinAuthorizedRoot).mockRejectedValueOnce(escape)
+
+			await expect(diffViewProvider.saveDirectly("linkdir/new/secret.txt", "x", false, false, 0)).rejects.toThrow(
+				"refusing to publish",
+			)
+
+			// The allow-list decision must precede the filesystem mutation:
+			// createDirectoriesForFile follows symlinks, so running it first would
+			// already have created the outside directory the check exists to protect.
+			expect(createDirectoriesForFile).not.toHaveBeenCalled()
+			expect(safeWriteText).not.toHaveBeenCalled()
+			// The stub rejects whatever it is handed, so without pinning the arguments this test
+			// would still pass if the guard were given the workspace root instead of the target.
+			expect(assertPublishWithinAuthorizedRoot).toHaveBeenCalledWith(
+				mockCwd,
+				`${mockCwd}/linkdir/new/secret.txt`,
+				`${mockCwd}/linkdir/new/secret.txt`,
+			)
+			vi.mocked(assertPublishWithinAuthorizedRoot).mockResolvedValue(undefined)
+		})
+		it("rejects a save over a read-only target instead of renaming onto it", async () => {
+			const fs = await import("fs/promises")
+			const { safeWriteText } = await import("../../../services/file-safety/safeWriteText")
+			vi.mocked(safeWriteText).mockClear()
+			const eacces = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" })
+			vi.mocked(fs.access).mockRejectedValueOnce(eacces)
+
+			await expect(diffViewProvider.saveDirectly("test.ts", "new content", true, true, 200)).rejects.toThrow(
+				"EACCES",
+			)
+
+			// The old fs.writeFile path failed the same way; safeWriteText must not run.
+			expect(safeWriteText).not.toHaveBeenCalled()
+			vi.mocked(fs.access).mockResolvedValue(undefined)
+		})
+
+		it("still writes when the target does not exist yet", async () => {
+			const fs = await import("fs/promises")
+			const { safeWriteText } = await import("../../../services/file-safety/safeWriteText")
+			vi.mocked(safeWriteText).mockClear()
+			vi.mocked(fs.access).mockRejectedValueOnce(Object.assign(new Error("ENOENT"), { code: "ENOENT" }))
+
+			const result = await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 200)
+
+			expect(safeWriteText).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", { authorizedRoot: mockCwd })
+			// Call counts alone would still pass if the publish ran but the caller reported
+			// nothing about the content it wrote.
+			expect(result.finalContent).toBe("new content")
+			expect(result.userEdits).toBeUndefined()
+			vi.mocked(fs.access).mockResolvedValue(undefined)
+		})
+
+		it("checks write permission on the absolute target before publishing", async () => {
+			const fs = await import("fs/promises")
+			const { safeWriteText } = await import("../../../services/file-safety/safeWriteText")
+			vi.mocked(safeWriteText).mockClear()
+			vi.mocked(fs.access).mockClear()
+
+			await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 200)
+
+			// safeWriteText publishes with rename, which only needs write permission on the
+			// DIRECTORY. The contract this unit preserves requires it on the FILE, so the guard
+			// must be handed the absolute target - not the relative path the caller passed.
+			const calls = vi.mocked(fs.access).mock.calls
+			const accessIndex = calls.findIndex((call) => String(call[0]).endsWith("test.ts"))
+			expect(accessIndex).toBeGreaterThanOrEqual(0)
+			expect(calls[accessIndex][0]).toBe(`${mockCwd}/test.ts`)
+
+			// An existence-only check would not catch a read-only target: the guard has to be handed
+			// the write bit, otherwise the read-only contract this unit preserves is silently gone.
+			const { constants } = await import("fs")
+			expect(calls[accessIndex][1]).toBe(constants.W_OK)
+
+			// The check is only meaningful before the publish; after it, an EACCES target would
+			// already have been renamed over.
+			expect(vi.mocked(fs.access).mock.invocationCallOrder[accessIndex]).toBeLessThan(
+				vi.mocked(safeWriteText).mock.invocationCallOrder[0],
+			)
+		})
+
+		it("reports a post-commit DACL restore failure as a warning instead of a failed save", async () => {
+			const { DaclRestoreError, safeWriteText } = await import("../../../services/file-safety/safeWriteText")
+			vi.mocked(safeWriteText).mockClear()
+			const warn = vi.spyOn(console, "warn").mockImplementation(() => {})
+			// The primitive reports a lost DACL after the commit rename as DaclRestoreError:
+			// the content is already on disk, so re-writing cannot change the access rights.
+			// The tool layer must still record the write as done, or it keeps stale state and
+			// retries content that is already published.
+			vi.mocked(safeWriteText).mockRejectedValueOnce(new DaclRestoreError(`${mockCwd}/test.ts`, null))
+
+			const result = await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 200)
+
+			expect(result.finalContent).toBe("new content")
+			expect(warn).toHaveBeenCalledWith(expect.stringContaining("content committed with a warning"))
+			const publishCalls = vi.mocked(safeWriteText).mock.calls.filter(function (call) {
+				return call[0] === `${mockCwd}/test.ts`
+			})
+			expect(publishCalls).toHaveLength(1)
+			warn.mockRestore()
+		})
+
+		it("reports an unconfirmed-durability publish as a warning instead of a failed save", async () => {
+			const { PublishNotDurableError, safeWriteText } =
+				await import("../../../services/file-safety/safeWriteText")
+			vi.mocked(safeWriteText).mockClear()
+			const warn = vi.spyOn(console, "warn").mockImplementation(() => {})
+			// The rename landed before the durability failure was observed, so the content is
+			// committed: a re-write would duplicate the work and skip the save bookkeeping.
+			vi.mocked(safeWriteText).mockRejectedValueOnce(
+				new PublishNotDurableError(`${mockCwd}/test.ts`, "directory fsync failed"),
+			)
+
+			const result = await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 200)
+
+			expect(result.finalContent).toBe("new content")
+			expect(warn).toHaveBeenCalledWith(expect.stringContaining("content committed with a warning"))
+			warn.mockRestore()
+		})
+
+		it("rethrows an ordinary publish failure instead of reporting a committed write", async () => {
+			const { safeWriteText } = await import("../../../services/file-safety/safeWriteText")
+			vi.mocked(safeWriteText).mockClear()
+			vi.mocked(vscode.window.showTextDocument).mockClear()
+			const warn = vi.spyOn(console, "warn").mockImplementation(() => {})
+			// Only the committed-publish classes are tolerated here. An ordinary failure means the
+			// bytes never landed, so the tool layer must reject rather than record the save, and
+			// must not open a document for content that was never published.
+			const ordinary = new Error("EACCES: ordinary publish failure")
+			vi.mocked(safeWriteText).mockRejectedValueOnce(ordinary)
+
+			await expect(diffViewProvider.saveDirectly("test.ts", "new content", true, true, 200)).rejects.toBe(
+				ordinary,
+			)
+
+			expect(vscode.window.showTextDocument).not.toHaveBeenCalled()
+			expect(warn).not.toHaveBeenCalledWith(expect.stringContaining("content committed with a warning"))
+			warn.mockRestore()
+		})
+
 		beforeEach(() => {
 			// Mock vscode functions
 			vi.mocked(vscode.window.showTextDocument).mockResolvedValue(makeTextEditor())
@@ -782,9 +946,9 @@ describe("DiffViewProvider", () => {
 
 			const result = await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 2000)
 
-			// Verify file was written
-			const fs = await import("fs/promises")
-			expect(fs.writeFile).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", "utf-8")
+			// Verify file was written via safeWriteText
+			const { safeWriteText } = await import("../../../services/file-safety/safeWriteText")
+			expect(safeWriteText).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", { authorizedRoot: mockCwd })
 
 			// Verify file was opened without focus
 			expect(vscode.window.showTextDocument).toHaveBeenCalledWith(
@@ -805,9 +969,9 @@ describe("DiffViewProvider", () => {
 		it("should not open file when openWithoutFocus is false", async () => {
 			await diffViewProvider.saveDirectly("test.ts", "new content", false, true, 1000)
 
-			// Verify file was written
-			const fs = await import("fs/promises")
-			expect(fs.writeFile).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", "utf-8")
+			// Verify file was written via safeWriteText
+			const { safeWriteText } = await import("../../../services/file-safety/safeWriteText")
+			expect(safeWriteText).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", { authorizedRoot: mockCwd })
 
 			// Verify file was NOT opened
 			expect(vscode.window.showTextDocument).not.toHaveBeenCalled()
@@ -820,9 +984,9 @@ describe("DiffViewProvider", () => {
 
 			await diffViewProvider.saveDirectly("test.ts", "new content", true, false, 1000)
 
-			// Verify file was written
-			const fs = await import("fs/promises")
-			expect(fs.writeFile).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", "utf-8")
+			// Verify file was written via safeWriteText
+			const { safeWriteText } = await import("../../../services/file-safety/safeWriteText")
+			expect(safeWriteText).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", { authorizedRoot: mockCwd })
 
 			// Verify delay was NOT called
 			expect(mockDelay).not.toHaveBeenCalled()

src/services/code-index/__tests__/cache-manager.spec.ts

@@ -7,9 +7,10 @@ import { CacheManager } from "../cache-manager"
 import { clearAllMocks } from "../../../test-utils/reset"
 
 // Mock safeWriteJson utility
-vitest.mock("../../../utils/safeWriteJson", () => ({
-	safeWriteJson: vitest.fn().mockResolvedValue(undefined),
-}))
+vitest.mock("../../../utils/safeWriteJson", async () => {
+	const actual = await vi.importActual<typeof import("../../../utils/safeWriteJson")>("../../../utils/safeWriteJson")
+	return { ...actual, safeWriteJson: vi.fn().mockResolvedValue(undefined) }
+})
 
 // Import the mocked version
 import { safeWriteJson } from "../../../utils/safeWriteJson"
@@ -184,6 +185,25 @@ describe("CacheManager", () => {
 			expect(cacheManager.getAllHashes()).toEqual({})
 		})
 
+		it("clears in-memory hashes when the empty cache committed but its durability was unconfirmed", async () => {
+			cacheManager.updateHash("test.ts", "hash")
+			const { PublishNotDurableError } = await import("../../file-safety/safeWriteText")
+			const consoleWarnSpy = vitest.spyOn(console, "warn").mockImplementation(function () {})
+			const committedError = new PublishNotDurableError(mockCachePath.fsPath, "EIO")
+			;(safeWriteJson as Mock).mockRejectedValue(committedError)
+
+			await cacheManager.clearCacheFile()
+
+			// The empty object is already on disk, so the old hashes must not survive in memory,
+			// and the warning names the exact post-commit error the consumer had to tolerate.
+			expect(cacheManager.getAllHashes()).toEqual({})
+			expect(consoleWarnSpy).toHaveBeenCalledWith(
+				"Cache file content committed but not fully verified:",
+				committedError,
+			)
+			consoleWarnSpy.mockRestore()
+		})
+
 		it("should handle clear errors gracefully", async () => {
 			const consoleErrorSpy = vitest.spyOn(console, "error").mockImplementation(function () {})
 			;(safeWriteJson as Mock).mockRejectedValue(new Error("Save failed"))

src/services/code-index/cache-manager.ts

@@ -3,6 +3,7 @@ import { createHash } from "crypto"
 import { ICacheManager } from "./interfaces/cache"
 import debounce from "lodash.debounce"
 import { safeWriteJson } from "../../utils/safeWriteJson"
+import { isPublishCommittedError } from "../../services/file-safety/safeWriteText"
 import { TelemetryService } from "@roo-code/telemetry"
 import { TelemetryEventName } from "@roo-code/types"
 
@@ -70,6 +71,14 @@ export class CacheManager implements ICacheManager {
 			await safeWriteJson(this.cachePath.fsPath, {})
 			this.fileHashes = {}
 		} catch (error) {
+			if (isPublishCommittedError(error)) {
+				// The empty cache object is already committed on disk: only its durability or the
+				// restored access rights could not be confirmed. Keeping the old in-memory hashes
+				// would make the process disagree with the file it just published.
+				this.fileHashes = {}
+				console.warn("Cache file content committed but not fully verified:", error)
+				return
+			}
 			console.error("Failed to clear cache file:", error, this.cachePath)
 			TelemetryService.instance.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, {
 				error: error instanceof Error ? error.message : String(error),