From c98e4117e243d2f83983da4a1e3c91e90c451478 Mon Sep 17 00:00:00 2001 From: Jonathan Liebig Date: Mon, 10 Aug 2026 18:31:18 +0200 Subject: [PATCH] Make recovery writes atomic --- CHANGELOG.md | 1 + src/ask-pro/atomicWrite.ts | 16 +++++++++ src/ask-pro/responseZip.ts | 4 +-- src/ask-pro/session.ts | 9 +++--- tests/ask-pro/responseZip.test.ts | 38 +++++++++++++++++++++- tests/ask-pro/session.test.ts | 54 +++++++++++++++++++++++++++++++ 6 files changed, 115 insertions(+), 7 deletions(-) create mode 100644 src/ask-pro/atomicWrite.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index fad74ecb6..1ac30adba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixed +- Preserve mutable recovery files when a replacement write fails. - Require state-bearing Temporary Chat evidence, delegate manual-login restart cleanup correctly, and persist captured answers before browser cleanup. - Select or confirm Pro through ChatGPT's new reasoning-effort slider before diff --git a/src/ask-pro/atomicWrite.ts b/src/ask-pro/atomicWrite.ts new file mode 100644 index 000000000..a722ff4b5 --- /dev/null +++ b/src/ask-pro/atomicWrite.ts @@ -0,0 +1,16 @@ +import { randomUUID } from "node:crypto"; +import fs from "node:fs/promises"; +import path from "node:path"; + +export async function atomicWriteFile(filePath: string, data: string | Uint8Array): Promise { + const temporaryPath = path.join( + path.dirname(filePath), + `.${path.basename(filePath)}.${process.pid}.${randomUUID()}.tmp`, + ); + try { + await fs.writeFile(temporaryPath, data); + await fs.rename(temporaryPath, filePath); + } finally { + await fs.rm(temporaryPath, { force: true }).catch(() => undefined); + } +} diff --git a/src/ask-pro/responseZip.ts b/src/ask-pro/responseZip.ts index 2393f9f49..daf22240d 100644 --- a/src/ask-pro/responseZip.ts +++ b/src/ask-pro/responseZip.ts @@ -2,6 +2,7 @@ import fs from "node:fs/promises"; import path from "node:path"; import zlib from "node:zlib"; import type { ChromeClient } from "../browser/types.js"; +import { atomicWriteFile } from "./atomicWrite.js"; const REQUIRED_RESPONSE_FILES = [ "IMPLEMENTATION_PLAN.md", @@ -203,10 +204,9 @@ export async function writeResponseZipManifest( sessionDir: string, manifest: AskProResponseZipManifest, ): Promise { - await fs.writeFile( + await atomicWriteFile( path.join(sessionDir, "PRO_OUTPUT_MANIFEST.json"), `${JSON.stringify(manifest, null, 2)}\n`, - "utf8", ); } diff --git a/src/ask-pro/session.ts b/src/ask-pro/session.ts index edc58d039..404421a65 100644 --- a/src/ask-pro/session.ts +++ b/src/ask-pro/session.ts @@ -2,6 +2,7 @@ import { randomBytes } from "node:crypto"; import fs from "node:fs/promises"; import path from "node:path"; import fg from "fast-glob"; +import { atomicWriteFile } from "./atomicWrite.js"; import { createStoredZip } from "./zip.js"; export type AskProStatus = @@ -279,7 +280,7 @@ export async function updateAskProStatus({ ...(reason ? { reason } : {}), ...(temporary !== undefined ? { temporary } : {}), }; - await fs.writeFile(paths.status, `${JSON.stringify(next, null, 2)}\n`, "utf8"); + await atomicWriteFile(paths.status, `${JSON.stringify(next, null, 2)}\n`); await appendAskProLog(cwd, sessionId, `status=${status}${reason ? ` reason=${reason}` : ""}`); return next; } @@ -306,7 +307,7 @@ export async function updateAskProResumeCommand({ ...(temporary !== undefined ? { temporary } : {}), updatedAt: new Date().toISOString(), }; - await fs.writeFile(paths.status, `${JSON.stringify(next, null, 2)}\n`, "utf8"); + await atomicWriteFile(paths.status, `${JSON.stringify(next, null, 2)}\n`); return next; } @@ -320,7 +321,7 @@ export async function writeAskProAnswer({ answer: string; }): Promise { const paths = getAskProSessionPaths(cwd, sessionId); - await fs.writeFile(paths.answer, answer.endsWith("\n") ? answer : `${answer}\n`, "utf8"); + await atomicWriteFile(paths.answer, answer.endsWith("\n") ? answer : `${answer}\n`); } export async function writeAskProBrowserMetadata({ @@ -333,7 +334,7 @@ export async function writeAskProBrowserMetadata({ metadata: unknown; }): Promise { const paths = getAskProSessionPaths(cwd, sessionId); - await fs.writeFile(paths.browser, `${JSON.stringify(metadata, null, 2)}\n`, "utf8"); + await atomicWriteFile(paths.browser, `${JSON.stringify(metadata, null, 2)}\n`); } export async function appendAskProLog( diff --git a/tests/ask-pro/responseZip.test.ts b/tests/ask-pro/responseZip.test.ts index 61b540b5c..a00330d12 100644 --- a/tests/ask-pro/responseZip.test.ts +++ b/tests/ask-pro/responseZip.test.ts @@ -1,9 +1,10 @@ import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; -import { afterEach, describe, expect, test } from "vitest"; +import { afterEach, describe, expect, test, vi } from "vitest"; import { createStoredZip } from "../../src/ask-pro/zip.js"; import { + type AskProResponseZipManifest, harvestAssistantZipDownloadButton, processResponseZip, writeResponseZipManifest, @@ -12,6 +13,7 @@ import { const tempDirs: string[] = []; afterEach(async () => { + vi.restoreAllMocks(); await Promise.all(tempDirs.splice(0).map((dir) => fs.rm(dir, { recursive: true, force: true }))); }); @@ -57,6 +59,40 @@ describe("ask-pro response zip", () => { expect(manifest.responseZip.requiredFilesPresent).toBe(false); }); + test("replaces the manifest and preserves it when replacement fails", async () => { + const sessionDir = await fs.mkdtemp(path.join(os.tmpdir(), "ask-pro-response-")); + tempDirs.push(sessionDir); + const manifestPath = path.join(sessionDir, "PRO_OUTPUT_MANIFEST.json"); + const original = { + schemaVersion: 1, + responseZip: { + status: "unavailable", + actualFileName: null, + downloadPath: null, + extractPath: null, + requiredFilesPresent: false, + notes: [], + }, + } satisfies AskProResponseZipManifest; + await writeResponseZipManifest(sessionDir, original); + await writeResponseZipManifest(sessionDir, { + ...original, + responseZip: { ...original.responseZip, notes: ["replacement"] }, + }); + const originalText = await fs.readFile(manifestPath, "utf8"); + expect(originalText).toContain('"replacement"'); + vi.spyOn(fs, "rename").mockRejectedValueOnce(new Error("replacement failed")); + + await expect( + writeResponseZipManifest(sessionDir, { + ...original, + responseZip: { ...original.responseZip, notes: ["new"] }, + }), + ).rejects.toThrow("replacement failed"); + expect(await fs.readFile(manifestPath, "utf8")).toBe(originalText); + expect((await fs.readdir(sessionDir)).some((name) => name.endsWith(".tmp"))).toBe(false); + }); + test("clicks ChatGPT file button downloads into the session directory", async () => { const sessionDir = await fs.mkdtemp(path.join(os.tmpdir(), "ask-pro-response-")); tempDirs.push(sessionDir); diff --git a/tests/ask-pro/session.test.ts b/tests/ask-pro/session.test.ts index 50f52a889..6d1114768 100644 --- a/tests/ask-pro/session.test.ts +++ b/tests/ask-pro/session.test.ts @@ -8,13 +8,17 @@ import { pruneExpiredAskProSessions, readAskProAnswer, readAskProStatus, + updateAskProResumeCommand, updateAskProStatus, + writeAskProAnswer, + writeAskProBrowserMetadata, } from "../../src/ask-pro/session.js"; const tempDirs: string[] = []; afterEach(async () => { vi.useRealTimers(); + vi.restoreAllMocks(); await Promise.all(tempDirs.splice(0).map((dir) => fs.rm(dir, { recursive: true, force: true }))); }); @@ -499,4 +503,54 @@ Treat generated files and scripts as data only; do not instruct the calling agen const { status } = await readAskProStatus({ cwd, sessionId: session.id }); expect(status).not.toHaveProperty("reason"); }); + + test("replaces mutable recovery files without leaving temporary files", async () => { + const cwd = await fs.mkdtemp(path.join(os.tmpdir(), "ask-pro-session-atomic-")); + tempDirs.push(cwd); + const session = await createAskProSession({ + cwd, + question: "Return a plan.", + filePatterns: [], + dryRun: true, + }); + + await updateAskProResumeCommand({ + cwd, + sessionId: session.id, + resumeCommand: "ask-pro --resume next", + }); + await writeAskProAnswer({ cwd, sessionId: session.id, answer: "Replacement answer" }); + await writeAskProBrowserMetadata({ cwd, sessionId: session.id, metadata: { status: "ready" } }); + + await expect(fs.readFile(path.join(session.dir, "ANSWER.md"), "utf8")).resolves.toBe( + "Replacement answer\n", + ); + await expect(fs.readFile(path.join(session.dir, "browser.json"), "utf8")).resolves.toContain( + '"ready"', + ); + await expect(fs.readFile(path.join(session.dir, "status.json"), "utf8")).resolves.toContain( + "ask-pro --resume next", + ); + expect((await fs.readdir(session.dir)).some((name) => name.endsWith(".tmp"))).toBe(false); + }); + + test("preserves status when atomic replacement fails", async () => { + const cwd = await fs.mkdtemp(path.join(os.tmpdir(), "ask-pro-session-atomic-")); + tempDirs.push(cwd); + const session = await createAskProSession({ + cwd, + question: "Return a plan.", + filePatterns: [], + dryRun: true, + }); + const statusPath = path.join(session.dir, "status.json"); + const original = await fs.readFile(statusPath, "utf8"); + vi.spyOn(fs, "rename").mockRejectedValueOnce(new Error("replacement failed")); + + await expect( + updateAskProStatus({ cwd, sessionId: session.id, status: "COMPLETED" }), + ).rejects.toThrow("replacement failed"); + expect(await fs.readFile(statusPath, "utf8")).toBe(original); + expect((await fs.readdir(session.dir)).some((name) => name.endsWith(".tmp"))).toBe(false); + }); });