项目文件夹

文件
Nebrass Lamouchi ba80096521 fix: handle screencast file extensions case-insensitively and clean up temp dir on failure (#2207)
Fixes #2206

### Problem

`screencast_start` matched the requested file extension with a
**case-sensitive** `endsWith()` against `['.webm', '.mp4']` and
**silently fell back to `.mp4`** when nothing matched. Combined with
`ensureExtension()` (which replaces the extension), a request for
`demo.WEBM` was recorded as **MP4** to **`demo.mp4`** — a different
format *and* path than requested — and any unsupported extension (e.g.
`recording.avi`) silently became `.mp4`.

Separately, when `screencast_start` is called without a `filePath`, it
creates a temp directory via `mkdtemp()`. If `page.screencast()` then
throws (e.g. ffmpeg missing), that directory was leaked.

### Changes

Two commits:

1. **`fix: match screencast extension case-insensitively and reject
unsupported ones`** — match via `path.extname().toLowerCase()`; reject
an explicitly requested but unsupported extension with an explicit error
listing the supported formats; a missing extension still defaults to
`.mp4`.
2. **`fix: clean up screencast temp directory when recording fails to
start`** — remove the generated temp dir in the `catch` handler, but
only when we own the generated path (never when the caller supplied
`filePath`).

| requested       | before          | after          |
| --------------- | --------------- | -------------- |
| `demo.WEBM`     | mp4 → `demo.mp4`| webm → `demo.webm` |
| `recording.avi` | mp4 → `recording.mp4` | error (rejected) |
| `demo.webm`     | webm → `demo.webm` | unchanged |
| *(no filePath)* | mp4 temp        | unchanged      |

The matched extension is normalized to lower case (`demo.WEBM` →
`demo.webm`).

### Testing

Added three regression tests to `tests/tools/screencast.test.ts` using
the existing `sinon`/`withMcpContext` harness. Verified locally against
Chrome for Testing 149 (`PUPPETEER_EXECUTABLE_PATH`):

- With the fix reverted, the two extension tests fail (uppercase `.WEBM`
→ mp4, `.avi` not rejected) and the cleanup test fails (temp dir left
behind) — i.e. they fail for the right reason.
- With the fix applied, the full `screencast.test.ts` suite passes
(11/11).
- `tsc --noEmit` and `npm run check-format` (eslint + prettier) are
clean.

> Note: I ran the `screencast` test file (which stubs `page.screencast`)
plus typecheck/lint locally; the rest of the browser-based suite I left
to CI.

### Notes for reviewers

- I chose to **`throw`** for an unsupported explicit extension
(consistent with the ffmpeg-missing `throw` in the same handler and with
the issue's "reject with an explicit error"). Happy to switch to the
softer `appendResponseLine(...) + return` style used by the in-progress
guard if you'd prefer.
- The two commits are independent and can be split if you'd rather take
them separately.
- I left the pre-existing `as \`${string}.webm\`` assertion on
`resolvedPath` untouched to keep the diff focused, though it's slightly
misleading now that the default is `.mp4`.

---------

Co-authored-by: Nicholas Roscino <nroscino@google.com>
2026-06-18 08:16:02 +00:00

291 行
9.2 KiB
TypeScript

/**
* @license
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
import assert from 'node:assert';
import fs from 'node:fs/promises';
import path from 'node:path';
import {describe, it, afterEach} from 'node:test';
import sinon from 'sinon';
import type {ParsedArguments} from '../../src/bin/chrome-devtools-mcp-cli-options.js';
import {startScreencast, stopScreencast} from '../../src/tools/screencast.js';
import {withMcpContext} from '../utils.js';
function createMockRecorder() {
return {
stop: sinon.stub().resolves(),
};
}
describe('screencast', () => {
afterEach(() => {
sinon.restore();
});
describe('screencast_start', () => {
it('starts a screencast recording with filePath', async () => {
await withMcpContext(async (response, context) => {
const mockRecorder = createMockRecorder();
const selectedPage = context.getSelectedPptrPage();
const screencastStub = sinon
.stub(selectedPage, 'screencast')
.resolves(mockRecorder as never);
await startScreencast().handler(
{
params: {filePath: '/tmp/test-recording.mp4'},
page: context.getSelectedMcpPage(),
},
response,
context,
);
sinon.assert.calledOnce(screencastStub);
const callArgs = screencastStub.firstCall.args[0];
assert.ok(callArgs);
assert.ok(callArgs.path?.endsWith('test-recording.mp4'));
assert.ok(context.getScreenRecorder() !== null);
assert.ok(
response.responseLines
.join('\n')
.includes('Screencast recording started'),
);
});
});
it('records WebM for an uppercase extension (case-insensitive)', async () => {
await withMcpContext(async (response, context) => {
const mockRecorder = createMockRecorder();
const selectedPage = context.getSelectedPptrPage();
const screencastStub = sinon
.stub(selectedPage, 'screencast')
.resolves(mockRecorder as never);
await startScreencast().handler(
{
params: {filePath: '/tmp/test-recording.WEBM'},
page: context.getSelectedMcpPage(),
},
response,
context,
);
sinon.assert.calledOnce(screencastStub);
const callArgs = screencastStub.firstCall.args[0];
assert.ok(callArgs);
assert.strictEqual(callArgs.format, 'webm');
assert.ok(callArgs.path?.endsWith('.webm'));
});
});
it('rejects an unsupported extension instead of silently using mp4', async () => {
await withMcpContext(async (response, context) => {
const selectedPage = context.getSelectedPptrPage();
const screencastStub = sinon.stub(selectedPage, 'screencast');
await assert.rejects(
startScreencast().handler(
{
params: {filePath: '/tmp/recording.avi'},
page: context.getSelectedMcpPage(),
},
response,
context,
),
/Unsupported screencast file extension/,
);
sinon.assert.notCalled(screencastStub);
assert.strictEqual(context.getScreenRecorder(), null);
});
});
it('starts a screencast recording with temp file when no filePath', async () => {
await withMcpContext(async (response, context) => {
const mockRecorder = createMockRecorder();
const selectedPage = context.getSelectedPptrPage();
const screencastStub = sinon
.stub(selectedPage, 'screencast')
.resolves(mockRecorder as never);
await startScreencast().handler(
{params: {}, page: context.getSelectedMcpPage()},
response,
context,
);
sinon.assert.calledOnce(screencastStub);
const callArgs = screencastStub.firstCall.args[0];
assert.ok(callArgs);
assert.ok(callArgs.path?.endsWith('.mp4'));
assert.ok(context.getScreenRecorder() !== null);
});
});
it('errors if a recording is already active', async () => {
await withMcpContext(async (response, context) => {
const mockRecorder = createMockRecorder();
context.setScreenRecorder({
recorder: mockRecorder as never,
filePath: '/tmp/existing.mp4',
});
const selectedPage = context.getSelectedPptrPage();
const screencastStub = sinon.stub(selectedPage, 'screencast');
await startScreencast().handler(
{params: {}, page: context.getSelectedMcpPage()},
response,
context,
);
sinon.assert.notCalled(screencastStub);
assert.ok(
response.responseLines
.join('\n')
.includes('a screencast recording is already in progress'),
);
});
});
it('provides a clear error when ffmpeg is not found', async () => {
await withMcpContext(async (response, context) => {
const selectedPage = context.getSelectedPptrPage();
const error = new Error('spawn ffmpeg ENOENT');
sinon.stub(selectedPage, 'screencast').rejects(error);
await assert.rejects(
startScreencast().handler(
{
params: {filePath: '/tmp/test.mp4'},
page: context.getSelectedMcpPage(),
},
response,
context,
),
/ffmpeg is required for screencast recording/,
);
assert.strictEqual(context.getScreenRecorder(), null);
});
});
it('cleans up the generated temp directory if recording fails to start', async () => {
await withMcpContext(async (response, context) => {
const selectedPage = context.getSelectedPptrPage();
const screencastStub = sinon
.stub(selectedPage, 'screencast')
.rejects(new Error('spawn ffmpeg ENOENT'));
await assert.rejects(
startScreencast().handler(
{params: {}, page: context.getSelectedMcpPage()},
response,
context,
),
/ffmpeg is required for screencast recording/,
);
// The temp directory generateTempFilePath() created must be removed.
const tempPath = screencastStub.firstCall.args[0]?.path as string;
assert.ok(tempPath);
await assert.rejects(fs.access(path.dirname(tempPath)));
assert.strictEqual(context.getScreenRecorder(), null);
});
});
it('passes ffmpegPath from args to puppeteer', async () => {
await withMcpContext(async (response, context) => {
const mockRecorder = createMockRecorder();
const selectedPage = context.getSelectedPptrPage();
const screencastStub = sinon
.stub(selectedPage, 'screencast')
.resolves(mockRecorder as never);
const experimentalFfmpegPath = '/custom/path/to/ffmpeg';
await startScreencast({
experimentalFfmpegPath,
} as ParsedArguments).handler(
{params: {}, page: context.getSelectedMcpPage()},
response,
context,
);
sinon.assert.calledOnce(screencastStub);
const callArgs = screencastStub.firstCall.args[0];
assert.strictEqual(callArgs?.ffmpegPath, experimentalFfmpegPath);
});
});
});
describe('screencast_stop', () => {
it('returns an error message if no recording is active', async () => {
await withMcpContext(async (response, context) => {
assert.strictEqual(context.getScreenRecorder(), null);
await stopScreencast.handler(
{params: {}, page: context.getSelectedMcpPage()},
response,
context,
);
assert.ok(
response.responseLines
.join('\n')
.includes('no active screencast recording to stop'),
);
});
});
it('stops an active recording and reports the file path', async () => {
await withMcpContext(async (response, context) => {
const mockRecorder = createMockRecorder();
const filePath = '/tmp/test-recording.mp4';
context.setScreenRecorder({
recorder: mockRecorder as never,
filePath,
});
await stopScreencast.handler(
{params: {}, page: context.getSelectedMcpPage()},
response,
context,
);
sinon.assert.calledOnce(mockRecorder.stop);
assert.strictEqual(context.getScreenRecorder(), null);
assert.ok(
response.responseLines
.join('\n')
.includes('stopped and saved to /tmp/test-recording.mp4'),
);
});
});
it('clears the recorder even if stop() throws', async () => {
await withMcpContext(async (response, context) => {
const mockRecorder = createMockRecorder();
mockRecorder.stop.rejects(new Error('ffmpeg process error'));
context.setScreenRecorder({
recorder: mockRecorder as never,
filePath: '/tmp/test.mp4',
});
await assert.rejects(
stopScreencast.handler(
{params: {}, page: context.getSelectedMcpPage()},
response,
context,
),
/ffmpeg process error/,
);
assert.strictEqual(context.getScreenRecorder(), null);
});
});
});
});