feat(file-safety): atomic text publish primitive + safeWriteJson refactor (A4, #1375)

src/eslint-suppressions.json

@@ -1686,7 +1686,7 @@
 	},
 	"utils/__tests__/safeWriteJson.test.ts": {
 		"@typescript-eslint/no-explicit-any": {
-			"count": 27
+			"count": 26
 		}
 	},
 	"utils/__tests__/shell.spec.ts": {
@@ -1716,7 +1716,7 @@
 	},
 	"utils/safeWriteJson.ts": {
 		"@typescript-eslint/no-explicit-any": {
-			"count": 4
+			"count": 3
 		}
 	},
 	"utils/tts.ts": {

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,7 @@ import { arePathsEqual, getReadablePath } from "../../utils/path"
 import { formatResponse } from "../../core/prompts/responses"
 import { diagnosticsToProblemsString, getNewDiagnostics } from "../diagnostics"
 import { Task } from "../../core/task/Task"
+import { safeWriteText } from "../../services/file-safety/safeWriteText"
 
 import { DecorationController } from "./DecorationController"
 
@@ -1156,7 +1158,20 @@ export class DiffViewProvider {
 
 		// 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
+		}
+		await safeWriteText(absolutePath, content)
 
 		// 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

@@ -16,8 +16,22 @@ 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),
+	}
+})
+
 // Mock utils
 vi.mock("../../../utils/fs", () => ({
 	createDirectoriesForFile: vi.fn().mockResolvedValue([]),
@@ -27,6 +41,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
@@ -780,6 +796,86 @@ describe("DiffViewProvider", () => {
 	})
 
 	describe("saveDirectly method", () => {
+		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")
+			// 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("surfaces a failed DACL restore as a rejection instead of a completed save", async () => {
+			const { DaclRestoreError, safeWriteText } = await import("../../../services/file-safety/safeWriteText")
+			vi.mocked(safeWriteText).mockClear()
+			// The primitive reports a lost DACL after the commit rename as DaclRestoreError.
+			// The direct-save path must not swallow it: a caller that sees saveDirectly
+			// resolve would report a save whose access rights silently changed.
+			vi.mocked(safeWriteText).mockRejectedValueOnce(new DaclRestoreError(`${mockCwd}/test.ts`, null))
+
+			await expect(
+				diffViewProvider.saveDirectly("test.ts", "new content", true, true, 200),
+			).rejects.toBeInstanceOf(DaclRestoreError)
+
+			// The rejection is the primitive's own - the publish was attempted exactly once,
+			// not retried into a success the caller could observe.
+			const publishCalls = vi.mocked(safeWriteText).mock.calls.filter(function (call) {
+				return call[0] === `${mockCwd}/test.ts`
+			})
+			expect(publishCalls).toHaveLength(1)
+		})
+
 		beforeEach(() => {
 			// Mock vscode functions
 			vi.mocked(vscode.window.showTextDocument).mockResolvedValue({} as any)
@@ -792,9 +888,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")
 
 			// Verify file was opened without focus
 			expect(vscode.window.showTextDocument).toHaveBeenCalledWith(
@@ -815,9 +911,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")
 
 			// Verify file was NOT opened
 			expect(vscode.window.showTextDocument).not.toHaveBeenCalled()
@@ -830,9 +926,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")
 
 			// Verify delay was NOT called
 			expect(mockDelay).not.toHaveBeenCalled()

src/services/file-safety/__tests__/safeWriteText.integration.spec.ts

@@ -0,0 +1,60 @@
+import fsSync from "node:fs"
+import os from "node:os"
+import path from "node:path"
+
+import { safeWriteText } from "../safeWriteText"
+
+/**
+ * Every other suite in this directory mocks fs/promises and fs, so nothing there proves the real
+ * open/write/fsync/rename sequence produces a file on disk. This suite uses a real temporary
+ * directory and no mocks at all: it asserts the outcome (bytes on disk) and the absence of residue
+ * (staging directory, temp file, backup).
+ */
+describe("safeWriteText against a real filesystem", () => {
+	let dir: string
+
+	beforeEach(() => {
+		dir = fsSync.mkdtempSync(path.join(os.tmpdir(), "safe-write-it-"))
+	})
+
+	afterEach(() => {
+		fsSync.rmSync(dir, { recursive: true, force: true })
+	})
+
+	function _leftovers(): string[] {
+		return (
+			fsSync
+				.readdirSync(dir)
+				// The Windows DACL dump is written next to the target and only ever shows up on the
+				// windows-latest run, so a filter that misses it lets a leaked dump pass a suite
+				// whose claim is the absence of residue.
+				.filter(function (name) {
+					return (
+						name.includes(".file-safety-staging") ||
+						name.includes(".new_") ||
+						name.includes(".bak") ||
+						name.includes(".acl.tmp")
+					)
+				})
+		)
+	}
+
+	it("creates a new file with the requested bytes and leaves no residue", async () => {
+		const target = path.join(dir, "brand-new.txt")
+
+		await safeWriteText(target, "first content")
+
+		expect(fsSync.readFileSync(target, "utf8")).toBe("first content")
+		expect(_leftovers()).toEqual([])
+	})
+
+	it("replaces an existing file and leaves no residue", async () => {
+		const target = path.join(dir, "existing.txt")
+		fsSync.writeFileSync(target, "old")
+
+		await safeWriteText(target, "replacement")
+
+		expect(fsSync.readFileSync(target, "utf8")).toBe("replacement")
+		expect(_leftovers()).toEqual([])
+	})
+})