From a37dd24f128e38aa4c4261d72ed580de73429fe2 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Thu, 27 Aug 2026 15:11:34 +0800 Subject: [PATCH 01/19] feat(file-safety): atomic text publish primitive + safeWriteJson refactor (A4, #1375) --- src/eslint-suppressions.json | 2 +- src/integrations/editor/DiffViewProvider.ts | 3 +- .../editor/__tests__/DiffViewProvider.spec.ts | 28 +- .../__tests__/safeWriteText.spec.ts | 614 ++++++++++++++++++ src/services/file-safety/safeWriteText.ts | 307 +++++++++ src/utils/__tests__/safeWriteJson.test.ts | 87 ++- src/utils/safeWriteJson.ts | 108 +-- 7 files changed, 1040 insertions(+), 109 deletions(-) create mode 100644 src/services/file-safety/__tests__/safeWriteText.spec.ts create mode 100644 src/services/file-safety/safeWriteText.ts diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 36cbfeac5b..77680449be 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -1721,7 +1721,7 @@ }, "utils/safeWriteJson.ts": { "@typescript-eslint/no-explicit-any": { - "count": 4 + "count": 3 } }, "utils/tts.ts": { diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index bb3368f063..36f5323f19 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -18,6 +18,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 +1157,7 @@ export class DiffViewProvider { // Write the content directly to the file await createDirectoriesForFile(absolutePath) - await fs.writeFile(absolutePath, content, "utf-8") + 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 diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index aee88f4061..511f0e7f3c 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -15,6 +15,14 @@ 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) +vi.mock("../../../services/file-safety/safeWriteText", () => ({ + safeWriteText: vi.fn().mockResolvedValue(undefined), })) // Mock utils @@ -26,6 +34,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 @@ -791,9 +801,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( @@ -814,9 +824,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() @@ -829,9 +839,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() diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts new file mode 100644 index 0000000000..4accc2b71e --- /dev/null +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -0,0 +1,614 @@ +import * as fs from "fs/promises" +import * as fsSync from "fs" +import { execFile } from "child_process" +import type { ChildProcess } from "child_process" +import * as path from "path" + +import { safeWriteText, type SafeWriteTextOptions } from "../safeWriteText" + +// Full mock for fs/promises — all methods are vi.fn() stubs +vi.mock("fs/promises", () => ({ + mkdir: vi.fn(), + access: vi.fn(), + rename: vi.fn(), + unlink: vi.fn(), + realpath: vi.fn(), +})) + +// Full mock for fs — all sync methods are vi.fn() stubs. Stats is a bare +// class stub so tests can build minimal Stats stand-ins via its prototype. +vi.mock("fs", () => ({ + openSync: vi.fn(), + writeSync: vi.fn(), + closeSync: vi.fn(), + mkdirSync: vi.fn(), + fsyncSync: vi.fn(), + chmodSync: vi.fn(), + fchmodSync: vi.fn(), + statSync: vi.fn(), + Stats: class Stats {}, +})) + +// Mock child_process.execFile (callback-based — must invoke callback to resolve) +vi.mock("child_process", () => ({ + execFile: vi.fn((cmd, args, opts, cb) => { + if (typeof cb === "function") cb(null) + }), +})) + +// Minimal stand-in for the ChildProcess that callback-form execFile returns. +const fakeChild = { kill: () => true } as unknown as ChildProcess + +// Helper that mirrors safeWriteText's path resolution exactly +function _resolvedTarget(filePath: string): string { + return path.resolve(filePath) +} +function _dirPath(filePath: string): string { + return path.dirname(_resolvedTarget(filePath)) +} +function _stagingDir(dir: string): string { + return path.join(dir, ".file-safety-staging") +} + +// Minimal Stats stand-in: the SUT only reads `.mode` from it. +function _stats(mode: number): fsSync.Stats { + const s = Object.create(fsSync.Stats.prototype) as fsSync.Stats + Object.assign(s, { mode }) + return s +} + +// ── Test 1: staging file created then cleaned after success ──────────────── + +describe("safeWriteText", () => { + beforeEach(() => { + vi.resetAllMocks() + // After resetAllMocks, vi.fn() returns undefined — restore promise defaults. + vi.mocked(fs.mkdir).mockResolvedValue(undefined) + vi.mocked(fs.access).mockResolvedValue(undefined) + vi.mocked(fs.rename).mockResolvedValue(undefined) + vi.mocked(fs.unlink).mockResolvedValue(undefined) + // Existing-target default: a regular 0o644 file. + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o644)) + // Default sync-write behaviour: report that all requested bytes were + // written. The Buffer overload passes (fd, buffer, offset, length), + // so the fourth argument is the requested length. + vi.mocked(fsSync.writeSync).mockImplementation((...args: unknown[]) => + typeof args[3] === "number" ? args[3] : 0, + ) + }) + + describe("staging and cleanup", () => { + it("creates a temp file in the staging dir, fsyncs it, renames to target, and cleans up on success", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) // fd=1 + vi.mocked(fsSync.closeSync).mockReturnValue(undefined) + + await safeWriteText(targetPath, "hello world", { platform: "linux" }) + + // staging dir was created with private permissions — use + // stringContaining to handle Windows path resolution + expect(fsSync.mkdirSync).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), { + recursive: true, + mode: 0o700, + }) + // a pre-existing staging dir is repaired to private permissions too + expect(fsSync.chmodSync).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), 0o700) + + // temp file was opened for writing with the existing target's mode + // (default 0o644 from the statSync default mock) + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), "w", 0o644) + + // content was written as a buffer (partial-write loop, full write) + expect(fsSync.writeSync).toHaveBeenCalledWith(1, Buffer.from("hello world", "utf8"), 0, 11) + + // fsync (sync form) was called on the fd + expect(fsSync.fsyncSync).toHaveBeenCalledWith(1) + + // file was closed + expect(fsSync.closeSync).toHaveBeenCalledWith(1) + + // atomic rename happened — realpath mock returns targetPath, so that's the dest + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // no unlink of temp (it's now the committed file; DACL skipped via platform:linux) + expect(fs.unlink).not.toHaveBeenCalled() + }) + }) + + // ── Test 2: fsync ordering ─────────────────────────────────────────────── + + describe("fsync ordering", () => { + it("calls fsync on the fd before close, and rename after close", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // Verify call order: openSync(temp) → writeSync → fsyncSync(temp) + // → closeSync(temp) → rename. On POSIX the parent directory is then + // opened and fsynced after the commit rename, so openSync/fsyncSync/ + // closeSync each have a second (directory) call. + expect(vi.mocked(fsSync.openSync).mock.calls.length).toBe(2) + expect(vi.mocked(fsSync.writeSync).mock.calls.length).toBe(1) + expect(vi.mocked(fsSync.fsyncSync).mock.calls.length).toBe(2) + expect(vi.mocked(fsSync.closeSync).mock.calls.length).toBe(2) + + // the temp file was fully closed before the commit rename + expect(vi.mocked(fsSync.closeSync).mock.calls[0][0]).toBe(1) + expect(fs.rename).toHaveBeenCalled() + }) + }) + + // ── Test 3: simulated failure between write and rename leaves target intact ── + + describe("crash/torn-write safety", () => { + it("simulated failure between fsync and rename leaves the target byte-identical and no temp left behind", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fs.rename).mockRejectedValue(new Error("ENOSPC")) + + await expect(safeWriteText(targetPath, "new data", { platform: "linux" })).rejects.toThrow("ENOSPC") + + // rename was attempted (the failure point) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // temp file was cleaned up on failure + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + + // backup was NOT created (backup:false by default), so target is untouched + // The only rename call was temp→target, not a rollback rename + expect(fs.rename).toHaveBeenCalledTimes(1) + }) + + it("a post-commit backup cleanup failure is non-fatal: the target stays committed and no temp is left behind", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The post-commit backup unlink (SUT step 6) fails — the write must + // still succeed; an orphaned backup is the documented acceptable + // outcome, so the failure is swallowed instead of rolling back. + vi.mocked(fs.unlink).mockRejectedValueOnce(new Error("EPERM")) + + await safeWriteText(targetPath, "data", { backup: true, platform: "linux" }) + + // the commit rename (temp -> target) still happened + expect(fs.rename).toHaveBeenNthCalledWith(2, expect.stringContaining("safeWriteText_"), targetPath) + + // the failing cleanup was the post-commit backup unlink + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + + // no rollback rename: the committed target is not restored from the backup + expect(fs.rename).toHaveBeenCalledTimes(2) + + // the staging temp was already committed by the rename; nothing + // temp-shaped is unlinked afterwards + expect(fs.unlink).not.toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + }) + + // ── Test 4: backup:true keeps old safeWriteJson semantics incl. rollback ── + + describe("backup:true", () => { + it("renames target -> backup before commit, deletes backup on success", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "new data", { backup: true }) + + // target was accessed (exists check) + expect(fs.access).toHaveBeenCalledWith(targetPath) + + // first rename: target -> backup + expect(fs.rename).toHaveBeenNthCalledWith(1, targetPath, expect.stringContaining("safeWriteText.bak_")) + + // second rename: temp -> target (realpath mock returns targetPath) + expect(fs.rename).toHaveBeenNthCalledWith(2, expect.stringContaining("safeWriteText_"), targetPath) + + // backup was deleted on success + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + }) + + it("rollback: on failure after rename target->backup, restores backup to target", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // first rename (target->backup) succeeds, second fails + let callCount = 0 + vi.mocked(fs.rename).mockImplementation(async () => { + callCount++ + if (callCount === 1) return // target -> backup + throw new Error("ENOSPC") // temp -> target fails + }) + + await expect(safeWriteText(targetPath, "new data", { backup: true })).rejects.toThrow("ENOSPC") + + // rollback rename is the 3rd call (after target->backup and temp->target failure) + expect(fs.rename).toHaveBeenNthCalledWith(3, expect.stringContaining("safeWriteText.bak_"), targetPath) + + // temp was cleaned up on failure + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("backup:true when target does not exist: no backup created, just commit", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // fs.access resolves for dirPath check, but rejects for target check (backup path) + vi.mocked(fs.access).mockImplementation(async (p) => { + if (typeof p === "string" && p.endsWith("target.txt")) throw { code: "ENOENT" } + }) + + await safeWriteText(targetPath, "new data", { backup: true, platform: "linux" }) + + // no backup rename (target didn't exist) + expect(fs.access).toHaveBeenCalledWith(targetPath) + + // only one rename: temp -> target + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // no unlink (no backup to delete; DACL skipped via platform:linux) + expect(fs.unlink).not.toHaveBeenCalled() + }) + }) + + // ── Test 5: win32 DACL path ────────────────────────────────────────────── + + describe("win32 DACL", () => { + it.skipIf(process.platform !== "win32")( + "copies target DACL onto staging file via icacls before rename on Windows", + async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // icacls dump + restore were called (execFile is callback-based mock) + expect(execFile).toHaveBeenCalledTimes(2) + }, + ) + + it("non-win32: DACL path is unreachable when platform is not win32", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // icacls was NOT called on non-win32 + expect(execFile).not.toHaveBeenCalled() + }) + + it("win32 DACL failure falls back to plain rename (never fails the write)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // icacls dump fails — the callback-based mock must invoke cb with an error. + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + if (typeof cb === "function") cb(new Error("icacls error"), "", "") + return fakeChild + }) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // write succeeded despite icacls failure (fallback to plain rename) + expect(fs.rename).toHaveBeenCalled() + }) + + it("win32 DACL save args are [targetPath, /save, dumpPath, /T] before backup rename", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { backup: true, platform: "win32" }) + + // icacls was called twice (save + restore) + expect(execFile).toHaveBeenCalledTimes(2) + + // First call: save DACL from target before backup rename + const firstCall = vi.mocked(execFile).mock.calls[0] + expect(firstCall[0]).toBe("icacls") + expect(firstCall[1]).toEqual([targetPath, "/save", expect.stringContaining(".acl.tmp"), "/T"]) + + // Second call: restore DACL onto directory after commit rename + const secondCall = vi.mocked(execFile).mock.calls[1] + expect(secondCall[0]).toBe("icacls") + expect(secondCall[1]).toEqual([ + expect.stringContaining("/tmp/test-dir"), + "/restore", + expect.stringContaining(".acl.tmp"), + ]) + + // dump file was unlinked after restore + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".acl.tmp")) + }) + + it("win32 DACL: dump is unlinked even when restore fails", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + // icacls save succeeds, restore fails + let callCount = 0 + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + callCount++ + if (typeof cb === "function") { + cb(callCount === 1 ? null : new Error("icacls restore error"), "", "") + } + return fakeChild + }) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // write succeeded despite restore failure (best-effort) + expect(fs.rename).toHaveBeenCalled() + + // dump file was still unlinked in finally + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".acl.tmp")) + }) + + it("win32 DACL: when target does not exist, no save/restore/dump", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + // fs.access rejects for targetPath (ENOENT), but resolves for dirPath + vi.mocked(fs.access).mockImplementation(async (p) => { + if (typeof p === "string" && p.endsWith("target.txt")) throw { code: "ENOENT" } + return undefined + }) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // icacls was NOT called (target absent → skip DACL entirely) + expect(execFile).not.toHaveBeenCalled() + + // no dump file created or unlinked + expect(fs.unlink).not.toHaveBeenCalled() + }) + }) + + // ── Test 6: pre-written temp path (tempPath option) ────────────────────── + + describe("pre-written temp path", () => { + it("uses the provided tempPath, fsyncs it, and renames to target", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + const customTempPath = "/tmp/custom-temp.tmp" + + // platform:linux skips DACL entirely so this test focuses on tempPath only + await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) + + // openSync was called on the custom temp path (r+ mode for fsync) + expect(fsSync.openSync).toHaveBeenCalledWith(customTempPath, "r+") + + // fsync was called + expect(fsSync.fsyncSync).toHaveBeenCalledWith(1) + + // rename happened — realpath mock returns targetPath + expect(fs.rename).toHaveBeenCalledWith(customTempPath, targetPath) + + // no unlink of custom temp (caller's concern; DACL skipped via platform:linux) + expect(fs.unlink).not.toHaveBeenCalled() + + // a caller-supplied tempPath must not create the staging directory + expect(fsSync.mkdirSync).not.toHaveBeenCalled() + }) + + it("applies the existing target's mode to a caller-supplied tempPath before publishing", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o600)) + vi.mocked(fsSync.openSync).mockReturnValue(2) + + const customTempPath = "/tmp/custom-temp.tmp" + + await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) + + // the caller-staged temp is fchmod'd to the restrictive target mode so + // the atomic rename cannot widen a 0o600 target (CWE-732 regression) + expect(fsSync.fchmodSync).toHaveBeenCalledWith(2, 0o600) + expect(fsSync.openSync).toHaveBeenCalledWith(customTempPath, "r+") + expect(fs.rename).toHaveBeenCalledWith(customTempPath, targetPath) + }) + + it("keeps the temp's default mode when the target does not exist yet (ENOENT)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + vi.mocked(fsSync.statSync).mockImplementation(() => { + throw enoent + }) + vi.mocked(fsSync.openSync).mockReturnValue(2) + + const customTempPath = "/tmp/custom-temp.tmp" + + await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) + + // no existing target, so nothing to preserve and no fchmod on the temp + expect(fsSync.fchmodSync).not.toHaveBeenCalled() + expect(fs.rename).toHaveBeenCalledWith(customTempPath, targetPath) + }) + + it("opens the temp before applying a read-only target's mode (0o444 does not block the open)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o444)) + vi.mocked(fsSync.openSync).mockReturnValue(3) + + const customTempPath = "/tmp/custom-temp.tmp" + + await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) + + // a 0o444 target must not make openSync(tempPath, "r+") fail: the mode + // is applied with fchmodSync on the already-open fd, after the open + expect(fsSync.openSync).toHaveBeenCalledWith(customTempPath, "r+") + expect(fsSync.fchmodSync).toHaveBeenCalledWith(3, 0o444) + const openIdx = vi.mocked(fsSync.openSync).mock.invocationCallOrder[0] + const fchmodIdx = vi.mocked(fsSync.fchmodSync).mock.invocationCallOrder[0] + expect(openIdx).toBeLessThan(fchmodIdx) + expect(fs.rename).toHaveBeenCalledWith(customTempPath, targetPath) + }) + }) + + // ── Test 7: symlink handling (Finding 4 regression test) ───────────────── + + describe("symlink handling", () => { + it("a write through a symlink commits onto the resolved referent, never the link path", async () => { + const linkPath = "/tmp/links/link.txt" + const referentPath = "/tmp/targets/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(referentPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(linkPath, "new-content", { platform: "linux" }) + + // The commit rename must target the realpath result (the referent), never the link itself — + // that is what guarantees a write through a symlink replaces the referent's content + // and preserves the link. + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), referentPath) + expect(fs.rename).not.toHaveBeenCalledWith(expect.anything(), linkPath) + }) + + it("when realpath reports ENOENT (target absent), uses the given path as-is", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // rename still happened with the fallback path (path.resolve on /tmp → C:\tmp) + const resolvedFallback = _resolvedTarget(targetPath) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), resolvedFallback) + }) + }) + + // ── Test 8: review fixes (permissions, partial writes, resolution, durability) ── + + describe("review fixes", () => { + it("preserves the target's restrictive mode and tolerates a failed staging-dir permission repair", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o600)) + // a pre-existing staging dir may fail its best-effort permission repair + vi.mocked(fsSync.chmodSync).mockImplementationOnce(() => { + throw new Error("EACCES") + }) + + await safeWriteText(targetPath, "secret", { platform: "linux" }) + + // the staging file inherits the target's 0o600 mode and the write commits + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), "w", 0o600) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("falls back to the 0o644 default when the target does not exist yet", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.statSync).mockImplementation(() => { + throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + }) + + await safeWriteText(targetPath, "fresh", { platform: "linux" }) + + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), "w", 0o644) + }) + + it("loops on short writes until the full content is durable before fsync", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const content = "0123456789" // 10 bytes + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const buffer = Buffer.from(content, "utf8") + // first write (offset 0) reports 4 bytes (short write); the loop continues + vi.mocked(fsSync.writeSync).mockImplementation((...args: unknown[]) => + args[2] === 0 ? 4 : typeof args[3] === "number" ? args[3] : 0, + ) + + await safeWriteText(targetPath, content, { platform: "linux" }) + + // [0,10) reports 4 bytes, then [4,10) writes the remaining 6 + expect(fsSync.writeSync).toHaveBeenCalledTimes(2) + expect(fsSync.writeSync).toHaveBeenNthCalledWith(1, 1, buffer, 0, 10) + expect(fsSync.writeSync).toHaveBeenNthCalledWith(2, 1, buffer, 4, 6) + expect(fsSync.fsyncSync).toHaveBeenCalledWith(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("fsyncs the parent directory after the commit rename on POSIX", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // temp fd=1 then parent-dir fd=2 - distinct fds prove the ordering + vi.mocked(fsSync.openSync).mockReturnValueOnce(1).mockReturnValue(2) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // the directory fsync (fd 2) happens only after the file fsync (fd 1); + // the dir path assertion is path-agnostic (stringContaining) because + // path.dirname renders the same input differently on Windows + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("test-dir"), "r") + expect(fsSync.fsyncSync).toHaveBeenNthCalledWith(1, 1) + expect(fsSync.fsyncSync).toHaveBeenNthCalledWith(2, 2) + expect(fsSync.closeSync).toHaveBeenCalledWith(2) + }) + + it("treats a failed parent-directory fsync as best-effort", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync) + .mockReturnValueOnce(1) + .mockImplementationOnce(() => { + throw new Error("EBADF") + }) + + // the content rename already committed; a missing directory fsync is not fatal + await safeWriteText(targetPath, "data", { platform: "linux" }) + + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("propagates realpath errors (EACCES and code-less) instead of the fallback path", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const eacces = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + vi.mocked(fs.realpath).mockRejectedValueOnce(eacces) + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toBe(eacces) + expect(fs.rename).not.toHaveBeenCalled() + + const plain = new Error("resolution failed") + vi.mocked(fs.realpath).mockRejectedValueOnce(plain) + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toBe(plain) + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("backup:true propagates access errors (EACCES and code-less) instead of skipping the backup", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const eacces = Object.assign(new Error("EACCES"), { code: "EACCES" }) + const plain = new Error("access failed") + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // each write accesses dirPath then target; only the target access rejects + const rejectTarget = (error: Error) => async (p: unknown) => { + if (typeof p === "string" && p.endsWith("target.txt")) throw error + } + vi.mocked(fs.access) + .mockImplementationOnce(rejectTarget(eacces)) + .mockImplementationOnce(rejectTarget(eacces)) + .mockImplementationOnce(rejectTarget(plain)) + .mockImplementationOnce(rejectTarget(plain)) + + await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux" })).rejects.toEqual( + expect.objectContaining({ code: "EACCES" }), + ) + await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux" })).rejects.toThrow( + "access failed", + ) + expect(fs.rename).not.toHaveBeenCalled() + }) + }) +}) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts new file mode 100644 index 0000000000..71871032e5 --- /dev/null +++ b/src/services/file-safety/safeWriteText.ts @@ -0,0 +1,307 @@ +import * as fs from "fs/promises" +import * as fsSync from "fs" +import * as path from "path" +import { execFile } from "child_process" + +/** + * Options for safeWriteText atomic text publish primitive. + */ +export interface SafeWriteTextOptions { + /** + * When true, preserve the old-file semantics: rename target -> backup first, + * after commit rename delete the backup; on failure roll the backup back to + * the target path. When false (default) the atomic rename simply replaces + * the target -- crash-safe window is zero. + */ + backup?: boolean + + /** + * Platform override for testing. When omitted the real process.platform + * value is used. Set to "win32" or "linux" / "darwin" from tests so that + * both branches are reachable without needing a real Windows runner. + */ + platform?: string + + /** + * Custom execFile runner for testing (e.g. vi.fn). When omitted the real + * child_process.execFile is used. + */ + execFileRunner?: typeof execFile + + /** + * Pre-written temp path to use for the commit phase. When provided, + * safeWriteText skips creating its own staging file and uses this path + * instead (it still fsyncs before rename). Useful when a caller has + * already written data to a temp file via a custom stream. + */ + tempPath?: string +} + +// -- helpers --------------------------------------------------------------- + +/** Generate a unique temp file name in the given directory. */ +function _tempName(dir: string, prefix: string): string { + return path.join(dir, "." + prefix + "_" + Date.now() + "_" + Math.random().toString(36).substring(2) + ".tmp") +} + +/** Create a private staging sub-directory inside *dir* so that multiple + * concurrent writes never collide on their temp names. */ +function _stagingDir(dir: string): string { + const sd = path.join(dir, ".file-safety-staging") + // mode:0o700 protects a freshly created staging dir; the best-effort chmod + // repairs a pre-existing one (mkdirSync with recursive:true never chmods an + // existing directory), so staged temp files are never group/world readable. + fsSync.mkdirSync(sd, { recursive: true, mode: 0o700 }) + try { + fsSync.chmodSync(sd, 0o700) + } catch { + // best-effort: chmod denied or unavailable; a fresh dir was still + // created with the requested mode + } + return sd +} + +/** + * fsync a file descriptor so its data is durable before the atomic rename. + * Uses the sync form because this repo's @types/node does not declare + * fs.promises.fsync; the staging file is small, so the blocking window is bounded. + */ +function _fsyncFile(fd: number): void { + fsSync.fsyncSync(fd) +} + +/** Save the DACL of *srcPath* to a dump file on Windows. + * Returns true when the dump was written successfully; false otherwise. + * Never throws — callers treat failure as "skip DACL handling". */ +async function _saveDaclWindows(srcPath: string, dumpPath: string, execFileRunner?: typeof execFile): Promise { + const runner = execFileRunner ?? execFile + try { + await new Promise((resolve, reject) => { + runner("icacls", [srcPath, "/save", dumpPath, "/T"], { windowsHide: true }, (err) => + err ? reject(err) : resolve(), + ) + }) + return true + } catch { + return false + } +} + +/** Restore a DACL dump onto *dirPath* on Windows. + * Best-effort: content is already committed, so failure is non-fatal. */ +async function _restoreDaclWindows(dirPath: string, dumpPath: string, execFileRunner?: typeof execFile): Promise { + const runner = execFileRunner ?? execFile + try { + await new Promise((resolve, reject) => { + runner("icacls", [dirPath, "/restore", dumpPath], { windowsHide: true }, (err) => + err ? reject(err) : resolve(), + ) + }) + } catch { + // best-effort; content already committed + } +} + +// -- public API ------------------------------------------------------------ + +/** + * Atomic text publish primitive. + * + * 1. Write content to a temp file in a private per-write staging subdir + * (same volume -> atomic rename guaranteed). + * 2. fsync the temp file, then close it. + * 3. win32 only: if target exists save its DACL dump BEFORE backup rename. + * 4. Optionally rename target -> backup (when backup:true). + * 5. Atomic rename temp -> target. + * 6. win32 only: restore DACL onto the directory AFTER commit rename. + * 7. On success: delete backup (if any) and unlink DACL dump. + * 8. On failure: rollback backup to target path; clean up temp + dump. + */ + +/** + * Resolve the publish target: the symlink referent when the given path is an + * existing symlink, the path itself otherwise. Only ENOENT (target absent yet) + * may fall back to the given path; any other resolution error (EACCES, EIO, ...) + * propagates so a broken or unreadable symlink is never written through its + * link path. Callers that stage a temp file themselves must stage it beside + * the resolved path: the commit is a rename onto the referent, and a rename + * across filesystems fails with EXDEV. + */ +export async function resolvePublishTarget(absoluteFilePath: string): Promise { + return fs.realpath(absoluteFilePath).catch((error: unknown) => { + const code = + typeof error === "object" && error !== null && "code" in error + ? (error as { code?: string }).code + : undefined + if (code !== "ENOENT") throw error + return absoluteFilePath + }) +} + +export async function safeWriteText(filePath: string, content: string, options?: SafeWriteTextOptions): Promise { + const absoluteFilePath = path.resolve(filePath) + + // Resolve the symlink referent (see resolvePublishTarget). + const targetPath = await resolvePublishTarget(absoluteFilePath) + const dirPath = path.dirname(targetPath) + + // Ensure parent directory exists (mirrors safeWriteJson behaviour). + await fs.mkdir(dirPath, { recursive: true }) + await fs.access(dirPath) + + // Create the staging directory only when we generate the temp file there; + // callers supplying their own tempPath (e.g. safeWriteJson) must not be left + // with an empty .file-safety-staging directory behind. + const tempPath = options?.tempPath ?? _tempName(_stagingDir(dirPath), "safeWriteText") + + let backupPath: string | null = null + let releaseBackupOnSuccess = false + let daclDumpPath: string | null = null // tracked for cleanup in finally + + try { + // -- Step 1: write content to staging temp file ------------------- + if (!options?.tempPath) { + // Preserve the existing target's permissions: the staging file must + // not be published wider than the file it replaces (a 0o600 target + // must not become 0o644 through the atomic rename). + let targetMode = 0o644 // default for a fresh target + try { + targetMode = fsSync.statSync(targetPath).mode & 0o777 + } catch { + // target does not exist yet - keep the default + } + const fd = fsSync.openSync(tempPath, "w", targetMode) + try { + // Loop until every byte is written: writeSync can report a short + // (partial) write, and publishing a truncated staging file would + // commit corrupt content. + const buffer = Buffer.from(content, "utf8") + let offset = 0 + while (offset < buffer.length) { + offset += fsSync.writeSync(fd, buffer, offset, buffer.length - offset) + } + _fsyncFile(fd) + } finally { + fsSync.closeSync(fd) + } + } else { + // Preserve the existing target's mode (CWE-732): the caller-staged + // temp carries its own creation mode, and publishing it as-is would + // widen a restrictive target (e.g. 0o600 -> 0o644) through rename. + // The mode is applied with fchmodSync on the open fd (AFTER openSync): + // chmodSync on the path before the open would make a read-only target + // (0o400/0o444) fail openSync(tempPath, "r+") with EACCES. + let targetMode: number | null = null + try { + targetMode = fsSync.statSync(targetPath).mode & 0o777 + } catch { + // target does not exist yet - keep the temp's default mode + } + const fd = fsSync.openSync(tempPath, "r+") + try { + if (targetMode !== null) { + fsSync.fchmodSync(fd, targetMode) + } + _fsyncFile(fd) + } finally { + fsSync.closeSync(fd) + } + } + + // -- Step 2 (win32): save DACL BEFORE backup rename --------------- + const platform = options?.platform ?? process.platform + if (platform === "win32") { + try { + await fs.access(targetPath) // target exists? + daclDumpPath = targetPath + ".acl.tmp" + const saved = await _saveDaclWindows(targetPath, daclDumpPath, options?.execFileRunner) + if (!saved) { + daclDumpPath = null // skip DACL handling entirely + } + } catch { + // target does not exist or access failed — no DACL handling + daclDumpPath = null + } + } + + try { + // -- Step 3 (backup:true): rename target -> backup -------------- + if (options?.backup) { + try { + await fs.access(targetPath) + backupPath = _tempName(dirPath, "safeWriteText.bak") + await fs.rename(targetPath, backupPath) + releaseBackupOnSuccess = true + } catch (err: unknown) { + const code = + typeof err === "object" && err !== null && "code" in err + ? (err as { code?: string }).code + : undefined + if (code !== "ENOENT") throw err + } + } + + // -- Step 4: atomic rename temp -> target --------------------- + await fs.rename(tempPath, targetPath) + + // -- Step 4b (POSIX): fsync the parent directory so the directory entry + // changed by the commit rename is durable, not just the file content. + if (platform !== "win32") { + try { + const dirFd = fsSync.openSync(dirPath, "r") + try { + _fsyncFile(dirFd) + } finally { + fsSync.closeSync(dirFd) + } + } catch { + // best-effort: the content rename already committed + } + } + + // -- Step 5 (win32): restore DACL AFTER commit rename --------- + if (platform === "win32" && daclDumpPath !== null) { + const restoredDir = path.dirname(targetPath) + await _restoreDaclWindows(restoredDir, daclDumpPath, options?.execFileRunner) + } + + // -- Step 6 (backup:true): delete backup on success ----------- + if (releaseBackupOnSuccess && backupPath) { + try { + await fs.unlink(backupPath) + } catch { + // non-fatal — orphaned backup is acceptable + } + } + } finally { + // Unlink DACL dump regardless of success/failure in this span. + if (daclDumpPath !== null) { + await fs.unlink(daclDumpPath).catch(() => {}) + } + } + + // tempPath is now the committed file; no cleanup needed. + } catch (originalError: unknown) { + // -- Rollback / cleanup on failure ---------------------------------- + if (backupPath && releaseBackupOnSuccess) { + try { + await fs.rename(backupPath, targetPath) + } catch { + // rollback failed — do not mask original error + } + } + + // Always clean up the staging temp file on failure. + try { + await fs.unlink(tempPath).catch(() => {}) + } catch { + // cleanup failure is non-fatal + } + + if (daclDumpPath !== null) { + await fs.unlink(daclDumpPath).catch(() => {}) + } + + throw originalError + } +} diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 79d08678a0..064207e21f 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -312,9 +312,8 @@ describe("safeWriteJson", () => { expect(content).toEqual(newData) }) - // Test for console error suppression during backup deletion - test("should suppress console.error when backup deletion fails", async () => { - const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) // Suppress console.error + // Test for best-effort backup deletion (the backup lifecycle now lives in safeWriteText) + test("does not fail the write when backup deletion fails (orphaned backup is acceptable)", async () => { const initialData = { message: "Initial" } const newData = { message: "New" } @@ -322,18 +321,23 @@ describe("safeWriteJson", () => { // 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_")) { + if (filePath.toString().includes("safeWriteText.bak_")) { throw new Error("Backup deletion failed") } return fsPromisesActuals.unlink!(filePath) }) + // The write must still succeed: backup cleanup is best-effort inside + // safeWriteText and never masks the committed content. await safeWriteJson(currentTestFilePath, newData) - // Verify console.error was called with the expected message - expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining("Successfully wrote"), expect.any(Error)) + const content = await readFileContent(currentTestFilePath) + expect(content).toEqual(newData) + + // The orphaned backup is still on disk because its deletion failed. + const entries = await fs.readdir(tempDir) + expect(entries.some((entry) => entry.includes("safeWriteText.bak_"))).toBe(true) - consoleErrorSpy.mockRestore() vi.mocked(fs.unlink).mockRestore() }) @@ -434,9 +438,9 @@ 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 () => { - const initialData = { message: "Initial, should be lost if rollback fails" } + // Test for rollback failure scenario (the rollback rename now lives in safeWriteText) + test("re-throws the original error when the rollback rename fails, leaving an orphaned backup", async () => { + const initialData = { message: "Initial, orphaned when rollback fails" } const newData = { message: "New content" } await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) @@ -451,20 +455,20 @@ describe("safeWriteJson", () => { // Second call: tempNewFilePath -> filePath (fail) throw new Error("Primary rename failed") } else if (renameCallCount === 3) { - // Third call: tempBackupFilePath -> filePath (rollback, also fail) + // Third call: backup -> filePath (rollback, also fail) throw new Error("Rollback rename failed") } return fsPromisesActuals.rename!(oldPath, newPath) }) - // Should throw the original error, not the rollback error + // The original error must propagate, 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 rollback failed inside safeWriteText, so the target is gone and + // the backup is orphaned on disk. + expect(await fileExists(currentTestFilePath)).toBe(false) + const entries = await fs.readdir(tempDir) + expect(entries.some((entry) => entry.includes("safeWriteText.bak_"))).toBe(true) consoleErrorSpy.mockRestore() }) @@ -542,4 +546,53 @@ describe("safeWriteJson", () => { const content = await readFileContent(currentTestFilePath) expect(content).toEqual({ c: 3 }) }) + + // The commit rename targets the symlink referent. The staged temp file must + // therefore be created beside the RESOLVED target — staging beside the link + // would make the commit rename fail with EXDEV when the referent is on + // another filesystem. (Real symlinks are unavailable in this CI lane, so the + // resolution is simulated by mocking fs.realpath the same way.) + test("stages the temp file beside the symlink referent and commits onto it", async () => { + const referentDir = path.join(tempDir, "referent") + const linkDir = path.join(tempDir, "link") + await fs.mkdir(referentDir, { recursive: true }) + await fs.mkdir(linkDir, { recursive: true }) + // caller-visible path (the link) vs the resolved referent path + const callerPath = path.join(linkDir, "test-file.json") + const referentPath = path.join(referentDir, "test-file.json") + // Seed the RESOLVED referent with real content (via the actual fs) so the + // write exercises replacement of an EXISTING referent: the lock is + // acquired on the caller path (realpath:false, which may be absent) while + // the backup + commit happen on the referent. + await fsPromisesActuals.writeFile!(referentPath, JSON.stringify({ seed: true })) + + vi.spyOn(fs, "realpath").mockResolvedValue(referentPath) + + await safeWriteJson(callerPath, { after: true }) + + // the temp file was created next to the resolved referent, NOT beside the link + const tempPaths = vi.mocked(fsSyncActual.createWriteStream).mock.calls.map((call) => String(call[0])) + expect(tempPaths.some((p) => p.startsWith(referentDir + path.sep) && p.includes(".new_"))).toBe(true) + expect(tempPaths.some((p) => p.startsWith(linkDir + path.sep))).toBe(false) + + // the content was committed onto the referent + expect(await readFileContent(referentPath)).toEqual({ after: true }) + }) + + // CWE-732 regression: safeWriteJson stages the temp itself and passes it + // via tempPath, so safeWriteText must apply the existing target's mode to + // the staged temp before the atomic rename — otherwise a 0o600 target is + // published as 0o644. POSIX-only assertion (Windows ignores POSIX modes). + test.skipIf(process.platform === "win32")( + "preserves a restrictive 0o600 target mode through the atomic publish", + async () => { + await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify({ before: true })) + fsSyncActual.chmodSync(currentTestFilePath, 0o600) + + await safeWriteJson(currentTestFilePath, { after: true }) + + expect(fsSyncActual.statSync(currentTestFilePath).mode & 0o777).toBe(0o600) + expect(await readFileContent(currentTestFilePath)).toEqual({ after: true }) + }, + ) }) diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index 957a0bb20f..26af906b43 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -4,6 +4,8 @@ import * as path from "path" import * as lockfile from "proper-lockfile" import { JsonStreamStringify } from "json-stream-stringify" +import { resolvePublishTarget, safeWriteText, type SafeWriteTextOptions } from "../services/file-safety/safeWriteText" + /** * Options for safeWriteJson function */ @@ -31,7 +33,7 @@ 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. + * - Writes to a temporary file first via JsonStreamStringify streaming. * - 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. @@ -41,7 +43,6 @@ export interface SafeWriteJsonOptions { * @param {SafeWriteJsonOptions} options - Optional configuration for JSON formatting. * @returns {Promise} */ - async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJsonOptions): Promise { const absoluteFilePath = path.resolve(filePath) let releaseLock = async () => {} // Initialized to a no-op @@ -51,10 +52,7 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso // Ensure directory structure exists with improved reliability try { - // Create directory with recursive option await fs.mkdir(dirPath, { recursive: true }) - - // Verify directory exists after creation attempt await fs.access(dirPath) } catch (dirError: any) { console.error(`Failed to create or access directory for ${absoluteFilePath}:`, dirError) @@ -84,13 +82,11 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso // 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. + // Variables to hold the actual path of the temp file if it is 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 @@ -110,79 +106,43 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso data = options.merge(existing, data) } - // Step 1: Write data to a new temporary file. + // Step 1: Write data to a new temporary file via JSON streaming. + // Stage it beside the *resolved* target (the symlink referent when the path is + // a symlink): safeWriteText commits by renaming onto that referent, and a + // rename across filesystems would fail with EXDEV. + const resolvedTargetPath = await resolvePublishTarget(absoluteFilePath) actualTempNewFilePath = path.join( - path.dirname(absoluteFilePath), - `.${path.basename(absoluteFilePath)}.new_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`, + path.dirname(resolvedTargetPath), + ".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. + // Step 2: Delegate backup + commit + rollback to safeWriteText with the + // pre-written temp path. backup:true keeps the old safeWriteJson + // semantics (target -> backup before commit, rollback on failure) and + // keeps the target in place until safeWriteText captures its Windows + // DACL (safeWriteText dumps the DACL before its own backup rename and + // restores it onto the directory after the commit rename). + const textOptions: SafeWriteTextOptions = { + tempPath: actualTempNewFilePath, + backup: true, } - // Step 3: Rename the new temporary file to the target file path. - // This is the main "commit" step. - await fs.rename(actualTempNewFilePath, absoluteFilePath) + await safeWriteText(absoluteFilePath, "", textOptions) - // 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" + // If we reach here, the new file is successfully in place and any + // backup has already been handled by safeWriteText. 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 + // A failed safeWriteText already rolled the backup (if any) back to + // the target path. Clean up the .new file if it still exists + // (safeWriteText also cleans up its tempPath on failure; this is a + // safety net in case its cleanup missed it). if (newFileToCleanupWithinCatch) { try { await fs.unlink(newFileToCleanupWithinCatch) @@ -194,26 +154,12 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso } } - // 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) } } From 991ab693526f1d9527fe8278a2e06f68b6575508 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Thu, 27 Aug 2026 17:42:34 +0800 Subject: [PATCH 02/19] feat(editor): async post-save diagnostics on chat-diff save path (L1, #1375) --- src/eslint-suppressions.json | 2 +- src/integrations/editor/DiffViewProvider.ts | 95 ++++++--- .../editor/__tests__/DiffViewProvider.spec.ts | 196 +++++++++++++++++- 3 files changed, 255 insertions(+), 38 deletions(-) diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 77680449be..aacb2ce602 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -1171,7 +1171,7 @@ }, "integrations/editor/__tests__/DiffViewProvider.spec.ts": { "@typescript-eslint/no-explicit-any": { - "count": 310 + "count": 306 } }, "integrations/editor/__tests__/EditorUtils.spec.ts": { diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 36f5323f19..386de8883c 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -1152,8 +1152,11 @@ export class DiffViewProvider { }> { const absolutePath = path.resolve(this.cwd, relPath) - // Get diagnostics before editing the file - this.preDiagnostics = vscode.languages.getDiagnostics() + // Get diagnostics before editing the file. Capture the snapshot locally: + // overlapping saveDirectly calls (multi-file edits) must not let a later + // call overwrite this one's baseline before its diagnostics tail runs. + const preDiagnostics = vscode.languages.getDiagnostics() + this.preDiagnostics = preDiagnostics // Write the content directly to the file await createDirectoriesForFile(absolutePath) @@ -1176,23 +1179,64 @@ export class DiffViewProvider { await doc.save() } - // Force a small delay to ensure diagnostics are triggered - await new Promise((resolve) => setTimeout(resolve, 100)) + // The 100 ms diagnostics-settle wait is carried by the + // emitPostSaveDiagnostics tail (inMemoryDocument) instead of here: + // blocking the save path delayed every openFile=false save even when + // diagnostics were disabled or the write delay was 0. } - let newProblemsMessage = "" - + // L1 (A2): resolve without awaiting the LSP diagnostics settle. The + // diagnostics check becomes a fire-and-forget tail that emits any new + // problems via the existing "error" ClineSay type; the returned + // newProblemsMessage is therefore always undefined. if (diagnosticsEnabled) { - // Add configurable delay to allow linters time to process - const safeDelayMs = Math.max(0, writeDelayMs) + // The method's outer try/catch guarantees it never rejects, so the + // fire-and-forget call needs no .catch wrapper. + void this.emitPostSaveDiagnostics(relPath, writeDelayMs, preDiagnostics, !openFile) + } - try { - await delay(safeDelayMs) - } catch (error) { - console.warn(`Failed to apply write delay: ${error}`) - } + // Store the results for formatFileWriteResponse + this.newProblemsMessage = undefined + this.userEdits = undefined + this.relPath = relPath + this.newContent = content - const postDiagnostics = vscode.languages.getDiagnostics() + return { + newProblemsMessage: undefined, + userEdits: undefined, + finalContent: content, + } + } + + // L1 (A2): fire-and-forget post-save diagnostics. After the write delay, + // collects new Error-severity problems and emits them via the existing + // "error" ClineSay type (only Error-severity diagnostics reach this branch; + // "error" carries no task-failure semantics in core). Abort-safe: say() + // rejects when the task is aborted, so the whole body sits inside a + // try/catch that degrades to a console.warn — the tail can never reject. + private async emitPostSaveDiagnostics( + relPath: string, + writeDelayMs: number, + preDiagnostics: [vscode.Uri, vscode.Diagnostic[]][], + inMemoryDocument = false, + ): Promise { + try { + // Add configurable delay to allow linters time to process. When the + // document was opened in memory (openFile=false), the tail also + // carries the 100 ms diagnostics-settle wait that used to block + // saveDirectly. delay() never rejects, so no catch is required here. + const safeDelayMs = Math.max(0, writeDelayMs) + (inMemoryDocument ? 100 : 0) + await delay(safeDelayMs) + + // Filter to the saved file: saveDirectly resolves before this tail + // completes, so in a multi-file write sequence (e.g. apply_patch) + // a later file's problems must not be attributed to this relPath. + const savedFilePath = path.resolve(this.cwd, relPath) + // arePathsEqual: case-insensitive on Windows, where a relPath whose + // casing differs from the diagnostic URI is still the same file. + const postDiagnostics = vscode.languages + .getDiagnostics() + .filter(([uri]) => arePathsEqual(uri.fsPath, savedFilePath)) // Get diagnostic settings from state const task = this.taskRef.deref() @@ -1201,27 +1245,20 @@ export class DiffViewProvider { const maxDiagnosticMessages = state?.maxDiagnosticMessages ?? 50 const newProblems = await diagnosticsToProblemsString( - getNewDiagnostics(this.preDiagnostics, postDiagnostics), + getNewDiagnostics(preDiagnostics, postDiagnostics), [vscode.DiagnosticSeverity.Error], this.cwd, includeDiagnosticMessages, maxDiagnosticMessages, ) - newProblemsMessage = - newProblems.length > 0 ? `\n\nNew problems detected after saving the file:\n${newProblems}` : "" - } - - // Store the results for formatFileWriteResponse - this.newProblemsMessage = newProblemsMessage - this.userEdits = undefined - this.relPath = relPath - this.newContent = content - - return { - newProblemsMessage, - userEdits: undefined, - finalContent: content, + if (newProblems.length > 0) { + await task?.say("error", `New problems detected after saving file: ${relPath}\n\n${newProblems}`) + } + } catch (error) { + // Abort-safe: never let a post-save diagnostic emit become an + // unhandled rejection (say() rejects when the task is aborted). + console.warn(`Post-save diagnostics emit failed: ${error}`) } } } diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 511f0e7f3c..05c65639dd 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -33,9 +33,14 @@ vi.mock("../../../utils/fs", () => ({ // Mock path vi.mock("path", () => ({ resolve: vi.fn((cwd, relPath) => `${cwd}/${relPath}`), + normalize: vi.fn((p: string) => p), basename: vi.fn((path) => path.split("/").pop()), dirname: vi.fn((path) => path.split("/").slice(0, -1).join("/") || "/"), join: (...args: string[]) => args.join("/"), + // diagnosticsToProblemsString formats its output header via + // path.relative(cwd, uri.fsPath).toPosix(); the object-with-toPosix shape + // mirrors the repo's own diagnostics.spec.ts mock. + relative: vi.fn((cwd: string, p: string) => ({ toPosix: () => p.replace(`${cwd}/`, "") })), })) // Mock vscode @@ -174,6 +179,8 @@ describe("DiffViewProvider", () => { }), }), }, + // L1: saveDirectly's fire-and-forget diagnostics tail emits via say(). + say: vi.fn().mockResolvedValue(true), } diffViewProvider = new DiffViewProvider(mockCwd, mockTask) @@ -811,12 +818,17 @@ describe("DiffViewProvider", () => { { preview: false, preserveFocus: true }, ) - // Verify diagnostics were checked after delay + // L1: saveDirectly resolves before the fire-and-forget diagnostics + // tail runs; flush one macrotask tick so the mocked delay (and the + // post-write getDiagnostics) have been reached before asserting. + await new Promise((resolve) => setTimeout(resolve, 0)) + + // Verify the tail applied the configured write delay expect(mockDelay).toHaveBeenCalledWith(2000) expect(vscode.languages.getDiagnostics).toHaveBeenCalled() - // Verify result - expect(result.newProblemsMessage).toBe("") + // Verify result: L1 no longer returns a problems message + expect(result.newProblemsMessage).toBeUndefined() expect(result.userEdits).toBeUndefined() expect(result.finalContent).toBe("new content") }) @@ -847,6 +859,10 @@ describe("DiffViewProvider", () => { expect(mockDelay).not.toHaveBeenCalled() // getDiagnostics is called once for pre-diagnostics, but not for post-diagnostics expect(vscode.languages.getDiagnostics).toHaveBeenCalledTimes(1) + + // L1: no diagnostics tail is launched, so nothing is ever emitted + await new Promise((resolve) => setTimeout(resolve, 0)) + expect(mockTask.say).not.toHaveBeenCalled() }) it("should handle negative delay values", async () => { @@ -855,6 +871,9 @@ describe("DiffViewProvider", () => { await diffViewProvider.saveDirectly("test.ts", "new content", true, true, -500) + // L1: the tail runs after resolve; flush one macrotask tick first. + await new Promise((resolve) => setTimeout(resolve, 0)) + // Verify delay was called with 0 (safe minimum) expect(mockDelay).toHaveBeenCalledWith(0) }) @@ -862,11 +881,172 @@ describe("DiffViewProvider", () => { it("should store results for formatFileWriteResponse", async () => { await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 1000) - // Verify internal state was updated - expect((diffViewProvider as any).newProblemsMessage).toBe("") - expect((diffViewProvider as any).userEdits).toBeUndefined() - expect((diffViewProvider as any).relPath).toBe("test.ts") - expect((diffViewProvider as any).newContent).toBe("new content") + // Verify internal state was updated (L1: the problems message is no + // longer stored; it is emitted asynchronously via say("error")) + expect(diffViewProvider["newProblemsMessage"]).toBeUndefined() + expect(diffViewProvider["userEdits"]).toBeUndefined() + expect(diffViewProvider["relPath"]).toBe("test.ts") + expect(diffViewProvider["newContent"]).toBe("new content") + }) + + it("resolves immediately and emits new problems via say('error') after the write delay", async () => { + const mockDelay = vi.mocked(delay) + mockDelay.mockClear() + vi.mocked(vscode.languages.getDiagnostics).mockClear() + + // Pre-write diagnostics are empty; the post-write snapshot (read by + // the fire-and-forget tail) reports one new Error-severity problem. + // vscode.workspace.fs.stat is an unimplemented vi.fn() mock, so + // diagnosticsToProblemsString takes its "(unavailable)" fallback + // branch and still formats the line. + const newDiag: vscode.Diagnostic = { + severity: vscode.DiagnosticSeverity.Error, + range: new vscode.Range(0, 0, 0, 1), + message: "boom", + } + const postDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [[makeUri(`${mockCwd}/test.ts`), [newDiag]]] + vi.mocked(vscode.languages.getDiagnostics).mockReturnValueOnce([]).mockReturnValue(postDiagnostics) + + const result = await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 100) + + // L1: saveDirectly resolves before the tail emits anything. + expect(result.newProblemsMessage).toBeUndefined() + expect(mockTask.say).not.toHaveBeenCalled() + + // Flush the fire-and-forget tail (the mocked delay resolves immediately). + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(mockDelay).toHaveBeenCalledWith(100) + expect(mockTask.say).toHaveBeenCalledTimes(1) + // The existing "error" ClineSay type is used, with the new-problems text. + expect(mockTask.say).toHaveBeenCalledWith( + "error", + expect.stringContaining("New problems detected after saving file: test.ts"), + ) + expect(mockTask.say.mock.calls[0]?.[1]).toContain("boom") + }) + + it("attributes only the saved file's new problems to the saved file", async () => { + vi.mocked(vscode.languages.getDiagnostics).mockClear() + + // Multi-file write sequence: a later file's new error must not be + // attributed to this tail's relPath by the workspace-wide snapshot. + const ownDiag: vscode.Diagnostic = { + severity: vscode.DiagnosticSeverity.Error, + range: new vscode.Range(0, 0, 0, 1), + message: "own-problem", + } + const otherDiag: vscode.Diagnostic = { + severity: vscode.DiagnosticSeverity.Error, + range: new vscode.Range(0, 0, 0, 1), + message: "other-file-problem", + } + const postDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [ + [makeUri(`${mockCwd}/test.ts`), [ownDiag]], + [makeUri(`${mockCwd}/other.ts`), [otherDiag]], + ] + vi.mocked(vscode.languages.getDiagnostics).mockReturnValueOnce([]).mockReturnValue(postDiagnostics) + + await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 100) + + // Flush the fire-and-forget tail. + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(mockTask.say).toHaveBeenCalledTimes(1) + expect(mockTask.say.mock.calls[0]?.[1]).toContain("own-problem") + expect(mockTask.say.mock.calls[0]?.[1]).not.toContain("other-file-problem") + }) + + it("attributes diagnostics to the saved file when the URI casing differs (Windows)", async () => { + const platformSpy = vi.spyOn(process, "platform", "get").mockReturnValue("win32") + vi.mocked(vscode.languages.getDiagnostics).mockClear() + + // The diagnostic URI uses different casing than the saved relPath: + // on Windows this is still the same file (arePathsEqual). + const newDiag: vscode.Diagnostic = { + severity: vscode.DiagnosticSeverity.Error, + range: new vscode.Range(0, 0, 0, 1), + message: "case-mismatch-problem", + } + const postDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [[makeUri(`${mockCwd}/Test.ts`), [newDiag]]] + vi.mocked(vscode.languages.getDiagnostics).mockReturnValueOnce([]).mockReturnValue(postDiagnostics) + + await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 100) + + // Flush the fire-and-forget tail. + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(mockTask.say).toHaveBeenCalledTimes(1) + expect(mockTask.say.mock.calls[0]?.[1]).toContain("case-mismatch-problem") + platformSpy.mockRestore() + }) + + it("does not block the save on a diagnostics settle delay when diagnostics are disabled", async () => { + // openFile=false used to await a 100 ms settle delay even when + // diagnostics were disabled; that delay now lives in the tail, which + // does not run at all when diagnosticsEnabled is false. + vi.mocked(vscode.languages.getDiagnostics).mockClear() + vi.mocked(vscode.languages.getDiagnostics).mockReturnValue([]) + const mockDelay = vi.mocked(delay) + mockDelay.mockClear() + + const result = await diffViewProvider.saveDirectly("test.ts", "new content", false, false) + + expect(result.finalContent).toBe("new content") + expect(mockDelay).not.toHaveBeenCalled() + expect(mockTask.say).not.toHaveBeenCalled() + }) + + it("carries the in-memory settle delay in the tail for openFile=false saves", async () => { + vi.mocked(vscode.languages.getDiagnostics).mockClear() + vi.mocked(vscode.languages.getDiagnostics).mockReturnValue([]) + const mockDelay = vi.mocked(delay) + mockDelay.mockClear() + + await diffViewProvider.saveDirectly("test.ts", "new content", false, true, 100) + + // writeDelayMs (100) + the 100 ms in-memory diagnostics settle, both + // applied by the tail instead of the save path. + expect(mockDelay).toHaveBeenCalledWith(200) + }) + + it("never calls say when there are no new problems", async () => { + vi.mocked(vscode.languages.getDiagnostics).mockClear() + vi.mocked(vscode.languages.getDiagnostics).mockReturnValue([]) + + await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 50) + + // Flush the fire-and-forget tail (pre/post snapshots are both empty, + // so diagnosticsToProblemsString returns "" and nothing is emitted). + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(mockTask.say).not.toHaveBeenCalled() + }) + + it("logs a warning instead of an unhandled rejection when the post-save say() is aborted", async () => { + const consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + vi.mocked(vscode.languages.getDiagnostics).mockClear() + + // One new Error-severity problem so the tail reaches say(). + const newDiag: vscode.Diagnostic = { + severity: vscode.DiagnosticSeverity.Error, + range: new vscode.Range(0, 0, 0, 1), + message: "boom", + } + const postDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [[makeUri(`${mockCwd}/test.ts`), [newDiag]]] + vi.mocked(vscode.languages.getDiagnostics).mockReturnValueOnce([]).mockReturnValue(postDiagnostics) + + // The task is aborted while the diagnostics tail is emitting: say() rejects. + mockTask.say.mockRejectedValueOnce(new Error("aborted")) + + await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 0) + // Flush the fire-and-forget tail. + await new Promise((resolve) => setTimeout(resolve, 0)) + + // The method's outer catch swallows the rejection with a warning. + expect(consoleWarnSpy).toHaveBeenCalledWith(expect.stringContaining("Post-save diagnostics emit failed:")) + + consoleWarnSpy.mockRestore() }) }) From cb1360613fa99272f67c123925dcaaa06aea6a89 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Sun, 30 Aug 2026 12:33:20 +0800 Subject: [PATCH 03/19] =?UTF-8?q?chore(ci):=20empty=20commit=20=E2=80=94?= =?UTF-8?q?=20re-trigger=20CI=20and=20the=20CodeRabbit=20current-head=20re?= =?UTF-8?q?view=20gate=20(no=20code=20change)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From 16caa6f8c846a41fa6506e32fbe951110d563395 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Tue, 6 Oct 2026 19:31:51 +0800 Subject: [PATCH 04/19] fix(file-safety): publish through a dangling symlink instead of replacing the link fs.realpath reports ENOENT both for an absent path and for a dangling symlink, and the ENOENT branch treated both as 'target absent', so a write through a dangling link replaced the link with a regular file. On ENOENT the code now checks whether the path is itself a link and, if so, resolves its intended referent; only a genuinely absent path still falls back to the given path. Also made the openFile=false save test assert the ordering it is about: the delay is held on a deferred promise and the save is shown to resolve while that promise is still pending, then released so the tail assertion still sees the combined delay. Local run: 27 safeWriteText tests pass, the DiffViewProvider suite passes, eslint clean, no suppression count change. --- .../editor/__tests__/DiffViewProvider.spec.ts | 19 ++++++++++++++++++- .../__tests__/safeWriteText.spec.ts | 19 +++++++++++++++++++ src/services/file-safety/safeWriteText.ts | 9 +++++++-- 3 files changed, 44 insertions(+), 3 deletions(-) diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 556a12e1b2..9af1d84d3e 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -1004,7 +1004,24 @@ describe("DiffViewProvider", () => { const mockDelay = vi.mocked(delay) mockDelay.mockClear() - await diffViewProvider.saveDirectly("test.ts", "new content", false, true, 100) + // Hold the tail delay so the save can be observed resolving while it is still + // pending: the save path must not await the settle delay itself. + let delayPending = true + let releaseDelay: () => void = () => {} + const tailDelay = new Promise((resolve) => { + releaseDelay = resolve + }) + void tailDelay.then(() => { + delayPending = false + }) + mockDelay.mockImplementation(() => tailDelay) + + const save = diffViewProvider.saveDirectly("test.ts", "new content", false, true, 100) + await save + expect(delayPending).toBe(true) + + releaseDelay() + await save // writeDelayMs (100) + the 100 ms in-memory diagnostics settle, both // applied by the tail instead of the save path. diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 4accc2b71e..48dd715d42 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -13,6 +13,7 @@ vi.mock("fs/promises", () => ({ rename: vi.fn(), unlink: vi.fn(), realpath: vi.fn(), + readlink: vi.fn(), })) // Full mock for fs — all sync methods are vi.fn() stubs. Stats is a bare @@ -477,6 +478,8 @@ describe("safeWriteText", () => { it("when realpath reports ENOENT (target absent), uses the given path as-is", async () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + // Not a link: readlink also reports ENOENT, so the path is genuinely absent. + vi.mocked(fs.readlink).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) vi.mocked(fsSync.openSync).mockReturnValue(1) await safeWriteText(targetPath, "data", { platform: "linux" }) @@ -485,6 +488,22 @@ describe("safeWriteText", () => { const resolvedFallback = _resolvedTarget(targetPath) expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), resolvedFallback) }) + + it("publishes through a dangling symlink to its intended referent", async () => { + const linkPath = "/tmp/test-dir/link.txt" + const referentPath = "/tmp/test-dir/real.txt" + // realpath reports ENOENT for a dangling link, so the code must not treat the link path + // as an absent target and replace the link with a regular file. + vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + vi.mocked(fs.readlink).mockResolvedValue("real.txt") + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(linkPath, "data", { platform: "linux" }) + + const expectedReferent = _resolvedTarget("/tmp/test-dir/real.txt") + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), expectedReferent) + expect(fs.rename).not.toHaveBeenCalledWith(expect.anything(), _resolvedTarget(linkPath)) + }) }) // ── Test 8: review fixes (permissions, partial writes, resolution, durability) ── diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index 71871032e5..ccfd5e869f 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -128,13 +128,18 @@ async function _restoreDaclWindows(dirPath: string, dumpPath: string, execFileRu * across filesystems fails with EXDEV. */ export async function resolvePublishTarget(absoluteFilePath: string): Promise { - return fs.realpath(absoluteFilePath).catch((error: unknown) => { + return fs.realpath(absoluteFilePath).catch(async (error: unknown) => { const code = typeof error === "object" && error !== null && "code" in error ? (error as { code?: string }).code : undefined if (code !== "ENOENT") throw error - return absoluteFilePath + // realpath reports ENOENT for an absent path and for a dangling symlink. A dangling + // symlink must still publish to its intended referent, otherwise the write replaces the + // link with a regular file and the link itself is lost. + const linkTarget = await fs.readlink(absoluteFilePath).catch(() => null) + if (linkTarget === null) return absoluteFilePath // genuinely absent: create at the given path + return path.resolve(path.dirname(absoluteFilePath), linkTarget) }) } From 83186ab12e24578fed6df7db70dba9e48747cbf3 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 04:06:14 +0800 Subject: [PATCH 05/19] fix(file-safety): distinguish readlink failures from an absent path, and report DACL restore failures resolvePublishTarget documents that only ENOENT may fall back to the given path, but the readlink fallback caught every error as "absent". EINVAL (not a link) and ENOENT (absent) are the only two that justify the fallback; EACCES/EIO/ENOTDIR mean the link could not be read, and falling back then publishes a regular file over the link and silently drops the referent contract. Those errors now propagate. _restoreDaclWindows swallowed every icacls failure with an empty catch and returned void, so a target that kept its inherited ACL instead of the DACL it had was indistinguishable from success. It now returns a status and logs the dump path, the target, and the exec error. The write still resolves: the content rename has already committed. Tests: EACCES from readlink rejects resolvePublishTarget with no rename; EINVAL falls back to the given path; a failing icacls restore logs the failure while the write still commits. Two fail on the pre-fix code (2 failed / 28 passed), all 30 pass after. safeWriteJson.test.ts (real filesystem) stays 22 passed. Not addressed here - see the reply on the DACL thread and the follow-up on the tracking issue: applying the saved DACL to the staged file BEFORE the commit rename. A prototype (per-write staging directory named after the target so `icacls /restore ` resolves the recorded entry, apply, abort on failure) breaks every write on a real Windows filesystem: safeWriteJson.test.ts runs against a real temp directory and the restore does not resolve the entry for a renamed/moved staged file, so the abort fires 16 times. Landing it needs a real-Windows integration test that pins icacls /restore semantics first. Local: eslint clean on both files with --prune-suppressions (no suppression change), package tsc clean. --- .../__tests__/safeWriteText.spec.ts | 49 ++++++++++++++++++- src/services/file-safety/safeWriteText.ts | 26 +++++++--- 2 files changed, 67 insertions(+), 8 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 48dd715d42..282884e165 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -4,7 +4,7 @@ import { execFile } from "child_process" import type { ChildProcess } from "child_process" import * as path from "path" -import { safeWriteText, type SafeWriteTextOptions } from "../safeWriteText" +import { resolvePublishTarget, safeWriteText, type SafeWriteTextOptions } from "../safeWriteText" // Full mock for fs/promises — all methods are vi.fn() stubs vi.mock("fs/promises", () => ({ @@ -351,6 +351,30 @@ describe("safeWriteText", () => { expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".acl.tmp")) }) + it("win32 DACL: a failed restore is reported instead of swallowed", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + let callCount = 0 + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + callCount++ + if (typeof cb === "function") cb(callCount === 1 ? null : new Error("icacls restore error"), "", "") + return fakeChild + }) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + // The write still succeeds (the content is already committed), but the permission + // regression is no longer invisible: a caller can otherwise never learn that the + // target kept the ACL it inherited instead of the one it had. + await safeWriteText(targetPath, "data", { platform: "win32" }) + + expect(fs.rename).toHaveBeenCalled() + expect(errorSpy).toHaveBeenCalledWith( + expect.stringContaining("Failed to restore the saved Windows DACL"), + expect.any(Error), + ) + }) + it("win32 DACL: when target does not exist, no save/restore/dump", async () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockResolvedValue(targetPath) @@ -475,7 +499,28 @@ describe("safeWriteText", () => { expect(fs.rename).not.toHaveBeenCalledWith(expect.anything(), linkPath) }) - it("when realpath reports ENOENT (target absent), uses the given path as-is", async () => { + it("only ENOENT/EINVAL from readlink may fall back to the link path", async () => { + const linkPath = "/tmp/test-dir/link.txt" + vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + // The path is a link we cannot read: falling back would publish a regular file + // over the link instead of failing, silently breaking the referent contract. + vi.mocked(fs.readlink).mockRejectedValue(Object.assign(new Error("EACCES"), { code: "EACCES" })) + + await expect(resolvePublishTarget(linkPath)).rejects.toThrow("EACCES") + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("readlink EINVAL (not a link) falls back to the given path", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + vi.mocked(fs.readlink).mockRejectedValue(Object.assign(new Error("EINVAL"), { code: "EINVAL" })) + + // resolvePublishTarget takes the already-resolved path and returns it unchanged + // when the path is not a link. + await expect(resolvePublishTarget(targetPath)).resolves.toBe(targetPath) + }) + + it("when realpath reports ENOENT (target absent), uses the given path as-is", async () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) // Not a link: readlink also reports ENOENT, so the path is genuinely absent. diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index ccfd5e869f..af83468ee4 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -88,8 +88,10 @@ async function _saveDaclWindows(srcPath: string, dumpPath: string, execFileRunne } /** Restore a DACL dump onto *dirPath* on Windows. - * Best-effort: content is already committed, so failure is non-fatal. */ -async function _restoreDaclWindows(dirPath: string, dumpPath: string, execFileRunner?: typeof execFile): Promise { + * Best-effort: the content is already committed, so a failure is not fatal - but it + * is reported, because a silently skipped restore is how a target ends up keeping + * the ACL it inherited instead of the one it had. */ +async function _restoreDaclWindows(dirPath: string, dumpPath: string, execFileRunner?: typeof execFile): Promise { const runner = execFileRunner ?? execFile try { await new Promise((resolve, reject) => { @@ -97,8 +99,10 @@ async function _restoreDaclWindows(dirPath: string, dumpPath: string, execFileRu err ? reject(err) : resolve(), ) }) - } catch { - // best-effort; content already committed + return true + } catch (error) { + console.error(`Failed to restore the saved Windows DACL from ${dumpPath} onto ${dirPath}:`, error) + return false } } @@ -137,8 +141,18 @@ export async function resolvePublishTarget(absoluteFilePath: string): Promise null) - if (linkTarget === null) return absoluteFilePath // genuinely absent: create at the given path + // EINVAL means the path exists but is not a link; ENOENT means it is absent. + // Anything else (EACCES, EIO, ENOTDIR) is a real resolution failure and must not + // be mistaken for "absent" and written through the link path. + const linkTarget = await fs.readlink(absoluteFilePath).catch((readlinkError: unknown) => { + const readlinkCode = + typeof readlinkError === "object" && readlinkError !== null && "code" in readlinkError + ? (readlinkError as { code?: string }).code + : undefined + if (readlinkCode !== "ENOENT" && readlinkCode !== "EINVAL") throw readlinkError + return null + }) + if (linkTarget === null) return absoluteFilePath // genuinely absent, or not a link: create at the given path return path.resolve(path.dirname(absoluteFilePath), linkTarget) }) } From 6e0ff8838002fbccecfb5e55d81ba1faf06d84f2 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 04:20:03 +0800 Subject: [PATCH 06/19] fix(file-safety): remove the staging directory after the write, and give the DACL dump a unique name _stagingDir creates .file-safety-staging inside the target's parent directory and nothing ever removed it, so every directory that receives a direct save keeps a permanent hidden directory in the explorer, in file listings and in watcher events. The directory is now removed best-effort after the commit rename and after failure cleanup; ENOTEMPTY (a concurrent writer still staging there) and any other error are ignored, since removing it is never worth failing a write whose content has already committed. A caller that supplied its own tempPath never created the directory, so nothing is removed for it. The DACL dump had the fixed name targetPath + ".acl.tmp": icacls /save would overwrite a user file at that path and the cleanup would then delete it, and two overlapping saves of the same target (saveDirectly takes no lock) would share one dump - one could unlink it while the other is still restoring from it. icacls /restore takes the dump path as an argument, so the name is free to choose; it is now unique per write. Tests: rmdir runs on the staging directory after success and after a failed write, and not at all when the caller supplied the temp; the dump path contains .acl.tmp, is not the old fixed name, and differs between two writes of the same target. Three fail on the pre-fix code (3 failed / 31 passed), all 34 pass after. safeWriteJson.test.ts (real filesystem) stays 22 passed. Local: eslint clean on both files with --prune-suppressions (no suppression change), package tsc clean. --- .../__tests__/safeWriteText.spec.ts | 55 +++++++++++++++++++ src/services/file-safety/safeWriteText.ts | 39 ++++++++++++- 2 files changed, 92 insertions(+), 2 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 282884e165..79b51969be 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -14,6 +14,7 @@ vi.mock("fs/promises", () => ({ unlink: vi.fn(), realpath: vi.fn(), readlink: vi.fn(), + rmdir: vi.fn(), })) // Full mock for fs — all sync methods are vi.fn() stubs. Stats is a bare @@ -259,6 +260,41 @@ describe("safeWriteText", () => { // ── Test 5: win32 DACL path ────────────────────────────────────────────── + describe("staging directory cleanup", () => { + it("removes the staging directory after a successful commit", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // The staging directory is created inside the user's own directory; leaving + // it behind litters every directory that ever receives a direct save. + expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + }) + + it("removes the staging directory after a failed write", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fs.rename).mockRejectedValue(new Error("EXDEV")) + + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toThrow("EXDEV") + + expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + }) + + it("leaves no staging directory to remove when the caller supplied the temp file", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "", { tempPath: "/tmp/test-dir/custom.tmp", platform: "linux" }) + + expect(fs.rmdir).not.toHaveBeenCalled() + }) + }) + describe("win32 DACL", () => { it.skipIf(process.platform !== "win32")( "copies target DACL onto staging file via icacls before rename on Windows", @@ -375,6 +411,25 @@ describe("safeWriteText", () => { ) }) + it("win32 DACL: the dump file name is unique per write", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "a", { platform: "win32" }) + const dump1 = String(vi.mocked(execFile).mock.calls[0][1]?.[2]) + vi.mocked(execFile).mockClear() + await safeWriteText(targetPath, "b", { platform: "win32" }) + const dump2 = String(vi.mocked(execFile).mock.calls[0][1]?.[2]) + + // A fixed name would let two overlapping saves of the same target share one + // dump (one unlinking it while the other is still restoring), and would let + // icacls /save overwrite - then delete - a user file at that path. + expect(dump1).toContain(".acl.tmp") + expect(dump1).not.toBe(targetPath + ".acl.tmp") + expect(dump2).not.toBe(dump1) + }) + it("win32 DACL: when target does not exist, no save/restore/dump", async () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockResolvedValue(targetPath) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index af83468ee4..4b39315a22 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -61,6 +61,29 @@ function _stagingDir(dir: string): string { return sd } +/** Unique name for the Windows DACL dump, kept beside the target. A fixed name + * would let icacls /save overwrite a user file that happens to live at that path + * (and the cleanup below would then delete it), and two overlapping saves of the + * same target would share one dump - one could unlink it while the other is still + * restoring from it. icacls /restore takes the dump path as an argument, so the + * name is free to choose. */ +function _aclDumpName(targetPath: string): string { + return targetPath + "." + Date.now() + "." + Math.random().toString(36).substring(2) + ".acl.tmp" +} + +/** Best-effort removal of the staging directory once the write is finished. The + * directory lives inside the user's own directory, so leaving it behind litters + * every directory that ever receives a write. ENOTEMPTY (a concurrent writer is + * still staging there) and any other error are ignored: removing it is never worth + * failing a write whose content has already committed. */ +async function _removeStagingDir(stagingDir: string): Promise { + try { + await fs.rmdir(stagingDir) + } catch { + // best-effort: not empty, already gone, or not permitted + } +} + /** * fsync a file descriptor so its data is durable before the atomic rename. * Uses the sync form because this repo's @types/node does not declare @@ -171,7 +194,9 @@ export async function safeWriteText(filePath: string, content: string, options?: // Create the staging directory only when we generate the temp file there; // callers supplying their own tempPath (e.g. safeWriteJson) must not be left // with an empty .file-safety-staging directory behind. - const tempPath = options?.tempPath ?? _tempName(_stagingDir(dirPath), "safeWriteText") + // Tracked so the directory can be removed again once the write is finished. + const stagingDirPath = options?.tempPath === undefined ? _stagingDir(dirPath) : null + const tempPath = options?.tempPath ?? _tempName(stagingDirPath ?? dirPath, "safeWriteText") let backupPath: string | null = null let releaseBackupOnSuccess = false @@ -232,7 +257,7 @@ export async function safeWriteText(filePath: string, content: string, options?: if (platform === "win32") { try { await fs.access(targetPath) // target exists? - daclDumpPath = targetPath + ".acl.tmp" + daclDumpPath = _aclDumpName(targetPath) const saved = await _saveDaclWindows(targetPath, daclDumpPath, options?.execFileRunner) if (!saved) { daclDumpPath = null // skip DACL handling entirely @@ -292,6 +317,11 @@ export async function safeWriteText(filePath: string, content: string, options?: // non-fatal — orphaned backup is acceptable } } + + // -- Step 7: remove the now-empty staging directory -------------- + if (stagingDirPath !== null) { + await _removeStagingDir(stagingDirPath) + } } finally { // Unlink DACL dump regardless of success/failure in this span. if (daclDumpPath !== null) { @@ -317,6 +347,11 @@ export async function safeWriteText(filePath: string, content: string, options?: // cleanup failure is non-fatal } + // And the staging directory itself, now that its only entry is gone. + if (stagingDirPath !== null) { + await _removeStagingDir(stagingDirPath) + } + if (daclDumpPath !== null) { await fs.unlink(daclDumpPath).catch(() => {}) } From a606b4189bf6f67f8cf7fccf3296d0a260d98e08 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 04:47:25 +0800 Subject: [PATCH 07/19] fix(utils): stage the JSON temp with the target's mode instead of the process default safeWriteJson streams the new payload into .new__.tmp beside the resolved target and only then delegates to safeWriteText. createWriteStream defaults to 0o666 masked by umask, i.e. 0o644, so a 0o600 settings file reached through a symlink that lives in a directory other users can read and search is republished as a world-readable temp for the whole duration of the write - the payload is readable before safeWriteText applies the target mode after streaming. The staged file now carries the existing target's mode (owner read/write is retained, so safeWriteText can still reopen it); a target that does not exist yet keeps the ordinary default. Tests (POSIX, skipped on win32 where the mode bits are not meaningful): staging a 0o600 target passes mode 0o600 to createWriteStream, and staging a 0o644 target passes 0o644 - no widening, no narrowing. Test hygiene in the diff-view spec: the in-memory settle-delay test asserted the tail's delay while the fire-and-forget tail was still running. Flush a macrotask after releasing it and assert task.say stayed silent, so the empty-diagnostics path is complete before the test ends. Local: safeWriteJson.test.ts 22 passed / 3 skipped, DiffViewProvider.spec.ts 78 passed, eslint clean on all three files with --prune-suppressions (no suppression change), package tsc clean. --- .../editor/__tests__/DiffViewProvider.spec.ts | 6 +++ src/utils/__tests__/safeWriteJson.test.ts | 41 +++++++++++++++++++ src/utils/safeWriteJson.ts | 25 +++++++++-- 3 files changed, 69 insertions(+), 3 deletions(-) diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 9af1d84d3e..167e5c32b5 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -1026,6 +1026,12 @@ describe("DiffViewProvider", () => { // writeDelayMs (100) + the 100 ms in-memory diagnostics settle, both // applied by the tail instead of the save path. expect(mockDelay).toHaveBeenCalledWith(200) + + // Let the fire-and-forget tail run to completion before the test ends: with no + // diagnostics the tail must stay silent, and an unfinished tail would leak + // into the next test's assertions. + await new Promise((resolve) => setTimeout(resolve, 0)) + expect(mockTask.say).not.toHaveBeenCalled() }) it("never calls say when there are no new problems", async () => { diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 064207e21f..39fa788508 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -102,6 +102,47 @@ describe("safeWriteJson", () => { } } + // Staging permissions + test.skipIf(process.platform === "win32")( + "stages the temp file with the existing target's mode instead of the process default", + async () => { + const target = path.join(tempDir, "private.json") + await fs.writeFile(target, JSON.stringify({ initial: 1 }), { mode: 0o600 }) + await fs.chmod(target, 0o600) + + const streamCalls = vi.mocked(fsSyncActual.createWriteStream) + streamCalls.mockClear() + + await safeWriteJson(target, { updated: 2 }) + + const staged = streamCalls.mock.calls.find((call) => String(call[0]).includes(".new_")) + expect(staged).toBeDefined() + // createWriteStream defaults to 0o666 (& ~umask = 0o644). The staged file holds + // the whole payload until the commit rename, so beside a 0o600 target it would + // be readable by other local users for the duration of the write. + expect(Number((staged![1] as { mode?: number } | undefined)?.mode)).toBe(0o600) + }, + ) + + test.skipIf(process.platform === "win32")( + "stages with the target's own mode when it is the ordinary 0o644", + async () => { + const target = path.join(tempDir, "public.json") + await fs.writeFile(target, JSON.stringify({ initial: 1 }), { mode: 0o644 }) + await fs.chmod(target, 0o644) + + const streamCalls = vi.mocked(fsSyncActual.createWriteStream) + streamCalls.mockClear() + + await safeWriteJson(target, { updated: 2 }) + + const staged = streamCalls.mock.calls.find((call) => String(call[0]).includes(".new_")) + expect(staged).toBeDefined() + // No widening and no narrowing: the staged file mirrors the target it replaces. + expect(Number((staged![1] as { mode?: number } | undefined)?.mode)).toBe(0o644) + }, + ) + // Success Scenarios // Note: Since we pre-create the file in beforeEach, this test will overwrite it. // If "creation from non-existence" is critical and locking prevents it, safeWriteJson or locking strategy needs review. diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index 61fafc369f..52a5a8c5ef 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -103,7 +103,19 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso ".new_" + Date.now() + "_" + Math.random().toString(36).substring(2) + ".tmp", ) - await _streamDataToFile(actualTempNewFilePath, data, options?.prettyPrint) + // The staged file holds the entire new content while it exists, so it must not + // be created with the process default (0o666 & ~umask, i.e. 0o644) next to a + // target that is deliberately narrower - a 0o600 settings file reached through + // a symlink that lives in a world-readable directory, for example. Match the + // existing target's mode; a new target keeps the normal default. + let stagingMode: number | undefined + try { + stagingMode = fsSync.statSync(resolvedTargetPath).mode & 0o777 + } catch { + stagingMode = undefined + } + + await _streamDataToFile(actualTempNewFilePath, data, options?.prettyPrint, stagingMode) // Step 2: Delegate backup + commit + rollback to safeWriteText with the // pre-written temp path. backup:true keeps the old safeWriteJson @@ -159,9 +171,16 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso * @param prettyPrint Whether to format the JSON with indentation. * @returns Promise */ -async function _streamDataToFile(targetPath: string, data: any, prettyPrint = false): Promise { +async function _streamDataToFile( + targetPath: string, + data: any, + prettyPrint = false, + mode?: number, +): Promise { // Stream data to avoid high memory usage for large JSON objects. - const fileWriteStream = fsSync.createWriteStream(targetPath, { encoding: "utf8" }) + // mode is explicit because createWriteStream defaults to 0o666 (& ~umask): the + // staged file is readable by others until the commit renames it onto the target. + const fileWriteStream = fsSync.createWriteStream(targetPath, { encoding: "utf8", ...(mode !== undefined ? { mode } : {}) }) // JsonStreamStringify traverses the object and streams tokens directly // The 'spaces' parameter adds indentation during streaming, not via a separate pass From 0278f02777c11606485630213f90a8fee7b16249 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 06:05:36 +0800 Subject: [PATCH 08/19] fix(utils): keep the staged JSON file owner-writable and surface non-ENOENT stat failures Same hardening as the sibling PR: mirroring the target mode verbatim broke the read-only case (a 0o400/0o444 target staged a file with no owner-write bit, and safeWriteText reopens the staged file with "r+" before it applies the target mode with fchmodSync, so the save failed with EACCES before the rename). The staged mode now ORs in 0o600 - owner read/write only, never group or world. The catch also swallowed every stat error and fell back to the 0o666 default creation mode, so a transient EIO would expose a restrictive target's new content in a shared directory for the duration of the write. Only ENOENT falls back; anything else is rethrown before anything is staged. Tests: a read-only (0o400) target stages with mode 0o600 and the write completes; a statSync that throws EIO rejects before createWriteStream is called and leaves no .new_ file. The EIO test fails on the pre-fix code (1 failed / 22 passed) and passes with the fix; the mode tests are POSIX-only and run on the ubuntu lane. Local: safeWriteJson.test.ts 23 passed / 4 skipped, eslint clean with --prune-suppressions (no suppression change), package tsc clean. --- src/utils/__tests__/safeWriteJson.test.ts | 51 +++++++++++++++++++++++ src/utils/safeWriteJson.ts | 21 +++++++--- 2 files changed, 67 insertions(+), 5 deletions(-) diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 39fa788508..ae95bfe139 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -143,6 +143,57 @@ describe("safeWriteJson", () => { }, ) + test.skipIf(process.platform === "win32")( + "keeps the staged file owner-writable when the target is read-only", + async () => { + const target = path.join(tempDir, "readonly.json") + await fs.writeFile(target, JSON.stringify({ initial: 1 })) + await fs.chmod(target, 0o400) + + const streamCalls = vi.mocked(fsSyncActual.createWriteStream) + streamCalls.mockClear() + + // Mirroring the target mode verbatim would stage a 0o400 file, and safeWriteText + // reopens the staged file with "r+" (before applying the target mode with + // fchmod), so the write would fail with EACCES for an ordinary user. + await safeWriteJson(target, { updated: 2 }) + + const staged = streamCalls.mock.calls.find((call) => String(call[0]).includes(".new_")) + expect(staged).toBeDefined() + expect(Number((staged![1] as { mode?: number } | undefined)?.mode)).toBe(0o600) + + expect(JSON.parse(await fs.readFile(target, "utf8"))).toEqual({ updated: 2 }) + await fs.chmod(target, 0o600) + }, + ) + + test( + "surfaces a stat failure other than ENOENT instead of staging with the default mode", + async () => { + const target = path.join(tempDir, "stat-fails.json") + await fs.writeFile(target, JSON.stringify({ initial: 1 })) + + const streamCalls = vi.mocked(fsSyncActual.createWriteStream) + streamCalls.mockClear() + const statSpy = vi.spyOn(fsSyncActual, "statSync").mockImplementation(() => { + throw Object.assign(new Error("EIO"), { code: "EIO" }) + }) + + try { + await expect(safeWriteJson(target, { updated: 2 })).rejects.toThrow(/EIO/) + // The stat failure has to surface BEFORE anything is staged: a staged file + // created with the wide default mode would sit beside a restrictive target + // for the duration of the write. Asserting no stream call (not just no + // leftover) is what pins the order - safeWriteText would also reject this + // EIO later, which alone would pass without any staging-mode check. + expect(streamCalls).not.toHaveBeenCalled() + expect((await fs.readdir(tempDir)).filter((entry) => entry.includes(".new_"))).toEqual([]) + } finally { + statSpy.mockRestore() + } + }, + ) + // Success Scenarios // Note: Since we pre-create the file in beforeEach, this test will overwrite it. // If "creation from non-existence" is critical and locking prevents it, safeWriteJson or locking strategy needs review. diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index 52a5a8c5ef..d0a93fca0e 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -103,15 +103,26 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso ".new_" + Date.now() + "_" + Math.random().toString(36).substring(2) + ".tmp", ) - // The staged file holds the entire new content while it exists, so it must not + // The staged file holds the entire new content while it exists, so it must not // be created with the process default (0o666 & ~umask, i.e. 0o644) next to a // target that is deliberately narrower - a 0o600 settings file reached through - // a symlink that lives in a world-readable directory, for example. Match the - // existing target's mode; a new target keeps the normal default. + // a symlink that lives in a world-readable directory, for example. Mirror the + // existing target's mode, but always keep the owner read/write bits: safeWriteText + // reopens the staged file with "r+" before it applies the target mode with fchmod, + // so a read-only mirror (0o400/0o444) would fail that open with EACCES. A target + // that does not exist yet keeps the normal default; any other stat failure is + // surfaced instead of silently widening the creation mode. let stagingMode: number | undefined try { - stagingMode = fsSync.statSync(resolvedTargetPath).mode & 0o777 - } catch { + stagingMode = (fsSync.statSync(resolvedTargetPath).mode & 0o777) | 0o600 + } catch (statError: unknown) { + const statCode = + typeof statError === "object" && statError !== null && "code" in statError + ? (statError as { code?: string }).code + : undefined + if (statCode !== "ENOENT") { + throw statError + } stagingMode = undefined } From ebc0aaf19ca507f83cca9d5200d9079df91303ff Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 06:23:55 +0800 Subject: [PATCH 09/19] fix(utils): key the safeWriteJson lock on the canonical publish target Same defect as the sibling PR: the advisory lock was taken on the caller's absolute path while the staging and the commit rename used the resolved publish target. proper-lockfile runs with realpath:false, so a writer that reaches the file through a symlink takes .lock while a writer that uses the referent takes .lock; the two do not serialize, and each read-modify-write merge starts from the same pre-write state, silently losing one update. resolvePublishTarget now runs before the lock, and the canonical path is used for the lock, the merge read, the staging directory, the staging-mode stat and the safeWriteText publish. Error messages still name the caller's path. Test: two concurrent merge writes, one through a symlink and one through its referent, with the first writer's commit rename gated until the second has taken its lock and read the target. POSIX-only (symlink creation fails with EPERM on this Windows host), so it runs on the ubuntu lane. The fsync ordering test also gained cross-mock invocation-order assertions (open/write/fsync/close of the staged file before the commit rename, staged fsync before its close, parent-directory fsync after the rename); the count-only version passed regardless of order. Local: safeWriteJson.test.ts 23 passed / 5 skipped, safeWriteText.spec.ts 34 passed, eslint clean with --prune-suppressions (no suppression change), package tsc clean. --- .../__tests__/safeWriteText.spec.ts | 16 +++++++ src/utils/__tests__/safeWriteJson.test.ts | 43 +++++++++++++++++++ src/utils/safeWriteJson.ts | 27 +++++++----- 3 files changed, 75 insertions(+), 11 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 79b51969be..4acb941e87 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -140,6 +140,22 @@ describe("safeWriteText", () => { // the temp file was fully closed before the commit rename expect(vi.mocked(fsSync.closeSync).mock.calls[0][0]).toBe(1) expect(fs.rename).toHaveBeenCalled() + + // Cross-mock invocation ORDER, not just call counts: a count-only assertion still + // passes if the implementation fsyncs after the commit rename, which is the exact + // durability regression this suite exists to catch. + const firstCallOf = (mock: { mock: { invocationCallOrder: number[] } }) => mock.mock.invocationCallOrder[0] + const renameOrder = firstCallOf(vi.mocked(fs.rename)) + expect(firstCallOf(vi.mocked(fsSync.openSync))).toBeLessThan(renameOrder) + expect(firstCallOf(vi.mocked(fsSync.writeSync))).toBeLessThan(renameOrder) + expect(firstCallOf(vi.mocked(fsSync.fsyncSync))).toBeLessThan(renameOrder) + expect(firstCallOf(vi.mocked(fsSync.closeSync))).toBeLessThan(renameOrder) + // the staged file is fsynced before it is closed + expect(vi.mocked(fsSync.fsyncSync).mock.invocationCallOrder[0]).toBeLessThan( + vi.mocked(fsSync.closeSync).mock.invocationCallOrder[0], + ) + // the parent-directory fsync is the second fsync and lands AFTER the rename + expect(vi.mocked(fsSync.fsyncSync).mock.invocationCallOrder[1]).toBeGreaterThan(renameOrder) }) }) diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index ae95bfe139..a66e19e53d 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -194,6 +194,49 @@ describe("safeWriteJson", () => { }, ) + test.skipIf(process.platform === "win32")( + "serializes a writer that reaches the file through a symlink with one that uses the referent", + async () => { + const referent = path.join(tempDir, "state.json") + await fsSyncActual.promises.writeFile(referent, JSON.stringify({ a: 1 }), "utf8") + const alias = path.join(tempDir, "alias.json") + await fs.symlink(referent, alias) + + const merge = (existing: unknown, incoming: unknown) => ({ + ...((existing ?? {}) as Record), + ...((incoming ?? {}) as Record), + }) + + // Hold the first writer's commit rename until the second writer has taken its + // lock and read the target. Keyed on the caller's alias the two writers take + // different lock files (.alias.json.lock vs state.json.lock), so the second + // read-modify-write starts from the pre-write state and its update is lost. + let proceed: () => void = () => {} + const proceedGate = new Promise((resolve) => { + proceed = resolve + }) + let reachedRename: () => void = () => {} + const reachedFirstRename = new Promise((resolve) => { + reachedRename = resolve + }) + vi.mocked(fs.rename).mockImplementationOnce(async (oldPath, newPath) => { + reachedRename() + await proceedGate + return fsPromisesActuals.rename!(oldPath, newPath) + }) + + const throughAlias = safeWriteJson(alias, { b: 2 }, { merge }) + await reachedFirstRename + const throughReferent = safeWriteJson(referent, { c: 3 }, { merge }) + // Let the second writer reach its lock attempt / read before the first commits. + await new Promise((resolve) => setTimeout(resolve, 150)) + proceed() + await Promise.all([throughAlias, throughReferent]) + + expect(JSON.parse(await fsSyncActual.promises.readFile(referent, "utf8"))).toEqual({ a: 1, b: 2, c: 3 }) + }, + ) + // Success Scenarios // Note: Since we pre-create the file in beforeEach, this test will overwrite it. // If "creation from non-existence" is critical and locking prevents it, safeWriteJson or locking strategy needs review. diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index d0a93fca0e..de360355c1 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -50,8 +50,16 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso const absoluteFilePath = path.resolve(filePath) let releaseLock = async () => {} // Initialized to a no-op + // Resolve the publish target (the symlink referent when the path is a symlink) + // BEFORE the lock is taken. The lock, the merge read, the staged file and the + // commit rename must all key off this one canonical path: if the lock is keyed on + // the caller's alias while the publish lands on the referent, two writers reaching + // the same file through different names (the link and its referent) serialize on + // different locks and silently lose each other's merged updates. + const canonicalPath = await resolvePublishTarget(absoluteFilePath) + // For directory creation - const dirPath = path.dirname(absoluteFilePath) + const dirPath = path.dirname(canonicalPath) // Ensure directory structure exists with improved reliability try { @@ -70,7 +78,7 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso // 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. - releaseLock = await acquireFileLock(absoluteFilePath) + releaseLock = await acquireFileLock(canonicalPath) // Variables to hold the actual path of the temp file if it is created. let actualTempNewFilePath: string | null = null @@ -82,7 +90,7 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso if (options?.merge) { let existing: unknown = null try { - existing = JSON.parse(await fs.readFile(absoluteFilePath, "utf8")) + existing = JSON.parse(await fs.readFile(canonicalPath, "utf8")) } catch (error: unknown) { const code = error && typeof error === "object" && "code" in error ? (error as { code: string }).code : undefined @@ -93,13 +101,10 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso data = options.merge(existing, data) } - // Step 1: Write data to a new temporary file via JSON streaming. - // Stage it beside the *resolved* target (the symlink referent when the path is - // a symlink): safeWriteText commits by renaming onto that referent, and a - // rename across filesystems would fail with EXDEV. - const resolvedTargetPath = await resolvePublishTarget(absoluteFilePath) + // Stage it beside the canonical target: safeWriteText commits by renaming onto + // that referent, and a rename across filesystems would fail with EXDEV. actualTempNewFilePath = path.join( - path.dirname(resolvedTargetPath), + path.dirname(canonicalPath), ".new_" + Date.now() + "_" + Math.random().toString(36).substring(2) + ".tmp", ) @@ -114,7 +119,7 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso // surfaced instead of silently widening the creation mode. let stagingMode: number | undefined try { - stagingMode = (fsSync.statSync(resolvedTargetPath).mode & 0o777) | 0o600 + stagingMode = (fsSync.statSync(canonicalPath).mode & 0o777) | 0o600 } catch (statError: unknown) { const statCode = typeof statError === "object" && statError !== null && "code" in statError @@ -139,7 +144,7 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso backup: true, } - await safeWriteText(absoluteFilePath, "", textOptions) + await safeWriteText(canonicalPath, "", textOptions) // If we reach here, the new file is successfully in place and any // backup has already been handled by safeWriteText. From 55db0130ce0aa2f43f5fac6c14216ec04d8eab38 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 08:03:37 +0800 Subject: [PATCH 10/19] fix(file-safety): default the staging mode only when the target is genuinely absent The target-mode lookup before staging swallowed every statSync error and fell back to 0o644. EACCES, ELOOP or ENOTDIR mean the target exists but its permissions are unknowable from here; staging at 0o644 and renaming over it can widen an existing 0o600 file, which is exactly what the lookup exists to prevent. The fallback is now ENOENT-only, using the same code extraction this file already applies at :163 and :290. Test: statSync throws EACCES for the target -> safeWriteText rejects with that error, fsSync.openSync is never called and nothing is renamed. Proven sensitive: reverting the source gives 1 failed / 34 passed; with it 35 passed. The existing ENOENT default test still passes. eslint clean; package tsc reports nothing in the files. --- .../__tests__/safeWriteText.spec.ts | 20 +++++++++++++++++++ src/services/file-safety/safeWriteText.ts | 14 +++++++++++-- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 4acb941e87..be0ffe2beb 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -531,6 +531,26 @@ describe("safeWriteText", () => { expect(fs.rename).toHaveBeenCalledWith(customTempPath, targetPath) }) + it("aborts instead of defaulting the mode when the target stat fails for another reason", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // EACCES means the target is there but unreadable to us, not that it is missing. + // Falling back to 0o644 would let the rename widen an existing file's mode. + const eacces = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + vi.mocked(fsSync.statSync).mockImplementation((p) => { + if (String(p) === targetPath) { + throw eacces + } + return _stats(0o644) + }) + + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toBe(eacces) + + // Nothing is staged and nothing is published on an unknown target mode. + expect(fsSync.openSync).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + }) + it("opens the temp before applying a read-only target's mode (0o444 does not block the open)", async () => { const targetPath = "/tmp/test-dir/target.txt" vi.mocked(fs.realpath).mockResolvedValue(targetPath) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index 4b39315a22..43116d0a2f 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -211,8 +211,18 @@ export async function safeWriteText(filePath: string, content: string, options?: let targetMode = 0o644 // default for a fresh target try { targetMode = fsSync.statSync(targetPath).mode & 0o777 - } catch { - // target does not exist yet - keep the default + } catch (statError) { + const statCode = + typeof statError === "object" && statError !== null && "code" in statError + ? (statError as { code?: string }).code + : undefined + // Only a genuinely missing target may take the default mode. Any other stat + // failure (EACCES, ELOOP, ENOTDIR) means the target exists but its real + // permissions are unknown from here: staging at 0o644 and renaming over it + // can widen an existing 0o600 file, so surface the error instead of guessing. + if (statCode !== "ENOENT") { + throw statError + } } const fd = fsSync.openSync(tempPath, "w", targetMode) try { From a95381915c301cfc07b0da625c94f7d6a01abc40 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 10:32:05 +0800 Subject: [PATCH 11/19] fix(mcp): confine a project-scoped settings write to the workspace safeWriteJson resolves the publish target through symlinks so the lock, the merge read and the commit rename all key off one canonical path. That is required for correctness, but it also means a repository that plants its project settings file (.roo/mcp.json) as a link to somewhere else redirects a settings write to the linked path. safeWriteJson now takes confineTo: when set, a resolved target that leaves that directory is rejected with ConfinedPathEscapeError before the lock is taken and before anything is staged. McpHub passes the workspace root for the writes whose path it picked from the workspace (updateMcpServers, the auto-approve tool list); the global settings file lives under the extension's global storage and is deliberately left unconfined. Tests: an out-of-scope target is rejected on every platform with nothing staged, and the planted-link case - a project mcp.json that links outside the workspace - is rejected with the linked file untouched while a link that stays inside the project still publishes to its referent (the two symlink cases are skipIf(win32), matching the existing symlink test in this file). --- src/services/mcp/McpHub.ts | 21 ++++++-- src/utils/__tests__/safeWriteJson.test.ts | 62 ++++++++++++++++++++++- src/utils/safeWriteJson.ts | 48 ++++++++++++++++++ 3 files changed, 127 insertions(+), 4 deletions(-) diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index 42786cfaa5..2f5a4baae2 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -621,6 +621,21 @@ export class McpHub { } // Get project-level MCP configuration path + /** + * Scope a settings write to the workspace when the caller picked the path from the + * workspace (the project .roo/mcp.json). safeWriteJson resolves the publish target + * through symlinks so the lock, the merge read and the commit rename all key off + * the same file - which means a repository that plants that file as a link to + * somewhere else would otherwise receive the settings write at the linked path. + * The global settings file lives under the extension's global storage rather than + * the workspace, so it is deliberately left unconfined. + */ + private confineToWorkspace(configPath: string): string | undefined { + const workspaceRoot = path.resolve(this.providerRef.deref()?.cwd ?? getWorkspacePath()) + const relative = path.relative(workspaceRoot, path.resolve(configPath)) + return relative !== "" && !relative.startsWith("..") && !path.isAbsolute(relative) ? workspaceRoot : undefined + } + private async getProjectMcpPath(): Promise { const workspacePath = this.providerRef.deref()?.cwd ?? getWorkspacePath() const projectMcpDir = path.join(workspacePath, ".roo") @@ -2091,7 +2106,7 @@ export class McpHub { } this.isProgrammaticUpdate = true try { - await safeWriteJson(configPath, updatedConfig, { prettyPrint: true }) + await safeWriteJson(configPath, updatedConfig, { prettyPrint: true, confineTo: this.confineToWorkspace(configPath) }) } finally { // Reset flag after watcher debounce period (non-blocking) this.flagResetTimer = setTimeout(() => { @@ -2176,7 +2191,7 @@ export class McpHub { mcpServers: config.mcpServers, } - await safeWriteJson(configPath, updatedConfig, { prettyPrint: true }) + await safeWriteJson(configPath, updatedConfig, { prettyPrint: true, confineTo: this.confineToWorkspace(configPath) }) // Update server connections with the correct source await this.updateServerConnections(config.mcpServers, serverSource) @@ -2385,7 +2400,7 @@ export class McpHub { } this.isProgrammaticUpdate = true try { - await safeWriteJson(normalizedPath, config, { prettyPrint: true }) + await safeWriteJson(normalizedPath, config, { prettyPrint: true, confineTo: this.confineToWorkspace(normalizedPath) }) } finally { // Reset flag after watcher debounce period (non-blocking) this.flagResetTimer = setTimeout(() => { diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index a66e19e53d..4d19348705 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -3,7 +3,7 @@ import { Writable } from "stream" import * as path from "path" import * as os from "os" -import { safeWriteJson } from "../safeWriteJson" +import { ConfinedPathEscapeError, safeWriteJson } from "../safeWriteJson" // Capture actual implementations before the vi.mock factory runs, // so they are never wrapped by vi.fn() — avoids infinite recursion when @@ -194,6 +194,66 @@ describe("safeWriteJson", () => { }, ) + test("rejects a confined write whose target is outside the confined directory", async () => { + const scope = path.join(tempDir, "project") + await fs.mkdir(scope) + const outside = path.join(tempDir, "elsewhere.json") + + // No symlink needed: the check runs on the resolved publish target, so an + // out-of-scope path is rejected on every platform, and it is rejected before the + // lock is taken and before anything is staged. + await expect(safeWriteJson(outside, { mcpServers: {} }, { confineTo: scope })).rejects.toThrow( + ConfinedPathEscapeError, + ) + + const left = await fs.readdir(tempDir) + expect(left).not.toContain("elsewhere.json") + expect(left.filter((entry) => entry.includes(".new_") || entry.endsWith(".lock"))).toEqual([]) + }) + + test.skipIf(process.platform === "win32")( + "rejects a confined write whose symlink resolves outside the confined directory", + async () => { + const projectDir = path.join(tempDir, "project") + await fs.mkdir(projectDir) + const outside = path.join(tempDir, "outside.json") + await fsSyncActual.promises.writeFile(outside, JSON.stringify({ secret: "original" }), "utf8") + // A repository that plants its project settings file as a link to somewhere else + // must not receive the settings write at the linked path. The caller picked + // projectDir/mcp.json from the workspace, so it declares that scope. + const projectConfig = path.join(projectDir, "mcp.json") + await fs.symlink(outside, projectConfig) + + await expect( + safeWriteJson(projectConfig, { mcpServers: {} }, { confineTo: projectDir }), + ).rejects.toThrow(ConfinedPathEscapeError) + + // The linked file is untouched and nothing was staged beside it. + expect(JSON.parse(await fsSyncActual.promises.readFile(outside, "utf8"))).toEqual({ secret: "original" }) + expect(await fs.readdir(tempDir)).toEqual(["outside.json", "project"]) + }, + ) + + test.skipIf(process.platform === "win32")( + "confines a write whose symlink referent stays inside the confined directory", + async () => { + const projectDir = path.join(tempDir, "project-in") + await fs.mkdir(projectDir) + const referent = path.join(projectDir, "real-mcp.json") + await fsSyncActual.promises.writeFile(referent, JSON.stringify({ mcpServers: {} }), "utf8") + const alias = path.join(projectDir, "mcp.json") + await fs.symlink(referent, alias) + + // Confining is about the scope, not about forbidding links: a link that stays + // inside the project still publishes to its referent. + await safeWriteJson(alias, { mcpServers: { local: { url: "http://localhost" } } }, { confineTo: projectDir }) + + expect(JSON.parse(await fsSyncActual.promises.readFile(referent, "utf8"))).toEqual({ + mcpServers: { local: { url: "http://localhost" } }, + }) + }, + ) + test.skipIf(process.platform === "win32")( "serializes a writer that reaches the file through a symlink with one that uses the referent", async () => { diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index de360355c1..e17feedf8e 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -30,6 +30,36 @@ export interface SafeWriteJsonOptions { * cannot be parsed. */ merge?: (existing: unknown, incoming: unknown) => unknown + + /** + * Confine the write to this directory. The publish target is resolved through + * symlinks (so the lock, the merge read and the commit rename all key off the same + * file), which means a caller that picked its path from a project directory can be + * redirected outside it by a link the repository planted. When this option is set, + * a resolved target that leaves the directory is rejected instead of written. + */ + confineTo?: string +} + +/** + * Error raised when a confined write resolves outside the directory it was confined + * to. The requested path is reported alongside the resolved one so the caller can + * tell a planted link from a plain wrong path. + */ +export class ConfinedPathEscapeError extends Error { + readonly requestedPath: string + readonly resolvedPath: string + readonly confineTo: string + + constructor(requestedPath: string, resolvedPath: string, confineTo: string) { + super( + `Refusing to write ${requestedPath}: it resolves to ${resolvedPath}, which is outside the confined directory ${confineTo}`, + ) + this.name = "ConfinedPathEscapeError" + this.requestedPath = requestedPath + this.resolvedPath = resolvedPath + this.confineTo = confineTo + } } /** @@ -58,6 +88,24 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso // different locks and silently lose each other's merged updates. const canonicalPath = await resolvePublishTarget(absoluteFilePath) + // Scope check after resolution and before the lock: this is where a link that + // leaves the caller's directory becomes visible, and rejecting here means no lock, + // no staging file and no publish for an out-of-scope target. + if (options?.confineTo) { + let scopeRoot: string + try { + scopeRoot = await fs.realpath(options.confineTo) + } catch { + // A scope that does not exist cannot contain the target either; fall back to + // the lexical root so the comparison still rejects the escape. + scopeRoot = path.resolve(options.confineTo) + } + const relative = path.relative(scopeRoot, canonicalPath) + if (relative === "" || relative === ".." || relative.startsWith(".." + path.sep) || path.isAbsolute(relative)) { + throw new ConfinedPathEscapeError(absoluteFilePath, canonicalPath, scopeRoot) + } + } + // For directory creation const dirPath = path.dirname(canonicalPath) From 0a3b9459eb2daf9ba299a189c9778576d76db9cc Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 10:48:33 +0800 Subject: [PATCH 12/19] fix(mcp): canonicalize both sides of the confinement check Two defects in the confineTo guard: - confineToWorkspace used a prefix test that treated any name starting with two dots as an escape, so a workspace entry named "..x" was left unconfined. The comparison is now separator-aware, matching safeWriteJson. - when realpath(confineTo) failed the scope fell back to its lexical path. A scope that runs through a symlinked prefix (macOS /var -> /private/var) or that does not exist yet then compared lexically against a resolved target and rejected every legitimate in-scope write. The nearest existing ancestor is now resolved and the remainder re-joined, and the same canonicalization is applied to the target: a target that does not exist yet still carries the alias components of the path it was given. The ubuntu platform-unit-test failure was an assertion in the new symlink case that forgot the file beforeEach pre-creates; it is now a filter for staging/lock residue instead of an exact directory listing. That case only runs on the POSIX lane (fs.symlink fails with EPERM on Windows), which is why it was not caught locally. --- src/services/mcp/McpHub.ts | 9 ++++- src/utils/__tests__/safeWriteJson.test.ts | 29 +++++++++++++- src/utils/safeWriteJson.ts | 49 ++++++++++++++++++----- 3 files changed, 76 insertions(+), 11 deletions(-) diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index 2f5a4baae2..d3c1da0bef 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -633,7 +633,14 @@ export class McpHub { private confineToWorkspace(configPath: string): string | undefined { const workspaceRoot = path.resolve(this.providerRef.deref()?.cwd ?? getWorkspacePath()) const relative = path.relative(workspaceRoot, path.resolve(configPath)) - return relative !== "" && !relative.startsWith("..") && !path.isAbsolute(relative) ? workspaceRoot : undefined + // Separator-aware: a sibling entry whose name merely starts with two dots + // ("..roo/mcp.json") is outside the workspace, while a directory named "..x" + // inside it is not. Comparing the prefix without the separator gets that wrong. + return ( + relative !== "" && relative !== ".." && !relative.startsWith(".." + path.sep) && !path.isAbsolute(relative) + ? workspaceRoot + : undefined + ) } private async getProjectMcpPath(): Promise { diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 4d19348705..48e44e54eb 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -230,7 +230,11 @@ describe("safeWriteJson", () => { // The linked file is untouched and nothing was staged beside it. expect(JSON.parse(await fsSyncActual.promises.readFile(outside, "utf8"))).toEqual({ secret: "original" }) - expect(await fs.readdir(tempDir)).toEqual(["outside.json", "project"]) + const entries = await fs.readdir(tempDir) + expect(entries).toContain("outside.json") + expect( + entries.filter((entry) => entry.includes(".new_") || entry.includes("safeWriteText") || entry.endsWith(".lock")), + ).toEqual([]) }, ) @@ -254,6 +258,29 @@ describe("safeWriteJson", () => { }, ) + test.skipIf(process.platform === "win32")( + "confines a scope path that itself runs through a symlink and does not exist yet", + async () => { + const real = path.join(tempDir, "real-project") + await fs.mkdir(real) + const alias = path.join(tempDir, "alias-project") + await fs.symlink(real, alias) + // The scope is declared through the alias, and the directory it names does not + // exist yet. Resolving it lexically would compare an unresolved scope against a + // fully resolved target and reject a write that is in fact inside the project - + // the macOS /var -> /private/var shape. The nearest existing ancestor is resolved + // and the remainder re-joined instead. + const nested = path.join(alias, "nested") + const target = path.join(nested, "mcp.json") + + await safeWriteJson(target, { mcpServers: {} }, { confineTo: nested }) + + expect( + JSON.parse(await fsSyncActual.promises.readFile(path.join(real, "nested", "mcp.json"), "utf8")), + ).toEqual({ mcpServers: {} }) + }, + ) + test.skipIf(process.platform === "win32")( "serializes a writer that reaches the file through a symlink with one that uses the referent", async () => { diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index e17feedf8e..2791db3133 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -62,6 +62,39 @@ export class ConfinedPathEscapeError extends Error { } } +/** + * Canonicalize the directory a write is confined to. The publish target is fully + * resolved through symlinks, so the scope has to be resolved the same way or a + * scope path that itself runs through a symlink (macOS /var -> /private/var is the + * common case) would compare lexically against a resolved target and reject every + * legitimate in-scope write. When the scope does not exist yet, the nearest + * existing ancestor is resolved and the remainder re-appended. + */ +async function _resolveScopeRoot(confineTo: string): Promise { + const lexical = path.resolve(confineTo) + try { + return await fs.realpath(lexical) + } catch { + // Walk up to the nearest ancestor that exists, then re-join what was missing. + const missing: string[] = [] + let ancestor = lexical + while (true) { + const parent = path.dirname(ancestor) + if (parent === ancestor) { + return lexical + } + missing.push(path.basename(ancestor)) + ancestor = parent + try { + const real = await fs.realpath(ancestor) + return path.join(real, ...missing.reverse()) + } catch { + // keep walking + } + } + } +} + /** * Safely writes JSON data to a file. * - Creates parent directories if they don't exist @@ -92,15 +125,13 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso // leaves the caller's directory becomes visible, and rejecting here means no lock, // no staging file and no publish for an out-of-scope target. if (options?.confineTo) { - let scopeRoot: string - try { - scopeRoot = await fs.realpath(options.confineTo) - } catch { - // A scope that does not exist cannot contain the target either; fall back to - // the lexical root so the comparison still rejects the escape. - scopeRoot = path.resolve(options.confineTo) - } - const relative = path.relative(scopeRoot, canonicalPath) + const scopeRoot = await _resolveScopeRoot(options.confineTo) + // Both sides have to be canonicalized the same way. The publish target is resolved + // through a symlink when it exists, but a target that does not exist yet keeps the + // alias components of the path it was given, so comparing it against a resolved + // scope would reject a write that is inside the scope. + const resolvedTarget = await _resolveScopeRoot(canonicalPath) + const relative = path.relative(scopeRoot, resolvedTarget) if (relative === "" || relative === ".." || relative.startsWith(".." + path.sep) || path.isAbsolute(relative)) { throw new ConfinedPathEscapeError(absoluteFilePath, canonicalPath, scopeRoot) } From 29819db8459dc925eff1d2ad4377b8d9957ea4a2 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 11:01:01 +0800 Subject: [PATCH 13/19] fix(file-safety): stop guessing when a scope or a mode cannot be read - _resolveScopeRoot walked up the tree for any realpath error. EACCES or ELOOP then produced a partly lexical root (a resolved ancestor plus unresolved components) that can disagree with the canonical target, so the confinement decision was made from a guess. Only ENOENT means "the path does not exist yet, walk up and re-join"; every other error is rethrown. - the caller-staged (tempPath) branch swallowed every statSync failure on the target and kept the temp's own mode. A non-ENOENT failure means the target exists but its permissions are unknown here, and publishing the temp as-is can widen a restrictive target (0o600 -> 0o644). The error now surfaces, matching the branch that stages itself. 137 passed / 8 skipped across safeWriteText.spec + safeWriteJson + DiffViewProvider; tsc and eslint clean. --- src/services/file-safety/safeWriteText.ts | 15 +++++++++++++-- src/utils/safeWriteJson.ts | 22 ++++++++++++++++++---- 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index 43116d0a2f..f9cbd5e43d 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -248,8 +248,19 @@ export async function safeWriteText(filePath: string, content: string, options?: let targetMode: number | null = null try { targetMode = fsSync.statSync(targetPath).mode & 0o777 - } catch { - // target does not exist yet - keep the temp's default mode + } catch (statError: unknown) { + // Only a genuinely missing target may keep the temp's own mode. Any other stat + // failure (EACCES, ELOOP, ENOTDIR) means the target exists but its real + // permissions are unknown from here, and publishing the caller-staged temp + // as-is can widen a restrictive target (0o600 -> 0o644), so surface the error + // instead of guessing. + const statCode = + typeof statError === "object" && statError !== null && "code" in statError + ? (statError as { code?: string }).code + : undefined + if (statCode !== "ENOENT") { + throw statError + } } const fd = fsSync.openSync(tempPath, "r+") try { diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index 2791db3133..ce362e6d44 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -74,8 +74,14 @@ async function _resolveScopeRoot(confineTo: string): Promise { const lexical = path.resolve(confineTo) try { return await fs.realpath(lexical) - } catch { - // Walk up to the nearest ancestor that exists, then re-join what was missing. + } catch (error: unknown) { + // Only a missing path means "walk up and re-join". EACCES or ELOOP means the + // scope cannot be canonicalized at all, and continuing would build a partly + // lexical root that can disagree with the canonical target - the failure has to + // surface rather than decide the scope from a guess. + if (_scopeErrorCode(error) !== "ENOENT") { + throw error + } const missing: string[] = [] let ancestor = lexical while (true) { @@ -88,13 +94,21 @@ async function _resolveScopeRoot(confineTo: string): Promise { try { const real = await fs.realpath(ancestor) return path.join(real, ...missing.reverse()) - } catch { - // keep walking + } catch (innerError: unknown) { + if (_scopeErrorCode(innerError) !== "ENOENT") { + throw innerError + } } } } } +function _scopeErrorCode(error: unknown): string | undefined { + return typeof error === "object" && error !== null && "code" in error + ? (error as { code?: string }).code + : undefined +} + /** * Safely writes JSON data to a file. * - Creates parent directories if they don't exist From 3fcb1376e4ec69bb50c544edc21207bc6a3f8538 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 06:49:12 +0000 Subject: [PATCH 14/19] chore: trigger a fresh review pass at this head From ad9764daaff32f7ee184f1ed8afa571d74a6ad7b Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 19:04:25 +0800 Subject: [PATCH 15/19] fix(file-safety): follow whole symlink chains and lock canonical paths Two ways a confined write could escape the scope it was checked in. 1. resolvePublishTarget followed a single hop when realpath reported ENOENT. For a dangling chain A -> B -> /outside/x, safeWriteJson resolved A to B, the confineTo check passed against in-scope B, and safeWriteText's own resolution of B then landed outside the scope. The chain is now followed to its end (MAX_SYMLINK_HOPS, then an error rather than a spin), so every resolution of the same path agrees on the final referent. 2. The advisory lock key was lexical: acquireFileLock disables proper-lockfile's realpath step (the file may not exist yet), so a writer reaching a file through a symlinked directory and a deleter reaching it lexically took different lock files and lost updates against each other. acquireFileLock and withFileLock now canonicalize the nearest existing ancestor and re-append the missing components, and withFileLock hands the canonical path to its callback. Tests: two-hop chain resolves to the final referent; a symlink loop errors instead of spinning; the dangling chain is rejected with ConfinedPathEscapeError and writes nothing; the fileLock spec pins the canonical key for the lock and for the callback, including the absent-file case. Controls: single-hop fails the two chain tests; a lexical lock key fails all three fileLock tests. --- .../__tests__/safeWriteText.spec.ts | 41 +++++++++- src/services/file-safety/safeWriteText.ts | 65 ++++++++++------ src/utils/__tests__/fileLock.spec.ts | 76 +++++++++++++++++++ src/utils/__tests__/safeWriteJson.test.ts | 25 ++++++ src/utils/fileLock.ts | 44 ++++++++++- 5 files changed, 226 insertions(+), 25 deletions(-) create mode 100644 src/utils/__tests__/fileLock.spec.ts diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index be0ffe2beb..608d532552 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -631,7 +631,11 @@ describe("safeWriteText", () => { // realpath reports ENOENT for a dangling link, so the code must not treat the link path // as an absent target and replace the link with a regular file. vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) - vi.mocked(fs.readlink).mockResolvedValue("real.txt") + vi.mocked(fs.readlink).mockImplementation(async (target) => { + if (String(target).endsWith("link.txt")) return "real.txt" + // The referent is absent and not a link either: the chain ends here. + throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + }) vi.mocked(fsSync.openSync).mockReturnValue(1) await safeWriteText(linkPath, "data", { platform: "linux" }) @@ -640,6 +644,41 @@ describe("safeWriteText", () => { expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), expectedReferent) expect(fs.rename).not.toHaveBeenCalledWith(expect.anything(), _resolvedTarget(linkPath)) }) + + it("follows a two-hop dangling chain to its final referent", async () => { + const linkPath = "/tmp/test-dir/link.txt" + // A -> B -> /elsewhere/final.txt, none of which exists. Stopping at B would let a + // scope check drawn around B pass while the commit landed outside that scope. + vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + vi.mocked(fs.readlink).mockImplementation(async (target) => { + if (String(target).endsWith("link.txt")) return "mid.txt" + if (String(target).endsWith("mid.txt")) return "/elsewhere/final.txt" + throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + }) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(linkPath, "data", { platform: "linux" }) + + expect(fs.rename).toHaveBeenCalledWith( + expect.stringContaining("safeWriteText_"), + _resolvedTarget("/elsewhere/final.txt"), + ) + expect(fs.rename).not.toHaveBeenCalledWith( + expect.anything(), + _resolvedTarget("/tmp/test-dir/mid.txt"), + ) + }) + + it("gives up on a symlink loop instead of spinning", async () => { + const linkPath = "/tmp/test-dir/loop.txt" + vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + vi.mocked(fs.readlink).mockImplementation(async () => linkPath) + + await expect(safeWriteText(linkPath, "data", { platform: "linux" })).rejects.toThrow( + /symlink hops/, + ) + expect(fs.rename).not.toHaveBeenCalled() + }) }) // ── Test 8: review fixes (permissions, partial writes, resolution, durability) ── diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index f9cbd5e43d..d1715e1028 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -145,6 +145,9 @@ async function _restoreDaclWindows(dirPath: string, dumpPath: string, execFileRu * 8. On failure: rollback backup to target path; clean up temp + dump. */ +/** Bound on how far a dangling chain is followed; past that the chain is a loop. */ +const MAX_SYMLINK_HOPS = 40 + /** * Resolve the publish target: the symlink referent when the given path is an * existing symlink, the path itself otherwise. Only ENOENT (target absent yet) @@ -153,31 +156,49 @@ async function _restoreDaclWindows(dirPath: string, dumpPath: string, execFileRu * link path. Callers that stage a temp file themselves must stage it beside * the resolved path: the commit is a rename onto the referent, and a rename * across filesystems fails with EXDEV. + * + * A dangling chain (A -> B -> C where C does not exist) is followed to its end, + * up to MAX_SYMLINK_HOPS, rather than stopping at the first referent: a caller + * that checks a scope against an intermediate hop must not have a later resolution + * of that hop land outside the scope it was checked in. */ + export async function resolvePublishTarget(absoluteFilePath: string): Promise { - return fs.realpath(absoluteFilePath).catch(async (error: unknown) => { - const code = - typeof error === "object" && error !== null && "code" in error - ? (error as { code?: string }).code - : undefined - if (code !== "ENOENT") throw error - // realpath reports ENOENT for an absent path and for a dangling symlink. A dangling - // symlink must still publish to its intended referent, otherwise the write replaces the - // link with a regular file and the link itself is lost. - // EINVAL means the path exists but is not a link; ENOENT means it is absent. - // Anything else (EACCES, EIO, ENOTDIR) is a real resolution failure and must not - // be mistaken for "absent" and written through the link path. - const linkTarget = await fs.readlink(absoluteFilePath).catch((readlinkError: unknown) => { - const readlinkCode = - typeof readlinkError === "object" && readlinkError !== null && "code" in readlinkError - ? (readlinkError as { code?: string }).code + let current = absoluteFilePath + for (let hop = 0; hop <= MAX_SYMLINK_HOPS; hop++) { + try { + return await fs.realpath(current) + } catch (error) { + const code = + typeof error === "object" && error !== null && "code" in error + ? (error as { code?: string }).code : undefined - if (readlinkCode !== "ENOENT" && readlinkCode !== "EINVAL") throw readlinkError - return null - }) - if (linkTarget === null) return absoluteFilePath // genuinely absent, or not a link: create at the given path - return path.resolve(path.dirname(absoluteFilePath), linkTarget) - }) + if (code !== "ENOENT") throw error + // realpath reports ENOENT for an absent path and for a dangling symlink. A dangling + // symlink must still publish to its intended referent, otherwise the write replaces the + // link with a regular file and the link itself is lost. + // EINVAL means the path exists but is not a link; ENOENT means it is absent. + // Anything else (EACCES, EIO, ENOTDIR) is a real resolution failure and must not + // be mistaken for "absent" and written through the link path. + const linkTarget = await fs.readlink(current).catch((readlinkError: unknown) => { + const readlinkCode = + typeof readlinkError === "object" && readlinkError !== null && "code" in readlinkError + ? (readlinkError as { code?: string }).code + : undefined + if (readlinkCode !== "ENOENT" && readlinkCode !== "EINVAL") throw readlinkError + return null + }) + if (linkTarget === null) return current // genuinely absent, or not a link: create there + // Follow the WHOLE chain, not one hop. Stopping at the first referent lets a chain + // A -> B -> /outside/x slip past a confinement check drawn around B: the check sees an + // in-scope B, while a later resolution of B lands outside the scope and the commit + // rename writes there. + current = path.resolve(path.dirname(current), linkTarget) + } + } + throw new Error( + `resolvePublishTarget: exceeded ${MAX_SYMLINK_HOPS} symlink hops resolving ${absoluteFilePath}`, + ) } export async function safeWriteText(filePath: string, content: string, options?: SafeWriteTextOptions): Promise { diff --git a/src/utils/__tests__/fileLock.spec.ts b/src/utils/__tests__/fileLock.spec.ts new file mode 100644 index 0000000000..97600bd68c --- /dev/null +++ b/src/utils/__tests__/fileLock.spec.ts @@ -0,0 +1,76 @@ +// npx vitest run utils/__tests__/fileLock.spec.ts + +import * as path from "path" + +import * as lockfile from "proper-lockfile" + +import { acquireFileLock, withFileLock } from "../fileLock" + +vi.mock("fs/promises", () => ({ + realpath: vi.fn(async (target: unknown) => { + const asString = String(target) + // A path that does not exist yet reports ENOENT, as the real fs does. + if (asString.includes("missing")) { + throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + } + return asString.replace("aliasDir", "realDir") + }), +})) + +vi.mock("proper-lockfile", () => ({ + lock: vi.fn(async () => async () => {}), +})) + +/** + * proper-lockfile derives its lock file from the path it is handed, and this module + * turns off the library's own realpath step because the file may not exist yet. Without + * a canonical lock key, a writer that reaches a file through a symlinked directory and a + * deleter that reaches the same file lexically take DIFFERENT locks and silently lose + * updates against each other - the shape a task file under a symlinked task directory + * has against a task-history delete. + */ +const STORE = path.resolve("/tmp/store") + +describe("fileLock - canonical lock keys", () => { + beforeEach(() => { + vi.mocked(lockfile.lock).mockClear() + }) + + test("locks the referent when the path runs through a symlinked directory", async () => { + const releaseLock = await acquireFileLock(path.join(STORE, "aliasDir", "task.json")) + + expect(lockfile.lock).toHaveBeenCalledWith( + path.join(STORE, "realDir", "task.json"), + expect.objectContaining({ realpath: false }), + ) + + await releaseLock() + }) + + test("hands the operation the canonical path, not the lexical one", async () => { + const seen: string[] = [] + await withFileLock(path.join(STORE, "aliasDir", "task.json"), async (absoluteFilePath) => { + seen.push(absoluteFilePath) + }) + + expect(seen).toEqual([path.join(STORE, "realDir", "task.json")]) + expect(lockfile.lock).toHaveBeenCalledWith( + path.join(STORE, "realDir", "task.json"), + expect.objectContaining({ realpath: false }), + ) + }) + + test("canonicalizes the nearest existing ancestor when the file does not exist yet", async () => { + // The file and its parent are absent, so realpath reports ENOENT; the deepest + // existing ancestor is resolved and the missing components are re-appended. + const seen: string[] = [] + await withFileLock( + path.join(STORE, "aliasDir", "missing", "new.json"), + async (absoluteFilePath) => { + seen.push(absoluteFilePath) + }, + ) + + expect(seen).toEqual([path.join(STORE, "realDir", "missing", "new.json")]) + }) +}) diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 48e44e54eb..087e30665a 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -258,6 +258,31 @@ describe("safeWriteJson", () => { }, ) + test.skipIf(process.platform === "win32")( + "rejects a dangling symlink chain that leaves the confined directory", + async () => { + const scope = path.join(tempDir, "scope-chain") + await fs.mkdir(scope) + // A -> B -> /outside-chain.json. B is inside the scope, the final referent + // is not, and nothing in the chain exists yet, so realpath reports ENOENT for every + // hop. Following only A -> B would confine-check an in-scope path and then publish + // outside it. + const link = path.join(scope, "mcp.json") + const middle = path.join(scope, "mid.json") + const outside = path.join(tempDir, "outside-chain.json") + await fs.symlink("mid.json", link) + await fs.symlink(outside, middle) + + await expect(safeWriteJson(link, { mcpServers: {} }, { confineTo: scope })).rejects.toThrow( + ConfinedPathEscapeError, + ) + + expect(await fileExists(outside)).toBe(false) + expect(await fileExists(middle)).toBe(false) + expect(await fileExists(link)).toBe(false) + }, + ) + test.skipIf(process.platform === "win32")( "confines a scope path that itself runs through a symlink and does not exist yet", async () => { diff --git a/src/utils/fileLock.ts b/src/utils/fileLock.ts index 9f7cad7653..79068cf382 100644 --- a/src/utils/fileLock.ts +++ b/src/utils/fileLock.ts @@ -1,5 +1,6 @@ import * as path from "path" import * as lockfile from "proper-lockfile" +import * as fs from "fs/promises" /** * Shared staleness window for per-file advisory locks. This module owns the @@ -8,6 +9,43 @@ import * as lockfile from "proper-lockfile" */ export const LOCK_STALE_MS = 31_000 +/** + * Canonical lock key for a path. + * + * proper-lockfile derives its lock file from the path it is handed, and this module + * turns off the library's own realpath step because the file may not exist yet. Two + * callers that reach the same file by different routes - one lexical, one through a + * symlinked directory - would then take DIFFERENT locks and silently lose updates + * against each other, which is exactly how a task file written through a symlinked + * task directory ends up unlocked against a task-history delete. + * + * The file itself may be absent (a create), so the nearest EXISTING ancestor is + * canonicalized and the missing components are re-appended. If nothing can be + * canonicalized the lexical absolute path is kept: lock keys stay stable and the + * write's own resolution still decides where content lands. + */ +async function canonicalLockPath(filePath: string): Promise { + const absoluteFilePath = path.resolve(filePath) + const missing: string[] = [] + let cursor = absoluteFilePath + for (;;) { + try { + const realPath = await fs.realpath(cursor) + return missing.length > 0 ? path.join(realPath, ...missing.reverse()) : realPath + } catch (error) { + const code = + typeof error === "object" && error !== null && "code" in error + ? (error as { code?: string }).code + : undefined + if (code !== "ENOENT" && code !== "ENOTDIR") return absoluteFilePath + const parent = path.dirname(cursor) + if (parent === cursor) return absoluteFilePath + missing.push(path.basename(cursor)) + cursor = parent + } + } +} + /** * Acquire the advisory lock for one file path using the exact protocol * `safeWriteJson` uses, so operations that hold this lock serialize with @@ -16,7 +54,7 @@ export const LOCK_STALE_MS = 31_000 * while holding it. */ export async function acquireFileLock(filePath: string): Promise<() => Promise> { - const absoluteFilePath = path.resolve(filePath) + const absoluteFilePath = await canonicalLockPath(filePath) try { return await lockfile.lock(absoluteFilePath, { stale: LOCK_STALE_MS, @@ -50,7 +88,9 @@ export async function withFileLock( filePath: string, operation: (absoluteFilePath: string) => Promise, ): Promise { - const absoluteFilePath = path.resolve(filePath) + // One canonical path for the lock AND for the operation: the callback must not + // unlink a lexical path while the lock was taken on the referent. + const absoluteFilePath = await canonicalLockPath(filePath) const releaseLock = await acquireFileLock(absoluteFilePath) let result: T From d88a43867a025540c6fc5d6536ff19dd3808c156 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 19:16:53 +0800 Subject: [PATCH 16/19] fix(utils): keep the caller's path for the locked operation The lock key stays canonical, but withFileLock no longer rewrites the path handed to its callback. platform-unit-test (windows-latest) caught the consequence: fs.realpath on a Windows runner answers with the 8.3 short form (C:\Users\RUNNER~1\AppData\Local\Temp for a temp dir under C:\Users\runneradmin\...), so TaskHistoryStore.deleteSemantics.spec.ts no longer recognized the path whose unlink it forces, and a batch that should have continued around a failed delete deleted the entry instead. The mutex only needs one key; the operation should keep the path its caller chose. --- src/utils/__tests__/fileLock.spec.ts | 18 +++++++++++++----- src/utils/fileLock.ts | 18 +++++++++++------- 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/src/utils/__tests__/fileLock.spec.ts b/src/utils/__tests__/fileLock.spec.ts index 97600bd68c..9fd8a7e80c 100644 --- a/src/utils/__tests__/fileLock.spec.ts +++ b/src/utils/__tests__/fileLock.spec.ts @@ -47,13 +47,17 @@ describe("fileLock - canonical lock keys", () => { await releaseLock() }) - test("hands the operation the canonical path, not the lexical one", async () => { + test("locks the referent but hands the operation the caller's own path", async () => { + // The mutex key is canonical; the path the caller works on is left alone. On Windows + // realpath can answer with the 8.3 short form, and rewriting the path a caller + // unlinks or compares would change behavior for every caller for no mutex gain. + const callerPath = path.join(STORE, "aliasDir", "task.json") const seen: string[] = [] - await withFileLock(path.join(STORE, "aliasDir", "task.json"), async (absoluteFilePath) => { + await withFileLock(callerPath, async (absoluteFilePath) => { seen.push(absoluteFilePath) }) - expect(seen).toEqual([path.join(STORE, "realDir", "task.json")]) + expect(seen).toEqual([callerPath]) expect(lockfile.lock).toHaveBeenCalledWith( path.join(STORE, "realDir", "task.json"), expect.objectContaining({ realpath: false }), @@ -61,7 +65,7 @@ describe("fileLock - canonical lock keys", () => { }) test("canonicalizes the nearest existing ancestor when the file does not exist yet", async () => { - // The file and its parent are absent, so realpath reports ENOENT; the deepest + // The file and its parent are absent, so realpath reports ENOENT for them; the deepest // existing ancestor is resolved and the missing components are re-appended. const seen: string[] = [] await withFileLock( @@ -71,6 +75,10 @@ describe("fileLock - canonical lock keys", () => { }, ) - expect(seen).toEqual([path.join(STORE, "realDir", "missing", "new.json")]) + expect(seen).toEqual([path.join(STORE, "aliasDir", "missing", "new.json")]) + expect(lockfile.lock).toHaveBeenCalledWith( + path.join(STORE, "realDir", "missing", "new.json"), + expect.objectContaining({ realpath: false }), + ) }) }) diff --git a/src/utils/fileLock.ts b/src/utils/fileLock.ts index 79068cf382..6f63c157a5 100644 --- a/src/utils/fileLock.ts +++ b/src/utils/fileLock.ts @@ -88,21 +88,25 @@ export async function withFileLock( filePath: string, operation: (absoluteFilePath: string) => Promise, ): Promise { - // One canonical path for the lock AND for the operation: the callback must not - // unlink a lexical path while the lock was taken on the referent. - const absoluteFilePath = await canonicalLockPath(filePath) - const releaseLock = await acquireFileLock(absoluteFilePath) + // The LOCK key is canonical, so a caller that reaches the file through a symlinked + // directory and one that reaches it lexically contend for the same lock file. The + // operation still receives the caller's own absolute path: on Windows realpath can + // answer with the 8.3 short form (C:\Users\RUNNER~1\... for a temp dir under + // C:\Users\runneradmin\...), and rewriting the path a caller unlinks or compares + // would change behavior for every caller while adding nothing to the mutex. + const operationPath = path.resolve(filePath) + const releaseLock = await acquireFileLock(operationPath) let result: T try { - result = await operation(absoluteFilePath) + result = await operation(operationPath) } catch (operationError) { // The operation error is the primary failure. Release without // reporting a secondary release error over it. try { await releaseLock() } catch (releaseError) { - console.error(`Failed to release lock for ${absoluteFilePath}:`, releaseError) + console.error(`Failed to release lock for ${operationPath}:`, releaseError) } throw operationError } @@ -112,7 +116,7 @@ export async function withFileLock( } catch (releaseError) { // The operation already succeeded, so a release failure is only // logged, matching how `safeWriteJson` handles release failures. - console.error(`Failed to release lock for ${absoluteFilePath}:`, releaseError) + console.error(`Failed to release lock for ${operationPath}:`, releaseError) } return result } From 0c88df6985277cf3c3b2d932a0b7e9ff9f2e1de1 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 19:38:21 +0800 Subject: [PATCH 17/19] fix(file-safety): retry transient Windows commit-rename failures saveDirectly now awaits safeWriteText, so a commit rename that fails because another process holds the destination (an indexer, an antivirus scan, an editor that opened the file without FILE_SHARE_DELETE) now surfaces as a failed save. On Windows fs.rename is MoveFileExW with MOVEFILE_REPLACE_EXISTING, and those handles are released on a millisecond scale. The commit rename is retried up to 4 times with 25/75/150ms backoff on win32 only, and only for EPERM/EACCES/EBUSY; other codes and POSIX renames fail immediately as before. Tests: EPERM then success commits on the next attempt; EBUSY exhausts the 4 attempts and still cleans up the staging file; EXDEV is not retried; POSIX does not retry. Controls: attempts pinned to 1 fails 2 tests, retrying any error fails 2, retrying on POSIX fails 1. --- .../__tests__/safeWriteText.spec.ts | 55 +++++++++++++++++++ src/services/file-safety/safeWriteText.ts | 34 +++++++++++- 2 files changed, 88 insertions(+), 1 deletion(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 608d532552..bf4447f7fe 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -681,6 +681,61 @@ describe("safeWriteText", () => { }) }) + // ── Windows commit-rename retry ────────────────────────────────────────── + + describe("commit rename retry on Windows", () => { + it("retries a sharing-violation rename and commits on the next attempt", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // An indexer or antivirus scan that holds the destination without FILE_SHARE_DELETE + // makes MoveFileExW fail with EPERM; the handle is released within milliseconds. + vi.mocked(fs.rename).mockRejectedValueOnce(Object.assign(new Error("EPERM"), { code: "EPERM" })) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + expect(fs.rename).toHaveBeenCalledTimes(2) + expect(fs.rename).toHaveBeenLastCalledWith( + expect.stringContaining("safeWriteText_"), + targetPath, + ) + }) + + it("gives up after the bounded attempts and surfaces the error", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fs.rename).mockRejectedValue(Object.assign(new Error("EBUSY"), { code: "EBUSY" })) + + await expect(safeWriteText(targetPath, "data", { platform: "win32" })).rejects.toThrow("EBUSY") + + expect(fs.rename).toHaveBeenCalledTimes(4) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("does not retry a rename failure that cannot be transient", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fs.rename).mockRejectedValue(Object.assign(new Error("EXDEV"), { code: "EXDEV" })) + + await expect(safeWriteText(targetPath, "data", { platform: "win32" })).rejects.toThrow("EXDEV") + + expect(fs.rename).toHaveBeenCalledTimes(1) + }) + + it("does not retry on POSIX, where rename does not fail on a held handle", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fs.rename).mockRejectedValue(Object.assign(new Error("EPERM"), { code: "EPERM" })) + + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toThrow("EPERM") + + expect(fs.rename).toHaveBeenCalledTimes(1) + }) + }) + // ── Test 8: review fixes (permissions, partial writes, resolution, durability) ── describe("review fixes", () => { diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index d1715e1028..9ebb868d08 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -201,6 +201,38 @@ export async function resolvePublishTarget(absoluteFilePath: string): Promise { + const attempts = platform === "win32" ? COMMIT_RENAME_ATTEMPTS : 1 + for (let attempt = 0; ; attempt++) { + try { + await fs.rename(tempPath, targetPath) + return + } catch (error) { + if (attempt + 1 >= attempts || !_isTransientRenameError(error)) throw error + await new Promise((resolve) => setTimeout(resolve, COMMIT_RENAME_RETRY_DELAYS_MS[attempt])) + } + } +} + export async function safeWriteText(filePath: string, content: string, options?: SafeWriteTextOptions): Promise { const absoluteFilePath = path.resolve(filePath) @@ -328,7 +360,7 @@ export async function safeWriteText(filePath: string, content: string, options?: } // -- Step 4: atomic rename temp -> target --------------------- - await fs.rename(tempPath, targetPath) + await _commitRename(tempPath, targetPath, platform) // -- Step 4b (POSIX): fsync the parent directory so the directory entry // changed by the commit rename is durable, not just the file content. From 59bb20a41cbeb0ed1a508d7fbdb67ba426676250 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Wed, 7 Oct 2026 19:54:23 +0800 Subject: [PATCH 18/19] fix(mcp,editor): confine by source, retry the backup rename, restore spies Four review findings: - McpHub confined every settings write by PATH containment. The global settings file is documented as unconfined, but a user who opens their home directory as the workspace has the extension global storage inside the workspace root, so a symlinked mcp_settings.json (a dotfiles checkout) started failing with ConfinedPathEscapeError on a toggle, timeout or delete. The decision moves to services/mcp/mcpWriteScope.ts and is made by SOURCE: only a project write is confined. - The Windows rename retry now covers the backup rename too. With backup:true the first rename is target -> backup, where the held handle is the source, and safeWriteJson always writes with backup:true, so JSON persistence still failed on Windows with the retry on the commit rename alone. ENOENT stays non-transient, so the no-target-yet path is unchanged. - Two DiffViewProvider specs restore their spies in finally, so a failed assertion cannot leak a win32 process.platform getter or a mocked console.warn into later tests. Tests: confinedWriteScope spec (project inside, global inside a home-directory workspace, project outside, ..-prefixed sibling vs traversal); backup retry asserts the three renames. Controls: path-only confinement fails the global case, dropping the backup retry fails its test. --- .../editor/__tests__/DiffViewProvider.spec.ts | 79 +++++++++++-------- .../__tests__/safeWriteText.spec.ts | 18 +++++ src/services/file-safety/safeWriteText.ts | 11 ++- src/services/mcp/McpHub.ts | 46 ++++++----- .../mcp/__tests__/mcpWriteScope.spec.ts | 39 +++++++++ src/services/mcp/mcpWriteScope.ts | 30 +++++++ 6 files changed, 164 insertions(+), 59 deletions(-) create mode 100644 src/services/mcp/__tests__/mcpWriteScope.spec.ts create mode 100644 src/services/mcp/mcpWriteScope.ts diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 167e5c32b5..3a881cf30e 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -960,26 +960,31 @@ describe("DiffViewProvider", () => { it("attributes diagnostics to the saved file when the URI casing differs (Windows)", async () => { const platformSpy = vi.spyOn(process, "platform", "get").mockReturnValue("win32") - vi.mocked(vscode.languages.getDiagnostics).mockClear() - - // The diagnostic URI uses different casing than the saved relPath: - // on Windows this is still the same file (arePathsEqual). - const newDiag: vscode.Diagnostic = { - severity: vscode.DiagnosticSeverity.Error, - range: new vscode.Range(0, 0, 0, 1), - message: "case-mismatch-problem", - } - const postDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [[makeUri(`${mockCwd}/Test.ts`), [newDiag]]] - vi.mocked(vscode.languages.getDiagnostics).mockReturnValueOnce([]).mockReturnValue(postDiagnostics) + try { + vi.mocked(vscode.languages.getDiagnostics).mockClear() + + // The diagnostic URI uses different casing than the saved relPath: + // on Windows this is still the same file (arePathsEqual). + const newDiag: vscode.Diagnostic = { + severity: vscode.DiagnosticSeverity.Error, + range: new vscode.Range(0, 0, 0, 1), + message: "case-mismatch-problem", + } + const postDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [[makeUri(`${mockCwd}/Test.ts`), [newDiag]]] + vi.mocked(vscode.languages.getDiagnostics).mockReturnValueOnce([]).mockReturnValue(postDiagnostics) - await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 100) + await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 100) - // Flush the fire-and-forget tail. - await new Promise((resolve) => setTimeout(resolve, 0)) + // Flush the fire-and-forget tail. + await new Promise((resolve) => setTimeout(resolve, 0)) - expect(mockTask.say).toHaveBeenCalledTimes(1) - expect(mockTask.say.mock.calls[0]?.[1]).toContain("case-mismatch-problem") - platformSpy.mockRestore() + expect(mockTask.say).toHaveBeenCalledTimes(1) + expect(mockTask.say.mock.calls[0]?.[1]).toContain("case-mismatch-problem") + } finally { + // Restore even when an assertion above fails: a leaked process.platform getter would + // make later tests see win32 (arePathsEqual and friends are platform sensitive). + platformSpy.mockRestore() + } }) it("does not block the save on a diagnostics settle delay when diagnostics are disabled", async () => { @@ -1049,28 +1054,32 @@ describe("DiffViewProvider", () => { it("logs a warning instead of an unhandled rejection when the post-save say() is aborted", async () => { const consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) - vi.mocked(vscode.languages.getDiagnostics).mockClear() - - // One new Error-severity problem so the tail reaches say(). - const newDiag: vscode.Diagnostic = { - severity: vscode.DiagnosticSeverity.Error, - range: new vscode.Range(0, 0, 0, 1), - message: "boom", - } - const postDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [[makeUri(`${mockCwd}/test.ts`), [newDiag]]] - vi.mocked(vscode.languages.getDiagnostics).mockReturnValueOnce([]).mockReturnValue(postDiagnostics) + try { + vi.mocked(vscode.languages.getDiagnostics).mockClear() - // The task is aborted while the diagnostics tail is emitting: say() rejects. - mockTask.say.mockRejectedValueOnce(new Error("aborted")) + // One new Error-severity problem so the tail reaches say(). + const newDiag: vscode.Diagnostic = { + severity: vscode.DiagnosticSeverity.Error, + range: new vscode.Range(0, 0, 0, 1), + message: "boom", + } + const postDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [[makeUri(`${mockCwd}/test.ts`), [newDiag]]] + vi.mocked(vscode.languages.getDiagnostics).mockReturnValueOnce([]).mockReturnValue(postDiagnostics) - await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 0) - // Flush the fire-and-forget tail. - await new Promise((resolve) => setTimeout(resolve, 0)) + // The task is aborted while the diagnostics tail is emitting: say() rejects. + mockTask.say.mockRejectedValueOnce(new Error("aborted")) - // The method's outer catch swallows the rejection with a warning. - expect(consoleWarnSpy).toHaveBeenCalledWith(expect.stringContaining("Post-save diagnostics emit failed:")) + await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 0) + // Flush the fire-and-forget tail. + await new Promise((resolve) => setTimeout(resolve, 0)) - consoleWarnSpy.mockRestore() + // The method's outer catch swallows the rejection with a warning. + expect(consoleWarnSpy).toHaveBeenCalledWith(expect.stringContaining("Post-save diagnostics emit failed:")) + } finally { + // Restore even when an assertion above fails, so the mocked console.warn does not + // leak into later tests. + consoleWarnSpy.mockRestore() + } }) }) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index bf4447f7fe..c01493ca5b 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -734,6 +734,24 @@ describe("safeWriteText", () => { expect(fs.rename).toHaveBeenCalledTimes(1) }) + + it("retries the backup rename too, so a held target does not fail a JSON write", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // With backup:true the first rename is target -> backup, where the held handle is the + // SOURCE. safeWriteJson always writes with backup:true, so without this retry every + // JSON persistence write still fails on Windows when the target is open elsewhere. + vi.mocked(fs.rename).mockRejectedValueOnce(Object.assign(new Error("EPERM"), { code: "EPERM" })) + + await safeWriteText(targetPath, "data", { backup: true, platform: "win32" }) + + // attempt 1 (backup) failed, attempt 2 (backup) committed, then the commit rename. + expect(fs.rename).toHaveBeenCalledTimes(3) + expect(fs.rename).toHaveBeenNthCalledWith(1, targetPath, expect.stringContaining("safeWriteText.bak_")) + expect(fs.rename).toHaveBeenNthCalledWith(2, targetPath, expect.stringContaining("safeWriteText.bak_")) + expect(fs.rename).toHaveBeenNthCalledWith(3, expect.stringContaining("safeWriteText_"), targetPath) + }) }) // ── Test 8: review fixes (permissions, partial writes, resolution, durability) ── diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index 9ebb868d08..f313d20f0c 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -220,11 +220,11 @@ function _isTransientRenameError(error: unknown): boolean { return code === "EPERM" || code === "EACCES" || code === "EBUSY" } -async function _commitRename(tempPath: string, targetPath: string, platform: string): Promise { +async function _renameWithRetry(source: string, destination: string, platform: string): Promise { const attempts = platform === "win32" ? COMMIT_RENAME_ATTEMPTS : 1 for (let attempt = 0; ; attempt++) { try { - await fs.rename(tempPath, targetPath) + await fs.rename(source, destination) return } catch (error) { if (attempt + 1 >= attempts || !_isTransientRenameError(error)) throw error @@ -348,7 +348,10 @@ export async function safeWriteText(filePath: string, content: string, options?: try { await fs.access(targetPath) backupPath = _tempName(dirPath, "safeWriteText.bak") - await fs.rename(targetPath, backupPath) + // The backup rename can hit the same sharing violation as the commit rename: the + // file being held open is the SOURCE here, and MoveFileExW fails the same way. + // ENOENT is not transient, so the "no target yet" handling below still applies. + await _renameWithRetry(targetPath, backupPath, platform) releaseBackupOnSuccess = true } catch (err: unknown) { const code = @@ -360,7 +363,7 @@ export async function safeWriteText(filePath: string, content: string, options?: } // -- Step 4: atomic rename temp -> target --------------------- - await _commitRename(tempPath, targetPath, platform) + await _renameWithRetry(tempPath, targetPath, platform) // -- Step 4b (POSIX): fsync the parent directory so the directory entry // changed by the commit rename is durable, not just the file content. diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index d3c1da0bef..ccd6ec9d9a 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -39,6 +39,7 @@ import { fileExistsAtPath } from "../../utils/fs" import { TOKEN_EXPIRY_BUFFER_MS, OAUTH_FLOW_TIMEOUT_MS } from "./constants" import { SecretStorageService } from "./SecretStorageService" import { McpOAuthClientProvider } from "./McpOAuthClientProvider" +import { confinedWriteScope } from "./mcpWriteScope" import { arePathsEqual, getWorkspacePath } from "../../utils/path" import { injectVariables } from "../../utils/config" import { safeWriteJson } from "../../utils/safeWriteJson" @@ -622,25 +623,21 @@ export class McpHub { // Get project-level MCP configuration path /** - * Scope a settings write to the workspace when the caller picked the path from the - * workspace (the project .roo/mcp.json). safeWriteJson resolves the publish target - * through symlinks so the lock, the merge read and the commit rename all key off - * the same file - which means a repository that plants that file as a link to - * somewhere else would otherwise receive the settings write at the linked path. - * The global settings file lives under the extension's global storage rather than - * the workspace, so it is deliberately left unconfined. + * Scope a settings write to the workspace when the write targets the project + * .roo/mcp.json. safeWriteJson resolves the publish target through symlinks so the + * lock, the merge read and the commit rename all key off the same file - which means + * a repository that plants that file as a link to somewhere else would otherwise + * receive the settings write at the linked path. + * + * The source decides, not path containment: the global settings file lives under the + * extension's global storage and is deliberately left unconfined, and a user who + * opens their home directory as the workspace has that storage inside the workspace + * root, so confining by path alone would reject a legitimate symlinked + * mcp_settings.json. */ - private confineToWorkspace(configPath: string): string | undefined { + private confineForSource(source: "global" | "project", configPath: string): string | undefined { const workspaceRoot = path.resolve(this.providerRef.deref()?.cwd ?? getWorkspacePath()) - const relative = path.relative(workspaceRoot, path.resolve(configPath)) - // Separator-aware: a sibling entry whose name merely starts with two dots - // ("..roo/mcp.json") is outside the workspace, while a directory named "..x" - // inside it is not. Comparing the prefix without the separator gets that wrong. - return ( - relative !== "" && relative !== ".." && !relative.startsWith(".." + path.sep) && !path.isAbsolute(relative) - ? workspaceRoot - : undefined - ) + return confinedWriteScope(source, configPath, workspaceRoot) } private async getProjectMcpPath(): Promise { @@ -2113,7 +2110,10 @@ export class McpHub { } this.isProgrammaticUpdate = true try { - await safeWriteJson(configPath, updatedConfig, { prettyPrint: true, confineTo: this.confineToWorkspace(configPath) }) + await safeWriteJson(configPath, updatedConfig, { + prettyPrint: true, + confineTo: this.confineForSource(source, configPath), + }) } finally { // Reset flag after watcher debounce period (non-blocking) this.flagResetTimer = setTimeout(() => { @@ -2198,7 +2198,10 @@ export class McpHub { mcpServers: config.mcpServers, } - await safeWriteJson(configPath, updatedConfig, { prettyPrint: true, confineTo: this.confineToWorkspace(configPath) }) + await safeWriteJson(configPath, updatedConfig, { + prettyPrint: true, + confineTo: this.confineForSource(serverSource, configPath), + }) // Update server connections with the correct source await this.updateServerConnections(config.mcpServers, serverSource) @@ -2407,7 +2410,10 @@ export class McpHub { } this.isProgrammaticUpdate = true try { - await safeWriteJson(normalizedPath, config, { prettyPrint: true, confineTo: this.confineToWorkspace(normalizedPath) }) + await safeWriteJson(normalizedPath, config, { + prettyPrint: true, + confineTo: this.confineForSource(source, normalizedPath), + }) } finally { // Reset flag after watcher debounce period (non-blocking) this.flagResetTimer = setTimeout(() => { diff --git a/src/services/mcp/__tests__/mcpWriteScope.spec.ts b/src/services/mcp/__tests__/mcpWriteScope.spec.ts new file mode 100644 index 0000000000..ff2f600b8a --- /dev/null +++ b/src/services/mcp/__tests__/mcpWriteScope.spec.ts @@ -0,0 +1,39 @@ +// npx vitest run services/mcp/__tests__/mcpWriteScope.spec.ts + +import * as path from "path" + +import { confinedWriteScope } from "../mcpWriteScope" + +describe("confinedWriteScope", () => { + const workspaceRoot = path.resolve("/home/user/project") + + it("confines a project settings file inside the workspace", () => { + const configPath = path.join(workspaceRoot, ".roo", "mcp.json") + + expect(confinedWriteScope("project", configPath, workspaceRoot)).toBe(workspaceRoot) + }) + + it("leaves a global settings file unconfined even when the workspace contains it", () => { + // The regression: a user who opens their home directory as the workspace has the + // extension's global storage under the workspace root. Deciding by path containment + // alone confined the global write, and a symlinked mcp_settings.json (a dotfiles + // checkout) then failed with ConfinedPathEscapeError on a toggle or a delete. + const globalConfig = path.join("/home/user", ".config", "Code", "User", "globalStorage", "roo", "mcp_settings.json") + + expect(confinedWriteScope("global", globalConfig, path.resolve("/home/user"))).toBeUndefined() + }) + + it("does not confine a project settings file that resolves outside the workspace", () => { + const escaped = path.join(workspaceRoot, "..", "elsewhere", "mcp.json") + + expect(confinedWriteScope("project", escaped, workspaceRoot)).toBeUndefined() + }) + + it("does not confuse a sibling named with leading dots with a parent traversal", () => { + // A sibling of the workspace whose name starts with two dots is outside it, and + // must not be treated as inside just because the name shares the ".." prefix. + expect(confinedWriteScope("project", path.join(workspaceRoot, "..", "..roo", "mcp.json"), workspaceRoot)).toBeUndefined() + // A directory NAMED "..x" inside the workspace is not a traversal. + expect(confinedWriteScope("project", path.join(workspaceRoot, "..x", "mcp.json"), workspaceRoot)).toBe(workspaceRoot) + }) +}) diff --git a/src/services/mcp/mcpWriteScope.ts b/src/services/mcp/mcpWriteScope.ts new file mode 100644 index 0000000000..2e8c1cfd02 --- /dev/null +++ b/src/services/mcp/mcpWriteScope.ts @@ -0,0 +1,30 @@ +import * as path from "path" + +/** + * Decide whether an MCP settings write is confined to the workspace. + * + * Only a project-scoped file is confined: the project .roo/mcp.json is content the + * repository controls, so a repository that plants it as a symlink elsewhere must not + * receive the write at the linked path. The global settings file lives under the + * extension's global storage and is deliberately left unconfined - and containment is + * not a substitute for the source, because a user who opens their home directory as the + * workspace has their global storage INSIDE the workspace root. Confining the global + * file by path alone would then reject a legitimate symlinked mcp_settings.json. + */ +export function confinedWriteScope( + source: "global" | "project", + configPath: string, + workspaceRoot: string, +): string | undefined { + if (source !== "project") { + return undefined + } + const root = path.resolve(workspaceRoot) + const relative = path.relative(root, path.resolve(configPath)) + // Separator-aware: a sibling entry whose name merely starts with two dots + // ("..roo/mcp.json") is outside the workspace, while a directory named "..x" + // inside it is not. Comparing the prefix without the separator gets that wrong. + return relative !== "" && relative !== ".." && !relative.startsWith(".." + path.sep) && !path.isAbsolute(relative) + ? root + : undefined +} From 7996380c07c7964a8bb58727ff866dc023836c9b Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Thu, 8 Oct 2026 15:49:45 +0800 Subject: [PATCH 19/19] fix(save): confine the project MCP write, cancel post-save tails, report failed rollbacks Addresses the Pre-merge check items raised against 59bb20a41. Security Boundaries: openProjectMcpSettings created .roo and wrote the placeholder with no confinement. A dangling symlink chain at .roo makes fileExistsAtPath report "absent", so a repository-provided link could create a directory OUTSIDE the workspace and drop mcp.json there. The blind mkdir is gone (safeWriteJson creates the parent only after its confineTo check) and the write now passes confineTo: the workspace folder, matching the three McpHub write sites that already use confineForSource. Lifecycle Resource Cleanup: the post-save diagnostics tail waited on a bare delay(), so Task.dispose() could not cancel it - the timer, the provider and the pre-save diagnostics snapshot survived the teardown and the tail then did stale work. Each tail now registers an AbortController; delay() gets its signal, the tail bails if cancellation lands before or during the wait, and Task.disposeOnce() calls cancelPostSaveDiagnosticsTails(). reset() does NOT cancel: reset follows a successful write and that write's tail still has to report. Persistence Integrity: when the commit rename fails AND the backup rename back also fails, the caller previously saw only the primary rename error while the target was gone and the previous content sat under a randomized backup name. safeWriteText now throws a RollbackFailedError carrying backupPath, the rollback failure as cause and the primary failure as originalError, with both messages in the text so existing handlers still match. Regression Evidence: DiffViewProvider gains an overlapping-saves test proving each tail diffs against its OWN pre-save baseline (a shared last-captured baseline silences both and the assertion count catches it) and a disposal test proving the cancelled tail never reaches the diagnostics query; safeWriteJson's rollback-failure test now asserts the partial-failure surface (name, backupPath, originalError, cause); the ClineProvider openProjectMcpSettings tests assert confineTo is passed and that the handler no longer creates .roo itself. Local: DiffViewProvider 80 passed, ClineProvider 178 passed, safeWriteText + safeWriteJson 62 passed / 9 skipped, Task dispose 9 passed; eslint clean on all seven files. --- src/core/task/Task.ts | 9 +++ .../webview/__tests__/ClineProvider.spec.ts | 20 +++--- src/core/webview/webviewMessageHandler.ts | 10 ++- src/integrations/editor/DiffViewProvider.ts | 43 ++++++++++- .../editor/__tests__/DiffViewProvider.spec.ts | 71 +++++++++++++++++-- src/services/file-safety/safeWriteText.ts | 52 +++++++++++++- src/utils/__tests__/safeWriteJson.test.ts | 20 +++++- 7 files changed, 204 insertions(+), 21 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 4de2b84590..8da6bb62a6 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -3345,6 +3345,15 @@ export class Task extends EventEmitter implements TaskLike { // could still build tools and call `createMessage()`. this.abort = true + // Stop post-save diagnostics tails that are still waiting on their delay. A + // disposed task cannot receive their say() emit, and without this the timer (and + // the provider + diagnostics snapshot it holds) survives the teardown. + try { + this.diffViewProvider.cancelPostSaveDiagnosticsTails() + } catch (error) { + console.error("Error cancelling post-save diagnostics tails:", error) + } + // Cancel any in-progress HTTP request try { this.cancelCurrentRequest() diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 57fe347acf..507edbb15a 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -3696,12 +3696,17 @@ describe("Project MCP Settings", () => { const expectedRooDir = path.join("/test/workspace", ".roo") const expectedMcpPath = path.join(expectedRooDir, "mcp.json") - // Check that fs.mkdir was called with the correct path - expect(mockedFs.mkdir).toHaveBeenCalledWith(expectedRooDir, { recursive: true }) + // The handler must not create .roo itself: a dangling symlink there resolves + // outside the workspace, and creating that parent is exactly what confinement is + // supposed to prevent. safeWriteJson creates it, but only after its scope check. + expect(mockedFs.mkdir).not.toHaveBeenCalledWith(expectedRooDir, { recursive: true }) expect(pathUtils.getWorkspacePath).toHaveBeenCalled() - // Verify file was created with default content - expect(safeWriteJson).toHaveBeenCalledWith(expectedMcpPath, { mcpServers: {} }, { prettyPrint: true }) + // The project-scoped write carries the workspace root as its confinement scope. + expect(safeWriteJson).toHaveBeenCalledWith(expectedMcpPath, { mcpServers: {} }, { + prettyPrint: true, + confineTo: "/test/workspace", + }) // Check that openFile was called expect(openFileSpy).toHaveBeenCalledWith(expectedMcpPath) @@ -3730,10 +3735,9 @@ describe("Project MCP Settings", () => { const pathUtils = await import("../../../utils/path") vi.mocked(pathUtils.getWorkspacePath).mockReturnValue("/test/workspace") - // Mock fs functions to fail - const fs = await import("fs/promises") - const mockedFs = vi.mocked(fs) - mockedFs.mkdir.mockRejectedValue(new Error("Failed to create directory")) + // The handler no longer creates the directory itself, so the failure has to come + // from the confined write. + vi.mocked(safeWriteJson).mockRejectedValueOnce(new Error("Failed to create directory")) // Trigger openProjectMcpSettings await messageHandler({ diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 4ba94d454c..4a7ba7e35b 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -1791,11 +1791,17 @@ export const webviewMessageHandler = async ( const mcpPath = path.join(rooDir, "mcp.json") try { - await fs.mkdir(rooDir, { recursive: true }) + // .roo/mcp.json is a path the repository controls, so it must be confined to the + // workspace before anything is created. A dangling symlink chain at .roo makes + // fileExistsAtPath report "absent" and lets a plain write create the parent + // directory OUTSIDE the workspace and drop the placeholder there. safeWriteJson + // resolves the publish target, checks confineTo and only then creates the parent + // directory, so the mkdir that used to run first is deliberately gone: nothing is + // created until the scope check has passed. const exists = await fileExistsAtPath(mcpPath) if (!exists) { - await safeWriteJson(mcpPath, { mcpServers: {} }, { prettyPrint: true }) + await safeWriteJson(mcpPath, { mcpServers: {} }, { prettyPrint: true, confineTo: workspaceFolder }) } await openFile(mcpPath) diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 386de8883c..ed5d38b22f 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -47,6 +47,10 @@ export class DiffViewProvider { private streamedLines: string[] = [] private preDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [] private preEditScrollLine: number | undefined + // One controller per post-save diagnostics tail that is still waiting, so task + // disposal can cancel the wait instead of leaving a timer (and this provider and + // the pre-save diagnostics snapshot it closes over) running past the teardown. + private readonly postSaveTails = new Set() // Tracks whether the user activated the target file's editor tab during the // diff session. When the file was not already open before the edit, we only // keep it open afterward if the user explicitly interacted with it. @@ -1214,19 +1218,38 @@ export class DiffViewProvider { // "error" carries no task-failure semantics in core). Abort-safe: say() // rejects when the task is aborted, so the whole body sits inside a // try/catch that degrades to a console.warn — the tail can never reject. + // The wait itself is registered in postSaveTails so Task disposal can cancel + // it: an unregistered delay keeps the timer, this provider and the pre-save + // diagnostics snapshot alive past disposal, and the tail then does stale + // diagnostics work against a task that is already gone. private async emitPostSaveDiagnostics( relPath: string, writeDelayMs: number, preDiagnostics: [vscode.Uri, vscode.Diagnostic[]][], inMemoryDocument = false, ): Promise { + const controller = new AbortController() + this.postSaveTails.add(controller) try { // Add configurable delay to allow linters time to process. When the // document was opened in memory (openFile=false), the tail also // carries the 100 ms diagnostics-settle wait that used to block - // saveDirectly. delay() never rejects, so no catch is required here. + // saveDirectly. The signal is the disposal hook: delay() rejects with + // AbortError once the task is gone, which is the tail's expected end. const safeDelayMs = Math.max(0, writeDelayMs) + (inMemoryDocument ? 100 : 0) - await delay(safeDelayMs) + try { + await delay(safeDelayMs, { signal: controller.signal }) + } catch (error) { + if (controller.signal.aborted) { + return + } + throw error + } + // A cancellation can also land between the wait resolving and the work + // starting; either way nothing is queried or emitted after it. + if (controller.signal.aborted) { + return + } // Filter to the saved file: saveDirectly resolves before this tail // completes, so in a multi-file write sequence (e.g. apply_patch) @@ -1260,5 +1283,21 @@ export class DiffViewProvider { // unhandled rejection (say() rejects when the task is aborted). console.warn(`Post-save diagnostics emit failed: ${error}`) } + finally { + this.postSaveTails.delete(controller) + } + } + + /** + * Cancel post-save diagnostics tails that are still waiting. Called when the + * owning task is disposed. Deliberately NOT called from reset(): reset follows a + * successful write, and the tail that write started still has to report the + * problems it is waiting for. + */ + public cancelPostSaveDiagnosticsTails(): void { + for (const controller of this.postSaveTails) { + controller.abort() + } + this.postSaveTails.clear() } } diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 3a881cf30e..a4dab1bf07 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -825,7 +825,7 @@ describe("DiffViewProvider", () => { await new Promise((resolve) => setTimeout(resolve, 0)) // Verify the tail applied the configured write delay - expect(mockDelay).toHaveBeenCalledWith(2000) + expect(mockDelay).toHaveBeenCalledWith(2000, expect.objectContaining({ signal: expect.any(AbortSignal) })) expect(vscode.languages.getDiagnostics).toHaveBeenCalled() // Verify result: L1 no longer returns a problems message @@ -876,7 +876,7 @@ describe("DiffViewProvider", () => { await new Promise((resolve) => setTimeout(resolve, 0)) // Verify delay was called with 0 (safe minimum) - expect(mockDelay).toHaveBeenCalledWith(0) + expect(mockDelay).toHaveBeenCalledWith(0, expect.objectContaining({ signal: expect.any(AbortSignal) })) }) it("should store results for formatFileWriteResponse", async () => { @@ -917,7 +917,7 @@ describe("DiffViewProvider", () => { // Flush the fire-and-forget tail (the mocked delay resolves immediately). await new Promise((resolve) => setTimeout(resolve, 0)) - expect(mockDelay).toHaveBeenCalledWith(100) + expect(mockDelay).toHaveBeenCalledWith(100, expect.objectContaining({ signal: expect.any(AbortSignal) })) expect(mockTask.say).toHaveBeenCalledTimes(1) // The existing "error" ClineSay type is used, with the new-problems text. expect(mockTask.say).toHaveBeenCalledWith( @@ -956,6 +956,69 @@ describe("DiffViewProvider", () => { expect(mockTask.say).toHaveBeenCalledTimes(1) expect(mockTask.say.mock.calls[0]?.[1]).toContain("own-problem") expect(mockTask.say.mock.calls[0]?.[1]).not.toContain("other-file-problem") + + }) + it("gives each post-save tail its own pre-save baseline when two saves overlap", async () => { + const mockDelay = vi.mocked(delay) + mockDelay.mockClear() + vi.mocked(vscode.languages.getDiagnostics).mockClear() + + const firstProblem: vscode.Diagnostic = { + severity: vscode.DiagnosticSeverity.Error, + range: new vscode.Range(0, 0, 0, 1), + message: "first-save-problem", + } + const withProblem: [vscode.Uri, vscode.Diagnostic[]][] = [[makeUri(`${mockCwd}/test.ts`), [firstProblem]]] + + // Save #1's tail waits past save #2, so #2's baseline is captured while #1's + // tail is still waiting. + mockDelay + .mockImplementationOnce(() => new Promise((resolve) => setTimeout(resolve, 20))) + .mockImplementationOnce(() => Promise.resolve()) + + // getDiagnostics order: baseline#1, baseline#2, post#2, post#1. + vi.mocked(vscode.languages.getDiagnostics) + .mockReturnValueOnce([]) + .mockReturnValueOnce(withProblem) + .mockReturnValueOnce(withProblem) + .mockReturnValueOnce(withProblem) + + await diffViewProvider.saveDirectly("test.ts", "a", true, true, 100) + await diffViewProvider.saveDirectly("test.ts", "b", true, true, 100) + await new Promise((resolve) => setTimeout(resolve, 50)) + + // Tail #1 saw the problem appear after ITS baseline and reports it; tail #2 had + // it already in its own baseline and stays silent. A shared (last-captured) + // baseline would silence both, so the count is the assertion that matters. + expect(mockTask.say).toHaveBeenCalledTimes(1) + expect(mockTask.say.mock.calls[0]?.[1]).toContain("first-save-problem") + }) + + it("stops a waiting post-save tail when the task is disposed", async () => { + const mockDelay = vi.mocked(delay) + mockDelay.mockClear() + vi.mocked(vscode.languages.getDiagnostics).mockClear() + + // delay() rejects with AbortError once its signal aborts - the disposal hook. + // Once only: the hanging implementation must not leak into the next test. + mockDelay.mockImplementationOnce((_ms: number, opts?: { signal?: AbortSignal }) => { + return new Promise((_resolve, reject) => { + opts?.signal?.addEventListener("abort", () => + reject(Object.assign(new Error("The operation aborted."), { name: "AbortError" })), + ) + }) + }) + + await diffViewProvider.saveDirectly("test.ts", "content", true, true, 5000) + expect(mockTask.say).not.toHaveBeenCalled() + + diffViewProvider.cancelPostSaveDiagnosticsTails() + await new Promise((resolve) => setTimeout(resolve, 0)) + + // Only the pre-save baseline was read: the tail never reached the diagnostics + // query, so nothing is computed or emitted against a disposed task. + expect(vscode.languages.getDiagnostics).toHaveBeenCalledTimes(1) + expect(mockTask.say).not.toHaveBeenCalled() }) it("attributes diagnostics to the saved file when the URI casing differs (Windows)", async () => { @@ -1030,7 +1093,7 @@ describe("DiffViewProvider", () => { // writeDelayMs (100) + the 100 ms in-memory diagnostics settle, both // applied by the tail instead of the save path. - expect(mockDelay).toHaveBeenCalledWith(200) + expect(mockDelay).toHaveBeenCalledWith(200, expect.objectContaining({ signal: expect.any(AbortSignal) })) // Let the fire-and-forget tail run to completion before the test ends: with no // diagnostics the tail must stay silent, and an unfinished tail would leak diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index f313d20f0c..33c4598147 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -233,6 +233,43 @@ async function _renameWithRetry(source: string, destination: string, platform: s } } +/** + * Raised when the commit rename failed AND the backup could not be renamed back onto the + * target: the target is missing and the previous content survives only under the + * randomized backup path. The primary failure is preserved as originalError so callers + * keep the reason the publish failed while also learning where the saved state is. + */ +class RollbackFailedError extends Error { + readonly originalError: unknown + + constructor( + public readonly filePath: string, + public readonly backupPath: string, + rollbackError: unknown, + originalError: unknown, + ) { + super(_rollbackFailureMessage(filePath, backupPath, rollbackError, originalError), { + cause: rollbackError, + }) + this.name = "RollbackFailedError" + this.originalError = originalError + } +} + +function _rollbackFailureMessage( + filePath: string, + backupPath: string, + rollbackError: unknown, + originalError: unknown, +): string { + const primary = originalError instanceof Error ? originalError.message : String(originalError) + const rollback = rollbackError instanceof Error ? rollbackError.message : String(rollbackError) + return ( + `Publish to ${filePath} failed (${primary}) and the backup could not be restored (${rollback}). ` + + `The previous content is still at ${backupPath}.` + ) +} + export async function safeWriteText(filePath: string, content: string, options?: SafeWriteTextOptions): Promise { const absoluteFilePath = path.resolve(filePath) @@ -409,11 +446,18 @@ export async function safeWriteText(filePath: string, content: string, options?: // tempPath is now the committed file; no cleanup needed. } catch (originalError: unknown) { // -- Rollback / cleanup on failure ---------------------------------- + let rollbackFailure: { backupPath: string; error: unknown } | null = null if (backupPath && releaseBackupOnSuccess) { try { await fs.rename(backupPath, targetPath) - } catch { - // rollback failed — do not mask original error + } catch (rollbackError: unknown) { + // The commit failed AND the restore failed: the target is missing and the only + // copy of the previous content is under the randomized backup name. Reporting + // only the primary rename error would leave the caller with no way to find that + // copy, so the rollback failure and the backup path are surfaced too. The + // original failure is kept as originalError (and in the message) so callers that + // branch on it do not lose it. + rollbackFailure = { backupPath, error: rollbackError } } } @@ -433,6 +477,10 @@ export async function safeWriteText(filePath: string, content: string, options?: await fs.unlink(daclDumpPath).catch(() => {}) } + if (rollbackFailure !== null) { + throw new RollbackFailedError(targetPath, rollbackFailure.backupPath, rollbackFailure.error, originalError) + } + throw originalError } } diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 087e30665a..5cc1393bc6 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -708,14 +708,28 @@ describe("safeWriteJson", () => { return fsPromisesActuals.rename!(oldPath, newPath) }) - // The original error must propagate, not the rollback error - await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow("Primary rename failed") + // The original failure must still be the thing the caller can read, even though + // the rollback failure is what gets thrown on top of it. + const rejection = await safeWriteJson(currentTestFilePath, newData).then(() => null, (error) => error) + expect(rejection).toBeInstanceOf(Error) + expect(rejection.name).toBe("RollbackFailedError") + expect(rejection.message).toContain("Primary rename failed") + + // Partial failure has to be actionable: the caller learns the previous content is + // recoverable and where it is, instead of only that a rename failed. + expect(rejection.backupPath).toMatch(/safeWriteText\.bak_/) + expect(String(rejection.message)).toContain("previous content is still at") + expect(rejection.originalError).toBeInstanceOf(Error) + expect((rejection.originalError as Error).message).toBe("Primary rename failed") + expect(rejection.cause).toBeInstanceOf(Error) + expect((rejection.cause as Error).message).toBe("Rollback rename failed") // The rollback failed inside safeWriteText, so the target is gone and - // the backup is orphaned on disk. + // the backup is orphaned on disk - and it is the file the error points at. expect(await fileExists(currentTestFilePath)).toBe(false) const entries = await fs.readdir(tempDir) expect(entries.some((entry) => entry.includes("safeWriteText.bak_"))).toBe(true) + expect(await fileExists(rejection.backupPath)).toBe(true) consoleErrorSpy.mockRestore() })