From da4f6a08a77595cfae798e76b259ebb41771df3b Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 12 May 2026 08:09:27 +0000 Subject: [PATCH 1/8] fix: rollback artifacts after manifest append failures --- src/screenshot/capture.ts | 6 +++ src/snapshot/capture.ts | 66 ++++++++++++++++------------ test/unit/screenshot/capture.test.ts | 11 ++--- test/unit/snapshot/capture.test.ts | 36 ++++++++++++++- 4 files changed, 84 insertions(+), 35 deletions(-) diff --git a/src/screenshot/capture.ts b/src/screenshot/capture.ts index 3f30f68..2a446ba 100644 --- a/src/screenshot/capture.ts +++ b/src/screenshot/capture.ts @@ -65,6 +65,8 @@ export async function captureScreenshotResult( `.tmp-screenshot-${ulid()}.png`, ); + let renamedArtifactPath: string | undefined; + try { const rendererResult = await options.backend.screenshot( temporaryOutputPath, @@ -134,6 +136,7 @@ export async function captureScreenshotResult( }; await rename(temporaryOutputPath, finalArtifactPath); + renamedArtifactPath = finalArtifactPath; await appendArtifact( options.sessionDir, createArtifactEntry({ @@ -159,6 +162,9 @@ export async function captureScreenshotResult( return publicResult; } catch (error) { await rm(temporaryOutputPath, { force: true }).catch(() => undefined); + if (renamedArtifactPath !== undefined) { + await rm(renamedArtifactPath, { force: true }).catch(() => undefined); + } throw error; } } diff --git a/src/snapshot/capture.ts b/src/snapshot/capture.ts index 3c82efa..e35d68f 100644 --- a/src/snapshot/capture.ts +++ b/src/snapshot/capture.ts @@ -1,3 +1,5 @@ +import { rm } from 'node:fs/promises'; + import type { SnapshotParams, SnapshotResult } from '../protocol/messages.js'; import type { SemanticSnapshot } from '../renderer/types.js'; @@ -126,33 +128,43 @@ export async function persistSnapshotArtifact( ); const snapshotArtifactPath = artifactPath(options.sessionDir, filename); - await writeTextFileAtomic({ - path: snapshotArtifactPath, - pathLabel: 'snapshot artifact path', - contents: `${JSON.stringify(options.result, null, 2)}\n`, - writeErrorMessage: `Failed to write snapshot artifact at ${snapshotArtifactPath}.`, - }); - - await appendArtifact( - options.sessionDir, - createArtifactEntry({ - kind: 'snapshot', - filename, - sessionId: options.snapshot.sessionId, - capturedAtSeq: options.snapshot.capturedAtSeq, - metadata: { - format: options.format, - rendererBackend: options.rendererBackend, - cols: options.snapshot.cols, - rows: options.snapshot.rows, - cursorRow: options.snapshot.cursorRow, - cursorCol: options.snapshot.cursorCol, - ...(options.snapshot.scrollbackLines === undefined - ? {} - : { scrollbackLineCount: options.snapshot.scrollbackLines.length }), - }, - }), - ); + let wroteArtifact = false; + + try { + await writeTextFileAtomic({ + path: snapshotArtifactPath, + pathLabel: 'snapshot artifact path', + contents: `${JSON.stringify(options.result, null, 2)}\n`, + writeErrorMessage: `Failed to write snapshot artifact at ${snapshotArtifactPath}.`, + }); + wroteArtifact = true; + + await appendArtifact( + options.sessionDir, + createArtifactEntry({ + kind: 'snapshot', + filename, + sessionId: options.snapshot.sessionId, + capturedAtSeq: options.snapshot.capturedAtSeq, + metadata: { + format: options.format, + rendererBackend: options.rendererBackend, + cols: options.snapshot.cols, + rows: options.snapshot.rows, + cursorRow: options.snapshot.cursorRow, + cursorCol: options.snapshot.cursorCol, + ...(options.snapshot.scrollbackLines === undefined + ? {} + : { scrollbackLineCount: options.snapshot.scrollbackLines.length }), + }, + }), + ); + } catch (error) { + if (wroteArtifact) { + await rm(snapshotArtifactPath, { force: true }).catch(() => undefined); + } + throw error; + } } export async function captureSnapshotResult( diff --git a/test/unit/screenshot/capture.test.ts b/test/unit/screenshot/capture.test.ts index 816e460..bf10f8e 100644 --- a/test/unit/screenshot/capture.test.ts +++ b/test/unit/screenshot/capture.test.ts @@ -384,7 +384,7 @@ describe('screenshot capture', () => { expect(manifest.artifacts).toEqual([]); }); - it('preserves the renamed final PNG when manifest append fails after rename', async () => { + it('removes the renamed final PNG when manifest append fails after rename', async () => { const sessionDirectory = await createSessionDir(); const finalFilename = screenshotFilename(5, 'reference-dark'); const finalPath = artifactPath(sessionDirectory, finalFilename); @@ -418,12 +418,9 @@ describe('screenshot capture', () => { }), ).rejects.toMatchObject({ code: 'MANIFEST_VALIDATION_ERROR' }); - // The final PNG survives because cleanup only removes the temp file, - // not the already-renamed artifact. This is intentional: the rename - // succeeded, so the PNG is valid; only the manifest entry is missing. - // Adding rename rollback is not addressed in this refactor; the - // follow-up is tracked in #79. - await expect(access(finalPath)).resolves.toBeUndefined(); + // The temp file has already been renamed, so rollback must remove the + // final artifact path to avoid leaving an unmanifested PNG behind. + await expect(access(finalPath)).rejects.toMatchObject({ code: 'ENOENT' }); if (observedTempPath !== undefined) { await expect(access(observedTempPath)).rejects.toMatchObject({ code: 'ENOENT', diff --git a/test/unit/snapshot/capture.test.ts b/test/unit/snapshot/capture.test.ts index 0c9b343..544f3d6 100644 --- a/test/unit/snapshot/capture.test.ts +++ b/test/unit/snapshot/capture.test.ts @@ -1,4 +1,5 @@ -import { access, mkdir, readFile } from 'node:fs/promises'; +import { access, mkdir, readFile, writeFile } from 'node:fs/promises'; +import { dirname } from 'node:path'; import { describe, expect, it } from 'vitest'; @@ -146,6 +147,39 @@ describe('snapshot capture', () => { }); }); + it('removes the snapshot artifact when manifest append fails after write', async () => { + const sessionDirectory = await createSessionDir(); + const snapshot = createTestSemanticSnapshot(); + const result = createSnapshotResult(snapshot, 'structured'); + const filename = snapshotFilename(5, 'structured'); + const snapshotPath = artifactPath(sessionDirectory, filename); + const manifestFilePath = artifactPath(sessionDirectory, 'manifest.json'); + const unrelatedManifest = { + version: 1, + sessionId: 'unrelated-session', + artifacts: [], + }; + await mkdir(dirname(manifestFilePath), { recursive: true }); + await writeFile(manifestFilePath, `${JSON.stringify(unrelatedManifest)}\n`); + + await expect( + persistSnapshotArtifact({ + sessionDir: sessionDirectory, + format: 'structured', + snapshot, + result, + rendererBackend: 'test-backend', + }), + ).rejects.toMatchObject({ code: 'MANIFEST_VALIDATION_ERROR' }); + + await expect(access(snapshotPath)).rejects.toMatchObject({ + code: 'ENOENT', + }); + await expect(readFile(manifestFilePath, 'utf8')).resolves.toBe( + `${JSON.stringify(unrelatedManifest)}\n`, + ); + }); + it('requires renderer backend metadata before writing artifacts', async () => { const sessionDirectory = await createSessionDir(); From e1aa1a387b5a8f6765f693f182fe5bcbb6413c8a Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 12 May 2026 08:49:19 +0000 Subject: [PATCH 2/8] fix: centralize artifact append rollback --- src/cli/commands/record-export.ts | 29 +++--- src/screenshot/capture.ts | 52 +++++----- src/snapshot/capture.ts | 34 +++---- src/storage/artifactManifest.ts | 26 ++++- test/unit/commands/record-export.test.ts | 115 ++++++++++++++++++++++- test/unit/screenshot/capture.test.ts | 17 ++-- test/unit/snapshot/capture.test.ts | 3 + 7 files changed, 200 insertions(+), 76 deletions(-) diff --git a/src/cli/commands/record-export.ts b/src/cli/commands/record-export.ts index 56ab389..1875c85 100644 --- a/src/cli/commands/record-export.ts +++ b/src/cli/commands/record-export.ts @@ -24,7 +24,7 @@ import { type RecordExportResult, } from '../../protocol/messages.js'; import { - appendArtifact, + appendArtifactWithRollback, createArtifactEntry, } from '../../storage/artifactManifest.js'; import { @@ -219,7 +219,6 @@ export async function runRecordExportCommand( const manifestFile = manifestPath(sessionDirectory); const manifest = await readManifestIfExists(manifestFile); - if (manifest === null) { throw makeCliError(ERROR_CODES.SESSION_NOT_FOUND, { message: `Session "${options.sessionId}" was not found.`, @@ -371,18 +370,20 @@ export async function runRecordExportCommand( sha256 = await computeFileHash(artifactOutputPath); } - await appendArtifact( - sessionDirectory, - createArtifactEntry({ - kind: artifactKind, - filename: basename(artifactOutputPath), - sessionId: manifest.sessionId, - capturedAtSeq, - sha256, - bytes, - metadata: artifactMetadata, - }), - ); + await appendArtifactWithRollback({ + sessionDir: sessionDirectory, + artifactPath: artifactOutputPath, + createEntry: () => + createArtifactEntry({ + kind: artifactKind, + filename: basename(artifactOutputPath), + sessionId: manifest.sessionId, + capturedAtSeq, + sha256, + bytes, + metadata: artifactMetadata, + }), + }); const rawResult = { sessionId: manifest.sessionId, diff --git a/src/screenshot/capture.ts b/src/screenshot/capture.ts index 2a446ba..60b2169 100644 --- a/src/screenshot/capture.ts +++ b/src/screenshot/capture.ts @@ -8,7 +8,7 @@ import type { RendererBackend } from '../renderer/backend.js'; import { ScreenshotResultSchema } from '../protocol/schemas.js'; import { parseValidatedResult } from '../protocol/validation.js'; import { - appendArtifact, + appendArtifactWithRollback, createArtifactEntry, } from '../storage/artifactManifest.js'; import { @@ -65,8 +65,6 @@ export async function captureScreenshotResult( `.tmp-screenshot-${ulid()}.png`, ); - let renamedArtifactPath: string | undefined; - try { const rendererResult = await options.backend.screenshot( temporaryOutputPath, @@ -136,35 +134,33 @@ export async function captureScreenshotResult( }; await rename(temporaryOutputPath, finalArtifactPath); - renamedArtifactPath = finalArtifactPath; - await appendArtifact( - options.sessionDir, - createArtifactEntry({ - kind: 'screenshot', - filename, - sessionId: publicResult.sessionId, - capturedAtSeq: publicResult.capturedAtSeq, - sha256, - metadata: { - profileName: publicResult.profileName, - cols: publicResult.cols, - rows: publicResult.rows, - pngSizeBytes: publicResult.pngSizeBytes, - cursorVisible: publicResult.cursorVisible, - rendererBackend: publicResult.rendererBackend, - pixelWidth: publicResult.pixelWidth, - pixelHeight: publicResult.pixelHeight, - renderProfileHash: publicResult.renderProfileHash, - }, - }), - ); + await appendArtifactWithRollback({ + sessionDir: options.sessionDir, + artifactPath: finalArtifactPath, + createEntry: () => + createArtifactEntry({ + kind: 'screenshot', + filename, + sessionId: publicResult.sessionId, + capturedAtSeq: publicResult.capturedAtSeq, + sha256, + metadata: { + profileName: publicResult.profileName, + cols: publicResult.cols, + rows: publicResult.rows, + pngSizeBytes: publicResult.pngSizeBytes, + cursorVisible: publicResult.cursorVisible, + rendererBackend: publicResult.rendererBackend, + pixelWidth: publicResult.pixelWidth, + pixelHeight: publicResult.pixelHeight, + renderProfileHash: publicResult.renderProfileHash, + }, + }), + }); return publicResult; } catch (error) { await rm(temporaryOutputPath, { force: true }).catch(() => undefined); - if (renamedArtifactPath !== undefined) { - await rm(renamedArtifactPath, { force: true }).catch(() => undefined); - } throw error; } } diff --git a/src/snapshot/capture.ts b/src/snapshot/capture.ts index e35d68f..b0ab85a 100644 --- a/src/snapshot/capture.ts +++ b/src/snapshot/capture.ts @@ -1,5 +1,3 @@ -import { rm } from 'node:fs/promises'; - import type { SnapshotParams, SnapshotResult } from '../protocol/messages.js'; import type { SemanticSnapshot } from '../renderer/types.js'; @@ -7,7 +5,7 @@ import { ERROR_CODES, makeCliError } from '../protocol/errors.js'; import { SnapshotResultSchema } from '../protocol/schemas.js'; import { parseValidatedResult } from '../protocol/validation.js'; import { - appendArtifact, + appendArtifactWithRollback, createArtifactEntry, } from '../storage/artifactManifest.js'; import { @@ -128,19 +126,17 @@ export async function persistSnapshotArtifact( ); const snapshotArtifactPath = artifactPath(options.sessionDir, filename); - let wroteArtifact = false; - - try { - await writeTextFileAtomic({ - path: snapshotArtifactPath, - pathLabel: 'snapshot artifact path', - contents: `${JSON.stringify(options.result, null, 2)}\n`, - writeErrorMessage: `Failed to write snapshot artifact at ${snapshotArtifactPath}.`, - }); - wroteArtifact = true; + await writeTextFileAtomic({ + path: snapshotArtifactPath, + pathLabel: 'snapshot artifact path', + contents: `${JSON.stringify(options.result, null, 2)}\n`, + writeErrorMessage: `Failed to write snapshot artifact at ${snapshotArtifactPath}.`, + }); - await appendArtifact( - options.sessionDir, + await appendArtifactWithRollback({ + sessionDir: options.sessionDir, + artifactPath: snapshotArtifactPath, + createEntry: () => createArtifactEntry({ kind: 'snapshot', filename, @@ -158,13 +154,7 @@ export async function persistSnapshotArtifact( : { scrollbackLineCount: options.snapshot.scrollbackLines.length }), }, }), - ); - } catch (error) { - if (wroteArtifact) { - await rm(snapshotArtifactPath, { force: true }).catch(() => undefined); - } - throw error; - } + }); } export async function captureSnapshotResult( diff --git a/src/storage/artifactManifest.ts b/src/storage/artifactManifest.ts index 551fb15..c75eedc 100644 --- a/src/storage/artifactManifest.ts +++ b/src/storage/artifactManifest.ts @@ -1,4 +1,5 @@ -import { basename, resolve } from 'node:path'; +import { rm } from 'node:fs/promises'; +import { basename, isAbsolute, resolve } from 'node:path'; import { ulid } from 'ulid'; import { z } from 'zod'; @@ -171,6 +172,12 @@ export async function writeArtifactManifest( }); } +export interface AppendArtifactWithRollbackOptions { + sessionDir: string; + artifactPath: string; + createEntry: () => ArtifactEntry; +} + export async function appendArtifact( sessionDir: string, entry: ArtifactEntry, @@ -188,6 +195,23 @@ export async function appendArtifact( }); } +export async function appendArtifactWithRollback( + options: AppendArtifactWithRollbackOptions, +): Promise { + invariant( + options.artifactPath.length > 0, + 'artifactPath must be a non-empty string', + ); + invariant(isAbsolute(options.artifactPath), 'artifactPath must be absolute'); + + try { + await appendArtifact(options.sessionDir, options.createEntry()); + } catch (error) { + await rm(options.artifactPath, { force: true }).catch(() => undefined); + throw error; + } +} + export function createArtifactEntry( entry: Omit, ): ArtifactEntry { diff --git a/test/unit/commands/record-export.test.ts b/test/unit/commands/record-export.test.ts index 03daf3f..dd99633 100644 --- a/test/unit/commands/record-export.test.ts +++ b/test/unit/commands/record-export.test.ts @@ -1,14 +1,22 @@ import { createHash } from 'node:crypto'; -import { mkdir, mkdtemp, realpath, rm, symlink } from 'node:fs/promises'; +import { + access, + mkdir, + mkdtemp, + realpath, + rm, + symlink, + writeFile, +} from 'node:fs/promises'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { dirname, join } from 'node:path'; import process from 'node:process'; import type * as FsPromises from 'node:fs/promises'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { ERROR_CODES } from '../../../src/protocol/errors.js'; +import { ERROR_CODES, makeCliError } from '../../../src/protocol/errors.js'; const mocks = vi.hoisted(() => ({ emitSuccess: vi.fn(), @@ -442,6 +450,107 @@ describe('record export command', () => { expect(emitSuccessArgs.result.durationMs).toBe(1_500); }); + it('removes asciicast artifacts when manifest append fails after write', async () => { + const sessionDirectory = await createTemporaryDirectory( + 'agent-tty-record-export-append-asciicast-', + ); + const artifactFile = join( + sessionDirectory, + 'artifacts', + 'recording-2-asciicast.cast', + ); + const manifestError = makeCliError(ERROR_CODES.MANIFEST_VALIDATION_ERROR, { + message: 'artifact manifest append failed', + }); + + mocks.sessionDir.mockReturnValue(sessionDirectory); + mocks.manifestPath.mockReturnValue(join(sessionDirectory, 'session.json')); + mocks.eventLogPath.mockReturnValue(join(sessionDirectory, 'events.jsonl')); + mocks.ensureArtifactsDir.mockResolvedValue( + join(sessionDirectory, 'artifacts'), + ); + mocks.recordingFilename.mockReturnValue('recording-2-asciicast.cast'); + mocks.writeTextFileAtomic.mockImplementation( + async (options: { path: string; contents: string }) => { + await mkdir(dirname(options.path), { recursive: true }); + await writeFile(options.path, options.contents, 'utf8'); + }, + ); + mocks.appendArtifact.mockRejectedValue(manifestError); + + await expect( + runRecordExportCommand({ + context: TEST_CONTEXT, + json: true, + sessionId: 'session-01', + format: 'asciicast', + }), + ).rejects.toBe(manifestError); + + await expect(access(artifactFile)).rejects.toMatchObject({ + code: 'ENOENT', + }); + expect(mocks.emitSuccess).not.toHaveBeenCalled(); + }); + + it('removes webm artifacts when manifest append fails after export', async () => { + const sessionDirectory = await createTemporaryDirectory( + 'agent-tty-record-export-append-webm-', + ); + const artifactFile = join( + sessionDirectory, + 'artifacts', + 'recording-1-webm.webm', + ); + const webmBytes = 12_345; + const webmContent = Buffer.alloc(webmBytes, 0x42); + const manifestError = makeCliError(ERROR_CODES.MANIFEST_VALIDATION_ERROR, { + message: 'artifact manifest append failed', + }); + + mocks.sessionDir.mockReturnValue(sessionDirectory); + mocks.manifestPath.mockReturnValue(join(sessionDirectory, 'session.json')); + mocks.eventLogPath.mockReturnValue(join(sessionDirectory, 'events.jsonl')); + mocks.ensureArtifactsDir.mockResolvedValue( + join(sessionDirectory, 'artifacts'), + ); + mocks.recordingFilename.mockReturnValue('recording-1-webm.webm'); + mocks.generateWebmExport.mockImplementation( + async (options: { outputPath: string }) => { + await mkdir(dirname(options.outputPath), { recursive: true }); + await writeFile(options.outputPath, webmContent); + return { + capturedAtSeq: 1, + durationMs: 1_500, + outputEventCount: 1, + resizeEventCount: 1, + cols: 80, + rows: 24, + profileName: 'reference-dark', + timingMode: 'accelerated', + rendererBackend: 'ghostty-web', + }; + }, + ); + mocks.stat.mockResolvedValue({ size: webmBytes }); + mocks.readFile.mockResolvedValue(webmContent); + mocks.appendArtifact.mockRejectedValue(manifestError); + + await expect( + runRecordExportCommand({ + context: TEST_CONTEXT, + json: true, + sessionId: 'session-01', + format: 'webm', + }), + ).rejects.toBe(manifestError); + + await expect(access(artifactFile)).rejects.toMatchObject({ + code: 'ENOENT', + }); + expect(mocks.emitSuccess).not.toHaveBeenCalled(); + }); + it('resolves WebM profile from command option, context default, and builtin fallback', async () => { mocks.recordingFilename.mockReturnValue('recording-1-webm.webm'); mocks.generateWebmExport.mockResolvedValue({ diff --git a/test/unit/screenshot/capture.test.ts b/test/unit/screenshot/capture.test.ts index bf10f8e..a65a762 100644 --- a/test/unit/screenshot/capture.test.ts +++ b/test/unit/screenshot/capture.test.ts @@ -392,15 +392,13 @@ describe('screenshot capture', () => { // `appendArtifact` raises a MANIFEST_VALIDATION_ERROR after the temp file // has already been renamed into place. const manifestFilePath = artifactPath(sessionDirectory, 'manifest.json'); + const unrelatedManifest = { + version: 1, + sessionId: 'unrelated-session', + artifacts: [], + }; await mkdir(dirname(manifestFilePath), { recursive: true }); - await writeFile( - manifestFilePath, - `${JSON.stringify({ - version: 1, - sessionId: 'unrelated-session', - artifacts: [], - })}\n`, - ); + await writeFile(manifestFilePath, `${JSON.stringify(unrelatedManifest)}\n`); let observedTempPath: string | undefined; const backend = createFakeBackend({ @@ -426,6 +424,9 @@ describe('screenshot capture', () => { code: 'ENOENT', }); } + await expect(readFile(manifestFilePath, 'utf8')).resolves.toBe( + `${JSON.stringify(unrelatedManifest)}\n`, + ); }); it('removes the temp file and rethrows when the renderer screenshot rejects', async () => { diff --git a/test/unit/snapshot/capture.test.ts b/test/unit/snapshot/capture.test.ts index 544f3d6..d0aa3ae 100644 --- a/test/unit/snapshot/capture.test.ts +++ b/test/unit/snapshot/capture.test.ts @@ -154,6 +154,9 @@ describe('snapshot capture', () => { const filename = snapshotFilename(5, 'structured'); const snapshotPath = artifactPath(sessionDirectory, filename); const manifestFilePath = artifactPath(sessionDirectory, 'manifest.json'); + // Pre-write a manifest whose sessionId does not match the directory so + // `appendArtifact` raises a MANIFEST_VALIDATION_ERROR after the snapshot + // artifact has already been written. const unrelatedManifest = { version: 1, sessionId: 'unrelated-session', From cba3350da77adba7b0547c630bb222df6615b93a Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 12 May 2026 09:21:14 +0000 Subject: [PATCH 3/8] test: mock centralized artifact rollback --- test/unit/commands/record-export.test.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/unit/commands/record-export.test.ts b/test/unit/commands/record-export.test.ts index dd99633..31b65c2 100644 --- a/test/unit/commands/record-export.test.ts +++ b/test/unit/commands/record-export.test.ts @@ -27,6 +27,7 @@ const mocks = vi.hoisted(() => ({ manifestPath: vi.fn(), eventLogPath: vi.fn(), writeTextFileAtomic: vi.fn(), + appendArtifactWithRollback: vi.fn(), appendArtifact: vi.fn(), createArtifactEntry: vi.fn(), ensureArtifactsDir: vi.fn(), @@ -62,6 +63,7 @@ vi.mock('../../../src/storage/sessionPaths.js', () => ({ })); vi.mock('../../../src/storage/artifactManifest.js', () => ({ + appendArtifactWithRollback: mocks.appendArtifactWithRollback, appendArtifact: mocks.appendArtifact, createArtifactEntry: mocks.createArtifactEntry, })); @@ -180,6 +182,22 @@ describe('record export command', () => { `${sessionDirectory}/artifacts/${filename}`, ); mocks.writeTextFileAtomic.mockResolvedValue(undefined); + mocks.appendArtifactWithRollback.mockImplementation( + async (options: { + sessionDir: string; + artifactPath: string; + createEntry: () => unknown; + }) => { + try { + await mocks.appendArtifact(options.sessionDir, options.createEntry()); + } catch (error) { + await rm(options.artifactPath, { force: true }).catch( + () => undefined, + ); + throw error; + } + }, + ); mocks.appendArtifact.mockResolvedValue(undefined); mocks.createArtifactEntry.mockImplementation((entry: unknown) => ({ id: 'artifact-01', From 5e69a19443427ddd7d750a98e6e3284294408154 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 12 May 2026 09:49:08 +0000 Subject: [PATCH 4/8] fix: harden centralized artifact rollback --- src/cli/commands/record-export.ts | 25 ++-- src/screenshot/capture.ts | 41 +++--- src/snapshot/capture.ts | 39 ++--- src/storage/artifactManifest.ts | 56 +++++--- test/unit/commands/record-export.test.ts | 63 +++++++-- test/unit/commands/screenshot.test.ts | 7 + test/unit/commands/snapshot.test.ts | 7 + test/unit/storage/artifactStorage.test.ts | 164 ++++++++++++++++++++-- 8 files changed, 309 insertions(+), 93 deletions(-) diff --git a/src/cli/commands/record-export.ts b/src/cli/commands/record-export.ts index 1875c85..a3633b2 100644 --- a/src/cli/commands/record-export.ts +++ b/src/cli/commands/record-export.ts @@ -370,19 +370,22 @@ export async function runRecordExportCommand( sha256 = await computeFileHash(artifactOutputPath); } + const artifactEntry = createArtifactEntry({ + kind: artifactKind, + filename: basename(artifactOutputPath), + sessionId: manifest.sessionId, + capturedAtSeq, + sha256, + bytes, + metadata: artifactMetadata, + }); + await appendArtifactWithRollback({ sessionDir: sessionDirectory, - artifactPath: artifactOutputPath, - createEntry: () => - createArtifactEntry({ - kind: artifactKind, - filename: basename(artifactOutputPath), - sessionId: manifest.sessionId, - capturedAtSeq, - sha256, - bytes, - metadata: artifactMetadata, - }), + entry: artifactEntry, + ...(options.out === undefined + ? { rollbackArtifactPath: artifactOutputPath } + : {}), }); const rawResult = { diff --git a/src/screenshot/capture.ts b/src/screenshot/capture.ts index 60b2169..797ca9d 100644 --- a/src/screenshot/capture.ts +++ b/src/screenshot/capture.ts @@ -133,29 +133,30 @@ export async function captureScreenshotResult( sha256: parsedResult.sha256, }; + const artifactEntry = createArtifactEntry({ + kind: 'screenshot', + filename, + sessionId: publicResult.sessionId, + capturedAtSeq: publicResult.capturedAtSeq, + sha256, + metadata: { + profileName: publicResult.profileName, + cols: publicResult.cols, + rows: publicResult.rows, + pngSizeBytes: publicResult.pngSizeBytes, + cursorVisible: publicResult.cursorVisible, + rendererBackend: publicResult.rendererBackend, + pixelWidth: publicResult.pixelWidth, + pixelHeight: publicResult.pixelHeight, + renderProfileHash: publicResult.renderProfileHash, + }, + }); + await rename(temporaryOutputPath, finalArtifactPath); await appendArtifactWithRollback({ sessionDir: options.sessionDir, - artifactPath: finalArtifactPath, - createEntry: () => - createArtifactEntry({ - kind: 'screenshot', - filename, - sessionId: publicResult.sessionId, - capturedAtSeq: publicResult.capturedAtSeq, - sha256, - metadata: { - profileName: publicResult.profileName, - cols: publicResult.cols, - rows: publicResult.rows, - pngSizeBytes: publicResult.pngSizeBytes, - cursorVisible: publicResult.cursorVisible, - rendererBackend: publicResult.rendererBackend, - pixelWidth: publicResult.pixelWidth, - pixelHeight: publicResult.pixelHeight, - renderProfileHash: publicResult.renderProfileHash, - }, - }), + entry: artifactEntry, + rollbackArtifactPath: finalArtifactPath, }); return publicResult; diff --git a/src/snapshot/capture.ts b/src/snapshot/capture.ts index b0ab85a..ed766c8 100644 --- a/src/snapshot/capture.ts +++ b/src/snapshot/capture.ts @@ -126,6 +126,24 @@ export async function persistSnapshotArtifact( ); const snapshotArtifactPath = artifactPath(options.sessionDir, filename); + const artifactEntry = createArtifactEntry({ + kind: 'snapshot', + filename, + sessionId: options.snapshot.sessionId, + capturedAtSeq: options.snapshot.capturedAtSeq, + metadata: { + format: options.format, + rendererBackend: options.rendererBackend, + cols: options.snapshot.cols, + rows: options.snapshot.rows, + cursorRow: options.snapshot.cursorRow, + cursorCol: options.snapshot.cursorCol, + ...(options.snapshot.scrollbackLines === undefined + ? {} + : { scrollbackLineCount: options.snapshot.scrollbackLines.length }), + }, + }); + await writeTextFileAtomic({ path: snapshotArtifactPath, pathLabel: 'snapshot artifact path', @@ -135,25 +153,8 @@ export async function persistSnapshotArtifact( await appendArtifactWithRollback({ sessionDir: options.sessionDir, - artifactPath: snapshotArtifactPath, - createEntry: () => - createArtifactEntry({ - kind: 'snapshot', - filename, - sessionId: options.snapshot.sessionId, - capturedAtSeq: options.snapshot.capturedAtSeq, - metadata: { - format: options.format, - rendererBackend: options.rendererBackend, - cols: options.snapshot.cols, - rows: options.snapshot.rows, - cursorRow: options.snapshot.cursorRow, - cursorCol: options.snapshot.cursorCol, - ...(options.snapshot.scrollbackLines === undefined - ? {} - : { scrollbackLineCount: options.snapshot.scrollbackLines.length }), - }, - }), + entry: artifactEntry, + rollbackArtifactPath: snapshotArtifactPath, }); } diff --git a/src/storage/artifactManifest.ts b/src/storage/artifactManifest.ts index c75eedc..974ec2f 100644 --- a/src/storage/artifactManifest.ts +++ b/src/storage/artifactManifest.ts @@ -174,42 +174,60 @@ export async function writeArtifactManifest( export interface AppendArtifactWithRollbackOptions { sessionDir: string; - artifactPath: string; - createEntry: () => ArtifactEntry; + entry: ArtifactEntry; + rollbackArtifactPath?: string; } -export async function appendArtifact( +async function appendArtifact( sessionDir: string, entry: ArtifactEntry, + rollbackArtifactPath: string | undefined, ): Promise { const resolvedSessionDir = resolve(sessionDir); const expectedSessionId = sessionIdFromSessionDir(resolvedSessionDir); const validatedEntry = validateArtifactEntry(entry, expectedSessionId); await appendSerializer.run(resolvedSessionDir, async () => { - const manifest = await readArtifactManifest(resolvedSessionDir); - await writeArtifactManifest(resolvedSessionDir, { - ...manifest, - artifacts: [...manifest.artifacts, validatedEntry], - }); + try { + const manifest = await readArtifactManifest(resolvedSessionDir); + await writeArtifactManifest(resolvedSessionDir, { + ...manifest, + artifacts: [...manifest.artifacts, validatedEntry], + }); + } catch (error) { + if (rollbackArtifactPath !== undefined) { + // Best-effort: swallow rm errors so the original manifest failure propagates. + await rm(rollbackArtifactPath, { force: true }).catch(() => undefined); + } + throw error; + } }); } +/** + * Append an artifact entry to the session manifest, removing the artifact file + * at `rollbackArtifactPath` if the append fails. Rollback is best-effort: rm + * errors are swallowed so the original manifest error propagates. + */ export async function appendArtifactWithRollback( options: AppendArtifactWithRollbackOptions, ): Promise { - invariant( - options.artifactPath.length > 0, - 'artifactPath must be a non-empty string', - ); - invariant(isAbsolute(options.artifactPath), 'artifactPath must be absolute'); - - try { - await appendArtifact(options.sessionDir, options.createEntry()); - } catch (error) { - await rm(options.artifactPath, { force: true }).catch(() => undefined); - throw error; + if (options.rollbackArtifactPath !== undefined) { + invariant( + options.rollbackArtifactPath.length > 0, + 'rollbackArtifactPath must be a non-empty string', + ); + invariant( + isAbsolute(options.rollbackArtifactPath), + 'rollbackArtifactPath must be absolute', + ); } + + await appendArtifact( + options.sessionDir, + options.entry, + options.rollbackArtifactPath, + ); } export function createArtifactEntry( diff --git a/test/unit/commands/record-export.test.ts b/test/unit/commands/record-export.test.ts index 31b65c2..403a11c 100644 --- a/test/unit/commands/record-export.test.ts +++ b/test/unit/commands/record-export.test.ts @@ -185,15 +185,17 @@ describe('record export command', () => { mocks.appendArtifactWithRollback.mockImplementation( async (options: { sessionDir: string; - artifactPath: string; - createEntry: () => unknown; + entry: unknown; + rollbackArtifactPath?: string; }) => { try { - await mocks.appendArtifact(options.sessionDir, options.createEntry()); + await mocks.appendArtifact(options.sessionDir, options.entry); } catch (error) { - await rm(options.artifactPath, { force: true }).catch( - () => undefined, - ); + if (options.rollbackArtifactPath !== undefined) { + await rm(options.rollbackArtifactPath, { force: true }).catch( + () => undefined, + ); + } throw error; } }, @@ -468,7 +470,7 @@ describe('record export command', () => { expect(emitSuccessArgs.result.durationMs).toBe(1_500); }); - it('removes asciicast artifacts when manifest append fails after write', async () => { + it('requests rollback for default asciicast artifacts when manifest append fails', async () => { const sessionDirectory = await createTemporaryDirectory( 'agent-tty-record-export-append-asciicast-', ); @@ -505,13 +507,13 @@ describe('record export command', () => { }), ).rejects.toBe(manifestError); - await expect(access(artifactFile)).rejects.toMatchObject({ - code: 'ENOENT', - }); + expect(mocks.appendArtifactWithRollback).toHaveBeenCalledWith( + expect.objectContaining({ rollbackArtifactPath: artifactFile }), + ); expect(mocks.emitSuccess).not.toHaveBeenCalled(); }); - it('removes webm artifacts when manifest append fails after export', async () => { + it('requests rollback for default webm artifacts when manifest append fails', async () => { const sessionDirectory = await createTemporaryDirectory( 'agent-tty-record-export-append-webm-', ); @@ -563,9 +565,44 @@ describe('record export command', () => { }), ).rejects.toBe(manifestError); - await expect(access(artifactFile)).rejects.toMatchObject({ - code: 'ENOENT', + expect(mocks.appendArtifactWithRollback).toHaveBeenCalledWith( + expect.objectContaining({ rollbackArtifactPath: artifactFile }), + ); + expect(mocks.emitSuccess).not.toHaveBeenCalled(); + }); + + it('preserves explicit output artifacts when manifest append fails', async () => { + const workspaceDirectory = await createTemporaryDirectory( + 'agent-tty-record-export-explicit-out-', + ); + const artifactFile = join(workspaceDirectory, 'custom.cast'); + const manifestError = makeCliError(ERROR_CODES.MANIFEST_VALIDATION_ERROR, { + message: 'artifact manifest append failed', }); + + mocks.writeTextFileAtomic.mockImplementation( + async (options: { path: string; contents: string }) => { + await mkdir(dirname(options.path), { recursive: true }); + await writeFile(options.path, options.contents, 'utf8'); + }, + ); + mocks.appendArtifact.mockRejectedValue(manifestError); + + await expect( + runRecordExportCommand({ + context: TEST_CONTEXT, + json: true, + sessionId: 'session-01', + format: 'asciicast', + out: artifactFile, + }), + ).rejects.toBe(manifestError); + + const appendCall = mocks.appendArtifactWithRollback.mock.calls.at(-1) as [ + { rollbackArtifactPath?: string }, + ]; + expect(appendCall[0]).not.toHaveProperty('rollbackArtifactPath'); + await expect(access(artifactFile)).resolves.toBeUndefined(); expect(mocks.emitSuccess).not.toHaveBeenCalled(); }); diff --git a/test/unit/commands/screenshot.test.ts b/test/unit/commands/screenshot.test.ts index 0844f5f..74380fb 100644 --- a/test/unit/commands/screenshot.test.ts +++ b/test/unit/commands/screenshot.test.ts @@ -13,6 +13,7 @@ const mocks = vi.hoisted(() => ({ manifestPath: vi.fn(), socketPath: vi.fn(), withOfflineReplayRenderer: vi.fn(), + appendArtifactWithRollback: vi.fn(), appendArtifact: vi.fn(), createArtifactEntry: vi.fn(), ensureArtifactsDir: vi.fn(), @@ -52,6 +53,7 @@ vi.mock('../../../src/replay/offlineReplay.js', () => ({ })); vi.mock('../../../src/storage/artifactManifest.js', () => ({ + appendArtifactWithRollback: mocks.appendArtifactWithRollback, appendArtifact: mocks.appendArtifact, createArtifactEntry: mocks.createArtifactEntry, })); @@ -193,6 +195,11 @@ describe('screenshot command', () => { (sessionDirectory: string) => `${sessionDirectory}/rpc.sock`, ); mocks.readManifestIfExists.mockResolvedValue(createRunningSessionRecord()); + mocks.appendArtifactWithRollback.mockImplementation( + async (options: { sessionDir: string; entry: unknown }) => { + await mocks.appendArtifact(options.sessionDir, options.entry); + }, + ); mocks.appendArtifact.mockResolvedValue(undefined); mocks.createArtifactEntry.mockImplementation((entry: unknown) => ({ id: 'artifact-01', diff --git a/test/unit/commands/snapshot.test.ts b/test/unit/commands/snapshot.test.ts index 7ca7c6d..c0e97f4 100644 --- a/test/unit/commands/snapshot.test.ts +++ b/test/unit/commands/snapshot.test.ts @@ -12,6 +12,7 @@ const mocks = vi.hoisted(() => ({ manifestPath: vi.fn(), socketPath: vi.fn(), withOfflineReplayRenderer: vi.fn(), + appendArtifactWithRollback: vi.fn(), appendArtifact: vi.fn(), createArtifactEntry: vi.fn(), artifactPath: vi.fn(), @@ -33,6 +34,7 @@ vi.mock('../../../src/replay/offlineReplay.js', () => ({ })); vi.mock('../../../src/storage/artifactManifest.js', () => ({ + appendArtifactWithRollback: mocks.appendArtifactWithRollback, appendArtifact: mocks.appendArtifact, createArtifactEntry: mocks.createArtifactEntry, })); @@ -161,6 +163,11 @@ describe('snapshot command', () => { (seq: number, format: string) => `snapshot-${String(seq)}-${format}.json`, ); mocks.ensureArtifactsDir.mockResolvedValue('/artifacts'); + mocks.appendArtifactWithRollback.mockImplementation( + async (options: { sessionDir: string; entry: unknown }) => { + await mocks.appendArtifact(options.sessionDir, options.entry); + }, + ); mocks.appendArtifact.mockResolvedValue(undefined); mocks.writeTextFileAtomic.mockResolvedValue(undefined); }); diff --git a/test/unit/storage/artifactStorage.test.ts b/test/unit/storage/artifactStorage.test.ts index 9f5fa46..ee3a76e 100644 --- a/test/unit/storage/artifactStorage.test.ts +++ b/test/unit/storage/artifactStorage.test.ts @@ -1,4 +1,4 @@ -import { access, readFile, writeFile } from 'node:fs/promises'; +import { access, mkdir, readFile, writeFile } from 'node:fs/promises'; import { join } from 'node:path'; @@ -11,7 +11,7 @@ import type { ArtifactManifest, } from '../../../src/storage/artifactManifest.js'; import { - appendArtifact, + appendArtifactWithRollback, ArtifactEntrySchema, readArtifactManifest, writeArtifactManifest, @@ -158,9 +158,9 @@ describe('artifact manifest storage', () => { artifacts: [], }); - await appendArtifact( + await appendArtifactWithRollback({ sessionDir, - createArtifactEntry({ + entry: createArtifactEntry({ id: '01JQ0000000000000000000001', kind: 'screenshot', filename: 'screenshot-4-reference-dark.png', @@ -171,7 +171,7 @@ describe('artifact manifest storage', () => { pngSizeBytes: 2048, }, }), - ); + }); await expect(readArtifactManifest(sessionDir)).resolves.toEqual({ version: 1, @@ -214,14 +214,14 @@ describe('artifact manifest storage', () => { await Promise.all( Array.from({ length: concurrentAppends }, (_value, index) => - appendArtifact( + appendArtifactWithRollback({ sessionDir, - createArtifactEntry({ + entry: createArtifactEntry({ id: `01JQ${String(index).padStart(22, '0')}`, filename: `snapshot-${String(index)}-structured.json`, capturedAtSeq: index, }), - ), + }), ), ); @@ -237,6 +237,148 @@ describe('artifact manifest storage', () => { ); }); + it('removes rollback artifact paths when manifest append fails', async () => { + const sessionDir = await createSessionDir(); + const artifactFile = artifactPath(sessionDir, 'orphan.json'); + + await ensureArtifactsDir(sessionDir); + await writeFile(artifactFile, 'artifact', 'utf8'); + await writeFile( + artifactPath(sessionDir, 'manifest.json'), + `${JSON.stringify({ + version: 1, + sessionId: 'other-session', + artifacts: [], + })}\n`, + 'utf8', + ); + + await expect( + appendArtifactWithRollback({ + sessionDir, + entry: createArtifactEntry({ filename: 'orphan.json' }), + rollbackArtifactPath: artifactFile, + }), + ).rejects.toMatchObject({ code: 'MANIFEST_VALIDATION_ERROR' }); + + await expect(access(artifactFile)).rejects.toMatchObject({ + code: 'ENOENT', + }); + }); + + it('preserves artifact paths when rollback is not requested', async () => { + const sessionDir = await createSessionDir(); + const artifactFile = artifactPath(sessionDir, 'explicit-out.cast'); + + await ensureArtifactsDir(sessionDir); + await writeFile(artifactFile, 'artifact', 'utf8'); + await writeFile( + artifactPath(sessionDir, 'manifest.json'), + `${JSON.stringify({ + version: 1, + sessionId: 'other-session', + artifacts: [], + })}\n`, + 'utf8', + ); + + await expect( + appendArtifactWithRollback({ + sessionDir, + entry: createArtifactEntry({ filename: 'explicit-out.cast' }), + }), + ).rejects.toMatchObject({ code: 'MANIFEST_VALIDATION_ERROR' }); + + await expect(access(artifactFile)).resolves.toBeUndefined(); + }); + + it('asserts rollback paths are non-empty and absolute', async () => { + const sessionDir = await createSessionDir(); + + await expect( + appendArtifactWithRollback({ + sessionDir, + entry: createArtifactEntry(), + rollbackArtifactPath: '', + }), + ).rejects.toThrow(/rollbackArtifactPath must be a non-empty string/u); + await expect( + appendArtifactWithRollback({ + sessionDir, + entry: createArtifactEntry(), + rollbackArtifactPath: 'relative-artifact.json', + }), + ).rejects.toThrow(/rollbackArtifactPath must be absolute/u); + }); + + it('does not remove queued rollback paths when an earlier append fails', async () => { + const sessionDir = await createSessionDir(); + const firstArtifact = artifactPath(sessionDir, 'first-orphan.json'); + const secondArtifact = artifactPath(sessionDir, 'second-orphan.json'); + + await ensureArtifactsDir(sessionDir); + await writeFile(firstArtifact, 'first', 'utf8'); + await writeFile(secondArtifact, 'second', 'utf8'); + await writeFile( + artifactPath(sessionDir, 'manifest.json'), + `${JSON.stringify({ + version: 1, + sessionId: 'other-session', + artifacts: [], + })}\n`, + 'utf8', + ); + + const results = await Promise.allSettled([ + appendArtifactWithRollback({ + sessionDir, + entry: createArtifactEntry({ filename: 'first-orphan.json' }), + rollbackArtifactPath: firstArtifact, + }), + appendArtifactWithRollback({ + sessionDir, + entry: createArtifactEntry({ filename: 'second-orphan.json' }), + rollbackArtifactPath: secondArtifact, + }), + ]); + + expect(results).toEqual([ + expect.objectContaining({ status: 'rejected' }), + expect.objectContaining({ status: 'rejected' }), + ]); + await expect(access(firstArtifact)).rejects.toMatchObject({ + code: 'ENOENT', + }); + await expect(access(secondArtifact)).resolves.toBeUndefined(); + }); + + it('does not mask the manifest error when rollback removal fails', async () => { + const sessionDir = await createSessionDir(); + const blockedPath = artifactPath(sessionDir, 'blocked-artifact'); + + await ensureArtifactsDir(sessionDir); + await writeFile( + artifactPath(sessionDir, 'manifest.json'), + `${JSON.stringify({ + version: 1, + sessionId: 'other-session', + artifacts: [], + })}\n`, + 'utf8', + ); + await mkdir(blockedPath); + await writeFile(join(blockedPath, 'child'), 'artifact', 'utf8'); + + await expect( + appendArtifactWithRollback({ + sessionDir, + entry: createArtifactEntry({ filename: 'blocked-artifact' }), + rollbackArtifactPath: blockedPath, + }), + ).rejects.toMatchObject({ code: 'MANIFEST_VALIDATION_ERROR' }); + await expect(access(join(blockedPath, 'child'))).resolves.toBeUndefined(); + }); + it('rejects invalid manifest contents and mismatched entries', async () => { const sessionDir = await createSessionDir(); @@ -255,12 +397,12 @@ describe('artifact manifest storage', () => { code: 'MANIFEST_VALIDATION_ERROR', }); await expect( - appendArtifact( + appendArtifactWithRollback({ sessionDir, - createArtifactEntry({ + entry: createArtifactEntry({ sessionId: 'other-session', }), - ), + }), ).rejects.toMatchObject({ code: 'MANIFEST_VALIDATION_ERROR', }); From 7da12b80b0102dbb7803106e3fa8ab9184743a66 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 12 May 2026 09:49:57 +0000 Subject: [PATCH 5/8] style: restore record export manifest spacing --- src/cli/commands/record-export.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/cli/commands/record-export.ts b/src/cli/commands/record-export.ts index a3633b2..681530f 100644 --- a/src/cli/commands/record-export.ts +++ b/src/cli/commands/record-export.ts @@ -219,6 +219,7 @@ export async function runRecordExportCommand( const manifestFile = manifestPath(sessionDirectory); const manifest = await readManifestIfExists(manifestFile); + if (manifest === null) { throw makeCliError(ERROR_CODES.SESSION_NOT_FOUND, { message: `Session "${options.sessionId}" was not found.`, From 565f6fd922af2651e356377cd72b5fad938687d2 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 12 May 2026 10:13:58 +0000 Subject: [PATCH 6/8] docs: clarify artifact rollback decisions --- src/cli/commands/record-export.ts | 3 +++ test/unit/commands/record-export.test.ts | 28 +++++++++++++++-------- test/unit/commands/screenshot.test.ts | 1 + test/unit/commands/snapshot.test.ts | 1 + test/unit/storage/artifactStorage.test.ts | 4 +++- 5 files changed, 27 insertions(+), 10 deletions(-) diff --git a/src/cli/commands/record-export.ts b/src/cli/commands/record-export.ts index 681530f..3d9068b 100644 --- a/src/cli/commands/record-export.ts +++ b/src/cli/commands/record-export.ts @@ -381,6 +381,9 @@ export async function runRecordExportCommand( metadata: artifactMetadata, }); + // Explicit --out files belong to the user and are valid without a + // manifest entry; only clean up default in-session artifacts that would + // otherwise be orphaned. await appendArtifactWithRollback({ sessionDir: sessionDirectory, entry: artifactEntry, diff --git a/test/unit/commands/record-export.test.ts b/test/unit/commands/record-export.test.ts index 403a11c..3c38814 100644 --- a/test/unit/commands/record-export.test.ts +++ b/test/unit/commands/record-export.test.ts @@ -28,6 +28,7 @@ const mocks = vi.hoisted(() => ({ eventLogPath: vi.fn(), writeTextFileAtomic: vi.fn(), appendArtifactWithRollback: vi.fn(), + // Test-internal delegate used by the appendArtifactWithRollback mock. appendArtifact: vi.fn(), createArtifactEntry: vi.fn(), ensureArtifactsDir: vi.fn(), @@ -292,15 +293,24 @@ describe('record export command', () => { markerCount: 1, }, }); - expect(mocks.appendArtifact).toHaveBeenCalledWith( - '/tmp/agent-tty/sessions/session-01', - expect.objectContaining({ - kind: 'recording', - filename: 'recording-2-asciicast.cast', - sha256: expectedSha256, - bytes: Buffer.byteLength(expectedContents, 'utf8'), - }), - ); + const appendCall = mocks.appendArtifactWithRollback.mock.calls.at(-1) as [ + { + sessionDir: string; + entry: Record; + rollbackArtifactPath?: string; + }, + ]; + expect(appendCall[0]).toMatchObject({ + sessionDir: '/tmp/agent-tty/sessions/session-01', + rollbackArtifactPath: + '/tmp/agent-tty/sessions/session-01/artifacts/recording-2-asciicast.cast', + }); + expect(appendCall[0].entry).toMatchObject({ + kind: 'recording', + filename: 'recording-2-asciicast.cast', + sha256: expectedSha256, + bytes: Buffer.byteLength(expectedContents, 'utf8'), + }); expect(mocks.emitSuccess).toHaveBeenCalledWith({ command: 'record export', json: true, diff --git a/test/unit/commands/screenshot.test.ts b/test/unit/commands/screenshot.test.ts index 74380fb..46fe244 100644 --- a/test/unit/commands/screenshot.test.ts +++ b/test/unit/commands/screenshot.test.ts @@ -14,6 +14,7 @@ const mocks = vi.hoisted(() => ({ socketPath: vi.fn(), withOfflineReplayRenderer: vi.fn(), appendArtifactWithRollback: vi.fn(), + // Test-internal delegate used by the appendArtifactWithRollback mock. appendArtifact: vi.fn(), createArtifactEntry: vi.fn(), ensureArtifactsDir: vi.fn(), diff --git a/test/unit/commands/snapshot.test.ts b/test/unit/commands/snapshot.test.ts index c0e97f4..8ce29d9 100644 --- a/test/unit/commands/snapshot.test.ts +++ b/test/unit/commands/snapshot.test.ts @@ -13,6 +13,7 @@ const mocks = vi.hoisted(() => ({ socketPath: vi.fn(), withOfflineReplayRenderer: vi.fn(), appendArtifactWithRollback: vi.fn(), + // Test-internal delegate used by the appendArtifactWithRollback mock. appendArtifact: vi.fn(), createArtifactEntry: vi.fn(), artifactPath: vi.fn(), diff --git a/test/unit/storage/artifactStorage.test.ts b/test/unit/storage/artifactStorage.test.ts index ee3a76e..4dbfec4 100644 --- a/test/unit/storage/artifactStorage.test.ts +++ b/test/unit/storage/artifactStorage.test.ts @@ -208,7 +208,7 @@ describe('artifact manifest storage', () => { ).resolves.toMatch(/\n$/u); }); - it('preserves all entries when many concurrent appendArtifact() calls race for the same session', async () => { + it('preserves all entries when many concurrent appendArtifactWithRollback() calls race for the same session', async () => { const sessionDir = await createSessionDir(); const concurrentAppends = 20; @@ -329,6 +329,8 @@ describe('artifact manifest storage', () => { 'utf8', ); + // The second append never runs because KeyedSerializer propagates the + // first rejection without calling queued callbacks. const results = await Promise.allSettled([ appendArtifactWithRollback({ sessionDir, From 78368536008cc8ec5703afe251fb458e941bf0d5 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 12 May 2026 10:38:24 +0000 Subject: [PATCH 7/8] fix: rollback invalid artifact entries --- src/storage/artifactManifest.ts | 3 +-- test/unit/commands/record-export.test.ts | 1 - test/unit/commands/screenshot.test.ts | 3 +-- test/unit/commands/snapshot.test.ts | 1 - test/unit/snapshot/capture.test.ts | 4 ++-- test/unit/storage/artifactStorage.test.ts | 23 +++++++++++++++++++++++ 6 files changed, 27 insertions(+), 8 deletions(-) diff --git a/src/storage/artifactManifest.ts b/src/storage/artifactManifest.ts index 974ec2f..f229db8 100644 --- a/src/storage/artifactManifest.ts +++ b/src/storage/artifactManifest.ts @@ -185,10 +185,9 @@ async function appendArtifact( ): Promise { const resolvedSessionDir = resolve(sessionDir); const expectedSessionId = sessionIdFromSessionDir(resolvedSessionDir); - const validatedEntry = validateArtifactEntry(entry, expectedSessionId); - await appendSerializer.run(resolvedSessionDir, async () => { try { + const validatedEntry = validateArtifactEntry(entry, expectedSessionId); const manifest = await readArtifactManifest(resolvedSessionDir); await writeArtifactManifest(resolvedSessionDir, { ...manifest, diff --git a/test/unit/commands/record-export.test.ts b/test/unit/commands/record-export.test.ts index 3c38814..f6900c8 100644 --- a/test/unit/commands/record-export.test.ts +++ b/test/unit/commands/record-export.test.ts @@ -65,7 +65,6 @@ vi.mock('../../../src/storage/sessionPaths.js', () => ({ vi.mock('../../../src/storage/artifactManifest.js', () => ({ appendArtifactWithRollback: mocks.appendArtifactWithRollback, - appendArtifact: mocks.appendArtifact, createArtifactEntry: mocks.createArtifactEntry, })); diff --git a/test/unit/commands/screenshot.test.ts b/test/unit/commands/screenshot.test.ts index 46fe244..1b236dd 100644 --- a/test/unit/commands/screenshot.test.ts +++ b/test/unit/commands/screenshot.test.ts @@ -55,7 +55,6 @@ vi.mock('../../../src/replay/offlineReplay.js', () => ({ vi.mock('../../../src/storage/artifactManifest.js', () => ({ appendArtifactWithRollback: mocks.appendArtifactWithRollback, - appendArtifact: mocks.appendArtifact, createArtifactEntry: mocks.createArtifactEntry, })); @@ -544,7 +543,7 @@ describe('screenshot command', () => { { force: true }, ); expect(mocks.rename).not.toHaveBeenCalled(); - expect(mocks.appendArtifact).not.toHaveBeenCalled(); + expect(mocks.appendArtifactWithRollback).not.toHaveBeenCalled(); expect(mocks.emitSuccess).not.toHaveBeenCalled(); }); diff --git a/test/unit/commands/snapshot.test.ts b/test/unit/commands/snapshot.test.ts index 8ce29d9..75393f5 100644 --- a/test/unit/commands/snapshot.test.ts +++ b/test/unit/commands/snapshot.test.ts @@ -36,7 +36,6 @@ vi.mock('../../../src/replay/offlineReplay.js', () => ({ vi.mock('../../../src/storage/artifactManifest.js', () => ({ appendArtifactWithRollback: mocks.appendArtifactWithRollback, - appendArtifact: mocks.appendArtifact, createArtifactEntry: mocks.createArtifactEntry, })); diff --git a/test/unit/snapshot/capture.test.ts b/test/unit/snapshot/capture.test.ts index d0aa3ae..d005555 100644 --- a/test/unit/snapshot/capture.test.ts +++ b/test/unit/snapshot/capture.test.ts @@ -155,8 +155,8 @@ describe('snapshot capture', () => { const snapshotPath = artifactPath(sessionDirectory, filename); const manifestFilePath = artifactPath(sessionDirectory, 'manifest.json'); // Pre-write a manifest whose sessionId does not match the directory so - // `appendArtifact` raises a MANIFEST_VALIDATION_ERROR after the snapshot - // artifact has already been written. + // `appendArtifactWithRollback` raises a MANIFEST_VALIDATION_ERROR after + // the snapshot artifact has already been written. const unrelatedManifest = { version: 1, sessionId: 'unrelated-session', diff --git a/test/unit/storage/artifactStorage.test.ts b/test/unit/storage/artifactStorage.test.ts index 4dbfec4..aa27093 100644 --- a/test/unit/storage/artifactStorage.test.ts +++ b/test/unit/storage/artifactStorage.test.ts @@ -266,6 +266,29 @@ describe('artifact manifest storage', () => { }); }); + it('removes rollback artifact paths when artifact entry validation fails', async () => { + const sessionDir = await createSessionDir(); + const artifactFile = artifactPath(sessionDir, 'invalid-entry.json'); + + await ensureArtifactsDir(sessionDir); + await writeFile(artifactFile, 'artifact', 'utf8'); + + await expect( + appendArtifactWithRollback({ + sessionDir, + entry: createArtifactEntry({ + filename: 'invalid-entry.json', + sessionId: 'other-session', + }), + rollbackArtifactPath: artifactFile, + }), + ).rejects.toMatchObject({ code: 'MANIFEST_VALIDATION_ERROR' }); + + await expect(access(artifactFile)).rejects.toMatchObject({ + code: 'ENOENT', + }); + }); + it('preserves artifact paths when rollback is not requested', async () => { const sessionDir = await createSessionDir(); const artifactFile = artifactPath(sessionDir, 'explicit-out.cast'); From 5365793961ca94f9ca9400f88efa8294a5042931 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 12 May 2026 10:55:38 +0000 Subject: [PATCH 8/8] docs: update screenshot rollback comment --- test/unit/screenshot/capture.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/screenshot/capture.test.ts b/test/unit/screenshot/capture.test.ts index a65a762..a5bf8e0 100644 --- a/test/unit/screenshot/capture.test.ts +++ b/test/unit/screenshot/capture.test.ts @@ -389,8 +389,8 @@ describe('screenshot capture', () => { const finalFilename = screenshotFilename(5, 'reference-dark'); const finalPath = artifactPath(sessionDirectory, finalFilename); // Pre-write a manifest whose sessionId does not match the directory so - // `appendArtifact` raises a MANIFEST_VALIDATION_ERROR after the temp file - // has already been renamed into place. + // `appendArtifactWithRollback` raises a MANIFEST_VALIDATION_ERROR after + // the temp file has already been renamed into place. const manifestFilePath = artifactPath(sessionDirectory, 'manifest.json'); const unrelatedManifest = { version: 1,