diff --git a/src/core/config/__tests__/importExport.spec.ts b/src/core/config/__tests__/importExport.spec.ts index 0287b26511..8a03663aee 100644 --- a/src/core/config/__tests__/importExport.spec.ts +++ b/src/core/config/__tests__/importExport.spec.ts @@ -1555,10 +1555,14 @@ describe("importExport", () => { expect(mockContextProxy.export).toHaveBeenCalled() expect(fs.mkdir).toHaveBeenCalledWith("/mock/path", { recursive: true }) - expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/zoo-code-settings.json", { - providerProfiles: mockProviderProfiles, - globalSettings: mockGlobalSettings, - }) + expect(safeWriteJson).toHaveBeenCalledWith( + "/mock/path/zoo-code-settings.json", + { + providerProfiles: mockProviderProfiles, + globalSettings: mockGlobalSettings, + }, + { refuseSymlinkTarget: true }, + ) }) it("should include globalSettings when allowedMaxRequests is null", async () => { @@ -1587,10 +1591,14 @@ describe("importExport", () => { contextProxy: mockContextProxy, }) - expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/zoo-code-settings.json", { - providerProfiles: mockProviderProfiles, - globalSettings: mockGlobalSettings, - }) + expect(safeWriteJson).toHaveBeenCalledWith( + "/mock/path/zoo-code-settings.json", + { + providerProfiles: mockProviderProfiles, + globalSettings: mockGlobalSettings, + }, + { refuseSymlinkTarget: true }, + ) }) it("should handle errors during the export process", async () => { @@ -1710,10 +1718,14 @@ describe("importExport", () => { contextProxy: mockContextProxy, }) - expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/zoo-code-settings.json", { - providerProfiles: mockProviderProfiles, - globalSettings: mockGlobalSettings, - }) + expect(safeWriteJson).toHaveBeenCalledWith( + "/mock/path/zoo-code-settings.json", + { + providerProfiles: mockProviderProfiles, + globalSettings: mockGlobalSettings, + }, + { refuseSymlinkTarget: true }, + ) }) it("should export model dimension for OpenAI Compatible provider", async () => { @@ -1863,10 +1875,14 @@ describe("importExport", () => { }) // Should not throw an error and should preserve original settings - expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/zoo-code-settings.json", { - providerProfiles: mockProviderProfiles, - globalSettings: mockGlobalSettings, // Should remain unchanged - }) + expect(safeWriteJson).toHaveBeenCalledWith( + "/mock/path/zoo-code-settings.json", + { + providerProfiles: mockProviderProfiles, + globalSettings: mockGlobalSettings, // Should remain unchanged + }, + { refuseSymlinkTarget: true }, + ) }) it("should maintain backward compatibility with existing exports", async () => { @@ -1906,10 +1922,14 @@ describe("importExport", () => { }) // Should not modify settings for non-openai-compatible providers - expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/zoo-code-settings.json", { - providerProfiles: mockProviderProfiles, - globalSettings: mockGlobalSettings, // Should remain unchanged - }) + expect(safeWriteJson).toHaveBeenCalledWith( + "/mock/path/zoo-code-settings.json", + { + providerProfiles: mockProviderProfiles, + globalSettings: mockGlobalSettings, // Should remain unchanged + }, + { refuseSymlinkTarget: true }, + ) }) it("should handle missing current provider gracefully", async () => { @@ -1951,10 +1971,14 @@ describe("importExport", () => { }) // Should not throw an error and should preserve original settings - expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/zoo-code-settings.json", { - providerProfiles: mockProviderProfiles, - globalSettings: mockGlobalSettings, // Should remain unchanged - }) + expect(safeWriteJson).toHaveBeenCalledWith( + "/mock/path/zoo-code-settings.json", + { + providerProfiles: mockProviderProfiles, + globalSettings: mockGlobalSettings, // Should remain unchanged + }, + { refuseSymlinkTarget: true }, + ) }) }) @@ -2188,10 +2212,14 @@ describe("importExport", () => { }) // Step 4: Verify the exported data includes the model dimension - expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/test-settings.json", { - providerProfiles: mockProviderProfiles, - globalSettings: mockGlobalSettings, - }) + expect(safeWriteJson).toHaveBeenCalledWith( + "/mock/path/test-settings.json", + { + providerProfiles: mockProviderProfiles, + globalSettings: mockGlobalSettings, + }, + { refuseSymlinkTarget: true }, + ) // Step 5: Get the exported data for import test const exportedData = (safeWriteJson as Mock).mock.calls[0][1] diff --git a/src/core/config/importExport.ts b/src/core/config/importExport.ts index 3c213fedf4..906640a79c 100644 --- a/src/core/config/importExport.ts +++ b/src/core/config/importExport.ts @@ -344,7 +344,7 @@ export const exportSettings = async ({ providerSettingsManager, contextProxy }: const dirname = path.dirname(uri.fsPath) await fs.mkdir(dirname, { recursive: true }) - await safeWriteJson(uri.fsPath, { providerProfiles, globalSettings }) + await safeWriteJson(uri.fsPath, { providerProfiles, globalSettings }, { refuseSymlinkTarget: true }) } catch (e) { console.error("Failed to export settings:", e) // Don't re-throw - the UI will handle showing error messages diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index c4c62f7f30..fe9a02082b 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -112,6 +112,7 @@ import { ToolRepetitionDetector } from "../tools/ToolRepetitionDetector" import { restoreTodoListForTask } from "../tools/UpdateTodoListTool" import { FileContextTracker } from "../context-tracking/FileContextTracker" import { RooIgnoreController } from "../ignore/RooIgnoreController" +import { ObservationRegistry } from "./observationRegistry" import { RooProtectedController } from "../protect/RooProtectedController" import { type AssistantMessageContent, presentAssistantMessage } from "../assistant-message" import { NativeToolCallParser } from "../assistant-message/NativeToolCallParser" @@ -292,6 +293,7 @@ export class Task extends EventEmitter implements TaskLike { readonly parentTask: Task | undefined = undefined readonly taskNumber: number readonly workspacePath: string + readonly observationRegistry = new ObservationRegistry() /** * The mode associated with this task. Persisted across sessions @@ -1409,6 +1411,7 @@ export class Task extends EventEmitter implements TaskLike { /** Cancels the current persistence generation before creating the next assistant-turn boundary. */ private resetAssistantMessagePersistence(): void { this.cancelAssistantMessagePersistence() + this.assistantMessagePersistencePromise = new Promise((resolve) => { this.resolveAssistantMessagePersistence = resolve }) @@ -3445,6 +3448,12 @@ export class Task extends EventEmitter implements TaskLike { console.log(`[Task#dispose] disposing task ${this.taskId}.${this.instanceId}`) this.cancelAssistantMessagePersistence() + // Drop the per-task file observations. The registry holds an entry per absolute + // path the task read or wrote (version token + timestamp); nothing can consume them + // after disposal, and a long-lived extension host would otherwise keep every path a + // finished task touched alive. + this.observationRegistry.close() + // Stop the idle telemetry check and report any unflushed activity as a // shutdown installment, so a task torn down mid-work (panel closed, task // switched, extension deactivated) isn't invisible to telemetry. diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index d6be42f0bd..8c12fa4782 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -1398,6 +1398,71 @@ describe("Cline", () => { }) }) + describe("observation registry lifecycle (S4a, epic #1375)", () => { + it("gives each Task its own observation registry", () => { + const firstTask = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "first observation task", + startTask: false, + }) + const secondTask = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "second observation task", + startTask: false, + }) + + // The guarded-write contract assumes an observation in one task never validates + // a write issued by another task. + expect(firstTask.observationRegistry).not.toBe(secondTask.observationRegistry) + firstTask.observationRegistry.observe("/workspace/a.ts", "v-a") + expect(firstTask.observationRegistry.get("/workspace/a.ts")?.version).toBe("v-a") + expect(secondTask.observationRegistry.get("/workspace/a.ts")).toBeUndefined() + }) + + it("clears the observation registry when the task is disposed", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "disposed observation task", + startTask: false, + }) + task.observationRegistry.observe("/workspace/a.ts", "v-a") + task.observationRegistry.observe("/workspace/b.ts", "v-b") + + await task.dispose() + + // A disposed task cannot serve another guarded write, so its observed paths + // (version token + timestamp each) must not stay reachable for the host lifetime. + expect(task.observationRegistry.get("/workspace/a.ts")).toBeUndefined() + expect(task.observationRegistry.get("/workspace/b.ts")).toBeUndefined() + // Closing is what makes a later observation refuse, so it is asserted rather than + // left to the test name: clearing the map alone would look identical from here. + expect(task.observationRegistry.isClosed).toBe(true) + }) + + it("refuses an observation recorded after the task was disposed", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "late observation task", + startTask: false, + }) + + await task.dispose() + + // A read that was already in flight when the task was disposed can finish late and + // call observe(). Recording then would repopulate a registry no task owns and hand a + // version token to a guarded write that will never happen. + task.observationRegistry.observe("/workspace/late.ts", "v-late") + + expect(task.observationRegistry.get("/workspace/late.ts")).toBeUndefined() + expect(task.observationRegistry.size).toBe(0) + expect(task.observationRegistry.isClosed).toBe(true) + }) + }) + describe("constructor", () => { it.each([{ apiConfigName: "parent-local-profile" }, { apiConfigName: undefined }])( "uses an explicit delegated-child context without shared state or startup persistence", diff --git a/src/core/task/__tests__/observationRegistry.spec.ts b/src/core/task/__tests__/observationRegistry.spec.ts new file mode 100644 index 0000000000..2301247f32 --- /dev/null +++ b/src/core/task/__tests__/observationRegistry.spec.ts @@ -0,0 +1,90 @@ +import { describe, it, expect, vi } from "vitest" + +import { ObservationRegistry } from "../observationRegistry" + +describe("ObservationRegistry", () => { + it("observe → get returns the recorded version and observedAt", () => { + const reg = new ObservationRegistry() + reg.observe("/a/b/c.ts", "1:2:300:4000000000:5000000000") + + const obs = reg.get("/a/b/c.ts") + expect(obs).toBeDefined() + expect(obs!.version).toBe("1:2:300:4000000000:5000000000") + expect(typeof obs!.observedAt).toBe("number") + }) + + it("re-observe replaces the entry with a fresh observedAt", () => { + vi.useFakeTimers() + const reg = new ObservationRegistry() + reg.observe("/a/b/c.ts", "v1") + const first = reg.get("/a/b/c.ts")! + expect(first.version).toBe("v1") + + vi.advanceTimersByTime(50) + reg.observe("/a/b/c.ts", "v2") + const second = reg.get("/a/b/c.ts")! + expect(second.version).toBe("v2") + expect(second.observedAt).toBeGreaterThan(first.observedAt) + + vi.useRealTimers() + }) + + it("has returns true for observed paths, false otherwise", () => { + const reg = new ObservationRegistry() + reg.observe("/x.ts", "t1") + expect(reg.has("/x.ts")).toBe(true) + expect(reg.has("/y.ts")).toBe(false) + }) + + it("size reflects the number of observed entries", () => { + const reg = new ObservationRegistry() + expect(reg.size).toBe(0) + reg.observe("/a.ts", "t1") + reg.observe("/b.ts", "t2") + expect(reg.size).toBe(2) + }) + + it("clear removes all entries and resets size to 0", () => { + const reg = new ObservationRegistry() + reg.observe("/a.ts", "t1") + reg.observe("/b.ts", "t2") + reg.clear() + expect(reg.size).toBe(0) + expect(reg.get("/a.ts")).toBeUndefined() + expect(reg.has("/b.ts")).toBe(false) + }) + + it("get on empty registry returns undefined", () => { + const reg = new ObservationRegistry() + expect(reg.get("/any.ts")).toBeUndefined() + }) + + it("separate instances are independent — observing in one does not appear in the other", () => { + const regA = new ObservationRegistry() + const regB = new ObservationRegistry() + regA.observe("/shared.ts", "v1") + expect(regA.get("/shared.ts")).toBeDefined() + expect(regB.get("/shared.ts")).toBeUndefined() + regB.observe("/shared.ts", "v2") + expect(regA.get("/shared.ts")!.version).toBe("v1") + expect(regB.get("/shared.ts")!.version).toBe("v2") + }) +}) + +describe("close() - disposal is terminal", () => { + it("drops every observation and refuses later ones", () => { + const registry = new ObservationRegistry() + registry.observe("/workspace/a.ts", "v-a") + expect(registry.size).toBe(1) + + registry.close() + + expect(registry.size).toBe(0) + expect(registry.isClosed).toBe(true) + + // A read that was in flight when the task was disposed must not repopulate it. + registry.observe("/workspace/late.ts", "v-late") + expect(registry.get("/workspace/late.ts")).toBeUndefined() + expect(registry.size).toBe(0) + }) +}) diff --git a/src/core/task/observationRegistry.ts b/src/core/task/observationRegistry.ts new file mode 100644 index 0000000000..db0ecfd07e --- /dev/null +++ b/src/core/task/observationRegistry.ts @@ -0,0 +1,75 @@ +/** + * Per-task file observation registry (upstream epic #1375, phase A2). + * + * Each Task owns its own instance so parent and subtask observations are + * independent. The S4 guarded-write will compare these versions against the + * token recomputed pre-write to detect stale reads or file replacement. + * + * Pure in-memory — zero I/O, no dependencies. The observations ARE consulted: + * guardedWrite reads this registry before publishing (src/core/tools/guardedWrite.ts) + * and compares the recorded version token against the token recomputed from disk, so a + * stale read or an out-of-band replacement that the check detects is rejected instead of + * published over. Detection is best effort against a non-cooperating process: the token is + * recomputed before the publish, so a replacement that lands after that check and before + * the rename is not observable from here and can still win. Closing that last window needs + * a cross-process lock or an atomic create, not a token comparison. + */ + +export interface FileObservation { + /** Version token derived from on-disk fs.stat (bigint mode). */ + version: string + /** Millisecond timestamp when the observation was recorded. */ + observedAt: number +} + +export class ObservationRegistry { + private readonly entries = new Map() + + /** Set by close(): after disposal the registry refuses further observations. */ + private closed = false + + /** + * Record an observation for a file at its absolute path, unless the registry is closed. + * + * Re-observing replaces the entry with a fresh observedAt timestamp and + * the new version token. A read that was already in flight can finish after + * Task.disposeOnce() dropped the observations; recording then would hand a version token + * to a task that no longer serves any request, and a later guarded write could consult + * it. close() therefore makes this a no-op, so disposal is terminal at this layer. + */ + observe(absolutePath: string, version: string): void { + if (this.closed) { + return + } + this.entries.set(absolutePath, { version, observedAt: Date.now() }) + } + + get(absolutePath: string): FileObservation | undefined { + return this.entries.get(absolutePath) + } + + has(absolutePath: string): boolean { + return this.entries.has(absolutePath) + } + + clear(): void { + this.entries.clear() + } + + /** + * Drop every observation and refuse any later one. Task.disposeOnce() calls this so a + * disposed task's registry cannot be repopulated by a read that finishes late. + */ + close(): void { + this.closed = true + this.entries.clear() + } + + get isClosed(): boolean { + return this.closed + } + + get size(): number { + return this.entries.size + } +} diff --git a/src/core/tools/ReadFileTool.ts b/src/core/tools/ReadFileTool.ts index 00fc507692..88d54ddc33 100644 --- a/src/core/tools/ReadFileTool.ts +++ b/src/core/tools/ReadFileTool.ts @@ -16,6 +16,7 @@ import type { ReadFileParams, ReadFileMode, ReadFileToolParams, FileEntry, LineR import { isLegacyReadFileParams, type ClineSayTool } from "@roo-code/types" import { Task } from "../task/Task" +import { versionTokenOfStat } from "../../utils/versionToken" import { formatResponse } from "../prompts/responses" import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { isPathOutsideWorkspace } from "../../utils/pathUtils" @@ -214,12 +215,29 @@ export class ReadFileTool extends BaseTool<"read_file"> { // Read text file content with lossy UTF-8 conversion // Reading as Buffer first allows graceful handling of non-UTF8 bytes // (they become U+FFFD replacement characters instead of throwing) + // A2 (epic #1375): capture the on-disk token before the read so a mutation + // landing mid-read is detected by the post-read stat below. + const preReadStats = await fs.stat(fullPath, { bigint: true }).catch(() => undefined) const buffer = await fs.readFile(fullPath) const fileContent = buffer.toString("utf-8") const result = this.processTextFile(fileContent, entry) await task.fileContextTracker.trackFileContext(relPath, "read_tool" as RecordSource) + // A2 (plan #33 / epic #1375): record the observed on-disk version for the future write guard. + // The token is captured before AND after the read; the target is observed only + // when both match — a mutation between the two stats means the content the model + // received is not the on-disk state, and observing it would let a later write + // match a token the model never saw. A stat failure leaves the target + // unobserved and never fails the read. + const postReadStats = await fs.stat(fullPath, { bigint: true }).catch(() => undefined) + if (preReadStats && postReadStats) { + const preReadToken = versionTokenOfStat(preReadStats) + if (preReadToken === versionTokenOfStat(postReadStats)) { + task.observationRegistry.observe(fullPath, preReadToken) + } + } + updateFileResult(relPath, { nativeContent: `File: ${relPath}\n${result}`, }) @@ -768,6 +786,9 @@ export class ReadFileTool extends BaseTool<"read_file"> { } // Read text file + // A2 (epic #1375): capture the on-disk token before the read so a mutation + // landing mid-read is detected by the post-read stat below. + const preReadStats = await fs.stat(fullPath, { bigint: true }).catch(() => undefined) const rawContent = await fs.readFile(fullPath, "utf8") // Handle line ranges if specified @@ -799,6 +820,19 @@ export class ReadFileTool extends BaseTool<"read_file"> { // Track file in context await task.fileContextTracker.trackFileContext(relPath, "read_tool") + + // A2 (plan #33 / epic #1375): mirror the native path — record the observed + // on-disk version so legacy-format reads also feed the future write guard. + // Observe only when the pre-read and post-read tokens match (a mutation between + // them means the returned content is not the on-disk state). A stat failure + // leaves the target unobserved and never fails the read. + const postReadStats = await fs.stat(fullPath, { bigint: true }).catch(() => undefined) + if (preReadStats && postReadStats) { + const preReadToken = versionTokenOfStat(preReadStats) + if (preReadToken === versionTokenOfStat(postReadStats)) { + task.observationRegistry.observe(fullPath, preReadToken) + } + } } catch (error) { const errorMsg = error instanceof Error ? error.message : String(error) results.push(`File: ${relPath}\nError: ${errorMsg}`) diff --git a/src/core/tools/__tests__/guardedWrite.spec.ts b/src/core/tools/__tests__/guardedWrite.spec.ts new file mode 100644 index 0000000000..c501db012f --- /dev/null +++ b/src/core/tools/__tests__/guardedWrite.spec.ts @@ -0,0 +1,776 @@ +/** + * Tests for the guarded-write compare-and-swap core (upstream epic #1375, + * phase A4a). + * + * Covers guard selection through the S2 observation registry, version-token + * CAS, remediation messages, and the per-absolute-path FIFO chain: FIFO + * ordering, exactly-one winner under concurrency, no wedge after a rejected + * link, and independence across paths. It also covers the publication-time + * re-verification that closes the check-to-rename window for writers + * serialized by the chain. + */ + +import * as fs from "fs/promises" +import * as path from "path" + +import { describe, expect, it, beforeEach, vi } from "vitest" + +import { createIfAbsent, guardedWrite, replaceIfVersion, resetChain } from "../guardedWrite" +import { safeWriteText, type SafeWriteTextOptions } from "../../../services/file-safety/safeWriteText" +import { computeVersionToken } from "../../../utils/versionToken" +import { ObservationRegistry } from "../../task/observationRegistry" +import type { Task } from "../../task/Task" + +// -- Mocks ------------------------------------------------------------------- + +vi.mock("fs/promises", () => ({ + access: vi.fn(), + stat: vi.fn(), +})) + +vi.mock("../../../utils/versionToken", () => ({ + computeVersionToken: vi.fn(), +})) + +vi.mock("../../../services/file-safety/safeWriteText", () => ({ + safeWriteText: vi.fn(), +})) + +const mockedFsAccess = vi.mocked(fs.access) +const mockedComputeVersionToken = vi.mocked(computeVersionToken) +const mockedSafeWriteText = vi.mocked(safeWriteText) + +// -- Fixtures ---------------------------------------------------------------- + +const WORKSPACE = "/test/workspace" + +/** Resolve a fixture path the same way guardedWrite resolves task.cwd-relative paths. */ +const abs = (relPath: string): string => path.resolve(WORKSPACE, relPath) + +interface MockTaskOptions { + cwd?: string + observationRegistry?: ObservationRegistry +} + +/** + * Minimal structural Task: guardedWrite only reads task.cwd and + * task.observationRegistry. The real Task constructor needs the full provider + * machinery, so a single documented double cast stands in for the class. + */ +function createMockTask(options: MockTaskOptions = {}): Task { + const task = { + cwd: options.cwd ?? WORKSPACE, + observationRegistry: options.observationRegistry ?? new ObservationRegistry(), + } + return task as unknown as Task +} + +// -- Tests ------------------------------------------------------------------- + +describe("guardedWrite (S4a, epic #1375)", () => { + beforeEach(() => { + vi.resetAllMocks() + resetChain() + }) + + describe("unobserved create", () => { + it("succeeds when the file is absent and publishes via safeWriteText", async () => { + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) + const task = createMockTask() + + await guardedWrite(task, "new-file.txt", "hello", "create") + + expect(mockedSafeWriteText).toHaveBeenCalledTimes(1) + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("new-file.txt"), "hello", { + verifyBeforeCommit: expect.any(Function), + }) + }) + + it("defaults kind to update when the caller omits the argument", async () => { + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) + const task = createMockTask() + + // Every other call in this file passes kind explicitly, so the default was never exercised. + // The default only matters on the UNOBSERVED path: once a file is observed the guard is + // chosen by the observation, not by kind. Omitting it must take the create-if-absent guard + // (which publishes); a default of "edit" would hit the read-first guard and reject instead. + await guardedWrite(task, "new-file.txt", "hello") + + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("new-file.txt"), "hello", { + verifyBeforeCommit: expect.any(Function), + }) + }) + + it("fails with the read-first remediation when the file exists - nothing published", async () => { + mockedFsAccess.mockResolvedValue(undefined) + const task = createMockTask() + + await expect(guardedWrite(task, "existing.txt", "hello", "create")).rejects.toThrow( + "File already exists at " + + abs("existing.txt") + + " and was not read before this write -- read the file first, then retry.", + ) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + + it("rethrows I/O errors that are not ENOENT verbatim (no guard verdict on access failure)", async () => { + const failures = [{ code: "EACCES" }, null, "volume offline", new Error("EIO-ish failure")] + for (const failure of failures) { + mockedFsAccess.mockRejectedValueOnce(failure) + await expect(createIfAbsent(abs("io-error.txt"), "x")).rejects.toBe(failure) + } + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + }) + + describe("deleted-after-read target", () => { + it("normalizes an ENOENT from the version token into the re-read remediation", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("vanished.txt"), "v1") + const task = createMockTask({ observationRegistry: reg }) + + // The file was deleted after the read: the token computation fails + // with a raw ENOENT, which the guard must convert into the standard + // re-read-then-retry contract. + mockedComputeVersionToken.mockRejectedValue({ code: "ENOENT" }) + + await expect(guardedWrite(task, "vanished.txt", "next", "update")).rejects.toThrow( + "File was deleted after it was read", + ) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + + it("rethrows non-ENOENT token failures verbatim from replaceIfVersion", async () => { + const failure = { code: "EACCES" } + mockedComputeVersionToken.mockRejectedValueOnce(failure) + + await expect(replaceIfVersion(abs("locked.txt"), "v1", "next")).rejects.toBe(failure) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + }) + describe("unobserved update", () => { + it("succeeds when the file is absent (same create guard)", async () => { + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) + const task = createMockTask() + + await guardedWrite(task, "new-file.txt", "hello", "update") + + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("new-file.txt"), "hello", { + verifyBeforeCommit: expect.any(Function), + }) + }) + + it("fails with the read-first remediation when the file exists - nothing published", async () => { + mockedFsAccess.mockResolvedValue(undefined) + const task = createMockTask() + + await expect(guardedWrite(task, "existing.txt", "hello", "update")).rejects.toThrow( + "File already exists at " + + abs("existing.txt") + + " and was not read before this write -- read the file first, then retry.", + ) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + }) + + describe("observed create", () => { + it("recreates a file that vanished after the read", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("gone.txt"), "v1") + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) + const task = createMockTask({ observationRegistry: reg }) + + await guardedWrite(task, "gone.txt", "back", "create") + + expect(mockedSafeWriteText).toHaveBeenCalledTimes(1) + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("gone.txt"), "back", { + verifyBeforeCommit: expect.any(Function), + }) + }) + + it("goes through the version guard when the file still exists", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("kept.txt"), "v1") + mockedFsAccess.mockResolvedValue(undefined) + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + await guardedWrite(task, "kept.txt", "rewritten", "create") + + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("kept.txt"), "rewritten", { + verifyBeforeCommit: expect.any(Function), + }) + }) + + it("uses the update guard when the caller omits the argument on an observed file", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("doc.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + // Documents the contract for the omitted argument on the observed path. Note that this case + // is NOT load-bearing: once a file is observed the guard is chosen by the observation, so + // changing the default does not change this call. The load-bearing pin for the default is + // 'defaults kind to update when the caller omits the argument' on the unobserved path. + await guardedWrite(task, "doc.txt", "new content") + + expect(mockedComputeVersionToken).toHaveBeenCalledWith(abs("doc.txt")) + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("doc.txt"), "new content", { + verifyBeforeCommit: expect.any(Function), + }) + }) + + it("fails with the stale remediation suffix when the version moved", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("kept.txt"), "v1") + mockedFsAccess.mockResolvedValue(undefined) + mockedComputeVersionToken.mockResolvedValue("v2") + const task = createMockTask({ observationRegistry: reg }) + + await expect(guardedWrite(task, "kept.txt", "rewritten", "create")).rejects.toThrow( + "Stale version -- the file changed since you read it (expected v1, current v2); re-read the file, then retry.", + ) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + + it("defers to the version guard when the access check is denied (not ENOENT)", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("locked.txt"), "v1") + mockedFsAccess.mockRejectedValue({ code: "EACCES" }) + mockedComputeVersionToken.mockResolvedValue("v2") + const task = createMockTask({ observationRegistry: reg }) + + await expect(guardedWrite(task, "locked.txt", "rewritten", "create")).rejects.toThrow( + "Stale version -- the file changed since you read it (expected v1, current v2); re-read the file, then retry.", + ) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + }) + + describe("observed update (version CAS)", () => { + it("publishes when the on-disk version matches the observation", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("doc.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + await guardedWrite(task, "doc.txt", "new content", "update") + + expect(mockedComputeVersionToken).toHaveBeenCalledWith(abs("doc.txt")) + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("doc.txt"), "new content", { + verifyBeforeCommit: expect.any(Function), + }) + }) + + it("fails with the stale remediation suffix when the version moved - nothing published", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("doc.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v2") + const task = createMockTask({ observationRegistry: reg }) + + await expect(guardedWrite(task, "doc.txt", "new content", "update")).rejects.toThrow( + "Stale version -- the file changed since you read it (expected v1, current v2); re-read the file, then retry.", + ) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + + it("does not probe existence or recreate when an updated file vanished after the read", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("doc.txt"), "v1") + const task = createMockTask({ observationRegistry: reg }) + // The file vanished: the CAS token recomputation rejects with ENOENT, + // which the version guard reports as the deleted-file remediation. + mockedComputeVersionToken.mockRejectedValue({ code: "ENOENT" }) + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) + + await expect(guardedWrite(task, "doc.txt", "new content", "update")).rejects.toThrow( + "File was deleted after it was read -- the version recorded at read time (v1) no longer exists; re-read the file, then retry.", + ) + + // The existence probe is a "create"-only step: an "update" must not run + // it (the branch condition short-circuits on kind) and must not recreate + // the vanished file - only the CAS branch may decide the write. + expect(mockedFsAccess).not.toHaveBeenCalled() + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + }) + + describe("edit", () => { + it("fails read-first when the file was never observed - nothing published, no I/O", async () => { + const task = createMockTask() + + await expect(guardedWrite(task, "any.txt", "patched", "edit")).rejects.toThrow( + "File not read yet -- read the file, then retry.", + ) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + expect(mockedComputeVersionToken).not.toHaveBeenCalled() + expect(mockedFsAccess).not.toHaveBeenCalled() + }) + + it("publishes when the version matches the observation", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("doc.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + await guardedWrite(task, "doc.txt", "patched", "edit") + + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("doc.txt"), "patched", { + verifyBeforeCommit: expect.any(Function), + }) + }) + + it("fails with the stale remediation suffix when the version moved", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("doc.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v3") + const task = createMockTask({ observationRegistry: reg }) + + await expect(guardedWrite(task, "doc.txt", "patched", "edit")).rejects.toThrow( + "Stale version -- the file changed since you read it (expected v1, current v3); re-read the file, then retry.", + ) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + }) + + describe("observation refresh after publication", () => { + it("re-observes the published token so a second update without a re-read succeeds", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("doc.txt"), "v1") + const task = createMockTask({ observationRegistry: reg }) + mockedComputeVersionToken.mockResolvedValue("v1") + // The publish changes the on-disk state: the token moves to v2. + mockedSafeWriteText.mockImplementation(async () => { + mockedComputeVersionToken.mockResolvedValue("v2") + }) + + await guardedWrite(task, "doc.txt", "one", "update") + + // The registry records what this write published, not the pre-write read. + expect(reg.get(abs("doc.txt"))?.version).toBe("v2") + + // A second write from the same task without a re-read succeeds: its CAS + // compares against the content the task itself published. + await guardedWrite(task, "doc.txt", "two", "update") + + expect(mockedSafeWriteText).toHaveBeenCalledTimes(2) + expect(reg.get(abs("doc.txt"))?.version).toBe("v2") + }) + + it("records an observation after creating an unobserved file so a follow-up write is not treated as unobserved", async () => { + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) + mockedComputeVersionToken.mockResolvedValue("created-v1") + const task = createMockTask() + // After the create the file exists on disk. + mockedSafeWriteText.mockImplementation(async () => { + mockedFsAccess.mockResolvedValue(undefined) + }) + + await guardedWrite(task, "made.txt", "content", "create") + + expect(task.observationRegistry.get(abs("made.txt"))?.version).toBe("created-v1") + + // Without the write-back this second write would take the unobserved + // branch and fail "File already exists ... was not read before this + // write" for a file the same task had just created. + await guardedWrite(task, "made.txt", "revised", "update") + + expect(mockedSafeWriteText).toHaveBeenCalledTimes(2) + }) + + it("re-observes after a successful edit write", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("doc.txt"), "v1") + const task = createMockTask({ observationRegistry: reg }) + mockedComputeVersionToken.mockResolvedValue("v1") + mockedSafeWriteText.mockImplementation(async () => { + mockedComputeVersionToken.mockResolvedValue("v2") + }) + + await guardedWrite(task, "doc.txt", "patched", "edit") + + expect(reg.get(abs("doc.txt"))?.version).toBe("v2") + }) + + it("keeps a published write successful when the post-publication token computation fails", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("doc.txt"), "v1") + const task = createMockTask({ observationRegistry: reg }) + mockedComputeVersionToken + .mockResolvedValueOnce("v1") // the CAS check at entry passes + .mockRejectedValueOnce({ code: "EACCES" }) // the post-publication token fails + + await expect(guardedWrite(task, "doc.txt", "new content", "update")).resolves.toBeUndefined() + + // The write published; the failed bookkeeping leaves the previous + // observation in place (a follow-up write then fails stale and re-reads). + expect(mockedSafeWriteText).toHaveBeenCalledTimes(1) + expect(reg.get(abs("doc.txt"))?.version).toBe("v1") + }) + }) + + describe("concurrency: per-path FIFO chain", () => { + it("two concurrent updates on one path - exactly one publishes, the other fails stale", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("shared.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + // The first publish changes the on-disk state (new token). + mockedSafeWriteText.mockImplementation(async () => { + mockedComputeVersionToken.mockResolvedValue("v2") + }) + + const p1 = guardedWrite(task, "shared.txt", "first", "update") + const p2 = guardedWrite(task, "shared.txt", "second", "update") + const [r1, r2] = await Promise.allSettled([p1, p2]) + + if (r1.status !== "fulfilled" || r2.status !== "rejected") { + throw new Error("expected exactly one publish, got " + r1.status + " / " + r2.status) + } + expect(mockedSafeWriteText).toHaveBeenCalledTimes(1) + expect(r2.reason.message).toBe( + "Stale version -- the file changed since you read it (expected v1, current v2); re-read the file, then retry.", + ) + }) + + it("holds the submission-time token when the registry moves on while the write waits", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("queued-token.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + let releaseFirst: () => void = () => {} + const gate = new Promise((resolve) => { + releaseFirst = resolve + }) + let publishes = 0 + mockedSafeWriteText.mockImplementation(async (_path: string, content: string) => { + publishes += 1 + if (content === "first") { + await gate + } + // Each publish moves the on-disk token forward. + mockedComputeVersionToken.mockResolvedValue("v2") + }) + + const first = guardedWrite(task, "queued-token.txt", "first", "update") + const second = guardedWrite(task, "queued-token.txt", "second", "update") + await new Promise((resolve) => setImmediate(resolve)) + // A re-read lands while both writes are still in flight: the registry + // moves to v2 behind the queued writes. + reg.observe(abs("queued-token.txt"), "v2") + releaseFirst() + + await first + // The second write was submitted against v1. The first write published + // over that state, so the queued write fails stale instead of publishing + // content derived from the v1 read over the v2 file. An execution-time + // registry lookup would rescue it with the refreshed token and lose the + // first write's update. + await expect(second).rejects.toThrow("Stale version") + expect(publishes).toBe(1) + }) + + it("observed-absent then two concurrent creates - the second fails stale", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("absent.txt"), "v1") // read before, file later vanished + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) + const task = createMockTask({ observationRegistry: reg }) + + let publishes = 0 + mockedSafeWriteText.mockImplementation(async () => { + publishes += 1 + if (publishes === 1) { + // After the first publish the file exists again under a new token. + mockedFsAccess.mockResolvedValue(undefined) + mockedComputeVersionToken.mockResolvedValue("v2") + } + }) + + const p1 = guardedWrite(task, "absent.txt", "first", "create") + const p2 = guardedWrite(task, "absent.txt", "second", "create") + const [r1, r2] = await Promise.allSettled([p1, p2]) + + if (r1.status !== "fulfilled" || r2.status !== "rejected") { + throw new Error("expected exactly one publish, got " + r1.status + " / " + r2.status) + } + expect(publishes).toBe(1) + expect(r2.reason.message).toContain("Stale version") + expect(r2.reason.message).toContain("re-read the file, then retry.") + }) + + it("the chain settles after a rejection - a later matching write still runs", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("settle.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v2") // already stale at v1 + const task = createMockTask({ observationRegistry: reg }) + + const p1 = guardedWrite(task, "settle.txt", "first", "update") + await expect(p1).rejects.toThrow("Stale version") + + // No resetChain: the rejected link must not wedge the chain. The + // caller re-reads the file (observation refreshed to v2) and retries. + reg.observe(abs("settle.txt"), "v2") + const p2 = guardedWrite(task, "settle.txt", "second", "update") + await expect(p2).resolves.toBeUndefined() + + expect(mockedSafeWriteText).toHaveBeenCalledTimes(1) + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("settle.txt"), "second", { + verifyBeforeCommit: expect.any(Function), + }) + }) + + it("a write submitted after an earlier one settled still serializes in submission order", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("evict.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + // A first write settles before the next two are submitted. + const p1 = guardedWrite(task, "evict.txt", "first", "update") + await expect(p1).resolves.toBeUndefined() + + // Two rapid writes submitted after that settlement must still run one at a + // time in submission order. Whether the settled entry has been evicted from the + // path map is not observable from here, so the test does not claim it. + const order: string[] = [] + mockedSafeWriteText.mockImplementation(async (_path: string, content: string) => { + order.push(content) + }) + const p2 = guardedWrite(task, "evict.txt", "second", "update") + const p3 = guardedWrite(task, "evict.txt", "third", "update") + await Promise.all([p2, p3]) + + expect(order).toEqual(["second", "third"]) + // Three publishes in total: the settled first write plus the two + // serialized rapid writes. + expect(mockedSafeWriteText).toHaveBeenCalledTimes(3) + }) + + it("writes on different paths are independent (no cross-path serialization)", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("a.txt"), "v1") + reg.observe(abs("b.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + const p1 = guardedWrite(task, "a.txt", "a", "update") + const p2 = guardedWrite(task, "b.txt", "b", "update") + await Promise.all([p1, p2]) + + expect(mockedSafeWriteText).toHaveBeenCalledTimes(2) + }) + }) + + describe("publication-time re-verification (check-to-rename window)", () => { + /** + * Drive a simulated race: the mocked publish primitive behaves like + * safeWriteText and invokes the pre-commit verification immediately + * before the commit rename. The "external writer" acts in that window + * (after the guard's entry check, before the pre-commit re-verification) + * by changing the mocked on-disk state. Returns a published() probe. + */ + const mockPublishWithRace = (mutate: () => void): (() => boolean) => { + let published = false + mockedSafeWriteText.mockImplementation( + async (_path: string, _content: string, options?: SafeWriteTextOptions) => { + mutate() + await options?.verifyBeforeCommit?.() + published = true + }, + ) + return () => published + } + + it("createIfAbsent rejects when an external writer creates the file between verification and publication", async () => { + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) // absent at entry + const task = createMockTask() + const published = mockPublishWithRace(() => { + // -- the race: an external writer publishes first ------------------ + mockedFsAccess.mockResolvedValue(undefined) // the file now exists + }) + + await expect(guardedWrite(task, "raced.txt", "mine", "create")).rejects.toThrow( + "File already exists at " + + abs("raced.txt") + + " and was not read before this write -- read the file first, then retry.", + ) + // nothing was published: the competing writer's file is preserved + expect(published()).toBe(false) + // entry check + pre-commit re-check + expect(mockedFsAccess).toHaveBeenCalledTimes(2) + }) + + it("replaceIfVersion rejects when an external writer modifies the file between verification and publication", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("raced.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") // matches at entry + const task = createMockTask({ observationRegistry: reg }) + const published = mockPublishWithRace(() => { + // -- the race: an external writer rewrites the file ---------------- + mockedComputeVersionToken.mockResolvedValue("v-external") + }) + + await expect(guardedWrite(task, "raced.txt", "mine", "update")).rejects.toThrow( + "Stale version -- the file changed since you read it (expected v1, current v-external); re-read the file, then retry.", + ) + expect(published()).toBe(false) + }) + + it("replaceIfVersion rejects deleted-after-read when the file is deleted between verification and publication", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("raced.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + const published = mockPublishWithRace(() => { + // -- the race: an external writer deletes the file ----------------- + mockedComputeVersionToken.mockRejectedValue({ code: "ENOENT" }) + }) + + await expect(guardedWrite(task, "raced.txt", "mine", "update")).rejects.toThrow( + "File was deleted after it was read", + ) + expect(published()).toBe(false) + }) + + it("rethrows non-guard I/O failures from the pre-commit verification verbatim", async () => { + const failure = { code: "EACCES" } + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) // absent at entry + const task = createMockTask() + const published = mockPublishWithRace(() => { + // the pre-commit re-check hits a real I/O failure, not a guard verdict + mockedFsAccess.mockRejectedValue(failure) + }) + + await expect(guardedWrite(task, "io-race.txt", "x", "create")).rejects.toBe(failure) + expect(published()).toBe(false) + }) + }) + + describe("path resolution", () => { + it("resolves a relative path against task.cwd", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("sub/dir.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + await guardedWrite(task, "sub/dir.txt", "content", "update") + + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("sub/dir.txt"), "content", { + verifyBeforeCommit: expect.any(Function), + }) + }) + + it("normalizes an already-absolute input (trailing separator) to the observation key", async () => { + const reg = new ObservationRegistry() + const canonical = abs("sub/dir.txt") + // ReadFileTool observes under path.resolve(task.cwd, relPath) — the + // canonical spelling. A write addressed with a trailing separator used + // to bypass the observation (isAbsolute passthrough) and fail + // "File already exists" / "File not read yet" for a file that was read. + reg.observe(canonical, "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + await guardedWrite(task, canonical + "/", "content", "update") + + expect(mockedSafeWriteText).toHaveBeenCalledTimes(1) + expect(mockedSafeWriteText).toHaveBeenCalledWith(canonical, "content", { + verifyBeforeCommit: expect.any(Function), + }) + }) + + it("serializes two spellings of one file through a single chain key", async () => { + const reg = new ObservationRegistry() + const canonical = abs("shared2.txt") + reg.observe(canonical, "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + // The first publish changes the on-disk state (new token). + mockedSafeWriteText.mockImplementation(async () => { + mockedComputeVersionToken.mockResolvedValue("v2") + }) + + // Plain spelling vs the trailing-separator spelling: with one chain key + // they are strictly ordered (first matches v1, second sees v2). + const p1 = guardedWrite(task, canonical, "first", "update") + const p2 = guardedWrite(task, canonical + "/", "second", "update") + const [r1, r2] = await Promise.allSettled([p1, p2]) + + if (r1.status !== "fulfilled" || r2.status !== "rejected") { + throw new Error("expected exactly one publish, got " + r1.status + " / " + r2.status) + } + expect(mockedSafeWriteText).toHaveBeenCalledTimes(1) + expect(r2.reason.message).toBe( + "Stale version -- the file changed since you read it (expected v1, current v2); re-read the file, then retry.", + ) + }) + }) + + describe("resetChain", () => { + it("detaches pending links so later writes start a fresh chain", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("x.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + await guardedWrite(task, "x.txt", "a", "update") + resetChain() + await guardedWrite(task, "x.txt", "b", "update") + + expect(mockedSafeWriteText).toHaveBeenLastCalledWith(abs("x.txt"), "b", { + verifyBeforeCommit: expect.any(Function), + }) + }) + }) +}) + +describe("task cancellation (S4a, epic #1375)", () => { + // The file's shared beforeEach lives inside the main describe, so this top-level one needs its + // own resets - otherwise assertions here see the previous test's recorded calls. + beforeEach(() => { + mockedSafeWriteText.mockClear() + mockedFsAccess.mockClear() + mockedComputeVersionToken.mockClear() + mockedFsAccess.mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + }) + + it("drops a queued write when the task is aborted while it waits behind another write", async () => { + let releaseFirst: () => void = () => {} + const firstGate = new Promise(function (resolve) { + releaseFirst = resolve + }) + mockedSafeWriteText.mockImplementationOnce(async () => { + await firstGate + }) + const task = createMockTask() + const first = guardedWrite(task, "queued.txt", "first", "create") + const second = guardedWrite(task, "queued.txt", "second", "create") + // Let the first link enter the publish (the chain runs on microtasks) before the + // disposal lands: Task.dispose() sets task.abort while the second write is still + // queued behind it. + await new Promise(function (resolve) { + setImmediate(resolve) + }) + task.abort = true + releaseFirst() + await first + await expect(second).rejects.toThrow(/was cancelled/) + // Only the first write published; the cancelled one touched nothing. + expect( + mockedSafeWriteText.mock.calls.map(function (call) { + return call[1] + }), + ).toEqual(["first"]) + }) + + it("refuses an already-cancelled task's write before any I/O", async () => { + const task = createMockTask() + task.abort = true + await expect(guardedWrite(task, "gone.txt", "x", "create")).rejects.toThrow(/was cancelled/) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + expect(mockedFsAccess).not.toHaveBeenCalled() + }) +}) diff --git a/src/core/tools/__tests__/readFileTool.spec.ts b/src/core/tools/__tests__/readFileTool.spec.ts index 5f9033eb7c..2e3faa9548 100644 --- a/src/core/tools/__tests__/readFileTool.spec.ts +++ b/src/core/tools/__tests__/readFileTool.spec.ts @@ -13,6 +13,9 @@ */ import path from "path" +import type { Stats } from "fs" + +import type { LegacyReadFileParams } from "@roo-code/types" import type { ToolUse } from "../../../shared/tools" import type { Task } from "../../task/Task" @@ -20,6 +23,8 @@ import type { Task } from "../../task/Task" import { isBinaryFile } from "isbinaryfile" import { readFileTool, ReadFileTool } from "../ReadFileTool" +import { ObservationRegistry } from "../../task/observationRegistry" +import { computeVersionToken } from "../../../utils/versionToken" import { formatResponse } from "../../prompts/responses" import { validateImageForProcessing, @@ -139,6 +144,7 @@ interface MockTaskOptions { rooIgnoreAllowed?: boolean maxImageFileSize?: number maxTotalImageSize?: number + observationRegistry?: ObservationRegistry } function createMockTask(options: MockTaskOptions = {}) { @@ -146,6 +152,9 @@ function createMockTask(options: MockTaskOptions = {}) { return { cwd: "/test/workspace", + // Mirror Task: every task always owns an observation registry (A2, #1375). + // Tests asserting on observations pass their own instance via options. + observationRegistry: options.observationRegistry ?? new ObservationRegistry(), api: { getModel: vi.fn().mockReturnValue({ info: { supportsImages }, @@ -190,7 +199,18 @@ describe("ReadFileTool", () => { vi.clearAllMocks() // Default mock implementations - mockedFsStat.mockResolvedValue({ isDirectory: () => false } as any) + // The stat default carries BigIntStats fields (A2, epic #1375): reads now + // token-ize the pre/post stats, so the default must look like a real bigint stat. + // Tests overriding it do so per-call with mockResolvedValue(Once). + mockedFsStat.mockResolvedValue({ + isDirectory: () => false, + dev: BigInt(1), + ino: BigInt(2), + size: BigInt(300), + mtimeNs: BigInt(4_000_000_000n), + ctimeNs: BigInt(5_000_000_000n), + // Cast: the mock only implements the members the tool and versionToken read. + } as unknown as Stats) mockedIsBinaryFile.mockResolvedValue(false) mockedFsReadFile.mockResolvedValue(Buffer.from("test content")) mockedReadWithSlice.mockReturnValue({ @@ -1555,5 +1575,330 @@ describe("ReadFileTool", () => { expect(mockTask.didToolFailInCurrentTurn).toBe(true) }) + + describe("observation registry", () => { + it("records an observation on successful read of an existing file", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + // Override the beforeEach default stat mock with proper BigIntStats. + mockedFsStat.mockResolvedValue({ + isDirectory: () => false, + dev: BigInt(1), + ino: BigInt(2), + size: BigInt(300), + mtimeNs: BigInt(4_000_000_000n), + ctimeNs: BigInt(5_000_000_000n), + // Cast: the mock only implements the members the tool and versionToken read. + } as unknown as Stats) + mockedIsBinaryFile.mockResolvedValue(false) + + // Spy on observe to capture the exact key used (Windows path.resolve may use backslashes). + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute({ path: "existing.ts" }, mockTask as unknown as Task, callbacks) + + // Verify the tool called observe exactly once with a valid token. + expect(observeSpy).toHaveBeenCalledTimes(1) + const [calledPath, calledVersion] = observeSpy.mock.calls[0] + expect(calledPath).toContain("existing.ts") + expect(calledVersion).toMatch(/^\d+:\d+:\d+:\d+:\d+$/) + + // The token is built from nanosecond fields, so both stats the read takes around the + // file must be the bigint variant. Counting the calls that asked for it is what makes + // this load-bearing: the mocked stat answers any options with bigint fields, so a + // looser assertion would pass while one of the two calls silently dropped the option + // and threw on a real filesystem, where mtimeNs and ctimeNs are undefined. + const bigintStats = mockedFsStat.mock.calls.filter((call) => { + const options = call[1] as { bigint?: boolean } | undefined + return options?.bigint === true && String(call[0]).includes("existing.ts") + }) + expect(bigintStats).toHaveLength(2) + + // Verify get() returns the same data using the spy-captured key. + const obs = reg.get(calledPath) + expect(obs).toBeDefined() + expect(obs!.version).toBe(calledVersion) + }) + + it("a failed read (absent path) leaves the registry size 0 and does not throw", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + mockedFsReadFile.mockRejectedValue(new Error("ENOENT")) + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute({ path: "missing.ts" }, mockTask as unknown as Task, callbacks) + + // observationRegistry is guaranteed present because we passed it in createMockTask. + const reg = mockTask.observationRegistry + expect(reg).toBeDefined() + expect(reg!.size).toBe(0) + }) + + it("records an observation for legacy-format reads of existing files", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + mockedFsStat.mockResolvedValue({ + isDirectory: () => false, + dev: BigInt(1), + ino: BigInt(2), + size: BigInt(300), + mtimeNs: BigInt(4_000_000_000n), + ctimeNs: BigInt(5_000_000_000n), + // Cast: the mock only implements the members the tool and versionToken read. + } as unknown as Stats) + mockedIsBinaryFile.mockResolvedValue(false) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + // Typed legacy (pre-refactor) params: the multi-file format with the + // _legacyFormat discriminant (see LegacyReadFileParams). + const legacyParams: LegacyReadFileParams = { + files: [{ path: "legacy.ts" }], + _legacyFormat: true, + } + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute(legacyParams, mockTask as unknown as Task, callbacks) + + expect(observeSpy).toHaveBeenCalledTimes(1) + const [calledPath, calledVersion] = observeSpy.mock.calls[0] + expect(calledPath).toContain("legacy.ts") + expect(calledVersion).toMatch(/^\d+:\d+:\d+:\d+:\d+$/) + + // Same requirement on the legacy multi-file path, which resolves its own stats. + + // The token is built from nanosecond fields, so both stats the read takes around the + // file must be the bigint variant. Counting the calls that asked for it is what makes + // this load-bearing: the mocked stat answers any options with bigint fields, so a + // looser assertion would pass while one of the two calls silently dropped the option + // and threw on a real filesystem, where mtimeNs and ctimeNs are undefined. + const bigintStats = mockedFsStat.mock.calls.filter((call) => { + const options = call[1] as { bigint?: boolean } | undefined + return options?.bigint === true && String(call[0]).includes("legacy.ts") + }) + expect(bigintStats).toHaveLength(2) + }) + + it("does not observe when the file mutates between the pre-read and post-read stats", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + const preStats = { + isDirectory: () => false, + dev: BigInt(1), + ino: BigInt(2), + size: BigInt(300), + mtimeNs: BigInt(4_000_000_000n), + ctimeNs: BigInt(5_000_000_000n), + } + // A mutation lands mid-read: the post-read stat differs. + const postStats = { ...preStats, size: BigInt(301) } + + // Call order: directory check, pre-read stat, post-read stat. + mockedFsStat + .mockResolvedValueOnce({ isDirectory: () => false } as unknown as Stats) + .mockResolvedValueOnce(preStats as unknown as Stats) + .mockResolvedValueOnce(postStats as unknown as Stats) + mockedIsBinaryFile.mockResolvedValue(false) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute({ path: "mutated.ts" }, mockTask as unknown as Task, callbacks) + + // The read itself succeeded, but the target stays unobserved: the content the + // model received is not the on-disk state, so observing it would let a later + // write match a token the model never saw. + expect(observeSpy).not.toHaveBeenCalled() + expect(reg.size).toBe(0) + expect(mockTask.didToolFailInCurrentTurn).toBe(false) + }) + + it("leaves the target unobserved without failing the read when the pre-read stat fails", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + // Directory check OK; the pre-read stat fails (caught, target unobserved). + mockedFsStat + .mockResolvedValueOnce({ isDirectory: () => false } as unknown as Stats) + .mockRejectedValueOnce(new Error("EACCES")) + mockedIsBinaryFile.mockResolvedValue(false) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute({ path: "stat-fail.ts" }, mockTask as unknown as Task, callbacks) + + expect(observeSpy).not.toHaveBeenCalled() + expect(reg.size).toBe(0) + // The read still succeeds — a stat failure never fails the read. + expect(mockTask.didToolFailInCurrentTurn).toBe(false) + expect(callbacks.pushToolResult).toHaveBeenCalled() + }) + + it("leaves the target unobserved without failing the read when the post-read stat fails", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + const okStats = { + isDirectory: () => false, + dev: BigInt(1), + ino: BigInt(2), + size: BigInt(300), + mtimeNs: BigInt(4_000_000_000n), + ctimeNs: BigInt(5_000_000_000n), + } + // Directory check and pre-read stat OK; the post-read stat fails. + mockedFsStat + .mockResolvedValueOnce({ isDirectory: () => false } as unknown as Stats) + .mockResolvedValueOnce(okStats as unknown as Stats) + .mockRejectedValueOnce(new Error("EACCES")) + mockedIsBinaryFile.mockResolvedValue(false) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute({ path: "post-stat-fail.ts" }, mockTask as unknown as Task, callbacks) + + expect(observeSpy).not.toHaveBeenCalled() + expect(reg.size).toBe(0) + expect(mockTask.didToolFailInCurrentTurn).toBe(false) + expect(callbacks.pushToolResult).toHaveBeenCalled() + }) + + it("legacy format: does not observe when the file mutates between the pre-read and post-read stats", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + const preStats = { + isDirectory: () => false, + dev: BigInt(1), + ino: BigInt(2), + size: BigInt(300), + mtimeNs: BigInt(4_000_000_000n), + ctimeNs: BigInt(5_000_000_000n), + } + // Call order: directory check, pre-read stat, post-read stat (mutated). + mockedFsStat + .mockResolvedValueOnce({ isDirectory: () => false } as unknown as Stats) + .mockResolvedValueOnce(preStats as unknown as Stats) + .mockResolvedValueOnce({ ...preStats, size: BigInt(301) } as unknown as Stats) + mockedIsBinaryFile.mockResolvedValue(false) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + const legacyParams: LegacyReadFileParams = { + files: [{ path: "legacy-mutated.ts" }], + _legacyFormat: true, + } + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute(legacyParams, mockTask as unknown as Task, callbacks) + + expect(observeSpy).not.toHaveBeenCalled() + expect(reg.size).toBe(0) + expect(mockTask.didToolFailInCurrentTurn).toBe(false) + }) + + it("legacy format: leaves the target unobserved when a stat fails without failing the read", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + // Directory check OK; the pre-read stat fails (caught, target unobserved). + mockedFsStat + .mockResolvedValueOnce({ isDirectory: () => false } as unknown as Stats) + .mockRejectedValueOnce(new Error("EACCES")) + mockedIsBinaryFile.mockResolvedValue(false) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + const legacyParams: LegacyReadFileParams = { + files: [{ path: "legacy-stat-fail.ts" }], + _legacyFormat: true, + } + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute(legacyParams, mockTask as unknown as Task, callbacks) + + expect(observeSpy).not.toHaveBeenCalled() + expect(reg.size).toBe(0) + expect(mockTask.didToolFailInCurrentTurn).toBe(false) + expect(callbacks.pushToolResult).toHaveBeenCalled() + }) + it("legacy format: leaves the target unobserved when the post-read stat fails", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + const okStats = { + isDirectory: () => false, + dev: BigInt(1), + ino: BigInt(2), + size: BigInt(300), + mtimeNs: BigInt(4_000_000_000n), + ctimeNs: BigInt(5_000_000_000n), + } + // Directory check and pre-read stat OK; the post-read stat fails. + mockedFsStat + .mockResolvedValueOnce({ isDirectory: () => false } as unknown as Stats) + .mockResolvedValueOnce(okStats as unknown as Stats) + .mockRejectedValueOnce(new Error("EACCES")) + mockedIsBinaryFile.mockResolvedValue(false) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + const legacyParams: LegacyReadFileParams = { + files: [{ path: "legacy-post-stat-fail.ts" }], + _legacyFormat: true, + } + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute(legacyParams, mockTask as unknown as Task, callbacks) + + expect(observeSpy).not.toHaveBeenCalled() + expect(reg.size).toBe(0) + expect(mockTask.didToolFailInCurrentTurn).toBe(false) + expect(callbacks.pushToolResult).toHaveBeenCalled() + }) + it("two separate Task-owned registries are independent", async () => { + const regA = new ObservationRegistry() + const regB = new ObservationRegistry() + regA.observe("/shared.ts", "v1") + expect(regA.get("/shared.ts")!.version).toBe("v1") + expect(regB.get("/shared.ts")).toBeUndefined() + regB.observe("/shared.ts", "v2") + expect(regA.get("/shared.ts")!.version).toBe("v1") + expect(regB.get("/shared.ts")!.version).toBe("v2") + }) + }) }) }) diff --git a/src/core/tools/guardedWrite.ts b/src/core/tools/guardedWrite.ts new file mode 100644 index 0000000000..fcf7d051fa --- /dev/null +++ b/src/core/tools/guardedWrite.ts @@ -0,0 +1,410 @@ +/** + * Guarded-write compare-and-swap core (upstream epic #1375, phase A4a). + * + * Wraps the S3 safeWriteText publish primitive behind version-token guards so + * that every write is deterministic: + * + * - an unobserved target may only be created when it is absent + * (createIfAbsent); + * - an observed target is published only when the on-disk version token still + * matches the token recorded at read time (replaceIfVersion); + * - an edit-style write requires a prior observation (unobservedEditGuard). + * + * A per-absolute-path FIFO chain of tail promises orders concurrent + * in-process writes to the same path: the first matching write wins, the rest + * fail stale. Observations come from the task's S2 ObservationRegistry and are + * captured at submission time; a successful publication re-observes the path + * with the published token so sequential writes from one task compare against + * the content that task published, not against its pre-write read. + * + * Check-to-publication window (CodeRabbit review, PRs #1405 / #1413): every + * guard predicate is enforced TWICE -- once at entry and once at publication + * time. The publication-time re-verification runs inside safeWriteText + * immediately before the atomic commit rename (its verifyBeforeCommit + * option), while the per-path FIFO chain holds the serialization across the + * whole verify+publish window, so the predicate is re-checked against the + * state the rename will actually replace and concurrent in-process writers + * stay fully ordered. + * + * Residual cross-process window: an external writer (another process) can + * still create or modify the target in the short interval between the + * pre-commit re-verification and the commit rename. A cross-platform atomic + * conditional publication would require an OS-level primitive beyond + * fs.promises (or a shared lock protocol every writer honors) and is tracked + * as a follow-up of epic #1375. + */ + +import * as fs from "fs/promises" +import * as path from "path" + +import { safeWriteText } from "../../services/file-safety/safeWriteText" +import { computeVersionToken } from "../../utils/versionToken" +import type { Task } from "../task/Task" + +// -- Types ------------------------------------------------------------------ + +/** Write kind that drives guard selection. */ +export type GuardedWriteKind = "create" | "update" | "edit" + +/** Internal error thrown when a guard rejects a write. */ +class GuardRejectedError extends Error { + constructor( + message: string, + readonly path: string, + ) { + super(message) + this.name = "GuardRejectedError" + } +} + +// -- Per-path tail-promise chain -------------------------------------------- + +/** + * Per-absolute-path FIFO chain of pending guarded writes (tail promise per + * path). Every write enqueues onto the current tail for its path, so + * concurrent writes to the same path run one at a time in submission order. + * + * The chain never leaks a rejection through itself: each link settles, a + * rejected link is skipped by the next writer (a failed write must not block + * later writes to the same path), and every caller receives its own link + * promise to handle. + * + * Settled entries are evicted (below), so a long-lived extension does not + * accumulate a map entry per distinct written path. + */ +const pendingChains = new Map>() + +/** + * Enqueue a write operation on the per-path FIFO chain. + * + * Returns the promise for this link; it always settles. A prior link that + * rejected is skipped, not propagated. The map entry for this link is + * deleted once it settles — but only while it is still the current tail for + * the path, so a replacement enqueued in the meantime keeps ownership. + */ +function enqueue(pathKey: string, fn: () => Promise): Promise { + const prev = pendingChains.get(pathKey) ?? Promise.resolve() + const next = prev.then(fn, fn) + pendingChains.set(pathKey, next) + void next.then( + () => { + if (pendingChains.get(pathKey) === next) { + pendingChains.delete(pathKey) + } + }, + () => { + if (pendingChains.get(pathKey) === next) { + pendingChains.delete(pathKey) + } + }, + ) + return next +} + +// -- Guard primitives -------------------------------------------------------- + +/** + * Extract a Node errno code (e.g. "ENOENT") from a thrown value, or + * undefined when the value carries none. + */ +function errorCode(error: unknown): string | undefined { + return typeof error === "object" && error !== null && "code" in error + ? (error as { code?: string }).code + : undefined +} + +/** True when the path is absent on disk (fs.access reports ENOENT). */ +async function fileIsAbsent(absolutePath: string): Promise { + try { + await fs.access(absolutePath) + return false + } catch (error: unknown) { + return errorCode(error) === "ENOENT" + } +} + +/** Build the read-first remediation error for an existing target. */ +function alreadyExistsError(absolutePath: string): GuardRejectedError { + return new GuardRejectedError( + "File already exists at " + + absolutePath + + " and was not read before this write -- read the file first, then retry.", + absolutePath, + ) +} + +/** Build the stale-version remediation error. */ +function staleVersionError(absolutePath: string, expectedVersion: string, currentVersion: string): GuardRejectedError { + return new GuardRejectedError( + "Stale version -- the file changed since you read it (expected " + + expectedVersion + + ", current " + + currentVersion + + "); re-read the file, then retry.", + absolutePath, + ) +} + +/** Build the deleted-after-read remediation error. */ +function deletedAfterReadError(absolutePath: string, expectedVersion: string): GuardRejectedError { + return new GuardRejectedError( + "File was deleted after it was read -- the version recorded at read time (" + + expectedVersion + + ") no longer exists; re-read the file, then retry.", + absolutePath, + ) +} + +/** + * Publish content only if the target file does not exist. + * + * The absence predicate is enforced at entry AND at publication time: the + * pre-commit re-check (verifyBeforeCommit, run by safeWriteText immediately + * before the commit rename) rejects with the same remediation when an + * external writer created the file in the check-to-rename window, instead of + * overwriting it. + * + * Rejects with a loud remediation error when the file already exists: the + * write was issued for a file that was never read, so the caller must read + * the file first, then retry. + */ +export async function createIfAbsent(absolutePath: string, content: string): Promise { + try { + await fs.access(absolutePath) + } catch (error: unknown) { + if (errorCode(error) !== "ENOENT") { + // A real I/O failure (EACCES, EIO, ...) -- not a guard verdict. + throw error + } + // Absent at entry. safeWriteText re-verifies absence at the last + // moment before the commit rename (see verifyStillAbsent) so a writer + // that creates the file in the check-to-rename window is rejected, + // not overwritten. + await safeWriteText(absolutePath, content, { + verifyBeforeCommit: () => verifyStillAbsent(absolutePath), + }) + return + } + + throw alreadyExistsError(absolutePath) +} + +/** + * Pre-commit absence check for createIfAbsent (publication-time re- + * verification): rejects with the standard read-first remediation when the + * target exists at publication time. ENOENT (still absent) passes; any other + * I/O error is rethrown verbatim (a real failure, not a guard verdict). + */ +async function verifyStillAbsent(absolutePath: string): Promise { + try { + await fs.access(absolutePath) + } catch (error: unknown) { + if (errorCode(error) === "ENOENT") return + throw error + } + throw alreadyExistsError(absolutePath) +} + +/** + * Publish content only if the current on-disk version token equals + * expectedVersion (the token observed at read time). + * + * The version predicate is enforced at entry AND at publication time: the + * pre-commit re-check (verifyBeforeCommit, run by safeWriteText immediately + * before the commit rename) rejects stale with the same remediation when an + * external writer modified the file in the check-to-rename window, instead + * of overwriting it. + * + * On a match the content is published via the S3 safeWriteText primitive; on + * a mismatch the write is rejected stale with a re-read-then-retry + * remediation suffix. + */ +export async function replaceIfVersion(absolutePath: string, expectedVersion: string, content: string): Promise { + let currentVersion: string + try { + currentVersion = await computeVersionToken(absolutePath) + } catch (error: unknown) { + if (errorCode(error) === "ENOENT") { + // The observed file was deleted after the read: the version recorded + // at read time no longer exists on disk. Normalize the raw ENOENT + // into the guard's re-read-then-retry contract so the caller gets a + // remediation it can act on, not a raw errno. + throw deletedAfterReadError(absolutePath, expectedVersion) + } + // A real I/O failure (EACCES, EIO, ...) -- not a guard verdict. + throw error + } + + if (currentVersion !== expectedVersion) { + throw staleVersionError(absolutePath, expectedVersion, currentVersion) + } + + // Match at entry. safeWriteText re-verifies the token at the last moment + // before the commit rename (see verifyVersionUnchanged) so a writer that + // modifies the file in the check-to-rename window is rejected stale, not + // overwritten. + await safeWriteText(absolutePath, content, { + verifyBeforeCommit: () => verifyVersionUnchanged(absolutePath, expectedVersion), + }) +} + +/** + * Pre-commit version check for replaceIfVersion (publication-time re- + * verification): rejects with the standard stale / deleted-after-read + * remediation when the on-disk token no longer matches expectedVersion at + * publication time. Any other I/O error is rethrown verbatim (a real failure, + * not a guard verdict). + */ +async function verifyVersionUnchanged(absolutePath: string, expectedVersion: string): Promise { + let currentVersion: string + try { + currentVersion = await computeVersionToken(absolutePath) + } catch (error: unknown) { + if (errorCode(error) === "ENOENT") { + throw deletedAfterReadError(absolutePath, expectedVersion) + } + throw error + } + if (currentVersion !== expectedVersion) { + throw staleVersionError(absolutePath, expectedVersion, currentVersion) + } +} + +/** + * Unobserved-edit guard: an edit-style write without a prior observation is + * rejected before any I/O. The literal-match / patch logic stays with the + * tools in S4b; this guard only verifies that a read happened first. + * + * Returns Promise because the rejection is total: this function + * never resolves. + */ +export async function unobservedEditGuard(absolutePath: string): Promise { + throw new GuardRejectedError("File not read yet -- read the file, then retry.", absolutePath) +} + +// -- Public API -------------------------------------------------------------- + +/** + * Resolve a relative or absolute path against task.cwd. + * + * path.resolve also normalizes an already-absolute input (collapsing "." / ".." + * segments and trailing separators), so the key always matches the + * ObservationRegistry key recorded at read time (ReadFileTool observes under + * path.resolve(task.cwd, relPath)) and two spellings of one file share one + * FIFO chain. + */ +function resolveAbsolutePath(task: Task, relPathOrAbsolute: string): string { + return path.resolve(task.cwd, relPathOrAbsolute) +} + +/** + * A queued guarded write reached the head of its path's chain after the task that + * issued it had already been aborted or disposed. Task.dispose() sets the same + * `abort` flag that abortTask() sets, so that flag is the disposal signal visible + * at this layer. + */ +export class CancelledTaskWriteError extends Error { + readonly path: string + constructor(absolutePath: string) { + super( + `Guarded write for ${absolutePath} was cancelled -- the task was aborted or disposed ` + + "before its turn in the per-path write queue; nothing was published.", + ) + this.name = "CancelledTaskWriteError" + this.path = absolutePath + } +} + +/** + * Guarded write entry point. + * + * 1. Resolves the absolute path against task.cwd. + * 2. Captures the task's S2 observation for the path at SUBMISSION time, not + * when the queued link runs: the content being written was derived from + * the read this observation records, so the CAS must compare against that + * token even if the registry moves on - through a re-read or through this + * task's own earlier write re-observing the path - while the link waits + * behind another write. A queued write whose token no longer matches fails + * stale and the caller re-reads; publishing it anyway would overwrite + * content the submitted write never saw. + * 3. Consults the captured observation to pick the guard: + * - unobserved + create/update: createIfAbsent (rejects if it exists); + * - observed + create on a file that vanished after the read: recreate; + * - observed otherwise: replaceIfVersion (CAS on the S1 version token); + * - unobserved + edit: unobservedEditGuard. + * The guard runs on the per-path FIFO chain so concurrent writes to the + * same path are deterministically ordered. The chain holds the + * serialization across the whole verify+publish window, and the guard's + * publication-time re-verification (inside safeWriteText, immediately + * before the commit rename) closes the check-to-rename window for writers + * serialized by the chain. + * 4. After a successful publication the new on-disk token is recorded back + * into the registry, so a follow-up write from the same task compares + * against the content this write published instead of failing stale + * against its own output (or treating a file it just created as + * unobserved). The re-observation runs inside the chain link, before the + * link settles, so the next queued link and the next submission both see + * the refreshed token. A token failure after a successful publication is + * swallowed: the write itself succeeded, and the registry keeps the + * previous observation, which a follow-up write already handles. + */ +export async function guardedWrite( + task: Task, + relPathOrAbsolute: string, + content: string, + kind: GuardedWriteKind = "update", +): Promise { + const absolutePath = resolveAbsolutePath(task, relPathOrAbsolute) + // Submission-time capture - see step 2 above. + const obs = task.observationRegistry.get(absolutePath) + + return enqueue(absolutePath, async () => { + // The link can reach the head of the queue long after the task that issued it is + // gone (panel closed, task switched, abort landed while another write held the + // path). Running it then would publish for a task that no longer serves requests + // and re-observe the path, so the write stops here instead. + if (task.abort) { + throw new CancelledTaskWriteError(absolutePath) + } + + if (obs === undefined) { + // Edit-style writes require a prior read: no observation, no write. + if (kind === "edit") { + await unobservedEditGuard(absolutePath) + } + // Never read: only an absent target may be created. (The edit guard + // above rejects before reaching this line.) + await createIfAbsent(absolutePath, content) + } else if (kind === "create" && (await fileIsAbsent(absolutePath))) { + // A "create" on a file that vanished after the read recreates it. + // Observed "edit" and "update" writes never reach the existence probe - + // the condition short-circuits on kind - and take the CAS branch below. + await createIfAbsent(absolutePath, content) + } else { + // The version recorded at read time must still match the on-disk + // token: for "edit" and "update", and for "create" on a file that + // still exists. + await replaceIfVersion(absolutePath, obs.version, content) + } + + // Re-observe the published version (step 4). A failure to compute the + // token after a successful publication must not turn the published + // write into a failed one, so it is swallowed here and the registry + // keeps the previous observation. + try { + const published = await computeVersionToken(absolutePath) + task.observationRegistry.observe(absolutePath, published) + } catch { + // Swallowed on purpose: the publication itself succeeded, and a + // follow-up write against the previous observation is the + // pre-existing contract (stale -> re-read). + } + }) +} + +/** + * Reset the per-path tail-promise chains (test hook). + */ +export function resetChain(): void { + pendingChains.clear() +} diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index e4b15aa27e..e7568413ab 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -976,7 +976,7 @@ }, "core/tools/__tests__/readFileTool.spec.ts": { "@typescript-eslint/no-explicit-any": { - "count": 96 + "count": 95 } }, "core/tools/__tests__/runSlashCommandTool.spec.ts": { @@ -1716,7 +1716,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 00b3dcaf7a..ac721f19a6 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -16,6 +16,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 @@ -27,6 +35,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 @@ -792,9 +802,9 @@ describe("DiffViewProvider", () => { const result = await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 2000) - // Verify file was written - const fs = await import("fs/promises") - expect(fs.writeFile).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", "utf-8") + // Verify file was written via safeWriteText + const { safeWriteText } = await import("../../../services/file-safety/safeWriteText") + expect(safeWriteText).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content") // Verify file was opened without focus expect(vscode.window.showTextDocument).toHaveBeenCalledWith( @@ -815,9 +825,9 @@ describe("DiffViewProvider", () => { it("should not open file when openWithoutFocus is false", async () => { await diffViewProvider.saveDirectly("test.ts", "new content", false, true, 1000) - // Verify file was written - const fs = await import("fs/promises") - expect(fs.writeFile).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", "utf-8") + // Verify file was written via safeWriteText + const { safeWriteText } = await import("../../../services/file-safety/safeWriteText") + expect(safeWriteText).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content") // Verify file was NOT opened expect(vscode.window.showTextDocument).not.toHaveBeenCalled() @@ -830,9 +840,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..cae522e6d4 --- /dev/null +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -0,0 +1,913 @@ +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(), + rmdirSync: 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 staging-file fsync failure aborts the publish and leaves the target untouched", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The durability step itself fails: the staged bytes never reached the disk, so + // publishing them would put content at the target that a crash can lose. + vi.mocked(fsSync.fsyncSync).mockImplementationOnce(() => { + throw new Error("EIO") + }) + + await expect(safeWriteText(targetPath, "new data", { platform: "linux" })).rejects.toThrow("EIO") + + // No commit: the rename that publishes the staged file never ran, so the target + // still holds whatever it held before the call. + expect(fs.rename).not.toHaveBeenCalled() + + // The fd is closed and the staged temp released. + expect(fsSync.closeSync).toHaveBeenCalledWith(1) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + 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: a partial dump left by a failed save is still unlinked", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // icacls can write a partial dump and still exit non-zero. The dump path must stay + // tracked so the cleanup removes it; clearing it on the failure would leave the + // dump sitting next to the target with nothing left to remove it. + 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" }) + + // No restore attempt (the save failed), but the dump is cleaned up. + expect(execFile).toHaveBeenCalledTimes(1) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".acl.tmp")) + }) + + 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() + }) + }) + + describe("parent directory creation", () => { + it("creates a missing parent directory before staging", async () => { + const targetPath = "/tmp/test-dir/nested/deeper/target.txt" + const dirPath = "/tmp/test-dir/nested/deeper" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data") + + expect(fs.mkdir).toHaveBeenCalledWith(dirPath, { recursive: true }) + expect(fs.access).toHaveBeenCalledWith(dirPath) + expect(vi.mocked(fs.mkdir).mock.invocationCallOrder[0]).toBeLessThan( + vi.mocked(fsSync.openSync).mock.invocationCallOrder[0], + ) + }) + + it("surfaces a parent directory creation failure before any staging", async () => { + const targetPath = "/tmp/test-dir/nested/deeper/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fs.mkdir).mockRejectedValue(Object.assign(new Error("EACCES mkdir"), { code: "EACCES" })) + + await expect(safeWriteText(targetPath, "data")).rejects.toThrow("EACCES mkdir") + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("surfaces a parent directory access failure before any staging", async () => { + const targetPath = "/tmp/test-dir/nested/deeper/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fs.access).mockRejectedValue(Object.assign(new Error("EACCES access"), { code: "EACCES" })) + + await expect(safeWriteText(targetPath, "data")).rejects.toThrow("EACCES access") + expect(fs.rename).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("leaves the creation mask in charge when the target does not exist yet", 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) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // openSync already applied the process umask to the requested mode. Forcing the + // 0o644 default back on with fchmod would undo a restrictive umask (0o600 under + // umask 077) and publish a group/world-readable file for a target that never + // existed, so the mask has to stay in charge. + expect(fsSync.fchmodSync).not.toHaveBeenCalled() + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), "w", 0o644) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("preserves an existing target's mode on the self-staged path", 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) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // The preservation rule still applies when a target exists: a 0o600 file must + // not become 0o644 through the atomic rename. + expect(fsSync.fchmodSync).toHaveBeenCalledWith(2, 0o600) + }) + + it("uses the supplied path as-is when the caller already resolved the publish target", async () => { + const targetPath = "/tmp/test-dir/target.txt" + // A second resolution would follow a link installed after the caller's own + // re-check, so with the flag set realpath must not run at all. + vi.mocked(fs.realpath).mockResolvedValue("/elsewhere/referent.txt") + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o644)) + vi.mocked(fsSync.openSync).mockReturnValue(2) + + await safeWriteText(targetPath, "data", { platform: "linux", targetPathIsResolved: true }) + + expect(fs.realpath).not.toHaveBeenCalled() + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), path.resolve(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) + }) + + // The staging sub-directory must not outlive the write: a hidden directory left + // in every directory Zoo writes to shows up in the explorer, watchers and indexers. + it("removes the staging directory it created once the write commits", 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(0o644)) + + await safeWriteText(targetPath, "content", { platform: "linux" }) + + expect(fsSync.rmdirSync).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + }) + + // A caller that staged its own temp file never had a staging directory created, + // so there is nothing of ours to remove - and removing a directory we did not + // create could delete one a concurrent writer is still using. + it("leaves no staging directory to remove when the caller supplied the tempPath", 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(0o644)) + + await safeWriteText(targetPath, "", { tempPath: "/tmp/test-dir/.staged.json", platform: "linux" }) + + expect(fsSync.mkdirSync).not.toHaveBeenCalledWith( + expect.stringContaining(".file-safety-staging"), + expect.anything(), + ) + expect(fsSync.rmdirSync).not.toHaveBeenCalled() + }) + + // openSync applies the process umask to the requested mode, so the mode has to be + // set on the descriptor: a 0o664 target must not be published as 0o644. + it("sets the staged mode with fchmodSync so the process umask cannot narrow it", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(7) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o664)) + + await safeWriteText(targetPath, "content", { platform: "linux" }) + + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), "w", 0o664) + expect(fsSync.fchmodSync).toHaveBeenCalledWith(7, 0o664) + }) + + // The failure path unlinks the staged temp, which empties the staging directory: + // it must be removed there too, or a failed write leaves the hidden directory. + it("removes the staging directory when the commit fails", 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(0o644)) + vi.mocked(fs.rename).mockRejectedValueOnce(Object.assign(new Error("EXDEV"), { code: "EXDEV" })) + + await expect(safeWriteText(targetPath, "content", { platform: "linux" })).rejects.toThrow("EXDEV") + + expect(fsSync.rmdirSync).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + }) + + 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() + }) + }) + + // ── Test 9: pre-commit verification (verifyBeforeCommit, A4a guarded write) ── + + describe("pre-commit verification (verifyBeforeCommit)", () => { + it("rejects publication when the hook fails: no commit rename, temp discarded, error propagated", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const guard = new Error("stale version") + + await expect( + safeWriteText(targetPath, "data", { + platform: "linux", + verifyBeforeCommit: async () => { + throw guard + }, + }), + ).rejects.toBe(guard) + + // the commit rename never happened + expect(fs.rename).not.toHaveBeenCalled() + // the staged temp was discarded + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("runs the hook exactly once, immediately before the commit rename", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const hook = vi.fn(async () => {}) + const order: string[] = [] + vi.mocked(fs.rename).mockImplementation(async () => { + order.push("rename") + }) + + await safeWriteText(targetPath, "data", { + platform: "linux", + verifyBeforeCommit: async () => { + order.push("verify") + return hook() + }, + }) + + expect(hook).toHaveBeenCalledTimes(1) + expect(order).toEqual(["verify", "rename"]) + expect(fs.rename).toHaveBeenCalledTimes(1) + }) + + it("publishes normally when the hook succeeds", 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", + verifyBeforeCommit: async () => {}, + }) + + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("runs the hook before the backup rename, so a rejection leaves the target in place (backup:true)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const guard = new Error("stale") + + await expect( + safeWriteText(targetPath, "data", { + backup: true, + platform: "linux", + verifyBeforeCommit: async () => { + throw guard + }, + }), + ).rejects.toBe(guard) + + // No rename at all: the hook runs before the target is moved aside, so a + // rejection never creates the moved-aside state that needed a rollback. + expect(fs.rename).not.toHaveBeenCalled() + // the staged temp was discarded + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("with backup:true the hook observes the target, not the post-backup absence", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // Simulate the on-disk truth: once the target has been renamed to the backup + // path, accessing it reports ENOENT. + let movedAside = false + vi.mocked(fs.rename).mockImplementation(async (from) => { + if (String(from) === targetPath) movedAside = true + return undefined + }) + vi.mocked(fs.access).mockImplementation(async (p) => { + if (String(p) === targetPath && movedAside) { + throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + } + return undefined + }) + + await safeWriteText(targetPath, "data", { + backup: true, + platform: "linux", + // A version-check hook reads the target - exactly what a replaceIfVersion + // guard does. After the backup rename it would see ENOENT and fail every + // write, and an absence-check hook would pass vacuously. + verifyBeforeCommit: async () => { + await fs.access(targetPath) + }, + }) + + // The backup rename did happen - after the hook ran. + expect(movedAside).toBe(true) + }) + }) +}) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts new file mode 100644 index 0000000000..7a59e9aa19 --- /dev/null +++ b/src/services/file-safety/safeWriteText.ts @@ -0,0 +1,445 @@ +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 + + /** + * Set when the caller has already resolved the publish target and guarded the + * symlink window itself (safeWriteJson resolves, locks, and re-checks the final + * component). safeWriteText then uses filePath as-is: resolving a second time would + * re-open the window the caller just closed, because a link installed after the + * caller's check would be followed here and the content committed to its referent. + */ + targetPathIsResolved?: boolean + + /** + * Pre-commit verification hook (A4a guarded write, epic #1375). Invoked + * immediately before the commit rename and BEFORE any backup rename moves the + * target aside, so a caller can re-check the target's state and reject + * publication when it changed since the caller's earlier verification. When the + * hook rejects, no commit rename is performed and no backup has been taken yet, + * so there is nothing to roll back: the staged temp file is discarded and the + * hook's rejection is propagated to the caller. + * + * Scope: the hook narrows - it does not close - the check-to-rename window for + * writers that the caller serializes (guardedWrite's per-path FIFO chain); an + * external process can still publish between the hook and the rename + * (documented in guardedWrite). + */ + verifyBeforeCommit?: () => Promise +} + +// -- 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 +} + +/** Remove the staging sub-directory when nothing is staged in it any more. + * rmdirSync fails on a non-empty directory (a concurrent write is still using it) + * and on a directory that is already gone, so the last write to finish cleans up + * and the others leave it to that writer - no shared bookkeeping is needed, and a + * persistent hidden directory is never left in the user's workspace. */ +function _removeStagingDirIfEmpty(dir: string): void { + try { + fsSync.rmdirSync(dir) + } catch { + // non-empty (a concurrent write is still staging there) or already removed + } +} + +/** + * 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 run the pre-commit verification hook (verifyBeforeCommit) before + * the target is moved aside, so it observes the state the rename replaces; + * a rejection aborts the publish (no commit rename, no backup taken yet) and + * propagates. + * 5. Optionally rename target -> backup (when backup:true). + * 6. Atomic rename temp -> target. + * 7. win32 only: restore DACL onto the directory AFTER commit rename. + * 8. On success: delete backup (if any) and unlink DACL dump; remove the staging + * sub-directory when it is empty. + * 9. On failure: rollback backup to target path; clean up temp + dump, and remove + * the staging sub-directory when it is empty. + */ + +/** + * 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 + }) +} + +/** + * Raised when the commit rename failed AND the backup could not be renamed back onto the + * target: the target path is absent 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) + + // Resolve the symlink referent (see resolvePublishTarget) - unless the caller + // already resolved it and closed the substitution window itself. + const targetPath = options?.targetPathIsResolved ? absoluteFilePath : 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. + let stagingDirPath: string | null = null + let tempPath: string + if (options?.tempPath) { + tempPath = options.tempPath + } else { + stagingDirPath = _stagingDir(dirPath) + tempPath = _tempName(stagingDirPath, "safeWriteText") + } + + let backupPath: string | null = null + let releaseBackupOnSuccess = false + let daclDumpPath: string | null = null // tracked for cleanup in finally + let daclSaved = false // the restore step runs only when the save succeeded + + 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 + let targetExists = false + try { + targetMode = fsSync.statSync(targetPath).mode & 0o777 + targetExists = true + } catch { + // target does not exist yet - keep the default + } + const fd = fsSync.openSync(tempPath, "w", targetMode) + try { + // openSync applies the process umask to the requested mode, so a 0o664 + // or 0o666 target would be staged as 0o644 under the common umask 022 and + // lose group write through the rename. Set the mode on the fd instead, the + // same way the caller-staged branch below does - but only when a target + // actually existed to preserve. For a new target the creation mask must win: + // forcing the 0o644 default back on with fchmod would undo a restrictive + // umask (0o600 under umask 077) and publish a group/world-readable file. + if (targetExists) { + fsSync.fchmodSync(fd, targetMode) + } + // 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) { + // Skip the RESTORE step only. icacls can create a partial dump and still exit + // non-zero, so the path stays tracked: dropping it here would leave that file + // next to the target with nothing left to remove it. + daclSaved = false + } else { + daclSaved = true + } + } catch { + // target does not exist or access failed — no DACL handling + daclDumpPath = null + } + } + + try { + // -- Step 3a (A4a): pre-commit verification -------------------------- + // Runs before the target is moved aside, so a conditional publication + // (guardedWrite's createIfAbsent / replaceIfVersion) validates against the state + // the commit rename will actually replace. Running it after the backup rename + // would make the target look absent: a version check would fail with ENOENT and + // an absence check would pass vacuously. A rejection skips the commit rename and + // discards the staged temp; no backup has been taken yet, so there is nothing to + // roll back. + if (options?.verifyBeforeCommit) { + await options.verifyBeforeCommit() + } + + // -- Step 3b (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" && daclSaved && 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. Remove the staging + // directory when this write was the last one using it. + if (stagingDirPath !== null) { + _removeStagingDirIfEmpty(stagingDirPath) + } + } 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 (rollbackError: unknown) { + // The commit failed AND the restore failed: the target path is absent and the + // previous content survives only under the randomized backup name. Reporting only + // the primary failure leaves the caller unable to find that copy, so both are + // surfaced - the primary error stays reachable as originalError and in the text. + rollbackFailure = { backupPath, error: rollbackError } + } + } + + // Always clean up the staging temp file on failure. + try { + await fs.unlink(tempPath).catch(() => {}) + } catch { + // cleanup failure is non-fatal + } + + // The temp is gone, so the staging directory is ours to remove when no other + // write is staging in it; a non-empty rmdir leaves it for that writer. + if (stagingDirPath !== null) { + _removeStagingDirIfEmpty(stagingDirPath) + } + + if (daclDumpPath !== null) { + await fs.unlink(daclDumpPath).catch(() => {}) + } + + if (rollbackFailure !== null) { + throw new RollbackFailedError(targetPath, rollbackFailure.backupPath, rollbackFailure.error, originalError) + } + + throw originalError + } +} diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index 42786cfaa5..e4c73f064a 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -495,6 +495,21 @@ export class McpHub { return mcpServersPath } + /** + * A project-scoped MCP settings file is content the REPOSITORY controls, so a + * repository that plants .roo/mcp.json as a symlink must not receive the merged + * settings at the linked target - the payload would land outside the workspace, and + * MCP configs carry secrets and server commands. Those writes therefore refuse a + * symlink target instead of following it. The global settings file is deliberately + * left to follow a symlink: users legitimately link mcp_settings.json, it lives in + * the extension's global storage, and no repository controls that path. + * A connection with no recorded source is treated as global, matching the rest of this + * file (`conn.server.source || "global"`). + */ + private symlinkPolicyForSource(source: "global" | "project" | undefined): { refuseSymlinkTarget?: boolean } { + return source === "project" ? { refuseSymlinkTarget: true } : {} + } + async getMcpSettingsFilePath(): Promise { const provider = this.providerRef.deref() if (!provider) { @@ -2091,7 +2106,10 @@ export class McpHub { } this.isProgrammaticUpdate = true try { - await safeWriteJson(configPath, updatedConfig, { prettyPrint: true }) + await safeWriteJson(configPath, updatedConfig, { + prettyPrint: true, + ...this.symlinkPolicyForSource(source), + }) } finally { // Reset flag after watcher debounce period (non-blocking) this.flagResetTimer = setTimeout(() => { @@ -2176,7 +2194,10 @@ export class McpHub { mcpServers: config.mcpServers, } - await safeWriteJson(configPath, updatedConfig, { prettyPrint: true }) + await safeWriteJson(configPath, updatedConfig, { + prettyPrint: true, + ...this.symlinkPolicyForSource(serverSource), + }) // Update server connections with the correct source await this.updateServerConnections(config.mcpServers, serverSource) @@ -2385,7 +2406,7 @@ export class McpHub { } this.isProgrammaticUpdate = true try { - await safeWriteJson(normalizedPath, config, { prettyPrint: true }) + await safeWriteJson(normalizedPath, config, { prettyPrint: true, ...this.symlinkPolicyForSource(source) }) } finally { // Reset flag after watcher debounce period (non-blocking) this.flagResetTimer = setTimeout(() => { diff --git a/src/services/mcp/__tests__/McpHub.spec.ts b/src/services/mcp/__tests__/McpHub.spec.ts index 441b0310e6..234fa431ff 100644 --- a/src/services/mcp/__tests__/McpHub.spec.ts +++ b/src/services/mcp/__tests__/McpHub.spec.ts @@ -1052,6 +1052,96 @@ describe("McpHub", () => { expect(writtenConfig.mcpServers["test-server"].alwaysAllow).toContain("new-tool") }) + describe("symlink policy on MCP settings writes", () => { + it("refuses a symlinked target when deleteServer omits the source but the server is a project server", async () => { + vi.mocked(fs.readFile).mockResolvedValue( + JSON.stringify({ + mcpServers: { "test-server": { type: "stdio", command: "node", args: ["test.js"] } }, + }), + ) + mcpHub.connections = [ + { + type: "connected", + server: { + name: "test-server", + type: "stdio", + command: "node", + args: ["test.js"], + source: "project", + }, + client: {}, + transport: {}, + } as unknown as ConnectedMcpConnection, + ] + + // deleteServer("name") takes no source: findConnection resolves the connection and + // configPath is chosen from THAT source. The write policy must be derived from the same + // value, or a project server reached through an omitted argument loses symlink refusal. + await mcpHub.deleteServer("test-server") + + const write = vi.mocked(safeWriteJson).mock.calls.at(-1) + expect(write && write[2]).toEqual(expect.objectContaining({ refuseSymlinkTarget: true })) + }) + + it("refuses a symlinked target for a project toggleToolAlwaysAllow write", async () => { + vi.mocked(fs.readFile).mockResolvedValue( + JSON.stringify({ + mcpServers: { + "test-server": { type: "stdio", command: "node", args: ["test.js"], alwaysAllow: [] }, + }, + }), + ) + mcpHub.connections = [ + { + type: "connected", + server: { + name: "test-server", + type: "stdio", + command: "node", + args: ["test.js"], + source: "project", + }, + client: {}, + transport: {}, + } as unknown as ConnectedMcpConnection, + ] + + await mcpHub.toggleToolAlwaysAllow("test-server", "project", "new-tool", true) + + const write = vi.mocked(safeWriteJson).mock.calls.at(-1) + expect(write && write[2]).toEqual(expect.objectContaining({ refuseSymlinkTarget: true })) + }) + + it("leaves the global toggleToolAlwaysAllow write without a symlink refusal", async () => { + vi.mocked(fs.readFile).mockResolvedValue( + JSON.stringify({ + mcpServers: { + "test-server": { type: "stdio", command: "node", args: ["test.js"], alwaysAllow: [] }, + }, + }), + ) + mcpHub.connections = [ + { + type: "connected", + server: { + name: "test-server", + type: "stdio", + command: "node", + args: ["test.js"], + source: "global", + }, + client: {}, + transport: {}, + } as unknown as ConnectedMcpConnection, + ] + + await mcpHub.toggleToolAlwaysAllow("test-server", "global", "new-tool", true) + + const write = vi.mocked(safeWriteJson).mock.calls.at(-1) + expect(write && write[2]).not.toHaveProperty("refuseSymlinkTarget") + }) + }) + it("should remove tool from always allow list when disabling", async () => { const mockConfig = { mcpServers: { @@ -1772,6 +1862,75 @@ describe("McpHub", () => { }) describe("updateServerTimeout", () => { + it("refuses a symlinked target for a project-scoped timeout write", async () => { + vi.mocked(fs.readFile).mockResolvedValueOnce( + JSON.stringify({ + mcpServers: { + "test-server": { type: "stdio", command: "node", args: ["test.js"], timeout: 60 }, + }, + }), + ) + // The SDK client/transport are never touched by this write path (it reads only + // server.name and server.source), so the literal is projected onto the connection + // type through unknown rather than adding another `as any` to this file. + mcpHub.connections = [ + { + type: "connected", + server: { + name: "test-server", + type: "stdio", + command: "node", + args: ["test.js"], + timeout: 60, + source: "project", + }, + client: {}, + transport: {}, + } as unknown as ConnectedMcpConnection, + ] + + await mcpHub.updateServerTimeout("test-server", 120) + + // A project .roo/mcp.json is repository-controlled: if it is a symlink, the merged + // settings (secrets + server commands) must NOT land at the referent outside the + // workspace, so this write refuses the link instead of following it. + const write = vi.mocked(safeWriteJson).mock.calls.at(-1) + expect(write && write[2]).toEqual(expect.objectContaining({ refuseSymlinkTarget: true })) + }) + + it("leaves the global settings write without a symlink refusal", async () => { + vi.mocked(fs.readFile).mockResolvedValueOnce( + JSON.stringify({ + mcpServers: { + "test-server": { type: "stdio", command: "node", args: ["test.js"], timeout: 60 }, + }, + }), + ) + mcpHub.connections = [ + { + type: "connected", + server: { + name: "test-server", + type: "stdio", + command: "node", + args: ["test.js"], + timeout: 60, + source: "global", + }, + client: {}, + transport: {}, + } as unknown as ConnectedMcpConnection, + ] + + await mcpHub.updateServerTimeout("test-server", 120) + + // The global file lives in the extension global storage and users legitimately link + // it, so this writer does not opt into the symlink refusal; what the assertion + // covers is that the option is absent, not what the filesystem then does. + const write = vi.mocked(safeWriteJson).mock.calls.at(-1) + expect(write && write[2]).not.toHaveProperty("refuseSymlinkTarget") + }) + it("should update server timeout in settings file", async () => { const mockConfig = { mcpServers: { diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 79d08678a0..632290941d 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -4,6 +4,7 @@ import * as path from "path" import * as os from "os" import { safeWriteJson } from "../safeWriteJson" +import * as lockfile from "proper-lockfile" // Capture actual implementations before the vi.mock factory runs, // so they are never wrapped by vi.fn() — avoids infinite recursion when @@ -312,9 +313,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 +322,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() }) @@ -385,7 +390,10 @@ describe("safeWriteJson", () => { // Clean up await fs.unlink(lockTestFilePath).catch(() => {}) // Ignore errors if file doesn't exist - vi.unmock("proper-lockfile") // Ensure the mock is removed after this test + // A hoisted vi.unmock runs before this test's runtime vi.doMock, so it + // cannot remove it; doUnmock + resetModules clear the registry entry. + vi.doUnmock("proper-lockfile") + vi.resetModules() }) test("should release lock even if an error occurs mid-operation", async () => { const data = { message: "test lock release on error" } @@ -434,9 +442,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 +459,37 @@ 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 - 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 primary failure has to stay readable 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 must be actionable: the caller learns the previous content is + // recoverable and exactly 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 - 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() }) @@ -542,4 +567,435 @@ describe("safeWriteJson", () => { const content = await readFileContent(currentTestFilePath) expect(content).toEqual({ c: 3 }) }) + + // The commit rename is no-follow for the final component: it targets the path the caller + // named, so a link the caller never chose is REPLACED by the rename instead of receiving the + // payload. Staging therefore happens beside the named path - the same directory the rename + // lands in - which is also what keeps the rename on one volume. (Real symlinks are unavailable + // in this CI lane, so the alias is simulated by mocking fs.realpath.) + test("stages beside the caller-named path and never publishes through the symlink", 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 referent a resolution would hand back + const callerPath = path.join(linkDir, "test-file.json") + const referentPath = path.join(referentDir, "test-file.json") + // Seed the referent with real content: if the publish followed the alias, this is what + // would be replaced. + 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 named path, NOT beside the referent + const tempPaths = vi.mocked(fsSyncActual.createWriteStream).mock.calls.map((call) => String(call[0])) + expect(tempPaths.some((p) => p.startsWith(linkDir + path.sep) && p.includes(".new_"))).toBe(true) + expect(tempPaths.some((p) => p.startsWith(referentDir + path.sep))).toBe(false) + + // the payload landed on the path the caller named; the referent is untouched + expect(await readFileContent(callerPath)).toEqual({ after: true }) + expect(await readFileContent(referentPath)).toEqual({ seed: true }) + }) + + // proper-lockfile with realpath:false keys the lock by the given path, so a + // symlink alias and its referent must coordinate through ONE lock on the + // resolved referent — otherwise a concurrent merge through both aliases + // reads the same JSON and overwrites one update. (Real symlinks are + // unavailable in this CI lane, so the resolution is simulated by mocking + // fs.realpath, the same way as the staging test above.) + test("acquires the lock on the resolved referent, not the caller alias", async () => { + vi.resetModules() // fresh module instances so the doMock below is picked up + + const referentDir = path.join(tempDir, "lock-referent") + const linkDir = path.join(tempDir, "lock-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, "locked.json") + const referentPath = path.join(referentDir, "locked.json") + await fsPromisesActuals.writeFile!(referentPath, JSON.stringify({ seed: 1 })) + + vi.spyOn(fs, "realpath").mockResolvedValue(referentPath) + + // Wrap the real lock in a capturing mock, and drive the two rare error paths + // (the onCompromised callback and a failing release) so they stay covered + // without real lockfile staleness. The callback rethrows by design, so + // the mock swallows that throw and lets the real lock proceed. + const realLockfile = await vi.importActual("proper-lockfile") + const lockMockFn = vi.fn( + async ( + file: Parameters[0], + options?: Parameters[1], + ) => { + try { + options?.onCompromised?.(new Error("lock compromised (test)")) + } catch { + // onCompromised rethrows by design; swallow so the real lock proceeds. + } + const release = await realLockfile.lock(file, options) + return async () => { + await release() + throw new Error("release failed (test)") + } + }, + ) + const lockMock = lockMockFn as unknown as typeof realLockfile.lock + vi.doMock("proper-lockfile", () => ({ + ...realLockfile, + lock: lockMock, + })) + + // Re-import safeWriteJson so it picks up the mocked proper-lockfile. + const { safeWriteJson: mockedSafeWriteJson } = await import("../safeWriteJson") + + const mergeFn = vi.fn((existing: unknown, incoming: unknown) => ({ + ...(existing as Record), + ...(incoming as Record), + })) + + // Capture the compromise + release-failure logs. + const consoleErrorSpy = vi.spyOn(console, "error") + await mockedSafeWriteJson(callerPath, { added: true }, { merge: mergeFn }) + + // The lock was keyed by the resolved referent — every alias shares it. + expect(lockMock).toHaveBeenCalledTimes(1) + expect(String(lockMockFn.mock.calls[0][0])).toBe(referentPath) + // The merge reads the path the caller named, not the referent: the publish is no-follow, + // so the referent's content is never read or written through the alias. + expect(mergeFn).toHaveBeenCalledTimes(1) + expect(await readFileContent(callerPath)).toEqual({ added: true }) + expect(await readFileContent(referentPath)).toEqual({ seed: 1 }) + // The compromise callback and the failed release were logged, not thrown. + expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining("was compromised"), expect.any(Error)) + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.stringContaining("Failed to release lock"), + expect.any(Error), + ) + + // The hoisted vi.unmock runs before this test's runtime vi.doMock, so it + // cannot remove it; doUnmock + resetModules clear the registry entry so + // later test files import the real proper-lockfile. + vi.doUnmock("proper-lockfile") + vi.resetModules() + }) + + // 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 }) + }, + ) + // A settings export carries API credentials, so it must not be redirected through + // a link the user never chose. (Real symlinks are unavailable in this CI lane, so + // the link is simulated by mocking fs.lstat.) + test("refuses to publish through a symlink when refuseSymlinkTarget is set", async () => { + const referentPath = path.join(tempDir, "refuse-referent.json") + const linkPath = path.join(tempDir, "refuse-link.json") + await fsPromisesActuals.writeFile!(referentPath, JSON.stringify({ seed: "untouched" })) + + vi.spyOn(fs, "lstat").mockResolvedValue({ + isSymbolicLink: () => true, + // The guard reads only isSymbolicLink(), and a real Stats cannot be + // produced for a simulated link in this CI lane, so the double is + // asserted through unknown rather than stubbing every Stats field. + } as unknown as fsSyncActual.Stats) + + await expect(safeWriteJson(linkPath, { leaked: true }, { refuseSymlinkTarget: true })).rejects.toThrow( + /refusing to write through the symlink/, + ) + + vi.restoreAllMocks() + // Nothing was resolved, staged, locked, or committed: the referent still holds + // the content it had before the refused write. + expect(await readFileContent(referentPath)).toEqual({ seed: "untouched" }) + }) + + test("still writes a regular file when refuseSymlinkTarget is set", async () => { + const target = path.join(tempDir, "refuse-regular.json") + await safeWriteJson(target, { written: true }, { refuseSymlinkTarget: true }) + expect(await readFileContent(target)).toEqual({ written: true }) + }) + + test("rejects when the destination is swapped for a link after the initial refusal check", async () => { + const referentPath = path.join(tempDir, "swap-referent.json") + const linkPath = path.join(tempDir, "swap-link.json") + await fsPromisesActuals.writeFile!(referentPath, JSON.stringify({ seed: "untouched" })) + await fsPromisesActuals.writeFile!(linkPath, JSON.stringify({ own: true })) + const asLink = { isSymbolicLink: () => true } as unknown as fsSyncActual.Stats + const asFile = { isSymbolicLink: () => false } as unknown as fsSyncActual.Stats + // The first lstat (the refusal) sees a regular file; the re-check after + // resolvePublishTarget sees the link a local writer installed in between. + // Path-aware rather than call-order: the ancestor walk also calls lstat, so a + // mockResolvedValueOnce chain would be consumed by the wrong component. The first look at + // the target sees a regular file; the re-check after resolvePublishTarget sees the link. + let targetLooks = 0 + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + if (String(p) === linkPath) { + targetLooks++ + return Promise.resolve(targetLooks === 1 ? asFile : asLink) + } + return Promise.resolve(asFile) + }) as unknown as typeof fs.lstat) + + await expect(safeWriteJson(linkPath, { leaked: true }, { refuseSymlinkTarget: true })).rejects.toThrow( + /after resolution/, + ) + + vi.restoreAllMocks() + expect(await readFileContent(referentPath)).toEqual({ seed: "untouched" }) + }) + + test("rejects when the destination becomes a link before the commit is staged", async () => { + const referentPath = path.join(tempDir, "late-referent.json") + const linkPath = path.join(tempDir, "late-link.json") + await fsPromisesActuals.writeFile!(referentPath, JSON.stringify({ seed: "untouched" })) + await fsPromisesActuals.writeFile!(linkPath, JSON.stringify({ own: true })) + const asLink = { isSymbolicLink: () => true } as unknown as fsSyncActual.Stats + const asFile = { isSymbolicLink: () => false } as unknown as fsSyncActual.Stats + // The swap happens after resolution and while the write is already under the + // lock: the in-lock re-check must stop the commit rename. + // Same path-aware scripting: the swap happens after resolution and while the write is + // already under the lock, so the third look at the target is the one that must be a link. + let lateLooks = 0 + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + if (String(p) === linkPath) { + lateLooks++ + return Promise.resolve(lateLooks <= 2 ? asFile : asLink) + } + return Promise.resolve(asFile) + }) as unknown as typeof fs.lstat) + + await expect(safeWriteJson(linkPath, { leaked: true }, { refuseSymlinkTarget: true })).rejects.toThrow( + /before publication/, + ) + + vi.restoreAllMocks() + expect(await readFileContent(referentPath)).toEqual({ seed: "untouched" }) + }) + + test.each([ + ["EACCES", Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" })], + ["a code-less error", new Error("lstat exploded")], + ])("fails closed when the refusal check fails with %s", async (_label, failure) => { + const target = path.join(tempDir, "refuse-lstat-failure.json") + // A stat error that is not ENOENT is not evidence that the destination is safe, so + // the write has to stop here rather than publish through an unexamined entry. + vi.spyOn(fs, "lstat").mockRejectedValue(failure) + + await expect(safeWriteJson(target, { written: true }, { refuseSymlinkTarget: true })).rejects.toThrow( + failure.message, + ) + + vi.restoreAllMocks() + // Nothing was locked, staged, or published: no target and no leftover temp file. + expect(await fileExists(target)).toBe(false) + const leftovers = fsSyncActual.readdirSync(tempDir).filter(function (entry) { + return entry.includes("refuse-lstat-failure") + }) + expect(leftovers).toEqual([]) + }) + + test("publishes onto the named path when a link is planted during target resolution", async () => { + const referentPath = path.join(tempDir, "race-referent.json") + const namedPath = path.join(tempDir, "race-named.json") + await fsPromisesActuals.writeFile!(referentPath, JSON.stringify({ seed: "untouched" })) + await fsPromisesActuals.writeFile!(namedPath, JSON.stringify({ own: true })) + const asFile = { isSymbolicLink: () => false } as unknown as fsSyncActual.Stats + // Every lstat in the sequence sees a regular file: the link a local writer plants + // while the destination is being resolved is gone again before the next check, so + // no check in the sequence can catch it. + vi.spyOn(fs, "lstat").mockResolvedValue(asFile) + // Resolution is the step that would hand back a destination the caller never chose. + const realpathSpy = vi.spyOn(fs, "realpath").mockResolvedValue(referentPath) + + await safeWriteJson(namedPath, { written: true }, { refuseSymlinkTarget: true }) + + vi.restoreAllMocks() + // With refuseSymlinkTarget the named path is the publish target, so resolution never + // runs and there is no window in which a planted link can redirect the payload. The + // commit is a rename, which replaces the named directory entry instead of writing + // through a link, so the referent cannot receive the export. + expect(realpathSpy).not.toHaveBeenCalled() + expect(await readFileContent(namedPath)).toEqual({ written: true }) + expect(await readFileContent(referentPath)).toEqual({ seed: "untouched" }) + }) + + test("resolves the publish target only once during a guarded publication", async () => { + const target = path.join(tempDir, "single-resolve.json") + // The caller resolves once for the lock key; safeWriteText must not resolve again, + // or a link installed after the caller's re-check would be followed there. + const realpath = vi.spyOn(fs, "realpath").mockImplementation(async (p) => String(p)) + + await safeWriteJson(target, { written: true }) + + expect(realpath).toHaveBeenCalledTimes(1) + vi.restoreAllMocks() + expect(await readFileContent(target)).toEqual({ written: true }) + }) + + test("refuses a write when an ancestor directory is a symlink", async () => { + const target = path.join(tempDir, "ancestor-link.json") + await fsPromisesActuals.writeFile!(target, JSON.stringify({ own: true })) + const asLink = { isSymbolicLink: () => true } as unknown as fsSyncActual.Stats + const asFile = { isSymbolicLink: () => false } as unknown as fsSyncActual.Stats + // The target itself looks ordinary; the link sits one directory above it. Checking only the + // final component would publish a credential payload outside the directory the caller named. + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + return Promise.resolve(String(p) === tempDir ? asLink : asFile) + }) as unknown as typeof fs.lstat) + + await expect(safeWriteJson(target, { leaked: true }, { refuseSymlinkTarget: true })).rejects.toThrow( + /is a symlink/, + ) + + vi.restoreAllMocks() + expect(await readFileContent(target)).toEqual({ own: true }) + }) + + // The initial walk and the commit rename are separated by the whole staging sequence, and + // Node exposes no handle-relative rename: an ancestor that passed the walk can be swapped + // while the payload is staged, and the path-based commit rename would follow it. The + // publication therefore re-derives the final entry and every recorded ancestor identity + // immediately before the commit rename. (Real symlinks are unavailable in this CI lane, so + // the swap is simulated by scripting fs.lstat per visit, the same way as the tests above.) + test("refuses the publish when an ancestor is swapped for a link after the initial walk", async () => { + const parent = path.join(tempDir, "swap-parent") + await fs.mkdir(parent, { recursive: true }) + const target = path.join(parent, "swap-ancestor.json") + await fsPromisesActuals.writeFile!(target, JSON.stringify({ own: true })) + // The guard reads isSymbolicLink plus the bigint identity, and a real BigIntStats cannot + // be produced for a simulated directory in this CI lane, so the doubles are asserted + // through unknown rather than stubbing every Stats field. + const asDir = { isSymbolicLink: () => false, dev: 1n, ino: 1n } as unknown as fsSyncActual.BigIntStats + const asLink = { isSymbolicLink: () => true, dev: 2n, ino: 2n } as unknown as fsSyncActual.BigIntStats + // Path-aware scripting: the first look at the parent is the walk that authorizes it; the + // next look happens inside the publication, where the swap must be caught. + let parentLooks = 0 + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + if (String(p) === parent) { + parentLooks++ + return Promise.resolve(parentLooks === 1 ? asDir : asLink) + } + return Promise.resolve(asDir) + }) as unknown as typeof fs.lstat) + + await expect(safeWriteJson(target, { leaked: true }, { refuseSymlinkTarget: true })).rejects.toThrow( + /is now a symlink/, + ) + + vi.restoreAllMocks() + // The re-check ran (the walk saw the directory once, the publication once more), the + // commit rename never happened, and the payload did not escape: the target still holds + // its own content and the staged temp was discarded, not left beside it. + expect(parentLooks).toBe(2) + expect(await readFileContent(target)).toEqual({ own: true }) + expect(fsSyncActual.readdirSync(parent)).toEqual(["swap-ancestor.json"]) + }) + + test("refuses the publish when an ancestor is replaced by a different directory", async () => { + const parent = path.join(tempDir, "repl-parent") + await fs.mkdir(parent, { recursive: true }) + const target = path.join(parent, "replaced-ancestor.json") + await fsPromisesActuals.writeFile!(target, JSON.stringify({ own: true })) + const asDir = { isSymbolicLink: () => false, dev: 1n, ino: 1n } as unknown as fsSyncActual.BigIntStats + // The rename-away-and-recreate shape: the name is unchanged and the entry is still a + // directory, so only the identity tells the two apart. + const asOtherDir = { isSymbolicLink: () => false, dev: 9n, ino: 9n } as unknown as fsSyncActual.BigIntStats + let parentLooks = 0 + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + if (String(p) === parent) { + parentLooks++ + return Promise.resolve(parentLooks === 1 ? asDir : asOtherDir) + } + return Promise.resolve(asDir) + }) as unknown as typeof fs.lstat) + + await expect(safeWriteJson(target, { leaked: true }, { refuseSymlinkTarget: true })).rejects.toThrow( + /is no longer the directory that was checked/, + ) + + vi.restoreAllMocks() + expect(parentLooks).toBe(2) + expect(await readFileContent(target)).toEqual({ own: true }) + expect(fsSyncActual.readdirSync(parent)).toEqual(["replaced-ancestor.json"]) + }) + + test("publishes when every recorded ancestor keeps its identity", async () => { + const parent = path.join(tempDir, "stable-parent") + await fs.mkdir(parent, { recursive: true }) + const target = path.join(parent, "stable-ancestor.json") + await fsPromisesActuals.writeFile!(target, JSON.stringify({ own: true })) + // Pinning must not reject the ordinary case: every ancestor keeps the identity the walk + // recorded, so the publication proceeds. + vi.spyOn(fs, "lstat").mockResolvedValue( + { isSymbolicLink: () => false, dev: 1n, ino: 1n } as unknown as fsSyncActual.BigIntStats, + ) + + await safeWriteJson(target, { written: true }, { refuseSymlinkTarget: true }) + + vi.restoreAllMocks() + expect(await readFileContent(target)).toEqual({ written: true }) + }) + + test("does not apply the ancestor refusal when refuseSymlinkTarget is not set", async () => { + const target = path.join(tempDir, "default-ancestor.json") + await fsPromisesActuals.writeFile!(target, JSON.stringify({ own: true })) + const asLink = { isSymbolicLink: () => true } as unknown as fsSyncActual.Stats + const asFile = { isSymbolicLink: () => false } as unknown as fsSyncActual.Stats + // The ancestor walk is part of the credential-write policy, not a global behavior change: + // callers that did not opt into refuseSymlinkTarget keep their previous semantics. + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + return Promise.resolve(String(p) === tempDir ? asLink : asFile) + }) as unknown as typeof fs.lstat) + + await safeWriteJson(target, { written: true }) + + vi.restoreAllMocks() + expect(await readFileContent(target)).toEqual({ written: true }) + }) + + test("fails closed when an ancestor directory cannot be inspected", async () => { + const target = path.join(tempDir, "ancestor-eacces.json") + await fsPromisesActuals.writeFile!(target, JSON.stringify({ own: true })) + const asFile = { isSymbolicLink: () => false } as unknown as fsSyncActual.Stats + const failure = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + // "Could not inspect" is not evidence that the path is safe: only ENOENT is tolerated. + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + if (String(p) === tempDir) { + return Promise.reject(failure) + } + return Promise.resolve(asFile) + }) as unknown as typeof fs.lstat) + + await expect(safeWriteJson(target, { leaked: true }, { refuseSymlinkTarget: true })).rejects.toThrow(/EACCES/) + + vi.restoreAllMocks() + expect(await readFileContent(target)).toEqual({ own: true }) + }) + + test("does not resolve the publish target when refuseSymlinkTarget is set", async () => { + const target = path.join(tempDir, "no-resolve.json") + // Resolving is what turns a planted link into a destination the caller never chose, + // so the credential-bearing path skips it entirely and publishes onto the name the + // caller gave. + const realpath = vi.spyOn(fs, "realpath").mockImplementation(async (p) => String(p)) + + await safeWriteJson(target, { written: true }, { refuseSymlinkTarget: true }) + + expect(realpath).not.toHaveBeenCalled() + vi.restoreAllMocks() + expect(await readFileContent(target)).toEqual({ written: true }) + }) }) diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index 7da68b2a7a..fcc7a28098 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -3,6 +3,8 @@ import * as fsSync from "fs" import * as path from "path" import { JsonStreamStringify } from "json-stream-stringify" +import { resolvePublishTarget, safeWriteText, type SafeWriteTextOptions } from "../services/file-safety/safeWriteText" + import { acquireFileLock } from "./fileLock" /** @@ -26,13 +28,46 @@ export interface SafeWriteJsonOptions { * cannot be parsed. */ merge?: (existing: unknown, incoming: unknown) => unknown + + /** + * Refuse to publish through a symlink at the target path. + * + * Two things differ by default and should not be conflated: the advisory LOCK is keyed to + * the resolved referent, which is what keeps every alias of one file behind a single lock, + * while the PUBLISH stays on the path the caller named - the commit is a rename, and a + * rename replaces that directory entry, so a symlink there is replaced rather than followed + * and its referent never receives the payload. + * That is the wrong default for a payload whose destination the user chose - + * settings exports carry API credentials - where following a link they never + * pointed at would write secrets into a file they did not pick. When this is + * set, a symlink at the final path component is an error instead. + * + * The refusal is not a one-time inspection: the final entry and every ancestor + * directory identity are re-derived inside the publication, immediately before + * the commit rename, so a directory swapped for a link during the staging + * sequence is caught before it can redirect the payload. Node exposes no + * handle-relative rename, so the window that remains is the commit rename + * itself, not the checks before it. + */ + refuseSymlinkTarget?: boolean +} + +/** + * An ancestor directory that passed the symlink refusal, recorded with the + * bigint device and inode it had at that moment so the publication can tell + * whether it is still the same directory before committing. + */ +interface CheckedDirectory { + dir: string + dev: bigint + ino: bigint } /** * 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. @@ -42,7 +77,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 @@ -52,28 +86,185 @@ 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) throw dirError } + // Every existing ancestor must be checked too, not just the final component: a symlinked + // directory above the target redirects the payload without leaving a trace on the target path + // itself (e.g. /.roo -> a directory outside the workspace). Only ENOENT stops the + // walk - a missing ancestor means nothing deeper exists to be a link. Any other inspection + // error fails closed, because "could not inspect" is not evidence that the path is safe. + // Each directory that passes is recorded with its bigint device and inode: a walk the + // publication does not re-derive is a refusal an attacker retires by swapping the directory + // after the walk ran, so the identities feed the pre-commit re-check below. + async function _refuseSymlinkedAncestors(absoluteFilePath: string): Promise { + const checked: CheckedDirectory[] = [] + let current = path.dirname(absoluteFilePath) + for (;;) { + let st: fsSync.BigIntStats + try { + st = await fs.lstat(current, { bigint: true }) + } catch (error: unknown) { + const code = + error && typeof error === "object" && "code" in error + ? (error as { code?: string }).code + : undefined + if (code === "ENOENT") { + return checked + } + throw error + } + if (st.isSymbolicLink()) { + throw new Error( + `safeWriteJson: refusing to write to ${absoluteFilePath}: ${current} is a symlink, and the payload would be written outside the directory the caller named.`, + ) + } + checked.push({ dir: current, dev: st.dev, ino: st.ino }) + const parent = path.dirname(current) + if (parent === current) { + return checked + } + current = parent + } + } + + // A credential-bearing payload must not be redirected through a link the user + // never chose: check the final path component before anything is resolved, + // staged, or locked. + let checkedAncestors: CheckedDirectory[] = [] + if (options?.refuseSymlinkTarget) { + let targetStat: fsSync.Stats | undefined + try { + targetStat = await fs.lstat(absoluteFilePath) + } catch (error: unknown) { + const code = + error && typeof error === "object" && "code" in error ? (error as { code?: string }).code : undefined + // Only a missing target means there is no link to refuse. Anything else - + // a permission error on the parent directory, for example - is not evidence + // that the destination is safe to publish into. + if (code !== "ENOENT") { + throw error + } + } + if (targetStat?.isSymbolicLink()) { + throw new Error( + `safeWriteJson: refusing to write through the symlink at ${absoluteFilePath}; the payload would land on its referent instead of the destination the user chose.`, + ) + } + + // The final component alone is not enough: a symlinked ancestor redirects the payload while + // leaving the target path looking ordinary. Checked before anything is resolved, staged or + // locked, and it fails closed on any inspection error that is not ENOENT. The identities it + // records are re-derived by the publication hook below - this walk alone leaves the whole + // staging window open to an ancestor swap. + checkedAncestors = await _refuseSymlinkedAncestors(absoluteFilePath) + } + + // Two paths, for two different jobs: lock identity and publication destination. + // The LOCK is keyed to the resolved referent, because proper-lockfile keys by the path it + // is given: an alias and its referent would otherwise take two locks for one underlying + // file, and a concurrent merge through both aliases could read the same JSON and overwrite + // one update. Resolution happens before the lock for that reason, and resolvePublishTarget + // tolerates a not-yet-existing file (it returns the given path on ENOENT), preserving the + // create-from-absent flow. + // The PUBLISH stays on the caller-named path in both modes: the commit is a rename, and a + // rename replaces the directory entry rather than writing through a link, so a link the + // caller never chose gets replaced instead of receiving the payload. That is the + // pre-existing behaviour - publishing onto the referent instead would be a new default for + // every caller. With refuseSymlinkTarget the named path is also the lock path: resolving it + // here would hand back a referent the caller never chose if a link is planted between the + // refusal check and this resolution, and every such writer keys its lock to the same named + // path, so the lock still serializes all writers to that entry. + const lockTargetPath = options?.refuseSymlinkTarget + ? absoluteFilePath + : await resolvePublishTarget(absoluteFilePath) + const publishTargetPath = absoluteFilePath + + // The refusal above and this resolution are separate syscalls, so a local writer + // could replace the final component with a link in between; the resolved path + // would then describe a destination the caller never chose. Re-check the component + // the caller named - once here and again under the lock before publishing - so the + // refusal stays effective through publication. + const assertFinalComponentNotReplaced = async (stage: string): Promise => { + // Fail closed: only ENOENT (nothing there that could be a link) is tolerated. A lstat + // failing for another reason - EACCES on the parent directory, for example - says + // nothing about whether the entry is safe, so the write stops instead of publishing + // blind through an unexamined destination. + let nowStat: fsSync.Stats | undefined + try { + nowStat = await fs.lstat(absoluteFilePath) + } catch (error: unknown) { + const code = + error && typeof error === "object" && "code" in error ? (error as { code?: string }).code : undefined + if (code !== "ENOENT") { + throw error + } + } + if (nowStat?.isSymbolicLink()) { + throw new Error( + `safeWriteJson: refusing to write through the symlink now at ${absoluteFilePath} (${stage}); the payload would land at ${publishTargetPath}, a destination the caller never chose.`, + ) + } + } + if (options?.refuseSymlinkTarget) { + await assertFinalComponentNotReplaced("after resolution") + } + + // The initial checks and the commit rename are separated by the whole staging sequence, and + // Node exposes no handle-relative rename (no renameat): a local writer that swaps an + // inspected ancestor for a link - or for a different directory - during that window would + // still redirect the path-based commit rename, and no recheck of the final component would + // notice, because the swap is one directory up. The publication therefore re-derives the + // complete ancestry itself - the final entry and every recorded ancestor identity - + // immediately before the commit rename, so the refusal is enforced at the action it guards. + // What cannot be closed without a native renameat is stated rather than claimed as + // protection: the window that remains is the commit rename itself. + const assertAncestryUnchangedBeforeCommit = async (): Promise => { + await assertFinalComponentNotReplaced("before publication") + for (const expected of checkedAncestors) { + let nowStat: fsSync.BigIntStats + try { + nowStat = await fs.lstat(expected.dir, { bigint: true }) + } catch (error: unknown) { + const code = + error && typeof error === "object" && "code" in error + ? (error as { code?: string }).code + : undefined + // The directory the walk recorded is gone: the ancestry the refusal was based on + // no longer exists, and a path-based rename would resolve a chain nobody checked. + if (code === "ENOENT") { + throw new Error( + `safeWriteJson: refusing to publish to ${absoluteFilePath}: ${expected.dir} no longer exists; an ancestor of the destination was replaced after it was checked.`, + ) + } + throw error + } + if (nowStat.isSymbolicLink()) { + throw new Error( + `safeWriteJson: refusing to publish to ${absoluteFilePath}: ${expected.dir} is now a symlink; the payload would be written outside the directory the caller named.`, + ) + } + if (nowStat.dev !== expected.dev || nowStat.ino !== expected.ino) { + throw new Error( + `safeWriteJson: refusing to publish to ${absoluteFilePath}: ${expected.dir} is no longer the directory that was checked; an ancestor of the destination was replaced after it was checked.`, + ) + } + } + } + // Acquire the lock before any file operations. `acquireFileLock` owns the - // shared advisory lock protocol, so callers that lock the same path with - // it (for example task-history deletion) serialize with this write. - // If lock acquisition fails, it throws immediately. The releaseLock - // remains a no-op, so the finally block in the main file operations - // try-catch-finally won't try to release an unacquired lock if this - // path is taken. - releaseLock = await acquireFileLock(absoluteFilePath) - - // Variables to hold the actual paths of temp files if they are created. + // shared advisory lock protocol, so callers that lock the same path with it + // (for example task-history deletion) serialize with this write. It locks the + // resolved publish target, which is the key every other writer to this file + // uses. If acquisition fails it throws immediately, so the finally block never + // releases an unacquired lock. + releaseLock = await acquireFileLock(lockTargetPath) + // 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 @@ -82,7 +273,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(publishTargetPath, "utf8")) } catch (error: unknown) { const code = error && typeof error === "object" && "code" in error ? (error as { code: string }).code : undefined @@ -93,79 +284,56 @@ 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 path being published (publishTargetPath, the caller-named one): + // safeWriteText commits by renaming onto that same entry, and a rename across + // filesystems would fail with EXDEV, so the staging directory has to be the one the + // commit lands in - not the directory of the lock key. actualTempNewFilePath = path.join( - path.dirname(absoluteFilePath), - `.${path.basename(absoluteFilePath)}.new_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`, + path.dirname(publishTargetPath), + ".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, + // This call already resolved the target (and re-checks the final component + // under the lock through the hook below). safeWriteText must not resolve it a + // second time: a link installed in that window would be followed there and the + // payload committed to the attacker's referent. + targetPathIsResolved: true, } - // Step 3: Rename the new temporary file to the target file path. - // This is the main "commit" step. - await fs.rename(actualTempNewFilePath, absoluteFilePath) + if (options?.refuseSymlinkTarget) { + // Re-derive the refusal inside the publication, immediately before the backup + // rename and the commit rename: the initial walk and this call are separated by + // the whole staging sequence, and a path-based rename follows an ancestor that + // was swapped for a link in that window. A rejection runs before anything is + // moved, so the target keeps its content and the staged temp is discarded. + textOptions.verifyBeforeCommit = assertAncestryUnchangedBeforeCommit + } + await safeWriteText(publishTargetPath, "", 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) + console.error(`Operation failed for ${publishTargetPath}: [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) @@ -177,27 +345,13 @@ 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) + console.error(`Failed to release lock for ${lockTargetPath}:`, unlockError) } } }