diff --git a/knip.json b/knip.json index 1f4956faa6..1dcb5c47ba 100644 --- a/knip.json +++ b/knip.json @@ -16,7 +16,12 @@ "playwright-ct": false, "workspaces": { "src": { - "entry": ["extension.ts!", "extension/api.ts!", "workers/countTokens.ts!"], + "entry": [ + "extension.ts!", + "extension/api.ts!", + "workers/countTokens.ts!", + "services/file-safety/safeWriteText.ts!" + ], "project": [ "**/*.ts!", "!**/__tests__/**!", @@ -26,6 +31,7 @@ "!utils/WorkspacePathResolver.ts!", "!test-utils/**!", "!utils/versionToken.ts!", + "!core/tools/guardedWrite.ts!", "!services/tools/Bm25Ranker.ts!", "!services/tools/types.ts!", "!core/tools/helpers/toolResultFormatting.ts!" diff --git a/src/core/config/__tests__/importExport.spec.ts b/src/core/config/__tests__/importExport.spec.ts index 6c66655325..ae83bc7d0f 100644 --- a/src/core/config/__tests__/importExport.spec.ts +++ b/src/core/config/__tests__/importExport.spec.ts @@ -1616,10 +1616,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 () => { @@ -1648,10 +1652,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 () => { @@ -1771,10 +1779,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 () => { @@ -1924,10 +1936,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 () => { @@ -1967,10 +1983,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 () => { @@ -2012,10 +2032,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 }, + ) }) }) @@ -2249,10 +2273,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 9882039ad3..edfbf0d7a6 100644 --- a/src/core/config/importExport.ts +++ b/src/core/config/importExport.ts @@ -346,7 +346,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 b911479bc6..db364fc1c1 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -107,6 +107,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 { DispatchState, type PendingWork } from "./dispatchState" @@ -302,6 +303,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 @@ -1464,6 +1466,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 }) @@ -3696,6 +3699,12 @@ export class Task extends EventEmitter implements TaskLike { this.cancelAssistantMessagePersistence() this.cancelPendingPartialMessageUpdate() + // 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() + // Release handler-owned host resources (e.g. VS Code configuration listeners). // This is the fallback for leases that never settle; later lease releases // find nothing retired and so cannot dispose twice. diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index c1ee5a0b13..64067e18a7 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -1496,6 +1496,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("create", () => { it("rejects missing task inputs before constructing the task", () => { const onCreated = vi.fn() 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 e4e69b30ad..19827009db 100644 --- a/src/core/tools/ReadFileTool.ts +++ b/src/core/tools/ReadFileTool.ts @@ -17,6 +17,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" @@ -215,12 +216,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}`, }) @@ -770,6 +788,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 @@ -801,6 +822,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..96adb58605 --- /dev/null +++ b/src/core/tools/__tests__/guardedWrite.spec.ts @@ -0,0 +1,778 @@ +/** + * 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 not read yet — read the file, 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 (observed + absent)", () => { + it("creates the file when it vanished after the read", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("vanished.txt"), "v1") + const task = createMockTask({ observationRegistry: reg }) + + // Observed + absent is the create row of the #1399 decision table: the file the + // caller read was deleted before this write, so the guard recreates it. + mockedComputeVersionToken.mockRejectedValue({ code: "ENOENT" }) + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) + + await guardedWrite(task, "vanished.txt", "next", "update") + + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("vanished.txt"), "next", { + verifyBeforeCommit: expect.any(Function), + }) + }) + + 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 not read yet — read the file, 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 — 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 — 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 — re-read the file, then retry", + ) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + + it("recreates when an updated file vanished after the read", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("doc.txt"), "v1") + const task = createMockTask({ observationRegistry: reg }) + // Observed + absent is the create row of the #1399 decision table: the + // file vanished between the read and this write, so the guard recreates it. + mockedComputeVersionToken.mockRejectedValue({ code: "ENOENT" }) + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) + + await guardedWrite(task, "doc.txt", "new content", "update") + + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("doc.txt"), "new content", { + verifyBeforeCommit: expect.any(Function), + }) + }) + }) + + 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 — 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 — 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 not read yet — read the file, 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 — 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 a non-ENOENT token failure from the pre-commit verification", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("raced.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + const failure = { code: "EACCES" } + const published = mockPublishWithRace(() => { + // A real I/O failure in the window is not a guard verdict: it must propagate. + mockedComputeVersionToken.mockRejectedValue(failure) + }) + + await expect(guardedWrite(task, "raced.txt", "mine", "update")).rejects.toBe(failure) + 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 — 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 52e04415ea..0872809396 100644 --- a/src/core/tools/__tests__/readFileTool.spec.ts +++ b/src/core/tools/__tests__/readFileTool.spec.ts @@ -14,6 +14,9 @@ import os from "os" 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, ToolExecutionContext } from "../../task/Task" @@ -21,6 +24,7 @@ import type { Task, ToolExecutionContext } from "../../task/Task" import { isBinaryFile } from "isbinaryfile" import { readFileTool } from "../ReadFileTool" +import { ObservationRegistry } from "../../task/observationRegistry" import { formatResponse } from "../../prompts/responses" import { validateImageForProcessing, processImageFile, isSupportedImageFormat } from "../helpers/imageHelpers" import { extractTextFromFile, getSupportedBinaryFormats } from "../../../integrations/misc/extract-text" @@ -135,6 +139,7 @@ interface MockTaskOptions { rooIgnoreAllowed?: boolean maxImageFileSize?: number maxTotalImageSize?: number + observationRegistry?: ObservationRegistry } function createMockTask(options: MockTaskOptions = {}) { @@ -142,6 +147,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 }, @@ -188,7 +196,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({ @@ -1688,5 +1707,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..ac39ac1875 --- /dev/null +++ b/src/core/tools/guardedWrite.ts @@ -0,0 +1,402 @@ +/** + * 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 { + // Exact remediation text required by the S4a tracking issue (#1399). + return new GuardRejectedError("file not read yet — read the file, then retry", absolutePath) +} + +/** Build the stale-version remediation error. */ +function staleVersionError(absolutePath: string, expectedVersion: string, currentVersion: string): GuardRejectedError { + // Exact remediation text required by #1399; the version detail is diagnostic only. + void expectedVersion + void currentVersion + return new GuardRejectedError("stale version — 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") { + // Observed + absent is a create row of the #1399 decision table: the file the + // caller read was deleted before this write, so creating it is the safe action. + await createIfAbsent(absolutePath, content) + return + } + // 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 { + // Exact remediation text required by #1399. + 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 42be14463c..cb6575a316 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -861,7 +861,7 @@ }, "core/tools/__tests__/readFileTool.spec.ts": { "@typescript-eslint/no-explicit-any": { - "count": 96 + "count": 95 } }, "core/tools/__tests__/runSlashCommandTool.spec.ts": { 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..afaee31e2d --- /dev/null +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -0,0 +1,989 @@ +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 } 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("rollback failure", () => { + it("renders non-Error failures through the rollback message", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.closeSync).mockReturnValue(undefined) + // Non-Error throwables are legal in JavaScript: the message must still render both + // the primary and rollback failures instead of failing to build the text. + vi.mocked(fs.rename) + .mockResolvedValueOnce(undefined) + .mockRejectedValueOnce("commit failed") + .mockRejectedValueOnce("rollback failed") + + const failure = await safeWriteText(targetPath, "hello", { backup: true, platform: "linux" }).then( + () => undefined, + (error: unknown) => error as Error & { name: string; cause?: unknown }, + ) + + expect(failure?.name).toBe("RollbackFailedError") + expect(failure?.message).toContain("commit failed") + expect(failure?.message).toContain("rollback failed") + expect(failure?.cause).toBe("rollback failed") + }) + + it("throws RollbackFailedError with the backup path when the rollback rename also fails", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.closeSync).mockReturnValue(undefined) + // backup rename succeeds, the commit rename fails, and the rollback rename fails. + vi.mocked(fs.rename) + .mockResolvedValueOnce(undefined) + .mockRejectedValueOnce(new Error("commit failed")) + .mockRejectedValueOnce(new Error("rollback failed")) + + const failure = await safeWriteText(targetPath, "hello", { backup: true, platform: "linux" }).then( + () => undefined, + (error: unknown) => + error as Error & { + name: string + filePath: string + backupPath: string + originalError?: Error + cause?: Error + }, + ) + + // The caller must learn both the primary failure and where the previous content + // survived: the backup path is the only way to recover it. + expect(failure?.name).toBe("RollbackFailedError") + expect(failure?.filePath).toBe(targetPath) + expect(failure?.backupPath).toContain(".safeWriteText.bak_") + expect(failure?.originalError?.message).toBe("commit failed") + expect(failure?.cause?.message).toBe("rollback failed") + expect(failure?.message).toContain("commit failed") + expect(failure?.message).toContain("rollback failed") + // The failure path still cleans up: the staged temp is unlinked and the staging + // directory is released, so a double-failure does not leave write artifacts behind. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + expect(fsSync.rmdirSync).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + expect(failure?.message).toContain(failure?.backupPath) + }) + }) + + 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) + + // Ordering contract: the content is fsynced, the descriptor closed, and only then + // the temp is renamed over the target, so a crash cannot publish unflushed bytes. + const fsyncOrder = vi.mocked(fsSync.fsyncSync).mock.invocationCallOrder[0] + const closeOrder = vi.mocked(fsSync.closeSync).mock.invocationCallOrder[0] + const renameOrder = vi.mocked(fs.rename).mock.invocationCallOrder[0] + expect(fsyncOrder).toBeLessThan(closeOrder) + expect(closeOrder).toBeLessThan(renameOrder) + + // 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): + // pin the commit rename onto the target - a rename to any other path + // would silently publish elsewhere - and pin that a failed save skips + // the restore step entirely. + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + expect(execFile).toHaveBeenCalledTimes(1) + }) + + 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): the commit rename ran and + // the saved DACL dump was still unlinked in the finally block. + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + expect(execFile).toHaveBeenCalledTimes(2) + 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 c973f35a03..48ae12ba4a 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -505,6 +505,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) { @@ -2099,7 +2114,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(() => { @@ -2184,7 +2202,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) @@ -2393,7 +2414,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 35ff10d64b..b89c55bc8d 100644 --- a/src/services/mcp/__tests__/McpHub.spec.ts +++ b/src/services/mcp/__tests__/McpHub.spec.ts @@ -35,6 +35,7 @@ vi.mock("fs/promises", () => { }) // Import safeWriteJson to use in mocks +import { safeWriteJson } from "../../../utils/safeWriteJson" // Mock safeWriteJson vi.mock("../../../utils/safeWriteJson", () => ({ @@ -1255,6 +1256,106 @@ 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"] } }, + }), + ) + // Typed close doubles: updateServerConnections tears the connection + // down through transport.close() and client.close(). Empty doubles + // raise a TypeError that deleteConnection swallows, so the teardown + // would silently do nothing and the test would still pass. + const transportClose = vi.fn(async () => {}) + const clientClose = vi.fn(async () => {}) + mcpHub.connections = [ + { + type: "connected", + server: { + name: "test-server", + type: "stdio", + command: "node", + args: ["test.js"], + source: "project", + }, + client: { close: clientClose }, + transport: { close: transportClose }, + } 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") + + // The teardown ran to completion rather than dying on a TypeError. + expect(transportClose).toHaveBeenCalledTimes(1) + expect(clientClose).toHaveBeenCalledTimes(1) + + 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: { @@ -2026,6 +2127,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 30ac500212..b23cb96b9c 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -6,7 +6,7 @@ import * as os from "os" import { safeWriteJson } from "../safeWriteJson" // Capture actual implementations before the vi.mock factory runs, -// so they are never wrapped by vi.fn() — avoids infinite recursion when +// so they are never wrapped by vi.fn() ? avoids infinite recursion when // test mockImplementation callbacks delegate to the real implementation. const fsPromisesActuals = vi.hoisted(() => ({ rename: undefined as (typeof import("fs/promises"))["rename"] | undefined, @@ -169,7 +169,7 @@ describe("safeWriteJson", () => { // Overwrite the pre-created file with specific initial data await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) - // fs.rename is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn + // fs.rename is already vi.fn() ? use vi.mocked to avoid double-wrapping via vi.spyOn // First call: the single-rename commit fails with the Windows-style EPERM // (a reader holds the target open). The fallback then renames the target // to a backup and the replacement into place, using real renames. @@ -198,7 +198,7 @@ describe("safeWriteJson", () => { // Track rename calls let renameCallCount = 0 - // fs.rename is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn + // fs.rename is already vi.fn() ? use vi.mocked to avoid double-wrapping via vi.spyOn vi.mocked(fs.rename).mockImplementation(async (oldPath, newPath) => { renameCallCount++ if (renameCallCount === 1) { @@ -267,7 +267,7 @@ describe("safeWriteJson", () => { }) test("should handle directory creation permission errors", async () => { - // fs.mkdir is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn + // fs.mkdir is already vi.fn() ? use vi.mocked to avoid double-wrapping via vi.spyOn vi.mocked(fs.mkdir).mockImplementationOnce(async () => { const error = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) throw error @@ -315,7 +315,7 @@ describe("safeWriteJson", () => { throw Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) }) - // fs.unlink is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn + // fs.unlink is already vi.fn() ? use vi.mocked to avoid double-wrapping via vi.spyOn vi.mocked(fs.unlink).mockImplementationOnce(async () => { throw new Error("Failed to delete backup file") }) @@ -341,7 +341,7 @@ describe("safeWriteJson", () => { throw Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) }) - // fs.unlink is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn + // fs.unlink is already vi.fn() ? use vi.mocked to avoid double-wrapping via vi.spyOn vi.mocked(fs.unlink).mockImplementation(async (filePath) => { if (filePath.toString().includes(".bak_")) { throw new Error("Backup deletion failed") @@ -366,7 +366,7 @@ describe("safeWriteJson", () => { await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) - // fs.rename is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn + // fs.rename is already vi.fn() ? use vi.mocked to avoid double-wrapping via vi.spyOn vi.mocked(fs.rename).mockImplementationOnce(async () => { throw new Error("Rename failed") }) @@ -422,7 +422,7 @@ describe("safeWriteJson", () => { test("should handle fs.access error that is not ENOENT", async () => { const data = { message: "access error test" } - // fs.access is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn + // fs.access is already vi.fn() ? use vi.mocked to avoid double-wrapping via vi.spyOn vi.mocked(fs.access).mockImplementationOnce(async () => { const error = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) throw error @@ -446,7 +446,7 @@ describe("safeWriteJson", () => { const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) // Suppress console.error - // fs.rename is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn + // fs.rename is already vi.fn() ? use vi.mocked to avoid double-wrapping via vi.spyOn let renameCallCount = 0 vi.mocked(fs.rename).mockImplementation(async (oldPath, newPath) => { renameCallCount++ @@ -819,3 +819,634 @@ describe("safeWriteJson", () => { expect(content).toEqual(newData) }) }) + +// Two guarded-write titles are deliberately absent from this file, because under main's +// replacement protocol the json entry no longer owns the backup/rollback surface: +// "does not fail the write when backup deletion fails (orphaned backup is acceptable)" +// "re-throws the original error when the rollback rename fails, leaving an orphaned backup" +// Both assert safeWriteText's backup naming and its RollbackFailedError taxonomy, and both +// claims are already tested at the module that provides them, in this same merge: +// src/services/file-safety/__tests__/safeWriteText.spec.ts +// "a post-commit backup cleanup failure is non-fatal: the target stays committed and no temp is left behind" +// "throws RollbackFailedError with the backup path when the rollback rename also fails" +// Nothing is dropped; the two claims stay tested at the layer that implements them. + +describe("safeWriteJson guarded-publish requirements carried by the guarded-write branch", () => { + let originalConsoleError: typeof console.error + + beforeAll(() => { + // Store original console.error + originalConsoleError = console.error + }) + + afterAll(() => { + // Restore original console.error + console.error = originalConsoleError + }) + + let tempDir: string + let currentTestFilePath: string + + beforeEach(async () => { + // Create a temporary directory for each test + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "safeWriteJson-test-")) + + // Create a unique file path for each test + currentTestFilePath = path.join(tempDir, "test-file.json") + + // Pre-create the file with initial content to ensure it exists + // This allows proper-lockfile to acquire a lock on an existing file. + await fs.writeFile(currentTestFilePath, JSON.stringify({ initial: "content" })) + }) + + afterEach(async () => { + // Clean up the temporary directory after each test + await fs.rm(tempDir, { recursive: true, force: true }) + + // Reset all mocks to their actual implementations + vi.restoreAllMocks() + }) + + // Helper function to read file content + async function readFileContent(filePath: string): Promise { + const readContent = await fs.readFile(filePath, "utf-8") + return JSON.parse(readContent) + } + + // Helper function to check if a file exists + async function fileExists(filePath: string): Promise { + try { + await fs.access(filePath) + return true + } catch { + return false + } + } + + // 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() + }) + + // 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("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("refuses the publish when an ancestor disappears before the commit", async () => { + const parent = path.join(tempDir, "gone-parent") + await fs.mkdir(parent, { recursive: true }) + const target = path.join(parent, "gone-ancestor.json") + await fsPromisesActuals.writeFile!(target, JSON.stringify({ own: true })) + const asDir = { isSymbolicLink: () => false, dev: 1n, ino: 1n } as unknown as fsSyncActual.BigIntStats + // The walk authorizes the directory; by publication time it is gone, so a path-based + // rename would resolve an ancestry nobody checked. The guard must fail closed. + let parentLooks = 0 + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + if (String(p) === parent) { + parentLooks++ + if (parentLooks === 1) return Promise.resolve(asDir) + return Promise.reject(Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" })) + } + return Promise.resolve(asDir) + }) as unknown as typeof fs.lstat) + + await expect(safeWriteJson(target, { leaked: true }, { refuseSymlinkTarget: true })).rejects.toThrow( + /no longer exists/, + ) + + vi.restoreAllMocks() + expect(await readFileContent(target)).toEqual({ own: true }) + expect(fsSyncActual.readdirSync(parent)).toEqual(["gone-ancestor.json"]) + }) + + test("fails closed when an ancestor re-check fails for a non-ENOENT reason", async () => { + const parent = path.join(tempDir, "eacces-parent") + await fs.mkdir(parent, { recursive: true }) + const target = path.join(parent, "eacces-ancestor.json") + await fsPromisesActuals.writeFile!(target, JSON.stringify({ own: true })) + const asDir = { isSymbolicLink: () => false, dev: 1n, ino: 1n } as unknown as fsSyncActual.BigIntStats + const failure = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + let parentLooks = 0 + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + if (String(p) === parent) { + parentLooks++ + if (parentLooks === 1) return Promise.resolve(asDir) + return Promise.reject(failure) + } + return Promise.resolve(asDir) + }) as unknown as typeof fs.lstat) + + // A stat failure says nothing about whether the entry is safe: the original error + // propagates instead of publishing blind. + await expect(safeWriteJson(target, { leaked: true }, { refuseSymlinkTarget: true })).rejects.toThrow(/EACCES/) + + vi.restoreAllMocks() + expect(await readFileContent(target)).toEqual({ own: true }) + }) + + test("fails closed when the destination re-check fails with a non-ENOENT error", async () => { + const parent = path.join(tempDir, "recheck-parent") + await fs.mkdir(parent, { recursive: true }) + const target = path.join(parent, "recheck-eacces.json") + const asDir = { isSymbolicLink: () => false, dev: 1n, ino: 1n } as unknown as fsSyncActual.BigIntStats + const asFile = { isSymbolicLink: () => false } as unknown as fsSyncActual.Stats + const failure = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + // Only ENOENT means "nothing there that could be a link"; any other lstat failure says + // nothing about the destination. The initial check passes and the pre-commit re-check + // is the one that fails, so this covers the publication-time inspection specifically. + let targetLooks = 0 + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + if (String(p) === target) { + targetLooks++ + if (targetLooks >= 2) return Promise.reject(failure) + return Promise.resolve(asFile) + } + return Promise.resolve(asDir) + }) as unknown as typeof fs.lstat) + + await expect(safeWriteJson(target, { written: true }, { refuseSymlinkTarget: true })).rejects.toThrow(/EACCES/) + + vi.restoreAllMocks() + // The pre-commit inspection ran (second look at the destination) and stopped the write. + expect(targetLooks).toBe(2) + expect(await fileExists(target)).toBe(false) + }) + + test("fails closed when the pre-commit publication inspection fails with a non-ENOENT error", async () => { + const parent = path.join(tempDir, "publication-parent") + await fs.mkdir(parent, { recursive: true }) + const target = path.join(parent, "publication-eacces.json") + const asDir = { isSymbolicLink: () => false, dev: 1n, ino: 1n } as unknown as fsSyncActual.BigIntStats + const asFile = { isSymbolicLink: () => false } as unknown as fsSyncActual.Stats + const failure = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + // The first two looks are the pre-staging destination checks; the third is the one the + // publication takes immediately before the commit rename. Rejecting the second, as the + // case above does, cannot show that a publication-time inspection error is honoured, so + // this case rejects the third and keeps the earlier case standing beside it. + let targetLooks = 0 + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + if (String(p) === target) { + targetLooks++ + if (targetLooks >= 3) return Promise.reject(failure) + return Promise.resolve(asFile) + } + return Promise.resolve(asDir) + }) as unknown as typeof fs.lstat) + + await expect(safeWriteJson(target, { written: true }, { refuseSymlinkTarget: true })).rejects.toThrow("EACCES") + + vi.restoreAllMocks() + // Three destination looks happened, so the failure came from the publication-time + // inspection rather than an earlier check, and the commit never landed. + expect(targetLooks).toBe(3) + expect(await fileExists(target)).toBe(false) + }) + + test("does not take the EPERM two-step fallback when the pre-commit ancestry assertion rejects with EPERM", async () => { + const parent = path.join(tempDir, "ancestry-eperm-parent") + await fs.mkdir(parent, { recursive: true }) + const target = path.join(parent, "ancestry-eperm.json") + // A committed destination is what makes the fallback observable: it renames the + // target to a backup before the second rename, so a mistaken fallback moves a file + // the inspection had just refused to publish over. + await fsPromisesActuals.writeFile!(target, JSON.stringify({ committed: true })) + const asDir = { isSymbolicLink: () => false, dev: 1n, ino: 1n } as unknown as fsSyncActual.BigIntStats + const asFile = { isSymbolicLink: () => false } as unknown as fsSyncActual.Stats + const failure = Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) + // The third destination look is the one the publication takes immediately before + // the commit rename, so this arms the ancestry assertion, not an earlier check. + let targetLooks = 0 + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + if (String(p) === target) { + targetLooks++ + if (targetLooks >= 3) return Promise.reject(failure) + return Promise.resolve(asFile) + } + return Promise.resolve(asDir) + }) as unknown as typeof fs.lstat) + const trace = vi.fn() + + await expect(safeWriteJson(target, { fresh: true }, { refuseSymlinkTarget: true, trace })).rejects.toThrow( + "EPERM", + ) + + vi.restoreAllMocks() + // The inspection ran and refused. The committed destination is still there, byte for + // byte, which is only true if the two-step replacement never started: an EPERM from an + // inspection is not a rename collision and must not move anything. + expect(targetLooks).toBe(3) + expect(await readFileContent(target)).toEqual({ committed: true }) + expect(trace).not.toHaveBeenCalledWith(expect.objectContaining({ stage: "backup-rename" })) + }) + + test("fails closed when an ancestor vanishes during the authorizing walk", async () => { + const parent = path.join(tempDir, "vanish-parent") + await fs.mkdir(parent, { recursive: true }) + const target = path.join(parent, "vanish-during-walk.json") + await fsPromisesActuals.writeFile!(target, JSON.stringify({ own: true })) + const asFile = { isSymbolicLink: () => false } as unknown as fsSyncActual.Stats + // A directory that vanishes mid-walk has no recorded identity for the pre-commit + // re-check, so a path that reappears as a symlink before staging would be followed by + // the path-based rename. The walk must fail closed instead of recording a partial chain. + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + if (String(p) === parent) { + return Promise.reject(Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" })) + } + return Promise.resolve(asFile) + }) as unknown as typeof fs.lstat) + + await expect(safeWriteJson(target, { written: true }, { refuseSymlinkTarget: true })).rejects.toThrow( + /does not exist, so the destination ancestry cannot be verified/, + ) + + vi.restoreAllMocks() + // Nothing was published: the existing target keeps its content. + expect(await readFileContent(target)).toEqual({ own: true }) + }) + + 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 e54b8c51d3..7b248af7d3 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 } from "../services/file-safety/safeWriteText" + import { getErrorCode } from "./errorCode" import { acquireFileLock } from "./fileLock" import { recoverInterruptedReplacement } from "./jsonFileRecovery" @@ -46,6 +48,39 @@ export interface SafeWriteJsonOptions { * to emit diagnostic trace lines; it never influences the write itself. */ trace?: SafeWriteJsonTrace + + /** + * 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 } function createTempFilePath(absoluteFilePath: string, kind: "new" | "bak"): string { @@ -62,6 +97,9 @@ function createTempFilePath(absoluteFilePath: string, kind: "new" | "bak"): stri * - Recovers an interrupted previous write before touching the file: restores an orphaned backup when the canonical path is absent, and removes leftover temp files. * - Writes to a temporary file first, then commits it with a single atomic rename, so the committed target stays readable through an interrupted write. * - When a single rename over the target fails with EPERM (a reader holds the file open on Windows), falls back to backing the target up before replacing it. + * - With `refuseSymlinkTarget`, refuses a publish whose final path component or any + * ancestor directory is (or becomes) a symlink, re-deriving the refusal immediately before + * the commit rename. * - Attempts to roll back and clean up in case of errors. * - Supports pretty-printing with indentation while maintaining streaming efficiency. * @@ -90,6 +128,174 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit 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") { + // Fail closed: a directory that vanishes mid-walk leaves no identity to re-check + // before publication, and a path that reappears as a symlink afterwards would be + // followed by the path-based commit rename. Refuse rather than publish blind. + throw new Error( + `safeWriteJson: refusing to write to ${absoluteFilePath}: ${current} does not exist, so the destination ancestry cannot be verified.`, + ) + } + 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. @@ -97,7 +303,7 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit // remains a no-op, so the finally block in the main file operations // try-catch-finally won't try to release an unacquired lock if this // path is taken. - releaseLock = await acquireFileLock(absoluteFilePath) + releaseLock = await acquireFileLock(lockTargetPath) // Variables to hold the actual paths of temp files if they are created. let actualTempNewFilePath: string | null = null @@ -122,7 +328,7 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit 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 = getErrorCode(error) if (!(error instanceof SyntaxError) && code !== "ENOENT") { @@ -144,6 +350,15 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit // committed target stays in place until this rename lands, so a // process exit before or during the commit can no longer leave the // canonical path absent. + if (options?.refuseSymlinkTarget) { + // Re-derive the refusal inside the publication, immediately before the commit rename, but + // outside the rename-fallback try: that handler tests only the error code, so an EPERM + // raised by this inspection would be mistaken for a rename collision and take the + // two-step replacement, moving the committed destination to a backup even though the + // refusal promises that nothing moves. Only rename failures reach the fallback now; + // the fallback and the crash-recovery pass at the start of the next write are unchanged. + await assertAncestryUnchangedBeforeCommit() + } try { options?.trace?.({ stage: "replacement-rename" }) await fs.rename(actualTempNewFilePath, absoluteFilePath) @@ -161,6 +376,10 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit options?.trace?.({ stage: "backup-rename", detail: "eperm-fallback" }) actualTempBackupFilePath = createTempFilePath(absoluteFilePath, "bak") await fs.rename(absoluteFilePath, actualTempBackupFilePath) + if (options?.refuseSymlinkTarget) { + // The fallback's second rename is the same commit action, so it gets the same pre-commit re-check. + await assertAncestryUnchangedBeforeCommit() + } options?.trace?.({ stage: "replacement-rename", detail: "eperm-fallback" }) await fs.rename(actualTempNewFilePath, absoluteFilePath) } @@ -230,7 +449,7 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit 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) } } }