diff --git a/knip.json b/knip.json index 1f4956faa6..1dcb5c47ba 100644 --- a/knip.json +++ b/knip.json @@ -16,7 +16,12 @@ "playwright-ct": false, "workspaces": { "src": { - "entry": ["extension.ts!", "extension/api.ts!", "workers/countTokens.ts!"], + "entry": [ + "extension.ts!", + "extension/api.ts!", + "workers/countTokens.ts!", + "services/file-safety/safeWriteText.ts!" + ], "project": [ "**/*.ts!", "!**/__tests__/**!", @@ -26,6 +31,7 @@ "!utils/WorkspacePathResolver.ts!", "!test-utils/**!", "!utils/versionToken.ts!", + "!core/tools/guardedWrite.ts!", "!services/tools/Bm25Ranker.ts!", "!services/tools/types.ts!", "!core/tools/helpers/toolResultFormatting.ts!" diff --git a/src/core/config/__tests__/importExport.spec.ts b/src/core/config/__tests__/importExport.spec.ts index 6c66655325..2cc0e11c8f 100644 --- a/src/core/config/__tests__/importExport.spec.ts +++ b/src/core/config/__tests__/importExport.spec.ts @@ -24,6 +24,7 @@ import { safeWriteJson } from "../../../utils/safeWriteJson" import type { Mock } from "vitest" import { providerIdentifiers, retiredProviderIdentifiers } from "@roo-code/types/provider-identifiers" +import { t } from "../../../i18n" function createDeferred() { let resolve!: (value: T) => void @@ -1616,10 +1617,14 @@ describe("importExport", () => { expect(mockContextProxy.export).toHaveBeenCalled() expect(fs.mkdir).toHaveBeenCalledWith("/mock/path", { recursive: true }) - expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/zoo-code-settings.json", { - providerProfiles: mockProviderProfiles, - globalSettings: mockGlobalSettings, - }) + expect(safeWriteJson).toHaveBeenCalledWith( + "/mock/path/zoo-code-settings.json", + { + providerProfiles: mockProviderProfiles, + globalSettings: mockGlobalSettings, + }, + { refuseSymlinkTarget: true }, + ) }) it("should include globalSettings when allowedMaxRequests is null", async () => { @@ -1648,10 +1653,14 @@ describe("importExport", () => { contextProxy: mockContextProxy, }) - expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/zoo-code-settings.json", { - providerProfiles: mockProviderProfiles, - globalSettings: mockGlobalSettings, - }) + expect(safeWriteJson).toHaveBeenCalledWith( + "/mock/path/zoo-code-settings.json", + { + providerProfiles: mockProviderProfiles, + globalSettings: mockGlobalSettings, + }, + { refuseSymlinkTarget: true }, + ) }) it("should handle errors during the export process", async () => { @@ -1678,10 +1687,52 @@ describe("importExport", () => { expect(mockProviderSettingsManager.export).toHaveBeenCalled() expect(mockContextProxy.export).toHaveBeenCalled() expect(fs.mkdir).toHaveBeenCalledWith("/mock/path", { recursive: true }) - expect(safeWriteJson).toHaveBeenCalled() // safeWriteJson is called, but it will throw - // The error is caught and the function exits silently. - // Optionally, ensure no error message was shown if that's part of "silent" - // expect(vscode.window.showErrorMessage).not.toHaveBeenCalled(); + }) + + it("notifies the user when the export write is refused, and still swallows the failure", async () => { + ;(vscode.window.showSaveDialog as Mock).mockResolvedValue({ + fsPath: "/mock/path/zoo-code-settings.json", + }) + + mockProviderSettingsManager.export.mockResolvedValue({ + currentApiConfigName: "test", + apiConfigs: { test: { apiProvider: providerIdentifiers.openai, id: "test-id" } }, + migrations: { rateLimitSecondsMigrated: false }, + }) + mockContextProxy.export.mockResolvedValue({ mode: "code" }) + + // The refusal this unit introduced: refuseSymlinkTarget is set at the call above, so a + // link at the path the user picked makes the writer reject instead of publishing. + const refusal = new Error( + "safeWriteJson: refusing to write through the symlink now at /mock/path/zoo-code-settings.json (before publication); the payload would land at /elsewhere/settings.json", + ) + ;(safeWriteJson as Mock).mockRejectedValueOnce(refusal) + + const showErrorMessageSpy = vi.spyOn(vscode.window, "showErrorMessage").mockResolvedValue(undefined) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + // The contract the caller relies on: webviewMessageHandler awaits exportSettings without a + // try, so a refusal must not become an unhandled rejection. + await expect( + exportSettings({ + providerSettingsManager: mockProviderSettingsManager, + contextProxy: mockContextProxy, + }), + ).resolves.toBeUndefined() + + // The user learns the file did not land, through the same channel and the same key shape the + // import route uses. Expected is computed with the same t() call the module makes, so the + // assertion holds whether this lane resolves t to the string or to the key. + expect(showErrorMessageSpy).toHaveBeenCalledTimes(1) + expect(showErrorMessageSpy).toHaveBeenCalledWith( + t("common:errors.settings_export_failed", { error: refusal.message }), + ) + expect(showErrorMessageSpy).toHaveBeenCalledWith(expect.stringContaining("errors.settings_export_failed")) + expect(showErrorMessageSpy).not.toHaveBeenCalledWith(expect.stringContaining("settings_import_failed")) + expect(consoleErrorSpy).toHaveBeenCalledWith("Failed to export settings:", refusal) + + showErrorMessageSpy.mockRestore() + consoleErrorSpy.mockRestore() }) it("should handle errors during directory creation", async () => { @@ -1771,10 +1822,14 @@ describe("importExport", () => { contextProxy: mockContextProxy, }) - expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/zoo-code-settings.json", { - providerProfiles: mockProviderProfiles, - globalSettings: mockGlobalSettings, - }) + expect(safeWriteJson).toHaveBeenCalledWith( + "/mock/path/zoo-code-settings.json", + { + providerProfiles: mockProviderProfiles, + globalSettings: mockGlobalSettings, + }, + { refuseSymlinkTarget: true }, + ) }) it("should export model dimension for OpenAI Compatible provider", async () => { @@ -1924,10 +1979,14 @@ describe("importExport", () => { }) // Should not throw an error and should preserve original settings - expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/zoo-code-settings.json", { - providerProfiles: mockProviderProfiles, - globalSettings: mockGlobalSettings, // Should remain unchanged - }) + expect(safeWriteJson).toHaveBeenCalledWith( + "/mock/path/zoo-code-settings.json", + { + providerProfiles: mockProviderProfiles, + globalSettings: mockGlobalSettings, // Should remain unchanged + }, + { refuseSymlinkTarget: true }, + ) }) it("should maintain backward compatibility with existing exports", async () => { @@ -1967,10 +2026,14 @@ describe("importExport", () => { }) // Should not modify settings for non-openai-compatible providers - expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/zoo-code-settings.json", { - providerProfiles: mockProviderProfiles, - globalSettings: mockGlobalSettings, // Should remain unchanged - }) + expect(safeWriteJson).toHaveBeenCalledWith( + "/mock/path/zoo-code-settings.json", + { + providerProfiles: mockProviderProfiles, + globalSettings: mockGlobalSettings, // Should remain unchanged + }, + { refuseSymlinkTarget: true }, + ) }) it("should handle missing current provider gracefully", async () => { @@ -2012,10 +2075,14 @@ describe("importExport", () => { }) // Should not throw an error and should preserve original settings - expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/zoo-code-settings.json", { - providerProfiles: mockProviderProfiles, - globalSettings: mockGlobalSettings, // Should remain unchanged - }) + expect(safeWriteJson).toHaveBeenCalledWith( + "/mock/path/zoo-code-settings.json", + { + providerProfiles: mockProviderProfiles, + globalSettings: mockGlobalSettings, // Should remain unchanged + }, + { refuseSymlinkTarget: true }, + ) }) }) @@ -2249,10 +2316,14 @@ describe("importExport", () => { }) // Step 4: Verify the exported data includes the model dimension - expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/test-settings.json", { - providerProfiles: mockProviderProfiles, - globalSettings: mockGlobalSettings, - }) + expect(safeWriteJson).toHaveBeenCalledWith( + "/mock/path/test-settings.json", + { + providerProfiles: mockProviderProfiles, + globalSettings: mockGlobalSettings, + }, + { refuseSymlinkTarget: true }, + ) // Step 5: Get the exported data for import test const exportedData = (safeWriteJson as Mock).mock.calls[0][1] diff --git a/src/core/config/importExport.ts b/src/core/config/importExport.ts index 9882039ad3..cb222ebc3c 100644 --- a/src/core/config/importExport.ts +++ b/src/core/config/importExport.ts @@ -346,10 +346,18 @@ export const exportSettings = async ({ providerSettingsManager, contextProxy }: const dirname = path.dirname(uri.fsPath) await fs.mkdir(dirname, { recursive: true }) - await safeWriteJson(uri.fsPath, { providerProfiles, globalSettings }) + await safeWriteJson(uri.fsPath, { providerProfiles, globalSettings }, { refuseSymlinkTarget: true }) } catch (e) { console.error("Failed to export settings:", e) - // Don't re-throw - the UI will handle showing error messages + // Don't re-throw - this route has no result object for the caller to report, so the + // notification happens here, the way the import route notifies through + // settings_import_failed below. A refusal to publish (a link at the chosen path the user + // never picked) is otherwise invisible: the dialog closes, the log line is not user-facing, + // and the file the user asked for does not exist. The writer's own message names the path it + // refused and the destination it would have written through, so the reason travels with the + // message. + const reason = e instanceof Error ? e.message : String(e) + await vscode.window.showErrorMessage(t("common:errors.settings_export_failed", { error: reason })) } } diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index b911479bc6..db364fc1c1 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -107,6 +107,7 @@ import { ToolRepetitionDetector } from "../tools/ToolRepetitionDetector" import { restoreTodoListForTask } from "../tools/UpdateTodoListTool" import { FileContextTracker } from "../context-tracking/FileContextTracker" import { RooIgnoreController } from "../ignore/RooIgnoreController" +import { ObservationRegistry } from "./observationRegistry" import { RooProtectedController } from "../protect/RooProtectedController" import { type AssistantMessageContent, presentAssistantMessage } from "../assistant-message" import { DispatchState, type PendingWork } from "./dispatchState" @@ -302,6 +303,7 @@ export class Task extends EventEmitter implements TaskLike { readonly parentTask: Task | undefined = undefined readonly taskNumber: number readonly workspacePath: string + readonly observationRegistry = new ObservationRegistry() /** * The mode associated with this task. Persisted across sessions @@ -1464,6 +1466,7 @@ export class Task extends EventEmitter implements TaskLike { /** Cancels the current persistence generation before creating the next assistant-turn boundary. */ private resetAssistantMessagePersistence(): void { this.cancelAssistantMessagePersistence() + this.assistantMessagePersistencePromise = new Promise((resolve) => { this.resolveAssistantMessagePersistence = resolve }) @@ -3696,6 +3699,12 @@ export class Task extends EventEmitter implements TaskLike { this.cancelAssistantMessagePersistence() this.cancelPendingPartialMessageUpdate() + // Drop the per-task file observations. The registry holds an entry per absolute + // path the task read or wrote (version token + timestamp); nothing can consume them + // after disposal, and a long-lived extension host would otherwise keep every path a + // finished task touched alive. + this.observationRegistry.close() + // Release handler-owned host resources (e.g. VS Code configuration listeners). // This is the fallback for leases that never settle; later lease releases // find nothing retired and so cannot dispose twice. diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index c1ee5a0b13..64067e18a7 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -1496,6 +1496,71 @@ describe("Cline", () => { }) }) + describe("observation registry lifecycle (S4a, epic #1375)", () => { + it("gives each Task its own observation registry", () => { + const firstTask = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "first observation task", + startTask: false, + }) + const secondTask = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "second observation task", + startTask: false, + }) + + // The guarded-write contract assumes an observation in one task never validates + // a write issued by another task. + expect(firstTask.observationRegistry).not.toBe(secondTask.observationRegistry) + firstTask.observationRegistry.observe("/workspace/a.ts", "v-a") + expect(firstTask.observationRegistry.get("/workspace/a.ts")?.version).toBe("v-a") + expect(secondTask.observationRegistry.get("/workspace/a.ts")).toBeUndefined() + }) + + it("clears the observation registry when the task is disposed", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "disposed observation task", + startTask: false, + }) + task.observationRegistry.observe("/workspace/a.ts", "v-a") + task.observationRegistry.observe("/workspace/b.ts", "v-b") + + await task.dispose() + + // A disposed task cannot serve another guarded write, so its observed paths + // (version token + timestamp each) must not stay reachable for the host lifetime. + expect(task.observationRegistry.get("/workspace/a.ts")).toBeUndefined() + expect(task.observationRegistry.get("/workspace/b.ts")).toBeUndefined() + // Closing is what makes a later observation refuse, so it is asserted rather than + // left to the test name: clearing the map alone would look identical from here. + expect(task.observationRegistry.isClosed).toBe(true) + }) + + it("refuses an observation recorded after the task was disposed", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "late observation task", + startTask: false, + }) + + await task.dispose() + + // A read that was already in flight when the task was disposed can finish late and + // call observe(). Recording then would repopulate a registry no task owns and hand a + // version token to a guarded write that will never happen. + task.observationRegistry.observe("/workspace/late.ts", "v-late") + + expect(task.observationRegistry.get("/workspace/late.ts")).toBeUndefined() + expect(task.observationRegistry.size).toBe(0) + expect(task.observationRegistry.isClosed).toBe(true) + }) + }) + describe("create", () => { it("rejects missing task inputs before constructing the task", () => { const onCreated = vi.fn() diff --git a/src/core/task/__tests__/observationRegistry.spec.ts b/src/core/task/__tests__/observationRegistry.spec.ts new file mode 100644 index 0000000000..2301247f32 --- /dev/null +++ b/src/core/task/__tests__/observationRegistry.spec.ts @@ -0,0 +1,90 @@ +import { describe, it, expect, vi } from "vitest" + +import { ObservationRegistry } from "../observationRegistry" + +describe("ObservationRegistry", () => { + it("observe → get returns the recorded version and observedAt", () => { + const reg = new ObservationRegistry() + reg.observe("/a/b/c.ts", "1:2:300:4000000000:5000000000") + + const obs = reg.get("/a/b/c.ts") + expect(obs).toBeDefined() + expect(obs!.version).toBe("1:2:300:4000000000:5000000000") + expect(typeof obs!.observedAt).toBe("number") + }) + + it("re-observe replaces the entry with a fresh observedAt", () => { + vi.useFakeTimers() + const reg = new ObservationRegistry() + reg.observe("/a/b/c.ts", "v1") + const first = reg.get("/a/b/c.ts")! + expect(first.version).toBe("v1") + + vi.advanceTimersByTime(50) + reg.observe("/a/b/c.ts", "v2") + const second = reg.get("/a/b/c.ts")! + expect(second.version).toBe("v2") + expect(second.observedAt).toBeGreaterThan(first.observedAt) + + vi.useRealTimers() + }) + + it("has returns true for observed paths, false otherwise", () => { + const reg = new ObservationRegistry() + reg.observe("/x.ts", "t1") + expect(reg.has("/x.ts")).toBe(true) + expect(reg.has("/y.ts")).toBe(false) + }) + + it("size reflects the number of observed entries", () => { + const reg = new ObservationRegistry() + expect(reg.size).toBe(0) + reg.observe("/a.ts", "t1") + reg.observe("/b.ts", "t2") + expect(reg.size).toBe(2) + }) + + it("clear removes all entries and resets size to 0", () => { + const reg = new ObservationRegistry() + reg.observe("/a.ts", "t1") + reg.observe("/b.ts", "t2") + reg.clear() + expect(reg.size).toBe(0) + expect(reg.get("/a.ts")).toBeUndefined() + expect(reg.has("/b.ts")).toBe(false) + }) + + it("get on empty registry returns undefined", () => { + const reg = new ObservationRegistry() + expect(reg.get("/any.ts")).toBeUndefined() + }) + + it("separate instances are independent — observing in one does not appear in the other", () => { + const regA = new ObservationRegistry() + const regB = new ObservationRegistry() + regA.observe("/shared.ts", "v1") + expect(regA.get("/shared.ts")).toBeDefined() + expect(regB.get("/shared.ts")).toBeUndefined() + regB.observe("/shared.ts", "v2") + expect(regA.get("/shared.ts")!.version).toBe("v1") + expect(regB.get("/shared.ts")!.version).toBe("v2") + }) +}) + +describe("close() - disposal is terminal", () => { + it("drops every observation and refuses later ones", () => { + const registry = new ObservationRegistry() + registry.observe("/workspace/a.ts", "v-a") + expect(registry.size).toBe(1) + + registry.close() + + expect(registry.size).toBe(0) + expect(registry.isClosed).toBe(true) + + // A read that was in flight when the task was disposed must not repopulate it. + registry.observe("/workspace/late.ts", "v-late") + expect(registry.get("/workspace/late.ts")).toBeUndefined() + expect(registry.size).toBe(0) + }) +}) diff --git a/src/core/task/observationRegistry.ts b/src/core/task/observationRegistry.ts new file mode 100644 index 0000000000..db0ecfd07e --- /dev/null +++ b/src/core/task/observationRegistry.ts @@ -0,0 +1,75 @@ +/** + * Per-task file observation registry (upstream epic #1375, phase A2). + * + * Each Task owns its own instance so parent and subtask observations are + * independent. The S4 guarded-write will compare these versions against the + * token recomputed pre-write to detect stale reads or file replacement. + * + * Pure in-memory — zero I/O, no dependencies. The observations ARE consulted: + * guardedWrite reads this registry before publishing (src/core/tools/guardedWrite.ts) + * and compares the recorded version token against the token recomputed from disk, so a + * stale read or an out-of-band replacement that the check detects is rejected instead of + * published over. Detection is best effort against a non-cooperating process: the token is + * recomputed before the publish, so a replacement that lands after that check and before + * the rename is not observable from here and can still win. Closing that last window needs + * a cross-process lock or an atomic create, not a token comparison. + */ + +export interface FileObservation { + /** Version token derived from on-disk fs.stat (bigint mode). */ + version: string + /** Millisecond timestamp when the observation was recorded. */ + observedAt: number +} + +export class ObservationRegistry { + private readonly entries = new Map() + + /** Set by close(): after disposal the registry refuses further observations. */ + private closed = false + + /** + * Record an observation for a file at its absolute path, unless the registry is closed. + * + * Re-observing replaces the entry with a fresh observedAt timestamp and + * the new version token. A read that was already in flight can finish after + * Task.disposeOnce() dropped the observations; recording then would hand a version token + * to a task that no longer serves any request, and a later guarded write could consult + * it. close() therefore makes this a no-op, so disposal is terminal at this layer. + */ + observe(absolutePath: string, version: string): void { + if (this.closed) { + return + } + this.entries.set(absolutePath, { version, observedAt: Date.now() }) + } + + get(absolutePath: string): FileObservation | undefined { + return this.entries.get(absolutePath) + } + + has(absolutePath: string): boolean { + return this.entries.has(absolutePath) + } + + clear(): void { + this.entries.clear() + } + + /** + * Drop every observation and refuse any later one. Task.disposeOnce() calls this so a + * disposed task's registry cannot be repopulated by a read that finishes late. + */ + close(): void { + this.closed = true + this.entries.clear() + } + + get isClosed(): boolean { + return this.closed + } + + get size(): number { + return this.entries.size + } +} diff --git a/src/core/tools/ReadFileTool.ts b/src/core/tools/ReadFileTool.ts index e4e69b30ad..19827009db 100644 --- a/src/core/tools/ReadFileTool.ts +++ b/src/core/tools/ReadFileTool.ts @@ -17,6 +17,7 @@ import type { ReadFileParams, ReadFileMode, ReadFileToolParams, FileEntry, LineR import { isLegacyReadFileParams, type ClineSayTool } from "@roo-code/types" import { Task } from "../task/Task" +import { versionTokenOfStat } from "../../utils/versionToken" import { formatResponse } from "../prompts/responses" import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { isPathOutsideWorkspace } from "../../utils/pathUtils" @@ -215,12 +216,29 @@ export class ReadFileTool extends BaseTool<"read_file"> { // Read text file content with lossy UTF-8 conversion // Reading as Buffer first allows graceful handling of non-UTF8 bytes // (they become U+FFFD replacement characters instead of throwing) + // A2 (epic #1375): capture the on-disk token before the read so a mutation + // landing mid-read is detected by the post-read stat below. + const preReadStats = await fs.stat(fullPath, { bigint: true }).catch(() => undefined) const buffer = await fs.readFile(fullPath) const fileContent = buffer.toString("utf-8") const result = this.processTextFile(fileContent, entry) await task.fileContextTracker.trackFileContext(relPath, "read_tool" as RecordSource) + // A2 (plan #33 / epic #1375): record the observed on-disk version for the future write guard. + // The token is captured before AND after the read; the target is observed only + // when both match — a mutation between the two stats means the content the model + // received is not the on-disk state, and observing it would let a later write + // match a token the model never saw. A stat failure leaves the target + // unobserved and never fails the read. + const postReadStats = await fs.stat(fullPath, { bigint: true }).catch(() => undefined) + if (preReadStats && postReadStats) { + const preReadToken = versionTokenOfStat(preReadStats) + if (preReadToken === versionTokenOfStat(postReadStats)) { + task.observationRegistry.observe(fullPath, preReadToken) + } + } + updateFileResult(relPath, { nativeContent: `File: ${relPath}\n${result}`, }) @@ -770,6 +788,9 @@ export class ReadFileTool extends BaseTool<"read_file"> { } // Read text file + // A2 (epic #1375): capture the on-disk token before the read so a mutation + // landing mid-read is detected by the post-read stat below. + const preReadStats = await fs.stat(fullPath, { bigint: true }).catch(() => undefined) const rawContent = await fs.readFile(fullPath, "utf8") // Handle line ranges if specified @@ -801,6 +822,19 @@ export class ReadFileTool extends BaseTool<"read_file"> { // Track file in context await task.fileContextTracker.trackFileContext(relPath, "read_tool") + + // A2 (plan #33 / epic #1375): mirror the native path — record the observed + // on-disk version so legacy-format reads also feed the future write guard. + // Observe only when the pre-read and post-read tokens match (a mutation between + // them means the returned content is not the on-disk state). A stat failure + // leaves the target unobserved and never fails the read. + const postReadStats = await fs.stat(fullPath, { bigint: true }).catch(() => undefined) + if (preReadStats && postReadStats) { + const preReadToken = versionTokenOfStat(preReadStats) + if (preReadToken === versionTokenOfStat(postReadStats)) { + task.observationRegistry.observe(fullPath, preReadToken) + } + } } catch (error) { const errorMsg = error instanceof Error ? error.message : String(error) results.push(`File: ${relPath}\nError: ${errorMsg}`) diff --git a/src/core/tools/__tests__/guardedWrite.spec.ts b/src/core/tools/__tests__/guardedWrite.spec.ts new file mode 100644 index 0000000000..c4f711605b --- /dev/null +++ b/src/core/tools/__tests__/guardedWrite.spec.ts @@ -0,0 +1,864 @@ +/** + * Tests for the guarded-write compare-and-swap core (upstream epic #1375, + * phase A4a). + * + * Covers guard selection through the S2 observation registry, version-token + * CAS, remediation messages, and the per-absolute-path FIFO chain: FIFO + * ordering, exactly-one winner under concurrency, no wedge after a rejected + * link, and independence across paths. It also covers the publication-time + * re-verification that closes the check-to-rename window for writers + * serialized by the chain. + */ + +import * as fs from "fs/promises" +import * as path from "path" + +import { describe, expect, it, beforeEach, vi } from "vitest" + +import { createIfAbsent, guardedWrite, replaceIfVersion, resetChain } from "../guardedWrite" +import { safeWriteText, type SafeWriteTextOptions } from "../../../services/file-safety/safeWriteText" +import { computeVersionToken } from "../../../utils/versionToken" +import { ObservationRegistry } from "../../task/observationRegistry" +import { Task } from "../../task/Task" +import { initialRunState, type RunState } from "../../task/run-state/runState" +import type { RunRejection } from "../../task/run-state/runStateShadow" + +// -- Mocks ------------------------------------------------------------------- + +vi.mock("fs/promises", () => ({ + access: vi.fn(), + stat: vi.fn(), +})) + +vi.mock("../../../utils/versionToken", () => ({ + computeVersionToken: vi.fn(), +})) + +vi.mock("../../../services/file-safety/safeWriteText", () => ({ + safeWriteText: vi.fn(), +})) + +const mockedFsAccess = vi.mocked(fs.access) +const mockedComputeVersionToken = vi.mocked(computeVersionToken) +const mockedSafeWriteText = vi.mocked(safeWriteText) + +// -- Fixtures ---------------------------------------------------------------- + +const WORKSPACE = "/test/workspace" + +/** Resolve a fixture path the same way guardedWrite resolves task.cwd-relative paths. */ +const abs = (relPath: string): string => path.resolve(WORKSPACE, relPath) + +interface MockTaskOptions { + cwd?: string + observationRegistry?: ObservationRegistry +} + +/** + * Task stand-in that runs the production cancellation path instead of re-implementing it. + * + * guardedWrite reads cwd, observationRegistry and abort off the task + * (src/core/tools/guardedWrite.ts:289, :351, :358), and building a real Task needs the full + * provider machinery: the smallest spec on this branch that does it, + * src/core/task/__tests__/Task.abort-reason-race.spec.ts, carries 12 vi.mock blocks and a + * 34-line provider double for exactly that reason. So this stands on Task.prototype and + * supplies only the values the constructor would have initialized. requestAbort is then the + * class's own method: the flag write at Task.ts:3596 and the run-state kernel it drives + * (Task.ts:3592-3598, driveRunEvent at :3610-3631 over runState and runRejections) are + * production code rather than a re-implementation in the double. + * + * Nothing here depends on which spelling of abort the tree carries. cwd is an accessor on + * Task.prototype (Task.ts:6494), so it is shadowed with an own property instead of assigned; + * abort is left to production - undefined until requestAbort runs, which reads the same as + * false at the truthy check in guardedWrite.ts:358 - because that call site reads only abort, + * while main (5319fe91f, #2283) exposes abort as a get-only accessor fed by the kernel latches + * (src/core/task/run-state/runState.ts:188), where a test that wrote the field would not + * compile: the CI typecheck runs on the PR merge tree, and this file writing task.abort is + * what made compile fail there. + */ +/** The fields Task's constructor initializes that this stand-in has to supply by hand. */ +interface TaskDoubleFields { + cwd?: string + observationRegistry?: ObservationRegistry + runState?: RunState + runRejections?: RunRejection[] +} + +function createMockTask(options: MockTaskOptions = {}): Task { + // Object.create is untyped in this lib: its signature takes no type argument, so the shape + // comes from the structural type above rather than from a generic parameter. + const task: TaskDoubleFields = Object.create(Task.prototype) + Object.defineProperty(task, "cwd", { + value: options.cwd ?? WORKSPACE, + writable: true, + configurable: true, + }) + task.observationRegistry = options.observationRegistry ?? new ObservationRegistry() + task.runState = initialRunState + task.runRejections = [] + // The stand-in covers only the members guardedWrite touches; Task's own surface is wider and + // no constructor runs here, so the double assertion stands in for that gap. + return task as unknown as Task +} +// -- Tests ------------------------------------------------------------------- + +describe("guardedWrite (S4a, epic #1375)", () => { + beforeEach(() => { + vi.resetAllMocks() + resetChain() + }) + + describe("unobserved create", () => { + it("succeeds when the file is absent and publishes via safeWriteText", async () => { + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) + const task = createMockTask() + + await guardedWrite(task, "new-file.txt", "hello", "create") + + expect(mockedSafeWriteText).toHaveBeenCalledTimes(1) + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("new-file.txt"), "hello", { + verifyBeforeCommit: expect.any(Function), + }) + }) + + it("defaults kind to update when the caller omits the argument", async () => { + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) + const task = createMockTask() + + // Every other call in this file passes kind explicitly, so the default was never exercised. + // The default only matters on the UNOBSERVED path: once a file is observed the guard is + // chosen by the observation, not by kind. Omitting it must take the create-if-absent guard + // (which publishes); a default of "edit" would hit the read-first guard and reject instead. + await guardedWrite(task, "new-file.txt", "hello") + + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("new-file.txt"), "hello", { + verifyBeforeCommit: expect.any(Function), + }) + }) + + it("fails with the read-first remediation when the file exists - nothing published", async () => { + mockedFsAccess.mockResolvedValue(undefined) + const task = createMockTask() + + await expect(guardedWrite(task, "existing.txt", "hello", "create")).rejects.toThrow( + "file not read yet — read the file, then retry", + ) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + + it("rethrows I/O errors that are not ENOENT verbatim (no guard verdict on access failure)", async () => { + const failures = [{ code: "EACCES" }, null, "volume offline", new Error("EIO-ish failure")] + for (const failure of failures) { + mockedFsAccess.mockRejectedValueOnce(failure) + await expect(createIfAbsent(abs("io-error.txt"), "x")).rejects.toBe(failure) + } + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + }) + + describe("deleted-after-read target (observed + absent)", () => { + it("creates the file when it vanished after the read", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("vanished.txt"), "v1") + const task = createMockTask({ observationRegistry: reg }) + + // Observed + absent is the create row of the #1399 decision table: the file the + // caller read was deleted before this write, so the guard recreates it. + mockedComputeVersionToken.mockRejectedValue({ code: "ENOENT" }) + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) + + await guardedWrite(task, "vanished.txt", "next", "update") + + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("vanished.txt"), "next", { + verifyBeforeCommit: expect.any(Function), + }) + }) + + it("rethrows non-ENOENT token failures verbatim from replaceIfVersion", async () => { + const failure = { code: "EACCES" } + mockedComputeVersionToken.mockRejectedValueOnce(failure) + + await expect(replaceIfVersion(abs("locked.txt"), "v1", "next")).rejects.toBe(failure) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + }) + describe("unobserved update", () => { + it("succeeds when the file is absent (same create guard)", async () => { + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) + const task = createMockTask() + + await guardedWrite(task, "new-file.txt", "hello", "update") + + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("new-file.txt"), "hello", { + verifyBeforeCommit: expect.any(Function), + }) + }) + + it("fails with the read-first remediation when the file exists - nothing published", async () => { + mockedFsAccess.mockResolvedValue(undefined) + const task = createMockTask() + + await expect(guardedWrite(task, "existing.txt", "hello", "update")).rejects.toThrow( + "file not read yet — read the file, then retry", + ) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + }) + + describe("observed create", () => { + it("recreates a file that vanished after the read", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("gone.txt"), "v1") + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) + const task = createMockTask({ observationRegistry: reg }) + + await guardedWrite(task, "gone.txt", "back", "create") + + expect(mockedSafeWriteText).toHaveBeenCalledTimes(1) + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("gone.txt"), "back", { + verifyBeforeCommit: expect.any(Function), + }) + }) + + it("goes through the version guard when the file still exists", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("kept.txt"), "v1") + mockedFsAccess.mockResolvedValue(undefined) + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + await guardedWrite(task, "kept.txt", "rewritten", "create") + + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("kept.txt"), "rewritten", { + verifyBeforeCommit: expect.any(Function), + }) + }) + + it("uses the update guard when the caller omits the argument on an observed file", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("doc.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + // Documents the contract for the omitted argument on the observed path. Note that this case + // is NOT load-bearing: once a file is observed the guard is chosen by the observation, so + // changing the default does not change this call. The load-bearing pin for the default is + // 'defaults kind to update when the caller omits the argument' on the unobserved path. + await guardedWrite(task, "doc.txt", "new content") + + expect(mockedComputeVersionToken).toHaveBeenCalledWith(abs("doc.txt")) + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("doc.txt"), "new content", { + verifyBeforeCommit: expect.any(Function), + }) + }) + + it("fails with the stale remediation suffix when the version moved", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("kept.txt"), "v1") + mockedFsAccess.mockResolvedValue(undefined) + mockedComputeVersionToken.mockResolvedValue("v2") + const task = createMockTask({ observationRegistry: reg }) + + await expect(guardedWrite(task, "kept.txt", "rewritten", "create")).rejects.toThrow( + "stale version — re-read the file, then retry", + ) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + + it("defers to the version guard when the access check is denied (not ENOENT)", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("locked.txt"), "v1") + mockedFsAccess.mockRejectedValue({ code: "EACCES" }) + mockedComputeVersionToken.mockResolvedValue("v2") + const task = createMockTask({ observationRegistry: reg }) + + await expect(guardedWrite(task, "locked.txt", "rewritten", "create")).rejects.toThrow( + "stale version — re-read the file, then retry", + ) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + }) + + describe("observed update (version CAS)", () => { + it("publishes when the on-disk version matches the observation", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("doc.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + await guardedWrite(task, "doc.txt", "new content", "update") + + expect(mockedComputeVersionToken).toHaveBeenCalledWith(abs("doc.txt")) + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("doc.txt"), "new content", { + verifyBeforeCommit: expect.any(Function), + }) + }) + + it("fails with the stale remediation suffix when the version moved - nothing published", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("doc.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v2") + const task = createMockTask({ observationRegistry: reg }) + + await expect(guardedWrite(task, "doc.txt", "new content", "update")).rejects.toThrow( + "stale version — re-read the file, then retry", + ) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + + it("recreates when an updated file vanished after the read", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("doc.txt"), "v1") + const task = createMockTask({ observationRegistry: reg }) + // Observed + absent is the create row of the #1399 decision table: the + // file vanished between the read and this write, so the guard recreates it. + mockedComputeVersionToken.mockRejectedValue({ code: "ENOENT" }) + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) + + await guardedWrite(task, "doc.txt", "new content", "update") + + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("doc.txt"), "new content", { + verifyBeforeCommit: expect.any(Function), + }) + }) + }) + + describe("edit", () => { + it("fails read-first when the file was never observed - nothing published, no I/O", async () => { + const task = createMockTask() + + await expect(guardedWrite(task, "any.txt", "patched", "edit")).rejects.toThrow( + "file not read yet — read the file, then retry", + ) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + expect(mockedComputeVersionToken).not.toHaveBeenCalled() + expect(mockedFsAccess).not.toHaveBeenCalled() + }) + + it("publishes when the version matches the observation", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("doc.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + await guardedWrite(task, "doc.txt", "patched", "edit") + + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("doc.txt"), "patched", { + verifyBeforeCommit: expect.any(Function), + }) + }) + + it("fails with the stale remediation suffix when the version moved", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("doc.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v3") + const task = createMockTask({ observationRegistry: reg }) + + await expect(guardedWrite(task, "doc.txt", "patched", "edit")).rejects.toThrow( + "stale version — re-read the file, then retry", + ) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + }) + }) + + describe("observation refresh after publication", () => { + it("re-observes the published token so a second update without a re-read succeeds", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("doc.txt"), "v1") + const task = createMockTask({ observationRegistry: reg }) + mockedComputeVersionToken.mockResolvedValue("v1") + // The publish changes the on-disk state: the token moves to v2. + mockedSafeWriteText.mockImplementation(async () => { + mockedComputeVersionToken.mockResolvedValue("v2") + }) + + await guardedWrite(task, "doc.txt", "one", "update") + + // The registry records what this write published, not the pre-write read. + expect(reg.get(abs("doc.txt"))?.version).toBe("v2") + + // A second write from the same task without a re-read succeeds: its CAS + // compares against the content the task itself published. + await guardedWrite(task, "doc.txt", "two", "update") + + expect(mockedSafeWriteText).toHaveBeenCalledTimes(2) + expect(reg.get(abs("doc.txt"))?.version).toBe("v2") + }) + + it("records an observation after creating an unobserved file so a follow-up write is not treated as unobserved", async () => { + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) + mockedComputeVersionToken.mockResolvedValue("created-v1") + const task = createMockTask() + // After the create the file exists on disk. + mockedSafeWriteText.mockImplementation(async () => { + mockedFsAccess.mockResolvedValue(undefined) + }) + + await guardedWrite(task, "made.txt", "content", "create") + + expect(task.observationRegistry.get(abs("made.txt"))?.version).toBe("created-v1") + + // Without the write-back this second write would take the unobserved + // branch and fail "File already exists ... was not read before this + // write" for a file the same task had just created. + await guardedWrite(task, "made.txt", "revised", "update") + + expect(mockedSafeWriteText).toHaveBeenCalledTimes(2) + }) + + it("re-observes after a successful edit write", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("doc.txt"), "v1") + const task = createMockTask({ observationRegistry: reg }) + mockedComputeVersionToken.mockResolvedValue("v1") + mockedSafeWriteText.mockImplementation(async () => { + mockedComputeVersionToken.mockResolvedValue("v2") + }) + + await guardedWrite(task, "doc.txt", "patched", "edit") + + expect(reg.get(abs("doc.txt"))?.version).toBe("v2") + }) + + it("keeps a published write successful when the post-publication token computation fails", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("doc.txt"), "v1") + const task = createMockTask({ observationRegistry: reg }) + mockedComputeVersionToken + .mockResolvedValueOnce("v1") // the CAS check at entry passes + .mockRejectedValueOnce({ code: "EACCES" }) // the post-publication token fails + + await expect(guardedWrite(task, "doc.txt", "new content", "update")).resolves.toBeUndefined() + + // The write published; the failed bookkeeping leaves the previous + // observation in place (a follow-up write then fails stale and re-reads). + expect(mockedSafeWriteText).toHaveBeenCalledTimes(1) + expect(reg.get(abs("doc.txt"))?.version).toBe("v1") + }) + }) + + describe("concurrency: per-path FIFO chain", () => { + it("two concurrent updates on one path - exactly one publishes, the other fails stale", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("shared.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + // The first publish changes the on-disk state (new token). + mockedSafeWriteText.mockImplementation(async () => { + mockedComputeVersionToken.mockResolvedValue("v2") + }) + + const p1 = guardedWrite(task, "shared.txt", "first", "update") + const p2 = guardedWrite(task, "shared.txt", "second", "update") + const [r1, r2] = await Promise.allSettled([p1, p2]) + + if (r1.status !== "fulfilled" || r2.status !== "rejected") { + throw new Error("expected exactly one publish, got " + r1.status + " / " + r2.status) + } + expect(mockedSafeWriteText).toHaveBeenCalledTimes(1) + expect(r2.reason.message).toBe("stale version — re-read the file, then retry") + }) + + it("holds the submission-time token when the registry moves on while the write waits", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("queued-token.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + let releaseFirst: () => void = () => {} + const gate = new Promise((resolve) => { + releaseFirst = resolve + }) + let publishes = 0 + mockedSafeWriteText.mockImplementation(async (_path: string, content: string) => { + publishes += 1 + if (content === "first") { + await gate + } + // Each publish moves the on-disk token forward. + mockedComputeVersionToken.mockResolvedValue("v2") + }) + + const first = guardedWrite(task, "queued-token.txt", "first", "update") + const second = guardedWrite(task, "queued-token.txt", "second", "update") + await new Promise((resolve) => setImmediate(resolve)) + // A re-read lands while both writes are still in flight: the registry + // moves to v2 behind the queued writes. + reg.observe(abs("queued-token.txt"), "v2") + releaseFirst() + + await first + // The second write was submitted against v1. The first write published + // over that state, so the queued write fails stale instead of publishing + // content derived from the v1 read over the v2 file. An execution-time + // registry lookup would rescue it with the refreshed token and lose the + // first write's update. + await expect(second).rejects.toThrow("stale version") + expect(publishes).toBe(1) + }) + + it("observed-absent then two concurrent creates - the second fails stale", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("absent.txt"), "v1") // read before, file later vanished + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) + const task = createMockTask({ observationRegistry: reg }) + + let publishes = 0 + mockedSafeWriteText.mockImplementation(async () => { + publishes += 1 + if (publishes === 1) { + // After the first publish the file exists again under a new token. + mockedFsAccess.mockResolvedValue(undefined) + mockedComputeVersionToken.mockResolvedValue("v2") + } + }) + + const p1 = guardedWrite(task, "absent.txt", "first", "create") + const p2 = guardedWrite(task, "absent.txt", "second", "create") + const [r1, r2] = await Promise.allSettled([p1, p2]) + + if (r1.status !== "fulfilled" || r2.status !== "rejected") { + throw new Error("expected exactly one publish, got " + r1.status + " / " + r2.status) + } + expect(publishes).toBe(1) + expect(r2.reason.message).toContain("stale version") + expect(r2.reason.message).toContain("re-read the file, then retry") + }) + + it("the chain settles after a rejection - a later matching write still runs", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("settle.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v2") // already stale at v1 + const task = createMockTask({ observationRegistry: reg }) + + const p1 = guardedWrite(task, "settle.txt", "first", "update") + await expect(p1).rejects.toThrow("stale version") + + // No resetChain: the rejected link must not wedge the chain. The + // caller re-reads the file (observation refreshed to v2) and retries. + reg.observe(abs("settle.txt"), "v2") + const p2 = guardedWrite(task, "settle.txt", "second", "update") + await expect(p2).resolves.toBeUndefined() + + expect(mockedSafeWriteText).toHaveBeenCalledTimes(1) + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("settle.txt"), "second", { + verifyBeforeCommit: expect.any(Function), + }) + }) + + // The 2359 lane carries an open CodeRabbit row (PRRT_kwDOSK-bwM61HM7S, guardedWrite.ts:163 at + // 4e1c51c31c) about a reserved chain promise that stays unattached for an event-loop turn, so + // Node can report an unhandled rejection even though the caller handles the write's promise. + // This unit has no reserved promise to leave unattached: enqueue composes with prev.then(fn, fn) + // (src/core/tools/guardedWrite.ts:87) and returns the same link its own bookkeeping handlers + // are attached to. Measured rather than asserted - a real cancellation rejection queued behind + // a gated link, a real macrotask flush, and the process's own unhandled-rejection signal. + it("reports no unhandled rejection for a queued write that rejects behind a pending one", async () => { + const unhandled: unknown[] = [] + const onUnhandledRejection = (reason: unknown): void => { + unhandled.push(reason) + } + let releaseFirst: () => void = () => {} + process.on("unhandledRejection", onUnhandledRejection) + try { + const firstGate = new Promise(function (resolve) { + releaseFirst = resolve + }) + mockedSafeWriteText.mockImplementationOnce(async () => { + await firstGate + }) + // The target is absent, the only state an unobserved "create" may publish into: both + // links' entry probes report ENOENT, so the first reaches the gated publish and the + // second stays queued behind it. + mockedFsAccess.mockRejectedValue( + Object.assign(new Error("ENOENT: no such file or directory, access"), { code: "ENOENT" }), + ) + const task = createMockTask() + const first = guardedWrite(task, "unhandled.txt", "first", "create") + const second = guardedWrite(task, "unhandled.txt", "second", "create") + await new Promise(function (resolve) { + setImmediate(resolve) + }) + // The second link is still queued behind the gated first one when the task goes away. + task.requestAbort(false) + releaseFirst() + await first + await expect(second).rejects.toThrow(/was cancelled/) + // Node decides unhandledness once the microtask queue drains, so one macrotask turn is + // long enough for a rejection nobody attached a handler to in time to be reported. + await new Promise(function (resolve) { + setImmediate(resolve) + }) + } finally { + releaseFirst() + process.off("unhandledRejection", onUnhandledRejection) + } + + expect(unhandled).toEqual([]) + }) + + it("a write submitted after an earlier one settled still serializes in submission order", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("evict.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + // A first write settles before the next two are submitted. + const p1 = guardedWrite(task, "evict.txt", "first", "update") + await expect(p1).resolves.toBeUndefined() + + // Two rapid writes submitted after that settlement must still run one at a + // time in submission order. Whether the settled entry has been evicted from the + // path map is not observable from here, so the test does not claim it. + const order: string[] = [] + mockedSafeWriteText.mockImplementation(async (_path: string, content: string) => { + order.push(content) + }) + const p2 = guardedWrite(task, "evict.txt", "second", "update") + const p3 = guardedWrite(task, "evict.txt", "third", "update") + await Promise.all([p2, p3]) + + expect(order).toEqual(["second", "third"]) + // Three publishes in total: the settled first write plus the two + // serialized rapid writes. + expect(mockedSafeWriteText).toHaveBeenCalledTimes(3) + }) + + it("writes on different paths are independent (no cross-path serialization)", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("a.txt"), "v1") + reg.observe(abs("b.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + const p1 = guardedWrite(task, "a.txt", "a", "update") + const p2 = guardedWrite(task, "b.txt", "b", "update") + await Promise.all([p1, p2]) + + expect(mockedSafeWriteText).toHaveBeenCalledTimes(2) + }) + }) + + describe("publication-time re-verification (check-to-rename window)", () => { + /** + * Drive a simulated race: the mocked publish primitive behaves like + * safeWriteText and invokes the pre-commit verification immediately + * before the commit rename. The "external writer" acts in that window + * (after the guard's entry check, before the pre-commit re-verification) + * by changing the mocked on-disk state. Returns a published() probe. + */ + const mockPublishWithRace = (mutate: () => void): (() => boolean) => { + let published = false + mockedSafeWriteText.mockImplementation( + async (_path: string, _content: string, options?: SafeWriteTextOptions) => { + mutate() + await options?.verifyBeforeCommit?.() + published = true + }, + ) + return () => published + } + + it("createIfAbsent rejects when an external writer creates the file between verification and publication", async () => { + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) // absent at entry + const task = createMockTask() + const published = mockPublishWithRace(() => { + // -- the race: an external writer publishes first ------------------ + mockedFsAccess.mockResolvedValue(undefined) // the file now exists + }) + + await expect(guardedWrite(task, "raced.txt", "mine", "create")).rejects.toThrow( + "file not read yet — read the file, then retry", + ) + // nothing was published: the competing writer's file is preserved + expect(published()).toBe(false) + // entry check + pre-commit re-check + expect(mockedFsAccess).toHaveBeenCalledTimes(2) + }) + + it("replaceIfVersion rejects when an external writer modifies the file between verification and publication", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("raced.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") // matches at entry + const task = createMockTask({ observationRegistry: reg }) + const published = mockPublishWithRace(() => { + // -- the race: an external writer rewrites the file ---------------- + mockedComputeVersionToken.mockResolvedValue("v-external") + }) + + await expect(guardedWrite(task, "raced.txt", "mine", "update")).rejects.toThrow( + "stale version — re-read the file, then retry", + ) + expect(published()).toBe(false) + }) + + it("replaceIfVersion rejects deleted-after-read when the file is deleted between verification and publication", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("raced.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + const published = mockPublishWithRace(() => { + // -- the race: an external writer deletes the file ----------------- + mockedComputeVersionToken.mockRejectedValue({ code: "ENOENT" }) + }) + + await expect(guardedWrite(task, "raced.txt", "mine", "update")).rejects.toThrow( + "File was deleted after it was read", + ) + expect(published()).toBe(false) + }) + + it("rethrows a non-ENOENT token failure from the pre-commit verification", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("raced.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + const failure = { code: "EACCES" } + const published = mockPublishWithRace(() => { + // A real I/O failure in the window is not a guard verdict: it must propagate. + mockedComputeVersionToken.mockRejectedValue(failure) + }) + + await expect(guardedWrite(task, "raced.txt", "mine", "update")).rejects.toBe(failure) + expect(published()).toBe(false) + }) + + it("rethrows non-guard I/O failures from the pre-commit verification verbatim", async () => { + const failure = { code: "EACCES" } + mockedFsAccess.mockRejectedValue({ code: "ENOENT" }) // absent at entry + const task = createMockTask() + const published = mockPublishWithRace(() => { + // the pre-commit re-check hits a real I/O failure, not a guard verdict + mockedFsAccess.mockRejectedValue(failure) + }) + + await expect(guardedWrite(task, "io-race.txt", "x", "create")).rejects.toBe(failure) + expect(published()).toBe(false) + }) + }) + + describe("path resolution", () => { + it("resolves a relative path against task.cwd", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("sub/dir.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + await guardedWrite(task, "sub/dir.txt", "content", "update") + + expect(mockedSafeWriteText).toHaveBeenCalledWith(abs("sub/dir.txt"), "content", { + verifyBeforeCommit: expect.any(Function), + }) + }) + + it("normalizes an already-absolute input (trailing separator) to the observation key", async () => { + const reg = new ObservationRegistry() + const canonical = abs("sub/dir.txt") + // ReadFileTool observes under path.resolve(task.cwd, relPath) — the + // canonical spelling. A write addressed with a trailing separator used + // to bypass the observation (isAbsolute passthrough) and fail + // "File already exists" / "File not read yet" for a file that was read. + reg.observe(canonical, "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + await guardedWrite(task, canonical + "/", "content", "update") + + expect(mockedSafeWriteText).toHaveBeenCalledTimes(1) + expect(mockedSafeWriteText).toHaveBeenCalledWith(canonical, "content", { + verifyBeforeCommit: expect.any(Function), + }) + }) + + it("serializes two spellings of one file through a single chain key", async () => { + const reg = new ObservationRegistry() + const canonical = abs("shared2.txt") + reg.observe(canonical, "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + // The first publish changes the on-disk state (new token). + mockedSafeWriteText.mockImplementation(async () => { + mockedComputeVersionToken.mockResolvedValue("v2") + }) + + // Plain spelling vs the trailing-separator spelling: with one chain key + // they are strictly ordered (first matches v1, second sees v2). + const p1 = guardedWrite(task, canonical, "first", "update") + const p2 = guardedWrite(task, canonical + "/", "second", "update") + const [r1, r2] = await Promise.allSettled([p1, p2]) + + if (r1.status !== "fulfilled" || r2.status !== "rejected") { + throw new Error("expected exactly one publish, got " + r1.status + " / " + r2.status) + } + expect(mockedSafeWriteText).toHaveBeenCalledTimes(1) + expect(r2.reason.message).toBe("stale version — re-read the file, then retry") + }) + }) + + describe("resetChain", () => { + it("detaches pending links so later writes start a fresh chain", async () => { + const reg = new ObservationRegistry() + reg.observe(abs("x.txt"), "v1") + mockedComputeVersionToken.mockResolvedValue("v1") + const task = createMockTask({ observationRegistry: reg }) + + await guardedWrite(task, "x.txt", "a", "update") + resetChain() + await guardedWrite(task, "x.txt", "b", "update") + + expect(mockedSafeWriteText).toHaveBeenLastCalledWith(abs("x.txt"), "b", { + verifyBeforeCommit: expect.any(Function), + }) + }) + }) +}) + +describe("task cancellation (S4a, epic #1375)", () => { + // The file's shared beforeEach lives inside the main describe, so this top-level one needs its + // own resets - otherwise assertions here see the previous test's recorded calls. + beforeEach(() => { + mockedSafeWriteText.mockClear() + mockedFsAccess.mockClear() + mockedComputeVersionToken.mockClear() + mockedFsAccess.mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + }) + + it("drops a queued write when the task is aborted while it waits behind another write", async () => { + let releaseFirst: () => void = () => {} + const firstGate = new Promise(function (resolve) { + releaseFirst = resolve + }) + mockedSafeWriteText.mockImplementationOnce(async () => { + await firstGate + }) + const task = createMockTask() + const first = guardedWrite(task, "queued.txt", "first", "create") + const second = guardedWrite(task, "queued.txt", "second", "create") + // Let the first link enter the publish (the chain runs on microtasks) before the + // disposal lands: Task.dispose() sets task.abort while the second write is still + // queued behind it. + await new Promise(function (resolve) { + setImmediate(resolve) + }) + task.requestAbort(false) + releaseFirst() + await first + await expect(second).rejects.toThrow(/was cancelled/) + // Only the first write published; the cancelled one touched nothing. + expect( + mockedSafeWriteText.mock.calls.map(function (call) { + return call[1] + }), + ).toEqual(["first"]) + }) + + it("refuses an already-cancelled task's write before any I/O", async () => { + const task = createMockTask() + task.requestAbort(false) + await expect(guardedWrite(task, "gone.txt", "x", "create")).rejects.toThrow(/was cancelled/) + expect(mockedSafeWriteText).not.toHaveBeenCalled() + expect(mockedFsAccess).not.toHaveBeenCalled() + }) +}) diff --git a/src/core/tools/__tests__/readFileTool.spec.ts b/src/core/tools/__tests__/readFileTool.spec.ts index 52e04415ea..0872809396 100644 --- a/src/core/tools/__tests__/readFileTool.spec.ts +++ b/src/core/tools/__tests__/readFileTool.spec.ts @@ -14,6 +14,9 @@ import os from "os" import path from "path" +import type { Stats } from "fs" + +import type { LegacyReadFileParams } from "@roo-code/types" import type { ToolUse } from "../../../shared/tools" import type { Task, ToolExecutionContext } from "../../task/Task" @@ -21,6 +24,7 @@ import type { Task, ToolExecutionContext } from "../../task/Task" import { isBinaryFile } from "isbinaryfile" import { readFileTool } from "../ReadFileTool" +import { ObservationRegistry } from "../../task/observationRegistry" import { formatResponse } from "../../prompts/responses" import { validateImageForProcessing, processImageFile, isSupportedImageFormat } from "../helpers/imageHelpers" import { extractTextFromFile, getSupportedBinaryFormats } from "../../../integrations/misc/extract-text" @@ -135,6 +139,7 @@ interface MockTaskOptions { rooIgnoreAllowed?: boolean maxImageFileSize?: number maxTotalImageSize?: number + observationRegistry?: ObservationRegistry } function createMockTask(options: MockTaskOptions = {}) { @@ -142,6 +147,9 @@ function createMockTask(options: MockTaskOptions = {}) { return { cwd: "/test/workspace", + // Mirror Task: every task always owns an observation registry (A2, #1375). + // Tests asserting on observations pass their own instance via options. + observationRegistry: options.observationRegistry ?? new ObservationRegistry(), api: { getModel: vi.fn().mockReturnValue({ info: { supportsImages }, @@ -188,7 +196,18 @@ describe("ReadFileTool", () => { vi.clearAllMocks() // Default mock implementations - mockedFsStat.mockResolvedValue({ isDirectory: () => false } as any) + // The stat default carries BigIntStats fields (A2, epic #1375): reads now + // token-ize the pre/post stats, so the default must look like a real bigint stat. + // Tests overriding it do so per-call with mockResolvedValue(Once). + mockedFsStat.mockResolvedValue({ + isDirectory: () => false, + dev: BigInt(1), + ino: BigInt(2), + size: BigInt(300), + mtimeNs: BigInt(4_000_000_000n), + ctimeNs: BigInt(5_000_000_000n), + // Cast: the mock only implements the members the tool and versionToken read. + } as unknown as Stats) mockedIsBinaryFile.mockResolvedValue(false) mockedFsReadFile.mockResolvedValue(Buffer.from("test content")) mockedReadWithSlice.mockReturnValue({ @@ -1688,5 +1707,330 @@ describe("ReadFileTool", () => { expect(mockTask.didToolFailInCurrentTurn).toBe(true) }) + + describe("observation registry", () => { + it("records an observation on successful read of an existing file", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + // Override the beforeEach default stat mock with proper BigIntStats. + mockedFsStat.mockResolvedValue({ + isDirectory: () => false, + dev: BigInt(1), + ino: BigInt(2), + size: BigInt(300), + mtimeNs: BigInt(4_000_000_000n), + ctimeNs: BigInt(5_000_000_000n), + // Cast: the mock only implements the members the tool and versionToken read. + } as unknown as Stats) + mockedIsBinaryFile.mockResolvedValue(false) + + // Spy on observe to capture the exact key used (Windows path.resolve may use backslashes). + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute({ path: "existing.ts" }, mockTask as unknown as Task, callbacks) + + // Verify the tool called observe exactly once with a valid token. + expect(observeSpy).toHaveBeenCalledTimes(1) + const [calledPath, calledVersion] = observeSpy.mock.calls[0] + expect(calledPath).toContain("existing.ts") + expect(calledVersion).toMatch(/^\d+:\d+:\d+:\d+:\d+$/) + + // The token is built from nanosecond fields, so both stats the read takes around the + // file must be the bigint variant. Counting the calls that asked for it is what makes + // this load-bearing: the mocked stat answers any options with bigint fields, so a + // looser assertion would pass while one of the two calls silently dropped the option + // and threw on a real filesystem, where mtimeNs and ctimeNs are undefined. + const bigintStats = mockedFsStat.mock.calls.filter((call) => { + const options = call[1] as { bigint?: boolean } | undefined + return options?.bigint === true && String(call[0]).includes("existing.ts") + }) + expect(bigintStats).toHaveLength(2) + + // Verify get() returns the same data using the spy-captured key. + const obs = reg.get(calledPath) + expect(obs).toBeDefined() + expect(obs!.version).toBe(calledVersion) + }) + + it("a failed read (absent path) leaves the registry size 0 and does not throw", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + mockedFsReadFile.mockRejectedValue(new Error("ENOENT")) + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute({ path: "missing.ts" }, mockTask as unknown as Task, callbacks) + + // observationRegistry is guaranteed present because we passed it in createMockTask. + const reg = mockTask.observationRegistry + expect(reg).toBeDefined() + expect(reg!.size).toBe(0) + }) + + it("records an observation for legacy-format reads of existing files", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + mockedFsStat.mockResolvedValue({ + isDirectory: () => false, + dev: BigInt(1), + ino: BigInt(2), + size: BigInt(300), + mtimeNs: BigInt(4_000_000_000n), + ctimeNs: BigInt(5_000_000_000n), + // Cast: the mock only implements the members the tool and versionToken read. + } as unknown as Stats) + mockedIsBinaryFile.mockResolvedValue(false) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + // Typed legacy (pre-refactor) params: the multi-file format with the + // _legacyFormat discriminant (see LegacyReadFileParams). + const legacyParams: LegacyReadFileParams = { + files: [{ path: "legacy.ts" }], + _legacyFormat: true, + } + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute(legacyParams, mockTask as unknown as Task, callbacks) + + expect(observeSpy).toHaveBeenCalledTimes(1) + const [calledPath, calledVersion] = observeSpy.mock.calls[0] + expect(calledPath).toContain("legacy.ts") + expect(calledVersion).toMatch(/^\d+:\d+:\d+:\d+:\d+$/) + + // Same requirement on the legacy multi-file path, which resolves its own stats. + + // The token is built from nanosecond fields, so both stats the read takes around the + // file must be the bigint variant. Counting the calls that asked for it is what makes + // this load-bearing: the mocked stat answers any options with bigint fields, so a + // looser assertion would pass while one of the two calls silently dropped the option + // and threw on a real filesystem, where mtimeNs and ctimeNs are undefined. + const bigintStats = mockedFsStat.mock.calls.filter((call) => { + const options = call[1] as { bigint?: boolean } | undefined + return options?.bigint === true && String(call[0]).includes("legacy.ts") + }) + expect(bigintStats).toHaveLength(2) + }) + + it("does not observe when the file mutates between the pre-read and post-read stats", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + const preStats = { + isDirectory: () => false, + dev: BigInt(1), + ino: BigInt(2), + size: BigInt(300), + mtimeNs: BigInt(4_000_000_000n), + ctimeNs: BigInt(5_000_000_000n), + } + // A mutation lands mid-read: the post-read stat differs. + const postStats = { ...preStats, size: BigInt(301) } + + // Call order: directory check, pre-read stat, post-read stat. + mockedFsStat + .mockResolvedValueOnce({ isDirectory: () => false } as unknown as Stats) + .mockResolvedValueOnce(preStats as unknown as Stats) + .mockResolvedValueOnce(postStats as unknown as Stats) + mockedIsBinaryFile.mockResolvedValue(false) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute({ path: "mutated.ts" }, mockTask as unknown as Task, callbacks) + + // The read itself succeeded, but the target stays unobserved: the content the + // model received is not the on-disk state, so observing it would let a later + // write match a token the model never saw. + expect(observeSpy).not.toHaveBeenCalled() + expect(reg.size).toBe(0) + expect(mockTask.didToolFailInCurrentTurn).toBe(false) + }) + + it("leaves the target unobserved without failing the read when the pre-read stat fails", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + // Directory check OK; the pre-read stat fails (caught, target unobserved). + mockedFsStat + .mockResolvedValueOnce({ isDirectory: () => false } as unknown as Stats) + .mockRejectedValueOnce(new Error("EACCES")) + mockedIsBinaryFile.mockResolvedValue(false) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute({ path: "stat-fail.ts" }, mockTask as unknown as Task, callbacks) + + expect(observeSpy).not.toHaveBeenCalled() + expect(reg.size).toBe(0) + // The read still succeeds — a stat failure never fails the read. + expect(mockTask.didToolFailInCurrentTurn).toBe(false) + expect(callbacks.pushToolResult).toHaveBeenCalled() + }) + + it("leaves the target unobserved without failing the read when the post-read stat fails", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + const okStats = { + isDirectory: () => false, + dev: BigInt(1), + ino: BigInt(2), + size: BigInt(300), + mtimeNs: BigInt(4_000_000_000n), + ctimeNs: BigInt(5_000_000_000n), + } + // Directory check and pre-read stat OK; the post-read stat fails. + mockedFsStat + .mockResolvedValueOnce({ isDirectory: () => false } as unknown as Stats) + .mockResolvedValueOnce(okStats as unknown as Stats) + .mockRejectedValueOnce(new Error("EACCES")) + mockedIsBinaryFile.mockResolvedValue(false) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute({ path: "post-stat-fail.ts" }, mockTask as unknown as Task, callbacks) + + expect(observeSpy).not.toHaveBeenCalled() + expect(reg.size).toBe(0) + expect(mockTask.didToolFailInCurrentTurn).toBe(false) + expect(callbacks.pushToolResult).toHaveBeenCalled() + }) + + it("legacy format: does not observe when the file mutates between the pre-read and post-read stats", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + const preStats = { + isDirectory: () => false, + dev: BigInt(1), + ino: BigInt(2), + size: BigInt(300), + mtimeNs: BigInt(4_000_000_000n), + ctimeNs: BigInt(5_000_000_000n), + } + // Call order: directory check, pre-read stat, post-read stat (mutated). + mockedFsStat + .mockResolvedValueOnce({ isDirectory: () => false } as unknown as Stats) + .mockResolvedValueOnce(preStats as unknown as Stats) + .mockResolvedValueOnce({ ...preStats, size: BigInt(301) } as unknown as Stats) + mockedIsBinaryFile.mockResolvedValue(false) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + const legacyParams: LegacyReadFileParams = { + files: [{ path: "legacy-mutated.ts" }], + _legacyFormat: true, + } + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute(legacyParams, mockTask as unknown as Task, callbacks) + + expect(observeSpy).not.toHaveBeenCalled() + expect(reg.size).toBe(0) + expect(mockTask.didToolFailInCurrentTurn).toBe(false) + }) + + it("legacy format: leaves the target unobserved when a stat fails without failing the read", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + // Directory check OK; the pre-read stat fails (caught, target unobserved). + mockedFsStat + .mockResolvedValueOnce({ isDirectory: () => false } as unknown as Stats) + .mockRejectedValueOnce(new Error("EACCES")) + mockedIsBinaryFile.mockResolvedValue(false) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + const legacyParams: LegacyReadFileParams = { + files: [{ path: "legacy-stat-fail.ts" }], + _legacyFormat: true, + } + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute(legacyParams, mockTask as unknown as Task, callbacks) + + expect(observeSpy).not.toHaveBeenCalled() + expect(reg.size).toBe(0) + expect(mockTask.didToolFailInCurrentTurn).toBe(false) + expect(callbacks.pushToolResult).toHaveBeenCalled() + }) + it("legacy format: leaves the target unobserved when the post-read stat fails", async () => { + const mockTask = createMockTask({ + observationRegistry: new ObservationRegistry(), + }) + const callbacks = createMockCallbacks() + + const okStats = { + isDirectory: () => false, + dev: BigInt(1), + ino: BigInt(2), + size: BigInt(300), + mtimeNs: BigInt(4_000_000_000n), + ctimeNs: BigInt(5_000_000_000n), + } + // Directory check and pre-read stat OK; the post-read stat fails. + mockedFsStat + .mockResolvedValueOnce({ isDirectory: () => false } as unknown as Stats) + .mockResolvedValueOnce(okStats as unknown as Stats) + .mockRejectedValueOnce(new Error("EACCES")) + mockedIsBinaryFile.mockResolvedValue(false) + + const reg = mockTask.observationRegistry! + const observeSpy = vi.spyOn(reg, "observe") + + const legacyParams: LegacyReadFileParams = { + files: [{ path: "legacy-post-stat-fail.ts" }], + _legacyFormat: true, + } + + // Cast: the mock task only implements the members ReadFileTool.execute touches. + await readFileTool.execute(legacyParams, mockTask as unknown as Task, callbacks) + + expect(observeSpy).not.toHaveBeenCalled() + expect(reg.size).toBe(0) + expect(mockTask.didToolFailInCurrentTurn).toBe(false) + expect(callbacks.pushToolResult).toHaveBeenCalled() + }) + it("two separate Task-owned registries are independent", async () => { + const regA = new ObservationRegistry() + const regB = new ObservationRegistry() + regA.observe("/shared.ts", "v1") + expect(regA.get("/shared.ts")!.version).toBe("v1") + expect(regB.get("/shared.ts")).toBeUndefined() + regB.observe("/shared.ts", "v2") + expect(regA.get("/shared.ts")!.version).toBe("v1") + expect(regB.get("/shared.ts")!.version).toBe("v2") + }) + }) }) }) diff --git a/src/core/tools/guardedWrite.ts b/src/core/tools/guardedWrite.ts new file mode 100644 index 0000000000..ac39ac1875 --- /dev/null +++ b/src/core/tools/guardedWrite.ts @@ -0,0 +1,402 @@ +/** + * Guarded-write compare-and-swap core (upstream epic #1375, phase A4a). + * + * Wraps the S3 safeWriteText publish primitive behind version-token guards so + * that every write is deterministic: + * + * - an unobserved target may only be created when it is absent + * (createIfAbsent); + * - an observed target is published only when the on-disk version token still + * matches the token recorded at read time (replaceIfVersion); + * - an edit-style write requires a prior observation (unobservedEditGuard). + * + * A per-absolute-path FIFO chain of tail promises orders concurrent + * in-process writes to the same path: the first matching write wins, the rest + * fail stale. Observations come from the task's S2 ObservationRegistry and are + * captured at submission time; a successful publication re-observes the path + * with the published token so sequential writes from one task compare against + * the content that task published, not against its pre-write read. + * + * Check-to-publication window (CodeRabbit review, PRs #1405 / #1413): every + * guard predicate is enforced TWICE -- once at entry and once at publication + * time. The publication-time re-verification runs inside safeWriteText + * immediately before the atomic commit rename (its verifyBeforeCommit + * option), while the per-path FIFO chain holds the serialization across the + * whole verify+publish window, so the predicate is re-checked against the + * state the rename will actually replace and concurrent in-process writers + * stay fully ordered. + * + * Residual cross-process window: an external writer (another process) can + * still create or modify the target in the short interval between the + * pre-commit re-verification and the commit rename. A cross-platform atomic + * conditional publication would require an OS-level primitive beyond + * fs.promises (or a shared lock protocol every writer honors) and is tracked + * as a follow-up of epic #1375. + */ + +import * as fs from "fs/promises" +import * as path from "path" + +import { safeWriteText } from "../../services/file-safety/safeWriteText" +import { computeVersionToken } from "../../utils/versionToken" +import type { Task } from "../task/Task" + +// -- Types ------------------------------------------------------------------ + +/** Write kind that drives guard selection. */ +export type GuardedWriteKind = "create" | "update" | "edit" + +/** Internal error thrown when a guard rejects a write. */ +class GuardRejectedError extends Error { + constructor( + message: string, + readonly path: string, + ) { + super(message) + this.name = "GuardRejectedError" + } +} + +// -- Per-path tail-promise chain -------------------------------------------- + +/** + * Per-absolute-path FIFO chain of pending guarded writes (tail promise per + * path). Every write enqueues onto the current tail for its path, so + * concurrent writes to the same path run one at a time in submission order. + * + * The chain never leaks a rejection through itself: each link settles, a + * rejected link is skipped by the next writer (a failed write must not block + * later writes to the same path), and every caller receives its own link + * promise to handle. + * + * Settled entries are evicted (below), so a long-lived extension does not + * accumulate a map entry per distinct written path. + */ +const pendingChains = new Map>() + +/** + * Enqueue a write operation on the per-path FIFO chain. + * + * Returns the promise for this link; it always settles. A prior link that + * rejected is skipped, not propagated. The map entry for this link is + * deleted once it settles — but only while it is still the current tail for + * the path, so a replacement enqueued in the meantime keeps ownership. + */ +function enqueue(pathKey: string, fn: () => Promise): Promise { + const prev = pendingChains.get(pathKey) ?? Promise.resolve() + const next = prev.then(fn, fn) + pendingChains.set(pathKey, next) + void next.then( + () => { + if (pendingChains.get(pathKey) === next) { + pendingChains.delete(pathKey) + } + }, + () => { + if (pendingChains.get(pathKey) === next) { + pendingChains.delete(pathKey) + } + }, + ) + return next +} + +// -- Guard primitives -------------------------------------------------------- + +/** + * Extract a Node errno code (e.g. "ENOENT") from a thrown value, or + * undefined when the value carries none. + */ +function errorCode(error: unknown): string | undefined { + return typeof error === "object" && error !== null && "code" in error + ? (error as { code?: string }).code + : undefined +} + +/** True when the path is absent on disk (fs.access reports ENOENT). */ +async function fileIsAbsent(absolutePath: string): Promise { + try { + await fs.access(absolutePath) + return false + } catch (error: unknown) { + return errorCode(error) === "ENOENT" + } +} + +/** Build the read-first remediation error for an existing target. */ +function alreadyExistsError(absolutePath: string): GuardRejectedError { + // Exact remediation text required by the S4a tracking issue (#1399). + return new GuardRejectedError("file not read yet — read the file, then retry", absolutePath) +} + +/** Build the stale-version remediation error. */ +function staleVersionError(absolutePath: string, expectedVersion: string, currentVersion: string): GuardRejectedError { + // Exact remediation text required by #1399; the version detail is diagnostic only. + void expectedVersion + void currentVersion + return new GuardRejectedError("stale version — re-read the file, then retry", absolutePath) +} + +/** Build the deleted-after-read remediation error. */ +function deletedAfterReadError(absolutePath: string, expectedVersion: string): GuardRejectedError { + return new GuardRejectedError( + "File was deleted after it was read -- the version recorded at read time (" + + expectedVersion + + ") no longer exists; re-read the file, then retry.", + absolutePath, + ) +} + +/** + * Publish content only if the target file does not exist. + * + * The absence predicate is enforced at entry AND at publication time: the + * pre-commit re-check (verifyBeforeCommit, run by safeWriteText immediately + * before the commit rename) rejects with the same remediation when an + * external writer created the file in the check-to-rename window, instead of + * overwriting it. + * + * Rejects with a loud remediation error when the file already exists: the + * write was issued for a file that was never read, so the caller must read + * the file first, then retry. + */ +export async function createIfAbsent(absolutePath: string, content: string): Promise { + try { + await fs.access(absolutePath) + } catch (error: unknown) { + if (errorCode(error) !== "ENOENT") { + // A real I/O failure (EACCES, EIO, ...) -- not a guard verdict. + throw error + } + // Absent at entry. safeWriteText re-verifies absence at the last + // moment before the commit rename (see verifyStillAbsent) so a writer + // that creates the file in the check-to-rename window is rejected, + // not overwritten. + await safeWriteText(absolutePath, content, { + verifyBeforeCommit: () => verifyStillAbsent(absolutePath), + }) + return + } + + throw alreadyExistsError(absolutePath) +} + +/** + * Pre-commit absence check for createIfAbsent (publication-time re- + * verification): rejects with the standard read-first remediation when the + * target exists at publication time. ENOENT (still absent) passes; any other + * I/O error is rethrown verbatim (a real failure, not a guard verdict). + */ +async function verifyStillAbsent(absolutePath: string): Promise { + try { + await fs.access(absolutePath) + } catch (error: unknown) { + if (errorCode(error) === "ENOENT") return + throw error + } + throw alreadyExistsError(absolutePath) +} + +/** + * Publish content only if the current on-disk version token equals + * expectedVersion (the token observed at read time). + * + * The version predicate is enforced at entry AND at publication time: the + * pre-commit re-check (verifyBeforeCommit, run by safeWriteText immediately + * before the commit rename) rejects stale with the same remediation when an + * external writer modified the file in the check-to-rename window, instead + * of overwriting it. + * + * On a match the content is published via the S3 safeWriteText primitive; on + * a mismatch the write is rejected stale with a re-read-then-retry + * remediation suffix. + */ +export async function replaceIfVersion(absolutePath: string, expectedVersion: string, content: string): Promise { + let currentVersion: string + try { + currentVersion = await computeVersionToken(absolutePath) + } catch (error: unknown) { + if (errorCode(error) === "ENOENT") { + // Observed + absent is a create row of the #1399 decision table: the file the + // caller read was deleted before this write, so creating it is the safe action. + await createIfAbsent(absolutePath, content) + return + } + // A real I/O failure (EACCES, EIO, ...) -- not a guard verdict. + throw error + } + + if (currentVersion !== expectedVersion) { + throw staleVersionError(absolutePath, expectedVersion, currentVersion) + } + + // Match at entry. safeWriteText re-verifies the token at the last moment + // before the commit rename (see verifyVersionUnchanged) so a writer that + // modifies the file in the check-to-rename window is rejected stale, not + // overwritten. + await safeWriteText(absolutePath, content, { + verifyBeforeCommit: () => verifyVersionUnchanged(absolutePath, expectedVersion), + }) +} + +/** + * Pre-commit version check for replaceIfVersion (publication-time re- + * verification): rejects with the standard stale / deleted-after-read + * remediation when the on-disk token no longer matches expectedVersion at + * publication time. Any other I/O error is rethrown verbatim (a real failure, + * not a guard verdict). + */ +async function verifyVersionUnchanged(absolutePath: string, expectedVersion: string): Promise { + let currentVersion: string + try { + currentVersion = await computeVersionToken(absolutePath) + } catch (error: unknown) { + if (errorCode(error) === "ENOENT") { + throw deletedAfterReadError(absolutePath, expectedVersion) + } + throw error + } + if (currentVersion !== expectedVersion) { + throw staleVersionError(absolutePath, expectedVersion, currentVersion) + } +} + +/** + * Unobserved-edit guard: an edit-style write without a prior observation is + * rejected before any I/O. The literal-match / patch logic stays with the + * tools in S4b; this guard only verifies that a read happened first. + * + * Returns Promise because the rejection is total: this function + * never resolves. + */ +export async function unobservedEditGuard(absolutePath: string): Promise { + // Exact remediation text required by #1399. + throw new GuardRejectedError("file not read yet — read the file, then retry", absolutePath) +} + +// -- Public API -------------------------------------------------------------- + +/** + * Resolve a relative or absolute path against task.cwd. + * + * path.resolve also normalizes an already-absolute input (collapsing "." / ".." + * segments and trailing separators), so the key always matches the + * ObservationRegistry key recorded at read time (ReadFileTool observes under + * path.resolve(task.cwd, relPath)) and two spellings of one file share one + * FIFO chain. + */ +function resolveAbsolutePath(task: Task, relPathOrAbsolute: string): string { + return path.resolve(task.cwd, relPathOrAbsolute) +} + +/** + * A queued guarded write reached the head of its path's chain after the task that + * issued it had already been aborted or disposed. Task.dispose() sets the same + * `abort` flag that abortTask() sets, so that flag is the disposal signal visible + * at this layer. + */ +export class CancelledTaskWriteError extends Error { + readonly path: string + constructor(absolutePath: string) { + super( + `Guarded write for ${absolutePath} was cancelled -- the task was aborted or disposed ` + + "before its turn in the per-path write queue; nothing was published.", + ) + this.name = "CancelledTaskWriteError" + this.path = absolutePath + } +} + +/** + * Guarded write entry point. + * + * 1. Resolves the absolute path against task.cwd. + * 2. Captures the task's S2 observation for the path at SUBMISSION time, not + * when the queued link runs: the content being written was derived from + * the read this observation records, so the CAS must compare against that + * token even if the registry moves on - through a re-read or through this + * task's own earlier write re-observing the path - while the link waits + * behind another write. A queued write whose token no longer matches fails + * stale and the caller re-reads; publishing it anyway would overwrite + * content the submitted write never saw. + * 3. Consults the captured observation to pick the guard: + * - unobserved + create/update: createIfAbsent (rejects if it exists); + * - observed + create on a file that vanished after the read: recreate; + * - observed otherwise: replaceIfVersion (CAS on the S1 version token); + * - unobserved + edit: unobservedEditGuard. + * The guard runs on the per-path FIFO chain so concurrent writes to the + * same path are deterministically ordered. The chain holds the + * serialization across the whole verify+publish window, and the guard's + * publication-time re-verification (inside safeWriteText, immediately + * before the commit rename) closes the check-to-rename window for writers + * serialized by the chain. + * 4. After a successful publication the new on-disk token is recorded back + * into the registry, so a follow-up write from the same task compares + * against the content this write published instead of failing stale + * against its own output (or treating a file it just created as + * unobserved). The re-observation runs inside the chain link, before the + * link settles, so the next queued link and the next submission both see + * the refreshed token. A token failure after a successful publication is + * swallowed: the write itself succeeded, and the registry keeps the + * previous observation, which a follow-up write already handles. + */ +export async function guardedWrite( + task: Task, + relPathOrAbsolute: string, + content: string, + kind: GuardedWriteKind = "update", +): Promise { + const absolutePath = resolveAbsolutePath(task, relPathOrAbsolute) + // Submission-time capture - see step 2 above. + const obs = task.observationRegistry.get(absolutePath) + + return enqueue(absolutePath, async () => { + // The link can reach the head of the queue long after the task that issued it is + // gone (panel closed, task switched, abort landed while another write held the + // path). Running it then would publish for a task that no longer serves requests + // and re-observe the path, so the write stops here instead. + if (task.abort) { + throw new CancelledTaskWriteError(absolutePath) + } + + if (obs === undefined) { + // Edit-style writes require a prior read: no observation, no write. + if (kind === "edit") { + await unobservedEditGuard(absolutePath) + } + // Never read: only an absent target may be created. (The edit guard + // above rejects before reaching this line.) + await createIfAbsent(absolutePath, content) + } else if (kind === "create" && (await fileIsAbsent(absolutePath))) { + // A "create" on a file that vanished after the read recreates it. + // Observed "edit" and "update" writes never reach the existence probe - + // the condition short-circuits on kind - and take the CAS branch below. + await createIfAbsent(absolutePath, content) + } else { + // The version recorded at read time must still match the on-disk + // token: for "edit" and "update", and for "create" on a file that + // still exists. + await replaceIfVersion(absolutePath, obs.version, content) + } + + // Re-observe the published version (step 4). A failure to compute the + // token after a successful publication must not turn the published + // write into a failed one, so it is swallowed here and the registry + // keeps the previous observation. + try { + const published = await computeVersionToken(absolutePath) + task.observationRegistry.observe(absolutePath, published) + } catch { + // Swallowed on purpose: the publication itself succeeded, and a + // follow-up write against the previous observation is the + // pre-existing contract (stale -> re-read). + } + }) +} + +/** + * Reset the per-path tail-promise chains (test hook). + */ +export function resetChain(): void { + pendingChains.clear() +} diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 42be14463c..cb6575a316 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -861,7 +861,7 @@ }, "core/tools/__tests__/readFileTool.spec.ts": { "@typescript-eslint/no-explicit-any": { - "count": 96 + "count": 95 } }, "core/tools/__tests__/runSlashCommandTool.spec.ts": { diff --git a/src/i18n/locales/ca/common.json b/src/i18n/locales/ca/common.json index d414d697f2..d366146475 100644 --- a/src/i18n/locales/ca/common.json +++ b/src/i18n/locales/ca/common.json @@ -51,6 +51,7 @@ "cannot_access_path": "No es pot accedir a la ruta {{path}}: {{error}}", "path_outside_workspace": "La ruta és fora de l'espai de treball", "settings_import_failed": "Ha fallat la importació de la configuració: {{error}}.", + "settings_export_failed": "Ha fallat l'exportació de la configuració: {{error}}.", "mistake_limit_guidance": "Això pot indicar un error en el procés de pensament del model o la incapacitat d'utilitzar una eina correctament, que es pot mitigar amb orientació de l'usuari (p. ex. \"Prova de dividir la tasca en passos més petits\").", "violated_organization_allowlist": "Ha fallat l'execució de la tasca: el perfil actual no és compatible amb la configuració de la teva organització", "condense_failed": "Ha fallat la condensació del context", diff --git a/src/i18n/locales/de/common.json b/src/i18n/locales/de/common.json index 8361221019..96aef6c7c7 100644 --- a/src/i18n/locales/de/common.json +++ b/src/i18n/locales/de/common.json @@ -47,6 +47,7 @@ "cannot_access_path": "Zugriff auf Pfad {{path}} nicht möglich: {{error}}", "path_outside_workspace": "Pfad liegt außerhalb des Arbeitsbereichs", "settings_import_failed": "Fehler beim Importieren der Einstellungen: {{error}}.", + "settings_export_failed": "Fehler beim Exportieren der Einstellungen: {{error}}.", "mistake_limit_guidance": "Dies kann auf einen Fehler im Denkprozess des Modells oder die Unfähigkeit hinweisen, ein Tool richtig zu verwenden, was durch Benutzerführung behoben werden kann (z.B. \"Versuche, die Aufgabe in kleinere Schritte zu unterteilen\").", "violated_organization_allowlist": "Aufgabe konnte nicht ausgeführt werden: Das aktuelle Profil ist nicht kompatibel mit den Einstellungen deiner Organisation", "condense_failed": "Fehler beim Verdichten des Kontexts", diff --git a/src/i18n/locales/en/common.json b/src/i18n/locales/en/common.json index 21cb77dd5f..b92a8c35d5 100644 --- a/src/i18n/locales/en/common.json +++ b/src/i18n/locales/en/common.json @@ -47,6 +47,7 @@ "cannot_access_path": "Cannot access path {{path}}: {{error}}", "path_outside_workspace": "Path is outside the workspace", "settings_import_failed": "Settings import failed: {{error}}.", + "settings_export_failed": "Settings export failed: {{error}}.", "mistake_limit_guidance": "This may indicate a failure in the model's thought process or inability to use a tool properly, which can be mitigated with some user guidance (e.g. \"Try breaking down the task into smaller steps\").", "violated_organization_allowlist": "Failed to run task: the current profile isn't compatible with your organization settings", "condense_failed": "Failed to condense context", diff --git a/src/i18n/locales/es/common.json b/src/i18n/locales/es/common.json index 5b959f6e2f..efc64f2db4 100644 --- a/src/i18n/locales/es/common.json +++ b/src/i18n/locales/es/common.json @@ -47,6 +47,7 @@ "cannot_access_path": "No se puede acceder a la ruta {{path}}: {{error}}", "path_outside_workspace": "La ruta está fuera del espacio de trabajo", "settings_import_failed": "Error al importar la configuración: {{error}}.", + "settings_export_failed": "Error al exportar la configuración: {{error}}.", "mistake_limit_guidance": "Esto puede indicar un fallo en el proceso de pensamiento del modelo o la incapacidad de usar una herramienta correctamente, lo cual puede mitigarse con orientación del usuario (ej. \"Intenta dividir la tarea en pasos más pequeños\").", "violated_organization_allowlist": "Error al ejecutar la tarea: el perfil actual no es compatible con la configuración de tu organización", "condense_failed": "Error al condensar el contexto", diff --git a/src/i18n/locales/fr/common.json b/src/i18n/locales/fr/common.json index 03e911de49..07a9971f5e 100644 --- a/src/i18n/locales/fr/common.json +++ b/src/i18n/locales/fr/common.json @@ -47,6 +47,7 @@ "cannot_access_path": "Impossible d'accéder au chemin {{path}} : {{error}}", "path_outside_workspace": "Le chemin est à l'extérieur de l'espace de travail", "settings_import_failed": "Échec de l'importation des paramètres : {{error}}", + "settings_export_failed": "Échec de l'exportation des paramètres : {{error}}", "mistake_limit_guidance": "Cela peut indiquer un échec dans le processus de réflexion du modèle ou une incapacité à utiliser un outil correctement, ce qui peut être atténué avec des conseils de l'utilisateur (par ex. \"Essaie de diviser la tâche en étapes plus petites\").", "violated_organization_allowlist": "Échec de l'exécution de la tâche : le profil actuel n'est pas compatible avec les paramètres de votre organisation", "condense_failed": "Échec de la condensation du contexte", diff --git a/src/i18n/locales/hi/common.json b/src/i18n/locales/hi/common.json index 36479f1060..5df87effed 100644 --- a/src/i18n/locales/hi/common.json +++ b/src/i18n/locales/hi/common.json @@ -47,6 +47,7 @@ "cannot_access_path": "पाथ {{path}} तक पहुंच नहीं पा रहे हैं: {{error}}", "path_outside_workspace": "पथ वर्कस्पेस से बाहर है", "settings_import_failed": "सेटिंग्स इम्पोर्ट करने में विफल: {{error}}।", + "settings_export_failed": "सेटिंग्स एक्सपोर्ट करने में विफल: {{error}}।", "mistake_limit_guidance": "यह मॉडल की सोच प्रक्रिया में विफलता या किसी टूल का सही उपयोग न कर पाने का संकेत हो सकता है, जिसे उपयोगकर्ता के मार्गदर्शन से ठीक किया जा सकता है (जैसे \"कार्य को छोटे चरणों में बांटने की कोशिश करें\")।", "violated_organization_allowlist": "कार्य चलाने में विफल: वर्तमान प्रोफ़ाइल आपके संगठन की सेटिंग्स के साथ संगत नहीं है", "condense_failed": "संदर्भ को संक्षिप्त करने में विफल", diff --git a/src/i18n/locales/id/common.json b/src/i18n/locales/id/common.json index d047cf3cbf..f5ee5d9128 100644 --- a/src/i18n/locales/id/common.json +++ b/src/i18n/locales/id/common.json @@ -47,6 +47,7 @@ "cannot_access_path": "Tidak dapat mengakses path {{path}}: {{error}}", "path_outside_workspace": "Path berada di luar workspace", "settings_import_failed": "Impor pengaturan gagal: {{error}}.", + "settings_export_failed": "Ekspor pengaturan gagal: {{error}}.", "mistake_limit_guidance": "Ini mungkin menunjukkan kegagalan dalam proses pemikiran model atau ketidakmampuan untuk menggunakan tool dengan benar, yang dapat diatasi dengan beberapa panduan pengguna (misalnya \"Coba bagi tugas menjadi langkah-langkah yang lebih kecil\").", "violated_organization_allowlist": "Gagal menjalankan tugas: profil saat ini tidak kompatibel dengan pengaturan organisasi kamu", "condense_failed": "Gagal mengompres konteks", diff --git a/src/i18n/locales/it/common.json b/src/i18n/locales/it/common.json index ecdf443a3d..a40eb356c8 100644 --- a/src/i18n/locales/it/common.json +++ b/src/i18n/locales/it/common.json @@ -47,6 +47,7 @@ "cannot_access_path": "Impossibile accedere al percorso {{path}}: {{error}}", "path_outside_workspace": "Il percorso è fuori dall'area di lavoro", "settings_import_failed": "Importazione delle impostazioni fallita: {{error}}.", + "settings_export_failed": "Esportazione delle impostazioni fallita: {{error}}.", "mistake_limit_guidance": "Questo può indicare un fallimento nel processo di pensiero del modello o l'incapacità di utilizzare correttamente uno strumento, che può essere mitigato con la guida dell'utente (ad es. \"Prova a suddividere l'attività in passaggi più piccoli\").", "violated_organization_allowlist": "Impossibile eseguire l'attività: il profilo corrente non è compatibile con le impostazioni della tua organizzazione", "condense_failed": "Impossibile condensare il contesto", diff --git a/src/i18n/locales/ja/common.json b/src/i18n/locales/ja/common.json index 2220a608d6..029fdc132f 100644 --- a/src/i18n/locales/ja/common.json +++ b/src/i18n/locales/ja/common.json @@ -47,6 +47,7 @@ "cannot_access_path": "パス {{path}} にアクセスできません：{{error}}", "path_outside_workspace": "パスはワークスペースの外にあります", "settings_import_failed": "設定のインポートに失敗しました：{{error}}", + "settings_export_failed": "設定のエクスポートに失敗しました：{{error}}", "mistake_limit_guidance": "これは、モデルの思考プロセスの失敗やツールを適切に使用できないことを示している可能性があり、ユーザーのガイダンスによって軽減できます（例：「タスクをより小さなステップに分割してみてください」）。", "violated_organization_allowlist": "タスクの実行に失敗しました: 現在のプロファイルは組織の設定と互換性がありません", "condense_failed": "コンテキストの圧縮に失敗しました", diff --git a/src/i18n/locales/ko/common.json b/src/i18n/locales/ko/common.json index b462dc8056..8fa3f75c64 100644 --- a/src/i18n/locales/ko/common.json +++ b/src/i18n/locales/ko/common.json @@ -47,6 +47,7 @@ "cannot_access_path": "경로 {{path}}에 접근할 수 없습니다: {{error}}", "path_outside_workspace": "경로가 작업 영역 밖에 있습니다", "settings_import_failed": "설정 가져오기 실패: {{error}}.", + "settings_export_failed": "설정 내보내기 실패: {{error}}.", "mistake_limit_guidance": "이는 모델의 사고 과정 실패나 도구를 제대로 사용하지 못하는 것을 나타낼 수 있으며, 사용자 가이드를 통해 완화할 수 있습니다 (예: \"작업을 더 작은 단계로 나누어 시도해보세요\").", "violated_organization_allowlist": "작업 실행 실패: 현재 프로필이 조직 설정과 호환되지 않습니다", "condense_failed": "컨텍스트 압축에 실패했습니다", diff --git a/src/i18n/locales/nl/common.json b/src/i18n/locales/nl/common.json index 01523541d9..da57a0a2db 100644 --- a/src/i18n/locales/nl/common.json +++ b/src/i18n/locales/nl/common.json @@ -47,6 +47,7 @@ "cannot_access_path": "Kan pad {{path}} niet openen: {{error}}", "path_outside_workspace": "Pad is buiten de werkomgeving", "settings_import_failed": "Importeren van instellingen mislukt: {{error}}.", + "settings_export_failed": "Exporteren van instellingen mislukt: {{error}}.", "mistake_limit_guidance": "Dit kan duiden op een fout in het denkproces van het model of het onvermogen om een tool correct te gebruiken, wat kan worden verminderd met gebruikersbegeleiding (bijv. \"Probeer de taak op te delen in kleinere stappen\").", "violated_organization_allowlist": "Taak uitvoeren mislukt: het huidige profiel is niet compatibel met de instellingen van uw organisatie", "condense_failed": "Comprimeren van context mislukt", diff --git a/src/i18n/locales/pl/common.json b/src/i18n/locales/pl/common.json index d2b6412257..7d867a3ca4 100644 --- a/src/i18n/locales/pl/common.json +++ b/src/i18n/locales/pl/common.json @@ -47,6 +47,7 @@ "cannot_access_path": "Nie można uzyskać dostępu do ścieżki {{path}}: {{error}}", "path_outside_workspace": "Ścieżka znajduje się poza obszarem roboczym", "settings_import_failed": "Nie udało się zaimportować ustawień: {{error}}.", + "settings_export_failed": "Nie udało się wyeksportować ustawień: {{error}}.", "mistake_limit_guidance": "To może wskazywać na błąd w procesie myślowym modelu lub niezdolność do prawidłowego użycia narzędzia, co można złagodzić poprzez wskazówki użytkownika (np. \"Spróbuj podzielić zadanie na mniejsze kroki\").", "violated_organization_allowlist": "Nie udało się uruchomić zadania: bieżący profil nie jest kompatybilny z ustawieniami Twojej organizacji", "condense_failed": "Nie udało się skondensować kontekstu", diff --git a/src/i18n/locales/pt-BR/common.json b/src/i18n/locales/pt-BR/common.json index 02e65d5ab9..fa089fa421 100644 --- a/src/i18n/locales/pt-BR/common.json +++ b/src/i18n/locales/pt-BR/common.json @@ -51,6 +51,7 @@ "cannot_access_path": "Não é possível acessar o caminho {{path}}: {{error}}", "path_outside_workspace": "O caminho está fora do espaço de trabalho", "settings_import_failed": "Falha ao importar configurações: {{error}}", + "settings_export_failed": "Falha ao exportar configurações: {{error}}", "mistake_limit_guidance": "Isso pode indicar uma falha no processo de pensamento do modelo ou incapacidade de usar uma ferramenta adequadamente, o que pode ser mitigado com orientação do usuário (ex. \"Tente dividir a tarefa em etapas menores\").", "violated_organization_allowlist": "Falha ao executar a tarefa: o perfil atual não é compatível com as configurações da sua organização", "condense_failed": "Falha ao condensar o contexto", diff --git a/src/i18n/locales/ru/common.json b/src/i18n/locales/ru/common.json index 64e60ab2ff..4fa090fcc1 100644 --- a/src/i18n/locales/ru/common.json +++ b/src/i18n/locales/ru/common.json @@ -47,6 +47,7 @@ "cannot_access_path": "Невозможно получить доступ к пути {{path}}: {{error}}", "path_outside_workspace": "Путь находится вне рабочего пространства", "settings_import_failed": "Не удалось импортировать настройки: {{error}}.", + "settings_export_failed": "Не удалось экспортировать настройки: {{error}}.", "mistake_limit_guidance": "Это может указывать на сбой в процессе мышления модели или неспособность правильно использовать инструмент, что можно смягчить с помощью руководства пользователя (например, \"Попробуйте разбить задачу на более мелкие шаги\").", "violated_organization_allowlist": "Не удалось выполнить задачу: текущий профиль несовместим с настройками вашей организации", "condense_failed": "Не удалось сжать контекст", diff --git a/src/i18n/locales/tr/common.json b/src/i18n/locales/tr/common.json index 992e03a419..e3eada9092 100644 --- a/src/i18n/locales/tr/common.json +++ b/src/i18n/locales/tr/common.json @@ -47,6 +47,7 @@ "cannot_access_path": "{{path}} yoluna erişilemiyor: {{error}}", "path_outside_workspace": "Yol çalışma alanının dışında", "settings_import_failed": "Ayarlar içe aktarılamadı: {{error}}.", + "settings_export_failed": "Ayarlar dışa aktarılamadı: {{error}}.", "mistake_limit_guidance": "Bu, modelin düşünce sürecindeki bir başarısızlığı veya bir aracı düzgün kullanamama durumunu gösterebilir, bu da kullanıcı rehberliği ile hafifletilebilir (örn. \"Görevi daha küçük adımlara bölmeyi deneyin\").", "violated_organization_allowlist": "Görev yürütülemedi: Geçerli profil kuruluşunuzun ayarlarıyla uyumlu değil", "condense_failed": "Bağlam sıkıştırılamadı", diff --git a/src/i18n/locales/vi/common.json b/src/i18n/locales/vi/common.json index 94c61bdaae..c7a0082112 100644 --- a/src/i18n/locales/vi/common.json +++ b/src/i18n/locales/vi/common.json @@ -47,6 +47,7 @@ "cannot_access_path": "Không thể truy cập đường dẫn {{path}}: {{error}}", "path_outside_workspace": "Đường dẫn nằm ngoài không gian làm việc", "settings_import_failed": "Nhập cài đặt thất bại: {{error}}.", + "settings_export_failed": "Xuất cài đặt thất bại: {{error}}.", "mistake_limit_guidance": "Điều này có thể cho thấy sự thất bại trong quá trình suy nghĩ của mô hình hoặc không thể sử dụng công cụ đúng cách, có thể được giảm thiểu bằng hướng dẫn của người dùng (ví dụ: \"Hãy thử chia nhỏ nhiệm vụ thành các bước nhỏ hơn\").", "violated_organization_allowlist": "Không thể chạy tác vụ: hồ sơ hiện tại không tương thích với cài đặt của tổ chức của bạn", "condense_failed": "Không thể nén ngữ cảnh", diff --git a/src/i18n/locales/zh-CN/common.json b/src/i18n/locales/zh-CN/common.json index 2c39a92571..beaca8ed9e 100644 --- a/src/i18n/locales/zh-CN/common.json +++ b/src/i18n/locales/zh-CN/common.json @@ -52,6 +52,7 @@ "cannot_access_path": "无法访问路径 {{path}}：{{error}}", "path_outside_workspace": "路径位于工作区之外", "settings_import_failed": "设置导入失败：{{error}}。", + "settings_export_failed": "设置导出失败：{{error}}。", "mistake_limit_guidance": "这可能表明模型思维过程失败或无法正确使用工具，可通过用户指导来缓解（例如\"尝试将任务分解为更小的步骤\"）。", "violated_organization_allowlist": "执行任务失败：当前配置文件与您的组织设置不兼容", "condense_failed": "压缩上下文失败", diff --git a/src/i18n/locales/zh-TW/common.json b/src/i18n/locales/zh-TW/common.json index a96493284b..86e31ab36d 100644 --- a/src/i18n/locales/zh-TW/common.json +++ b/src/i18n/locales/zh-TW/common.json @@ -47,6 +47,7 @@ "cannot_access_path": "無法存取路徑 {{path}}：{{error}}", "path_outside_workspace": "路徑位於工作區之外", "settings_import_failed": "設定匯入失敗：{{error}}。", + "settings_export_failed": "設定匯出失敗：{{error}}。", "mistake_limit_guidance": "這可能表明模型思維過程失敗或無法正確使用工具，可透過使用者指導來緩解（例如「嘗試將工作分解為更小的步驟」）。", "violated_organization_allowlist": "執行工作失敗：目前設定檔與您的組織設定不相容", "condense_failed": "壓縮上下文失敗", 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..6e8b922bb7 --- /dev/null +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -0,0 +1,1055 @@ +import * as fs from "fs/promises" +import * as fsSync from "fs" +import { execFile } from "child_process" +import type { ChildProcess } from "child_process" +import * as path from "path" + +import { safeWriteText } from "../safeWriteText" + +// Full mock for fs/promises — all methods are vi.fn() stubs +vi.mock("fs/promises", () => ({ + mkdir: vi.fn(), + access: vi.fn(), + rename: vi.fn(), + unlink: vi.fn(), + realpath: vi.fn(), +})) + +// Full mock for fs — all sync methods are vi.fn() stubs. Stats is a bare +// class stub so tests can build minimal Stats stand-ins via its prototype. +vi.mock("fs", () => ({ + openSync: vi.fn(), + writeSync: vi.fn(), + closeSync: vi.fn(), + mkdirSync: vi.fn(), + fsyncSync: vi.fn(), + chmodSync: vi.fn(), + fchmodSync: vi.fn(), + rmdirSync: vi.fn(), + statSync: vi.fn(), + Stats: class Stats {}, +})) + +// Mock child_process.execFile (callback-based — must invoke callback to resolve) +vi.mock("child_process", () => ({ + execFile: vi.fn((cmd, args, opts, cb) => { + if (typeof cb === "function") cb(null) + }), +})) + +// Minimal stand-in for the ChildProcess that callback-form execFile returns. +const fakeChild = { kill: () => true } as unknown as ChildProcess + +// Helper that mirrors safeWriteText's path resolution exactly +function _resolvedTarget(filePath: string): string { + return path.resolve(filePath) +} +function _dirPath(filePath: string): string { + return path.dirname(_resolvedTarget(filePath)) +} +function _stagingDir(dir: string): string { + return path.join(dir, ".file-safety-staging") +} + +// Minimal Stats stand-in: the SUT only reads `.mode` from it. +function _stats(mode: number): fsSync.Stats { + const s = Object.create(fsSync.Stats.prototype) as fsSync.Stats + Object.assign(s, { mode }) + return s +} + +// ── Test 1: staging file created then cleaned after success ──────────────── + +describe("safeWriteText", () => { + beforeEach(() => { + vi.resetAllMocks() + // After resetAllMocks, vi.fn() returns undefined — restore promise defaults. + vi.mocked(fs.mkdir).mockResolvedValue(undefined) + vi.mocked(fs.access).mockResolvedValue(undefined) + vi.mocked(fs.rename).mockResolvedValue(undefined) + vi.mocked(fs.unlink).mockResolvedValue(undefined) + // Existing-target default: a regular 0o644 file. + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o644)) + // Default sync-write behaviour: report that all requested bytes were + // written. The Buffer overload passes (fd, buffer, offset, length), + // so the fourth argument is the requested length. + vi.mocked(fsSync.writeSync).mockImplementation((...args: unknown[]) => + typeof args[3] === "number" ? args[3] : 0, + ) + }) + + describe("rollback failure", () => { + it("renders non-Error failures through the rollback message", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.closeSync).mockReturnValue(undefined) + // Non-Error throwables are legal in JavaScript: the message must still render both + // the primary and rollback failures instead of failing to build the text. + vi.mocked(fs.rename) + .mockResolvedValueOnce(undefined) + .mockRejectedValueOnce("commit failed") + .mockRejectedValueOnce("rollback failed") + + const failure = await safeWriteText(targetPath, "hello", { backup: true, platform: "linux" }).then( + () => undefined, + (error: unknown) => error as Error & { name: string; cause?: unknown }, + ) + + expect(failure?.name).toBe("RollbackFailedError") + expect(failure?.message).toContain("commit failed") + expect(failure?.message).toContain("rollback failed") + expect(failure?.cause).toBe("rollback failed") + }) + + it("throws RollbackFailedError with the backup path when the rollback rename also fails", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.closeSync).mockReturnValue(undefined) + // backup rename succeeds, the commit rename fails, and the rollback rename fails. + vi.mocked(fs.rename) + .mockResolvedValueOnce(undefined) + .mockRejectedValueOnce(new Error("commit failed")) + .mockRejectedValueOnce(new Error("rollback failed")) + + const failure = await safeWriteText(targetPath, "hello", { backup: true, platform: "linux" }).then( + () => undefined, + (error: unknown) => + error as Error & { + name: string + filePath: string + backupPath: string + originalError?: Error + cause?: Error + }, + ) + + // The caller must learn both the primary failure and where the previous content + // survived: the backup path is the only way to recover it. + expect(failure?.name).toBe("RollbackFailedError") + expect(failure?.filePath).toBe(targetPath) + expect(failure?.backupPath).toContain(".safeWriteText.bak_") + expect(failure?.originalError?.message).toBe("commit failed") + expect(failure?.cause?.message).toBe("rollback failed") + expect(failure?.message).toContain("commit failed") + expect(failure?.message).toContain("rollback failed") + // The failure path still cleans up: the staged temp is unlinked and the staging + // directory is released, so a double-failure does not leave write artifacts behind. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + expect(fsSync.rmdirSync).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + expect(failure?.message).toContain(failure?.backupPath) + }) + }) + + describe("staging and cleanup", () => { + it("creates a temp file in the staging dir, fsyncs it, renames to target, and cleans up on success", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) // fd=1 + vi.mocked(fsSync.closeSync).mockReturnValue(undefined) + + await safeWriteText(targetPath, "hello world", { platform: "linux" }) + + // staging dir was created with private permissions — use + // stringContaining to handle Windows path resolution + expect(fsSync.mkdirSync).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), { + recursive: true, + mode: 0o700, + }) + // a pre-existing staging dir is repaired to private permissions too + expect(fsSync.chmodSync).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), 0o700) + + // temp file was opened for writing with the existing target's mode + // (default 0o644 from the statSync default mock) + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), "w", 0o644) + + // content was written as a buffer (partial-write loop, full write) + expect(fsSync.writeSync).toHaveBeenCalledWith(1, Buffer.from("hello world", "utf8"), 0, 11) + + // fsync (sync form) was called on the fd + expect(fsSync.fsyncSync).toHaveBeenCalledWith(1) + + // file was closed + expect(fsSync.closeSync).toHaveBeenCalledWith(1) + + // atomic rename happened — realpath mock returns targetPath, so that's the dest + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // Ordering contract: the content is fsynced, the descriptor closed, and only then + // the temp is renamed over the target, so a crash cannot publish unflushed bytes. + const fsyncOrder = vi.mocked(fsSync.fsyncSync).mock.invocationCallOrder[0] + const closeOrder = vi.mocked(fsSync.closeSync).mock.invocationCallOrder[0] + const renameOrder = vi.mocked(fs.rename).mock.invocationCallOrder[0] + expect(fsyncOrder).toBeLessThan(closeOrder) + expect(closeOrder).toBeLessThan(renameOrder) + + // no unlink of temp (it's now the committed file; DACL skipped via platform:linux) + expect(fs.unlink).not.toHaveBeenCalled() + }) + }) + + // The staging directory's name is the chain's single shape for this directory (tracking issue 1991 + // section 6): it is generated per write, the way the temp file inside it is generated, so no path the + // user's workspace already controls can become the directory this primitive mkdirs, chmods and rmdirs. + describe("per-write staging directory name", () => { + // The generated shape: the fixed prefix plus the same Date.now + random suffix the temp file gets, + // end-anchored so a suffix-less or hand-named path fails. + const _isPerWriteStagingName = (name: string): boolean => /^\.file-safety-staging_\d+_[a-z0-9]+$/.test(name) + + it("names the staging directory of the generated shape", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "hello", { targetPathIsResolved: true, platform: "linux" }) + + const staged = vi.mocked(fsSync.mkdirSync).mock.calls.map((call) => String(call[0])) + expect(staged).toHaveLength(1) + expect(_isPerWriteStagingName(path.basename(staged[0]))).toBe(true) + // Staged beside the directory the publish targets, not somewhere else. + expect(path.resolve(path.dirname(staged[0]))).toBe(path.resolve(_dirPath(targetPath))) + }) + + it("gives each self-staged write its own staging directory", async () => { + const dir = "/tmp/test-dir" + const targetA = path.join(dir, "target-a.txt") + const targetB = path.join(dir, "target-b.txt") + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await Promise.all([ + safeWriteText(targetA, "A", { targetPathIsResolved: true, platform: "linux" }), + safeWriteText(targetB, "B", { targetPathIsResolved: true, platform: "linux" }), + ]) + + // Two writes into one directory must not stage into one shared directory: with a shared name + // each write's cleanup rmdir runs while the other write still has a temp open in it, so a + // release either fails or removes a directory another write is about to publish from. + const staged = vi.mocked(fsSync.mkdirSync).mock.calls.map((call) => String(call[0])) + expect(staged).toHaveLength(2) + expect(new Set(staged).size).toBe(2) + for (const stagedDir of staged) { + expect(_isPerWriteStagingName(path.basename(stagedDir))).toBe(true) + } + }) + + it("leaves a path the workspace controls at the fixed shared name untouched", async () => { + const targetPath = "/tmp/test-dir/target.txt" + // The name the shared-name shape mkdir-ed, chmod-ed and rmdir-ed. A workspace can commit a + // symlink or a directory at exactly this path, and mkdirSync with recursive:true accepts the + // existing entry without creating anything, so the chmod would repair a directory outside the + // target directory and the staged file would land in it. + const sharedName = path.basename(_stagingDir(_dirPath(targetPath))) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "hello", { targetPathIsResolved: true, platform: "linux" }) + + const mkdirNames = vi.mocked(fsSync.mkdirSync).mock.calls.map((call) => path.basename(String(call[0]))) + expect(mkdirNames).not.toContain(sharedName) + const chmodNames = vi + .mocked(fsSync.chmodSync) + .mock.calls.filter((call) => call[1] === 0o700) + .map((call) => path.basename(String(call[0]))) + expect(chmodNames).not.toContain(sharedName) + const rmdirNames = vi.mocked(fsSync.rmdirSync).mock.calls.map((call) => path.basename(String(call[0]))) + expect(rmdirNames).not.toContain(sharedName) + }) + }) + + // ── Test 2: fsync ordering ─────────────────────────────────────────────── + + describe("fsync ordering", () => { + it("calls fsync on the fd before close, and rename after close", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // Verify call order: openSync(temp) → writeSync → fsyncSync(temp) + // → closeSync(temp) → rename. On POSIX the parent directory is then + // opened and fsynced after the commit rename, so openSync/fsyncSync/ + // closeSync each have a second (directory) call. + expect(vi.mocked(fsSync.openSync).mock.calls.length).toBe(2) + expect(vi.mocked(fsSync.writeSync).mock.calls.length).toBe(1) + expect(vi.mocked(fsSync.fsyncSync).mock.calls.length).toBe(2) + expect(vi.mocked(fsSync.closeSync).mock.calls.length).toBe(2) + + // the temp file was fully closed before the commit rename + expect(vi.mocked(fsSync.closeSync).mock.calls[0][0]).toBe(1) + expect(fs.rename).toHaveBeenCalled() + }) + }) + + // ── Test 3: simulated failure between write and rename leaves target intact ── + + describe("crash/torn-write safety", () => { + it("simulated failure between fsync and rename leaves the target byte-identical and no temp left behind", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fs.rename).mockRejectedValue(new Error("ENOSPC")) + + await expect(safeWriteText(targetPath, "new data", { platform: "linux" })).rejects.toThrow("ENOSPC") + + // rename was attempted (the failure point) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // temp file was cleaned up on failure + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + + // backup was NOT created (backup:false by default), so target is untouched + // The only rename call was temp→target, not a rollback rename + expect(fs.rename).toHaveBeenCalledTimes(1) + }) + + it("a staging-file fsync failure aborts the publish and leaves the target untouched", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The durability step itself fails: the staged bytes never reached the disk, so + // publishing them would put content at the target that a crash can lose. + vi.mocked(fsSync.fsyncSync).mockImplementationOnce(() => { + throw new Error("EIO") + }) + + await expect(safeWriteText(targetPath, "new data", { platform: "linux" })).rejects.toThrow("EIO") + + // No commit: the rename that publishes the staged file never ran, so the target + // still holds whatever it held before the call. + expect(fs.rename).not.toHaveBeenCalled() + + // The fd is closed and the staged temp released. + expect(fsSync.closeSync).toHaveBeenCalledWith(1) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("a post-commit backup cleanup failure is non-fatal: the target stays committed and no temp is left behind", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The post-commit backup unlink (SUT step 6) fails — the write must + // still succeed; an orphaned backup is the documented acceptable + // outcome, so the failure is swallowed instead of rolling back. + vi.mocked(fs.unlink).mockRejectedValueOnce(new Error("EPERM")) + + await safeWriteText(targetPath, "data", { backup: true, platform: "linux" }) + + // the commit rename (temp -> target) still happened + expect(fs.rename).toHaveBeenNthCalledWith(2, expect.stringContaining("safeWriteText_"), targetPath) + + // the failing cleanup was the post-commit backup unlink + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + + // no rollback rename: the committed target is not restored from the backup + expect(fs.rename).toHaveBeenCalledTimes(2) + + // the staging temp was already committed by the rename; nothing + // temp-shaped is unlinked afterwards + expect(fs.unlink).not.toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + }) + + // ── Test 4: backup:true keeps old safeWriteJson semantics incl. rollback ── + + describe("backup:true", () => { + it("renames target -> backup before commit, deletes backup on success", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "new data", { backup: true }) + + // target was accessed (exists check) + expect(fs.access).toHaveBeenCalledWith(targetPath) + + // first rename: target -> backup + expect(fs.rename).toHaveBeenNthCalledWith(1, targetPath, expect.stringContaining("safeWriteText.bak_")) + + // second rename: temp -> target (realpath mock returns targetPath) + expect(fs.rename).toHaveBeenNthCalledWith(2, expect.stringContaining("safeWriteText_"), targetPath) + + // backup was deleted on success + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + }) + + it("rollback: on failure after rename target->backup, restores backup to target", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // first rename (target->backup) succeeds, second fails + let callCount = 0 + vi.mocked(fs.rename).mockImplementation(async () => { + callCount++ + if (callCount === 1) return // target -> backup + throw new Error("ENOSPC") // temp -> target fails + }) + + await expect(safeWriteText(targetPath, "new data", { backup: true })).rejects.toThrow("ENOSPC") + + // rollback rename is the 3rd call (after target->backup and temp->target failure) + expect(fs.rename).toHaveBeenNthCalledWith(3, expect.stringContaining("safeWriteText.bak_"), targetPath) + + // temp was cleaned up on failure + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("backup:true when target does not exist: no backup created, just commit", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // fs.access resolves for dirPath check, but rejects for target check (backup path) + vi.mocked(fs.access).mockImplementation(async (p) => { + if (typeof p === "string" && p.endsWith("target.txt")) throw { code: "ENOENT" } + }) + + await safeWriteText(targetPath, "new data", { backup: true, platform: "linux" }) + + // no backup rename (target didn't exist) + expect(fs.access).toHaveBeenCalledWith(targetPath) + + // only one rename: temp -> target + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // no unlink (no backup to delete; DACL skipped via platform:linux) + expect(fs.unlink).not.toHaveBeenCalled() + }) + }) + + // ── Test 5: win32 DACL path ────────────────────────────────────────────── + + describe("win32 DACL", () => { + it.skipIf(process.platform !== "win32")( + "copies target DACL onto staging file via icacls before rename on Windows", + async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // icacls dump + restore were called (execFile is callback-based mock) + expect(execFile).toHaveBeenCalledTimes(2) + }, + ) + + it("non-win32: DACL path is unreachable when platform is not win32", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // icacls was NOT called on non-win32 + expect(execFile).not.toHaveBeenCalled() + }) + + it("win32 DACL failure falls back to plain rename (never fails the write)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // icacls dump fails — the callback-based mock must invoke cb with an error. + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + if (typeof cb === "function") cb(new Error("icacls error"), "", "") + return fakeChild + }) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // write succeeded despite icacls failure (fallback to plain rename): + // pin the commit rename onto the target - a rename to any other path + // would silently publish elsewhere - and pin that a failed save skips + // the restore step entirely. + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + expect(execFile).toHaveBeenCalledTimes(1) + }) + + it("win32 DACL: a partial dump left by a failed save is still unlinked", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // icacls can write a partial dump and still exit non-zero. The dump path must stay + // tracked so the cleanup removes it; clearing it on the failure would leave the + // dump sitting next to the target with nothing left to remove it. + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + if (typeof cb === "function") cb(new Error("icacls error"), "", "") + return fakeChild + }) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // No restore attempt (the save failed), but the dump is cleaned up. + expect(execFile).toHaveBeenCalledTimes(1) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".acl.tmp")) + }) + + it("win32 DACL save args are [targetPath, /save, dumpPath, /T] before backup rename", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { backup: true, platform: "win32" }) + + // icacls was called twice (save + restore) + expect(execFile).toHaveBeenCalledTimes(2) + + // First call: save DACL from target before backup rename + const firstCall = vi.mocked(execFile).mock.calls[0] + expect(firstCall[0]).toBe("icacls") + expect(firstCall[1]).toEqual([targetPath, "/save", expect.stringContaining(".acl.tmp"), "/T"]) + + // Second call: restore DACL onto directory after commit rename + const secondCall = vi.mocked(execFile).mock.calls[1] + expect(secondCall[0]).toBe("icacls") + expect(secondCall[1]).toEqual([ + expect.stringContaining("/tmp/test-dir"), + "/restore", + expect.stringContaining(".acl.tmp"), + ]) + + // dump file was unlinked after restore + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".acl.tmp")) + }) + + it("win32 DACL: dump is unlinked even when restore fails", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + // icacls save succeeds, restore fails + let callCount = 0 + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + callCount++ + if (typeof cb === "function") { + cb(callCount === 1 ? null : new Error("icacls restore error"), "", "") + } + return fakeChild + }) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // write succeeded despite restore failure (best-effort): the commit rename ran and + // the saved DACL dump was still unlinked in the finally block. + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + expect(execFile).toHaveBeenCalledTimes(2) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".acl.tmp")) + }) + + it("win32 DACL: when target does not exist, no save/restore/dump", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + // fs.access rejects for targetPath (ENOENT), but resolves for dirPath + vi.mocked(fs.access).mockImplementation(async (p) => { + if (typeof p === "string" && p.endsWith("target.txt")) throw { code: "ENOENT" } + return undefined + }) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // icacls was NOT called (target absent → skip DACL entirely) + expect(execFile).not.toHaveBeenCalled() + + // no dump file created or unlinked + expect(fs.unlink).not.toHaveBeenCalled() + }) + }) + + describe("parent directory creation", () => { + it("creates a missing parent directory before staging", async () => { + const targetPath = "/tmp/test-dir/nested/deeper/target.txt" + const dirPath = "/tmp/test-dir/nested/deeper" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data") + + expect(fs.mkdir).toHaveBeenCalledWith(dirPath, { recursive: true }) + expect(fs.access).toHaveBeenCalledWith(dirPath) + expect(vi.mocked(fs.mkdir).mock.invocationCallOrder[0]).toBeLessThan( + vi.mocked(fsSync.openSync).mock.invocationCallOrder[0], + ) + }) + + it("surfaces a parent directory creation failure before any staging", async () => { + const targetPath = "/tmp/test-dir/nested/deeper/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fs.mkdir).mockRejectedValue(Object.assign(new Error("EACCES mkdir"), { code: "EACCES" })) + + await expect(safeWriteText(targetPath, "data")).rejects.toThrow("EACCES mkdir") + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("surfaces a parent directory access failure before any staging", async () => { + const targetPath = "/tmp/test-dir/nested/deeper/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fs.access).mockRejectedValue(Object.assign(new Error("EACCES access"), { code: "EACCES" })) + + await expect(safeWriteText(targetPath, "data")).rejects.toThrow("EACCES access") + expect(fs.rename).not.toHaveBeenCalled() + }) + }) + + // ── Test 6: pre-written temp path (tempPath option) ────────────────────── + + describe("pre-written temp path", () => { + it("uses the provided tempPath, fsyncs it, and renames to target", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + const customTempPath = "/tmp/custom-temp.tmp" + + // platform:linux skips DACL entirely so this test focuses on tempPath only + await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) + + // openSync was called on the custom temp path (r+ mode for fsync) + expect(fsSync.openSync).toHaveBeenCalledWith(customTempPath, "r+") + + // fsync was called + expect(fsSync.fsyncSync).toHaveBeenCalledWith(1) + + // rename happened — realpath mock returns targetPath + expect(fs.rename).toHaveBeenCalledWith(customTempPath, targetPath) + + // no unlink of custom temp (caller's concern; DACL skipped via platform:linux) + expect(fs.unlink).not.toHaveBeenCalled() + + // a caller-supplied tempPath must not create the staging directory + expect(fsSync.mkdirSync).not.toHaveBeenCalled() + }) + + it("applies the existing target's mode to a caller-supplied tempPath before publishing", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o600)) + vi.mocked(fsSync.openSync).mockReturnValue(2) + + const customTempPath = "/tmp/custom-temp.tmp" + + await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) + + // the caller-staged temp is fchmod'd to the restrictive target mode so + // the atomic rename cannot widen a 0o600 target (CWE-732 regression) + expect(fsSync.fchmodSync).toHaveBeenCalledWith(2, 0o600) + expect(fsSync.openSync).toHaveBeenCalledWith(customTempPath, "r+") + expect(fs.rename).toHaveBeenCalledWith(customTempPath, targetPath) + }) + + it("keeps the temp's default mode when the target does not exist yet (ENOENT)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + vi.mocked(fsSync.statSync).mockImplementation(() => { + throw enoent + }) + vi.mocked(fsSync.openSync).mockReturnValue(2) + + const customTempPath = "/tmp/custom-temp.tmp" + + await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) + + // no existing target, so nothing to preserve and no fchmod on the temp + expect(fsSync.fchmodSync).not.toHaveBeenCalled() + expect(fs.rename).toHaveBeenCalledWith(customTempPath, targetPath) + }) + + it("leaves the creation mask in charge when the target does not exist yet", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + vi.mocked(fsSync.statSync).mockImplementation(() => { + throw enoent + }) + vi.mocked(fsSync.openSync).mockReturnValue(2) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // openSync already applied the process umask to the requested mode. Forcing the + // 0o644 default back on with fchmod would undo a restrictive umask (0o600 under + // umask 077) and publish a group/world-readable file for a target that never + // existed, so the mask has to stay in charge. + expect(fsSync.fchmodSync).not.toHaveBeenCalled() + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), "w", 0o644) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("preserves an existing target's mode on the self-staged path", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o600)) + vi.mocked(fsSync.openSync).mockReturnValue(2) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // The preservation rule still applies when a target exists: a 0o600 file must + // not become 0o644 through the atomic rename. + expect(fsSync.fchmodSync).toHaveBeenCalledWith(2, 0o600) + }) + + it("uses the supplied path as-is when the caller already resolved the publish target", async () => { + const targetPath = "/tmp/test-dir/target.txt" + // A second resolution would follow a link installed after the caller's own + // re-check, so with the flag set realpath must not run at all. + vi.mocked(fs.realpath).mockResolvedValue("/elsewhere/referent.txt") + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o644)) + vi.mocked(fsSync.openSync).mockReturnValue(2) + + await safeWriteText(targetPath, "data", { platform: "linux", targetPathIsResolved: true }) + + expect(fs.realpath).not.toHaveBeenCalled() + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), path.resolve(targetPath)) + }) + + it("opens the temp before applying a read-only target's mode (0o444 does not block the open)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o444)) + vi.mocked(fsSync.openSync).mockReturnValue(3) + + const customTempPath = "/tmp/custom-temp.tmp" + + await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) + + // a 0o444 target must not make openSync(tempPath, "r+") fail: the mode + // is applied with fchmodSync on the already-open fd, after the open + expect(fsSync.openSync).toHaveBeenCalledWith(customTempPath, "r+") + expect(fsSync.fchmodSync).toHaveBeenCalledWith(3, 0o444) + const openIdx = vi.mocked(fsSync.openSync).mock.invocationCallOrder[0] + const fchmodIdx = vi.mocked(fsSync.fchmodSync).mock.invocationCallOrder[0] + expect(openIdx).toBeLessThan(fchmodIdx) + expect(fs.rename).toHaveBeenCalledWith(customTempPath, targetPath) + }) + }) + + // ── Test 7: symlink handling (Finding 4 regression test) ───────────────── + + describe("symlink handling", () => { + it("a write through a symlink commits onto the resolved referent, never the link path", async () => { + const linkPath = "/tmp/links/link.txt" + const referentPath = "/tmp/targets/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(referentPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(linkPath, "new-content", { platform: "linux" }) + + // The commit rename must target the realpath result (the referent), never the link itself — + // that is what guarantees a write through a symlink replaces the referent's content + // and preserves the link. + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), referentPath) + expect(fs.rename).not.toHaveBeenCalledWith(expect.anything(), linkPath) + }) + + it("when realpath reports ENOENT (target absent), uses the given path as-is", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // rename still happened with the fallback path (path.resolve on /tmp → C:\tmp) + const resolvedFallback = _resolvedTarget(targetPath) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), resolvedFallback) + }) + }) + + // ── Test 8: review fixes (permissions, partial writes, resolution, durability) ── + + describe("review fixes", () => { + it("preserves the target's restrictive mode and tolerates a failed staging-dir permission repair", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o600)) + // a pre-existing staging dir may fail its best-effort permission repair + vi.mocked(fsSync.chmodSync).mockImplementationOnce(() => { + throw new Error("EACCES") + }) + + await safeWriteText(targetPath, "secret", { platform: "linux" }) + + // the staging file inherits the target's 0o600 mode and the write commits + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), "w", 0o600) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("falls back to the 0o644 default when the target does not exist yet", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.statSync).mockImplementation(() => { + throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + }) + + await safeWriteText(targetPath, "fresh", { platform: "linux" }) + + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), "w", 0o644) + }) + + // The staging sub-directory must not outlive the write: a hidden directory left + // in every directory Zoo writes to shows up in the explorer, watchers and indexers. + it("removes the staging directory it created once the write commits", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o644)) + + await safeWriteText(targetPath, "content", { platform: "linux" }) + + expect(fsSync.rmdirSync).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + }) + + // A caller that staged its own temp file never had a staging directory created, + // so there is nothing of ours to remove - and removing a directory we did not + // create could delete one a concurrent writer is still using. + it("leaves no staging directory to remove when the caller supplied the tempPath", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o644)) + + await safeWriteText(targetPath, "", { tempPath: "/tmp/test-dir/.staged.json", platform: "linux" }) + + expect(fsSync.mkdirSync).not.toHaveBeenCalledWith( + expect.stringContaining(".file-safety-staging"), + expect.anything(), + ) + expect(fsSync.rmdirSync).not.toHaveBeenCalled() + }) + + // openSync applies the process umask to the requested mode, so the mode has to be + // set on the descriptor: a 0o664 target must not be published as 0o644. + it("sets the staged mode with fchmodSync so the process umask cannot narrow it", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(7) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o664)) + + await safeWriteText(targetPath, "content", { platform: "linux" }) + + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), "w", 0o664) + expect(fsSync.fchmodSync).toHaveBeenCalledWith(7, 0o664) + }) + + // The failure path unlinks the staged temp, which empties the staging directory: + // it must be removed there too, or a failed write leaves the hidden directory. + it("removes the staging directory when the commit fails", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o644)) + vi.mocked(fs.rename).mockRejectedValueOnce(Object.assign(new Error("EXDEV"), { code: "EXDEV" })) + + await expect(safeWriteText(targetPath, "content", { platform: "linux" })).rejects.toThrow("EXDEV") + + expect(fsSync.rmdirSync).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + }) + + it("loops on short writes until the full content is durable before fsync", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const content = "0123456789" // 10 bytes + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const buffer = Buffer.from(content, "utf8") + // first write (offset 0) reports 4 bytes (short write); the loop continues + vi.mocked(fsSync.writeSync).mockImplementation((...args: unknown[]) => + args[2] === 0 ? 4 : typeof args[3] === "number" ? args[3] : 0, + ) + + await safeWriteText(targetPath, content, { platform: "linux" }) + + // [0,10) reports 4 bytes, then [4,10) writes the remaining 6 + expect(fsSync.writeSync).toHaveBeenCalledTimes(2) + expect(fsSync.writeSync).toHaveBeenNthCalledWith(1, 1, buffer, 0, 10) + expect(fsSync.writeSync).toHaveBeenNthCalledWith(2, 1, buffer, 4, 6) + expect(fsSync.fsyncSync).toHaveBeenCalledWith(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("fsyncs the parent directory after the commit rename on POSIX", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // temp fd=1 then parent-dir fd=2 - distinct fds prove the ordering + vi.mocked(fsSync.openSync).mockReturnValueOnce(1).mockReturnValue(2) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // the directory fsync (fd 2) happens only after the file fsync (fd 1); + // the dir path assertion is path-agnostic (stringContaining) because + // path.dirname renders the same input differently on Windows + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("test-dir"), "r") + expect(fsSync.fsyncSync).toHaveBeenNthCalledWith(1, 1) + expect(fsSync.fsyncSync).toHaveBeenNthCalledWith(2, 2) + expect(fsSync.closeSync).toHaveBeenCalledWith(2) + }) + + it("treats a failed parent-directory fsync as best-effort", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync) + .mockReturnValueOnce(1) + .mockImplementationOnce(() => { + throw new Error("EBADF") + }) + + // the content rename already committed; a missing directory fsync is not fatal + await safeWriteText(targetPath, "data", { platform: "linux" }) + + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("propagates realpath errors (EACCES and code-less) instead of the fallback path", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const eacces = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + vi.mocked(fs.realpath).mockRejectedValueOnce(eacces) + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toBe(eacces) + expect(fs.rename).not.toHaveBeenCalled() + + const plain = new Error("resolution failed") + vi.mocked(fs.realpath).mockRejectedValueOnce(plain) + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toBe(plain) + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("backup:true propagates access errors (EACCES and code-less) instead of skipping the backup", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const eacces = Object.assign(new Error("EACCES"), { code: "EACCES" }) + const plain = new Error("access failed") + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // each write accesses dirPath then target; only the target access rejects + const rejectTarget = (error: Error) => async (p: unknown) => { + if (typeof p === "string" && p.endsWith("target.txt")) throw error + } + vi.mocked(fs.access) + .mockImplementationOnce(rejectTarget(eacces)) + .mockImplementationOnce(rejectTarget(eacces)) + .mockImplementationOnce(rejectTarget(plain)) + .mockImplementationOnce(rejectTarget(plain)) + + await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux" })).rejects.toEqual( + expect.objectContaining({ code: "EACCES" }), + ) + await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux" })).rejects.toThrow( + "access failed", + ) + expect(fs.rename).not.toHaveBeenCalled() + }) + }) + + // ── Test 9: pre-commit verification (verifyBeforeCommit, A4a guarded write) ── + + describe("pre-commit verification (verifyBeforeCommit)", () => { + it("rejects publication when the hook fails: no commit rename, temp discarded, error propagated", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const guard = new Error("stale version") + + await expect( + safeWriteText(targetPath, "data", { + platform: "linux", + verifyBeforeCommit: async () => { + throw guard + }, + }), + ).rejects.toBe(guard) + + // the commit rename never happened + expect(fs.rename).not.toHaveBeenCalled() + // the staged temp was discarded + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("runs the hook exactly once, immediately before the commit rename", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const hook = vi.fn(async () => {}) + const order: string[] = [] + vi.mocked(fs.rename).mockImplementation(async () => { + order.push("rename") + }) + + await safeWriteText(targetPath, "data", { + platform: "linux", + verifyBeforeCommit: async () => { + order.push("verify") + return hook() + }, + }) + + expect(hook).toHaveBeenCalledTimes(1) + expect(order).toEqual(["verify", "rename"]) + expect(fs.rename).toHaveBeenCalledTimes(1) + }) + + it("publishes normally when the hook succeeds", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { + platform: "linux", + verifyBeforeCommit: async () => {}, + }) + + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("runs the hook before the backup rename, so a rejection leaves the target in place (backup:true)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const guard = new Error("stale") + + await expect( + safeWriteText(targetPath, "data", { + backup: true, + platform: "linux", + verifyBeforeCommit: async () => { + throw guard + }, + }), + ).rejects.toBe(guard) + + // No rename at all: the hook runs before the target is moved aside, so a + // rejection never creates the moved-aside state that needed a rollback. + expect(fs.rename).not.toHaveBeenCalled() + // the staged temp was discarded + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("with backup:true the hook observes the target, not the post-backup absence", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // Simulate the on-disk truth: once the target has been renamed to the backup + // path, accessing it reports ENOENT. + let movedAside = false + vi.mocked(fs.rename).mockImplementation(async (from) => { + if (String(from) === targetPath) movedAside = true + return undefined + }) + vi.mocked(fs.access).mockImplementation(async (p) => { + if (String(p) === targetPath && movedAside) { + throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + } + return undefined + }) + + await safeWriteText(targetPath, "data", { + backup: true, + platform: "linux", + // A version-check hook reads the target - exactly what a replaceIfVersion + // guard does. After the backup rename it would see ENOENT and fail every + // write, and an absence-check hook would pass vacuously. + verifyBeforeCommit: async () => { + await fs.access(targetPath) + }, + }) + + // The backup rename did happen - after the hook ran. + expect(movedAside).toBe(true) + }) + }) +}) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts new file mode 100644 index 0000000000..1941409c30 --- /dev/null +++ b/src/services/file-safety/safeWriteText.ts @@ -0,0 +1,456 @@ +import * as fs from "fs/promises" +import * as fsSync from "fs" +import * as path from "path" +import { execFile } from "child_process" + +/** + * Options for safeWriteText atomic text publish primitive. + */ +export interface SafeWriteTextOptions { + /** + * When true, preserve the old-file semantics: rename target -> backup first, + * after commit rename delete the backup; on failure roll the backup back to + * the target path. When false (default) the atomic rename simply replaces + * the target -- crash-safe window is zero. + */ + backup?: boolean + + /** + * Platform override for testing. When omitted the real process.platform + * value is used. Set to "win32" or "linux" / "darwin" from tests so that + * both branches are reachable without needing a real Windows runner. + */ + platform?: string + + /** + * Custom execFile runner for testing (e.g. vi.fn). When omitted the real + * child_process.execFile is used. + */ + execFileRunner?: typeof execFile + + /** + * Pre-written temp path to use for the commit phase. When provided, + * safeWriteText skips creating its own staging file and uses this path + * instead (it still fsyncs before rename). Useful when a caller has + * already written data to a temp file via a custom stream. + */ + tempPath?: string + + /** + * Set when the caller has already resolved the publish target and guarded the + * symlink window itself (safeWriteJson resolves, locks, and re-checks the final + * component). safeWriteText then uses filePath as-is: resolving a second time would + * re-open the window the caller just closed, because a link installed after the + * caller's check would be followed here and the content committed to its referent. + */ + targetPathIsResolved?: boolean + + /** + * Pre-commit verification hook (A4a guarded write, epic #1375). Invoked + * immediately before the commit rename and BEFORE any backup rename moves the + * target aside, so a caller can re-check the target's state and reject + * publication when it changed since the caller's earlier verification. When the + * hook rejects, no commit rename is performed and no backup has been taken yet, + * so there is nothing to roll back: the staged temp file is discarded and the + * hook's rejection is propagated to the caller. + * + * Scope: the hook narrows - it does not close - the check-to-rename window for + * writers that the caller serializes (guardedWrite's per-path FIFO chain); an + * external process can still publish between the hook and the rename + * (documented in guardedWrite). + */ + verifyBeforeCommit?: () => Promise +} + +// -- helpers --------------------------------------------------------------- + +/** Generate a unique temp file name in the given directory. */ +function _tempName(dir: string, prefix: string): string { + return path.join(dir, "." + prefix + "_" + Date.now() + "_" + Math.random().toString(36).substring(2) + ".tmp") +} + +/** Create a private per-write staging sub-directory inside *dir*. The name is + * generated here, the way the temp file inside it is named, so concurrent writes + * never collide on their temp names and never share a directory whose release one + * write would have to decide about while another write is still staging in it. */ +function _stagingDir(dir: string): string { + // A shared fixed name is a path the workspace controls: mkdirSync with + // recursive:true accepts an existing entry of that name without creating + // anything, so a committed .file-safety-staging symlink would have the chmod + // below repair the permissions of a directory outside the target directory and + // put the staged file in it. A name this call generated cannot be pre-planted. + 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 + // covers the case where an entry already sits at the generated name (mkdirSync + // with recursive:true never chmods an existing directory), so staged temp files + // are never group/world readable. + fsSync.mkdirSync(sd, { recursive: true, mode: 0o700 }) + try { + fsSync.chmodSync(sd, 0o700) + } catch { + // best-effort: chmod denied or unavailable; a fresh dir was still + // created with the requested mode + } + return sd +} + +/** Remove this write's staging sub-directory when nothing is staged in it any + * more. rmdirSync fails on a non-empty directory (this write's temp unlink did not + * land) and on a directory that is already gone; both are tolerated. The name is + * this call's, so the rmdir never has to decide about a directory another write is + * still staging in, and a persistent hidden directory is never left in the user's + * workspace. */ +function _removeStagingDirIfEmpty(dir: string): void { + try { + fsSync.rmdirSync(dir) + } catch { + // non-empty (the temp unlink did not land) or already removed + } +} + +/** + * fsync a file descriptor so its data is durable before the atomic rename. + * Uses the sync form because this repo's @types/node does not declare + * fs.promises.fsync; the staging file is small, so the blocking window is bounded. + */ +function _fsyncFile(fd: number): void { + fsSync.fsyncSync(fd) +} + +/** Save the DACL of *srcPath* to a dump file on Windows. + * Returns true when the dump was written successfully; false otherwise. + * Never throws — callers treat failure as "skip DACL handling". */ +async function _saveDaclWindows(srcPath: string, dumpPath: string, execFileRunner?: typeof execFile): Promise { + const runner = execFileRunner ?? execFile + try { + await new Promise((resolve, reject) => { + runner("icacls", [srcPath, "/save", dumpPath, "/T"], { windowsHide: true }, (err) => + err ? reject(err) : resolve(), + ) + }) + return true + } catch { + return false + } +} + +/** Restore a DACL dump onto *dirPath* on Windows. + * Best-effort: content is already committed, so failure is non-fatal. */ +async function _restoreDaclWindows(dirPath: string, dumpPath: string, execFileRunner?: typeof execFile): Promise { + const runner = execFileRunner ?? execFile + try { + await new Promise((resolve, reject) => { + runner("icacls", [dirPath, "/restore", dumpPath], { windowsHide: true }, (err) => + err ? reject(err) : resolve(), + ) + }) + } catch { + // best-effort; content already committed + } +} + +// -- public API ------------------------------------------------------------ + +/** + * Atomic text publish primitive. + * + * 1. Write content to a temp file in a private per-write staging subdir + * (same volume -> atomic rename guaranteed). + * 2. fsync the temp file, then close it. + * 3. win32 only: if target exists save its DACL dump BEFORE backup rename. + * 4. Optionally run the pre-commit verification hook (verifyBeforeCommit) before + * the target is moved aside, so it observes the state the rename replaces; + * a rejection aborts the publish (no commit rename, no backup taken yet) and + * propagates. + * 5. Optionally rename target -> backup (when backup:true). + * 6. Atomic rename temp -> target. + * 7. win32 only: restore DACL onto the directory AFTER commit rename. + * 8. On success: delete backup (if any) and unlink DACL dump; remove the staging + * sub-directory when it is empty. + * 9. On failure: rollback backup to target path; clean up temp + dump, and remove + * the staging sub-directory when it is empty. + */ + +/** + * Resolve the publish target: the symlink referent when the given path is an + * existing symlink, the path itself otherwise. Only ENOENT (target absent yet) + * may fall back to the given path; any other resolution error (EACCES, EIO, ...) + * propagates so a broken or unreadable symlink is never written through its + * link path. Callers that stage a temp file themselves must stage it beside + * the resolved path: the commit is a rename onto the referent, and a rename + * across filesystems fails with EXDEV. + */ +export async function resolvePublishTarget(absoluteFilePath: string): Promise { + return fs.realpath(absoluteFilePath).catch((error: unknown) => { + const code = + typeof error === "object" && error !== null && "code" in error + ? (error as { code?: string }).code + : undefined + if (code !== "ENOENT") throw error + return absoluteFilePath + }) +} + +/** + * Raised when the commit rename failed AND the backup could not be renamed back onto the + * target: the target path is absent and the previous content survives only under the + * randomized backup path. The primary failure is preserved as originalError so callers + * keep the reason the publish failed while also learning where the saved state is. + */ +class RollbackFailedError extends Error { + readonly originalError: unknown + + constructor( + public readonly filePath: string, + public readonly backupPath: string, + rollbackError: unknown, + originalError: unknown, + ) { + super(_rollbackFailureMessage(filePath, backupPath, rollbackError, originalError), { + cause: rollbackError, + }) + this.name = "RollbackFailedError" + this.originalError = originalError + } +} + +function _rollbackFailureMessage( + filePath: string, + backupPath: string, + rollbackError: unknown, + originalError: unknown, +): string { + const primary = originalError instanceof Error ? originalError.message : String(originalError) + const rollback = rollbackError instanceof Error ? rollbackError.message : String(rollbackError) + return ( + `Publish to ${filePath} failed (${primary}) and the backup could not be restored (${rollback}). ` + + `The previous content is still at ${backupPath}.` + ) +} + +export async function safeWriteText(filePath: string, content: string, options?: SafeWriteTextOptions): Promise { + const absoluteFilePath = path.resolve(filePath) + + // Resolve the symlink referent (see resolvePublishTarget) - unless the caller + // already resolved it and closed the substitution window itself. + const targetPath = options?.targetPathIsResolved ? absoluteFilePath : await resolvePublishTarget(absoluteFilePath) + const dirPath = path.dirname(targetPath) + + // Ensure parent directory exists (mirrors safeWriteJson behaviour). + await fs.mkdir(dirPath, { recursive: true }) + await fs.access(dirPath) + + // Create the staging directory only when we generate the temp file there; + // callers supplying their own tempPath (e.g. safeWriteJson) must not be left + // with an empty staging directory behind, and a directory named here is this + // call's own, never a path the workspace already controls. + let stagingDirPath: string | null = null + let tempPath: string + if (options?.tempPath) { + tempPath = options.tempPath + } else { + stagingDirPath = _stagingDir(dirPath) + tempPath = _tempName(stagingDirPath, "safeWriteText") + } + + let backupPath: string | null = null + let releaseBackupOnSuccess = false + let daclDumpPath: string | null = null // tracked for cleanup in finally + let daclSaved = false // the restore step runs only when the save succeeded + + try { + // -- Step 1: write content to staging temp file ------------------- + if (!options?.tempPath) { + // Preserve the existing target's permissions: the staging file must + // not be published wider than the file it replaces (a 0o600 target + // must not become 0o644 through the atomic rename). + let targetMode = 0o644 // default for a fresh target + let targetExists = false + try { + targetMode = fsSync.statSync(targetPath).mode & 0o777 + targetExists = true + } catch { + // target does not exist yet - keep the default + } + const fd = fsSync.openSync(tempPath, "w", targetMode) + try { + // openSync applies the process umask to the requested mode, so a 0o664 + // or 0o666 target would be staged as 0o644 under the common umask 022 and + // lose group write through the rename. Set the mode on the fd instead, the + // same way the caller-staged branch below does - but only when a target + // actually existed to preserve. For a new target the creation mask must win: + // forcing the 0o644 default back on with fchmod would undo a restrictive + // umask (0o600 under umask 077) and publish a group/world-readable file. + if (targetExists) { + fsSync.fchmodSync(fd, targetMode) + } + // Loop until every byte is written: writeSync can report a short + // (partial) write, and publishing a truncated staging file would + // commit corrupt content. + const buffer = Buffer.from(content, "utf8") + let offset = 0 + while (offset < buffer.length) { + offset += fsSync.writeSync(fd, buffer, offset, buffer.length - offset) + } + _fsyncFile(fd) + } finally { + fsSync.closeSync(fd) + } + } else { + // Preserve the existing target's mode (CWE-732): the caller-staged + // temp carries its own creation mode, and publishing it as-is would + // widen a restrictive target (e.g. 0o600 -> 0o644) through rename. + // The mode is applied with fchmodSync on the open fd (AFTER openSync): + // chmodSync on the path before the open would make a read-only target + // (0o400/0o444) fail openSync(tempPath, "r+") with EACCES. + let targetMode: number | null = null + try { + targetMode = fsSync.statSync(targetPath).mode & 0o777 + } catch { + // target does not exist yet - keep the temp's default mode + } + const fd = fsSync.openSync(tempPath, "r+") + try { + if (targetMode !== null) { + fsSync.fchmodSync(fd, targetMode) + } + _fsyncFile(fd) + } finally { + fsSync.closeSync(fd) + } + } + + // -- Step 2 (win32): save DACL BEFORE backup rename --------------- + const platform = options?.platform ?? process.platform + if (platform === "win32") { + try { + await fs.access(targetPath) // target exists? + daclDumpPath = targetPath + ".acl.tmp" + const saved = await _saveDaclWindows(targetPath, daclDumpPath, options?.execFileRunner) + if (!saved) { + // Skip the RESTORE step only. icacls can create a partial dump and still exit + // non-zero, so the path stays tracked: dropping it here would leave that file + // next to the target with nothing left to remove it. + daclSaved = false + } else { + daclSaved = true + } + } catch { + // target does not exist or access failed — no DACL handling + daclDumpPath = null + } + } + + try { + // -- Step 3a (A4a): pre-commit verification -------------------------- + // Runs before the target is moved aside, so a conditional publication + // (guardedWrite's createIfAbsent / replaceIfVersion) validates against the state + // the commit rename will actually replace. Running it after the backup rename + // would make the target look absent: a version check would fail with ENOENT and + // an absence check would pass vacuously. A rejection skips the commit rename and + // discards the staged temp; no backup has been taken yet, so there is nothing to + // roll back. + if (options?.verifyBeforeCommit) { + await options.verifyBeforeCommit() + } + + // -- Step 3b (backup:true): rename target -> backup -------------- + if (options?.backup) { + try { + await fs.access(targetPath) + backupPath = _tempName(dirPath, "safeWriteText.bak") + await fs.rename(targetPath, backupPath) + releaseBackupOnSuccess = true + } catch (err: unknown) { + const code = + typeof err === "object" && err !== null && "code" in err + ? (err as { code?: string }).code + : undefined + if (code !== "ENOENT") throw err + } + } + + // -- Step 4: atomic rename temp -> target --------------------- + await fs.rename(tempPath, targetPath) + + // -- Step 4b (POSIX): fsync the parent directory so the directory entry + // changed by the commit rename is durable, not just the file content. + if (platform !== "win32") { + try { + const dirFd = fsSync.openSync(dirPath, "r") + try { + _fsyncFile(dirFd) + } finally { + fsSync.closeSync(dirFd) + } + } catch { + // best-effort: the content rename already committed + } + } + + // -- Step 5 (win32): restore DACL AFTER commit rename --------- + if (platform === "win32" && daclSaved && daclDumpPath !== null) { + const restoredDir = path.dirname(targetPath) + await _restoreDaclWindows(restoredDir, daclDumpPath, options?.execFileRunner) + } + + // -- Step 6 (backup:true): delete backup on success ----------- + if (releaseBackupOnSuccess && backupPath) { + try { + await fs.unlink(backupPath) + } catch { + // non-fatal — orphaned backup is acceptable + } + } + } finally { + // Unlink DACL dump regardless of success/failure in this span. + if (daclDumpPath !== null) { + await fs.unlink(daclDumpPath).catch(() => {}) + } + } + + // tempPath is now the committed file; no cleanup needed. The staging directory + // carries this call's generated name, so removing it cannot reach another write. + if (stagingDirPath !== null) { + _removeStagingDirIfEmpty(stagingDirPath) + } + } catch (originalError: unknown) { + // -- Rollback / cleanup on failure ---------------------------------- + let rollbackFailure: { backupPath: string; error: unknown } | null = null + if (backupPath && releaseBackupOnSuccess) { + try { + await fs.rename(backupPath, targetPath) + } catch (rollbackError: unknown) { + // The commit failed AND the restore failed: the target path is absent and the + // previous content survives only under the randomized backup name. Reporting only + // the primary failure leaves the caller unable to find that copy, so both are + // surfaced - the primary error stays reachable as originalError and in the text. + rollbackFailure = { backupPath, error: rollbackError } + } + } + + // Always clean up the staging temp file on failure. + try { + await fs.unlink(tempPath).catch(() => {}) + } catch { + // cleanup failure is non-fatal + } + + // The temp is gone, so this write's staging directory is empty and can go. A + // non-empty rmdir means the unlink above did not land; the directory is this + // call's own, so a concurrent write never loses its staging place here. + if (stagingDirPath !== null) { + _removeStagingDirIfEmpty(stagingDirPath) + } + + if (daclDumpPath !== null) { + await fs.unlink(daclDumpPath).catch(() => {}) + } + + if (rollbackFailure !== null) { + throw new RollbackFailedError(targetPath, rollbackFailure.backupPath, rollbackFailure.error, originalError) + } + + throw originalError + } +} diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index c973f35a03..48ae12ba4a 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -505,6 +505,21 @@ export class McpHub { return mcpServersPath } + /** + * A project-scoped MCP settings file is content the REPOSITORY controls, so a + * repository that plants .roo/mcp.json as a symlink must not receive the merged + * settings at the linked target - the payload would land outside the workspace, and + * MCP configs carry secrets and server commands. Those writes therefore refuse a + * symlink target instead of following it. The global settings file is deliberately + * left to follow a symlink: users legitimately link mcp_settings.json, it lives in + * the extension's global storage, and no repository controls that path. + * A connection with no recorded source is treated as global, matching the rest of this + * file (`conn.server.source || "global"`). + */ + private symlinkPolicyForSource(source: "global" | "project" | undefined): { refuseSymlinkTarget?: boolean } { + return source === "project" ? { refuseSymlinkTarget: true } : {} + } + async getMcpSettingsFilePath(): Promise { const provider = this.providerRef.deref() if (!provider) { @@ -2099,7 +2114,10 @@ export class McpHub { } this.isProgrammaticUpdate = true try { - await safeWriteJson(configPath, updatedConfig, { prettyPrint: true }) + await safeWriteJson(configPath, updatedConfig, { + prettyPrint: true, + ...this.symlinkPolicyForSource(source), + }) } finally { // Reset flag after watcher debounce period (non-blocking) this.flagResetTimer = setTimeout(() => { @@ -2184,7 +2202,10 @@ export class McpHub { mcpServers: config.mcpServers, } - await safeWriteJson(configPath, updatedConfig, { prettyPrint: true }) + await safeWriteJson(configPath, updatedConfig, { + prettyPrint: true, + ...this.symlinkPolicyForSource(serverSource), + }) // Update server connections with the correct source await this.updateServerConnections(config.mcpServers, serverSource) @@ -2393,7 +2414,7 @@ export class McpHub { } this.isProgrammaticUpdate = true try { - await safeWriteJson(normalizedPath, config, { prettyPrint: true }) + await safeWriteJson(normalizedPath, config, { prettyPrint: true, ...this.symlinkPolicyForSource(source) }) } finally { // Reset flag after watcher debounce period (non-blocking) this.flagResetTimer = setTimeout(() => { diff --git a/src/services/mcp/__tests__/McpHub.spec.ts b/src/services/mcp/__tests__/McpHub.spec.ts index 35ff10d64b..b89c55bc8d 100644 --- a/src/services/mcp/__tests__/McpHub.spec.ts +++ b/src/services/mcp/__tests__/McpHub.spec.ts @@ -35,6 +35,7 @@ vi.mock("fs/promises", () => { }) // Import safeWriteJson to use in mocks +import { safeWriteJson } from "../../../utils/safeWriteJson" // Mock safeWriteJson vi.mock("../../../utils/safeWriteJson", () => ({ @@ -1255,6 +1256,106 @@ describe("McpHub", () => { expect(writtenConfig.mcpServers["test-server"].alwaysAllow).toContain("new-tool") }) + describe("symlink policy on MCP settings writes", () => { + it("refuses a symlinked target when deleteServer omits the source but the server is a project server", async () => { + vi.mocked(fs.readFile).mockResolvedValue( + JSON.stringify({ + mcpServers: { "test-server": { type: "stdio", command: "node", args: ["test.js"] } }, + }), + ) + // Typed close doubles: updateServerConnections tears the connection + // down through transport.close() and client.close(). Empty doubles + // raise a TypeError that deleteConnection swallows, so the teardown + // would silently do nothing and the test would still pass. + const transportClose = vi.fn(async () => {}) + const clientClose = vi.fn(async () => {}) + mcpHub.connections = [ + { + type: "connected", + server: { + name: "test-server", + type: "stdio", + command: "node", + args: ["test.js"], + source: "project", + }, + client: { close: clientClose }, + transport: { close: transportClose }, + } as unknown as ConnectedMcpConnection, + ] + + // deleteServer("name") takes no source: findConnection resolves the connection and + // configPath is chosen from THAT source. The write policy must be derived from the same + // value, or a project server reached through an omitted argument loses symlink refusal. + await mcpHub.deleteServer("test-server") + + // The teardown ran to completion rather than dying on a TypeError. + expect(transportClose).toHaveBeenCalledTimes(1) + expect(clientClose).toHaveBeenCalledTimes(1) + + const write = vi.mocked(safeWriteJson).mock.calls.at(-1) + expect(write && write[2]).toEqual(expect.objectContaining({ refuseSymlinkTarget: true })) + }) + + it("refuses a symlinked target for a project toggleToolAlwaysAllow write", async () => { + vi.mocked(fs.readFile).mockResolvedValue( + JSON.stringify({ + mcpServers: { + "test-server": { type: "stdio", command: "node", args: ["test.js"], alwaysAllow: [] }, + }, + }), + ) + mcpHub.connections = [ + { + type: "connected", + server: { + name: "test-server", + type: "stdio", + command: "node", + args: ["test.js"], + source: "project", + }, + client: {}, + transport: {}, + } as unknown as ConnectedMcpConnection, + ] + + await mcpHub.toggleToolAlwaysAllow("test-server", "project", "new-tool", true) + + const write = vi.mocked(safeWriteJson).mock.calls.at(-1) + expect(write && write[2]).toEqual(expect.objectContaining({ refuseSymlinkTarget: true })) + }) + + it("leaves the global toggleToolAlwaysAllow write without a symlink refusal", async () => { + vi.mocked(fs.readFile).mockResolvedValue( + JSON.stringify({ + mcpServers: { + "test-server": { type: "stdio", command: "node", args: ["test.js"], alwaysAllow: [] }, + }, + }), + ) + mcpHub.connections = [ + { + type: "connected", + server: { + name: "test-server", + type: "stdio", + command: "node", + args: ["test.js"], + source: "global", + }, + client: {}, + transport: {}, + } as unknown as ConnectedMcpConnection, + ] + + await mcpHub.toggleToolAlwaysAllow("test-server", "global", "new-tool", true) + + const write = vi.mocked(safeWriteJson).mock.calls.at(-1) + expect(write && write[2]).not.toHaveProperty("refuseSymlinkTarget") + }) + }) + it("should remove tool from always allow list when disabling", async () => { const mockConfig = { mcpServers: { @@ -2026,6 +2127,75 @@ describe("McpHub", () => { }) describe("updateServerTimeout", () => { + it("refuses a symlinked target for a project-scoped timeout write", async () => { + vi.mocked(fs.readFile).mockResolvedValueOnce( + JSON.stringify({ + mcpServers: { + "test-server": { type: "stdio", command: "node", args: ["test.js"], timeout: 60 }, + }, + }), + ) + // The SDK client/transport are never touched by this write path (it reads only + // server.name and server.source), so the literal is projected onto the connection + // type through unknown rather than adding another `as any` to this file. + mcpHub.connections = [ + { + type: "connected", + server: { + name: "test-server", + type: "stdio", + command: "node", + args: ["test.js"], + timeout: 60, + source: "project", + }, + client: {}, + transport: {}, + } as unknown as ConnectedMcpConnection, + ] + + await mcpHub.updateServerTimeout("test-server", 120) + + // A project .roo/mcp.json is repository-controlled: if it is a symlink, the merged + // settings (secrets + server commands) must NOT land at the referent outside the + // workspace, so this write refuses the link instead of following it. + const write = vi.mocked(safeWriteJson).mock.calls.at(-1) + expect(write && write[2]).toEqual(expect.objectContaining({ refuseSymlinkTarget: true })) + }) + + it("leaves the global settings write without a symlink refusal", async () => { + vi.mocked(fs.readFile).mockResolvedValueOnce( + JSON.stringify({ + mcpServers: { + "test-server": { type: "stdio", command: "node", args: ["test.js"], timeout: 60 }, + }, + }), + ) + mcpHub.connections = [ + { + type: "connected", + server: { + name: "test-server", + type: "stdio", + command: "node", + args: ["test.js"], + timeout: 60, + source: "global", + }, + client: {}, + transport: {}, + } as unknown as ConnectedMcpConnection, + ] + + await mcpHub.updateServerTimeout("test-server", 120) + + // The global file lives in the extension global storage and users legitimately link + // it, so this writer does not opt into the symlink refusal; what the assertion + // covers is that the option is absent, not what the filesystem then does. + const write = vi.mocked(safeWriteJson).mock.calls.at(-1) + expect(write && write[2]).not.toHaveProperty("refuseSymlinkTarget") + }) + it("should update server timeout in settings file", async () => { const mockConfig = { mcpServers: { diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 30ac500212..784723303d 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -6,7 +6,7 @@ import * as os from "os" import { safeWriteJson } from "../safeWriteJson" // Capture actual implementations before the vi.mock factory runs, -// so they are never wrapped by vi.fn() — avoids infinite recursion when +// so they are never wrapped by vi.fn() ? avoids infinite recursion when // test mockImplementation callbacks delegate to the real implementation. const fsPromisesActuals = vi.hoisted(() => ({ rename: undefined as (typeof import("fs/promises"))["rename"] | undefined, @@ -14,6 +14,13 @@ const fsPromisesActuals = vi.hoisted(() => ({ writeFile: undefined as (typeof import("fs/promises"))["writeFile"] | undefined, })) +// The sync open is captured the same way: the staging rows below spy fs.openSync to inspect +// or refuse the exclusive create, and a spy that delegated to the spied binding would recurse +// instead of opening the file. +const fsSyncActuals = vi.hoisted(() => ({ + openSync: undefined as (typeof import("fs"))["openSync"] | undefined, +})) + vi.mock("fs/promises", async () => { const actual = await vi.importActual("fs/promises") fsPromisesActuals.rename = actual.rename @@ -42,6 +49,10 @@ vi.mock("fs/promises", async () => { // Mock the 'fs' module for fsSync.createWriteStream vi.mock("fs", async () => { const actualFs = await vi.importActual("fs") + // The staging rows spy fs.openSync and delegate through this captured real open, + // so a spy never recurses into its own binding. + fsSyncActuals.openSync = actualFs.openSync + return { ...actualFs, // Spread actual implementations createWriteStream: vi.fn(actualFs.createWriteStream), // Default to actual, but mockable @@ -50,6 +61,13 @@ vi.mock("fs", async () => { import * as fs from "fs/promises" // This will now be the mocked version +/** One recorded fs.openSync of a staging file. */ +interface StageOpen { + path: string + flags: string + mode: number | undefined +} + function makeErrorWriteStream(message: string): fsSyncActual.WriteStream { const stream = Object.assign( new Writable({ @@ -169,7 +187,7 @@ describe("safeWriteJson", () => { // Overwrite the pre-created file with specific initial data await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) - // fs.rename is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn + // fs.rename is already vi.fn() ? use vi.mocked to avoid double-wrapping via vi.spyOn // First call: the single-rename commit fails with the Windows-style EPERM // (a reader holds the target open). The fallback then renames the target // to a backup and the replacement into place, using real renames. @@ -198,7 +216,7 @@ describe("safeWriteJson", () => { // Track rename calls let renameCallCount = 0 - // fs.rename is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn + // fs.rename is already vi.fn() ? use vi.mocked to avoid double-wrapping via vi.spyOn vi.mocked(fs.rename).mockImplementation(async (oldPath, newPath) => { renameCallCount++ if (renameCallCount === 1) { @@ -267,7 +285,7 @@ describe("safeWriteJson", () => { }) test("should handle directory creation permission errors", async () => { - // fs.mkdir is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn + // fs.mkdir is already vi.fn() ? use vi.mocked to avoid double-wrapping via vi.spyOn vi.mocked(fs.mkdir).mockImplementationOnce(async () => { const error = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) throw error @@ -315,7 +333,7 @@ describe("safeWriteJson", () => { throw Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) }) - // fs.unlink is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn + // fs.unlink is already vi.fn() ? use vi.mocked to avoid double-wrapping via vi.spyOn vi.mocked(fs.unlink).mockImplementationOnce(async () => { throw new Error("Failed to delete backup file") }) @@ -328,8 +346,9 @@ describe("safeWriteJson", () => { expect(content).toEqual(newData) }) - // Test for console error suppression during backup deletion - test("should suppress console.error when backup deletion fails", async () => { + // The json entry point's own half of the backup-deletion claim: the failure is reported, + // not fatal, and the orphaned backup stays recoverable. + test("should keep the committed content and the orphaned backup when backup deletion fails", async () => { const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) // Suppress console.error const initialData = { message: "Initial" } const newData = { message: "New" } @@ -341,7 +360,7 @@ describe("safeWriteJson", () => { throw Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) }) - // fs.unlink is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn + // fs.unlink is already vi.fn() ? use vi.mocked to avoid double-wrapping via vi.spyOn vi.mocked(fs.unlink).mockImplementation(async (filePath) => { if (filePath.toString().includes(".bak_")) { throw new Error("Backup deletion failed") @@ -354,6 +373,16 @@ describe("safeWriteJson", () => { // Verify console.error was called with the expected message expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining("Successfully wrote"), expect.any(Error)) + // The claim the absent title made about json behaviour, asserted here at this entry + // point instead of only at the text layer: the payload is committed, the backup that + // could not be deleted survives with the pre-write content for a later writer to + // recover, and no staging file is left behind. + expect(await readFileContent(currentTestFilePath)).toEqual(newData) + const orphanedBackups = (await fs.readdir(tempDir)).filter((entry) => entry.includes(".bak_")) + expect(orphanedBackups).toHaveLength(1) + expect(await readFileContent(path.join(tempDir, orphanedBackups[0]))).toEqual(initialData) + expect((await fs.readdir(tempDir)).filter((entry) => entry.includes(".new_"))).toHaveLength(0) + consoleErrorSpy.mockRestore() vi.mocked(fs.unlink).mockRestore() }) @@ -366,7 +395,7 @@ describe("safeWriteJson", () => { await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) - // fs.rename is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn + // fs.rename is already vi.fn() ? use vi.mocked to avoid double-wrapping via vi.spyOn vi.mocked(fs.rename).mockImplementationOnce(async () => { throw new Error("Rename failed") }) @@ -422,7 +451,7 @@ describe("safeWriteJson", () => { test("should handle fs.access error that is not ENOENT", async () => { const data = { message: "access error test" } - // fs.access is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn + // fs.access is already vi.fn() ? use vi.mocked to avoid double-wrapping via vi.spyOn vi.mocked(fs.access).mockImplementationOnce(async () => { const error = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) throw error @@ -446,7 +475,7 @@ describe("safeWriteJson", () => { const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) // Suppress console.error - // fs.rename is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn + // fs.rename is already vi.fn() ? use vi.mocked to avoid double-wrapping via vi.spyOn let renameCallCount = 0 vi.mocked(fs.rename).mockImplementation(async (oldPath, newPath) => { renameCallCount++ @@ -819,3 +848,778 @@ describe("safeWriteJson", () => { expect(content).toEqual(newData) }) }) + +// Two guarded-write titles are deliberately absent from this file. The claims they name split +// in two, and where each half is tested is measured rather than asserted: + +// The json-layer consequences are asserted in this file, at this entry point: +// "should keep the committed content and the orphaned backup when backup deletion fails" +// the write succeeds, the target holds the new payload, the .bak_ survives with the +// pre-write content, and no .new_ staging file is left. +// "should log error and re-throw original if rollback fails" +// the original error rejects the write, the .bak_ survives with the pre-write content, +// and a later write recovers it. Inherited from upstream main at bb6cbee59, not new here. + +// What stays only at the text layer is that module's own taxonomy, which this file never +// claimed to cover and does not: +// src/services/file-safety/__tests__/safeWriteText.spec.ts +// "a post-commit backup cleanup failure is non-fatal: the target stays committed and no temp is left behind" +// "throws RollbackFailedError with the backup path when the rollback rename also fails" +// The RollbackFailedError type and the .bak_ naming live in safeWriteText.ts; this entry +// point has its own backup protocol and never constructs that error. + +describe("safeWriteJson guarded-publish requirements carried by the guarded-write branch", () => { + let originalConsoleError: typeof console.error + + beforeAll(() => { + // Store original console.error + originalConsoleError = console.error + }) + + afterAll(() => { + // Restore original console.error + console.error = originalConsoleError + }) + + let tempDir: string + let currentTestFilePath: string + + beforeEach(async () => { + // Create a temporary directory for each test + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "safeWriteJson-test-")) + + // Create a unique file path for each test + currentTestFilePath = path.join(tempDir, "test-file.json") + + // Pre-create the file with initial content to ensure it exists + // This allows proper-lockfile to acquire a lock on an existing file. + await fs.writeFile(currentTestFilePath, JSON.stringify({ initial: "content" })) + }) + + afterEach(async () => { + // Clean up the temporary directory after each test + await fs.rm(tempDir, { recursive: true, force: true }) + + // Reset all mocks to their actual implementations + vi.restoreAllMocks() + }) + + // Helper function to read file content + async function readFileContent(filePath: string): Promise { + const readContent = await fs.readFile(filePath, "utf-8") + return JSON.parse(readContent) + } + + // Helper function to check if a file exists + async function fileExists(filePath: string): Promise { + try { + await fs.access(filePath) + return true + } catch { + return false + } + } + + // The commit rename is no-follow for the final component: it targets the path the caller + // named, so a link the caller never chose is REPLACED by the rename instead of receiving the + // payload. Staging therefore happens beside the named path - the same directory the rename + // lands in - which is also what keeps the rename on one volume. (Real symlinks are unavailable + // in this CI lane, so the alias is simulated by mocking fs.realpath.) + test("stages beside the caller-named path and never publishes through the symlink", async () => { + const referentDir = path.join(tempDir, "referent") + const linkDir = path.join(tempDir, "link") + await fs.mkdir(referentDir, { recursive: true }) + await fs.mkdir(linkDir, { recursive: true }) + // caller-visible path (the link) vs the referent a resolution would hand back + const callerPath = path.join(linkDir, "test-file.json") + const referentPath = path.join(referentDir, "test-file.json") + // Seed the referent with real content: if the publish followed the alias, this is what + // would be replaced. + await fsPromisesActuals.writeFile!(referentPath, JSON.stringify({ seed: true })) + + vi.spyOn(fs, "realpath").mockResolvedValue(referentPath) + + await safeWriteJson(callerPath, { after: true }) + + // the temp file was created next to the named path, NOT beside the referent + const tempPaths = vi.mocked(fsSyncActual.createWriteStream).mock.calls.map((call) => String(call[0])) + expect(tempPaths.some((p) => p.startsWith(linkDir + path.sep) && p.includes(".new_"))).toBe(true) + expect(tempPaths.some((p) => p.startsWith(referentDir + path.sep))).toBe(false) + + // the payload landed on the path the caller named; the referent is untouched + expect(await readFileContent(callerPath)).toEqual({ after: true }) + expect(await readFileContent(referentPath)).toEqual({ seed: true }) + }) + + // proper-lockfile with realpath:false keys the lock by the given path, so a + // symlink alias and its referent must coordinate through ONE lock on the + // resolved referent ? otherwise a concurrent merge through both aliases + // reads the same JSON and overwrites one update. (Real symlinks are + // unavailable in this CI lane, so the resolution is simulated by mocking + // fs.realpath, the same way as the staging test above.) + test("acquires the lock on the resolved referent, not the caller alias", async () => { + vi.resetModules() // fresh module instances so the doMock below is picked up + + const referentDir = path.join(tempDir, "lock-referent") + const linkDir = path.join(tempDir, "lock-link") + await fs.mkdir(referentDir, { recursive: true }) + await fs.mkdir(linkDir, { recursive: true }) + // caller-visible path (the link) vs the resolved referent path + const callerPath = path.join(linkDir, "locked.json") + const referentPath = path.join(referentDir, "locked.json") + await fsPromisesActuals.writeFile!(referentPath, JSON.stringify({ seed: 1 })) + + vi.spyOn(fs, "realpath").mockResolvedValue(referentPath) + + // Wrap the real lock in a capturing mock, and drive the two rare error paths + // (the onCompromised callback and a failing release) so they stay covered + // without real lockfile staleness. The callback rethrows by design, so + // the mock swallows that throw and lets the real lock proceed. + const realLockfile = await vi.importActual("proper-lockfile") + const lockMockFn = vi.fn( + async ( + file: Parameters[0], + options?: Parameters[1], + ) => { + try { + options?.onCompromised?.(new Error("lock compromised (test)")) + } catch { + // onCompromised rethrows by design; swallow so the real lock proceeds. + } + const release = await realLockfile.lock(file, options) + return async () => { + await release() + throw new Error("release failed (test)") + } + }, + ) + const lockMock = lockMockFn as unknown as typeof realLockfile.lock + vi.doMock("proper-lockfile", () => ({ + ...realLockfile, + lock: lockMock, + })) + + // Re-import safeWriteJson so it picks up the mocked proper-lockfile. + const { safeWriteJson: mockedSafeWriteJson } = await import("../safeWriteJson") + + const mergeFn = vi.fn((existing: unknown, incoming: unknown) => ({ + ...(existing as Record), + ...(incoming as Record), + })) + + // Capture the compromise + release-failure logs. + const consoleErrorSpy = vi.spyOn(console, "error") + await mockedSafeWriteJson(callerPath, { added: true }, { merge: mergeFn }) + + // The lock was keyed by the resolved referent ? every alias shares it. + expect(lockMock).toHaveBeenCalledTimes(1) + expect(String(lockMockFn.mock.calls[0][0])).toBe(referentPath) + // The merge reads the path the caller named, not the referent: the publish is no-follow, + // so the referent's content is never read or written through the alias. + expect(mergeFn).toHaveBeenCalledTimes(1) + expect(await readFileContent(callerPath)).toEqual({ added: true }) + expect(await readFileContent(referentPath)).toEqual({ seed: 1 }) + // The compromise callback and the failed release were logged, not thrown. + expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining("was compromised"), expect.any(Error)) + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.stringContaining("Failed to release lock"), + expect.any(Error), + ) + + // The hoisted vi.unmock runs before this test's runtime vi.doMock, so it + // cannot remove it; doUnmock + resetModules clear the registry entry so + // later test files import the real proper-lockfile. + vi.doUnmock("proper-lockfile") + vi.resetModules() + }) + + // The staged payload can carry API credentials, and the staging path is generated inside a + // directory the workspace already controls. Two properties make writing into it safe: it is + // opened exclusively, so a file another writer planted at the generated name fails the write + // instead of receiving the payload, and it is created at the mode of the file it replaces, so + // the commit rename can neither widen a 0o600 target nor narrow a shared one. The text writer + // applies the same rule: src/services/file-safety/safeWriteText.ts stages at the target's mode + // with 0o644 as the default for a target that does not exist yet. + /** Record every open of a staging file made through fs.openSync, and pass each call to the + * captured real open unless this row asked to be refused. */ + function spyOnStageOpens(refuseStagedOpens: boolean): StageOpen[] { + const stageOpens: StageOpen[] = [] + const openSyncSpy = vi.spyOn(fsSyncActual, "openSync") + openSyncSpy.mockImplementation((filePath, flags, mode) => { + const openedPath = String(filePath) + if (openedPath.includes(".new_")) { + stageOpens.push({ + path: openedPath, + flags: String(flags), + mode: typeof mode === "number" ? mode : undefined, + }) + if (refuseStagedOpens) { + throw Object.assign(new Error("EEXIST: file already exists, open"), { code: "EEXIST" }) + } + } + return fsSyncActuals.openSync!(filePath, flags, mode) + }) + return stageOpens + } + + test("refuses to write into a staging path another writer already created", async () => { + const stageOpens = spyOnStageOpens(true) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await expect(safeWriteJson(currentTestFilePath, { message: "New" })).rejects.toThrow("EEXIST") + + // The refusal happened at a generated staging path, under an exclusive create, and not at + // the target: the committed file keeps its pre-write content and nothing is left behind. + expect(stageOpens.length).toBeGreaterThan(0) + expect(stageOpens.every((open) => open.flags === "wx")).toBe(true) + expect(await readFileContent(currentTestFilePath)).toEqual({ initial: "content" }) + expect((await fs.readdir(tempDir)).filter((entry) => entry.includes(".new_"))).toHaveLength(0) + + consoleErrorSpy.mockRestore() + }) + + test("stages the payload at the mode of the file it replaces", async () => { + const targetMode = fsSyncActual.statSync(currentTestFilePath).mode & 0o777 + const stageOpens = spyOnStageOpens(false) + + await safeWriteJson(currentTestFilePath, { message: "New" }) + + expect(stageOpens).toHaveLength(1) + expect(stageOpens[0].flags).toBe("wx") + expect(stageOpens[0].mode).toBe(targetMode) + expect(await readFileContent(currentTestFilePath)).toEqual({ message: "New" }) + }) + + test("keeps the creation default for a target that does not exist yet", async () => { + const freshTarget = path.join(tempDir, "brand-new.json") + const stageOpens = spyOnStageOpens(false) + + await safeWriteJson(freshTarget, { created: true }) + + // A target that does not exist yet is staged at the 0o644 default, so the process umask + // still decides what the published file becomes, exactly as it did before this change. + expect(stageOpens).toHaveLength(1) + expect(stageOpens[0].flags).toBe("wx") + expect(stageOpens[0].mode).toBe(0o644) + expect(await readFileContent(freshTarget)).toEqual({ created: true }) + }) + + // The two alias rows above simulate the link by mocking fs.realpath. This one asks the platform + // for a real link: a junction on Windows, which needs no elevation, and a directory symlink + // elsewhere. Where the platform refuses, the row records the refusal instead of asserting a + // resolution it never exercised. + test("keys the lock to the referent of a real link, not to the alias the caller named", async () => { + vi.resetModules() + + const referentDir = path.join(tempDir, "real-referent") + const aliasDir = path.join(tempDir, "real-alias") + await fs.mkdir(referentDir, { recursive: true }) + let linkRefusal: string | undefined + try { + await fs.symlink(referentDir, aliasDir, process.platform === "win32" ? "junction" : "dir") + } catch (error: unknown) { + linkRefusal = + error && typeof error === "object" && "code" in error + ? String((error as { code?: string }).code) + : "unknown" + } + if (linkRefusal !== undefined) { + // Capability probe: a lane that cannot create the link says why rather than passing + // a row that never resolved anything. + expect(["EPERM", "ENOSYS", "ENOTSUP", "EOPNOTSUPP"]).toContain(linkRefusal) + return + } + + const aliasFile = path.join(aliasDir, "locked.json") + const referentFile = path.join(referentDir, "locked.json") + await fsPromisesActuals.writeFile!(referentFile, JSON.stringify({ seed: 1 })) + + const realLockfile = await vi.importActual("proper-lockfile") + const lockMockFn = vi.fn( + async (file: Parameters[0], options?: Parameters[1]) => + realLockfile.lock(file, options), + ) + // The mock must be assignable to the module's lock signature; this is the same double + // shape the alias row above uses. + const lockMock = lockMockFn as unknown as typeof realLockfile.lock + vi.doMock("proper-lockfile", () => ({ ...realLockfile, lock: lockMock })) + + const { safeWriteJson: realLinkSafeWriteJson } = await import("../safeWriteJson") + const mergeFn = vi.fn((existing: unknown, incoming: unknown) => ({ + ...(existing as Record), + ...(incoming as Record), + })) + await realLinkSafeWriteJson(aliasFile, { added: true }, { merge: mergeFn }) + + // The link is the operating system's, so the resolution is not a mock's: the alias and the + // referent are two names for one file, and both writers must contend on one lock. + const resolvedReferent = await fs.realpath(aliasFile) + expect(resolvedReferent).not.toBe(aliasFile) + expect(lockMockFn).toHaveBeenCalledTimes(1) + expect(String(lockMockFn.mock.calls[0][0])).toBe(resolvedReferent) + // The merge read the seeded content through the alias, and the write landed on the same + // file, seen from either name. + expect(mergeFn.mock.calls[0][0]).toEqual({ seed: 1 }) + expect(await readFileContent(aliasFile)).toEqual({ seed: 1, added: true }) + expect(await readFileContent(referentFile)).toEqual({ seed: 1, added: true }) + + // Measured on this box: removing the junction leaves the referent and its content. + await fs.unlink(aliasDir) + vi.doUnmock("proper-lockfile") + vi.resetModules() + }) + + // A settings export carries API credentials, so it must not be redirected through + // a link the user never chose. (Real symlinks are unavailable in this CI lane, so + // the link is simulated by mocking fs.lstat.) + test("refuses to publish through a symlink when refuseSymlinkTarget is set", async () => { + const referentPath = path.join(tempDir, "refuse-referent.json") + const linkPath = path.join(tempDir, "refuse-link.json") + await fsPromisesActuals.writeFile!(referentPath, JSON.stringify({ seed: "untouched" })) + + vi.spyOn(fs, "lstat").mockResolvedValue({ + isSymbolicLink: () => true, + // The guard reads only isSymbolicLink(), and a real Stats cannot be + // produced for a simulated link in this CI lane, so the double is + // asserted through unknown rather than stubbing every Stats field. + } as unknown as fsSyncActual.Stats) + + await expect(safeWriteJson(linkPath, { leaked: true }, { refuseSymlinkTarget: true })).rejects.toThrow( + /refusing to write through the symlink/, + ) + + vi.restoreAllMocks() + // Nothing was resolved, staged, locked, or committed: the referent still holds + // the content it had before the refused write. + expect(await readFileContent(referentPath)).toEqual({ seed: "untouched" }) + }) + + test("still writes a regular file when refuseSymlinkTarget is set", async () => { + const target = path.join(tempDir, "refuse-regular.json") + await safeWriteJson(target, { written: true }, { refuseSymlinkTarget: true }) + expect(await readFileContent(target)).toEqual({ written: true }) + }) + + test("rejects when the destination is swapped for a link after the initial refusal check", async () => { + const referentPath = path.join(tempDir, "swap-referent.json") + const linkPath = path.join(tempDir, "swap-link.json") + await fsPromisesActuals.writeFile!(referentPath, JSON.stringify({ seed: "untouched" })) + await fsPromisesActuals.writeFile!(linkPath, JSON.stringify({ own: true })) + const asLink = { isSymbolicLink: () => true } as unknown as fsSyncActual.Stats + const asFile = { isSymbolicLink: () => false } as unknown as fsSyncActual.Stats + // The first lstat (the refusal) sees a regular file; the re-check after + // resolvePublishTarget sees the link a local writer installed in between. + // Path-aware rather than call-order: the ancestor walk also calls lstat, so a + // mockResolvedValueOnce chain would be consumed by the wrong component. The first look at + // the target sees a regular file; the re-check after resolvePublishTarget sees the link. + let targetLooks = 0 + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + if (String(p) === linkPath) { + targetLooks++ + return Promise.resolve(targetLooks === 1 ? asFile : asLink) + } + return Promise.resolve(asFile) + }) as unknown as typeof fs.lstat) + + await expect(safeWriteJson(linkPath, { leaked: true }, { refuseSymlinkTarget: true })).rejects.toThrow( + /after resolution/, + ) + + vi.restoreAllMocks() + expect(await readFileContent(referentPath)).toEqual({ seed: "untouched" }) + }) + + test("rejects when the destination becomes a link before the commit is staged", async () => { + const referentPath = path.join(tempDir, "late-referent.json") + const linkPath = path.join(tempDir, "late-link.json") + await fsPromisesActuals.writeFile!(referentPath, JSON.stringify({ seed: "untouched" })) + await fsPromisesActuals.writeFile!(linkPath, JSON.stringify({ own: true })) + const asLink = { isSymbolicLink: () => true } as unknown as fsSyncActual.Stats + const asFile = { isSymbolicLink: () => false } as unknown as fsSyncActual.Stats + // The swap happens after resolution and while the write is already under the + // lock: the in-lock re-check must stop the commit rename. + // Same path-aware scripting: the swap happens after resolution and while the write is + // already under the lock, so the third look at the target is the one that must be a link. + let lateLooks = 0 + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + if (String(p) === linkPath) { + lateLooks++ + return Promise.resolve(lateLooks <= 2 ? asFile : asLink) + } + return Promise.resolve(asFile) + }) as unknown as typeof fs.lstat) + + await expect(safeWriteJson(linkPath, { leaked: true }, { refuseSymlinkTarget: true })).rejects.toThrow( + /before publication/, + ) + + vi.restoreAllMocks() + expect(await readFileContent(referentPath)).toEqual({ seed: "untouched" }) + }) + + test("publishes onto the named path when a link is planted during target resolution", async () => { + const referentPath = path.join(tempDir, "race-referent.json") + const namedPath = path.join(tempDir, "race-named.json") + await fsPromisesActuals.writeFile!(referentPath, JSON.stringify({ seed: "untouched" })) + await fsPromisesActuals.writeFile!(namedPath, JSON.stringify({ own: true })) + const asFile = { isSymbolicLink: () => false } as unknown as fsSyncActual.Stats + // Every lstat in the sequence sees a regular file: the link a local writer plants + // while the destination is being resolved is gone again before the next check, so + // no check in the sequence can catch it. + vi.spyOn(fs, "lstat").mockResolvedValue(asFile) + // Resolution is the step that would hand back a destination the caller never chose. + const realpathSpy = vi.spyOn(fs, "realpath").mockResolvedValue(referentPath) + + await safeWriteJson(namedPath, { written: true }, { refuseSymlinkTarget: true }) + + vi.restoreAllMocks() + // With refuseSymlinkTarget the named path is the publish target, so resolution never + // runs and there is no window in which a planted link can redirect the payload. The + // commit is a rename, which replaces the named directory entry instead of writing + // through a link, so the referent cannot receive the export. + expect(realpathSpy).not.toHaveBeenCalled() + expect(await readFileContent(namedPath)).toEqual({ written: true }) + expect(await readFileContent(referentPath)).toEqual({ seed: "untouched" }) + }) + + test("resolves the publish target only once during a guarded publication", async () => { + const target = path.join(tempDir, "single-resolve.json") + // The caller resolves once for the lock key; safeWriteText must not resolve again, + // or a link installed after the caller's re-check would be followed there. + const realpath = vi.spyOn(fs, "realpath").mockImplementation(async (p) => String(p)) + + await safeWriteJson(target, { written: true }) + + expect(realpath).toHaveBeenCalledTimes(1) + vi.restoreAllMocks() + expect(await readFileContent(target)).toEqual({ written: true }) + }) + + test("refuses a write when an ancestor directory is a symlink", async () => { + const target = path.join(tempDir, "ancestor-link.json") + await fsPromisesActuals.writeFile!(target, JSON.stringify({ own: true })) + const asLink = { isSymbolicLink: () => true } as unknown as fsSyncActual.Stats + const asFile = { isSymbolicLink: () => false } as unknown as fsSyncActual.Stats + // The target itself looks ordinary; the link sits one directory above it. Checking only the + // final component would publish a credential payload outside the directory the caller named. + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + return Promise.resolve(String(p) === tempDir ? asLink : asFile) + }) as unknown as typeof fs.lstat) + + await expect(safeWriteJson(target, { leaked: true }, { refuseSymlinkTarget: true })).rejects.toThrow( + /is a symlink/, + ) + + vi.restoreAllMocks() + expect(await readFileContent(target)).toEqual({ own: true }) + }) + + // The initial walk and the commit rename are separated by the whole staging sequence, and + // Node exposes no handle-relative rename: an ancestor that passed the walk can be swapped + // while the payload is staged, and the path-based commit rename would follow it. The + // publication therefore re-derives the final entry and every recorded ancestor identity + // immediately before the commit rename. (Real symlinks are unavailable in this CI lane, so + // the swap is simulated by scripting fs.lstat per visit, the same way as the tests above.) + test("refuses the publish when an ancestor is swapped for a link after the initial walk", async () => { + const parent = path.join(tempDir, "swap-parent") + await fs.mkdir(parent, { recursive: true }) + const target = path.join(parent, "swap-ancestor.json") + await fsPromisesActuals.writeFile!(target, JSON.stringify({ own: true })) + // The guard reads isSymbolicLink plus the bigint identity, and a real BigIntStats cannot + // be produced for a simulated directory in this CI lane, so the doubles are asserted + // through unknown rather than stubbing every Stats field. + const asDir = { isSymbolicLink: () => false, dev: 1n, ino: 1n } as unknown as fsSyncActual.BigIntStats + const asLink = { isSymbolicLink: () => true, dev: 2n, ino: 2n } as unknown as fsSyncActual.BigIntStats + // Path-aware scripting: the first look at the parent is the walk that authorizes it; the + // next look happens inside the publication, where the swap must be caught. + let parentLooks = 0 + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + if (String(p) === parent) { + parentLooks++ + return Promise.resolve(parentLooks === 1 ? asDir : asLink) + } + return Promise.resolve(asDir) + }) as unknown as typeof fs.lstat) + + await expect(safeWriteJson(target, { leaked: true }, { refuseSymlinkTarget: true })).rejects.toThrow( + /is now a symlink/, + ) + + vi.restoreAllMocks() + // The re-check ran (the walk saw the directory once, the publication once more), the + // commit rename never happened, and the payload did not escape: the target still holds + // its own content and the staged temp was discarded, not left beside it. + expect(parentLooks).toBe(2) + expect(await readFileContent(target)).toEqual({ own: true }) + expect(fsSyncActual.readdirSync(parent)).toEqual(["swap-ancestor.json"]) + }) + + test("refuses the publish when an ancestor is replaced by a different directory", async () => { + const parent = path.join(tempDir, "repl-parent") + await fs.mkdir(parent, { recursive: true }) + const target = path.join(parent, "replaced-ancestor.json") + await fsPromisesActuals.writeFile!(target, JSON.stringify({ own: true })) + const asDir = { isSymbolicLink: () => false, dev: 1n, ino: 1n } as unknown as fsSyncActual.BigIntStats + // The rename-away-and-recreate shape: the name is unchanged and the entry is still a + // directory, so only the identity tells the two apart. + const asOtherDir = { isSymbolicLink: () => false, dev: 9n, ino: 9n } as unknown as fsSyncActual.BigIntStats + let parentLooks = 0 + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + if (String(p) === parent) { + parentLooks++ + return Promise.resolve(parentLooks === 1 ? asDir : asOtherDir) + } + return Promise.resolve(asDir) + }) as unknown as typeof fs.lstat) + + await expect(safeWriteJson(target, { leaked: true }, { refuseSymlinkTarget: true })).rejects.toThrow( + /is no longer the directory that was checked/, + ) + + vi.restoreAllMocks() + expect(parentLooks).toBe(2) + expect(await readFileContent(target)).toEqual({ own: true }) + expect(fsSyncActual.readdirSync(parent)).toEqual(["replaced-ancestor.json"]) + }) + + test("refuses the publish when an ancestor disappears before the commit", async () => { + const parent = path.join(tempDir, "gone-parent") + await fs.mkdir(parent, { recursive: true }) + const target = path.join(parent, "gone-ancestor.json") + await fsPromisesActuals.writeFile!(target, JSON.stringify({ own: true })) + const asDir = { isSymbolicLink: () => false, dev: 1n, ino: 1n } as unknown as fsSyncActual.BigIntStats + // The walk authorizes the directory; by publication time it is gone, so a path-based + // rename would resolve an ancestry nobody checked. The guard must fail closed. + let parentLooks = 0 + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + if (String(p) === parent) { + parentLooks++ + if (parentLooks === 1) return Promise.resolve(asDir) + return Promise.reject(Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" })) + } + return Promise.resolve(asDir) + }) as unknown as typeof fs.lstat) + + await expect(safeWriteJson(target, { leaked: true }, { refuseSymlinkTarget: true })).rejects.toThrow( + /no longer exists/, + ) + + vi.restoreAllMocks() + expect(await readFileContent(target)).toEqual({ own: true }) + expect(fsSyncActual.readdirSync(parent)).toEqual(["gone-ancestor.json"]) + }) + + test("fails closed when an ancestor re-check fails for a non-ENOENT reason", async () => { + const parent = path.join(tempDir, "eacces-parent") + await fs.mkdir(parent, { recursive: true }) + const target = path.join(parent, "eacces-ancestor.json") + await fsPromisesActuals.writeFile!(target, JSON.stringify({ own: true })) + const asDir = { isSymbolicLink: () => false, dev: 1n, ino: 1n } as unknown as fsSyncActual.BigIntStats + const failure = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + let parentLooks = 0 + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + if (String(p) === parent) { + parentLooks++ + if (parentLooks === 1) return Promise.resolve(asDir) + return Promise.reject(failure) + } + return Promise.resolve(asDir) + }) as unknown as typeof fs.lstat) + + // A stat failure says nothing about whether the entry is safe: the original error + // propagates instead of publishing blind. + await expect(safeWriteJson(target, { leaked: true }, { refuseSymlinkTarget: true })).rejects.toThrow(/EACCES/) + + vi.restoreAllMocks() + expect(await readFileContent(target)).toEqual({ own: true }) + }) + + test("fails closed when the destination re-check fails with a non-ENOENT error", async () => { + const parent = path.join(tempDir, "recheck-parent") + await fs.mkdir(parent, { recursive: true }) + const target = path.join(parent, "recheck-eacces.json") + const asDir = { isSymbolicLink: () => false, dev: 1n, ino: 1n } as unknown as fsSyncActual.BigIntStats + const asFile = { isSymbolicLink: () => false } as unknown as fsSyncActual.Stats + const failure = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + // Only ENOENT means "nothing there that could be a link"; any other lstat failure says + // nothing about the destination. The initial check passes and the pre-commit re-check + // is the one that fails, so this covers the publication-time inspection specifically. + let targetLooks = 0 + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + if (String(p) === target) { + targetLooks++ + if (targetLooks >= 2) return Promise.reject(failure) + return Promise.resolve(asFile) + } + return Promise.resolve(asDir) + }) as unknown as typeof fs.lstat) + + await expect(safeWriteJson(target, { written: true }, { refuseSymlinkTarget: true })).rejects.toThrow(/EACCES/) + + vi.restoreAllMocks() + // The pre-commit inspection ran (second look at the destination) and stopped the write. + expect(targetLooks).toBe(2) + expect(await fileExists(target)).toBe(false) + }) + + test("fails closed when the pre-commit publication inspection fails with a non-ENOENT error", async () => { + const parent = path.join(tempDir, "publication-parent") + await fs.mkdir(parent, { recursive: true }) + const target = path.join(parent, "publication-eacces.json") + const asDir = { isSymbolicLink: () => false, dev: 1n, ino: 1n } as unknown as fsSyncActual.BigIntStats + const asFile = { isSymbolicLink: () => false } as unknown as fsSyncActual.Stats + const failure = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + // The first two looks are the pre-staging destination checks; the third is the one the + // publication takes immediately before the commit rename. Rejecting the second, as the + // case above does, cannot show that a publication-time inspection error is honoured, so + // this case rejects the third and keeps the earlier case standing beside it. + let targetLooks = 0 + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + if (String(p) === target) { + targetLooks++ + if (targetLooks >= 3) return Promise.reject(failure) + return Promise.resolve(asFile) + } + return Promise.resolve(asDir) + }) as unknown as typeof fs.lstat) + + await expect(safeWriteJson(target, { written: true }, { refuseSymlinkTarget: true })).rejects.toThrow("EACCES") + + vi.restoreAllMocks() + // Three destination looks happened, so the failure came from the publication-time + // inspection rather than an earlier check, and the commit never landed. + expect(targetLooks).toBe(3) + expect(await fileExists(target)).toBe(false) + }) + + test("does not take the EPERM two-step fallback when the pre-commit ancestry assertion rejects with EPERM", async () => { + const parent = path.join(tempDir, "ancestry-eperm-parent") + await fs.mkdir(parent, { recursive: true }) + const target = path.join(parent, "ancestry-eperm.json") + // A committed destination is what makes the fallback observable: it renames the + // target to a backup before the second rename, so a mistaken fallback moves a file + // the inspection had just refused to publish over. + await fsPromisesActuals.writeFile!(target, JSON.stringify({ committed: true })) + const asDir = { isSymbolicLink: () => false, dev: 1n, ino: 1n } as unknown as fsSyncActual.BigIntStats + const asFile = { isSymbolicLink: () => false } as unknown as fsSyncActual.Stats + const failure = Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) + // The third destination look is the one the publication takes immediately before + // the commit rename, so this arms the ancestry assertion, not an earlier check. + let targetLooks = 0 + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + if (String(p) === target) { + targetLooks++ + if (targetLooks >= 3) return Promise.reject(failure) + return Promise.resolve(asFile) + } + return Promise.resolve(asDir) + }) as unknown as typeof fs.lstat) + const trace = vi.fn() + + await expect(safeWriteJson(target, { fresh: true }, { refuseSymlinkTarget: true, trace })).rejects.toThrow( + "EPERM", + ) + + vi.restoreAllMocks() + // The inspection ran and refused. The committed destination is still there, byte for + // byte, which is only true if the two-step replacement never started: an EPERM from an + // inspection is not a rename collision and must not move anything. + expect(targetLooks).toBe(3) + expect(await readFileContent(target)).toEqual({ committed: true }) + expect(trace).not.toHaveBeenCalledWith(expect.objectContaining({ stage: "backup-rename" })) + }) + + test("fails closed when an ancestor vanishes during the authorizing walk", async () => { + const parent = path.join(tempDir, "vanish-parent") + await fs.mkdir(parent, { recursive: true }) + const target = path.join(parent, "vanish-during-walk.json") + await fsPromisesActuals.writeFile!(target, JSON.stringify({ own: true })) + const asFile = { isSymbolicLink: () => false } as unknown as fsSyncActual.Stats + // A directory that vanishes mid-walk has no recorded identity for the pre-commit + // re-check, so a path that reappears as a symlink before staging would be followed by + // the path-based rename. The walk must fail closed instead of recording a partial chain. + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + if (String(p) === parent) { + return Promise.reject(Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" })) + } + return Promise.resolve(asFile) + }) as unknown as typeof fs.lstat) + + await expect(safeWriteJson(target, { written: true }, { refuseSymlinkTarget: true })).rejects.toThrow( + /does not exist, so the destination ancestry cannot be verified/, + ) + + vi.restoreAllMocks() + // Nothing was published: the existing target keeps its content. + expect(await readFileContent(target)).toEqual({ own: true }) + }) + + test("publishes when every recorded ancestor keeps its identity", async () => { + const parent = path.join(tempDir, "stable-parent") + await fs.mkdir(parent, { recursive: true }) + const target = path.join(parent, "stable-ancestor.json") + await fsPromisesActuals.writeFile!(target, JSON.stringify({ own: true })) + // Pinning must not reject the ordinary case: every ancestor keeps the identity the walk + // recorded, so the publication proceeds. + vi.spyOn(fs, "lstat").mockResolvedValue({ + isSymbolicLink: () => false, + dev: 1n, + ino: 1n, + } as unknown as fsSyncActual.BigIntStats) + + await safeWriteJson(target, { written: true }, { refuseSymlinkTarget: true }) + + vi.restoreAllMocks() + expect(await readFileContent(target)).toEqual({ written: true }) + }) + + test("does not apply the ancestor refusal when refuseSymlinkTarget is not set", async () => { + const target = path.join(tempDir, "default-ancestor.json") + await fsPromisesActuals.writeFile!(target, JSON.stringify({ own: true })) + const asLink = { isSymbolicLink: () => true } as unknown as fsSyncActual.Stats + const asFile = { isSymbolicLink: () => false } as unknown as fsSyncActual.Stats + // The ancestor walk is part of the credential-write policy, not a global behavior change: + // callers that did not opt into refuseSymlinkTarget keep their previous semantics. + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + return Promise.resolve(String(p) === tempDir ? asLink : asFile) + }) as unknown as typeof fs.lstat) + + await safeWriteJson(target, { written: true }) + + vi.restoreAllMocks() + expect(await readFileContent(target)).toEqual({ written: true }) + }) + + test("fails closed when an ancestor directory cannot be inspected", async () => { + const target = path.join(tempDir, "ancestor-eacces.json") + await fsPromisesActuals.writeFile!(target, JSON.stringify({ own: true })) + const asFile = { isSymbolicLink: () => false } as unknown as fsSyncActual.Stats + const failure = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + // "Could not inspect" is not evidence that the path is safe: only ENOENT is tolerated. + vi.spyOn(fs, "lstat").mockImplementation(((p: unknown) => { + if (String(p) === tempDir) { + return Promise.reject(failure) + } + return Promise.resolve(asFile) + }) as unknown as typeof fs.lstat) + + await expect(safeWriteJson(target, { leaked: true }, { refuseSymlinkTarget: true })).rejects.toThrow(/EACCES/) + + vi.restoreAllMocks() + expect(await readFileContent(target)).toEqual({ own: true }) + }) + + test("does not resolve the publish target when refuseSymlinkTarget is set", async () => { + const target = path.join(tempDir, "no-resolve.json") + // Resolving is what turns a planted link into a destination the caller never chose, + // so the credential-bearing path skips it entirely and publishes onto the name the + // caller gave. + const realpath = vi.spyOn(fs, "realpath").mockImplementation(async (p) => String(p)) + + await safeWriteJson(target, { written: true }, { refuseSymlinkTarget: true }) + + expect(realpath).not.toHaveBeenCalled() + vi.restoreAllMocks() + expect(await readFileContent(target)).toEqual({ written: true }) + }) +}) diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index e54b8c51d3..f9073dcd64 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -3,6 +3,8 @@ import * as fsSync from "fs" import * as path from "path" import { JsonStreamStringify } from "json-stream-stringify" +import { resolvePublishTarget } from "../services/file-safety/safeWriteText" + import { getErrorCode } from "./errorCode" import { acquireFileLock } from "./fileLock" import { recoverInterruptedReplacement } from "./jsonFileRecovery" @@ -46,6 +48,39 @@ export interface SafeWriteJsonOptions { * to emit diagnostic trace lines; it never influences the write itself. */ trace?: SafeWriteJsonTrace + + /** + * Refuse to publish through a symlink at the target path. + * + * Two things differ by default and should not be conflated: the advisory LOCK is keyed to + * the resolved referent, which is what keeps every alias of one file behind a single lock, + * while the PUBLISH stays on the path the caller named - the commit is a rename, and a + * rename replaces that directory entry, so a symlink there is replaced rather than followed + * and its referent never receives the payload. + * That is the wrong default for a payload whose destination the user chose - + * settings exports carry API credentials - where following a link they never + * pointed at would write secrets into a file they did not pick. When this is + * set, a symlink at the final path component is an error instead. + * + * The refusal is not a one-time inspection: the final entry and every ancestor + * directory identity are re-derived inside the publication, immediately before + * the commit rename, so a directory swapped for a link during the staging + * sequence is caught before it can redirect the payload. Node exposes no + * handle-relative rename, so the window that remains is the commit rename + * itself, not the checks before it. + */ + refuseSymlinkTarget?: boolean +} + +/** + * An ancestor directory that passed the symlink refusal, recorded with the + * bigint device and inode it had at that moment so the publication can tell + * whether it is still the same directory before committing. + */ +interface CheckedDirectory { + dir: string + dev: bigint + ino: bigint } function createTempFilePath(absoluteFilePath: string, kind: "new" | "bak"): string { @@ -62,6 +97,9 @@ function createTempFilePath(absoluteFilePath: string, kind: "new" | "bak"): stri * - Recovers an interrupted previous write before touching the file: restores an orphaned backup when the canonical path is absent, and removes leftover temp files. * - Writes to a temporary file first, then commits it with a single atomic rename, so the committed target stays readable through an interrupted write. * - When a single rename over the target fails with EPERM (a reader holds the file open on Windows), falls back to backing the target up before replacing it. + * - With `refuseSymlinkTarget`, refuses a publish whose final path component or any + * ancestor directory is (or becomes) a symlink, re-deriving the refusal immediately before + * the commit rename. * - Attempts to roll back and clean up in case of errors. * - Supports pretty-printing with indentation while maintaining streaming efficiency. * @@ -90,6 +128,174 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit throw dirError } + // Every existing ancestor must be checked too, not just the final component: a symlinked + // directory above the target redirects the payload without leaving a trace on the target path + // itself (e.g. /.roo -> a directory outside the workspace). Only ENOENT stops the + // walk - a missing ancestor means nothing deeper exists to be a link. Any other inspection + // error fails closed, because "could not inspect" is not evidence that the path is safe. + // Each directory that passes is recorded with its bigint device and inode: a walk the + // publication does not re-derive is a refusal an attacker retires by swapping the directory + // after the walk ran, so the identities feed the pre-commit re-check below. + async function _refuseSymlinkedAncestors(absoluteFilePath: string): Promise { + const checked: CheckedDirectory[] = [] + let current = path.dirname(absoluteFilePath) + for (;;) { + let st: fsSync.BigIntStats + try { + st = await fs.lstat(current, { bigint: true }) + } catch (error: unknown) { + const code = + error && typeof error === "object" && "code" in error + ? (error as { code?: string }).code + : undefined + if (code === "ENOENT") { + // Fail closed: a directory that vanishes mid-walk leaves no identity to re-check + // before publication, and a path that reappears as a symlink afterwards would be + // followed by the path-based commit rename. Refuse rather than publish blind. + throw new Error( + `safeWriteJson: refusing to write to ${absoluteFilePath}: ${current} does not exist, so the destination ancestry cannot be verified.`, + ) + } + throw error + } + if (st.isSymbolicLink()) { + throw new Error( + `safeWriteJson: refusing to write to ${absoluteFilePath}: ${current} is a symlink, and the payload would be written outside the directory the caller named.`, + ) + } + checked.push({ dir: current, dev: st.dev, ino: st.ino }) + const parent = path.dirname(current) + if (parent === current) { + return checked + } + current = parent + } + } + + // A credential-bearing payload must not be redirected through a link the user + // never chose: check the final path component before anything is resolved, + // staged, or locked. + let checkedAncestors: CheckedDirectory[] = [] + if (options?.refuseSymlinkTarget) { + let targetStat: fsSync.Stats | undefined + try { + targetStat = await fs.lstat(absoluteFilePath) + } catch (error: unknown) { + const code = + error && typeof error === "object" && "code" in error ? (error as { code?: string }).code : undefined + // Only a missing target means there is no link to refuse. Anything else - + // a permission error on the parent directory, for example - is not evidence + // that the destination is safe to publish into. + if (code !== "ENOENT") { + throw error + } + } + if (targetStat?.isSymbolicLink()) { + throw new Error( + `safeWriteJson: refusing to write through the symlink at ${absoluteFilePath}; the payload would land on its referent instead of the destination the user chose.`, + ) + } + + // The final component alone is not enough: a symlinked ancestor redirects the payload while + // leaving the target path looking ordinary. Checked before anything is resolved, staged or + // locked, and it fails closed on any inspection error that is not ENOENT. The identities it + // records are re-derived by the publication hook below - this walk alone leaves the whole + // staging window open to an ancestor swap. + checkedAncestors = await _refuseSymlinkedAncestors(absoluteFilePath) + } + + // Two paths, for two different jobs: lock identity and publication destination. + // The LOCK is keyed to the resolved referent, because proper-lockfile keys by the path it + // is given: an alias and its referent would otherwise take two locks for one underlying + // file, and a concurrent merge through both aliases could read the same JSON and overwrite + // one update. Resolution happens before the lock for that reason, and resolvePublishTarget + // tolerates a not-yet-existing file (it returns the given path on ENOENT), preserving the + // create-from-absent flow. + // The PUBLISH stays on the caller-named path in both modes: the commit is a rename, and a + // rename replaces the directory entry rather than writing through a link, so a link the + // caller never chose gets replaced instead of receiving the payload. That is the + // pre-existing behaviour - publishing onto the referent instead would be a new default for + // every caller. With refuseSymlinkTarget the named path is also the lock path: resolving it + // here would hand back a referent the caller never chose if a link is planted between the + // refusal check and this resolution, and every such writer keys its lock to the same named + // path, so the lock still serializes all writers to that entry. + const lockTargetPath = options?.refuseSymlinkTarget + ? absoluteFilePath + : await resolvePublishTarget(absoluteFilePath) + const publishTargetPath = absoluteFilePath + + // The refusal above and this resolution are separate syscalls, so a local writer + // could replace the final component with a link in between; the resolved path + // would then describe a destination the caller never chose. Re-check the component + // the caller named - once here and again under the lock before publishing - so the + // refusal stays effective through publication. + const assertFinalComponentNotReplaced = async (stage: string): Promise => { + // Fail closed: only ENOENT (nothing there that could be a link) is tolerated. A lstat + // failing for another reason - EACCES on the parent directory, for example - says + // nothing about whether the entry is safe, so the write stops instead of publishing + // blind through an unexamined destination. + let nowStat: fsSync.Stats | undefined + try { + nowStat = await fs.lstat(absoluteFilePath) + } catch (error: unknown) { + const code = + error && typeof error === "object" && "code" in error ? (error as { code?: string }).code : undefined + if (code !== "ENOENT") { + throw error + } + } + if (nowStat?.isSymbolicLink()) { + throw new Error( + `safeWriteJson: refusing to write through the symlink now at ${absoluteFilePath} (${stage}); the payload would land at ${publishTargetPath}, a destination the caller never chose.`, + ) + } + } + if (options?.refuseSymlinkTarget) { + await assertFinalComponentNotReplaced("after resolution") + } + + // The initial checks and the commit rename are separated by the whole staging sequence, and + // Node exposes no handle-relative rename (no renameat): a local writer that swaps an + // inspected ancestor for a link - or for a different directory - during that window would + // still redirect the path-based commit rename, and no recheck of the final component would + // notice, because the swap is one directory up. The publication therefore re-derives the + // complete ancestry itself - the final entry and every recorded ancestor identity - + // immediately before the commit rename, so the refusal is enforced at the action it guards. + // What cannot be closed without a native renameat is stated rather than claimed as + // protection: the window that remains is the commit rename itself. + const assertAncestryUnchangedBeforeCommit = async (): Promise => { + await assertFinalComponentNotReplaced("before publication") + for (const expected of checkedAncestors) { + let nowStat: fsSync.BigIntStats + try { + nowStat = await fs.lstat(expected.dir, { bigint: true }) + } catch (error: unknown) { + const code = + error && typeof error === "object" && "code" in error + ? (error as { code?: string }).code + : undefined + // The directory the walk recorded is gone: the ancestry the refusal was based on + // no longer exists, and a path-based rename would resolve a chain nobody checked. + if (code === "ENOENT") { + throw new Error( + `safeWriteJson: refusing to publish to ${absoluteFilePath}: ${expected.dir} no longer exists; an ancestor of the destination was replaced after it was checked.`, + ) + } + throw error + } + if (nowStat.isSymbolicLink()) { + throw new Error( + `safeWriteJson: refusing to publish to ${absoluteFilePath}: ${expected.dir} is now a symlink; the payload would be written outside the directory the caller named.`, + ) + } + if (nowStat.dev !== expected.dev || nowStat.ino !== expected.ino) { + throw new Error( + `safeWriteJson: refusing to publish to ${absoluteFilePath}: ${expected.dir} is no longer the directory that was checked; an ancestor of the destination was replaced after it was checked.`, + ) + } + } + } + // Acquire the lock before any file operations. `acquireFileLock` owns the // shared advisory lock protocol, so callers that lock the same path with // it (for example task-history deletion) serialize with this write. @@ -97,7 +303,7 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit // remains a no-op, so the finally block in the main file operations // try-catch-finally won't try to release an unacquired lock if this // path is taken. - releaseLock = await acquireFileLock(absoluteFilePath) + releaseLock = await acquireFileLock(lockTargetPath) // Variables to hold the actual paths of temp files if they are created. let actualTempNewFilePath: string | null = null @@ -122,7 +328,7 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit if (options?.merge) { let existing: unknown = null try { - existing = JSON.parse(await fs.readFile(absoluteFilePath, "utf8")) + existing = JSON.parse(await fs.readFile(publishTargetPath, "utf8")) } catch (error: unknown) { const code = getErrorCode(error) if (!(error instanceof SyntaxError) && code !== "ENOENT") { @@ -137,13 +343,22 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit options?.trace?.({ stage: "write-start" }) actualTempNewFilePath = createTempFilePath(absoluteFilePath, "new") - await _streamDataToFile(actualTempNewFilePath, data, options?.prettyPrint) + await _streamDataToFile(actualTempNewFilePath, data, publishTargetPath, options?.prettyPrint) 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 // canonical path absent. + if (options?.refuseSymlinkTarget) { + // Re-derive the refusal inside the publication, immediately before the commit rename, but + // outside the rename-fallback try: that handler tests only the error code, so an EPERM + // raised by this inspection would be mistaken for a rename collision and take the + // two-step replacement, moving the committed destination to a backup even though the + // refusal promises that nothing moves. Only rename failures reach the fallback now; + // the fallback and the crash-recovery pass at the start of the next write are unchanged. + await assertAncestryUnchangedBeforeCommit() + } try { options?.trace?.({ stage: "replacement-rename" }) await fs.rename(actualTempNewFilePath, absoluteFilePath) @@ -161,6 +376,10 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit options?.trace?.({ stage: "backup-rename", detail: "eperm-fallback" }) actualTempBackupFilePath = createTempFilePath(absoluteFilePath, "bak") await fs.rename(absoluteFilePath, actualTempBackupFilePath) + if (options?.refuseSymlinkTarget) { + // The fallback's second rename is the same commit action, so it gets the same pre-commit re-check. + await assertAncestryUnchangedBeforeCommit() + } options?.trace?.({ stage: "replacement-rename", detail: "eperm-fallback" }) await fs.rename(actualTempNewFilePath, absoluteFilePath) } @@ -230,7 +449,7 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit await releaseLock() } catch (unlockError) { // Do not re-throw here, as the originalError from the try/catch (if any) is more important. - console.error(`Failed to release lock for ${absoluteFilePath}:`, unlockError) + console.error(`Failed to release lock for ${lockTargetPath}:`, unlockError) } } } @@ -239,12 +458,52 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit * Helper function to stream JSON data to a file. * @param targetPath The path to write the stream to. * @param data The data to stream. + * @param preserveModeOf The existing file whose mode the staged file must inherit. * @param prettyPrint Whether to format the JSON with indentation. * @returns Promise */ -async function _streamDataToFile(targetPath: string, data: unknown, prettyPrint = false): Promise { +async function _streamDataToFile( + targetPath: string, + data: unknown, + preserveModeOf: string, + prettyPrint = false, +): Promise { // Stream data to avoid high memory usage for large JSON objects. - const fileWriteStream = fsSync.createWriteStream(targetPath, { encoding: "utf8" }) + // The staged payload can carry API credentials and the staging path is generated inside a + // directory the workspace already controls, so it is opened exclusively: a file a local + // writer planted at that name fails the write with EEXIST instead of receiving the payload, + // and a staging path left by a crashed writer is not silently overwritten. The creation mode + // comes from the file being replaced, so the commit rename can neither widen a 0o600 target + // nor narrow a shared one - the same rule src/services/file-safety/safeWriteText.ts applies to + // its own staging file, with 0o644 as the default for a target that does not exist yet. + let stageMode = 0o644 + let targetExisted = true + try { + stageMode = fsSync.statSync(preserveModeOf).mode & 0o777 + } catch { + // No target yet: the default below leaves the process umask in charge, as before. + targetExisted = false + } + const stagedFd = fsSync.openSync(targetPath, "wx", stageMode) + if (targetExisted) { + // openSync applies the process umask to the requested mode, so a 0o664 or 0o666 target + // would be staged as 0o644 under the common umask 022 and lose group write through the + // rename. Set the mode on the descriptor instead. A filesystem that refuses chmod keeps + // the creation mode, which is no wider than what this writer published before. + try { + fsSync.fchmodSync(stagedFd, stageMode) + } catch { + // best-effort mode alignment; the write itself is not blocked + } + } + let fileWriteStream: fsSync.WriteStream + try { + // The stream owns the descriptor: it closes it on finish and on error. + fileWriteStream = fsSync.createWriteStream(targetPath, { fd: stagedFd, encoding: "utf8" }) + } catch (streamError) { + fsSync.closeSync(stagedFd) + throw streamError + } // JsonStreamStringify traverses the object and streams tokens directly // The 'spaces' parameter adds indentation during streaming, not via a separate pass