2
0
Эх сурвалжийг харах

Revert "fix: use safeWriteJson for all JSON file writes" (#4471)

Revert "fix: use safeWriteJson for all JSON file writes (#3772)"

This reverts commit 1be30fc8c2a274467227fc7d695b323510160fcb.
Matt Rubens 6 сар өмнө
parent
commit
8d2eeda4ad

+ 0 - 5
.roo/rules-code/use-safeWriteJson.md

@@ -1,5 +0,0 @@
-# JSON File Writing Must Be Atomic
-
-- MUST ALWAYS use `safeWriteJson(filePath: string, data: any): Promise<void>` from `src/utils/safeWriteJson.ts` instead of `JSON.stringify` with file-write operations
-- `safeWriteJson` prevents data corruption via atomic writes with locking and streams the write to minimize memory footprint
-- Test files are exempt from this rule

+ 0 - 66
pnpm-lock.yaml

@@ -702,9 +702,6 @@ importers:
       pretty-bytes:
         specifier: ^6.1.1
         version: 6.1.1
-      proper-lockfile:
-        specifier: ^4.1.2
-        version: 4.1.2
       ps-tree:
         specifier: ^1.2.0
         version: 1.2.0
@@ -732,9 +729,6 @@ importers:
       sound-play:
         specifier: ^1.1.0
         version: 1.1.0
-      stream-json:
-        specifier: ^1.8.0
-        version: 1.9.1
       string-similarity:
         specifier: ^4.0.4
         version: 4.0.4
@@ -817,15 +811,9 @@ importers:
       '@types/node-ipc':
         specifier: ^9.2.3
         version: 9.2.3
-      '@types/proper-lockfile':
-        specifier: ^4.1.4
-        version: 4.1.4
       '@types/ps-tree':
         specifier: ^1.1.6
         version: 1.1.6
-      '@types/stream-json':
-        specifier: ^1.7.8
-        version: 1.7.8
       '@types/string-similarity':
         specifier: ^4.0.2
         version: 4.0.2
@@ -4466,9 +4454,6 @@ packages:
   '@types/[email protected]':
     resolution: {integrity: sha512-gNMvNH49DJ7OJYv+KAKn0Xp45p8PLl6zo2YnvDIbTd4J6MER2BmWN49TG7n9LvkyihINxeKW8+3bfS2yDC9dzQ==}
 
-  '@types/[email protected]':
-    resolution: {integrity: sha512-uo2ABllncSqg9F1D4nugVl9v93RmjxF6LJzQLMLDdPaXCUIDPeOJ21Gbqi43xNKzBi/WQ0Q0dICqufzQbMjipQ==}
-
   '@types/[email protected]':
     resolution: {integrity: sha512-PtrlVaOaI44/3pl3cvnlK+GxOM3re2526TJvPvh7W+keHIXdV4TE0ylpPBAcvFQCbGitaTXwL9u+RF7qtVeazQ==}
 
@@ -4483,21 +4468,12 @@ packages:
   '@types/[email protected]':
     resolution: {integrity: sha512-A4STmOXPhMUtHH+S6ymgE2GiBSMqf4oTvcQZMcHzokuTLVYzXTB8ttjcgxOVaAp2lGwEdzZ0J+cRbbeevQj1UQ==}
 
-  '@types/[email protected]':
-    resolution: {integrity: sha512-3xSjTp3v03X/lSQLkczaN9UIEwJMoMCA1+Nb5HfbJEQWogdeQIyVtTvxPXDQjZ5zws8rFQfVfRdz03ARihPJgw==}
-
   '@types/[email protected]':
     resolution: {integrity: sha512-+UE8GAGRPbJVQDdxi16dgadcBfQ+KG2vgZhV1+3A1XmHbmwcdwhCUwIdy+d3pAGrbvgRoVSjeI9vOWyq376Yzw==}
 
   '@types/[email protected]':
     resolution: {integrity: sha512-9aEbYZ3TbYMznPdcdr3SmIrLXwC/AKZXQeCf9Pgao5CKb8CyHuEX5jzWPTkvregvhRJHcpRO6BFoGW9ycaOkYw==}
 
-  '@types/[email protected]':
-    resolution: {integrity: sha512-guDyAl6s/CAzXUOWpGK2bHvdiopLIwpGu8v10+lb9hnQOyo4oj/ZUQFOvqFjKGsE3wJP1fpIesCcMvbXuWsqOg==}
-
-  '@types/[email protected]':
-    resolution: {integrity: sha512-MU1OB1eFLcYWd1LjwKXrxdoPtXSRzRmAnnxs4Js/ayB5O/NvHraWwuOaqMWIebpYwM6khFlsJOHEhI9xK/ab4Q==}
-
   '@types/[email protected]':
     resolution: {integrity: sha512-LkJQ/jsXtCVMK+sKYAmX/8zEq+/46f1PTQw7YtmQwb74jemS1SlNLmARM2Zml9DgdDTWKAtc5L13WorpHPDjDA==}
 
@@ -8961,9 +8937,6 @@ packages:
     resolution: {integrity: sha512-vGrhOavPSTz4QVNuBNdcNXePNdNMaO1xj9yBeH1ScQPjk/rhg9sSlCXPhMkFuaNNW/syTvYqsnbIJxMBfRbbag==}
     engines: {node: '>= 8'}
 
-  [email protected]:
-    resolution: {integrity: sha512-TjNPblN4BwAWMXU8s9AEz4JmQxnD1NNL7bNOY/AKUzyamc379FWASUhc/K1pL2noVb+XmZKLL68cjzLsiOAMaA==}
-
   [email protected]:
     resolution: {integrity: sha512-YUHSPk+A30YPv+0Qf8i9Mbfe/C0hdPXk1s1jPVToV8pk8BQtpw10ct89Eo7OWkutrwqvT0eicAxlOg3dOAu8JA==}
 
@@ -9319,10 +9292,6 @@ packages:
     resolution: {integrity: sha512-oMA2dcrw6u0YfxJQXm342bFKX/E4sG9rbTzO9ptUcR/e8A33cHuvStiYOwH7fszkZlZ1z/ta9AAoPk2F4qIOHA==}
     engines: {node: '>=18'}
 
-  [email protected]:
-    resolution: {integrity: sha512-9LkiTwjUh6rT555DtE9rTX+BKByPfrMzEAtnlEtdEwr3Nkffwiihqe2bWADg+OQRjt9gl6ICdmB/ZFDCGAtSow==}
-    engines: {node: '>= 4'}
-
   [email protected]:
     resolution: {integrity: sha512-g6QUff04oZpHs0eG5p83rFLhHeV00ug/Yf9nZM6fLeUrPguBTkTQOdpAWWspMh55TZfVQDPaN3NQJfbVRAxdIw==}
     engines: {iojs: '>=1.0.0', node: '>=0.10.0'}
@@ -9656,15 +9625,9 @@ packages:
       prettier:
         optional: true
 
-  [email protected]:
-    resolution: {integrity: sha512-1TJmBx6aSWqZ4tx7aTpBDXK0/e2hhcNSTV8+CbFJtDjbb+I1mZ8lHit0Grw9GRT+6JbIrrDd8esncgBi8aBXGA==}
-
   [email protected]:
     resolution: {integrity: sha512-rT00SPnTVyRsaSz5zgSPma/aHSOic5U1prhYdRy5HS2kTZviFpmDgzilbtsJsxiroqACmayynDN/9VzIbX5DOw==}
 
-  [email protected]:
-    resolution: {integrity: sha512-uWkjJ+2Nt/LO9Z/JyKZbMusL8Dkh97uUBTv3AJQ74y07lVahLY4eEFsPsE97pxYBwr8nnjMAIch5eqI0gPShyw==}
-
   [email protected]:
     resolution: {integrity: sha512-Mcc5wHehp9aXz1ax6bZUyY5afg9u2rv5cqQI3mRrYkGC8rW2hM02jWuwjtL++LS5qinSyhj2QfLyNsuc+VsExg==}
     engines: {node: '>=10.0.0'}
@@ -14760,10 +14723,6 @@ snapshots:
 
   '@types/[email protected]': {}
 
-  '@types/[email protected]':
-    dependencies:
-      '@types/retry': 0.12.5
-
   '@types/[email protected]': {}
 
   '@types/[email protected](@types/[email protected])':
@@ -14777,21 +14736,10 @@ snapshots:
 
   '@types/[email protected]': {}
 
-  '@types/[email protected]': {}
-
   '@types/[email protected]': {}
 
   '@types/[email protected]': {}
 
-  '@types/[email protected]':
-    dependencies:
-      '@types/node': 22.15.20
-
-  '@types/[email protected]':
-    dependencies:
-      '@types/node': 22.15.20
-      '@types/stream-chain': 2.1.0
-
   '@types/[email protected]': {}
 
   '@types/[email protected]': {}
@@ -20105,12 +20053,6 @@ snapshots:
 
   [email protected]: {}
 
-  [email protected]:
-    dependencies:
-      graceful-fs: 4.2.11
-      retry: 0.12.0
-      signal-exit: 3.0.7
-
   [email protected]:
     dependencies:
       xtend: 4.0.2
@@ -20590,8 +20532,6 @@ snapshots:
       onetime: 7.0.0
       signal-exit: 4.1.0
 
-  [email protected]: {}
-
   [email protected]: {}
 
   [email protected]: {}
@@ -21015,16 +20955,10 @@ snapshots:
       - supports-color
       - utf-8-validate
 
-  [email protected]: {}
-
   [email protected]:
     dependencies:
       duplexer: 0.1.2
 
-  [email protected]:
-    dependencies:
-      stream-chain: 2.2.5
-
   [email protected]: {}
 
   [email protected]:

+ 1 - 2
src/api/providers/fetchers/modelCache.ts

@@ -2,7 +2,6 @@ import * as path from "path"
 import fs from "fs/promises"
 
 import NodeCache from "node-cache"
-import { safeWriteJson } from "../../../utils/safeWriteJson"
 
 import { ContextProxy } from "../../../core/config/ContextProxy"
 import { getCacheDirectoryPath } from "../../../utils/storage"
@@ -20,7 +19,7 @@ const memoryCache = new NodeCache({ stdTTL: 5 * 60, checkperiod: 5 * 60 })
 async function writeModels(router: RouterName, data: ModelRecord) {
 	const filename = `${router}_models.json`
 	const cacheDir = await getCacheDirectoryPath(ContextProxy.instance.globalStorageUri.fsPath)
-	await safeWriteJson(path.join(cacheDir, filename), data)
+	await fs.writeFile(path.join(cacheDir, filename), JSON.stringify(data))
 }
 
 async function readModels(router: RouterName): Promise<ModelRecord | undefined> {

+ 1 - 2
src/api/providers/fetchers/modelEndpointCache.ts

@@ -2,7 +2,6 @@ import * as path from "path"
 import fs from "fs/promises"
 
 import NodeCache from "node-cache"
-import { safeWriteJson } from "../../../utils/safeWriteJson"
 import sanitize from "sanitize-filename"
 
 import { ContextProxy } from "../../../core/config/ContextProxy"
@@ -19,7 +18,7 @@ const getCacheKey = (router: RouterName, modelId: string) => sanitize(`${router}
 async function writeModelEndpoints(key: string, data: ModelRecord) {
 	const filename = `${key}_endpoints.json`
 	const cacheDir = await getCacheDirectoryPath(ContextProxy.instance.globalStorageUri.fsPath)
-	await safeWriteJson(path.join(cacheDir, filename), data)
+	await fs.writeFile(path.join(cacheDir, filename), JSON.stringify(data, null, 2))
 }
 
 async function readModelEndpoints(key: string): Promise<ModelRecord | undefined> {

+ 12 - 16
src/core/config/__tests__/importExport.test.ts

@@ -12,7 +12,6 @@ import { importSettings, exportSettings } from "../importExport"
 import { ProviderSettingsManager } from "../ProviderSettingsManager"
 import { ContextProxy } from "../ContextProxy"
 import { CustomModesManager } from "../CustomModesManager"
-import { safeWriteJson } from "../../../utils/safeWriteJson"
 
 jest.mock("vscode", () => ({
 	window: {
@@ -34,8 +33,6 @@ jest.mock("os", () => ({
 	homedir: jest.fn(() => "/mock/home"),
 }))
 
-jest.mock("../../../utils/safeWriteJson")
-
 describe("importExport", () => {
 	let mockProviderSettingsManager: jest.Mocked<ProviderSettingsManager>
 	let mockContextProxy: jest.Mocked<ContextProxy>
@@ -375,10 +372,11 @@ describe("importExport", () => {
 			expect(mockContextProxy.export).toHaveBeenCalled()
 			expect(fs.mkdir).toHaveBeenCalledWith("/mock/path", { recursive: true })
 
-			expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/roo-code-settings.json", {
-				providerProfiles: mockProviderProfiles,
-				globalSettings: mockGlobalSettings,
-			})
+			expect(fs.writeFile).toHaveBeenCalledWith(
+				"/mock/path/roo-code-settings.json",
+				JSON.stringify({ providerProfiles: mockProviderProfiles, globalSettings: mockGlobalSettings }, null, 2),
+				"utf-8",
+			)
 		})
 
 		it("should include globalSettings when allowedMaxRequests is null", async () => {
@@ -407,10 +405,11 @@ describe("importExport", () => {
 				contextProxy: mockContextProxy,
 			})
 
-			expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/roo-code-settings.json", {
-				providerProfiles: mockProviderProfiles,
-				globalSettings: mockGlobalSettings,
-			})
+			expect(fs.writeFile).toHaveBeenCalledWith(
+				"/mock/path/roo-code-settings.json",
+				JSON.stringify({ providerProfiles: mockProviderProfiles, globalSettings: mockGlobalSettings }, null, 2),
+				"utf-8",
+			)
 		})
 
 		it("should handle errors during the export process", async () => {
@@ -425,8 +424,7 @@ describe("importExport", () => {
 			})
 
 			mockContextProxy.export.mockResolvedValue({ mode: "code" })
-			// Simulate an error during the safeWriteJson operation
-			;(safeWriteJson as jest.Mock).mockRejectedValueOnce(new Error("Safe write error"))
+			;(fs.writeFile as jest.Mock).mockRejectedValue(new Error("Write error"))
 
 			await exportSettings({
 				providerSettingsManager: mockProviderSettingsManager,
@@ -437,10 +435,8 @@ 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
+			expect(fs.writeFile).toHaveBeenCalled()
 			// 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("should handle errors during directory creation", async () => {

+ 1 - 2
src/core/config/importExport.ts

@@ -1,4 +1,3 @@
-import { safeWriteJson } from "../../utils/safeWriteJson"
 import os from "os"
 import * as path from "path"
 import fs from "fs/promises"
@@ -117,6 +116,6 @@ 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 fs.writeFile(uri.fsPath, JSON.stringify({ providerProfiles, globalSettings }, null, 2), "utf-8")
 	} catch (e) {}
 }

+ 1 - 2
src/core/context-tracking/FileContextTracker.ts

@@ -1,4 +1,3 @@
-import { safeWriteJson } from "../../utils/safeWriteJson"
 import * as path from "path"
 import * as vscode from "vscode"
 import { getTaskDirectoryPath } from "../../utils/storage"
@@ -131,7 +130,7 @@ export class FileContextTracker {
 			const globalStoragePath = this.getContextProxy()!.globalStorageUri.fsPath
 			const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId)
 			const filePath = path.join(taskDir, GlobalFileNames.taskMetadata)
-			await safeWriteJson(filePath, metadata)
+			await fs.writeFile(filePath, JSON.stringify(metadata, null, 2))
 		} catch (error) {
 			console.error("Failed to save task metadata:", error)
 		}

+ 1 - 2
src/core/task-persistence/apiMessages.ts

@@ -1,4 +1,3 @@
-import { safeWriteJson } from "../../utils/safeWriteJson"
 import * as path from "path"
 import * as fs from "fs/promises"
 
@@ -47,5 +46,5 @@ export async function saveApiMessages({
 }) {
 	const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId)
 	const filePath = path.join(taskDir, GlobalFileNames.apiConversationHistory)
-	await safeWriteJson(filePath, messages)
+	await fs.writeFile(filePath, JSON.stringify(messages))
 }

+ 1 - 2
src/core/task-persistence/taskMessages.ts

@@ -1,4 +1,3 @@
-import { safeWriteJson } from "../../utils/safeWriteJson"
 import * as path from "path"
 import * as fs from "fs/promises"
 
@@ -38,5 +37,5 @@ export type SaveTaskMessagesOptions = {
 export async function saveTaskMessages({ messages, taskId, globalStoragePath }: SaveTaskMessagesOptions) {
 	const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId)
 	const filePath = path.join(taskDir, GlobalFileNames.uiMessages)
-	await safeWriteJson(filePath, messages)
+	await fs.writeFile(filePath, JSON.stringify(messages))
 }

+ 4 - 4
src/core/webview/__tests__/ClineProvider.test.ts

@@ -13,7 +13,6 @@ import { experimentDefault } from "../../../shared/experiments"
 import { setTtsEnabled } from "../../../utils/tts"
 import { ContextProxy } from "../../config/ContextProxy"
 import { Task, TaskOptions } from "../../task/Task"
-import { safeWriteJson } from "../../../utils/safeWriteJson"
 
 import { ClineProvider } from "../ClineProvider"
 
@@ -42,8 +41,6 @@ jest.mock("axios", () => ({
 	post: jest.fn(),
 }))
 
-jest.mock("../../../utils/safeWriteJson")
-
 jest.mock(
 	"@modelcontextprotocol/sdk/types.js",
 	() => ({
@@ -2004,7 +2001,10 @@ describe("Project MCP Settings", () => {
 		)
 
 		// Verify file was created with default content
-		expect(safeWriteJson).toHaveBeenCalledWith(expect.stringContaining("mcp.json"), { mcpServers: {} })
+		expect(fs.writeFile).toHaveBeenCalledWith(
+			expect.stringContaining("mcp.json"),
+			JSON.stringify({ mcpServers: {} }, null, 2),
+		)
 	})
 
 	test("handles openProjectMcpSettings when workspace is not open", async () => {

+ 1 - 2
src/core/webview/webviewMessageHandler.ts

@@ -1,4 +1,3 @@
-import { safeWriteJson } from "../../utils/safeWriteJson"
 import * as path from "path"
 import fs from "fs/promises"
 import pWaitFor from "p-wait-for"
@@ -500,7 +499,7 @@ export const webviewMessageHandler = async (provider: ClineProvider, message: We
 				const exists = await fileExistsAtPath(mcpPath)
 
 				if (!exists) {
-					await safeWriteJson(mcpPath, { mcpServers: {} })
+					await fs.writeFile(mcpPath, JSON.stringify({ mcpServers: {} }, null, 2))
 				}
 
 				await openFile(mcpPath)

+ 1 - 5
src/package.json

@@ -361,11 +361,11 @@
 		"@google/genai": "^0.13.0",
 		"@mistralai/mistralai": "^1.3.6",
 		"@modelcontextprotocol/sdk": "^1.9.0",
-		"@qdrant/js-client-rest": "^1.14.0",
 		"@roo-code/cloud": "workspace:^",
 		"@roo-code/ipc": "workspace:^",
 		"@roo-code/telemetry": "workspace:^",
 		"@roo-code/types": "workspace:^",
+		"@qdrant/js-client-rest": "^1.14.0",
 		"@types/lodash.debounce": "^4.0.9",
 		"@vscode/codicons": "^0.0.36",
 		"async-mutex": "^0.5.0",
@@ -398,7 +398,6 @@
 		"pdf-parse": "^1.1.1",
 		"pkce-challenge": "^4.1.0",
 		"pretty-bytes": "^6.1.1",
-		"proper-lockfile": "^4.1.2",
 		"ps-tree": "^1.2.0",
 		"puppeteer-chromium-resolver": "^23.0.0",
 		"puppeteer-core": "^23.4.0",
@@ -408,7 +407,6 @@
 		"serialize-error": "^11.0.3",
 		"simple-git": "^3.27.0",
 		"sound-play": "^1.1.0",
-		"stream-json": "^1.8.0",
 		"string-similarity": "^4.0.4",
 		"strip-ansi": "^7.1.0",
 		"strip-bom": "^5.0.0",
@@ -438,9 +436,7 @@
 		"@types/node": "20.x",
 		"@types/node-cache": "^4.1.3",
 		"@types/node-ipc": "^9.2.3",
-		"@types/proper-lockfile": "^4.1.4",
 		"@types/ps-tree": "^1.1.6",
-		"@types/stream-json": "^1.7.8",
 		"@types/string-similarity": "^4.0.2",
 		"@types/tmp": "^0.2.6",
 		"@types/turndown": "^5.0.5",

+ 15 - 16
src/services/code-index/__tests__/cache-manager.test.ts

@@ -2,7 +2,6 @@ import * as vscode from "vscode"
 import { createHash } from "crypto"
 import debounce from "lodash.debounce"
 import { CacheManager } from "../cache-manager"
-import { safeWriteJson } from "../../../utils/safeWriteJson"
 
 // Mock vscode
 jest.mock("vscode", () => ({
@@ -12,16 +11,12 @@ jest.mock("vscode", () => ({
 	workspace: {
 		fs: {
 			readFile: jest.fn(),
+			writeFile: jest.fn(),
 			delete: jest.fn(),
 		},
 	},
 }))
 
-// Mock safeWriteJson
-jest.mock("../../../utils/safeWriteJson", () => ({
-	safeWriteJson: jest.fn(),
-}))
-
 // Mock debounce to execute immediately
 jest.mock("lodash.debounce", () => jest.fn((fn) => fn))
 
@@ -93,7 +88,7 @@ describe("CacheManager", () => {
 			cacheManager.updateHash(filePath, hash)
 
 			expect(cacheManager.getHash(filePath)).toBe(hash)
-			expect(safeWriteJson).toHaveBeenCalled()
+			expect(vscode.workspace.fs.writeFile).toHaveBeenCalled()
 		})
 
 		it("should delete hash and trigger save", () => {
@@ -104,7 +99,7 @@ describe("CacheManager", () => {
 			cacheManager.deleteHash(filePath)
 
 			expect(cacheManager.getHash(filePath)).toBeUndefined()
-			expect(safeWriteJson).toHaveBeenCalled()
+			expect(vscode.workspace.fs.writeFile).toHaveBeenCalled()
 		})
 
 		it("should return shallow copy of hashes", () => {
@@ -129,14 +124,18 @@ describe("CacheManager", () => {
 
 			cacheManager.updateHash(filePath, hash)
 
-			expect(safeWriteJson).toHaveBeenCalledWith(mockCachePath.fsPath, { [filePath]: hash })
+			expect(vscode.workspace.fs.writeFile).toHaveBeenCalledWith(mockCachePath, expect.any(Uint8Array))
 
-			// No need to parse the data since safeWriteJson receives the object directly
+			// Verify the saved data
+			const savedData = JSON.parse(
+				Buffer.from((vscode.workspace.fs.writeFile as jest.Mock).mock.calls[0][1]).toString(),
+			)
+			expect(savedData).toEqual({ [filePath]: hash })
 		})
 
 		it("should handle save errors gracefully", async () => {
 			const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation()
-			;(safeWriteJson as jest.Mock).mockRejectedValue(new Error("Save failed"))
+			;(vscode.workspace.fs.writeFile as jest.Mock).mockRejectedValue(new Error("Save failed"))
 
 			cacheManager.updateHash("test.ts", "hash")
 
@@ -153,19 +152,19 @@ describe("CacheManager", () => {
 		it("should clear cache file and reset state", async () => {
 			cacheManager.updateHash("test.ts", "hash")
 
-			// Reset the mock to ensure safeWriteJson succeeds for clearCacheFile
-			;(safeWriteJson as jest.Mock).mockClear()
-			;(safeWriteJson as jest.Mock).mockResolvedValue(undefined)
+			// Reset the mock to ensure writeFile succeeds for clearCacheFile
+			;(vscode.workspace.fs.writeFile as jest.Mock).mockClear()
+			;(vscode.workspace.fs.writeFile as jest.Mock).mockResolvedValue(undefined)
 
 			await cacheManager.clearCacheFile()
 
-			expect(safeWriteJson).toHaveBeenCalledWith(mockCachePath.fsPath, {})
+			expect(vscode.workspace.fs.writeFile).toHaveBeenCalledWith(mockCachePath, Buffer.from("{}"))
 			expect(cacheManager.getAllHashes()).toEqual({})
 		})
 
 		it("should handle clear errors gracefully", async () => {
 			const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation()
-			;(safeWriteJson as jest.Mock).mockRejectedValue(new Error("Save failed"))
+			;(vscode.workspace.fs.writeFile as jest.Mock).mockRejectedValue(new Error("Save failed"))
 
 			await cacheManager.clearCacheFile()
 

+ 2 - 3
src/services/code-index/cache-manager.ts

@@ -2,7 +2,6 @@ import * as vscode from "vscode"
 import { createHash } from "crypto"
 import { ICacheManager } from "./interfaces/cache"
 import debounce from "lodash.debounce"
-import { safeWriteJson } from "../../utils/safeWriteJson"
 
 /**
  * Manages the cache for code indexing
@@ -47,7 +46,7 @@ export class CacheManager implements ICacheManager {
 	 */
 	private async _performSave(): Promise<void> {
 		try {
-			await safeWriteJson(this.cachePath.fsPath, this.fileHashes)
+			await vscode.workspace.fs.writeFile(this.cachePath, Buffer.from(JSON.stringify(this.fileHashes, null, 2)))
 		} catch (error) {
 			console.error("Failed to save cache:", error)
 		}
@@ -58,7 +57,7 @@ export class CacheManager implements ICacheManager {
 	 */
 	async clearCacheFile(): Promise<void> {
 		try {
-			await safeWriteJson(this.cachePath.fsPath, {})
+			await vscode.workspace.fs.writeFile(this.cachePath, Buffer.from("{}"))
 			this.fileHashes = {}
 		} catch (error) {
 			console.error("Failed to clear cache file:", error, this.cachePath)

+ 3 - 4
src/services/mcp/McpHub.ts

@@ -1,4 +1,3 @@
-import { safeWriteJson } from "../../utils/safeWriteJson"
 import { Client } from "@modelcontextprotocol/sdk/client/index.js"
 import { StdioClientTransport, getDefaultEnvironment } from "@modelcontextprotocol/sdk/client/stdio.js"
 import { SSEClientTransport } from "@modelcontextprotocol/sdk/client/sse.js"
@@ -1341,7 +1340,7 @@ export class McpHub {
 			mcpServers: config.mcpServers,
 		}
 
-		await safeWriteJson(configPath, updatedConfig)
+		await fs.writeFile(configPath, JSON.stringify(updatedConfig, null, 2))
 	}
 
 	public async updateServerTimeout(
@@ -1419,7 +1418,7 @@ export class McpHub {
 					mcpServers: config.mcpServers,
 				}
 
-				await safeWriteJson(configPath, updatedConfig)
+				await fs.writeFile(configPath, JSON.stringify(updatedConfig, null, 2))
 
 				// Update server connections with the correct source
 				await this.updateServerConnections(config.mcpServers, serverSource)
@@ -1561,7 +1560,7 @@ export class McpHub {
 			}
 
 			// Write updated config back to file
-			await safeWriteJson(normalizedPath, config)
+			await fs.writeFile(normalizedPath, JSON.stringify(config, null, 2))
 
 			// Update the tools list to reflect the change
 			if (connection) {

+ 1 - 31
src/services/mcp/__tests__/McpHub.test.ts

@@ -3,35 +3,8 @@ import type { ClineProvider } from "../../../core/webview/ClineProvider"
 import type { ExtensionContext, Uri } from "vscode"
 import { ServerConfigSchema } from "../McpHub"
 
-const { McpHub } = require("../McpHub")
-const path = require("path")
-
-// Mock fs/promises before importing anything that uses it
-jest.mock("fs/promises", () => ({
-	access: jest.fn().mockResolvedValue(undefined),
-	writeFile: jest.fn().mockResolvedValue(undefined),
-	readFile: jest.fn().mockResolvedValue("{}"),
-	unlink: jest.fn().mockResolvedValue(undefined),
-	rename: jest.fn().mockResolvedValue(undefined),
-	lstat: jest.fn().mockImplementation(() =>
-		Promise.resolve({
-			isDirectory: () => true,
-		}),
-	),
-	mkdir: jest.fn().mockResolvedValue(undefined),
-}))
-
-// Import the mocked fs
 const fs = require("fs/promises")
-
-// Mock safeWriteJson
-jest.mock("../../../utils/safeWriteJson", () => ({
-	safeWriteJson: jest.fn(async (filePath, data) => {
-		// Instead of trying to write to the file system, just call fs.writeFile mock
-		// This avoids the complex file locking and temp file operations
-		return fs.writeFile(filePath, JSON.stringify(data), "utf8")
-	}),
-}))
+const { McpHub } = require("../McpHub")
 
 jest.mock("vscode", () => ({
 	workspace: {
@@ -73,9 +46,6 @@ describe("McpHub", () => {
 		// Mock console.error to suppress error messages during tests
 		console.error = jest.fn()
 
-		// Reset the mock implementations before each test
-		jest.clearAllMocks()
-
 		const mockUri: Uri = {
 			scheme: "file",
 			authority: "",

+ 0 - 507
src/utils/__tests__/safeWriteJson.test.ts

@@ -1,507 +0,0 @@
-const actualFsPromises = jest.requireActual("fs/promises")
-const originalFsPromisesRename = actualFsPromises.rename
-const originalFsPromisesUnlink = actualFsPromises.unlink
-const originalFsPromisesWriteFile = actualFsPromises.writeFile
-const _originalFsPromisesAccess = actualFsPromises.access
-
-jest.mock("fs/promises", () => {
-	const actual = jest.requireActual("fs/promises")
-	// Start with all actual implementations.
-	const mockedFs = { ...actual }
-
-	// Selectively wrap functions with jest.fn() if they are spied on
-	// or have their implementations changed in tests.
-	// This ensures that other fs.promises functions used by the SUT
-	// (like proper-lockfile's internals) will use their actual implementations.
-	mockedFs.writeFile = jest.fn(actual.writeFile)
-	mockedFs.readFile = jest.fn(actual.readFile)
-	mockedFs.rename = jest.fn(actual.rename)
-	mockedFs.unlink = jest.fn(actual.unlink)
-	mockedFs.access = jest.fn(actual.access)
-	mockedFs.mkdtemp = jest.fn(actual.mkdtemp)
-	mockedFs.rm = jest.fn(actual.rm)
-	mockedFs.readdir = jest.fn(actual.readdir)
-	// fs.stat and fs.lstat will be available via { ...actual }
-
-	return mockedFs
-})
-
-// Mock the 'fs' module for fsSync.createWriteStream
-jest.mock("fs", () => {
-	const actualFs = jest.requireActual("fs")
-	return {
-		...actualFs, // Spread actual implementations
-		createWriteStream: jest.fn((...args: any[]) => actualFs.createWriteStream(...args)), // Default to actual, but mockable
-	}
-})
-
-import * as fs from "fs/promises" // This will now be the mocked version
-import * as fsSyncActual from "fs" // This will now import the mocked 'fs'
-import * as path from "path"
-import * as os from "os"
-// import * as lockfile from 'proper-lockfile' // No longer directly used in tests
-import { safeWriteJson } from "../safeWriteJson"
-import { Writable } from "stream" // For typing mock stream
-
-describe("safeWriteJson", () => {
-	jest.useRealTimers() // Use real timers for this test suite
-
-	let tempTestDir: string = ""
-	let currentTestFilePath = ""
-
-	beforeEach(async () => {
-		// Create a unique temporary directory for each test
-		const tempDirPrefix = path.join(os.tmpdir(), "safeWriteJson-test-")
-		tempTestDir = await fs.mkdtemp(tempDirPrefix)
-		currentTestFilePath = path.join(tempTestDir, "test-data.json")
-		// Ensure the file exists for locking purposes by default.
-		// Tests that need it to not exist must explicitly unlink it.
-		await fs.writeFile(currentTestFilePath, JSON.stringify({ initial: "content by beforeEach" }), "utf8")
-	})
-
-	afterEach(async () => {
-		if (tempTestDir) {
-			await fs.rm(tempTestDir, { recursive: true, force: true })
-			tempTestDir = ""
-		}
-		// activeLocks is no longer used
-
-		// Explicitly reset mock implementations to default (actual) behavior
-		// This helps prevent state leakage between tests if spy.mockRestore() isn't fully effective
-		// for functions on the module mock created by the factory.
-		;(fs.writeFile as jest.Mock).mockImplementation(actualFsPromises.writeFile)
-		;(fs.rename as jest.Mock).mockImplementation(actualFsPromises.rename)
-		;(fs.unlink as jest.Mock).mockImplementation(actualFsPromises.unlink)
-		;(fs.access as jest.Mock).mockImplementation(actualFsPromises.access)
-		;(fs.readFile as jest.Mock).mockImplementation(actualFsPromises.readFile)
-		;(fs.mkdtemp as jest.Mock).mockImplementation(actualFsPromises.mkdtemp)
-		;(fs.rm as jest.Mock).mockImplementation(actualFsPromises.rm)
-		;(fs.readdir as jest.Mock).mockImplementation(actualFsPromises.readdir)
-		// Ensure all mocks are reset after each test
-		jest.restoreAllMocks()
-	})
-
-	const readJsonFile = async (filePath: string): Promise<any | null> => {
-		try {
-			const content = await fs.readFile(filePath, "utf8") // Now uses the mocked fs
-			return JSON.parse(content)
-		} catch (error: any) {
-			if (error && error.code === "ENOENT") {
-				return null // File not found
-			}
-			throw error
-		}
-	}
-
-	const listTempFiles = async (dir: string, baseName: string): Promise<string[]> => {
-		const files = await fs.readdir(dir) // Now uses the mocked fs
-		return files.filter((f: string) => f.startsWith(`.${baseName}.new_`) || f.startsWith(`.${baseName}.bak_`))
-	}
-
-	// Success Scenarios
-	// Note: With the beforeEach change, this test now effectively tests overwriting the initial file.
-	// If "creation from non-existence" is critical and locking prevents it, safeWriteJson or locking strategy needs review.
-	test("should successfully write a new file (overwriting initial content from beforeEach)", async () => {
-		const data = { message: "Hello, new world!" }
-		await safeWriteJson(currentTestFilePath, data)
-
-		const writtenData = await readJsonFile(currentTestFilePath)
-		expect(writtenData).toEqual(data)
-		const tempFiles = await listTempFiles(tempTestDir, "test-data.json")
-		expect(tempFiles.length).toBe(0)
-	})
-
-	test("should successfully overwrite an existing file", async () => {
-		const initialData = { message: "Initial content" }
-		await fs.writeFile(currentTestFilePath, JSON.stringify(initialData)) // Now uses the mocked fs for setup
-
-		const newData = { message: "Updated content" }
-		await safeWriteJson(currentTestFilePath, newData)
-
-		const writtenData = await readJsonFile(currentTestFilePath)
-		expect(writtenData).toEqual(newData)
-		const tempFiles = await listTempFiles(tempTestDir, "test-data.json")
-		expect(tempFiles.length).toBe(0)
-	})
-
-	// Failure Scenarios
-	test("should handle failure when writing to tempNewFilePath", async () => {
-		// currentTestFilePath exists due to beforeEach, allowing lock acquisition.
-		const data = { message: "This should not be written" }
-
-		const mockErrorStream = new Writable() as jest.Mocked<Writable> & { _write?: any }
-		mockErrorStream._write = (_chunk: any, _encoding: any, callback: (error?: Error | null) => void) => {
-			// Simulate an error during write
-			callback(new Error("Simulated Stream Error: createWriteStream failed"))
-		}
-
-		// Mock createWriteStream to simulate a failure during the streaming of data to the temp file.
-		;(fsSyncActual.createWriteStream as jest.Mock).mockImplementationOnce((_path: any, _options: any) => {
-			const stream = new Writable({
-				write(_chunk, _encoding, cb) {
-					cb(new Error("Simulated Stream Error: createWriteStream failed"))
-				},
-				// Ensure destroy is handled to prevent unhandled rejections in stream internals
-				destroy(_error, cb) {
-					if (cb) cb(_error)
-				},
-			})
-			return stream as fsSyncActual.WriteStream
-		})
-
-		await expect(safeWriteJson(currentTestFilePath, data)).rejects.toThrow(
-			"Simulated Stream Error: createWriteStream failed",
-		)
-
-		const writtenData = await readJsonFile(currentTestFilePath)
-		// If write to .new fails, original file (from beforeEach) should remain.
-		expect(writtenData).toEqual({ initial: "content by beforeEach" })
-		const tempFiles = await listTempFiles(tempTestDir, "test-data.json")
-		expect(tempFiles.length).toBe(0) // All temp files should be cleaned up
-	})
-
-	test("should handle failure when renaming filePath to tempBackupFilePath (filePath exists)", async () => {
-		const initialData = { message: "Initial content, should remain" }
-		await originalFsPromisesWriteFile(currentTestFilePath, JSON.stringify(initialData)) // Use original for setup
-
-		const newData = { message: "This should not be written" }
-		const renameSpy = jest.spyOn(fs, "rename")
-		// First rename is target to backup
-		renameSpy.mockImplementationOnce(async (oldPath: any, newPath: any) => {
-			if (typeof newPath === "string" && newPath.includes(".bak_")) {
-				throw new Error("Simulated FS Error: rename to tempBackupFilePath")
-			}
-			return originalFsPromisesRename(oldPath, newPath) // Use constant
-		})
-
-		await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow(
-			"Simulated FS Error: rename to tempBackupFilePath",
-		)
-
-		const writtenData = await readJsonFile(currentTestFilePath)
-		expect(writtenData).toEqual(initialData) // Original file should be intact
-		const tempFiles = await listTempFiles(tempTestDir, "test-data.json")
-		// tempNewFile was created, but should be cleaned up. Backup was not created.
-		expect(tempFiles.filter((f: string) => f.includes(".new_")).length).toBe(0)
-		expect(tempFiles.filter((f: string) => f.includes(".bak_")).length).toBe(0)
-
-		renameSpy.mockRestore()
-	})
-
-	test("should handle failure when renaming tempNewFilePath to filePath (filePath exists, backup succeeded)", async () => {
-		const initialData = { message: "Initial content, should be restored" }
-		await fs.writeFile(currentTestFilePath, JSON.stringify(initialData)) // Use mocked fs for setup
-
-		const newData = { message: "This is in tempNewFilePath" }
-		const renameSpy = jest.spyOn(fs, "rename")
-		let renameCallCountTest1 = 0
-		renameSpy.mockImplementation(async (oldPath: any, newPath: any) => {
-			const oldPathStr = oldPath.toString()
-			const newPathStr = newPath.toString()
-			renameCallCountTest1++
-			console.log(`[TEST 1] fs.rename spy call #${renameCallCountTest1}: ${oldPathStr} -> ${newPathStr}`)
-
-			// First rename call by safeWriteJson (if target exists) is target -> .bak
-			if (renameCallCountTest1 === 1 && !oldPathStr.includes(".new_") && newPathStr.includes(".bak_")) {
-				console.log("[TEST 1] Spy: Call #1 (target->backup), executing original rename.")
-				return originalFsPromisesRename(oldPath, newPath)
-			}
-			// Second rename call by safeWriteJson is .new -> target
-			else if (
-				renameCallCountTest1 === 2 &&
-				oldPathStr.includes(".new_") &&
-				path.resolve(newPathStr) === path.resolve(currentTestFilePath)
-			) {
-				console.log("[TEST 1] Spy: Call #2 (.new->target), THROWING SIMULATED ERROR.")
-				throw new Error("Simulated FS Error: rename tempNewFilePath to filePath")
-			}
-			// Fallback for unexpected calls or if the target file didn't exist (only one rename: .new -> target)
-			else if (
-				renameCallCountTest1 === 1 &&
-				oldPathStr.includes(".new_") &&
-				path.resolve(newPathStr) === path.resolve(currentTestFilePath)
-			) {
-				// This case handles if the initial file didn't exist, so only one rename happens.
-				// For this specific test, we expect two renames.
-				console.warn(
-					"[TEST 1] Spy: Call #1 was .new->target, (unexpected for this test scenario, but handling)",
-				)
-				throw new Error("Simulated FS Error: rename tempNewFilePath to filePath")
-			}
-			console.warn(
-				`[TEST 1] Spy: Unexpected call #${renameCallCountTest1} or paths. Defaulting to original rename. ${oldPathStr} -> ${newPathStr}`,
-			)
-			return originalFsPromisesRename(oldPath, newPath)
-		})
-
-		// This scenario should reject because the new data couldn't be written to the final path,
-		// even if rollback succeeds.
-		await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow(
-			"Simulated FS Error: rename tempNewFilePath to filePath",
-		)
-
-		const writtenData = await readJsonFile(currentTestFilePath)
-		expect(writtenData).toEqual(initialData) // Original file should be restored from backup
-
-		const tempFiles = await listTempFiles(tempTestDir, "test-data.json")
-		expect(tempFiles.length).toBe(0) // All temp/backup files should be cleaned up
-
-		renameSpy.mockRestore()
-	})
-
-	test("should handle failure when deleting tempBackupFilePath (filePath exists, all renames succeed)", async () => {
-		const initialData = { message: "Initial content" }
-		await fs.writeFile(currentTestFilePath, JSON.stringify(initialData)) // Use mocked fs for setup
-
-		const newData = { message: "This should be the final content" }
-		const unlinkSpy = jest.spyOn(fs, "unlink")
-		// The unlink that targets the backup file fails
-		unlinkSpy.mockImplementationOnce(async (filePath: any) => {
-			const filePathStr = filePath.toString()
-			if (filePathStr.includes(".bak_")) {
-				console.log("[TEST unlink bak] Mock: Simulating failure for unlink backup.")
-				throw new Error("Simulated FS Error: delete tempBackupFilePath")
-			}
-			console.log("[TEST unlink bak] Mock: Condition NOT MET. Using originalFsPromisesUnlink.")
-			return originalFsPromisesUnlink(filePath)
-		})
-
-		// The function itself should still succeed from the user's perspective,
-		// as the primary operation (writing the new data) was successful.
-		// The error during backup cleanup is logged but not re-thrown to the caller.
-		// However, the current implementation *does* re-throw. Let's test that behavior.
-		// If the desired behavior is to not re-throw on backup cleanup failure, the main function needs adjustment.
-		// The current safeWriteJson logic is to log the error and NOT reject.
-		const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation(() => {}) // Suppress console.error
-
-		await expect(safeWriteJson(currentTestFilePath, newData)).resolves.toBeUndefined()
-
-		// The main file should be the new data
-		const writtenData = await readJsonFile(currentTestFilePath)
-		expect(writtenData).toEqual(newData)
-
-		// Check that the cleanup failure was logged
-		expect(consoleErrorSpy).toHaveBeenCalledWith(
-			expect.stringContaining(`Successfully wrote ${currentTestFilePath}, but failed to clean up backup`),
-			expect.objectContaining({ message: "Simulated FS Error: delete tempBackupFilePath" }),
-		)
-
-		const tempFiles = await listTempFiles(tempTestDir, "test-data.json")
-		// The .new file is gone (renamed to target), the .bak file failed to delete
-		expect(tempFiles.filter((f: string) => f.includes(".new_")).length).toBe(0)
-		expect(tempFiles.filter((f: string) => f.includes(".bak_")).length).toBe(1) // Backup file remains
-
-		unlinkSpy.mockRestore()
-		consoleErrorSpy.mockRestore()
-	})
-
-	// Note: With beforeEach change, currentTestFilePath will exist.
-	// This test's original intent was "filePath does not exist".
-	// It will now test the "filePath exists" path for the rename mock.
-	// The expected error message might need to change if the mock behaves differently.
-	test("should handle failure when renaming tempNewFilePath to filePath (filePath initially exists)", async () => {
-		// currentTestFilePath exists due to beforeEach.
-		// The original test unlinked it; we are removing that unlink to allow locking.
-		const data = { message: "This should not be written" }
-		const renameSpy = jest.spyOn(fs, "rename")
-
-		// The rename from tempNew to target fails.
-		// The mock needs to correctly simulate failure for the "filePath exists" case.
-		// The original mock was for "no prior file".
-		// For this test to be meaningful, the rename mock should simulate the failure
-		// appropriately when the target file (currentTestFilePath) exists.
-		// The existing complex mock in `test("should handle failure when renaming tempNewFilePath to filePath (filePath exists, backup succeeded)"`
-		// might be more relevant or adaptable here.
-		// For simplicity, let's use a direct mock for the second rename call (new->target).
-		let renameCallCount = 0
-		renameSpy.mockImplementation(async (oldPath: any, newPath: any) => {
-			renameCallCount++
-			const oldPathStr = oldPath.toString()
-			const newPathStr = newPath.toString()
-
-			if (renameCallCount === 1 && !oldPathStr.includes(".new_") && newPathStr.includes(".bak_")) {
-				// Allow first rename (target to backup) to succeed
-				return originalFsPromisesRename(oldPath, newPath)
-			}
-			if (
-				renameCallCount === 2 &&
-				oldPathStr.includes(".new_") &&
-				path.resolve(newPathStr) === path.resolve(currentTestFilePath)
-			) {
-				// Fail the second rename (tempNew to target)
-				throw new Error("Simulated FS Error: rename tempNewFilePath to existing filePath")
-			}
-			return originalFsPromisesRename(oldPath, newPath)
-		})
-
-		await expect(safeWriteJson(currentTestFilePath, data)).rejects.toThrow(
-			"Simulated FS Error: rename tempNewFilePath to existing filePath",
-		)
-
-		// After failure, the original content (from beforeEach or backup) should be there.
-		const writtenData = await readJsonFile(currentTestFilePath)
-		expect(writtenData).toEqual({ initial: "content by beforeEach" }) // Expect restored content
-		// The assertion `expect(writtenData).toBeNull()` was incorrect if rollback is successful.
-		const tempFiles = await listTempFiles(tempTestDir, "test-data.json")
-		expect(tempFiles.length).toBe(0) // All temp files should be cleaned up
-
-		renameSpy.mockRestore()
-	})
-
-	test("should throw an error if an inter-process lock is already held for the filePath", async () => {
-		jest.resetModules() // Clear module cache to ensure fresh imports for this test
-
-		const data = { message: "test lock" }
-		// Ensure the resource file exists.
-		await fs.writeFile(currentTestFilePath, "{}", "utf8")
-
-		// Temporarily mock proper-lockfile for this test only
-		jest.doMock("proper-lockfile", () => ({
-			...jest.requireActual("proper-lockfile"),
-			lock: jest.fn().mockRejectedValueOnce(new Error("Failed to get lock.")),
-		}))
-
-		// Re-require safeWriteJson so it picks up the mocked proper-lockfile
-		const { safeWriteJson: safeWriteJsonWithMockedLock } =
-			require("../safeWriteJson") as typeof import("../safeWriteJson")
-
-		try {
-			await expect(safeWriteJsonWithMockedLock(currentTestFilePath, data)).rejects.toThrow(
-				/Failed to get lock.|Lock file is already being held/i,
-			)
-		} finally {
-			jest.unmock("proper-lockfile") // Ensure the mock is removed after this test
-		}
-	})
-	test("should release lock even if an error occurs mid-operation", async () => {
-		const data = { message: "test lock release on error" }
-
-		// Mock createWriteStream to simulate a failure during the streaming of data,
-		// to test if the lock is released despite this mid-operation error.
-		;(fsSyncActual.createWriteStream as jest.Mock).mockImplementationOnce((_path: any, _options: any) => {
-			const stream = new Writable({
-				write(_chunk, _encoding, cb) {
-					cb(new Error("Simulated Stream Error during mid-operation write"))
-				},
-				// Ensure destroy is handled
-				destroy(_error, cb) {
-					if (cb) cb(_error)
-				},
-			})
-			return stream as fsSyncActual.WriteStream
-		})
-
-		await expect(safeWriteJson(currentTestFilePath, data)).rejects.toThrow(
-			"Simulated Stream Error during mid-operation write",
-		)
-
-		// Lock should be released, meaning the .lock file should not exist
-		const lockPath = `${path.resolve(currentTestFilePath)}.lock`
-		await expect(fs.access(lockPath)).rejects.toThrow(expect.objectContaining({ code: "ENOENT" }))
-	})
-
-	test("should handle fs.access error that is not ENOENT", async () => {
-		const data = { message: "access error test" }
-		const accessSpy = jest.spyOn(fs, "access").mockImplementationOnce(async () => {
-			const err = new Error("Simulated EACCES Error") as NodeJS.ErrnoException
-			err.code = "EACCES" // Simulate a permissions error, for example
-			throw err
-		})
-
-		await expect(safeWriteJson(currentTestFilePath, data)).rejects.toThrow("Simulated EACCES Error")
-
-		// Lock should be released, meaning the .lock file should not exist
-		const lockPath = `${path.resolve(currentTestFilePath)}.lock`
-		await expect(fs.access(lockPath)).rejects.toThrow(expect.objectContaining({ code: "ENOENT" }))
-
-		const tempFiles = await listTempFiles(tempTestDir, "test-data.json")
-		// .new file might have been created before access check, should be cleaned up
-		expect(tempFiles.filter((f: string) => f.includes(".new_")).length).toBe(0)
-
-		accessSpy.mockRestore()
-	})
-
-	// Test for rollback failure scenario
-	test("should log error and re-throw original if rollback fails", async () => {
-		const initialData = { message: "Initial, should be lost if rollback fails" }
-		await fs.writeFile(currentTestFilePath, JSON.stringify(initialData)) // Use mocked fs for setup
-		const newData = { message: "New data" }
-
-		const renameSpy = jest.spyOn(fs, "rename")
-		const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation(() => {}) // Suppress console.error
-		let renameCallCountTest2 = 0
-
-		renameSpy.mockImplementation(async (oldPath: any, newPath: any) => {
-			const oldPathStr = oldPath.toString()
-			const newPathStr = newPath.toString()
-			renameCallCountTest2++
-			const resolvedOldPath = path.resolve(oldPathStr)
-			const resolvedNewPath = path.resolve(newPathStr)
-			const resolvedCurrentTFP = path.resolve(currentTestFilePath)
-			console.log(
-				`[TEST 2] fs.promises.rename call #${renameCallCountTest2}: oldPath=${oldPathStr} (resolved: ${resolvedOldPath}), newPath=${newPathStr} (resolved: ${resolvedNewPath}), currentTFP (resolved: ${resolvedCurrentTFP})`,
-			)
-
-			if (renameCallCountTest2 === 1) {
-				// Call 1: Original -> Backup (Succeeds)
-				if (resolvedOldPath === resolvedCurrentTFP && newPathStr.includes(".bak_")) {
-					console.log("[TEST 2] Call #1 (Original->Backup): Condition MET. originalFsPromisesRename.")
-					return originalFsPromisesRename(oldPath, newPath)
-				}
-				console.error("[TEST 2] Call #1: UNEXPECTED args.")
-				throw new Error("Unexpected args for rename call #1 in test")
-			} else if (renameCallCountTest2 === 2) {
-				// Call 2: New -> Original (Fails - this is the "original error")
-				if (oldPathStr.includes(".new_") && resolvedNewPath === resolvedCurrentTFP) {
-					console.log(
-						'[TEST 2] Call #2 (New->Original): Condition MET. Throwing "Simulated FS Error: new to original".',
-					)
-					throw new Error("Simulated FS Error: new to original")
-				}
-				console.error("[TEST 2] Call #2: UNEXPECTED args.")
-				throw new Error("Unexpected args for rename call #2 in test")
-			} else if (renameCallCountTest2 === 3) {
-				// Call 3: Backup -> Original (Rollback attempt - Fails)
-				if (oldPathStr.includes(".bak_") && resolvedNewPath === resolvedCurrentTFP) {
-					console.log(
-						'[TEST 2] Call #3 (Backup->Original Rollback): Condition MET. Throwing "Simulated FS Error: backup to original (rollback)".',
-					)
-					throw new Error("Simulated FS Error: backup to original (rollback)")
-				}
-				console.error("[TEST 2] Call #3: UNEXPECTED args.")
-				throw new Error("Unexpected args for rename call #3 in test")
-			}
-			console.error(`[TEST 2] Unexpected fs.promises.rename call count: ${renameCallCountTest2}`)
-			return originalFsPromisesRename(oldPath, newPath)
-		})
-
-		await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow("Simulated FS Error: new to original")
-
-		// Check that the rollback failure was logged
-		expect(consoleErrorSpy).toHaveBeenCalledWith(
-			expect.stringContaining(
-				`Operation failed for ${path.resolve(currentTestFilePath)}: [Original Error Caught]`,
-			),
-			expect.objectContaining({ message: "Simulated FS Error: new to original" }), // The original error
-		)
-		expect(consoleErrorSpy).toHaveBeenCalledWith(
-			expect.stringMatching(/\[Catch\] Failed to restore backup .*?\.bak_.*?\s+to .*?:/), // Matches the backup filename pattern
-			expect.objectContaining({ message: "Simulated FS Error: backup to original (rollback)" }), // The rollback error
-		)
-		// The original error is logged first in safeWriteJson's catch block, then the rollback failure.
-
-		// File system state: original file is lost (backup couldn't be restored and was then unlinked),
-		// new file was cleaned up. The target path `currentTestFilePath` should not exist.
-		const finalState = await readJsonFile(currentTestFilePath)
-		expect(finalState).toBeNull()
-
-		const tempFiles = await listTempFiles(tempTestDir, "test-data.json")
-		// Backup file should also be cleaned up by the final unlink attempt in safeWriteJson's catch block,
-		// as that unlink is not mocked to fail.
-		expect(tempFiles.filter((f: string) => f.includes(".bak_")).length).toBe(0)
-		expect(tempFiles.filter((f: string) => f.includes(".new_")).length).toBe(0)
-
-		renameSpy.mockRestore()
-		consoleErrorSpy.mockRestore()
-	})
-})

