chromedevtools--chrome-devtools-mcp
Nebrass Lamouchi
ba80096521
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>