Skip to content

fix(ego-browser): report an actionable error when ffmpeg is a batch shim - #251

Open
Hotragn wants to merge 1 commit into
citrolabs:devfrom
Hotragn:fix/ffmpeg-batch-shim
Open

fix(ego-browser): report an actionable error when ffmpeg is a batch shim#251
Hotragn wants to merge 1 commit into
citrolabs:devfrom
Hotragn:fix/ffmpeg-batch-shim

Conversation

@Hotragn

@Hotragn Hotragn commented Aug 11, 2026

Copy link
Copy Markdown

Summary

Screencast recording fails with a bare Error: spawn EINVAL — no mention of ffmpeg, the path, or a remedy — whenever EGO_BROWSER_FFMPEG_PATH points at a .cmd/.bat shim, which is how Chocolatey, npm wrappers, and several scoop configurations install ffmpeg on Windows. The failure lands on the exact remediation path the existing error message recommends, and it leaves the recorder half-constructed so the follow-up stop() reports a second, invented error: ffmpeg exited with code undefined.

Root cause: Node refuses to launch a batch file without a shell (the CVE-2024-27980 mitigation) and reports it by throwing EINVAL synchronously out of spawn(). VideoRecorder.start() assigns this._process = spawnProcess(...) outside any try/catch, so that throw bypasses the ENOENT-to-guidance mapping that handles the process "error" event, and this._process / this._exitPromise are never assigned.

Related issue

No existing issue — found while checking whether page.screencast works on Windows, in the same Windows-readiness sweep as #223/#224/#226/#228.

Reproduction

Windows 11, Node 24, against the built bundle:

> ffmpegPath = C:\...\ffmpeg.cmd   (any batch shim; contents irrelevant)
before:  start FAILED -> Error | spawn EINVAL
         stop  FAILED -> Error | ffmpeg exited with code undefined
after:   start FAILED -> Error | FFmpeg path "C:\...\ffmpeg.cmd" is a batch shim, which cannot be
                          launched directly. Point EGO_BROWSER_FFMPEG_PATH at the ffmpeg executable itself.
         stop  OK

Changes

  • src/video-recorder.ts — the spawn() call is wrapped so a synchronous failure is routed through the same mapping as the async "error" event. Both paths now call one startupError() helper: ENOENT keeps today's exact wording, EINVAL on a .cmd/.bat target explains the batch-shim limitation and names the offending path, and any other failure propagates unchanged.
  • _stop() returns early (after cleaning up the temp path) when no process was ever created, so a failed start reports only its own cause instead of a fabricated exit code.
  • src/video-recorder.test.mjs — 4 tests: batch-shim guidance, synchronous ENOENT, unrecognized codes passing through untouched, and stop() after a failed start staying quiet.

Deliberately not doing: re-spawning batch shims through shell: true. That would put a user-supplied path and the filter string (commas, colons, parentheses) through a shell parser on Windows for a marginal convenience — the executable is one setting away, and this repo prefers the smaller change.

Verification

Run from package/ego-browser (Windows 11, Node 24; the new tests inject spawnProcess, so they are platform-neutral and assert the same behavior on any OS):

npm test                      -> 315 pass, 0 fail (311 existing + 4 new)
npm run typecheck             -> ok
npm run validate:site-skills  -> site skills ok
npm audit --audit-level=moderate -> 0 vulnerabilities

Revert-check: with src/video-recorder.ts reverted and the new tests kept, "explains a batch shim ffmpeg path rejected by the OS", "reports a missing executable thrown synchronously", and "stop after a failed start does not invent an exit code" all fail (11 pass / 3 fail); with the fix, 15 pass.

Impact

  • Public helper API or behavior
  • Agent skill or instructions
  • Site learning
  • Installation or update flow
  • Build, CI, or release process
  • Documentation only
  • No externally visible impact

Error reporting only — no successful recording changes behavior. The ENOENT message is byte-identical to today's (its existing test is untouched and still passes), so nothing that matches on that string breaks. stop() after a successful start is unchanged; only the never-started case is newly quiet.

Checklist

  • The PR targets the correct base branch (dev for normal changes; only dev may target main).
  • The change is focused and does not include unrelated cleanup.
  • Tests were added or updated for behavior changes, or the reason they are unnecessary is explained above.
  • Relevant tests and validation commands pass locally.
  • Public helper JSDoc and agent-facing documentation are updated when the helper surface changes.
  • No credentials, tokens, cookies, personal data, or other secrets are included.
  • A release-note label is selected (fix).

@Hotragn

Hotragn commented Aug 11, 2026

Copy link
Copy Markdown
Author

Flagging this myself so it does not sit here consuming review attention: I believe this PR is superseded on the sprint-1.3.0 line.

That branch adds playwright-core@1.52.0 as a runtime dependency and no longer contains src/video-recorder.ts at all. With Playwright's native video recording there is no spawn("ffmpeg") to harden, so the EINVAL-on-batch-shim failure this fixes cannot occur there.

It remains a real defect on dev, where src/video-recorder.ts still spawns ffmpeg directly and a .cmd/.bat path surfaces as a bare spawn EINVAL (plus a misleading ffmpeg exited with code undefined from stop()), so I have left the PR open against the base CONTRIBUTING documents rather than closing it on my own judgement. Say the word and I will close it, or I will close it once the sprint work reaches dev/main.

#224 and #252 are in the same position (src/driver/downloads.ts, src/driver/screencast.ts — both absent on sprint-1.3.0).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant