diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 4de2b84590..5fbac2dd3d 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -111,6 +111,7 @@ import { buildNativeToolsArrayWithRestrictions } from "./build-tools" import { ToolRepetitionDetector } from "../tools/ToolRepetitionDetector" import { restoreTodoListForTask } from "../tools/UpdateTodoListTool" import { FileContextTracker } from "../context-tracking/FileContextTracker" +import { ObservationRegistry } from "./observationRegistry" import { RooIgnoreController } from "../ignore/RooIgnoreController" import { RooProtectedController } from "../protect/RooProtectedController" import { type AssistantMessageContent, presentAssistantMessage } from "../assistant-message" @@ -286,6 +287,10 @@ export class Task extends EventEmitter implements TaskLike { readonly instanceId: string readonly metadata: TaskMetadata + // The observed on-disk version of each file this task has read. Declared here so the + // read tools can record it; a write guard later compares a token against this registry. + readonly observationRegistry = new ObservationRegistry() + todoList?: TodoItem[] readonly rootTask: Task | undefined = undefined diff --git a/src/core/task/__tests__/observationRegistry.spec.ts b/src/core/task/__tests__/observationRegistry.spec.ts new file mode 100644 index 0000000000..a3c55ebc6d --- /dev/null +++ b/src/core/task/__tests__/observationRegistry.spec.ts @@ -0,0 +1,108 @@ +import { describe, it, expect, vi } from "vitest" + +import { ObservationRegistry } from "../observationRegistry" + +describe("ObservationRegistry", () => { + it("observe → get returns the recorded version and observedAt", () => { + const reg = new ObservationRegistry() + reg.observe("/a/b/c.ts", "1:2:300:4000000000:5000000000") + + const obs = reg.get("/a/b/c.ts") + expect(obs).toBeDefined() + expect(obs!.version).toBe("1:2:300:4000000000:5000000000") + expect(typeof obs!.observedAt).toBe("number") + }) + + it("re-observe replaces the entry with a fresh observedAt", () => { + vi.useFakeTimers() + const reg = new ObservationRegistry() + reg.observe("/a/b/c.ts", "v1") + const first = reg.get("/a/b/c.ts")! + expect(first.version).toBe("v1") + + vi.advanceTimersByTime(50) + reg.observe("/a/b/c.ts", "v2") + const second = reg.get("/a/b/c.ts")! + expect(second.version).toBe("v2") + expect(second.observedAt).toBeGreaterThan(first.observedAt) + + vi.useRealTimers() + }) + + it("has returns true for observed paths, false otherwise", () => { + const reg = new ObservationRegistry() + reg.observe("/x.ts", "t1") + expect(reg.has("/x.ts")).toBe(true) + expect(reg.has("/y.ts")).toBe(false) + }) + + it("size reflects the number of observed entries", () => { + const reg = new ObservationRegistry() + expect(reg.size).toBe(0) + reg.observe("/a.ts", "t1") + reg.observe("/b.ts", "t2") + expect(reg.size).toBe(2) + }) + + it("clear removes all entries and resets size to 0", () => { + const reg = new ObservationRegistry() + reg.observe("/a.ts", "t1") + reg.observe("/b.ts", "t2") + reg.clear() + expect(reg.size).toBe(0) + expect(reg.get("/a.ts")).toBeUndefined() + expect(reg.has("/b.ts")).toBe(false) + }) + + it("get on empty registry returns undefined", () => { + const reg = new ObservationRegistry() + expect(reg.get("/any.ts")).toBeUndefined() + }) + + it("separate instances are independent — observing in one does not appear in the other", () => { + const regA = new ObservationRegistry() + const regB = new ObservationRegistry() + regA.observe("/shared.ts", "v1") + expect(regA.get("/shared.ts")).toBeDefined() + expect(regB.get("/shared.ts")).toBeUndefined() + regB.observe("/shared.ts", "v2") + expect(regA.get("/shared.ts")!.version).toBe("v1") + expect(regB.get("/shared.ts")!.version).toBe("v2") + }) + + describe("completeness scope (S4b follow-up #46)", () => { + it("defaults to a complete observation when the read scope is not given", () => { + const reg = new ObservationRegistry() + reg.observe("/a/b/c.ts", "v1") + + expect(reg.get("/a/b/c.ts")!.complete).toBe(true) + }) + + it("records a partial observation when the read only returned a view of the file", () => { + const reg = new ObservationRegistry() + reg.observe("/a/b/c.ts", "v1", false) + + expect(reg.get("/a/b/c.ts")!.complete).toBe(false) + }) + + it("re-observing replaces the entry's completeness with the new read's scope", () => { + const reg = new ObservationRegistry() + reg.observe("/a/b/c.ts", "v1", false) + reg.observe("/a/b/c.ts", "v2") + + const obs = reg.get("/a/b/c.ts")! + expect(obs.version).toBe("v2") + expect(obs.complete).toBe(true) + }) + + it("re-observing with a partial scope downgrades a previously complete entry", () => { + const reg = new ObservationRegistry() + reg.observe("/a/b/c.ts", "v1") + reg.observe("/a/b/c.ts", "v2", false) + + const obs = reg.get("/a/b/c.ts")! + expect(obs.version).toBe("v2") + expect(obs.complete).toBe(false) + }) + }) +}) diff --git a/src/core/task/observationRegistry.ts b/src/core/task/observationRegistry.ts new file mode 100644 index 0000000000..0ef9115f21 --- /dev/null +++ b/src/core/task/observationRegistry.ts @@ -0,0 +1,59 @@ +/** + * 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 S4 guarded-write consults + * these observations for the version check and for the completeness check that + * gates a full-file replacement. + */ + +export interface FileObservation { + /** Version token derived from on-disk fs.stat (bigint mode). */ + version: string + /** Millisecond timestamp when the observation was recorded. */ + observedAt: number + /** + * Whether the read that produced this observation returned the complete + * file. A slice, line-range, truncated, or indentation-block read returns + * only a view of the file; such an observation authorizes targeted edits + * on the view the model saw, but never a full-file replacement. + */ + complete: boolean +} + +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, the + * new version token, and the read's completeness. `complete` defaults to + * true for callers that read the whole file themselves (spec doubles, + * WriteToFileTool). A caller whose read is internal to a targeted edit must + * carry the model's prior completeness instead, so the tool's own read cannot + * upgrade a partial read into authority for a full-file replacement. + */ + observe(absolutePath: string, version: string, complete: boolean = true): void { + this.entries.set(absolutePath, { version, observedAt: Date.now(), complete }) + } + + 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/ReadFileTool.ts b/src/core/tools/ReadFileTool.ts index 2107cfe21b..7d1820d137 100644 --- a/src/core/tools/ReadFileTool.ts +++ b/src/core/tools/ReadFileTool.ts @@ -16,13 +16,14 @@ 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" import { getReadablePath } from "../../utils/path" import { extractTextFromFile, addLineNumbers, getSupportedBinaryFormats } from "../../integrations/misc/extract-text" import { readWithIndentation, readWithSlice } from "../../integrations/misc/indentation-reader" -import { DEFAULT_LINE_LIMIT } from "../prompts/tools/native-tools/read_file" +import { DEFAULT_LINE_LIMIT, MAX_LINE_LENGTH } from "../prompts/tools/native-tools/read_file" import type { ToolUse, PushToolResult } from "../../shared/tools" import { @@ -214,14 +215,36 @@ 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) + // A lossy decode is not the whole file: the model never saw those bytes. + const lossyDecode = !Buffer.from(fileContent).equals(buffer) + // S4b follow-up (#46 / epic #1375): processTextFile reports whether the + // returned content is the whole file; the observation below records that + // scope so the write guard can deny full-file updates built on a partial view. + const processed = 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, processed.complete && !lossyDecode) + } + } + updateFileResult(relPath, { - nativeContent: `File: ${relPath}\n${result}`, + nativeContent: `File: ${relPath}\n${processed.content}`, }) } catch (error) { const errorMsg = error instanceof Error ? error.message : String(error) @@ -265,8 +288,14 @@ export class ReadFileTool extends BaseTool<"read_file"> { /** * Process a text file according to the requested mode. + * + * Returns the content string plus whether that content is the complete + * file (S4b follow-up #46 / epic #1375): slice mode is complete only + * when it starts at line 1, returns every line, and was not truncated; + * indentation mode is never complete because it returns semantic blocks + * of the file, not the file itself. */ - private processTextFile(content: string, entry: InternalFileEntry): string { + private processTextFile(content: string, entry: InternalFileEntry): { content: string; complete: boolean } { const mode = entry.mode || "slice" if (mode === "indentation") { @@ -299,7 +328,8 @@ export class ReadFileTool extends BaseTool<"read_file"> { output += `\n\nIncluded ranges: ${rangeStr} (total: ${result.totalLines} lines)` } - return output + // Indentation mode returns semantic blocks: never a complete file view. + return { content: output, complete: false } } // Slice mode (default): simple offset/limit reading @@ -322,11 +352,27 @@ export class ReadFileTool extends BaseTool<"read_file"> { To read more: Use the read_file tool with offset=${nextOffset} and limit=${limit}. ${result.content}` + if (result.hasClippedLines) { + // The slice cut lines off and also clipped long lines inside it, so both + // notices belong to the response. + output += `\nNote: Some lines in this view exceed ${MAX_LINE_LENGTH} characters and were clipped in this view.` + } + } else if (result.hasClippedLines) { + // Every line was returned, so there is no later offset to read: report the + // clipping without a next-offset hint, and keep the read incomplete so a + // full-file replacement cannot be built from a clipped line. + output = `IMPORTANT: Some lines exceed ${MAX_LINE_LENGTH} characters and were clipped in this view. ${offset1 === 1 ? "The file was read in full" : `The returned slice starts at line ${offset1} and reaches the end of the file`}, but the clipped lines were not shown in full.\n${result.content}` } else if (result.returnedLines === 0) { output = "Note: File is empty" } - return output + // Complete only when the slice starts at line 1, returned every line, and + // showed every line in full (returnedLines === totalLines follows from the + // first two conditions): a partial start, a truncated tail, or a clipped + // line means the model did not see the whole file. + const complete = offset0 === 0 && !result.wasTruncated && !result.hasClippedLines + + return { content: output, complete } } /** @@ -768,9 +814,20 @@ export class ReadFileTool extends BaseTool<"read_file"> { } // Read text file - const rawContent = await fs.readFile(fullPath, "utf8") + // 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 rawBuffer = await fs.readFile(fullPath) + const rawContent = rawBuffer.toString("utf-8") + // Same contract: a lossy decode is a partial view. + const lossyDecode = !Buffer.from(rawContent).equals(rawBuffer) // Handle line ranges if specified + // S4b follow-up (#46 / epic #1375): a line-range read returns only the requested + // ranges, and a slice truncated to DEFAULT_LINE_LIMIT returns only the head of + // the file — record such observations as partial so the write guard denies a + // full-file update built on them. + let readComplete = false let content: string if (entry.lineRanges && entry.lineRanges.length > 0) { const lines = rawContent.split("\n") @@ -790,8 +847,16 @@ export class ReadFileTool extends BaseTool<"read_file"> { // Read with default limits using slice mode const result = readWithSlice(rawContent, 0, DEFAULT_LINE_LIMIT) content = result.content + readComplete = !result.wasTruncated && !result.hasClippedLines if (result.wasTruncated) { content += `\n\n[File truncated: showing ${result.returnedLines} of ${result.totalLines} total lines]` + if (result.hasClippedLines) { + // Both notices: the slice was truncated and a line inside it was + // clipped. + content += `\n\n[Some lines exceed the per-line length cap and were clipped in this view]` + } + } else if (result.hasClippedLines) { + content += `\n\n[Some lines exceed the per-line length cap and were clipped in this view]` } } @@ -799,6 +864,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, readComplete && !lossyDecode) + } + } } catch (error) { const errorMsg = error instanceof Error ? error.message : String(error) results.push(`File: ${relPath}\nError: ${errorMsg}`) diff --git a/src/core/tools/__tests__/readFileTool.spec.ts b/src/core/tools/__tests__/readFileTool.spec.ts index 6c9e177d38..bdf1a6ea0d 100644 --- a/src/core/tools/__tests__/readFileTool.spec.ts +++ b/src/core/tools/__tests__/readFileTool.spec.ts @@ -13,10 +13,16 @@ */ import path from "path" +import type { Stats } from "fs" + +import type { LegacyReadFileParams } from "@roo-code/types" import { isBinaryFile } from "isbinaryfile" import { readFileTool, ReadFileTool } from "../ReadFileTool" +import type { Task } from "../../task/Task" +import { ObservationRegistry } from "../../task/observationRegistry" +import { computeVersionToken } from "../../../utils/versionToken" import { formatResponse } from "../../prompts/responses" import { validateImageForProcessing, @@ -136,6 +142,7 @@ interface MockTaskOptions { rooIgnoreAllowed?: boolean maxImageFileSize?: number maxTotalImageSize?: number + observationRegistry?: ObservationRegistry } function createMockTask(options: MockTaskOptions = {}) { @@ -143,6 +150,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 }, @@ -187,7 +197,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({ @@ -839,7 +860,7 @@ describe("ReadFileTool", () => { mockTask.ask.mockResolvedValue({ response: "yesButtonClicked", text: undefined, images: undefined }) // fs.readFile with "utf8" encoding returns a string, not a Buffer - mockedFsReadFile.mockResolvedValue("line1\nline2\nline3\nline4\nline5" as any) + mockedFsReadFile.mockResolvedValue(Buffer.from("line1\nline2\nline3\nline4\nline5")) await readFileTool.execute( { files: [{ path: "test.ts", lineRanges: [{ start: 2, end: 4 }] }] } as any, @@ -1489,5 +1510,792 @@ describe("ReadFileTool", () => { expect(mockTask.didToolFailInCurrentTurn).toBe(true) }) + + describe("observation registry", () => { + it("records an observation on successful read of an existing file", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + // Override the beforeEach default stat mock with proper BigIntStats. + mockedFsStat.mockResolvedValue({ + isDirectory: () => false, + dev: BigInt(1), + ino: BigInt(2), + size: BigInt(300), + mtimeNs: BigInt(4_000_000_000n), + ctimeNs: BigInt(5_000_000_000n), + // Cast: the mock only implements the members the tool and versionToken read. + } as unknown as Stats) + mockedIsBinaryFile.mockResolvedValue(false) + + // Spy on observe to capture the exact key used (Windows path.resolve may use backslashes). + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute({ path: "existing.ts" }, mockTask as unknown as Task, callbacks) + + // Verify the tool called observe exactly once with a valid token. + expect(observeSpy).toHaveBeenCalledTimes(1) + const [calledPath, calledVersion] = observeSpy.mock.calls[0] + expect(calledPath).toContain("existing.ts") + expect(calledVersion).toMatch(/^\d+:\d+:\d+:\d+:\d+$/) + + // Verify get() returns the same data using the spy-captured key. + const obs = reg.get(calledPath) + expect(obs).toBeDefined() + expect(obs!.version).toBe(calledVersion) + }) + + it("a failed read (absent path) leaves the registry size 0 and does not throw", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + mockedFsReadFile.mockRejectedValue(new Error("ENOENT")) + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute({ path: "missing.ts" }, mockTask as unknown as Task, callbacks) + + // observationRegistry is guaranteed present because we passed it in createMockTask. + const reg = mockTask.observationRegistry + expect(reg).toBeDefined() + expect(reg!.size).toBe(0) + }) + + it("records an observation for legacy-format reads of existing files", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + mockedFsStat.mockResolvedValue({ + isDirectory: () => false, + dev: BigInt(1), + ino: BigInt(2), + size: BigInt(300), + mtimeNs: BigInt(4_000_000_000n), + ctimeNs: BigInt(5_000_000_000n), + // Cast: the mock only implements the members the tool and versionToken read. + } as unknown as Stats) + mockedIsBinaryFile.mockResolvedValue(false) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + // Typed legacy (pre-refactor) params: the multi-file format with the + // _legacyFormat discriminant (see LegacyReadFileParams). + const legacyParams: LegacyReadFileParams = { + files: [{ path: "legacy.ts" }], + _legacyFormat: true, + } + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute(legacyParams, mockTask as unknown as Task, callbacks) + + expect(observeSpy).toHaveBeenCalledTimes(1) + const [calledPath, calledVersion] = observeSpy.mock.calls[0] + expect(calledPath).toContain("legacy.ts") + expect(calledVersion).toMatch(/^\d+:\d+:\d+:\d+:\d+$/) + }) + + 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) + // Assert the pushed payload, not just that something was pushed. + const pushed = callbacks.pushToolResult.mock.calls[0][0] + expect(pushed).toContain("File: stat-fail.ts") + expect(pushed).toContain("test content") + expect(pushed).not.toContain("Error:") + }) + + 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) + // Assert the pushed payload, not just that something was pushed. + const pushed = callbacks.pushToolResult.mock.calls[0][0] + expect(pushed).toContain("File: post-stat-fail.ts") + expect(pushed).toContain("test content") + expect(pushed).not.toContain("Error:") + }) + + 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) + // Assert the pushed payload, not just that something was pushed. + const pushed = callbacks.pushToolResult.mock.calls[0][0] + expect(pushed).toContain("File: legacy-stat-fail.ts") + expect(pushed).toContain("test content") + expect(pushed).not.toContain("Error:") + }) + 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) + // Assert the pushed payload, not just that something was pushed. + const pushed = callbacks.pushToolResult.mock.calls[0][0] + expect(pushed).toContain("File: legacy-post-stat-fail.ts") + expect(pushed).toContain("test content") + expect(pushed).not.toContain("Error:") + }) + 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") + }) + }) + + describe("read completeness scope (S4b follow-up #46)", () => { + // The stat mock only implements the members the tool and versionToken read. + const bigintStats = (): Stats => + ({ + isDirectory: () => false, + dev: BigInt(1), + ino: BigInt(2), + size: BigInt(300), + mtimeNs: BigInt(4_000_000_000n), + ctimeNs: BigInt(5_000_000_000n), + }) as unknown as Stats + + it("native: a full, untruncated slice read from line 1 records a complete observation", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + mockedFsStat.mockResolvedValue(bigintStats()) + mockedIsBinaryFile.mockResolvedValue(false) + mockedFsReadFile.mockResolvedValue(Buffer.from("a\nb\n")) + mockedReadWithSlice.mockReturnValue({ + content: "1 | a\n2 | b", + returnedLines: 2, + totalLines: 2, + wasTruncated: false, + includedRanges: [[1, 2]], + }) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + await readFileTool.execute({ path: "full.ts" }, mockTask as unknown as Task, callbacks) + + expect(observeSpy).toHaveBeenCalledTimes(1) + const [calledPath, calledVersion, calledComplete] = observeSpy.mock.calls[0] + expect(calledPath).toContain("full.ts") + expect(calledVersion).toMatch(/^\d+:\d+:\d+:\d+:\d+$/) + expect(calledComplete).toBe(true) + expect(reg.get(calledPath)!.complete).toBe(true) + }) + + it("native: a truncated slice read records a partial observation", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + mockedFsStat.mockResolvedValue(bigintStats()) + mockedIsBinaryFile.mockResolvedValue(false) + mockedFsReadFile.mockResolvedValue(Buffer.from("a\nb\nc\nd\ne")) + mockedReadWithSlice.mockReturnValue({ + content: "1 | a", + returnedLines: 1, + totalLines: 5, + wasTruncated: true, + includedRanges: [[1, 1]], + }) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + await readFileTool.execute( + { path: "trunc.ts", offset: 1, limit: 1 }, + mockTask as unknown as Task, + callbacks, + ) + + expect(observeSpy).toHaveBeenCalledTimes(1) + const [calledPath, , calledComplete] = observeSpy.mock.calls[0] + expect(calledComplete).toBe(false) + expect(reg.get(calledPath)!.complete).toBe(false) + }) + + it("native: a full read whose line content was clipped records a partial observation", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + mockedFsStat.mockResolvedValue(bigintStats()) + mockedIsBinaryFile.mockResolvedValue(false) + mockedFsReadFile.mockResolvedValue(Buffer.from("a\nb\n")) + mockedReadWithSlice.mockReturnValue({ + content: "1 | a\n2 | b", + returnedLines: 2, + totalLines: 2, + wasTruncated: false, + hasClippedLines: true, + includedRanges: [[1, 2]], + }) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + await readFileTool.execute({ path: "clipped.ts" }, mockTask as unknown as Task, callbacks) + + const [, , calledComplete] = observeSpy.mock.calls[0] + expect(calledComplete).toBe(false) + // Every line was returned, so the notice must not point at a next + // offset that is beyond the file. + const pushed = callbacks.pushToolResult.mock.calls[0][0] + expect(pushed).toContain("clipped in this view") + expect(pushed).not.toContain("To read more") + // The notice is added on top of the read, it does not replace it. + expect(pushed).toContain("1 | a") + }) + it("native: a clipped slice that starts after line 1 names the slice instead of claiming a full read", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + mockedFsStat.mockResolvedValue(bigintStats()) + mockedIsBinaryFile.mockResolvedValue(false) + mockedFsReadFile.mockResolvedValue(Buffer.from("a\nb\nc\nd\ne\n")) + mockedReadWithSlice.mockReturnValue({ + content: "3 | c\n4 | d", + returnedLines: 2, + totalLines: 5, + wasTruncated: false, + hasClippedLines: true, + includedRanges: [[3, 4]], + }) + + await readFileTool.execute({ path: "clipped-slice.ts", offset: 3 }, mockTask as unknown as Task, callbacks) + + const pushed = callbacks.pushToolResult.mock.calls[0][0] + expect(pushed).toContain("clipped in this view") + // Lines 1-2 were omitted, so the notice must describe the slice that was + // returned rather than claim the whole file was read. + expect(pushed).toContain("starts at line 3") + expect(pushed).not.toContain("The file was read in full") + expect(pushed).toContain("3 | c") + }) + + it("native: a truncated slice that also clipped a line reports both notices", async () => { + // A long line inside a slice that also cut lines off is a plausible case, + // and the response has to say both things. + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + mockedFsStat.mockResolvedValue(bigintStats()) + mockedIsBinaryFile.mockResolvedValue(false) + mockedFsReadFile.mockResolvedValue(Buffer.from("a\nb\nc")) + mockedReadWithSlice.mockReturnValue({ + content: "1 | a\n2 | b", + returnedLines: 2, + totalLines: 3, + wasTruncated: true, + hasClippedLines: true, + includedRanges: [[1, 2]], + }) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + await readFileTool.execute({ path: "both.ts" }, mockTask as unknown as Task, callbacks) + + const [, , calledComplete] = observeSpy.mock.calls[0] + expect(calledComplete).toBe(false) + + const pushed = callbacks.pushToolResult.mock.calls[0][0] + expect(pushed).toContain("Showing lines 1-2 of 3 total lines") + expect(pushed).toContain("clipped in this view") + }) + + it("native: an offset read that is not truncated still records a partial observation", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + mockedFsStat.mockResolvedValue(bigintStats()) + mockedIsBinaryFile.mockResolvedValue(false) + mockedFsReadFile.mockResolvedValue(Buffer.from("a\nb\nc\nd\ne")) + mockedReadWithSlice.mockReturnValue({ + content: "4 | d\n5 | e", + returnedLines: 2, + totalLines: 5, + wasTruncated: false, + includedRanges: [[4, 5]], + }) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + await readFileTool.execute( + { path: "offset.ts", offset: 4, limit: 2 }, + mockTask as unknown as Task, + callbacks, + ) + + expect(observeSpy).toHaveBeenCalledTimes(1) + const [calledPath, , calledComplete] = observeSpy.mock.calls[0] + expect(calledComplete).toBe(false) + }) + + it("native: an indentation-mode block read records a partial observation", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + mockedFsStat.mockResolvedValue(bigintStats()) + mockedIsBinaryFile.mockResolvedValue(false) + mockedFsReadFile.mockResolvedValue(Buffer.from("function f() { return 1 }")) + mockedReadWithIndentation.mockReturnValue({ + content: "10 | function f() {", + wasTruncated: false, + includedRanges: [[10, 20]], + totalLines: 100, + returnedLines: 11, + }) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + await readFileTool.execute( + { path: "indent.ts", mode: "indentation" }, + mockTask as unknown as Task, + callbacks, + ) + + expect(observeSpy).toHaveBeenCalledTimes(1) + const [calledPath, , calledComplete] = observeSpy.mock.calls[0] + expect(calledComplete).toBe(false) + }) + + it("legacy: a line-range read records a partial observation", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + mockedFsStat.mockResolvedValue(bigintStats()) + mockedIsBinaryFile.mockResolvedValue(false) + mockedFsReadFile.mockResolvedValue(Buffer.from("a\nb\nc\nd\ne")) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + const legacyParams: LegacyReadFileParams = { + files: [{ path: "ranges.ts", lineRanges: [{ start: 2, end: 4 }] }], + _legacyFormat: true, + } + + await readFileTool.execute(legacyParams, mockTask as unknown as Task, callbacks) + + expect(observeSpy).toHaveBeenCalledTimes(1) + const [calledPath, , calledComplete] = observeSpy.mock.calls[0] + expect(calledPath).toContain("ranges.ts") + expect(calledComplete).toBe(false) + }) + + it("legacy: a full, untruncated slice read records a complete observation", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + mockedFsStat.mockResolvedValue(bigintStats()) + mockedIsBinaryFile.mockResolvedValue(false) + mockedFsReadFile.mockResolvedValue(Buffer.from("a\nb")) + mockedReadWithSlice.mockReturnValue({ + content: "1 | a\n2 | b", + returnedLines: 2, + totalLines: 2, + wasTruncated: false, + includedRanges: [[1, 2]], + }) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + const legacyParams: LegacyReadFileParams = { + files: [{ path: "legacy-full.ts" }], + _legacyFormat: true, + } + + await readFileTool.execute(legacyParams, mockTask as unknown as Task, callbacks) + + expect(observeSpy).toHaveBeenCalledTimes(1) + const [calledPath, , calledComplete] = observeSpy.mock.calls[0] + expect(calledComplete).toBe(true) + + // Nothing was omitted and nothing was clipped, so no note is added. + const pushed = callbacks.pushToolResult.mock.calls[0][0] + expect(pushed).not.toContain("clipped in this view") + expect(pushed).not.toContain("total lines") + }) + + it("legacy: a full read with a clipped line records a partial observation and reports the clipping", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + mockedFsStat.mockResolvedValue(bigintStats()) + mockedIsBinaryFile.mockResolvedValue(false) + mockedFsReadFile.mockResolvedValue(Buffer.from("a\nb")) + mockedReadWithSlice.mockReturnValue({ + content: "1 | a\n2 | b", + returnedLines: 2, + totalLines: 2, + wasTruncated: false, + hasClippedLines: true, + includedRanges: [[1, 2]], + }) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + const legacyParams: LegacyReadFileParams = { + files: [{ path: "legacy-clipped.ts" }], + _legacyFormat: true, + } + + await readFileTool.execute(legacyParams, mockTask as unknown as Task, callbacks) + + const [, , calledComplete] = observeSpy.mock.calls[0] + expect(calledComplete).toBe(false) + + // Every line was returned, so the note reports the clipping instead of + // a showing-N-of-N count that would point past the file. + const pushed = callbacks.pushToolResult.mock.calls[0][0] + expect(pushed).toContain("clipped in this view") + expect(pushed).not.toContain("showing 2 of 2 total lines") + }) + + it("legacy: a truncated slice that also clipped a line reports both notices", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + mockedFsStat.mockResolvedValue(bigintStats()) + mockedIsBinaryFile.mockResolvedValue(false) + mockedFsReadFile.mockResolvedValue(Buffer.from("a\nb\nc")) + mockedReadWithSlice.mockReturnValue({ + content: "1 | a\n2 | b", + returnedLines: 2, + totalLines: 3, + wasTruncated: true, + hasClippedLines: true, + includedRanges: [[1, 2]], + }) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + const legacyParams: LegacyReadFileParams = { + files: [{ path: "legacy-both.ts" }], + _legacyFormat: true, + } + + await readFileTool.execute(legacyParams, mockTask as unknown as Task, callbacks) + + const [, , calledComplete] = observeSpy.mock.calls[0] + expect(calledComplete).toBe(false) + + const pushed = callbacks.pushToolResult.mock.calls[0][0] + expect(pushed).toContain("showing 2 of 3 total lines") + expect(pushed).toContain("clipped in this view") + }) + + it("legacy: a slice truncated to the default limit records a partial observation", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + mockedFsStat.mockResolvedValue(bigintStats()) + mockedIsBinaryFile.mockResolvedValue(false) + mockedFsReadFile.mockResolvedValue(Buffer.from("a\nb\nc")) + mockedReadWithSlice.mockReturnValue({ + content: "1 | a", + returnedLines: 1, + totalLines: 5000, + wasTruncated: true, + includedRanges: [[1, 1]], + }) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + const legacyParams: LegacyReadFileParams = { + files: [{ path: "legacy-trunc.ts" }], + _legacyFormat: true, + } + + await readFileTool.execute(legacyParams, mockTask as unknown as Task, callbacks) + + expect(observeSpy).toHaveBeenCalledTimes(1) + const [calledPath, , calledComplete] = observeSpy.mock.calls[0] + expect(calledComplete).toBe(false) + + // Lines were omitted here, so the note reports the omitted range rather + // than clipping. + const pushed = callbacks.pushToolResult.mock.calls[0][0] + expect(pushed).toContain("showing 1 of 5000 total lines") + }) + + it("native: a read whose bytes did not survive the UTF-8 decode records a partial observation", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + // 0xFF is not valid UTF-8, so the model receives U+FFFD instead of the byte. + const raw = Buffer.from([0x61, 0xff]) + mockedFsStat.mockResolvedValue(bigintStats()) + mockedIsBinaryFile.mockResolvedValue(false) + mockedFsReadFile.mockResolvedValue(raw) + mockedReadWithSlice.mockReturnValue({ + content: "1 | a\uFFFD", + returnedLines: 1, + totalLines: 1, + wasTruncated: false, + includedRanges: [[1, 1]], + }) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + await readFileTool.execute({ path: "lossy.ts" }, mockTask as unknown as Task, callbacks) + + expect(observeSpy).toHaveBeenCalledTimes(1) + const [calledPath, , calledComplete] = observeSpy.mock.calls[0] + expect(calledComplete).toBe(false) + expect(reg.get(calledPath)!.complete).toBe(false) + }) + + it("legacy: a read whose bytes did not survive the UTF-8 decode records a partial observation", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + const raw = Buffer.from([0x61, 0xff]) + mockedFsStat.mockResolvedValue(bigintStats()) + mockedIsBinaryFile.mockResolvedValue(false) + mockedFsReadFile.mockResolvedValue(raw) + mockedReadWithSlice.mockReturnValue({ + content: "1 | a\uFFFD", + returnedLines: 1, + totalLines: 1, + wasTruncated: false, + includedRanges: [[1, 1]], + }) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + const legacyParams: LegacyReadFileParams = { + files: [{ path: "legacy-lossy.ts" }], + _legacyFormat: true, + } + + await readFileTool.execute(legacyParams, mockTask as unknown as Task, callbacks) + + expect(observeSpy).toHaveBeenCalledTimes(1) + const [calledPath, , calledComplete] = observeSpy.mock.calls[0] + expect(calledComplete).toBe(false) + expect(reg.get(calledPath)!.complete).toBe(false) + }) + }) }) }) diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 583485c628..d607509cdc 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": 98 + "count": 96 } }, "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/misc/__tests__/indentation-reader.spec.ts b/src/integrations/misc/__tests__/indentation-reader.spec.ts index d46cb54277..e9b27e7191 100644 --- a/src/integrations/misc/__tests__/indentation-reader.spec.ts +++ b/src/integrations/misc/__tests__/indentation-reader.spec.ts @@ -1,4 +1,5 @@ import { describe, it, expect } from "vitest" +import { MAX_LINE_LENGTH } from "../../../core/prompts/tools/native-tools/read_file" import { parseLines, formatWithLineNumbers, @@ -279,11 +280,45 @@ describe("readWithSlice", () => { expect(result.wasTruncated).toBe(true) }) + it("reports a clipped line separately from omitted lines", () => { + // Every line is returned, but formatWithLineNumbers clips a line longer + // than MAX_LINE_LENGTH, so the model did not see the whole file. + const lines = ["x".repeat(MAX_LINE_LENGTH + 10), "short"].join("\n") + const result = readWithSlice(lines, 0, 10) + + expect(result.returnedLines).toBe(2) + expect(result.wasTruncated).toBe(false) + expect(result.hasClippedLines).toBe(true) + }) + + it("keeps a slice complete when a line is exactly at the length cap", () => { + // formatWithLineNumbers clips only lines strictly longer than the cap, so a + // line at exactly MAX_LINE_LENGTH is shown in full and the read is complete. + const lines = ["x".repeat(MAX_LINE_LENGTH), "short"].join("\n") + const result = readWithSlice(lines, 0, 10) + + expect(result.returnedLines).toBe(2) + expect(result.wasTruncated).toBe(false) + expect(result.hasClippedLines).toBe(false) + }) + + it("flags clipping when any line is clipped, not only when every line is", () => { + // The first line is clipped and the second is shown in full: some lines are + // a partial view even though every line was returned. + const lines = ["y".repeat(MAX_LINE_LENGTH + 1), "short"].join("\n") + const result = readWithSlice(lines, 0, 10) + + expect(result.returnedLines).toBe(2) + expect(result.hasClippedLines).toBe(true) + }) + it("should handle offset beyond file end", () => { const result = readWithSlice(SIMPLE_CODE, 1000, 10) expect(result.returnedLines).toBe(0) expect(result.content).toContain("Error") + // No line was returned, so nothing could have been clipped. + expect(result.hasClippedLines).toBe(false) }) it("should handle negative offset", () => { @@ -297,6 +332,14 @@ describe("readWithSlice", () => { // ─── readWithIndentation Tests ──────────────────────────────────────────────── describe("readWithIndentation", () => { + it("reports an out-of-range anchor as an error with no clipping", () => { + const result = readWithIndentation(SIMPLE_CODE, { anchorLine: 1000 }) + + expect(result.content).toContain("out of range") + expect(result.returnedLines).toBe(0) + expect(result.hasClippedLines).toBe(false) + }) + describe("basic block extraction", () => { it("should extract content around the anchor line", () => { const result = readWithIndentation(PYTHON_CODE, { diff --git a/src/integrations/misc/indentation-reader.ts b/src/integrations/misc/indentation-reader.ts index aecabd5982..5cbd23d168 100644 --- a/src/integrations/misc/indentation-reader.ts +++ b/src/integrations/misc/indentation-reader.ts @@ -58,8 +58,10 @@ export interface IndentationReadResult { totalLines: number /** Lines actually returned */ returnedLines: number - /** Whether output was truncated due to limit */ + /** Whether output was truncated because lines were omitted */ wasTruncated: boolean + /** Whether any returned line was clipped by the per-line length cap */ + hasClippedLines?: boolean } // ─── Constants ──────────────────────────────────────────────────────────────── @@ -306,6 +308,7 @@ export function readWithIndentation(content: string, options: IndentationReadOpt totalLines, returnedLines: 0, wasTruncated: false, + hasClippedLines: false, } } @@ -448,6 +451,7 @@ export function readWithSlice( totalLines, returnedLines: 0, wasTruncated: false, + hasClippedLines: false, } } @@ -455,6 +459,11 @@ export function readWithSlice( const endIdx = Math.min(offset + limit, totalLines) const selectedLines = lines.slice(offset, endIdx) const wasTruncated = endIdx < totalLines + // A returned line can still be a partial view: formatWithLineNumbers clips a + // line longer than MAX_LINE_LENGTH, so a slice that returned every line may + // still hide content. Clipping is reported separately from omission so the + // caller does not suggest a next offset that is beyond the file. + const hasClippedLines = selectedLines.some((line) => line.content.length > MAX_LINE_LENGTH) // Format output const formattedContent = formatWithLineNumbers(selectedLines) @@ -465,5 +474,6 @@ export function readWithSlice( totalLines, returnedLines: selectedLines.length, wasTruncated, + hasClippedLines, } } 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..8b76906dda --- /dev/null +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -0,0 +1,1134 @@ +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 { + PostCommitDurabilityError, + resolveLockKey, + RollbackFailureError, + safeWriteText, + StagingPathError, + type SafeWriteTextOptions, +} from "../safeWriteText" + +// Full mock for fs/promises — all methods are vi.fn() stubs +vi.mock("fs/promises", () => ({ + mkdir: vi.fn(), + access: vi.fn(), + rename: vi.fn(), + unlink: vi.fn(), + rmdir: vi.fn(), + realpath: vi.fn(), + lstat: vi.fn(), + readlink: 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)) +} +// Minimal Stats stand-in: the SUT only reads `.mode` from it. +// Async lstat stand-in: the SUT only asks whether the path is a link or a file. +// Built on the Stats prototype so the mock value still satisfies fsSync.Stats. +function _fileStats(isLink: boolean): fsSync.Stats { + const s = Object.create(fsSync.Stats.prototype) as fsSync.Stats + s.isSymbolicLink = () => isLink + s.isFile = () => !isLink + return s +} + +function mockDefaults(): void { + 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) + vi.mocked(fs.rmdir).mockResolvedValue(undefined) + // Existing-target default: a regular 0o644 file. + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o644)) + // Staged-file default: a regular file, not a link, so a caller-supplied + // tempPath passes the location and file-type check by default. + vi.mocked(fs.lstat).mockResolvedValue(_fileStats(false)) +} +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(() => { + mockDefaults() + // 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 now-empty staging directory after a successful self-staged commit", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "hello", { platform: "linux" }) + + // the staging subdir is removed best-effort after the commit rename + // (stringContaining: the SUT and the test helper resolve Windows + // drive-relative paths differently, as in the existing staging tests) + expect(fs.rmdir).toHaveBeenCalledTimes(1) + expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + // the win32 DACL restore gate must stay closed on other platforms: + // no icacls save or restore is attempted + expect(execFile).not.toHaveBeenCalled() + }) + + it("still removes the staging directory when no options are supplied at all", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + // options is undefined: the self-staged check and the optional-chained + // DACL runner lookup must not dereference it + await expect(safeWriteText(targetPath, "hello")).resolves.toBeUndefined() + + expect(fs.rmdir).toHaveBeenCalledTimes(1) + expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + if (process.platform === "win32") { + // default platform is win32: the DACL save + restore still ran + // through the default icacls path (options?.execFileRunner must + // not throw when options is undefined) + expect(vi.mocked(execFile)).toHaveBeenCalledTimes(2) + expect(vi.mocked(fs.unlink)).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.acl")) + } + }) + + it("does not remove the staging directory when the caller supplies its own tempPath", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const callerTemp = "/tmp/test-dir/caller-staged.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "hello", { platform: "linux", tempPath: callerTemp }) + + // the caller owns its temp file's directory; safeWriteText must not + // rmdir a directory it did not create + expect(fs.rmdir).not.toHaveBeenCalled() + }) + + it("a failed staging-dir removal never fails the committed write", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fs.rmdir).mockRejectedValue(Object.assign(new Error("ENOTEMPTY"), { code: "ENOTEMPTY" })) + + await expect(safeWriteText(targetPath, "hello", { platform: "linux" })).resolves.toBeUndefined() + + // the commit rename still happened and the rmdir error was swallowed + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath) + expect(fs.rmdir).toHaveBeenCalledTimes(1) + expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + }) + + it("gives each self-staged write its own staging directory so a concurrent write cannot remove it", async () => { + const targetA = "/tmp/test-dir/target-a.txt" + const targetB = "/tmp/test-dir/target-b.txt" + vi.mocked(fs.realpath).mockImplementation((p) => Promise.resolve(p as string)) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetA, "a", { platform: "linux" }) + await safeWriteText(targetB, "b", { platform: "linux" }) + + // Two self-staged writes in the same directory must not share one staging + // directory: the first write's best-effort rmdir would otherwise delete the + // directory the second write had created but not yet opened (ENOENT on openSync). + const created = vi.mocked(fsSync.mkdirSync).mock.calls.map((c) => String(c[0])) + const staging = created.filter((p) => p.includes(".file-safety-staging_")) + expect(staging).toHaveLength(2) + expect(staging[0]).not.toBe(staging[1]) + // Uniqueness comes from the documented name shape + // /.file-safety-staging__: pinning the shape + // keeps the separator and the random suffix meaningful, not just the prefix. + for (const dir of staging) { + expect(dir).toMatch(/\.file-safety-staging_\d+_[a-z0-9]+$/) + } + const removed = vi.mocked(fs.rmdir).mock.calls.map((c) => String(c[0])) + expect(removed).toEqual([staging[0], staging[1]]) + }) + + it("removes its own staging directory when a self-staged write 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(Object.assign(new Error("EACCES"), { code: "EACCES" })) + + await expect(safeWriteText(targetPath, "hello", { platform: "linux" })).rejects.toThrow("EACCES") + + // The failed write's temp file is unlinked, then the directory it + // created is removed — a failed write must not leave an empty + // .file-safety-staging directory behind. + // mkdirSync created this write's staging directory; the temp file lives + // inside it, so the unlink targets a path under that directory. + const staging = vi.mocked(fsSync.mkdirSync).mock.calls.map((c) => String(c[0])) + expect(staging).toHaveLength(1) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(staging[0])) + expect(fs.rmdir).toHaveBeenCalledWith(staging[0]) + // The directory is only empty after its temp file is gone, so the + // unlink must happen before the rmdir. + expect(vi.mocked(fs.unlink).mock.invocationCallOrder[0]).toBeLessThan( + vi.mocked(fs.rmdir).mock.invocationCallOrder[0], + ) + }) + + it("does not remove a staging directory it did not create when a caller-staged write fails", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const callerTemp = "/tmp/test-dir/caller-staged.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fs.rename).mockRejectedValue(Object.assign(new Error("EACCES"), { code: "EACCES" })) + + await expect( + safeWriteText(targetPath, "hello", { platform: "linux", tempPath: callerTemp }), + ).rejects.toThrow("EACCES") + + // The caller owns that directory: only the caller's temp file is cleaned, + // never a rmdir of a directory safeWriteText never created. + expect(fs.unlink).toHaveBeenCalledWith(callerTemp) + expect(fs.rmdir).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).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + // The title promises the order, so compare the invocations rather than + // only count them: a rename before closeSync, or a close before fsync, + // would not be a durable commit. + const fsyncOrder = vi.mocked(fsSync.fsyncSync).mock.invocationCallOrder[0] + const closeOrder = vi.mocked(fsSync.closeSync).mock.invocationCallOrder[0] + const renameOrder = vi.mocked(fs.rename).mock.invocationCallOrder[0] + expect(fsyncOrder).toBeLessThan(closeOrder) + expect(closeOrder).toBeLessThan(renameOrder) + }) + }) + + // ── Test 3: simulated failure between write and rename leaves target intact ── + + describe("crash/torn-write safety", () => { + it("simulated failure between fsync and rename leaves the target byte-identical and no temp left behind", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fs.rename).mockRejectedValue(new Error("ENOSPC")) + + await expect(safeWriteText(targetPath, "new data", { platform: "linux" })).rejects.toThrow("ENOSPC") + + // rename was attempted (the failure point) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // temp file was cleaned up on failure + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + + // backup was NOT created (backup:false by default), so target is untouched + // The only rename call was temp→target, not a rollback rename + expect(fs.rename).toHaveBeenCalledTimes(1) + }) + + it("a post-commit backup cleanup failure is non-fatal: the target stays committed and no temp is left behind", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The post-commit backup unlink (SUT step 6) fails — the write must + // still succeed; an orphaned backup is the documented acceptable + // outcome, so the failure is swallowed instead of rolling back. + vi.mocked(fs.unlink).mockRejectedValueOnce(new Error("EPERM")) + + await safeWriteText(targetPath, "data", { backup: true, platform: "linux" }) + + // the commit rename (temp -> target) still happened + expect(fs.rename).toHaveBeenNthCalledWith(2, expect.stringContaining("safeWriteText_"), targetPath) + + // the failing cleanup was the post-commit backup unlink + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + + // no rollback rename: the committed target is not restored from the backup + expect(fs.rename).toHaveBeenCalledTimes(2) + + // the staging temp was already committed by the rename; nothing + // temp-shaped is unlinked afterwards + expect(fs.unlink).not.toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("a failed post-commit directory fsync does not roll the backup back over the published content", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const dirPath = path.dirname(targetPath) + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // The file fd opens normally; the parent-directory open after the commit + // rename fails, which is the post-commit durability failure. + vi.mocked(fsSync.openSync).mockImplementation((target) => { + if (String(target) === dirPath) throw new Error("EBADF") + return 1 + }) + + await expect(safeWriteText(targetPath, "new data", { backup: true, platform: "linux" })).rejects.toThrow(PostCommitDurabilityError) + + // The commit rename already published the new content, so the backup must + // not be renamed back over it: only target->backup and temp->target run. + expect(fs.rename).toHaveBeenNthCalledWith(1, targetPath, expect.stringContaining("safeWriteText.bak_")) + expect(fs.rename).toHaveBeenNthCalledWith(2, expect.stringContaining("safeWriteText_"), targetPath) + expect(fs.rename).toHaveBeenCalledTimes(2) + }) + }) + + // ── Test 4: backup:true keeps old safeWriteJson semantics incl. rollback ── + + describe("backup:true", () => { + it("renames target -> backup before commit, deletes backup on success", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "new data", { backup: true }) + + // target was accessed (exists check) + expect(fs.access).toHaveBeenCalledWith(targetPath) + + // first rename: target -> backup + expect(fs.rename).toHaveBeenNthCalledWith(1, targetPath, expect.stringContaining("safeWriteText.bak_")) + + // second rename: temp -> target (realpath mock returns targetPath) + expect(fs.rename).toHaveBeenNthCalledWith(2, expect.stringContaining("safeWriteText_"), targetPath) + + // backup was deleted on success + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + }) + + it("rollback: on failure after rename target->backup, restores backup to target", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // first rename (target->backup) succeeds, second fails + let callCount = 0 + vi.mocked(fs.rename).mockImplementation(async () => { + callCount++ + if (callCount === 1) return // target -> backup + if (callCount === 2) throw new Error("ENOSPC") // temp -> target fails + return // the rollback rename succeeds + }) + + await expect(safeWriteText(targetPath, "new data", { backup: true })).rejects.toThrow("ENOSPC") + + // rollback rename is the 3rd call (after target->backup and temp->target failure) + expect(fs.rename).toHaveBeenNthCalledWith(3, expect.stringContaining("safeWriteText.bak_"), targetPath) + + // temp was cleaned up on failure + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("a failed rollback reports the partial state, not only the publish error", async () => { + // The content is still on disk, but only at the backup path. A caller that gets + // just the publish error has data it cannot find at the expected path. + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + let callCount = 0 + vi.mocked(fs.rename).mockImplementation(async () => { + callCount++ + if (callCount === 1) return // target -> backup + if (callCount === 2) throw new Error("ENOSPC") // temp -> target fails + throw new Error("EACCES") // the rollback rename fails too + }) + + let failure: RollbackFailureError | undefined + await safeWriteText(targetPath, "new data", { backup: true }).catch((e: unknown) => { + if (e instanceof RollbackFailureError) { + failure = e + return + } + throw e + }) + + expect(failure).toBeInstanceOf(RollbackFailureError) + expect(failure?.publishError).toBeInstanceOf(Error) + expect((failure?.publishError as Error).message).toBe("ENOSPC") + expect((failure?.rollbackError as Error).message).toBe("EACCES") + expect(failure?.backupPath).toContain("safeWriteText.bak_") + // The backup is what the caller can still recover, so it must stay on disk. + expect(fs.unlink).not.toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + }) + + 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).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath) + // one icacls attempt only: a failed DACL apply must not try to restore + expect(execFile).toHaveBeenCalledTimes(1) + }) + + it("win32 DACL: a partial dump left by a failed save is removed and never restored", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // icacls save fails — a real icacls may have written a partial dump + // before erroring, so the dump path must be cleaned up and must never + // be used for a restore. + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + if (typeof cb === "function") cb(new Error("icacls save error"), "", "") + return fakeChild + }) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // write committed; only the save was attempted (no restore from a failed dump) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(execFile).toHaveBeenCalledTimes(1) + const saveArgs = vi.mocked(execFile).mock.calls[0]?.[1] + expect(saveArgs?.[1]).toBe("/save") + // the dump path (possibly partially created by icacls) was unlinked + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.acl")) + }) + + 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("safeWriteText.acl"), "/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("safeWriteText.acl"), + ]) + + // dump file was unlinked after restore + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.acl")) + }) + + it("win32 DACL save runs before the backup rename, not after it", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + // The title is about order, so assert the order the mocks were actually + // called in. If the save ran after the backup rename the target would + // already be gone and the dump would describe the wrong file. + await safeWriteText(targetPath, "data", { backup: true, platform: "win32" }) + + const callOrder = vi.mocked(execFile).mock.invocationCallOrder + const renameOrder = vi.mocked(fs.rename).mock.invocationCallOrder + const saveCall = callOrder[0] + const restoreCall = callOrder[1] + const backupRename = renameOrder[0] + const commitRename = renameOrder[1] + + expect(saveCall).toBeLessThan(backupRename) + expect(backupRename).toBeLessThan(commitRename) + expect(commitRename).toBeLessThan(restoreCall) + }) + + 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).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + expect(fs.rename).toHaveBeenCalledTimes(1) + + // dump file was still unlinked in finally + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.acl")) + }) + + 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/test-dir/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/test-dir/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/test-dir/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("propagates a non-ENOENT stat failure rather than defaulting the mode (caller-staged)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + const eacces = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + vi.mocked(fsSync.statSync).mockImplementation(() => { + throw eacces + }) + vi.mocked(fsSync.openSync).mockReturnValue(2) + + const customTempPath = "/tmp/test-dir/custom-temp.tmp" + + // A target that cannot be stat'd is not a fresh target: publishing with + // the default mode would widen a restrictive target through the rename. + await expect( + safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }), + ).rejects.toThrow("EACCES") + expect(fsSync.fchmodSync).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("propagates a non-ENOENT stat failure rather than defaulting the mode (self-staged)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + const eio = Object.assign(new Error("EIO: i/o error"), { code: "EIO" }) + vi.mocked(fsSync.statSync).mockImplementation(() => { + throw eio + }) + + // The mode is read before the temp is opened, so a real I/O failure stops + // the write before anything is staged. + await expect(safeWriteText(targetPath, "hello world", { platform: "linux" })).rejects.toThrow("EIO") + expect(fsSync.openSync).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("opens the temp before applying a read-only target's mode (0o444 does not block the open)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o444)) + vi.mocked(fsSync.openSync).mockReturnValue(3) + + const customTempPath = "/tmp/test-dir/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) + }) + + it("applies the existing target's exact mode to the self-staged temp (umask must not narrow it)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o664)) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // openSync's creation mode is narrowed by the process umask (0o664 -> 0o644 with + // the common 0o022), and the rename publishes the temp's mode onto the target, + // so the existing target's mode must be applied on the fd before the commit. + expect(fsSync.fchmodSync).toHaveBeenCalledWith(1, 0o664) + 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(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("does not fchmod the self-staged temp for a fresh target", 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(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // Nothing exists to preserve: the default creation mode is the intended one. + expect(fsSync.fchmodSync).not.toHaveBeenCalled() + }) + }) + + // ── 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" })) + // lstat reports the path itself as absent, so this is a new target and + // the fallback is allowed. + vi.mocked(fs.lstat).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) + }) + + it("propagates a dangling symlink instead of writing through the link path", async () => { + // realpath resolves the referent, so a link whose target is missing reports + // ENOENT. Falling back to the link path would replace the symlink with a + // regular file, so the error must propagate and nothing may be committed. + const linkPath = "/tmp/test-dir/dangling-link.txt" + vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + const linkStats = Object.create(fsSync.Stats.prototype) as fsSync.Stats + linkStats.isSymbolicLink = () => true + vi.mocked(fs.lstat).mockResolvedValue(linkStats) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await expect(safeWriteText(linkPath, "data", { platform: "linux" })).rejects.toThrow("ENOENT") + + expect(fs.rename).not.toHaveBeenCalled() + }) + }) + + // ── 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("reports a failed parent-directory fsync instead of claiming a durable write", 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 committed, so the caller can still find the data at + // the target; what the write cannot claim is that the directory entry + // reached the disk. Returning success here would claim durability the + // filesystem did not grant. + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toThrow( + PostCommitDurabilityError, + ) + + 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() + }) + }) + describe("content bytes", () => { + const targetPath = "/tmp/enc-dir/target.txt" + + beforeEach(() => { + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + }) + + it("stages UTF-8 bytes for string content", async () => { + await safeWriteText(targetPath, "héllo", { platform: "linux" }) + expect(fsSync.writeSync).toHaveBeenCalledWith(1, Buffer.from("héllo", "utf8"), 0, 6) + }) + + it("publishes caller-supplied bytes unchanged instead of re-encoding them", async () => { + // The extension host encodes a document with VS Code's own codec, which + // covers the legacy code pages and BOMs Node cannot represent, and hands + // the result over: those bytes must reach the commit rename exactly as + // they were given. + const bytes = Buffer.from([0x00, 0x68, 0x00, 0x69]) + await safeWriteText(targetPath, bytes, { platform: "linux" }) + expect(fsSync.writeSync).toHaveBeenCalledWith(1, bytes, 0, 4) + }) + }) +}) + +// ── Test 12: lock key, staging path, and post-commit durability ───────────── + +describe("resolveLockKey", () => { + beforeEach(() => mockDefaults()) + + it("canonicalizes the parent directory, not just the file", async () => { + vi.mocked(fs.realpath).mockImplementation(async (target) => { + const key = String(target) + if (key === "/tmp/linkdir/file.json") return "/real/dir/file.json" + if (key === "/real/dir") return "/real/dir" + return key + }) + + // The key is the canonical directory plus the basename, so a symlinked + // ancestor and its referent share one lock. + await expect(resolveLockKey("/tmp/linkdir/file.json")).resolves.toBe(path.join("/real/dir", "file.json")) + }) + + it("computes a key for a dangling link, which resolvePublishTarget refuses", async () => { + const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + vi.mocked(fs.realpath).mockRejectedValue(enoent) + vi.mocked(fs.lstat).mockResolvedValue(_fileStats(true)) + // Only the link path is read, so a single answer is enough and keeps the mock's + // return type matching fs.promises.readlink. + vi.mocked(fs.readlink).mockResolvedValue("referent.json") + + // Mid-commit a peer writer renames the referent away and back, so the key + // must still be computable while the link dangles. + await expect(resolveLockKey("/tmp/linkdir/file.json")).resolves.toBe( + path.resolve(path.join("/tmp/linkdir", "referent.json")), + ) + }) + + it("terminates on a two-link cycle instead of walking forever", async () => { + const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + vi.mocked(fs.realpath).mockRejectedValue(enoent) + vi.mocked(fs.lstat).mockResolvedValue(_fileStats(true)) + // Every readlink answers with the same link, so an unbounded walk would + // never end; the bounded walk returns the key it actually reached. + vi.mocked(fs.readlink).mockImplementation(async () => "a.json") + + await expect(resolveLockKey("/tmp/linkdir/a.json")).resolves.toBe( + path.resolve(path.join("/tmp/linkdir", "a.json")), + ) + expect(fs.readlink).toHaveBeenCalledTimes(8) + }) +}) + +describe("caller-supplied staging path", () => { + beforeEach(() => mockDefaults()) + + it("rejects a staging file outside the target's directory before writing anything", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + + // A rename across filesystems fails with EXDEV, and a path elsewhere lets + // a caller publish an unrelated file onto the target. + await expect( + safeWriteText(targetPath, "data", { tempPath: "/tmp/other-dir/x.tmp", platform: "linux" }), + ).rejects.toThrow(StagingPathError) + expect(fsSync.openSync).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("rejects a staging path that is a symlink", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fs.lstat).mockResolvedValue(_fileStats(true)) + + // Renaming a link over the target publishes whatever the link points at. + await expect( + safeWriteText(targetPath, "data", { tempPath: "/tmp/test-dir/x.tmp", platform: "linux" }), + ).rejects.toThrow(StagingPathError) + expect(fsSync.openSync).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + }) +}) + +describe("cleanup before a rollback failure is reported", () => { + beforeEach(() => mockDefaults()) + + it("releases the staged file and its own staging directory before throwing", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + let callCount = 0 + vi.mocked(fs.rename).mockImplementation(async () => { + callCount++ + if (callCount === 1) return // target -> backup + if (callCount === 2) throw new Error("ENOSPC") // temp -> target fails + throw new Error("EACCES") // the rollback rename fails too + }) + + await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux" })).rejects.toThrow( + RollbackFailureError, + ) + + // The backup is what the caller can still recover, so it stays on disk; the + // staging file and this write's own directory must not leak alongside it. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + const stagingDirs = vi.mocked(fsSync.mkdirSync).mock.calls.map((call) => String(call[0])) + expect(stagingDirs.length).toBe(1) + expect(fs.rmdir).toHaveBeenCalledWith(stagingDirs[0]) + + const failingRenameOrder = vi.mocked(fs.rename).mock.invocationCallOrder[2] + const unlinkOrder = vi.mocked(fs.unlink).mock.invocationCallOrder[0] + const rmdirOrder = vi.mocked(fs.rmdir).mock.invocationCallOrder[0] + expect(unlinkOrder).toBeGreaterThan(failingRenameOrder) + expect(rmdirOrder).toBeGreaterThan(failingRenameOrder) + }) +}) + +describe("resolvePublishTarget", () => { + beforeEach(() => mockDefaults()) + + it("propagates an lstat failure that is not ENOENT instead of falling back to the link path", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + const eacces = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + vi.mocked(fs.realpath).mockRejectedValue(enoent) + vi.mocked(fs.lstat).mockRejectedValue(eacces) + + // A failed lstat says nothing about whether the path is a link, so the + // fallback would publish through a link we were not allowed to inspect. + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toBe(eacces) + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("still falls back to the given path when lstat also reports the path as absent", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + vi.mocked(fs.realpath).mockRejectedValue(enoent) + vi.mocked(fs.lstat).mockRejectedValue(enoent) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // The fallback is the resolved path, not the string that was handed in. + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), path.resolve(targetPath)) + }) +}) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts new file mode 100644 index 0000000000..e548e5c3b5 --- /dev/null +++ b/src/services/file-safety/safeWriteText.ts @@ -0,0 +1,503 @@ +import * as fs from "fs/promises" +import * as fsSync from "fs" +import * as path from "path" +import { execFile } from "child_process" + +export interface SafeWriteTextOptions { + /** + * When true, preserve the old-file semantics: rename target -> backup first, + * after commit rename delete the backup; on failure roll the backup back to + * the target path. When false (default) the atomic rename simply replaces + * the target -- crash-safe window is zero. + */ + backup?: boolean + + /** + * Platform override for testing. When omitted the real process.platform + * value is used. Set to "win32" or "linux" / "darwin" from tests so that + * both branches are reachable without needing a real Windows runner. + */ + platform?: string + + /** + * Custom execFile runner for testing (e.g. vi.fn). When omitted the real + * child_process.execFile is used. + */ + execFileRunner?: typeof execFile + + /** + * Pre-written temp path to use for the commit phase. When provided, + * safeWriteText skips creating its own staging file and uses this path + * instead (it still fsyncs before rename). Useful when a caller has + * already written data to a temp file via a custom stream. + */ + tempPath?: string +} + +/** + * A publish that failed and whose rollback also failed: the content survives only + * at the backup path, not at the canonical target. The publish failure stays the + * cause, and the rollback failure plus the backup location travel with the error so + * the caller can tell what it is looking at. + */ +export class RollbackFailureError extends Error { + readonly publishError: unknown + readonly rollbackError: unknown + readonly backupPath: string + + constructor(publishError: unknown, rollbackError: unknown, backupPath: string) { + super( + `Publish failed (${publishError instanceof Error ? publishError.message : String(publishError)}) and the backup could not be restored to its original path -- the content is preserved at the backup location reported on this error.`, + { cause: publishError }, + ) + this.name = "RollbackFailureError" + this.publishError = publishError + this.rollbackError = rollbackError + this.backupPath = backupPath + } +} + +/** + * A caller-supplied staging path that is not a file this write may publish: it + * sits outside the target's directory (so the commit rename would cross + * filesystems) or is not a regular file. Rejecting it before any write keeps the + * target from being replaced by whatever the path points at. + */ +export class StagingPathError extends Error { + readonly stagingPath: string + + constructor(message: string, stagingPath: string) { + super(message) + this.name = "StagingPathError" + this.stagingPath = stagingPath + } +} + +/** + * The commit rename succeeded but the parent-directory fsync did not, so the + * directory entry is not known to be durable. The content is at the target; the + * caller cannot assume it survives a crash. Reported as its own error so a + * successful return never claims durability the filesystem did not grant. + */ +export class PostCommitDurabilityError extends Error { + readonly targetPath: string + + constructor(targetPath: string, cause: unknown) { + super( + "The rename committed but the parent directory could not be fsynced -- the content is at the target path reported on this error, and the directory entry may not be durable.", + { cause }, + ) + this.name = "PostCommitDurabilityError" + this.targetPath = targetPath + } +} +// -- 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 per-write staging sub-directory inside *dir*. The name is + * unique per write, so concurrent writes never collide on their temp names and + * never remove a staging directory another write is still using: with one shared + * name, one write's best-effort rmdir could delete the directory another write + * had just created but not yet opened, failing its openSync with ENOENT. */ +function _stagingDir(dir: string): string { + const sd = path.join(dir, ".file-safety-staging_" + Date.now() + "_" + Math.random().toString(36).substring(2)) + // 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 +} + +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 ------------------------------------------------------------ + +/** + * 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(async (error: unknown) => { + if (errorCode(error) !== "ENOENT") throw error + // ENOENT also covers a dangling symlink, which must never be written through. + // Only a lstat that also reports the path as absent may fall back to the + // given path; a real lstat failure (EACCES, EIO) says nothing about whether + // the path is a link, so falling back would write through a link we were + // simply not allowed to inspect. + const linkStat = await fs.lstat(absoluteFilePath).catch((lstatError: unknown) => { + if (errorCode(lstatError) === "ENOENT") return undefined + throw lstatError + }) + if (linkStat?.isSymbolicLink()) throw error + return absoluteFilePath + }) +} +/** + * Distinguish "the target does not exist" from a real I/O failure (EACCES, + * EIO, ...). The mode-preservation path may only fall back to the fresh-file + * default on ENOENT; any other failure is propagated, otherwise a restrictive + * target (0o600) would be published with the default 0o644 through the rename. + */ +function errorCode(error: unknown): string | undefined { + return typeof error === "object" && error !== null && "code" in error + ? String((error as { code: unknown }).code) + : undefined +} + +/** + * Canonicalize the parent directory and re-join the basename. fs.realpath + * canonicalizes every component, including a symlinked ancestor directory or a + * Windows 8.3 short name, so a lock key must be canonical even when the file + * itself is not there yet -- otherwise the key for one file depends on whether + * the file exists when the key is computed, and two writers take two locks. + */ + +async function canonicalDirKey(absoluteFilePath: string): Promise { + const dirPath = path.dirname(absoluteFilePath) + const canonicalDir = await fs.realpath(dirPath).catch(() => dirPath) + return path.join(canonicalDir, path.basename(absoluteFilePath)) +} + +/** + * Lock key for a publish target: the symlink referent when the path is an + * existing symlink, the path itself otherwise. Unlike resolvePublishTarget this + * tolerates a dangling link, because the lock key has to be computable while a + * peer writer is mid-commit (backup mode renames the referent away and back). + * The walk is bounded so a two-link cycle terminates, and every key it returns is + * canonicalized through canonicalDirKey. + */ +export async function resolveLockKey(absoluteFilePath: string): Promise { + try { + return await canonicalDirKey(await resolvePublishTarget(absoluteFilePath)) + } catch { + // A real readlink throws for anything that is not a link, so a normal chain + // ends the walk. Two links that point at each other never would, so the + // walk is bounded and callers use the key they actually reached. + let key = absoluteFilePath + for (let depth = 0; depth < 8; depth++) { + const target = await fs.readlink(key).catch(() => undefined) + if (target === undefined) return await canonicalDirKey(key) + key = await canonicalDirKey(path.resolve(path.dirname(key), target)) + } + return await canonicalDirKey(key) + } +} + +export async function safeWriteText( + filePath: string, + content: string | Uint8Array, + 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. Track the directory this + // write created so its cleanup removes its own directory, not a shared one. + let stagingDir: string | null = null + let tempPath: string + if (options?.tempPath) { + // A caller-supplied staging file is only safe when it is the file this + // write is staging, not an arbitrary path. Two properties are checked: + // it must sit beside the resolved target (a rename across filesystems + // fails with EXDEV, and a path elsewhere lets a caller publish an + // unrelated file onto the target), and it must be a regular file rather + // than a link — renaming a link over the target publishes whatever the + // link points at, which is the same trust problem as writing through a + // dangling symlink in resolvePublishTarget. + const supplied = path.resolve(options.tempPath) + if (path.dirname(supplied) !== path.resolve(dirPath)) { + throw new StagingPathError( + `Staging file must sit in the target's directory (${dirPath}), got ${supplied}`, + supplied, + ) + } + const stagingStat = await fs.lstat(supplied) + if (stagingStat.isSymbolicLink() || !stagingStat.isFile()) { + throw new StagingPathError( + `Staging file must be a regular file, not ${stagingStat.isSymbolicLink() ? "a symlink" : "another file type"}`, + supplied, + ) + } + // The caller's own path is used as given; only the check is canonical. + tempPath = options.tempPath + } else { + stagingDir = _stagingDir(dirPath) + tempPath = _tempName(stagingDir, "safeWriteText") + } + + let backupPath: string | null = null + let releaseBackupOnSuccess = false + // Set once the commit rename has published the new content. After that point the + // backup is no longer a safe restore source: rolling it back would overwrite + // content the caller can already observe at the target path. + let committed = false + // Non-null only when the win32 step-2 block saved a successful DACL dump: + // it gates the step-5 restore and is tracked for the cleanup unlinks. + let daclDumpPath: string | null = null + // Set when the rollback itself fails, so cleanup runs before the error that + // reports the partial state is thrown. + // Held as a pair so the reported error still names the path the content survived at; + // declaring it as `unknown` alone would lose the string narrowing at the throw site. + let rollbackFailure: { error: unknown; backupPath: string } | null = null + + 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). + // Encode before opening the staging file: an encoding Node cannot + // represent must not leave a half-written temp file behind. + // A string is encoded as UTF-8; bytes handed in by the caller (the + // extension host encodes a document with VS Code's own codec, which + // covers the legacy code pages Node cannot represent) are published + // unchanged. + const buffer = Buffer.from(content) + let targetMode = 0o644 // default for a fresh target + let targetExists = false + try { + targetMode = fsSync.statSync(targetPath).mode & 0o777 + targetExists = true + } catch (error: unknown) { + if (errorCode(error) !== "ENOENT") throw error + // target does not exist yet - keep the default + } + // openSync's creation mode is narrowed by the process umask, so an + // existing 0o664 target would be published as 0o644 through the + // rename. Apply the existing target's exact mode on the fd, as the + // caller-staged branch does; a fresh target keeps the default mode. + const fd = fsSync.openSync(tempPath, "w", targetMode) + try { + if (targetExists) { + fsSync.fchmodSync(fd, targetMode) + } + // Loop until every byte is written: writeSync can report a short + // (partial) write, and publishing a truncated staging file would + // commit corrupt content. + 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 (error: unknown) { + if (errorCode(error) !== "ENOENT") throw error + // 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? + const dumpPath = _tempName(dirPath, "safeWriteText.acl") + const saved = await _saveDaclWindows(targetPath, dumpPath, options?.execFileRunner) + if (saved) { + // Only a successfully saved dump may be restored onto the + // committed file (step 5). + daclDumpPath = dumpPath + } else { + // A failed icacls may have left a partial dump behind; + // remove it now (best-effort) so no partial dump survives and + // no later step can restore from it. + await fs.unlink(dumpPath).catch(() => {}) + } + } catch { + // target does not exist or access failed — no DACL handling + daclDumpPath = null + } + } + + try { + // -- Step 3 (backup:true): rename target -> backup -------------- + if (options?.backup) { + try { + await fs.access(targetPath) + backupPath = _tempName(dirPath, "safeWriteText.bak") + await fs.rename(targetPath, backupPath) + releaseBackupOnSuccess = true + } catch (err: unknown) { + if (errorCode(err) !== "ENOENT") throw err + } + } + + // -- Step 4: atomic rename temp -> target --------------------- + await fs.rename(tempPath, targetPath) + committed = true + + // -- 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 (error: unknown) { + // The content rename committed, but the directory entry that + // points at it is not known to be durable. Reporting success + // here would let a caller believe the write survives a crash, + // so the failure is surfaced as its own error: the caller can + // still find the content at the target, it just cannot rely on + // the directory entry having reached the disk. + throw new PostCommitDurabilityError(targetPath, error) + } + } + + // -- Step 5 (win32): restore DACL AFTER commit rename --------- + // daclDumpPath is non-null only when the win32 step-2 block saved a + // successful dump, so this gate is closed on every other platform + // and on every failed save. + if (daclDumpPath !== null) { + const restoredDir = path.dirname(targetPath) + await _restoreDaclWindows(restoredDir, daclDumpPath, options?.execFileRunner) + } + + // -- Step 6 (backup:true): delete backup on success ----------- + if (releaseBackupOnSuccess && backupPath) { + try { + await fs.unlink(backupPath) + } catch { + // non-fatal — orphaned backup is acceptable + } + } + } finally { + // Unlink DACL dump regardless of success/failure in this span. + if (daclDumpPath !== null) { + await fs.unlink(daclDumpPath).catch(() => {}) + } + } + + // tempPath is now the committed file; no cleanup needed. + + // Best-effort: remove the now-empty staging directory. Self-staged + // writes only, and only this write's own directory: a per-write directory + // cannot be the one another concurrent write is still using. A failure must + // never un-commit a published file, so the removal swallows all errors. + if (stagingDir) { + await fs.rmdir(stagingDir).catch(() => {}) + } + } catch (originalError: unknown) { + // Only a pre-commit failure can restore the backup. Once the commit rename + // published, a later failure (for example the post-commit directory fsync) + // must not overwrite the published content with the old file. + if (backupPath && releaseBackupOnSuccess && !committed) { + try { + await fs.rename(backupPath, targetPath) + } catch (rollbackError: unknown) { + // The content survives only at the backup path now, and the canonical + // target is gone. Reporting just the publish failure would leave the + // caller with data it cannot find at the expected path, so the + // partial-failure state travels with the error. The staged temp file + // and this write's staging directory are released first: a rollback + // failure is already a hard enough state to reason about without also + // leaking the staging file. + rollbackFailure = { error: rollbackError, backupPath } + } + } + try { + await fs.unlink(tempPath).catch(() => {}) + } catch { + // cleanup failure is non-fatal + } + + // A failed self-staged write must not leave its staging directory behind. + // Only the directory this write created, and only after its temp file is + // gone, so the directory is empty and the removal stays best-effort. + if (stagingDir) { + await fs.rmdir(stagingDir).catch(() => {}) + } + + if (daclDumpPath !== null) { + await fs.unlink(daclDumpPath).catch(() => {}) + } + + if (rollbackFailure) { + throw new RollbackFailureError(originalError, rollbackFailure.error, rollbackFailure.backupPath) + } + + throw originalError + } +} diff --git a/src/utils/__tests__/safeWriteJson.lockKey.spec.ts b/src/utils/__tests__/safeWriteJson.lockKey.spec.ts new file mode 100644 index 0000000000..33989f8b81 --- /dev/null +++ b/src/utils/__tests__/safeWriteJson.lockKey.spec.ts @@ -0,0 +1,183 @@ +// npx vitest run utils/__tests__/safeWriteJson.lockKey.spec.ts + +import * as os from "os" +import path from "path" +import type { BigIntStats } from "fs" +import * as fs from "fs/promises" +import { acquireFileLock } from "../fileLock" +import { safeWriteJson } from "../safeWriteJson" +import { resolveLockKey } from "../../services/file-safety/safeWriteText" + +vi.mock("../fileLock", () => ({ + acquireFileLock: vi.fn(async () => async () => {}), +})) + +vi.mock("fs/promises", async () => { + const actual = await vi.importActual("fs/promises") + return { ...actual, realpath: vi.fn(), lstat: vi.fn(), readlink: vi.fn() } +}) + +const mockedRealpath = vi.mocked(fs.realpath) +const mockedLstat = vi.mocked(fs.lstat) +const mockedReadlink = vi.mocked(fs.readlink) +const mockedAcquireFileLock = vi.mocked(acquireFileLock) + +const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + +// Each test creates a real temp directory so the real fs calls still work. +// doubles between tests so an implementation from one test cannot carry over. +const createdDirs: string[] = [] +async function makeDir(prefix: string): Promise { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), prefix)) + createdDirs.push(dir) + return dir +} + +beforeEach(() => { + mockedRealpath.mockReset() + mockedLstat.mockReset() + mockedReadlink.mockReset() + mockedAcquireFileLock.mockReset() +}) + +afterEach(async () => { + for (const dir of createdDirs) { + await fs.rm(dir, { recursive: true, force: true }).catch(() => undefined) + } + createdDirs.length = 0 +}) + +// Only isSymbolicLink() is consulted by the guard, so the double carries just +// that method. The mocks reject asynchronously: a synchronous throw would bypass +// resolvePublishTarget's catch and skip the ENOENT/symlink branch under test. +const symlinkStat = (target: unknown) => ({ + isSymbolicLink: () => target === currentLink, + // The staging-path check in safeWriteText also asks whether the path is a + // regular file, so the double carries that predicate as well. + isFile: () => target !== currentLink, +}) as unknown as BigIntStats +let currentLink = "" + +describe("safeWriteJson lock key under a peer commit", () => { + it("waits for the peer instead of rejecting, and locks the referent", async () => { + const order: string[] = [] + const dir = await makeDir("lockkey-") + const referent = path.join(dir, "history_item.json") + currentLink = path.join(dir, "link.json") + + // The peer writer has renamed the referent away and has not committed yet, + // so the first resolution fails with ENOENT while lstat still reports a + // symbolic link. A strict resolve here rejects the caller before it can ever + // queue behind the peer, and the caller's delta write is lost. + mockedRealpath + .mockImplementationOnce(async () => { + order.push("resolve-failed") + throw enoent + }) + .mockImplementation(async (target) => { + order.push("resolve") + // The second call happens under the lock, where the peer has committed. + return target === currentLink ? referent : String(target) + }) + mockedLstat.mockImplementation(async (target) => { + order.push("lstat") + return symlinkStat(target) + }) + mockedReadlink.mockImplementation(async (target) => + target === currentLink ? referent : Promise.reject(new Error("not a link")), + ) + mockedAcquireFileLock.mockImplementation(async () => { + order.push("lock") + return async () => {} + }) + + await safeWriteJson(currentLink, { id: "task-1" }) + + // The lock key is the key every other writer to this file uses, so the caller + // queued behind the peer instead of failing before the lock. + expect(mockedAcquireFileLock).toHaveBeenCalledWith(referent) + // The trailing lstat is safeWriteText's staging-path check on the temp file + // this write created: it runs after the key was resolved and the lock taken, + // so it does not change which lock the caller queued behind. + expect(order).toEqual(["resolve-failed", "lstat", "resolve", "resolve", "lock", "resolve", "resolve", "lstat"]) + expect(JSON.parse(await fs.readFile(referent, "utf8"))).toEqual({ id: "task-1" }) + }) + + it("releases the lock when the resolution under the lock rejects", async () => { + const order: string[] = [] + let released = false + const dir = await makeDir("lockkey-") + const referent = path.join(dir, "history_item.json") + currentLink = path.join(dir, "link.json") + + // A real dangling link: the walk tolerates it so the caller can queue behind + // the peer, but once the lock is held the strict rejection still applies. A + // rejection outside the protected block would leave the lock held until the + // stale timeout for every other writer to the same file. + mockedRealpath.mockImplementation(async () => { + throw enoent + }) + mockedLstat.mockImplementation(async (target) => { + order.push("lstat") + return symlinkStat(target) + }) + mockedReadlink.mockImplementation(async (target) => + target === currentLink ? referent : Promise.reject(new Error("not a link")), + ) + mockedAcquireFileLock.mockImplementation(async () => { + order.push("lock") + return async () => { + order.push("release") + released = true + } + }) + + await expect(safeWriteJson(currentLink, { id: "task-1" })).rejects.toThrow(enoent) + expect(released).toBe(true) + // The strict rejection is reached through the ENOENT + symlink branch, not + // through a synchronous throw that skips it. + expect(order).toEqual(["lstat", "lock", "lstat", "release"]) + }) + + it("canonicalizes the parent directory when the file itself is not there yet", async () => { + // fs.realpath canonicalizes every component, including a symlinked ancestor + // directory or a Windows 8.3 short name. If the fallback returns the alias + // directory, the key depends on whether the file exists at the moment the key + // is computed, and a writer that resolved the canonical directory takes a + // different lock for the same file. + const aliasDir = path.join(os.tmpdir(), "alias-dir") + const canonicalDir = path.join(os.tmpdir(), "canonical-dir") + const file = path.join(aliasDir, "history_item.json") + mockedRealpath.mockImplementation(async (target) => { + if (target === file) throw enoent + return canonicalDir + }) + mockedLstat.mockImplementation(async () => ({ isSymbolicLink: () => false, isFile: () => true }) as unknown as BigIntStats) + + expect(await resolveLockKey(file)).toBe(path.join(canonicalDir, "history_item.json")) + }) +}) + +it("does not log a cleanup error when the safety net finds the temp file already gone", async () => { + // safeWriteText removes its own temp file on failure, so the safety net in + // safeWriteJson normally finds it gone. That is the expected outcome, not a + // second failure, and it must not be logged as one. + const dir = await makeDir("cleanup-") + const target = path.join(dir, "history_item.json") + currentLink = "" + mockedRealpath.mockImplementation(async (t) => String(t)) + mockedLstat.mockImplementation(async (t) => symlinkStat(t)) + + const renameSpy = vi.spyOn(fs, "rename").mockRejectedValue(new Error("commit rename failed")) + const unlinkSpy = vi.spyOn(fs, "unlink").mockRejectedValue(enoent) + const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}) + + await expect(safeWriteJson(target, { id: "task-1" })).rejects.toThrow("commit rename failed") + + // Only the original failure is reported. + expect(consoleError).toHaveBeenCalledTimes(1) + + renameSpy.mockRestore() + unlinkSpy.mockRestore() + consoleError.mockRestore() +}) diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 79d08678a0..245af61910 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -4,6 +4,8 @@ import * as path from "path" import * as os from "os" import { safeWriteJson } from "../safeWriteJson" +import { RollbackFailureError } from "../../services/file-safety/safeWriteText" +import * as lockfile from "proper-lockfile" // Capture actual implementations before the vi.mock factory runs, // so they are never wrapped by vi.fn() — avoids infinite recursion when @@ -312,9 +314,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 +323,23 @@ describe("safeWriteJson", () => { // fs.unlink is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn vi.mocked(fs.unlink).mockImplementation(async (filePath: any) => { - if (filePath.toString().includes(".bak_")) { + if (filePath.toString().includes("safeWriteText.bak_")) { throw new Error("Backup deletion failed") } return fsPromisesActuals.unlink!(filePath) }) + // The write must still succeed: backup cleanup is best-effort inside + // safeWriteText and never masks the committed content. await safeWriteJson(currentTestFilePath, newData) - // Verify console.error was called with the expected message - expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining("Successfully wrote"), expect.any(Error)) + const content = await readFileContent(currentTestFilePath) + expect(content).toEqual(newData) + + // The orphaned backup is still on disk because its deletion failed. + const entries = await fs.readdir(tempDir) + expect(entries.some((entry) => entry.includes("safeWriteText.bak_"))).toBe(true) - consoleErrorSpy.mockRestore() vi.mocked(fs.unlink).mockRestore() }) @@ -434,9 +440,9 @@ describe("safeWriteJson", () => { expect(vi.mocked(fs.access)).toHaveBeenCalled() }) - // Test for rollback failure scenario - test("should log error and re-throw original if rollback fails", async () => { - const initialData = { message: "Initial, should be lost if rollback fails" } + // Test for rollback failure scenario (the rollback rename now lives in safeWriteText) + test("re-throws the original error when the rollback rename fails, leaving an orphaned backup", async () => { + const initialData = { message: "Initial, orphaned when rollback fails" } const newData = { message: "New content" } await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) @@ -451,20 +457,34 @@ describe("safeWriteJson", () => { // Second call: tempNewFilePath -> filePath (fail) throw new Error("Primary rename failed") } else if (renameCallCount === 3) { - // Third call: tempBackupFilePath -> filePath (rollback, also fail) + // Third call: backup -> filePath (rollback, also fail) throw new Error("Rollback rename failed") } return fsPromisesActuals.rename!(oldPath, newPath) }) - // Should throw the original error, not the rollback error - await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow("Primary rename failed") + // The original error must propagate, not the rollback error + // The rollback also failed, so the error reports the partial state: the publish + // failure stays the cause and the backup location is named. + let failure: RollbackFailureError | undefined + await safeWriteJson(currentTestFilePath, newData).catch((e: unknown) => { + if (e instanceof RollbackFailureError) { + failure = e + return + } + throw e + }) + + expect(failure).toBeInstanceOf(RollbackFailureError) + expect(failure?.cause).toBeInstanceOf(Error) + expect(failure?.rollbackError).toBeInstanceOf(Error) + expect(failure?.backupPath).toContain("safeWriteText.bak_") - // Verify console.error was called for the rollback failure - expect(consoleErrorSpy).toHaveBeenCalledWith( - expect.stringContaining("Failed to restore backup"), - expect.objectContaining({ message: "Rollback rename failed" }), - ) + // The rollback failed inside safeWriteText, so the target is gone and + // the backup is orphaned on disk. + expect(await fileExists(currentTestFilePath)).toBe(false) + const entries = await fs.readdir(tempDir) + expect(entries.some((entry) => entry.includes("safeWriteText.bak_"))).toBe(true) consoleErrorSpy.mockRestore() }) @@ -542,4 +562,144 @@ 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 })) + + // Only the file resolves through the link; the directory is already canonical, + // so the lock key is the referent rather than the alias directory + basename. + vi.spyOn(fs, "realpath").mockImplementation(async (target) => + target === callerPath ? referentPath : String(target), + ) + + 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 })) + + // Only the file resolves through the link; the directory is already canonical, + // so the lock key is the referent rather than the alias directory + basename. + const realpathSpy = vi + .spyOn(fs, "realpath") + .mockImplementation(async (target) => (target === callerPath ? referentPath : String(target))) + + // Wrap the real lock in a capturing mock, and drive the two rare error paths + // (the onCompromised callback and a failing release) so they stay covered + // without real lockfile staleness. The callback rethrows by design, so + // the mock swallows that throw and lets the real lock proceed. + const realLockfile = await vi.importActual("proper-lockfile") + const lockMockFn = vi.fn( + async ( + file: Parameters[0], + options?: Parameters[1], + ) => { + try { + options?.onCompromised?.(new Error("lock compromised (test)")) + } catch { + // onCompromised rethrows by design; swallow so the real lock proceeds. + } + const release = await realLockfile.lock(file, options) + return async () => { + await release() + throw new Error("release failed (test)") + } + }, + ) + const lockMock = lockMockFn as unknown as typeof realLockfile.lock + vi.doMock("proper-lockfile", () => ({ + ...realLockfile, + lock: lockMock, + })) + + // Re-import safeWriteJson so it picks up the mocked proper-lockfile. + const { safeWriteJson: mockedSafeWriteJson } = await import("../safeWriteJson") + + const mergeFn = vi.fn((existing: unknown, incoming: unknown) => ({ + ...(existing as Record), + ...(incoming as Record), + })) + + // Capture the compromise + release-failure logs. + const consoleErrorSpy = vi.spyOn(console, "error") + try { + 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 { + // Cleanup must run even when an assertion fails: a leaked mock + // registration or console spy changes later tests, and vi.unmock + // alone does not reset a module that already imported the mock. + realpathSpy.mockRestore() + vi.unmock("proper-lockfile") + vi.resetModules() + consoleErrorSpy.mockRestore() + } + }) + + // 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..bae200dd38 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -4,6 +4,12 @@ import * as path from "path" import { JsonStreamStringify } from "json-stream-stringify" import { acquireFileLock } from "./fileLock" +import { + resolveLockKey, + resolvePublishTarget, + safeWriteText, + type SafeWriteTextOptions, +} from "../services/file-safety/safeWriteText" /** * Options for safeWriteJson function @@ -32,7 +38,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 +48,6 @@ export interface SafeWriteJsonOptions { * @param {SafeWriteJsonOptions} options - Optional configuration for JSON formatting. * @returns {Promise} */ - async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJsonOptions): Promise { const absoluteFilePath = path.resolve(filePath) let releaseLock = async () => {} // Initialized to a no-op @@ -51,38 +56,46 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso const dirPath = path.dirname(absoluteFilePath) // Ensure directory structure exists with improved reliability + // Declared outside the protected block so the catch and finally can still name + // the target when the resolution itself rejects. + let resolvedTargetPath: string | undefined + 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 } - // 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) + // Lock key: the symlink referent when the path is an existing symlink, so a + // symlink alias and its referent share one lock. The key must be computable + // while a peer writer is mid-commit (backup mode renames the referent away and + // back), so the walk tolerates a dangling link instead of rejecting it here. + const lockKey = await resolveLockKey(absoluteFilePath) - // Variables to hold the actual paths of temp files if they are created. + // Acquire the lock before any file operations. If acquisition fails it throws + // immediately, and releaseLock stays a no-op so the finally block does not try + // to release an unacquired lock. + releaseLock = await acquireFileLock(lockKey) + + // 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 { + // Resolve the publish target under the lock: the peer has committed by now, so + // the strict dangling-link rejection still applies to a real dangling link. It + // must stay inside the protected block, otherwise a rejection here leaves the + // advisory lock held until the stale timeout for every other writer. + resolvedTargetPath = await resolvePublishTarget(absoluteFilePath) + // If a merge callback was provided, read the current file under the lock // and let the caller merge before we write. Must be inside try/finally // so a throwing merge still releases the lock. 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,111 +106,73 @@ 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 (target -> backup before commit, rollback on failure) and + // keeps the target in place until safeWriteText captures its Windows + // DACL (safeWriteText dumps the DACL before its own backup rename and + // restores it onto the directory after the commit rename). + const textOptions: SafeWriteTextOptions = { + tempPath: actualTempNewFilePath, + backup: true, } - // Step 3: Rename the new temporary file to the target file path. - // This is the main "commit" step. - await fs.rename(actualTempNewFilePath, absoluteFilePath) + await safeWriteText(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 ?? absoluteFilePath}: [Original Error Caught]`, + originalError, + ) const newFileToCleanupWithinCatch = actualTempNewFilePath - const backupFileToRollbackOrCleanupWithinCatch = actualTempBackupFilePath - - // Attempt rollback if a backup was made - if (backupFileToRollbackOrCleanupWithinCatch) { - try { - await fs.rename(backupFileToRollbackOrCleanupWithinCatch, absoluteFilePath) - // Mark as handled, prevent later unlink of this path - actualTempBackupFilePath = null - } catch (rollbackError) { - // actualTempBackupFilePath (outer scope) remains pointing to backupFileToRollbackOrCleanupWithinCatch - console.error( - `[Catch] Failed to restore backup ${backupFileToRollbackOrCleanupWithinCatch} to ${absoluteFilePath}:`, - rollbackError, - ) - } - } - // Cleanup the .new file if it exists + // A failed safeWriteText already rolled the backup (if any) back to + // the target path. Clean up the .new file if it still exists + // (safeWriteText also cleans up its tempPath on failure; this is a + // safety net in case its cleanup missed it). if (newFileToCleanupWithinCatch) { try { await fs.unlink(newFileToCleanupWithinCatch) - } catch (cleanupError) { - console.error( - `[Catch] Failed to clean up temporary new file ${newFileToCleanupWithinCatch}:`, - cleanupError, - ) + } catch (cleanupError: unknown) { + // The expected case: safeWriteText already removed its own temp file, so a + // missing file here is not a cleanup failure worth logging. Returning would + // also swallow the original error the caller needs. + const isAbsent = + typeof cleanupError === "object" && + cleanupError !== null && + "code" in cleanupError && + cleanupError.code === "ENOENT" + if (!isAbsent) { + console.error( + `[Catch] Failed to clean up temporary new file ${newFileToCleanupWithinCatch}:`, + cleanupError, + ) + } } } - // Cleanup the .bak file if it still needs to be (i.e., wasn't successfully restored) - if (actualTempBackupFilePath) { - try { - await fs.unlink(actualTempBackupFilePath) - } catch (cleanupError) { - console.error( - `[Catch] Failed to clean up temporary backup file ${actualTempBackupFilePath}:`, - cleanupError, - ) - } - } throw originalError // This MUST be the error that rejects the promise. } finally { // Release the lock in the main finally block. try { - // releaseLock will be the actual unlock function if lock was acquired, - // or the initial no-op if acquisition failed. await releaseLock() } catch (unlockError) { - // Do not re-throw here, as the originalError from the try/catch (if any) is more important. - console.error(`Failed to release lock for ${absoluteFilePath}:`, unlockError) + console.error(`Failed to release lock for ${resolvedTargetPath ?? absoluteFilePath}:`, unlockError) } } }