refactor(file-safety): delegate safeWriteJson to safeWriteText atomic publish (S3 v2-2, epic #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/utils/__tests__/safeWriteJson.test.ts

@@ -4,6 +4,7 @@ import * as path from "path"
 import * as os from "os"
 
 import { safeWriteJson } from "../safeWriteJson"
+import { safeWriteText } from "../../services/file-safety/safeWriteText"
 
 // Capture actual implementations before the vi.mock factory runs,
 // so they are never wrapped by vi.fn() — avoids infinite recursion when
@@ -48,6 +49,18 @@ vi.mock("fs", async () => {
 	}
 })
 
+// Spy the atomic-publish boundary: the wrapper delegates to the real
+// implementation, so every delegation still executes for real — the spy only
+// records the exact boundary arguments so the delegation contract (empty
+// content string + staged tempPath + backup) can be pinned.
+vi.mock("../../services/file-safety/safeWriteText", async (importOriginal) => {
+	const actual = await importOriginal<typeof import("../../services/file-safety/safeWriteText")>()
+	return {
+		...actual,
+		safeWriteText: vi.fn(actual.safeWriteText),
+	}
+})
+
 import * as fs from "fs/promises" // This will now be the mocked version
 
 describe("safeWriteJson", () => {
@@ -127,6 +140,44 @@ describe("safeWriteJson", () => {
 		expect(content).toEqual(newData)
 	})
 
+	test("should stage the serialized temp beside the target with a deterministic name", async () => {
+		// Pin the name inputs so the staged path is deterministic: the generated
+		// name must be exactly ".<name>.new_<ts>_<rand>.tmp" inside the target's
+		// own directory (same volume as the commit rename — no EXDEV).
+		vi.spyOn(Date, "now").mockReturnValue(1700000000000)
+		vi.spyOn(Math, "random").mockReturnValue(0.123456789)
+
+		await safeWriteJson(currentTestFilePath, { pinned: "name" })
+
+		// The implementation stages beside the *resolved* target (fs.realpath),
+		// so mirror that resolution when building the expectation: on Windows CI
+		// runners os.tmpdir() is the 8.3 short form (C:\Users\RUNNER~1\...) while
+		// realpath returns the long form (C:\Users\runneradmin\...).
+		const resolvedDir = path.dirname(await fs.realpath(currentTestFilePath))
+		const expectedTemp = path.join(resolvedDir, ".test-file.json.new_1700000000000_4fzzzxjylrx.tmp")
+		expect(vi.mocked(fsSyncActual.createWriteStream)).toHaveBeenCalledWith(expectedTemp, { encoding: "utf8" })
+	})
+
+	test("delegates the staged temp to safeWriteText with an empty content string", async () => {
+		// When tempPath is supplied, the primitive's content argument is not
+		// used — the staged file is the content source. Pin the exact boundary
+		// call so that contract (and the delegation itself) stays covered.
+		const spy = vi.mocked(safeWriteText)
+		spy.mockClear()
+
+		await safeWriteJson(currentTestFilePath, { delegated: true })
+
+		expect(spy).toHaveBeenCalledTimes(1)
+		expect(spy).toHaveBeenCalledWith(
+			path.resolve(currentTestFilePath),
+			"",
+			expect.objectContaining({
+				tempPath: expect.stringContaining(".new_"),
+				backup: true,
+			}),
+		)
+	})
+
 	// Failure Scenarios
 	test("should handle failure when writing to tempNewFilePath", async () => {
 		// currentTestFilePath exists due to beforeEach, allowing lock acquisition.
@@ -312,26 +363,28 @@ describe("safeWriteJson", () => {
 		expect(content).toEqual(newData)
 	})
 
-	// Test for console error suppression during backup deletion
-	test("should suppress console.error when backup deletion fails", async () => {
+	// Backup cleanup is delegated to safeWriteText, which swallows a
+	// non-fatal backup-deletion failure silently (the content is already
+	// committed; an orphaned backup is acceptable) — no console.error here.
+	test("should not log when delegated backup deletion fails", async () => {
 		const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) // Suppress console.error
 		const initialData = { message: "Initial" }
 		const newData = { message: "New" }
 
 		await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData))
 
 		// fs.unlink is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn
-		vi.mocked(fs.unlink).mockImplementation(async (filePath: any) => {
-			if (filePath.toString().includes(".bak_")) {
-				throw new Error("Backup deletion failed")
-			}
-			return fsPromisesActuals.unlink!(filePath)
+		vi.mocked(fs.unlink).mockImplementationOnce(async () => {
+			throw new Error("Backup deletion failed")
 		})
 
 		await safeWriteJson(currentTestFilePath, newData)
 
-		// Verify console.error was called with the expected message
-		expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining("Successfully wrote"), expect.any(Error))
+		// The write succeeds and the new content is in place...
+		const content = await readFileContent(currentTestFilePath)
+		expect(content).toEqual(newData)
+		// ...and the delegated cleanup failure is silent.
+		expect(consoleErrorSpy).not.toHaveBeenCalled()
 
 		consoleErrorSpy.mockRestore()
 		vi.mocked(fs.unlink).mockRestore()
@@ -434,8 +487,11 @@ describe("safeWriteJson", () => {
 		expect(vi.mocked(fs.access)).toHaveBeenCalled()
 	})
 
-	// Test for rollback failure scenario
-	test("should log error and re-throw original if rollback fails", async () => {
+	// Test for rollback failure scenario. The rollback rename happens inside
+	// safeWriteText, which swallows a failed rollback silently so the original
+	// error is never masked — but the original content then only exists under
+	// the (orphaned) backup name.
+	test("should re-throw original error if rollback fails (backup is orphaned)", async () => {
 		const initialData = { message: "Initial, should be lost if rollback fails" }
 		const newData = { message: "New content" }
 
@@ -460,11 +516,12 @@ describe("safeWriteJson", () => {
 		// Should throw the original error, not the rollback error
 		await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow("Primary rename failed")
 
-		// Verify console.error was called for the rollback failure
-		expect(consoleErrorSpy).toHaveBeenCalledWith(
-			expect.stringContaining("Failed to restore backup"),
-			expect.objectContaining({ message: "Rollback rename failed" }),
-		)
+		// The outer handler still logs the original error (loud failure)...
+		expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining("Operation failed for"), expect.any(Error))
+		// ...and the target path is gone: the original file was renamed to the
+		// backup, the commit failed, and the rollback rename failed too, leaving
+		// the backup orphaned at its generated name.
+		await expect(fs.access(currentTestFilePath)).rejects.toThrow()
 
 		consoleErrorSpy.mockRestore()
 	})

src/utils/safeWriteJson.ts

@@ -3,6 +3,7 @@ import * as fsSync from "fs"
 import * as path from "path"
 import * as lockfile from "proper-lockfile"
 import { JsonStreamStringify } from "json-stream-stringify"
+import { resolvePublishTarget, safeWriteText } from "../services/file-safety/safeWriteText"
 
 /**
  * Options for safeWriteJson function
@@ -31,10 +32,11 @@ export interface SafeWriteJsonOptions {
  * Safely writes JSON data to a file.
  * - Creates parent directories if they don't exist
  * - Uses 'proper-lockfile' for inter-process advisory locking to prevent concurrent writes to the same path.
- * - Writes to a temporary file first.
- * - If the target file exists, it's backed up before being replaced.
- * - Attempts to roll back and clean up in case of errors.
- * - Supports pretty-printing with indentation while maintaining streaming efficiency.
+ * - Serializes to a temp file beside the resolved target (streaming, so large
+ *   payloads stay out of memory), then delegates the backup/commit/rollback
+ *   dance to the atomic text publish primitive (safeWriteText) with
+ *   tempPath + backup:true — the same crash-safe, mode-preserving,
+ *   DACL-preserving, symlink-referent semantics as every other file-safety write.
  *
  * @param {string} filePath - The absolute path to the target file.
  * @param {any} data - The data to serialize to JSON and write.
@@ -46,8 +48,10 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso
 	const absoluteFilePath = path.resolve(filePath)
 	let releaseLock = async () => {} // Initialized to a no-op
 
-	// For directory creation
-	const dirPath = path.dirname(absoluteFilePath)
+	// Resolve the symlink referent so the staged temp file is created beside
+	// the file that will actually be replaced (a rename across volumes fails EXDEV).
+	const targetPath = await resolvePublishTarget(absoluteFilePath)
+	const dirPath = path.dirname(targetPath)
 
 	// Ensure directory structure exists with improved reliability
 	try {
@@ -88,10 +92,6 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso
 		throw lockError
 	}
 
-	// Variables to hold the actual paths of temp files if they are created.
-	let actualTempNewFilePath: string | null = null
-	let actualTempBackupFilePath: string | null = null
-
 	try {
 		// If a merge callback was provided, read the current file under the lock
 		// and let the caller merge before we write. Must be inside try/finally
@@ -110,101 +110,32 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso
 			data = options.merge(existing, data)
 		}
 
-		// Step 1: Write data to a new temporary file.
-		actualTempNewFilePath = path.join(
-			path.dirname(absoluteFilePath),
-			`.${path.basename(absoluteFilePath)}.new_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`,
+		// Stage the serialized JSON in a temp file beside the resolved target.
+		// Streaming keeps large payloads out of memory; the safeWriteText commit
+		// is a rename on the same volume, so no EXDEV.
+		const tempPath = path.join(
+			dirPath,
+			`.${path.basename(targetPath)}.new_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`,
 		)
 
-		await _streamDataToFile(actualTempNewFilePath, data, options?.prettyPrint)
-
-		// Step 2: Check if the target file exists. If so, rename it to a backup path.
 		try {
-			// Check for target file existence
-			await fs.access(absoluteFilePath)
-			// Target exists, create a backup path and rename.
-			actualTempBackupFilePath = path.join(
-				path.dirname(absoluteFilePath),
-				`.${path.basename(absoluteFilePath)}.bak_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`,
-			)
-			await fs.rename(absoluteFilePath, actualTempBackupFilePath)
-		} catch (accessError: any) {
-			// Explicitly type accessError
-			if (accessError.code !== "ENOENT") {
-				// An error other than "file not found" occurred during access check.
-				throw accessError
-			}
-			// Target file does not exist, so no backup is made. actualTempBackupFilePath remains null.
+			await _streamDataToFile(tempPath, data, options?.prettyPrint)
+		} catch (streamError) {
+			// A failed stream can leave a partial (or empty) temp file behind —
+			// unlink it before propagating so no orphan staging file remains.
+			await fs.unlink(tempPath).catch(() => {})
+			throw streamError
 		}
 
-		// Step 3: Rename the new temporary file to the target file path.
-		// This is the main "commit" step.
-		await fs.rename(actualTempNewFilePath, absoluteFilePath)
-
-		// If we reach here, the new file is successfully in place.
-		// The original actualTempNewFilePath is now the main file, so we shouldn't try to clean it up as "temp".
-		// Mark as "used" or "committed"
-		actualTempNewFilePath = null
-
-		// Step 4: If a backup was created, attempt to delete it.
-		if (actualTempBackupFilePath) {
-			try {
-				await fs.unlink(actualTempBackupFilePath)
-				// Mark backup as handled
-				actualTempBackupFilePath = null
-			} catch (unlinkBackupError) {
-				// Log this error, but do not re-throw. The main operation was successful.
-				// actualTempBackupFilePath remains set, indicating an orphaned backup.
-				console.error(
-					`Successfully wrote ${absoluteFilePath}, but failed to clean up backup ${actualTempBackupFilePath}:`,
-					unlinkBackupError,
-				)
-			}
-		}
+		// Delegate the backup/commit/rollback to the atomic text publish
+		// primitive: it applies the target's mode to the staged temp, fsyncs,
+		// backs up the existing target (backup:true), atomically renames
+		// temp -> target, and rolls the backup back on failure. The content
+		// argument is not used when tempPath is supplied — the staged file is
+		// the content source.
+		await safeWriteText(absoluteFilePath, "", { tempPath, backup: true })
 	} catch (originalError) {
 		console.error(`Operation failed for ${absoluteFilePath}: [Original Error Caught]`, originalError)
-
-		const newFileToCleanupWithinCatch = actualTempNewFilePath
-		const backupFileToRollbackOrCleanupWithinCatch = actualTempBackupFilePath
-
-		// Attempt rollback if a backup was made
-		if (backupFileToRollbackOrCleanupWithinCatch) {
-			try {
-				await fs.rename(backupFileToRollbackOrCleanupWithinCatch, absoluteFilePath)
-				// Mark as handled, prevent later unlink of this path
-				actualTempBackupFilePath = null
-			} catch (rollbackError) {
-				// actualTempBackupFilePath (outer scope) remains pointing to backupFileToRollbackOrCleanupWithinCatch
-				console.error(
-					`[Catch] Failed to restore backup ${backupFileToRollbackOrCleanupWithinCatch} to ${absoluteFilePath}:`,
-					rollbackError,
-				)
-			}
-		}
-
-		// Cleanup the .new file if it exists
-		if (newFileToCleanupWithinCatch) {
-			try {
-				await fs.unlink(newFileToCleanupWithinCatch)
-			} catch (cleanupError) {
-				console.error(
-					`[Catch] Failed to clean up temporary new file ${newFileToCleanupWithinCatch}:`,
-					cleanupError,
-				)
-			}
-		}
-
-		// Cleanup the .bak file if it still needs to be (i.e., wasn't successfully restored)
-		if (actualTempBackupFilePath) {
-			try {
-				await fs.unlink(actualTempBackupFilePath)
-			} catch (cleanupError) {
-				console.error(
-					`[Catch] Failed to clean up temporary backup file ${actualTempBackupFilePath}:`,
-					cleanupError,
-				)
-			}
-		}
 		throw originalError // This MUST be the error that rejects the promise.
 	} finally {
 		// Release the lock in the main finally block.