From bb89f141dc92e5964d2a207ed9b5d132ef5c67c7 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 12 Nov 2026 09:04:39 +0800 Subject: [PATCH 1/8] fix(safe-write,read-file): publish through the guarded text writer and its readers Unit 1 of the split plan for pull 1913, carried on the branch 1913 already tracks. It holds the guarded text writer and everything that reads through it: the new src/services/file-safety/safeWriteText with its canonical key walk, the staging and durability errors, the ancestor and dangling-link refusals, the ReadFileTool and indentation-reader recovery, the lock-key handover in fileLock, and the specs for each of them. The JSON publish layer and its MCP call sites are not here; they travel in unit 2, which brings the consumers of resolvePublishTarget and canonicalDirKey and adds the export keyword back with them. In this unit the only non-test importer of this file is src/utils/fileLock.ts and it asks for resolveLockKey alone, so those two names are declared without export here rather than exported for a consumer that has not arrived yet, and no knip config entry is added to cover them. --- src/core/tools/ReadFileTool.ts | 117 +- src/core/tools/__tests__/readFileTool.spec.ts | 1208 ++++++++++++- src/eslint-suppressions.json | 2 +- .../indentation-reader-unicode.spec.ts | 9 + .../misc/__tests__/indentation-reader.spec.ts | 82 + src/integrations/misc/indentation-reader.ts | 19 +- .../safeWriteText.integration.spec.ts | 70 + .../__tests__/safeWriteText.spec.ts | 1553 +++++++++++++++++ src/services/file-safety/safeWriteText.ts | 660 +++++++ .../__tests__/fileLock.canonicalKey.spec.ts | 71 + src/utils/fileLock.ts | 13 +- 11 files changed, 3791 insertions(+), 13 deletions(-) create mode 100644 src/services/file-safety/__tests__/safeWriteText.integration.spec.ts create mode 100644 src/services/file-safety/__tests__/safeWriteText.spec.ts create mode 100644 src/services/file-safety/safeWriteText.ts create mode 100644 src/utils/__tests__/fileLock.canonicalKey.spec.ts 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..4a000a5154 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,1120 @@ 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:") + }) + 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..8f7a924810 --- /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. + */ +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. + */ + +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/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/fileLock.ts b/src/utils/fileLock.ts index 49807ea154..6ca7004566 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" /** @@ -73,7 +74,17 @@ export async function withFileLock( options?: FileLockOptions, ): Promise { const absoluteFilePath = path.resolve(filePath) - const releaseLock = await acquireFileLock(absoluteFilePath, options) + // Lock the canonical key - the same one safeWriteJson computes - so a symlink + // alias and its referent take ONE lock. Locking the raw path here would let a + // deletion through this helper run concurrently with a safeWriteJson + // read-modify-write on the referent (the two would watch different lock + // files), breaking the compound atomicity the deletion contract promises. + // The operation still receives the caller's resolved path: its own file + // operations (e.g. 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 lockKey = await resolveLockKey(absoluteFilePath) + const releaseLock = await acquireFileLock(lockKey, options) let result: T try { From 316abe2079eeedfdedaa5ee6aa0a2294ff0e7026 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 12 Nov 2026 09:07:34 +0800 Subject: [PATCH 2/8] fix(safe-write-json,mcp-hub): publish json through the guarded writer and confine the hub reads Unit 2 of the split plan for pull 1913, stacked on unit 1 and recorded on the unit tracking issue that carries the plan. It ports the JSON publish layer and the MCP call sites unchanged from the pull head acc49b99a0, with their specs, and re-adds the export keyword on resolvePublishTarget and canonicalDirKey together with the consumers that need them: safeWriteJson imports canonicalDirKey and calls resolvePublishTarget when it resolves and re-checks its lock key, and the three McpHub call sites pass the confine argument. Unit 1 declares those two names without export because nothing in unit 1 imports them, so the tree at this commit is byte-for-byte the tree of the pull being split. --- src/services/file-safety/safeWriteText.ts | 4 +- src/services/mcp/McpHub.ts | 68 +- src/services/mcp/__tests__/McpHub.spec.ts | 170 +++- .../safeWriteJson.interruption.test.ts | 36 +- .../__tests__/safeWriteJson.lockKey.spec.ts | 300 +++++++ src/utils/__tests__/safeWriteJson.test.ts | 739 +++++++++++++++++- src/utils/safeWriteJson.ts | 400 +++++++++- 7 files changed, 1685 insertions(+), 32 deletions(-) create mode 100644 src/utils/__tests__/safeWriteJson.lockKey.spec.ts diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index 8f7a924810..c2d83ea2cf 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -192,7 +192,7 @@ async function _restoreDaclWindows( * the resolved path: the commit is a rename onto the referent, and a rename * across filesystems fails with EXDEV. */ -async function resolvePublishTarget(absoluteFilePath: string): Promise { +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. @@ -217,7 +217,7 @@ async function resolvePublishTarget(absoluteFilePath: string): Promise { * the file exists when the key is computed, and two writers take two locks. */ -async function canonicalDirKey(absoluteFilePath: string): Promise { +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 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__/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..412e59074f 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), @@ -818,4 +819,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/safeWriteJson.ts b/src/utils/safeWriteJson.ts index e54b8c51d3..db8ce07e9e 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,9 +376,34 @@ 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) + } + + // 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. + await _copyTargetMode(resolvedTargetPath, actualTempNewFilePath) + await _flushFileToDisk(actualTempNewFilePath) // Step 2: Commit the replacement with a single atomic rename. The @@ -146,7 +412,8 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit // 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 +426,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 +439,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 +479,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 +519,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 +569,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 +583,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 +603,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. From 9f9c33db056ac211d74ea0ea689c43a7f1abde4a Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 13 Nov 2026 09:48:23 +0800 Subject: [PATCH 3/8] fix(file-lock): take the canonical lock key once, inside acquireFileLock Port of the answer to review comment 4545130063 on pull 1913, paid on this branch: its src/utils/fileLock.ts blob 6ca70045661c is byte-identical to the blob pull 1913 carried before that fix. acquireFileLock now computes the canonical key once and every caller of the protocol gets it, so safeWriteJson and a task-history deletion through withFileLock watch one lock file for a symlink alias and its referent. withFileLock still hands its operation the caller's resolved path so the unlink keeps alias semantics. --- src/utils/fileLock.ts | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/utils/fileLock.ts b/src/utils/fileLock.ts index 6ca7004566..8d1717eb49 100644 --- a/src/utils/fileLock.ts +++ b/src/utils/fileLock.ts @@ -28,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, { @@ -74,17 +84,7 @@ export async function withFileLock( options?: FileLockOptions, ): Promise { const absoluteFilePath = path.resolve(filePath) - // Lock the canonical key - the same one safeWriteJson computes - so a symlink - // alias and its referent take ONE lock. Locking the raw path here would let a - // deletion through this helper run concurrently with a safeWriteJson - // read-modify-write on the referent (the two would watch different lock - // files), breaking the compound atomicity the deletion contract promises. - // The operation still receives the caller's resolved path: its own file - // operations (e.g. 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 lockKey = await resolveLockKey(absoluteFilePath) - const releaseLock = await acquireFileLock(lockKey, options) + const releaseLock = await acquireFileLock(absoluteFilePath, options) let result: T try { From e6021d63b5675b348d3364e9c513959d5a399569 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 13 Nov 2026 09:48:23 +0800 Subject: [PATCH 4/8] test(file-lock): pin one lock key for withFileLock and safeWriteJson Answers the ask in review comment 4545130063 on pull 1913 on this head. On 316abe2079eeed the two callers already computed the canonical key for themselves, so the pairing rows pass with or without the fix; the third row, which hands the protocol the alias directly, fails at the pre-fix blob and fails again if the canonicalization is taken out of acquireFileLock, so the spec pins the protocol rather than mirroring the fix. --- src/utils/__tests__/lockKeyAlignment.spec.ts | 96 ++++++++++++++++++++ 1 file changed, 96 insertions(+) create mode 100644 src/utils/__tests__/lockKeyAlignment.spec.ts diff --git a/src/utils/__tests__/lockKeyAlignment.spec.ts b/src/utils/__tests__/lockKeyAlignment.spec.ts new file mode 100644 index 0000000000..3d5c1776c0 --- /dev/null +++ b/src/utils/__tests__/lockKeyAlignment.spec.ts @@ -0,0 +1,96 @@ +// 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)) + }) +}) From 75998dbb4538bf0384b2d7f79ccd99600bb721b0 Mon Sep 17 00:00:00 2001 From: "user.email" <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Fri, 13 Nov 2026 17:06:57 +0800 Subject: [PATCH 5/8] style(file-lock): reformat the spec the formatter named at the head 2433 carries prettier --check named src/utils/__tests__/lockKeyAlignment.spec.ts at head e6021d63b5675b348d3364e9c513959d5a399569, and that was the only failing required leg on the pull. This commit reformats that one file and no other: one call-out line was over the printWidth and is wrapped by prettier, +3 / -1. Gates at this commit, each exit printed at the time of writing: prettier --check on the file exits 0 and the repository format:check exits 0; eslint --prune-suppressions --max-warnings=0 on the file exits 0 with its per-file suppression count absent before and after (265 registry keys either way, registry restored byte-identical to HEAD at sha256-16 c910ac80835ca199); vitest run on the file exits 0 with 1 test file and 3 tests passed; tsc --noEmit and tsc -p tsconfig.json --noEmit (typescript 5.9.3, by path) exit 0 with 0 error lines and turbo run check-types over 13 packages exits 0 with 0 error TS lines. No suppression added, no directive written, nothing resolved, no label touched. --- src/utils/__tests__/lockKeyAlignment.spec.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/utils/__tests__/lockKeyAlignment.spec.ts b/src/utils/__tests__/lockKeyAlignment.spec.ts index 3d5c1776c0..aeddc73c3b 100644 --- a/src/utils/__tests__/lockKeyAlignment.spec.ts +++ b/src/utils/__tests__/lockKeyAlignment.spec.ts @@ -75,7 +75,9 @@ describe("lock-key alignment between withFileLock and safeWriteJson", () => { 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 }) + 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]) From 7611518bf715d46144f800d108919961cd1487b8 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sat, 14 Nov 2026 07:05:05 +0800 Subject: [PATCH 6/8] fix(file-lock): flush the staged file before the target mode is copied onto it _copyTargetMode chmods the staged file to the target's mode and _flushFileToDisk opens that same staged file "r+" to sync it. In the order this commit replaces, a target whose mode withholds write access hands the staged file a mode that denies the flush's own open, and _flushFileToDisk only logs a failed flush, so the fsync is skipped while the publish still succeeds. The two calls are now in the other order; the mode copy still runs, and the mode it copies is still the mode the target itself reported. src/utils/__tests__/safeWriteJson.test.ts gains one test that records the order of those two fs/promises calls: it asserts the open of the staged file precedes the chmod of it, that the chmod receives the mode the target reported, and that the flush logged nothing. At the parent head's source with the test in the tree that assertion is what fails - expected [ 'mode-copy', 'flush-open' ] to deeply equal [ 'flush-open', 'mode-copy' ], 1 failed, 60 passed, 4 skipped. With the two calls moved the same file is 61 passed, 4 skipped. The order is asserted through the fs/promises mocks rather than a real read-only target because the mode bits that make the denial bite are enforced only where the runner honours them, and a windows runner does not enforce them. Gates at this commit, each exit printed: prettier --write and --check on both files, and pnpm run format:check over the tree, exit 0. eslint --prune-suppressions --max-warnings=0 on both files, exit 0, with the per-file count for each of them absent, zero, before the run and absent, zero, after it; that run rewrote src/eslint-suppressions.json by serialization and the file was restored with git checkout HEAD -- byte-identical to the head blob, 30910 B sha256-16 c910ac80835ca199, before the test gate ran. tsc --noEmit in src, exit 0. pnpm run check-types, exit 0, 12 of 12 tasks. The four sibling specs that read this publication path, exit 0, 31 tests passed. No suppression added, no directive written, nothing resolved, no label touched. --- src/utils/__tests__/safeWriteJson.test.ts | 35 +++++++++++++++++++++++ src/utils/safeWriteJson.ts | 10 +++++-- 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 412e59074f..2065221f4b 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -757,6 +757,41 @@ 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) + test("should garbage collect orphaned replacement files when the canonical path exists", async () => { const initialData = { message: "Initial content" } diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index db8ce07e9e..0930c77209 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -396,16 +396,20 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit 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. + // 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) - await _flushFileToDisk(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 From 4be089e918336cc8e450d80680faa8f90804bb5c Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sat, 14 Nov 2026 07:28:58 +0800 Subject: [PATCH 7/8] test(read-file): seed a prior observation in the stat-failure tests The four stat-failure tests in src/core/tools/__tests__/readFileTool.spec.ts 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. Four tests are added beside them, two on the native path and two on the legacy path, each seeding a complete observation for the same path before the failing re-read and asserting the returned view plus the entry's seeded version token, complete flag and observedAt. That pins the policy the source states - a failed stat leaves the prior entry untouched rather than invalidating it - which is the decision the review thread asked to be made before writing the test. Controls, each run by name through cmd.exe because a node child process cannot start the package manager here. With the parent head's source and these tests beside it, pnpm --dir src exec vitest run core/tools/__tests__/readFileTool.spec.ts exits 0 with Test Files 1 passed (1) and Tests 122 passed (122): no source change was needed for them, and none is made here. The named regression - both observation guards changed so a missing pre-read or post-read stat calls observationRegistry.clear() on the entry the Task already had - makes the same command exit 1 with Tests 4 failed | 118 passed (122), four AssertionError: expected false to be true lines, one per seeded test, which is the regression the empty-registry tests could not see. Restoring the source with git checkout HEAD -- gives 122 passed (122) again, exit 0. Gates at this commit, each exit printed: prettier --write and --check on the touched file, and pnpm run format:check over the tree, exit 0. eslint --prune-suppressions --max-warnings=0 on the touched file, exit 0 with no output, its per-file count absent, zero, before the run and absent, zero, after it; that run rewrote src/eslint-suppressions.json by serialization to 33,294 B sha256-16 3dbacfef48102e09 and it was restored with git checkout HEAD -- to 30,910 B sha256-16 c910ac80835ca199, byte-identical to the head blob read at this head, before any test gate ran rather than after it. The two sibling specs that own the same registry, core/task/__tests__/observationRegistry.spec.ts and core/task/__tests__/Task.dispose.test.ts, exit 0 with Test Files 2 passed (2) and Tests 29 passed (29). tsc --noEmit in src exits 0 with 0 error lines, and pnpm run check-types exits 0 with Tasks: 12 successful, 12 total. No suppression added, no directive written, no label touched, and no thread is disposed by the act of writing this note. --- src/core/tools/__tests__/readFileTool.spec.ts | 243 ++++++++++++++++++ 1 file changed, 243 insertions(+) diff --git a/src/core/tools/__tests__/readFileTool.spec.ts b/src/core/tools/__tests__/readFileTool.spec.ts index 4a000a5154..b54d9ccbb0 100644 --- a/src/core/tools/__tests__/readFileTool.spec.ts +++ b/src/core/tools/__tests__/readFileTool.spec.ts @@ -2282,6 +2282,249 @@ describe("ReadFileTool", () => { 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() From 95ec553924d7eec237596c278bffa52ab5904266 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Sat, 14 Nov 2026 09:17:53 +0800 Subject: [PATCH 8/8] test(utils): assert the flush sync and the 0o444 mode survive a publish Adds the posix shaped case the open review thread asks for: publish onto a target whose mode is 0o444, assert the handle the publish opened synced the staged file, and assert the published target still reads 0o444. Skipped on win32. --- src/utils/__tests__/safeWriteJson.test.ts | 59 +++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 2065221f4b..fd767c419f 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -792,6 +792,65 @@ describe("safeWriteJson", () => { 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" }