diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index c4c62f7f30..9062716466 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -112,6 +112,7 @@ import { ToolRepetitionDetector } from "../tools/ToolRepetitionDetector" import { restoreTodoListForTask } from "../tools/UpdateTodoListTool" import { FileContextTracker } from "../context-tracking/FileContextTracker" import { RooIgnoreController } from "../ignore/RooIgnoreController" +import { ObservationRegistry } from "./observationRegistry" import { RooProtectedController } from "../protect/RooProtectedController" import { type AssistantMessageContent, presentAssistantMessage } from "../assistant-message" import { NativeToolCallParser } from "../assistant-message/NativeToolCallParser" @@ -292,6 +293,7 @@ export class Task extends EventEmitter implements TaskLike { readonly parentTask: Task | undefined = undefined readonly taskNumber: number readonly workspacePath: string + readonly observationRegistry = new ObservationRegistry() /** * The mode associated with this task. Persisted across sessions diff --git a/src/core/task/__tests__/observationRegistry.spec.ts b/src/core/task/__tests__/observationRegistry.spec.ts new file mode 100644 index 0000000000..51b73aabde --- /dev/null +++ b/src/core/task/__tests__/observationRegistry.spec.ts @@ -0,0 +1,72 @@ +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") + }) +}) diff --git a/src/core/task/observationRegistry.ts b/src/core/task/observationRegistry.ts new file mode 100644 index 0000000000..7ca7c801aa --- /dev/null +++ b/src/core/task/observationRegistry.ts @@ -0,0 +1,49 @@ +/** + * 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 is rejected instead of published over. + */ + +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() + + /** + * Record an observation for a file at its absolute path. + * + * Re-observing replaces the entry with a fresh observedAt timestamp and + * the new version token. + */ + observe(absolutePath: string, version: string): void { + 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() + } + + get size(): number { + return this.entries.size + } +} diff --git a/src/core/tools/ApplyDiffTool.ts b/src/core/tools/ApplyDiffTool.ts index 3b664b3bd2..20e847844d 100644 --- a/src/core/tools/ApplyDiffTool.ts +++ b/src/core/tools/ApplyDiffTool.ts @@ -8,6 +8,7 @@ import { getReadablePath } from "../../utils/path" import { Task } from "../task/Task" import { formatResponse } from "../prompts/responses" import { fileExistsAtPath } from "../../utils/fs" +import { versionTokenOfStat } from "../../utils/versionToken" import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { unescapeHtmlEntities } from "../../utils/text-normalization" import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" @@ -68,7 +69,22 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { return } + // The diff below is built from this exact read, so the save that follows must be + // authorized against the version captured here - not against whatever version the + // diff view happens to stat afterwards (there is no diff view on the + // focus-disruption path). Same contract as ApplyPatchTool's hunk read: stat + // around the read and observe only when the file did not change underneath it. + // Without it the saveDirectly("edit") below has no observation for the path and the + // guarded publish fails with "File not read yet -- read the file, then retry." + const preReadStats = await fs.stat(absolutePath, { bigint: true }).catch(() => undefined) const originalContent: string = await fs.readFile(absolutePath, "utf-8") + const postReadStats = await fs.stat(absolutePath, { bigint: true }).catch(() => undefined) + if (preReadStats && postReadStats) { + const preReadToken = versionTokenOfStat(preReadStats) + if (preReadToken === versionTokenOfStat(postReadStats)) { + task.observationRegistry.observe(absolutePath, preReadToken) + } + } // Apply the diff to the original content const diffResult = (await task.diffStrategy?.applyDiff( @@ -173,7 +189,8 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { return } - // Save directly without showing diff view or opening the file + // Save directly without showing diff view or opening the file. The diff is + // applied to an existing file, so edit-guard semantics require a prior read. task.diffViewProvider.editType = "modify" task.diffViewProvider.originalContent = originalContent await task.diffViewProvider.saveDirectly( @@ -182,6 +199,7 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { false, diagnosticsEnabled, writeDelayMs, + "edit", ) } else { // Original behavior with diff view diff --git a/src/core/tools/ApplyPatchTool.ts b/src/core/tools/ApplyPatchTool.ts index 56b2bf8909..3daf3a116e 100644 --- a/src/core/tools/ApplyPatchTool.ts +++ b/src/core/tools/ApplyPatchTool.ts @@ -11,6 +11,7 @@ import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { fileExistsAtPath } from "../../utils/fs" import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" import { sanitizeUnifiedDiff, computeDiffStats } from "../diff/stats" +import { versionTokenOfStat } from "../../utils/versionToken" import { BaseTool, ToolCallbacks } from "./BaseTool" import type { ToolUse } from "../../shared/tools" import { parsePatch, ParseError, processAllHunks } from "./apply-patch" @@ -85,10 +86,25 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { return } - // Process each hunk + // Process each hunk. The read doubles as the S2 observation for the + // guarded publish (ReadFileTool contract: stat before and after the + // read, observe only when the on-disk version is unchanged between the + // two stats). Without it, the in-place modify publish is an unobserved + // write and the composed chat-diff default rejects it ("File already + // exists ... and was not read before this write") even though this tool + // just read the exact content the patch was applied to. const readFile = async (filePath: string): Promise => { const absolutePath = path.resolve(task.cwd, filePath) - return await fs.readFile(absolutePath, "utf8") + const preReadStats = await fs.stat(absolutePath, { bigint: true }).catch(() => undefined) + const content: string = await fs.readFile(absolutePath, "utf8") + const postReadStats = await fs.stat(absolutePath, { bigint: true }).catch(() => undefined) + if (preReadStats && postReadStats) { + const preReadToken = versionTokenOfStat(preReadStats) + if (preReadToken === versionTokenOfStat(postReadStats)) { + task.observationRegistry.observe(absolutePath, preReadToken) + } + } + return content } let changes: ApplyPatchFileChange[] @@ -214,7 +230,16 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { // Save the changes if (isPreventFocusDisruptionEnabled) { - await task.diffViewProvider.saveDirectly(relPath, newContent, true, diagnosticsEnabled, writeDelayMs) + // Guarded publish: the patch supplies the complete new content, so create-guard + // semantics apply (an unobserved existing target is rejected, not overwritten). + await task.diffViewProvider.saveDirectly( + relPath, + newContent, + true, + diagnosticsEnabled, + writeDelayMs, + "create", + ) } else { await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) } @@ -408,12 +433,14 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { // Save new content to the new path if (isPreventFocusDisruptionEnabled) { + // The move destination is published with the complete new content. await task.diffViewProvider.saveDirectly( change.movePath, newContent, false, diagnosticsEnabled, writeDelayMs, + "create", ) } else { // Write to new path and delete old file @@ -433,7 +460,16 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { } else { // Save changes to the same file if (isPreventFocusDisruptionEnabled) { - await task.diffViewProvider.saveDirectly(relPath, newContent, false, diagnosticsEnabled, writeDelayMs) + // Guarded publish: the patched file content is complete, so create-guard + // semantics apply (stale observed versions are rejected with a re-read hint). + await task.diffViewProvider.saveDirectly( + relPath, + newContent, + false, + diagnosticsEnabled, + writeDelayMs, + "create", + ) } else { await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) } diff --git a/src/core/tools/EditFileTool.ts b/src/core/tools/EditFileTool.ts index a7301e2ac9..0e10184100 100644 --- a/src/core/tools/EditFileTool.ts +++ b/src/core/tools/EditFileTool.ts @@ -4,6 +4,7 @@ import path from "path" import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" import { getReadablePath } from "../../utils/path" +import { versionTokenOfStat } from "../../utils/versionToken" import { isPathOutsideWorkspace } from "../../utils/pathUtils" import { Task } from "../task/Task" import { formatResponse } from "../prompts/responses" @@ -231,10 +232,23 @@ export class EditFileTool extends BaseTool<"edit_file"> { // Read file or determine if creating new if (fileExists) { try { + // The guarded saveDirectly below authorizes the publish against the version + // this read saw, and the focus-disruption path has no diff view that could + // observe the file. Same contract as ApplyDiffTool/ApplyPatchTool: stat around + // the read and observe only when the file did not change underneath it. Without + // it the saveDirectly("edit") below rejects with "File not read yet". + const preReadStats = await fs.stat(absolutePath, { bigint: true }).catch(() => undefined) currentContent = await fs.readFile(absolutePath, "utf8") + const postReadStats = await fs.stat(absolutePath, { bigint: true }).catch(() => undefined) originalEol = detectLineEnding(currentContent) // Normalize line endings to LF for matching currentContentLF = normalizeToLF(currentContent) + if (preReadStats && postReadStats) { + const preReadToken = versionTokenOfStat(preReadStats) + if (preReadToken === versionTokenOfStat(postReadStats)) { + task.observationRegistry.observe(absolutePath, preReadToken) + } + } } catch (error) { task.consecutiveMistakeCount++ task.didToolFailInCurrentTurn = true @@ -436,13 +450,16 @@ export class EditFileTool extends BaseTool<"edit_file"> { // Save the changes if (isPreventFocusDisruptionEnabled) { - // Direct file write without diff view or opening the file + // Direct file write without diff view or opening the file. In-place edits + // use edit-guard semantics (a prior read is required); new-file creation + // keeps create-guard semantics. await task.diffViewProvider.saveDirectly( relPath, newContent, isNewFile, diagnosticsEnabled, writeDelayMs, + isNewFile ? "create" : "edit", ) } else { // Call saveChanges to update the DiffViewProvider properties diff --git a/src/core/tools/EditTool.ts b/src/core/tools/EditTool.ts index 2ae8bf4ed0..7dfe9c7cb1 100644 --- a/src/core/tools/EditTool.ts +++ b/src/core/tools/EditTool.ts @@ -4,6 +4,7 @@ import path from "path" import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" import { getReadablePath } from "../../utils/path" +import { versionTokenOfStat } from "../../utils/versionToken" import { isPathOutsideWorkspace } from "../../utils/pathUtils" import { Task } from "../task/Task" import { formatResponse } from "../prompts/responses" @@ -89,9 +90,22 @@ export class EditTool extends BaseTool<"edit"> { let fileContent: string try { + // The guarded saveDirectly below authorizes the publish against the version this + // read saw, and the focus-disruption path has no diff view that could observe + // the file. Same contract as ApplyDiffTool/ApplyPatchTool: stat around the read + // and observe only when the file did not change underneath it. Without it the + // saveDirectly("edit") below rejects with "File not read yet". + const preReadStats = await fs.stat(absolutePath, { bigint: true }).catch(() => undefined) fileContent = await fs.readFile(absolutePath, "utf8") + const postReadStats = await fs.stat(absolutePath, { bigint: true }).catch(() => undefined) // Normalize line endings to LF for consistent matching fileContent = fileContent.replace(/\r\n/g, "\n") + if (preReadStats && postReadStats) { + const preReadToken = versionTokenOfStat(preReadStats) + if (preReadToken === versionTokenOfStat(postReadStats)) { + task.observationRegistry.observe(absolutePath, preReadToken) + } + } } catch (error) { task.consecutiveMistakeCount++ task.recordToolError("edit") @@ -211,8 +225,16 @@ export class EditTool extends BaseTool<"edit"> { // Save the changes if (isPreventFocusDisruptionEnabled) { - // Direct file write without diff view or opening the file - await task.diffViewProvider.saveDirectly(relPath, newContent, false, diagnosticsEnabled, writeDelayMs) + // Direct file write without diff view or opening the file. This tool only + // edits existing files, so edit-guard semantics require a prior read. + await task.diffViewProvider.saveDirectly( + relPath, + newContent, + false, + diagnosticsEnabled, + writeDelayMs, + "edit", + ) } else { // Call saveChanges to update the DiffViewProvider properties await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) diff --git a/src/core/tools/ReadFileTool.ts b/src/core/tools/ReadFileTool.ts index 00fc507692..88d54ddc33 100644 --- a/src/core/tools/ReadFileTool.ts +++ b/src/core/tools/ReadFileTool.ts @@ -16,6 +16,7 @@ import type { ReadFileParams, ReadFileMode, ReadFileToolParams, FileEntry, LineR import { isLegacyReadFileParams, type ClineSayTool } from "@roo-code/types" import { Task } from "../task/Task" +import { versionTokenOfStat } from "../../utils/versionToken" import { formatResponse } from "../prompts/responses" import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { isPathOutsideWorkspace } from "../../utils/pathUtils" @@ -214,12 +215,29 @@ export class ReadFileTool extends BaseTool<"read_file"> { // Read text file content with lossy UTF-8 conversion // Reading as Buffer first allows graceful handling of non-UTF8 bytes // (they become U+FFFD replacement characters instead of throwing) + // A2 (epic #1375): capture the on-disk token before the read so a mutation + // landing mid-read is detected by the post-read stat below. + const preReadStats = await fs.stat(fullPath, { bigint: true }).catch(() => undefined) const buffer = await fs.readFile(fullPath) const fileContent = buffer.toString("utf-8") const result = this.processTextFile(fileContent, entry) await task.fileContextTracker.trackFileContext(relPath, "read_tool" as RecordSource) + // A2 (plan #33 / epic #1375): record the observed on-disk version for the future write guard. + // The token is captured before AND after the read; the target is observed only + // when both match — a mutation between the two stats means the content the model + // received is not the on-disk state, and observing it would let a later write + // match a token the model never saw. A stat failure leaves the target + // unobserved and never fails the read. + const postReadStats = await fs.stat(fullPath, { bigint: true }).catch(() => undefined) + if (preReadStats && postReadStats) { + const preReadToken = versionTokenOfStat(preReadStats) + if (preReadToken === versionTokenOfStat(postReadStats)) { + task.observationRegistry.observe(fullPath, preReadToken) + } + } + updateFileResult(relPath, { nativeContent: `File: ${relPath}\n${result}`, }) @@ -768,6 +786,9 @@ export class ReadFileTool extends BaseTool<"read_file"> { } // Read text file + // A2 (epic #1375): capture the on-disk token before the read so a mutation + // landing mid-read is detected by the post-read stat below. + const preReadStats = await fs.stat(fullPath, { bigint: true }).catch(() => undefined) const rawContent = await fs.readFile(fullPath, "utf8") // Handle line ranges if specified @@ -799,6 +820,19 @@ export class ReadFileTool extends BaseTool<"read_file"> { // Track file in context await task.fileContextTracker.trackFileContext(relPath, "read_tool") + + // A2 (plan #33 / epic #1375): mirror the native path — record the observed + // on-disk version so legacy-format reads also feed the future write guard. + // Observe only when the pre-read and post-read tokens match (a mutation between + // them means the returned content is not the on-disk state). A stat failure + // leaves the target unobserved and never fails the read. + const postReadStats = await fs.stat(fullPath, { bigint: true }).catch(() => undefined) + if (preReadStats && postReadStats) { + const preReadToken = versionTokenOfStat(preReadStats) + if (preReadToken === versionTokenOfStat(postReadStats)) { + task.observationRegistry.observe(fullPath, preReadToken) + } + } } catch (error) { const errorMsg = error instanceof Error ? error.message : String(error) results.push(`File: ${relPath}\nError: ${errorMsg}`) diff --git a/src/core/tools/SearchReplaceTool.ts b/src/core/tools/SearchReplaceTool.ts index e29b124010..4e932ee5c4 100644 --- a/src/core/tools/SearchReplaceTool.ts +++ b/src/core/tools/SearchReplaceTool.ts @@ -4,6 +4,7 @@ import path from "path" import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" import { getReadablePath } from "../../utils/path" +import { versionTokenOfStat } from "../../utils/versionToken" import { isPathOutsideWorkspace } from "../../utils/pathUtils" import { Task } from "../task/Task" import { formatResponse } from "../prompts/responses" @@ -94,9 +95,22 @@ export class SearchReplaceTool extends BaseTool<"search_replace"> { let fileContent: string try { + // The guarded saveDirectly below authorizes the publish against the version this + // read saw, and the focus-disruption path has no diff view that could observe + // the file. Same contract as ApplyDiffTool/ApplyPatchTool: stat around the read + // and observe only when the file did not change underneath it. Without it the + // saveDirectly("edit") below rejects with "File not read yet". + const preReadStats = await fs.stat(absolutePath, { bigint: true }).catch(() => undefined) fileContent = await fs.readFile(absolutePath, "utf8") + const postReadStats = await fs.stat(absolutePath, { bigint: true }).catch(() => undefined) // Normalize line endings to LF for consistent matching fileContent = fileContent.replace(/\r\n/g, "\n") + if (preReadStats && postReadStats) { + const preReadToken = versionTokenOfStat(preReadStats) + if (preReadToken === versionTokenOfStat(postReadStats)) { + task.observationRegistry.observe(absolutePath, preReadToken) + } + } } catch (error) { task.consecutiveMistakeCount++ task.recordToolError("search_replace") @@ -207,8 +221,16 @@ export class SearchReplaceTool extends BaseTool<"search_replace"> { // Save the changes if (isPreventFocusDisruptionEnabled) { - // Direct file write without diff view or opening the file - await task.diffViewProvider.saveDirectly(relPath, newContent, false, diagnosticsEnabled, writeDelayMs) + // Direct file write without diff view or opening the file. This tool only + // edits existing files, so edit-guard semantics require a prior read. + await task.diffViewProvider.saveDirectly( + relPath, + newContent, + false, + diagnosticsEnabled, + writeDelayMs, + "edit", + ) } else { // Call saveChanges to update the DiffViewProvider properties await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 0c5c80abb9..71bb864c43 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -132,7 +132,16 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { return } - await task.diffViewProvider.saveDirectly(relPath, newContent, false, diagnosticsEnabled, writeDelayMs) + // Guarded publish: this write carries the complete file content, so it uses + // create-guard semantics (unobserved targets may only be created when absent). + await task.diffViewProvider.saveDirectly( + relPath, + newContent, + false, + diagnosticsEnabled, + writeDelayMs, + "create", + ) } else { if (!task.diffViewProvider.isEditing) { const partialMessage = JSON.stringify(sharedMessageProps) diff --git a/src/core/tools/__tests__/applyDiffTool.guardedWrite.spec.ts b/src/core/tools/__tests__/applyDiffTool.guardedWrite.spec.ts new file mode 100644 index 0000000000..2f2dba2acc --- /dev/null +++ b/src/core/tools/__tests__/applyDiffTool.guardedWrite.spec.ts @@ -0,0 +1,274 @@ +// npx vitest run core/tools/__tests__/applyDiffTool.guardedWrite.spec.ts + +import path from "path" +import type { BigIntStats } from "fs" +import type { MockedFunction } from "vitest" + +import { fileExistsAtPath } from "../../../utils/fs" +import { ObservationRegistry } from "../../task/observationRegistry" +import type { Task } from "../../task/Task" +import { ApplyDiffTool } from "../ApplyDiffTool" + +vi.mock("fs/promises", () => ({ + default: { + readFile: vi.fn().mockResolvedValue("original file content\n"), + stat: vi.fn().mockResolvedValue({ + dev: 7n, + ino: 4242n, + size: 1234n, + mtimeNs: 1700000000123456789n, + ctimeNs: 1700000000789999999n, + }), + }, +})) + +vi.mock("../../../utils/fs", () => ({ + fileExistsAtPath: vi.fn().mockResolvedValue(true), +})) + +vi.mock("../../prompts/responses", () => ({ + formatResponse: { + toolError: vi.fn((msg: string) => `Error: ${msg}`), + rooIgnoreError: vi.fn((filePath: string) => `Access denied: ${filePath}`), + createPrettyPatch: vi.fn(() => "mock-diff"), + }, +})) + +vi.mock("../../diff/stats", () => ({ + sanitizeUnifiedDiff: vi.fn((diff: string) => diff), + computeDiffStats: vi.fn(() => ({ additions: 1, deletions: 1 })), +})) + +describe("ApplyDiffTool.execute - guarded write (S4b, epic #1375)", () => { + const mockedFileExistsAtPath = fileExistsAtPath as MockedFunction + + let tool: ApplyDiffTool + let mockTask: Pick< + Task, + | "cwd" + | "consecutiveMistakeCount" + | "consecutiveMistakeCountForApplyDiff" + | "recordToolError" + | "rooIgnoreController" + | "rooProtectedController" + | "say" + | "processQueuedMessages" + | "didEditFile" + | "api" + | "diffStrategy" + | "diffViewProvider" + | "providerRef" + | "fileContextTracker" + | "observationRegistry" + > + let mockSaveDirectly: MockedFunction<(...args: unknown[]) => Promise> + let mockAskApproval: MockedFunction<(...args: unknown[]) => Promise> + let mockHandleError: MockedFunction<(...args: unknown[]) => Promise> + let mockPushToolResult: MockedFunction<(...args: unknown[]) => void> + + beforeEach(() => { + vi.clearAllMocks() + + mockedFileExistsAtPath.mockResolvedValue(true) + + mockSaveDirectly = vi.fn().mockResolvedValue({ + newProblemsMessage: "", + userEdits: undefined, + finalContent: "new content", + }) + + // Structural stubs for the guarded-write path: the real DiffViewProvider is + // out of scope here, so vi.fn() doubles stand in for the members the tool + // touches (the saveDirectly double also records the writeKind plumbing). + const diffViewProviderStub = { + editType: undefined as "create" | "modify" | undefined, + originalContent: undefined as string | undefined, + saveDirectly: mockSaveDirectly, + pushToolWriteResult: vi.fn().mockResolvedValue("Saved file"), + reset: vi.fn().mockResolvedValue(undefined), + } + mockTask = { + cwd: "/workspace/project", + consecutiveMistakeCount: 0, + consecutiveMistakeCountForApplyDiff: new Map(), + recordToolError: vi.fn(), + rooIgnoreController: { + validateAccess: vi.fn().mockReturnValue(true), + } as unknown as Task["rooIgnoreController"], + rooProtectedController: { + isWriteProtected: vi.fn().mockReturnValue(false), + } as unknown as Task["rooProtectedController"], + say: vi.fn().mockResolvedValue(undefined), + processQueuedMessages: vi.fn(), + didEditFile: false, + api: { + getModel: () => ({ id: "claude-sonnet-4-5" }), + } as unknown as Task["api"], + diffStrategy: { + applyDiff: vi.fn().mockResolvedValue({ success: true, content: "modified file content\n" }), + } as unknown as Task["diffStrategy"], + diffViewProvider: diffViewProviderStub as unknown as Task["diffViewProvider"], + providerRef: { + deref: vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + // Exercise the focus-disruption (saveDirectly) save path. + experiments: { preventFocusDisruption: true }, + }), + }), + } as unknown as Task["providerRef"], + fileContextTracker: { + trackFileContext: vi.fn().mockResolvedValue(undefined), + } as unknown as Task["fileContextTracker"], + // Real registry: the point of these tests is which version token ends up + // recorded, so a stub would only restate the call. + observationRegistry: new ObservationRegistry(), + } + + mockAskApproval = vi.fn().mockResolvedValue(true) + mockHandleError = vi.fn().mockResolvedValue(undefined) + mockPushToolResult = vi.fn() + + tool = new ApplyDiffTool() + }) + + it("publishes through the guarded saveDirectly with edit kind", async () => { + await tool.execute({ path: "src/thing.ts", diff: "unified diff" }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockSaveDirectly).toHaveBeenCalledWith( + "src/thing.ts", + "modified file content\n", + false, + true, + 1000, + "edit", + ) + expect(mockPushToolResult).toHaveBeenCalledWith("Saved file") + expect(mockTask.didEditFile).toBe(true) + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("surfaces the unobserved edit remediation as a tool error", async () => { + const guardError = new Error("File not read yet -- read the file, then retry.") + mockSaveDirectly.mockRejectedValue(guardError) + + await tool.execute({ path: "src/thing.ts", diff: "unified diff" }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockHandleError).toHaveBeenCalledWith("applying diff", guardError) + expect(vi.mocked(mockTask.diffViewProvider.reset)).toHaveBeenCalled() + expect(mockTask.didEditFile).toBe(false) + expect(mockPushToolResult).not.toHaveBeenCalledWith("Saved file") + }) + it("records the version the diff was computed against so the guarded edit is authorized", async () => { + await tool.execute({ path: "src/thing.ts", diff: "unified diff" }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // The focus-disruption save has no diff view to observe the file, so the tool read + // itself must leave an observation for the version the hunks were computed against - + // otherwise saveDirectly("edit") rejects with "File not read yet". + const observed = mockTask.observationRegistry.get(path.resolve("/workspace/project", "src/thing.ts")) + expect(observed?.version).toBe("7:4242:1234:1700000000123456789:1700000000789999999") + expect(mockSaveDirectly).toHaveBeenCalledWith( + "src/thing.ts", + "modified file content\n", + false, + true, + 1000, + "edit", + ) + }) + + it("does not observe when the file changes underneath the read", async () => { + // Only the fields versionTokenOfStat reads; a full BigIntStats cannot be built + // against the mocked fs, so the double assertion is the narrowest option. + const statMock = vi.mocked((await import("fs/promises")).default.stat) + statMock + .mockResolvedValueOnce({ + dev: 7n, + ino: 4242n, + size: 1234n, + mtimeNs: 1n, + ctimeNs: 2n, + } as unknown as BigIntStats) + .mockResolvedValueOnce({ + dev: 7n, + ino: 4242n, + size: 9999n, + mtimeNs: 3n, + ctimeNs: 4n, + } as unknown as BigIntStats) + + await tool.execute({ path: "src/thing.ts", diff: "unified diff" }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // A read that straddled a write proves nothing about the current version, so no + // observation may be recorded and the guarded publish keeps its remediation. + expect(mockTask.observationRegistry.has(path.resolve("/workspace/project", "src/thing.ts"))).toBe(false) + }) + it.each(["pre-read", "post-read"])( + "continues the read and records no observation when the %s stat fails", + async (which) => { + const statMock = vi.mocked((await import("fs/promises")).default.stat) + const stable = { + dev: 7n, + ino: 4242n, + size: 1234n, + mtimeNs: 1700000000123456789n, + ctimeNs: 1700000000789999999n, + } as unknown as BigIntStats + const failure = new Error("EACCES: permission denied") + if (which === "pre-read") { + statMock.mockRejectedValueOnce(failure).mockResolvedValueOnce(stable) + } else { + statMock.mockResolvedValueOnce(stable).mockRejectedValueOnce(failure) + } + + // With no observation the guarded publish refuses, which is what the second half + // of this assertion checks. + const guardError = new Error("File not read yet -- read the file, then retry.") + mockSaveDirectly.mockRejectedValue(guardError) + + await tool.execute({ path: "src/thing.ts", diff: "unified diff" }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // Both stat calls must be consumed: beforeEach only clears call history, so a + // queued mockResolvedValueOnce would leak into the next test if the tool ever + // skipped one of the reads. + expect(statMock).toHaveBeenCalledTimes(2) + + // A stat failure is not evidence the file changed and not a reason to abort: the + // diff still ran against the content that WAS read successfully. + expect(mockSaveDirectly).toHaveBeenCalledWith( + "src/thing.ts", + "modified file content\n", + false, + true, + 1000, + "edit", + ) + // Nothing may be observed from a read whose version is unknown. + expect(mockTask.observationRegistry.has(path.resolve("/workspace/project", "src/thing.ts"))).toBe(false) + // And the refused publish surfaces as an error rather than a saved file. + expect(mockHandleError).toHaveBeenCalledWith("applying diff", guardError) + expect(mockPushToolResult).not.toHaveBeenCalledWith("Saved file") + }, + ) +}) diff --git a/src/core/tools/__tests__/applyPatchTool.execute.spec.ts b/src/core/tools/__tests__/applyPatchTool.execute.spec.ts index 72ffb112bc..66db86570e 100644 --- a/src/core/tools/__tests__/applyPatchTool.execute.spec.ts +++ b/src/core/tools/__tests__/applyPatchTool.execute.spec.ts @@ -4,12 +4,36 @@ import type { MockedFunction } from "vitest" import { fileExistsAtPath } from "../../../utils/fs" import { isPathOutsideWorkspace } from "../../../utils/pathUtils" +import path from "path" +import * as fsPromises from "fs/promises" import type { Task } from "../../task/Task" +import { ObservationRegistry } from "../../task/observationRegistry" import { ApplyPatchTool } from "../ApplyPatchTool" +// The vi.mock factory exposes the fs/promises functions under a `default` +// property (matching the SUT's default import), which the static module type +// does not declare; cast once at this boundary rather than at each call site. +const mockedFsPromises = vi.mocked( + fsPromises as unknown as { + default: { + stat: ReturnType + unlink: MockedFunction + } + }, +) + vi.mock("fs/promises", () => ({ default: { readFile: vi.fn().mockResolvedValue("original file content\n"), + // Stable on-disk version for the S2 self-read observation (the hunk + // read now stats before and after; equal tokens record the observe). + stat: vi.fn().mockResolvedValue({ + dev: 7n, + ino: 4242n, + size: 1234n, + mtimeNs: 1_700_000_000_123_456_789n, + ctimeNs: 1_700_000_000_789_999_999n, + }), unlink: vi.fn().mockResolvedValue(undefined), }, })) @@ -38,6 +62,7 @@ describe("ApplyPatchTool.execute - delete file success path", () => { | "say" | "processQueuedMessages" | "didEditFile" + | "observationRegistry" > let mockAskApproval: MockedFunction<(...args: unknown[]) => Promise> let mockHandleError: MockedFunction<(...args: unknown[]) => Promise> @@ -52,6 +77,7 @@ describe("ApplyPatchTool.execute - delete file success path", () => { mockTask = { cwd: "/workspace/project", consecutiveMistakeCount: 0, + observationRegistry: new ObservationRegistry(), recordToolUsage: vi.fn(), recordToolError: vi.fn(), rooIgnoreController: { @@ -94,3 +120,301 @@ describe("ApplyPatchTool.execute - delete file success path", () => { expect(mockTask.recordToolError).not.toHaveBeenCalled() }) }) + +describe("ApplyPatchTool.execute - guarded write (S4b, epic #1375)", () => { + const mockedFileExistsAtPath = fileExistsAtPath as MockedFunction + + let tool: ApplyPatchTool + let mockTask: Pick< + Task, + | "cwd" + | "consecutiveMistakeCount" + | "recordToolError" + | "rooIgnoreController" + | "rooProtectedController" + | "say" + | "processQueuedMessages" + | "didEditFile" + | "diffViewProvider" + | "providerRef" + | "fileContextTracker" + | "observationRegistry" + > + let mockSaveDirectly: MockedFunction<(...args: unknown[]) => Promise> + let mockAskApproval: MockedFunction<(...args: unknown[]) => Promise> + let mockHandleError: MockedFunction<(...args: unknown[]) => Promise> + let mockPushToolResult: MockedFunction<(...args: unknown[]) => void> + + const updatePatch = `*** Begin Patch +*** Update File: src/thing.ts +@@ +-original file content ++modified file content +*** End Patch` + + const addPatch = `*** Begin Patch +*** Add File: src/new.ts ++new line one ++new line two +*** End Patch` + + const movePatch = `*** Begin Patch +*** Update File: src/old.ts +*** Move to: src/new.ts +@@ +-original file content ++modified file content +*** End Patch` + + beforeEach(() => { + vi.clearAllMocks() + + mockedFileExistsAtPath.mockResolvedValue(true) + + mockSaveDirectly = vi.fn().mockResolvedValue({ + newProblemsMessage: "", + userEdits: undefined, + finalContent: "new content", + }) + + // Structural stubs for the guarded-write path: the real DiffViewProvider is + // out of scope here, so vi.fn() doubles stand in for the members the tool + // touches (the saveDirectly double also records the writeKind plumbing). + const diffViewProviderStub = { + editType: undefined as "create" | "modify" | undefined, + originalContent: undefined as string | undefined, + saveDirectly: mockSaveDirectly, + pushToolWriteResult: vi.fn().mockResolvedValue("Saved file"), + reset: vi.fn().mockResolvedValue(undefined), + } + mockTask = { + cwd: "/workspace/project", + consecutiveMistakeCount: 0, + observationRegistry: new ObservationRegistry(), + recordToolError: vi.fn(), + rooIgnoreController: { + validateAccess: vi.fn().mockReturnValue(true), + } as unknown as Task["rooIgnoreController"], + rooProtectedController: { + isWriteProtected: vi.fn().mockReturnValue(false), + } as unknown as Task["rooProtectedController"], + say: vi.fn().mockResolvedValue(undefined), + processQueuedMessages: vi.fn(), + didEditFile: false, + diffViewProvider: diffViewProviderStub as unknown as Task["diffViewProvider"], + providerRef: { + deref: vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + // Exercise the focus-disruption (saveDirectly) save path. + experiments: { preventFocusDisruption: true }, + }), + }), + } as unknown as Task["providerRef"], + fileContextTracker: { + trackFileContext: vi.fn().mockResolvedValue(undefined), + } as unknown as Task["fileContextTracker"], + } + + mockAskApproval = vi.fn().mockResolvedValue(true) + mockHandleError = vi.fn().mockResolvedValue(undefined) + mockPushToolResult = vi.fn() + + tool = new ApplyPatchTool() + }) + + it("update: publishes through the guarded saveDirectly with create kind", async () => { + await tool.execute({ patch: updatePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockSaveDirectly).toHaveBeenCalledWith( + "src/thing.ts", + "modified file content\n", + false, + true, + 1000, + "create", + ) + expect(mockPushToolResult).toHaveBeenCalledWith("Saved file") + expect(mockTask.didEditFile).toBe(true) + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("update: observes the hunk read so the guarded publish is not unobserved", async () => { + await tool.execute({ patch: updatePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // The hunk read doubles as the S2 observation (ReadFileTool contract): + // stable pre/post stats record the version token, so the in-place modify + // publish is not rejected as an unobserved write. + const observed = mockTask.observationRegistry.get(path.resolve("/workspace/project", "src/thing.ts")) + expect(observed?.version).toBe("7:4242:1234:1700000000123456789:1700000000789999999") + }) + + it("update: does not observe when the pre- and post-read tokens disagree", async () => { + // The file changed mid-read: pre/post stats differ, so no observation is + // recorded and the guarded publish surfaces the unobserved-existing + // remediation instead of publishing against a stale version. + const statMock = mockedFsPromises.default.stat + statMock.mockResolvedValueOnce({ dev: 7n, ino: 4242n, size: 1234n, mtimeNs: 1n, ctimeNs: 2n }) + statMock.mockResolvedValueOnce({ dev: 7n, ino: 4242n, size: 9999n, mtimeNs: 3n, ctimeNs: 4n }) + + const guardError = new Error( + "File already exists at /workspace/project/src/thing.ts and was not read before this write -- read the file first, then retry.", + ) + mockSaveDirectly.mockRejectedValue(guardError) + + await tool.execute({ patch: updatePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockTask.observationRegistry.has(path.resolve("/workspace/project", "src/thing.ts"))).toBe(false) + expect(mockHandleError).toHaveBeenCalledWith("apply patch", guardError) + }) + + it.each(["pre-read", "post-read"])( + "update: continues the hunk read and records no observation when the %s stat fails", + async (which) => { + const statMock = mockedFsPromises.default.stat + const stable = { + dev: 7n, + ino: 4242n, + size: 1234n, + mtimeNs: 1700000000123456789n, + ctimeNs: 1700000000789999999n, + } + const failure = new Error("EACCES: permission denied") + if (which === "pre-read") { + statMock.mockRejectedValueOnce(failure).mockResolvedValueOnce(stable) + } else { + statMock.mockResolvedValueOnce(stable).mockRejectedValueOnce(failure) + } + + const guardError = new Error( + "File already exists at /workspace/project/src/thing.ts and was not read before this write -- read the file first, then retry.", + ) + mockSaveDirectly.mockRejectedValue(guardError) + + await tool.execute({ patch: updatePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // A stat failure is not evidence the file changed and not a reason to abort the + // read: the hunk was still processed against the content that was read. + expect(mockSaveDirectly).toHaveBeenCalledTimes(1) + // But a read whose version is unknown may not be observed... + expect(mockTask.observationRegistry.has(path.resolve("/workspace/project", "src/thing.ts"))).toBe(false) + // ...and the publish it leaves unauthorized is reported as an error, not a save. + expect(mockHandleError).toHaveBeenCalledWith("apply patch", guardError) + expect(mockPushToolResult).not.toHaveBeenCalledWith("Saved file") + }, + ) + + it("add: publishes the new file through the guarded saveDirectly with create kind", async () => { + mockedFileExistsAtPath.mockResolvedValueOnce(false) + + await tool.execute({ patch: addPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockSaveDirectly).toHaveBeenCalledWith( + "src/new.ts", + "new line one\nnew line two\n", + true, + true, + 1000, + "create", + ) + expect(mockPushToolResult).toHaveBeenCalledWith("Saved file") + expect(mockTask.didEditFile).toBe(true) + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("move: publishes the destination through the guarded saveDirectly with create kind", async () => { + await tool.execute({ patch: movePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockSaveDirectly).toHaveBeenCalledWith( + "src/new.ts", + "modified file content\n", + false, + true, + 1000, + "create", + ) + expect(mockTask.didEditFile).toBe(true) + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("move: surfaces a guarded destination publish failure as a tool error", async () => { + const publishError = new Error( + "Stale version at commit time -- the file changed while this write was being staged; re-read the file, then retry.", + ) + mockSaveDirectly.mockRejectedValue(publishError) + + await tool.execute({ patch: movePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // The destination saveDirectly is the guarded publish for a move: a failure there + // must route through handleError, reset the diff view, and leave no success result + // (the source file stays where it was). + expect(mockHandleError).toHaveBeenCalledWith("apply patch", publishError) + expect(vi.mocked(mockTask.diffViewProvider.reset)).toHaveBeenCalled() + expect(mockTask.didEditFile).toBe(false) + expect(mockPushToolResult).not.toHaveBeenCalledWith("Saved file") + }) + + it("update: surfaces the unobserved-existing remediation as a tool error", async () => { + const guardError = new Error( + "File already exists at /workspace/project/src/thing.ts and was not read before this write -- read the file first, then retry.", + ) + mockSaveDirectly.mockRejectedValue(guardError) + + await tool.execute({ patch: updatePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockHandleError).toHaveBeenCalledWith("apply patch", guardError) + expect(vi.mocked(mockTask.diffViewProvider.reset)).toHaveBeenCalled() + expect(mockTask.didEditFile).toBe(false) + expect(mockPushToolResult).not.toHaveBeenCalledWith("Saved file") + }) + + it("update: surfaces the stale-version remediation as a tool error", async () => { + const guardError = new Error( + "Stale version -- the file changed since you read it (expected v1, current v2); re-read the file, then retry.", + ) + mockSaveDirectly.mockRejectedValue(guardError) + + await tool.execute({ patch: updatePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockHandleError).toHaveBeenCalledWith("apply patch", guardError) + expect(vi.mocked(mockTask.diffViewProvider.reset)).toHaveBeenCalled() + expect(mockTask.didEditFile).toBe(false) + }) +}) diff --git a/src/core/tools/__tests__/editFileTool.spec.ts b/src/core/tools/__tests__/editFileTool.spec.ts index 1ff8d52a8d..9370c529f9 100644 --- a/src/core/tools/__tests__/editFileTool.spec.ts +++ b/src/core/tools/__tests__/editFileTool.spec.ts @@ -4,6 +4,7 @@ import fs from "fs/promises" import type { MockedFunction } from "vitest" import { fileExistsAtPath } from "../../../utils/fs" +import { ObservationRegistry } from "../../task/observationRegistry" import { isPathOutsideWorkspace } from "../../../utils/pathUtils" import { getReadablePath } from "../../../utils/path" import { ToolUse, ToolResponse, AskApproval, HandleError, PushToolResult } from "../../../shared/tools" @@ -12,12 +13,23 @@ import { editFileTool } from "../EditFileTool" vi.mock("fs/promises", () => ({ default: { readFile: vi.fn().mockResolvedValue(""), + stat: vi.fn().mockResolvedValue({ + dev: 7n, + ino: 4242n, + size: 1234n, + mtimeNs: 1700000000123456789n, + ctimeNs: 1700000000789999999n, + }), }, })) vi.mock("path", async () => { const originalPath = await vi.importActual("path") - return { + // The default import must be the mocked object too: the tools read path through + // `import path from "path"`, and spreading originalPath alone left the default + // import on the REAL module - production then resolved against the runner's cwd + // (drive-dependent on Windows) instead of the mocked resolve. + const mocked = { ...originalPath, resolve: vi.fn().mockImplementation((...args) => { const separator = process.platform === "win32" ? "\\" : "/" @@ -28,6 +40,7 @@ vi.mock("path", async () => { return to }), } + return { ...mocked, default: mocked } }) vi.mock("delay", () => ({ @@ -145,6 +158,9 @@ describe("editFileTool", () => { mockTask.fileContextTracker = { trackFileContext: vi.fn().mockResolvedValue(undefined), } + // Real registry: the regression test asserts which version token the tool's own + // read leaves behind, so a stub would only restate the call. + mockTask.observationRegistry = new ObservationRegistry() mockTask.say = vi.fn().mockResolvedValue(undefined) mockTask.ask = vi.fn().mockResolvedValue(undefined) mockTask.recordToolError = vi.fn() @@ -168,6 +184,7 @@ describe("editFileTool", () => { fileContent?: string isPartial?: boolean accessAllowed?: boolean + experiments?: Record } = {}, ): Promise { const fileExists = options.fileExists ?? true @@ -178,6 +195,13 @@ describe("editFileTool", () => { mockedFileExistsAtPath.mockResolvedValue(fileExists) mockedFsReadFile.mockResolvedValue(fileContent) mockTask.rooIgnoreController.validateAccess.mockReturnValue(accessAllowed) + mockTask.providerRef.deref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: options.experiments ?? {}, + }), + }) const nativeArgs: Record = { file_path: testFilePath, @@ -687,6 +711,117 @@ describe("editFileTool", () => { }) }) + describe("guarded write (S4b, epic #1375)", () => { + const focusDisruption = { preventFocusDisruption: true } + + it("publishes an existing-file edit through saveDirectly with edit kind", async () => { + const result = await executeEditFileTool( + { old_string: "Line 2", new_string: "Modified Line 2" }, + { fileExists: true, fileContent: "Line 1\nLine 2\nLine 3", experiments: focusDisruption }, + ) + + expect(mockTask.diffViewProvider.saveDirectly).toHaveBeenCalledWith( + testFilePath, + "Line 1\nModified Line 2\nLine 3", + false, + true, + 1000, + "edit", + ) + expect(mockTask.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(mockTask.didEditFile).toBe(true) + expect(result).toContain("Tool result message") + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("records the version its own read saw so the guarded edit is authorized", async () => { + // No prior read_file observation exists for this path. The tool's own read + // must leave the observation that authorizes saveDirectly("edit"), or the + // guarded publish rejects with "File not read yet". + const result = await executeEditFileTool( + { old_string: "Line 2", new_string: "Modified Line 2" }, + { fileExists: true, fileContent: "Line 1\nLine 2\nLine 3", experiments: focusDisruption }, + ) + + expect(mockTask.observationRegistry.get(absoluteFilePath)?.version).toBe( + "7:4242:1234:1700000000123456789:1700000000789999999", + ) + expect(mockTask.diffViewProvider.saveDirectly).toHaveBeenCalledWith( + testFilePath, + "Line 1\nModified Line 2\nLine 3", + false, + true, + 1000, + "edit", + ) + expect(result).toContain("Tool result message") + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("publishes new-file creation through saveDirectly with create kind", async () => { + await executeEditFileTool( + { old_string: "", new_string: "New file content" }, + { fileExists: false, experiments: focusDisruption }, + ) + + expect(mockTask.diffViewProvider.saveDirectly).toHaveBeenCalledWith( + testFilePath, + "New file content", + true, + true, + 1000, + "create", + ) + expect(mockTask.didEditFile).toBe(true) + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("surfaces the unobserved edit remediation as a tool error and publishes nothing", async () => { + const guardError = new Error("File not read yet -- read the file, then retry.") + mockTask.diffViewProvider.saveDirectly.mockRejectedValue(guardError) + + const result = await executeEditFileTool( + { old_string: "Line 2", new_string: "Modified Line 2" }, + { fileExists: true, fileContent: "Line 1\nLine 2\nLine 3", experiments: focusDisruption }, + ) + + expect(mockHandleError).toHaveBeenCalledWith("edit_file", guardError) + expect(result).toBeUndefined() + expect(mockTask.diffViewProvider.reset).toHaveBeenCalled() + expect(mockTask.didToolFailInCurrentTurn).toBe(true) + expect(mockTask.didEditFile).toBe(false) + }) + + it("surfaces the stale-version remediation as a tool error and publishes nothing", async () => { + const guardError = new Error( + "Stale version -- the file changed since you read it (expected v1, current v2); re-read the file, then retry.", + ) + mockTask.diffViewProvider.saveDirectly.mockRejectedValue(guardError) + + const result = await executeEditFileTool( + { old_string: "Line 2", new_string: "Modified Line 2" }, + { fileExists: true, fileContent: "Line 1\nLine 2\nLine 3", experiments: focusDisruption }, + ) + + expect(mockHandleError).toHaveBeenCalledWith("edit_file", guardError) + expect(result).toBeUndefined() + expect(mockTask.diffViewProvider.reset).toHaveBeenCalled() + expect(mockTask.didEditFile).toBe(false) + }) + + it("still fails a literal mismatch with the existing message before the guard runs", async () => { + const result = await executeEditFileTool( + { old_string: "NonExistent", new_string: "Whatever" }, + { fileExists: true, fileContent: "Line 1\nLine 2\nLine 3", experiments: focusDisruption }, + ) + + expect(result).toContain("No match found") + expect(result).toContain("") + expect(mockTask.diffViewProvider.saveDirectly).not.toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + }) + }) + describe("CRLF normalization", () => { it("preserves CRLF line endings on output", async () => { const contentWithCRLF = "Line 1\r\nLine 2\r\nLine 3" diff --git a/src/core/tools/__tests__/editTool.spec.ts b/src/core/tools/__tests__/editTool.spec.ts index a5f665b9e5..ba1985354b 100644 --- a/src/core/tools/__tests__/editTool.spec.ts +++ b/src/core/tools/__tests__/editTool.spec.ts @@ -4,6 +4,7 @@ import fs from "fs/promises" import type { MockedFunction } from "vitest" import { fileExistsAtPath } from "../../../utils/fs" +import { ObservationRegistry } from "../../task/observationRegistry" import { isPathOutsideWorkspace } from "../../../utils/pathUtils" import { getReadablePath } from "../../../utils/path" import { ToolUse, ToolResponse, AskApproval, HandleError, PushToolResult } from "../../../shared/tools" @@ -12,12 +13,23 @@ import { editTool } from "../EditTool" vi.mock("fs/promises", () => ({ default: { readFile: vi.fn().mockResolvedValue(""), + stat: vi.fn().mockResolvedValue({ + dev: 7n, + ino: 4242n, + size: 1234n, + mtimeNs: 1700000000123456789n, + ctimeNs: 1700000000789999999n, + }), }, })) vi.mock("path", async () => { const originalPath = await vi.importActual("path") - return { + // The default import must be the mocked object too: the tools read path through + // `import path from "path"`, and spreading originalPath alone left the default + // import on the REAL module - production then resolved against the runner's cwd + // (drive-dependent on Windows) instead of the mocked resolve. + const mocked = { ...originalPath, resolve: vi.fn().mockImplementation((...args) => { const separator = process.platform === "win32" ? "\\" : "/" @@ -28,6 +40,7 @@ vi.mock("path", async () => { return to }), } + return { ...mocked, default: mocked } }) vi.mock("delay", () => ({ @@ -141,6 +154,9 @@ describe("editTool", () => { mockTask.fileContextTracker = { trackFileContext: vi.fn().mockResolvedValue(undefined), } + // Real registry: the regression test asserts which version token the tool's own + // read leaves behind, so a stub would only restate the call. + mockTask.observationRegistry = new ObservationRegistry() mockTask.say = vi.fn().mockResolvedValue(undefined) mockTask.ask = vi.fn().mockResolvedValue(undefined) mockTask.recordToolError = vi.fn() @@ -169,6 +185,7 @@ describe("editTool", () => { fileContent?: string isPartial?: boolean accessAllowed?: boolean + experiments?: Record } = {}, ): Promise { const fileExists = options.fileExists ?? true @@ -179,6 +196,13 @@ describe("editTool", () => { mockedFileExistsAtPath.mockResolvedValue(fileExists) mockedFsReadFile.mockResolvedValue(fileContent) mockTask.rooIgnoreController.validateAccess.mockReturnValue(accessAllowed) + mockTask.providerRef.deref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: options.experiments ?? {}, + }), + }) const defaultParams = { file_path: testFilePath, @@ -424,4 +448,66 @@ describe("editTool", () => { expect(mockTask.fileContextTracker.trackFileContext).toHaveBeenCalledWith(testFilePath, "roo_edited") }) }) + + describe("guarded write (S4b, epic #1375)", () => { + const focusDisruption = { preventFocusDisruption: true } + + it("publishes through saveDirectly with edit kind", async () => { + const result = await executeEditTool( + { old_string: "Line 2", new_string: "Modified Line 2" }, + { fileContent: "Line 1\nLine 2\nLine 3", experiments: focusDisruption }, + ) + + expect(mockTask.diffViewProvider.saveDirectly).toHaveBeenCalledWith( + testFilePath, + "Line 1\nModified Line 2\nLine 3", + false, + true, + 1000, + "edit", + ) + expect(mockTask.didEditFile).toBe(true) + expect(result).toBe("Tool result message") + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("records the version its own read saw so the guarded edit is authorized", async () => { + // No prior read_file observation exists for this path. The tool's own read + // must leave the observation that authorizes saveDirectly("edit"), or the + // guarded publish rejects with "File not read yet". + const result = await executeEditTool( + { old_string: "Line 2", new_string: "Modified Line 2" }, + { fileContent: "Line 1\nLine 2\nLine 3", experiments: focusDisruption }, + ) + + expect(mockTask.observationRegistry.get(absoluteFilePath)?.version).toBe( + "7:4242:1234:1700000000123456789:1700000000789999999", + ) + expect(mockTask.diffViewProvider.saveDirectly).toHaveBeenCalledWith( + testFilePath, + "Line 1\nModified Line 2\nLine 3", + false, + true, + 1000, + "edit", + ) + expect(result).toBe("Tool result message") + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("surfaces the unobserved edit remediation as a tool error and publishes nothing", async () => { + const guardError = new Error("File not read yet -- read the file, then retry.") + mockTask.diffViewProvider.saveDirectly.mockRejectedValue(guardError) + + const result = await executeEditTool( + { old_string: "Line 2", new_string: "Modified Line 2" }, + { fileContent: "Line 1\nLine 2\nLine 3", experiments: focusDisruption }, + ) + + expect(mockHandleError).toHaveBeenCalledWith("edit", guardError) + expect(result).toBeUndefined() + expect(mockTask.diffViewProvider.reset).toHaveBeenCalled() + expect(mockTask.didEditFile).toBe(false) + }) + }) }) diff --git a/src/core/tools/__tests__/guardedWrite.spec.ts b/src/core/tools/__tests__/guardedWrite.spec.ts new file mode 100644 index 0000000000..3325707353 --- /dev/null +++ b/src/core/tools/__tests__/guardedWrite.spec.ts @@ -0,0 +1,735 @@ +/** + * 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. + */ + +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 { resolvePublishTarget, safeWriteText } from "../../../services/file-safety/safeWriteText" +import { computeVersionToken } from "../../../utils/versionToken" +import { acquireFileLock } from "../../../utils/fileLock" +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("../../../utils/fileLock", () => ({ + acquireFileLock: vi.fn(async () => releaseMock), // resolves to the release function +})) + +vi.mock("../../../services/file-safety/safeWriteText", () => ({ + safeWriteText: vi.fn(), + resolvePublishTarget: vi.fn(async (p: string) => p), +})) + +const mockedFsAccess = vi.mocked(fs.access) +const mockedComputeVersionToken = vi.mocked(computeVersionToken) +const mockedSafeWriteText = vi.mocked(safeWriteText) +const mockedResolvePublishTarget = vi.mocked(resolvePublishTarget) +let releaseMock: () => Promise = async () => {} +const mockedAcquireFileLock = vi.mocked(acquireFileLock) + +// -- 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 reads task.cwd, task.observationRegistry and + * task.abort (the flag Task.dispose() sets). 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(), + abort: false, + } + 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", { + preCommitVerify: expect.any(Function), + }) + }) + + it("fails with the read-first remediation when the file exists - nothing published", async () => { + mockedFsAccess.mockResolvedValue(undefined) + const task = createMockTask() + + await expect(guardedWrite(task, "existing.txt", "hello", "create")).rejects.toThrow( + "File already exists at " + + abs("existing.txt") + + " and was not read before this write -- read the file first, then retry.", + ) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + + it("rethrows I/O errors that are not ENOENT verbatim (no guard verdict on access failure)", async () => { + const failures = [{ code: "EACCES" }, null, "volume offline", new Error("EIO-ish failure")] + for (const failure of failures) { + mockedFsAccess.mockRejectedValueOnce(failure) + await expect(createIfAbsent(abs("io-error.txt"), "x")).rejects.toBe(failure) + } + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + }) + + describe("deleted-after-read target", () => { + it("normalizes an ENOENT from the version token into the re-read remediation", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("vanished.txt"), "v1") + const task = createMockTask({ observationRegistry: reg }) + + // The file was deleted after the read: the token computation fails + // with a raw ENOENT, which the guard must convert into the standard + // re-read-then-retry contract. + mockedComputeVersionToken.mockRejectedValue({ code: "ENOENT" }) + + await expect(guardedWrite(task, "vanished.txt", "next", "update")).rejects.toThrow( + "File was deleted after it was read", + ) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + + it("rethrows non-ENOENT token failures verbatim from replaceIfVersion", async () => { + const failure = { code: "EACCES" } + mockedComputeVersionToken.mockRejectedValueOnce(failure) + + await expect(replaceIfVersion(abs("locked.txt"), "v1", "next")).rejects.toBe(failure) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + }) + describe("unobserved update", () => { + it("succeeds when the file is absent (same create guard)", async () => { + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) + const task = createMockTask() + + await guardedWrite(task, "new-file.txt", "hello", "update") + + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("new-file.txt"), "hello", { + preCommitVerify: expect.any(Function), + }) + }) + + it("fails with the read-first remediation when the file exists - nothing published", async () => { + mockedFsAccess.mockResolvedValue(undefined) + const task = createMockTask() + + await expect(guardedWrite(task, "existing.txt", "hello", "update")).rejects.toThrow( + "File already exists at " + + abs("existing.txt") + + " and was not read before this write -- read the file first, then retry.", + ) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + }) + + describe("observed create", () => { + it("recreates a file that vanished after the read", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("gone.txt"), "v1") + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) + const task = createMockTask({ observationRegistry: reg }) + + await guardedWrite(task, "gone.txt", "back", "create") + + expect(mockedSafeWriteText).toHaveBeenCalledTimes(1) + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("gone.txt"), "back", { + preCommitVerify: 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", { + preCommitVerify: expect.any(Function), + }) + }) + + it("fails with the stale remediation suffix when the version moved", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("kept.txt"), "v1") + mockedFsAccess.mockResolvedValue(undefined) + mockedComputeVersionToken.mockResolvedValue("v2") + const task = createMockTask({ observationRegistry: reg }) + + await expect(guardedWrite(task, "kept.txt", "rewritten", "create")).rejects.toThrow( + "Stale version -- the file changed since you read it (expected v1, current v2); re-read the file, then retry.", + ) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + + it("defers to the version guard when the access check is denied (not ENOENT)", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("locked.txt"), "v1") + mockedFsAccess.mockRejectedValue({ code: "EACCES" }) + mockedComputeVersionToken.mockResolvedValue("v2") + const task = createMockTask({ observationRegistry: reg }) + + await expect(guardedWrite(task, "locked.txt", "rewritten", "create")).rejects.toThrow( + "Stale version -- the file changed since you read it (expected v1, current v2); re-read the file, then retry.", + ) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + }) + + describe("observed update (version CAS)", () => { + it("publishes when the on-disk version matches the observation", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("doc.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + await guardedWrite(task, "doc.txt", "new content", "update") + + expect(mockedComputeVersionToken).toHaveBeenCalledWith(abs("doc.txt")) + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("doc.txt"), "new content", { + preCommitVerify: expect.any(Function), + }) + }) + + it("fails with the stale remediation suffix when the version moved - nothing published", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("doc.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v2") + const task = createMockTask({ observationRegistry: reg }) + + await expect(guardedWrite(task, "doc.txt", "new content", "update")).rejects.toThrow( + "Stale version -- the file changed since you read it (expected v1, current v2); re-read the file, then retry.", + ) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + }) + + 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", { + preCommitVerify: expect.any(Function), + }) + }) + + it("fails with the stale remediation suffix when the version moved", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("doc.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v3") + const task = createMockTask({ observationRegistry: reg }) + + await expect(guardedWrite(task, "doc.txt", "patched", "edit")).rejects.toThrow( + "Stale version -- the file changed since you read it (expected v1, current v3); re-read the file, then retry.", + ) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + }) + + describe("observation refresh after a publish", () => { + it("records the published token so a later write needs no re-read", async () => { + const task = createMockTask() + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) + mockedComputeVersionToken.mockResolvedValue("v-created") + + await guardedWrite(task, "created.txt", "hello", "create") + + // The created file was never read, so without this the registry stays empty + // and the next write fails with "File not read yet". + expect(task.observationRegistry.get(abs("created.txt"))?.version).toBe("v-created") + + mockedFsAccess.mockResolvedValue(undefined) + mockedComputeVersionToken.mockResolvedValue("v-created") + await guardedWrite(task, "created.txt", "again", "update") + + expect(mockedSafeWriteText).toHaveBeenCalledTimes(2) + expect(mockedSafeWriteText).toHaveBeenLastCalledWith(abs("created.txt"), "again", { + preCommitVerify: expect.any(Function), + }) + }) + + it("replaces the read-time token after a successful update", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("doc.txt"), "v1") + mockedFsAccess.mockResolvedValue(undefined) + // First call: the CAS check. Second call: the token of what was just written. + mockedComputeVersionToken.mockResolvedValueOnce("v1").mockResolvedValueOnce("v2") + const task = createMockTask({ observationRegistry: reg }) + + await guardedWrite(task, "doc.txt", "patched", "edit") + + // Leaving v1 here makes the next edit fail with "Stale version" even though + // nothing else touched the file. + expect(reg.get(abs("doc.txt"))?.version).toBe("v2") + }) + }) + + describe("concurrency: per-path FIFO chain", () => { + it("two concurrent updates on one path - exactly one publishes, the other fails stale", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("shared.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + // The first publish changes the on-disk state (new token). + mockedSafeWriteText.mockImplementation(async () => { + mockedComputeVersionToken.mockResolvedValue("v2") + }) + + const p1 = guardedWrite(task, "shared.txt", "first", "update") + const p2 = guardedWrite(task, "shared.txt", "second", "update") + const [r1, r2] = await Promise.allSettled([p1, p2]) + + if (r1.status !== "fulfilled" || r2.status !== "rejected") { + throw new Error("expected exactly one publish, got " + r1.status + " / " + r2.status) + } + expect(mockedSafeWriteText).toHaveBeenCalledTimes(1) + expect(r2.reason.message).toBe( + "Stale version -- the file changed since you read it (expected v1, current v2); re-read the file, then retry.", + ) + }) + + it("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", { + preCommitVerify: expect.any(Function), + }) + }) + + it("evicts settled chain entries - a later write still serializes in order", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("evict.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + // A first write settles; its chain entry is evicted with it. + const p1 = guardedWrite(task, "evict.txt", "first", "update") + await expect(p1).resolves.toBeUndefined() + + // Two rapid writes submitted after the eviction must still run one + // at a time in submission order (the eviction must not drop the + // chain for in-flight or just-enqueued links). + 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("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", { + preCommitVerify: 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", { + preCommitVerify: expect.any(Function), + }) + }) + + it("serializes two spellings of one file through a single chain key", async () => { + const reg = new ObservationRegistry() + const canonical = abs("shared2.txt") + reg.observe(canonical, "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + // The first publish changes the on-disk state (new token). + mockedSafeWriteText.mockImplementation(async () => { + mockedComputeVersionToken.mockResolvedValue("v2") + }) + + // Plain spelling vs the trailing-separator spelling: with one chain key + // they are strictly ordered (first matches v1, second sees v2). + const p1 = guardedWrite(task, canonical, "first", "update") + const p2 = guardedWrite(task, canonical + "/", "second", "update") + const [r1, r2] = await Promise.allSettled([p1, p2]) + + if (r1.status !== "fulfilled" || r2.status !== "rejected") { + throw new Error("expected exactly one publish, got " + r1.status + " / " + r2.status) + } + expect(mockedSafeWriteText).toHaveBeenCalledTimes(1) + expect(r2.reason.message).toBe( + "Stale version -- the file changed since you read it (expected v1, current v2); re-read the file, then retry.", + ) + }) + }) + + describe("resetChain", () => { + it("detaches pending links so later writes start a fresh chain", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("x.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + await guardedWrite(task, "x.txt", "a", "update") + resetChain() + await guardedWrite(task, "x.txt", "b", "update") + + expect(mockedSafeWriteText).toHaveBeenLastCalledWith(abs("x.txt"), "b", { + preCommitVerify: expect.any(Function), + }) + }) + }) + + describe("atomicity of the guard check and publish", () => { + it("holds the lock across the version check and the publish", async () => { + const order: string[] = [] + releaseMock = async () => { + order.push("release") + } + mockedAcquireFileLock.mockImplementation(async () => { + order.push("acquire") + return releaseMock + }) + mockedComputeVersionToken.mockImplementation(async () => { + order.push("check") + return "v1" + }) + mockedSafeWriteText.mockImplementation(async () => { + order.push("publish") + }) + + const published = await replaceIfVersion(abs("x.txt"), "v1", "new") + + // A writer that honors the same lock cannot slip between the check and the publish, + // and the token handed back is recomputed before the lock is released. + expect(order).toEqual(["acquire", "check", "publish", "check", "release"]) + expect(published).toBe("v1") + }) + + it("locks the canonical publish target so a safeWriteJson writer serializes with it", async () => { + // safeWriteJson resolves its publish target before locking, and acquireFileLock runs + // with realpath:false. If the guarded write locked the caller's spelling instead, + // the alias and the referent would take two different lock files and a guarded + // write could race past a safeWriteJson publish of the same file. + const alias = abs("alias.txt") + const referent = abs("target.txt") + mockedResolvePublishTarget.mockResolvedValueOnce(referent) + mockedComputeVersionToken.mockResolvedValue("v1") + + await replaceIfVersion(alias, "v1", "new") + + expect(mockedAcquireFileLock).toHaveBeenCalledWith(referent) + expect(mockedAcquireFileLock).not.toHaveBeenCalledWith(alias) + // The publish still goes through the path the caller (and safeWriteText's own + // resolution) owns, so the observation key and symlink semantics are unchanged. + expect(mockedSafeWriteText).toHaveBeenCalledWith(alias, "new", { preCommitVerify: expect.any(Function) }) + }) + + it("releases the lock when the guard rejects as stale", async () => { + const order: string[] = [] + releaseMock = async () => { + order.push("release") + } + mockedAcquireFileLock.mockImplementation(async () => { + order.push("acquire") + return releaseMock + }) + mockedComputeVersionToken.mockResolvedValue("v2") + + await expect(replaceIfVersion(abs("x.txt"), "v1", "new")).rejects.toThrow(/Stale version/) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + expect(order).toEqual(["acquire", "release"]) + }) + + it("re-checks the version token immediately before the commit rename", async () => { + // The pre-flight check passes, then a writer that ignores the advisory lock + // rewrites the file while the staged copy is being written and fsynced. + mockedComputeVersionToken.mockResolvedValueOnce("v1").mockResolvedValueOnce("v2") + + await replaceIfVersion(abs("x.txt"), "v1", "new") + + // The publish is handed a verifier, so the comparison runs after staging and fsync + // rather than only before them: the race window shrinks to the commit syscall, and + // a newer version cannot be replaced by an older one without the write being refused. + const verifier = mockedSafeWriteText.mock.calls[0]?.[2]?.preCommitVerify + expect(verifier).toBeTypeOf("function") + if (verifier) { + await expect(verifier(abs("x.txt"))).rejects.toThrow(/Stale version at commit time/) + } + }) + + it("re-asserts absence immediately before the commit rename", async () => { + mockedComputeVersionToken.mockResolvedValue("v1") + // Guard check: the file is absent, so the create is allowed to proceed. + mockedFsAccess.mockRejectedValueOnce(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + + await createIfAbsent(abs("new.txt"), "content") + + const verifier = mockedSafeWriteText.mock.calls[0]?.[2]?.preCommitVerify + expect(verifier).toBeTypeOf("function") + if (verifier) { + // Still absent: the commit may land. + mockedFsAccess.mockRejectedValueOnce(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + await expect(verifier(abs("new.txt"))).resolves.toBeUndefined() + // Created by someone during the staging span: the commit must be refused rather + // than clobbering whoever created the file. + mockedFsAccess.mockResolvedValueOnce(undefined) + await expect(verifier(abs("new.txt"))).rejects.toThrow(/appeared at/) + } + }) + + it("holds the lock across the absence check and the create publish", async () => { + const order: string[] = [] + releaseMock = async () => { + order.push("release") + } + mockedAcquireFileLock.mockImplementation(async () => { + order.push("acquire") + return releaseMock + }) + mockedFsAccess.mockImplementation(async () => { + order.push("check") + throw { code: "ENOENT" } + }) + mockedSafeWriteText.mockImplementation(async () => { + order.push("publish") + }) + + await createIfAbsent(abs("new.txt"), "hello") + + expect(order).toEqual(["acquire", "check", "publish", "release"]) + }) + }) + + describe("cancellation while queued (S4b lifecycle)", () => { + it("does not publish a queued write after the issuing task is disposed", async () => { + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) + mockedComputeVersionToken.mockResolvedValue("v1") + // The first link holds the path's chain, so the second write is genuinely queued. + 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() + }) + }) +}) + +describe("commit-time verifier (S4a, epic #1375)", () => { + beforeEach(() => { + vi.resetAllMocks() + resetChain() + mockedSafeWriteText.mockImplementation((async ( + p: string, + _c: string, + options: { preCommitVerify?: (target: string) => Promise }, + ) => { + // Run the verifier the way safeWriteText does: after staging, immediately before the + // commit rename. Without this the callback would never execute and the assertions below + // would pass against a verifier that was never called. + await options.preCommitVerify?.(p) + return undefined + }) as never) + }) + + it("propagates a non-ENOENT access error from the create verifier instead of publishing", async () => { + const ioError = Object.assign(new Error("EACCES"), { code: "EACCES" }) + mockedFsAccess.mockRejectedValueOnce({ code: "ENOENT" }) + mockedFsAccess.mockRejectedValueOnce(ioError) + const task = createMockTask() + + await expect(guardedWrite(task, "new-file.txt", "hello", "create")).rejects.toBe(ioError) + }) + + it("rejects when the file appears at commit time on the create path", async () => { + mockedFsAccess.mockRejectedValueOnce({ code: "ENOENT" }) + mockedFsAccess.mockResolvedValue(undefined) + const task = createMockTask() + + await expect(guardedWrite(task, "new-file.txt", "hello", "create")).rejects.toThrow(/File appeared at/) + }) + + it("converts a commit-time ENOENT on the observed update path into the re-read remediation", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("doc.txt"), "v1") + const task = createMockTask({ observationRegistry: reg }) + mockedFsAccess.mockResolvedValue(undefined) + // The guard's read-time check runs first, then the commit-time verifier re-computes the + // token: the first call is the guard, the second is the verifier finding the file gone. + mockedComputeVersionToken.mockResolvedValueOnce("v1") + mockedComputeVersionToken.mockRejectedValueOnce(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + + await expect(guardedWrite(task, "doc.txt", "new content", "update")).rejects.toThrow( + /no longer exists; re-read the file, then retry/, + ) + }) +}) diff --git a/src/core/tools/__tests__/readFileTool.spec.ts b/src/core/tools/__tests__/readFileTool.spec.ts index 5f9033eb7c..67f7202b01 100644 --- a/src/core/tools/__tests__/readFileTool.spec.ts +++ b/src/core/tools/__tests__/readFileTool.spec.ts @@ -13,6 +13,9 @@ */ import path from "path" +import type { Stats } from "fs" + +import type { LegacyReadFileParams } from "@roo-code/types" import type { ToolUse } from "../../../shared/tools" import type { Task } from "../../task/Task" @@ -20,6 +23,8 @@ import type { Task } from "../../task/Task" import { isBinaryFile } from "isbinaryfile" import { readFileTool, ReadFileTool } from "../ReadFileTool" +import { ObservationRegistry } from "../../task/observationRegistry" +import { computeVersionToken } from "../../../utils/versionToken" import { formatResponse } from "../../prompts/responses" import { validateImageForProcessing, @@ -139,6 +144,7 @@ interface MockTaskOptions { rooIgnoreAllowed?: boolean maxImageFileSize?: number maxTotalImageSize?: number + observationRegistry?: ObservationRegistry } function createMockTask(options: MockTaskOptions = {}) { @@ -146,6 +152,9 @@ function createMockTask(options: MockTaskOptions = {}) { return { cwd: "/test/workspace", + // Mirror Task: every task always owns an observation registry (A2, #1375). + // Tests asserting on observations pass their own instance via options. + observationRegistry: options.observationRegistry ?? new ObservationRegistry(), api: { getModel: vi.fn().mockReturnValue({ info: { supportsImages }, @@ -190,7 +199,18 @@ describe("ReadFileTool", () => { vi.clearAllMocks() // Default mock implementations - mockedFsStat.mockResolvedValue({ isDirectory: () => false } as any) + // The stat default carries BigIntStats fields (A2, epic #1375): reads now + // token-ize the pre/post stats, so the default must look like a real bigint stat. + // Tests overriding it do so per-call with mockResolvedValue(Once). + mockedFsStat.mockResolvedValue({ + isDirectory: () => false, + dev: BigInt(1), + ino: BigInt(2), + size: BigInt(300), + mtimeNs: BigInt(4_000_000_000n), + ctimeNs: BigInt(5_000_000_000n), + // Cast: the mock only implements the members the tool and versionToken read. + } as unknown as Stats) mockedIsBinaryFile.mockResolvedValue(false) mockedFsReadFile.mockResolvedValue(Buffer.from("test content")) mockedReadWithSlice.mockReturnValue({ @@ -1555,5 +1575,313 @@ 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).toBe(path.resolve("/test/workspace", "existing.ts")) + expect(calledVersion).toBe("1:2:300:4000000000:5000000000") + + // 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] + // Exact key and token, mirroring the native-format test: a mismatch between the + // key the read side records under and the key the guard looks up would otherwise + // still pass a substring/regex assertion. + expect(calledPath).toBe(path.resolve("/test/workspace", "legacy.ts")) + expect(calledVersion).toBe("1:2:300:4000000000:5000000000") + // The observation is retrievable under that exact key. + const obs = reg.get(calledPath) + expect(obs).toBeDefined() + expect(obs!.version).toBe(calledVersion) + }) + + 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/__tests__/searchReplaceTool.spec.ts b/src/core/tools/__tests__/searchReplaceTool.spec.ts index 5cf10790d4..619784e55f 100644 --- a/src/core/tools/__tests__/searchReplaceTool.spec.ts +++ b/src/core/tools/__tests__/searchReplaceTool.spec.ts @@ -4,6 +4,7 @@ import fs from "fs/promises" import type { MockedFunction } from "vitest" import { fileExistsAtPath } from "../../../utils/fs" +import { ObservationRegistry } from "../../task/observationRegistry" import { isPathOutsideWorkspace } from "../../../utils/pathUtils" import { getReadablePath } from "../../../utils/path" import { ToolUse, ToolResponse, AskApproval, HandleError, PushToolResult } from "../../../shared/tools" @@ -12,12 +13,23 @@ import { searchReplaceTool } from "../SearchReplaceTool" vi.mock("fs/promises", () => ({ default: { readFile: vi.fn().mockResolvedValue(""), + stat: vi.fn().mockResolvedValue({ + dev: 7n, + ino: 4242n, + size: 1234n, + mtimeNs: 1700000000123456789n, + ctimeNs: 1700000000789999999n, + }), }, })) vi.mock("path", async () => { const originalPath = await vi.importActual("path") - return { + // The default import must be the mocked object too: the tools read path through + // `import path from "path"`, and spreading originalPath alone left the default + // import on the REAL module - production then resolved against the runner's cwd + // (drive-dependent on Windows) instead of the mocked resolve. + const mocked = { ...originalPath, resolve: vi.fn().mockImplementation((...args) => { const separator = process.platform === "win32" ? "\\" : "/" @@ -28,6 +40,7 @@ vi.mock("path", async () => { return to }), } + return { ...mocked, default: mocked } }) vi.mock("delay", () => ({ @@ -143,6 +156,9 @@ describe("searchReplaceTool", () => { mockCline.fileContextTracker = { trackFileContext: vi.fn().mockResolvedValue(undefined), } + // Real registry: the regression test asserts which version token the tool's own + // read leaves behind, so a stub would only restate the call. + mockCline.observationRegistry = new ObservationRegistry() mockCline.say = vi.fn().mockResolvedValue(undefined) mockCline.ask = vi.fn().mockResolvedValue(undefined) mockCline.recordToolError = vi.fn() @@ -166,6 +182,7 @@ describe("searchReplaceTool", () => { fileContent?: string isPartial?: boolean accessAllowed?: boolean + experiments?: Record } = {}, ): Promise { const fileExists = options.fileExists ?? true @@ -176,6 +193,13 @@ describe("searchReplaceTool", () => { mockedFileExistsAtPath.mockResolvedValue(fileExists) mockedFsReadFile.mockResolvedValue(fileContent) mockCline.rooIgnoreController.validateAccess.mockReturnValue(accessAllowed) + mockCline.providerRef.deref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: options.experiments ?? {}, + }), + }) const nativeArgs: Record = { file_path: testFilePath, @@ -439,4 +463,66 @@ describe("searchReplaceTool", () => { expect(mockAskApproval).toHaveBeenCalled() }) }) + + describe("guarded write (S4b, epic #1375)", () => { + const focusDisruption = { preventFocusDisruption: true } + + it("publishes through saveDirectly with edit kind", async () => { + const result = await executeSearchReplaceTool( + { old_string: "Line 2", new_string: "Modified Line 2" }, + { fileContent: "Line 1\nLine 2\nLine 3", experiments: focusDisruption }, + ) + + expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalledWith( + testFilePath, + "Line 1\nModified Line 2\nLine 3", + false, + true, + 1000, + "edit", + ) + expect(mockCline.didEditFile).toBe(true) + expect(result).toBe("Tool result message") + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("records the version its own read saw so the guarded edit is authorized", async () => { + // No prior read_file observation exists for this path. The tool's own read + // must leave the observation that authorizes saveDirectly("edit"), or the + // guarded publish rejects with "File not read yet". + const result = await executeSearchReplaceTool( + { old_string: "Line 2", new_string: "Modified Line 2" }, + { fileContent: "Line 1\nLine 2\nLine 3", experiments: focusDisruption }, + ) + + expect(mockCline.observationRegistry.get(absoluteFilePath)?.version).toBe( + "7:4242:1234:1700000000123456789:1700000000789999999", + ) + expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalledWith( + testFilePath, + "Line 1\nModified Line 2\nLine 3", + false, + true, + 1000, + "edit", + ) + expect(result).toBe("Tool result message") + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("surfaces the unobserved edit remediation as a tool error and publishes nothing", async () => { + const guardError = new Error("File not read yet -- read the file, then retry.") + mockCline.diffViewProvider.saveDirectly.mockRejectedValue(guardError) + + const result = await executeSearchReplaceTool( + { old_string: "Line 2", new_string: "Modified Line 2" }, + { fileContent: "Line 1\nLine 2\nLine 3", experiments: focusDisruption }, + ) + + expect(mockHandleError).toHaveBeenCalledWith("search and replace", guardError) + expect(result).toBeUndefined() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockCline.didEditFile).toBe(false) + }) + }) }) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 52a7e3c052..930bc1661c 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -26,6 +26,13 @@ vi.mock("delay", () => ({ default: vi.fn(), })) +// The focus-disruption save path reads the original file content via fs.readFile. +vi.mock("fs/promises", () => ({ + default: { + readFile: vi.fn().mockResolvedValue("original content"), + }, +})) + vi.mock("../../../utils/fs", () => ({ fileExistsAtPath: vi.fn().mockResolvedValue(false), createDirectoriesForFile: vi.fn().mockResolvedValue([]), @@ -156,6 +163,11 @@ describe("writeToFileTool", () => { userEdits: null, finalContent: "final content", }), + saveDirectly: vi.fn().mockResolvedValue({ + newProblemsMessage: "", + userEdits: undefined, + finalContent: "final content", + }), scrollToFirstDiff: vi.fn(), updateDiagnosticSettings: vi.fn(), pushToolWriteResult: vi.fn().mockImplementation(async function ( @@ -187,6 +199,7 @@ describe("writeToFileTool", () => { mockCline.say = vi.fn().mockResolvedValue(undefined) mockCline.ask = vi.fn().mockResolvedValue(undefined) mockCline.recordToolError = vi.fn() + mockCline.processQueuedMessages = vi.fn() mockCline.sayAndCreateMissingParamError = vi.fn().mockResolvedValue("Missing param error") mockAskApproval = vi.fn().mockResolvedValue(true) @@ -204,6 +217,7 @@ describe("writeToFileTool", () => { fileExists?: boolean isPartial?: boolean accessAllowed?: boolean + experiments?: Record } = {}, ): Promise { // Configure mocks based on test scenario @@ -213,6 +227,13 @@ describe("writeToFileTool", () => { mockedFileExistsAtPath.mockResolvedValue(fileExists) mockCline.rooIgnoreController.validateAccess.mockReturnValue(accessAllowed) + mockCline.providerRef.deref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: options.experiments ?? {}, + }), + }) // Create a tool use object const toolUse: ToolUse = { @@ -450,6 +471,58 @@ describe("writeToFileTool", () => { }) }) + describe("guarded write (S4b, epic #1375)", () => { + const focusDisruption = { preventFocusDisruption: true } + + it("publishes through saveDirectly with create kind when the write is approved", async () => { + const result = await executeWriteFileTool({}, { fileExists: true, experiments: focusDisruption }) + + expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalledWith( + testFilePath, + testContent, + false, + true, + 1000, + "create", + ) + expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(mockCline.fileContextTracker.trackFileContext).toHaveBeenCalledWith(testFilePath, "roo_edited") + expect(mockCline.didEditFile).toBe(true) + expect(mockCline.consecutiveMistakeCount).toBe(0) + expect(result).toBe("Tool result message") + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("surfaces the unobserved-existing remediation as a tool error and publishes nothing", async () => { + const guardError = new Error( + `File already exists at ${absoluteFilePath} and was not read before this write -- read the file first, then retry.`, + ) + mockCline.diffViewProvider.saveDirectly.mockRejectedValue(guardError) + + const result = await executeWriteFileTool({}, { fileExists: true, experiments: focusDisruption }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", guardError) + expect(result).toBeUndefined() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockCline.diffViewProvider.saveChanges).not.toHaveBeenCalled() + expect(mockCline.didEditFile).toBe(false) + }) + + it("surfaces the stale-version remediation as a tool error and publishes nothing", async () => { + const guardError = new Error( + "Stale version -- the file changed since you read it (expected v1, current v2); re-read the file, then retry.", + ) + mockCline.diffViewProvider.saveDirectly.mockRejectedValue(guardError) + + const result = await executeWriteFileTool({}, { fileExists: true, experiments: focusDisruption }) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", guardError) + expect(result).toBeUndefined() + expect(mockCline.diffViewProvider.reset).toHaveBeenCalled() + expect(mockCline.didEditFile).toBe(false) + }) + }) + describe("error handling", () => { it("handles general file operation errors", async () => { mockCline.diffViewProvider.open.mockRejectedValue(new Error("General error")) diff --git a/src/core/tools/guardedWrite.ts b/src/core/tools/guardedWrite.ts new file mode 100644 index 0000000000..bb340bd1ee --- /dev/null +++ b/src/core/tools/guardedWrite.ts @@ -0,0 +1,387 @@ +/** + * 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. + */ + +import * as fs from "fs/promises" +import * as path from "path" + +import { resolvePublishTarget, safeWriteText } from "../../services/file-safety/safeWriteText" +import { computeVersionToken } from "../../utils/versionToken" +import { acquireFileLock } from "../../utils/fileLock" +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" + } +} + +/** + * 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 + } +} + +// -- 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 +} + +/** + * Run a guard check and its publish as one atomic step with respect to every writer + * that honors the same advisory lock (safeWriteJson acquires it through fileLock). + * Without the lock the version token can change between the check and the rename, so + * a write that looked valid on the check can overwrite a newer file. + */ +async function withWriteLock(absolutePath: string, run: () => Promise): Promise { + // Lock the canonical publish target, not the caller's spelling of the path. + // safeWriteJson resolves its publish target before locking, and acquireFileLock + // runs with realpath:false, so an alias and its referent would otherwise take two + // different lock files: a guarded write through the alias would not serialize with + // a safeWriteJson publish through the referent and could overwrite it. + const lockPath = await resolvePublishTarget(absolutePath) + const release = await acquireFileLock(lockPath) + try { + return await run() + } finally { + await release() + } +} + +/** 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" + } +} + +/** + * Publish content only if the target file does not exist. + * + * 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 { + return withWriteLock(absolutePath, async () => { + 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 under the lock: no other lock-honoring writer can create it between + // this check and the publish. A writer that does NOT honor the advisory lock can + // still create the file while the staged copy is being written and fsynced, so + // absence is re-asserted inside safeWriteText immediately before the commit + // rename: the window then covers only that syscall, and a file that appears + // before verification rejects the write instead of being silently clobbered; + // one that appears after verification returns can still be overwritten. + await safeWriteText(absolutePath, content, { + preCommitVerify: async (targetPath) => { + try { + await fs.access(targetPath) + } catch (error: unknown) { + if (errorCode(error) === "ENOENT") { + return + } + throw error + } + throw new GuardRejectedError( + "File appeared at " + + targetPath + + " while this write was being staged -- read the file first, then retry.", + absolutePath, + ) + }, + }) + // Recomputed under the same lock: the caller records this as the new + // observation, so a later write is compared against what this publish + // actually wrote rather than against no observation at all. + return computeVersionToken(absolutePath) + } + + throw new GuardRejectedError( + "File already exists at " + + absolutePath + + " and was not read before this write -- read the file first, then retry.", + absolutePath, + ) + }) +} + +/** + * Publish content only if the current on-disk version token equals + * expectedVersion (the token observed at read time). + * + * 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 { + return withWriteLock(absolutePath, async () => { + let currentVersion: string + try { + currentVersion = await computeVersionToken(absolutePath) + } catch (error: unknown) { + if (errorCode(error) === "ENOENT") { + // The observed file was deleted after the read: the version recorded + // at read time no longer exists on disk. Normalize the raw ENOENT + // into the guard's re-read-then-retry contract so the caller gets a + // remediation it can act on, not a raw errno. + throw 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, + ) + } + // A real I/O failure (EACCES, EIO, ...) -- not a guard verdict. + throw error + } + + if (currentVersion === expectedVersion) { + // The check and the publish run under the same lock, so the token cannot + // change between them for any writer that honors it. A writer that does NOT + // honor the advisory lock can still rewrite the file while the staged copy is + // being written and fsynced, so the token is re-checked inside safeWriteText + // immediately before the commit rename: the window then covers only that + // syscall, and the race surfaces as a rejected stale write instead of a newer + // version being replaced by the older one. + await safeWriteText(absolutePath, content, { + preCommitVerify: async (targetPath) => { + let verifiedVersion: string + try { + verifiedVersion = await computeVersionToken(targetPath) + } catch (error: unknown) { + if (errorCode(error) === "ENOENT") { + throw 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, + ) + } + throw error + } + if (verifiedVersion !== expectedVersion) { + throw new GuardRejectedError( + "Stale version at commit time -- the file changed while this write was being " + + "staged (expected " + + expectedVersion + + ", current " + + verifiedVersion + + "); re-read the file, then retry.", + absolutePath, + ) + } + }, + }) + // The publish moved the token; hand the new one back so the caller can + // record it instead of leaving the read-time token in the registry. + return computeVersionToken(absolutePath) + } + + throw new GuardRejectedError( + "Stale version -- the file changed since you read it (expected " + + expectedVersion + + ", current " + + currentVersion + + "); re-read the file, then retry.", + absolutePath, + ) + }) +} + +/** + * Unobserved-edit guard: an edit-style write without a prior observation is + * rejected before any I/O. The literal-match / patch logic stays with the + * tools in S4b; this guard only verifies that a read happened first. + * + * Returns Promise because the rejection is total: this function + * never resolves. + */ +export async function unobservedEditGuard(absolutePath: string): Promise { + throw new GuardRejectedError("File not read yet -- read the file, then retry.", absolutePath) +} + +// -- Public API -------------------------------------------------------------- + +/** + * Resolve a relative or absolute path against task.cwd. + * + * path.resolve also normalizes an already-absolute input (collapsing "." / ".." + * segments and trailing separators), so the key always matches the + * ObservationRegistry key recorded at read time (ReadFileTool observes under + * path.resolve(task.cwd, relPath)) and two spellings of one file share one + * FIFO chain. + */ +function resolveAbsolutePath(task: Task, relPathOrAbsolute: string): string { + return path.resolve(task.cwd, relPathOrAbsolute) +} + +/** + * Guarded write entry point. + * + * 1. Resolves the absolute path against task.cwd. + * 2. Consults the task's S2 observation registry 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. + * 3. Runs the chosen guard on the per-path FIFO chain so concurrent writes to + * the same path are deterministically ordered. + */ +export async function guardedWrite( + task: Task, + relPathOrAbsolute: string, + content: string, + kind: GuardedWriteKind = "update", +): Promise { + const absolutePath = resolveAbsolutePath(task, relPathOrAbsolute) + + // Snapshot the observation when the write is issued, not when its link runs. + // A refresh after a publish must not retroactively validate a second write that + // was issued against the pre-publish state: two concurrent writes to one path + // still have exactly one winner. + 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) + } + + let publishedVersion: string + + 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.) + publishedVersion = await createIfAbsent(absolutePath, content) + } else if (kind === "edit") { + publishedVersion = await replaceIfVersion(absolutePath, obs.version, content) + } else if (kind === "create" && (await fileIsAbsent(absolutePath))) { + // kind is "create" or "update": a "create" on a file that vanished + // after the read recreates it. + publishedVersion = await createIfAbsent(absolutePath, content) + } else { + // Otherwise the version recorded at read time must still match the + // on-disk token. + publishedVersion = await replaceIfVersion(absolutePath, obs.version, content) + } + + // The file on disk is now what this write published. Record that token: a + // later guarded write must compare against it rather than the read-time (or, + // for a created file, missing) token, which is what the result message + // "you do not need to re-read the file" promises. + task.observationRegistry.observe(absolutePath, publishedVersion) + }) +} + +/** + * Reset the per-path tail-promise chains (test hook). + */ +export function resetChain(): void { + pendingChains.clear() +} diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index e4b15aa27e..e7568413ab 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -976,7 +976,7 @@ }, "core/tools/__tests__/readFileTool.spec.ts": { "@typescript-eslint/no-explicit-any": { - "count": 96 + "count": 95 } }, "core/tools/__tests__/runSlashCommandTool.spec.ts": { @@ -1716,7 +1716,7 @@ }, "utils/safeWriteJson.ts": { "@typescript-eslint/no-explicit-any": { - "count": 4 + "count": 3 } }, "utils/tts.ts": { diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index bb3368f063..51bce14cc7 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -18,6 +18,7 @@ import { arePathsEqual, getReadablePath } from "../../utils/path" import { formatResponse } from "../../core/prompts/responses" import { diagnosticsToProblemsString, getNewDiagnostics } from "../diagnostics" import { Task } from "../../core/task/Task" +import { guardedWrite, type GuardedWriteKind } from "../../core/tools/guardedWrite" import { DecorationController } from "./DecorationController" @@ -1136,6 +1137,10 @@ export class DiffViewProvider { * @param relPath - Relative path to the file * @param content - Content to write to the file * @param openFile - Whether to show the file in editor (false = open in memory only for diagnostics) + * @param writeKind - Guarded-write kind that selects the S4a guard for this publish. + * Defaults to "create" because this method always publishes a complete file + * content: an unobserved target may only be created when absent, and an + * observed target must still carry the version token recorded at read time. * @returns Result of the save operation including any new problems detected */ async saveDirectly( @@ -1144,6 +1149,7 @@ export class DiffViewProvider { openFile: boolean = true, diagnosticsEnabled: boolean = true, writeDelayMs: number = DEFAULT_WRITE_DELAY_MS, + writeKind: GuardedWriteKind = "create", ): Promise<{ newProblemsMessage: string | undefined userEdits: string | undefined @@ -1154,9 +1160,19 @@ export class DiffViewProvider { // Get diagnostics before editing the file this.preDiagnostics = vscode.languages.getDiagnostics() - // Write the content directly to the file + // Publish through the S4 guarded-write API (epic #1375): an unobserved + // write to an existing file and a stale observed version are rejected + // with a re-read-then-retry remediation instead of overwriting the file. + // Concurrent in-process writes to the same path are already ordered by + // the guard's per-path FIFO chain, so no additional locking is added here. + const task = this.taskRef.deref() + if (!task) { + // Fail closed: without the owning task the observation registry is + // unreachable and the write cannot be guarded. + throw new Error("Cannot guard the write: the owning task is no longer available") + } await createDirectoriesForFile(absolutePath) - await fs.writeFile(absolutePath, content, "utf-8") + await guardedWrite(task, relPath, content, writeKind) // Open the document to ensure diagnostics are loaded // When openFile is false (PREVENT_FOCUS_DISRUPTION enabled), we only open in memory diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 00b3dcaf7a..7f73f06586 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -6,6 +6,14 @@ import { DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" import { makeRange, makeTextDocument, makeTextEditor, makeUri } from "../../../test-utils/vscode" +import * as fs from "fs/promises" + +import { computeVersionToken } from "../../../utils/versionToken" +import { safeWriteText } from "../../../services/file-safety/safeWriteText" +import { ObservationRegistry } from "../../../core/task/observationRegistry" +import { acquireFileLock } from "../../../utils/fileLock" +import type { Task } from "../../../core/task/Task" + // Mock delay vi.mock("delay", () => ({ default: vi.fn().mockResolvedValue(undefined), @@ -16,9 +24,31 @@ vi.mock("fs/promises", () => ({ readFile: vi.fn().mockResolvedValue("file content"), writeFile: vi.fn().mockResolvedValue(undefined), access: vi.fn().mockResolvedValue(undefined), + mkdir: vi.fn().mockResolvedValue(undefined), + rename: vi.fn().mockResolvedValue(undefined), + unlink: vi.fn().mockResolvedValue(undefined), +})) + +// Mock safeWriteText (used by saveDirectly) +vi.mock("../../../services/file-safety/safeWriteText", () => ({ + safeWriteText: vi.fn().mockResolvedValue(undefined), + // guardedWrite locks the canonical publish target, so the module mock has to + // expose the resolver too; identity keeps this spec's path handling unchanged. + resolvePublishTarget: vi.fn(async (p: string) => p), +})) + +// Mock the S1 version token (used by the S4 guarded write); the real +// computeVersionToken needs fs.stat, which is not part of the fs/promises mock above. +vi.mock("../../../utils/versionToken", () => ({ + computeVersionToken: vi.fn(), })) // Mock utils +// Mock the advisory lock the guarded write uses to keep its check and publish atomic. +vi.mock("../../../utils/fileLock", () => ({ + acquireFileLock: vi.fn(async () => async () => {}), +})) + vi.mock("../../../utils/fs", () => ({ createDirectoriesForFile: vi.fn().mockResolvedValue([]), })) @@ -26,7 +56,10 @@ vi.mock("../../../utils/fs", () => ({ // Mock path vi.mock("path", () => ({ resolve: vi.fn((cwd, relPath) => `${cwd}/${relPath}`), + isAbsolute: vi.fn((p: string) => p.startsWith("/")), basename: vi.fn((path) => path.split("/").pop()), + dirname: vi.fn((path) => path.split("/").slice(0, -1).join("/") || "/"), + join: (...args: string[]) => args.join("/"), })) // Mock vscode @@ -150,8 +183,11 @@ describe("DiffViewProvider", () => { return mockWorkspaceEdit as any }) - // Create a mock Task instance + // Create a mock Task instance. The guarded write (S4b) consults the task's + // S2 observation registry, so the mock carries a real (in-memory) instance. mockTask = { + cwd: mockCwd, + observationRegistry: new ObservationRegistry(), providerRef: { deref: vi.fn().mockReturnValue({ getState: vi.fn().mockResolvedValue({ @@ -784,6 +820,13 @@ describe("DiffViewProvider", () => { // Mock vscode functions vi.mocked(vscode.window.showTextDocument).mockResolvedValue({} as any) vi.mocked(vscode.languages.getDiagnostics).mockReturnValue([]) + + // Baseline for the single-writer flow these tests encode: the file was read + // before the write, so the observation registry holds the version token the + // guarded write recomputes and compares, and the target exists on disk. + mockTask.observationRegistry.observe(`${mockCwd}/test.ts`, "v1") + vi.mocked(computeVersionToken).mockResolvedValue("v1") + vi.mocked(fs.access).mockResolvedValue(undefined) }) it("should write content directly to file without opening diff view", async () => { @@ -792,9 +835,11 @@ describe("DiffViewProvider", () => { const result = await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 2000) - // Verify file was written - const fs = await import("fs/promises") - expect(fs.writeFile).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", "utf-8") + // Verify file was written via safeWriteText + const { safeWriteText } = await import("../../../services/file-safety/safeWriteText") + expect(safeWriteText).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", { + preCommitVerify: expect.any(Function), + }) // Verify file was opened without focus expect(vscode.window.showTextDocument).toHaveBeenCalledWith( @@ -815,9 +860,11 @@ describe("DiffViewProvider", () => { it("should not open file when openWithoutFocus is false", async () => { await diffViewProvider.saveDirectly("test.ts", "new content", false, true, 1000) - // Verify file was written - const fs = await import("fs/promises") - expect(fs.writeFile).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", "utf-8") + // Verify file was written via safeWriteText + const { safeWriteText } = await import("../../../services/file-safety/safeWriteText") + expect(safeWriteText).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", { + preCommitVerify: expect.any(Function), + }) // Verify file was NOT opened expect(vscode.window.showTextDocument).not.toHaveBeenCalled() @@ -830,9 +877,11 @@ describe("DiffViewProvider", () => { await diffViewProvider.saveDirectly("test.ts", "new content", true, false, 1000) - // Verify file was written - const fs = await import("fs/promises") - expect(fs.writeFile).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", "utf-8") + // Verify file was written via safeWriteText + const { safeWriteText } = await import("../../../services/file-safety/safeWriteText") + expect(safeWriteText).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", { + preCommitVerify: expect.any(Function), + }) // Verify delay was NOT called expect(mockDelay).not.toHaveBeenCalled() @@ -859,6 +908,94 @@ describe("DiffViewProvider", () => { expect((diffViewProvider as any).relPath).toBe("test.ts") expect((diffViewProvider as any).newContent).toBe("new content") }) + + describe("guarded write (S4b, epic #1375)", () => { + const enoent = () => Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + + it("rejects an unobserved write to an existing file with the read-first remediation", async () => { + mockTask.observationRegistry.clear() + + await expect(diffViewProvider.saveDirectly("test.ts", "new content", true, false, 0)).rejects.toThrow( + "File already exists at /mock/cwd/test.ts and was not read before this write -- read the file first, then retry.", + ) + expect(safeWriteText).not.toHaveBeenCalled() + }) + + it("creates an unobserved file when the target is absent", async () => { + mockTask.observationRegistry.clear() + vi.mocked(fs.access).mockRejectedValue(enoent()) + + await diffViewProvider.saveDirectly("test.ts", "new content", true, false, 0) + + expect(safeWriteText).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", { + preCommitVerify: expect.any(Function), + }) + }) + it("holds the lock across the guard check and the publish", async () => { + const order: string[] = [] + vi.mocked(acquireFileLock).mockImplementation(async () => { + order.push("acquire") + return async () => { + order.push("release") + } + }) + mockTask.observationRegistry.clear() + vi.mocked(fs.access).mockImplementation(async () => { + order.push("check") + throw Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + }) + vi.mocked(safeWriteText).mockImplementation(async () => { + order.push("publish") + }) + + await diffViewProvider.saveDirectly("test.ts", "new content", true, false, 0) + + // A writer that honours the same lock cannot slip between the check and the publish. + expect(order).toEqual(["acquire", "check", "publish", "release"]) + }) + + it("rejects an observed write whose version token is stale", async () => { + // The file changed on disk after the read that recorded "v1". + vi.mocked(computeVersionToken).mockResolvedValue("v2") + + const result = diffViewProvider.saveDirectly("test.ts", "new content", true, false, 0) + + await expect(result).rejects.toThrow("Stale version") + await expect(result).rejects.toThrow("re-read the file, then retry.") + expect(safeWriteText).not.toHaveBeenCalled() + }) + + it("rejects an unobserved edit-kind write before any I/O", async () => { + mockTask.observationRegistry.clear() + + await expect( + diffViewProvider.saveDirectly("test.ts", "new content", true, false, 0, "edit"), + ).rejects.toThrow("File not read yet -- read the file, then retry.") + expect(safeWriteText).not.toHaveBeenCalled() + expect(fs.access).not.toHaveBeenCalled() + }) + + it("recreates an observed file that vanished after the read", async () => { + vi.mocked(fs.access).mockRejectedValue(enoent()) + + await diffViewProvider.saveDirectly("test.ts", "new content", true, false, 0) + + expect(safeWriteText).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", { + preCommitVerify: expect.any(Function), + }) + }) + + it("fails closed when the owning task has been collected", async () => { + // A real WeakRef cannot be forced to deref to undefined deterministically + // (GC timing), so a structural stub stands in for the collected reference. + diffViewProvider["taskRef"] = { deref: () => undefined } as unknown as WeakRef + + await expect(diffViewProvider.saveDirectly("test.ts", "new content", true, false, 0)).rejects.toThrow( + "Cannot guard the write: the owning task is no longer available", + ) + expect(safeWriteText).not.toHaveBeenCalled() + }) + }) }) describe("saveChanges method with diagnostic settings", () => { 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..f9d5430960 --- /dev/null +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -0,0 +1,743 @@ +import * as fs from "fs/promises" +import * as fsSync from "fs" +import { execFile } from "child_process" +import type { ChildProcess } from "child_process" +import * as path from "path" + +import { safeWriteText, type SafeWriteTextOptions } from "../safeWriteText" + +// Full mock for fs/promises — all methods are vi.fn() stubs +vi.mock("fs/promises", () => ({ + mkdir: vi.fn(), + access: vi.fn(), + copyFile: vi.fn(), + chmod: vi.fn(), + rename: vi.fn(), + unlink: vi.fn(), + realpath: vi.fn(), + rmdir: vi.fn(), +})) + +// Full mock for fs — all sync methods are vi.fn() stubs. Stats is a bare +// class stub so tests can build minimal Stats stand-ins via its prototype. +vi.mock("fs", () => ({ + openSync: vi.fn(), + writeSync: vi.fn(), + closeSync: vi.fn(), + mkdirSync: vi.fn(), + fsyncSync: vi.fn(), + chmodSync: vi.fn(), + fchmodSync: vi.fn(), + statSync: vi.fn(), + Stats: class Stats {}, +})) + +// Mock child_process.execFile (callback-based — must invoke callback to resolve) +vi.mock("child_process", () => ({ + execFile: vi.fn((cmd, args, opts, cb) => { + if (typeof cb === "function") cb(null) + }), +})) + +// Minimal stand-in for the ChildProcess that callback-form execFile returns. +const fakeChild = { kill: () => true } as unknown as ChildProcess + +// Helper that mirrors safeWriteText's path resolution exactly +function _resolvedTarget(filePath: string): string { + return path.resolve(filePath) +} +function _dirPath(filePath: string): string { + return path.dirname(_resolvedTarget(filePath)) +} +function _stagingDir(dir: string): string { + return path.join(dir, ".file-safety-staging") +} + +// Minimal Stats stand-in: the SUT only reads `.mode` from it. +function _stats(mode: number): fsSync.Stats { + const s = Object.create(fsSync.Stats.prototype) as fsSync.Stats + Object.assign(s, { mode }) + return s +} + +// ── Test 1: staging file created then cleaned after success ──────────────── + +describe("safeWriteText", () => { + beforeEach(() => { + vi.resetAllMocks() + // After resetAllMocks, vi.fn() returns undefined — restore promise defaults. + vi.mocked(fs.mkdir).mockResolvedValue(undefined) + vi.mocked(fs.access).mockResolvedValue(undefined) + vi.mocked(fs.rename).mockResolvedValue(undefined) + vi.mocked(fs.unlink).mockResolvedValue(undefined) + // Existing-target default: a regular 0o644 file. + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o644)) + // Default sync-write behaviour: report that all requested bytes were + // written. The Buffer overload passes (fd, buffer, offset, length), + // so the fourth argument is the requested length. + vi.mocked(fsSync.writeSync).mockImplementation((...args: unknown[]) => + typeof args[3] === "number" ? args[3] : 0, + ) + }) + + describe("staging and cleanup", () => { + it("creates a temp file in the staging dir, fsyncs it, renames to target, and cleans up on success", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) // fd=1 + vi.mocked(fsSync.closeSync).mockReturnValue(undefined) + + await safeWriteText(targetPath, "hello world", { platform: "linux" }) + + // staging dir was created with private permissions — use + // stringContaining to handle Windows path resolution + expect(fsSync.mkdirSync).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), { + recursive: true, + mode: 0o700, + }) + // a pre-existing staging dir is repaired to private permissions too + expect(fsSync.chmodSync).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), 0o700) + + // temp file was opened for writing with the existing target's mode + // (default 0o644 from the statSync default mock) + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), "w", 0o644) + + // content was written as a buffer (partial-write loop, full write) + expect(fsSync.writeSync).toHaveBeenCalledWith(1, Buffer.from("hello world", "utf8"), 0, 11) + + // fsync (sync form) was called on the fd + expect(fsSync.fsyncSync).toHaveBeenCalledWith(1) + + // file was closed + expect(fsSync.closeSync).toHaveBeenCalledWith(1) + + // atomic rename happened — realpath mock returns targetPath, so that's the dest + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // no unlink of temp (it's now the committed file; DACL skipped via platform:linux) + expect(fs.unlink).not.toHaveBeenCalled() + }) + + it("removes the staging directory after the commit lands", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // The directory only exists to hold this write's temp file, so a successful publish + // must not leave it behind in the user's workspace. + expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + }) + + it("re-creates the staging directory when a concurrent write removes it mid-write", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync) + .mockImplementationOnce(() => { + // Another writer committed and rmdir'd the shared staging directory + // between _stagingDir() and this open. + throw Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + }) + .mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // Once for the original staging call, once for the recovery. + expect(fsSync.mkdirSync).toHaveBeenCalledTimes(2) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("gives up after one recovery when the staging open keeps failing with 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.openSync).mockImplementation(() => { + throw enoent + }) + + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toBe(enoent) + + // One staging create plus one recovery attempt, then the error surfaces + // instead of looping. + expect(fsSync.mkdirSync).toHaveBeenCalledTimes(2) + expect(fsSync.openSync).toHaveBeenCalledTimes(2) + expect(fs.rename).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("removes the empty 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(fs.rename).mockRejectedValue(new Error("ENOSPC")) + + await expect(safeWriteText(targetPath, "new data", { platform: "linux" })).rejects.toThrow("ENOSPC") + + // The temp file is gone, so the staging directory it was created for is empty. + // Leaving it behind puts hidden clutter in the user's workspace until some later + // successful write happens to remove it. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + }) + + it("a preCommitVerify rejection skips the commit rename 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) + const verify = vi.fn(async () => { + throw new Error("Stale version at commit time") + }) + + await expect( + safeWriteText(targetPath, "new data", { platform: "linux", preCommitVerify: verify }), + ).rejects.toThrow("Stale version at commit time") + + // The verifier runs after staging and fsync but before the rename, so the target + // was never moved and the staged content was cleaned up. + expect(verify).toHaveBeenCalledWith(targetPath) + expect(fs.rename).not.toHaveBeenCalled() + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + }) + + 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, and it is the only rename: + // the backup is a copy, so the target was never moved out of the way. + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + expect(fs.rename).toHaveBeenCalledTimes(1) + + // the failing cleanup was the post-commit backup unlink + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + + // no rollback rename: the backup is a copy, so nothing is renamed back over + // the committed target. + expect(fs.rename).toHaveBeenCalledTimes(1) + + // 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, copy-based ── + + describe("backup:true", () => { + it("copies target -> backup before commit without moving the target, and deletes the copy 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) + + // The backup is a copy: the target never leaves its path, so a reader sees the + // pre-write content for the whole attempt and there is nothing to roll back. + expect(fs.copyFile).toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) + + // The only rename is the commit: temp -> target. + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // backup copy was deleted on success + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + }) + + it("creates the backup privately before its content exists, then fsyncs it", 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 }) + + // The destination must exist with a private mode before copyFile writes anything + // into it: copyFile chooses the destination mode itself, so a restrictive target + // could otherwise leave a group/world-readable copy that a later chmod cannot + // undo. "wx" also means a pre-existing path is never silently reused. + const seedOpen = vi.mocked(fsSync.openSync).mock.calls.find(function (call) { + return String(call[0]).includes("safeWriteText.bak_") && call[1] === "wx" + }) + expect(seedOpen).toBeDefined() + expect(seedOpen?.[2]).toBe(0o600) + const seedOrder = vi.mocked(fsSync.openSync).mock.invocationCallOrder[ + vi.mocked(fsSync.openSync).mock.calls.indexOf(seedOpen!) + ] + expect(seedOrder).toBeLessThan(vi.mocked(fs.copyFile).mock.invocationCallOrder[0]) + + // The chmod keeps a copied read-only attribute (Windows) from breaking the fsync + // open, and keeps a backup of a permissive file private. + expect(fs.copyFile).toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) + expect(fs.chmod).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_"), 0o600) + expect(vi.mocked(fs.chmod).mock.invocationCallOrder[0]).toBeGreaterThan( + vi.mocked(fs.copyFile).mock.invocationCallOrder[0], + ) + + // The copy is then opened for fsync with the writable flag. + const backupOpen = vi.mocked(fsSync.openSync).mock.calls.find(function (call) { + return String(call[0]).includes("safeWriteText.bak_") && call[1] === "r+" + }) + expect(backupOpen).toBeDefined() + // The writable open must come after the chmod: an open taken before it would + // fsync the copy while copyFile's chosen mode is still on the file. + const backupOpenOrder = vi.mocked(fsSync.openSync).mock.invocationCallOrder[ + vi.mocked(fsSync.openSync).mock.calls.indexOf(backupOpen!) + ] + expect(backupOpenOrder).toBeGreaterThan(vi.mocked(fs.chmod).mock.invocationCallOrder[0]) + }) + + it("a failed commit leaves the pre-write content at the target and drops the backup copy", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The commit rename is the only rename in this flow, and it fails. + vi.mocked(fs.rename).mockRejectedValue(new Error("ENOSPC")) + + await expect(safeWriteText(targetPath, "new data", { backup: true })).rejects.toThrow("ENOSPC") + + // The backup was a copy of the target, taken before the commit. + expect(fs.copyFile).toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) + // No restore: the target was never moved, so no rename can put the copy back. + expect(fs.rename).toHaveBeenCalledTimes(1) + // The copy is dropped and the staging temp is released. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("backup:true when target does not exist: no backup created, just commit", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // fs.access resolves for dirPath check, but rejects for target check (backup path) + vi.mocked(fs.access).mockImplementation(async (p) => { + if (typeof p === "string" && p.endsWith("target.txt")) throw { code: "ENOENT" } + }) + + await safeWriteText(targetPath, "new data", { backup: true, platform: "linux" }) + + // no backup rename (target didn't exist) + expect(fs.access).toHaveBeenCalledWith(targetPath) + + // only one rename: temp -> target + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // no unlink (no backup to delete; DACL skipped via platform:linux) + expect(fs.unlink).not.toHaveBeenCalled() + }) + }) + + // ── Test 5: win32 DACL path ────────────────────────────────────────────── + + describe("win32 DACL", () => { + it.skipIf(process.platform !== "win32")( + "copies target DACL onto staging file via icacls before rename on Windows", + async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // icacls dump + restore were called (execFile is callback-based mock) + expect(execFile).toHaveBeenCalledTimes(2) + }, + ) + + it("non-win32: DACL path is unreachable when platform is not win32", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // icacls was NOT called on non-win32 + expect(execFile).not.toHaveBeenCalled() + }) + + it("win32 DACL failure falls back to plain rename (never fails the write)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // icacls dump fails — the callback-based mock must invoke cb with an error. + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + if (typeof cb === "function") cb(new Error("icacls error"), "", "") + return fakeChild + }) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // write succeeded despite icacls failure (fallback to plain rename) + expect(fs.rename).toHaveBeenCalled() + }) + + it("win32 DACL save args are [targetPath, /save, dumpPath, /T] before backup rename", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { backup: true, platform: "win32" }) + + // icacls was called twice (save + restore) + expect(execFile).toHaveBeenCalledTimes(2) + + // First call: save DACL from target before backup rename + const firstCall = vi.mocked(execFile).mock.calls[0] + expect(firstCall[0]).toBe("icacls") + expect(firstCall[1]).toEqual([targetPath, "/save", expect.stringContaining(".acl.tmp"), "/T"]) + + // Second call: restore DACL onto directory after commit rename + const secondCall = vi.mocked(execFile).mock.calls[1] + expect(secondCall[0]).toBe("icacls") + expect(secondCall[1]).toEqual([ + expect.stringContaining("/tmp/test-dir"), + "/restore", + expect.stringContaining(".acl.tmp"), + ]) + + // dump file was unlinked after restore + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".acl.tmp")) + }) + + it("win32 DACL: dump is unlinked even when restore fails", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + // icacls save succeeds, restore fails + let callCount = 0 + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + callCount++ + if (typeof cb === "function") { + cb(callCount === 1 ? null : new Error("icacls restore error"), "", "") + } + return fakeChild + }) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // write succeeded despite restore failure (best-effort) + expect(fs.rename).toHaveBeenCalled() + + // dump file was still unlinked in finally + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".acl.tmp")) + }) + + it("win32 DACL: when target does not exist, no save/restore/dump", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + // fs.access rejects for targetPath (ENOENT), but resolves for dirPath + vi.mocked(fs.access).mockImplementation(async (p) => { + if (typeof p === "string" && p.endsWith("target.txt")) throw { code: "ENOENT" } + return undefined + }) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // icacls was NOT called (target absent → skip DACL entirely) + expect(execFile).not.toHaveBeenCalled() + + // no dump file created or unlinked + expect(fs.unlink).not.toHaveBeenCalled() + }) + }) + + // ── Test 6: pre-written temp path (tempPath option) ────────────────────── + + describe("pre-written temp path", () => { + it("uses the provided tempPath, fsyncs it, and renames to target", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + const customTempPath = "/tmp/custom-temp.tmp" + + // platform:linux skips DACL entirely so this test focuses on tempPath only + await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) + + // openSync was called on the custom temp path (r+ mode for fsync) + expect(fsSync.openSync).toHaveBeenCalledWith(customTempPath, "r+") + + // fsync was called + expect(fsSync.fsyncSync).toHaveBeenCalledWith(1) + + // rename happened — realpath mock returns targetPath + expect(fs.rename).toHaveBeenCalledWith(customTempPath, targetPath) + + // no unlink of custom temp (caller's concern; DACL skipped via platform:linux) + expect(fs.unlink).not.toHaveBeenCalled() + + // a caller-supplied tempPath must not create the staging directory + expect(fsSync.mkdirSync).not.toHaveBeenCalled() + }) + + it("applies the existing target's mode to a caller-supplied tempPath before publishing", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o600)) + vi.mocked(fsSync.openSync).mockReturnValue(2) + + const customTempPath = "/tmp/custom-temp.tmp" + + await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) + + // the caller-staged temp is fchmod'd to the restrictive target mode so + // the atomic rename cannot widen a 0o600 target (CWE-732 regression) + expect(fsSync.fchmodSync).toHaveBeenCalledWith(2, 0o600) + expect(fsSync.openSync).toHaveBeenCalledWith(customTempPath, "r+") + expect(fs.rename).toHaveBeenCalledWith(customTempPath, targetPath) + }) + + it("keeps the temp's default mode when the target does not exist yet (ENOENT)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + vi.mocked(fsSync.statSync).mockImplementation(() => { + throw enoent + }) + vi.mocked(fsSync.openSync).mockReturnValue(2) + + const customTempPath = "/tmp/custom-temp.tmp" + + await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) + + // no existing target, so nothing to preserve and no fchmod on the temp + expect(fsSync.fchmodSync).not.toHaveBeenCalled() + expect(fs.rename).toHaveBeenCalledWith(customTempPath, targetPath) + }) + + it("opens the temp before applying a read-only target's mode (0o444 does not block the open)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o444)) + vi.mocked(fsSync.openSync).mockReturnValue(3) + + const customTempPath = "/tmp/custom-temp.tmp" + + await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) + + // a 0o444 target must not make openSync(tempPath, "r+") fail: the mode + // is applied with fchmodSync on the already-open fd, after the open + expect(fsSync.openSync).toHaveBeenCalledWith(customTempPath, "r+") + expect(fsSync.fchmodSync).toHaveBeenCalledWith(3, 0o444) + const openIdx = vi.mocked(fsSync.openSync).mock.invocationCallOrder[0] + const fchmodIdx = vi.mocked(fsSync.fchmodSync).mock.invocationCallOrder[0] + expect(openIdx).toBeLessThan(fchmodIdx) + expect(fs.rename).toHaveBeenCalledWith(customTempPath, targetPath) + }) + }) + + // ── Test 7: symlink handling (Finding 4 regression test) ───────────────── + + describe("symlink handling", () => { + it("a write through a symlink commits onto the resolved referent, never the link path", async () => { + const linkPath = "/tmp/links/link.txt" + const referentPath = "/tmp/targets/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(referentPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(linkPath, "new-content", { platform: "linux" }) + + // The commit rename must target the realpath result (the referent), never the link itself — + // that is what guarantees a write through a symlink replaces the referent's content + // and preserves the link. + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), referentPath) + expect(fs.rename).not.toHaveBeenCalledWith(expect.anything(), linkPath) + }) + + it("when realpath reports ENOENT (target absent), uses the given path as-is", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // rename still happened with the fallback path (path.resolve on /tmp → C:\tmp) + const resolvedFallback = _resolvedTarget(targetPath) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), resolvedFallback) + }) + }) + + // ── Test 8: review fixes (permissions, partial writes, resolution, durability) ── + + describe("review fixes", () => { + it("preserves the target's restrictive mode and tolerates a failed staging-dir permission repair", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o600)) + // a pre-existing staging dir may fail its best-effort permission repair + vi.mocked(fsSync.chmodSync).mockImplementationOnce(() => { + throw new Error("EACCES") + }) + + await safeWriteText(targetPath, "secret", { platform: "linux" }) + + // the staging file inherits the target's 0o600 mode and the write commits + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), "w", 0o600) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("falls back to the 0o644 default when the target does not exist yet", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.statSync).mockImplementation(() => { + throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + }) + + await safeWriteText(targetPath, "fresh", { platform: "linux" }) + + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), "w", 0o644) + }) + + it("loops on short writes until the full content is durable before fsync", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const content = "0123456789" // 10 bytes + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const buffer = Buffer.from(content, "utf8") + // first write (offset 0) reports 4 bytes (short write); the loop continues + vi.mocked(fsSync.writeSync).mockImplementation((...args: unknown[]) => + args[2] === 0 ? 4 : typeof args[3] === "number" ? args[3] : 0, + ) + + await safeWriteText(targetPath, content, { platform: "linux" }) + + // [0,10) reports 4 bytes, then [4,10) writes the remaining 6 + expect(fsSync.writeSync).toHaveBeenCalledTimes(2) + expect(fsSync.writeSync).toHaveBeenNthCalledWith(1, 1, buffer, 0, 10) + expect(fsSync.writeSync).toHaveBeenNthCalledWith(2, 1, buffer, 4, 6) + expect(fsSync.fsyncSync).toHaveBeenCalledWith(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("fsyncs the parent directory after the commit rename on POSIX", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // temp fd=1 then parent-dir fd=2 - distinct fds prove the ordering + vi.mocked(fsSync.openSync).mockReturnValueOnce(1).mockReturnValue(2) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // the directory fsync (fd 2) happens only after the file fsync (fd 1); + // the dir path assertion is path-agnostic (stringContaining) because + // path.dirname renders the same input differently on Windows + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("test-dir"), "r") + expect(fsSync.fsyncSync).toHaveBeenNthCalledWith(1, 1) + expect(fsSync.fsyncSync).toHaveBeenNthCalledWith(2, 2) + expect(fsSync.closeSync).toHaveBeenCalledWith(2) + }) + + it("treats a failed parent-directory fsync as best-effort", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync) + .mockReturnValueOnce(1) + .mockImplementationOnce(() => { + throw new Error("EBADF") + }) + + // the content rename already committed; a missing directory fsync is not fatal + await safeWriteText(targetPath, "data", { platform: "linux" }) + + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("propagates realpath errors (EACCES and code-less) instead of the fallback path", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const eacces = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + vi.mocked(fs.realpath).mockRejectedValueOnce(eacces) + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toBe(eacces) + expect(fs.rename).not.toHaveBeenCalled() + + const plain = new Error("resolution failed") + vi.mocked(fs.realpath).mockRejectedValueOnce(plain) + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toBe(plain) + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("backup:true propagates access errors (EACCES and code-less) instead of skipping the backup", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const eacces = Object.assign(new Error("EACCES"), { code: "EACCES" }) + const plain = new Error("access failed") + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // each write accesses dirPath then target; only the target access rejects + const rejectTarget = (error: Error) => async (p: unknown) => { + if (typeof p === "string" && p.endsWith("target.txt")) throw error + } + vi.mocked(fs.access) + .mockImplementationOnce(rejectTarget(eacces)) + .mockImplementationOnce(rejectTarget(eacces)) + .mockImplementationOnce(rejectTarget(plain)) + .mockImplementationOnce(rejectTarget(plain)) + + await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux" })).rejects.toEqual( + expect.objectContaining({ code: "EACCES" }), + ) + await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux" })).rejects.toThrow( + "access failed", + ) + expect(fs.rename).not.toHaveBeenCalled() + }) + }) +}) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts new file mode 100644 index 0000000000..ac93a789bf --- /dev/null +++ b/src/services/file-safety/safeWriteText.ts @@ -0,0 +1,406 @@ +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: copy the target to a backup and + * fsync the copy before the commit, delete the backup after a successful commit, + * and drop the backup on failure. The target is never moved, so a failed write + * leaves the pre-write content at the target path instead of rolling anything + * back. 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 + + /** + * Verification that runs AFTER the staged copy is written and fsynced and IMMEDIATELY + * BEFORE the commit rename. A guard that has to compare on-disk state against an + * expectation (a version token, or absence) cannot do that before the staging work: + * the window between check and publish would then span the whole staging + fsync + * sequence. Running the check here shrinks it to the rename syscall itself. A + * rejection skips the commit rename, so the target is left exactly as it was and the + * staged temp is cleaned up by the failure path. + * + * This is not an atomic compare-and-swap. No portable rename primitive compares the + * on-disk CONTENT against an expectation, so a writer that neither takes the advisory + * lock nor goes through this path can still change the file inside that last window. + * The guard narrows the window: a change detected during verification rejects the + * write instead of being silently lost, but a change that lands after verification + * returns and before the rename can still be overwritten. Full atomicity would need + * a content-addressed publish (or a lock every writer in the ecosystem honors), + * which this layer cannot enforce from outside. + */ + preCommitVerify?: (targetPath: string) => 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 +} + +/** + * 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 the backup copy. + * 4. Optionally copy target -> backup (when backup:true) and fsync the copy. + * 5. Atomic rename temp -> target. + * 6. win32 only: restore DACL onto the directory AFTER commit rename. + * 7. On success: delete backup (if any) and unlink DACL dump. + * 8. On failure: drop the backup copy; clean up temp + dump. + */ + +/** + * Resolve the publish target: the symlink referent when the given path is an + * existing symlink, the path itself otherwise. Only ENOENT (target absent yet) + * may fall back to the given path; any other resolution error (EACCES, EIO, ...) + * propagates so a broken or unreadable symlink is never written through its + * link path. Callers that stage a temp file themselves must stage it beside + * the resolved path: the commit is a rename onto the referent, and a rename + * across filesystems fails with EXDEV. + */ +export async function resolvePublishTarget(absoluteFilePath: string): Promise { + return fs.realpath(absoluteFilePath).catch((error: unknown) => { + const code = + typeof error === "object" && error !== null && "code" in error + ? (error as { code?: string }).code + : undefined + if (code !== "ENOENT") throw error + return absoluteFilePath + }) +} + +export async function safeWriteText(filePath: string, content: string, options?: SafeWriteTextOptions): Promise { + const absoluteFilePath = path.resolve(filePath) + + // Resolve the symlink referent (see resolvePublishTarget). + const targetPath = await resolvePublishTarget(absoluteFilePath) + const dirPath = path.dirname(targetPath) + + // Ensure parent directory exists (mirrors safeWriteJson behaviour). + await fs.mkdir(dirPath, { recursive: true }) + await fs.access(dirPath) + + // Create the staging directory only when we generate the temp file there; + // callers supplying their own tempPath (e.g. safeWriteJson) must not be left + // with an empty .file-safety-staging directory behind. + const stagingDir = options?.tempPath ? null : _stagingDir(dirPath) + const tempPath = options?.tempPath ?? _tempName(stagingDir ?? dirPath, "safeWriteText") + + let backupPath: string | null = null + let releaseBackupOnSuccess = false + let daclDumpPath: string | null = null // tracked for cleanup in finally + + try { + // -- Step 1: write content to staging temp file ------------------- + if (!options?.tempPath) { + // Preserve the existing target's permissions: the staging file must + // not be published wider than the file it replaces (a 0o600 target + // must not become 0o644 through the atomic rename). + let targetMode = 0o644 // default for a fresh target + try { + targetMode = fsSync.statSync(targetPath).mode & 0o777 + } catch { + // target does not exist yet - keep the default + } + let fd: number + try { + fd = fsSync.openSync(tempPath, "w", targetMode) + } catch (error: unknown) { + // Every write to one directory shares the staging dir, and a write that finishes + // removes it when empty. A concurrent writer can therefore remove it between + // _stagingDir() and this open. Re-create it and try once; any other errno, or + // a second ENOENT, is a real failure. + const code = + typeof error === "object" && error !== null && "code" in error + ? (error as { code?: string }).code + : undefined + if (code !== "ENOENT" || stagingDir === null) throw error + _stagingDir(dirPath) + fd = fsSync.openSync(tempPath, "w", targetMode) + } + try { + // Loop until every byte is written: writeSync can report a short + // (partial) write, and publishing a truncated staging file would + // commit corrupt content. + const buffer = Buffer.from(content, "utf8") + let offset = 0 + while (offset < buffer.length) { + offset += fsSync.writeSync(fd, buffer, offset, buffer.length - offset) + } + _fsyncFile(fd) + } finally { + fsSync.closeSync(fd) + } + } else { + // Preserve the existing target's mode (CWE-732): the caller-staged + // temp carries its own creation mode, and publishing it as-is would + // widen a restrictive target (e.g. 0o600 -> 0o644) through rename. + // The mode is applied with fchmodSync on the open fd (AFTER openSync): + // chmodSync on the path before the open would make a read-only target + // (0o400/0o444) fail openSync(tempPath, "r+") with EACCES. + let targetMode: number | null = null + try { + targetMode = fsSync.statSync(targetPath).mode & 0o777 + } catch { + // target does not exist yet - keep the temp's default mode + } + const fd = fsSync.openSync(tempPath, "r+") + try { + if (targetMode !== null) { + fsSync.fchmodSync(fd, targetMode) + } + _fsyncFile(fd) + } finally { + fsSync.closeSync(fd) + } + } + + // -- Step 2 (win32): save DACL BEFORE backup rename --------------- + const platform = options?.platform ?? process.platform + if (platform === "win32") { + try { + await fs.access(targetPath) // target exists? + daclDumpPath = targetPath + ".acl.tmp" + const saved = await _saveDaclWindows(targetPath, daclDumpPath, options?.execFileRunner) + if (!saved) { + daclDumpPath = null // skip DACL handling entirely + } + } catch { + // target does not exist or access failed — no DACL handling + daclDumpPath = null + } + } + + try { + // -- Step 3 (backup:true): durable copy target -> backup ---- + if (options?.backup) { + try { + await fs.access(targetPath) + backupPath = _tempName(dirPath, "safeWriteText.bak") + try { + // Create the destination BEFORE any content exists at it, with the mode fixed + // at open time. fs.copyFile picks the destination mode itself (the platform + // creation mask subject to umask on some platforms, the source's mode - or its + // read-only attribute - on others), so letting it create the file would either + // leave a restrictive target's bytes briefly readable to others, or leave the + // copy unwritable so the fsync open below fails with EACCES. open() ignores its + // mode argument for an existing file, so this 0o600 survives the copy on POSIX; + // the chmod afterwards is what clears a copied read-only attribute on Windows + // and keeps a backup of a permissive file private. + const seedFd = fsSync.openSync(backupPath, "wx", 0o600) + fsSync.closeSync(seedFd) + await fs.copyFile(targetPath, backupPath) + await fs.chmod(backupPath, 0o600) + // "r+" not "r": fsync on a read-only handle is EPERM on Windows, and this is + // the same flag the staged temp file is opened with above. + const backupFd = fsSync.openSync(backupPath, "r+") + try { + _fsyncFile(backupFd) + } finally { + fsSync.closeSync(backupFd) + } + } catch (backupError: unknown) { + // A half-written backup is worse than none: it looks like a restore source. + await fs.unlink(backupPath).catch(() => {}) + backupPath = null + throw backupError + } + 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 3a: pre-commit verification ------------------------- + // Runs before the commit rename so a rejection leaves the target untouched - + // no backup is taken and nothing is moved yet. + if (options?.preCommitVerify) { + await options.preCommitVerify(targetPath) + } + + // -- Step 4: atomic rename temp -> target --------------------- + await fs.rename(tempPath, targetPath) + + // -- Step 4b (POSIX): fsync the parent directory so the directory entry + // changed by the commit rename is durable, not just the file content. + if (platform !== "win32") { + try { + const dirFd = fsSync.openSync(dirPath, "r") + try { + _fsyncFile(dirFd) + } finally { + fsSync.closeSync(dirFd) + } + } catch { + // best-effort: the content rename already committed + } + } + + // -- Step 5 (win32): restore DACL AFTER commit rename --------- + if (platform === "win32" && daclDumpPath !== null) { + const restoredDir = path.dirname(targetPath) + await _restoreDaclWindows(restoredDir, daclDumpPath, options?.execFileRunner) + } + + // -- Step 6 (backup:true): delete backup on success ----------- + if (releaseBackupOnSuccess && backupPath) { + try { + await fs.unlink(backupPath) + } catch { + // non-fatal — orphaned backup is acceptable + } + } + + // The staging sub-directory only exists to hold this write's temp file. Once the + // commit lands it is empty clutter in the user's workspace, so remove it when nothing + // else is in it; a concurrent write still holding a temp file makes rmdir fail and is + // left alone. + if (stagingDir !== null) { + try { + await fs.rmdir(stagingDir) + } catch { + // best-effort: a concurrent write may still hold a temp file here + } + } + } finally { + // Unlink DACL dump regardless of success/failure in this span. + if (daclDumpPath !== null) { + await fs.unlink(daclDumpPath).catch(() => {}) + } + } + + // tempPath is now the committed file; no cleanup needed. + } catch (originalError: unknown) { + // -- Rollback / cleanup on failure ---------------------------------- + if (backupPath && releaseBackupOnSuccess) { + // Nothing to restore: the backup is a copy, so the target still holds whatever + // the commit left there - before the commit that is the pre-write content, and + // after it the published content. Either way the copy has served its purpose + // and must not be left beside the target where no caller can find it. The old + // design renamed the target away and back, which could itself fail and leave + // persisted state missing at its expected path. + await fs.unlink(backupPath).catch(() => {}) + backupPath = null + } + + // Always clean up the staging temp file on failure. + try { + await fs.unlink(tempPath).catch(() => {}) + } catch { + // cleanup failure is non-fatal + } + + // The staging directory was created for this write's temp file, so a failed write + // must not leave an empty .file-safety-staging in the user's workspace. rmdir only + // removes an empty directory, so a concurrent write still holding a temp file here + // keeps it in place. + if (stagingDir !== null) { + try { + await fs.rmdir(stagingDir) + } catch { + // best-effort: non-empty (concurrent write) or already removed + } + } + + if (daclDumpPath !== null) { + await fs.unlink(daclDumpPath).catch(() => {}) + } + + throw originalError + } +} diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 79d08678a0..7bab4b842f 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -4,6 +4,7 @@ import * as path from "path" import * as os from "os" import { safeWriteJson } from "../safeWriteJson" +import * as lockfile from "proper-lockfile" // Capture actual implementations before the vi.mock factory runs, // so they are never wrapped by vi.fn() — avoids infinite recursion when @@ -184,29 +185,15 @@ describe("safeWriteJson", () => { // Overwrite the pre-created file with specific initial data await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) - // Track rename calls - let renameCallCount = 0 - - // 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) { - // First call: filePath -> tempBackupFilePath (should succeed) - return fsPromisesActuals.rename!(oldPath, newPath) - } else if (renameCallCount === 2) { - // Second call: tempNewFilePath -> filePath (should fail) - throw new Error("Rename from temp to final failed") - } else if (renameCallCount === 3) { - // Third call: tempBackupFilePath -> filePath (rollback, should succeed) - return fsPromisesActuals.rename!(oldPath, newPath) - } - // Default: use original implementation - return fsPromisesActuals.rename!(oldPath, newPath) + // The commit rename is the only rename in this flow, and it fails. + vi.mocked(fs.rename).mockImplementationOnce(async () => { + throw new Error("Rename from temp to final failed") }) await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow("Rename from temp to final failed") - // Verify the file was restored to initial content + // The backup was a copy, so the target never left its path: the pre-write content + // is still what a reader sees, and the copy is dropped rather than renamed back. const content = await readFileContent(currentTestFilePath) expect(content).toEqual(initialData) }) @@ -312,9 +299,8 @@ describe("safeWriteJson", () => { expect(content).toEqual(newData) }) - // Test for console error suppression during backup deletion - test("should suppress console.error when backup deletion fails", async () => { - const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) // Suppress console.error + // Test for best-effort backup deletion (the backup lifecycle now lives in safeWriteText) + test("does not fail the write when backup deletion fails (orphaned backup is acceptable)", async () => { const initialData = { message: "Initial" } const newData = { message: "New" } @@ -322,18 +308,23 @@ describe("safeWriteJson", () => { // fs.unlink is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn vi.mocked(fs.unlink).mockImplementation(async (filePath: any) => { - if (filePath.toString().includes(".bak_")) { + if (filePath.toString().includes("safeWriteText.bak_")) { throw new Error("Backup deletion failed") } return fsPromisesActuals.unlink!(filePath) }) + // The write must still succeed: backup cleanup is best-effort inside + // safeWriteText and never masks the committed content. await safeWriteJson(currentTestFilePath, newData) - // Verify console.error was called with the expected message - expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining("Successfully wrote"), expect.any(Error)) + const content = await readFileContent(currentTestFilePath) + expect(content).toEqual(newData) + + // The orphaned backup is still on disk because its deletion failed. + const entries = await fs.readdir(tempDir) + expect(entries.some((entry) => entry.includes("safeWriteText.bak_"))).toBe(true) - consoleErrorSpy.mockRestore() vi.mocked(fs.unlink).mockRestore() }) @@ -345,21 +336,14 @@ 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 - let renameCallCount = 0 - vi.mocked(fs.rename).mockImplementation(async (oldPath, newPath) => { - renameCallCount++ - if (renameCallCount === 2) { - // Second call: tempNewFilePath -> filePath (should fail) - throw new Error("Rename failed") - } - // For all other calls, use the original implementation - return fsPromisesActuals.rename!(oldPath, newPath) + // The commit rename is the only rename in this flow, and it fails. + vi.mocked(fs.rename).mockImplementationOnce(async () => { + throw new Error("Rename failed") }) await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow("Rename failed") - // The file should be restored to its initial content + // The target was never moved, so its previous content is still there. const content = await readFileContent(currentTestFilePath) expect(content).toEqual(initialData) }) @@ -434,37 +418,29 @@ describe("safeWriteJson", () => { expect(vi.mocked(fs.access)).toHaveBeenCalled() }) - // Test for rollback failure scenario - test("should log error and re-throw original if rollback fails", async () => { - const initialData = { message: "Initial, should be lost if rollback fails" } + // A failed commit with a copy-based backup: the target keeps its previous content + // and the copy is dropped, so nothing is orphaned beside it. + test("a failed commit keeps the previous content at the target and removes the backup copy", async () => { + const initialData = { message: "Initial" } const newData = { message: "New content" } await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) 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 - let renameCallCount = 0 - vi.mocked(fs.rename).mockImplementation(async (oldPath, newPath) => { - renameCallCount++ - if (renameCallCount === 2) { - // Second call: tempNewFilePath -> filePath (fail) - throw new Error("Primary rename failed") - } else if (renameCallCount === 3) { - // Third call: tempBackupFilePath -> filePath (rollback, also fail) - throw new Error("Rollback rename failed") - } - return fsPromisesActuals.rename!(oldPath, newPath) + // The commit rename is the only rename in this flow, and it fails. + vi.mocked(fs.rename).mockImplementationOnce(async () => { + throw new Error("Primary rename failed") }) - // Should throw the original error, not the rollback error await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow("Primary rename failed") - // Verify console.error was called for the rollback failure - expect(consoleErrorSpy).toHaveBeenCalledWith( - expect.stringContaining("Failed to restore backup"), - expect.objectContaining({ message: "Rollback rename failed" }), - ) + // The target was never moved, so the pre-write content is still what a reader + // sees, and the backup copy is dropped instead of being renamed back. + expect(await fileExists(currentTestFilePath)).toBe(true) + expect(await readFileContent(currentTestFilePath)).toEqual(initialData) + const entries = await fs.readdir(tempDir) + expect(entries.some((entry) => entry.includes("safeWriteText.bak_"))).toBe(false) consoleErrorSpy.mockRestore() }) @@ -542,4 +518,135 @@ describe("safeWriteJson", () => { const content = await readFileContent(currentTestFilePath) expect(content).toEqual({ c: 3 }) }) + + // The commit rename targets the symlink referent. The staged temp file must + // therefore be created beside the RESOLVED target — staging beside the link + // would make the commit rename fail with EXDEV when the referent is on + // another filesystem. (Real symlinks are unavailable in this CI lane, so the + // resolution is simulated by mocking fs.realpath the same way.) + test("stages the temp file beside the symlink referent and commits onto it", async () => { + const referentDir = path.join(tempDir, "referent") + const linkDir = path.join(tempDir, "link") + await fs.mkdir(referentDir, { recursive: true }) + await fs.mkdir(linkDir, { recursive: true }) + // caller-visible path (the link) vs the resolved referent path + const callerPath = path.join(linkDir, "test-file.json") + const referentPath = path.join(referentDir, "test-file.json") + // Seed the RESOLVED referent with real content (via the actual fs) so the + // write exercises replacement of an EXISTING referent: the lock, the + // backup, and the commit all target the resolved referent. + await fsPromisesActuals.writeFile!(referentPath, JSON.stringify({ seed: true })) + + vi.spyOn(fs, "realpath").mockResolvedValue(referentPath) + + await safeWriteJson(callerPath, { after: true }) + + // the temp file was created next to the resolved referent, NOT beside the link + const tempPaths = vi.mocked(fsSyncActual.createWriteStream).mock.calls.map((call) => String(call[0])) + expect(tempPaths.some((p) => p.startsWith(referentDir + path.sep) && p.includes(".new_"))).toBe(true) + expect(tempPaths.some((p) => p.startsWith(linkDir + path.sep))).toBe(false) + + // the content was committed onto the referent + expect(await readFileContent(referentPath)).toEqual({ after: true }) + }) + + // 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, + })) + + try { + // 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 read the referent's content through that single lock. + expect(mergeFn).toHaveBeenCalledWith({ seed: 1 }, { added: true }) + expect(await readFileContent(referentPath)).toEqual({ seed: 1, added: true }) + // 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), + ) + } finally { + // vi.unmock is hoisted, so it cannot undo the vi.doMock above at runtime; doUnmock + // does. resetModules keeps a later test from re-importing this module with the + // throwing release mock still attached - which is what would happen whenever an + // assertion above failed before the cleanup line. + vi.doUnmock("proper-lockfile") + vi.resetModules() + } + }) + + // CWE-732 regression: safeWriteJson stages the temp itself and passes it + // via tempPath, so safeWriteText must apply the existing target's mode to + // the staged temp before the atomic rename — otherwise a 0o600 target is + // published as 0o644. POSIX-only assertion (Windows ignores POSIX modes). + test.skipIf(process.platform === "win32")( + "preserves a restrictive 0o600 target mode through the atomic publish", + async () => { + await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify({ before: true })) + fsSyncActual.chmodSync(currentTestFilePath, 0o600) + + await safeWriteJson(currentTestFilePath, { after: true }) + + expect(fsSyncActual.statSync(currentTestFilePath).mode & 0o777).toBe(0o600) + expect(await readFileContent(currentTestFilePath)).toEqual({ after: true }) + }, + ) }) diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index 7da68b2a7a..fd348e52ee 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -3,6 +3,8 @@ import * as fsSync from "fs" import * as path from "path" import { JsonStreamStringify } from "json-stream-stringify" +import { resolvePublishTarget, safeWriteText, type SafeWriteTextOptions } from "../services/file-safety/safeWriteText" + import { acquireFileLock } from "./fileLock" /** @@ -32,7 +34,7 @@ export interface SafeWriteJsonOptions { * Safely writes JSON data to a file. * - Creates parent directories if they don't exist * - Uses 'proper-lockfile' for inter-process advisory locking to prevent concurrent writes to the same path. - * - Writes to a temporary file first. + * - Writes to a temporary file first via JsonStreamStringify streaming. * - If the target file exists, it's backed up before being replaced. * - Attempts to roll back and clean up in case of errors. * - Supports pretty-printing with indentation while maintaining streaming efficiency. @@ -42,7 +44,6 @@ export interface SafeWriteJsonOptions { * @param {SafeWriteJsonOptions} options - Optional configuration for JSON formatting. * @returns {Promise} */ - async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJsonOptions): Promise { const absoluteFilePath = path.resolve(filePath) let releaseLock = async () => {} // Initialized to a no-op @@ -52,28 +53,31 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso // Ensure directory structure exists with improved reliability try { - // Create directory with recursive option await fs.mkdir(dirPath, { recursive: true }) - - // Verify directory exists after creation attempt await fs.access(dirPath) } catch (dirError: any) { console.error(`Failed to create or access directory for ${absoluteFilePath}:`, dirError) throw dirError } + // Resolve the publish target BEFORE acquiring the lock: proper-lockfile keys + // the lock by the given path, so a symlink alias and its referent would + // otherwise take two distinct locks for one underlying file - a concurrent + // merge through both aliases could then read the same JSON and overwrite one + // update. Locking the resolved referent coordinates every alias through one + // lock. resolvePublishTarget tolerates a not-yet-existing file (it returns + // the given path on ENOENT), preserving the previous create-from-absent flow. + const resolvedTargetPath = await resolvePublishTarget(absoluteFilePath) + // Acquire the lock before any file operations. `acquireFileLock` owns the - // shared advisory lock protocol, so callers that lock the same path with - // it (for example task-history deletion) serialize with this write. - // If lock acquisition fails, it throws immediately. The releaseLock - // remains a no-op, so the finally block in the main file operations - // try-catch-finally won't try to release an unacquired lock if this - // path is taken. - releaseLock = await acquireFileLock(absoluteFilePath) - - // Variables to hold the actual paths of temp files if they are created. + // shared advisory lock protocol, so callers that lock the same path with it + // (for example task-history deletion) serialize with this write. It locks the + // resolved publish target, which is the key every other writer to this file + // uses. If acquisition fails it throws immediately, so the finally block never + // releases an unacquired lock. + releaseLock = await acquireFileLock(resolvedTargetPath) + // Variables to hold the actual path of the temp file if it is created. let actualTempNewFilePath: string | null = null - let actualTempBackupFilePath: string | null = null try { // If a merge callback was provided, read the current file under the lock @@ -82,7 +86,7 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso if (options?.merge) { let existing: unknown = null try { - existing = JSON.parse(await fs.readFile(absoluteFilePath, "utf8")) + existing = JSON.parse(await fs.readFile(resolvedTargetPath, "utf8")) } catch (error: unknown) { const code = error && typeof error === "object" && "code" in error ? (error as { code: string }).code : undefined @@ -93,79 +97,44 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso data = options.merge(existing, data) } - // Step 1: Write data to a new temporary file. + // Step 1: Write data to a new temporary file via JSON streaming. + // Stage it beside the *resolved* target (the symlink referent when the path is + // a symlink; resolvedTargetPath above): safeWriteText commits by renaming + // onto that referent, and a rename across filesystems would fail with EXDEV. actualTempNewFilePath = path.join( - path.dirname(absoluteFilePath), - `.${path.basename(absoluteFilePath)}.new_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`, + path.dirname(resolvedTargetPath), + ".new_" + Date.now() + "_" + Math.random().toString(36).substring(2) + ".tmp", ) await _streamDataToFile(actualTempNewFilePath, data, options?.prettyPrint) - // Step 2: Check if the target file exists. If so, rename it to a backup path. - try { - // Check for target file existence - await fs.access(absoluteFilePath) - // Target exists, create a backup path and rename. - actualTempBackupFilePath = path.join( - path.dirname(absoluteFilePath), - `.${path.basename(absoluteFilePath)}.bak_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`, - ) - await fs.rename(absoluteFilePath, actualTempBackupFilePath) - } catch (accessError: any) { - // Explicitly type accessError - if (accessError.code !== "ENOENT") { - // An error other than "file not found" occurred during access check. - throw accessError - } - // Target file does not exist, so no backup is made. actualTempBackupFilePath remains null. + // Step 2: Delegate backup + commit + rollback to safeWriteText with the + // pre-written temp path. backup:true keeps the old safeWriteJson semantics (a + // copy of the target is taken before the commit, and a failure removes that + // copy - the target itself is never moved) and keeps the target in place until + // safeWriteText captures its Windows DACL (safeWriteText dumps the DACL before + // making the backup copy and restores it onto the directory after the commit + // rename). + const textOptions: SafeWriteTextOptions = { + tempPath: actualTempNewFilePath, + backup: true, } - // Step 3: Rename the new temporary file to the target file path. - // This is the main "commit" step. - await fs.rename(actualTempNewFilePath, absoluteFilePath) + await safeWriteText(resolvedTargetPath, "", textOptions) - // If we reach here, the new file is successfully in place. - // The original actualTempNewFilePath is now the main file, so we shouldn't try to clean it up as "temp". - // Mark as "used" or "committed" + // If we reach here, the new file is successfully in place and any + // backup has already been handled by safeWriteText. actualTempNewFilePath = null - - // Step 4: If a backup was created, attempt to delete it. - if (actualTempBackupFilePath) { - try { - await fs.unlink(actualTempBackupFilePath) - // Mark backup as handled - actualTempBackupFilePath = null - } catch (unlinkBackupError) { - // Log this error, but do not re-throw. The main operation was successful. - // actualTempBackupFilePath remains set, indicating an orphaned backup. - console.error( - `Successfully wrote ${absoluteFilePath}, but failed to clean up backup ${actualTempBackupFilePath}:`, - unlinkBackupError, - ) - } - } } catch (originalError) { - console.error(`Operation failed for ${absoluteFilePath}: [Original Error Caught]`, originalError) + console.error(`Operation failed for ${resolvedTargetPath}: [Original Error Caught]`, originalError) const newFileToCleanupWithinCatch = actualTempNewFilePath - const backupFileToRollbackOrCleanupWithinCatch = actualTempBackupFilePath - // Attempt rollback if a backup was made - if (backupFileToRollbackOrCleanupWithinCatch) { - try { - await fs.rename(backupFileToRollbackOrCleanupWithinCatch, absoluteFilePath) - // Mark as handled, prevent later unlink of this path - actualTempBackupFilePath = null - } catch (rollbackError) { - // actualTempBackupFilePath (outer scope) remains pointing to backupFileToRollbackOrCleanupWithinCatch - console.error( - `[Catch] Failed to restore backup ${backupFileToRollbackOrCleanupWithinCatch} to ${absoluteFilePath}:`, - rollbackError, - ) - } - } - - // Cleanup the .new file if it exists + // A failed safeWriteText leaves the target exactly where it was: its backup is a + // copy, so undoing a failure removes that copy rather than moving anything back + // over the target path. Clean up the .new file if it still exists (safeWriteText + // also removes its own tempPath on failure; this is a safety net in case its + // cleanup missed it). if (newFileToCleanupWithinCatch) { try { await fs.unlink(newFileToCleanupWithinCatch) @@ -177,27 +146,13 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso } } - // Cleanup the .bak file if it still needs to be (i.e., wasn't successfully restored) - if (actualTempBackupFilePath) { - try { - await fs.unlink(actualTempBackupFilePath) - } catch (cleanupError) { - console.error( - `[Catch] Failed to clean up temporary backup file ${actualTempBackupFilePath}:`, - cleanupError, - ) - } - } throw originalError // This MUST be the error that rejects the promise. } finally { // Release the lock in the main finally block. try { - // releaseLock will be the actual unlock function if lock was acquired, - // or the initial no-op if acquisition failed. await releaseLock() } catch (unlockError) { - // Do not re-throw here, as the originalError from the try/catch (if any) is more important. - console.error(`Failed to release lock for ${absoluteFilePath}:`, unlockError) + console.error(`Failed to release lock for ${resolvedTargetPath}:`, unlockError) } } }