From 5e558272b011b873cfda43a4350072855d15a955 Mon Sep 17 00:00:00 2001 From: natew Date: Sun, 25 Oct 2026 00:42:08 -1000 Subject: [PATCH 1/2] fix(record_sim_video): reject incomplete recordings Report force-killed stops and MP4s without a complete moov box as failures instead of copying them as a successful recording. Process output and the source file are preserved. --- CHANGELOG.md | 6 ++ .../__tests__/record_sim_video.test.ts | 69 ++++++++++++++++ src/mcp/tools/simulator/record_sim_video.ts | 32 +++++++- src/utils/__tests__/video_capture.test.ts | 82 +++++++++++++++++++ src/utils/video-capture/index.ts | 2 + src/utils/video_capture.ts | 77 ++++++++++++++++- 6 files changed, 265 insertions(+), 3 deletions(-) create mode 100644 src/utils/__tests__/video_capture.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index f47e609ad..95ea194c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## [Unreleased] + +### Fixed + +- `record_sim_video` no longer reports success for an incomplete recording. A stop that needed a force-kill, or an MP4 with no `moov` box or a truncated box, now fails with the AXe output and the file left in place. + ## [2.7.1] ### Changed diff --git a/src/mcp/tools/simulator/__tests__/record_sim_video.test.ts b/src/mcp/tools/simulator/__tests__/record_sim_video.test.ts index 913c5d590..08974192a 100644 --- a/src/mcp/tools/simulator/__tests__/record_sim_video.test.ts +++ b/src/mcp/tools/simulator/__tests__/record_sim_video.test.ts @@ -39,6 +39,7 @@ describe('record_sim_video tool - validation', () => { describe('record_sim_video logic - start behavior', () => { it('starts with default fps (30) and warns when outputFile is provided on start (ignored)', async () => { const video: any = { + checkMp4Finalized: async () => ({ finalized: true }), startSimulatorVideoCapture: async () => ({ started: true, sessionId: 'sess-123', @@ -86,6 +87,7 @@ describe('record_sim_video logic - start behavior', () => { it('keeps start failure summary short and preserves diagnostics', async () => { const video: any = { + checkMp4Finalized: async () => ({ finalized: true }), startSimulatorVideoCapture: async () => ({ started: false, error: 'AXe stderr: permission denied', @@ -124,6 +126,7 @@ describe('record_sim_video logic - start behavior', () => { describe('record_sim_video logic - end-to-end stop with rename', () => { it('stops, parses stdout path, and renames to outputFile', async () => { const video: any = { + checkMp4Finalized: async () => ({ finalized: true }), startSimulatorVideoCapture: async () => ({ started: true, sessionId: 'sess-abc', @@ -181,6 +184,7 @@ describe('record_sim_video logic - end-to-end stop with rename', () => { it('keeps missing parsed path summary short and preserves raw AXe output', async () => { const video: any = { + checkMp4Finalized: async () => ({ finalized: true }), startSimulatorVideoCapture: async () => ({ started: true, sessionId: 'sess-abc', @@ -226,6 +230,7 @@ describe('record_sim_video logic - version gate', () => { }; const video: any = { + checkMp4Finalized: async () => ({ finalized: true }), startSimulatorVideoCapture: async () => ({ started: true, sessionId: 'sess-xyz', @@ -256,3 +261,67 @@ describe('record_sim_video logic - version gate', () => { expect(text).toContain('AXe v1.1.0'); }); }); + +describe('record_sim_video logic - incomplete recordings', () => { + const axe = { + areAxeToolsAvailable: () => true, + isAxeAtLeastVersion: async () => true, + }; + + async function stopWith(stopResult: object, finalization: object) { + const copied: string[] = []; + const fs = createMockFileSystemExecutor({ + cp: async (source: string) => { + copied.push(source); + }, + }); + const video: any = { + startSimulatorVideoCapture: async () => ({ started: true, sessionId: 's' }), + stopSimulatorVideoCapture: async () => ({ stopped: true, ...stopResult }), + checkMp4Finalized: async () => finalization, + }; + const { result, run } = createMockToolHandlerContext(); + await run(() => + record_sim_videoLogic( + { simulatorId: VALID_SIM_ID, stop: true, outputFile: '/var/videos/final.mp4' } as any, + DUMMY_EXECUTOR, + axe, + video, + fs, + ), + ); + return { result, copied }; + } + + it('fails without copying when the recording has no moov box', async () => { + const { result, copied } = await stopWith( + { parsedPath: '/tmp/recorded.mp4', stdout: 'Saved /tmp/recorded.mp4', exitCode: 0 }, + { finalized: false, reason: 'no moov box found' }, + ); + expect(result.isError()).toBe(true); + expect(copied).toEqual([]); + const text = result.text(); + expect(text).toContain('incomplete'); + expect(text).toContain('no moov box found'); + expect(text).toContain('Saved /tmp/recorded.mp4'); + }); + + it('fails when AXe had to be force-killed even if the file looks finalized', async () => { + const { result, copied } = await stopWith( + { parsedPath: '/tmp/recorded.mp4', stdout: 'Saved /tmp/recorded.mp4', forcedKill: true }, + { finalized: true }, + ); + expect(result.isError()).toBe(true); + expect(copied).toEqual([]); + expect(result.text()).toContain('force-killed'); + }); + + it('copies a finalized recording', async () => { + const { result, copied } = await stopWith( + { parsedPath: '/tmp/recorded.mp4', stdout: 'Saved /tmp/recorded.mp4', exitCode: 0 }, + { finalized: true }, + ); + expect(result.isError()).toBe(false); + expect(copied).toEqual(['/tmp/recorded.mp4']); + }); +}); diff --git a/src/mcp/tools/simulator/record_sim_video.ts b/src/mcp/tools/simulator/record_sim_video.ts index 805a8e508..b5e500a8d 100644 --- a/src/mcp/tools/simulator/record_sim_video.ts +++ b/src/mcp/tools/simulator/record_sim_video.ts @@ -14,6 +14,7 @@ import { AXE_NOT_AVAILABLE_MESSAGE, } from '../../../utils/axe/index.ts'; import { + checkMp4Finalized, startSimulatorVideoCapture, stopSimulatorVideoCapture, } from '../../../utils/video-capture/index.ts'; @@ -106,6 +107,7 @@ export function createRecordSimVideoExecutor( video: { startSimulatorVideoCapture: typeof startSimulatorVideoCapture; stopSimulatorVideoCapture: typeof stopSimulatorVideoCapture; + checkMp4Finalized: typeof checkMp4Finalized; }, fs: FileSystemExecutor, ): NonStreamingExecutor { @@ -173,10 +175,36 @@ export function createRecordSimVideoExecutor( }); } + const rawOutput = stopRes.stdout ?? '(no output captured)'; + if (stopRes.parsedPath) { + const processOutcome = `exit code: ${stopRes.exitCode ?? 'none'}, signal: ${stopRes.signal ?? 'none'}`; + let incompleteReason: string | undefined; + if (stopRes.forcedKill) { + incompleteReason = 'AXe did not exit after the stop signal and was force-killed'; + } else { + try { + const finalization = await video.checkMp4Finalized(stopRes.parsedPath); + if (!finalization.finalized) { + incompleteReason = finalization.reason; + } + } catch (error) { + incompleteReason = `could not inspect ${stopRes.parsedPath}: ${error instanceof Error ? error.message : String(error)}`; + } + } + if (incompleteReason) { + return createRecordSimVideoResult({ + simulatorId: params.simulatorId, + didError: true, + error: 'Recording stopped but the video file is incomplete.', + diagnosticsMessage: `${incompleteReason} (${processOutcome}). Left in place: ${stopRes.parsedPath}. Raw output: ${rawOutput}`, + }); + } + } + try { if (params.outputFile) { if (!stopRes.parsedPath) { - const diagnosticMessage = `Recording stopped but could not determine the recorded file path from AXe output. Raw output: ${stopRes.stdout ?? '(no output captured)'}`; + const diagnosticMessage = `Recording stopped but could not determine the recorded file path from AXe output. Raw output: ${rawOutput}`; return createRecordSimVideoResult({ simulatorId: params.simulatorId, didError: true, @@ -228,9 +256,11 @@ export async function record_sim_videoLogic( video: { startSimulatorVideoCapture: typeof startSimulatorVideoCapture; stopSimulatorVideoCapture: typeof stopSimulatorVideoCapture; + checkMp4Finalized: typeof checkMp4Finalized; } = { startSimulatorVideoCapture, stopSimulatorVideoCapture, + checkMp4Finalized, }, fs: FileSystemExecutor = getDefaultFileSystemExecutor(), ): Promise { diff --git a/src/utils/__tests__/video_capture.test.ts b/src/utils/__tests__/video_capture.test.ts new file mode 100644 index 000000000..c6c1c1160 --- /dev/null +++ b/src/utils/__tests__/video_capture.test.ts @@ -0,0 +1,82 @@ +import { describe, expect, it } from 'vitest'; +import { mkdtempSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { spawn } from 'node:child_process'; +import { + checkMp4Finalized, + startSimulatorVideoCapture, + stopSimulatorVideoCapture, +} from '../video_capture.ts'; + +function box(type: string, payload: Buffer): Buffer { + const header = Buffer.alloc(8); + header.writeUInt32BE(8 + payload.length, 0); + header.write(type, 4, 'latin1'); + return Buffer.concat([header, payload]); +} + +function writeMp4(parts: Buffer[]): string { + const path = join(mkdtempSync(join(tmpdir(), 'mp4-')), 'a.mp4'); + writeFileSync(path, Buffer.concat(parts)); + return path; +} + +describe('checkMp4Finalized', () => { + const ftyp = box('ftyp', Buffer.from('isom0000', 'latin1')); + const mdat = box('mdat', Buffer.alloc(1000, 1)); + const moov = box('moov', Buffer.alloc(100, 2)); + + it('accepts moov after mdat and moov before mdat', async () => { + expect(await checkMp4Finalized(writeMp4([ftyp, mdat, moov]))).toEqual({ finalized: true }); + expect(await checkMp4Finalized(writeMp4([ftyp, moov, mdat]))).toEqual({ finalized: true }); + }); + + it('rejects a recording with no moov box', async () => { + const result = await checkMp4Finalized(writeMp4([ftyp, mdat])); + expect(result).toEqual({ finalized: false, reason: 'no moov box found' }); + }); + + it('rejects a truncated mdat and a truncated moov', async () => { + const cutMdat = await checkMp4Finalized(writeMp4([ftyp, mdat.subarray(0, 500)])); + expect(cutMdat.finalized).toBe(false); + const cutMoov = await checkMp4Finalized(writeMp4([ftyp, mdat, moov.subarray(0, 50)])); + expect(cutMoov.finalized).toBe(false); + }); + + it('rejects an empty file', async () => { + expect((await checkMp4Finalized(writeMp4([]))).finalized).toBe(false); + }); +}); + +describe('stopSimulatorVideoCapture process outcome', () => { + const helpers = { getAxePath: () => '/axe', getBundledAxeEnvironment: () => ({}) }; + const executorFor = (script: string) => + (async () => ({ + success: true, + output: '', + process: spawn(process.execPath, ['-e', script]), + })) as never; + + it('reports a child that ignores the stop signal as force-killed with its output', async () => { + const script = + "process.on('SIGINT',()=>{});console.log('Saved /tmp/t.mp4');setInterval(()=>{},1000)"; + await startSimulatorVideoCapture({ simulatorUuid: 'sim-kill' }, executorFor(script), helpers); + await new Promise((resolve) => setTimeout(resolve, 300)); + const result = await stopSimulatorVideoCapture({ simulatorUuid: 'sim-kill' }, null as never); + expect(result).toMatchObject({ + stopped: true, + forcedKill: true, + parsedPath: '/tmp/t.mp4', + }); + }, 15000); + + it('reports a child that exits on the stop signal as not force-killed', async () => { + const script = + "process.on('SIGINT',()=>process.exit(0));console.log('Saved /tmp/t.mp4');setInterval(()=>{},1000)"; + await startSimulatorVideoCapture({ simulatorUuid: 'sim-ok' }, executorFor(script), helpers); + await new Promise((resolve) => setTimeout(resolve, 300)); + const result = await stopSimulatorVideoCapture({ simulatorUuid: 'sim-ok' }, null as never); + expect(result).toMatchObject({ stopped: true, forcedKill: false, exitCode: 0 }); + }, 15000); +}); diff --git a/src/utils/video-capture/index.ts b/src/utils/video-capture/index.ts index 3880d2606..6b4cb9c61 100644 --- a/src/utils/video-capture/index.ts +++ b/src/utils/video-capture/index.ts @@ -1,7 +1,9 @@ export { startSimulatorVideoCapture, stopSimulatorVideoCapture, + checkMp4Finalized, type AxeHelpers, + type Mp4FinalizationResult, type StartVideoCaptureResult, type StopVideoCaptureResult, } from '../video_capture.ts'; diff --git a/src/utils/video_capture.ts b/src/utils/video_capture.ts index 30208c07d..b2ecb1aa5 100644 --- a/src/utils/video_capture.ts +++ b/src/utils/video_capture.ts @@ -5,6 +5,7 @@ * It aggregates stdout/stderr to parse the generated MP4 path on stop. */ +import { open } from 'node:fs/promises'; import type { ChildProcess } from 'child_process'; import { log } from './logging/index.ts'; import { getAxePath, getBundledAxeEnvironment } from './axe-helpers.ts'; @@ -17,6 +18,7 @@ type Session = { startedAt: number; buffer: string; ended: boolean; + forcedKill?: boolean; releaseActivity?: () => void; }; @@ -33,7 +35,15 @@ export type StartVideoCaptureResult = | { started: false; error: string }; export type StopVideoCaptureResult = - | { stopped: true; sessionId: string; stdout?: string; parsedPath?: string } + | { + stopped: true; + sessionId: string; + stdout?: string; + parsedPath?: string; + forcedKill?: boolean; + exitCode?: number | null; + signal?: string | null; + } | { stopped: false; error: string }; function createTimeoutPromise(timeoutMs: number): Promise<'timed_out'> { @@ -72,6 +82,7 @@ async function waitForChildToStop(session: Session, timeoutMs: number): Promise< const outcome = await Promise.race([closePromise, createTimeoutPromise(timeoutMs)]); if (outcome === 'timed_out') { + session.forcedKill = true; try { child.kill('SIGKILL'); } catch { @@ -80,7 +91,14 @@ async function waitForChildToStop(session: Session, timeoutMs: number): Promise< } } -type StopSessionSuccess = { sessionId: string; stdout: string; parsedPath?: string }; +type StopSessionSuccess = { + sessionId: string; + stdout: string; + parsedPath?: string; + forcedKill: boolean; + exitCode: number | null; + signal: string | null; +}; type StopSessionError = { error: string }; type StopSessionResult = StopSessionSuccess | StopSessionError; @@ -117,6 +135,9 @@ async function stopSession( sessionId: session.sessionId, stdout: combinedOutput, parsedPath, + forcedKill: session.forcedKill === true, + exitCode: child?.exitCode ?? null, + signal: child?.signalCode ?? null, }; } @@ -265,9 +286,61 @@ export async function stopSimulatorVideoCapture( sessionId: result.sessionId, stdout: result.stdout, parsedPath: result.parsedPath, + forcedKill: result.forcedKill, + exitCode: result.exitCode, + signal: result.signal, }; } +export type Mp4FinalizationResult = { finalized: true } | { finalized: false; reason: string }; + +/** + * Walks the top-level MP4 boxes and requires a `moov` box with every declared box + * fully present, so a recording cut off before the writer finalized it is rejected. + */ +export async function checkMp4Finalized(path: string): Promise { + const handle = await open(path, 'r'); + try { + const size = (await handle.stat()).size; + const header = Buffer.alloc(16); + let offset = 0; + let sawMoov = false; + let sawFtyp = false; + + while (offset < size) { + const { bytesRead } = await handle.read(header, 0, 16, offset); + if (bytesRead < 8) { + return { finalized: false, reason: `truncated box header at byte ${offset}` }; + } + let boxSize = header.readUInt32BE(0); + const type = header.toString('latin1', 4, 8); + if (boxSize === 1) { + if (bytesRead < 16) { + return { finalized: false, reason: `truncated box header at byte ${offset}` }; + } + boxSize = Number(header.readBigUInt64BE(8)); + } else if (boxSize === 0) { + boxSize = size - offset; + } + if (boxSize < 8 || offset + boxSize > size) { + return { + finalized: false, + reason: `'${type}' box at byte ${offset} declares ${boxSize} bytes but the file ends at ${size}`, + }; + } + if (type === 'ftyp') sawFtyp = true; + if (type === 'moov') sawMoov = true; + offset += boxSize; + } + + if (!sawFtyp) return { finalized: false, reason: 'no ftyp box found' }; + if (!sawMoov) return { finalized: false, reason: 'no moov box found' }; + return { finalized: true }; + } finally { + await handle.close(); + } +} + export async function stopAllVideoCaptureSessions(timeoutMs = 1000): Promise<{ stoppedSessionCount: number; errorCount: number; From a37c7c2a3c4db113c856865404dcfe5067a6552e Mon Sep 17 00:00:00 2001 From: natew Date: Sun, 25 Oct 2026 00:45:47 -1000 Subject: [PATCH 2/2] fix(video-capture): respect extended box header size, await child readiness in tests --- src/utils/__tests__/video_capture.test.ts | 50 ++++++++++++++++++----- src/utils/video_capture.ts | 4 +- 2 files changed, 43 insertions(+), 11 deletions(-) diff --git a/src/utils/__tests__/video_capture.test.ts b/src/utils/__tests__/video_capture.test.ts index c6c1c1160..56fc2dd1e 100644 --- a/src/utils/__tests__/video_capture.test.ts +++ b/src/utils/__tests__/video_capture.test.ts @@ -44,6 +44,23 @@ describe('checkMp4Finalized', () => { expect(cutMoov.finalized).toBe(false); }); + it('rejects an extended-size box smaller than its own 16 byte header', async () => { + const extended = Buffer.alloc(16); + extended.writeUInt32BE(1, 0); + extended.write('moov', 4, 'latin1'); + extended.writeBigUInt64BE(8n, 8); + const result = await checkMp4Finalized(writeMp4([ftyp, extended])); + expect(result.finalized).toBe(false); + }); + + it('accepts a valid extended-size moov box', async () => { + const extended = Buffer.alloc(16); + extended.writeUInt32BE(1, 0); + extended.write('moov', 4, 'latin1'); + extended.writeBigUInt64BE(16n, 8); + expect(await checkMp4Finalized(writeMp4([ftyp, mdat, extended]))).toEqual({ finalized: true }); + }); + it('rejects an empty file', async () => { expect((await checkMp4Finalized(writeMp4([]))).finalized).toBe(false); }); @@ -51,18 +68,26 @@ describe('checkMp4Finalized', () => { describe('stopSimulatorVideoCapture process outcome', () => { const helpers = { getAxePath: () => '/axe', getBundledAxeEnvironment: () => ({}) }; - const executorFor = (script: string) => - (async () => ({ - success: true, - output: '', - process: spawn(process.execPath, ['-e', script]), - })) as never; + // resolves once the child has printed, which its scripts do after installing the SIGINT handler + const executorFor = (script: string, ready: { promise?: Promise }) => + (async () => { + const child = spawn(process.execPath, ['-e', script]); + ready.promise = new Promise((resolve) => { + child.stdout.once('data', () => resolve()); + }); + return { success: true, output: '', process: child }; + }) as never; it('reports a child that ignores the stop signal as force-killed with its output', async () => { const script = "process.on('SIGINT',()=>{});console.log('Saved /tmp/t.mp4');setInterval(()=>{},1000)"; - await startSimulatorVideoCapture({ simulatorUuid: 'sim-kill' }, executorFor(script), helpers); - await new Promise((resolve) => setTimeout(resolve, 300)); + const ready: { promise?: Promise } = {}; + await startSimulatorVideoCapture( + { simulatorUuid: 'sim-kill' }, + executorFor(script, ready), + helpers, + ); + await ready.promise; const result = await stopSimulatorVideoCapture({ simulatorUuid: 'sim-kill' }, null as never); expect(result).toMatchObject({ stopped: true, @@ -74,8 +99,13 @@ describe('stopSimulatorVideoCapture process outcome', () => { it('reports a child that exits on the stop signal as not force-killed', async () => { const script = "process.on('SIGINT',()=>process.exit(0));console.log('Saved /tmp/t.mp4');setInterval(()=>{},1000)"; - await startSimulatorVideoCapture({ simulatorUuid: 'sim-ok' }, executorFor(script), helpers); - await new Promise((resolve) => setTimeout(resolve, 300)); + const ready: { promise?: Promise } = {}; + await startSimulatorVideoCapture( + { simulatorUuid: 'sim-ok' }, + executorFor(script, ready), + helpers, + ); + await ready.promise; const result = await stopSimulatorVideoCapture({ simulatorUuid: 'sim-ok' }, null as never); expect(result).toMatchObject({ stopped: true, forcedKill: false, exitCode: 0 }); }, 15000); diff --git a/src/utils/video_capture.ts b/src/utils/video_capture.ts index b2ecb1aa5..de29a94c9 100644 --- a/src/utils/video_capture.ts +++ b/src/utils/video_capture.ts @@ -313,16 +313,18 @@ export async function checkMp4Finalized(path: string): Promise size) { + if (boxSize < headerSize || offset + boxSize > size) { return { finalized: false, reason: `'${type}' box at byte ${offset} declares ${boxSize} bytes but the file ends at ${size}`,