diff --git a/src/core/tools/ReadFileTool.ts b/src/core/tools/ReadFileTool.ts index e4e69b30ad..f80212eda8 100644 --- a/src/core/tools/ReadFileTool.ts +++ b/src/core/tools/ReadFileTool.ts @@ -17,13 +17,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 { @@ -215,14 +216,49 @@ 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. + // A task that was cancelled or disposed while this read was awaiting I/O must + // not record an observation: disposeOnce() has retired the registry, and the + // content this turn produced will never be acted on. Skipping also drops the + // post-read stat work for a task that no longer has a consumer. + if (!task.abort) { + const postReadStats = await fs.stat(fullPath, { bigint: true }).catch(() => undefined) + // Re-checked after the await: the guard above cannot see a cancellation + // that lands while the stat is in flight, and that read belongs to a run + // that will never act on it. + if (preReadStats && postReadStats && !task.abort) { + 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) @@ -266,8 +302,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") { @@ -300,7 +342,15 @@ export class ReadFileTool extends BaseTool<"read_file"> { output += `\n\nIncluded ranges: ${rangeStr} (total: ${result.totalLines} lines)` } - return output + if (result.hasClippedLines) { + // A returned line can still be a partial view: clipping is reported + // separately from the omission notice, because the block may show + // every line it includes in truncated form. + output += `\nNote: Some lines in this view exceed ${MAX_LINE_LENGTH} characters and were clipped in this view.` + } + + // Indentation mode returns semantic blocks: never a complete file view. + return { content: output, complete: false } } // Slice mode (default): simple offset/limit reading @@ -323,11 +373,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 && result.totalLines === 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 } } /** @@ -770,9 +836,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") @@ -792,8 +869,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]` } } @@ -801,6 +886,24 @@ 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. + // Same contract as the native path: an aborted or disposed task records nothing. + if (!task.abort) { + const postReadStats = await fs.stat(fullPath, { bigint: true }).catch(() => undefined) + // Re-checked after the await, as on the native path: a cancel landing + // during the stat must still record nothing. + if (preReadStats && postReadStats && !task.abort) { + 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 52e04415ea..b54d9ccbb0 100644 --- a/src/core/tools/__tests__/readFileTool.spec.ts +++ b/src/core/tools/__tests__/readFileTool.spec.ts @@ -14,13 +14,18 @@ import os from "os" import path from "path" +import type { Stats } from "fs" + +import type { LegacyReadFileParams } from "@roo-code/types" import type { ToolUse } from "../../../shared/tools" import type { Task, ToolExecutionContext } from "../../task/Task" import { isBinaryFile } from "isbinaryfile" +import { MAX_LINE_LENGTH } from "../../prompts/tools/native-tools/read_file" import { readFileTool } from "../ReadFileTool" +import { ObservationRegistry } from "../../task/observationRegistry" import { formatResponse } from "../../prompts/responses" import { validateImageForProcessing, processImageFile, isSupportedImageFormat } from "../helpers/imageHelpers" import { extractTextFromFile, getSupportedBinaryFormats } from "../../../integrations/misc/extract-text" @@ -135,13 +140,26 @@ interface MockTaskOptions { rooIgnoreAllowed?: boolean maxImageFileSize?: number maxTotalImageSize?: number + observationRegistry?: ObservationRegistry + aborted?: boolean } function createMockTask(options: MockTaskOptions = {}) { - const { supportsImages = false, rooIgnoreAllowed = true, maxImageFileSize = 5, maxTotalImageSize = 20 } = options + const { + supportsImages = false, + rooIgnoreAllowed = true, + maxImageFileSize = 5, + maxTotalImageSize = 20, + aborted = false, + } = options return { cwd: "/test/workspace", + // Mirror Task.abort: disposal and cancellation both set it before the loop resumes. + abort: aborted, + // Mirror Task: every task always owns an observation registry (A2, #1375). + // Tests asserting on observations pass their own instance via options. + observationRegistry: options.observationRegistry ?? new ObservationRegistry(), api: { getModel: vi.fn().mockReturnValue({ info: { supportsImages }, @@ -188,7 +206,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({ @@ -975,7 +1004,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, @@ -1347,6 +1376,64 @@ describe("ReadFileTool", () => { expect(callbacks.pushToolResult).toHaveBeenCalledWith(expect.stringContaining("To read more")) }) + it("should show a clipping notice in indentation mode separately from the omission notice", async () => { + const mockTask = createMockTask() + const callbacks = createMockCallbacks() + + mockedFsReadFile.mockResolvedValue(Buffer.from("content")) + mockedReadWithIndentation.mockReturnValue({ + content: "1 | block", + returnedLines: 10, + totalLines: 100, + wasTruncated: false, + hasClippedLines: true, + includedRanges: [[1, 10]], + }) + + await readFileTool.execute( + { + path: "test.ts", + mode: "indentation", + indentation: { anchor_line: 1 }, + }, + mockTask as unknown as Task, + callbacks, + ) + + const pushed = callbacks.pushToolResult.mock.calls[0][0] as string + expect(pushed).toContain("clipped in this view") + expect(pushed).not.toContain("File content truncated") + }) + + it("should report clipping alongside truncation in indentation mode", async () => { + const mockTask = createMockTask() + const callbacks = createMockCallbacks() + + mockedFsReadFile.mockResolvedValue(Buffer.from("content")) + mockedReadWithIndentation.mockReturnValue({ + content: "1 | block", + returnedLines: 50, + totalLines: 200, + wasTruncated: true, + hasClippedLines: true, + includedRanges: [[1, 50]], + }) + + await readFileTool.execute( + { + path: "test.ts", + mode: "indentation", + indentation: { anchor_line: 1 }, + }, + mockTask as unknown as Task, + callbacks, + ) + + const pushed = callbacks.pushToolResult.mock.calls[0][0] as string + expect(pushed).toContain("File content truncated") + expect(pushed).toContain("clipped in this view") + }) + it("should include range information in indentation mode when not truncated", async () => { const mockTask = createMockTask() const callbacks = createMockCallbacks() @@ -1688,5 +1775,1363 @@ describe("ReadFileTool", () => { expect(mockTask.didToolFailInCurrentTurn).toBe(true) }) + + describe("observation registry", () => { + // Indices of fs.stat calls against a path ending in fileName, filtered by + // whether the call passed { bigint: true }. Counting matching calls is + // the load-bearing form here: toHaveBeenCalledWith asserts membership, + // which the directory-check stat alone satisfies, and it cannot tell a + // skipped stat from one that ran. + const usesBigintOptions = (options: unknown): boolean => + typeof options === "object" && options !== null && (options as { bigint?: unknown }).bigint === true + const statCallsMatching = (fileName: string, bigint: boolean): number[] => + mockedFsStat.mock.calls + .map((_, index) => index) + .filter((index) => { + const [statPath, options] = mockedFsStat.mock.calls[index] + return String(statPath).endsWith(fileName) && usesBigintOptions(options) === bigint + }) + // Vitest records invocationCallOrder parallel to mock.calls; comparing it + // against the readFile call tells the pre-read observation stat from the + // post-read one instead of trusting positional order by eye. + const statOrderBeforeRead = (statIndex: number): boolean => + mockedFsStat.mock.invocationCallOrder[statIndex] < mockedFsReadFile.mock.invocationCallOrder[0] + 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) + // Regression evidence: both observation stats must be bigint stats. + // Count the calls that match - correct path AND { bigint: true } - + // because a membership assertion passes on its own: the read path + // also stats the same path without options for the directory check. + const bigintStatIndexes = statCallsMatching("existing.ts", true) + expect(bigintStatIndexes).toHaveLength(2) + // Neither observation stat is a plain stat: the directory check stays + // the only stat without options... + expect(statCallsMatching("existing.ts", false)).toHaveLength(1) + // ...and the two bigint stats bracket the read: exactly one before it + // (pre-read observation) and exactly one after (post-read observation). + expect(bigintStatIndexes.filter((index) => statOrderBeforeRead(index))).toHaveLength(1) + expect(bigintStatIndexes.filter((index) => !statOrderBeforeRead(index))).toHaveLength(1) + }) + + 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+$/) + // Regression evidence, same contract on the legacy path: count the + // matching calls (correct path AND { bigint: true }) instead of + // asserting membership, which the directory-check stat alone satisfies. + const bigintStatIndexes = statCallsMatching("legacy.ts", true) + expect(bigintStatIndexes).toHaveLength(2) + expect(statCallsMatching("legacy.ts", false)).toHaveLength(1) + // The two bigint stats bracket the read: pre-read before it, post-read after. + expect(bigintStatIndexes.filter((index) => statOrderBeforeRead(index))).toHaveLength(1) + expect(bigintStatIndexes.filter((index) => !statOrderBeforeRead(index))).toHaveLength(1) + }) + + 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("records nothing when the task is aborted before the post-read stat (native path)", async () => { + // Task.disposeOnce() sets abort before an awaited read resumes, and abortTask() + // sets it on cancellation. The read still answers the model, but the observation - + // and the post-read stat work behind it - must not repopulate a retired registry. + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + aborted: true, + }) + 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") + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute({ path: "existing.ts" }, mockTask as unknown as Task, callbacks) + + expect(observeSpy).not.toHaveBeenCalled() + expect(reg.size).toBe(0) + expect(mockTask.didToolFailInCurrentTurn).toBe(false) + // The abort must skip only the post-read observation: the pre-read + // observation stat is still issued, and no bigint stat follows the + // read. Counted by matching calls (correct path AND { bigint: true }): + // a membership assertion cannot tell "post-stat skipped" from + // "post-stat ran". + const bigintStatIndexes = statCallsMatching("existing.ts", true) + expect(bigintStatIndexes).toHaveLength(1) + // The single bigint stat ran before the read: it is the pre-read one. + expect(statOrderBeforeRead(bigintStatIndexes[0])).toBe(true) + // The directory-check stat is untouched: still exactly one plain stat. + expect(statCallsMatching("existing.ts", false)).toHaveLength(1) + }) + + it("records nothing when the task is aborted before the post-read stat (legacy path)", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + aborted: true, + }) + 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") + + 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).not.toHaveBeenCalled() + expect(reg.size).toBe(0) + // Same contract on the legacy path: the pre-read observation stat is + // still issued and the post-read one is skipped - counted by matching + // calls, not by membership. + const bigintStatIndexes = statCallsMatching("legacy.ts", true) + expect(bigintStatIndexes).toHaveLength(1) + expect(statOrderBeforeRead(bigintStatIndexes[0])).toBe(true) + expect(statCallsMatching("legacy.ts", false)).toHaveLength(1) + }) + + it("records nothing when the task is cancelled while the post-read stat is in flight (native path)", async () => { + // The guard before the stat cannot see a cancellation that lands DURING the + // stat. The re-check before observe() is what keeps a read that belongs to a + // cancelled run out of the registry - and out of a registry that disposal has + // already retired. + const mockTask = createMockTask({ observationRegistry: new ObservationRegistry() }) + const callbacks = createMockCallbacks() + + const stats = { + 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 + mockedFsStat + .mockResolvedValueOnce({ isDirectory: () => false } as unknown as Stats) // directory check + .mockResolvedValueOnce(stats) // pre-read + .mockImplementationOnce(async () => { + // The cancel lands while this await is pending. + mockTask.abort = true + return stats + }) // post-read + 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: "existing.ts" }, mockTask as unknown as Task, callbacks) + + expect(observeSpy).not.toHaveBeenCalled() + expect(reg.size).toBe(0) + // The read itself still answered the model; only the observation is withheld. + expect(mockTask.didToolFailInCurrentTurn).toBe(false) + }) + + it("records nothing when the task is cancelled while the post-read stat is in flight (legacy path)", async () => { + const mockTask = createMockTask({ observationRegistry: new ObservationRegistry() }) + const callbacks = createMockCallbacks() + + const stats = { + 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 + mockedFsStat + .mockResolvedValueOnce({ isDirectory: () => false } as unknown as Stats) // directory check + .mockResolvedValueOnce(stats) // pre-read + .mockImplementationOnce(async () => { + // The cancel lands while this await is pending. + mockTask.abort = true + return stats + }) // post-read + mockedIsBinaryFile.mockResolvedValue(false) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + 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).not.toHaveBeenCalled() + expect(reg.size).toBe(0) + }) + + 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:") + }) + // The four stat-failure tests above start every registry empty, so a regression that + // clears or downgrades an entry the Task already had cannot fail them: on a failed stat + // the read skips observe(), and with nothing to skip onto the suite stays green. These + // seed a complete observation for the same path before the failing re-read, which is + // what pins the policy the source states - a failed stat leaves the prior entry + // untouched rather than invalidating it. + + it("keeps a prior complete observation untouched when the pre-read stat fails", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + const reg = mockTask.observationRegistry! + // Seed a complete observation for the same path the read will look up. path.resolve + // is mocked to join with "/", so this is the key the tool builds for the entry. + const seededPath = path.resolve(mockTask.cwd, "seeded-native-pre-stat-fail.ts") + reg.observe(seededPath, "1:2:3:4:5", true) + const seeded = reg.get(seededPath)! + expect(seeded.complete).toBe(true) + + // Directory check OK; the pre-read stat fails (caught, nothing observed). + mockedFsStat + .mockResolvedValueOnce({ isDirectory: () => false } as unknown as Stats) + .mockRejectedValueOnce(new Error("EACCES")) + mockedIsBinaryFile.mockResolvedValue(false) + // The fresh bytes and the fresh view: what the model receives on this re-read. + mockedFsReadFile.mockResolvedValue(Buffer.from("fresh bytes after the pre-read stat failure")) + mockedReadWithSlice.mockReturnValue({ + content: "1 | fresh view after the pre-read stat failure", + returnedLines: 1, + totalLines: 1, + wasTruncated: false, + includedRanges: [[1, 1]], + }) + + const observeSpy = vi.spyOn(reg, "observe") + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute( + { path: "seeded-native-pre-stat-fail.ts" }, + mockTask as unknown as Task, + callbacks, + ) + + // The read still succeeds with the content this re-read produced: a stat failure never + // fails the read. Assert the pushed payload, not just that something was pushed. + expect(mockTask.didToolFailInCurrentTurn).toBe(false) + const pushed = callbacks.pushToolResult.mock.calls[0][0] + expect(pushed).toContain("File: seeded-native-pre-stat-fail.ts") + expect(pushed).toContain("1 | fresh view after the pre-read stat failure") + expect(pushed).not.toContain("Error:") + // And the entry the Task already had is kept: same version token, same completeness, + // same timestamp, still the only entry. A failed stat invalidates nothing. + expect(observeSpy).not.toHaveBeenCalled() + expect(reg.has(seededPath)).toBe(true) + expect(reg.size).toBe(1) + expect(reg.get(seededPath)!.version).toBe("1:2:3:4:5") + expect(reg.get(seededPath)!.complete).toBe(true) + expect(reg.get(seededPath)!.observedAt).toBe(seeded.observedAt) + }) + + it("keeps a prior complete observation untouched when the post-read stat fails", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + const reg = mockTask.observationRegistry! + // Seed a complete observation for the same path the read will look up. path.resolve + // is mocked to join with "/", so this is the key the tool builds for the entry. + const seededPath = path.resolve(mockTask.cwd, "seeded-native-post-stat-fail.ts") + reg.observe(seededPath, "1:2:3:4:5", true) + const seeded = reg.get(seededPath)! + expect(seeded.complete).toBe(true) + + 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) + // The fresh bytes and the fresh view: what the model receives on this re-read. + mockedFsReadFile.mockResolvedValue(Buffer.from("fresh bytes after the post-read stat failure")) + mockedReadWithSlice.mockReturnValue({ + content: "1 | fresh view after the post-read stat failure", + returnedLines: 1, + totalLines: 1, + wasTruncated: false, + includedRanges: [[1, 1]], + }) + + const observeSpy = vi.spyOn(reg, "observe") + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute( + { path: "seeded-native-post-stat-fail.ts" }, + mockTask as unknown as Task, + callbacks, + ) + + // The read still succeeds with the content this re-read produced: a stat failure never + // fails the read. Assert the pushed payload, not just that something was pushed. + expect(mockTask.didToolFailInCurrentTurn).toBe(false) + const pushed = callbacks.pushToolResult.mock.calls[0][0] + expect(pushed).toContain("File: seeded-native-post-stat-fail.ts") + expect(pushed).toContain("1 | fresh view after the post-read stat failure") + expect(pushed).not.toContain("Error:") + // And the entry the Task already had is kept: same version token, same completeness, + // same timestamp, still the only entry. A failed stat invalidates nothing. + expect(observeSpy).not.toHaveBeenCalled() + expect(reg.has(seededPath)).toBe(true) + expect(reg.size).toBe(1) + expect(reg.get(seededPath)!.version).toBe("1:2:3:4:5") + expect(reg.get(seededPath)!.complete).toBe(true) + expect(reg.get(seededPath)!.observedAt).toBe(seeded.observedAt) + }) + + it("legacy format: keeps a prior complete observation untouched when the pre-read stat fails", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + const reg = mockTask.observationRegistry! + // Seed a complete observation for the same path the read will look up. path.resolve + // is mocked to join with "/", so this is the key the tool builds for the entry. + const seededPath = path.resolve(mockTask.cwd, "seeded-legacy-pre-stat-fail.ts") + reg.observe(seededPath, "1:2:3:4:5", true) + const seeded = reg.get(seededPath)! + expect(seeded.complete).toBe(true) + + // Directory check OK; the pre-read stat fails (caught, nothing observed). + mockedFsStat + .mockResolvedValueOnce({ isDirectory: () => false } as unknown as Stats) + .mockRejectedValueOnce(new Error("EACCES")) + mockedIsBinaryFile.mockResolvedValue(false) + // The fresh bytes and the fresh view: what the model receives on this re-read. + mockedFsReadFile.mockResolvedValue(Buffer.from("fresh bytes after the pre-read stat failure")) + mockedReadWithSlice.mockReturnValue({ + content: "1 | fresh view after the pre-read stat failure", + returnedLines: 1, + totalLines: 1, + wasTruncated: false, + includedRanges: [[1, 1]], + }) + + const observeSpy = vi.spyOn(reg, "observe") + + const legacyParams: LegacyReadFileParams = { + files: [{ path: "seeded-legacy-pre-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) + + // The read still succeeds with the content this re-read produced: a stat failure never + // fails the read. Assert the pushed payload, not just that something was pushed. + expect(mockTask.didToolFailInCurrentTurn).toBe(false) + const pushed = callbacks.pushToolResult.mock.calls[0][0] + expect(pushed).toContain("File: seeded-legacy-pre-stat-fail.ts") + expect(pushed).toContain("1 | fresh view after the pre-read stat failure") + expect(pushed).not.toContain("Error:") + // And the entry the Task already had is kept: same version token, same completeness, + // same timestamp, still the only entry. A failed stat invalidates nothing. + expect(observeSpy).not.toHaveBeenCalled() + expect(reg.has(seededPath)).toBe(true) + expect(reg.size).toBe(1) + expect(reg.get(seededPath)!.version).toBe("1:2:3:4:5") + expect(reg.get(seededPath)!.complete).toBe(true) + expect(reg.get(seededPath)!.observedAt).toBe(seeded.observedAt) + }) + + it("legacy format: keeps a prior complete observation untouched when the post-read stat fails", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + const reg = mockTask.observationRegistry! + // Seed a complete observation for the same path the read will look up. path.resolve + // is mocked to join with "/", so this is the key the tool builds for the entry. + const seededPath = path.resolve(mockTask.cwd, "seeded-legacy-post-stat-fail.ts") + reg.observe(seededPath, "1:2:3:4:5", true) + const seeded = reg.get(seededPath)! + expect(seeded.complete).toBe(true) + + 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) + // The fresh bytes and the fresh view: what the model receives on this re-read. + mockedFsReadFile.mockResolvedValue(Buffer.from("fresh bytes after the post-read stat failure")) + mockedReadWithSlice.mockReturnValue({ + content: "1 | fresh view after the post-read stat failure", + returnedLines: 1, + totalLines: 1, + wasTruncated: false, + includedRanges: [[1, 1]], + }) + + const observeSpy = vi.spyOn(reg, "observe") + + const legacyParams: LegacyReadFileParams = { + files: [{ path: "seeded-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) + + // The read still succeeds with the content this re-read produced: a stat failure never + // fails the read. Assert the pushed payload, not just that something was pushed. + expect(mockTask.didToolFailInCurrentTurn).toBe(false) + const pushed = callbacks.pushToolResult.mock.calls[0][0] + expect(pushed).toContain("File: seeded-legacy-post-stat-fail.ts") + expect(pushed).toContain("1 | fresh view after the post-read stat failure") + expect(pushed).not.toContain("Error:") + // And the entry the Task already had is kept: same version token, same completeness, + // same timestamp, still the only entry. A failed stat invalidates nothing. + expect(observeSpy).not.toHaveBeenCalled() + expect(reg.has(seededPath)).toBe(true) + expect(reg.size).toBe(1) + expect(reg.get(seededPath)!.version).toBe("1:2:3:4:5") + expect(reg.get(seededPath)!.complete).toBe(true) + expect(reg.get(seededPath)!.observedAt).toBe(seeded.observedAt) + }) + + 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 full read joins the notice and the read with the exact separator", async () => { + // Review row 4202836427: in the clipped-but-not-truncated branch the two + // sentences and the read are joined by a specific separator, and that join is + // what tells the model every line was returned but some were cut. Assert the + // separator and both ends of the response rather than a loose substring. + 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-full.ts", offset: 1 }, + mockTask as unknown as Task, + callbacks, + ) + + const pushed = callbacks.pushToolResult.mock.calls[0][0] + // The header line and the notice are adjacent: the notice is the first thing + // after the file identity, and the character cap it names is the one the tool + // actually applied. + expect( + pushed.startsWith( + `File: clipped-full.ts\nIMPORTANT: Some lines exceed ${MAX_LINE_LENGTH} characters and were clipped in this view.`, + ), + ).toBe(true) + expect(pushed).toContain( + "were clipped in this view. The file was read in full, but the clipped lines were not shown in full.", + ) + expect(pushed.endsWith("were not shown in full.\n1 | a\n2 | b")).toBe(true) + expect(pushed).toContain("The file was read in full") + // Every line was returned, so the notice must not advertise a next offset. + expect(pushed).not.toContain("To read more") + // The read stays incomplete: the clipped tails were never shown, so a + // full-file replacement must not be built from this view. + const [, , calledComplete] = observeSpy.mock.calls[0] + expect(calledComplete).toBe(false) + }) + 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) + }) + + // Positive control for the row above: EF BF BD is valid UTF-8, so a file that already + // carries a U+FFFD decodes and re-encodes to the same bytes and the read is complete. The + // row above asserts only the partial direction, and it would still pass if the writer marked + // every U+FFFD it saw as a lossy decode. + it("native: a file that already contains a U+FFFD byte sequence is a complete read", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + const raw = Buffer.from([0x61, 0xef, 0xbf, 0xbd]) + 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: "already-lossy.ts" }, mockTask as unknown as Task, callbacks) + + expect(observeSpy).toHaveBeenCalledTimes(1) + const [calledPath, , calledComplete] = observeSpy.mock.calls[0] + expect(calledComplete).toBe(true) + expect(reg.get(calledPath)!.complete).toBe(true) + }) + + 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) + }) + // Positive control for the row above: EF BF BD is valid UTF-8, so a file that already + // carries a U+FFFD decodes and re-encodes to the same bytes and the read is complete. The + // row above asserts only the partial direction, and it would still pass if the writer marked + // every U+FFFD it saw as a lossy decode. + it("legacy: a file that already contains a U+FFFD byte sequence is a complete read", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + const raw = Buffer.from([0x61, 0xef, 0xbf, 0xbd]) + 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-already-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(true) + expect(reg.get(calledPath)!.complete).toBe(true) + }) + }) }) }) diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index ae3d504003..f3714cd1f5 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -861,7 +861,7 @@ }, "core/tools/__tests__/readFileTool.spec.ts": { "@typescript-eslint/no-explicit-any": { - "count": 96 + "count": 94 } }, "core/tools/__tests__/runSlashCommandTool.spec.ts": { diff --git a/src/integrations/misc/__tests__/indentation-reader-unicode.spec.ts b/src/integrations/misc/__tests__/indentation-reader-unicode.spec.ts index bfc397cf09..1d41ac30f3 100644 --- a/src/integrations/misc/__tests__/indentation-reader-unicode.spec.ts +++ b/src/integrations/misc/__tests__/indentation-reader-unicode.spec.ts @@ -74,6 +74,7 @@ describe("Unicode clipping through the existing readers", () => { totalLines: 3, returnedLines: 1, wasTruncated: true, + hasClippedLines: true, }) expect(Buffer.from(result.content).toString("utf8")).toBe(result.content) }) @@ -89,6 +90,8 @@ describe("Unicode clipping through the existing readers", () => { totalLines: 3, returnedLines: 1, wasTruncated: true, + // The returned line is clipped, so the flag belongs to the result. + hasClippedLines: true, }) expect(Buffer.from(result.content).toString("utf8")).toBe(result.content) }) @@ -104,6 +107,8 @@ describe("Unicode clipping through the existing readers", () => { totalLines: 5, returnedLines: 3, wasTruncated: false, + // The block shows every line it includes, but one is clipped. + hasClippedLines: true, }) expect(Buffer.from(result.content).toString("utf8")).toBe(result.content) }) @@ -115,6 +120,7 @@ describe("Unicode clipping through the existing readers", () => { totalLines: 1, returnedLines: 1, wasTruncated: false, + hasClippedLines: true, }) expect(readWithSlice("")).toEqual({ content: "1 | ", @@ -122,6 +128,7 @@ describe("Unicode clipping through the existing readers", () => { totalLines: 1, returnedLines: 1, wasTruncated: false, + hasClippedLines: false, }) }) @@ -132,6 +139,7 @@ describe("Unicode clipping through the existing readers", () => { totalLines: 1, returnedLines: 0, wasTruncated: false, + hasClippedLines: false, }) expect(readWithIndentation(longLine, { anchorLine: 0 })).toEqual({ content: "Error: anchor_line 0 is out of range (1-1)", @@ -139,6 +147,7 @@ describe("Unicode clipping through the existing readers", () => { totalLines: 1, returnedLines: 0, wasTruncated: false, + hasClippedLines: false, }) }) }) diff --git a/src/integrations/misc/__tests__/indentation-reader.spec.ts b/src/integrations/misc/__tests__/indentation-reader.spec.ts index a092dff8fc..622a150235 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, @@ -278,11 +279,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: offset 1001 is beyond file end") + // No line was returned, so nothing could have been clipped. + expect(result.hasClippedLines).toBe(false) }) it("should handle negative offset", () => { @@ -296,6 +331,53 @@ 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) + }) + + it("flags a clipped line in the single-line limit case", () => { + // finalLimit === 1 returns only the anchor line, and formatWithLineNumbers + // clips it when it exceeds the cap: the flag must still be set. + const code = ["short()", `long(${"x".repeat(MAX_LINE_LENGTH + 5)})`, "tail()"].join("\n") + const result = readWithIndentation(code, { anchorLine: 2, limit: 1 }) + + expect(result.returnedLines).toBe(1) + expect(result.hasClippedLines).toBe(true) + }) + + it("keeps the single-line limit case unflagged when the line fits the cap", () => { + // Clipping is strictly-longer-than-cap, so a line at exactly the cap is + // shown in full and the read carries no clipping flag. + const code = ["short()", `fits(${"y".repeat(MAX_LINE_LENGTH - 6)})`, "tail()"].join("\n") + const result = readWithIndentation(code, { anchorLine: 2, limit: 1 }) + + expect(result.returnedLines).toBe(1) + expect(result.hasClippedLines).toBe(false) + }) + + it("flags a clipped line inside an expanded block", () => { + // The expansion returns every line of the block, but the long one is + // clipped in the returned view: the flag must be set even with no + // omission. + const code = ["start()", `long(${"x".repeat(MAX_LINE_LENGTH + 1)})`, "end()"].join("\n") + const result = readWithIndentation(code, { anchorLine: 2, limit: 10 }) + + expect(result.returnedLines).toBeGreaterThan(1) + expect(result.hasClippedLines).toBe(true) + }) + + it("keeps an expanded block unflagged when every line fits the cap", () => { + const code = ["start()", "mid()", "end()"].join("\n") + const result = readWithIndentation(code, { anchorLine: 2, limit: 10 }) + + expect(result.returnedLines).toBeGreaterThan(1) + 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 43e86a47ad..ff324738a3 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 ──────────────────────────────────────────────────────────────── @@ -244,6 +246,7 @@ export function readWithIndentation(content: string, options: IndentationReadOpt totalLines, returnedLines: 0, wasTruncated: false, + hasClippedLines: false, } } @@ -276,6 +279,9 @@ export function readWithIndentation(content: string, options: IndentationReadOpt totalLines, returnedLines: 1, wasTruncated: totalLines > 1, + // The single-line case is no exception to the clipping contract: + // formatWithLineNumbers clips this line when it exceeds the cap. + hasClippedLines: lines[anchorIdx].content.length > MAX_LINE_LENGTH, } } @@ -358,6 +364,10 @@ export function readWithIndentation(content: string, options: IndentationReadOpt totalLines, returnedLines: result.length, wasTruncated: wasTruncated && result.length < totalLines, + // The expansion path reports clipping like the slice path: a block can + // return every line it includes and still hide content behind the + // per-line length cap. + hasClippedLines: result.some((line) => line.content.length > MAX_LINE_LENGTH), } } @@ -386,6 +396,7 @@ export function readWithSlice( totalLines, returnedLines: 0, wasTruncated: false, + hasClippedLines: false, } } @@ -393,6 +404,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) @@ -403,5 +419,6 @@ export function readWithSlice( totalLines, returnedLines: selectedLines.length, wasTruncated, + hasClippedLines, } } diff --git a/src/services/file-safety/__tests__/safeWriteText.integration.spec.ts b/src/services/file-safety/__tests__/safeWriteText.integration.spec.ts new file mode 100644 index 0000000000..4a3ee6e8e5 --- /dev/null +++ b/src/services/file-safety/__tests__/safeWriteText.integration.spec.ts @@ -0,0 +1,70 @@ +import * as fs from "fs/promises" +import * as os from "os" +import * as path from "path" + +import { safeWriteText } from "../safeWriteText" + +// No fs mocks in this file: the point is to assert what a real filesystem ends up +// holding after a publish attempt, which the mocked spec cannot show. The failure is +// provoked with real filesystem semantics rather than with a stubbed call. +describe("safeWriteText against a real filesystem", () => { + let dir: string + + beforeEach(async () => { + dir = await fs.mkdtemp(path.join(os.tmpdir(), "safe-write-text-int-")) + }) + + afterEach(async () => { + await fs.rm(dir, { recursive: true, force: true }) + }) + + it("publishes the new bytes and leaves no staging or backup residue", async () => { + const targetPath = path.join(dir, "target.txt") + await fs.writeFile(targetPath, "old bytes") + + // No platform override: the real platform's own durability and ACL steps run, and this + // case is the successful restore. A failed restore now throws DaclRestoreError after + // the commit rename has already happened (the focused unit test "win32 DACL: a failed + // restore is an error and the dump is still unlinked" covers that path), so on a + // machine where icacls cannot put a saved ACL back into a throwaway temp directory + // this publish reports that error instead of resolving, and the residue assertions + // below are reached only where the restore worked. + await safeWriteText(targetPath, "new bytes", { backup: true }) + + expect(await fs.readFile(targetPath, "utf8")).toBe("new bytes") + expect(await fs.readdir(dir)).toEqual(["target.txt"]) + }) + + it("leaves the target bytes untouched when the commit cannot replace it", async () => { + // A regular file cannot be renamed over a directory, so the backup copy and + // the commit both fail on a real filesystem with no mocking at all. + const targetPath = path.join(dir, "target-dir") + await fs.mkdir(targetPath) + const inside = path.join(targetPath, "payload.txt") + await fs.writeFile(inside, "original bytes") + + await expect(safeWriteText(targetPath, "new data", { backup: true })).rejects.toThrow() + + // The directory and its content are exactly as they were, and no backup copy + // or staging directory was left behind next to them. + expect(await fs.readFile(inside, "utf8")).toBe("original bytes") + expect(await fs.readdir(dir)).toEqual(["target-dir"]) + }) + + it("cleans up the staging file when the commit rename itself fails", async () => { + // With backup:false the backup step is skipped, so this is the only shape of + // this scenario that reaches Step 4: the rename of the staging FILE over the + // target DIRECTORY fails (EISDIR / ENOTDIR / EPERM depending on platform), and + // the cleanup must remove the staging file. The backup:true case above bails + // out in the backup step and never exercises the commit-failure path. + const targetPath = path.join(dir, "target-dir") + await fs.mkdir(targetPath) + const inside = path.join(targetPath, "payload.txt") + await fs.writeFile(inside, "original bytes") + + await expect(safeWriteText(targetPath, "new data", { backup: false })).rejects.toThrow() + + expect(await fs.readFile(inside, "utf8")).toBe("original bytes") + expect(await fs.readdir(dir)).toEqual(["target-dir"]) + }) +}) 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..fc8f30e212 --- /dev/null +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -0,0 +1,1553 @@ +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 { + DaclInspectionError, + DaclRestoreError, + PostCommitDurabilityError, + resolveLockKey, + safeWriteText, + StagingPathError, +} from "../safeWriteText" + +// Full mock for fs/promises — all methods are vi.fn() stubs +vi.mock("fs/promises", () => ({ + copyFile: vi.fn(), + chmod: vi.fn(), + 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 +} + +// fs.BigIntStats is a type-only export (fs.BigIntStats is undefined at runtime), so the stand-in is +// a Stats object carrying bigint ino/dev - exactly what fs.lstat(path, { bigint: true }) hands +// back at runtime. +function _fileStatsWithIdentity(ino: bigint, dev: bigint): fsSync.BigIntStats { + // Double assertion: the runtime value is the Stats stand-in, the type is the bigint variant. + const s = _fileStats(false) as unknown as fsSync.BigIntStats + s.ino = ino + s.dev = dev + 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() + }) + }) + + it("win32: a rejecting async onWarning does not displace the caller's error or leak an unhandled rejection", async () => { + // TypeScript accepts an async sink where a void callback is expected, so the + // wrapper has to attach a handler to the returned promise: an unhandled + // rejection can end the process under Node's default mode. The notice exercised + // here is the retained-backup one - the DACL notices became refusals - so a sink + // failure now has to be reported without displacing the error the caller catches. + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The DACL save succeeds and only the restore fails: the publish commits and + // reports its retained backup copy through the sink. + let icaclsCalls = 0 + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + icaclsCalls++ + if (typeof cb === "function") cb(icaclsCalls === 1 ? null : new Error("icacls restore error"), "", "") + return fakeChild + }) + const consoleWarn = vi.spyOn(console, "warn").mockImplementation(() => {}) + + await expect( + safeWriteText(targetPath, "data", { + platform: "win32", + backup: true, + onWarning: async () => { + throw new Error("async sink down") + }, + }), + ).rejects.toBeInstanceOf(DaclRestoreError) + + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + // The rejection is reported through the fallback sink rather than surfacing as an + // unhandled rejection. + expect(consoleWarn).toHaveBeenCalledWith(expect.stringContaining("onWarning callback rejected")) + consoleWarn.mockRestore() + }) + + // ── 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; it is the only rename + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // the failing cleanup was the post-commit backup unlink + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + + // 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, and the backup was only + // ever a copy: the target was never moved, so there is nothing to rename back. + expect(fs.copyFile).toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // The durability failure is reported, not swallowed - and the backup copy is not + // left beside the target where no caller could find it. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + }) + }) + + // ── Test 4: backup:true keeps old safeWriteJson semantics, copy-based ── + + describe("backup:true", () => { + it("copies target -> backup before commit without moving the target, deletes the copy on success", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "new data", { backup: true }) + + // target was accessed (exists check) + expect(fs.access).toHaveBeenCalledWith(targetPath) + + // The backup is a copy: the canonical target is never moved away, so readers + // never see a missing file and no later step can clobber a concurrent publish. + expect(fs.copyFile).toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) + expect(fs.rename).not.toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) + + // the only rename is the atomic commit temp -> target + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // backup copy was deleted on success + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + }) + + it("a failed commit does not move the target, so nothing has to be rolled back", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The commit rename is the only rename in the flow and it fails. + vi.mocked(fs.rename).mockRejectedValue(new Error("ENOSPC")) + + await expect(safeWriteText(targetPath, "new data", { backup: true })).rejects.toThrow("ENOSPC") + + // The target never left its path, so there is no restore rename and the + // pre-write content is still what a reader sees at targetPath. + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(fs.copyFile).toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) + + // Both the backup copy and the staging temp are cleaned up on failure. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("creates the backup privately before its content exists, then fsyncs it", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "new data", { backup: true }) + + // The destination must exist with a private mode before copyFile writes anything + // into it: copyFile chooses the destination mode itself, so a restrictive target + // could otherwise leave a group/world-readable copy that a later chmod cannot + // undo. "wx" also means a pre-existing path is never silently reused. + const seedOpen = vi.mocked(fsSync.openSync).mock.calls.find(function (call) { + return String(call[0]).includes("safeWriteText.bak_") && call[1] === "wx" + }) + expect(seedOpen).toBeDefined() + expect(seedOpen?.[2]).toBe(0o600) + const seedOrder = vi.mocked(fsSync.openSync).mock.invocationCallOrder[ + vi.mocked(fsSync.openSync).mock.calls.indexOf(seedOpen!) + ] + expect(seedOrder).toBeLessThan(vi.mocked(fs.copyFile).mock.invocationCallOrder[0]) + + // The chmod keeps a copied read-only attribute (Windows) from breaking the fsync + // open, and keeps a backup of a permissive file private. + expect(fs.copyFile).toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) + expect(fs.chmod).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_"), 0o600) + expect(vi.mocked(fs.chmod).mock.invocationCallOrder[0]).toBeGreaterThan( + vi.mocked(fs.copyFile).mock.invocationCallOrder[0], + ) + + // The copy is then opened for fsync with the writable flag. + const backupOpen = vi.mocked(fsSync.openSync).mock.calls.find(function (call) { + return String(call[0]).includes("safeWriteText.bak_") && call[1] === "r+" + }) + expect(backupOpen).toBeDefined() + }) + + it("a failed backup flush is reported and leaves no partial backup behind", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The copy lands, but the fsync of the copy fails: the retained content is not + // known to be durable, so the write must not proceed on a half-written backup. + // The staged temp is fsynced earlier with a different handle, so target the + // backup's fd specifically. + vi.mocked(fsSync.openSync).mockImplementation((p: unknown) => + String(p).includes("safeWriteText.bak_") ? 7 : 1, + ) + vi.mocked(fsSync.fsyncSync).mockImplementation((fd: unknown) => { + if (fd === 7) { + throw new Error("EIO") + } + }) + + await expect(safeWriteText(targetPath, "new data", { backup: true, platform: "linux" })).rejects.toThrow( + "EIO", + ) + + // Nothing was published, and the incomplete copy is removed rather than left + // next to the target looking like a usable backup. + expect(fs.rename).not.toHaveBeenCalled() + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("a backup copy that fails part-way is removed, not left as a usable-looking backup", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The destination is seeded with openSync("wx") BEFORE any content exists at it, + // and the copy then fails part-way: the name is there, holding a partial copy of + // nothing usable. Cleanup has to key off the attempt, not off a copy that + // succeeded, or this file outlives the failed write beside the target. + vi.mocked(fs.copyFile).mockRejectedValue(Object.assign(new Error("EIO"), { code: "EIO" })) + + await expect(safeWriteText(targetPath, "new data", { backup: true, platform: "linux" })).rejects.toThrow( + "EIO", + ) + + // Nothing was published, and the half-written copy is removed rather than left + // next to the target looking like a backup someone could restore. + expect(fs.rename).not.toHaveBeenCalled() + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("backup:true when target does not exist: no backup created, just commit", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // fs.access resolves for dirPath check, but rejects for target check (backup path) + vi.mocked(fs.access).mockImplementation(async (p) => { + if (typeof p === "string" && p.endsWith("target.txt")) throw { code: "ENOENT" } + }) + + await safeWriteText(targetPath, "new data", { backup: true, platform: "linux" }) + + // no backup rename (target didn't exist) + expect(fs.access).toHaveBeenCalledWith(targetPath) + + // only one rename: temp -> target + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // no unlink (no backup to delete; DACL skipped via platform:linux) + expect(fs.unlink).not.toHaveBeenCalled() + }) + }) + + // ── Test 5: win32 DACL path ────────────────────────────────────────────── + + describe("win32 DACL", () => { + it.skipIf(process.platform !== "win32")( + "copies target DACL onto staging file via icacls before rename on Windows", + async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // icacls dump + restore were called (execFile is callback-based mock) + expect(execFile).toHaveBeenCalledTimes(2) + }, + ) + + it("non-win32: DACL path is unreachable when platform is not win32", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // icacls was NOT called on non-win32 + expect(execFile).not.toHaveBeenCalled() + }) + + it("win32 DACL failure refuses the publish instead of renaming over the target", 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 + }) + + // Captured once: calling safeWriteText twice here would double-count the single + // icacls attempt asserted below. + const refusal = await safeWriteText(targetPath, "data", { platform: "win32" }).catch( + (error: unknown) => error, + ) + + expect(refusal).toBeInstanceOf(DaclInspectionError) + // The class alone does not pin the contract: swapping the phase or dropping the + // target path still satisfies toBeInstanceOf, and both are caller-visible - the + // phase says which check refused, the path names the file the caller must not + // assume was saved. + expect(refusal).toMatchObject({ + name: "DaclInspectionError", + phase: "save", + targetPath, + message: expect.stringContaining("its DACL could not be saved"), + }) + + // Contract change (Security Boundaries row): a target whose DACL could not be + // saved is no longer replaced by a file that inherits different rights. + expect(fs.rename).not.toHaveBeenCalled() + // one icacls attempt only: a failed capture must not try to restore + expect(execFile).toHaveBeenCalledTimes(1) + }) + + it("win32: refuses the publish without an onWarning notice when the DACL cannot be saved", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + if (typeof cb === "function") cb(new Error("icacls error"), "", "") + return fakeChild + }) + const warnings: string[] = [] + + await expect( + safeWriteText(targetPath, "data", { platform: "win32", onWarning: (m) => warnings.push(m) }), + ).rejects.toBeInstanceOf(DaclInspectionError) + + // Contract change: the refusal replaces the warning. There is no replacement whose + // access rights could have changed, so no notice is emitted for a write that did + // not happen - a warning beside a refused publish would let a caller that ignores + // the error still read the write as a save. + expect(fs.rename).not.toHaveBeenCalled() + expect(warnings).toHaveLength(0) + }) + + it("win32: refuses the publish when the target cannot be checked for DACL preservation", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The target exists but is not readable: that is not "absent", and skipping DACL + // preservation has to stop the publish rather than be reported beside it. + vi.mocked(fs.access).mockImplementation(async (p) => { + if (String(p) === targetPath) { + throw Object.assign(new Error("EACCES"), { code: "EACCES" }) + } + }) + const warnings: string[] = [] + + // Captured once: a second call would double-count the assertions below. + const refusal = await safeWriteText(targetPath, "data", { + platform: "win32", + onWarning: (m) => warnings.push(m), + }).catch((error: unknown) => error) + + expect(refusal).toBeInstanceOf(DaclInspectionError) + expect(refusal).toMatchObject({ + name: "DaclInspectionError", + phase: "inspect", + targetPath, + message: expect.stringContaining("its access rights could not be checked"), + }) + + // Contract change: "there but unreadable" is no longer a reason to publish blind. + // The publish is refused before any icacls runs, and nothing is warned about a + // write that did not happen. + expect(execFile).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + expect(warnings).toHaveLength(0) + }) + + // The refusals sit inside the try whose catch performs the cleanup, so a refused + // publish is cleaned up by that handler and by nothing else. Counted, not matched + // with toHaveBeenCalledWith: that matcher passes however many times the same path + // is passed, so it cannot see a cleanup that runs twice. + it("win32: a refused publish removes its staged file and staging directory exactly once", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + if (typeof cb === "function") cb(new Error("icacls error"), "", "") + return fakeChild + }) + + await expect(safeWriteText(targetPath, "data", { platform: "win32" })).rejects.toBeInstanceOf( + DaclInspectionError, + ) + + const unlinkCalls = vi.mocked(fs.unlink).mock.calls.map((call) => String(call[0])) + const rmdirCalls = vi.mocked(fs.rmdir).mock.calls.map((call) => String(call[0])) + expect(unlinkCalls.filter((p) => p.includes("safeWriteText_"))).toHaveLength(1) + expect(rmdirCalls.filter((p) => p.includes(".file-safety-staging"))).toHaveLength(1) + }) + + // Warning delivery is advisory: it must not be able to change what the caller learns. + it("win32: a throwing onWarning does not replace the error the caller receives", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The DACL save succeeds and the restore fails, so the notice this sink throws on + // is the retained-backup one delivered from the failure handler. + let icaclsCalls = 0 + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + icaclsCalls++ + if (typeof cb === "function") cb(icaclsCalls === 1 ? null : new Error("icacls restore error"), "", "") + return fakeChild + }) + const consoleWarn = vi.spyOn(console, "warn").mockImplementation(() => {}) + + const failure = await safeWriteText(targetPath, "data", { + platform: "win32", + backup: true, + onWarning: () => { + throw new Error("callback down") + }, + }).catch((error: unknown) => error) + + // A broken notice cannot turn a refused publish into an unreported one, and cannot + // swallow the write either: the caller still holds the DACL error. + expect(failure).toBeInstanceOf(DaclRestoreError) + expect(fs.rename).toHaveBeenCalledTimes(1) + // The sink really was reached, so this is not a test that passes because no + // notice was ever delivered. + expect(consoleWarn).toHaveBeenCalledWith(expect.stringContaining("onWarning callback failed")) + consoleWarn.mockRestore() + }) + + 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 expect(safeWriteText(targetPath, "data", { platform: "win32" })).rejects.toBeInstanceOf( + DaclInspectionError, + ) + + // Contract change: the publish is refused, so nothing is renamed. Only the save was + // attempted, and the partial dump is still removed below. + expect(fs.rename).not.toHaveBeenCalled() + 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 copy, 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 copy the dump could describe a + // file that a concurrent publish had already replaced. + await safeWriteText(targetPath, "data", { backup: true, platform: "win32" }) + + const callOrder = vi.mocked(execFile).mock.invocationCallOrder + const copyOrder = vi.mocked(fs.copyFile).mock.invocationCallOrder + const renameOrder = vi.mocked(fs.rename).mock.invocationCallOrder + const saveCall = callOrder[0] + const restoreCall = callOrder[1] + const backupCopy = copyOrder[0] + const commitRename = renameOrder[0] + + expect(saveCall).toBeLessThan(backupCopy) + expect(backupCopy).toBeLessThan(commitRename) + expect(commitRename).toBeLessThan(restoreCall) + }) + + it("win32 DACL: a failed restore is an error and the dump is still unlinked", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const warnings: string[] = [] + + // 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 + }) + + const failure = await safeWriteText(targetPath, "data", { + platform: "win32", + onWarning: (m) => warnings.push(m), + }).catch((error: unknown) => error) + + expect(failure).toBeInstanceOf(DaclRestoreError) + // The class alone does not pin the contract: the committed path is what a caller + // needs in order to tell which file answers to different rights than it did. + expect(failure).toMatchObject({ + name: "DaclRestoreError", + targetPath, + message: expect.stringContaining("could not be restored"), + }) + + // The content did commit before the restore failed, so the rename happened exactly + // once even though the publish reports an error. + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + expect(fs.rename).toHaveBeenCalledTimes(1) + + // Contract change: the changed access rights are an error the caller receives, not + // a notice beside a write that resolved. With no backup copy in play there is no + // leftover to name, so this publish stays silent and reports through the error. + expect(warnings).toHaveLength(0) + + // dump file was still unlinked in finally + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.acl")) + }) + + it("win32 DACL: a failed restore keeps the backup copy that still carries the original access rights", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const warnings: string[] = [] + 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 expect( + safeWriteText(targetPath, "data", { + platform: "win32", + backup: true, + onWarning: (m) => warnings.push(m), + }), + ).rejects.toBeInstanceOf(DaclRestoreError) + + // The publish committed, and the copy made from the file it replaced is the only + // artifact left holding the access rights that could not be restored. Removing it + // would destroy the recovery path for exactly the fact the error reports, so the + // failure handler must leave it in place. Counted, because toHaveBeenCalledWith + // cannot see a second unlink of the same path. + const unlinks = vi.mocked(fs.unlink).mock.calls.map((call) => String(call[0])) + expect(unlinks.filter((p) => p.includes("safeWriteText.bak_"))).toHaveLength(0) + expect(fs.rename).toHaveBeenCalledTimes(1) + + // The retained path is named to the human: this primitive has no structured result + // to carry it on, and an invisible leftover is worse than a visible one. + expect(warnings.filter((m) => m.includes("safeWriteText.bak_"))).toHaveLength(1) + }) + + it("win32 DACL: a failed restore reaches the caller-staged publish safeWriteJson hands in", async () => { + // safeWriteJson stages its own temp file and calls in with backup:true, so this is + // the shape its callers actually see: the publish must reject rather than resolve + // over a file whose rights changed, and keep the copy that still carries them. + 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) + let callCount = 0 + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + callCount++ + if (typeof cb === "function") cb(callCount === 1 ? null : new Error("icacls restore error"), "", "") + return fakeChild + }) + + const outcome = await safeWriteText(targetPath, "", { + platform: "win32", + backup: true, + tempPath: callerTemp, + }).then( + () => "resolved" as const, + (error: unknown) => error, + ) + + // Not "resolved": no caller can observe success for a publish whose DACL was not + // preserved. + expect(outcome).toBeInstanceOf(DaclRestoreError) + expect(fs.rename).toHaveBeenCalledWith(callerTemp, targetPath) + const unlinks = vi.mocked(fs.unlink).mock.calls.map((call) => String(call[0])) + expect(unlinks.filter((p) => p.includes("safeWriteText.bak_"))).toHaveLength(0) + }) + + 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() + }) + + it("rejects a staging path that is the target itself", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // Same inode and device for the supplied staging path and the target: the + // failure handler would unlink the only copy of the content, so a failed + // write would delete the file it was meant to protect. + const stats = _fileStatsWithIdentity(42n, 7n) + vi.mocked(fs.lstat).mockResolvedValue(stats) + + await expect(safeWriteText(targetPath, "data", { tempPath: targetPath, platform: "linux" })).rejects.toThrow( + StagingPathError, + ) + expect(fsSync.openSync).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + // The comparison is only sound when both stats are read as bigint: on NTFS/ReFS the file + // identifiers exceed Number.MAX_SAFE_INTEGER. + // Filter on the options, not the spelling: path.resolve prefixes a drive letter on Windows, + // so the two identity reads are the calls that asked for options at all. + const identityLookups = vi.mocked(fs.lstat).mock.calls.filter((c) => c[1] !== undefined) + expect(identityLookups.length).toBeGreaterThanOrEqual(2) + for (const c of identityLookups) { + expect(c[1]).toEqual({ bigint: true }) + } + expect(fs.unlink).not.toHaveBeenCalled() + }) + + it("rejects when the target identity cannot be compared for a reason other than a missing target", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // A hard-linked staging file shares the target's inode, so the identity comparison is the only thing + // between this write and a rename onto the very file the guard protects. An EACCES from the target + // lstat must not be mistaken for "there is no target". + const stagingStats = _fileStatsWithIdentity(42n, 7n) + vi.mocked(fs.lstat).mockImplementation(async (p) => { + if (String(p) === targetPath) { + throw Object.assign(new Error("EACCES"), { code: "EACCES" }) + } + return stagingStats + }) + + await expect( + safeWriteText(targetPath, "data", { tempPath: "/tmp/test-dir/hardlink.txt", platform: "linux" }), + ).rejects.toThrow("Staging file could not be compared with the target") + expect(fsSync.openSync).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + // Both identity reads must ask for bigint stats, or the comparison silently falls + // back to rounded numbers on NTFS/ReFS. + const identityLookups = vi.mocked(fs.lstat).mock.calls.filter((c) => c[1] !== undefined) + expect(identityLookups).toHaveLength(2) + for (const c of identityLookups) { + expect(c[1]).toEqual({ bigint: true }) + } + }) +}) + +describe("cleanup when a backed-up write fails before commit", () => { + beforeEach(() => mockDefaults()) + + it("releases the staged file, its copy 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) + // The commit rename is the only rename in this flow and it fails. + vi.mocked(fs.rename).mockRejectedValue(new Error("ENOSPC")) + + await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux" })).rejects.toThrow("ENOSPC") + + // The staging file and this write's own directory must not leak, and neither may + // the backup copy: the target still holds the pre-write content on disk. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + 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[0] + 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)) + }) +}) + +describe("resolveLockKey when the parent directory does not exist yet", () => { + beforeEach(() => mockDefaults()) + + it("canonicalizes through the nearest existing ancestor instead of the literal parent", async () => { + // Two writers must take ONE lock: the one whose parent directory is already there, + // and the one racing to create it. Resolving only the immediate parent and falling + // back to its literal spelling on ENOENT gave them different keys whenever an + // ancestor was a symlink or a Windows short name, so a read-modify-write under the + // advisory lock lost one side. + const aliasDir = path.resolve("/tmp/alias-parent") + const canonicalDir = path.resolve("/tmp/real-parent") + const nested = path.join(aliasDir, "nested") + const target = path.join(nested, "history_item.json") + vi.mocked(fs.lstat).mockResolvedValue(_fileStats(false)) + vi.mocked(fs.realpath).mockImplementation(async (p) => { + const s = String(p) + if (s === target || s === nested) { + throw Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + } + if (s === aliasDir) { + return canonicalDir + } + return s + }) + + expect(await resolveLockKey(target)).toBe(path.join(canonicalDir, "nested", "history_item.json")) + }) + + it("propagates a realpath failure that is not ENOENT instead of guessing a key", async () => { + // A realpath that fails for another reason says nothing about the canonical form; + // returning a literal key would silently put this writer on a different lock. + const target = path.join(path.resolve("/tmp/test-dir"), "history_item.json") + vi.mocked(fs.lstat).mockResolvedValue(_fileStats(false)) + // Not a link: the walk must reach the canonicalization step, which is where the + // non-ENOENT failure has to surface. + vi.mocked(fs.readlink).mockRejectedValue( + Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }), + ) + vi.mocked(fs.realpath).mockImplementation(async (p) => { + if (String(p) === target) return target + throw Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + }) + + await expect(resolveLockKey(target)).rejects.toThrow("EACCES") + }) +}) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts new file mode 100644 index 0000000000..c2d83ea2cf --- /dev/null +++ b/src/services/file-safety/safeWriteText.ts @@ -0,0 +1,660 @@ +import * as fs from "fs/promises" +import * as fsSync from "fs" +import * as path from "path" +import { execFile } from "child_process" + +import { getErrorCode } from "../../utils/errorCode" + +export interface SafeWriteTextOptions { + /** + * When true, keep the old-file semantics without ever removing the target: the + * previous content is copied to a hidden backup path and flushed before the + * commit rename, the commit rename atomically replaces the target, and on success + * the backup copy is deleted. A failure before the commit leaves the target + * untouched (there is nothing to roll back) and removes the backup copy. When + * false (default) the atomic rename simply replaces the target. + */ + 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 + + /** + * Sink for non-fatal safety notices. The notice that still reaches it is a leftover: the backup + * copy kept beside the target after a Windows DACL restore failed, named with its path. A DACL + * that could not be captured is no longer a notice here - the publish is refused with + * DaclInspectionError before anything is committed - and a saved DACL that could not be put back + * after the commit arrives as DaclRestoreError rather than a warning, so neither reaches this + * sink. Defaults to console.warn. + */ + onWarning?: (message: string) => void + + /** + * 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 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 + } +} + +/** + * The target exists but its access rights could not be inspected or saved, so publishing would + * replace it with a file that inherits different rights. On Windows the publish is refused before + * anything is committed: a save that silently widened who can read the file is not a save the + * caller can trust, and nothing has been published yet, so the target still holds its content. + */ +export class DaclInspectionError extends Error { + constructor( + readonly targetPath: string, + readonly phase: "inspect" | "save", + readonly causeError: unknown, + ) { + const detail = phase === "save" ? "its DACL could not be saved" : "its access rights could not be checked" + super(`safeWriteText: refusing to publish over ${targetPath} because ${detail}`) + this.name = "DaclInspectionError" + } +} + +/** + * The content is committed but the saved DACL could not be put back on it, so the file at the + * target answers to different access rights than the one it replaced. Reported as an error rather + * than a warning: no caller can observe success for a publish whose DACL was not preserved. The + * failure handler keeps this write's backup copy for it - that copy is the only artifact still + * carrying the access rights of the file the publish replaced. + */ +export class DaclRestoreError extends Error { + constructor( + readonly targetPath: string, + readonly causeError: unknown, + ) { + super(`safeWriteText: content committed at ${targetPath}, but its saved access rights could not be restored`) + this.name = "DaclRestoreError" + } +} +// -- 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 +} + +/** 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. + * Returns whether icacls succeeded; the caller reports a failure. */ +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(), + ) + }) + return true + } catch { + return false + } +} + +// -- 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 (getErrorCode(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 (getErrorCode(lstatError) === "ENOENT") return undefined + throw lstatError + }) + if (linkStat?.isSymbolicLink()) throw error + return absoluteFilePath + }) +} + +/** + * 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. + */ + +export async function canonicalDirKey(absoluteFilePath: string): Promise { + const dirPath = path.dirname(absoluteFilePath) + // Walk up to the nearest ancestor that EXISTS, canonicalize that, and re-join the + // components that are not there yet. Falling back to the unresolved spelling of the + // whole parent - what a single realpath(...).catch(() => dirPath) used to do - makes + // the key depend on whether the directory happens to exist: a writer whose parent is + // already there canonicalizes through a symlinked ancestor (or a short name) while a + // writer racing to create the same directory gets the literal spelling, so the two + // take different locks for one file and a read-modify-write loses one side. + // A realpath failure that is not "not there yet" says nothing about the canonical + // form, so it is propagated rather than papered over with a key that may be wrong. + let cursor = dirPath + const missing: string[] = [] + for (;;) { + const canonical = await fs.realpath(cursor).catch((error: unknown) => { + if (getErrorCode(error) === "ENOENT") return undefined + throw error + }) + if (canonical !== undefined) { + return path.join(canonical, ...missing.reverse(), path.basename(absoluteFilePath)) + } + missing.push(path.basename(cursor)) + const parent = path.dirname(cursor) + if (parent === cursor) { + // Every component up to the root is missing: there is nothing to canonicalize + // against, and the literal path is the only key left. + return path.join(dirPath, path.basename(absoluteFilePath)) + } + cursor = parent + } +} + +/** + * 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 (a publish renames the staged file onto the referent, + * and backup mode keeps a copy beside it). + * 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) + } +} + +/** + * Publish text by staging beside the resolved target and committing with a rename. + * + * @internal Production callers of this file use resolvePublishTarget and + * resolveLockKey; the write entry point itself has no production caller while #2350 + * decides which primitive main keeps, so it is tagged internal instead of being + * reported as an unused export. + */ +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, + ) + } + // BigInt stats: on NTFS/ReFS the file identity can exceed Number.MAX_SAFE_INTEGER, and + // a rounded number makes two different files look identical (rejecting a valid staging + // file) or hides a real alias. + const stagingStat = await fs.lstat(supplied, { bigint: true }) + if (stagingStat.isSymbolicLink() || !stagingStat.isFile()) { + throw new StagingPathError( + `Staging file must be a regular file, not ${stagingStat.isSymbolicLink() ? "a symlink" : "another file type"}`, + supplied, + ) + } + // A staging path that is the target would be unlinked by the failure handler + // while it still holds the only copy of the content, so a failed write would + // delete the file it was meant to protect. Compare identities, not spellings: + // an alias of the target is the same hazard. + // Only a missing target may be skipped: an EACCES/ELOOP/ENOTDIR here means the + // identity comparison could not be made, and treating that as "no target" would let a + // staging alias reach the commit and let cleanup delete the file it was meant to + // protect. + const targetStat = await fs.lstat(targetPath, { bigint: true }).catch((error: unknown) => { + if (getErrorCode(error) !== "ENOENT") { + throw new StagingPathError("Staging file could not be compared with the target", supplied) + } + return null + }) + if ( + targetStat && + typeof stagingStat.ino === "bigint" && + typeof targetStat.ino === "bigint" && + stagingStat.ino === targetStat.ino && + stagingStat.dev === targetStat.dev + ) { + throw new StagingPathError("Staging file must not be the target itself", 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. + // 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 + // Warning delivery must never abort the write: the notice below describes a leftover + // beside a committed file, and a caller whose callback throws (a UI sink, a logger that is + // mid-restart) must not turn that into a different failure than the one the caller is told + // about. Declared above the try so the failure handler can report a retained backup through it. + const warn = (message: string) => { + const report = (label: string, error: unknown) => { + console.warn( + `safeWriteText: onWarning callback ${label}: ${error instanceof Error ? error.message : String(error)}`, + ) + } + try { + const sink = options?.onWarning ?? ((m: string) => console.warn(m)) + const result: unknown = sink(message) + // A sink may be async - TypeScript accepts a value-returning callback where + // a void one is expected. Awaiting it would let warning delivery delay a + // write that has already committed (and hang it if the sink never settles), + // while leaving the promise unhandled turns a rejection into an unhandled + // rejection, which under Node's default mode can end the process after a + // successful write. Attach a handler without awaiting. + if (result instanceof Promise) { + result.catch((error: unknown) => report("rejected", error)) + } + } catch (error: unknown) { + report("failed", error) + } + } + 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 (getErrorCode(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) + } + fsSync.fsyncSync(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 (getErrorCode(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) + } + fsSync.fsyncSync(fd) + } finally { + fsSync.closeSync(fd) + } + } + + // -- Step 2 (win32): save DACL BEFORE the backup copy ----------- + const platform = options?.platform ?? process.platform + if (platform === "win32") { + let accessError: unknown = null + try { + await fs.access(targetPath) // target exists? + } catch (error: unknown) { + accessError = error + } + if (accessError === null) { + 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(() => {}) + // The target exists and its DACL could not be captured, so the commit rename + // would replace it with a file that inherits different access rights, and nothing + // here can verify an equivalent restrictive ACL on that replacement. The publish is + // refused instead of warned about: a save that silently changed who can read the + // file is not a save the caller can trust. Nothing is committed yet, so the target + // still holds its content, and this throw lands in the handler at the bottom of + // this try - the one place that removes the staged file and this write's own + // staging directory - so a refused publish strands neither. + throw new DaclInspectionError(targetPath, "save", null) + } + } else if (getErrorCode(accessError) !== "ENOENT") { + // Not "absent": the target is there but its access rights could not be read + // (EACCES, ...), so publishing would replace a file whose rights this call never + // learned. Same rule as the failed save above: refuse before anything is committed. + throw new DaclInspectionError(targetPath, "inspect", accessError) + } + } + try { + // -- Step 3 (backup:true): durable copy target -> backup ---- + if (options?.backup) { + try { + await fs.access(targetPath) + backupPath = _tempName(dirPath, "safeWriteText.bak") + // Copy, never move. Renaming the target away leaves the canonical path absent for + // the whole commit window: readers see a missing file, and a concurrent + // writer can create a new target that a later rollback would destroy. A copy + // keeps the target present, so the step 4 rename is the only change to the + // canonical path. The copy is flushed so the retained content survives a crash. + try { + // Create the destination BEFORE any content exists at it, with the mode fixed + // at open time. fs.copyFile picks the destination mode itself (the platform + // creation mask subject to umask on some platforms, the source's mode - or its + // read-only attribute - on others), so letting it create the file would either + // leave a restrictive target's bytes briefly readable to others, or leave the + // copy unwritable so the fsync open below fails with EACCES. open() ignores its + // mode argument for an existing file, so this 0o600 survives the copy on POSIX; + // the chmod afterwards is what clears a copied read-only attribute on Windows + // and keeps a backup of a permissive file private. + const seedFd = fsSync.openSync(backupPath, "wx", 0o600) + fsSync.closeSync(seedFd) + await fs.copyFile(targetPath, backupPath) + await fs.chmod(backupPath, 0o600) + // "r+" not "r": fsync on a read-only handle is EPERM on Windows, and the same + // flag the staged temp file uses above. + const backupFd = fsSync.openSync(backupPath, "r+") + try { + fsSync.fsyncSync(backupFd) + } finally { + fsSync.closeSync(backupFd) + } + } catch (backupError: unknown) { + // A partial backup must not outlive this attempt: it is not a complete copy + // of anything, and once the write fails nothing else removes it. + await fs.unlink(backupPath).catch(() => {}) + backupPath = null + throw backupError + } + releaseBackupOnSuccess = true + } catch (err: unknown) { + if (getErrorCode(err) !== "ENOENT") throw err + } + } + + // -- Step 4: atomic rename temp -> target --------------------- + await fs.rename(tempPath, targetPath) + + // -- Step 4b (POSIX): fsync the parent directory so the directory entry + // changed by the commit rename is durable, not just the file content. + if (platform !== "win32") { + try { + const dirFd = fsSync.openSync(dirPath, "r") + try { + fsSync.fsyncSync(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) + const restored = await _restoreDaclWindows(restoredDir, daclDumpPath, options?.execFileRunner) + if (!restored) { + // The content is committed, but the published file answers to a different DACL + // from the one that was saved, and nothing here verified an equivalent + // restrictive ACL on the replacement. This reaches the caller as an error rather + // than a warning: no caller can observe success for a publish whose DACL was not + // preserved. The handler at the bottom of this try keeps the backup copy for this + // error instead of rolling the published content back, so the throw reports the + // changed access rights without undoing a write the caller can already observe. + throw new DaclRestoreError(targetPath, null) + } + } + + // -- 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) { + if (originalError instanceof DaclRestoreError) { + // The one post-commit failure that keeps its copy. The published file answers to + // the descriptor its staging file was created with, and this copy - made from the + // file the publish replaced, so on Windows it carries that file's security + // attributes - is the only artifact left holding the access rights that could not + // be restored. Deleting it would destroy the only recovery path for exactly the + // fact the error reports, so the copy stays and its path is named to the human: + // the caller already holds the error, and this primitive reports a leftover on + // the warning sink because it has no structured result to carry it on. + warn( + `${originalError.message}; its backup copy is kept at ${backupPath} and still carries the access rights of the file it replaced.`, + ) + } else { + // Nothing to restore: the backup is a copy, so the target still holds whatever + // the commit left there - before the commit that is the pre-write content, and + // after it the published content. Either way the copy has served its purpose + // and must not be left beside the target where no caller can find it. + await fs.unlink(backupPath).catch(() => {}) + } + backupPath = null + } + 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(() => {}) + } + + throw originalError + } +} diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index c973f35a03..50ae26ebb3 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -630,8 +630,9 @@ export class McpHub { } // Get project-level MCP configuration path - private async getProjectMcpPath(): Promise { - const workspacePath = this.providerRef.deref()?.cwd ?? getWorkspacePath() + private async getProjectMcpPath( + workspacePath: string | undefined = this.providerRef.deref()?.cwd ?? getWorkspacePath(), + ): Promise { const projectMcpDir = path.join(workspacePath, ".roo") const projectMcpPath = path.join(projectMcpDir, "mcp.json") @@ -643,6 +644,41 @@ export class McpHub { } } + /** + * The root a project-scoped MCP write has to stay inside. + * + * safeWriteJson resolves the publish target with realpath before staging beside it, so a + * repository that ships .roo/mcp.json as a symlink to a file OUTSIDE the workspace would + * have that outside file replaced as soon as the user edits a project MCP setting or + * allowlist. Passing the workspace root as confineTo makes that write fail closed instead. + * Global writes are deliberately unconstrained: they target the user's own settings + * directory, which is not under the workspace. + * + * DECISION AND LIMIT - these three call sites carry credentials (MCP server env and + * headers), so the limit of the control they pass is stated here rather than left to the + * option doc: confineTo decides twice from the resolved publish target and stops at its + * final component. If a workspace replaces an ANCESTOR directory - .roo, or the workspace + * directory itself - with a link after the last check, the credential write follows that + * link and can still land outside the workspace; see the confineTo doc in + * src/utils/safeWriteJson.ts for the two outcomes of that window. The control is kept + * anyway, not dropped, because the case a committed tree can actually plant is a link at + * the settings file itself, and that is the case this option does refuse. Closing the + * ancestor window needs a handle-relative no-follow publish, which this runtime does not + * offer, and the alternative reading - removing the control from the credential path - + * would give up the refusal that works today to close a window no unit can close here. + */ + private confineForMcpWrite( + source: "global" | "project", + // The root chosen by the same write: reading it twice lets a workspace refresh between the + // path lookup and this check reject a valid project write with a confinement error. + workspacePath: string | undefined = this.providerRef.deref()?.cwd ?? getWorkspacePath(), + ): string | undefined { + if (source !== "project") { + return undefined + } + return workspacePath + } + // Initialize project-level MCP servers private async initializeProjectMcpServers(): Promise { await this.initializeMcpServers("project") @@ -2038,10 +2074,12 @@ export class McpHub { configUpdate: Record, source: "global" | "project" = "global", ): Promise { - // Determine which config file to update + // Determine which config file to update. The write root is captured once and reused by + // the confinement check below, so a workspace refresh cannot split the two reads. let configPath: string + const writeRoot = source === "project" ? (this.providerRef.deref()?.cwd ?? getWorkspacePath()) : undefined if (source === "project") { - const projectMcpPath = await this.getProjectMcpPath() + const projectMcpPath = await this.getProjectMcpPath(writeRoot) if (!projectMcpPath) { throw new Error("Project MCP configuration file not found") } @@ -2099,7 +2137,10 @@ export class McpHub { } this.isProgrammaticUpdate = true try { - await safeWriteJson(configPath, updatedConfig, { prettyPrint: true }) + await safeWriteJson(configPath, updatedConfig, { + prettyPrint: true, + confineTo: this.confineForMcpWrite(source, writeRoot), + }) } finally { // Reset flag after watcher debounce period (non-blocking) this.flagResetTimer = setTimeout(() => { @@ -2143,10 +2184,12 @@ export class McpHub { // Determine config file based on server source const isProjectServer = serverSource === "project" let configPath: string + const writeRoot = + serverSource === "project" ? (this.providerRef.deref()?.cwd ?? getWorkspacePath()) : undefined if (isProjectServer) { // Get project MCP config path - const projectMcpPath = await this.getProjectMcpPath() + const projectMcpPath = await this.getProjectMcpPath(writeRoot) if (!projectMcpPath) { throw new Error("Project MCP configuration file not found") } @@ -2184,7 +2227,10 @@ export class McpHub { mcpServers: config.mcpServers, } - await safeWriteJson(configPath, updatedConfig, { prettyPrint: true }) + await safeWriteJson(configPath, updatedConfig, { + prettyPrint: true, + confineTo: this.confineForMcpWrite(serverSource, writeRoot), + }) // Update server connections with the correct source await this.updateServerConnections(config.mcpServers, serverSource) @@ -2342,9 +2388,10 @@ export class McpHub { // Determine the correct config path based on the source let configPath: string + const writeRoot = source === "project" ? (this.providerRef.deref()?.cwd ?? getWorkspacePath()) : undefined if (source === "project") { // Get project MCP config path - const projectMcpPath = await this.getProjectMcpPath() + const projectMcpPath = await this.getProjectMcpPath(writeRoot) if (!projectMcpPath) { throw new Error("Project MCP configuration file not found") } @@ -2393,7 +2440,10 @@ export class McpHub { } this.isProgrammaticUpdate = true try { - await safeWriteJson(normalizedPath, config, { prettyPrint: true }) + await safeWriteJson(normalizedPath, config, { + prettyPrint: true, + confineTo: this.confineForMcpWrite(source, writeRoot), + }) } finally { // Reset flag after watcher debounce period (non-blocking) this.flagResetTimer = setTimeout(() => { diff --git a/src/services/mcp/__tests__/McpHub.spec.ts b/src/services/mcp/__tests__/McpHub.spec.ts index 35ff10d64b..31792de545 100644 --- a/src/services/mcp/__tests__/McpHub.spec.ts +++ b/src/services/mcp/__tests__/McpHub.spec.ts @@ -3,7 +3,8 @@ import * as path from "path" import type { Mock } from "vitest" import type { ExtensionContext, Uri } from "vscode" -import type { Client } from "@modelcontextprotocol/sdk/client/index.js" +import { StdioClientTransport } from "@modelcontextprotocol/sdk/client/stdio.js" +import { Client } from "@modelcontextprotocol/sdk/client/index.js" import type { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js" import type { ClineProvider } from "../../../core/webview/ClineProvider" @@ -35,6 +36,7 @@ vi.mock("fs/promises", () => { }) // Import safeWriteJson to use in mocks +import { safeWriteJson } from "../../../utils/safeWriteJson" // Mock safeWriteJson vi.mock("../../../utils/safeWriteJson", () => ({ @@ -154,10 +156,35 @@ vi.mock("chokidar", () => ({ }, })) +// Only getWorkspacePath is replaced; the original module is loaded and spread so its side effect +// (installing String.prototype.toPosix) and its other exports keep working for this file. The fallback +// is what the fallback test inside the confinement describe below exercises, so it must be distinguishable +// from any workspace path a test could otherwise compute. +vi.mock("../../../utils/path", async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, getWorkspacePath: vi.fn(() => "/fallback/workspace") } +}) + describe("McpHub", () => { let mcpHub: McpHubType let mockProvider: Partial + // A fully typed connection double: the SDK constructors are mocked at the top of the + // file, so no cast is needed to build a connected connection here. Shared by the + // write-confinement tests for every project-capable entry point. + const connectionFor = (source: "global" | "project"): ConnectedMcpConnection => ({ + type: "connected", + server: { + name: "test-server", + config: JSON.stringify({ type: "stdio", command: "node", args: ["test.js"], alwaysAllow: [] }), + status: "connected", + source, + errorHistory: [], + }, + client: new Client({ name: "test-client", version: "1.0.0" }), + transport: new StdioClientTransport({ command: "node", args: ["test.js"] }), + }) + // Store original console methods const originalConsoleError = console.error const originalPlatform = Object.getOwnPropertyDescriptor(process, "platform") @@ -1205,7 +1232,148 @@ describe("McpHub", () => { }) }) + describe("project-scoped write confinement at every call site", () => { + // safeWriteJson resolves the publish target with realpath before staging beside it, so + // a project .roo/mcp.json that links outside the workspace would be written THROUGH the + // link unless the caller declares the workspace as the confinement root. Every + // project-capable entry point has to pass it. A lower-level safeWriteJson test cannot + // see a missing MCP argument (the writer is mocked here), so each call site is pinned. + const projectConfigJson = JSON.stringify({ + mcpServers: { "test-server": { type: "stdio", command: "node", args: ["test.js"], timeout: 60 } }, + }) + + it("confines a project-scoped timeout write to the workspace root", async () => { + Object.defineProperty(mockProvider, "cwd", { value: "/mock/workspace", configurable: true }) + vi.mocked(fs.readFile).mockResolvedValueOnce(projectConfigJson) + mcpHub.connections = [connectionFor("project")] + + await mcpHub.updateServerTimeout("test-server", 120) + + const write = vi.mocked(safeWriteJson).mock.calls.find((call) => String(call[0]).includes(".roo")) + expect(write).toBeDefined() + expect(write![2]).toEqual(expect.objectContaining({ confineTo: "/mock/workspace" })) + }) + + it("leaves a global-scoped timeout write unconstrained", async () => { + Object.defineProperty(mockProvider, "cwd", { value: "/mock/workspace", configurable: true }) + // The global path creates the default settings file when it is missing, and that + // write's merge callback reads the file; updateServerConfig then reads it too. + vi.mocked(fs.readFile).mockResolvedValueOnce(projectConfigJson) + vi.mocked(fs.readFile).mockResolvedValueOnce(projectConfigJson) + mcpHub.connections = [connectionFor("global")] + + await mcpHub.updateServerTimeout("test-server", 120) + + const writes = vi + .mocked(safeWriteJson) + .mock.calls.filter((call) => String(call[0]).includes("mcp_settings")) + expect(writes.length).toBeGreaterThan(0) + for (const write of writes) { + expect(write[2]?.confineTo).toBeUndefined() + } + }) + + it("confines a project-scoped server deletion to the workspace root", async () => { + Object.defineProperty(mockProvider, "cwd", { value: "/mock/workspace", configurable: true }) + vi.mocked(fs.readFile).mockResolvedValueOnce(projectConfigJson) + mcpHub.connections = [connectionFor("project")] + + await mcpHub.deleteServer("test-server", "project") + + const write = vi.mocked(safeWriteJson).mock.calls.find((call) => String(call[0]).includes(".roo")) + expect(write).toBeDefined() + expect(write![2]).toEqual(expect.objectContaining({ confineTo: "/mock/workspace" })) + }) + + it("leaves a global-scoped server deletion unconstrained", async () => { + Object.defineProperty(mockProvider, "cwd", { value: "/mock/workspace", configurable: true }) + vi.mocked(fs.readFile).mockResolvedValueOnce(projectConfigJson) + vi.mocked(fs.readFile).mockResolvedValueOnce(projectConfigJson) + mcpHub.connections = [connectionFor("global")] + + await mcpHub.deleteServer("test-server", "global") + + const writes = vi + .mocked(safeWriteJson) + .mock.calls.filter((call) => String(call[0]).includes("mcp_settings")) + expect(writes.length).toBeGreaterThan(0) + for (const write of writes) { + expect(write[2]?.confineTo).toBeUndefined() + } + }) + it("uses the fallback workspace path for both the settings path and the confinement root when the provider carries no cwd", async () => { + // The provider is reachable but has no cwd, which is the state the `?? getWorkspacePath()` fallback at + // src/services/mcp/McpHub.ts:2080 exists for. The row pins that ONE root serves both purposes: the + // project settings path is built from the fallback and the confinement root handed to safeWriteJson is + // that same fallback, so a project write taken without a provider cwd is confined to the very directory + // it writes into rather than going unconstrained or being confined to a different directory. cwd is a + // getter on ClineProvider, so the test installs the absent value. + Object.defineProperty(mockProvider, "cwd", { value: undefined, configurable: true }) + vi.mocked(fs.readFile).mockResolvedValueOnce(projectConfigJson) + mcpHub.connections = [connectionFor("project")] + + await mcpHub.updateServerTimeout("test-server", 120) + + const write = vi.mocked(safeWriteJson).mock.calls.find((call) => String(call[0]).includes(".roo")) + expect(write).toBeDefined() + expect(String(write![0]).split("\\").join("/")).toContain("/fallback/workspace/.roo/mcp.json") + expect(write![2]).toEqual(expect.objectContaining({ confineTo: "/fallback/workspace" })) + }) + }) + describe("toggleToolAlwaysAllow", () => { + const projectConnection = (source: "global" | "project" = "project"): ConnectedMcpConnection => + connectionFor(source) + + it("confines a project-scoped allowlist write to the workspace root", async () => { + // A repository can ship .roo/mcp.json as a symlink to a file outside the workspace. + // safeWriteJson resolves the publish target before staging, so without confinement an + // allowlist edit would replace that outside file. The workspace root has to be handed + // over as confineTo so the write fails closed instead. + // cwd is a read-only getter on ClineProvider, so the test installs the value. + Object.defineProperty(mockProvider, "cwd", { value: "/mock/workspace", configurable: true }) + vi.mocked(fs.readFile).mockResolvedValueOnce( + JSON.stringify({ + mcpServers: { + "test-server": { type: "stdio", command: "node", args: ["test.js"], alwaysAllow: [] }, + }, + }), + ) + mcpHub.connections = [projectConnection()] + + await mcpHub.toggleToolAlwaysAllow("test-server", "project", "new-tool", true) + + const write = vi.mocked(safeWriteJson).mock.calls.find((call) => String(call[0]).includes("mcp.json")) + expect(write).toBeDefined() + expect(write![2]).toEqual(expect.objectContaining({ confineTo: "/mock/workspace" })) + }) + + it("leaves a global-scoped allowlist write unconstrained", async () => { + // Global settings live in the user's own settings directory, which is not under the + // workspace; confining them would break every global edit. + Object.defineProperty(mockProvider, "cwd", { value: "/mock/workspace", configurable: true }) + mcpHub.connections = [projectConnection("global")] + + await mcpHub.toggleToolAlwaysAllow("test-server", "global", "another-tool", true) + + // Name the exact call instead of picking any match: a find on the substring + // "mcp" returns the FIRST mcp-named write, which need not be the allowlist + // write - the default-creation write getMcpSettingsFilePath makes when the + // settings file is missing (McpHub.ts:513) also matches, and it never passes + // confineTo, so the assertion could judge that write and stay green while a + // confinement root on the real allowlist write went unnoticed. Check every + // write to the settings file, as the timeout and delete tests above do. + const writes = vi + .mocked(safeWriteJson) + .mock.calls.filter((call) => String(call[0]).endsWith("mcp_settings.json")) + expect(writes.length).toBeGreaterThan(0) + // undefined confineTo is the unconstrained case: safeWriteJson only checks the path + // when a confinement root is supplied. + for (const write of writes) { + expect(write[2]?.confineTo).toBeUndefined() + } + }) + it("should add tool to always allow list when enabling", async () => { const mockConfig = { mcpServers: { diff --git a/src/utils/__tests__/fileLock.canonicalKey.spec.ts b/src/utils/__tests__/fileLock.canonicalKey.spec.ts new file mode 100644 index 0000000000..c60afdc56b --- /dev/null +++ b/src/utils/__tests__/fileLock.canonicalKey.spec.ts @@ -0,0 +1,71 @@ +// npx vitest run utils/__tests__/fileLock.canonicalKey.spec.ts + +import path from "path" +import type { BigIntStats } from "fs" +import * as fs from "fs/promises" +import * as lockfile from "proper-lockfile" +import { withFileLock } from "../fileLock" +import { resolveLockKey } from "../../services/file-safety/safeWriteText" + +vi.mock("proper-lockfile", () => ({ + lock: vi.fn(async () => async () => {}), + unlock: vi.fn(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 mockedLock = vi.mocked(lockfile.lock) + +const dir = path.resolve(path.join(path.sep + "work", "history")) +const referent = path.join(dir, "item.json") +const alias = path.join(dir, "alias.json") + +beforeEach(() => { + mockedRealpath.mockReset() + mockedLstat.mockReset() + mockedReadlink.mockReset() + mockedLock.mockReset() + // alias.json is a symlink to item.json; everything else resolves to itself. + mockedRealpath.mockImplementation(async (target) => (target === alias ? referent : String(target))) + // fs/promises types lstat as returning BigIntStats and this mock only ever needs + // isSymbolicLink(), so the assertion is single-step and names the subset the double + // actually fakes. AGENTS.md asks for a reason next to a cast a typed API cannot + // express: no test in this file reads a bigint field, so no bigint field is faked. + mockedLstat.mockImplementation(async () => ({ isSymbolicLink: () => false }) as BigIntStats) + mockedReadlink.mockRejectedValue(Object.assign(new Error("not a link"), { code: "EINVAL" })) +}) + +describe("withFileLock lock-key canonicalization", () => { + it("locks the canonical referent that safeWriteJson locks, not the raw alias", async () => { + // The compound-atomicity contract of task-history deletion is that an + // unlink under this helper and a safeWriteJson read-modify-write on the + // same file serialize. That holds only while both compute ONE key: + // safeWriteJson locks resolveLockKey(target), so withFileLock must too. + // Locking the raw alias here would watch a different lock file and let + // the deletion interleave with the merge write. + await withFileLock(alias, async () => undefined) + + expect(mockedLock).toHaveBeenCalledTimes(1) + expect(mockedLock.mock.calls[0][0]).toBe(referent) + // The shared-key contract stated directly: whatever safeWriteJson would + // lock for the same path is exactly what this helper locked. + expect(mockedLock.mock.calls[0][0]).toBe(await resolveLockKey(alias)) + }) + + it("hands the operation the caller's own path so its unlink keeps alias semantics", async () => { + // unlink(alias) removes the link; unlink(referent) removes the target + // file. Deletion must act on the path the caller named, even though the + // lock is taken on the canonical key. + const seen: string[] = [] + await withFileLock(alias, async (absoluteFilePath) => { + seen.push(absoluteFilePath) + }) + expect(seen).toEqual([alias]) + }) +}) diff --git a/src/utils/__tests__/lockKeyAlignment.spec.ts b/src/utils/__tests__/lockKeyAlignment.spec.ts new file mode 100644 index 0000000000..aeddc73c3b --- /dev/null +++ b/src/utils/__tests__/lockKeyAlignment.spec.ts @@ -0,0 +1,98 @@ +// Pairing test for the ask in review comment 4545130063 on pull 1913, "Use the same lock key as +// `safeWriteJson`.", paid on pull 2433, whose src/utils/fileLock.ts blob 6ca70045661c is +// byte-identical to the blob 1913 carried before that fix. withFileLock (the door a task-history +// deletion comes through), safeWriteJson and the protocol inside acquireFileLock must all hold the +// canonical referent key for a symlink alias. On this head both callers already compute the +// canonical key for themselves - fileLock.ts:86 and safeWriteJson.ts:248 - so the first two rows +// below pass with or without the canonicalization inside acquireFileLock. The third row is the one +// that pins that line: take it out and that row fails, which is what makes this an answer to the +// ask rather than a mirror of the fix. +import * as fs from "fs/promises" +import * as os from "os" +import * as path from "path" +import type { BigIntStats } from "fs" +import * as lockfile from "proper-lockfile" +import { safeWriteJson } from "../safeWriteJson" +import { acquireFileLock, withFileLock } from "../fileLock" +import { resolveLockKey } from "../../services/file-safety/safeWriteText" + +vi.mock("proper-lockfile", () => ({ + lock: vi.fn(async () => async () => {}), + unlock: vi.fn(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 mockedLock = vi.mocked(lockfile.lock) + +let dir = "" +let referent = "" +let alias = "" + +beforeAll(async () => { + dir = await fs.mkdtemp(path.join(os.tmpdir(), "lockkey-alignment-")) + referent = path.join(dir, "task.json") + alias = path.join(dir, "alias.json") + await fs.writeFile(referent, "{}", "utf8") +}) + +afterAll(async () => { + if (dir) await fs.rm(dir, { recursive: true, force: true }) +}) + +beforeEach(() => { + mockedRealpath.mockReset() + mockedLstat.mockReset() + mockedReadlink.mockReset() + mockedLock.mockReset() + // alias.json is a symlink to task.json; every other path resolves to itself. + mockedRealpath.mockImplementation(async (target) => (String(target) === alias ? referent : String(target))) + // fs/promises types lstat as returning BigIntStats and this double only ever needs isSymbolicLink(). + // AGENTS.md asks for a reason beside a cast a typed API cannot express: no row in this file reads a + // bigint field, so no bigint field is faked. + mockedLstat.mockImplementation(async () => ({ isSymbolicLink: () => false }) as BigIntStats) + mockedReadlink.mockRejectedValue(Object.assign(new Error("not a link"), { code: "EINVAL" })) +}) + +describe("lock-key alignment between withFileLock and safeWriteJson", () => { + it("locks the canonical referent for an alias, whichever door the caller comes through", async () => { + await withFileLock(alias, async () => undefined) + const deletionKey = mockedLock.mock.calls[0][0] + await safeWriteJson(alias, { pinned: 1 }) + const writeKeys = mockedLock.mock.calls.map((call) => call[0]) + const writeKey = writeKeys[writeKeys.length - 1] + expect(deletionKey).toBe(referent) + expect(writeKey).toBe(referent) + expect(deletionKey).toBe(writeKey) + expect(deletionKey).toBe(await resolveLockKey(alias)) + }) + + it("keeps one key for a plain path and hands the operation its own path", async () => { + let seenByOperation = "" + await withFileLock(referent, async (absoluteFilePath) => { + seenByOperation = absoluteFilePath + }) + const plainKey = mockedLock.mock.calls[0][0] + await safeWriteJson(referent, { pinned: 2 }) + const plainWriteKeys = mockedLock.mock.calls.map((call) => call[0]) + expect(plainKey).toBe(plainWriteKeys[plainWriteKeys.length - 1]) + expect(plainKey).toBe(await resolveLockKey(referent)) + expect(seenByOperation).toBe(referent) + }) + + it("locks the canonical referent when a caller comes through acquireFileLock itself", async () => { + // The row that pins the canonicalization inside the shared protocol: a caller that hands the + // protocol the alias directly must still get the referent, because no caller spelling is trusted + // any more than the two above. + await acquireFileLock(alias) + expect(mockedLock.mock.calls.length).toBe(1) + expect(mockedLock.mock.calls[0][0]).toBe(referent) + expect(mockedLock.mock.calls[0][0]).toBe(await resolveLockKey(alias)) + }) +}) diff --git a/src/utils/__tests__/safeWriteJson.interruption.test.ts b/src/utils/__tests__/safeWriteJson.interruption.test.ts index c9c5993f84..45fa6a735e 100644 --- a/src/utils/__tests__/safeWriteJson.interruption.test.ts +++ b/src/utils/__tests__/safeWriteJson.interruption.test.ts @@ -32,12 +32,44 @@ const targetPath = path.resolve(process.env.INTERRUPTION_TARGET_PATH) const fsPromises = require("fs/promises") const actualRename = fsPromises.rename +// Identity, not spelling. The writer renames to the publish target it resolved through +// symlinks, and node's async fs.realpath expands an NTFS 8.3 alias, so on a runner whose temp +// path is reported as an alias (the normal windows-latest condition: the profile is RUNNER~1 +// there) the parent's authorized spelling and the writer's rename destination are different +// strings for one file. Text equality here never fires, every case prints WRITE_COMPLETED, and +// the injection silently stops being an injection. Decide what the destination IS: same name, +// and the same directory by dev+ino. The basenames under test are unique inside the storage +// directory, so the pair cannot land on another file. +const targetBase = path.basename(targetPath) +function sameTargetAsAuthorized(candidate) { + if (path.basename(String(candidate)) !== targetBase) { + return false + } + const resolved = path.resolve(String(candidate)) + if (resolved === targetPath) { + return true + } + let candidateDir = null + let authorizedDir = null + try { + candidateDir = fs.statSync(path.dirname(resolved), { bigint: true }) + } catch { + return false + } + try { + authorizedDir = fs.statSync(path.dirname(targetPath), { bigint: true }) + } catch { + return false + } + return candidateDir.dev === authorizedDir.dev && candidateDir.ino === authorizedDir.ino +} + // Fault injection: hard-kill before the replacement lands. Any rename -// whose destination is the canonical path is the commit step, whatever +// whose destination is the publish target is the commit step, whatever // replacement protocol the writer uses; exit before performing it. let commitAttempts = 0 fsPromises.rename = async function interruptionRename(oldPath, newPath) { - if (path.resolve(String(newPath)) === targetPath) { + if (sameTargetAsAuthorized(newPath)) { commitAttempts++ if (process.env.INTERRUPTION_MODE === "fallback" && commitAttempts === 1) { return Promise.reject(Object.assign(new Error("Injected EPERM"), { code: "EPERM" })) diff --git a/src/utils/__tests__/safeWriteJson.lockKey.spec.ts b/src/utils/__tests__/safeWriteJson.lockKey.spec.ts new file mode 100644 index 0000000000..051b2e656f --- /dev/null +++ b/src/utils/__tests__/safeWriteJson.lockKey.spec.ts @@ -0,0 +1,300 @@ +// 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 extra resolves are the post-lock passes: the publish target is resolved, the lock key + // is re-derived from the alias, and every derivation canonicalizes through realpath. The final + // stability check no longer looks the alias up a second time: it canonicalizes the directory + // of the target that was stored under the lock, so the flow makes one fewer realpath than + // it did while that check re-derived from the alias. All of them run after the lock was + // taken, so they do not change the lock the caller queued behind. The two trailing lstat + // calls belonged to the delegation flow's staging-path checks; the replacement protocol + // stages the temp itself and checks no identity beside it, so they are gone with the + // delegation. + expect(order).toEqual([ + "resolve-failed", + "lstat", + "resolve", + "resolve", + "lock", + "resolve", + "resolve", + "resolve", + "resolve", + ]) + 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("releases the first referent lock and acquires the new referent lock when the alias moves under the lock", async () => { + const order: string[] = [] + const dir = await makeDir("lockswitch-") + const referentA = path.join(dir, "history_item_a.json") + const referentB = path.join(dir, "history_item_b.json") + currentLink = path.join(dir, "link.json") + await fs.writeFile(referentA, JSON.stringify({ id: "a" }), "utf8") + await fs.writeFile(referentB, JSON.stringify({ id: "b" }), "utf8") + + // The alias points at A when the lock key is computed and at B by the time that lock is held. + // A writer that keeps A's lock merges into B while a second writer queues behind A, so one side + // of the update is lost. The reacquisition pass must release A and take B before reading for + // the merge. A mock that always returned one referent could not tell that apart from skipping + // the pass, so this row drives the change under the lock. + let aliasTarget = referentA + mockedRealpath.mockImplementation(async (target) => + String(target) === currentLink ? aliasTarget : String(target), + ) + mockedLstat.mockImplementation(async (target) => symlinkStat(target)) + mockedReadlink.mockImplementation(async (target) => + target === currentLink ? aliasTarget : Promise.reject(new Error("not a link")), + ) + mockedAcquireFileLock.mockImplementation(async (key) => { + order.push("acquire:" + (key === referentA ? "A" : "B")) + if (key === referentA) { + aliasTarget = referentB + } + return async () => { + order.push("release:" + (key === referentA ? "A" : "B")) + } + }) + + await safeWriteJson(currentLink, { id: "task-1" }) + + expect(mockedAcquireFileLock.mock.calls.map((call) => call[0])).toEqual([referentA, referentB]) + expect(order).toEqual(["acquire:A", "release:A", "acquire:B", "release:B"]) + expect(JSON.parse(await fs.readFile(referentB, "utf8"))).toEqual({ id: "task-1" }) + expect(JSON.parse(await fs.readFile(referentA, "utf8"))).toEqual({ id: "a" }) + }) + it("refuses to publish when the captured target sits under a different lock than the one held", async () => { + const order: string[] = [] + const dir = await makeDir("captured-") + const referentA = path.join(dir, "history_item_a.json") + const referentB = path.join(dir, "history_item_b.json") + currentLink = path.join(dir, "link.json") + await fs.writeFile(referentA, JSON.stringify({ id: "a" }), "utf8") + await fs.writeFile(referentB, JSON.stringify({ id: "b" }), "utf8") + + // A peer retargets the alias from A to B while this writer is taking A's lock, and moves it back + // to A before the flow recomputes the key under that lock. The recomputation therefore agrees + // with the key held, the loop that would move the lock to the current referent breaks on its + // first pass, and the only thing standing between this write and the commit rename is the + // target it captured while the alias pointed at B. A check that re-derives that target from a + // second lookup of the alias sees A, agrees with the key it holds, and renames B's contents + // away while still holding A's lock. A check that canonicalizes the captured target sees B, + // disagrees with the key A, and refuses. The alias lookups below record the A to B to A rather + // than narrating it, so the row shows the sequence it depends on. + const aliasLookups: string[] = [] + mockedRealpath.mockImplementation(async (target) => { + const spelling = String(target) + if (spelling !== currentLink) { + return spelling + } + const referent = aliasLookups.length === 1 ? referentB : referentA + aliasLookups.push(referent) + return referent + }) + mockedLstat.mockImplementation(async (target) => symlinkStat(target)) + mockedReadlink.mockImplementation(async (target) => + target === currentLink ? referentA : Promise.reject(new Error("not a link")), + ) + mockedAcquireFileLock.mockImplementation(async (key) => { + order.push(key === referentA ? "acquire:A" : "acquire:other") + return async () => { + order.push(key === referentA ? "release:A" : "release:other") + } + }) + + await expect(safeWriteJson(currentLink, { id: "task-1" })).rejects.toThrow( + "the resolved target kept changing while acquiring the lock", + ) + + // The alias moved A to B while the lock was taken and back to A before the key recomputation. + // The mismatch belongs to the captured target, not to the alias, which is why the guard has to + // read the target this write is about to rename onto instead of looking the alias up again. + expect(aliasLookups).toEqual([referentA, referentB, referentA]) + // The recomputation agreed with the held key, so no lock was moved: A is the only lock this + // write acquired, and it is the lock the finally released. + expect(order).toEqual(["acquire:A", "release:A"]) + expect(mockedAcquireFileLock.mock.calls.map((call) => call[0])).toEqual([referentA]) + // Refused before the merge read and before staging, so nothing was written beside either target. + const left = await fs.readdir(dir) + expect(left.filter((entry) => entry.includes(".new_") || entry.includes(".bak_"))).toEqual([]) + // Both targets still hold the bytes they had before this write tried. + expect(JSON.parse(await fs.readFile(referentA, "utf8"))).toEqual({ id: "a" }) + expect(JSON.parse(await fs.readFile(referentB, "utf8"))).toEqual({ id: "b" }) + }) +}) + +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 30ac500212..fd767c419f 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -3,7 +3,7 @@ import { Writable } from "stream" import * as path from "path" import * as os from "os" -import { safeWriteJson } from "../safeWriteJson" +import { ConfinedPathEscapeError, safeWriteJson, undoEscapedPublish } from "../safeWriteJson" // Capture actual implementations before the vi.mock factory runs, // so they are never wrapped by vi.fn() — avoids infinite recursion when @@ -28,6 +28,7 @@ vi.mock("fs/promises", async () => { unlink: vi.fn(actual.unlink), access: vi.fn(actual.access), stat: vi.fn(actual.stat), + chmod: vi.fn(actual.chmod), open: vi.fn(actual.open), mkdtemp: vi.fn(actual.mkdtemp), rm: vi.fn(actual.rm), @@ -756,6 +757,100 @@ describe("safeWriteJson", () => { expect(await readFileContent(currentTestFilePath)).toEqual({ saved: true }) }) + // The two steps that publish a staged file both touch the staged path, in an order that matters: + // _copyTargetMode chmods it to the target's mode and _flushFileToDisk opens it "r+" to + // sync it. Run the other way round, a target whose mode withholds write access hands the + // staged file a mode that denies the flush's own open, and the flush only logs what it + // could not do, so the fsync is skipped while the publish still succeeds. Asserted as the + // order of the two fs/promises calls this file already mocks: the order is the same fact + // on every platform, while the mode bits that make the denial bite are enforced only where + // the runner honours them. + test("flushes the staged file before the target mode is copied onto it", async () => { + const actual = await vi.importActual("fs/promises") + const targetModeBefore = ((await actual.stat(currentTestFilePath)).mode & 0o777) >>> 0 + const steps: string[] = [] + let copiedMode: number | undefined + vi.mocked(fs.open).mockImplementation(async (filePath, flags) => { + steps.push("flush-open") + return actual.open(filePath, flags) + }) + vi.mocked(fs.chmod).mockImplementation(async (filePath, mode) => { + steps.push("mode-copy") + copiedMode = Number(mode) + return actual.chmod(filePath, mode) + }) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await safeWriteJson(currentTestFilePath, { ordered: true }) + + expect(steps).toEqual(["flush-open", "mode-copy"]) + // The flush opened the staged file for writing and sync succeeded on it: a swallowed + // flush failure logs, so no log means the open the mode copy would have denied. + expect(consoleErrorSpy).not.toHaveBeenCalled() + // The copy still runs, and still copies the mode the target itself reported. + expect(copiedMode).toBe(targetModeBefore) + expect(await readFileContent(currentTestFilePath)).toEqual({ ordered: true }) + }, 20_000) + + // The mode half of the same finding, run for real on a platform that honours mode bits. A target + // whose mode is 0o444 withholds write access, so a mode copy that ran first would hand the staged + // file that read-only mode and the flush open of this very file would then fail: the flush only + // logs what it could not do, so the sync would be skipped while the publish still succeeded. This + // row therefore asserts the two facts the ordering comment above was standing in for, on the + // handle the publish opened and on the target after the rename, and it drives them with a real + // 0o444 mode instead of a recorded call order. Skipped on windows, where 0o444 lands as the + // read-only attribute and the commit rename reaches the EPERM fallback for reasons that belong + // to the platform and not to this order. + test.skipIf(process.platform === "win32")( + "publishes onto a 0o444 target, syncing the staged file and leaving the published mode 0o444", + async () => { + const actual = await vi.importActual("fs/promises") + await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify({ before: true })) + await actual.chmod(currentTestFilePath, 0o444) + // The precondition, asserted so that a runner whose filesystem ignores mode bits turns this + // row red instead of passing it for the wrong reason. + const targetModeBefore = ((await actual.stat(currentTestFilePath)).mode & 0o777) >>> 0 + expect(targetModeBefore).toBe(0o444) + const steps: string[] = [] + const syncPaths: string[] = [] + let flushPath: string | undefined + vi.mocked(fs.open).mockImplementation(async (filePath, flags) => { + flushPath = String(filePath) + steps.push("flush-open") + const handle = await actual.open(filePath, flags) + const realSync = handle.sync.bind(handle) + vi.spyOn(handle, "sync").mockImplementation(async () => { + syncPaths.push(String(filePath)) + steps.push("sync") + await realSync() + }) + return handle + }) + vi.mocked(fs.chmod).mockImplementation(async (filePath, mode) => { + steps.push("mode-copy") + return actual.chmod(filePath, mode) + }) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + await safeWriteJson(currentTestFilePath, { after: true }) + // The handle belongs to the staged file and not to the target, and its sync ran against it. + expect(flushPath).toBeDefined() + expect(path.basename(String(flushPath))).toContain(".new_") + expect(syncPaths).toEqual([String(flushPath)]) + // It ran before the target mode was copied onto the staged file, the only order in which a + // 0o444 target cannot deny the open above. + expect(steps).toEqual(["flush-open", "sync", "mode-copy"]) + expect(consoleErrorSpy).not.toHaveBeenCalled() + consoleErrorSpy.mockRestore() + expect(await readFileContent(currentTestFilePath)).toEqual({ after: true }) + // The published target still reads the mode its owner set and not the staged file mode. + expect(((await actual.stat(currentTestFilePath)).mode & 0o777) >>> 0).toBe(0o444) + const leftovers = (await fs.readdir(path.dirname(currentTestFilePath))).filter( + (entry) => entry.includes(".new_") || entry.endsWith(".lock"), + ) + expect(leftovers).toEqual([]) + }, + ) + test("should garbage collect orphaned replacement files when the canonical path exists", async () => { const initialData = { message: "Initial content" } @@ -818,4 +913,740 @@ describe("safeWriteJson", () => { const content = await readFileContent(currentTestFilePath) expect(content).toEqual(newData) }) + + // 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. vi.doUnmock is the non-hoisted counterpart + // of the vi.doMock above; vi.unmock is hoisted to the top of the file, so it would + // not undo this registration from here. + realpathSpy.mockRestore() + vi.doUnmock("proper-lockfile") + vi.resetModules() + consoleErrorSpy.mockRestore() + } + }) + + test("rejects a confined write whose target is outside the confined directory", async () => { + const scope = path.join(tempDir, "project") + await fs.mkdir(scope) + const outside = path.join(tempDir, "elsewhere.json") + + // No symlink needed: the check runs on the resolved publish target, so an + // out-of-scope path is rejected on every platform, and it is rejected before the + // lock is taken and before anything is staged. + await expect(safeWriteJson(outside, { mcpServers: {} }, { confineTo: scope })).rejects.toThrow( + ConfinedPathEscapeError, + ) + + const left = await fs.readdir(tempDir) + expect(left).not.toContain("elsewhere.json") + expect(left.filter((entry) => entry.includes(".new_") || entry.endsWith(".lock"))).toEqual([]) + }) + + test.skipIf(process.platform === "win32")( + "rejects a confined write whose symlink resolves outside the confined directory", + async () => { + const projectDir = path.join(tempDir, "project") + await fs.mkdir(projectDir) + const outside = path.join(tempDir, "outside.json") + await fsSyncActual.promises.writeFile(outside, JSON.stringify({ secret: "original" }), "utf8") + // A repository that plants its project settings file as a link to somewhere else + // must not receive the settings write at the linked path. The caller picked + // projectDir/mcp.json from the workspace, so it declares that scope. + const projectConfig = path.join(projectDir, "mcp.json") + await fs.symlink(outside, projectConfig) + + await expect(safeWriteJson(projectConfig, { mcpServers: {} }, { confineTo: projectDir })).rejects.toThrow( + ConfinedPathEscapeError, + ) + + // The linked file is untouched and nothing was staged beside it. + expect(JSON.parse(await fsSyncActual.promises.readFile(outside, "utf8"))).toEqual({ secret: "original" }) + const entries = await fs.readdir(tempDir) + expect(entries).toContain("outside.json") + expect( + entries.filter( + (entry) => entry.includes(".new_") || entry.includes("safeWriteText") || entry.endsWith(".lock"), + ), + ).toEqual([]) + }, + ) + + test.skipIf(process.platform === "win32")( + "confines a write whose symlink referent stays inside the confined directory", + async () => { + const projectDir = path.join(tempDir, "project-in") + await fs.mkdir(projectDir) + const referent = path.join(projectDir, "real-mcp.json") + await fsSyncActual.promises.writeFile(referent, JSON.stringify({ mcpServers: {} }), "utf8") + const alias = path.join(projectDir, "mcp.json") + await fs.symlink(referent, alias) + + // Confining is about the scope, not about forbidding links: a link that stays + // inside the project still publishes to its referent. + await safeWriteJson( + alias, + { mcpServers: { local: { url: "http://localhost" } } }, + { confineTo: projectDir }, + ) + + expect(JSON.parse(await fsSyncActual.promises.readFile(referent, "utf8"))).toEqual({ + mcpServers: { local: { url: "http://localhost" } }, + }) + }, + ) + + test.skipIf(process.platform === "win32")( + "confines a scope path that itself runs through a symlink and does not exist yet", + async () => { + const real = path.join(tempDir, "real-project") + await fs.mkdir(real) + const alias = path.join(tempDir, "alias-project") + await fs.symlink(real, alias) + // The scope is declared through the alias, and the directory it names does not + // exist yet. Resolving it lexically would compare an unresolved scope against a + // fully resolved target and reject a write that is in fact inside the project - + // the macOS /var -> /private/var shape. The nearest existing ancestor is resolved + // and the remainder re-joined instead. + const nested = path.join(alias, "nested") + const target = path.join(nested, "mcp.json") + + await safeWriteJson(target, { mcpServers: {} }, { confineTo: nested }) + + expect( + JSON.parse(await fsSyncActual.promises.readFile(path.join(real, "nested", "mcp.json"), "utf8")), + ).toEqual({ mcpServers: {} }) + }, + ) + + // The two confinement rows above skip Windows because they need a symlink. Confinement itself is + // not the skipped property: a plain in-scope write must succeed on every platform, including + // inside a nonempty directory. Without this row a Windows regression that rejects valid + // project-settings saves would pass the whole confinement matrix unnoticed, because every + // other accept-path row in it is skipped on that platform. + test("confines a plain write inside a nonempty confined directory on every platform", async () => { + const projectDir = path.join(tempDir, "plain-confine") + await fs.mkdir(projectDir) + await fs.writeFile(path.join(projectDir, "unrelated.json"), JSON.stringify({ keep: true }), "utf8") + const target = path.join(projectDir, "mcp.json") + + await safeWriteJson(target, { mcpServers: { local: { url: "http://localhost" } } }, { confineTo: projectDir }) + + expect(JSON.parse(await fs.readFile(target, "utf8"))).toEqual({ + mcpServers: { local: { url: "http://localhost" } }, + }) + expect(JSON.parse(await fs.readFile(path.join(projectDir, "unrelated.json"), "utf8"))).toEqual({ keep: true }) + }) + + 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 }) + }, + ) + // The two failure paths of _copyTargetMode: a target whose mode cannot be read, and a staged file + // whose mode cannot be set. Both refuse the publish and leave the target exactly as it was: proceeding + // would land a umask-derived mode over a target whose owner had narrowed it, which is the same + // widening the copy exists to prevent. They are driven through the fs/promises mocks rather than a + // real EACCES/EPERM, because forcing either one for real needs privileges a developer box and a CI + // runner neither have. That is also why these two run on every platform while the symlink and mode + // rows above skip windows. + test("refuses the publish and keeps the target's bytes when the target's mode cannot be read", async () => { + await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify({ before: true })) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + // fs.stat is already vi.fn() in the factory above - vi.mocked keeps the real + // implementation reachable once this single call is done. + vi.mocked(fs.stat).mockImplementationOnce(async () => { + const error: NodeJS.ErrnoException = new Error(`EACCES: permission denied, stat '${currentTestFilePath}'`) + error.code = "EACCES" + throw error + }) + + await expect(safeWriteJson(currentTestFilePath, { after: true })).rejects.toThrow(/Could not read the mode of/) + + // The target still holds the bytes it had before the refused write, and the staged file is + // gone: a refused publish must not leave a second copy of the payload beside it. + expect(await readFileContent(currentTestFilePath)).toEqual({ before: true }) + const leftovers = (await fs.readdir(path.dirname(currentTestFilePath))).filter( + (entry) => entry.includes(".new_") || entry.endsWith(".lock"), + ) + expect(leftovers).toEqual([]) + // The refusal names the target it could not read. Pinned to the components that survive + // path resolution on every platform rather than the caller's spelling. + const logged = consoleErrorSpy.mock.calls.map((call: unknown[]) => String(call[0])).join("\n") + expect(logged).toContain(path.basename(path.dirname(currentTestFilePath))) + expect(logged).toContain(path.basename(currentTestFilePath)) + consoleErrorSpy.mockRestore() + }, 20_000) + + test("refuses the publish and keeps the target's bytes when the staged file's mode cannot be set", async () => { + await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify({ before: true })) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + // chmod is a vi.fn() in the factory above for the same reason. + vi.mocked(fs.chmod).mockImplementationOnce(async () => { + const error: NodeJS.ErrnoException = new Error( + `EPERM: operation not permitted, chmod '${currentTestFilePath}'`, + ) + error.code = "EPERM" + throw error + }) + + await expect(safeWriteJson(currentTestFilePath, { after: true })).rejects.toThrow(/Failed to copy mode/) + + expect(await readFileContent(currentTestFilePath)).toEqual({ before: true }) + const leftovers = (await fs.readdir(path.dirname(currentTestFilePath))).filter( + (entry) => entry.includes(".new_") || entry.endsWith(".lock"), + ) + expect(leftovers).toEqual([]) + consoleErrorSpy.mockRestore() + }, 20_000) + + // CWE-732 on the staged file itself: fs.createWriteStream defaults its creation mode to 0o666 + // narrowed only by the process umask, so a payload carrying MCP credentials briefly sits in a + // file every local user can read before the mode copy and the commit rename land. The writer + // pins the creation mode instead. Asserted on the options handed to createWriteStream because + // the mode a freshly created file answers to is umask- and platform-dependent, and a runner's + // umask is not the property under test. + test("creates the staged file with an explicit restrictive mode instead of the umask default", async () => { + const streamCallsBefore = vi.mocked(fsSyncActual.createWriteStream).mock.calls.length + await safeWriteJson(currentTestFilePath, { value: 1 }) + const calls = vi.mocked(fsSyncActual.createWriteStream).mock.calls.slice(streamCallsBefore) + expect(calls.length).toBeGreaterThan(0) + const modes = calls.map((call) => (call[1] as { mode?: number } | undefined)?.mode) + expect(modes).toContain(0o600) + expect(await readFileContent(currentTestFilePath)).toEqual({ value: 1 }) + }) + + // Ordering matters for the security guarantee: proper-lockfile creates + // ${lockKey}.lock beside the lock key, and the key is the symlink referent. If + // confinement were checked only after the lock, an out-of-scope target would first + // create a lock directory outside the scope. A lock mock that throws if reached + // proves the check runs first. Written without symlinks so it runs on every lane. + test("rejects an out-of-scope target before the advisory lock is taken", async () => { + vi.resetModules() + const projectDir = path.join(tempDir, "order-project-plain") + await fs.mkdir(projectDir) + const outside = path.join(tempDir, "order-outside-plain.json") + + const realLockfile = await vi.importActual("proper-lockfile") + const lockMockFn = vi.fn(async () => { + throw new Error("lock taken for an out-of-scope target (test)") + }) + vi.doMock("proper-lockfile", () => ({ ...realLockfile, lock: lockMockFn })) + const { safeWriteJson: lockedSafeWriteJson } = await import("../safeWriteJson") + + try { + await expect(lockedSafeWriteJson(outside, { mcpServers: {} }, { confineTo: projectDir })).rejects.toThrow( + /resolves outside the confined directory/, + ) + expect(lockMockFn).not.toHaveBeenCalled() + const entries = await fs.readdir(tempDir) + expect(entries.filter((entry) => entry.endsWith(".lock") || entry.includes(".new_"))).toEqual([]) + } finally { + vi.doUnmock("proper-lockfile") + vi.resetModules() + } + }) + + test("does not create the parent directory of an out-of-scope confined target", async () => { + const projectDir = path.join(tempDir, "scope-dir-project") + await fs.mkdir(projectDir) + // The parent does not exist yet: the mkdir in safeWriteJson would create it - + // a filesystem change outside confineTo - before the confinement check rejected + // the write. + const outside = path.join(tempDir, "scope-missing-parent", "nested.json") + + await expect(safeWriteJson(outside, { mcpServers: {} }, { confineTo: projectDir })).rejects.toThrow( + /resolves outside the confined directory/, + ) + + const entries = await fs.readdir(tempDir) + expect(entries).not.toContain("scope-missing-parent") + expect(entries.filter((entry) => entry.endsWith(".lock") || entry.includes(".new_"))).toEqual([]) + }) + + // A caller that declares a scope must never get the unconstrained path: an + // empty confineTo is a declared scope, not an absent one. This value occurs in + // practice - McpHub.confineForMcpWrite returns getWorkspacePath(), which is "" + // while no workspace folder is open. The truthiness checks skipped both + // confinement gates for "", so the write followed any symlink with no scope + // check at all. + test("rejects a declared-but-empty confineTo instead of writing unconstrained", async () => { + const error = await safeWriteJson(currentTestFilePath, { mcpServers: {} }, { confineTo: "" }).then( + () => undefined, + (reason: unknown) => reason, + ) + expect(error).toBeInstanceOf(ConfinedPathEscapeError) + if (error instanceof ConfinedPathEscapeError) { + // The rejection must name the declared empty root: a substituted root + // (path.resolve("") is the process cwd) would mean confining the write + // to a directory nobody declared. + expect(error.confineTo).toBe("") + } + // The write did not proceed: the target keeps its pre-existing content. + expect(await readFileContent(currentTestFilePath)).toEqual({ initial: "content" }) + }) + + test("rejects an empty confineTo before creating the target parent directory", async () => { + // The same fail-closed decision pinned at the ordering the pre-lock check + // guarantees: a rejected confined write must not create the missing parent + // directory of the target. + const outside = path.join(tempDir, "empty-scope-missing-parent", "nested.json") + + const error = await safeWriteJson(outside, { mcpServers: {} }, { confineTo: "" }).then( + () => undefined, + (reason: unknown) => reason, + ) + expect(error).toBeInstanceOf(ConfinedPathEscapeError) + if (error instanceof ConfinedPathEscapeError) { + expect(error.confineTo).toBe("") + } + + const entries = await fs.readdir(tempDir) + expect(entries).not.toContain("empty-scope-missing-parent") + expect(entries.filter((entry) => entry.endsWith(".lock") || entry.includes(".new_"))).toEqual([]) + }) + + test("rejects a whitespace-only confineTo and names the declared root", async () => { + // path.resolve(" ") is a directory named " " under the process cwd - a + // scope nobody declared. Whitespace-only fails closed with the declared + // root, the same decision as the empty string. + const error = await safeWriteJson(currentTestFilePath, { mcpServers: {} }, { confineTo: " " }).then( + () => undefined, + (reason: unknown) => reason, + ) + expect(error).toBeInstanceOf(ConfinedPathEscapeError) + if (error instanceof ConfinedPathEscapeError) { + expect(error.confineTo).toBe(" ") + } + expect(await readFileContent(currentTestFilePath)).toEqual({ initial: "content" }) + }) + + // The confinement option decides from the resolved publish target and stops at its final + // component. An ancestor DIRECTORY replaced by a link in the window between that + // decision and the commit rename is caught fail-closed: publication takes a path and + // re-resolves the tree at rename time, so the confinement option re-resolves the + // published path after the commit and undoes an escaped write. These two tests are + // that contract as an executable pair - one per outcome of the window. The swap is driven from the merge + // callback, which safeWriteJson invokes after the in-lock confinement check and before + // anything is staged, so the ancestor moves inside the window rather than hopefully so. + // A directory link needs no privilege on win32 as a junction; "dir" is the POSIX + // spelling. Both make the ancestor itself the link, which is the shape under test. + const ancestorLinkKind = process.platform === "win32" ? ("junction" as const) : ("dir" as const) + + function swapAncestorAfterTheCheck(ancestor: string, pointsAt: string): void { + fsSyncActual.renameSync(ancestor, ancestor + "-moved-aside") + fsSyncActual.symlinkSync(pointsAt, ancestor, ancestorLinkKind) + } + + test("refuses and undoes the publish when an ancestor is swapped after the final check", async () => { + const projectDir = path.join(tempDir, "swap-project") + const ancestor = path.join(projectDir, "settings") + await fs.mkdir(ancestor, { recursive: true }) + const target = path.join(ancestor, "mcp.json") + await fs.writeFile(target, JSON.stringify({ inside: true })) + // The swapped-in directory has no file at the target name yet: this is the half of the + // window in which the commit itself succeeds and only the post-commit re-check sees + // the escape. + const swappedIn = path.join(tempDir, "elsewhere", "settings") + await fs.mkdir(swappedIn, { recursive: true }) + + // The confinement option now fails closed on the swapped ancestor: the + // post-commit re-check re-resolves the published path, sees it outside the + // declared scope, removes the escaped file, and reports the escape. + await expect( + safeWriteJson( + target, + { written: "payload" }, + { + confineTo: projectDir, + merge: () => { + swapAncestorAfterTheCheck(ancestor, swappedIn) + return { written: "payload" } + }, + }, + ), + ).rejects.toBeInstanceOf(ConfinedPathEscapeError) + + // The escaped publication was undone: nothing remains outside the scope... + expect(fsSyncActual.existsSync(path.join(swappedIn, "mcp.json"))).toBe(false) + // ...and the file that was inside the scope keeps its pre-write bytes - the + // publish did not reach it under the moved-aside name either. + expect(JSON.parse(fsSyncActual.readFileSync(path.join(ancestor + "-moved-aside", "mcp.json"), "utf8"))).toEqual( + { inside: true }, + ) + // The scope was declared and active in this very fixture: with the ancestor left + // alone, the same option refuses an out-of-scope target. Without this contrast the + // escape above would also be the outcome of a confinement check that never ran. + await expect( + safeWriteJson(path.join(tempDir, "elsewhere", "other.json"), { other: true }, { confineTo: projectDir }), + ).rejects.toThrow(ConfinedPathEscapeError) + }) + + test("undoes an escaped publish only when the bytes on disk are still this write's bytes", async () => { + const outside = path.join(tempDir, "undo-escaped") + await fs.mkdir(outside, { recursive: true }) + const escaped = path.join(outside, "mcp.json") + // A peer wrote its own file at the escaped name after our commit published ours and + // before the post-commit re-check ran. Removing it on the name alone would delete the + // peer's file; the byte comparison must leave it in place. + fsSyncActual.writeFileSync(escaped, JSON.stringify({ peer: true })) + + await undoEscapedPublish(escaped, Buffer.from(JSON.stringify({ ours: true }))) + + expect(fsSyncActual.existsSync(escaped)).toBe(true) + expect(JSON.parse(fsSyncActual.readFileSync(escaped, "utf8"))).toEqual({ peer: true }) + + // The bytes are still ours: the escaped publish is undone. + await undoEscapedPublish(escaped, Buffer.from(JSON.stringify({ peer: true }))) + expect(fsSyncActual.existsSync(escaped)).toBe(false) + + // A missing file and an unreadable byte record are both no-ops, never a throw. + await undoEscapedPublish(escaped, Buffer.from("{}")) + await undoEscapedPublish(path.join(outside, "absent.json"), undefined) + }) + test("leaves the escaped publish alone when the file identity changed although the bytes stayed equal", async () => { + // The row at 1310 varies the BYTES. This one holds the bytes equal and changes the + // file itself between the byte read and the identity re-stat at safeWriteJson.ts:202, + // which is how a peer wins: it writes an equal-length copy after our commit, so the + // byte check passes and only the dev/ino/size comparison can stop the unlink. The swap + // is driven from the byte read itself, the seam this file already uses, so it lands + // inside the window rather than hopefully next to it. + const outside = path.join(tempDir, "identity-undo") + await fs.mkdir(outside, { recursive: true }) + const escaped = path.join(outside, "mcp.json") + const ours = Buffer.from(JSON.stringify({ ours: true })) + fsSyncActual.writeFileSync(escaped, ours) + const actual = await vi.importActual("fs/promises") + vi.mocked(fs.readFile).mockImplementationOnce(() => { + // The swap has to change the file identity on whichever runner executes it. On ext4 an + // unlink plus a create of the same bytes reuses dev, ino and size (measured in WSL2: the + // same inode on both sides of such a swap), so a remove and recreate here let the guard + // at safeWriteJson.ts:202 read an unchanged identity and unlink the publish. A rename + // moves the inode on ext4 and on NTFS alike, and it is how the guarded writer itself + // replaces a file, so the row states the condition it asserts. + const peer = escaped + ".peer" + fsSyncActual.writeFileSync(peer, ours) + fsSyncActual.renameSync(peer, escaped) + return actual.readFile(escaped) + }) + await undoEscapedPublish(escaped, ours) + // Equal bytes, a different file: the identity comparison is what left it in place. + expect(fsSyncActual.existsSync(escaped)).toBe(true) + expect(JSON.parse(fsSyncActual.readFileSync(escaped, "utf8"))).toEqual({ ours: true }) + // The guard is not a blanket refusal: with the identity unchanged the same call + // undoes the publish. + await undoEscapedPublish(escaped, ours) + expect(fsSyncActual.existsSync(escaped)).toBe(false) + }) + // Two payloads against the same fixture. The distinct-bytes case is the one that already + // passed: there the byte comparison inside undoEscapedPublish is what leaves the restored + // original alone. The byte-equal case is the one content cannot tell apart, so it rests on + // the rollback-restored check in safeWriteJson.ts instead. The fixture reaches the restored + // file by rename in both cases - the ancestor swap is a rename and so is the rollback - + // which is what keeps this pair measuring the teardown rather than ext4 reusing dev, ino + // and size across a remove and create, the accident a remove-and-recreate fixture would + // have been reading on a Linux runner. Each case takes its own directories because the + // first leaves the ancestor as a junction or symlink the second could not reuse. + it.each([ + { + label: "distinct staged bytes", + dir: "distinct", + payload: { written: "payload" }, + restored: { inside: true }, + }, + { label: "byte-equal staged bytes", dir: "equal", payload: { inside: true }, restored: { inside: true } }, + ])( + "tears down rollback and staged-file cleanup before reporting a confinement escape ($label)", + async ({ dir, payload, restored }: { dir: string; payload: unknown; restored: unknown }) => { + // The catch-path re-check reported the escape ahead of the backup rollback and the + // staged-file cleanup, so a recheck failure skipped both. This fixture takes the Windows + // EPERM fallback so a backup and a staged file both exist when the failure lands, and + // swaps the ancestor between the fallback two renames via the trace seam, moving the + // ancestor own contents under the pointing link so every path stays reachable and the + // teardown is asserted rather than merely attempted. + const projectDir = path.join(tempDir, "teardown-project-" + dir) + const ancestor = path.join(projectDir, "settings") + await fs.mkdir(ancestor, { recursive: true }) + const target = path.join(ancestor, "mcp.json") + await fs.writeFile(target, JSON.stringify({ inside: true })) + const swapRoot = path.join(tempDir, "teardown-swap-root-" + dir) + await fs.mkdir(swapRoot, { recursive: true }) + const renames: string[][] = [] + let commitAttempts = 0 + vi.mocked(fs.rename).mockImplementation(async (from, to) => { + const source = String(from) + renames.push([source, String(to)]) + if (source.includes(".new_")) { + commitAttempts++ + if (commitAttempts === 1) { + throw Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) + } + if (commitAttempts === 2) { + throw Object.assign(new Error("EIO on the fallback commit"), { code: "EIO" }) + } + } + return fsPromisesActuals.rename!(from, to) + }) + const reason = await safeWriteJson(target, payload, { + confineTo: projectDir, + trace: ({ stage, detail }) => { + if (stage === "replacement-rename" && detail === "eperm-fallback") { + fsSyncActual.renameSync(ancestor, path.join(swapRoot, "settings")) + fsSyncActual.symlinkSync(path.join(swapRoot, "settings"), ancestor, ancestorLinkKind) + } + }, + }).then( + () => undefined, + (rejection: unknown) => rejection, + ) + expect(reason).toBeInstanceOf(ConfinedPathEscapeError) + // A rename FROM the backup path is the rollback; none happened while the escape threw + // ahead of the teardown. + expect(renames.some((call) => call[0].includes(".bak_"))).toBe(true) + // The original content is back where the target lives, not stranded in the backup. + expect(await readFileContent(target)).toEqual(restored) + // The staged .new file is gone rather than left beside it. + expect((await fs.readdir(ancestor)).filter((entry) => entry.includes(".new_"))).toEqual([]) + }, + ) + + test("keeps the restored original when the escape re-check fires over a successful fallback commit", async () => { + // The teardown fixture above fails the fallback commit, so it never reaches the shape the + // finding names. Here the EPERM fallback commits, the post-commit re-check sees the escape, + // the undo removes this write's publish, and the catch path renames the backup back onto the + // target - after which the same re-check fires a second time over the restored file. The + // payload is byte-equal to the content already at the target, which is the condition that + // made the second undo delete what the rollback had just restored: the byte comparison at + // safeWriteJson.ts:197 matches and the identity comparison at :203 sees no change across a + // quiet stat-read-stat, so the unlink follows. Both the ancestor swap and the rollback are + // renames, so this row does not depend on ext4 reusing dev, ino and size across a remove + // and create; it states the condition it asserts. + const projectDir = path.join(tempDir, "restored-project") + const ancestor = path.join(projectDir, "settings") + await fs.mkdir(ancestor, { recursive: true }) + const target = path.join(ancestor, "mcp.json") + await fs.writeFile(target, JSON.stringify({ inside: true })) + const swapRoot = path.join(tempDir, "restored-swap-root") + await fs.mkdir(swapRoot, { recursive: true }) + const renames: string[][] = [] + let commitAttempts = 0 + vi.mocked(fs.rename).mockImplementation(async (from, to) => { + const source = String(from) + renames.push([source, String(to)]) + if (source.includes(".new_")) { + commitAttempts++ + if (commitAttempts === 1) { + // A reader holds the target open: the single rename over it fails with EPERM and the + // writer falls back to backup-then-replace, which then lands. + throw Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) + } + } + return fsPromisesActuals.rename!(from, to) + }) + const reason = await safeWriteJson( + target, + // Byte-equal to the bytes already at the target: the condition the second undo has to + // survive. + { inside: true }, + { + confineTo: projectDir, + trace: ({ stage, detail }) => { + if (stage === "replacement-rename" && detail === "eperm-fallback") { + fsSyncActual.renameSync(ancestor, path.join(swapRoot, "settings")) + fsSyncActual.symlinkSync(path.join(swapRoot, "settings"), ancestor, ancestorLinkKind) + } + }, + }, + ).then( + () => undefined, + (rejection: unknown) => rejection, + ) + expect(reason).toBeInstanceOf(ConfinedPathEscapeError) + // A rename FROM the backup path is the rollback, and it ran. + expect(renames.some((call) => call[0].includes(".bak_"))).toBe(true) + // The second re-check left the restored bytes where the rollback put them instead of + // undoing them as if they were still this write's publish. + expect(await readFileContent(target)).toEqual({ inside: true }) + // Neither a staged file nor a backup is stranded beside the restored original. + expect( + fsSyncActual + .readdirSync(path.join(swapRoot, "settings")) + .filter((entry) => entry.includes(".new_") || entry.includes(".bak_")), + ).toEqual([]) + }) + test("leaves a file this write never published alone when the commit rename is refused", async () => { + // The row above pins what the commit flag skips. This row pins the case the flag exists for: the + // commit rename is refused outright, so this write published nothing, and the escaped location is + // holding bytes only because a peer put them there through the swapped-in directory. Those bytes + // are exactly the ones this write staged, which is the condition the byte comparison inside + // undoEscapedPublish accepts, so content alone cannot tell the two files apart. Deleting them + // here removes a file this write never published: that is the data loss the finding names. + const projectDir = path.join(tempDir, "guard-project") + const ancestor = path.join(projectDir, "settings") + await fs.mkdir(ancestor, { recursive: true }) + const target = path.join(ancestor, "mcp.json") + await fs.writeFile(target, JSON.stringify({ inside: true })) + const swapRoot = path.join(tempDir, "guard-swap-root") + await fs.mkdir(swapRoot, { recursive: true }) + const escaped = path.join(swapRoot, "mcp.json") + const renames: string[][] = [] + vi.mocked(fs.rename).mockImplementation(async (from, to) => { + const source = String(from) + renames.push([source, String(to)]) + if (source.includes(".new_")) { + // Hand the escaped location the bytes this write staged, then refuse the commit. A + // non-EPERM refusal takes no backup, so the rollback below does nothing and the flag + // stays false. + fsSyncActual.writeFileSync(escaped, fsSyncActual.readFileSync(source)) + throw Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + } + return fsPromisesActuals.rename!(from, to) + }) + const reason = await safeWriteJson( + target, + { written: "payload" }, + { + confineTo: projectDir, + merge: () => { + swapAncestorAfterTheCheck(ancestor, swapRoot) + return { written: "payload" } + }, + }, + ).then( + () => undefined, + (rejection: unknown) => rejection, + ) + expect(reason).toBeInstanceOf(ConfinedPathEscapeError) + // No backup was taken and no commit landed, so there is nothing here for this write to undo. + expect(renames.some((call) => call[0].includes(".bak_"))).toBe(false) + // The escaped file carries the bytes this write staged, but this write never published it, so it + // keeps its own content: the undo is skipped even though the byte comparison would have matched. + expect(fsSyncActual.existsSync(escaped)).toBe(true) + expect(JSON.parse(fsSyncActual.readFileSync(escaped, "utf8"))).toEqual({ written: "payload" }) + expect(JSON.parse(fsSyncActual.readFileSync(path.join(ancestor + "-moved-aside", "mcp.json"), "utf8"))).toEqual( + { inside: true }, + ) + expect( + (await fs.readdir(ancestor)).filter((entry) => entry.includes(".new_") || entry.includes(".bak_")), + ).toEqual([]) + }) }) diff --git a/src/utils/fileLock.ts b/src/utils/fileLock.ts index 49807ea154..8d1717eb49 100644 --- a/src/utils/fileLock.ts +++ b/src/utils/fileLock.ts @@ -1,5 +1,6 @@ import * as path from "path" import { tracePersistenceEvent } from "../core/task-persistence/persistenceTrace" +import { resolveLockKey } from "../services/file-safety/safeWriteText" import * as lockfile from "proper-lockfile" /** @@ -27,7 +28,17 @@ export interface FileLockOptions { * while holding it. */ export async function acquireFileLock(filePath: string, options?: FileLockOptions): Promise<() => Promise> { - const absoluteFilePath = path.resolve(filePath) + // Lock the canonical key - the referent of a symlink alias - so an alias and its + // referent take ONE lock. Every caller of this protocol, safeWriteJson included, + // gets the key computed here: a caller that passed the raw spelling would watch a + // different lock file than the write it means to serialize with, which is how a + // deletion through withFileLock could run beside a safeWriteJson read-modify-write + // on the referent and break the compound atomicity the deletion contract promises. + // withFileLock still hands its operation the caller's resolved path, because the + // operation's own file operations (the task-history unlink) must keep the caller's + // alias semantics - unlinking the referent instead of the alias would delete a + // different directory entry. + const absoluteFilePath = await resolveLockKey(path.resolve(filePath)) if (options?.waitForStaleLock) tracePersistenceEvent("json-lock-wait", { filePath: absoluteFilePath }) try { return await lockfile.lock(absoluteFilePath, { diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index e54b8c51d3..0930c77209 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -3,6 +3,8 @@ import * as fsSync from "fs" import * as path from "path" import { JsonStreamStringify } from "json-stream-stringify" +import { canonicalDirKey, resolveLockKey, resolvePublishTarget } from "../services/file-safety/safeWriteText" + import { getErrorCode } from "./errorCode" import { acquireFileLock } from "./fileLock" import { recoverInterruptedReplacement } from "./jsonFileRecovery" @@ -46,6 +48,50 @@ export interface SafeWriteJsonOptions { * to emit diagnostic trace lines; it never influences the write itself. */ trace?: SafeWriteJsonTrace + /** + * Restrict the write to a directory. The publish target is resolved through + * symlinks before this check runs, so a caller that picked the path from a + * known scope (a workspace, a project settings directory) can refuse a write + * that a planted symlink would land somewhere else. The check runs before the + * advisory lock is taken and before anything is staged, and it is repeated on + * the resolved publish target inside the lock so a peer writer that moved the + * referent in between is caught too. Confinement is active whenever this option + * is DEFINED: a declared root that is empty or whitespace-only is rejected + * rather than silently disabling the checks. + * + * What it does NOT cover: an ancestor DIRECTORY of the target that is replaced + * by a link in the window between the last check and the commit rename. Both + * checks decide from the final component as it resolves at their own instant, + * and publication is path based - the parent-directory creation and the commit + * rename both take a path - so an ancestor swapped after the check redirects the + * publish. What happens then depends on what sits at the swapped-in path: if it + * already has a file at the target name the publish fails in the staging + * identity guard (StagingPathError) and nothing is written; if it does not, the + * write follows the swapped ancestor and lands OUTSIDE the declared scope. This + * option claims neither outcome: it confines the target it resolved, not the + * directory tree above it. Making the claim cover that window needs a publish + * that walks from an already-open directory handle with no-follow semantics + * (openat/renameat over a directory fd); this runtime exposes neither, so the + * claim is narrowed here instead of being made true. A caller that needs + * containment across that window cannot get it from this option. + */ + confineTo?: string +} + +/** + * Thrown when a write declared with `confineTo` resolves outside that directory. + */ +export class ConfinedPathEscapeError extends Error { + constructor( + readonly requestedPath: string, + readonly resolvedPath: string, + readonly confineTo: string, + ) { + super( + `Refusing to write ${resolvedPath}: it resolves outside the confined directory ${confineTo} (requested ${requestedPath}).`, + ) + this.name = "ConfinedPathEscapeError" + } } function createTempFilePath(absoluteFilePath: string, kind: "new" | "bak"): string { @@ -55,6 +101,111 @@ function createTempFilePath(absoluteFilePath: string, kind: "new" | "bak"): stri ) } +/** + * Canonicalize the directory a write is confined to. The publish target is fully + * resolved through symlinks, so the scope has to be resolved the same way or a + * scope path that itself runs through a symlink (macOS /var -> /private/var is the + * common case) would compare lexically against a resolved target and reject every + * legitimate in-scope write. When the scope does not exist yet, the nearest + * existing ancestor is resolved and the remainder re-appended. + */ +async function _resolveScopeRoot(confineTo: string): Promise { + const lexical = path.resolve(confineTo) + try { + return await fs.realpath(lexical) + } catch (error: unknown) { + // Only a missing path means "walk up and re-join". EACCES or ELOOP means the + // scope cannot be canonicalized at all, and continuing would build a partly + // lexical root that can disagree with the canonical target - the failure has to + // surface rather than decide the scope from a guess. + if (getErrorCode(error) !== "ENOENT") { + throw error + } + const missing: string[] = [] + let ancestor = lexical + while (true) { + const parent = path.dirname(ancestor) + if (parent === ancestor) { + return lexical + } + missing.push(path.basename(ancestor)) + ancestor = parent + try { + const real = await fs.realpath(ancestor) + return path.join(real, ...missing.reverse()) + } catch (innerError: unknown) { + if (getErrorCode(innerError) !== "ENOENT") { + throw innerError + } + } + } + } +} + +/** + * Reject a candidate publish path that escapes the caller's confined scope. + * Shared by the pre-lock check and the in-lock check so both canonicalize the + * same way: the candidate is resolved through symlinks and compared against the + * resolved scope root. + */ +function _assertWithinScope(requestedPath: string, candidatePath: string, scopeRoot: string): void { + const relative = path.relative(scopeRoot, candidatePath) + if (relative === "" || relative === ".." || relative.startsWith(".." + path.sep) || path.isAbsolute(relative)) { + throw new ConfinedPathEscapeError(requestedPath, candidatePath, scopeRoot) + } +} + +/** + * The scope root a caller declared, or undefined when no scope was declared. + * Confinement is active whenever confineTo is DEFINED: a declared root that is + * empty or whitespace-only cannot contain any canonicalized path - path.resolve("") + * is the process cwd, not the scope the caller named - so it fails closed here + * instead of silently disabling the checks below, which would let the write + * follow any symlink with no scope check at all. Both confinement checks key off + * this single decision, so they can never disagree about whether a scope is + * active. + */ +function _declaredScopeRoot(confineTo: string | undefined, requestedPath: string): string | undefined { + if (confineTo === undefined) { + return undefined + } + if (confineTo.trim() === "") { + throw new ConfinedPathEscapeError(requestedPath, requestedPath, confineTo) + } + return confineTo +} + +/** + * Undo exactly the file this write published at the escaped location. + * + * The path alone cannot prove the file is ours: a concurrent writer can replace it + * between the commit and this re-check, and unlinking on the name would then delete a + * peer's file. The bytes this write staged are therefore compared with the bytes now on + * disk, and a mismatch leaves the peer's file untouched. The escape is reported to the + * caller either way. + * @internal Exported for the escape-undo regression tests. + */ +export async function undoEscapedPublish(publishedPath: string, expectedBytes: Buffer | undefined): Promise { + if (expectedBytes === undefined) { + return + } + const before = await fs.stat(publishedPath, { bigint: true }).catch(() => undefined) + if (!before) { + return + } + const actualBytes = await fs.readFile(publishedPath).catch(() => undefined) + if (actualBytes === undefined || !actualBytes.equals(expectedBytes)) { + return + } + // Re-stat immediately before the unlink: a peer may have replaced the file between + // the read and the delete, and unlinking it would remove that peer's content. + const after = await fs.stat(publishedPath, { bigint: true }).catch(() => undefined) + if (!after || after.dev !== before.dev || after.ino !== before.ino || after.size !== before.size) { + return + } + await fs.unlink(publishedPath).catch(() => {}) +} + /** * Safely writes JSON data to a file. * - Creates parent directories if they don't exist @@ -62,6 +213,7 @@ function createTempFilePath(absoluteFilePath: string, kind: "new" | "bak"): stri * - Recovers an interrupted previous write before touching the file: restores an orphaned backup when the canonical path is absent, and removes leftover temp files. * - Writes to a temporary file first, then commits it with a single atomic rename, so the committed target stays readable through an interrupted write. * - When a single rename over the target fails with EPERM (a reader holds the file open on Windows), falls back to backing the target up before replacing it. + * - With confineTo declared, refuses a publish whose resolved target lands outside that directory, before the lock and again under it. * - Attempts to roll back and clean up in case of errors. * - Supports pretty-printing with indentation while maintaining streaming efficiency. * @@ -78,6 +230,37 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit // For directory creation 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 + + // Confinement is decided once, up front, so both checks below agree on whether a + // scope is active: active whenever confineTo is defined, with a declared-but-empty + // root failing closed before the lock key is resolved, before the lock is taken, + // before any directory is created, and before anything is staged. + const confinementRoot = _declaredScopeRoot(options?.confineTo, 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. + let lockKey = await resolveLockKey(absoluteFilePath) + + // Confinement, if the caller declared a scope, is checked BEFORE the lock is + // Also before the parent-directory creation below: an out-of-scope target with a + // missing parent would otherwise get a directory created outside confineTo. + // taken: proper-lockfile creates ${lockKey}.lock beside the lock key, and the key + // is the symlink referent, so a repository-planted link out of the scope would + // otherwise create a lock directory outside the scope (and an unwritable referent + // directory would surface a lock-acquisition error after retries instead of + // ConfinedPathEscapeError). Repeated on the resolved publish target inside the + // lock, since a peer writer may move the referent in between. + if (confinementRoot !== undefined) { + const scopeRoot = await _resolveScopeRoot(confinementRoot) + _assertWithinScope(absoluteFilePath, await _resolveScopeRoot(lockKey), scopeRoot) + } + // Ensure directory structure exists with improved reliability try { // Create directory with recursive option @@ -97,17 +280,75 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit // remains a no-op, so the finally block in the main file operations // try-catch-finally won't try to release an unacquired lock if this // path is taken. - releaseLock = await acquireFileLock(absoluteFilePath) + releaseLock = await acquireFileLock(lockKey) + + // Declared before the protected block so the catch can name the bytes an escaped + // publish left behind; the assignment below stays where our copy put it. + let stagedBytes: Buffer | undefined // Variables to hold the actual paths of temp files if they are created. let actualTempNewFilePath: string | null = null let actualTempBackupFilePath: string | null = null + // Whether a commit rename landed the staged file. The catch path below may only undo a file + // this write published: when the commit never landed, whatever sits at the re-resolved + // location is somebody else's, even when its bytes equal the ones this write staged. + let committed = false 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) + + // The key was computed before the lock was taken, but the publish target is resolved + // under it: a symlink retargeted in between means this writer holds the OLD referent's + // lock and would merge into the new one. Re-derive the key and move to it before + // staging, so every writer through the current referent queues behind the same lock. + for (let attempt = 0; attempt < 3; attempt++) { + const recomputedLockKey = await resolveLockKey(absoluteFilePath) + if (recomputedLockKey === lockKey) { + break + } + if (confinementRoot !== undefined) { + const scopeRoot = await _resolveScopeRoot(confinementRoot) + _assertWithinScope(absoluteFilePath, await _resolveScopeRoot(recomputedLockKey), scopeRoot) + } + await releaseLock() + releaseLock = async () => {} + lockKey = recomputedLockKey + releaseLock = await acquireFileLock(lockKey) + resolvedTargetPath = await resolvePublishTarget(absoluteFilePath) + } + // The loop above moved the lock to the key the alias resolves to. This compares the target + // this write is about to rename onto with the key it actually holds: deriving the comparison + // from a second lookup of the alias passes while the stored target sits under a different + // lock, which is the window the finding names. + if ((await canonicalDirKey(resolvedTargetPath)) !== lockKey) { + throw new Error( + `safeWriteJson: refusing to publish ${absoluteFilePath}: the resolved target kept changing while acquiring the lock`, + ) + } + + // Confinement, if the caller declared a scope. Both sides are canonicalized the + // same way: the publish target is resolved through symlinks, and a target that + // does not exist yet still carries the alias components of the path it was + // given. This runs before the merge read and before anything is staged, so a + // rejected write leaves nothing behind. It is also the LAST confinement + // decision of this write: it decides the final component as it resolves here, + // and an ancestor directory replaced by a link between this point and the + // commit rename below is outside its reach, because the rename takes a path and + // re-resolves the tree at rename time. See the confineTo option doc for what the + // two outcomes of that window are and why neither is claimed by this option. + if (confinementRoot !== undefined) { + const scopeRoot = await _resolveScopeRoot(confinementRoot) + _assertWithinScope(absoluteFilePath, await _resolveScopeRoot(resolvedTargetPath), scopeRoot) + } + // Heal any state an interrupted previous write left behind: an absent // canonical path with an orphaned backup, or leftover replacement // files. Runs first so a merge callback reads the restored content. - const recovery = await recoverInterruptedReplacement(absoluteFilePath).catch((error: unknown) => { + const recovery = await recoverInterruptedReplacement(resolvedTargetPath).catch((error: unknown) => { const code = getErrorCode(error) ?? "unknown" options?.trace?.({ stage: "recover", detail: `failed:${code}` }) throw error @@ -122,7 +363,7 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit if (options?.merge) { let existing: unknown = null try { - existing = JSON.parse(await fs.readFile(absoluteFilePath, "utf8")) + existing = JSON.parse(await fs.readFile(resolvedTargetPath, "utf8")) } catch (error: unknown) { const code = getErrorCode(error) if (!(error instanceof SyntaxError) && code !== "ENOENT") { @@ -135,18 +376,48 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit // Step 1: Write data to a new temporary file and flush it to storage, // making a best-effort flush before committing the replacement. options?.trace?.({ stage: "write-start" }) - actualTempNewFilePath = createTempFilePath(absoluteFilePath, "new") + actualTempNewFilePath = createTempFilePath(resolvedTargetPath, "new") + + // Staging, the commit rename, the EPERM fallback's two renames and the rollback all take + // resolvedTargetPath - one resolved string for the whole publication. Staging beside the + // raw path when the target is a link's referent would make the commit rename cross + // filesystems, and mixing the two spellings is how a publish ends up beside the link + // it meant to replace. await _streamDataToFile(actualTempNewFilePath, data, options?.prettyPrint) + + // The bytes this write publishes identify the file a post-commit escape undo may + // remove. They are read after the staged file is written - before it there is nothing + // to read, and reading early failed with ENOENT on every confined write that reached + // the publish - and before the mode copy below, so a restrictive target mode cannot + // turn this read into a failure. A read failure aborts the write: publishing without + // the bytes would leave the undo unable to verify (and remove) an escaped file. + if (confinementRoot !== undefined) { + stagedBytes = await fs.readFile(actualTempNewFilePath) + } + await _flushFileToDisk(actualTempNewFilePath) + // CWE-732: the commit rename below lands the staged file onto the target, so the + // file that survives carries the staged file's mode, not the target's. A 0o600 + // target would otherwise be published 0o644 from the umask-derived default. The + // #1912 head (085fbf85e) carries this step for the same function; the blob main + // brought in does not, and without it the two resolutions differ on a permission + // property and the row that pins it can only pass on one platform. This step sits + // after the flush above because that flush opens the staged file with "r+", and a + // target that withholds write access hands the staged file a mode that denies the + // open. The flush only logs what it could not do, so syncing after the copy can + // skip the sync without failing the publish. + await _copyTargetMode(resolvedTargetPath, actualTempNewFilePath) + // Step 2: Commit the replacement with a single atomic rename. The // committed target stays in place until this rename lands, so a // process exit before or during the commit can no longer leave the // canonical path absent. try { options?.trace?.({ stage: "replacement-rename" }) - await fs.rename(actualTempNewFilePath, absoluteFilePath) + await fs.rename(actualTempNewFilePath, resolvedTargetPath) + committed = true } catch (renameError: unknown) { if (getErrorCode(renameError) !== "EPERM") { throw renameError @@ -159,10 +430,11 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit // second rename fails, and the recovery pass at the start of the // next write repairs a crash between the two renames. options?.trace?.({ stage: "backup-rename", detail: "eperm-fallback" }) - actualTempBackupFilePath = createTempFilePath(absoluteFilePath, "bak") - await fs.rename(absoluteFilePath, actualTempBackupFilePath) + actualTempBackupFilePath = createTempFilePath(resolvedTargetPath, "bak") + await fs.rename(resolvedTargetPath, actualTempBackupFilePath) options?.trace?.({ stage: "replacement-rename", detail: "eperm-fallback" }) - await fs.rename(actualTempNewFilePath, absoluteFilePath) + await fs.rename(actualTempNewFilePath, resolvedTargetPath) + committed = true } // If we reach here, the new file is successfully in place. @@ -171,6 +443,30 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit actualTempNewFilePath = null options?.trace?.({ stage: "write-complete" }) + // Fail-closed confinement re-check. The check above decides the target as it + // resolves at check time; publication takes a path and re-resolves the tree + // at commit time, so an ancestor directory replaced by a link inside that + // window would publish outside the declared scope. Re-resolve after the + // commit: if the bytes landed outside, undo by removing exactly the file + // this write published at the escaped location (never a same-named file a peer + // placed there after the commit) and report the escape. The peer's swap itself is + // left as the peer made it. (A swapped-in directory + // that already carried a target file is not stopped earlier here: the commit rename + // replaces the file it lands on, so the undo below is what keeps the escaped publish + // from leaving our bytes in place of somebody else's.) + if (confinementRoot !== undefined) { + const publishedPath = await _resolveScopeRoot(resolvedTargetPath) + const scopeRoot = await _resolveScopeRoot(confinementRoot) + try { + _assertWithinScope(absoluteFilePath, publishedPath, scopeRoot) + } catch (escapeError) { + // Undo only the file this write published, never a same-named file a peer + // placed there after the commit. + await undoEscapedPublish(publishedPath, stagedBytes) + throw escapeError + } + } + // Step 3: If a backup was created, attempt to delete it. if (actualTempBackupFilePath) { try { @@ -187,17 +483,32 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit } } } 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 + // Whether the rollback below put the backup's bytes back at the target. The escape re-check + // underneath this catch must not undo what that rollback restored: undoEscapedPublish keys + // on the bytes this write staged, and a restored original that carries those same bytes is + // indistinguishable from our own publish by content alone. + // The commit flag covers the mirror case: a commit that never landed published nothing, so + // the undo below must not delete the file that was already sitting at the escaped name. + let backupRestored = false // Attempt rollback if a backup was made if (backupFileToRollbackOrCleanupWithinCatch) { try { - await fs.rename(backupFileToRollbackOrCleanupWithinCatch, absoluteFilePath) + // A backup only ever exists once the resolved target is fixed, so the fallback is + // for the type, not for a reachable case; naming it here keeps the rollback and the + // commit rename on the same resolved string. + await fs.rename(backupFileToRollbackOrCleanupWithinCatch, resolvedTargetPath ?? absoluteFilePath) // Mark as handled, prevent later unlink of this path actualTempBackupFilePath = null + // The target holds the pre-write bytes again, restored by rename rather than by a copy. + backupRestored = true } catch (rollbackError) { // actualTempBackupFilePath (outer scope) remains pointing to backupFileToRollbackOrCleanupWithinCatch console.error( @@ -212,13 +523,45 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit try { await fs.unlink(newFileToCleanupWithinCatch) } catch (cleanupError) { - console.error( - `[Catch] Failed to clean up temporary new file ${newFileToCleanupWithinCatch}:`, - cleanupError, - ) + // The staged file is often already gone: the commit rename consumed it, or an + // earlier teardown removed it. Finding it absent is the expected outcome of a + // failed publish, not a second failure, so it is not logged as one. Any other + // cleanup failure is still reported. + if (getErrorCode(cleanupError) !== "ENOENT") { + console.error( + `[Catch] Failed to clean up temporary new file ${newFileToCleanupWithinCatch}:`, + cleanupError, + ) + } } } + // A confinement escape is reported as the escape, not as whatever the teardown + // surfaced: re-resolve the publish target, and if the committed bytes landed outside + // the declared scope, remove exactly the file this write published there and report + // ConfinedPathEscapeError. undoEscapedPublish compares the staged bytes first, so a + // peer's file at the same name is left alone; what it cannot tell apart is our publish + // and a backup this rollback just restored when the restored bytes equal the staged + // ones, or a peer's file that already carried those bytes when this write never landed a + // commit at all. Both cases are skipped by the flags above instead of by the byte + // comparison. + // Errors that are not an escape keep their own type. + if (confinementRoot !== undefined && resolvedTargetPath !== undefined) { + const publishedPath = await _resolveScopeRoot(resolvedTargetPath) + const scopeRoot = await _resolveScopeRoot(confinementRoot) + try { + _assertWithinScope(absoluteFilePath, publishedPath, scopeRoot) + } catch (escapeError) { + // Two conditions must hold before the undo: a commit rename must have landed this write's + // bytes, and the rollback above must not have renamed the backup back onto the target. If + // either fails, the bytes at the escaped name are not this write's publish, and deleting them + // would remove a file this write never published. + if (committed && !backupRestored) { + await undoEscapedPublish(publishedPath, stagedBytes) + } + throw escapeError + } + } // A failed rollback leaves the backup as the only committed copy. // Preserve it for recovery by a later writer. throw originalError // This MUST be the error that rejects the promise. @@ -230,7 +573,7 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit await releaseLock() } catch (unlockError) { // Do not re-throw here, as the originalError from the try/catch (if any) is more important. - console.error(`Failed to release lock for ${absoluteFilePath}:`, unlockError) + console.error(`Failed to release lock for ${resolvedTargetPath ?? absoluteFilePath}:`, unlockError) } } } @@ -244,7 +587,7 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit */ async function _streamDataToFile(targetPath: string, data: unknown, prettyPrint = false): Promise { // Stream data to avoid high memory usage for large JSON objects. - const fileWriteStream = fsSync.createWriteStream(targetPath, { encoding: "utf8" }) + const fileWriteStream = fsSync.createWriteStream(targetPath, { encoding: "utf8", mode: 0o600 }) // JsonStreamStringify traverses the object and streams tokens directly // The 'spaces' parameter adds indentation during streaming, not via a separate pass @@ -264,7 +607,34 @@ async function _streamDataToFile(targetPath: string, data: unknown, prettyPrint } /** - * Flushes a fully written temp file to storage before it is committed. + * Copies the existing target's permission mode onto the staged file so the publish + * does not silently widen it. A missing target (ENOENT, ENOTDIR) has no mode to copy and + * the staged file keeps its own. Any other stat or chmod failure is a refusal, not a log: + * the publish would land a umask-derived mode over a target whose owner had narrowed it, + * and a 0o600 MCP configuration replaced by a 0o644 file is readable by every local user + * that can traverse the directory. Refusing leaves the target exactly as it was. + */ +async function _copyTargetMode(targetPath: string, stagedPath: string): Promise { + let mode: number + try { + const stats = await fs.stat(targetPath) + mode = stats.mode & 0o777 + } catch (statError: unknown) { + const code = getErrorCode(statError) + if (!code || (code !== "ENOENT" && code !== "ENOTDIR")) { + throw new Error(`Could not read the mode of ${targetPath} before publishing onto it: ${statError}`) + } + return + } + try { + await fs.chmod(stagedPath, mode) + } catch (chmodError: unknown) { + throw new Error(`Failed to copy mode ${mode.toString(8)} from ${targetPath}: ${chmodError}`) + } +} + +/** + * Syncs the directory that holds a freshly committed file. * Best effort: durability against power loss is a bonus here, process-crash * safety comes from the rename protocol itself, and a filesystem that * refuses fsync must not block the write.