+ 0 - 218
src/utils/safeWriteJson.ts

@@ -1,218 +0,0 @@
-import * as fs from "fs/promises"
-import * as fsSync from "fs"
-import * as path from "path"
-import * as lockfile from "proper-lockfile"
-import Disassembler from "stream-json/Disassembler"
-import Stringer from "stream-json/Stringer"
-
-/**
- * Safely writes JSON data to a file.
- * - 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.
- *
- * @param {string} filePath - The absolute path to the target file.
- * @param {any} data - The data to serialize to JSON and write.
- * @returns {Promise<void>}
- */
-
-async function safeWriteJson(filePath: string, data: any): Promise<void> {
-	const absoluteFilePath = path.resolve(filePath)
-	let releaseLock = async () => {} // Initialized to a no-op
-
-	// Acquire the lock before any file operations
-	try {
-		releaseLock = await lockfile.lock(absoluteFilePath, {
-			stale: 31000, // Stale after 31 seconds
-			update: 10000, // Update mtime every 10 seconds to prevent staleness if operation is long
-			retries: {
-				// Configuration for retrying lock acquisition
-				retries: 5, // Number of retries after the initial attempt
-				factor: 2, // Exponential backoff factor (e.g., 100ms, 200ms, 400ms, ...)
-				minTimeout: 100, // Minimum time to wait before the first retry (in ms)
-				maxTimeout: 1000, // Maximum time to wait for any single retry (in ms)
-			},
-			onCompromised: (err) => {
-				console.error(`Lock at ${absoluteFilePath} was compromised:`, err)
-				throw err
-			},
-		})
-	} catch (lockError) {
-		// If lock acquisition fails, we throw immediately.
-		// The releaseLock remains a no-op, so the finally block in the main file operations
-		// try-catch-finally won't try to release an unacquired lock if this path is taken.
-		console.error(`Failed to acquire lock for ${absoluteFilePath}:`, lockError)
-		// Propagate the lock acquisition error
-		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 {
-		// 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`,
-		)
-
-		await _streamDataToFile(actualTempNewFilePath, data)
-
-		// 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.
-		}
-
-		// 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,
-				)
-			}
-		}
-	} 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.
-		try {
-			// releaseLock will be the actual unlock function if lock was acquired,
-			// or the initial no-op if acquisition failed.
-			await releaseLock()
-		} catch (unlockError) {
-			// Do not re-throw here, as the originalError from the try/catch (if any) is more important.
-			console.error(`Failed to release lock for ${absoluteFilePath}:`, unlockError)
-		}
-	}
-}
-
-/**
- * Helper function to stream JSON data to a file.
- * @param targetPath The path to write the stream to.
- * @param data The data to stream.
- * @returns Promise<void>
- */
-async function _streamDataToFile(targetPath: string, data: any): Promise<void> {
-	// Stream data to avoid high memory usage for large JSON objects.
-	const fileWriteStream = fsSync.createWriteStream(targetPath, { encoding: "utf8" })
-	const disassembler = Disassembler.disassembler()
-	// Output will be compact JSON as standard Stringer is used.
-	const stringer = Stringer.stringer()
-
-	return new Promise<void>((resolve, reject) => {
-		let errorOccurred = false
-		const handleError = (_streamName: string) => (err: Error) => {
-			if (!errorOccurred) {
-				errorOccurred = true
-				if (!fileWriteStream.destroyed) {
-					fileWriteStream.destroy(err)
-				}
-				reject(err)
-			}
-		}
-
-		disassembler.on("error", handleError("Disassembler"))
-		stringer.on("error", handleError("Stringer"))
-		fileWriteStream.on("error", (err: Error) => {
-			if (!errorOccurred) {
-				errorOccurred = true
-				reject(err)
-			}
-		})
-
-		fileWriteStream.on("finish", () => {
-			if (!errorOccurred) {
-				resolve()
-			}
-		})
-
-		disassembler.pipe(stringer).pipe(fileWriteStream)
-
-		// stream-json's Disassembler might error if `data` is undefined.
-		// JSON.stringify(undefined) would produce the string "undefined" if it's the root value.
-		// Writing 'null' is a safer JSON representation for a root undefined value.
-		if (data === undefined) {
-			disassembler.write(null)
-		} else {
-			disassembler.write(data)
-		}
-		disassembler.end()
-	})
-}
-
-export { safeWriteJson }