diff --git a/apps/vscode-e2e/src/suite/restart-persistence.test.ts b/apps/vscode-e2e/src/suite/restart-persistence.test.ts index 699b64e9cc..0d508273e3 100644 --- a/apps/vscode-e2e/src/suite/restart-persistence.test.ts +++ b/apps/vscode-e2e/src/suite/restart-persistence.test.ts @@ -54,6 +54,13 @@ async function runCreate(api: RooCodeAPI): Promise { await waitUntilCompleted({ api, taskId }) assert.strictEqual(sawMarker, true, `Completion should include ${MARKER}`) + // TaskCompleted fires before the final saveClineMessages() write is + // necessarily durable on disk. Wait until the completion marker is + // persisted in ui_messages.json before quitting, so the verify phase + // never reads a task directory whose final write has not landed yet. + const createdTaskId: string = taskId + await waitFor(() => api.hasTaskUiMessageText(createdTaskId, MARKER)) + const result: PhaseResult = { version: PHASE_RESULT_VERSION, phase: "create", diff --git a/packages/types/src/api.ts b/packages/types/src/api.ts index de23f67491..e64406fa9c 100644 --- a/packages/types/src/api.ts +++ b/packages/types/src/api.ts @@ -68,6 +68,15 @@ export interface RooCodeAPI extends EventEmitter { taskId: string, sequence: TaskApiConversationHistorySequence, ): Promise + /** + * Checks whether the persisted ui_messages.json for a task contains a + * message with the given text. Reads from disk, so it only returns true + * once the final write is durable. Intended for use in tests only. + * @param taskId The ID of the task. + * @param text The text to look for in persisted UI messages. + * @returns True when a persisted UI message contains the text, or false if unavailable. + */ + hasTaskUiMessageText(taskId: string, text: string): Promise /** * Returns the current task stack. * @returns An array of task IDs. diff --git a/src/core/task-persistence/__tests__/taskMessages.spec.ts b/src/core/task-persistence/__tests__/taskMessages.spec.ts index e494b5b594..6b968fe96c 100644 --- a/src/core/task-persistence/__tests__/taskMessages.spec.ts +++ b/src/core/task-persistence/__tests__/taskMessages.spec.ts @@ -20,6 +20,7 @@ vi.mock("../../../utils/safeWriteJson", () => ({ // Import after mocks import { saveTaskMessages, readTaskMessages } from "../taskMessages" +import { formatErrorDetail } from "../readFileWithMissingRetry" let tmpBaseDir: string @@ -99,6 +100,23 @@ describe("taskMessages.saveTaskMessages", () => { expect.objectContaining({ ts: 2, text: "incoming" }), ]) }) + + it("propagates a forced write failure without swallowing or replacing it", async () => { + // Fault injection: simulate an interrupted/failed final write (e.g. the + // extension host quitting mid-save or the disk refusing the write). The + // save must reject with the original error so callers never treat the + // messages as durably persisted, and the underlying code stays visible. + const writeFailure = Object.assign(new Error("no space left on device"), { code: "ENOSPC" }) + hoisted.safeWriteJsonMock.mockRejectedValueOnce(writeFailure) + + await expect( + saveTaskMessages({ + messages: [{ ts: 1, type: "say", say: "text", text: "unsaved" }], + taskId: "task-write-failure", + globalStoragePath: tmpBaseDir, + }), + ).rejects.toBe(writeFailure) + }) }) describe("taskMessages.readTaskMessages", () => { @@ -151,24 +169,63 @@ describe("taskMessages.readTaskMessages", () => { expect(hoisted.readFileMock).toHaveBeenCalledTimes(2) }) - it("throws when the missing-file retry also fails", async () => { + it("throws with the underlying error code when the missing-file retry also fails", async () => { vi.spyOn(Math, "random").mockReturnValue(0) const missing = Object.assign(new Error("missing"), { code: "ENOENT" }) hoisted.readFileMock.mockRejectedValue(missing) - await expect( - readTaskMessages({ taskId: "task-still-missing", globalStoragePath: tmpBaseDir }), - ).rejects.toMatchObject({ kind: "not_found" }) + const error = await readTaskMessages({ taskId: "task-still-missing", globalStoragePath: tmpBaseDir }).catch( + (caught: unknown) => caught, + ) + expect(error).toMatchObject({ + kind: "not_found", + originalError: missing, + message: expect.stringContaining("(code: ENOENT)"), + }) expect(hoisted.readFileMock).toHaveBeenCalledTimes(2) }) - it("does not retry non-ENOENT read failures", async () => { + it("does not retry non-ENOENT read failures and surfaces the underlying code", async () => { const denied = Object.assign(new Error("denied"), { code: "EACCES" }) hoisted.readFileMock.mockRejectedValueOnce(denied) - await expect(readTaskMessages({ taskId: "task-denied", globalStoragePath: tmpBaseDir })).rejects.toMatchObject({ - kind: "io_error", - }) + const error = await readTaskMessages({ taskId: "task-denied", globalStoragePath: tmpBaseDir }).catch( + (caught: unknown) => caught, + ) + expect(error).toMatchObject({ kind: "io_error" }) + expect((error as Error).message).toContain("(code: EACCES)") expect(hoisted.readFileMock).toHaveBeenCalledTimes(1) }) + + it("surfaces the parse error detail for a torn (half-written) file", async () => { + // Fault injection: a truncated/torn write leaves invalid JSON on disk. + // The read error must identify the parse failure (not a missing file) + // so a torn write is distinguishable from a late write in the logs. + const taskId = "task-torn-write" + const taskDir = path.join(tmpBaseDir, "tasks", taskId) + await fs.mkdir(taskDir, { recursive: true }) + await fs.writeFile(path.join(taskDir, "ui_messages.json"), '[{"ts":1,"type":"sa', "utf8") + + const error = await readTaskMessages({ taskId, globalStoragePath: tmpBaseDir }).catch( + (caught: unknown) => caught, + ) + expect(error).toMatchObject({ kind: "invalid" }) + expect((error as Error).message).toContain("SyntaxError") + }) +}) + +describe("readFileWithMissingRetry formatErrorDetail", () => { + it("prefers the filesystem error code when present", () => { + expect(formatErrorDetail(Object.assign(new Error("missing"), { code: "ENOENT" }))).toBe(" (code: ENOENT)") + }) + + it("falls back to the error name and message", () => { + expect(formatErrorDetail(new SyntaxError("Unexpected end of JSON input"))).toBe( + " (SyntaxError: Unexpected end of JSON input)", + ) + }) + + it("stringifies non-error causes", () => { + expect(formatErrorDetail("boom")).toBe(" (boom)") + }) }) diff --git a/src/core/task-persistence/readFileWithMissingRetry.ts b/src/core/task-persistence/readFileWithMissingRetry.ts index d3369fb44c..1036049470 100644 --- a/src/core/task-persistence/readFileWithMissingRetry.ts +++ b/src/core/task-persistence/readFileWithMissingRetry.ts @@ -1,5 +1,8 @@ import * as fs from "fs/promises" +// safeWriteJson replaces a file in two renames (target to backup, new file to +// target) because a single rename over an open file fails with EPERM on +// Windows. A reader can see ENOENT between the two renames, so retry once. const READ_RETRY_MIN_MS = 1 const READ_RETRY_MAX_MS = 10 const READ_RETRY_RANGE_MS = READ_RETRY_MAX_MS - READ_RETRY_MIN_MS + 1 @@ -10,6 +13,24 @@ export function getErrorCode(error: unknown): string | undefined { : undefined } +/** + * Formats the actionable detail of an underlying error for inclusion in + * wrapping error messages: the filesystem error code when present (e.g. + * ENOENT vs EACCES), otherwise the error name and message (e.g. SyntaxError + * for a torn/half-written JSON file). Keeps wrappers such as + * TaskMessagesReadError diagnosable without logging the full cause chain. + */ +export function formatErrorDetail(error: unknown): string { + const code = getErrorCode(error) + if (code) { + return ` (code: ${code})` + } + if (error instanceof Error) { + return ` (${error.name}: ${error.message})` + } + return ` (${String(error)})` +} + export async function readFileWithMissingRetry(filePath: string): Promise { try { return await fs.readFile(filePath, "utf8") diff --git a/src/core/task-persistence/taskMessages.ts b/src/core/task-persistence/taskMessages.ts index 89ec372c61..b67ad2b7f8 100644 --- a/src/core/task-persistence/taskMessages.ts +++ b/src/core/task-persistence/taskMessages.ts @@ -6,7 +6,7 @@ import { safeWriteJson } from "../../utils/safeWriteJson" import { GlobalFileNames } from "../../shared/globalFileNames" import { getTaskDirectoryPath } from "../../utils/storage" import { ensureMessageIdentifiers, mergeClineMessageSnapshots } from "./mergeMessageSnapshots" -import { getErrorCode, readFileWithMissingRetry } from "./readFileWithMissingRetry" +import { formatErrorDetail, getErrorCode, readFileWithMissingRetry } from "./readFileWithMissingRetry" export type TaskMessagesReadErrorKind = "not_found" | "invalid" | "io_error" @@ -38,14 +38,22 @@ export async function readTaskMessages({ fileContent = await readFileWithMissingRetry(filePath) } catch (error) { const kind = getErrorCode(error) === "ENOENT" ? "not_found" : "io_error" - throw new TaskMessagesReadError(kind, `Failed to read task messages for ${taskId} at ${filePath}`, error) + throw new TaskMessagesReadError( + kind, + `Failed to read task messages for ${taskId} at ${filePath}${formatErrorDetail(error)}`, + error, + ) } let parsedData: unknown try { parsedData = JSON.parse(fileContent) } catch (error) { - throw new TaskMessagesReadError("invalid", `Failed to parse task messages for ${taskId} at ${filePath}`, error) + throw new TaskMessagesReadError( + "invalid", + `Failed to parse task messages for ${taskId} at ${filePath}${formatErrorDetail(error)}`, + error, + ) } if (!Array.isArray(parsedData)) { diff --git a/src/extension/__tests__/api-has-task-ui-message-text.spec.ts b/src/extension/__tests__/api-has-task-ui-message-text.spec.ts new file mode 100644 index 0000000000..697a2e0809 --- /dev/null +++ b/src/extension/__tests__/api-has-task-ui-message-text.spec.ts @@ -0,0 +1,88 @@ +import { describe, it, expect, vi, beforeEach } from "vitest" +import * as vscode from "vscode" + +import { API } from "../api" +import { ClineProvider } from "../../core/webview/ClineProvider" +import { readTaskMessages, TaskMessagesReadError } from "../../core/task-persistence/taskMessages" +import { makeClineProviderFactory } from "../../test-utils/provider" + +vi.mock("vscode") +vi.mock("../../core/webview/ClineProvider") +vi.mock("../../core/task-persistence/taskMessages", async (importOriginal) => ({ + ...(await importOriginal()), + readTaskMessages: vi.fn(), +})) + +describe("API#hasTaskUiMessageText", () => { + let api: API + let mockProvider: ClineProvider + + beforeEach(() => { + vi.mocked(readTaskMessages).mockReset() + + // API logging only needs appendLine in this suite; a full OutputChannel fake would obscure the tested contract. + const mockOutputChannel = { + appendLine: vi.fn(), + } as unknown as vscode.OutputChannel + + // The method under test only reads contextProxy.globalStorageUri; the + // remaining members exist so the API constructor's listener wiring works. + mockProvider = { + context: {} as vscode.ExtensionContext, + contextProxy: { globalStorageUri: { fsPath: "/mock/global/storage" } }, + taskHistoryStore: { get: vi.fn() }, + getTaskWithId: vi.fn().mockResolvedValue({}), + on: vi.fn(), + } as unknown as ClineProvider + + api = new API(mockOutputChannel, mockProvider, makeClineProviderFactory()) + }) + + it("returns true when a persisted ui message contains the text", async () => { + vi.mocked(readTaskMessages).mockResolvedValue([ + { ts: 1, type: "say", say: "text", text: "done RESTART_PERSISTENCE_MARKER" }, + ]) + + await expect(api.hasTaskUiMessageText("task-1", "RESTART_PERSISTENCE_MARKER")).resolves.toBe(true) + expect(readTaskMessages).toHaveBeenCalledWith({ taskId: "task-1", globalStoragePath: "/mock/global/storage" }) + }) + + it("returns false when no persisted ui message contains the text", async () => { + vi.mocked(readTaskMessages).mockResolvedValue([{ ts: 1, type: "say", say: "text", text: "unrelated" }]) + + await expect(api.hasTaskUiMessageText("task-1", "RESTART_PERSISTENCE_MARKER")).resolves.toBe(false) + }) + + it("finds the text in a later message and skips messages without text", async () => { + vi.mocked(readTaskMessages).mockResolvedValue([ + { ts: 1, type: "say", say: "api_req_started" }, + { ts: 2, type: "say", say: "text", text: "unrelated" }, + { ts: 3, type: "say", say: "completion_result", text: "done RESTART_PERSISTENCE_MARKER" }, + ]) + + await expect(api.hasTaskUiMessageText("task-1", "RESTART_PERSISTENCE_MARKER")).resolves.toBe(true) + }) + + it("returns false when only messages without text exist", async () => { + vi.mocked(readTaskMessages).mockResolvedValue([{ ts: 1, type: "say", say: "api_req_started" }]) + + await expect(api.hasTaskUiMessageText("task-1", "RESTART_PERSISTENCE_MARKER")).resolves.toBe(false) + }) + + it.each(["not_found", "invalid", "io_error"] as const)( + "returns false instead of throwing on a %s read failure", + async (kind) => { + // The completion write may not have landed yet; callers poll until it does. + vi.mocked(readTaskMessages).mockRejectedValue(new TaskMessagesReadError(kind, `read failed: ${kind}`)) + + await expect(api.hasTaskUiMessageText("task-1", "RESTART_PERSISTENCE_MARKER")).resolves.toBe(false) + }, + ) + + it("returns false without reading disk for a task id that is not in history", async () => { + vi.mocked(mockProvider.getTaskWithId).mockRejectedValue(new Error("Task not found")) + + await expect(api.hasTaskUiMessageText("../../escape", "RESTART_PERSISTENCE_MARKER")).resolves.toBe(false) + expect(readTaskMessages).not.toHaveBeenCalled() + }) +}) diff --git a/src/extension/api.ts b/src/extension/api.ts index 78a9496a3f..6a8ed8dfdd 100644 --- a/src/extension/api.ts +++ b/src/extension/api.ts @@ -28,6 +28,7 @@ import { Package } from "../shared/package" import type { Mode } from "../shared/modes" import type { ClineProvider } from "../core/webview/ClineProvider" import type { ClineProviderFactory } from "../core/webview/ClineProviderFactory" +import { readTaskMessages } from "../core/task-persistence/taskMessages" import { Terminal } from "../integrations/terminal/Terminal" import { TerminalRegistry } from "../integrations/terminal/TerminalRegistry" import { getCommands } from "../services/command/commands" @@ -305,6 +306,22 @@ export class API extends EventEmitter implements RooCodeAPI { ) } + /** Checks persisted ui_messages.json contents without exposing messages to tests. */ + public async hasTaskUiMessageText(taskId: string, text: string): Promise { + let uiMessages: Awaited> + try { + // Reject unknown ids first: readTaskMessages creates the task directory. + await this.sidebarProvider.getTaskWithId(taskId) + uiMessages = await readTaskMessages({ + taskId, + globalStoragePath: this.sidebarProvider.contextProxy.globalStorageUri.fsPath, + }) + } catch { + return false + } + return uiMessages.some((message) => message.text?.includes(text)) + } + public getCurrentTaskStack() { return this.sidebarProvider.getCurrentTaskStack() }